From 221eac2a53de4aef52b1f7828bd79521c8d07fad Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Fri, 18 Sep 2026 16:06:20 -0400 Subject: [PATCH] fix(#1938): add the builds read path, and stop flags from eating positionals Three defects, all of which made a build's data look absent rather than unreachable. `butter builds show ` printed nothing and exited 0. The subcommand was never wired into the dispatcher, so it fell through the switch and out of main(). An exit 0 with no output reads as "this build has no data", which is why it was reported as a backend problem - the endpoint existed and worked the whole time. Unknown subcommands now fail loudly instead of returning 0. `butter builds investigate --json ` appeared to print nothing while the plain form rendered fine. parseArgs treated every `--flag` as taking a value whenever the next argv entry did not start with `-`, so `--json` before a positional parsed as `json: ""` and left no build id behind; the command printed its usage error and exited 1. Position in the command line must not change what a boolean flag means, so switches are now declared, and `--key=value` is honored for any key. There was no GET counterpart to `investigate`. Checking whether an investigation had finished meant POSTing again, and each POST is a paid AI call. `butter builds investigation ` reads the result for free, `builds show` embeds it, and `investigate` now points at the read path rather than letting callers poll by re-POSTing. `builds show` also distinguishes three cases a single falsy check used to collapse: an investigation exists, none has been run, and the server predates the read path. Reporting an old server as "no investigation" is the exact false negative that sent people back to POSTing to find out. Attribution prints "unattributed" when the investigation made none, rather than omitting the line. A failure caused by a file that was never submitted has no offending changelist and no culpable author, and that is a real answer. Minor version: four new behaviours, no breaking changes. Co-Authored-By: Claude Opus 5 --- bin/butter | 227 +++++++++++++++++++++++++++++++++++++++++--- package.json | 2 +- test/butter.test.js | 213 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 12 deletions(-) diff --git a/bin/butter b/bin/butter index 572951f..4bc0f53 100755 --- a/bin/butter +++ b/bin/butter @@ -540,6 +540,161 @@ async function buildsList(args) { } } +async function buildsShow(args) { + const projectId = args.project || loadConfig().defaultProject; + const buildId = args._[2]; + if (!projectId || !buildId) { + console.error(`${colors.err}✗ Usage: butter builds show --project ${colors.reset}`); + process.exit(1); + } + + try { + const res = await request("GET", `/api/v1/projects/${projectId}/build_runs/${buildId}`); + if (args.json) { + console.log(JSON.stringify(res, null, 2)); + return; + } + + const statusColor = + res.status === "completed" ? colors.ok : res.status === "failed" ? colors.err : colors.warn; + console.log(`\n${colors.bold}BUILD ${res.id}${colors.reset}`); + console.log("----------------------------------------------------------------------"); + console.log(` Status: ${statusColor}${String(res.status).toUpperCase()}${colors.reset}`); + console.log(` Overall: ${res.overall_status || "-"}`); + console.log(` Job: ${res.ci_job_name || "-"}`); + console.log(` Commit: ${res.commit_hash || "-"}`); + console.log(` Type: ${res.build_type || "-"}${res.target_type ? ` / ${res.target_type}` : ""}`); + console.log(` Duration: ${res.duration != null ? `${res.duration}s` : "-"}`); + console.log(` Created: ${res.created_at || "-"}`); + + // #1940: an investigation with no log to read cannot say anything useful, + // so surface that here rather than leaving it to be inferred from a + // vague diagnosis. + if (res.log_available === false) { + console.log(` Build log: ${colors.warn}not available${colors.reset}`); + if (res.log_unavailable_reason) console.log(` ${colors.muted}${res.log_unavailable_reason}${colors.reset}`); + } else if (res.log_available === true) { + console.log(` Build log: ${colors.ok}available${colors.reset}`); + } + + if (Array.isArray(res.steps) && res.steps.length) { + console.log(`\n${colors.bold} Steps:${colors.reset}`); + res.steps.forEach((s) => { + const c = s.status === "completed" ? colors.ok : s.status === "failed" ? colors.err : colors.warn; + console.log( + ` ${c}[${s.status}]${colors.reset} ${s.step_type}${s.message ? ` ${colors.muted}- ${s.message}${colors.reset}` : ""}` + ); + }); + } + + // Three distinct cases, and conflating them is how #1938 stayed confusing: + // an investigation exists; none has been run; or the server predates the + // read path and cannot tell us either way. A server without the key must + // not be reported as "no investigation" - that is the exact false negative + // that sent people back to POSTing `investigate` to find out. + if (res.investigation) { + console.log(""); + printInvestigation(res.investigation); + } else if (Object.prototype.hasOwnProperty.call(res, "investigation")) { + console.log(`\n ${colors.muted}No AI investigation has been run for this build.${colors.reset}`); + console.log(` ${colors.muted}Run: butter builds investigate ${res.id} --project ${projectId}${colors.reset}`); + } else { + console.log(`\n ${colors.warn}This ButterStack server does not expose investigations on the build detail.${colors.reset}`); + console.log(` ${colors.muted}Try: butter builds investigation ${res.id} --project ${projectId}${colors.reset}`); + } + console.log(""); + } catch (err) { + console.error(`${colors.err}✗ Failed to show build: ${err.message}${colors.reset}`); + process.exit(1); + } +} + +function printInvestigation(inv) { + const statusColor = + inv.status === "completed" ? colors.ok : inv.status === "failed" ? colors.err : colors.warn; + console.log(`${colors.bold}AI Failure Investigation:${colors.reset}`); + console.log(` Status: ${statusColor}${inv.status}${colors.reset}`); + + if (inv.status === "pending" || inv.status === "running") { + console.log(` ${colors.muted}Still in flight. Re-read with \`butter builds investigation\` - do not`); + console.log(` re-run \`investigate\`, which starts a new paid analysis.${colors.reset}`); + return; + } + + if (inv.status === "failed") { + console.log(` Error: ${colors.err}${inv.error_message || "unknown"}${colors.reset}`); + return; + } + + if (inv.diagnosis_category) { + console.log(` Category: ${inv.diagnosis_category}${inv.severity ? ` (${inv.severity})` : ""}`); + } + if (inv.confidence) console.log(` Confidence: ${inv.confidence}`); + if (inv.summary) console.log(` Summary: ${inv.summary}`); + if (inv.diagnosis) console.log(` Diagnosis: ${inv.diagnosis}`); + if (inv.suggested_fix) console.log(` Fix: ${colors.accent}${inv.suggested_fix}${colors.reset}`); + + // Attribution is printed only when the investigation actually made one. + // "unattributed" is a real, common and correct answer -- a file that was + // never submitted has no offending changelist and no culpable author. + if (inv.attributed_changelist || inv.attributed_user_id) { + console.log( + ` Attributed: ${inv.attributed_changelist ? `CL ${inv.attributed_changelist}` : ""}${ + inv.attributed_user_id ? ` (user ${inv.attributed_user_id})` : "" + }` + ); + } else { + console.log(` Attributed: ${colors.muted}unattributed${colors.reset}`); + } + + if (Array.isArray(inv.affected_files) && inv.affected_files.length) { + console.log(` Files: ${inv.affected_files.join(", ")}`); + } + if (Array.isArray(inv.evidence) && inv.evidence.length) { + console.log(`${colors.bold} Evidence:${colors.reset}`); + inv.evidence.forEach((e) => console.log(` ${colors.muted}- ${e}${colors.reset}`)); + } +} + +// Read-only counterpart to `investigate` (#1938). Before this existed, the +// only way to see a result was to POST investigate again, which spends +// credits and starts a fresh Bedrock run once the previous one has finished. +async function buildsInvestigation(args) { + const projectId = args.project || loadConfig().defaultProject; + const buildId = args._[2]; + if (!projectId || !buildId) { + console.error(`${colors.err}✗ Usage: butter builds investigation --project ${colors.reset}`); + process.exit(1); + } + + try { + const res = await request("GET", `/api/v1/projects/${projectId}/build_runs/${buildId}/investigation`); + if (args.json) { + console.log(JSON.stringify(res, null, 2)); + return; + } + console.log(""); + printInvestigation(res); + if (args.steps && Array.isArray(res.steps) && res.steps.length) { + console.log(`\n${colors.bold} Steps:${colors.reset}`); + res.steps.forEach((s) => { + console.log(` ${colors.muted}${String(s.step).padStart(3)} ${s.step_type}${colors.reset}${s.content ? ` ${s.content}` : ""}`); + }); + } else if (Array.isArray(res.steps) && res.steps.length) { + console.log(`\n ${colors.muted}${res.steps.length} step(s). Pass --steps to print the trail.${colors.reset}`); + } + console.log(""); + } catch (err) { + if (err.statusCode === 404) { + console.error(`${colors.warn}No investigation has been run for build ${buildId}.${colors.reset}`); + console.error(`${colors.muted}Start one with: butter builds investigate ${buildId} --project ${projectId}${colors.reset}`); + process.exit(1); + } + console.error(`${colors.err}✗ Failed to read investigation: ${err.message}${colors.reset}`); + process.exit(1); + } +} + async function buildsInvestigate(args) { const projectId = args.project || loadConfig().defaultProject; const buildId = args._[2]; @@ -549,17 +704,24 @@ async function buildsInvestigate(args) { } try { - console.log(`${colors.accent}Triggering AI Failure Investigation for Build #${buildId}...${colors.reset}`); + if (!args.json) { + console.log(`${colors.accent}Triggering AI Failure Investigation for Build #${buildId}...${colors.reset}`); + } const res = await request("POST", `/api/v1/projects/${projectId}/build_runs/${buildId}/investigate`); if (args.json) { console.log(JSON.stringify(res, null, 2)); return; } - console.log(`\n${colors.bold}AI Failure Investigation Report:${colors.reset}`); - console.log(` Status: ${colors.ok}${res.status}${colors.reset}`); - console.log(` Diagnosis: ${res.diagnosis || res.summary || "Pending analysis..."}`); - if (res.suggested_fix) console.log(` Fix: ${colors.accent}${res.suggested_fix}${colors.reset}`); - if (res.attributed_author) console.log(` Attributed: ${res.attributed_author}`); + console.log(""); + printInvestigation(res); + // An investigation runs asynchronously, so the POST almost always returns + // `pending`. Point at the read path rather than letting the caller poll + // by re-POSTing, which is what made this cost money to check on (#1938). + if (res.status === "pending" || res.status === "running") { + console.log( + `\n ${colors.muted}Check the result with: butter builds investigation ${buildId} --project ${projectId}${colors.reset}` + ); + } console.log(""); } catch (err) { console.error(`${colors.err}✗ Failed to investigate build: ${err.message}${colors.reset}`); @@ -649,16 +811,45 @@ async function assetsDeny(args) { } } +// Flags that are switches, never name/value pairs (#1938). +// +// The parser used to treat every `--flag` as taking a value whenever the next +// argv entry did not start with `-`. That is fine at the end of a line, but +// `butter builds investigate --json ` parsed as +// `json: ""` with no positional left, so the command printed its +// usage error and exited 1 -- looking, from the caller's side, like `--json` +// silently produced no output while the plain form worked. Position in the +// command line must not change what a boolean flag means. +const BOOLEAN_FLAGS = new Set([ + "json", + "help", + "h", + "pending", + "verbose", + "yes", + "force", + "no-color", + "steps" +]); + +// `--key=value` is always explicit, so it is honored for any key including +// the booleans above (`--json=false` is not special-cased: anything after `=` +// is taken literally, which is the same behavior every other key gets). function parseArgs(rawArgs) { const parsed = { _: [] }; for (let i = 0; i < rawArgs.length; i++) { const arg = rawArgs[i]; if (arg.startsWith("--")) { - const key = arg.substring(2); - if (i + 1 < rawArgs.length && !rawArgs[i + 1].startsWith("-")) { - parsed[key] = rawArgs[++i]; + const body = arg.substring(2); + const eq = body.indexOf("="); + if (eq !== -1) { + parsed[body.substring(0, eq)] = body.substring(eq + 1); + } else if (BOOLEAN_FLAGS.has(body)) { + parsed[body] = true; + } else if (i + 1 < rawArgs.length && !rawArgs[i + 1].startsWith("-")) { + parsed[body] = rawArgs[++i]; } else { - parsed[key] = true; + parsed[body] = true; } } else if (arg.startsWith("-")) { const key = arg.substring(1); @@ -682,7 +873,10 @@ ${colors.bold}COMMANDS:${colors.reset} ${colors.accent}auth${colors.reset} login | whoami | logout Authenticate via browser OAuth loopback ${colors.accent}projects${colors.reset} list | show View and inspect game projects ${colors.accent}tasks${colors.reset} list | create | update Manage tasks, bugs, and backlog - ${colors.accent}builds${colors.reset} list | show | investigate Inspect CI/CD engine builds and AI failure logs + ${colors.accent}builds${colors.reset} list | show | investigate | investigation + Inspect CI/CD builds and AI failure investigations. + ${colors.bold}investigate${colors.reset} STARTS a paid AI analysis; + ${colors.bold}investigation${colors.reset} reads the result back for free. ${colors.accent}assets${colors.reset} list | show | approve | deny Review and approve textures, models, and audio ${colors.bold}GLOBAL OPTIONS:${colors.reset} @@ -732,7 +926,9 @@ async function main() { case "builds": case "build": if (subcmd === "list" || !subcmd) return buildsList(args); + if (subcmd === "show") return buildsShow(args); if (subcmd === "investigate") return buildsInvestigate(args); + if (subcmd === "investigation") return buildsInvestigation(args); break; case "assets": case "asset": @@ -745,6 +941,15 @@ async function main() { printHelp(); process.exit(1); } + + // Reaching here means the command matched but its subcommand did not, and + // every branch above `break`s rather than returning. That used to fall out + // of main() silently: `butter builds show ` printed nothing and exited + // 0, which reads as "this build has no data" rather than "this subcommand + // does not exist" (#1938). An unhandled subcommand is an error. + console.error(`${colors.err}Unknown subcommand: ${cmd} ${subcmd || ""}${colors.reset}`.trimEnd()); + printHelp(); + process.exit(1); } if (require.main === module) { diff --git a/package.json b/package.json index 43e340f..a836bf5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "butterstack-cli", - "version": "0.1.1", + "version": "0.2.0", "description": "Fast, scriptable, zero-dependency command line interface for the ButterStack game development pipeline: auth, projects, tasks, builds, and asset approvals.", "bin": { "butter": "./bin/butter" diff --git a/test/butter.test.js b/test/butter.test.js index 5fe79bd..9a70cf8 100644 --- a/test/butter.test.js +++ b/test/butter.test.js @@ -364,3 +364,216 @@ test("F2 (real flow): auth login persists the token when granted scope matches w rmHome(home); } }); + +// -- #1938: the builds read path --------------------------------------------- +// +// Three defects, all of which made a build's data look absent rather than +// unreachable: `builds show` was never wired into the dispatcher (silent +// exit 0), `--json` before a positional swallowed it as the flag's value, +// and there was no GET counterpart to `investigate`, so checking a result +// meant paying for a new one. + +// A server that answers the build endpoints with fixed JSON and records +// every request, so a test can assert on the METHOD used, not just output. +function startApiServer(routes) { + const requests = createQueue(); + const server = http.createServer((req, res) => { + requests.push({ method: req.method, url: req.url }); + const key = `${req.method} ${req.url.split("?")[0]}`; + const route = routes[key]; + const status = route ? route.status || 200 : 404; + const body = JSON.stringify(route ? route.body : { error: "not_found", message: "no route" }); + res.writeHead(status, { "Content-Type": "application/json", "Content-Length": Buffer.byteLength(body) }); + res.end(body); + }); + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => resolve({ server, port: server.address().port, requests })); + }); +} + +const BUILD_ID = "aeae7611-6d48-4601-8654-80169314e5ec"; + +const BUILD_DETAIL = { + id: BUILD_ID, + status: "failed", + overall_status: "failed", + build_type: "custom", + target_type: null, + commit_hash: "p4-207", + ci_job_name: "PilotLight_Verify", + duration: 0.764199, + created_at: "2026-09-18T10:55:02-04:00", + log_available: false, + log_unavailable_reason: + "TeamCity cannot be polled by ButterStack, so its logs must be pushed on the webhook payload as `logs_tail`.", + steps: [{ step_type: "external_build", status: "failed", duration: null, message: "TeamCity build failed" }], + investigation: null +}; + +const INVESTIGATION = { + investigation_id: "inv-1", + status: "completed", + diagnosis_category: "compile_error", + severity: "high", + confidence: "medium", + summary: "Godot parse error in main.gd", + diagnosis: "main.gd calls show_results() with 7 arguments against a 6-parameter definition.", + suggested_fix: "Reconcile screen_flow.gd with the current show_results() signature.", + attributed_user_id: null, + attributed_changelist: null, + affected_files: ["src/main.gd"], + evidence: ["SCRIPT ERROR: Parse Error: Too many arguments"], + error_message: null, + steps: [{ step: 1, step_type: "tool_use", content: null }] +}; + +test("#1938: `builds show` renders a build instead of exiting 0 with no output", async () => { + const home = mkHome(); + const { server, port } = await startApiServer({ + [`GET /api/v1/projects/108/build_runs/${BUILD_ID}`]: { body: BUILD_DETAIL } + }); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + const { stdout, success } = await runButter(["builds", "show", BUILD_ID, "--project", "108"], { home }); + + assert.equal(success, true); + assert.ok(stdout.includes(BUILD_ID), "the build id should be printed"); + assert.ok(stdout.includes("PilotLight_Verify"), "the job name should be printed"); + assert.ok(stdout.includes("p4-207"), "the commit should be printed"); + assert.ok(stdout.trim().length > 0, "the whole defect was that this printed nothing"); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("#1940: `builds show` says the build log is missing, and why", async () => { + const home = mkHome(); + const { server, port } = await startApiServer({ + [`GET /api/v1/projects/108/build_runs/${BUILD_ID}`]: { body: BUILD_DETAIL } + }); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + const { stdout } = await runButter(["builds", "show", BUILD_ID, "--project", "108"], { home }); + + assert.ok(stdout.includes("not available"), "a missing log must be called out"); + assert.ok(stdout.includes("logs_tail"), "and the reason must name the fix"); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("#1938: a boolean flag before a positional no longer swallows it", async () => { + const home = mkHome(); + const { server, port } = await startApiServer({ + [`GET /api/v1/projects/108/build_runs/${BUILD_ID}`]: { body: BUILD_DETAIL } + }); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + + // --json BEFORE the build id: the exact shape that used to parse as + // json: "" and leave no positional argument behind. + const before = await runButter(["builds", "show", "--json", BUILD_ID, "--project", "108"], { home }); + assert.equal(before.success, true, `expected success, got stderr: ${before.stderr}`); + assert.equal(JSON.parse(before.stdout).id, BUILD_ID); + + // ...and after it, which always worked, must keep working. + const after = await runButter(["builds", "show", BUILD_ID, "--project", "108", "--json"], { home }); + assert.equal(after.success, true); + assert.equal(JSON.parse(after.stdout).id, BUILD_ID); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("#1938: `builds investigation` reads the result with GET, never POST", async () => { + const home = mkHome(); + const { server, port, requests } = await startApiServer({ + [`GET /api/v1/projects/108/build_runs/${BUILD_ID}/investigation`]: { body: INVESTIGATION } + }); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + const { stdout, success } = await runButter( + ["builds", "investigation", BUILD_ID, "--project", "108"], + { home } + ); + + assert.equal(success, true); + assert.ok(stdout.includes("Godot parse error in main.gd")); + assert.ok(stdout.includes("compile_error")); + + const req = await requests.pop(2000); + assert.equal(req.method, "GET", "reading a result must not be a POST -- a POST is a paid AI call"); + assert.ok(req.url.endsWith("/investigation")); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("#1940: an unattributed investigation prints 'unattributed', not a guess", async () => { + const home = mkHome(); + const { server, port } = await startApiServer({ + [`GET /api/v1/projects/108/build_runs/${BUILD_ID}/investigation`]: { body: INVESTIGATION } + }); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + const { stdout } = await runButter(["builds", "investigation", BUILD_ID, "--project", "108"], { home }); + assert.ok(stdout.includes("unattributed")); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("#1938: a missing investigation is an explicit message, not an empty success", async () => { + const home = mkHome(); + const { server, port } = await startApiServer({}); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + const { stderr, success } = await runButter( + ["builds", "investigation", BUILD_ID, "--project", "108"], + { home } + ); + + assert.equal(success, false, "nothing to read is a failure exit, not a silent 0"); + assert.ok(stderr.includes("No investigation has been run")); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("#1938: an unknown subcommand fails loudly instead of exiting 0 silently", async () => { + const home = mkHome(); + try { + const { stderr, success } = await runButter(["builds", "frobnicate", "--project", "108"], { home }); + assert.equal(success, false); + assert.ok(stderr.includes("Unknown subcommand")); + } finally { + rmHome(home); + } +}); + +test("#1938: a server with no investigation key is not reported as 'no investigation'", async () => { + const home = mkHome(); + const { investigation, ...withoutKey } = BUILD_DETAIL; // eslint-disable-line no-unused-vars + const { server, port } = await startApiServer({ + [`GET /api/v1/projects/108/build_runs/${BUILD_ID}`]: { body: withoutKey } + }); + try { + writeCredentials(home, { host: `http://127.0.0.1:${port}` }); + const { stdout } = await runButter(["builds", "show", BUILD_ID, "--project", "108"], { home }); + + assert.ok(stdout.includes("does not expose investigations")); + assert.ok( + !stdout.includes("No AI investigation has been run"), + "an old server must not be reported as a build with no investigation" + ); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +});