Repository navigation
fix(sentry): keep malformed search entity refs out of Sentry - #936
Conversation
parseEntityRef and parseValueEntityId threw plain Errors for bad agent input, so all-failed entity batches were reported to Sentry as platform bugs (issue 7631143592). Throw McpCallerError instead, and attach per-entity failure context when a batch still fails for platform reasons. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughSearch entity parsers now classify invalid references as ChangesMCP search error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
|
🔎 Preview deployed: https://kody-pr-936.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tools/search-handler.node.test.ts`:
- Line 742: Update the error assertion for runSearchTool to match the nested
error contract: assert the logged outer Error first, then verify its cause is an
AggregateError rather than expecting the logged value itself to be an
AggregateError.
🪄 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: f6ea605d-01ad-447a-a2b2-7de553f1b0df
📒 Files selected for processing (6)
packages/worker/src/mcp/tools/search-detail.node.test.tspackages/worker/src/mcp/tools/search-entities.tspackages/worker/src/mcp/tools/search-format-helpers.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-handler.node.test.tspackages/worker/src/mcp/tools/search-tool-runner.ts
| errorName: 'EntityBatchError', | ||
| cause: expect.objectContaining({ | ||
| message: 'All entity lookups failed.', | ||
| cause: expect.any(AggregateError), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the nested AggregateError contract.
runSearchTool logs an outer Error whose .cause is the AggregateError; this direct assertion will fail.
Proposed fix
- cause: expect.any(AggregateError),
+ cause: expect.objectContaining({
+ cause: expect.any(AggregateError),
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cause: expect.any(AggregateError), | |
| cause: expect.objectContaining({ | |
| cause: expect.any(AggregateError), | |
| }), |
🤖 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/tools/search-handler.node.test.ts` at line 742,
Update the error assertion for runSearchTool to match the nested error contract:
assert the logged outer Error first, then verify its cause is an AggregateError
rather than expecting the logged value itself to be an AggregateError.
Summary
Sentry issue 7631143592 (
Error: All entity lookups failed.inrunSearchTool) was fired when an agent batch-looked-up entities with malformed refs (or other caller-input parse failures).parseEntityRef/parseValueEntityIdthrew plainError, so the entity-batch path treated every failure as a platform incident and reported a syntheticAll entity lookups failed.exception.This continues the
#919/#922McpCallerErrorcarve-out: bad entity ref / value-entity format is a caller mistake and stays on themcp-eventlog line. Genuine platform batch failures still reach Sentry, now with per-entity failure context + anAggregateErrorcause so triage is not blind.Changes
McpCallerErrorfromparseEntityRefandparseValueEntityIdentityFailurescontext; for non-caller failures, chain anAggregateErrorunder the synthetic causeRisk / merge
Low (
composes): observability attribution only; agent-facing error text unchanged. Platform batch failures still report to Sentry.Siblings
7631582437(Provide "query", "entity", or "domain") does not share this root cause — that path already setscallerError: true(event was from a pre-#919release).7631797267, are separate.System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@42d211e0· Head:e22f04b7Classification: composes — reuses the existing
McpCallerError/callerErrorobservability carve-out; no new primitives, no contract change to search results beyond Sentry attribution.Primitives touched
mcp-serverSystem map
Malformed
search({ entity })refs are attributed as caller mistakes so they stay off Sentry; platform batch failures still alert with per-entity detail.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
userId).Risk notes
Docs
Summary by CodeRabbit
Bug Fixes
Tests