From d2af5815614ae5bcfaf63f5f5b2fbb1cc70f8f46 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 08:48:05 -0700 Subject: [PATCH] fix(miner): use claude/codex's real non-interactive CLI argv Advances #5135 defaultCliSubprocessArgs built ["--max-turns", N, "--acceptance-criteria", path, instructions] -- neither --max-turns nor --acceptance-criteria is a real flag on the claude or codex CLI (verified against `claude --help` and `codex exec --help`, plus a live invocation), and the argv never included -p/--print (claude) or the exec subcommand (codex) at all. Without those, a spawned claude/codex process starts an INTERACTIVE session against a subprocess whose stdin is closed -- meaning the claude-cli/codex-cli providers had no real non-interactive argv shape in production. Splits the shared builder into defaultClaudeCliArgs (--print --output-format json --permission-mode acceptEdits) and defaultCodexCliArgs (exec --json --sandbox workspace-write), matching each CLI's own real, verified interface. --output-format json / --json also finally activate claudeErrorStatus/codexErrorFromStdout's existing JSON(L) parsing, which was already written for exactly this shape but never actually triggered since the flag was never passed. Surfaces two more real, previously-latent gaps this exposed: - claude's own documented behavior ("--output-format json sometimes exits non-zero") implies it can also exit 0 while the JSON envelope reports is_error: true -- confirmed empirically via a live invocation. claudeErrorStatus was already written to detect this exact shape but was only ever checked on the code!==0 branch; now also checked on success exit codes. - Adds real cost parsing (ported from src/selfhost/ai.ts's own COST_KEYS/extractCliUsage best-effort pattern) so claude-cli/codex-cli now report costUsd from their own real JSON(L) output too, closing the "CLI providers report no cost" gap #5356 documented. driver-factory.ts's buildCliArgsWithConfiguredModel is now command-aware: codex's own -m/--model flag is scoped to the exec subcommand (per codex exec --help) and must land after "exec", not prefixed before everything the way claude's top-level --model flag can be -- a single shared prefix-everything scheme would have silently misparsed for codex. --- packages/gittensory-engine/src/index.ts | 3 +- .../src/miner/cli-subprocess-driver.ts | 123 +++++++++++++-- .../src/miner/driver-factory.ts | 27 +++- test/unit/cli-subprocess-driver.test.ts | 149 +++++++++++++++++- test/unit/coding-agent-miner.test.ts | 11 +- 5 files changed, 280 insertions(+), 33 deletions(-) diff --git a/packages/gittensory-engine/src/index.ts b/packages/gittensory-engine/src/index.ts index 9189a2669..2b722f294 100644 --- a/packages/gittensory-engine/src/index.ts +++ b/packages/gittensory-engine/src/index.ts @@ -277,7 +277,8 @@ export { } from "./miner/coding-agent-driver.js"; export { createCliSubprocessCodingAgentDriver, - defaultCliSubprocessArgs, + defaultClaudeCliArgs, + defaultCodexCliArgs, type CliSubprocessDriverOptions, type CliSubprocessSpawnFn, } from "./miner/cli-subprocess-driver.js"; diff --git a/packages/gittensory-engine/src/miner/cli-subprocess-driver.ts b/packages/gittensory-engine/src/miner/cli-subprocess-driver.ts index 171a9e256..6721479e7 100644 --- a/packages/gittensory-engine/src/miner/cli-subprocess-driver.ts +++ b/packages/gittensory-engine/src/miner/cli-subprocess-driver.ts @@ -59,8 +59,8 @@ export type CliSubprocessDriverOptions = { /** Known secret values (e.g. an injected auth token) to strip from any surfaced output, on top of the well-known * token-shape patterns. */ knownSecrets?: readonly string[]; - /** Build the CLI argv from a task. Default is a generic max-turns / acceptance-criteria / instructions argv that a - * caller overrides to match the real CLI's flags. */ + /** Build the CLI argv from a task. Defaults to `defaultClaudeCliArgs`/`defaultCodexCliArgs` based on + * `command` -- a caller overrides this to point at a different real CLI's own flags. */ buildArgs?: (task: CodingAgentDriverTask) => readonly string[]; }; @@ -84,8 +84,88 @@ const CODING_AGENT_ENV_ALLOWLIST = [ "no_proxy", ] as const; -function defaultBuildArgs(task: CodingAgentDriverTask): string[] { - return defaultCliSubprocessArgs(task); +/** Real, verified `claude -p` non-interactive argv (confirmed against `claude --help` and a live invocation, + * #5135 follow-up -- the previous default argv here used `--max-turns`/`--acceptance-criteria`, neither of + * which is a real `claude` CLI flag, and never passed `-p`/`--print` at all, meaning a spawned `claude` + * process would start an INTERACTIVE session against a subprocess whose stdin is closed). + * - `-p`/`--print` is REQUIRED for non-interactive use; without it `claude` starts an interactive session. + * - `--output-format json` produces a single parseable JSON result on stdout, matching `claudeErrorStatus`'s + * own `JSON.parse(stdout.trim())` assumption (which was already written for this shape, just never + * actually triggered because this flag was never passed). + * - `--permission-mode acceptEdits` is the same edit-permission scope the Agent-SDK driver already uses + * (#4267) -- file edits run unattended inside the scoped worktree, nothing broader. + * There is no turn-budget flag on the real CLI (verified via `claude --help`) and no acceptance-criteria + * flag either -- the coding agent discovers `task.acceptanceCriteriaPath` itself via its own Read tool + * inside the scoped working directory, exactly like the Agent-SDK driver already does (agent-sdk-driver.ts + * never passes it as a distinct option either; only `task.instructions` is forwarded as the prompt there + * too). The wall-clock `timeoutMs` (already implemented) is this provider's only real turn/cost ceiling. */ +export function defaultClaudeCliArgs(task: CodingAgentDriverTask): string[] { + return ["--print", "--output-format", "json", "--permission-mode", "acceptEdits", task.instructions]; +} + +/** Real, verified `codex exec` non-interactive argv (confirmed against `codex exec --help`, #5135 + * follow-up). `exec` is a REQUIRED subcommand -- without it `codex` starts an interactive session, the same + * class of bug the missing `-p` had for claude. `--json` emits the JSONL event stream + * `codexErrorFromStdout` already parses line-by-line. `--sandbox workspace-write` is codex's own equivalent + * of claude's `acceptEdits` -- the model may write within the workspace, nothing broader. Same "no + * turn-budget flag" and "no acceptance-criteria flag" gaps as claude apply here too. */ +export function defaultCodexCliArgs(task: CodingAgentDriverTask): string[] { + return ["exec", "--json", "--sandbox", "workspace-write", task.instructions]; +} + +/** Resolve the default argv builder for a known production command; throws for anything else so an + * unrecognized `command` fails at driver-construction time (never silently invokes a binary with no known + * real argv shape) unless the caller supplies an explicit `options.buildArgs` override. */ +function resolveDefaultBuildArgs(command: string): (task: CodingAgentDriverTask) => readonly string[] { + if (command === "claude") return defaultClaudeCliArgs; + if (command === "codex") return defaultCodexCliArgs; + throw new Error(`unsupported_cli_subprocess_command:${command}`); +} + +/** Best-effort real dollar-cost extraction from a CLI's own stdout. Mirrors src/selfhost/ai.ts's + * `extractCliUsage`/`COST_KEYS` (redeclared here, not imported, per this file's own no-src-import + * convention) but narrowed to just the cost field this driver's `CodingAgentDriverResult` surfaces -- tokens + * and model aren't part of that shape. Tries the whole trimmed stdout as one JSON object first (claude's + * `--output-format json` shape, empirically confirmed to carry `total_cost_usd`, the exact same field name + * the Agent-SDK's own result message carries), then scans line by line (codex's `--json` JSONL stream, + * "still evolving" per src/selfhost/ai.ts's own comment, so multiple real key spellings are tolerated). A + * missing/malformed field means "no cost signal", never an error -- never fabricated. */ +const COST_KEYS = ["total_cost_usd", "totalCostUsd", "cost_usd", "costUsd"] as const; + +function finiteNonNegativeNumber(value: unknown): number | undefined { + const n = typeof value === "number" ? value : typeof value === "string" && value.trim() ? Number(value) : NaN; + return Number.isFinite(n) && n >= 0 ? n : undefined; +} + +function costUsdFromRecord(record: Record): number | undefined { + let best: number | undefined; + for (const key of COST_KEYS) { + const n = finiteNonNegativeNumber(record[key]); + if (n !== undefined) best = Math.max(best ?? 0, n); + } + return best; +} + +function extractCostUsd(stdout: string): number | undefined { + const trimmed = stdout.trim(); + if (!trimmed) return undefined; + let best: number | undefined; + const tryLine = (text: string): void => { + try { + const parsed = JSON.parse(text) as unknown; + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + const n = costUsdFromRecord(parsed as Record); + if (n !== undefined) best = Math.max(best ?? 0, n); + } + } catch { + /* not JSON -- best-effort only */ + } + }; + tryLine(trimmed); + for (const line of trimmed.split(/\r?\n/)) { + if (line.trim()) tryLine(line); + } + return best; } /** Claude Code's `--output-format json` sometimes exits non-zero while still emitting a structured @@ -131,18 +211,6 @@ function codexErrorFromStdout(stdout: string): string | null { return null; } -/** The default argv contract, exported so the factory (#4289) can PREFIX provider config (e.g. a configured - * model flag) without re-inventing — and silently drifting from — this baseline argv shape. */ -export function defaultCliSubprocessArgs(task: CodingAgentDriverTask): string[] { - return [ - "--max-turns", - String(task.maxTurns), - "--acceptance-criteria", - task.acceptanceCriteriaPath, - task.instructions, - ]; -} - /** * Create a {@link CodingAgentDriver} that runs the coding agent as a CLI subprocess. A non-zero or absent exit * code, or a timeout, yields `ok: false` with a redacted error; exit `0` yields `ok: true`. Any subprocess output @@ -150,7 +218,7 @@ export function defaultCliSubprocessArgs(task: CodingAgentDriverTask): string[] */ export function createCliSubprocessCodingAgentDriver(options: CliSubprocessDriverOptions): CodingAgentDriver { const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; - const buildArgs = options.buildArgs ?? defaultBuildArgs; + const buildArgs = options.buildArgs ?? resolveDefaultBuildArgs(options.command); const knownSecrets = options.knownSecrets ?? []; return { async run(task: CodingAgentDriverTask): Promise { @@ -241,11 +309,32 @@ export function createCliSubprocessCodingAgentDriver(options: CliSubprocessDrive error: `${options.command}_exit_${spawned.code}: ${detail}`, }; } + // Claude Code's own documented behavior: `--output-format json` "sometimes exits non-zero", implying + // it can ALSO exit 0 while still reporting `is_error: true` in its structured JSON envelope (confirmed + // empirically: a live `claude -p --output-format json` invocation returns a real `is_error`/`subtype` + // field on every result, success or not). claudeErrorStatus was already written to detect exactly this + // shape but, before this fix, was only ever checked on the `code !== 0` branch above -- a code-0 error + // envelope silently read as `ok: true`. Checked here, not folded into that branch, since it's a + // genuinely different condition (exit code 0). + if (options.command === "claude") { + const errStatus = claudeErrorStatus(spawned.stdout); + if (errStatus) { + return { + ok: false, + changedFiles: [], + summary: `${options.command} exited 0 but reported an error envelope`, + transcript, + error: redactSecrets(`claude_code_error_${errStatus}`, knownSecrets), + }; + } + } + const costUsd = extractCostUsd(spawned.stdout); return { ok: true, changedFiles: [], summary: `${options.command} completed for ${task.attemptId}`, transcript, + ...(costUsd !== undefined ? { costUsd } : {}), }; }, }; diff --git a/packages/gittensory-engine/src/miner/driver-factory.ts b/packages/gittensory-engine/src/miner/driver-factory.ts index c641d6b02..e0014c67b 100644 --- a/packages/gittensory-engine/src/miner/driver-factory.ts +++ b/packages/gittensory-engine/src/miner/driver-factory.ts @@ -20,7 +20,8 @@ import type { CodingAgentDriverResult, CodingAgentDriverTask } from "./coding-ag import { guardCodingAgentDriverResult, type LintGuardOptions, type LintGuardResult } from "./lint-guard.js"; import { createCliSubprocessCodingAgentDriver, - defaultCliSubprocessArgs, + defaultClaudeCliArgs, + defaultCodexCliArgs, type CliSubprocessSpawnFn, } from "./cli-subprocess-driver.js"; import { @@ -119,12 +120,24 @@ export type CreateCodingAgentDriverOptions = { knownSecrets?: readonly string[] | undefined; }; -/** Build a CLI provider's argv: the driver's own default argv contract, prefixed with the CONFIGURED model - * flag when the provider's `CODING_AGENT_DRIVER_CONFIG_ENV` model key is set — this is where that declared - * config is actually consumed. */ -function buildCliArgsWithConfiguredModel(model: string | undefined): ((task: CodingAgentDriverTask) => readonly string[]) | undefined { +/** Build a CLI provider's argv: the driver's own real default argv contract, with the CONFIGURED model + * flag inserted at the RIGHT position for that command — this is where that declared config is actually + * consumed. claude's `--model` is a top-level flag (prefixed before everything else is fine); codex's + * `-m`/`--model` is scoped to the `exec` subcommand (per `codex exec --help`) and must be inserted AFTER + * the leading `"exec"` token, not before it — a single shared prefix-everything scheme would silently + * misparse for codex. */ +function buildCliArgsWithConfiguredModel( + command: "claude" | "codex", + model: string | undefined, +): ((task: CodingAgentDriverTask) => readonly string[]) | undefined { if (model === undefined) return undefined; - return (task) => ["--model", model, ...defaultCliSubprocessArgs(task)]; + if (command === "claude") { + return (task) => ["--model", model, ...defaultClaudeCliArgs(task)]; + } + return (task) => { + const [subcommand, ...rest] = defaultCodexCliArgs(task); + return [subcommand!, "--model", model, ...rest]; + }; } function createCliProvider( @@ -146,7 +159,7 @@ function createCliProvider( } const model = firstConfiguredEnvValue(env[modelEnvKey]); const timeoutMs = configuredTimeoutMs(env); - const buildArgs = buildCliArgsWithConfiguredModel(model); + const buildArgs = buildCliArgsWithConfiguredModel(command, model); return createCliSubprocessCodingAgentDriver({ command, spawn: options.spawn, diff --git a/test/unit/cli-subprocess-driver.test.ts b/test/unit/cli-subprocess-driver.test.ts index 2d46e9f86..a9da19a12 100644 --- a/test/unit/cli-subprocess-driver.test.ts +++ b/test/unit/cli-subprocess-driver.test.ts @@ -36,14 +36,39 @@ describe("createCliSubprocessCodingAgentDriver (#4266)", () => { expect(calls[0]?.opts.cwd).toBe("/work/attempt-1"); expect(calls[0]?.opts.timeoutMs).toBe(120_000); expect(calls[0]?.args).toEqual([ - "--max-turns", - "6", - "--acceptance-criteria", - "/work/attempt-1/acceptance-criteria.json", + "--print", + "--output-format", + "json", + "--permission-mode", + "acceptEdits", "Fix the pagination bug.", ]); }); + it("uses codex's own real default argv (the exec subcommand, not claude's flags)", async () => { + const { spawn, calls } = fakeSpawn({ stdout: "done", code: 0 }); + const driver = createCliSubprocessCodingAgentDriver({ command: "codex", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + expect(calls[0]?.cmd).toBe("codex"); + expect(calls[0]?.args).toEqual(["exec", "--json", "--sandbox", "workspace-write", "Fix the pagination bug."]); + }); + + it("fails closed at construction time for an unrecognized command with no explicit buildArgs override", () => { + const { spawn } = fakeSpawn({ stdout: "", code: 0 }); + expect(() => createCliSubprocessCodingAgentDriver({ command: "some-other-cli", spawn })).toThrow( + "unsupported_cli_subprocess_command:some-other-cli", + ); + }); + + it("an explicit buildArgs override still works for an unrecognized command (the fail-closed default is bypassable on purpose)", async () => { + const { spawn, calls } = fakeSpawn({ stdout: "done", code: 0 }); + const driver = createCliSubprocessCodingAgentDriver({ command: "some-other-cli", spawn, buildArgs: () => ["--custom"] }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + expect(calls[0]?.args).toEqual(["--custom"]); + }); + it("returns a redacted error on a non-zero exit code", async () => { const { spawn } = fakeSpawn({ stdout: "", @@ -332,4 +357,120 @@ describe("createCliSubprocessCodingAgentDriver (#4266)", () => { expect(result.error).toContain("[redacted]"); }); }); + + describe("REGRESSION: claude exits 0 while still reporting an error envelope (#5135 follow-up)", () => { + it("reports ok:false when --output-format json's envelope carries is_error:true even on exit code 0", async () => { + const { spawn } = fakeSpawn({ + stdout: JSON.stringify({ type: "result", subtype: "success", is_error: true, api_error_status: "invalid_api_key", total_cost_usd: 0 }), + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "claude", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(false); + expect(result.error).toBe("claude_code_error_invalid_api_key"); + }); + + it("still reports ok:true on a genuine exit-0 success envelope with is_error:false", async () => { + const { spawn } = fakeSpawn({ + stdout: JSON.stringify({ type: "result", subtype: "success", is_error: false, result: "done", total_cost_usd: 0.02 }), + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "claude", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + }); + + it("never inspects the envelope for a non-claude command on exit 0 either", async () => { + const { spawn } = fakeSpawn({ + stdout: JSON.stringify({ is_error: true, api_error_status: "invalid_api_key" }), + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "codex", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + }); + }); + + describe("REGRESSION: real dollar-cost extraction (#5135 follow-up)", () => { + it("extracts claude's real total_cost_usd from its single JSON result on success", async () => { + const { spawn } = fakeSpawn({ + stdout: JSON.stringify({ type: "result", subtype: "success", is_error: false, result: "done", total_cost_usd: 0.1234 }), + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "claude", spawn }); + const result = await driver.run(TASK); + expect(result.costUsd).toBe(0.1234); + }); + + it("extracts codex's real cost from its JSONL event stream, tolerating either key spelling", async () => { + const { spawn } = fakeSpawn({ + stdout: '{"type":"start"}\n{"type":"turn","total_cost_usd":0.05}\n{"type":"end"}', + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "codex", spawn }); + const result = await driver.run(TASK); + expect(result.costUsd).toBe(0.05); + }); + + it("stays undefined (never fabricated) when stdout carries no cost field at all", async () => { + const { spawn } = fakeSpawn({ stdout: "plain text output, no JSON", code: 0 }); + const driver = createCliSubprocessCodingAgentDriver({ command: "claude", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + expect(result.costUsd).toBeUndefined(); + }); + + it("takes the largest cost value seen across a multi-event codex stream (cumulative, matches src/selfhost/ai.ts's own convention)", async () => { + const { spawn } = fakeSpawn({ + stdout: '{"total_cost_usd":0.01}\n{"total_cost_usd":0.07}\n{"total_cost_usd":0.03}', + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "codex", spawn }); + const result = await driver.run(TASK); + expect(result.costUsd).toBe(0.07); + }); + + it("ignores a non-numeric cost field value instead of throwing or fabricating a number", async () => { + const { spawn } = fakeSpawn({ + stdout: JSON.stringify({ total_cost_usd: "not-a-number" }), + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "claude", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + expect(result.costUsd).toBeUndefined(); + }); + + it("ignores a negative cost value (a real number, but not a valid cost) instead of surfacing it", async () => { + const { spawn } = fakeSpawn({ + stdout: JSON.stringify({ total_cost_usd: -1 }), + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "claude", spawn }); + const result = await driver.run(TASK); + expect(result.costUsd).toBeUndefined(); + }); + + it("ignores a JSONL line that parses to a non-object (array or primitive) instead of throwing", async () => { + const { spawn } = fakeSpawn({ + stdout: '[1,2,3]\n"just a string"\n{"total_cost_usd":0.09}', + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "codex", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + expect(result.costUsd).toBe(0.09); + }); + + it("skips a blank interior line in the JSONL stream rather than treating it as malformed JSON", async () => { + const { spawn } = fakeSpawn({ + stdout: '{"total_cost_usd":0.06}\n\n{"type":"end"}', + code: 0, + }); + const driver = createCliSubprocessCodingAgentDriver({ command: "codex", spawn }); + const result = await driver.run(TASK); + expect(result.ok).toBe(true); + expect(result.costUsd).toBe(0.06); + }); + }); }); diff --git a/test/unit/coding-agent-miner.test.ts b/test/unit/coding-agent-miner.test.ts index 0732a59eb..fa99f11bd 100644 --- a/test/unit/coding-agent-miner.test.ts +++ b/test/unit/coding-agent-miner.test.ts @@ -442,7 +442,8 @@ describe("createCodingAgentDriver provider resolution (#4289)", () => { expect(result.ok).toBe(true); expect(calls[0]!.cmd).toBe("claude"); expect(calls[0]!.args).not.toContain("--model"); - expect(calls[0]!.args).toContain("--max-turns"); + expect(calls[0]!.args).toContain("--print"); + expect(calls[0]!.args).toContain("--output-format"); expect(calls[0]!.opts.cwd).toBe(cliTask.workingDirectory); }); @@ -456,10 +457,10 @@ describe("createCodingAgentDriver provider resolution (#4289)", () => { await driver.run(cliTask); const args = [...calls[0]!.args]; expect(args.slice(0, 2)).toEqual(["--model", "claude-sonnet-5"]); - expect(args).toContain("--max-turns"); + expect(args).toContain("--print"); }); - it("codex-cli reads ITS OWN model key and ignores claude's", async () => { + it("codex-cli reads ITS OWN model key and ignores claude's, placing --model AFTER the exec subcommand", async () => { const { spawn, calls } = recordingSpawn(); const driver = createCodingAgentDriver({ providerName: "codex-cli", @@ -468,7 +469,9 @@ describe("createCodingAgentDriver provider resolution (#4289)", () => { }); await driver.run(cliTask); expect(calls[0]!.cmd).toBe("codex"); - expect([...calls[0]!.args].slice(0, 2)).toEqual(["--model", "gpt-5.1-codex"]); + // codex's own --model/-m flag is scoped to the `exec` subcommand (codex exec --help), not a top-level + // flag the way claude's is -- it must land AFTER "exec", never prefixed before everything. + expect([...calls[0]!.args].slice(0, 3)).toEqual(["exec", "--model", "gpt-5.1-codex"]); }); it("CONSUMES the declared timeout env key when it is a positive integer, else defers to the driver default", async () => {