Repository navigation
fix(agent): allow Code Mode subagent tools - #2473
Conversation
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Parent tool dispatch authority src/main/tape/domain/executionContract.ts, test/main/tape/executionContract.test.ts |
Dispatch checks use the parent tool’s frozen ceiling when provided. Tests cover parent membership, target matching, effect and mode limits, workspace, subagent depth, and request identity. |
Code Mode parent propagation src/main/tool/index.ts, test/main/tool/toolService.test.ts |
Code Mode passes the matching entry-tool definition through runtime options to nested dispatch validation. Tests exercise nested Agent and MCP calls under both provider entrypoints. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: yyhhyyyyyy
Merge Risk: 
Minimal · up to a087b
-
No actionable merge-blocking issue was established; the change is ready to merge after normal checks.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Linked Issues check
-
Explanation
Issue
#2471requires Code Mode nested tools to use the outer entrypoint ceiling, limit child effects, and preserve workspace and subagent-depth checks.executionContract.tsuses the supplied parent tool for frozen membership, target, and mode validation. It also requires the child effect to fit within the parent effect.tool/index.tssupplies the configured function or Codex Code Mode entrypoint as the parent. Tests coverexec,read,glob, and an MCP tool for bothdeepseekandopenai-codex. The contract tests retain coverage for workspace, depth, and request identity checks.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#2471. Production changes add parent-entrypoint dispatch authorization and pass the entrypoint through Code Mode runtime options. The added tests verify the required nested native and MCP calls and the existing security restrictions. No unrelated behavior or files appear in the reviewed diff.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
zerob13
left a comment
There was a problem hiding this comment.
Review: approve, no blocking findings
This is a self-authored PR, so GitHub does not allow submitting a formal approve or request-changes review. Recording the verdict here instead.
What it does
Before this change, a Code Mode turn froze only its top-level entrypoint (run_code, or exec / wait on the Codex frontend) into the execution contract, but nested subtool calls were validated against that same frozen list. Since native and MCP subtools are never in that list, every tools.read(...) / tools.edit(...) / tools.mcp__x__y(...) call was rejected. The model could see the tools and still not use them.
Now a nested call is bounded by its parent entrypoint: the parent must be in the frozen View with a matching target and mode, and the child's effect must fit inside the parent's effect. The child no longer needs its own entry in the View. Identity, workspace, subagent-depth, disabled-tool and enabled-MCP-server checks are unchanged.
Why I think it holds
- The child definition is not model-chosen: nested calls are resolved by
bindingIdfrom the cell's own binding table (src/main/tool/codeMode/runCodeRuntimeManager.ts:641), and the definition hash is journaled. The new check therefore cannot be used to reach a tool that was never offered to the sandbox. waitis safe: a resumed cell inherits the wait call's options, andwaitis a write-effect tool present in the Codex View, so subtool calls after a resume still satisfy both containment checks.- The execution catalog is re-resolved every turn (
src/main/agent/deepchat/runtime/turnCoordinator.ts:390,src/main/agent/deepchat/runtime/deepChatLoopRunner.ts:1015), so requiring the parent entrypoint to be in the current request's View still enforces per-request exposure of Code Mode itself. - CI on
a087ba19is green, and the service-level test does reproduce the original failure path rather than asserting internals.
Non-blocking notes
src/main/tape/domain/executionContract.ts:980— the error text namescurrentTooleven when the parent is the one over the ceiling. The two combined conditions share one message, so a parent-level violation reports the wrong tool. Split them or include both names.src/main/tape/domain/executionContract.ts:986— only the parent'sexecution.modeis compared with the frozen ceiling; the child's mode is now unchecked. If mode is purely a scheduling hint this is fine, but it is the one contract field that lost its child-side check. Worth one line in the comment or a test that pins the intended behavior.src/main/tool/index.ts:637—input.mode === 'agent' ? [] : ...stores dead state; leavingcodeEntryToolsundefined for agent/minimal mode is equivalent and less surface.src/main/tool/index.ts:91—MainProcessToolCallOptionsnow carriescodeModeNestedandcodeModeParentTool, andcodeModeNestedis what bypasses the Code Mode direct-call guard atsrc/main/tool/index.ts:682. Nothing imports this type today, so there is no practical exposure, but keeping those two fields on the internal runtime-callback type would stop a future caller from accidentally opting into nested dispatch.test/main/tape/executionContract.test.ts:559— oneitcovers about eleven boundaries (membership, target, effect, mode, workspace, depth, identity). A failure will not say which boundary broke; splitting it would make regressions cheaper to read.
Test coverage
The contract test covers the new parent-authority path plus the retained workspace, depth and identity checks. The service test simulates the utility-process host for both frontends and asserts that exec, read and glob reach the agent tool manager and the MCP tool reaches the MCP service, all with ok: true nested results — that is the regression that matters here. Codex wait resume and the live desktop/model path are not covered; the PR body states the latter explicitly, which is fine for this fix.
Code Mode subagents reject native and MCP subtool calls because their frozen ExecutionContract lists only provider-visible entrypoints. On the Codex frontend, native
execalso collides with the distinct Code Modeexectarget.Bind nested dispatch to the configured outer
run_code,exec, orwaitentrypoint. Validate its frozen target and execution mode, and bound the child's effect by both the current parent policy and the frozen ceiling. Keep request identity, live tool/server restrictions, workspace, and subagent-depth checks in place.Fixes #2471.
Validation:
pnpm run format:checkpnpm run i18npnpm run lintpnpm run typecheckThe integration regression uses a simulated utility-process transport. Live desktop/model end-to-end verification was not performed.
Summary by CodeRabbit