Skip to content

docs(adr): ACP Server with WebSocket Transport#1258

Open
chaodu-agent wants to merge 2 commits into
mainfrom
docs/adr-acp-server-websocket
Open

docs(adr): ACP Server with WebSocket Transport#1258
chaodu-agent wants to merge 2 commits into
mainfrom
docs/adr-acp-server-websocket

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Proposes implementing an ACP (Agent Client Protocol) compliant server endpoint in OpenAB with WebSocket as the primary transport.

Architecture

┌─────────────────────────────────────────────────────────────┐
│                      ACP Clients                            │
├─────────────┬──────────────┬────────────┬───────┬──────────┤
│ IDE Plugin  │ Desktop App  │  Web App   │  CLI  │  Mobile  │
│ (Zed, JB)  │(AniCompanion)│ (Browser)  │       │          │
└──────┬──────┴──────┬───────┴─────┬──────┴───┬───┴────┬─────┘
       │             │             │          │        │
       └─────────────┴──────┬──────┴──────────┴────────┘
                            │
                   WSS  GET /acp  (JSON-RPC 2.0)
                            │
┌───────────────────────────┼─────────────────────────────────┐
│                    OpenAB Unified Binary                     │
│                           │                                 │
│  ┌────────────────────────▼────────────────────────────┐    │
│  │              ACP Server Adapter                      │    │
│  │  initialize / session/new / session/prompt           │    │
│  │  AgentMessageChunk / ToolCall / requestPermission    │    │
│  └────────────────────────┬────────────────────────────┘    │
│                           │                                 │
│  ┌────────────────────────▼────────────────────────────┐    │
│  │              Internal Dispatcher                     │    │
│  │         GatewayEvent / GatewayReply                  │    │
│  └───────┬────────────────┼────────────────┬───────────┘    │
│          │                │                │                │
│  ┌───────▼──────┐ ┌──────▼───────┐ ┌──────▼───────┐       │
│  │ Claude Agent │ │ Codex Agent  │ │  Kiro Agent  │       │
│  │  Container   │ │  Container   │ │  Container   │       │
│  └──────────────┘ └──────────────┘ └──────────────┘       │
│                                                             │
│  Existing adapters (Discord, Telegram, LINE, etc.)          │
│  continue to work in parallel                               │
└─────────────────────────────────────────────────────────────┘

Why ACP?

  • Industry standard — backed by Zed, JetBrains, GitHub, Block (3.5k★ spec repo)
  • Bidirectional — agent can request permission, push tool status, initiate messages
  • Any client connects — IDE, desktop app, web app, CLI — no bespoke adapters needed
  • Official SDKs — Rust, TypeScript, Python, Kotlin, Java

OAB's Unique Position

Existing ACP servers (Anvil, Goose, codex-acp):
  Local machine → stdio pipe → 1 agent process

OAB as ACP server:
  Remote clients → WebSocket → Multi-agent platform
                                 ├── Agent routing
                                 ├── Fan-out & aggregate
                                 ├── Session persistence
                                 ├── MCP bridging
                                 └── Tool policy engine

No one is building a remote multi-agent ACP server yet. This is OAB's differentiation.

What's in this PR

  • docs/adr/acp-server-websocket.md — full ADR with context, decision, consequences, alternatives considered, and phased implementation plan

Implementation Phases

  1. Core ACP Server (MVP) — WebSocket upgrade, initialize/session/prompt, streaming replies
  2. Tool Calls & Permissions — ToolCall notifications, requestPermission flow
  3. Multi-Session & Resume — multiple sessions per connection, session/load
  4. Multi-Agent Fan-Out — one prompt → multiple agents → aggregated result
  5. Streamable HTTP — POST+SSE fallback for serverless environments

References

cc @pahud

@chaodu-agent
chaodu-agent requested a review from thepagent as a code owner June 30, 2026 03:22
@chaodu-agent

This comment has been minimized.

@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Well-structured ADR with solid technical grounding for adopting ACP as the standard client protocol.

What This PR Does

Proposes adding an ACP (Agent Client Protocol) compliant server endpoint to OpenAB with WebSocket transport, replacing the need for bespoke per-frontend adapters with a single industry-standard protocol.

How It Works

A new GET /acp endpoint with WebSocket upgrade is added to the existing axum HTTP listener, using the official agent-client-protocol Rust crate for JSON-RPC dispatch. ACP lifecycle methods (initialize, session/new, session/prompt) map to OAB's internal GatewayEvent/GatewayReply flow, with agent routing via the model field. Feature-gated behind acp with Bearer token auth on upgrade.

Findings

# Severity Finding Location
1 🟢 Excellent strategic positioning — correctly identifies OAB's unique value as a remote multi-agent ACP server vs. existing local single-agent implementations §1 Context
2 🟢 Related ADR references (unified-binary.md, multi-platform-adapters.md) both exist on main — cross-references are valid §1 Related
3 🟢 Phased implementation plan is pragmatic — MVP first with progressive enhancement, deferring optional HTTP transport to Phase 5 §5 Implementation
4 🟢 Alternatives section is thorough — each rejected option (custom WS, OpenAI Realtime, MCP Streamable HTTP, per-frontend adapters) has clear justification §4 Alternatives
5 🟢 Correctly notes WebSocket transport RFD is in Draft status, honestly acknowledging spec dependency risk §3 Consequences
6 🟢 Clean feature-flag design with OPENAB_ACP_ENABLED and OPENAB_ACP_AUTH_KEY — non-invasive to existing adapters §2 Decision
What's Good (🟢)
  • Strategic clarity: The ADR identifies a genuine gap — no remote multi-agent ACP server exists — and positions OAB to fill it
  • Spec alignment: The proposed GET /acp with WebSocket upgrade matches ACP transport RFD conventions
  • Practical scope: Feature-gated, auth-gated, with graceful coexistence alongside existing adapters and the VTuber endpoint
  • Accurate cross-references: Both related ADRs (unified-binary.md, multi-platform-adapters.md) exist in docs/adr/ on main
  • Good risk acknowledgment: Honestly states spec dependency risk and current client adoption gap
  • Clean ADR structure: Follows standard Context → Decision → Consequences → Alternatives → Implementation format
  • Multi-agent fan-out vision: The ensemble dispatch concept (openab::ensemble) leverages OAB's existing multi-agent infrastructure in a novel way
Baseline Check
  • PR opened: 2026-06-30
  • Main already has: docs/adr/ directory with 20 existing ADRs (including referenced unified-binary.md and multi-platform-adapters.md)
  • Main does NOT have: any ACP-related code or documentation
  • Net-new value: First ADR establishing the architectural direction for ACP adoption — entirely new content, consistent with existing ADR corpus

thepagent pushed a commit that referenced this pull request Jul 23, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant