Repository navigation
The connector runs where it is started: no worktrees - #761
Merged
Merged
Conversation
The connector should not care where work happens. It runs where it is started, and the agent decides the rest — from its skills, its CLAUDE.md, its memory. A task that needs a clone or a directory of its own makes one. Sandboxing is being built separately and is where containment belongs. Worktrees existed so two tasks on one repository could run side by side. That is a concurrency answer, and concurrency is not what they are being kept for: two workers in one directory is managed by the people running them until boxes are real. What goes, in whole files: worktrees.go, ledger_worktrees.go, connect_worktrees.go and their tests — about 5,400 lines — plus the references in twenty-odd others. `connect worktrees list` and `prune` go with them, and .surface records the removal. Worktrees turned out to be the only implementation of the Workspaces seam, so the seam went too: PerTaskWorkspaces and WaitingWorkspaces are worktree concepts by name, and RecoveringWorkspaces existed to reconcile worktrees on start. A task's working directory is its route, as it already was whenever worktrees were off, and the dispatcher prepares nothing. That cascaded once more. Nothing can now raise ErrRouteUnusable — only a worktree that could not be made ever did — so Ledger.RefuseStart, the StartRefused hook, ReasonRouteUnusable, refusedStartKey and renderRefusedStart are all unreachable and go with it. A route directory that is missing or unreadable is unchanged from the worktrees-off path it already took: the spawn fails, is retried once, and the record is blocked spawn_failed with a notice on the card. The lifecycle map keeps its admitted→blocked edges; shipped transition rules are not edited for a caller that left. The ledger keeps migration 9, byte for byte. A migration that has been applied is never taken out of the list — removing it would change what every later one means, and a fresh ledger has to walk the same numbers — so migration 12 drops the table instead. The directories are not touched: a worktree the connector kept is still on disk and still git's, and `git worktree remove` still removes it. connect.json keeps a read-and-discard LegacyWorktrees field. **Do not tidy it away.** Parse uses DisallowUnknownFields, so an unknown key is a refusal; every connect.json ever written carries "worktrees", and deleting the field outright would stop every connector that has been set up. It is zeroed after the decode and omitempty keeps it out of anything written, so the next `connect setup` takes the key out for good.
The rule is about a task's working directory; it no longer has a worktree to also be about. The setup test's repository route existed only so a rerun could turn --worktrees on.
Prose, no behaviour. Both files are in directories other agents are working in, so this is a commit of its own and can be dropped whole. The codex driver's point about its sandbox stands — it is policy, not containment — but the consequence it drew was about a worktree's git data. The acp adapter's read seam is still there and still nil-able; nothing passes it a reader now that doctor reads the route itself.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It removes a large stateful subsystem, and legacy refusal-intent compatibility still needs correction.
Pull request overview
Removes connector-managed Git worktrees so tasks run directly in their routed directories.
Changes:
- Removes worktree creation, recovery, status, pruning, and refusal paths.
- Preserves legacy configuration parsing and drops obsolete ledger data through migration 12.
- Updates tests, skills, command metadata, and public surface snapshots.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Documents route-directory execution. |
| skills/basecamp-connect/SKILL.md | Removes worktree setup guidance. |
| skill-evals/cases/basecamp-connect/route-quoting.yml | Updates mocked connector output. |
| skill-evals/cases/basecamp-connect/not-ready-agent-reads.yml | Updates mocked connector output. |
| internal/connector/worktrees_route_unix_test.go | Removes worktree route tests. |
| internal/connector/worktrees_route_test.go | Removes worktree route tests. |
| internal/connector/setup/route_repository_test.go | Removes repository checks. |
| internal/connector/setup/file.go | Accepts and discards legacy settings. |
| internal/connector/setup/file_test.go | Tests legacy configuration compatibility. |
| internal/connector/setup/checks.go | Removes worktree route validation. |
| internal/connector/setup/apply.go | Removes worktree configuration changes. |
| internal/connector/setup/apply_test.go | Updates setup application tests. |
| internal/connector/retraction_test.go | Retains legacy retraction fixtures. |
| internal/connector/recovery_harness_test.go | Removes workspace instrumentation. |
| internal/connector/recovery_dispatch_test.go | Updates recovery assertions. |
| internal/connector/recovery_connector_test.go | Removes harness workspaces. |
| internal/connector/outbox.go | Removes refused-start key handling. |
| internal/connector/outbox_run.go | Simplifies holding-reply claims. |
| internal/connector/operator_status_test.go | Removes worktree status tests. |
| internal/connector/lifecycle.go | Removes refused-start lifecycle handling. |
| internal/connector/ledger.go | Adds the table-removal migration. |
| internal/connector/ledger_worktrees.go | Removes worktree ledger operations. |
| internal/connector/ledger_tasks.go | Removes route-refusal state handling. |
| internal/connector/ledger_status.go | Removes worktree status fields. |
| internal/connector/ledger_migrations_test.go | Tests upgrading an old ledger. |
| internal/connector/ledger_dispatch.go | Updates transition documentation. |
| internal/connector/driver/worker.go | Updates ownership documentation. |
| internal/connector/driver/drivertest/drivertest.go | Updates test-package documentation. |
| internal/connector/driver/codex/codex.go | Clarifies Codex sandbox behavior. |
| internal/connector/driver/acp/adapters.go | Updates preflight documentation. |
| internal/connector/dispatcher.go | Runs tasks directly in routes. |
| internal/connector/dispatcher_test.go | Removes workspace-specific tests. |
| internal/connector/dispatcher_boundary_test.go | Narrows release-point checks. |
| internal/connector/dispatch_route_refusal_test.go | Removes worktree refusal tests. |
| internal/connector/admission/policy.go | Updates configuration documentation. |
| internal/commands/connect.go | Removes worktree commands and flags. |
| internal/commands/connect_worktrees.go | Removes list and prune commands. |
| internal/commands/connect_worktrees_test.go | Removes command tests. |
| internal/commands/connect_setup_test.go | Updates setup scenarios. |
| internal/commands/connect_run.go | Removes workspace initialization. |
| internal/commands/connect_operator.go | Removes worktree status output. |
| internal/commands/connect_operator_test.go | Updates operator and doctor tests. |
| internal/commands/connect_doctor.go | Checks routed directories directly. |
| internal/commands/commands.go | Removes worktrees from the catalog. |
| e2e/smoke/smoke_lifecycle.bats | Removes obsolete smoke entries. |
| .surface-breaking | Records removed public commands and flags. |
| .surface | Removes obsolete CLI surface entries. |
Review details
- Files reviewed: 49/49 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot on #761. An upgraded ledger still holds the holding replies a connector that made worktrees wrote — `holding_reply:refused:event:*`, over records blocked `route_unusable` — and nothing produces that reason now or clears it on a record's behalf. Reading every holding reply as `no_route` got both of those rows wrong. Pending: the claim asked whether the record was still blocked `no_route`, it was blocked `route_unusable`, so the reply was canceled "no longer called for". The record is still blocked and still needs the person that reply was going to ask, and nothing tells them. Sent, then redispatched: worse, and the opposite way round. A redispatch of a blocked record leaves it blocked — it authorizes what blocked it to run again (ledger_decisions.go, `case StateBlocked`) — so the ask is still open when the retraction is claimed. askStillOpen compared against `no_route`, answered "not open", and the retraction went out saying event 1 "is not waiting for it now" to a person it is still waiting for. That is the thing askStillOpen's own comment exists to prevent. holdingReplyReason comes back and both reads go through it: the key says which reason the reply answers. Only reads — nothing writes a refused key now. Same shape as migration 9, which stays in the list byte for byte, and as connect.json's LegacyWorktrees: a row is read as it was written, and what is written now is written the new way. Both states are tested against a real upgraded ledger, and each fails without the mapping — pending canceled with nothing posted, sent posting its retraction over a record still blocked.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The broad runtime removal, destructive schema migration, compatibility handling, and public CLI changes warrant final human validation.
Review details
- Files reviewed: 49/49 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The connector cared where work happened. It routed a project to a directory, then made a git worktree of that directory for every task, branched it, tracked it in the ledger, kept it when the task ended, and gave you two commands to list and prune what it had kept. That was about 5,400 lines in worktree-only files and references in twenty-odd more.
It did bound a worker's edits, and that is worth stating accurately: the permission policy refuses an edit whose paths resolve outside the working directory, symlinks followed, without asking anyone — and with worktrees on, that directory was the worktree. But it is policy, not containment. Nothing in the kernel enforces it, so anything that escapes the tool layer — a shell, a path the check cannot see — is unstopped, which is what the Codex driver means when it says the sandbox is Codex's and not the connector's. This change moves that bound from a per-task worktree to the route itself, which is where it already sat whenever worktrees were off. It is neutral on safety: the case for it is not that it makes anything safer, but that worktrees answered a concurrency question we are not keeping, at the price of 5,400 lines and two public commands.
Worktrees existed so two tasks on one repository could run side by side. That is a concurrency answer, and it is not one we are keeping: two workers in one directory is managed by the people running them until sandboxes are real, and sandboxing is being built separately. The connector runs where it is started, and the agent decides the rest — from its skills, its
CLAUDE.md, its memory. A task that needs a clone or a directory of its own makes one.Originally tracked in The connector runs where it is started: no routes to directories, no worktrees. This is the worktrees half only; the path half of a route —
Route.Path,--route <id>=<dir>, directory resolution — is untouched and comes next. It closes rather than fixes Two connector settings that do not say what they cost, A route waiting for a worktree is visible nowhere and Worktrees and a Codex worker: accepted, and nothing could commit, all of which were about worktrees behaving badly.What the shape of it is
Worktrees turned out to be the only implementation of the
Workspacesseam, so the seam went with them —PerTaskWorkspacesandWaitingWorkspacesare worktree concepts by name, andRecoveringWorkspacesexisted to reconcile worktrees on start. A task's working directory is its route, exactly as it already was whenever worktrees were off, and the dispatcher prepares nothing before a launch and releases nothing after one.That cascaded once more. Nothing can raise
ErrRouteUnusableany more — only a worktree that could not be made ever did — so the whole refused-start path is unreachable and goes too: the ledger's refusal, its hook, its blocked reason, and the holding reply it posted. Route waits go with it; they were in-memory in the worktree preparer, and nothing else waits on a route.Two things worth a reviewer's eye
The ledger keeps migration 9 and adds migration 12. A migration that has been applied is never taken out of the list: removing it would change what every later one means, and a fresh ledger has to walk the same numbers to reach the newest. So migration 9 stays byte for byte, unedited, and migration 12 drops the table it made. The directories are not touched — a worktree the connector kept is still on disk and still git's, and
git worktree removestill removes it. A test upgrades a ledger that really ran 9, with a retained row in it, and proves nothing namedworktrees*is left.connect.jsonkeeps a read-and-discard field for the oldworktreeskey, and it must stay. The file is parsed withDisallowUnknownFields, which is what makes a misspelled setting a refusal rather than a silent nothing. Everyconnect.jsonever written carries"worktrees", so deleting the field outright would have stopped every connector that has been set up. It is zeroed after the decode and kept out of anything written, so the nextconnect setuptakes the key out of the file for good.What happens now when a route's directory is gone
Removing the refusal removed one way a person found out their work could not start, so: the worktrees-off path never checked the route at dispatch —
Preparewasif w.off { return route, nil }, no stat, no refusal — so that error could only ever arise with worktrees on. A vanished or unreadable directory has always surfaced the same way and still does: the spawn fails, is retried once, and the record is blockedspawn_failed, which posts "the worker could not be started. Needs a person:basecamp connect redispatch <n>" on the card. No regression.One thing the tests caught
Taking
renderRefusedStartout broke the retraction test that proves an upgraded ledger answers both of the asks it inherits, and it broke it quietly: the stand-in body I put in the fixture had no redispatch ask in it, and a notice with no ask is a noticepostedAskscorrectly passes over. The assertion caught it because it counts — two retractions, not "the call returned no error" — and a test written the other way would have gone green on one. The fixture now carries the message that build really posted, ask included.Notes
connect worktrees listandpruneare public commands, so.surfacerecords their removal and.surface-breakingacknowledges the 73 entries that go.Two pieces of route-path machinery crossed over: the route-is-a-repository check in setup and the helper behind it were both gated on worktrees, so they became unreachable the moment worktrees went rather than waiting for the follow-up.
The last commit is comments only, in
driver/codexanddriver/acp, where other work is in flight. It can be dropped whole without touching anything else.