Repository navigation
feat(github-evidence-guard): warn before redoing work an existing PR covers - #6305
john-the-dev wants to merge 3 commits into
Conversation
…covers Registers github-evidence-guard.py a second time under PreToolUse[Bash] with --dup-check. Before a new branch, gh pr create or gh pr review it makes one GitHub search and names PRs that may already cover the work: ticket-id hits in any state first and untruncated, then PRs changing the same function, then keyword hits. Advisory only; never denies. A failed search prints one line and logs UNKNOWN to <workspace>/logs/dup-work.jsonl. The heredoc splitter that shell-trap-guard and stale-head-post-guard each carried moves into _shell_scan.heredocs, which the new check also uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Sutando-Task: 20261107-032
sonichi-dot
left a comment
There was a problem hiding this comment.
Reviewed head 2e795bc7de2c0c55955812e88918ff3a0830e7ff.
Not ready to merge: two reproducible silent-miss paths and the new test suite's hermetic-environment CI failure need fixing. The PR also remains draft pending its stated live trial.
Validation: inspected all 12 changed files and their activated paths. Locally passed the 31 duplicate-check tests (also under the workspace guard), 53 shell-trap tests (1 skipped), 31 stale-head tests, 26 settings-builder tests, decision/outcome evidence and related hook suites, four launcher suites, and the full-diff review scanner. Additional probes confirmed the two runtime findings below. Source/test files were verified against this head's Git blob hashes. I did not run the full repository suite or a live-core trial.
The current CI job python standalone tests (2/15) failed in hermetic-env-structural.test.py, explicitly naming the new test at lines 28 and 358. Other CI jobs were still running; CLA was successful.
Worst-case user impact: this always-on hook can silently miss duplicate work, or add noisy/slow advice to branch/PR commands. It has no denial path, and env.DUP_WORK_CHECK: off provides an escape hatch, but failed reads must remain distinguishable from clean results.
| def pr_diff(run: Run, repo: str, number: int, timeout: float) -> Optional[str]: | ||
| if timeout < 0.3: | ||
| return None | ||
| try: | ||
| p = run(["gh", "pr", "diff", str(number), "-R", repo], capture_output=True, text=True, timeout=timeout) | ||
| except (OSError, subprocess.SubprocessError): | ||
| return None | ||
| return p.stdout if p.returncode == 0 else None |
There was a problem hiding this comment.
[P2] Preserve failed diff reads as UNKNOWN
pr_diff() returns None on an error, timeout, or exhausted budget, and callers pass that to hunk_functions(), which turns it into an empty function set. In review mode there are no keyword/ticket searches, so _one() then reports OK with empty output even when the function comparison never ran. I reproduced this with a same-function candidate: successful reads produce FLAG; changing either the reviewed PR's diff or the candidate's diff to exit 1 with HTTP 502 produces OK and ''. Propagate an unavailable comparison separately from a successful empty diff, and emit/log UNKNOWN (or an explicit incomplete-check warning) rather than silently clearing it.
| DUP_TRIGGER_KEYS = ("checkout -b", "checkout -B", "switch -c", "switch -C", "switch --create", | ||
| "switch --force-create", "worktree add", "pr create", "pr review") | ||
|
|
||
|
|
||
| def dup_check(data: dict, command: str, argv) -> dict: | ||
| """PreToolUse advice naming PRs that may already cover this work (`--dup-check` registration only).""" | ||
| if data.get("hook_event_name", "PreToolUse") != "PreToolUse" or not any(k in command for k in DUP_TRIGGER_KEYS): | ||
| return {} |
There was a problem hiding this comment.
[P2] Let all parser-supported commands reach the duplicate check
The literal substring prefilter drops valid triggers before _dup_work.triggers() can recognize them. For example, git checkout --quiet -b fix/rmabcdefghij12-test, git checkout -b fix/rmabcdefghij12-test (two spaces), and gh pr review 9 -R o/r --comment -b x all produce one trigger when parsed directly, but guard.decide() makes zero GitHub calls and returns {}. Thus ordinary flag placement or whitespace silently disables the check. Make this fast filter conservative enough to admit the parser's supported forms and test these through decide(), not only through triggers().
| gh = Path(d, "gh") | ||
| gh.write_text("#!/bin/sh\necho 'HTTP 502' >&2\nexit 1\n") | ||
| gh.chmod(0o755) | ||
| env = dict(os.environ, PATH=f"{d}{os.pathsep}{os.environ['PATH']}") |
There was a problem hiding this comment.
[P2] Build the test environments with the existing hermetic helper
The current-head CI failure is from this new suite: hermetic-env-structural.test.py reports :28: unisolated for the module load and :358: copy for dict(os.environ, ...), failing both tree checks. The temporary workspace does not satisfy the repository's separate environment-isolation contract. Use tests/_helpers/hermetic_env.py to isolate the process before loading the hook and construct the child environment, then rerun the structural suite as well as this suite. The failing job's final summary identifies this as its one failed test file: https://github.com/sonichi/sutando/actions/runs/42444120450/job/128619386057
hermetic-env-structural refuses a test that loads modules or spawns children on the outer environment; isolate at module top and hand the end-to-end child isolated_child_env(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Sutando-Task: 20261107-032
sonichi-dot
left a comment
There was a problem hiding this comment.
Re-reviewed cumulative changes at 73a64658fd881861f7bbf15487e7de39e5533572; this update changes only the test environment setup.
The environment-isolation finding is resolved: hermetic-env-structural.test.py passes all 28 tests, and the duplicate-check suite passes all 31 under the workspace guard.
Not ready to merge: the failed diff reads reported as OK and valid triggers skipped by the prefilter remain unchanged and still reproduce on this head. No new findings in this update; keeping those existing threads rather than duplicating them.
Also reran shell-trap (53 tests, 1 skipped), stale-head (31), settings-builder (26), and the full-diff review scanner successfully. Verified reviewed source/test blobs against this SHA. CI is still running, CLA is green, and the stated live trial remains pending. I did not run the full repository suite or a live core.
User impact remains missed duplicate-work advice in an always-on hook; the off switch mitigates noise, not those silent misses.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
Diff coverage of hooks/_dup_work.py was 85%: unreadable responses, gh or git that cannot start, an unreadable local diff, a missing review target, the log resolver and the summary CLI had no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Sutando-Task: 20261107-032
|
@cla-assistant check |
sonichi-dot
left a comment
There was a problem hiding this comment.
Re-reviewed the cumulative 12-file diff at 1bd97c1a8417568c371eccd60c04898ec43b7f90. This update adds 15 tests; production behavior is unchanged.
Not ready to merge: the two existing runtime findings remain reproducible: failed diff reads become OK and valid commands miss the prefilter. No additional findings or duplicate inline threads.
The new test_pr_diff_unavailable_or_out_of_time_is_no_function_signal checks only that the helper returns None. It should also exercise the complete hook and require UNKNOWN plus advice on failure; the existing end-to-end repro still produces OK and empty output. Increased line coverage does not resolve that behavior.
Verified local results: duplicate-check 46 tests (under the workspace guard), environment-isolation 28, shell-trap 53 (1 skipped), stale-head 31, settings-builder 26, and the full-diff scanner all pass. Reviewed blobs match this SHA. Local coverage measurement was unavailable because the coverage module is not installed. Full CI and the live-core trial remain pending; CLA is green.
The user-facing risk remains silent missed warnings in the always-on hook. Its off switch mitigates noise, but the failure paths still need explicit incomplete-check advice.
Draft until a 12-hour trial reports. This PR becomes ready for review only after a 12h trial on a live core, due 07:24 PT, has been summarised with
python3 hooks/_dup_work.py --summaryand its false-alarm count is added here.What changed, and why
This automates the existing manual check. The check that works by hand is
gh pr list --state all --search <ticket id>, run on its own, before starting work. The documented misses all come from skipping it or diluting it:_one_line). The titles share no phrase, so a title search finds neither from the other. Only the touched function collides.That makes six documented incidents. Duplicate work comes up in the agent's notes on 11 different days since July.
So the ticket-id query runs on its own, in every state, and its hits are always listed first and never cut by a result cap. Keyword and file signals come after it.
Where it hooks in
There is no new hook file.
hooks/github-evidence-guard.pyalready readsgh pr …commands, so this PR adds a--dup-checkmode to it. The core registers the same file once more underPreToolUse[Bash]aspython3 <hook> --dup-check --repo <checkout>. That registration runs only this check. The hook's existing behaviour, and any other registration of it, is unchanged.This follows the registration pattern of #6304, but does not depend on it. #6304 is still open.
Read first:
hooks/_dup_work.py: the policy, a helper beside the hook in the_shell_scan.pystyle. It covers trigger parsing, the single search, matching and ranking, the log, the off switch and--summary.hooks/github-evidence-guard.py:dup_check()and the--dup-checkbranch indecide()andmain(), about 25 lines.src/agent/claude/cli/build-core-settings.mjs(arg 9) andsession-launch.sh: the registration.hooks/_shell_scan.py:heredocs().shell-trap-guard.pyandstale-head-post-guard.pyeach carried their own copy of this heredoc splitter, and this check needs a third. Both now delegate to the shared one. Their behaviour is unchanged and both suites pass.Behaviour
Triggers:
git checkout -b|-B,git switch -c|-C|--create|--force-createandgit worktree add -b|-B. It followscd X &&andgit -C X.gh pr create.gh pr review <n|url>.Heredoc bodies are data, so a note that mentions
git checkout -bis not a trigger. A heredoc body passed togh pr create --bodyis read for ticket ids.One
gh api graphqlread per trigger (5 s budget). It carries every search as an alias:tkrm[a-z0-9]{10,14}from the branch, title or body;#NNNNfrom the branch or title, or after closes/fixes/resolves in the bodyko/kcropr createandpr reviewonlyFor
pr create, the local side isgit diff --name-only origin/<base>...HEADplusgit diff -U0hunk headers. Forpr review, it is the reviewed PR's files, read in the same query, and itsgh pr diff, read in parallel.Ranking, in this order:
@@hunk-header function matches one of yours. Only the top 3 by shared files are diffed (gh pr diff), inside the same 5 s budget.File-only overlap is printed only as a count, and only beside another hit. On a busy file it flags many unrelated PRs (see the Replay section).
Excluded: the PR being reviewed, and any PR whose head is the branch being created or pushed.
Weak-input line. When the branch name carries no ticket id, the output adds one line: "No ticket id in the branch name, so the duplicate check is weaker." That is exactly where the one replay miss came from.
Output mechanism. The repo has two kinds of hook output. A denial is
permissionDecision: deny. Advice isadditionalContext, with exit 0, and the command still runs. This check only ever emitsadditionalContext. When nothing matches and the branch has a ticket id, it prints nothing.Never silent on failure. These all log
UNKNOWNand print one line,Duplicate-work check could not run (<reason>); search open and recent PRs yourself before continuing.:Logging. Each trigger appends one row to
<workspace>/logs/dup-work.jsonl, with the path resolved throughworkspace_default. Each row hasts,trigger,repo,branch,pr,verdict(FLAG / OK / UNKNOWN),ticket,function,keyword,file_onlyandelapsed_ms.python3 hooks/_dup_work.py --summaryprints counts by verdict and trigger, and p50/p95.Config.
env.DUP_WORK_CHECKinsutando.config.local.json, read throughconfig_get, the same convention as #6304.off(or0/false/no) turns the check off. Any other value, or no value, leaves it on.Can this get in the way?
Can it block legitimate work? No. It has no deny path. In the hook,
--dup-checkreturns fromdecide()before the existing deny branch can run. Inmain(), an exception in this mode becomes the one-line advisory instead of a denial.The worst case is a noisy warning that the model reads and ignores. That noise is bounded:
The worst case for time is the 5 s budget on a trigger command, and only on a trigger command. The off switch is
env.DUP_WORK_CHECK: off.What existing mechanism already sees this event, and why not extend it?
github-evidence-guard.pyis the GitHub-evidence hook, so this PR extends it. Onmainit is not auto-registered for the core, and itsgh (pr|run) (view|checks|merge)|gh apifilter matches none of these triggers. That is why it needs a registration and a pre-filter of its own.stale-head-post-guard.pyseesgh pr reviewbut owns post staleness, and it never sees branch creation.shell-trap-guard.pysees every Bash command, but owns shell portability.Folding GitHub reads into either of those would mix policies. The trade-off is one more hook process per Bash command (see Cost).
Merge order with #6304. Both PRs register the same file as arg 9. Whichever lands second should fold into one registration,
--merge-gate --dup-check, so that both run in one process.decide()would then need to run both branches instead of returning on the first.Cost
pr review, live (3 PRs)gh pr diff, in parallelpr create, live (1, empty local diff)The in-hook numbers come from
tests/github-evidence-guard-dup-check.test.py(Cost). The live numbers were measured on a laptop against sonichi/sutando.A non-trigger command returns on a substring check, before
_dup_workis imported. The 25 ms process cost is what any newPreToolUse[Bash]registration pays, and #6304 pays it too. Folding the two registrations removes one of them.The
pr reviewandpr createpaths go beyond the single search: they read up to 3 candidate diffs, plus the reviewed PR's own diff for a review. These are bounded by count and by the same 5 s deadline.Replay of the recorded incidents
Source:
shadow-dup-replay.json(generated 2026-11-08T03:36Z). It replayed each incident against GitHub as it stood at the time: only PRs created beforeas_of, each in the state it had then.It ran the workspace prototype of this check, not this exact code. This PR keeps the prototype's keyword and ticket logic, and adds the function-overlap ranking the replay showed was needed.
usage)The tally: 6 of 7 caught, 1 missed, 3 not replayable.
Two changes came out of the replay:
_one_linefunction overlap isolated fix(gateway): flatten field values on the separator set readers actually split on #3877. This PR therefore ranks function overlap above keywords, and shows file-only overlap only as a count.The
Replaytest class pins each recorded case as a fixture of what GitHub returned. That includes the #3964 review, where #3877 has to win on function overlap over seven same-file PRs.Tests
python3 tests/github-evidence-guard-dup-check.test.py: 46 tests, OK (also under/usr/bin/python33.9 andscripts/hermetic-workspace-guard.py run). Its environment is built withtests/_helpers/hermetic_env.py, sotests/hermetic-env-structural.test.pypasses. Line coverage ofhooks/_dup_work.pyis 100%.pr create. The trigger parser handlescd,-C,--create=, worktree, review URLs and env prefixes._dup_work.check. The check runs only before the tool, and the hook is unchanged without the flag. Subprocess end to end with a failinggh, in a hermetic fake checkout. The switch comes fromsutando.config.main()never blocks whendecide()raises. The--summaryCLI.node --test tests/agent/claude/cli/build-core-settings.test.ts: 26/26. This adds registration and omission cases.tests/shell-trap-guard.test.py(53) andtests/stale-head-post-guard.test.py(31), the two hooks whose heredoc splitter moved.tests/pr-decision-evidence.test.pyandtests/pr-outcome-evidence.test.py.tests/gdocs-write-guard.test.pyandtests/claude-hook-paths-repair.test.py.session-launch.sh:claude-plugin-launch-args,start-cli-claude-config-dir,session-launch-named-pidsandlaunch-worker-session-absolute-paths.bash scripts/review-checks.sh --diffon the full PR diff: PASS.lint-workspace-resolution.sh --diff: clean.gen-src-map.py --check: up to date.ruff==0.15.22: clean.docs_audit.py: PASS.Feature/documentation consistency
hooks/README.mdhas a new section.docs/github-verification.mdhas a new paragraph, and itslast_verifiedindocs/catalog.jsonis bumped.Known limits
#NNNNin a body counts only after closes/fixes/resolves. A bare "see Notes browser: add markdown rendering #123" is not searched, to keep prior-art mentions from becoming hits.Trial plan
Run a live core with this registration for 12 hours. Then post the
--summaryoutput here: counts by verdict and trigger, p50/p95, and a hand count of false alarms among the FLAG rows. The PR leaves draft only after that count is in.Sutando-Task: 20261107-032
🤖 Generated with Claude Code