fix(mcp): inject send_feedback once, on the first tools/list page

[gesh]

Problem

The SDK appended the virtual send_feedback tool to every tools/list page. A cursor-following client concatenates pages, so it saw the tool once per page.

Changes

Append the tool once, at the end of the first page — the page every client reads, including clients that never follow cursors. "First page" means a request with no cursor; an empty-string cursor is a continuation page. A single-page catalogue keeps today's behavior at zero extra cost.

Name conflicts are the MCP owner's responsibility, with cheap page-local detection instead of a catalogue walk:

• A real tool named send_feedback on the first page wins: the SDK warns, skips injection, and forwards its calls (the existing first-page probe on the call path, unchanged).
• A real tool on a later page is shadowed. The SDK logs a warning when a client fetches that page.
• Both warnings point at the fix the SDK already ships:

instrument(server, posthog, {
  collectFeedback: { toolName: 'posthog_feedback' },
})

The call path, get_more_tools, PostHogMCP, and the option surface are untouched.

• Tests: a paginated-catalogue suite (first-page-only injection, empty-string cursor semantics, first-page collision, later-page shadowing) plus real-server probes on both SDK majors, including a colliding paginated catalogue and a per-request factory.

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-feedback-pagination.md)

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Authored with Claude in PostHog Desktop. Reworked from an earlier revision that walked the whole catalogue before every injection and interception; the walk, its budgets, and its fail-safe plumbing are gone in favor of the owner-responsibility contract above. Verified: 883 unit tests, all four harness lanes green on both SDK majors, lint and build clean.

---

Created with PostHog Desktop

[greptile-apps]

Prompt To Fix All With AI

### Issue 1
packages/mcp/src/extensions/instrumentation.ts:853
**Collision state leaks across clients**

If a long-lived server returns role- or session-specific tool catalogues, one client that lists a real `send_feedback` tool sets this server-wide flag permanently. Other clients then lose virtual-tool injection and feedback-call interception even when their catalogues have no collision. Concurrent page requests can also process the final page before the colliding page and advertise both tools. Scope collision state to each session or list traversal, and reset it when a new enumeration begins.

### Issue 2
packages/mcp/harness/dual-era/probe-feedback.mjs:136
**Probe leaves fetch failures unhandled**

The new `fetch()` and response parsing can reject without a `catch`, so the probe exits with an unstructured exception instead of recording a failed check. The repository requires asynchronous calls that may fail to be handled with `try/catch`. Wrap this request and parsing path and report failures through `check`; this requirement must be satisfied before merging.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (1): Last reviewed commit: "test(mcp): add a plain collectFeedback l..." | Re-trigger Greptile}

[gesh]

Note

🤖 Automated comment by QA Swarm — not written by a human

QA Swarm review complete — see inline comments. Verdict and evidence in the pinned summary comment.

[gesh]

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 WITH NITS _{(round 6 @ 3a28019)}

The PR was rewritten from scratch on top of main: the whole-catalogue ownership walk (and its budgets, loop guards, _meta forwarding) is gone, replaced by an owner-responsibility contract — first-page-only injection, page-local collision detection, and the collectFeedback: { toolName } rename as the documented fix. Round 6 reviewed the new ~585-line diff as a fresh change: LOW danger, HIGH confidence, no delegation. One MEDIUM finding, out of scope but worth tracking.

Key findings

• 🟡 MEDIUM — get_more_tools still injects on every page (instrumentation.ts:827–841). The sibling block above the fix has no first-page guard, so paginated catalogues still duplicate get_more_tools per page — the defect class this PR fixes for send_feedback. Pre-existing, not a regression; moot if reportMissing is removed as planned, otherwise worth a follow-up.

Superseded by the rewrite

Round 4/5's three open findings (final-page single-page-client visibility, get_more_tools check asymmetry, ownership-cache leak across catalogues) all targeted the catalogue-walk implementation, which no longer exists. The new revision injects on the first page (resolving the single-page-client concern directly), performs no walk anywhere, and caches ownership only from served pages.

Convergence

None — single reviewer round.

Reviewer summaries

Reviewer	Assessment
🧭 router (round 6, sonnet)	LOW danger, HIGH confidence, no delegation. Traced both dispatchers through the shared getTracedToolsList, verified cursor-presence semantics ("" = continuation), and ran the suites: tools-list-envelope 12/12, probe-pagination 18/18, probe-feedback 13/13.

Previous rounds (5)

