Skip to content

fix(mcp): serve the facade stateless so a stale session never 404s - #1578

Merged
thepagent merged 4 commits into
openabdev:mainfrom
rockexe0000:fix/facade-stateless
Oct 27, 2026
Merged

thepagent merged 4 commits into
openabdev:mainfrom
rockexe0000:fix/facade-stateless

Conversation

@rockexe0000

@rockexe0000 rockexe0000 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this solve?

The OAB MCP Facade serves Streamable HTTP with rmcp's LocalSessionManager::default(), whose SessionConfig::DEFAULT_KEEP_ALIVE retires a session after 5 idle minutes (and every session on restart). The first facade call after an idle gap therefore carries a retired Mcp-Session-Id and gets 404 Not Found: Session not found — that request is not executed.

Claude Code (observed on 2.1.296) then runs two recovery paths at once: (a) its transport closes and starts a background auto-reconnect, and (b) the tool-call layer retries the call once on a fresh session. The retry does run downstream, but (a)'s next reconnect attempt (scheduled ~1000 ms later) closes the connection the retry is using, so the in-flight call fails with Connection closed, is surfaced as McpSessionExpiredError, and the agent sees MCP server "oab-facade" session expired.

Net effect: any facade call whose downstream work takes longer than ~1 s (most GitHub writes) lands, but is reported as failed. Agents that retry then duplicate non-idempotent writes (e.g. add_issue_comment), or wrongly conclude nothing happened. We hit it on 4/4 first-calls-after-idle in one deployment (idle gaps 43 min – 19 h; every error at 1.05–1.07 s after the call started).

Closes # (no upstream issue — searched issues/PRs for session expired / Session not found / stateless / Mcp-Session-Id / facade idle; none found)

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1564158355285745737

Review Contract

Goal

A facade call must never be answered with 404 Session not found because of idle time or a facade restart, so MCP clients never enter their session-recovery path for it.

Non-goals

  • Fixing Claude Code's recovery race itself (client bug; to be reported to the client separately).
  • Implementing the MCP 2026-07-28 stateless protocol (server/discover, per-request _meta capabilities) — that depends on rmcp support.
  • Making the facade's transport mode configurable — nothing in the facade needs a session (see Why).

Accepted Residual Risks

  • Client compatibility: the wire changes (no Mcp-Session-Id from initialize; GET/DELETE /mcp → 405) are all explicitly allowed by the Streamable HTTP spec, so compliant clients must accept them. A client that requires a session id or treats GET 405 as fatal would break. Verified with Claude Code (against a mock server) and end-to-end with Antigravity CLI agy 1.2.13 (real facade, PR preview image); rmcp-based clients accept it by default (allow_stateless: true, GET 405 → ServerDoesNotSupportSse); Kiro and others should get a smoke test (connect + one call).
  • No SSE resumability: a broken response stream loses the result (no Last-Event-ID resume). On a loopback listener this only happens on process restart, where stateful sessions were lost anyway; the 2026-07-28 spec removes resumability too.
  • notifications/cancelled is now dropped by rmcp instead of cancelling the request's token. No behavior change in practice: the facade's call_tool never observed that token, so a client cancel did not stop the downstream call before either; per-server timeouts in the runtime still apply.
  • Mid-call connection drops (e.g. facade restart during a call) can still trigger the client race; frequency drops from every idle gap to rare.

Acceptance Criteria

  • cargo clippy --workspace -- -D warnings and cargo test --workspace pass.
  • New test http_stateless_issues_no_session_and_never_404s_a_stale_one: initialize returns no Mcp-Session-Id; a tools/call carrying a stale session id returns 200 with a result; GET /mcp returns 405.
  • Existing http_e2e_session_token_gates_source_visibility still proves the per-request bearer gates session-bound sources (now without an initialize/session step).
  • Manual: with Claude Code, a facade call after > 5 min idle and a call after a facade restart both return a normal result and execute exactly once.

Follow-ups

  • Report the Claude Code retry-vs-background-reconnect race upstream.
  • Adopt the MCP 2026-07-28 protocol (server/discover, subscriptions/listen) once rmcp supports it.

At a Glance

