Skip to content

fix(codex): avoid review prompt with --base#1209

Open
jbetala7 wants to merge 1 commit intogarrytan:mainfrom
jbetala7:oss/fix-1196-codex-review-base
Open

fix(codex): avoid review prompt with --base#1209
jbetala7 wants to merge 1 commit intogarrytan:mainfrom
jbetala7:oss/fix-1196-codex-review-base

Conversation

@jbetala7
Copy link
Copy Markdown
Contributor

Summary

  • remove the unsupported codex review <PROMPT> --base <base> shape from /codex, /review, and /ship
  • move the diff scope into the prompt so Codex runs git diff origin/<base>...HEAD itself
  • regenerate affected skill docs and host golden fixtures
  • add regression coverage for the prompt-scoped review form

Fixes #1196.

Testing

  • bun run gen:skill-docs --host all
  • bun test test/gen-skill-docs.test.ts
  • bun test test/host-config.test.ts
  • bun test test/skill-validation.test.ts --test-name-pattern "codex review invocations"

Note: bun test test/gen-skill-docs.test.ts test/skill-validation.test.ts test/host-config.test.ts still hits an unrelated existing guard failure because tracked fixture browse/test/fixtures/security-bench-haiku-responses.json is 27MB and exceeds the 2MB limit.

@jbetala7 jbetala7 force-pushed the oss/fix-1196-codex-review-base branch from fc5c02f to 9463b75 Compare April 29, 2026 06:07
@habassa5
Copy link
Copy Markdown

habassa5 commented May 5, 2026

Empirical confirmation from a downstream user — this is a real and recurring failure mode.

We've hit the silent stall ~5 times across PRs in our project (claude-teams-bot) when running /codex review from gstack. Root-caused 2026-05-04 to the same parse error described in #1196 (Codex CLI 0.124+ conflicts_with between [PROMPT] and --base). The fix in this PR — pushing diff scope into the prompt so Codex runs git diff origin/<base>...HEAD itself — works locally and matches the canonical resolution.

Currently maintaining a hand-patched ~/.claude/skills/codex/SKILL.md with the same change (drops --base, prompt-scopes the diff). Local patch gets blown away by every gstack-upgrade (git reset --hard origin/main), so this PR landing would close that recurring tax.

A few notes from the downstream perspective:

  1. The PR's broader scope (/codex, /review, /ship) is correct — we initially patched only /codex and didn't realize /review and /ship were silently affected too. Catching all three at once is a real win.
  2. The regression test (test/skill-validation.test.ts --test-name-pattern "codex review invocations") is exactly the right place to lock this in. Future Codex CLI breaking changes that modify the supported arg shape will be caught at gstack release time rather than in users' CI.
  3. The 27MB fixture issue mentioned in the testing note appears unrelated to this PR's scope — would suggest splitting that into its own follow-up rather than blocking fix(codex): avoid review prompt with --base #1209 on it.

Happy to test against this branch from our worktrees if you'd like a downstream confirmation before merge. Any timeline thoughts on landing this?

Filing a related defensive recommendation as a separate issue (the wrapper currently swallows non-zero exits other than 124, which is what made this failure mode look like a silent stall in the first place — different problem from the parse error itself, but same observable symptom).

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.

Upstream bug report — gstack-codex emits incompatible codex review flags

2 participants