Repository navigation
Connect ACP clients to a live REPL Sidekick - #989
Conversation
PR #989: Connect ACP clients to a live REPL Sidekick36 files, +4397 / -213 Scope🔴 PR has 4610 lines changed. Split into focused PRs. 🟡 4610 lines changed. PRs under 400 receive more thorough review. 🟡 36 files changed. Are all changes related? 🟡 PR mixes config and source changes. Structural🟡 3 console statements. Oxlint structural signals:
SlopOxlint slop signals:
Static AnalysisOxlint: 177 diagnostics across 13 files (20 rules) no-shadow (71): packages/cli/src/repl/sidekick/dispatch.ts, packages/cli/src/repl/sidekick/history.ts, packages/cli/src/repl/sidekick/model.ts (+5) CorrectnessNo extraneous code patterns detected. |
d9c4380 to
e016900
Compare
Carries the conflict resolution of 806dc51, which merged the earlier TestAgent delivery into the attachment branch: the completed-turn row joins the turn's event stream before reading its result, because the result settles before the final store writes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every saved Sidekick message carries an opaque ID and a UI or attachment origin. Both are required: a record missing either is unreadable rather than read as an earlier shape, and nothing projects a substitute origin for one. The chat item carries the ID and origin the record holds, so the live attachment's own saved message is found by that ID alone. There is one supported format for this data, so the stored records move to it in place: the two pinned chat fixtures gain those fields on their message records, and each receipt restates its own byte count and digest. The architecture and REPL specification say the fields are required. Formatting of the files this feature already touches is brought to oxfmt's output, which `deno task lint` checks.
806dc51 to
40be3e3
Compare
Three failures CI demonstrated on 40be3e3, each at a boundary the focused selection had not reached. An error thrown out of the Sidekick owner is an authority — persistence, the files provider, disclosure — ending the command, not one message refusing. The shared dispatch caught every throw and reported it as a failed exchange, so a fatal authority failure stopped nothing: the REPL stayed up and `REPL.Test retained and fatal outcomes` waited for a resolution that could never come. The producer now records the failed result for this exchange's observers, releases the reservation, and raises the error again to the owner it belongs to. Ordinary refusals still arrive as a failed Result and still end at the chat. The attachment bridge imports `@agentclientprotocol/sdk`, which only `packages/cli/deno.json` declared. There are two dependency layouts: Deno resolved it and pnpm and Bun never linked it under `packages/cli`, so every Node and Bun invocation of this CLI failed to load the module. It is declared in `packages/cli/package.json` at the one version `deno.json` pins, with the lockfiles that declaration implies, and a boundary case holds the two manifests together. The A5 attachment case reached for `Promise.withResolvers`, which the Node project's ES2022 library does not carry, failing `tsc` before any Node shard ran a test. It builds the one raw promise its callback needs without an ES2024 builtin.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // which arrive as a failed Result: it is an authority — persistence, | ||
| // the files provider, disclosure — saying this command is over, so it | ||
| // reaches the owner of this producer rather than ending here as one | ||
| // message's failure. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // message's failure. |
The attachment case that proves the bridge against the real released client located it at `node_modules/acpx/package.json`, a path under the repository root. Nothing at the root declares ACPX: `packages/acp` and `packages/test-agent` do, and each runtime's installer places a declared package differently. Deno's install links it at the root and pnpm's links it beside the declarer, so that path resolved under both. Bun's install does neither, so both rows that establish client provenance failed on `lstat` before reaching the bridge. The resolver answers instead of a guess: a require rooted at `packages/acp/package.json` resolves `acpx/package.json`, which ACPX exports, and every layout that package can resolve under is a layout this finds. Nothing else about the rows changes, and the vendored snapshot stays the only ACPX production reaches.
| // which arrive as a failed Result: it is an authority — persistence, | ||
| // the files provider, disclosure — saying this command is over, so it | ||
| // reaches the owner of this producer rather than ending here as one | ||
| // message's failure. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // message's failure. |
| ) { | ||
| throw new Error("The attachment file must be a private regular file of at most 16 KiB."); | ||
| } | ||
| // The open descriptor, rather than a second path read, fences symlink replacement. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // The open descriptor, rather than a second path read, fences symlink replacement. |
| }); | ||
| try { | ||
| const temp = yield* until(realpath(tmpdir())); | ||
| // Cancellation must join creation and identity before removing the owned directory. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // Cancellation must join creation and identity before removing the owned directory. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Permanent reviewer: correctness, performance impact, minimalism
Reviewed the exact published head 8cf8756c1c40c64441714631cd4aa406a2368988 against base 71083a5093e52fa2a2e95cf3b6c0fc7669c13ffd (36 files, +4283/−213), read against AGENTS.md, the Effection v4 contract, architecture.md (State ownership, REPL kernel, Local Sidekick attachments, Materialization) and the touched sections of specs/repl-spec.md and specs/acp-client-spec.md. Required green is reported passing for this head (https://github.com/taras/executable.md/actions/runs/40411665313).
Verdict: no blocking defect found. The accepted behaviour holds on direct trace: one root-owned dispatch spans each exchange (packages/cli/src/repl/sidekick/dispatch.ts), UI Send/Retry/Stop and the ACP bridge share the same reservation and admission, origin is captured lexically before the producer spawns, acceptance is published only after the message append is acknowledged, exact-handle cancel joins the owner's halt and an old handle cannot stop a successor, fresh --sidekick-acp startup joins one real initialization turn (and Control-C/EOF during it return through the normal exit) and existing selected history reuses its conversation. The POSIX transport creates every native resource after the cleanup that releases it and removes only names whose identity it still owns.
Three findings, each as an inline comment with evidence and a concrete recommendation:
- Correctness (narrow, non-blocking):
packages/cli/src/sidekick-acp/posix.tsremoves theconnectionlistener in the resource body'sfinally, before the asynchronous teardown reachesserver.close(). A peer that connects in that window is counted bynet.Serverbut never tracked or destroyed, soclose()cannot drain until that peer goes away. One-line fix. - Performance (measured, conditional):
packages/cli/src/cli.tsnow statically imports the bridge, so everyxmdinvocation loads@agentclientprotocol/sdk. Under Node the SDK import costs roughly 60–70 ms wall, about 40 ms beyond the zod the CLI already loads. A focused base-vs-head measurement ofxmd --helpis identified in the comment; a lazy import behindxmd sidekick acpremoves the cost if confirmed. - Minimalism: two filter+map pairs in
packages/cli/src/sidekick-acp/server.tscarry unreachableunknown/nullbranches thatflatMapremoves, which also lets the bridge'sloadedschema drop theunknownvariant and itscontinue.
Accepted consequences noted, not findings: requiring id and origin on every saved message record (commit 40be3e3) makes a sidekick.jsonl written by an earlier build unreadable rather than read as an older shape; that is the ruled current-format policy and the two pinned fixtures were migrated in place with restated digests. A fresh --sidekick-acp start draws no frame until the initialization turn completes; interrupts still work, as the bootstrap shutdown cases show.
Evidence limits: I traced source and the retained review artifacts and ran one read-only module-load timing; I re-ran no product tests, installs or link changes, and the local timing is not a clean-install or compiled-binary number. verify:clean, a current compiled candidate and external provider smoke remain delivery work owned by the existing delivery actors; this review does not approve or integrate.
| }, | ||
| }); | ||
| } finally { | ||
| server?.off("connection", connected); |
There was a problem hiding this comment.
Correctness (narrow): a connection accepted during teardown can stall server.close().
This finally runs when provide() unwinds, which is before the ensure() registered at line 300 starts (Effection runs the task body's own finally first, then the scope destructors — createTask wraps the body as try { … } finally { destroy }). Between this off("connection") and the held.close() at line 312 the teardown suspends on Promise.allSettled(acquisitions), directoryAcquisition and one closeSocket() per tracked socket, while the listener is still bound to the socket path. A peer that connects in that window is still accepted and counted by net.Server (onconnection increments _connections whether or not a listener exists), but connected no longer runs, so the socket is never added to sockets and never destroyed. server.close(cb) then waits for that peer to disconnect, and the peer's wire.receive() is itself waiting for a hello reply with no timeout, so command exit blocks until the client side gives up.
A realistic trigger is the bridge's session/cancel handler (bridge.ts line 312), which opens a fresh connection right after its in-flight prompt connection is closed by this very teardown; cross-process timing makes it unlikely but not impossible, and the consequence is a hung REPL exit.
Recommendation: delete this server?.off("connection", connected) line and keep only the one at line 309, which runs synchronously immediately before held.close() with no suspension in between, so every connection accepted until the listener closes is tracked and closed by the loop at line 305 (a Set iterates entries added during iteration). The off("error", serverFailed) on the next line is fine to keep here. Verification: the existing A5/A6: enclosing owner teardown case still passes; a focused regression would connect once from inside a wrapped closeSocket during teardown and assert the owner scope exits.
| @@ -1,4 +1,6 @@ | |||
| import { resolve } from "node:path"; | |||
| import { runSidekickCommand, SIDEKICK_HELP } from "./sidekick-acp/command.ts"; | |||
There was a problem hiding this comment.
Performance (measured, conditional): every xmd command now loads the ACP SDK at startup.
This static import chain is cli.ts → sidekick-acp/command.ts → sidekick-acp/bridge.ts → @agentclientprotocol/sdk, so xmd run, xmd --help, the credential helper path and so on pay for the SDK module graph even though only xmd sidekick acp uses it. Read-only timing on this head (Node v26.5.1, arm64 macOS, prepared worktree, three runs):
import('@agentclientprotocol/sdk') 69.4 ms / 59.2 ms / 66.1 ms
import('zod') alone 22.6 ms (already a CLI dependency)
deno run packages/cli/src/deno.ts --help 0.88 s / 0.54 s / 0.52 s (whole command, head only)
So the marginal cost is on the order of 40 ms per process under Node source; I did not measure base, the compiled binary or Bun, so treat this as a bound to confirm rather than a regression number.
Recommendation: measure xmd --help base vs head under Node and the compiled binary (for example hyperfine --warmup 3 'node packages/cli/src/node.ts --help' on each, or the compiled xmd --help). If the delta is confirmed, load the bridge lazily behind the sidekick acp branch of runSidekickCommand with a literal dynamic import("./bridge.ts") adapted through until() — the CLI already does this for the workflow host (packages/cli/src/workflow.ts line 1045), so deno compile and knip handle the pattern. protocol.ts/client.ts can stay static; they only pull zod. This changes no behaviour and keeps the manifest-boundary regression in repl-boundaries.test.ts valid.
| ? { kind: "person", text: one.text } | ||
| : one.kind === "reply" | ||
| ? { kind: "reply", text: one.text } | ||
| : { kind: "unknown", text: "" }, |
There was a problem hiding this comment.
Minimalism: two filter+map pairs carry branches the filter already made unreachable.
replay() filters to person/reply and then maps with an unknown fallback that can never be produced; the bridge mirrors it with an "unknown" member in the loaded schema and a continue (bridge.ts lines 37 and 125–127). status().messages (lines 114–125) does the same with a null branch, plus an one.id !== undefined test that is always true now that id is a required string on the chat item.
Recommendation (behaviour-preserving):
function replay(root: AttachmentRoot, descriptor: Descriptor) {
const read = root.owner.reading;
if (!read.ok) {
throw read.error;
}
return read.value.chat.flatMap((one) =>
one.conversation === descriptor.conversation && (one.kind === "person" || one.kind === "reply")
? [{ kind: one.kind, text: one.text }]
: [],
);
}messages: read.value.chat.flatMap((one) =>
one.kind === "person" && one.conversation === descriptor.conversation
? [{ execution: descriptor.execution, conversation: one.conversation, id: one.id }]
: [],
),Then drop "unknown" from the bridge's loaded.replay enum and the if (item.kind === "unknown") continue;. The wire shapes are unchanged: the current code never emits unknown or null, and the attachment test's statusSchema already requires non-null entries. Minor, same spirit: application.ts, model.ts and owner.ts reference import("./history.ts").MessageOrigin inline where the rest of this change (dispatch.ts) uses a top-level import type.
Review 5660106220 on PR 989 traced a window in the POSIX attachment's teardown. The resource body's finally detached the connection handler, and that block runs before the teardown it encloses: the listener was still accepting while cleanup awaited its acquisitions and closed the sockets it knew about. A peer arriving in that window was accepted and counted by the listener, tracked by nobody, and destroyed by nothing, so `close()` could not drain and the command stayed up until that peer hung up. Only the error handler comes off there now; the teardown already takes the connection handler off synchronously, in the statement before it closes the listener. The replay and status readings say what they mean with `flatMap`: both filtered to person and reply rows and then mapped with branches those filters had already excluded, including one that invented a message with an empty body. The bridge's replay schema drops the `unknown` kind it could no longer receive, and the saved-message reading drops a test for an ID that every record now carries. `MessageOrigin` is imported where its three readers declare it.
The listener correction had only a direct trace behind it. This is the observation the A5 journey asks for: a peer accepted after teardown begins stays owned, closes, and lets the owner join. The case publishes a real private attachment, has the listener accept one peer, then begins halting the owner and stops inside that peer's own cleanup, immediately before its close. The listener still exists at that barrier and the owner has not joined. A second peer connects there, and the listener's own acceptance of it — not a client connect, which proves only that the kernel queued something — is what the case waits for. Releasing the barrier must then join the owner and must close that late peer without the fixture touching it, leaving no descriptor, socket or socket directory behind. Restoring the early handler removal fails it on the late acceptance, so it discriminates the defect rather than the trace. Holding that boundary takes one optional operation on the transport, invoked inside the accepted socket's existing cleanup just before the close it already owned. It is passed by that one case; the four runtime entrypoints keep calling the transport as they did, and the close stays where it was. Watchdogs only fail the case; they order nothing, and production gains no timeout.
Why
Connect an ACP client to the Sidekick already held by a live REPL, so it can send a text request and inspect the resulting public activity and entry evidence.
This is a dependent draft on #988. Its initial head is
806dc51cde6b9ad32b63cfaeeb8f39a17fea786e; the selected parent branch isagent/testagent-model-options-984-deliveryatd9c43800b491271a1ad07f9cda24caa66ea8c830.What changes
Before: the REPL's Sidekick has no external ACP attachment or passive command-line read path.
After: start or resume a REPL with an established selected Sidekick conversation and opt in to a private attachment file. Use an existing owner-controlled parent directory:
xmd repl 'xmd://repl/<execution>/entries' --sidekick-acp /absolute/private/attachment.jsonConfigure an ACP client to launch this stdio backend:
A separate command can read current public status without sending a prompt:
xmd sidekick read --attach /absolute/private/attachment.json statusMessage reads can follow an acknowledged message reference; entry reads return bounded Source, Prompt, Output or Journal pages. The attachment creates no underlying conversation and keeps the selected provider and permissions.
How it works
The opt-in endpoint uses a same-user POSIX Unix socket and a private descriptor bound to the live owner, execution, conversation and cwd. Strict requests and bounded observer queues keep malformed or slow clients from blocking the producer.
Disconnect detaches observation while the REPL continues its exchange. Explicit ACP cancel/close captures and joins only that attachment's exchange. Busy work refuses before a message is saved. External messages retain origin and an acknowledged opaque ID without clearing the human's composer or taking over a held draft.
Review guide
Start with
specs/repl-spec.md, “Attaching to the live Sidekick”.Then review:
packages/cli/src/repl/sidekick/dispatch.tsand its application integration for shared admission and producer ownership.packages/cli/src/sidekick-acp/for descriptor identity, bridge mappings, safe reads and exact cancellation.repl-sidekick-acp.test.tsandrepl-sidekick-dispatch.test.tsfor the attachment and shared-dispatch regressions.Look carefully at completed-load replay, disconnect versus cancellation, replaced-file cleanup, and a failed follow-up after an earlier answer.
What must stay true
end_turncannot establish a missing completion.The dispatch, attachment, lifecycle, control and repository regression coverage checks these boundaries.
How to verify it
Recorded focused evidence passed on the candidate whose complete source corresponds to this exact head: the eight-file attachment/help/boundary/lifecycle/control/observation/repository/Session-configuration selection reported 39 tests, 186 steps, zero failures, with two real-child summaries of one test and six steps each.
The controlled integration exercises the released ACPX 0.12.0 CLI and XMD's distinct patched provider/runtime, including serial requests, passive reconnect/load, cancellation and incomplete-result controls. The first selection exposed an A6 fixture teardown failure; the correction drains the real event iterator before closing the runtime. The first failure remains retained. A temporary-file collision is a scheduling inference, not an instrumented interleaving.
Both original independent source reviewers passed this exact head. One committed
deno task verify:cleanpassed on it; that proves dependency ownership and non-interference, separately from product correctness.These are recorded local results on Darwin arm64/Deno 2.9.7, not final CI results or a live operator Sidekick recovery trial. No tests were rerun to prepare this description.
Scope
Included: shared Sidekick dispatch, opt-in local ACP text/load/close support, passive status/message/entry reads, runtime adapter installation, and the associated help, specifications and regressions.
Deferred scope includes queueing, coediting, cross-client question answers, live relationship recovery, remote hosting and repository-wide ACP. The current
--observe-worktreespelling remains unchanged; its separate naming proposal has no human approval.Refs #961, #963.
New abstractions and dependencies
The private dispatch handle serves UI Send and ACP while keeping the producer under the REPL root. The private transport supports the ACP bridge and passive CLI reader without exposing the execution controller.
The CLI declares its direct
@agentclientprotocol/sdk@1.3.0import for the stdio bridge, using the already selected SDK version.deno.lockrecords that real dependency edge. ACPX and vendor bytes are unchanged; no generated browser or vendor output is added.Risks and remaining delivery gates
This remains a draft. Combined-source runner measurement and authentic artifact/weight integration are pending delivery work; this description claims no run ID, outcome or consumed artifact. Existing d9 measurements and CI retain their own commit provenance.
Required CI must prove the actual final published head and current main. Any resulting weight or integration commit needs affected evidence correspondence and both original final-head reviews. Final Planner metadata APPLY and independent VERIFY remain outstanding after the configured CI and artifact gates.
The endpoint is POSIX-only and requires an established selected conversation. Switching conversations invalidates the attachment. Controlled client evidence does not resolve the held live Sidekick's unknown feedback acceptance.
Scope confirmation