Skip to content

refactor(coderd/x/chatd): move turn preparation helpers into turn_environment.go - #30870

Merged
ibetitsmike merged 1 commit into
mainfrom
mike/chatd-turn-env-split/move-turn-helpers
Oct 16, 2026
Merged

ibetitsmike merged 1 commit into
mainfrom
mike/chatd-turn-env-split/move-turn-helpers

Conversation

@ibetitsmike

@ibetitsmike ibetitsmike commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Moves the helpers that buildTurnEnvironment uses from chatd.go, and the turnEnvironment type from generation.go, into turn_environment.go. The code moves without edits. Only the import blocks change.

The moved helpers build the turn's workspace context, tool set, system prompt, and skills. chatd.go goes from 4,396 to 3,131 lines, and turn_environment.go from 1,187 to 2,520.

To check that only imports changed, compare the removed lines with the added lines:

B=mike/chatd-turn-env-split/receiver-name
H=mike/chatd-turn-env-split/move-turn-helpers
git fetch origin "$B" "$H"
diff <(git diff "origin/$B" "origin/$H" | grep '^-[^-]' | cut -c2- | sort) \
  <(git diff "origin/$B" "origin/$H" | grep '^+[^+]' | cut -c2- | sort)

The output lists only import lines. git diff --color-moved also marks the blocks as moved.

After this PR, the stack has the same code as #28757 (head 6ee4ecb35f8, on current main), except that turn_environment.go names the *Server receiver p (#30869).

Validation: go build ./coderd/... ./enterprise/... and go vet ./coderd/x/chatd/... pass, and the pre-commit hook passed. go test ./coderd/x/chatd/... ./enterprise/coderd/x/chatd/... -count=1 passed in all packages except one known load flake, TestChatTurnStopsAtStepLimit/Default (timeout while seeding 1,199 steps). A rerun of ./coderd/x/chatd 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. refactor(coderd/x/chatd): remove the no-op allowedBehaviorToolNames filter #30864 Remove the no-op allowedBehaviorToolNames filter
  2. chore(coderd/x/chatd): log workspace agent lookup errors in turn preparation #30865 Log workspace agent lookup errors in turn preparation
  3. refactor: rename prepareGeneration to buildTurnEnvironment #30866 Rename prepareGeneration to buildTurnEnvironment
  4. test(coderd): cover the turn tool policy at buildTurnEnvironment #30867 Cover the turn tool policy at buildTurnEnvironment
  5. test(coderd/x/chatd): remove end-to-end tool policy tests #30868 Remove seven end-to-end tool policy tests
  6. refactor(coderd/x/chatd): name the Server receiver p in turn_environment.go #30869 Name the *Server receiver p in turn_environment.go
  7. refactor(coderd/x/chatd): move turn preparation helpers into turn_environment.go #30870 Move the turn preparation helpers into turn_environment.go

Xum, an AI agent, opened this PR for @ibetitsmike and wrote this description.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T17:45:24.678104Z e5001d5 Draft marked ready
ℹ️ 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" or "@codex security review".

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
…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
ibetitsmike force-pushed the mike/chatd-turn-env-split/move-turn-helpers branch from da5c0d8 to 961f627 Compare October 16, 2026 20:44
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
ibetitsmike force-pushed the mike/chatd-turn-env-split/move-turn-helpers branch from 961f627 to fb0918c Compare October 16, 2026 20:59
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
ibetitsmike force-pushed the mike/chatd-turn-env-split/move-turn-helpers branch from fb0918c to 85de9d1 Compare October 16, 2026 21:20
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
ibetitsmike force-pushed the mike/chatd-turn-env-split/move-turn-helpers branch from 85de9d1 to 334a006 Compare October 16, 2026 21:35
ibetitsmike added a commit that referenced this pull request Oct 16, 2026
Removes seven end-to-end tool policy tests from `chatd_test.go` (949
lines). The table test from #30867 checks the same policy at
`buildTurnEnvironment`.

- `TestPlanModeSubagentChatExcludesAskUserQuestion`
- `TestPlanModeRootChatAllowsApprovedExternalMCPTools`
- `TestExploreSubagentIsReadOnly`
- `TestExploreChatUsesPersistedMCPSnapshot`
- `TestExploreChatSendMessageCannotMutateMCPSnapshot`
- `TestRootExploreChatStaysBuiltinOnlyAtRuntime`
- `TestRootExploreChatExcludesWebSearchProviderToolAtRuntime`

Each 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 the
`TestExploreChatSendMessageCannotMutateMCPSnapshot` scenario: the
`ExploreChildAfterMCPReplacement` case calls `SendMessage` on an Explore
child chat, checks the stored MCP servers, and builds the next turn.
`TestEditMessage_MCPServerIDs` still covers the same guard through
`EditMessage`. The first mutation run used an earlier `main` and 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 in `generation.go` that clears the active
tools of Explore chats just before the model call. The same change
without the Explore condition fails `TestMCPToolSearchGenerationFlows`,
because the step from `buildTurnEnvironment` to 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 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
ibetitsmike force-pushed the mike/chatd-turn-env-split/move-turn-helpers branch from 334a006 to a9d35cb Compare October 16, 2026 21:47
Base automatically changed from mike/chatd-turn-env-split/receiver-name to main October 16, 2026 21:58
ibetitsmike added a commit that referenced this pull request Oct 16, 2026
…ent.go (#30869)

Renames the `*Server` receiver in `turn_environment.go` from `server` to
`p`. Nothing else changes: each changed line differs only in that name
(`git diff --word-diff` shows it).

#30870 moves helpers from `chatd.go` into this file. Those helpers name
the receiver `p`, as do 22 files in the package, and revive's
`receiver-naming` rule allows one receiver name per type in a file. With
this rename first, the move needs no edits.

Validation: build and vet pass, the `TestBuildTurnEnvironment*` tests
pass, and the pre-commit hook (`lint/go` with revive) 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.
…ironment.go

Move the helpers that buildTurnEnvironment uses (turn workspace context,
tool policy, system prompt, and skill loaders) from chatd.go, and the
turnEnvironment type from generation.go, into turn_environment.go. The
code moves without edits; only the import blocks change.
@ibetitsmike
ibetitsmike force-pushed the mike/chatd-turn-env-split/move-turn-helpers branch from a9d35cb to a4358f3 Compare October 16, 2026 21:58
@ibetitsmike
ibetitsmike merged commit 54ba766 into main Oct 16, 2026
29 checks passed
@ibetitsmike
ibetitsmike deleted the mike/chatd-turn-env-split/move-turn-helpers branch October 16, 2026 22:08
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants