Six stacked pull requests, landed as one - #748
Conversation
A shadow already held by --hold keeps that cause through the promote, so a promote killed after its rename was refused when run again. The crash tests now cover a held shadow; the promote tests are Unix-only, as the process signals they use are.
…draw every waiting redispatch A redispatch that supersedes a live task's token leaves that task running until its worker is gone; a new event on its conversation must not be handed to a worker whose token is refused. An import is a cutover review, so it withdraws a redispatch still waiting for its task whether or not the file names the record, as a hold does.
…ken checked when it is bound
…ady made An import cancels the lifecycle messages of a record it closes, as a discard does, and a completion notice asks for a redispatch only where one is still open. The run command passes the hold to the outbox and records its connection state for status, and a decision refuses to migrate the ledger under a connector running on an older schema.
…ames no path The metadata beside the lock is best-effort, so a connector on an older schema could be missed and its ledger migrated underneath it; the lock itself now says whether one is running, and it is held until the ledger is closed. An empty permission location resolved to the working directory, which let an edit that named no path pass the in-directory rule.
… rendered at the send, no follow-up under the hold Import took the instance lock and then refused itself over it on the very ledger it exists to migrate: the open takes the lock now, once, for the whole command. A completion notice is rendered again from the records when the outbox claims it, so a record decided while it waited asks nothing of a person. JoinConversation was the one hand-off to a worker without the hold's check.
Redispatch stops the worker it replaces only while driver.OwnsWorker says the recorded process — pid and recorded start time — is still that worker, then confirms its group gone; a group that outlived its leader, or an identity that cannot be established, is left alone and reported, and the redispatched record still waits for the task's owner to confirm the group gone. Status reports each live attempt's worker the same way, signaling nothing.
A replaced attempt still launching has no recorded worker, and is reported so rather than as gone; a worker that still runs outside its recorded group is not reported stopped. A completion notice is re-rendered only when an event its settlement names was decided.
… and status and doctor claim only what they check A redispatch of a blocked record is a decision though the record stays blocked, so a completion notice claimed meanwhile asks nothing more. The run command records running once its parts have started and stopped on exit, rather than connection states intake never reports. Doctor says its MCP handshake is the agent's server with a worker's environment, without the dispatch domain only a task's token opens. The one test that needed a member-less process group fakes the driver instead of reusing a freed id.
A redispatch's authorization stayed on the record, so once a person had redispatched an event every later failure of it was reported as decided and asked nobody. A completion notice now counts an authorization as a decision only when it was made after the attempt ended, and a blocked record counts as authorized only when it was authorized since it was last blocked.
…build wrote An import's done decision on a completed record whose outcome waited for a person (unknown or failed) now closes it as discarded(imported_done), against the import's decision row, so no redispatch is accepted and no notice asks for one. Status and the operator commands refuse a ledger at a newer schema than this build writes, as the worker's open does.
… import actually did An older basecamp rolled back onto a ledger this migration wrote skipped every migration it knew and ran over held records and triggers it does not understand; the owner's open refuses a newer schema now. An import's done decision on a completed success records completed, the state it left, not discarded.
…issing shadow ledger at the shadow run
…ommands, doctor ends its server's whole group The basecamp-connect skill no longer says the connector's state does not exist or offers to start it; it names where the ledger lives and how to read it, who decides what, and how to take the commands' hints. basecamp commands lists the operator commands. Doctor's MCP server is ended as a group before its leader is reaped, on a timeout and on a failed handshake, so a descendant it started does not outlive the check. Redispatch reports worker_signaled rather than a worker_stopped that could contradict worker_state.
…efore it is reaped, race-free The SDK's command transport waits on the process in its own goroutine, so a check of whether the leader was reaped raced that wait. Doctor now starts the server itself, hands the SDK only its pipes, and alone signals the group and then waits.
…read a reconciliation file whole A redispatch runs a record's prerequisite as the agent, so a BASECAMP_TOKEN in the environment would decide it as somebody else; both redispatch and doctor now refuse before the ledger is opened. The reconciliation parser accepted a stray closing brace after its one value, which Decoder.More does not report.
…octor's timeout seam
…does not stop The decision results carried Go field names into --json while status beside them was snake_case. The hold's invariant now also says what it does not reach: a worker a crashed connector left running holds its own token until a start recovers it, which is the one-owner rule's to end.
A worker's MCP server takes the task token and lives in a process group of its own, so it can outlive the worker that started it and still hold the token. Status asks the same one-owner question of it and says running, gone, held or unverified.
The startup scan used base ten and the first occurrence; pflag uses any base and the last. On a spelling only one of them accepted, the descriptor was drained and closed while the command served from another, or the scan missed it and the read fell back into RunE — after the root hooks had run, which is the window the startup read exists to close. The scan now reads the flag as pflag does, stopping at a bare --, and there is no late read: a server whose token was not taken at startup refuses to start. A test runs both parsers over the same arguments. An acknowledgement arriving after the outcome is refused rather than written, and a worker holding an event reaches the model as not_dispatchable rather than as a ledger fault.
The command was gated to Linux when the task token's hand-over onto a descriptor the next program inherits was sealed there and nowhere else. The skill still told a reader it runs on macOS too.
The harness writes down the processes it starts so it can end them the way the connector does: pid, group and start time, through driver.OwnsWorker and driver.TerminateRecorded. It wrote a wall-clock stamp, which is not an identity — the one-owner rule answers driver.ErrIdentityUnknown to a record whose start time the kernel never gave — so every one of those calls refused and the harness signalled nothing it had started. The fake worker that is to kill its connector could not confirm the pid it was about to signal, so it killed nothing and the run went on to its cap instead of dying; killAgents left workers and their grandchildren running after a failed test; and the surviving-tree case could not end the grandchild whose death the next assertion waits for. The agent log now records the kernel's start time for the worker and for the grandchild it forks, or none at all where the kernel cannot be asked, and every reader turns that into an identity the same way the ledger does.
There was a problem hiding this comment.
🔵 Needs a closer look
The broad security-sensitive process, credential, migration, recovery, and filesystem changes require final human validation despite extensive test coverage.
Review details
- Files reviewed: 57/127 changed files
- Comments generated: 0 new
- Review effort level: Balanced
staticcheck reads a raw C1 or DEL in a literal as a mistake, which is fair: the test is about those bytes, so it should name them where a reader can see them. The unix listener in the bridge test takes the test's context too.
There was a problem hiding this comment.
🔵 Needs a closer look
The large, security-sensitive change spans process control, crash recovery, database invariants, networking, and destructive worktree operations and warrants final human review.
Review details
- Files reviewed: 57/127 changed files
- Comments generated: 0 new
- Review effort level: Balanced
internal/connector runs real sockets, real processes and the recovery harness. Under -race that is about five minutes on a fast box and roughly twice that on a runner, against Go's default ten-minute package budget. The two failures this fixes named different tests, one of them zero seconds in, which is what a budget running out looks like rather than a test hanging. The hang that did exist was a real bug and is fixed separately.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
There was a problem hiding this comment.
🔵 Needs a closer look
The recovery harness still omits Codex and ACP despite requiring all supported drivers.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/connect_doctor.go:258
- On macOS this failure message is self-contradictory: it says the connector cannot run there and then says macOS can provide the stated process-group/start-time capability. The actual Linux-only constraint is the inherited task-token descriptor sealing described by
connectSupportedOS; report that constraint soconnect doctorgives an actionable explanation on Darwin.
- Files reviewed: 58/128 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…a platform On macOS the Platform check said the connector does not run there and then named a capability macOS has, so the failure contradicted itself and sent a Mac reader after the wrong thing. The reason is the one the run command already gave, from #736: the task token reaches a worker's MCP server over an inherited descriptor, and Linux alone seals the descriptors a process passes on. Reading a process's start time is the half macOS has. doctor and the run command now take that reason from one place, so a person who meets both is not told two different stories about their machine.
The harness said each driver the connector starts workers with registers a row, and only the Claude Code spawn driver's did. Codex and the acp driver now have rows of their own, so every kill point is proved against all three, and a test holds the list to the drivers `basecamp connect run` resolves rather than to whoever remembered to register one. Recovery is where the drivers differ: each leaves a different process tree behind and says the worker is gone in its own way. Both new rows caught their own driver's rule when it was mutated away — Codex's rollout policy check, and the acp driver's confirmation of the asking mode. Two things the harness does not cover are now said where it says what it covers. A case about a second prompt on a live session runs only on the drivers whose sessions take one: a Codex process is one turn, and what the dispatcher does with a follow-up it cannot hand over is its own test in dispatcher_test.go. And the run against real agent binaries is still Claude Code alone.
|
🤖 Both findings from the last review are on The recovery harness. It did claim all drivers — its own doc said "each driver the connector can start workers with registers a row", and only Claude Code's was registered. So the claim was false rather than narrow, and the fix was to make it true rather than scope it down. Cost decided it and it was small: the package went 108s to 134s, and under Tested per driver now: every kill point in the crash table, spawn-failure retry and its budget across restarts, the handshake-failure case, redispatch after an unknown outcome, the held-attempt rule, the surviving-tree one-owner rule, the guard acknowledgement, and outbox reconciliation — each against that driver's real wire. Two things deliberately not tested, written into the harness's own "what this does not cover" section. The follow-up cases skip Codex: a Codex process is one turn, so the dispatcher never exposes a follow-up to the live worker and The rows are load-bearing, not decoration. Mutating Codex's rollout policy check reddens only the codex row; mutating the ACP driver's asking-mode confirmation reddens only the acp row. The doctor message. Fixed, and the reason now lives in one place. The Platform check reports the constraint that actually applies — the task token reaches a worker's MCP server over an inherited descriptor, and Linux alone seals the descriptors a process passes on — rather than the process-group capability macOS has. Three more of the same shape, filed rather than fixed here. This pull request has been through seven review rounds and is large enough; growing it further trades one risk for another. The sharpest: |
There was a problem hiding this comment.
🔵 Needs a closer look
The 45,000-line change spans security-sensitive credential transfer, process ownership, durable migrations, recovery, and destructive worktree operations.
Review details
- Files reviewed: 58/130 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Six pull requests that could only be reviewed apart, and only worked together.
Cards 18 through 23 were built as a stack: the dispatcher, the lifecycle outbox, the Codex
worker and its worktrees, the operator commands, the recovery harness, and the ACP driver.
Each sat on the one below it, so each was open against its parent branch rather than
main— andtest.ymlonly fires on pull requests targetingmain. None of them had runthe test suite, the linter, the race detector or the surface check in days. They had
gate,labeland cubic.Landing them one at a time meant six rounds of retarget, re-merge and re-review, with
#736 freshly squash-merged underneath
them all. This is those six brought together instead: one branch off
main, one suite, onereview.
Bringing them together is what found the bugs. Three collisions exist only where two
branches meet, so no individual pull request could have surfaced them however green it was:
redispatchsuperseded a task with a bareUPDATE tasks SET superseded_at. Another branchadded a trigger that retires
task_eventsin that same write. Together, redispatch retireda task's rows without returning the work it never exposed, stranding a sibling event as
dispatchedon a dead task — and the settlement that would recover it only reads live rows.The conversation was blocked permanently. It merged clean, compiled clean, and surfaced only
when a test ran far enough to reach it.
The read-only ledger open built a
Ledgerby hand, soClosedereferenced a nil fileregistry. The nil guard would have hidden the real problem: that path also opened and closed
its own descriptor for the privacy check, and POSIX drops every lock a process holds on a
file when any descriptor for it closes — including the locks a writer beside it is holding.
It now goes through the same claim-and-release path the writer uses.
And four branches each wrote their own "migration 8". Shipping together settles the order
once — 7 tasks and attempts, 8 outbox, 9 worktrees, 10 the operator's tables — and these
numbers never move again, which is the whole point of a migration that has shipped.
Supersedes #738, #739, #740, #741, #742 and #743.