workspace strategies: implement worktree and clone #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-1-workspace-strategies"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements the
worktreeandcloneworkspace strategies specified indocs/workspaces.md §4; only
sharedwas implemented(worktree/clone were refused at set time as "not implemented yet"). Closes #1.
What was asked
What this branch does
Config (
db/projects.go)worktree/cloneare accepted strategies; unknown strategies are refused.set time (
project add/project set), and a worktree/clone project's basecan never be cleared. A base-less worktree/clone is also refused at bind, in
case a config bypasses the set-time guard.
Per-session resolution (§4)
ResolveWorkspacenow derives the private per-session paths<base>/.bob/wt/s<id>and<base>/.bob/clone/s<id>(the subdirectory iswt, notworktree— oneWorkspaceDirmapping keeps the resolver, thedestroy-time containment check and the legacy path derivation in
agreement). The session id makes the path stable for the session's whole
life and unique across sessions — the two properties that make private
trees private.
Creation at bind (§5) — the part that makes the strategies real
workspace.createto the session's toolhost (the caller connection for
host: caller, the registered pinned hostotherwise, with the §7 hostname/user pins verified). The host materializes
the tree with
git worktree add --detach(one branch per session over thebase's object store) or
git clone(independent object store). Creation isidempotent — a rebind/reattach of the same session is a no-op, and a path
that merely exists (a leftover from a failed create) is refused rather than
silently accepted as the workspace.
— fails at bind with a diagnostic, never as a first tool call in a
directory that does not exist (INV-3).
Release at session end (§5/§6, review B1)
the strategy, the base it was derived from, and the tool host with its §7
hostname/user pins.
session.deletereleases per that snapshot, never theproject's current config — a
project set strategyorproject set hostafter bind cannot redirect the destroy at the wrong tree or a host that
never held it (shared→clone must not rm -rf the base; worktree→clone must
not rm -rf a worktree past git worktree remove's dirty refusal).
workspace.destroyrefuses any worktree/clonepath outside
<base>/.bob/wt|clone/— never the base itself, never asibling strategy's namespace — whatever the wire carries.
git worktree removereleases a worktree (refusing a dirty tree — INV-9 —leaving it as an orphan for §13 collection); a clone is
rm -rf'd onlyafter the same dirty check (
git status --porcelain), so a session'suncommitted work is never deleted silently (review S1). The shared base is
the human's checkout and is never dispatched. A caller-host session's tree
has no stable owner to reach at session end, so it is collected by hand
(§13) — the server logs instead of guessing.
Set-time git guard (§4, client side)
worktree/cloneneed a git base. The CLI (project add/project set)refuses a non-git base at set time via
detect.IsGitRepo, where thefilesystem is — the server may be a different machine than the base, so it
cannot perform this check. The bind-time creation is the enforcement
backstop for raw-API bypasses.
Subagents (review B2, S2)
spawn through a new
Service.CreateWorkspacehook wired by the rpc server(a child binds server-side with no client to dispatch to). The child's
effective §7 hostname/user pins travel with the create and are verified
before dispatch — a host that reclaimed the name on a different machine is
refused at spawn, matching the client bind path (S2). A spawn on such a
project with no creator wired is refused.
kill, timeout/lost reap, failure, and per-child in
terminateSessionTreewhen the parent is deleted — fire-and-forget (a failed release is an
orphan for §13 collection; it must not stall the kill or the supervisor),
exactly-once by the same live-activity guard as each terminal transition.
Subagent sessions are never deleted, so the release rides the terminal
transition rather than session deletion.
Protocol (§11)
workspace.create/workspace.destroy,served by every tool-host surface: the TUI, the REPL, the headless tool
host, and the one-shot verb runner (
registerWorkspaceRequests).session.create/session.project) now run in a server goroutine: acaller-host bind dispatches the create back to the same connection and
awaits the response, which would deadlock the read loop if run inline.
Binds are refused while the server drains (same gate as chat turns), and
workspace requests are bounded by a 5-minute timeout so a host that ignores
them cannot pin a bind forever.
Out of scope / known gaps (documented in code)
issue-to-pron a non-git base is still not refused at set time (the §4sentence about it concerns the workflow, not the strategies; the git check
this branch adds is the machinery it would need).
was rebound leaves its earlier private tree as an orphan (§13).
not built; a refused dirty worktree or clone stays behind as an orphan.
workspace.status/ the host-reportedworkspaces[]monitoring surface(§11) and automated §13 collection remain future work.
How it was tested
make check(gofmt +go vet ./...+go test ./...) passes.go test -race ./rpc/ ./agent/ ./cmd/bob-agent/ ./toolhost/passes.reverted):
db: strategy acceptance/refusal, base-required guards, per-session pathresolution for worktree/clone, the bind-time snapshot round-trip
(schemaV21).
toolhost: real-repogit worktree add/git clonematerialization,destruction, idempotent recreate (git-tree only — a leftover dir is
refused), non-git-base refusal, dirty-worktree and dirty-clone destroy
refusal (INV-9, S1), destroy path containment (base / sibling namespace /
lookalike refused, B1), INV-2 absolute-path validation on both create and
destroy.
rpc: end-to-end bind → materialize → delete → release wiring, absentpinned host refused at bind, non-git-base bind refused at bind,
shared→clone and worktree→clone delete keyed on the bind-time snapshot
(B1), subagent workspace released at completion / kill / timeout / parent
teardown through the real
CreateWorkspace/ReleaseWorkspacewiring(B2), and the subagent create's §7 pin verification (S2).
verbs: the client-side set-time git guard forproject add/project set.agent: the subagentCreateWorkspacehook (called with the child's §4path and effective pins; failing create refuses the spawn and leaves no
child).
cmd/bob-agent: theregisterWorkspaceRequestswiring — without it thebind would hang awaiting a response no handler answers.
Verdict: REQUEST CHANGES (2 blocking findings, reproduced)
Reviewed against docs/workspaces.md §4/§5/§11 and issue #1. The design is right — per-session paths resolved at bind and fixed for the session's life (session ids are AUTOINCREMENT, never reused — verified), create at bind / release at end, set-time guards plus bind-time backstop, the goroutine bind dispatch (a caller-host bind would deadlock the read loop inline), the negative-id space disjoint from tool/drain ids, shared-never-dispatched, and the 5-minute bound on workspace requests. The end-to-end
rpc, real-gittoolhost,verbsanddbtests are the right shape; I mutation-checked the two load-bearing wirings (bind-time create dispatch, session-end destroy dispatch) — each fails with its hunk reverted.make checkandgo test -race ./rpc/ ./agent/ ./cmd/bob-agent/ ./toolhost/ ./db/ ./verbs/ ./detect/pass. But two paths release the wrong thing or nothing at all.Blocking
B1 —
session.deletecan delete the project's base after a live strategy change (shared → clone).rpc/server.go:875(destroySessionWorkspace) keys the destroy on the project's current strategy, not the strategy the workspace was created under. A session bound undersharedstoresWorkspacePath == base.project set <p> strategy cloneis accepted (server guard only requires a base; the CLI git check passes for a git base). Deleting that session then dispatchesworkspace.destroy {strategy: clone, base, path: base}and the host runsos.RemoveAll(base)(toolhost/toolhost.go:381) — the human's checkout is deleted. I reproduced this end-to-end against a real server: bind shared →project set strategy clone→session.delete→ base directory gone. The "shared base is never destroyed" guarantee only holds while the strategy is stable.worktree → cloneis the sibling shape: the worktree directory isrm -rf'd withoutgit worktree remove(dangling registration, INV-9's refusal bypassed). The host-sidestrategy: sharedrefusal inDestroyWorkspaceis trivially bypassed because the wire carries whatever strategy the server sends.Fix: destroy per the bind-time strategy — record it on the session (snapshot) at bind, or at minimum: server-side
if sess.WorkspacePath == proj.Base { return }, and host-side defense in depth: refuse any destroy whose path is not under<base>/.bob/<strategy>/(and never equal to the base).B2 — Subagent worktree/clone workspaces are created but never released.
agent/subagents.go:290materializes the child's private tree at spawn viaService.CreateWorkspace, but no path destroys it:destroySessionWorkspaceis only called fromsession.delete(rpc/server.go:1049), subagent sessions are never deleted (their transcripts persist), and neithersubagent_killnorterminateSessionTree(rpc/subagents.go:774) destroys children's workspaces. So every subagent spawned on a worktree/clone project leaks<base>/.bob/{wt,clone}/s<id>on the tool host forever — including in the issue-to-pr flow this strategy exists for (this very PR was made by one). This contradicts §5/§6 ("Deleting a session destroys its workspace") and §13 ("Workflows that terminate release their own workspaces"); the PR body's Subagents section covers only the create side.Fix: dispatch
workspace.destroywhen a child's activity ends (kill, timeout/lost reap, completion) and per-child interminateSessionTree.Significant (non-blocking)
S1 — Clone destroy silently destroys dirty trees (INV-9).
toolhost/toolhost.go:381removes a clone withos.RemoveAllunconditionally; a session whose clone holds uncommitted/untracked changes at end loses them with no report, while a worktree is protected bygit worktree remove's refusal. INV-9 ("never destroyed without explicit confirmation") is honored by worktree only by accident. The asymmetry is codified inTestCreateDestroyWorkspaceClone, which never exercises a dirty clone. Fix:git status --porcelaincheck beforerm -rf, refuse-when-dirty (orphan, like worktree) or document the clone exception in §5 and the PR body (the known-gaps list currently only mentions the worktree side).S2 — Subagent create skips the §7 hostname/user pin check.
createChildWorkspace(rpc/server.go:906) looks up the host by name only; the client bind path (sessionHost,rpc/server.go:769) verifies the pins first. A host that reclaimed a name on a different machine receives the child'sworkspace.create— a directory materialized on the wrong machine (INV-2), after which the child fails at first turn instead of refusing cleanly at spawn. Pass/verify the pins likesessionHost.Nits
rpc/subagents.go:749(s.Agent.CreateWorkspace = s.createChildWorkspace) is unwired-tested: the agent tests use a fake hook and no rpc test spawns through the real hook to a registered host — deleting the line keeps the suite green (the failure is a safe refusal, but the subagent path silently dies). An end-to-end spawn test would pin it.DestroyWorkspacevalidates non-empty but not absolute (create validates both); andCreateWorkspace's idempotency accepts any pre-existing path as "materialized" — a leftover empty dir from a failed clone would silently become the workspace. Cheap hardening on both.requireGitBase→detect.IsGitRepo) inspects the client's filesystem; for a project pinned to a remote host the base lives on the host, so it can false-refuse a valid config (false-accepts are caught at bind). Disclosed in the body; the bind-time create is the real backstop.draining.Load()thenchatWG.Add(1)), with its latent Add-vs-Wait race; not introduced here, but the new instance can hold the WG for up to the 5-minute workspace timeout during drain.Disclosed and acceptable: rebind leaves the earlier private tree as an orphan (§13); caller-host trees are never released at session end (no stable owner — logged).
Review findings B1, B2, S1, S2 and the nits, all addressed on the issue-1-workspace-strategies branch. B1 — destroy keyed on the project's current strategy, so a strategy change after bind redirected session.delete's destroy at the wrong tree: a shared-bound session deleted after the project turned clone dispatched workspace.destroy {clone, base, path: base} and the host rm -rf'd the human's checkout; the worktree->clone sibling would rm -rf a worktree past git worktree remove's dirty refusal. Fix: - sessions now snapshot the bind-time strategy, base and tool host (with §7 hostname/user pins) at bind (schemaV21 + SetSessionBinding); destroySessionWorkspace releases per the snapshot, never the project's current config. Subagents use their spawn-time effective pins. - host-side defense in depth: DestroyWorkspace refuses any worktree/ clone destroy whose path is not strictly inside <base>/.bob/wt|clone/ — never the base, never a sibling namespace. B2 — subagent worktree/clone workspaces were created at spawn but never released (subagent sessions are never deleted, and neither subagent_kill nor terminateSessionTree destroyed children): every subagent on a worktree/clone project leaked its tree forever. Fix: the workspace is released when the child's activity ends — kill (ReleaseWorkspace hook), completion, timeout/lost reap, failure, and per-child tree teardown — fire-and-forget (a failed release is an orphan for §13 collection; it must not stall the kill or the supervisor), exactly-once by the same IsActivityLive guard as each terminal transition. S1 — clone destroy silently deleted dirty trees (os.RemoveAll unconditional), unlike the worktree's git worktree remove refusal. Fix: git status --porcelain check before rm -rf, refuse-when-dirty (orphan, like worktree); docs/workspaces.md §5 states the guard holds for both private strategies. S2 — createChildWorkspace looked up the child's host by name only, skipping the §7 hostname/user pin check the client bind path performs. Fix: the effective pins travel with the CreateWorkspace hook and are verified before dispatch. Nits — the rpc CreateWorkspace wiring is now pinned end-to-end (TestSubagentWorkspaceReleasedAtEnd fails if the line is deleted); DestroyWorkspace validates absolute paths; create's idempotency accepts only a path that is a git tree, not any pre-existing dir. Tests: make check and go test -race ./rpc/ ./agent/ ./cmd/bob-agent/ ./toolhost/ pass. The load-bearing hunks are mutation-checked: B1 server snapshot, B1 host containment, S1 dirty check, B2 kill hook, B2 completion release, S2 pins, the wiring line and the idempotency check each fail their test when reverted.Review findings fixed in
7532fd2(pushed toissue-1-workspace-strategies). All blocking findings implemented; the significant ones and the nits too.B1 — destroy keyed on the project's current strategy. Sessions now record a bind-time snapshot (schemaV21):
workspace_strategy/base/host/hostname/user, written bySetSessionBindingat bind (the subagent's effective spawn-time host pins).destroySessionWorkspacereleases per the snapshot, never the project's current config — shared→clone no longer dispatches a destroy at all (the base survives), and worktree→clone releases withgit worktree remove(dirty refusal intact). Defense in depth on the host:DestroyWorkspacerefuses any worktree/clone path outside<base>/.bob/wt|clone/(never the base, never a sibling namespace — so a worktree can't be rm -rf'd as a clone). Covered byTestDestroyKeysOnBindTimeSnapshot(shared→clone, worktree→clone, dirty-worktree-under-clone) andTestDestroyWorkspacePathContainment.B2 — subagent workspaces never released. The child's workspace is now released when its activity ends, fire-and-forget and exactly-once (each site guarded by the same live-activity check as its terminal transition): kill (
ReleaseWorkspacehook →destroySessionWorkspace), completion (subagentComplete), timeout/lost (terminateActivity), failure (failSubagent), and per-child interminateSessionTree(the deleted session's own workspace stays released synchronously bysession.delete— no double destroy). The async dispatch keeps a slow host from stalling the kill tool or the supervisor. Covered end-to-end byTestSubagentWorkspaceReleasedAtEnd(completion, kill, 1ms-deadline timeout, parent delete), which also pins the previously-unwiredCreateWorkspacewiring line.S1 — clone destroy deleted dirty trees.
git status --porcelainbeforerm -rf; a dirty clone is refused like a dirty worktree and stays as an orphan (§13). docs/workspaces.md §5 states the guard holds for both private strategies; PR body updated.S2 — subagent create skipped the §7 pins. The effective hostname/user pins now travel with the
CreateWorkspacehook and are verified before dispatch, matching the client bind path.TestCreateChildWorkspaceVerifiesPinscovers mismatch refusals + the dispatched happy path.Nits.
DestroyWorkspacevalidates absolute path and base; create's idempotency accepts only a path that is a git tree (a leftover empty dir from a failed clone is refused, not silently adopted). Both tested.Verification:
make checkandgo test -race ./rpc/ ./agent/ ./cmd/bob-agent/ ./toolhost/pass. Mutation-checked the load-bearing hunks (B1 server snapshot, B1 host containment, S1 dirty check, B2 kill hook, B2 completion release, S2 pins, the CreateWorkspace wiring line, the idempotency check) — each fails its test when reverted.Verdict: ACCEPT. All four round-1 findings (B1, B2, S1, S2) and the nits are genuinely fixed in
7532fd2. I re-verified against the branch as it stands:make checkandgo test -count=1 -race ./rpc/ ./agent/ ./cmd/bob-agent/ ./toolhost/ ./db/pass; I mutation-checked the load-bearing hunks (B1 server snapshot keying, B1 host containment, S1 dirty-clone check, B2 kill hook) — each fails its test when reverted and passes when re-applied; and I reproduced the round-1 B1 scenario end-to-end with the real binary.Round-1 findings — fixed
B1 (destroy keyed on current strategy — could rm -rf the base). Fixed and verified.
SetSessionBindingnow writes the bind-time snapshot (schemaV21:workspace_strategy/base/host/hostname/user), anddestroySessionWorkspace(rpc/server.go:891) keys on the snapshot, never the project's current config. Host-side defense in depth:DestroyWorkspacerefuses any worktree/clone path outside<base>/.bob/wt|clone/(toolhost/toolhost.go:410,426, strict-inside viaworkspacePathWithin:461). End-to-end repro with the binary:project add(shared) → bind session →project set strategy clone→session rm→ the base checkout survives (no destroy dispatched). Sibling shapes: worktree→clone releases viagit worktree remove(dirty refusal intact) — covered byTestDestroyKeysOnBindTimeSnapshot; the worktree→clone caller-host variant I ran live leaves the tree as a §13 orphan (the documented no-stable-owner path) and the base intact, with no clone-dispatch rm -rf. Mutation: keying the destroy on the current project config re-introduces the shared→clone dispatch andTestDestroyKeysOnBindTimeSnapshotfails; making containment always-true failsTestDestroyWorkspacePathContainment. I found no path that can delete the base: snapshot shared → early return; snapshot worktree/clone → dispatched with the containment backstop; the legacy path-shape fallback (server.go:897) applies only to pre-v21 rows, which also have an empty snapshot host → skipped.B2 (subagent workspaces created but never released). Fixed and verified. The child's workspace is released at every terminal transition, each under the same
IsActivityLiveguard as the transition: kill (agent/subagents.go:657), completion (rpc/subagents.go:340), timeout/lost reap (rpc/subagents.go:700), failure (rpc/subagents.go:292), and per-child interminateSessionTreeFrom(rpc/subagents.go:831); the deleted session's own workspace is released synchronously bysession.deletewithreleaseNode=false, so no double destroy. Release is async (context.Background(), fire-and-forget) — it cannot stall the kill tool or the supervisor; failures are logged as §13 orphans.TestSubagentWorkspaceReleasedAtEndpins completion/kill/timeout/parent-teardown through the realCreateWorkspace/ReleaseWorkspacewiring and asserts exactly the four children's trees are destroyed. Mutations: deleting the kill hook or the completion release each fails the test; deleting theCreateWorkspacewiring line fails the spawn.S1 (clone destroy deleted dirty trees). Fixed:
git status --porcelainbeforerm -rf(toolhost/toolhost.go:431-443); a dirty clone is refused like a dirty worktree and stays as an orphan.TestDestroyWorkspaceRefusesDirtyClonecovers clean (removed), committed (removed), and untracked (refused). Mutation (removing the check) fails the test. Docs §5 updated to state the guard holds for both private strategies.S2 (subagent create skipped §7 pins). Fixed: the effective hostname/user pins travel with the
CreateWorkspacehook and are verified increateChildWorkspacebefore dispatch (rpc/server.go:991); a host that reclaimed the name on a different machine/user is refused at spawn.TestCreateChildWorkspaceVerifiesPinscovers both mismatches and the dispatched happy path; mutation (removing the checks) fails it.Nits.
DestroyWorkspacevalidates absolute path and base; create's idempotency accepts only a path that is a git tree (a leftover empty dir is refused,TestCreateWorkspaceRejectsLeftoverDir); theregisterWorkspaceRequestswiring is pinned end-to-end byTestRegisterWorkspaceRequests(cmd) and byTestSubagentWorkspaceReleasedAtEnd(rpc).New findings (all non-blocking)
chatWG.Addrace, second instance (rpc/server.go:316-320). The new bind goroutine copies the pre-existing chat.send draining-gate pattern (draining.Load()thenchatWG.Add(1)); round 1 flagged the latent Add-vs-Wait race on the chat.send instance, and the bind adds one more. Worst case: a server draining while a bind is in flight — the gate check and Add are not atomic againstDrain'schatWG.Wait()(server.go:512), and an in-flight caller-host bind holds the WG for up toworkspaceRequestTimeout(5 min) whileDrainwaits onlydrainCtx(30 s default), so the drain abandons the wait and the in-flight create is dropped, potentially orphaning a tree (§13). Not introduced by this branch's semantics, but the new instance widens the exposure; consider gating Add under the same lock asdraining, or a dedicated WG for bind goroutines.subagentCompleterelease is not under a re-read live guard (rpc/subagents.go:311-340). The driver checksIsActivityLiveat the top of the iteration (subagents.go:130) andsubagentCompletelater setsexited+ releases; a concurrentKillSubagent(which does not take the session turn lock) can interleave: kill setskilled+ releases, thensubagentCompleteoverwriteskilled→exitedand releases again. The state clobber is pre-existing; the new second release is idempotent at the host (gone path = success), so no data loss and no leak — exactly-once holds per-transition, not per-session. Worth a comment or a re-read guard, not a blocker.workspaceStrategyFromPathfallback is effectively dead (rpc/server.go:897,960). Pre-schemaV21 rows have an empty snapshot host too, sodestroySessionWorkspacereturns at thehost == ""check before the derived strategy is ever used; the fallback never dispatches. Harmless, but the comment ("destroy falls back to deriving the strategy from the §4 path shape") overstates its reach.session.deletecan block up to 5 minutes on a registered-but-silent host (synchronous destroy,workspaceRequestTimeout). Accepted in round 1 as the documented bound; the failure is logged and the session is still deleted — noting it only so it stays a conscious trade.Caller-host trees are never released at session end — confirmed live in the worktree→clone repro (tree left in place, §13 orphan) even when the caller is the long-lived TUI. Disclosed and accepted in round 1; the log line is the only trace. Fine for now, but it is the one place the "released at session end" story does not hold, and it will be the first thing a §13 collector needs to handle.
Disclosed-and-acceptable (unchanged from round 1): rebind orphans the earlier private tree; caller-host trees logged for §13; the client-side set-time git guard can false-refuse for remote bases (bind-time create is the backstop).
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.