From 0b4558e072e10b670e569dc3851c87051932b1e8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 27 Jun 2026 09:02:04 -0700 Subject: [PATCH 1/2] fix(observability): capture AI-CLI stderr + surface exhausted provider failures to Sentry The claude/codex review CLIs intermittently failed with an opaque claude_code_exit_1 that was undiagnosable: defaultSpawn discarded stderr, and per-attempt provider failures logged at warn level, which the central Sentry forwarder (error/fatal only) skips. - defaultSpawn now captures the child stderr and threads it into the exit error, so the real cause (auth, rate limit, model-not-supported) reaches the logs. - runWorkersOpinion emits one ai_review_provider_exhausted log at error level once all models and attempts have thrown, so a genuine reviewer outage surfaces in Sentry while the noisy per-attempt retries stay at warn. --- src/selfhost/ai.ts | 16 +++++++++------ src/services/ai-review.ts | 18 +++++++++++++++++ test/unit/ai-review.test.ts | 37 +++++++++++++++++++++++++++++++++++ test/unit/selfhost-ai.test.ts | 33 ++++++++++++++++++++++++++++++- 4 files changed, 97 insertions(+), 7 deletions(-) diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index b82ec506fc..ecc89cb9a0 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -216,7 +216,7 @@ type SpawnFn = ( cmd: string, args: string[], opts: { env: Record; input?: string; timeoutMs: number; cwd?: string }, -) => Promise<{ stdout: string; code: number | null }>; +) => Promise<{ stdout: string; code: number | null; stderr?: string }>; async function defaultSpawn(): Promise { const cp = await import("node:child_process"); @@ -225,6 +225,9 @@ async function defaultSpawn(): Promise { const stdio: ["pipe", "pipe", "pipe"] = ["pipe", "pipe", "pipe"]; const child = cp.spawn(cmd, args, { cwd: o.cwd, env: o.env as NodeJS.ProcessEnv, stdio }); let stdout = ""; + // Capture stderr too — the CLI's actual error (auth, rate limit, model-not-supported, OOM) lands here, and + // it's what makes a `claude_code_exit_1` / `codex_exit_1` diagnosable instead of an opaque exit code (#26). + let stderr = ""; /* v8 ignore start */ // a 120s subprocess timeout is not unit-testable without a 2-minute wait const timer = setTimeout(() => { child.kill("SIGKILL"); @@ -232,13 +235,14 @@ async function defaultSpawn(): Promise { }, o.timeoutMs); /* v8 ignore stop */ child.stdout?.on("data", (d: Buffer) => (stdout += d.toString("utf8"))); + child.stderr?.on("data", (d: Buffer) => (stderr += d.toString("utf8"))); child.on("error", (e) => { clearTimeout(timer); reject(e); }); child.on("close", (code) => { clearTimeout(timer); - resolve({ stdout, code }); + resolve({ stdout, code, stderr }); }); if (o.input != null) { child.stdin?.write(o.input); @@ -262,12 +266,12 @@ export function createClaudeCodeAi(parentEnv: Record const spawn = spawnImpl ?? (await defaultSpawn()); const claudeModel = resolveModel(configuredModel(parentEnv), model, "claude-sonnet-4-6"); const effort = resolveEffort(parentEnv.AI_EFFORT); - const { stdout, code } = await spawn( + const { stdout, code, stderr } = await spawn( "claude", ["--print", "--output-format", "json", "--model", claudeModel, "--permission-mode", "plan", "--effort", effort, "--disallowedTools", "Bash,Edit,Write,WebFetch,WebSearch"], { env, input: prompt, timeoutMs: resolveCliTimeoutMs(parentEnv), cwd: await isolatedCliCwd() }, ); - if (code !== 0) throw new Error(`claude_code_exit_${code ?? "null"}`); + if (code !== 0) throw new Error(`claude_code_exit_${code ?? "null"}: ${(stderr ?? "").slice(0, 500)}`); const errStatus = claudeErrorStatus(stdout); if (errStatus) throw new Error(`claude_code_error_${errStatus}`); const text = extractCliText(stdout); @@ -296,12 +300,12 @@ export function createCodexAi(parentEnv: Record, spa const args = ["exec", "--json", "--skip-git-repo-check", "--sandbox", "read-only"]; if (codexModel) args.push("--model", codexModel); args.push("--", prompt); - const { stdout, code } = await spawn("codex", args, { + const { stdout, code, stderr } = await spawn("codex", args, { env, timeoutMs: resolveCliTimeoutMs(parentEnv), cwd: await isolatedCliCwd(), }); - if (code !== 0) throw new Error(`codex_exit_${code ?? "null"}`); + if (code !== 0) throw new Error(`codex_exit_${code ?? "null"}: ${(stderr ?? "").slice(0, 500)}`); const text = extractCliText(stdout); if (!text) throw new Error("codex_empty_output"); return { response: text }; diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 032f052f05..f150a80c60 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -518,6 +518,9 @@ async function runWorkersOpinion( const extra: AiGatewayOptions | undefined = gatewayId ? { gateway: { id: gatewayId } } : undefined; + // Track the last provider error so we can fail-LOUD once ALL models × attempts are exhausted (below). Per-attempt + // logs are warn (noisy retries, skipped by the central Sentry forwarder); the exhausted summary is error (#26). + let lastError: unknown; for (const model of fallback && fallback !== primary ? [primary, fallback] : [primary]) { @@ -550,9 +553,24 @@ async function runWorkersOpinion( error: errorMessage(error), }), ); + lastError = error; } } } + // All models × attempts threw (vs "ran but returned unparseable output", where lastError stays undefined): the + // reviewer is genuinely DOWN. Emit one level:error log so the central Sentry forwarder surfaces the outage — the + // per-attempt warns above are invisible to it. (#26 fail-loud) + if (lastError !== undefined) { + console.log( + JSON.stringify({ + level: "error", + event: "ai_review_provider_exhausted", + primary, + fallback, + error: errorMessage(lastError), + }), + ); + } return null; } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 19b964d982..695e9bc09b 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -1188,6 +1188,43 @@ describe("pure helpers", () => { expect(run).toHaveBeenCalledTimes(1); }); + it("logs ai_review_provider_exhausted at error level when every attempt throws (#26 fail-loud)", async () => { + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const run = vi.fn(async () => { + throw new Error("ENOENT: claude binary not found"); + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const result = await runWorkersOpinion(env, "primary-model", "", "sys", "user", 256); + expect(result).toBeNull(); + const exhausted = logSpy.mock.calls + .map((c) => c[0]) + .find((l) => typeof l === "string" && l.includes("ai_review_provider_exhausted")); + expect(exhausted).toBeDefined(); + expect(JSON.parse(exhausted as string)).toMatchObject({ + level: "error", + event: "ai_review_provider_exhausted", + primary: "primary-model", + error: expect.stringContaining("ENOENT"), + }); + logSpy.mockRestore(); + warnSpy.mockRestore(); + }); + + it("does NOT log exhausted when the model runs but returns unparseable output (no provider error)", async () => { + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const run = vi.fn(async () => ({ response: "not json at all" })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const result = await runWorkersOpinion(env, "primary-model", "", "sys", "user", 256); + expect(result).toBeNull(); + expect( + logSpy.mock.calls + .map((c) => c[0]) + .some((l) => typeof l === "string" && l.includes("ai_review_provider_exhausted")), + ).toBe(false); + logSpy.mockRestore(); + }); + it("applies the default daily neuron budget when none is configured", async () => { const run = vi.fn(async (_model: string) => ({ response: reviewJson() })); const env = createTestEnv({ diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index c2ce8b218d..47d09f8a24 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -52,7 +52,7 @@ describe("resolveCliTimeoutMs (#selfhost — subprocess timeout scales with effo afterEach(() => vi.unstubAllGlobals()); -type SpawnResult = { stdout: string; code: number | null }; +type SpawnResult = { stdout: string; code: number | null; stderr?: string }; type StubSpawn = ( cmd: string, args: string[], @@ -456,6 +456,37 @@ describe("subscription CLI helpers + fail-safe", () => { await expect(createCodexAi({}, empty).run("gpt-5", { prompt: "x" })).rejects.toThrow(/codex_empty_output/); }); + it("surfaces the CLI's stderr in the non-zero-exit error (diagnosable failures, #26)", async () => { + // Without stderr in the message, a `claude_code_exit_1` / `codex_exit_1` is an opaque dead-end; with it the real + // cause (auth, rate limit, model-not-supported) reaches the logs + Sentry. (stderr-present branch of `?? ""`.) + const claudeErr: StubSpawn = async () => ({ stdout: "", code: 1, stderr: "Invalid API key · auth_error" }); + await expect( + createClaudeCodeAi({ CLAUDE_CODE_OAUTH_TOKEN: "t" }, claudeErr).run("m", { prompt: "x" }), + ).rejects.toThrow(/claude_code_exit_1: Invalid API key/); + const codexErr: StubSpawn = async () => ({ stdout: "", code: 1, stderr: "stream error: rate limit reached" }); + await expect(createCodexAi({}, codexErr).run("m", { prompt: "x" })).rejects.toThrow( + /codex_exit_1: stream error: rate limit reached/, + ); + }); + + it("defaultSpawn captures a failing CLI's stderr and surfaces it on the exit error (#26)", async () => { + const dir = mkdtempSync(join(tmpdir(), "fakecli-")); + const fake = join(dir, "claude"); + // a fake `claude` that reads stdin (so the parent's write never EPIPEs), then writes to STDERR and exits non-zero + // — the real failure shape we previously couldn't diagnose. + writeFileSync(fake, "#!/usr/bin/env node\nlet i='';process.stdin.on('data',d=>i+=d);process.stdin.on('end',()=>{process.stderr.write('BOOM: auth failed');process.exit(1);});\n"); + chmodSync(fake, 0o755); + const origPath = process.env.PATH; + process.env.PATH = `${dir}:${origPath ?? ""}`; + try { + await expect( + createClaudeCodeAi({ ...process.env, CLAUDE_CODE_OAUTH_TOKEN: "t" }).run("sonnet", { prompt: "x" }), + ).rejects.toThrow(/claude_code_exit_1: BOOM: auth failed/); + } finally { + process.env.PATH = origPath; + } + }); + it("defaultSpawn rejects when the CLI binary is missing (error handler)", async () => { const origPath = process.env.PATH; process.env.PATH = "/nonexistent-gittensory-empty"; From 20f47ac9b303022ebcf19a01535e563dc9c97ad9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 27 Jun 2026 09:31:42 -0700 Subject: [PATCH 2/2] fix(security): redact secrets from AI-CLI stderr before embedding in errors The prior commit embedded raw claude/codex subprocess stderr into the thrown exit error. That stderr can echo the OAuth token we pass via env (or a key from a config the CLI reads), and the error flows to logs and the Sentry forwarder, whose scrubber only redacts secret-keyed fields, never free-text -- so a token inside the error string would leak. Add redactSecrets(): strip the caller's known secret values exactly, then well-known credential shapes (sk-/sk-ant-/sk-proj-, GitHub PAT/OAuth/server tokens, JWT, AWS key id), each word-boundary anchored so genuine diagnostics survive. Applied at both the claude (with the OAuth token) and codex throw sites before truncation. --- src/selfhost/ai.ts | 28 ++++++++++++++++++++++-- test/unit/selfhost-ai.test.ts | 40 ++++++++++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index ecc89cb9a0..bba446bf46 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -251,6 +251,30 @@ async function defaultSpawn(): Promise { }); } +/** Credential/token shapes that must never reach logs or Sentry. High-precision (prefixed key formats + JWT, each + * anchored on a word boundary) so genuine diagnostics — auth/rate-limit/model errors — survive redaction. */ +const SECRET_PATTERNS: readonly RegExp[] = [ + /\bsk-[A-Za-z0-9_-]{16,}/g, // OpenAI / Anthropic keys (sk-..., sk-ant-..., sk-proj-...) + /\bgh[oprsu]_[A-Za-z0-9]{20,}/g, // GitHub PAT / OAuth / server / refresh tokens + /\bgithub_pat_[A-Za-z0-9_]{20,}/g, // GitHub fine-grained PAT + /\beyJ[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_-]{6,}/g, // JWT (header.payload.signature) + /\bAKIA[0-9A-Z]{16}/g, // AWS access key id +]; + +/** Redact secrets from untrusted CLI stderr before it enters an error message that flows to logs/Sentry. The + * claude/codex subprocesses can echo back the OAuth token we hand them via env (or a key from a config they read), + * and the central Sentry forwarder only scrubs secret-KEYED fields, never free-text — so a token inside an error + * string would otherwise leak. Strips the caller's known secret values exactly, then well-known token shapes. */ +export function redactSecrets(text: string, knownSecrets: readonly string[] = []): string { + let out = text; + for (const secret of knownSecrets) { + // Length-guard so a short/empty token (e.g. a stubbed "t") can't blank out unrelated diagnostic text. + if (secret.length >= 8) out = out.split(secret).join("[redacted]"); + } + for (const pattern of SECRET_PATTERNS) out = out.replace(pattern, "[redacted]"); + return out; +} + /** Claude Code subscription (CLAUDE_CODE_OAUTH_TOKEN via `claude setup-token`). Headless, read-only, JSON. */ export function createClaudeCodeAi(parentEnv: Record, spawnImpl?: SpawnFn): SelfHostAi { return { @@ -271,7 +295,7 @@ export function createClaudeCodeAi(parentEnv: Record ["--print", "--output-format", "json", "--model", claudeModel, "--permission-mode", "plan", "--effort", effort, "--disallowedTools", "Bash,Edit,Write,WebFetch,WebSearch"], { env, input: prompt, timeoutMs: resolveCliTimeoutMs(parentEnv), cwd: await isolatedCliCwd() }, ); - if (code !== 0) throw new Error(`claude_code_exit_${code ?? "null"}: ${(stderr ?? "").slice(0, 500)}`); + if (code !== 0) throw new Error(`claude_code_exit_${code ?? "null"}: ${redactSecrets(stderr ?? "", [token]).slice(0, 500)}`); const errStatus = claudeErrorStatus(stdout); if (errStatus) throw new Error(`claude_code_error_${errStatus}`); const text = extractCliText(stdout); @@ -305,7 +329,7 @@ export function createCodexAi(parentEnv: Record, spa timeoutMs: resolveCliTimeoutMs(parentEnv), cwd: await isolatedCliCwd(), }); - if (code !== 0) throw new Error(`codex_exit_${code ?? "null"}: ${(stderr ?? "").slice(0, 500)}`); + if (code !== 0) throw new Error(`codex_exit_${code ?? "null"}: ${redactSecrets(stderr ?? "").slice(0, 500)}`); const text = extractCliText(stdout); if (!text) throw new Error("codex_empty_output"); return { response: text }; diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index 47d09f8a24..3781675a9a 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -2,7 +2,7 @@ import { chmodSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { buildProvider, claudeErrorStatus, createAnthropicAi, createChainAi, createClaudeCodeAi, createCodexAi, createOpenAiCompatibleAi, createSelfHostAi, extractCliText, resolveAiReviewerPlan, resolveCliTimeoutMs, resolveEffort, resolveModel, resolveProviderNames, resolveRequiredCliProviders, routeProviders, subscriptionCliEnv } from "../../src/selfhost/ai"; +import { buildProvider, claudeErrorStatus, createAnthropicAi, createChainAi, createClaudeCodeAi, createCodexAi, createOpenAiCompatibleAi, createSelfHostAi, extractCliText, resolveAiReviewerPlan, resolveCliTimeoutMs, resolveEffort, resolveModel, resolveProviderNames, resolveRequiredCliProviders, redactSecrets, routeProviders, subscriptionCliEnv } from "../../src/selfhost/ai"; describe("resolveModel (#979 — never leak the Workers-AI default to a self-host backend)", () => { const WORKERS_DEFAULT = "@cf/meta/llama-3.1-8b-instruct-fp8-fast"; @@ -469,6 +469,22 @@ describe("subscription CLI helpers + fail-safe", () => { ); }); + it("redacts the OAuth token and key-shaped tokens from claude stderr before they reach the error (#1605 sec)", async () => { + // The CLI can echo the token we hand it via env; it must never land in an error string forwarded to Sentry. + const token = "oauth-tok-abcdef123456"; + const leaky: StubSpawn = async () => ({ stdout: "", code: 1, stderr: `fatal: rejected token ${token} (key sk-ant-api03-ABCDEFGHIJKLMNOPqrstuvwx)` }); + const err = await createClaudeCodeAi({ CLAUDE_CODE_OAUTH_TOKEN: token }, leaky).run("m", { prompt: "x" }).catch((e: Error) => e.message); + expect(err).toContain("claude_code_exit_1:"); + expect(err).not.toContain(token); + expect(err).not.toContain("sk-ant-api03"); + expect(err).toContain("[redacted]"); + }); + + it("redacts key-shaped tokens from codex stderr (no env token to key off) (#1605 sec)", async () => { + const leaky: StubSpawn = async () => ({ stdout: "", code: 1, stderr: "auth failed: ghp_ABCDEFGHIJ0123456789KLMNOPQRSTUV" }); + await expect(createCodexAi({}, leaky).run("m", { prompt: "x" })).rejects.toThrow(/codex_exit_1: auth failed: \[redacted\]/); + }); + it("defaultSpawn captures a failing CLI's stderr and surfaces it on the exit error (#26)", async () => { const dir = mkdtempSync(join(tmpdir(), "fakecli-")); const fake = join(dir, "claude"); @@ -502,3 +518,25 @@ describe("subscription CLI helpers + fail-safe", () => { expect(extractCliText("not json\nstill not json")).toBe(""); }); }); + +describe("redactSecrets — strip credentials from untrusted CLI stderr before it reaches logs/Sentry (#1605 sec)", () => { + it("redacts caller-known secret values (>= 8 chars) and leaves short ones untouched", () => { + expect(redactSecrets("token=supersecretvalue used", ["supersecretvalue"])).toBe("token=[redacted] used"); + // a short known value must NOT blank out unrelated text (length-guard false branch) + expect(redactSecrets("the cat sat", ["cat"])).toBe("the cat sat"); + }); + + it("redacts well-known token shapes with no known-value list (default arg)", () => { + expect(redactSecrets("key sk-ant-api03-ABCDEFGHIJKLMNOPqrstuvwx12")).toBe("key [redacted]"); + expect(redactSecrets("pat ghp_ABCDEFGHIJ0123456789KLMNOPQRSTUV")).toBe("pat [redacted]"); + expect(redactSecrets("fine github_pat_ABCDEFGHIJ0123456789KLMNO")).toBe("fine [redacted]"); + expect(redactSecrets("jwt eyJhbGciOi.eyJzdWIiOi.S1gnaTuRe99")).toBe("jwt [redacted]"); + expect(redactSecrets("aws AKIAIOSFODNN7EXAMPLE here")).toBe("aws [redacted] here"); + }); + + it("leaves benign diagnostics intact, including words that merely contain a token prefix", () => { + expect(redactSecrets("Invalid API key · auth_error")).toBe("Invalid API key · auth_error"); + // "disk-usage-report-2024-summary" must survive — the \b anchor prevents an in-word `sk-` false positive + expect(redactSecrets("disk-usage-report-2024-summary failed")).toBe("disk-usage-report-2024-summary failed"); + }); +});