feat(acp): ACP server over WebSocket (revives #1260)#1418
Conversation
Restore the ACP server adapter from the auto-closed openabdev#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 openabdev#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>
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>
Absorbs the pending ADR proposal from openabdev#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>
openab-app
Bot
added
the
closing-soon
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
| } | ||
| let auth_key = std::env::var("OPENAB_ACP_AUTH_KEY").ok(); | ||
| if auth_key.is_none() { | ||
| warn!("OPENAB_ACP_AUTH_KEY not set — ACP endpoint is UNAUTHENTICATED"); |
There was a problem hiding this comment.
🔴 F1 — Fail closed when ACP authentication is absent
Enabling ACP with no key leaves /acp unauthenticated; an empty environment value is also accepted as a configured key. On the default 0.0.0.0 listener, a configuration omission exposes agent access to the network.
Requested change: Require a non-empty key before mounting /acp, or gate anonymous mode behind an explicit loopback-only opt-in.
|
|
||
| // Convert to GatewayEvent and dispatch | ||
| let event = GatewayEvent::new( | ||
| "acp", |
There was a problem hiding this comment.
🔴 F2 — Register ACP with the shared trust gate
Both gateway ingress paths gate this platform: "acp" event, but the trust registry never registers acp; its fallback is L3 deny-all. Authenticated prompts therefore stop before dispatch.
Requested change: Add an explicit ACP trust policy tied to successful transport authentication and test that authenticated prompts are admitted.
|
|
||
| # Opt-in: compile all gateway adapters into a single unified binary | ||
| unified = ["telegram", "line", "feishu", "googlechat", "wecom", "teams"] | ||
| unified = ["telegram", "line", "feishu", "googlechat", "wecom", "teams", "acp"] |
There was a problem hiding this comment.
🔴 F3 — Do not advertise incomplete unified support
This enables ACP in unified builds, but the unified Axum router does not mount /acp and UnifiedGatewayAdapter::dispatch_reply has no "acp" arm, so the feature is unreachable and replies would be dropped.
Requested change: Wire both route and reply dispatch in unified mode, or remove acp from this feature until that integration is complete.
| registry.lock().unwrap_or_else(|e| e.into_inner()).remove(key); | ||
| } | ||
| } | ||
| None | Some("send_message") => { |
There was a problem hiding this comment.
🔴 F4 — Use an explicit end-of-turn signal
Inferring completion from send_message breaks both modes: streaming normally finalizes by editing the first chunk and never emits Done, while send-once output over 4096 characters becomes multiple sends and this branch ends/removes the route after the first chunk.
Requested change: Carry explicit turn completion across the core↔gateway contract and test short streaming plus multi-chunk send-once responses.
| if let Some(k) = sess_key { | ||
| let notify = sessions.lock().await.get(k).and_then(|s| s.cancel.clone()); | ||
| if let Some(n) = notify { | ||
| n.notify_one(); |
There was a problem hiding this comment.
🔴 F5 — Propagate cancellation to the backend
This only wakes the WebSocket-side waiter. The core session keeps running, so tool calls and side effects can continue after the client receives stopReason: "cancelled".
Requested change: Send cancellation through a core control path to the underlying session and finalize the ACP request only after cancellation is propagated.
| registry | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .insert(channel_id.clone(), reply_tx); |
There was a problem hiding this comment.
🟡 F6 — Add connection ownership to resumed sessions
The reply registry is global and keyed only by channel ID, while each connection owns a separate session map. Concurrent connections can resume the same session and this insertion replaces the earlier waiter; stale disconnect cleanup can then remove the newer route.
Requested change: Track connection/turn ownership, define reject-or-handoff semantics, and use compare-and-remove cleanup.
| let mut prompt_tasks: Vec<tokio::task::JoinHandle<()>> = Vec::new(); | ||
|
|
||
| // Channel for sending messages back to the client | ||
| let (out_tx, mut out_rx) = mpsc::unbounded_channel::<String>(); |
There was a problem hiding this comment.
🟡 F7 — Bound client-controlled queues and session state
The outbound queue is unbounded and the same connection can create unlimited sessions and prompt tasks. A leaked shared token can therefore drive unbounded memory growth.
Requested change: Use a bounded queue and enforce per-connection session/prompt caps plus a global connection limit with explicit overload errors.
d28d352 to
01ab50b
Compare
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
| } | ||
| let auth_key = std::env::var("OPENAB_ACP_AUTH_KEY").ok(); | ||
| if auth_key.is_none() { | ||
| warn!("OPENAB_ACP_AUTH_KEY not set — ACP endpoint is UNAUTHENTICATED"); |
There was a problem hiding this comment.
🔴 F1 — Fail closed when ACP authentication is absent
Enabling ACP with no key leaves /acp unauthenticated, and an empty value is accepted as the expected key. The embedded server listens on 0.0.0.0:8080 by default, so a configuration omission exposes agent access.
Requested change: Require a non-empty key before mounting /acp, or gate anonymous mode behind an explicit loopback-only opt-in.
|
|
||
| // Convert to GatewayEvent and dispatch | ||
| let event = GatewayEvent::new( | ||
| "acp", |
There was a problem hiding this comment.
🔴 F2 — Bind ACP authentication to trust admission
This emits platform: "acp", but the shared trust registry never registers ACP; the unknown-platform fallback is L3 deny-all. Every prompt is rejected before dispatch even after the new embedded route is mounted.
Requested change: Register an ACP policy tied to successful transport authentication and test authenticated admission plus unauthenticated denial.
| registry.lock().unwrap_or_else(|e| e.into_inner()).remove(key); | ||
| } | ||
| } | ||
| None | Some("send_message") => { |
There was a problem hiding this comment.
🔴 F3 — Use an explicit end-of-turn signal
Treating send_message as completion breaks both delivery modes: normal streaming finalizes via edit_message and never emits Done, while send-once output over 4096 characters produces multiple sends and this first send closes the route before later chunks.
Requested change: Carry explicit turn completion/correlation across the core↔gateway contract and test short streaming plus multi-chunk send-once responses.
| if let Some(k) = sess_key { | ||
| let notify = sessions.lock().await.get(k).and_then(|s| s.cancel.clone()); | ||
| if let Some(n) = notify { | ||
| n.notify_one(); |
There was a problem hiding this comment.
🔴 F4 — Propagate cancellation to the backend
This only wakes the WebSocket-side waiter. The core session keeps running, so tool calls and side effects can continue after the client receives stopReason: "cancelled", and late output can race with a subsequent prompt.
Requested change: Cancel the underlying core session and retain a generation/ownership barrier until the old turn cannot affect a new prompt.
| registry | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .insert(channel_id.clone(), reply_tx); |
There was a problem hiding this comment.
🟡 F5 — Add process-wide ownership to resumed sessions
The global registry is keyed only by channel ID, while session state is per connection. A second connection can resume the same session and overwrite this sender; stale prompt or disconnect cleanup can then remove the newer route.
Requested change: Track connection/turn ownership, define duplicate-resume semantics, and use generation-bound compare-and-remove cleanup.
| let mut prompt_tasks: Vec<tokio::task::JoinHandle<()>> = Vec::new(); | ||
|
|
||
| // Channel for sending messages back to the client | ||
| let (out_tx, mut out_rx) = mpsc::unbounded_channel::<String>(); |
There was a problem hiding this comment.
🟡 F6 — Bound client-controlled state and queues
This outbound queue is unbounded, as are per-prompt reply queues, session entries, and prompt tasks. Full response snapshots are cloned into queues, so a slow or unauthenticated client can drive unbounded memory growth.
Requested change: Use bounded queues and enforce per-connection session/prompt caps plus a global connection limit with explicit overload errors.
| // The ACP endpoint (mounted below) needs this embedded HTTP server too — | ||
| // start it even when only non-webhook platforms (e.g. Discord, which the | ||
| // core connects to directly) are configured. | ||
| let acp_enabled = cfg!(feature = "acp") |
There was a problem hiding this comment.
🟡 F7 — Compile the embedded server for the ACP-only feature
This branch is nested inside an outer #[cfg(any(...))] that omits feature = "acp"; the same omission guards mod unified_adapter and unified_platform_enabled. Therefore --no-default-features --features acp compiles out the server and /acp route.
Requested change: Add ACP to all three guards and add an ACP-only feature-matrix build/test.
| continue; | ||
| } | ||
|
|
||
| let id = req.id.clone().unwrap_or(Value::Null); |
There was a problem hiding this comment.
🟡 F8 — Require IDs for request-only ACP methods
A missing ID becomes null, after which request-only methods execute and send id: null responses. Notification-shaped session/new and session/prompt can mutate state or start backend work without a correlatable request.
Requested change: Do not execute request-only ACP methods without an ID, and never send a response to a JSON-RPC notification.
| .get("authorization") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(|v| v.strip_prefix("Bearer ")) | ||
| .or_else(|| query.get("token").map(|s| s.as_str())); |
There was a problem hiding this comment.
🟡 F9 — Keep long-lived bearer credentials out of URLs
?token= can be captured by reverse-proxy/access logs, browser history, and observability systems, leaking the shared ACP key.
Requested change: Use Authorization for non-browser clients and a redacted short-lived ticket or secure cookie flow for browsers; otherwise require short-lived query credentials and documented log redaction.
35b59a4 to
19ec888
Compare
openab-app
Bot
removed
the
closing-soon
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
| if !enabled { | ||
| return None; | ||
| } | ||
| let auth_key = std::env::var("OPENAB_ACP_AUTH_KEY").ok(); |
There was a problem hiding this comment.
🔴 F1 — Fail closed when ACP authentication is absent
Enabling ACP without a key skips authentication entirely, and an empty configured key accepts an empty bearer/query token. On the default 0.0.0.0:8080 listener, a configuration omission exposes agent access.
Requested change: Require a non-empty key before mounting /acp, or gate anonymous mode behind an explicit loopback-only opt-in; test absent, empty, invalid, and valid keys.
| match reply.command.as_deref() { | ||
| Some("edit_message") => { | ||
| // Streaming update — send as text snapshot | ||
| if tx.send(ReplyChunk::Text(full_text)).is_err() { |
There was a problem hiding this comment.
🔴 F2 — Use an explicit end-of-turn signal
Streaming finalization arrives as edit_message, which emits text but never Done, so successful prompts wait for the 180-second timeout. Conversely, a multi-chunk send-once response emits Done and removes the route after its first chunk.
Requested change: Carry explicit correlated turn completion across the core↔gateway contract and test short streaming plus multi-chunk send-once responses.
| if let Some(k) = sess_key { | ||
| let notify = sessions.lock().await.get(k).and_then(|s| s.cancel.clone()); | ||
| if let Some(n) = notify { | ||
| n.notify_one(); |
There was a problem hiding this comment.
🔴 F3 — Propagate cancellation to the backend
This only wakes the WebSocket-side waiter. The core session keeps running, so tool calls and side effects can continue after the client receives stopReason: "cancelled", and late output can race with the next prompt.
Requested change: Cancel the underlying core session and retain a generation/ownership fence until the old turn can no longer affect a new prompt.
| registry | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .insert(channel_id.clone(), reply_tx); |
There was a problem hiding this comment.
🟡 F4 — Add process-wide ownership to resumed sessions
The global registry is keyed only by deterministic channel ID while session state is per connection. A second connection can replace this sender, and stale prompt/disconnect cleanup can remove the newer route.
Requested change: Track connection/turn generations, define duplicate-resume semantics, and use generation-bound compare-and-remove cleanup.
| let mut prompt_tasks: Vec<tokio::task::JoinHandle<()>> = Vec::new(); | ||
|
|
||
| // Channel for sending messages back to the client | ||
| let (out_tx, mut out_rx) = mpsc::unbounded_channel::<String>(); |
There was a problem hiding this comment.
🟡 F5 — Bound client-controlled state and queues
This outbound queue is unbounded, as are per-prompt reply queues, session entries, and prompt tasks. Full response snapshots are cloned into queues, so a slow client or leaked credential can drive unbounded memory growth.
Requested change: Use bounded queues and enforce per-connection session/prompt caps, a global connection limit, input limits, and deterministic overload behavior.
| // The ACP endpoint (mounted below) needs this embedded HTTP server too — | ||
| // start it even when only non-webhook platforms (e.g. Discord, which the | ||
| // core connects to directly) are configured. | ||
| let acp_enabled = cfg!(feature = "acp") |
There was a problem hiding this comment.
🟡 F6 — Compile the embedded server for the ACP-only feature
This runtime branch is nested inside outer #[cfg(any(...))] guards that omit feature = "acp"; the same omission guards mod unified_adapter and unified_platform_enabled. Therefore an ACP-only root feature build compiles out the server and route.
Requested change: Add ACP to all enclosing guards and add an ACP-only startup test for /health plus /acp.
| continue; | ||
| } | ||
|
|
||
| let id = req.id.clone().unwrap_or(Value::Null); |
There was a problem hiding this comment.
🟡 F7 — Require IDs for request-only ACP methods
A missing ID becomes null, after which request-only methods execute and send id: null responses. Notification-shaped session/new and session/prompt can mutate state or start backend work without a correlatable request.
Requested change: Do not execute request-only ACP methods without an ID, and never send a response to a JSON-RPC notification.
| .get("authorization") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(|v| v.strip_prefix("Bearer ")) | ||
| .or_else(|| query.get("token").map(|s| s.as_str())); |
There was a problem hiding this comment.
🟡 F8 — Keep long-lived bearer credentials out of URLs
?token= can be captured by reverse-proxy/CDN logs, browser history, and observability systems, leaking the shared ACP key.
Requested change: Use Authorization for non-browser clients and a redacted short-lived ticket or secure-cookie flow for browsers; otherwise require short-lived query credentials and documented log redaction.
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>
19ec888 to
0c099df
Compare
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>
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>
`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>
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>
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>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
| if !enabled { | ||
| return None; | ||
| } | ||
| let auth_key = std::env::var("OPENAB_ACP_AUTH_KEY").ok(); |
There was a problem hiding this comment.
🔴 F1 — Fail closed when transport authentication is absent
Enabling ACP without a non-empty key skips upgrade authentication. Because every connection becomes the same synthetic acp_client, allowing that identity in the trust registry makes an omitted key a network-wide bypass.
Requested change: Refuse to enable/mount /acp without a non-empty key, or require an explicit loopback-only insecure mode; test absent, empty, invalid, and valid keys.
| registry.lock().unwrap_or_else(|e| e.into_inner()).remove(key); | ||
| } | ||
| } | ||
| None | Some("send_message") => { |
There was a problem hiding this comment.
🔴 F2 — Do not infer turn completion from send_message
Core normally finalizes streaming with edit_message, which never emits Done, while any send_message emits Done and removes the route. Short streamed turns time out; multi-chunk output can end after the first send and drop the rest.
Requested change: Carry an explicit correlated turn-finished signal (or aggregate the complete turn) and test short streaming plus every overflow/send-once path.
| if let Some(k) = sess_key { | ||
| let notify = sessions.lock().await.get(k).and_then(|s| s.cancel.clone()); | ||
| if let Some(n) = notify { | ||
| n.notify_one(); |
There was a problem hiding this comment.
🔴 F3 — Propagate cancellation to the backend
This only wakes the local WebSocket waiter. The dispatched core/downstream turn keeps running, so tool calls and side effects can continue after the client receives stopReason:"cancelled".
Requested change: Route cancellation through the core session-control path, observe backend termination, and fence late output before allowing the next prompt.
| registry | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .insert(channel_id.clone(), reply_tx); |
There was a problem hiding this comment.
🟡 F4 — Bind reply routes to a connection/turn owner
The registry is process-wide and keyed only by deterministic channel ID, while busy is connection-local. Two holders of the same resumed session can overwrite this sender, and either stale prompt/disconnect can remove the newer route.
Requested change: Add connection/turn generations, reject or explicitly hand off duplicate ownership, and perform compare-and-remove cleanup.
| let mut prompt_tasks: Vec<tokio::task::JoinHandle<()>> = Vec::new(); | ||
|
|
||
| // Channel for sending messages back to the client | ||
| let (out_tx, mut out_rx) = mpsc::unbounded_channel::<String>(); |
There was a problem hiding this comment.
🟡 F5 — Bound client-controlled state and queues
The outbound channel is unbounded; the per-prompt reply channel is also unbounded, and session/task counts have no caps. A slow or malicious authenticated client can drive unbounded memory/task growth.
Requested change: Use bounded queues/backpressure, per-connection session/prompt caps, a global connection limit, and deterministic overload behavior.
| // The ACP endpoint (mounted below) needs this embedded HTTP server too — | ||
| // start it even when only non-webhook platforms (e.g. Discord, which the | ||
| // core connects to directly) are configured. | ||
| let acp_enabled = cfg!(feature = "acp") |
There was a problem hiding this comment.
🟡 F6 — Make the ACP-only feature path reachable
This check is nested inside outer #[cfg(any(...))] guards that omit feature = "acp"; the same omission guards mod unified_adapter and unified_platform_enabled. An ACP-only root build therefore compiles this branch out despite the ADR saying it starts the embedded listener.
Requested change: Add ACP to every enclosing/paired guard and cover --no-default-features --features acp with an endpoint smoke test.
| continue; | ||
| } | ||
|
|
||
| let id = req.id.clone().unwrap_or(Value::Null); |
There was a problem hiding this comment.
🟡 F7 — Enforce request-versus-notification shape
Converting a missing ID to null lets request-only methods execute and respond to notification-shaped messages. That can create sessions or start backend work with no correlatable request.
Requested change: Require IDs for ACP request methods, never execute/respond to notification-shaped requests, and validate session/cancel as a notification.
| .get("authorization") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(|v| v.strip_prefix("Bearer ")) | ||
| .or_else(|| query.get("token").map(|s| s.as_str())); |
There was a problem hiding this comment.
🟡 F8 — Keep long-lived credentials out of URLs
A shared key in ?token= can be retained by reverse-proxy/CDN logs, browser history, observability systems, and traces.
Requested change: Use the Authorization header where possible and a short-lived one-use ticket or secure-cookie flow for browsers; otherwise require expiry and log redaction.
|
|
||
| | Method | Direction | Purpose | OpenAB base | | ||
| |---|---|---|---| | ||
| | `session/cancel` | Client → Agent | Cancel in-flight work (one-way, no response) | ✅ conformant (notification; prompt ends `stopReason:"cancelled"`) | |
There was a problem hiding this comment.
🟡 F10 — Do not mark cancellation conformant before backend cancellation works
The implementation only cancels the local waiter; it does not cancel the backend turn. This table and the new accepted ADR also claim wire conformance/end-to-end verification while turn completion, request-shape handling, and ACP-only startup still diverge from the described contract.
Requested change: Downgrade these claims and list the known divergences until executable tests verify the full behavior.
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>
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>
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
| if !enabled { | ||
| return None; | ||
| } | ||
| let auth_key = std::env::var("OPENAB_ACP_AUTH_KEY").ok(); |
There was a problem hiding this comment.
🔴 F1 — Fail closed when ACP authentication is absent
Enabling ACP without a non-empty key skips upgrade authentication; an empty configured key also accepts an empty token. On the default network listener, a configuration omission can expose the admitted synthetic ACP identity.
Requested change: Require a non-empty key before mounting /acp, or gate an explicit insecure mode to loopback; test absent, empty, invalid, and valid keys.
| registry.lock().unwrap_or_else(|e| e.into_inner()).remove(key); | ||
| } | ||
| } | ||
| None | Some("send_message") => { |
There was a problem hiding this comment.
🔴 F2 — Do not infer turn completion from message commands
Streaming normally finalizes through edit_message, which never emits Done; multi-chunk send paths hit this branch repeatedly, but the first call emits Done and removes the route. Successful turns can therefore time out or lose later chunks.
Requested change: Carry an explicit correlated turn-finished signal (or aggregate the complete turn) and test streaming plus all overflow/send-once paths.
| if let Some(k) = sess_key { | ||
| let notify = sessions.lock().await.get(k).and_then(|s| s.cancel.clone()); | ||
| if let Some(n) = notify { | ||
| n.notify_one(); |
There was a problem hiding this comment.
🔴 F3 — Propagate cancellation to the backend
This only wakes the local WebSocket waiter. The dispatched core/downstream turn keeps running, so model requests, tool calls, and side effects can continue after the client receives stopReason:"cancelled". ACP requires the cancelled response only after ongoing operations have been aborted.
Requested change: Route cancellation through the backend session-control path, observe termination, and fence late output before allowing the next prompt.
| registry | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .insert(channel_id.clone(), reply_tx); |
There was a problem hiding this comment.
🟡 F4 — Bind reply routes to a connection and turn owner
The registry is process-wide and keyed only by deterministic channel ID while busy/session state is connection-local. Another connection can replace this sender, and stale prompt/disconnect cleanup can remove the newer route.
Requested change: Add connection/turn generations, reject or explicitly hand off duplicate ownership, preserve active resume state, and use compare-and-remove cleanup.
| let mut prompt_tasks: Vec<tokio::task::JoinHandle<()>> = Vec::new(); | ||
|
|
||
| // Channel for sending messages back to the client | ||
| let (out_tx, mut out_rx) = mpsc::unbounded_channel::<String>(); |
There was a problem hiding this comment.
🟡 F5 — Bound client-controlled state and queues
This outbound channel and the per-prompt reply channels are unbounded; session and prompt-task counts are also uncapped. A slow or malicious authenticated client can drive unbounded memory/task growth.
Requested change: Add bounded queues/backpressure, per-connection caps, a global connection/worker limit, input limits, idle eviction, and deterministic overload behavior.
| continue; | ||
| } | ||
|
|
||
| let id = req.id.clone().unwrap_or(Value::Null); |
There was a problem hiding this comment.
🟡 F7 — Enforce request-versus-notification shape
Converting a missing ID to null lets request-only methods execute and emit id:null responses. ACP/JSON-RPC notifications must never receive success or error responses; the generated schema now distinguishes these shapes but is not used here.
Requested change: Require IDs for ACP request methods, never execute/respond to notification-shaped requests, and validate session/cancel as a notification.
| .get("authorization") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(|v| v.strip_prefix("Bearer ")) | ||
| .or_else(|| query.get("token").map(|s| s.as_str())); |
There was a problem hiding this comment.
🟡 F8 — Keep long-lived credentials out of URLs
A shared key in ?token= can be retained by reverse-proxy/CDN logs, browser history, observability systems, and traces.
Requested change: Use Authorization where possible and a short-lived one-use ticket or secure-cookie flow for browsers; otherwise require expiry and mandatory log redaction.
| let send_task = tokio::spawn(async move { | ||
| while let Some(msg) = out_rx.recv().await { | ||
| if trace { | ||
| info!(connection = %send_conn, dir = "out", frame = %msg, "ACP frame"); |
There was a problem hiding this comment.
🟡 F9 — Do not log complete ACP payloads at INFO
These frames contain prompts, responses, resources, session capability IDs, and future tool inputs/outputs. INFO logs commonly have broad access and long retention, so enabling diagnostics can create a durable sensitive-data copy.
Requested change: Log redacted metadata by default; put any raw payload dump behind an explicitly unsafe development-only switch, document the risk, and add redaction tests.
| pub mod acp_server; | ||
| #[cfg(feature = "acp")] | ||
| #[allow(clippy::all, dead_code, unused)] | ||
| pub mod acp_schema; |
There was a problem hiding this comment.
🟡 F10 — Wire generated types into behavior or defer them
The latest commit adds 27,873 lines of schema/generated code, exports it under broad dead-code/lint allowances, but no runtime code consumes it. The live parser remains hand-written, so this does not enforce conformance or fix request/notification handling.
Requested change: Use the generated types at the current wire boundary with fixture/round-trip tests and narrow allowances, or defer this module until the phase that consumes it.
|
|
||
| | Method | Direction | Purpose | OpenAB base | | ||
| |---|---|---|---| | ||
| | `session/cancel` | Client → Agent | Cancel in-flight work (one-way, no response) | ✅ conformant (notification; prompt ends `stopReason:"cancelled"`) | |
There was a problem hiding this comment.
🟡 F12 — Do not claim cancellation conformance before backend cancellation works
The implementation confirms cancellation after stopping only the local waiter; it does not stop model/tool activity. Turn completion and request/notification handling also diverge from the claimed wire-conformant subset.
Requested change: List these divergences and downgrade the verified/conformant claims until executable tests prove the full contract.
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>
…treaming 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>
|
/review |
|
Important CHANGES REQUESTED What This PR DoesThis PR adds a feature-gated ACP v1 WebSocket server at How It Works
Findings
Finding Details🔴 F1: Reserve cancellation state before spawning the promptThe WebSocket loop spawns Requested change: Reserve a per-prompt cancellation state synchronously before spawning the task, or add a pending-cancel/generation state machine so a cancel received during task startup is consumed by that prompt. Add a back-to-back prompt/cancel regression test that does not wait for a first update. 🔴 F2: Preserve active ownership across resume and cleanup
Requested change: Reject resume while the local session is busy, or make resume generation/owner-aware. Cleanup must compare the active turn/owner before removing the registry entry or resetting 🟡 F3: Restore progressive ACP updates or narrow the contractThe ACP branch explicitly forces Requested change: Either implement a dedicated append-only ACP delivery path that emits multiple 🟡 F4: Run the ACP feature-gated tests in CIThe new server tests are behind Requested change: Add 🟡 F5: Make the smoke assertion fail closedAt Requested change: Require every expected marker (including Accepted Residual RisksThe PR explicitly documents backend cancellation propagation, full outbound backpressure/global limits, and the legacy query-token fallback as follow-ups or residual risks. They remain operational concerns, but are not duplicated as new blocking findings in this round. The synthetic identity also intentionally requires Addressing External Reviewer Feedbackbrettchien — live conformance reports (27/27)
✅ Accepted as evidence for those exercised paths. The run does not cover immediate prompt-to-cancel ordering, resume during an active prompt, or feature-gated CI coverage, so it does not resolve F1, F2, or F4 above. It also observes a cancellation response but cannot establish backend work termination, which the PR lists as an accepted residual risk. 5️⃣ Three Reasons We Might Not Need This PR
Baseline Check
What's Good (🟢)
|
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
| .and_then(|p| p.get("sessionId")) | ||
| .and_then(|v| v.as_str()); | ||
| if let Some(k) = sess_key { | ||
| let notify = sessions.lock().await.get(k).and_then(|s| s.cancel.clone()); |
There was a problem hiding this comment.
🔴 F1 — Reserve cancellation state before spawning the prompt
The prompt task installs AcpSession.cancel only after it is scheduled. If the next WebSocket frame is session/cancel, this lookup can observe None and silently drop the cancellation.
Requested change: Reserve a per-prompt cancellation state before spawning, or queue cancellation by prompt generation so an immediate cancel cannot be lost.
| format!("Too many sessions on this connection (max {MAX_SESSIONS_PER_CONNECTION})"), | ||
| ); | ||
| } | ||
| guard.insert( |
There was a problem hiding this comment.
🔴 F2 — Preserve active session ownership during resume
This unconditional insert resets busy and cancel for an existing session. A resume during an active prompt can replace the shared reply sink; the old prompt's cleanup then removes the newer turn's route and state.
Requested change: Reject resume while busy or make the session generation/owner-aware, and compare ownership before cleanup.
| // coupling): it streams append-only `agent_message_chunk` deltas built from the | ||
| // post+edit (`edit_message` snapshot) path, i.e. streaming=false. Decide it | ||
| // explicitly by platform rather than by whatever Telegram happens to be set to. | ||
| let streaming = if thread_channel.platform == "acp" { |
There was a problem hiding this comment.
🟡 F3 — Preserve progressive ACP updates
For platform == "acp", this forces streaming=false, so the edit/snapshot path is never used and clients normally receive only the final send_message chunk. That does not provide the progressive session/update streaming promised by the PR.
Requested change: Add a dedicated append-only ACP delivery path or narrow the documented contract to final-chunk delivery, with a test proving multiple updates precede the prompt response.
…6-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>
`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>
…l 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>
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>
… (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 openabdev#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>
|
/review |
…e (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>
|
Thanks for the round-16 review — the findings were precise and actionable. All five are addressed on the current head ( Round-16 findings
Local verification at the new head: Review statusThe Could a maintainer take a look for the human review pass? Happy to adjust anything. |
|
@brettchien We are evaluating an ACP-over-WS client for a persistent Live2D/Electron companion. Our current frontend uses the VTuber OpenAI-compatible endpoint and keeps continuity through the existing VTuber gateway path. For a future ACP client, is there an intended/supported way to link or hand off continuity between an ACP session and an existing platform/gateway session, or are ACP sessions intentionally isolated ( We understand Phase 1 deliberately emits a terminal reply rather than progressive chunks, which is fine for an initial canary. Is cross-connection ownership/handoff for |
|
@smallgun01 Thanks — good questions, and they land right on the boundaries the Phase-1 ADR calls out explicitly. 1. ACP session isolation vs. handoff to the existing gateway session Isolation is intentional by design. An ACP session lives entirely in its own namespace: So your instinct is right: treat the ACP client as its own companion session, and don't rely on cross-channel session behavior — it isn't supported and isn't planned as a cross-platform handoff. Continuity within ACP does persist (the core keeps a If what you actually want is one logical companion identity shared across both frontends, that's an orchestration concern on the client side (your app deciding both surfaces talk to the same logical companion), not something ACP is intended to broker. 2. Phase-1 terminal reply Correct, and glad it works as a canary. Phase-1 emits the whole reply as one terminal 3. For now, treat a session capability as single-active-client. The base is 1:1 by construction (ADR §5): the reply registry is The supported resume flow is reconnect: on WS disconnect the per-connection session map is dropped, and the same client reconnects with What is not in the base is generation/owner-aware cleanup for two different connections claiming the same Happy to go deeper on any of these, or to note your Live2D/Electron use case as a driver for the Phase-2 streaming + ownership work. |
|
Important CHANGES REQUESTED What This PR DoesThis PR adds a feature-gated ACP v1 WebSocket server at How It Works
Findings
Finding Details🔴 F1: Validate browser Origin in anonymous loopback modeWhen Requested change: Reject non-allowlisted 🟡 F2: Remove or harden the query-token compatibility pathThe handler still accepts Requested change: Remove the query fallback by default. If backward compatibility is required, use a short-lived one-use ticket and document mandatory URL redaction at every proxy and observability layer. 🟡 F3: Enforce the generated ACP request shapes at the wire boundaryThe generated Requested change: Deserialize request payloads through the generated types (or explicitly document and version extensions), reject non-conformant shapes, and add negative tests for string prompts, incomplete resource links, and request-shaped 🟢 F4: Good lifecycle hardeningThe prompt cancellation state is reserved synchronously before spawning, resume rejects an in-flight session without clobbering its cancel handle, and reply routing fences stale event ids. The PR also adds feature-gated ACP tests and a unified build step to CI, and the Phase-1 terminal-only streaming contract is now documented rather than overstated. Addressing External Reviewer FeedbackExternal client/session feedback
Accepted. The PR documents Existing review-round feedbackThe previously reported prompt-to-cancel race, busy-session resume clobber, ACP-only CI coverage, terminal-only streaming contract, and smoke assertion issues are acknowledged as addressed by the commits leading to the current head. Backend cancellation propagation, full outbound backpressure/global limits, and cross-connection ownership remain explicitly documented follow-ups/residuals. Baseline Check
What's Good (🟢)
5. Three Reasons We Might Not Need This PR
|
![chaodu-obk[bot]](https://avatars.githubusercontent.com/u/262003297?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1418 (comment)
4 participants
What problem does this solve?
OpenAB exposes agents only through platform-specific adapters (Discord, Telegram, LINE, …) and an OpenAI-compatible VTuber endpoint. Every new frontend — IDE plugin, desktop app, browser/web client, CLI — needs a bespoke adapter, and none of them get bidirectional streaming, tool-call approval, or session resume for free.
This adds an ACP (Agent Client Protocol) server over WebSocket at
GET /acp, so any standard ACP client can drive an OpenAB agent through one wire-conformant protocol.acp-server-websocket.md), plus an as-built companion ADR (acp-server-websocket-base.md) and a design-only forward ADR (acp-server-websocket-mcp-browser.md, Status: Proposed) that records the post-base roadmap referenced from base ADR §6. No browser/MCP code lands in this PR — the ADR is documentation only.Discord Discussion URL: https://discord.com/channels/1491295327620169908/1527521703255605338
At a Glance
Prior Art & Industry Research
OpenClaw —
openclaw acp(docs, acpx) is a Gateway-backed bridge: it speaks ACP over stdio and forwards prompts to the OpenClaw Gateway over WebSocket, mapping each ACP session id to a Gateway session key so IDEs can reconnect to the same transcript. This is essentially the architecture used here (ACP ↔ gateway event bus,sessionId↔channel_id/session_key, default isolatedacp:<uuid>session). The difference: OpenClaw's bridge is a locally-spawned stdio process; OpenAB exposes ACP directly at the gateway over WebSocket.Hermes Agent —
hermes acp(docs, #569) starts a stdio JSON-RPC ACP server used by VS Code / Zed / JetBrains, exposing session creation, prompt, streaming agent-message chunks, tool-call events, permission requests, cancel, and auth, with sessions that persist across editor restarts. OpenAB's Phase 1 matches the chat subset (create / resume / prompt / stream / cancel); tool-calls + permissions are deferred to Phase 2.Protocol — Agent Client Protocol (Apache-2.0, by Zed; backed by Zed + JetBrains). This PR is pinned to Schema v1.19.0; the exact method surface and OpenAB's coverage are tracked in
docs/acp-official-methods.md.Both references use stdio (local, IDE-spawned). OpenAB deliberately chooses WebSocket to also serve remote and browser clients, which stdio cannot.
Proposed Solution
A new
acp_servergateway adapter behind theacpfeature +OPENAB_ACP_ENABLED, wire-conformant with ACP Schema v1.19.0:initialize→ integerprotocolVersion: 1, officialagentCapabilities(sessionCapabilities.resume,loadSession: false,promptCapabilities),authMethods: [].session/new→{ sessionId };session/promptdelivers the reply via asession/updatenotification (agent_message_chunk) — Phase-1 sends the whole reply as one terminal chunk (backendstreaming=false); progressive multi-chunk streaming is Phase-2 — and returns{ stopReason }(end_turn/cancelled).session/cancel→ one-way notification; ends the in-flight prompt withstopReason:"cancelled".session/resume→ re-attach to a persisted session without replaying history.textandresource_link(ACP baseline) are accepted;resource_linkis passed through as a text reference (not fetched — SSRF-safe), and any other block type (image / audio / embedded resource) is rejected explicitly rather than silently dropped.sessionId = sess_<uuid>andchannel_id = acp_<uuid>share one uuid; prompts become aGatewayEventand OAB core keys continuity bysession_key = acp:<channel_id>.Why this approach?
session/resume, notsession/load— OpenAB keeps no replayable upstream transcript (conversation state lives in the downstream agent CLI), so it cannot satisfysession/load's replay contract; it advertisesloadSession: falseand re-attaches via core's persisted session mapping. Rationale and the core evidence are indocs/adr/acp-server-websocket-base.md§3.agent-client-protocolcrate dependency.Alternatives Considered
session/loadwith replay — deferred to Phase 3; requires an upstream transcript store OpenAB does not have today.Validation
Mirrors
ci.yml:cargo check --workspacecargo clippy --workspace -- -D warningscargo clippy --workspace --features unified -- -D warningscargo test --workspaceManual client testing against a live ACP client (Zed) for field-level exactness is noted as follow-up in
docs/acp-official-methods.md.Scope / follow-ups (not in this PR)
session/updatevariants +session/request_permission.session/loadwith history replay.cwd/mcpServersare accepted for wire conformance but not yet propagated to the agent.Review Contract
Goal
Ship a wire-conformant ACP v1 server over WebSocket at
GET /acpfor the streaming chat subset (initialize / session.new / session.resume / session.prompt / session.cancel), mounted on both the standalone gateway and the embeddedopenab runserver, with generated (typify) serde-only wire types and conformance/handler/streaming tests. Also lands the review-hardening fixes: JSON-RPC notification semantics, required-param validation, content-block rejection, resource caps, trace redaction, grapheme-safe message splitting, and full (untruncated) ACP reply delivery.Non-goals
session.request_permission,fs/*,terminal/*(later phases).session.loadhistory replay.Accepted Residual Risks
/acprequiresOPENAB_ACP_AUTH_KEYon any non-loopback bind (fail-closed off loopback, F1); no-key is permitted only on a loopback bind for local dev. The key is carried viaAuthorization: Beareror, for browsers, theSec-WebSocket-Protocolsubprotocol (openab.bearer.<key>) — kept out of the URL (F11);?token=remains only as a deprecated fallback. Residual: a header-logging middlebox could still capture the key (usewss://); identity admission still relies on the gateway trust registry (GATEWAY_ALLOWED_USERS).session.canceldoes not stop the backend — it ends the gateway waiter withstopReason:"cancelled"; downstream model/tool work may continue (F3).Acceptance Criteria
cargo clippy --workspace --features unified -- -D warnings,cargo test --workspace,cargo build --features unified.scripts/acp-ws-smoke.pydrives a live deployment.stopReason:end_turn→ resume, plus/modeland/resetreplies rendering back over ACP.Follow-ups
initializenegotiation validation (F8). (F10resource_linksupport is resolved — accepted as SSRF-safe passthrough; see Proposed Solution.)