ONE-1222: Wire MCP gateway request handling#292
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoWire MCP JSON-RPC gateway handling for POST /mcp
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27931a7689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(world_ref) = body.world_ref.as_deref() { | ||
| candidate = candidate.with_world( | ||
| parse_entity_id_param(world_ref, "entity.body.world_ref").map_err(mcp_api_error)?, | ||
| ); |
There was a problem hiding this comment.
When an MCP credential is registered with a scoped world/facet, this code trusts the caller-supplied entity.body.world_ref (and the following scope) without comparing it to actor.scope from the resolved credential. Because the write Gate only checks the actor ceiling/source/criticality and not the connector's registered scope, a scoped connector with an allowed actor can submit oneiron.edit remember claims for a different world or no world at all, bypassing the scope that was validated in the actor metadata.
Useful? React with 👍 / 👎.
| let key = oneiron::claim::ScopedReadActorKey::with_actor_class( | ||
| &actor.gate_actor_ref, | ||
| actor.gate_actor_class, | ||
| ) |
There was a problem hiding this comment.
For credentials registered with McpConnectorScope::scoped, the read handle is created from only the actor ref/class, so oneiron.read and oneiron.nav searches are filtered solely by the actor's policy grants and never by the credential's world/facet scope. If the same actor has broader scoped-read grants than this connector credential, the connector can read results outside the scope it was issued for even though ensure_mcp_actor_matches required the request metadata to echo that narrower scope.
Useful? React with 👍 / 👎.
| let mcp_registry = Mutex::new(McpConnectorActorRegistry::new( | ||
| McpCredentialHashKey::from_bytes(mcp_registry_hash_key(&config)), |
There was a problem hiding this comment.
Every production SyncServer starts with this empty process-local registry, and this change adds no non-test path that calls McpConnectorActorRegistry::register, so /mcp initialize, tools/list, and tools/call all fail credential resolution with mcp_credential_unknown for any real connector credential. The new gateway is therefore only usable in the integration tests that mutate server.mcp_registry directly.
Useful? React with 👍 / 👎.
| ) | ||
| .with_field("entity") | ||
| })?; | ||
| let id = parse_optional_entity_id(entity.id.as_deref(), "entity.id").map_err(mcp_api_error)?; |
There was a problem hiding this comment.
When a client retries oneiron.edit remember after a timeout and omits the optional entity.id, this line mints a fresh EntityId::now() each time, while the required idempotency_key is only copied into provenance and never used to deduplicate the write. That makes transport retries create duplicate claims instead of replaying the original MCP edit.
Useful? React with 👍 / 👎.
| if args.dry_run { | ||
| return Ok(json!({ | ||
| "content": [mcp_text_content("edit validated")], |
There was a problem hiding this comment.
For dry_run: true, the handler returns success before running the remember-specific checks in execute_mcp_remember, so a non-claim entity or malformed claim body is reported as edit validated even though the same request fails when executed for real. Connectors that use dry run as a preflight will accept edits that cannot actually be applied.
Useful? React with 👍 / 👎.
| let source = mcp_claim_source(body.source.as_deref().unwrap_or("tool_output"))?; | ||
| let approval = match body.approval.as_deref() { | ||
| Some(value) => mcp_claim_approval(value)?, |
There was a problem hiding this comment.
For MCP writes, the connector controls both source and approval here. In the Gate path, source trust is only enforced for Auto approvals, so a connector with an auto actor ceiling can mark a generated/tool claim as approval: "approved" or relabel it as user_stated and bypass the source-trust/pending-consent behavior the gateway is supposed to preserve.
Useful? React with 👍 / 👎.
Code Review by Qodo
Context used 1. /mcp missing audit events
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| async fn mcp_gateway( | ||
| headers: HeaderMap, | ||
| State(server): State<Arc<SyncServer>>, | ||
| body: Bytes, | ||
| ) -> impl IntoResponse { | ||
| let raw: Value = match serde_json::from_slice(&body) { | ||
| Ok(raw) => raw, | ||
| Err(error) => { | ||
| return Json(mcp_error_response( | ||
| Value::Null, | ||
| McpGatewayError::new(-32700, "parse_error", error.to_string()), | ||
| )); | ||
| } | ||
| }; | ||
| let id = raw.get("id").cloned().unwrap_or(Value::Null); | ||
| let request = match serde_json::from_value::<McpJsonRpcRequest>(raw) { | ||
| Ok(request) => request, | ||
| Err(error) => { | ||
| return Json(mcp_error_response( | ||
| id, | ||
| McpGatewayError::new(-32600, "invalid_request", error.to_string()), | ||
| )); | ||
| } | ||
| }; | ||
| let id = request.id.clone().unwrap_or(id); | ||
|
|
||
| let result = handle_mcp_request(&headers, &server, request).await; | ||
| Json(match result { | ||
| Ok(result) => json!({ | ||
| "jsonrpc": "2.0", | ||
| "id": id, | ||
| "result": result, | ||
| }), | ||
| Err(error) => mcp_error_response(id, error), | ||
| }) | ||
| } | ||
|
|
||
| async fn handle_mcp_request( | ||
| headers: &HeaderMap, | ||
| server: &Arc<SyncServer>, | ||
| request: McpJsonRpcRequest, | ||
| ) -> Result<Value, McpGatewayError> { | ||
| if request.jsonrpc != "2.0" { | ||
| return Err( | ||
| McpGatewayError::new(-32600, "invalid_request", "jsonrpc must be \"2.0\"") | ||
| .with_field("jsonrpc"), | ||
| ); | ||
| } | ||
|
|
||
| match request.method.as_str() { | ||
| "initialize" => { | ||
| let actor = resolve_mcp_gateway_actor(headers, server).await?; | ||
| Ok(json!({ | ||
| "protocolVersion": MCP_PROTOCOL_VERSION, | ||
| "serverInfo": { | ||
| "name": "oneiron", | ||
| "version": env!("CARGO_PKG_VERSION"), | ||
| }, | ||
| "capabilities": { | ||
| "tools": { "listChanged": false }, | ||
| }, | ||
| "instructions": "Oneiron MCP exposes foreign-client tools over the same read and write Gate as the REST core surface. Use tools/list for schemas and tools/call with connector actor metadata matching this authenticated credential.", | ||
| "actor": mcp_actor_result(&actor), | ||
| })) | ||
| } | ||
| "notifications/initialized" => Ok(json!({})), | ||
| "tools/list" => { | ||
| let actor = resolve_mcp_gateway_actor(headers, server).await?; | ||
| Ok(json!({ | ||
| "tools": crate::mcp::mcp_tool_schemas(), | ||
| "actor": mcp_actor_result(&actor), | ||
| })) | ||
| } | ||
| "tools/call" => { | ||
| let actor = resolve_mcp_gateway_actor(headers, server).await?; | ||
| let params: McpToolCallParams = mcp_params(request.params, "params")?; | ||
| let tool = McpToolName::from_name(¶ms.name).ok_or_else(|| { | ||
| McpGatewayError::new( | ||
| -32602, | ||
| "unknown_tool", | ||
| "tool name is not advertised by this Oneiron MCP gateway", | ||
| ) | ||
| .with_field("name") | ||
| })?; | ||
| let args = validate_mcp_tool_args(tool, params.arguments) | ||
| .map_err(mcp_tool_validation_error)?; | ||
| ensure_mcp_actor_matches(&args, &actor)?; | ||
| execute_mcp_tool(server, args, &actor) | ||
| } | ||
| _ => Err(McpGatewayError::new( | ||
| -32601, | ||
| "method_not_found", | ||
| format!("unsupported MCP method {}", request.method), | ||
| ) | ||
| .with_field("method")), | ||
| } | ||
| } |
There was a problem hiding this comment.
1. /mcp missing audit events 📎 Requirement gap ◔ Observability
The new MCP gateway processes initialize, tools/list, and tools/call requests without emitting any audit event for allowed/denied/malformed outcomes. This violates the requirement for MCP gateway calls to emit audit events for accountability and observability.
Agent Prompt
## Issue description
The MCP gateway handler returns JSON-RPC results/errors but does not emit audit events for request outcomes (success/denial/malformed). The compliance checklist requires audit events for each processed MCP call outcome.
## Issue Context
The new `POST /mcp` route handles authentication, schema validation, and Gate outcomes, but there is no corresponding audit emission in the handler path.
## Fix Focus Areas
- crates/oneiron-server/src/api.rs[1600-1696]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| match request.method.as_str() { | ||
| "initialize" => { | ||
| let actor = resolve_mcp_gateway_actor(headers, server).await?; | ||
| Ok(json!({ | ||
| "protocolVersion": MCP_PROTOCOL_VERSION, | ||
| "serverInfo": { | ||
| "name": "oneiron", | ||
| "version": env!("CARGO_PKG_VERSION"), | ||
| }, | ||
| "capabilities": { | ||
| "tools": { "listChanged": false }, | ||
| }, | ||
| "instructions": "Oneiron MCP exposes foreign-client tools over the same read and write Gate as the REST core surface. Use tools/list for schemas and tools/call with connector actor metadata matching this authenticated credential.", | ||
| "actor": mcp_actor_result(&actor), | ||
| })) | ||
| } | ||
| "notifications/initialized" => Ok(json!({})), | ||
| "tools/list" => { |
There was a problem hiding this comment.
2. notifications/initialized skips auth 📎 Requirement gap ⛨ Security
The MCP method notifications/initialized is accepted without authenticating the connector actor. This can allow a supported MCP call to be processed without authentication, contrary to the MCP gateway compliance requirement.
Agent Prompt
## Issue description
`handle_mcp_request` accepts `notifications/initialized` without resolving/authenticating the MCP connector actor.
## Issue Context
Other MCP methods (`initialize`, `tools/list`, `tools/call`) call `resolve_mcp_gateway_actor(...)` before proceeding, but `notifications/initialized` returns `{}` immediately.
## Fix Focus Areas
- crates/oneiron-server/src/api.rs[1649-1666]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let credential = mcp_connector_credential(headers)?; | ||
| let registry = server.mcp_registry.lock().await; | ||
| registry | ||
| .resolve(&credential, unix_seconds_now(), |actor_class, actor_ref| { | ||
| server | ||
| .vault | ||
| .gate_actor_ceiling_exists(actor_class, actor_ref) | ||
| .unwrap_or(false) | ||
| }) |
There was a problem hiding this comment.
3. Registry lock throttles mcp 🐞 Bug ➹ Performance
resolve_mcp_gateway_actor() holds server.mcp_registry's async mutex while calling into vault.gate_actor_ceiling_exists() (LMDB read txn + policy manifest resolution), forcing all concurrent /mcp requests to serialize on that lock and increasing tail latency under load.
Agent Prompt
### Issue description
`resolve_mcp_gateway_actor` takes `server.mcp_registry.lock().await` and keeps it held while performing `vault.gate_actor_ceiling_exists(...)` inside the registry resolution closure. This makes every MCP request contend on a single async mutex, and it holds that mutex across LMDB work.
### Issue Context
- `/mcp` is expected to be used for repeated tool calls; a global mutex in the request hot-path will become a throughput bottleneck.
- The vault ceiling check opens a read transaction and resolves the policy manifest, which is not “free” work to do inside the registry mutex critical section.
### Fix Focus Areas
- crates/oneiron-server/src/api.rs[1698-1712]
- crates/oneiron-server/src/server.rs[60-66]
- crates/oneiron/src/vault.rs[5189-5194]
### Suggested change
- Refactor MCP actor resolution so the registry lock is held only long enough to look up/clone the connector actor record.
- Option A: Add a `lookup_resolved_actor(credential, now)`/`resolve_without_ceiling_check` method on `McpConnectorActorRegistry` that does not call back into the vault, returns `McpResolvedActor` (or the record).
- Then, after releasing the mutex, call `vault.gate_actor_ceiling_exists(...)` and map errors appropriately.
- Avoid `unwrap_or(false)` so vault/policy read errors can surface as engine/internal errors rather than being misreported as “missing actor ceiling”.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| fn mcp_actor_class_wire(actor_class: McpActorClass) -> &'static str { | ||
| match actor_class { | ||
| McpActorClass::Human => "human", | ||
| McpActorClass::Agent => "agent", | ||
| } |
There was a problem hiding this comment.
4. System actor class unsupported 🐞 Bug ≡ Correctness
The MCP gateway can resolve a connector registered as EdgeActorClass::System (gate actor class "system"), but MCP tool actor metadata and mcp_actor_class_wire only support human/agent, so such a credential can succeed at initialize/tools/list yet tools/call can never provide a schema-valid/matching actor_class.
Agent Prompt
### Issue description
MCP actor metadata uses `McpActorClass` (human/agent), but the connector registry stores `EdgeActorClass` which includes `System` and maps to the gate actor class string `"system"`. The gateway compares tool-provided actor_class against the resolved gate actor class; for system actors, there is no way to provide a schema-valid MCP actor_class that will match.
### Issue Context
This manifests as a confusing configuration:
- `initialize` and `tools/list` can resolve the credential and return an actor payload containing `"actor_class": "system"`.
- `tools/call` tool argument decoding/matching cannot ever accept `"system"` because MCP actor_class is constrained to human/agent.
### Fix Focus Areas
- crates/oneiron-server/src/api.rs[1805-1841]
- crates/oneiron-server/src/api.rs[2326-2343]
- crates/oneiron-server/src/mcp.rs[356-361]
- crates/oneiron-server/src/mcp.rs[1232-1247]
- crates/oneiron/src/types.rs[1078-1101]
### Suggested change (pick one)
1) **Fail fast with a clear error (recommended if system connectors are not intended):**
- In `resolve_mcp_gateway_actor` (or immediately after it), if `resolved.gate_actor_class == "system"`, return a stable MCP error like `mcp_actor_invalid` / `mcp_actor_class_unsupported` and do not proceed.
- Optionally also prevent registering `EdgeActorClass::System` as an MCP connector actor (change constructor to a `try_new` that rejects System).
2) **Add first-class system support (only if intended):**
- Extend `McpActorClass` to include `System` and update the tool schemas + `mcp_actor_class_wire` accordingly.
- Update validation and matching logic so `system` is representable and consistent end-to-end.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Linear: ONE-1222
Summary
POST /mcpfor initialize, tool listing, and tool calls.oneiron.edit rememberclaim writes through the existing Gate decision path.Validation
rtk proxy cargo fmt --all --checkrtk proxy cargo clippy --workspace --all-targets --all-features -- -D warningsrtk proxy cargo nextest run --workspace --all-features --profile full(2232 passed)rtk proxy cargo test --doc --workspace --exclude oneiron-bench --all-features(1 doc test passed; expected cdylib doc-test warning for oneiron-napi)rtk proxy env RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-depsrtk proxy cargo nextest run -p oneiron --features sync --profile full(1832 passed, 1 slow)rtk git diff --check