feat(mcp): HTTP transport + serve-mcp CLI for ChatGPT App Directory#238
Draft
kaghni wants to merge 1 commit into
Draft
feat(mcp): HTTP transport + serve-mcp CLI for ChatGPT App Directory#238kaghni wants to merge 1 commit into
kaghni wants to merge 1 commit into
Conversation
Adds a Streamable HTTP wrapper around the existing 3 MCP tools (hivemind_search/read/index) so remote clients like the ChatGPT App Directory can reach them. The stdio MCP server used by local agents (Hermes/Cursor/Claude Code) is unchanged. What's new: - src/mcp/tools.ts: factor out the 3 tool registrations into a shared module so stdio and HTTP transports stay single-source- of-truth. Adds read-only annotations (readOnlyHint: true, openWorldHint: false, destructiveHint: false) — the #1 cited rejection cause in OpenAI's App Submission Guidelines. - src/mcp/server.ts: refactored to import from tools.ts (stdio behavior unchanged). - src/mcp/http-server.ts: new HTTP MCP server using the official @modelcontextprotocol/sdk's StreamableHTTPServerTransport. One McpServer + transport per session (keyed by Mcp-Session-Id header per the SDK's pattern). Health + 401+WWW-Authenticate + routing. - src/cli/serve-mcp.ts + dispatch wire-up in src/cli/index.ts: the `hivemind serve-mcp` command for hosting in a container. Configured via env vars (see deploy/mcp/README.md). - tests/shared/mcp-http-server.test.ts: 5 tests covering health, 401+WWW-Authenticate shape, malformed-auth, 404, and no-leak of auth metadata on /health. - deploy/mcp/{Dockerfile,k8s.yaml,README.md}: starting templates for SRE — multi-stage Alpine image with non-root user, k8s manifest with ingress rules tuned for SSE (proxy-buffering off, long timeouts). Auth model: the user's Auth0 JWT arrives in Authorization: Bearer. We don't validate it locally — DeeplakeApi forwards it to deeplake-api, which validates it via the existing internal/auth/jwt.go. Missing token => 401 with WWW-Authenticate pointing at the Auth0 issuer (the discovery mechanism ChatGPT needs). Org resolution: defaults to `GET /organizations` and picking the first org for the user. Env vars HIVEMIND_MCP_ORG_ID + HIVEMIND_MCP_WORKSPACE + HIVEMIND_MCP_TABLE + HIVEMIND_MCP_SESSIONS_TABLE pin the org/workspace/tables at boot for single-tenant deployments — needed for the activeloop deployment because /organizations returns 34 orgs in unstable order and the user's primary may not be first. Multi-tenant auto-resolution is captured as a follow-up. End-to-end smoke against api.deeplake.ai production succeeded: initialize, tools/list, hivemind_index, and hivemind_search all return real data over the HTTP transport using a real Auth0 token forwarded through to deeplake-api. Dockerfile uses `npm install --ignore-scripts` as a workaround for an existing package-lock.json drift on origin/main (pg@8.21 in package.json, ^8.11 in lockfile). Should switch back to `npm ci` once the lockfile is regenerated in a separate PR.
📝 WalkthroughWalkthroughThis PR introduces an HTTP transport for the Hivemind MCP server, enabling ChatGPT App Directory integration. It refactors tool logic into a reusable registration function, implements a stateful HTTP server with per-session MCP contexts and Bearer token auth, wires a new CLI command, adds contract tests, and provides Docker and Kubernetes deployment infrastructure. ChangesHTTP MCP Server with Deployment
Sequence DiagramsequenceDiagram
participant Client
participant HTTPServer as HTTP Server
participant Session as MCP Session
participant Transport as StreamableHTTPServerTransport
participant DeeplakeApi as Deeplake API
Client->>HTTPServer: POST /mcp with Bearer token
HTTPServer->>HTTPServer: Extract + validate Bearer token
alt No Bearer Token
HTTPServer-->>Client: 401 with WWW-Authenticate
else Token Present
HTTPServer->>HTTPServer: Get/create session by mcp-session-id
alt Session is New
HTTPServer->>Session: Create McpServer instance
HTTPServer->>Transport: Create StreamableHTTPServerTransport
HTTPServer->>Session: registerHivemindTools(server, getContext)
HTTPServer->>Transport: Connect server to transport
end
HTTPServer->>HTTPServer: buildGetContext callback
HTTPServer->>DeeplakeApi: Resolve primary organization
HTTPServer->>Session: handleRequest(JSON body)
Transport->>Session: Process tool call
Session-->>Transport: Tool response
Transport-->>HTTPServer: Serialized response
HTTPServer-->>Client: JSON or SSE stream
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function extractBearer(req: http.IncomingMessage): string | null { | ||
| const header = req.headers["authorization"]; | ||
| if (!header || typeof header !== "string") return null; | ||
| const m = header.match(/^Bearer\s+(.+)$/i); |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 5 files changed
Generated for commit 3e47025. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/shared/mcp-http-server.test.ts (1)
64-65: ⚡ Quick winHarden
WWW-Authenticateassertions to exact parameter checks.
toContain(...)can pass even with malformed header structure/order. Parse the challenge (or assert a strict regex) and validate scheme + exactrealmanderrorparameter values.Suggested test tightening
- expect(wwwAuth).toContain(`realm="${AUTH_ISSUER}"`); - expect(wwwAuth).toContain("invalid_token"); + expect(wwwAuth).toMatch( + new RegExp(`^Bearer\\s+.*\\brealm="${AUTH_ISSUER.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}"\\b.*\\berror="invalid_token"\\b`), + ); ... - expect(resp.headers.get("www-authenticate")).toContain(`realm="${AUTH_ISSUER}"`); + expect(resp.headers.get("www-authenticate")).toMatch( + new RegExp(`^Bearer\\s+.*\\brealm="${AUTH_ISSUER.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}"\\b`), + );As per coding guidelines:
tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.Also applies to: 82-82
🤖 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/shared/mcp-http-server.test.ts` around lines 64 - 65, The current assertions on the WWW-Authenticate header are too loose; update the test that reads the header into wwwAuth to parse the challenge and assert exact scheme and parameter values instead of using toContain. Specifically, split wwwAuth to validate the auth scheme (e.g., "Bearer"), parse the comma-separated key="value" params into a map, then assert params.realm === AUTH_ISSUER and params.error === "invalid_token" (also tighten the similar assertion around the other occurrence at the later assertion). Use the existing wwwAuth and AUTH_ISSUER symbols to locate the header and replace the substring checks with these precise checks.src/cli/serve-mcp.ts (1)
52-56: 💤 Low valueConsider logging the actual resolved port for consistency.
The
startHttpMcpServerfunction returns the actual bound port (which can differ from the requested port if the OS assigns one). While your validation on lines 38–42 rejectsport <= 0(so OS-assigned ports are not supported), it would be slightly more consistent with the server's design to destructure and log the actual port.♻️ Optional refactor to use actualPort
- const { close } = await startHttpMcpServer({ + const { close, port: actualPort } = await startHttpMcpServer({ port, host, authIssuer, apiUrl, orgId, workspaceId, memoryTable, sessionsTable, }); - log(`hivemind serve-mcp ready on http://${host}:${port}/mcp`); + log(`hivemind serve-mcp ready on http://${host}:${actualPort}/mcp`);🤖 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/cli/serve-mcp.ts` around lines 52 - 56, Destructure the actual bound port returned by startHttpMcpServer (e.g., const { close, port: actualPort } = await startHttpMcpServer(...)) and use that actualPort in the log call instead of the requested port variable; update the log invocation at the current log(...) call to reference actualPort so the printed URL reflects the server's resolved port while keeping the existing validation around the requested port.
🤖 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 `@deploy/mcp/k8s.yaml`:
- Around line 113-115: The comment above the Ingress spec is inaccurate: it
claims routes for "/mcp" and "/.well-known" but the Ingress only defines "/mcp"
(Prefix) and "/health" (Exact) in the rules; update the comment to reflect the
actual configured paths or add the missing "/.well-known" path rule to the
Ingress spec. Specifically, edit the comment that references "Routes
api.deeplake.ai/mcp and /.well-known paths" to instead list "/mcp and /health"
(or add a new path block for "/.well-known" pointing to the MCP service if that
behavior is required), and ensure the Ingress path types (Prefix/Exact) and
backend service name in the spec (the MCP service block) match the comment.
In `@deploy/mcp/README.md`:
- Around line 10-19: Add the four optional environment variables that
src/cli/serve-mcp.ts reads to the configuration table: HIVEMIND_MCP_ORG_ID
(optional pin to a specific org ID, skips /organizations lookup),
HIVEMIND_MCP_WORKSPACE (optional workspace ID, default "default"),
HIVEMIND_MCP_TABLE (optional memory table name, default "memory"), and
HIVEMIND_MCP_SESSIONS_TABLE (optional sessions table name, default "sessions");
update the table rows to include each Var, its default/value note, and a short
description so the README matches the comments and behavior in serve-mcp.ts.
In `@src/mcp/http-server.ts`:
- Around line 321-327: The shutdown order in the close function is reversed:
call session.transport.close() for every active session first (iterating over
sessions.values() and awaiting each transport.close() with errors caught) and
only after all transports have been closed await the httpServer.close() Promise;
update the close function (referencing close, sessions, session.transport.close,
and httpServer.close) to perform transport shutdown before invoking and awaiting
httpServer.close() to prevent hanging SSE responses.
- Around line 121-133: The readBody function currently buffers the entire
request with no size limit and can exhaust memory; modify readBody(req) to
enforce a sane max payload (e.g., MAX_BODY_BYTES constant) by tracking
cumulative bytes while iterating the async chunks, stop reading and return a
clear sentinel (e.g., throw or return a specific error/marker like {tooLarge:
true} or throw a custom PayloadTooLargeError) as soon as the limit is exceeded,
and ensure you do not concatenate further chunks; then update handleMcpRequest
to detect that sentinel or catch the custom error and respond with HTTP 413
instead of treating it as a generic 500.
- Around line 223-250: The current lookup reuses sessions by mcp-session-id
alone; change the logic so sessions are bound to the bearer token: when creating
a session in createSession, record the token on the session object (e.g.,
session.token or session.ownerToken) and when looking up an existing session
(sessionIdHeader / existingId) ensure the incoming token matches session.token
before reusing it; register sessions in st.sessions under a composite key or
check token equality after lookup (use session.transport.sessionId as the
session id) and when handling teardown/DELETE remove the stored session from
st.sessions so the old token/state is evicted; update the registration block
that uses session.transport.sessionId to include the token binding so subsequent
lookups validate token equality.
In `@tests/shared/mcp-http-server.test.ts`:
- Around line 38-40: The afterEach teardown currently calls await
running.close() unguarded and will throw if beforeEach failed to assign running;
update afterEach to first check that the test server handle exists (e.g., if
(running) or if (running != null)) before calling close so teardown is skipped
when setup failed; ensure the variable referenced is the same test-scoped symbol
named running used in beforeEach/afterEach.
---
Nitpick comments:
In `@src/cli/serve-mcp.ts`:
- Around line 52-56: Destructure the actual bound port returned by
startHttpMcpServer (e.g., const { close, port: actualPort } = await
startHttpMcpServer(...)) and use that actualPort in the log call instead of the
requested port variable; update the log invocation at the current log(...) call
to reference actualPort so the printed URL reflects the server's resolved port
while keeping the existing validation around the requested port.
In `@tests/shared/mcp-http-server.test.ts`:
- Around line 64-65: The current assertions on the WWW-Authenticate header are
too loose; update the test that reads the header into wwwAuth to parse the
challenge and assert exact scheme and parameter values instead of using
toContain. Specifically, split wwwAuth to validate the auth scheme (e.g.,
"Bearer"), parse the comma-separated key="value" params into a map, then assert
params.realm === AUTH_ISSUER and params.error === "invalid_token" (also tighten
the similar assertion around the other occurrence at the later assertion). Use
the existing wwwAuth and AUTH_ISSUER symbols to locate the header and replace
the substring checks with these precise checks.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 31c7a1a6-1791-4380-905d-073c13c5ca85
📒 Files selected for processing (9)
deploy/mcp/Dockerfiledeploy/mcp/README.mddeploy/mcp/k8s.yamlsrc/cli/index.tssrc/cli/serve-mcp.tssrc/mcp/http-server.tssrc/mcp/server.tssrc/mcp/tools.tstests/shared/mcp-http-server.test.ts
Comment on lines
+113
to
+115
| # Routes api.deeplake.ai/mcp and /.well-known paths to the MCP service. | ||
| # Everything else on api.deeplake.ai (the existing API) stays on its | ||
| # current backend — adjust the host's other path rules accordingly. |
There was a problem hiding this comment.
Misleading comment about routed paths.
The comment states "Routes api.deeplake.ai/mcp and /.well-known paths" but the actual Ingress spec only defines two paths: /mcp (Prefix, lines 136–142) and /health (Exact, lines 143–149). There are no /.well-known paths configured.
📝 Suggested fix
-# Routes api.deeplake.ai/mcp and /.well-known paths to the MCP service.
+# Routes api.deeplake.ai/mcp and /health paths to the MCP service.
# Everything else on api.deeplake.ai (the existing API) stays on its
# current backend — adjust the host's other path rules accordingly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Routes api.deeplake.ai/mcp and /.well-known paths to the MCP service. | |
| # Everything else on api.deeplake.ai (the existing API) stays on its | |
| # current backend — adjust the host's other path rules accordingly. | |
| # Routes api.deeplake.ai/mcp and /health paths to the MCP service. | |
| # Everything else on api.deeplake.ai (the existing API) stays on its | |
| # current backend — adjust the host's other path rules accordingly. |
🤖 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 `@deploy/mcp/k8s.yaml` around lines 113 - 115, The comment above the Ingress
spec is inaccurate: it claims routes for "/mcp" and "/.well-known" but the
Ingress only defines "/mcp" (Prefix) and "/health" (Exact) in the rules; update
the comment to reflect the actual configured paths or add the missing
"/.well-known" path rule to the Ingress spec. Specifically, edit the comment
that references "Routes api.deeplake.ai/mcp and /.well-known paths" to instead
list "/mcp and /health" (or add a new path block for "/.well-known" pointing to
the MCP service if that behavior is required), and ensure the Ingress path types
(Prefix/Exact) and backend service name in the spec (the MCP service block)
match the comment.
Comment on lines
+10
to
+19
| ## Configuration (env vars) | ||
|
|
||
| All read by `src/cli/serve-mcp.ts`: | ||
|
|
||
| | Var | Default | Notes | | ||
| |---|---|---| | ||
| | `HIVEMIND_MCP_AUTH_ISSUER` | _(required)_ | Auth0 issuer URL (trailing slash). Advertised in `WWW-Authenticate` on 401 so ChatGPT can discover the auth server. e.g. `https://auth-beta.deeplake.ai/` | | ||
| | `HIVEMIND_MCP_PORT` | `8787` | Listen port | | ||
| | `HIVEMIND_MCP_HOST` | `0.0.0.0` | Bind host | | ||
| | `HIVEMIND_API_URL` | `https://api.deeplake.ai` | deeplake-api base URL | |
There was a problem hiding this comment.
Document the optional org/workspace pinning environment variables.
The configuration table is missing four optional environment variables that src/cli/serve-mcp.ts reads (lines 46–49 and documented in comments on lines 17–21):
HIVEMIND_MCP_ORG_ID— pin to a specific org ID (skips /organizations lookup)HIVEMIND_MCP_WORKSPACE— workspace ID (default "default")HIVEMIND_MCP_TABLE— memory table name (default "memory")HIVEMIND_MCP_SESSIONS_TABLE— sessions table name (default "sessions")
These should be added to the table for completeness.
📝 Suggested addition to the configuration table
| Var | Default | Notes |
|---|---|---|
| `HIVEMIND_MCP_AUTH_ISSUER` | _(required)_ | Auth0 issuer URL (trailing slash). Advertised in `WWW-Authenticate` on 401 so ChatGPT can discover the auth server. e.g. `https://auth-beta.deeplake.ai/` |
| `HIVEMIND_MCP_PORT` | `8787` | Listen port |
| `HIVEMIND_MCP_HOST` | `0.0.0.0` | Bind host |
| `HIVEMIND_API_URL` | `https://api.deeplake.ai` | deeplake-api base URL |
+| `HIVEMIND_MCP_ORG_ID` | _(optional)_ | Pin to a specific org ID (skips /organizations lookup) |
+| `HIVEMIND_MCP_WORKSPACE` | `default` | Workspace ID |
+| `HIVEMIND_MCP_TABLE` | `memory` | Memory table name |
+| `HIVEMIND_MCP_SESSIONS_TABLE` | `sessions` | Sessions table name |🤖 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 `@deploy/mcp/README.md` around lines 10 - 19, Add the four optional environment
variables that src/cli/serve-mcp.ts reads to the configuration table:
HIVEMIND_MCP_ORG_ID (optional pin to a specific org ID, skips /organizations
lookup), HIVEMIND_MCP_WORKSPACE (optional workspace ID, default "default"),
HIVEMIND_MCP_TABLE (optional memory table name, default "memory"), and
HIVEMIND_MCP_SESSIONS_TABLE (optional sessions table name, default "sessions");
update the table rows to include each Var, its default/value note, and a short
description so the README matches the comments and behavior in serve-mcp.ts.
Comment on lines
+121
to
+133
| async function readBody(req: http.IncomingMessage): Promise<unknown> { | ||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of req) { | ||
| chunks.push(chunk as Buffer); | ||
| } | ||
| if (chunks.length === 0) return undefined; | ||
| const raw = Buffer.concat(chunks).toString("utf8"); | ||
| if (!raw) return undefined; | ||
| try { | ||
| return JSON.parse(raw); | ||
| } catch { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Cap buffered request size before parsing JSON.
This reads the entire POST body into memory with no limit. Because extractBearer() only checks header shape, a single oversized /mcp request with Authorization: Bearer x can exhaust memory before anything downstream rejects it. Return 413 once the payload crosses a sane cap.
Suggested fix
-async function readBody(req: http.IncomingMessage): Promise<unknown> {
+async function readBody(req: http.IncomingMessage, maxBytes = 1024 * 1024): Promise<unknown> {
const chunks: Buffer[] = [];
+ let total = 0;
for await (const chunk of req) {
- chunks.push(chunk as Buffer);
+ const buf = chunk as Buffer;
+ total += buf.length;
+ if (total > maxBytes) {
+ throw new Error("payload_too_large");
+ }
+ chunks.push(buf);
}
if (chunks.length === 0) return undefined;You'll also want handleMcpRequest() to translate that sentinel into an HTTP 413 instead of the generic 500.
🤖 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/mcp/http-server.ts` around lines 121 - 133, The readBody function
currently buffers the entire request with no size limit and can exhaust memory;
modify readBody(req) to enforce a sane max payload (e.g., MAX_BODY_BYTES
constant) by tracking cumulative bytes while iterating the async chunks, stop
reading and return a clear sentinel (e.g., throw or return a specific
error/marker like {tooLarge: true} or throw a custom PayloadTooLargeError) as
soon as the limit is exceeded, and ensure you do not concatenate further chunks;
then update handleMcpRequest to detect that sentinel or catch the custom error
and respond with HTTP 413 instead of treating it as a generic 500.
Comment on lines
+223
to
+250
| const sessionIdHeader = req.headers["mcp-session-id"]; | ||
| const existingId = typeof sessionIdHeader === "string" ? sessionIdHeader : undefined; | ||
|
|
||
| let session = existingId ? st.sessions.get(existingId) : undefined; | ||
| if (!session) { | ||
| // New session — body must be the MCP `initialize` request. | ||
| session = await createSession(token, st.options); | ||
| // The transport assigns the session id on init; store under that id once known. | ||
| // We use the transport's sessionId (set by the SDK after handleRequest). | ||
| } | ||
|
|
||
| const body = req.method === "POST" ? await readBody(req) : undefined; | ||
|
|
||
| try { | ||
| await session.transport.handleRequest(req, res, body); | ||
| } catch (err) { | ||
| if (!st.options.silent) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write(`mcp http error: ${msg}\n`); | ||
| } | ||
| if (!res.headersSent) sendJson(res, 500, { error: "internal_error" }); | ||
| return; | ||
| } | ||
|
|
||
| // After the SDK assigns sessionId, register under that id for subsequent lookups. | ||
| const assignedId = session.transport.sessionId; | ||
| if (assignedId && !st.sessions.has(assignedId)) { | ||
| st.sessions.set(assignedId, session); |
There was a problem hiding this comment.
Bind reused sessions to the bearer token and evict them on teardown.
Line 226 trusts mcp-session-id alone. Once a session exists, a request with any Bearer token will reuse it, and buildGetContext() continues querying with the original state.token. That makes the session id the real auth boundary and also leaves the old token/state resident after DELETE.
Suggested fix
let session = existingId ? st.sessions.get(existingId) : undefined;
+ if (session && session.token !== token) {
+ send401(res, st.options.authIssuer, "Bearer token does not match the active MCP session.");
+ return;
+ }
if (!session) {
// New session — body must be the MCP `initialize` request.
session = await createSession(token, st.options);
// The transport assigns the session id on init; store under that id once known.
// We use the transport's sessionId (set by the SDK after handleRequest).
@@
- const assignedId = session.transport.sessionId;
+ const assignedId = session.transport.sessionId ?? existingId;
if (assignedId && !st.sessions.has(assignedId)) {
st.sessions.set(assignedId, session);
}
+ if (req.method === "DELETE" && assignedId) {
+ st.sessions.delete(assignedId);
+ }🤖 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/mcp/http-server.ts` around lines 223 - 250, The current lookup reuses
sessions by mcp-session-id alone; change the logic so sessions are bound to the
bearer token: when creating a session in createSession, record the token on the
session object (e.g., session.token or session.ownerToken) and when looking up
an existing session (sessionIdHeader / existingId) ensure the incoming token
matches session.token before reusing it; register sessions in st.sessions under
a composite key or check token equality after lookup (use
session.transport.sessionId as the session id) and when handling teardown/DELETE
remove the stored session from st.sessions so the old token/state is evicted;
update the registration block that uses session.transport.sessionId to include
the token binding so subsequent lookups validate token equality.
Comment on lines
+321
to
+327
| const close = async (): Promise<void> => { | ||
| await new Promise<void>((resolve, reject) => | ||
| httpServer.close(err => (err ? reject(err) : resolve())), | ||
| ); | ||
| for (const session of sessions.values()) { | ||
| await session.transport.close().catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Close active transports before awaiting httpServer.close().
httpServer.close() waits for open /mcp SSE responses to finish. Those responses only terminate when session.transport.close() runs, so the current order can hang shutdown indefinitely under an active session.
Suggested fix
const close = async (): Promise<void> => {
- await new Promise<void>((resolve, reject) =>
- httpServer.close(err => (err ? reject(err) : resolve())),
- );
- for (const session of sessions.values()) {
- await session.transport.close().catch(() => {});
- }
+ await Promise.all(
+ Array.from(sessions.values(), session => session.transport.close().catch(() => {})),
+ );
sessions.clear();
+ await new Promise<void>((resolve, reject) =>
+ httpServer.close(err => (err ? reject(err) : resolve())),
+ );
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const close = async (): Promise<void> => { | |
| await new Promise<void>((resolve, reject) => | |
| httpServer.close(err => (err ? reject(err) : resolve())), | |
| ); | |
| for (const session of sessions.values()) { | |
| await session.transport.close().catch(() => {}); | |
| } | |
| const close = async (): Promise<void> => { | |
| await Promise.all( | |
| Array.from(sessions.values(), session => session.transport.close().catch(() => {})), | |
| ); | |
| sessions.clear(); | |
| await new Promise<void>((resolve, reject) => | |
| httpServer.close(err => (err ? reject(err) : resolve())), | |
| ); | |
| }; |
🤖 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/mcp/http-server.ts` around lines 321 - 327, The shutdown order in the
close function is reversed: call session.transport.close() for every active
session first (iterating over sessions.values() and awaiting each
transport.close() with errors caught) and only after all transports have been
closed await the httpServer.close() Promise; update the close function
(referencing close, sessions, session.transport.close, and httpServer.close) to
perform transport shutdown before invoking and awaiting httpServer.close() to
prevent hanging SSE responses.
Comment on lines
+38
to
+40
| afterEach(async () => { | ||
| await running.close(); | ||
| }); |
There was a problem hiding this comment.
Guard teardown when setup fails before assignment.
If beforeEach throws before running is set, afterEach can throw and obscure the real failure.
Minimal safeguard
interface RunningServer {
baseUrl: string;
close: () => Promise<void>;
}
...
describe("HTTP MCP server", () => {
- let running: RunningServer;
+ let running: RunningServer | undefined;
...
afterEach(async () => {
- await running.close();
+ await running?.close();
});🤖 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/shared/mcp-http-server.test.ts` around lines 38 - 40, The afterEach
teardown currently calls await running.close() unguarded and will throw if
beforeEach failed to assign running; update afterEach to first check that the
test server handle exists (e.g., if (running) or if (running != null)) before
calling close so teardown is skipped when setup failed; ensure the variable
referenced is the same test-scoped symbol named running used in
beforeEach/afterEach.
Collaborator
Author
Status: how close to ChatGPT App Directory appearanceEngineering — done
Both PRs are ready for the review/merge cycle. CodeRabbit + codex haven't been triggered yet on either. Remaining — 5 things, none mine aloneIn strict critical-path order:
Calendar estimateAssuming nothing blocks:
~1-2 weeks from today, gated by OpenAI's 2-5 business day review. Parallelizable while waiting
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the smallest possible HTTP wrapper around the existing 3 MCP tools so Hivemind can appear in the ChatGPT App Directory. The local stdio MCP server (used by Hermes/Cursor/Claude Code) is unchanged — this PR only adds a second transport reachable over the network.
src/mcp/tools.tsso stdio and HTTP transports register the same tool handlers (single source of truth). Tool annotations set: all 3readOnlyHint: true, openWorldHint: false, destructiveHint: false— the initial virtual fs implementation #1 cited rejection cause in OpenAI's App Submission Guidelines.src/mcp/http-server.tsusingStreamableHTTPServerTransportfrom@modelcontextprotocol/sdkv1.29 — the transport Apps SDK actually negotiates.hivemind serve-mcpCLI command for hosting in a container.deploy/mcp/{Dockerfile,k8s.yaml,README.md}— starting templates for SRE.tests/shared/mcp-http-server.test.ts.Auth (no new auth code)
The user's Auth0 JWT arrives in
Authorization: Bearer …. We don't validate it locally —DeeplakeApiforwards it to deeplake-api, which validates it via the existinginternal/auth/jwt.go. Missing/malformed token →401withWWW-Authenticate: Bearer realm="<auth0-issuer>"so ChatGPT discovers where to send the user to sign in. The auth flow that ChatGPT runs is the standard Auth0 Universal Login — the same screen deeplake.ai users already see today.Org resolution
HIVEMIND_MCP_ORG_ID+HIVEMIND_MCP_WORKSPACE+HIVEMIND_MCP_TABLE+HIVEMIND_MCP_SESSIONS_TABLEpin the org/workspace/tables at boot. Needed for the activeloop deployment becauseGET /organizationsreturns 34 orgs in unstable order — auto-picking the first org defaulted to a test org during the smoke. Multi-tenant auto-resolution (picking the user's primary org reliably) is a follow-up.If none of the env pins are set, the server falls back to "first org from
GET /organizations" +workspaceId=\"default\"+memory/sessionstable names (matching the CLI defaults insrc/config.ts:55-58).Verification
End-to-end smoke against
api.deeplake.aiproduction succeeded using a real Auth0 token from~/.deeplake/credentials.json(org pinned toactiveloop/ workspacehivemindvia env vars):initialize→ session assigned, capabilities advertisedtools/list→ 3 tools with the read-only annotationstools/call hivemind_index limit=3→ 3 real summaries from the activeloop/hivemind workspacetools/call hivemind_search query=\"chatgpt\"→ 4 KB of real matching contentThe full chain works: Bearer JWT → MCP HTTP transport → deeplake-api validation → memory query → tool result. No deeplake-api changes needed.
Dockerfile caveat
Uses
npm install --ignore-scriptsas a workaround for an existingpackage-lock.jsondrift on origin/main (pg@8.21in package.json,^8.11in lockfile). Should switch back tonpm ci(no--ignore-scripts) once the lockfile is regenerated in a separate cleanup PR.Not in this PR
Test plan
npm test -- tests/shared/mcp-http-server.test.ts(5/5)docker build+docker runsmokesrc/graph/extract/typescript.tsare pre-existing on origin/main; not from this PR)hivemind serve-mcpboots, missing env yields a clear messageSummary by CodeRabbit
New Features
serve-mcpCLI command for running the serverDocumentation