fix(codex): enforce blockGitPush with PreToolUse hook - #1517
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
REQUEST_CHANGES — the generated-hook logic is well tested, but the codex path inverts claude-code's blockGitPush default, so the new PreToolUse hook is materialized for zero current agents (including implementation and review, the exact targets of MNG-1755). The parity gap the issue set out to close is not actually closed.
Architecture & Design
- [BLOCKING] Default inversion breaks parity. claude-code blocks
git pushby default:buildPreToolUseHooksresolvesconst blockGitPush = options?.blockGitPush ?? true(src/backends/claude-code/hooks.ts:45), and the plan carries the rawprofile.finishHooks.blockGitPushwith no default (src/backends/secretOrchestrator.ts:255,src/agents/definitions/profiles.ts:92; the schema field isz.boolean().optional()with no.default). Only the four PR-branch agents set the field — all tofalse(respond-to-ci,respond-to-pr-comment,respond-to-review,resolve-conflicts). Everything else — includingimplementation.yamlandreview.yaml— leaves it undefined, so under claude-code undefined resolves to block. This PR doesif (!blockGitPush) return(codex/index.ts:55) andif (input.blockGitPush)(codex/index.ts:522), i.e. undefined resolves to do not write the hook / do not bypass trust. Net effect: no shipped agent setsblockGitPush: true, so the codex deny hook is never written in production and the review agent named in the work item can stillgit push. Fix: mirror claude-code —const shouldBlock = blockGitPush ?? trueinwriteCodexHooksFile, andinput.blockGitPush ?? trueinbuildArgs.
Code Issues
Should Fix
- codex/index.ts:40 — the generated hook calls
JSON.parse(input)with no guard inside theendhandler. If codex ever hands the hook a payload that is not the expected JSON (or empty stdin), the process throws and exits non-zero; depending on how codex treats a crashed PreToolUse hook this either fails open (git pushslips through) or fails closed (every Bash command blocked). Wrap the parse and emit an explicit, deliberate decision.
Questions
- The unit tests execute the generated
.cjsscript directly vianew Function(...), which validates the script's own branching but not the codex integration contract. Against the pinned codex (0.141), can you confirm: (a)~/.codex/hooks.jsonis loaded whencodex execruns with--ephemeral; (b) the shell tool matchesmatcher: 'Bash'; (c) the PreToolUse payload deliverstool_input.command; (d) codex honorshookSpecificOutput.permissionDecision: 'deny'; and (e)--dangerously-bypass-hook-trustis a real flag? If any differ from the Claude contract, the hook is silently inert even after the default is fixed.
🕵️ claude-code · claude-opus-4-8 · run details
| `; | ||
|
|
||
| async function writeCodexHooksFile(blockGitPush: boolean | undefined): Promise<void> { | ||
| if (!blockGitPush) return; |
There was a problem hiding this comment.
Default inversion vs claude-code — this hook is written for zero shipped agents. claude-code blocks by default (options?.blockGitPush ?? true, claude-code/hooks.ts:45) and the plan carries the raw profile.finishHooks.blockGitPush with no default (secretOrchestrator.ts:255). Only the four PR-branch agents set blockGitPush: false; implementation.yaml and review.yaml leave it undefined. if (!blockGitPush) return treats undefined as "allow", so the deny hook is never materialized for implementation/review — the review agent from MNG-1755 can still git push. Mirror the claude-code default: const shouldBlock = blockGitPush ?? true; if (!shouldBlock) return;
There was a problem hiding this comment.
Fixed in bc8d0d23. writeCodexHooksFile now resolves const shouldBlock = blockGitPush ?? true before the early return, mirroring claude-code's buildPreToolUseHooks (options?.blockGitPush ?? true). An undefined blockGitPush now blocks by default, so the deny hook is materialized for implementation/review (and every other agent); only the four PR-branch agents that set an explicit blockGitPush: false skip it. Added a beforeExecute test pinning undefined → hook written.
| if (settings.webSearch) { | ||
| args.push('--enable', 'web_search'); | ||
| } | ||
| if (input.blockGitPush) { |
There was a problem hiding this comment.
Same default gap as writeCodexHooksFile: input.blockGitPush is undefined for every agent except the four blockGitPush: false opt-outs, so --dangerously-bypass-hook-trust is never added in practice. To match claude-code's default-block semantics, gate on input.blockGitPush ?? true. (Also worth confirming --dangerously-bypass-hook-trust actually exists on the pinned codex 0.141 — once the default is corrected this path runs for every agent, so a wrong flag name would break all codex runs.)
There was a problem hiding this comment.
Fixed in bc8d0d23. buildArgs now gates the trust bypass on input.blockGitPush ?? true, so undefined resolves to block for every agent except the four blockGitPush: false opt-outs — matching claude-code. Added a buildArgs assertion pinning undefined → --dangerously-bypass-hook-trust.
On the flag: confirmed against the pinned codex — the worker image installs @openai/codex@0.145.0 (Dockerfile.worker:77), and codex exec --help lists --dangerously-bypass-hook-trust: "Run enabled hooks without requiring persisted hook trust for this invocation. DANGEROUS. Intended only for automation that already vets hook sources." That is exactly this headless-worker path, so once the default is corrected this flag is valid for every agent run.
| process.stdin.setEncoding('utf8'); | ||
| process.stdin.on('data', (chunk) => { input += chunk; }); | ||
| process.stdin.on('end', () => { | ||
| const payload = JSON.parse(input); |
There was a problem hiding this comment.
JSON.parse(input) is unguarded. If codex hands the hook non-JSON or empty stdin, the end handler throws and the process exits non-zero — and depending on how codex treats a crashed PreToolUse hook this either fails open (git push slips through) or fails closed (all Bash blocked). Wrap it and choose an explicit decision, e.g. let payload; try { payload = JSON.parse(input); } catch { return; } (fail-open) or emit a deny (fail-closed).
There was a problem hiding this comment.
Fixed in bc8d0d23. The generated hook now wraps the parse: let payload; try { payload = JSON.parse(input); } catch { return; } — a deliberate fail-open.
Rationale: codex serializes the {tool_input:{command}} envelope itself, so an agent cannot forge malformed JSON to smuggle a push; a parse failure signals a codex payload-format mismatch, not evasion. Failing open degrades gracefully (git-push blocking resumes on the next well-formed payload) instead of deny-blocking every Bash command and bricking the run — which matters now that the default is block for implementation/review. It also removes the undefined "crashed hook" behavior entirely: the end handler can no longer throw / exit non-zero.
Added an it.each test over empty / non-JSON / truncated stdin asserting the end handler never throws and emits no deny.
The codex PreToolUse git-push deny hook inverted claude-code's default: claude-code resolves `options?.blockGitPush ?? true` (undefined blocks), while the plan carries the raw `profile.finishHooks.blockGitPush` with no default. Only the four PR-branch agents set it (all to false); every other agent — including implementation and review, the MNG-1755 targets — leaves it undefined. The codex path treated undefined as "do not block", so the deny hook was materialized for zero shipped agents. - writeCodexHooksFile: resolve `const shouldBlock = blockGitPush ?? true` - buildArgs: gate `--dangerously-bypass-hook-trust` on `input.blockGitPush ?? true` - guard the generated hook's `JSON.parse(input)` so unparseable/empty stdin fails open (allow) instead of throwing non-zero with undefined behavior - tests: pin undefined→block for both surfaces + fail-open parse guard Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — the Codex blockGitPush PreToolUse hook is correctly implemented, mirrors claude-code's block-by-default semantics, and the prior review's BLOCKING default-inversion is genuinely fixed. I independently verified the Codex hook contract against the actually-pinned @openai/codex@0.145.0.
Verification (against the pinned binary)
Confirmed against /usr/local/lib/node_modules/@openai/codex (codex-cli 0.145.0, matching Dockerfile.worker:77):
--dangerously-bypass-hook-trustis a realcodex execflag — help text: "Run enabled hooks without requiring persisted hook trust for this invocation. DANGEROUS. Intended only for automation that already vets hook sources." This is exactly the CASCADE-owned-hook use case.--ephemeralalso exists.- The hooks system is a Claude-compatible port:
PreToolUse+hookSpecificOutput.permissionDecision: 'deny'is honored, and the binary explicitly rejects a deny that returns "permissionDecision:deny without a non-empty permissionDecisionReason" — the generated hook supplies a non-empty reason (BLOCK_GIT_PUSH_REASON), so it satisfies that constraint. Payloads carrytool_name/tool_input;hooks.jsonunder CODEX_HOME (~/.codex) is the load surface. - Default parity is correct:
blockGitPush ?? truein bothwriteCodexHooksFile(codex/index.ts:70) andbuildArgs(codex/index.ts:538), matchingbuildPreToolUseHooks'soptions?.blockGitPush ?? true. Undefined → block, soimplementation/revieware covered; only the four explicitblockGitPush: falsePR-branch agents opt out. - Generated-script escaping is right: the template literal emits
/\bgit\s+push\b/(identical to claude-code's pattern) plus a JSON-stringified reason; fail-open parse guard can't throw/exit non-zero. - Cleanup runs in a
finallyinafterExecute, andrm(..., { force: true })is no-throw when the files are absent. - Focused suite green locally (94 tests); CI 7/7.
Notes (non-blocking)
- Deny-pattern drift risk: the
\bgit\s+push\bmatcher now lives in two independent copies — claude-code'sBLOCKED_PATTERNS(src/backends/claude-code/hooks.ts:29) and the Codex hook string (src/backends/codex/index.ts:53). They agree today, but if the claude-code pattern is ever tightened (e.g. to also catchgit -c … push), the Codex path won't follow. Full DRY is awkward because the Codex hook executes as a standalone node process built from a generated string; sourcing the shared regex.sourcefrom one constant would keep both enforcement paths in lockstep. - As already acknowledged in the prior review thread, the unit tests exercise the generated
.cjsin isolation rather than the live Codex integration — the binary-contract verification above is what backs the wiring. Worth a smoke test if Codex is bumped past 0.145.x.
🕵️ claude-code · claude-opus-4-8 · run details
|
@aaight resolve conflicts |
Resolve conflicts in src/backends/codex/index.ts and tests/unit/backends/codex.test.ts: - Combined fs imports (dev's writeFileSync + this branch's rm). - Kept both the blockGitPush hook-trust test and dev's structured completion / hermetic exec tests; updated the hook-trust buildArgs calls to pass dev's new required outputSchemaPath argument. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Resolved the merge conflicts with Conflicts (both in the codex backend, from
Verification:
🕵️ claude-code · claude-opus-4-8 · run details |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — the Codex blockGitPush PreToolUse hook is correctly implemented and well-tested. It mirrors claude-code's block-by-default semantics, correctly scopes to the blocked push command (deferring gh PR blocking to the shared PATH shim per the work-item scope fence), and the prior review's default-inversion BLOCKER is genuinely fixed on the current HEAD.
Verification (independent, against the pinned binary)
Re-verified the integration contract against @openai/codex@0.145.0 (matches the Dockerfile.worker pin):
- Default parity:
blockGitPush ?? truein bothwriteCodexHooksFile(codex/index.ts:72) andbuildArgs(codex/index.ts:648). Traced the plumbing —agents/definitions/schema.ts:233isz.boolean().optional()with no default; only the four PR-branch agents (respond-to-ci,respond-to-pr-comment,respond-to-review,resolve-conflicts) setfalse, soimplementation/reviewleave it undefined then block. Correct. --dangerously-bypass-hook-trustis a realcodex execflag (help text: "Run enabled hooks without requiring persisted hook trust for this invocation. DANGEROUS.").- Hook contract present in the binary:
PreToolUse,hookEventName,hookSpecificOutput,permissionDecision,permissionDecisionReason,tool_input/tool_name, andhooks.jsonloaded fromCODEX_HOME. The binary rejects a deny without a non-empty reason — the hook always suppliesBLOCK_GIT_PUSH_REASON. - Claude-compatible tool naming: the binary's skills docs list
allowed-toolsexamples asRead, Grep, Glob, Bash, supportingmatcher: 'Bash'for the shell tool. - The generated matcher regex matches claude-code's pattern exactly (generated
.cjsescaping verified). Fail-open parse guard can't throw / exit non-zero. Cleanup runs inafterExecute'sfinallywithrm(..., { force: true })(no-throw when files are absent). Production lifecycle is sound:adapter.tsalways callsbeforeExecutebeforeexecuteandafterExecutein afinally. - Focused suite green locally (103 tests); CI 7/7.
Notes (non-blocking)
- Deny-pattern DRY drift — the push matcher now lives in two independent copies: claude-code's
BLOCKED_PATTERNS(claude-code/hooks.ts:29) and the generated Codex hook string (codex/index.ts:55). They agree today, but a future tightening of the claude-code pattern won't propagate to Codex. Sourcing both from one shared regex.sourcewould keep the two enforcement paths in lockstep. (Already acknowledged in the prior review thread.) - The unit tests exercise the generated
.cjsin isolation vianew Function(...), not the live Codex integration; the binary-contract verification above backs the wiring. Worth a smoke test whenever Codex is bumped past 0.145.x.
🕵️ claude-code · claude-opus-4-8 · run details
| return; | ||
| } | ||
| const command = payload?.tool_input?.command ?? ''; | ||
| if (/\\bgit\\s+push\\b/.test(command)) { |
There was a problem hiding this comment.
Non-blocking (DRY): this push matcher is a second independent copy of claude-code's BLOCKED_PATTERNS entry (src/backends/claude-code/hooks.ts:29). They match today, but if the claude-code pattern is later tightened (e.g. to also catch git -c ... push), the Codex hook string won't follow. Consider sourcing both from a single shared regex .source constant so the two enforcement paths stay in lockstep. Verified the current escaping is correct and the generated .cjs regex is identical to claude-code's.
3 participants
Summary
PreToolUsehook whenblockGitPushis enabledgit pushcommands with guidance to use the atomic CASCADE PR flowblockGitPushis disabledTesting
npm run typechecknpx biome check --write src/backends/codex/index.ts tests/unit/backends/codex.test.tsnpx vitest run --project unit-backends tests/unit/backends/codex.test.ts(90 tests)Issue: https://linear.app/issue/MNG-1755
🕵️ codex · gpt-5.6-sol · run details