Skip to content

Commit c32b9dc

Browse files
d-morrisonclaude
andauthored
hooks: catch a terminal PR-readiness claim relayed from a subagent report, or phrased outside the guard vocabulary (#3475)
* Stop hook: fix subagent-relevance scoping, source_note derivation, and citations (ai-config#3472) Extends no-incomplete-check-enumeration.py to WARN on broader merge-readiness vocabulary and on a terminal claim resting solely on a dispatched subagent's own report. Addresses five findings from an adversarial review of the staged extension: 1. BLOCKING REGRESSION: subagent relevance was scoped to the whole transcript, so any unrelated earlier Agent dispatch silently downgraded the original BLOCK case to a WARN. Now scoped to the claim's own evidence window (after the CI reading in play, or a matching PR target). 2. The WARN message's source_note was chosen by an independent predicate that could disagree with what actually fired, producing a false explanation. Now derived directly from the firing condition. 3. Wrong citations (ai-config#3468, d-morrison/macros#87 as if they were write-ups). Fixed to cite ai-config#3472 as the write-up, naming the other two as the subject PRs where the claims were made in chat. 4. RX_DECLARE_MERGE_READY had no PR/git anchoring, so an ordinary data-frame merge sentence could false-positive. Now requires a nearby #N/PR/branch token. 5. Removed unreachable dead-code branch in the test suite's classify(). Registered in hooks.json but NOT activated as a new hook -- it already governs an existing, currently-active hook, so no separate activation gate applies; this PR only changes that hook's already-active behavior on merge. Closes #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: isolate the matches_target arm of _relevant_last_subagent Review of d1673c2 found the branch untested: deleting it outright (`matches_target = False`) left all 34 cases passing, so its documented behaviour was unverified and could have been inverted or reading the wrong PR set without the suite noticing. Every existing subagent-relevance case varies only the TIMING of the dispatch, so all of them are decided by `in_window` alone. Nothing exercised the other arm. Adds SAME_PR_AGENT_DISPATCH / SAME_PR_AGENT_REPORT and a case pairing them: a subagent dispatched about the SAME PR as the claim, landing BEFORE the CI reading. It is outside the timing window, so `in_window` is False, and only `matches_target` can carry it. Its shape is identical to the unrelated-#9999 case directly above, which blocks -- the PR reference is the single difference between them. Mutation-checked, which is the point of the case: matches_target = False -> 34/35, FAIL warn (got block) restored -> 35/35 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): stop the WARN text asserting things the transcript contradicts Three fixes from the review of 1e3bfcf (#3475), plus the generated plugin copy that CI caught out of date. **Finding 2 -- the scoping fix was inert wherever it mattered most.** `in_window = idx > last_partial` reads `idx > -1` when there is no CI reading anywhere in the transcript, which is true for every event. So a transcript with no partial reading re-admitted the entire transcript through the very branch written to scope it, and an unrelated dispatch for #9999 made a later claim about #123 look subagent-sourced. Requires a reading to exist before there is a window to be inside. **Finding 1 -- the WARN blamed the subagent for a push's staleness.** The subagent reason fired whenever a relevant event existed, without asking whether the subagent is why the claim is uncovered. On AGENT_DISPATCH, AGENT_REPORT, CHECKER, PUSH the session *did* run the complete instrument, after the report -- so "not a reading you ran yourself" is false, and the push is the real cause. Gated on `last_subagent > last_complete`, with a distinct push-staleness reason for the other case. This suite had that scenario baked in backwards: its CONTENT_CASES asserted the subagent text was correct there. Corrected, and paired with a case where the report IS the last evidence, so each reason is pinned to the situation it is true in. Mutation-checked, each against the restored 37/37: in_window back to `idx > last_partial` -> 36/37 subagent reason ungated -> 36/37 push-staleness reason removed -> 36/37 Also regenerates skills/ai-config-hooks/hooks/hooks.json, which `gen-hooks-plugin.py --check` failed on in CI. Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): give the tie a reason, so no WARN ships an empty explanation Round-2 review of e9eaeb5 found the same defect class the previous commit claimed to have closed, one case further out. A push and a complete instrument read in the SAME turn -- two tool_use blocks in one message, or one `git push && check-pr-fully-clean.py` command -- land on the same transcript index. Both reason guards are strict `>`, so at a tie neither fires; with `hit_core` true the vocabulary reason does not fire either, and `source_note` comes out empty. The message then had a blank gap where its explanation belongs, and its headline asserted no instrument run existed "since the most recent push" when one had run in that very turn. Three changes: - A tie reason. It says what is actually true: the transcript cannot order two events in one turn, so re-run the instrument alone. - The headline now says "postdates" rather than "since", which stays true at a tie. - The push reason drops its `last_complete >= last_subagent` conjunct. Paired with the subagent guard's `>`, that made the two mutually exclusive, so the adjacent comment's "both apply when both reasons are in play" could never happen. Both can now fire when both are true. Also drops the `and last_partial >= 0` conjunct from `is_original_ci_case`, which the guard above already forces, and covers `your call to merge`, an alternative deletable with the suite green. Mutation-checked against the restored 39/39: tie reason removed -> 38/39 `your call to merge` removed -> 38/39 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): derive the WARN reason from an argmax, not from independent guards Three review rounds each found another transcript where the WARN text asserted something the transcript contradicts, and each was patched with one more guard. Round 3's was a partial CI reading landing AFTER a subagent's report: `last_subagent > last_complete` held, so the message still said the subagent's report was "the most recent evidence in this transcript", while a fresher reading sat right there. Tightening that guard to `last_subagent > max(last_complete, last_partial)` just resurfaces round 2's empty-note bug on the same transcript -- no reason fires at all. That is the tell: the shape is wrong, not the thresholds. Independent predicates over four indices have no reason to be exhaustive, and nothing makes them agree with each other. So derive the reason from which evidence is actually newest: evidence = [(last_subagent, "subagent"), (last_partial, "partial"), (last_push, "push"), (last_complete, "complete")] newest = max(v for v, _ in evidence) kinds = {k for v, k in evidence if v == newest and v >= 0} A complete read tied with something it would have to postdate is the tie case. Otherwise every kind at the newest index contributes its reason, so the note names exactly what the transcript holds -- and ties between two non-complete kinds name both rather than picking one. An argmax cannot be empty and cannot name evidence that is not the newest, so both failure modes are structural rather than patched. Adds the missing partial-reading reason, which had no WARN-path text at all -- the gap that let round 3's transcript fall through to the subagent. Verified against the reviewer's own reproducer: subagent report, then `gh pr checks 42`, then "PR #42 is ready to merge." now names the short CI surface and does not blame the subagent. Mutation-checked against the restored 40/40: partial dropped from the evidence set -> 39/40 tie branch removed -> 39/40 partial reason removed -> 39/40 subagent reason removed -> 39/40 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): the push reason must not assert a complete read that never ran Round 4. The argmax rewrite carried the push reason's text over from the guarded form it replaced, and dropped the guard with it. That text opens "A complete instrument read is in this transcript, but a `git push` landed after it" -- unconditional on `last_complete >= 0`. So on gh pr checks 100 -> git push -> "#100 is good to merge" with no `check-pr-fully-clean.py` anywhere, the WARN blamed a complete read that does not exist. The true account is that a short CI surface was outdated by the push. Splits the reason in two, one sentence per case, and adds the missing one. Also pins two things the reviewer found resting on nothing: - `_relevant_last_subagent`'s strict `idx > last_partial`. A subagent report sharing an index with a partial reading is not after it, so it stays outside the window and the original case still BLOCKs. Loosening to `>=` flips that input to WARN, and until now nothing noticed. - The `len(kinds) > 1` conjunct on the tie branch, which exhaustive enumeration over the 920 reachable states shows is inert -- the early return forecloses `complete` being the sole newest kind. Removed, with a comment recording why reaching that line means a tie. Mutation-checked against the restored 42/42: push sentence ungated -> 41/42 no-complete-read reason gone -> 41/42 `in_window` loosened to `>=` -> 41/42 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): label the claim's own PR, not the first one in the message Round 5. `_pr_label` took the first `#N` anywhere in the message, so "#100 was closed as a duplicate. #200 is green, awaiting your merge." reported a claim "about #100" and handed back a remediation command pointed at the wrong PR -- on both the BLOCK and WARN paths. A guard that tells you to run the instrument is worth little if it names the wrong argument. Takes the nearest reference at or before the matched claim phrase, since a claim's subject usually precedes it, falling back to the nearest one after and then to the old behaviour. Mutation-checked against the restored 43/43: _pr_label called without the match -> 42/43 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): bound the PR label, and decline to name one outside the window Round 6. The nearest-before heuristic was unbounded, so it reached across a whole message to grab a reference the text itself called unrelated, and an issue number sitting before the claim outranked the PR the claim was about. Both produce a confident wrong label in the remediation command the user is being told to run. No heuristic over free text picks right every time, so this one stops trying to. It takes the nearest reference within 120 characters of the claim phrase, and otherwise returns "the PR you named". A vague label the reader resolves themselves is strictly better than a precise one pointing at the wrong PR: the first costs a moment, the second sends the instrument somewhere it will answer a question nobody asked. Mutation-checked against the restored 44/44: _LABEL_WINDOW unbounded -> 43/44 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): resolve the claim's subject once, so two call sites cannot disagree Round 7. `claim_pr_refs` took every `#N` in the message while `_pr_label`, added two rounds earlier, took a windowed reference near the claim. Two notions of "what this claim is about", derived from the same text, and the disagreement was exploitable: dispatch(#100) -> report(#100) -> gh pr checks 651 -> "#100 was closed as a duplicate. #651 is fully clean." The #100 subagent matched `claim_pr_refs`, so `last_subagent >= 0` flipped `is_original_ci_case` to False and the canonical BLOCK case only WARNed -- reopening round 1's regression for any message naming more than one PR. A multi-PR status recap is exactly the shape `_pr_label`'s own tests model, so the guard was defeated by the situation it had just been taught about. Resolves the subject once, via `_pr_label`, and derives `claim_pr_refs` from it. This is round 3's lesson again: the defect was two things allowed to disagree, so the fix removes the possibility rather than aligning them. Mutation-checked against the restored 46/46: claim_pr_refs back to whole-message refs -> 44/46 (both new cases fail: the #200 claim warns off a #100 subagent, and the canonical #651 BLOCK downgrades to a WARN) Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): stop free-text PR matching from deciding BLOCK at all Round 8. Round 7 narrowed `claim_pr_refs` to the single labelled PR to stop an unrelated subagent downgrading the canonical BLOCK case. That purchased the fix with a worse failure: on dispatch(#500) -> report(#500) -> "#500: implemented the changes over in #501 as a follow-on. It's fully clean." the label resolves to #501, the #500 subagent stops matching, and with no CI reading to supply a timing match the guard goes SILENT -- on exactly the shape the extension exists to catch. Neither width works, because the two consumers want opposite things. The LABEL needs one PR, conservatively, since it goes into the command the user runs. The MATCH needs all plausible referents, permissively, since it only asks whether a subagent touched something this claim is about. So separate what each may decide, rather than trying to find one width that serves both: - `subagent_timed` -- landed after the CI reading, pure ordering, no text heuristic -- is now the ONLY thing that can suppress the BLOCK. Free-text subject attribution is not reliably decidable, so it must not decide whether the canonical case blocks. - `last_subagent` stays permissive over a window around the claim and feeds only the reasons and `reading_needed_since`, whose worst outcome is a WARN. This hook family accepts noise over silence. Three existing expectations changed, deliberately, and each is now annotated with why: a same-PR subagent before the reading blocks rather than warns, a nearby unrelated subagent warns rather than staying silent, and one content case moved to merge-ready vocabulary to stay on the WARN path it was written to check. `last_partial >= 0` returns to `is_original_ci_case`, and its history is worth keeping. Round 4 proved it inert by exhaustive enumeration, and it was -- while the line read `last_subagent < 0`, which the early return made imply it. Keying on `subagent_timed` breaks the implication and the conjunct is load-bearing again. An inertness proof is a statement about the surrounding guards, not about the conjunct. Mutation-checked against the restored 48/48: BLOCK routed through the permissive match again -> 45/48 `last_partial >= 0` dropped -> 40/48 matching narrowed to the single label -> 45/48 matching widened to the whole message -> 47/48 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): hedge the subagent reason when the match is only proximity Round 9. The routing held up, but the WARN's sentence did not. On dispatch(#100) -> report(#100) -> "#100 was closed as a duplicate. #200 is fully clean." the message correctly labelled the claim as being about #200 and then asserted that "the most recent evidence in this transcript for that claim is a dispatched subagent's OWN report" -- of a subagent that worked only on #100, in a message whose own text says #100 is closed and unrelated. A reader could go audit the wrong PR's evidence chain. The permissive window is right for deciding whether to look; it is not evidence of what the claim rests on. So the sentence now depends on how the subagent was matched: - Matched by ordering, or working on the PR the claim names: assert. Both are facts the transcript settles. - Matched only by a nearby reference to some other PR: hedge. It says a report concerns a PR mentioned alongside the claim, that whether the claim rests on it is not something the transcript settles, and what follows if it does. This is the round-3 lesson at the level of prose rather than control flow: derive what you say from what you actually know, instead of from the condition that happened to fire. Mutation-checked against the restored 49/49: hedged variant removed -> 48/49 label check dropped from on-topic -> 48/49 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hooks): ordering settles when a report arrived, not what it concerns Round 10. The previous commit's `subagent_on_topic` kept an ordering disjunct -- `last_subagent == subagent_timed` -- and that arm carries no topical check at all. So the round-9 false sentence came back through the timing path instead of the proximity one, on the repo's own fixture whose dispatch prompt literally reads "look into unrelated task #9999, nothing to do with 651": gh pr checks 651 -> dispatch(#9999) -> report(#9999) -> "#651 is fully clean at a5f4f3f2." The WARN named that report as "the most recent evidence in this transcript for that claim". It is the newest event, and it concerns nothing the claim mentions. The disjunct is gone. Topic is decided by topic: does the newest relevant subagent's own refs contain the PR this claim names. Ordering still decides relevance and still decides the BLOCK routing, where it is the right instrument because it needs no text heuristic -- it just cannot also underwrite a sentence about subject matter. The existing case over that fixture pair asserted only `decision == "warn"`, so the wording was unpinned; a content case now covers it. Mutation-checked against the restored 50/50: ordering disjunct restored -> 49/50 Refs #3472 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 45e26a0 commit c32b9dc

5 files changed

Lines changed: 831 additions & 81 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ The payload gaps that remain and the per-guard status are in
431431
| `no-unfiled-finding.py` | `Stop` | blocks the *declarative* "worth its own issue" that leaves no filing behind |
432432
| `flag-unfiled-issue.py` | `Stop` | warns, never blocks, when a reply reports a known gap as "is/was still unfiled", "hasn't been filed", or similar RETROSPECTIVE status wording, with no issue-create or issue-comment call after it; distinct from `no-unfiled-finding.py`'s forward "worth an issue" phrasing, which this misses entirely --- reporting the gap again is not tracking it, however many replies it gets repeated across |
433433
| `no-stale-pr-status.py` | `Stop` | blocks a reply asserting a PR's check state from a reading older than the last push |
434-
| `no-incomplete-check-enumeration.py` | `Stop` | blocks a reply declaring a PR clean when the only reading is `gh pr checks` or `statusCheckRollup` (short surfaces, not the complete instrument) |
434+
| `no-incomplete-check-enumeration.py` | `Stop` | blocks a reply declaring a PR clean when the only reading is `gh pr checks` or `statusCheckRollup` (short surfaces, not the complete instrument); also warns on broader merge-readiness vocabulary or a claim resting solely on a subagent's own report |
435435
| `remind-ums-after-error.py` | `UserPromptSubmit` | reminds, never blocks, when an admitted error has no recorded learning after it |
436436
| `remind-ci-crosscheck-sim-verdict.py` | `UserPromptSubmit` | reminds, never blocks, when a verdict-shaped figure follows a LOCAL sim/transcript run with no CI-side read in between -- the same clip and seed have been measured reading FAIL locally and PASS on CI |
437437
| `no-mistake-without-a-hook.py` | `UserPromptSubmit, Stop` | blocks after an admitted, mechanizable mistake, capped once per admitted phrase within a short transcript window (a re-admission of the same phrase well beyond that window blocks again) |

hooks/hooks.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1059,7 +1059,7 @@
10591059
"command": "python3 \"${CLAUDE_PLUGIN_ROOT}/hooks/no-incomplete-check-enumeration.py\"",
10601060
"timeout": 10,
10611061
"script": "no-incomplete-check-enumeration.py",
1062-
"why": "Blocks a reply declaring a PR clean when the only reading is gh pr checks or statusCheckRollup (short surfaces, not check-pr-fully-clean.py). Distinct from no-stale-pr-status.py, which asks whether a reading is current."
1062+
"why": "Blocks a reply declaring a PR clean when the only reading is gh pr checks or statusCheckRollup (short surfaces, not check-pr-fully-clean.py). Distinct from no-stale-pr-status.py, which asks whether a reading is current. Extended 2026-09-09, written up in ai-config#3472 (the false claims themselves were made in chat on Morrison-Lab/ai-config#3468 and d-morrison/macros#87 -- those are the subject PRs, not write-ups), to WARN on two more gaps: broader merge-readiness vocabulary ('green, awaiting your merge'), and a terminal claim resting solely on a dispatched subagent's own report with no reading by the conducting session."
10631063
},
10641064
{
10651065
"type": "command",

0 commit comments

Comments
 (0)