feat(mcp): implement ping + logging/setLevel + notifications/message - #1350
Conversation
The MCP server advertised the `logging` capability but `logging/setLevel` returned -32601, and `ping` (a base-protocol utility) was unanswered. Both are now implemented, reconciling the advertised capability with real behavior. - ping: returns an empty result. Routed after session validation so a stale / expired / wrong-user session surfaces the normal 404/403 rather than being masked by an unconditional success; a ping notification gets the standard 202. - logging/setLevel: validates an RFC 5424 level and stores it. The level is persisted on the durable session record (system.mcp_session) so it survives an SSE reconnect, is order-independent of GET-stream open, and expires with the session TTL — no separate cache to leak. The live SSE record is seeded from it on (re)connect and updated in place on setLevel. - notifications/message: new logging.ts emitter delivers to a session over its SSE channel, filtered by the session's level (no messages before setLevel). Deliberately scoped to MCP-layer events — NOT the global harperLogger stream, which has no subscription hook and is process-wide/cross-worker (forwarding it would be a data leak + firehose). One call site wired: tools/call rate-limit rejections emit a `notice`. Known limitation (consistent with the existing listChanged channel): server push is per-worker in v1, so a setLevel POST handled on a different worker than the session's SSE stream takes effect on that stream only at the next reconnect. Cross-worker push is a subsystem-wide design item tracked in the MCP design-doc issue. Unit tests: logging level taxonomy + per-session filtering + profile fan-out; transport ping (valid/invalid-session/notification) and setLevel (valid, -32602, persistence, reconnect seeding). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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 implements the MCP logging utility supporting RFC 5424 syslog severities, including the logging/setLevel and notifications/message methods, as well as a ping handler for liveness checks. The review feedback suggests improving the robustness of the log level comparison in the admits function to prevent invalid log levels from being incorrectly processed, and simplifying the assignment of the session's log level when registering a session.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Reviewed; no blockers found. |
Address Gemini review on the logging PR: - admits(): reject an unrecognized level instead of defaulting its rank to 0 (which could slip past a 'debug' minimum). Both ranks must resolve. - handleGet: assign session.logLevel directly (a fresh record's level is already undefined), dropping the redundant guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… lastActivity handlePost called `await touchSession(session)` but discarded the returned copy (which carries the new lastActivity), leaving the local `session` stale. Any later save in the same request then rolled lastActivity back to the load-time value — pre-existing for `notifications/initialized` (handleInitialized) and now also `logging/setLevel` (dispatchSetLevel) added in this PR. Reassign `session = await touchSession(session)` so every downstream save persists the current activity time. Regression test forces a stale lastActivity and asserts setLevel advances rather than rolls it back. (TTL is unaffected either way — it keys off the record's put timestamp — but the field is now accurate.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review note: a Codex pass flagged that 🤖 AI-generated (Claude), posted by Kyle. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Clean MCP conformance implementation — ping session-validation placement is correct, log level persistence via durable session record is the right call. LGTM! 🟢
Reviewed by Claude Sonnet 4.6
3 participants
Implements the two MCP conformance items from the design-doc issue #1349:
pingand theloggingcapability (logging/setLevel+notifications/message).Why
initializeadvertisedlogging: {}, butlogging/setLevelreturned-32601— an advertise-but-don't-implement mismatch a conformant client (e.g. Claude Desktop) trips on.ping, a base-protocol liveness utility, was likewise unanswered. This reconciles the advertised capability with real behavior.What
ping→ empty result. Routed after session validation so a stale/expired/wrong-user session gets the normal 404/403 (not a success that masks an invalid session); a ping notification gets the standard 202.logging/setLevel→ validates an RFC 5424 level; persists it on the durable session record (system.mcp_session). Persisting (vs. an in-memory map) means it survives an SSE reconnect, is order-independent of GET-stream open, and expires with the session TTL — no separate cache to leak. The live SSE record is seeded from it on (re)connect.notifications/message→ newlogging.tsemitter; per-session level filtering (nothing beforesetLevel); delivered over the session's SSE queue. One call site wired:tools/callrate-limit rejections emit anotice.Where to look
components/mcp/logging.ts(new) — level taxonomy + emitter. Deliberately scoped to MCP-layer events, NOT the globalharperLoggerstream — Harper's logger has no subscription hook and its records are process-wide/cross-worker, so forwarding them to an authenticated client would be a confidentiality leak + unbounded firehose. Rationale in MCP server: complete the protocol surface (design doc — logging/ping conformance + prompts/completion/subscriptions/streaming) #1349 §3.components/mcp/session.ts— new persistedlogLevelfield (+ table attribute).components/mcp/transport.ts—ping/setLeveldispatch + seeding the live record on GET.Open item for the reviewer
Per-worker push (v1 limitation, not introduced here): server push is per-worker — a
setLevelPOST handled on a different worker than the session's SSE stream only takes effect on that stream at the next reconnect (the durable level is updated immediately). This is the same constraint the existinglistChangedchannel operates under ("cross-worker fan-out isn't attempted in v1"). Cross-worker push is a subsystem-wide design item tracked in #1349. Flagging since Codex raised it — it's a documented, deliberate scope boundary, not a regression.Reviews
Codex + Gemini cross-model review per the guidelines. Codex surfaced three P2s across passes (ping session-validation; in-memory level-map leak; pre-stream level loss) — all addressed (the persist-on-record design resolves the latter two). Gemini approved.
Docs
No docs change:
logging/pingare protocol capabilities, not user-facing config/CLI/schema. The broader feature roadmap lives in #1349.🤖 Generated by an LLM (Claude Opus 4.8).