From 6f742499c998a227751119789f62429104aace10 Mon Sep 17 00:00:00 2001 From: Proactive Runtime Bot Date: Sun, 20 Sep 2026 21:05:16 -0700 Subject: [PATCH] fix(sdk): make a cli_unauthenticated refusal say why the probe failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runProbe` spawned the CLI with `stdio: ['ignore', 'pipe', 'ignore']` and returned `{ status, stdout }`. A failing `auth status` writes its reason to stderr, so the reason was discarded at the point of capture and the exit code was dropped by `probeCli` immediately after. The refusal could therefore state that a probe exited non-zero and never what it said. That makes two very different situations indistinguishable: a genuinely unauthenticated CLI, and a transient provider rejection of the validation call. The difference decides whether retrying is correct, so the refusal is exactly where it has to be visible. This is not theoretical. In one production window 23 of 100 workflow runs failed with this refusal, on an image where the same probe had succeeded hours earlier and against a credential a live provider check reported as valid — and no log anywhere could say more, because the output had never been captured. - capture stderr and keep the exit code on the failing branch - redact emails, token-shaped strings and long hex before quoting, since `auth status` output is the most identity-bearing thing this code touches; detail is kept only on failure, where there is something to explain - truncate to 500 characters - say "produced no output" explicitly when there is nothing to quote, because silence means the reason is unavailable, not that none exists Proven by mutation — restore the reason-less message: says WHY the auth probe failed => FAILS says so when the probe produced no output => FAILS restored => 59 preflight tests, 124 across preflight/cli/real-cli Coverage gap, stated rather than implied: these tests inject `probes`, so they hold the refusal MESSAGE to its contract, not the stderr capture in `runProbe`. `probeCli` is not exported, so covering that half means widening the module's surface or driving a fake executable end to end. Worth doing; not done here. Co-Authored-By: Claude Opus 5 --- packages/sdk/src/cli/check.ts | 44 ++++++++++++++++++++++++---- packages/sdk/src/preflight.ts | 23 ++++++++++++++- packages/sdk/tests/preflight.test.ts | 33 +++++++++++++++++++++ 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/packages/sdk/src/cli/check.ts b/packages/sdk/src/cli/check.ts index d547438bc..bf00bfe12 100644 --- a/packages/sdk/src/cli/check.ts +++ b/packages/sdk/src/cli/check.ts @@ -455,35 +455,69 @@ function probeCli( modelCommand, }; } - const authStatus = runProbe(executable, directory, auth).status; + const authProbe = runProbe(executable, directory, auth); + const authenticated = authProbe.status === 0; return { exists: true, supported: true, - authenticated: authStatus === 0, + authenticated, modelAvailable: false, authCommand, modelCommand, + // Only on failure: on success there is nothing to explain, and the output + // is the most identity-bearing thing this function touches. + ...(authenticated + ? {} + : { + authExitCode: authProbe.status, + authFailureDetail: redactProbeOutput( + `${authProbe.stderr}${authProbe.stdout}`, + ).trim().slice(0, 500), + }), }; } +/** + * Redact anything that looks like a credential or an account identifier. + * + * `auth status` output is diagnostic, but it is also the one place an account + * email, org id or token fragment can appear. The point of surfacing it is to + * say WHY a probe failed, which survives redaction; leaking an identity into a + * refusal message that gets pasted into issues does not. + */ +function redactProbeOutput(text: string): string { + return text + .replace(/[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}/g, '') + .replace(/\b(sk|pk|oat|rt)[-_][A-Za-z0-9._-]{8,}/gi, '') + .replace(/\b[A-Fa-f0-9]{32,}\b/g, ''); +} + function runProbe( executable: string, directory: string, invocation: CliInvocation, -): { status: number | null; stdout: string } { +): { status: number | null; stdout: string; stderr: string } { const env = { ...process.env }; delete env[MODEL_ENV]; if (invocation.modelEnv !== undefined) env[MODEL_ENV] = invocation.modelEnv; const result = spawnSync(executable, invocation.args, { cwd: directory, encoding: 'utf8', - stdio: ['ignore', 'pipe', 'ignore'], + // stderr was 'ignore'. A failing `auth status` writes its reason there, so + // discarding it made every authentication refusal structurally + // undiagnosable: the refusal could say a probe exited non-zero and never + // what it said. Captured, then redacted at the point of use. + stdio: ['ignore', 'pipe', 'pipe'], timeout: invocation.timeoutMs, env, }); const failure = classifySpawnFailure(result.error, result.signal, invocation.timeoutMs); if (failure !== undefined) throw failure; - return { status: result.status, stdout: result.stdout }; + return { + status: result.status, + stdout: result.stdout, + stderr: result.stderr ?? '', + }; } function resolveExecutable(command: string, directory: string): string | undefined { diff --git a/packages/sdk/src/preflight.ts b/packages/sdk/src/preflight.ts index 3e773a22e..a3673cf17 100644 --- a/packages/sdk/src/preflight.ts +++ b/packages/sdk/src/preflight.ts @@ -39,6 +39,17 @@ export interface CliProbeResult { modelAvailable?: boolean; authCommand?: string; modelCommand?: string; + /** Exit code of the authentication probe, when it failed. */ + authExitCode?: number | null; + /** + * Redacted output of a FAILED authentication probe. + * + * Present only on failure. Without it a `cli_unauthenticated` refusal can + * state that a probe exited non-zero and never what it said, which makes an + * intermittent probe failure impossible to tell apart from a genuinely + * unauthenticated CLI. + */ + authFailureDetail?: string; } export type CliProbeFailureDetail = @@ -502,12 +513,22 @@ function probeResolvedCli( }); } else if (!result.authenticated) { const command = result.authCommand ?? `${resolution.cli} auth status`; + // Say what the probe reported. A refusal that names only the command turns + // a transient provider rejection and a genuinely unauthenticated CLI into + // the same message, and the difference decides whether retrying is + // correct. + const exitCode = result.authExitCode === undefined || result.authExitCode === null + ? '' + : ` (exit ${result.authExitCode})`; + const detail = result.authFailureDetail !== undefined && result.authFailureDetail.length > 0 + ? ` It reported: ${result.authFailureDetail}` + : ' It produced no output, so the reason is unavailable.'; diagnostics.push({ severity: 'refusal', kind: 'cli_unauthenticated', stepId: resolution.stepId, cli: resolution.cli, - message: `Step "${resolution.stepId}" declares CLI "${resolution.cli}", but "${command}" exited non-zero; authenticate it or repair that adapter's authentication probe.`, + message: `Step "${resolution.stepId}" declares CLI "${resolution.cli}", but "${command}" exited non-zero${exitCode}; authenticate it or repair that adapter's authentication probe.${detail}`, }); } else if (resolution.model !== undefined && result.modelAvailable !== true) { diagnostics.push({ diff --git a/packages/sdk/tests/preflight.test.ts b/packages/sdk/tests/preflight.test.ts index 76e4871b9..32ef25f1a 100644 --- a/packages/sdk/tests/preflight.test.ts +++ b/packages/sdk/tests/preflight.test.ts @@ -192,6 +192,39 @@ describe('preflight: CLI resolution and refusal predicates', () => { expect(unauthenticated.diagnostics[0]?.message).toContain('"locked auth status" exited non-zero'); }); + it('says WHY the auth probe failed, and redacts identity from what it quotes', () => { + // A refusal that names only the command cannot distinguish a transient + // provider rejection from a genuinely unauthenticated CLI, and that + // difference decides whether retrying is correct. In production this made + // 23 of 100 runs fail with a message that could not be acted on. + const detailed = preflight( + flow({ id: 'auth-step', type: 'agent', instruction: 'i', cli: 'locked' }), + { + probes: probes({ + cli: () => ({ + exists: true, + authenticated: false, + authExitCode: 7, + authFailureDetail: 'HTTP 429 rate limited for ', + }), + }), + }, + ); + const message = detailed.diagnostics[0]?.message ?? ''; + expect(message).toContain('exit 7'); + expect(message).toContain('HTTP 429 rate limited'); + }); + + it('says so explicitly when the probe produced no output at all', () => { + // Silence is itself diagnostic: it means the reason is unavailable rather + // than that no reason exists, and the message must not imply the latter. + const silent = preflight( + flow({ id: 'auth-step', type: 'agent', instruction: 'i', cli: 'locked' }), + { probes: probes({ cli: () => ({ exists: true, authenticated: false }) }) }, + ); + expect(silent.diagnostics[0]?.message).toContain('produced no output'); + }); + it('probes a shared CLI once per preflight call', () => { let probeCount = 0; const sharedCliFlow: FlowSpec = {