morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #5491] [CLOSED] fix(mcp): refuse a one-element initialize batch without evicting the session #6936

New issue

Closed

opened 2026-11-01 21:44:11 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:44:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5491
Author: @Yashasm18
Created: 10/20/2026
Status: ❌ Closed

Base: main ← Head: fix/5445-initialize-batch-session

📝 Commits (1)

caf612d fix(mcp): refuse a one-element initialize batch without evicting the session

📊 Changes

2 files changed (+34 additions, -29 deletions)

View changed files

📝 src/server/http/serveStreamableHttp.test.ts (+31 -24)
📝 src/server/http/serveStreamableHttp.ts (+3 -5)

📄 Description

Summary

Fixes #5445

In HTTP mode, a sessionless POST with a one-element array holding initialize was treated like a lone initialize. The server closed the live session before dispatching the request, and a fresh transport then accepted the array. The original client got 404 Session not found on its next call. The docs say only a lone initialize can replace an open session, and that a batch carrying one is left to the transport's refusal. This PR makes the code follow that rule.

Problem and expected behavior

Here's the sequence: a client opens session A, then someone sends [{"jsonrpc":"2.0","id":2,"method":"initialize",...}] without a session header. On main, that request gets 200 and a new session ID, and A is gone. The existing preservation test only covered a two-element batch, so the suite passed while the one-element case was broken.

After this change, the one-element batch gets a 400, no replacement session ID is issued, and A keeps working. A lone initialize object can still replace the session, as documented.

Change and scope

The change is in isInitializePost in src/server/http/serveStreamableHttp.ts. That function decides whether a sessionless POST is allowed to evict the live session. It used to unwrap a one-element array and check the item inside. Now it only returns true when the body itself is an initialize request. Any array falls through to the live transport, which already refuses it with 400 Server already initialized. That's the same path the two-element batch already took.

Nothing else changes. Header checks, the body size bound, the abort check and the replacement flow for a lone initialize are untouched. Scope decision: not applicable, this is a regular fix inside existing boundaries.

Contract and boundary impact

Semantic owner and earliest changed stage: isInitializePost, the session replacement decision in the HTTP session slot, before transport dispatch.
Contribution scope fit and linked maintainer decision: bug fix within existing boundaries, not applicable.
CLI and MCP/tool-catalog contract: HTTP transport behavior now matches docs/mcp-contracts.md. No tool schemas change.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details: the live session is no longer closed for this request shape. Lone initialize replacement and its cleanup are unchanged.
Generated metadata, package, or installation impact: none.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

I turned the existing batch preservation test in src/server/http/serveStreamableHttp.test.ts into a parameterized test. It now covers both the one-element batch and the two-element batch, and it also checks that no mcp-session-id header comes back. On main, the new one-element case fails with expected 200 to be greater than or equal to 400, and the two-element case passes. With the fix, both pass. The existing test where a second and third client replace the session with a lone initialize still passes, so the documented replacement behavior is still covered.

I also ran the steps from the issue against the built CLI (rea mcp --http --host 127.0.0.1 --port 0), with isolated HOME and XDG directories.

Before, on main:

lone initialize A: 200
A initialized: 202
A tool before batch: 200
sessionless one-element initialize batch: 200 new session issued
A tool after batch: 404 {"jsonrpc":"2.0","error":{"code":-32001,"message":"Session not found"},"id":null}
control, lone initialize replaces A: 200 new session issued
A tool after lone replacement: 404

After, with this change:

lone initialize A: 200
A initialized: 202
A tool before batch: 200
sessionless one-element initialize batch: 400 no session issued
A tool after batch: 200
control, lone initialize replaces A: 200 new session issued
A tool after lone replacement: 404

The main gap is that I only ran this on macOS. The change is platform independent, and CI covers the other platforms.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

All on macOS arm64 with Node v22.23.1:

npm run test:local -- src/server/http/serveStreamableHttp.test.ts: 63 passed. Before the fix, the new one-element case failed as shown above.
npm run check:changed: static checks passed, and 78 affected tests passed.
npm run test:focused on the HTTP boundary suites (streamableHttpMcp, mcpHttpOptions, environmentServeMcp, promptMcp, mainErrors, dispatcher, mcpHttpShutdown, environmentServeShutdown): 8 files, 39 tests passed.
npm run check: passed, with 0 lint errors and the architecture guards passing.
The built CLI reproduction above, run before and after the change.

Compatibility, safety, and release

Breaking changes or migration steps: none. A one-element initialize batch was never documented as able to replace a session, and the docs already said it would be refused.
Real Hopper/Ghidra, browser, or OS coverage: no provider is involved. I only tested on macOS.
Package or release metadata impact: none.
Security, privacy, process, or containment review: this narrows when the live session can be closed. One malformed sessionless request can no longer disconnect the active client.

Author readiness

