security-guidance: push-sweep re-reviews the same diff because commit review records reviewed SHAs only after the review completes #5425

Activity

1. mbastawisy commented on Sep 14, 2026

   

   mbastawisy

   on Sep 14, 2026

   More actions

   Confirming this from a second repo, and adding that it is not only the push sweep: the commit-to-commit reflog path races the same way. Plugin 2.0.8 (identical to main), 23 commit reviews over four weeks, 5 overlapping pairs.

   Local reproduction from ~/.claude/security/log.txt:

   19:21:00.113  Commit review: reflog shows 2 fresh unreviewed commit(s) (67a386f...)
   19:21:00.201  Commit review: 2/2 sha(s) resolved, 15 files      <- review #1 starts
   19:21:26.752  Commit review: reflog shows 4 fresh unreviewed commit(s) (111a2f8...)
   19:21:26.883  Commit review: 4/4 sha(s) resolved, 9 files       <- review #2 starts
   19:22:07.558  Commit review: no security issues found           <- review #1 records its SHAs
   

   _append_reviewed_shas runs at security_reminder_hook.py:1426, after the 1 to 4 minute agentic review at :1395; both dedup reads (:1116-1117 reflog, :1725 push sweep) happen before that. A second commit 26 s later re-swept the first two SHAs, so the two reviews overlapped (15 vs 9 files) rather than duplicated exactly.

   Aggravating factor worth noting for anyone measuring this: when git commit runs inside a compound command with piped output, [branch sha] never reaches stdout, every commit logs stdout had no [branch sha], and dedup falls through to the 5-entry / 120 s reflog scan (gitutil.py:290). That makes the race window the normal path, not the fallback.

   Suggested shape. Simply moving _append_reviewed_shas before the review would regress #5322 (a crashed review permanently silences those SHAs). A TTL-expiring in-flight sentinel avoids both, following the existing sg-hook-once-<id> idiom (same gitdir, O_CREAT|O_EXCL, opportunistic GC):

   • diffstate.py: _mark_inflight(repo_root, shas) writes sg-inflight-<sha>; _load_inflight_shas(repo_root) returns those younger than ~900 s and unlinks older ones.
   • security_reminder_hook.py: call _mark_inflight right before the review starts (requires hoisting the rev-parse loop at :1417-1425 above it), and have both dedup reads use _load_reviewed_shas(root) | _load_inflight_shas(root).

   Trade-off: a genuinely failed review suppresses a duplicate pass for at most the TTL, after which the commit is eligible again. Files live in the common gitdir, so worktrees of one clone share the marker, which is the desired behaviour.
2. rjmurillo commented on Sep 25, 2026

   

   rjmurillo

   on Sep 25, 2026

   More actions

   Independent confirmation on 2.0.8 (marketplace commit 55b58ec6e5649104f926ba7558b567dc8d33c5ff), plus a suggested dedupe key that also covers cases a SHA claim cannot.

   Measurement. I audited child review transcripts from one machine (2026-09-10 to 2026-09-20; 223 child sessions, 5,475,987 prompt bytes). I hashed each prompt's diff body with SHA-256.

   • 8 pairs of investigate sessions reviewed byte-identical diffs. That is 660,787 redundant prompt bytes, about 165,000 tokens, or 12.1% of all prompt bytes in the window. The count is before each session re-reads its context on every turn.
   • 7 of the 8 pairs ran in the same checkout. Five of them started 4.5 to 43 seconds apart, which matches the commit-then-push race described above. The plugin log had already rotated, so I cannot tie each pair to a specific hook event.
   • One pair ran in two different checkouts, 891 seconds apart. One was a linked worktree that no longer exists, so I cannot confirm it shared a repository with the other. If it did, the pair matches the worktree split in the secondary issue above.

   Suggested key. The Stop hook already skips an unchanged diff with reviewed_diff_hash (security_reminder_hook.py:2043-2046). Commit review and push sweep could use the same idea:

   1. Before calling _agentic_review_with_race, compute SHA-256 over the capped diff text that goes into the prompt. Scope it by repository identity (git rev-parse --git-common-dir) so all worktrees share it.
   2. Record the hash as in flight before the review starts. Record the result when the review finishes.
   3. Skip, or reuse the stored result, when an in-flight or finished record has the same hash.

   A hash of the exact prompt input catches the race, the worktree split, and identical content under a new SHA (rebase, cherry-pick, or a no-op amend). It never skips a diff that differs by even one byte. The SHA claim proposed above still makes sense next to it, because it keeps the sg-reviewed-shas record accurate.

   One more repeat, smaller. When pass 1 returns one or two candidates, iter2_prompt (llm.py:1421) is user_prompt plus exclusions, sent to a fresh child session. Each iter2 session I measured re-created the cache for the whole diff on its first turn: cache_creation_input_tokens was 5,351 to 16,240. Resuming the investigate session for pass 2 would read the diff from cache instead. It would also change the isolation between passes, so I note it only as an option.

   Checked and rejected: artifact references. I also tested replacing the inline diff with a content-addressed file reference that the reviewer fetches with a tool. With the plugin's own AGENTIC_INVESTIGATE_SYSTEM and FINDINGS_SCHEMA, the referenced prompt used more input tokens in 5 of 6 measured cells (+3% to +44%). It usually added a fetch turn and saved nothing, because the reviewer has to read the diff anyway. Removing the duplicate review is where the savings are.
3. rjmurillo

   added a commit that references this issue on Sep 25, 2026

   docs(analysis): reject artifact references for security-guidance diff…

   ...

   95383d2