Skip to content

fix: stop-hook round counter race / round-skip bug#3

Open
OpeSodeinde wants to merge 1 commit into
promptadvisers:mainfrom
OpeSodeinde:fix/stop-hook-round-skip
Open

fix: stop-hook round counter race / round-skip bug#3
OpeSodeinde wants to merge 1 commit into
promptadvisers:mainfrom
OpeSodeinde:fix/stop-hook-round-skip

Conversation

@OpeSodeinde

Copy link
Copy Markdown

Closes #2.

Summary

The reviewing-phase handler in plugins/claudex/hooks/stop-hook.sh unconditionally incremented the round counter on every Stop event whose state was phase=reviewing AND decision_signal != 'no-material-findings'. In a multi-Stop-hook environment (project-level Stop hooks alongside the claudex one), each meta-response from Claude triggered another Stop fire while still in the reviewing phase — advancing the counter without any reviewer agent having run. Loops self-closed via the max-rounds path after as little as 30 seconds of meta-traffic, with three counter increments in a 30-second window and zero reviewer invocations between any pair.

Empirically reproducible as a unit test against the script in pure isolation — no Claude Code session, no other Stop hooks needed. The test in tests/round-skip-regression.sh proves it; full reproduction trace in issue #2.

Fix shape

  1. New helper build_review_round_block. Single source for the per-round BLOCK message. All values passed as explicit positional args so it can be called from either the round-advance branch OR the new re-fire guard (where post-increment vars are not yet set).

  2. write_runner_script now emits a sentinel. The generated runner pre-cleans any stale findings-round-N.md and findings-round-N.md.ok at start, then writes the .ok sentinel ONLY when Codex exits zero AND the findings file is non-empty.

  3. Re-fire guard. Before the unconditional increment in the reviewing branch, the guard checks for the current round's sentinel. If absent, the Stop fire is from a meta-response (sibling Stop hook, infrastructure error reply) — re-emit the same round-N block instead of advancing.

Net: the counter only advances when a round legitimately completes.

Verification

$ bash tests/round-skip-regression.sh
PASS [step1]: drafting -> reviewing, round=1
PASS [step2]: re-fire kept round=1 (no sentinel)
PASS [step3]: 3 consecutive re-fires kept round=1
PASS [step4]: sentinel present -> advanced to round=2
PASS [step5]: round-2 sentinel -> round=3, summarizing, max-reached
ALL PASS

Same test against the unpatched script: FAIL [step2]: re-fire advanced round to 2 without sentinel. Regression-bound.

The test drives setup through the real start-loop.sh + drafting → reviewing transition (rather than hand-built state) so write_runner_script produces an actual runner — confirming the BLOCK message references something that exists.

Process notes (transparent)

This fix was drafted using the buggy plugin to plan its own patch — /claudex:plan --rounds 1. Single-round was chosen deliberately because the round-skip bug only fires on multi-round loops, so round 1 takes a code path that isn't broken. Codex senior-engineer review surfaced 7 findings (2 High, 3 Medium, 2 Low) — all accepted. The two High findings drove the two biggest design changes:

  • "Non-empty findings file" was originally proposed as the completion signal. Codex caught that this is too weak: a stale, partial, or invalid file can advance the loop. Replaced with the runner-emitted sentinel, which can only land on zero-exit + non-empty assertion.
  • An earlier draft of the local-recovery script blindly copied a saved patched file over the cache. Codex caught that this could erase upstream fixes after a future plugin update. The local-recovery script is SHA-gated and refuses to overwrite drift. (That part lives downstream of this PR; mentioned for context.)

Limitations / deliberate non-goals

  • Single-session-per-repo only. Two simultaneous Claude Code sessions running plan/review loops in the same repository remain unsupported — round updates are not CAS-protected. Out of scope for this surgical fix; happy to add a follow-up if you want lock/CAS for multi-session.
  • Review-mode (/claudex:review) loop not touched. Same hook handles both modes; if review mode has the same bug class, that's a separate PR.
  • No UX polish (rounds validation, prettier banners, etc.).

Files

  • plugins/claudex/hooks/stop-hook.sh — helper extraction + sentinel-based guard + runner template change
  • tests/round-skip-regression.sh — synthetic regression test (new file, executable)

Test plan

  • bash -n syntax check passes
  • tests/round-skip-regression.sh PASSes on patched
  • Same test FAILs at step 2 on unpatched — regression-bound proof
  • Reviewer check: helper-extraction reconciles the two existing block templates correctly (the line-274 template had slightly more prose than line-445; the helper uses the line-445 form, which is what every round after the first sees anyway)

Closes promptadvisers#2

The reviewing-phase handler in plugins/claudex/hooks/stop-hook.sh
unconditionally incremented the round counter on every Stop event whose
state was phase=reviewing AND decision_signal != 'no-material-findings'.
In multi-Stop-hook environments (project-level stop hooks alongside the
claudex one), each meta-response from Claude triggered another Stop fire
while still in 'reviewing' phase -- advancing the counter without any
reviewer agent having run. Loops self-closed via the max-rounds path
after as little as 30 seconds of meta-traffic.

Empirically reproducible as a unit test against the script in pure
isolation (no other hooks, no Claude Code session). The test under
tests/round-skip-regression.sh proves it.

Fix shape:

1. New helper build_review_round_block in stop-hook.sh. Single source
   for the per-round BLOCK message; values passed as explicit positional
   args so the helper can be called from either the round-advance branch
   OR the new re-fire guard.

2. write_runner_script now generates a runner that:
   - pre-cleans stale findings outputs at runner start
   - writes findings-round-N.md.ok SENTINEL only on Codex zero-exit AND
     a non-empty findings file

3. Re-fire guard in the reviewing branch BEFORE the round-counter
   increment: if the sentinel for the current round is absent, re-emit
   the same round-N block instead of advancing.

Effect: Stop fires from sibling hooks or meta-responses no longer drain
the round budget. The counter only advances when a round legitimately
completes (Codex returned zero AND wrote findings).

Limitations and non-goals (deliberate):

- Single-session-per-repo only. Two simultaneous Claude Code sessions
  in the same repo running plan/review loops are still unsupported --
  round updates are not CAS-protected. Tracking a separate enhancement
  if multi-session is desired; this PR keeps the surgical fix.

Verification:

  $ bash tests/round-skip-regression.sh
  PASS [step1]: drafting -> reviewing, round=1
  PASS [step2]: re-fire kept round=1 (no sentinel)
  PASS [step3]: 3 consecutive re-fires kept round=1
  PASS [step4]: sentinel present -> advanced to round=2
  PASS [step5]: round-2 sentinel -> round=3, summarizing, max-reached
  ALL PASS

Same test against the unpatched script: FAIL at step 2 with
"re-fire advanced round to 2 without sentinel". Regression-bound proof.
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.

Stop hook unconditionally increments round counter in reviewing phase — race condition with Claude Code multi-turn flows

1 participant