Rectify: review-pr checked-out ref mutation immunity - #4592
Merged
Trecek merged 47 commits intoAug 15, 2026
Merged
Conversation
…ts finding) test_annotate_pr_diff_cleans_commit_marker_on_baseexception_and_retry parameterized [RuntimeError, KeyboardInterrupt, _AnnotationSentinel] to drive the ``except BaseException`` cleanup branch. KeyboardInterrupt IS-A BaseException and is covered by _AnnotationSentinel's direct BaseException inheritance, making the explicit KeyboardInterrupt parametrize value redundant. Removed it; kept RuntimeError (Exception subclass) and _AnnotationSentinel (BaseException subclass) to cover both cleanup branches.
…explicit checks (defense finding) Defensive isinstance checks via `assert` are stripped under `python -O`, silently disabling the guard for dict/list shape assumptions. Replaced four `assert isinstance(...)` sites in _all_threatened, _threatened_for_target, and _classify_git_segment with explicit `if not isinstance(...) raise TypeError(...)` so the guard raises on malformed context regardless of optimization level.
…xemption model (cohesion/arch finding) The PR removed `exempt_session_types` from the git_ops_guard HookDef in hook_registry.py, making the all-session preflight unconditional. The matching _EXEMPT_SESSION_TYPES frozenset and the orchestrator check in main() were orphaned, leaving a stale "must stay in sync with..." comment plus a dead code path that granted a session-type exemption only on the legacy destructive-ops branch. Removing the constant, the check, and the comment collapses the two-phase split-exemption model into a single uniform policy.
…aking -m in update-ref parsing (defense finding) _classify_update_ref bundled `-m` (value-taking) with `--create-reflog` (boolean) into a single elif chain that called _consume_option_value for both. For `git update-ref --create-reflog -d <ref> <newvalue>`, the parser advanced by 2 after seeing --create-reflog, swallowing `-d` as if it were the reflog's value. The delete flag was silently dropped, the delete detection never fired, and the parser treated the request as a plain update to <ref>. Split the elif so --create-reflog advances by 1 and -m keeps its consume-by-2 behavior.
…rev-parse lookups return empty (defense finding)
_classify_reset compared _resolve_attempted_sha against _git_text(cwd,
"rev-parse", target). When BOTH lookups fail (git unreachable, transient
fs error, ref removed) both sides collapse to "", the equality
"" == "" evaluates True, the function returns None, and the guard
silently allows the reset. Detect the both-empty case before the
comparison and return the unresolved-ambiguous ("","<unresolved>",True)
tuple so the caller routes the request through _all_threatened (every
owned ref) instead of silently allowing. This also closes the same
root cause in the _symbolic_head() empty-return path.
…er (bugs finding) _classify_git_segment's symbolic-ref branch required exactly 2 positional args, so `git symbolic-ref HEAD <ref> -m <msg>` (3 positionals after the no-flag filter) slipped past the guard. Loosened to "first positional is HEAD", which matches both the common 2-token form and the -m-flag form.
…(defense finding) main()'s checked-out-ref preflight was wrapped in ``except (OSError, subprocess.SubprocessError, TypeError, UnicodeDecodeError, ValueError): sys.exit(0)``. That silently allowed every operation through whenever the preflight raised any of those exceptions. For a security preflight, fail-closed is the only acceptable default: log to stderr and exit 2 so the caller sees the failure rather than the operation silently proceeding. (The explicit deny path uses ``raise SystemExit(0)`` from _deny_checked_out_ref; SystemExit inherits BaseException, not Exception, so it is preserved by the rewrite.)
…helper and rewrite dangling comment (smoke_utils findings)
1. local_base_observation invoked subprocess.run directly while every
other git invocation in annotate_pr_diff routes through the file's
own _run helper. The helper raises on non-zero returncode, but the
observation code intentionally treats rc=1 ("ref does not exist") as
a benign warning. Added a sibling _run_tolerant helper that accepts
an ok_returncodes frozenset and switched the call site to use it,
collapsing the duplicated subprocess.run kwargs into one place.
2. Restored the LOCAL_ROUND_EXEMPT_VERDICTS block comment, which had
been compressed into a sentence fragment beginning with "of" — a
botched edit. Now reads as a proper 3-line comment explaining the
rationale for both exempted verdicts.
…n downstream-review test (tests finding) test_byte_stable_on_downstream_review_failure was vacuous: it called annotate_pr_diff once with a stub, then defined a separate fail_downstream_review(result) function that raised before any second subprocess.run was invoked. The assertion ``run.assert_not_called()`` explicitly proved the production failure path was never exercised. Reworked the test to inject the failure into the SECOND _read_provider_authority call inside annotate_pr_diff itself (the local-mode path calls _read_provider_authority twice around the ``git diff`` to detect provider head/base movement). This drives the real metrics_path.unlink(missing_ok=True) cleanup branch in the ``except BaseException`` handler. Also corrected the module docstring which overclaimed "Real-Git regression coverage" when the test functions all patch subprocess.run — the docstring now accurately describes the real-git fixture setup + patched subprocess + state verification pattern.
… split exemption model (cohesion/arch finding)" This reverts commit ef9c12e.
…odes kwarg
The earlier split introduced _run_tolerant as a separate helper purely to
accept an ok_returncodes parameter; that bloats the file and forces
test_schema_version_convention to track two distinct allowlist entries.
Folded ok_returncodes into _run directly with default frozenset({0})
preserving the strict-by-default behavior, and removed _run_tolerant.
Switched the call site to pass ok_returncodes=frozenset({0, 1}).
Also tightened the LOCAL_ROUND_EXEMPT_VERDICTS block comment back to
two lines.
…authority cross-check The local review mode docstring implied no GitHub API calls, but annotate_pr_diff still calls `gh api` to cross-validate the provider head/base SHAs against the local checkout (security guarantee against a tampered or stale local branch). Added an explicit note so future readers don't mistakenly try to remove those calls as "not needed in local mode".
…ls refactor The smoke_utils/_review.py consolidation of _run_tolerant into _run and the trimmed comment shifted the atomic_write sites by one line. Update the hard-curated allowlist from 734/838 to 735/839 so the AST-scan test matches actual source.
Adversarial verification found the pre-fix _classify_update_ref
bundled --create-reflog (boolean) with -m (value-taking). The bypass
is real for two documented forms the existing parametrize missed:
- `git update-ref --create-reflog -d refs/heads/develop`
(--create-reflog consumes -d as its value)
- `git update-ref -d --create-reflog refs/heads/develop`
(control: -d alone, but the bundling dropped the flag context)
Added both to test_deletion_forms_protect_checked_out_destinations.
The fix in commit a5959ab denies both forms with
attempted_value == "<delete>".
…stry sync) Adversarial verification of commit 1b2766a (main() fail-closed) flagged that FAIL_CLOSED_GUARD_BASENAMES in hook_registry.py and the mirrored AGENTS.md / docs/safety/hooks.md tables were not updated when git_ops_guard was made fail-closed on unhandled preflight exceptions. AGENTS.md §3.4 mandates same-commit registry updates for this class of change. Added git_ops_guard.py to FAIL_CLOSED_GUARD_BASENAMES with the specific fail-closed condition (exit 2 + stderr on the caught exception tuple) and rationale, and mirrored the addition in both doc tables. Bumped the count from six to seven in both lists. This keeps the registry the source of truth for fail-closed guards and ensures tests/arch/test_fail_closed_guard_contract.py and tests/docs/test_guard_fail_mode_docs.py enforce the new entry.
…ass in _classify_reset Adversarial verification of the original _classify_reset fix (3511c83) noted the commit message claimed "this also closes the same root cause in the _symbolic_head() empty-return path" but the diff did not deliver that — `if not target: return None` at the top of the function still silently allowed any `git reset` when git was fully unreachable (so _symbolic_head returned ""). Change `return None` to `return ("", "<unresolved>", True)` so the caller routes the request through _all_threatened (deny against every owned ref) instead of silently allowing. The contract `("", "<unresolved>", True)` is the established fail-closed ambiguous marker used by every other classifier in this module.
…ses in symbolic-ref Adversarial verification of commit 8408144 (the original symbolic-ref length fix) returned FIX_REGRESSION. The previous fix changed ``if len(positional) == 2 and positional[0] == "HEAD"`` to ``if positional and positional[0] == "HEAD"``, which: 1. IndexError on 1-positional read forms (``git symbolic-ref HEAD``, ``--short HEAD``, ``-q HEAD``, ``-d HEAD``). The IndexError escapes the preflight except tuple (OSError, SubprocessError, TypeError, UnicodeDecodeError, ValueError), the hook exits 1, and Claude Code treats exit 1 as non-blocking — fail-open. Worse, because the exception happens inside the per-segment loop in _preflight_checked_out_ref_mutation, ALL subsequent segments are skipped, turning a benign read into a universal guard-disable prefix (e.g. ``git symbolic-ref HEAD; git update-ref ...``). 2. Missed two of the three valid ``-m <reason>`` orderings that git accepts. The naive ``[t for t in args if not t.startswith("-")]`` treats the value of ``-m`` as a positional, shifting the index so the check on positional[0] == "HEAD" silently fails. Fix: parse positionals while consuming the value-taking ``-m`` flag, and require ``len(positional) >= 2`` so read forms neither IndexError nor accidentally classify a 1-token read as a mutation. Added parametrized regression tests covering: - All three ``-m`` orderings (deny) - All read forms (``HEAD``, ``--short HEAD``, ``-q HEAD`` — allow, no exception)
Adversarial verification of commits 83fd570 / 3b873ca noted two follow-up cleanups: 1. Once ``local_base_observation`` is routed through ``_run`` with ``ok_returncodes=frozenset({0, 1})``, the subsequent ``if local_base_observation.returncode not in (0, 1)`` block is unreachable (the helper only returns for rc in ok_returncodes). Inline the probe as a side-effecting _run call and drop the dead check. 2. The rewritten LOCAL_ROUND_EXEMPT_VERDICTS comment was based on a misread of the original. The original was a complete sentence with standard 100-col line wrap, not a dangling fragment. Restored the original wording.
…t trim The dead-code cleanup commit trimmed the explanatory comment from 5 lines back to 2, which shifted the atomic_write sites by -3 lines. Update the hard-curated allowlist from 737/841 to 734/838 so the AST-scan test matches actual source.
…switch -C positional parser
…2 in _classify_fetch
… _same_repository
…IX for consistency
…after _review.py dedent" This reverts commit 4e1f51c.
…_review.py (1005 lines)" This reverts commit 7404520.
…(restore 999 LOC, drop exemption)
…review-design (479 + 545 LOC)
…file count to 12 for _review_design split
…m _review to _review_design
critical: _classify_push normalized colonless refspecs and treated same-repo push with no refspec as ambiguous (fail-closed). warning: _classify_branch_position now gathers positionals across the whole arg list once force marker is detected anywhere; cross-worktree HEAD/refs paths now fail-closed in _raw_target_mutations; cwd tracker recognizes 'cd -P/--' and 'pushd' variants. warning: dropped the rc-discarding 'git rev-parse --verify -q' smoke call in _review.py: it neither proved the base branch existed nor influenced any later decision. cohesion: extracted _resolve_git_common_dir helper to deduplicate rev-parse logic shared by _repository_context and _same_repository; extracted _ctx_str helper to deduplicate the 'str(context[k]) if context else ...' pattern in _deny_checked_out_ref; consolidated the inline subprocess.run cat-file check in _ensure_provider_objects into the existing _run helper. Co-Authored-By: Claude <noreply@anthropic.com>
Adversarial validation of the prior commit b4e158c surfaced a security regression: collecting all positionals across the entire arg list broke \git checkout <sha> -B <branch>\ and \git switch <sha> -C <branch>\ — the SHA was treated as the target (which is never an owned ref) and the checked-out branch was silently allowed. For \git branch -f <name> [<start-point>]\ the force flag may appear anywhere, so gather all positionals. Target is the first positional, start-point is the second. For \git checkout -B <branch>\ and \git switch -C <branch>\ the marker MUST be followed immediately by the new branch name; the start-point can appear anywhere else. Treat the token after the marker as the target and any other positional as the start-point. Also bails \cd -\ to an empty cwd so subsequent git segments are skipped rather than classified against the unresolvable OLDPWD swap path. Adds 5 regression tests for force-marker-anywhere and checkout/switch-before-marker cases in tests/infra/test_git_ops_guard.py. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 15, 2026
Trecek
deleted the
impl-rectify-review-pr-checked-out-ref-immunity-20260814-190524
branch
August 15, 2026 23:46
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
Local
review-prcurrently conflates four different Git authorities: the checkout head, the current local base-branch tip, the PR API's base snapshot, and the PR's merge base. When the local tip differed from the older PR snapshot,annotate_pr_diff()rejected a healthy review. An interactive agent then made the invalid predicate true by runninggit update-refagainst a shared branch. Because linked worktrees share their common Git directory, that out-of-band ref write silently moved the primary checkout's symbolicHEADtarget without updating its index or worktree.Closes #4588
Implementation Plan
Plan file:
/home/talon/projects/generic_automation_mcp/.autoskillit/temp/rectify/rectify_review_pr_checked_out_ref_immunity_2026-08-14_162757.md🤖 Generated with Claude Code via AutoSkillit