From 4e8bfd420f27c21e85895fb1b209b961611e31ed Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Wed, 22 Jul 2026 21:07:31 +0100 Subject: [PATCH] =?UTF-8?q?fix(guard):=20mind=5Fcommit=5Fguard=20v1.3=20?= =?UTF-8?q?=E2=80=94=20fail=20open=20on=20shell=20it=20cannot=20attribute?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three live firings, three false positives on the guard's own author (v1.0 quoted gh body, v1.1 cd-away commit, v1.2 cd inside a for-loop body), zero confirmed catches of a real bad Mind commit. All three came from attributing a commit to a repo by parsing arbitrary shell. Adds _unattributable(): compounds (for/while/until/select/case), subshells and brace groups, and any non-clause-leading cd now ALLOW rather than being guessed at. The two high-confidence denials — no `--` section, and a directory pathspec — are unchanged. Quoted text stays a single token, so a commit message mentioning 'for'/'done' still hits the bare-commit denial. 5 regression tests (22 total green), and the outcome recorded in docs/agent_failure_modes.md mitigation 2: a refusal whose trigger requires modelling an unbounded input space spends its budget on false positives. Build-chain campaign PyAutoHands#155, remaining-queue item 5. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XFnvcMxTMgKvfZmxame6WG --- bin/mind_commit_guard.py | 64 +++++++++++++++++++++++++++++++++ docs/agent_failure_modes.md | 19 ++++++++++ tests/test_mind_commit_guard.py | 50 ++++++++++++++++++++++++++ 3 files changed, 133 insertions(+) diff --git a/bin/mind_commit_guard.py b/bin/mind_commit_guard.py index 0a27894..f99a9de 100644 --- a/bin/mind_commit_guard.py +++ b/bin/mind_commit_guard.py @@ -21,6 +21,30 @@ (fail-open — a guard that misfires trains bypass-by-default). Escape hatch: ``PYAUTO_SKIP_MIND_GUARD=1`` in the environment or as a prefix in the command text (for the deliberate exceptional case, e.g. a bulk migration). + +v1.3 — fail open on shell this parser cannot attribute +------------------------------------------------------ +Every live firing of this guard so far has been a false positive **on its own +author**, and each came from the same root cause: it attributes a commit to a +repo by parsing arbitrary shell, and the parser's model is ``cd X && git +commit`` / ``git -C X commit``. + + - v1.0 — regex clause split cut inside a quoted ``gh`` comment body. + - v1.1 — a ``cd`` away from Mind before committing was ignored (fixed v1.2). + - v1.2 — a ``cd`` inside a ``for``-loop *body* is not a clause-leading token, + so ``_cd_target`` never tracked it and the commit resolved to the ambient + Mind cwd. + +Three false positives, zero confirmed catches of a real bad Mind commit — the +``-- `` habit has done the actual work. Per +``docs/agent_failure_modes.md`` §4 a noisy refusal trains bypass-by-default, so +the guard is narrowed rather than patched again: **it may only DENY when it is +confident**. When the command contains a construct the clause walk cannot +follow — a ``for``/``while``/``until``/``case`` compound, a subshell or brace +group, a function body, or a ``cd`` that is not a clause-leading token — the +guard allows (see ``_unattributable``). The two high-confidence denials are +unchanged: a ``git commit`` that unambiguously resolves to a Mind checkout with +no ``--`` section, and a directory pathspec after ``--``. """ from __future__ import annotations @@ -80,6 +104,41 @@ def _clauses(command: str): yield clause +# Shell keywords that open a compound the clause walk cannot follow: their +# bodies are not clauses in this parser's sense, so a `cd` (or the commit +# itself) inside one is attributed to the wrong repo. +_COMPOUND_KEYWORDS = frozenset({"for", "while", "until", "select", "case", "esac", "function", "do", "done"}) + + +def _unattributable(command: str) -> bool: + """True when the command contains shell this parser cannot confidently + attribute to a repo — in which case the guard must allow (v1.3). + + Three shapes, all of which produced a live false positive or would: + + - a compound keyword (``for``/``while``/``case``/…): the body is not walked + as clauses, so a ``cd`` inside it is invisible (the v1.2 FP); + - a subshell ``(...)`` or brace group ``{...}``: same, plus the cwd change + is scoped to the group; + - a ``cd`` at a non-leading position in a clause: ``_cd_target`` only reads + ``tokens[0]``, so such a ``cd`` is silently dropped. + + Quoted text (commit messages, ``gh`` bodies) is a single token by then, so + a message that merely *mentions* one of these words cannot trip it — and if + a token is ambiguous, the resolution is to allow, which is the safe + direction for a guard whose only justification is certainty. + """ + for tokens in _clauses(command): + for i, tok in enumerate(tokens): + if tok in _COMPOUND_KEYWORDS: + return True + if tok in {"{", "}"} or "(" in tok or ")" in tok: + return True + if tok == "cd" and i != 0: + return True + return False + + def _under_mind(path: Path) -> Path | None: """If ``path`` is inside a PyAutoMind checkout, return that checkout root; else None.""" @@ -120,6 +179,11 @@ def check_command(command: str, cwd: str = "") -> str | None: # (a `cd`/`git -C` path) or in the ambient cwd. If neither, nothing to do. if MIND_MARKER not in command and MIND_MARKER not in (cwd or ""): return None + # Only deny when confident (v1.3): shell the clause walk cannot attribute + # is allowed rather than guessed at — every historical false positive was a + # guess of this kind. + if _unattributable(command): + return None effective_cwd: Path | None = Path(cwd) if cwd else None diff --git a/docs/agent_failure_modes.md b/docs/agent_failure_modes.md index e6f6264..f49d709 100644 --- a/docs/agent_failure_modes.md +++ b/docs/agent_failure_modes.md @@ -95,6 +95,25 @@ Each: catalogue entries caught → why it fires at the decisive moment → cost check scoped to one repo; false-positive surface: deliberate multi-file registry commits, handled by listing the files (which is the desired behaviour anyway). + + **Outcome (2026-07-22, v1.3) — the false-positive surface was the real + cost, and it was bigger than predicted.** Deployed as + `bin/mind_commit_guard.py`, the guard fired live **three times, every one a + false positive on its own author** (v1.0 a quoted `gh` body; v1.1 a `cd` + away from Mind; v1.2 a `cd` inside a `for`-loop body), against **zero + confirmed catches** of a real bad Mind commit — the `-- ` habit did + that work. Each FP came from the same place: attributing a commit to a repo + by parsing arbitrary shell. Since §4's own argument is that a noisy refusal + trains bypass-by-default, v1.3 narrows the guard to **deny only when + confident**: `_unattributable()` fails open on compounds (`for`/`while`/ + `case`), subshells and brace groups, and any `cd` that is not + clause-leading. The two high-confidence denials (no `--` section; a + directory pathspec) are unchanged. Retiring the guard outright was + considered and rejected — the narrowed deny surface is cheap and still + covers the exact E1/F1 shape — but note the general lesson: **a refusal + whose trigger requires modelling an unbounded input space will spend its + budget on false positives.** Prefer refusals whose precondition is + *decidable* (mitigation 3's merged-branch check, which has true-positived). 3. **Stale-claim auto-expiry (refusal-adjacent).** F4 shows completed tasks linger in `active.md` and block; the conflict guard catches it late but correctly. Cheaper at the source: `worktree_remove` (already the cleanup diff --git a/tests/test_mind_commit_guard.py b/tests/test_mind_commit_guard.py index 3eba87c..79d83c6 100644 --- a/tests/test_mind_commit_guard.py +++ b/tests/test_mind_commit_guard.py @@ -144,3 +144,53 @@ def test_git_dash_C_into_mind_from_other_cwd_still_denied(): 'git -C /home/x/PyAutoMind commit -m "m"', cwd="/home/x/wt/PyAutoHands" ) assert r is not None + + +# --- v1.3: fail open on shell the clause walk cannot attribute -------------- +# Every live firing of the guard was a false positive on its own author. Each +# of the three shapes below is allowed now; the narrow high-confidence denials +# are re-asserted underneath so the narrowing cannot silently widen. + + +def test_for_loop_with_inner_cd_is_allowed(): + # The 3rd false positive (2026-07-17): the `cd` follows `do`, so it is not + # a clause-leading token and v1.2 resolved the commit to the ambient Mind + # cwd. These commits were all in workspace repos. + r = check_command( + 'for r in autofit_workspace autolens_workspace; do cd /home/x/$r && ' + 'git commit -m "floors" -- config/general.yaml; done', + cwd="/home/x/PyAutoMind", + ) + assert r is None + + +def test_subshell_cd_then_commit_is_allowed(): + r = check_command( + '(cd /home/x/wt/PyAutoHands && git commit -m "m")', + cwd="/home/x/PyAutoMind", + ) + assert r is None + + +def test_while_loop_commit_is_allowed(): + r = check_command( + 'while read -r f; do git commit -m "m" -- "$f"; done < list.txt', + cwd="/home/x/PyAutoMind", + ) + assert r is None + + +def test_compound_word_inside_quoted_message_does_not_fail_open(tmp_path): + # `for`/`done` inside a commit message are one shlex token, so they must + # NOT be read as compound keywords — the bare-commit denial still fires. + mind = tmp_path / "PyAutoMind" + mind.mkdir() + r = check_command(f'git -C {mind} commit -m "done waiting for the run"') + assert r is not None + + +def test_simple_shapes_still_denied_after_v13(tmp_path): + mind = tmp_path / "PyAutoMind" + (mind / "active").mkdir(parents=True) + assert check_command(f'cd {mind} && git commit -m "m"') is not None + assert check_command(f'git -C {mind} commit -m "m" -- active/') is not None