Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions packages/sdk/src/cli/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Comment on lines +496 to +498

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟥 Authentication diagnostics can expose credentials

Failed auth output enters authFailureDetail after narrow pattern redaction. Unmatched API keys, bearer tokens, cookies, usernames, and organization identifiers then reach reports and errors.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

}),
};
}

/**
* 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, '<redacted-email>')
.replace(/\b(sk|pk|oat|rt)[-_][A-Za-z0-9._-]{8,}/gi, '<redacted-token>')
.replace(/\b[A-Fa-f0-9]{32,}\b/g, '<redacted-hex>');
Comment on lines +488 to +492

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the established secret redactor for probe output

If a failing provider or custom wrapper includes a credential in stderr, this new redactor exposes many secret formats already recognized elsewhere in this SDK, including Relay rk_live_/ot_live_ keys, GitHub ghp_ tokens, Slack xoxb- tokens, AWS access keys, bearer headers, and exact secret-valued environment variables. Because the resulting diagnostic is printed verbatim and is explicitly expected to be pasted into issues, route this output through the comprehensive redaction machinery in src/redact.ts or agent-transcript.ts rather than this narrower token regex.

Useful? React with 👍 / 👎.

}

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 ?? '',
Comment on lines +540 to +543

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Model-less auth failures stay unexplained

Without a model, probeCli discards runProbe output and keeps only its status. Codex and wrapper auth refusals therefore report no output despite captured diagnostics.

Learn more

The new output fields are returned from runProbe, but the model-less branch at probeCli evaluates only .status. Codex and custom wrapper adapters define no default model, so ordinary declarations without an explicit model take this branch. Their failed authentication probe can write a reason to stderr, yet CliProbeResult receives neither authExitCode nor authFailureDetail.

Example: A cli: codex step has no explicit model. codex login status exits 1 with Not logged in on stderr. Preflight emits “It produced no output” instead of quoting that reason.

Recommended fix: Store the full authentication result in the model-less branch and populate the same redacted failure fields used by the model-scoped branch. Add an end-to-end test using a failing executable so stderr capture and redaction are both exercised.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

};
}

function resolveExecutable(command: string, directory: string): string | undefined {
Expand Down
23 changes: 22 additions & 1 deletion packages/sdk/src/preflight.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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({
Expand Down
33 changes: 33 additions & 0 deletions packages/sdk/tests/preflight.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <redacted-email>',
}),
}),
},
);
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 = {
Expand Down
Loading