Skip to content

[FEATURE] Add shared loopback HTTP transport - #7

Open
shllg wants to merge 3 commits into
thomaswitt:mainfrom
shllg:feat/mcp-http-daemon
Open

[FEATURE] Add shared loopback HTTP transport#7
shllg wants to merge 3 commits into
thomaswitt:mainfrom
shllg:feat/mcp-http-daemon

Conversation

@shllg

@shllg shllg commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Blocked by:

Merge order: #5#6#7. PRs #5 and #6 must be merged and closed
before this PR.

Summary

  • Add an opt-in authenticated Streamable HTTP transport for the wrapper-owned
    codex, claude, and gemini providers.
  • Keep stdio as the default and preserve the raw browser and frozen
    codex-legacy transport boundaries.
  • Create a fresh MCP server for every HTTP request so initialized legacy and
    request-contained 2026-07-28 exchanges remain independent.
  • Pool Codex App Server runtimes by canonical project root, with
    promise-deduplicated startup and safe idle reaping.
  • Add a http-auth-headers helper for Claude Code's dynamic Authorization
    header.
  • Document the stdio/HTTP lifecycle and resource trade-offs, authenticated
    Claude configuration, and Linux/macOS user services.

The branch is directly stacked on PR #6, which is stacked on PR #5. Because
the head branches live in a fork and I do not have upstream push permission,
this PR targets main; its diff will include both dependencies until they
merge.

Transport and lifecycle

Provider stdio HTTP Shared HTTP runtime
codex unchanged default opt-in one App Server per canonical project root
claude unchanged default opt-in process-wide job/runtime state
gemini unchanged default opt-in process-wide backend state
browser unchanged raw proxy rejected n/a
codex-legacy unchanged frozen proxy rejected n/a

Codex runtimes remain alive while an MCP request, foreground turn,
interaction, startup, or nonterminal background job is active. Once a runtime
becomes evictable, it receives the full idle grace period before shutdown.
Concurrent first requests for the same canonical root share one startup
promise; symlink aliases resolve to the same pool key.

Local security boundary

  • Listen only on 127.0.0.1.
  • Require a 32-byte bearer token on every /mcp request.
  • Store the generated token in a current-user-owned regular file with mode
    0600 under a current-user-owned private directory.
  • Reject unsafe token files, shared token directories, non-local Host headers,
    and cross-origin requests.
  • Require X-Mcp-Agents-Project-Root for Codex HTTP requests and canonicalize
    it before runtime lookup.

Verification

  • Feature-specific HTTP suite: 12 passed, 0 failed across auth, Host/Origin
    validation, modern and legacy HTTP, canonical-root pooling, idle reaping,
    active-work retention, and shutdown.
  • Claude Code 2.1.252 connected through headersHelper, reported the HTTP
    server connected, negotiated the modern era, and called ping, returning
    pong.
  • A real pooled Codex App Server call returned HTTP_POOL_OK; a second client
    reused App Server generation 1.
  • Ten-client cold discovery benchmark: stdio used 10 adapter processes /
    1444.6 MiB RSS / 7471.7 ms; HTTP used 1 process / 140.6 MiB RSS / 3248.3 ms.
  • README verification: all 6 JSON snippets parsed and the launchd plist parsed
    as valid XML.
  • node --check server.js, bash -n test.sh, git diff --check,
    npm ls --depth=0, and npm pack --dry-run --json: passed.
  • Full local SKIP_INTEGRATION=1 ./test.sh: 390 passed, 1
    deadline-sensitive legacy timing failure. Every HTTP test and the final
    child-leak check passed; the failed legacy case passed when rerun in
    isolation.

- Compare stdio and HTTP lifecycle and resource trade-offs
- Document authenticated systemd and launchd setup

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

It introduces a potentially flaky test due to a hard-coded HTTP port and contains a README statement that no longer matches the updated readiness log output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an opt-in authenticated loopback Streamable HTTP transport (alongside the existing stdio default) for wrapper-owned providers, and updates the wrapper to MCP v2-era APIs to support both legacy and 2026-07-28 “modern” exchanges per request/connection.

Changes:

  • Add --transport http mode with bearer-token auth, loopback Host/Origin validation, and a http-auth-headers helper command for clients that support dynamic headers (e.g., Claude Code).
  • Pool Codex App Server runtimes by canonical project root for HTTP requests, including idle reaping and safe shutdown draining.
  • Expand the fast test suite to cover HTTP auth/security, runtime pooling/eviction, and modern-era negotiation/foreground continuation behavior.
