#6874

security-guidance: #5425's in-flight markers are a no-op on native Windows (fcntl only), so a push right after a commit still re-reviews the same commit

Openparka4408opened 7d ago

Summary

The fix for #5425 — the sg-inflight-<sha> markers in hooks/diffstate.py — relies on fcntl.flock. On native Windows Python, import fcntl fails, so _mark_inflight() returns without creating a marker and _load_inflight_shas() always returns an empty set. A push made while the commit's review is still running sees that commit as neither reviewed nor in flight, and reviews it again from scratch.

The code comment already notes this ("Without flock (native Windows Python, some network filesystems) there are no markers and nothing changes."), so it is a known gap rather than a regression. I'm filing it so Windows has a tracked path to the fix, with measurements and some Windows-specific notes below.

Environment

  • security-guidance 2.0.16 (claude-plugins-official)
  • Windows 11, native CPython 3.12.13 (no WSL)
  • Agentic commit review enabled

Evidence

~/.claude/security/log.txt (repo path and sha redacted):

16:58:08.542 Commit review: detected git commit in command
16:58:08.598 Commit review: 1/1 sha(s) resolved, 16 files                <- review #1 starts
16:58:10.876 Push sweep: repo resolved via 2 -> '<repo>'
16:58:10.931 Push sweep: range=1 prefix_advanced=0 base=<sha> tail=1      <- review #2 of the same commit
16:58:41.818 Commit review: no security issues found
16:58:43.746 Push sweep: no new findings
  • In a two-hour log window, 3 of the 5 push reviews that ran overlapped a still-running review of the same commit like this (the push came 2–10 s after the commit).
  • Over five days and ~850 agentic reviews on this machine, reviews that started while another review was running in the same checkout accounted for up to 12% of review cost. This is an upper bound, because it also counts unrelated commits that legitimately overlap. Measured with a wrapper set as SG_AGENTIC_CLI_PATH that logs each run's start time, duration, cwd and total_cost_usd.
  • When the commit review has already finished, the push is skipped correctly (Push sweep: every pushed commit already reviewed). The sg-reviewed-shas record works on Windows through the plain-append fallback; only the in-flight path is missing.

Notes for a Windows implementation

All checked with the plugin's own venv interpreter (CPython 3.12.13, win32):

  • import fcntl raises ModuleNotFoundError.
  • msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) gives a non-blocking exclusive lock: a second fd on the same file gets PermissionError until the first fd is closed. It has no shared mode, so _load_inflight_shas() would need to probe with an exclusive lock and release it right away, or use LockFileEx via ctypes for a real shared lock.
  • A file opened with os.open() cannot be unlinked while it is open (PermissionError, WinError 32); after os.close() it can. _release_inflight() unlinks before closing, so on Windows the unlink fails and the marker stays until the 3600 s GC. The order needs to be close, then unlink. For the same reason, the "locked an already-unlinked file" case that the inode re-check in _mark_inflight() guards against cannot occur on Windows.
  • Alternatively, the lock-free O_CREAT | O_EXCL + TTL sentinel suggested in the #5425 discussion works the same on every platform.

Expected

A push made while a commit review is in flight waits for that review, as it does on macOS and Linux, and skips the commit once the commit is recorded in sg-reviewed-shas.

Activity

0

No activity yet.