Add standalone Agent Client Protocol client layer

[brennanb2025]

	Files	Added	Deleted	Net
Test	8	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​1607	$\color{#cf222e}{\Huge{\mathbf{−}}}$​15	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​1592
Prod	22	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​3165	$\color{#cf222e}{\Huge{\mathbf{−}}}$​7	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​3158

ELI5

Orca can drive only Claude and Codex as structured chats today. Other coding agents (Grok first) speak the Agent Client Protocol (ACP), and Orca has no reusable client for it. This PR adds that client as a standalone building block. No user-visible change: nothing calls it until a later adapter connects it to native chat.

Earlier review rounds found the first version would give up on a permission prompt after two minutes, end a turn after thirty minutes, pick a login method on its own, and drop messages containing a field value it did not recognize. Those are gone: decisions and turns wait until answered, cancelled or closed; the caller picks the login; unfamiliar values reach the caller.

This revision also moves into this PR the changes the Grok adapter branch (#25225) had been making to these files, so the client lands with the contract the rest of the stack actually uses. It fixes three problems found by review:

• An agent asking permission with a value newer than the protocol version Orca was built against (a new tool kind, a new tool status, a new option kind) was refused with an "invalid params" error. The user never saw the prompt, and Grok ends the tool or the turn when its question is answered with an error.
• Grok can start a turn on its own (after a background task finishes). The client dropped Grok's "turn completed" message for that turn, could not send a stop for it, and auto-declined any permission it asked.
• A message longer than the 16 MB line limit was only logged. If it was the answer to a request, that request waited forever; if it was the agent asking something, the agent waited forever.
• Steering had only the Stop's cancel. When the person adds a message while the agent works, the adapter cancels the running reply so the new message can follow. The client's only cancel waited 10 seconds and then closed the connection, which ends the agent: a slow agent would lose its whole process just because the person added a message.

What Changed

An Electron-free module accepts the agent's input/output streams and speaks two-way JSON-RPC 2.0 over newline-delimited JSON: initialization, caller-chosen authentication, new/load/resume session, prompts, cancellation, and mode/model/configuration settings. File and terminal access are advertised as disabled. The caller owns the process and must call close() when the child exits, even if a descendant keeps stdout open; closing the connection does not kill the process.

• Unfamiliar values are accepted. The generated validators treat protocol enums (tool kind and status, permission option kind, plan entry status and priority, stop reason) as open: the known values plus any other string, typed so code can still check for the known ones. A turn ending with a newer stop reason now completes instead of failing.
• Permission requests: only the session id, the tool call id, and options with ids are required. A field Orca cannot read (for example, a newer content type in the tool call) is dropped from the request with a diagnostic, and the rest still reaches the caller. Anything Orca cannot send back (the caller's handler throws, returns something malformed, or picks an option that was not offered) is answered as the protocol's "cancelled" outcome with a diagnostic, never as an error. An "invalid params" error is sent only when the request has no usable options.
• Turns the agent starts itself: messages other than session/update (an agent's own protocol extensions, such as Grok's "turn completed") are delivered through onExtensionNotification, in arrival order with session updates. cancel() (a Stop) always sends the protocol's stop message once the session is running. If Orca's own prompt is running, it also waits up to 10 seconds (configurable) for that prompt to finish, then closes an unresponsive connection. A prompt that finishes in time leaves the agent running; the code that owns the process ends it on Stop. Permission requests during an agent-started turn reach the caller, which decides whether to show them.
• Stopping: cancel signals every open agent request, and each handler sends its own answer: a permission answers "cancelled" (the protocol requires it), and a custom question can send the agent's own "abandoned" reply or finish an answer the user already gave. A handler that throws sends the standard "request cancelled" error. Contract for onRequest handlers: the handler owns its request and must answer it or throw once the signal aborts; Orca never answers for a handler that is still running. A request whose handler never answers stays open until the session closes; with Orca's prompt running, the 10-second stop bound closes it. A permission whose handler had not started yet still answers "cancelled". Only a stop message that was actually written is reused by repeat calls; if writing it failed, the next cancel() tries again.
• Steering: requestSteerCancel() sends the stop message once per prompt (a second call returns the first write), signals the agent's open requests, and answers later permission requests "cancelled". It never times out and never closes the connection: the prompt's own reply ends it, and the steer's prompt follows on the same connection. With no prompt of Orca's running it sends nothing. A Stop after a steer still waits its bounded 10 seconds and closes. If the prompt fails instead of ending, the caller must not send the steer into that session until it rebuilds it. Both cancels now live in acp-prompt-cancel.ts.
• Errors: AcpAgentError means the agent itself answered with an error. AcpInvalidResponseError means Orca could not read the agent's answer, and it keeps the raw answer and the validation problems. Callers can tell "the agent refused" from "Orca failed".
• _meta (the protocol's free-form metadata field) can be sent on prompt, set mode, set model, set configuration option and cancel.
• Oversized lines are classified from their first bytes, using the same classifier as the Codex reader (moved from src/main/codex/ to src/shared/json-rpc-record-prefix.ts, unchanged). The request owed that answer fails with AcpFrameTooLargeError; an oversized request from the agent gets an error answer; an oversized notification is logged. An oversized answer whose request cannot be identified closes the connection, because any pending call could be the one that never settles. Only the first 64 KB of a rejected line is inspected, so an oversized agent request that puts a huge params before method cannot be identified and also closes the connection (recorded agents send id, method, params in that order). Unlike the Codex reader, an oversized line that is not JSON-RPC at all (stray log output) is logged rather than closing the session.
• Generated schema: definitions come from the published schema-v1.21.0 unstable schema plus the older model API from v0.11.6, with SHA-256-pinned downloads and the full Apache-2.0 license kept. The file header now records the input digests, the generator's own digest and a hash of the generated body, so pnpm run verify:acp-protocol detects a stale or hand-edited file without network access. It runs in pnpm lint and as a step in the PR workflow. --check-online still regenerates from the downloads and compares. (The offline check catches accidental edits; an edit that also rewrites the recorded body hash is caught only by --check-online.)

Lines, queued writes and concurrent requests stay bounded, and writes honor backpressure. No dependency or lockfile change.

Why

The protocol layer should deliver what the agent sends and let the caller decide what to wait for, what to show and how to log in. A browser login cannot be assumed safe on a remote host, so authentication-required surfaces the advertised methods unless the caller names one, which is tried once.

Opening the enums in the generator fixes unfamiliar values everywhere at once. That includes the next layer, which re-reads the same updates, rather than patching each place that reads a message. A hand-written subset would avoid generation but lose the pinned provenance and make every new protocol field a hand edit. The permission-gating rule moved out because only the adapter knows when the agent has started a turn of its own. cancel() lost its caller-asserted "the agent is in a turn" flag for the same reason: the stop message is session-wide in the protocol, and an agent with nothing running ignores it.

A steer only asks the agent to wrap up its reply, so it gets the protocol's stop message and nothing else; ending the agent is a Stop's job. That is the common pattern's split: a steer sends the cancel notification with no timer and no kill, and a Stop waits briefly and then ends the agent for every ACP agent.

The bounded line framer and now the Codex reader's oversized-line classifier are reused. The Codex connection itself combines Codex-specific error handling with process ownership, so ACP gets its own small JSON-RPC peer for now (see the temporary item below).

Linked Issue

Internal foundational work; no pre-existing issue supplied.

Visual Proof

N/A — standalone protocol module with no UI or launch wiring.

Testing

• I manually tested these changes locally
• Automated tests added/updated

What I verified:

• Steering: 3 tests in acp-session-runtime.test.ts. A steer sends one stop message, never times out or closes even with a 100 ms Stop bound, aborts the open permission's handler, keeps a late permission from the handler, and the next prompt works on the same connection. A Stop after a steer still closes on its bound. A steer with no prompt running sends nothing. Each was checked by breaking the code: routing the steer through the Stop path, dropping its send-once guard, or skipping the open-request cancel each turns a test red.
• After merging current main: the 6 ACP test files pass on macOS (85 tests, including the 3 steering tests). Earlier revision: 114 tests pass on macOS. That's 82 fake-agent ACP tests across 6 files, including 2 new files: acp-permission-requests.test.ts and acp-session-agent-turns.test.ts. It also includes the 32 Codex connection tests that use the moved classifier.
• New tests cover:
  • permissions with a newer tool kind, tool status or option kind;
  • a permission with unreadable fields;
  • handler throw or malformed reply answered "cancelled";
  • a permission and stop during an agent-started turn;
  • extension messages ordered with session updates;
  • _meta on every session call;
  • agent errors versus unreadable answers;
  • a newer stop reason;
  • a retried stop after a failed write;
  • custom agent questions answering their own reply on stop, throwing (request cancelled), or ignoring the stop (left open, no answer from Orca);
  • a handler answer that finishes saving to disk after the stop (its own answer goes out, exactly once);
  • a stop that lands before the permission handler starts (answered "cancelled");
  • each oversized-line case.
• The review's adversarial reproductions now fail against this code as intended: unfamiliar permission values, a stop with no prompt of Orca's, the latched failed stop, and indistinguishable errors.
• The review's replay of 9 recorded real Grok process runs (682 session updates) still parses with no diagnostics. That replay lives outside this PR; the recordings belong to the Grok adapter PR.
• verify:acp-protocol passes offline, and catches a hand edit of the body and a changed generator. --check-online passes.
• Scoped Node typecheck shows no errors in changed files. Its only errors are missing stream-json/stream-chain modules from a stale local install, in files this PR does not touch.
• The pull-request workflow's static-analysis steps, run locally without pnpm, all pass: whole-repo oxlint, the anti-slop pass (config/oxlint-anti-slop.json, which the previous revision failed), the focused code-quality plugins, type-aware code quality on changed files, the changed-lines gate, the reliability-gate manifest, the max-lines and ts-nocheck ratchets, the Node runtime pin, README links, verify:acp-protocol and formatting.

What I didn't verify: no real agent CLI, Electron app, Linux/Windows run, SSH session or packaged app. CI on this revision is the authority for typecheck and static analysis. The cross-version qoder-history-search-downgrade test fails on unrelated PRs too and is not part of this PR's acceptance.

AI Disclosure

Review

Differences from the common pattern

• Intended: the caller supplies the streams and owns the process lifetime; the common pattern spawns the agent inside the connection. Here the code that starts the agent process is also the code that confirms it exited, and the client never assumes it runs on the user's machine, so the same client works in Orca's background service on any host.
• Intended: agent messages other than session/update are delivered through onExtensionNotification; the common pattern drops them. They carry the only end signal of a turn the agent starts on its own.
• Intended: permission option kinds and the tool, status and stop-reason enums are open; the common pattern keeps the option-kind enum closed. An unknown option is still a choice the user can see by name.
• Intended: the stop message is sent whenever the session is running, not only during Orca's own prompt, because agents start turns themselves.
• Intended: on stop, the agent's own open requests (permissions and custom questions) are answered by the handlers that own them, never by Orca on a timing guess; "request cancelled" goes out only when a handler throws. The common pattern answers custom questions immediately when they arrive, so it has no equivalent. Letting the owner answer keeps the agent's own cancelled reply and a user answer that is still being saved.
• Intended: gating permissions on "a turn is running" is left to the adapter. The common pattern gates in the layer that owns turns, and here that layer is the adapter.
• Intended: a generated, pinned subset of the schema with open enums, instead of a hand-written subset or a full schema with a separate compatibility layer.
• Intended: authentication is chosen only by the caller; no interactive or environment-variable login is picked automatically.
• Intended: retained input/output and concurrent requests are bounded with backpressure; there is no time limit on a decision or a turn. Only an explicit stop has a bounded (configurable, 10 s) confirmation wait.
• Intended: a steer's cancel is the protocol's stop message only: no timer, no close, sent once per prompt, as in the common pattern. A Stop keeps the bounded wait.
• Intended: cancel() closes the connection only when the wait runs out; ending the agent on every Stop is left to the process owner (the adapter), which the common pattern does inside its connection. Same reason as the first item: the code that starts the process confirms it exited.
• Temporary: a second JSON-RPC peer beside the Codex app-server reader. A follow-up merges them into one peer with protocol-specific error mapping on top; this PR already shares the oversized-line classifier so the two converge.
• Temporary: $/cancel_request (the agent withdrawing one of its own requests) is not implemented, as in the common pattern. Stopping or closing the session already clears open requests; a follow-up adds it before any caller relies on it.

Agent skill upstream boundary

• Not applicable; no upstream skill source is copied.

Notes

Paths, credentials and streams belong to the caller's execution host. No local-machine or Git-worktree assumption, Electron import, Orca remote RPC change, or new advertised runtime capability is introduced. The Grok adapter branch (#25225) and the translation branch (#25090) build on this; their owners pick these changes up on their next merge. The PR remains draft.

Checklist

• This PR is focused on the standalone protocol client
• I explained the problem, user-facing change, mechanism, and alternatives
• N/A for visual proof, with reason
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path impact considered
• CI "static analysis and typecheck" passes on 88490e58cf3 (job); the test and packaging jobs were still running when this was written

[brennanb2025]

Review summary (head 7aa40a99e5e)

The problem. To run Agent Client Protocol agents (Grok first) as structured chats, Orca needs a client that speaks the protocol over the agent's standard input and output. It covers setup and login, prompts, stopping, mode and model changes, streamed updates, and the requests the agent sends that need Orca's answer, such as permission prompts. It must run without Electron, on any host.

What changes for users. Nothing visible on its own. Grok's native chat (#25225) builds on it.

What the review rounds changed

• No time limits on decisions or turns. A permission prompt left open for 2 minutes used to be answered to the agent with an error. A turn past 30 minutes used to close the connection. Neither has a time limit now. Only an explicit stop has a bounded 10 s confirmation wait.
• Tolerant of newer or vendor-specific messages. The generated protocol types now accept values they don't know yet, for tool kinds, statuses, permission options and stop reasons. So an unfamiliar value no longer drops an update, blocks a permission prompt from reaching the user, or rejects a finished turn. Permission requests are checked only for the fields needed to route them. A local failure is answered as "cancelled", never as a protocol error.
• The contract Grok actually uses now lives here instead of being patched in by feat(native-chat): Grok as a structured chat over the Agent Client Protocol #25225:
  • agent messages other than session updates are delivered, including the only end signal of a turn Grok starts on its own after a background task;
  • extra protocol metadata can be sent on requests;
  • the agent's own errors are a distinct error type;
  • stop always tells the agent to cancel, including turns the agent started itself.
• Stop semantics. When a stop arrives, each open request the agent sent is answered by the handler that owns it, never by Orca on a timing guess. A permission answers "cancelled"; an approval that is still being saved keeps its answer. A request whose handler never answers is closed when the connection closes. A failed stop message no longer prevents a retry.
• Oversized lines fail the matching request or answer the agent's request, using the Codex reader's existing line classifier, now shared, instead of hanging.
• Login is chosen only by the caller; nothing interactive is picked automatically. The generated schema is one file, with line endings pinned. An offline check in lint and CI catches the file drifting from its generator.

For #25090/#25225 owners: this changes signatures #25225 relied on. Notably, stop has no "agent turn" flag, because it always sends the cancel. #25225's request handlers must answer requests nobody claimed with Grok's cancelled reply when a stop arrives. The details are in the review notes. #25090 and #25225 need to merge this branch.

Heads-up for landing order: #24988 adds a test that fails once src/main/acp/ exists while that PR still lists it as allowed-missing. Whichever lands second drops that allowance.

Deferred: answering the protocol's cancel-request message (temporary, labelled in the description). Merging this client with the Codex app-server reader into one peer (temporary, labelled). Starting a session twice with different options silently returning the first session. Minor error-detail and naming nits.

What was verified

• Four review rounds plus a readiness checklist. Every fix was also removed from a copy of the code and confirmed to fail a PR test (12 removals). Replaying 9 real recorded Grok sessions parsed 682 updates with 0 errors and delivered all 500 Grok-specific messages. The shared line classifier is byte-identical, and the Codex connection tests pass. 114 explicit tests pass on the final code.
• CI: static analysis and typecheck passed on 88490e58cf3. On that head one unit shard failed in pr-preflight-gates.test.mjs, caused by this PR's new CI step not being registered with a phase. That is fixed in 7aa40a99e5e, and the 32 config-script tests that read the workflow pass locally. The other shard failures there were only main's renderer translation tests.
• On 7aa40a99e5e, CI stops early in the localization audit, on strings main itself added in fix: close worktree dialog before slow script checks #25472 (worktree-creation-hook-preparation.ts; reproduced by running the audit on main). So typecheck and the test shards haven't run on this head yet; they will once main is fixed and CI is re-run.

Not verified: no live agent run in this PR (live Grok checks belong to #25225). No Windows, Linux or SSH host run.

[brennanb2025]

Merged current main into this branch (clean, no conflicts) and added a separate cancel for steering (now 58bb525f828).

• What changed: steering used to share the Stop's cancel, which waits 10 s and then closes the connection, ending the agent. requestSteerCancel() now sends the protocol's stop message once per prompt, cancels the agent's open requests, never times out and never closes. cancel() stays the Stop: the bounded wait, then close. This matches the common pattern: a steer only sends the cancel notification, and a Stop ends the agent. The description is updated.

• Review: a fresh reviewer found nothing above P2 and confirmed the Stop path is unchanged. I fixed the P2s:

  • one test assertion couldn't fail;
  • the doc comment didn't say what the caller owns when a prompt fails after a steer cancel.

  I also fixed one P3: a repeated steer now returns the write already in flight.

• Verified: 3 new tests. Each fails when its rule is removed: the steer routed through the Stop path, no send-once guard, no cancel of open requests. The 6 ACP test files pass (85 tests). The scoped typecheck has only a stale-dependency error. The line-limit lint passes on the merge result.

• CI: 17 pass. "Relay integration" sat without a runner during today's backlog and was cancelled.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: cb12f81a-6fe9-43f4-aad3-4ce6eb766b85

📥 Commits

Reviewing files that changed from the base of the PR and between 58bb525 and 320f347.

⛔ Files ignored due to path filters (1)

• src/main/acp/generated/acp-protocol.generated.ts is excluded by !**/*.generated.*, !**/generated/**

📒 Files selected for processing (29)

• .gitattributes
• .github/workflows/pr.yml
• .oxlintrc.json
• config/scripts/acp/generate-protocol.mjs
• config/scripts/check-runtime-electron-ratchet.mjs
• config/scripts/check-runtime-electron-ratchet.test.mjs
• config/scripts/pr-preflight-gates.test.mjs
• package.json
• src/main/acp/acp-errors.ts
• src/main/acp/acp-incoming-requests.ts
• src/main/acp/acp-json-rpc-peer.test.ts
• src/main/acp/acp-json-rpc-peer.ts
• src/main/acp/acp-oversized-lines.ts
• src/main/acp/acp-peer-limits.ts
• src/main/acp/acp-permission-requests.test.ts
• src/main/acp/acp-permission-requests.ts
• src/main/acp/acp-prompt-cancel.ts
• src/main/acp/acp-scripted-agent.test-support.ts
• src/main/acp/acp-session-agent-turns.test.ts
• src/main/acp/acp-session-events.ts
• src/main/acp/acp-session-lifecycle.test.ts
• src/main/acp/acp-session-notifications.test.ts
• src/main/acp/acp-session-runtime.test.ts
• src/main/acp/acp-session-runtime.ts
• src/main/acp/acp-session-setup.ts
• src/main/acp/acp-stdio-error-boundary.ts
• src/main/acp/acp-write-queue.ts
• src/main/codex/codex-app-server-record-dispatch.ts
• src/shared/json-rpc-record-prefix.ts

📝 Walkthrough

Walkthrough

The pull request adds ACP protocol schema generation and verification, a JSON-RPC peer over newline-delimited streams, and a session runtime for authentication, session operations, permissions, notifications, and cancellation. It also adds transport and runtime tests. The Codex app-server dispatcher now imports the JSON-RPC prefix classifier from the shared module.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 58bb5

The new ACP client is not yet used by any adapter, so users see no change today. In one narrow edge case, a failed Stop after a steer can show a permission prompt for a turn that is already ending. It is mergeable with a small follow-up fix.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 58bb5

The new client separates permission decisions from transport handling and includes resource limits and explicit cancellation controls. No currently reachable privileged operation or verified security weakness was established. Remaining uncertainty concerns how future callers enforce session identity, authorization, and recovery after failed session creation.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated new boundary is a caller-supplied stream pair and its session callbacks. Privileged operations, credentials, workspace access, and cross-tenant exposure depend on the embedding caller and selected peer; this standalone implementation does not establish a currently reachable tenant-wide or fleet-wide attack path.

Trust Boundaries and Controls

• observed — Agent-controlled permission requests do not directly authorize execution: the runtime checks session identity and delegates the decision. Missing handlers, aborted requests, malformed replies, exceptions, and selections not offered by the agent return cancelled. Non-permission methods require caller-provided authorization and otherwise fail as unknown methods.
• observed — Default limits are 16 MiB per line, 32 MiB of queued writes, and 128 incoming and pending requests each. Configured limits must be positive safe integers. Malformed identifiable responses reject matching calls, duplicate open incoming IDs are ignored, and oversized frames are refused, rejected, or connection-fatal according to their readable prefix.

Resilience and Maintainability Implications

• observed — Cancellation preserves generic handler response ownership rather than fabricating a reply for a still-running handler. Close aborts and abandons requests and clears transport state. Existing tests explicitly describe custom cancellation replies, silent handlers, and approved answers whose saves finish after cancellation.

Hardening Proposals

• proposed — Before connecting privileged callbacks, define the caller's method allowlist, session binding, and policy for unfamiliar permission kinds. Consumers should validate event session IDs before changing session-owned state, and the process owner should explicitly terminate or contain the agent after Stop.
• proposed — Establish recovery semantics for a session created remotely whose response cannot be accepted locally, such as producer-supported deduplication, reconciliation, or explicit cleanup before retry. This is an unresolved lifecycle guarantee, not evidence of an existing duplicate-session vulnerability.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 34.48% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 22 files. (4 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: adding a standalone Agent Client Protocol client layer.
Description check	✅ Passed	The description is detailed and follows the template. It explains the change, rationale, testing, visual-proof status, review considerations, and checklist. It identifies the work as internal and stat…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 34.48% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 22 files. (4 skipped: 4 unsupported.)

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e1706c5b-dd1e-46b1-8a7e-868f4796efda

📥 Commits

Reviewing files that changed from the base of the PR and between 57fedee and 58bb525.

⛔ Files ignored due to path filters (1)

• src/main/acp/generated/acp-protocol.generated.ts is excluded by !**/*.generated.*, !**/generated/**

📒 Files selected for processing (27)

• .gitattributes
• .github/workflows/pr.yml
• .oxlintrc.json
• config/scripts/acp/generate-protocol.mjs
• config/scripts/pr-preflight-gates.test.mjs
• package.json
• src/main/acp/acp-errors.ts
• src/main/acp/acp-incoming-requests.ts
• src/main/acp/acp-json-rpc-peer.test.ts
• src/main/acp/acp-json-rpc-peer.ts
• src/main/acp/acp-oversized-lines.ts
• src/main/acp/acp-peer-limits.ts
• src/main/acp/acp-permission-requests.test.ts
• src/main/acp/acp-permission-requests.ts
• src/main/acp/acp-prompt-cancel.ts
• src/main/acp/acp-scripted-agent.test-support.ts
• src/main/acp/acp-session-agent-turns.test.ts
• src/main/acp/acp-session-events.ts
• src/main/acp/acp-session-lifecycle.test.ts
• src/main/acp/acp-session-notifications.test.ts
• src/main/acp/acp-session-runtime.test.ts
• src/main/acp/acp-session-runtime.ts
• src/main/acp/acp-session-setup.ts
• src/main/acp/acp-stdio-error-boundary.ts
• src/main/acp/acp-write-queue.ts
• src/main/codex/codex-app-server-record-dispatch.ts
• src/shared/json-rpc-record-prefix.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep cancelling set when a steer cancel already exists and a Stop write fails.

Consider this sequence:

1. requestAcpSteerCancel sends session/cancel. It sets active.cancelling = true and stores active.steerCancel.
2. A later Stop calls confirmAcpPromptCancel.
3. The Stop's connection.send() rejects, for example with "ACP write queue capacity exceeded".

The catch block on Line 54 then resets active.cancelling to false. The steer's cancel was still sent to the agent, so the turn is still being cancelled. With cancelling === false, AcpSessionRuntime.handleRequest in src/main/acp/acp-session-runtime.ts stops answering new session/request_permission requests with cancelled. It forwards them to onPermission, so the user can see a permission prompt for a turn that is already ending.

requestAcpSteerCancel cannot repair the flag. When steerCancel is set, it returns the existing promise and does not touch cancelling. This conflicts with the stated rule "Only a cancel that reached the agent counts". Clear cancelling only when no steer cancel is outstanding.

🐛 Proposed fix

     await Promise.race([connection.send(), unconfirmed]).catch((error) => {
       // Only a cancel that reached the agent is shared; a failed write is retried next call.
       active.cancelPromise = undefined
-      active.cancelling = false
+      // A steer's cancel that was already sent still counts.
+      active.cancelling = active.steerCancel !== undefined
       throw error
     })

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	await Promise.race([connection.send(), unconfirmed]).catch((error) => {
	// Only a cancel that reached the agent is shared; a failed write is retried next call.
	active.cancelPromise = undefined
	active.cancelling = false
	throw error
	})
	await Promise.race([connection.send(), unconfirmed]).catch((error) => {
	// Only a cancel that reached the agent is shared; a failed write is retried next call.
	active.cancelPromise = undefined
	// A steer's cancel that was already sent still counts.
	active.cancelling = active.steerCancel !== undefined
	throw error
	})

[brennanb2025]

Merged current main and applied the same Electron-import check change #25225 carries: src/main/acp is now a required lane (this PR creates it), and the now-unused 'may be absent' mechanism is removed. Without it, merging this PR would have failed main's lets only directories that have not landed yet be absent test, the same way #24989 did for provider-process. Ratchet lane/fixture tests pass locally; the two tests that build the real tree need fresh packages (CI).