Human owner: @Yashasm18

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5491 **Author:** [@Yashasm18](https://github.com/Yashasm18) **Created:** 10/20/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5445-initialize-batch-session` --- ### 📝 Commits (1) - [`caf612d`](https://github.com/morluto/rea/commit/caf612d0ae5be0a3a277805c578e22f91ac56ec3) fix(mcp): refuse a one-element initialize batch without evicting the session ### 📊 Changes **2 files changed** (+34 additions, -29 deletions) <details> <summary>View changed files</summary> 📝 `src/server/http/serveStreamableHttp.test.ts` (+31 -24) 📝 `src/server/http/serveStreamableHttp.ts` (+3 -5) </details> ### 📄 Description ## Summary Fixes #5445 In HTTP mode, a sessionless POST with a one-element array holding `initialize` was treated like a lone `initialize`. The server closed the live session before dispatching the request, and a fresh transport then accepted the array. The original client got `404 Session not found` on its next call. The docs say only a lone `initialize` can replace an open session, and that a batch carrying one is left to the transport's refusal. This PR makes the code follow that rule. ## Problem and expected behavior Here's the sequence: a client opens session A, then someone sends `[{"jsonrpc":"2.0","id":2,"method":"initialize",...}]` without a session header. On main, that request gets `200` and a new session ID, and A is gone. The existing preservation test only covered a two-element batch, so the suite passed while the one-element case was broken. After this change, the one-element batch gets a `400`, no replacement session ID is issued, and A keeps working. A lone `initialize` object can still replace the session, as documented. ## Change and scope The change is in `isInitializePost` in `src/server/http/serveStreamableHttp.ts`. That function decides whether a sessionless POST is allowed to evict the live session. It used to unwrap a one-element array and check the item inside. Now it only returns true when the body itself is an `initialize` request. Any array falls through to the live transport, which already refuses it with `400 Server already initialized`. That's the same path the two-element batch already took. Nothing else changes. Header checks, the body size bound, the abort check and the replacement flow for a lone `initialize` are untouched. Scope decision: not applicable, this is a regular fix inside existing boundaries. ## Contract and boundary impact - Semantic owner and earliest changed stage: `isInitializePost`, the session replacement decision in the HTTP session slot, before transport dispatch. - Contribution scope fit and linked maintainer decision: bug fix within existing boundaries, not applicable. - CLI and MCP/tool-catalog contract: HTTP transport behavior now matches `docs/mcp-contracts.md`. No tool schemas change. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: the live session is no longer closed for this request shape. Lone `initialize` replacement and its cleanup are unchanged. - Generated metadata, package, or installation impact: none. Expected effects: - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage I turned the existing batch preservation test in `src/server/http/serveStreamableHttp.test.ts` into a parameterized test. It now covers both the one-element batch and the two-element batch, and it also checks that no `mcp-session-id` header comes back. On main, the new one-element case fails with `expected 200 to be greater than or equal to 400`, and the two-element case passes. With the fix, both pass. The existing test where a second and third client replace the session with a lone `initialize` still passes, so the documented replacement behavior is still covered. I also ran the steps from the issue against the built CLI (`rea mcp --http --host 127.0.0.1 --port 0`), with isolated HOME and XDG directories. Before, on main: ``` lone initialize A: 200 A initialized: 202 A tool before batch: 200 sessionless one-element initialize batch: 200 new session issued A tool after batch: 404 {"jsonrpc":"2.0","error":{"code":-32001,"message":"Session not found"},"id":null} control, lone initialize replaces A: 200 new session issued A tool after lone replacement: 404 ``` After, with this change: ``` lone initialize A: 200 A initialized: 202 A tool before batch: 200 sessionless one-element initialize batch: 400 no session issued A tool after batch: 200 control, lone initialize replaces A: 200 new session issued A tool after lone replacement: 404 ``` The main gap is that I only ran this on macOS. The change is platform independent, and CI covers the other platforms. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed All on macOS arm64 with Node v22.23.1: - `npm run test:local -- src/server/http/serveStreamableHttp.test.ts`: 63 passed. Before the fix, the new one-element case failed as shown above. - `npm run check:changed`: static checks passed, and 78 affected tests passed. - `npm run test:focused` on the HTTP boundary suites (`streamableHttpMcp`, `mcpHttpOptions`, `environmentServeMcp`, `promptMcp`, `mainErrors`, `dispatcher`, `mcpHttpShutdown`, `environmentServeShutdown`): 8 files, 39 tests passed. - `npm run check`: passed, with 0 lint errors and the architecture guards passing. - The built CLI reproduction above, run before and after the change. ## Compatibility, safety, and release - Breaking changes or migration steps: none. A one-element initialize batch was never documented as able to replace a session, and the docs already said it would be refused. - Real Hopper/Ghidra, browser, or OS coverage: no provider is involved. I only tested on macOS. - Package or release metadata impact: none. - Security, privacy, process, or containment review: this narrows when the live session can be closed. One malformed sessionless request can no longer disconnect the active client. ## Author readiness Human owner: @Yashasm18 - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:44:11 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:48:00 +00:00

[PR #6936] [MERGED] fix(http): preserve legal response header names #7458

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6936

No description provided.

Rows
Columns