File summaries
File Description
test.sh Adds modern stdio negotiation tests, HTTP daemon contract tests, Codex HTTP pooling/eviction tests, and closed-stderr shutdown regression coverage.
server.js Implements authenticated loopback HTTP transport, token management, Codex runtime pooling for HTTP, and MCP v2 server/stdio handling with modern-era support.
README.md Documents transport modes and configuration for running a shared HTTP daemon and dynamic auth header helper usage.
package.json Replaces the v1 SDK dependency with MCP v2 @modelcontextprotocol/server/node and adds @modelcontextprotocol/client for tests.
package-lock.json Updates lockfile to reflect the MCP v2 dependency graph changes.
CHANGELOG.md Adds unreleased entries describing HTTP transport support, modern negotiation, and the stderr-EPIPE shutdown fix.
Review details
  • Files reviewed: 5/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test.sh
Comment on lines +711 to +721
local tmpdir shared_dir stderr_output status mode

echo "--- $label ---"
tmpdir=$(mktemp -d)
shared_dir="$tmpdir/shared"
mkdir "$shared_dir"
chmod 0755 "$shared_dir"
set +e
stderr_output=$($TIMEOUT_CMD 2 $SERVER --provider gemini \
--transport http --http-port 18766 \
--http-token-file "$shared_dir/bearer-token" 2>&1 >/dev/null)
Comment thread README.md
Comment on lines +165 to +167
The bridge writes `[mcp-agents] ready (provider: <name>)` to stderr when
listening; stdout remains MCP-only. Use the Quickstart configuration above if
you prefer this mode.
@thomaswitt

Copy link
Copy Markdown
Owner

Review: 5 blockers — please hold

The security boundary here is genuinely well built, and I want to lead with that because I tested it rather than read it. Against a running daemon:

Probe Result
No / wrong / same-length-off-by-one token 401 each
Host: evil.com (DNS rebind) · Origin: http://evil.com 403 each
Valid token, path /admin 404
Socket binding 127.0.0.1 only — LAN IP 192.168.2.10 refused
Token file / dir -rw------- / drwx------, 64 hex chars, correct owner
SIGTERM clean, zero orphans

timingSafeEqual with a length pre-check, randomBytes(32), openSync(…, "wx", 0o600), and the SDK's own localhostHostValidation/localhostOriginValidation are all the right primitives. http-auth-headers works and emits clean JSON. HTTP is genuinely opt-in behind an explicit flag, and browser / codex-legacy are correctly rejected at parse time.

The problems aren't in the security boundary. They're all one root cause:

stdio's per-client process was doing invisible garbage collection. Orphaned turns, unreleased leases, connection-scoped job records and abandoned children were all reclaimed for free when the bridge exited at EOF. This PR removes that escape hatch without replacing it.

Line numbers are from this branch's tree.

🔴 Blocker — one App Server child exit pins the pooled runtime forever

server.js:9069

canEvict() requires activeTurns.size === 0. But when the child exits mid-turn, onGenerationGone (7531–7551) sets turn.state = "outcome_unknown", turn.safeToRelease = false, clears the timers and rejects the turn — and never calls forgetTurn. The handler finally clauses release only if (turn.terminal || turn.safeToRelease), and grep shows the trap:

  • turn.terminal = true — only at 6912, inside finishTurn, not reached on this path
  • safeToRelease — only ever assigned false (7534). Never true.

So that turn is in activeTurns permanently, canEvict() is permanently false, and:

  1. reap() skips that pool entry forever; its sidecar, owner heartbeat and retention timers live for the daemon's lifetime.
  2. The next request for that root spawns a new codex app-server child on the same never-evictable runtime — which then also never dies.
  3. Every later codex-reply / codex-review / goal / fork / archive on that thread fails permanently with codex_thread_busy, because the orphaned lease names the still-running daemon PID and processExists() returns true.

Triggers aren't exotic: any child crash or OOM, the deliberate killChildGroup on a >10 MiB frame (7716), the noise-line kill (7571–7575), and the turn/start returned no turn ID path (8062).

Under stdio this self-healed when the per-client bridge exited. The daemon removes that escape hatch, so the feature meant to reduce process count strands one codex app-server per crashed child, permanently.

