Four checker misfires from one session get fixtures and fixes - #482
Open
EdbertChan wants to merge 21 commits into
Open
Four checker misfires from one session get fixtures and fixes#482EdbertChan wants to merge 21 commits into
EdbertChan wants to merge 21 commits into
Conversation
Before regex guard: FAIL: test_false_positive_about_another_checker_stays_clean (test_self_retraction_scan.TestFindAdmission.test_false_positive_about_another_checker_stays_clean) AssertionError: 'false' is not None After regex guard: Ran 202 tests in 10.404s OK
…iew claim: agent-relay-attribution counts a number as pasted evidence only when it has three or more digits, so a two-digit timeout value in unrelated output no longer clears a relayed claim.
Review lane: behavior
Safety invariant: Existing positive fixtures still fire and existing negative fixtures stay silent; the hook stays advisory. Pending user confirmation in this session.
Effectiveness measurement: The new fixture pair fails before the one-character change and passes after, both outputs in the commit body.
Slice rationale: One regex floor in one hook, so the diff carries one behavior claim.
Architectural effect: None; the evidence-number threshold changes from two digits to three.
Goal: Change the floor and add the real reply pair as fixtures.
Motivation: A relayed claim about a hook's reset behavior was treated as verified because a pasted tool result contained "timeout=30".
Alternative considerations: Matching only numbers adjacent to a count word was considered and left for later; the digit floor is the smallest change that separates the real pair.
Implementation details: In engine/hooks/agent-relay-attribution/detect.py line 31, EVIDENCE_NUMBER_RE becomes \b\d{3,}\b. Add one entry to tests/fixtures/relay_fires.json whose reply is the sentence "The cheaper catch is our existing repeat-error-stop hook, which today resets its counter on every edit, so it stayed silent in both loops above." with a transcript that carries a task-notification from a subagent and a Bash tool result containing "timeout=30" and no other digits of three or more; add one entry to tests/fixtures/relay_silent.json whose reply is "UNVERIFIED: the subagent reports its counter resets on every edit at detect.py:229-242." with the same transcript. Mirror the existing entry shape (label, reply, transcript). Add one test per entry in tests/test_hooks.py in the existing style. Run the suite before the regex change and paste the failing line, then after and paste the passing line, both in the commit body.
Non-goals: No change to the attribution phrases, the status words, or the teammate envelope handling.
Layer: domain
Feature state: active
Files:
- engine/hooks/agent-relay-attribution/detect.py
- engine/hooks/agent-relay-attribution/tests/fixtures/relay_fires.json
- engine/hooks/agent-relay-attribution/tests/fixtures/relay_silent.json
- engine/hooks/agent-relay-attribution/tests/test_hooks.py
Change types:
- engine/hooks/agent-relay-attribution/detect.py: modify
- engine/hooks/agent-relay-attribution/tests/fixtures/relay_fires.json: modify
- engine/hooks/agent-relay-attribution/tests/fixtures/relay_silent.json: modify
- engine/hooks/agent-relay-attribution/tests/test_hooks.py: modify
Acceptance criteria:
- `python3 -m unittest discover -s engine/hooks/agent-relay-attribution/tests -v` exits 0 and includes the two new tests.
- `python3 scripts/check_hook_test_coverage.py engine/hooks/agent-relay-attribution` exits 0.
- `python3 scripts/check_no_new_comments.py --base origin/main` exits 0.
Exit code: 0
Invoker-Finalize-Id: fd696907-ab92-4b58-9c39-f00d6cd973c9
Regression evidence: Before implementation: python3 -m unittest discover -s engine/skills/reflect/scripts/tests -p 'test_help_flags.py' -v test_token_audit_help (test_help_flags.TestHelpFlags.test_token_audit_help) ... FAIL test_top_sessions_help (test_help_flags.TestHelpFlags.test_top_sessions_help) ... FAIL FAILED (failures=2) After implementation: python3 -m unittest discover -s engine/skills/reflect/scripts/tests -p 'test_help_flags.py' -v test_token_audit_help (test_help_flags.TestHelpFlags.test_token_audit_help) ... ok test_top_sessions_help (test_help_flags.TestHelpFlags.test_top_sessions_help) ... ok Ran 2 tests in 0.158s OK
…ew claim: the phrase "false positive", "false negative", or "false alarm" no longer counts as a self-correction in the reflect self-retraction scan or in the wrong-check phrase dictionary. Review lane: behavior Safety invariant: Every existing admission case in the scan's tests still matches, and the dictionary's existing match and not_match entries are unchanged. Pending user confirmation in this session. Effectiveness measurement: A new scan test with the real sentence fails before the regex guard and passes after, both outputs in the commit body; the dictionary gains three not_match sentences. Slice rationale: One word in one regex plus three dictionary entries about the same phrase, so the diff carries one behavior claim. Architectural effect: None; the wrongness word "false" gains a lookahead that exempts the three compound terms. Goal: Guard the word and teach the dictionary. Motivation: A reply that called another gate's refusal a false positive was counted as the assistant retracting its own claim, which then triggered a full reflect pass. Alternative considerations: Dropping "false" from the wrongness words entirely was considered and left out; "my earlier claim was false" is a real admission. Implementation details: In engine/skills/reflect/scripts/self_retraction_scan.py lines 23-24, replace the bare false alternative inside WRONGNESS_RE with false(?). In engine/skills/reflect/scripts/tests/test_self_retraction_scan.py add a test that "I'll note the guard's false positive on a read-only prompt as a backlog item." returns None and a test that "My earlier claim was false; the real count is 12." still returns a hit. In engine/hooks/llm-judge/phrases/wrong-check-reflect.json append to not_match: "I'll note the guard's false positive on a read-only prompt as a backlog item.", "That refusal was a false alarm from the spawn guard, not my error.", "The scan reported a false negative on the second fixture." Paste fail-before and pass-after runner lines for the scan test in the commit body. Non-goals: No change to the judge prompt, the hook wrappers, or any other wrongness word. Layer: domain Feature state: active Files: - engine/skills/reflect/scripts/self_retraction_scan.py - engine/skills/reflect/scripts/tests/test_self_retraction_scan.py - engine/hooks/llm-judge/phrases/wrong-check-reflect.json Change types: - engine/skills/reflect/scripts/self_retraction_scan.py: modify - engine/skills/reflect/scripts/tests/test_self_retraction_scan.py: modify - engine/hooks/llm-judge/phrases/wrong-check-reflect.json: modify Acceptance criteria: - `python3 -m unittest discover -s engine/skills/reflect/scripts/tests -v` exits 0 and includes the two new tests. - `python3 -m unittest discover -s engine/hooks/llm-judge/tests -v` exits 0. - `python3 scripts/check_no_new_comments.py --base origin/main` exits 0. Exit code: 0 Invoker-Finalize-Id: d2267ebd-11c1-493d-a627-a1b2dfc8c82d
… token_audit.py and top_sessions.py print their usage and exit 0 on --help or -h instead of crashing or reporting an unknown mode.
Review lane: behavior
Safety invariant: Every existing invocation shape keeps its behavior; only the two help flags gain a branch. Pending user confirmation in this session.
Effectiveness measurement: Two new tests invoke each script with --help through subprocess and assert exit 0 and the word Usage in stdout; both fail before and pass after, outputs in the commit body.
Slice rationale: One flag branch in each of two scripts, so the diff carries one behavior claim.
Architectural effect: None; an early usage branch in each entry point.
Goal: Add the help branch to both entry points.
Motivation: In one session both scripts were called with --help to learn their shape; one printed "unknown mode: --help" and the other raised ValueError from int("--help").
Alternative considerations: Switching both scripts to argparse was considered and left out as a larger change than the miss warrants.
Implementation details: In engine/skills/reflect/scripts/token_audit.py _parse_argv (around lines 1413-1440), when argv[1] is --help or -h print the module docstring usage text to stdout and exit 0 before mode handling. In engine/skills/reflect/scripts/top_sessions.py main (around line 227), when sys.argv[1] is --help or -h print a one-paragraph usage to stdout and exit 0 before the int() conversion. Add tests in engine/skills/reflect/scripts/tests that run each script with --help via subprocess and assert returncode 0 and "Usage" in stdout. Paste fail-before and pass-after runner lines in the commit body.
Non-goals: No argparse migration, no change to any other flag.
Layer: domain
Feature state: active
Files:
- engine/skills/reflect/scripts/token_audit.py
- engine/skills/reflect/scripts/top_sessions.py
- engine/skills/reflect/scripts/tests/test_help_flags.py
Change types:
- engine/skills/reflect/scripts/token_audit.py: modify
- engine/skills/reflect/scripts/top_sessions.py: modify
- engine/skills/reflect/scripts/tests/test_help_flags.py: create
Acceptance criteria:
- `python3 engine/skills/reflect/scripts/token_audit.py --help` exits 0 and prints Usage.
- `python3 engine/skills/reflect/scripts/top_sessions.py --help` exits 0 and prints Usage.
- `python3 -m unittest discover -s engine/skills/reflect/scripts/tests -v` exits 0.
Exit code: 0
Invoker-Finalize-Id: 5d2b5136-437e-4ca5-9603-4ca1b5e75418
FAIL before change: test_silent_on_generic_blame_after_latest_refusing_gate_was_read (test_hooks.TestSilentWhenTheGateWasReadOrCited.test_silent_on_generic_blame_after_latest_refusing_gate_was_read) ... FAIL OK after change: Ran 31 tests in 0.282s OK after change: OK Verification: python3 -m unittest discover -s engine/hooks/gate-blame-needs-evidence/tests -v python3 scripts/check_hook_test_coverage.py engine/hooks/gate-blame-needs-evidence python3 scripts/check_no_new_comments.py --base origin/main
…laim: when a reply blames a gate with a generic phrase and names no gate in that sentence, gate-blame-needs-evidence attaches the blame to the gate that last refused, not to every gate the reply mentions anywhere. Review lane: behavior Safety invariant: A blame sentence that names a gate still attaches to that gate; the hook still fires when the blamed gate has no successful read in the session. Pending user confirmation in this session. Effectiveness measurement: The real reply fixture fails before the change (two gates blamed, one wrongly) and passes after (one gate blamed), both outputs in the commit body. Slice rationale: One widening clause in one function, so the diff carries one behavior claim. Architectural effect: None; blame attaches to the refusing gate alone when the sentence is generic. Goal: Narrow the fallback in blamed_gates and add the real reply as a fixture. Motivation: A reply that had read agent-routing-guard's source and called its refusal a false positive was flagged for wrong-check-reflect, a gate the reply named only because a hook message quoted it. Alternative considerations: Requiring a gate name in every blame sentence was considered and left out; a generic phrase right after a refusal is a real blame of that refusal. Implementation details: In engine/hooks/gate-blame-needs-evidence/detect.py lines 339-341, the fallback for a generic blame becomes latest_refusal_gate(lines) alone; the in_reply widening is dropped from that branch. Add a fixture entry to tests/fixtures/real_session.json in the existing shape: a reply that reads "Its source at engine/hooks/agent-routing-guard/detect.py:66 counts any bare merge word as a publication verb, and my read-only prompt said PRs 288, 289 (merged). I'll note the guard's false positive on a read-only prompt as a backlog item." with transcript lines showing a successful grep of agent-routing-guard/detect.py, a refusal from agent-routing-guard, and a Stop feedback line that quotes wrong-check-reflect. Add a test asserting the only blamed gate is agent-routing-guard and, since it was read, the hook stays silent. Paste fail-before and pass-after runner lines in the commit body. Non-goals: No change to blame_sentences, delete_requests, or the read-tracking logic. Layer: domain Feature state: active Files: - engine/hooks/gate-blame-needs-evidence/detect.py - engine/hooks/gate-blame-needs-evidence/tests/fixtures/real_session.json - engine/hooks/gate-blame-needs-evidence/tests/test_hooks.py Change types: - engine/hooks/gate-blame-needs-evidence/detect.py: modify - engine/hooks/gate-blame-needs-evidence/tests/fixtures/real_session.json: modify - engine/hooks/gate-blame-needs-evidence/tests/test_hooks.py: modify Acceptance criteria: - `python3 -m unittest discover -s engine/hooks/gate-blame-needs-evidence/tests -v` exits 0 and includes the new test. - `python3 scripts/check_hook_test_coverage.py engine/hooks/gate-blame-needs-evidence` exits 0. - `python3 scripts/check_no_new_comments.py --base origin/main` exits 0. Exit code: 0 Invoker-Finalize-Id: 9d899c51-5b72-4dd0-97cd-ff5daf677b60
…l-only/g0.t0.a-aafb5bd74-94e0acd8
…-admission/g0.t0.a-ac5aec14b-05ebe834
…p/g0.t0.a-af7691ae3-5cb7bf57
…m: the reflect skill page says a subagent never ends its turn while a task it launched is still live, and that a rejection labelled "already covered" names the covering file and line. Review lane: docs Safety invariant: Prose only; no script, hook, installer, or settings change. Effectiveness measurement: Both sentences are present in the page, checked by grep in the proof task. Slice rationale: Two sentences on one page from one reflect pass, reviewed apart from the code fixes. Architectural effect: None; documentation only. Goal: Add the two sentences where the page already discusses subagents and rejections. Motivation: A reflect subagent ended its turn with its own waiter alive and had to be resumed at roughly twelve times the cost of waiting; a rejection row said "already covered" with nothing to point at, and the claim turned out to be unchecked. Alternative considerations: A hook that catches a subagent ending with a live child was considered; nothing on the SubagentStop surface can see the child today, so the sentence is the catch for now. Implementation details: In engine/skills/reflect/SKILL.md, in the section headed "Always run inside a subagent" (around lines 40-42), add one sentence: the subagent does not end its turn while a task it launched is still running; it waits for that task and returns once, because a resumed subagent replays its whole context. In the synthesis section's Rejected bullet (around line 95), add one sentence: a rejection that says already covered by X names X at file and line, and a sentence a hook challenged gets evidence or a tag, never deletion. Non-goals: No change to any other section, script, or hook. Layer: docs Feature state: active Files: - engine/skills/reflect/SKILL.md Change types: - engine/skills/reflect/SKILL.md: modify Acceptance criteria: - `git grep -n "does not end its turn while a task it launched" -- engine/skills/reflect/SKILL.md` prints one line. - `git grep -n "names X at file and line" -- engine/skills/reflect/SKILL.md` prints one line. - `python3 scripts/check_no_dated_provenance.py` exits 0. Exit code: 0 Invoker-Finalize-Id: 51b91fb9-eadf-4bfc-a51f-da1ee55eb69a
… claim: the reflect scripts suite and the llm-judge suite pass on the finished branch. Review lane: proof Safety invariant: Verification is read-only and does not alter any file. Effectiveness measurement: The two suite runs are the direct measurement. Slice rationale: One focused proof for the two script-level fixes before review. Architectural effect: None; verification only. Goal: Prove the false-positive guard, the dictionary entries, and the help flags hold together. Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing. Motivation: Each fix was proven alone; this proves they hold on the merged branch. Alternative considerations: The full repository suite was rejected; these are the modules the fixes touch. Implementation details: Run the two unittest discovers. Non-goals: No mutations. Layer: app_regression Feature state: active Acceptance criteria: - Exits 0 only when both pass. Exit code: 0 Invoker-Finalize-Id: a040da09-4186-4ced-826d-e59d40e90a7f
…dit scripts answer --help with exit 0 on the finished branch. Review lane: proof Safety invariant: Verification is read-only and does not alter any file. Effectiveness measurement: The two --help invocations are the direct measurement. Slice rationale: One focused proof for the help branch before review. Architectural effect: None; verification only. Goal: Prove neither script crashes on --help. Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing. Motivation: Both scripts crashed on this flag before the fix. Alternative considerations: Trusting the subprocess tests alone was rejected; the real invocation is cheaper than the doubt. Implementation details: Run each script with --help. Non-goals: No mutations. Layer: app_regression Feature state: active Acceptance criteria: - Exits 0 only when both invocations exit 0. Exit code: 0 Invoker-Finalize-Id: 35f8d5f3-e1ba-47a0-9e93-8a367c64eec0
…e-blame-needs-evidence suite and its coverage gate pass on the finished branch. Review lane: proof Safety invariant: Verification is read-only and does not alter any file. Effectiveness measurement: The suite run and the coverage gate are the direct measurement. Slice rationale: One focused proof per fixed hook before review. Architectural effect: None; verification only. Goal: Prove the narrowed fallback holds with every fixture. Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing. Motivation: Each fix was proven alone; this proves it holds on the merged branch. Alternative considerations: The full repository suite was rejected; this is the module the fix touches. Implementation details: Run the hook's unittest discover and its coverage gate. Non-goals: No mutations. Layer: app_regression Feature state: active Acceptance criteria: - Exits 0 only when both pass. Exit code: 0 Invoker-Finalize-Id: 64c3aee9-e1c3-4551-9131-226719b67c7d
…the agent-relay-attribution suite and its coverage gate pass on the finished branch. Review lane: proof Safety invariant: Verification is read-only and does not alter any file. Effectiveness measurement: The suite run and the coverage gate are the direct measurement. Slice rationale: One focused proof per fixed hook before review. Architectural effect: None; verification only. Goal: Prove the three-digit floor holds with every fixture. Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing. Motivation: Each fix was proven alone; this proves it holds on the merged branch. Alternative considerations: The full repository suite was rejected; this is the module the fix touches. Implementation details: Run the hook's unittest discover and its coverage gate. Non-goals: No mutations. Layer: app_regression Feature state: active Acceptance criteria: - Exits 0 only when both pass. Exit code: 0 Invoker-Finalize-Id: f640585c-12c9-4e9e-ac09-25b81f699f34
…a-a898927cb-a5864655
…nd-judge/g0.t0.a-a5b3f66a5-971c13ac
…a-a75b7806a-40a47ed7
…o ephemeral inter-task handoff files remain in the worktree before the merge gate. Review lane: cleanup Safety invariant: The scrub script only checks for known handoff artifact names and never touches source, tests, or other repository files. Effectiveness measurement: The script exits non-zero if any handoff artifact remains. Slice rationale: Required terminal scrub for every implementation workflow. Architectural effect: None; hygiene only. Goal: Leave the branch free of handoff artifacts. Motivation: Handoff files must not reach the PR. Alternative considerations: Manual cleanup was rejected as non-deterministic. Implementation details: Run scripts/scrub-handoff-artifacts.sh. Non-goals: No product edits. Layer: app_regression Feature state: active Acceptance criteria: - `bash scripts/scrub-handoff-artifacts.sh` exits 0. Exit code: 0 Invoker-Finalize-Id: 8bb5dd02-f928-47d8-889b-bb80a8773fee
…d107e968-d9894790 — Review claim: No ephemeral inter-task handoff files remain in the worktree before the merge gate. Review lane: cleanup Safety invariant: The scrub script only checks for known handoff artifact names and never touches source, tests, or other repository files. Effectiveness measurement: The script exits non-zero if any handoff artifact remains. Slice rationale: Required terminal scrub for every implementation workflow. Architectural effect: None; hygiene only. Goal: Leave the branch free of handoff artifacts. Motivation: Handoff files must not reach the PR. Alternative considerations: Manual cleanup was rejected as non-deterministic. Implementation details: Run scripts/scrub-handoff-artifacts.sh. Non-goals: No product edits. Layer: app_regression Feature state: active Acceptance criteria: - `bash scripts/scrub-handoff-artifacts.sh` exits 0.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e3ef94fa-0a79-4571-aa45-1647a29a2841) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several small safeguards read assistant replies and session tools before allowing follow-up checks.
One generic blame was attached to the wrong refusal, and language describing another check as a false positive looked like a self-correction.
Two audit commands also crashed when asked for help, while the guidance lacked two wait-and-evidence rules.
The fixes narrow generic blame to the latest refusal, exempt three false compounds, add help paths, and clarify the guidance.
Review Claim
Approve the checker and guidance updates so unrelated wording no longer creates extra reflection work, while real admissions and existing checks retain their behavior.
Review Lane
behavior
Review Unit
engine-runtime
Safety Invariant
Existing positive and negative fixtures retain their behavior; no new block is introduced, and hook registration, installation, and settings stay unchanged.
Slice Rationale
These changes came from one reflect pass on one session and share the engine-runtime review unit. The fixes are separate behaviors with focused fixtures and tests.
Non-goals
Test Plan
Test Plan
python3 engine/skills/make-pr/scripts/preflight.py --base origin/main—ok preflight passed.OK.check_hook_test_coverage: OK (1 hook(s) checked).OK.--helpcommands — exit 0.bash scripts/scrub-handoff-artifacts.sh—scrub-handoff-artifacts-ok.Revert Plan
Revert Plan
git revert <merge-sha>.Note
Medium Risk
Changes Stop-hook attribution and admission heuristics in engine-runtime checkers, but behavior is pinned by expanded fixtures and focused tests with no registration or installer changes.
Overview
Fixes four checker/guidance misfires from a single reflect session: generic gate blame attaching to the wrong hook, “false positive” wording triggering wrong-check reflection, audit scripts failing on
--help, and missing reflect process rules.gate-blame-needs-evidence splits unnamed blame resolution in
blamed_gates: hook-specific blame still falls back to the latest refusing gate plus gates named elsewhere in the reply; generic blame that only mentions a gate noun (e.g. “false positive”) now resolves only to the latest tool refusal—not Stop feedback hooks likewrong-check-reflect. A real-session fixture and test lock in silent behavior afteragent-routing-guardsource was read.wrong-check-reflect adds
not_matchexamples for blaming another checker’s false positive/alarm; the offlineself_retraction_scanregex treatsfalse positives/negatives/alarmsas product/checker wording, not self-admission (real admissions like “my earlier claim was false” still match).reflect skill documents that subagents must wait for launched tasks before ending a turn, and that Rejected items citing “already covered” must name file/line and must not delete hook-challenged sentences without evidence.
token_audit.pyandtop_sessions.pynow handle-h/--helpcleanly; small unittest coverage was added.Reviewed by Cursor Bugbot for commit ca4e5f1. Bugbot is set up for automated code reviews on this repo. Configure here.