feat(acp): browser control via MCP-over-ACP + stdio bridge (Phase 2)

[brettchien]

What problem does this solve?

The ACP-over-WebSocket base (#1418) ships a 1:1 streaming chat surface at GET /acp: a browser side-panel extension connects as an ACP client and drives an OpenAB agent. It deliberately stops at chat — no tool calls, no way for the agent's LLM to act on the page it is talking about.

This PR delivers the base ADR §6 north-star: let the agent's LLM autonomously operate the user's real, logged-in Chrome (read the DOM, screenshot, navigate, click, type) by exposing the browser as MCP tools and routing them MCP-over-ACP over the /acp WebSocket the extension already holds. No sandbox VM, no second connection, no per-frontend adapter.

It also generalizes that mechanism: any ACP WS client may declare one or more type:acp MCP servers, and they reach the agent through the OAB MCP Facade (#1448/#1453) as session-aware capability sources (#1454) — not through a bespoke per-session MCP server. Design: acp-server-websocket-reverse-mcp.md — the mechanism, the §6 multi-server generalization, and §7's worked example (browser control, incl. the contract the extension implements).

Closes # — None; tracked by the base ADR §6 "Critical path" roadmap, not a separate issue.

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1527521703255605338

At a Glance

Architecture — the extension (a can't-listen MV3 client) is the MCP server, serving tools over its own outbound /acp WS; OpenAB is the MCP proxy; the agent LLM is the MCP client. Only the extension hop leaves the pod. Downstream, browser capabilities are delivered through the OAB MCP Facade (default); the pre-facade proxy and bridge transports remain as explicit OPENAB_BROWSER_MODE opt-outs.

flowchart LR
  EXT["<b>Side-panel MV3 extension</b> = MCP SERVER<br/>(cannot open a listening socket → serves MCP<br/>over the outbound /acp WS it already holds)<br/>declares {type:acp, id, name:'katashiro'}<br/>tools: katashiro.read_dom · screenshot · navigate · click · type"]
  subgraph POD["OPENAB POD — 'openab run', one process tree"]
    direction LR
    GW["<b>openab-gateway</b><br/>/acp WS server<br/>AcpTunnelRegistry<br/>keyed (channel_id, server_id)"]
    SRC["<b>AcpTunnelSource</b><br/>CapabilitySource<br/>requires_session<br/>allowlist + tool pin"]
    FAC["<b>OAB MCP Facade</b><br/>127.0.0.1:8848/mcp<br/>search_capabilities<br/>execute_capability"]
    AGENT["<b>agent CLI</b><br/>Cursor · Kiro · Claude · Codex<br/>LLM = MCP CLIENT"]
    GW <--> SRC
    SRC --> FAC
    FAC ==>|"Authorization: Bearer ${OPENAB_SESSION_TOKEN}<br/>(broker-minted per session, revoked on evict)"| AGENT
  end
  EXT <==>|"UPSTREAM — only remote hop<br/>MCP-over-ACP · mcp/message framing<br/>multiplexed with ACP chat on ONE /acp WSS<br/>8 MiB frame cap · JPEG screenshots"| GW
  classDef remote fill:#fde68a,stroke:#b45309,color:#111;
  classDef pod fill:#bfdbfe,stroke:#1e40af,color:#111;
  class EXT remote;
  class GW,SRC,FAC,AGENT pod;

Loading

MCP usage sequence (katashiro.click example) — Phase 1 connect + discovery, Phase 2 one autonomous action:

sequenceDiagram
    autonumber
    participant Tab as Chrome tab
    participant Ext as extension<br/>MCP SERVER
    participant GW as gateway /acp
    participant Src as AcpTunnelSource
    participant LLM as agent LLM<br/>MCP client
    Note over Ext,LLM: PHASE 1 — connect & discovery
    Ext->>GW: initialize · mcpServers=[{type:acp, id, name:"katashiro"}]
    Ext->>GW: session/new (or resume)
    GW->>GW: register TunnelHandle at (channel_id, server_id)
    GW->>Src: broker mints session token → agent MCP config
    LLM->>Src: search_capabilities (via facade)
    Src->>GW: tools/list (mcp/message) — once per declared server
    GW->>Ext: mcp/message → tools/list
    Ext-->>Src: real tool list → cached per (channel_id, server_id)
    Src-->>LLM: capabilities: katashiro.* (fetched ∩ allowed)
    Note over Tab,LLM: PHASE 2 — one autonomous action
    LLM->>Src: execute_capability katashiro.click(selector)
    Src->>GW: tools/call (mcp/message, same /acp WS)
    GW->>Ext: mcp/message → tools/call
    Ext->>Tab: chrome.scripting / tabs API
    Tab-->>Ext: DOM mutated / pixels
    Ext-->>LLM: tool result (JPEG screenshots, frame <= 8 MiB)
    Note over GW,Ext: only the gateway-to-extension hop leaves the pod

Loading

Prior Art & Industry Research

The problem: give the agent's LLM tools that drive the user's own remote, logged-in browser, where the tool provider is an MV3 extension that cannot open a listening socket.

OpenClaw — browser control is either an OpenClaw-managed, isolated Chrome profile driven by a local control service inside the Gateway (Control UI, managed browser), or the Chrome DevTools MCP server against a locally-reachable Chrome (MCP docs). openclaw mcp serve is a stdio bridge that keeps a stdio MCP session open and forwards to a local/remote Gateway over WebSocket — but it exposes routed channel conversations as MCP, not a remote browser. In every case the MCP server is colocated with the browser; OpenClaw does not route tools from a remote, can't-listen extension back to a colocated agent.

Hermes Agent — built-in browser tools work over accessibility-tree snapshots + stable refs + screenshots, with pluggable backends (Browserbase / Browser Use / Firecrawl cloud, or local Chromium / Camofox) (browser feature). hermes-computer-use is a pixel-level MCP server (screenshot + xdotool on an Xvfb display) that any MCP client connects to (repo). Here too the MCP server runs where the browser runs.

Takeaway — both projects colocate the browser-tool MCP server with the browser and let the agent be a normal MCP client. Neither tunnels MCP from a remote user's own can't-listen extension back to a colocated agent over the client's existing protocol connection. That inversion is the novel part. (ACP itself, and the general OpenClaw/Hermes ACP comparison, are covered in #1418.)

Proposed Solution

Three layers, behind the existing acp feature.

1. Protocol gap — agent→client REQUEST direction. The base only did client→agent prompts and agent→client notifications. Browser control needs the agent to ask the client and await a result, so acp_server's dispatch loop gains a server-initiated request path. Wire types are the generated serde-only v1 types from #1418.

2. Upstream hop — MCP-over-ACP tunnel (extension ↔ gateway). A tunnel frame API multiplexes MCP tools/list / tools/call / results over the same /acp WS using the official mcp/message framing; the client declares type:acp mcpServers on initialize and the gateway establishes a TunnelHandle per (channel_id, server_id) — a compound key, so one session can hold several declared servers (re-attach under the same name is last-write-wins). Opened on session/new and session/resume, torn down on cleanup. Wire contract: docs/mcp-over-acp-tunnel-contract.md.

3. Downstream hop — one CapabilitySource behind the OAB MCP Facade. Every client-declared server is exposed through a single in-process AcpTunnelSource (src/browser_source.rs) registered with the facade:

• requires_session() — anonymous facade clients neither discover nor can execute these tools. Identity is the facade's SessionTokens: the broker mints one opaque bearer per agent session, hands it to the agent as ${OPENAB_SESSION_TOKEN} (process env, not a config-file secret) and revokes it on evict.
• Multi-server fan-out — tools(ctx) returns the tools of every server that session's client declared; call routes on the <server>.<tool> prefix back to the right tunnel. No browser-specific branch in the source: admitting another client-side MCP service is config work, not a code change.
• Discovery — each declared server's real tools/list is fetched once on attach and cached per (channel_id, server_id); the catalog is served from cache regardless of current attach state, and backend unavailability surfaces as a call error, never a vanishing capability. notifications/tools/list_changed is deliberately not implemented: facade discovery is pull-based (search_capabilities re-reads per call), so nothing caches a tool list to invalidate.
• Trust gate — feat(facade): session-aware in-process capability sources #1454 assumes a source's tool set is the operator's grant, but a tunnel source's tools are client-declared. So an operator allowlist ([[mcp.acp_servers]], default: katashiro only) gates admitted server names, and each entry carries a deny-all tools pin. The name allowlist alone grants nothing — the catalog is fetched ∩ allowed, so a client declaring a trusted name still cannot publish extra tools.

proxy (per-session loopback HTTP MCP) and bridge (per-pod socket + openab browser-bridge stdio relay) are retained as explicit OPENAB_BROWSER_MODE opt-outs for CLIs or rollouts that need them.

Toolset — five DOM-semantic tools (katashiro.read_dom, katashiro.screenshot, katashiro.navigate, katashiro.click, katashiro.type). The ACP frame cap is raised 1→8 MiB to carry screenshot results (JPEG).

Why this approach?

• Extension-as-MCP-server over its own outbound WS — an MV3 extension cannot listen, and the user's browser is remote. Serving MCP over the /acp WS it already holds is the only way a can't-listen remote provider can be a full MCP server, and it reuses the one connection.
• MCP tools, not a custom ACP ExtRequest — only tools appear in the LLM's tool list, so only tools let the model autonomously act.
• DOM-semantic, not pixel computer tools — click(selector) / read_dom are cheaper, more reliable and model-agnostic.
• Behind the facade rather than a second inbound MCP server — the OAB MCP Adapter ADR (docs(adr): OAB MCP adapter #1446) §6.2 assigns the facade the same aggregator role and its Alternative C rejects "a second generic inbound MCP server". Reusing the facade also inherits its policy runtime (schema validation, timeouts, circuit breaking, redaction, audit) instead of reimplementing it.
• Static-advertise from a cache, not list_changed — keeps a stable catalog across reconnects without fabricating tools.

Known limitations — the browser tunnel binds to the ACP session, so the agent must be driven through the extension's ACP session (no Discord↔browser bridge). Under the facade the LLM reaches an action via search_capabilities → execute_capability, one hop more per turn than a direct tool; a per-provider "expose directly" option is deliberately deferred until interactive latency proves it necessary.

Alternatives Considered

• Custom ExtRequest per browser action — rejected: not surfaced to the LLM as a tool.
• Extension hosts a standalone HTTP/SSE MCP server — rejected: MV3 extensions cannot listen.
• Anthropic-style computer tool (screenshot + pixel coords) — subsumed; DOM-semantic tools are cheaper and model-agnostic.
• Colocated/sandbox browser (OpenClaw-managed / Hermes Xvfb) — rejected: the goal is the user's real, logged-in Chrome.
• Bespoke per-session MCP server + N mcp.json entries (this PR's own earlier design) — superseded by the facade integration; it would have reinvented the catalog, discovery and policy runtime the facade already provides, and violated the adapter ADR's "one aggregation point".
• One source per declared server — impossible by construction: facade sources are registered once at startup, so a single source fans out internally instead.

Validation

Rust changes, gated behind --features acp (openab-gateway/acp + openab-core/acp-mcp):

• cargo build --features acp — clean
• cargo clippy --workspace -- -D warnings and cargo clippy --workspace --features unified -- -D warnings — both clean
• cargo test -p openab-gateway --features acp — green (the acp-gated test target CI runs)
• Unit/integration coverage for the new behaviour: compound-key registry + last-attach-wins, multi-server routing by <server>.<tool>, allowlist + tool-pin refusal (unpinned tool denied, not "unknown server"), discovery cache narrowing (fetched ∩ allowed, cache can never widen past policy), and two client-declared servers in one session
• MCP-over-ACP tunnel e2e in scripts/acp-ws-smoke.py (producer section: tools/list / tools/call, fan-out + filtering)
• Live deployment — Facade mode running on a real agent (OrbStack, Cursor CLI): facade listener up on 127.0.0.1:8848, search_capabilities returns provider openab-browser with exactly the five pinned katashiro.* capabilities alongside host-level mcp.json providers, and anonymous probes of the facade correctly see only the two meta-tools (session-bound source hidden without a token)
• Live execute_capability round-trip driving the real tab — the browser extension, rebuilt on the renamed surface, declares {type:acp, name:"katashiro"}; the gateway registers the tunnel at (channel_id, server_id), the broker mints the session token, and the agent's execute_capability katashiro.read_dom reaches the user's actual tab and returns its DOM. The facade's audit trail brackets the dispatch:

  mcp.audit: facade source call      provider="openab-browser" tool=katashiro.read_dom channel="acp_27da364b…" args_sha256=44136fa3…
  mcp.audit: facade source call exit provider="openab-browser" tool=katashiro.read_dom channel="acp_27da364b…" args_sha256=44136fa3…
  

• Two-window session isolation (each window's agent reaching only its own browser) — covered by unit tests, not yet re-verified live on the renamed surface

Review Contract

Goal

Let a colocated agent's LLM autonomously operate the user's real remote browser via DOM-semantic MCP tools tunnelled MCP-over-ACP over the existing /acp WebSocket, and do it through the OAB MCP Facade as a session-aware CapabilitySource rather than a second agent-facing MCP server. Concretely: the agent→client request direction; the mcp/message tunnel keyed by (channel_id, server_id); AcpTunnelSource with multi-server fan-out, per-server discovery cache and an operator allowlist with deny-all tool pinning; broker-minted per-session tokens for identity.

Non-goals

• Retiring the pre-facade proxy / bridge transports (kept as OPENAB_BROWSER_MODE opt-outs; see Follow-ups).
• notifications/tools/list_changed — deliberately dropped, not deferred (pull-based facade discovery has no consumer for it).
• A per-provider "expose tools directly" bypass of the meta-tool path.
• Any Discord↔browser bridge: the tunnel is bound to the extension's ACP session by design.
• Fine-grained per-tool consent UX (core still auto-approves permissions, ADR D1).

Accepted Residual Risks

• Downstream MCP is loopback + bearer only. The facade binds 127.0.0.1 and trusts the host boundary; the per-session bearer rides the agent's process env. A hostile in-pod process shares the agent subprocess's trust boundary already.
• Client-declared tool sets are gated, not verified. The allowlist + deny-all pin stop a client publishing extra tools under a trusted name, but a compromised extension can still misuse the five pinned tools within the user's logged-in session — which is inherent to driving the user's real browser.
• 8 MiB frame cap — screenshots must be JPEG; a pathological result over the cap closes the frame rather than truncating.
• Static-advertise from cache can briefly show a capability whose backend has just detached; the call then returns an MCP error rather than the capability disappearing mid-turn. This is the deliberate trade (ADR §6.3).
• bridge mode resolves its channel by process ancestry — correct for the normal spawn tree; a relay started outside it fails closed.

Acceptance Criteria

• cargo build --features acp, cargo clippy --workspace [--features unified] -- -D warnings, and cargo test -p openab-gateway --features acp all green.
• Multi-server behaviour proven by test: two declared servers in one session are both discoverable and callable, routed by prefix, with no name collision.
• Trust gate proven by test: an unpinned tool from an allowlisted server is refused as a tool-pin refusal, and the cache can never widen the catalog past policy.
• Live: facade serving, search_capabilities lists exactly the pinned capabilities for a session-bound client, and anonymous clients see none of them.

Follow-ups

• Retire the bespoke per-session proxy and bridge mode once Facade mode has soaked. Beyond the adapter ADR's "one aggregation point", live testing surfaced a concrete hazard: each transport writer only adds its own mcp.json entry and never removes the other's, so an agent that has run in more than one mode loads both servers and exposes the same tools twice — once through the facade (policy + audit) and once straight through the old transport (neither). The model picks the direct one and the call leaves no audit trail at all while working perfectly; auditing looks dead with nothing indicating why. Documented in browser-mcp-agent-setup.md as an operational warning, but the durable fix is one transport (and, until then, cleanup-on-mode-switch).
• Reconcile the flagged divergence with the adapter ADR: it specifies delivery via ACP mcpServers and explicitly not by writing CLI config files, while the as-built writes a static openab entry because Cursor ignores ACP-passed mcpServers (browser ADR D2 / zed#50924). Owner of the facade contract to decide.
• Re-verify two-window session isolation live on the renamed surface (unit-tested; the live check predates the rename).
• Optional: carry a tool manifest in the client's initialize declaration so the catalog is known without a first tunnel round-trip.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - The exact head does not advertise or implement the required bidirectional MCP-over-ACP contract and leaves session authority unsafe during a hung-session replacement.

What This PR Does

This PR adds an MCP-over-ACP tunnel so an ACP client can provide MCP tools, including browser-control tools, to an OpenAB agent through the existing WebSocket. It adds tunnel registration, inner MCP lifecycle handling, facade-backed session tokens, discovery, and CI coverage.

How It Works

The gateway accepts type:acp declarations, creates a tunnel per declared server, and exposes discovered tools through AcpTunnelSource behind the local MCP facade. The facade resolves a session token to a channel and routes calls to the tunnel registered for that channel and server name.

Findings

#	Severity	Finding	Location
1	🔴	The required mcpCapabilities.acp capability is emitted only in _meta, so conforming clients will not detect native MCP-over-ACP support.	crates/openab-gateway/src/adapters/acp_server.rs:1913-1920
2	🔴	Inbound mcp/message requests and notifications are unimplemented despite the bidirectional protocol contract.	crates/openab-gateway/src/adapters/acp_server.rs:1800-1809
3	🔴	A hung, evicted connection can retain a valid channel-scoped facade token and use a successor session's browser tunnel.	crates/openab-core/src/acp/pool.rs:797-846
4	🟡	Session busy state is per WebSocket, while replies are globally keyed only by channel, so concurrent cross-socket resume can overwrite and remove another live turn's reply sink.	crates/openab-gateway/src/adapters/acp_server.rs:1321-1323,2261-2264,2353-2355
5	🟡	Generic MCP tools are advertised unchanged but calls require the tool name to start with the declared server name.	src/acp_tunnel_source.rs:238-259
6	🟡	Hung-session warnings log raw ACP channel and session identifiers even though either is a resumable-session credential.	crates/openab-core/src/acp/pool.rs:807-810

Finding Details

🔴 F1: Advertise the standardized capability field

The official MCP-over-ACP RFD requires clients to check mcpCapabilities.acp in the initialize response. The gateway instead sends _meta: { "acp": true }; its own comment recognizes that the generated schema lacks the field. A conforming client will therefore conclude that ACP transport is unsupported and will not declare a type:acp server.

Requested change: Update the schema dependency or response model so the wire response contains "mcpCapabilities":{"acp":true,"http":false,"sse":false}. Add a wire-level compatibility test that asserts the direct field, not a private _meta convention.

🔴 F2: Implement both directions of mcp/message

The local tunnel contract and the official RFD both state that mcp/message is bidirectional. The dispatcher has no mcp/message arm; request-shaped frames fall through to -32601, and notification-shaped frames are ignored. This prevents a generic client-provided MCP server from sending server-originated MCP requests or notifications after connection.

Requested change: Route inbound mcp/message by connectionId, preserve request versus notification semantics, and add real-socket tests for an inbound request and an inbound notification. Keep the contract and implementation aligned for notifications/tools/list_changed as well.

🔴 F3: Revoke authority when hung eviction begins

Hung cleanup removes the active connection after scheduling cancellation and process termination, but it does not revoke the exact facade token at that point. The token is retained by an AcpConnection drop guard; a hung task can keep that object alive after eviction. SessionTokens intentionally allows multiple tokens for one channel, and AcpTunnelSource authorizes by channel only, so an old agent can still call whichever tunnel a replacement session registered for that channel.

Requested change: Revoke the exact token synchronously when detaching a hung connection, or bind the session context and tunnel lookup to a connection generation. Add a replacement-while-hung test proving the predecessor token cannot invoke the successor tunnel.

🟡 F4: Make resume and reply ownership process-wide

session/resume checks a map local to each WebSocket. Two clients holding the same resumable session identifier can both mark the session idle and start prompts. Both then overwrite the process-global reply sink under the same channel key, and either completion unconditionally removes it. The code documents this as an accepted cross-connection residual, but it loses or misroutes a live turn.

Requested change: Establish one process-wide owner or generation per resumable session, and make reply-sink insert/remove conditional on that owner and turn. Cover two concurrent WebSocket connections resuming and prompting the same session.

🟡 F5: Route tools without imposing an undocumented naming rule

tools() returns names exactly as the connected server publishes them, but call() rejects every name without a dot and uses the first segment as a declared server name. MCP tool names do not have to be prefixed by their ACP declaration name; for example, a server named project-tools that publishes build is discoverable but not callable.

Requested change: Maintain an advertised-name to (server, original-tool) mapping, or expose a documented collision-safe alias and validate against it before forwarding. Add a test for an unprefixed tool and for a tool whose first segment differs from the declared server name.

🟡 F6: Apply existing ACP credential redaction to hung cleanup

redact_session_ids explicitly treats both acp_<uuid> and sess_<uuid> as credentials because either resumes the session. The hung-session warning bypasses that helper and emits both raw values. Logs commonly have a broader retention and audience than an active ACP session.

Requested change: Redact key and session_id in this warning and add a regression test that neither raw identifier reaches structured log fields.

Baseline Check

• PR opened: 2026-07-24.
• Base branch: main at c5a75ac6e8fdc11a3b229a0c609769e90d261daf.
• Reviewed head: ff0a8c3d5f42c7c6c3203065d74b4035855771bf.
• Merge-base: c5a75ac6e8fdc11a3b229a0c609769e90d261daf.
• Diff: 29 files, 7804 additions, 254 deletions.
• Main already has the ACP WebSocket and MCP facade foundations. The net-new value is reverse MCP-over-ACP routing, session-aware tunnel capability delivery, and browser-control integration.

Validation and Review Coverage

• The specified SHA was verified as the current PR head before publishing.
• git diff --check origin/main...pr-1447 passed.
• GitHub reports 45 completed exact-head check runs, all successful.
• Local Rust validation could not run because cargo and rustc are absent in this review environment.
• The review inspected protocol compatibility, session/token isolation, generic tool routing, CI, docs, and external feedback. The official MCP-over-ACP RFD was used for the capability and bidirectional-message requirements.

Addressing External Reviewer Feedback

Prior consolidated reviews (Rounds 1-3)

Earlier rounds identified tunnel lifecycle, framing, transport cleanup, token lifecycle, and feature-gated CI issues.

ℹ Rechecked on this head: many earlier findings have targeted fixes and the exact-head CI is green. The six findings above are independently reproducible on the current SHA and are not duplicated from completed rounds.

PR author update

The implementation was narrowed to facade-only delivery and documented as a generic client-provided MCP-server mechanism.

ℹ Accepted as current scope: this review evaluates that final facade-only shape. The direct capability advertisement, bidirectional wire behavior, and generic tool-routing requirements remain necessary for the stated generic MCP-over-ACP contract.

What's Good (🟢)

• The compound tunnel key, generation-aware replacement work, inner MCP initialization, and feature-gated CI coverage are meaningful improvements over the earlier rounds.
• Session tokens are randomly generated, compared in constant time, kept out of generated configuration, and the facade remains loopback-bound.
• The source caches discovery per channel and keeps attachment flapping from needlessly erasing the catalog.
• The PR documents its architecture and residual decisions in substantially more detail than a typical transport change.

5. Three Reasons We Might Not Need This PR

1. A narrower browser-only integration may be safer first - The generic transport claim adds interoperability and lifecycle requirements beyond the one extension currently exercised.
2. Existing colocated-browser MCP deployments may satisfy many users - They avoid session-resume and remote-client routing complexity where the user does not need their existing logged-in browser.
3. The facade can mature independently - Landing the session and protocol ownership model before exposing remote browser actions could reduce the cost of securing a privileged capability surface.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ - Four important correctness and contract issues remain in the capability catalog, reconnect behavior, documentation, and audit correlation.

What This PR Does

This PR adds a gateway-initiated MCP-over-ACP tunnel so an ACP client can expose MCP tools to an OpenAB agent over the existing WebSocket. It connects those session-scoped tools to the OAB MCP Facade through AcpTunnelSource.

How It Works

The gateway registers client-declared type:acp servers by (channel_id, server_id). The facade source discovers each server's tools, caches the catalog by (channel_id, declared_name), and routes execute_capability calls through the currently resolved tunnel.

Findings

#	Severity	Finding	Location
1	🟡	Duplicate tool names are advertised with aliases, but every alias dispatches to the same publisher.	src/acp_tunnel_source.rs:152-173, crates/openab-mcp/src/mcp/facade.rs:212-215,261-303
2	🟡	A same-name reconnect never refreshes its discovered tool catalog.	src/acp_tunnel_source.rs:231-258
3	🟡	The external tunnel contract documents a 180s default while the actual default is 170s.	docs/mcp-over-acp-tunnel-contract.md:178, crates/openab-core/src/config.rs:127-129
4	🟡	ACP session redaction uses incompatible hash inputs across the new call and audit paths.	crates/openab-core/src/redact.rs:25-39, crates/openab-gateway/src/adapters/acp_server.rs:2168-2179

Finding Details

🟡 F1: Preserve publisher identity for colliding tool names

When two declared servers publish the same tool name, AcpTunnelSource::tools() returns both names. The facade exposes the first as build and the later one as openab-browser:build. During execution, however, the alias matches the first Tool in the source loop and calls source.call(..., "build", ...). server_publishing() then selects the lexicographically first publisher. Both advertised capability names therefore invoke the same server; the other publisher is unreachable.

Requested change: Deduplicate collisions before publishing, or retain a collision-safe (declared_server, original_tool_name) identity through discovery and execution. Add a two-server test where both publish build and prove each published capability reaches its own tunnel.

🟡 F2: Refresh a catalog when its server reconnects

The cache is keyed by declared name, and tools() starts discovery only for a None cache entry. After a server reconnects with the same name and a new id, its existing Some(fetched) entry is returned indefinitely; no tools/list request is made for the new tunnel. Tools added by the reconnecting server never appear, removed tools remain advertised, and calls can be sent to a server that no longer implements the cached tool. This contradicts the contract's statement that changed tools are picked up by re-discovery.

Requested change: Record the tunnel id or attachment generation with cached tools and refresh after it changes, while retaining the last successful catalog if the refresh fails. Add a reconnect test with a changed tool list and assert the new id is used.

🟡 F3: Correct the documented tunnel timeout default

The protocol contract tells extension implementers that [mcp] tunnel_timeout_seconds defaults to 180 seconds. default_tunnel_timeout_seconds() returns 170 seconds, intentionally below the 180-second ACP prompt idle ceiling. An implementation following the published contract can plan cancellation and retry timing against a value OpenAB never uses.

Requested change: Change the contract default to 170 seconds and keep the default/ceiling relationship covered by one shared regression assertion or generated documentation input.

🟡 F4: Use one ACP session tag algorithm everywhere

redact_session_ids() removes the acp_ or sess_ prefix before hashing, as does the gateway's redact_id(). The facade and other redact_channel() copies hash the full acp_<uuid> string instead. For the same test UUID the two algorithms produce #12b9377c and #850414fa, respectively. The new source error path and facade audit path can therefore log one session under different tags, despite comments promising cross-crate correlation.

Requested change: Standardize on hashing the UUID portion and delegate to one helper where dependency direction permits; otherwise pin a cross-module vector that tests both input forms and all implementations.

Baseline Check

• PR opened: 2026-07-24.
• Base branch: main at c5a75ac6e8fdc11a3b229a0c609769e90d261daf.
• Reviewed head: 3d74995aad205dd731ee7fa024b8057ea2fbcab4.
• Merge-base: c5a75ac6e8fdc11a3b229a0c609769e90d261daf.
• Diff: 29 files, 8346 additions, 312 deletions.
• Main already has the ACP WebSocket and MCP facade foundations. The net-new value is reverse MCP-over-ACP routing and session-aware client capability delivery.

Validation and External Feedback

• The requested SHA was verified as the current PR head before publishing this review.
• git diff --check origin/main...pr-1447 passed in an isolated worktree.
• The completed check, validate, and Review Contract jobs were successful; several smoke jobs were still in progress at review time.
• Targeted Rust tests could not run because this review environment has no cargo executable. Static inspection and deterministic SHA-256 vectors were used for the findings above.
• Existing inline threads and prior consolidated reviews were read. The current non-outdated frame-limit thread remains historical context; no separate current-head inline finding is being duplicated here.

Addressing External Reviewer Feedback

Prior consolidated review rounds

Earlier rounds raised tunnel lifecycle, session authority, framing, transport cleanup, and CI concerns.

ℹ Rechecked: those comments target earlier heads or behavior that was removed during the facade-only redesign. This round is limited to four independently reproducible issues on 3d74995aad205dd731ee7fa024b8057ea2fbcab4 and does not duplicate retired findings.

PR author updates

The implementation is now facade-only and client-declared MCP servers publish their discovered tools through the tunnel.

ℹ Accepted as current scope: this review evaluates that final shape. The duplicate-name routing and reconnect-refresh bugs arise specifically from the generic multi-server discovery model and must be fixed for the advertised capability catalog to be reliable.

What's Good (🟢)

• The per-session server count and in-flight establish limits give the new connection path explicit resource boundaries.
• The source correctly requires a facade session token, keeping client-provided tools hidden from anonymous facade clients.
• The gateway's UUID-only redaction helper correctly recognizes both acp_ and sess_ encodings.
• The new CI steps explicitly select ACP-gated gateway, core, pool, and root test areas rather than relying only on default workspace features.

5. Three Reasons We Might Not Need This PR

1. A browser-specific first version could be smaller - Generic multi-server fan-out introduces collision and cache-invalidation rules that the single browser client does not need immediately.
2. The remote-client trust boundary is substantial - A colocated MCP server avoids resumable-session and tunnel lifecycle complexity for users without a logged-in remote browser requirement.
3. The facade can mature separately - Stabilizing generic capability naming and catalog refresh before exposing privileged browser actions would reduce rollout risk.

[brettchien]

Round 4 — all six findings answered, head c9d5890

Round 4 reviewed ff0a8c3. Nine commits have landed since. Four findings are fixed, one shipped in a
different shape than asked with the reasoning below, and one is declined with its reason stated
rather than deferred.

Two things before the itemised part. Every check run on c9d5890 is green — the two non-successes
are the operator and poll-and-review jobs, both skipped, and Review Contract passes; the
OpenAB PR Review status is still pending, so nothing here has been re-reviewed at this head. And
round 5 landed at 01:30 against 3d74995, two commits behind the current head; one of its
findings is a direct residue of the F5 fix below, so it is called out at the end rather than left to
be discovered.

F1 — declared as a reverse-DNS _meta key, not a core field

7153004, with a doc-precision follow-up in c9d5890.

The ask was a core mcpCapabilities.acp field. The 2026-07-28 MCP spec moves the other way: _meta
is the sanctioned place for this kind of declaration, and namespaced keys there are reverse-DNS (its
own reserved keys look like io.modelcontextprotocol/logLevel). So the bare _meta.acp we shipped
was an informal convention the spec now supersedes, and a core field is the pre-convention direction.

The key is now dev.openab/acp under agentCapabilities.mcpCapabilities._meta — the reverse-DNS
of openab's domain (openab.dev) plus the capability key. It rides the free-form _meta map of the
vendored v1 McpCapabilities, which carries only http, sse and _meta (acp_schema.rs:6201);
no extensions field is added, so the generated wire types are not forked, which is what F1(a)
deliberately avoided. handle_initialize and the conformance mirror both advertise it, ADR §3.1
documents it, and the test asserts three things: the reverse-DNS key is present, the bare _meta.acp
key is gone, and there is no core mcpCapabilities.acp field.

c9d5890 corrects a citation, not the behaviour. The 2026-07-28 spec has two easily-conflated
mechanisms — the _meta namespaced-key convention (SEP-1788, which is what logLevel and
dev.openab/acp actually are) and a separate typed extensions map. The ADR and the code
comment described the latter while the implementation follows the former. Both now name them apart
and record that the declaration would move to the typed map if and when the vendored schema gains
it. Wording only; the wire string is unchanged.

F2 — declined, not deferred: the transport is gateway-initiated

3d74995, documentation only.

The finding is correct about the code: there is no inbound mcp/message dispatch arm, so
server-originated requests and push notifications are not carried. What was wrong was our
documentation, which described the transport as bidirectional in a way that implied inbound MCP
semantics. Contract §4 and ADR §6.3 now state plainly that the shipped tunnel is
gateway-initiated — the gateway asks (initialize / tools/list / tools/call), the extension
answers — and that the gateway has no dispatch arm for an inbound mcp/message.

We are not implementing the inbound direction, and the reason is the spec rather than cost. The
2026-07-28 revision retires exactly that surface: server-initiated requests are deprecated
(SEP-2577) and redesigned into multi-round-trip requests (SEP-2322, InputRequiredResult /
inputRequests re-issued with inputResponses); change-notification delivery moves off HTTP/SSE
push to subscriptions/listen, with cache-expiry-and-refetch as the preferred lower-push mechanism;
and the streamable-HTTP session model is removed as the transport goes stateless. Building the push
form now means building on a mechanism the spec is actively restructuring, on a twelve-month
offramp.

The notification half is also consumerless today. The example extension has a static five-tool set
(read_dom, screenshot, navigate, click, type), so it never emits tools/list_changed and
an invalidation path would never fire. Both documents now say this with the citations, and commit to
aligning with the new mechanisms if and when a real client-provided server needs them.

F3 — the facade token is revoked synchronously on every removal path

84cde81, with a feature-gating fix in fd45f92.

Diagnosis confirmed. Revocation rode the AcpConnection drop guard, and a hung connection never
drops because the stuck streaming task still holds an Arc. Combined with AcpTunnelSource
authorizing by channel alone, a hung predecessor's token stayed valid for whatever tunnel a
successor later registered for that channel.

Each connection's exact token is now recorded in PoolState.facade_tokens, keyed by pool key, so
eviction never needs the connection lock — and revocation is by exact token value, which makes
it idempotent and safe to overlap with the guard on clean paths. Five paths revoke: hung eviction
(after apply_hung_eviction, which is why purge_session_entries deliberately leaves
facade_tokens in place — so the token is still there to revoke), install-time supersede inside
install_facade_token (covering the ordering where a successor registers before the hung
predecessor is detected), idle eviction, pool-full suspend, and reset.

On coverage, to be precise about what the tests do and do not reach: two helper-level tests assert
the exact revoked-token list — installing a successor revokes only the superseded predecessor and
keeps the successor's, and revoking by key revokes that key's token and leaves an unrelated
session's untouched. Driving the full path end-to-end needs a real AcpConnection to enter
apply_hung_eviction, which the fixture cannot build, so the wiring from that call site to the
helper is by inspection rather than by test.

fd45f92 is the batch gate earning its keep: the default-feature clippy pass caught facade_tokens
being added unconditionally while every reader is #[cfg(feature = "acp-mcp")] — a -D warnings
error that a targeted --features acp-mcp check could not see.

F4 — the reply sink is generation- and turn-scoped

fc8584f.

Accepted as described. Busy state is per connection while the reply-sink registry is process-wide
and keyed by channel_id, so two connections resuming one session both saw busy=false, both
inserted a sink under the same key, and either turn's completion did an unconditional
remove(channel_id) — deleting whichever sink was there, including a successor's live one. That
unconditional remove was the accepted residual from round 3's F5; it is now closed.

The fix is on the registry, not on resume ownership: the per-connection busy gate cannot serialize
turns across connections, so the registry operations became conditional instead. A sink now carries the
connection_generation of the connection that installed it — already stamped at accept — alongside
its turn_id and owning connection id.
install_reply_sink refuses to install over a strictly newer generation — a reconnecting client
resuming the same session still legitimately takes the channel over, but an older connection whose
prompt is processed late cannot clobber it, and the same connection starting its next turn still
replaces its own sink. remove_reply_sink_if_owner removes only if that turn_id still owns the
entry. Reads were already turn-fenced; this closes the write side.

The tests exercise the registry helpers directly with synthesised sinks rather than driving two live
sockets: an A/B generation sequence asserting neither connection clobbers the other, a same-connection
next-turn case, and a late-cleanup case. All three are mutation-verified — restoring the
unconditional remove fails on "A's completion must not remove B's sink."

F5 — routing by what was discovered, plus a shadowing mitigation

f4e4c11, then a7f3600.

The finding was right and the generic multi-server claim did not hold. tools() advertised names
exactly as published while call() derived the target from the text before the first ., so a
generic server publishing a bare build, or one publishing a name whose first segment is not its
own, was discoverable but not callable.

Routing no longer parses the tool string. server_publishing looks the target up in the discovery
cache — the server whose fetched tools/list actually contained that tool. split_prefix survives
only as a pre-discovery fallback, so a cold call to a prefixed name reports "not connected" rather
than "not available". Tests cover the unprefixed generic server and the mismatched-prefix case.

One caveat, raised in our own review rather than in yours: content routing plus no allowlist plus
keyless loopback means a second local server whose declared name sorts earlier could shadow a
legitimate tool by publishing the same literal name. a7f3600 adds a tiebreak — when the tool is
<prefix>.<...> and one publisher is <prefix>, prefer it — restoring the prefix's authority
without reinstating the old hard <server>.<tool> requirement. Mutation-verified: dropping the
tiebreak routes to the impostor and fails the test. A genuinely bare name has no prefix to appeal to,
so it still falls to the deterministic lexicographic minimum, which is where round 5's F1 picks up.

F6 — both identifiers redacted in the hung-eviction warning

e3e06b6.

The warning logged thread_id and session_id raw, and both are credentials: the pool key carries
acp_<uuid> and the session id is sess_<uuid>, either of which resumes the session. Round 1
redacted the sites it enumerated and this one sat outside that list — the trap of treating an
enumerated finding as the scope claim.

The warn! is extracted into warn_force_evicting_hung so a real capture-subscriber test exercises
the redaction instead of leaving it to inspection. Both fields go through redact_session_ids, which
strips the prefix and keeps the platform half greppable. The test asserts the warning fires, that no
raw uuid and no acp_/sess_ prefix reaches the output, that the # tag is present, and that
discord survives — so it fails if either field is un-redacted.

On round 5, and where F5 is incomplete

Round 5's F1 is the mirror image of the F5 work above, and we would rather name that than let the F5
entry read as a closed case. The tiebreak stops an impostor from stealing a namespaced tool. It
does nothing for two servers publishing the same bare name. The facade advertises the first as
<tool> and the second as openab-browser:<tool>, but that alias is built from
source.provider() — one constant string for the whole source, not the declaring server — and
execute_capability resolves both names against the first matching Tool in the source's list, then
dispatches source.call(ctx, tool.name, …) under the bare name. Both published names therefore
reach the same lexicographically-first publisher, and the second server's tool is
advertised-but-unreachable.

Checking that turned up something the finding does not state. tools() builds its catalog by
iterating a HashSet of server names, and sorted() is a stable sort by name, so among equal names
the surviving order is the set's iteration order — nondeterministic per process. Execution meanwhile
resolves deterministically to the minimum. The schema shown for a colliding bare name can therefore
belong to one server while the call is validated against it and dispatched to another.

The other three are accepted as stated and confirmed against the current head.

• F4 (redaction inputs disagree). Already on our follow-up list, and your #12b9377c versus
  #850414fa is the whole problem in one line: redact_session_ids and the gateway's redact_id
  strip the prefix before hashing, while the three redact_channel copies hash the full
  acp_<uuid>. What makes it more than an inconsistency is that each of those three copies pins a
  cross-crate test vector asserting its tag is the shared one, which is how two conventions each
  came to claim canonical status. It has already cost us once, reading a disjoint set of channel tags
  across two logs as evidence of a routing bug when it was purely the two hash inputs disagreeing.
  Standardising on the uuid-only input, which redact_id's own comment argues for, is the fix.
• F2 (a same-name reconnect never refreshes its cached catalog). Confirmed, and it is currently
  asserted as intended: the cache is keyed (channel_id, declared_name) precisely so a
  freshly-minted id does not orphan the entry, and a test asserts the discovered set survives the id
  change. Discovery only spawns when the name has no cached entry, so a reconnected server with a
  changed tool set is served from the stale catalog. The fix is to refresh on a changed tunnel id
  while keeping the no-shrink property — which needs no list_changed, and so is compatible with the
  F2 decision above; cache-expiry-and-refetch is the direction the spec now prefers.
• F3 (180s versus 170s). Confirmed. The contract table documents the tunnel_timeout_seconds
  default as 180s while default_tunnel_timeout_seconds() returns 170. 180 is the ceiling — the
  agent-side ACP_PROMPT_IDLE_TIMEOUT_SECS — and the 170 default exists to keep a margin under it,
  so the table is documenting the wrong number.

These four are being worked next.

[brettchien]

Round 5 — all four findings fixed, head 97e8eee

Round 5 read 3d74995, which was already two commits behind when the review landed. Three commits
have gone in since, covering all four findings. All four were confirmed against the current head
before anything was changed, and F1 turned out to be worse than stated in one respect, described
below.

Verification, stated precisely: every required check is green on 97e8eee — cargo check, both
clippy passes with -D warnings (default features and unified), every test step including
cargo test (acp root) where the new tests live, plus every smoke variant (packaged and unified)
and validate-packaged-pins. Unlike the previous round these guards were not mutation-verified,
so read them as passing rather than as proven load-bearing.

F1 — one catalog now produces both the advertised names and the routes

1d1fa2b.

Confirmed as described. tools() returned a flat Vec<Tool> with no server attribution, so two
servers publishing one name produced two entries under it; the facade's alias is built from
source.provider(), a single constant string for the whole source, which cannot tell two of its own
servers apart; and execute_capability resolved both names to the first matching Tool and
dispatched under the bare name. Both published names reached the lexicographically-first publisher and
the other server's tool was advertised but unreachable.

One thing the finding does not state, which we found while checking it. tools() built its catalog by
iterating a HashSet of server names and sorted() is a stable sort, so among equal names the
surviving order was the set's iteration order — nondeterministic per process — while execution
resolved deterministically to the minimum. The schema shown for a colliding name could therefore
belong to one server while the call was validated against it and dispatched to another.

Of the two options offered we took the second: identity is retained rather than collisions
deduplicated, because dropping a tool is a worse answer than naming it apart. catalog() pairs every
advertised name with the (declared_server, published_tool) that produced it, and both tools() and
call() go through it — so a name that was advertised is callable, and reaches the server whose schema
was shown for it. It is deterministic by construction: the discovered map is a BTreeMap, each
server's tools are sorted, and the keeper of a colliding name is chosen by rule. keeper() prefers the
prefix's namesake — promoting the D-34 shadowing mitigation from a routing tiebreak to a naming rule —
and otherwise the lexicographically-first publisher; every other publisher is advertised as
<declared_server>.<published_tool>, routed to its own tunnel and forwarded under the name it
published. Keepers claim their names before any rename is assigned, so a rename can never take a name
a server actually published, with a numeric suffix as the last resort for a server that publishes both
x and its own <server>.x.

The facade's <provider>:<tool> alias is deliberately untouched. It resolves source-versus-mcp.json
collisions, which is the collision it was designed for and where the provider string is the right
discriminator; it was never able to separate two servers inside one source, and now it does not have
to, because the source no longer emits duplicate names.

Tests: two servers publishing one bare name are both advertised, both callable, and reach different
tunnels each under its own published name. The tunnel double now stamps its server's name into the
advertised schema, so the test also pins whose schema was shown for the shared name — the half of this
defect that is invisible if you only check where the call landed. Plus a rename stepping aside from a
literal <server>.<tool> a server really published, and the namesake keeping its name with the
impostor still reachable under its own.

F2 — a reconnect refreshes the catalog it inherited

Same commit, 1d1fa2b, because it is the same construct: the catalog above is built from the cache
entries this changes.

Confirmed. Surviving a reconnect is exactly why the cache is keyed by declared name, but nothing
compared the surviving entry against the connection now attached, so a reconnected server served its
predecessor's catalog for the rest of the session. Each entry now records the server_id its
tools/list came from, and tools() refetches when that differs from the id resolved now — one rule
covering both the cold start and the reconnect.

The two constraints you asked for are kept. The inherited set continues to be served until the
refetch lands, so a refresh never shrinks the catalog; and only a successful fetch writes, so a
failed refresh retains the last good catalog — the same insert path an existing test already pins for
a failed first discovery. There is also a case worth naming: a fetch that started before the
reconnect can land after it and write the superseded set, but it writes under the old id, which the
next round sees as a mismatch and refetches. The staleness is self-correcting rather than sticky.

On "assert the new id is used": the reconnect test asserts the new set replaces the inherited one and
that exactly one refetch happened, which proves the new id indirectly but soundly — the double
resolves a declared name from the server_id it is handed, and the old id is no longer registered
after the reattach, so a fetch on the old id fails and writes nothing. The assertion that the new set
appeared can only pass if the refetch went to the new id. A converse test pins the other half: an
unchanged connection is not refetched on every discovery round.

This is cache-expiry-and-refetch, which is also the mechanism the 2026-07-28 spec prefers over push
invalidation — so it is the consistent counterpart to declining inbound tools/list_changed in the
previous round rather than a contradiction of it.

F3 — the documented default was the only broken half

97e8eee.

The contract's limits table now states 170s and says what 180s actually is: the agent-side
ACP_PROMPT_IDLE_TIMEOUT_SECS, the ceiling the default keeps a margin under so a hung tunnel call
fails as a tunnel timeout — which sends mcp/cancel — rather than as a prompt-idle timeout, which
just ends the turn and strands the work on the extension.

On the second half of the request, the shared regression assertion already exists:
the_default_tunnel_timeout_stays_beneath_the_idle_timeout in src/main.rs asserts
default_tunnel_timeout_seconds() < ACP_PROMPT_IDLE_TIMEOUT_SECS and that the shipped default is not
a value the startup warning fires on. It lives in the binary because that is the only place both are
visible — the gateway owns the ceiling and does not depend on openab-core, and openab-core owns the
default and cannot see the constant. So the invariant was already guarded in code; only the published
number was wrong, which is the more dangerous of the two failures for an implementer following the
contract.

F4 — one tag per session, and the reason two conventions survived

1ebe108.

All three redact_channel copies now hash the uuid, so every redactor in the tree agrees on
#12b9377c. They also accept the sess_ form, which previously fell through the
starts_with("acp_") guard and reached the log unredacted — a resume credential in cleartext for any
call site that passed a session id, which is worth separating from the correlation problem.

What made this durable rather than a slip is the test shape: each of the three copies pinned its own
cross-crate vector asserting that its tag was the shared one, so both conventions could claim
canonical status and a divergence still passed three green tests. Where a crate has a second redactor
of its own, its test now compares against that instead of a copied literal — openab-core against
redact_session_ids, openab-gateway against redact_id (made pub(crate) for it, under the acp
feature that compiles it). openab-mcp has nothing to compare against, so the literal stays there and
the comment says why.

The copies themselves remain, and that is the residual worth stating plainly: these three crates
deliberately do not depend on one another, so delegating to one helper needs either a new dependency
edge or a small shared crate. Standing up a crate for six lines mid-PR trades a reviewable diff for an
architectural change, so it is a follow-up. The behaviour is now identical and cross-checked; the
duplication is not.

The four inline threads GitHub still shows as unresolved

All four are from 2026-07-27 and none of them was ever answered in the thread, which is our omission
— from the outside the PR reads as having four open conversations. Their actual state, so the record
is not "ignored":

Gate tunnel creation on resume success — fixed. The resume path reaches spawn_acp_tunnels only
through resumed_channel, which the handler sets only on a resume it accepted. Deriving the channel
from the requested sessionId was considered and rejected as the guard: a well-formed id derives fine
on every rejection path, so busy, over-cap and unknown-session would all still have opened a tunnel.
Because same-name re-attach is last-write-wins, that would have let a refused request evict the very
tunnel it was refused in favour of. Regression test
a_rejected_resume_yields_no_channel_to_open_tunnels_with, plus
resume_while_busy_is_rejected_and_preserves_state.

Bound the 8 MiB frame increase — addressed by the second of the two options, and the limits of that
are worth stating. MAX_NON_TUNNEL_FRAME_BYTES (1 MiB) now holds every method-bearing frame, which
is every client request and notification and therefore every path prompt text arrives on; the 8 MiB
allowance is reachable only by client responses — id present, no method — which is what tunnel
results are. MAX_INFLIGHT_PROMPTS × 8 MiB of retained prompt text is no longer a reachable shape.
Two paired tests, because the two ceilings have deliberately different outcomes and one test cannot
show that: over 8 MiB the frame cannot be parsed at all, so a request cannot be told from a
notification and no id can be recovered, and the connection closes rather than risk answering a
notification (a_frame_over_the_transport_ceiling_closes_the_connection); a method-bearing frame over
1 MiB is answered ACP_OVERLOADED and the connection survives
(a_method_frame_over_its_ceiling_is_refused_but_keeps_the_connection). What was not done: there
is still no per-connection byte budget, and the outbound channel is still
mpsc::unbounded_channel. The exposure is reduced by 8× and given a tested boundary, not eliminated.
One correction of record: this round's validation note describes this thread as "historical context",
but it is the one thread of the four that is not outdated, and it is the one that did receive a code
change.

Do not continue with an unusable facade session — fixed. setup_facade_session returns None
when write_facade_mcp_config fails, so no OPENAB_SESSION_TOKEN is minted and there is no live
credential valid until eviction for a session that could never present it; the failure logs at error
level saying the session is starting without browser capabilities. Of the options you offered this is
"report a degraded session" rather than propagate: a session with no path to the facade is still a
working agent session for everything else, and failing the whole session would be a larger outage than
the capability that is missing. Test: no_token_is_minted_when_the_facade_config_write_fails.

Remove or reject stale direct transport entries — declined for this PR, deliberately, which is why
this thread should stay open rather than be resolved. write_facade_mcp_config preserves every
existing entry byte-for-byte, including a stale openab-browser entry and a Kiro @openab-browser
grant, and a test asserts that preservation byte-for-byte so nobody "fixes" this by clobbering user
config. The hazard you describe is real and is stated in the PR body's first Follow-up in stronger
terms than the review used: an agent that has run in both modes exposes the same tools twice, once
through the facade with policy and audit and once straight through the old transport with neither, and
the direct path works perfectly while leaving no audit trail at all. Our judgement is that the durable
fix is retiring the second transport, not teaching this writer to delete other transports' entries —
that is the change that removes the bypass instead of racing it. Until then it is an operational
warning in browser-mcp-agent-setup.md and operator-performed cleanup. If you would rather have
cleanup-on-mode-switch inside this PR, say so and it goes in; we did not want to add config-deleting
behaviour to a PR whose contract does not mention it.

On the review loop itself

The review-limit-reached label was applied at 07:14 today, before the current head existed, and it
gates both the scheduled path and /review — so unless a maintainer removes it, round 5 is the last
automated round and this reply has no reviewer left to answer it. Worth knowing how the threshold was
reached: the breaker counts OpenAB PR Review statuses in pending state across the PR's commits,
and this PR has 30 pending, 5 failure, 1 error and zero success. The pending ones are dispatches
that never completed — the review agent hit an upstream weekly limit and left its status behind — so
the number stands for abandoned dispatches, not for 30 review cycles against 30 heads. Five
consolidated reviews have actually been published. The error status also sits on c9d5890, not on the current head, which
is why the head's checks are clean. We are not asking for the label to be lifted as a favour; it is a
counter that cannot distinguish a stalled dispatch from a completed round, and reconciling stale
pending statuses looks like a real bug in the loop, worth its own issue whichever way this PR goes.

Two notes on the record

The round-4 F5 residue is closed by F1's naming rule. The tiebreak shipped then stopped an impostor
taking a namespaced name; what it could not do was give the losing publisher of a bare name anywhere
to live, which is what <declared_server>.<published_tool> now does.

And one divergence we are deliberately not editing away. The PR description's acceptance criterion
"routed by prefix, with no name collision" is now stale in both halves — routing has not been by
prefix since F5, and collisions exist and are resolved by naming rather than prevented. The body is
being left as written rather than rewritten, so that what was claimed at each point stays legible;
treat this paragraph as the correction of record. If you would rather the contract itself carry it, say
so and we will append a dated revision beneath the criterion rather than overwrite the line.

On "a browser-specific first version could be smaller"

The first of the three reasons deserves a straight answer, because F1 is evidence for it. The
collision rules are a cost of the generic model: a browser-only source with one declared server would
not need keeper(), renames, or a deterministic catalog at all. F2 is not that kind of cost, though —
a single browser server that reconnects has exactly the same stale-catalog bug, so cache invalidation
is owed by the narrow version too. And the gateway is keyed by (channel_id, server_id) regardless,
because the client declares its servers in session/new; a single-server source would be a narrower
facade over the same tunnel registry rather than a smaller design. The honest summary is that reason 1
buys back the naming rules and nothing else, which is why the naming rules are now stated in the ADR
rather than left implicit.

Where this leaves the decision

Per the Stopping Rule this is past the default three-stage cap, so the call is a maintainer's:
authorize another focused round, revise or split the contract, or merge on the frozen criteria with the
Follow-ups as written. What is decided and stated rather than pending: the stale-transport cleanup
(Follow-up 1), the three redaction copies (F4 above), the missing per-connection byte budget (inline
thread 2), and the four Accepted Residual Risks in the body. Everything else raised in five rounds is
either fixed with a test or answered with a reason.