diff --git a/.agents/skills/repo-worktree/SKILL.md b/.agents/skills/repo-worktree/SKILL.md index d4f9e0e4..9e675f0a 100644 --- a/.agents/skills/repo-worktree/SKILL.md +++ b/.agents/skills/repo-worktree/SKILL.md @@ -61,6 +61,18 @@ from `develop` unless the task is explicitly about `main`-only content, per `GOV "Branching Model". Fetch immediately before creating and base on the remote ref, because a clone is whatever it last fetched rather than the branch it names. +The base clone is a fetch source, not a place to do task work. `fetch` and `worktree add` run +against it for that purpose, and outside "Listing and Cleanup"'s own terminal step below, nothing +else does: never `checkout`, `pull`, `reset`, `commit`, or any other command that mutates its own +working tree, index, or HEAD while a task is in progress. That distinction is the one a real +incident missed, reusing a primary checkout as the working directory itself rather than only as +the source a worktree is created from. On Claude Code this is also a mechanical stop for most +of that list. `merge --ff-only`/`pull --ff-only` and a `checkout `/`switch ` carrying no +force flag stay exempt even there, matching this skill's own cleanup step below, which needs +exactly those. +Prose remains the only enforcement for a non-Claude-Code agent, and for the shapes the hook itself +exempts. + ## Creating a Worktree The fleet layout convention keeps every base clone and every in-flight task visible in one @@ -123,6 +135,13 @@ task branch from `origin/develop`. A standalone clone keeps its worktree and Git directory under the same writable root. It therefore supports edits, explicit-path staging, commits, and branch updates without sharing the base clone's index. +On Claude Code, a standalone clone is structurally a primary checkout to `gh-write-guard.py`'s own +rule 6 test (`--git-dir` equals `--git-common-dir` there too, since it is not a linked worktree of +anything), so the hook denies the very commits/edits this fallback exists to make. Set +`GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT` for the session before using one -- the same escape hatch +`host-setup/agent-safety/README.md`'s requirement 6 already documents -- since this fallback is +exactly the narrow, already-approval-gated case that grant exists for. + A temporary standalone clone is a degraded handoff, not an equivalent location. The base clone does not register it, `git worktree list` does not show it, and an IDE opened on the base clone does not discover its changes. The maintainer must navigate to it manually, and the operating diff --git a/.agents/skills/resync-a-repo/SKILL.md b/.agents/skills/resync-a-repo/SKILL.md index 9ac991f7..67918990 100644 --- a/.agents/skills/resync-a-repo/SKILL.md +++ b/.agents/skills/resync-a-repo/SKILL.md @@ -37,8 +37,15 @@ anything else. The finding kind names the procedure the repo is owed. ## Reach the hub and measure before changing anything Fetch a hub checkout of your own immediately before reading it, per RESYNC.md section 1, since a -stale clone answers confidently instead of failing, and verify the host with -`python3 scripts/host_gate.py --repo `. Then run the audit end to end, +stale clone answers confidently instead of failing. Never operate against an existing checkout +already present at a known or shared path, the maintainer's own primary checkout included, even +one that looks current -- always fetch into a private worktree of your own, per `repo-worktree`. +On Claude Code this is now also a mechanical stop for most such commands (a `PreToolUse` hook +denies a mutating git operation run directly in a primary checkout), though this prose is still +the only enforcement for a non-Claude-Code agent, and for the narrow shapes the hook itself +exempts, so following it here is not optional even where no hook can catch a lapse. Verify the +host with `python3 scripts/host_gate.py --repo `. Then run the audit end +to end, RESYNC.md section 2, against the target's `main` branch, never `develop`. A finding is a snapshot, so quote the run stamp in anything derived from it and re-run before acting on a finding read earlier in the session. diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 1254e367..c46d6208 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -7c7db19359804689 +76568e633c063da3 diff --git a/.claude-plugin/fleet-skills/skills/repo-worktree/SKILL.md b/.claude-plugin/fleet-skills/skills/repo-worktree/SKILL.md index d4f9e0e4..9e675f0a 100644 --- a/.claude-plugin/fleet-skills/skills/repo-worktree/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/repo-worktree/SKILL.md @@ -61,6 +61,18 @@ from `develop` unless the task is explicitly about `main`-only content, per `GOV "Branching Model". Fetch immediately before creating and base on the remote ref, because a clone is whatever it last fetched rather than the branch it names. +The base clone is a fetch source, not a place to do task work. `fetch` and `worktree add` run +against it for that purpose, and outside "Listing and Cleanup"'s own terminal step below, nothing +else does: never `checkout`, `pull`, `reset`, `commit`, or any other command that mutates its own +working tree, index, or HEAD while a task is in progress. That distinction is the one a real +incident missed, reusing a primary checkout as the working directory itself rather than only as +the source a worktree is created from. On Claude Code this is also a mechanical stop for most +of that list. `merge --ff-only`/`pull --ff-only` and a `checkout `/`switch ` carrying no +force flag stay exempt even there, matching this skill's own cleanup step below, which needs +exactly those. +Prose remains the only enforcement for a non-Claude-Code agent, and for the shapes the hook itself +exempts. + ## Creating a Worktree The fleet layout convention keeps every base clone and every in-flight task visible in one @@ -123,6 +135,13 @@ task branch from `origin/develop`. A standalone clone keeps its worktree and Git directory under the same writable root. It therefore supports edits, explicit-path staging, commits, and branch updates without sharing the base clone's index. +On Claude Code, a standalone clone is structurally a primary checkout to `gh-write-guard.py`'s own +rule 6 test (`--git-dir` equals `--git-common-dir` there too, since it is not a linked worktree of +anything), so the hook denies the very commits/edits this fallback exists to make. Set +`GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT` for the session before using one -- the same escape hatch +`host-setup/agent-safety/README.md`'s requirement 6 already documents -- since this fallback is +exactly the narrow, already-approval-gated case that grant exists for. + A temporary standalone clone is a degraded handoff, not an equivalent location. The base clone does not register it, `git worktree list` does not show it, and an IDE opened on the base clone does not discover its changes. The maintainer must navigate to it manually, and the operating diff --git a/.claude-plugin/fleet-skills/skills/resync-a-repo/SKILL.md b/.claude-plugin/fleet-skills/skills/resync-a-repo/SKILL.md index 9ac991f7..67918990 100644 --- a/.claude-plugin/fleet-skills/skills/resync-a-repo/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/resync-a-repo/SKILL.md @@ -37,8 +37,15 @@ anything else. The finding kind names the procedure the repo is owed. ## Reach the hub and measure before changing anything Fetch a hub checkout of your own immediately before reading it, per RESYNC.md section 1, since a -stale clone answers confidently instead of failing, and verify the host with -`python3 scripts/host_gate.py --repo `. Then run the audit end to end, +stale clone answers confidently instead of failing. Never operate against an existing checkout +already present at a known or shared path, the maintainer's own primary checkout included, even +one that looks current -- always fetch into a private worktree of your own, per `repo-worktree`. +On Claude Code this is now also a mechanical stop for most such commands (a `PreToolUse` hook +denies a mutating git operation run directly in a primary checkout), though this prose is still +the only enforcement for a non-Claude-Code agent, and for the narrow shapes the hook itself +exempts, so following it here is not optional even where no hook can catch a lapse. Verify the +host with `python3 scripts/host_gate.py --repo `. Then run the audit end +to end, RESYNC.md section 2, against the target's `main` branch, never `develop`. A finding is a snapshot, so quote the run stamp in anything derived from it and re-run before acting on a finding read earlier in the session. diff --git a/.github/actions/validate/action.yml b/.github/actions/validate/action.yml index caf2ed79..f7792dad 100644 --- a/.github/actions/validate/action.yml +++ b/.github/actions/validate/action.yml @@ -33,8 +33,8 @@ runs: uvx coverage@latest run --source=scripts,spec,host-setup -m unittest discover -s scripts/tests uvx coverage@latest run --source=scripts,spec,host-setup --append spec/audit.py --selftest uvx coverage@latest run --source=scripts,spec,host-setup --append spec/workflow_reuse.py --selftest - uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/gh-write-guard.py --selftest - uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/test_install.py + uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/claude/gh-write-guard.py --selftest + uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/claude/test_install.py uvx coverage@latest report # Read-only: fails if a generated skill distribution differs from .agents/skills/. diff --git a/.github/skills/repo-worktree/SKILL.md b/.github/skills/repo-worktree/SKILL.md index d4f9e0e4..9e675f0a 100644 --- a/.github/skills/repo-worktree/SKILL.md +++ b/.github/skills/repo-worktree/SKILL.md @@ -61,6 +61,18 @@ from `develop` unless the task is explicitly about `main`-only content, per `GOV "Branching Model". Fetch immediately before creating and base on the remote ref, because a clone is whatever it last fetched rather than the branch it names. +The base clone is a fetch source, not a place to do task work. `fetch` and `worktree add` run +against it for that purpose, and outside "Listing and Cleanup"'s own terminal step below, nothing +else does: never `checkout`, `pull`, `reset`, `commit`, or any other command that mutates its own +working tree, index, or HEAD while a task is in progress. That distinction is the one a real +incident missed, reusing a primary checkout as the working directory itself rather than only as +the source a worktree is created from. On Claude Code this is also a mechanical stop for most +of that list. `merge --ff-only`/`pull --ff-only` and a `checkout `/`switch ` carrying no +force flag stay exempt even there, matching this skill's own cleanup step below, which needs +exactly those. +Prose remains the only enforcement for a non-Claude-Code agent, and for the shapes the hook itself +exempts. + ## Creating a Worktree The fleet layout convention keeps every base clone and every in-flight task visible in one @@ -123,6 +135,13 @@ task branch from `origin/develop`. A standalone clone keeps its worktree and Git directory under the same writable root. It therefore supports edits, explicit-path staging, commits, and branch updates without sharing the base clone's index. +On Claude Code, a standalone clone is structurally a primary checkout to `gh-write-guard.py`'s own +rule 6 test (`--git-dir` equals `--git-common-dir` there too, since it is not a linked worktree of +anything), so the hook denies the very commits/edits this fallback exists to make. Set +`GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT` for the session before using one -- the same escape hatch +`host-setup/agent-safety/README.md`'s requirement 6 already documents -- since this fallback is +exactly the narrow, already-approval-gated case that grant exists for. + A temporary standalone clone is a degraded handoff, not an equivalent location. The base clone does not register it, `git worktree list` does not show it, and an IDE opened on the base clone does not discover its changes. The maintainer must navigate to it manually, and the operating diff --git a/.github/skills/resync-a-repo/SKILL.md b/.github/skills/resync-a-repo/SKILL.md index 9ac991f7..67918990 100644 --- a/.github/skills/resync-a-repo/SKILL.md +++ b/.github/skills/resync-a-repo/SKILL.md @@ -37,8 +37,15 @@ anything else. The finding kind names the procedure the repo is owed. ## Reach the hub and measure before changing anything Fetch a hub checkout of your own immediately before reading it, per RESYNC.md section 1, since a -stale clone answers confidently instead of failing, and verify the host with -`python3 scripts/host_gate.py --repo `. Then run the audit end to end, +stale clone answers confidently instead of failing. Never operate against an existing checkout +already present at a known or shared path, the maintainer's own primary checkout included, even +one that looks current -- always fetch into a private worktree of your own, per `repo-worktree`. +On Claude Code this is now also a mechanical stop for most such commands (a `PreToolUse` hook +denies a mutating git operation run directly in a primary checkout), though this prose is still +the only enforcement for a non-Claude-Code agent, and for the narrow shapes the hook itself +exempts, so following it here is not optional even where no hook can catch a lapse. Verify the +host with `python3 scripts/host_gate.py --repo `. Then run the audit end +to end, RESYNC.md section 2, against the target's `main` branch, never `develop`. A finding is a snapshot, so quote the run stamp in anything derived from it and re-run before acting on a finding read earlier in the session. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index cc652655..cfc04bb5 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -17,6 +17,7 @@ The specific rules in this file implement a few governing principles. Read these - **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor (a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid) belongs in a committed governance file (`AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog such as a `README.md` TODO section). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state, never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. - **Keep the governance current as you work.** When work surfaces something durable (a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out), record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream instead of only fixing it locally. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. +- **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** Three conditions together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all three (the #1073 incident happened under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. This section keeps the full rules and is surfaced at its decision moment by the `agent-conduct` Skill at `.agents/skills/agent-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. @@ -30,7 +31,7 @@ A state-changing GitHub call is the highest-blast-radius thing an agent does her - **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`), because the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless**, because the operation may have succeeded on the server while the client reported an error, so confirm the actual state before retrying or moving on. The ban targets hiding a *failure*. An ad-hoc call's response is the only signal you get, so `>/dev/null 2>&1`, `|| true`, and `|| echo`, which swallow the error stream or force success, are never acceptable on one. A committed script under `set -e` is a narrow exception: it may send a write's *stdout* to `/dev/null` to drop the success-response noise, because stderr stays visible and a failed write still aborts loudly (the hub's own `repo-config/configure.sh` does exactly this, and a repository reaches it there rather than carrying a copy). The exception is stdout-only suppression inside a reviewed, fail-loud script, never `2>&1` or a force-success tail, and never an ad-hoc command. - **A refused write is reported, never re-shaped, and the maintainer's say-so does not lift a refusal by the harness.** These are two different permissions and only one of them is the maintainer's to give. When the agent harness refuses a write, the maintainer authorizing it in conversation does not change the outcome, and the identical call is refused again, so a second attempt is not worth making and reading the second refusal as a flake is how an agent starts hunting for another shape of the same request. **That hunt is the failure this rule exists to stop.** Re-expressing a refused `gh` command as a raw `gh api -X POST` reaches the same endpoint with the same identity and the same blast radius, having defeated the one control that stopped it, and it is the more dangerous version because the agent believes it has permission. So a refused write is never re-attempted through a different API surface, a different tool, or a rephrasing, and it is never routed around by the agent writing itself a permission rule, which is self-authorization whatever the maintainer said. Two routes remain, both of them the maintainer's: they add the permission rule themselves, or they run the command themselves. Raise it as a blocked decision naming those two (see "Communicating with the User"), and where the work needs the result rather than the call, say what the agent will verify once the maintainer has run it. **A refusal is also a fact about the contract, not just about the session**: where a required verification can only be performed by a write the agent is refused, the document requiring it says so and names who runs it, since a check that is mandatory and unperformable is quietly dropped and then reported as done. - **Each task runs in its own checkout, in its own directory, on its own feature branch.** The unit is the task rather than the agent, since one agent moving between two repositories meets the same hazard as two agents sharing one tree, and a rule written per agent permits exactly the case that goes wrong. The commands that cross the boundary are the ordinary ones rather than the reckless ones, and each is correct in isolation: a blanket `git add -A` sweeps another task's uncommitted work into the commit, a `git reset --hard` deletes it, and a branch switch carries it into an unrelated change. The mechanical habit that holds the rule up is that a mutating command takes an absolute path, or a `cd` to one in the same invocation, rather than the working directory it inherited, because a read in the wrong directory is a wasted call and a write there is damage. -- **A task isolates into its own worktree before its first file edit, and a continuation re-isolates.** All new work begins by creating a unique git worktree (or clone) on its own feature branch, based on the branch work starts on for the repository's model per "Branching Model", which is `develop` unless the task is explicitly about `main`-only content. The primary checkout is the maintainer's own surface, so a session launched there isolates before writing rather than after noticing contention, and a session resuming a prior task creates a fresh worktree rather than resuming wherever its branch happens to be checked out, since a branch sitting checked out in a shared tree is exactly how two sessions end up in one checkout. The moment this rule binds is the first file edit, because the commit-time and review-time checks all run after another task's uncommitted work can already be swept. The worktree mechanics, the layout convention, and the cleanup are packaged as the `repo-worktree` Skill at `.agents/skills/repo-worktree/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, and this section keeps the rule. +- **A task isolates into its own worktree before its first file edit, and a continuation re-isolates.** All new work begins by creating a unique git worktree (or clone) on its own feature branch, based on the branch work starts on for the repository's model per "Branching Model", which is `develop` unless the task is explicitly about `main`-only content. The primary checkout is the maintainer's own surface, so a session launched there isolates before writing rather than after noticing contention, and a session resuming a prior task creates a fresh worktree rather than resuming wherever its branch happens to be checked out, since a branch sitting checked out in a shared tree is exactly how two sessions end up in one checkout. The moment this rule binds is the first file edit, because the commit-time and review-time checks all run after another task's uncommitted work can already be swept. The worktree mechanics, the layout convention, and the cleanup are packaged as the `repo-worktree` Skill at `.agents/skills/repo-worktree/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, and this section keeps the rule. For Claude Code sessions, the `gh-write-guard` hook now backstops a mechanical subset of this rule directly, denying a mutating git operation (`reset`, `add`, `commit`, and most of the rest, a documented handful of exemptions such as a fast-forward-only pull kept aside) run against a primary checkout rather than a worktree. Every other agent, and everything about isolation a hook cannot see (which checkout a read happens in, whether another task is live in the tree), still relies on this prose alone. The parenthetical "(or clone)" above names `repo-worktree`'s own standalone-clone fallback, used when a linked worktree is unavailable, and that fallback is itself structurally a primary checkout to the hook's own primary-vs-worktree test, so a session using it on Claude Code sets the hook's `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT` grant for that session, per `repo-worktree`'s own instructions, rather than being silently denied the commits the fallback exists to make. - **A checkout another task is live in is left rather than shared, and a footprint already left there is undone deliberately.** Two signals say someone else is in the tree, a branch that changes when nothing you did changed it, and an edit of yours reverted with no conflict, and the response to either is to stop rather than to re-apply the edit, which is the instinct and the wrong one. Leaving and cloning your own costs about a minute against an incident that costs the better part of an hour, so it is the cheap move rather than the cautious one. Once you have written there, leaving it alone arrives too late, so save your work aside, restore only the files you touched, verify the tree is clean, delete your branch from that clone, and then say plainly what was touched, since a regenerated report left behind reads as the other task's own and is committed by whoever runs the next blanket add. ## Representative Data in Agent-Authored Text diff --git a/OPERATIONS.md b/OPERATIONS.md index 28e81e80..15dce873 100644 --- a/OPERATIONS.md +++ b/OPERATIONS.md @@ -23,8 +23,8 @@ uvx ruff@latest format --check . uvx mypy@latest uvx coverage@latest run --source=scripts,spec,host-setup -m unittest discover -s scripts/tests uvx coverage@latest run --source=scripts,spec,host-setup --append spec/audit.py --selftest -uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/gh-write-guard.py --selftest -uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/test_install.py +uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/claude/gh-write-guard.py --selftest +uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/claude/test_install.py uvx coverage@latest report python3 scripts/build_dist.py --check python3 scripts/repo_gate.py diff --git a/README.md b/README.md index ab0b8676..dd0f67cc 100644 --- a/README.md +++ b/README.md @@ -163,21 +163,24 @@ Four things are deployed from here, and they land in different places. The host The guardrails are **host state rather than repository content**, because they have to cover ad-hoc sessions in no project at all. Each provider's implementation stays separate: -- **Claude Code:** the installable safety kit denies a mis-targeted GitHub write under your identity. It also denies a git operation that would bypass a branch rule. -- **Codex:** no equivalent host hook ships yet. The carried repository rules and Codex's own sandbox and execution policies remain active. [Issue #781][issue-781] tracks the missing hook. -- **opencode:** no equivalent host hook ships yet. The carried repository rules and opencode's own permission model remain active. [Issue #781][issue-781] tracks the missing hook. +- **Claude Code:** the installable safety kit denies a mis-targeted GitHub write under your identity. It also denies a git operation that would bypass a branch rule, or run directly against a primary checkout. [`host-setup/agent-safety/claude/README.md`][agent-safety-claude] has the details. +- **Codex:** no equivalent host hook ships yet. The carried repository rules and Codex's own sandbox and execution policies remain active. [Issue #781][issue-781] tracks the missing hook, and [`host-setup/agent-safety/codex/README.md`][agent-safety-codex] states the gap. +- **opencode:** no equivalent host hook ships yet. The carried repository rules and opencode's own permission model remain active. [Issue #781][issue-781] tracks the missing hook, and [`host-setup/agent-safety/opencode/README.md`][agent-safety-opencode] states the gap. + +The requirements every agent's kit is built and audited against, agent-agnostic, are in +[`host-setup/agent-safety/README.md`][agent-safety]. #### Claude Code ```shell -host-setup/agent-safety/install.sh # Linux, WSL, macOS +host-setup/agent-safety/claude/install.sh # Linux, WSL, macOS ``` ```powershell -.\host-setup\agent-safety\install.ps1 # Windows, and the .\ prefix is required +.\host-setup\agent-safety\claude\install.ps1 # Windows, and the .\ prefix is required ``` -Restart Claude Code sessions on the machine afterward so the hook and the `CLAUDE.md` blocks load. The installer is idempotent, so re-running it is also how a machine picks up an upstream change to the guard. What it installs, how to verify it, and what it deliberately does not catch are in [`host-setup/agent-safety/README.md`][agent-safety]. The surrounding host prerequisites are in [`docs/host-setup.md`][host-setup]. +Restart Claude Code sessions on the machine afterward so the hook and the `CLAUDE.md` blocks load. The installer is idempotent, so re-running it is also how a machine picks up an upstream change to the guard. What it installs, how to verify it, and what it deliberately does not catch are in [`host-setup/agent-safety/claude/README.md`][agent-safety-claude]. The surrounding host prerequisites are in [`docs/host-setup.md`][host-setup]. ### Install the Fleet Skills @@ -220,7 +223,7 @@ A rule that cannot be diverged from is a rule people work around silently, which The guard denies a `gh` write whose explicit target sits under an owner other than the checkout's `origin` owner, which is the shape that once put a stray comment on a stranger's repository. Sibling repositories under the same owner are allowed, so the denial appears only on a write that leaves the owner, and the common case that raises it is a fork, where `origin` is yours and `upstream` is the project you forked from. -The only way past it is a grant the maintainer makes **outside the session**, in `GH_WRITE_GUARD_ALLOW`. It is deliberately not something an agent can do for itself once blocked, so an inline `GH_WRITE_GUARD_ALLOW=owner/repo gh ...` prefix and an `export` inside a shell call both leave the write denied. The worked example, the file the grant goes in, and how to confirm one took effect are in [`docs/host-setup.md` "Granting a Write the Guard Denies"][host-setup-granting-a-write-the-guard-denies]. +The only way past it is a grant the maintainer makes **outside the session**, in `GH_WRITE_GUARD_ALLOW`. It is deliberately not something an agent can do for itself once blocked, so an inline `GH_WRITE_GUARD_ALLOW=owner/repo gh ...` prefix and an `export` inside a shell call both leave the write denied. The worked example, the file the grant goes in, and how to confirm one took effect are in [`host-setup/agent-safety/claude/README.md` "Granting a Write That the Guard Denies"][agent-safety-claude-grant]. ## Rules @@ -342,6 +345,10 @@ Licensed under the [MIT License][license]\ [agent-safety]: ./host-setup/agent-safety/README.md +[agent-safety-claude]: ./host-setup/agent-safety/claude/README.md +[agent-safety-claude-grant]: ./host-setup/agent-safety/claude/README.md#granting-a-write-that-the-guard-denies +[agent-safety-codex]: ./host-setup/agent-safety/codex/README.md +[agent-safety-opencode]: ./host-setup/agent-safety/opencode/README.md [agents]: ./AGENTS.md [audit]: ./AUDIT.md [catalog]: ./catalog/ @@ -364,7 +371,6 @@ Licensed under the [MIT License][license]\ [history]: ./HISTORY.md [host-setup]: ./docs/host-setup.md [host-setup-dir]: ./host-setup/ -[host-setup-granting-a-write-the-guard-denies]: ./docs/host-setup.md#granting-a-write-the-guard-denies [issue-781]: https://github.com/ptr727/ProjectTemplate/issues/781 [license]: ./LICENSE [matrix]: ./reports/conformance-matrix.md diff --git a/TODO.md b/TODO.md index ee31d2cf..703502d5 100644 --- a/TODO.md +++ b/TODO.md @@ -578,4 +578,4 @@ Regenerate [reports/divergences.md][divergences-report] before using it as the w [validate-task]: ./.github/workflows/validate-task.yml [workflow]: ./WORKFLOW.md [workflow-reuse-report]: ./reports/workflow-reuse.md -[write-guard]: ./host-setup/agent-safety/gh-write-guard.py +[write-guard]: ./host-setup/agent-safety/claude/gh-write-guard.py diff --git a/docs/host-setup.md b/docs/host-setup.md index d7807ff7..f1febfb7 100644 --- a/docs/host-setup.md +++ b/docs/host-setup.md @@ -185,65 +185,32 @@ Choose the SSH key generated above when prompted. Host-level write safety is required where an agent runs with the maintainer's `gh` credentials. Each provider's implementation stays in its own subsection. -### Claude Code Write Safety - -The Claude Code safety kit is the first agent-specific control to deploy on a new system. Install it from this repo. The installer is idempotent and safe to re-run: - -```shell -host-setup/agent-safety/install.sh # Linux, WSL, macOS -``` - -```powershell -.\host-setup\agent-safety\install.ps1 # Windows, and the .\ prefix is required -``` - -Both wrap one `install.py`, so every platform runs the same tested path. Restart Claude Code sessions on the machine afterward so the hook and the `CLAUDE.md` block load. Details, verification, and scope limits are in [`host-setup/agent-safety/README.md`][agent-safety]. - -This is a **host** control, not a repo one. The carried `GOVERNANCE.md` rules reach fleet repos only, while the hook and the `CLAUDE.md` block cover every session on the machine, including ad-hoc work in no project at all, which is where the incident behind the kit happened. - -#### Granting a Write the Guard Denies - -The guard denies a `gh` write whose explicit target sits under an owner other than the checkout's `origin` owner, and the denial names `GH_WRITE_GUARD_ALLOW` as the way past it. That grant is the maintainer's to make, and making it is a deliberate act taken outside the session rather than something an agent does for itself once blocked. - -**The case that raises it is usually a fork.** `origin` is your own fork under your own owner, and `upstream` is the project it was forked from under someone else's. Everything aimed at the fork is in scope and never denies, and only the half that leaves the owner stops: filing an issue on the upstream, opening a pull request against it, or commenting on one there. The grant therefore names the upstream alone, and the fork needs no grant at all. That asymmetry is what a reader hits first, since half the session's writes succeed and the other half do not. - -**The grant goes in the checkout's `.claude/settings.local.json`, as an `env` block:** - -```json -{ - "env": { - "GH_WRITE_GUARD_ALLOW": "upstream-owner/upstream-repo second-owner/other-repo third-owner/*" - } -} -``` - -**The value is one string holding every grant, never a JSON array**, since the hook reads an environment variable and an environment variable is a string. The three tokens above are three separate grants: two naming one repository each, and `third-owner/*` granting every repository under that owner. +The requirements every agent's kit is built and audited against, agent-agnostic, are in +[`host-setup/agent-safety/README.md`][agent-safety], the spec. Each agent's own implementation +detail lives one level down, following the same contract-vs-implementation split this file uses +for [`host-setup/`][host-setup-dir]'s own per-platform subdirectories: this file states the +requirement, the per-agent `README.md` owns the how-to. -Tokens are separated by **any run of whitespace or commas**, so `a/b c/d`, `a/b,c/d`, and `a/b, c/d` all parse to the same two grants and the choice is cosmetic. A token carrying no `/` is ignored, so a malformed entry grants nothing rather than granting everything, and it also fails silently, which is why the confirmation step below is worth running. Grant the narrowest thing that unblocks the work, since a repository grant does not extend to that owner's other repositories and that containment is the property worth keeping. - -**The grant is per checkout, not per host.** `.claude/settings.local.json` lives in the working tree and is git-ignored, so it applies to sessions started in that checkout and does not follow the agent into another repository's sessions. That is the intended scope: a grant made to file one upstream issue from one fork does not quietly become a standing permission everywhere. - -**Restart the session afterward.** The hook reads the value from the environment the session was launched with, which is what makes the channel one an agent cannot use on itself, and it is equally why a grant added to a live session does nothing until that session restarts. - -**Two forms look right and leave the write denied.** An inline `GH_WRITE_GUARD_ALLOW=owner/repo gh ...` prefix sets the environment of the `gh` process, and an `export` inside a shell call sets the environment of that shell. The hook runs as its own process and sees neither, so the write stays denied with nothing to explain the difference. [`gh-write-guard.py`][write-guard] asserts the inline-prefix case in its own self-test, so this is settled behavior rather than a quirk to work around. - -**Confirm the grant loaded before relying on it**, since inferring it from a write that no longer denies means learning the answer by making the write. In a restarted session in that checkout, read the variable the hook reads: - -```shell -printenv GH_WRITE_GUARD_ALLOW -``` - -Run it bare, with no `VAR=value` prefix of its own, which would report a value the hook never sees. An empty result means the grant did not load, and the fix is the file location or the restart rather than the token. Feeding the hook a synthetic payload is not a usable probe from inside a session, because the payload text carries the very write shape the guard matches and the guard denies the probe command itself. +### Claude Code Write Safety -Withdraw a grant by deleting the `env` entry and restarting. Nothing expires it, so a grant left in place stays live for every later session in that checkout, which is the reason to remove it once the work that needed it is done. +The Claude Code safety kit is the first agent-specific control to deploy on a new system, and the +only one implemented today. Install, verify, scope limits, and the cross-owner write grant +mechanism are all in [`host-setup/agent-safety/claude/README.md`][agent-safety-claude]. This is a +**host** control, not a repo one: the carried `GOVERNANCE.md` rules reach fleet repos only, while +the hook and the `CLAUDE.md` block cover every session on the machine, including ad-hoc work in no +project at all, which is where the incident behind the kit happened. ### Codex Write Safety -No equivalent host write hook ships yet for Codex. Keep Codex's sandbox and execution policies enabled. The carried repository rules remain the behavioral layer in a fleet checkout. [Issue #781][issue-781] tracks the missing hook. +No equivalent host write hook ships yet for Codex. Keep Codex's sandbox and execution policies +enabled meanwhile. [`host-setup/agent-safety/codex/README.md`][agent-safety-codex] states the gap +and what implementing against the spec would look like. [Issue #781][issue-781] tracks it. ### opencode Write Safety -No equivalent host write hook ships yet for opencode. Keep opencode's own permission model enabled. The carried repository rules remain the behavioral layer in a fleet checkout. [Issue #781][issue-781] tracks the missing hook. +No equivalent host write hook ships yet for opencode. Keep opencode's own permission model enabled +meanwhile. [`host-setup/agent-safety/opencode/README.md`][agent-safety-opencode] states the gap and +what implementing against the spec would look like. [Issue #781][issue-781] tracks it. ## Agent Worktree Access @@ -274,7 +241,34 @@ This setting grants filesystem writes only under the worktree parent. It does no ### Claude Code Worktree Access -Claude Code does not read Codex's `config.toml`. Keep its permission mode in Claude Code's user-level configuration. Its sessions must be allowed to create and edit the same host-specific worktree parent. +Claude Code does not read Codex's `config.toml`. Keep its permission mode in Claude Code's user-level configuration. + +**The one-time approval prompt on `EnterWorktree` into the fleet's worktree parent is expected, and not eliminable through permission rules.** Claude Code's own `EnterWorktree` tool defaults to `.claude/worktrees//` under the repository root. Entering a path outside that directory (the fleet's own `~/repos/worktrees/-` convention always is) asks for approval first, because the move relocates the session's working directory, write access, and project configuration. Neither an `EnterWorktree(...)` permission rule nor "don't ask again" suppresses this specific prompt -- only bypassPermissions mode does, whether set as a standing default (the nested `defaultMode` key under `permissions` in `settings.json`, set to `"bypassPermissions"`) or a one-time session override (`--permission-mode bypassPermissions`, or its `--dangerously-skip-permissions` alias), none of which is recommended as a standing setting. Stop trying to permission this prompt away. It fires once per worktree entered, by design. + +Separately, ordinary `Bash(...)` rules do stop the *follow-up* command prompts once inside the worktree, as long as the pattern matches what actually runs there. A command executed after Claude has moved into the worktree (via `cd`, or via `EnterWorktree` itself) runs with no `-C`/prefix naming the worktree, so a rule scoped to `Bash(git -C :*)` does not match it -- match the bare command instead: + +```json +{ + "permissions": { + "allow": [ + "Bash(git worktree add:*)", + "Bash(git worktree list:*)", + "Bash(git add:*)", + "Bash(git commit:*)", + "Bash(git push:*)" + ], + "additionalDirectories": [ + "/absolute/path/to/repos/worktrees" + ] + } +} +``` + +`additionalDirectories` grants filesystem read/write scope under that path. It does not itself suppress a Bash or `EnterWorktree` confirmation, so it is necessary alongside the rules above, not a substitute for them. + +**These `:*` rules are a confirmation-prompt convenience, not a safety boundary, and admit destructive variants of the same commands** (`git add -A`, `git commit -a`, `git worktree add --force`). Claude Code's permission layer decides whether a prompt appears before a command runs, it does not decide whether the command itself is safe -- that is `git-commit-conventions`' "stage by explicit path, never `git add -A`" prose discipline for staging, and `gh-write-guard.py`'s own rule 6 for a mutation aimed at the primary checkout specifically, neither of which this permission block replaces or narrows. A worktree the agent's own task owns is exactly where these ordinary forms are expected to run, so the trade is deliberate: broader, prompt-free convenience for routine work inside a task's own worktree, resting on the agent's documented discipline rather than a mechanical block, the same posture Claude Code's permission system takes everywhere else. + +**Call `EnterWorktree` after `git worktree add`, not just `cd`.** The `repo-worktree` skill already documents this (`git worktree add`, then attach with `EnterWorktree` `path:`), and it is worth the one extra approval: Claude Code tracks a session as "isolated in a worktree" only once `EnterWorktree` (or the `--worktree` launch flag) has actually run, and while a session is tracked that way it gets a further, built-in enforcement layer with no configuration at all -- blocking a file edit that targets the main checkout, a Bash/PowerShell/Monitor command whose working directory resolves to (or can't be verified to stay outside) the main checkout, a git redirect into the main checkout (`git -C`, `--git-dir`, `GIT_DIR`/`GIT_WORK_TREE`, or a `cd` into the main checkout before running git), and any command shape it can't verify stays inside the worktree at all. A `cd` alone, with no `EnterWorktree` call, gets none of this: the session's own bookkeeping never marked it as isolated, so these checks never engage, and `host-setup/agent-safety/claude/gh-write-guard.py`'s own rule 6 is what remains -- covering the same `-C`/`--git-dir`/`GIT_DIR`/`GIT_WORK_TREE`/`--work-tree` redirect shapes and a `sh -c`/`bash -c` wrapper, but, unlike Claude Code's own layer, deliberately not a `checkout`/`switch` of an actual ref carrying no force flag, or a fast-forward-only `pull` (the documented base-clone cleanup step needs exactly those, though a `checkout` of something that is not a ref, such as `git checkout .`, is still denied), and silently inert on a git old enough to lack `rev-parse --path-format`. Its own `claude/README.md` "Scope and Limits" states these precisely. ### opencode Worktree Access @@ -362,6 +356,9 @@ A host that fails any row is not ready for the procedure that row names, and the [agent-safety]: ../host-setup/agent-safety/README.md +[agent-safety-claude]: ../host-setup/agent-safety/claude/README.md +[agent-safety-codex]: ../host-setup/agent-safety/codex/README.md +[agent-safety-opencode]: ../host-setup/agent-safety/opencode/README.md [audit]: ../AUDIT.md [bootstrap]: ../host-setup/bootstrap.sh [bootstrap-ps1]: ../host-setup/bootstrap.ps1 @@ -379,7 +376,6 @@ A host that fails any row is not ready for the procedure that row names, and the [spec-dir]: ../spec/ [ssh-signing]: ./ssh-signing.md [standup]: ../STANDUP.md -[write-guard]: ../host-setup/agent-safety/gh-write-guard.py diff --git a/host-setup/README.md b/host-setup/README.md index 674474cf..2d318aa2 100644 --- a/host-setup/README.md +++ b/host-setup/README.md @@ -100,7 +100,7 @@ Reaching `spec/audit.py` and `scripts/carry.py` from outside a hub checkout mean ## Three Rules This Directory Follows -**Group by whichever axis has one member.** `agent-safety/` is one concern across three platforms, so it is a concern directory holding `install.sh`, `install.ps1` and `install.py`. `linux/` is three concerns on one platform, so it is a platform directory. Windows host tooling therefore sits at `windows/` rather than beside the Linux scripts, because the `winget` equivalent of `install-tools.sh` is a different program rather than a translation of one. It carries one registry record per tool where the Linux script carries four functions, since every Windows source is `winget` and the per-tool variation those functions exist for does not arise. `windows/` also carries a fourth script with no Linux peer, because WSL is a Windows-side concern. The loader and the menu are the same shape as `agent-safety/`, not as `linux/`/`windows/`: one concern, two platforms, so `bootstrap.ps1` sits beside `bootstrap.sh`, and `menu.ps1` beside `menu.sh`, at the top level rather than inside `windows/`. +**Group by whichever axis has one member.** `linux/` is three concerns on one platform, so it is a platform directory. Windows host tooling therefore sits at `windows/` rather than beside the Linux scripts, because the `winget` equivalent of `install-tools.sh` is a different program rather than a translation of one. It carries one registry record per tool where the Linux script carries four functions, since every Windows source is `winget` and the per-tool variation those functions exist for does not arise. `windows/` also carries a fourth script with no Linux peer, because WSL is a Windows-side concern. The loader and the menu are one concern, two platforms, so `bootstrap.ps1` sits beside `bootstrap.sh`, and `menu.ps1` beside `menu.sh`, at the top level rather than inside `windows/`. `agent-safety/` follows the same platform-grouping shape one level down: it holds one concern (write-safety guards) implemented per *agent* rather than per OS, so `claude/`, `codex/`, and `opencode/` sit beside each other the way `linux/` and `windows/` do, each holding that one agent's own files (a Claude Code implementation carries its own `install.sh`/`install.ps1` pair inside `claude/`, the same platform variation `linux/`/`windows/` carry at the top level, just nested one directory deeper because the outer axis here is the agent, not the OS). Its root `README.md` is the agent-agnostic spec those per-agent directories each implement, not a concern directory of its own. **Nothing here needs Python to stand a host up, and neither loader needs an interpreter to fetch what it drives.** [`docs/host-setup.md`][host-setup] carries that as part of the contract, with the reasoning. `bootstrap.sh` needs only `curl` and `tar`. `bootstrap.ps1` needs only `tar.exe`, which has shipped with Windows since 1803, and installs its one further dependency, `pwsh`, itself through `winget`. The one exception is the `install-skills` pair, which drives the Python installer at `scripts/skills_install.py` and runs last in a stand-up for exactly that reason: `install-tools` has provided the interpreter by then, and run alone on a host without one it stops and names the tools step as its prerequisite. The menu needs `git` to fetch the hub, and it checks for a working Python the same lazy way: only the tasks that call a Python tool ask for it, and every host task still works without one. Neither loader runs a gate as a closing step: [`scripts/host_gate.py`][host-gate] measures a host against the floors and is not called from here, and nothing here is called from it. A host set up by hand years ago is an ordinary host, so the gate reports what it is missing and running this tooling is a remedy a person chooses. The two are joined at code time instead, by [`scripts/tests/test_bootstrap.py`][test-bootstrap] asserting that every tool the spec requires is one this tooling can provide. diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index a7da831b..d8225bee 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -1,105 +1,251 @@ -# Claude Code Write-Safety Kit - -These per-machine, user-account-scoped Claude Code guards cover two action classes. The first is a mis-targeted GitHub **write** under the maintainer's identity. The second is a **git operation that bypasses a branch rule or required check**. That class includes a forbidden push, force-push, or delete. It also includes an override flag such as `--admin` or `--no-verify`. Deploy the kit first on any system where Claude Code uses the maintainer's `gh` credentials. Linux, WSL, macOS, and Windows are supported. This kit does not configure Codex or opencode. Their host-specific controls live in [`docs/host-setup.md` "Agent Write-Safety"][host-setup-write-safety], and [issue #781][issue-781] tracks their missing hooks. - -## What It Installs - -Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): - -- **`hooks/gh-write-guard.py`**: a PreToolUse hook that denies two classes of dangerous action. First, the GitHub **write** footguns behind the cross-repo comment incident: a state-changing `gh` call whose output is discarded, a GraphQL mutation passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is under an owner other than the checkout origin's. Sibling repositories under the same owner are allowed, since the harm this guards is reaching a stranger's repository rather than working across one maintainer's own fleet, and a different owner is allowed only when the maintainer names it in `GH_WRITE_GUARD_ALLOW` (an `owner/repo` list, where `owner/*` grants a whole owner). That variable is read from the environment the session was launched with, which is the one channel an agent cannot set for itself: a hook runs as its own process, so an inline `VAR=x cmd` prefix or an `export` inside a Bash call never reaches it. Second, a **git operation that bypasses a repository protection**. The branch-rule cases (a direct push to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked) are judged against the branch's **live** rules, so a code-style `develop` is denied while a config-style `develop` is allowed with no per-repo configuration. A push to a protected-default branch fails closed when its rules cannot be determined, either because the API is unreachable or because the checkout's origin cannot be resolved to query them. The explicit-bypass flags are denied **unconditionally**, since the flag is itself the bypass and needs no branch query: `gh pr merge --admin` overrides the server-side merge gate, and `git commit`/`git push --no-verify` skips the local git hooks. Reads and everything else pass through. It fires even in autonomous / bypass-permissions sessions, which is how the incident happened. -- **Two marker-delimited blocks of host-wide content in `CLAUDE.md`**, loaded into every session on the machine (including ad-hoc work outside any project), installed and updated independently so one can change without rewriting the other. - - The `agent-safety` block carries restrictions alone, so nothing in it can widen a permission. `Repository Boundaries and Write Safety` mirrors the committed `GOVERNANCE.md` section of the same name, carrying as behavioral guidance its write rules and its rules that a task works in its own checkout and isolates into its own worktree before its first file edit. `Data in Agent-Authored Text` mirrors `GOVERNANCE.md` "Representative Data in Agent-Authored Text", so an agent illustrates with constructed data rather than data observed on the machine. `Authorization Scope and Memory Hygiene` bounds how a granted permission is recorded and read. Each mirrors a rule that otherwise reaches fleet repos only. - - The `fleet-bootstrap` block carries `Fleet Governance Entry Point`, which names the template repository and routes by the state a repository is actually in. It is separate precisely because it enables rather than restricts, and the safety block's own text says nothing in it widens a permission, so merging the two would contradict that. It is host-wide rather than per repository because the repositories that most need it are the ones carrying no instruction set to point the way, and it mirrors the byte-locked `AGENTS.md` "Fleet Bootstrap" section that a conformant repository carries. -- **The permission rules this kit owns, merged into `settings.json`** beside the hook registration. Each is declared as a prefix and a rule, and a re-run drops every rule the prefix owns before adding the current one, so a rule whose spelling changes updates in place rather than accumulating beside the version it replaced. Ownership requires a rule-syntax delimiter after the prefix, since the prefix ends at the script name and a bare prefix test would also claim a longer path such as `pr_review.py-custom`, so a rule written by hand for a different script is never touched. These widen rather than restrict, which is why they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one. Today the list holds one rule, for `scripts/pr_review.py`, the review loop's reply and resolve. Driving that loop by hand needs a raw GraphQL mutation carrying a node id, which is the shape that reached a stranger's repository, where the script queries the id itself and takes no argument an id fits in. What the rule decides is which command runs without a prompt, and it matches the command text rather than the directory the command runs in, so it reaches a `scripts/pr_review.py` in any checkout that carries one. An absolute path would not narrow that, since the hub is reached as a checkout of the caller's own and its location differs per task, so pinning one path would name a checkout the next task does not use. What bounds it is the rule that an agent reaches the hub as a checkout of its own, fetched immediately before it is read, rather than a copy it happens to find on disk, which the `fleet-bootstrap` block beside this carries and [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states in full. - -The hook is the mechanical backstop. The CLAUDE.md rules and the carried GOVERNANCE.md rules are the behavioral layer. Prose alone is not enough, since the incident happened under prose rules, so both ship. The GitHub write rules have a hook behind them, as do the git operations that bypass a repository protection, while which checkout a command belongs in, the data a line of text quotes, and how an authorization is recorded are prose only, since no hook can see any of the three. - -## Install (Idempotent, Safe to Re-Run to Update) - -```sh -# Linux / WSL / macOS -host-setup/agent-safety/install.sh -``` - -```powershell -# Windows - the .\ prefix is required, PowerShell does not run a script from a relative path without it -.\host-setup\agent-safety\install.ps1 +# Agent Write-Safety Spec + +What any coding agent must be stopped from doing when it runs on this host with the maintainer's +`gh` credentials, stated once, independent of which agent implements it. This file is the source +of truth: an implementation is built from the requirements below, and an implementation is audited +by checking its decisions against them, not by reading its source as the implicit spec. + +## Why This Exists + +A mis-targeted GitHub write acts publicly under the maintainer's identity: a fabricated node id +once posted a stray comment, as the maintainer, to a stranger's repository. A mutating git command +run directly in a primary checkout destroys another task's uncommitted work without ever reaching +GitHub. Both incidents happened under prose rules the agent had already read. Neither was fixed by +writing the rule more clearly. [`GOVERNANCE.md`][governance] "Durable Knowledge and Self-Improvement" +states the general criteria for when a rule like this earns a mechanical hook instead of staying +prose. The requirements below are the write-safety instance of that criteria, applied. + +## Requirements + +Each requirement is stated as a decision rule, precise enough to implement against any agent's own +hook or approval-gate API, not tied to Claude Code's `PreToolUse` JSON shape. + +1. **A GitHub write with its output discarded or forced to success is denied.** A state-changing + `gh`/API call piped to `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, or `|| echo` + hides the one signal that tells a client-reported failure apart from a server-side success. Deny + the write, then allow it once run so its real result is read. +2. **A GraphQL mutation carrying a literal GitHub node id is denied.** Node ids resolve globally, + so a fabricated, stale, or hand-typed id can land on a real object in a different repository. A + literal id, such as one prefixed `PR_`, `PRRT_`, `IC_`, or `BOT_` (an uppercase-letter prefix + followed by an underscore and a long body, or the legacy `MD`-prefixed base64 form), is denied. A + `-F name="$VAR"` value in the same position is allowed instead of being pattern-matched, **not** + because the hook has verified where `$VAR`'s value came from -- a static, pre-execution hook + cannot see a shell variable's runtime binding, only the command text -- but because this rule's + job is to catch the literal-id mistake specifically, and a captured-variable convention is what + the fleet's own prose rule (`GOVERNANCE.md` "Repository Boundaries and Write Safety") requires + agent behavior to uphold. Enforcing that the value genuinely came from a live query is + behavioral, not something this decidable-from-text-alone rule can check. +3. **A GitHub write with an explicit target outside the checkout's own owner is denied, unless the + maintainer granted it.** Compare the write's explicit `-R`/`--repo`/`repos//` target + against the checkout's own `origin` owner, when an `origin` resolves at all. A sibling repository + under the same owner is allowed with no grant, since the harm this guards is reaching a + stranger's repository, not working across one maintainer's own fleet. A different owner is + allowed only when named in a grant read from the environment the session was launched with -- + never a channel the agent itself can set (an inline `VAR=x cmd` prefix or an `export` inside the + same call must not satisfy this). **When no `origin` resolves at all** (a non-git directory, or a + checkout whose remote can't be read), this requirement has nothing to compare the target against + and does not fire -- requirements 1 and 2 still apply regardless, and this is the same + precision-over-recall stance every requirement but 4 takes. +4. **A git operation that would only succeed by bypassing an active branch rule is denied**: a + direct push to a branch whose rules require a pull request, a force-push where history is + protected, a delete where deletion is blocked, or an explicit-bypass flag (`--admin` on a merge, + `--no-verify` on a commit/push). Judge branch-rule cases against that branch's *live* rules, so a + code-style `develop` denies and a config-style `develop` allows with no per-repo configuration. + **This one fails closed, but only for a branch protected by default** (`main`, `master`, + `develop`): when that branch's rules cannot be determined at all (network unreachable, origin + unresolvable), deny rather than allow, because the harm is a silent success under the + maintainer's own admin bypass. A push to any other branch whose rules cannot be determined + passes this requirement instead, since there is nothing yet on record to bypass. Every other + requirement here favors precision over recall throughout, denying only a positively-identified + dangerous shape, since a hook that fails closed on an unrelated resolution failure blocks + legitimate work far more often than it catches a real bypass. +5. **A hand-rolled reply or resolve on a review thread, bypassing the one-call helper, is denied + (where a helper exists) unless the maintainer's cross-owner grant already covers it.** Splitting + a reply and a resolve into two separate hand-run API calls is what let a reply sit unresolved + across a push, reading as untriaged. Where the agent's fleet ships a single documented helper for + this (this repo's `scripts/pr_review.py reply --resolve`), a raw mutation reaching the same + endpoint is denied in favor of it. The one exception is a target the maintainer has already + granted this session: the helper itself refuses a cross-owner pull request outright, so the + hand-run form is then the documented fallback for that specific repository, and this is allowed + through the same grant channel requirement 3 reads rather than a separate one. A REST reply's own + URL can be checked against the grant. A `resolveReviewThread` mutation's thread id is opaque, so + any active grant is the only signal available there, a coarser check than a REST reply gets and a + residual gap this requirement accepts rather than blocking every grant-holding session's replies + on an unrelated target. + +6. **A mutating git operation run directly against a primary checkout is denied.** "Primary" means + not a linked worktree. The decidable test is a comparison, not a filesystem-shape guess: `git + rev-parse --path-format=absolute --git-dir --git-common-dir` returns equal paths for a primary + checkout and unequal paths for a linked worktree. A `.git`-is-a-directory heuristic is wrong (a + submodule's `.git` is a file yet is still a primary working tree that can lose uncommitted + work). Deny `checkout`/`switch`/`pull`/`reset`/`rebase`/`merge`/`cherry-pick`/`revert`/`restore`/ + `stash` (anything but `list`/`show`)/`clean -f|-fd`/`add`/`commit`/`rm`/`mv`/`apply`/`am`/`push`/ + `worktree remove -f|--force` there. `push` is denied unconditionally too, even though it does + not mutate the local working tree or HEAD the way the rest of this list does: no documented + fleet workflow ever pushes from a primary checkout, every push runs from a task's own worktree, + and rule 4's own branch-rule checks already run before this rule and can deny a push on their + own separate grounds regardless. `clean` is exempt when `-n`/`--dry-run` is given before any + `--` (bundled or not, e.g. `-nfd`, since a `-n` after `--` is an unconditional pathspec instead + naming a real file, confirmed live), and confirmed live to always win over `-f`/`--force` + regardless of order or repetition: it deletes nothing, only previews what a later, real forced + clean would remove, so denying it adds no safety. A `checkout`/`switch` + force flag (`-b`/`-B` for checkout, + `-c`/`-C`/`--create`/`--force-create` for switch, `-f`/`--force`/`--discard-changes`/`--orphan` + for either -- switch has no `-b`/`-B` and checkout has no `-c`/`-C`, confirmed against each + subcommand's own `-h` output, so neither letter pair collides with an unrelated flag on the + other) is recognized bundled into a short-option cluster or attached to its own value with no + space (`-qf`, `-Bname`, `-Cother`), not only as an exact argv token -- an exact-token check + alone lets `-qf`/`-Bname` reach the ref-switch exemption below while still forcing the checkout + through, and would equally have let `switch -C ` through, confirmed live to + reset that branch to the current HEAD with no dirty-tree warning at all, since it is not a + working-tree overwrite. A `worktree remove`'s own `-f` is bundled the same way, since `remove` + has no other short option `-f` could combine with: git requires `-f` given twice to remove a + locked worktree, and the bundled `-ff` spelling satisfies that exactly as `-f -f` does, + confirmed live to forcibly remove a locked worktree's uncommitted content. Allow + `worktree add|list|prune`, a plain `worktree remove` with no force flag, any read, `merge + --ff-only`/`pull --ff-only` (git's + own semantics mean neither can discard anything), a bare `-` as a `checkout`/`switch` argument + (porcelain shorthand for the previous branch, which only those two subcommands themselves + understand, so it is exempt outright rather than checked), and a `checkout `/`switch ` + carrying no force flag whose argument verifiably resolves as a ref -- checked live (`git rev-parse + --verify --quiet ^{commit}`), since git's own ref-switch path refuses to overwrite a local + modification but its pathspec-restore fallback for an argument that does not resolve as a ref + (`git checkout .`, `checkout -- `, `checkout -- `, more than one bare + positional) carries no such check and is denied. A non-force flag alongside the ref, such as + `--detach`/`-q`, stays exempt too -- verified live, it changes nothing about git's own + overwrite-refusal, so this is a real-ref-with-no-force-flag test, not a strictly zero-flags one, + despite reading as "flagless" at a glance. These exemptions are the normal, documented way an + agent uses a primary checkout as a fetch source and returns it to a base branch afterward, and + denying them adds no safety while breaking routine, correct work. The ref-checkout exemption is a + deliberate, validated scope boundary worth naming explicitly: the incident behind this requirement + (#1073) ran exactly this shape (an unforced `checkout` then an `--ff-only` pull), so this + requirement does not deny that incident's own literal commands. The concurrent-access hazard those + commands still carried either way -- switching HEAD or fast-forwarding a checkout another task + might be relying on, whether or not the working tree was dirty -- is not decidable from the + command text alone, so it stays the prose rule's job (`GOVERNANCE.md` "Repository Boundaries and + Write Safety", `repo-worktree`), not this one's. + + A subcommand name this requirement does not otherwise recognize is resolved through a chain of + git aliases before being allowed to fall through -- an inline `-c alias.=` override + on the same invocation first, then the target checkout's own persisted config (`git config --get + alias.`), matching real git's own override order, up to a bounded number of hops -- so a + custom alias that expands to a denied builtin (`git -c alias.wipe='reset --hard' wipe`, or the + same `wipe` alias persisted in the checkout's own config) is denied exactly as the builtin itself + would be. A `!`-prefixed alias hands git an arbitrary shell string rather than naming another git + subcommand, and this requirement does not and cannot safely interpret one, so it denies that + shape outright against a primary checkout, the one place this requirement departs from its own + fail-open stance, because the alias definition itself is concrete evidence of an attempt to run + something via git in exactly the directory this requirement protects. + + Resolve the target directory the way real git itself does, not by a last-option-wins scan across + every directory-naming option: any `-C ` options on the invocation compose sequentially (an + absolute value replaces the running directory outright, a relative one joins onto the previous + result) onto a leading `cd &&`/`cd ;` prefix on the same command -- read inside a + `sh -c`/`bash -c` wrapper too, and inherited from an outer leading `cd` when a wrapped string + carries none of its own -- or, absent one, the invocation's own working directory. An explicit + `--work-tree`/`GIT_WORK_TREE=` value, when given anywhere on the invocation, then wins over that + `-C`-chain result regardless of how many `-C` options preceded it, matching how `--work-tree` + names the actual mutation target independent of where `-C` points, and a relative `--work-tree` + value still resolves against the `-C` chain's own result. A leading `export FOO=x BAR=y &&` + prefix (`GIT_WORK_TREE`/`GIT_DIR` in place of `FOO`/`BAR`, a bare `;` in place of `&&` too) + redirects the invocation the same way an inline `VAR=x git ...` prefix already does, since a + real shell export persists into the following command exactly as effectively, confirmed live + with a real reset that discards a tracked local modification with no redirect at all on the `git` + invocation itself, a shape an inline-prefix scan alone cannot see. `--git-dir`/`GIT_DIR=` alone, + with no `--work-tree`/`GIT_WORK_TREE=` anywhere on the same invocation, never relocates that reported + target, matching git's own documented fallback. + + Whether the invocation targets a primary checkout at all is a separate question from that + reported target, though. An explicit `--git-dir`/`GIT_DIR=` is resolved and tested for + primary-checkout-ness directly (`git --git-dir= rev-parse ...`, no `-C`), independent of + `--work-tree`, since `--git-dir` names the repository actually mutated regardless of where + `--work-tree`/cwd point -- confirmed live: `git --git-dir=/.git --work-tree= + commit` mutates `` even though `` resolves as no git repository at all, which + testing the resolved `--work-tree` value alone fails open on. Absent an explicit `--git-dir`, the + test falls back to ordinary ancestor-based discovery from the reported target, exactly as real git + itself does. `~`/`$HOME` is expanded throughout (a bare `$HOME` only when not immediately followed + by another identifier character, so `$HOMEPATH`/`$HOMEDRIVE` are left alone rather than misread as + a `$HOME` prefix), and a relative value is joined against the running result rather than wherever + the hook process's own OS-level cwd happens to be. Fail open (allow) when no git repository + resolves at all, matching this requirement's own + precision-over-recall stance, not requirement 4's fail-closed one -- the harm here needs a + positively-identified primary checkout to fire on. Granted only by + `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT`, read from the same session-start-environment channel + `GH_WRITE_GUARD_ALLOW` is, though interpreted differently: `GH_WRITE_GUARD_ALLOW` is an + `owner/repo` allowlist, while this one is a boolean escape hatch, granted by any non-falsy value + and withheld by a recognized falsy one ("0"/"false"/"no"/"off"/empty), not by list membership. + +## Decision Flow + +```mermaid +flowchart TD + cmd["Tool call: a shell/git/gh command"] --> isgit{"A git operation\nthat bypasses a\nbranch rule\nor a bypass flag?"} + isgit -- yes --> deny4["DENY - requirement 4\n(fails closed for a\nprotected-default branch\nwith undeterminable rules)"] + isgit -- no --> isprimary{"A mutating git op\ntargeting a primary\ncheckout, not exempt?"} + isprimary -- yes --> deny6["DENY - requirement 6"] + isprimary -- no --> isghwrite{"A GitHub-write\ncommand at all?"} + isghwrite -- no --> allow["ALLOW"] + isghwrite -- yes --> suppressed{"Output discarded or\nforced to success?"} + suppressed -- yes --> deny1["DENY - requirement 1"] + suppressed -- no --> literalid{"Mutation carries a\nliteral node id?"} + literalid -- yes --> deny2["DENY - requirement 2"] + literalid -- no --> crossowner{"Explicit target outside\nthe checkout's owner,\nnot granted?"} + crossowner -- yes --> deny3["DENY - requirement 3"] + crossowner -- no --> handrolled{"Hand-rolled reply/resolve,\nhelper available?"} + handrolled -- yes --> deny5["DENY - requirement 5"] + handrolled -- no --> allow ``` -Both are thin wrappers around `install.py`, so every OS runs one tested code path. The installer self-tests the hook before registering it, merges the settings.json hook entry and the permission rules without clobbering other keys, and updates each CLAUDE.md block in place by its own markers rather than duplicating it, so the two blocks move independently. The settings file is read once and written once, so the hook and the permission rules land together or not at all. - -**Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load. - -## Refreshing After an Upstream Change - -The deployed copy on each machine is a snapshot, so when the guard changes upstream (a new rule or a fix) every machine keeps running the old hook until it is refreshed. The installer **is** the refresh: pull the latest template and re-run `install.sh` (or `install.ps1`) on each machine. It re-copies the hook, re-runs the self-test, and re-registers in place, so a re-run is safe and updates the deployed copy. [#365][issue-365] tracks the per-machine rollout and its re-runs. - -## Verify (POSIX Shell) - -```sh -python3 ~/.claude/hooks/gh-write-guard.py --selftest # decision matrix: all cases pass -grep -c 'agent-safety v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) -grep -c 'fleet-bootstrap v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) -grep -cF 'Bash(python3 scripts/pr_review.py:*)' ~/.claude/settings.json # expect 1 (never duplicated) +```mermaid +flowchart LR + subgraph layer1["Loading layer"] + L1["Does the governing rule\neven reach the session's\ncontext? (e.g. CLAUDE.md\nimporting AGENTS.md)"] + end + subgraph layer2["Prose layer"] + L2["GOVERNANCE.md / AGENTS.md /\nSkill trigger descriptions --\nread and followed by judgment"] + end + subgraph layer3["Hook layer"] + L3["This spec's requirements --\nmechanically decided from the\ntool call alone, no judgment"] + end + L1 -->|"fixed by a structural\nload/import fix, not a hook"| L2 + L2 -->|"promoted here only when\nmechanically decidable +\ndestructive, per GOVERNANCE.md\n'Durable Knowledge'"| L3 ``` -On Windows PowerShell: - -```powershell -py -3 "$env:USERPROFILE\.claude\hooks\gh-write-guard.py" --selftest # all cases pass -(Select-String 'agent-safety v' "$env:USERPROFILE\.claude\CLAUDE.md").Count # expect 2 -(Select-String 'fleet-bootstrap v' "$env:USERPROFILE\.claude\CLAUDE.md").Count # expect 2 -(Select-String -SimpleMatch 'Bash(python3 scripts/pr_review.py:*)' "$env:USERPROFILE\.claude\settings.json").Count # expect 1 -``` - -Live end-to-end (in any repo): attempt a discarded-output write and confirm the Bash tool is blocked: - -```sh -gh api graphql -f query='mutation{noop}' -F t="PRRT_x" >/dev/null 2>&1 || true # blocked by the hook -``` - -## Granting a Cross-Owner Write - -The cross-origin rule is the one denial a maintainer has to act on, because it is the only one with a grant behind it. The others name a shape to stop using, while this one names a target that may be entirely legitimate. - -`GH_WRITE_GUARD_ALLOW` is one string holding every grant, since it is an environment variable, and it is split into `owner/repo` tokens on any run of whitespace or commas, so `a/b c/d` and `a/b, c/d` are the same two grants. `owner/*` grants a whole owner. A token carrying no `/` is ignored, so a malformed grant grants nothing, and a repository grant does not extend to that owner's other repositories. - -The hook reads it from the environment the session was launched with, which is the one channel an agent cannot use on itself: the hook runs as its own process, so an inline `VAR=x cmd` prefix and an `export` inside a Bash call both leave the write denied. Granting is therefore a deliberate act taken outside the session, and a blocked agent asks rather than unblocks itself. - -The channel that works is an `env` block in the checkout's `.claude/settings.local.json`, which scopes the grant to sessions started in that checkout, followed by a session restart. The worked example, the fork case that raises this most often, and how to confirm a grant loaded without making the write are in [`docs/host-setup.md` "Granting a Write the Guard Denies"][host-setup-grant]. - -## Manual settings.json Shape (for Reference) - -The installer writes this. It is here so you can inspect or hand-place it: - -```json -{ - "hooks": { - "PreToolUse": [ - { "matcher": "Bash", "hooks": [ { "type": "command", "command": "\"python3\" \"/.claude/hooks/gh-write-guard.py\"" } ] } - ] - }, - "permissions": { - "allow": [ "Bash(python3 scripts/pr_review.py:*)" ] - } -} -``` - -Every other key in the file is left as it stands, `permissions.allow` included, apart from the rules whose prefix this kit owns. - -## Scope and Limits - -- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365][issue-365] tracks. -- **Precision over recall for the write footguns.** The hook denies the specific dangerous write shapes with high confidence rather than gating every write, so it never blocks legitimate work. A shape it does not catch still falls under the behavioral rules. -- **The branch-bypass rule fails closed.** Unlike the write-footgun rules, a push to `main`/`master`/`develop` is denied even when its rules cannot be determined (the API is unreachable, or the checkout's origin cannot be resolved to query them), because the harm there is a silent success under the maintainer's admin bypass. The rule reads each branch's live rules, so it adapts to every repo (a code-style `develop` denies, a config-style `develop` allows) with no per-repo configuration, and hands the exact command to the maintainer to run when a bypass is genuinely intended. -- **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id, which is exactly why rule 2 blocks a *literal* id at all, since a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit `-R`/`repos//` target. A write from a non-git directory, or one whose target is only a node id, is evaluated by rules 1 and 2 alone. -- **A write inside a script file is unseen, so never batch writes into a script.** The hook reads the command the agent runs, which for `bash deploy.sh` is that one string, so a `git push` or a `gh` mutation inside the script reaches the server without the hook ever parsing it. This is the widest hole in the kit and it is one an agent opens by accident, since collecting fifteen repetitive pushes into a loop looks like tidiness rather than like disabling a guard. Issue each write as its own command. A script that only reads, computes, or prepares local commits is fine, because the boundary is the write and not the script. -- **The hook's own parser over-blocks a `git push` followed by a newline.** Git and GitHub are not involved in this one: the hook splits the command string to find each `git push` and its arguments, and that split ends an argument list at `&&` but not at a newline, so it reads every token on a later line of the same command as a refspec of that push. Measured against the installed hook, `git push -u origin revendor/x` resolves to that one branch, while the same push followed by a newline and a `gh pr create` naming `develop` as its base resolves to five, meaning `revendor/x`, `gh`, `pr`, `create`, and `develop`. The hook then denies the push as a direct push to a protected branch that the push never named. The direction is safe, since it blocks rather than admits, but the denial names a bypass the agent never attempted, and a guard that cries wolf is one an agent starts working around. Until the parser is fixed, issue the push as its own command, which is the rule directly above in any case. Tracked in `TODO.md`. -- **Not a credential control.** A fine-grained PAT limited to owned repositories is a separate, stronger structural guard (a hard `403` on any non-owned repo) and is left to per-machine credential setup, out of this kit. +The first diagram is this spec's actual decision flow, generalized from `claude/gh-write-guard.py`'s +`classify()`. The second is why a failure lands in one layer and not another. A rule that never +reached the session at all is a loading bug, fixed the way PR #1081 fixed `local-strict-review`'s +missed trigger, by wiring `CLAUDE.md` to import `AGENTS.md`. A rule that reached the session and +was still not followed, where the trigger is mechanically decidable and the harm is destructive, +is promoted to a hook (requirement 6, above, tracked at [issue #1073][issue-1073], is the worked +example). A rule whose violation can only be judged, not mechanically decided (was a +review finding actually evidence-backed?), stays prose and a chained Skill trigger, since a hook +there could only nag, never decide. + +## Per-Agent Status + +| Agent | Status | Implementation | +| --- | --- | --- | +| Claude Code | All 6 requirements, via a `PreToolUse` hook | [`claude/README.md`][claude] | +| Codex | No hook yet -- tracked at [issue #781][issue-781] | [`codex/README.md`][codex] | +| opencode | No hook yet -- tracked at [issue #781][issue-781] | [`opencode/README.md`][opencode] | + +GitHub Copilot carries no subdirectory here: it reviews through GitHub's own hosted infrastructure +rather than running local shell commands under the maintainer's credentials, so it has no analogous +local write-safety hazard for this kit to cover. + +## Auditing an Implementation Against This Spec + +Run the implementation's own self-test (`claude/gh-write-guard.py --selftest` for Claude Code) and +compare every case against the requirements list above, one by one, rather than reading the +implementation's source as though it were the spec. A case the self-test doesn't cover is a gap in +the audit, not evidence the requirement is satisfied. This is the concrete shape of "ask Claude to +audit the Claude hooks against the spec" or "ask Codex to implement Codex's own hooks against the +spec": point the agent at this file's requirements, not at another agent's source code. +[claude]: ./claude/README.md +[codex]: ./codex/README.md +[opencode]: ./opencode/README.md [governance]: ../../GOVERNANCE.md -[host-setup-grant]: ../../docs/host-setup.md#granting-a-write-the-guard-denies -[host-setup-write-safety]: ../../docs/host-setup.md#agent-write-safety -[issue-365]: https://github.com/ptr727/ProjectTemplate/issues/365 [issue-781]: https://github.com/ptr727/ProjectTemplate/issues/781 +[issue-1073]: https://github.com/ptr727/ProjectTemplate/issues/1073 diff --git a/host-setup/agent-safety/.markdownlint-cli2.jsonc b/host-setup/agent-safety/claude/.markdownlint-cli2.jsonc similarity index 100% rename from host-setup/agent-safety/.markdownlint-cli2.jsonc rename to host-setup/agent-safety/claude/.markdownlint-cli2.jsonc diff --git a/host-setup/agent-safety/claude/README.md b/host-setup/agent-safety/claude/README.md new file mode 100644 index 00000000..107ce97b --- /dev/null +++ b/host-setup/agent-safety/claude/README.md @@ -0,0 +1,147 @@ +# Claude Code Write-Safety Kit + +This is the Claude Code implementation of the write-safety spec at [`../README.md`][spec] -- +per-machine, user-account-scoped guards, deployed first on any system where Claude Code uses the +maintainer's `gh` credentials. Linux, WSL, macOS, and Windows are supported. See the spec for the +requirements this kit satisfies and why each exists. What follows here is Claude-Code-specific +installation and operational detail. Codex and opencode have no equivalent hook yet -- see +[`../codex/README.md`][codex] and [`../opencode/README.md`][opencode] for their status, tracked at +[issue #781][issue-781]. + +## What It Installs + +Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): + +- **`hooks/gh-write-guard.py`**: a PreToolUse hook that denies three classes of dangerous action. First, the GitHub **write** footguns behind the cross-repo comment incident: a state-changing `gh` call whose output is discarded, a GraphQL mutation passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is under an owner other than the checkout origin's. Sibling repositories under the same owner are allowed, since the harm this guards is reaching a stranger's repository rather than working across one maintainer's own fleet, and a different owner is allowed only when the maintainer names it in `GH_WRITE_GUARD_ALLOW` (an `owner/repo` list, where `owner/*` grants a whole owner). That variable is read from the environment the session was launched with, which is the one channel an agent cannot set for itself: a hook runs as its own process, so an inline `VAR=x cmd` prefix or an `export` inside a Bash call never reaches it. Second, a **git operation that bypasses a repository protection**. The branch-rule cases (a direct push to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked) are judged against the branch's **live** rules, so a code-style `develop` is denied while a config-style `develop` is allowed with no per-repo configuration. A push to a protected-default branch fails closed when its rules cannot be determined, either because the API is unreachable or because the checkout's origin cannot be resolved to query them. The explicit-bypass flags are denied **unconditionally**, since the flag is itself the bypass and needs no branch query: `gh pr merge --admin` overrides the server-side merge gate, and `git commit`/`git push --no-verify` skips the local git hooks. Third, a **mutating git operation run directly against a primary checkout** (`checkout`/`switch`/`pull`/`reset`/`rebase`/`merge`/`cherry-pick`/`revert`/`restore`/`add`/`commit`/`rm`/`mv`/`apply`/`am`/`push`/`stash` (anything but `list`/`show`)/`clean -f`/`worktree remove -f`), the harm behind a second incident where an agent reused the maintainer's own primary checkout instead of a worktree despite having read the prose rule against it. "Primary" is decided by comparing `git rev-parse --git-dir` against `--git-common-dir`, not a `.git`-is-a-directory guess. `worktree add`/`list`/`prune`, an unforced `worktree remove`, `merge --ff-only`/`pull --ff-only`, and a `checkout `/`switch ` carrying no force flag (bundled or attached forms like `-qf`/`-Bname` recognized, not only an exact token) stay exempt even in a primary checkout, matching the documented base-clone cleanup step the `repo-worktree` skill already carries. A subcommand this rule does not recognize is resolved through a chain of git aliases, inline and persisted, before falling through, so a custom alias cannot hide a denied builtin behind an unrecognized name. A `!`-prefixed shell alias is denied outright rather than interpreted. The target directory follows real git's own `-C`/`--work-tree`/`--git-dir` priority, not a last-option-wins scan. A different owner-style grant, `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT`, follows the same channel as `GH_WRITE_GUARD_ALLOW`. Reads and everything else pass through. It fires even in autonomous / bypass-permissions sessions, which is how the first incident happened. See the root spec's requirement 6 for the exact rules. +- **Two marker-delimited blocks of host-wide content in `CLAUDE.md`**, loaded into every session on the machine (including ad-hoc work outside any project), installed and updated independently so one can change without rewriting the other. + - The `agent-safety` block carries restrictions alone, so nothing in it can widen a permission. `Repository Boundaries and Write Safety` mirrors the committed `GOVERNANCE.md` section of the same name, carrying as behavioral guidance its write rules and its rules that a task works in its own checkout and isolates into its own worktree before its first file edit. `Data in Agent-Authored Text` mirrors `GOVERNANCE.md` "Representative Data in Agent-Authored Text", so an agent illustrates with constructed data rather than data observed on the machine. `Authorization Scope and Memory Hygiene` bounds how a granted permission is recorded and read. Each mirrors a rule that otherwise reaches fleet repos only. + - The `fleet-bootstrap` block carries `Fleet Governance Entry Point`, which names the template repository and routes by the state a repository is actually in. It is separate precisely because it enables rather than restricts, and the safety block's own text says nothing in it widens a permission, so merging the two would contradict that. It is host-wide rather than per repository because the repositories that most need it are the ones carrying no instruction set to point the way, and it mirrors the byte-locked `AGENTS.md` "Fleet Bootstrap" section that a conformant repository carries. +- **The permission rules this kit owns, merged into `settings.json`** beside the hook registration. Each is declared as a prefix and a rule, and a re-run drops every rule the prefix owns before adding the current one, so a rule whose spelling changes updates in place rather than accumulating beside the version it replaced. Ownership requires a rule-syntax delimiter after the prefix, since the prefix ends at the script name and a bare prefix test would also claim a longer path such as `pr_review.py-custom`, so a rule written by hand for a different script is never touched. These widen rather than restrict, which is why they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one. Today the list holds one rule, for `scripts/pr_review.py`, the review loop's reply and resolve. Driving that loop by hand needs a raw GraphQL mutation carrying a node id, which is the shape that reached a stranger's repository, where the script queries the id itself and takes no argument an id fits in. What the rule decides is which command runs without a prompt, and it matches the command text rather than the directory the command runs in, so it reaches a `scripts/pr_review.py` in any checkout that carries one. An absolute path would not narrow that, since the hub is reached as a checkout of the caller's own and its location differs per task, so pinning one path would name a checkout the next task does not use. What bounds it is the rule that an agent reaches the hub as a checkout of its own, fetched immediately before it is read, rather than a copy it happens to find on disk, which the `fleet-bootstrap` block beside this carries and [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states in full. + +See [`../README.md`][spec] "Requirements" for which of these each rule implements, and "Auditing an +Implementation Against This Spec" for how to check this hook still satisfies them after a change. + +## Install (Idempotent, Safe to Re-Run to Update) + +```sh +# Linux / WSL / macOS +host-setup/agent-safety/claude/install.sh +``` + +```powershell +# Windows - the .\ prefix is required, PowerShell does not run a script from a relative path without it +.\host-setup\agent-safety\claude\install.ps1 +``` + +Both are thin wrappers around `install.py`, so every OS runs one tested code path. The installer self-tests the hook before registering it, merges the settings.json hook entry and the permission rules without clobbering other keys, and updates each CLAUDE.md block in place by its own markers rather than duplicating it, so the two blocks move independently. The settings file is read once and written once, so the hook and the permission rules land together or not at all. + +**Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load. + +## Refreshing After an Upstream Change + +The deployed copy on each machine is a snapshot, so when the guard changes upstream (a new rule or a fix) every machine keeps running the old hook until it is refreshed. The installer **is** the refresh: pull the latest template and re-run `install.sh` (or `install.ps1`) on each machine. It re-copies the hook, re-runs the self-test, and re-registers in place, so a re-run is safe and updates the deployed copy. [#365][issue-365] tracks the per-machine rollout and its re-runs. + +## Verify (POSIX Shell) + +```sh +python3 ~/.claude/hooks/gh-write-guard.py --selftest # decision matrix: all cases pass +grep -c 'agent-safety v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) +grep -c 'fleet-bootstrap v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) +grep -cF 'Bash(python3 scripts/pr_review.py:*)' ~/.claude/settings.json # expect 1 (never duplicated) +``` + +On Windows PowerShell: + +```powershell +py -3 "$env:USERPROFILE\.claude\hooks\gh-write-guard.py" --selftest # all cases pass +(Select-String 'agent-safety v' "$env:USERPROFILE\.claude\CLAUDE.md").Count # expect 2 +(Select-String 'fleet-bootstrap v' "$env:USERPROFILE\.claude\CLAUDE.md").Count # expect 2 +(Select-String -SimpleMatch 'Bash(python3 scripts/pr_review.py:*)' "$env:USERPROFILE\.claude\settings.json").Count # expect 1 +``` + +Live end-to-end (in any repo): attempt a discarded-output write and confirm the Bash tool is blocked: + +```sh +gh api graphql -f query='mutation{noop}' -F t="PRRT_x" >/dev/null 2>&1 || true # blocked by the hook +``` + +## Granting a Write That the Guard Denies + +The guard denies a `gh` write whose explicit target sits under an owner other than the checkout's `origin` owner, and the denial names `GH_WRITE_GUARD_ALLOW` as the way past it. That grant is the maintainer's to make, and making it is a deliberate act taken outside the session rather than something an agent does for itself once blocked. This is the one denial a maintainer has to act on, because it is the only one with a grant behind it -- the others name a shape to stop using, while this one names a target that may be entirely legitimate. + +**The case that raises it is usually a fork.** `origin` is your own fork under your own owner, and `upstream` is the project it was forked from under someone else's. Everything aimed at the fork is in scope and never denies, and only the half that leaves the owner stops: filing an issue on the upstream, opening a pull request against it, or commenting on one there. The grant therefore names the upstream alone, and the fork needs no grant at all. That asymmetry is what a reader hits first, since half the session's writes succeed and the other half do not. + +**The grant goes in the checkout's `.claude/settings.local.json`, as an `env` block:** + +```json +{ + "env": { + "GH_WRITE_GUARD_ALLOW": "upstream-owner/upstream-repo second-owner/other-repo third-owner/*" + } +} +``` + +**The value is one string holding every grant, never a JSON array**, since the hook reads an environment variable and an environment variable is a string. The three tokens above are three separate grants: two naming one repository each, and `third-owner/*` granting every repository under that owner. + +Tokens are separated by **any run of whitespace or commas**, so `a/b c/d`, `a/b,c/d`, and `a/b, c/d` all parse to the same two grants and the choice is cosmetic. A token carrying no `/` is ignored, so a malformed entry grants nothing rather than granting everything, and it also fails silently, which is why the confirmation step below is worth running. Grant the narrowest thing that unblocks the work, since a repository grant does not extend to that owner's other repositories and that containment is the property worth keeping. + +**The grant is per checkout, not per host.** `.claude/settings.local.json` lives in the working tree and is git-ignored, so it applies to sessions started in that checkout and does not follow the agent into another repository's sessions. That is the intended scope: a grant made to file one upstream issue from one fork does not quietly become a standing permission everywhere. + +**Restart the session afterward.** The hook reads the value from the environment the session was launched with, which is what makes the channel one an agent cannot use on itself, and it is equally why a grant added to a live session does nothing until that session restarts. + +**Two forms look right and leave the write denied.** An inline `GH_WRITE_GUARD_ALLOW=owner/repo gh ...` prefix sets the environment of the `gh` process, and an `export` inside a shell call sets the environment of that shell. The hook runs as its own process and sees neither, so the write stays denied with nothing to explain the difference. [`gh-write-guard.py`][write-guard] asserts the inline-prefix case in its own self-test, so this is settled behavior rather than a quirk to work around. + +**Confirm the grant loaded before relying on it**, since inferring it from a write that no longer denies means learning the answer by making the write. In a restarted session in that checkout, read the variable the hook reads: + +```shell +printenv GH_WRITE_GUARD_ALLOW +``` + +Run it bare, with no `VAR=value` prefix of its own, which would report a value the hook never sees. An empty result means the grant did not load, and the fix is the file location or the restart rather than the token. Feeding the hook a synthetic payload is not a usable probe from inside a session, because the payload text carries the very write shape the guard matches and the guard denies the probe command itself. + +Withdraw a grant by deleting the `env` entry and restarting. Nothing expires it, so a grant left in place stays live for every later session in that checkout, which is the reason to remove it once the work that needed it is done. + +## Manual settings.json Shape (for Reference) + +The installer writes this. It is here so you can inspect it, not as a template to hand-place: the +literal `"python3"` shown for `command` is what the installer writes when a `python3` launcher is +available, but it substitutes `sys.executable`'s own absolute path instead when it is not (a +Windows host with no `python3` shim, most commonly), while `permissions.allow`'s own +`Bash(python3 scripts/pr_review.py:*)` rule always names `python3` literally regardless, since it +matches the command Claude Code itself runs `pr_review.py` under, not the hook's own launcher. +Run the installer rather than hand-copying this block on a host where the two might differ. + +```json +{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [ { "type": "command", "command": "\"python3\" \"/.claude/hooks/gh-write-guard.py\"" } ] } + ] + }, + "permissions": { + "allow": [ "Bash(python3 scripts/pr_review.py:*)" ] + } +} +``` + +Every other key in the file is left as it stands, `permissions.allow` included, apart from the rules whose prefix this kit owns. + +## Scope and Limits + +- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365][issue-365] tracks. +- **Precision over recall for the write footguns.** The hook denies the specific dangerous write shapes with high confidence rather than gating every write, so it is written to avoid blocking legitimate work -- a known false-positive shape is a parser defect to fix, not an accepted cost. A shape it does not catch still falls under the behavioral rules. +- **The branch-bypass rule fails closed.** Unlike the write-footgun rules, a push to `main`/`master`/`develop` is denied even when its rules cannot be determined (the API is unreachable, or the checkout's origin cannot be resolved to query them), because the harm there is a silent success under the maintainer's admin bypass. The rule reads each branch's live rules, so it adapts to every repo (a code-style `develop` denies, a config-style `develop` allows) with no per-repo configuration, and hands the exact command to the maintainer to run when a bypass is genuinely intended. +- **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id, which is exactly why rule 2 blocks a *literal* id at all, since a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit `-R`/`repos//` target. A write from a non-git directory, or one whose target is only a node id, is evaluated by rules 1 and 2 alone. +- **A write inside a script file is unseen, so never batch writes into a script.** The hook reads the command the agent runs, which for `bash deploy.sh` is that one string, so a `git push` or a `gh` mutation inside the script reaches the server without the hook ever parsing it. This is the widest hole in the kit and it is one an agent opens by accident, since collecting fifteen repetitive pushes into a loop looks like tidiness rather than like disabling a guard. Issue each write as its own command. A script that only reads, computes, or prepares local commits is fine, because the boundary is the write and not the script. +- **Not a credential control.** A fine-grained PAT limited to owned repositories is a separate, stronger structural guard (a hard `403` on any non-owned repo) and is left to per-machine credential setup, out of this kit. +- **Rule 6's ref-checkout and `--ff-only` exemptions are deliberate, not a gap.** The #1073 incident's own literal commands (an unforced `git checkout main`, then `git pull --ff-only`) are exactly this shape, so this rule does not deny the incident's own commands specifically. Denying them would also break the documented base-clone cleanup step in the `repo-worktree` skill, which needs precisely those two. The concurrent-access hazard those commands still carry (switching HEAD, or fast-forwarding, in a checkout another task might be relying on) is not decidable from the command text alone, so it stays the prose rule's job. A non-force flag alongside the ref, such as `--detach`, is exempt too, not only a strictly zero-flags invocation -- git's own overwrite-refusal on a dirty tracked file is unaffected by it, verified live. +- **Rule 6 needs git 2.31 or newer for `rev-parse --path-format=absolute`.** An older git errors on that flag, `_is_primary_checkout` reads that as an unresolved target, and the rule fails open silently on that host -- no crash, no denial, and no diagnostic pointing at the version. `spec/host-tools.json` carries no floor on `git` today (see its own `why` field for the reasoning that applies fleet-wide). This is a narrower, rule-specific dependency worth knowing about rather than a case for raising that floor on its own. + + +[spec]: ../README.md +[codex]: ../codex/README.md +[opencode]: ../opencode/README.md +[governance]: ../../../GOVERNANCE.md +[write-guard]: ./gh-write-guard.py +[issue-365]: https://github.com/ptr727/ProjectTemplate/issues/365 +[issue-781]: https://github.com/ptr727/ProjectTemplate/issues/781 diff --git a/host-setup/agent-safety/claude-md-fleet.md b/host-setup/agent-safety/claude/claude-md-fleet.md similarity index 100% rename from host-setup/agent-safety/claude-md-fleet.md rename to host-setup/agent-safety/claude/claude-md-fleet.md diff --git a/host-setup/agent-safety/claude-md-safety.md b/host-setup/agent-safety/claude/claude-md-safety.md similarity index 93% rename from host-setup/agent-safety/claude-md-safety.md rename to host-setup/agent-safety/claude/claude-md-safety.md index 54a52a87..a9c60e66 100644 --- a/host-setup/agent-safety/claude-md-safety.md +++ b/host-setup/agent-safety/claude/claude-md-safety.md @@ -7,7 +7,7 @@ A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted w - **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a node id, a numeric id, a thread or comment id) is captured from a live query in the same session into a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail. It writes to the wrong target, in another repository. If a query returns no id, stop rather than invent one. - **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works, and never append an output-discarding or force-success tail (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) to a mutation. A write that appears to fail is verified, not assumed harmless, because it may have succeeded on the server. - **Each task runs in its own checkout, in its own directory, on its own feature branch.** The unit is the task rather than the agent, since one agent moving between two repositories meets the same hazard as two agents sharing one tree. A blanket `git add -A`, a `git reset --hard`, and a branch switch each cross the boundary while being correct in isolation, so a mutating command takes an absolute path, or a `cd` to one in the same invocation, rather than the working directory it inherited. A branch that changes when nothing you did changed it, or an edit of yours reverted with no conflict, means another task is live in that tree, and the response is to stop rather than to re-apply the edit. -- **A task isolates into its own git worktree before its first file edit, and a continuation re-isolates.** All new work, a continuation of a prior session's task included, begins by creating a unique worktree (or clone) on its own feature branch, based on the branch work starts on for that repository rather than whichever branch a tool defaulted to. The primary checkout is the maintainer's own surface, so a session launched there isolates before writing rather than after noticing contention. Staging is by explicit path, never `git add -A` or `git add .`, since a blanket add is how a shared tree turns into a mis-scoped commit. A committed repo's `GOVERNANCE.md` "Repository Boundaries and Write Safety" and the hub's `repo-worktree` skill carry the full rule and the mechanics. +- **A task isolates into its own git worktree before its first file edit, and a continuation re-isolates.** All new work, a continuation of a prior session's task included, begins by creating a unique worktree (or clone) on its own feature branch, based on the branch work starts on for that repository rather than whichever branch a tool defaulted to. The primary checkout is the maintainer's own surface, so a session launched there isolates before writing rather than after noticing contention. Staging is by explicit path, never `git add -A` or `git add .`, since a blanket add is how a shared tree turns into a mis-scoped commit. `gh-write-guard` now also denies a mutating git operation (reset, add, commit, and most of the rest, a documented handful of exemptions such as a fast-forward-only pull kept aside) run directly against a primary checkout, a mechanical backstop for the recurring case of this rule. Which checkout a read happens in, and everything else about isolation, stays prose only. A committed repo's `GOVERNANCE.md` "Repository Boundaries and Write Safety" and the hub's `repo-worktree` skill carry the full rule and the mechanics. ## Data in Agent-Authored Text (Any Project, Every Session) diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py similarity index 50% rename from host-setup/agent-safety/gh-write-guard.py rename to host-setup/agent-safety/claude/gh-write-guard.py index 4da0284f..0ed265d8 100755 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -1,15 +1,18 @@ #!/usr/bin/env python3 -"""PreToolUse guard: deny the GitHub-write footguns behind the cross-repo comment incident. +"""PreToolUse guard: deny the GitHub-write footguns and the primary-checkout mutation behind two incidents. Registered as a Claude Code PreToolUse hook on the Bash tool. It reads the tool-input JSON on stdin, classifies the command, and DENIES (with a reason shown to the agent) when a command is a GitHub *write* -matching a known-dangerous pattern. Reads and everything that is not a clear write pass through. +matching a known-dangerous pattern, or a mutating git operation run directly against a primary checkout. +Reads and everything that is not a clear write pass through. See host-setup/agent-safety/README.md for +the requirements this implements, stated once, agent-agnostic, and for how to audit this file against +them. -Precision over recall for the write-footgun shapes (1-3): they deny the specific shapes that caused the -incident, not everything unparseable - a false deny would break the agent, and a miss still falls under -the GOVERNANCE.md "Repository Boundaries and Write Safety" prose rules. The branch-bypass rule (4) instead -fails CLOSED on the protected-by-default branches, because the harm there is a silent success under the -maintainer's admin bypass. The denied shapes: +Precision over recall for the write-footgun shapes (1-3) and the primary-checkout shape (6): they deny +the specific shapes that caused an incident, not everything unparseable, since a false deny would break the +agent, and a miss still falls under the GOVERNANCE.md "Repository Boundaries and Write Safety" prose +rules. The branch-bypass rule (4) instead fails CLOSED on the protected-by-default branches, because the +harm there is a silent success under the maintainer's admin bypass. The denied shapes: 1. a state-changing gh call whose output is discarded or forced to success (>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo) @@ -30,6 +33,46 @@ skimming the pull request (the incident behind this rule). Permitted only under the same GH_WRITE_GUARD_ALLOW grant rule 3 reads, since the helper refuses a cross-owner pull request outright and the hand-run GraphQL form is then the documented fallback, not a footgun. + 6. a mutating git operation (checkout/switch/pull/reset/rebase/merge/cherry-pick/revert/restore/stash + (anything but list/show)/clean -f/add/commit/rm/mv/apply/am/push/worktree remove -f) run + directly against a primary checkout, not a linked worktree. This is the harm behind a + separate incident, where an agent reused the maintainer's own primary checkout instead of a + worktree twice despite having read the prose rule against it. A "primary checkout" is decided + by comparing `git rev-parse --git-dir` against `--git-common-dir`, never a `.git`-is-a-directory + guess (a submodule's `.git` is a file and is still primary). The target directory follows real + git's own priority rather than a last-option-wins scan: any `-C ` options on the + invocation compose sequentially onto a leading `cd` (inside a `sh -c`/`bash -c` wrapper too) or + the session's own cwd, then an explicit `--work-tree`/`GIT_WORK_TREE=` value, when given, wins + over that result regardless of `-C`, and `--git-dir`/`GIT_DIR=` alone never relocates that + reported target, matching git's own fallback. A leading `export GIT_WORK_TREE=x GIT_DIR=y &&` + prefix is read the same way an inline `VAR=x git ...` prefix already is, since a real shell + export persists into the following command exactly as effectively, confirmed live to discard a + tracked local modification with no redirect at all on the git invocation itself, a shape the + inline-prefix scan alone cannot see. + Whether the invocation is primary-checkout at all is a separate question from the mutation target, + though: an explicit `--git-dir`/`GIT_DIR=` is resolved and tested for primary-checkout-ness + directly, regardless of `--work-tree`, since `--git-dir` names the repository actually mutated, + confirmed live that `--git-dir=/.git --work-tree=` mutates `` even + though `` resolves as no git repository at all, which testing the work-tree value alone + would fail open on. `~`/`$HOME` is expanded throughout and a relative value is joined against the + running result. Checkout/switch force flags (`-b`/`-B` for checkout, `-c`/`-C` for switch, + `-f`/`--force`/`--discard-changes`/`--orphan` for either) are recognized bundled or attached into a + short-option cluster (`-qf`, `-Bname`, `-Cother`), not only as an exact token. A subcommand this + rule does not recognize is resolved through a bounded chain of git aliases (inline `-c + alias.=`, then the target's own persisted config) before falling through to allow. A + `!`-prefixed shell alias is denied outright rather than interpreted. Exempt: `worktree + add/list/prune` and an unforced `worktree remove` (the + documented way to use a primary checkout at all), `merge --ff-only`/`pull --ff-only` (can never + discard anything), and a `checkout `/`switch ` carrying no force-oriented flag whose + argument actually resolves as a ref, verified live (git's own ref-switch path refuses to carry a + local modification, but its pathspec-restore fallback for an argument that is not a ref, such as + `checkout .` or `checkout -- `, carries no such check and is denied). A non-force flag such + as `--detach`/`-q` alongside the ref stays exempt too, since it changes nothing about git's own + overwrite-refusal, verified live, so admitting it widens no actual safety hole, only the exemption's + literal shape, matching the documented base-clone cleanup step in the repo-worktree skill. Granted + only by GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT (a recognized falsy + value such as "0"/"false" reads as not granted, not as any-non-empty-string-is-truthy), the same + channel shape as GH_WRITE_GUARD_ALLOW. Run `gh-write-guard.py --selftest` to verify the decision matrix without Claude Code. """ @@ -302,6 +345,9 @@ def _shell_tokens(cmd): try: lex = shlex.shlex(cmd, posix=True, punctuation_chars=_PUNCTUATION_CHARS) lex.whitespace_split = True + # `shlex.shlex`'s own default keeps `#` as a comment starter, unlike `shlex.split()`, which explicitly clears it, and confirmed live to otherwise fuse `git fetch origin # x\ngit reset --hard` into one invocation, hiding the second command from every tokenizer-based rule. + # Cleared unconditionally: a truncated command is a far worse failure than an ordinary `#` becoming literal trailing argv words instead. + lex.commenters = "" lex.whitespace = lex.whitespace.replace( "\n", "" ) # A newline is an operator above rather than a gap between words. @@ -407,6 +453,670 @@ def _git_subcommand_arglists(cmd, sub): return out +# --- Rule 6: a mutating git op against a primary checkout --------------------------------------------- +# `-C `, `--work-tree `/`--work-tree=`, and a `GIT_WORK_TREE=` command-text prefix are the options this rule resolves the mutation target directory from, following real git's own priority rather than a last-one-wins scan across all three. +# `--work-tree`/`GIT_WORK_TREE` name the working tree a mutating command like `reset --hard`/`clean -f` actually writes into, and win regardless of where `-C` points or how many `-C` options preceded it. +# Multiple `-C` options compose sequentially, each resolved against the previous one exactly as git's own "run as if git was started in " describes, an absolute value replacing the running directory outright and a relative one joining onto it. +# `--git-dir`/`GIT_DIR=` alone, with no `--work-tree`/`GIT_WORK_TREE` given anywhere on the same invocation, does not relocate the mutation target at all -- per git's own documented fallback, the working tree stays the effective directory reached by any `-C` chain (or the session's cwd, with none), so this rule never reads `--git-dir`/`GIT_DIR=` as a target-setting option for that purpose. +# An explicit `--git-dir`/`GIT_DIR=` is still read and resolved separately, though, for a different purpose: deciding whether the invocation is primary-checkout or not. +# When `--git-dir` and `--work-tree` are both given and point at different trees, the repository actually mutated is the one `--git-dir` names, not whatever `--work-tree` happens to be -- confirmed live (`git --git-dir=/.git --work-tree= commit` mutates ``, even though `` resolves as no git repository at all) -- so testing the resolved `--work-tree` value alone for primary-checkout-ness would fail open exactly there. +# Every other value-taking global option is skipped like `_git_subcommand_arglists` already does, since none of the others name a directory this rule reads. +_GIT_ENV_WORK_TREE_VAR = "GIT_WORK_TREE" +_GIT_ENV_GIT_DIR_VAR = "GIT_DIR" + + +_ENV_ASSIGN_RE = re.compile(r"^([A-Za-z_][A-Za-z0-9_]*)=(.*)$") + + +def _env_prefix_dirs(toks, git_index): + """The values of a `GIT_WORK_TREE=`/`GIT_DIR=` assignment immediately preceding the git + invocation at `toks[git_index]`, in the shape `GIT_WORK_TREE=x GIT_DIR=y git ...`, as a + `(work_tree, git_dir)` pair, either of which may be `None`. Scans backward only through + consecutive `NAME=value`-shaped tokens, stopping at the first token that is not one (a shell + separator, another command, or the start of the string), so this never reads an assignment + belonging to an earlier, unrelated command in the same compound line. Read from the command's + own text, not a real process environment: an inline assignment here genuinely does redirect + the invocation it prefixes, unlike the `GH_WRITE_GUARD_ALLOW=x gh ...` shape documented + elsewhere in this file, which never reaches the hook's own environment. + """ + found = {} + k = git_index - 1 + while k >= 0: + m = _ENV_ASSIGN_RE.match(toks[k]) + if not m: + break + found.setdefault(m.group(1), m.group(2)) + k -= 1 + return found.get(_GIT_ENV_WORK_TREE_VAR), found.get(_GIT_ENV_GIT_DIR_VAR) + + +_INLINE_ALIAS_RE = re.compile(r"^alias\.(\S+)=(.*)$") + + +def _git_invocations(cmd): + """Every `git [global-options] [args...]` invocation in the command, as + `(c_dirs, work_tree, git_dir, inline_aliases, sub, args)` tuples. `c_dirs` is the list of + `-C ` values on this specific invocation, in argv order, since real git composes multiple + `-C` options sequentially rather than having only the last one take effect. `work_tree` is the + value of a `--work-tree` global option on this invocation, or a `GIT_WORK_TREE=` prefix + immediately before it when no `--work-tree` flag is given, or `None` when neither is given, in + which case the caller resolves the mutation target from the `-C` chain alone. `git_dir` is the + same shape for `--git-dir`/`GIT_DIR=`: never used to relocate the mutation target on its own + (matching real git's "no relocation without `--work-tree`" fallback), but read and resolved + separately, since a `--git-dir` explicitly naming a different tree than `--work-tree` is the + tree actually mutated, not the one `--work-tree` names. `inline_aliases` is a `name -> + expansion` dict of every `-c alias.=` override on this invocation, in argv order + (a later `-c` for the same name wins, matching real git's own repeated `-c` semantics), read + here since an inline alias is exactly as effective at hiding a mutating command behind an + unrecognized name as a persisted one, per requirement 6's alias-resolution rule. + """ + # Shares its tokenizing and global-option skipping with `_git_subcommand_arglists`, generalized to read every subcommand rather than one named subcommand, and to capture the directory-naming and alias-defining options along the way. + toks = _shell_tokens(cmd) + n = len(toks) + out = [] + i = 0 + while i < n: + if not _is_git_exe(toks[i]): + i += 1 + continue + j = i + 1 + c_dirs = [] + work_tree = None + git_dir = None + inline_aliases = {} + while j < n and toks[j].startswith("-"): + opt = toks[j] + if opt == "-C": + if j + 1 < n: + c_dirs.append(toks[j + 1]) + j += 2 + else: + j += 1 + elif opt.startswith("--work-tree="): + work_tree = opt.split("=", 1)[1] + j += 1 + elif opt == "--work-tree": + if j + 1 < n: + work_tree = toks[j + 1] + j += 2 + else: + j += 1 + elif opt.startswith("--git-dir="): + git_dir = opt.split("=", 1)[1] + j += 1 + elif opt == "--git-dir": + if j + 1 < n: + git_dir = toks[j + 1] + j += 2 + else: + j += 1 + elif opt == "-c": + if j + 1 < n: + m = _INLINE_ALIAS_RE.match(toks[j + 1]) + if m: + inline_aliases[m.group(1)] = m.group(2) + j += 2 + else: + j += 1 + elif opt in _GIT_GLOBAL_VALUE_OPTS and "=" not in opt: + j += 2 + else: + j += 1 + env_work_tree, env_git_dir = _env_prefix_dirs(toks, i) + if work_tree is None: + work_tree = env_work_tree + if git_dir is None: + git_dir = env_git_dir + if j < n and not _is_shell_op(toks[j]): + sub = toks[j] + args, k = _collect_arglist(toks, j + 1) + out.append((c_dirs, work_tree, git_dir, inline_aliases, sub, args)) + i = k + else: + i = j if j > i else i + 1 # a bare `git` with no subcommand at all; keep scanning + return out + + +def _all_git_invocations(cmd): + """`_git_invocations` for `cmd` itself, plus for every command string a `sh -c`/`bash -c`-style + wrapper embeds in it, the same expansion `_all_gh_arg_lists` gives the GitHub-write rules, so a + mutating git command hidden behind such a wrapper is scanned exactly like a bare one. Each + tuple carries a seventh element, the leading-`cd` directory in effect for the exact command + string (outer or inner) it came from -- a `cd` embedded inside a wrapper's own command string + (`bash -c 'cd /x && git ...'`) is invisible to a leading-`cd` check run only against the outer + command, and the outer command's own leading `cd` (`cd /x && bash -c 'git ...'`) takes effect + inside the wrapper too, via the shell's own inherited cwd, when the wrapped string carries no + leading `cd` of its own to override it. A leading `export GIT_WORK_TREE=x GIT_DIR=y &&` prefix + is folded into `work_tree`/`git_dir` themselves the same way, whenever the invocation's own + flags or inline `VAR=x git ...` prefix leave either unset, since an exported assignment + persists into a following command exactly as effectively as either of those, and this rule + must not fail open just because the redirect came from `export` rather than from `-C`, an + inline prefix, or a flag. + """ + outer_leading_cd = _leading_cd_dir(cmd) + outer_export_wt, outer_export_gd = _leading_export_dirs(cmd) + out = [] + for c_dirs, work_tree, git_dir, inline_aliases, sub, args in _git_invocations(cmd): + work_tree = work_tree if work_tree is not None else outer_export_wt + git_dir = git_dir if git_dir is not None else outer_export_gd + out.append((c_dirs, work_tree, git_dir, inline_aliases, sub, args, outer_leading_cd)) + for inner in _embedded_wrapper_commands(cmd): + leading_cd = _leading_cd_dir(inner) or outer_leading_cd + inner_export_wt, inner_export_gd = _leading_export_dirs(inner) + export_wt = inner_export_wt if inner_export_wt is not None else outer_export_wt + export_gd = inner_export_gd if inner_export_gd is not None else outer_export_gd + for c_dirs, work_tree, git_dir, inline_aliases, sub, args in _git_invocations(inner): + work_tree = work_tree if work_tree is not None else export_wt + git_dir = git_dir if git_dir is not None else export_gd + out.append((c_dirs, work_tree, git_dir, inline_aliases, sub, args, leading_cd)) + return out + + +_HOME_VAR_RE = re.compile(r"\$\{HOME\}|\$HOME(?![A-Za-z0-9_])") + + +def _expand_dir(value): + """Expand a leading `~`/`~user` the same way a shell would, plus a literal `$HOME`/`${HOME}` + reference, both resolvable without executing anything -- `~/repos/` is the fleet's own + documented primary-checkout path convention, so leaving it unexpanded would fail open on the + single most common way to spell the path this rule exists to catch. The `${HOME}` form is + always exact, its closing brace delimits the name, but a bare `$HOME` is matched only when not + immediately followed by another identifier character, so this never matches only a prefix of + an unrelated variable such as `$HOMEPATH` or `$HOMEDRIVE`. Any other `$VAR` is left as is and + resolves nowhere real via a plain `-C`, which is this rule's documented fail-open case + already, not a new one: a hook cannot see a shell variable's runtime value without executing + something, and it never does. + """ + if value is None: + return None + value = os.path.expanduser(value) + return _HOME_VAR_RE.sub(lambda _m: os.environ.get("HOME", ""), value) + + +def _join_relative(base, value): + """Expand `~`/`$HOME` in `value`, then join it onto `base` when it is relative and `base` is + known, or return the expanded value as-is when it is already absolute or there is no base to + join onto -- the one join rule every directory-naming option (`-C`, a leading `cd`, + `--work-tree`, `--git-dir`) resolves a relative value with, so a relative spelling always + resolves against the session's own reported cwd rather than wherever the hook process's own + OS-level working directory happens to be, which the two are never guaranteed to share. + """ + v = _expand_dir(value) + if base and not os.path.isabs(v): + return os.path.normpath(os.path.join(base, v)) + return v + + +def _effective_cwd(c_dirs, leading_cd, cwd): + """The directory git treats as its own current working directory for this invocation, after + folding a leading `cd` prefix and then any `-C` chain onto the hook's own reported `cwd`, in + that order -- the same base both `--work-tree` and `--git-dir` resolve a relative value + against. Multiple `-C` options compose sequentially, each resolved against the previous one + exactly as git's own "run as if git was started in " describes for a repeated `-C`, an + absolute value replacing the running directory outright and a relative one joining onto it. + """ + base = _expand_dir(cwd) if cwd is not None else None + if leading_cd is not None: + base = _join_relative(base, leading_cd) + for c in c_dirs: + base = _join_relative(base, c) + return base + + +def _resolve_target_dir(c_dirs, work_tree, leading_cd, cwd): + """The mutation target directory for this invocation, following real git's own priority + rather than a last-option-wins scan across `-C`/`--work-tree`: the effective directory reached + by a leading `cd` and any `-C` chain (see `_effective_cwd`), with an explicit + `--work-tree`/`GIT_WORK_TREE=` value, when given, winning over that result regardless of how + many `-C` options preceded it, matching how `--work-tree`/`GIT_WORK_TREE` name the actual + mutation target independent of where `-C` points. A relative `work_tree` still resolves + against the effective directory, the same as git resolves a relative `--work-tree` against its + own effective directory. + """ + base = _effective_cwd(c_dirs, leading_cd, cwd) + if work_tree is not None: + return _join_relative(base, work_tree) + return base + + +def _resolve_repo_dir(git_dir, c_dirs, leading_cd, cwd): + """The explicit `--git-dir`/`GIT_DIR=` value on this invocation, resolved the same way a + `--work-tree` value is (joined onto the effective directory reached by a leading `cd` and any + `-C` chain, see `_effective_cwd`), or `None` when no explicit git-dir was given on this + invocation at all -- in which case the caller falls back to ordinary ancestor-based repository + discovery from the resolved mutation target instead, exactly as real git itself does absent an + explicit `--git-dir`. + """ + if git_dir is None: + return None + return _join_relative(_effective_cwd(c_dirs, leading_cd, cwd), git_dir) + + +# A single leading `cd &&`/`cd ;` prefix, and no more, a narrow, tractable parse rather than tracking shell execution state. +# `git status && cd x && git pull` still resolves the second invocation's target from cwd, a materially smaller gap than an entirely unresolved one. +_CD_CHAIN_SEPS = ("&&", ";") + + +def _leading_cd_dir(cmd): + """The directory a command starts with `cd &&` or `cd ;`, or `None`.""" + toks = _shell_tokens(cmd) + if ( + len(toks) >= 3 + and toks[0] == "cd" + and not toks[1].startswith("-") + and toks[2] in _CD_CHAIN_SEPS + ): + return toks[1] + return None + + +def _leading_export_dirs(cmd): + """The `GIT_WORK_TREE`/`GIT_DIR` values from a single leading `export NAME=value ... &&`/`;` + prefix, as a `(work_tree, git_dir)` pair, either of which may be `None` -- the same narrow, + tractable scope `_leading_cd_dir` already takes (only a leading prefix is read, one appearing + after the first command in a chain is the accepted gap), extended to the one other shell shape + that redirects a git invocation carrying no `-C`/`--work-tree`/`--git-dir`/inline-prefix of its + own: a real shell `export` makes an assignment persist into every later command in the same + session, unlike the inline `VAR=x git ...` prefix `_env_prefix_dirs` already reads, which + redirects only the one command it immediately precedes. Confirmed live: `export + GIT_DIR=/.git GIT_WORK_TREE= && git reset --hard` discards a tracked local + modification in ``, with no redirect at all on the `git` invocation itself, a shape + `_env_prefix_dirs` alone cannot see. Bails to `(None, None)` on anything but a clean run of + `NAME=value` tokens between `export` and the first separator, rather than guessing at a + non-assignment `export` form (`export -p`, `export EXISTING_VAR` with no `=`). + """ + toks = _shell_tokens(cmd) + n = len(toks) + if not toks or toks[0] != "export": + return None, None + work_tree = git_dir = None + i = 1 + while i < n and not _is_shell_op(toks[i]): + m = _ENV_ASSIGN_RE.match(toks[i]) + if not m: + return None, None + if m.group(1) == _GIT_ENV_WORK_TREE_VAR: + work_tree = m.group(2) + elif m.group(1) == _GIT_ENV_GIT_DIR_VAR: + git_dir = m.group(2) + i += 1 + if i >= n or i == 1 or toks[i] not in _CD_CHAIN_SEPS: + return None, None + return work_tree, git_dir + + +def _is_primary_checkout(target_dir, git_dir=None): + """`True` when the repository this invocation targets is a primary (non-worktree) git + checkout, `False` when it is a linked worktree, `None` when no git repository resolves at all + (the caller fails open on `None`, matching this rule's own precision-over-recall stance). + + The test is a `rev-parse` comparison, not a filesystem-shape guess: `--git-dir` equals + `--git-common-dir` for a primary checkout and differs for a linked worktree. A `.git`-is-a- + directory heuristic is deliberately not used instead, since a plain submodule's `.git` is a + file while it is still a primary working tree that can lose uncommitted work, and that + heuristic would wrongly exempt it. `--path-format=absolute` must precede the two paths in + argv, verified silently ineffective (relative paths, no error) in the other order, which would + misclassify a primary checkout as a worktree the moment a command runs from one of its + subdirectories. + + When `git_dir` is given (an explicit `--git-dir`/`GIT_DIR=` was resolved on the invocation), + the check runs against that value directly (`git --git-dir= rev-parse ...`, no `-C` + at all) rather than against `target_dir` via ordinary ancestor discovery, since `--git-dir` + names the repository actually mutated independent of where `--work-tree`/cwd point, confirmed + live: `git --git-dir=/.git --work-tree= commit` mutates `` even + though `` resolves as no git repository at all. Testing `target_dir` in that case + would fail open exactly there. + """ + if git_dir is not None: + argv = ["git", f"--git-dir={git_dir}"] + else: + argv = ["git", "-C", target_dir or "."] + argv += ["rev-parse", "--path-format=absolute", "--git-dir", "--git-common-dir"] + try: + r = subprocess.run( + argv, + capture_output=True, + text=True, + timeout=5, + check=False, + ) + except Exception: # noqa: BLE001 - a crashed/absent git binary is treated as "unresolvable", the same fail-open outcome a non-zero exit already produces below, not a defect to propagate as a hook-crashing traceback. + return None + if r.returncode != 0: + return None + lines = r.stdout.strip().splitlines() + if len(lines) != 2: + return None + return lines[0].strip() == lines[1].strip() + + +# Flags that turn an otherwise-denied `checkout`/`switch` into a real branch-creating or force-discarding operation, the cases git itself does not already refuse on its own. +# `-b`/`-B` are checkout's own create/force-create spellings; `-c`/`-C` are switch's (switch has no `-b`/`-B`, checkout has no `-c`/`-C`), and both pairs mean the same thing to their own subcommand, confirmed live: `git switch -C ` resets that branch to the current HEAD, discarding any commits unique to it, with no dirty-tree warning at all since it is not a working-tree overwrite. +_CHECKOUT_FORCE_FLAGS = { + "-b", + "-B", + "-c", + "-C", + "--force", + "-f", + "--discard-changes", + "--orphan", + "--create", + "--force-create", +} +# The single-character short forms above, checked against every character of a short-option token, not just an exact-token match: git bundles boolean short flags together (`-qf` is `-q`+`-f`) and attaches a short flag's own value with no space (`-Bname` is `-B name`), and in both shapes the exact-token check below never sees a bare `-f`/`-B`/`-c`/`-C` to match against. +# `-b`/`-B`/`-c`/`-C` are the only checkout/switch short options that take an attached value at all, so this scan cannot mistake an unrelated flag's attached argument for a force flag. +# Neither subcommand has any other flag using these letters, checked directly against each subcommand's own `-h` output, so this scan produces no false positive on either. +_CHECKOUT_FORCE_CHARS = {"b", "B", "c", "C", "f"} +# Flags that make `git clean` an actual deletion rather than the dry-run it defaults to. +_CLEAN_FORCE_FLAGS = {"-f", "--force"} +# `-n`/`--dry-run` always wins over `-f`/`--force`, confirmed live regardless of which order the two are given in or how many times `-f` repeats: `git clean -f -n`, `-n -f`, and `--dry-run -f` all print "Would remove" and delete nothing. +_CLEAN_DRY_RUN_FLAGS = {"-n", "--dry-run"} + + +def _args_before_double_dash(args): + """`args` truncated at the first bare `--`, or `args` unchanged when there is none -- every + argument from `--` onward is an unconditional pathspec to git, never a flag, confirmed live: + `git clean -f -- -n` deletes a file literally named `-n` rather than behaving as a dry run, + and `git clean -- -f` (with no real `-f` before the `--`) names a file rather than forcing + anything. Flag detection must never scan past this boundary. + """ + if "--" in args: + return args[: args.index("--")] + return args + + +def _has_clean_dry_run_flag(args): + """Whether `args` carries `-n`/`--dry-run`, bundled into a short-option cluster (`-nfd`) or + not, the same bundled-cluster scan `_has_checkout_force_flag` already gives checkout/switch's + own force flags -- a real, confirmed usability gap this rule's `git clean` case would + otherwise have: `-nfd` denies the exact same harmless dry run `-n` alone does not, purely + because it also carries an `f` character the force-flag scan below reads on its own. + """ + for a in _args_before_double_dash(args): + if a in _CLEAN_DRY_RUN_FLAGS: + return True + if a.startswith("--"): + continue + if a.startswith("-") and len(a) > 1 and "n" in a[1:]: + return True + return False + + +def _has_checkout_force_flag(args): + """Whether `args` carries a checkout/switch force flag, as an exact token + (`--force`/`--orphan`/`--discard-changes`, or a lone `-f`/`-b`/`-B`) or bundled/attached into a + short-option cluster (`-qf`, `-Bname`, `-qBname`). A long-option token (`--...`) is never + scanned character-by-character, only matched exactly, since `--discard-changes` legitimately + contains an `f`. + """ + for a in args: + if a in _CHECKOUT_FORCE_FLAGS: + return True + if a.startswith("--"): + continue + if a.startswith("-") and len(a) > 1 and any(c in _CHECKOUT_FORCE_CHARS for c in a[1:]): + return True + return False + + +# Subcommands denied unconditionally in a primary checkout, no flag or argv shape exempts them. +_ALWAYS_DENY_SUBS = { + "reset", + "rebase", + "cherry-pick", + "revert", + "restore", + "add", + "commit", + "rm", + "mv", + "apply", + "am", + # A push doesn't mutate the local working tree or HEAD the way the rest of this set does, but it publishes whatever is there, and no documented fleet workflow ever pushes from a primary checkout: every push runs from a task's own worktree instead. + # Rule 4's own branch-rule checks (_check_push_bypass) already run before this rule and can deny a push on their own grounds, so this is an added, independent reason to deny, not a replacement for that check. + "push", +} + + +def _resolves_as_ref(target_dir, ref, verify=None): + """Whether `ref` resolves as a real ref (branch, tag, or commit-ish) in `target_dir`'s + repository -- the same test git itself uses to decide whether a bare `checkout`/`switch` + argument names something it safety-checks (a ref switch, refused when it would overwrite a + local modification) or falls back to treating the argument as a pathspec restore, which + carries no such safety check at all. `verify`, when given, stands in for the live subprocess + call so the self-test runs deterministically. + """ + if verify is not None: + return verify(target_dir, ref) + try: + r = subprocess.run( + ["git", "-C", target_dir or ".", "rev-parse", "--verify", "--quiet", ref + "^{commit}"], + capture_output=True, + text=True, + timeout=5, + check=False, + ) + except Exception: # noqa: BLE001 - a crashed/absent git binary is treated as "does not resolve as a ref", the safer of the two branches this call disambiguates, not a defect to propagate as a hook-crashing traceback. + return False + return r.returncode == 0 + + +# A subcommand name this rule does not recognize could be a git alias rather than an unrelated tool invocation this rule has no reason to inspect, so resolution stops once this many aliases have been chased, rather than looping forever on a self-referential or absurdly deep alias chain. +_MAX_ALIAS_DEPTH = 5 + + +def _config_alias(target_dir, name, config_lookup=None): + """The expansion text of the git alias named `name` in `target_dir`'s own config (merged + local/global/system, the same precedence `git config --get` itself reads), or `None` when no + such alias is defined. `config_lookup`, when given, stands in for the live subprocess call so + the self-test runs deterministically offline. + """ + if config_lookup is not None: + return config_lookup(target_dir, name) + try: + r = subprocess.run( + ["git", "-C", target_dir or ".", "config", "--get", f"alias.{name}"], + capture_output=True, + text=True, + timeout=5, + check=False, + ) + except Exception: # noqa: BLE001 - a crashed/absent git binary is treated the same as "no alias defined", the caller's existing fall-through-to-allow case, not a defect to propagate as a hook-crashing traceback. + return None + if r.returncode != 0: + return None + return r.stdout.strip() or None + + +def _resolve_alias(sub, args, inline_aliases, target_dir, config_lookup=None): + """Expand `sub` through a chain of git aliases -- an inline `-c alias.=` override + on this invocation first, then the target checkout's own persisted config, matching real + git's own override order -- up to `_MAX_ALIAS_DEPTH` levels, so a subcommand this rule does + not otherwise recognize is not silently allowed just because it is spelled as a custom alias + rather than the built-in name it actually expands to. Returns `(sub, args, opaque)`: `opaque` + is `True` the moment any alias in the chain is a `!`-prefixed shell command rather than a git + subcommand alias, since that shape hands git an arbitrary shell string this function does not + and must not execute to interpret -- the caller denies that shape outright against a primary + checkout rather than either running it or letting it fall through to an ordinary allow, the + one place this rule departs from its usual fail-open stance, because the alias definition + itself is concrete, positive evidence of an attempt to run something via git in exactly the + directory this rule exists to protect. + """ + seen = set() + depth = 0 + while depth < _MAX_ALIAS_DEPTH and sub not in seen: + seen.add(sub) + expansion = inline_aliases.get(sub) + if expansion is None: + expansion = _config_alias(target_dir, sub, config_lookup) + if expansion is None: + return sub, args, False + if expansion.startswith("!"): + return sub, args, True + try: + expanded = shlex.split(expansion) + except ValueError: + # Malformed alias text (unbalanced quotes): treat it as unresolvable rather than crashing the hook on a config value neither the agent nor this rule controls. + return sub, args, False + if not expanded: + return sub, args, False + sub, args = expanded[0], expanded[1:] + args + depth += 1 + return sub, args, False + + +def _primary_checkout_verdict(sub, args, target_dir=None, ref_resolver=None): + """Whether this `(subcommand, args)` pair is a mutating operation requirement 6 denies against + a primary checkout: `True` (deny), `False` (exempt, explicitly allowed), or `None` (not a + subcommand this rule concerns itself with, allowed by falling through). `target_dir` and + `ref_resolver` are used only by the `checkout`/`switch` case, to disambiguate a bare argument + from a live git call; every other case is pure text/argv classification. + """ + if sub == "worktree": + # `add`/`list`/`prune` are always allowed, the documented way to use a primary checkout from an agent session. + # `remove` is allowed too unless forced: git itself already refuses to remove a worktree carrying uncommitted changes without --force, so only the forced form reproduces the harm this rule exists to catch. + # `-f` is bundled the same way checkout/switch's own force flags already are: git requires `-f` given twice to remove a locked worktree, and `-ff` satisfies that, confirmed live. + # Scanned before any `--`, the same cutoff `clean`'s own force scan already applies: confirmed live that `git worktree remove -- -f` reads `-f` as a worktree path argument (erroring since none is literally named that), not a force flag. + if not args or args[0] != "remove": + return None + return any( + a in ("-f", "--force") or (a.startswith("-") and not a.startswith("--") and "f" in a) + for a in _args_before_double_dash(args[1:]) + ) + if sub in ("checkout", "switch"): + # `--` unambiguously means every following argument is a pathspec, not a ref: `checkout -- `/`checkout -- ` restores that path from the index unconditionally, with none of the "would overwrite a local modification" safety check a ref switch gets. + if "--" in args: + return True + if _has_checkout_force_flag(args): + return True + # A bare `-` is itself a real, git-recognized ref (the previous branch), not a flag, even though it starts with the same character every flag does. + positional = [a for a in args if a == "-" or not a.startswith("-")] + # More than one bare positional with no `--` is the same ambiguous/pathspec-leaning shape (`checkout `), denied rather than guessed at. + # Exactly one is the case that needs disambiguating live, below. + if len(positional) != 1: + return True + # A bare `-` is exempt outright rather than live-checked: it is porcelain shorthand for "the previous branch" that only `checkout`/`switch` themselves understand, and `git rev-parse` (what the live check below runs) does not resolve it as a ref at all, which would otherwise misread this exact safe case as a pathspec. + if positional[0] == "-": + return False + # `checkout `/`switch `, with any non-force flag also allowed alongside it (already filtered out of `positional` above), is exempt only when `` actually resolves as a ref. + # Git's own ref-switch path refuses to overwrite a local modification regardless of a non-force flag like --detach/-q, but its pathspec-restore fallback (what git runs when the argument is not a ref, such as `git checkout .`) carries no such check, so denying it is exactly as safe as denying the `--` form above. + # This is the one case in this rule that needs a live git call to decide. + return not _resolves_as_ref(target_dir, positional[0], ref_resolver) + if sub in ("merge", "pull"): + # `--ff-only` can never discard a commit or a local change, failing cleanly instead of mutating when a fast-forward is not possible. + return "--ff-only" not in args + if sub in _ALWAYS_DENY_SUBS: + return True + if sub == "stash": + # `list`/`show` only read the stash; everything else, bare `stash`/`push`/`save` included, mutates the working tree the same way `pop`/`apply`/`drop` obviously do. + return not args or args[0] not in ("list", "show") + if sub == "clean": + # -n/--dry-run always wins over -f/--force, confirmed live: `-nfd` deletes nothing, so denying it would add no safety while breaking a genuinely harmless, read-only preview of what a later, real `clean -fd` would remove. + if _has_clean_dry_run_flag(args): + return False + # Scanned before any `--`: everything from `--` onward is an unconditional pathspec, confirmed live that `git clean -f -- -n` deletes a file literally named `-n` rather than reading as a dry run, and `git clean -- -f` names a file rather than forcing anything with no real `-f` before the `--`. + return any( + a in _CLEAN_FORCE_FLAGS or (a.startswith("-") and not a.startswith("--") and "f" in a) + for a in _args_before_double_dash(args) + ) + return None + + +# Values of GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT read as not granted, the same convention a shell boolean env var commonly uses, so setting it to "0"/"false"/"no" to turn the grant *off* actually does. +# The bare `environ.get(...)` truthiness this replaces read any non-empty string, that one included, as granted. +_FALSY_ENV_VALUES = {"", "0", "false", "no", "off"} + + +def _check_primary_checkout_mutation( + cmd, cwd, environ=None, primary_checkout_lookup=None, ref_resolver=None, config_lookup=None +): + """Rule 6: deny a mutating git operation run directly against a primary (non-worktree) + checkout. `environ` is a test seam, the same shape rules 3 and 5 already take. + `primary_checkout_lookup`, when given, stands in for `_is_primary_checkout` so the self-test + runs deterministically offline instead of resolving a real checkout on the machine running it. + `ref_resolver` is the same kind of seam for `_resolves_as_ref`, and `config_lookup` the same + kind of seam for `_config_alias`. + + Fails open (allow) when no git repository resolves at the target at all, matching the + footgun rules' precision-over-recall stance rather than rule 4's fail-closed one: the harm + here needs a positively-identified primary checkout to fire on, and a hard fail-closed would + deny unrelated Bash work in any non-git directory. + """ + environ = environ if environ is not None else os.environ + # Read the same way GH_WRITE_GUARD_ALLOW is: from the environment the session was launched with, never a channel the agent itself can set (an inline `VAR=x cmd` prefix or an `export` inside the same call must not satisfy this). + grant_value = environ.get("GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT", "").strip().lower() + if grant_value not in _FALSY_ENV_VALUES: + return "allow", "" + # Two independent caches, deliberately never merged into one: an identity-dimension key and a file-dimension key can coincide as the same literal string while needing different test methods (--git-dir=X directly versus ordinary -C X discovery), and a single shared cache keyed only by that string would silently reuse one method's answer for the other's lookup. + identity_cache = {} + file_cache = {} + for c_dirs, work_tree, git_dir, inline_aliases, sub, args, leading_cd in _all_git_invocations( + cmd + ): + resolved = _resolve_target_dir(c_dirs, work_tree, leading_cd, cwd) + repo_git_dir = _resolve_repo_dir(git_dir, c_dirs, leading_cd, cwd) + # This rule tests two independent dimensions of "does this touch a primary checkout", since git's own --work-tree/--git-dir split lets a single invocation mutate one repository's index/refs/HEAD while writing working-tree files into an entirely different directory. + # The identity dimension is the repository whose index, refs, and HEAD actually change: the explicit --git-dir/GIT_DIR= value when one was given (independent of where --work-tree/cwd point, confirmed live to diverge from the mutation target when the two are given together and point at different trees), or, absent one, the repository ordinary ancestor search discovers from the effective cwd (the -C/leading-cd chain) -- never from --work-tree, which only ever redirects where working-tree files are read/written, not where the index, refs, or HEAD live. + # The file dimension is `resolved` itself, the same mutation target already used everywhere else (work-tree when given, else the effective cwd): confirmed live that `git --work-tree= reset --hard HEAD~1`, with no --git-dir override, run from inside a primary checkout with a staged change, moves the *primary's own* branch pointer back a commit and discards the primary's own staged index entry (the identity dimension), even though the command's working-tree-file side effects (the file dimension) land in `` instead -- either dimension resolving primary is enough to deny, since either is a real, distinct way this invocation can destroy a primary checkout's own state. + identity_key = ( + repo_git_dir if repo_git_dir is not None else _effective_cwd(c_dirs, leading_cd, cwd) + ) + file_key = resolved + # Whether this even targets a primary checkout is checked before the subcommand/argv verdict, not after. + # The verdict for `checkout`/`switch` can need its own live git call to disambiguate a ref from a pathspec, and skipping straight past that for the ordinary case (a checkout in a worktree, or targeting no git repository at all) avoids paying for it where the answer would be "allow" regardless. + if identity_key not in identity_cache: + if primary_checkout_lookup is not None: + identity_cache[identity_key] = primary_checkout_lookup(identity_key) + elif repo_git_dir is not None: + identity_cache[identity_key] = _is_primary_checkout( + identity_key, git_dir=repo_git_dir + ) + else: + identity_cache[identity_key] = _is_primary_checkout(identity_key) + if file_key not in file_cache: + file_cache[file_key] = ( + primary_checkout_lookup(file_key) + if primary_checkout_lookup is not None + else _is_primary_checkout(file_key) + ) + is_identity_primary = identity_cache[identity_key] + is_file_primary = file_cache[file_key] + repo_key = identity_key if is_identity_primary else file_key + if not is_identity_primary and not is_file_primary: + continue + verdict = _primary_checkout_verdict(sub, args, resolved, ref_resolver) + if verdict is None: + # `sub` is not one of this rule's own recognized names -- it may be a git alias (inline `-c alias.=...`, or one persisted in the target checkout's own config) expanding to one of them, which is exactly as effective a way to hide a mutating command as spelling it out directly. + sub, args, opaque = _resolve_alias( + sub, args, inline_aliases, repo_git_dir or resolved, config_lookup + ) + if opaque: + return "deny", ( + f"This `git {sub}` resolves to a `!`-prefixed shell alias in a primary " + f"checkout ({repo_key}), which this rule cannot safely inspect. Denied " + "conservatively rather than risking an unreviewed shell command against a " + "checkout a mutating git operation there could destroy another task's " + "uncommitted work in. Create or use a worktree instead (`git worktree add " + "...`), per GOVERNANCE.md 'Repository Boundaries and Write Safety' and the " + "repo-worktree skill. If this primary checkout is genuinely the intended " + "target, ask the maintainer to set GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT " + "before the session starts." + ) + verdict = _primary_checkout_verdict(sub, args, resolved, ref_resolver) + if not verdict: + continue + return "deny", ( + f"This `git {sub}` runs directly against a primary checkout ({repo_key}), not a " + "linked worktree. A mutating git operation there can destroy another task's " + "uncommitted work. Create or use a worktree instead (`git worktree add ...`), per " + "GOVERNANCE.md 'Repository Boundaries and Write Safety' and the repo-worktree " + "skill. If this primary checkout is genuinely the intended target, ask the " + "maintainer to set GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT before the session starts." + ) + return "allow", "" + + def _gh_arg_lists(cmd): """Every `gh [args...]` invocation's own argv, from the token after `gh` up to the next shell separator, in `cmd` itself, not inside any `sh -c`/`bash -c` wrapper (`_all_gh_arg_lists` covers @@ -890,14 +1600,29 @@ def _check_reply_resolve_helper(cmd, environ): return "allow", "" -def classify(cmd, cwd=None, origin=None, current_branch=None, rules_lookup=None, environ=None): +def classify( + cmd, + cwd=None, + origin=None, + current_branch=None, + rules_lookup=None, + environ=None, + primary_checkout_lookup=None, + ref_resolver=None, + config_lookup=None, +): """Return (decision, reason). decision is 'allow' or 'deny'. origin, when given, is a (owner, repo) tuple used instead of resolving from cwd - the self-test - passes it for a deterministic, offline run. current_branch, rules_lookup and environ are likewise - test seams: current_branch stands in for the git resolution of a bare push, rules_lookup(branch) - stands in for the live branch-rules query, and environ stands in for the process environment the - maintainer's grant is read from. + passes it for a deterministic, offline run. current_branch, rules_lookup, environ, + primary_checkout_lookup, ref_resolver and config_lookup are likewise test seams: + current_branch stands in for the git resolution of a bare push, rules_lookup(branch) stands in + for the live branch-rules query, environ stands in for the process environment the + maintainer's grant is read from, primary_checkout_lookup(dir) stands in for resolving a real + checkout's primary-vs-worktree status on the machine running the self-test, ref_resolver(dir, + ref) stands in for the live check that disambiguates a bare `checkout`/`switch` argument as a + ref rather than a pathspec, and config_lookup(dir, name) stands in for the live git-config + read that resolves a persisted (non-inline) alias. """ # Fold shell line-continuations so a multi-line Bash invocation, such as `gh pr merge 5 \ --admin`, parses as one command. # Only backslash-newline is joined, so a real newline between commands still separates them. @@ -913,6 +1638,12 @@ def classify(cmd, cwd=None, origin=None, current_branch=None, rules_lookup=None, dec, reason = _check_push_bypass(cmd, cwd, origin, current_branch, rules_lookup) if dec == "deny": return dec, reason + # Rule 6 covers a mutating git operation against a primary checkout, also not a GitHub write, so it is checked here too, before the gh-write gate below would otherwise skip past it. + dec, reason = _check_primary_checkout_mutation( + cmd, cwd, environ, primary_checkout_lookup, ref_resolver, config_lookup + ) + if dec == "deny": + return dec, reason if not _is_gh_write(cmd): return "allow", "" @@ -1803,12 +2534,633 @@ def classify(cmd, cwd=None, origin=None, current_branch=None, rules_lookup=None, ), ] +# (command, cwd, {dir: is_primary_or_None}, {(dir, ref): resolves_as_ref} or None, expected_decision, label) -- is_primary is True (a primary checkout), False (a linked worktree), or None (unresolved, for example when no repository exists or the git query itself fails, such as on a pre-2.31 git lacking `rev-parse --path-format`). +# A None ref-map means every ref-check in the case resolves True (an ordinary branch name), the common case; only the pathspec-disambiguation cases below need a real map. +_PRIMARY_CHECKOUT_CASES = [ + ( + "git reset --hard origin/main", + "/primary", + {"/primary": True}, + None, + "deny", + "a git reset --hard in a primary checkout is exactly the GOVERNANCE.md-named harm", + ), + ( + "git reset --hard origin/main", + "/worktree", + {"/worktree": False}, + None, + "allow", + "the same command in a linked worktree is allowed", + ), + ( + "git checkout main", + "/primary", + {"/primary": True}, + {("/primary", "main"): True}, + "allow", + ( + "a flagless checkout of a real ref is exempt even in a primary checkout, an accepted " + "scope boundary: the #1073 incident's own literal commands (checkout, then --ff-only " + "pull) are this exact shape, and the concurrent-access hazard they still carried is " + "the prose rule's job, not this mechanically decidable one's" + ), + ), + ( + "git checkout .", + "/primary", + {"/primary": True}, + {("/primary", "."): False}, + "deny", + ( + "a single bare argument that does not resolve as a ref falls back to git's own " + "pathspec-restore path, which carries none of the ref-switch safety check the " + "flagless exemption relies on" + ), + ), + ( + "git checkout -- .", + "/primary", + {"/primary": True}, + None, + "deny", + "-- unambiguously means every following argument is a pathspec, denied with no ref check", + ), + ( + "git checkout HEAD -- src/", + "/primary", + {"/primary": True}, + None, + "deny", + "a ref plus -- pathspec is still the unconditional pathspec-restore form", + ), + ( + "git checkout main src/", + "/primary", + {"/primary": True}, + None, + "deny", + "two positional arguments with no -- is the same ambiguous/pathspec-leaning shape, denied rather than guessed at", + ), + ( + "git switch feature/x", + "/primary", + {"/primary": True}, + {("/primary", "feature/x"): True}, + "allow", + "switch gets the same ref-verified flagless exemption as checkout", + ), + ( + "git switch -C other", + "/primary", + {"/primary": True}, + None, + "deny", + ( + "switch -C force-resets an existing branch to the current HEAD with no dirty-tree " + "warning at all, empirically confirmed; -c/-C are switch's own create/force-create " + "spellings, distinct from checkout's -b/-B, and neither subcommand has any other flag " + "using those letters" + ), + ), + ( + "git switch -c newbranch", + "/primary", + {"/primary": True}, + None, + "deny", + "switch -c creates a new branch, the same branch-creating class as checkout -b", + ), + ( + "git pull", + "/primary", + {"/primary": True}, + None, + "deny", + "a bare pull in a primary checkout is denied", + ), + ( + "git pull --ff-only", + "/primary", + {"/primary": True}, + None, + "allow", + "--ff-only can never discard anything, exempt even in a primary checkout", + ), + ( + "git merge --ff-only origin/develop", + "/primary", + {"/primary": True}, + None, + "allow", + "the documented base-clone cleanup step (repo-worktree 'Listing and Cleanup')", + ), + ( + "git worktree add ../x origin/develop", + "/primary", + {"/primary": True}, + None, + "allow", + "creating a worktree from the primary checkout is the documented, intended use", + ), + ( + "git fetch origin develop", + "/primary", + {"/primary": True}, + None, + "allow", + "a read is never denied, even in a primary checkout", + ), + ( + "git status", + "/primary", + {"/primary": True}, + None, + "allow", + "status is not even a subcommand this rule inspects", + ), + ( + "git commit -m x", + "/primary", + {"/primary": True}, + None, + "deny", + "commit has no flag-based exemption, denied unconditionally in a primary checkout", + ), + ( + "git add -A", + "/primary", + {"/primary": True}, + None, + "deny", + "a blanket add in a primary checkout is exactly the #1073 incident class", + ), + ( + "git rm -rf .", + "/primary", + {"/primary": True}, + None, + "deny", + "git rm deletes working-tree files exactly as unconditionally as add/commit mutate them", + ), + ( + "git mv a.txt b.txt", + "/primary", + {"/primary": True}, + None, + "deny", + "git mv renames a tracked file and stages the change exactly as unconditionally as rm deletes one", + ), + ( + "git apply patch.diff", + "/primary", + {"/primary": True}, + None, + "deny", + "git apply mutates the working tree", + ), + ( + "git am 0001-fix.patch", + "/primary", + {"/primary": True}, + None, + "deny", + "git am mutates the working tree", + ), + ( + "git push origin feature/x", + "/primary", + {"/primary": True}, + None, + "deny", + ( + "a push does not mutate the local working tree the way the rest of this rule's ops " + "do, but no documented fleet workflow ever pushes from a primary checkout, so it is " + "denied unconditionally there too, independent of rule 4's own branch-rule checks" + ), + ), + ( + "git push origin feature/x", + "/worktree", + {"/worktree": False}, + None, + "allow", + "the same push from a linked worktree, where every documented push actually happens, is allowed", + ), + ( + "git stash", + "/primary", + {"/primary": True}, + None, + "deny", + "bare stash mutates the working tree exactly as push/pop/apply/drop do", + ), + ( + "git stash push -m wip", + "/primary", + {"/primary": True}, + None, + "deny", + "stash push is the same mutation as bare stash, spelled out", + ), + ( + "git stash list", + "/primary", + {"/primary": True}, + None, + "allow", + "stash list only reads the stash, denying it would add no safety", + ), + ( + "git clean -fd", + "/primary", + {"/primary": True}, + None, + "deny", + "an ordinary forced clean deletes untracked files/directories, the harm this rule guards against", + ), + ( + "git clean -nfd", + "/primary", + {"/primary": True}, + None, + "allow", + ( + "-n/--dry-run always wins over -f/--force, confirmed live regardless of order: " + "-nfd deletes nothing, only previews what a later -fd would remove, so denying it " + "would add no safety while breaking a genuinely harmless preview" + ), + ), + ( + "git clean --dry-run --force", + "/primary", + {"/primary": True}, + None, + "allow", + "the long-flag spelling of the same dry-run-wins-over-force exemption", + ), + ( + "git clean -f -- -n", + "/primary", + {"/primary": True}, + None, + "deny", + ( + "-n after -- is an unconditional pathspec (a file literally named -n), not the " + "--dry-run flag, confirmed live: this deletes that file despite the -n-shaped token, " + "so scanning for a dry-run flag anywhere in args rather than only before -- would " + "have wrongly exempted a real, forced deletion" + ), + ), + ( + "git clean -- -f", + "/primary", + {"/primary": True}, + None, + "allow", + ( + "-f after -- is a pathspec (a file literally named -f), not a real force flag, so " + "with no actual -f/--force before --, git itself refuses to run at all" + ), + ), + ( + "git checkout -b feature/x", + "/primary", + {"/primary": True}, + None, + "deny", + "checkout -b is branch-creating, not the exempt ref-switch form", + ), + ( + "git checkout --detach main", + "/primary", + {"/primary": True}, + {("/primary", "main"): True}, + "allow", + ( + "a non-force flag alongside a real ref stays exempt too: --detach changes nothing " + "about git's own overwrite-refusal on a dirty tracked file, empirically confirmed, so " + "the exemption is a real-ref-with-no-force-flag test, not a strictly zero-flags one" + ), + ), + ( + "git checkout -qf other", + "/primary", + {"/primary": True}, + {("/primary", "other"): True}, + "deny", + ( + "-qf bundles -q (quiet) and -f (force) into one short-option cluster; an exact-token " + "check never sees a bare -f to match, but real git still forces the checkout through, " + "empirically confirmed to discard a dirty tracked file" + ), + ), + ( + "git checkout -Bnewbranch", + "/primary", + {"/primary": True}, + None, + "deny", + ( + "-Bnewbranch attaches -B's mandatory branch-name value with no space, the same " + "force-creating operation as -B newbranch as two tokens, empirically confirmed to work" + ), + ), + ( + "git checkout -qt main", + "/primary", + {"/primary": True}, + {("/primary", "main"): True}, + "allow", + "a bundled cluster with no b/B/f character (-q quiet, -t track) is not a force flag", + ), + ( + "git worktree remove --force ../x", + "/primary", + {"/primary": True}, + None, + "deny", + "a forced worktree remove reproduces the harm this rule guards against", + ), + ( + "git worktree remove -ff ../x", + "/primary", + {"/primary": True}, + None, + "deny", + ( + "-ff bundles force twice, exactly the -f -f git itself requires to remove a locked " + "worktree, confirmed live to forcibly remove one with uncommitted content, which an " + "exact-token check alone misses since remove has no other short option -f could " + "combine with" + ), + ), + ( + "git worktree remove -- -f", + "/primary", + {"/primary": True}, + None, + "allow", + ( + "-f after -- is a worktree path argument, not a force flag, confirmed live: git " + "reads it as a literal worktree name (erroring since none is named that) rather than " + "forcing anything, the same -- cutoff clean's own force scan already applies" + ), + ), + ( + "git worktree remove ../x", + "/primary", + {"/primary": True}, + None, + "allow", + "an unforced worktree remove is exempt: git itself refuses one carrying local changes", + ), + ( + "git -C /worktree reset --hard origin/main", + "/primary", + {"/primary": True, "/worktree": False}, + None, + "allow", + "-C overrides cwd: a worktree named explicitly is allowed even though cwd is the primary", + ), + ( + "git -C /primary reset --hard origin/main", + "/worktree", + {"/primary": True, "/worktree": False}, + None, + "deny", + "-C overrides cwd the other way: the primary named explicitly is denied from a worktree", + ), + ( + "git --work-tree /primary reset --hard", + "/worktree", + {"/primary": True, "/worktree": False}, + None, + "deny", + "--work-tree redirects the mutation the same way -C does, and is read the same way", + ), + ( + "GIT_WORK_TREE=/primary GIT_DIR=/primary/.git git reset --hard", + "/worktree", + {"/primary/.git": True, "/worktree": False}, + None, + "deny", + ( + "a GIT_WORK_TREE=/GIT_DIR= prefix in the command's own text redirects the " + "invocation, unlike a real env var the hook process never sees; the primary-checkout " + "test itself keys on the resolved GIT_DIR= value, since that is what --git-dir/GIT_DIR " + "name for repository identity" + ), + ), + ( + "export GIT_DIR=/primary/.git GIT_WORK_TREE=/primary && git reset --hard", + "/somewhere-else", + {"/primary/.git": True, "/somewhere-else": False}, + None, + "deny", + ( + "a leading export makes the assignment persist into the following command exactly " + "as a real shell would, confirmed live to discard a tracked local modification with " + "no redirect at all on the git invocation itself -- a shape an inline VAR=x git ... " + "prefix scan alone cannot see, since export and the git invocation are separate " + "commands joined by &&, not one command with a prefix" + ), + ), + ( + "git status && export GIT_DIR=/primary/.git && git reset --hard", + "/somewhere-else", + {"/primary/.git": True, "/somewhere-else": False}, + None, + "allow", + ( + "only a leading export is read, matching the same accepted-gap scope leading cd " + "already has: export here is not the first token of the command (git status is), so " + "it has no effect under this rule's narrow scope even though a real shell would still " + "apply it to the following reset --hard" + ), + ), + ( + "git --git-dir=/primary/.git --work-tree=/primary -C /worktree reset --hard", + "/somewhere-else", + {"/primary/.git": True, "/worktree": False}, + None, + "deny", + ( + "--work-tree wins over -C regardless of argv order for the mutation target message, " + "and the primary-checkout test itself keys on --git-dir's own resolved value: real " + "git mutates /primary here, not /worktree, even though -C is the option nearer the " + "subcommand" + ), + ), + ( + "git -C /a -C /worktree reset --hard origin/main", + "/primary", + {"/a": True, "/worktree": False}, + None, + "allow", + "multiple -C options compose sequentially, the last (absolute) one replacing the running directory outright, matching real git's own repeated -C semantics", + ), + ( + "git -C /repos -C sub/primary reset --hard origin/main", + "/somewhere-else", + {"/repos/sub/primary": True, "/somewhere-else": False}, + None, + "deny", + "a relative -C after an earlier -C resolves against that earlier -C's own (absolute) result, not the session cwd", + ), + ( + "git --git-dir=/primary/.git reset --hard", + "/worktree", + {"/primary/.git": True, "/worktree": False}, + None, + "deny", + ( + "--git-dir alone, with no --work-tree, never relocates the mutation-target message " + "(git's own fallback keeps the working tree at the effective cwd, here the linked " + "worktree), but the primary-checkout test itself always keys on an explicit --git-dir " + "when one is given, confirmed live: it names the repository actually mutated " + "regardless of where the working tree files live" + ), + ), + ( + "cd /primary && git reset --hard origin/main", + "/somewhere-else", + {"/primary": True, "/somewhere-else": False}, + None, + "deny", + "a leading cd resolves the target when the hook's own cwd points elsewhere entirely", + ), + ( + "git status && cd /primary && git reset --hard origin/main", + "/somewhere-else", + {"/primary": True, "/somewhere-else": False}, + None, + "allow", + "only a leading cd is read; one appearing after the first command is the accepted gap", + ), + ( + "bash -c 'cd /primary && git reset --hard origin/main'", + "/somewhere-else", + {"/primary": True, "/somewhere-else": False}, + None, + "deny", + "a bash -c wrapper is expanded the same way the GitHub-write rules already expand one, so it does not hide the mutation from this rule either", + ), + ( + "git fetch origin # refresh the base\ngit reset --hard origin/main", + "/primary", + {"/primary": True}, + None, + "deny", + ( + "a mid-line # is not a comment starter left uncleared on the tokenizer's own shlex " + "instance, confirmed live to silently swallow everything through the next newline " + "and fuse the two lines into one `git fetch` invocation carrying the whole `reset " + "--hard` as extra argv, hiding it from every tokenizer-based rule; a genuine # is " + "now read as an ordinary character rather than a comment, so the newline separator " + "and the second git invocation both survive" + ), + ), + ( + "echo a#b && git -C /primary reset --hard origin/main", + "/somewhere-else", + {"/primary": True, "/somewhere-else": False}, + None, + "deny", + "a literal mid-word # (which real bash never treats as a comment starter either) no longer truncates the rest of the command and hides the mutation after it", + ), + ( + "git -C ~/repos/primary reset --hard origin/main", + "/somewhere-else", + {os.path.expanduser("~/repos/primary"): True, "/somewhere-else": False}, + None, + "deny", + "a ~-prefixed target expands the same way a shell would, since ~/repos/ is the fleet's own documented primary-checkout path convention", + ), + ( + "git -C /opt/$HOMEPATH/primary reset --hard origin/main", + "/somewhere-else", + {"/opt/$HOMEPATH/primary": True, "/somewhere-else": False}, + None, + "deny", + "$HOMEPATH is left unexpanded, matching this rule's own documented fail-open stance for a $VAR it cannot resolve, not misread as a prefix match on $HOME", + ), + ( + "git -C ../primary reset --hard origin/main", + "/repos/worktree-task", + {"/repos/primary": True, "/repos/worktree-task": False}, + None, + "deny", + "a relative -C (../primary) resolves against the session's own cwd (/repos/worktree-task -> /repos/primary), not wherever the hook process's own cwd happens to be", + ), + ( + "cd ../primary && git reset --hard origin/main", + "/repos/worktree-task", + {"/repos/primary": True, "/repos/worktree-task": False}, + None, + "deny", + ( + "a relative leading cd (../primary) is joined against the session's own cwd exactly " + "like a relative -C is, not left unjoined and resolved against wherever the hook " + "process's own OS-level cwd happens to be" + ), + ), + ( + "git reset --hard origin/main", + "/primary/.git", + {"/primary/.git": True}, + None, + "deny", + "cwd inside .git itself still resolves as the primary checkout", + ), + ( + "git reset --hard origin/main", + "/not-a-repo", + {"/not-a-repo": None}, + None, + "allow", + "an unresolvable target fails open, precision over recall like every rule but 4", + ), + ( + "git --git-dir=/primary/.git --work-tree=/safe commit --allow-empty -m probe", + "/safe", + {"/primary/.git": True, "/safe": None}, + None, + "deny", + ( + "the CodeRabbit-reported gap: --work-tree names a directory that resolves as no git " + "repository at all, but real git still mutates the repository --git-dir names -- " + "confirmed live with the exact reproduction script CodeRabbit supplied -- so keying " + "the primary-checkout test on --git-dir rather than the resolved --work-tree value is " + "what catches this instead of failing open on the unresolvable work-tree" + ), + ), + ( + "git --work-tree=/other-checkout reset --hard HEAD", + "/primary", + {"/primary": True, "/other-checkout": False}, + None, + "deny", + ( + "the mirror gap a local-strict-review pass found: --work-tree given with no " + "--git-dir resolves as a linked worktree (or unresolvable), but real git still " + "discovers the repository from the effective cwd with no --git-dir override, " + "confirmed live to move the primary's own branch pointer back a commit and discard " + "its own staged index entry even though the working-tree-file side effects land in " + "the other checkout -- the identity dimension (effective cwd) catches this even " + "though the file dimension (the resolved --work-tree value) alone would not" + ), + ), +] + def _selftest(): # A deterministic offline run, pinning origin to ptr727/PlexCleaner, the incident repo, so the cross-origin case resolves without touching a real checkout. # The gh-write cases inject empty rules and a feature current-branch so no case reaches the live branch-rules query. origin = ("ptr727", "plexcleaner") ok = True + # Every existing loop below pins primary_checkout_lookup to a constant False (never a primary checkout), so rule 6 stays inert for every case that predates it. + # Without this, a mutating subcommand incidental to a case testing a different rule (git commit, in a few of them) would fall through to the real _is_primary_checkout and resolve against wherever the self-test process actually runs, which is a primary checkout in CI, silently changing what those cases test. for cmd, want, label in _CASES: got, _ = classify( cmd, @@ -1816,6 +3168,7 @@ def _selftest(): current_branch="feature/x", rules_lookup=lambda br: set(), environ={}, + primary_checkout_lookup=lambda d: False, ) mark = "ok " if got == want else "FAIL" if got != want: @@ -1828,6 +3181,7 @@ def _selftest(): current_branch="feature/x", rules_lookup=lambda br: set(), environ=env, + primary_checkout_lookup=lambda d: False, ) mark = "ok " if got == want else "FAIL" if got != want: @@ -1840,6 +3194,7 @@ def _selftest(): current_branch="feature/x", rules_lookup=lambda br: set(), environ=env, + primary_checkout_lookup=lambda d: False, ) mark = "ok " if got == want else "FAIL" if got != want: @@ -1852,6 +3207,149 @@ def _selftest(): current_branch=cur, rules_lookup=lambda br, _m=rmap: _m.get(br), environ={}, + primary_checkout_lookup=lambda d: False, + ) + mark = "ok " if got == want else "FAIL" + if got != want: + ok = False + print(f" {mark} [{got:5}] want={want:5} {label}") + for cmd, cwd, pmap, refmap, want, label in _PRIMARY_CHECKOUT_CASES: + # A None refmap means every ref-check resolves True (an ordinary branch name). + # A real map defaults a pair it does not name to True too, since only the pathspec-disambiguation cases care about a False answer. + if refmap is None: + ref_resolver = lambda _d, _r: True + else: + ref_resolver = lambda d, r, _m=refmap: _m.get((d, r), True) + got, _ = classify( + cmd, + cwd=cwd, + origin=origin, + current_branch="feature/x", + rules_lookup=lambda br: set(), + environ={}, + primary_checkout_lookup=lambda d, _m=pmap: _m.get(d), + ref_resolver=ref_resolver, + # No persisted alias resolves for any of these cases; only the dedicated alias table below exercises `_config_alias`. + config_lookup=lambda _d, _n: None, + ) + mark = "ok " if got == want else "FAIL" + if got != want: + ok = False + print(f" {mark} [{got:5}] want={want:5} {label}") + # Git-alias resolution (requirement 6, finding 11): (cmd, cwd, pmap, config_map, want, label). + # `config_map` stands in for the target checkout's own persisted `alias.` config; an inline `-c alias.=...` on the command itself needs no seam, it is read straight from the command text. + for cmd, cwd, pmap, config_map, want, label in ( + ( + "git -c alias.wipe='reset --hard' wipe", + "/primary", + {"/primary": True}, + {}, + "deny", + "an inline alias expanding to reset --hard is exactly as denied as spelling reset --hard out directly", + ), + ( + "git wipe", + "/primary", + {"/primary": True}, + {"wipe": "reset --hard"}, + "deny", + "a persisted (non-inline) alias in the target checkout's own config resolves the same way", + ), + ( + "git wipe", + "/worktree", + {"/worktree": False}, + {"wipe": "reset --hard"}, + "allow", + "the same alias in a linked worktree is allowed, rule 6 stays inert there regardless of alias resolution", + ), + ( + "git -c alias.wipe='!rm -rf .' wipe", + "/primary", + {"/primary": True}, + {}, + "deny", + "a !-prefixed shell alias is opaque and denied conservatively rather than executed or allowed through", + ), + ( + "git peek", + "/primary", + {"/primary": True}, + {"peek": "status"}, + "allow", + "an alias expanding to a read-only builtin is allowed, exactly as the builtin itself would be", + ), + ( + "git wipe", + "/primary", + {"/primary": True}, + {"wipe": "alsowipe", "alsowipe": "reset --hard"}, + "deny", + "a chained alias (wipe -> alsowipe -> reset --hard) is followed through more than one hop", + ), + ( + "git nonexistent-alias", + "/primary", + {"/primary": True}, + {}, + "allow", + "a subcommand this rule does not recognize and that resolves to no alias at all falls through to allow, matching the rule's stance for any other unrecognized subcommand", + ), + ( + 'git -c alias.wipe="reset --hard \'unterminated" wipe', + "/primary", + {"/primary": True}, + {}, + "allow", + ( + "a malformed alias expansion (unbalanced quotes) fails to parse as shell words; " + "this is treated as unresolvable rather than raising ValueError and crashing the " + "hook on a config value neither the agent nor this rule controls" + ), + ), + ): + got, _ = classify( + cmd, + cwd=cwd, + origin=origin, + current_branch="feature/x", + rules_lookup=lambda br: set(), + environ={}, + primary_checkout_lookup=lambda d, _m=pmap: _m.get(d), + config_lookup=lambda _d, n, _m=config_map: _m.get(n), + ) + mark = "ok " if got == want else "FAIL" + if got != want: + ok = False + print(f" {mark} [{got:5}] want={want:5} {label}") + # The GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT escape hatch, checked once here rather than folded into the table above, since these are the cases needing a non-empty environ alongside a primary_checkout_lookup. + # "git reset --hard", not a flagless checkout, since checkout is exempt anyway and would pass identically with the grant check deleted, the exact vacuous-test gap a review caught here. + for env, want, label in ( + ( + {"GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT": "1"}, + "allow", + "the escape hatch allows even a denied shape when granted", + ), + ( + {"GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT": "0"}, + "deny", + "a value of 0 reads as not granted, not as any-non-empty-string-is-truthy", + ), + ( + {"GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT": "false"}, + "deny", + "a value of false reads as not granted either", + ), + ({}, "deny", "no grant at all is the ordinary denied case"), + ): + got, _ = classify( + "git reset --hard", + cwd="/primary", + origin=origin, + current_branch="feature/x", + rules_lookup=lambda br: set(), + environ=env, + primary_checkout_lookup=lambda d: {"/primary": True}.get(d), ) mark = "ok " if got == want else "FAIL" if got != want: @@ -1866,12 +3364,19 @@ def _main(): try: data = json.load(sys.stdin) # The hook must never crash on input it does not recognize. - except Exception: # noqa: BLE001 + except Exception: # noqa: BLE001 - malformed JSON on stdin is exactly the "not our event shape" case this guards, not a defect to propagate as a hook-crashing traceback. sys.exit(0) # not our event shape - do not interfere - if data.get("tool_name") != "Bash": + # Valid JSON that is not a dict (a bare string, number, or list), or a `tool_input`/`command` value of the wrong type, is the same "not our event shape" case the JSON-parse guard above already handles, confirmed to otherwise raise AttributeError/TypeError uncaught and exit non-zero rather than the documented deny/allow shape. + # A non-zero exit is a hook error, not a decision, and a PreToolUse hook erroring lets the tool call proceed exactly as if this hook had allowed it, so failing open here on a malformed shape matches what an uncaught crash would do anyway, deliberately rather than by accident. + if not isinstance(data, dict) or data.get("tool_name") != "Bash": sys.exit(0) - cmd = (data.get("tool_input") or {}).get("command", "") + tool_input = data.get("tool_input") + cmd = tool_input.get("command", "") if isinstance(tool_input, dict) else "" + if not isinstance(cmd, str): + cmd = "" cwd = data.get("cwd") or os.getcwd() + if not isinstance(cwd, str): + cwd = os.getcwd() decision, reason = classify(cmd, cwd) if decision == "deny": # Documented PreToolUse deny contract (confirm field names against current docs before shipping). diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/claude/install.ps1 similarity index 100% rename from host-setup/agent-safety/install.ps1 rename to host-setup/agent-safety/claude/install.ps1 diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/claude/install.py similarity index 100% rename from host-setup/agent-safety/install.py rename to host-setup/agent-safety/claude/install.py diff --git a/host-setup/agent-safety/install.sh b/host-setup/agent-safety/claude/install.sh similarity index 100% rename from host-setup/agent-safety/install.sh rename to host-setup/agent-safety/claude/install.sh diff --git a/host-setup/agent-safety/test_install.py b/host-setup/agent-safety/claude/test_install.py similarity index 100% rename from host-setup/agent-safety/test_install.py rename to host-setup/agent-safety/claude/test_install.py diff --git a/host-setup/agent-safety/codex/README.md b/host-setup/agent-safety/codex/README.md new file mode 100644 index 00000000..8eaa16c4 --- /dev/null +++ b/host-setup/agent-safety/codex/README.md @@ -0,0 +1,35 @@ +# Codex Write-Safety Gap + +No hook implements [`../README.md`][spec]'s requirements for Codex yet. Tracked at +[issue #781][issue-781]. + +## What To Keep Enabled Meanwhile + +Keep Codex's own sandbox and execution policies enabled. The carried repository rules +(`GOVERNANCE.md` "Repository Boundaries and Write Safety" and the rest of the fleet's prose) +remain the only behavioral layer in a fleet checkout until a hook exists -- there is no mechanical +backstop for Codex today, which means a mistake that the Claude Code hook would deny goes through +unblocked in a Codex session. + +## Implementing Against the Spec + +[`../README.md`][spec] states each requirement as an agent-agnostic decision rule, not tied to any +one hook API. [`../claude/gh-write-guard.py`][claude-hook] is a worked reference implementation of +all six requirements against Claude Code's `PreToolUse` hook, including its tokenizer, its +self-test matrix (`--selftest`), and its documented fail-open/fail-closed choices per requirement +-- useful as a model for argv parsing and edge cases, not as something to port line for line, since +Codex's own approval/sandbox extension points differ from Claude Code's hook shape. Whatever +mechanism Codex offers for intercepting or gating a command before it runs is the place to +implement requirements 1-6. If Codex offers no such extension point at all, that finding belongs on +[issue #781][issue-781], not silently worked around. + +## Auditing + +Once a Codex-side implementation exists, audit it the way [`../README.md`][spec] "Auditing an +Implementation Against This Spec" describes: run its own self-test and check every case against the +six requirements, not against `gh-write-guard.py`'s source. + + +[spec]: ../README.md +[claude-hook]: ../claude/gh-write-guard.py +[issue-781]: https://github.com/ptr727/ProjectTemplate/issues/781 diff --git a/host-setup/agent-safety/opencode/README.md b/host-setup/agent-safety/opencode/README.md new file mode 100644 index 00000000..9644a126 --- /dev/null +++ b/host-setup/agent-safety/opencode/README.md @@ -0,0 +1,35 @@ +# opencode Write-Safety Gap + +No hook implements [`../README.md`][spec]'s requirements for opencode yet. Tracked at +[issue #781][issue-781]. + +## What To Keep Enabled Meanwhile + +Keep opencode's own permission model enabled. The carried repository rules (`GOVERNANCE.md` +"Repository Boundaries and Write Safety" and the rest of the fleet's prose) remain the only +behavioral layer in a fleet checkout until a hook exists -- there is no mechanical backstop for +opencode today, which means a mistake that the Claude Code hook would deny goes through unblocked +in an opencode session. + +## Implementing Against the Spec + +[`../README.md`][spec] states each requirement as an agent-agnostic decision rule, not tied to any +one hook API. [`../claude/gh-write-guard.py`][claude-hook] is a worked reference implementation of +all six requirements against Claude Code's `PreToolUse` hook, including its tokenizer, its +self-test matrix (`--selftest`), and its documented fail-open/fail-closed choices per requirement +-- useful as a model for argv parsing and edge cases, not as something to port line for line, since +opencode's own permission-model extension points differ from Claude Code's hook shape. Whatever +mechanism opencode offers for intercepting or gating a command before it runs is the place to +implement requirements 1-6. If opencode offers no such extension point at all, that finding belongs +on [issue #781][issue-781], not silently worked around. + +## Auditing + +Once an opencode-side implementation exists, audit it the way [`../README.md`][spec] "Auditing an +Implementation Against This Spec" describes: run its own self-test and check every case against the +six requirements, not against `gh-write-guard.py`'s source. + + +[spec]: ../README.md +[claude-hook]: ../claude/gh-write-guard.py +[issue-781]: https://github.com/ptr727/ProjectTemplate/issues/781 diff --git a/host-setup/menu.ps1 b/host-setup/menu.ps1 index 4d4c14b8..e299d885 100644 --- a/host-setup/menu.ps1 +++ b/host-setup/menu.ps1 @@ -182,6 +182,37 @@ function Get-HubMutexName { return "ProjectTemplateHostSetupMenu_$safe" } +function Invoke-WithHubLock { + # The single lock over $script:DIR\hub, shared by every writer (Invoke-FetchHub, Invoke-Cleanup) and every reader (Invoke-HostTool, and each action's Confirm-HubRoot-then-Invoke-HubPython span) alike. + # Unlike menu.sh's flock, which gives readers a genuine shared mode, a named .NET Mutex has no such mode, and building a correct cross-process reader count on top of one (a shared counter, itself needing its own guard) is real complexity for a rarely-hit race in a low-traffic interactive tool. + # Every caller here takes the same exclusive lock instead, trading reader concurrency for a locking scheme simple enough to get right. + # That trade is not always brief: Invoke-HostTool holds this lock for its entire spawned tool run, which can be a long OS package upgrade, so a second session waiting here can wait as long as that run takes, not just for a quick read. + # It still closes the actual TOCTOU: a concurrent Invoke-FetchHub's Remove-Item can no longer land between a reader confirming $HUB_ROOT is fresh and that reader actually using it, since both now hold this same lock for that whole span, not just around the read's own final call. + # ArgumentList is forwarded to the scriptblock positionally (its own param() block names them), rather than relying on the scriptblock closing over the caller's variables directly. + # Passed this way, PSScriptAnalyzer's PSReviewUnusedParameter sees the caller's own parameters referenced at the call site, where a bare closure reads as an unused parameter to it, since the rule does not trace a variable read inside a nested scriptblock back to the enclosing function's own param() block. + param([Parameter(Mandatory)][scriptblock]$ScriptBlock, [object[]]$ArgumentList = @()) + $mutex = New-Object System.Threading.Mutex($false, (Get-HubMutexName)) + $acquired = $false + try { + try { + # A quick non-blocking probe first, so a session that has to wait says so instead of blocking with no output, matching Invoke-Cleanup's own pattern. + if (-not $mutex.WaitOne(0)) { + info "Waiting for another session using $script:DIR\hub..." + $acquired = $mutex.WaitOne() + } else { + $acquired = $true + } + } catch [System.Threading.AbandonedMutexException] { + # A prior holder crashed mid-operation, which leaves whatever it was doing in a bad state rather than the lock itself, so ownership passes to this run. + $acquired = $true + } + & $ScriptBlock @ArgumentList + } finally { + if ($acquired) { $mutex.ReleaseMutex() } + $mutex.Dispose() + } +} + function Invoke-FetchHub { # -DryRun promises to change nothing, and fetching is the one real change this whole script makes to the host. if ($script:DRY_RUN) { @@ -201,7 +232,7 @@ function Invoke-FetchHub { return (Invoke-FetchHubLocked) } finally { # Released here, once the fetch itself finishes, rather than held for the rest of this session: Invoke-InteractiveMenu's loop keeps a session alive well past its one fetch, and holding the lock that long would block every other menu.ps1 sharing this -Dir until this session quits. - # A second session starting its own fetch while this one is still reading the tree it just cloned is the accepted residual race left by that choice, the same one menu.sh's own flock accepts for the same reason. + # A second session starting its own fetch while this one is still reading the tree it just cloned is not a residual race: every read (Invoke-HostTool, and each action's Confirm-HubRoot-then-Invoke-HubPython span) takes this same lock itself, via Invoke-WithHubLock, for its own whole span. if ($acquired) { $mutex.ReleaseMutex() } $mutex.Dispose() } @@ -311,10 +342,14 @@ function Test-DownstreamCheckout { function Invoke-Cleanup { if ($script:KEEP -or -not $script:HUB_FETCHED) { return } if (-not (Test-Path (Get-MarkerPath))) { return } - $hubPath = Join-Path $script:DIR 'hub' - # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it: a suppressed removal failure must not leave a leftover hub with no marker to explain it. - Remove-Item -Recurse -Force $hubPath -ErrorAction SilentlyContinue - if (-not (Test-Path $hubPath)) { Remove-Item -Force (Get-MarkerPath) -ErrorAction SilentlyContinue } + # Same exclusive lock every reader and writer takes, via Invoke-WithHubLock, so this waits out another session still mid-use rather than deleting the tree out from under it. + # No -ArgumentList needed: the scriptblock only reads script-scoped state ($script:DIR, Get-MarkerPath), not a local parameter of this function, so there is nothing for PSReviewUnusedParameter to flag here the way Invoke-HostTool's own $Tool/$Arguments needed forwarding. + Invoke-WithHubLock -ScriptBlock { + $hubPath = Join-Path $script:DIR 'hub' + # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it: a suppressed removal failure must not leave a leftover hub with no marker to explain it. + Remove-Item -Recurse -Force $hubPath -ErrorAction SilentlyContinue + if (-not (Test-Path $hubPath)) { Remove-Item -Force (Get-MarkerPath) -ErrorAction SilentlyContinue } + } } # --- Running a tool --- @@ -323,18 +358,22 @@ function Invoke-Cleanup { # Spawned as its own pwsh process rather than dot-sourced, since every host-setup\windows script ends its own main with exit, which would otherwise end this menu too. function Invoke-HostTool { param([Parameter(Mandatory)][string]$Tool, [Parameter(ValueFromRemainingArguments)][string[]]$Arguments) - if (-not (Confirm-HubRoot)) { return 1 } - $path = Join-Path $script:HUB_ROOT 'host-setup\windows' | Join-Path -ChildPath $Tool - if (-not (Test-Path $path)) { - fail "$script:HUB_ROOT carries no $Tool at host-setup\windows, so this ref is not one to run tasks from" - return 1 - } - $flags = @() - if ($script:ASSUME_YES) { $flags += '-Yes' } - if ($script:DRY_RUN) { $flags += '-DryRun' } - # Out-Host again, for the same reason as the git calls above: bare, this would join the exit code below into one leaked return value. - & $script:PWSH_PATH -NoProfile -ExecutionPolicy Bypass -File $path @Arguments @flags | Out-Host - return $LASTEXITCODE + # The whole body runs inside Invoke-WithHubLock, Confirm-HubRoot included, not just the final invocation, since locking only around the last call would still leave Confirm-HubRoot's own reads unlocked, the same gap menu.sh's equivalent fix closes. + return (Invoke-WithHubLock -ArgumentList @($Tool, $Arguments) -ScriptBlock { + param($Tool, $Arguments) + if (-not (Confirm-HubRoot)) { return 1 } + $path = Join-Path $script:HUB_ROOT 'host-setup\windows' | Join-Path -ChildPath $Tool + if (-not (Test-Path $path)) { + fail "$script:HUB_ROOT carries no $Tool at host-setup\windows, so this ref is not one to run tasks from" + return 1 + } + $flags = @() + if ($script:ASSUME_YES) { $flags += '-Yes' } + if ($script:DRY_RUN) { $flags += '-DryRun' } + # Out-Host again, for the same reason as the git calls above: bare, this would join the exit code below into one leaked return value. + & $script:PWSH_PATH -NoProfile -ExecutionPolicy Bypass -File $path @Arguments @flags | Out-Host + return $LASTEXITCODE + }) } # The first candidate that is a Python 3.7+, the same probe install-skills.ps1 uses: none of py, python3 or python guarantees that version by construction on Windows. @@ -373,15 +412,22 @@ function Invoke-AuditRepo { $default = $default.Split('/')[-1] $name = Read-Host "Repo to audit [$default]" if (-not $name) { $name = $default } - if (-not (Confirm-HubRoot)) { return 1 } - return (Invoke-HubPython -ScriptPath 'spec/audit.py' -Arguments $name) + # Confirm-HubRoot and the Invoke-HubPython call it gates both run inside the same Invoke-WithHubLock span, matching Invoke-HostTool, so nothing reads $HUB_ROOT unlocked. + return (Invoke-WithHubLock { + if (-not (Confirm-HubRoot)) { return 1 } + return (Invoke-HubPython -ScriptPath 'spec/audit.py' -Arguments $name) + }) } function Invoke-CheckSkillsDist { - if (-not (Confirm-HubRoot)) { return 1 } - $rc = Invoke-HubPython -ScriptPath 'scripts/build_dist.py' -Arguments '--check' + $rc = Invoke-WithHubLock { + # 2, not 1: scripts\build_dist.py --check documents 1 as its own "stale" result, and the switch below reads that value as a genuine check outcome. + # A Confirm-HubRoot failure must not collide with it, or a failed confirmation reports as "stale" with exit code 0 instead of the failure it actually is. + if (-not (Confirm-HubRoot)) { return 2 } + return (Invoke-HubPython -ScriptPath 'scripts/build_dist.py' -Arguments '--check') + } # Only 0 (clean) and 1 (stale) are outcomes scripts\build_dist.py --check documents for itself, so only those two read as a check result. - # Anything else is this task failing to run rather than a finding. + # Anything else is this task failing to run rather than a finding, Confirm-HubRoot's own failure (2) included. switch ($rc) { 0 { info 'Every generated Skills distribution matches .agents/skills/'; return 0 } 1 { info 'A generated Skills distribution is stale. This menu does not regenerate it from a fetched checkout, since the result has to be committed in the hub itself.'; return 0 } @@ -406,8 +452,11 @@ function Invoke-CarryAction { $default = $script:DOWNSTREAM_NAME $name = Read-Host "Repo name as cataloged in registry/repos.json [$default]" if (-not $name) { $name = $default } - if (-not (Confirm-HubRoot)) { return 1 } - return (Invoke-HubPython -ScriptPath 'scripts/carry.py' -Arguments $Mode, $name, '--target', $script:DOWNSTREAM_ROOT) + return (Invoke-WithHubLock -ArgumentList @($Mode, $name) -ScriptBlock { + param($Mode, $name) + if (-not (Confirm-HubRoot)) { return 1 } + return (Invoke-HubPython -ScriptPath 'scripts/carry.py' -Arguments $Mode, $name, '--target', $script:DOWNSTREAM_ROOT) + }) } # --- Menu --- diff --git a/host-setup/menu.sh b/host-setup/menu.sh index d18b7f09..69ed63d8 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -23,6 +23,10 @@ HUB_FETCHED=false DOWNSTREAM_ROOT="" DOWNSTREAM_NAME="" +# The reader half of the fetch/use reader-writer lock over $DIR/hub, the writer half being fetch_hub's own exclusive flock, unchanged. +# Global rather than function-local: the fd is opened by hub_read_lock_acquire in one function and closed by hub_read_lock_release in a later, separate call in the same caller, so it has to survive the return in between. +HUB_READ_LOCK_FD="" + # --- Output --- log() { printf '%s\n' "$*"; } @@ -84,27 +88,74 @@ remove_unowned_hub_check() { return 1 } -# The lock lives only here, in the wrapper, so the locked body below can use its ordinary fail/return pattern with no awareness of it. -# A RETURN trap was tried and dropped: bash does not scope one to the function that set it, so it re-fires (against an already-unset local by then) on whatever function returns next, which surfaced as this script's own "unbound variable" crash on the very next return up the call chain. +# Acquires the reader half of the fetch/use lock, held from immediately before ensure_hub_root's own freshness check through the caller's entire use of $HUB_ROOT. +# Scoping it to only a final tool invocation would leave ensure_hub_root's own git reads unlocked, the same interleaving one call frame up from the read this lock exists to close: a concurrent fetch_hub's rm -rf could still land between ensure_hub_root confirming $HUB_ROOT is fresh and the caller actually reading a file under it. +# Shared (flock -s) rather than exclusive, since concurrent reads don't conflict with each other, only with a writer, so this only blocks behind an in-progress fetch_hub or cleanup. +# A no-op in --dry-run: dry-run's own branch in ensure_hub_root never fetches and never reads $DIR/hub (it resolves HUB_ROOT from cwd or fails outright), so there is nothing here for it to protect, and --dry-run must not create $DIR or a lock file where neither existed, since fetching is documented as the one real host change this script makes. +hub_read_lock_acquire() { + [[ $DRY_RUN == true ]] && return 0 + mkdir -p "$DIR" + # Checked explicitly, not left to fail silently into the flock call below: this function is always called as `hub_read_lock_acquire || ...`, and set -e is suspended for a callee's own body in that shape, confirmed live, so an unwritable $DIR would otherwise leave HUB_READ_LOCK_FD unset and surface only flock's own confusing error about a missing fd. + if ! exec {HUB_READ_LOCK_FD}>"$DIR/hub.lock"; then + fail "Could not open $DIR/hub.lock for locking (permission, or the directory is not writable)" + return 1 + fi + # A quick non-blocking probe first, so a session that has to wait says so instead of looking hung: another session's fetch_hub or host_tool can hold the exclusive lock for as long as a full clone or a long OS package upgrade takes. + # A busy lock's own failure is silent (confirmed live), so any stderr this specific call produces is a real error (a missing flock, a bad fd), not contention, and prints as one instead of the misleading "waiting" framing below. + local probe_err + if ! probe_err=$(flock -sn "$HUB_READ_LOCK_FD" 2>&1); then + if [[ -n $probe_err ]]; then + # A real error, not contention: retrying via the blocking call below would not help and risks a second, more confusing failure (or a hang, if the failure mode is one blocking mode does not handle the same way), so this fails immediately instead of falling through to it. + fail "flock -sn reported: $probe_err" + exec {HUB_READ_LOCK_FD}>&- + HUB_READ_LOCK_FD="" + return 1 + fi + info "Waiting for another session using $DIR/hub..." + if ! flock -s "$HUB_READ_LOCK_FD"; then + fail "Could not lock $DIR/hub.lock" + exec {HUB_READ_LOCK_FD}>&- + HUB_READ_LOCK_FD="" + return 1 + fi + fi +} + +# Releases the fd hub_read_lock_acquire opened. +# Safe to call even when acquire failed or was never called, so a caller's cleanup path never has to track whether it actually holds the lock. +hub_read_lock_release() { + [[ -n $HUB_READ_LOCK_FD ]] || return 0 + exec {HUB_READ_LOCK_FD}>&- + HUB_READ_LOCK_FD="" +} + +# `ensure_hub_root` is `fetch_hub`'s only caller, and it only ever runs inside a `hub_read_lock_acquire` span (`host_tool`, `audit_repo`, `check_skills_dist`, `carry_action` all acquire it first), so $HUB_READ_LOCK_FD is always already open here. +# Escalating that same fd from shared to exclusive, rather than opening a second fd on the same lock file, is what makes this safe: flock treats two different fds on one file as independent lock holders even within one process, so a second fd's blocking exclusive wait would deadlock against the first fd's own shared hold forever, verified by reproducing exactly that hang before this fix. +# `flock` changes an already-held fd's own lock type in place with no such deadlock, since the kernel recognizes it as the same holder taking a different mode, not a second competing one. +# The conversion itself is not atomic (flock(2): the shared lock is dropped before the exclusive one is granted), so another session's own conversion could win the gap and clone first, but fetch_hub_locked's own remove_unowned_hub_check re-verifies ownership before removing or recloning, so that race costs a redundant clone at worst, never a torn one. +# Downgraded back to shared once the fetch itself is done, restoring reader concurrency for the rest of the caller's own read-and-use span, rather than holding exclusive (which would still be correct, only more conservative than necessary). fetch_hub() { - # --dry-run promises to change nothing, and fetching is the one real change this whole script makes to the host. + # --dry-run promises to change nothing, and this clone (or re-clone) is the one real change this function itself makes to the host. + # The reader lock (hub_read_lock_acquire) already no-ops under --dry-run, never opening a lock at all, so without this check fetch_hub would instead fail on its own "no reader lock held" internal-error message below. + # This check exists to give the clearer, task-specific diagnostic first. [[ $DRY_RUN == true ]] && { fail "This task needs a fetched hub checkout, and fetching one is itself a change --dry-run does not make. Run without --dry-run, or from inside a hub checkout already on $DEFAULT_REF." return 1 } + [[ -n $HUB_READ_LOCK_FD ]] || { + fail "fetch_hub run with no reader lock held (internal error, expected hub_read_lock_acquire first)" + return 1 + } mkdir -p "$DIR" - # Held for the rest of this fetch, so a second menu.sh sharing this --dir blocks here instead of passing remove_unowned_hub_check and deleting the tree this one is still cloning into. - # Closed unconditionally on the way out, success or failure, rather than left open for the rest of this process: interactive_menu's loop keeps it alive well past this one fetch otherwise, and a lock nothing ever releases blocks every other menu.sh sharing this --dir until this session quits. - local lock_fd rc - exec {lock_fd}>"$DIR/hub.lock" - if ! flock "$lock_fd"; then + if ! flock "$HUB_READ_LOCK_FD"; then fail "Could not lock $DIR/hub.lock" - exec {lock_fd}>&- return 1 fi - rc=0 + local rc=0 fetch_hub_locked || rc=$? - exec {lock_fd}>&- + # Best-effort downgrade. + # Holding exclusive a little longer on failure is safe, just more conservative. + flock -s "$HUB_READ_LOCK_FD" || true return "$rc" } @@ -210,16 +261,52 @@ detect_downstream_root() { cleanup() { [[ $KEEP == true || $HUB_FETCHED == false ]] && return 0 [[ -e "$(marker_path)" ]] || return 0 + # Released first and unconditionally, even though a normal exit has already released it: an interrupt (Ctrl-C) fires this EXIT trap with $HUB_READ_LOCK_FD still open mid-dispatch, and this process's own still-held shared lock would otherwise deadlock the exclusive wait below against itself, reproduced and confirmed before this fix. + # The read that fd was protecting is being abandoned along with the rest of this process, so releasing it here costs nothing. + hub_read_lock_release + # Exclusive, the same writer half fetch_hub takes, so this waits out a concurrent reader still mid-use rather than deleting the tree out from under it. + local lock_fd + # Checked explicitly, the same reasoning hub_read_lock_acquire's own open already applies: cleanup runs from an EXIT trap, not a plain top-level statement, so an unwritable $DIR here would otherwise leave lock_fd unset and surface only flock's own confusing error about a missing fd. + if ! exec {lock_fd}>"$DIR/hub.lock"; then + fail "Could not open $DIR/hub.lock for locking (permission, or the directory is not writable)" + return 1 + fi + local probe_err + if ! probe_err=$(flock -n "$lock_fd" 2>&1); then + if [[ -n $probe_err ]]; then + # A real error, not contention, the same distinction hub_read_lock_acquire's own probe already makes: retrying via the blocking call below would not help and risks a second, more confusing failure. + fail "flock -n reported: $probe_err" + exec {lock_fd}>&- + return 1 + fi + info "Waiting for another session using $DIR/hub..." + flock "$lock_fd" || { + fail "Could not lock $DIR/hub.lock" + exec {lock_fd}>&- + return 1 + } + fi # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it: a suppressed removal failure must not leave a leftover hub with no marker to explain it. rm -rf "$DIR/hub" [[ -e "$DIR/hub" ]] || rm -f "$(marker_path)" + exec {lock_fd}>&- } # --- Running a tool --- +# The reader-lock wrapper, matching fetch_hub's own wrapper/locked-body split. +# The lock lives only here, so host_tool_locked below can use its ordinary fail/return pattern with no awareness of it. +host_tool() { + hub_read_lock_acquire || return 1 + local rc=0 + host_tool_locked "$@" || rc=$? + hub_read_lock_release + return "$rc" +} + # Every host tool runs from inside the hub tree, and this is the only place a path inside it is named, matching bootstrap.sh's run_tool. # Resolves the hub root itself rather than assuming a caller already did: a host task must work standalone, off a downstream checkout, or off no checkout at all, none of which set HUB_ROOT on their own. -host_tool() { +host_tool_locked() { local tool="$1" shift ensure_hub_root || return 1 @@ -238,6 +325,8 @@ host_tool() { # The Python tools under scripts/ and spec/ resolve their own root from __file__ rather than the working directory, so they are called by absolute path from wherever this script runs and need no cd. # Checked here rather than upfront in main, the same reasoning host-setup/linux/install-skills.sh already carries: a host with no interpreter yet can still use every host action, and only the actions that need one name it as their own prerequisite. # The prerequisite failure returns 127, bash's own "command not found" convention, so a caller reading a specific exit code from the tool itself (build_dist.py's 0-clean/1-stale contract) can tell "python3 never ran" apart from "python3 ran and returned 1". +# Callers hold the reader lock themselves, hub_read_lock_acquire before their own ensure_hub_root call, rather than this function taking it. +# Every caller pairs this with ensure_hub_root and the lock has to cover both, so taking it only here would still leave ensure_hub_root's own reads unlocked, the gap this whole reader-half addition exists to close. hub_python() { command -v python3 >/dev/null || { fail "python3 is required for this task. host-setup/linux/install-tools.sh provides it." @@ -248,6 +337,30 @@ hub_python() { python3 "$HUB_ROOT/$script" "$@" } +# The single reader-lock span for a task that needs ensure_hub_root plus one hub_python call, matching host_tool's own wrapper/locked-body split. +# Not reentrant: hub_read_lock_acquire's exec unconditionally overwrites HUB_READ_LOCK_FD, so a nested call would leak the outer fd and silently drop the lock it represented. +# No current caller nests, and this guard is what keeps it that way rather than a rule to remember. +with_hub_read_lock() { + # 2, not 1, for every failure of this wrapper's own preconditions: check_skills_dist's own caller reads 1 as scripts/build_dist.py --check's "stale" result, so a wrapper-level failure must not collide with the wrapped command's own exit code, the same reasoning Invoke-CheckSkillsDist's own Confirm-HubRoot fix already applies on the PowerShell side. + (($# > 0)) || { + fail "with_hub_read_lock called with no command (internal error)" + return 2 + } + [[ -z $HUB_READ_LOCK_FD ]] || { + fail "with_hub_read_lock nested (internal error)" + return 2 + } + hub_read_lock_acquire || return 2 + if ! ensure_hub_root; then + hub_read_lock_release + return 2 + fi + local rc=0 + "$@" || rc=$? + hub_read_lock_release + return "$rc" +} + # --- Actions --- audit_repo() { @@ -256,16 +369,14 @@ audit_repo() { local name read -r -p "Repo to audit [$default]: " name name="${name:-$default}" - ensure_hub_root || return 1 - hub_python spec/audit.py "$name" + with_hub_read_lock hub_python spec/audit.py "$name" } check_skills_dist() { - ensure_hub_root || return 1 local rc=0 - hub_python scripts/build_dist.py --check || rc=$? + with_hub_read_lock hub_python scripts/build_dist.py --check || rc=$? # Only 0 (clean) and 1 (stale) are outcomes scripts/build_dist.py --check documents for itself, so only those two read as a check result. - # Anything else, 127 included, is hub_python or the tool itself failing to run rather than a finding, and is reported as the task error it is. + # Anything else, 127 included, is with_hub_read_lock, hub_python, or the tool itself failing to run rather than a finding, and is reported as the task error it is. case "$rc" in 0) info "Every generated Skills distribution matches .agents/skills/" ;; 1) info "A generated Skills distribution is stale. This menu does not regenerate it from a fetched checkout, since the result has to be committed in the hub itself." ;; @@ -294,8 +405,7 @@ carry_action() { local name read -r -p "Repo name as cataloged in registry/repos.json [$default]: " name name="${name:-$default}" - ensure_hub_root || return 1 - hub_python scripts/carry.py "$mode" "$name" --target "$DOWNSTREAM_ROOT" + with_hub_read_lock hub_python scripts/carry.py "$mode" "$name" --target "$DOWNSTREAM_ROOT" } # --- Menu --- diff --git a/host-setup/windows/README.md b/host-setup/windows/README.md index 2070eeff..702796a9 100644 --- a/host-setup/windows/README.md +++ b/host-setup/windows/README.md @@ -25,9 +25,9 @@ host-setup\windows\setup-wsl.ps1 -Status **winget**, which arrives with App Installer from the Microsoft Store. -**Script execution.** A `git clone` carries no mark of the web, so these run under the default `RemoteSigned` policy. A browser-downloaded zip does carry one, and is blocked until `Unblock-File` clears the mark. The `.\` prefix is required when running a script from the current directory, exactly as it is for [`agent-safety/install.ps1`][agent-safety]. +**Script execution.** A `git clone` carries no mark of the web, so these run under the default `RemoteSigned` policy. A browser-downloaded zip does carry one, and is blocked until `Unblock-File` clears the mark. The `.\` prefix is required when running a script from the current directory, exactly as it is for [`agent-safety/claude/install.ps1`][agent-safety]. -`pwsh -File .\install-tools.ps1` answers the `.\` rule and **not** the policy, which still applies to it: on a marked file under `RemoteSigned` it fails with a `SecurityError` naming the file as unsigned. The form that runs whatever the policy says is `pwsh -ExecutionPolicy Bypass -File .\install-tools.ps1`, which is what [`docs/host-setup.md`][host-setup] already gives for the write-safety installer. Prefer clearing the mark with `Unblock-File` over bypassing, since the bypass covers every script that run touches. +`pwsh -File .\install-tools.ps1` answers the `.\` rule and **not** the policy, which still applies to it: on a marked file under `RemoteSigned` it fails with a `SecurityError` naming the file as unsigned. The form that gets past the mark is `pwsh -ExecutionPolicy Bypass -File .\install-tools.ps1`, the same shape [`agent-safety/claude/README.md`][agent-safety-claude-readme] gives for the write-safety installer -- `-ExecutionPolicy Bypass` sets only the process scope, which a machine- or user-level policy set by Group Policy still overrides. Prefer clearing the mark with `Unblock-File` over bypassing, since the bypass covers every script that the run touches and does not universally guarantee the run. ## Why winget Is the Only Source @@ -148,7 +148,8 @@ The scripts are checked by `PSScriptAnalyzer`, which runs in CI as the peer of t -[agent-safety]: ../agent-safety/install.ps1 +[agent-safety]: ../agent-safety/claude/install.ps1 +[agent-safety-claude-readme]: ../agent-safety/claude/README.md [bootstrap]: ../bootstrap.sh [bootstrap-ps1]: ../bootstrap.ps1 [governance]: ../../GOVERNANCE.md diff --git a/scripts/README.md b/scripts/README.md index f416a0a8..3941be23 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -236,13 +236,13 @@ The tool validates the registry identity, target origin, feature-branch worktree ## `skills_install.py` -Installs the fleet's Skills for the current machine, cross-platform and idempotent, mirroring [`host-setup/agent-safety/install.py`][agent-safety-install]'s shape: `skills_install.sh` and `skills_install.ps1` are thin wrappers that locate a Python 3 interpreter and hand off, so every OS runs one tested code path. Two independent things happen on a run, since the three tools this fleet targets discover skills differently: `.agents/skills/` is materialized (not symlinked) to `$HOME/.agents/skills/`, so Codex and opencode's global scan covers every repo on the machine rather than only the one that happens to be open, and this repo's marketplace is registered with the `claude` CLI (`claude plugin marketplace add`, `claude plugin install`) so Claude Code loads the same content the other two read directly. The marketplace/plugin registration goes through the `claude` CLI's own commands rather than writing its internal `known_marketplaces.json` by hand, because that file's shape is the CLI's state, not a documented contract, and a hand-written copy risks drifting from what the CLI expects on its next release. +Installs the fleet's Skills for the current machine, cross-platform and idempotent, mirroring [`host-setup/agent-safety/claude/install.py`][agent-safety-install]'s shape: `skills_install.sh` and `skills_install.ps1` are thin wrappers that locate a Python 3 interpreter and hand off, so every OS runs one tested code path. Two independent things happen on a run, since the three tools this fleet targets discover skills differently: `.agents/skills/` is materialized (not symlinked) to `$HOME/.agents/skills/`, so Codex and opencode's global scan covers every repo on the machine rather than only the one that happens to be open, and this repo's marketplace is registered with the `claude` CLI (`claude plugin marketplace add`, `claude plugin install`) so Claude Code loads the same content the other two read directly. The marketplace/plugin registration goes through the `claude` CLI's own commands rather than writing its internal `known_marketplaces.json` by hand, because that file's shape is the CLI's state, not a documented contract, and a hand-written copy risks drifting from what the CLI expects on its next release. `--report` reads the stamp a prior run wrote (`$HOME/.agents/skills-install-stamp.json`, naming the hub commit installed) against the current checkout and says whether the machine is current, without installing anything. A repository whose `AGENTS.md` keeps needing a rule restated is usually this: the machine was never installed, or was installed from an older commit. -[agent-safety-install]: ../host-setup/agent-safety/install.py +[agent-safety-install]: ../host-setup/agent-safety/claude/install.py [agents-skills]: ../.agents/skills/README.md [audit]: ../spec/audit.py [copilot-instructions]: ../.github/copilot-instructions.md