Kill the connector at every ledger state, and prove it recovers

[jorgemanrubia]

Stacked on Let a person see, authorize, close and hold what the connector runs, and must not merge before it.

Each lower PR tests its own piece of the connector's recovery, in process, with the ledger driven by hand. Nothing yet kills the whole connector as a process in the middle of a transition and checks that a restart keeps the spec's promises:

• no instruction a worker may have seen runs twice;
• no lifecycle message is posted twice;
• nothing the feed served is lost;
• nothing held is dispatched;
• no task's process tree, directory or record is released while any of its processes still run.

The ACP driver can't become the default until all of that also holds with ACP.

Originally tracked in Integrated recovery test. Spec: "Recovery", "Lifecycle message idempotency" and "Hold and migration" in the Connector spec.

What changes

Only tests change. No production code.

• The connector under test is a real process. It's this test binary, wired the way the run command wires it:

  • the outbox reconciles and sends on start, before any other part runs;
  • intake runs on the feed's real run loop, followed by admission, the dispatcher with a real driver, and the outbox with its lifecycle hooks;
  • if any part stops on its own, the run fails.

  A test SIGKILLs the process at a named point and restarts it over the same ledger file. Each run stops on a ledger predicate. Assertions read the ledger, status, and what reached a fake Basecamp.

• Kill points add no seam to the connector.

  • After a commit: the kill fires when the connector writes the stdout line for that commit. The kill happens inside that write, so no other part can move in between. Rows that would otherwise race the dispatcher or the outbox run without them.
  • Inside a transaction: a ledger hook dies before returning.
  • Mid-read, mid-post and mid-repair-walk: the fake Basecamp dies.
  • Between get_dispatch, ack_dispatch and complete_dispatch: the fake worker kills the connector, checking the pid and start time the connector recorded.
  • Shadow promote and import: the step seams from Let a person see, authorize, close and hold what the connector runs #741.

• A table of drivers. Each row builds the driver with a fake agent as its binary and supplies the fake agent's side of the driver's wire. The Claude Code row speaks stream-json. Whatever the wire, the fake worker binds to its task the way basecamp mcp --connect-state does:

  • it reads the MCP server declaration it was handed;
  • it resolves the state directory by location and name, under the server's declared state home;
  • it opens the ledger as it is.

  Every dispatch, hold and cutover test runs once per row. The Codex (codex exec --json) and ACP (JSON-RPC) rows join after Answer mentions with Codex, and give each task its own worktree and Drive coding agents over the Agent Client Protocol merge and this branch is rebased, so this PR alone doesn't meet the card's done-when. Each new row is one file that registers its wire.

• The credential rule is checked on every run, for every task token the connector minted — recorded by its launch hook, so the check covers a real agent's run as much as a fake worker's, and a run whose worker never took its token. The token's one carriage is the connector's one-use socket, so it must not appear in:

  • the MCP server declaration that names that socket, in its environment or its arguments;
  • an agent's argv or environment;
  • the connector's output, lines, posts, polls or workspace records;
  • a file under a working directory or the attempt's session directory, at the end of a run and at any moment during it — the parent watches, because a worker the connector ends by its process group takes any report of its own with it;
  • a file under the state directory, scanned from a separate process, since reading a live ledger's files in-process drops SQLite's locks on them.

  The check fails closed. It reports how many tokens it checked and how many files it read, and fails if it read none, if a file could not be read (only a file that vanishes mid-scan is tolerated), if the number of tokens does not match the tasks the ledger launched, or if a worker that started neither took its token nor said why it could not. What it can claim is bounded, and that is the claim: 1 to 2 tokens per run, 2 to 8 files read after the run, each token watched for the whole run. A token does reach disk in one place by design — nothing but the socket's own path, which carries no secret.

• An opt-in run against the real agents. Against the real claude, the harness exercises the kill points the connector reaches on its own. The real basecamp mcp built from this tree is the MCP server, holding a token and a base URL that reach no Basecamp.

Guarantees asserted

Each guarantee has its own test or table row, and I watched each one go red with the rule reverted (see Evidence).

1. Recovery never resends a lifecycle message.
   • A completion notice or guard acknowledgement left sending with nothing posted becomes indeterminate and shows in status.
   • One that was posted but lost its receipt is adopted, not reposted.
   • The outbox settles that before the restarted connector dispatches anything new.
   • A second restart posts nothing.
