Reorganize tests into component-based structure #76
Conversation
…types, and array/object constraints
- Implement comprehensive JSON Schema parser with support for types, constraints, and combinations - Add property-based invariants for response validation and error checking - Update documentation with schema parser and invariants details - Fix tests and linting issues Fixes #10: Add property-based invariants and checks Fixes #12: Enhance JSON Schema parsing for fuzz strategies Fixes #53: Type safety and code quality enhancements
WalkthroughAdds component-aware test tooling and CI workflows, a JSON Schema-based strategy parser, runtime invariant checks integrated into the fuzz engine, streaming transport APIs and factory URL support, many new unit/integration tests and test helpers, plus related typing and executor shutdown behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant GH as GitHub Actions
participant Job as component-tests job
participant Repo as repo
participant Py as pytest
participant CC as Codecov
Dev->>GH: workflow_dispatch (components?)
GH->>Job: start component-tests
Job->>Repo: checkout (fetch-depth:0)
Job->>Job: determine per-component booleans from input or enable all
alt component X enabled
Job->>Py: run pytest - tests/unit/X -> coverage.X.xml
end
Job->>Py: run pytest - integration if any component enabled -> coverage.integration.xml
Job->>Job: check coverage files & CODECOV_TOKEN
alt has_token && has_coverage
Job->>CC: upload coverage.*.xml
end
sequenceDiagram
autonumber
participant User
participant Py as pytest
participant Hook as tests/conftest.py
participant Git as git
User->>Py: pytest [--component=a,b | --changed-only]
Py->>Hook: register options & configure
alt --changed-only
Hook->>Git: git diff HEAD -- mcp_fuzzer/*
Git-->>Hook: changed file paths -> components set
end
Hook->>Py: pytest_collection_modifyitems -> skip unit tests not matching selected components (integration exempt)
Py-->>User: run filtered tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
- Coverage 60.83% 58.36% -2.47%
==========================================
Files 51 53 +2
Lines 3679 4393 +714
Branches 542 747 +205
==========================================
+ Hits 2238 2564 +326
- Misses 1299 1602 +303
- Partials 142 227 +85 ☔ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (3)
129-147: Patch async function with AsyncMock; current patch will be awaited and fail.
fuzz_toolsawaitsself.fuzzer.fuzz_tool, but this patch creates a non-awaitable MagicMock.Fix by using AsyncMock and an async side_effect:
- with patch.object(self.fuzzer, "fuzz_tool") as mock_fuzz: - - def side_effect(tool, runs, phase="aggressive"): + with patch.object(self.fuzzer, "fuzz_tool", new_callable=AsyncMock) as mock_fuzz: + + async def side_effect(tool, runs, phase="aggressive"): if tool["name"] == "tool1": raise Exception("Test exception") else: return [ { "tool_name": tool["name"], "run": 1, "args": {"param2": 42}, "success": True, } ]
524-532: Await the coroutines; test currently treats coroutines as results.
fuzz_toolis async; not awaiting leads to TypeError in assertions.Apply:
- results1 = fuzzer1.fuzz_tool(tool, runs=1) - results2 = fuzzer2.fuzz_tool(tool, runs=1) + results1 = await fuzzer1.fuzz_tool(tool, runs=1) + results2 = await fuzzer2.fuzz_tool(tool, runs=1)
1-536: Add module-level pytest markers
Insert at the top of tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py:import pytest pytestmark = [pytest.mark.unit, pytest.mark.fuzz_engine, pytest.mark.fuzzer]
🧹 Nitpick comments (25)
.github/workflows/tests.yml (1)
15-17: Speed up CI and improve signal.
- Cache pip to cut install time.
- Consider branch coverage for richer reports.
Apply:
- with: - python-version: '3.11' + with: + python-version: '3.11' + cache: 'pip' @@ - pytest -vv --cov=mcp_fuzzer --cov-report=xml + pytest -vv --cov=mcp_fuzzer --cov-report=xml --cov-branchAlso applies to: 25-25
tests/unit/safety_system/test_policy.py (1)
1-11: Optional: add explicit component markers here.
If marker selection is used, make it explicit in this module to avoid relying solely on auto-marking.@@ #!/usr/bin/env python3 import os +import pytest @@ from mcp_fuzzer.safety_system.policy import ( @@ ) +pytestmark = [pytest.mark.unit, pytest.mark.safety_system]tests/unit/config/test_config_loader.py (1)
19-24: Use tmp_path to simplify temp management.
Reduces manual cleanup and flakiness.-@pytest.fixture -def config_files(): - """Create temporary YAML files for testing.""" - # Create temporary directory - temp_dir = tempfile.TemporaryDirectory() +@pytest.fixture +def config_files(tmp_path): + """Create temporary YAML files for testing.""" @@ - yml_path = os.path.join(temp_dir.name, "mcp-fuzzer.yml") - with open(yml_path, "w") as f: + yml_path = os.path.join(tmp_path, "mcp-fuzzer.yml") + with open(yml_path, "w") as f: @@ - yaml_path = os.path.join(temp_dir.name, "mcp-fuzzer.yaml") - with open(yaml_path, "w") as f: + yaml_path = os.path.join(tmp_path, "mcp-fuzzer.yaml") + with open(yaml_path, "w") as f: @@ - "temp_dir": temp_dir, - "yml_path": yml_path, - "yaml_path": yaml_path, + "temp_dir": tmp_path, + "yml_path": yml_path, + "yaml_path": yaml_path, } - - # Cleanup - temp_dir.cleanup()Also applies to: 36-55
tests/unit/fuzz_engine/runtime/test_runtime.py (2)
1-21: Optional: add component markers for selection.
Make runtime/fuzz_engine component intent explicit.@@ import pytest @@ from mcp_fuzzer.fuzz_engine.runtime import ( @@ ) +pytestmark = [pytest.mark.unit, pytest.mark.fuzz_engine, pytest.mark.runtime]
245-247: Avoid sleep in unit tests.
Replace real sleeps with direct timestamp manipulation to keep tests fast/deterministic.- # Wait a small amount of time to ensure timestamp would be different - await asyncio.sleep(0.01) + # Simulate time passing without real sleep + async with watchdog._lock: + watchdog._processes[12345]["last_activity"] -= 1.0pyproject.toml (1)
76-78: Resolve verbosity/coverage defaults split between tox and addopts.
-qin addopts and-vvdefault in tox will fight. Prefer one source of truth.Example:
-addopts = "-q --maxfail=1 --disable-warnings --cov=mcp_fuzzer --cov-report=term-missing --cov-report=xml" +addopts = "--maxfail=1 --disable-warnings --cov=mcp_fuzzer --cov-report=term-missing --cov-report=xml"Then keep verbosity only in tox or vice versa.
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (1)
337-357: Minor: also assert run indices map to generated args.Optional, but strengthens the coupling between i and args.
for i, result in enumerate(results): self.assertIn("success", result) self.assertTrue(result["success"], "Expected success=True for each run") self.assertIn("args", result) # Test that we got the expected arguments from our mock args = result["args"] self.assertIsInstance(args, dict) + self.assertEqual(result["run"], i + 1) self.assertEqual(args["name"], f"test_{i}") self.assertEqual(args["count"], i) self.assertEqual(args["enabled"], i % 2 == 0)tox.ini (1)
10-21: Align verbosity/coverage with pyproject addopts to avoid conflicting flags.Currently defaults here (
-vv) can conflict with addopts (-q). Pick one place for verbosity.Example:
-commands = - pytest {posargs:-vv --cov=mcp_fuzzer --cov-report=term-missing} +commands = + pytest {posargs}Let pyproject drive defaults.
tests/README.md (3)
9-24: Add language to fenced code block.Fixes MD040 warning; use “text” for directory trees.
-``` +```text tests/ ├── unit/ # Unit tests for individual components ... -``` +```
99-105: Tighten phrasing per grammar hint.Minor style polish.
-Integration tests typically: +Integration tests typically do the following: - Test the interaction between two or more components - Test end-to-end functionality - May use real or simulated external services
32-36: Consider showing component-marked examples to reinforce new workflow.Add
-mexamples to the “Run all tests”/tox snippets for discoverability.Example:
pytest # or tox -e tests +# run only fuzz_engine unit tests +pytest -m "unit and fuzz_engine" +# or via tox +tox -e tests -- -m "unit and fuzz_engine"Also applies to: 41-52, 66-71
tests/unit/fuzz_engine/test_executor.py (1)
182-199: Flakiness risk: timing-based concurrency assertion.Rare CI jitter could cause >2 starts before first end. Consider asserting semaphore-bound parallelism via injected probe instead of sleeps.
Sketch:
- first_end_idx = next(i for i, event in enumerate(execution_order) if event.endswith("_end")) - starts_before_any_end = execution_order[:first_end_idx] - self.assertLessEqual(len(starts_before_any_end), 2) + # Assert no more than 2 concurrent “_start” without a matching “_end” + in_flight = 0 + peak = 0 + for e in execution_order: + in_flight += 1 if e.endswith("_start") else -1 + peak = max(peak, in_flight) + self.assertLessEqual(peak, 2)tests/integration/test_client_transport.py (2)
20-25: Initialization handshake will trigger an extra POST; simplify by skipping it.Set transport._initialized to True in setUp to focus this test on a single request path.
self.transport = StreamableHTTPTransport(self.base_url) self.client = ProtocolClient(self.transport) + # Skip handshake to avoid mocking multiple requests + self.transport._initialized = True
42-43: Unnecessary unittest main guard for pytest-run tests.Remove or replace to avoid accidental direct execution.
-if __name__ == "__main__": - unittest.main() +if __name__ == "__main__": + raise SystemExit("Run this module with pytest")tests/unit/fuzz_engine/runtime/test_manager.py (1)
1-6: Trim unused imports to reduce noise.signal/time/call appear unused; consider removing to keep tests tidy.
-import asyncio -import os -import signal -import subprocess -import time -from unittest.mock import patch, MagicMock, AsyncMock, call +import asyncio +import os +import subprocess +from unittest.mock import patch, MagicMock, AsyncMocktests/add_markers.py (3)
59-65: Docstring detection supports only triple double quotes.Support both '''...''' and """...""" to place imports/markers correctly.
- module_header_end = re.search(r'""".*?"""\s*', content, re.DOTALL) + module_header_end = re.search(r'("""|\'\'\').*?\1\s*', content, re.DOTALL)
117-123: Mirror the broader docstring regex here too.- module_header_end = re.search(r'""".*?"""\s*', content, re.DOTALL) + module_header_end = re.search(r'("""|\'\'\').*?\1\s*', content, re.DOTALL)
51-55: Idempotency: existing pytestmark may miss new component markers.Current early-return skips adding missing markers if any pytestmark exists. Optionally merge missing markers instead of returning.
tests/conftest.py (2)
13-17: Register component markers to avoid PytestUnknownMarkWarning.If not already in pyproject.toml, add them here.
def pytest_configure(config): """Configure pytest with custom component selection logic.""" config.addinivalue_line("markers", "unit: mark test as a unit test") config.addinivalue_line("markers", "integration: mark test as an integration test") + for m in [ + "auth", + "cli", + "client", + "config", + "fuzz_engine", + "fuzzer", + "runtime", + "strategy", + "safety_system", + "transport", + ]: + config.addinivalue_line("markers", f"{m}: component marker")
33-55: Make changed-only base ref configurable for CI.Allow overriding the diff base via env to work on detached HEAD builds.
- try: - # Get changed files from git - result = subprocess.run( - ["git", "diff", "--name-only", "HEAD"], + try: + # Get changed files from git (allow base override for CI) + base = os.environ.get("PYTEST_CHANGED_BASE", "HEAD") + result = subprocess.run( + ["git", "diff", "--name-only", base], capture_output=True, text=True, check=True, ).github/workflows/component-tests.yml (5)
23-26: Enable pip cache for faster runs.Leverage setup-python’s built-in caching.
- name: Set up Python uses: actions/setup-python@v5 with: python-version: '3.11' + cache: 'pip'
27-32: Consider installing test extras in one shot.If the project defines a
[project.optional-dependencies].test(orextras_require['test']), preferpip install -e ".[test]"to ensure plugins/tools stay in sync. Otherwise current commands are fine.
6-12: Optional: trigger when test/config files change.Right now the workflow won’t run if only tests or pytest config change. If desired, expand paths.
paths: - 'mcp_fuzzer/auth/**' - 'mcp_fuzzer/cli/**' - 'mcp_fuzzer/client/**' - 'mcp_fuzzer/fuzz_engine/**' - 'mcp_fuzzer/safety_system/**' - 'mcp_fuzzer/transport/**' + # Optional: ensure component tests run when only tests/config change + - 'tests/**' + - 'pyproject.toml' + - 'tox.ini'
1-5: Optional: cancel in-flight runs on new commits.Add workflow-level concurrency to save CI minutes.
Add at the top-level (outside jobs):
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true
36-45: Use PR SHAs and anchored grep for reliable diffs
- In .github/workflows/component-tests.yml (lines 36–45), replace
BASE_BRANCH=${{ github.base_ref }}
with
BASE_SHA=${{ github.event.pull_request.base.sha }}
HEAD_SHA=${{ github.event.pull_request.head.sha }}(baeldung.com)- Change each diff invocation to
git diff --name-only "$BASE_SHA" "$HEAD_SHA"
instead oforigin/$BASE_BRANCH HEAD(github.com)- Anchor grep patterns to the path start, e.g.
grep -q "^mcp_fuzzer/auth/"(phoenixnap.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.github/workflows/component-tests.yml(1 hunks).github/workflows/tests.yml(1 hunks)pyproject.toml(1 hunks)tests/README.md(1 hunks)tests/add_markers.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_client_transport.py(1 hunks)tests/unit/auth/test_auth.py(2 hunks)tests/unit/config/test_config_loader.py(7 hunks)tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py(9 hunks)tests/unit/fuzz_engine/runtime/test_manager.py(33 hunks)tests/unit/fuzz_engine/runtime/test_runtime.py(11 hunks)tests/unit/fuzz_engine/test_executor.py(7 hunks)tests/unit/safety_system/test_policy.py(1 hunks)tox.ini(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
tests/integration/test_client_transport.py (2)
mcp_fuzzer/client/protocol_client.py (1)
ProtocolClient(19-399)mcp_fuzzer/transport/streamable_http.py (1)
StreamableHTTPTransport(41-370)
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (2)
mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py (1)
fuzz_tool_arguments(54-61)mcp_fuzzer/fuzz_engine/fuzzer/tool_fuzzer.py (1)
ToolFuzzer(21-257)
tests/unit/safety_system/test_policy.py (1)
mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)configure_network_policy(26-66)
tests/unit/fuzz_engine/runtime/test_runtime.py (3)
tests/unit/fuzz_engine/runtime/test_manager.py (1)
test_start_process_success(27-46)mcp_fuzzer/fuzz_engine/runtime/watchdog.py (5)
WatchdogConfig(21-28)start(43-48)register_process(239-258)unregister_process(260-268)stop(50-57)mcp_fuzzer/fuzz_engine/runtime/manager.py (3)
ProcessManager(35-415)get_process_status(182-198)ProcessConfig(23-32)
tests/unit/fuzz_engine/runtime/test_manager.py (1)
mcp_fuzzer/fuzz_engine/runtime/manager.py (7)
start_process(46-95)ProcessConfig(23-32)stop_process(97-127)get_process_status(182-198)list_processes(200-213)wait_for_process(215-233)cleanup_finished_processes(256-274)
tests/unit/config/test_config_loader.py (2)
mcp_fuzzer/config_loader.py (2)
find_config_file(20-60)apply_config_file(102-133)mcp_fuzzer/config.py (1)
update(100-102)
tests/unit/fuzz_engine/test_executor.py (1)
mcp_fuzzer/fuzz_engine/executor.py (3)
execute_with_retry(97-151)execute_batch(153-199)_execute_and_track(201-218)
🪛 LanguageTool
tests/README.md
[grammar] ~101-~101: There might be a mistake here.
Context: ...n` marker. Integration tests typically: - Test the interaction between two or more...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
tests/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (14)
tests/unit/auth/test_auth.py (1)
12-12: LGTM on pytest import.
Import is correct and keeps deps minimal..github/workflows/tests.yml (1)
25-25: Coverage XML generation looks good.
Command will emit ./coverage.xml for Codecov. No issues.tests/unit/safety_system/test_policy.py (1)
52-62: Whitespace-only change — OK.
No functional impact; tests remain readable.tests/unit/config/test_config_loader.py (1)
24-56: Formatting-only edits — OK.
No behavioral changes detected; tests still exercise config discovery and application paths.Also applies to: 62-77, 99-107, 116-124, 126-134, 136-151, 152-159
tests/unit/fuzz_engine/runtime/test_runtime.py (1)
26-48: Track and unskip start_process test.
The skip is reasonable short-term; please add an issue reference or TODO with a target date to unskip. When ready, patch asyncio.create_subprocess_exec and assert register_process was called.Would you like me to draft a minimal unskipped test using AsyncMock for create_subprocess_exec and asserting watchdog.register_process interactions?
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (3)
161-164: LGTM on safety patching scope.Patching
is_safe_tool_callat the module path is correct for isolating schema-focused behavior.
182-190: LGTM on deterministic sanitize mock.Predictable sanitized args make assertions robust here.
315-321: Nice deterministic inputs for “different runs” test.Good use of side_effect sequence to assert variability.
tox.ini (1)
20-20: LGTM: posargs passthrough with sensible defaults.This makes CI/local overrides easier while preserving coverage defaults.
tests/unit/fuzz_engine/test_executor.py (2)
93-107: LGTM: retries test accurately asserts attempts = retries + 1.Clear coverage of “all fail” path.
218-234: LGTM: shutdown covers cancellation path and leaves only completed tasks tracked.Good coverage of graceful shutdown with running tasks.
tests/unit/fuzz_engine/runtime/test_manager.py (2)
26-47: Process start path test looks solid.Mocks and assertions align with start_process: watchdog.start, register_process, and status “running” are verified.
349-373: Timeout handling assertion matches manager.wait_for_process semantics.Asserting None on timeout is consistent with returning current returncode when asyncio.TimeoutError is raised.
tests/conftest.py (1)
71-76: Skip logic is clear and minimal. LGTM.
| markers = [] | ||
| for path_prefix, path_markers in COMPONENT_MARKERS.items(): | ||
| if parent_dir.startswith(path_prefix): | ||
| markers = path_markers | ||
| break | ||
|
|
||
| if not markers: | ||
| print(f"Could not determine markers for {file_path}") | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefix shadowing: choose the most specific (longest) directory match.
As written, “unit/fuzz_engine” preempts “unit/fuzz_engine/runtime”. Pick the longest matching prefix to apply runtime/fuzzer/strategy markers correctly.
- markers = []
- for path_prefix, path_markers in COMPONENT_MARKERS.items():
- if parent_dir.startswith(path_prefix):
- markers = path_markers
- break
+ markers = []
+ # Choose the most specific (longest) matching prefix
+ best_prefix = None
+ for path_prefix in COMPONENT_MARKERS.keys():
+ if parent_dir.startswith(path_prefix) and (
+ best_prefix is None or len(path_prefix) > len(best_prefix)
+ ):
+ best_prefix = path_prefix
+ if best_prefix:
+ markers = COMPONENT_MARKERS[best_prefix]📝 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.
| markers = [] | |
| for path_prefix, path_markers in COMPONENT_MARKERS.items(): | |
| if parent_dir.startswith(path_prefix): | |
| markers = path_markers | |
| break | |
| if not markers: | |
| print(f"Could not determine markers for {file_path}") | |
| return False | |
| markers = [] | |
| # Choose the most specific (longest) matching prefix | |
| best_prefix = None | |
| for path_prefix in COMPONENT_MARKERS.keys(): | |
| if parent_dir.startswith(path_prefix) and ( | |
| best_prefix is None or len(path_prefix) > len(best_prefix) | |
| ): | |
| best_prefix = path_prefix | |
| if best_prefix: | |
| markers = COMPONENT_MARKERS[best_prefix] | |
| if not markers: | |
| print(f"Could not determine markers for {file_path}") | |
| return False |
🤖 Prompt for AI Agents
In tests/add_markers.py around lines 37 to 45, the loop picks the first
path_prefix match which causes shorter prefixes (e.g. "unit/fuzz_engine") to
shadow longer, more specific ones ("unit/fuzz_engine/runtime"); change the logic
to find the longest matching prefix: iterate all COMPONENT_MARKERS keys, for
each if parent_dir.startswith(path_prefix) check its length and keep the
path_markers from the longest matching prefix seen so far (track best_length),
then after the loop use that best match's markers (leave the existing "if not
markers" error path intact).
Add JSON Schema parser and property-based invariants system
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/integration/test_client_transport.py (1)
6-13: Resolved: missing httpx/json imports from prior review.The added imports address the earlier failures and align with the mocking strategy.
🧹 Nitpick comments (4)
tests/integration/test_client_transport.py (4)
33-37: Optionally attach a Request to the mocked Response for richer diagnostics.Not required for success, but setting Response.request improves error messages if the test ever fails on raise_for_status().
mock_post.return_value = httpx.Response( 200, headers={"Content-Type": "application/json"}, - content=b'{"jsonrpc":"2.0","id":"1","result":"test"}', + content=b'{"jsonrpc":"2.0","id":"1","result":"test"}', + request=httpx.Request("POST", self.base_url), )
6-12: Trim unused imports (nit).asyncio, json, and MagicMock aren’t used.
-import asyncio -import json import unittest -from unittest.mock import MagicMock, patch +from unittest.mock import patch @@ import httpx import pytest
1-1: Shebang is unnecessary in test modules (nit).Tests are executed by pytest, not as executables.
-#!/usr/bin/env python3
49-50: Drop the unittest main guard (nit).Pytest will discover and run the test; this block is redundant and can confuse execution paths.
-if __name__ == "__main__": - unittest.main()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/component-tests.yml(1 hunks)pyproject.toml(1 hunks)tests/add_markers.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_client_transport.py(1 hunks)tests/unit/auth/test_auth.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pyproject.toml
- tests/unit/auth/test_auth.py
- tests/add_markers.py
- tests/conftest.py
- .github/workflows/component-tests.yml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/test_client_transport.py (1)
mcp_fuzzer/client/protocol_client.py (1)
_send_generic_request(389-395)
🔇 Additional comments (1)
tests/integration/test_client_transport.py (1)
16-16: Good use of component markers for selective runs.This aligns with the PR goal to organize and run tests per component.
| # Verify the transport was used correctly | ||
| mock_post.assert_called_once() | ||
| self.assertEqual(result, {"result": "test"}) |
There was a problem hiding this comment.
Account for the implicit initialize handshake: expect two POSTs and assert methods.
StreamableHTTPTransport performs an initialize before non-initialize calls, so post() is invoked twice. The current assert_called_once() will fail nondeterministically.
- mock_post.assert_called_once()
+ # initialize + the actual request
+ self.assertEqual(mock_post.call_count, 2)
+ first_call, second_call = mock_post.call_args_list
+ self.assertEqual(first_call.kwargs["json"]["method"], "initialize")
+ self.assertEqual(second_call.kwargs["json"]["method"], "test_method")
self.assertEqual(result, {"result": "test"})📝 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.
| # Verify the transport was used correctly | |
| mock_post.assert_called_once() | |
| self.assertEqual(result, {"result": "test"}) | |
| # Verify the transport was used correctly | |
| # initialize + the actual request | |
| self.assertEqual(mock_post.call_count, 2) | |
| first_call, second_call = mock_post.call_args_list | |
| self.assertEqual(first_call.kwargs["json"]["method"], "initialize") | |
| self.assertEqual(second_call.kwargs["json"]["method"], "test_method") | |
| self.assertEqual(result, {"result": "test"}) |
🤖 Prompt for AI Agents
In tests/integration/test_client_transport.py around lines 44-46, the test
incorrectly assumes only one POST call even though StreamableHTTPTransport
performs an initialize handshake and will call post() twice; update the
assertions to expect two POSTs (e.g., assert call_count == 2) and replace
assert_called_once() with assertions that verify both POSTs occurred and their
order/arguments as needed (use call_count and assert_has_calls or inspect
mock_post.call_args_list to check the first is the init handshake and the second
is the actual request), then keep the result equality assertion.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (3)
309-313: Type hint mismatch: server_response can be a list._send_fuzzed_request may return a list (batch) but the annotation claims Dict[str, Any]. This leaks into callers and TypedDicts and will trip static analysis.
Apply:
- ) -> Tuple[Optional[Dict[str, Any]], Optional[str]]: + ) -> Tuple[Optional[Union[Dict[str, Any], List[Any]]], Optional[str]]:
347-349: Align create_fuzz_result signature with batch responses.server_response is documented/used as either dict or list. Update annotation accordingly.
Apply:
- server_response: Optional[Dict[str, Any]], + server_response: Optional[Union[Dict[str, Any], List[Any]]],
342-372: Add missing TypedDict keys for FuzzDataResult
In mcp_fuzzer/types.py, FuzzDataResult currently omitsinvariant_violations(andpassed_invariantsif you choose to track it), yet both are written into the result in protocol_fuzzer.py. Extend the TypedDict with:class FuzzDataResult(TypedDict, total=False): … invariant_violations: List[str] passed_invariants: List[str] # add only if useddocs/architecture.md (1)
321-339: Duplicate section numbering (two “### 6”).“Authentication System” is numbered 6 but follows “Safety System” (also 6). Renumber to keep sequence consistent.
Apply:
-### 6. Authentication System +### 7. Authentication System @@ -### 7. Reporting System +### 8. Reporting System
🧹 Nitpick comments (36)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (8)
32-49: allOf merge is shallow; merge type constraints too.Merging only properties/required may yield invalid composites. Intersect type constraints across subschemas.
def _merge_allOf(schemas: List[Dict[str, Any]]) -> Dict[str, Any]: """Deep merge for allOf schemas.""" merged: Dict[str, Any] = {} props: Dict[str, Any] = {} required: List[str] = [] + merged_types: "set[str] | None" = None for s in schemas: if "properties" in s and isinstance(s["properties"], dict): props.update(s["properties"]) if "required" in s and isinstance(s["required"], list): required.extend([r for r in s["required"] if isinstance(r, str)]) + t = s.get("type") + if t: + tset = set(t if isinstance(t, list) else [t]) + merged_types = tset if merged_types is None else (merged_types & tset) for k, v in s.items(): if k not in ("properties", "required"): merged[k] = v if k not in merged else merged[k] if props: merged["properties"] = props if required: merged["required"] = sorted(set(required)) + if merged_types: + merged["type"] = list(merged_types) if len(merged_types) > 1 else next(iter(merged_types)) return merged
68-71: Type-aware recursion cutoff.Returning {} or [] blindly may violate schema types. Make the guard respect the declared type.
- # Prevent excessive recursion - if recursion_depth > MAX_RECURSION_DEPTH: - return {} if random.random() < 0.5 else [] + # Prevent excessive recursion + if recursion_depth > MAX_RECURSION_DEPTH: + t = schema.get("type") + return { + "object": {}, + "array": [], + "string": "", + "integer": 0, + "number": 0.0, + "boolean": False, + "null": None, + }.get(t, None)
171-178: Honor property defaults in realistic mode.If a property schema defines "default", prefer it sometimes to improve validity.
- for prop_name, prop_schema in properties.items(): + for prop_name, prop_schema in properties.items(): # For required properties or by chance for optional ones if prop_name in required or random.random() < 0.8: - result[prop_name] = make_fuzz_strategy_from_jsonschema( - prop_schema, phase, recursion_depth + 1 - ) + if phase == "realistic" and "default" in prop_schema and random.random() < 0.5: + result[prop_name] = prop_schema["default"] + else: + result[prop_name] = make_fuzz_strategy_from_jsonschema( + prop_schema, phase, recursion_depth + 1 + )
188-199: JS prototype-pollution keys — intentional? Gate behind a flag.Injecting "proto"/"constructor" is useful for security fuzzing, but can corrupt downstream JS consumers. Consider a toggle (e.g., aggressive_js_object_keys=True) or a marker to enable/disable.
205-215: Add tuple validation support for items as a list.JSON Schema allows items to be an array (positional schemas). Minimal support improves coverage.
def _handle_array_type( @@ - items_schema = schema.get("items", {}) + items_schema = schema.get("items", {}) @@ - # If this is an array property without items specification, + # Tuple validation: items is a list of schemas + if isinstance(items_schema, list): + return [ + make_fuzz_strategy_from_jsonschema(sub, phase, recursion_depth + 1) + for sub in items_schema + ] + + # If this is an array property without items specification, # generate an array of simple values if not items_schema: return [_generate_default_value(phase) for _ in range(random.randint(1, 3))]Also applies to: 246-251
23-27: uniqueItems handling: use a stable JSON hash, not str(item).str() can collide and misses structural differences. Use json.dumps(sort_keys=True).
@@ -import random +import random +import json @@ - item_hash = str(item) + item_hash = json.dumps(item, sort_keys=True, default=str) attempts = 0 - while item_hash in seen_values and attempts < 10: + while item_hash in seen_values and attempts < 10: item = make_fuzz_strategy_from_jsonschema( items_schema, phase, recursion_depth + 1 ) - item_hash = str(item) + item_hash = json.dumps(item, sort_keys=True, default=str) attempts += 1 seen_values.add(item_hash)Also applies to: 256-265
575-605: Aggressive numbers: sometimes return NaN/Inf even if out-of-bounds.Current range-filtering drops NaN/Inf. Keep a small chance to emit them to exercise parsers.
else: # In aggressive mode, sometimes use edge cases - if random.random() < 0.7: + if random.random() < 0.7: # Normal value return random.uniform(minimum, maximum) else: # Edge cases - edge_cases = [ + # 25%: special IEEE-754 values irrespective of bounds + if random.random() < 0.25: + return random.choice([float("inf"), float("-inf"), float("nan")]) + edge_cases = [ minimum, # Minimum value maximum, # Maximum value 0.0, # Zero -0.0, # Negative zero - float("inf"), # Infinity - float("-inf"), # Negative infinity - float("nan"), # Not a number ]
284-325: Regex pattern generation is naive — document limitations or plug a generator.Consider integrating exrex/rstr/graph-based generators under an optional dependency to better satisfy complex patterns; otherwise, document this limitation in docs.
mcp_fuzzer/fuzz_engine/invariants.py (3)
85-96: Requests vs. responses: enforce expectations or rename API.check_response_validity currently accepts request objects (has method and id) as “valid responses.” Either reject them or rename to reflect “envelope validity.”
- # Determine if this is a notification, request, or response + # Determine if this is a notification, request, or response @@ - # If it has id but no method, it's a response and needs result or error + # If it has id but no method, it's a response and needs result or error if not has_result and not has_error and has_id and not has_method: # Responses must have result or error raise InvariantViolation( "JSON-RPC response must have either 'result' or 'error'", response, ) + # If it has both method and id, treat as a request; this checker expects responses + if has_method and has_id and not (has_result or has_error): + raise InvariantViolation( + "Received a JSON-RPC request where a response was expected", response + )
211-213: jsonschema optional dependency — make desired behavior explicit.Currently we warn and return True. Consider a strict flag to fail fast when schema checks are requested but jsonschema isn’t installed.
Would you like a PR follow-up adding a
strict_schema_validationtoggle to raise instead of warn?
324-345: State consistency: allow explicit additions/removals lists.Support expected_additions/expected_removals to model create/delete flows without overloading expected_changes.
docs/reference.md (2)
188-197: Document JSON-RPC strictness and limitations.Mention that invariants treat non-JSON-RPC dicts as invalid and require id/result|error for responses; add a note on optional jsonschema dependency.
528-536: Avoid real-looking secret prefixes in examples.Use neutral placeholders to prevent scanners from flagging docs.
- "api_key": "sk-your-openai-api-key", + "api_key": "YOUR_OPENAI_API_KEY", @@ - "api_key": "ghp-your-github-token", + "api_key": "YOUR_GITHUB_TOKEN",mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (2)
220-252: Tighten batch validation call and make timeout a constant.Minor cleanups:
- Avoid creating an intermediate task; wait_for can take the coroutine directly.
- Hoist the 5s literal into a named constant for configurability.
Apply:
@@ - try: - # Handle batch responses with timeout to prevent hanging - batch = await asyncio.wait_for( - asyncio.create_task( - verify_batch_responses(server_response) - ), - timeout=5.0, # 5 second timeout for batch validation - ) + try: + # Handle batch responses with timeout to prevent hanging + batch = await asyncio.wait_for( + verify_batch_responses(server_response), + timeout=self.BATCH_VALIDATION_TIMEOUT, + )And near the class constants (after PROTOCOL_TYPES), add:
@@ class ProtocolFuzzer: PROTOCOL_TYPES: ClassVar[Tuple[str, ...]] = ( "InitializeRequest", ... "CompleteRequest", ) + # Seconds to wait for invariant validation of batch responses + BATCH_VALIDATION_TIMEOUT: ClassVar[float] = 5.0
363-372: Clarify success semantics vs invariant failures.Currently success reflects transport acceptance only. Consider exposing a derived flag to simplify consumers.
Apply:
return { "protocol_type": protocol_type, "run": run_index + 1, "fuzz_data": fuzz_data, "success": server_error is None, "server_response": server_response, "server_error": server_error, "server_rejected_input": server_error is not None, "invariant_violations": [], # Will be populated if violations occur + # Derived convenience flag for report filtering + # "passed_invariants" will be backfilled by caller if violations found + "passed_invariants": True, }And when setting violations:
- if invariant_violations: - result["invariant_violations"] = invariant_violations + if invariant_violations: + result["invariant_violations"] = invariant_violations + result["passed_invariants"] = Falsedocs/architecture.md (3)
247-258: Minor wording polish in Fuzzing Engine bullets and flow steps.Tighten phrasing and fix small grammar nits.
Apply:
-- `invariants.py`: Implements property-based invariants and checks for fuzz testing +- `invariants.py`: Implements property-based invariants and checks for fuzz testing. @@ -5. **Invariant Verification**: Verify responses against property-based invariants -6. **Analysis**: Analyze results and generate reports +5. **Invariant Verification**: Verify responses against property-based invariants. +6. **Analysis**: Analyze results and generate reports.
275-288: Capitalize “JSON Schema” and fix list punctuation.Improve consistency and readability.
Apply:
-The schema parser provides comprehensive support for parsing JSON Schema definitions and generating appropriate test data based on schema specifications. It handles: +The schema parser provides comprehensive support for parsing JSON Schema definitions and generating appropriate test data based on schema specifications. It handles: @@ -- Basic types: string, number, integer, boolean, array, object, null -- String constraints: minLength, maxLength, pattern, format -- Number/Integer constraints: minimum, maximum, exclusiveMinimum, exclusiveMaximum, multipleOf -- Array constraints: minItems, maxItems, uniqueItems -- Object constraints: required properties, minProperties, maxProperties -- Schema combinations: oneOf, anyOf, allOf -- Enums and constants +- Basic types: string, number, integer, boolean, array, object, null. +- String constraints: minLength, maxLength, pattern, format. +- Number/integer constraints: minimum, maximum, exclusiveMinimum, exclusiveMaximum, multipleOf. +- Array constraints: minItems, maxItems, uniqueItems. +- Object constraints: required properties, minProperties, maxProperties. +- Schema combinations: oneOf, anyOf, allOf. +- Enums and constants.
291-303: Invariants System bullets: grammar consistency.Add articles and end punctuation.
Apply:
-- `check_response_validity`: Ensures responses follow JSON-RPC 2.0 specification -- `check_error_type_correctness`: Verifies error responses have correct structure and codes -- `check_response_schema_conformity`: Validates responses against JSON schema definitions -- `verify_response_invariants`: Orchestrates multiple invariant checks on a single response -- `verify_batch_responses`: Applies invariant checks to batches of responses -- `check_state_consistency`: Ensures server state remains consistent during fuzzing +- `check_response_validity`: Ensures responses follow the JSON-RPC 2.0 specification. +- `check_error_type_correctness`: Verifies that error responses have the correct structure and codes. +- `check_response_schema_conformity`: Validates responses against JSON Schema definitions. +- `verify_response_invariants`: Orchestrates multiple invariant checks on a single response. +- `verify_batch_responses`: Applies invariant checks to batches of responses. +- `check_state_consistency`: Ensures server state remains consistent during fuzzing.tests/test_schema_parser.py (5)
10-19: Avoid testing private helpers directly where possible.Importing underscored functions ties tests to internals and increases churn. Prefer exercising behavior via make_fuzz_strategy_from_jsonschema; keep a minimal smoke test on internals only where the public path can’t cover it.
25-53: Seed randomness for stability (optional).Many assertions check ranges/types and are fine, but seeding reduces flaky edge cases across environments.
Apply:
@@ -import unittest +import unittest +import random @@ -class TestSchemaParser(unittest.TestCase): +class TestSchemaParser(unittest.TestCase): """Test cases for schema_parser.py - BEHAVIOR focused.""" + @classmethod + def setUpClass(cls): + random.seed(12345)
144-154: Stronger URI validation.Instead of startswith checks, validate scheme via urlparse to avoid false positives.
Apply:
@@ - self.assertTrue( - uri_result.startswith("http://") or uri_result.startswith("https://"), - "URI should start with http:// or https://", - ) + from urllib.parse import urlparse + self.assertIn(urlparse(uri_result).scheme, ("http", "https"), "Invalid URI scheme")
291-400: Strengthen format validations in complex schema.Add concrete checks for UUID and RFC3339 date-time when present.
Apply:
@@ - # Check id format - self.assertIsInstance(result["id"], str, "id should be a string") + # Check id format (UUID-like) + self.assertIsInstance(result["id"], str, "id should be a string") + try: + import uuid as _uuid + _ = _uuid.UUID(result["id"]) + except Exception: + self.fail("id is not a valid UUID string") @@ - if "created" in result["metadata"]: - self.assertIsInstance( - result["metadata"]["created"], str, "created should be a string" - ) + if "created" in result["metadata"]: + self.assertIsInstance(result["metadata"]["created"], str, "created should be a string") + # Best-effort ISO 8601/RFC3339 validation + from datetime import datetime + try: + datetime.fromisoformat(result["metadata"]["created"].replace("Z", "+00:00")) + except Exception: + self.fail("created is not a valid date-time string")
401-403: Add coverage for multipleOf constraints.Covers common schema constraint paths for both integers and numbers.
Apply:
@@ if __name__ == "__main__": - unittest.main() + unittest.main() + +class TestSchemaParserConstraints(unittest.TestCase): + def test_integer_multiple_of(self): + schema = {"type": "integer", "minimum": 0, "maximum": 100, "multipleOf": 7} + val = _handle_integer_type(schema, phase="realistic") + self.assertIsInstance(val, int) + self.assertEqual(val % 7, 0) + self.assertGreaterEqual(val, 0) + self.assertLessEqual(val, 100) + + def test_number_multiple_of(self): + schema = {"type": "number", "minimum": 0.0, "maximum": 10.0, "multipleOf": 2.5} + val = _handle_number_type(schema, phase="realistic") + self.assertTrue(isinstance(val, (int, float))) + # Use tolerance due to float + self.assertLess(abs((val / 2.5) - round(val / 2.5)), 1e-9) + self.assertGreaterEqual(val, 0.0) + self.assertLessEqual(val, 10.0)mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py (3)
149-157: Don't swallow schema-parser errors silently; at least log for observability.Catching a blanket Exception and defaulting to {} obscures real parser defects and makes failures hard to diagnose in CI.
Consider:
- try: - args = make_fuzz_strategy_from_jsonschema(schema, phase="realistic") - except Exception: - args = {} + try: + args = make_fuzz_strategy_from_jsonschema(schema, phase="realistic") + except Exception as e: + # Optional: add module-level logger if not already present + # logger.debug("Schema parser failed in realistic phase: %s", e) + args = {}
204-213: Respect exclusiveMinimum/Maximum and multipleOf when present.Current integer/number generation ignores these common JSON Schema constraints.
Example patch for integers:
- minimum = prop_spec.get("minimum", -100) - maximum = prop_spec.get("maximum", 100) - args[prop_name] = random.randint(minimum, maximum) + minimum = prop_spec.get("minimum", -100) + maximum = prop_spec.get("maximum", 100) + if prop_spec.get("exclusiveMinimum") is True: + minimum += 1 + if prop_spec.get("exclusiveMaximum") is True: + maximum -= 1 + step = prop_spec.get("multipleOf") + val = random.randint(minimum, maximum) + if isinstance(step, int) and step > 0: + val = minimum + ((val - minimum) // step) * step + args[prop_name] = valSimilar logic can be applied to numbers using uniform() and snapping to multipleOf.
233-236: Emit RFC 3339 UTC timestamps with trailing 'Z' to match docstring.Docstring says “ending with Z” but isoformat() yields “+00:00”.
Apply:
- elif format_type == "date-time": - args[prop_name] = datetime.now(timezone.utc).isoformat() + elif format_type == "date-time": + args[prop_name] = ( + datetime.now(timezone.utc) + .isoformat(timespec="seconds") + .replace("+00:00", "Z") + )tests/test_invariants.py (1)
6-18: Add component markers to integrate with component-based test selection.Aligns with the PR goal to organize and run tests per component.
Add at module top (after imports):
+pytestmark = [pytest.mark.unit, pytest.mark.fuzz_engine]mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
256-286: Optionally backfill required fields even in aggressive mode to reach deeper code paths.Including required keys (with intentionally malformed values) improves coverage while preserving “aggressive” intent.
Example:
if not args and schema.get("properties"): # Fallback to basic property handling properties = schema.get("properties", {}) for prop_name, prop_spec in properties.items(): if random.random() < 0.8: # 80% chance to include each property prop_type = prop_spec.get("type", "string") @@ else: args[prop_name] = generate_aggressive_text() + + # Ensure required keys exist (values may still be adversarial) + required = schema.get("required", []) + for key in required or []: + if key not in args: + args[key] = generate_aggressive_text()tests/unit/fuzz_engine/strategy/test_realistic_strategies.py (8)
7-14: Add missing import for URI validation (used in assertions below).You'll need urlparse for robust URI checks.
import uuid -from datetime import datetime +from datetime import datetime +from urllib.parse import urlparse
32-44: Strengthen Base64 validation (use strict decoder).Use validate=True to reject non-Base64 chars and catch padding issues.
- decoded = base64.b64decode(value) + decoded = base64.b64decode(value, validate=True)
72-84: Also assert UTC offset is zero.Confirms timestamps are truly UTC, not just “timezone present”.
parsed_dt = datetime.fromisoformat(value) assert isinstance(parsed_dt, datetime) # Should have timezone info assert parsed_dt.tzinfo is not None + # And it must be UTC + assert parsed_dt.utcoffset() is not None and parsed_dt.utcoffset().total_seconds() == 0
149-155: Check microseconds semantically, not by string search.Parsing avoids false positives due to formatting differences.
- # Should not contain microseconds (no .) - assert "." not in value + parsed = datetime.fromisoformat(value) + assert parsed.microsecond == 0
232-252: Also assert “scores” array element types.You define a number-typed array but don’t validate it. Add checks to ensure numeric types.
If this fails, production code likely doesn’t honor item type for arrays; see follow-up verification script below.
assert isinstance(result["numbers"], list) assert 1 <= len(result["numbers"]) <= 3 assert all(isinstance(num, int) for num in result["numbers"]) + assert isinstance(result["scores"], list) + assert 1 <= len(result["scores"]) <= 3 + assert all(isinstance(s, (int, float)) for s in result["scores"])
287-291: Fix tautological uniqueness check.len(set(texts)) >= 1 is always true. Require at least two distinct samples.
- assert len(set(texts)) >= 1 + assert len(set(texts)) > 1
295-297: Remove redundant re-imports inside tests.These functions are already imported at module level; local re-imports add noise.
- from mcp_fuzzer.fuzz_engine.strategy.realistic.tool_strategy import ( - base64_strings, - ) + # base64_strings imported at module level- from mcp_fuzzer.fuzz_engine.strategy.realistic.tool_strategy import ( - uuid_strings, - ) + # uuid_strings imported at module level- from mcp_fuzzer.fuzz_engine.strategy.realistic.tool_strategy import ( - timestamp_strings, - ) + # timestamp_strings imported at module levelAlso applies to: 318-319, 352-354
309-313: Alphabet parameter is a no-op in production; either remove this sub-test or update generator to honor it.As written, base64_strings ignores alphabet; this test implies otherwise.
Would you prefer I open a follow-up PR to make base64_strings use the alphabet for pre-encoding data?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
docs/architecture.md(3 hunks)docs/reference.md(2 hunks)mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py(3 hunks)mcp_fuzzer/fuzz_engine/invariants.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/schema_parser.py(1 hunks)tests/test_invariants.py(1 hunks)tests/test_schema_parser.py(1 hunks)tests/unit/fuzz_engine/strategy/test_realistic_strategies.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/test_schema_parser.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (8)
make_fuzz_strategy_from_jsonschema(52-128)_handle_enum(131-153)_handle_string_type(274-324)_handle_integer_type(462-527)_handle_number_type(530-604)_handle_boolean_type(607-627)_handle_array_type(204-271)_handle_object_type(156-201)
mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
make_fuzz_strategy_from_jsonschema(52-128)
tests/unit/fuzz_engine/strategy/test_realistic_strategies.py (2)
mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py (5)
base64_strings(19-42)timestamp_strings(78-102)uuid_strings(45-75)generate_realistic_text(105-137)fuzz_tool_arguments_realistic(140-243)mcp_fuzzer/fuzz_engine/strategy/realistic/protocol_type_strategy.py (3)
json_rpc_id_values(42-56)method_names(59-96)protocol_version_strings(16-39)
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
make_fuzz_strategy_from_jsonschema(52-128)
tests/test_invariants.py (1)
mcp_fuzzer/fuzz_engine/invariants.py (7)
InvariantViolation(41-47)check_response_validity(50-126)check_error_type_correctness(129-185)check_response_schema_conformity(188-212)verify_response_invariants(215-245)verify_batch_responses(248-302)check_state_consistency(305-345)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (1)
mcp_fuzzer/fuzz_engine/invariants.py (3)
verify_response_invariants(215-245)InvariantViolation(41-47)verify_batch_responses(248-302)
🪛 LanguageTool
docs/architecture.md
[grammar] ~247-~247: There might be a mistake here.
Context: ...d invariants and checks for fuzz testing - executor.py: Provides asynchronous execution framew...
(QB_NEW_EN)
[grammar] ~279-~279: There might be a mistake here.
Context: ...r, integer, boolean, array, object, null - String constraints: minLength, maxLength...
(QB_NEW_EN)
[grammar] ~280-~280: There might be a mistake here.
Context: ...s: minLength, maxLength, pattern, format - Number/Integer constraints: minimum, max...
(QB_NEW_EN)
[grammar] ~281-~281: There might be a mistake here.
Context: ...iveMinimum, exclusiveMaximum, multipleOf - Array constraints: minItems, maxItems, u...
(QB_NEW_EN)
[grammar] ~282-~282: There might be a mistake here.
Context: ...traints: minItems, maxItems, uniqueItems - Object constraints: required properties,...
(QB_NEW_EN)
[grammar] ~283-~283: There might be a mistake here.
Context: ...properties, minProperties, maxProperties - Schema combinations: oneOf, anyOf, allOf...
(QB_NEW_EN)
[grammar] ~284-~284: There might be a mistake here.
Context: ...Schema combinations: oneOf, anyOf, allOf - Enums and constants The module supports...
(QB_NEW_EN)
[grammar] ~295-~295: There might be a mistake here.
Context: ...ponses follow JSON-RPC 2.0 specification - check_error_type_correctness: Verifies error responses have correct ...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...sponses have correct structure and codes - check_response_schema_conformity: Validates responses against JSON schem...
(QB_NEW_EN)
[grammar] ~297-~297: There might be a mistake here.
Context: ...esponses against JSON schema definitions - verify_response_invariants: Orchestrates multiple invariant checks...
(QB_NEW_EN)
[grammar] ~298-~298: There might be a mistake here.
Context: ...le invariant checks on a single response - verify_batch_responses: Applies invariant checks to batches of...
(QB_NEW_EN)
[grammar] ~299-~299: There might be a mistake here.
Context: ...invariant checks to batches of responses - check_state_consistency: Ensures server state remains consisten...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (7)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (3)
17-21: Good addition: invariant checks are now first-class.Importing InvariantViolation, verify_response_invariants, and verify_batch_responses brings the runtime assertions into the fuzzer path cleanly.
258-261: Results now carry invariant_violations — nice.Attaching violations only when present avoids noise while keeping the field discoverable via the default. LGTM.
371-371: Default key in result shape is helpful for consumers.Including invariant_violations as a defaulted field stabilizes downstream parsing.
tests/test_invariants.py (3)
172-176: LGTM: covers the no-jsonschema path with logging assertion.Good negative-path coverage; this will prevent silent skips regressing unnoticed.
220-237: LGTM: async batch verification happy path.Solid use of patching and asyncio marker; asserts are precise.
239-256: LGTM: async batch verification error path.Effectively validates per-index error propagation and call counts.
tests/unit/fuzz_engine/strategy/test_realistic_strategies.py (1)
174-201: Overall: Solid, focused coverage for strategy and schema-driven paths.Nice breadth across formats, ranges, nested shapes, and edge cases.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/fuzz_engine/test_executor.py (1)
206-234: Shutdown semantics mismatch: tasks aren’t cancelled on timeout.
AsyncFuzzExecutor.shutdown()only waits; if it times out, it logs a warning and returns, leaving tasks running. This test expects cancellation, which is desirable for clean teardown.Proposed change to
mcp_fuzzer/fuzz_engine/executor.py:async def shutdown(self, timeout: float = 5.0) -> None: ... try: await asyncio.wait_for( asyncio.gather(*self._running_tasks, return_exceptions=True), timeout=timeout, ) except asyncio.TimeoutError: self._logger.warning( "Shutdown timed out with %d tasks still running", len(self._running_tasks), ) + # Proactively cancel outstanding tasks and wait for them to finish + for task in list(self._running_tasks): + task.cancel() + await asyncio.gather(*self._running_tasks, return_exceptions=True) + finally: + # Ensure the set is cleaned up + self._running_tasks = {t for t in self._running_tasks if not t.cancelled() and not t.done()}If you don’t want to change production code, relax the test to assert that shutdown returns and that tasks are either done or still pending (but that’s a weaker guarantee).
mcp_fuzzer/transport/sse.py (1)
26-40: Missing safety policy enforcement and header sanitization on network calls.Unlike HTTPTransport, SSETransport skips is_host_allowed and sanitize_headers; this can bypass network safety policy and leak denied headers.
Apply:
+from ..safety_system.policy import is_host_allowed, sanitize_headers @@ - async with httpx.AsyncClient(timeout=self.timeout) as client: - response = await client.post(self.url, json=payload, headers=self.headers) + async with httpx.AsyncClient(timeout=self.timeout) as client: + if not is_host_allowed(self.url): + raise Exception("Network to non-local host is disallowed by safety policy") + safe_headers = sanitize_headers(self.headers) + response = await client.post(self.url, json=payload, headers=safe_headers) @@ - async with httpx.AsyncClient(timeout=self.timeout) as client: - response = await client.post(self.url, json=payload, headers=self.headers) + async with httpx.AsyncClient(timeout=self.timeout) as client: + if not is_host_allowed(self.url): + raise Exception("Network to non-local host is disallowed by safety policy") + safe_headers = sanitize_headers(self.headers) + response = await client.post(self.url, json=payload, headers=safe_headers) @@ - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with httpx.AsyncClient(timeout=self.timeout) as client: + if not is_host_allowed(self.url): + raise Exception("Network to non-local host is disallowed by safety policy") + safe_headers = sanitize_headers(self.headers) async with client.stream( "POST", self.url, json=payload, - headers=self.headers, + headers=safe_headers, ) as response:Also applies to: 52-56, 66-73
mcp_fuzzer/transport/http.py (1)
126-129: Bug: _update_activity is async but not awaited.This silently drops the coroutine and skips activity updates.
- self._update_activity() + await self._update_activity()
♻️ Duplicate comments (1)
tests/unit/auth/test_auth.py (1)
27-27: Module-level pytest markers now correctly applied (pytest-style tests).Good move switching from unittest.TestCase to pytest functions; module-level pytestmark will work with -m selection as intended.
🧹 Nitpick comments (38)
tests/unit/safety_system/test_safety.py (10)
1-1: Remove unnecessary shebang in a test module.The file isn’t executed directly.
-#!/usr/bin/env python3
7-7: Drop unused import.
MagicMockisn’t used.-from unittest.mock import patch, MagicMock +from unittest.mock import patch
50-57: Make URL detection test deterministic and remove noisy prints.Domain-only
"example.com"may or may not match depending on default patterns; prefer an explicit scheme. Also remove"ftp://dangerous.net", "file:///etc/passwd", "www.evil.com", - "example.com", + "https://example.com", ] for url in dangerous_urls: result = safety_filter.contains_dangerous_url(url) - if not result: - print(f"URL not detected as dangerous: {url}") assert result, f"URL should be detected as dangerous: {url}"Also applies to: 61-63
98-103: Remove prints in command-pattern test.Assertions are sufficient; prints add noise on failure.
for command in dangerous_commands: result = safety_filter.contains_dangerous_command(command) - if not result: - print(f"Command not detected as dangerous: {command}") assert result, f"Command should be detected as dangerous: {command}"
143-144: Prefer public API over private helper in tests.Using
_sanitize_valueties tests to internals. Exercise behavior viasanitize_tool_argumentsfor stability.- result = safety_filter._sanitize_value("root", complex_value) + result = safety_filter.sanitize_tool_arguments("test_tool", complex_value)
105-117: Reduce reliance on private methods across tests.Calls to
_sanitize_string_argumentcouple tests to internals. Consider validating viasanitize_tool_arguments("tool", {"key": value})instead. Keep one targeted unit test for the private if truly needed.Also applies to: 120-131, 330-345, 347-359, 479-491
372-374: Fix misleading comment about recursion.
should_skip_tool_calldoes not recurse into nested dicts (by design); this case should not block.- # This should be checked because we recurse through nested structures + # This should NOT block: implementation checks only top-level strings and lists of strings.
499-499: Trim depth to keep unit tests fast (or mark as slow).Depth 100 is okay but unnecessary for coverage; 30–50 keeps signal high and runtime low.
- for i in range(100): + for i in range(50):
270-282: Assert a more specific logging call if feasible.Optionally assert the first warning equals
"Tool: test_tool"to make the test crisper and decouple from call_args serialization.
434-439: Add summary coverage (optional).Consider tests for
get_blocked_operations_summary()to validate counts by tool, reason, and content types after a few blocked operations.tests/unit/fuzz_engine/strategy/test_schema_parser.py (7)
6-7: Drop unused imports.
jsonand the typing imports aren’t used; remove to keep tests clean.Apply:
-import json -from typing import Any, Dict, List
11-20: Avoid coupling tests to private helpers.Importing and testing underscored functions ties tests to internals and increases maintenance. Prefer driving behavior via
make_fuzz_strategy_from_jsonschemawith schema fixtures that exercise those paths.
120-135: Strengthen format assertions.Email/URI checks are minimal. Consider validating with
email.utils.parseaddrorurllib.parse.urlparseto reduce false positives.
174-191: MaxProperties assertion may be misleading.Generator doesn’t enforce
maxProperties, but this test still passes only because defined props ≤ max. If enforcement is added later (or extra props appear), this can become brittle. Either:
- Assert only
minProperties, or- Add explicit enforcement in the generator and then assert it.
279-372: Great end-to-end complex schema test.Optional: assert UUID validity with
uuid.UUID(result["id"])and validatedate-timeusingdatetime.fromisoformattry/except.
24-24: Optional: seed RNG to reduce flakiness.If you’ve seen intermittent failures, seed
randomper-test.Apply:
+import random + +@pytest.fixture(autouse=True) +def _seed_rng(): + random.seed(0) + yield + random.seed(0)
372-372: Add constraint-focused cases (multipleOf, exclusives).These ensure numeric constraints are honored.
Apply:
+def test_integer_multiple_of_realistic(): + schema = {"type": "integer", "minimum": 0, "maximum": 100, "multipleOf": 7} + v = make_fuzz_strategy_from_jsonschema(schema, phase="realistic") + assert isinstance(v, int) + assert 0 <= v <= 100 + assert v % 7 == 0 + +def test_number_multiple_of_realistic(): + schema = {"type": "number", "minimum": 0.0, "maximum": 10.0, "multipleOf": 2.5} + v = make_fuzz_strategy_from_jsonschema(schema, phase="realistic") + assert isinstance(v, (int, float)) + assert 0.0 <= v <= 10.0 + # Allow small FP error + assert abs((v / 2.5) - round(v / 2.5)) < 1e-7 + +def test_integer_exclusive_bounds(): + schema = {"type": "integer", "minimum": 0, "exclusiveMinimum": True, "maximum": 3, "exclusiveMaximum": True} + v = make_fuzz_strategy_from_jsonschema(schema, phase="realistic") + assert v in (1, 2)tests/unit/fuzz_engine/test_executor.py (3)
21-37: Unnecessary asyncio mark.
test_initis synchronous; dropping@pytest.mark.asynciowill shave event-loop setup overhead.Apply:
-@pytest.mark.asyncio -async def test_init(): +def test_init():
30-35: Avoid relying on private semaphore internals.
_semaphore._valueis private and version-dependent. Prefer asserting configured values or exercising concurrency behavior instead.Apply:
- assert executor._semaphore._value == 5 + assert executor._max_concurrency == 5
173-197: Semaphore usage test doesn’t catch double-acquire in batch path.
execute_batchacquires a semaphore before callingexecute, which also acquires it—halving effective concurrency. Add a batch-focused test that asserts a single acquire per op, or remove the outer acquire inexecute_batch.Proposed fix in executor (outside this file):
async def execute_batch(...): - async def _bounded_execute_and_track(op, args, kwargs): - async with self._semaphore: - return await self._execute_and_track(op, args, kwargs) + async def _bounded_execute_and_track(op, args, kwargs): + # execute() already enforces the semaphore + return await self._execute_and_track(op, args, kwargs)tests/unit/fuzz_engine/test_invariants.py (3)
176-194: Caplog may miss the warning without level set.Ensure the module logger’s WARNING is captured.
Apply:
+import logging @@ - with patch("mcp_fuzzer.fuzz_engine.invariants.HAVE_JSONSCHEMA", False): + with patch("mcp_fuzzer.fuzz_engine.invariants.HAVE_JSONSCHEMA", False): + caplog.set_level(logging.WARNING) assert check_response_schema_conformity(response, schema)
120-124: Also test bool error code rejection.
True/Falsemust be rejected even though they are ints in Python.Apply:
def test_check_error_type_correctness_invalid_message_type(): @@ check_error_type_correctness(error) + +def test_check_error_type_correctness_bool_code(): + """Bool codes must be rejected (JSON-RPC requires integer, bool not allowed).""" + error = {"code": True, "message": "Invalid Request"} + with raises(InvariantViolation): + check_error_type_correctness(error)
310-310: Add guard-case tests for batch verifier.Cover
responses=Noneand non-list input.Apply at end of file:
+@pytest.mark.asyncio +async def test_verify_batch_responses_none_input(): + results = await verify_batch_responses(None) + assert results == {} + +@pytest.mark.asyncio +async def test_verify_batch_responses_non_list_input(): + results = await verify_batch_responses({"jsonrpc": "2.0"}) + assert 0 in results and "Expected a list of responses" in results[0]tests/unit/auth/test_auth.py (2)
295-338: Add negative test for invalid MCP_CUSTOM_HEADERS JSON.setup_auth_from_env silently ignores bad JSON; add a test to lock this behavior in and avoid regressions.
Example test to add:
def test_setup_auth_from_env_invalid_custom_headers_ignored(): with patch.dict(os.environ, {"MCP_CUSTOM_HEADERS": "{not: json}"}, clear=True): auth_manager = setup_auth_from_env() assert isinstance(auth_manager, AuthManager) assert "custom" not in auth_manager.auth_providers
340-373: Broaden config test to include oauth/custom providers.Extend the valid config test to cover oauth and custom providers to better exercise loaders.load_auth_config.
mcp_fuzzer/transport/base.py (1)
30-49: Declare _stream_request as an abstract method and type the streaming API.Make the contract explicit and improve type-checking by annotating stream_request’s return type.
Apply:
-from typing import Any, Dict, Optional, List +from typing import Any, Dict, Optional, List, AsyncIterator @@ - async def stream_request(self, payload: Dict[str, Any]): + async def stream_request(self, payload: Dict[str, Any]) -> AsyncIterator[Dict[str, Any]]: @@ - async for response in self._stream_request(payload): + async for response in self._stream_request(payload): yield response + + @abstractmethod + async def _stream_request(self, payload: Dict[str, Any]) -> AsyncIterator[Dict[str, Any]]: + """Subclasses must implement streaming of requests.""" + passtests/integration/test_client_transport.py (1)
27-34: Lightweight smoke test looks good; consider asserting streaming capability exists.Optionally add a check that the transport exposes stream_request to reflect the new base API.
mcp_fuzzer/transport/sse.py (1)
1-6: Use typing.List for broader Python compatibility.list[str] requires Python 3.9+. If supporting 3.8, switch to List[str].
-from typing import Any, Dict, Optional +from typing import Any, Dict, Optional, List @@ - data_parts: list[str] = [] + data_parts: List[str] = []Also applies to: 101-116
mcp_fuzzer/transport/http.py (2)
63-70: Redirect fallback fabricates a trailing “/” when Location is missing.This is unusual for 307/308; consider returning None instead to avoid unintended requests.
- if not location and not self.url.endswith("/"): - location = self.url + "/" - if not location: - return None + if not location: + return None
197-206: Optional: annotate _stream_request return type for clarity.Improve readability and tooling.
- async def _stream_request(self, payload: Dict[str, Any]): + async def _stream_request(self, payload: Dict[str, Any]): + # type: (Dict[str, Any]) -> typing.AsyncIterator[Dict[str, Any]]Or import AsyncIterator and annotate the signature directly.
mcp_fuzzer/transport/factory.py (2)
42-45: Parse stdio: URL command instead of forcing empty commandPermit “stdio:cmd” or “stdio://cmd” to pass a command through (keeps tests green while enabling utility).
- if scheme == "stdio": - # Use empty command by default; tests only assert type - return StdioTransport("", **kwargs) + if scheme == "stdio": + # Allow stdio:cmd or stdio://cmd; default empty if none + cmd = (parsed.netloc + parsed.path).lstrip("/") if (parsed.netloc or parsed.path) else "" + return StdioTransport(cmd, **kwargs)
17-30: More actionable error on unsupported protocolInclude the supported set in the exception for easier debugging.
- except KeyError: - raise ValueError(f"Unsupported protocol: {url_or_protocol}") + except KeyError: + supported = ", ".join(sorted(mapping.keys())) + raise ValueError(f"Unsupported protocol: {url_or_protocol!r}. Supported: {supported}")mcp_fuzzer/transport/stdio.py (3)
24-26: request_id never advances; risk of ID collisions across multiple callsIncrement after each use to avoid ambiguous responses, even in tests.
- self.request_id = 1 + self.request_id = 1Apply the increments in the send/stream helpers below.
213-231: Make test helper robust: use injectable IO, flush, and advance request_idAvoid hard-wiring module-level sys; allow tests to set transport._sys while keeping sane defaults; flush writes; increment id.
- async def _send_request(self, payload: Dict[str, Any]) -> Dict[str, Any]: + async def _send_request(self, payload: Dict[str, Any]) -> Dict[str, Any]: """Compatibility method for tests expecting sys-based stdio behavior. @@ - message = {**payload, "id": self.request_id, "jsonrpc": "2.0"} - # Do not append a newline here; some tests assert exact written content - sys.stdout.write(json.dumps(message)) - - line = sys.stdin.readline() + io = getattr(self, "_sys", sys) + message = {**payload, "id": self.request_id, "jsonrpc": "2.0"} + # Do not append newline here; tests assert exact payload + io.stdout.write(json.dumps(message)) + if hasattr(io.stdout, "flush"): + io.stdout.flush() + # Advance id to avoid collisions + self.request_id += 1 + + line = io.stdin.readline() if inspect.isawaitable(line): line = await line if isinstance(line, bytes): line = line.decode() if not line: raise Exception("No response received on stdio") return json.loads(line)
139-171: Minor: json parsing errors lumped under generic ExceptionConsider catching json.JSONDecodeError separately to improve logs and avoid resetting _initialized on transient payload issues.
- except Exception as e: + except json.JSONDecodeError as e: + logging.error(f"Invalid JSON from stdio transport: {e}") + raise + except Exception as e: logging.error(f"Failed to receive message from stdio transport: {e}") self._initialized = False raisetests/unit/transport/test_transport.py (3)
208-239: Use MagicMock for raise_for_status; keep aiter_lines contract closer to httpxSlightly closer fidelity: raise_for_status is sync; aiter_lines is iterated without awaiting in most httpx versions.
- # Create a proper AsyncMock for the response - mock_response = AsyncMock() + # Create a mock response; aclose remains awaited later + mock_response = AsyncMock() @@ - # Add raise_for_status method - mock_response.raise_for_status = AsyncMock() + # raise_for_status is sync in httpx + from unittest.mock import MagicMock as _MM + mock_response.raise_for_status = _MM()
471-502: Add coverage for streamablehttp and URL param preservation; test back-compat signatureThese will lock in factory behavior and prevent regressions.
@@ def test_create_transport_invalid_scheme(): """Test create_transport with invalid URL scheme.""" with pytest.raises(ValueError): create_transport("invalid://example.com") + + +def test_create_transport_streamablehttp(): + transport = create_transport("streamablehttp://example.com/api?x=1#frag") + assert isinstance(transport, HTTPTransport.__class__.__mro__[1]) or isinstance(transport, type(create_transport("http://x"))) + # The factory returns StreamableHTTPTransport; import inside to avoid circulars + from mcp_fuzzer.transport.streamable_http import StreamableHTTPTransport + assert isinstance(transport, StreamableHTTPTransport) + assert transport.url == "http://example.com/api?x=1" + + +def test_create_transport_sse_preserves_query(): + transport = create_transport("sse://example.com/events?x=1") + assert isinstance(transport, SSETransport) + assert transport.url == "http://example.com/events?x=1" + + +def test_create_transport_back_compat_protocol_endpoint(): + # Old signature: (protocol, endpoint) + t = create_transport("https", "https://example.com/api") + assert isinstance(t, HTTPTransport) + assert t.url == "https://example.com/api"
492-496: Optional: Assert stdio command parsing when providedAfter the factory enhancement, add:
def test_create_transport_stdio(): """Test create_transport with stdio URL.""" - transport = create_transport("stdio:") + transport = create_transport("stdio:") assert isinstance(transport, StdioTransport) + # With a command + transport2 = create_transport("stdio:mycmd") + assert isinstance(transport2, StdioTransport) + assert transport2.command == "mycmd"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
mcp_fuzzer/transport/base.py(1 hunks)mcp_fuzzer/transport/factory.py(1 hunks)mcp_fuzzer/transport/http.py(1 hunks)mcp_fuzzer/transport/sse.py(2 hunks)mcp_fuzzer/transport/stdio.py(4 hunks)tests/integration/test_client_transport.py(1 hunks)tests/unit/auth/test_auth.py(1 hunks)tests/unit/fuzz_engine/strategy/test_realistic_strategies.py(1 hunks)tests/unit/fuzz_engine/strategy/test_schema_parser.py(1 hunks)tests/unit/fuzz_engine/test_executor.py(1 hunks)tests/unit/fuzz_engine/test_invariants.py(1 hunks)tests/unit/safety_system/test_safety.py(1 hunks)tests/unit/transport/test_transport.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/fuzz_engine/strategy/test_realistic_strategies.py
🧰 Additional context used
🧬 Code graph analysis (12)
tests/unit/fuzz_engine/strategy/test_schema_parser.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (8)
make_fuzz_strategy_from_jsonschema(52-128)_handle_enum(131-153)_handle_string_type(274-324)_handle_integer_type(462-527)_handle_number_type(530-604)_handle_boolean_type(607-627)_handle_array_type(204-271)_handle_object_type(156-201)
mcp_fuzzer/transport/factory.py (4)
mcp_fuzzer/transport/http.py (1)
HTTPTransport(22-266)mcp_fuzzer/transport/streamable_http.py (1)
StreamableHTTPTransport(41-370)mcp_fuzzer/transport/sse.py (1)
SSETransport(10-129)mcp_fuzzer/transport/stdio.py (1)
StdioTransport(20-424)
mcp_fuzzer/transport/http.py (4)
mcp_fuzzer/transport/sse.py (1)
_stream_request(57-99)mcp_fuzzer/transport/stdio.py (2)
_stream_request(243-267)_update_activity(44-49)mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)tests/unit/transport/test_streamable_http.py (3)
post(78-82)json(38-39)aiter_lines(58-64)
tests/unit/fuzz_engine/test_invariants.py (1)
mcp_fuzzer/fuzz_engine/invariants.py (7)
InvariantViolation(41-47)check_response_validity(50-126)check_error_type_correctness(129-185)check_response_schema_conformity(188-212)verify_response_invariants(215-245)verify_batch_responses(248-302)check_state_consistency(305-345)
tests/unit/safety_system/test_safety.py (1)
mcp_fuzzer/safety_system/safety.py (8)
SafetyFilter(41-348)is_safe_tool_call(413-414)sanitize_tool_call(417-421)create_safety_response(424-425)contains_dangerous_url(84-92)contains_dangerous_command(94-102)_sanitize_string_argument(133-169)_sanitize_value(113-131)
tests/unit/auth/test_auth.py (3)
mcp_fuzzer/auth/providers.py (9)
APIKeyAuth(16-25)AuthProvider(6-13)BasicAuth(28-39)CustomHeaderAuth(54-62)OAuthTokenAuth(42-51)create_api_key_auth(65-66)create_basic_auth(69-70)create_custom_header_auth(77-78)create_oauth_auth(73-74)mcp_fuzzer/auth/manager.py (6)
AuthManager(6-35)add_auth_provider(13-14)map_tool_to_auth(16-17)get_auth_for_tool(19-23)get_auth_headers_for_tool(25-29)get_auth_params_for_tool(31-35)mcp_fuzzer/auth/loaders.py (2)
load_auth_config(59-107)setup_auth_from_env(14-56)
tests/unit/transport/test_transport.py (6)
tests/unit/transport/test_streamable_http.py (4)
json(38-39)aiter_lines(58-64)raise_for_status(34-36)raise_for_status(55-56)mcp_fuzzer/transport/http.py (5)
HTTPTransport(22-266)send_request(72-124)send_raw(126-160)send_notification(162-185)_stream_request(197-252)mcp_fuzzer/transport/sse.py (6)
SSETransport(10-129)send_request(19-24)send_raw(26-47)send_notification(49-55)_stream_request(57-99)_parse_sse_event(102-129)mcp_fuzzer/transport/stdio.py (6)
StdioTransport(20-424)send_request(172-196)send_raw(198-211)send_notification(232-241)_send_request(213-230)_stream_request(243-267)mcp_fuzzer/transport/base.py (7)
TransportProtocol(13-92)send_request(15-18)send_raw(21-22)send_notification(25-28)connect(30-32)disconnect(34-36)stream_request(38-48)mcp_fuzzer/transport/factory.py (1)
create_transport(9-49)
mcp_fuzzer/transport/base.py (4)
mcp_fuzzer/transport/http.py (1)
_stream_request(197-252)mcp_fuzzer/transport/sse.py (1)
_stream_request(57-99)mcp_fuzzer/transport/stdio.py (1)
_stream_request(243-267)tests/unit/transport/test_transport.py (3)
_stream_request(53-54)_stream_request(87-88)_stream_request(123-126)
tests/integration/test_client_transport.py (2)
mcp_fuzzer/client/protocol_client.py (1)
ProtocolClient(19-399)mcp_fuzzer/transport/streamable_http.py (1)
StreamableHTTPTransport(41-370)
mcp_fuzzer/transport/stdio.py (4)
tests/unit/transport/test_transport.py (6)
_send_request(50-51)_send_request(83-85)_send_request(120-121)_stream_request(53-54)_stream_request(87-88)_stream_request(123-126)tests/unit/transport/test_streamable_http.py (1)
json(38-39)mcp_fuzzer/transport/http.py (1)
_stream_request(197-252)mcp_fuzzer/transport/sse.py (1)
_stream_request(57-99)
mcp_fuzzer/transport/sse.py (3)
mcp_fuzzer/transport/http.py (1)
_stream_request(197-252)mcp_fuzzer/transport/stdio.py (1)
_stream_request(243-267)tests/unit/transport/test_transport.py (3)
_stream_request(53-54)_stream_request(87-88)_stream_request(123-126)
tests/unit/fuzz_engine/test_executor.py (1)
mcp_fuzzer/fuzz_engine/executor.py (5)
AsyncFuzzExecutor(11-244)execute(47-95)execute_with_retry(97-151)execute_batch(153-199)_execute_and_track(201-218)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (15)
tests/unit/safety_system/test_safety.py (1)
16-17: Markers look good for component-based runs.
pytestmark = [pytest.mark.unit, pytest.mark.safety_system]aligns with the PR’s component selection goals.tests/unit/fuzz_engine/strategy/test_schema_parser.py (7)
22-22: Markers look good.Component and unit markers align with the PR’s component-based test selection.
25-53: Solid coverage of basic types.Assertions are tolerant to generator variability.
55-63: Array test is resilient to empty outputs.Good guard against empty arrays.
65-83: Object generation checks are appropriate.Verifies required fields without over-constraining optional ones.
85-91: Enum handling covered.Realistic phase expectations are correct.
93-118: Combinations (oneOf/anyOf/allOf) covered.Good functional checks without brittleness.
193-211: Enum aggressive case is safe here.Aggressive invalids remain strings for string enums, so the type assertion holds.
tests/unit/fuzz_engine/test_executor.py (1)
111-151: Batch success/mixed tests look solid.Order-insensitive checks avoid flakiness.
tests/unit/fuzz_engine/test_invariants.py (3)
241-258: Batch all-valid path is good.Mocks align with async to_thread usage.
260-278: Batch mixed path is good.Clear signal of per-item outcomes.
304-310: State consistency negative path covered well.Validates unexpected change handling.
mcp_fuzzer/transport/sse.py (1)
22-24: SSETransport.send_request change is isolated—no production code invokes it.mcp_fuzzer/transport/factory.py (1)
36-37: LGTM for http(s) passthroughSingle-URL http/https path is clear and backward compatible.
mcp_fuzzer/transport/stdio.py (1)
243-268: Refactor_stream_requestto use subprocess pipes
Add a branch in_stream_requestthat writes to the child process’s stdin and reads from its stdout (e.g. viaawait self.process_manager.get_process()), falling back to the patchedself._sysfor tests. No production code invokesStdioTransportor streams over modulesysoutside of test fixtures, so this refactor is safe.
| def test_check_response_validity_valid_notification(): | ||
| """Test that a valid JSON-RPC notification passes validation.""" | ||
| response = {"jsonrpc": "2.0"} | ||
| assert check_response_validity(response) | ||
|
|
There was a problem hiding this comment.
Notification example is invalid JSON-RPC.
A notification must include a method and no id. Update the fixture.
Apply:
-response = {"jsonrpc": "2.0"}
+response = {"jsonrpc": "2.0", "method": "notify", "params": {}}📝 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 test_check_response_validity_valid_notification(): | |
| """Test that a valid JSON-RPC notification passes validation.""" | |
| response = {"jsonrpc": "2.0"} | |
| assert check_response_validity(response) | |
| def test_check_response_validity_valid_notification(): | |
| """Test that a valid JSON-RPC notification passes validation.""" | |
| response = {"jsonrpc": "2.0", "method": "notify", "params": {}} | |
| assert check_response_validity(response) |
🤖 Prompt for AI Agents
In tests/unit/fuzz_engine/test_invariants.py around lines 39 to 43, the test
uses an invalid JSON-RPC notification fixture that only contains "jsonrpc";
update the fixture to include a "method" field (e.g., a valid method name
string) and ensure there is no "id" field so it represents a proper
notification; keep the assertion calling check_response_validity(response)
unchanged.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (1)
442-456: Fix: logging test patches the module, but the code uses an instance logger.
ToolFuzzer logs via self._logger.info; patching the module won’t observe those calls, making this test flaky/incorrect.Apply:
- with patch("mcp_fuzzer.fuzz_engine.fuzzer.tool_fuzzer.logging") as mock_logging: - await self.fuzzer.fuzz_tool(tool, runs=1) - - # Check that logging.info was called - mock_logging.info.assert_called() - - # Check that the log message contains expected information - call_args = mock_logging.info.call_args_list - found_tool = False - for call_args_tuple in call_args: - if "test_tool" in str(call_args_tuple): - found_tool = True - break - self.assertTrue(found_tool) + with patch.object(self.fuzzer, "_logger") as mock_logger: + await self.fuzzer.fuzz_tool(tool, runs=1) + mock_logger.info.assert_called() + # Ensure the tool name made it into an info log + calls = [str(c) for c in mock_logger.info.call_args_list] + self.assertTrue(any("test_tool" in c for c in calls))
♻️ Duplicate comments (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
116-131: Const support added — thanks for addressing earlier feedback.This implements the previously requested fast-path for
"const".
🧹 Nitpick comments (14)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (4)
44-45: Make batch validation timeout configurable.Expose
BATCH_VALIDATION_TIMEOUTvia ctor arg or env var to tune for slower servers in CI.- # Seconds to wait for invariant validation of batch responses - BATCH_VALIDATION_TIMEOUT: ClassVar[float] = 5.0 + # Seconds to wait for invariant validation of batch responses + BATCH_VALIDATION_TIMEOUT: ClassVar[float] = 5.0 + + def __init__(self, transport: Optional[Any] = None, max_concurrency: int = 5, + batch_validation_timeout: Optional[float] = None): + ... + self.BATCH_VALIDATION_TIMEOUT = ( + batch_validation_timeout + if batch_validation_timeout is not None + else self.BATCH_VALIDATION_TIMEOUT + )
222-252: Good invariant integration; consider passing context.Pass expected error codes and/or schema when known to improve signal; also consider logging count of violations for batches.
- batch = await asyncio.wait_for( - verify_batch_responses(server_response), + batch = await asyncio.wait_for( + verify_batch_responses( + server_response, + # expected_error_codes=expected_codes, + # schema=known_schema, + ), timeout=self.BATCH_VALIDATION_TIMEOUT, )
258-261: Reflect invariant violations in success flag.If invariants fail, set
success=Falseto surface failures in summaries.- if invariant_violations: - result["invariant_violations"] = invariant_violations + if invariant_violations: + result["invariant_violations"] = invariant_violations + result["success"] = False
436-446: Avoid hard-coded 30s per-type timeout.Make per-type timeout configurable; cancel with shielded cleanup to avoid leaks.
- results = await asyncio.wait_for(task, timeout=30.0) + results = await asyncio.wait_for(task, timeout=self._per_type_timeout)(Add
_per_type_timeoutas a ctor arg with a sane default.)mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (4)
28-31: Expose recursion depth tuning and guard unsatisfied schemas.Make
MAX_RECURSION_DEPTHconfigurable and, when exceeded, prefer schema-compliant fallbacks (e.g., respecttypelists fully) to reduce invalid outputs in “realistic”.-MAX_RECURSION_DEPTH = 5 +MAX_RECURSION_DEPTH = int(os.getenv("MCP_MAX_SCHEMA_DEPTH", "5"))(Import
os.)
235-313: uniqueItems may still emit duplicates after 10 attempts.Switch to a while-based fill loop that enforces uniqueness deterministically in “realistic”; relax only in “aggressive”.
- result = [] - seen_values = set() # For uniqueItems constraint - - for _ in range(array_size): + result: List[Any] = [] + seen_values: set[str] = set() # For uniqueItems constraint + def _stable_hash(v) -> str: + import json as _json + try: return _json.dumps(v, sort_keys=True, default=str) + except Exception: return repr(v) + while len(result) < array_size: # Generate item based on items schema item = make_fuzz_strategy_from_jsonschema( items_schema, phase, recursion_depth + 1 ) - - # Handle uniqueItems constraint - if unique_items: - # For simple types, ensure uniqueness - import json as _json - - attempts = 0 - while attempts < 10: - try: - item_hash = _json.dumps(item, sort_keys=True, default=str) - except Exception: - # Fallback to repr if dumps fails - item_hash = repr(item) - if item_hash not in seen_values: - seen_values.add(item_hash) - break - item = make_fuzz_strategy_from_jsonschema( - items_schema, phase, recursion_depth + 1 - ) - attempts += 1 - - result.append(item) + if unique_items: + item_hash = _stable_hash(item) + if item_hash in seen_values: + if phase == "realistic": + continue # try again to maintain uniqueness + # aggressive: allow occasional dup + else: + seen_values.add(item_hash) + result.append(item)
315-366: Regex generation is very limited; consider optional lib integration.Optionally use
rstr/exrexwhen available to cover more patterns; fallback to current logic.- return _generate_string_from_pattern(pattern, min_length, max_length) + try: + import rstr # type: ignore + return rstr.xeger(pattern)[:max_length] + except Exception: + return _generate_string_from_pattern(pattern, min_length, max_length)
571-646: Float multipleOf: reduce FP error with Decimal quantization.Floating rounding can miss valid multiples; prefer
DecimalwhenmultipleOfis finite.- if multiple_of: + if multiple_of and math.isfinite(float(multiple_of)): try: - m = float(multiple_of) + from decimal import Decimal, getcontext + getcontext().prec = 28 + m = Decimal(str(multiple_of)) + dmin, dmax = Decimal(str(minimum)), Decimal(str(maximum)) @@ - k_start = math.ceil(minimum / m) - k_end = math.floor(maximum / m) + k_start = math.ceil((dmin / m)) + k_end = math.floor((dmax / m)) @@ - value = k * m + value = float(Decimal(k) * m)tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (6)
131-136: Tighten mock to real signature with autospec for coroutine.
Use autospec with AsyncMock so calls that drift from the real async signature fail loudly.Apply:
- with patch.object( - self.fuzzer, "fuzz_tool", new_callable=AsyncMock - ) as mock_fuzz: + with patch.object( + self.fuzzer, "fuzz_tool", new_callable=AsyncMock, autospec=True + ) as mock_fuzz:
164-166: Cover the negative safety path.
You only test the “safe” branch. Add a test asserting safety_blocked=True and safety_reason when is_safe_tool_call returns False.Here’s a concise test you can drop in:
@pytest.mark.asyncio @patch("mcp_fuzzer.fuzz_engine.fuzzer.tool_fuzzer.is_safe_tool_call", return_value=False) async def test_fuzz_tool_safety_blocked(mock_is_safe): fuzzer = ToolFuzzer() tool = {"name": "danger_tool", "inputSchema": {"properties": {"p": {"type": "string"}}}} res = await fuzzer.fuzz_tool(tool, runs=1) assert len(res) == 1 assert res[0]["success"] is False assert res[0].get("safety_blocked") is True assert "safety_reason" in res[0]Also applies to: 304-306
185-194: Assert sanitize was used with expected inputs.
Lock in behavior by verifying sanitize_tool_call was called once with "complex_tool" and the pre-sanitized args.Add after the await:
mock_sanitize.assert_called_once() called_tool, called_args = mock_sanitize.call_args[0] self.assertEqual(called_tool, "complex_tool") self.assertIsInstance(called_args, dict)
322-325: Avoid magic numbers in side_effect; derive from a single constant.
Prevents future drift between the comprehension and the runs argument.Apply:
- mock_strategies.fuzz_tool_arguments.side_effect = [ - {"name": f"test_{i}", "count": i, "enabled": i % 2 == 0} for i in range(5) - ] + num_runs = 5 + mock_strategies.fuzz_tool_arguments.side_effect = [ + {"name": f"test_{i}", "count": i, "enabled": i % 2 == 0} + for i in range(num_runs) + ]And update the call to use num_runs:
- results = await self.fuzzer.fuzz_tool(tool, runs=5) + results = await self.fuzzer.fuzz_tool(tool, runs=num_runs)
424-431: Clarify expectation for None tool name.
Current assertion accepts both "unknown" and None. Given implementation returns the original name (even if None), make the expectation deterministic (None) or split into two tests (sanitized vs original).For a deterministic check now:
- self.assertIn(result["tool_name"], ["unknown", None]) + self.assertIsNone(result["tool_name"])If behavior changes to prefer sanitized names later, update accordingly.
469-474: Phase assertion could be parameterized for both realistic and aggressive.
Covers strategy routing logic across phases in one place.Example:
@pytest.mark.asyncio @pytest.mark.parametrize("phase", ["aggressive", "realistic"]) async def test_strategy_integration_param(self, phase): tool = {"name": "test_tool", "inputSchema": {"properties": {"name": {"type": "string"}}}} with patch.object(self.fuzzer.strategies, "fuzz_tool_arguments") as mock_fuzz: mock_fuzz.return_value = {"name": "v"} await self.fuzzer.fuzz_tool(tool, runs=1, phase=phase) mock_fuzz.assert_called_with(tool, phase=phase)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py(6 hunks)mcp_fuzzer/fuzz_engine/invariants.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/schema_parser.py(1 hunks)tests/add_markers.py(1 hunks)tests/integration/test_client_transport.py(1 hunks)tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/add_markers.py
- mcp_fuzzer/fuzz_engine/invariants.py
- tests/integration/test_client_transport.py
🧰 Additional context used
🧬 Code graph analysis (2)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (2)
mcp_fuzzer/types.py (1)
FuzzDataResult(12-22)mcp_fuzzer/fuzz_engine/invariants.py (3)
verify_response_invariants(222-252)InvariantViolation(41-47)verify_batch_responses(255-309)
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (2)
mcp_fuzzer/fuzz_engine/fuzzer/tool_fuzzer.py (1)
ToolFuzzer(21-257)mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py (1)
fuzz_tool_arguments(54-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: component-tests
- GitHub Check: tests
🔇 Additional comments (6)
mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py (1)
311-312: Apply TypedDict updates to FuzzDataResult
Updatemcp_fuzzer/types.pyso thatserver_responseallowsList[Any]and add the newinvariant_violationsfield—no dict-only access patterns were found, so this change is safe.--- a/mcp_fuzzer/types.py +++ b/mcp_fuzzer/types.py @@ class FuzzDataResult(TypedDict, total=False): - server_response: Optional[Dict[str, Any]] + server_response: Optional[Union[Dict[str, Any], List[Any]]] @@ class FuzzDataResult(TypedDict, total=False): exception: Optional[str] + invariant_violations: List[str]tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (5)
9-9: Good addition: AsyncMock import aligns tests with async API.
This enables correct patching of coroutine functions used by the fuzzer.
195-203: LGTM: deterministic mocks keep complex-schema test stable.
The sanitize and strategy mocks make this test resilient while still validating integration points.
241-251: LGTM: empty schema case is exercised without over-constraining behavior.
Asserting dict structure only is appropriate here.
327-327: Re-instantiating fuzzer to pick up patched ToolStrategies is correct.
This ensures the constructor uses the mocked factory.
528-529: LGTM: awaiting both fuzzer instances enforces true async execution and isolation.
This matches the async API and avoids false positives.
| def _merge_allOf(schemas: List[Dict[str, Any]]) -> Dict[str, Any]: | ||
| """Deep merge for allOf schemas.""" | ||
| merged: Dict[str, Any] = {} | ||
| props: Dict[str, Any] = {} | ||
| required: List[str] = [] | ||
| merged_types = None # track intersection of declared types | ||
| for s in schemas: | ||
| if "properties" in s and isinstance(s["properties"], dict): | ||
| props.update(s["properties"]) | ||
| if "required" in s and isinstance(s["required"], list): | ||
| required.extend([r for r in s["required"] if isinstance(r, str)]) | ||
| t = s.get("type") | ||
| if t is not None: | ||
| tset = set(t if isinstance(t, list) else [t]) | ||
| merged_types = tset if merged_types is None else (merged_types & tset) | ||
| for k, v in s.items(): | ||
| if k not in ("properties", "required"): | ||
| merged[k] = v if k not in merged else merged[k] | ||
| if props: | ||
| merged["properties"] = props | ||
| if required: | ||
| merged["required"] = sorted(set(required)) | ||
| if merged_types: | ||
| if len(merged_types) > 1: | ||
| merged["type"] = list(merged_types) | ||
| else: | ||
| merged["type"] = next(iter(merged_types)) | ||
| return merged | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
allOf merge is too lossy for constraints; combine restrictively.
Current merge keeps first/last values arbitrarily, producing invalid “realistic” data when constraints conflict. Merge numerics/lengths using max(minimum/minLength) and min(maximum/maxLength); combine required; intersect types.
def _merge_allOf(schemas: List[Dict[str, Any]]) -> Dict[str, Any]:
@@
- merged: Dict[str, Any] = {}
+ merged: Dict[str, Any] = {}
props: Dict[str, Any] = {}
required: List[str] = []
merged_types = None # track intersection of declared types
+ mins = {}; maxs = {}; # name -> list
+ for k in ("minimum","minLength","exclusiveMinimum"):
+ mins[k]=[]
+ for k in ("maximum","maxLength","exclusiveMaximum"):
+ maxs[k]=[]
for s in schemas:
@@
- for k, v in s.items():
- if k not in ("properties", "required"):
- merged[k] = v if k not in merged else merged[k]
+ for k, v in s.items():
+ if k in mins and isinstance(v, (int, float)):
+ mins[k].append(v)
+ elif k in maxs and isinstance(v, (int, float)):
+ maxs[k].append(v)
+ elif k not in ("properties", "required", "type"):
+ merged.setdefault(k, v)
@@
if merged_types:
@@
return merged
+ # After loop:
+ for k, vals in mins.items():
+ if vals: merged[k] = max(vals)
+ for k, vals in maxs.items():
+ if vals: merged[k] = min(vals)(Adjust for boolean-style exclusives as needed.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp_fuzzer/fuzz_engine/strategy/schema_parser.py around lines 32-60, the
allOf merge currently preserves first/last values and loses constraints,
producing invalid merged schemas; change the merge to be restrictive: for
numeric bounds (minimum/maximum) and length bounds (minLength/maxLength) compute
the most restrictive values (maximum of all minima, minimum of all maxima) and
if exclusivity flags (exclusiveMinimum/exclusiveMaximum) are present treat them
as restrictive (e.g., if any schema sets exclusive true for the active boundary,
mark it exclusive); intersect types as already intended but fail/return a
clearly impossible schema when intersections are empty; intersect enums (take
set intersection) and for boolean-style exclusives or other constraint pairs
choose the stricter interpretation (e.g., exclusive beats inclusive when equal);
keep combining required and properties as before but replace simple overwrite
behavior for other constraint keys by applying the more restrictive value
instead (prefer intersection/stricter numeric/length constraint where
applicable).
| def _handle_object_type( | ||
| schema: Dict[str, Any], phase: str, recursion_depth: int | ||
| ) -> Dict[str, Any]: | ||
| """Handle object type schema.""" | ||
| result = {} | ||
|
|
||
| # Get properties | ||
| properties = schema.get("properties", {}) | ||
| required = schema.get("required", []) | ||
|
|
||
| # Handle property constraints | ||
| min_properties = schema.get("minProperties", 0) | ||
| # We don't need to explicitly use max_properties as we're not | ||
| # enforcing a maximum number of properties in the generated objects | ||
|
|
||
| # Process each property | ||
| for prop_name, prop_schema in properties.items(): | ||
| # For required properties or by chance for optional ones | ||
| if prop_name in required or random.random() < 0.8: | ||
| result[prop_name] = make_fuzz_strategy_from_jsonschema( | ||
| prop_schema, phase, recursion_depth + 1 | ||
| ) | ||
|
|
||
| # Ensure we meet minProperties constraint | ||
| if len(result) < min_properties: | ||
| # Add additional properties if needed | ||
| additional_count = min_properties - len(result) | ||
| for i in range(additional_count): | ||
| prop_name = f"additional_prop_{i}" | ||
| result[prop_name] = _generate_default_value(phase) | ||
|
|
||
| # In aggressive mode, sometimes add extra properties | ||
| if phase == "aggressive" and random.random() < 0.3: | ||
| # Add some potentially problematic properties | ||
| extra_props = { | ||
| "__proto__": {"isAdmin": True}, | ||
| "constructor": {"prototype": {"isAdmin": True}}, | ||
| "eval": "console.log('code injection')", | ||
| } | ||
| result.update( | ||
| random.sample( | ||
| extra_props.items(), k=min(len(extra_props), random.randint(1, 3)) | ||
| ) | ||
| ) | ||
|
|
||
| return result | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Honor additionalProperties when adding or padding properties.
Respect additionalProperties: false; don’t synthesize additional_prop_* or extra props in that case. Also, prefer patternProperties when available to satisfy minProperties.
- # Ensure we meet minProperties constraint
+ # Ensure we meet minProperties constraint
if len(result) < min_properties:
- # Add additional properties if needed
- additional_count = min_properties - len(result)
- for i in range(additional_count):
- prop_name = f"additional_prop_{i}"
- result[prop_name] = _generate_default_value(phase)
+ allow_additional = schema.get("additionalProperties", True) is not False
+ if allow_additional:
+ additional_count = min_properties - len(result)
+ for i in range(additional_count):
+ prop_name = f"additional_prop_{i}"
+ result[prop_name] = _generate_default_value(phase)
+ # If not allowed, leave as-is; schema may be unsatisfiable w.r.t minProperties.
@@
- if phase == "aggressive" and random.random() < 0.3:
+ if phase == "aggressive" and random.random() < 0.3 and schema.get("additionalProperties", True) is not 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 _handle_object_type( | |
| schema: Dict[str, Any], phase: str, recursion_depth: int | |
| ) -> Dict[str, Any]: | |
| """Handle object type schema.""" | |
| result = {} | |
| # Get properties | |
| properties = schema.get("properties", {}) | |
| required = schema.get("required", []) | |
| # Handle property constraints | |
| min_properties = schema.get("minProperties", 0) | |
| # We don't need to explicitly use max_properties as we're not | |
| # enforcing a maximum number of properties in the generated objects | |
| # Process each property | |
| for prop_name, prop_schema in properties.items(): | |
| # For required properties or by chance for optional ones | |
| if prop_name in required or random.random() < 0.8: | |
| result[prop_name] = make_fuzz_strategy_from_jsonschema( | |
| prop_schema, phase, recursion_depth + 1 | |
| ) | |
| # Ensure we meet minProperties constraint | |
| if len(result) < min_properties: | |
| # Add additional properties if needed | |
| additional_count = min_properties - len(result) | |
| for i in range(additional_count): | |
| prop_name = f"additional_prop_{i}" | |
| result[prop_name] = _generate_default_value(phase) | |
| # In aggressive mode, sometimes add extra properties | |
| if phase == "aggressive" and random.random() < 0.3: | |
| # Add some potentially problematic properties | |
| extra_props = { | |
| "__proto__": {"isAdmin": True}, | |
| "constructor": {"prototype": {"isAdmin": True}}, | |
| "eval": "console.log('code injection')", | |
| } | |
| result.update( | |
| random.sample( | |
| extra_props.items(), k=min(len(extra_props), random.randint(1, 3)) | |
| ) | |
| ) | |
| return result | |
| # Ensure we meet minProperties constraint | |
| if len(result) < min_properties: | |
| allow_additional = schema.get("additionalProperties", True) is not False | |
| if allow_additional: | |
| additional_count = min_properties - len(result) | |
| for i in range(additional_count): | |
| prop_name = f"additional_prop_{i}" | |
| result[prop_name] = _generate_default_value(phase) | |
| # If not allowed, leave as-is; schema may be unsatisfiable w.r.t minProperties. | |
| # In aggressive mode, sometimes add extra properties | |
| if phase == "aggressive" and random.random() < 0.3 and schema.get("additionalProperties", True) is not False: | |
| # Add some potentially problematic properties | |
| extra_props = { | |
| "__proto__": {"isAdmin": True}, | |
| "constructor": {"prototype": {"isAdmin": True}}, | |
| "eval": "console.log('code injection')", | |
| } | |
| result.update( | |
| random.sample( | |
| extra_props.items(), k=min(len(extra_props), random.randint(1, 3)) | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In mcp_fuzzer/fuzz_engine/strategy/schema_parser.py around lines 187 to 233, the
code currently always synthesizes additional properties and aggressive extra
props without honoring additionalProperties or patternProperties; update the
logic so that when adding/padding properties you first check
schema.get("additionalProperties", True) and do not create any additional_prop_*
or extra_props if that value is False; if patternProperties exists, prefer to
satisfy minProperties by creating entries whose names match one of the
patternProperties keys and use make_fuzz_strategy_from_jsonschema with the
corresponding patternProperties schema (or a merged schema with
additionalProperties when present); when falling back to generated names only do
so if additionalProperties is True; likewise guard the aggressive-mode injection
of prototype/constructor/eval props by additionalProperties and avoid injecting
reserved keys when disallowed.
| from unittest.mock import MagicMock, call, patch, AsyncMock | ||
| from mcp_fuzzer.fuzz_engine.fuzzer.tool_fuzzer import ToolFuzzer | ||
|
|
||
| pytestmark = [pytest.mark.unit, pytest.mark.fuzz_engine, pytest.mark.fuzzer] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Project-wide markers: register them to avoid PytestUnknownMarkWarning and to support component-scoped runs.
Nice touch adding unit/fuzz_engine/fuzzer markers; make sure they’re declared in pytest config so CI can select changed components per #70.
Run this to verify marker registration:
If missing, add in pytest.ini or pyproject.toml (tool.pytest.ini_options.markers):
- unit
- fuzz_engine
- fuzzer
Then CI can run:pytest -m "unit and fuzz_engine and fuzzer".
🏁 Script executed:
#!/bin/bash
# Look for marker declarations in pytest config files
fd -a 'pyproject.toml|pytest.ini|tox.ini' -x rg -nC1 -i 'markers|ini_options'Length of output: 306
Add the unit, fuzz_engine, and fuzzer markers to your pytest config: the project’s [tool.pytest.ini_options] currently only declares "auth"; you must append entries for unit, fuzz_engine, and fuzzer under markers in pyproject.toml or pytest.ini so that pytest -m "unit and fuzz_engine and fuzzer" runs without warnings.
🤖 Prompt for AI Agents
In tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py around line 12, the test
file uses pytest markers "unit", "fuzz_engine", and "fuzzer" but the project
pytest config only declares "auth", causing marker warnings; update
pyproject.toml (or pytest.ini) under [tool.pytest.ini_options] -> markers (or
markers list in pytest.ini) to include "unit", "fuzz_engine", and "fuzzer"
entries so pytest recognizes them (add each marker as a separate string entry or
line).
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
245-253: Schema parser fallback added — good parity with realistic.The try/except around
make_fuzz_strategy_from_jsonschemaprevents hard crashes and aligns behavior with the realistic strategy. Nice.
🧹 Nitpick comments (4)
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (2)
264-289: Handle multi-type schemas (type as list).Property
typemay be a list per JSON Schema. Randomly choose one to diversify aggressive inputs.- if random.random() < 0.8: # 80% chance to include each property - prop_type = prop_spec.get("type", "string") + if random.random() < 0.8: # 80% chance to include each property + prop_type = prop_spec.get("type", "string") + if isinstance(prop_type, list) and prop_type: + prop_type = random.choice(prop_type)
291-295: Optionally make required fields structurally valid for objects/arrays.Even in aggressive mode, using
{}/[]for required object/array fields can keep servers from short-circuiting on type errors, allowing deeper fuzzing.- for key in required or []: - if key not in args: - args[key] = generate_aggressive_text() + for key in required or []: + if key not in args: + spec = (schema.get("properties") or {}).get(key, {}) if isinstance(schema, dict) else {} + t = spec.get("type") + if t == "object": + args[key] = {} + elif t == "array": + args[key] = [] + else: + args[key] = generate_aggressive_text()mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py (2)
171-207: Drop inner re-import; reuse the already-imported parser.The extra import inside the loop adds overhead and noise. Call the function you imported at Line 142.
- for _ in range(count): - try: - from ..schema_parser import ( - make_fuzz_strategy_from_jsonschema as _mk, - ) - - val = _mk(items_schema, phase="realistic") + for _ in range(count): + try: + val = make_fuzz_strategy_from_jsonschema( + items_schema, phase="realistic" + )
208-220: Same here: remove inner re-import for nested objects.- try: - from ..schema_parser import ( - make_fuzz_strategy_from_jsonschema as _mk, - ) - - nested = _mk(prop_spec, phase="realistic") + try: + nested = make_fuzz_strategy_from_jsonschema( + prop_spec, phase="realistic" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T11:50:01.877Z
Learnt from: Agent-Hellboy
PR: Agent-Hellboy/mcp-server-fuzzer#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/fuzz_engine/strategy/aggressive/tool_strategy.py
🧬 Code graph analysis (2)
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
make_fuzz_strategy_from_jsonschema(62-159)
mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
make_fuzz_strategy_from_jsonschema(62-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: component-tests
🔇 Additional comments (2)
mcp_fuzzer/fuzz_engine/strategy/realistic/tool_strategy.py (2)
142-157: Schema-driven generation with guards — LGTM.Local import, non-dict schema guard, and parser try/except produce a robust baseline.
16-16: No action needed: Hypothesis is declared
Hypothesis appears in pyproject.toml (line 33).
| schema = tool.get("inputSchema", {}) | ||
| properties = schema.get("properties", {}) | ||
|
|
||
| args = {} | ||
| # Use the enhanced schema parser to generate aggressive values | ||
| try: | ||
| args = make_fuzz_strategy_from_jsonschema(schema, phase="aggressive") | ||
| except Exception: | ||
| args = {} | ||
|
|
||
| for prop_name, prop_spec in properties.items(): | ||
| prop_type = prop_spec.get("type", "string") | ||
| # If the schema parser returned something other than a dict, create a default dict | ||
| if not isinstance(args, dict): | ||
| args = {} | ||
|
|
||
| # Generate aggressive values based on type | ||
| if prop_type == "string": | ||
| args[prop_name] = generate_aggressive_text() | ||
| elif prop_type == "integer": | ||
| args[prop_name] = _generate_aggressive_integer() | ||
| elif prop_type == "number": | ||
| args[prop_name] = _generate_aggressive_float() | ||
| elif prop_type == "boolean": | ||
| # Use non-boolean values to test validation | ||
| args[prop_name] = random.choice( | ||
| [ | ||
| None, | ||
| "", | ||
| "true", | ||
| "false", | ||
| 0, | ||
| 1, | ||
| -1, | ||
| 42, | ||
| [], | ||
| {}, | ||
| "yes", | ||
| "no", | ||
| "on", | ||
| "off", | ||
| "True", | ||
| "False", | ||
| ] | ||
| ) | ||
| elif prop_type == "array": | ||
| # Generate malformed arrays | ||
| aggressive_arrays = [ | ||
| None, # Not an array | ||
| "not_an_array", # String instead | ||
| [generate_aggressive_text() for _ in range(random.randint(0, 10))], | ||
| [None] * random.randint(1, 5), # Array of nulls | ||
| [{}] * random.randint(1, 3), # Array of empty objects | ||
| [], # Empty array | ||
| ["A" * 10000], # Huge string in array | ||
| list(range(10000)), # Huge array | ||
| ] | ||
| args[prop_name] = random.choice(aggressive_arrays) | ||
| elif prop_type == "object": | ||
| # Generate malformed objects | ||
| aggressive_objects = [ | ||
| None, # Not an object | ||
| "not_an_object", # String instead | ||
| [], # Array instead | ||
| {}, # Empty object | ||
| {generate_aggressive_text(): generate_aggressive_text()}, | ||
| {"evil": "<script>alert('xss')</script>"}, | ||
| {"\x00null\x00": "null_key"}, | ||
| {"a" * 1000: "huge_key"}, | ||
| ] | ||
| args[prop_name] = random.choice(aggressive_objects) | ||
| else: | ||
| # Fallback for unknown types - try to break them | ||
| args[prop_name] = random.choice( | ||
| [ | ||
| None, | ||
| generate_aggressive_text(), | ||
| _generate_aggressive_integer(), | ||
| _generate_aggressive_float(), | ||
| [], | ||
| {}, | ||
| ] | ||
| ) | ||
| # Ensure we have at least some arguments | ||
| if not args and schema.get("properties"): | ||
| # Fallback to basic property handling |
There was a problem hiding this comment.
Guard against non-dict schemas to avoid AttributeError at .get.
If tool["inputSchema"] is not a dict, the later schema.get("properties") access will raise. Mirror the realistic strategy’s type check.
- schema = tool.get("inputSchema", {})
+ schema = tool.get("inputSchema")
+ if not isinstance(schema, dict):
+ 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.
| schema = tool.get("inputSchema", {}) | |
| properties = schema.get("properties", {}) | |
| args = {} | |
| # Use the enhanced schema parser to generate aggressive values | |
| try: | |
| args = make_fuzz_strategy_from_jsonschema(schema, phase="aggressive") | |
| except Exception: | |
| args = {} | |
| for prop_name, prop_spec in properties.items(): | |
| prop_type = prop_spec.get("type", "string") | |
| # If the schema parser returned something other than a dict, create a default dict | |
| if not isinstance(args, dict): | |
| args = {} | |
| # Generate aggressive values based on type | |
| if prop_type == "string": | |
| args[prop_name] = generate_aggressive_text() | |
| elif prop_type == "integer": | |
| args[prop_name] = _generate_aggressive_integer() | |
| elif prop_type == "number": | |
| args[prop_name] = _generate_aggressive_float() | |
| elif prop_type == "boolean": | |
| # Use non-boolean values to test validation | |
| args[prop_name] = random.choice( | |
| [ | |
| None, | |
| "", | |
| "true", | |
| "false", | |
| 0, | |
| 1, | |
| -1, | |
| 42, | |
| [], | |
| {}, | |
| "yes", | |
| "no", | |
| "on", | |
| "off", | |
| "True", | |
| "False", | |
| ] | |
| ) | |
| elif prop_type == "array": | |
| # Generate malformed arrays | |
| aggressive_arrays = [ | |
| None, # Not an array | |
| "not_an_array", # String instead | |
| [generate_aggressive_text() for _ in range(random.randint(0, 10))], | |
| [None] * random.randint(1, 5), # Array of nulls | |
| [{}] * random.randint(1, 3), # Array of empty objects | |
| [], # Empty array | |
| ["A" * 10000], # Huge string in array | |
| list(range(10000)), # Huge array | |
| ] | |
| args[prop_name] = random.choice(aggressive_arrays) | |
| elif prop_type == "object": | |
| # Generate malformed objects | |
| aggressive_objects = [ | |
| None, # Not an object | |
| "not_an_object", # String instead | |
| [], # Array instead | |
| {}, # Empty object | |
| {generate_aggressive_text(): generate_aggressive_text()}, | |
| {"evil": "<script>alert('xss')</script>"}, | |
| {"\x00null\x00": "null_key"}, | |
| {"a" * 1000: "huge_key"}, | |
| ] | |
| args[prop_name] = random.choice(aggressive_objects) | |
| else: | |
| # Fallback for unknown types - try to break them | |
| args[prop_name] = random.choice( | |
| [ | |
| None, | |
| generate_aggressive_text(), | |
| _generate_aggressive_integer(), | |
| _generate_aggressive_float(), | |
| [], | |
| {}, | |
| ] | |
| ) | |
| # Ensure we have at least some arguments | |
| if not args and schema.get("properties"): | |
| # Fallback to basic property handling | |
| schema = tool.get("inputSchema") | |
| if not isinstance(schema, dict): | |
| schema = {} | |
| # Use the enhanced schema parser to generate aggressive values | |
| try: | |
| args = make_fuzz_strategy_from_jsonschema(schema, phase="aggressive") | |
| except Exception: | |
| args = {} | |
| # If the schema parser returned something other than a dict, create a default dict | |
| if not isinstance(args, dict): | |
| args = {} | |
| # Ensure we have at least some arguments | |
| if not args and schema.get("properties"): | |
| # Fallback to basic property handling |
🤖 Prompt for AI Agents
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py around lines 247 to
261: the code assumes tool.get("inputSchema") is a dict and later calls
schema.get("properties"), which will raise AttributeError if inputSchema is not
a dict; guard by validating the schema type immediately after assignment (e.g.,
if not isinstance(schema, dict): schema = {}), or mirror the realistic
strategy’s type check to coerce non-dict schemas to an empty dict before calling
make_fuzz_strategy_from_jsonschema and before any schema.get(...) calls so
subsequent logic always works with a dict.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
27-27: Expose pytest args, cache pip, and ensure Codecov runs on failure
Themcp_fuzzer/package exists, so--cov=mcp_fuzzeris correct. Consider:
- Adding a
pytest_argsworkflow input and passing${{ inputs.pytest_args }}topytestfor marker/-kflexibility.- Enabling
cache: 'pip'inactions/setup-python@v5to speed up CI runs (setup-python supports pip caching) (github.com).- Adding
if: always()to the Codecov upload step so coverage is uploaded even if tests fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: component-tests
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/component-tests.yml (1)
83-89: Gate Codecov upload on token and files to avoid failures.Prevents upload on repos without CODECOV_TOKEN or when no reports exist.
- - name: Upload coverage to Codecov - if: ${{ steps.changes.outputs.auth == 'true' || steps.changes.outputs.cli == 'true' || steps.changes.outputs.client == 'true' || steps.changes.outputs.fuzz_engine == 'true' || steps.changes.outputs.safety == 'true' || steps.changes.outputs.transport == 'true' }} + - name: Upload coverage to Codecov + if: ${{ (steps.changes.outputs.auth == 'true' || steps.changes.outputs.cli == 'true' || steps.changes.outputs.client == 'true' || steps.changes.outputs.fuzz_engine == 'true' || steps.changes.outputs.safety == 'true' || steps.changes.outputs.transport == 'true') && secrets.CODECOV_TOKEN != '' && hashFiles('coverage.*.xml') != '' }} uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} files: coverage.*.xml fail_ci_if_error: truemcp_fuzzer/transport/factory.py (1)
38-66: URL normalization preserves params/query/fragment — nice.This addresses prior feedback; conversions for sse:// and streamablehttp:// keep full routing info.
mcp_fuzzer/transport/http.py (1)
234-241: Do not await aiter_lines; iterate directly and drop inspect.Awaiting here is unnecessary and risks TypeError. Simplify to iterate the async iterator.
- # Iterate over streamed lines; support coroutine-returning aiter_lines - lines_iter = response.aiter_lines() - if inspect.iscoroutine(lines_iter): - lines_iter = await lines_iter - async for line in lines_iter: + # Iterate over streamed lines + async for line in response.aiter_lines():Also remove the unused import:
-import inspect
🧹 Nitpick comments (5)
.github/workflows/component-tests.yml (4)
59-82: Add integration tests step to meet PR objectives.Objectives include integration tests; workflow runs only unit tests. Add an integration step (component-gated if you have marks).
Example (adjust markers/paths as applicable):
+ - name: Run integration tests + if: ${{ steps.changes.outputs.auth == 'true' || steps.changes.outputs.cli == 'true' || steps.changes.outputs.client == 'true' || steps.changes.outputs.fuzz_engine == 'true' || steps.changes.outputs.safety == 'true' || steps.changes.outputs.transport == 'true' }} + run: | + pytest -vv tests/integration --cov-report=xml:coverage.integration.xml
20-23: Enable pip cache for faster runs.Small speedup with zero behavior change.
- name: Set up Python uses: actions/setup-python@v5 with: - python-version: '3.11' + python-version: '3.11' + cache: 'pip'
17-18: Either use git history for change detection or drop fetch-depth comment.You’re not diffing; the “Need full history” note is misleading.
Options:
- Keep fetch-depth: 0 and add a paths-filter step for PRs.
- Or remove the comment to avoid confusion.
7-7: Align naming: input says 'safety' but tests path/module are 'safety_system'.Minor clarity tweak to reduce confusion for operators.
- description: "Comma-separated components to run (auth,cli,client,fuzz_engine,safety,transport)" + description: "Comma-separated components to run (auth,cli,client,fuzz_engine,safety[safety_system],transport)"Also applies to: 75-77
mcp_fuzzer/transport/streamable_http.py (1)
398-406: Update session headers during streaming.Capture and persist session headers (e.g., mcp-session-id) before iterating, so downstream requests propagate updated session state.
try: - response.raise_for_status() + response.raise_for_status() + # Update session headers from streaming response + self._maybe_extract_session_headers(response) async for line in response.aiter_lines():
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/component-tests.yml(1 hunks)mcp_fuzzer/transport/base.py(2 hunks)mcp_fuzzer/transport/factory.py(1 hunks)mcp_fuzzer/transport/http.py(3 hunks)mcp_fuzzer/transport/sse.py(3 hunks)mcp_fuzzer/transport/streamable_http.py(1 hunks)tests/test_invariants.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_invariants.py
🧰 Additional context used
🧬 Code graph analysis (5)
mcp_fuzzer/transport/base.py (5)
tests/unit/transport/test_transport.py (4)
AsyncIterator(214-227)_stream_request(53-54)_stream_request(87-88)_stream_request(123-126)mcp_fuzzer/transport/http.py (1)
_stream_request(196-253)mcp_fuzzer/transport/sse.py (1)
_stream_request(68-115)mcp_fuzzer/transport/streamable_http.py (1)
_stream_request(372-415)mcp_fuzzer/transport/stdio.py (1)
_stream_request(243-267)
mcp_fuzzer/transport/streamable_http.py (6)
mcp_fuzzer/transport/base.py (1)
_stream_request(53-57)mcp_fuzzer/transport/http.py (1)
_stream_request(196-253)mcp_fuzzer/transport/sse.py (1)
_stream_request(68-115)mcp_fuzzer/transport/stdio.py (1)
_stream_request(243-267)mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)tests/unit/transport/test_streamable_http.py (3)
post(78-82)json(38-39)aiter_lines(58-64)
mcp_fuzzer/transport/sse.py (3)
mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)mcp_fuzzer/transport/base.py (2)
send_raw(21-22)_stream_request(53-57)mcp_fuzzer/transport/http.py (2)
send_raw(125-159)_stream_request(196-253)
mcp_fuzzer/transport/http.py (6)
mcp_fuzzer/transport/stdio.py (2)
_update_activity(44-49)_stream_request(243-267)mcp_fuzzer/transport/base.py (1)
_stream_request(53-57)mcp_fuzzer/transport/sse.py (1)
_stream_request(68-115)mcp_fuzzer/transport/streamable_http.py (1)
_stream_request(372-415)mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)tests/unit/transport/test_streamable_http.py (3)
post(78-82)json(38-39)aiter_lines(58-64)
mcp_fuzzer/transport/factory.py (4)
mcp_fuzzer/transport/http.py (1)
HTTPTransport(23-267)mcp_fuzzer/transport/streamable_http.py (1)
StreamableHTTPTransport(41-415)mcp_fuzzer/transport/sse.py (1)
SSETransport(11-145)mcp_fuzzer/transport/stdio.py (1)
StdioTransport(20-424)
🔇 Additional comments (4)
mcp_fuzzer/transport/streamable_http.py (1)
371-415: Streaming flow looks solid.Good: policy check, header sanitization, redirect handling with close, robust JSON/SSE line parsing, and guaranteed response close.
mcp_fuzzer/transport/base.py (1)
38-56: Clean abstraction for streaming API.The default stream_request delegator and abstract _stream_request contract are clear and composable.
mcp_fuzzer/transport/sse.py (1)
23-25: Confirm non-streaming SSE send_request impact
send_request on SSETransport now raises NotImplementedError, which breaks all send*_request methods in mcp_fuzzer/client/protocol_client.py and default get_tools/tools-call in mcp_fuzzer/transport/base.py. Verify no production code instantiates SSETransport for non-streaming requests or update factory/docs to guard against these calls.mcp_fuzzer/transport/http.py (1)
125-127: Good call updating activity in send_raw.Keeps activity tracking consistent across code paths.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests.yml (1)
39-45: Codecov step will fail on forked PRs due to secret usage; make upload resilient.Either remove the token (tokenless upload) or skip upload for forked PRs; also run upload even if tests fail to aid debugging. Add fetch-depth: 0 for better PR diff coverage.
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + fetch-depth: 0 @@ - - name: Upload coverage to Codecov - uses: codecov/codecov-action@v4 - with: - token: ${{ secrets.CODECOV_TOKEN }} - files: ./coverage.xml - fail_ci_if_error: true + - name: Upload coverage to Codecov + if: ${{ always() && !(github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) }} + uses: codecov/codecov-action@v4 + with: + files: ./coverage.xml + fail_ci_if_error: trueIf you prefer tokenless for public repos, also remove the token line and (optionally) add:
permissions: contents: read + id-token: write
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
5-7: Re‑enabled CI triggers — good fix and aligns with earlier review.push/pull_request on main are back; this addresses the prior “triggers missing” comment.
🧹 Nitpick comments (2)
.github/workflows/tests.yml (2)
15-17: Minimal permissions are correct; consider OIDC if going tokenless with Codecov.If you drop the Codecov token (recommended for public repos), add id-token: write.
Apply if switching to tokenless:
permissions: contents: read + id-token: write
18-20: Refine concurrency key to avoid cross-event collisions.Using head_ref for PRs prevents canceling unrelated branch runs.
- group: tests-${{ github.workflow }}-${{ github.ref }} + group: tests-${{ github.workflow }}-${{ github.head_ref || github.ref }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
🔇 Additional comments (2)
.github/workflows/tests.yml (2)
8-13: Manual dispatch preserved — LGTM.Keeping workflow_dispatch with an input is useful for ad‑hoc runs.
38-38: Coverage target confirmed:mcp_fuzzerexists at the repository root and is importable in tests, so the workflow’s--cov=mcp_fuzzerflag is correct.
There was a problem hiding this comment.
Summary of applied fixes for CodeRabbit review items:
Implemented (actionable):
- Workflows:
- .github/workflows/tests.yml: re-enabled push/pull_request triggers, kept workflow_dispatch, added minimal permissions and concurrency. (Also compatible with pip cache; retained coverage on mcp_fuzzer.)
- .github/workflows/component-tests.yml: added pip cache, exact token parsing for components input, separate per-component coverage files (coverage.*.xml), Codecov upload gated on token and files, added integration tests step, updated description to clarify safety vs safety_system.
- Markers and selection:
- pyproject: unit marker registered; tests/conftest.py already adds component markers at runtime.
- tests/add_markers.py: longest-prefix match and idempotent merge of existing pytestmark; improved docstring regex.
- Transport fixes:
- HTTPTransport._stream_request: iterate response.aiter_lines() directly (no await), removed inspect, ensured response close; safety checks and header sanitization consistent across transports.
- SSETransport: enforced safety policy with is_host_allowed/sanitize_headers, disabled env proxies and redirects, joined multi-line SSE data with "\n", kept send_request NotImplemented.
- StreamableHTTPTransport: update session headers during streaming.
- Base transport: typed stream_request and added abstract _stream_request.
- Factory: preserved params/query/fragment for sse:// and streamablehttp://; back-compat signature retained.
- Integration test cleanup:
- tests/integration/test_client_transport.py: correct httpx/json imports, patch AsyncClient.post, use _send_generic_request, assert two POSTs (initialize + request), optional skip handshake supported.
- Fuzzer/Schema/Invariant improvements:
- ProtocolFuzzer: widened types for server_response (dict|list), added BATCH_VALIDATION_TIMEOUT constant, tightened wait_for usage; results include invariant_violations and reflect invariant failures.
- Schema parser: added const fast-path, intersected allOf types, tuple-items support, stable uniqueItems hashing, type-aware recursion cutoff.
- Realistic tool strategy: schema-driven arrays/objects, numeric arrays honor constraints, guarded overwrites of format fields, backfill required fields.
- Aggressive tool strategy: guarded parser errors, backfill required keys, safer defaults; handled non-dict schemas.
- Invariants: treat request envelopes as invalid in response context; fixed valid notification tests to include method.
Tests and lint:
- tox: all tests passing locally; ruff clean.
Optional nits (tracked but not blocking):
- Docs tweaks in docs/reference.md and docs/architecture.md (bullet grammar/numbering) can be addressed in a follow-up docs PR if desired.
- tests/README.md code-fence language and examples; happy to include in a docs polish commit.
If you want any of the optional documentation nits folded into this PR, I can push those edits as well. Otherwise, from my side, all actionable CodeRabbit comments are resolved.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
.github/workflows/component-tests.yml (1)
72-95: Per-component jobs and unique coverage filenames are solid.Good isolation, clear gating, and no coverage clobbering. Matches prior feedback.
mcp_fuzzer/transport/sse.py (2)
28-33: Good parity: redirects disabled and env proxies ignored.AsyncClient now sets follow_redirects=False and trust_env=False across all paths; aligns with safety policy and other transports.
Also applies to: 63-67, 85-89
155-157: Spec-correct multi-line join.Joining data lines with "\n" matches SSE spec. Good catch.
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (3)
210-237: RespectadditionalProperties: falsewhen generating extra properties.The code correctly checks
additionalPropertiesbefore adding extra properties in Lines 213-218 and 221-225. Good implementation!
124-130: Excellent implementation of const value handling!The implementation properly handles const values with appropriate behavior for both realistic and aggressive phases, avoiding returning the const value in aggressive mode when trying to generate violations.
32-60: allOf merge loses restrictive constraints.The current
_merge_allOffunction arbitrarily preserves first/last values for non-property/required fields, which can produce invalid schemas when constraints conflict. For example, if one schema hasminimum: 10and another hasminimum: 20, the merged schema should useminimum: 20(the more restrictive value). Similarly,maxLengthshould take the minimum of all values.Apply this fix to properly merge numeric and length constraints:
def _merge_allOf(schemas: List[Dict[str, Any]]) -> Dict[str, Any]: """Deep merge for allOf schemas.""" merged: Dict[str, Any] = {} props: Dict[str, Any] = {} required: List[str] = [] merged_types = None # track intersection of declared types + # Collect numeric/length constraints for proper merging + mins = {"minimum": [], "minLength": [], "minItems": [], "minProperties": []} + maxs = {"maximum": [], "maxLength": [], "maxItems": [], "maxProperties": []} + exclusive_mins = [] + exclusive_maxs = [] + for s in schemas: if "properties" in s and isinstance(s["properties"], dict): props.update(s["properties"]) if "required" in s and isinstance(s["required"], list): required.extend([r for r in s["required"] if isinstance(r, str)]) t = s.get("type") if t is not None: tset = set(t if isinstance(t, list) else [t]) merged_types = tset if merged_types is None else (merged_types & tset) + + # Collect constraint values + for k in mins: + if k in s and isinstance(s[k], (int, float)): + mins[k].append(s[k]) + for k in maxs: + if k in s and isinstance(s[k], (int, float)): + maxs[k].append(s[k]) + if "exclusiveMinimum" in s: + exclusive_mins.append(s["exclusiveMinimum"]) + if "exclusiveMaximum" in s: + exclusive_maxs.append(s["exclusiveMaximum"]) + for k, v in s.items(): - if k not in ("properties", "required"): + if k not in ("properties", "required", "type") and k not in mins and k not in maxs: merged[k] = v if k not in merged else merged[k] + + # Apply most restrictive constraints + for k, vals in mins.items(): + if vals: + merged[k] = max(vals) # Most restrictive minimum + for k, vals in maxs.items(): + if vals: + merged[k] = min(vals) # Most restrictive maximum + + # Handle exclusive bounds (if any schema sets exclusive, the merged should be exclusive) + if exclusive_mins: + merged["exclusiveMinimum"] = max(v if isinstance(v, (int, float)) else merged.get("minimum", 0) for v in exclusive_mins) + if exclusive_maxs: + merged["exclusiveMaximum"] = min(v if isinstance(v, (int, float)) else merged.get("maximum", 0) for v in exclusive_maxs) + if props: merged["properties"] = props if required: merged["required"] = sorted(set(required)) if merged_types: if len(merged_types) > 1: merged["type"] = list(merged_types) else: merged["type"] = next(iter(merged_types)) return mergedmcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
243-256: Excellent error handling for schema parser integration!The implementation properly handles parser failures with try/except and includes appropriate type checking for non-dict schemas. This ensures robust fallback behavior when the schema parser encounters issues.
🧹 Nitpick comments (7)
.github/workflows/component-tests.yml (3)
31-64: Optional: auto-detect components when no input is provided.Objectives mention “only run components that have code changes.” Currently the else-branch runs all. Consider diffing against
origin/mainto set flags automatically wheninputs.componentsis empty.Sketch:
CHANGED=$(git diff --name-only origin/main...HEAD || true) AUTH_CHANGES=$(echo "$CHANGED" | grep -Eq '^mcp_fuzzer/auth/|^tests/(unit/auth|integration)/' && echo true || echo false) # repeat per component…
15-18: Nit:fetch-depth: 0is unused.If you don’t implement git-diff detection, you can drop full history to speed up checkout.
25-30: Nit: consolidate dev deps.If you have an extras entry (e.g.,
.[dev]), install once instead of multiple pip installs to save time.mcp_fuzzer/transport/sse.py (2)
40-57: Handle multi-line SSE events in send_raw, not just first data line.Current logic may miss/garble payloads split across multiple data: lines. Buffer lines until a blank line, then parse once with _parse_sse_event.
Apply:
- for line in response.text.splitlines(): - if line.startswith("data:"): - try: - data = json.loads(line[len("data:") :].strip()) - if "error" in data: - raise Exception(f"Server error: {data['error']}") - return data.get("result", data) - except json.JSONDecodeError: - logging.error("Failed to parse SSE data line as JSON") - continue + buffer: List[str] = [] + def flush_once() -> Optional[Dict[str, Any]]: + if not buffer: + return None + event_text = "\n".join(buffer) + buffer.clear() + try: + data = SSETransport._parse_sse_event(event_text) + except json.JSONDecodeError: + logging.error("Failed to parse SSE data as JSON") + return None + if data is None: + return None + if "error" in data: + raise Exception(f"Server error: {data['error']}") + return data.get("result", data) + + for line in response.text.splitlines(): + if not line.strip(): + result = flush_once() + if result is not None: + return result + continue + buffer.append(line) + result = flush_once() + if result is not None: + return result
15-18: Consider explicit SSE-friendly headers.Add Cache-Control: no-cache to reduce intermediaries’ buffering.
Apply:
self.headers = { "Accept": "text/event-stream", "Content-Type": "application/json", + "Cache-Control": "no-cache", }mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
342-344: Consider improving error handling specificity.The bare
except Exceptionon line 342 could mask important errors during pattern generation. Consider catching specific exceptions that_generate_string_from_patternmight raise.- except Exception: # Handle specific exceptions when possible + except (ValueError, TypeError, AttributeError): # Known exceptions from pattern parsing # Fallback if pattern generation fails passmcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
156-156: Fix potential string slicing issue for exact length enforcement.In line 156, when generating digits only, the code uses
("100" * ((length + 2) // 3))[:length]to enforce exact length. However, this might not produce exactlylengthcharacters whenlengthis not divisible by 3.- ("100" * ((length + 2) // 3))[:length], # digits only; enforce exact length + ("".join(str(i % 10) for i in range(length))), # digits only; exact length
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/component-tests.yml(1 hunks)mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/schema_parser.py(1 hunks)mcp_fuzzer/transport/sse.py(3 hunks)mcp_fuzzer/transport/streamable_http.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp_fuzzer/transport/streamable_http.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T11:50:01.877Z
Learnt from: Agent-Hellboy
PR: Agent-Hellboy/mcp-server-fuzzer#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/fuzz_engine/strategy/aggressive/tool_strategy.py
🧬 Code graph analysis (2)
mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (1)
mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (1)
make_fuzz_strategy_from_jsonschema(62-159)
mcp_fuzzer/transport/sse.py (3)
mcp_fuzzer/transport/base.py (3)
TransportProtocol(13-101)send_raw(21-22)_stream_request(53-57)mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)mcp_fuzzer/transport/streamable_http.py (2)
send_raw(173-278)_stream_request(372-417)
🪛 actionlint (1.7.7)
.github/workflows/component-tests.yml
102-102: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (8)
.github/workflows/component-tests.yml (2)
43-54: Exact token parsing looks good.Correctly avoids the “cli” vs “client” substring bug and maps
safety|safety_systemconsistently.
96-100: No change required: pytest-cov target already defined in pyproject.toml
The project’s pyproject.toml under[tool.pytest.ini_options]includes--cov=mcp_fuzzerand[tool.coverage.run]specifies the source, so pytest will measure coverage for mcp_fuzzer by default.mcp_fuzzer/transport/sse.py (1)
23-25: No callers rely on SSETransport.send_request returning data
The only invocation is in test_transport.py, which asserts NotImplementedError—so removing the non-streaming path has no downstream impact.mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (2)
1-704: Well-structured JSON Schema parser implementation!The module provides comprehensive support for JSON Schema-based fuzzing with clear separation between "realistic" and "aggressive" modes. The implementation handles edge cases, recursion depth limits, and various schema types properly.
536-547: Robust multipleOf implementation for integers.The implementation correctly handles the
multipleOfconstraint by finding valid multiples within the range. The error handling ensures fallback to a valid value even if the calculation fails.mcp_fuzzer/fuzz_engine/strategy/aggressive/tool_strategy.py (3)
257-260: Good defensive programming with type check.The additional type check for the parser result ensures that even if the parser returns a non-dict value without raising an exception, the code handles it gracefully.
262-291: Well-designed fallback mechanism for argument generation.The fallback path provides comprehensive coverage when the schema parser fails or returns empty results. The 80% inclusion probability for optional properties and type-specific generation logic ensures good test coverage even in fallback scenarios.
292-296: Important: Required fields enforcement.Great addition to ensure that required fields are always present in the generated arguments, even if they were missed during the initial generation or fallback process.
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
.github/workflows/tests.yml (2)
46-46: Nit: guard Codecov step when coverage.xml is missing.With
if: always(), uploads run even if tests fail and no coverage is produced, causing noisy failures.Apply:
- - name: Upload coverage to Codecov - if: ${{ always() }} + - name: Upload coverage to Codecov + if: ${{ always() && hashFiles('coverage.xml') != '' }} uses: codecov/codecov-action@v4 with: files: ./coverage.xml fail_ci_if_error: true
14-17: Optional: allow marker/selection on PRs too.
pytest_argsonly works for manual runs. If you want selective runs on PRs based on changed components, consider adding a paths-filter step and pass computed markers via env to pytest..github/workflows/component-tests.yml (4)
15-16: Optional: set full fetch depth for better PR context.Helps coverage upload and potential git-based tooling.
Apply:
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + fetch-depth: 0
11-13: Optional: add concurrency to avoid duplicate manual runs.Prevents overlapping component test executions.
Apply:
jobs: + # Avoid duplicate runs of this workflow on the same ref/inputs +concurrency: + group: component-tests-${{ github.workflow }}-${{ github.ref }}-${{ github.event.inputs.components }} + cancel-in-progress: true
3-10: Optional: support PR-triggered component selection.If you want automatic per-component runs on PRs, add
pull_requesttrigger and detect changes with a paths filter, falling back to all on empty matches. Keepworkflow_dispatchfor manual overrides.
99-104: Optional: make Codecov non-blocking for manual runs.If uploads fail (e.g., Codecov outage), you may not want to fail the whole manual workflow.
Apply:
- name: Upload coverage to Codecov if: ${{ (steps.changes.outputs.auth == 'true' || steps.changes.outputs.cli == 'true' || steps.changes.outputs.client == 'true' || steps.changes.outputs.fuzz_engine == 'true' || steps.changes.outputs.safety == 'true' || steps.changes.outputs.transport == 'true') && hashFiles('coverage.*.xml') != '' }} uses: codecov/codecov-action@v4 with: files: coverage.*.xml - fail_ci_if_error: true + fail_ci_if_error: falsemcp_fuzzer/types.py (2)
9-9: Introduce a JSON container type alias to clarify intent and avoid repetition.This field models JSON-RPC single vs. batch responses. A small alias improves readability and keeps usage consistent across the codebase.
-from typing import Any, Dict, List, Optional, TypedDict, Union +from typing import Any, Dict, List, Optional, TypedDict, Union +try: + from typing import TypeAlias # py>=3.10 +except ImportError: # py<3.10 + from typing_extensions import TypeAlias + +# JSON containers (object or array) used for server responses +JSONContainer: TypeAlias = Union[Dict[str, Any], List[Any]]
23-23: Use structured violation records instead of plain strings.
- In mcp_fuzzer/types.py change
toinvariant_violations: List[str]and define a local TypedDict:invariant_violations: List[InvariantViolationDict]class InvariantViolationDict(TypedDict, total=False): id: str message: str severity: str # “info” | “warning” | “error” path: Optional[str] context: Dict[str, Any]- In mcp_fuzzer/fuzz_engine/fuzzer/protocol_fuzzer.py update all sites that do
to build and append instances of InvariantViolationDict instead.invariant_violations.extend([str(v) …]) invariant_violations.append(str(e))- If there’s a ProtocolFuzzResult type, add the new invariant_violations field there for consistency with FuzzDataResult.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/component-tests.yml(1 hunks).github/workflows/tests.yml(1 hunks)mcp_fuzzer/transport/factory.py(1 hunks)mcp_fuzzer/types.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp_fuzzer/transport/factory.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (4)
.github/workflows/tests.yml (3)
19-22: Good: OIDC permissions set for tokenless Codecov.
id-token: writeenables Codecov v4 OIDC uploads without a repo secret. Nice.
23-25: Good: sensible concurrency guard.Prevents duplicate runs on the same ref.
31-33: Good: full fetch depth for better coverage attribution.
fetch-depth: 0helps Codecov with commit parent detection..github/workflows/component-tests.yml (1)
29-69: Good: exact token parsing for components.Switching to tokenized case matching avoids the previous substring bug (cli vs client).
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/tests.yml (4)
19-22: Clarify permissions: remove unused id-token or switch fully to tokenless Codecov.Currently you grant id-token: write but still use a Codecov secret. Choose one:
- If you’ll keep the secret-based upload, drop id-token.
- If you want tokenless OIDC (preferred), remove the secret and the probing step below and rely on id-token.
Diff to drop id-token (secret-based path):
permissions: contents: read - id-token: write
36-39: Strengthen pip cache key for reliability.setup-python’s pip cache benefits from explicit dependency paths (pyproject/requirements). Add cache-dependency-path to avoid cache misses.
uses: actions/setup-python@v5 with: python-version: '3.11' - cache: 'pip' + cache: 'pip' + cache-dependency-path: | + pyproject.toml + requirements*.txt
45-46: Make pytest_args robust across events.On push/PR, github.event.inputs may be null. Use an env with a guarded expression, then reference $PYTEST_ARGS.
- - name: Run tests with coverage - run: | - pytest -vv --cov=mcp_fuzzer --cov-report=xml ${{ github.event.inputs.pytest_args }} + - name: Run tests with coverage + env: + PYTEST_ARGS: ${{ github.event_name == 'workflow_dispatch' && (github.event.inputs.pytest_args || '') || '' }} + run: | + pytest -vv --cov=mcp_fuzzer --cov-report=xml $PYTEST_ARGS
47-56: Simplify Codecov: prefer tokenless OIDC and remove the probing step.Since permissions include id-token: write, you can drop the secret + check step and rely on OIDC. Keep the fork safeguard. Also consider not failing CI on upload hiccups.
- - name: Check Codecov token - id: codecov_token - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - run: | - if [ -n "$CODECOV_TOKEN" ]; then - echo "has_token=true" >> $GITHUB_OUTPUT - else - echo "has_token=false" >> $GITHUB_OUTPUT - fi - name: Upload coverage to Codecov - if: ${{ steps.codecov_token.outputs.has_token == 'true' && hashFiles('coverage.xml') != '' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.fork == false) }} + if: ${{ hashFiles('coverage.xml') != '' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.fork == false) }} uses: codecov/codecov-action@v4 with: - token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage.xml - fail_ci_if_error: true + use_oidc: true + fail_ci_if_error: falseAdditional: Pin actions to a commit SHA for supply-chain hardening (instead of @v4).
Also applies to: 58-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (3)
.github/workflows/tests.yml (3)
5-17: Triggers and manual inputs look good.push/pull_request on main plus workflow_dispatch with a pytest_args input aligns with the PR goals for selective runs. No issues here.
23-25: Good use of concurrency.The group key dedupes runs per branch/PR and cancel-in-progress prevents wasted minutes.
31-33: Checkout with full history: LGTM.fetch-depth: 0 helps coverage/parent detection and change-based tooling.
…, schema parser, and codecov fixes
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_fuzzer/fuzz_engine/executor.py (1)
171-175: Deadlock risk: double-acquire of self._semaphore in execute_batch + execute()._bounded_execute_and_track acquires the semaphore, and execute() also acquires it. Under load, tasks will take the first permit and block forever on the second, stalling the batch.
Apply this diff to avoid the double-acquire (keep concurrency enforcement inside execute):
- async def _bounded_execute_and_track(op, args, kwargs): - # Acquire semaphore before execution and release after - async with self._semaphore: - return await self._execute_and_track(op, args, kwargs) + async def _bounded_execute_and_track(op, args, kwargs): + # Concurrency is enforced inside execute(); avoid double-acquire deadlock + return await self._execute_and_track(op, args, kwargs)Also applies to: 73-77
♻️ Duplicate comments (3)
mcp_fuzzer/types.py (2)
11-12: Good alias; matches prior guidance.JSONContainer is clear and centralizes the shape. Consider exporting via all for downstream use.
+# Explicit re-exports (optional) +__all__ = [ + "JSONContainer", + "FuzzDataResult", + "ProtocolFuzzResult", + "ToolFuzzResult", + "BatchExecutionResult", + "SafetyCheckResult", + "TransportStats", +]
20-22: Clear “absent vs None” contract.The comment resolves earlier ambiguity; signature is correct.
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (1)
12-13: Register project-wide markers to avoid PytestUnknownMarkWarning.Add unit, fuzz_engine, fuzzer to pytest config.
Apply in pyproject.toml (or pytest.ini):
[tool.pytest.ini_options] -markers = [ - "auth: tests that exercise authentication", -] +markers = [ + "auth: tests that exercise authentication", + "unit: unit tests", + "fuzz_engine: fuzz engine component", + "fuzzer: tool fuzzer component", +]Verify:
#!/bin/bash fd -a 'pyproject.toml|pytest.ini|tox.ini' -x rg -nC1 -i 'markers|ini_options'
🧹 Nitpick comments (10)
tests/unit/transport/test_transport.py (5)
6-11: Remove unused imports to avoid lint noise.
os,uuid, andcallare unused.-import os -import uuid -from unittest.mock import AsyncMock, MagicMock, call, patch +from unittest.mock import AsyncMock, MagicMock, patch
154-160: Consider asserting exact header values.Verifying
AcceptandContent-Typevalues (e.g.,application/json) would harden the contract.
162-176: Rename for accuracy: the test exercisessend_raw, notsend_request.-async def test_http_transport_send_request(http_transport): - """Test HTTPTransport send_request method.""" +async def test_http_transport_send_raw(http_transport): + """Test HTTPTransport send_raw method."""
386-392: Avoid shadow field_sys; the module-level patch is sufficient.
StdioTransport._send_request/_stream_requestreference the patched modulesys; settingtransport._sysis unnecessary.- transport = StdioTransport("test_command") - transport._sys = mock_sys # Attach the mock to the transport + transport = StdioTransport("test_command")
23-24: Component markers align with the PR’s component-based test strategy.Consider wiring CI to select tests by component label from changed paths (e.g., pytest -k "transport or common") to satisfy “only run components with code changes”.
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (3)
86-114: Stabilize test_fuzz_tools to avoid flakiness from safety filter.Patch is_safe_tool_call to True for this test so “at least some runs were successful” is deterministic.
- @pytest.mark.asyncio - @patch("mcp_fuzzer.fuzz_engine.fuzzer.tool_fuzzer.logging") - async def test_fuzz_tools(self, mock_logging): + @pytest.mark.asyncio + @patch("mcp_fuzzer.fuzz_engine.fuzzer.tool_fuzzer.is_safe_tool_call", return_value=True) + @patch("mcp_fuzzer.fuzz_engine.fuzzer.tool_fuzzer.logging") + async def test_fuzz_tools(self, mock_logging, mock_is_safe): """Test fuzzing multiple tools."""
442-447: Make log assertion less brittle.Avoid stringifying Call objects; inspect args directly.
- calls = [str(c) for c in mock_logger.info.call_args_list] - self.assertTrue(any("test_tool" in c for c in calls)) + calls = mock_logger.info.call_args_list + self.assertTrue(any(("test_tool" in (c.args[0] if c.args else "")) for c in calls))
519-522: Close executors to prevent resource leakage.Shut down fuzzer executors in this test.
- results1 = await fuzzer1.fuzz_tool(tool, runs=1) - results2 = await fuzzer2.fuzz_tool(tool, runs=1) + results1 = await fuzzer1.fuzz_tool(tool, runs=1) + results2 = await fuzzer2.fuzz_tool(tool, runs=1) + await fuzzer1.shutdown() + await fuzzer2.shutdown()mcp_fuzzer/fuzz_engine/strategy/schema_parser.py (2)
149-163: Recursion cap off-by-one.Use >= so MAX_RECURSION_DEPTH is the true ceiling.
- if recursion_depth > MAX_RECURSION_DEPTH: + if recursion_depth >= MAX_RECURSION_DEPTH:
267-289: Honor maxProperties.Currently ignored; clamp extras in realistic mode to not exceed maxProperties.
# Ensure we meet minProperties constraint @@ # In aggressive mode, sometimes add extra properties (if allowed) @@ - return result + # Respect maxProperties if present (best-effort) + max_props = schema.get("maxProperties") + if isinstance(max_props, int) and max_props >= 0 and len(result) > max_props: + # Keep required first when trimming + required_set = set(required) + ordered = dict( + list((k, v) for k, v in result.items() if k in required_set) + + list((k, v) for k, v in result.items() if k not in required_set) + ) + result = dict(list(ordered.items())[:max_props]) + return result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/component-tests.yml(1 hunks)docs/reference.md(2 hunks)mcp_fuzzer/fuzz_engine/executor.py(1 hunks)mcp_fuzzer/fuzz_engine/strategy/schema_parser.py(1 hunks)mcp_fuzzer/transport/sse.py(3 hunks)mcp_fuzzer/types.py(1 hunks)tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py(13 hunks)tests/unit/transport/test_transport.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/reference.md
- .github/workflows/component-tests.yml
- mcp_fuzzer/transport/sse.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/transport/test_transport.py (6)
tests/unit/transport/test_streamable_http.py (4)
json(38-39)aiter_lines(58-64)raise_for_status(34-36)raise_for_status(55-56)mcp_fuzzer/transport/http.py (5)
HTTPTransport(23-267)send_request(71-123)send_raw(125-159)send_notification(161-184)_stream_request(196-253)mcp_fuzzer/transport/sse.py (6)
SSETransport(11-198)send_request(20-25)send_raw(27-57)send_notification(59-74)_stream_request(76-168)_parse_sse_event(171-198)mcp_fuzzer/transport/stdio.py (6)
StdioTransport(20-424)send_request(172-196)send_raw(198-211)send_notification(232-241)_send_request(213-230)_stream_request(243-267)mcp_fuzzer/transport/base.py (8)
TransportProtocol(13-101)send_request(15-18)send_raw(21-22)send_notification(25-28)_stream_request(53-57)connect(30-32)disconnect(34-36)stream_request(38-50)mcp_fuzzer/transport/factory.py (1)
create_transport(9-75)
tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (2)
mcp_fuzzer/fuzz_engine/fuzzer/tool_fuzzer.py (1)
ToolFuzzer(21-257)mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py (1)
fuzz_tool_arguments(54-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (8)
tests/unit/transport/test_transport.py (6)
27-33: Abstract base checks look good.
39-63: Connection no-op coverage is fine.
479-491: Factory HTTP/HTTPS coverage looks good.
493-498: SSE factory mapping assertion is solid.
500-504: Use a non-empty stdio command to avoid ambiguous semantics.Some implementations require a command;
stdio:may be treated as empty.-def test_create_transport_stdio(): +def test_create_transport_stdio(): """Test create_transport with stdio URL.""" - transport = create_transport("stdio:") + transport = create_transport("stdio://python") assert isinstance(transport, StdioTransport)Would you like me to add an assertion on the parsed command (e.g., equals
"python")?
506-509: Invalid scheme test is correct.mcp_fuzzer/types.py (1)
27-27: Invariant_violations always a List[str]
Per-run and default result paths initializeinvariant_violations = []and only extend or assign aList[str]; omission when empty aligns withtotal=False.tests/unit/fuzz_engine/fuzzer/test_tool_fuzzer.py (1)
130-149: AsyncMock usage LGTM.Side effect coroutine matches fuzz_tool signature; covers error path cleanly.
| # Proactively cancel outstanding tasks and wait for them to finish | ||
| for task in list(self._running_tasks): | ||
| task.cancel() | ||
| await asyncio.gather(*self._running_tasks, return_exceptions=True) | ||
| finally: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cancel-then-await can hang indefinitely; bound the second wait.
If tasks ignore cancellation or are stuck in uninterruptible awaits, the unconditional gather() can block shutdown forever.
Apply this diff to add a bounded “grace” wait and log if tasks remain:
for task in list(self._running_tasks):
task.cancel()
- await asyncio.gather(*self._running_tasks, return_exceptions=True)
+ try:
+ await asyncio.wait_for(
+ asyncio.gather(*self._running_tasks, return_exceptions=True),
+ timeout=min(2.0, timeout),
+ )
+ except asyncio.TimeoutError:
+ self._logger.error(
+ "Forced shutdown still timed out; %d tasks may still be running",
+ sum(1 for t in self._running_tasks if not t.done()),
+ )📝 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.
| # Proactively cancel outstanding tasks and wait for them to finish | |
| for task in list(self._running_tasks): | |
| task.cancel() | |
| await asyncio.gather(*self._running_tasks, return_exceptions=True) | |
| finally: | |
| # Proactively cancel outstanding tasks and wait for them to finish | |
| for task in list(self._running_tasks): | |
| task.cancel() | |
| try: | |
| await asyncio.wait_for( | |
| asyncio.gather(*self._running_tasks, return_exceptions=True), | |
| timeout=min(2.0, timeout), | |
| ) | |
| except asyncio.TimeoutError: | |
| self._logger.error( | |
| "Forced shutdown still timed out; %d tasks may still be running", | |
| sum(1 for t in self._running_tasks if not t.done()), | |
| ) | |
| finally: |
| # Ensure the set is cleaned up | ||
| self._running_tasks = { | ||
| t for t in self._running_tasks if not t.cancelled() and not t.done() | ||
| } |
There was a problem hiding this comment.
Rebinding _running_tasks breaks done callbacks; mutate in place instead.
add_done_callback(self._running_tasks.discard) captures the old set object. Reassigning self._running_tasks creates a new set, so callbacks remove from the stale set, leaking entries in the current set.
Apply this diff to preserve the set identity and safely remove completed/cancelled tasks:
- # Ensure the set is cleaned up
- self._running_tasks = {
- t for t in self._running_tasks if not t.cancelled() and not t.done()
- }
+ # Ensure the set is cleaned up without rebinding (callbacks rely on identity)
+ to_remove = {t for t in tuple(self._running_tasks) if t.done() or t.cancelled()}
+ self._running_tasks.difference_update(to_remove)📝 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.
| # Ensure the set is cleaned up | |
| self._running_tasks = { | |
| t for t in self._running_tasks if not t.cancelled() and not t.done() | |
| } | |
| # Ensure the set is cleaned up without rebinding (callbacks rely on identity) | |
| to_remove = {t for t in tuple(self._running_tasks) if t.done() or t.cancelled()} | |
| self._running_tasks.difference_update(to_remove) |
🤖 Prompt for AI Agents
mcp_fuzzer/fuzz_engine/executor.py around lines 250-253: reassigning
self._running_tasks to a new set breaks existing
add_done_callback(self._running_tasks.discard) callbacks because they hold a
reference to the old set; instead mutate the existing set in place to preserve
identity. Replace the reassignment with an in-place removal of
completed/cancelled tasks (e.g., use set.difference_update(...) or iterate and
call self._running_tasks.discard(...) for each finished task) so callbacks
remove items from the live set and no entries are leaked.
| def _merge_allOf(schemas: List[Dict[str, Any]]) -> Dict[str, Any]: | ||
| """ | ||
| Deep merge for allOf schemas. | ||
| Combines properties and required fields, intersects types, | ||
| and applies the most restrictive constraints (max of minimums, min of maximums). | ||
| """ | ||
| merged: Dict[str, Any] = {} | ||
| props: Dict[str, Any] = {} | ||
| required: List[str] = [] | ||
| merged_types = None # track intersection of declared types | ||
|
|
||
| # Track min/max constraint values | ||
| min_constraints = { | ||
| "minLength": None, | ||
| "minItems": None, | ||
| "minProperties": None, | ||
| "minimum": None, | ||
| "exclusiveMinimum": None, | ||
| } | ||
| max_constraints = { | ||
| "maxLength": None, | ||
| "maxItems": None, | ||
| "maxProperties": None, | ||
| "maximum": None, | ||
| "exclusiveMaximum": None, | ||
| } | ||
|
|
||
| for s in schemas: | ||
| # Merge properties | ||
| if "properties" in s and isinstance(s["properties"], dict): | ||
| props.update(s["properties"]) | ||
|
|
||
| # Merge required fields | ||
| if "required" in s and isinstance(s["required"], list): | ||
| required.extend([r for r in s["required"] if isinstance(r, str)]) | ||
|
|
||
| # Intersect types | ||
| t = s.get("type") | ||
| if t is not None: | ||
| tset = set(t if isinstance(t, list) else [t]) | ||
| merged_types = tset if merged_types is None else (merged_types & tset) | ||
|
|
||
| # Handle const values | ||
| if "const" in s: | ||
| merged["const"] = s["const"] | ||
|
|
||
| # Track min constraints (take maximum value) | ||
| for key in min_constraints: | ||
| if key in s: | ||
| current = min_constraints[key] | ||
| new_val = s[key] | ||
| if current is None or (new_val is not None and new_val > current): | ||
| min_constraints[key] = new_val | ||
|
|
||
| # Track max constraints (take minimum value) | ||
| for key in max_constraints: | ||
| if key in s: | ||
| current = max_constraints[key] | ||
| new_val = s[key] | ||
| if current is None or (new_val is not None and new_val < current): | ||
| max_constraints[key] = new_val | ||
|
|
||
| # Copy other fields (non-constraint) | ||
| for k, v in s.items(): | ||
| if ( | ||
| k not in ("properties", "required", "type", "const") | ||
| and k not in min_constraints | ||
| and k not in max_constraints | ||
| ): | ||
| merged[k] = v if k not in merged else merged[k] | ||
|
|
||
| # Apply merged properties | ||
| if props: | ||
| merged["properties"] = props | ||
|
|
||
| # Apply merged required fields | ||
| if required: | ||
| merged["required"] = sorted(set(required)) | ||
|
|
||
| # Apply merged types | ||
| if merged_types: | ||
| if len(merged_types) > 1: | ||
| merged["type"] = list(merged_types) | ||
| else: | ||
| merged["type"] = next(iter(merged_types)) | ||
|
|
||
| # Apply min constraints | ||
| for key, value in min_constraints.items(): | ||
| if value is not None: | ||
| merged[key] = value | ||
|
|
||
| # Apply max constraints | ||
| for key, value in max_constraints.items(): | ||
| if value is not None: | ||
| merged[key] = value | ||
|
|
||
| return merged |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
allOf merge: fix const conflicts and exclusive merging; avoid mixed-type errors.*
Current merge overwrites const and mixes bool/number exclusive*; can yield invalid schemas.
- # Track min/max constraint values
- min_constraints = {
- "minLength": None,
- "minItems": None,
- "minProperties": None,
- "minimum": None,
- "exclusiveMinimum": None,
- }
- max_constraints = {
- "maxLength": None,
- "maxItems": None,
- "maxProperties": None,
- "maximum": None,
- "exclusiveMaximum": None,
- }
+ # Track min/max constraint values (exclusive* handled separately)
+ min_constraints = {"minLength": None, "minItems": None, "minProperties": None, "minimum": None}
+ max_constraints = {"maxLength": None, "maxItems": None, "maxProperties": None, "maximum": None}
+ # Track exclusives (draft-04 booleans vs draft-06+ numbers)
+ exc_min_bool = False
+ exc_max_bool = False
+ exc_min_num: Optional[float] = None
+ exc_max_num: Optional[float] = None
+ # Track consts to detect conflicts
+ const_values: List[Any] = []
@@
- # Handle const values
- if "const" in s:
- merged["const"] = s["const"]
+ # Collect const values
+ if "const" in s:
+ const_values.append(s["const"])
@@
- # Track min constraints (take maximum value)
+ # Track min constraints (take maximum value)
for key in min_constraints:
if key in s:
current = min_constraints[key]
new_val = s[key]
if current is None or (new_val is not None and new_val > current):
min_constraints[key] = new_val
@@
- # Track max constraints (take minimum value)
+ # Track max constraints (take minimum value)
for key in max_constraints:
if key in s:
current = max_constraints[key]
new_val = s[key]
if current is None or (new_val is not None and new_val < current):
max_constraints[key] = new_val
+
+ # Track exclusiveMinimum/Maximum
+ em = s.get("exclusiveMinimum", None)
+ if isinstance(em, bool) and em:
+ exc_min_bool = True
+ elif isinstance(em, (int, float)):
+ exc_min_num = float(em) if exc_min_num is None else max(exc_min_num, float(em))
+ ex = s.get("exclusiveMaximum", None)
+ if isinstance(ex, bool) and ex:
+ exc_max_bool = True
+ elif isinstance(ex, (int, float)):
+ exc_max_num = float(ex) if exc_max_num is None else min(exc_max_num, float(ex))
@@
# Apply min constraints
for key, value in min_constraints.items():
if value is not None:
merged[key] = value
@@
# Apply max constraints
for key, value in max_constraints.items():
if value is not None:
merged[key] = value
+
+ # Apply exclusives with stricter interpretation
+ if exc_min_num is not None:
+ merged["exclusiveMinimum"] = exc_min_num
+ elif exc_min_bool:
+ merged["exclusiveMinimum"] = True
+ if exc_max_num is not None:
+ merged["exclusiveMaximum"] = exc_max_num
+ elif exc_max_bool:
+ merged["exclusiveMaximum"] = True
+
+ # Apply const only if all consts agree
+ if const_values:
+ if len(set(map(repr, const_values))) == 1:
+ merged["const"] = const_values[0]
+ else:
+ # conflicting consts => unsatisfiable; omit const to allow aggressive generation
+ merged.pop("const", None)Also, if merged_types becomes an empty set, consider signaling unsatisfiable (e.g., store merged["type"] = [] or set a marker) so callers can choose aggressive fallback.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the schema_parser file
echo "Locating schema_parser.py:"
fd -g schema_parser.py
# Show the _merge_allOf function definition
echo "Showing _merge_allOf implementation:"
rg -nP 'def _merge_allOf' -C5 mcp_fuzzer/fuzz_engine/strategy/schema_parser.py
# Search tests for allOf usage
echo "Tests referencing allOf:"
rg -n 'allOf' -C2 .
# Search tests for merge_allOf usage
echo "Tests referencing merge_allOf:"
rg -n 'merge_allOf' -C2 .Length of output: 6960
🏁 Script executed:
#!/bin/bash
set -e
echo "Code references to exclusiveMinimum:"
rg -n 'exclusiveMinimum' -C2 mcp_fuzzer/fuzz_engine/strategy/schema_parser.py || true
echo "Test references to exclusiveMinimum:"
rg -n 'exclusiveMinimum' -C2 tests || true
echo "Code references to const:"
rg -n 'const' -C2 mcp_fuzzer/fuzz_engine/strategy/schema_parser.py || true
echo "Test references to const:"
rg -n 'const' -C2 tests || trueLength of output: 19655
Refactor _merge_allOf to correctly handle const conflicts and exclusiveMinimum/exclusiveMaximum
- Separate
exclusiveMinimum/exclusiveMaximumfrom the general min/max maps: track booleans vs numeric values and apply the strictest numeric bounds when present (currently allOf treatsexclusive*like other constraints and may mix bool/number) - Aggregate all
constvalues across schemas and only emitmerged["const"]if they’re identical; drop or mark unsatisfiable on conflict (currently later schemas overwrite earlier ones) - Detect when
merged_typesbecomes empty and signal unsatisfiable (e.g. settype: []or use a marker) so callers can choose an aggressive fallback - Add unit tests covering allOf merges with conflicting
constvalues and mixedexclusiveMinimum/exclusiveMaximumscenarios
🤖 Prompt for AI Agents
In mcp_fuzzer/fuzz_engine/strategy/schema_parser.py around lines 32-129,
_merge_allOf currently overwrites consts, treats
exclusiveMinimum/exclusiveMaximum like numeric bounds, and silently allows an
empty type intersection; change it to (1) collect all const values seen and only
set merged["const"] if all are identical, otherwise set an explicit
unsatisfiable marker (e.g. merged["_unsatisfiable"]=True or drop const and
record conflict) so callers can handle it; (2) separate handling for
exclusiveMinimum/exclusiveMaximum by tracking both a numeric bound and a boolean
strict flag for each schema, then when merging choose the most restrictive
numeric limit and set exclusive* True if any schema requires strictness; (3)
after intersecting types, detect when merged_types becomes empty and mark merged
as unsatisfiable (e.g. merged["type"]=[] or merged["_unsatisfiable"]=True) so
callers can make an aggressive fallback; and (4) add unit tests that cover
conflicting consts, mixed exclusiveMinimum/exclusiveMaximum (number vs boolean),
and empty type intersections to validate behavior.
| phase == "aggressive" | ||
| and random.random() < 0.3 | ||
| and schema.get("additionalProperties", True) is not False | ||
| ): | ||
| # Add some potentially problematic properties | ||
| extra_props = { | ||
| "__proto__": {"isAdmin": True}, | ||
| "constructor": {"prototype": {"isAdmin": True}}, | ||
| "eval": "console.log('code injection')", | ||
| } | ||
| result.update( | ||
| random.sample( | ||
| extra_props.items(), k=min(len(extra_props), random.randint(1, 3)) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
random.sample on dict_items may TypeError; convert to list.
Ensure compatibility across Python versions.
- result.update(
- random.sample(
- extra_props.items(), k=min(len(extra_props), random.randint(1, 3))
- )
- )
+ pool = list(extra_props.items())
+ result.update(
+ random.sample(pool, k=min(len(pool), random.randint(1, 3)))
+ )📝 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.
| phase == "aggressive" | |
| and random.random() < 0.3 | |
| and schema.get("additionalProperties", True) is not False | |
| ): | |
| # Add some potentially problematic properties | |
| extra_props = { | |
| "__proto__": {"isAdmin": True}, | |
| "constructor": {"prototype": {"isAdmin": True}}, | |
| "eval": "console.log('code injection')", | |
| } | |
| result.update( | |
| random.sample( | |
| extra_props.items(), k=min(len(extra_props), random.randint(1, 3)) | |
| ) | |
| ) | |
| if ( | |
| phase == "aggressive" | |
| and random.random() < 0.3 | |
| and schema.get("additionalProperties", True) is not False | |
| ): | |
| # Add some potentially problematic properties | |
| extra_props = { | |
| "__proto__": {"isAdmin": True}, | |
| "constructor": {"prototype": {"isAdmin": True}}, | |
| "eval": "console.log('code injection')", | |
| } | |
| pool = list(extra_props.items()) | |
| result.update( | |
| random.sample(pool, k=min(len(pool), random.randint(1, 3))) | |
| ) |
🤖 Prompt for AI Agents
In mcp_fuzzer/fuzz_engine/strategy/schema_parser.py around lines 292 to 306, the
call to random.sample(extra_props.items(), ...) can raise a TypeError on some
Python versions because dict_items is not accepted by random.sample; convert the
dict_items to a list before sampling. Replace the random.sample call argument
with list(extra_props.items()) (i.e., random.sample(list(extra_props.items()),
k=...)) to ensure compatibility across Python versions.
| with patch.object(httpx.AsyncClient, "post") as mock_post: | ||
| mock_response = MagicMock() | ||
| mock_response.json.return_value = test_response | ||
| mock_response.status_code = 200 | ||
| mock_post.return_value = mock_response | ||
|
|
||
| # Test send_request | ||
| result = await http_transport.send_raw(test_payload) | ||
|
|
There was a problem hiding this comment.
Patch async httpx.AsyncClient.post with AsyncMock; MagicMock is not awaitable.
AsyncClient.post is awaited in the implementation. Patch it with new_callable=AsyncMock to prevent “object MagicMock is not awaitable”.
- with patch.object(httpx.AsyncClient, "post") as mock_post:
+ with patch.object(httpx.AsyncClient, "post", new_callable=AsyncMock) as mock_post:📝 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.
| with patch.object(httpx.AsyncClient, "post") as mock_post: | |
| mock_response = MagicMock() | |
| mock_response.json.return_value = test_response | |
| mock_response.status_code = 200 | |
| mock_post.return_value = mock_response | |
| # Test send_request | |
| result = await http_transport.send_raw(test_payload) | |
| with patch.object(httpx.AsyncClient, "post", new_callable=AsyncMock) as mock_post: | |
| mock_response = MagicMock() | |
| mock_response.json.return_value = test_response | |
| mock_response.status_code = 200 | |
| mock_post.return_value = mock_response | |
| # Test send_request | |
| result = await http_transport.send_raw(test_payload) |
🤖 Prompt for AI Agents
In tests/unit/transport/test_transport.py around lines 168 to 176, the test
patches httpx.AsyncClient.post with a MagicMock which is not awaitable; replace
the patch call to use new_callable=AsyncMock (from unittest.mock) so post is
awaitable, keep the MagicMock response and set mock_post.return_value =
mock_response (or set mock_post.return_value.json.return_value = test_response)
so awaiting post returns the prepared response.
| with patch.object(httpx.AsyncClient, "post") as mock_post: | ||
| mock_post.side_effect = httpx.RequestError("Connection error") | ||
|
|
||
| # Test send_request with error | ||
| with pytest.raises(httpx.RequestError): | ||
| await http_transport.send_raw(test_payload) | ||
|
|
There was a problem hiding this comment.
Same awaitability issue for error-path patch.
- with patch.object(httpx.AsyncClient, "post") as mock_post:
+ with patch.object(httpx.AsyncClient, "post", new_callable=AsyncMock) as mock_post:📝 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.
| with patch.object(httpx.AsyncClient, "post") as mock_post: | |
| mock_post.side_effect = httpx.RequestError("Connection error") | |
| # Test send_request with error | |
| with pytest.raises(httpx.RequestError): | |
| await http_transport.send_raw(test_payload) | |
| with patch.object(httpx.AsyncClient, "post", new_callable=AsyncMock) as mock_post: | |
| mock_post.side_effect = httpx.RequestError("Connection error") | |
| # Test send_request with error | |
| with pytest.raises(httpx.RequestError): | |
| await http_transport.send_raw(test_payload) |
🤖 Prompt for AI Agents
In tests/unit/transport/test_transport.py around lines 191 to 197, the test
patches httpx.AsyncClient.post but uses a regular Mock so awaiting it in the
code under test fails; make the patch awaitable by patching with an AsyncMock
(or use new_callable=AsyncMock) or set mock_post.side_effect to an async
function that raises httpx.RequestError, then run the test expecting the
exception as before.
| with patch.object(httpx.AsyncClient, "post") as mock_post: | ||
| # Create a proper AsyncMock for the response | ||
| mock_response = AsyncMock() | ||
|
|
||
| # Create a simpler mock for the async iterator | ||
| async def mock_aiter_lines(): | ||
| class AsyncIterator: | ||
| def __init__(self, items): | ||
| self.items = items |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix stream test: post should be AsyncMock; raise_for_status should be sync; ensure aclose is async.
- Patch
postwithAsyncMock. - Set
raise_for_statustoMagicMock()(httpx has sync method); asserting an awaited call risks coroutine-not-awaited warnings.
- with patch.object(httpx.AsyncClient, "post") as mock_post:
+ with patch.object(httpx.AsyncClient, "post", new_callable=AsyncMock) as mock_post:
@@
- # Add raise_for_status method
- mock_response.raise_for_status = AsyncMock()
+ # Add raise_for_status method (sync in httpx)
+ mock_response.raise_for_status = MagicMock()
+ # Ensure aclose is awaitable
+ mock_response.aclose = AsyncMock()
@@
- # raise_for_status in httpx is sync on Response; our code calls it sync
- # so assert it was called (not awaited)
- assert mock_response.raise_for_status.called
+ mock_response.raise_for_status.assert_called_once()Also applies to: 236-257
🤖 Prompt for AI Agents
In tests/unit/transport/test_transport.py around lines 208-216 (and similarly
236-257), the test patches httpx.AsyncClient.post incorrectly and configures
response methods as coroutines; change the patch to use AsyncMock for post, set
mock_response.raise_for_status = MagicMock() (since httpx.raise_for_status is
synchronous) and ensure mock_response.aclose is an AsyncMock(), and keep the
async iterator for .aiter_lines as an async generator/iterator; update
assertions to await only actual async calls (post, aclose) and not
raise_for_status.
| with patch.object(httpx.AsyncClient, "stream") as mock_stream: | ||
| # Mock streaming response | ||
| mock_response = MagicMock() | ||
| mock_response.aiter_text.return_value = sse_events | ||
| mock_stream.return_value.__aenter__.return_value = mock_response | ||
|
|
||
| # Test stream_request | ||
| responses = [] | ||
| async for response in sse_transport._stream_request(test_payload): | ||
| responses.append(response) | ||
|
|
||
| # Check the results | ||
| assert len(responses) == 2 | ||
| assert responses[0] == {"id": 1, "result": "streaming"} | ||
| assert responses[1] == {"id": 2, "result": "complete"} | ||
| mock_stream.assert_called_once() | ||
|
|
There was a problem hiding this comment.
Make httpx.AsyncClient.stream an async context manager and aiter_text() async-iterable.
Current setup returns a plain list for aiter_text(), which is not async-iterable, and stream is not an async CM. Use an async contextmanager and async generator.
- with patch.object(httpx.AsyncClient, "stream") as mock_stream:
- # Mock streaming response
- mock_response = MagicMock()
- mock_response.aiter_text.return_value = sse_events
- mock_stream.return_value.__aenter__.return_value = mock_response
+ from contextlib import asynccontextmanager
+ with patch.object(httpx.AsyncClient, "stream") as mock_stream:
+ mock_response = MagicMock()
+ async def aiter_text():
+ for ev in sse_events:
+ yield ev
+ mock_response.aiter_text.return_value = aiter_text()
+ mock_response.raise_for_status = MagicMock()
+ @asynccontextmanager
+ async def stream_ctx(*_args, **_kwargs):
+ yield mock_response
+ mock_stream.return_value = stream_ctx()📝 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.
| with patch.object(httpx.AsyncClient, "stream") as mock_stream: | |
| # Mock streaming response | |
| mock_response = MagicMock() | |
| mock_response.aiter_text.return_value = sse_events | |
| mock_stream.return_value.__aenter__.return_value = mock_response | |
| # Test stream_request | |
| responses = [] | |
| async for response in sse_transport._stream_request(test_payload): | |
| responses.append(response) | |
| # Check the results | |
| assert len(responses) == 2 | |
| assert responses[0] == {"id": 1, "result": "streaming"} | |
| assert responses[1] == {"id": 2, "result": "complete"} | |
| mock_stream.assert_called_once() | |
| from contextlib import asynccontextmanager | |
| with patch.object(httpx.AsyncClient, "stream") as mock_stream: | |
| mock_response = MagicMock() | |
| async def aiter_text(): | |
| for ev in sse_events: | |
| yield ev | |
| mock_response.aiter_text.return_value = aiter_text() | |
| mock_response.raise_for_status = MagicMock() | |
| @asynccontextmanager | |
| async def stream_ctx(*_args, **_kwargs): | |
| yield mock_response | |
| mock_stream.return_value = stream_ctx() | |
| # Test stream_request | |
| responses = [] | |
| async for response in sse_transport._stream_request(test_payload): | |
| responses.append(response) | |
| # Check the results | |
| assert len(responses) == 2 | |
| assert responses[0] == {"id": 1, "result": "streaming"} | |
| assert responses[1] == {"id": 2, "result": "complete"} | |
| mock_stream.assert_called_once() |
| with patch.object(httpx.AsyncClient, "stream") as mock_stream: | ||
| mock_stream.side_effect = httpx.RequestError("Connection error") | ||
|
|
||
| # Test stream_request with error | ||
| with pytest.raises(httpx.RequestError): | ||
| async for _ in sse_transport._stream_request(test_payload): | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Raise error from the async context manager instead of the function patch.
Ensure the failure occurs on entering the async with block.
- with patch.object(httpx.AsyncClient, "stream") as mock_stream:
- mock_stream.side_effect = httpx.RequestError("Connection error")
+ from contextlib import asynccontextmanager
+ with patch.object(httpx.AsyncClient, "stream") as mock_stream:
+ @asynccontextmanager
+ async def stream_ctx(*_args, **_kwargs):
+ raise httpx.RequestError("Connection error")
+ yield # unreachable
+ mock_stream.return_value = stream_ctx()📝 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.
| with patch.object(httpx.AsyncClient, "stream") as mock_stream: | |
| mock_stream.side_effect = httpx.RequestError("Connection error") | |
| # Test stream_request with error | |
| with pytest.raises(httpx.RequestError): | |
| async for _ in sse_transport._stream_request(test_payload): | |
| pass | |
| from contextlib import asynccontextmanager | |
| with patch.object(httpx.AsyncClient, "stream") as mock_stream: | |
| @asynccontextmanager | |
| async def stream_ctx(*_args, **_kwargs): | |
| raise httpx.RequestError("Connection error") | |
| yield # unreachable | |
| mock_stream.return_value = stream_ctx() | |
| # Test stream_request with error | |
| with pytest.raises(httpx.RequestError): | |
| async for _ in sse_transport._stream_request(test_payload): | |
| pass |
🤖 Prompt for AI Agents
In tests/unit/transport/test_transport.py around lines 348 to 354, the patch
currently makes httpx.RequestError raise when the stream function is called, but
the test needs the error to be raised when entering the async context manager;
update the mock so the async context manager returned by
httpx.AsyncClient.stream raises on __aenter__ (e.g., set
mock_stream.return_value.__aenter__.side_effect to
httpx.RequestError("Connection error")), then keep the existing pytest.raises
block and async for to ensure the failure happens on entering the async with.
…ctionlint, SSE multi-line events
Documentation PreviewYour documentation changes have been built successfully! Preview URL: https://Agent-Hellboy.github.io/mcp-server-fuzzer/ The documentation will be automatically deployed to the main site when this PR is merged. This comment was automatically generated by the documentation workflow. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
mcp_fuzzer/transport/sse.py (3)
28-32: Good hardening: no env proxies and no implicit redirects.trust_env=False and follow_redirects=False align this transport with safety policy.
Also applies to: 81-85, 103-107
188-217: SSE spec compliance: joining multi-line data with “\n”.Correct and matches spec; prevents payload corruption.
121-177: Critical: chunk-boundary bug — aiter_text can split lines mid-field. Use aiter_lines with event buffering.Current logic can corrupt lines across text chunks, causing JSON parse failures or dropped events.
- chunks = response.aiter_text() - buffer = [] # Buffer to accumulate SSE event data + lines = response.aiter_lines() + event_lines: List[str] = [] + + def flush_event() -> Optional[Dict[str, Any]]: + if not event_lines: + return None + event_text = "\n".join(event_lines) + event_lines.clear() + try: + parsed = SSETransport._parse_sse_event(event_text) + except json.JSONDecodeError: + logging.error("Failed to parse SSE event payload as JSON") + return None + return parsed @@ - if hasattr(chunks, "__aiter__"): - async for chunk in chunks: # type: ignore[func-returns-value] - if not chunk: - continue - - # Process each line in the chunk - for line in chunk.splitlines(): - if line.strip(): - # Non-empty line: add to current event buffer - buffer.append(line) - else: - # Empty line: marks end of an event, process the buffer - if buffer: - try: - event_text = "\n".join(buffer) - parsed = SSETransport._parse_sse_event( - event_text - ) - if parsed is not None: - yield parsed - except json.JSONDecodeError: - logging.error( - "Failed to parse SSE event payload as JSON" - ) - finally: - buffer = [] # Clear buffer for next event + if hasattr(lines, "__aiter__"): + async for line in lines: # type: ignore[func-returns-value] + if not line.strip(): + parsed = flush_event() + if parsed is not None: + yield parsed + continue + event_lines.append(line) else: - for chunk in chunks: # type: ignore[assignment] - if not chunk: - continue - - # Process each line in the chunk - for line in chunk.splitlines(): - if line.strip(): - # Non-empty line: add to current event buffer - buffer.append(line) - else: - # Empty line: marks end of an event, process the buffer - if buffer: - try: - event_text = "\n".join(buffer) - parsed = SSETransport._parse_sse_event( - event_text - ) - if parsed is not None: - yield parsed - except json.JSONDecodeError: - logging.error( - "Failed to parse SSE event payload as JSON" - ) - finally: - buffer = [] # Clear buffer for next event + for line in lines: # type: ignore[assignment] + if not line.strip(): + parsed = flush_event() + if parsed is not None: + yield parsed + continue + event_lines.append(line) @@ - # Process any remaining buffered data at the end of the stream - if buffer: - try: - event_text = "\n".join(buffer) - parsed = SSETransport._parse_sse_event(event_text) - if parsed is not None: - yield parsed - except json.JSONDecodeError: - logging.error("Failed to parse SSE event payload as JSON") + # Final flush + parsed = flush_event() + if parsed is not None: + yield parsed
🧹 Nitpick comments (10)
.github/workflows/component-tests.yml (7)
22-27: Strengthen pip cache hits by hashing your deps files.Add cache-dependency-path so setup-python’s pip cache invalidates correctly when deps change.
- name: Set up Python uses: actions/setup-python@v5 with: python-version: '3.11' cache: 'pip' + cache-dependency-path: | + pyproject.toml + requirements*.txt
28-33: Consider installing test extras in one shot.If you have a test extra, prefer
pip install ".[test]"to ensure parity with local runs. Otherwise, this is fine.
34-67: Lowercase the input tokens to be resilient to case.You already trim whitespace; add lowercasing so inputs like “CLI, Transport” work.
for t in "${TOKENS[@]}"; do t="${t//[[:space:]]/}" + t="${t,,}" case "$t" in auth) AUTH_CHANGES=true ;;
34-67: Step name is a bit misleading.This step parses the manual selection, not “changed” files. Rename for clarity or add a separate change-detector in a PR-triggered workflow.
- - name: Determine changed components + - name: Parse requested componentsIs automatic changed-only selection handled in another workflow (e.g., via dorny/paths-filter on pull_request)? If not, I can add it.
75-98: Unify if: expression style for readability.Some steps use bare expressions, others wrap with ${{ }}. Pick one style; GA docs prefer ${{ }}.
- if: steps.changes.outputs.auth == 'true' + if: ${{ steps.changes.outputs.auth == 'true' }} @@ - if: steps.changes.outputs.cli == 'true' + if: ${{ steps.changes.outputs.cli == 'true' }} @@ - if: steps.changes.outputs.client == 'true' + if: ${{ steps.changes.outputs.client == 'true' }} @@ - if: steps.changes.outputs.fuzz_engine == 'true' + if: ${{ steps.changes.outputs.fuzz_engine == 'true' }} @@ - if: steps.changes.outputs.safety == 'true' + if: ${{ steps.changes.outputs.safety == 'true' }} @@ - if: steps.changes.outputs.transport == 'true' + if: ${{ steps.changes.outputs.transport == 'true' }}
113-130: Simplify Codecov upload by relying on OIDC (drop token gating).Since you already grant id-token: write, v4 can upload without a token. This avoids secret management and still skips forks (this workflow is manual anyway).
- - name: Check Codecov token - id: codecov_token - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - run: | - if [ -n "$CODECOV_TOKEN" ]; then - echo "has_token=true" >> $GITHUB_OUTPUT - else - echo "has_token=false" >> $GITHUB_OUTPUT - fi - - name: Upload coverage to Codecov - if: ${{ (steps.changes.outputs.auth == 'true' || steps.changes.outputs.cli == 'true' || steps.changes.outputs.client == 'true' || steps.changes.outputs.fuzz_engine == 'true' || steps.changes.outputs.safety == 'true' || steps.changes.outputs.transport == 'true') && steps.coverage_check.outputs.has_coverage == 'true' && steps.codecov_token.outputs.has_token == 'true' }} + if: ${{ (steps.changes.outputs.auth == 'true' || steps.changes.outputs.cli == 'true' || steps.changes.outputs.client == 'true' || steps.changes.outputs.fuzz_engine == 'true' || steps.changes.outputs.safety == 'true' || steps.changes.outputs.transport == 'true') && steps.coverage_check.outputs.has_coverage == 'true' }} uses: codecov/codecov-action@v4 with: - token: ${{ secrets.CODECOV_TOKEN }} files: coverage.*.xml fail_ci_if_error: true
15-18: Optional: add concurrency to auto-cancel superseded runs.Prevents stacked manual runs on the same ref.
permissions: contents: read id-token: write actions: read + +concurrency: + group: component-tests-${{ github.ref }} + cancel-in-progress: truemcp_fuzzer/transport/sse.py (3)
34-36: Use specific exception types for policy and server errors.Typed exceptions make it easier for callers to handle failures distinctly.
Apply:
- raise Exception( + raise NetworkPolicyError( "Network to non-local host is disallowed by safety policy" ) @@ - if "error" in data: - raise Exception(f"Server error: {data['error']}") + if "error" in data: + raise ServerError(f"Server error: {data['error']}")Add near the top of this module:
class NetworkPolicyError(PermissionError): pass class ServerError(RuntimeError): passAlso applies to: 54-56
58-75: send_raw parses full body; consider streaming to avoid buffering entire SSE responses.If servers keep connections open or emit large streams, this may hang or inflate memory. Optionally mirror the streaming path and return the first event.
94-102: Add return type for _stream_request and import AsyncIterator.Improves readability and type-checking.
-from typing import Any, Dict, Optional, List +from typing import Any, Dict, Optional, List, AsyncIterator @@ - async def _stream_request(self, payload: Dict[str, Any]): + async def _stream_request(self, payload: Dict[str, Any]) -> AsyncIterator[Dict[str, Any]]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/component-tests.yml(1 hunks)mcp_fuzzer/fuzz_engine/executor.py(2 hunks)mcp_fuzzer/transport/sse.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp_fuzzer/fuzz_engine/executor.py
🧰 Additional context used
🧬 Code graph analysis (1)
mcp_fuzzer/transport/sse.py (2)
mcp_fuzzer/transport/base.py (3)
TransportProtocol(13-101)send_raw(21-22)_stream_request(53-57)mcp_fuzzer/safety_system/policy.py (2)
is_host_allowed(69-112)sanitize_headers(158-166)
🔇 Additional comments (4)
.github/workflows/component-tests.yml (3)
11-15: Permissions look correct for Codecov v4 (OIDC) and minimal checkout.
contents: readandid-token: writeare set at workflow scope; good baseline.
99-103: LGTM on integration run and coverage target.Runs only when any component is selected and measures the package.
104-112: Coverage presence check is fine.Simple and reliable; no changes needed.
mcp_fuzzer/transport/sse.py (1)
23-25: SSETransport.send_request unimplemented—override get_tools/call_tool or restrict usage
send_requestis correctly raisingNotImplementedError(covered by tests ), butTransportProtocol.get_tools(base.py §61) andmcp_fuzzer/client/tool_client.py:55callsend_request("tools/list"). Either overrideget_tools(andcall_tool) insse.pyto delegate to_stream_request, or ensure the factory never returns SSETransport for non-streaming flows.
fixes #70
Summary by CodeRabbit
New Features
Tests
CI/CD
Documentation
Chores