refactor(agent): type tool execution contract - #2022
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds typed MCP tool execution contracts, classifies built-in and external tools, centralizes fail-closed batch scheduling, excludes execution metadata from token estimates, provider schemas, and tape manifest hashes, and hardens MCP parameter-schema rendering. ChangesExecution Contract and Scheduling
MCP Tool Schema Rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRuntime
participant BatchPolicy
participant ToolCatalog
participant ToolExecutor
AgentRuntime->>BatchPolicy: submit permission mode and tool calls
BatchPolicy->>ToolCatalog: resolve execution contracts
ToolCatalog-->>BatchPolicy: return tool effect and execution mode
BatchPolicy-->>AgentRuntime: select parallel or sequential mode
AgentRuntime->>ToolExecutor: settle the selected batch
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/main/agent/deepchat/runtime/dispatch.test.ts (1)
103-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep execution metadata explicit in test fixtures.
makeAgentToolstill infers parallel execution from the nameread, so tests can omit the contract and preserve the name-based behavior this PR is removing. Default to sequential write and passPARALLEL_READ_TOOL_EXECUTIONexplicitly where needed.Suggested adjustment
function makeAgentTool( name: string, - execution: ToolExecutionContract = - name === 'read' ? PARALLEL_READ_TOOL_EXECUTION : SEQUENTIAL_WRITE_TOOL_EXECUTION + execution: ToolExecutionContract = SEQUENTIAL_WRITE_TOOL_EXECUTION ): MCPToolDefinition {🤖 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 `@test/main/agent/deepchat/runtime/dispatch.test.ts` around lines 103 - 109, Update the makeAgentTool test fixture to always default its execution contract to SEQUENTIAL_WRITE_TOOL_EXECUTION instead of inferring it from the tool name. Add PARALLEL_READ_TOOL_EXECUTION explicitly at each read-tool fixture call site that requires parallel execution.
🤖 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 `@src/renderer/src/components/mcp-config/components/McpToolPanel.vue`:
- Around line 181-185: Update the item-enum rendering condition near line 380 to
also accept parameters whose mapped type is “array[enum]”, matching the type
assigned by the enumValues mapper. Preserve the existing “array” behavior so
enum badges render for both array parameter variants.
---
Nitpick comments:
In `@test/main/agent/deepchat/runtime/dispatch.test.ts`:
- Around line 103-109: Update the makeAgentTool test fixture to always default
its execution contract to SEQUENTIAL_WRITE_TOOL_EXECUTION instead of inferring
it from the tool name. Add PARALLEL_READ_TOOL_EXECUTION explicitly at each
read-tool fixture call site that requires parallel execution.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4c7d988-fde0-4680-aa04-a6fd453f8b1e
📒 Files selected for processing (38)
docs/architecture/deepchat-agent-harness-boundaries/plan.mddocs/architecture/deepchat-agent-harness-boundaries/spec.mddocs/architecture/deepchat-agent-harness-boundaries/tasks.mdsrc/main/agent/deepchat/runtime/contextBuilder.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/toolExecutionPolicy.tssrc/main/mcp/index.tssrc/main/mcp/toolManager.tssrc/main/tape/domain/viewManifest.tssrc/main/tool/agentTools/agentImageGenerationTool.tssrc/main/tool/agentTools/agentMemoryTools.tssrc/main/tool/agentTools/agentPlanTool.tssrc/main/tool/agentTools/agentTapeTools.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/agentTools/chatSettingsTools.tssrc/main/tool/agentTools/cronJobTool.tssrc/main/tool/agentTools/subagentOrchestratorTool.tssrc/main/tool/browser/definitions.tssrc/main/tool/index.tssrc/renderer/src/components/mcp-config/components/McpToolPanel.vuesrc/shared/types/core/mcp.tssrc/shared/types/mcp.tstest/main/agent/acp/runtime/acpCompatibilityPromptBuilder.test.tstest/main/agent/deepchat/instance/deepChatAgentRuntime.test.tstest/main/agent/deepchat/runtime/contextBuilder.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/deepchat/runtime/toolExecutionPolicy.test.tstest/main/agent/deepchat/runtime/toolOutputGuard.test.tstest/main/evals/nativeAgent/harness.tstest/main/mcp/toolManager.test.tstest/main/provider/aiSdkToolMapper.test.tstest/main/provider/baseProvider.test.tstest/main/session/data/tapeViewManifest.test.tstest/main/tool/agentTools/agentToolManagerRead.test.tstest/main/tool/toolService.test.tstest/renderer/composables/useChatInputMentions.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/renderer/components/McpToolPanel.test.ts (1)
25-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this test into the mirrored component directory.
Place it under
test/renderer/components/mcp-config/components/McpToolPanel.test.tsto mirrorsrc/renderer/src/components/mcp-config/components/McpToolPanel.vue.As per coding guidelines, “Place tests in directories mirroring their source under
test/main/**andtest/renderer/**.”🤖 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 `@test/renderer/components/McpToolPanel.test.ts` around lines 25 - 109, Move the McpToolPanel test suite from the current renderer components test location into the mirrored mcp-config/components directory, preserving the existing describe block and test behavior unchanged.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/renderer/components/McpToolPanel.test.ts`:
- Around line 25-109: Move the McpToolPanel test suite from the current renderer
components test location into the mirrored mcp-config/components directory,
preserving the existing describe block and test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1d848e2-6e7e-4c8b-9964-71995aa27338
📒 Files selected for processing (35)
docs/architecture/deepchat-agent-harness-boundaries/plan.mddocs/architecture/deepchat-agent-harness-boundaries/spec.mddocs/architecture/deepchat-agent-harness-boundaries/tasks.mdsrc/main/agent/deepchat/runtime/toolExecutionPolicy.tssrc/main/mcp/index.tssrc/main/mcp/toolManager.tssrc/main/tool/agentTools/agentImageGenerationTool.tssrc/main/tool/agentTools/agentMemoryTools.tssrc/main/tool/agentTools/agentPlanTool.tssrc/main/tool/agentTools/agentTapeTools.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/agentTools/chatSettingsTools.tssrc/main/tool/agentTools/cronJobTool.tssrc/main/tool/agentTools/subagentOrchestratorTool.tssrc/main/tool/browser/definitions.tssrc/renderer/src/components/mcp-config/components/McpToolPanel.vuesrc/shared/types/core/mcp.tssrc/shared/types/mcp.tstest/main/agent/acp/runtime/acpCompatibilityPromptBuilder.test.tstest/main/agent/deepchat/instance/deepChatAgentRuntime.test.tstest/main/agent/deepchat/runtime/contextBuilder.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/deepchat/runtime/toolExecutionPolicy.test.tstest/main/agent/deepchat/runtime/toolOutputGuard.test.tstest/main/evals/nativeAgent/harness.tstest/main/mcp/toolManager.test.tstest/main/provider/aiSdkToolMapper.test.tstest/main/provider/baseProvider.test.tstest/main/session/data/tapeViewManifest.test.tstest/main/tool/agentTools/agentToolManagerRead.test.tstest/main/tool/toolService.test.tstest/renderer/components/McpToolPanel.test.tstest/renderer/composables/useChatInputMentions.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- test/main/mcp/toolManager.test.ts
- test/main/agent/deepchat/runtime/toolOutputGuard.test.ts
- docs/architecture/deepchat-agent-harness-boundaries/tasks.md
- src/main/agent/deepchat/runtime/toolExecutionPolicy.ts
- src/main/tool/agentTools/agentTapeTools.ts
- src/renderer/src/components/mcp-config/components/McpToolPanel.vue
- test/main/session/data/tapeViewManifest.test.ts
- test/main/agent/deepchat/runtime/toolExecutionPolicy.test.ts
- docs/architecture/deepchat-agent-harness-boundaries/spec.md
- docs/architecture/deepchat-agent-harness-boundaries/plan.md
- test/main/agent/deepchat/runtime/contextBuilder.test.ts
- test/main/agent/deepchat/runtime/dispatch.test.ts
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
Design
Each executable tool now declares:
effect:readorwriteexecutionMode:sequentialorparallelThe type contract makes
write + parallelunrepresentable.A batch runs in parallel only when:
full_access;read + parallel.Missing, malformed, duplicate, mixed, or write-capable definitions fall back to whole-batch sequential execution.
External MCP and plugin tools remain
write + sequential. Their advisory metadata is not trusted as an execution-safety boundary. Currently, only the filesystemreadtool opts into parallel execution.Summary by CodeRabbit