Suggested fix: give outcome_unknown turns a bounded life — either move them out of activeTurns into a bounded uncertainTurns map that codex-peek still reports from but canEvict() ignores, or exclude turn.uncertain from the canEvict() count. The durable sidecar already records the state for codex-peek, so nothing observable is lost.

🔴 Blocker — the per-request abort signal interrupts the very turn input_required is preserving

server.js:7910

awaitTurn attaches onAbort → interruptTurn(turn, "MCP request canceled") to ctx.mcpReq.signal and removes it in a finally that only runs when turn.completion settles. When the interactionReady branch wins the race, that listener stays registered, while awaitForegroundResult deliberately preserves the turn and returns inputRequired(...).

Over HTTP that's fatal. createMcpHandler builds a fresh per-request Server on a single-exchange PerRequestHTTPServerTransport; send() calls settleResponse(...) then queueMicrotask(() => this.close()), and close()Protocol._onclose() aborts every controller still in _requestHandlerAbortControllers. That microtask runs before _onrequest's .finally() deletes the controller — so the signal for the request that just returned input_required is aborted with ConnectionClosed, before the client can possibly answer.

interruptTurn isn't guarded against waiting_for_input: it flips the turn to canceling, sends turn/interrupt, and arms cancelSettleTimer to reject after cancelGraceMs (30s). Every foreground approval or question over HTTP destroys the turn it was meant to preserve. When round 2 arrives the caller gets codex_turn_interrupted, or -32602 "Invalid or expired…" if the grace already elapsed.

Secondary: owner.json / active-turns.json then publish canceling for a turn that is actually waiting on the user, contradicting the AGENTS.md liveness contract.

This is untested — the only input_required test runs over stdio, and none of the seven test_codex_http_case cases exercise a foreground interaction.

Suggested fix: detach the abort listener when the round yields an interaction, rather than when the turn completes — e.g. give awaitTurn an explicit detach() that awaitForegroundRound calls in its finally whenever it returns {kind: "interaction"}.

🔴 Blocker — background job records become process-global; codex-peek hands any client another connection's jobId

server.js:6088, 8656

AGENTS.md is explicit: "Background job records remain connection-local."

const jobs = new Map() lives at 6088 inside createCodexRuntime. buildCodexMcpServer is defined in that same closure and handed out as buildMcpServer (9066). The HTTP factory does pool.acquire(projectRoot)runtime.buildMcpServer(era) — so the MCP server is fresh per request, but jobs (and interactions, and activeTurns) are shared by every HTTP request for that project root. Under stdio each client was its own OS process, so the isolation was free.

Failure: client A starts a background job. Client B calls codex-peek, receives A's jobId, and can then codex-status / codex-result / codex-cancel it. Same for codex-interactions / codex-interaction-resolve (8974) — B can read and answer A's Codex prompts.

I realise a single shared bearer token means all clients are equally authenticated, so this may be intended. But if so it needs to be stated, because it silently inverts a documented invariant — and codex-cancel across clients is a footgun regardless of trust.

Suggested fix: stamp each job and interaction with an owning identity at creation (the MCP session id the transport already tracks, or an explicit client-id header alongside X-Mcp-Agents-Project-Root) and scope codex-peek / -status / -result / -commentary / -cancel / -interactions / -interaction-resolve to that owner. At minimum, exclude foreground interactions from the background queue tools.

🟠 Majors

  • server.js:576readHttpBearerToken skips the directory ownership/permission check that the create path enforces, and validates by lstat then reads by path (TOCTOU). A pre-existing token file under a world-writable directory is accepted. Fix: share one preflight between both paths, and validate the descriptor (openSyncfstatSync) rather than the path.
  • server.js:9073 — idle reaping destroys completed background-job results ~50 minutes before the documented one-hour retention. Fix: have canEvict() also require every terminal job to be past expiresAt or already read.

🔵 Minors

  • No request-body size cap on the authenticated transport (server.js:703); a 200 MB POST takes the daemon from 94 MB to 1.5 GB RSS.
  • The 8-active / 32-retained job caps are now shared across all clients (server.js:8440).
  • Durable state is keyed by the client-supplied project-root header rather than sha256(canonical STARTUP_CWD), which AGENTS.md specifies (server.js:5942).
  • test_http_token_parent_safety hard-codes --http-port 18766 and will fail nondeterministically if that port is busy — use an ephemeral port like the other HTTP tests. (Copilot flagged this one.)

What's genuinely good

The pool's mechanics are careful: a rejected startup promise is evicted from the map (no poisoned cache), the entry.closing retry loop correctly handles the reap race, evictableSince gives a full grace period, and the reaper is unref()'d. canEvict() covers exactly the conditions the description claims. The test suite is thorough and includes real negative tests (HTTP daemon rejects unauthorized and cross-origin requests, HTTP token creation never chmods an existing shared parent, Codex HTTP retains runtimes while background jobs are active). SKIP_INTEGRATION=1 ./test.sh gave me 391 passed, 0 failed, and npm pack produces a working 5-file package.

Method

Multi-agent review across 10 dimensions, each finding then checked by three independent skeptics prompted to refute it (32 raw → 19 survived; everything above was upheld unanimously unless noted). I independently reproduced or statically verified three of the blockers rather than take them on trust.

Also — please rebase once #6 lands; #5 is merged to main now.

@thomaswitt

Copy link
Copy Markdown
Owner

Follow-up: independent Codex review converged on the same defects, plus two new ones

I ran a second, fully independent review through OpenAI Codex — fresh session, read-only, given the tree and the intent but not my findings, so it read everything cold. It landed on the same five defects, at the same functions. Given they were found twice by different means, I'd treat them as settled rather than debatable.

My finding Codex's independent finding Codex severity
7910 abort signal interrupts the preserved turn 7935 / 7911 — same defect CRITICAL
7927 unhandled rejection kills the bridge 7921 / 6967 / 9848 — same defect CRITICAL
6088 / 8656 cross-client jobs & interactions 6083 — same defect CRITICAL
9069 runtime permanently non-evictable 7531 / 9067 — same defect CRITICAL
9830 keepAlive not cleared on transport close 9100 / 9818 — same defect CRITICAL

Codex's overall call, in its own words:

"I would not merge the stack yet. PR #5 fixes the reported recursive-stderr CPU spin, but PRs #6 and #7 introduce several independent ways to strand the bridge or App Server children. The HTTP foreground interaction flow is also broken on a normal request lifecycle."

Note it rates the keepAlive/transport-close item critical where I had it as major. Having reproduced it (a >10 MiB frame leaves an immortal process where main exits cleanly), I'm persuaded by Codex's grading and I'm upgrading it — leftover processes are the specific thing this stack exists to fix.

Two findings I missed

1. HTTP disconnect never cancels a blocking Claude/Gemini callserver.js:9573 / 9712

ctx.mcpReq.signal is plumbed into every Codex path (7951, 7967, 8122, 8732, 8840) and into waitForStatus (2596), but it never reaches runCli — which is declared runCli(command, args, opts = {}) at 1661 with no abort plumbing at all.

Failure: a client makes a blocking claude_code or gemini call over HTTP and disconnects (tab closed, client restarted, network blip). The child keeps running to its full timeout — 15 minutes for Claude, 5 for Gemini. Repeated disconnects accumulate children directly. Under stdio this was survivable because the bridge process died with the client; the daemon makes each one a 15-minute orphan.

I'd rate this higher than Codex's P2, precisely because "many processes alive" is the complaint this PR is meant to answer.

2. The runtime pool is unbounded, and --http-runtime-idle-timeout 0 disables reaping entirelyserver.js:9143 / 9192

reap() returns early when idleTimeoutMs === 0, and the pool Map is keyed by the client-supplied X-Mcp-Agents-Project-Root header with no daemon-wide cap on runtimes or spawned App Server children. Worth at least a documented maximum, and worth reconsidering whether 0 should mean "never reap" on a long-lived daemon.

Reframing the cross-client finding — this one is Thomas's call, not a bug report

I owe you a correction on how I framed the job-visibility blocker. Codex pointed me at test.sh (~3300–3315), and it asserts the behaviour deliberately: starter opens on project-a, starts a background job, closes; observer then opens on project-link (symlink, same canonical root), calls codex-status with the starter's jobId, asserts state === "running", and cancels it. The error string is "shared job was not running" — so this is intentional and encoded, not an accident.

