Repository navigation
fix(dogfood): record candidate argv independently of the execution adapter - #2848
Conversation
…apter The linux-x64-musl leg is the only dogfood platform that runs the candidate through an execution adapter (scripts/run-musl-dogfood.sh, Alpine in Docker). captureCommand folded the adapter's spawn vector into the evidence `argv`, so that leg recorded ['<genie binary>', 'task', 'list', '--json'] where every other platform recorded ['task', 'list', '--json'] — and validate-live-dogfood-evidence's standalone assertion, which requires an exact 'task list --json' / 'board --json', failed stages 5-7 on musl alone. Stable release publish has been failing at that gate. Evidence `argv` is now always the candidate's OWN arguments. When an adapter wraps the run, the real spawned vector is recorded separately as `adapterArgv: [candidateBinary, ...argv]`; `executable` remains the adapter and `candidateBinary` the genie binary, as before. The stage command projection and the requireExit diagnostic use `adapterArgv ?? argv`, so the human-readable command line stays the true spawned command. capturePtySetup had the same fold and is fixed through the shared resolveInvocation helper. The validator accepts the optional `adapterArgv` (rejecting a non-string-array), keeps the standalone check on `argv`, and extends the retired-MCP rejection to cover both vectors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bf5d1453d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| command.adapterArgv !== undefined && | ||
| (!Array.isArray(command.adapterArgv) || command.adapterArgv.some((arg) => typeof arg !== 'string')) | ||
| ) { | ||
| errors.push(`${label}.adapterArgv must be a string array`); |
There was a problem hiding this comment.
For adapter-backed evidence, this only verifies that adapterArgv is a string array; it never requires it to equal [candidateBinary, ...argv]. Consequently, evidence can claim standalone task list --json and board --json in argv while the recorded spawn vector invokes different arguments or even a different binary, and the release validator still accepts the supposed standalone proof. Enforce the documented relationship before using argv as candidate-execution evidence.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
| const vectors = commands.flatMap((value) => [value.record.argv, value.record.adapterArgv]); | ||
| if (vectors.some((args) => Array.isArray(args) && args.some((arg) => String(arg).toLowerCase().includes('mcp')))) { |
There was a problem hiding this comment.
On adapter-backed runs, adapterArgv[0] is the absolute candidate-binary path rather than a candidate argument. If TMPDIR or an injected dogfood root contains the substring mcp (for example /tmp/mcp-release), every otherwise valid musl standalone observation is rejected as retired MCP evidence. Scan only the actual candidate arguments, or at least omit the adapter vector's first element.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
Problem
Stable release publish fails on linux-x64-musl only. Run 33291504929, job
publish / Codex standalone task/board dogfood / linux-x64-musl:glibc / arm64 / darwin all pass.
Root cause
musl is the only leg that runs the candidate through an execution adapter (
scripts/run-musl-dogfood.sh, digest-pinned Alpine in Docker).captureCommandintests/support/codex-dogfood-harness.tsfolded the adapter's spawn vector into the recorded evidenceargv:So musl recorded
['<genie binary>', 'task', 'list', '--json']where every other platform recorded['task', 'list', '--json'], andvalidateStandaloneObservationinscripts/validate-live-dogfood-evidence.tsrequiresargv.join(' ')to be exactlytask list --json/board --json.capturePtySetuphad the identical fold (argv: commandArgs.slice(1), which under an adapter leaves the binary at index 0).Fix
Evidence
argvis now always the candidate's own arguments, on every platform. When an adapter wraps the run, the real spawned vector is recorded separately asadapterArgv: [candidateBinary, ...argv];executablestays the adapter path andcandidateBinarythe genie binary, both unchanged.resolveInvocation()helper splits{ command, argv, adapterArgv, spawnArgv }; bothcaptureCommandandcapturePtySetupgo through it.stage()commandprojection, therequireExitdiagnostic, and the validator'ssummary— usesadapterArgv ?? argv, so the human-readable line remains the true spawned command.adapterArgv(rejects a non-string-array), keeps the standalone assertion onargv, and extends the retired-MCP rejection to cover both vectors. There is no strict key allowlist for command records, so no allowlist change was needed.Evidence stays honest (the real docker invocation is still recorded, in its own field) and the validator stays strict.
Tests
scripts/validate-live-dogfood-evidence.test.ts— new case reshapes every stage observation into adapter form (adapter asexecutable,adapterArgv = [binary, ...argv],argv= candidate args) and asserts it validates clean; the pre-fix binary-prefixed shape still fails with the exact production message for stages 5/6/7; a non-stringadapterArgvis rejected; an MCP token smuggled intoadapterArgvis rejected.tests/support/codex-dogfood-harness.test.ts(new) — unit-coversresolveInvocation, then really spawns through a tiny fake adapter script (exec "$candidate" "$@") and provesargv == candidate args,adapterArgv == [binary, ...args],executable == adapter,candidateBinary == binary, and that the candidate's echoed argv matches.Gates:
bun run typecheck✅ ·bun run lint✅ (3 pre-existingsrc/complexity warnings, untouched) ·bun run dead-code✅ ·bun run lint:complexity-budget✅ (OK: budget intact). Pre-push hooks ran clean.Rollout note
The release-publish workflow checks out this harness and validator from
main(atgithub.sha), so this fix only takes effect after the nextdev→mainpromotion. Until that promotion lands, every stable release will keep failing at the musl dogfood gate.🤖 Generated with Claude Code
https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3