Before (stateful; rmcp keep_alive 5 min idle)
  CLI --tools/call [Mcp-Session-Id: S1, retired]--> facade --> 404 Session not found (not executed)
  CLI --(Claude Code) reconnect + retry on S2-----> facade --> downstream   (write LANDS)
  CLI <-- own background reconnect closes S2 @~1s --- result dropped --> "session expired"

After (stateless; NeverSessionManager, stateful_mode = false)
  CLI --tools/call [no / any Mcp-Session-Id]------> facade --> downstream --> result (once)

Prior Art & Industry Research

Not applicable for OpenClaw / Hermes Agent — this is a transport-mode bug fix inside openab's own facade, not an architectural change. Relevant references instead:

Proposed Solution

build_router now builds StreamableHttpService with NeverSessionManager and StreamableHttpServerConfig::default().with_stateful_mode(false). Responses stay SSE (json_response unchanged) to keep the change to session behavior only. rmcp's stateless path still injects the HTTP Parts into request extensions, so session_ctx_from_extensions (the Authorization: Bearer identity) works unchanged. No config keys, public function signatures, or tool contracts change.

Tests: the e2e session-token test drops the initialize → session-id step (a bare tools/call is the whole exchange); a POST helper mcp_post is shared; a new regression test covers no-session-issued / stale-id-served / GET 405. docs/oab-mcp-facade.md gains a "Stateless transport" section.

Why this approach?

The facade needs nothing a session provides: identity is per-request, the two tools are plain request/response, it advertises no listChanged, sends no progress, and never issues server→client requests (sampling/elicitation/roots). The session only added a state that can expire. Stateless removes the trigger for both idle expiry and restarts, in a two-line change, and aligns with where the spec is going.

Alternatives Considered

  • Raise SessionConfig.keep_alive (e.g. 24 h, or configurable): same size, but still 404s after a facade restart or a longer idle gap, and leaks sessions of clients that exit without DELETE.
  • Stay stateful and add an in-memory SessionStore (transparent restore): rmcp deletes a stored session only on a client DELETE, not on idle expiry, so with a store an idle-retired session id is restored in place (rmcp replays initialize) and the request is served instead of 404'd. This is the zero-wire-change option: session ids, the standalone GET stream and DELETE keep working, so even a client that requires Mcp-Session-Id is unaffected. Costs: a small SessionStore impl with a size/TTL bound (clients often exit without DELETE), and an in-memory store does not survive a facade restart (fine when the facade and the CLI restart together, as in a pod; a standalone facade would need a persistent store). If maintainers prefer no wire change, this PR can switch to this approach.
  • Client-side fix only: out of our control and leaves every other client exposed to the same 404 path.

Validation

  • cargo check / cargo clippy --workspace -- -D warnings (+ unified) / cargo test --workspace — green in this PR's CI (CI, CI openab-agent); not run locally (the authoring environment has no Rust toolchain). Both Docker Smoke Test workflows are green as well.
  • rmcp 1.8.0 source review: stateless branch skips all session lookups, injects Parts, returns 405 for GET/DELETE; NeverSessionManager + with_stateful_mode exist and are public; on the stateful path a notifications/cancelled only cancels the request token (the handler is not aborted).
  • Manual repro with Claude Code 2.1.296 against a mock Streamable HTTP server:
    • stateful, 4 s idle expiry, tool 200 ms → 404, transparent recovery, executed once, success.
    • stateful, tool 1 s / 3 s → 404, retry executed, client closed the stream at ~1 s, agent got session expired (1043 / 1047 ms).
    • stateless, after 10 s idle and after a server restart → success, executed once, no reconnect; server/discover probe got -32601 and the client fell back to the 2025 protocol; GET 405 handled silently.
  • E2E with Antigravity CLI (agy 1.2.13) on this PR’s preview image (pr1-antigravity) against the real facade + downstream: discovery and a read call OK; after an 11-minute idle gap a ~3 s call returned normally — no session expired, no MCP reconnect.

🤖 Generated with Claude Code

The facade used rmcp's LocalSessionManager with default config, which retires a
session after 5 idle minutes (and all of them on restart). The client's next
call then got `404 Session not found`. Claude Code recovers by retrying the call
on a fresh session, but its own background reconnect closes that connection
about a second later: the retried call runs downstream (a write lands) while the
agent is told `MCP server "oab-facade" session expired`. Retrying by hand then
duplicates non-idempotent writes.

