Skip to content

feat(mcp): add Streamable HTTP transport - #599

Closed
ekinnee wants to merge 17 commits into
mnemosyne-oss:mainfrom
ekinnee:feat/streamable-http-mcp
Closed

ekinnee wants to merge 17 commits into
mnemosyne-oss:mainfrom
ekinnee:feat/streamable-http-mcp

Conversation

@ekinnee

@ekinnee ekinnee commented Aug 1, 2026 •

Copy link
Copy Markdown

Description

Adds stateful MCP Streamable HTTP support at /mcp using the MCP SDK 2.x
session manager. The new transport is available through
mnemosyne mcp --transport streamable-http; stdio remains the default and the
legacy SSE transport remains available.

The implementation also shares bearer authentication across network MCP
transports, enables DNS-rebinding protection for loopback Streamable HTTP,
uses a bounded idle timeout, and unregisters sessions after explicit DELETE.
Documentation, CLI help, and regression coverage are included.

Related Issue

Closes #598

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor / performance
  • Test improvement
  • CI / build tooling

How Has This Been Tested?

  • Existing tests still pass (pytest tests/ -v)
  • New tests added for new functionality
  • Manual verification (MCP initialize/list/call/DELETE lifecycle, exact
    /mcp routing, bearer rejection, DNS-rebinding rejection, and session
    registry cleanup)

Focused validation:

  • pytest tests/test_streamable_http.py tests/test_tool_surface_parity.py -q:
    15 passed
  • Generated-doc parity, Python compilation, fatal Ruff gate, and
    git diff --check: passed

The previously completed CI-shaped serial run reported 2,381 passed and 2
skipped. A later rerun reproduced the known batch rollback/order-sensitive
failures and was interrupted during its slow tail. Parallel xdist execution
is not currently valid for this suite because concurrent module collection
shares SQLite initialization state and produces schema/collection races.

Checklist

  • Version bumped in mnemosyne/__init__.py
  • CHANGELOG.md updated with a brief entry
  • README updated because user-facing behavior changed
  • Code follows the project's principles:
    • No new external dependencies without good reason
    • No cloud / API key requirement for core functionality
    • SQLite is the only database dependency

Summary

  • Adds opt-in, stateful MCP Streamable HTTP support at /mcp.
  • Preserves stdio as the default and retains legacy SSE support.
  • Extends MCP integration for remote agents and Codex through authenticated HTTP.
  • Updates the CLI, documentation, tests, dependency bounds, Docker builds, and release metadata.

Architecture

  • The change does not modify working, episodic, or BEAM memory tiers.
  • The change does not modify retrieval, consolidation, veracity, sync, or benchmark systems.
  • MCP sessions support idle expiry and explicit DELETE cleanup.
  • MCP SDK dependencies are constrained to version 2.x.
  • The transport-specific design is the right call because it adds remote access without changing the core memory architecture.

Privacy and Local-First Guarantees

  • Non-loopback binds require bearer authentication.
  • Loopback Streamable HTTP uses DNS-rebinding protection.
  • Stdio remains the default and does not require a network service.
  • Network exposure remains opt-in.
  • Remote deployment requires HTTPS or a private tunnel.
  • The HTTP option adds a network exposure path, but it does not change the local-first default.

Agent Integration Surfaces

  • MCP gains a stateful /mcp Streamable HTTP endpoint.
  • The CLI supports stdio, sse, and streamable-http.
  • Codex documentation covers HTTPS, bearer tokens, and environment-based token configuration.
  • No Hermes integration changes are described.

Maintainability

  • Transport-specific startup and routing isolate network behavior from the core memory architecture.
  • Shared bearer-token handling reduces duplicated authentication logic.
  • _resolve_sse_auth remains as a compatibility wrapper.
  • Regression tests cover tool calls, authentication, DNS rebinding, unknown sessions, cleanup, idle expiry, and session isolation.
  • Documentation and generated configuration references use consistent network MCP authentication terminology.
  • Session cleanup limits resource retention for abandoned HTTP sessions.

