From a2ff96b8126e880b2cd23e4b74e9678bf7911c2e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 24 Jul 2026 20:51:24 +0000 Subject: [PATCH] Keep unknown search domains and platform batch failures correctly classified Rebuilt on current main after #919 merged. #919 and this branch were written in parallel against the same files and both created caller-error.ts and the observability carve-out; #919 landed first, so everything it already covers is dropped here. What remains is the content unique to this branch: - searchUnified rejects an unknown domain with McpCallerError. Callers hit this by passing a package kody id such as "skills" where a capability domain is expected, which is a caller mistake, not a platform bug. - The entity-batch failure path only marks the batch callerError when every entry failed with a caller error, and passes a cause otherwise so genuine platform failures (for example a D1 read failing mid-batch) still reach Sentry as exceptions. Two tests cover both directions, guarding the carve-out against over-suppression. - resolveOwnedPackageSource uses the existing getEntitySourceByIdForUser helper instead of loading a source and filtering by user_id in app code, so the scoping lives in the query. --- .../packages/get-git-remote.node.test.ts | 39 ++++--- .../publish-external-push.node.test.ts | 10 +- .../packages/resolve-package-source.ts | 9 +- .../worker/src/mcp/observability.node.test.ts | 14 ++- packages/worker/src/mcp/tools/search-core.ts | 5 +- .../src/mcp/tools/search-handler.node.test.ts | 106 +++++++++++++++--- .../src/mcp/tools/search-tool-runner.ts | 24 ++-- .../worker/src/mcp/tools/search.node.test.ts | 25 +++-- 8 files changed, 164 insertions(+), 68 deletions(-) diff --git a/packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts b/packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts index 0eb70b3645..faddcb0a23 100644 --- a/packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts +++ b/packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts @@ -3,7 +3,7 @@ import { expect, test, vi } from 'vitest' const mockModule = vi.hoisted(() => ({ getSavedPackageById: vi.fn(), getSavedPackageByKodyId: vi.fn(), - getEntitySourceById: vi.fn(), + getEntitySourceByIdForUser: vi.fn(), resolveArtifactDefaultBranchHead: vi.fn(), resolveExistingArtifactSourceRepo: vi.fn(), createStubSavedPackage: vi.fn(), @@ -24,8 +24,8 @@ vi.mock('#worker/package-registry/package-owner.ts', () => ({ })) vi.mock('#worker/repo/entity-sources.ts', () => ({ - getEntitySourceById: (...args: Array) => - mockModule.getEntitySourceById(...args), + getEntitySourceByIdForUser: (...args: Array) => + mockModule.getEntitySourceByIdForUser(...args), })) vi.mock('#worker/repo/artifacts.ts', async () => { @@ -122,20 +122,25 @@ function mockPackageSource(sourceUserId = 'user-1') { name: '@kentcdodds/unleashed-wifi', sourceId: 'source-1', }) - mockModule.getEntitySourceById.mockResolvedValue({ - id: 'source-1', - user_id: sourceUserId, - entity_kind: 'package', - entity_id: 'package-1', - repo_id: 'package-package-1', - published_commit: 'commit-1', - indexed_commit: null, - manifest_path: 'package.json', - source_root: '/', - last_external_check_at: null, - created_at: '2026-05-04T00:00:00.000Z', - updated_at: '2026-05-04T00:00:00.000Z', - }) + mockModule.getEntitySourceByIdForUser.mockImplementation( + async (_db: unknown, input: { id: string; userId: string }) => { + if (input.userId !== sourceUserId) return null + return { + id: 'source-1', + user_id: sourceUserId, + entity_kind: 'package', + entity_id: 'package-1', + repo_id: 'package-package-1', + published_commit: 'commit-1', + indexed_commit: null, + manifest_path: 'package.json', + source_root: '/', + last_external_check_at: null, + created_at: '2026-05-04T00:00:00.000Z', + updated_at: '2026-05-04T00:00:00.000Z', + } + }, + ) const createToken = vi.fn(async (scope: 'read' | 'write', ttl: number) => ({ id: 'token-1', plaintext: `art_v1_${scope}_token?expires=${Math.floor(Date.now() / 1000) + ttl}`, diff --git a/packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts b/packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts index 891dd0a9da..5b1665928e 100644 --- a/packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts +++ b/packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts @@ -5,7 +5,7 @@ const mockModule = vi.hoisted(() => ({ captureException: vi.fn(), getSavedPackageById: vi.fn(), getSavedPackageByKodyId: vi.fn(), - getEntitySourceById: vi.fn(), + getEntitySourceByIdForUser: vi.fn(), resolveArtifactSourceHead: vi.fn(), publishFromExternalRef: vi.fn(), listPublishedPackageArtifactTargets: vi.fn(), @@ -26,8 +26,8 @@ vi.mock('#worker/package-registry/repo.ts', () => ({ })) vi.mock('#worker/repo/entity-sources.ts', () => ({ - getEntitySourceById: (...args: Array) => - mockModule.getEntitySourceById(...args), + getEntitySourceByIdForUser: (...args: Array) => + mockModule.getEntitySourceByIdForUser(...args), })) vi.mock('#worker/repo/artifacts.ts', () => ({ @@ -67,7 +67,7 @@ function setupDefaultMocks() { name: '@kentcdodds/demo-package', sourceId: 'source-1', }) - mockModule.getEntitySourceById.mockResolvedValue({ + mockModule.getEntitySourceByIdForUser.mockResolvedValue({ id: 'source-1', user_id: 'user-1', entity_kind: 'package', @@ -470,7 +470,7 @@ test('publishExternalPush recovers from transient Durable Object resets', async expect(consoleWarn).toHaveBeenCalledTimes(1) setupDefaultMocks() - mockModule.getEntitySourceById + mockModule.getEntitySourceByIdForUser .mockResolvedValueOnce({ id: 'source-1', user_id: 'user-1', diff --git a/packages/worker/src/mcp/capabilities/packages/resolve-package-source.ts b/packages/worker/src/mcp/capabilities/packages/resolve-package-source.ts index 9a334ce708..e994e8251a 100644 --- a/packages/worker/src/mcp/capabilities/packages/resolve-package-source.ts +++ b/packages/worker/src/mcp/capabilities/packages/resolve-package-source.ts @@ -3,7 +3,7 @@ import { getSavedPackageById, getSavedPackageByKodyId, } from '#worker/package-registry/repo.ts' -import { getEntitySourceById } from '#worker/repo/entity-sources.ts' +import { getEntitySourceByIdForUser } from '#worker/repo/entity-sources.ts' import { type EntitySourceRow } from '#worker/repo/types.ts' export type PackageSourceIdentity = { @@ -47,8 +47,11 @@ export async function resolveOwnedPackageSource(input: { const missingId = input.args.package_id ?? input.args.kody_id throw new McpCallerError(`Saved package "${missingId}" was not found.`) } - const source = await getEntitySourceById(input.db, savedPackage.sourceId) - if (!source || source.user_id !== input.userId) { + const source = await getEntitySourceByIdForUser(input.db, { + id: savedPackage.sourceId, + userId: input.userId, + }) + if (!source) { throw new McpCallerError('Repo source was not found for this user.') } return { diff --git a/packages/worker/src/mcp/observability.node.test.ts b/packages/worker/src/mcp/observability.node.test.ts index 79d59ca751..df0872465e 100644 --- a/packages/worker/src/mcp/observability.node.test.ts +++ b/packages/worker/src/mcp/observability.node.test.ts @@ -124,6 +124,18 @@ test('logMcpEvent keeps caller mistakes out of Sentry', () => { cause: new McpCallerError('Provide "query" or "domain".'), }) + logMcpEvent({ + ...callerFailureBase, + tool: 'search', + toolName: 'search', + errorName: 'McpCallerError', + errorMessage: + 'Unknown domain "skills". Available domains: account, packages.', + cause: new McpCallerError( + 'Unknown domain "skills". Available domains: account, packages.', + ), + }) + logMcpEvent({ ...callerFailureBase, capabilityName: 'repo_open_session', @@ -154,7 +166,7 @@ test('logMcpEvent keeps caller mistakes out of Sentry', () => { }) }) - expect(payloads).toHaveLength(4) + expect(payloads).toHaveLength(5) expect(sentryMock.captureException).not.toHaveBeenCalled() expect(sentryMock.captureMessage).not.toHaveBeenCalled() }) diff --git a/packages/worker/src/mcp/tools/search-core.ts b/packages/worker/src/mcp/tools/search-core.ts index 3477acabca..b384d21a17 100644 --- a/packages/worker/src/mcp/tools/search-core.ts +++ b/packages/worker/src/mcp/tools/search-core.ts @@ -1,3 +1,4 @@ +import { McpCallerError } from '#mcp/caller-error.ts' import { deterministicEmbedding, embedTextForVectorize, @@ -174,7 +175,9 @@ export async function searchUnified(input: { if (domainFilter) { const availableDomains = listSearchDomainNames(input.registry) if (!availableDomains.includes(domainFilter)) { - throw new Error( + // Caller passed a non-domain id (often a package kody id such as + // "skills"). Clear from the message alone — keep it off Sentry. + throw new McpCallerError( `Unknown domain "${domainFilter}". Available domains: ${[...availableDomains].sort().join(', ')}.`, ) } diff --git a/packages/worker/src/mcp/tools/search-handler.node.test.ts b/packages/worker/src/mcp/tools/search-handler.node.test.ts index 69ecbee01c..255e1849c7 100644 --- a/packages/worker/src/mcp/tools/search-handler.node.test.ts +++ b/packages/worker/src/mcp/tools/search-handler.node.test.ts @@ -643,24 +643,96 @@ test('search tool batches entity detail with per-ref isolation and preserves sin ]) mockPerformanceNow.mockReturnValueOnce(400).mockReturnValueOnce(410) - const allFailed = await handler({ - entity: ['missing_a:capability', 'missing_b:capability'], - conversationId: 'conv-batch-all-failed', - }) - expect(allFailed.isError).toBe(true) - expect(allFailed.structuredContent.error).toMatch( - /all entity lookups failed/i, - ) - expect(allFailed.structuredContent.result).toEqual([ - expect.objectContaining({ - entityRef: 'missing_a:capability', - error: expect.any(String), - }), - expect.objectContaining({ - entityRef: 'missing_b:capability', - error: expect.any(String), + const observability = await import('#mcp/observability.ts') + const logMcpEventSpy = vi.spyOn(observability, 'logMcpEvent') + try { + const allFailed = await handler({ + entity: ['missing_a:capability', 'missing_b:capability'], + conversationId: 'conv-batch-all-failed', + }) + expect(allFailed.isError).toBe(true) + expect(allFailed.structuredContent.error).toMatch( + /all entity lookups failed/i, + ) + expect(allFailed.structuredContent.result).toEqual([ + expect.objectContaining({ + entityRef: 'missing_a:capability', + error: expect.any(String), + }), + expect.objectContaining({ + entityRef: 'missing_b:capability', + error: expect.any(String), + }), + ]) + expect(logMcpEventSpy).toHaveBeenCalledWith( + expect.objectContaining({ + outcome: 'failure', + callerError: true, + errorName: 'EntityBatchError', + }), + ) + } finally { + logMcpEventSpy.mockRestore() + } +}) + +test('entity batch all-fail reports platform errors to Sentry (no callerError)', async () => { + const observability = await import('#mcp/observability.ts') + const logMcpEventSpy = vi.spyOn(observability, 'logMcpEvent') + mockModule.getSavedPackageById.mockImplementation( + async (_db: unknown, input: { packageId: string }) => ({ + id: input.packageId, + userId: 'user-1', + name: input.packageId, + kodyId: input.packageId, + description: 'pkg', + tags: [], + searchText: 'pkg', + sourceId: `source-${input.packageId}`, + hasApp: false, + hidden: false, + isPrivate: true, + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', }), - ]) + ) + mockModule.loadPackageSourceBySourceId.mockRejectedValue( + new Error('D1 read failed'), + ) + try { + const { handler } = await getSearchRegistration({ + user: { + userId: 'user-1', + email: 'user@example.com', + displayName: 'User', + username: 'user', + }, + }) + mockPerformanceNow.mockReturnValueOnce(500).mockReturnValueOnce(510) + const response = await handler({ + entity: ['pkg-a:package', 'pkg-b:package'], + conversationId: 'conv-batch-platform-fail', + }) + expect(response.isError).toBe(true) + expect(logMcpEventSpy).toHaveBeenCalledWith( + expect.objectContaining({ + outcome: 'failure', + errorName: 'EntityBatchError', + cause: expect.objectContaining({ + message: 'All entity lookups failed.', + }), + }), + ) + const failureCall = logMcpEventSpy.mock.calls.find( + (call) => + (call[0] as { errorName?: string }).errorName === 'EntityBatchError', + ) + expect(failureCall?.[0]).not.toHaveProperty('callerError', true) + } finally { + logMcpEventSpy.mockRestore() + mockModule.getSavedPackageById.mockReset() + mockModule.loadPackageSourceBySourceId.mockReset() + } }) test('integration entity detail enriches related packages without bloating ranked search', async () => { diff --git a/packages/worker/src/mcp/tools/search-tool-runner.ts b/packages/worker/src/mcp/tools/search-tool-runner.ts index accfc3dfec..93189554cf 100644 --- a/packages/worker/src/mcp/tools/search-tool-runner.ts +++ b/packages/worker/src/mcp/tools/search-tool-runner.ts @@ -324,18 +324,9 @@ export async function runSearchTool(input: { markdownParts.push(entityResult.markdown) } const allFailed = successCount === 0 - const failedEntries = outcome.results.filter( - ( - entry, - ): entry is Extract<(typeof outcome.results)[number], { ok: false }> => - !entry.ok, - ) - // Only treat a total batch failure as caller-caused when every - // failed lookup was a caller mistake. Shared platform failures - // (DB/load) still reach Sentry. - const allCallerFailures = - failedEntries.length > 0 && - failedEntries.every((entry) => entry.callerError) + const allFailuresAreCallerErrors = + allFailed && + outcome.results.every((entry) => !entry.ok && entry.callerError) logMcpEvent({ category: 'mcp', tool: 'search', @@ -348,7 +339,14 @@ export async function runSearchTool(input: { ...(allFailed ? { sandboxError: false, - ...(allCallerFailures ? { callerError: true } : {}), + // Only treat the batch as a caller mistake when every + // entry failed with McpCallerError. Mixed/platform + // failures must still reach Sentry. + ...(allFailuresAreCallerErrors + ? { callerError: true } + : { + cause: new Error('All entity lookups failed.'), + }), errorName: 'EntityBatchError', errorMessage: 'All entity lookups failed.', } diff --git a/packages/worker/src/mcp/tools/search.node.test.ts b/packages/worker/src/mcp/tools/search.node.test.ts index 3b7ff145dd..d1778d4d25 100644 --- a/packages/worker/src/mcp/tools/search.node.test.ts +++ b/packages/worker/src/mcp/tools/search.node.test.ts @@ -1,5 +1,6 @@ import { expect, test, vi } from 'vitest' import { consoleWarn } from '#worker/test-support/console-spies.ts' +import { McpCallerError } from '#mcp/caller-error.ts' import { buildCapabilityRegistry } from '#mcp/capabilities/build-capability-registry.ts' import { CAPABILITY_EMBEDDING_DIMENSIONS, @@ -2001,17 +2002,19 @@ test('searchUnified domain scoping: filter, browse, reject unknown, and overview expect(names).toContain('email_send') expect(names).not.toContain('job_schedule') - await expect( - searchUnified({ - env: {} as Env, - query: 'send email', - limit: 10, - userId: 'user-1', - registry, - optionalRows: emptyOptionalSearchRows, - domain: 'nope', - }), - ).rejects.toThrow(/Unknown domain "nope"/) + const unknownDomain = await searchUnified({ + env: {} as Env, + query: 'send email', + limit: 10, + userId: 'user-1', + registry, + optionalRows: emptyOptionalSearchRows, + domain: 'nope', + }).catch((error: unknown) => error) + expect(unknownDomain).toBeInstanceOf(McpCallerError) + expect(unknownDomain).toMatchObject({ + message: expect.stringMatching(/Unknown domain "nope"/), + }) const browse = await searchUnified({ env: {} as Env,