feat(hooks): mechanical git-freeze guard for shared-workspace subagents (#2705) - #2718
Conversation
…ts (#2705) The shared-workspace git-state freeze in AGENTS.md — shared-workspace subagents never run checkout/switch/reset/stash/rebase, only the orchestrator moves HEAD — was enforced by brief prose alone. #2705 asked whether a dispatch-level guard could enforce it without false-positives. It can, on Claude Code. Measured on 2.1.220: the PreToolUse payload carries agent_id/agent_type. Main-thread Bash calls arrive with agent_id: null, agent_type: null; a spawned subagent's arrive with agent_id: "<id>", agent_type: "general-purpose" under the same session_id and the same cwd. That is the orchestrator-vs-subagent discriminator the freeze needs, and nothing else in the payload provides it. The new PreToolUse:Bash handler (priority 2, alongside the other deny-guards) walks a compound command left to right, tracks literal cd/pushd, honours git -C, and denies a frozen subcommand only when `git rev-parse --show-toplevel` of the target directory equals that of the session cwd. A linked worktree resolves to a different top level, so `git worktree add <path>` followed by `git -C <path> switch` — the documented escape hatch — is untouched. Every ambiguity fails OPEN, deliberately: no agent_id (main thread, Codex, a client that drops the field), a non-literal cd/-C target, --git-dir/--work-tree overrides, or an unresolvable working tree all allow. Unlike branch-guard, the freeze has no server-side backstop, so a wrong deny has no escape hatch; the founding incident was an accidental HEAD move, and a guard that catches the literal forms and never fires on a legitimate one beats a guard nobody keeps enabled. It is a guardrail, not a sandbox. Also lifts branch-guard's quote masker into src/hooks/shell-quoting.ts so both Bash classifiers share one implementation. Coverage gap kept explicit: Codex PreToolUse carries no subagent identity, and `genie launch` Warp panes have no hook surface at all (they are worktree-isolated by construction, so exempt). Full assessment per runtime is recorded on #2705.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4142e40c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| continue; | ||
| } | ||
| const invocation = parseGitInvocation(tokens, dir); | ||
| if (invocation && isFrozenInvocation(invocation)) return invocation; |
There was a problem hiding this comment.
When a compound Bash input first runs a frozen command in another worktree and then one in the shared checkout, such as git -C /wt checkout dev && git reset --hard, this return stops at the first invocation. The handler resolves /wt as a different root and allows the entire tool call, so the second command can reset the shared checkout. git -h confirms -C <path> as a supported global form; continue evaluating all frozen invocations before allowing the payload.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (tokens[0] === 'cd' || tokens[0] === 'pushd') { | ||
| dir = joinPath(dir, tokens[1]); |
There was a problem hiding this comment.
When a cd is conditional on an earlier command, this traversal applies it even if the shell skips it; for example, false && cd /wt; git reset --hard leaves the shell in the shared checkout, but the guard records /wt, resolves the reset against the other worktree, and allows it. Preserve the separators and account for whether a directory transition executes instead of treating every split segment as unconditional.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
…st git call Two verifier defects in the freeze guard, one in each direction. A subshell was a false positive. `(cd /other && git switch main)` denied, because the token `(cd` is not `cd`, so the directory change was dropped and the git call was judged against the session cwd — a deny against a command that never touches the shared checkout, in a module whose whole posture is fail-open. Statements now carry their subshell parentheses: a leading `(` opens a directory scope and a trailing `)` closes it, so the `cd` is tracked inside the group and does not leak past it. Both spellings parse, `(cd /x` and `( cd /x`. That accounting only holds if the parentheses counted are the shell's grouping and nothing else, so command and process substitutions are masked first — the stray `)` of a `$(pwd)` would otherwise close a real group early and leak its `cd` outward. They mask to `$`, which the non-literal-path test already rejects, keeping `git -C $(pwd)` failing open as before. The cost is that a frozen call nested inside a substitution is no longer seen; that gap is documented in the module header and pinned by a test. A compound command was a false negative. `git -C /other switch main && git switch dev` was allowed, because the finder returned the first frozen invocation and the guard, finding it outside the shared checkout, stopped — a legitimate lead call shielded a frozen one behind it. Every frozen invocation is now collected and resolved, and the deny is raised for the first one that lands in the shared root. The "no git subprocess unless a frozen subcommand is present" property is preserved, and repeated directories resolve once. 15 regression tests covering both directions; 60 existing tests unchanged.
1 participant
Closes the investigation half of #2705 and lands the guard it asked for.
Verdict: feasible on Claude Code, not portable
#2705 asked whether the shared-workspace git-state freeze can be enforced mechanically at dispatch, and named three false-positive traps: compound commands,
git -C <path>forms targeting a worktree the agent owns, and the orchestrator's own operations.The orchestrator-vs-subagent question — the one the issue flagged as possibly fatal — is answered by the runtime, not by inference. Claude Code's
PreToolUsepayload carriesagent_idandagent_type. Measured empirically on Claude Code 2.1.220 (headless run, hook dumping raw stdin, one main-thread Bash call and one Task-subagent Bash call):agent_idagent_typesession_idcwdnullnullcbc35a0d-…<project>"a59efa9aa4e5c4138""general-purpose"cbc35a0d-…(same)<project>(same)Same session, same cwd, different
agent_id. Nothing else in the payload distinguishes them, and no environment variable does either —CLAUDE_CODE_SESSION_ID/CLAUDE_PIDare identical for both.Per-runtime assessment (full write-up posted on #2705):
PreToolUsepayload has no subagent identity field; the guard cannot distinguish a Codex subagent shell from the Codex orchestrator, so it fails open there and the freeze stays prose-only on Codex.genie launch— no hook surface at all; panes are plain shells. Moot: panes are worktree-isolated by construction and already exempt.What landed
src/hooks/handlers/git-freeze-guard.ts—PreToolUse:Bash, priority 2, registered insrc/hooks/index.tsalongside the existing deny-guards.For a payload that carries
agent_id, it:branch-guard, see below) so a frozen command named inside a-m/--bodyargument is never a match;&&,||,;,|,&, newline and walks the statements left to right, tracking literalcd/pushdand dropping to "unknown" onpopd;gitglobal flags, honouring-C <path>(chained, relative-resolved);checkout,switch,reset,stash,rebase— exempting the read-onlygit stash list/git stash show, and never matchinggit worktree add/remove/prune/list(permitted orchestrator-side plumbing per AGENTS.md);git rev-parse --show-toplevelof the resolved target directory equals that of the sessioncwd.Step 5 is what makes
git -C <owned-worktree>safe: a linked worktree has its own HEAD and its own top level, so it never compares equal to the shared checkout. That also makes the in-session escape hatch work without any special-casing:The deny message cites the AGENTS.md freeze verbatim and lists all three remedies: take a worktree and address it with
git -C, usegenie launch <wish-slug>for a whole group, or sequence the mutation back through the orchestrator.Fail-open, deliberately
Every ambiguity resolves to allow: no
agent_id(main thread, Codex, a client that drops the field), a non-literalcd/-Ctarget ($VAR,~, glob, command substitution, quoted),--git-dir/--work-tree/--namespaceoverrides, or a top level git cannot resolve.This is the opposite of
branch-guard's fail-closed posture and the inversion is the point.branch-guardcan fail closed because server-side branch protection backstops it; the freeze has no backstop, so a wrong deny simply breaks a working agent with no escape hatch. The founding incident was an accidental HEAD move in a shared checkout. A guard that catches the literal common forms and never fires on a legitimate one is worth more than a guard nobody keeps enabled.Stated plainly in the module header: this is a guardrail, not a sandbox. An agent that wants to route around it (
bash -c, a script file,GIT_DIR=, a Pythonsubprocess) trivially can. It closes the accident class, not an adversarial one.Cost is near-zero on the hot path: no subprocess runs unless a frozen subcommand is actually found in the command text — asserted by two tests.
Drive-by
branch-guard's quote masker moved verbatim tosrc/hooks/shell-quoting.ts; both Bash classifiers now import one implementation instead of two copies. No behavior change —branch-guard's existing suite is unmodified and green.Validation
The single full-suite failure is the known pre-existing macOS-only
ui-bridge lifetime > holds zero listening TCP sockets while running(needs Linuxss), unrelated to this change.Coverage: 60 tests spanning all 13 frozen forms, the orchestrator exemption, the three
agent_id-absent paths, 14 allowed non-frozen commands, 6git -Ccases, 9 compound-command cases (including every fail-open trap), 4 quoted-argument cases, and the worktree-scoped-session semantics.Left for the orchestrator to disposition
AGENTS.md is unchanged. Its "Flip conditions" section still lists #2705 as an open investigation whose infeasibility would be evidence toward flip condition (i). The finding is partial feasibility — mechanical on Claude Code, prose-only on Codex — which is a council call, not an engineer call. The per-runtime analysis is posted on #2705 so it can be weighed there.