Skip to content

fix(codex): put exec options before the resume subcommand - #1553

Merged
zbigniewsobiecki merged 2 commits into
devfrom
fix/codex-resume-arg-order
Sep 21, 2026
Merged

zbigniewsobiecki merged 2 commits into
devfrom
fix/codex-resume-arg-order

Conversation

@zbigniewsobiecki

Copy link
Copy Markdown
Member

Problem

When a completion check fails after the first turn (e.g. a respond-to-pr-comment run that answered a question without pushing a commit), the Codex backend resumes the recorded thread. The generated command was:

codex exec resume <id> --json … -C <dir> -m <model> -s <mode> …

-C/--cd and -s/--sandbox are parent-only codex exec options (not clap globals), so the pinned CLI rejects the whole invocation before contacting the model:

error: unexpected argument '-C' found
Usage: codex exec resume [OPTIONS] [SESSION_ID] [PROMPT]

The run is then recorded as failed even though the first turn's work (e.g. the posted PR comment) already landed. Introduced in #1521, whose unit test pinned the broken ordering against a mocked spawn.

Fix

buildArgs now emits every option first and the subcommand last:

codex exec --json … -C <dir> -m <model> -s <mode> … resume <id> -

Verification

  • Fed the real buildArgs output to @openai/codex@0.145.0 (the worker pin): it now fails only at thread lookup (no rollout found for thread id …, exit 1), not in argument parsing. Same grammar holds on 0.155.1.
  • Unit tests pin the structure (exec first, option set identical to the initial turn, resume <id> - last) instead of the old ordering.
  • npm run verify green.

Follow-ups (separate PRs)

  • Codex bump to 0.155.1 + gpt-6-astra in the model dropdown (with a build-time parser smoke check).
  • Spec 025: outcome-aware completion so a comment-only PR reply completes without a push.

🤖 Generated with Claude Code

Continuation turns resumed a Codex thread with
`codex exec resume <id> ... -C <dir> ... -s <mode> ...`. In the pinned CLI
`-C`/`--cd` and `-s`/`--sandbox` are parent-only `exec` options, so
clap rejected the resume with "unexpected argument '-C'" before the model
was reached and the run was recorded as failed. Emit every option first and
the subcommand plus stdin marker last: `codex exec <options> resume <id> -`.

Verified the generated argv against @openai/codex@0.145.0: it now fails only
at thread lookup ("no rollout found"), not in argument parsing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@nhopeatall nhopeatall left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

APPROVE — correct, minimal bug fix. buildArgs now emits all codex exec options before the resume subcommand, matching the pinned CLI grammar (codex exec [OPTIONS] resume [SESSION_ID] [PROMPT]) so continuation turns are no longer rejected with error: unexpected argument '-C' found.

Verification

  • Fix is correct: resume <sessionId> moves from position 2 (immediately after exec) to just before the trailing - stdin marker. Parent-only exec options (-C, -s, -m, -o, --output-schema, -c …) now precede the subcommand, and the - PROMPT positional stays last — the prompt is delivered via child.stdin.write(promptText) regardless of turn, so the stdin mechanism is unchanged.
  • No regression on the initial turn: with sessionId undefined the output is exec --json … -, byte-for-byte identical to the prior behavior (the old resume insertion was already conditional).
  • Resume path is real: runState.sessionId is populated from the thread.started event (src/backends/codex/index.ts:397) and fed to buildArgs on continuation turns.
  • Tests: ran tests/unit/backends/codex.test.ts → 104/104 passing. The new keeps every exec option ahead of the resume subcommand test asserts the resume option set equals the initial-turn option set and that resume <id> - is last; the CodexEngine mock also confirms -o output still resolves in the resume invocation.
  • Docs: CHANGELOG.md and docs/architecture/05-engine-backends.md accurately describe the new ordering.

No blocking or should-fix issues. CI was still in progress at review time; the code change itself is sound and locally verified.

🕵️ claude-code · claude-opus-4-8 · run details

js-yaml 4.0.0-4.3.1 is affected by GHSA-2883-xcg3-v3hh (maxTotalMergeKeys
does not limit CPU use for empty merge sources), which fails the CI
'npm audit --omit=dev --audit-level=high' step. Bump the direct
dependency to ^4.3.2 and add a matching override so all transitive
resolutions (via @commitlint, llmist) pick up the patched version.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@nhopeatall nhopeatall left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

APPROVE — correct, minimal fix that reorders the Codex resume invocation so every parent-only codex exec option (-C/--cd, -s/--sandbox, and the rest) precedes the resume subcommand, with tests that pin the right structure and strong verification against the pinned CLI.

Verification

  • Fix is sound and isolated. buildArgs is the only constructor of the resume command (rg 'resume' across src/ confirms), so there is no second code path still carrying the old exec resume <id> … ordering. The - stdin prompt marker stays last and the child is spawned with stdio: ['pipe','pipe','pipe'], so prompt delivery is unchanged.
  • No regression risk. The prior ordering was rejected at argument parsing (error: unexpected argument '-C'), i.e. resume never worked at all; the new ordering is a strict improvement and the author validated it parses on both @openai/codex@0.145.0 (worker pin) and 0.155.1, failing only at thread lookup.
  • Tests are meaningful. resumeArgs.slice(1, resumeIndex) === initialArgs.slice(1, -1) asserts the resume turn carries the identical exec-level option set as the initial turn — exactly the invariant that #1521 broke. Ran vitest run --project unit-backends tests/unit/backends/codex.test.ts → 104/104 pass. CHANGELOG + 05-engine-backends.md note accurately describe the new grammar.

Non-blocking note

  • The PR bundles a second, unrelated commit (b98c10d6) bumping js-yaml 4.3.1 → 4.3.2 in dependencies + overrides to clear a high-severity npm audit (GHSA-2883-xcg3-v3hh). The change itself is clean and consistent (js-yaml is a real direct dep used across src/), but it isn't mentioned in the PR description — worth a one-line callout there for reviewers/changelog traceability. Not a blocker.

🕵️ claude-code · claude-opus-4-8 · run details

@zbigniewsobiecki
zbigniewsobiecki merged commit e4aabca into dev Sep 21, 2026
9 checks passed
bychkov pushed a commit to bychkov/cascade that referenced this pull request Sep 26, 2026
Outcome-aware PR-comment completion (plans 1-3). The Codex resume half of
the spec (ACs 9 and 10) shipped ahead of planning in mongrel-intelligence#1553 and mongrel-intelligence#1554; the
coverage map records that.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants