Skip to content

feat(acp): add steer support (session/inject) - #6192

Open
2BAB wants to merge 18 commits into
apache:mainfrom
2BAB:feat/acp-session-inject
Open

2BAB wants to merge 18 commits into
apache:mainfrom
2BAB:feat/acp-session-inject

Conversation

@2BAB

@2BAB 2BAB commented Oct 20, 2026 •

Copy link
Copy Markdown

Stacked on #6189 — the first commits of this branch are that PR's model work. What belongs to this PR is the five commits above it: the injection feature plus its review follow-ups.

What it does

ACP now supports steer: a client can send another user message into a turn that is already running, instead of waiting for it to finish.

One method, session/inject, with two modes:

  • steer — delivered at the next break point of the running turn (after the current model stream or tool call)
  • queue — delivered when the turn ends

Both answer with a messageId, which is also the identity the message keeps in session/load replay. The client can cancel before delivery with session/revoke_inject, which every agent that advertises inject must support.

The runtime already does the steering; this exposes it over ACP using the shape proposed in #1261 and #2043.

One Host change comes with it: turn.message.submit accepts an optional expectedTurnId, so admission can refuse a steer whose target Turn has ended instead of starting new work. The strict input decoder rejects unknown fields, so the compatibility epoch moves to 227 — an older Host and this Client must refuse each other rather than fail mid-steer.

Review follow-ups in this branch

  • revocation retracts the one queued entry by entryId instead of clearing the whole queue, and re-reads the Host queue before calling an id unknown — a cached projection lags the submit it is being asked about.
  • the ACP-facing connection forwards the live Host epoch instead of copying it at creation, so a Host restart does not leave admission signing with a dead epoch.
  • a steer carries the running Turn's identity into Host admission, so a Turn that ends in between answers -32010 instead of starting a second Turn. Review showed this cannot be declared as a compatible protocol extension, so the epoch moved to 227.
  • an idle steer is rejected with -32010 no_running_turn before admission; the Host used to start a hidden turn nobody could observe.
  • replay uses the row identity the client was given, so a steered message no longer changes messageId on session/load.
  • the child-process initialize expectation includes the inject capability.
  • a follow-up Turn's permission or elicitation request is answered instead of stalling: interaction authority follows observation, while the read-only marker keeps Stop and cancel bookkeeping with the Turn's owner.
  • a queued follow-up's assistant reply now streams live: the prompt's notify is kept for the Session, and a Turn adopted without an external context is observed read-only, so it forwards output without claiming interaction, Stop or cancel authority. A later load/resume upgrades that same observation instead of starting a second consumer.

Known gaps

  • No live delivery notification for injected user messages: the Host reports them on session/load replay only, and this adapter is one client per session.
  • A queued inject on a cold Session has no live output. session/new then inject { mode: "queue" } before any prompt, load or resume runs on the Host but streams nothing back: live delivery needs an attachment, and those three requests are what open one. session/load shows the reply afterwards.
  • Error shapes deviate from the draft proposal: an unknown and an already-delivered id both answer -32002, and session/cancel drops pending injects. Both are documented next to the module's scope note.

Tests

6 unit tests for the injection module, 358 across the ACP suites, tsc --noEmit clean.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude (Anthropic) authored the implementation, tests and commit messages in this branch, and Antigravity (agy) authored the read-only observation patch for live follow-up replies; the contributor reviewed the design, ran the verification and owns the change. Commits carry Generated-by trailers naming the tool that wrote them.

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Oct 20, 2026
@2BAB 2BAB changed the title feat(acp): expose mid-turn message injection (session/inject) feat(acp): let a client steer a running turn Oct 20, 2026
@2BAB 2BAB changed the title feat(acp): let a client steer a running turn feat(acp): add steer support (session/inject) Oct 20, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed aef48933cab7e764c9e2cad25be93e2223d343c7, focusing on the final injection commit relative to c31560b6bcef56da572508e3fb1d4af4d0f16de8. This is stacked on #6189; conclusions about its model-selection changes are recorded in that review.

I found three P2 issues in the injection commit:

  1. Revoking one injected message clears the entire pending queue. With a real Host process, revoking queued A also cancels queued B; revoking an unknown ID can likewise destroy pending messages before returning an error. The existing single-entry Host operation preserves B in the control.
  2. The stdio adapter snapshots the initial Host epoch. After a real Host restart and successful reconnection, fresh injections fail with outcome_unknown; forwarding the epoch through a getter restores admission in the control.
  3. The new initialize capability breaks the existing child-process integration assertion. Updating its expected capability shape makes that original test pass.

Validation: fresh dependencies and dependency patching, the normal clean root test build, workspace type checks, lint, format, ASF headers, and the protocol-epoch guard passed (epoch 225; no Host protocol change). The original ACP tests completed with 337 passed and 10 failed: one failure is the new injection-capability assertion above; the other nine compare configuration snapshots from the earlier model-selection commits. Additional probes exercised actual SDK/stdio routing, the production registry and Host message coordinator, a real Host process, cancellation state queries, and real Host replacement with the getter control. An attachment-rejection hypothesis did not hold in the real Host probe and is not reported as a finding.

Main remains e66a655e4f262c68205bbda6e76613311db2b3c2; the trial merge is clean, but I did not test a merged tree. At handoff, the only check reported for this head is the successful PR effort label, not a test CI run. I did not run the full repository suite, native desktop/Windows checks, or an external ACP client interoperability test. Please also add the contribution statement required by CONTRIBUTING.md, explicitly stating whether generative tools made a material contribution.

const connection = await this.#getConnection('queue.retract');
let retracted = false;
try {
const result = await connection.request('queue.retract', {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Retract only the requested injected message

queue.retract clears every queued steering and follow-up message; retractId identifies the mutation, not the message to remove. I reproduced this through actual ACP stdio and a running Runtime Host: inject queued A and B, then revoke A. The ACP call returns success, but Host execution queries report both A and B as cancelled. An unknown message ID also clears pending messages before returning unknown_message_id. Resolve the requested message to its queue entry and use the existing single-entry retraction operation, without mutating anything for an unknown or already delivered ID. A control using queue.entry.retract cancels A while B remains pending.

Comment thread packages/cli/src/acp/stdio-server.ts Outdated
};
// Message admission carries the Host epoch; the ACP-facing interface does
// not list it, so it is attached rather than widening every fixture.
return Object.assign(acpConnection, { hostEpoch: connection.hostEpoch });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the live Host epoch across reconnects

The reconnecting connection exposes hostEpoch through a getter, but this Object.assign captures its initial value permanently. After the Host restarts, requests use the new transport while inject and revokeInject still send the old originHostEpoch. I reproduced a real Host stop/start with the same ACP process: the connection successfully reports the replacement epoch, but a fresh session/inject fails with outcome_unknown. Changing only this property to delegate through a getter makes the same restart scenario admit the new message successfully. Forward the current epoch rather than copying it when the adapter is created.

resume: {},
close: {},
// Not in the v1 schema yet: the shape of `session/inject`.
inject: INJECT_CAPABILITY,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Update the child-process initialize expectation for the new capability

This changes the wire response, but acp-child-process.test.ts still expects sessionCapabilities to contain only list/resume/close. After a fresh normal root build, the existing “serves ACP without a Runtime Host and exits after stdin EOF” test fails on the added inject property. Updating only that expected capability shape makes the original child-process test pass. Please update this existing integration assertion as well as the agent/stdio fixtures so the repository test command can pass. The separate model-option snapshot failures belong to the earlier stacked commits.

@2BAB
2BAB force-pushed the feat/acp-session-inject branch from aef4893 to 8545886 Compare October 20, 2026 14:03

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review from aef48933cab7e764c9e2cad25be93e2223d343c7 to 85458867d54667c6595e6b7ca5918f6ab85761d3, focused on message injection and its follow-up fix. The merge-base remains a78901e9472b95e01adfc40923414e3dc5a60430; the four earlier model commits are unchanged. The added model-selection fix is covered by the updated #6189 review at 696943ea.

All three prior P2 findings are fixed. I found no additional P0–P2 issue in the injection changes.

  • Single-message revocation: session-registry.ts:622–634 resolves the requested message to its queue entry and uses queue.entry.retract with a separate mutation ID. Through actual ACP stdio and a real Host, revoking A cancelled A while B remained pending. Unknown, repeated, and already delivered IDs returned -32002; unknown/repeated revocations preserved the other pending message. Restoring only the old revocation method again cancelled both pending messages.
  • Host replacement: stdio-server.ts:77–78 forwards hostEpoch through a getter. After stopping and restarting the real Host with the same ACP process, new injections and a single-message revocation succeeded. Restoring the copied epoch made fresh injections fail with outcome_unknown again.
  • Initialize assertion: acp-child-process.test.ts:147–152 now expects the injection capability. The original child-process initialize test passes without a fixture override.

Fresh dependencies and dependency patches, the normal clean root test build, workspace type checks, lint, formatting, ASF headers, and diff/epoch checks passed. The original ACP test group ran 350 tests: 349 passed and one multipart Artifact test exceeded its 15-second client timeout. That unchanged test passed when rerun alone. Both additional real-Host probes passed; the two old-implementation controls failed at the expected checks.

No Host protocol files changed; the epoch remains 225. A trial merge into current main dc4515cd6e7637e8267d59cd4f71ad53f403f9e7 is clean; I did not build the merged tree. At handoff, exact-head CI 39435863422 and Windows ACP integration 39435863389 are in progress, so their results remain outstanding. I did not run the full repository suite or interoperability checks with an external ACP client application.

P3: the PR body still needs the CONTRIBUTING.md statement explicitly saying whether generative tools made a material contribution.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Follow-up review of 85458867d54667c6595e6b7ca5918f6ab85761d3, checking what an ACP client actually receives. Three additional P2 issues remain. This supersedes my earlier “no additional P0–P2” conclusion for this head. The earlier revocation, reconnect-epoch, and initialize-test fixes remain valid. The model-selection prerequisite is covered by the updated #6189 review.

  1. P2 — a running-turn steer replays under a different message ID (packages/cli/src/acp/session-event-mapper.ts:156). With a real ACP child process, official client SDK, production AiSdk backend, and local streaming provider, the steer was acknowledged with one messageId; after delivery, session/load emitted its matching text under a different ID. steeringEventId is the newly generated runtime event ID; the submitted identity is stored separately in refs.providerEventId. Preserve that submitted identity through transcript projection and replay. This finding is specific to steering: single queued messages, two queued messages, and idle-started messages all retained their returned IDs in my checks.

  2. P2 — an idle steer starts an unexpected, unobserved turn (packages/cli/src/acp/session-registry.ts:596–614). On a fresh session/new session, mode: "steer" returns success and starts model work. The client receives no live assistant reply, and a subsequent ordinary session/prompt fails with session_busy while that turn is running. The injection proposal referenced by this PR requires -32010 with reason: "no_running_turn" for an idle steer. Reject this case before admitting work; returning an error after starting the turn would leave the side effect in place. Idle queue is allowed to start a turn by the proposal, but its reply also lacks observation here.

  3. P2 — queued follow-up replies are not delivered in the normal new/prompt flow (packages/cli/src/acp/maka-acp-agent.ts:276, session-registry.ts:1768–1775). I held the first provider stream, queued a second user message, then released it. The original prompt returned end_turn; the Host started and completed a different turn, and the provider produced its answer, but the client received no live agent_message_chunk for that answer. A later session/load replay contained it. Calling session/load before the initial prompt made the same follow-up reply arrive live. The injection route needs a retained client notification context: #adoptTurn currently exits when no external observation context exists. This affects assistant replies, beyond the explicitly deferred live echo of injected user messages.

Validation: the normal clean root test build passed again. Six end-to-end scenarios covered idle steer/queue, running steer, one/two queued messages, and the pre-load observation control. They used production Host/runtime code and a real ACP process; model traffic went only to a controlled local HTTP provider. Both exact-head CI 39435863422 and Windows ACP integration 39435863389 pass. No Host protocol files changed; epoch remains 225. Current main is dc4515cd6e7637e8267d59cd4f71ad53f403f9e7; the previously checked trial merge is clean. I did not rerun the full repository suite, native desktop, or a third-party ACP client application.

messageId: message.id,
// The steering event id is the id `session/inject` returned;
// the stored row may carry its own id.
messageId: message.steeringEventId ?? message.id,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the submitted steer identity in replay

A real ACP client receives one messageId from a running-turn session/inject, but session/load replays that same text with a different ID. steeringEventId is minted by the runtime; lease.messageId is stored separately in refs.providerEventId and is not carried into this field. Preserve the submitted ID through projection so clients can correlate the accepted message with its delivery. Single and multiple queue-mode messages retained the correct IDs in the control; this finding concerns steering.

} catch (error) {
throw requestErrorFromRuntimeHost(error, 'turn.message.submit');
}
if (disposition === 'blocked') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reject idle steers before starting work

On a fresh session/new session, mode: "steer" returns a messageId and starts a real model request despite there being no running turn. No assistant reply is pushed to the ACP client; another session/prompt meanwhile fails with session_busy. The referenced injection proposal requires -32010 with reason: "no_running_turn". Checking only blocked accepts turn_started after the side effect has already occurred. Enforce the idle-steer precondition before admission; merely mapping the response to an error would still start the hidden turn.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Second, independent read of the two inject commits at 85458867, posted as a supplement to the review above. It agrees with the three earlier fixes and with the three P2s listed there; its broader replay-ID concern for queued messages did not reproduce client-side, so only the running-steer case stands. Additional non-blocking notes:

  • P3 — revoke may read a stale queue snapshot. #pendingEntryId prefers the attachment's cached snapshot.queue, which the Host refreshes after the submit response. An inject followed immediately by a revoke could get -32002 while the message is still queued. Falling back to an authoritative read when the cache misses would close this. Unreproduced.
  • P3 — revoke error shape vs the RFD. Unknown and already-delivered ids both return -32002 without a reason; the draft uses -32010 + already_delivered vs -32002 + unknown_message_id, and asks that repeating a revoke of an already-revoked id succeed. An in_flight entry is treated as unknown, and a Host operation_conflict surfaces as a generic -32603.
  • P3 — session/cancel drops pending injects. The stop fence discards queued steering and follow-up entries. The RFD recommends they survive cancel and asks agents to document any deviation; neither the capability nor the module docs mention it.
  • P3 — attachments. inject() maps content but does not run publishAcpPromptAttachments, so resource_link content goes to the Host as lazily read file refs, unlike session/prompt's upload-before-admission. Edits after the inject change what the model sees.
  • P3 — tests. No registry-level test of inject() dispositions (steering/followup/turn_started/blocked/inbox) or of the cached-snapshot branch of revoke.

@2BAB
2BAB force-pushed the feat/acp-session-inject branch from 8545886 to d9758d9 Compare October 20, 2026 23:22

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed d9758d9b6ab958779437d6346247e28b9a7a20a3 against the preceding reviewed injection implementation at 85458867d54667c6595e6b7ca5918f6ab85761d3. The first two injection commits are unchanged after restacking on the updated model-selection prerequisite; the added commit changes the in-flight revocation error mapping and epoch regression coverage. Three previously reported P2 issues remain, and a fourth is now reproduced. The prerequisite's no-LLM-connection selector fix was also independently verified at 36c1bc5c64730fd5936a88249077e2eb2c11ea28.

  1. Newly confirmed P2 — cached queue state rejects revocation of a pending message (packages/cli/src/acp/session-registry.ts:667). A previous supplementary comment raised this as an unreproduced P3; actual client execution now confirms the failure. With the real ACP CLI child process, official SDK, production AiSdk backend and a local streaming provider, I loaded a session, started a prompt, then injected and immediately revoked queued messages. 24/24 revocations returned -32002, while an independent Host query still reported each message as pending. The messages remain eligible for later execution despite the attempted cancellation. The attached snapshot is preferred even when it lacks the newly admitted message, so the lookup never consults the current Host queue. Changing only that lookup to reread the Host made 24/24 messages cancel successfully. Resolve the entry from current authoritative state before declaring it absent; keep the existing atomic entry retraction to handle delivery races.

  2. P2 remains — running-turn steering replays under a different ID (session-event-mapper.ts:156). The same six client scenarios from the preceding review still reproduce this: a steer was accepted as a7bd2798-9196-4625-9280-5afb1308ab21, then replayed under 415cf722-36ec-4c82-b2b8-df2874b59080. Single queued messages, two queued messages and idle-started messages retain their accepted IDs. This identity finding concerns running-turn steering.

  3. P2 remains — idle steering starts an unobserved turn (session-registry.ts:596–614). A fresh session accepts steer, starts real provider work without live assistant output, and rejects the next normal prompt as busy. The draft injection proposal still specifies -32010 / no_running_turn for this case. Enforce the precondition before admitting work. The proposal permits idle queue to start work; that behavior itself is not a finding.

  4. P2 remains — queued follow-up assistant replies are not pushed in the normal new/prompt flow (maka-acp-agent.ts:276 and the registry's external-turn observation path). After the original prompt returns end_turn, the Host completes a different queued turn and the provider produces its answer, but the client receives no live assistant chunk for it. Loading later replays the answer. The otherwise identical control that calls session/load before the first prompt receives that follow-up live. This concerns assistant output, beyond the deliberately deferred echo of injected user messages.

The new in-flight branch does eliminate an internal-error response: I paused the real Host queue after it had acquired a steering lease and before returning it to the test backend. The actual retraction returned operation_conflict; current ACP code answered -32002, while restoring only the prior predicate answered -32603. Single-entry revocation still preserves the other queued message, and stopping/restarting a real Host still permits fresh admission and revocation with its new epoch. The lease-race probe uses the test backend and a scheduling pause; the six client scenarios and 24-message cancellation comparison use the production AiSdk backend without that pause.

Validation passed: fresh dependency installation/patching; the normal clean root test build; 352 original ACP tests; CLI type checking; changed-file Biome checks; and diff/epoch checks. No Host protocol files change; epoch remains 225. The trial merge with main dc4515cd6e7637e8267d59cd4f71ad53f403f9e7 is clean. Exact-head test CI and Windows ACP CI pass; their success does not cover the failures above.

P3: update the stale single-commit/test description and add the required generative-tool-use declaration. The draft also distinguishes an already-delivered message from an unknown ID; the new -32002 mapping should not be described as full proposal conformance. I did not run a third-party editor UI, native desktop or paid model service, and did not build the merged tree.

messageId: string,
): Promise<string | undefined> {
const attached = await this.#attachments.get(sessionId)?.catch(() => undefined);
const queue = attached?.snapshot.queue ?? (await this.#readQueueOnce(connection, sessionId));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Refresh the Host queue before rejecting a pending revocation

This attached snapshot can predate a successful session/inject. In a real CLI/SDK/Host session using the production AiSdk backend, I loaded the session, held the current provider stream, then injected and immediately revoked 24 queued messages. All 24 revocations returned -32002, while independent Host execution queries still reported those messages as pending. They can therefore execute later despite the attempted cancellation. Replacing only this queue selection with a fresh Host read made all 24 cancel successfully. Refresh authoritative queue state before treating a missing cached entry as absent, while retaining atomic queue.entry.retract for delivery races.

@2BAB
2BAB force-pushed the feat/acp-session-inject branch from d9758d9 to 2635c36 Compare October 21, 2026 01:37

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at 2635c36876caead0734f3cba01686ff3513470d0, following d9758d9b6ab958779437d6346247e28b9a7a20a3. The three preceding injection commits are unchanged after restacking on model-selection head bf337224e8b4357d7d090f49478046e94ba16ef8, which I also verified independently. The added commit refreshes a missed queue lookup, checks for a running turn before steering, and documents existing deviations. The pending-revocation P2 is fixed. The idle-steer P2 is partially fixed; its admission race remains, along with the two other client P2 issues.

Fixed — pending message revocation. In the same real CLI-child/SDK/production-AiSdk/Host scenario as the previous review, all 24/24 immediately revoked queued messages are now cancelled. Restoring only the previous cached-miss policy makes all 24/24 revocations return -32002 while the Host still reports them pending. A missing cached entry now triggers a fresh Host read, and atomic single-entry retraction still protects the other queued message. The independent real-Host restart/admission/revocation checks also pass.

Partially fixed — idle steering still races with turn completion (P2, packages/cli/src/acp/session-registry.ts:596–613). A session that is already idle now returns -32010 / no_running_turn, makes no provider request and accepts a subsequent ordinary prompt. However, the running-turn read and message submission remain separate operations. I forced that legal interleaving with a scheduling pause immediately after the new precheck: the first production provider stream then finished normally, the original prompt returned end_turn, and the Host reported its root turn completed before the steer was submitted. Releasing admission still returned a messageId and started a different root turn, causing a second provider request with no live assistant reply at the client. Only scheduling was paused; the actual CLI validation, Host admission and production backend were unchanged. Enforce the running-turn condition at the Host's admission boundary so a steer cannot become new work after its target turn ends. The draft proposal still treats an idle steer as a failed precondition.

Two previously reported P2 issues are unchanged and reproduced again:

  • Running-turn steer identity: session-event-mapper.ts:156 replays the injected text with a different ID from the one returned on acceptance. Single queue, double queue and idle queue controls preserve their IDs.
  • Queued follow-up assistant output: in the ordinary new/prompt flow, the Host finishes the later queued turn but the client receives no live assistant reply; loading later replays it. Calling session/load before the original prompt still makes that same follow-up reply arrive live. The new scope comment acknowledges this limitation but does not change its runtime effect; it is separate from deferred user-message echo.

Validation passed: fresh dependency installation/patching, the normal clean root test build, 355 original ACP tests, CLI type checking, changed-file Biome checks and diff/epoch checks. The six original client scenarios were rerun, with the idle-steer expectation updated to verify its new rejection and absence of side effects; the other five retain their original identity/output controls. The 24-message comparison and turn-completion race use real ACP CLI processes and the production AiSdk backend with a controlled local streaming provider. The separate Host-restart checks use the repository's deterministic test backend.

No Host protocol files change; epoch remains 225. The trial merge with main dc4515cd6e7637e8267d59cd4f71ad53f403f9e7 is clean. Exact-head test CI and Windows ACP CI pass. P3: the PR description still claims a single old commit/test count and lacks the required generative-tool-use declaration. I did not run a third-party editor UI, native desktop or paid model service, or build the merged tree.

);
const connection = await this.#getConnection('turn.message.submit');
const messageId = this.#newTurnId();
if (params.mode === 'steer' && !(await this.#turnIsRunning(connection, params.sessionId))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Make the running-turn condition part of Host admission

The turn can finish after this precheck and before turn.message.submit. Using the real CLI child, SDK, Host and production AiSdk backend, I paused only scheduling after this check, let the original provider stream finish normally, awaited end_turn and confirmed the Host root was completed. Resuming the actual submit still accepted the steer and started a different root turn with a second provider request and no live reply to the client. The already-idle case is fixed, but this completion race retains the same unexpected new-work behavior. Enforce the condition atomically where the Host admits the message rather than relying on this earlier read.

@2BAB
2BAB force-pushed the feat/acp-session-inject branch from def4b84 to e5ea863 Compare October 21, 2026 03:34

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at e5ea863ab858b609afe93a14f78a4f27f45370f2, following 2635c36876caead0734f3cba01686ff3513470d0. The complete source difference is confined to the historical-message mapper and its test; the remaining source tree is unchanged. The steer replay-identity P2 is fixed. Two previously reported P2 issues remain.

Fixed — the accepted steer keeps its ID on replay. The actual ACP CLI child, official SDK, Host and production AiSdk backend now return the same message ID at injection acceptance and subsequent session/load replay. Restoring only the old mapper expression makes the same equality assertion fail with two different IDs. Queue, double-queue and idle-queue controls continue to preserve their message identities.

The two remaining P2 issues were reproduced again on this exact head:

  • Turn-completion race (packages/cli/src/acp/session-registry.ts:596–613). An already-idle steer correctly returns -32010 / no_running_turn with no provider request. The precheck and Host submission are still separate operations: pausing only CLI scheduling after the precheck lets the original provider stream end naturally, the original prompt return end_turn, and the Host mark its root completed before admission resumes. That actual submission still accepts the steer and starts a different root with a second provider request and no live assistant reply. No Host state, validation or provider result was fabricated; this demonstrates a legal interleaving, not its uncontrolled frequency. The running-turn requirement needs enforcement at the admission boundary.
  • No live assistant reply for an ordinary queued follow-up. Starting with the normal session/new → session/prompt flow, both a single queued message and two queued messages execute later Host turns but produce no live follow-up assistant chunks at the client. session/load subsequently replays the replies. Loading the Session before the original prompt makes the same queued reply arrive live. This is separate from the deliberately deferred echo of injected user text. The updated description acknowledges the limitation; the behavior remains.

The previous revocation fix remains effective: all 24 immediately revoked queued messages were cancelled. The separate actual-Host single-entry revocation and Host-restart checks both passed. The six production-client scenarios and turn-completion race also passed their expected assertions. Fresh installation/patching, the normal clean root build, CLI type checking, changed-file formatting, diff and epoch checks passed. Of 355 original ACP tests, 354 passed initially; an unchanged multipart Artifact test hit its 15-second client-operation timeout under the parallel run and passed in an isolated rerun. I did not establish that timeout as a PR regression.

No Host protocol files change; epoch remains 225. The trial merge with main dc4515cd6e7637e8267d59cd4f71ad53f403f9e7 is clean, and the exact-head test and windows_acp checks pass. The updated PR description resolves the earlier attribution and stale-scope notes. I did not run a third-party editor UI, native desktop or paid model service, or build the merged tree.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at f5560417f1149472e35f52e856ccd4ec90cf6a74, following e5ea863ab858b609afe93a14f78a4f27f45370f2. The added commit carries the intended Turn ID from ACP into Host admission and checks it while admitting the message. The turn-completion race is fixed. The queued-follow-up P2 remains, and the new same-epoch protocol declaration introduces another P2.

Fixed — steering cannot become a new Turn after its target ends. I reran the exact production-client interleaving used previously: the CLI reads the running Turn, admission pauses, the real provider stream finishes naturally, the prompt returns end_turn, and the Host root becomes completed before submission resumes. The new head returns -32010 / no_running_turn, keeps the original root identity and makes no second provider request. Removing only the expectation sent by the current CLI makes this assertion fail again. Stable-idle rejection and ordinary active steering also pass.

P2 — the optional input is not compatible with the old strict Host codec (protocol-compatible-changes/steer-expected-turn-id.json:4). A real new ACP CLI connected successfully to a Host using the actual previous e5ea863a production protocol and admission modules. Both advertise epoch 225; initialization and an ordinary streaming prompt succeed. Steering then sends expectedTurnId, which the previous strict input decoder does not accept: the connection closes and the client returns -32603 with request_interrupted / connection_lost. The previous CLI against that same previous Host accepts steering, and the new CLI against the new Host also works. Only the two changed Host modules were substituted with their actual previous source; no errors, messages or admission results were fabricated. The declaration considers an older caller talking to a newer Host, but misses the reverse pairing. Use a new compatibility epoch, or negotiate this input explicitly rather than advertising both protocols as interchangeable.

Main has now advanced to epoch 226. The branch's direct epoch check rejects 226 → 225; its clean trial merge with main 85e942724c02e7149c4a56f44a0ad2f1350563be inherits 226 but then fails the actual guard because this newly added declaration still pins 225. Re-pinning the declaration alone would not repair the mixed-peer failure. The protocol update needs the next unused epoch (currently 227 or later), with checks rerun against the current main.

Remaining P2 — no live assistant reply for an ordinary queued follow-up. The same actual CLI/SDK/production-backend scenarios still complete one or two queued Host turns without delivering their follow-up assistant chunks live. Loading later replays those replies; loading before the initial prompt makes the same follow-up arrive live. The previously fixed steering message identity remains stable. The PR description still lists the now-fixed admission race as an outstanding limitation; update that text after the code change.

Validation passed: fresh installation and dependency patches, the normal clean root build, 377 focused CLI/Host/protocol tests, six actual client scenarios, the admission-race check and its previous-behavior control, both mixed-peer checks, CLI/Host type checking, formatting and diff checks. The current head's existing test and windows_acp jobs pass against their earlier base; they do not establish compatibility with the newly advanced main. I did not run a third-party editor UI, paid provider, native desktop or a full merged-tree build.

{
"epoch": 225,
"files": ["packages/runtime-host/src/protocol/message.ts"],
"reason": "Adds an optional turn.message.submit input field. A peer that never sends it sees no change: admission keeps today's rules, and no codec, output or stored shape moves."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not declare this strict-input change compatible with the previous Host

The old Host rejects unknown turn.message.submit fields, while the new ACP CLI always adds expectedTurnId when steering. In a real CLI/SDK/Host test using the actual prior e5 protocol and admission modules, both peers complete the epoch-225 handshake and an ordinary prompt, but steering closes the connection and returns -32603/request_interrupted/connection_lost. The prior CLI against that same Host accepts the steer; the new CLI against the new Host also works. Optionality only preserves the old-client/new-Host direction. Remove this compatibility claim and advance the epoch (main is already 226), or explicitly negotiate support before sending the field.

@Astro-Han

Copy link
Copy Markdown
Contributor

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Note on the epoch: main is now at 226 (#6176 merged). Because this head adds expectedTurnId to the Host steer admission and an older Host drops the connection on it (see the review above), it needs a real epoch bump to the next unused value (227 at the time of writing; #6195, #6206, #6091 and #6163 are also waiting on 227+), with a history comment in packages/runtime-host/src/protocol/index.ts.

2BAB and others added 14 commits October 21, 2026 15:24
ACP sessions could change permission, thinking level, collaboration and orchestration modes, but not the model, even though the runtime already accepts an explicit model target.

Adds a select config option, id 'model', projected from the session's own connection catalogue and applied through session/set_config_option as { modelTarget: { kind: 'explicit', connectionId, connectionSlug, model } }.

Two deliberate constraints:

- A locked connection projects only the model the session already has. Listing the rest and refusing every one of them would advertise choices a client cannot take.
- Validation is session-scoped: a value the session's connection does not offer is rejected, and a request with no resolvable catalogue is refused rather than guessed. Option ids stay the Host's; the CLI does not re-derive them.

Generated-by: Claude
setConfigOption pre-validates the request against CONFIG_SPECS before the
per-session work. The model option is not in that static list (its values come
from the session's connection catalogue), so every model change was rejected as
an unsupported configId before reaching the code that can validate it.

The pre-check now narrows the payload only, and the model value is validated in

Generated-by: Claude
#setConfigOption where the session context exists.
The model option listed every entry the connection resolves, including models it
has not enabled. Selecting one of those is refused by the Host (and, on this
build, surfaces as an internal failure), so the list advertised a dead choice.

The projection now keeps entries in the connection's enabledModelIds, plus the
session's current model so an existing selection stays visible even if it was
disabled afterwards.

Generated-by: Claude
Review findings on the model option:

- the picker collapsed to one entry once `connectionLocked` turned true, but
  the Host locks the connection, not the model: it still accepts a
  same-connection switch, and it records the lock on the first durable message
  and on every model change. Offer the connection's models regardless.
- a model change kept the session's thinking level, so the Host refused any
  target model that did not support it. Clear it, as the TUI's `setModel` does.
- an invalid model value reached the client as `internal_failure`: the patch
  builder throws inside the update callback, where the entry-point mapping does
  not run. Map it to `invalid_params` with `requestErrorFromConfigInput`.
- the selection context listed every catalog entry while the picker listed only
  enabled ones. Both now go through `offerableModelChoices`, so a value the
  client can see is a value the Host accepts.

The ACP fixtures never learned about the model option, so their snapshots still
asserted four entries; they now assert five and match the real projection.

Generated-by: Claude
A Session without an LLM connection (a plugin-executor Session) has no
catalogue, and `#modelConfigContext` refuses a model request for exactly that
reason. The projection advertised the option anyway, so the picker offered a
choice that could never be committed. Project it only when the Session has a
connection, and pin that with a test.

Generated-by: Claude
Clearing the level unconditionally also dropped an explicit `high` when a
client re-sent the `currentValue` it already had, which is a no-op selection
with a visible side effect. Clear it only when the selection actually switches
models, where the Host would refuse a target that cannot honour the carried
level.

Generated-by: Claude
Implements the shape proposed for mid-turn input: one session/inject request with a
queue or steer mode, an agent-owned messageId in the response, and the mandatory
session/revoke_inject companion.

The runtime underneath already distinguishes the two placements, so this is the
ACP surface of an existing capability rather than a second implementation:
steer is admitted into the running turn and delivered at its next break point,
queue waits for it to end.

Scope: the response acknowledges admission, not delivery. A delivered message
appears in session/load replay under the id the client was given; live delivery
is not echoed because the Host does not surface user messages on the live event
stream and this adapter is one client per session.

Generated-by: Claude
Review findings on the injection commit:

- revoking took the whole queue with it: `queue.retract` empties every queued
  steering and follow-up message, and its `retractId` names the mutation rather
  than the message. Resolve the message to its queue entry and use
  `queue.entry.retract`, which removes one entry. An unknown or already
  delivered id now answers -32002 (the proposal's "not found") without sending
  any mutation at all.
- the ACP-facing connection copied the Host epoch when it was created, so after
  a Host restart `session/inject` kept signing admissions with the epoch that no
  longer existed, and the Host answered `outcome_unknown`. Forward the current
  epoch through a getter instead.
- `acp-child-process` still expected an initialize response without the
  `inject` capability, so the root test command could not pass.

The registry had no coverage of revocation at all, which is how the
whole-queue retraction survived review; two tests now pin the single-entry
behaviour and the no-mutation answer for an unknown id.

Generated-by: Claude
Two follow-ups on the revocation path:

- an entry that becomes `in_flight` between the queue read and the
  `queue.entry.retract` answers `operation_conflict`, not `not_found`, so the
  client saw an internal failure for a message that had simply been delivered.
  Both codes now map to the proposal's -32002.
- the epoch forwarding had no test. One now drives two admissions across a Host
  replacement through the real stdio server and asserts the second carries the
  new epoch; the getter is also no longer enumerable, so it stays an internal
  detail rather than something a spread or a deep-equal can pin.

Generated-by: Claude
Follow-ups on the injection path:

- a cached queue projection is refreshed by the Host only after it answers the
  submit, so the read taken right after `session/inject` is expected to miss.
  Treating that miss as absence answered -32002 for messages that were still
  pending and could still execute; the Host is now asked before an id is
  declared unknown.
- a `steer` on an idle Session made the Host start a turn nobody asked for and
  nobody could observe, while the next `session/prompt` failed with
  `session_busy`. The proposal's `-32010 no_running_turn` precondition is now
  checked before admission. The check and the submit are not atomic: a turn can
  end in between, and only the Host could close that window.
- the deviations from the injection proposal that this adapter keeps
  deliberately (error shape, cancel dropping pending injects, attachment
  handling, assistant replies to a queued follow-up) are now written down next
  to the scope note instead of being implicit.

Generated-by: Claude
`session/inject` answers with a message id, and the Host projects replay rows
under that same id — `stableMessageId` resolves `refs.providerEventId` first, and
the submitted id is what the runtime event records there. `steeringEventId` is
the runtime event that carried the text, a different id, and the mapper used to
prefer it, so `session/load` replayed an injected message under an identity the
client had never seen.

Replay now uses the row id for every user message, steered or not. Queue-mode
messages were already correct because their row id was the only candidate.

Generated-by: Claude
The idle-steer check and `turn.message.submit` were separate operations, so a
turn that ended in between still accepted the message and started a new turn
nobody asked for — the client could not see it, and the next prompt failed with
`session_busy`.

`turn.message.submit` now takes an optional `expectedTurnId`: admission answers
`operation_conflict` when that Turn is no longer running, and callers that do not
send it keep today's rules. The adapter resolves the running Turn once and
carries its identity into the submit, mapping the conflict back to `-32010
no_running_turn`, so the requirement is enforced where the decision is made.

The protocol epoch stays at 225: this is an optional input extension, recorded
in `protocol-compatible-changes/steer-expected-turn-id.json` because nothing a
peer observes changes when it does not send the field.

Generated-by: Claude
A queued follow-up's Turn starts after the prompt that queued it has already
answered, so it began with no client notification context and `#adoptTurn`
bailed. The Host ran the Turn and produced a reply the client never saw live;
`session/load` was the only way to read it back, and loading before the prompt
was the only way to see it live.

The prompt's notify is now kept for the Session, and a turn adopted without an
external context is observed in read-only mode: it forwards output, and claims
no interaction, Stop or cancel authority over a Turn it did not start. A later
load or resume upgrades the same observation instead of starting a second
consumer, which the single-consumer event queue could not have served anyway.
Cancellation still stops the turn through the attachment path.

Patch by Antigravity (agy); reviewed and verified here.

Generated-by: Antigravity (agy)
2BAB added 2 commits October 21, 2026 15:24
The read-only observation forwards a Host-started follow-up Turn's output but
deliberately claims no interaction authority. That left a permission or
elicitation request from that Turn unanswered: the client never saw the prompt
and the Turn waited forever.

Interaction authority now follows observation, not the read-only flag: a Turn
this connection observes is one it answers. The read-only marker still keeps
Stop and cancel bookkeeping with the Turn's owner, which is what it was for.
Multi-client sessions remain out of scope: this adapter is one client per
Session, and a Turn nobody observes still leaves its authority where it was.

The new test drives the reviewer's flow up to the pending question and asserts
the client is asked; reverting the guard makes it time out.

Generated-by: Claude
…hange compatible

`turn.message.submit` gained an optional `expectedTurnId`, and the input decoder
rejects unknown fields. Optionality only covers an older Client against a newer
Host: a newer Client that sends the field to an older Host has its connection
closed mid-steer, which review reproduced against the previous Host modules.

So the compatibility declaration goes, and the epoch moves to 227 — main is
already at 226, and this is the next unused value.

Generated-by: Claude

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at e12418fc186f69dc9168dd7012a9304ba318d3a1, following f5560417f1149472e35f52e856ccd4ec90cf6a74. The previous 13 author commits remain equivalent after rebasing. The three new commits preserve a notification target for subsequent queued Turns, coordinate observation startup and interaction handling, and replace the protocol compatibility declaration with epoch 227. Both previously reported P2 findings are fixed for the advertised running-Turn workflow; I found no new confirmed P0–P2 in this increment.

Fixed — queued follow-up replies arrive live. With the actual ACP CLI, official SDK, production Host and a local streaming provider, an ordinary prompt followed by one or two queued injections now delivers every follow-up assistant reply without an intervening load or resume. Replacing only the registry with its actual previous implementation makes the live-reply assertion fail again. A further real queued follow-up successfully completes an MCP tool permission request and form elicitation, returns the supplied form values to the provider, and streams its final answer without loading the Session first.

Fixed — protocol admission now distinguishes incompatible peers. The invalid steer-expected-turn-id.json exception is removed, and protocol/index.ts:111 advances 226 → 227. The protocol guard passes against current main ff74d21c8f1e2904bb85a00b3ffef98c7834792c. The previously verified atomic steering check also still passes: when the original Turn finishes after the CLI precheck but before admission, steering returns -32010/no_running_turn, retains the completed root identity and starts no second provider request. Single-message revocation, immediate revocation, in-flight conflict mapping, restart recovery and replayed steering identity remain correct in the exercised scenarios.

P3 — clarify the cold-Session queue boundary (session-registry.ts:1843–1850). Creating a Session and immediately injecting in queue mode, before any prompt/load/resume establishes an observer, is accepted and runs on the Host but emits no live assistant reply. Loading afterward replays the answer; loading before the same injection delivers it live. The new notification target is installed by prompt, so it cannot cover this path. I treat this as a scope/documentation limitation because the PR describes injection into an already running Turn, rather than repeating the now-fixed ordinary follow-up P2. Document the precondition or explicitly refuse/support this cold path.

Validation passed: fresh dependencies and patches, the normal root build:test, 528 focused ACP/Host/protocol tests, 11 independent production-client/Host scenarios, the previous-registry negative control, two cold-queue controls, the real permission/form follow-up, CLI/Host type checks, formatting and the protocol guard. One reused probe initially referenced a missing instrumentation file; correcting that probe import made its in-flight scenario pass without changing product code.

The current head is mergeable and a trial merge with current main is clean; I have not built the merged tree. The exact-head windows_acp check has passed and test is still running. PR #6091 also proposes epoch 227, so whichever lands second must choose the next unused epoch. I did not test a third-party editor UI, paid model service or native desktop. This remains a feature requiring maintainer judgment and completed exact-head CI.

…sibling

The multipart upload and delete case drives a real Host, and on a loaded CI
runner it took 18s against the harness's 15s client-operation default, failing
as a timeout while passing in isolation. The sibling real-Host case in the same
file already asks for 45s, so this one does too.

No production change: the flake is the harness default, not the adapter.

Generated-by: Claude
@2BAB
2BAB force-pushed the feat/acp-session-inject branch from 3cb9939 to ca56039 Compare October 21, 2026 08:24
Review found that `session/new` → `inject { mode: "queue" }` before any prompt,
load or resume is accepted and runs on the Host, yet nothing streams back. The
cause is structural rather than a missing wiring: live delivery needs an
attachment, and this adapter opens one from those three requests. The message is
readable afterwards through `session/load`.

Recording it next to the other deviations instead of refusing the inject, which
would drop work the Host can already do.

Generated-by: Claude

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at 07c7a8bb0a57af1d05b5a8067d9243d97d0e5e90, following e12418fc186f69dc9168dd7012a9304ba318d3a1. I found no new P0–P2 issue. The preceding 16 commits are unchanged. The two new commits only raise the multipart Artifact integration test's client-operation timeout from 15 to 45 seconds and document the cold-Session queue limitation.

The only changed production file, packages/cli/src/acp/session-inject.ts, differs by a comment. Its JavaScript output with comments removed is identical to the preceding revision; all other production files are byte-identical. The earlier running-turn queue streaming and interaction fixes are therefore preserved rather than reimplemented here.

I reran 12 original Artifact/injection tests and nine actual SDK/Host scenarios on this head, all passing. These include the changed multipart upload/export/delete case, running steer/queue and multiple queued messages, idle behavior, cold queues with and without a preceding load, and a queued follow-up that completes a real MCP permission/form exchange without another load. The model responses come from controlled local servers.

The previously noted P3 cold-Session boundary remains, now explicitly documented: a queued request can execute before the adapter has attached an observer, so its output is available through a later session/load but is not delivered live. The direct cold-queue probe confirms this; loading first supplies the observer and delivers the output live. This increment documents that behavior and does not change it.

The normal clean root test build, Biome on both changed files and git diff --check passed. There is no additional protocol change in this increment; the PR remains at epoch 227, and the protocol guard passes against current main's 226. A static merge with main 9826db1b9dad32897be105b2c1c621becedb27ce is clean. The exact-head test and windows_acp checks succeeded. If another epoch-227 protocol change lands first, this PR will need a new unused epoch and another check.

I did not rerun the complete repository suite, native Desktop, authenticated providers or a separately built merged tree. This incremental result relies on the demonstrated production-code equivalence and the targeted execution above; it does not claim that every earlier check was repeated.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants