Repository navigation
test(coderd/x/chatd): remove end-to-end tool policy tests - #30868
Merged
ibetitsmike merged 1 commit intoOct 16, 2026
Merged
Conversation
ibetitsmike
added this pull request to stack #30871
October 15, 2026 17:42
This was referenced Oct 15, 2026
ibetitsmike
marked this pull request as ready for review
October 15, 2026 17:43
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ibetitsmike
added a commit
that referenced
this pull request
Oct 16, 2026
An audit of every check in the seven tests that #30868 removes found four regressions that only those tests caught: - a plan-mode root chat does not get spawn_agent - a general sub-agent of a plan-mode chat loses the plan mode - POST /chats drops the requested plan mode - Server.CreateChat drops the MCP selection of a root Explore chat PlanRoot now requires spawn_agent and runs it to check the child's plan mode. TestCreateExploreChatPreservesMCPSelection checks the stored selection, and TestPostChats/Success creates a plan-mode chat.
hugodutka
approved these changes
Oct 16, 2026
ibetitsmike
added a commit
that referenced
this pull request
Oct 16, 2026
…ilter (#30864) Removes `allowedBehaviorToolNames`, `allToolNames`, and `TestAllowedBehaviorToolNames`, and the active-name check they fed in `appendDynamicTools`. The check never skipped a tool. `appendDynamicTools` returns early for Explore mode and plan mode, so at the check, `allowedBehaviorToolNames` always returned every tool name. A dynamic tool that shares a built-in tool's name is still dropped with a warning. Validation: `go test ./coderd/x/chatd/ -run 'Dynamic|AllowedExplore|StopAfterPlan'` passes, and the pre-commit hook (build, `lint/go`, `lint/go/new-code`) passed. ## Stack This stack splits #28757, as requested in review. It moves the code that prepares a chat turn into one file, `turn_environment.go`, and moves the tool policy tests from full turns to a table test. chatd behaves as on `main`, except for the debug log in #30865. 1. #30864 Remove the no-op `allowedBehaviorToolNames` filter 2. #30865 Log workspace agent lookup errors in turn preparation 3. #30866 Rename `prepareGeneration` to `buildTurnEnvironment` 4. #30867 Cover the turn tool policy at `buildTurnEnvironment` 5. #30868 Remove seven end-to-end tool policy tests 6. #30869 Name the `*Server` receiver `p` in `turn_environment.go` 7. #30870 Move the turn preparation helpers into `turn_environment.go` > Xum, an AI agent, opened this PR for @ibetitsmike and wrote this description.
ibetitsmike
force-pushed
the
mike/chatd-turn-env-split/remove-e2e-tool-policy-tests
branch
from
October 16, 2026 20:44
a641bf0 to
90eb38f
Compare
ibetitsmike
added a commit
that referenced
this pull request
Oct 16, 2026
…aration (#30865) Logs the error from `getWorkspaceAgent` at debug level in turn preparation. `prepareGeneration` discarded it. The turn still goes on without the agent, as before. The agent only adds its OS and working directory to the instruction header. Validation: build and vet pass, and the pre-commit hook passed. ## Stack This stack splits #28757, as requested in review. It moves the code that prepares a chat turn into one file, `turn_environment.go`, and moves the tool policy tests from full turns to a table test. chatd behaves as on `main`, except for the debug log in #30865. 1. #30864 Remove the no-op `allowedBehaviorToolNames` filter 2. #30865 Log workspace agent lookup errors in turn preparation 3. #30866 Rename `prepareGeneration` to `buildTurnEnvironment` 4. #30867 Cover the turn tool policy at `buildTurnEnvironment` 5. #30868 Remove seven end-to-end tool policy tests 6. #30869 Name the `*Server` receiver `p` in `turn_environment.go` 7. #30870 Move the turn preparation helpers into `turn_environment.go` > Xum, an AI agent, opened this PR for @ibetitsmike and wrote this description.
ibetitsmike
force-pushed
the
mike/chatd-turn-env-split/remove-e2e-tool-policy-tests
branch
from
October 16, 2026 20:59
90eb38f to
f58997e
Compare
ibetitsmike
added a commit
that referenced
this pull request
Oct 16, 2026
Renames the turn preparation step after what it returns: the environment for one turn. Only names change, plus one small change to how callers release the environment. - `generation_preparer.go` becomes `turn_environment.go`, and its test file becomes `turn_environment_internal_test.go`. GitHub shows both as renames. A comment in `compactionTriggers.test.ts` that names the test file follows the rename. - `prepareGeneration` becomes `buildTurnEnvironment`, and its result type `generationPrepared` becomes `turnEnvironment`. Four `TestPrepareGeneration*` tests become `TestBuildTurnEnvironment*`. - The `Cleanup` field becomes the unexported `cleanup`, and callers use `Close()`. `Close()` does nothing on a zero value. The type stays in `generation.go` for now. #30870 moves it next to `buildTurnEnvironment`. Validation: build and vet pass, the renamed tests and the `toolinput` tests pass, and the pre-commit hook passed. ## Stack This stack splits #28757, as requested in review. It moves the code that prepares a chat turn into one file, `turn_environment.go`, and moves the tool policy tests from full turns to a table test. chatd behaves as on `main`, except for the debug log in #30865. 1. #30864 Remove the no-op `allowedBehaviorToolNames` filter 2. #30865 Log workspace agent lookup errors in turn preparation 3. #30866 Rename `prepareGeneration` to `buildTurnEnvironment` 4. #30867 Cover the turn tool policy at `buildTurnEnvironment` 5. #30868 Remove seven end-to-end tool policy tests 6. #30869 Name the `*Server` receiver `p` in `turn_environment.go` 7. #30870 Move the turn preparation helpers into `turn_environment.go` > Xum, an AI agent, opened this PR for @ibetitsmike and wrote this description.
ibetitsmike
force-pushed
the
mike/chatd-turn-env-split/remove-e2e-tool-policy-tests
branch
from
October 16, 2026 21:20
f58997e to
5e08d7d
Compare
Base automatically changed from
mike/chatd-turn-env-split/tool-policy-table-test
to
main
October 16, 2026 21:34
TestBuildTurnEnvironmentToolPolicy checks the plan mode and Explore mode tool policy at buildTurnEnvironment. These seven tests checked the same policy by running a full turn against a fake model server, each with its own fixture.
ibetitsmike
added a commit
that referenced
this pull request
Oct 16, 2026
) Adds `TestBuildTurnEnvironmentToolPolicy`, a table test of the tool policy at the place chatd applies it: the result of `buildTurnEnvironment`. #30868 then removes seven end-to-end tests that checked the same policy through a full turn. The six cases (`Root`, `PlanRoot`, `PlanChild`, `ExploreRoot`, `ExploreChild`, `ExploreChildAfterMCPReplacement`) share one fixture: - three MCP servers that every chat selects: approved for plan mode, not approved, and approved with a tool allowlist - two MCP servers that no chat selects: one Force On and one ordinary - one inline MCP server on each root chat, one without `allow_in_subagents` on the parent of the child chats, and one workspace MCP server - a model with the `web_search` provider tool Each case builds the tool definitions with `chatloop.BuildToolDefinitions`, the same function the chat loop uses for the model request. It then compares the tool names, active tools, and provider tools with the case. It also searches the full system prompt for the plan mode and Explore mode overlays and for the root-only `propose_plan` instruction. `ExploreChildAfterMCPReplacement` first calls `SendMessage` with a request to replace the Explore child's MCP servers with the unselected one. The test checks that the stored selection does not change, then builds the next turn from the stored chat and messages. `PlanRoot` runs its `spawn_agent` tool and checks that the general sub-agent keeps the plan mode. The PR also adds: - `TestCreateExploreChatPreservesMCPSelection`: `Server.CreateChat` stores the MCP servers that a root Explore chat selects - a plan-mode request in `TestPostChats/Success`: `POST /chats` stores the requested plan mode - `TestBuildTurnEnvironmentSplitsPendingUserRowsFromCompactionPrompt` and a project instructions assertion in `TestBuildTurnEnvironmentMemory` ## Validation - For each new assertion, Xum removed the code that the assertion checks and saw the test fail. On this branch, for example, removing the Explore overlay in `chatd.go` fails `ExploreRoot` and `ExploreChild`. - In a mutation run, these tests catch 7 mutants (copies of the code with one policy branch removed) that no test on `main` caught. Two examples: root Explore chats get MCP tools, and Explore chats get workspace MCP tools. That run used an earlier `main` and the #28757 branch, which has the same test code. - Review found regressions that only the end-to-end tests removed in #30868 caught. Xum applied each one to this branch: the first version of these tests passes, and the current version fails in the cases below. - Two independent audits then listed every check in the seven removed tests, including implicit ones such as a scripted `spawn_agent` call, and tried a mutant for each check without an obvious replacement. A mutant counts as a gap only if a removed test fails with it and the rest of `coderd/x/chatd/...` plus the chat, MCP, Explore, and plan tests in `coderd` and `enterprise/coderd` pass. The audits found the last two rows. They also found one gap that this PR leaves open; #30868 explains why. - A line coverage comparison found 4 statements in `coderd/x/chatd/...` that only the seven removed tests run. All 4 are error returns. | Regression | Failing cases | |---|---| | Explore chats get `edit_files` | `ExploreRoot`, `ExploreChild` | | Explore children keep only the MCP servers approved for plan mode | `ExploreChild`, `ExploreChildAfterMCPReplacement` | | `SendMessage` replaces an Explore child's MCP servers | `ExploreChildAfterMCPReplacement` | | Chats outside Explore mode get every enabled MCP server in the organization | `Root` | | A plan-mode child gets the root-only `propose_plan` instruction | `PlanChild` | | A plan-mode root chat does not get `spawn_agent` | `PlanRoot` | | `Server.CreateChat` drops the MCP servers of a root Explore chat | `TestCreateExploreChatPreservesMCPSelection` | | A general sub-agent of a plan-mode chat loses the plan mode | `PlanRoot` | | `POST /chats` drops the requested plan mode | `TestPostChats/Success` | ## Stack This stack splits #28757, as requested in review. It moves the code that prepares a chat turn into one file, `turn_environment.go`, and moves the tool policy tests from full turns to a table test. chatd behaves as on `main`, except for the debug log in #30865. 1. #30864 Remove the no-op `allowedBehaviorToolNames` filter 2. #30865 Log workspace agent lookup errors in turn preparation 3. #30866 Rename `prepareGeneration` to `buildTurnEnvironment` 4. #30867 Cover the turn tool policy at `buildTurnEnvironment` 5. #30868 Remove seven end-to-end tool policy tests 6. #30869 Name the `*Server` receiver `p` in `turn_environment.go` 7. #30870 Move the turn preparation helpers into `turn_environment.go` > Xum, an AI agent, opened this PR for @ibetitsmike and wrote this description.
ibetitsmike
force-pushed
the
mike/chatd-turn-env-split/remove-e2e-tool-policy-tests
branch
from
October 16, 2026 21:35
5e08d7d to
5e71364
Compare
ibetitsmike
deleted the
mike/chatd-turn-env-split/remove-e2e-tool-policy-tests
branch
October 16, 2026 21:47
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Removes seven end-to-end tool policy tests from
chatd_test.go(949 lines). The table test from #30867 checks the same policy atbuildTurnEnvironment.TestPlanModeSubagentChatExcludesAskUserQuestionTestPlanModeRootChatAllowsApprovedExternalMCPToolsTestExploreSubagentIsReadOnlyTestExploreChatUsesPersistedMCPSnapshotTestExploreChatSendMessageCannotMutateMCPSnapshotTestRootExploreChatStaysBuiltinOnlyAtRuntimeTestRootExploreChatExcludesWebSearchProviderToolAtRuntimeEach test started chatd with a fake model server, ran a turn, and read the tool names from the model request. Each had its own fixture.
Why removal is safe: a test was removed only if other tests still catch every mutant it caught. On
main, 9 mutants were caught only by these tests, and the table test catches all 9. Review and two audits of every check in these tests then found nine more regressions that only these tests caught, and #30867 now covers each one. That includes theTestExploreChatSendMessageCannotMutateMCPSnapshotscenario: theExploreChildAfterMCPReplacementcase callsSendMessageon an Explore child chat, checks the stored MCP servers, and builds the next turn.TestEditMessage_MCPServerIDsstill covers the same guard throughEditMessage. The first mutation run used an earliermainand the #28757 branch, which removed the same lines.Tradeoff: the table test stops at the result of
buildTurnEnvironment. No test now checks plan mode and Explore tool lists in the model request itself. The audits found one mutant that only these tests catch: a branch ingeneration.gothat clears the active tools of Explore chats just before the model call. The same change without the Explore condition failsTestMCPToolSearchGenerationFlows, because the step frombuildTurnEnvironmentto the model request is the same for every mode. Only an end-to-end test like the ones this PR removes would catch the Explore-only form, so this PR does not add one.Validation: build and vet pass, and the pre-commit hook passed.
Stack
This stack splits #28757, as requested in review. It moves the code that prepares a chat turn into one file,
turn_environment.go, and moves the tool policy tests from full turns to a table test. chatd behaves as onmain, except for the debug log in #30865.allowedBehaviorToolNamesfilterprepareGenerationtobuildTurnEnvironmentbuildTurnEnvironment*Serverreceiverpinturn_environment.goturn_environment.go