Repository navigation
feat: mcp - #74
Merged
Merged
feat: mcp#74
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughReplaces the old multi-transport MCP worker with a simplified HTTP-only MCP bridge: new config, protocol helpers, and a compact JSON-RPC dispatcher; removes legacy stdio/transport, session state, prompts, spec helpers, and worker-manager code; adds manifest support and a Cucumber BDD test harness with feature files and step defs. ChangesMCP Worker Restructure (core feature DAG)
BDD Test Harness & Fixtures (independent DAG focused on tests)
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Engine as iii Engine<br>(HTTP)
participant Handler as mcp::handler
participant Protocol as protocol helpers
participant Skills as skills Worker
participant Tool as Tool Function
Client->>Engine: POST /mcp {"jsonrpc":"2.0","id":1,"method":"tools/list"}
Engine->>Handler: TriggerRequest(payload)
Handler->>Handler: parse_body() / validate_frame()
Handler->>Engine: trigger("engine::functions::list")
Engine->>Handler: FunctionInfo[]
Handler->>Protocol: filter hidden/exposed & function_to_tool()
Handler->>Engine: return HTTP envelope (200) with JSON-RPC result
Engine->>Client: 200 + body with tools list
Client->>Engine: POST /mcp {"jsonrpc":"2.0","id":2,"method":"tools/call","params":{...}}
Engine->>Handler: TriggerRequest(payload)
Handler->>Protocol: tool_name_to_function_id(), is_hidden()
Handler->>Engine: trigger("bdd::echo", params)
Engine->>Tool: execute
Tool->>Engine: result
Handler->>Protocol: tool_text()/tool_error()
Handler->>Engine: return HTTP envelope (200) with tool result
Client->>Engine: POST /mcp {"jsonrpc":"2.0","id":3,"method":"resources/read","params":{"uri":"iii://demo"}}
Engine->>Handler: TriggerRequest(payload)
Handler->>Handler: delegate to skills::resources-read
Handler->>Engine: trigger("skills::resources-read", params)
Engine->>Skills: execute
Skills->>Engine: resource payload
Handler->>Engine: return HTTP envelope (200) with resource result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
mcp/src/manifest.rs (1)
14-38: ⚡ Quick winDerive
default_configfromMcpConfig::default()rather than hardcoding it.The inline
serde_json::json!({...})block on lines 21–35 duplicates the defaults already authoritative inconfig.rs. When a field is added to or changed inMcpConfig, this block silently drifts — and the duplication already shows:require_expose(present inMcpConfig) is absent here. SerializingMcpConfig::default()directly keeps the two in sync without a second list to maintain.♻️ Proposed refactor
+use crate::config::McpConfig; use serde::Serialize; pub fn build_manifest() -> ModuleManifest { + let default_cfg = McpConfig::default(); ModuleManifest { name: env!("CARGO_PKG_NAME").to_string(), version: env!("CARGO_PKG_VERSION").to_string(), description: "Model Context Protocol bridge. Exposes iii functions as MCP tools and the skills worker as MCP resources/prompts over POST /mcp." .to_string(), - default_config: serde_json::json!({ - "api_path": "mcp", - "state_timeout_ms": 30_000, - "hidden_prefixes": [ - "engine::", - "state::", - "stream::", - "iii.", - "iii::", - "mcp::", - "a2a::", - "skills::", - "prompts::" - ] - }), + default_config: serde_json::to_value(&default_cfg) + .expect("McpConfig is always serializable"), supported_targets: vec![env!("TARGET").to_string()], } }🤖 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/src/manifest.rs` around lines 14 - 38, The manifest's default_config is hardcoded via serde_json::json! causing duplication with McpConfig defaults; change build_manifest to set ModuleManifest.default_config by serializing McpConfig::default() instead (e.g., use serde_json::to_value(McpConfig::default()) or equivalent) so that the default_config derives from McpConfig::default() and includes fields like require_expose automatically; update the build_manifest function (reference: build_manifest, ModuleManifest, default_config, McpConfig::default) to perform that serialization and handle any Result/error as needed.
🤖 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/README.md`:
- Around line 7-14: The README lists `prompts/list` and `prompts/get` as backed
by the `skills` worker but the runtime actually wires them to the prompts worker
under `prompts::mcp-list` and `prompts::mcp-get`; update the table rows and any
install guidance so that `prompts/list` and `prompts/get` reference the prompts
worker (e.g., `prompts::mcp-list`, `prompts::mcp-get`) instead of `skills`, and
ensure the later method coverage table matches this same change so users are
pointed to the correct backing worker.
In `@mcp/src/config.rs`:
- Around line 60-64: The load_config function should validate or normalize the
api_path field on McpConfig to reject or strip a leading '/' so downstream code
(e.g., register_http_trigger which does format!("/{}", api_path) in
functions::mod.rs) does not produce double slashes; update load_config to parse
the YAML into McpConfig then either trim a leading '/' from cfg.api_path or
return an Err if api_path starts_with('/') and include a clear error message,
referencing the McpConfig type and the load_config function for location.
In `@mcp/src/functions/handler.rs`:
- Around line 193-229: In tools_call, enforce the same "require_expose"
execution guard used by tools/list: after computing function_id with
protocol::tool_name_to_function_id and before triggering ctx.iii.trigger, check
the require_expose flag on ctx.cfg and verify the function is explicitly exposed
(use the same predicate used by tools/list — e.g. a protocol::is_exposed or
equivalent check against ctx.cfg.exposed list); if require_expose is true and
the function is not exposed, return protocol::tool_error denying execution
instead of proceeding to ctx.iii.trigger. Ensure you reference tools_call,
protocol::tool_name_to_function_id, ctx.cfg.require_expose (or equivalent), and
the exposure-check helper used by tools/list.
- Around line 111-146: The dispatch function currently treats any message
lacking an id as a notification before verifying the shape of the JSON-RPC
frame, allowing malformed payloads (e.g. {}, [], scalars) to be swallowed;
change dispatch to first validate that the incoming body is a JSON object and
that "method" exists and is a string (use the same extraction logic around
method = body.get("method").and_then(|v| v.as_str()) but check for None/invalid
types), and if validation fails return a JSON-RPC Invalid Request error (use
JsonRpcResponse::error with the INVALID_REQUEST constant) rather than returning
None; only after confirming a valid string method should you apply the
notification fast-path (method.starts_with("notifications/") || id.is_none())
and proceed to the existing match (e.g., tools_list, tools_call, resources_read,
prompts_get, etc.).
In `@mcp/src/functions/mod.rs`:
- Around line 20-23: Change register_all to return a Result or status rather
than unit so HTTP trigger registration failures bubble to the caller: update the
signature of register_all(iii: &Arc<III>, cfg: &Arc<McpConfig>) -> Result<(),
SomeError> (or -> bool) and have it call register_handler and then call
register_http_trigger propagating any error from register_http_trigger (do not
swallow errors). Update register_handler and/or register_http_trigger return
types as needed so register_all can propagate failures, and update the calling
site in main to check the returned Result/status and abort or avoid advertising
readiness on error; the same change should be applied to the other registration
block referenced (the code around register calls at 62-79). Ensure error types
are mapped or converted consistently so main can inspect and log the failure.
- Around line 62-70: Normalize or reject leading slashes on api_path in
register_http_trigger: inside the register_http_trigger function, inspect
cfg.api_path before building RegisterTriggerInput and either strip a leading '/'
(e.g., api_path = api_path.trim_start_matches('/')) or return/raise an error
when api_path.starts_with('/'); then pass the normalized string into the JSON
config for RegisterTriggerInput (config.api_path) so the engine never receives a
value beginning with '/'.
In `@mcp/src/protocol.rs`:
- Around line 106-112: The current bidirectional mapping in
function_id_to_tool_name and tool_name_to_function_id is ambiguous when function
IDs contain "__"; fix by making the encoding collision-free or by rejecting
ambiguous IDs: either (A) change the converters to encode "::" with a safe
scheme (e.g., percent-encode "::" or use an escape sequence that cannot occur in
function IDs) so round-trips are lossless, or (B) add explicit validation that
function IDs do not contain "__" (validate in function_id_to_tool_name and at
the tools/call dispatch entry) and return a clear error when violated; update
the dispatch handling that looks up tools/call to check for this guard and
expand the round-trip tests to include the previously ambiguous case
("worker_v2__util::action") to ensure correct behavior.
In `@mcp/tests/common/engine.rs`:
- Around line 57-60: The closure passed to get_or_init is swallowing
registration errors by calling register_all(&iii).await.ok()? which converts
failures into None (treated as "skip"); change this to propagate failures
instead so test setup fails on registration errors—replace the .ok()? pattern
with a fallible await (e.g., use register_all(&iii).await? or explicitly map the
Result to an error) so that register_all errors cause the get_or_init future to
return an Err and not set world.iii to None; update the closure around
try_connect_raw(), register_all, and the Some(iii) return to propagate
registration errors rather than downgrading them to skips.
In `@mcp/tests/steps/core.rs`:
- Around line 17-20: The step functions (e.g., call_handler) currently use
guarded early returns when world.iii is None, which skips tests; instead require
the engine be present so the scenario fails: replace the pattern `let Some(iii)
= world.iii.clone() else { return; }` with a hard requirement (e.g.,
assert/expect that world.iii is Some or call unwrap/expect) so the test fails if
the engine is missing; apply the same change to the other step functions
referenced (the guards at the other listed locations) so all steps depend on
world.iii being present rather than silently returning.
In `@mcp/tests/steps/tools.rs`:
- Around line 47-59: In includes_echo, the test currently asserts msg is the
first entry by checking echo["inputSchema"]["required"][0]; change this to
assert that the "required" array contains "msg" regardless of order: locate the
echo object found via tools.iter().find(...) (function includes_echo and
variables v, tools, echo) and replace the index-based assertion with a
membership check over echo["inputSchema"]["required"] (e.g., iterate or use
any/contains logic) so the test passes even if field order changes.
---
Nitpick comments:
In `@mcp/src/manifest.rs`:
- Around line 14-38: The manifest's default_config is hardcoded via
serde_json::json! causing duplication with McpConfig defaults; change
build_manifest to set ModuleManifest.default_config by serializing
McpConfig::default() instead (e.g., use
serde_json::to_value(McpConfig::default()) or equivalent) so that the
default_config derives from McpConfig::default() and includes fields like
require_expose automatically; update the build_manifest function (reference:
build_manifest, ModuleManifest, default_config, McpConfig::default) to perform
that serialization and handle any Result/error as needed.
🪄 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: 37cd4da0-e860-49c4-ab0c-e7043a50ac12
⛔ Files ignored due to path filters (1)
mcp/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
mcp/Cargo.tomlmcp/README.mdmcp/build.rsmcp/config.yamlmcp/examples/default-secure-auth.rsmcp/iii.worker.yamlmcp/src/config.rsmcp/src/functions/handler.rsmcp/src/functions/mod.rsmcp/src/handler.rsmcp/src/lib.rsmcp/src/main.rsmcp/src/manifest.rsmcp/src/prompts.rsmcp/src/protocol.rsmcp/src/spec.rsmcp/src/transport.rsmcp/src/worker_manager.rsmcp/tests/bdd.rsmcp/tests/cli.rsmcp/tests/common/engine.rsmcp/tests/common/mod.rsmcp/tests/common/workers.rsmcp/tests/common/world.rsmcp/tests/features/core.featuremcp/tests/features/prompts.featuremcp/tests/features/resources.featuremcp/tests/features/tools.featuremcp/tests/manifest.rsmcp/tests/protocol_loop_guard.rsmcp/tests/spec_2a.rsmcp/tests/steps/core.rsmcp/tests/steps/mod.rsmcp/tests/steps/prompts.rsmcp/tests/steps/resources.rsmcp/tests/steps/tools.rs
💤 Files with no reviewable changes (9)
- mcp/tests/cli.rs
- mcp/examples/default-secure-auth.rs
- mcp/src/prompts.rs
- mcp/tests/protocol_loop_guard.rs
- mcp/src/worker_manager.rs
- mcp/src/handler.rs
- mcp/src/transport.rs
- mcp/tests/spec_2a.rs
- mcp/src/spec.rs
Contributor
Author
CodeRabbit review passWorked through every actionable comment + the nitpick. Summary of what landed and what I'm deferring, with rationale. Addressed (10 of 11)
Deferred (1 of 11) — comment is based on a wrong premise
Verification
Files touched |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/protocol.rs`:
- Around line 185-187: In sanitize_schema, the current branch that replaces any
boolean schema (matches!(prop, Value::Bool(_))) with json!({}) incorrectly turns
false (never-valid) into {} (always-valid); change it to inspect the boolean
value and: if true, set *prop = json!({}) (always-valid), if false, set *prop =
json!({"not": {}}) (never-valid) so JSON Schema semantics are preserved. Target
the sanitize_schema function and the variable prop in that match arm to
implement this conditional replacement.
🪄 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: cb6ea97a-a310-4d2e-8eba-66b8b39256aa
📒 Files selected for processing (11)
mcp/README.mdmcp/src/config.rsmcp/src/functions/handler.rsmcp/src/functions/mod.rsmcp/src/main.rsmcp/src/manifest.rsmcp/src/protocol.rsmcp/tests/bdd.rsmcp/tests/common/engine.rsmcp/tests/common/workers.rsmcp/tests/steps/tools.rs
✅ Files skipped from review due to trivial changes (2)
- mcp/tests/common/workers.rs
- mcp/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- mcp/src/manifest.rs
- mcp/tests/bdd.rs
- mcp/src/config.rs
- mcp/src/main.rs
- mcp/src/functions/handler.rs
This was referenced May 6, 2026
Merged
This was referenced Jul 3, 2026
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.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests