Drive coding agents over the Agent Client Protocol by jorgemanrubia · Pull Request #742 · basecamp/basecamp-cli

jorgemanrubia · 2026-09-17T07:01:28Z

Stacked on Dispatch admitted mentions to Claude Code workers. Do not merge before it.

The connector can only start workers by spawning claude -p. That means every coding agent needs a driver of its own: its own event schema, permissions frozen into flags, and cancel by killing the process. The spec's second driver fixes that with the Agent Client Protocol: one client, and any agent with an ACP adapter plugs in.

Tracked in ACP driver: spike, then v1 client. The spike on that card showed what the pinned adapters can do and where they can't be trusted, and this driver is built on those findings. Jorge's decision was to follow the original plan. ACP can be selected now (--driver acp), and spawn stays the default. ACP becomes the default only once card 22's recovery harness passes against both adapters.

What changes

internal/connector/driver/acp: a hand-rolled ACP v1 client that speaks newline-delimited JSON-RPC 2.0 over the adapter's stdio, behind driver.Driver.
- It calls initialize, then session/new {cwd, mcpServers}, or session/load / session/resume when the agent advertises it.
- It sends session/prompt and session/cancel.
- It reduces session/update to kinds, ids and counts.
- It answers session/request_permission from the policy and declines fs and terminal.
- Session ids go into attempts.session_id, so no migration.
- No library: the community Go SDKs track the protocol's unstable drafts, and we use a handful of methods.
Pinned adapters.
- @agentclientprotocol/claude-agent-acp@0.78.0 and @agentclientprotocol/codex-acp@1.12.0 come from a lockfile. They are installed once with make acp-adapters, which runs --ignore-scripts --engine-strict.
- acp.Locate finds them and refuses any other version. They are never downloaded at dispatch.
- Variables that would swap the agent an adapter bundles (CLAUDE_CODE_EXECUTABLE, CODEX_PATH) are not passed through.
make test-acp-compat: the spike's four checks, run through this driver against both installed adapters. It sits behind the acpcompat tag and adds four checks:
- The worker's shell sees neither the task token nor the host's token.
- An MCP server the working directory declares never runs.
- The connector's token bridge is reached from wherever each adapter starts its MCP servers, the token appears in no process's environment or argv and in no file, and a session whose MCP server did not connect is refused rather than run.
- What each adapter does when a session's MCP server dies mid-session (below).
basecamp connect --driver acp [--acp-adapters DIR] picks the adapter from connect.json's worker.

Where the rules live

Three of these are rules rather than single checks, and each new site of one was costing a review round. Each is now stated once, at the top of the one file that holds it:

