Repository navigation
fix(r011): gate TypeScript ping dispatch on a sibling MCP method - #377
Conversation
dheerajjha
left a comment
There was a problem hiding this comment.
Most of this is right, and the tests read like the proposal on #89. I merged it into current main (41cc690) and the suite passes, 1382. That includes #379's new check that a Python-only scan never loads the parser; your import inside the TS path keeps that true.
Then I ran R011 over the seven TypeScript repos from the #372 review and compared the dispatch findings against main:
| repo | finding | main | #377 |
|---|---|---|---|
| inspector | clients/mcpdo/src/daemon/server.ts:406 case "ping": |
fires | silent |
| mcp-remote | src/lib/protocol-era.ts:247 ping: {}, |
silent | fires |
| typescript-sdk | wire/rev2025-11-25/registry.ts:72, 109, 194 |
silent | fires |
| supergateway | src/lib/combinedPeer.ts:142 |
fires | silent |
| supergateway | src/gateways/statelessRequest.ts:151, src/lib/childInitialization.ts:81 |
fires | silent |
The first three are improvements, or at least defensible:
- inspector. That switch is on
request.opnext todaemon/statusanddaemon/stop. It's the daemon's own IPC, so dropping it removes a false positive. - mcp-remote.
ANSWERED_LOCALLYis the proxy answeringpingfor old clients. That's real ping handling, and your identifier-key support is what found it. - typescript-sdk. The registry rows are the old revision's method tables, in a file main already flags for
PingRequestSchema.
The one I can't accept losing is combinedPeer.ts:
private async handle(request: Request): Promise<Params | undefined> {
const { method, params } = request
if (method === 'initialize') return this.initialize(params)
if (method === 'ping') return {}
...
if (method === 'logging/setLevel') return this.setLevel(params)That's a hand-written MCP dispatcher, which is exactly what R011 is for. It's written as consecutive early-return ifs rather than an else-chain, so each if is its own construct and ping never sees its siblings. Your #89 proposal said "if/else chain", and I approved it without thinking of this shape. That's on me, not you. The other two supergateway lines are a ternary and an || guard, not dispatch constructs. Going silent on those is the price of the gate, and I'm fine paying it.
Two changes, then:
- Count consecutive
ifs in one block as one construct when they test the same receiver, including each one's else-chain. I tried it locally by replacing theif_statementbranch with a pass overstatement_block/program/switch_casechildren (sketch below). It brings backcombinedPeer.ts:142and adds nothing else across the seven repos. The only test it breaks isif (method === "ping") ping(); if (method === "tools/list") list();, which should flip toTrue. - Declare the TypeScript gate in
boundaries. Add aFiresfor aninitialize/pingswitch with no SDK import, and aSilentfor the #89 table's health check (Clientimport plusif (method === "ping") runHealthCheck();), both withlanguage="typescript". #373 declared the Python gate that way, andtest_rule_boundariesruns them.
The sketch I measured with
elif node.type in {"statement_block", "program", "switch_case"}:
comparisons = []
for statement in children(node):
current = statement
while current is not None and current.type == "if_statement":
item = comparison(current)
if item:
comparisons.append((current, *item))
alternatives = children(current.child_by_field_name("alternative"))
current = alternatives[0] if len(alternatives) == 1 else None
for branch, name, value in comparisons:
if value == "ping" and any(
other_name == name and other_value in MCP_METHODS
for _, other_name, other_value in comparisons
):
yield branchNot blocking, but measured while I was in there:
- Parse errors. Silencing a file that doesn't parse costs little in practice. 3 of the 280 files containing
pingacross the seven repos have parse errors, all in the SDK's codemod transforms. - Speed. R011 over inspector's 723 TS files goes from 2.45s to 4.55s.
Syntax()builds a scope index this walk never reads, and a bareParser(...).parse(...)brings it to 3.85s. Take it or leave it.
0.22.0 goes out today with the R003 work and the Python half of R011. This will lead the next one, so there's no rush.
|
Consecutive if statements now share dispatch context when they test the same receiver, including else-chains, without duplicate findings. Covered by Added TypeScript boundaries for a no-import initialize/ping switch and an SDK-import health check, exercised by |
dheerajjha
left a comment
There was a problem hiding this comment.
That's the one. Your branch already carries 0.22.0, so I ran it as-is. The suite passes, 1388, and test_rule_boundaries[R011] passes with the two new TypeScript boundaries.
The same seven repos, dispatch findings against 0.22.0:
- Recovered. supergateway's
combinedPeer.ts:142(if (method === 'ping') return {}straight after theinitializebranch) fires again. - Stays gone. The
mcpdodaemon'scase "ping"onrequest.opis still dropped. - New, as before. mcp-remote's
ANSWERED_LOCALLYand the three rows in the SDK's old-revision registry. - Silent, as agreed. supergateway's ternary and
||lines. - Nothing else moved. That's exactly the set my sketch predicted, from a stricter rule than my sketch had.
On that strictness: you end a run at any statement that isn't an if, so if (…ping…) …; log(); if (…tools/list…) … stays silent. My sketch grouped the whole block. Yours is the more conservative of the two and still catches the real dispatcher, so I'd rather ship yours. Every-ping-once is a good test to have, too.
I regraded the board on this head: 23 entries, 0 drifts. Merging. With this in, both rules from #89's table are done.
Closes the TypeScript half of R011 in #89, using the approach agreed in #89 (comment).
What changes
In TypeScript, R011's
"ping"dispatch signal no longer depends on the file having MCP surface. It counts only when the same dispatch construct also names another MCP JSON-RPC method (initialize,tools/list,tools/call,resources/read, ...):switch: a siblingcasein the same switch (nested switches don't count).method,request.method, ...).ping: handler) and static computed (["ping"]) keys are accepted.String literals and static template literals (no
${}, no escapes) both count. Anything I can't resolve stays silent, for example aliases, substitutions, or a tree with parse errors. The analysis reuses the Tree-sitter helpers from_r003_typescript.py. Files that don't containpingare skipped before parsing.The table's repro (
Clientimport +if (method === "ping") runHealthCheck();) is now silent. A hand-writteninitialize/pingdispatcher fires even without an SDK import. Under the old file gate that case was missed, becauseinitializewasn't in the surface regex.Unchanged: the
PingRequest*identifier signal, the Python signals, severity, and messages.Python: left as #373 made it (file gate only). Sharing the sibling check would need a separate Python AST walk rather than reusing this code, so I kept this PR TypeScript-only, as you said was fine.
Tests
tests/test_r011_ts_dispatch.py(new, 25 cases), both directions:initialize/pingswitch with and without an SDK import; an if/else chain overrequest.methodwithtools/list; an object map with"tools/call"and"ping"; template-literal cases and comparisons; identifier and computed keys.case "ping":; nested switches; different receivers in one if/else chain; template literals with${}; an object with only apingkey; malformed TypeScript.tests/test_typescript.py::test_r011_finds_method_dispatch_in_typescriptpreviously relied on the file gate with a lonemethod === "ping". It now includes a siblingtools/listbranch so it still describes a real dispatcher.Checks
uv run pytest: 1381 passed (Windows, Python via uv).uvx ruff checkon the touched files reports the same six findingsmainalready has (import order and unused names intest_typescript.py, import order inr011_ping_removed.py). This change adds none.I wrote this with AI assistance, checked the behaviour and reviewed the diff myself.