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..d1ed514a6abd --- /dev/null +++ b/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 { + 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..6e67f1f7fc55 --- /dev/null +++ b/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): 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 + + 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], + }, + }) + ) + }) +}) 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) => {