Skip to content

refactor(coderd/x/chatd): clean generation preparer setup #26511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
hugodutka wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −24
Draft

refactor(coderd/x/chatd): clean generation preparer setup #26511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions coderd/x/chatd/chatd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ const (
// It is intentionally generous relative to the advisor's concise
// guidance remit so short plans are not truncated mid-reasoning.
defaultAdvisorMaxOutputTokens = 16384

// defaultChatMaxOutputTokens caps a model response when the model config
// does not set a provider-specific output limit. This keeps unbounded
// requests from accidentally exhausting large-context models.
defaultChatMaxOutputTokens int64 = 32_000
Comment on lines +129 to +132

@coder-agents-review coder-agents-review Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit [CRF-1] The first sentence ("caps a model response when the model config does not set a provider-specific output limit") restates what the name + usage site already show: default = fallback, ChatMaxOutputTokens = caps tokens, and the callConfig.MaxOutputTokens == nil guard shows when it fires.

Downgraded from P2 because the sibling defaultAdvisorMaxOutputTokens follows the identical what-then-why pattern, so this matches local convention.

(Gon P2, downgraded to Nit by orchestrator)

🤖

Comment on lines +129 to +132

@coder-agents-review coder-agents-review Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit [CRF-3] The adjacent defaultAdvisorMaxOutputTokens comment explains its value: "intentionally generous relative to the advisor's concise guidance remit so short plans are not truncated mid-reasoning." That tells you why 16,384 and not 8,192.

This constant says what it prevents but not why 32,000. Is it half a 64k context window? A provider default? Empirically determined? The next person tuning this value has no anchor.

(Leorio)

🤖

@coder-agents-review coder-agents-review Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit [CRF-4] Commit subject "clean generation preparer setup" is vague. "clean" doesn't convey what changed. Something like "centralize preparer error cleanup via named return" or "simplify preparer setup: extract token constant, defer cleanup" tells git log --oneline readers what happened.

(Leorio)

🤖

)

