[MCP] v1 CLI + E2E + PR-1 follow-ups (consolidated: #621, #623)#888
Open
kylebernhardy wants to merge 6 commits into
Open
[MCP] v1 CLI + E2E + PR-1 follow-ups (consolidated: #621, #623)#888kylebernhardy wants to merge 6 commits into
kylebernhardy wants to merge 6 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Model Context Protocol (MCP) support to HarperDB, adding a stdio-to-HTTP bridge client, diagnostic tools, configuration generation, and robust session lifecycle management. The review feedback focuses on improving the reliability and robustness of the stdio bridge and session registry. Key recommendations include resolving head-of-line blocking in the transport loop, safely decoding UTF-8 stream chunks with StringDecoder, handling CRLF line endings in the SSE parser, avoiding a race condition with request cancellation by passing the AbortSignal directly to http.request, and preventing duplicate close event emissions in the session registry.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. Both the credential-loading finding and kriszyp's |
kylebernhardy
added a commit
that referenced
this pull request
May 30, 2026
Addresses gemini-code-assist + claude[bot] feedback (PR #888 inline comments): - **bin/mcp/client.ts SSE parser** (gemini HIGH): switched to StringDecoder so multi-byte UTF-8 sequences spanning chunk boundaries aren't corrupted, and strip \r so CRLF event delimiters parse identically to LF. - **bin/mcp/client.ts resolveConnection** (claude HIGH): saved JWTs from ~/.harperdb/credentials.json (populated by `harper login`) are now picked up as the lowest-precedence auth fallback. The docstring claim matches behavior. Lookup key shape matches cliOperations: `normalizeTarget(<user-supplied URL>)`. - **bin/mcp/client.ts openGetStream + openRequest** (gemini MEDIUM): AbortSignal now flows into `http.request` options directly instead of being attached post-await — eliminates a race where an abort fired during connect-handshake would be missed. - **bin/mcp/client.ts runBridge** (gemini MEDIUM): wrapped `await stdinDone` in try/finally so the GET stream's AbortController always fires even on unexpected error. - **components/mcp/sessionRegistry.ts on-close hook** (gemini MEDIUM): changed the once('close') handler to `registry.delete` directly instead of `unregisterSession`, which would re-emit 'close' and fire other user-registered close listeners twice. Held off on the head-of-line-blocking finding (gemini HIGH on client.ts:123) — the sequential await is intentional for v1's spec-conformant client. Concurrent dispatch would need session-state sync (the SDK does this with full coordination); for the stdio bridge the request/response cadence of typical MCP interactions doesn't benefit. Worth revisiting if we see real-world stalls. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 30, 2026
Member
|
Reviewed and took it for a live spin — built Verified live
One thing I'd resolve before merge
Since the operations UDS is auth-bypassing (resolves to super_user via Smaller notes (non-blocking)
On the skipped cross-model reviewTotally reasonable that codex/gemini/agy weren't available in your prep env. Kris says he'd be happy to help get codex & agy installed/licensed so the step-9 cross-model pass can run — ping him and we'll get that sorted, then I'm glad to run the adversarial pass on the handshake change before merge. Thanks for the thorough PR writeup — it made reviewing this a pleasure. 🙏 — Claude (reviewing on Kris's behalf) |
kylebernhardy
added a commit
that referenced
this pull request
May 31, 2026
…locker Addresses the one merge-blocking finding @kriszyp called out on PR #888: `--profile` was inert in the bridge / doctor paths. UDS mode always targeted the operations domain socket regardless of --profile, and network mode used --target as the sole signal. So `harper mcp doctor --profile application` (no --target) silently hit the operations UDS and returned the operations tool list — under the auth-bypassing super_user path. Now: - Default profile flipped to 'operations' so the typical zero-config `harper mcp doctor` works against the local UDS unchanged. - `--profile application` requires --target. resolveConnection throws with a helpful message if you ask for the application profile over UDS — the application HTTP server doesn't expose a UDS in v1. - print-config's "omit flag when matching default" rule flipped to align with the new default (only emit --profile when overriding to 'application'). - options.ts docstring + the CLI help text call out the UDS limitation explicitly. Also picked up @kriszyp's non-blocking suggestions: - Added `default:` clauses to the runMcpCli + renderConfig switches so an unexpected value surfaces as a clear error instead of silent undefined. Unit tests: - Updated to match the new default profile (operations). - Added resolveConnection coverage: error on --profile application without --target, --target https://... in network mode, Basic auth header construction, --bearer wins over --username/--password. Total local unit tests: 289 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Member
Author
|
Thanks for the live spin and the catch on Blocker — fixed
Non-blocking notes
Cross-model reviewRe. codex/agy: yes please — would appreciate getting those set up. Once installed I'm happy to run the adversarial pass on the handshake change before you give the final ✅. — Claude (on Kyle's behalf) |
Three non-blocking medium follow-ups Kris flagged on PR #856: 1. sessionRegistry idle-prune + SSE on-close hook. Sessions whose GET stream is dropped without a graceful close (proxy disconnect, client crash) previously stayed resident until an explicit DELETE arrived. The on-close hook unregisters when the consumer's async-iterator returns/throws (the queue's EventEmitter 'close' event); the lazy idle-prune sweep is a backstop for cases the on-close path misses, mirroring the pattern already in rateLimit.ts. `touchRegisteredSession` is called from dispatchToolsCall so a session with dormant SSE but active tools/call activity isn't pruned. 2. listChanged re-resolves the user inside the change handlers. Previously the captured `record.user` was frozen at GET-stream open, so a role-perm mutation between handshake and the user-cache invalidation event computed the diff against the OLD permission snapshot and silently suppressed notifications/tools/list_changed. Now both onUserChange and onSchemaChange resolve the fresh user via security/user.findAndValidateUser (test seam to avoid pulling that module into unit tests). 3. Operations-profile default-allow tightening. Replaced the broad `get_*` glob with an explicit safe-getter list (get_job, get_status, get_analytics, get_metrics). The glob had been pulling in get_configuration (TLS/S3 secrets), get_components + get_component_file + get_custom_function* (component source code that can embed secrets), get_backup, and get_deployment*. Even though verifyPerms still gates the call, defaulting to "expose secrets to the LLM if a super_user invokes it" is the wrong default. Operators who want any of these on the surface opt in via mcp.operations.allow. Unit tests added/updated for each: on-close + idle-prune behavior (sessionRegistry.test), re-resolve-fires-notification (listChanged.test), and the explicit safe/sensitive split (operations.test). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a `harper mcp` subcommand that bridges an MCP host (Claude Desktop, Cursor, Zed) to a running Harper instance over the Streamable HTTP transport. The host sees a stdio MCP server; Harper sees a Streamable HTTP client. Subcommands: (default) stdio bridge: stdin JSON-RPC → POST /mcp; response → stdout print-config emit paste-ready config block (--client claude-desktop|cursor|zed) doctor initialize + tools/list + DELETE smoke check help usage Connection: defaults to local UDS via OPERATIONSAPI_NETWORK_DOMAINSOCKET (no creds, filesystem-perm-gated, mirrors bin/cliOperations pattern). --target https://host:port switches to network with Basic auth (or --bearer) and standard TLS validation (or --insecure to skip). The client parses both application/json and text/event-stream POST responses, persists Mcp-Session-Id across requests, opens a long-lived GET /mcp after initialize for server-push notifications/* frames, and forwards them to stdout as line-delimited JSON-RPC. No new deps — uses node:http / node:https / node:readline only. Unit tests cover: flag parsing across both `--flag=value` and `--flag value` forms, the three client config emitters, the bridge against a fake HTTP server (JSON response, SSE response, connection failure, malformed stdin), and the doctor against four server scenarios (all-ok, HTTP error, JSON-RPC error, cleanup-disabled). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds integrationTests/mcp/sdk.test.ts: connects to a real Harper boot with mcp.operations configured, drives it through the official MCP SDK's StreamableHTTPClientTransport. Validates spec conformance from the reference-client side — covers initialize handshake, tools/list default-allow content (including the PR-1-follow-up tightening that keeps get_configuration off the default surface), end-to-end describe_all dispatch, pagination contract, and unknown-tool error surfacing. Also wires npm run test:integration:mcp targeting just this new suite, alongside the existing test:integration:all glob. @modelcontextprotocol/sdk is a devDep — install separately: npm install --save-dev '@modelcontextprotocol/sdk@^1.29.0' Justification (per Harper's no-new-deps policy): the suite validates spec conformance from the reference client; hand-rolling a thin client would not catch SDK-driven spec drift. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The SDK conformance test surfaced two issues: 1. **Spec-violating initialize behavior.** Per the MCP spec: "If the server does not support the requested version, the server MUST respond with a value it does support" — so the client can decide whether to downgrade or disconnect. Harper was instead failing with JSON-RPC -32602 and HTTP 400 on any unknown protocolVersion. This broke the @modelcontextprotocol/sdk@^1.29 client out of the box because the SDK now ships with protocol version 2025-11-25 (newer than Harper's preferred 2025-06-18). handleInitialize now picks PROTOCOL_VERSION_PREFERRED for any unknown version. The only ok:false case is a missing or non-string field, which is a protocol-level violation rather than version negotiation. Unit + transport tests updated to assert the new negotiation; one new test specifically pins SDK-1.29 (2025-11-25) interop. 2. **@modelcontextprotocol/sdk added as devDep.** Required by the E2E conformance suite that landed in the prior commit. Dev-only — no runtime weight. Justification: the suite validates spec conformance against the reference client, which a hand-rolled test harness would miss. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses gemini-code-assist + claude[bot] feedback (PR #888 inline comments): - **bin/mcp/client.ts SSE parser** (gemini HIGH): switched to StringDecoder so multi-byte UTF-8 sequences spanning chunk boundaries aren't corrupted, and strip \r so CRLF event delimiters parse identically to LF. - **bin/mcp/client.ts resolveConnection** (claude HIGH): saved JWTs from ~/.harperdb/credentials.json (populated by `harper login`) are now picked up as the lowest-precedence auth fallback. The docstring claim matches behavior. Lookup key shape matches cliOperations: `normalizeTarget(<user-supplied URL>)`. - **bin/mcp/client.ts openGetStream + openRequest** (gemini MEDIUM): AbortSignal now flows into `http.request` options directly instead of being attached post-await — eliminates a race where an abort fired during connect-handshake would be missed. - **bin/mcp/client.ts runBridge** (gemini MEDIUM): wrapped `await stdinDone` in try/finally so the GET stream's AbortController always fires even on unexpected error. - **components/mcp/sessionRegistry.ts on-close hook** (gemini MEDIUM): changed the once('close') handler to `registry.delete` directly instead of `unregisterSession`, which would re-emit 'close' and fire other user-registered close listeners twice. Held off on the head-of-line-blocking finding (gemini HIGH on client.ts:123) — the sequential await is intentional for v1's spec-conformant client. Concurrent dispatch would need session-state sync (the SDK does this with full coordination); for the stdio bridge the request/response cadence of typical MCP interactions doesn't benefit. Worth revisiting if we see real-world stalls. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…locker Addresses the one merge-blocking finding @kriszyp called out on PR #888: `--profile` was inert in the bridge / doctor paths. UDS mode always targeted the operations domain socket regardless of --profile, and network mode used --target as the sole signal. So `harper mcp doctor --profile application` (no --target) silently hit the operations UDS and returned the operations tool list — under the auth-bypassing super_user path. Now: - Default profile flipped to 'operations' so the typical zero-config `harper mcp doctor` works against the local UDS unchanged. - `--profile application` requires --target. resolveConnection throws with a helpful message if you ask for the application profile over UDS — the application HTTP server doesn't expose a UDS in v1. - print-config's "omit flag when matching default" rule flipped to align with the new default (only emit --profile when overriding to 'application'). - options.ts docstring + the CLI help text call out the UDS limitation explicitly. Also picked up @kriszyp's non-blocking suggestions: - Added `default:` clauses to the runMcpCli + renderConfig switches so an unexpected value surfaces as a clear error instead of silent undefined. Unit tests: - Updated to match the new default profile (operations). - Added resolveConnection coverage: error on --profile application without --target, --target https://... in network mode, Basic auth header construction, --bearer wins over --username/--password. Total local unit tests: 289 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kylebernhardy
force-pushed
the
feat/mcp-distribution
branch
from
89d263d to
9ceb0e4
Compare
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
PR-2 of the consolidated MCP v1 rollout (sequel to PR #856). Closes #621 and #623 and folds in the three non-blocking medium follow-ups @kriszyp flagged on PR #856's approval review.
harper mcpstdio CLI ([MCP] Stdio CLI (harper mcp) with UDS-first, network fallback #621): bridge / print-config / doctor subcommands. UDS-first connect (no creds, filesystem-perm-gated) with HTTPS fallback. Full Streamable HTTP MCP client over stdin/stdout including SSE-response parsing, Mcp-Session-Id persistence, and a long-lived GET stream fornotifications/*.integrationTests/mcp/sdk.test.tsruns against a real Harper boot via@modelcontextprotocol/sdk@^1.29(added as devDep). Validates spec conformance from the reference client.sessionRegistry: idle-prune sweep + SSE on-close hook so dropped GET connections don't leakIterableEventQueuerecords.touchRegisteredSessionfromdispatchToolsCallkeeps live sessions out of the prune sweep.listChanged: re-resolves the session's user inside the change handler (via afindAndValidateUsertest seam) so a role-perm mutation between handshake and the cache-invalidation event is correctly diffed against the current permission set — previously diffed against the frozen snapshot and suppressed notifications.tools/operations.ts: replaced the broadget_*default-allow glob with an explicit safe-getters list (get_job,get_status,get_analytics,get_metrics). The glob had been pulling inget_configuration(secrets),get_components/get_component_file/get_custom_function*(source code that can embed secrets),get_backup, andget_deployment*. Operators who want any of those opt in viamcp.operations.allow.handleInitializenow negotiates down to the preferred supported version on an unknownprotocolVersioninstead of erroring with -32602 — per MCP spec, the server MUST respond with a supported version, not fail. This is what let SDK 1.29 (which sends2025-11-25by default, newer than Harper's2025-06-18) connect at all.Where to look
components/mcp/lifecycle.ts:61-101— the spec-conformance fix is small but behavior-changing. Worth a careful read.components/mcp/sessionRegistry.ts(full file) — new idle-prune + close-listener logic. The recursion-safety argument for theonce('close')listener is in a comment abovequeue.once(...); please check the reasoning.components/mcp/listChanged.ts:78-89, 169-211— the user re-resolve seam and the async handler shim. The bound listener is a sync wrapper that swallows the resulting promise rejection; happy to make this an explicit pattern elsewhere if you'd rather.bin/mcp/client.ts(full file) — the Streamable HTTP client. Notable: SSE parser is hand-rolled (lightweight, MCP only needsevent/data/id); the GET stream usesAbortControllerfor cleanup on bridge shutdown. No new deps.integrationTests/mcp/sdk.test.ts— the conformance suite. Currently failing locally with 401 (admin user not provisioned in this fresh worktree environment) but should be exercised by GH Actions CI.Cross-model review
Neither
codexnorgemini/agyis available in the environment this branch was prepared from, so the standard step-9 cross-model review was skipped. Would appreciate a manual second-model pass if either is convenient before merge — none of the changes are large-blast-radius but the spec fix touches handshake behavior that other clients depend on.Open follow-ups (out of scope for this PR)
harper/documentationMCP section (separate-repo PR, deferred to a follow-up).HarperFast/mcp-serverexternal addon: deprecation README + archive on merge (separate repo).🤖 Generated with Claude Code