feat(acp): ACP Server with WebSocket transport#1260
Conversation
Add an ACP-compliant server endpoint at GET /acp that accepts WebSocket connections and speaks JSON-RPC 2.0 per the Agent Client Protocol spec. Implements: - WebSocket upgrade with Bearer token auth (OPENAB_ACP_AUTH_KEY) - initialize: capability negotiation - session/new: create OAB session mapped to internal channel - session/prompt: dispatch to agent via GatewayEvent, stream back AgentMessageChunk notifications from GatewayReply - session/cancel: placeholder for Phase 2 Architecture: ACP Client --WS JSON-RPC--> /acp endpoint --> GatewayEvent GatewayReply --> AcpReplyRegistry --> AgentMessageChunk notification --> Client Feature-gated behind 'acp' feature flag. Enable with: OPENAB_ACP_ENABLED=true cargo run --features acp Refs: ADR PR #1258
This comment has been minimized.
This comment has been minimized.
F1: Use subtle::ConstantTimeEq for auth token comparison (timing attack) F2: Remove duplicate AcpConfig/registry construction in serve() F3: Validate jsonrpc == "2.0" per spec (reject with -32600) F4: Defer reply channel creation to session/prompt (no dead _reply_rx) + Add tracing::debug for reply send failure (race condition visibility)
This comment has been minimized.
This comment has been minimized.
Critical fixes:
- 🔴 Reject concurrent prompts per session (-32001 'Session busy')
- 🔴 Abort prompt tasks on disconnect to prevent registry leaks
- 🔴 Clean up registry entries after prompt completes (not just on Done)
Important fixes:
- 🟡 Check event_tx.send() result — return JSON-RPC error if no agent connected
- 🟡 Remove unused agent-client-protocol dep (Phase 1 is manual JSON-RPC)
- 🟡 Add 'acp' to unified feature list in root Cargo.toml
- 🟡 Switch AcpReplyRegistry from tokio::sync::Mutex to std::sync::Mutex
(operations are CPU-bound, never held across .await)
Also:
- Rewrite acp_server.rs for clarity after multiple incremental fixes
- Add busy flag per session to track in-flight prompts
- Prompt handler now properly cleans up registry on all exit paths
|
CHANGES REQUESTED What This PR DoesAdds ACP (Agent Client Protocol) server support to OpenAB, enabling any ACP-compliant client (Zed, JetBrains, desktop apps, CLIs) to connect via WebSocket at How It WorksA new Findings
Finding Details🟡 F1: Duplicate AcpConfig::from_env() in serve()In the acp: adapters::acp_server::AcpConfig::from_env(),
acp_reply_registry: adapters::acp_server::AcpConfig::from_env()
.map(|_| adapters::acp_server::new_reply_registry()),Issues:
Fix: Extract once before the struct literal, matching the pattern in #[cfg(feature = "acp")]
let acp = adapters::acp_server::AcpConfig::from_env();
#[cfg(feature = "acp")]
let acp_reply_registry = acp.as_ref().map(|_| adapters::acp_server::new_reply_registry());Then reference the variables in the struct literal. This ensures the registry is only created when ACP is enabled and both fields share the same 🟢 F2–F6: All prior findings addressed
Baseline Check
What's Good (🟢)
CI Status
|
|
🔒 Auto-closing: this PR has had the If you'd like to continue working on this, please submit a new PR and link to this one if necessary. |
openabdev#1260 mounted the ACP endpoint only on the standalone `openab-gateway` binary (`serve()`). Fleet deployments run the unified binary's embedded gateway (`openab run`, main.rs) instead, which never called `serve()` — so `/acp` was unreachable there. Mount `/acp` on the embedded gateway too (gated by `OPENAB_ACP_ENABLED`), and route ACP replies back through the unified adapter's `dispatch_reply` (`platform == "acp"`). Requires the embedded gateway to be active (at least one platform / `[gateway]` configured). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openabdev#1260 mounted the ACP endpoint only on the standalone `openab-gateway` binary (`serve()`). Fleet deployments run the unified binary's embedded gateway (`openab run`, main.rs) instead, which never called `serve()` — so `/acp` was unreachable there. Mount `/acp` on the embedded gateway too (gated by `OPENAB_ACP_ENABLED`), and route ACP replies back through the unified adapter's `dispatch_reply` (`platform == "acp"`). Also start the embedded HTTP server when ACP is enabled even if only non-webhook platforms (e.g. Discord, which the core connects to directly) are configured — otherwise the listener never binds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openabdev#1260 mounted the ACP endpoint only on the standalone `openab-gateway` binary (`serve()`). Fleet deployments run the unified binary's embedded gateway (`openab run`, main.rs) instead, which never called `serve()` — so `/acp` was unreachable there. Mount `/acp` on the embedded gateway too (gated by `OPENAB_ACP_ENABLED`), and route ACP replies back through the unified adapter's `dispatch_reply` (`platform == "acp"`). Also start the embedded HTTP server when ACP is enabled even if only non-webhook platforms (e.g. Discord, which the core connects to directly) are configured — otherwise the listener never binds. Seed the `acp` platform into the gateway trust registry so its identity gating honours `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` (the ACP sender id is `acp_client`); without this the acp platform defaulted to deny-all and every prompt was rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openabdev#1260 mounted the ACP endpoint only on the standalone `openab-gateway` binary (`serve()`). Fleet deployments run the unified binary's embedded gateway (`openab run`, main.rs) instead, which never called `serve()` — so `/acp` was unreachable there. Mount `/acp` on the embedded gateway too (gated by `OPENAB_ACP_ENABLED`), and route ACP replies back through the unified adapter's `dispatch_reply` (`platform == "acp"`). Also start the embedded HTTP server when ACP is enabled even if only non-webhook platforms (e.g. Discord, which the core connects to directly) are configured — otherwise the listener never binds. Seed the `acp` platform into the gateway trust registry so its identity gating honours `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` (the ACP sender id is `acp_client`); without this the acp platform defaulted to deny-all and every prompt was rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openabdev#1260 mounted the ACP endpoint only on the standalone `openab-gateway` binary (`serve()`). Fleet deployments run the unified binary's embedded gateway (`openab run`, main.rs) instead, which never called `serve()` — so `/acp` was unreachable there. Mount `/acp` on the embedded gateway too (gated by `OPENAB_ACP_ENABLED`), and route ACP replies back through the unified adapter's `dispatch_reply` (`platform == "acp"`). Also start the embedded HTTP server when ACP is enabled even if only non-webhook platforms (e.g. Discord, which the core connects to directly) are configured — otherwise the listener never binds. Seed the `acp` platform into the gateway trust registry so its identity gating honours `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` (the ACP sender id is `acp_client`); without this the acp platform defaulted to deny-all and every prompt was rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(acp): revive ACP server over WebSocket (#1260) Restore the ACP server adapter from the auto-closed #1260, rebased onto main. Exposes OAB as an ACP server over WebSocket at `GET /acp`, gated by the `acp` feature + `OPENAB_ACP_ENABLED`. Prompts are bridged to `GatewayEvent` and replies streamed back through the existing pipeline. Also resolves the outstanding review finding on #1260: `serve()` built `AcpConfig::from_env()` twice (two independent registries) — now extracted once, matching `AppState::from_env()`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(acp): ACP wire conformance, session/resume, streaming panic-safety Align the ACP server to the official Agent Client Protocol (Schema v1.19.0) so standard ACP clients interoperate: - initialize: integer `protocolVersion: 1`, official `agentCapabilities` (`sessionCapabilities.resume`, `loadSession: false`, `promptCapabilities`), `authMethods: []`. - streaming: `session/update` notifications with `sessionUpdate: "agent_message_chunk"` and a `content` ContentBlock (was a custom `session/notification` + `chunk` wrapper). - stopReason: official snake_case (`end_turn` / `cancelled`); a backend timeout has no ACP stopReason and returns a JSON-RPC error instead. - session/cancel: one-way notification (no response) that ends the in-flight prompt with `stopReason: "cancelled"`. - session/resume: re-attach to a persisted session without replaying history (`session/load` is intentionally unsupported — the gateway keeps no replayable transcript; conversation state lives in the downstream agent). Also make delta streaming panic-safe: slice the reply snapshot with `str::get` instead of a byte index, so a boundary landing mid-codepoint (CJK / emoji) skips the frame instead of panicking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): as-built Phase 1 ADR + official method coverage Absorbs the pending ADR proposal from #1258 and adds two companion docs: - `docs/adr/acp-server-websocket.md` — the original proposal (@pahud), verbatim. - `docs/adr/acp-server-websocket-phase1.md` — the as-built Phase 1 ADR: the wire-conformant primitive surface, the `session/resume` (not `session/load`) decision with its rationale, and divergences from the proposal. - `docs/acp-official-methods.md` — the official ACP method surface pinned to Schema v1.19.0, with OpenAB's Phase 1 coverage and intentional non-support. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(acp): serve /acp from the embedded `openab run` gateway #1260 mounted the ACP endpoint only on the standalone `openab-gateway` binary (`serve()`). Fleet deployments run the unified binary's embedded gateway (`openab run`, main.rs) instead, which never called `serve()` — so `/acp` was unreachable there. Mount `/acp` on the embedded gateway too (gated by `OPENAB_ACP_ENABLED`), and route ACP replies back through the unified adapter's `dispatch_reply` (`platform == "acp"`). Also start the embedded HTTP server when ACP is enabled even if only non-webhook platforms (e.g. Discord, which the core connects to directly) are configured — otherwise the listener never binds. Seed the `acp` platform into the gateway trust registry so its identity gating honours `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` (the ACP sender id is `acp_client`); without this the acp platform defaulted to deny-all and every prompt was rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): add Phase 2 browser-control (MCP-over-ACP) design ADR Proposed design for the agent's LLM to autonomously operate the user's browser ("computer use" against the real, logged-in Chrome), exposed as MCP tools tunnelled over the existing `/acp` WebSocket (MCP-over-ACP): - extension runs the MCP server role over its outbound WS (MV3 can't listen); - OpenAB core proxies the browser tools to the in-pod agent (MCP client); - needs the agent→client request direction added, and generated (v1) types. Design only — not implemented. Linked from the Phase 1 roadmap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): rename the Phase 1 ADR to "base" Reframe the as-built ACP-over-WS server ADR from "Phase 1" to "base" — it is the foundation later phases build on, not one numbered phase of many. Renames `acp-server-websocket-phase1.md` → `acp-server-websocket-base.md`, retitles, and updates the two referencing docs (method coverage + the Phase 2 browser-control ADR). Roadmap phase numbers (Phase 2–5) are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): rename browser ADR into the acp-server-websocket family `acp-mcp-browser-control.md` → `acp-server-websocket-mcp-browser.md` so all three ADRs share the `acp-server-websocket-*` prefix (proposal / base / mcp-browser) and group together. Updates the base ADR roadmap link. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): consolidate base + browser ADRs to the re-scoped roadmap Replace the original proposal's numbered Phase 2–5 roadmap with a re-scoped plan driven by the north-star goal (LLM operating the user's browser): - base §6: Critical path (agent→client requests, request_permission, MCP-over-ACP + core proxy, generated typed v1) / Optional / Not needed / Observability tiers. - Multi-agent "conversation" clarified: N independent OpenAB instances relayed by the client (a room) — client-side, not ACP fan-out. Fan-out removed. - Note OpenAB command parity is mostly free over ACP (text directives/slash are platform-agnostic). - Recommend an ACP trace mode first (know behavior, de-risk generated types). - Browser ADR de-phased (proposed north-star, not "Phase 2"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): merge the typing/dependency decision into the base ADR Fold the standalone typing analysis into base ADR §7: hand-rolled now → generated (typify, serde-only, ~0 dep) for the expanded surface; full `agent-client-protocol` crate rejected (2nd async runtime), schema-only crate is +24 schemars-heavy deps; v1 only; round-trip caveat; hand-roll trivial / generate complex, downstream core client is the bigger ROI. Renumbers refs to §8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(acp): add OPENAB_ACP_TRACE frame tracing (both directions) Flag-gated (OPENAB_ACP_TRACE=1|true) logging of every JSON-RPC frame on the upstream client<->gateway hop, in both directions (dir="in"/"out"). Off by default. Traced at the two choke points: the inbound ws_rx read and the single outbound writer task. Captures real ACP traffic to validate the planned generated-type round-trip against what clients/agents actually emit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(acp): vendor + generate serde-only ACP v1 wire types Add crates/openab-gateway/src/adapters/acp_schema.rs, generated by cargo-typify 0.7.0 from the vendored ACP v1 JSON schema (schemas/acp-v1.schema.json, pinned to upstream schema.json @ eb88e992 / ACP Schema v1.19.0). Plain serde — no schemars/serde_with runtime dep. Feature-gated (acp), allow(dead_code) until acp_server migrates onto it. Full v1 surface is generated (one closed dep graph); the base wires only the chat subset. Round-trip of the chat subset against known-good wire was proven before committing (Initialize/NewSession/Prompt/StopReason/ContentBlock/ SessionNotification all exact; StopReason snake_case; ContentBlock untagged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(acp): conformance guard pinning hand-rolled wire to generated types Add acp_conformance test module: every payload acp_server hand-rolls (initialize / session.new / resume / prompt stopReason / session.update agent_message_chunk) and accepts (prompt request) is asserted to deserialize into the generated acp_schema ACP v1 types, with serde proven a stable fixed point. Any casing / field-name / shape drift — the class of bug fixed during the base build (agentMessageChunk->agent_message_chunk, integer protocolVersion, snake_case stopReason) — now fails CI. Per ADR §7 the trivial chat payloads stay hand-rolled; this guard locks them to the schema. Typed construction for the complex bidirectional/MCP surface is the roadmap's job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): update base ADR §7 to as-built (generated types shipped + conformance-pinned) §7 now records what shipped: acp_schema.rs generated + committed (typify 0.7.0, serde-only, schema vendored/pinned to v1.19.0), trivial chat payloads kept hand-rolled per the rule, and the acp_conformance test pinning them to the generated types as the round-trip validation. Full typed construction migration is documented as deferred to the bidirectional/MCP roadmap. Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): record cwd non-propagation as a deliberate security decision (§5) Investigation for the "propagate cwd" item found the downstream plumbing already exists (openab-core pool working_dir_override) but is sourced only via the [[ws:…]] directive, resolved by resolve_workspace which contains every path under bot_home. Honoring a raw client cwd as current_dir would bypass that containment — unsafe for a default-unauthenticated endpoint, and meaningless for the base's browser client (no valid pod-side path). §5 now documents this as a deliberate decision and records the safe wiring (route cwd through resolve_workspace) for when an authenticated IDE-style client actually needs it. Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): record command-parity verification by construction (§5) Traced the command path for the "verify openab commands over ACP" item: directives (parse_directives in dispatch.rs) and slash commands (/reset, /model in process_gateway_event + run_gateway_adapter) are platform-agnostic and keyed on event.platform generically — nothing special-cases acp out, and interception reads event.content.text (the ACP prompt text). §5 upgraded from assertion to verified-by- construction. Remaining runtime item (does a slash reply render back into the ACP stream) folds into the deploy-gated e2e. Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(acp): add WebSocket /acp e2e smoke script + wire into canary-tests scripts/acp-ws-smoke.py: a uv-runnable ACP-over-WebSocket client for the upstream client<->gateway /acp hop (distinct from the downstream stdio hop in Layer 2). Drives initialize -> session/new -> session/prompt (stream + stopReason) -> session/resume, and confirms /model and /reset slash-command replies render back over ACP as agent_message_chunk. Parameterized WS URL + optional OPENAB_ACP_TOKEN; exits non-zero unless all 7 checks pass. Verified 7/7 against a live deployment. docs/canary-tests.md: new "WebSocket transport (/acp)" subsection under Layer 2 pointing at the script and cross-linking the offline acp_conformance cargo test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(acp): edge-case coverage (emoji/Unicode) + streaming slicer unit tests Fills the Unicode/emoji test gap. Extracts the incremental delta logic into a pure `stream_delta(sent_len, full_text)` so the char-boundary safety is unit-testable (behavior unchanged), then adds: - acp_streaming (6): append reconstructs exactly; multi-byte codepoints (astral emoji / ZWJ family / flag / VS16 / CJK) never split across snapshots; emoji emitted whole in one delta; mid-codepoint sent_len skipped not panicked; no-new-text / empty → None. - acp_conformance edge (5 more): ContentBlock + SessionNotification round-trip for astral emoji, ZWJ family, regional-indicator flag, VS16, astral CJK, mixed run; boundary strings (empty / whitespace / JSON-special / control chars / 4KB); all five StopReason variants; multi-block emoji prompt. 17 ACP tests total, all green (clippy -D warnings clean, workspace tests pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(format): grapheme-safe, whitespace-preferred message splitting split_message's hard-split iterated by char (Unicode scalar), so it could break a grapheme cluster mid-way — splitting an emoji ZWJ sequence, a VS16 emoji, a regional-indicator flag, or a combining mark across two messages. It also broke words mid-token. Now the hard-split walks grapheme clusters (unicode-segmentation) so a codepoint sequence is never split; outside code fences it also backtracks to the last whitespace so words / CJK / emoji stay intact (code fences stay grapheme-safe but are not reflowed at spaces). New `split_point` / `first_grapheme_len` helpers. Tests: grapheme clusters never split (astral emoji / ZWJ family / flag / VS16 / astral CJK) across several limits; plain split prefers whitespace; long CJK run breaks between characters with all content preserved; fenced emoji hard-split stays grapheme-safe. All existing split_message tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp-smoke): treat JSON-RPC errors/timeouts as failures (F13) The /model and /reset checks passed on `bool(r.get("error"))`, so a JSON-RPC error counted as a successful command render. Now a check passes only on a real rendered reply — no error, no timeout, chunks present, and the expected keyword in the streamed text. Verified 7/7 still green against a live deployment. Addresses review finding F13. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): correct overclaimed cancel/verification status (F15) session/cancel is downgraded from "✅ conformant" to⚠️ partial: the notification is accepted and the waiter ends with stopReason:"cancelled", but cancellation is not propagated to the backend (F3). The live-verification section drops the unqualified "session/cancel drives a real backend" claim and adds a Known-limits block recording the two verified-but-unfixed defects — long replies truncate over ACP (F2) and cancel does not stop the backend (F3) — as tracked follow-ups. Addresses review finding F15. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): include acp in embedded-server feature guards (F7) Three `#[cfg(any(...))]` guards (the `mod unified_adapter` gate, the pool/ unified-platform setup, and the embedded HTTP-server block that mounts `/acp`) listed the webhook platforms but omitted `acp`, so `--no-default-features --features acp` compiled the embedded server out before the `acp_enabled` runtime check could run — contradicting the documented ACP-only `openab run` deployment. Added `feature = "acp"` to all three. Verified `cargo build --no-default-features --features acp` now succeeds. Addresses review finding F7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(format): bound over-limit graphemes so every chunk stays <= limit (F4) The grapheme fallback emitted a whole grapheme cluster even when it was wider than the caller's limit, producing a chunk above a hard platform limit (undeliverable on Discord/Slack, and it could eat a mention footer's reserved capacity). Replace first_grapheme_len with codepoint_split_point: when — and only when — a single grapheme is wider than the target width, split it by codepoint as a last resort so the chunk still satisfies the limit (a cluster wider than the whole limit cannot be kept intact and also fit). Tests: oversized grapheme respects limit + preserves content across limits 2/3/5; mention-reserve path honors the reduced limit. Adjusted grapheme_clusters_never_split to limits >= the widest grapheme (oversized-split is the new dedicated test). Addresses review finding F4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): trace frames at debug! + truncate, document content logging (F12) OPENAB_ACP_TRACE logged full prompts/responses/capabilities at info!, so enabling it surfaced message content at the default log level and dumped whole frames. Frames now log at debug! (never at the default level) and are truncated to 512 scalar values via trace_frame, and acp_trace_enabled's doc states plainly that it is an opt-in debugging tool that records message content, for trusted environments only. Addresses review finding F12. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): honor JSON-RPC notification vs request id semantics (F8) The dispatch collapsed an omitted `id` to `Value::Null`, so notifications executed as requests and got `id:null` responses, unknown notifications got error responses, and `session/cancel` never responded even when sent with an id. Notification detection now uses raw key PRESENCE (`raw.get("id").is_none()`) — serde's `Option<Value>` maps both omitted and explicit `null` to `None`, so the struct field cannot distinguish them. A notification never receives a response: request-only methods (initialize/session.new/resume/prompt) sent without an id are ignored (they cannot return a result), and an unknown notification is dropped instead of erroring. A request (id present, incl. null) is always answered; `session/cancel` sent as a request is acknowledged with an empty result. Bad-version/invalid-request errors are likewise sent only for requests. Test: raw key presence distinguishes omitted id / explicit null / numeric id. Addresses review finding F8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): validate required session/new & session/resume params (F9) session/new ignored its params entirely and session/resume only read sessionId, so malformed requests were silently accepted. Both now validate params against the generated request types (NewSessionRequest / ResumeSessionRequest) and reject missing/malformed required fields with -32602 (session/new needs { cwd, mcpServers }; session/resume needs { sessionId, cwd }). Shape-only — the base still does not propagate cwd/mcpServers (ADR §5); the sessionId sess_<uuid> format check remains in the handler. New validate_params helper + test. Addresses review finding F9. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): reject unsupported prompt content blocks instead of dropping (F10) extract_prompt_params filter_map'd the prompt array, silently discarding any non-text block (image / audio / resource / resource_link), so a client's content vanished with no signal. It now returns an error on the first unsupported block type (and on a block missing its type/text), which the caller surfaces as -32602. The base is text-only; plain-string prompts and text blocks are unchanged. New test. Addresses review finding F10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): lightweight per-connection resource caps (F6) A client could create unlimited sessions and prompt tasks and send arbitrarily large frames. Add deterministic overload caps: MAX_FRAME_BYTES (1 MiB) rejects an oversized inbound frame before parsing; MAX_SESSIONS_PER_CONNECTION (128) bounds session/new; MAX_INFLIGHT_PROMPTS (32) bounds concurrent prompt tasks (finished tasks are reaped first). Each returns a JSON-RPC -32000 overload error. Full backpressure (bounded outbound channel), idle eviction, and global connection/worker limits remain a follow-up (documented as roadmap). Addresses review finding F6 (lightweight scope). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(acp): handler-level tests exercising the real handlers (F14) Add acp_handlers: call handle_initialize / handle_session_new / handle_session_resume directly and assert their actual output + side effects, not just literal round-trips. Covers the conformant initialize capabilities, session/new minting a sess_<uuid> and storing it, and session/resume returning {} + storing for a valid id while rejecting a malformed / missing sessionId with -32602. Addresses review finding F14. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): deliver long replies whole to avoid truncation (F2) A reply over the adapter's message limit was split into several messages, but the ACP reply route is closed after the first delivered message, so overflow chunks were dropped — a long reply truncated over ACP (verified: a 900-line reply arrived as ~413 lines). ACP is a WebSocket transport with no small per-message limit, so it now delivers replies whole: reply_message_limit() returns usize::MAX for platform "acp" (a single split_message chunk → one delivered message → full text), while every other platform keeps the adapter's chunk limit. Overflow-safe (only saturating_sub is applied). Test: ACP limit is unbounded, others unchanged, and a 50k-char reply is a single chunk. Addresses review finding F2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): accept baseline resource_link (F10) + validate/negotiate initialize (F8) F10 — resource_link is baseline ACP content (every agent MUST accept text + resource_link); the prior commit rejected it. extract_prompt_params now accepts resource_link and passes the link reference through as text ([name](uri) or the bare uri; missing uri → -32602) without fetching it (no SSRF). Only the capability-gated variants this agent does not advertise (image / audio / embedded resource) are rejected. F8 — handle_initialize ignored its params, always returned version 1, and marked the connection initialized unconditionally. It now deserializes the official InitializeRequest (protocolVersion required → -32602 if missing/malformed), negotiates the version (min of client and ours; a version below v1 is rejected; a higher version negotiates down to 1), responds with the negotiated version, and the dispatch marks the connection initialized only on a successful negotiation. Tests: baseline vs gated content blocks; initialize negotiation (down-negotiate, reject version 0 / missing protocolVersion / missing params). Addresses review findings F8 and F10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): fail closed off loopback without a transport key (F1/F11) ACP was enabled with auth_key:None and the WS upgrade skipped validation in that state, and an empty OPENAB_ACP_AUTH_KEY was accepted as a valid key — so a config omission could expose agent execution on the default 0.0.0.0 bind. Add acp_auth_ok_for_bind(auth_key, listen_addr): a non-empty key suffices on any bind; without a key, fail-open is allowed ONLY on a loopback bind (127.0.0.0/8 / ::1 / localhost). A non-loopback bind (0.0.0.0 / LAN / LoadBalancer) without a key refuses to mount /acp — checked at both mount sites (standalone gateway serve() and embedded `openab run`). AcpConfig now treats an empty key as unset. Base ADR §2 documents the loopback fail-open policy. Test covers key/no-key × loopback/non-loopback/empty. Addresses review findings F1 and F11 (a non-loopback deployment now requires a key; the key is still accepted via header or query — query use is a documented residual for browser clients). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(acp): strengthen /acp smoke into an item-by-item conformance suite scripts/acp-ws-smoke.py now prints a grouped, item-by-item report (for pasting into the PR as evidence): Transport/Auth (token required off loopback — no-token/wrong-token rejected, valid token accepted), Protocol compliance (JSON-RPC envelope + ACP wire shapes, initialize negotiation, session lifecycle, session/update notification shape, snake_case stopReason, resume), and Protocol edge cases (version negotiation & -32602 rejects, required-param validation, -32002 not-initialized, -32600 bad jsonrpc, resource_link accepted / image rejected, notification silence, F2 oversized-reply whole delivery, Unicode/emoji stream integrity). OPENAB_ACP_TOKEN is now mandatory. Updates canary-tests.md accordingly. Note: several checks require this branch deployed (they exercise F8/F9/F10/F2/F1) and a transport key, so the suite is green only after a redeploy with a token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(acp): carry the transport bearer key via Sec-WebSocket-Protocol (F11) The token was accepted via `Authorization: Bearer` or a `?token=` query. Browsers can't set an Authorization header on a WS handshake, so they fell back to `?token=`, which leaks the key into URLs/access logs/history (F11). Add the subprotocol path: a client offers `Sec-WebSocket-Protocol: openab.bearer.<key>, acp.v1`; ws_upgrade extracts the key from the `openab.bearer.` entry (timing-safe compare as before) and echoes the real `acp.v1` subprotocol so the browser handshake completes. This keeps the key out of the URL — the de facto browser-WS bearer pattern (Kubernetes API server). Priority: Authorization header → subprotocol → `?token=` (legacy/deprecated). We deliberately do NOT repurpose ACP's authenticate/authMethods (agent→provider auth, credential out-of-band — not client→server); authMethods stays []. ADR §2 documents the methodology and rationale; the conformance suite authenticates via the subprotocol. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): flesh out the tool_call display roadmap item (§6) Record the finding that downstream tool events currently reach the ACP client only merged into the reply text (openab-core compose_display prepends '🔧 …' lines), and the recommended approach: for the acp platform, tap AcpEvent::ToolStart/ToolDone before the merge and emit structured session/update tool_call/tool_call_update notifications so clients render tool chips distinctly (ACP-native, Zed-compatible). Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): validate JSON-RPC id type (F13), close oversized frames (F7), redact session logs (F12) - F13: reject a request whose `id` is not a string / number / null (object/array/bool) with -32600, per JSON-RPC. Conformance suite adds an id-object check. - F7: an oversized inbound frame can't be parsed, so we can't tell request from notification or recover its id — stop fabricating a JSON-RPC error (which broke notification silence) and close the connection as a transport-level violation. - F12: the session lifecycle logs (created / resumed / prompt dispatched) carried the sessionId (a resume capability; the channel_id shares its uuid) at info!; downgraded to debug! so the capability stays out of normal logs. Connection-level info! is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): stream append-only answer text, not the re-rendered tool display (F2) ACP streamed `compose_display(tool_lines, text)` snapshots over `edit_message`, but the tool-status prefix (🔧/✅ …) is re-rendered as tools progress, so the snapshot is a non-append REPLACEMENT — while `agent_message_chunk` is append-only and acp_server's stream_delta assumes append. The mismatch duplicated/garbled/truncated tool-involving replies (e.g. "…資訊。打資訊。"). For the `acp` platform, stream the raw append-only answer text instead of the tool-merged display (new `display_for` helper; `platform_is_acp` gates the four reply-path call sites). Tool activity will be surfaced separately as structured `tool_call` session/update notifications (roadmap, ADR §6). Other platforms are unchanged. Addresses review finding F2 (streaming completion / replacement). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(acp): add lifecycle e2e checks (cancel, oversized-frame close, header auth) New "Lifecycle / transport" group in the conformance suite: session/cancel ends the in-flight prompt with stopReason:"cancelled" (the base cancel primitive's effect, not just its notification silence); an oversized frame closes the connection (F7); and a valid token is accepted via the Authorization: Bearer header (the non-browser transport path). Verified 27/27 against a live deployment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): enforce resume session cap, fence stale replies, decouple streaming Group-review follow-ups on the ACP server base: - session/resume now applies the same MAX_SESSIONS_PER_CONNECTION cap as session/new (was an unbounded insert path — a client can mint unlimited sess_<uuid>); over-cap resume returns -32000. - Fence stale replies after timeout/cancel: register each turn's reply sink under the originating GatewayEvent id (round-tripped as reply_to) so a late reply from a superseded turn is dropped, not delivered into the next prompt's stream on the reused channel_id. - ACP no longer inherits the unified adapter's Telegram streaming flag; it decides streaming explicitly by platform. - has_unified_platform recognises an ACP-only deploy so `openab run` mounts /acp without another platform configured. - Warn when OPENAB_ACP_AUTH_KEY has non-RFC6455-token chars (base64 = and / break the browser subprotocol handshake). - Document the caps/fence/backend-cancel residual in the base ADR. Adds regression tests for the resume cap, the stale-reply fence, and the subprotocol charset check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(smoke): require every emoji/CJK marker + fail-closed on error (R16-F5) The ZWJ-family check parsed as `("🎉" in text and "👨👩👧👦" in text) or ("👨" in text)`, so a lone 👨 (or any partial reply) passed. Require ALL of 你好/🎉/👨👩👧👦/❤️ via all(), and fail closed first on a timeout or JSON-RPC error so a dropped reply can't slip through the content assertion. Script-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: run acp-gated gateway tests + unified build (R16-F4) `cargo test --workspace` builds with default features, so openab-gateway's `acp`-gated conformance/handler/streaming tests never compiled or ran in CI (and the old comment claiming they were "covered by cargo test --workspace" was wrong). Add an explicit `cargo test -p openab-gateway --features acp` step (scoped to the one crate to avoid the workspace hooks::tests parallel flake) and a `cargo build --features unified` step so the embedded /acp endpoint is compiled in CI. YAML validated with yq; local unified gate green (clippy + full test + build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): reserve prompt cancel state before spawn — fix prompt→cancel race (R16-F1) An immediate session/prompt → session/cancel could process the cancel before the spawned prompt task installed its cancel handle: s.cancel was still None, so the notification was dropped and the prompt ran uncancelled. Move the reservation (busy-check + install the cancel Notify, under the session lock) into the session/prompt read-loop branch, synchronously, BEFORE tokio::spawn — the read loop is sequential, so a cancel on the next frame now finds s.cancel installed. The handler takes the reserved session_id + cancel, releases the reservation on every early return (new release_prompt helper), and no longer re-reserves. A tokio::Notify stores one permit, so even a cancel fired before the handler's select! is consumed as stopReason "cancelled". Regression test: pre-reserved session + pre-fired cancel → the turn resolves "cancelled" and the reservation is released. Gate: clippy --features unified -D warnings + test --features unified (705+292) + build, all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): reject session/resume while a prompt is in flight (R16-F2) handle_session_resume unconditionally rewrote AcpSession{busy:false,cancel:None}. Resuming during an active prompt therefore dropped the in-flight turn's cancel handle, and then that turn's cleanup clobbered the freshly-resumed registry entry / state — losing the newer turn's replies. Minimal, deterministic fix: reject a resume whose local session is currently `busy` with a JSON-RPC -32001, leaving the active turn's busy flag + cancel handle untouched (owner/generation tagging is the Phase-2 robust variant, deferred). Regression test: resume against a busy session is rejected and the session's busy + cancel survive. Gate: clippy --features unified -D warnings + test --features unified (705+293) + build, green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): narrow Phase-1 streaming contract to single terminal chunk (R16-F3 option A) The ACP ChatAdapter is streaming=false, so Phase-1 delivers the whole reply as one terminal agent_message_chunk before the session/prompt response — not progressive multi-chunk streaming. Align the docs to that reality (option A; progressive streaming stays Phase-2, not implemented here): - ADR acp-server-websocket-base.md: reword the §1 scope + the agent_message_chunk acceptance wording to "single terminal chunk (streaming=false)", and add a Phase-2 progressive-streaming roadmap bullet under §6. - PR #1418 body: softened the At-a-Glance diagram + Proposed-Solution line the same way (wording-only; applied via gh, everything else byte-for-byte). - Test phase1_emits_single_terminal_agent_message_chunk: a final (send_message) reply yields exactly ONE agent_message_chunk + stopReason end_turn, anchoring the claim. Gate: clippy --features unified -D warnings + test --features unified (705+294) + build, green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): note the busy-gate invariant that keeps prompt cleanup safe (R16-F2) The unconditional registry remove / busy reset in the prompt cleanup path is correct only because no newer turn can exist on the same session_id while one is in flight — session/prompt and session/resume both reject with -32001 when busy. Document that coupling so a future relaxation of the busy gate doesn't silently reintroduce the F2 cleanup-clobber; cross-connection ownership stays a residual (F5). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): validate browser Origin in keyless loopback mode (R17-F1) WS handshakes bypass the browser same-origin policy, so a keyless `ws://127.0.0.1/acp` was reachable cross-origin by any web page. In keyless mode the upgrade handler now rejects a browser-set `Origin` that is not allowlisted (403); a request with no `Origin` (non-browser client) is still admitted, and the keyed/bearer path is unchanged. The allowlist is opt-in via `OPENAB_ACP_ALLOWED_ORIGINS` (comma-separated), plumbed through `AcpConfig` next to `auth_key`. Documented in the ADR §2 security note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): remove the ?token= query fallback (R17-F2) A bearer key in the URL query leaks into access logs / browser history / referers. The two header-borne sources — `Authorization: Bearer` and the `Sec-WebSocket-Protocol: openab.bearer.<token>` subprotocol — already cover both non-browser and browser clients, so drop the legacy `?token=` fallback. Token extraction is factored into `ws_bearer_token` (headers only); the `query`/`Query` extractor is gone from `ws_upgrade`. A request presenting a credential solely via `?token=` is now rejected 401 in keyed mode. ADR §2 updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): reject non-ContentBlock (string) prompt shapes (R17-F3a) The generated `PromptRequest.prompt` is `[ContentBlock]`. `extract_prompt_params` previously coerced a plain-string `prompt` (and fell through to a generic error for other shapes); now any non-array `prompt` is rejected, surfaced as -32602 invalid params at the call site, rather than leniently accepted. Baseline text / resource_link array handling is unchanged. Test updated: a string prompt now returns an error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): enforce ResourceLink required fields (R17-F3b) The generated `ResourceLink` requires both `name` and `uri`. The prompt parser previously required only `uri`, treating `name` as optional and falling back to `title`/a bare-uri render. It now rejects a resource_link missing its required `name` (-32602 invalid params) rather than silently rendering it, matching the schema. Accepted links still render as `[name](uri)`. Tests updated: a no-`name` link is now an error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): don't empty-success a request-shaped session/cancel (R17-F3c) ACP defines `session/cancel` as notification-only. A request-shaped cancel (with an `id`) was being acknowledged with `success({})`, which contradicts the ADR's "No response" contract and falsely implies cancel is a valid request method. Extract `handle_session_cancel`: the notification form still fires the session's cancel signal with no response; a request-shaped cancel is now rejected -32600 invalid request and does not fire. Tests: cancel with an id -> -32600 (no empty-success frame); notification -> no frame + fires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Implements Phase 1 MVP of the ACP Server per ADR #1258.
Any ACP-compliant client (Zed, JetBrains, desktop apps, web apps, CLIs) can now connect to OAB via WebSocket and interact with the multi-agent platform using the standard Agent Client Protocol.
Architecture
What's implemented (Phase 1)
GET /acpWebSocket upgrade endpointOPENAB_ACP_AUTH_KEYenv var)initialize— protocol version + capability negotiationsession/new— creates OAB session with internal channel mappingsession/prompt— converts prompt toGatewayEvent, dispatches through existing pipelineGatewayReplytranslated toAgentMessageChunknotifications (delta-based)session/cancel— placeholderAcpReplyRegistry) for routing replies back to the correct WebSocketacpfeature flagFiles changed
Cargo.tomlacpfeature flagcrates/openab-gateway/Cargo.tomlagent-client-protocoloptional dep +acpfeaturecrates/openab-gateway/src/adapters/acp_server.rscrates/openab-gateway/src/adapters/mod.rsacp_servermodulecrates/openab-gateway/src/lib.rsConfiguration
What's next (Phase 2+)
ToolCall,ToolCallUpdate)requestPermissionflow for dangerous operationssession/load— resume existing sessionsmodelparameterTesting
Refs: ADR PR #1258
cc @pahud