From ba0b0f6d8671e9a4dde171e1549bac3a0b6f0f48 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 5 Feb 2026 00:22:31 +0100 Subject: [PATCH] feat(coding-agent): refactored model role system and extracted model resolution utilities - Added 'commit' model role for dedicated commit message generation. - Exported 'resolveModelOverride' function from model-resolver module for public use. - Refactored model role system to centralize role configuration in MODEL_ROLES registry. - Extracted model resolution utilities into dedicated model-resolver module for improved code organization. - Made 'tag' and 'color' properties optional in ModelRoleInfo interface to support roles without visual indicators. - Enhanced model selector with defensive null/undefined checks for optional role properties. --- packages/coding-agent/CHANGELOG.md | 10 ++++ .../src/commit/model-selection.ts | 57 ++++++------------- .../coding-agent/src/config/model-registry.ts | 9 +-- .../coding-agent/src/config/model-resolver.ts | 28 ++++----- .../src/modes/components/model-selector.ts | 4 +- .../src/modes/controllers/input-controller.ts | 2 +- .../modes/controllers/selector-controller.ts | 6 +- .../coding-agent/src/session/agent-session.ts | 2 +- packages/coding-agent/src/task/executor.ts | 34 +---------- packages/coding-agent/src/task/index.ts | 11 +--- 10 files changed, 58 insertions(+), 105 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 221a186c2..4c69d3193 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,8 +1,18 @@ # Changelog ## [Unreleased] + +### Added + +- Added `commit` model role for dedicated commit message generation +- Exported `resolveModelOverride` function from model resolver for external use + ### Changed +- Updated model role resolution to accept optional `roleOrder` parameter for custom role priority +- Made `tag` and `color` properties optional in `ModelRoleInfo` interface +- Updated model selector to safely handle roles without tag or color definitions +- Refactored role label display to use centralized `MODEL_ROLES` registry instead of hardcoded strings - Refactored model role system to use centralized `MODEL_ROLES` registry with consistent tag, name, and color definitions - Simplified model role resolution to use `MODEL_ROLE_IDS` array instead of hardcoded role checks - Updated model selector to dynamically generate menu actions from `MODEL_ROLES` registry diff --git a/packages/coding-agent/src/commit/model-selection.ts b/packages/coding-agent/src/commit/model-selection.ts index 3ad8eea32..9db23dcb2 100644 --- a/packages/coding-agent/src/commit/model-selection.ts +++ b/packages/coding-agent/src/commit/model-selection.ts @@ -1,5 +1,12 @@ import type { Api, Model } from "@oh-my-pi/pi-ai"; -import { parseModelPattern, parseModelString, SMOL_MODEL_PRIORITY } from "../config/model-resolver"; +import { MODEL_ROLE_IDS } from "../config/model-registry"; +import { + expandRoleAlias, + parseModelPattern, + resolveModelFromSettings, + resolveModelFromString, + SMOL_MODEL_PRIORITY, +} from "../config/model-resolver"; import type { Settings } from "../config/settings"; export async function resolvePrimaryModel( @@ -12,9 +19,15 @@ export async function resolvePrimaryModel( ): Promise<{ model: Model; apiKey: string }> { const available = modelRegistry.getAvailable(); const matchPreferences = { usageOrder: settings.getStorage()?.getModelUsageOrder() }; + const roleOrder = ["commit", "smol", ...MODEL_ROLE_IDS] as const; const model = override ? resolveModelFromString(expandRoleAlias(override, settings), available, matchPreferences) - : resolveModelFromSettings(settings, available, matchPreferences); + : resolveModelFromSettings({ + settings, + availableModels: available, + matchPreferences, + roleOrder, + }); if (!model) { throw new Error("No model available for commit generation"); } @@ -37,7 +50,9 @@ export async function resolveSmolModel( const available = modelRegistry.getAvailable(); const matchPreferences = { usageOrder: settings.getStorage()?.getModelUsageOrder() }; const role = settings.getModelRole("smol"); - const roleModel = role ? resolveModelFromString(role, available, matchPreferences) : undefined; + const roleModel = role + ? resolveModelFromString(expandRoleAlias(role, settings), available, matchPreferences) + : undefined; if (roleModel) { const apiKey = await modelRegistry.getApiKey(roleModel); if (apiKey) return { model: roleModel, apiKey }; @@ -52,39 +67,3 @@ export async function resolveSmolModel( return { model: fallbackModel, apiKey: fallbackApiKey }; } - -function resolveModelFromSettings( - settings: Settings, - available: Model[], - matchPreferences: { usageOrder?: string[] }, -): Model | undefined { - const roles = ["commit", "smol", "default"]; - for (const role of roles) { - const configured = settings.getModelRole(role); - if (!configured) continue; - const resolved = resolveModelFromString(expandRoleAlias(configured, settings), available, matchPreferences); - if (resolved) return resolved; - } - return available[0]; -} - -function resolveModelFromString( - value: string, - available: Model[], - matchPreferences: { usageOrder?: string[] }, -): Model | undefined { - const parsed = parseModelString(value); - if (parsed) { - return available.find(model => model.provider === parsed.provider && model.id === parsed.id); - } - return parseModelPattern(value, available, matchPreferences).model; -} - -function expandRoleAlias(value: string, settings: Settings): string { - const lower = value.toLowerCase(); - if (lower.startsWith("pi/")) { - const role = value.slice(3); - return settings.getModelRole(role) ?? value; - } - return value; -} diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index cc86a8d5d..cbbecd888 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -18,12 +18,12 @@ import { YAML } from "bun"; import type { ThemeColor } from "../modes/theme/theme"; import type { AuthStorage } from "../session/auth-storage"; -export type ModelRole = "default" | "smol" | "slow" | "plan"; +export type ModelRole = "default" | "smol" | "slow" | "plan" | "commit"; export interface ModelRoleInfo { - tag: string; + tag?: string; name: string; - color: ThemeColor; + color?: ThemeColor; } export const MODEL_ROLES: Record = { @@ -31,9 +31,10 @@ export const MODEL_ROLES: Record = { smol: { tag: "SMOL", name: "Fast", color: "warning" }, slow: { tag: "SLOW", name: "Thinking", color: "accent" }, plan: { tag: "PLAN", name: "Architect", color: "muted" }, + commit: { name: "Commit" }, }; -export const MODEL_ROLE_IDS: ModelRole[] = ["default", "smol", "slow", "plan"]; +export const MODEL_ROLE_IDS: ModelRole[] = ["default", "smol", "slow", "plan", "commit"]; const Ajv = (AjvModule as any).default || AjvModule; diff --git a/packages/coding-agent/src/config/model-resolver.ts b/packages/coding-agent/src/config/model-resolver.ts index 8b7275e17..55694f896 100644 --- a/packages/coding-agent/src/config/model-resolver.ts +++ b/packages/coding-agent/src/config/model-resolver.ts @@ -6,7 +6,7 @@ import { type Api, type KnownProvider, type Model, modelsAreEqual } from "@oh-my import chalk from "chalk"; import { isValidThinkingLevel } from "../cli/args"; import { fuzzyMatch } from "../utils/fuzzy"; -import { MODEL_ROLE_IDS, type ModelRegistry } from "./model-registry"; +import { MODEL_ROLE_IDS, type ModelRegistry, type ModelRole } from "./model-registry"; import type { Settings } from "./settings"; /** Default model IDs for each known provider */ @@ -311,28 +311,30 @@ export function parseModelPattern( return parseModelPatternWithContext(pattern, availableModels, context); } -const MODEL_ROLE_ALIAS_PREFIX = "pi/"; +const PREFIX_MODEL_ROLE = "pi/"; const DEFAULT_MODEL_ROLE = "default"; -const DEFAULT_MODEL_ALIASES = new Set([DEFAULT_MODEL_ROLE, `${MODEL_ROLE_ALIAS_PREFIX}${DEFAULT_MODEL_ROLE}`]); /** * Check if a model override value is effectively the default role. */ export function isDefaultModelAlias(value: string | string[] | undefined): boolean { if (!value) return true; - const values = Array.isArray(value) ? value : [value]; - if (values.length === 0) return true; - return values.every(entry => DEFAULT_MODEL_ALIASES.has(entry.trim().toLowerCase())); + if (Array.isArray(value)) return value.every(entry => isDefaultModelAlias(entry)); + if (value.startsWith(PREFIX_MODEL_ROLE)) { + value = value.slice(PREFIX_MODEL_ROLE.length); + } + return value === DEFAULT_MODEL_ROLE; } /** * Expand a role alias like "pi/smol" to the configured model string. */ export function expandRoleAlias(value: string, settings?: Settings): string { - const normalized = value.trim().toLowerCase(); - if (!normalized.startsWith(MODEL_ROLE_ALIAS_PREFIX)) return value; - const role = normalized.slice(MODEL_ROLE_ALIAS_PREFIX.length); - if (!MODEL_ROLE_IDS.includes(role as (typeof MODEL_ROLE_IDS)[number])) return value; + const normalized = value.trim(); + if (normalized === "default") return settings?.getModelRole("default") ?? value; + if (!normalized.startsWith(PREFIX_MODEL_ROLE)) return value; + const role = normalized.slice(PREFIX_MODEL_ROLE.length) as ModelRole; + if (!MODEL_ROLE_IDS.includes(role)) return value; return settings?.getModelRole(role) ?? value; } @@ -358,7 +360,7 @@ export function resolveModelFromSettings(options: { settings: Settings; availableModels: Model[]; matchPreferences?: ModelMatchPreferences; - roleOrder?: string[]; + roleOrder?: readonly ModelRole[]; }): Model | undefined { const { settings, availableModels, matchPreferences, roleOrder } = options; const roles = roleOrder ?? MODEL_ROLE_IDS; @@ -382,8 +384,8 @@ export function resolveModelOverride( if (modelPatterns.length === 0) return {}; const matchPreferences = { usageOrder: settings?.getStorage()?.getModelUsageOrder() }; for (const pattern of modelPatterns) { - const normalized = pattern.trim().toLowerCase(); - if (!normalized || DEFAULT_MODEL_ALIASES.has(normalized)) { + const normalized = pattern.trim(); + if (!normalized || isDefaultModelAlias(normalized)) { continue; } const effectivePattern = expandRoleAlias(pattern, settings); diff --git a/packages/coding-agent/src/modes/components/model-selector.ts b/packages/coding-agent/src/modes/components/model-selector.ts index aa3b70c6d..9d193de59 100644 --- a/packages/coding-agent/src/modes/components/model-selector.ts +++ b/packages/coding-agent/src/modes/components/model-selector.ts @@ -356,8 +356,8 @@ export class ModelSelectorComponent extends Container { const badges: string[] = []; for (const role of MODEL_ROLE_IDS) { const { tag, color } = MODEL_ROLES[role]; - if (modelsAreEqual(this.roles[role], item.model)) { - badges.push(makeInvertedBadge(tag, color)); + if (tag && modelsAreEqual(this.roles[role], item.model)) { + badges.push(makeInvertedBadge(tag, color ?? "success")); } } const badgeText = badges.length > 0 ? ` ${badges.join(" ")}` : ""; diff --git a/packages/coding-agent/src/modes/controllers/input-controller.ts b/packages/coding-agent/src/modes/controllers/input-controller.ts index 2bedcbf5e..11af5745a 100644 --- a/packages/coding-agent/src/modes/controllers/input-controller.ts +++ b/packages/coding-agent/src/modes/controllers/input-controller.ts @@ -649,7 +649,7 @@ export class InputController { async cycleRoleModel(options?: { temporary?: boolean }): Promise { try { - const roleOrder = ["slow", "default", "smol"]; + const roleOrder = ["slow", "default", "smol"] as const; const result = await this.ctx.session.cycleRoleModels(roleOrder, options); if (!result) { this.ctx.showStatus("Only one role model available"); diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index df25e23cc..aa3b5f474 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -3,6 +3,7 @@ import type { OAuthProvider } from "@oh-my-pi/pi-ai"; import type { Component } from "@oh-my-pi/pi-tui"; import { Input, Loader, Spacer, Text } from "@oh-my-pi/pi-tui"; import { getAgentDbPath } from "../../config"; +import { MODEL_ROLES } from "../../config/model-registry"; import { settings } from "../../config/settings"; import { DebugSelectorComponent } from "../../debug"; import { disableProvider, enableProvider } from "../../discovery"; @@ -277,7 +278,7 @@ export class SelectorController { this.ctx.session.scopedModels, async (model, role) => { try { - if (role === "temporary") { + if (role === null) { // Temporary: update agent state but don't persist to settings await this.ctx.session.setModelTemporary(model); this.ctx.statusLine.invalidate(); @@ -294,7 +295,8 @@ export class SelectorController { // Don't call done() - selector stays open for role assignment } else { // Other roles (smol, slow): just update settings, not current model - const roleLabel = role === "smol" ? "Smol" : role; + const roleInfo = MODEL_ROLES[role]; + const roleLabel = roleInfo?.name ?? role; this.ctx.showStatus(`${roleLabel} model: ${model.id}`); // Don't call done() - selector stays open } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 0541833ab..a629b64f1 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -1854,7 +1854,7 @@ export class AgentSession { * @param options - Optional settings: `temporary` to not persist to settings */ async cycleRoleModels( - roleOrder: ModelRole[], + roleOrder: readonly ModelRole[], options?: { temporary?: boolean }, ): Promise { const availableModels = this._modelRegistry.getAvailable(); diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index 6ba5e344c..97a465319 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -10,7 +10,7 @@ import { logger, untilAborted } from "@oh-my-pi/pi-utils"; import type { TSchema } from "@sinclair/typebox"; import Ajv, { type ValidateFunction } from "ajv"; import type { ModelRegistry } from "../config/model-registry"; -import { parseModelPattern } from "../config/model-resolver"; +import { resolveModelOverride } from "../config/model-resolver"; import { type PromptTemplate, renderPromptTemplate } from "../config/prompt-templates"; import { Settings } from "../config/settings"; import type { CustomTool } from "../extensibility/custom-tools/types"; @@ -128,38 +128,6 @@ function getReportFindingKey(value: unknown): string | null { return `${filePath}:${lineStart}:${lineEnd}:${priority ?? ""}:${title}`; } -function resolveModelOverride( - modelPatterns: string[], - modelRegistry: ModelRegistry, - settings?: Settings, -): { model?: Model; thinkingLevel?: ThinkingLevel } { - if (modelPatterns.length === 0) return {}; - const matchPreferences = { usageOrder: settings?.getStorage()?.getModelUsageOrder() }; - for (const pattern of modelPatterns) { - const normalized = pattern.trim().toLowerCase(); - if (!normalized || normalized === "default" || normalized === "pi/default") { - continue; - } - let effectivePattern = pattern; - if (normalized.startsWith("pi/")) { - const role = pattern.slice(3); - const configured = settings?.getModelRole(role); - if (configured) { - effectivePattern = configured; - } - } - const { model, thinkingLevel } = parseModelPattern( - effectivePattern, - modelRegistry.getAvailable(), - matchPreferences, - ); - if (model) { - return { model, thinkingLevel: thinkingLevel !== "off" ? thinkingLevel : undefined }; - } - } - return {}; -} - function buildSubmitResultToolChoice(model?: Model): ToolChoice | undefined { if (!model) return undefined; if ( diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index 937029b98..06bd8dff1 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -20,6 +20,7 @@ import type { Usage } from "@oh-my-pi/pi-ai"; import { $ } from "bun"; import { nanoid } from "nanoid"; import type { ToolSession } from ".."; +import { isDefaultModelAlias } from "../config/model-resolver"; import { renderPromptTemplate } from "../config/prompt-templates"; import type { Theme } from "../modes/theme/theme"; import planModeSubagentPrompt from "../prompts/system/plan-mode-subagent.md" with { type: "text" }; @@ -165,16 +166,6 @@ export class TaskTool implements AgentTool { - if (!value) return true; - const values = Array.isArray(value) ? value : [value]; - if (values.length === 0) return true; - return values.every(entry => { - const normalized = entry.trim().toLowerCase(); - return normalized === "default" || normalized === "pi/default"; - }); - }; - // Validate agent exists const agent = getAgent(agents, agentName); if (!agent) {