That changes the finding but doesn't remove it. As written, the PR knowingly contradicts the AGENTS.md invariant "Background job records remain connection-local" without amending AGENTS.md, the CHANGELOG, or the README. So the question isn't "is this a bug" — it's "is this the intended semantics for the daemon, and if so, should the invariant be rewritten?" That's a maintainer decision. @thomaswitt should rule on it; if the answer is yes, the invariant and the docs need to move with it, and codex-steer / codex-cancel reaching across clients should be called out explicitly since those are write operations.

What Codex independently confirmed as sound

Worth recording, since it clears several things I had flagged only for scrutiny:

  • Continuation tokens are signed, expire, and carry only wrapper IDs plus a prompt hash — no prompts, questions, answers, command output or native App Server request IDs leak into the token.
  • Interaction settlement is synchronously single-use; no double-resolution race.
  • Pool startup dedup works, and a rejected startup promise is evicted rather than cached forever.
  • The closing gate correctly prevents acquisition from racing an in-progress eviction.
  • Runtime refcounting uses finally, so ordinary handler exceptions do release the lease.
  • HTTP token generation, permissions, loopback binding, Host/Origin validation, and the browser / codex-legacy HTTP rejection are all correct.
  • codex-legacy stays isolated; no raw App Server frames reach MCP stdout.

Codex could not run test.sh (its read-only sandbox blocks the suite's initial mktemp), so it verified the abort-signal claim with a direct no-write probe against the installed v2 SDK instead.

@thomaswitt

Copy link
Copy Markdown
Owner

Two further findings from a second independent Codex run

I ran Codex a second time in a separate session (different thread, again read-only, again given no findings from me). It reproduced everything above independently, and surfaced two defects that neither the first Codex run nor my own review caught. Flagging them because one of them undermines a core assumption of the pooling design.

🔴 The project-root header does not constrain where the turn actually runs

server.js:9282 (routing) vs server.js:7970 (execution)

These are two entirely independent client-supplied inputs:

  • X-Mcp-Agents-Project-RootcanonicalizeHttpProjectRoot()pool.acquire(projectRoot). This selects the runtime and the durable state directory, which is keyed sha256(canonicalProjectCwd).
  • args.cwdworkspace = { cwd: args.cwd, sandbox: args.sandbox }. This is where the turn actually executes.

The only validation applied to args.cwd is assertWorkspaceOutsideState(workspace?.cwd), which merely checks the workspace isn't inside the state directory. There is no equality check, no containment check, and canonicalizeHttpProjectRoot() never sees args.cwd at all.

Failure: a request routed under project A executes in project B, while its sessions, leases, bridge sidecars and interactions are all stored under A's state hash. A later request correctly routed under B cannot find that state. And combined with the cross-client visibility above, clients sharing A's runtime can observe and control work that is really running in B.

Codex rated this Medium. I'd put it with the blockers: it means the pool key is a routing hint rather than an isolation boundary, which is the opposite of what "pool by canonical project root" implies — and it's the assumption the whole HTTP design rests on.

Suggested fix: require args.cwd to equal, or be contained within, the canonicalised project-root header, and reject the call otherwise. codex-reply inherits its workspace from the thread, so it should be validated against the same root.

🔵 callHash is an unkeyed digest, so continuation tokens aren't quite content-independent

server.js:5957

The PR describes request state as content-free, and that is almost right — no raw prompts, questions, answers, command output or native App Server request IDs are embedded, which both Codex runs confirmed. But:

const fingerprintToolCall = (toolName, args) => createHash("sha256")
  .update(`${toolName}\n${canonicalJson(args)}`)
  .digest("hex");

That's a plain SHA-256 over the tool name and the complete arguments, including the prompt — and the signed request-state body is readable rather than encrypted. So the token is a commitment to its own arguments: anyone holding it can test candidate prompts offline and confirm a match. For a low-entropy prompt that's a real disclosure channel.

Suggested fix: you already mint a per-runtime randomBytes(32) key for the codec — deriving callHash as an HMAC under that key preserves the binding exactly while closing the offline-guessing channel. One-line change, no protocol impact.


For context on how much scrutiny this has now had: this PR has been through my multi-agent review (10 dimensions, 3 adversarial verifiers per finding), two independent Codex sessions, and a GitHub Copilot review. The fourth pass still found new defects. That isn't a criticism of the work — the security boundary and the pool mechanics really are carefully built — but it's a fair signal of how much surface area the HTTP transport adds, and an argument for landing it in smaller pieces if that's practical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants