Skip to content

fix(coordinator-mcp): answer ping keepalive with empty result - #4412

Merged
Yeachan-Heo merged 1 commit into
Yeachan-Heo:devfrom
developjik:fix/coordinator-mcp-ping-keepalive
Aug 13, 2026
Merged

fix(coordinator-mcp): answer ping keepalive with empty result#4412
Yeachan-Heo merged 1 commit into
Yeachan-Heo:devfrom
developjik:fix/coordinator-mcp-ping-keepalive

Conversation

@developjik

Copy link
Copy Markdown
Contributor

handleJsonRpc handled initialize, tools/list, prompts/list, resources/list and tools/call but had no branch for the MCP ping method, so clients using ping as a liveness probe (e.g. Claude Code) received -32601 unknown_method:ping and kept reconnecting.

Per the MCP spec, ping MUST return an empty result {}. The pump's dispatch already routes ping as a control frame bypassing the data-concurrency cap (and pump.test.ts already assumes ping returns an empty result), but the handler never actually answered it.

Add the missing ping branch returning { result: {} } and a focused regression test against the real handleJsonRpc.

What

Why

Testing

GJC verdict

gajae.pr-review-verdict.v1 <merge-approved|merge-blocked|needs-human> sha256:<exact-head-or-diff-hash> reviewer:<architect|critic|human> evidence:<ci-run-url-or-local-command>

  • Target branch is dev
  • bun check passes
  • Tested locally
  • CHANGELOG updated (if user-facing)
  • Verdict above matches the exact PR head, not an earlier commit

@Yeachan-Heo
Yeachan-Heo force-pushed the fix/coordinator-mcp-ping-keepalive branch from 938c52c to cc379c1 Compare August 13, 2026 03:26
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Yeachan-Heo
Yeachan-Heo changed the base branch from main to dev August 13, 2026 03:26
@Yeachan-Heo
Yeachan-Heo force-pushed the fix/coordinator-mcp-ping-keepalive branch from cc379c1 to 6c88645 Compare August 13, 2026 03:29
@Yeachan-Heo

Copy link
Copy Markdown
Owner

Maintainer review — coordinator MCP ping keepalive

Base diagnosis

The original head 938c52c6 was based on main@d915ffe5, which has no valid integration lineage against dev CI. This PR has been retargeted to dev, and the contributor's single functional commit has been cherry-picked onto the current dev tip, preserving the original author attribution:

  • old head (main-based): 938c52c68359c7d1dde71db9763822145f975ed6 (author: @developjik)
  • intermediate head (dev 8d6784cb): ba3e55909ecc379c18e3
  • new head (dev 300e49ddf): 6c886456569a6a35f5e403937a6d38451111cfa9 (author preserved: developjik <developjik@users.noreply.github.com>)
  • rebased via --force-with-lease; main merge history was not replayed.

Correctness review

