Repository navigation
Keep OpenAPI and MCP caller mistakes out of Sentry - #919
Merged
Merged
Conversation
Capability handlers throw plain Errors for caller mistakes (missing arguments, ids that do not resolve, preconditions the caller must clear) and every one of them opened a Sentry issue that reads like a platform bug. Five of the fourteen open kody-cloudflare issues are this class. Adds McpCallerError so a failure site can say the caller caused it, skips Sentry for parse_input failures (arguments never matched the declared schema), and adds a callerError payload flag for the search paths that report a caller mistake without throwing. Extends the same carve-out #916 and #917 made for connector disconnects and sandbox execute failures. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Caller mistakes like omitting organization_id_or_slug were thrown as plain Errors from buildOperationUrl and opened Sentry issues that look like platform bugs (KODY-CLOUDFLARE-1S). Throw McpCallerError instead so observability keeps them on mcp-event logs only.
kentcdodds
marked this pull request as ready for review
July 24, 2026 19:57
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ef030b. Configure here.
📝 WalkthroughWalkthroughIntroduces ChangesMCP caller error attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPCaller
participant MCPHandler
participant logMcpEvent
participant Sentry
MCPCaller->>MCPHandler: Submit MCP request
MCPHandler->>logMcpEvent: Log caller or platform failure
logMcpEvent->>Sentry: Report only non-caller failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
🔎 Preview deployed: https://kody-pr-919.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/worker/src/mcp/capabilities/packages/resolve-package-source.ts`:
- Around line 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.
In `@packages/worker/src/mcp/tools/search-tool-runner.ts`:
- Around line 336-338: Update the batch lookup error handling around the
callerError assignment so each failed entity retains whether its failure was
caller-attributed or caused by storage/source-loading infrastructure. Set
callerError only when all failed lookups are caller-attributed; leave it unset
for any shared or platform failure so incidents remain reportable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d8d9a75-09ff-4773-a17f-771819ebc58a
📒 Files selected for processing (14)
packages/worker/src/mcp/caller-error.tspackages/worker/src/mcp/capabilities/meta/search.tspackages/worker/src/mcp/capabilities/openapi-provider/index.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tspackages/worker/src/mcp/capabilities/packages/delete-package.tspackages/worker/src/mcp/capabilities/packages/get-package.tspackages/worker/src/mcp/capabilities/packages/package-update.tspackages/worker/src/mcp/capabilities/packages/resolve-package-source.tspackages/worker/src/mcp/capabilities/repo/repo-open-session.tspackages/worker/src/mcp/observability.node.test.tspackages/worker/src/mcp/observability.tspackages/worker/src/mcp/tools/search-detail.tspackages/worker/src/mcp/tools/search-tool-runner.ts
Comment on lines
50
to
+52
| 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.') |
There was a problem hiding this comment.
🔒 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
Review feedback: a total entity-batch failure could hide platform incidents when every lookup failed for DB/load reasons. Preserve per-entry McpCallerError provenance and set callerError only when all failures are caller mistakes. Also mark search-detail not-found paths as McpCallerError.
cursor Bot
pushed a commit
that referenced
this pull request
Jul 24, 2026
…ssified 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.
kody-bot
pushed a commit
that referenced
this pull request
Jul 24, 2026
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. Co-authored-by: Cursor Agent <cursoragent@cursor.com>
cursor Bot
pushed a commit
that referenced
this pull request
Jul 24, 2026
#919 carved out a first batch of caller-error throw sites. Sibling sites raise the identical message strings from other files, and because Sentry groups by message they would reopen the same archived groups under a different culprit. Convert the rest of the identity-lookup family: - repo target resolution and publish-note source lookups (same messages as the already-converted package source resolver) - the value/integration/secret/capability branches of resolveEntityDetail, which previously only carved out the saved-package branch - package run, invocation token, package service, and OpenAPI binding lookups by caller-supplied id Messages are unchanged, so the MCP response a caller sees is identical. Authentication and authorization denials are deliberately excluded. An earlier revision of this branch routed them through McpCallerError too; they are not a noise problem (no such event has ever reached Sentry) and silencing them would remove the only signal we have for a principal probing the permission surface. Attack visibility for those is handled separately in the audit log. Package-runtime secret mounts and admin account lookups also keep reporting: those ids come from our own resolution, so a miss is a real inconsistency.
kody-bot
pushed a commit
that referenced
this pull request
Jul 24, 2026
…n the audit log (#923) * Keep remaining MCP not-found caller errors out of Sentry #919 carved out a first batch of caller-error throw sites. Sibling sites raise the identical message strings from other files, and because Sentry groups by message they would reopen the same archived groups under a different culprit. Convert the rest of the identity-lookup family: - repo target resolution and publish-note source lookups (same messages as the already-converted package source resolver) - the value/integration/secret/capability branches of resolveEntityDetail, which previously only carved out the saved-package branch - package run, invocation token, package service, and OpenAPI binding lookups by caller-supplied id Messages are unchanged, so the MCP response a caller sees is identical. Authentication and authorization denials are deliberately excluded. An earlier revision of this branch routed them through McpCallerError too; they are not a noise problem (no such event has ever reached Sentry) and silencing them would remove the only signal we have for a principal probing the permission surface. Attack visibility for those is handled separately in the audit log. Package-runtime secret mounts and admin account lookups also keep reporting: those ids come from our own resolution, so a miss is a real inconsistency. * Record MCP auth denials in the audit log instead of Sentry MCP authentication and authorization denials had no home. Routing them through McpCallerError would have silenced them entirely, and leaving them as Sentry errors treats a routine agent turn as a platform defect. Neither gives us the one thing that matters: noticing a principal probing the permission surface. Record them where browser and OAuth sign-in failures already go. audit_events hashes identifiers, keeps rows for 180 days, is queryable by admins through admin_audit_log_query, and feeds the failure-per-day and failure-per-hour charts on /admin/insights, so a burst surfaces on a chart that already exists without anything new to build. There is no counter, window, or in-process state, so nothing to lose on a Workers cold start or reconcile across Durable Objects. Two sites record: handleMcpRequest rejecting a resolved grant, and assertCallerCanAccessCapability refusing a capability. The latter is the single choke point every capability call passes through, which is why it covers the whole authorization surface rather than the two guards that prompted this. Its denial tail is restructured so one audit call replaces five throw sites; the messages are unchanged. Rejections before a grant resolves are deliberately not recorded. They are reachable by any anonymous request, so a row per attempt would let a stranger drive unbounded D1 writes, and an unattributable bad token carries little signal. The cost is that token replay against /mcp is invisible here; that is edge rate-limiting work, not application writes. Documented in security.md. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
This was referenced Jul 25, 2026
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes KODY-CLOUDFLARE-1S.
A handled MCP call to
openapi:sentry:listorganizationissuesomittedorganization_id_or_slug, threw frombuildOperationUrl, and opened a Sentry issue that looks like a platform bug.This PR:
McpCallerErrorand skips Sentry for caller failures,parse_input, and an explicitcallerErrorflag.McpCallerErrorfrom OpenAPI path-param and args-shape validation.callerErrorwhen every failed lookup is caller-attributed (review fix).No siblings shared this root cause.
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@dc25e719· Head:e1f0ad14Classification: extends — MCP observability treats caller-clearable failures as non-Sentry; OpenAPI validation uses that contract.
Primitives touched
mcp-serveropenapi-bindingsMcpCallerErrorcapability-registrysaved-packagesmemoriesSystem map
Caller mistakes are marked at capability sites; MCP observability keeps them on mcp-event only.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Summary by CodeRabbit