mcp.go — the MCP isolation boundary. What is declared (wireServers), what the adapter must not add (strictMcpConfig, the Codex preflight), and what actually connected (reportMCPServers, the one judge of the adapter's own account).
permission.go — who may decide a permission, and on what evidence. The conditions a request must meet to reach the policy at all, the three fields that are trusted (sessionId as a guard, options[].kind, toolCallId as an opaque key), and what an adapter can forge, with what is done about each.
limits.go — what bounds every buffer. Every bound named in one block: per line, per session, per turn, per tool call, per option list, at once, and in time.

Invariants

These are the package doc's invariants. Each is held by a test driven by a fake agent: the test binary re-executed as the adapter. I reverted each rule on the head and watched its test go red.

The adapter's environment is an allowlist. It gets the session's environment plus variables it names exactly. MCP servers get exactly their declared env.
- The host's CLAUDE_CODE_MESSAGING_TOKEN reaches neither.
- The task token isn't in any environment: it reaches the MCP server over the connector's one-use socket.
No session runs outside its asking mode. After session/new or a load, the driver sets the mode and reads it back, from session/set_config_option's option list or a current_mode_update. A mode report that overtakes the read-back isn't overwritten by it. If the mode can't be set and confirmed, the result is ErrUnsafeMode and the process group is ended. If the agent later reports another mode, the turn fails and the worker is ended.
Permissions are chosen by option kind, never by id or label, and an allow is only ever allow_once. Requests are refused without asking the policy in these cases:
- a request outside a turn, for another session, before the mode is confirmed, or read after the agent answered the prompt;
- an allow that comes back after the turn was canceled or settled.
A tool name that isn't plain is no name, never one made plain. A codex MCP call is named only when its marker, title and raw input agree.
A refusal is the driver's own record. A refusal lands on its turn even when made without asking the policy. A cancelled stop the connector didn't ask for is TurnRefusal when the turn had refusals and an error otherwise, never TurnCanceled. A cancel is one-shot: it ends the turn it was meant for and is never sent after the agent answered.
Load follows what the agent advertises, and its replay is not progress.
The adapter is the pinned one. Beyond that:
- Only a start that ran nothing is ErrNotStarted, and a configuration no retry can fix is also ErrUnusable.
- A handshake that fails after the adapter started returns a driver.StartError carrying its process.
- Close and Cancel are bounded, including against an adapter that stops reading or a descendant that leaves the group.
Only the session's MCP servers run. Claude sessions set strictMcpConfig. A Codex session is refused before anything starts when any config layer on disk declares mcp_servers, because Codex would merge those servers in, and one named basecamp would replace the connector's with every tool allowed.
No session goes on without its MCP servers. The adapters each say whether the servers connected — Claude Code's init, forwarded on request; codex-acp's startup failures — and the session ends with ErrSessionUnverified when a server is not connected, when a Claude turn ends with no init at all, or when the init names a server the session never gave it. Without this a worker runs on without the Basecamp tools or its task token, answers nothing, and settles as finished.
Nothing the agent volunteers is kept, and nothing of the worker's gets out. _auth/status_update, which carries the account email, is dropped unread. Updates carry no text, titles or inputs, and their ids are bounded. Everything that crosses back — errors, update ids and names, refusals, the adapter's stderr — goes through the driver package's redactor, and drivertest.RequireRedacted feeds a planted secret through this driver's five error paths.
A refusal is recorded where it survives the session. Each one is written through SessionConfig.Refusals as it is made, once per tool call id, so a worker that dies before its result has already reported it.

What the adapters taught, beyond the spike

claude-agent-acp loads the host's Claude Code settings: bypassPermissions, allow rules, hooks, plugins' MCP servers. The session's _meta turns them off (settingSources: [], bypass disallowed, strictMcpConfig, and no plan mode, which would leave the verified mode). Set-and-verify stays the protocol-level backstop.
claude-agent-acp hands mcpServers[].env to the Claude CLI on its command line (--mcp-config). A token carried there would be readable by any local process for the whole session. That is why the token goes over the socket bridge. Check 7 goes red on both adapters if the token is put back into the env.
codex-acp names an MCP call by id alone in its permission request. The driver remembers what each announced call said.
Codex's asking mode leaves /tmp and $TMPDIR writable without asking, and the adapter fixes that sandbox per turn. This is policy, not containment.
codex-acp runs codex app-server, which has no --ignore-user-config.
- The host's plugins, hooks, apps and skill instructions are switched off through the thread config (CODEX_CONFIG).
- Host MCP servers can't be switched off that way, so the preflight refuses the session.
- The preflight can't see configuration that isn't a file on this machine (MDM, cloud-managed, plugin), and its comment says so.
Both adapters report a server that did not start, in their own shapes: Claude forwards its CLI's init message, Codex reports a failed mcp_startup tool call. Neither reports success in a way the other does.
Both re-run an MCP server's command when the server dies mid-session (check 8, which kills the session's stub server by the pid the stub recorded and then insists the agent use its tool). claude-agent-acp's new process lands in the worker's own process group; codex-acp's lands in a group of its own, descended from the worker's leader — the two shapes the connector's token socket already accepts, so its one handoff per start is the right shape. Neither adapter tells the client its server died, so nothing here sees it: ErrMCPServerNotConnected covers a session's start, not a death twenty minutes in. The consequence worth watching is the handoff budget — a crash-looping server burns one handoff per start, and a start past the budget comes up with no Basecamp tools, which the connector now logs against the attempt.
Neither adapter's session/new waits for its MCP servers. So the connector can name the worker's process group to the token socket after NewSession returns: the handoff was delivered about 0.8s after the session began on both.

Review

Opus adversarial reviews: twelve rounds against the head each time. They found six blocking defects, all fixed with fail-first tests: foreign MCP servers reaching the session, a Close that could wait forever on output an escaped descendant held, the token in the Claude CLI's argv, a session running on without the MCP server it was given, an unbounded map of agent-chosen session ids held while session/new was pending (Copilot found this one independently), and a replayed tool call that could name a privileged MCP tool for a later permission request to inherit. The fixes to their non-blocking findings are listed under Invariants. Their findings were new every round but clustered: each one was a different site of a rule already written down elsewhere in the package, which is what the consolidation above answers.
Copilot: every thread is answered. The two that stayed open for shared rules — redaction and refusal accounting — are adopted now that card 18 has landed them: this driver uses the package's redactor and records each refusal through the ledger's recorder.