round 1 @ 1a35a9c — ⚠️ REQUEST CHANGES: 1 HIGH (sticky-flag lifetime), 3 MEDIUM, 2 LOW, 1 NIT. Six threads actioned in 52c2974.
round 2 @ 52307c7 — 💬 APPROVE WITH NITS: 1 MEDIUM, 1 NIT. Both fixed in 97e4020.
round 3 @ 5e0df8b — 💬 APPROVE WITH NITS: no new findings; oxfmt CI fix landed in 5e0df8b.
round 4 @ 74d32cb — 💬 APPROVE WITH NITS: 2 MEDIUM, 2 LOW, 1 NIT; two closed, three carried.
round 5 @ 6958b32 — 💬 APPROVE WITH NITS: delta-only review, nothing new; three round-4 findings left open, all later superseded by the round-6 rewrite.

---

Automated by QA Swarm — not a human review

[gesh]

Note

🤖 Automated comment by QA Swarm — not written by a human

QA Swarm round 2 — reviewed the delta 1a35a9c..52307c7. The refactor is behaviour-preserving and every check is green; two findings inline. Verdict in the pinned summary.

[veria-ai]

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 2 · PR risk: 0/10

[gesh]

Review feedback is addressed in 74d32cb. The open decision on the collision-flag lifetime is resolved with option 2: the flag is gone, and collision state is scoped to each list traversal.

How it works now

• isToolAdvertised follows nextCursor through the whole raw catalogue, with a 100-page cap. A real owner on any page keeps its calls, so the call path needs no memory of earlier listings.
• The final tools/list page re-walks the raw listing when the request came through a cursor. A real owner on any earlier page blocks injection. A single-page catalogue is checked locally, with no extra handler calls.
• MCPAnalyticsData.feedbackToolShadowed is removed. No state persists between requests, so one client's filtered catalogue cannot suppress the virtual tool for other clients, and a per-request factory gets the same verdict as a long-lived instance.

Thread disposition

• Greptile P1, Veria "cross-client feedback suppression", QA Swarm HIGH (sticky-flag lifetime): fixed as above. The QA Swarm reproduction (3 pages, real owner on the middle page, per-request factory) is now a harness fixture in probe-pagination.mjs and passes on both majors.
• Greptile P2 (unhandled fetch in probe-feedback.mjs): this was fixed in 5e0df8b, before this round. The fetch and the parse sit in a try/catch and report through check.
• QA Swarm MEDIUM (tests assert a reused-instance boundary): the boundary no longer exists. The test comments now say so, and two new unit tests exercise fresh instances per request.
• QA Swarm LOW (probe cannot catch the per-request defect): the colliding-catalogue fixture closes this. Its listing check fails against the previous commit on v2.
• get_more_tools stays per-page by design; the code comment in getTracedToolsList records why. The non-paginating-client trade-off stays documented in the README.

Divergence from posthog-python: the Python SDK keeps a per-instance flag. The JS SDK cannot, because per-request factories are a first-class JS topology, so it asks the catalogue instead. The observable contract (virtual tool once, on the final page; a real owner always wins) is the same.

Cost note: the re-walk runs only on the final page of a genuinely paginated enumeration, and on calls that use the configured feedback tool name. Both are rare next to ordinary tool calls.

Verified locally: 882/882 unit tests, probe-pagination 8/8 and probe-feedback 6/6–7/7 on both majors, lint and format clean.

[stamphog]

Not approved yet — waiting on the conditions below.

Re-add the stamphog label to request another review once you have addressed this.

Deterministic gates denied this PR (package.json script/hook edit tripped the deny-list, and size/scope classified it T2-never), so it cannot be auto-approved regardless of code quality. The package.json change itself only adds a new probe invocation to existing test scripts (no lifecycle hooks), but a prior automated review flagged a real cross-client state-leak issue that later commits appear to have redesigned away (re-walking the raw catalogue instead of caching a flag) — a human should confirm that fix is complete before merging, since this changes MCP tool interception/dispatch behavior.

• 👍 on the PR from greptile-apps[bot].
• Gate denial: package.json scripts changed (deny-listed), plus PR classified T2-never by size/scope
• Reviewer (Greptile P1, veria-ai) flagged collision-state leaking across clients/sessions; later commits appear to redesign the mechanism to re-walk the catalogue instead of caching state, but this should be confirmed by a human reviewer rather than assumed fixed
• No CODEOWNERS/ownership match found, so there's no clear owning-team assurance path

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✗	matches: deps_toolchain (scripts/hooks changed in package.json)
size	✓	497L, 9F substantive, 784L/12F incl. docs/generated/snapshots — within ceiling
tier	✗	classified as T2-never: T2-never (784L, 12F, two-areas, fix)
stamphog 2.0.0b4		.stamphog/policy.yml @ 74d32cb · reviewed head 74d32cb

[gesh]

Note

🤖 Automated comment by QA Swarm — not written by a human

