From ee16a4debcfe6a26addea5c9b4c318a2460a2b4c Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 20 Jun 2026 18:17:01 +0900 Subject: [PATCH] ticket: approve mcp tools call --- .yoi/tickets/00001KVHR3WSD/item.md | 2 +- .yoi/tickets/00001KVHR3WSD/thread.md | 50 ++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/.yoi/tickets/00001KVHR3WSD/item.md b/.yoi/tickets/00001KVHR3WSD/item.md index f06fcfd0..e937bfc8 100644 --- a/.yoi/tickets/00001KVHR3WSD/item.md +++ b/.yoi/tickets/00001KVHR3WSD/item.md @@ -2,7 +2,7 @@ title: 'MCP: execute tools/call through ordinary Tool path' state: 'inprogress' created_at: '2026-06-20T05:30:04Z' -updated_at: '2026-06-20T09:09:50Z' +updated_at: '2026-06-20T09:16:53Z' assignee: null readiness: 'implementation_ready' risk_flags: ['mcp', 'tools-call', 'permission', 'history', 'bounded-output'] diff --git a/.yoi/tickets/00001KVHR3WSD/thread.md b/.yoi/tickets/00001KVHR3WSD/thread.md index f9e49eab..2d7c4972 100644 --- a/.yoi/tickets/00001KVHR3WSD/thread.md +++ b/.yoi/tickets/00001KVHR3WSD/thread.md @@ -167,3 +167,53 @@ Review dispatch: - Reviewer is instructed not to edit source, commit, merge, close the Ticket, or use TicketReview directly; it will report verdict/evidence back to Orchestrator。 --- + + + +## Review: approve + +Verdict: `approve` + +確認範囲: +- Ticket contract / Orchestrator IntentPacket。 +- Implementation diff: `92432ad7..9a245403`。 +- 主な対象: `crates/pod/src/feature/mcp.rs`, `crates/mcp/src/stdio.rs`, `crates/mcp/tests/stdio_lifecycle.rs`, `crates/mcp/tests/fixtures/mock_server.rs`。 +- Ordinary Worker/permission path: `crates/llm-worker/src/worker.rs`, `crates/pod/src/permission.rs`, `crates/pod/src/feature.rs`。 +- Out-of-scope MCP surfaces (`resources/read`, `prompts/get`, `list_changed`, sampling/elicitation) の追加有無。 + +Blocking issues: none。 + +Critical risk checks: +- Permission denial occurs before `tools/call` can be sent: + - MCP-discovered tools are ordinary `ToolDefinition`s wrapping `McpStdioTool`。 + - Protocol-provider contributions install through `register_tool_contribution`, not a private bypass。 + - Worker executes `pre_tool_call` first and only executes approved tools afterward。 + - Pod manifest permissions are `PreToolCall` hooks returning deny as synthetic results。 +- Result flow uses ordinary Tool result/history path: + - `McpStdioTool::execute` returns `ToolOutput`。 + - Worker converts to `ToolResult` and commits through existing history append path。 + - No hidden context injection or out-of-band model context path found。 +- Normal MCP result / MCP `isError: true` / JSON-RPC protocol error are distinct: + - `CallToolResult` carries `is_error` separately from JSON-RPC error。 + - MCP `isError: true` renders as model-visible untrusted data with `"status": "mcp_is_error"`。 + - JSON-RPC error becomes `ToolError::ExecutionFailed` with protocol error wording。 +- Output bounding/untrusted handling exists for content blocks, text/string caps, JSON depth/node caps, rich image/audio data omission, and final byte cap。 +- Result root is marked `untrusted_mcp_tools_call_result` and preserves server payload as data。 +- Existing lifecycle/redaction/shutdown behavior is preserved。 +- No resources/prompts/list_changed/sampling/elicitation scope creep found。 + +Non-blocking concerns / follow-ups: +- MCP-specific permission-denial test is lifecycle no-call style, while actual Worker deny/no-execute behavior is covered by generic Worker synthetic-result test path. Code inspection confirms MCP tools use ordinary Worker path, so this is not blocking. Future MCP-specific Worker-level regression would make the invariant more direct。 +- Minor wording nit: successful `tools/call` with shutdown failure says “shutdown after tools/call failed”; functionally non-blocking。 + +Reviewer validation: +- `git diff --check 92432ad7..HEAD`: passed。 +- `cargo fmt --check`: passed。 +- `cargo test -p mcp --test stdio_lifecycle`: passed, 12 tests。 +- `cargo test -p pod feature::mcp`: passed, 9 tests。 +- `cargo check -p mcp -p pod`: passed。 +- `nix build .#yoi --no-link` not run by Reviewer because diff has no Cargo/Nix/source-filter changes and focused Rust checks passed。 + +Worktree status at review end: clean。 + +---