fix(mcp): inject get_more_tools once, on the first tools/list page - #4967
Conversation
The SDK appended the virtual send_feedback tool to every tools/list page, so a cursor-following client saw it once per page. Now the tool is appended only to the first page (the request with no cursor; an empty-string cursor is a continuation page), which every client reads. Name conflicts are the MCP owner's responsibility. Detection is cheap and page-local: a real first-page tool with the name wins (the SDK warns, skips injection, and forwards its calls through the existing first-page probe), and a real tool on a later page is shadowed, with a warning logged when a client fetches that page. Both warnings point at the collectFeedback.toolName rename option. Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
Flatten the nested first-page/collision conditionals into one if/else-if chain keyed on an isFirstPage constant. Behavior unchanged. Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
Extract the feedbackOnFirstPageOnly predicate shared by three probe assertions, and give setupPaginatedServer one options object instead of two positional parameters. Behavior unchanged. Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
Give the missing-capability virtual tool the same first-page rule as send_feedback: append on the cursorless request only, let a real first-page owner win with a warning, and warn when a later page reveals a shadowed real owner. The shared isFirstPage constant is hoisted above both virtual-tool blocks so they cannot drift. Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
Prompt To Fix All With AI### Issue 1
packages/mcp/README.md:289-291
**Direct pagination guidance is incomplete**
The direct-dispatcher instruction only gates `collectFeedback`, but `prepareToolList()` also injects `get_more_tools` whenever `reportMissing` is true and cannot determine the current page itself. Callers following this guidance can therefore inject `get_more_tools` on every page. Instruct callers to enable both options only on the first page.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(mcp): inject get_more_tools once, on..." | Re-trigger Greptile |
marandaneto
left a comment
There was a problem hiding this comment.
The code looks good; no qualifying findings. Verdict: correct.
gesh
left a comment
There was a problem hiding this comment.
QA Swarm review complete. See inline comments.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 3 @ 003dbab)Rounds 2-3 re-reviewed the fix and refactor deltas: clean, danger LOW, no delegation. All round-1 findings are fixed and their threads resolved. Key findingsNone open. Fixed this iteration (commit Reviewer summaries
Previous rounds (2)round 1 @ b81e6a7 — ✅ APPROVE: 2 NITs (comment parity, test constant), both fixed in Automated by QA Swarm — not a human review |
…tant Also completes the direct-dispatcher pagination guidance in the README: prepareToolList() injects get_more_tools whenever reportMissing is true with no first-page gating of its own, same as collectFeedback, so the guidance needs to call out both options. Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
Hoist the listBothPages pagination-walk helper to file scope so the get_more_tools tests reuse it instead of inlining the two requests. Generated-By: PostHog Desktop Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
There was a problem hiding this comment.
Approved.
Contained bug fix to MCP tool-list pagination mirroring the already-merged send_feedback fix, with tests, a human approval, and agent-reviewer sign-off with no unresolved findings.
- Author wrote 0% of the modified lines and has 31 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 24L, 1F substantive, 111L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (111L, 4F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 003dbab · reviewed head 003dbab |
The PR was retargeted to a different base branch, so the approved diff is no longer what was reviewed. Stamphog re-reviews automatically.
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
Gates denied this PR: it has unresolved merge conflicts against the base branch, and it was classified above the auto-approval size/scope ceiling. The package.json script edits themselves look benign (just adding a new probe script to existing test commands), but the merge-conflict and tier failures are hard blockers regardless of code quality.
- lucasheriques reviewed the current head.
- Merge conflicts present against the base branch — must be resolved before this can be merged
- Classified as T2-never (733 lines / 8 files, two areas) — exceeds the size/scope ceiling for automated approval regardless of review quality
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | merge conflicts present |
| deny-list | ✗ | matches: deps_toolchain (scripts/hooks changed in package.json) |
| size | ✓ | 498L, 4F substantive, 733L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (733L, 8F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 003dbab · reviewed head 003dbab |
…tools-first-page # Conflicts: # packages/mcp/README.md # packages/mcp/src/__tests__/tools-list-envelope.test.ts # packages/mcp/src/extensions/instrumentation.ts
* fix(mcp): inject virtual tools once, on the first tools/list page The SDK advertises two virtual tools into tools/list: get_more_tools (report_missing) and send_feedback (collect_feedback). A client concatenates every page into one list, so each may appear on exactly one page. get_more_tools had no page gate at all, so a cursor-following client saw it once per page. send_feedback went on the last page only, hiding it from every client that never follows nextCursor. Both now go on the first page -- the page every client reads -- matching @posthog/mcp. An empty-string cursor is a valid opaque cursor, so it reads as a continuation page rather than the first one. get_more_tools also had no collision handling anywhere: no warning, no shadow state, and unconditional interception on all four adapter paths, so a host tool named get_more_tools was silently swallowed and the agent got PostHog's canned reply. Both tools now share one kind-keyed resolver, so this cannot drift again: - A real tool owning the name on the first page wins: warn, skip injection, dispatch its calls normally. - A real tool appearing only on a later page is shadowed, since page one cannot see page two. Warn when that page is served. @posthog/mcp behaves the same way. - Ownership is also settled at call time, which covers a call reaching a process that never served a listing -- the ordinary multi-pod case. FastMCP and v2 MCPServer are asked via their tool registry; raw low-level servers have none, so the SDK asks the host's own tools/list handler, and only on a name match. - Configuring both virtual tools with one name is detected and warned about; missing-capability wins, as every call path already assumed. Warnings name the option that renames PostHog's tool and go to the posthog.mcp stdlib logger as well as the logger option, so a default-configured host actually sees them. Two places matched the missing-capability name literally rather than as configured: a renamed virtual tool picked up a conversation_id argument the default-named one never got, and a real tool named get_more_tools lost its context injection and its $mcp_intent. mutate_tool_schema now takes is_sdk_virtual_tool from its caller instead of guessing from the name. PostHogMCP.prepare_tool_call honours original_tool for the missing-capability tool as it already did for feedback. Ports PostHog/posthog-js#4953 and PostHog/posthog-js#4967. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * fix(mcp): never read an injected virtual tool back as a real one Review found two ways the SDK mistook its own virtual tool for a host tool. A host may return the SAME tools/list result object from every request -- a module-level constant, or its own cache. The appends mutated that object in place, so the next read of it showed PostHog's virtual tool sitting in what looked like the host's catalogue. The SDK reported a collision against itself, stopped intercepting, and handed the agent "Unknown tool: send_feedback" instead of recording its feedback. Appends now build a copy and the adapters serve that, so every read of a listing is a faithful view of what the host wrote. Reported by @marandaneto, with a reproduction. On the PostHogMCP path, _is_virtual_tool_name matched on the name alone, in both directions: a host's own tool called get_more_tools was skipped for context injection and silently lost its $mcp_intent, while a host re-preparing an already-prepared list had PostHog's descriptor treated as a host collision -- warning about ourselves. Both now compare against the descriptor the SDK would build for that name; the description is ours and, unlike the schema, survives the model-injection pass. Flagged by Greptile. Also stop re-invoking the host's tools/list handler from the call path once a listing has been observed: its collision state already carries the answer. The probe now runs only while this process has served no listing -- the multi-pod case it exists for -- so a stateful or expensive handler is not re-entered on every virtual-tool call. Flagged by Greptile. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * chore(mcp): trim the changeset to what users need to know Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * chore(mcp): name which renamed tool got a stray conversation_id Only the missing-capability name was matched literally; the feedback name already resolved correctly, so a renamed send_feedback was never affected. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * fix(mcp): drop the dead observed_listing flag and pin the probe cost `observed_listing` was declared and read but never assigned, so the guard never activated. My reply on #962 claimed otherwise -- the claim was wrong, not the code. Keep the per-call behaviour, which is the correct one, and fix the text instead. `virtual_tool_collisions` is per-server state rewritten by whichever listing ran last, so a raw server serving different catalogues to different callers would answer one caller from another's listing. Asking per call cannot go stale that way, and `@posthog/mcp` probes per call for the same reason. Gating on a first-listing flag would also have falsified the README sentence that the call-time checks are the reliable signal for per-caller tool sets. test_listing_stops_the_call_path_reprobing_the_host could not fail: the probe closes over the original handler at wrap time, so replacing the `request_handlers` entry observed nothing. Replaced with a test that counts inside the host's own handler and pins the actual cost -- one probe per call to a virtual-tool name, none for ordinary tool traffic. Writing it turned up a fourth invocation that is the MCP SDK's own `req is None` cache repopulation on a call to an unlisted name, so the test excludes that rather than miscounting it as ours. README now states the real cost and offers renaming as the way to avoid the check. Reported by veria-ai and QA Swarm on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * chore(mcp): claim only the context injection, not the intent A real tool of yours named `get_more_tools` now gets `context` injected unconditionally, but whether its value is captured as `$mcp_intent` still depends on this instance having served a tools/list. That half belongs with the attribution work in the follow-up PR, so don't claim it here. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * fix(mcp): delegate the call when ownership cannot be determined The call-time ownership check has three outcomes, not two: the host owns the name, the host does not, or the question could not be asked -- a raw low-level server whose own tools/list handler is failing. The third collapsed into "does not own", so the SDK intercepted. That is the wrong direction. Guessing toward interception breaks the host's tool, silently, for as long as their handler stays unwell, to protect an analytics affordance. Guessing toward delegation costs one visible failed call to a tool of PostHog's, which nothing depends on. An analytics SDK does not get to break the product it measures, which is the rule the rest of this package already follows. `raw_listing_owns_tool_name` now returns Optional[bool], and the four call sites intercept only on a definite False. Registry lookups are unchanged: "not found" there is an answer, not a failure. The host gets a warning naming the tool and the reason. Also removes a divergence from @posthog/mcp, which gates on `isToolAdvertised(...) === false` and delegates for the same reason. My earlier reply on this PR claimed we already matched it; we did not. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * docs(mcp): trim the changeset and README to what a host needs The changeset listed five bullets, two of them restating internals. Cut to the three things a host upgrading would notice, plus the listed_tool_names shift for anyone charting it. The README section repeated the warning texts the code already emits and the option snippet twice. Cut to the rules and their limits. Also one real inconsistency in the code: a page's tool names were gathered two ways, once through a helper and once inline, and the helper's name shadowed the unrelated $mcp_listed_tool_names event field. Now one helper, named advertised_tool_names. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6 * fix(mcp): stop the ownership probe corrupting the tool cache The call-time ownership probe added in this branch runs the host's own tools/list handler. When that handler is registered through the SDK's `@server.list_tools()` decorator, mcp 1.x rebuilds `Server._tool_cache` from the tools it returns on every invocation - and the probe, unlike the wrapper's `req is None` branch, never re-ran schema injection afterwards. That cache is what the SDK validates real tool arguments against, so one ordinary `get_more_tools` call left every later call to a real tool rejected with "Additional properties are not allowed ('context' was unexpected)" for sending the argument we advertised, until the next client-facing listing healed it. An analytics SDK breaking the host's tools is the failure this branch exists to prevent. The probe also closed over the handler captured at instrument() time, so a host registering tools/list afterwards had ownership answered from a catalogue no client ever sees - a confident wrong answer that swallowed their real tool. `@posthog/mcp` re-captures the handler for this reason; read the current one instead and report the question as unanswerable when it is no longer ours, so the call is delegated. Both paths are covered by tests that fail without the fix. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65 * refactor(mcp): decide virtual-tool injection from the page, not stored state `virtual_tool_collisions` existed to carry a listing's answer to the call path. It was not needed for either job. At listing time the page's own tools are already in hand, so the decision is local: on a first page inject the name or warn that a real tool has it, on a later page warn that ours already shadows theirs. Four branches, no state - the shape `@posthog/mcp` uses. At call time every site already gates on a stronger signal than the set ever was: the tool registry on FastMCP and v2 MCPServer, the listing probe on raw low-level servers. Those answer correctly in a process that never served a listing, which is exactly where the set was empty and so silently wrong - the multi-pod case it was meant to cover. Names now resolve through `enabled_virtual_tool_names` everywhere, which keeps the fix that matters: with `report_missing` off, a real tool called `get_more_tools` is an ordinary tool and keeps its `context` captured as intent. Attribution for a real tool that *collides* with a virtual tool's name - `$mcp_intent` and `$mcp_conversation_id` - is dropped here and belongs to the stacked follow-up, which can read it off the ownership answer the call path already computes rather than from state that goes stale. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65 * fix(mcp): never answer "not yours" from a lookup that failed Three fixes from review of the two commits before this one. `_name_owned_by_real_tool`'s standalone-fastmcp branch caught every exception from `get_tool` and answered False - "no real tool owns this name" - on the assumption that an unknown name raises. In fastmcp 3.x it does not: `get_tool` returns None for an unknown name and raises only when the lookup itself fails, and its provider chain can reach a mounted or proxied upstream over the network. So a connection blip was read as "the name is free", and a call to a host tool that shares a virtual tool's name was swallowed and answered with PostHog's canned reply and isError=False - a fabricated success over a tool that never ran. Until the commit before this one a listing-derived collision flag made interception impossible for a name the host owned, which masked this. Removing that flag left the lookup deciding alone. Catch fastmcp's not-found and disabled errors as a real answer; delegate on anything else, which both call sites already do for None. The continuation-page "shadowed" warning no longer fires for a name a first page already blocked. Nothing of ours is advertised in that case and the host's tool runs, so telling them "the real tool will not run" sent them chasing a bug that is not there. The foreign-handler guard now covers a removed handler as well as a replaced one, and says so once. The virtual tools stay advertised when another layer wraps tools/list - a chained wrapper still runs our injection - but nothing behind them is ever intercepted, and a silent stop is invisible in the captured data. The README already promised this was logged. Also corrects a `raw_listing_owns_tool_name` docstring that described an approach never taken (reading `Server._tool_cache`), which would have invited removing the re-injection that repairs it, and sweeps five comments describing the deleted collision state. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65 * fix(mcp): don't warn that a tool we never injected is shadowing theirs Three review fixes. The continuation-page "shadowed" warning skipped a name a first page had blocked, but not one the other virtual tool had won under the duplicate name check. Configure both tools to one name and a host tool by that name on a later page drew two warnings, one of them for a tool that was never advertised. Skip any kind that did not make it onto the first page, whichever way it lost. The foreign-handler warning covers a removed handler as well as a replaced one, so it says so, and both branches now have a test - the removed one had none, which is why the wording went unnoticed. Also corrects the comment on the ownership lookup's except branch. It claimed a raised exception meant a mounted or proxied upstream had blipped. It cannot: fastmcp gathers its providers with `return_exceptions=True` and drops the failures, so an upstream blip reads back as a plain None, indistinguishable from "no such tool", and resolves to False without reaching that branch. What does reach it is the visibility, transform and auth work layered on top of the providers. The provider case is unchanged from before this SDK grew an ownership check - on the missing-capability path it is strictly better, since that path took no ownership check at all. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65 * docs(mcp): record why a degraded provider cannot be told from an absent tool A `FastMCP` is an `AggregateProvider`, which gathers its providers with `return_exceptions=True` and drops the failures, so an unreachable mounted or proxied sub-server reads back as a plain None from `get_tool` - indistinguishable from "no such tool". If that sub-server owned a real tool by a virtual tool's name, we treat the name as free and intercept. Worth writing down because the obvious fix does not work: the same failure is dropped from `list_tools` too, so a listing fallback returns the same blind answer, and fastmcp 3.x exposes no error strategy to opt out of. A later reader would otherwise rediscover this and add a probe that cannot help. Narrower than it first reads, which the note also records: during the outage the host's tool is missing from tools/list as well, so it could not have been dispatched either way. Only a provider that recovers between the check and dispatch loses a call that would have worked. Behaviour unchanged. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65 * refactor(mcp): collapse the virtual-tool appenders and trim the prose The kind-keyed resolver put per-tool policy in one place, but the layer below it stayed four copies. `append_get_more_tools`, `append_send_feedback` and their two v2 twins differed only by descriptor builder and `inputSchema` vs `input_schema`, and the block that called them was repeated verbatim in all three adapters. - One `append_virtual_tool_by_kind` behind one `virtual_tool_descriptor` switch, applied through one `apply_virtual_tool_injection`. The missing-capability-then-feedback order the `duplicate` rule depends on is now stated once instead of implied in three places. - Both raw probes use `advertised_tool_names` rather than a third copy of the same comprehension. - `VirtualToolInjection` wrapped a dict and two `.get`s that nothing reads any more, so the resolver returns the dict. - Dead branches out: the `missing_name or name` fallback the `is_missing_capability` property already rules out, the unreachable `options is None` guard in the feedback appenders, and an `event` looked up before a branch that never used it. One behaviour change. `_name_owned_by_real_tool` on the low-level adapter was tri-state — a lookup that raises means "could not answer", and the call is delegated — but the FastMCP and v2 registry paths still read every failure as "the name is free" and swallowed the host's own tool. All three now share the contract, with a test per adapter that makes the registry raise and asserts the real tool runs. Both fail without the change. Prose: the PR was adding roughly 0.7 lines of comment per line of code against ~0.2 in the same files. Cut the narrative, the `@posthog/mcp` commentary and the sentences that had reached three copies; kept the `_tool_cache` re-injection reason, the late-bound handler lookup, the empty-string cursor rule, the JS-parity warning and the provider note, which was stated twice and is now stated once. Tests: `_make_paged_lowlevel`, `_list_page`, `_call_request` and `_ECHO_TOOL` were defined in both virtual-tool test files; they move to `_helpers_lowlevel`, kept out of `_helpers` because that one loads under both SDK majors. Dropped one resolver test subsumed by its neighbours. 515 passed on mcp 1.x, 440 passed / 19 skipped on 2.x, mypy and the public API snapshot clean. Generated-By: PostHog Desktop Task-Id: b86d25ed-d899-4e29-a52f-cd0ab29ae38d * docs(mcp): cut the changeset to the page rule and the warnings Four bullets down to two sentences, matching the one-line house style of the changesets around it. Drops the claim that a real tool of yours sharing a virtual tool's name "now wins". That holds on the first page; on a later page this PR deliberately inverts it, so as a release note it would tell a host in that shape their tool is safe when it is not. The README and the PR body carry the full rule. What a reader needs from a changelog is that the names can collide and that PostHog says so, which the warning sentence gives them. Also drops the reused-result-object fix and the listed_tool_names note: both are repairs to behaviour this release introduces, invisible to anyone upgrading from the last one. Generated-By: PostHog Desktop Task-Id: b86d25ed-d899-4e29-a52f-cd0ab29ae38d * fix(mcp): keep one precedence when both virtual tools share a name `instrument()` already handled a host that configures `missing_capability_tool_name` and `collect_feedback.tool_name` to the same string: it drops the feedback tool, keeps missing-capability precedence, and warns "duplicate". The `PostHogMCP` dispatcher did neither. `prepare_tool_list` fell through to its "name already taken" branch, so the feedback descriptor went unadvertised with no warning. `prepare_tool_call` then set `is_feedback` and `is_missing_capability` both true, and the README's dispatch snippet tests `is_feedback` first, so every missing-capability call became a bogus $mcp_feedback capture -- the inverse of the precedence the other path sets on purpose. Also folds four copies of the ownership-probe warning into one helper, and names the three collision variants, so a typo is a type error rather than silently picking the "blocked" wording. Generated-By: PostHog Desktop Task-Id: 7047b34a-b562-40ab-ae65-8b0f24700e57
Problem
Stacked on #4953, which fixed per-page duplication for
send_feedback. The siblingget_more_toolsblock kept the old behavior: it appends the virtual tool to everytools/listpage, so a cursor-following client's concatenated listing carriesget_more_toolsonce per page.Changes
Give
get_more_toolsthe exact same first-page rule:get_more_toolsstill wins: warn, skip injection, forward its calls (the existing call-path check is unchanged).missingCapabilityToolNamerename option, now named in both warnings.isFirstPageconstant above both blocks keeps the two virtual tools from drifting again.get_more_tools(first-page-only injection; later-page shadowing with warning and interception). README and changeset updated.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
.changeset/mcp-get-more-tools-pagination.md)🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude in PostHog Desktop. Verified: 885 unit tests, all four MCP harness lanes green on both SDK majors, lint and build clean.
Created with PostHog Desktop