redesign config subsystem #134
Conversation
WalkthroughThis PR refactors the config subsystem into a port-and-adapter design: constants moved into config.core, config manager created there, loader split into discovery/parser/loader with ConfigLoader and ConfigSearchParams, transport extension moved to an extensions package, and a ConfigPort/ConfigAdapter mediator (config_mediator) introduced; call sites updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (caller)
participant Mediator as config_mediator (ConfigPort)
participant Adapter as ConfigAdapter
participant Discoverer as File Discoverer
participant Parser as YAML Parser
participant Transport as Transport Loader
participant Global as Global Config (Configuration)
App->>Mediator: apply_file(config_path)
Mediator->>Adapter: apply_file(config_path)
Adapter->>Discoverer: find_config_file(config_path)
Discoverer-->>Adapter: file_path or None
alt file found
Adapter->>Parser: load_config_file(file_path)
Parser-->>Adapter: config_dict
Adapter->>Transport: load_custom_transports(config_dict)
Transport-->>Adapter: registrations (or error)
Adapter->>Global: update(config_dict)
Adapter-->>Mediator: True
Mediator-->>App: True
else not found / error
Adapter-->>Mediator: False
Mediator-->>App: False
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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 #134 +/- ##
==========================================
+ Coverage 69.94% 70.89% +0.94%
==========================================
Files 108 123 +15
Lines 6905 7094 +189
Branches 1043 1046 +3
==========================================
+ Hits 4830 5029 +199
+ Misses 1764 1758 -6
+ Partials 311 307 -4 ☔ 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: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
mcp_fuzzer/config/__init__.py(2 hunks)mcp_fuzzer/config/discovery.py(1 hunks)mcp_fuzzer/config/loader.py(1 hunks)mcp_fuzzer/config/parser.py(1 hunks)mcp_fuzzer/config/schema.py(1 hunks)mcp_fuzzer/config/transports.py(1 hunks)mcp_fuzzer/reports/reporter/dependencies.py(0 hunks)tests/unit/config/test_config_loader.py(1 hunks)tests/unit/config/test_discovery_transports.py(1 hunks)
💤 Files with no reviewable changes (1)
- mcp_fuzzer/reports/reporter/dependencies.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: examples/auth_config.json:3-6
Timestamp: 2025-08-08T11:47:21.915Z
Learning: In repo Agent-Hellboy/mcp-server-fuzzer, the file examples/auth_config.json uses "api_key": "secret123" intentionally for the dummy example server. Do not flag this as a leaked secret in future reviews; maintainers prefer keeping it unchanged. If needed, suggest adding a README note rather than changing the example value.
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.
🧬 Code graph analysis (6)
mcp_fuzzer/config/transports.py (3)
mcp_fuzzer/exceptions.py (2)
ConfigFileError(272-276)MCPError(19-44)mcp_fuzzer/transport/custom.py (1)
register_custom_transport(157-175)mcp_fuzzer/transport/base.py (1)
TransportProtocol(8-162)
mcp_fuzzer/config/parser.py (1)
mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)
tests/unit/config/test_discovery_transports.py (5)
mcp_fuzzer/config/discovery.py (1)
find_config_file(10-45)mcp_fuzzer/config/transports.py (1)
load_custom_transports(17-74)mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)mcp_fuzzer/transport/base.py (1)
TransportProtocol(8-162)mcp_fuzzer/transport/custom.py (1)
list_custom_transports(190-196)
mcp_fuzzer/config/loader.py (5)
mcp_fuzzer/config/discovery.py (1)
find_config_file(10-45)mcp_fuzzer/config/parser.py (1)
load_config_file(14-49)mcp_fuzzer/config/schema.py (1)
get_config_schema(9-195)mcp_fuzzer/config/transports.py (1)
load_custom_transports(17-74)mcp_fuzzer/exceptions.py (2)
ConfigFileError(272-276)MCPError(19-44)
tests/unit/config/test_config_loader.py (6)
mcp_fuzzer/config/loader.py (3)
ConfigLoader(26-77)load(39-59)apply(61-77)mcp_fuzzer/config/discovery.py (1)
find_config_file(10-45)mcp_fuzzer/config/schema.py (1)
get_config_schema(9-195)mcp_fuzzer/config/parser.py (1)
load_config_file(14-49)mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)mcp_fuzzer/config/manager.py (2)
update(47-49)get(39-41)
mcp_fuzzer/config/__init__.py (1)
mcp_fuzzer/config/loader.py (1)
ConfigLoader(26-77)
🪛 GitHub Actions: Lint
tests/unit/config/test_discovery_transports.py
[error] 50-50: E501 Line too long (93 > 88) in Ruff check. Line 50 exceeds maximum length.
[error] 57-57: E501 Line too long (90 > 88) in Ruff check. Line 57 exceeds maximum length.
🔇 Additional comments (4)
mcp_fuzzer/config/__init__.py (1)
35-42: ConfigLoader public export wiring looks correctRe-exporting
ConfigLoaderalongside the existing loader helpers keeps the public config API consistent with the new class-based design and matches how tests import it.Also applies to: 71-78
mcp_fuzzer/config/transports.py (1)
17-74: Robust custom transport loading and validationThe validation flow here (module import, class lookup,
issubclasscheck, optional factory resolution, and consistentConfigFileErrorwrapping) is solid and matches how the tests exercise success and failure paths.tests/unit/config/test_config_loader.py (1)
24-190: Comprehensive coverage of the new ConfigLoader flowThe fixture plus this suite of tests give good end‑to‑end coverage: discovery defaults, parser success/error cases,
apply_config_filebehavior, andConfigLoader.load/applywith injectable discoverer/parser/transport_loader. The mocking strategy (patchingmcp_fuzzer.config.loader.*) matches how the loader is wired and should keep these tests stable across internal refactors.mcp_fuzzer/config/loader.py (1)
2-13: Module-level docstring and imports look goodDocstring,
__future__usage, discovery/parser/transports imports, and the explicitget_config_schemare-export via# noqa: F401are coherent and match the new modular design; no issues from a correctness standpoint.
mcp_fuzzer/config/discovery.py
Outdated
| if config_path and os.path.isfile(config_path): | ||
| return config_path | ||
|
|
||
| if search_paths is None: | ||
| search_paths = [ | ||
| os.getcwd(), | ||
| str(Path.home() / ".config" / "mcp-fuzzer"), | ||
| ] | ||
|
|
||
| if file_names is None: | ||
| file_names = ["mcp-fuzzer.yml", "mcp-fuzzer.yaml"] | ||
|
|
||
| for path in search_paths: | ||
| if not os.path.isdir(path): | ||
| continue | ||
| for name in file_names: | ||
| file_path = os.path.join(path, name) | ||
| if os.path.isfile(file_path): | ||
| return file_path | ||
|
|
||
| return None |
There was a problem hiding this comment.
Avoid falling back to other locations when an explicit config_path is invalid
Right now, if config_path is provided but doesn’t point to an existing file, find_config_file silently ignores it and searches search_paths/file_names. That means a typo in --config can cause some unrelated config file (e.g. from $PWD or ~/.config/mcp-fuzzer) to be used instead, which is very hard to spot.
To make config_path truly authoritative, consider short‑circuiting on it and not falling back when it’s invalid, e.g.:
- if config_path and os.path.isfile(config_path):
- return config_path
+ if config_path is not None:
+ # Explicit path is authoritative: either use it or report "not found"
+ return config_path if os.path.isfile(config_path) else NoneIf you prefer to treat an invalid explicit path as an error, the caller (e.g. ConfigLoader) could detect config_path is not None and result is None and raise ConfigFileError instead of treating it as “no config anywhere”.
🤖 Prompt for AI Agents
In mcp_fuzzer/config/discovery.py around lines 25-45, the function currently
ignores an explicit config_path when it doesn't exist and falls back to
searching other locations; change this so an explicit config_path is
authoritative: if config_path is provided and os.path.isfile(config_path) return
it, otherwise immediately return None (do not continue searching), so callers
can detect a provided-but-missing path and handle/raise a ConfigFileError as
appropriate.
mcp_fuzzer/config/loader.py
Outdated
| from typing import Any | ||
|
|
||
| import yaml | ||
| from typing import Any, Callable, Tuple |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Modernize type usage and drop unused Tuple import (optional)
Since you already use PEP 604 unions (|) and have from __future__ import annotations, you can simplify by using built‑in tuple[...] and removing the Tuple import.
-from typing import Any, Callable, Tuple
+from typing import Any, Callable
@@
- ) -> Tuple[ConfigDict | None, str | None]:
+ ) -> tuple[ConfigDict | None, str | None]:Also applies to: 18-24, 44-44
🤖 Prompt for AI Agents
In mcp_fuzzer/config/loader.py around line 7 (and also apply to annotations at
lines 18-24 and line 44), the code imports Tuple from typing but the project
uses PEP 604 unions and future annotations; remove the unused Tuple import from
the typing import list and replace any usages of typing.Tuple[...] with the
builtin tuple[...] form (or equivalent concise annotation) so all type
annotations use modern syntax and no longer reference Tuple.
mcp_fuzzer/config/loader.py
Outdated
| def __init__( | ||
| self, | ||
| discoverer: FileDiscoverer | None = None, | ||
| parser: ConfigParser | None = None, | ||
| transport_loader: TransportLoader | None = None, | ||
| ): | ||
| self.discoverer = discoverer or find_config_file | ||
| self.parser = parser or load_config_file | ||
| self.transport_loader = transport_loader or load_custom_transports |
There was a problem hiding this comment.
Avoid or when defaulting injected callables
Using discoverer or find_config_file can accidentally discard a valid but falsey injected implementation (e.g., a callable object with __bool__ returning False). It’s safer and more explicit to check against None.
- self.discoverer = discoverer or find_config_file
- self.parser = parser or load_config_file
- self.transport_loader = transport_loader or load_custom_transports
+ self.discoverer = discoverer if discoverer is not None else find_config_file
+ self.parser = parser if parser is not None else load_config_file
+ self.transport_loader = (
+ transport_loader
+ if transport_loader is not None
+ else load_custom_transports
+ )🤖 Prompt for AI Agents
In mcp_fuzzer/config/loader.py around lines 29 to 37, the constructor currently
uses truthy checks like "discoverer or find_config_file" which can override
valid but falsey injected callables; change these to explicit None checks so
defaults are only applied when the argument is None (e.g., self.discoverer =
discoverer if discoverer is not None else find_config_file, and likewise for
parser and transport_loader), preserving any injected callable even if it
evaluates to False.
mcp_fuzzer/config/loader.py
Outdated
| def load( | ||
| self, | ||
| config_path: str | None = None, | ||
| search_paths: list[str] | None = None, | ||
| file_names: list[str] | None = None, | ||
| ) -> Tuple[ConfigDict | None, str | None]: | ||
| """Return the configuration dictionary and source file path.""" | ||
| file_path = self.discoverer(config_path, search_paths, file_names) | ||
| if not file_path: | ||
| logger.debug("No configuration file found") | ||
| return None, None | ||
|
|
||
| logger.info("Loading configuration from %s", file_path) | ||
| try: | ||
| config_data = self.parser(file_path) | ||
| self.transport_loader(config_data) | ||
| except (ConfigFileError, MCPError): | ||
| logger.exception("Failed to load configuration from %s", file_path) | ||
| raise | ||
|
|
||
| Args: | ||
| file_path: Path to the configuration file | ||
| return config_data, file_path |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging unexpected exceptions from injected components
load() currently logs and re‑raises only ConfigFileError and MCPError. With custom injected parser/transport_loader, unexpected exceptions (e.g., ValueError) will bubble up without logging at this layer. That may be fine if callers/logging are centralized elsewhere, but adding a generic catch‑and‑rethrow with logging can improve diagnostics without changing behavior.
- try:
- config_data = self.parser(file_path)
- self.transport_loader(config_data)
- except (ConfigFileError, MCPError):
- logger.exception("Failed to load configuration from %s", file_path)
- raise
+ try:
+ config_data = self.parser(file_path)
+ self.transport_loader(config_data)
+ except (ConfigFileError, MCPError):
+ logger.exception("Failed to load configuration from %s", file_path)
+ raise
+ except Exception:
+ logger.exception(
+ "Unexpected error while loading configuration from %s", file_path
+ )
+ raise🤖 Prompt for AI Agents
In mcp_fuzzer/config/loader.py around lines 39 to 59, the load() method only
logs and re-raises ConfigFileError and MCPError, so unexpected exceptions from
injected parser/transport_loader will go unlogged here; wrap the try block with
an additional broad except Exception as e that logs the unexpected exception
with logger.exception("Unexpected error loading configuration from %s: %s",
file_path, e) (or logger.exception with contextual message) and then re-raise to
preserve original behavior and traceback.
mcp_fuzzer/config/loader.py
Outdated
| def apply( | ||
| self, | ||
| config_path: str | None = None, | ||
| search_paths: list[str] | None = None, | ||
| file_names: list[str] | None = None, | ||
| ) -> bool: | ||
| """Load configuration and merge it into the runtime state.""" | ||
| try: | ||
| config_data, file_path = self.load(config_path, search_paths, file_names) | ||
| except (ConfigFileError, MCPError): | ||
| return False | ||
|
|
||
| Raises: | ||
| ConfigFileError: If the file cannot be found, parsed, or has permission issues | ||
| """ | ||
| if not os.path.isfile(file_path): | ||
| raise ConfigFileError(f"Configuration file not found: {file_path}") | ||
| if not file_path: | ||
| return False | ||
|
|
||
| # Verify file extension | ||
| if not file_path.endswith((".yml", ".yaml")): | ||
| raise ConfigFileError( | ||
| f"Unsupported configuration file format: {file_path}. " | ||
| "Only YAML files with .yml or .yaml extensions are supported." | ||
| ) | ||
| config.update(config_data or {}) | ||
| return True |
There was a problem hiding this comment.
🧩 Analysis chain
Verify behavior contract for apply() / apply_config_file (exceptions vs bool)
apply() (and thus apply_config_file) now swallows ConfigFileError/MCPError and returns False on failure or missing config, while load() still propagates those errors. This is a reasonable API split, but if previous apply_config_file behavior was to raise, some callers/tests or CLI code might rely on exceptions rather than a boolean status.
You can quickly check current call sites to ensure they handle the boolean contract correctly:
Also applies to: 80-92
🏁 Script executed:
#!/bin/bash
# Inspect apply_config_file and ConfigLoader.apply usage and expectations
rg -n "apply_config_file" -C3
rg -n "ConfigLoader\\(" -C3Length of output: 8417
I found a critical issue that the review comment correctly anticipated.Now let me check if the CLI code needs to be updated based on the behavior change:
Update mcp_fuzzer/cli/args.py lines 481-483 to handle the new boolean return from apply_config_file()
The change to apply_config_file() (returning False on error instead of raising exceptions) is a breaking change for the CLI code. At cli/args.py:481-483, the try/except block expecting exceptions will never trigger since apply_config_file() now swallows errors and returns a boolean. Configuration loading errors will be silently ignored without logging.
Update the code to:
if not apply_config_file():
logging.debug("Error loading default configuration file")The try/except is no longer needed since exceptions are already handled internally by apply_config_file().
🤖 Prompt for AI Agents
In mcp_fuzzer/cli/args.py around lines 481 to 483, the caller still expects
apply_config_file() to raise exceptions, but apply_config_file() now returns a
boolean and swallows errors; update this call to remove the try/except and
instead check the boolean return value, e.g. call apply_config_file() and if it
returns False log a debug/error message indicating the default configuration
file failed to load so configuration loading failures are not silently ignored.
mcp_fuzzer/config/parser.py
Outdated
| def load_config_file(file_path: str) -> dict[str, Any]: | ||
| """Load configuration from a YAML file. | ||
| Args: | ||
| file_path: Path to the configuration file | ||
| Returns: | ||
| Dictionary containing the configuration | ||
| Raises: | ||
| ConfigFileError: If the file cannot be found, parsed, or has permission issues | ||
| """ | ||
| if not os.path.isfile(file_path): | ||
| raise ConfigFileError(f"Configuration file not found: {file_path}") | ||
|
|
||
| if not file_path.endswith((".yml", ".yaml")): | ||
| raise ConfigFileError( | ||
| f"Unsupported configuration file format: {file_path}. " | ||
| "Only YAML files with .yml or .yaml extensions are supported." | ||
| ) | ||
|
|
||
| try: | ||
| with open(file_path, "r", encoding="utf-8") as f: | ||
| return yaml.safe_load(f) or {} | ||
| except yaml.YAMLError as e: | ||
| raise ConfigFileError( | ||
| f"Error parsing YAML configuration file {file_path}: {str(e)}" | ||
| ) | ||
| except PermissionError: | ||
| raise ConfigFileError( | ||
| f"Permission denied when reading configuration file: {file_path}" | ||
| ) | ||
| except Exception as e: | ||
| raise ConfigFileError( | ||
| f"Unexpected error reading configuration file {file_path}: {str(e)}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Enforce mapping-shaped configs to match the type annotation and downstream usage
yaml.safe_load can return non-dict top-level values (lists, scalars, etc.), but the function is annotated as returning dict[str, Any] and downstream code treats the result as a mapping (e.g. config.update(config_data or {}), config_data.get("custom_transports", {})).
To avoid obscure AttributeError/TypeError when someone supplies a structurally wrong YAML, consider normalizing and validating the type:
- try:
- with open(file_path, "r", encoding="utf-8") as f:
- return yaml.safe_load(f) or {}
+ try:
+ with open(file_path, "r", encoding="utf-8") as f:
+ data = yaml.safe_load(f) or {}
+ if not isinstance(data, dict):
+ raise ConfigFileError(
+ f"Top-level configuration in {file_path} must be a mapping/object"
+ )
+ return dataYou may also want to chain the underlying exceptions when re-raising (e.g. raise ConfigFileError(... ) from e) so debugging retains the original traceback.
🤖 Prompt for AI Agents
In mcp_fuzzer/config/parser.py around lines 14 to 48, yaml.safe_load may return
non-mapping types (list/scalar) but the function is annotated to return
dict[str, Any] and callers expect a mapping; update the code to validate and
normalize the loaded result to a dict (e.g., if result is None -> {}, if not
isinstance(result, dict) -> raise ConfigFileError explaining expected mapping),
and when re-raising catch exceptions include the original exception as context
using "raise ConfigFileError(... ) from e" so traceback is preserved.
mcp_fuzzer/config/schema.py
Outdated
| "safety_enabled": { | ||
| "type": "boolean", | ||
| "description": "Whether safety features are enabled", | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify overlapping safety flags and consider tightening object schemas
- The schema exposes both top-level
"safety_enabled"and nested"safety": {"enabled": ...}. If these represent the same concern, it would be good to either document the precedence or consolidate to a single canonical field to avoid configuration drift and confusion. - If configs are meant to be validated strictly, consider adding
"additionalProperties": false(and possiblyrequired) on selected objects such as"safety"and"output"so that obvious typos don’t silently pass validation.
Also applies to: 128-141, 142-193
Agent-Hellboy
force-pushed
the
redesign_config
branch
from
24b46aa to
2380c37
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_standardized_output.py (1)
124-136: Usingconfig_mediator.updatefits the new config flow; consider test isolationDriving the reporter configuration via
config_mediator.update({...})is consistent with the new mediator design and validates the intended behavior. The only caveat is that this mutates global config state; if other tests rely on default settings, consider resetting the mediator (or using a fixture to snapshot/restore config) around this test to avoid hidden cross‑test coupling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (45)
.gitignore(1 hunks)mcp_fuzzer/cli/config_merge.py(4 hunks)mcp_fuzzer/cli/startup_info.py(1 hunks)mcp_fuzzer/cli/validators.py(2 hunks)mcp_fuzzer/client/__init__.py(1 hunks)mcp_fuzzer/client/adapters/__init__.py(1 hunks)mcp_fuzzer/client/adapters/config_adapter.py(1 hunks)mcp_fuzzer/client/constants.py(1 hunks)mcp_fuzzer/client/ports/__init__.py(1 hunks)mcp_fuzzer/client/ports/config_port.py(1 hunks)mcp_fuzzer/client/tool_client.py(1 hunks)mcp_fuzzer/config/__init__.py(2 hunks)mcp_fuzzer/config/core/__init__.py(1 hunks)mcp_fuzzer/config/core/constants.py(1 hunks)mcp_fuzzer/config/core/manager.py(1 hunks)mcp_fuzzer/config/extensions/__init__.py(1 hunks)mcp_fuzzer/config/extensions/transports.py(1 hunks)mcp_fuzzer/config/loader.py(0 hunks)mcp_fuzzer/config/loading/__init__.py(1 hunks)mcp_fuzzer/config/loading/discovery.py(1 hunks)mcp_fuzzer/config/loading/loader.py(1 hunks)mcp_fuzzer/config/loading/parser.py(1 hunks)mcp_fuzzer/config/loading/search_params.py(1 hunks)mcp_fuzzer/config/manager.py(0 hunks)mcp_fuzzer/config/schema/__init__.py(1 hunks)mcp_fuzzer/config/schema/builders.py(1 hunks)mcp_fuzzer/config/schema/composer.py(1 hunks)mcp_fuzzer/fuzz_engine/runtime/watchdog.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/realistic/protocol_type_strategy.py(1 hunks)mcp_fuzzer/reports/reporter/__init__.py(1 hunks)mcp_fuzzer/safety_system/policy.py(1 hunks)mcp_fuzzer/transport/http.py(1 hunks)mcp_fuzzer/transport/stdio.py(1 hunks)mcp_fuzzer/transport/streamable_http.py(1 hunks)tests/integration/test_custom_transport.py(2 hunks)tests/integration/test_standardized_output.py(1 hunks)tests/unit/cli/test_cli.py(4 hunks)tests/unit/client/test_config_adapter.py(1 hunks)tests/unit/config/test_config_loader.py(1 hunks)tests/unit/config/test_constants.py(1 hunks)tests/unit/config/test_discovery_transports.py(1 hunks)tests/unit/config/test_manager.py(1 hunks)tests/unit/config/test_parser.py(1 hunks)tests/unit/config/test_schema_builders.py(1 hunks)tests/unit/config/test_search_params.py(1 hunks)
💤 Files with no reviewable changes (2)
- mcp_fuzzer/config/loader.py
- mcp_fuzzer/config/manager.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: examples/auth_config.json:3-6
Timestamp: 2025-08-08T11:47:21.915Z
Learning: In repo Agent-Hellboy/mcp-server-fuzzer, the file examples/auth_config.json uses "api_key": "secret123" intentionally for the dummy example server. Do not flag this as a leaked secret in future reviews; maintainers prefer keeping it unchanged. If needed, suggest adding a README note rather than changing the example value.
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.
🧬 Code graph analysis (24)
mcp_fuzzer/config/schema/__init__.py (1)
mcp_fuzzer/config/schema/composer.py (1)
get_config_schema(20-42)
mcp_fuzzer/config/core/__init__.py (1)
mcp_fuzzer/config/core/manager.py (1)
Configuration(43-80)
mcp_fuzzer/config/loading/__init__.py (4)
mcp_fuzzer/config/loading/discovery.py (2)
find_config_file(12-32)find_config_file_from_params(35-44)mcp_fuzzer/config/loading/loader.py (2)
ConfigLoader(27-135)apply_config_file(138-158)mcp_fuzzer/config/loading/parser.py (1)
load_config_file(14-49)mcp_fuzzer/config/loading/search_params.py (1)
ConfigSearchParams(10-24)
mcp_fuzzer/config/extensions/transports.py (3)
mcp_fuzzer/exceptions.py (2)
ConfigFileError(272-276)MCPError(19-44)mcp_fuzzer/transport/custom.py (1)
register_custom_transport(157-175)mcp_fuzzer/transport/base.py (1)
TransportProtocol(8-162)
mcp_fuzzer/client/adapters/__init__.py (1)
mcp_fuzzer/client/adapters/config_adapter.py (1)
ConfigAdapter(21-108)
mcp_fuzzer/config/extensions/__init__.py (1)
mcp_fuzzer/config/extensions/transports.py (1)
load_custom_transports(17-79)
tests/unit/config/test_discovery_transports.py (6)
mcp_fuzzer/config/loading/discovery.py (2)
find_config_file(12-32)find_config_file_from_params(35-44)mcp_fuzzer/config/loading/search_params.py (1)
ConfigSearchParams(10-24)mcp_fuzzer/config/extensions/transports.py (1)
load_custom_transports(17-79)mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)mcp_fuzzer/transport/base.py (1)
TransportProtocol(8-162)mcp_fuzzer/transport/custom.py (1)
list_custom_transports(190-196)
tests/unit/config/test_parser.py (2)
mcp_fuzzer/config/loading/parser.py (1)
load_config_file(14-49)mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)
mcp_fuzzer/config/schema/composer.py (1)
mcp_fuzzer/config/schema/builders.py (8)
build_auth_schema(88-115)build_basic_schema(32-47)build_custom_transports_schema(118-156)build_fuzzing_schema(50-73)build_network_schema(76-85)build_output_schema(179-234)build_safety_schema(159-176)build_timeout_schema(9-29)
tests/unit/config/test_search_params.py (1)
mcp_fuzzer/config/loading/search_params.py (1)
ConfigSearchParams(10-24)
tests/unit/client/test_config_adapter.py (4)
mcp_fuzzer/client/adapters/config_adapter.py (6)
ConfigAdapter(21-108)get(37-47)update(58-64)load_file(66-78)apply_file(80-100)get_schema(102-108)mcp_fuzzer/client/ports/config_port.py (6)
ConfigPort(15-95)get(23-33)update(46-52)load_file(55-67)apply_file(70-86)get_schema(89-95)mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)mcp_fuzzer/config/core/manager.py (2)
get(70-72)update(78-80)
tests/unit/config/test_manager.py (1)
mcp_fuzzer/config/core/manager.py (5)
Configuration(43-80)_get_bool_from_env(27-40)_get_float_from_env(8-24)get(70-72)update(78-80)
mcp_fuzzer/config/loading/parser.py (1)
mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)
tests/integration/test_standardized_output.py (3)
mcp_fuzzer/client/adapters/config_adapter.py (1)
update(58-64)mcp_fuzzer/client/ports/config_port.py (1)
update(46-52)mcp_fuzzer/config/core/manager.py (1)
update(78-80)
mcp_fuzzer/config/core/manager.py (2)
mcp_fuzzer/client/adapters/config_adapter.py (2)
get(37-47)update(58-64)mcp_fuzzer/client/ports/config_port.py (2)
get(23-33)update(46-52)
mcp_fuzzer/config/loading/discovery.py (1)
mcp_fuzzer/config/loading/search_params.py (1)
ConfigSearchParams(10-24)
mcp_fuzzer/client/ports/config_port.py (2)
mcp_fuzzer/client/adapters/config_adapter.py (5)
get(37-47)update(58-64)load_file(66-78)apply_file(80-100)get_schema(102-108)mcp_fuzzer/config/core/manager.py (2)
get(70-72)update(78-80)
mcp_fuzzer/client/ports/__init__.py (1)
mcp_fuzzer/client/ports/config_port.py (1)
ConfigPort(15-95)
mcp_fuzzer/cli/startup_info.py (2)
mcp_fuzzer/client/adapters/config_adapter.py (1)
load_file(66-78)mcp_fuzzer/client/ports/config_port.py (1)
load_file(55-67)
mcp_fuzzer/client/__init__.py (2)
mcp_fuzzer/client/adapters/config_adapter.py (1)
ConfigAdapter(21-108)mcp_fuzzer/client/ports/config_port.py (1)
ConfigPort(15-95)
mcp_fuzzer/cli/validators.py (2)
mcp_fuzzer/client/adapters/config_adapter.py (1)
load_file(66-78)mcp_fuzzer/client/ports/config_port.py (1)
load_file(55-67)
mcp_fuzzer/config/loading/loader.py (6)
mcp_fuzzer/config/core/manager.py (2)
Configuration(43-80)update(78-80)mcp_fuzzer/config/extensions/transports.py (1)
load_custom_transports(17-79)mcp_fuzzer/config/loading/discovery.py (1)
find_config_file(12-32)mcp_fuzzer/config/loading/parser.py (1)
load_config_file(14-49)mcp_fuzzer/config/loading/search_params.py (1)
ConfigSearchParams(10-24)mcp_fuzzer/exceptions.py (2)
ConfigFileError(272-276)MCPError(19-44)
tests/integration/test_custom_transport.py (3)
mcp_fuzzer/config/extensions/transports.py (1)
load_custom_transports(17-79)mcp_fuzzer/client/adapters/config_adapter.py (1)
apply_file(80-100)mcp_fuzzer/client/ports/config_port.py (1)
apply_file(70-86)
tests/unit/config/test_schema_builders.py (2)
mcp_fuzzer/config/schema/builders.py (8)
build_auth_schema(88-115)build_basic_schema(32-47)build_custom_transports_schema(118-156)build_fuzzing_schema(50-73)build_network_schema(76-85)build_output_schema(179-234)build_safety_schema(159-176)build_timeout_schema(9-29)mcp_fuzzer/config/schema/composer.py (1)
get_config_schema(20-42)
🔇 Additional comments (71)
.gitignore (1)
21-21: Consistent with existing documentation patterns.The addition of
*USAGE.mdaligns well with the existing wildcard-based patterns (*REDESIGN*.md,*FLOW.md) for ignoring draft/generated documentation files.mcp_fuzzer/config/schema/__init__.py (1)
1-6: Public schema facade looks goodRe-exporting
get_config_schemavia__all__is clear and keeps the schema surface small; no issues spotted.mcp_fuzzer/config/extensions/__init__.py (1)
1-6: Clean extensions entrypointThe package-level re-export of
load_custom_transportsis straightforward and matches the new config-extensions layering.mcp_fuzzer/config/core/manager.py (1)
8-83: Configuration manager and env parsing look correctEnv helpers handle invalid values by falling back to defaults, and
Configurationprovides a simple, predictable dict-backed store; importing a single globalconfiginstance is consistent with the rest of the refactor.mcp_fuzzer/transport/stdio.py (1)
23-24: Constants import realigned with new config.core namespaceSwitching
PROCESS_WAIT_TIMEOUTtoconfig.core.constants(and documenting that only values, not behavior, are imported) matches the new config layout without changing semantics.mcp_fuzzer/fuzz_engine/runtime/watchdog.py (1)
22-27: Watchdog now sources timeouts from config.core.constantsRouting the watchdog timeout constants through
config.core.constants(with a clear comment separating values from behavior) keeps the refactor coherent and does not affect the termination logic.mcp_fuzzer/cli/startup_info.py (1)
24-35: Usingconfig_mediator.load_filefor CLI preview is aligned with the new config flowLoading the main config via
config_mediator.load_file(args.config)ensures the startup preview goes through the same mediator/loader pipeline as the rest of the application, avoiding duplicate parsing logic and keeping behavior consistent.tests/unit/config/test_search_params.py (1)
11-63: TheConfigSearchParamsclass is already correctly decorated with@dataclass—no action needed.The implementation at
mcp_fuzzer/config/loading/search_params.pyalready includes the@dataclassdecorator (line 9) and has the expected field definitions. The tests will pass as written because the dataclass provides both field-wise equality and a repr that includes field values. The concern raised in the review comment is unfounded.mcp_fuzzer/transport/http.py (1)
13-17: LGTM!The import path update to
..config.core.constantsaligns well with the centralized constants architecture. The explanatory comment clarifies the design intent—constants are values, not behavior—which aids future maintainability.mcp_fuzzer/config/core/__init__.py (1)
1-67: LGTM!The package initializer properly aggregates and re-exports constants and manager components, establishing a clean public API surface. The
__all__definition is consistent with the imports, and the organization clearly separates constants from manager exports.mcp_fuzzer/client/ports/__init__.py (1)
1-11: LGTM!Clean package initializer establishing the ports layer for the Port and Adapter pattern. The docstring clearly communicates the architectural intent—modules should depend on the
ConfigPortinterface, not concrete implementations—promoting loose coupling and testability.mcp_fuzzer/safety_system/policy.py (1)
15-22: LGTM!The import path update is consistent with the centralized constants architecture. The two-line comment provides helpful architectural guidance, distinguishing value-based constants (imported directly) from behavioral components (accessed via mediator).
mcp_fuzzer/fuzz_engine/strategy/realistic/protocol_type_strategy.py (1)
13-14: LGTM!The import path update correctly navigates to the centralized constants module. The relative import depth (
....config.core.constants) is appropriate for this file's location in thefuzz_engine/strategy/realistic/subdirectory.mcp_fuzzer/cli/validators.py (1)
14-15: Config validation correctly routed throughconfig_mediatorSwitching the import to
config_mediatorand usingconfig_mediator.load_file(path)keeps the CLI’svalidate_config_filealigned with the new config port/adapter layer and preserves the “fail on exception, succeed silently” behavior. No issues from a correctness or testability standpoint.Also applies to: 66-69
mcp_fuzzer/transport/streamable_http.py (1)
7-16: Direct import of core config constants is appropriateImporting HTTP/MCP constants from
config.core.constants(with local aliases for back‑compat) keeps this transport decoupled from higher‑level config surfaces and doesn’t alter behavior. Looks good.mcp_fuzzer/reports/reporter/__init__.py (1)
72-78: Reporter now correctly defaults toconfig_mediatorUsing
config_mediatoras the defaultconfig_provideraligns the reporter with the rest of the config subsystem and matches the “dict‑like provider” expectation. The lazy import inside__init__is a good choice to avoid unnecessary initialization and potential cycles.tests/unit/cli/test_cli.py (1)
268-275: Tests correctly patched to the mediator-based config APIUpdating the patch targets to
...validators.config_mediator.load_fileand...config_merge.config_mediator.{load_file,apply_file}keeps these tests exercising the same behavior through the new mediator surface. As long asconfig_mergeimportsconfig_mediatorat module scope (which appears consistent with the rest of the PR), these patches will remain stable.Also applies to: 545-577
mcp_fuzzer/config/core/constants.py (1)
24-28: Clarified semantics ofSAFETY_NO_NETWORK_DEFAULTThe updated comment now accurately reflects that
Falsemeans “network allowed by default” andTrueflips to “deny by default,” which matches the variable name and value. No behavioral change; comment clarity improvement only.mcp_fuzzer/client/tool_client.py (1)
15-21: Tool client now sources timeouts from core constantsImporting
DEFAULT_TOOL_RUNS,DEFAULT_MAX_TOOL_TIME,DEFAULT_MAX_TOTAL_FUZZING_TIME, andDEFAULT_FORCE_KILL_TIMEOUTfromconfig.core.constantscentralizes their definition without altering behavior of the timeouts or fuzzing loops. Change looks solid.tests/unit/config/test_manager.py (3)
19-35: Good coverage of float env helper behaviorThese tests exercise present, missing, and invalid values for
_get_float_from_env, matching the implementation’s defaulting behavior. No issues spotted.
37-57: Boolean env parsing is thoroughly exercisedTrue/false and default behaviors for
_get_bool_from_envare covered with realistic value variations and both default branches. Implementation and tests are consistent.
60-118: Configuration and global instance tests look solidYou verify env-driven loading, default fallbacks, get/set/update, and that the shared
configinstance is a usableConfiguration. This gives good safety net around the core manager.mcp_fuzzer/config/loading/search_params.py (1)
9-24: ConfigSearchParams dataclass is well-shapedThe dataclass cleanly groups config search parameters, uses
Noneinstead of mutable defaults for lists, and matches the documented attributes and discovery API usage.tests/unit/client/test_config_adapter.py (2)
17-37: Core ConfigAdapter interface behavior well coveredThese tests confirm
ConfigAdapterimplementsConfigPortand correctly proxiesget,set, andupdate, including default handling onget. This anchors the adapter’s basic contract.
39-74: File loading and applying paths are exercised correctlyThe positive/negative tests around
load_fileandapply_filevalidate success, not-found mapping toConfigFileError/False, and that applied values are observable viaget. This aligns with the parser and loader behavior.tests/integration/test_custom_transport.py (1)
8-9: Integration test now correctly routes config loading through mediatorSwitching to
config_mediator.apply_file(config_path=...)keeps the test semantics while aligning with the new port/adapter config architecture. The assertion on the return value preserves the previous guarantee that the config file was actually applied.Also applies to: 100-101
mcp_fuzzer/client/adapters/__init__.py (1)
1-10: Adapters package public API is clean and focusedRe-exporting
ConfigAdapterandconfig_mediatorvia__all__gives a stable, discoverable surface (mcp_fuzzer.client.adapters) that matches how tests and CLI now import the mediator.mcp_fuzzer/cli/config_merge.py (2)
10-63: Config-to-CLI transfer now cleanly depends on mediatorRouting
_transfer_config_to_argsthroughconfig_mediator.getinstead of a concrete global config keeps the mapping logic intact while aligning the CLI with the new port/adapter abstraction. The default-handling logic remains unchanged.
72-139: build_cli_config correctly delegates all config file handling to mediatorUsing
config_mediator.load_file/updatewhenargs.configis set andconfig_mediator.apply_file()for defaults centralizes config file concerns behind the adapter while preserving exception behavior. The finalconfig_mediator.update(merged)keeps the global config in sync with the merged view used forCliConfig.tests/unit/config/test_discovery_transports.py (3)
22-48: DummyTransport and registry fixture provide solid isolation
DummyTransportcorrectly implementsTransportProtocol’s abstract methods, and the autouseclear_registryfixture ensures the custom transport registry is clean before and after each test, avoiding cross‑test interference.
50-73: Config discovery tests align with new discovery/search_params APIThe tests for
find_config_fileandfind_config_file_from_paramscover explicitconfig_pathprecedence, search viasearch_paths/file_names, and the missing‑fileNonecase. Usage ofConfigSearchParamsmatches the new discovery helpers.Also applies to: 116-140
75-155: Custom transport loading and logging behavior are well validatedYou exercise successful registration, missing-module and invalid-class errors (including the non‑class TypeError branch), and confirm that logging uses
%-style formatting with separate args rather than f-strings. This gives strong coverage ofload_custom_transports.Also applies to: 157-184
tests/unit/config/test_parser.py (3)
16-38: Happy-path and basic failure modes of load_config_file are coveredThe success test plus not-found and invalid-extension cases track the parser’s file existence and extension checks, and confirm type/values from YAML parsing.
40-57: YAML error and permission handling are accurately exercisedInvalid YAML and permission-denied scenarios assert the correct
ConfigFileErrormessages, and the exception-formatting test ensures the message cleanly includes the original YAML error and file path without redundantstr()noise.Also applies to: 59-74
76-92: Empty and comments-only YAML handling matches parser contractVerifying that empty and comment-only files yield
{}matches theyaml.safe_load(...) or {}behavior, guarding callers against unexpectedNone.tests/unit/config/test_constants.py (1)
1-115: LGTM!The test module thoroughly validates the exported configuration constants, covering both value assertions and type checks. The tests align well with the new public API surface introduced by the config subsystem redesign.
mcp_fuzzer/config/loading/__init__.py (1)
1-17: LGTM!The package initializer cleanly re-exports the public API for configuration loading. The
__all__definition aligns with the imports, and the# noqa: F401comment correctly suppresses the unused import warning for the re-exportedload_config_file.mcp_fuzzer/config/loading/parser.py (1)
14-49: LGTM!The parser function handles error cases comprehensively with clear error messages. Using
yaml.safe_loadis the correct choice for security. The validation order (existence → extension → parse) provides clear error messages for each failure mode.mcp_fuzzer/client/adapters/config_adapter.py (1)
111-112: LGTM!The global
config_mediatorinstance correctly provides a single point of access for configuration operations, implementing the mediator pattern as intended.tests/unit/config/test_schema_builders.py (1)
111-148: LGTM!The composition and independence tests are well-structured and correctly named. They effectively verify that
get_config_schemaincludes all sections and that individual builders don't have overlapping keys.mcp_fuzzer/config/loading/discovery.py (3)
12-32: LGTM!Clean public API that provides a convenient interface with individual parameters while delegating to the shared implementation.
35-44: LGTM!Good complement to
find_config_filefor callers who already have aConfigSearchParamsinstance.
47-78: LGTM!The implementation correctly handles the priority order (explicit path → search paths) and gracefully skips non-existent directories. The default search locations are sensible.
mcp_fuzzer/client/ports/config_port.py (1)
15-95: LGTM!Well-designed port interface that properly abstracts configuration access. The method signatures align with the
ConfigAdapterimplementation, and the docstrings clearly document the contract.tests/unit/config/test_config_loader.py (8)
26-50: LGTM!Good reusable fixture with representative configuration content.
53-77: LGTM!Good coverage of the
find_config_filefunction including explicit path, search paths, default behavior, and missing file scenarios.
80-103: LGTM!Thorough testing of the YAML loader including success path and error conditions (invalid extension, not found, malformed YAML).
106-125: LGTM!Proper mocking and verification of the global state update flow.
128-133: LGTM!Good sanity check validating key schema properties are present.
136-195: LGTM!Comprehensive tests for
ConfigLoadercovering dependency injection, success paths, and error handling for both parser and transport loader failures.
198-230: LGTM!Good verification that configuration loading uses appropriate log levels. The logging tests ensure DEBUG is used rather than INFO, avoiding log noise.
233-259: LGTM!Good coverage of the
ConfigSearchParams-based APIs.mcp_fuzzer/config/loading/loader.py (6)
27-44: LGTM!Excellent dependency injection design. The optional parameters with sensible defaults enable both convenient usage and easy testing with mocks.
46-75: LGTM!Clear loading pipeline with proper exception handling and DEBUG-level logging. The re-raise behavior correctly propagates errors to callers.
77-92: LGTM!Clean delegation from the parameter object to the individual parameter API.
94-120: LGTM!Good error handling that converts exceptions to boolean return values. The defensive
config_data or {}on line 119 provides a safe fallback.
122-135: LGTM!Consistent delegation pattern matching
load_from_params.
138-158: LGTM!Useful convenience function that provides a simple API for common use cases while leveraging the full
ConfigLoaderinternally.mcp_fuzzer/config/schema/builders.py (8)
9-29: LGTM!Clear schema for timeout-related configuration with appropriate types.
32-47: LGTM!Appropriate schema with enum constraint for log levels.
50-73: LGTM!Comprehensive fuzzing configuration schema with appropriate enums and types.
76-85: LGTM!Simple and correct schema for network configuration.
88-115: LGTM!Well-structured authentication schema with proper nesting and required fields.
118-156: LGTM!Excellent use of
patternPropertiesfor dynamic transport registration. The regex pattern correctly restricts to valid Python identifiers, andadditionalProperties: falsehelps catch configuration typos.
159-176: LGTM!Comprehensive safety configuration schema covering all security-related options.
179-234: LGTM!Well-structured output configuration schema with appropriate nested objects and enums. The human-readable
max_sizeformat (e.g., "1GB") is a good UX choice.mcp_fuzzer/config/__init__.py (2)
4-49: Well-organized import structure with clear section separation.The imports are logically grouped by their source modules (core, loading, schema, extensions) with descriptive comments. This improves maintainability.
51-91: LGTM!The
__all__definition is comprehensive and well-organized with section comments. All imported names are properly exported.mcp_fuzzer/client/__init__.py (2)
3-34: Clean implementation of the ports-and-adapters pattern.Exposing both
ConfigPort(the interface) andConfigAdapter(the implementation) along with the singletonconfig_mediatorprovides flexibility for dependency injection while maintaining a convenient default.
38-72: LGTM!The
__all__correctly exports the client classes, port/adapter components, and all re-exported constants. The aliasUnifiedMCPFuzzerClientis preserved for backward compatibility.mcp_fuzzer/client/constants.py (1)
106-134: LGTM!The
__all__list is consistent with the module-level constant assignments.
| def apply_file( | ||
| self, | ||
| config_path: str | None = None, | ||
| search_paths: list[str] | None = None, | ||
| file_names: list[str] | None = None, | ||
| ) -> bool: | ||
| """Load and apply configuration from a file. | ||
| Args: | ||
| config_path: Explicit path to config file | ||
| search_paths: List of directories to search | ||
| file_names: List of file names to search for | ||
| Returns: | ||
| True if configuration was loaded and applied, False otherwise | ||
| """ | ||
| return apply_config_file( | ||
| config_path=config_path, | ||
| search_paths=search_paths, | ||
| file_names=file_names, | ||
| ) |
There was a problem hiding this comment.
Potential inconsistency with custom config_instance.
When ConfigAdapter is initialized with a custom config_instance, the apply_file() method still delegates to the module-level apply_config_file(), which updates the global config rather than self._config. This creates inconsistent behavior where get/set/update operate on the custom instance but apply_file bypasses it.
Consider injecting the config instance into the loader or documenting this as intentional behavior.
def apply_file(
self,
config_path: str | None = None,
search_paths: list[str] | None = None,
file_names: list[str] | None = None,
) -> bool:
- return apply_config_file(
- config_path=config_path,
- search_paths=search_paths,
- file_names=file_names,
- )
+ from ...config.loading import ConfigLoader
+ loader = ConfigLoader(config_instance=self._config)
+ return loader.apply(
+ config_path=config_path,
+ search_paths=search_paths,
+ file_names=file_names,
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp_fuzzer/client/adapters/config_adapter.py around lines 80 to 100,
apply_file() calls the module-level apply_config_file which mutates the global
config rather than the instance passed to ConfigAdapter, causing inconsistency
with methods that operate on self._config; modify apply_file to either call a
loader that accepts and populates self._config (pass self._config into
apply_config_file or a new helper) or, after calling apply_config_file,
replace/update self._config with the loaded configuration object so all
get/set/update operations use the same instance.
| # Lazy import to avoid circular dependencies | ||
| # Import happens at module level but after client module is initialized | ||
| def _get_constants(): | ||
| """Lazy import of constants to break circular dependencies.""" | ||
| from mcp_fuzzer.config.core.constants import ( | ||
| CONTENT_TYPE_HEADER, | ||
| DEFAULT_FORCE_KILL_TIMEOUT, | ||
| DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT, | ||
| DEFAULT_HTTP_ACCEPT, | ||
| DEFAULT_MAX_TOTAL_FUZZING_TIME, | ||
| DEFAULT_MAX_TOOL_TIME, | ||
| DEFAULT_PROTOCOL_RUNS_PER_TYPE, | ||
| DEFAULT_PROTOCOL_VERSION, | ||
| DEFAULT_TIMEOUT, | ||
| DEFAULT_TOOL_RUNS, | ||
| DEFAULT_TOOL_TIMEOUT, | ||
| JSON_CONTENT_TYPE, | ||
| MCP_PROTOCOL_VERSION_HEADER, | ||
| MCP_SESSION_ID_HEADER, | ||
| PROCESS_CLEANUP_TIMEOUT, | ||
| PROCESS_FORCE_KILL_TIMEOUT, | ||
| PROCESS_TERMINATION_TIMEOUT, | ||
| PROCESS_WAIT_TIMEOUT, | ||
| SAFETY_ENV_ALLOWLIST, | ||
| SAFETY_HEADER_DENYLIST, | ||
| SAFETY_LOCAL_HOSTS, | ||
| SAFETY_NO_NETWORK_DEFAULT, | ||
| SAFETY_PROXY_ENV_DENYLIST, | ||
| SSE_CONTENT_TYPE, | ||
| WATCHDOG_DEFAULT_CHECK_INTERVAL, | ||
| WATCHDOG_EXTRA_BUFFER, | ||
| WATCHDOG_MAX_HANG_ADDITIONAL, | ||
| ) | ||
| return { | ||
| "CONTENT_TYPE_HEADER": CONTENT_TYPE_HEADER, | ||
| "DEFAULT_FORCE_KILL_TIMEOUT": DEFAULT_FORCE_KILL_TIMEOUT, | ||
| "DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT": DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT, | ||
| "DEFAULT_HTTP_ACCEPT": DEFAULT_HTTP_ACCEPT, | ||
| "DEFAULT_MAX_TOTAL_FUZZING_TIME": DEFAULT_MAX_TOTAL_FUZZING_TIME, | ||
| "DEFAULT_MAX_TOOL_TIME": DEFAULT_MAX_TOOL_TIME, | ||
| "DEFAULT_PROTOCOL_RUNS_PER_TYPE": DEFAULT_PROTOCOL_RUNS_PER_TYPE, | ||
| "DEFAULT_PROTOCOL_VERSION": DEFAULT_PROTOCOL_VERSION, | ||
| "DEFAULT_TIMEOUT": DEFAULT_TIMEOUT, | ||
| "DEFAULT_TOOL_RUNS": DEFAULT_TOOL_RUNS, | ||
| "DEFAULT_TOOL_TIMEOUT": DEFAULT_TOOL_TIMEOUT, | ||
| "JSON_CONTENT_TYPE": JSON_CONTENT_TYPE, | ||
| "MCP_PROTOCOL_VERSION_HEADER": MCP_PROTOCOL_VERSION_HEADER, | ||
| "MCP_SESSION_ID_HEADER": MCP_SESSION_ID_HEADER, | ||
| "PROCESS_CLEANUP_TIMEOUT": PROCESS_CLEANUP_TIMEOUT, | ||
| "PROCESS_FORCE_KILL_TIMEOUT": PROCESS_FORCE_KILL_TIMEOUT, | ||
| "PROCESS_TERMINATION_TIMEOUT": PROCESS_TERMINATION_TIMEOUT, | ||
| "PROCESS_WAIT_TIMEOUT": PROCESS_WAIT_TIMEOUT, | ||
| "SAFETY_ENV_ALLOWLIST": SAFETY_ENV_ALLOWLIST, | ||
| "SAFETY_HEADER_DENYLIST": SAFETY_HEADER_DENYLIST, | ||
| "SAFETY_LOCAL_HOSTS": SAFETY_LOCAL_HOSTS, | ||
| "SAFETY_NO_NETWORK_DEFAULT": SAFETY_NO_NETWORK_DEFAULT, | ||
| "SAFETY_PROXY_ENV_DENYLIST": SAFETY_PROXY_ENV_DENYLIST, | ||
| "SSE_CONTENT_TYPE": SSE_CONTENT_TYPE, | ||
| "WATCHDOG_DEFAULT_CHECK_INTERVAL": WATCHDOG_DEFAULT_CHECK_INTERVAL, | ||
| "WATCHDOG_EXTRA_BUFFER": WATCHDOG_EXTRA_BUFFER, | ||
| "WATCHDOG_MAX_HANG_ADDITIONAL": WATCHDOG_MAX_HANG_ADDITIONAL, | ||
| } | ||
|
|
||
| # Import constants lazily | ||
| _constants = _get_constants() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the import pattern: this is "deferred" not "lazy".
The docstring describes "lazy imports" but _get_constants() is called eagerly at module load time (line 75). The pattern does help break circular dependencies by wrapping imports in a function (delaying resolution until the function is defined), but the term "lazy" typically implies on-demand loading which doesn't occur here.
Consider updating the docstring to clarify the actual behavior:
-# Lazy import to avoid circular dependencies
-# Import happens at module level but after client module is initialized
+# Deferred import to avoid circular dependencies
+# Import is wrapped in a function to delay resolution until after module initialization
def _get_constants():
- """Lazy import of constants to break circular dependencies."""
+ """Import constants from config module to break circular dependencies."""🤖 Prompt for AI Agents
In mcp_fuzzer/client/constants.py around lines 11 to 75, the docstring and
comments call this a "lazy import" but the function _get_constants() is invoked
immediately at module load (line 75), so it's actually a deferred/import-wrapper
pattern rather than true on-demand lazy loading; update the docstring and the
top-of-file comment to call it "deferred import" or "deferred import wrapper"
and briefly note that imports are wrapped to break circular dependencies (or, if
you prefer true lazy behavior, remove the eager _get_constants() call and call
it only where needed) so the text accurately describes the behavior.
| return { | ||
| "CONTENT_TYPE_HEADER": CONTENT_TYPE_HEADER, | ||
| "DEFAULT_FORCE_KILL_TIMEOUT": DEFAULT_FORCE_KILL_TIMEOUT, | ||
| "DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT": DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT, | ||
| "DEFAULT_HTTP_ACCEPT": DEFAULT_HTTP_ACCEPT, | ||
| "DEFAULT_MAX_TOTAL_FUZZING_TIME": DEFAULT_MAX_TOTAL_FUZZING_TIME, | ||
| "DEFAULT_MAX_TOOL_TIME": DEFAULT_MAX_TOOL_TIME, | ||
| "DEFAULT_PROTOCOL_RUNS_PER_TYPE": DEFAULT_PROTOCOL_RUNS_PER_TYPE, | ||
| "DEFAULT_PROTOCOL_VERSION": DEFAULT_PROTOCOL_VERSION, | ||
| "DEFAULT_TIMEOUT": DEFAULT_TIMEOUT, | ||
| "DEFAULT_TOOL_RUNS": DEFAULT_TOOL_RUNS, | ||
| "DEFAULT_TOOL_TIMEOUT": DEFAULT_TOOL_TIMEOUT, | ||
| "JSON_CONTENT_TYPE": JSON_CONTENT_TYPE, | ||
| "MCP_PROTOCOL_VERSION_HEADER": MCP_PROTOCOL_VERSION_HEADER, | ||
| "MCP_SESSION_ID_HEADER": MCP_SESSION_ID_HEADER, | ||
| "PROCESS_CLEANUP_TIMEOUT": PROCESS_CLEANUP_TIMEOUT, | ||
| "PROCESS_FORCE_KILL_TIMEOUT": PROCESS_FORCE_KILL_TIMEOUT, | ||
| "PROCESS_TERMINATION_TIMEOUT": PROCESS_TERMINATION_TIMEOUT, | ||
| "PROCESS_WAIT_TIMEOUT": PROCESS_WAIT_TIMEOUT, | ||
| "SAFETY_ENV_ALLOWLIST": SAFETY_ENV_ALLOWLIST, | ||
| "SAFETY_HEADER_DENYLIST": SAFETY_HEADER_DENYLIST, | ||
| "SAFETY_LOCAL_HOSTS": SAFETY_LOCAL_HOSTS, | ||
| "SAFETY_NO_NETWORK_DEFAULT": SAFETY_NO_NETWORK_DEFAULT, | ||
| "SAFETY_PROXY_ENV_DENYLIST": SAFETY_PROXY_ENV_DENYLIST, | ||
| "SSE_CONTENT_TYPE": SSE_CONTENT_TYPE, | ||
| "WATCHDOG_DEFAULT_CHECK_INTERVAL": WATCHDOG_DEFAULT_CHECK_INTERVAL, | ||
| "WATCHDOG_EXTRA_BUFFER": WATCHDOG_EXTRA_BUFFER, | ||
| "WATCHDOG_MAX_HANG_ADDITIONAL": WATCHDOG_MAX_HANG_ADDITIONAL, | ||
| } | ||
|
|
||
| # Import constants lazily | ||
| _constants = _get_constants() | ||
|
|
||
| # Export all constants | ||
| CONTENT_TYPE_HEADER = _constants["CONTENT_TYPE_HEADER"] | ||
| DEFAULT_FORCE_KILL_TIMEOUT = _constants["DEFAULT_FORCE_KILL_TIMEOUT"] | ||
| DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT = _constants["DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT"] | ||
| DEFAULT_HTTP_ACCEPT = _constants["DEFAULT_HTTP_ACCEPT"] | ||
| DEFAULT_MAX_TOTAL_FUZZING_TIME = _constants["DEFAULT_MAX_TOTAL_FUZZING_TIME"] | ||
| DEFAULT_MAX_TOOL_TIME = _constants["DEFAULT_MAX_TOOL_TIME"] | ||
| DEFAULT_PROTOCOL_RUNS_PER_TYPE = _constants["DEFAULT_PROTOCOL_RUNS_PER_TYPE"] | ||
| DEFAULT_PROTOCOL_VERSION = _constants["DEFAULT_PROTOCOL_VERSION"] | ||
| DEFAULT_TIMEOUT = _constants["DEFAULT_TIMEOUT"] | ||
| DEFAULT_TOOL_RUNS = _constants["DEFAULT_TOOL_RUNS"] | ||
| DEFAULT_TOOL_TIMEOUT = _constants["DEFAULT_TOOL_TIMEOUT"] | ||
| JSON_CONTENT_TYPE = _constants["JSON_CONTENT_TYPE"] | ||
| MCP_PROTOCOL_VERSION_HEADER = _constants["MCP_PROTOCOL_VERSION_HEADER"] | ||
| MCP_SESSION_ID_HEADER = _constants["MCP_SESSION_ID_HEADER"] | ||
| PROCESS_CLEANUP_TIMEOUT = _constants["PROCESS_CLEANUP_TIMEOUT"] | ||
| PROCESS_FORCE_KILL_TIMEOUT = _constants["PROCESS_FORCE_KILL_TIMEOUT"] | ||
| PROCESS_TERMINATION_TIMEOUT = _constants["PROCESS_TERMINATION_TIMEOUT"] | ||
| PROCESS_WAIT_TIMEOUT = _constants["PROCESS_WAIT_TIMEOUT"] | ||
| SAFETY_ENV_ALLOWLIST = _constants["SAFETY_ENV_ALLOWLIST"] | ||
| SAFETY_HEADER_DENYLIST = _constants["SAFETY_HEADER_DENYLIST"] | ||
| SAFETY_LOCAL_HOSTS = _constants["SAFETY_LOCAL_HOSTS"] | ||
| SAFETY_NO_NETWORK_DEFAULT = _constants["SAFETY_NO_NETWORK_DEFAULT"] | ||
| SAFETY_PROXY_ENV_DENYLIST = _constants["SAFETY_PROXY_ENV_DENYLIST"] | ||
| SSE_CONTENT_TYPE = _constants["SSE_CONTENT_TYPE"] | ||
| WATCHDOG_DEFAULT_CHECK_INTERVAL = _constants["WATCHDOG_DEFAULT_CHECK_INTERVAL"] | ||
| WATCHDOG_EXTRA_BUFFER = _constants["WATCHDOG_EXTRA_BUFFER"] | ||
| WATCHDOG_MAX_HANG_ADDITIONAL = _constants["WATCHDOG_MAX_HANG_ADDITIONAL"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the re-export pattern.
The current approach builds an intermediate dictionary, then extracts each value to a module-level variable. This is verbose and requires maintaining three parallel lists (imports, dict keys, module assignments).
A simpler approach would update globals() directly:
def _init_constants():
"""Import and inject constants into module namespace."""
from mcp_fuzzer.config.core.constants import (
CONTENT_TYPE_HEADER,
# ... other imports ...
)
# Inject into module globals
g = globals()
for name in __all__:
g[name] = locals()[name]
__all__ = [
"CONTENT_TYPE_HEADER",
# ... other names ...
]
_init_constants()This reduces duplication if maintainability is a concern. The current approach works correctly, so this is purely a refactoring suggestion.
🤖 Prompt for AI Agents
In mcp_fuzzer/client/constants.py around lines 44 to 104, the module builds an
intermediate _get_constants() dict then assigns each constant from that dict to
module-level names, which is verbose and duplicates lists; replace this with a
single _init_constants() function that imports the constants from
mcp_fuzzer.config.core.constants, declares the __all__ list of exported names,
then injects them into the module namespace by assigning globals()[name] =
locals()[name] for each name in __all__, and call _init_constants() at module
import time; remove the intermediate dict and the individual re-assignments
(_constants = _get_constants(), CONTENT_TYPE_HEADER =
_constants["CONTENT_TYPE_HEADER"], etc.) so maintenance only requires updating
the import list and __all__.
| def load_custom_transports(config_data: dict[str, Any]) -> None: | ||
| """Load and register custom transports from configuration. | ||
| Args: | ||
| config_data: Configuration dictionary containing custom_transports section | ||
| """ | ||
| custom_transports = config_data.get("custom_transports", {}) | ||
|
|
||
| for transport_name, transport_config in custom_transports.items(): |
There was a problem hiding this comment.
Guard against non-mapping custom_transports to avoid raw AttributeError
Right now custom_transports = config_data.get("custom_transports", {}) followed by custom_transports.items() will raise an AttributeError if the section is null, a list, or otherwise non-mapping. That happens before the inner try/except, so callers see a raw Python error instead of a ConfigFileError, and a benign null config would fail hard.
Consider normalizing and validating the section up front:
- custom_transports = config_data.get("custom_transports", {})
+ raw_section = config_data.get("custom_transports") or {}
+ if not isinstance(raw_section, dict):
+ raise ConfigFileError(
+ "custom_transports section must be a mapping of name -> config"
+ )
+ custom_transports = raw_sectionThis keeps error semantics consistent and treats null as “no custom transports” while still failing fast on clearly invalid shapes.
📝 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 load_custom_transports(config_data: dict[str, Any]) -> None: | |
| """Load and register custom transports from configuration. | |
| Args: | |
| config_data: Configuration dictionary containing custom_transports section | |
| """ | |
| custom_transports = config_data.get("custom_transports", {}) | |
| for transport_name, transport_config in custom_transports.items(): | |
| def load_custom_transports(config_data: dict[str, Any]) -> None: | |
| """Load and register custom transports from configuration. | |
| Args: | |
| config_data: Configuration dictionary containing custom_transports section | |
| """ | |
| raw_section = config_data.get("custom_transports") or {} | |
| if not isinstance(raw_section, dict): | |
| raise ConfigFileError( | |
| "custom_transports section must be a mapping of name -> config" | |
| ) | |
| custom_transports = raw_section | |
| for transport_name, transport_config in custom_transports.items(): |
🤖 Prompt for AI Agents
In mcp_fuzzer/config/extensions/transports.py around lines 17 to 25, the code
assumes config_data.get("custom_transports", {}) is a mapping and calling
.items() can raise AttributeError for nulls or non-mappings; normalize and
validate the section up front by retrieving the raw value, treat None as an
empty dict, and raise a ConfigFileError (with a clear message and the offending
value) if the value is not a dict before iterating—this ensures null becomes “no
custom transports” and invalid shapes fail with a ConfigFileError rather than a
raw AttributeError.
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| from typing import Any, Callable, Tuple |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using built-in tuple type hint.
Python 3.9+ supports generic built-in types directly. Since the file uses dict[str, Any] and list[str] (built-in generics), using Tuple from typing is inconsistent.
-from typing import Any, Callable, Tuple
+from typing import Any, CallableThen update the return type annotations (lines 51, 79) from Tuple[...] to tuple[...].
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp_fuzzer/config/loading/loader.py at line 7 and affecting return
annotations at lines 51 and 79, replace the imported typing.Tuple usage with the
built-in generic tuple to match existing use of built-in generics (e.g.,
dict[str, Any], list[str]). Remove Tuple from the import list and update the
function return type annotations from Tuple[...] to tuple[...], preserving the
same element types and order.
| def testbuild_network_schema(): | ||
| """Test that network schema includes network-related fields.""" | ||
| schema = build_network_schema() | ||
| assert "no_network" in schema | ||
| assert "allow_hosts" in schema | ||
| assert schema["no_network"]["type"] == "boolean" | ||
| assert schema["allow_hosts"]["type"] == "array" |
There was a problem hiding this comment.
Test function will not be discovered by pytest.
Same issue: testbuild_network_schema should be test_build_network_schema.
-def testbuild_network_schema():
+def test_build_network_schema():📝 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 testbuild_network_schema(): | |
| """Test that network schema includes network-related fields.""" | |
| schema = build_network_schema() | |
| assert "no_network" in schema | |
| assert "allow_hosts" in schema | |
| assert schema["no_network"]["type"] == "boolean" | |
| assert schema["allow_hosts"]["type"] == "array" | |
| def test_build_network_schema(): | |
| """Test that network schema includes network-related fields.""" | |
| schema = build_network_schema() | |
| assert "no_network" in schema | |
| assert "allow_hosts" in schema | |
| assert schema["no_network"]["type"] == "boolean" | |
| assert schema["allow_hosts"]["type"] == "array" |
🤖 Prompt for AI Agents
In tests/unit/config/test_schema_builders.py around lines 54 to 60, the test
function name is misspelled as testbuild_network_schema so pytest won't discover
it; rename the function to test_build_network_schema (update the def line and
any internal references) so it follows pytest's test naming convention and will
be discovered and run.
| def testbuild_auth_schema(): | ||
| """Test that auth schema includes auth configuration.""" | ||
| schema = build_auth_schema() | ||
| assert "auth" in schema | ||
| auth_schema = schema["auth"] | ||
| assert auth_schema["type"] == "object" | ||
| assert "providers" in auth_schema["properties"] | ||
| assert "mappings" in auth_schema["properties"] |
There was a problem hiding this comment.
Test function will not be discovered by pytest.
Same issue: testbuild_auth_schema should be test_build_auth_schema.
-def testbuild_auth_schema():
+def test_build_auth_schema():📝 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 testbuild_auth_schema(): | |
| """Test that auth schema includes auth configuration.""" | |
| schema = build_auth_schema() | |
| assert "auth" in schema | |
| auth_schema = schema["auth"] | |
| assert auth_schema["type"] == "object" | |
| assert "providers" in auth_schema["properties"] | |
| assert "mappings" in auth_schema["properties"] | |
| def test_build_auth_schema(): | |
| """Test that auth schema includes auth configuration.""" | |
| schema = build_auth_schema() | |
| assert "auth" in schema | |
| auth_schema = schema["auth"] | |
| assert auth_schema["type"] == "object" | |
| assert "providers" in auth_schema["properties"] | |
| assert "mappings" in auth_schema["properties"] |
🤖 Prompt for AI Agents
In tests/unit/config/test_schema_builders.py around lines 63 to 70, the test
function name is missing an underscore and won’t be discovered by pytest; rename
testbuild_auth_schema to test_build_auth_schema (and update any
references/imports if present) so pytest will automatically collect and run this
test.
| def testbuild_custom_transports_schema(): | ||
| """Test that custom transports schema is properly structured.""" | ||
| schema = build_custom_transports_schema() | ||
| assert "custom_transports" in schema | ||
| transport_schema = schema["custom_transports"] | ||
| assert transport_schema["type"] == "object" | ||
| assert "patternProperties" in transport_schema | ||
| assert "additionalProperties" in transport_schema | ||
| assert transport_schema["additionalProperties"] is False |
There was a problem hiding this comment.
Test function will not be discovered by pytest.
Same issue: testbuild_custom_transports_schema should be test_build_custom_transports_schema.
-def testbuild_custom_transports_schema():
+def test_build_custom_transports_schema():🤖 Prompt for AI Agents
In tests/unit/config/test_schema_builders.py around lines 73 to 81, the test
function name is misspelled as testbuild_custom_transports_schema which prevents
pytest from discovering it; rename the function to
test_build_custom_transports_schema (and update any references/imports if
present) so it follows pytest's test_ naming convention and will be executed.
| def testbuild_safety_schema(): | ||
| """Test that safety schema includes all safety-related fields.""" | ||
| schema = build_safety_schema() | ||
| assert "safety" in schema | ||
| safety_schema = schema["safety"] | ||
| assert safety_schema["type"] == "object" | ||
| props = safety_schema["properties"] | ||
| assert "enabled" in props | ||
| assert "local_hosts" in props | ||
| assert "no_network" in props | ||
| assert "header_denylist" in props | ||
|
|
There was a problem hiding this comment.
Test function will not be discovered by pytest.
Same issue: testbuild_safety_schema should be test_build_safety_schema.
-def testbuild_safety_schema():
+def test_build_safety_schema():📝 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 testbuild_safety_schema(): | |
| """Test that safety schema includes all safety-related fields.""" | |
| schema = build_safety_schema() | |
| assert "safety" in schema | |
| safety_schema = schema["safety"] | |
| assert safety_schema["type"] == "object" | |
| props = safety_schema["properties"] | |
| assert "enabled" in props | |
| assert "local_hosts" in props | |
| assert "no_network" in props | |
| assert "header_denylist" in props | |
| def test_build_safety_schema(): | |
| """Test that safety schema includes all safety-related fields.""" | |
| schema = build_safety_schema() | |
| assert "safety" in schema | |
| safety_schema = schema["safety"] | |
| assert safety_schema["type"] == "object" | |
| props = safety_schema["properties"] | |
| assert "enabled" in props | |
| assert "local_hosts" in props | |
| assert "no_network" in props | |
| assert "header_denylist" in props |
🤖 Prompt for AI Agents
In tests/unit/config/test_schema_builders.py around lines 84 to 95, the test
function name is misspelled as testbuild_safety_schema so pytest will not
discover it; rename the function to test_build_safety_schema to follow pytest's
test_* naming convention so the test is discovered and executed.
| def testbuild_output_schema(): | ||
| """Test that output schema includes output configuration.""" | ||
| schema = build_output_schema() | ||
| assert "output" in schema | ||
| output_schema = schema["output"] | ||
| assert output_schema["type"] == "object" | ||
| props = output_schema["properties"] | ||
| assert "format" in props | ||
| assert "directory" in props | ||
| assert "compress" in props | ||
| assert "types" in props | ||
| assert "retention" in props |
There was a problem hiding this comment.
Test function will not be discovered by pytest.
Same issue: testbuild_output_schema should be test_build_output_schema.
-def testbuild_output_schema():
+def test_build_output_schema():📝 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 testbuild_output_schema(): | |
| """Test that output schema includes output configuration.""" | |
| schema = build_output_schema() | |
| assert "output" in schema | |
| output_schema = schema["output"] | |
| assert output_schema["type"] == "object" | |
| props = output_schema["properties"] | |
| assert "format" in props | |
| assert "directory" in props | |
| assert "compress" in props | |
| assert "types" in props | |
| assert "retention" in props | |
| def test_build_output_schema(): | |
| """Test that output schema includes output configuration.""" | |
| schema = build_output_schema() | |
| assert "output" in schema | |
| output_schema = schema["output"] | |
| assert output_schema["type"] == "object" | |
| props = output_schema["properties"] | |
| assert "format" in props | |
| assert "directory" in props | |
| assert "compress" in props | |
| assert "types" in props | |
| assert "retention" in props |
🤖 Prompt for AI Agents
In tests/unit/config/test_schema_builders.py around lines 97 to 108, the test
function is named testbuild_output_schema which prevents pytest from discovering
it; rename the function to test_build_output_schema (and update any
references/imports if present) so it follows pytest's test naming convention and
will be discovered and executed.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/cli/test_cli.py (1)
573-580: Tighten the expected exception type for bad config files
build_cli_confignow wraps failures in aConfigFileError, but the test only asserts a genericException. AssertingConfigFileErrorwould better lock in the public error contract:- with pytest.raises(Exception): + from mcp_fuzzer.exceptions import ConfigFileError + + with pytest.raises(ConfigFileError): build_cli_config(args)mcp_fuzzer/cli/config_merge.py (1)
72-141: Preserve underlying exception context when wrapping config load failuresThe switch to
config_mediator.load_file/apply_filecorrectly differentiates between explicit config (fatal on error) and default discovery (best-effort with a debug log). Only concern is that theexcept Exception as exc:block re-raises a freshConfigFileErrorwithout chaining, which loses the original traceback.Consider:
- except Exception as exc: - raise ConfigFileError( - f"Failed to load configuration file '{args.config}': {exc}" - ) + except Exception as exc: + raise ConfigFileError( + f"Failed to load configuration file '{args.config}': {exc}" + ) from excThis keeps the new error surface while retaining the root cause for debugging.
♻️ Duplicate comments (1)
mcp_fuzzer/config/loading/loader.py (1)
6-24: Align type hints with builtin generics (repeat of earlier nit)This module already uses builtin generics like
dict[str, Any], but still imports and usesTuplefromtyping. For consistency, you can dropTupleand use builtintuple[...]in the return annotations:-from typing import Any, Callable, Tuple +from typing import Any, Callable ... - ) -> Tuple[ConfigDict | None, str | None]: + ) -> tuple[ConfigDict | None, str | None]: ... - ) -> Tuple[ - ConfigDict | None, str | None - ]: + ) -> tuple[ConfigDict | None, str | None]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
mcp_fuzzer/cli/config_merge.py(4 hunks)mcp_fuzzer/config/loading/loader.py(1 hunks)mcp_fuzzer/config/loading/parser.py(1 hunks)mcp_fuzzer/config/schema/builders.py(1 hunks)tests/unit/cli/test_cli.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: examples/auth_config.json:3-6
Timestamp: 2025-08-08T11:47:21.915Z
Learning: In repo Agent-Hellboy/mcp-server-fuzzer, the file examples/auth_config.json uses "api_key": "secret123" intentionally for the dummy example server. Do not flag this as a leaked secret in future reviews; maintainers prefer keeping it unchanged. If needed, suggest adding a README note rather than changing the example value.
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.
📚 Learning: 2025-08-08T11:50:01.877Z
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.
Applied to files:
mcp_fuzzer/config/loading/loader.py
🧬 Code graph analysis (3)
mcp_fuzzer/cli/config_merge.py (3)
mcp_fuzzer/client/adapters/config_adapter.py (4)
get(37-47)load_file(66-78)update(58-64)apply_file(80-100)mcp_fuzzer/client/ports/config_port.py (4)
get(23-33)load_file(55-67)update(46-52)apply_file(70-86)mcp_fuzzer/config/core/manager.py (2)
get(70-72)update(78-80)
mcp_fuzzer/config/loading/parser.py (1)
mcp_fuzzer/exceptions.py (1)
ConfigFileError(272-276)
mcp_fuzzer/config/loading/loader.py (7)
mcp_fuzzer/config/core/manager.py (2)
Configuration(43-80)update(78-80)mcp_fuzzer/config/extensions/transports.py (1)
load_custom_transports(17-79)mcp_fuzzer/config/schema/composer.py (1)
get_config_schema(20-42)mcp_fuzzer/config/loading/discovery.py (1)
find_config_file(12-32)mcp_fuzzer/config/loading/parser.py (1)
load_config_file(14-61)mcp_fuzzer/config/loading/search_params.py (1)
ConfigSearchParams(10-24)mcp_fuzzer/exceptions.py (2)
ConfigFileError(272-276)MCPError(19-44)
🔇 Additional comments (10)
mcp_fuzzer/config/schema/builders.py (4)
32-56: LGTM!Good documentation clarifying the precedence between
safety_enabledandsafety.enabled. The enum constraint forlog_levelis appropriate.
85-94: LGTM!Simple and well-structured schema for network configuration.
127-165: LGTM!Well-structured schema with appropriate use of
patternPropertiesfor dynamic transport names and strictadditionalProperties: Falseconstraints.
168-197: LGTM!Clear documentation about precedence rules and consistent use of
additionalProperties: False.mcp_fuzzer/config/loading/parser.py (1)
14-61: YAML loader behavior and error normalization look correctThe function cleanly validates existence/extension, treats empty/null YAML as
{}, enforces a mapping top level, and normalizes parse/permission/other errors intoConfigFileErrorwhile preserving context. No issues from my side.tests/unit/cli/test_cli.py (3)
268-275: Mediator patch target for validate_config looks correctPatching
mcp_fuzzer.cli.validators.config_mediator.load_filematches the new mediator-based implementation ofValidationManager.validate_config_file. This keeps the test aligned with the refactor without changing its intent.
545-559: Config file merge test correctly targets config_mediatorSwitching the patch to
mcp_fuzzer.cli.config_merge.config_mediator.load_fileand drivingbuild_cli_configviaconfig="custom.yml", endpoint=Noneproperly exercises the mediator-backed loading path and verifies CLI args are populated from config. Looks good.
560-572: apply_file=False and debug log behavior are well coveredThe test accurately encodes the new contract where
apply_file()returnsFalseon failure and ensures the CLI falls back to defaults while emitting a debug message. This gives good regression coverage around the non-fatal default-config path.mcp_fuzzer/cli/config_merge.py (1)
62-70: Mediator-based config lookup in_transfer_config_to_argslooks goodUsing
config_mediator.gethere keeps the CLI wired to the new configuration core while preserving the “CLI overrides config/env defaults” behavior via the(config_value is not None and args_value == default_value)check. No issues.mcp_fuzzer/config/loading/loader.py (1)
96-160: apply/apply_config_file correctly expose a non-raising, boolean API
ConfigLoader.applyreturningFalseonConfigFileError/MCPError(or missing file) andTrueon success matches the newconfig_mediator.apply_filecontract, while still updating the injectedConfigurationinstance. Theapply_from_paramsandapply_config_filehelpers are thin and predictable. No issues here.
| def load( | ||
| self, | ||
| config_path: str | None = None, | ||
| search_paths: list[str] | None = None, | ||
| file_names: list[str] | None = None, | ||
| ) -> Tuple[ConfigDict | None, str | None]: | ||
| """Return the configuration dictionary and source file path. | ||
| Args: | ||
| config_path: Explicit path to config file | ||
| search_paths: List of directories to search | ||
| file_names: List of file names to search for | ||
| Returns: | ||
| Tuple of (config_dict, file_path) or (None, None) if not found | ||
| """ | ||
| file_path = self.discoverer(config_path, search_paths, file_names) | ||
| if not file_path: | ||
| logger.debug("No configuration file found") | ||
| return None, None | ||
|
|
||
| logger.debug("Loading configuration from %s", file_path) | ||
| try: | ||
| config_data = self.parser(file_path) | ||
| self.transport_loader(config_data) | ||
| except (ConfigFileError, MCPError): | ||
| logger.exception("Failed to load configuration from %s", file_path) | ||
| raise | ||
|
|
||
| return config_data, file_path | ||
|
|
||
| def load_from_params( | ||
| self, params: ConfigSearchParams | ||
| ) -> Tuple[ | ||
| ConfigDict | None, str | None | ||
| ]: | ||
| """Load configuration using ConfigSearchParams. | ||
| Args: | ||
| params: Configuration search parameters | ||
| Returns: | ||
| Tuple of (config_dict, file_path) or (None, None) if not found | ||
| """ | ||
| return self.load( | ||
| config_path=params.config_path, | ||
| search_paths=params.search_paths, | ||
| file_names=params.file_names, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ConfigLoader.load / load_from_params behavior matches the new stack, with one logging nit
The discovery → parse → transport-loading flow is clean, and returning (None, None) when no file is found gives a clear signal to callers. Note that load_custom_transports already logs and wraps failures in ConfigFileError; calling logger.exception here on (ConfigFileError, MCPError) will log the same failure twice. If log noise becomes an issue, consider downgrading this to logger.debug or relying on the inner logging.
🤖 Prompt for AI Agents
In mcp_fuzzer/config/loading/loader.py around lines 46 to 95, the except block
logs ConfigFileError and MCPError with logger.exception which duplicates error
output because inner calls already log and wrap failures; change the except
handler to either log at a lower level (logger.debug or logger.info) with a
short contextual message before re-raising, or remove the logger call entirely
and just re-raise the caught exception so the inner logging remains the single
source of truth.
| def build_timeout_schema() -> dict[str, Any]: | ||
| """Build schema for timeout-related configuration.""" | ||
| return { | ||
| "timeout": {"type": "number", "description": "Default timeout in seconds"}, | ||
| "tool_timeout": { | ||
| "type": "number", | ||
| "description": "Tool-specific timeout in seconds", | ||
| }, | ||
| "http_timeout": { | ||
| "type": "number", | ||
| "description": "HTTP transport timeout in seconds", | ||
| }, | ||
| "sse_timeout": { | ||
| "type": "number", | ||
| "description": "SSE transport timeout in seconds", | ||
| }, | ||
| "stdio_timeout": { | ||
| "type": "number", | ||
| "description": "STDIO transport timeout in seconds", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding minimum constraints for timeout values.
Timeout values should logically be non-negative. Without a minimum constraint, the schema would accept negative values which are semantically invalid and could cause unexpected behavior.
def build_timeout_schema() -> dict[str, Any]:
"""Build schema for timeout-related configuration."""
return {
- "timeout": {"type": "number", "description": "Default timeout in seconds"},
+ "timeout": {"type": "number", "minimum": 0, "description": "Default timeout in seconds"},
"tool_timeout": {
"type": "number",
+ "minimum": 0,
"description": "Tool-specific timeout in seconds",
},
"http_timeout": {
"type": "number",
+ "minimum": 0,
"description": "HTTP transport timeout in seconds",
},
"sse_timeout": {
"type": "number",
+ "minimum": 0,
"description": "SSE transport timeout in seconds",
},
"stdio_timeout": {
"type": "number",
+ "minimum": 0,
"description": "STDIO transport timeout in seconds",
},
}🤖 Prompt for AI Agents
In mcp_fuzzer/config/schema/builders.py around lines 9 to 29, the timeout fields
lack a minimum constraint and currently allow negative values; update each
timeout field ("timeout", "tool_timeout", "http_timeout", "sse_timeout",
"stdio_timeout") to include a "minimum": 0 constraint so values are
non-negative. Keep the type as "number" and add the minimum property to each
field's schema entry.
| "runs": {"type": "integer", "description": "Number of fuzzing runs"}, | ||
| "runs_per_type": { | ||
| "type": "integer", | ||
| "description": "Number of runs per protocol type", | ||
| }, | ||
| "protocol_type": { | ||
| "type": "string", | ||
| "description": "Specific protocol type to fuzz", | ||
| }, | ||
| "max_concurrency": { | ||
| "type": "integer", | ||
| "description": "Maximum concurrent operations", | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding minimum constraints for integer fields.
Fields like runs, runs_per_type, and max_concurrency should have sensible minimum values to prevent invalid configurations (e.g., 0 or negative runs).
- "runs": {"type": "integer", "description": "Number of fuzzing runs"},
+ "runs": {"type": "integer", "minimum": 1, "description": "Number of fuzzing runs"},
"runs_per_type": {
"type": "integer",
+ "minimum": 1,
"description": "Number of runs per protocol type",
},
...
"max_concurrency": {
"type": "integer",
+ "minimum": 1,
"description": "Maximum concurrent operations",
},📝 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.
| "runs": {"type": "integer", "description": "Number of fuzzing runs"}, | |
| "runs_per_type": { | |
| "type": "integer", | |
| "description": "Number of runs per protocol type", | |
| }, | |
| "protocol_type": { | |
| "type": "string", | |
| "description": "Specific protocol type to fuzz", | |
| }, | |
| "max_concurrency": { | |
| "type": "integer", | |
| "description": "Maximum concurrent operations", | |
| }, | |
| "runs": {"type": "integer", "minimum": 1, "description": "Number of fuzzing runs"}, | |
| "runs_per_type": { | |
| "type": "integer", | |
| "minimum": 1, | |
| "description": "Number of runs per protocol type", | |
| }, | |
| "protocol_type": { | |
| "type": "string", | |
| "description": "Specific protocol type to fuzz", | |
| }, | |
| "max_concurrency": { | |
| "type": "integer", | |
| "minimum": 1, | |
| "description": "Maximum concurrent operations", | |
| }, |
🤖 Prompt for AI Agents
In mcp_fuzzer/config/schema/builders.py around lines 69 to 81, the integer
fields lack minimum constraints; add a "minimum": 1 property to "runs",
"runs_per_type", and "max_concurrency" schema entries so they cannot be zero or
negative, and ensure the JSON Schema remains valid after the change (update any
tests or callers that assume 0 if needed).
| def build_auth_schema() -> dict[str, Any]: | ||
| """Build schema for authentication configuration.""" | ||
| return { | ||
| "auth": { | ||
| "type": "object", | ||
| "properties": { | ||
| "providers": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "type": { | ||
| "type": "string", | ||
| "enum": ["api_key", "basic", "oauth", "custom"], | ||
| }, | ||
| "id": {"type": "string"}, | ||
| "config": {"type": "object"}, | ||
| }, | ||
| "required": ["type", "id"], | ||
| }, | ||
| }, | ||
| "mappings": { | ||
| "type": "object", | ||
| "additionalProperties": {"type": "string"}, | ||
| }, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent additionalProperties constraint.
Other schemas in this file (safety, output, custom_transports) explicitly set additionalProperties: False to prevent unknown properties. The auth object and provider items lack this constraint, which could allow typos or invalid properties to slip through validation silently.
"auth": {
"type": "object",
"properties": {
"providers": {
"type": "array",
"items": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": ["api_key", "basic", "oauth", "custom"],
},
"id": {"type": "string"},
"config": {"type": "object"},
},
"required": ["type", "id"],
+ "additionalProperties": False,
},
},
"mappings": {
"type": "object",
"additionalProperties": {"type": "string"},
},
},
+ "additionalProperties": False,
},📝 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 build_auth_schema() -> dict[str, Any]: | |
| """Build schema for authentication configuration.""" | |
| return { | |
| "auth": { | |
| "type": "object", | |
| "properties": { | |
| "providers": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "type": { | |
| "type": "string", | |
| "enum": ["api_key", "basic", "oauth", "custom"], | |
| }, | |
| "id": {"type": "string"}, | |
| "config": {"type": "object"}, | |
| }, | |
| "required": ["type", "id"], | |
| }, | |
| }, | |
| "mappings": { | |
| "type": "object", | |
| "additionalProperties": {"type": "string"}, | |
| }, | |
| }, | |
| }, | |
| } | |
| def build_auth_schema() -> dict[str, Any]: | |
| """Build schema for authentication configuration.""" | |
| return { | |
| "auth": { | |
| "type": "object", | |
| "properties": { | |
| "providers": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "type": { | |
| "type": "string", | |
| "enum": ["api_key", "basic", "oauth", "custom"], | |
| }, | |
| "id": {"type": "string"}, | |
| "config": {"type": "object"}, | |
| }, | |
| "required": ["type", "id"], | |
| "additionalProperties": False, | |
| }, | |
| }, | |
| "mappings": { | |
| "type": "object", | |
| "additionalProperties": {"type": "string"}, | |
| }, | |
| }, | |
| "additionalProperties": False, | |
| }, | |
| } |
🤖 Prompt for AI Agents
In mcp_fuzzer/config/schema/builders.py around lines 97 to 124, the auth schema
lacks explicit additionalProperties: False which is inconsistent with other
schemas and allows unexpected fields; update the returned dict to add
"additionalProperties": False on the "auth" object and also add
"additionalProperties": False to the provider item object (the object under
"items") so unknown keys are rejected; leave "config" as an unconstrained object
only if intended, otherwise set constraints there too.
| "retention": { | ||
| "type": "object", | ||
| "properties": { | ||
| "days": { | ||
| "type": "integer", | ||
| "description": "Number of days to retain output files", | ||
| }, | ||
| "max_size": { | ||
| "type": "string", | ||
| "description": ( | ||
| "Maximum size of output directory " | ||
| "(e.g., '1GB', '500MB')" | ||
| ), | ||
| }, | ||
| }, | ||
| "additionalProperties": False, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding constraints for retention fields.
The days field should have a minimum value, and max_size could benefit from a pattern to validate the expected format.
"retention": {
"type": "object",
"properties": {
"days": {
"type": "integer",
+ "minimum": 0,
"description": "Number of days to retain output files",
},
"max_size": {
"type": "string",
+ "pattern": "^\\d+(\\.\\d+)?\\s*(B|KB|MB|GB|TB)$",
"description": (
"Maximum size of output directory "
"(e.g., '1GB', '500MB')"
),
},
},
"additionalProperties": False,
},📝 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.
| "retention": { | |
| "type": "object", | |
| "properties": { | |
| "days": { | |
| "type": "integer", | |
| "description": "Number of days to retain output files", | |
| }, | |
| "max_size": { | |
| "type": "string", | |
| "description": ( | |
| "Maximum size of output directory " | |
| "(e.g., '1GB', '500MB')" | |
| ), | |
| }, | |
| }, | |
| "additionalProperties": False, | |
| }, | |
| "retention": { | |
| "type": "object", | |
| "properties": { | |
| "days": { | |
| "type": "integer", | |
| "minimum": 0, | |
| "description": "Number of days to retain output files", | |
| }, | |
| "max_size": { | |
| "type": "string", | |
| "pattern": "^\\d+(\\.\\d+)?\\s*(B|KB|MB|GB|TB)$", | |
| "description": ( | |
| "Maximum size of output directory " | |
| "(e.g., '1GB', '500MB')" | |
| ), | |
| }, | |
| }, | |
| "additionalProperties": False, | |
| }, |
🤖 Prompt for AI Agents
In mcp_fuzzer/config/schema/builders.py around lines 237 to 253, the retention
schema lacks validation constraints: add a "minimum" (e.g., 1) to the "days"
integer to prevent non-positive values, and add a "pattern" to "max_size" to
enforce a size format such as digits followed by a unit (e.g., regex like
^\d+(KB|MB|GB|TB)$) so only values like "500MB" or "1GB" are accepted; keep
additionalProperties False and update the field descriptions if desired to
document the enforced constraints.
Pull Request Summary: Redesign Config Subsystem
Overview
This PR implements a comprehensive redesign of the configuration subsystem using enterprise architecture patterns (Hexagonal Architecture, Ports & Adapters) to improve modularity, testability, and separation of concerns. The refactoring transforms a monolithic config module into a layered, decoupled system with clear abstractions while maintaining full backward compatibility.
Design Changes & Architecture
1. Hexagonal Architecture (Ports & Adapters) Implementation
Core Abstraction Layer:
ConfigPort Interface (
mcp_fuzzer/client/ports/config_port.py): Defines abstract contract for all configuration operationsget(),set(),update()- configuration state managementload_file(),apply_file()- file-based configuration loadingget_schema()- configuration schema retrievalConfigAdapter (
mcp_fuzzer/client/adapters/config_adapter.py): Concrete implementation (~113 lines) delegating to config moduleconfig_mediator: Global singleton providing unified access point for configuration across codebase
Architecture Benefits:
2. Strategic Separation of Concerns
The monolithic
config/loader.py(completely removed) has been decomposed into focused modules:config/core/manager.pyconfig/core/constants.pyconfig/loading/discovery.pyconfig/loading/parser.pyconfig/loading/loader.pyconfig/extensions/transports.pyconfig/schema/builders.pyconfig/schema/composer.pySingle Responsibility Achievement: Each module handles exactly one concern with clear boundaries.
3. Dependency Injection & Strategy Pattern
ConfigLoaderclass accepts three injectable components as callables:Design Benefits:
4. Constant Reorganization: Three-Layer Architecture
Problem Solved: Prevent circular imports while maintaining flat public API
Solution:
config/core/constants.py: Canonical definitions (authoritative source)client/constants.py: Lazy-loaded re-export layer with deferred importsmcp_fuzzer.client: Unified public API for constantsLazy Initialization Pattern in
client/constants.py:Why This Works: Deferring imports to function-execution time breaks circular dependency chain at import time.
5. Schema as First-Class Citizen
Builder Pattern Implementation:
build_timeout_schema()- timeout configurationbuild_basic_schema()- basic properties (log_level, mode, fs_root)build_fuzzing_schema()- fuzzing parametersbuild_network_schema()- network configurationbuild_auth_schema()- authentication settingsbuild_custom_transports_schema()- transport customizationbuild_safety_schema()- security settingsbuild_output_schema()- output configurationComposition:
get_config_schema()merges all builders into single schema objectAdvantages:
Design Patterns Applied
SOLID Principles Compliance
✅ Single Responsibility Principle - ACHIEVED
✅ Open/Closed Principle - ACHIEVED
load_custom_transports✅ Interface Segregation Principle - ACHIEVED
✅ Dependency Inversion Principle - ACHIEVED
Code & Design Smells Detected
🟡 Issues & Mitigations
client/constants.pydefers imports; structure validated; no config→client imports detectedconfig_mediatoris hidden dependency; mocking supported in tests; consider constructor injection in futureraise ... from edetected); should standardize across all modules✅ Observed Best Practices
🔴 Code Smell: Exception Handling Inconsistency (Minor)
Location:
mcp_fuzzer/config/extensions/transports.pyIssue: Mixed exception handling patterns - while exception chaining is present (
raise ... from e), standardization could be improved across all error pathsExample of Good Pattern Found:
Impact: Low - makes debugging stack traces slightly harder in edge cases
Recommendation: Consistently apply
raise ... from epattern when re-raising exceptions throughout extensions moduleArchitecture Strengths
✅ Excellent Separation of Concerns: Each module 40-160 lines with single responsibility
✅ Strong Abstraction Through Ports: ConfigPort enables testing and future alternatives
✅ Strategic Dependency Injection: Reduces coupling, improves testability significantly
✅ Smart Constants Reorganization: Solves circular imports without sacrificing flat API
✅ Schema as Composable Data: Supports validation, documentation, extensibility
✅ Comprehensive Test Coverage: 982 lines of test code across 7 test modules
✅ Backward Compatibility: Original API preserved through loading module re-exports
✅ SOLID Compliance: 5/5 principles well-addressed with documented trade-offs
✅ Zero Breaking Changes: All existing code continues to function without modification
Remaining Architectural Considerations
client/constants.pyis solution; verified no config→client imports exist; future additions need careful reviewconfig_mediatoris hidden dependency; recommend documenting as such and considering constructor injection for critical components in future refactorsraise ... from epattern across extensions moduleMigration Impact & Backward Compatibility
Scope: Changes propagate through CLI, integration layers, and test infrastructure while maintaining full backward compatibility
config_merge.py,startup_info.py,validators.pymigrated to useconfig_mediatorconfig.core.constantsdirectlyKey Achievement: Zero breaking changes to public API; all existing code continues to function while new code can adopt improved patterns
Summary
This PR successfully implements enterprise-grade architecture patterns to transform a monolithic configuration system (single 200+ line file) into a modular, testable, decoupled design (8 focused modules, 40-160 lines each).
Architecture Quality:
Code Quality:
Trade-offs & Considerations:
Recommendation: PR ready for merge; consider standardizing exception chaining patterns and adding schema composition validator in follow-up work.