Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions packages/worker/src/mcp/caller-error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { getErrorCauseChain } from '@kody-internal/shared/error-message.ts'

/**
* A failure the caller can clear from the message alone: a missing or
* conflicting argument, an id that does not resolve, a precondition the caller
* has to undo first. Kody's MCP surface is agent-facing, so these are routine
* conversation turns rather than defects — they stay on the `mcp-event` log
* line and out of Sentry, where they open issues that look like platform bugs
* and trip triage automation.
*/
export class McpCallerError extends Error {
constructor(message: string, options?: ErrorOptions) {
super(message, options)
this.name = 'McpCallerError'
}
}

export function isMcpCallerError(error: unknown) {
return getErrorCauseChain(error).some(
(entry) => entry instanceof McpCallerError,
)
}
3 changes: 2 additions & 1 deletion packages/worker/src/mcp/capabilities/meta/search.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from 'zod'
import { McpCallerError } from '#mcp/caller-error.ts'
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
Expand Down Expand Up @@ -115,7 +116,7 @@ export const searchCapability = defineDomainCapability(
const query = args.query?.trim() ?? ''
const domainFilter = args.domain?.trim() || undefined
if (!query && !domainFilter) {
throw new Error('Provide "query" or "domain".')
throw new McpCallerError('Provide "query" or "domain".')
}
const conversationId = resolveConversationId(args.conversationId)
const userId = ctx.callerContext.user?.userId ?? null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { defineCapability } from '#mcp/capabilities/define-capability.ts'
import { defineDomain } from '#mcp/capabilities/define-domain.ts'
import { type CapabilityDomain } from '#mcp/capabilities/domain-metadata.ts'
import { type Capability, type DomainSpec } from '#mcp/capabilities/types.ts'
import { McpCallerError } from '#mcp/caller-error.ts'
import {
type OpenApiBinding,
type OpenApiBindingOperation,
Expand Down Expand Up @@ -190,7 +191,7 @@ function createCapabilityFromOperation(input: {
async handler(args, ctx) {
const userId = ctx.callerContext.user?.userId
if (!userId) {
throw new Error(
throw new McpCallerError(
`OpenAPI capability "${binding.name}:${operation.slug}" requires an authenticated user.`,
)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect, test, vi } from 'vitest'
import { McpCallerError } from '#mcp/caller-error.ts'
import * as secretService from '#mcp/secrets/service.ts'
import * as communityRepo from '#worker/community/repo.ts'
import * as packageRepo from '#worker/package-registry/repo.ts'
Expand Down Expand Up @@ -222,6 +223,49 @@ test('rejects absolute and protocol-relative operation paths', async () => {
}
})

test('missing path parameters throw McpCallerError so Sentry skips them', async () => {
const error = await executeOpenApiOperationRequest({
env: createEnv(),
userId: 'user-1',
baseUrl: 'https://app.example.com',
storageContext: null,
binding: {
...bindingBase,
apiBaseUrl: 'https://api.widgets.example',
},
operation: getWidget,
args: {},
globalFetch: vi.fn() as unknown as typeof fetch,
}).catch((caught: unknown) => caught)

expect(error).toBeInstanceOf(McpCallerError)
expect(error).toMatchObject({
message:
'Missing required path parameter "widgetId" for OpenAPI operation path /widgets/{widgetId}.',
})
})

test('non-object params throw McpCallerError', async () => {
const error = await executeOpenApiOperationRequest({
env: createEnv(),
userId: 'user-1',
baseUrl: 'https://app.example.com',
storageContext: null,
binding: {
...bindingBase,
apiBaseUrl: 'https://api.widgets.example',
},
operation: getWidget,
args: { params: ['not-an-object'] as unknown as Record<string, unknown> },
globalFetch: vi.fn() as unknown as typeof fetch,
}).catch((caught: unknown) => caught)

expect(error).toBeInstanceOf(McpCallerError)
expect(error).toMatchObject({
message: 'OpenAPI operation args.params must be an object.',
})
})

test('injects auth headers and overrides caller attempts', async () => {
const usageSpy = vi
.spyOn(usageModule, 'recordUsage')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
parseIntegrationJson,
type IntegrationConfig,
} from '#mcp/capabilities/integrations/integration-shared.ts'
import { McpCallerError } from '#mcp/caller-error.ts'
import { assertIntegrationHostAllowed } from '#mcp/execute-modules/integration-host-allowlist.ts'
import { executeGatewayFetch } from '#mcp/fetch-gateway.ts'
import {
Expand Down Expand Up @@ -161,7 +162,8 @@ function buildOperationUrl(input: {
let path = input.path.trim()
path = path.replace(/\{([^}/]+)\}/g, (_match, name: string) => {
if (!(name in input.params)) {
throw new Error(
// Caller omitted a path placeholder; clear from the message alone.
throw new McpCallerError(
`Missing required path parameter "${name}" for OpenAPI operation path ${input.path}.`,
)
}
Expand Down Expand Up @@ -665,13 +667,15 @@ function asStringRecord(
): Record<string, string> {
if (value == null) return {}
if (typeof value !== 'object' || Array.isArray(value)) {
throw new Error(`OpenAPI operation args.${fieldName} must be an object.`)
throw new McpCallerError(
`OpenAPI operation args.${fieldName} must be an object.`,
)
}
const result: Record<string, string> = {}
for (const [key, entry] of Object.entries(value)) {
if (entry == null) continue
if (typeof entry !== 'string' && typeof entry !== 'number') {
throw new Error(
throw new McpCallerError(
`OpenAPI operation args.${fieldName}.${key} must be a string or number.`,
)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from 'zod'
import { McpCallerError } from '#mcp/caller-error.ts'
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'
Expand Down Expand Up @@ -43,7 +44,7 @@ export const deletePackageCapability = defineDomainCapability(
packageId: args.package_id,
})
if (!existing) {
throw new Error('Saved package not found for this user.')
throw new McpCallerError('Saved package not found for this user.')
}
await deleteSavedPackageProjection({
env: ctx.env,
Expand Down
3 changes: 2 additions & 1 deletion packages/worker/src/mcp/capabilities/packages/get-package.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from 'zod'
import { McpCallerError } from '#mcp/caller-error.ts'
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'
Expand Down Expand Up @@ -54,7 +55,7 @@ export const getPackageCapability = defineDomainCapability(
packageId: args.package_id,
})
if (!saved) {
throw new Error('Saved package not found for this user.')
throw new McpCallerError('Saved package not found for this user.')
}
const username = await resolvePublicUsername({
db: ctx.env.APP_DB,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from 'zod'
import { McpCallerError } from '#mcp/caller-error.ts'
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'
Expand Down Expand Up @@ -61,14 +62,14 @@ export const packageUpdateCapability = defineDomainCapability(
hidden: args.changes.hidden,
})
if (!changed) {
throw new Error('Saved package not found for this user.')
throw new McpCallerError('Saved package not found for this user.')
}
const savedPackage = await getSavedPackageById(ctx.env.APP_DB, {
userId: owner.ownerUserId,
packageId: args.package_id,
})
if (!savedPackage) {
throw new Error('Saved package not found for this user.')
throw new McpCallerError('Saved package not found for this user.')
}
return {
ok: true as const,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { McpCallerError } from '#mcp/caller-error.ts'
import {
getSavedPackageById,
getSavedPackageByKodyId,
Expand All @@ -15,7 +16,9 @@ function requireExactlyOnePackageSourceIdentity(input: PackageSourceIdentity) {
(input.package_id !== undefined ? 1 : 0) +
(input.kody_id !== undefined ? 1 : 0)
if (count !== 1) {
throw new Error('Provide exactly one of `package_id` or `kody_id`.')
throw new McpCallerError(
'Provide exactly one of `package_id` or `kody_id`.',
)
}
}

Expand All @@ -42,11 +45,11 @@ export async function resolveOwnedPackageSource(input: {
})
if (!savedPackage) {
const missingId = input.args.package_id ?? input.args.kody_id
throw new Error(`Saved package "${missingId}" was not found.`)
throw new McpCallerError(`Saved package "${missingId}" was not found.`)
}
const source = await getEntitySourceById(input.db, savedPackage.sourceId)
if (!source || source.user_id !== input.userId) {
throw new Error('Repo source was not found for this user.')
throw new McpCallerError('Repo source was not found for this user.')
Comment on lines 50 to +52

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Scope the entity-source lookup by userId.

getEntitySourceById currently reads entity_sources by id alone, then checks source.user_id afterward. Although the check prevents returning another user’s source today, the read path itself is not user-scoped. Change the helper/query to enforce WHERE id = ? AND user_id = ?, then treat a null result as “not found.”

As per coding guidelines, every read/write path for Kody’s multi-user data must be scoped by userId; cross-user data sharing is a bug.

Proposed direction
-const source = await getEntitySourceById(input.db, savedPackage.sourceId)
-if (!source || source.user_id !== input.userId) {
+const source = await getEntitySourceById(input.db, {
+	id: savedPackage.sourceId,
+	userId: input.userId,
+})
+if (!source) {
	throw new McpCallerError('Repo source was not found for this user.')
}

Update getEntitySourceById accordingly so the SQL predicate includes user_id.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/capabilities/packages/resolve-package-source.ts`
around lines 50 - 52, Update getEntitySourceById to accept and use userId in its
entity_sources query, enforcing both id and user_id predicates. In the
resolve-package-source flow, pass input.userId to the helper and treat a null
result as “Repo source was not found for this user,” removing the redundant
post-fetch ownership check.

Source: Coding guidelines

}
return {
packageId: savedPackage.id,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { McpCallerError } from '#mcp/caller-error.ts'
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
Expand Down Expand Up @@ -25,7 +26,9 @@ export const repoOpenSessionCapability = defineDomainCapability(
async handler(args, ctx: CapabilityContext) {
const user = ctx.callerContext.user
if (!user) {
throw new Error('repo_open_session requires an authenticated user.')
throw new McpCallerError(
'repo_open_session requires an authenticated user.',
)
}

const requested = await resolveRepoSourceReference({
Expand All @@ -42,7 +45,7 @@ export const repoOpenSessionCapability = defineDomainCapability(
})
if (existingSession) {
if (existingSession.source_id !== requested.source.id) {
throw new Error(
throw new McpCallerError(
'Active repo session does not match the requested source. Discard the current session before opening a new source.',
)
}
Expand Down
100 changes: 89 additions & 11 deletions packages/worker/src/mcp/observability.node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ vi.mock('@sentry/cloudflare', () => ({
}))

const { logMcpEvent } = await import('./observability.ts')
const { McpCallerError } = await import('./caller-error.ts')

test('logMcpEvent keeps sandbox failures on mcp-event logs and skips Sentry', () => {
function captureMcpEvents(run: () => void) {
sentryMock.captureException.mockClear()
sentryMock.captureMessage.mockClear()
sentryMock.withScope.mockClear()
Expand All @@ -44,6 +45,25 @@ test('logMcpEvent keeps sandbox failures on mcp-event logs and skips Sentry', ()
}
}) as typeof console.info
try {
run()
} finally {
console.info = originalInfo
}
return payloads
}

const callerFailureBase = {
category: 'mcp',
tool: 'capability',
outcome: 'failure',
durationMs: 3,
baseUrl: 'https://example.com',
hasUser: true,
userId: 'user-1',
} as const

test('logMcpEvent keeps sandbox failures on mcp-event logs and skips Sentry', () => {
const payloads = captureMcpEvents(() => {
logMcpEvent({
category: 'mcp',
tool: 'execute',
Expand All @@ -61,24 +81,16 @@ test('logMcpEvent keeps sandbox failures on mcp-event logs and skips Sentry', ()
})

logMcpEvent({
category: 'mcp',
tool: 'capability',
...callerFailureBase,
capabilityName: 'value_get',
capabilitySource: 'builtin',
outcome: 'failure',
durationMs: 3,
baseUrl: 'https://example.com',
hasUser: true,
userId: 'user-1',
sandboxError: false,
failurePhase: 'handler',
errorName: 'Error',
errorMessage: 'platform handler blew up',
cause: new Error('platform handler blew up'),
})
} finally {
console.info = originalInfo
}
})

expect(payloads).toHaveLength(2)
expect(JSON.parse(payloads[0]!)).toMatchObject({
Expand All @@ -100,3 +112,69 @@ test('logMcpEvent keeps sandbox failures on mcp-event logs and skips Sentry', ()
expect(sentryMock.scope.setLevel).toHaveBeenCalledWith('error')
expect(sentryMock.scope.setUser).toHaveBeenCalledWith({ id: 'user-1' })
})

test('logMcpEvent keeps caller mistakes out of Sentry', () => {
const payloads = captureMcpEvents(() => {
logMcpEvent({
...callerFailureBase,
capabilityName: 'search',
failurePhase: 'handler',
errorName: 'McpCallerError',
errorMessage: 'Provide "query" or "domain".',
cause: new McpCallerError('Provide "query" or "domain".'),
})

logMcpEvent({
...callerFailureBase,
capabilityName: 'repo_open_session',
failurePhase: 'handler',
errorName: 'Error',
errorMessage: 'Opening the session failed.',
cause: new Error('Opening the session failed.', {
cause: new McpCallerError('Discard the current session first.'),
}),
})

logMcpEvent({
...callerFailureBase,
capabilityName: 'value_get',
failurePhase: 'parse_input',
errorName: 'ZodError',
errorMessage: 'name: Required',
cause: new Error('name: Required'),
})

logMcpEvent({
...callerFailureBase,
tool: 'search',
toolName: 'search',
callerError: true,
errorName: 'EntityBatchError',
errorMessage: 'All entity lookups failed.',
})
})

expect(payloads).toHaveLength(4)
expect(sentryMock.captureException).not.toHaveBeenCalled()
expect(sentryMock.captureMessage).not.toHaveBeenCalled()
})

test('logMcpEvent still reports platform failures that wrap no caller error', () => {
captureMcpEvents(() => {
logMcpEvent({
...callerFailureBase,
capabilityName: 'package_get',
failurePhase: 'handler',
errorName: 'Error',
errorMessage: 'D1 write failed.',
cause: new Error('D1 write failed.', {
cause: new Error('storage unavailable'),
}),
})
})

expect(sentryMock.captureException).toHaveBeenCalledTimes(1)
expect(sentryMock.captureException).toHaveBeenCalledWith(
expect.objectContaining({ message: 'D1 write failed.' }),
)
})
Loading
Loading