Evidence

make test-acp-compat: seven of seven checks pass against both adapters, installed fresh. Check 7 also proves the refusal above live: the bridge's basecamp mcp cannot authenticate with the check's dummy profile, so both adapters report the server failed and the driver refuses the turn.
End to end on the scratch project, as the agent, pinned to that project, with the token bridge:
- claude-agent-acp answered card 23: ACP claude answers.
- codex-acp answered card 23: ACP codex answers. This run used a local build with codex in setup.Workers, which card 19 registers.
In both runs the handoff was delivered, get_dispatch pulled the event, complete_dispatch recorded succeeded with the reply id, and the attempt ended finished with the ACP session id in the ledger. While the connector ran, a scanner compared every token-shaped string against the ledger's token hashes. It covered every process's environment and argv and every file in the state, working and session directories, and found no match (110 and 220 scans).
CI: this is a stacked PR, so test.yml doesn't run on its own; I dispatch it by hand on each head. It is green on this one.

Not in this PR

ACP as the default driver: that waits on card 22's harness passing against both adapters.
A doctor check for the adapters. acp.Locate is the check it will call.

Copilot

🟡 Changes recommended

Permission-cache poisoning and inherited Codex MCP configuration weaken the stated security boundaries.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an opt-in ACP driver for running connector workers through pinned Claude and Codex adapters, stacked on PR #738.

Changes:

Implements ACP v1 sessions, permissions, mode verification, cancellation, and lifecycle handling.
Pins and locates ACP adapters with compatibility testing.
Adds connect --driver acp and adapter-directory configuration.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries

File	Description
`Makefile`	Adds adapter installation and compatibility-test targets.
`internal/connector/driver/acp/testdata/stubmcp/main.go`	Provides the compatibility-test MCP server.
`internal/connector/driver/acp/session.go`	Implements ACP sessions, turns, updates, and permissions.
`internal/connector/driver/acp/rpc.go`	Implements newline-delimited JSON-RPC transport.
`internal/connector/driver/acp/fakeagent_test.go`	Provides a scripted fake ACP agent.
`internal/connector/driver/acp/compat_test.go`	Exercises pinned adapters against real models.
`internal/connector/driver/acp/adapters/package.json`	Declares pinned adapter dependencies.
`internal/connector/driver/acp/adapters/package-lock.json`	Locks adapter dependency versions.
`internal/connector/driver/acp/adapters.go`	Defines adapter configuration and discovery.
`internal/connector/driver/acp/acp.go`	Implements the ACP driver lifecycle.
`internal/connector/driver/acp/acp_test.go`	Tests ACP invariants and lifecycle behavior.
`internal/commands/connect_run.go`	Wires ACP selection into `connect`.
`internal/commands/connect_run_test.go`	Tests ACP driver selection and location.
`.surface`	Records the new CLI flag.
`.naming-allowlist`	Excludes lockfile hashes from naming checks.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Suppressed comments (1)

internal/connector/driver/acp/compat_test.go:77

