feat(gastown): add per-role model configuration for agent roles#1541
Merged
feat(gastown): add per-role model configuration for agent roles#1541
Conversation
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)No new issues found in the incremental diff. Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (2 files)
Reviewed by gpt-5.4-20260305 · 438,275 tokens |
…outer Add optional role_models field with per-role (mayor, refinery, polecat) model overrides to TownConfigSchema, TownConfigUpdateSchema, and the admin router's TownConfigRecord mirror. Backward compatible — existing towns without role_models continue to parse correctly. Refs: #1512
Check role_models[role] before falling back to default_model. Priority: role_models[role] → default_model → hardcoded fallback. Rename _role to role now that the parameter is used.
…notation Replace unused _role parameter with active role-based lookup using Record<string, string | undefined> type annotation instead of 'as' cast. This is functionally identical but satisfies the coding style rule against TypeScript 'as' operator.
Update updateTownConfig to compare the mayor's effective model (resolved via role_models.mayor → default_model → fallback) instead of only comparing default_model. This ensures mayor session restarts when role_models.mayor is added, changed, or removed.
…o town settings - Add per-role model overrides (mayor, refinery, polecat) in accordion UI - Each role selector has 'Use default' placeholder and clear (X) button - Save logic sends role_models with empty strings as undefined (fallback) - Page reloads when the mayor's effective model changes - Replace max polecats input with slider (1-50, 100% width) - Update tRPC type declarations with role_models field
- Update max_polecats_per_rig Zod validation from .max(20) to .max(50) in both TownConfigSchema and TownConfigUpdateSchema to match the UI slider range (1-50) per the feature spec - Regenerate packages/trpc/dist/index.d.ts from feature branch source, removing previously included unrelated changes (Discord, billing promo, etc.) that were artifacts of a stale build
Cover the full resolution priority chain (role override → default_model → hardcoded fallback), all eight specified test cases, and backward compatibility with legacy TownConfig objects that lack role_models.
58fdd0c to
d9244a8
Compare
updateAgentModel was calling ensureSDKServer with an empty env dict, so the gastown plugin could not identify itself as a mayor agent and registered zero tools. Reconstruct the required GASTOWN_* env vars from the ManagedAgent record so the plugin initializes identically to the initial dispatch. Also fix accordion chevron direction: use ChevronRight rotating to 90° (down) on open, instead of ChevronDown rotating to 180° (up).
Instead of manually reconstructing a subset of env vars for the gastown plugin, store the complete buildAgentEnv dict on ManagedAgent at initial dispatch and replay it during model hot-swap. This preserves GIT_AUTHOR_*, GIT_COMMITTER_*, KILOCODE_TOKEN, GH_TOKEN, and all other env vars the SDK server needs. KILO_CONFIG_CONTENT and OPENCODE_CONFIG_CONTENT are excluded from the replay since updateAgentModel already rebuilds them with the new model.
GASTOWN_CONTAINER_TOKEN rotates via /refresh-token after initial dispatch. Prefer the current process.env value over the stale startupEnv snapshot when building the hot-swap env dict.
Reconstruct conversation history from AgentDO events during model hot-swap, using the same mechanism as container restarts (PR #1494). The TownDO reconstructs the transcript and passes it through the PATCH /agents/:id/model endpoint to the container, where it is prepended to the startup prompt so the mayor retains context.
…hot-swap syncConfigToContainer updates process.env when settings change, but the model hot-swap was replaying the stale startupEnv snapshot. Now prefer the live process.env for all vars that syncConfigToContainer can update at runtime (GIT_TOKEN, GITHUB_CLI_PAT, GITLAB_TOKEN, git identity vars, etc.). Also re-derive GH_TOKEN from the live GITHUB_CLI_PAT > GIT_TOKEN > GITHUB_TOKEN priority chain, matching buildAgentEnv's logic. This fixes gh CLI auth loss after a model change when the user has a GitHub CLI PAT configured.
The UI was omitting empty fields from the config update, so the server merge logic preserved the old value. Now send empty strings for clearable fields (github_cli_pat, git_auth tokens, gitlab URL, default_model, git identity) so the server correctly clears them. Also add mask-preservation for github_cli_pat on the server side, matching the existing pattern for git_auth tokens and env_vars.
The kilo serve child process captures process.env at spawn time, so clearing or changing the GitHub CLI PAT, git tokens, etc. only takes effect after an SDK server restart. Now detect auth-relevant config changes (github_cli_pat, github_token, gitlab_token) and trigger updateMayorModel to restart the SDK server, even when the model itself hasn't changed. The hot-swap path re-derives GH_TOKEN from the live process.env, so the new kilo serve process gets the correct fallback to the integration token.
… restart syncConfigToContainer updates the TownContainer DO's stored env vars, but those only take effect on the next container boot — the running container's process.env is not updated. When the SDK server restarts for auth config changes, it was still reading stale process.env. Now the PATCH /agents/:id/model endpoint receives fresh town config via X-Town-Config header and syncs it into process.env before the SDK server restart. This ensures the new kilo serve child process inherits the correct GITHUB_CLI_PAT, GIT_TOKEN, git identity, etc.
The server restarts the mayor's SDK server when auth config changes, creating a new session. But the UI only reloaded for model changes, leaving the frontend connected to the stale session. Now detect changes to github_cli_pat, github_token, and gitlab_token and trigger the same delayed page reload.
src/app/(app)/gastown/[townId]/settings/TownSettingsPageClient.tsx
Outdated
Show resolved
Hide resolved
…gitlab URL - Delete GH_TOKEN from hotSwapEnv when all auth sources are cleared, preventing stale credentials from surviving auth removal - Revert default_model to conditional spread so empty string doesn't override resolveModel()'s hardcoded fallback - Include gitlab_instance_url in the auth change detection on both the server (tRPC router) and client (settings page reload) so switching GitLab hosts triggers an SDK server restart and page reload
| ...(gitlabToken.startsWith('****') ? {} : { gitlab_token: gitlabToken }), | ||
| gitlab_instance_url: gitlabInstanceUrl, | ||
| }, | ||
| ...(defaultModel ? { default_model: defaultModel } : {}), |
Contributor
There was a problem hiding this comment.
WARNING: Clearing the default model no longer persists
Omitting default_model when the selector is blank means the server keeps the previous saved value, because this mutation only patches fields that are present. Towns that already have a default_model set can no longer return to resolveModel()'s hardcoded fallback from this page, so the old model keeps being used after refresh and on later agent dispatches.
pandemicsyn
approved these changes
Mar 25, 2026
Send default_model: '' from the UI when the selector is blank, and normalize it to undefined server-side in updateTownConfig. This allows resolveModel()'s nullish-coalescing fallback to kick in, restoring the hardcoded default. Previously, omitting the field preserved the old value, and sending '' bypassed the ?? fallback.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
role_models) to TownConfig, allowing mayor, refinery, and polecat roles to each use a different LLM model instead of sharing a singledefault_modelresolveModel()priority chain: role-specific override >default_model> hardcoded fallbackresolveModel()for correct effective model detectionVerification
git pushVisual Changes
Reviewer Notes
resolveModelfunction has thorough unit tests covering the full priority chain and backward compatibility with legacy configsreconstructConversation) delegates work to the AgentDO to avoid loading thousands of events in the TownDOtrpc/dist/index.d.tschanges are generated build artifacts from the gastown router type changes