feat(mcp): classify MCP tool errors by category for alerting#2570
Merged
Conversation
- Add error categorization (user vs server) to MCP tool error responses using a WeakMap side-channel that can't leak through SDK serialization - Use instanceof ClickHouseError instead of duck-typing for type detection - Add isServerError() to detect both ClickHouse server-side error types (NETWORK_ERROR, SOCKET_TIMEOUT, etc.) and Node.js TCP errors (ECONNREFUSED, ENOTFOUND, etc.) with full cause-chain walking - Classify errors on OTel spans (mcp.tool.error_category) and metric counters so server errors trigger alerts while user errors don't - Use clickHouseErrorResult in catch blocks across eventDeltas, breakdown, and waterfall tool handlers for consistent categorization - Fall back through e.message → e.cause.message → String(e) for errors where common-utils' ClickHouseQueryError wraps with an empty message Tests: - Unit: getClickHouseErrorType, isServerError, clickHouseErrorResult with getErrorCategory assertions for all error shapes - Unit: withToolTracing error_category on spans and counters - Integration: syntax errors, unknown columns/tables across sql, table, timeseries tools; unreachable host errors; no _errorCategory leak
- Add isClickHouseError() with constructor-name fallback for duplicate @clickhouse/client-common packages across the monorepo - Migrate assertSourceKindMatchesSelect and validateMetricSelectItems from inline error objects to mcpUserError() (fixes WeakMap bypass) - Reclassify timeouts as server errors in describeMetric, describeSource, listMetrics (infrastructure condition, not user input) - Reclassify getColumns failure to use clickHouseErrorResult for auto-classification in describeMetric - Reclassify MongoDB query failure as server error in searchDashboards - Reclassify pattern mining failure as server error in runEventPatterns - Remove dead mcpError() alias, inline mcpUserError in validateObjectId - Fix TypeScript narrowing in tracing.ts for getErrorCategory call
🦋 Changeset detectedLatest commit: 56e4a66 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-4
Contributor
🔴 Tier 4 — CriticalTouches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD. Why this tier:
Additional context: agent branch ( Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Contributor
Greptile SummaryThis PR introduces a structured error categorization system for MCP tools, distinguishing
Confidence Score: 5/5Safe to merge — the classification logic is well-encapsulated, the WeakMap side-channel correctly prevents metadata leakage, and all new paths have thorough unit and integration test coverage. The core implementation — WeakMap side-channel, cause-chain walking in isServerError, auto-classification in clickHouseErrorResult, and tracing propagation — is correct and verified by tests covering circular references, cross-package ClickHouseError identity, AggregateError, and the no-leak wire format. The ~25 tool migrations are mechanical and follow a consistent pattern. The only note is the prerelease snapshot for @clickhouse/client-common, which does not affect correctness at runtime (yarn.lock preserves the resolution) but is a long-term maintenance consideration. packages/api/package.json — the @clickhouse/client-common prerelease pin is worth revisiting when a stable release is available. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP Tool Handler] -->|returns result| B{result.isError?}
B -->|No| C[setStatus OK\nlog info]
B -->|Yes| D[getErrorCategory via WeakMap]
D -->|category set| E{category}
D -->|undefined| F[default: 'server']
E -->|user| G[span: error_category=user\ncounter: error_category=user]
E -->|server| H[span: error_category=server\ncounter: error_category=server]
F --> H
subgraph Error Creation
M[mcpUserError] -->|WeakMap.set → 'user'| N[McpErrorResult]
O[mcpServerError] -->|WeakMap.set → 'server'| N
P[clickHouseErrorResult] --> Q{isServerError?}
Q -->|true - NETWORK_ERROR/ECONNREFUSED etc| O
Q -->|false - SYNTAX_ERROR etc| M
end
subgraph isServerError - walks .cause chain
R[ClickHouseError.type in SERVER_CH_ERROR_TYPES] -->|NETWORK_ERROR etc| S[return true]
T[Node.js Error.code in SERVER_NODE_ERROR_CODES] -->|ECONNREFUSED etc| S
U[AggregateError.errors TCP code] --> S
end
A -->|throws| V[catch in withToolTracing\nsetAttribute error_category=server\nrethrow → withSpan records exception]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[MCP Tool Handler] -->|returns result| B{result.isError?}
B -->|No| C[setStatus OK\nlog info]
B -->|Yes| D[getErrorCategory via WeakMap]
D -->|category set| E{category}
D -->|undefined| F[default: 'server']
E -->|user| G[span: error_category=user\ncounter: error_category=user]
E -->|server| H[span: error_category=server\ncounter: error_category=server]
F --> H
subgraph Error Creation
M[mcpUserError] -->|WeakMap.set → 'user'| N[McpErrorResult]
O[mcpServerError] -->|WeakMap.set → 'server'| N
P[clickHouseErrorResult] --> Q{isServerError?}
Q -->|true - NETWORK_ERROR/ECONNREFUSED etc| O
Q -->|false - SYNTAX_ERROR etc| M
end
subgraph isServerError - walks .cause chain
R[ClickHouseError.type in SERVER_CH_ERROR_TYPES] -->|NETWORK_ERROR etc| S[return true]
T[Node.js Error.code in SERVER_NODE_ERROR_CODES] -->|ECONNREFUSED etc| S
U[AggregateError.errors TCP code] --> S
end
A -->|throws| V[catch in withToolTracing\nsetAttribute error_category=server\nrethrow → withSpan records exception]
Reviews (5): Last reviewed commit: "Merge branch 'main' into claude/mcp-erro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
E2E Test Results✅ All tests passed • 224 passed • 3 skipped • 1520s
Tests ran across 4 shards in parallel. |
Contributor
Deep Review✅ No critical issues found. The migration is clean — all ~25 tools flow through 🟡 P2 -- recommended
🔵 P3 nitpicks (6)
Reviewers (7): correctness, testing, maintainability, project-standards, reliability, kieran-typescript, adversarial. Testing gaps:
|
isServerError now checks for ClickHouseError server-side types at every depth of the cause chain, not just depth 0-1. Previously a NETWORK_ERROR nested 2+ levels deep would be missed while a Node.js ECONNREFUSED at the same depth was correctly caught.
- Fix eventDeltas.ts:343 misclassification: use clickHouseErrorResult for 'Failed to build sample queries' catch (matches sibling at 381) - Pin @clickhouse/client-common to 1.23.0-head.fae5998.1 matching common-utils, eliminating duplicate package installs - Add changeset for @hyperdx/api - Add tests: cross-package constructor-name fallback, deep cause chain ClickHouseError detection, AggregateError TCP errors, circular cause guard
brandon-pereira
requested review from
a team and
knudtty
and removed request for
a team
knudtty
approved these changes
Jul 2, 2026
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
2 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.
What
Classify MCP tool errors into user vs server categories so we can alert on infrastructure failures without noise from agent input errors.
Changes
Error categorization infrastructure (
packages/api/src/mcp/utils/errors.ts):mcpUserError()/mcpServerError()helpers replace inline error boilerplate across all MCP toolsWeakMapside-channel — prevents metadata from leaking through the MCP SDK serialization layergetErrorCategory()retrieves category for tracing/metricsClickHouse error classification (
packages/api/src/mcp/tools/query/helpers.ts):clickHouseErrorResult()auto-classifies ClickHouse errors: infrastructure types (NETWORK_ERROR, SOCKET_TIMEOUT, etc.) → server; query errors → userisServerError()walks the full.causechain for both ClickHouse server-side error types and Node.js TCP-level errors (ECONNREFUSED, ENOTFOUND, etc.)isClickHouseError()with constructor-name fallback handles duplicate@clickhouse/client-commonpackages across the monorepo (root vs common-utils)Tracing enrichment (
packages/api/src/mcp/utils/tracing.ts):error_categoryattribute added to spans and thehyperdx.mcp.tool.errorscounterserverwhen category is unset (safe default that surfaces unclassified errors in alerts)Tool migrations (~25 files):
{ isError: true, content: [...] }to typed helpersdescribeMetric,describeSource,listMetrics→mcpServerError(infrastructure condition)searchDashboards→mcpServerErrorrunEventPatterns→mcpServerErrorassertSourceKindMatchesSelect,validateMetricSelectItems) →mcpUserErrorWhy
Without error categorization, every
isError: trueresult looks the same in metrics/alerts. Agent typos (wrong source ID, bad query syntax) and real infrastructure outages (ClickHouse down, MongoDB timeout) are indistinguishable. This makes MCP error alerts either too noisy (fire on every user error) or useless (too many false positives to trust).With this change, alerting rules can filter on
error_category = "server"to catch only infrastructure failures.Testing
Unit tests (
query.test.ts):getClickHouseErrorType— ClickHouseError type extraction, cross-package scenarios, duck-typing resilienceisServerError— ClickHouse error types, Node.js TCP errors, cause-chain walkingclickHouseErrorResult— auto-classification for infrastructure vs query errors, prefix/suffix formatting, hint compositionTracing tests (
tracing.test.ts):withToolTracingfor user, server, and unset categories_errorCategoryproperty on serialized result)Integration tests (
queryTool.test.ts):isErrorisError_errorCategorydoes not leak on the wire result