Unknown values in BASECAMP_ACP_CHECKS are silently skipped, so a typo such as BASECAMP_ACP_CHECKS=6 reports success without running a check. Fail on an unknown check name instead.

				check, ok := checks[strings.TrimSpace(n)]
				if !ok {
					continue

Files reviewed: 14/15 changed files
Comments generated: 7
Review effort level: Balanced (auto)

Note

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

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

Copilot

🟡 Changes recommended

Shutdown can hang on escaped descendants, and configuration/error sanitization has security gaps.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Suppressed comments (1)

internal/connector/driver/acp/session.go:593

The handshake-failure path has the same unbounded stdout wait: an adapter can start a descendant outside its process group before the handshake fails, and that descendant can retain the pipe after Terminate(0). Close the local stdout reader after termination so abort always returns.

		s.worker.Terminate(0)
		<-s.readerEnd

Files reviewed: 14/15 changed files
Comments generated: 3
Review effort level: Balanced (auto)

Note

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

Copilot

🟡 Changes recommended

Permission cancellation races, a bypassable Codex preflight, and unreliable timeout and compatibility checks remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Suppressed comments (2)

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

internal/connector/driver/acp/compat_test.go:448

This compatibility check can falsely pass on macOS, which this connector supports: sha256sum is not part of the default macOS toolchain. The pipeline then creates an empty host-probe.txt, and the later inequality check treats that as proof that the token did not leak. Use a portable probe and require a valid 64-hex-character digest before comparing it.
internal/connector/driver/acp/session.go:424
Prompt does not observe ctx until after it acquires promptSem and completes the synchronous pipe write. If another write holds the semaphore, or the adapter stops reading and this write fills the pipe, even an already-canceled context cannot make Prompt return as required by the driver.Session contract. Make semaphore acquisition context-aware and arrange for the turn/write to continue asynchronously when the caller's context ends.

Files reviewed: 14/15 changed files
Comments generated: 2
Review effort level: Balanced (auto)

Note

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

Copilot

🟡 Changes recommended

Cancellation persists across turns, and valid TOML syntax can bypass the foreign MCP-server preflight.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Files reviewed: 14/15 changed files
Comments generated: 3
Review effort level: Balanced (auto)

Note

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

Copilot

🟡 Changes recommended

Permission-name normalization and unbounded request handlers introduce security and reliability risks.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Suppressed comments (1)

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

internal/connector/driver/acp/compat_test.go:450

This compatibility check silently passes on a stock macOS host, where sha256sum is unavailable: the failed producer still feeds cut, which creates an empty host-probe.txt, and the empty digest compares unequal to the host token. Use the native shasum -a 256 fallback and fail the probe if neither utility exists so the supported macOS path actually detects token leakage.

Files reviewed: 14/15 changed files
Comments generated: 2
Review effort level: Balanced (auto)

Note

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

Copilot

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Note

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

The agent boundary is ACP v1's session model: a Driver opens or reloads a session in a working directory with explicit MCP servers, a Session takes prompts that return a stop reason, streams content-free updates, cancels a turn, and answers permissions through a policy. The Claude Code spawn driver adapts `claude -p` stream-json onto it, with the policy frozen into flags and the permission mode verified on the init message. The ledger gains attempts and the rest of a task: launching is written in the transaction that exposes the originating event, a proven spawn failure withdraws the exposure once, and ending an attempt supersedes the token, settles every event and ends the task in one transaction, with hooks for the lifecycle outbox inside each transition.

…er is read A handshake that fails after the adapter started returns a driver.StartError carrying the adapter's process, as the contract now asks, so the connector confirms the group gone before it settles the attempt. The environment test uses drivertest's secret checks: the task token is in no file of the working or private directory at any moment, nor in the adapter's environment or command line. A prompt's answer marks its turn settled on the reading goroutine, before anything read after it is admitted, so a request that follows the answer on the wire is outside the turn however soon the turn's own goroutine runs. The fake agent's record lives apart from the working directory it serves.

Compatibility check 7 serves a dummy task token on the connector's one-use socket, starts each adapter with the worker-mcp bridge as its MCP server, and names the worker's group only once NewSession returns, as the dispatcher does. Both adapters reach the socket: the handoff is delivered. The token is in no environment or command line of any process descended from the adapter, and in no file of the working, private or state directory. Put back into mcpServers[].env, the check goes red on both, and on claude-agent-acp the token shows up in the Claude CLI's argv.

The host-token probe is a script the worker runs, so what is under test is what its shell holds rather than how well a model retypes a pipeline, and it uses sha256sum or shasum, whichever the machine has. A probe that is not a digest — neither tool, or an empty pipeline — is an error rather than proof that the token was absent; on stock macOS the old check would have passed without hashing anything. make acp-adapters installs with --engine-strict, so an adapter whose Node requirement this machine does not meet fails the install rather than the first dispatch.

A session whose Basecamp MCP server never connected ran anyway: the worker lost the tools and the task token the connector meant it to have, its turn ended cleanly, and the attempt settled as finished with the mention unanswered. The token bridge counts on that not happening — a bridge that gets no token is meant to show up as a server that did not connect. The adapters do say so, each in its own way, and the driver now reads it: claude-agent-acp forwards Claude Code's init when the session asks for it, and its MCP server statuses are read from that message and nothing else; codex-acp reports a server that failed or was canceled at startup as a failed mcp_startup tool call. A server that is not connected, or a Claude turn that ends with no init at all, fails the turn with ErrMCPServerNotConnected and ends the worker. Compatibility check 7 proves it live on both adapters: the bridge's own `basecamp mcp` cannot authenticate there, so the agent reports the server failed and the turn is refused rather than run. It also waits for the bridge to become `basecamp mcp` before its last walk of the worker's processes, so the process holding the token is among those checked. Requests refused at the handler bound are answered off the reading goroutine, so an agent that floods them while it has stopped reading its input cannot stall what the client reads.

Refusals at the handler bound are written off the reading goroutine, and that queue is bounded too. Dropping past it left the agent's requests unanswered for ever, which is how CI found it: a flood of sixty never got its sixtieth answer. The queue now holds what any real agent asks, and an agent that outruns even that has stopped working with this client, so the session ends instead of waiting on it.

…nth review Everything this session says now passes through the driver package's redactor, built from the dispatcher's redaction plus the environment the driver builds, its MCP servers' environments and its private directory: errors, update ids and names, refusals, and the adapter's stderr tail. Each refusal is recorded through SessionConfig.Refusals as it is made, once per tool call id, so the ledger holds it rather than a session's memory. From the review: a session is given its MCP status and server names before its reader starts rather than after; the failure that ends a session is claimed under the lock that saw the reason, and is what a failed handshake reports; an init that names a server the session never gave fails it too, and a codex startup failure names the server codex named. `make vet` now builds the compatibility test, which nothing did.

ErrMCPServerNotConnected wraps driver.ErrSessionUnverified, so a session that is not the one the connector asked for settles as failed whichever driver ran it. The redaction case (drivertest.RequireRedacted) covers this driver's five error paths, and found updates carrying the agent's own ids and names unsanitized. The refusal case holds the ledger rule: each refusal recorded as it is made, once per tool call id. An init that names a server the session never gave it fails the session too — the agent's own account is the cheapest proof that strictMcpConfig and the Codex preflight held — and a failure while the session is opening is what the start reports, rather than the closed stream it caused.

…ng ids Three from the review. A Codex startup report naming a server the session never gave it is evidence that strictMcpConfig and the preflight did not hold, so it ends the session whether or not the report is the agent's whole account. An init that arrives before the session's own id does is held until the id is known and applied only if it named this session, so an init for another session can no longer vouch for this one. And a refusal is deduplicated by a digest of the id the agent sent, rather than by the cut and redacted id shown, so two long ids are two refusals in the ledger.

Two things, and the first is why the second exists. The eleventh review's findings, which were still in my tree when the move below was made: the refusal of a request turned away at the connection's handler bound is now recorded off the reading goroutine (the busy queue carries the request, not just its id); the map of refusals a session remembers having recorded is bounded, deduplicated per turn by a digest of the id the agent sent, and a refusal with no tool call id is recorded every time rather than folded into one; a permission request too malformed to read is a refusal of this driver's, recorded and emitted as one; the wait for the reader is bounded by the close grace; a failure claimed while the handshake was returning means the session is never handed out and its group is confirmed gone; and the dispatcher can ask for the adapter's stderr tail. Then the consolidation. Each of those was a new site of a rule already written down somewhere else in the package, which is what kept producing rounds. The three rules that are rules rather than single checks now each have one home, stated once at the top of it: mcp.go for the MCP isolation boundary (what is declared, what the adapter must not add, what actually connected), permission.go for who may decide a permission and on what evidence — which fields of a request are trusted and which an adapter can forge — and limits.go for what bounds every buffer, naming each bound per line, per session, per turn, per tool call, at once and in time. That part moves code and changes no behaviour. The package's invariants are renumbered (the unusable-configuration rule had been written as 6a beside a second 6), and vet now runs over the acpcompat-tagged test too, which nothing else builds.

Reading the three doc blocks back against the code found three claims that were looser than the code. The updates channel had its size written as a literal beside a doc block that claims to name every bound, so it is now updatesBuffer in limits.go. A tool call id is not bounded everywhere, only wherever it is kept or shown, and digested where once-ness is decided; the permission block says that instead. And an MCP server's environment is not the only thing that decides what it inherits — what the adapter itself was given is, and that is the allowlist held in Driver.open, which the MCP block now points at rather than claiming twice.

…e gated The eleventh adversarial review and Copilot's pass on the last head found the same blocking defect independently, and eleven more between them. Every one was a second path answering a question this package already answered somewhere else, so each fix is the two paths brought together rather than a patch at the new site. What bounds what the agent writes. An account of the MCP servers that arrives before the session's id does was held in a map the agent filled: any number of ids, each as long as it liked, each account as wide. Two minutes of a pending session/new is tens of gigabytes, and a connector killed for memory mid-handshake leaves the adapter's process group behind. An account is now held only for an id this session could be given, at most maxEarlyInit of them, and reduced first to what judging it needs: the servers the session gave, and the one name it did not, which is what fails it. A tool call's paths were bounded in number and not in bytes; a path is now cut to what a pathname can be, keeping the leading part the policy judges. The mode the agent reports is bounded too. What a permission decision may rest on. A request reaches the policy only inside a turn the agent has not answered, but what the session knew of a tool call was taken from any update at all, a load's replayed history included. A replayed call could name mcp__basecamp__* under an id, and a later request naming that id alone inherited the name, which the policy allows by prefix. One predicate, mayAskLocked, now gates both, and a refusal turned away at the connection's own bound carries the turn it was read in rather than finding one later. Duplicate option ids now select nothing: a list that gives one id to two options says nothing about which the agent will act on. What may reach a session's MCP servers. The Codex preflight read for a key at the start of a line, so mcp_servers in an inline table passed it, and it treated a config it could not read as one that was not there. It now refuses on the name anywhere in the file and on a file it cannot read, and it reads the environment the adapter will run with rather than the connector's. Two MCP servers of one name are ErrUnusable: one name in the agent's account cannot stand for two servers. Also: a second Cancel sends no second cancel; a prompt's write is off the caller's goroutine, so an agent that has stopped reading cannot hold it past its context; the compat test carries the constraint its helpers do, so vet with its tag compiles everywhere; and the check for a failure claimed as the session is handed out has a seam, because the test that claimed to hold it passed without it thirty times over. Compat check 8 answers what the adapters do when a session's MCP server dies: both re-run the server's command as a fresh process, claude-agent-acp in the worker's own process group and codex-acp in a group of its own descended from the worker's leader. Neither tells the client, so a death mid-session is seen by nothing here.

Rebased onto card 18's head. Two of its additions are this driver's too. A handshake that fails now carries every bounded line of the adapter's stderr rather than the last one: an adapter that cannot start says why on one line and prints a stack trace after it, and the last line of that trace explains nothing. And the directory holding the task token's socket joins the private directory in what a session's redaction removes from anything it passes on.

A model that answers without reaching for its tool tells nothing about the adapter, and which of the two happened is not visible from the client. The check now insists, twice, and says plainly that it cannot tell the two apart when it sees no restart.

Copilot's three findings on the last head, all real, and the first is a hole the last commit opened. The reduction that bounds a held account of MCP servers put every foreign name through the redactor's sanitizer, and the account was then judged by name: a server called "base\acamp" reads as "basecamp" once its control character is gone, so an account naming only that server, connected, could vouch for the server the session actually gave. A name the session never gave is now not kept as a name at all — only a flag and a sanitized reason for the error — and the account that reaches reportMCPServers is keyed by the session's own names, which are the one thing there that is not the agent's text. A permission request read in no turn now belongs to no turn. The lookup that found "the turn in flight" was right when a refusal could be made before its turn was read; with the claim carrying the turn, it could only attach a refusal to a prompt that began after the request was read — a refusal on a result nobody asked for, and an unsolicited canceled stop read as TurnRefusal. The ledger still records it. And a replayed startup failure fails a load, which is the safe way round and now says so where it happens: nothing on the wire tells codex-acp's replayed mcp_startup failure from the failure of the server this process just started, and a session that cannot be loaded is started fresh, while a startup failure taken for history would be a worker running without the tools it was given.

Card 18 found the same miscount on their side of this rule: a guard that compares an empty tool call id against an empty tool call id collapses every nameless denial into one. This driver already counts each one, in the ledger and on the turn's result; nothing held that it did. Two mutations, one per guard, now go red.

Copilot

🟡 Changes recommended

MCP status normalization and cross-turn tool metadata can bypass intended verification and permission boundaries.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Suppressed comments (1)

internal/connector/driver/acp/session.go:560

Starting a follow-up leaves s.tools populated by unfinished calls from the previous turn. An ID-only permission request that reuses such an ID inherits its old tool name; if that name is mcp__basecamp__..., the connector policy auto-allows the new request. Tool-call evidence must be scoped to the turn that announced it, so clear this cache when registering each new turn (or store the owning turn with every entry).

	s.turn = t

Files reviewed: 18/19 changed files
Comments generated: 1
Review effort level: Balanced (auto)

Note

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

+		if i := slices.Index(names, name); i >= 0 {
+			// Keyed by the session's own name, which is the one thing here
+			// that is not the agent's text.
+			held.statuses[names[i]] = s.conn.agentText(status)
+			continue
+		}


…part The twelfth adversarial review, four blocking findings, two of them holes the bounding commit opened. A call names paths and the policy allows it only when every one of them is inside the working directory. Cutting the list at maxLocations and cutting a path at maxLocationPath both threw away exactly what refuses a call: 64 paths in the working directory and a 65th in /etc allowed the call, and a long path that walks out of the directory allowed it too, because what was cut off was the walking out. Both are now one rule: a call whose paths this driver cannot carry whole is unplaceable, and an unplaceable call is refused without being asked, recorded like any other refusal. Proved against the real policy, not only this package's: Decide(all 65) is false and Decide(the 64 that fit) is true, which is the allow that fix prevents. A tool call that has finished is now forgotten whatever the session could be asked at that moment. Gating what the session remembers on mayAskLocked put the eviction behind the same gate, so a call that completed after its turn was answered was never forgotten, and its name — the adapter's, not the model's — was inherited by a request in the next turn that named only its id. mcp__basecamp__* is allowed by prefix, so that was an allow the policy never gave. Whose account of the MCP servers an init is, is now decided under one lock. The check that it named this session was taken under one acquisition and the decision to apply it under a later one, so an account read while the session's id was still unknown could be applied, unreduced, as this session's the moment the id arrived — one in twenty thousand rounds, and the whole of invariant 9 with it. Both paths now apply the same reduced account, so neither can be the lenient one. Smaller, from the same review: the option list an update carries is bounded, an error carries the adapter's last few stderr lines rather than fifty, a rejected session id goes through the redaction like every other agent text, and the two doc blocks that disagreed about what happens past the refusal queue now say what the code does. The fake agent that stops reading stops existing after ten minutes, so an interrupted run leaves nothing behind.

Copilot

🟡 Changes recommended

MCP configuration preflight bypasses and incomplete location validation leave security-sensitive invariants unenforced.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

internal/connector/driver/acp/adapters/package-lock.json: Generated file

Suppressed comments (3)

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

internal/connector/driver/acp/acp.go:233

Preflight errors bypass the session redactor because it is constructed below this return. For example, an unreadable MCP config under a session-provided CODEX_HOME includes that environment value in the returned error even though non-base environment values are explicitly redacted everywhere else. Build the redactor before preflight and sanitize this error too.
internal/connector/driver/acp/adapters.go:205
This walk follows the lexical cwd, but the adapter process observes the resolved working directory. For a route such as /work/repo -> /srv/org/repo, this checks /work/.codex but misses /srv/org/.codex; Codex can therefore load an ancestor MCP configuration that the preflight never inspected. Walk both the lexical and EvalSymlinks roots.
internal/connector/driver/acp/adapters.go:222
The escape check only examines text before the first =. Valid nested inline TOML such as profiles = { demo = { "mcp\u005fservers" = { basecamp = { command = "/bin/evil" } } } } contains neither the literal substring nor a backslash before the first assignment, so this preflight accepts configuration that decodes to mcp_servers. Parse keys structurally or extend the fail-closed scanner to cover inline-table keys without rejecting escapes in values.

Files reviewed: 18/19 changed files
Comments generated: 1
Review effort level: Balanced (auto)

Note

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

+			if json.Unmarshal(l, &loc) != nil || loc.Path == "" {
+				continue
+			}


The preflight and the adapter switches are what the connector asked for. After launch the session asks the adapter what MCP configuration it is actually running -- both pinned adapters answer their own /mcp themselves, with no model and no tokens -- and refuses the session unless that answer is the servers it was given, plus at most a server the pinned adapter brings whose tools the model is never offered. One check, in one place, on what it got rather than on what it was handed.

#736 squash-merged, so the 33 commits this branch shares with it are one commit on main and a rebase would replay every one of them against itself. The parent is merged in instead and the four conflicts resolved at the tip. connect_run.go and connect_run_test.go are add/add on text that is identical either side but for the acp driver this branch adds, so the branch's own version stands. ledger.go and ledger_tasks_test.go this branch never touched beyond what it carries of the parent's, so the parent's version stands whole -- which is how the ledger keeps main's migration 6, the acknowledgement trigger, with the dispatcher's tasks and attempts renumbered to 7 behind it. This branch adds no migration of its own.

jorgemanrubia · 2026-09-18T10:19:22Z

🤖 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.

github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) labels Sep 17, 2026

