Repository navigation
Keep unknown search domains out of Sentry, and stop the batch carve-out over-suppressing - #922
Conversation
📝 WalkthroughWalkthroughChangesThe MCP surface now uses MCP caller failure handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPCaller
participant MCPHandler
participant logMcpEvent
participant Sentry
MCPCaller->>MCPHandler: submit invalid or unauthorized request
MCPHandler-->>MCPCaller: throw McpCallerError
MCPHandler->>logMcpEvent: record caller failure
logMcpEvent->>Sentry: skip 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 |
cursor
Bot
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 40c0932. Configure here.
|
🔎 Preview deployed: https://kody-pr-922.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: 3
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.ts (1)
226-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover invalid individual argument values too.
These tests cover missing path parameters and non-object
args.params, but notasStringRecord’s newMcpCallerErrorpath for values such asparams: { widgetId: true }atpackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tsLines 677-680. Add an assertion for its error type and message.🤖 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/openapi-provider/operation-request.node.test.ts` around lines 226 - 268, Add a test alongside the existing invalid-parameter cases that calls executeOpenApiOperationRequest with args.params containing a non-string value such as widgetId: true. Assert the rejected error is a McpCallerError and its message matches the validation message produced by asStringRecord for invalid parameter values.
🤖 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 the repo-source lookup in the resolve-package-source
flow to use a persistence query or helper that filters by both sourceId and
input.userId, rather than calling getEntitySourceById with only the ID and
checking source.user_id afterward. Treat a null result as “Repo source was not
found for this user” and remove the redundant in-memory ownership check.
In `@packages/worker/src/mcp/tools/search-detail.ts`:
- Around line 72-74: Update resolveEntityDetail so missing capabilities, values,
integrations, and secrets throw McpCallerError instead of plain Error, matching
the existing authentication and entity-miss handling. Keep the current
caller-facing messages and lookup behavior unchanged while converting all
remaining lookup-failure throw sites.
In `@packages/worker/src/mcp/tools/search-tool-runner.ts`:
- Around line 336-338: Update the batch error handling around the
allFailed/callerError classification so callerError is set only when every
failed entity entry is classified as caller-caused. Preserve each entry’s
existing caller/platform failure classification, and ensure any batch containing
a platform or repository failure remains eligible for Sentry reporting.
---
Nitpick comments:
In
`@packages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.ts`:
- Around line 226-268: Add a test alongside the existing invalid-parameter cases
that calls executeOpenApiOperationRequest with args.params containing a
non-string value such as widgetId: true. Assert the rejected error is a
McpCallerError and its message matches the validation message produced by
asStringRecord for invalid parameter values.
🪄 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: aa67ca3a-4151-428d-b922-c77158cc7034
📒 Files selected for processing (16)
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-core.tspackages/worker/src/mcp/tools/search-detail.tspackages/worker/src/mcp/tools/search-tool-runner.tspackages/worker/src/mcp/tools/search.node.test.ts
…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.
c452e16 to
a2ff96b
Compare
cursor
Bot
changed the title
|
Heads up: I rebuilt this branch on current WhyThis PR and #919 were written in parallel, within about an hour of each other, against the same files. Both independently created Rather than close it — most of a rebase would have been resolving conflicts against a copy of your own change — I rebuilt the branch from Kept
DroppedEverything #919 already shipped: Original history is preserved in the PR body for reference: State
ContextThis branch is one of three that independently invented #923 covers the remaining sibling throw sites in the same family. It and this PR touch disjoint files, so they can land in either order. |
Review follow-up. resolveRepoSourceReference and repo_show_publish_note loaded a source by id and then compared source.user_id in app code. Use the existing getEntitySourceByIdForUser helper so the user predicate is part of the query, matching what #922 did for resolveOwnedPackageSource. Behaviour is unchanged; another user's source now simply is not returned rather than being returned and rejected. Also covers the missing-identity branch of resolveRepoSourceReference, which the previous commit converted without a test.
Review follow-up. resolveRepoSourceReference and repo_show_publish_note loaded a source by id and then compared source.user_id in app code. Use the existing getEntitySourceByIdForUser helper so the user predicate is part of the query, matching what #922 did for resolveOwnedPackageSource. Behaviour is unchanged; another user's source now simply is not returned rather than being returned and rejected. Also covers the missing-identity branch of resolveRepoSourceReference, which the previous commit converted without a test. Co-authored-by: Cursor Agent <cursoragent@cursor.com>
3 participants
Summary
Fixes KODY-CLOUDFLARE-1T.
An MCP client called
search({ domain: "skills" }).skillsis a popular package kody id, not a capability domain.searchUnifiedcorrectly rejected it, but threw a plainError, so observability opened a Sentry issue that looks like a platform bug.What is left in this PR
Unknown search domain is a caller mistake.
searchUnifiednow throwsMcpCallerErrorfor a domain id that is not in the registry, so the failure stays on themcp-eventlog line. This is the actual-1Tfix.The entity-batch carve-out no longer over-suppresses. Keep OpenAPI and MCP caller mistakes out of Sentry #919 marks a fully-failed
search({ entity: [...] })batch ascallerError. That is right when every ref was simply unresolvable, but wrong when the batch failed because something underneath broke — a D1 read failing mid-batch would have been silently swallowed. The batch is now only markedcallerErrorwhen every entry failed with a caller error, and otherwise passes acauseso the failure reaches Sentry as an exception. Two tests insearch-handler.node.test.tscover both directions, which is the regression guard for the whole carve-out.resolveOwnedPackageSourcescopes in the query. It used the existinggetEntitySourceByIdForUserhelper instead of loading a source by id and then filtering onsource.user_idin app code. Behaviour is unchanged; the scoping just lives whereAGENTS.mdasks for it. Mock updates inget-git-remote.node.test.tsandpublish-external-push.node.test.tsfollow from that.Item 3 is unrelated to the Sentry cleanup and is easy to drop if you would rather keep this PR to items 1 and 2 — it survived only because it had already been written and reviewed here.
Not in this PR any more
Everything #919 shipped:
caller-error.ts, theisCallerFailureskip inobservability.ts, and themeta/search.ts,openapi-provider/*,packages/*,repo-open-session.ts, andsearch-detail.tsthrow-site conversions.Related
#923 finishes the same family from the other direction — the sibling throw sites that raise message strings identical to ones #919 converted, which would otherwise have reopened the archived Sentry groups under a different culprit. #923 and this PR touch disjoint files.
Validation
npm run validategreen in full on the rebuilt branch:format:check,lint(1 pre-existing warning insentry-tunnel.node.test.ts, 0 errors),typecheck, 1289 unit tests across 397 files, 18 Playwright E2E, 2 MCP E2E,backup:build,primitives:check,migrations:check.System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@653ae7c1· Head:a2ff96b8Classification: composes — reuses #919's
McpCallerErrorcontract at one more throw site and tightens the batch classification it introduced. No primitive changes shape.Primitives touched
mcp-serversearchdomain throwsMcpCallerError; entity-batch failures only count as caller errors when every entry was onesaved-packagesSystem map
Unknown domain ids and fully-unresolvable entity batches are classified as caller mistakes; anything else in those paths still reaches Sentry.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
Before:
search({ domain: "skills" })opened a Sentry issue. A fully-failed entity batch was always treated as a caller mistake, so a platform failure underneath it was suppressed.After: the unknown domain stays on
mcp-event. A fully-failed batch is only suppressed when every entry was itself a caller error; otherwise it reaches Sentry as an exception.Invariants
Per-user isolation is preserved and slightly tightened:
resolveOwnedPackageSourcenow filters byuserIdin the query rather than after the read.Summary by CodeRabbit