Repository navigation
feat: MCP Streamable HTTP transport (engram-server 0.2.0) - #9
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR releases version 0.2.0 of jamjet-engram, adding a Python ChangesEngram 0.2.0: MCP Streamable HTTP Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR introduces a cohesive new feature (HTTP MCP server with CLI) involving multiple independent modules (transport, auth, CLI, HTTP app integration) that must work together correctly. The changes are heterogeneous (new modules, API signature changes, Docker configuration, test fixtures) and require understanding the relationships between bearer-token auth, Streamable HTTP session management, CLI validation rules, and the asymmetric-auth model (MCP gated, REST open). However, the changes follow a clear logical narrative and are well-tested, reducing per-file reasoning overhead.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/engram/server/http.py (1)
131-146: ⚡ Quick winHardcoded version drift risk — derive from package metadata.
version="0.2.0"is duplicated in both branches and will silently desync frompyproject.tomlon the next bump. The CLI already pulls the version viaimportlib.metadata.version("jamjet-engram")for--version; doing the same here keeps a single source of truth and makes the OpenAPI//docspage accurate after a release bump.♻️ Proposed fix
+from importlib.metadata import PackageNotFoundError, version as _pkg_version + +try: + _APP_VERSION = _pkg_version("jamjet-engram") +except PackageNotFoundError: # editable installs without dist-info + _APP_VERSION = "0.0.0+local" @@ - app = FastAPI(title="Engram", version="0.2.0", lifespan=lifespan) + app = FastAPI(title="Engram", version=_APP_VERSION, lifespan=lifespan) @@ - app = FastAPI(title="Engram", version="0.2.0") + app = FastAPI(title="Engram", version=_APP_VERSION)🤖 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 `@src/engram/server/http.py` around lines 131 - 146, Replace the hardcoded version="0.2.0" used when constructing the FastAPI app with a single-source-of-truth value obtained from package metadata (e.g. importlib.metadata.version("jamjet-engram")) and reuse that variable in both branches where FastAPI(...) is created (the branch that sets lifespan and app.router.routes.append(Route("/mcp", endpoint=_MCPHandler())) and the else branch), so the OpenAPI/docs version always reflects the package version.src/engram/cli.py (1)
27-31: 💤 Low valueUse Typer's
is_eagercallback for--versioninstead of mid-handler raise.
_print_version()is invoked from insidemain, which means it runs after Typer has already parsed every other option (and would conflict if a future option grew side effects in its parser/callback). Typer's idiomatic pattern is anis_eager=Truecallback that prints and exits before the rest of the command body. Not strictly required — current code works — flagging as nice-to-have.🤖 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 `@src/engram/cli.py` around lines 27 - 31, Change the version handler to an eager Typer callback so it runs before the main command body: update the _print_version function signature to accept (value: bool, ctx: typer.Context) or (ctx, value) per Typer convention, use importlib.metadata.version inside it to typer.echo the package version and then call raise typer.Exit() (or ctx.exit()) to exit; then register the option on the command (where the --version option is declared) as typer.Option(False, "--version", help=..., is_eager=True, callback=_print_version) so Typer invokes _print_version early and avoids raising inside the main handler.tests/integration/test_http_mcp.py (2)
25-73: 💤 Low valueLifespan teardown cancels mid-flight instead of completing graceful shutdown.
The fixture cancels
lifespan_taskdirectly, which short-circuits theasynccontextmanagerand skipsmanager.run().__aexit__. For per-test fixtures with freshEngraminstances this is contained, but a graceful path (sendlifespan.shutdownand wait forlifespan.shutdown.complete) is closer to what real servers do and avoids surfacing MCP teardown bugs only in production. Optional polish; current behavior is acceptable.🤖 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/integration/test_http_mcp.py` around lines 25 - 73, The fixture currently cancels lifespan_task which aborts the ASGI asynccontextmanager; change _start_lifespan to expose a way to shutdown gracefully (e.g., return the background Task plus the send channel or a shutdown waiter), and in the app fixture send a {"type":"lifespan.shutdown"} via the returned send_chan and await a corresponding shutdown_complete event (handle it in _send by setting shutdown_complete when "lifespan.shutdown.complete" is received) before cancelling the task as a last resort; update references to _start_lifespan, send_chan, startup_complete, and lifespan_task accordingly so teardown performs send -> wait -> cancel sequence.
230-232: 💤 Low valueTighten the recall assertion since the test uses deterministic embeddings.
The test's embedding provider (
SyntheticEmbedding) is deterministic by design — it produces stable vectors derived from hash-seeded RNG. Since"Alice's dog is named Pepper"is recorded and then recalled with a deterministic query, the result should consistently return the stored text. Replace the permissive assertion withassert "Pepper" in recall_textto catch retrieval regressions.Note: The
ENGRAM_LLM_PROVIDER="mock"environment variable set in the fixture is unused in the codebase and does not affect this test's determinism.🤖 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/integration/test_http_mcp.py` around lines 230 - 232, The current assertion in the test uses a permissive OR allowing "(no matches)" even though embeddings are deterministic; update the assertion in tests/integration/test_http_mcp.py that checks recall_text (variable recall_text) to require the stored phrase is present by replacing the conditional assert with a strict assertion: assert "Pepper" in recall_text; this ensures the deterministic embedding provider (SyntheticEmbedding) and the recall flow return the recorded "Alice's dog is named Pepper" consistently and will catch regressions in the recall path.tests/unit/test_cli.py (1)
12-15: 💤 Low valueBrittle version literal will fail on the next bump.
assert "0.2.0" in result.stdoutcouples this test to today's release; it will need editing on every bump (and silently fail open if someone forgets). Pulling the expected value from the same source the CLI uses keeps the test honest.♻️ Proposed fix
def test_version_flag(): + from importlib.metadata import version as _pkg_version + result = runner.invoke(app, ["--version"]) assert result.exit_code == 0 - assert "0.2.0" in result.stdout + assert _pkg_version("jamjet-engram") in result.stdout🤖 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/unit/test_cli.py` around lines 12 - 15, The test test_version_flag is brittle because it asserts a hard-coded string "0.2.0"; instead import the canonical version used by the CLI (the same constant the app uses, e.g. the package __version__ or VERSION exported from the module that defines app) and assert that str(version) is in result.stdout after runner.invoke(app, ["--version"]); replace the literal with that imported version so the test follows the source of truth.
🤖 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 `@CHANGELOG.md`:
- Around line 17-37: Update the changelog footer links so the "Unreleased"
comparison starts from v0.2.0 and add a release link target for the new [0.2.0]
section: modify the footer link reference for "Unreleased" to compare against
"v0.2.0" instead of the previous base, and add a new link entry for "[0.2.0]"
pointing to the repository release/diff URL for v0.2.0; ensure the identifiers
[0.2.0] and Unreleased in the file match the new footer entries exactly.
In `@src/engram/cli.py`:
- Around line 95-104: The current pattern uses asyncio.run(_setup()) to open
Engram (Engram.open) and then calls uvicorn.run(...) which creates a new event
loop, causing loop-bound resources (aiosqlite) to be attached to a
closed/different loop and never closed; change the flow so the entire lifecycle
stays inside a single asyncio.run: in the async _setup() create the Engram
instance (resolved_db), build_mcp_server and build_http_app, then construct a
uvicorn.Config/uvicorn.Server and await server.serve() inside a try/finally that
calls await engram.close() on shutdown; remove the separate uvicorn.run(...)
call and ensure asyncio.run only wraps the coroutine that both starts the server
and closes Engram.
In `@src/engram/server/auth.py`:
- Around line 57-69: The authorization-scheme check is currently case-sensitive
(header_value.startswith(b"Bearer ")), so update the logic in the auth handling
block that reads header_value to perform a case-insensitive check and token
extraction: convert the header prefix to a normalized form (either decode
header_value first and use .lower().startswith("bearer ") or compare the first
bytes using header_value[:7].lower() == b"bearer "), then slice/parse the token
after the scheme, decode it to a string, and continue to call
compare_digest(token, expected_token) and await app(scope, receive, send) on
success; leave _send_401(send) as the failure path. Ensure you still handle
empty header_value safely.
---
Nitpick comments:
In `@src/engram/cli.py`:
- Around line 27-31: Change the version handler to an eager Typer callback so it
runs before the main command body: update the _print_version function signature
to accept (value: bool, ctx: typer.Context) or (ctx, value) per Typer
convention, use importlib.metadata.version inside it to typer.echo the package
version and then call raise typer.Exit() (or ctx.exit()) to exit; then register
the option on the command (where the --version option is declared) as
typer.Option(False, "--version", help=..., is_eager=True,
callback=_print_version) so Typer invokes _print_version early and avoids
raising inside the main handler.
In `@src/engram/server/http.py`:
- Around line 131-146: Replace the hardcoded version="0.2.0" used when
constructing the FastAPI app with a single-source-of-truth value obtained from
package metadata (e.g. importlib.metadata.version("jamjet-engram")) and reuse
that variable in both branches where FastAPI(...) is created (the branch that
sets lifespan and app.router.routes.append(Route("/mcp",
endpoint=_MCPHandler())) and the else branch), so the OpenAPI/docs version
always reflects the package version.
In `@tests/integration/test_http_mcp.py`:
- Around line 25-73: The fixture currently cancels lifespan_task which aborts
the ASGI asynccontextmanager; change _start_lifespan to expose a way to shutdown
gracefully (e.g., return the background Task plus the send channel or a shutdown
waiter), and in the app fixture send a {"type":"lifespan.shutdown"} via the
returned send_chan and await a corresponding shutdown_complete event (handle it
in _send by setting shutdown_complete when "lifespan.shutdown.complete" is
received) before cancelling the task as a last resort; update references to
_start_lifespan, send_chan, startup_complete, and lifespan_task accordingly so
teardown performs send -> wait -> cancel sequence.
- Around line 230-232: The current assertion in the test uses a permissive OR
allowing "(no matches)" even though embeddings are deterministic; update the
assertion in tests/integration/test_http_mcp.py that checks recall_text
(variable recall_text) to require the stored phrase is present by replacing the
conditional assert with a strict assertion: assert "Pepper" in recall_text; this
ensures the deterministic embedding provider (SyntheticEmbedding) and the recall
flow return the recorded "Alice's dog is named Pepper" consistently and will
catch regressions in the recall path.
In `@tests/unit/test_cli.py`:
- Around line 12-15: The test test_version_flag is brittle because it asserts a
hard-coded string "0.2.0"; instead import the canonical version used by the CLI
(the same constant the app uses, e.g. the package __version__ or VERSION
exported from the module that defines app) and assert that str(version) is in
result.stdout after runner.invoke(app, ["--version"]); replace the literal with
that imported version so the test follows the source of truth.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cddd9fb-dda7-4cbb-a1f1-677a85773650
📒 Files selected for processing (15)
.dockerignore.github/workflows/release.ymlCHANGELOG.mdDockerfileREADME.mdpyproject.tomlsrc/engram/cli.pysrc/engram/server/auth.pysrc/engram/server/http.pysrc/engram/server/transport.pytests/integration/test_http_mcp.pytests/integration/test_v1_rest_unaffected.pytests/unit/test_auth.pytests/unit/test_cli.pytests/unit/test_transport.py
…casing, changelog links - src/engram/cli.py: extract serve_engram() and drive uvicorn via asyncio.run so Engram.open() (which holds a loop-bound aiosqlite connection) lives in the same event loop uvicorn serves on. Previously asyncio.run(_setup()) closed the loop before uvicorn.run() opened a new one — first MCP tool call would throw "Event loop is closed". Also calls engram.close() on shutdown (was leaked). - src/engram/server/auth.py: case-insensitive Bearer scheme match (RFC 9110 §11). Some clients (Go oauth2, lowercase curl) emit "bearer" — was 401. - tests/unit/test_auth.py: add positive tests for "bearer" and "BEARER". - CHANGELOG.md: add [0.2.0] release link target; update [unreleased] to compare from v0.2.0. Verified end-to-end via Docker smoke test: memory_record over /mcp now succeeds (this is the path that would have hit the loop bug).
Summary
Ships an
engram-serverPython binary with MCP Streamable HTTP transport atPOST /mcp, mounted alongside the existing/v1/memory/*REST surface on the same FastAPI app. Also ships a new Docker imageghcr.io/jamjet-labs/engram-py-server(separate tag space from the Rustengram-serverimage) and extends the release workflow to push it to GHCR.This unblocks JamJet Cloud agents and remote MCP clients (ChatGPT custom connectors, Claude Desktop's HTTP transport) talking to a hosted Engram. Stdio Claude Desktop users keep using the Rust v0.5.0 image — no behavior change there.
Architecture (one-liner per file)
src/engram/server/auth.py— bearer-token ASGI middleware (chosen over FastAPI dependency because/mcpis mounted as a sub-ASGI app)src/engram/server/transport.py— wrapsmcp.server.ServerinStreamableHTTPSessionManager(stateless=True); the only file in the codebase that imports the MCP SDK's transport APIsrc/engram/server/http.py— extended with optionalmcp_serverandauth_tokenkwargs; mounts/mcpviaRoute(notMount— Mount 307-redirects bare/mcp)src/engram/cli.py— typer-basedengram-server; fail-closed on missing token,--no-authopt-out only with--host 127.0.0.1or::1(localhostrejected)Dockerfile—python:3.13-slim, non-root,/datavolume, healthcheckAuth model (asymmetric, intentional)
/mcp(POST + GET)ENGRAM_AUTH_TOKENrequired, fail-closed/v1/memory/*/healthzThe asymmetry is documented in README and guarded by
tests/integration/test_v1_rest_unaffected.pyso anyone adding/v1auth later has to update the test deliberately.Test Plan
python -m buildproduces wheel + sdist;twine checkpasses bothpip install jamjet_engram-0.2.0-py3-none-any.whlthenengram-server --versionprints0.2.0docker build -t engram-py-server:dev .succeeds/healthz200,/mcp401 with wrong bearerv0.2.0triggers release workflow then PyPI publish + GHCR push ofengram-py-server:0.2.0and:latestOut of scope (deferred)
--transport=stdioreserved, currently rejects)/v1/memory/*auth gatingengram-serverGHCR imageSummary by CodeRabbit
Release Notes: v0.2.0
New Features
engram-serverCLI for hosting Engram as an MCP server over HTTPENGRAM_AUTH_TOKEN; REST memory API remains accessible without authenticationDocumentation
Chores