From 1e6391943c990ff2d526aca5ec497dd48e6459d5 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Fri, 24 Jul 2026 17:20:49 +0000 Subject: [PATCH] Extend guard_master.py to gate Edit/Write/NotebookEdit worktree-path trap Four sessions on 2026-07-23/24 accidentally edited the main checkout via absolute-path file-tool calls; PreToolUse now blocks writes that resolve outside .claude/worktrees/ from a worker worktree session (WORKERS.md and journal/ still exempt; Read stays ungated). --- .claude/hooks/guard_master.py | 131 +++++++++++++++++- .claude/hooks/test_guard_master.py | 205 +++++++++++++++++++++++++++++ .claude/settings.json | 2 +- docs/lessons.md | 12 ++ docs/troubleshooting.md | 43 ++++++ 5 files changed, 389 insertions(+), 4 deletions(-) diff --git a/.claude/hooks/guard_master.py b/.claude/hooks/guard_master.py index f366cad7e..1aeac22d4 100755 --- a/.claude/hooks/guard_master.py +++ b/.claude/hooks/guard_master.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 -"""PreToolUse gate: no automated session merges or pushes master directly. +"""PreToolUse gate: no automated session merges or pushes master directly, +and no worker worktree session editing the main checkout's files. Scope (owner decision 2026-07-19): - `gh pr merge` and `git merge` into master: blocked everywhere, @@ -13,6 +14,29 @@ checkout's interactive pushes to master stay untouched, per this repo's standing convention (see CLAUDE.md, Push policy). +Scope added 2026-07-24 (worktree-path trap, see docs/lessons.md's +"Absolute paths to the repo root silently target the wrong checkout in +a worktree session"): `Edit`/`Write`/`NotebookEdit` calls are blocked +when the session's cwd is under a worker worktree +(.claude/worktrees//...) AND the tool's target path resolves to +the main checkout root -- i.e. the repo path with the +`.claude/worktrees/` subtree excluded. Four independent sessions on +2026-07-23/24 accidentally edited the shared main checkout this way +(all self-caught before landing anything). Read-only `Read` calls are +deliberately NOT gated -- blocking reads would break legitimate +cross-referencing against the main checkout (e.g. diffing against +master's on-disk state), and a read can't silently land content on the +wrong branch the way a write can. A path under the session's OWN +worktree, under any OTHER worktree, or outside the repo entirely +(/tmp, the orchestration repo, the memory dir) is unaffected -- only a +target that resolves inside the repo root but outside +`.claude/worktrees/` trips this rule. One deliberate exception: +`WORKERS.md` and `journal/` are gitignored and, by established +convention (CLAUDE.local.md's multi-worker protocol), live in the main +checkout on purpose -- a worker worktree session writing its own +coordination row there is expected behavior, not the trap (see +`_MAIN_CHECKOUT_WRITE_EXCEPTIONS`). + Both `current_branch(...) == "master"` checks above resolve branch state via `effective_dir()`, not the raw session cwd -- a session's registered cwd can differ from where a `cd &&`-chained command actually runs git, e.g. @@ -52,6 +76,23 @@ MASTER_TOKENS = {"master", "origin/master", "HEAD:master", "refs/heads/master"} +# Write-capable file-edit tools this guard also gates (2026-07-24). Read is +# deliberately excluded -- see module docstring. +WRITE_TOOLS = {"Edit", "Write", "NotebookEdit"} + +# The tool_input key each write tool carries its target path under. +_FILE_PATH_KEYS = ("file_path", "notebook_path") + +_WORKTREES_MARKER = "/.claude/worktrees/" + +# WORKERS.md and journal/ are gitignored and, by established convention +# (CLAUDE.local.md's multi-worker protocol; see docs/lessons.md's +# "Absolute paths to the repo root silently target the wrong checkout in +# a worktree session"), live in the main checkout on purpose -- a worker +# worktree session is EXPECTED to write its own coordination row there. +# These are the one deliberate exception to the trap this rule closes. +_MAIN_CHECKOUT_WRITE_EXCEPTIONS = ("WORKERS.md", "journal/") + def log_stub(rule, command, cwd): try: @@ -212,17 +253,101 @@ def deny(reason): sys.exit(2) +def worktree_main_checkout_root(cwd): + """Return the main checkout root if `cwd` is inside a worker worktree. + + A worker worktree's cwd looks like + `/.claude/worktrees//...`. This returns + `` (normalized to forward slashes for comparison), + or None if `cwd` isn't under `.claude/worktrees/` at all -- i.e. this + is the main checkout's own session, which this rule never gates. + """ + norm = cwd.replace(os.sep, "/") + idx = norm.find(_WORKTREES_MARKER) + if idx == -1: + return None + return norm[:idx] or "/" + + +def resolve_write_target(tool_input, cwd): + """Pull the target path out of an Edit/Write/NotebookEdit tool_input. + + Edit and Write carry it as `file_path`; NotebookEdit carries it as + `notebook_path`. A relative value (not expected from these tools in + practice, but handled defensively) is joined against `cwd` before + normalizing, same as a shell would resolve it. Returns None if + tool_input has neither key or the value isn't a non-empty string. + """ + for key in _FILE_PATH_KEYS: + value = tool_input.get(key) + if isinstance(value, str) and value: + if not os.path.isabs(value): + value = os.path.join(cwd, value) + return os.path.normpath(value).replace(os.sep, "/") + return None + + +def targets_main_checkout(target_path, main_root): + """True if `target_path` resolves inside `main_root` but OUTSIDE any + `.claude/worktrees/` subtree of it (i.e. the shared main checkout's own + tracked files, not any worker worktree's copy -- own or another's). + + False for anything outside `main_root` entirely (paths outside the + repo, e.g. /tmp, the orchestration repo, the memory dir, are never + gated by this rule), and False for the documented WORKERS.md/journal/ + exceptions (see `_MAIN_CHECKOUT_WRITE_EXCEPTIONS`) -- those are + intentional main-checkout writes, not the trap this rule closes. + """ + if target_path != main_root and not target_path.startswith(main_root + "/"): + return False + rel = target_path[len(main_root) :].lstrip("/") + if rel.startswith(".claude/worktrees/"): + return False + if rel in _MAIN_CHECKOUT_WRITE_EXCEPTIONS or any( + rel.startswith(exc) for exc in _MAIN_CHECKOUT_WRITE_EXCEPTIONS if exc.endswith("/") + ): + return False + return True + + +def check_worktree_write_guard(tool_name, tool_input, cwd): + main_root = worktree_main_checkout_root(cwd) + if main_root is None: + return # not a worker worktree session -- this rule doesn't apply + + target = resolve_write_target(tool_input, cwd) + if target is None: + return # no resolvable path in this tool_input -- nothing to judge + + if not targets_main_checkout(target, main_root): + return + + log_stub("write-main-checkout-from-worktree", f"{tool_name} {target}", cwd) + deny( + f"[guard_master] this is a worker worktree (.claude/worktrees/) -- " + f"edit your own worktree's copy, not the main checkout at " + f"{main_root} ({target}). Absolute main-checkout paths from a " + "worktree session silently edit the wrong branch." + ) + + def main(): try: payload = json.load(sys.stdin) except Exception: sys.exit(0) # malformed input: fail open, never block on a parse error - if payload.get("tool_name") != "Bash": + tool_name = payload.get("tool_name") + cwd = payload.get("cwd") or os.getcwd() + + if tool_name in WRITE_TOOLS: + check_worktree_write_guard(tool_name, payload.get("tool_input") or {}, cwd) + sys.exit(0) + + if tool_name != "Bash": sys.exit(0) command = (payload.get("tool_input") or {}).get("command") or "" - cwd = payload.get("cwd") or os.getcwd() in_worker_worktree = "/.claude/worktrees/" in cwd.replace(os.sep, "/") if re.search(r"(^|[;&|])\s*gh\s+pr\s+merge(\s|$)", command): diff --git a/.claude/hooks/test_guard_master.py b/.claude/hooks/test_guard_master.py index 224a1434c..c774fef16 100644 --- a/.claude/hooks/test_guard_master.py +++ b/.claude/hooks/test_guard_master.py @@ -34,6 +34,13 @@ under-triggering only ever produces an unnecessary DENY, never a wrong ALLOW. +2026-07-24: added coverage for the `Edit`/`Write`/`NotebookEdit` +worktree-path-trap guard (see the module docstring's "Scope added +2026-07-24" section) -- both end-to-end `run_hook_payload()` cases +against the actual tool_input shapes those three tools send, and direct +unit cases against `worktree_main_checkout_root()` / `resolve_write_target()` +/ `targets_main_checkout()`. + On the "does this survive bypassPermissions / --dangerously-skip-permissions" requirement: Claude Code's own hook contract guarantees a PreToolUse hook's exit-2 decision applies in every permission mode, including @@ -82,6 +89,13 @@ def run_hook(tool_name, command, cwd): return result.returncode, result.stderr.strip() +def run_hook_payload(payload): + result = subprocess.run( + [sys.executable, HOOK], input=json.dumps(payload), capture_output=True, text=True, timeout=10 + ) + return result.returncode, result.stderr.strip() + + def main(): root = tempfile.mkdtemp(prefix="guard_master_test_") try: @@ -256,6 +270,128 @@ def main(): failures += 1 print(f" expected deny={expect_deny}, got deny={denied}, stderr={stderr!r}") + # End-to-end coverage for the 2026-07-24 Edit/Write/NotebookEdit + # worktree-path-trap guard, against the actual tool_input shapes + # those three tools send (file_path for Edit/Write, notebook_path + # for NotebookEdit). + worker_task_subdir = os.path.join(worker_task, "sub", "dir") + os.makedirs(worker_task_subdir, exist_ok=True) + other_worktree_target = os.path.join(worker_feature, "f.txt") + main_checkout_target = os.path.join(main_master, "f.txt") + own_worktree_target = os.path.join(worker_task, "f.txt") + # Deliberately a sibling of `root`, not under it -- stands in for + # /tmp, the orchestration repo, or the memory dir: genuinely + # outside the repo entirely, not just outside the main checkout. + outside_repo_target = os.path.join(tempfile.gettempdir(), "guard_master_test_outside_repo_notes.txt") + + write_guard_cases = [ + # (label, payload, expect_deny) + ( + "Edit targets main checkout, worker cwd -> DENY", + {"tool_name": "Edit", "cwd": worker_task, "tool_input": {"file_path": main_checkout_target}}, + True, + ), + ( + "Write targets main checkout, worker cwd -> DENY", + { + "tool_name": "Write", + "cwd": worker_task, + "tool_input": {"file_path": main_checkout_target, "content": "x"}, + }, + True, + ), + ( + "NotebookEdit targets main checkout via notebook_path, worker cwd -> DENY", + { + "tool_name": "NotebookEdit", + "cwd": worker_task, + "tool_input": {"notebook_path": main_checkout_target.replace(".txt", ".ipynb")}, + }, + True, + ), + ( + "Edit targets main checkout, cwd nested deep inside the worker worktree -> DENY " + "(main-root resolution isn't sensitive to how deep cwd is under .claude/worktrees/)", + {"tool_name": "Edit", "cwd": worker_task_subdir, "tool_input": {"file_path": main_checkout_target}}, + True, + ), + ( + "Edit targets the session's OWN worktree copy -> ALLOW", + {"tool_name": "Edit", "cwd": worker_task, "tool_input": {"file_path": own_worktree_target}}, + False, + ), + ( + "Edit targets a DIFFERENT worker worktree's copy -> ALLOW " + "(only main-checkout-outside-any-worktree is gated)", + {"tool_name": "Edit", "cwd": worker_task, "tool_input": {"file_path": other_worktree_target}}, + False, + ), + ( + "Edit targets a path entirely outside the repo (e.g. /tmp) -> ALLOW", + {"tool_name": "Edit", "cwd": worker_task, "tool_input": {"file_path": outside_repo_target}}, + False, + ), + ( + "Edit targets the main checkout, but session cwd IS the main checkout (not a worktree) -> ALLOW " + "(rule only applies to worker worktree sessions)", + {"tool_name": "Edit", "cwd": main_master, "tool_input": {"file_path": main_checkout_target}}, + False, + ), + ( + "Edit with a relative file_path from a worker cwd -> ALLOW (resolves under cwd, inside the worktree)", + {"tool_name": "Edit", "cwd": worker_task, "tool_input": {"file_path": "f.txt"}}, + False, + ), + ( + "Read targets the main checkout, worker cwd -> ALLOW (read-only calls are never gated)", + {"tool_name": "Read", "cwd": worker_task, "tool_input": {"file_path": main_checkout_target}}, + False, + ), + ( + # NOTE: relative to `root` (this fixture's `main_root`, since + # `worker_task` = root/.claude/worktrees/worker-task), not + # `main_master` (which is just an unrelated sibling repo used + # by the other cases above) -- see worktree_main_checkout_root(). + "Edit targets main-root WORKERS.md, worker cwd -> ALLOW " + "(documented multi-worker coordination exception)", + { + "tool_name": "Edit", + "cwd": worker_task, + "tool_input": {"file_path": os.path.join(root, "WORKERS.md")}, + }, + False, + ), + ( + "Edit targets a file under main-root journal/, worker cwd -> ALLOW " "(documented journal/ exception)", + { + "tool_name": "Edit", + "cwd": worker_task, + "tool_input": {"file_path": os.path.join(root, "journal", "2026-07-24-notes.md")}, + }, + False, + ), + ( + "Edit targets a file merely NAMED like the journal/ exception " + "(journal-archive/notes.md) -> DENY (no false-negative prefix match)", + { + "tool_name": "Edit", + "cwd": worker_task, + "tool_input": {"file_path": os.path.join(root, "journal-archive", "notes.md")}, + }, + True, + ), + ] + + for label, payload, expect_deny in write_guard_cases: + code, stderr = run_hook_payload(payload) + denied = code == 2 + ok = denied == expect_deny + status = "PASS" if ok else "FAIL" + print(f"[{status}] {label} (exit={code})") + if not ok: + failures += 1 + print(f" expected deny={expect_deny}, got deny={denied}, stderr={stderr!r}") + # Direct unit coverage for effective_dir(), covering the cd-chain # walk and its boundaries (see module docstring / effective_dir()'s # own docstring for what changed 2026-07-22 and why). @@ -353,6 +489,75 @@ def main(): failures += 1 print(f" expected {expected!r}, got {got!r}") + # Direct unit coverage for the worktree-write-guard helpers + # (worktree_main_checkout_root / resolve_write_target / + # targets_main_checkout), independent of the end-to-end cases above. + norm = lambda p: p.replace(os.sep, "/") # noqa: E731 + worktree_helper_cases = [ + ( + "worktree_main_checkout_root: worker cwd -> the dir before .claude/worktrees/", + gm.worktree_main_checkout_root(worker_task), + norm(root), + ), + ( + "worktree_main_checkout_root: main checkout cwd (no .claude/worktrees/ in path) -> None", + gm.worktree_main_checkout_root(main_master), + None, + ), + ( + "resolve_write_target: absolute file_path passes through normalized", + gm.resolve_write_target({"file_path": main_checkout_target}, worker_task), + norm(os.path.normpath(main_checkout_target)), + ), + ( + "resolve_write_target: notebook_path key is also recognized", + gm.resolve_write_target({"notebook_path": "/a/b.ipynb"}, worker_task), + "/a/b.ipynb", + ), + ( + "resolve_write_target: relative file_path resolves against cwd", + gm.resolve_write_target({"file_path": "f.txt"}, worker_task), + norm(os.path.normpath(os.path.join(worker_task, "f.txt"))), + ), + ( + "resolve_write_target: neither key present -> None", + gm.resolve_write_target({"content": "x"}, worker_task), + None, + ), + ] + for label, got, expected in worktree_helper_cases: + ok = got == expected + status = "PASS" if ok else "FAIL" + print(f"[{status}] {label}") + if not ok: + failures += 1 + print(f" expected {expected!r}, got {got!r}") + + targets_main_checkout_cases = [ + ( + "targets_main_checkout: path under main root, not under .claude/worktrees/ -> True", + gm.targets_main_checkout(norm(main_checkout_target), norm(root)), + True, + ), + ( + "targets_main_checkout: path under main root's .claude/worktrees/ subtree -> False", + gm.targets_main_checkout(norm(own_worktree_target), norm(root)), + False, + ), + ( + "targets_main_checkout: path outside main root entirely -> False", + gm.targets_main_checkout(norm(outside_repo_target), norm(root)), + False, + ), + ] + for label, got, expected in targets_main_checkout_cases: + ok = got == expected + status = "PASS" if ok else "FAIL" + print(f"[{status}] {label}") + if not ok: + failures += 1 + print(f" expected {expected!r}, got {got!r}") + # Malformed input: fail open, never block on a parse error. result = subprocess.run([sys.executable, HOOK], input="not json", capture_output=True, text=True, timeout=10) ok = result.returncode == 0 diff --git a/.claude/settings.json b/.claude/settings.json index 4f05b6709..fbde43f54 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -5,7 +5,7 @@ "hooks": { "PreToolUse": [ { - "matcher": "Bash", + "matcher": "Bash|Edit|Write|NotebookEdit", "hooks": [ { "type": "command", diff --git a/docs/lessons.md b/docs/lessons.md index a58a67b86..87a5e329e 100644 --- a/docs/lessons.md +++ b/docs/lessons.md @@ -120,6 +120,18 @@ checkout** specifically, so their absolute main-checkout paths are correct on purpose — the trap is specifically for git-tracked files that need to land on the worktree's branch. +**2026-07-24: closed at the tool layer.** Four independent sessions hit +this exact trap via `Read`/`Edit`/`Write` (not `Bash`) on 2026-07-23/24 +alone, all self-caught before landing anything. `guard_master.py`'s +PreToolUse hook now also matches `Edit`/`Write`/`NotebookEdit` calls and +blocks any absolute target path that resolves inside the main checkout +root but outside `.claude/worktrees/` from a worker worktree session — +same `WORKERS.md`/`journal/` exception preserved, `Read` deliberately +left ungated (see `.claude/hooks/guard_master.py`'s module docstring and +`docs/troubleshooting.md`'s `guard_master.py` entry for the mechanics). +The advice above (always use relative/`pwd`-derived paths) still holds +as the primary defense — the hook is the backstop, not a replacement. + ## Swap in a debug color to disambiguate same-colored overlapping elements A pixel/computed-color check at one sample point can be genuinely ambiguous diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 54c1ec9c1..40c5c2bf2 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -1398,6 +1398,49 @@ under-triggering only ever produces an unnecessary DENY, never a wrong ALLOW, and `cd`-chains are the pattern that actually occurred in production. +**2026-07-24: extended to `Edit`/`Write`/`NotebookEdit`, closing the +worktree-path trap at the tool layer.** Everything above in this entry +covers `guard_master.py`'s `Bash`-matched rules only. Separately from +those, `.claude/settings.json`'s PreToolUse matcher now also fires on +`Edit`, `Write`, and `NotebookEdit`, and `guard_master.py` dispatches +those three to `check_worktree_write_guard()` before falling through to +the `Bash`-only logic. Motivation: four independent sessions +accidentally edited the shared main checkout via absolute-path +`Read`/`Edit`/`Write` calls on 2026-07-23/24 (all self-caught before +landing anything) — the exact failure mode described in +`docs/lessons.md`'s "Absolute paths to the repo root silently target +the wrong checkout in a worktree session", but via a file tool instead +of `git`/`gh` in `Bash`, so the existing rules never saw it. + +Mechanics: `worktree_main_checkout_root(cwd)` returns the path before +`/.claude/worktrees/` in the session's cwd (None if the session isn't a +worker worktree at all, in which case this rule is a no-op). +`resolve_write_target(tool_input, cwd)` pulls the target path out of +`tool_input["file_path"]` (`Edit`/`Write`) or `tool_input["notebook_path"]` +(`NotebookEdit`), joining a relative value against `cwd` first. +`targets_main_checkout(target, main_root)` is True only when the +resolved target is under `main_root` but NOT under `main_root`'s own +`.claude/worktrees/` subtree (so the session's own worktree, any other +session's worktree, and anything outside the repo entirely — `/tmp`, +the orchestration repo, the memory dir — all pass through untouched), +with one deliberate exception: `WORKERS.md` and `journal/` (see the +`docs/lessons.md` entry above) are excluded from the block, since a +worker worktree session writing its own coordination row there is the +documented, intentional workflow, not the trap. + +`Read` is deliberately never gated — blocking reads would break +legitimate cross-referencing against the main checkout's on-disk state +(e.g. diffing a worktree's change against what's on master), and a read +can't silently land content on the wrong branch the way a write can. + +Regression coverage: `.claude/hooks/test_guard_master.py`'s +`write_guard_cases` (end-to-end, via `run_hook_payload()`, against the +real `tool_input` shapes each of the three tools sends) plus direct unit +cases against the three helper functions above. This suite has no CI +wiring (same as the pre-existing `Bash`-rule tests it extends) — it's a +local dev-loop check, run manually with +`python3 .claude/hooks/test_guard_master.py`. + ## `DisplayPage.spec.ts`'s "floating sheet-position pill updates live while scrolling at phone width (D17)" fails intermittently with "2/3" instead of "3/3" **Symptom**: `tests/DisplayPage.spec.ts`'s sheet-position-pill test