diff --git a/packages/sdk/src/cli/check.ts b/packages/sdk/src/cli/check.ts index 22c362c9c..5293cb157 100644 --- a/packages/sdk/src/cli/check.ts +++ b/packages/sdk/src/cli/check.ts @@ -478,36 +478,70 @@ function probeCli( modelCommand, }; } - const authStatus = probe(auth).status; + const authProbe = probe(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, environment: NodeJS.ProcessEnv = process.env, -): { status: number | null; stdout: string } { +): { status: number | null; stdout: string; stderr: string } { const env = { ...environment }; 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 2ebad36dd..6e30ab72e 100644 --- a/packages/sdk/src/preflight.ts +++ b/packages/sdk/src/preflight.ts @@ -40,6 +40,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 = @@ -510,12 +521,22 @@ function probeResolvedCli( message: `Step "${resolution.stepId}" uses Relay's interactive CLI transport. The executable exists, but authentication${resolution.model ? ' and access to model "' + resolution.model + '"' : ''} could not be verified before startup. The managed session must execute its task and report completion; startup or task failure fails the step.` }); } else if (result.authenticated !== true) { 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 6c5596f9c..ad9a3c35f 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 = {