fix(stop-review-gate-hook): always emit valid JSON on stdout#352
fix(stop-review-gate-hook): always emit valid JSON on stdout#352AliceLJY wants to merge 2 commits into
Conversation
main() returns silently in three no-decision paths (stopReviewGate=false,
setup note shown, review allow), leaving stdout empty. Newer hook
executors strict-parse stdout as JSON and report
"Stop hook (failed): hook returned invalid stop hook JSON output" after
every Claude turn. Older Claude Code tolerated empty stdout as
"no decision".
Emit `{}` before each silent return so stdout is always valid JSON.
Behavior unchanged for the existing block path which already emits
decision JSON, and stderr logNote messages are preserved.
![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: 2db16dd9ca
ℹ️ 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 (!config.stopReviewGate) { | ||
| logNote(runningTaskNote); | ||
| emitDecision({}); |
There was a problem hiding this comment.
This new no-decision output changes the observable contract of the stop hook, but the runtime tests still assert empty stdout for the disabled-gate path and the other allow paths (tests/runtime.test.mjs lines 1897, 1925, and 1950). With this line, the disabled-gate test now receives {} and fails in CI even though the hook exits successfully, so the corresponding assertions need to be updated to parse/expect the empty JSON object.
Useful? React with 👍 / 👎.
Three runtime tests asserted empty stdout for stop hook no-decision
paths. After emitting `{}` from the disabled-gate, allow-review, and
missing-setup branches, those assertions need to match the new
contract.
Addresses the P1 review note on PR openai#352 from the Codex bot.
|
Addressed the P1 review note from @chatgpt-codex-connector in commit a1020ad — updated the three Verified locally: all 5 stop-hook tests now pass. |
|
Note To use Codex here, create an environment for this repo. |
1 participant
Summary
scripts/stop-review-gate-hook.mjsreturns silently in three no-decision paths inmain():if (!config.stopReviewGate)— the default, hit on every Stop event when the user has not opted into the review gateif (setupNote)— Codex not yet set upALLOWIn all three,
logNote()writes to stderr but stdout is never touched. Newer hook executors (observed in current Claude Code / Codex CLI builds) strict-parse hook stdout as JSON. Empty stdout is no longer treated as "no decision" — it surfaces as a visible error after every Claude turn:Older Claude Code tolerated empty stdout, which is why this only became visible recently.
Fix
Emit
{}(a no-decision JSON object) before each silentreturn. Three-line change, no behavior change other than satisfying the strict parser. The block path is untouched — it already emits a{decision: "block", reason}object.Test plan
echo '{"hook_event_name":"Stop","session_id":"test","stop_hook_active":false}' | node scripts/stop-review-gate-hook.mjs→ stdout is{}, exit 0JSON.parse(stdout)succeedslogNotemessages preserved (no regression in user-facing diagnostics)decision: "block"path unchangedRelated issues / context
I searched open issues and PRs for
stop-review-gate,empty stdout,hook returned invalid,JSON output— none describe this specific defect. The closest are:parseStopReviewOutput, but only the gate-enabled pathsBoth are orthogonal to this fix.
🤖 Generated with Claude Code