fix(tool): enforce isolated-child git safety in the bash tool (mechanism, not prompt text) - #1959
fix(tool): enforce isolated-child git safety in the bash tool (mechanism, not prompt text)#1959wqymi wants to merge 1 commit into
Conversation
All worktrees of a repo share ONE .git/ ref store, so an isolated child session running `git rebase main` or `git checkout main` inside its own worktree writes refs the MAIN checkout is sitting on and can move its HEAD — corrupting the tree the user and other agents are working in. This has happened for real. Until now the rule existed only as prompt text in orchestrator.txt with zero enforcement. Add a cross-branch git guard on the bash tool's existing parsed-command choke point. It fires only for isolated children (Instance.directory under `<data>/worktree/…`), so the orchestrator and ordinary sessions still integrate branches freely. Blocks rebase/merge, branch switches, branch delete/force-move, foreign force-push/delete-push, worktree mutations and direct ref writes; leaves the child's own add/commit/push/status/diff/ stash/file-restore/new-branch work untouched. The error names what was blocked, why, and what to do instead. Also honor an explicit `session grant-approval` on the parent-grant inheritance path: decideAskRouting routes an ordinary background subagent to `inherit`, never to `forward`, so the DB-backed grant was consulted nowhere on a subagent's path — the documented command silently did nothing and the child's external_directory ask failed closed mid-task.
|
Folded into #1741 as 6acd5c2 (cherry-picked cleanly, no conflicts). Verified on the remote by content, not by oid: Reason: this mechanism exists because #1741's prompt-level rule was measured not to hold — Branch retained. |
Why
packages/opencode/src/session/prompt/orchestrator.txt(§ Integrating an isolated child's work) already tells an isolated child that it must operate only in its own worktree and touch only its own branch — nevergit rebase/git merge/git checkouta branch it does not own.That rule had zero enforcement. And it matters at the mechanism level, not the etiquette level: all worktrees of a repository share ONE
.git/ref store (--git-common-dir). A child runninggit rebase mainorgit checkout maininside its own worktree therefore writes refs the MAIN checkout is sitting on and can move that checkout's HEAD, corrupting the working tree the user and other agents are actively using. This has happened for real in this project.A prior investigation concluded that a model-behaviour test can at most sample whether the orchestrator mentions the rule in a dispatched brief, and that the enforceable fix is mechanism-level: reject cross-branch git operations in an isolated child's bash tool. This is that PR.
Where it hooks, and why there
packages/opencode/src/tool/bash.ts— insideexecute, immediately after the command is parsed and beforecollect/ask:One choke point, chosen because:
commands(root)/parts(node)), so every command node in a pipeline, an&&chain or a subshell is inspected — no second parser, no regex over the raw string.ask(), so a rejected command never prompts the user and never spawns a process.Instance.directory, not on the command'scwd: a child thatcds into the main checkout and rebases there is precisely the accident this exists to stop, and keying oncwdwould silently disable the guard for it.Isolation signal
Instance.directoryunder the app-managed worktree rootpath.join(Global.Path.data, "worktree").session create --isolatecreates the child's worktree at<data>/worktree/<projectID>/<name>(src/worktree/index.tsmakeWorktreeInfo), andsrc/tool/session.tsspawns the peer withcwd: effectiveDir= that path;spawn.tsbinds it as the child fiber'sInstanceRef, whichInstance.directoryreads.Worktree.get(sessionID), noisolatedfield onSession.Info), so directory containment is the reliable signal available synchronously at bash-execution time. Same trusted-root test already used bysrc/tool/external-directory.ts.--isolatethat degraded to shared (non-git dir) also evaluates false, correctly.One wrinkle worth flagging for future callers of this pattern:
Instance.providestores a realpath-resolved directory, so a raw string-prefix test misses on a symlinked prefix (macOS/var→/private/var).isIsolatedWorktreecompares realpaths as a fallback. The existingexternal-directory.tscheck has the same latent fragility.The child's own branch is read without spawning git:
<worktree>/.git→gitdir:pointer →HEAD→ref: refs/heads/<branch>. Best-effort;undefinedon a detached HEAD or unreadable layout, in which case the ownership-sensitive rules fail closed (those forms are destructive and a child never needs them).Blocked
git rebase …git merge …git checkout <branch>/git switch <branch>/git checkout -git checkout --detach/git switch --detachgit checkout -B <other>/git switch -C <other>git branch -d/-D/--delete/-f/--force/-m/-M/--move <other>git push --force/-f/+refspecto a non-own branchgit push --delete/:refspecfor a non-own branchgit worktree add/remove/move/prune/repair/lock/unlockgit update-ref(non-own ref),git symbolic-ref <name> <ref>Leading git global options are skipped before the subcommand is read, so
git --no-pager rebase main,git -c core.pager=cat merge mainandgit -C /elsewhere checkout mainare all still seen.Allowed (regression-tested, because a false block breaks real work)
git add,git commit(incl.--amend),git status,git diff,git log,git show,git fetch,git stash/stash pop,git restore,git push/git push origin HEAD/git push -u origin <own>/git push --force origin <own>/--force-with-lease <own>,git checkout -b <new>/git switch -c <new>,git checkout <own-branch>,git checkout -B <own-branch>,git checkout -- <file>,git checkout .,git checkout <existing-path>,git checkout HEAD -- <file>,git rebase --abort/--continue,git merge --abort,git worktree list,git branch,git branch --show-current,git symbolic-ref HEAD.git checkout <arg>is disambiguated by whether<arg>exists on disk, plus two structural facts that mean git cannot be switching branches: a--separator, or 2+ positionals (tree-ish + paths).Deliberately NOT blocked
git reset(incl.--hard) — it can only move the current worktree's own branch, so it structurally cannot corrupt another checkout's HEAD. Its destructive form is already behind the forced-askbash_deleteprompt inbash.ts. A rule here would be theatre.git pull— only fast-forwards/merges into the child's own branch; never writes another branch's ref.Honest limitation: this is not a sandbox
A string/argv-level check over the parsed AST cannot stop a determined command.
eval,sh -c "$(printf …)", a wrapper script, a repo-configured git alias, or$GIT_DIRenv tricks will all evade it. (git -Cand leading global options are handled, but that is opportunistic, not a guarantee.)The goal is explicitly narrower and worth stating plainly: stop the accidental HEAD-moving mistake that actually happens, and when it is stopped, say clearly what to do instead. A hostile process is out of scope, and pretending otherwise would be worse than nothing.
Error output
Second, separate fix in here:
grant-approvalnever reached background subagentsWhile investigating a related report (an
external_directorygrant "lapsing" mid-task and killing a subagent'sbash/read/editaccess to a worktree outright), I found a real gap on currentmain:session grant-approval <child|all>writesforwardRef.setGrant, which is write-through to SQLite precisely so it survives restarts and separate-process children.forwardRef.grantAllowedis consulted in exactly one place: theinput.forwardbranch ofPermission.ask(src/permission/index.ts).decideAskRouting(src/agent/config.ts) routes an ordinary background subagent toinherit, never toforward— only orchestrator peers getforward.Net effect: for a background subagent,
grant-approvalsilently did nothing, even though the tool replies "Future permission asks from child X will be auto-approved." The child's ask fell through to the non-interactive gate and hard-denied. Theinheritpath's own mechanism (forwardRef.getParentGrants) is an in-memory-onlyMap, populated as a side effect of the parent'sask()calls and dropped on session delete — so it is absent for a separate-process/isolated child and for a parent that never asked, which is exactly what "the grant lapsed mid-task" looks like from the child's side.Fix is three lines at the point
main's own design makes the decision — honor the DB-backed explicit grant inside theinheritbranch:Deliberately not a re-application of the older stranded approach (see below): no
grantResolvedparameter, nodecideAskRoutingsignature change, no re-routing of subagents toforward. Deny-precedence is untouched (it sits after the deny loop) and forced-ask (bash_delete) still falls through — both regression-tested.Tests
New:
test/tool/isolated-git-guard.test.ts— 85 tests: every blocked form, every allowed form, global-option smuggling, unknown-own-branch fail-closed behaviour, the isolation signal,ownBranchparsing, and the error text.test/tool/bash-isolated-git-guard.test.ts— 4 end-to-end tests through the realBashTool.execute, proving the guard is actually wired (not just a unit-tested module) and that a non-isolated session is unaffected.test/permission/inherit.test.ts— 5 added tests for the grant fix (incl. no-grant fails closed, foreign-child grant does not leak, forced-ask not auto-approved).Intentional-failure proof (revert only the src change, observe the failure)
bash.ts2 pass / 2 fail—Expected to contain: "Blocked in an isolated child session"/Received: ""violates()neutered to always allow54 pass / 35 fail—expect(received).toBeTruthy()/Received: undefinedacross every blocked formassertIsolatedGitAllowed84 pass / 1 fail— "a NON-isolated session is completely unaffected" fails with the full guard error thrownbash.tsapplies the guard to every sessiongrantAllowedcheck removed from theinheritbranch8 pass / 2 fail—Expected: "Success" / Received: "Failure"Suite results (this branch,
--timeout 120000frompackages/opencode)test/tool test/permission test/agent test/cli→ 1314 pass / 10 skip / 0 fail (1324 tests, 108 files)test/toolalone → 837 tests, 0 fail (748 pre-existing + 89 new)test/cli test/permission→ 417 pass / 0 failbun typecheck→ exit 0Pristine
mainbaselines measured previously with the identical command weretest/cli + test/permission→ 396/0 andtest/tool→ ~745 pass with 0 failures; both failure sets here are still empty, and the test-count deltas are the newly added tests plusmainmoving forward.Investigation: the two PR #1624 rebase content divergences
Closed PR #1624's branch
fix/orchestrator-tui-and-infra-bugshad two commits whose landed twins onmaindiffer in content. Verdicts:1.
c05fe6f92vs landed426543c22— REAL LOSS (178 changed lines → 4), but superseded, so NOT re-applied as-is.The landed twin is comment-only: 2 lines in
src/session/prompt.ts. The stranded commit's actual mechanism — agrantResolvedfield ondecideAskRouting, resolved viaforwardRef.grantAllowed, routing a granted background subagent toforward— plus its 5 unit tests and 2 permission tests were all dropped.git grep grantResolved origin/main→ absent.But
maindid not simply lose the capability: it later implemented a different and broader mechanism, theinheritrouting (decideAskRoutingreturns{ interactive: false, inherit: { parentSessionID } }, consumed by the parent-grant-inheritance branch inPermission.ask), which consults the parent's actual ruleset+approved snapshot with correct two-phase deny precedence. That is a deliberate adaptation and strictly better for path-based grants. Re-applying the old hunks would be a revert in disguise — the signature and the routing model both changed underneath.What genuinely remained lost is the narrower thing above: explicit
grant-approvalnever reaching a background subagent, becausegrantAllowedis only consulted on theforwardpath. That is the residual cause consistent with the reported symptom, and it is fixed here as a small re-expression against currentmainrather than a re-apply. The separate persistence fixa0ced0655→241a6f048is fully onmainand is untouched.2.
b789c100f8vs landede36926f7d— NO LOSS. Pure rebase artifact.The changed lines are byte-identical. The whole diff-of-diffs is blob indices, hunk line offsets (
@@ -458,11→@@ -484,12), and one extra context line (skipPermissions,) that exists onmainbut not on the stranded branch's base. Nothing to do.Left alone deliberately
orchestrator.txtprose — unchanged; the prompt guidance is still correct and now backed by a mechanism.session create --isolate's existing git-safety warning — unchanged.inheritdesign itself, and the in-memory-only nature ofparentGrants— I only added the DB-backed explicit-grant check. Making theparentGrantssnapshot durable/cross-process is a real but larger design question and belongs in its own PR.git reset/git pull— see Deliberately NOT blocked above.