Add PipelineError for mcpd plugin pipeline failures#38
Conversation
Detect pipeline failures via Mcpd-Error-Type HTTP header on 500 responses. Raises PipelineError with pipeline_flow indicating request or response failure. - Add PipelineError class and PIPELINE_FLOW_REQUEST/RESPONSE constants - Extract HTTP error handling into _raise_for_http_error() helper - Add comprehensive unit tests for error handling - Update README with error handling documentation
WalkthroughAdds pipeline flow constants and a new PipelineError exception, centralises HTTP error handling into a private helper in the client, updates public exports, introduces pipeline-aware error raising, and expands README error-handling docs and unit tests. 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)src/mcpd/mcpd_client.py (1)
🪛 Ruff (0.14.10)src/mcpd/mcpd_client.py52-54: Avoid specifying long messages outside the exception class (TRY003) 57-57: Avoid specifying long messages outside the exception class (TRY003) 74-78: Avoid specifying long messages outside the exception class (TRY003) 81-85: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (4)
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.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/mcpd/exceptions.py:
- Around line 361-379: The __init__ signature of PipelineError has parameters
with default None but lacks explicit union typing; update the annotations to use
Optional[str] (or Union[str, None]) for server_name, operation, and
pipeline_flow in PipelineError.__init__, and mirror this explicit Optional
typing for other exception constructors in the file that use = None (e.g.,
ServerNotFoundError, ToolExecutionError) to keep signatures consistent with PEP
484 and the rest of the module.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.mdsrc/mcpd/__init__.pysrc/mcpd/exceptions.pysrc/mcpd/mcpd_client.pytests/unit/test_exceptions.pytests/unit/test_mcpd_client.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/mcpd/mcpd_client.py (1)
src/mcpd/exceptions.py (4)
McpdError(28-79)PipelineError(328-379)ServerNotFoundError(125-152)ToolExecutionError(224-260)
src/mcpd/__init__.py (1)
src/mcpd/exceptions.py (4)
AuthenticationError(103-122)ConnectionError(82-100)McpdError(28-79)PipelineError(328-379)
tests/unit/test_mcpd_client.py (2)
src/mcpd/exceptions.py (6)
AuthenticationError(103-122)McpdError(28-79)PipelineError(328-379)ServerNotFoundError(125-152)ServerUnhealthyError(155-187)ToolExecutionError(224-260)src/mcpd/mcpd_client.py (3)
HealthStatus(102-118)McpdClient(121-1054)_raise_for_http_error(43-85)
🪛 Ruff (0.14.10)
src/mcpd/mcpd_client.py
52-54: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
74-78: Avoid specifying long messages outside the exception class
(TRY003)
81-85: Avoid specifying long messages outside the exception class
(TRY003)
src/mcpd/exceptions.py
361-361: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
364-364: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
365-365: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
366-366: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (7)
src/mcpd/__init__.py (1)
16-42: LGTM!The new exports for
PipelineError,PIPELINE_FLOW_REQUEST, andPIPELINE_FLOW_RESPONSEare correctly imported and re-exported. The additions follow the existing patterns in the file and properly expose the new public API surface.tests/unit/test_mcpd_client.py (1)
44-193: Comprehensive test coverage for_raise_for_http_error.The test class provides thorough coverage of the new helper function:
- All HTTP status code branches (401, 404, 500 with/without headers, 5xx, 4xx)
- Pipeline flow detection for both request and response failures
- Edge cases including case-insensitive header handling, empty response bodies, and error chaining
The helper method
_make_http_erroris a clean abstraction for creating mock HTTP errors.README.md (1)
283-330: Well-documented error handling section.The expanded documentation provides:
- Clear exception hierarchy table covering all SDK exceptions
- Practical example demonstrating
PipelineErrorhandling with flow constants- Proper exception ordering (specific before generic)
The example correctly shows how to differentiate between request and response pipeline failures using the exported constants.
src/mcpd/mcpd_client.py (2)
43-85: Well-structured HTTP error handling helper.The function correctly centralises HTTP error mapping logic with proper exception chaining. The flow handles:
- Authentication failures (401)
- Not found errors (404)
- Pipeline failures (500 with specific header)
- Generic server errors (5xx)
- Client errors (4xx)
The case-insensitive header lookup via
.lower()ensures robustness against header value casing variations.
268-269: Clean refactoring of HTTP error handling.The integration delegates all HTTP error classification to the new
_raise_for_http_errorhelper, simplifying_perform_callwhilst maintaining the same exception semantics.tests/unit/test_exceptions.py (1)
464-610: Thorough test coverage for PipelineError.The test class provides comprehensive coverage:
- Unit tests for exception attributes and constants
- Verification of the internal
_PIPELINE_ERROR_FLOWSmapping- Integration tests exercising the full error path through
_perform_call- Edge cases including case-insensitive header handling and empty response bodies
The tests correctly mirror the tests in
test_mcpd_client.pywhilst focusing on exception-specific behaviour.src/mcpd/exceptions.py (1)
13-25: Well-defined pipeline flow constants and mapping.The constants and internal mapping are correctly structured:
- Flow constants have clear docstring comments explaining their semantics
- The
_PIPELINE_ERROR_FLOWSmapping uses lowercase keys, aligning with the case-insensitive lookup in_raise_for_http_error
Handle potential None header value when checking for pipeline error type.
Detect pipeline failures via Mcpd-Error-Type HTTP header on 500 responses. Raises PipelineError with pipeline_flow indicating request or response failure.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.