From a3faef0c55677694f00bab6a447bd5794b47d109 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 12:41:35 +0000 Subject: [PATCH 1/3] fix(mcp): stop insight-update from reporting success when nothing persists The insight-update schema drops keys it does not declare. A nested or misnamed field reached the API as an empty PATCH, which saved the row, changed only updated_at, and returned success. A hand-written override now rejects a call with no field to change, and it checks the saved insight against the requested name, description, favorited, tags, and dashboards before it returns success. Co-Authored-By: Claude Opus 5.5 Generated-By: PostHog Desktop Task-Id: 52fe957e-c02e-4ed1-b620-cbebd01a7b05 --- services/mcp/src/tools/index.ts | 2 + services/mcp/src/tools/insights/update.ts | 92 +++++++++++++++++++ .../unit/insight-update-persistence.test.ts | 84 +++++++++++++++++ .../tests/unit/tool-name-validation.test.ts | 2 +- 4 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 services/mcp/src/tools/insights/update.ts create mode 100644 services/mcp/tests/unit/insight-update-persistence.test.ts diff --git a/services/mcp/src/tools/index.ts b/services/mcp/src/tools/index.ts index b2c5ad08f008..534fa8a90305 100644 --- a/services/mcp/src/tools/index.ts +++ b/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 ToolBase> = { // 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, diff --git a/services/mcp/src/tools/insights/update.ts b/services/mcp/src/tools/insights/update.ts new file mode 100644 index 000000000000..9e15497ca1b3 --- /dev/null +++ b/services/mcp/src/tools/insights/update.ts @@ -0,0 +1,92 @@ +/** + * 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' + +export 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[] => [...ids].sort((a, b) => a - b) + +/** Names the requested fields that the saved insight does not hold. `query` is not + * compared, because the API normalizes the query it stores. */ +export 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') + } + // API-token callers can stop receiving `dashboards`, so compare it only when the response has it. + if ( + params.dashboards !== undefined && + Array.isArray(saved.dashboards) && + JSON.stringify(sortedIds(params.dashboards)) !== JSON.stringify(sortedIds(saved.dashboards)) + ) { + mismatched.push('dashboards') + } + return mismatched +} + +export default function updateInsightVerifyingPersistence(): ToolBase { + 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 + }, + } +} diff --git a/services/mcp/tests/unit/insight-update-persistence.test.ts b/services/mcp/tests/unit/insight-update-persistence.test.ts new file mode 100644 index 000000000000..27416e67098e --- /dev/null +++ b/services/mcp/tests/unit/insight-update-persistence.test.ts @@ -0,0 +1,84 @@ +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): 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'], + dashboards: [1], +} + +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' }) + + await expect(tool.handler(createMockContext(request), params)).rejects.toThrow( + /does not hold the requested value for: name, tags\./ + ) + }) + + 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'], + dashboards: [2, 1], + }) + + 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 + + expect(result['name']).toBe('New name') + expect(request).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'PATCH', + path: '/api/projects/42/insights/7/', + }) + ) + }) +}) diff --git a/services/mcp/tests/unit/tool-name-validation.test.ts b/services/mcp/tests/unit/tool-name-validation.test.ts index 2ac4ba7048d7..676381c79837 100644 --- a/services/mcp/tests/unit/tool-name-validation.test.ts +++ b/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) => { From c5297656f04ca09de279170ba2328c9b9f651833 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 12:42:19 +0000 Subject: [PATCH 2/3] chore(mcp): keep insight-update helpers module-private Co-Authored-By: Claude Opus 5.5 Generated-By: PostHog Desktop Task-Id: 52fe957e-c02e-4ed1-b620-cbebd01a7b05 --- services/mcp/src/tools/insights/update.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/mcp/src/tools/insights/update.ts b/services/mcp/src/tools/insights/update.ts index 9e15497ca1b3..422a7db2f8b5 100644 --- a/services/mcp/src/tools/insights/update.ts +++ b/services/mcp/src/tools/insights/update.ts @@ -15,7 +15,7 @@ import { ToolInputValidationError } from '@/lib/errors' import { GENERATED_TOOLS } from '@/tools/generated/product_analytics' import type { Context, ToolBase, ZodObjectAny } from '@/tools/types' -export const UPDATABLE_INSIGHT_FIELDS = ['name', 'description', 'query', 'tags', 'favorited', 'dashboards'] as const +const UPDATABLE_INSIGHT_FIELDS = ['name', 'description', 'query', 'tags', 'favorited', 'dashboards'] as const type UpdateParams = { id: number | string @@ -35,7 +35,7 @@ const sortedIds = (ids: number[]): number[] => [...ids].sort((a, b) => a - b) /** Names the requested fields that the saved insight does not hold. `query` is not * compared, because the API normalizes the query it stores. */ -export function unpersistedInsightFields(params: UpdateParams, saved: Schemas.Insight): string[] { +function unpersistedInsightFields(params: UpdateParams, saved: Schemas.Insight): string[] { const mismatched: string[] = [] if (params.name !== undefined && normalizeText(params.name) !== normalizeText(saved.name)) { mismatched.push('name') From 0e3df84bbac8764c1bf1e35f827dc6d39e7e108d Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 12:58:34 +0000 Subject: [PATCH 3/3] fix(mcp): verify insight-update dashboards through dashboard_tiles The PATCH response can omit the deprecated `dashboards` field, so a dashboards-only update skipped the persistence check. The check now reads dashboard ids from `dashboard_tiles`, which the response always includes. The test also asserts the PATCH body. Co-Authored-By: Claude Opus 5.5 Generated-By: PostHog Desktop Task-Id: 52fe957e-c02e-4ed1-b620-cbebd01a7b05 --- services/mcp/src/tools/insights/update.ts | 16 +++++++++--- .../unit/insight-update-persistence.test.ts | 25 ++++++++++++++++--- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/services/mcp/src/tools/insights/update.ts b/services/mcp/src/tools/insights/update.ts index 422a7db2f8b5..d1ed514a6abd 100644 --- a/services/mcp/src/tools/insights/update.ts +++ b/services/mcp/src/tools/insights/update.ts @@ -31,7 +31,15 @@ type UpdateParams = { 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[] => [...ids].sort((a, b) => a - b) +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. */ @@ -53,11 +61,11 @@ function unpersistedInsightFields(params: UpdateParams, saved: Schemas.Insight): ) { mismatched.push('tags') } - // API-token callers can stop receiving `dashboards`, so compare it only when the response has it. + const dashboardIds = savedDashboardIds(saved) if ( params.dashboards !== undefined && - Array.isArray(saved.dashboards) && - JSON.stringify(sortedIds(params.dashboards)) !== JSON.stringify(sortedIds(saved.dashboards)) + dashboardIds !== undefined && + JSON.stringify(sortedIds(params.dashboards)) !== JSON.stringify(sortedIds(dashboardIds)) ) { mismatched.push('dashboards') } diff --git a/services/mcp/tests/unit/insight-update-persistence.test.ts b/services/mcp/tests/unit/insight-update-persistence.test.ts index 27416e67098e..6e67f1f7fc55 100644 --- a/services/mcp/tests/unit/insight-update-persistence.test.ts +++ b/services/mcp/tests/unit/insight-update-persistence.test.ts @@ -26,7 +26,8 @@ const saved = { description: 'Old description', favorited: false, tags: ['old'], - dashboards: [1], + // A session-authenticated PATCH response omits the deprecated `dashboards` field. + dashboard_tiles: [{ id: 11, dashboard_id: 1, deleted: false }], } describe('insight-update persistence check', () => { @@ -48,10 +49,16 @@ describe('insight-update persistence check', () => { 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' }) + 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\./ + /does not hold the requested value for: name, tags, dashboards\./ ) }) @@ -61,7 +68,11 @@ describe('insight-update persistence check', () => { name: 'New name', favorited: true, tags: ['growth', 'q3'], - dashboards: [2, 1], + 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({ @@ -78,6 +89,12 @@ describe('insight-update persistence check', () => { expect.objectContaining({ method: 'PATCH', path: '/api/projects/42/insights/7/', + body: { + name: ' New name ', + favorited: true, + tags: ['Q3', ' growth', 'q3'], + dashboards: [1, 2], + }, }) ) })