jorgemanrubia requested a balanced review from Copilot September 17, 2026 07:02

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 07:03 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

jorgemanrubia force-pushed the connect-dispatcher branch from 9428454 to 7866973 Compare September 17, 2026 07:19

jorgemanrubia force-pushed the connect-acp-driver branch from afbe984 to 51d1aea Compare September 17, 2026 07:26

jorgemanrubia requested a balanced review from Copilot September 17, 2026 07:26

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 07:27 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread internal/connector/driver/acp/adapters.go Outdated

Comment thread internal/connector/driver/acp/session.go Outdated

Comment thread internal/connector/driver/acp/rpc.go Outdated

jorgemanrubia requested a balanced review from Copilot September 17, 2026 07:35

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 07:36 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread internal/connector/driver/acp/adapters.go Outdated

Comment thread internal/connector/driver/acp/session.go Outdated

jorgemanrubia force-pushed the connect-dispatcher branch from 7866973 to b5efc24 Compare September 17, 2026 07:44

jorgemanrubia force-pushed the connect-acp-driver branch from ec00695 to e4d1e0f Compare September 17, 2026 07:57

jorgemanrubia requested a balanced review from Copilot September 17, 2026 07:59

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 07:59 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread internal/connector/driver/acp/adapters.go Outdated

Comment thread internal/connector/driver/acp/session.go Outdated