Nothing in the facade needs a protocol-level session: identity is the
per-request `Authorization` bearer (rmcp still injects the HTTP Parts in
stateless mode), the two tools are plain request/response, and it never pushes
notifications or sends server->client requests. Serve it stateless
(NeverSessionManager + stateful_mode = false): no Mcp-Session-Id is issued, a
stale one is ignored instead of 404'd, GET/DELETE return 405. This also matches
the MCP 2026-07-28 revision, which removes protocol-level sessions.

- e2e session-token test: drop the initialize -> session-id step; a bare
  tools/call with or without the bearer is the whole exchange now.
- New regression test: initialize issues no session id, a stale session id is
  still served (200, not 404), GET is 405.
- docs/oab-mcp-facade.md: document the stateless transport and why.

Agent-Bot: alpha
Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
@openab-app openab-app Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Oct 25, 2026
@openab-app openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Oct 26, 2026
@rockexe0000 rockexe0000 reopened this Oct 26, 2026
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

- F1: gmail adapter doc no longer claims to mirror the facade transport; it
  states that gmail is still stateful (same idle/restart 404 exposure) and what
  must be shared before it can go stateless. Also correct "one instance per
  serve process" for HTTP mode.
- F2: scope the 2026-07-28 claim to the removal of protocol-level sessions (the
  facade still speaks the 2025 initialize handshake), in the build_router doc
  and docs/oab-mcp-facade.md.
- F3: ADR 6.2 gains a "Stateless transport" bullet (gmail remains stateful);
  the operator guide states that a stale Mcp-Session-Id is ignored rather than
  rejected, and the Trust model section now says "agent session" for the
  bearer-backed session so it is not confused with the MCP protocol session;
  the e2e test doc says "session-token (bearer) mechanism".
- F4: the regression test checks the initialize body (result + protocolVersion,
  no error) and asserts DELETE /mcp -> 405 alongside GET, both with Allow: POST.
- From the earlier non-blocking notes: document that future facade tools must
  not rely on server->client requests or the GET SSE stream, and note why
  NeverSessionManager is intentionally redundant with stateful_mode = false.

Agent-Bot: alpha
Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
@rockexe0000

Copy link
Copy Markdown
Contributor Author

Thanks for the review — round 1 is addressed in 174e566:

  • F1: gmail serve_http doc now states it is still stateful (same idle/restart exposure) and what must be shared before it can go stateless; GmailNative doc corrected for HTTP mode.
  • F2: 2026-07-28 claim scoped to the removal of protocol-level sessions (code doc + operator guide).
  • F3: ADR §6.2 gains a "Stateless transport" bullet; the guide states a stale Mcp-Session-Id is ignored, not rejected; Trust model now says "agent session" (bearer) vs the MCP protocol session; test doc renamed to "session-token (bearer) mechanism".
  • F4: regression test checks the initialize body (result + protocolVersion, no error) and asserts DELETE /mcp → 405 alongside GET, both with Allow: POST.
    Also documented the constraint that future facade tools must not rely on server→client requests or the GET SSE stream, and why NeverSessionManager is intentionally redundant with with_stateful_mode(false).
    Gmail stateless is planned as the first follow-up.

@chaodu-agent

This comment has been minimized.

- F1: finish the "session" disambiguation: ADR 6.2 Security posture and the
  operator guide's Trust model now say "agent session" / "per-agent-session
  bearer" / "agent-session-free" wherever they mean the bearer-backed session.
