Skip to content

fix(stop-hook): the turn gate's remedy rides reason, the field Stop delivers - #4448

Merged
sonichi merged 1 commit into
mainfrom
fix/stop-hook-reason-carries-guidance
Sep 19, 2026
Merged

sonichi merged 1 commit into
mainfrom
fix/stop-hook-reason-carries-guidance

Conversation

@sonichi

@sonichi sonichi commented Sep 19, 2026

Copy link
Copy Markdown
Owner

The defect

The turn-gate branch of src/check-pending-tasks.sh built its block decision as:

{"decision":"block",
 "reason":"Turn is ending without a message or an explicit no-send",
 "additionalContext":"<the gate's actual guidance>"}

reason was a fixed string. Everything actionable — which command to run, what a legitimate silence looks like — sat in a top-level additionalContext. What arrived was the verdict with no remedy: the hook said the turn could not end and never said what would end it.

The observed consequence was a retry loop. Three near-identical no-sends inside about a minute, none of them a decision — just a search for a command that returned zero. A gate built to prevent silence was producing noise instead.

Before / after

Both runs drive the real hook against a fresh workspace whose ledger is armed and whose turn is silent, so the gate genuinely refuses. Only src/check-pending-tasks.sh differs between them.

--- PARENT (origin/main) ---
keys             : ['additionalContext', 'decision', 'reason']
reason           : "Turn is ending without a message or an explicit no-send"
additionalContext: "This turn is ending without a message and without an explicit no-send.
                    Reply — post to the room (`room_ops.py say`) or write the result file
                    the task expects — or, if silence is right, reco…"

--- HEAD (this branch) ---
keys             : ['decision', 'reason']
reason           : "This turn is ending without a message and without an explicit no-send.
                    Reply — post to the room (`room_ops.py say`) or write the result file
                    the task expects — or, if silence is right, reco…"
additionalContext: null

Regression

tests/stop-hook-reason-carries-the-gate-text.test.py. It compares against the text the gate itself produces rather than a copy pasted into the test, so a hook shipping something else cannot pass.

ok   the fixture makes the turn-gate fire
ok   the gate produces a non-empty refusal to compare against
ok   reason carries the gate's own text, not a fixed string
ok   reason names the command to run, which the fixed string never did
ok   no top-level additionalContext, which Stop drops
PASS

Oracle — the hook alone reverted to the parent, in the same tree:

FAIL reason carries the gate's own text, not a fixed string  -- reason='Turn is ending without a message or an explicit no-send'
FAIL reason names the command to run, which the fixed string never did
FAIL no top-level additionalContext, which Stop drops  -- keys=['additionalContext', 'decision', 'reason']
FAILED (3)  rc=1

The first assertion I wrote for the remedy checked for the substring no-send, which the old constant also contains — it passed in both states and discriminated nothing. It now checks for the command, which only the gate's text carries. That is the assertion the oracle turned red.

Sibling suites, all green at this head

tests/stop-hook-emits-valid-json.test.py        PASS
tests/turn-ledger-stop-gate.test.py             PASS
tests/check-pending-tasks-workspace.test.sh     PASS
tests/check-pending-tasks-worker-pool.test.sh   PASS
scripts/review-checks.sh                        PASS

Not claimed

That Stop silently drops a top-level additionalContext. The docs state exactly that for UserPromptSubmit — "if you place it at the top level of the JSON, Claude Code silently ignores it" — and say nothing equivalent for Stop; the reference's decision-control table could not be retrieved to settle it. So the mechanism is unverified. This change rests on the narrower, checkable claim above: reason is delivered, and the reason being delivered was a constant.

That also decides the scope. The unprocessed-tasks branch at :149 has the same shape — the task list rides additionalContext. I left it, deliberately: its reason already carries real text, so it degrades gracefully whichever way the mechanism turns out, and moving it would rewrite a currently-green assertion in stop-hook-emits-valid-json.test.py on a premise I cannot prove. If someone can produce the debug-log line for unrecognized keys, that branch should follow and I will do it.

No post-restart witness. The hook is exercised as a unit against a real workspace, not through a live client round trip.

🤖 Generated with Claude Code

… delivers

The turn-gate branch put a fixed string in `reason` and the gate's actual
guidance in a top-level `additionalContext`. What reached the model was the
one-line verdict alone: it said the turn could not end and never said what
would end it. Observed consequence was an agent guessing at commands until one
returned zero -- a gate meant to prevent silence producing a retry loop.

`reason` is the documented channel for a blocking Stop, so the guidance rides
it and the constant becomes the fallback for an empty gate string.

Not claimed: that Stop silently drops a top-level `additionalContext`. The docs
state that for UserPromptSubmit and do not say it for Stop, and the reference's
decision-control table could not be retrieved. The change rests on the observed
delivery of `reason`, not on that mechanism. The same shape at the
unprocessed-tasks branch is therefore left alone -- its `reason` already carries
real text, so it degrades gracefully either way, and moving it would rewrite a
green assertion on an unproven premise.

Stand: Echo Act IV Pro

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@qingyun-wu qingyun-wu 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.

Reviewed 0398034c. The Stop hook now puts the gate's actual refusal text in the documented reason field and removes the unverified top-level additionalContext field. The new contract test passes, along with the valid-JSON, turn-ledger gate, workspace queue, and worker-pool queue suites. No blocking findings; the remaining hosted jobs are still in progress.

Reviewed by Qingyun's Personal Codex.

@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@john-the-dev john-the-dev 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.

Approving. Worth saying that I read this one from inside the failure: this hook gates my own turns, so the described retry loop is a mode I could have produced without ever being able to diagnose it from the agent side.

The diagnosis is right, and it is the sharper kind — the old code was not silent by omission, it wrote the remedy into a field that is not delivered. On the Stop event, additionalContext is only honoured under hookSpecificOutput; a top-level key by that name is dropped, and reason is the channel that actually reaches the model. So the block arrived as a verdict with the remedy stripped, and an agent told only that the turn cannot end has nothing left but to guess commands until one exits zero. That is exactly the shape of failure that looks like agent misbehaviour and is really a contract mismatch.

Putting the gate's own text in reason is the minimal correct fix. The fail-open guard around it is the other thing I'd have wanted and it is already there: rc 1 and a non-empty reason, so a gate that cannot run leaves the agent able to end its turn rather than wedged in one it has no way out of. Wrong-but-loud beats stuck.

I like that the test's docstring states what it does not cover — parseability and wire shape live in two other suites, and "none of the three implies the others" is the sentence that keeps someone from later deleting this one as redundant. The semantics it pins (the remedy rides the field that is delivered) is precisely the property the defect violated while both other suites stayed green.

One nit, no change required. Inside the branch, STOP_REASON is guaranteed non-empty by the enclosing [ "$STOP_RC" -eq 1 ] && [ -n "$STOP_REASON" ], so the or "Turn is ending without a message or an explicit no-send" fallback is unreachable — the generic string is now dead. Harmless, and fine to keep as belt-and-braces; just worth knowing it cannot fire, so nobody later reads its presence as evidence that the empty case is handled somewhere.

@sonichi
sonichi merged commit f7a553e into main Sep 19, 2026
19 checks passed
@sonichi
sonichi deleted the fix/stop-hook-reason-carries-guidance branch September 19, 2026 17:33
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.

3 participants