Comment thread internal/connector/driver/acp/compat_test.go Outdated

jorgemanrubia mentioned this pull request Sep 17, 2026

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

Closed

jorgemanrubia requested a balanced review from Copilot September 17, 2026 08:14

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 08:14 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread internal/connector/driver/acp/rpc.go Outdated

Comment thread internal/connector/driver/acp/session.go

jorgemanrubia force-pushed the connect-acp-driver branch from 728c3a8 to 5b7f6ce Compare September 17, 2026 08:24

jorgemanrubia requested a balanced review from Copilot September 17, 2026 08:47

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 08:48 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

jorgemanrubia force-pushed the connect-acp-driver branch from 1fb8154 to e942fe8 Compare September 17, 2026 09:16

jorgemanrubia force-pushed the connect-dispatcher branch from dcda11b to b3742e8 Compare September 17, 2026 09:43

jorgemanrubia force-pushed the connect-acp-driver branch from e942fe8 to 2a5c53e Compare September 17, 2026 10:03

jorgemanrubia added 15 commits September 17, 2026 17:16

jorgemanrubia force-pushed the connect-acp-driver branch from 6119f20 to 439311a Compare September 17, 2026 15:17

jorgemanrubia requested a balanced review from Copilot September 17, 2026 15:17

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 15:18 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread internal/connector/driver/acp/mcp.go

