feat: allow approved external MCP tools in root plan mode#24509
feat: allow approved external MCP tools in root plan mode#24509ibetitsmike merged 6 commits intomainfrom
Conversation
…mode Rebase merge note: resolved chatd conflicts against origin/main and renumbered the allow_in_plan_mode migration to 000472 to avoid the new 000471 chat explore mode migration on main.
|
@codex review |
![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: c6f0d7c1b8
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Well-structured change. The three-layer defense-in-depth design (config pre-filter, tool post-filter, active-tool-name filter) is genuinely solid, and the security reviewers confirmed every boundary holds. The MCPToolIdentifier interface pattern structurally excludes workspace MCP tools without relying on name conventions. Test coverage is proportional: 780 lines of tests for 230 lines of production code, covering visibility, invocation, workflow completion, and regressions.
One P1 blocks merge (DEREM-3, below). Two P3s worth addressing. Four nits, all optional.
P1 [DEREM-3] coderd/database/migrations/testdata/fixtures/000447_mcp_server_configs.up.sql:10 -- The 447 fixture now references allow_in_plan_mode, but that column is introduced in migration 472. When TestMigrateUpWithFixtures replays migrations with fixtures, this INSERT runs at version 447 against a schema that lacks the column, producing a "column allow_in_plan_mode does not exist" error. This is likely the root cause of the CI failures (test-go-pg, test-go-pg-17, test-go-race-pg). Fix: remove allow_in_plan_mode and its value from this fixture. If fixture coverage for the new column is needed, add a separate 000472_mcp_server_allow_in_plan_mode.up.sql fixture that ALTERs the existing row.
"Two functions that both claim to resolve 'which tools are active in plan mode' now produce different results for the same inputs." -- Razor
Severity counts: 1 P1, 2 P3, 4 Nit.
coderd/x/chatd/chatd.go:4551
P3 [DEREM-4] allowedPlanToolNames is dead code on every production path after this PR. appendDynamicTools (line 4974) returns early for plan mode before reaching allowedBehaviorToolNames, so this function is never called at runtime. The live plan-mode gate is now activeToolNamesForTurn, which includes approved MCP tools via toolAllowedForTurn.
The two functions answer the same question ("which tools are active in plan mode?") but give different answers: allowedPlanToolNames returns builtins only, activeToolNamesForTurn returns builtins + approved MCP. They share builtinPlanToolAllowed, so the builtin allowlist can't drift, but a future editor re-introducing a call to allowedBehaviorToolNames in plan mode would silently exclude MCP tools. TestAllowedPlanToolNames would pass while production diverges.
Could we remove allowedPlanToolNames and simplify allowedBehaviorToolNames to handle only the explore-subagent dispatch? If unit coverage of the builtin switch is desired, test builtinPlanToolAllowed directly. (Meruem P3, Hisoka P3, Mafuuu P3, Ryosuke P3, Chopper P3, Gon P3, Razor P3, Knov P4, Pariston Note, Kite Note)
🤖
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
6 of 7 findings from R1 were addressed. Nice work on the fixture fix, the unit test for filterExternalMCPConfigsForTurn, and all four nits.
Further review is blocked until the following finding is addressed or acknowledged:
DEREM-4 (P3) chatd.go:4551 -- allowedPlanToolNames is dead code on every production path. appendDynamicTools returns early for plan mode before reaching allowedBehaviorToolNames, so the plan-mode branch is never executed. The live gate is activeToolNamesForTurn. The two functions answer "which tools are active in plan mode?" with different answers: builtins-only vs builtins + approved MCP. 10 reviewers flagged this independently.
Options: remove the dead function, add a comment explaining why it's kept, or push back if you disagree with the finding.
🤖 This review was automatically generated with Coder Agents.
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Addressed DEREM-4 on
|
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
All 8 findings from R1 are resolved. The DEREM-4 fix (removing dead allowedPlanToolNames, narrowing allowedBehaviorToolNames) is clean: no orphaned references, tests updated, comments accurate.
R3 panel (Mafuuu, Meruem, Bisky, Kite, Razor) found no new issues. Razor verified 8 PR claims against the implementation; all hold. Bisky confirmed the test suite is thorough across unit, integration, and e2e layers. Kite verified the four-layer trust boundary and ran the full test set. The defense-in-depth architecture is intact.
CI is pending (21 passed, 2 pending at setup). No findings.
"One function, two enforcement layers, no drift." -- Kite
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
|
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
Re-reviewed after merge of main. Netero and panel (Mafuuu, Kite, Meruem) found no new issues. All 8 R1 findings remain resolved. The merge introduced no conflicts in the PR's code; the defense-in-depth architecture is intact.
CI is pending (11 passed, 11 pending at setup). No findings across 4 rounds.
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
No changes since R4 (same head SHA a3fe03b). All 8 R1 findings remain resolved. Panels in R3 and R4 found 0 new issues on this exact diff. CI progressing (21 passed, 2 pending). Prior approval stands; nothing to re-review.
🤖 This review was automatically generated with Coder Agents.
Ran the post-idle PR check on head |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
R6 re-review on unchanged code (same head SHA since R4). Netero and panel (Mafuuu, Kite, Chopper) found 0 new issues. All 8 R1 findings remain resolved. Four consecutive clean panel rounds (R3, R4, R5, R6). CI steady (21 passed, 2 pending).
This PR is ready for human merge.
🤖 This review was automatically generated with Coder Agents.
|
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
R7: unchanged head (a3fe03b). Netero + panel (Mafuuu, Meruem, Kite) found 0 issues. Five consecutive clean panel rounds. All 8 R1 findings resolved. CI steady. Ready for human merge.
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
R8: unchanged head (a3fe03b). Netero + panel (Mafuuu, Kite, Razor) found 0 issues. Six consecutive clean panel rounds. All 8 R1 findings resolved. CI steady. Ready for human merge.
🤖 This review was automatically generated with Coder Agents.
3 participants
Summary
Allow root plan-mode chats to use MCP tools from external servers that an admin has explicitly approved for plan mode. Workspace MCP and plan-mode subagents remain blocked.
Problem
chatd.goexcluded every MCP tool whenisPlanModeTurnwas true, so planning had no access to tools like docs search, ticketing, etc. Lifting that guard wholesale was unsafe:mcp_server_configsalready has centralized admin governance, but workspace-local MCP (discovered from agent.mcp.json) does not, and subagents use a narrower trust boundary.Fix
Add an admin-controlled per-server
allow_in_plan_modeflag (defaultfalse) and gate plan-mode MCP access on it.Backend / schema
000472_mcp_server_allow_in_plan_mode.{up,down}.sqland matching fixture update.mcpserverconfigs.sql+ generated code: persist and read the new column.codersdk/mcp.go: thread the field throughMCPServerConfig,Create*, andUpdate*request types.coderd/mcp.go: validate, persist, and return the flag in get/list/create/update handlers.chatd
coderd/x/chatd/chatd.go: pre-filter selected external MCP configs byAllowInPlanModebefore callingmcpclient.ConnectAllon plan-mode root turns. Workspace MCP discovery is skipped entirely on plan-mode turns.coderd/x/chatd/prompt.go: update the root plan-mode overlay text to match the new boundary.UI
MCPServerAdminPanel.tsx: add an explicit toggle ("Allow all tools from this MCP server in root plan mode") next to the existing governance controls.site/src/api/typesGenerated.ts.Docs
docs/ai-coder/agents/architecture.md: replace the blanket "MCP is unavailable in plan mode" note with the new root-only, external-only, admin-approved policy. Explicitly call out that workspace MCP and plan-mode subagents are still excluded.Tests
propose_plan.Policy precedence
allow_in_plan_modeis an additional requirement on top of existingenabled, availability, chat-selected / forced server IDs, and tool allow/deny lists. It approves all tools on that server for root plan mode; a per-tool plan allowlist is deliberately deferred.Follow-ups (explicitly out of scope)
.mcp.jsonschema vs. a coderd-managed workspace MCP config).Validation
go vet ./coderd/x/chatd/...go test ./coderd/x/chatd -run 'TestPlan.*|TestMCP.*' -count=1go test ./coderd/x/chatd -count=1 -timeout 5m(full chatd suite)make fmt(no diff)