@CLAassistant

CLAassistant commented Aug 1, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Streamable HTTP MCP transport is available at /mcp. It supports stateful sessions, authentication, DNS-rebinding protection, explicit termination, idle cleanup, CLI dispatch, documentation, and MCP SDK 2.x dependency bounds.

Changes

Streamable HTTP MCP transport

Layer / File(s) Summary
Shared network transport authentication
mnemosyne/mcp_server.py
Bearer-token authentication and shared Starlette middleware now support network MCP transports. SSE authentication remains available through a compatibility wrapper.
Stateful Streamable HTTP application
mnemosyne/mcp_server.py, tests/test_streamable_http.py
The server handles stateful /mcp GET, POST, and DELETE requests. Tests cover session lifecycle, cleanup, authentication, and DNS-rebinding protection.
Transport dispatch and CLI integration
mnemosyne/mcp_server.py, mnemosyne/cli.py
The CLI accepts stdio, sse, and streamable-http transports. Host and port help text covers network MCP transports.
Release and integration documentation
mnemosyne/__init__.py, CHANGELOG.md, README.md, docs/api/*, docs/cli-reference.md, docs/integrations/codex-mcp.md, scripts/generate-docs.py, pyproject.toml, Dockerfile
Version metadata, dependency bounds, container installation, and documentation describe Streamable HTTP setup and authentication.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Uvicorn
  participant StreamableHTTPApp
  participant MCPServerSessionManager
  Client->>Uvicorn: POST /mcp initialize
  Uvicorn->>StreamableHTTPApp: Forward authenticated request
  StreamableHTTPApp->>MCPServerSessionManager: Create MCP session
  MCPServerSessionManager-->>StreamableHTTPApp: Return session ID
  StreamableHTTPApp-->>Client: Return initialization response
  Client->>Uvicorn: POST /mcp with session ID
  Uvicorn->>StreamableHTTPApp: Route MCP request
  StreamableHTTPApp->>MCPServerSessionManager: Route request to session
  StreamableHTTPApp-->>Client: Return MCP response
  Client->>Uvicorn: DELETE /mcp with session ID
  Uvicorn->>StreamableHTTPApp: Forward termination request
  StreamableHTTPApp->>MCPServerSessionManager: Remove session
  StreamableHTTPApp-->>Client: Return termination response
Loading

Suggested reviewers: axdsan

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The Dockerfile now installs the local checkout instead of the published package, which changes image build behavior beyond the linked feature requirements. Move the Dockerfile change to a separate issue or document why local-checkout installation is required to deliver and validate this feature.
Docstring Coverage ⚠️ Warning Docstring coverage is 55.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: adding the MCP Streamable HTTP transport.
Linked Issues check ✅ Passed The changes satisfy #598 by adding stateful /mcp support, CLI selection, authentication, DNS-rebinding protection, cleanup, compatibility, documentation, and tests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ekinnee
ekinnee marked this pull request as ready for review August 1, 2026 14:09

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/test_streamable_http.py (1)

50-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Coverage is strong but has three gaps; add them together.

The lifecycle, auth, DELETE-cleanup, DNS-rebinding, and independent-session tests are solid and each asserts a concrete outcome. Per the comprehensive-review requirement for tests/**, here are the missing scenarios to add in one pass rather than incrementally:

  1. GET method on /mcp. The route accepts methods=["GET", "POST", "DELETE"] (mnemosyne/mcp_server.py Lines 330-334), but no test exercises the GET path (server-initiated SSE stream). Add a test that opens a GET stream on an initialized session and confirms it is accepted (not 405/404).
  2. Wrong (non-empty, invalid) bearer token for Streamable HTTP. test_streamable_http_non_loopback_rejects_missing_bearer_token (Lines 157-173) only covers a missing token. Add a case sending Authorization: Bearer wrong-token against a non-loopback app and assert the {"error": "invalid bearer token"} 401 path in _BearerTokenMiddleware (mnemosyne/mcp_server.py Lines 239-246) is actually reachable.
  3. Idle-timeout behavior. test_streamable_http_delete_removes_session_from_manager (Lines 109-133) only asserts manager.session_idle_timeout == 1800, a static config check, not that idle sessions actually expire. Add a test that builds the app with a very small session_idle_timeout (or monkeypatches the manager after construction) and asserts an idle session is reaped and subsequent requests return 404, matching the "bounded idle timeout" requirement from the PR objectives.

Per path instructions: "Flag missing or weak test scenarios. Verify that new tests actually assert something meaningful (not just "doesn't crash")."

As per path instructions for tests/**: "COMPREHENSIVE REVIEW REQUIRED IN A SINGLE PASS. Group ALL suggestions for related fixtures/test files together — never iterate one-by-one... Flag missing or weak test scenarios."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_streamable_http.py` around lines 50 - 211, Extend the Streamable
HTTP tests around _build_streamable_http_app with three concrete scenarios: open
a GET /mcp request for an initialized session and assert it is accepted; send an
incorrect non-empty bearer token to a non-loopback app and assert the 401
response with {"error": "invalid bearer token"}; and configure a very short
session_idle_timeout, verify an initialized idle session is reaped, then assert
a subsequent session request returns 404. Ensure each test exercises the
behavior rather than only checking construction or static configuration.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/cli-reference.md`:
- Around line 95-98: Update the `mcp` CLI entry in the command reference to
include the `[--host HOST]` option, and state that it defaults to `127.0.0.1`
alongside the existing token requirement for non-loopback network transports.

In `@docs/integrations/codex-mcp.md`:
- Around line 37-38: Update the non-loopback MCP startup example near the
mnemosyne command to explicitly assign a placeholder bearer token rather than
merely expanding MNEMOSYNE_MCP_TOKEN. Use a clearly non-secret documentation
value while preserving the existing host, port, and transport options.

In `@mnemosyne/mcp_server.py`:
- Around line 258-264: Wrap the Streamable HTTP imports in
_build_streamable_http_app with an ImportError handler matching _build_sse_app,
including mcp.server.streamable_http_manager, mcp.server.transport_security, and
starlette.routing. Convert missing or incompatible dependencies into the same
actionable RuntimeError with installation instructions, rather than allowing a
raw import failure; keep _run_streamable_http’s separate uvicorn import handling
unchanged.
- Around line 308-317: Update the DELETE cleanup in the request handler around
StreamableHTTPSessionManager to avoid unguarded reliance on the private
_server_instances and _session_owners attributes: pin mcp to a bounded range
such as >=2.0.0,<3, and make cleanup skip safely when either private attribute
is unavailable so exceptions cannot escape the finally block.

---

Outside diff comments:
In `@tests/test_streamable_http.py`:
- Around line 50-211: Extend the Streamable HTTP tests around
_build_streamable_http_app with three concrete scenarios: open a GET /mcp
request for an initialized session and assert it is accepted; send an incorrect
non-empty bearer token to a non-loopback app and assert the 401 response with
{"error": "invalid bearer token"}; and configure a very short
session_idle_timeout, verify an initialized idle session is reaped, then assert
a subsequent session request returns 404. Ensure each test exercises the
behavior rather than only checking construction or static configuration.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 77399a12-402d-4836-9179-eab05f51e19f

📥 Commits

Reviewing files that changed from the base of the PR and between 22c60e2 and 7bd6fa2.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • README.md
  • docs/api/configuration.mdx
  • docs/api/tool-schema.mdx
  • docs/cli-reference.md
  • docs/integrations/codex-mcp.md
  • mnemosyne/__init__.py
  • mnemosyne/cli.py
  • mnemosyne/mcp_server.py
  • scripts/generate-docs.py
  • tests/test_streamable_http.py

Comment thread docs/cli-reference.md Outdated
Comment thread docs/integrations/codex-mcp.md Outdated
Comment thread mnemosyne/mcp_server.py Outdated
Comment thread mnemosyne/mcp_server.py Outdated

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
mnemosyne/mcp_server.py (1)

343-356: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Wire lifespan through the normal Starlette constructor instead of mutating app.router.lifespan_context.

_build_authenticated_mcp_app currently returns Starlette(routes=routes, middleware=middleware) with no lifespan support, then this path overwrites the internal lifespan_context. Add an optional lifespan parameter and pass it to Starlette(...), or thread the context inside _build_authenticated_mcp_app, so the Streamable HTTP lifecycle is managed through the documented Starlette API.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mnemosyne/mcp_server.py` around lines 343 - 356, Update
_build_authenticated_mcp_app to accept an optional lifespan context and pass it
through the Starlette constructor, then provide lifespan when building the
Streamable HTTP app and remove the direct app.router.lifespan_context mutation.
Preserve existing behavior for callers that do not supply a lifespan.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_streamable_http.py`:
- Around line 204-227: Increase the polling window in
test_streamable_http_idle_session_is_reaped so CI scheduling delays do not cause
intermittent failures; retain the existing 404 assertion and session-id request
flow while using a longer retry budget or bounded wait for the eventual
expired-session response.

---

Outside diff comments:
In `@mnemosyne/mcp_server.py`:
- Around line 343-356: Update _build_authenticated_mcp_app to accept an optional
lifespan context and pass it through the Starlette constructor, then provide
lifespan when building the Streamable HTTP app and remove the direct
app.router.lifespan_context mutation. Preserve existing behavior for callers
that do not supply a lifespan.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: aeac2501-f509-48c2-bc33-4e0895c7603e

📥 Commits

Reviewing files that changed from the base of the PR and between 7bd6fa2 and bf63cb6.

📒 Files selected for processing (5)
  • docs/cli-reference.md
  • docs/integrations/codex-mcp.md
  • mnemosyne/mcp_server.py
  • pyproject.toml
  • tests/test_streamable_http.py

Comment thread tests/test_streamable_http.py

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_streamable_http.py`:
- Around line 214-225: Update the session cleanup test around
manager._server_instances and manager._session_owners to first assert that
session_id is registered in both registries. Poll until session_id has been
removed from both registries, then preserve the existing assertion and request
validation so the test cannot pass when registration never occurred.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dcc8b779-0fe6-438e-acc4-94bff77dc0f1

📥 Commits

Reviewing files that changed from the base of the PR and between bf63cb6 and dd84c7d.

📒 Files selected for processing (1)
  • tests/test_streamable_http.py

Comment thread tests/test_streamable_http.py

@coderabbitai coderabbitai Bot 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
tests/test_streamable_http.py (1)

235-254: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Complete the network-security test matrix.

Verify that invalid Host and invalid Origin use the correct statuses. MCP SDK 2.0.0 returns 421 for an invalid Host and 403 for an invalid Origin. If the test expects 421 for both, correct the assertion. (raw.githubusercontent.com)

The bearer tests cover only missing and invalid credentials. Add a valid-token initialization and follow-up request. Assert 200, mcp-session-id, and a successful session operation. Otherwise, middleware that rejects every token could still pass the tests.

As per path instructions: tests/** requires a “COMPREHENSIVE REVIEW REQUIRED IN A SINGLE PASS”, meaningful assertions, edge-case coverage, and coverage of the MCP tool surface.

Also applies to: 257-296

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_streamable_http.py` around lines 235 - 254, Expand
test_streamable_http_loopback_rejects_dns_rebinding_headers to separately verify
invalid Host returns 421 and invalid Origin returns 403, rather than asserting
one status for both headers. Extend the bearer-auth tests around the existing
missing and invalid credential cases with a valid-token initialization and
follow-up session operation, asserting status 200, the mcp-session-id header,
and successful MCP behavior; include meaningful edge-case coverage for the
exposed MCP tool surface as required.

Source: Path instructions

mnemosyne/mcp_server.py (2)

231-239: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Parse bearer tokens case-insensitively.

Authorization scheme names are case-insensitive, so bearer <token> and BEARER <token> are rejected by header.startswith("Bearer ") even with correct tokens. Compare bearer with casefold(), and feed the stripped credentials into hmac.compare_digest.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mnemosyne/mcp_server.py` around lines 231 - 239, Update the bearer-token
validation in the authorization handling block to recognize the Bearer scheme
case-insensitively using casefold(), while preserving the missing-token response
for other schemes. Strip the scheme and whitespace first, then pass the
resulting credentials to hmac.compare_digest against expected.

Source: Path instructions


277-292: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include bare loopback hosts with ports in Streamable HTTP security checks.

:* patterns only match values that include a port. Add exact bare entries for localhost, 127.0.0.1, [::1], and ip6-localhost alongside their :* entries. Include the same bare origins for http://localhost, http://127.0.0.1, http://[::1], and http://ip6-localhost so default-port Streamable HTTP requests are not rejected.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mnemosyne/mcp_server.py` around lines 277 - 292, Update the
TransportSecuritySettings construction in the loopback branch of the server
setup to add bare host entries alongside each existing `:*` allowed_hosts
pattern, and add the corresponding bare HTTP origins alongside the existing
port-pattern entries in allowed_origins for localhost, 127.0.0.1, [::1], and
ip6-localhost.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@mnemosyne/mcp_server.py`:
- Around line 231-239: Update the bearer-token validation in the authorization
handling block to recognize the Bearer scheme case-insensitively using
casefold(), while preserving the missing-token response for other schemes. Strip
the scheme and whitespace first, then pass the resulting credentials to
hmac.compare_digest against expected.
- Around line 277-292: Update the TransportSecuritySettings construction in the
loopback branch of the server setup to add bare host entries alongside each
existing `:*` allowed_hosts pattern, and add the corresponding bare HTTP origins
alongside the existing port-pattern entries in allowed_origins for localhost,
127.0.0.1, [::1], and ip6-localhost.

In `@tests/test_streamable_http.py`:
- Around line 235-254: Expand
test_streamable_http_loopback_rejects_dns_rebinding_headers to separately verify
invalid Host returns 421 and invalid Origin returns 403, rather than asserting
one status for both headers. Extend the bearer-auth tests around the existing
missing and invalid credential cases with a valid-token initialization and
follow-up session operation, asserting status 200, the mcp-session-id header,
and successful MCP behavior; include meaningful edge-case coverage for the
exposed MCP tool surface as required.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f0d91330-aaed-46db-805e-2abacd6e5264

📥 Commits

Reviewing files that changed from the base of the PR and between dd84c7d and a8bb43b.

📒 Files selected for processing (2)
  • mnemosyne/mcp_server.py
  • tests/test_streamable_http.py

@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ekinnee

ekinnee commented Aug 1, 2026

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Dockerfile`:
- Around line 31-34: Update the Dockerfile installation step for the checkout
copied to /opt/mnemosyne so the image installs the dependencies required by the
SSE transport, including starlette and uvicorn, while retaining the existing mcp
dependencies. Use the package’s established SSE extra if available; otherwise
add the required dependencies explicitly to the pip install command so mnemosyne
mcp --transport sse starts successfully.

In `@tests/test_streamable_http.py`:
- Around line 241-265: Add positive coverage alongside exercise for the expanded
loopback allowlist by parameterizing initialization requests with matching
unported Host and Origin values for localhost, [::1], and ip6-localhost. Send
each request through _streamable_http_client and assert successful status, while
preserving the existing rejected-host and rejected-origin assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 778077ef-acda-447b-b8b0-0825cd668ae9

📥 Commits

Reviewing files that changed from the base of the PR and between a8bb43b and e6a416e.

📒 Files selected for processing (3)
  • Dockerfile
  • mnemosyne/mcp_server.py
  • tests/test_streamable_http.py

Comment thread Dockerfile Outdated
Comment on lines +31 to +34
# Install this checkout with MCP + SSE extras so locally built images contain
# the source that was used to build them.
COPY . /opt/mnemosyne
RUN pip install --no-cache-dir "/opt/mnemosyne[mcp]"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

rg -n -C 8 'optional-dependencies|mcp|sse' pyproject.toml
rg -n -C 4 -- '--transport.*sse|transport.*sse' docker-compose.yml

Repository: mnemosyne-oss/mnemosyne

Length of output: 2508


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'Tracked Docker/Compose/config files:\n'
git ls-files | rg -n '(^|/)(Dockerfile|docker-compose\.ya?ml|pyproject\.toml|\.dockerignore)$' || true

printf '\nDockerfile excerpt:\n'
cat -n Dockerfile | sed -n '1,70p'

printf '\npyproject relevant dependency definitions:\n'
cat -n pyproject.toml | sed -n '1,90p'

Repository: mnemosyne-oss/mnemosyne

Length of output: 5575


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'Relevant source files:\n'
git ls-files | rg 'mnemosyne/(cli|integrations/mcp|mc)' || true

printf '\nSearch for SSE imports/usages in mnemosyne packages:\n'
rg -n -C 4 'sse|SSE|sse_transport|sse_server|transport' mnemosyne tests 2>/dev/null || true

printf '\nPyProject script/config candidates containing "mcp" or CLI:\n'
rg -n -C 3 'mnemosyne.*mcp|mcp.*mnemosyne|project\.scripts|entry_points|tool\.setuptools' pyproject.toml README.md docs README.rst 2>/dev/null || true

Repository: mnemosyne-oss/mnemosyne

Length of output: 50380


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'mnemosyne package source files (max 200):\n'
git ls-files 'mnemosyne/*' | head -n 200
git ls-files 'mnemosyne_integrations/*' | head -n 200

printf '\nTop-level scripts/entrypoint candidates:\n'
rg -n -C 4 '^(\[project\.scripts\]|[[:space:]]*mnemosyne.*=[[:space:]]*)' pyproject.toml
rg -n -C 8 'def mcp|class mcp|MCP|mcp' mnemosyne pyproject.toml README.md 2>/dev/null | head -n 220

printf '\nSSE-related imports/usages in mnemosyne source ONLY:\n'
rg -n -C 4 'sse|SSE|sse_transport|sse_server|Transport=' mnemosyne 2>/dev/null | head -n 240

printf '\nDocker context exclusions and dependency install comments:\n'
cat -n .dockerignore | sed -n '1,120p'

Repository: mnemosyne-oss/mnemosyne

Length of output: 31822


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'mnemosyne/mcp_server.py outline:\n'
ast-grep outline mnemosyne/mcp_server.py --view compact || true
printf '\n'
wc -l mnemosyne/mcp_server.py

printf '\nMCP server implementation excerpt:\n'
cat -n mnemosyne/mcp_server.py | sed -n '1,260p'

printf '\nmcp server CLI integration excerpt:\n'
cat -n mnemosyne/cli.py | sed -n '1780,1905p'

Repository: mnemosyne-oss/mnemosyne

Length of output: 12836


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

cat -n mnemosyne/cli.py | sed -n '1788,1828p'
cat -n mnemosyne/mcp_server.py | sed -n '260,420p'
python3 - <<'PY'
from pathlib import Path
p=Path('pyproject.toml')
s=p.read_text()
start=s.index('[project.optional-dependencies]')
end=s.find('\n[', start+1)
block=s[start:end].strip()
print(block)
print('\nmcp extra contains starlette or uvicorn:', 'starlette' in block or 'uvicorn' in block)
PY

Repository: mnemosyne-oss/mnemosyne

Length of output: 7783


Install the MCP SSE dependencies in the Docker image.

The mcp extra installs only mcp and anyio, while --transport sse requires starlette and uvicorn. Add those deps to the install command, or add an explicit sse extra that covers them, so the container can start with mnemosyne mcp --transport sse.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Dockerfile` around lines 31 - 34, Update the Dockerfile installation step for
the checkout copied to /opt/mnemosyne so the image installs the dependencies
required by the SSE transport, including starlette and uvicorn, while retaining
the existing mcp dependencies. Use the package’s established SSE extra if
available; otherwise add the required dependencies explicitly to the pip install
command so mnemosyne mcp --transport sse starts successfully.

Comment thread tests/test_streamable_http.py
@ekinnee
ekinnee force-pushed the feat/streamable-http-mcp branch from e6a416e to 3092f3b Compare August 2, 2026 02:06
@ekinnee
ekinnee requested review from AxDSan and dplush as code owners August 2, 2026 02:06
@dplush

dplush commented Aug 2, 2026 •

Copy link
Copy Markdown
Collaborator

Thanks for the thorough follow-up work. I need to revise my initial review before proceeding: an additional focused review surfaced concerns that require maintainer review through the appropriate channel.

Please hold the CI rerun and CodeRabbit follow-up for the moment. I will follow up through that channel before we continue the merge review.

@ekinnee
ekinnee force-pushed the feat/streamable-http-mcp branch from 596ec07 to e615d7c Compare August 5, 2026 20:39
@AxDSan

AxDSan commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the Streamable HTTP transport work. It's conflicting with main right now. Could you rebase your branch onto current main and push the refreshed head? Happy to review once it's clean.

@AxDSan

AxDSan commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

@ekinnee one more thing to fix while you're rebasing this. The .dockerignore doesn't exclude .env or .hermes/. The Dockerfile does COPY . /opt/mnemosyne, which copies the whole build context. If someone runs docker build from a checkout that has local secrets in .env or runtime state in .hermes/, those get baked into the image. Both are local secret/runtime paths that .gitignore excludes but this .dockerignore misses.

Please add both to .dockerignore before merge:

.env
.env.*
.hermes

That closes the leak. Thanks for the Streamable HTTP work, this is a solid feature. Happy to merge once it's rebased and this is tightened up.

@ekinnee
ekinnee force-pushed the feat/streamable-http-mcp branch from ea8c1fa to 2eb3272 Compare August 11, 2026 19:46

AxDSan commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

@ekinnee I owe you a decision and a straight explanation, three weeks late.

I am taking #749 as the canonical Streamable HTTP transport, and closing this as superseded. I want to be precise about why, because "superseded" on its own reads like a verdict on your work and it is not one.

You opened this on July 30. At that point the MCP SDK 2.x streamable_http_app was not a practical option, so you solved it with what existed: a transport route we own and maintain. #749 opened on August 14 and could use the SDK-native transport, reuse the existing pure-ASGI bearer middleware so SSE and HTTP resolve auth through one gate, and inherit the SDK's DNS-rebinding protection. Those are the three things that decided it, and two of the three were unavailable to you when you wrote this.

The reason two implementations exist at all is that I did not make this call for three weeks while both sat with zero review. That is the actual failure here, and it is mine.

What I am doing about it:

If you want to review #749 before it lands, I would genuinely value it. You have thought harder about this transport than anyone except its author, and you know where the rough edges are.

Closing this one. Nothing about it reflects badly on you, and I hope you send the next one.


Generated by Claude Code

@AxDSan AxDSan closed this Aug 22, 2026
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.

[FEATURE] Add Streamable HTTP MCP transport

4 participants