-
Notifications
You must be signed in to change notification settings - Fork 3
Add PipelineError for plugin pipeline failures #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Adds support for detecting and handling mcpd plugin pipeline failures: - Add PipelineError class with pipelineFlow, serverName, and operation properties - Detect Mcpd-Error-Type header on 500 responses (request-pipeline-failure, response-pipeline-failure) - Enrich PipelineError with server/tool context when called through performCall - Export PipelineFlow type and PIPELINE_FLOW_REQUEST/PIPELINE_FLOW_RESPONSE constants - Add comprehensive tests for pipeline error handling - Update README with PipelineError documentation
WalkthroughThe PR introduces a new Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/unit/client.test.ts (2)
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @tests/unit/client.test.ts:
- Around line 1296-1327: The test duplicates the same mock response and lacks a
fail assertion inside the try/catch; consolidate to a single
mockFetch.mockResolvedValueOnce(...) before the assertions, call await
expect(client.listServers()).rejects.toThrow(PipelineError) and then if you
still want to inspect the error instance use a second await
client.listServers().catch(error => {
expect(error).toBeInstanceOf(PipelineError); const pipelineError = error as
PipelineError; expect(pipelineError.pipelineFlow).toBe("response");
expect(pipelineError.message).toContain("Response processing failed"); }) but
ensure you add an explicit fail() (or another expect to reject) immediately
after the awaited call in the try block so the test fails if no error is thrown;
reference client.listServers, PipelineError, and mockFetch in the changes.
- Around line 1329-1457: Tests that expect an exception (the try/catch blocks
around client.listServers() and client.servers.time!.tools.get_current_time!())
currently lack a failing assertion if no error is thrown; add an immediate
assertion like expect.fail("Expected error to be thrown") right after each
awaited call inside those try blocks (i.e., after await client.listServers() in
the request/500/case-insensitive/other-500 tests and after await
client.servers.time!.tools.get_current_time!({ timezone: "UTC" }) in the
performCall test) so the test fails if no error occurs.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
README.mdsrc/client.tssrc/errors.tssrc/index.tstests/unit/client.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/client.test.ts (1)
src/errors.ts (2)
PipelineError(272-291)McpdError(34-41)
src/client.ts (1)
src/errors.ts (4)
PipelineFlow(23-25)PIPELINE_FLOW_REQUEST(13-13)PIPELINE_FLOW_RESPONSE(18-18)PipelineError(272-291)
src/errors.ts (2)
src/index.ts (5)
PIPELINE_FLOW_REQUEST(26-26)PIPELINE_FLOW_RESPONSE(27-27)PipelineFlow(34-34)PipelineError(25-25)McpdError(22-22)src/client.ts (8)
serverName(386-401)serverName(424-447)serverName(470-489)serverName(512-535)serverName(558-581)serverName(604-614)serverName(736-753)serverName(817-866)
🔇 Additional comments (10)
src/index.ts (1)
25-34: LGTM!The new pipeline-related exports are correctly added and follow the existing re-export pattern. The
PipelineFlowtype is properly exported with thetypekeyword for type-only exports.README.md (1)
612-650: LGTM!The documentation comprehensively covers the new
PipelineErrorclass, including:
- Clear explanation of when pipeline errors occur
- Distinction between request and response pipeline failures
- Complete property documentation
- Practical example showing how to handle different pipeline flows
This provides excellent guidance for SDK users.
tests/unit/client.test.ts (2)
3-8: LGTM!Imports correctly updated to include
McpdErrorandPipelineErrorfrom the errors module.
1387-1438: LGTM!This test thoroughly validates the error enrichment logic, correctly asserting that
serverNameandoperationare populated when aPipelineErrorpropagates throughperformCall. The mock sequence properly simulates the health check, tools list, and tool call chain.src/errors.ts (2)
10-25: LGTM!The pipeline flow constants and type are well-designed:
- Using
as constensures literal type inference- The
PipelineFlowunion type correctly references the constants, providing type safety without string duplication
221-291: LGTM!The
PipelineErrorclass is well-implemented:
- Follows the established pattern of other error classes in this module
- Comprehensive JSDoc documentation with clear explanations of request vs response pipeline failures
- Includes a practical code example in the documentation
- Correctly calls
Error.captureStackTracefor proper stack tracessrc/client.ts (4)
22-25: LGTM!Imports correctly added for the new pipeline error handling functionality.
81-92: LGTM!The header constant and flow mapping are well-designed:
PIPELINE_ERROR_FLOWSuses lowercase keys to support case-insensitive header value matching- The mapping cleanly translates HTTP header values to typed flow constants
256-274: LGTM!The pipeline error detection logic is sound:
- Correctly checks for 500 status before examining the header
- Converts header value to lowercase for case-insensitive matching
- Falls back gracefully through
errorModel?.detail, body, and finally a default message- Leaves
serverNameandoperationundefined for caller enrichment
843-852: LGTM!The error enrichment in
#performCallcorrectly:
- Catches
PipelineErrorand re-throws with server/tool context- Preserves the original
pipelineFlowandcausefrom the caught error- Formats
operationas"serverName.toolName"for clear error context
Adds support for detecting and handling
mcpdplugin pipeline failures:Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.