Skip to content

fix(mcp): inject get_more_tools once, on the first tools/list page - #4967

Merged
gesh merged 8 commits into
mainfrom
posthog/mcp-get-more-tools-first-page
Sep 16, 2026
Merged

gesh merged 8 commits into
mainfrom
posthog/mcp-get-more-tools-first-page

Conversation

@gesh

@gesh gesh commented Sep 15, 2026

Copy link
Copy Markdown
Member

Problem

Stacked on #4953, which fixed per-page duplication for send_feedback. The sibling get_more_tools block kept the old behavior: it appends the virtual tool to every tools/list page, so a cursor-following client's concatenated listing carries get_more_tools once per page.

Changes

Give get_more_tools the exact same first-page rule:

const isFirstPage = request.params?.cursor == null   // hoisted above BOTH virtual-tool blocks

if (data.options.reportMissing) {
  if (isFirstPage && alreadyPresent) { /* warn + skip — real tool wins */ }
  else if (isFirstPage) { /* inject */ }
  else if (alreadyPresent) { /* warn: later-page owner is shadowed */ }
}
  • A real first-page tool named get_more_tools still wins: warn, skip injection, forward its calls (the existing call-path check is unchanged).
  • A real tool on a later page is shadowed — same owner-responsibility contract as fix(mcp): inject send_feedback once, on the first tools/list page #4953 — with a warning when a client fetches that page. The fix is the existing missingCapabilityToolName rename option, now named in both warnings.
  • Hoisting the shared isFirstPage constant above both blocks keeps the two virtual tools from drifting again.
  • Tests: two new paginated-catalogue cases for 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

  • @posthog/mcp (not in the template list)

Checklist

  • Tests for new code
  • Accounted for the impact of any changes across different platforms
  • Accounted for backwards compatibility of any changes (no breaking changes!)
  • Took care not to unnecessarily increase the bundle size

If releasing new changes

  • Changeset file added (.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

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
@gesh
gesh requested review from a team as code owners September 15, 2026 11:20
Generated-By: PostHog Desktop
Task-Id: 3b751ad5-c551-4ad2-b90b-e50438622b88
@gesh
gesh removed the request for review from a team September 15, 2026 11:22
@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor
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

Comment thread packages/mcp/README.md Outdated

@marandaneto marandaneto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code looks good; no qualifying findings. Verdict: correct.

@gesh gesh left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

QA Swarm review complete. See inline comments.

Comment thread packages/mcp/src/extensions/instrumentation.ts
Comment thread packages/mcp/src/__tests__/tools-list-envelope.test.ts Outdated
@gesh

gesh commented Sep 15, 2026 •

Copy link
Copy Markdown
Member Author

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 findings

None open. Fixed this iteration (commit 9c24a56): the missing twin comment on the get_more_tools shadowing branch, the hardcoded test constant (now imports GET_MORE_TOOLS_NAME), and a greptile-flagged README gap (the prepareToolList() first-page guidance now covers reportMissing too). Commit 003dbab hoisted the listBothPages test helper for reuse.

Reviewer summaries

Reviewer Assessment
🧭 router (sonnet) LOW danger, HIGH confidence across all 3 rounds. Verified hoist neutrality, branch parity with send_feedback, pinned warning substrings, and that the test refactor preserves every assertion.
Previous rounds (2)

round 1 @ b81e6a7 — ✅ APPROVE: 2 NITs (comment parity, test constant), both fixed in 9c24a56.
round 2 @ 9c24a56 — ✅ APPROVE: fix delta clean; greptile README finding fixed in the same commit.


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
@gesh gesh added the stamphog label Sep 15, 2026
stamphog[bot]
stamphog Bot previously approved these changes Sep 15, 2026

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

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

@lucasheriques lucasheriques left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving at 003dbab: same first-page rule as #4953 applied to get_more_tools, the two blocks now share isFirstPage, and the call-path probe stays in lockstep in every topology. 885 unit tests pass on this head.

Base automatically changed from posthog/mcp-feedback-pagination to main September 15, 2026 16:00
@stamphog
stamphog Bot dismissed their stale review September 15, 2026 16:00

The PR was retargeted to a different base branch, so the approved diff is no longer what was reviewed. Stamphog re-reviews automatically.

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

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

@stamphog stamphog Bot removed the stamphog label Sep 15, 2026
…tools-first-page

# Conflicts:
#	packages/mcp/README.md
#	packages/mcp/src/__tests__/tools-list-envelope.test.ts
#	packages/mcp/src/extensions/instrumentation.ts
@gesh
gesh merged commit 916e163 into main Sep 16, 2026
61 checks passed
@gesh
gesh deleted the posthog/mcp-get-more-tools-first-page branch September 16, 2026 06:44
gesh added a commit to PostHog/posthog-python that referenced this pull request Sep 17, 2026
* 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
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