feat(mcp): add BDD testing framework and modularize HTTP bridge#88
Merged
sergiofilhowz merged 1 commit intomainfrom May 6, 2026
Merged
feat(mcp): add BDD testing framework and modularize HTTP bridge#88sergiofilhowz merged 1 commit intomainfrom
sergiofilhowz merged 1 commit intomainfrom
Conversation
- Introduced a BDD testing framework using Cucumber for behavior-driven development. - Added new feature tests for error handling, initialization, notifications, prompts, resources, and tools. - Modularized the HTTP bridge by creating a dedicated `lib.rs` for the MCP library. - Updated `Cargo.toml` to include new dependencies for BDD testing and futures. - Removed outdated integration tests and manifest tests to streamline the testing process. - Enhanced the skills bridge to better handle nested URIs in markdown. This commit lays the groundwork for comprehensive testing of the MCP worker's functionality and improves the overall architecture of the codebase.
📝 WalkthroughWalkthroughThis PR introduces BDD testing infrastructure for the MCP crate via Cucumber, refactors module organization by extracting public modules into a library target, updates the nested-URI regex pattern in skills_bridge for improved depth-awareness, and removes legacy integration and manifest tests. ChangesBDD Testing Infrastructure & Module Reorganization
Skills Description Extraction Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mcp/tests/common/http.rs (1)
63-66: ⚡ Quick win
parse_sse_optcaptures the firstdata:line — should capture the lastMCP Streamable HTTP allows a server to emit intermediate SSE events (e.g., progress notifications) before the final result event.
find_mapstops at the firstdata:line, so if the bridge ever starts sending a leading notification event, allThenassertions that calllast_rpcwould silently inspect the notification envelope instead of the result, producing misleading failures like"expected result.prompts to be an array".♻️ Proposed fix — capture the last data line
pub fn parse_sse_opt(body: &str) -> Option<Value> { - let line = body.lines().find_map(|l| l.strip_prefix("data: "))?; + let line = body.lines().filter_map(|l| l.strip_prefix("data: ")).last()?; serde_json::from_str(line).ok() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tests/common/http.rs` around lines 63 - 66, The current parse_sse_opt function uses body.lines().find_map to grab the first "data: " line, but it needs to capture the final SSE data envelope (the last "data: " line) so test helpers like last_rpc inspect the final result; change the line-selection logic in parse_sse_opt to search from the end (e.g., iterate lines in reverse or collect and take the last element) to pick the last line that strips the "data: " prefix, then pass that to serde_json::from_str(line).ok() as before.mcp/tests/steps/tools.rs (1)
44-54: ⚡ Quick winHardcoded
badprefix list diverges fromdefault_hidden_prefixes()over time.The list manually encodes the
::→__transformation of every prefix inconfig::default_hidden_prefixes(). Adding a new hidden prefix to the config requires a matching update here, and there's no compile-time link to enforce it.Consider deriving the list programmatically from the actual defaults:
♻️ Proposed refactor
+use iii_mcp::config::default_hidden_prefixes; #[then("no advertised tool name starts with a hidden prefix")] fn no_hidden_prefix_in_tools(world: &mut McpWorld) { if world.iii.is_none() { return; } let v = last_rpc(world); let arr = v["result"]["tools"].as_array().unwrap_or_else(|| { panic!("expected result.tools to be an array, got {v:#}"); }); - // The MCP tool name encoding maps `::` to `__`, so check the - // matching prefixes too. - let bad: &[&str] = &[ - "engine__", - "state__", - "stream__", - "iii.", - "iii__", - "mcp__", - "a2a__", - "skills__", - "prompts__", - ]; + // Derive encoded prefixes from the canonical config defaults. + let encoded: Vec<String> = default_hidden_prefixes() + .into_iter() + .map(|p| p.replace("::", "__")) + .collect(); + let bad: Vec<&str> = encoded.iter().map(|s| s.as_str()).collect(); for tool in arr { let name = tool["name"].as_str().unwrap_or(""); for prefix in bad {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tests/steps/tools.rs` around lines 44 - 54, The test currently hardcodes the `bad` prefixes (in `bad: &[&str]`) by manually encoding the `::`→`__` transformation, which can drift from `config::default_hidden_prefixes()`; change the test to derive `bad` programmatically by calling `config::default_hidden_prefixes()` and mapping each prefix with the same `::` → `__` transformation used by production code (e.g., replace("::", "__")), then use that generated collection (Vec<String> or Vec<&str>/iter comparison) in place of the hardcoded slice so the test always reflects the real defaults (refer to `default_hidden_prefixes()` and the `bad` variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/tests/features/notifications.feature`:
- Around line 1-16: Add assertions in both scenarios of notifications.feature to
verify the response body is empty: after the existing "Then the http response
status is 202" step, add a step that calls the test helper to assert body
emptiness (use the existing notifications.rs step that invokes
last_body(world).is_empty()). Update or add the corresponding step
implementation in notifications.rs to call last_body(world).is_empty() and fail
the step if it returns false, ensuring the spec "with no body" is actually
enforced.
---
Nitpick comments:
In `@mcp/tests/common/http.rs`:
- Around line 63-66: The current parse_sse_opt function uses
body.lines().find_map to grab the first "data: " line, but it needs to capture
the final SSE data envelope (the last "data: " line) so test helpers like
last_rpc inspect the final result; change the line-selection logic in
parse_sse_opt to search from the end (e.g., iterate lines in reverse or collect
and take the last element) to pick the last line that strips the "data: "
prefix, then pass that to serde_json::from_str(line).ok() as before.
In `@mcp/tests/steps/tools.rs`:
- Around line 44-54: The test currently hardcodes the `bad` prefixes (in `bad:
&[&str]`) by manually encoding the `::`→`__` transformation, which can drift
from `config::default_hidden_prefixes()`; change the test to derive `bad`
programmatically by calling `config::default_hidden_prefixes()` and mapping each
prefix with the same `::` → `__` transformation used by production code (e.g.,
replace("::", "__")), then use that generated collection (Vec<String> or
Vec<&str>/iter comparison) in place of the hardcoded slice so the test always
reflects the real defaults (refer to `default_hidden_prefixes()` and the `bad`
variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21754251-6848-4040-b61e-edd8a4142a0f
⛔ Files ignored due to path filters (2)
mcp/Cargo.lockis excluded by!**/*.lockskills/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
mcp/Cargo.tomlmcp/src/lib.rsmcp/src/main.rsmcp/src/skills_bridge.rsmcp/tests/bdd.rsmcp/tests/common/engine.rsmcp/tests/common/http.rsmcp/tests/common/mod.rsmcp/tests/common/world.rsmcp/tests/features/errors.featuremcp/tests/features/initialize.featuremcp/tests/features/manifest.featuremcp/tests/features/notifications.featuremcp/tests/features/prompts.featuremcp/tests/features/resources.featuremcp/tests/features/tools.featuremcp/tests/integration.rsmcp/tests/manifest.rsmcp/tests/steps/errors.rsmcp/tests/steps/initialize.rsmcp/tests/steps/manifest.rsmcp/tests/steps/mod.rsmcp/tests/steps/notifications.rsmcp/tests/steps/prompts.rsmcp/tests/steps/resources.rsmcp/tests/steps/tools.rsskills/src/functions/skills.rsskills/tests/features/markdown.feature
💤 Files with no reviewable changes (2)
- mcp/tests/manifest.rs
- mcp/tests/integration.rs
Comment on lines
+1
to
+16
| @engine @notifications | ||
| Feature: notifications return 202 Accepted with no body | ||
| MCP Streamable HTTP §"server response": notification-only requests | ||
| (no `id` field) MUST return 202. The bridge accepts both the | ||
| spec'd `notifications/initialized` and any unknown notification. | ||
|
|
||
| Background: | ||
| Given the iii engine is reachable | ||
|
|
||
| Scenario: notifications/initialized returns 202 | ||
| When I send a notifications/initialized notification | ||
| Then the http response status is 202 | ||
|
|
||
| Scenario: an unknown notification still returns 202 | ||
| When I send an unknown notification with method "notifications/something-novel" | ||
| Then the http response status is 202 |
There was a problem hiding this comment.
"With no body" is stated but not asserted
Both the Feature description and module doc promise 202 with no body, but neither scenario includes a Then step verifying the empty response body. The 202 status is tested, but the body-emptiness half of the contract is unverified, leaving room for a regression where the bridge starts returning a body without breaking these tests.
💡 Suggested addition to both scenarios
Scenario: notifications/initialized returns 202
When I send a notifications/initialized notification
Then the http response status is 202
+ And the response body is empty
Scenario: an unknown notification still returns 202
When I send an unknown notification with method "notifications/something-novel"
Then the http response status is 202
+ And the response body is emptyYou would add a matching step in notifications.rs using last_body(world).is_empty().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/tests/features/notifications.feature` around lines 1 - 16, Add assertions
in both scenarios of notifications.feature to verify the response body is empty:
after the existing "Then the http response status is 202" step, add a step that
calls the test helper to assert body emptiness (use the existing
notifications.rs step that invokes last_body(world).is_empty()). Update or add
the corresponding step implementation in notifications.rs to call
last_body(world).is_empty() and fail the step if it returns false, ensuring the
spec "with no body" is actually enforced.
Merged
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lib.rsfor the MCP library.Cargo.tomlto include new dependencies for BDD testing and futures.This commit lays the groundwork for comprehensive testing of the MCP worker's functionality and improves the overall architecture of the codebase.
Summary by CodeRabbit
Tests
Bug Fixes