2. No instruction runs again after a worker was exposed to it, whether or not the worker acknowledged it. A crash at running, after get_dispatch, after ack_dispatch or inside the settlement leaves the instruction completed(unknown), or keeps the worker's reported outcome. The instruction is never re-run.
3. One owner, one release point.
   • A worker a crash leaves behind is ended as a process group, its child included. No settled attempt's worker or group member still runs (driver.OwnsWorker).
   • A crash at launching leaves an attempt nobody can identify. Every restart holds it: the record stays dispatched, its directory keeps new work out, nothing is posted, and nothing runs for it. Work in another project still runs.
   • A worker whose grandchild outlives it keeps its attempt live and its directory held through every restart. Recovery doesn't signal a group whose leader it can't verify. Once the tree is gone, the next restart settles the attempt, the waiting event runs, and one notice is posted.
4. Spawn errors. A spawn error retries once, and the retry budget survives a restart. A process that started and then failed its handshake is never retried.
5. Unknown outcomes. unknown is posted once, on the recording that asked, naming the redispatch. redispatch runs the event again exactly once, as a new task.
6. Follow-ups. Follow-ups and their siblings survive a task's end and a crash. An exposed one is never re-run. A never-exposed one runs as a task of its own.
7. Filter changes. A filter change re-enters after the last poll-served id, including after a crash before the new filter set saved a position.
8. Overflow (10,002 real live frames into the feed's 10,000-event buffer):
   • the dropped ids are on disk before the walk starts;
   • a crash between the signal and the walk resumes the walk on start;
   • the feed's checkpoint never holds a repair position at any sampled moment, and is at most the last poll-served id after the crash;
   • the range behind the burst is served before anything from the burst;
   • the walk keeps repeating, the straggler's loss is recovered, and the id that is never served is recorded unrecovered when the window closes;
   • a stalled walk doesn't stop live intake.
9. Backlog saturation pauses the feed without checkpointing the page behind the pause, and a crash there loses nothing.
10. The dispatch prompt stays under budget. It's measured as the worker received it over the driver's wire:
    • a production-sized prompt (810 bytes) is 296 tokens counted by Claude Opus 5, and 405 by the repo's upper-bound estimator;
    • the worst case the prompt can carry — ids at the end of their range and a URL at MaxPromptURL — is 449 by that bound, and a longer URL is omitted whole rather than carried;
    • budget: 500, asserted on the bound in both cases.
11. Hold and cutover.
    • A record whose read is in flight when --hold is set becomes held when the read completes, including across a crash mid-read. The same goes for a shadow being promoted.
    • A crash at every step of shadow promote and import leaves either the untouched shadow or a held ledger.
    • No supervisor restart dispatches a held record, and release leaves it held.

Cost

• The ordinary test job gains about 14 s.
• Under -race, a race-instrumented test binary takes over a second to start, and the harness starts one per connector run and per worker. So the race job runs two representative rows (about 11 s), and the full table runs in the ordinary job.

Evidence

• make check targets are green locally. The worst-case prompt subtest is red until the dispatcher PR's prompt change lands; see Known red.

• CI is green on every job for this head, dispatched by hand (stacked PRs get no CI of their own): Tests, Race Detection, Lint, Integration Tests, CLI Surface Check, Security, Nix. Cross-compiled for windows, darwin, freebsd and openbsd as well.

• Stable under load. 24 copies of the harness ran concurrently at -test.count=2: 48 of 48 passed, twice over.

• Opt-in real run against Claude Code, through the real token socket, the real basecamp connect worker-mcp bridge and the real basecamp mcp. No crash: the real worker took its token and read its dispatch, which the run asserts. Killed at launching: held, while a real worker finished an event in the other project. Killed at running or after the real worker's get_dispatch: unknown, one attempt, no surviving worker.

• Every guarantee proved failing. In a scratch worktree, I reverted each rule one at a time, watched its test go red, and restored it. Examples:

  • the outbox's reconciliation turned back into a resend;
  • an exposed event settled as never-exposed, and the reverse;
  • the exposure at launch dropped;
  • the retry budget widened;
  • the hold's trigger removed;
  • a held attempt settled, or its continue removed;
  • recovery settling around a worker it can't verify;
  • a worker signaled by pid rather than by group;
  • the walk writing the feed's checkpoint;
  • the backlog dropping an offer instead of pausing;
  • an import committing entry by entry.

  The scripts (mutate*.py) and logs are in the card's scratchpad. They will be re-run on the final head after the last rebase.

• One clause is proved only by its outcome, and this says so. "A dropped live id far ahead must not skip the unpolled range behind it" has no connector-side rule to revert on its own: the feed package decides that only poll pages move a position. What the connector owns is that its repair walk never writes that position, and that is proved by mutation.

• Adversarial reviews (Opus, not a human), three rounds, each on the exact head of the time, fixing everything each one found:

  • assertions that could never fail or ran before their state existed;
  • kill points that raced the dispatcher or the outbox;
  • a fake worker more permissive than a worker's MCP server;
  • hold tests stopped before the connector had acted;
  • no process tree for the group rule to act on;
  • kills of remembered pids without an identity check.

  A fourth review found one blocking gap — the credential check did not watch the attempt's session directory, where the driver writes what it hands the agent — and Copilot then found that the same check skipped the real-agent runs entirely, and that a scan which cannot read a file is not a scan that cleared it. Both are fixed here, and two mutations hold the check to it: one writes the token into the session directory, the other into the server's declared environment. The first used to pass. Copilot's findings on this PR's files are fixed too, including an operator's BASECAMP_BASE_URL reaching the real-agent run's server.

• Found along the way:

  • A test process that reads a live SQLite ledger's own files through another descriptor drops SQLite's POSIX locks. When another process then closes the ledger, the held handle goes stale or fails with I/O error 522. There's a deterministic repro, it's fixed here by scanning from a subprocess, and it has been relayed to the cards that use the credential helpers.
  • CI runs no tests for stacked PRs, since test.yml only fires against main. Manual dispatches are used instead.

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

This PR expands the Basecamp connector implementation and CLI surface, adding worker selection, lifecycle/outbox behavior, operator workflows (hold/release, shadow promote, import), and a comprehensive recovery harness to validate crash-safety invariants.

Changes:

• Add connect as a “bare” command group (run is the default) and introduce new subcommands (status/doctor/redispatch/discard/release/shadow promote/import).
• Introduce worker selection (worker) in connect.json, spawn-driver dispatch via worker constructors, and a default permission policy.
• Add lifecycle messaging via an outbox (post + reconcile), shadow promote/import flows, and extensive kill/recovery tests.

Reviewed changes

Copilot reviewed 72 out of 72 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
scripts/check-bare-groups.sh	Allowlists connect as a bare command group.
internal/connector/setup/file_test.go	Tests default/validation behavior for the new worker field.
internal/connector/setup/file.go	Adds worker to connect.json with defaults + validation helper.
internal/connector/setup/apply.go	Allows applying a worker change with validation.
internal/connector/sdk_dispatch.go	SDK-based implementations for reply listing and membership listing.
internal/connector/recovery_worker_test.go	Adds a fake worker for recovery harness scenarios.
internal/connector/recovery_real_test.go	Optional recovery test against real agent binaries.
internal/connector/recovery_race_test.go	Race-mode build flag wiring for recovery harness.
internal/connector/recovery_norace_test.go	Non-race build flag wiring for recovery harness.
internal/connector/recovery_intake_test.go	Recovery tests around intake/feed checkpointing and repair behavior.
internal/connector/recovery_hold_test.go	Recovery tests around hold behavior and cutover safety.
internal/connector/recovery_harness_test.go	Integrated kill/restart harness for end-to-end recovery invariants.
internal/connector/recovery_claude_test.go	Recovery harness driver registration for Claude spawn driver.
internal/connector/promote.go	Implements shadow ledger promotion under an operator hold.
internal/connector/policy_test.go	Tests for permission policy containment + prompt redaction behaviors.
internal/connector/policy.go	Implements default permission policy with symlink-aware path containment.
internal/connector/outbox_kill_unix_test.go	Kill/restart test ensuring outbox send/receipt behavior is safe.
internal/connector/outbox_fakes_test.go	Outbox test fixtures (fake Basecamp + ledger helpers).
internal/connector/outbox_basecamp_test.go	Tests BasecampPoster post/list semantics and paging behavior.
internal/connector/outbox_basecamp.go	Implements BasecampPoster post/list with unlistable handling.
internal/connector/operator_status_test.go	Tests status reads beside writers and does not leak secrets.
internal/connector/operator_migration_test.go	Tests promote/import invariants including kill-at-step crash tests.
internal/connector/lock.go	Adds non-locking instance holder inspection for status/diagnostics.
internal/connector/lifecycle_test.go	Tests lifecycle templates, completion rules, redaction, and adoption behavior.
internal/connector/lifecycle.go	Implements lifecycle message rendering + reconciliation text normalization.
internal/connector/ledger_import.go	Adds reconciliation parsing and atomic import behavior.
internal/connector/ledger_events.go	Extends transitions for operator edges and adjusts content dropping behavior.
internal/connector/ledger_admission.go	Reads back held-vs-admitted state so hooks/logging reflect what was written.
internal/connector/ledger.go	Adds held state + registers new migrations (tasks/attempts, outbox, operator).
internal/connector/driver/worker_unix.go	Unix process-group helpers for worker lifecycle.
internal/connector/driver/worker_other.go	Non-unix stubs refusing worker support.
internal/connector/driver/worker.go	Implements worker process management (env isolation, process groups, termination).
internal/connector/driver/spawn/spawn_test.go	Ensures each configured worker has a spawn driver row.
internal/connector/driver/spawn/spawn.go	Adds worker->driver constructor mapping for spawn driver selection.
internal/connector/driver/proctime_other.go	Platform fallback for process start-time reading.
internal/connector/driver/proctime_linux.go	Linux /proc-based process start time resolution.
internal/connector/driver/proctime_darwin.go	macOS sysctl-based process start time resolution.
internal/connector/driver/env.go	Adds minimal env allowlist building and redaction utilities.
internal/connector/driver/driver_test.go	Tests environment isolation and process-group termination behaviors.
internal/connector/driver/driver.go	Introduces driver boundary abstractions + invariant documentation.
internal/connector/driver/claude/claude_test.go	Extensive tests for Claude driver invariants and safety behaviors.
internal/connector/admission/matrix.go	Adds held state as “never a verdict” but a ledger-written state.
internal/connector/admission/commit_test.go	Updates commit serializer test to acknowledge held writes.
internal/connector/admission/commit.go	Allows admitted verdicts to be written as queued or held.
internal/commands/connect_run_test.go	Tests connect routing TTL behavior and platform gating.
internal/commands/connect_process_unix.go	Adds pid-existence check on unix for status/diagnostics.
internal/commands/connect_process_other.go	Non-unix processAlive stub.
internal/commands/connect_operator_test.go	Tests operator subcommands (status/redispatch/discard/release/promote/import/doctor MCP env).
internal/commands/connect_doctor_mcp_unix.go	Implements doctor MCP handshake check via a spawned MCP server.
internal/commands/connect_doctor_mcp_other.go	Skips doctor MCP handshake off unix.
internal/commands/connect_doctor.go	Implements connect doctor readiness checks.
internal/commands/connect.go	Makes connect run the connector by default and adds new subcommands + setup worker flag.
e2e/smoke/smoke_lifecycle.bats	Marks connector lifecycle/operator commands out of smoke-test scope.
STYLE.md	Documents connect as a bare group exception and why.
.surface	Updates CLI surface definitions for new connect subcommands/flags.

Suppressed comments (2)

internal/connector/ledger_import.go:1

• json.Decoder.More() does not detect trailing top-level JSON values (it’s only meaningful while decoding arrays/objects). This will incorrectly accept inputs like {\"version\":1,\"entries\":[]} {} and contradicts the added strictness tests. Fix by attempting a second Decode and requiring io.EOF (or alternatively consume trailing tokens and ensure only whitespace remains).
  internal/connector/policy.go:1
• policyAllowedKinds includes read and search, but inside() returns true when Locations is empty (loop doesn’t run), so a read/search request without paths would be allowed. That’s a concrete policy bypass risk if an agent omits locations. Consider handling kinds individually: require len(req.Locations) > 0 for filesystem-touching kinds like read/search, while still allowing think with no locations.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved moderate and critical recovery-test coverage and isolation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (10)

internal/connector/recovery_connector_test.go:283

• The production run wires LifecycleFilteredReplies{Lister: poster, Ledger: ledger} here, but the harness supplies the raw lister. Consequently an outbox post that reached Basecamp before its receipt was recorded can be returned as a worker reply during a redispatch, because this lister has no body-based exclusion for unreceipted lifecycle messages. Use the same filtered reply adapter so the recovery harness covers the actual adopted-reply contract.

		Replies:            storeReplies{dir: dir},

internal/connector/recovery_connector_test.go:530

• This range count does not prove that every expected feed event is present: an extra ledger event inside [lowest, highest] can offset a missing expected ID and make have >= want pass. The harness therefore can declare the connector settled while a specific served event is absent; compare the actual IDs with the expected set (or query membership), not only the interval count.

		var have int64
		if err := l.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM events WHERE id BETWEEN ? AND ?`, lowest, highest).Scan(&have); err != nil {
			return false, err
		}
		if have < want {

internal/connector/recovery_dispatch_test.go:114

• processGone is compiled on Darwin as well as Linux, but it unconditionally reads Linux's /proc/<pid>/stat. When an exited worker is still an unreaped zombie on Darwin, kill(pid, 0) succeeds and this read fails, so the helper reports the worker as alive; the worker-kill cases can then fail or time out. Keep the zombie check in OS-specific implementations (or use Darwin's process-status API).

func processGone(pid int) bool {
	if err := syscall.Kill(pid, 0); err != nil {
		return true
	}
	stat, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat")

internal/connector/recovery_dispatch_test.go:153

• This crash table never kills the connector while an event is in queued: the follow-up tests wait until event 102 is dispatched before killing, and every row here uses a single event that cannot queue behind another task. The PR's “every ledger state” recovery proof therefore omits the queued-state restart path; add a same-conversation follow-up that is killed before the queue drains and assert it is not lost or run twice.

var crashRows = []crashRow{
	{name: "seen, the read in flight", kill: "read:5001", plan: completedWork,
		handed: 1, outcome: OutcomeSucceeded, stop: StopFinished},
	{name: "seen, the verdict not committed", kill: "tx:verdict", plan: completedWork,
		handed: 1, outcome: OutcomeSucceeded, stop: StopFinished},

internal/connector/recovery_dispatch_test.go:157

• The line:event:admitted kill is invoked after the admission commit, but the dispatcher can observe that committed state concurrently and launch the event before the line writer runs. The row still requires exactly one prompt and a succeeded outcome; if the kill lands after launch but before prompting, recovery legitimately yields an unknown outcome (or an unidentifiable live launch), making this case scheduling-dependent. Add a deterministic post-commit barrier or make the assertion cover the valid state reached at the kill.

	{name: "admitted", kill: "line:event:admitted", plan: completedWork,
		handed: 1, outcome: OutcomeSucceeded, stop: StopFinished},
	{name: "dispatched, attempt running before the prompt", kill: "line:dispatch:running", plan: completedWork,
		handed: 0, outcome: OutcomeUnknown, stop: StopLost, notices: 1},

internal/connector/recovery_fakes_test.go:221

• This fake drops the Filters argument, so the filter-change test is not exercising the feed contract: it publishes events in buckets 9004/9005 while the narrowed filter permits only harnessBucket, yet this poll still returns them. If the fake is corrected, ledgerSettled also needs to count only entries matching the active filter (and the test data must put the expected replay in that filter); otherwise the test either proves the wrong behavior or times out instead of validating re-entry.

func (p *filePolls) Poll(ctx context.Context, cursor eventfeed.Cursor, _ eventfeed.Filters) (eventfeed.PollPage, error) {

internal/connector/recovery_harness_test.go:351

• Every harness start is wrapped in a 90-second CommandContext, while runHarnessConnector deliberately gives real-driver runs a five-minute runFor. A legitimate slow real-agent turn is therefore killed by the parent at 90 seconds and reported as a test failure before the connector's intended real-run bound is reached. Derive this parent timeout from the driver (at least five minutes for real rows).

	ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second)

internal/connector/recovery_harness_test.go:1

• The recovery files are guarded by the broad unix tag, but the production connector intentionally supports only Linux and Darwin, and driver/proctime_other.go cannot terminate recorded workers on the other Unix targets. On a target such as FreeBSD, any row that leaves a worker live will therefore remain unsettled and time out instead of testing recovery. Restrict this harness to the supported targets or skip it there.

//go:build unix

internal/connector/recovery_harness_test.go:403

• A h.run with Killed: true is also killed by the 90-second exec.CommandContext deadline in start, and h.wait accepts any SIGKILL as proof that the requested crash point was reached. Thus a connector that hangs or never reaches post-before, a worker kill, or another non-line point can be reported as a successful kill test. Track the command deadline/cause or require a point-specific marker before accepting the signal.

		var exit *exec.ExitError
		require.True(h.t, errors.As(err, &exit), "the connector must die (kill %q): %v\n%s", r.Kill, err, out.String())
		status, ok := exit.Sys().(syscall.WaitStatus)
		require.True(h.t, ok)
		require.True(h.t, status.Signaled() && status.Signal() == syscall.SIGKILL, "killed at %q, got %v\n%s", r.Kill, err, out.String())

internal/connector/recovery_real_test.go:42

• This real-agent row cannot reach the assertions below: dispatch:launching is emitted before NewSession starts a worker, so the crash leaves the attempt in launching with no recorded PID. Dispatcher.Recover intentionally keeps such an attempt live, while the subsequent h.run waits for ledgerSettled (which counts every non-ended attempt as busy) and times out. Remove this row from the settling matrix or give it a separate assertion for the unrecoverable/live state, rather than expecting StopLost.

		{name: "attempt launching", kill: "line:dispatch:launching", lost: true},

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

[Copilot]

🔵 Needs a closer look

Unresolved recovery-test coverage, harness reliability, and platform-support issues remain.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/connector/recovery_connector_test.go:550

• This predicate only compares the number of rows in the numeric range, not the IDs themselves. A normal feed can have non-contiguous event IDs; if, for example, 102 is recorded while 103 is missing, the two-row range can still make have == want, so the harness can stop and declare recovery complete with a served event absent. Compare the actual wanted IDs (for example, select the range into a set) before returning settled.

internal/connector/recovery_connector_test.go:563

• ledgerSettled never checks the losses table, so an open repair loss is treated as idle once ordinary events, attempts, and outbox work are gone. A default harness run can therefore stop before the repair walk completes, weakening any recovery test that relies on the default predicate after a crash. Include open losses in this settled condition (or make the default predicate use the losses-closed condition).

• Files reviewed: 68/91 changed files
• Comments generated: 0 new
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Token-leak checks can silently skip real-agent runs, terminated workers, and unreadable state files.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Live-event persistence can invalidate crash recovery evidence, and failed real-agent tests can leave worker processes running.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

internal/connector/recovery_fakes_test.go:416

• The fake marks a live event durable only after delivering it. A SIGKILL between Serve and the liveFile append makes the next connector process replay that event, even though the comment and recovery scenario require a live push to happen only once. In the overflow case, this can let an event dropped before the crash return through the live lane and make the recovery test pass for the wrong reason. Persist the batch before exposing any frame.
  internal/connector/recovery_harness_test.go:475
• This cleanup only discovers processes from agentLog, which is written by fakeWorker; a Real driver starts the actual agent and never writes a start entry. If an opt-in real-agent subtest fails after its connector is killed but before recovery completes, cleanup leaves that agent/process group running (potentially continuing a paid model call). Cleanup also needs to terminate identities recorded in the real run's ledger.

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The token-watcher shutdown race can leave findings unreported and leak a watcher.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/connector/recovery_harness_test.go:549

• Closing stop does not wait for the discovery goroutine to finish. If that goroutine has already read knownTokens() and is blocked on watchMu, this method can drain the map and return; once the lock is released, the goroutine adds a new WatchForSecretFiles watcher that is never stopped or inspected. That both leaks the watcher and can discard a transient-token finding. Add a completion channel for the discovery goroutine, close stop, wait for completion without holding watchMu, and only then drain the per-token watchers.

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[jorgemanrubia]

🤖 Landed in #748, 6d0079ca — the six of us as one commit. Closing this as superseded, not abandoned: every commit here is in that merge.

Combining them is what found the defects none of us could see alone — a redispatch that blocked a conversation permanently once another branch's trigger landed beside it, a read-only ledger open that dropped the POSIX locks a writer held, doctor rejecting the configurations the same release adds, a recorded process losing its kernel start time so status called every live worker unverified, and four branches each writing their own migration 8.