QA Swarm round 3 — reviewed 424c45a..74d32cb (router on the cheap tier, one mandatory delegation to the stronger tier on the cursor-walk and the two SDK-major wrappers). The three findings from rounds 1 and 2 are confirmed fixed. Verdict and evidence in the sticky summary comment.

[stamphog]

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 (deny-listed package.json script edit plus a size/scope classification that requires human sign-off), and it modifies MCP tool-injection/collision-detection logic — risky surface. Automated reviewers (QA Swarm, Greptile, Veria) converged on approval after fixes, but one MEDIUM finding is explicitly left as an open product decision for a human ("needs your call"), so this needs a maintainer to make that call rather than auto-approve.

• 👍 on the PR from greptile-apps[bot].
• Gate denied: tier classified T2-never (770 lines/12 files) and deny-list matched a package.json script change — both require human review per policy.
• The package.json change itself is benign (adds a new integration probe to existing test:integration:sdk-v1/v2 scripts, no lifecycle hooks or new script keys).
• QA Swarm round 5 leaves an open design decision unresolved: whether injecting on the final page (current behavior) vs. first page is the right tradeoff for non-paginating clients — this is a product call, not a bug, but needs a human to confirm.

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✗	matches: deps_toolchain (scripts/hooks changed in package.json)
size	✓	482L, 9F substantive, 770L/12F incl. docs/generated/snapshots — within ceiling
tier	✗	classified as T2-never: T2-never (770L, 12F, two-areas, fix)
stamphog 2.0.0b4		.stamphog/policy.yml @ 6958b32 · reviewed head 6958b32

[lucasheriques]

MCP allows an empty string as a cursor. This check treats cursor: "" as the first page. The two nextCursor checks also treat "" as the end of the list.

I tested this case. The SDK adds send_feedback twice. If a later page contains a real tool with that name, the SDK handles its calls instead of the application.

Please check for null or undefined instead. Please also correct the test that describes empty cursors as invalid.

MCP cursor requirements

[gesh]

Fixed in 9e65f15. You are right, and the spec is explicit: "Don't make any determination based on cursor value other than whether a non-null value was provided (e.g. an empty string is a valid cursor and thus MUST NOT be treated as the end of results)."

Every cursor decision now turns on presence alone. Three sites changed: the first-page test is request.params?.cursor != null, the end-of-walk test is response.nextCursor == null, and the third folded into the merged function. The comment claiming a server never issues an empty cursor is gone.

The test that described empty cursors as invalid now pins the opposite, and two more cover the return direction:

• treats an empty-string cursor as a continuation page, not the first one
• reads an empty-string nextCursor as more pages, so a real owner beyond it blocks injection
• keeps a real owner's calls when only an empty-string cursor reaches its page

Your reproduction is also a harness fixture now, on both majors. Against the previous commit it reports exactly what you saw:

✗ v1 · empty-string cursor · send_feedback is listed once, on the first page
  {"pageOne":["page_one_tool","send_feedback"],"pageTwo":["page_two_tool","send_feedback"]}

[lucasheriques]

These page requests omit the original request’s _meta. A list handler can require fields from _meta, such as protocolVersion.

I tested a handler with this requirement. The first page succeeds. The internal request fails because protocolVersion is missing. The SDK then omits the feedback tool.

This test passes before the PR change and fails after it.

Please preserve the required metadata when the SDK requests more pages. Apply the same change to the check made before a tool call.

[gesh]

Fixed in 9e65f15. Confirmed: 2026-07-28 makes io.modelcontextprotocol/protocolVersion and clientCapabilities required on every request, and "A request missing any required field is malformed; the server MUST reject it with JSON-RPC error code -32602." A bare { cursor } probe cannot pass a handler that enforces that, and the walk's fail-safe then drops the tool.

Internal page reads now carry the triggering request's _meta. Applied on both paths, since the merged function takes the request.

progressToken is the one key left out. It opts the client's request into progress notifications, and an internal read must not report progress against it — clearest on the call path, where the token belongs to a tools/call and the read is a tools/list. A unit test pins that it stays behind.

One detail worth recording for whoever reads this next: the harness fixture keys on a vendor _meta key rather than a reserved one. SDK v2 lifts the reserved io.modelcontextprotocol/* keys out of params._meta into the envelope before a handler runs, so a fixture keyed on a reserved field would only exercise v1. Failing against the previous commit:

✗ v1 · _meta · injection survives a list handler that requires request metadata  ["page_one_tool"]
✗ v1 · _meta · the pre-call walk survives it too  {"content":[{"type":"text","text":"called: send_feedback"}]}

Same two on v2. Three unit tests cover the listing path, the call path, and the progressToken exclusion.

[lucasheriques]

Optional design suggestion:

Two functions share the rules for checking whether an application tool owns a name. One checks the first page. The other checks later pages and assumes that one page has already used part of the limit.

Please consider putting these rules in one function. Give it the original request and the first page, if available.

That function can check all pages, preserve metadata, and enforce the page limit. Callers would no longer need to manage these details.

[gesh]

Taken, in 9e65f15. Both defects above lived in the duplicated rules, so merging was the smaller change overall.

isToolAdvertised and the inline first-page scan are now one function:

async function findRealToolOwner(
  server: MCPServerLike,
  toolName: string,
  request: MCPRequestLike,
  extra: CompatibleRequestHandlerExtra | undefined,
  logger: LoggerFn,
  servedPage?: CompatibleToolsListLike
): Promise<boolean | undefined>

servedPage is the page the caller already holds — first-page injection passes the response it is about to serve. Its tools are scanned in place, the walk starts at its own nextCursor, and it still spends one unit of the page budget, so the listing walk and the call-time walk reach the same distance. The tools/call path omits it and walks from the start. Callers no longer manage cursors, metadata or the budget.

One change beyond your three comments, flagged so it is visible rather than buried: the walk now has a 5-second budget beside its 25-page one. The page cap bounds how many reads it makes, not what they cost, and the README already notes that the first page does not reach the client until the walk ends. Expiry takes the fail-safe path that already existed, so there is no new branch.

Verification: 896 unit tests, both integration lanes green, lint and format clean. The seven new unit tests and three new probe fixtures all fail against the previous commit on both majors.

[stamphog]

Not approved yet — waiting on the conditions below.

Re-add the stamphog label to request another review once you have addressed this.

Deterministic gates denied this PR: it changed a package.json test script list (flagged by the toolchain deny-list), and it exceeds the size/complexity ceiling for auto-review (packages/mcp/src/extensions/instrumentation.ts is a substantial, security-relevant rewrite of tool-call interception logic). Gate denials require a human decision regardless of the review activity already on the thread.

• 👍 on the PR from greptile-apps[bot].
• Gates denied: package.json test:integration scripts were edited (adding a new probe invocation) — not a malicious hook, but it trips the toolchain deny-list and needs a human sign-off.
• Gates denied: change exceeds the size/complexity ceiling for auto-review (993 substantive lines, T2-never tier, two-areas scope) covering core MCP tool-listing/interception logic.
• The change is otherwise well-covered (QA Swarm rounds, Greptile, Veria security bot, and a human APPROVE from @lucasheriques), but that assurance doesn't override the hard gate denial.

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✗	matches: deps_toolchain (scripts/hooks changed in package.json)
size	✗	too large for auto-review (993L substantive in global — ceiling is 800L; 993L, 9F total, 1611L/12F incl. docs/generated/snapshots)
tier	✗	classified as T2-never: T2-never (1611L, 12F, two-areas, fix)
stamphog 2.0.0b4		.stamphog/policy.yml @ 9e65f15 · reviewed head 9e65f15

[gesh]

QA Swarm review complete. See inline comments.

[stamphog]

Not approved yet — waiting on the conditions below.

Re-add the stamphog label to request another review once you have addressed this.

Gates denied: the PR modifies package.json test-script entries (adding a new integration probe invocation) and exceeds the automatic size/complexity ceiling across two areas, so it can't be auto-approved even though the manifest change itself is benign (no lifecycle hooks, no lockfile-affecting deps). The change has substantial independent review (QA Swarm rounds, Greptile, Veria security bot all landed with fixes applied and no open concerns), but policy requires a human to sign off once gates deny on tier/size grounds.

• 👍 on the PR from greptile-apps[bot].
• Gate denial: tier classified T2-never due to size (660 lines, 7 files, two areas) and a deny-listed package.json scripts change (test:integration:sdk-v1/v2 entries), requiring human sign-off despite no lifecycle-hook or dependency additions.
• Extensive review history (QA Swarm, Greptile, Veria) shows all raised issues fixed, but no human reviewer has approved the current head.

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✗	matches: deps_toolchain (scripts/hooks changed in package.json)
size	✓	484L, 4F substantive, 660L/7F incl. docs/generated/snapshots — within ceiling
tier	✗	classified as T2-never: T2-never (660L, 7F, two-areas, fix)
stamphog 2.0.0b4		.stamphog/policy.yml @ 1194ac3 · reviewed head 1194ac3

[lucasheriques]

Re-approving at 1194ac3: traced listing-time vs call-time rules across page-1 owner, later-page owner, per-request factory, single page, and empty-string cursor; unit suite green on the stacked head (885 tests).