fix(mcp): stop insight-update from reporting success when nothing persists

services/mcp/src/tools/index.ts

@@ -19,6 +19,7 @@ import submitFeedback from './feedback/submit'
 import { GENERATED_TOOL_MAP } from './generated'
 // Insights
 import queryInsight from './insights/query'
+import updateInsightVerifyingPersistence from './insights/update'
 // Links (utility — builds canonical app URLs from the frontend's route table)
 import generateAppUrl from './links/generate-app-url'
 import loopsReview from './loops/loopsReview'
@@ -100,6 +101,7 @@ export const TOOL_MAP: Record<string, () => ToolBase<ZodObjectAny>> = {
 
     // Insights
     'insight-query': queryInsight,
+    'insight-update': updateInsightVerifyingPersistence,
 
     // Links (utility — canonical app URLs so the model never hand-builds/mis-slugs entity links)
     'generate-app-url': generateAppUrl,

services/mcp/src/tools/insights/update.ts

@@ -0,0 +1,100 @@
+/**
+ * Hand-written override of generated `insight-update`. It refuses a call with no
+ * field to change, and it checks that the saved insight holds the requested
+ * values before it reports success. Everything else delegates to the codegen factory.
+ *
+ * The schema drops keys it does not declare, so a misnamed or nested field reaches
+ * the handler as an empty patch. The API accepts an empty PATCH, saves the row, and
+ * returns it with only `updated_at` changed, which reads as a success.
+ *
+ * Title, description, and scopes still resolve from the generated
+ * `insight-update` entry, not from this file.
+ */
+import type { Schemas } from '@/api/generated'
+import { ToolInputValidationError } from '@/lib/errors'
+import { GENERATED_TOOLS } from '@/tools/generated/product_analytics'
+import type { Context, ToolBase, ZodObjectAny } from '@/tools/types'
+
+const UPDATABLE_INSIGHT_FIELDS = ['name', 'description', 'query', 'tags', 'favorited', 'dashboards'] as const
+
+type UpdateParams = {
+    id: number | string
+    name?: string | null
+    description?: string | null
+    tags?: unknown[]
+    favorited?: boolean
+    dashboards?: number[]
+    [key: string]: unknown
+}
+
+// The API trims text fields and stores tags trimmed, lowercased, and deduplicated.
+const normalizeText = (value: unknown): string => (typeof value === 'string' ? value.trim() : '')
+const normalizeTags = (tags: unknown[]): string[] =>
+    [...new Set(tags.map((tag) => normalizeText(tag).toLowerCase()))].sort()
+const sortedIds = (ids: number[]): number[] => [...new Set(ids)].sort((a, b) => a - b)
+
+// The API can omit the deprecated `dashboards` field, but it always returns `dashboard_tiles`.
+function savedDashboardIds(saved: Schemas.Insight): number[] | undefined {
+    if (Array.isArray(saved.dashboard_tiles)) {
+        return saved.dashboard_tiles.filter((tile) => !tile.deleted).map((tile) => tile.dashboard_id)
+    }
+    return Array.isArray(saved.dashboards) ? saved.dashboards : undefined
+}
+
+/** Names the requested fields that the saved insight does not hold. `query` is not
+ *  compared, because the API normalizes the query it stores. */
+function unpersistedInsightFields(params: UpdateParams, saved: Schemas.Insight): string[] {
+    const mismatched: string[] = []
+    if (params.name !== undefined && normalizeText(params.name) !== normalizeText(saved.name)) {
+        mismatched.push('name')
+    }
+    if (params.description !== undefined && normalizeText(params.description) !== normalizeText(saved.description)) {
+        mismatched.push('description')
+    }
+    if (params.favorited !== undefined && params.favorited !== saved.favorited) {
+        mismatched.push('favorited')
+    }
+    if (
+        params.tags !== undefined &&
+        Array.isArray(saved.tags) &&
+        JSON.stringify(normalizeTags(params.tags)) !== JSON.stringify(normalizeTags(saved.tags))
+    ) {
+        mismatched.push('tags')
+    }
+    const dashboardIds = savedDashboardIds(saved)
+    if (
+        params.dashboards !== undefined &&
+        dashboardIds !== undefined &&
+        JSON.stringify(sortedIds(params.dashboards)) !== JSON.stringify(sortedIds(dashboardIds))
+    ) {
+        mismatched.push('dashboards')
+    }
+    return mismatched
+}
+
+export default function updateInsightVerifyingPersistence(): ToolBase<ZodObjectAny> {
+    const generated = GENERATED_TOOLS['insight-update']!()
+
+    return {
+        // Spreading the generated tool carries over any field codegen adds later.
+        ...generated,
+        handler: async (context: Context, params: UpdateParams) => {
+            if (!UPDATABLE_INSIGHT_FIELDS.some((field) => params[field] !== undefined)) {
+                throw new ToolInputValidationError(
+                    `insight-update received no field to change, so nothing was saved. Pass at least one of ${UPDATABLE_INSIGHT_FIELDS.join(', ')} at the top level, next to "id". The tool ignores any other key.`,
+                    { inputKeys: Object.keys(params) }
+                )
+            }
+
+            const result = await generated.handler(context, params)
+
+            const mismatched = unpersistedInsightFields(params, result as Schemas.Insight)
+            if (mismatched.length > 0) {
+                throw new Error(
+                    `insight-update sent the change, but the saved insight does not hold the requested value for: ${mismatched.join(', ')}. Call insight-get to read the current state before you retry.`
+                )
+            }
+            return result
+        },
+    }
+}

services/mcp/tests/unit/insight-update-persistence.test.ts

@@ -0,0 +1,101 @@
+import { describe, expect, it, vi } from 'vitest'
+
+import { ToolInputValidationError } from '@/lib/errors'
+import updateInsightVerifyingPersistence from '@/tools/insights/update'
+import type { Context } from '@/tools/types'
+
+function createMockContext(requestMock: ReturnType<typeof vi.fn>): Context {
+    return {
+        api: {
+            request: requestMock,
+            getProjectBaseUrl: (projectId: string) => `https://us.posthog.com/project/${projectId}`,
+        } as any,
+        stateManager: { getProjectId: vi.fn().mockResolvedValue('42') } as any,
+        env: {} as any,
+        sessionManager: {} as any,
+        cache: {} as any,
+        getDistinctId: async () => 'test-distinct-id',
+        trackEvent: async () => {},
+    }
+}
+
+const saved = {
+    id: 7,
+    short_id: 'AaVQ8Ijw',
+    name: 'Old name',
+    description: 'Old description',
+    favorited: false,
+    tags: ['old'],
+    // A session-authenticated PATCH response omits the deprecated `dashboards` field.
+    dashboard_tiles: [{ id: 11, dashboard_id: 1, deleted: false }],
+}
+
+describe('insight-update persistence check', () => {
+    const tool = updateInsightVerifyingPersistence()
+
+    it.each([
+        ['only an id', { id: 7 }],
+        ['fields nested under an undeclared key', { id: 7, insight: { name: 'New name' } }],
+        ['a misnamed field', { id: 7, title: 'New name' }],
+    ])('rejects %s without sending a PATCH', async (_label, input) => {
+        const request = vi.fn()
+
+        const params = tool.schema.parse(input)
+
+        await expect(tool.handler(createMockContext(request), params)).rejects.toBeInstanceOf(ToolInputValidationError)
+        expect(request).not.toHaveBeenCalled()
+    })
+
+    it('reports the requested fields that the saved insight does not hold', async () => {
+        const request = vi.fn().mockResolvedValue(saved)
+
+        const params = tool.schema.parse({
+            id: 7,
+            name: 'New name',
+            tags: ['new'],
+            description: 'Old description',
+            dashboards: [1, 2],
+        })
+
+        await expect(tool.handler(createMockContext(request), params)).rejects.toThrow(
+            /does not hold the requested value for: name, tags, dashboards\./
+        )
+    })
+
+    it('returns the insight when the saved values match the API normalization', async () => {
+        const request = vi.fn().mockResolvedValue({
+            ...saved,
+            name: 'New name',
+            favorited: true,
+            tags: ['growth', 'q3'],
+            dashboard_tiles: [
+                { id: 12, dashboard_id: 2, deleted: false },
+                { id: 11, dashboard_id: 1, deleted: false },
+                { id: 13, dashboard_id: 3, deleted: true },
+            ],
+        })
+
+        const params = tool.schema.parse({
+            id: 7,
+            name: '  New name ',
+            favorited: true,
+            tags: ['Q3', ' growth', 'q3'],
+            dashboards: [1, 2],
+        })
+        const result = (await tool.handler(createMockContext(request), params)) as Record<string, unknown>
+
+        expect(result['name']).toBe('New name')
+        expect(request).toHaveBeenCalledWith(
+            expect.objectContaining({
+                method: 'PATCH',
+                path: '/api/projects/42/insights/7/',
+                body: {
+                    name: '  New name ',
+                    favorited: true,
+                    tags: ['Q3', ' growth', 'q3'],
+                    dashboards: [1, 2],
+                },
+            })
+        )
+    })
+})

services/mcp/tests/unit/tool-name-validation.test.ts

@@ -14,7 +14,7 @@ describe('Tool name validation', () => {
     it('shadows only the generated tools it means to', () => {
         const shadowed = Object.keys(TOOL_MAP).filter((name) => name in GENERATED_TOOL_MAP)
 
-        expect(shadowed.sort()).toEqual(['session-recording-get', 'update-feature-flag'])
+        expect(shadowed.sort()).toEqual(['insight-update', 'session-recording-get', 'update-feature-flag'])
     })
 
     it.each(Object.keys(allTools))('%s — name matches map key, length, and pattern', (mapKey) => {