feat: mcp worker#82
Merged
sergiofilhowz merged 3 commits intomainfrom May 6, 2026
Merged
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (2)
📝 WalkthroughWalkthroughThe PR comprehensively refactors the MCP worker from a Cucumber BDD-based test suite to a production-ready HTTP/SSE binary worker. Changes include a new JSON-RPC 2.0 protocol stack with SSE transport, simplified configuration, skill/tool bridging logic, revised test infrastructure, and updated documentation including a new "Binary worker how-to" guide. ChangesMCP Worker Refactoring
Sequence DiagramsequenceDiagram
participant Client as JSON-RPC Client
participant Server as MCP Handler<br>(HTTP/SSE)
participant Engine as iii Engine
participant Skill as Skills Worker
participant Tool as Tool Dispatcher
Client->>Server: POST /mcp<br>(JSON-RPC initialize)
activate Server
Server->>Server: Validate JSON-RPC shape
Server->>Server: Parse request + extract ID
alt initialize
Server->>Server: handle_initialize()
Server-->>Server: Outcome::Response(success)
end
Server->>Server: Encode as JsonRpcResponse
Server->>Server: Format as SSE frame
Server-->>Client: 200 SSE<br>(event: message)
deactivate Server
Client->>Server: POST /mcp<br>(JSON-RPC tools/list)
activate Server
Server->>Server: Parse + route to tools handler
Server->>Engine: list_functions(ENGINE_LIST_FUNCTIONS)
activate Engine
Engine-->>Server: [{ id, name, metadata }]
deactivate Engine
Server->>Server: Filter hidden by config.hidden_prefixes
Server->>Server: Encode names, build inputSchema
Server-->>Server: Outcome::Response(tools array)
Server-->>Client: 200 SSE<br>(tools response)
deactivate Server
Client->>Server: POST /mcp<br>(JSON-RPC tools/call)
activate Server
Server->>Server: Parse tool_name, arguments
Server->>Tool: Decode tool_name (\_\_ → ::)
activate Tool
Tool->>Server: Visibility check vs config
deactivate Tool
Server->>Engine: Call function<br>(decoded name, args)
activate Engine
Engine-->>Server: Result (object/error)
deactivate Engine
Server->>Server: Stringify result<br>(text + structured)
Server-->>Server: Outcome::Response(content)
Server-->>Client: 200 SSE<br>(tool result)
deactivate Server
Client->>Server: POST /mcp<br>(JSON-RPC resources/list)
activate Server
Server->>Server: Route to skills_bridge
Server->>Skill: Delegate to skills::resources-list
activate Skill
Skill-->>Server: { resources: [...] }
deactivate Skill
Server->>Server: Filter nested iii:// URIs
Server-->>Server: Outcome::Response(filtered)
Server-->>Client: 200 SSE<br>(resources list)
deactivate Server
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 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: 11
🧹 Nitpick comments (1)
mcp/tests/integration.rs (1)
96-97: ⚡ Quick winReplace fixed sleep with an active readiness probe to reduce flakiness.
Line 97 uses a hardcoded 2s delay. In slower CI, worker registration can take longer and cause intermittent failures; in fast runs it just adds latency.
Proposed refactor
- // Give the worker a moment to register the http trigger with the engine. - sleep(Duration::from_millis(2000)).await; + // Wait until the MCP endpoint responds, up to a bounded timeout. + let client = reqwest::Client::builder() + .timeout(Duration::from_secs(2)) + .build() + .ok()?; + let ready_deadline = std::time::Instant::now() + Duration::from_secs(10); + loop { + if std::time::Instant::now() >= ready_deadline { + let _ = iii.kill(); + let _ = iii.wait(); + return None; + } + let probe = client + .post(ENGINE_HTTP) + .header("content-type", "application/json") + .header("accept", "text/event-stream") + .body(r#"{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}"#) + .send() + .await; + if probe.as_ref().map(|r| r.status().is_success()).unwrap_or(false) { + break; + } + sleep(Duration::from_millis(200)).await; + }🤖 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/integration.rs` around lines 96 - 97, Replace the fixed 2s sleep with an active readiness probe loop that polls for the worker's HTTP trigger registration and only proceeds once registration is observed (or a configurable timeout is reached); specifically, remove the call to sleep(Duration::from_millis(2000)).await and implement an async retry loop (polling every ~100–250ms up to a max e.g. 10s) that queries the engine/registry endpoint or uses the existing client call used elsewhere in this test to verify the trigger is registered before continuing, and fail the test if the timeout elapses. Ensure the probe uses the same HTTP/client functions used in this test file so it reliably detects the worker registration.
🤖 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 `@binary-worker.md`:
- Around line 538-540: Update the documentation to state that
CARGO_BIN_EXE_<BIN> uses the binary name exactly as-is (hyphens preserved)
rather than converting hyphens to underscores; change the descriptive text that
currently claims hyphens are replaced to instead show the correct placeholder
CARGO_BIN_EXE_<worker> (not CARGO_BIN_EXE_<worker_underscored>) and replace
every occurrence of CARGO_BIN_EXE_<worker_underscored> with
CARGO_BIN_EXE_<worker> so examples and explanations use the exact binary name
form used by env!("CARGO_BIN_EXE_<worker>").
In `@mcp/config.yaml`:
- Line 22: The api_path value currently includes a leading slash which causes
double-slash routes; update the RegisterTriggerInput.config.api_path entry from
"/mcp" to "mcp" and likewise remove leading slashes from any example/override
values (e.g. change "/mcp-rs" to "mcp-rs") so the iii-engine's prepended '/'
produces the correct route; verify any other api_path usages follow the same
slash-free convention.
In `@mcp/README.md`:
- Around line 73-78: Update the README examples to remove the leading slash from
api_path values so they match the config.yaml/config.rs change: change the
config block value api_path: "/mcp" to api_path: "mcp" and change the override
example `/mcp-rs` to `mcp-rs` wherever they appear; ensure references to
api_path and the example showing co-deploy with the TypeScript bridge (text
mentioning src/mcp/) are updated accordingly.
In `@mcp/src/config.rs`:
- Around line 32-34: The default_api_path() function currently returns "/mcp"
which introduces a leading slash and causes double-slash routes; change it to
return "mcp" (remove the leading '/') and update all usages/tests accordingly:
update default_api_path(), the two test assertions in config tests, the test
YAML example, the hardcoded "api_path": "/mcp" in default_config in manifest.rs
to "mcp", and the api_path value/comment in config.yaml so no api_path values
begin with a leading slash.
In `@mcp/src/functions/mod.rs`:
- Around line 95-105: The send_sse_control error path returns early and skips
closing the SSE channel, leaving clients hanging; update the error handling
around send_sse_control(&writer, status).await in functions/mod.rs so that
writer.close().await (or the appropriate close method on writer) is invoked
before returning when send_sse_control fails—e.g., ensure in the Err(err) branch
you call writer.close().await, log the error with writer_ref.channel_id and
status as currently done, then return Ok(Value::Null); alternatively refactor to
use a scope or finally-style drop/cleanup to always call writer.close() after
set_status/set_headers/send_sse_control.
In `@mcp/src/jsonrpc.rs`:
- Around line 180-185: parse_json_rpc_request currently treats any unsupported
or malformed "id" as None (notification); change it to detect when the "id" key
is present but JsonRpcId::from_value fails and surface that as an error instead
of silently dropping the id. Update parse_json_rpc_request to return
Result<JsonRpcRequest, ParseError> (or similar) so that if
obj.contains_key("id") but JsonRpcId::from_value returns None/Err you return Err
indicating an invalid id; then ensure handle_mcp_body maps that parse error to
an INVALID_REQUEST response with id: null. You can either change
JsonRpcId::from_value to return Result<JsonRpcId, _> or keep it and check
obj.get("id").is_some() && JsonRpcId::from_value(...).is_none() to trigger the
error path.
In `@mcp/src/manifest.rs`:
- Around line 19-23: The default_config JSON in manifest.rs currently hardcodes
"api_path": "/mcp", which will diverge from the new cfg.api_path value after you
stripped the leading slash; update that literal to "mcp" (or better, replace the
literal with a call to default_api_path()) so the default_config["api_path"]
matches cfg.api_path used in the guard test (see the serde_json::json! block and
functions default_hidden_prefixes() and default_api_path()).
In `@mcp/src/skills_bridge.rs`:
- Around line 13-33: The current is_missing_delegate() treats any
Runtime/Handler message containing "not found" as a missing delegate; tighten it
so only explicit function-missing signals are considered: keep the Remote branch
behavior (IIIError::Remote checks code == "NOT_FOUND" or "FUNCTION_NOT_FOUND")
but change the Runtime/Handler branches to look only for explicit
function-related indicators (e.g., exact phrases or a strict regex like
"function not found" or "unknown function" or "no such function") and do not
match generic "not found" or "resource not found"; update is_missing_delegate to
only return true for these explicit function-missing patterns.
In `@mcp/src/tools.rs`:
- Around line 157-160: The code currently defaults non-object
params.get("arguments") to an empty JSON and proceeds to call the tool (e.g.,
iii) which can cause unintended side effects; change the logic in tools.rs so
that when params.get("arguments") exists but is not an object (e.g., array or
string) you reject the request early rather than setting arguments = json!({}).
Specifically, in the block that inspects params.get("arguments") use a strict
type check and return an error (or Err variant) explaining "malformed arguments"
instead of assigning {} and invoking iii (or other tool call sites), leaving the
normal path only for Some(v) if v.is_object().
In `@mcp/tests/integration.rs`:
- Around line 78-94: boot() can return early after spawning the worker process
(the child returned by Command::new(worker_bin).spawn()) which leaves that
process ("iii") orphaned; ensure the spawned child is cleaned up on all early
returns before Harness is created by capturing the Child (e.g., worker or
worker_child) and calling kill() and wait() (or otherwise terminating it)
whenever boot() would return None, or refactor so Harness owns the Child
immediately (so its Drop will kill it); update the code around the worker_bin /
Command::new(...).spawn() site to guarantee the child is terminated on any error
path before returning.
In `@mcp/tests/manifest.rs`:
- Around line 29-31: The test currently only checks
manifest["default_config"]["api_path"].is_string(); update the test in
manifest.rs to also assert the value does not start with a leading slash (e.g.,
ensure the string for manifest["default_config"]["api_path"] does not begin with
'/'), so the contract that api_path is slashless is enforced and prevents
double-slash issues during iii engine matching.
---
Nitpick comments:
In `@mcp/tests/integration.rs`:
- Around line 96-97: Replace the fixed 2s sleep with an active readiness probe
loop that polls for the worker's HTTP trigger registration and only proceeds
once registration is observed (or a configurable timeout is reached);
specifically, remove the call to sleep(Duration::from_millis(2000)).await and
implement an async retry loop (polling every ~100–250ms up to a max e.g. 10s)
that queries the engine/registry endpoint or uses the existing client call used
elsewhere in this test to verify the trigger is registered before continuing,
and fail the test if the timeout elapses. Ensure the probe uses the same
HTTP/client functions used in this test file so it reliably detects the worker
registration.
🪄 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: 32e6c9ae-ea63-40d6-83ed-6fa9a3ff6c52
⛔ Files ignored due to path filters (1)
mcp/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
binary-worker.mdmcp/Cargo.tomlmcp/README.mdmcp/config.yamlmcp/iii.worker.yamlmcp/src/config.rsmcp/src/functions/handler.rsmcp/src/functions/mod.rsmcp/src/handler.rsmcp/src/jsonrpc.rsmcp/src/lib.rsmcp/src/main.rsmcp/src/manifest.rsmcp/src/protocol.rsmcp/src/skills_bridge.rsmcp/src/tools.rsmcp/src/transport.rsmcp/tests/bdd.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/integration.rsmcp/tests/manifest.rsmcp/tests/steps/core.rsmcp/tests/steps/mod.rsmcp/tests/steps/prompts.rsmcp/tests/steps/resources.rsmcp/tests/steps/tools.rsworker-readme.md
💤 Files with no reviewable changes (17)
- mcp/tests/common/engine.rs
- mcp/tests/features/tools.feature
- mcp/tests/common/mod.rs
- mcp/tests/features/prompts.feature
- mcp/tests/steps/mod.rs
- mcp/tests/steps/resources.rs
- mcp/tests/steps/core.rs
- mcp/tests/steps/prompts.rs
- mcp/src/protocol.rs
- mcp/tests/common/world.rs
- mcp/src/lib.rs
- mcp/tests/bdd.rs
- mcp/tests/features/core.feature
- mcp/src/functions/handler.rs
- mcp/tests/features/resources.feature
- mcp/tests/steps/tools.rs
- mcp/tests/common/workers.rs
Comment on lines
+538
to
+540
| **`CARGO_BIN_EXE_*`:** Cargo exposes `env!("CARGO_BIN_EXE_<BIN>")` where `<BIN>` | ||
| is the **binary name** with `-` replaced by `_` (e.g. `acme-helper` → | ||
| `CARGO_BIN_EXE_acme_helper`). |
There was a problem hiding this comment.
CARGO_BIN_EXE_* uses the binary name exactly as-is — hyphens are preserved, not converted to underscores.
The official Cargo reference states: "The <n> is the name of the binary target, exactly as-is. For example, CARGO_BIN_EXE_my-program for a binary named my-program."
The guide currently says hyphens become underscores, which would produce a silent compile error (env! on an undefined variable) for anyone with a hyphenated binary name. The placeholder throughout the guide should be <worker> (not <worker_underscored>), and the descriptive text needs correction.
📝 Proposed fix
-**`CARGO_BIN_EXE_*`:** Cargo exposes `env!("CARGO_BIN_EXE_<BIN>")` where `<BIN>`
-is the **binary name** with `-` replaced by `_` (e.g. `acme-helper` →
-`CARGO_BIN_EXE_acme_helper`).
+**`CARGO_BIN_EXE_*`:** Cargo exposes `env!("CARGO_BIN_EXE_<BIN>")` where `<BIN>`
+is the **binary name exactly as-is** (e.g. `acme-helper` →
+`CARGO_BIN_EXE_acme-helper`).Also update every occurrence of CARGO_BIN_EXE_<worker_underscored> in this file (lines 554, 586, 640, 1164, 1233) to CARGO_BIN_EXE_<worker>.
🤖 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 `@binary-worker.md` around lines 538 - 540, Update the documentation to state
that CARGO_BIN_EXE_<BIN> uses the binary name exactly as-is (hyphens preserved)
rather than converting hyphens to underscores; change the descriptive text that
currently claims hyphens are replaced to instead show the correct placeholder
CARGO_BIN_EXE_<worker> (not CARGO_BIN_EXE_<worker_underscored>) and replace
every occurrence of CARGO_BIN_EXE_<worker_underscored> with
CARGO_BIN_EXE_<worker> so examples and explanations use the exact binary name
form used by env!("CARGO_BIN_EXE_<worker>").
| # HTTP path bound by the bridge's `http` trigger. Override this if you need to | ||
| # co-deploy with the TypeScript MCP bridge in `src/mcp/` (which also binds | ||
| # `/mcp`); for example, `/mcp-rs`. | ||
| api_path: "/mcp" |
There was a problem hiding this comment.
api_path must not carry a leading slash — will produce //mcp at the engine.
Per the codebase convention, RegisterTriggerInput.config.api_path must be slash-free (e.g. "mcp", not "/mcp"); the iii-engine prepends its own / during route matching, so a leading slash yields a double-slash path and causes 404s in downstream deploys. The comment's override example (/mcp-rs) also needs updating.
🐛 Proposed fix
-# co-deploy with the TypeScript MCP bridge in `src/mcp/` (which also binds
-# `/mcp`); for example, `/mcp-rs`.
-api_path: "/mcp"
+# co-deploy with the TypeScript MCP bridge in `src/mcp/` (which also binds
+# `mcp`); for example, `mcp-rs`.
+api_path: "mcp"Based on learnings from the iii-hq/workers codebase: api_path values passed to RegisterTriggerInput must never start with a leading /; the iii-engine prepends its own slash.
🤖 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/config.yaml` at line 22, The api_path value currently includes a leading
slash which causes double-slash routes; update the
RegisterTriggerInput.config.api_path entry from "/mcp" to "mcp" and likewise
remove leading slashes from any example/override values (e.g. change "/mcp-rs"
to "mcp-rs") so the iii-engine's prepended '/' produces the correct route;
verify any other api_path usages follow the same slash-free convention.
Comment on lines
+73
to
+78
| api_path: "/mcp" | ||
| ``` | ||
|
|
||
| CLI flags: | ||
| `require_expose: true` advertises only functions whose `metadata.mcp.expose == | ||
| true`. `api_path` lets you co-deploy with the TypeScript bridge in | ||
| `src/mcp/` by binding a different path (e.g. `/mcp-rs`). |
There was a problem hiding this comment.
api_path values in the docs also need the leading slash stripped.
Both the config block (api_path: "/mcp") and the override example (/mcp-rs) carry a leading slash here. These need to match the fix in config.yaml and config.rs ("mcp" / "mcp-rs").
🤖 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/README.md` around lines 73 - 78, Update the README examples to remove the
leading slash from api_path values so they match the config.yaml/config.rs
change: change the config block value api_path: "/mcp" to api_path: "mcp" and
change the override example `/mcp-rs` to `mcp-rs` wherever they appear; ensure
references to api_path and the example showing co-deploy with the TypeScript
bridge (text mentioning src/mcp/) are updated accordingly.
Comment on lines
+32
to
34
| fn default_api_path() -> String { | ||
| "/mcp".to_string() | ||
| } |
There was a problem hiding this comment.
default_api_path() must not return a leading slash — this is the code root cause of the //mcp routing failure.
cfg.api_path is passed verbatim into RegisterTriggerInput.config["api_path"] in main.rs. The iii-engine prepends its own / during HTTP route matching, so "/mcp" becomes "//mcp" → 404 for every MCP request.
The same fix must be applied in all three co-sources:
mcp/src/config.rslines 33, 61, 70, 75 (default_api_path, two test assertions, one test YAML)mcp/config.yamlline 22 and its comment examplemcp/src/manifest.rsline 22 ("api_path": "/mcp"hardcoded indefault_config)
🐛 Proposed fix for `mcp/src/config.rs`
fn default_api_path() -> String {
- "/mcp".to_string()
+ "mcp".to_string()
}- assert_eq!(cfg.api_path, "/mcp");
+ assert_eq!(cfg.api_path, "mcp");-api_path: "/mcp-rs"
+api_path: "mcp-rs"- assert_eq!(cfg.api_path, "/mcp-rs");
+ assert_eq!(cfg.api_path, "mcp-rs");Based on learnings from the iii-hq/workers codebase: api_path values in RegisterTriggerInput.config must never start with a leading /; the iii-engine prepends its own slash, causing //-prefixed paths and 404s.
🤖 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/config.rs` around lines 32 - 34, The default_api_path() function
currently returns "/mcp" which introduces a leading slash and causes
double-slash routes; change it to return "mcp" (remove the leading '/') and
update all usages/tests accordingly: update default_api_path(), the two test
assertions in config tests, the test YAML example, the hardcoded "api_path":
"/mcp" in default_config in manifest.rs to "mcp", and the api_path value/comment
in config.yaml so no api_path values begin with a leading slash.
Comment on lines
+95
to
105
| if let Err(err) = send_sse_control(&writer, status).await { | ||
| ctx.log.warn( | ||
| "mcp: failed to send SSE control frames", | ||
| Some(json!({ | ||
| "error": err.to_string(), | ||
| "status": status, | ||
| "channel_id": writer_ref.channel_id, | ||
| })), | ||
| ); | ||
| return Ok(Value::Null); | ||
| } |
There was a problem hiding this comment.
Close the SSE channel even when control-frame setup fails.
This early return skips writer.close(). If set_status succeeds and set_headers fails, the client can be left waiting on an open response channel until timeout.
Proposed fix
if let Err(err) = send_sse_control(&writer, status).await {
ctx.log.warn(
"mcp: failed to send SSE control frames",
Some(json!({
"error": err.to_string(),
"status": status,
"channel_id": writer_ref.channel_id,
})),
);
+ let _ = writer.close().await;
return Ok(Value::Null);
}📝 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.
Suggested change
| if let Err(err) = send_sse_control(&writer, status).await { | |
| ctx.log.warn( | |
| "mcp: failed to send SSE control frames", | |
| Some(json!({ | |
| "error": err.to_string(), | |
| "status": status, | |
| "channel_id": writer_ref.channel_id, | |
| })), | |
| ); | |
| return Ok(Value::Null); | |
| } | |
| if let Err(err) = send_sse_control(&writer, status).await { | |
| ctx.log.warn( | |
| "mcp: failed to send SSE control frames", | |
| Some(json!({ | |
| "error": err.to_string(), | |
| "status": status, | |
| "channel_id": writer_ref.channel_id, | |
| })), | |
| ); | |
| let _ = writer.close().await; | |
| return Ok(Value::Null); | |
| } |
🤖 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/functions/mod.rs` around lines 95 - 105, The send_sse_control error
path returns early and skips closing the SSE channel, leaving clients hanging;
update the error handling around send_sse_control(&writer, status).await in
functions/mod.rs so that writer.close().await (or the appropriate close method
on writer) is invoked before returning when send_sse_control fails—e.g., ensure
in the Err(err) branch you call writer.close().await, log the error with
writer_ref.channel_id and status as currently done, then return Ok(Value::Null);
alternatively refactor to use a scope or finally-style drop/cleanup to always
call writer.close() after set_status/set_headers/send_sse_control.
Comment on lines
+19
to
+23
| default_config: serde_json::json!({ | ||
| "hidden_prefixes": default_hidden_prefixes(), | ||
| "require_expose": false, | ||
| "api_path": "/mcp", | ||
| }), |
There was a problem hiding this comment.
Hardcoded "/mcp" here won't be fixed by changing default_api_path() — it needs an independent update.
The "api_path" in the serde_json::json! block is a separate literal, not a call to default_api_path(). After stripping the slash from config.rs, this value also needs to change to "mcp". The guard test at line 57 (m.default_config["api_path"] == cfg.api_path) will fail until both are updated in lockstep.
🐛 Proposed fix
- "api_path": "/mcp",
+ "api_path": "mcp",Optionally, import and call default_api_path() here instead of a literal to eliminate the drift risk entirely.
📝 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.
Suggested change
| default_config: serde_json::json!({ | |
| "hidden_prefixes": default_hidden_prefixes(), | |
| "require_expose": false, | |
| "api_path": "/mcp", | |
| }), | |
| default_config: serde_json::json!({ | |
| "hidden_prefixes": default_hidden_prefixes(), | |
| "require_expose": false, | |
| "api_path": "mcp", | |
| }), |
🤖 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 19 - 23, The default_config JSON in
manifest.rs currently hardcodes "api_path": "/mcp", which will diverge from the
new cfg.api_path value after you stripped the leading slash; update that literal
to "mcp" (or better, replace the literal with a call to default_api_path()) so
the default_config["api_path"] matches cfg.api_path used in the guard test (see
the serde_json::json! block and functions default_hidden_prefixes() and
default_api_path()).
Comment on lines
+13
to
+33
| fn is_missing_delegate(err: &IIIError) -> bool { | ||
| match err { | ||
| IIIError::Remote { code, message, .. } => { | ||
| let upper = code.to_uppercase(); | ||
| if upper == "NOT_FOUND" || upper == "FUNCTION_NOT_FOUND" { | ||
| return true; | ||
| } | ||
| let lower = message.to_lowercase(); | ||
| lower.contains("not found") | ||
| || lower.contains("unknown function") | ||
| || lower.contains("no such function") | ||
| } | ||
| IIIError::Runtime(message) | IIIError::Handler(message) => { | ||
| let lower = message.to_lowercase(); | ||
| lower.contains("not found") | ||
| || lower.contains("unknown function") | ||
| || lower.contains("no such function") | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
is_missing_delegate() is broad enough to hide real resource/prompt failures.
Matching any Handler/Runtime message containing "not found" means a real delegate error like "resource not found" gets converted into an empty MCP response. That makes genuine read/get failures look like “nothing here” instead of surfacing the actual error.
Please tighten this to explicit function-missing signals from the engine dispatch layer rather than generic substring matches.
🤖 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/skills_bridge.rs` around lines 13 - 33, The current
is_missing_delegate() treats any Runtime/Handler message containing "not found"
as a missing delegate; tighten it so only explicit function-missing signals are
considered: keep the Remote branch behavior (IIIError::Remote checks code ==
"NOT_FOUND" or "FUNCTION_NOT_FOUND") but change the Runtime/Handler branches to
look only for explicit function-related indicators (e.g., exact phrases or a
strict regex like "function not found" or "unknown function" or "no such
function") and do not match generic "not found" or "resource not found"; update
is_missing_delegate to only return true for these explicit function-missing
patterns.
Comment on lines
+157
to
+160
| let arguments = match params.get("arguments") { | ||
| Some(v) if v.is_object() => v.clone(), | ||
| _ => json!({}), | ||
| }; |
There was a problem hiding this comment.
Reject malformed arguments instead of silently calling the tool with {}.
Right now {"arguments": []} or {"arguments": "x"} still invokes the iii function with an empty payload. That turns an invalid MCP request into a real tool execution, which is risky for tools with defaults or side effects.
Proposed fix
- let arguments = match params.get("arguments") {
- Some(v) if v.is_object() => v.clone(),
- _ => json!({}),
- };
+ let arguments = match params.get("arguments") {
+ None | Some(Value::Null) => json!({}),
+ Some(v) if v.is_object() => v.clone(),
+ Some(_) => {
+ return Err(McpToolsCallError::InvalidParams(McpInvalidParamsError(
+ "tools/call arguments must be an object".into(),
+ )));
+ }
+ };🤖 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/tools.rs` around lines 157 - 160, The code currently defaults
non-object params.get("arguments") to an empty JSON and proceeds to call the
tool (e.g., iii) which can cause unintended side effects; change the logic in
tools.rs so that when params.get("arguments") exists but is not an object (e.g.,
array or string) you reject the request early rather than setting arguments =
json!({}). Specifically, in the block that inspects params.get("arguments") use
a strict type check and return an error (or Err variant) explaining "malformed
arguments" instead of assigning {} and invoking iii (or other tool call sites),
leaving the normal path only for Some(v) if v.is_object().
Comment on lines
+78
to
+94
| if !wait_for_tcp(WS_PROBE_ADDR, Duration::from_secs(10)).await { | ||
| return None; | ||
| } | ||
| if !wait_for_tcp(HTTP_PROBE_ADDR, Duration::from_secs(10)).await { | ||
| return None; | ||
| } | ||
|
|
||
| let worker_bin = env!("CARGO_BIN_EXE_mcp"); | ||
| let manifest_dir = env!("CARGO_MANIFEST_DIR"); | ||
| let config_path = format!("{manifest_dir}/config.yaml"); | ||
|
|
||
| let worker = Command::new(worker_bin) | ||
| .args(["--url", ENGINE_WS, "--config", &config_path]) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .ok()?; |
There was a problem hiding this comment.
Prevent orphan iii processes on early boot() failures.
On Line 78, Line 81, and Line 94, boot() returns None after iii is spawned, but before Harness is created. That bypasses Drop cleanup and can leave iii running, causing cross-test interference and port conflicts.
Proposed fix
async fn boot() -> Option<Harness> {
@@
- let iii = Command::new(&iii_bin)
+ let mut iii = Command::new(&iii_bin)
.arg("--use-default-config")
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
.ok()?;
if !wait_for_tcp(WS_PROBE_ADDR, Duration::from_secs(10)).await {
+ let _ = iii.kill();
+ let _ = iii.wait();
return None;
}
if !wait_for_tcp(HTTP_PROBE_ADDR, Duration::from_secs(10)).await {
+ let _ = iii.kill();
+ let _ = iii.wait();
return None;
}
@@
- let worker = Command::new(worker_bin)
+ let worker = Command::new(worker_bin)
.args(["--url", ENGINE_WS, "--config", &config_path])
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
- .ok()?;
+ .ok()
+ .or_else(|| {
+ let _ = iii.kill();
+ let _ = iii.wait();
+ None
+ })?;🤖 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/integration.rs` around lines 78 - 94, boot() can return early after
spawning the worker process (the child returned by
Command::new(worker_bin).spawn()) which leaves that process ("iii") orphaned;
ensure the spawned child is cleaned up on all early returns before Harness is
created by capturing the Child (e.g., worker or worker_child) and calling kill()
and wait() (or otherwise terminating it) whenever boot() would return None, or
refactor so Harness owns the Child immediately (so its Drop will kill it);
update the code around the worker_bin / Command::new(...).spawn() site to
guarantee the child is terminated on any error path before returning.
Comment on lines
+29
to
+31
| assert!(manifest["default_config"].is_object()); | ||
| assert!(manifest["default_config"]["hidden_prefixes"].is_array()); | ||
| assert!(manifest["default_config"]["api_path"].is_string()); |
There was a problem hiding this comment.
Assert the manifest api_path is slashless here.
Checking only is_string() will still accept "/mcp", which is the path shape that breaks iii trigger matching with a double slash. This test is a good place to lock that contract down.
Proposed fix
assert!(manifest["default_config"].is_object());
assert!(manifest["default_config"]["hidden_prefixes"].is_array());
- assert!(manifest["default_config"]["api_path"].is_string());
+ let api_path = manifest["default_config"]["api_path"]
+ .as_str()
+ .expect("default_config.api_path must be a string");
+ assert!(!api_path.is_empty());
+ assert!(!api_path.starts_with('/'));Based on learnings: api_path must not start with a leading slash because the iii engine prepends / during path matching, and //... can 404 downstream.
📝 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.
Suggested change
| assert!(manifest["default_config"].is_object()); | |
| assert!(manifest["default_config"]["hidden_prefixes"].is_array()); | |
| assert!(manifest["default_config"]["api_path"].is_string()); | |
| assert!(manifest["default_config"].is_object()); | |
| assert!(manifest["default_config"]["hidden_prefixes"].is_array()); | |
| let api_path = manifest["default_config"]["api_path"] | |
| .as_str() | |
| .expect("default_config.api_path must be a string"); | |
| assert!(!api_path.is_empty()); | |
| assert!(!api_path.starts_with('/')); |
🤖 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/manifest.rs` around lines 29 - 31, The test currently only checks
manifest["default_config"]["api_path"].is_string(); update the test in
manifest.rs to also assert the value does not start with a leading slash (e.g.,
ensure the string for manifest["default_config"]["api_path"] does not begin with
'/'), so the contract that api_path is slashless is enforced and prevents
double-slash issues during iii engine matching.
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
Release Notes
New Features
Documentation
Tests