- Non-blocking notes taken:
  - move the misplaced serve_http_with description back above serve_http_with
    (it had run into the build_router rustdoc);
  - trim the build_router rustdoc to the architectural reason and point to the
    operator guide for the client-side failure narrative (fixes the "and all of
    them on restart" wording on the way);
  - extend the future-tool constraint: a tool that needs client cancellation
    must wire the request cancellation token, since stateless mode drops
    notifications/cancelled (code doc + guide);
  - regression test: a notification-only POST is 202, and the tools/call half
    asserts "id":1 and no "isError":true.

Agent-Bot: alpha
Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
…ion test

Peer review of the round-2 changes:
- The future-tool constraint said a tool needing client cancellation "must wire
  RequestContext.ct". That cannot work stateless: rmcp drops
  notifications/cancelled on the stateless POST path and each POST runs its own
  one-shot service, so the token never fires on a client cancel. State that
  cancellation is unavailable and that needing it (like server->client requests
  or the GET stream) means revisiting the choice (code doc + guide).
- Regression test: assert "isError":false positively instead of the absence of
  "isError":true; bound the SSE body reads with a 10 s timeout so a stream that
  never ends fails instead of hanging; doc comment lists all checks.

Agent-Bot: alpha
Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
@rockexe0000

Copy link
Copy Markdown
Contributor Author

Round 2 addressed in e8b1575 + c2741f3:

  • F1: ADR §6.2 Security posture and the guide's Trust model now say "agent session" / "per-agent-session bearer" / "agent-session-free" at all five spots.
  • From the non-blocking notes: moved the misplaced serve_http_with rustdoc back above it; trimmed the build_router rustdoc to the architectural reason with a pointer to the guide (fixes the "all of them on restart" wording); the future-tool constraint now also states that client cancellation is unavailable (stateless mode drops notifications/cancelled, so RequestContext.ct never fires on a client cancel) — code doc + guide.
  • Regression test: notification-only POST → 202; tools/call asserts "id":1 and "isError":false; SSE body reads are bounded by a timeout so a never-ending stream fails instead of hanging.
  • CI: the fork runs of these exact commits are green (fix(mcp): address review round 1 on the stateless facade rockexe0000/openab#2 for 174e566, fix: use app bot identity for chart bump commits #3 for the round-2 commits); the upstream workflows are awaiting maintainer approval.

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Note

LGTM ✅ - The round-2 wording finding is fixed at every flagged location, the round-3 delta is docs, rustdoc, and test hardening only, and no runtime, auth, or performance issue was found. Two merge preconditions remain: upstream CI must run green at this head, and a maintainer must run one kiro-cli smoke call against the facade.

What This PR Does

The OAB MCP Facade served Streamable HTTP with rmcp's LocalSessionManager. That manager retires a session after 5 idle minutes and drops every session on restart, so the next call got 404 Session not found. That 404 pushed Claude Code into a recovery race: the retried call ran downstream (the write landed), but the agent was told session expired. This PR serves the facade statelessly (NeverSessionManager + with_stateful_mode(false)), so no protocol-level session exists to expire. The PR body considers and rejects three alternatives: raising keep_alive, an in-memory SessionStore, and a client-only fix.

Round 3 reviews c2741f3, which addresses round 2 in e8b1575 and c2741f3. The production transport configuration is the same as in rounds 1 and 2.

How It Works

  • build_router builds StreamableHttpService with NeverSessionManager and StreamableHttpServerConfig::default().with_stateful_mode(false) (facade.rs:504-527). Responses stay SSE.
  • Checked against the rmcp 1.8.0 crate source, the stateless POST path does the following:
    • never consults Mcp-Session-Id, so a stale id is ignored, not rejected;
    • still injects the request Parts, so the per-request Authorization bearer still resolves identity;
    • still validates MCP-Protocol-Version;
    • answers notification-only POSTs with 202;
    • answers GET/DELETE /mcp with 405 + Allow: POST.
  • Each POST builds a fresh McpFacade from shallow Arc clones of McpRuntimeManager, sources, and SessionTokens. No reqwest::Client, connection pool, OAuth manager, background task, or downstream connection is created per request.
  • Round 3 delta (174e566..c2741f3, 3 files, +56/-28):
    • completes the agent-session vs. MCP-protocol-session wording in ADR 6.2 and the operator guide;
    • moves the misplaced serve_http_with rustdoc back above that function;
    • trims the build_router rustdoc to the architectural reason and points to the guide for the full story;
    • documents that client cancellation is unavailable in stateless mode;
    • hardens the regression test: SSE bodies are read under a 10 s timeout; a notification-only POST must return 202; tools/call must return "id":1 and "isError":false.

Findings

# Severity Finding Location
F1 🟢 Round-2 F1 resolved: every flagged "session" occurrence now says "agent session", and the guide adds a definition line before the Trust model text docs/adr/oab-mcp-adapter.md:433-437, docs/oab-mcp-facade.md:157-164
F2 🟢 The rustdoc now sits on the right functions, and the build_router doc no longer duplicates the client-race narrative from the guide facade.rs:480-503, facade.rs:530-534
F3 🟢 The cancellation constraint is accurate: rmcp 1.8.0 tower.rs ignores client notifications on the stateless path, so RequestContext.ct never fires on a client cancel. The new 202 assertion exercises exactly that path facade.rs:500-503, docs/oab-mcp-facade.md:150-153
F4 🟢 Test hardening: a bounded SSE read turns a never-ending stream into a failure instead of a hung test; the 202, "id":1, and "isError":false assertions close the gaps noted in rounds 1 and 2 facade.rs:763-858
F5 🟢 Auth boundary unchanged: identity is still the per-request bearer; a stale or forged Mcp-Session-Id grants nothing; error paths still go through redact_secrets; no dependency, feature-flag, config, or public API change facade.rs:432-445, facade.rs:504-527
F6 🟢 Operational profile: restart and upgrade are transparent to clients that still hold an old session id; rollback is a two-line revert back to existing behavior; no config migration; no session map or idle reaper facade.rs:521-525
F7 🟢 Scope discipline: the Gmail adapter stays stateful on purpose, and the comment states the actual prerequisite (share reqwest::Client and the refresh lock outside the per-session factory) gmail.rs:103, gmail.rs:706-722
Finding Details

🟢 F1: Wording disambiguation complete

All five locations requested in round 2 were changed: docs/adr/oab-mcp-adapter.md:433, 435, 436, 437 and docs/oab-mcp-facade.md:163. Line 164 of the guide was also fixed (resolves to an agent session), and a definition line was added at docs/oab-mcp-facade.md:157.

Session-bound sources (ADR line 434, guide line 163) was left unchanged, and that is correct. It is an established term: it appears 13 times in facade.rs, 3 times in sources.rs, and in six other docs. Renaming it in one ADR bullet would break consistency with the code. The surrounding sentences already say "agent session".

🟢 F2: Rustdoc placement and de-duplication

The serve_http_with description used to run straight into the build_router doc comment, so each function showed the wrong text. Both are now attached correctly. The full idle -> 404 -> recovery-race narrative now lives only in docs/oab-mcp-facade.md ("Stateless transport"). The code keeps the architectural reason and the constraint for future tools.

🟢 F3: Cancellation constraint verified

In rmcp 1.8.0 transport/streamable_http_server/tower.rs, the stateless branch handles ClientJsonRpcMessage::Notification(_) by ignoring it and returning 202 Accepted. There is no session to route notifications/cancelled to an in-flight request, so RequestContext.ct never fires on a client cancel. The rustdoc says RequestContext.ct, which suits Rust readers. The guide says "a tool's request cancellation token", which suits operators. Both describe the same mechanism. This is not a regression: on the stateful path, call_tool never read the token either.

🟢 F4: Regression test hardening

  • sse_body (facade.rs:769-775) wraps collect() in a 10 s tokio::time::timeout, so a regression that leaves the SSE stream open fails the test instead of hanging CI.
  • notifications/initialized -> 202 covers the notification-only POST path.
  • tools/call now asserts "id":1, "result", unavailable, and "isError":false. In rmcp 1.8.0, CallToolResult::success sets is_error: Some(false) and the field is skip_serializing_if = "Option::is_none", so "isError":false is always serialized today. (See non-blocking suggestion 1 about future-proofing.)

🟢 F5: Auth boundary unchanged

  • Mcp-Session-Id was never an identity input. Ignoring a stale id introduces no session fixation, replay, or privilege path.
  • A bare tools/call without initialize is now served. That grants nothing extra, because session-bound sources are still gated by the bearer.
  • 405 + Allow: POST on GET/DELETE does not reveal whether any session exists.
  • No new dependency: NeverSessionManager is already in the enabled rmcp 1.8.0 features.

🟢 F6: Operational profile

Item Result
Upgrade (client holds a pre-upgrade session id) Served; the stateless path never checks the header
Rollback Revert two lines; clients simply re-initialize, same as any restart today
Config migration None; the behavior is fixed in build_router
Resource profile No session map, idle reaper, or abandoned-session residency; per-POST cost is a few Arc clones plus a short-lived task
Fail-loud guard NeverSessionManager is deliberately redundant: if stateful mode is flipped back on, initialize fails instead of silently re-enabling sessions

🟢 F7: Gmail kept stateful with an accurate prerequisite

GmailNative::new builds a fresh reqwest::Client and refresh Mutex per instance. In stateless mode its factory would run once per POST, which would lose connection pooling and refresh serialization. Keeping Gmail stateful in this PR, with the prerequisite written down, is the right cut.

Merge preconditions (not code findings)
  1. Upstream CI must be green at c2741f3. At this head, only Review Contract and PR Discussion URL Check have run upstream. CI (cargo check / clippy -D warnings / cargo test --workspace), CI openab-agent, and both Docker Smoke Test workflows are waiting for maintainer approval (fork workflows). The author reports that the fork run of these commits is green (fix(mcp): address review round 2 on the stateless facade rockexe0000/openab#3). We did not verify that independently, and the review environment has no Rust toolchain. The new and modified HTTP-level tests run under cargo test --workspace.
  2. Manual kiro-cli smoke (maintainer): kiro-cli completes one tools/call against the stateless facade. initialize must be accepted without Mcp-Session-Id, and GET /mcp -> 405 must not be treated as fatal. CI does not cover this: the Docker smoke workflows only verify binaries and versions (plus an ACP stdio handshake for some agents) and never start the MCP facade. An idle-gap variant is not needed, because in stateless mode the server holds no state that can expire. Per the 2025-06-18 Streamable HTTP spec, compliant clients must accept a missing session id. The PR already lists client compatibility as an accepted residual risk.
Non-blocking suggestions
  1. "isError":false assertion (facade.rs:837): this ties the test to rmcp's compact serialization and to the Some(false) default in CallToolResult::success. It is correct for rmcp 1.8.0. Asserting that the body does not contain "isError":true would keep the error check and survive an rmcp change that omits a false value.
  2. Unbounded body read in the other e2e test (facade.rs:710): http_e2e_session_token_gates_source_visibility still uses resp.into_body().collect() without a timeout. This is pre-existing. Consider moving sse_body to module scope and reusing it there.
  3. ADR 6.2 "Stateless transport" bullet: the rustdoc and the guide both state the cost of the stateless choice (no server-to-client requests, no GET SSE stream, no client cancellation), but the ADR bullet only says "no Mcp-Session-Id". One sentence there would make the ADR a self-contained decision record.
  4. Startup visibility: a single tracing::info! in serve_http_with naming the transport mode (stateless) would help operators confirm the running mode. Optional, because a regression back to stateful already fails loudly at initialize.
  5. Duplication: the cancellation constraint appears in both the rustdoc and the guide. Keeping it next to the code is defensible, since it is a constraint on future tools. Alternatively, keep one line in the rustdoc and point to the guide, as was done for the race narrative.
  6. Test name: http_stateless_issues_no_session_and_never_404s_a_stale_one now also covers 202 and the body checks. The doc comment covers this; a broader name or a split into smaller tests would make failures easier to diagnose.
Follow-ups (out of scope for this PR)
  1. Gmail HTTP transport -> stateless (recommended first follow-up; please open an issue). It has the same idle/restart 404 race, and sending mail is a non-idempotent write. Prerequisite: hoist reqwest::Client and the refresh lock into shared Arc state outside the factory.
  2. Client cancellation gap: stateless mode drops notifications/cancelled (verified against rmcp 1.8.0 tower.rs). This matters if a facade tool ever grows beyond short request/response. Related but distinct: streamable-http-server: client TCP disconnect does not cancel in-flight tool futures (RequestContext::ct never fires) modelcontextprotocol/rust-sdk#857 (a TCP disconnect does not cancel RequestContext::ct) was closed as completed without a linked fix, and its behavior in 1.8.0 is unverified.
  3. Facade coverage in Docker smoke tests: add a facade tools/call step to docker-smoke-test-unified.yml, at least for kiro-cli, so precondition 2 stops being manual.
  4. Idempotency for non-idempotent execute_capability writes: a mid-call connection drop can still produce "write landed, reported as failed".
  5. Report the Claude Code retry-vs-background-reconnect race upstream (already listed in the PR).

5. Three Reasons We Might Not Need This PR

  1. "A zero-wire-change option exists." Stateful + SessionStore keeps session ids, the GET stream, and DELETE. Rebuttal: it adds state, TTL, and size bookkeeping for a session nothing uses; an in-memory store still does not survive a restart; and it invests in a mechanism the MCP 2026-07-28 revision removes.
  2. "Stateless may have to be reverted." A future tool that needs sampling, elicitation, roots, the GET SSE stream, or client cancellation would force a return to sessions. Rebuttal: the constraint is now documented at the seam and in the guide, reverting is two lines, and the 2026 protocol redesigns those flows without sessions.
  3. "Round 3 adds no runtime value, and client compatibility is unproven in CI." Rebuttal: round 3 was meant to close review findings, not change runtime behavior. Client compatibility is allowed by the spec, verified with Claude Code and agy, and gated here by a manual kiro-cli precondition, with a CI follow-up.

Net: the PR is necessary, minimal, and well-documented. It can merge once the two preconditions above are met.

Addressing Earlier Feedback

chaodu-agent (round 2, 174e566)

F1: "Session" wording only half-disambiguated in ADR 6.2 Security posture and the operator guide.

✅ Addressed in e8b1575: all five requested locations now say "agent session" / "per-agent-session bearer" / "agent-session-free". Guide line 164 is fixed as well, and a definition line was added at guide line 157. Session-bound sources is kept as the established codebase term (see F1).

Non-blocking: misplaced serve_http_with rustdoc; rationale duplicated in the rustdoc; "all of them on restart" grammar; cancellation note; test hardening (202, "id":1, no isError:true).

✅ Addressed in e8b1575 / c2741f3: the rustdoc is moved and trimmed (the grammar fix came with the trim); the cancellation constraint is documented in code and guide; the 202, "id":1, and "isError":false assertions are added, and SSE reads are bounded by a timeout. ℹ️ Not taken (optional): ADR section 8 entry for the rejected SessionStore option, and shortening the Gmail comment once a follow-up issue exists.

Merge precondition: approve the workflows and confirm CI is green.

ℹ️ Still open: upstream workflows are still awaiting approval at c2741f3 (see Merge preconditions). Correction to rounds 1 and 2: those rounds cited green smoke-test-unified runs, including kiro-cli. Those workflows never start the MCP facade, so they are not evidence of facade client compatibility. That is why precondition 2 is a manual check.

@rockexe0000 (round-2 response)

Round 2 addressed in e8b1575 + c2741f3 ... CI: the fork runs of these exact commits are green (rockexe0000#2, #3); the upstream workflows are awaiting maintainer approval.

Thanks. Every item was checked against the files at c2741f3 and is confirmed above. The fork CI result is noted, but upstream CI remains the merge gate.

No inline review threads or submitted reviews exist on this PR.

Baseline Check
  • PR opened: 2026-10-25, by rockexe0000; base main (default branch, not stacked)
  • Head: c2741f399c20821913e3ef9dea773cfc421a85d2; merge-base: 20830d5dfb24cff96599001832164152a22bbb71
  • Diff: 4 files, +216 / -56 (crates/openab-mcp/src/mcp/facade.rs, crates/openab-mcp/src/native/gmail.rs, docs/adr/oab-mcp-adapter.md, docs/oab-mcp-facade.md)
  • Round-3 delta 174e566..c2741f3: 3 files, +56 / -28; rustdoc, docs, and tests only; git diff --check clean
  • Main already has: per-request bearer identity (SessionTokens / SessionCtx), loopback enforcement, stateful Streamable HTTP via LocalSessionManager
  • Net-new value: a stateless facade transport, a regression test for the stale-session path, and docs
  • CI at head: Review Contract and PR Discussion URL Check green; CI, CI openab-agent, Docker Smoke Test, Docker Smoke Test (Unified) awaiting maintainer approval
What's Good (🟢)
  • Every round-2 item, blocking and non-blocking, was fixed precisely, with no scope creep.
  • The root-cause analysis is reproducible, with measured timings, and the rationale now lives in one place (the guide), with a pointer from the code.
  • Residual risks (cancellation, resumability, mid-call drops, client compatibility) are stated honestly and match the rmcp source.
  • The production change removes state instead of adding retry or TTL machinery.

@thepagent
thepagent merged commit d59f7de into openabdev:main Oct 27, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants