fix(mcp): harden send_feedback against integer-extra drift, error-log echo, and dispatcher shadowing

.changeset/mcp-feedback-hardening.md

@@ -0,0 +1,5 @@
+---
+'@posthog/mcp': patch
+---
+
+Harden feedback validation, error handling, and tool-name collision routing.

packages/mcp/README.md

@@ -263,14 +263,19 @@ const posthog = new PostHogMCP(process.env.POSTHOG_PROJECT_TOKEN, {
 return { tools: posthog.prepareToolList(myTools, { collectFeedback: true }) }
 
 // tools/call dispatcher
-const prepared = posthog.prepareToolCall(name, rawArgs)
+const originalTool = myTools.find((tool) => tool.name === name)
+const prepared = posthog.prepareToolCall(name, rawArgs, { originalTool })
 if (prepared.isFeedback) {
   posthog.captureFeedback({ report: prepared.feedbackReport!, ...identity })
   await myFeedbackBackend.record(prepared.feedbackReport!)
   return sendFeedbackResult() // or a custom text reply
 }
 ```
 
+`originalTool` must come from the application's tool list before `prepareToolList()` adds PostHog's
+virtual tools. This lets a real application tool with the configured feedback name win, including
+when `tools/list` and `tools/call` reach different server replicas.
+
 `send_feedback` covers what `reportMissing` covers — a capability gap is
 `feedback_type: "missing_capability"` — so new integrations should enable only `collectFeedback`.
 `reportMissing` and its `$mcp_missing_capability` event stay unchanged for existing users; enabling

packages/mcp/src/__tests__/feedback.test.ts

@@ -420,6 +420,36 @@ describe('collectFeedback (send_feedback virtual tool)', () => {
       await capture.stop()
     })
 
+    it('rejects a fractional number for a declared integer extra, accepting whole values', async () => {
+      const capture = new EventCapture()
+      await capture.start()
+      const onFeedback = vi.fn()
+      instrument(server, fakePostHog(), {
+        collectFeedback: { extraProperties: { score: { type: 'integer' } }, onFeedback },
+      })
+
+      await callTool(client, SEND_FEEDBACK, {
+        feedback_type: 'praise',
+        summary: 'Great tools.',
+        score: 3.5, // fractional — a JSON Schema `integer` does not accept it
+      })
+      await callTool(client, SEND_FEEDBACK, {
+        feedback_type: 'praise',
+        summary: 'Great tools again.',
+        score: 3, // whole — conforms
+      })
+
+      await new Promise((r) => setTimeout(r, 50))
+      const events = capture.findCapturesByEvent(PostHogMCPAnalyticsEvent.Feedback)
+      expect(events[0].properties.$mcp_feedback_score).toBeUndefined()
+      expect(events[1].properties.$mcp_feedback_score).toBe(3)
+      expect(onFeedback.mock.calls[0][0].extras).toEqual({})
+      expect(onFeedback.mock.calls[0][0].raw.score).toBe(3.5)
+      expect(onFeedback.mock.calls[1][0].extras).toEqual({ score: 3 })
+
+      await capture.stop()
+    })
+
     it('redacts credential-named keys inside nested extras', async () => {
       const capture = new EventCapture()
       await capture.start()
@@ -505,10 +535,14 @@ describe('collectFeedback (send_feedback virtual tool)', () => {
     it('falls back to the default reply when onFeedback throws, and still captures the event', async () => {
       const capture = new EventCapture()
       await capture.start()
+      const logged: string[] = []
       instrument(server, fakePostHog(), {
+        logger: (message: string) => logged.push(message),
         collectFeedback: {
           onFeedback: async () => {
-            throw new Error('backend down')
+            const error = new Error('backend down for jane@example.com')
+            error.name = 'jane@example.com\nforged log line'
+            throw error
           },
         },
       })
@@ -517,6 +551,14 @@ describe('collectFeedback (send_feedback virtual tool)', () => {
 
       expect(result.content[0].text).toContain('recorded')
 
+      // No part of the thrown value reaches the log. Both its message and its
+      // mutable name can contain agent-controlled PII or log-forging newlines.
+      const warning = logged.find((line) => line.includes('onFeedback handler threw'))
+      expect(warning).toBe('Warning: onFeedback handler threw; returning the default acknowledgement')
+      expect(warning).not.toContain('backend down')
+      expect(warning).not.toContain('jane@example.com')
+      expect(warning).not.toContain('forged log line')
+
       await new Promise((r) => setTimeout(r, 50))
       expect(capture.findCapturesByEvent(PostHogMCPAnalyticsEvent.Feedback)).toHaveLength(1)
 
@@ -701,6 +743,33 @@ describe('PostHogMCP (custom dispatcher path)', () => {
     await posthog.shutdown()
   })
 
+  it('a supplied originalTool wins a feedback-name collision', async () => {
+    const posthog = newClient({ collectFeedback: true })
+
+    const realTools = [
+      {
+        name: SEND_FEEDBACK,
+        inputSchema: { type: 'object', properties: { note: { type: 'string' } } },
+      },
+    ]
+    const listed = posthog.prepareToolList(realTools, { collectFeedback: true })
+    expect(listed).toHaveLength(1)
+
+    // The descriptor comes from the host's original list, not the prepared list.
+    // This is stateless proof that the real application tool owns the name.
+    const originalTool = realTools.find((tool) => tool.name === SEND_FEEDBACK)
+    const collided = posthog.prepareToolCall(SEND_FEEDBACK, { note: 'hi' }, { originalTool })
+    expect(collided.isFeedback).toBe(false)
+    expect(collided.feedbackReport).toBeUndefined()
+
+    // Without originalTool the name match stands.
+    const virtual = posthog.prepareToolCall(SEND_FEEDBACK, { feedback_type: 'other', summary: 'A note.' })
+    expect(virtual.isFeedback).toBe(true)
+    expect(virtual.feedbackReport).toBeDefined()
+
+    await posthog.shutdown()
+  })
+
   it('captureFeedback emits $mcp_feedback with the report properties and intent', async () => {
     const posthog = newClient({
       collectFeedback: { toolName: 'agent-feedback', extraProperties: { product_area: { type: 'string' } } },

packages/mcp/src/extensions/feedback.ts

@@ -184,7 +184,13 @@ function parseSentiment(value: unknown): FeedbackSentiment | undefined {
  */
 function matchesExtraSchema(value: unknown, schema: FeedbackExtraPropertySchema): boolean {
   const type = Array.isArray(value) ? 'array' : value === null ? 'null' : typeof value
-  if (schema.type !== type && !(schema.type === 'integer' && typeof value === 'number')) {
+  if (schema.type === 'integer') {
+    // A JSON Schema `integer` accepts a whole-valued number (`3`, `3.0`) but
+    // not a fractional one (`3.5`) — `typeof` alone can't tell them apart.
+    if (!Number.isInteger(value)) {
+      return false
+    }
+  } else if (schema.type !== type) {
     return false
   }
   return !Array.isArray(schema.enum) || schema.enum.includes(value as string)
@@ -341,8 +347,10 @@ export async function handleFeedback(
       if (typeof reply === 'string' && reply.trim()) {
         return { content: [{ type: 'text' as const, text: reply }] }
       }
-    } catch (error) {
-      logger(`Warning: onFeedback handler threw; returning the default acknowledgement - ${error}`)
+    } catch {
+      // The whole thrown value is host-controlled and mutable. Do not log any
+      // part of it because it can contain PII or log-forging newlines.
+      logger('Warning: onFeedback handler threw; returning the default acknowledgement')
     }
   }
   return sendFeedbackResult()

packages/mcp/src/extensions/posthog-mcp.ts

@@ -278,8 +278,12 @@ export class PostHogMCP extends PostHog {
    *
    * @example
    * ```ts
+   * const originalTool = myTools.find((tool) => tool.name === name)
    * const { intent, intentSource, llmModel, llmModelSource, args, isMissingCapability } =
-   *   posthog.prepareToolCall(name, rawArgs, { requestMeta: request.params?._meta })
+   *   posthog.prepareToolCall(name, rawArgs, {
+   *     originalTool,
+   *     requestMeta: request.params?._meta,
+   *   })
    * if (isMissingCapability) {
    *   posthog.captureMissingCapability({ context: intent, llmModel, llmModelSource, ...identity })
    *   return getMoreToolsResult()
@@ -304,7 +308,13 @@ export class PostHogMCP extends PostHog {
       ? resolveModel({ params: { arguments: args, _meta: options.requestMeta } }, ownsModel)
       : undefined
     const strippedArgs = stripContext(args)
-    const isFeedback = this.#feedbackOptions !== undefined && name === this.#feedbackToolName
+    // A supplied `originalTool` is a real application tool by this name (it
+    // comes from the host's own list, which never holds the virtual tool), so
+    // the real tool wins — the stateless twin of instrument()'s listing-derived
+    // collision handling. Without it the name match stands, and the documented
+    // remedy for a collision is configuring a non-colliding `toolName`.
+    const isFeedback =
+      this.#feedbackOptions !== undefined && name === this.#feedbackToolName && options.originalTool == null
     return {
       intent,
       intentSource: intent ? 'context_parameter' : undefined,

packages/mcp/src/types.ts

@@ -689,8 +689,11 @@ export interface PrepareToolListOptions {
 /** Options for {@link PostHogMCP.prepareToolCall}. */
 export interface PrepareToolCallOptions {
   /**
-   * The tool descriptor before PostHog preparation. Pass this on stateless or
+   * The tool descriptor before PostHog preparation, from the host's own tool
+   * list (the SDK's virtual tools never exist there). Pass this on stateless or
    * multi-replica servers so SDK argument ownership is resolved per request.
+   * Passing it also disambiguates a feedback-tool name collision: a real tool
+   * by that name is dispatched normally instead of being flagged as feedback.
    */
   originalTool?: { inputSchema?: unknown }
   /** The incoming `tools/call` request's `_meta`, used for recognized client model metadata. */
@@ -717,8 +720,10 @@ export interface PreparedToolCall {
   isMissingCapability: boolean
   /**
    * True when `name` is the `send_feedback` virtual tool AND the constructor's
-   * `collectFeedback` option is set. Always false without that opt-in, so a real
-   * tool that happens to use the name is never shadowed.
+   * `collectFeedback` option is set AND no `originalTool` was supplied. Always
+   * false without that opt-in — and a supplied `originalTool` proves a real
+   * application tool owns the name — so a real tool that happens to use the
+   * name is never shadowed.
    */
   isFeedback: boolean
   /**