The fix adds a ping branch to handleJsonRpc returning { jsonrpc: "2.0", id, result: {} }. Verified against the MCP specification (2024-11-05 protocol version, the coordinator's negotiated version):

  • Valid ping requests only — gated on request.method === "ping"; all other methods fall through to tools/list, prompts/list, resources/list, tools/call, or -32601 unknown_method.
  • Exact request id preservedconst id = request.id ?? null mirrors the other handlers; numeric, string, and null ids round-trip faithfully.
  • Empty result {} — matches MCP spec: "ping MUST return an empty result".
  • No notification responses — the pump dispatch (pumpCoordinatorMcpStream) drops no-id frames before they reach the handler's response path, so ping notifications emit nothing.
  • No tools/data concurrency — the pump routes ping as control: true, bypassing the maxDataConcurrency cap; head-of-line blocking under a saturated data cap is explicitly tested.
  • No side effects — the ping branch is a pure return; it touches no coordinator state files, registers no tools, and writes no reports. Confirmed by a no-side-effect assertion.
  • Initialize/version/capability gating preservedinitialize remains the first branch and still returns protocolVersion, capabilities, and serverInfo; ping is placed after it and cannot bypass initialization.

Tests added

  • coordinator-mcp-server.test.ts: numeric id (original), string id round-trip, malformed/extra params ignored, no state-file side effects.
  • coordinator-mcp/pump.test.ts: ping request answered + ping notification (no id) emits nothing.

All existing pump tests (pumpCoordinatorMcpStream keepalive/overflow/drain/writer-robustness) continue to pass.

Verification

  • bun test packages/coding-agent/test/coordinator-mcp-server.test.ts -t "ping" — 5 pass
  • bun test packages/coding-agent/test/coordinator-mcp/pump.test.ts — 15 pass
  • bun --cwd=packages/coding-agent run check:types — clean

Thanks @developjik for the clean, spec-correct fix — it unblocks liveness probes (e.g. Claude Code) that were reconnecting on -32601 unknown_method:ping. The maintainer additions here are test coverage only; the functional change is entirely yours.

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — spec-correct, minimal, well-tested. Approving on behalf of the maintainer review above.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6c88645656

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +5701 to +5702
if (request.method === "ping") {
return { jsonrpc: "2.0", id, result: {} };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add the ping fix to the coding-agent changelog

This changes user-visible MCP behavior for clients that use ping as a liveness probe, but the commit leaves packages/coding-agent/CHANGELOG.md unchanged, so the fix will be absent from the package's release notes. Add an entry under its ## [Unreleased] section.

AGENTS.md reference: AGENTS.md:L186-L186

Useful? React with 👍 / 👎.

handleJsonRpc handled initialize, tools/list, prompts/list,
resources/list and tools/call but had no branch for the MCP `ping`
method, so clients using ping as a liveness probe (e.g. Claude Code)
received `-32601 unknown_method:ping` and kept reconnecting.

Per the MCP spec, ping MUST return an empty result `{}`. The pump's
dispatch already routes ping as a control frame bypassing the
data-concurrency cap (and pump.test.ts already assumes ping returns an
empty result), but the handler never actually answered it.

Add the missing `ping` branch returning `{ result: {} }` and a focused
regression test against the real handleJsonRpc.
@Yeachan-Heo
Yeachan-Heo force-pushed the fix/coordinator-mcp-ping-keepalive branch from 6c88645 to b27fbe8 Compare August 13, 2026 04:33

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b27fbe8681

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +5701 to +5702
if (request.method === "ping") {
return { jsonrpc: "2.0", id, result: {} };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle ping in the compatibility entry point

When an integration calls the exported handleCoordinatorMcpRequest compatibility handler rather than createCoordinatorMcpServer().handleJsonRpc (the package wildcard exports make this module addressable), ping still falls through to unknown_method:ping. Add the same empty-result branch to that duplicate JSON-RPC dispatcher so all coordinator MCP entry points implement the keepalive fix consistently.

Useful? React with 👍 / 👎.

@Yeachan-Heo
Yeachan-Heo merged commit 3354698 into Yeachan-Heo:dev Aug 13, 2026
20 of 24 checks passed
Yeachan-Heo pushed a commit that referenced this pull request Aug 13, 2026
handleJsonRpc handled initialize, tools/list, prompts/list,
resources/list and tools/call but had no branch for the MCP `ping`
method, so clients using ping as a liveness probe (e.g. Claude Code)
received `-32601 unknown_method:ping` and kept reconnecting.

Per the MCP spec, ping MUST return an empty result `{}`. The pump's
dispatch already routes ping as a control frame bypassing the
data-concurrency cap (and pump.test.ts already assumes ping returns an
empty result), but the handler never actually answered it.

Add the missing `ping` branch returning `{ result: {} }` and a focused
regression test against the real handleJsonRpc.

Co-authored-by: developjik <developjik@users.noreply.github.com>
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.

2 participants