var (
Expand Down
38 changes: 14 additions & 24 deletions coderd/x/chatd/generation_preparer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
func (server *Server) prepareGeneration(
ctx context.Context,
input generationPrepareInput,
) (generationPrepared, error) {
) (prepared generationPrepared, err error) {
chat := input.Chat
logger := server.logger.With(
slog.F("chat_id", chat.ID),
Expand Down Expand Up @@ -85,7 +85,6 @@ func (server *Server) prepareGeneration(
modelOpts = modelBuildOptionsFromMessages(promptRows)
ctx = withActiveTurnAPIKeyID(ctx, modelOpts)

var err error
model, modelConfig, providerKeys, modelRoute, debugEnabled, resolvedProvider, debugModel, err = server.resolveChatModel(ctx, chat, modelOpts)
if err != nil {
return generationPrepared{}, err
Expand All @@ -97,7 +96,7 @@ func (server *Server) prepareGeneration(
}

if callConfig.MaxOutputTokens == nil {
maxOutputTokens := int64(32_000)
maxOutputTokens := defaultChatMaxOutputTokens

@coder-agents-review coder-agents-review Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit [CRF-2] Go 1.26 supports new(expr) for constant expressions. This two-line address-of-local pattern:

maxOutputTokens := defaultChatMaxOutputTokens
callConfig.MaxOutputTokens = &maxOutputTokens

can be:

callConfig.MaxOutputTokens = new(defaultChatMaxOutputTokens)

Verified: new(defaultChatMaxOutputTokens) produces *int64 with value 32000 under go 1.26.4. Minor, but removes a variable that exists solely to be addressable.

(Ging-Go)

🤖

callConfig.MaxOutputTokens = &maxOutputTokens
}

Expand All @@ -121,8 +120,7 @@ func (server *Server) prepareGeneration(

var advisorRuntime *chatadvisor.Runtime
if advisorCfg.Enabled && isRootChat && !isPlanModeTurn && !isExploreSubagent {
var advisorErr error
advisorRuntime, advisorErr = server.newAdvisorRuntime(
advisorRuntime, err = server.newAdvisorRuntime(
ctx,
chat,
advisorCfg,
Expand All @@ -132,8 +130,8 @@ func (server *Server) prepareGeneration(
modelOpts,
logger,
)
if advisorErr != nil {
return generationPrepared{}, advisorErr
if err != nil {
return generationPrepared{}, err
}
}

Expand All @@ -157,9 +155,18 @@ func (server *Server) prepareGeneration(
currentChat: &currentChat,
loadChatSnapshot: loadChatSnapshot,
}
var mcpCleanup func()
cleanup := func() {
if mcpCleanup != nil {
mcpCleanup()
}
workspaceCtx.close()
}
defer func() {
if err != nil {
cleanup()
}
}()
Comment on lines +158 to +169

@coder-agents-review coder-agents-review Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note [CRF-5] Five reviewers independently noticed this restructuring incidentally fixes a latent MCP connection leak.

In the old code, cleanup() at the g2.Wait() error return (base ~line 276) was just workspaceCtx.close(). The mcpCleanup chaining into cleanup happened after that error check (base ~line 280). Since errgroup.Wait() guarantees all goroutines complete before returning, the MCP goroutine had already set mcpCleanup, but it was never called on that error path. MCP connections leaked until context cancellation.

The new code captures mcpCleanup by reference from the start, so every error path (including the g2.Wait() path) correctly tears down MCP connections. Low practical impact (triggers only when ConvertMessagesWithFiles fails with active MCP configs), but the correctness improvement is real and worth noting in the PR description.

(Hisoka, Pariston, Meruem, Mafuuu, Kite)

🤖


planPathFn := func(ctx context.Context) (string, string, error) {
conn, err := workspaceCtx.getWorkspaceConn(ctx)
Expand Down Expand Up @@ -208,7 +215,6 @@ func (server *Server) prepareGeneration(
prompt []fantasy.Message
instruction string
mcpTools []fantasy.AgentTool
mcpCleanup func()
workspaceMCPTools []fantasy.AgentTool
workspaceSkills []chattool.SkillMeta
personalSkills []skillspkg.Skill
Expand Down Expand Up @@ -273,18 +279,9 @@ func (server *Server) prepareGeneration(
})
}
if err := g2.Wait(); err != nil {
cleanup()
return generationPrepared{}, err
}

if mcpCleanup != nil {
previousCleanup := cleanup
cleanup = func() {
mcpCleanup()
previousCleanup()
}
}

prompt, sanitizeStats := chatsanitize.SanitizeAnthropicProviderToolHistory(model.Provider(), prompt)
chatsanitize.LogAnthropicProviderToolSanitization(
ctx,
Expand Down Expand Up @@ -427,7 +424,6 @@ func (server *Server) prepareGeneration(

tools, dynamicToolNames, err := appendDynamicTools(ctx, logger, tools, chat.DynamicTools, currentPlanMode, chat.Mode)
if err != nil {
cleanup()
return generationPrepared{}, err
}

Expand All @@ -449,12 +445,10 @@ func (server *Server) prepareGeneration(
if isComputerUse {
computerUseProvider, computerUseModelProvider, computerUseModelName, err := server.computerUseProviderAndModelFromConfig(ctx)
if err != nil {
cleanup()
return generationPrepared{}, xerrors.Errorf("resolve computer use provider and model: %w", err)
}
computerUseRoute, keyErr := server.resolveModelRouteForProviderType(ctx, chat.OwnerID, computerUseModelProvider)
if keyErr != nil {
cleanup()
return generationPrepared{}, xerrors.Errorf("resolve computer use provider route: %w", keyErr)
}
modelRoute = computerUseRoute
Expand All @@ -469,7 +463,6 @@ func (server *Server) prepareGeneration(
modelOpts,
)
if cuErr != nil {
cleanup()
return generationPrepared{}, cuErr
}
model = cuModel
Expand All @@ -486,7 +479,6 @@ func (server *Server) prepareGeneration(
logger: server.logger.Named("computer_use"),
})
if err != nil {
cleanup()
return generationPrepared{}, xerrors.Errorf("register computer use provider tool for provider %q: %w", computerUseProvider, err)
}
} else {
Expand All @@ -495,7 +487,6 @@ func (server *Server) prepareGeneration(
isComputerUse: false,
})
if err != nil {
cleanup()
return generationPrepared{}, err
}
}
Expand Down Expand Up @@ -529,7 +520,6 @@ func (server *Server) prepareGeneration(
var debug *generationDebug
if debugEnabled {
if debugSvc == nil {
cleanup()
return generationPrepared{}, xerrors.New("chat debug service missing after enablement check")
}
debug = &generationDebug{
Expand Down
Loading