refactor(coderd/x/chatd): unify LLM model-call construction behind a resolver - #28145
refactor(coderd/x/chatd): unify LLM model-call construction behind a resolver#28145ibetitsmike wants to merge 12 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ 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". |
…d turn Add modelCallSpec/resolveModelCall as the single pipeline from a call spec to a ready model client: config selection, call-config parse, route and identity resolution, client construction with debug recording, and provider-option derivation. Migrate prepareGeneration and the computer-use substitution onto the resolver, delete resolveComputerUseModel, and turn resolveChatModel into a transitional wrapper for the remaining quickgen callers.
…-label flows to resolver Title (auto, manual, override), whole-chat summary, and turn-status label flows now resolve their model through resolveModelCall with purpose-specific specs. fantasy.ObjectCall envelopes come from resolvedModelCall.newObjectCall. Deletes titleGenerationProviderOptions, newQuickgenDebugModel, and the transitional resolveChatModel wrapper. Summary and status-label calls keep their historical provider-option omission as declared policy.
…le debug paths to resolver Compaction overrides build through compactionOverrideSpec, advisor overrides through advisorOverrideSpec with a typed modelCallConfigParseError preserving the soft fallback on corrupt options JSON, and manual-title debug rebuilds through manualTitleDebugSpec. Deletes newDebugAwareModel, buildCompactionOverrideModel, and compactionOverrideProviderOptions.
… templates resolvedModelCall.newCall is now the only production fantasy.Call construction point. chatloop.GenerateAssistantOptions carries a CallTemplate instead of ModelConfig plus ProviderOptions, compaction options carry a prebuilt SummaryCall with tool use forbidden, and chatadvisor.RuntimeConfig carries a CallTemplate that runs clone before mutating provider options. The chat-model compaction summary keeps its historical provider-option omission via compactionSummaryOverrides.
Comment audit: delete restating comments, shorten verbose ones, and fix the advisor fallback description. Simplify findings: parse model config options once in resolveModelCall and pass the typed CallConfig to newModel, and stop grafting the chat config onto the computer-use resolution. Deslop findings: rename runChatResult.StatusLabel to StatusLabelCall and lock the resolver's provider-option policy with TestModelCallShapeProviderOptionPolicy instead of a misleading fixture.
Replace the providerOptionPolicy enum with an omitProviderOptions bool, replace callOverrides and compactionSummaryOverrides with newCall and newCompactionSummaryCall, pass newObjectCall arguments directly, and turn callPurpose into a plain log string. No behavior change.
…ners Fold standard-turn and compaction-summary template assertions into the reasoning-effort clamp test, manual-title provider-option assertions into the synthetic API key test, and status-label envelope assertions into the structured status-label test. Replace the end-to-end summary fixture with a focused resolver-level omission test that also covers summaryObjectCall. Delete modelcall_shape_internal_test.go.
… data Cleanup-gate follow-up: replace the newCompactionSummaryCall method plus caller-side ProviderOptions mutation with a compactionSummaryCall free function that takes the provider options directly.
…spec options Delete the purpose-specific spec constructors and the per-flow policy flags (omitProviderOptions, debug tri-state, defaultMaxOutputTokens, routeOverride). Every resolved call now derives provider options, defaults MaxOutputTokens, and wraps for debug recording when chat debug is enabled; clients are always constructed with the configured model string so gateway validation sees the configured name. Debug helpers only create and finalize run records instead of rebuilding models. Callers describe calls with inline modelCallSpec literals whose fields are plain data: model source, requested effort, and chatd-scoped routing.
ff9830d to
4a5f5f0
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ 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". |
Centralizes every LLM client build in chatd behind one resolver and makes all model calls behave the same, instead of preserving per-flow quirks.
Why
chatdbuilt LLM clients in five places (standard turn, compaction override, quickgen, title override, advisor), each repeating config parsing, route resolution, client construction, and debug wrapping with slightly different behavior. An earlier iteration of this PR centralized construction but preserved every historical quirk behind purpose-specific spec constructors and policy flags. Review concluded the quirks themselves were the problem.What changed
Server.resolveModelCall(modelcall.go) is the single pipeline from amodelCallSpecto a ready client plus call metadata. Callers build spec literals inline; the only spec fields are data: model source (chat config, explicit config row, or fixed provider/model pair for computer use), requested reasoning effort, chatd-scoped routing, and build options.MaxOutputTokensalways defaults to 32k when the config leaves it unset. The compaction summary call nulls the cap after copying the template because it is non-streaming and the Anthropic SDK rejects non-streaming requests whose cap implies a completion over ten minutes.manualTitleDebugSpec,quickgenDebugSpec) are deleted; debug helpers now only create and finalize run records, since run association flows through the generate context, not the model.resolvedModelCall.newCall/newObjectCallare the only production constructors offantasy.Call/fantasy.ObjectCall; object calls now carry the config's sampling parameters.Validation
./coderd/x/chatd/...suite green (PostgreSQL-backed).