Skip to content

fix: implement MCP ping health checks - #600

Merged
bensig merged 1 commit into
MemPalace:developfrom
grtninja:codex/issue-584-mcp-ping
Apr 11, 2026
Merged

fix: implement MCP ping health checks#600
bensig merged 1 commit into
MemPalace:developfrom
grtninja:codex/issue-584-mcp-ping

Conversation

@grtninja

Copy link
Copy Markdown
Contributor

What

  • add base-protocol ping handling to mempalace.mcp_server.handle_request
  • return the MCP-spec empty result object {} for health checks
  • add a focused regression test for the ping dispatch path

Why

AnythingLLM v1.12.0 now sends ping before accepting an MCP server connection. MemPalace currently falls through to -32601 Unknown method: ping, which causes repeated restart loops and prevents the server from appearing in Agent Skills.

Validation

  • pytest tests/test_mcp_server.py -q
  • ruff check mempalace/mcp_server.py tests/test_mcp_server.py

Risk Notes

  • narrow protocol-compatibility fix only
  • no dependency changes
  • no tool behavior changes beyond supporting the standard health check

@web3guru888 web3guru888 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.

This is exactly the fix diagnosed in #584. The implementation is correct on all counts:

Spec compliance: is the right response body — per MCP spec §base-protocol/ping the result MUST be an empty object, not a string or acknowledgment.

Placement: The branch goes before and the final unknown-method fallthrough, which is exactly where it belongs in the dispatch chain.

Test coverage: verifies both uid=0(root) gid=0(root) groups=0(root) echo and the empty result object — that's the right pair of assertions for this dispatch path.

Scope: Two-line patch to production code, seven-line test addition. Appropriately minimal for a protocol-compatibility fix.

This unblocks AnythingLLM v1.12.0 users (and anyone else running a spec-compliant MCP host that issues health checks before accepting the server).

LGTM — no notes.

@web3guru888 web3guru888 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.

Minimal, correct, and well-placed.

The two-line patch is exactly right: ping gets its own elif branch before the notifications/initialized handler (both are protocol-level methods, so the grouping is logical) and returns the spec-mandated empty result object with the request ID preserved. The test covers the dispatch path and asserts both the ID round-trip and the empty result — nothing missing there.

One thing worth noting: the test imports handle_request inside the test body rather than at module level. That's consistent with the pattern used in the surrounding tests, so it fits in. No issue.

This unblocks AnythingLLM v1.12.0 users and any other client that sends a health-check ping before accepting the connection. Clean fix — LGTM.

@grtninja
grtninja force-pushed the codex/issue-584-mcp-ping branch from cae8460 to d85374b Compare April 11, 2026 06:33
@bensig
bensig changed the base branch from main to develop April 11, 2026 22:21
@bensig
bensig requested a review from igorls as a code owner April 11, 2026 22:21
@bensig
bensig merged commit 154e8a7 into MemPalace:develop Apr 11, 2026
6 checks passed
@grtninja
grtninja deleted the codex/issue-584-mcp-ping branch April 11, 2026 23:44
NickShtefan pushed a commit to NickShtefan/mempalace that referenced this pull request Apr 12, 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.

3 participants