Comment on lines +209 to +214

if i := slices.Index(names, name); i >= 0 {

// Keyed by the session's own name, which is the one thing here

// that is not the agent's text.

held.statuses[names[i]] = s.conn.agentText(status)

continue

}

jorgemanrubia requested a balanced review from Copilot September 17, 2026 15:30

Copilot started reviewing on behalf of jorgemanrubia September 17, 2026 15:31 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread internal/connector/driver/acp/session.go

Comment on lines +937 to +939

if json.Unmarshal(l, &loc) != nil || loc.Path == "" {

continue

}

jorgemanrubia force-pushed the connect-dispatcher branch from 0fc79b0 to caab8f1 Compare September 17, 2026 15:47

jorgemanrubia added 2 commits September 18, 2026 09:19

jorgemanrubia mentioned this pull request Sep 18, 2026

Six stacked pull requests, landed as one #748

Merged

jorgemanrubia deleted the branch connect-dispatcher September 18, 2026 10:19

jorgemanrubia closed this Sep 18, 2026

Conversation

jorgemanrubia commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Where the rules live

Invariants

What the adapters taught, beyond the spike

Review

Evidence

Not in this PR

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (1)

Suppressed comments (3)

Uh oh!

jorgemanrubia commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jorgemanrubia commented Sep 17, 2026 •

edited

Loading