Redesign transport #143
Redesign transport #143
Conversation
WalkthroughThis pull request performs a comprehensive refactoring of the transport layer, replacing the old factory-based API with a modular catalog system. Core changes include renaming classes (TransportProtocol → TransportDriver, HTTPTransport → HttpDriver, etc.), introducing a new DriverCatalog-based builder, reorganizing interfaces into behaviors, and renaming controller classes (TransportManager → ProcessSupervisor). The JsonRpcAdapter now mediates client-transport interactions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
- Coverage 69.94% 69.73% -0.22%
==========================================
Files 108 117 +9
Lines 6905 7154 +249
Branches 1043 1061 +18
==========================================
+ Hits 4830 4989 +159
- Misses 1764 1848 +84
- Partials 311 317 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
mcp_fuzzer/client/tool_client.py (1)
309-382: Two‑phase fuzzing path still callstransport.call_tool– likely runtime bugIn
_process_fuzz_results, tool calls go directly through:result = await self.transport.call_tool(tool_name, args_for_call)However, after this refactor:
- The configured
transportinstance is a driver (e.g.,HttpDriver,StreamHttpDriver,SseDriver,StdioDriver) built viabuild_driver_with_auth, and these drivers exposesend_request/send_raw, notcall_tool.- The new, canonical way to perform JSON‑RPC tool calls is via
self._rpc.call_tool(...), which you already use infuzz_tool.This mismatch means that two‑phase fuzzing (
fuzz_tool_both_phases→_process_fuzz_results) is very likely to raiseAttributeError: '<DriverClass>' object has no attribute 'call_tool'at runtime.You should route this path through the adapter as well, mirroring
fuzz_tool:- try: - result = await self.transport.call_tool(tool_name, args_for_call) + try: + result = await self._rpc.call_tool(tool_name, args_for_call)This keeps all tool calls consistently on the JSON‑RPC adapter and decouples ToolClient from concrete transport driver APIs.
mcp_fuzzer/transport/controller/process_supervisor.py (1)
12-29: Fix duplicate field definition.The
restart_countfield is defined twice in theProcessStatedataclass (lines 20 and 25). This is redundant and may cause unexpected behavior.Apply this diff to remove the duplicate:
@dataclass class ProcessState: """Lightweight state container for a transport-managed process. ``restart_count`` tracks the lifetime number of restarts (used for reporting), while :class:`ProcessSupervisor._restart_attempts` counts consecutive restart trials within the current backoff window. """ - restart_count: int = 0 - pid: int | None = None started_at: float | None = None exited_at: float | None = None restart_count: int = 0 last_signal: str | None = None last_error: str | None = None last_stdout_tail: str | None = None last_stderr_tail: str | None = Nonemcp_fuzzer/transport/drivers/sse_driver.py (1)
182-230: Consider extracting the duplicated event processing logic.The async and sync iterable handling (lines 183-206 and 207-230) contain nearly identical logic. Consider extracting the line processing into a helper method to reduce duplication.
+ def _process_stream_line( + self, line: str, buffer: list[str] + ) -> tuple[dict[str, Any] | None, list[str]]: + """Process a single line from the stream. + + Returns: + Tuple of (parsed_event_or_none, updated_buffer) + """ + if line.strip(): + buffer.append(line) + return None, buffer + + if not buffer: + return None, buffer + + try: + event_text = "\n".join(buffer) + parsed = self.parse_sse_event(event_text) + return parsed, [] + except json.JSONDecodeError: + self._logger.error("Failed to parse SSE event payload as JSON") + return None, []mcp_fuzzer/transport/drivers/stream_http_driver.py (1)
127-134: Bareexcept Exception: passsilently swallows all errors.This catch-all suppresses legitimate issues (e.g., unexpected types, attribute errors on mock objects in tests). At minimum, log the exception at debug level.
try: if isinstance(result, dict) and "protocolVersion" in result: pv = result.get("protocolVersion") if pv is not None: self.protocol_version = str(pv) self._logger.debug("Negotiated protocol version: %s", pv) - except Exception: - pass + except Exception as e: + self._logger.debug("Failed to extract protocol version: %s", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (37)
mcp_fuzzer/cli/validators.py(4 hunks)mcp_fuzzer/client/main.py(2 hunks)mcp_fuzzer/client/tool_client.py(6 hunks)mcp_fuzzer/client/transport/__init__.py(1 hunks)mcp_fuzzer/client/transport/factory.py(3 hunks)mcp_fuzzer/config/loader.py(8 hunks)mcp_fuzzer/events.py(2 hunks)mcp_fuzzer/transport/__init__.py(1 hunks)mcp_fuzzer/transport/catalog/__init__.py(1 hunks)mcp_fuzzer/transport/catalog/builder.py(1 hunks)mcp_fuzzer/transport/catalog/catalog.py(1 hunks)mcp_fuzzer/transport/catalog/custom_catalog.py(1 hunks)mcp_fuzzer/transport/catalog/resolver.py(1 hunks)mcp_fuzzer/transport/controller/__init__.py(1 hunks)mcp_fuzzer/transport/controller/coordinator.py(1 hunks)mcp_fuzzer/transport/controller/process_supervisor.py(3 hunks)mcp_fuzzer/transport/custom.py(0 hunks)mcp_fuzzer/transport/drivers/__init__.py(1 hunks)mcp_fuzzer/transport/drivers/http_driver.py(1 hunks)mcp_fuzzer/transport/drivers/sse_driver.py(6 hunks)mcp_fuzzer/transport/drivers/stdio_driver.py(4 hunks)mcp_fuzzer/transport/drivers/stream_http_driver.py(13 hunks)mcp_fuzzer/transport/factory.py(0 hunks)mcp_fuzzer/transport/interfaces/__init__.py(1 hunks)mcp_fuzzer/transport/interfaces/behaviors.py(8 hunks)mcp_fuzzer/transport/interfaces/driver.py(1 hunks)mcp_fuzzer/transport/interfaces/rpc_adapter.py(3 hunks)mcp_fuzzer/transport/interfaces/states.py(1 hunks)tests/integration/test_client_transport.py(3 hunks)tests/integration/test_custom_transport.py(9 hunks)tests/integration/test_standardized_output.py(9 hunks)tests/unit/cli/test_cli.py(12 hunks)tests/unit/client/test_client_main.py(11 hunks)tests/unit/transport/test_custom.py(23 hunks)tests/unit/transport/test_stdio.py(11 hunks)tests/unit/transport/test_streamable_http.py(5 hunks)tests/unit/transport/test_transport.py(17 hunks)
💤 Files with no reviewable changes (2)
- mcp_fuzzer/transport/factory.py
- mcp_fuzzer/transport/custom.py
🧰 Additional context used
🧬 Code graph analysis (23)
mcp_fuzzer/config/loader.py (3)
mcp_fuzzer/transport/catalog/custom_catalog.py (1)
register_custom_driver(192-210)mcp_fuzzer/transport/interfaces/driver.py (1)
TransportDriver(13-116)mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)
mcp_fuzzer/transport/controller/__init__.py (2)
mcp_fuzzer/transport/controller/coordinator.py (1)
TransportCoordinator(18-242)mcp_fuzzer/transport/controller/process_supervisor.py (2)
ProcessSupervisor(56-142)ProcessState(12-53)
tests/unit/transport/test_streamable_http.py (1)
mcp_fuzzer/transport/drivers/stream_http_driver.py (1)
StreamHttpDriver(49-505)
mcp_fuzzer/transport/drivers/__init__.py (4)
mcp_fuzzer/transport/drivers/http_driver.py (1)
HttpDriver(26-315)mcp_fuzzer/transport/drivers/sse_driver.py (1)
SseDriver(17-240)mcp_fuzzer/transport/drivers/stdio_driver.py (1)
StdioDriver(27-528)mcp_fuzzer/transport/drivers/stream_http_driver.py (1)
StreamHttpDriver(49-505)
mcp_fuzzer/transport/controller/coordinator.py (4)
mcp_fuzzer/transport/interfaces/driver.py (3)
TransportDriver(13-116)send_request(22-37)send_raw(40-52)mcp_fuzzer/transport/interfaces/rpc_adapter.py (3)
JsonRpcAdapter(17-182)get_tools(42-77)call_tool(79-100)mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (2)
build_driver(253-288)list_transports(189-209)
tests/unit/transport/test_custom.py (6)
mcp_fuzzer/transport/interfaces/driver.py (2)
TransportDriver(13-116)send_request(22-37)mcp_fuzzer/transport/interfaces/rpc_adapter.py (2)
JsonRpcAdapter(17-182)get_tools(42-77)mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (5)
build_driver(253-288)list_custom_drivers(232-251)clear(290-293)list_transports(189-209)register(47-102)mcp_fuzzer/transport/catalog/custom_catalog.py (7)
build_driver(159-185)CustomDriverCatalog(20-185)register_custom_driver(192-210)list_custom_drivers(227-233)clear(33-41)list_transports(151-157)register(43-72)mcp_fuzzer/exceptions.py (2)
ConnectionError(55-59)TransportRegistrationError(90-94)
mcp_fuzzer/client/transport/__init__.py (1)
mcp_fuzzer/client/transport/factory.py (1)
build_driver_with_auth(17-60)
mcp_fuzzer/transport/interfaces/behaviors.py (3)
mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)mcp_fuzzer/transport/interfaces/states.py (1)
DriverState(13-21)mcp_fuzzer/transport/drivers/http_driver.py (1)
_update_activity(89-91)
mcp_fuzzer/transport/drivers/http_driver.py (3)
mcp_fuzzer/transport/interfaces/driver.py (1)
TransportDriver(13-116)mcp_fuzzer/transport/interfaces/behaviors.py (3)
HttpClientBehavior(286-388)ResponseParserBehavior(391-479)LifecycleBehavior(482-611)mcp_fuzzer/safety_system/policy.py (1)
resolve_redirect_safely(115-138)
tests/unit/cli/test_cli.py (1)
mcp_fuzzer/client/transport/factory.py (1)
build_driver_with_auth(17-60)
tests/integration/test_standardized_output.py (4)
mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (1)
build_driver(253-288)mcp_fuzzer/transport/controller/coordinator.py (1)
build_driver(45-87)mcp_fuzzer/reports/reporter/contracts.py (1)
add_tool_results(19-20)
mcp_fuzzer/transport/interfaces/__init__.py (4)
mcp_fuzzer/transport/interfaces/driver.py (1)
TransportDriver(13-116)mcp_fuzzer/transport/interfaces/states.py (2)
DriverState(13-21)ParsedEndpoint(25-58)mcp_fuzzer/transport/interfaces/behaviors.py (7)
DriverBaseBehavior(104-283)HttpClientBehavior(286-388)ResponseParserBehavior(391-479)LifecycleBehavior(482-611)TransportError(74-77)NetworkError(80-83)PayloadValidationError(86-89)mcp_fuzzer/transport/interfaces/rpc_adapter.py (1)
JsonRpcAdapter(17-182)
mcp_fuzzer/transport/controller/process_supervisor.py (2)
mcp_fuzzer/events.py (1)
ProcessEventObserver(39-42)mcp_fuzzer/transport/interfaces/behaviors.py (1)
TransportError(74-77)
mcp_fuzzer/client/transport/factory.py (4)
mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (1)
build_driver(253-288)mcp_fuzzer/transport/catalog/custom_catalog.py (1)
build_driver(159-185)mcp_fuzzer/transport/controller/coordinator.py (1)
build_driver(45-87)
mcp_fuzzer/transport/catalog/resolver.py (2)
mcp_fuzzer/transport/interfaces/states.py (1)
ParsedEndpoint(25-58)mcp_fuzzer/transport/catalog/catalog.py (3)
DriverCatalog(36-293)is_registered(122-131)is_custom_transport(133-145)
tests/integration/test_custom_transport.py (6)
mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (2)
build_driver(253-288)list_custom_drivers(232-251)mcp_fuzzer/transport/catalog/custom_catalog.py (3)
build_driver(159-185)register_custom_driver(192-210)list_custom_drivers(227-233)mcp_fuzzer/transport/interfaces/driver.py (1)
TransportDriver(13-116)mcp_fuzzer/transport/interfaces/rpc_adapter.py (3)
JsonRpcAdapter(17-182)get_tools(42-77)call_tool(79-100)mcp_fuzzer/exceptions.py (1)
TransportRegistrationError(90-94)
mcp_fuzzer/client/tool_client.py (2)
mcp_fuzzer/transport/interfaces/rpc_adapter.py (3)
JsonRpcAdapter(17-182)get_tools(42-77)call_tool(79-100)mcp_fuzzer/transport/controller/coordinator.py (2)
get_tools(187-197)call_tool(199-213)
tests/integration/test_client_transport.py (1)
mcp_fuzzer/transport/drivers/stream_http_driver.py (1)
StreamHttpDriver(49-505)
mcp_fuzzer/transport/__init__.py (7)
mcp_fuzzer/transport/interfaces/states.py (2)
DriverState(13-21)ParsedEndpoint(25-58)mcp_fuzzer/transport/interfaces/behaviors.py (1)
DriverBaseBehavior(104-283)mcp_fuzzer/transport/interfaces/rpc_adapter.py (1)
JsonRpcAdapter(17-182)mcp_fuzzer/transport/catalog/catalog.py (3)
DriverCatalog(36-293)build_driver(253-288)list_custom_drivers(232-251)mcp_fuzzer/transport/catalog/custom_catalog.py (3)
build_driver(159-185)CustomDriverCatalog(20-185)list_custom_drivers(227-233)mcp_fuzzer/transport/catalog/resolver.py (1)
EndpointResolver(18-161)mcp_fuzzer/transport/controller/process_supervisor.py (2)
ProcessSupervisor(56-142)ProcessState(12-53)
tests/unit/transport/test_transport.py (7)
mcp_fuzzer/transport/drivers/http_driver.py (2)
HttpDriver(26-315)send_request(108-150)mcp_fuzzer/transport/drivers/sse_driver.py (2)
SseDriver(17-240)send_request(52-62)mcp_fuzzer/transport/interfaces/driver.py (4)
TransportDriver(13-116)connect(69-75)disconnect(77-83)send_request(22-37)mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (1)
build_driver(253-288)mcp_fuzzer/transport/interfaces/behaviors.py (4)
DriverBaseBehavior(104-283)HttpClientBehavior(286-388)ResponseParserBehavior(391-479)parse_sse_event(394-428)tests/unit/transport/test_custom.py (9)
send_request(26-29)send_request(326-331)send_request(390-393)send_request(447-450)send_request(491-494)send_request(527-530)send_request(588-599)send_request(660-663)send_request(705-708)
mcp_fuzzer/transport/catalog/catalog.py (4)
mcp_fuzzer/transport/interfaces/driver.py (1)
TransportDriver(13-116)mcp_fuzzer/exceptions.py (1)
TransportRegistrationError(90-94)mcp_fuzzer/transport/catalog/custom_catalog.py (8)
register(43-72)unregister(74-97)get_transport_class(99-123)get_transport_info(125-149)list_transports(151-157)list_custom_drivers(227-233)build_driver(159-185)clear(33-41)mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)
mcp_fuzzer/cli/validators.py (4)
mcp_fuzzer/transport/catalog/builder.py (1)
build_driver(18-74)mcp_fuzzer/transport/catalog/catalog.py (1)
build_driver(253-288)mcp_fuzzer/transport/catalog/custom_catalog.py (1)
build_driver(159-185)mcp_fuzzer/transport/controller/coordinator.py (1)
build_driver(45-87)
mcp_fuzzer/transport/drivers/sse_driver.py (2)
mcp_fuzzer/transport/interfaces/driver.py (3)
TransportDriver(13-116)send_raw(40-52)_stream_request(103-116)mcp_fuzzer/transport/interfaces/behaviors.py (9)
HttpClientBehavior(286-388)ResponseParserBehavior(391-479)TransportError(74-77)_validate_network_request(289-301)_prepare_safe_headers(303-312)_create_http_client(314-327)_handle_http_response_error(329-343)parse_sse_event(394-428)_extract_result_from_response(258-283)
🔇 Additional comments (69)
mcp_fuzzer/transport/interfaces/rpc_adapter.py (1)
144-182: Collation logic for batch responses is robust and appropriate for JSON‑RPCThe
collate_batch_responsesmethod correctly:
- Tracks only non‑None request IDs as expected responses.
- Associates responses by
idwhile warning on unmatched responses.- Synthesizes a JSON‑RPC style error object for missing responses.
This gives callers a predictable map without silently dropping failures; no changes needed here.
mcp_fuzzer/client/tool_client.py (4)
21-46: RPC adapter wiring in ToolClient constructor is appropriateImporting
JsonRpcAdapterand initializingself._rpc = JsonRpcAdapter(transport)centralizes JSON‑RPC operations behind a single helper while keeping the raw transport accessible asself.transport. This is a clean layering choice and aligns with the new transport abstractions.
59-76: Delegating tool discovery to JsonRpcAdapter is consistent with new transport APISwitching
_get_tools_from_serverto:tools = await self._rpc.get_tools()is consistent with the adapter’s responsibility and keeps ToolClient free of JSON‑RPC details. The additional guard/logging when
toolsis empty is reasonable given that the adapter already logs malformed responses.
96-101: Minor style change on task naming is harmlessAdding the trailing comma in
asyncio.create_task(..., name=...)has no behavioral impact and can slightly reduce future diff noise. No further action needed.
184-196: Using JsonRpcAdapter for tool calls aligns with transport refactorRouting fuzzed tool invocations through:
result = await self._rpc.call_tool(tool_name, args_for_call)correctly reuses the JSON‑RPC helper and avoids assuming any higher‑level API on the underlying transport drivers (which only provide
send_request/send_raw). This is the right abstraction boundary.tests/integration/test_client_transport.py (1)
10-20: Integration test correctly targetsStreamHttpDriverUpdating the fixture and type assertion to use
StreamHttpDriverinstead of the oldStreamableHTTPTransportkeeps this test aligned with the new drivers package. The_initializedshortcut remains appropriate for avoiding extra handshake mocking.Also applies to: 29-36
mcp_fuzzer/client/main.py (1)
16-17: Main client entrypoint correctly switched tobuild_driver_with_authSwapping
create_transport_with_authforbuild_driver_with_authin both the import and call site keeps the CLI entrypoint in sync with the new client transport factory. The test suite already patches this symbol by its new name, so this change is consistent end‑to‑end.Also applies to: 40-51
mcp_fuzzer/transport/drivers/__init__.py (1)
1-13: Driver re-exports provide a clean public surfaceRe-exporting
HttpDriver,SseDriver,StdioDriver, andStreamHttpDriverfrom the drivers package via__all__is a good consolidation step and supports stable import paths for callers and tests.tests/unit/client/test_client_main.py (1)
55-63: Unit tests correctly updated to patchbuild_driver_with_authAll the patched targets have been updated to
mcp_fuzzer.client.main.build_driver_with_auth, matching the new import inclient.main. This keeps the tests exercisingunified_client_mainbehavior without depending on real transport construction.Also applies to: 78-85, 99-107, 120-127, 165-172, 192-199, 212-219, 234-241, 253-260
mcp_fuzzer/client/transport/__init__.py (1)
1-3: Client transport package now exposesbuild_driver_with_authas intendedImporting and re-exporting
build_driver_with_authhere keeps a single, clear public entrypoint for building transports from client code and tests. This matches howclient.mainand the tests are now consuming the factory.mcp_fuzzer/events.py (1)
5-26: Event contract documentation correctly updated forProcessSupervisorThe docstring now accurately attributes
signal_failedandoversized_outputtoProcessSupervisorand replaces the oldTransportManagernaming where appropriate. This keeps the documented event surface in sync with the refactored controller components.mcp_fuzzer/cli/validators.py (1)
15-15: LGTM! Import and API usage updated correctly.The import and function call have been updated to use the new
build_driverAPI from the transport catalog, aligning with the broader refactoring across the PR.Also applies to: 161-165
mcp_fuzzer/transport/drivers/stdio_driver.py (1)
14-46: LGTM! Class and import refactoring executed correctly.The class has been successfully renamed from
StdioTransporttoStdioDriver, with all related imports and manager references updated to align with the new architecture (TransportDriver,ProcessSupervisor).mcp_fuzzer/transport/controller/process_supervisor.py (1)
56-79: LGTM! Class renamed correctly.The class has been successfully renamed from
TransportManagertoProcessSupervisorwith all references and initialization updated appropriately.tests/integration/test_standardized_output.py (1)
14-14: LGTM! Import updated to new transport API.The import has been correctly updated to use
build_driverfrom the new transport catalog, aligning with the broader API refactoring.tests/unit/transport/test_streamable_http.py (1)
6-9: LGTM! Test updated for renamed transport driver.All references to
StreamableHTTPTransporthave been correctly updated toStreamHttpDriver, maintaining test behavior and assertions.Also applies to: 112-112, 149-149, 168-168, 189-189
mcp_fuzzer/transport/controller/__init__.py (1)
1-23: LGTM! Lazy loading implemented correctly.The lazy loading pattern using
__getattr__is properly implemented, providing on-demand imports forTransportCoordinator,ProcessSupervisor, andProcessState. This is an idiomatic Python pattern that can help with circular import issues and startup performance.mcp_fuzzer/transport/drivers/http_driver.py (1)
10-54: LGTM! HTTP driver refactored to use behavior composition.The class has been successfully renamed to
HttpDriverand updated to use the new behavior-based architecture (TransportDriver,HttpClientBehavior,ResponseParserBehavior,LifecycleBehavior). The documentation clearly explains the composition pattern and the responsibilities of each behavior.tests/unit/cli/test_cli.py (1)
25-25: LGTM! Tests updated for new transport API.All imports, function calls, and patch targets have been correctly updated to use the new transport API (
build_driver_with_auth,base_build_driver,build_driver), maintaining test coverage and assertions.Also applies to: 313-320, 400-404, 411-415
mcp_fuzzer/client/transport/factory.py (1)
12-12: Auth-aware factory correctly migrated to catalog-based builderThe switch to
base_build_driverwith(args.protocol, args.endpoint, **factory_kwargs)keeps the previous semantics while integrating with the new catalog; HTTP-family protocols still get auth headers only when available, and the public API surface (build_driver_with_auth+__all__) is updated consistently. No issues spotted here.Also applies to: 17-55, 63-63
mcp_fuzzer/transport/interfaces/states.py (1)
1-58: DriverState and ParsedEndpoint abstractions look solidThe lifecycle states are comprehensive and match the usage pattern in
LifecycleBehavior, andParsedEndpointcleanly encapsulates resolver output with a sensible__post_init__fallback tooriginal_urlwhenendpointis empty. No behavioral issues identified.mcp_fuzzer/transport/interfaces/__init__.py (1)
1-28: Interfaces package re-exports are coherent and consistentThe module aggregates the key driver interface, states, behaviors, errors, and
JsonRpcAdapterunder a single namespace with a matching__all__, which should make external usage much cleaner. Looks good.mcp_fuzzer/transport/catalog/__init__.py (1)
1-24: Catalog package surface is well-structuredThe initializer exports the unified registry (
DriverCatalog/driver_catalog), the high-levelbuild_driverfactory, resolver, and custom-driver helpers, giving callers a single, consistent entrypoint into the transport catalog. No issues noted.mcp_fuzzer/transport/catalog/builder.py (1)
15-75: build_driver correctly fronts the catalog with resolver-driven parsingThe factory uses
EndpointResolverto normalize(url_or_protocol, endpoint)into aParsedEndpoint, validates the scheme against the registry, and delegates todriver_catalog.build_driverwith clear, informative error messages including available built-in and custom schemes. This is a clean, centralized entrypoint for driver construction and the error-wrapping withTransportRegistrationErroris appropriate.mcp_fuzzer/transport/interfaces/driver.py (1)
1-116: TransportDriver interface cleanly defines the new transport contractThe abstract methods and streaming pattern (
stream_requestdelegating to_stream_request) establish a clear, transport-focused contract, while the default no-opconnect/disconnectkeep simple drivers lightweight. This aligns well with the rest of the refactor that moves JSON-RPC specifics intoJsonRpcAdapter.tests/unit/transport/test_stdio.py (4)
10-10: LGTM: Import updated to new module path.The import correctly references the new
StdioDriverclass from the restructureddrivers.stdio_drivermodule.
15-20: LGTM: Test class and fixture updated consistently.The test class rename to
TestStdioDriverand instantiation ofStdioDrivercorrectly align with the transport redesign.
38-39: LGTM: Patch targets consistently updated.All patch decorators and context managers correctly reference the new module path
mcp_fuzzer.transport.drivers.stdio_driverforasyncio.create_subprocess_exec,shlex.split,uuid,os, andlogging.Also applies to: 71-71, 88-88, 213-213, 240-240, 266-266, 395-398, 418-421, 440-443
264-270: The concern is invalid; the project explicitly requires Python 3.10+, which fully supports parenthesized context managers.The project's
pyproject.tomldeclaresrequires-python = ">=3.10"and explicitly classifies support for Python 3.10 through 3.14. Parenthesized context managers were introduced in PEP 617 and are available since Python 3.10. The syntax used at lines 264-270 and 474-479 is compatible with the project's minimum Python version requirement and requires no changes.tests/unit/transport/test_transport.py (7)
15-31: LGTM: Imports updated to new public API surface.The imports correctly reference the renamed classes (
HttpDriver,SseDriver,StdioDriver,TransportDriver) and the newbuild_driverfactory. The behavior interfaces are properly imported from the newinterfaces.behaviorsmodule.
37-43: LGTM: Abstract class test updated.The test correctly verifies that
TransportDrivercannot be instantiated directly, maintaining the abstract base class contract.
157-161: LGTM: HttpDriver fixture updated.The fixture correctly instantiates
HttpDriverand the docstring reflects the new class name.
294-298: LGTM: SseDriver fixture updated.The fixture correctly instantiates
SseDriverwith appropriate test URL.
369-394: LGTM: SSE event parsing tests updated.The tests correctly instantiate
SseDriverand callparse_sse_eventmethod, which is now inherited fromResponseParserBehavior.
397-404: LGTM: StdioDriver fixture updated.The fixture correctly patches the new module path and instantiates
StdioDriver.
491-528: LGTM: Factory function tests updated to usebuild_driver.All
create_transportcalls have been replaced withbuild_driver, and the tests correctly verify:
- HTTP/HTTPS URLs return
HttpDriver- SSE URLs return
SseDriver- stdio protocol returns
StdioDriver- Protocol+endpoint pattern works for built-in transports
- Invalid schemes raise
TransportRegistrationErrormcp_fuzzer/transport/__init__.py (1)
1-35: LGTM: Clean modular public API surface.The restructured exports provide a well-organized API with clear separation:
- Interfaces: Core types, behaviors, and errors
- Drivers: Concrete implementations
- Catalog: Registration and building utilities
- Controller: Process management primitives
The imports are comprehensive and the
__all__list correctly reflects all exports.tests/integration/test_custom_transport.py (6)
10-12: LGTM: Imports updated to new public API.The imports correctly reference the renamed functions (
build_driver,register_custom_driver) and new types (TransportDriver,JsonRpcAdapter,custom_driver_catalog).
16-68: LGTM: Test transport updated to implementTransportDriver.
IntegrationTestTransportcorrectly inherits fromTransportDriverand implements all required abstract methods:send_request,send_raw,send_notification, and_stream_request.
74-77: LGTM: Setup methods correctly clear custom transports.The setup methods properly use
custom_driver_catalog.list_transports()andcustom_driver_catalog.unregister()to clean up custom transports between tests.Also applies to: 134-137, 201-204, 239-242
177-191: LGTM: JsonRpcAdapter integration test.The test correctly demonstrates the new pattern of using
JsonRpcAdapterto mediate JSON-RPC operations (get_tools) on top of a transport instance.
212-215: LGTM: Error message expectations updated.The expected error messages correctly reflect the new API:
"must inherit from TransportDriver"for invalid class registration"Unsupported transport scheme"for unknown transport creationAlso applies to: 230-233
265-269: LGTM: RPC helper usage for tool calls.The test correctly uses
JsonRpcAdapter.call_tool()to invoke tools through the transport, demonstrating the new separation of concerns where RPC operations are handled by the adapter.mcp_fuzzer/transport/catalog/resolver.py (3)
1-15: LGTM: Clean module structure with TYPE_CHECKING import.The module correctly uses
TYPE_CHECKINGto avoid circular imports withDriverCatalog, and the docstring clearly explains the module's purpose.
90-124: LGTM: URL parsing implementation.The
_parse_urlmethod correctly:
- Handles non-standard schemes that
urlparsedoesn't recognize- Uses
is_custom_transport()to determine custom status- Populates all ParsedEndpoint fields from parsed components
126-161: LGTM: Endpoint resolution logic.The
_resolve_endpointmethod correctly handles:
- stdio: Extracts command from netloc/path
- sse/streamablehttp: Converts to HTTP URL
- http/https: Returns original URL
- Custom transports: Falls through to return original URL
mcp_fuzzer/transport/drivers/sse_driver.py (4)
17-27: Well-structured mixin composition.The class effectively uses mixin-based composition to share functionality across HTTP-based transports. The docstring clearly documents the mixin responsibilities.
52-62: LGTM!Correctly raises
NotImplementedErrorsince SSE transport only supports streaming requests.
64-128: LGTM!The
send_rawmethod correctly implements SSE parsing with appropriate fallback to JSON. The buffer-based approach handles SSE events properly, and the error handling is consistent with the mixin patterns.
130-150: LGTM!Notification handling is correctly implemented using shared mixin utilities. The fire-and-forget semantics are preserved.
mcp_fuzzer/transport/controller/coordinator.py (3)
45-87: LGTM!The
build_drivermethod correctly delegates to the builder function, tracks transports by ID, and wraps exceptions consistently. The local import to avoid circular dependencies is appropriate.
115-139: LGTM!The disconnect method correctly logs errors without re-raising, following best practices for cleanup operations where we want to continue cleanup even if individual disconnects fail.
231-242: LGTM!The cleanup method correctly creates a copy of active transports for safe iteration and ensures complete cleanup even if individual disconnects fail.
mcp_fuzzer/transport/interfaces/behaviors.py (5)
30-89: LGTM!The JSON-RPC type definitions correctly follow the JSON-RPC 2.0 specification, and the exception hierarchy provides clear categorization for transport errors.
314-327: LGTM!The HTTP client configuration includes good security defaults: disabled redirects prevent SSRF redirect attacks, and
trust_env=Falseprevents unintended proxy usage from environment variables.
430-479: LGTM!The
parse_streaming_responsemethod includes appropriate memory protection with the configurable buffer size limit, preventing potential DoS through large SSE events.
482-605: LGTM!The
LifecycleBehaviorclass provides comprehensive lifecycle management with proper state tracking, timing metrics, and safe callback execution that catches exceptions to prevent callback failures from breaking the transport.
206-209: String formatting issue creates incorrect error message.The backslash-continuation in the string literal will include the backslash in the error message instead of joining the lines.
- raise PayloadValidationError( - "Response must have exactly one of \ -result or error" - ) + raise PayloadValidationError( + "Response must have exactly one of result or error" + )Likely an incorrect or invalid review comment.
mcp_fuzzer/transport/catalog/catalog.py (1)
47-102: LGTM!The registration method provides thorough validation with clear error messages, case-insensitive key handling, and appropriate logging.
tests/unit/transport/test_custom.py (5)
45-174: LGTM!The test class provides comprehensive coverage of the
CustomDriverCatalogAPI with proper isolation throughsetup_methodcleanup and fresh registry instances per test.
269-311: LGTM!The compliance tests verify all required
TransportDriverinterface methods and properly restore mocked methods using afinallyblock.
314-424: LGTM!Thorough testing of transport close behavior, including state verification after close and resource cleanup patterns.
567-751: LGTM!Comprehensive testing of self-registered transport instantiation, including direct registry instantiation, factory URL format, and custom factory functions.
7-15:build_custom_driveris correctly defined and exported fromcustom_catalog.The function is defined in
mcp_fuzzer/transport/catalog/custom_catalog.pyat line 213 and properly exported through the module's__init__.pyand re-exported at package level. The import in the test file is valid.mcp_fuzzer/transport/drivers/stream_http_driver.py (5)
41-46: Back-compat aliases are a good practice.These aliases help maintain backward compatibility with existing tests while the codebase transitions to the new naming conventions.
63-91: LGTM on initialization.The use of
asyncio.Lockfor synchronizing the initialization handshake is appropriate. The defaultauth_headers=Nonecorrectly avoids the mutable default argument pitfall.
176-226: SSE parsing logic is solid.The implementation correctly handles JSON-RPC responses embedded in SSE. Note that
lstrip()on line 220 removes all leading whitespace, while the SSE spec technically says to strip only a single leading space. For a fuzzer, this more permissive behavior is likely intentional.
346-351: Single redirect follow is intentional but worth documenting.The code follows at most one redirect. If this is a security measure to prevent redirect chains, consider adding a comment. If multiple redirects should be supported, a loop with a max-redirects limit would be needed.
473-483: I'll verify the review comment about httpx streaming API usage. Let me start by examining the actual code and understanding how httpx's streaming API works.
<function_calls>
cat -n mcp_fuzzer/transport/drivers/stream_http_driver.py | head -100
</function_calls>Let me get more context around the lines in question:
<function_calls>
sed -n '450,510p' mcp_fuzzer/transport/drivers/stream_http_driver.py
</function_calls>Now let me search for httpx usage patterns in the codebase:
<function_calls>
rg -n "client.stream" --type py -B2 -A2
</function_calls>Let me also check the imports and dependencies to understand which httpx version is being used:
<function_calls>
head -50 mcp_fuzzer/transport/drivers/stream_http_driver.py
</function_calls>Now let me verify the httpx documentation for the correct streaming API usage:
<function_calls>
httpx AsyncClient stream method return type async context manager
</function_calls>I need to also check if there's error handling or context manager usage around this code:
<function_calls>
sed -n '456,510p' mcp_fuzzer/transport/drivers/stream_http_driver.py | cat -n
</function_calls><function_calls>
grep -n "requirements" . -r --include=".txt" --include=".toml" | head -5
</function_calls><function_calls>
find . -name "pyproject.toml" -o -name "requirements*.txt" | head -3
</function_calls><function_calls>
cat pyproject.toml | grep -A 20 "dependencies|httpx"
</function_calls>
| from ..transport.catalog.custom_catalog import register_custom_driver | ||
| from ..transport.interfaces import TransportDriver | ||
| import importlib |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Custom transport loading correctly targets the new TransportDriver interface
The loader now cleanly aligns with the redesigned transport layer: it imports TransportDriver, validates custom classes with issubclass(transport_class, TransportDriver), and registers them through register_custom_driver, with the schema text updated to reference build_driver. This tightens type safety around the new driver abstraction; just be aware this is a breaking change for any external custom transports that previously subclassed the old protocol base and will now need to adopt TransportDriver.
Also applies to: 137-257, 330-376
🤖 Prompt for AI Agents
In mcp_fuzzer/config/loader.py around lines 16-18 (and similarly at 137-257 and
330-376), the loader must validate and register custom transport classes against
the new TransportDriver interface: ensure importlib is used to load the module,
import TransportDriver at the top, call issubclass(transport_class,
TransportDriver) when validating discovered classes, and pass the validated
class to register_custom_driver (ensuring schema references build_driver if
applicable); update any old protocol-base checks to use TransportDriver so
external custom transports are rejected or adapted until they subclass the new
interface.
| from .controller.coordinator import TransportCoordinator | ||
| from .controller.process_supervisor import ProcessSupervisor, ProcessState |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cat mcp_fuzzer/transport/controller/__init__.py 2>/dev/null || echo "File not found"Repository: Agent-Hellboy/mcp-server-fuzzer
Length of output: 711
Import from .controller package to leverage its re-export interface.
The controller package's __init__.py explicitly re-exports TransportCoordinator, ProcessSupervisor, and ProcessState via __all__ and lazy-loading through __getattr__. Importing from .controller instead of directly from submodules will align with the package's intended interface and eliminate unnecessary direct submodule coupling.
-from .controller.coordinator import TransportCoordinator
-from .controller.process_supervisor import ProcessSupervisor, ProcessState
+from .controller import TransportCoordinator, ProcessSupervisor, ProcessState📝 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.
| from .controller.coordinator import TransportCoordinator | |
| from .controller.process_supervisor import ProcessSupervisor, ProcessState | |
| from .controller import TransportCoordinator, ProcessSupervisor, ProcessState |
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/__init__.py around lines 33-34, the file currently
imports TransportCoordinator, ProcessSupervisor, and ProcessState directly from
submodules; change these imports to import from the package re-export interface
by importing them from .controller instead (e.g. from .controller import
TransportCoordinator, ProcessSupervisor, ProcessState) so the code uses the
controller package's __all__ and __getattr__ lazy-loading and removes direct
submodule coupling.
| # Register built-in transports with the global unified registry | ||
| def _register_builtin_transports(): | ||
| """Register all built-in transport types.""" | ||
| from ..drivers.http_driver import HttpDriver | ||
| from ..drivers.sse_driver import SseDriver | ||
| from ..drivers.stdio_driver import StdioDriver | ||
| from ..drivers.stream_http_driver import StreamHttpDriver | ||
|
|
||
| # Only register if not already registered (allow tests to override) | ||
| transports = { | ||
| "http": HttpDriver, | ||
| "https": HttpDriver, | ||
| "sse": SseDriver, | ||
| "stdio": StdioDriver, | ||
| "streamablehttp": StreamHttpDriver, | ||
| } | ||
|
|
||
| for name, cls in transports.items(): | ||
| if not driver_catalog.is_registered(name): | ||
| driver_catalog.register( | ||
| name, | ||
| cls, | ||
| description=f"Built-in {name.upper()} driver", | ||
| is_custom=False, | ||
| ) | ||
|
|
||
|
|
||
| # Register built-in transports on module import | ||
| _register_builtin_transports() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import-time registration of built-in drivers relies on importing this module
Registering built-in drivers inside _register_builtin_transports() and calling it at module import time works well as long as all code paths obtain the catalog via this builder (or the transport.catalog package, which imports it). Any code that imports driver_catalog directly from .catalog without going through this module would see an empty registry. That’s acceptable if the intended public entrypoints are build_driver and mcp_fuzzer.transport.catalog, but it’s worth keeping in mind for future imports to avoid subtle initialization bugs.
| class TransportMetadata: | ||
| """Metadata for a registered transport.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| transport_class: Type[TransportDriver], | ||
| description: str = "", | ||
| config_schema: dict[str, Any] | None = None, | ||
| factory_function: Callable | None = None, | ||
| is_custom: bool = False, | ||
| ): | ||
| self.transport_class = transport_class | ||
| self.description = description | ||
| self.config_schema = config_schema | ||
| self.factory_function = factory_function | ||
| self.is_custom = is_custom |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using @dataclass for TransportMetadata.
The class is a simple data holder that could benefit from @dataclass for automatic __repr__, __eq__, etc.
+from dataclasses import dataclass
+
+@dataclass
class TransportMetadata:
"""Metadata for a registered transport."""
-
- def __init__(
- self,
- transport_class: Type[TransportDriver],
- description: str = "",
- config_schema: dict[str, Any] | None = None,
- factory_function: Callable | None = None,
- is_custom: bool = False,
- ):
- self.transport_class = transport_class
- self.description = description
- self.config_schema = config_schema
- self.factory_function = factory_function
- self.is_custom = is_custom
+ transport_class: Type[TransportDriver]
+ description: str = ""
+ config_schema: dict[str, Any] | None = None
+ factory_function: Callable | None = None
+ is_custom: bool = False🤖 Prompt for AI Agents
In mcp_fuzzer/transport/catalog/catalog.py around lines 18 to 33,
TransportMetadata is a plain data-holder class; replace it with a dataclass: add
"from dataclasses import dataclass, field" and appropriate typing imports (e.g.
Optional, Callable, Type, Any), annotate all attributes with their types and
defaults (use Optional[...] or field(default=None) where needed), remove the
explicit __init__ and let @dataclass generate it, and keep the existing
attribute names and default values (including is_custom: bool = False) so
behavior stays the same.
| def list_builtin_transports(self) -> dict[str, dict[str, Any]]: | ||
| """List only built-in transports. | ||
| Returns: | ||
| Dictionary mapping transport names to their information | ||
| """ | ||
| return { | ||
| name: info | ||
| for name, metadata in self._transports.items() | ||
| if not metadata.is_custom | ||
| for info in [ | ||
| { | ||
| "class": metadata.transport_class, | ||
| "description": metadata.description, | ||
| "config_schema": metadata.config_schema, | ||
| "factory": metadata.factory_function, | ||
| "is_custom": metadata.is_custom, | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| def list_custom_drivers(self) -> dict[str, dict[str, Any]]: | ||
| """List only custom transports. | ||
| Returns: | ||
| Dictionary mapping transport names to their information | ||
| """ | ||
| return { | ||
| name: info | ||
| for name, metadata in self._transports.items() | ||
| if metadata.is_custom | ||
| for info in [ | ||
| { | ||
| "class": metadata.transport_class, | ||
| "description": metadata.description, | ||
| "config_schema": metadata.config_schema, | ||
| "factory": metadata.factory_function, | ||
| "is_custom": metadata.is_custom, | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the comprehension pattern.
The for info in [{...}] pattern is clever but unusual. Consider using a helper method or simpler construction for readability.
+ def _metadata_to_info(self, metadata: TransportMetadata) -> dict[str, Any]:
+ """Convert TransportMetadata to info dict."""
+ return {
+ "class": metadata.transport_class,
+ "description": metadata.description,
+ "config_schema": metadata.config_schema,
+ "factory": metadata.factory_function,
+ "is_custom": metadata.is_custom,
+ }
+
def list_builtin_transports(self) -> dict[str, dict[str, Any]]:
- return {
- name: info
- for name, metadata in self._transports.items()
- if not metadata.is_custom
- for info in [
- {
- "class": metadata.transport_class,
- "description": metadata.description,
- "config_schema": metadata.config_schema,
- "factory": metadata.factory_function,
- "is_custom": metadata.is_custom,
- }
- ]
- }
+ return {
+ name: self._metadata_to_info(metadata)
+ for name, metadata in self._transports.items()
+ if not metadata.is_custom
+ }📝 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.
| def list_builtin_transports(self) -> dict[str, dict[str, Any]]: | |
| """List only built-in transports. | |
| Returns: | |
| Dictionary mapping transport names to their information | |
| """ | |
| return { | |
| name: info | |
| for name, metadata in self._transports.items() | |
| if not metadata.is_custom | |
| for info in [ | |
| { | |
| "class": metadata.transport_class, | |
| "description": metadata.description, | |
| "config_schema": metadata.config_schema, | |
| "factory": metadata.factory_function, | |
| "is_custom": metadata.is_custom, | |
| } | |
| ] | |
| } | |
| def list_custom_drivers(self) -> dict[str, dict[str, Any]]: | |
| """List only custom transports. | |
| Returns: | |
| Dictionary mapping transport names to their information | |
| """ | |
| return { | |
| name: info | |
| for name, metadata in self._transports.items() | |
| if metadata.is_custom | |
| for info in [ | |
| { | |
| "class": metadata.transport_class, | |
| "description": metadata.description, | |
| "config_schema": metadata.config_schema, | |
| "factory": metadata.factory_function, | |
| "is_custom": metadata.is_custom, | |
| } | |
| ] | |
| } | |
| def _metadata_to_info(self, metadata: TransportMetadata) -> dict[str, Any]: | |
| """Convert TransportMetadata to info dict.""" | |
| return { | |
| "class": metadata.transport_class, | |
| "description": metadata.description, | |
| "config_schema": metadata.config_schema, | |
| "factory": metadata.factory_function, | |
| "is_custom": metadata.is_custom, | |
| } | |
| def list_builtin_transports(self) -> dict[str, dict[str, Any]]: | |
| """List only built-in transports. | |
| Returns: | |
| Dictionary mapping transport names to their information | |
| """ | |
| return { | |
| name: self._metadata_to_info(metadata) | |
| for name, metadata in self._transports.items() | |
| if not metadata.is_custom | |
| } | |
| def list_custom_drivers(self) -> dict[str, dict[str, Any]]: | |
| """List only custom transports. | |
| Returns: | |
| Dictionary mapping transport names to their information | |
| """ | |
| return { | |
| name: info | |
| for name, metadata in self._transports.items() | |
| if metadata.is_custom | |
| for info in [ | |
| { | |
| "class": metadata.transport_class, | |
| "description": metadata.description, | |
| "config_schema": metadata.config_schema, | |
| "factory": metadata.factory_function, | |
| "is_custom": metadata.is_custom, | |
| } | |
| ] | |
| } |
🤖 Prompt for AI Agents
mcp_fuzzer/transport/catalog/catalog.py around lines 211 to 251: both
list_builtin_transports and list_custom_drivers use an unusual "for info in
[{...}]" comprehension pattern; replace this with a clearer approach by adding a
small helper method (e.g. _transport_info(self, metadata) -> dict) that builds
and returns the info dict, then rewrite each method as a simple dict
comprehension like {name: self._transport_info(metadata) for name, metadata in
self._transports.items() if <condition>} to improve readability and remove the
nested single-item loop.
| # Use the base mixin's redirect resolution (imported from safety policy) | ||
| from ...safety_system.policy import resolve_redirect_safely | ||
|
|
||
| resolved = resolve_redirect_safely(self.url, location) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move import to module level.
Function-level imports defer import errors to runtime and add overhead on each call. Move this import to the top of the file with other imports.
Add to the imports section at the top:
from ...safety_system.policy import resolve_redirect_safelyThen simplify this method:
- # Use the base mixin's redirect resolution (imported from safety policy)
- from ...safety_system.policy import resolve_redirect_safely
-
- resolved = resolve_redirect_safely(self.url, location)
+ resolved = resolve_redirect_safely(self.url, location)🤖 Prompt for AI Agents
In mcp_fuzzer/transport/drivers/stream_http_driver.py around lines 155 to 158,
the review flags a function-level import of resolve_redirect_safely; move that
import to the module imports at the top of the file (add: from
...safety_system.policy import resolve_redirect_safely) and remove the inline
import inside the method so the method just calls resolved =
resolve_redirect_safely(self.url, location); this avoids runtime import overhead
and defers import errors to startup.
| async def _post_with_retries( | ||
| self, | ||
| client: httpx.AsyncClient, | ||
| url: str, | ||
| json: dict[str, Any], | ||
| headers: dict[str, str], | ||
| retries: int = 2, | ||
| ) -> httpx.Response: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parameter name json shadows the built-in module.
The parameter json: dict[str, Any] shadows the json module imported at line 11, which could cause confusion and bugs if the module is needed within this method. Consider renaming to payload or body.
async def _post_with_retries(
self,
client: httpx.AsyncClient,
url: str,
- json: dict[str, Any],
+ payload: dict[str, Any],
headers: dict[str, str],
retries: int = 2,
) -> httpx.Response:And update the usage inside the method accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/drivers/stream_http_driver.py around lines 228 to 235,
the parameter named `json` shadows the imported `json` module; rename the
parameter to `payload` (or `body`) and update every usage inside the
_post_with_retries method to use the new name so the module remains accessible
and no shadowing occurs.
| request_id = str(asyncio.get_running_loop().time()) | ||
| payload = { | ||
| "jsonrpc": "2.0", | ||
| "id": request_id, | ||
| "method": method, | ||
| "params": params or {}, | ||
| } | ||
| payload = self._create_jsonrpc_request(method, params, request_id) | ||
| return await self.send_raw(payload) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Request ID from loop time may collide under rapid requests.
Using asyncio.get_running_loop().time() as the request ID could produce duplicates if multiple requests are issued within the same microsecond. Consider using a counter or UUID for guaranteed uniqueness.
+import uuid
+
async def send_request(
self, method: str, params: dict[str, Any] | None = None
) -> Any:
- request_id = str(asyncio.get_running_loop().time())
+ request_id = str(uuid.uuid4())
payload = self._create_jsonrpc_request(method, params, request_id)
return await self.send_raw(payload)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/drivers/stream_http_driver.py around lines 303-305, the
current request_id uses asyncio.get_running_loop().time() which can collide
under rapid requests; replace it with a guaranteed-unique ID (e.g., generate a
UUID string or use an atomic/incrementing counter on the driver instance).
Import and use uuid.uuid4() to create request_id = str(uuid.uuid4()) or add a
thread-safe counter attribute (increment before use) and convert that to a
string, then pass that ID into _create_jsonrpc_request and send_raw.
| response.raise_for_status() | ||
| except httpx.HTTPStatusError as exc: | ||
| self._raise_http_status_error(exc, method=method) | ||
| self._handle_http_response_error(response) |
There was a problem hiding this comment.
Inconsistent error handling compared to send_raw.
send_raw (lines 366-373) catches DriverNetworkError and converts it to TransportError, but send_notification does not. This could leak internal error types to callers.
- self._handle_http_response_error(response)
+ try:
+ self._handle_http_response_error(response)
+ except DriverNetworkError as exc:
+ context = {"url": self.url, "status": response.status_code}
+ raise TransportError(str(exc), context=context) from exc📝 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.
| self._handle_http_response_error(response) | |
| try: | |
| self._handle_http_response_error(response) | |
| except DriverNetworkError as exc: | |
| context = {"url": self.url, "status": response.status_code} | |
| raise TransportError(str(exc), context=context) from exc |
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/drivers/stream_http_driver.py around line 425,
send_notification calls self._handle_http_response_error(response) but does not
catch DriverNetworkError and convert it to a TransportError like send_raw does
(lines ~366-373); update send_notification to wrap the network call in a
try/except that catches DriverNetworkError and raises a TransportError with a
clear message (including the original exception as the cause) so internal
DriverNetworkError is not leaked to callers and error handling matches send_raw.
| def __init__(self, transport: TransportDriver | None = None): | ||
| """Initialize the JSON-RPC helper. | ||
| async def disconnect(self) -> None: | ||
| """Disconnect from the transport. Default implementation does nothing.""" | ||
| pass | ||
| Args: | ||
| transport: Optional transport to use for requests. Can be set later. | ||
| """ | ||
| self._transport = transport | ||
| self._logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
JsonRpcAdapter structure and batch behavior look sound; minor logging/style tweak optional
The adapter cleanly encapsulates JSON‑RPC concerns (transport null‑checks, tools listing, tool invocation, and batch handling). send_batch_request correctly distinguishes notifications vs requests and normalizes responses for later collation.
Only nit-level suggestion: in the exception handler you mix f-strings with structured logging:
self._logger.warning(f"Failed to send batch request: {e}")Using parameterized logging (self._logger.warning("Failed to send batch request", exc_info=e)) would preserve stack traces and avoid eager string formatting, but this is not a functional issue.
Also applies to: 102-142
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/interfaces/rpc_adapter.py around lines 25-33 (and
similarly around lines 102-142), the exception handler uses an f-string for
logging which eagerly formats the exception and drops structured traceback;
change calls like self._logger.warning(f"Failed to send batch request: {e}") to
use the logging API with parameters and exception info (e.g.,
self._logger.warning("Failed to send batch request", exc_info=e) or
self._logger.warning("Failed to send batch request: %s", e, exc_info=e)) so the
stack trace is preserved and formatting is deferred.
Pull Request Summary: Redesign Transport
Overview
This PR represents a comprehensive architectural redesign of the transport layer, transforming it from a flat, protocol-oriented structure to a modular, layered architecture based on composition behaviors, factory patterns, and registry management. The changes touch 30+ files across core transport logic, clients, configuration, and tests.
Major Design Changes
1. Naming Nomenclature Shift
TransportProtocol→TransportDriverreflects a shift from abstract interfaces to concrete driver implementationscreate_transport()→build_driver()aligns with builder pattern semanticsTransportManager→ProcessSupervisorclarifies responsibility boundariesTransportProcessState→ProcessStatereduces namespace coupling2. Architectural Restructuring
Before: Flat transport module with mixed concerns
After: Modular, layered architecture
3. Design Patterns Introduced
a) Behavior Composition over Mixin Inheritance
Replaces multiple inheritance mixin pattern with explicit behavior composition:
DriverBaseBehavior(base behavior contract)HttpClientBehavior(HTTP network operations)ResponseParserBehavior(response parsing logic)LifecycleBehavior(connection lifecycle tracking)Benefit: More explicit, clearer intent, easier to understand responsibilities.
b) Factory Pattern with Registry
build_driver(url_or_protocol, endpoint=None, **kwargs)- Main factory entry pointDriverCatalog- Unified registry managing both built-in and custom driversCustomDriverCatalog- Wrapper providing custom-driver-specific APIc) Adapter Pattern
JsonRpcAdapter- WrapsTransportDriverto provide JSON-RPC method callsd) Strategy Pattern via EndpointResolver
EndpointResolverencapsulates different URL/protocol parsing strategies:e) Coordinator/Facade Pattern
TransportCoordinatorprovides centralized orchestration of:4. SOLID Principles Analysis
✅ Single Responsibility Principle (SRP)
LifecycleBehaviorhandles multiple state tracking responsibilities (connection_state, activity tracking, duration calculations) - slight violation✅ Open/Closed Principle (OCP)
DriverCatalogis open for extension viaregister()without modificationbuild_driver()in builder.py has hardcoded scheme mapping at import time - not fully open to custom schemes without code changesJsonRpcAdapter.send_request()has different semantics thanTransportDriver.send_request()- adapter returns RPC results while drivers return raw responsesLifecycleBehaviormethods (is_connected, is_closed) suggest state queries that may not apply to all driver implementations✅ Interface Segregation Principle (ISP)
build_driver()imports concrete drivers directly at module level (HttpDriver,SseDriver, etc.)EndpointResolvertakesDriverCatalogas dependency but defaults to None, requiring late binding viaset_registry()5. Code Smells and Issues
a) Feature Envy
StreamHttpDriveris large (~400+ lines in the summary) with many helper methods (_prepare_headers, _resolve_redirect, _extract_content_type, etc.) - suggests potential extraction into helpersb) Lazy Initialization Anti-Pattern
Violates fail-fast principle; should require registry in constructor or validate before use.
c) Circular Dependency Risk
catalog/builder.pyimports fromdrivers/andinterfaces/drivers/import frominterfaces/(acceptable)interfaces/rpc_adapter.pydepends on drivers via type hints (acceptable with TYPE_CHECKING)d) God Object Risk
DriverCataloghas many responsibilities:Could benefit from splitting query/list operations into separate repository class.
e) Mixed Abstraction Levels
build_driver()mixes high-level factory orchestration with low-level error message formattingf) Incomplete Error Handling
JsonRpcAdapterhas runtime checks for transport presence (if not self._transport) - should use dependency injection insteadTransportCoordinatorconstruction references undefinedconfigparameter impact on driver creationg) Magic Strings and Hardcoding
Scheme names are hardcoded; should be centralized as constants.
h) Missing Documentation
ParsedEndpoint.__post_init__()behavior (endpoint derivation) not clearly documentedLifecycleBehavior.register_activity_callback()subscription mechanism not fully explainedTransportCoordinator,ProcessSupervisor, andToolClientuncleari) Behavioral Coupling
j) Type Hint Issues
6. Architectural Strengths
7. Recommendations for Improvement
EndpointResolver.__init__or require registry in constructorStreamHttpDriverinto separate helper classesDriverCatalogby extracting query/list operations into a separateTransportRepositoryclasskwargsin driver factory methodsJsonRpcAdapter.transportto fail-fast on misconfigurationDriverStatelifecycleSummary
This is a well-structured redesign that significantly improves the codebase's organization and maintainability through modular architecture, explicit composition, and clear separation of concerns. While it generally follows SOLID principles well, there are opportunities to improve dependency management, reduce some god-object concerns, and clarify component interactions. The naming improvements and layered architecture are particularly beneficial for long-term code maintenance and extensibility.