fix: create PR for chart bump instead of direct push - #8
Merged
Conversation
pahud
approved these changes
Apr 3, 2026
henrieopenclaw
added a commit
to henrieopenclaw/agent-broker
that referenced
this pull request
Apr 6, 2026
Co-authored-by: thepagent <thepagent@users.noreply.github.com>
Reese-max
pushed a commit
to Reese-max/openab
that referenced
this pull request
Apr 12, 2026
Co-authored-by: thepagent <thepagent@users.noreply.github.com>
This was referenced May 6, 2026
chaodu-agent
pushed a commit
that referenced
this pull request
Jun 24, 2026
- Add child.wait() arm to URL-collection select! so a fast-failing auth command reports immediately instead of stalling the full 30s window (#1) - Reject bot users in /auth, consistent with /remind (#8) - Record invoking user_id in the auth start audit log (#7) - Truncate output by UTF-16 code units to match Discord's 2000-char limit, preventing rejection on non-BMP-heavy output (#5) - Handle std::sync::Mutex poison in drain/collect paths to avoid panic cascade and silent output loss (#9) - Clarify the 'no output' error message with cause and remedy (#6) - Fix docs intro contradicting /auth DM-only and mark it DM-only in the command table (#3)
brettchien
added a commit
to brettchien/openab
that referenced
this pull request
Jun 24, 2026
The §9 Q4 tail referenced 'openab-agent-mcp.md open items openabdev#1 (reqwest 0.12/0.13 split) and openabdev#8 (doctor/runtime two-store split)' — but that ADR's §10 Open Questions has only two items (mcp.json location; native-vs-broker parity), neither matching, and no such numbered items / terms exist anywhere in it. The phantom reference dated to the original draft. Replace with an accurate statement: McpCredentialStore reuses the same TokenStore/auth.json storage (openab-agent-mcp.md §6.1), so the lock lands once and serves both; the reqwest version conflict is the rmcp-OAuth dependency issue surfaced on the feat/openab-agent-mcp-resilience PR, not an mcp-ADR open item. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepagent
added a commit
that referenced
this pull request
Jun 27, 2026
…g (codex + MCP) (#1190) * docs(adr): openab-agent multi-vendor OAuth & credential storage Proposed ADR for the openab-agent LLM-provider OAuth revamp: a two-axis OAuthVendor adapter (auth flow vs inference transport), a cross-process flock-guarded credential-store invariant for auth.json, the CLAUDE_CODE_OAUTH_TOKEN env route, a 14-variant vendor feasibility matrix, and the /auth (PR #1185) auth-trigger model. Surfaced while reviewing PR #1187 (first OAuth vendor). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): address review — prefer oauth2 crate, drop rollout, vendor names - Build the OAuthVendor driver on the official `oauth2` crate (already in-tree via the MCP side) instead of a hand-rolled PKCE/exchange/refresh flow; the Anthropic JSON-token-body quirk is applied via the crate's custom http-client hook. Reframe §8 accordingly (hand-rolled flow is the rejected alternative). - Remove the project Rollout-plan section (internal sequencing, not ADR material); keep the race-window mitigation in §5.4. - Use vendor names only; drop internal fleet-agent references from the matrix. - Replace unexplained "ToS-gray" with "ToS-risk" + a definition. - Fix cross-references (crate-qualified paths, §-refs, line numbers) and move the settled model-default decision under "Decisions & open questions". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): per-tenant refresh lock + per-user PKCE keying (Mira review r1) Fold in two release-blocker-class concurrency fixes from Mira's review: - §5.4 refresh-token rotation: the prior "refresh outside lock, re-read on commit" claim was wrong — N processes each send a refresh with the same RT_old before committing, tripping OAuth 2.1 §10.4 reuse -> token-family revocation. Replace with a per-tenant exclusive lock: network refresh held under the tenant lock only (not the global lock), so exactly one refresh per tenant per expiry with no head-of-line blocking across tenants. Extends to mcp:<server> tenants. - §7 /auth: key the pending PKCE verifier+state by the initiating Discord user id instead of a single global entry — prevents concurrent-user overwrite (PKCE mismatch) and session hijack. - §5.1 OAuthVendor::redirect() -> Option, since device flows have no redirect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): bundled-secret storage + pending-entry GC (Mira review r2) - §6/§9 Q2: correct the stale "git-safe" claim on the gemini GOCSPX- secret. The value is non-confidential by RFC 8252 / Google docs, but GitHub now push-protects Google secrets by default (changelog 2026-03) and partner-scans them for auto-revoke, so a raw literal is not safe in a public repo. Decide: encode-at-rest (scanner-evasion for a non-secret, NOT a security control) or env-inject at runtime — not raw text. - §7: pending PKCE entries get created_at + a 15-min GC sweep in with_auth_locked so abandoned /auth attempts don't accumulate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): close §9 Q2/Q3 — env-injection default, vendor go/no-go (Brett decisions r3) - §9 Q2 DECIDED: env-injection (b) is the default for the gemini/agy bundled client_secret; encode-at-rest (a) is the fallback for bundled zero-config binaries only, framed as scanner-evasion (not security). Cite rclone rcloneEncryptedClientSecret + obscure.MustReveal() as the canonical precedent (§10). - §9 Q3 DECIDED: GO gemini/grok (first wave) + agy (experimental, opt-in, ToS caveat — shares gemini's Code-Assist provider, residual risk is ToS not secret storage); No-Go cursor/kiro. Mirror as a build-decision line under §6. - §10: add rclone + GitHub/Google secret-scanning references. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): confirm agy secret + ecosystem evidence (GitHub survey r4) GitHub survey 2026-06-24 grounds the agy GO decision: - §6/§9 Q2: agy client_secret requirement CONFIRMED — it needs a GOCSPX- secret, a public constant >=20 antigravity-auth repos hardcode verbatim (NoeFabris/opencode-antigravity-auth, router-for-me/CLIProxyAPI, ...). Literal deliberately NOT pasted into the doc — would trip the very §9 Q2 push-protection, so we dogfood the env/encode decision. Redirect confirmed localhost:51121/oauth-callback. - §9 Q3: ecosystem evidence — agy OAuth is widely ported (opencode/pi/hermes/ openclaw plugins + proxies), proving the integration, while the same ecosystem's anti-ban / quota-locking / multi-account-rotation tooling empirically confirms the ToS-ban + 429 risks, reinforcing the opt-in gate. - §10: add the antigravity OAuth ecosystem reference set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): agy-vendor vs agy-CLI/ACP clarification + MCP store revamp in scope (r5) Grounded by a codebase survey (2026-06-24): - §4/§6: clarify "agy as a GO vendor" means a native OAuthVendor + Code-Assist inference provider — it does NOT run the agy CLI. agy speaks no ACP; the existing `antigravity` runtime variant (Mira/ECS) only works via a dedicated agy-acp adapter that shells out to the agy binary per prompt and polls its SQLite DB. The provider path sidesteps ACP entirely and supersedes the CLI-wrapper for native use, so agy's lack of ACP doesn't block the GO. - §5.4/§9 Q4: make the MCP CredentialStore revamp explicitly in-scope. auth.json has NO lock today (only atomic rename); provider save_tokens (auth.rs:234) and McpCredentialStore::save/clear (auth.rs:284-328) are two independent unlocked RMW callers, so with_auth_locked must wrap BOTH or the race persists. Also correct the stale save_tokens_for name and note with_auth_locked is new. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): fix stale §9 Q4 cross-reference The §9 Q4 tail referenced 'openab-agent-mcp.md open items #1 (reqwest 0.12/0.13 split) and #8 (doctor/runtime two-store split)' — but that ADR's §10 Open Questions has only two items (mcp.json location; native-vs-broker parity), neither matching, and no such numbered items / terms exist anywhere in it. The phantom reference dated to the original draft. Replace with an accurate statement: McpCredentialStore reuses the same TokenStore/auth.json storage (openab-agent-mcp.md §6.1), so the lock lands once and serves both; the reqwest version conflict is the rmcp-OAuth dependency issue surfaced on the feat/openab-agent-mcp-resilience PR, not an mcp-ADR open item. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): §9 Q2 — encode-at-rest is the default, env is the alternative (Brett) Brett's call given the 93%-plaintext ecosystem survey: keep the bundled zero-config UX via encode-at-rest (obscure, rclone obscure.MustReveal style) as the DEFAULT, with env-injection as the alternative for fleet/pod. Reverses the prior (b)-default ordering. Framing unchanged: encode-at-rest is scanner-evasion for a non-confidential value, not a security control. Record the survey numbers (99/107 plaintext; auto-revoke largely unrealized) and that the real risk mitigated is org-repo push-protection friction, not credential loss. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(auth): cross-process locking for auth.json — codex + MCP (ADR §5.4) Implements the ADR §5.4 invariant. auth.json had NO lock: provider save_tokens and McpCredentialStore::save/clear each did an independent unlocked read-modify-write, so a concurrent codex refresh and MCP save last-writer-wins the whole map, and N processes (one openab-agent per Discord thread) could each refresh the codex token with the same RT_old → OAuth 2.1 §10.4 token-family revocation (fleet-wide logout). Two locks, flock(2) on sidecar files (kernel auto-releases on death), cfg(unix) with a non-unix no-op: - with_auth_locked: global exclusive lock across the re-read -> mutate -> atomic-write. ALL writers funnel through it — save_tokens (codex) and McpCredentialStore::save/clear (MCP) — so writers merge onto the latest on-disk state instead of lost-updating. Held only for the fast file RMW, never across network I/O. - lock_tenant_refresh: per-tenant refresh serialisation. get_valid_token / force_refresh take the codex tenant lock (non-blocking try + async backoff + 10s timeout, held across the network refresh), with a double-checked re-read so a process that waited adopts the token another already refreshed → exactly one real refresh per tenant per expiry, no RT_old reuse. Uses libc::flock (already a cfg(unix) dep); rustix is NOT in-tree (ADR text was optimistic). New test asserts the locked RMW merges codex + MCP tenants without lost-update. fmt + clippy -D warnings + test (191 passed) green. Known gap (for review): the MCP *network* refresh is owned by rmcp's AuthorizationManager (it refreshes then calls CredentialStore::save), so MCP writes get the file-integrity lock but MCP refreshes are not yet tenant- serialised. Closing that needs an rmcp-level hook — proposed as follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(auth): address Mira code review — pending GC + portable WouldBlock Round-2 code review (Mira): - §7 pending-entry GC: add created_at to PendingPasteLogin and sweep AuthEntry::Pending older than 15 min inside with_auth_locked on every write, so abandoned /auth two-step attempts don't accumulate. PendingPasteLogin is currently a legacy tombstone with no live writer (PKCE state lives in rmcp's in-memory StateStore); the field + GC land now per ADR §7 and are forward-compatible with the forthcoming /auth two-step flow, and meanwhile sweep legacy stray entries (created_at default 0 reads as ancient). New test covers stale-swept / fresh-kept / real-tenant-untouched. - flock_try_exclusive: match std::io::ErrorKind::WouldBlock instead of a single raw EWOULDBLOCK errno, covering EAGAIN/EWOULDBLOCK across libc/BSD. - MCP network-refresh serialisation stays a follow-up (rmcp owns the refresh) — Mira concurs. fmt + clippy -D warnings + test (192 passed) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(auth): serialise MCP token refresh cross-process (close §5.4 (b) gap) Closes the known gap from f370110: MCP refreshes now get the same per-tenant serialisation as codex. rmcp's CredentialStore exposes no pre-refresh hook, but openab drives the MCP refresh explicitly in McpRuntimeManager::resolve_oauth_dial via client.get_access_token(). Wrap that call (per-server) with the existing auth::lock_tenant_refresh so only one process refreshes a given server at a time. No explicit double-check needed: rmcp's get_access_token re-load()s auth.json each call and returns the cached token without a network refresh when remaining >= REFRESH_BUFFER (rmcp auth.rs:1238), so a process that loses the race adopts the token the winner wrote to the shared file — no second RT_old presentation, no OAuth 2.1 §10.4 family revocation. rmcp already single-flights within one process via its AuthorizationManager Mutex; this closes the cross-process gap. - auth.rs: lock_tenant_refresh + AuthFileLock made pub(crate) for the mcp module. - ADR §5.4: document the resolve_oauth_dial serialisation point. fmt + clippy -D warnings + test (192 passed) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(auth): /simplify cleanups — dedup lock acquire, reuse refresh fd Quality-only cleanups from a 4-angle /simplify pass (no behaviour change): - Extract lock_global(path) -> Result<Option<AuthFileLock>> and route both with_auth_locked and McpCredentialStore::clear through it, so the "global" sidecar name + the cfg(unix) acquire live in one place instead of two copy-pasted blocks. clear flattens (drop the inner run closure). - lock_tenant_refresh opens the lock fd once and re-issues flock on it each retry, instead of re-opening (and re-create_dir_all-ing) the file every 100ms under contention. Removes the now-unused flock_try_exclusive helper. - Document lock_tenant_refresh's double-check contract (the lock only serialises; callers must re-check freshness after acquiring). fmt + clippy -D warnings + test (192 passed) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(auth): /code-review — harden GC + refresh-lock contract comments Doc-only clarifications from a /code-review pass (no behaviour change): - PendingPasteLogin.created_at: warn loudly that any writer of a fresh Pending entry MUST stamp created_at, else gc_stale_pending sweeps it on the next locked write (latent footgun for the forthcoming /auth two-step writer). - lock_tenant_refresh: correct the contract — reuse-safety comes from loading the refresh token INSIDE the lock (so force_refresh, which always refreshes on a 401, is reuse-safe too); the post-lock expiry re-check is only an optimisation to skip a redundant refresh. Review also surfaced a pre-existing, out-of-scope namespacing gap (an MCP server literally named "codex" collides with the codex tenant's auth.json key and refresh lock — committed MCP creds use the bare server name, not mcp:<server> as ADR §5.4 describes) — flagged for a separate change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * build(openab-agent): declare standalone [workspace] to fix CI ci-openab-agent.yml runs cargo fmt/clippy/test/build with working-directory: openab-agent, but the crate is not a member of the parent openab workspace (members = crates/openab-core, openab-gateway) and was not excluded, so cargo errors 'current package believes it's in a workspace when it's not' and every step fails before doing any work. openab-agent is intentionally standalone (own version + dual reqwest 0.12/0.13 for rmcp), so the correct fix is an empty [workspace] table making it its own root. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revert "build(openab-agent): declare standalone [workspace] to fix CI" This reverts commit b40693c. * ci(openab-agent): append [workspace] in CI + harden ACP smoke test ci-openab-agent.yml ran cargo from working-directory: openab-agent without the [workspace] table the crate needs (it's standalone, not a parent-workspace member), so every step failed with 'believes it's in a workspace when it's not' — the workflow had been red independently of any PR. Dockerfile.unified already works around this by appending the table at build time; replicate that in the workflow rather than committing it to Cargo.toml (which would double-append in the Dockerfile and break the image build). Also harden the ACP smoke test: build the release binary in its own step and bump the response timeout 5s -> 30s so a loaded runner doesn't flake the agent's first-response window (the binary itself responds in <1s locally, verified incl. a clean-HOME release build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(openab-agent): surface ACP smoke-test stderr + exit code for diagnosis The smoke test produces empty stdout only in CI (the binary responds with agentInfo in <1s locally across debug/release/clean-HOME/CI-env-var runs). Capture stderr + exit code so the next CI run reveals why stdout is empty. * openab-agent: address review findings on OAuth ADR + auth.json locking Resolves the Changes-Requested review on the multi-vendor OAuth ADR + cross-process auth.json locking PR. No behavioural change beyond log level and a non-unix diagnostic. ADR (docs/adr/openab-agent-oauth.md) - Fix lock-file names to match code: auth.json.global.lock and auth.json.refresh.<tenant>.lock (were auth.json.lock / auth.json.<tenant>.lock). - Add the path parameter to the with_auth_locked pseudocode signature. - Correct the MCP refresh note: initialize_from_store() does the disk reload, not get_access_token. - Correct the crate note: libc::flock directly (rustix is not in-tree). - Document the deliberate fail-open trade-off on tenant-lock timeout. - Mark the default-model removal (Decision 1) as a follow-up; this PR ships the ADR + locking only. Code - auth.rs: escalate the tenant-lock timeout log from warn! to error! and document the fail-open trade-off on lock_tenant_refresh. - auth.rs: non-unix lock_global no-op now warns once instead of silently providing zero cross-process protection. - mcp/runtime.rs: fix the rmcp reload comment and document the cross-module invariant that the refresh lock and credential entry share the server name. CI (ci-openab-agent.yml) - ACP smoke test now fails on a non-zero/timeout exit code, and the agentInfo assertions use { } so exit fails the step rather than just a subshell. Gate: cargo fmt --check, clippy -D warnings, test (192 passed) all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * openab-agent: fail closed on a contended refresh lock (F6) Replaces the fail-open tenant-lock timeout with a bounded-refresh + fail-closed design, so a contended lock can no longer reintroduce the double-refresh it exists to prevent. - Bound the refresh network call with an explicit 8s HTTP timeout on both the codex client (auth.rs) and the MCP AuthClient (mcp/runtime.rs), strictly shorter than the 10s lock-acquire deadline. With flock(2) auto-release on death, a live holder always frees the tenant lock before a waiter's deadline, so a lock-acquire timeout is genuinely abnormal. - lock_tenant_refresh now returns RefreshLock { Held, Unavailable, TimedOut }. On TimedOut callers fail closed instead of refreshing unserialised: - codex get_valid_token / force_refresh return a retryable error; - MCP resolve_oauth_dial returns a new OauthDialError::Transient that leaves the server retryable WITHOUT forcing re-login (NeedsAuth) or tripping the circuit breaker (auth-level failures still map to NeedsAuth as before). A filesystem error opening the sidecar returns Unavailable and degrades to a best-effort unserialised refresh rather than blocking every refresh. - Add a fail-closed timeout test (injectable deadline) and update ADR §5.4 to document the bounded-refresh + fail-closed invariant, superseding the earlier fail-open note. Gate: cargo fmt --check, clippy -D warnings, test (193 passed) all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * openab-agent: size lock timeout above worst-case multi-call refresh hold Mira review (PR #1190) found the fail-closed invariant held only for the codex path. The MCP path holds the tenant lock across TWO sequential bounded calls — rmcp's initialize_from_store() (AS discovery) then get_access_token() (refresh) — so the worst-case lock-hold is ~2 x REFRESH_HTTP_TIMEOUT (16s), which could exceed the fixed 10s lock deadline and fail a waiter closed while the holder is still legitimately progressing. Derive REFRESH_LOCK_TIMEOUT from the bound instead of hardcoding it: REFRESH_LOCK_TIMEOUT = MAX_REFRESH_ROUND_TRIPS (2) * REFRESH_HTTP_TIMEOUT + 4s = 20s so the deadline is always above the worst-case hold and only a genuinely stuck holder trips it. Normal-case latency is unchanged (the waiter polls every 100ms and acquires as soon as the holder releases). Update the fn doc and ADR §5.4 to state the multi-call hold explicitly. Gate: cargo fmt --check, clippy -D warnings, test (193 passed) all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: thepagent <hehsieh1010@gmail.com>
thepagent
added a commit
that referenced
this pull request
Jun 29, 2026
…-agent (#1187) * feat(native-agent): add Anthropic OAuth (Claude Pro/Max) login + provider Phase 1 of porting Pi's auth methods into openab-agent. Adds an `anthropic-oauth` tenant alongside Codex: PKCE browser/paste login against platform.claude.com, JSON token exchange + scope-less refresh, and an OAuth mode on AnthropicProvider (Bearer + Claude Code identity headers/system block, tool-name normalisation). Wires provider selection in acp.rs/llm.rs and a new `auth anthropic-oauth` CLI subcommand. Verified: cargo build clean (0 warnings), 194 tests pass incl. 4 new. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(native-agent): default Anthropic model to claude-opus-4-8 The fallback default was claude-sonnet-4-20250514 (Sonnet 4.0, ~13mo old), which 404s on Claude Pro/Max OAuth subscriptions. Bump the three default-model fallbacks to the current claude-opus-4-8 (verified live via OAuth). The model catalog already listed it; only the fallback was stale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(native-agent): address PR review (CI workspace, PKCE state, error UX) - F1 (blocker): root Cargo.toml `exclude = ["openab-agent"]` so `cd openab-agent && cargo fmt/clippy/test` resolves standalone. The workspace restructure left openab-agent neither a member nor excluded; CI openab-agent only runs on openab-agent/** so it was dormant on main — this PR is the first change to trigger it. Also ran `cargo fmt`. - F2: use an independent 32-byte random PKCE `state` instead of reusing the verifier, keeping the verifier back-channel-only (claude.ai rejects a short state as "Invalid request format"; 32 bytes matches the verifier length). Verified end-to-end with a real Pro/Max login + chat. - F3: credential-error messages now name fully-qualified subcommands (`openab-agent auth anthropic-oauth` / `... codex-oauth`) and preserve the underlying read/parse error. - F4: drop the `ponytail:` placeholder tag from a comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(native-agent): flush stdout drain on ACP server shutdown The dispatch loop fed responses to a detached stdout-drain task; on stdin EOF the loop ended and `#[tokio::main]` aborted the drain before it flushed the last queued line, so a one-shot `initialize` could return nothing. This was a latent race (main wins it by timing); this branch's slightly different startup timing made the binary lose it ~85% locally, surfacing as the red `CI openab-agent` ACP smoke test. Capture the drain handle and, after the loop, drop the senders and bounded-await the drain so queued output is flushed before return. Race test: 20/20 after (was 3/20). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address review #6/#7/#11 (auth mode, 401 refresh, error UX) Non-blocking polish from the PR re-review: - #6 (acp.rs): on ACP model switch, an OAuth-forced session was rebuilt via `auto_with_model`, which prefers ANTHROPIC_API_KEY and silently dropped the forced anthropic-oauth provider when a key was also present. Rebuild now preserves the session's auth mode via a new LlmProvider::is_oauth() (Agent::provider_is_oauth()). - #7 (llm.rs): the OAuth 401 branch swallowed force_refresh_for errors (`let _ = ...`) and retried with the stale token. Bubble the error. - #11 (auth.rs): refresh_token failure message named bare `openab-agent auth`; now names the tenant subcommand via a shared auth_subcommand() helper (also dedupes load_tokens_for). Deferred as follow-up (noted in PR): #8 --no-browser state validation, #9 save_tokens_for keying, #10 non-Unix atomic write. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h * fix: address review #8 — always verify CSRF state on bare-code paste `--no-browser` bare-code paste defaulted the pasted state to the expected value when no `#state` was present, so the `st != state` check passed trivially and CSRF state was never verified. Require the `code#state` form (or a full redirect URL) and reject a bare code with a clear message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h * fix: don't pin a hardcoded Anthropic default model (review F4 follow-up) Per @brettchien: dateless 4.6+ model IDs are fixed canonical IDs, not evergreen pointers, so a hardcoded default (claude-opus-4-8) is a per-generation 404 timebomb — the same failure that retired the previous claude-sonnet-4-20250514 default. It also silently bumped API-key users onto pricier Opus (review #5). Resolve the Anthropic model as: explicit override → OPENAB_AGENT_MODEL → error ("no model configured; set OPENAB_AGENT_MODEL or select a model"). - llm.rs: `anthropic_model()` is now fallible (no default); constructors refactored (`build`/`api_key_from_env`/`ensure_oauth_token`) so a model override never requires OPENAB_AGENT_MODEL, and credential errors still precede the model error. `auto()` only falls through to OAuth when no API key is present. - acp.rs: session new/load report the provider's resolved model instead of a hardcoded fallback. Removed the opus/gpt default sites. - Kept the claude-opus-4-8 entry in the model catalog (offering ≠ default). - docs/native-agent.md: document OPENAB_AGENT_MODEL is required for Anthropic (zero-config now fails loud). Behavior change: no zero-config default model. Deployments set it via env/values.yaml; local/zero-config users must export OPENAB_AGENT_MODEL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h * docs: fix stale PKCE state comment (review F5) The doc comment on login_anthropic_browser_flow still said the verifier doubles as `state` (Pi's old convention); since the PKCE fix the state is an independent 32-byte random value. Correct the comment to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h * feat(native-agent): accept canonical provider/model in OPENAB_AGENT_MODEL ModelRef::parse + resolve_provider_choice let OPENAB_AGENT_MODEL carry `provider/model_id` (e.g. anthropic/claude-sonnet-4-6) as a single source of truth for both provider and model. Bare model ids and the existing OPENAB_AGENT_PROVIDER var remain fully backward compatible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: satisfy clippy::manual_is_multiple_of (Rust 1.96 stable) Stable clippy 1.96 added manual_is_multiple_of; pre-existing modulo checks in openab-core (format.rs, pre_seed.rs) and openab-gateway (wecom.rs) fail `clippy --workspace -D warnings`. Mechanical fix; unblocks CI. Unrelated to the OAuth change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: limit OAuth 401 force-refresh to one retry The comment said "retry once" but `attempt < max_retries` allowed up to 3 consecutive force-refreshes on persistent 401s — wasting round-trips and risking rotating a still-valid refresh token off the server's grace window. Use an `oauth_refreshed` flag to match the documented intent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(native-agent): introduce OAuthVendor descriptor abstraction (ADR §5.1) Collapse the hand-rolled per-vendor OAuth surface into a single OAuthVendor trait + CodexVendor/AnthropicVendor descriptors. Adding a subscription-OAuth vendor is now a descriptor, not a new hand-rolled flow: - Unify the codex + anthropic PKCE login flows into one shared `login_pkce_flow` driven by the descriptor; fold the codex flow into the `accept_callback_code` / `code_from_redirect` helpers (the long-standing TODO) and unify the 127.0.0.1 bind. - Drive `refresh_token` and the authorization-code exchange off `vendor.token_body()` instead of an `if provider == ANTHROPIC_NAMESPACE` branch. - Shared pure builders `build_authorize_url` / `token_store_from_payload`, pinned by wire-format unit tests — the login authorize-URL/exchange hit live OAuth servers, so no integration test covers them. The driver keeps the proven reqwest flows; swapping the engine onto `oauth2::BasicClient` (as `mcp/runtime.rs` already does via a custom http hook) is a follow-up internal change invisible to vendor authors — only the descriptor surface lands here. `client_secret()`/`grant()` are the ADR §5.1 surface for later vendors (gemini/agy bundled secret; copilot/kiro device-code). 205 tests pass (4 new wire-format locks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(native-agent): credential precedence + CLAUDE_CODE_OAUTH_TOKEN (ADR §5.3) Add `AnthropicAuth::OAuthEnv` for the pre-provisioned long-lived subscription token route (`CLAUDE_CODE_OAUTH_TOKEN`) — the recommended fleet mode (ops mints once, injects as a k8s secret; no interactive flow, no auth.json write, no refresh race). - `AnthropicProvider::auto[_with_model]()` now resolves in ADR §5.3 precedence: `ANTHROPIC_API_KEY` → `CLAUDE_CODE_OAUTH_TOKEN` → stored `anthropic-oauth` tenant. Each source surfaces its own errors rather than falling through to a lower-precedence credential error. - `OAuthEnv` shares the OAuth `Bearer` + Claude Code identity path (extracted into `oauth_headers`); `is_oauth()` covers it so the system block + tool-name casing apply. The 401 force-refresh is gated to the stored tenant only — the env token has no tenant to refresh, so a 401 there surfaces (re-mint) instead of erroring on a missing tenant. 207 tests pass (+2 precedence tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(native-agent): ModelRef::parse only splits known provider prefixes (review F4) `provider/model` previously split on the first `/`, mis-parsing HuggingFace-style `org/model` ids (e.g. `meta-llama/Llama-3-8B`) for custom/OpenAI-compatible endpoints — `org` became the "provider" and the real id was truncated. Now the prefix is split off only when it's a `KNOWN_PROVIDERS` entry; otherwise the whole string is the model id. Load-bearing for the planned single-string `provider/model` config field. Known prefixes still split unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(native-agent): fail loud on present-but-misconfigured Anthropic creds (review F3) `select_provider`'s auto-detect arm discarded the `AnthropicProvider::auto()` error and fell through to Codex. A present Anthropic credential that failed for a config reason (e.g. a key set but no model) was silently masked — the user got a Codex provider, or a Codex-only error that hid the real cause. Now: if any Anthropic credential source exists (API key / CLAUDE_CODE_OAUTH_TOKEN / stored tenant), an `auto()` failure surfaces as a config error ("credential present but unusable"). Codex fallthrough happens only when no Anthropic credential exists at all, and that final error names both credential routes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(native-agent): centralized config.json for default model/params (ADR §5.5) Add a small JSON config file next to auth.json so a deployment can declare the default provider/model (and max_tokens) in a file instead of only via env vars — the centralized-config gap the multi-vendor work surfaced. - New `config` module: `{ model, max_tokens }` (single `provider/model` string, reusing ModelRef). Unknown keys tolerated (forward-compat for `providers`/etc.); malformed JSON fails loud; a missing file is an empty config. - Resolution is env-over-config: `anthropic_model` / `anthropic_max_tokens` / `resolve_provider_choice` now fall `OPENAB_AGENT_*` env → config.json → built-in. A pod's injected env stays authoritative over a baked config. - Secrets never live here — they stay in the locked `auth.json` store. Per-provider `base_url` routing is a deliberate follow-up; this lands the default provider/model/params surface. 213 tests pass (+5). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(native-agent): document OAuthVendor, credential precedence, config.json (review F5) - Anthropic credentials section: the ADR §5.3 precedence (ANTHROPIC_API_KEY → CLAUDE_CODE_OAUTH_TOKEN fleet route → interactive anthropic-oauth) + the `openab-agent auth anthropic-oauth` login. - New "Configuration file (config.json)" section: schema, env-over-config precedence, secrets-stay-in-auth.json. - "Adding an OAuth vendor": the OAuthVendor descriptor model (ADR §5.1). - Env table: CLAUDE_CODE_OAUTH_TOKEN, OPENAB_AGENT_ANTHROPIC_CLIENT_ID, OPENAB_CONFIG_PATH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: cargo fmt (OAuthVendor + config.json) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(native-agent): prove §5.4 lock single-flight for the anthropic-oauth tenant Brett asked to cover the new tenant the same way #1190's codex lock tests do, so single-flight / fail-closed is proven for it rather than only structurally similar: - `with_auth_locked_merges_anthropic_tenant_no_lost_update` — a concurrent codex write does not clobber a just-written `anthropic-oauth` token (the new tenant rides the same locked RMW funnel). - `lock_tenant_refresh_fails_closed_for_anthropic_and_is_per_tenant` — a held anthropic refresh lock makes a second anthropic acquire fail closed (`TimedOut`), and does NOT block codex (per-tenant isolation — the reason §5.4 uses a per-tenant lock, not the global one). 215 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(review): OAuth model-switch uses env token before auth.json F1/F2: select_provider and ACP model-switch now check CLAUDE_CODE_OAUTH_TOKEN before falling back to the stored anthropic-oauth tenant in auth.json. Fleet pods that only set the env token can switch models without an auth.json file. Adds from_oauth_auto() / from_oauth_auto_with_model() that implement the env-over-store precedence for OAuth rebuild paths. F5: config.rs logs malformed config at error level (was warn) so production monitoring catches a typo'd config.json. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Can <can@zeabur.com> Co-authored-by: thepagent <hehsieh1010@gmail.com> Co-authored-by: chaodu-agent <chaodu-agent@openab.dev>
thepagent
pushed a commit
that referenced
this pull request
Jul 13, 2026
* docs(adr): revise identity-trust-none to three-layer architecture Receiver → Trust Gate → Handler replaces the previous 'gate at handle_message()' design. Addresses all findings from the PR #1263 mob review (howie + 3 LLM reviewers). Key changes: - §4.2: Trust Gate is a dedicated ingress layer upstream of Handler - §5: New architecture diagram showing three-layer separation - §7: Implementation plan starts with Receiver/Handler split - Address #1: gate at actual convergence point (not handle_message) - Address #2: trust lookup keys off per-event platform (not adapter) - Address #3: slash commands gated (Handler is downstream of gate) - Address #4: exhaustive scattered-checks inventory - Address #5: explicit empty-vs-missing semantics - Address #6: phased rollout (Phase 0-3) - Address #7: echo rate-limit + bot exclusion + DM-preferred - Address #8: gateway vs first-class section precedence - Address #9: no static HashSet (runtime construction) - Address #10: structured logging on allow + deny - Address #11-#15: minor fixes (Teams ID, bot semantics, etc.) * docs(adr): address team review findings - Add type-level guarantee (GatedEvent vs InboundEvent) — compile-time enforcement, not just convention (#4) - Clarify Gateway Receiver is one receiver that demuxes by platform (#11) - Fix layer numbering inconsistency — use names, not numbers (#21) - Add sender ID format table with per-platform gotchas (#22, #23, #24) - Clarify is_bot bypass is caller-side, not inside decide() (擺渡-1) - Change echo group fallback to silent drop (avoid UID leakage) (#6) * docs(adr): add event loop binding design + fix is_bot L2 bypass - Add §5 'Event loop binding' section: run_platform generic pipeline, EventReceiver/EventHandler traits, main.rs startup wiring - Gateway platforms: one shared WS, demux by event.platform, fan-out to per-platform Handlers - Fix is_bot bypass: bots skip L3 but STILL enforce L2 scope (擺渡-1 🔴) - Add cross-crate boundary note for Gateway Receiver (擺渡-2 🟡) - Include binding topology summary diagram * docs(adr): address round 3 findings — tighten pseudocode precision - GatedEvent: private field in narrow module (not pub(crate)), with read-only accessors and module layout diagram (諸葛村夫-1) - gate_event: use configs.get().surface_allowed() to match real API (擺渡-3) - Phase table: add Phase 0.5 for current partially-wired state on main, clarify Phase 2 means 'refuse to start' (諸葛村夫-2) * docs(adr): fix seal() visibility — private fn, not pub(super) seal() lives in the same module as gate_event(), so it should be a plain private fn. pub(super) would unnecessarily expose it to the parent module. * docs(adr): fix stale module layout comment — constructor is private, not pub(super) * docs(adr): address LINE/Slack/Feishu review feedback - Replace line-number refs with symbol+semantic descriptions (drift-proof) - Rewrite echo section: platform-specific echo trait (LINE=Reply only, Slack=chat.postEphemeral, Discord=DM); leak-safe content by scope - Add is_bot per-platform derivation table (pinned canonical rules) - Document trusted_bot_ids as shared config (resolves Feishu circular dep) - Clarify slash commands scope (Slack doesn't consume them) - Update Slack sender ID: Enterprise Grid composite key (team_id, sender_id) - Add non-message events section (assistant_thread_started must gate) - Add Slack scope notes (Socket Mode only, MPIM=channel) - Add LINE group policy: open/members dual-mode in decide() - Add LINE @mention pre-filter as documented Receiver exception - Feishu: gateway=L1 only, eliminate double-gating, empty list=deny-all Addresses feedback from: - @luffy-aiagent (LINE platform review) - @antigenius0910 (Slack platform review) - @wangyuyan-agent (Feishu platform review) * docs(adr): address 9 review findings — API contract gaps + hardening Fixes identified during group review: F1+F6: Add workspace_id to InboundEvent; define Slack Enterprise Grid canonical sender_id format and config examples for Grid deployments F2: Replace HashMap<String, TrustConfig> with enum PlatformTrustConfig (Base/Line/Slack) — LINE group policy and Slack workspace-scoped trust now have proper type representations F3: Add cron bypass in gate_event() — system-initiated events skip L2/L3 (platform='cron' or sender_id='openab-cron') F4: Add #[cfg(test)] assume_trusted_for_test() constructor for GatedEvent — enables Handler unit testing without full pipeline F5+F9: Change into_inner() to pub(crate); adjust safety claim wording from 'bypass impossible' to 'accidental bypass compile error' F7: Change gate_event() signature to take InboundEvent by value — zero-copy hot path (no .clone() on RawPlatformEvent) F8: Specify bounded LRU cache (max_capacity + TTL) for rate-limit state — prevents OOM from random sender_id flooding * fix(adr): address review findings F1-F3 on identity-trust-none F1 (critical): Remove sender_id spoofing hole in cron bypass — only check platform == "cron" since WeCom allows freeform UserIDs that could match any synthetic value. Update rationale accordingly. F2: Add WeCom, Google Chat, MS Teams to pinned is_bot derivation and echo-delivery tables (all 8 platforms now covered). F3: Fix pseudocode precision — unwrap_or no longer borrows a temporary; add PlatformTrustConfigs::get() and PlatformTrustConfig::surface_allowed() delegating method used by gate_event. * fix(adr): address group review round 2 — 9 findings from B1/B4/B5/B8/B11/B12/B15 Fixes: - WeCom is_bot: remove enter_agent (user-initiated, not bot); keep only trusted_bot_ids (B5 F1, B12 F1) - SlackTrustConfig::decide(): workspace_users is now strict override (ignore allow_all_users); base fallback supports team_id:user_id composite key for Enterprise Grid (B4 F1, B5 F2, B12 F3) - LINE group policy: fix prose vs code contradiction — unconfigured groups use default_group_policy, not DenyScope (B12 F2) - Reserved platform validation: MUST-level requirement for all external Receivers to reject reserved platform names; cron bypass invariant documented; Phase 1 SHOULD for typed InboundSource enum (B1) - decide() simplified to use self.get() — remove duplicate default logic (B5 F3) - Echo rate-limit key updated to (platform, workspace_id, sender_id) (B12 F4, B15 F2) - Module layout: InboundEvent in mod.rs (public), gate.rs narrow (B12 F5) - into_inner() trust boundary doc: module-level vs crate-level explained, Phase 1 SHOULD for lint/annotation (B11, B15 F1) - Slack Enterprise Grid gotcha: split dense table cell into footnote (B8 F2) * fix(adr): add platform lowercase invariant to InboundEvent (B14 F2) InboundEvent.platform MUST be lowercase — Receivers normalize before constructing. This ensures consistency between gate_event's == "cron" check and PlatformTrustConfigs::get()'s to_lowercase() lookup. * docs(adr): clarify gw_event.platform source in run_gateway_platforms Add comment noting platform field is assigned by gateway routing config, not from webhook payload body — satisfies reserved platform invariant. * docs(adr): WeCom corrections from external review (canyugs#18) Three verified adjustments to the v2 trust architecture text: - WeCom callback mode is DM-only: channel_type always "direct" with a per-user channel id, so "group routing" does not exist for WeCom and its L2 scope is effectively allow_dm only (verified wecom.rs:1059-71) - Sender-id table: plain UserIDs are self-built-app only; external contact / ISV callbacks carry wm/wo-prefixed external_userid or encrypted OpenUserID (verified against official WeCom API docs) - Bot-bypass wording: is_bot is hardcoded false in the WeCom Receiver today, so the L3 bot-bypass is a no-op for WeCom — the bypass is available uniformly but effective only where the Receiver can derive is_bot Credit: canyugs. * docs(adr): refresh Phase 0.5 snapshot to shipped state Since the last revision, main gained: Slack L3 gate (#1363), per-platform [section] trust for all 8 platforms (#1365/#1366/#1385), full config-first parity with conformance guard (#1375/#1387), and the L1 unenforceable-auth startup warning (#1373). The Phase 0.5 row now lists the actual inventory and names the one outstanding Phase 1 prerequisite (standalone WS path still on should_skip_event, tracked on #1356). --------- Co-authored-by: chaodu-agent <chaodu-agent@openab.dev> Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>
brettchien
added a commit
to brettchien/openab
that referenced
this pull request
Jul 20, 2026
…penabdev#8) How the openab-browser tools reach each agent CLI: the per-session loopback proxy + where openab writes the {url, headers} entry per variant (Cursor/Kiro auto today; Claude/Codex/Gemini paths documented, not yet auto). Honest caveat: static manual config awaits the stable-endpoint redesign (openabdev#9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
brettchien
added a commit
to brettchien/openab
that referenced
this pull request
Jul 24, 2026
…penabdev#8) How the openab-browser tools reach each agent CLI: the per-session loopback proxy + where openab writes the {url, headers} entry per variant (Cursor/Kiro auto today; Claude/Codex/Gemini paths documented, not yet auto). Honest caveat: static manual config awaits the stable-endpoint redesign (openabdev#9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepagent
pushed a commit
that referenced
this pull request
Aug 3, 2026
…1447) * docs(acp): add MCP-over-ACP browser-control implementation blueprint (§7) Break the north-star (LLM operating the browser via MCP over /acp) into T0–T7 with sub-tasks, an OpenAB-side vs extension-side ownership split meeting at the MCP-over-ACP wire contract (T4), and the key findings that reshape the work: the agent→client request direction already exists on the downstream hop (request_permission is auto-replied in openab-core, so T1 is a relay not green-field), and mcpServers is currently [] (T5 injects a core proxy). Suggested order + which items are heavy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): resolve MCP-over-ACP browser-control design (D1-D4) + flow diagrams Fold the resolved design decisions into the browser-control ADR §7: - D1: auto-approve all browser tool permissions (core keeps auto-replying request_permission); fine-grained control deferred. Drops the dedicated request_permission-relay task; T1's server->client machinery stays (needed by the upstream MCP tunnel). - D2: inject the proxy via each agent's native MCP config (Cursor -> .cursor/mcp.json), not ACP session/new mcpServers (Cursor ignores those; cf. zed-industries/zed#50924). Content (HTTP url+headers) is portable; no universal config location exists. - D3: downstream (agent<->core) is a normal in-process Streamable-HTTP MCP server on loopback (via rmcp), NOT an on-ACP-stream tunnel (the ACP maintainer backed off on-stream MCP; cf. discussion #58). Upstream (core/gateway<->extension) is the one legitimate tunnel and adopts the official MCP-over-ACP RFD framing (mcp/connect + mcp/message); the RFD's "type":"acp" downstream injection is unused (Cursor unsupported). - D4: core's HTTP MCP server is always-on and decoupled from the extension WS, so the WS can attach after session start; core static-advertises the browser toolset and emits notifications/tools/list_changed on attach/detach. Also adds a TL;DR flow, an as-designed execution flow, a detailed message-level runtime sequence, and the T0 spike checklist; updates Findings/Tasks/Ownership accordingly (T3 dropped, T4 = RFD framing, T5 = HTTP MCP server + per-adapter config injection). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway/acp): server-initiated request direction (T1.2/1.3) Add the agent->client REQUEST direction to the ACP WebSocket server, the plumbing the MCP-over-ACP tunnel needs. The base only had client->server requests plus server->client notifications; this adds: - route_client_response(): the read loop now recognises an inbound client *response* (id present, no `method`, carries result/error) and routes it to the waiting request via a per-connection pending map, instead of answering it with -32600. Gated on !is_notification so notification/request handling is untouched. - pending_requests: Arc<Mutex<HashMap<u64, oneshot::Sender<Value>>>> per connection, drained on disconnect so in-flight awaiters unblock with "connection closed" rather than hanging until timeout. - send_request() + JsonRpcRequestOut: mint an id, register the oneshot, send the frame over the existing outbound channel, timeout-await the correlated response. Landed with #[allow(dead_code)] as ready infrastructure; its caller arrives with T1.4 (the core<->gateway bridge). Mirrors the existing client-side pattern in openab-core/src/acp/connection.rs. Adds an acp_requests test module (route + send_request round-trip, id minting, request/notification rejection, unmatched id). Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(gateway/acp): construct trivial responses from generated types (T2.1) Migrate the three trivial outbound response payloads from hand-rolled `json!` to the generated `acp_schema` types, so the wire shape is type-checked: - session/new → NewSessionResponse { session_id: SessionId(..) } - session/resume → ResumeSessionResponse::default() (serializes to {}) - prompt final → PromptResponse { stop_reason }, with `stop_reason` now a typed StopReason enum (EndTurn / Cancelled) instead of a &str literal. rename + skip_serializing_if make the emitted wire byte-identical to the prior `json!`, so the existing conforms::<T> round-trip tests and handler behaviour tests are unchanged (292 pass). handle_initialize stays hand-rolled for now (nested agentCapabilities/agentInfo; low value, per base ADR §7 the trivial chat subset does not require typed construction). The remaining T2.2 (typing the mcp/connect + mcp/message bidirectional frames) lands with T4. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway/acp): MCP-over-ACP tunnel frame API (T4.1) Add the gateway-side helpers for the official MCP-over-ACP RFD tunnel (agentclientprotocol.com/rfds/mcp-over-acp), built on the T1 send_request machinery (its first real callers): - mcp_connect(acpId) -> connectionId - mcp_message_request(connectionId, method, params) -> inner MCP result - mcp_disconnect(connectionId) - McpConnectParams / McpConnectResult / McpMessageParams / McpDisconnectParams (hand-rolled: these RFC methods are not in the generated acp_schema, which only has the session/new McpServer* declaration types + McpCapabilities), plus frame_result() to unwrap a response frame's result / surface its error. Per the RFD, mcp/message flattens the inner MCP method/params into the params object WITHOUT the inner MCP id; correlation is purely by the outer ACP id, and the response result is the inner MCP result payload. Adds a mock-tunnel round-trip test (mcp/connect -> connectionId, mcp/message tools/list -> result). Helpers carry #[allow(dead_code)] until T5 wires them to the core MCP proxy (their real caller). Remaining T4: session/new "type":"acp" parsing + advertise mcpCapabilities.acp, gateway<->core routing (with T5), contract doc. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): scaffold MCP proxy server deps + static browser toolset (T5.1a) Introduce the core-hosted MCP proxy for MCP-over-ACP browser control (D3/D4), starting with the dependency integration + the static tool set: - openab-core gains optional rmcp (server + transport-streamable-http-server), axum 0.8 (matches the gateway, one axum in the workspace), and tokio-util, gated behind a new `acp-mcp` feature so non-acp builds don't pull them. The root `acp` feature (included by `unified`) now enables `openab-core/acp-mcp`. This is the workspace's first rmcp server-side usage; it resolves + compiles cleanly alongside openab-agent's rmcp client features. - New `mcp_proxy` module (feature-gated) with `browser_tools()`: the fixed DOM-semantic tool set (click / read_dom / navigate / type / screenshot) that, per D4, core static-advertises regardless of whether an extension is attached. Built from rmcp `Tool::new` + typed input schemas; unit-tested. `browser_tools()` carries #[allow(dead_code)] until the ServerHandler wires it. Next (T5.1b): ServerHandler impl + spawn_mcp_server (loopback + bearer axum listener); then T5.2 config injection and T5.3 tunnel wiring. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): MCP proxy ServerHandler + loopback server (T5.1b) Stand up the core-hosted MCP server the colocated agent connects to (D3): - ProxyHandler impls rmcp ServerHandler: get_info advertises the tools capability; list_tools returns the static browser tool set (D4 static- advertise); call_tool returns "browser not connected" until the tunnel is wired (T5.3) — failing gracefully rather than hiding the tools (D4). - spawn_mcp_server binds an OS-assigned 127.0.0.1 port with its own axum listener (StreamableHttpService, stateless + JSON responses), graceful shutdown via a CancellationToken. The caller hands the port to the agent's native MCP config in T5.2. An HTTP integration test spawns the server, confirms it binds loopback, and that an MCP initialize returns a result advertising the tools capability. Bearer auth on the listener is added in T5.2 (the token is minted alongside the .cursor/mcp.json injection). Tunnel wiring (RemoteExtensionChannel -> mcp_connect /mcp_message) is T5.3. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): bearer gate on the MCP proxy server (T5.2 part 1) Even bound to loopback, the core MCP server now requires the token the agent's MCP config carries (D3), so another local process on the host can't reach the browser tools. spawn_mcp_server takes a `bearer` and layers an axum middleware that returns 401 when Authorization: Bearer <token> is absent or wrong; the caller mints the token and shares it with the agent config. Tests: authed initialize -> 200 + tools capability; missing / wrong token -> 401. Remaining T5.2: the per-agent adapter writes { url: 127.0.0.1:<port>, headers: Authorization Bearer } into the agent's native MCP config (Cursor -> .cursor/mcp.json) before boot, and wires spawn_mcp_server into openab startup. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): correct runtime diagram — mcp/message flattens, no inner id The MCP-over-ACP RFD flattens the inner MCP method/params into the mcp/message params and does NOT carry an inner MCP id; correlation on the upstream tunnel is by the outer ACP id alone, and the response result IS the inner MCP result payload. Fix the detailed runtime sequence + the id-space note: mcp#7 lives only on the agent<->core HTTP hop; the core proxy maps its downstream mcp#7 <-> the upstream acp#55. (Was: "carried verbatim agent<->core<->extension".) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway/acp): parse + record client-declared type:acp mcpServers (T4) The browser extension declares its MCP-over-ACP server in session/new via the RFD's mcpServers entry {"type":"acp","id":...,"name":...}. Parse those (raw, since the "acp" transport is an RFD proposal not in the generated schema) and record them per session (AcpSession.acp_mcp_servers), so the gateway can later mcp/connect to them (T5.3). session/resume re-records them since the client re-presents mcpServers. http/sse/stdio servers are ignored (the agent connects to those itself). D5-agnostic: needed regardless of the core MCP server topology. Field is #[allow(dead_code)] until the mcp/connect wiring consumes it. Tests: parse keeps only acp entries (+ empty cases); session/new records them. Not done here: advertising mcpCapabilities.acp in initialize — the generated McpCapabilities has only http/sse (the RFD acp flag isn't in stable v1), and since we own both ends the extension can declare type:acp unconditionally; left as a follow-up. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: MCP-over-ACP tunnel contract for the extension (T4.3) The spec the browser extension (katashiro, T6) implements: the gateway<->extension hop of MCP-over-ACP. Covers the type:acp session/new declaration, mcp/connect -> connectionId, mcp/message (inner method/params flattened, correlate by outer ACP id, result = inner MCP result), mcp/disconnect, the baseline browser tool set (click/read_dom/navigate/type/screenshot), and that permissions are auto-approved (D1) + the WS may attach after session start (D4). D5-agnostic (only the external hop; OpenAB-internal proxy/topology is out of scope), so it lets the extension side proceed in parallel. Linked from ADR §7 T4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway/acp): TunnelHandle — per-connection MCP tunnel handle (T5.3) The reusable abstraction the core MCP proxy needs to reach a specific browser: TunnelHandle bundles one /acp connection's outbound channel + pending-request map + id counter + the mcp/connect connectionId, and exposes async mcp_message() / disconnect() that tunnel an inner MCP request to that extension and await the result. Built on the T1/T4.1 send_request + mcp_message_request helpers. D5-agnostic: both the per-session and shared core-server designs route through this same handle. Round-trip test via a mock extension driver. Next: register a TunnelHandle per session's channel_id (after mcp/connect) in a shared registry (AppState), and consume it from the core ProxyHandler. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway/acp): tunnel registry + establish_and_register_tunnel (T5.3) - AcpTunnelRegistry (channel_id -> TunnelHandle), mirroring AcpReplyRegistry, so the core MCP proxy can look up the tunnel for a given browser session. - establish_and_register_tunnel: mcp/connect to a session's declared "type":"acp" server, build a TunnelHandle from the returned connectionId, and register it under the session's channel_id. First real caller of mcp_connect/send_request. Documented as spawn-only (awaiting mcp_connect inline in the read loop would deadlock, since only that loop delivers the response). Test: a mock extension answers mcp/connect; the handle lands in the registry keyed by channel_id. #[allow(dead_code)] until the read loop spawns it and it's threaded through AppState (next). Then the core ProxyHandler consumes the registry to forward tools/list + tools/call. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway): thread AcpTunnelRegistry through AppState (T5.3) Add acp_tunnel_registry: Option<AcpTunnelRegistry> to AppState alongside acp_reply_registry (same #[cfg(feature="acp")] gate), initialized wherever the reply registry is. This is the shared handle the connection read loop will populate (spawning establish_and_register_tunnel per declared type:acp server) and the core MCP proxy will consume to route a tool call to the right browser. No behaviour change yet — the field is constructed but not read until the read-loop spawn wiring lands next. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(gateway/acp): open MCP tunnels on session/new + cleanup (T5.3) Wire the tunnel producer into the connection read loop: - handle_acp_connection mints a per-connection next_req_id (Arc<AtomicU64>) for server-initiated requests. - handle_session_new returns the minted channel_id alongside the response. - On session/new, for each declared "type":"acp" server, tokio::spawn establish_and_register_tunnel (mcp/connect -> register a TunnelHandle under the channel_id). Spawned, never awaited inline: it awaits mcp/connect whose response only this same read loop delivers, so awaiting inline would deadlock. The task is tracked in prompt_tasks (aborted on disconnect). - Disconnect cleanup now removes the connection's channel_ids from BOTH the reply and tunnel registries (gathered once). Live behaviour needs a real extension (T7); this lands the plumbing + keeps the unit tests green. Next: the core ProxyHandler consumes acp_tunnel_registry to forward tools/list + tools/call to the right browser. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): BrowserTunnel trait + ProxyHandler forwards tool calls (T5.3, D6-a') Add the core-side tunnel interface (D6-a'): `trait BrowserTunnel { async fn call(channel_id, method, params) }`, implemented by the root (bridging to the gateway registry) so no core<->gateway crate dependency is introduced — matching the existing ChatAdapter pattern. - ProxyHandler now carries its session channel_id + an Option<Arc<dyn BrowserTunnel>> (D5-a: one server per session). call_tool forwards the tool as an MCP tools/call over the tunnel; list_tools stays static-advertised (D4); no tunnel / no browser attached -> "browser not connected" (D4). - spawn_mcp_server takes (channel_id, tunnel) and builds a per-session ProxyHandler in the service factory. Tests: forward via a mock BrowserTunnel; not-connected without one. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): start_session_server — per-session server + .cursor/mcp.json (T5.2/D5-a) start_session_server(channel_id, workdir, tunnel): mint a fresh bearer, start the loopback MCP proxy for that session, and MERGE an `openab-browser` HTTP entry (url + Authorization: Bearer) into <workdir>/.cursor/mcp.json without clobbering any servers already there (Cursor's native config; D2). Returns the bound addr + a CancellationToken the pool cancels to stop the server on session evict. Tests: writes the cursor config (url+bearer); merges into an existing mcp.json. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): start per-session MCP proxy on agent spawn (T5.2/D5-a) Wire the per-session MCP proxy into the pool's agent-launch path (feature acp-mcp): - For a browser (`acp:`) session, get_or_create starts a loopback MCP server + writes .cursor/mcp.json BEFORE spawning the agent, so the agent connects to it on boot. Non-acp sessions (Discord, etc.) are untouched. - Lifecycle: the server's CancellationToken drop_guard is stored INSIDE the AcpConnection (new mcp_server_guard field), so the server is cancelled whenever the connection is dropped — through any evict/suspend/hung-kill path — without touching each removal site. On a failed spawn/init the guard drops early and cancels too. - SessionPool gains a browser_tunnel field + with_browser_tunnel() builder (set by the root, D6-a'); passed into each per-session ProxyHandler. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(root): wire the browser tunnel bridge end-to-end (T5.3, D6-a') RootBrowserTunnel (src/browser_tunnel.rs) implements openab-core's BrowserTunnel trait by looking up a channel_id in the gateway's AcpTunnelRegistry and calling TunnelHandle.mcp_message — the root glue that connects the two sibling crates without either depending on the other (mirrors the ChatAdapter pattern). Wiring in `openab run` (feature acp): create ONE shared acp_tunnel_registry before the pool; give the pool a RootBrowserTunnel over it (with_browser_tunnel), and inject the SAME registry into the gateway AppState so acp_server populates the exact map the bridge reads. This closes the loop: agent tools/call -> core per-session MCP proxy -> RootBrowserTunnel -> gateway TunnelHandle -> mcp/message -> extension. Live path still needs a real extension + deploy (T7); everything compiles + unit tests green. Gate green: clippy -D warnings + test --test-threads=1 + build, --features unified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): record the as-built OpenAB side (D5-a + D6-a', end-to-end) Add a §7 "As-built" section documenting the two decisions settled during implementation and the realised call path: D5 = per-session MCP server bound to the existing channel_id map (lifetime tied to the AcpConnection via a CancellationToken DropGuard); D6 = BrowserTunnel trait in core + impl in the root (RootBrowserTunnel), keeping core/gateway sibling-independent like the existing ChatAdapter glue. Notes remaining T5.4 / T6 / T7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): MCP-over-ACP tunnel producer section in acp-ws-smoke (T7) Add a "MCP-over-ACP tunnel" section to the smoke suite: a mock extension declares a {type:acp} mcpServers entry in session/new, then asserts the gateway issues a server-initiated mcp/connect carrying the declared acpId, and answers it with a connectionId (registering the tunnel). This exercises the live read-loop spawn + server->client request path end-to-end — the concurrency unit tests can't reach. Runs against a live server (deploy T7). The tunnel path is inert for normal sessions (only triggers on a type:acp declaration), so it does not affect existing ACP traffic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): complete the MCP-over-ACP tunnel suite (fan-out + filtering) Extend the tunnel section from a single-server check to full producer coverage via a collect_mcp_connects() helper: single type:acp → exactly one mcp/connect; fan-out (two type:acp servers → one distinct mcp/connect each, distinct request ids); mixed acp+http mcpServers → only the acp entry is tunnelled. All deterministic. Validated live against Falcon: 34/34 (tunnel 7/7). The agent→tool→browser leg is out of the WS suite's reach (needs a real extension, T6). Run: OPENAB_ACP_TOKEN=<key> uv run scripts/acp-ws-smoke.py ws://<host>/acp Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp-mcp): address Mira review nits — constant-time bearer + string-number id Two non-blocking hardening nits from the openab-side review: - mcp_proxy require_bearer: compare the loopback MCP bearer in constant time (subtle::ConstantTimeEq, matching the gateway's feishu/wecom signature checks) so a wrong token can't be recovered byte-by-byte via response timing. Adds `subtle` as an optional dep under the `acp-mcp` feature. - acp_server route_client_response: accept a stringified-number JSON-RPC id ("1") in addition to a numeric id, so a spec-loose client's responses still correlate to their pending request instead of being silently dropped. Gate (targeted, no repo-wide fmt — this container's rustfmt disagrees with the branch on pre-existing import ordering): clippy clean on both crates; openab-core mcp_proxy tests 8/8, openab-gateway acp_server tests 35/35 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp-mcp): establish the browser tunnel on session/resume, not just session/new katashiro persists its ACP session and RECONNECTS via session/resume (not session/new), re-declaring its "type":"acp" browser MCP server each time. The session/new branch spawns establish_and_register_tunnel for each declared server, but session/resume only recorded them in the session state and never opened a tunnel — so a resumed browser session had no entry in the tunnel registry and the core MCP proxy returned "no browser attached to session acp_<uuid>" on every call. Mirror the session/new logic in the resume branch: derive the same deterministic channel_id from the sessionId and spawn establish_and_register_tunnel for each declared type:acp server. This is what makes the live loop work across katashiro's auto-reconnect (which always resumes). Gate: clippy clean, openab-gateway acp_server tests 35/35. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(acp-mcp): log browser tunnel open/register for live-session observability establish_and_register_tunnel is reached only when a client declared a "type":"acp" server, so an info line there answers "did the extension advertise itself?" from the gateway log alone (the raw upstream session frame isn't otherwise logged). Logs on open and on successful registry insert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp-mcp): raise ACP frame cap 1→8 MiB for browser-tool results Browser tool results carried over the MCP-over-ACP tunnel (notably screenshots) routinely exceed the old 1 MiB inbound frame cap, which closed the WebSocket mid-response and wedged the extension in a reconnect loop. 8 MiB gives ample room for a compressed screenshot / large DOM snapshot while staying a sane DoS bound. Pairs with the katashiro-side switch to JPEG screenshots. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp-mcp): one browser tunnel per session — fix fan-out overwrite/orphan (M-B1) The tunnel registry is keyed by channel_id, and both the session/new and session/resume paths looped over every declared type:acp server calling establish_and_register_tunnel. With >1 server each insert overwrote the previous under the same channel_id, leaving the earlier tunnel opened-but-unreachable (orphaned). The core proxy only ever resolves a browser by channel_id, so one tunnel per session is the actual model. Factor both call sites into spawn_browser_tunnel(), which establishes only the first declared server and warns on extras. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp-mcp): 0600 mcp.json + strip stale bearer on evict (M-B2) start_session_server wrote <workdir>/.cursor/mcp.json with tokio::fs::write, leaving it at the umask default (typically 0644) — but the file embeds the live loopback bearer token, so any local user could read it. Write via write_private() which chmods it 0600. Also, on session evict (CancellationToken fires) strip the now-dead openab-browser entry so a stale credential doesn't linger; guarded to only remove the entry if it still points at our addr, so a concurrent/reconnected session that already replaced it isn't clobbered (the mcp.json path is shared across acp: sessions). Adds a 0600 assertion to the existing config-write test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(acp-mcp): lock subtle dependency Cargo.lock was missing the openab-core `subtle` entry added for the constant-time bearer compare, which would fail a `--locked` build. No code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(mcp-proxy): write openab-browser into kiro-cli config too, not just Cursor start_session_server now merges the openab-browser entry into BOTH .cursor/mcp.json and .kiro/settings/mcp.json (each CLI ignores the other's), and cleans both on evict. kiro-cli parses the {url, headers} shape identically. Deployed as acpmcp-kirofix. Also add .dockerignore (exclude target/, .git/, data/) for the acp image builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: browser MCP agent setup — per-variant mcp.json how-to (Phase 2 #8) How the openab-browser tools reach each agent CLI: the per-session loopback proxy + where openab writes the {url, headers} entry per variant (Cursor/Kiro auto today; Claude/Codex/Gemini paths documented, not yet auto). Honest caveat: static manual config awaits the stable-endpoint redesign (#9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(mcp-proxy): per-pod browser-bridge socket server (Option C, P1) serve_browser_socket: one unix socket multiplexes all sessions; the openab browser-bridge shim forwards {channel_id, inner MCP request} frames, routed via dispatch_browser_mcp -> the shared BrowserTunnel by channel. Reuses browser_tools() + tunnel.call (single source of truth vs the HTTP ProxyHandler). +8 tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(cli): openab browser-bridge subcommand — stdio MCP relay to the browser socket (Option C, P2) A thin per-session shim: reads OPENAB_BROWSER_CHANNEL, wraps each stdin MCP request as {channel_id, request}, forwards to the per-pod core socket, relays responses to stdout verbatim. All browser MCP logic stays in core; the agent's config line is static. Gated by feature acp. + wrap/relay tests over in-memory pipes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(acp): inject OPENAB_BROWSER_CHANNEL into the agent env (Option C, P3) AcpConnection::spawn gains a browser_channel param; for an acp: session the pool passes the channel_id so the agent (and the browser-bridge shim it later spawns) inherits it and routes browser tool calls to THIS session's tunnel. env_clear-safe (re-injected explicitly). + set_browser_channel unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(mcp-proxy): static write-once browser-bridge config (Option C, P4) write_bridge_mcp_config writes the SAME {command:openab, args:[browser-bridge]} entry to cursor + kiro mcp.json — no port/bearer, so it never goes stale and can't clobber across sessions (the root cause of multi-window browser flakiness). Merges without touching the user's servers; idempotent. Additive — P5 wires the proxy/bridge toggle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: OPENAB_BROWSER_MODE proxy|bridge toggle wiring (Option C, P5) BrowserMode + browser_mode() (default proxy) + shared browser_socket_path(). Pool branches: proxy = per-session HTTP server (unchanged default); bridge = static write-once config, no per-session server. Broker starts the per-pod socket server once in bridge mode. browser-bridge shim uses the shared socket path. + parse tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browser-bridge): resolve channel via process-ancestry, not env (Option C, b2 B1) The MCP client scrubs the child env (cursor gives the bridge only HOME/PATH/USER or the pod env, never the per-session OPENAB_BROWSER_CHANNEL), so env inheritance can't carry the channel. resolve_channel() now walks up the PPID chain and reads OPENAB_BROWSER_CHANNEL from the ancestor agent's /proc/<pid>/environ (openab injected it via the pool) — generic across all stdio-MCP vendors. Logs the resolved channel to stderr. + parse_ppid_from_stat / parse_channel_from_environ unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(mcp-proxy): revert bridge config to pure {command,args} (Option C, b2 B2) Drop the ${OPENAB_BROWSER_CHANNEL} config env — cursor doesn't expand it (spawns from pod/clean env). The bridge now resolves its channel via process-ancestry (B1), so the config is a byte-identical static entry again: idempotent, never stale, no clobber. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(acp): add acp_mcp_servers to test-only AcpSession initializers The 4 AcpSession constructors in `mod acp_review_fixes` tests missed the acp_mcp_servers field added in T4, breaking `cargo test -p openab-gateway --features acp` (E0063). build/clippy don't compile this crate's test target under `acp`, so only CI caught it. Also syncs Cargo.lock to the already-committed openab 0.10.0 version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(acp): split into reverse-MCP mechanism ADR + browser-control ADR, embed diagrams Two ADRs instead of one: - acp-server-websocket-reverse-mcp.md — the generic reverse-MCP-over-ACP mechanism (roles, call route, protocol gap, §6 multi-server generalization: compound-key routing, dynamic tools/list + list_changed, per-server Option B). Embeds the architecture + MCP-usage sequence diagrams (mermaid), using browser control as the example. Flipped Proposed -> Accepted (as-built in #1447). - acp-server-websocket-mcp-browser.md — the browser-specific design + the contract the browser extension implements (D1-D6, detailed id-paired runtime sequence, tasks, as-built). Defers the mechanism to the reverse-MCP ADR. Update base ADR + tunnel-contract cross-links; mark base §6 browser critical-path done. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): correct D4 list_changed overclaim in browser ADR list_changed is designed but not yet implemented (0 hits in gateway/core crates); it was described as shipped alongside static-advertise. Reword to mark it as P2b-tracked (reverse-MCP §6.2), not as-built. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(acp): compound-key (channel_id,server_id) tunnel registry + rename BrowserTunnel->AcpMcpTunnel (P1, Fork A) Behavior-preserving refactor toward generic multi-server MCP-over-ACP (reverse-MCP ADR §6). No functional change for the single browser server. - AcpTunnelRegistry: HashMap<String,_> -> HashMap<(String,String),_>; register under the client-declared srv.id; evict all (channel_id,*) on teardown. - Core trait BrowserTunnel -> AcpMcpTunnel; call() gains a server_id param. - Read side (Fork A): the single-browser proxy + bridge pass an empty server_id sentinel; RootBrowserTunnel resolves the sole tunnel on the channel (errors if ambiguous). Real per-server read-side routing (bridge-frame server_id) is deferred to P2. Gate: build --features acp, clippy --workspace -D warnings (+unified), test -p openab-gateway --features acp — all green (307 passed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: regenerate Cargo.lock for merged deps * fix(mcp-proxy): register browser server in kiro per-agent configs (--agent mode) When kiro-cli runs with --agent <name> — as every OAB bot deployment does — the MCP server list comes from .kiro/agents/<name>.json and tools are gated by that file's default-deny allowedTools, NOT from .kiro/settings/mcp.json (verified live on the b2 fleet deployment; see docs/gmail-native.md 'Kiro CLI gotcha'). Without this, browser tools are invisible to exactly the deployments this feature targets. - merge_kiro_agent_configs: merge the openab-browser entry into every .kiro/agents/*.json and add @openab-browser to allowedTools; agent files carry unrelated config, so unparseable files are skipped, never clobbered (unlike the settings writer); macOS ._* droppings ignored; idempotent; 0600 (proxy entries carry a live bearer). - cleanup_kiro_agent_configs on session evict: remove the entry and revoke the allowlist grant only when the URL is still ours, preserving a concurrent session's live entry (same rule as the settings cleanup). - Wired into both the per-session proxy writer and the static Option C bridge writer; 4 new tests. * feat(mcp): browser capabilities through the session-aware facade (Facade mode, default) Routes browser tools through the OAB MCP Facade as a session-aware in-process capability source (openab-mcp #1454), replacing per-session proxy servers as the default transport. Proxy and Option C bridge modes are unchanged and remain explicit opt-outs (OPENAB_BROWSER_MODE). - src/browser_source.rs: CapabilitySource over the existing AcpMcpTunnel (requires_session; D4 static-advertise; tunnel errors surface as MCP error results); FacadeRegistrar adapts the facade's SessionTokens to core's new SessionTokenRegistrar hook (core stays openab-mcp-free). - core mcp_proxy: BrowserMode::Facade (new default; runtime fallback to Proxy when no facade is serving), write_facade_mcp_config — a static, write-once 'openab' entry whose Authorization references ${OPENAB_SESSION_TOKEN}; the per-session secret rides the agent process env instead of config files, eliminating the shared-workdir clobber class entirely (incl. kiro --agent files + @openab allowlist). - pool: with_facade_sessions wiring; mints/injects the token per spawn, revokes via the same DropGuard plumbing proxy mode uses. - main: facade constructed with the BrowserSource; one listener, one discovery surface (search_capabilities/execute_capability). - rmcp re-exported from openab-mcp for source implementors. - docs: facade-mode section in the browser setup guide. * fix: facade_serving is acp-only — derive it, don't flag it (default-features -D warnings) * docs(adr): §6 builds on the OAB MCP Facade — one AcpTunnelSource, static-advertise, trust gate Rewrites reverse-MCP ADR §6.2-§6.6 now that the facade seam landed upstream (#1448/#1453 facade, #1454 session-aware CapabilitySource, #1446 ADR): - §6.2 expose every client-declared type:acp server through ONE in-process CapabilitySource (AcpTunnelSource) registered with the facade, rather than the bespoke per-(session,server) loopback proxies + N mcp.json entries. Sources are registered once at construction, so the source fans out internally and routes on the <server>.<tool> prefix to (channel_id, server_id). Session identity moves to the facade's SessionTokens. - §6.3 drop notifications/tools/list_changed outright — facade discovery is pull-based (search_capabilities re-reads per call), so nothing caches a tool list to invalidate. Keep the static-advertise posture, implemented as fetch-once-per-declared-server + per-(channel,server) cache; unavailability is a call error, not a vanishing catalog entry. - §6.4 new trust requirement: #1454 assumes operator-granted tool sets, but a tunnel source's tools are client-declared — require an operator allowlist (default: browser only) plus a per-declared-server tool_filter. - §6.5 records what this retires and leaves stdio bridge-mode removal as an explicit operator call; flags the meta-tool-vs-direct hop as open. - Browser ADR: correct D4 (list_changed dropped, static-advertise kept) and add a supersession notice over D2/D3/D5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): align §6 with the merged facade series + record the mcpServers divergence Follows the merge of main (adapter ADR #1446, gmail doc #1455) into this branch: - Link the OAB MCP Adapter ADR directly now that it is present, and note the whole facade series (#1446/#1448/#1449/#1450/#1453/#1454) is merged with no facade PR left open — §6 builds on a settled foundation. - Cite adapter ADR §6.2 / Alternative C ("no second generic inbound MCP server", browser and external capabilities share one delivery mechanism), which makes retiring the bespoke per-session proxy (F5) an upstream design requirement rather than optional cleanup. - Record an unresolved divergence: the adapter ADR says the facade is delivered via ACP `mcpServers` and explicitly not by editing CLI config files, while the as-built `write_facade_mcp_config` does write a static entry — deliberately, since browser D2 found Cursor ignores ACP-passed mcpServers. Flagged for the facade contract owner instead of unilaterally reconciling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): tighten §6.4 trust gate — deny-all tool_filter + pinned browser tool set Falcon's review of §6 (F7): the operator allowlist of declared server names is not a trust boundary on its own. The name is chosen by the same remote client that declares the tools, so a client may declare a server named `browser` and publish an arbitrary tool set under it. Align the §6.6 F4 summary with the requirement §6.4 now states: the per-declared-server tool_filter is deny-all by default, and the `browser` entry ships pinned to its five known tools so a same-name declaration cannot inject others. Co-Authored-By: Claude <noreply@anthropic.com> * docs(adr): §6.1/§6.2 — declared id vs name, and last-attach-wins on same-name tunnels Found while scoping F1': the routing contract as written could not be implemented. A declaration is {type:"acp", id, name} and the reference client mints `id` as a fresh crypto.randomUUID() per connection while `name` ("browser") is stable. The registry is keyed by `id`, but the `<server>` segment of a tool name (`browser.click`) and the §6.4 allowlist are the `name` — so routing "on the prefix to the matching (channel_id, server_id) tunnel" can never match: the key is a UUID the tool name never contains. Record what review settled (Mira + Falcon, 2026-07-26): - registry stays keyed by (channel_id, id) — keying by name would let two same-name tunnels overwrite each other, the fan-out collapse §6 fixes — but must also record the declared name so a source can enumerate (name, id); - trust gating is keyed by name, since ids are per-connection UUIDs; - same-name collisions are last-attach-wins: the new tunnel replaces and evicts the older entry. Answering "ambiguous" there would wedge the client out of its own tools on every reconnect, because each reconnect mints a new id. Also clarify that the prefix selects the tunnel and is NOT stripped: the full published name is what goes over the tunnel, since that is what the server's own tools/call expects. Co-Authored-By: Claude <noreply@anthropic.com> * feat(acp): record declared server name, establish all declared tunnels, LWW on re-attach Tunnel-layer plumbing for §6 F1' (generalising the capability source to N client-declared servers). Behaviour-neutral for today's single-browser client: one declared server still yields exactly one tunnel. A declaration is {type:"acp", id, name} and the two fields have different lifetimes — the reference client mints `id` as a fresh crypto.randomUUID() per connection while `name` ("browser") is stable. The registry is keyed by `id`, but a tool name carries the `name` (browser.click) and the §6.4 trust gate is keyed by it too, so the name has to survive registration to be routable. - TunnelHandle records the declared `server_name` and exposes it. - establish_and_register_tunnel takes the declared name and resolves a re-declared name last-attach-wins: the new tunnel evicts stale same-name entries on the channel. Because a reconnect mints a new id, the dead tunnel would otherwise linger beside the live one, and answering "ambiguous, pass a server_id" there would wedge the client out of its own tools on every reconnect. The eviction is also what bounds registry growth. - spawn_browser_tunnel -> spawn_acp_tunnels now establishes EVERY declared server. The old first-only limit existed because the registry was keyed by channel_id alone, where a second server overwrote the first and orphaned its tunnel; the compound key removed that collision. - AcpMcpTunnel gains servers(channel_id) -> Vec<(name, id)>, implemented by RootBrowserTunnel over the registry. This is what lets a capability source resolve a tool prefix back to a tunnel; matching a prefix against the registry key alone can never work, since the key is a UUID the tool name never contains. Default impl is empty so test doubles are unaffected. The source-side consumer (AcpTunnelSource routing + the §6.4 trust gate) is the next step; nothing reads servers() yet. Co-Authored-By: Claude <noreply@anthropic.com> * feat(acp): route client-declared servers by name and gate their tool sets Second half of §6 F1' — the consumer of the tunnel plumbing added in 5dc45de0. BrowserSource becomes AcpTunnelSource: it fans out to every client-declared MCP server instead of assuming one implicit browser, and enforces the §6.4 trust gate. Behaviour for today's single-browser client is unchanged. Routing (§6.1/§6.2): the `<server>` prefix of a published tool name is the declared *name*, while the registry is keyed by the per-connection `id`, so `call` resolves name -> (channel_id, id) via the registry enumeration. The full published name is forwarded (`browser.click`), not the suffix — the prefix selects the tunnel, it is not stripped, because the server's own tools/call expects the name it published. Trust gate (§6.4), two independent checks: - the declared name must be in the operator allowlist (default: browser only); - the tool must be one that server is pinned to. The second is not redundant with the first. The name is chosen by the same remote client that declares the tools, so a client can re-declare the trusted name `browser` and publish `browser.exec`; the pin is what refuses it. Denied calls never reach the tunnel. Both cases are covered by tests. `browser` appears only as an entry in the default policy table — deliberately data, not a branch — so the routing code stays generic and admitting another client-side MCP service is a table entry (§6.2: no browser-specific branch). tools() serves the policy table statically and is deliberately NOT intersected with the tunnels currently attached: intersecting would make the catalog flap as a tab detaches, which §6.3 forbids, and would lose the pre-attach discovery D4 already provided. Availability is reported by call, never by a shrinking catalog. Session *scope* — restricting to the servers a given client declared — is a separate axis needing F3's declaration cache, so tools() ignores ctx for now; an allowlisted server's pinned tools are advertised to every session, which is the status quo for the browser. Co-Authored-By: Claude <noreply@anthropic.com> * docs(adr): §6.3 — policy entries seed the catalog; cache narrows, never grants Found while scoping the discovery cache: §6.3 said an un-cached declared server "contributes an empty set", which contradicts the static-advertise posture that §6.4's pinned sets and D4 both rely on, and which the source implemented in ba94efec (browser's pinned tools are advertised before the extension attaches — confirmed correct on review). Record the layering review settled instead: - a server's §6.4 policy entry is its pre-attach SEED as well as its filter, so a pinned server never drops to empty just because nothing has attached; - the per-(channel_id, server_id) cache holds fetched ∩ allowed and replaces the seed once a fetch succeeds, narrowing the catalog to what the server really publishes without ever widening past the policy; - a declared server with no policy entry contributes nothing because §6.4 is deny-all, not because it is un-cached. Caching is never itself a grant. Also record the ordering consequence: deny-all plus pinned entries that already carry full Tool schemas means fetching cannot surface anything the operator has not already permitted, so the discovery cache is invisible until the operator-facing config surface exists. The config surface lands first; the cache then supplies real schemas once operators may list tools by name alone. Co-Authored-By: Claude <noreply@anthropic.com> * feat(mcp): operator config surface for the client-declared server allowlist Completes the §6.4 gate: enforcement landed in ba94efec, but the allowlist and per-server tool filter were a hardcoded table with no way for an operator to change them. Adds [[mcp.acp_servers]] entries of {name, tools}. Keyed by the declared name, never the id — the reference client mints its id as a fresh UUID per connection, so an allowlist of ids could not match twice. ServerPolicy now separates the two jobs that were conflated in one field, which is the layering §6.3 settled: - `allowed` is the deny-all gate over tool NAMES; - `seed` is the pre-attach advertisement (full Tool values), always a subset of `allowed`, so narrowing the policy narrows the catalog and can never widen it. Operators may list tools by name alone. For a server with a built-in catalog the schemas are taken from it and narrowed to what was permitted, so restricting the browser to read_dom needs no restated JSON schema. A server admitted by name with no built-in catalog has no seed yet: it dispatches, but advertises nothing until discovery caching can fetch its real schemas — which is precisely the job that gives the cache a non-redundant purpose. Two deliberate behaviours, both tested: - an ABSENT/empty section keeps the built-in browser default, so omitting the config cannot silently break existing browser control; - writing ANY entry takes over the allowlist wholesale — browser is not retained alongside an operator's list, so the config never grants more than it states. Co-Authored-By: Claude <noreply@anthropic.com> * feat(acp): discovery cache — fetch each declared server's real tools/list Implements §6 F3'. A server an operator admitted by name alone had no schemas to advertise; it could dispatch but was invisible. Discovery fills that gap, which is the job that gives the cache a non-redundant purpose. Two deviations from §6.3 as written, both applied to the ADR in this commit: 1. The cache is keyed by (channel_id, NAME), not (channel_id, server_id). Ids are minted per connection, so an id-keyed entry would be orphaned by exactly the reconnect the cache exists to survive — it could never outlive the attach that populated it, which is the opposite of "serve regardless of current attach state". This is a corollary of the id-vs-name distinction §6.1 already records. Same-name collisions are impossible under last-attach-wins, so the name is a safe key. Covered by a test that reconnects under a fresh id. 2. Discovery is pull-triggered, not attach-triggered: a declared server with no cache entry has its fetch started from the next tools() call and its real set appears one discovery round later. The facade re-reads the catalog on every call, so one round of staleness is the whole cost, and it avoids threading an attach hook from the gateway (which owns attach) into the root (which owns the source). The cache stores what the server PUBLISHED, unfiltered, and the policy is applied on read. Filtering on read means tightening the policy takes effect immediately rather than waiting for an entry to be invalidated, and it keeps the invariant that caching is never itself a grant: a server that publishes a tool the operator never permitted stays both invisible and uncallable. Tested. A failed fetch leaves the seed in place — a seeded server never drops to empty — and clears its in-flight marker so the next round retries. Repeated discovery rounds do not pile up duplicate tools/list requests on one tunnel. Co-Authored-By: Claude <noreply@anthropic.com> * test(acp): two client-declared servers in one session Covers the multi-server claim §6.2 makes, through the real source: one session declares `browser` and a second, non-browser server; both are discovered and callable, tool names do not collide, and each server's policy is enforced independently. - both servers contribute to one catalog, each under its own prefix, with no duplicate names (the case a naive un-prefixed catalog would collapse); - each tool reaches the tunnel of the server that declared it, by name -> id; - a permission granted to one server does not leak to another: browser.click is permitted while notes.click is refused, proving the gate is per-server rather than a global tool-name allowlist — an easy thing to regress in a refactor and invisible with only one server configured; - one server detaching leaves its neighbour callable. SCOPE: this is the source-side half of F6, not a full end-to-end. F6 asks that "the agent discovers + calls tools from BOTH", and the agent-side leg runs through the facade's meta-tools — which cannot be exercised while facade mode is not live anywhere (no [mcp] configured; the browser deployment runs OPENAB_BROWSER_MODE=bridge). That is the same precondition F5 is blocked on. The gateway-side half — two type:acp servers each getting their own mcp/connect — is already covered by section_tunnel in scripts/acp-ws-smoke.py. What remains genuinely unproven is the facade <-> source seam. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(mcp): rename the client-declared browser surface to katashiro.* Counterpart to the katashiro-side rename. `browser` / `browser.*` collided with Playwright MCP's `browser_*` tools; the declared name and tool prefix become `katashiro` / `katashiro.*`. - mcp_proxy::browser_tools(): the five seed tools (D4 static-advertise) - browser_source::builtin_catalogs(): the catalog key, and every test that rides the default policy - config: the acp_servers doc comment naming the built-in default - docs: tunnel contract declaration + tool table, agent-setup tool list Both injection-regression tests (`unpinned_tool_on_an_allowlisted_server_is _refused`, `caching_is_never_itself_a_grant`) called `browser.exec`. Left unrenamed they still assert is_err, but for the wrong reason — an unknown server name rather than an unpinned tool on a trusted one — quietly gutting the check. They now call `katashiro.exec`. Names only; policy semantics, routing and schemas unchanged. Co-Authored-By: Claude <noreply@anthropic.com> * docs(adr): follow the katashiro.* rename through both ADRs 270a2ffd renamed the client-declared surface (`browser.*` -> `katashiro.*`) in code, config and the tunnel contract, but the two ADRs still described the old tool names — including the §6.4 pinned-tool list and the runtime sequence diagrams, which readers would otherwise copy verbatim into a policy that no longer matches. The one remaining `browser.click` is inside a verbatim quote of upstream #1454's sources.rs doc comment and is left as written. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: lead the browser-MCP setup guide with facade mode, demote proxy/bridge The guide still opened with the per-session loopback proxy as "how it reaches the agent" and only mentioned the facade in a trailing section, so a reader took the superseded design as current. Facade mode has been the default since bf37d25e. - Lead with a mode table (facade default; proxy/bridge as explicit opt-outs) and the `[mcp]` + `[[mcp.acp_servers]]` config needed to enable it. - Document what actually changes under the facade: one listener, a static write-once entry referencing `${OPENAB_SESSION_TOKEN}` (secret rides the process env, not a file), and discovery via search_capabilities rather than the agent's own tools/list. - Note the consequence the old text got backwards: because the entry is static, hand-configuring a variant openab doesn't auto-write is now viable. The old "gated on a stable browser-MCP endpoint" caveat is resolved, not pending. - Keep proxy's per-variant config table under an explicit "Legacy" heading. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): fold the browser ADR into the reverse-MCP ADR as its worked example Consolidates the ACP ADR family from four documents to three (base, the original @pahud proposal, and this one). The browser design was split out earlier in this PR, but with the facade integration the two documents had grown overlapping diagrams and the browser ADR had accumulated content that no longer described anything shipping. Folded in as §7 "Worked example — browser control": the toolset (incl. why the declared name moved to `katashiro`), D1-D6 with their supersession notice, and the message-level round-trip with the two id spaces. Dropped rather than carried over: - "Execution flow (bootstrap)" — described the pre-facade per-session-proxy boot path and duplicated D2/D3/D5. - "Tasks (as executed)" — project-management history; the commits are the record. - The separate context/references sections, which duplicated §1 and §11. Net ~90 lines smaller than the two documents were. Referrers updated, including the two links in the merged OAB MCP Adapter ADR; no dangling references remain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(mcp): note that mcp.audit must be named in RUST_LOG or auditing is silently off `mcp.audit` is a bare tracing target, not under the `openab` prefix, so the filter the deployment docs and our own fleet use — `RUST_LOG=openab=debug,openab_agent=debug` — matches none of the audit events and drops every audit line. Nothing indicates that auditing is disabled, so a deployment can believe it has a tool-call audit trail and have none. Found while verifying a live facade dispatch: the call demonstrably executed (the tool result reached the agent) with zero audit output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: warn that a leftover mode entry silently bypasses the facade Each transport writer only adds its own mcp.json entry — facade writes `openab`, bridge writes `openab-browser` — and neither removes the other's. An agent that has run in both modes therefore loads both servers, exposing the same `katashiro.*` tools twice: once through the facade (policy + audit) and once straight through the old transport (neither). The model calls the direct one and the call leaves no audit trail at all, while appearing to work perfectly. Corrects the evidence in 1c1919ce: that commit attributed the missing audit lines to `RUST_LOG` alone, having assumed the observed call was a facade dispatch. It was not — a stale bridge entry was carrying it. The RUST_LOG note there is still correct and still required; it was simply not the reason auditing looked dead in that instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: run the acp-mcp core and acp root tests `cargo test --workspace` builds with default features, so two sets of tests this PR added never compiled in CI: openab-core's `acp-mcp`-gated mcp_proxy tests and the root package's `acp`-gated browser_source / browser_bridge tests. 67 tests between them — including the capability source's routing and trust-gate coverage — so the fixes they back could not gate a merge. Add two steps mirroring the existing acp-gateway one. The core step is filtered to `mcp_proxy::` on purpose. `acp-mcp` gates exactly one module, so the filter loses no coverage, and an unfiltered `-p openab-core` would pull in hooks::tests — the parallel flake the gateway step's comment already documents avoiding. Co-Authored-By: Claude <noreply@anthropic.com> * fix(acp): do not open tunnels after a rejected session/resume The read loop derived its own channel_id from the requested sessionId and used that as the condition for spawning tunnels. A well-formed `sess_<uuid>` derives successfully on all four of the handler's rejection paths — missing sessionId, malformed sessionId, per-connection cap, busy — so the guard was not checking what it appeared to check. Combined with last-write-wins same-name re-attach, a refused resume could evict the live tunnel it had just been refused in favour of: a client mid-prompt (busy) or over the cap would knock out the browser control of the session that legitimately held it. handle_session_resume now returns (JsonRpcResponse, Option<String>), handing back the channel only when the resume actually succeeded, and the loop spawns only on that Some. This mirrors handle_session_new's (resp, channel_id) and deletes the independent derivation rather than adding a second check beside it — leaving the derive in place would keep a misleading guard available for reuse. Regression coverage asserts a None channel on each of the four rejections. The over-cap and busy cases are the load-bearing ones: their sessionIds are well formed and their sessions really exist, so the old guard produced a channel and spawned. Co-Authored-By: Claude <noreply@anthropic.com> * fix(acp): do not mint a facade session token when its config write fails When write_facade_mcp_config failed, the code warned and carried on: it minted OPENAB_SESSION_TOKEN and spawned the agent anyway. The agent then had no `openab` entry, so it could not reach the facade at all, while a live credential stayed registered for that channel until eviction — and the only trace was a warning. Mint only when the write succeeded. The session still starts; it simply has no browser capabilities, which is the honest description of what happened. The failure is logged at ERROR, and with no token there is no revoke guard to arm. Two alternatives were considered and rejected. Aborting session setup lets a config-write failure kill an otherwise working agent, and browser control is one capability among many. Falling back to a direct transport is worse than it looks: proxy and bridge write into the same workdir, so a failure there is likely to repeat, and silently switching to a direct entry re-creates the facade-bypass this PR's other fix exists to remove. Extracted setup_facade_session so the invariant is testable — the pool's tests are pure-function units and driving the real path spawns an agent. A counting registrar proves mint is never called when the write fails, forced by making <workdir>/.cursor a file so create_dir_all errors. Co-Authored-By: Claude <noreply@anthropic.com> * fix(mcp): retire the direct browser transport when facade mode sets up The facade writer added its `openab` entry but left any previous `openab-browser` entry in place, so both loaded and the model could take the direct path — reaching the browser without passing through facade policy and audit. Observed live 2026-07-26 and until now only documented. Remove the stale entry from all three places the direct transports wrote: .cursor/mcp.json, .kiro/settings/mcp.json, and the kiro per-agent files. For the agent files the `@openab-browser` grant goes too — `allowedTools` is default deny, so a leftover grant is what keeps the bypass reachable even once the server entry is gone; removing one without the other is a half fix. Ownership is decided by exact shape, never by the key. `openab-browser` is not proof we wrote it, and an operator may have configured their own server there. Only the two shapes we ever wrote are removable: the bridge entry {command:"openab",args:["browser-bridge"]}, and the per-session proxy entry — a loopback http://127.0.0.1:<port>/mcp url carrying a bearer header. A remote url, a bearer-less loopback, a different command or an empty port are treated as operator-owned and preserved verbatim. The matcher deliberately errs toward under-removal: a leftover entry only preserves the bypass, while deleting an operator's configuration destroys work. Tests cover shape recognition against five foreign shapes, removal alongside untouched user servers and unrelated top-level keys, a foreign `openab-browser` preserved verbatim, and the agent-file case where @github survives while @openab-browser goes. Co-Authored-By: Claude <noreply@anthropic.com> * fix(acp): keep the 8 MiB frame allowance to tunnel results only Raising MAX_FRAME_BYTES to 8 MiB for browser tool results also raised it for every other inbound frame, so one connection could hold MAX_INFLIGHT_PROMPTS (32) x 8 MiB of prompt text — the ~256 MiB worst case the review flagged. Bound the raise to the traffic it was for. Browser results arrive as client RESPONSES to our server-initiated `mcp/message` requests — id present, no `method` — so responses keep the 8 MiB ceiling, while every method-bearing frame (session/prompt included) is held to MAX_NON_TUNNEL_FRAME_BYTES, the pre-existing 1 MiB. That is what removes the exposure: the worst case came from prompts, which are method-bearing. Note this is deliberately not a `method == "mcp/message"` test, even though that is the obvious reading. `mcp/message` is only ever sent outbound; there is no inbound frame carrying that method, so matching on it would cap the screenshot responses at 1 MiB and break the case the raise exists for. The 8 MiB check stays pre-parse and still closes the connection: an oversized frame cannot be parsed back to its id, so no response can be fabricated for it. The per-kind check runs after parsing, where the id is available — oversized requests get ACP_OVERLOADED with their id, and oversized notifications are dropped without a reply, since answering a notification is a protocol violation. The unbounded outbound channel is untouched and remains a documented follow-up inherited from #1418 F6; this change bounds only what this PR added. Co-Authored-By: Claude <noreply@anthropic.com> * fix(mcp): authenticate the bridge connection, not the frame The unix socket authenticated nothing. 0600 proves the peer shares our uid, but says nothing about which session it belongs to, and the channel_id in each frame is a value the caller picks — so any same-uid process could connect and drive another live session's browser. Derive the channel server-side instead. The shim already walked its own /proc ancestry for OPENAB_BROWSER_CHANNEL; that logic was right but ran on the wrong side, because a caller can always lie about its own answer. The server now takes the peer pid from SO_PEERCRED and runs the same walk itself, so the peer cannot choose. A connection whose channel cannot be established is refused outright rather than given a default session. Frames may still carry channel_id — the shim sends it — but it is only ever compared against the authenticated value, never used to select a session; a mismatch is dropped and logged. The ancestry helpers move from the shim into openab-core, next to the server that now authenticates with them, so there is one implementation rather than two that can drift. The shim delegates to it and its frame value is advisory. serve_browser_socket keeps its signature; serve_browser_socket_with_resolver takes an injectable peer->channel mapping because a test binary's ancestry carries no channel, so the real resolver would refuse every test connection. The regression test proves a frame naming another session gets no reply at all while the next legitimate frame is answered. Co-Authored-By: Claude <noreply@anthropic.com> * fix(mcp): revoke facade session tokens by token, not by channel Session lifetimes overlap. `mint` replaces whatever token a channel holds, so a replaced session's drop guard runs after its successor has already minted — and the guard revoked by channel, which removed the live token. The new agent lost facade access wit…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After image build, bot creates a chart bump PR for human approval. Merging triggers release.yml.