From 2e001bbf8d4a058112e710a761b9e34229396e82 Mon Sep 17 00:00:00 2001 From: can1357 Date: Wed, 4 Mar 2026 23:21:10 +0100 Subject: [PATCH] fix(coding-agent): fixed model selector to handle multiple roles on same model - Added formatRoleThinkingModeLabel helper to display 'inherit' for default thinking mode, preventing badge ambiguity when multiple roles share the same model. Enhanced role menu labels to include role tags for clarity. Fixed model resolver to avoid substring matching that could incorrectly resolve exact model IDs to similar variants. --- .../src/modes/components/model-selector.ts | 27 +++++++----- .../test/agent-session-role-thinking.test.ts | 28 ++++++++++++ .../coding-agent/test/model-resolver.test.ts | 43 ++++++++++++++++++- ...model-selector-role-badge-thinking.test.ts | 18 ++++++-- 4 files changed, 100 insertions(+), 16 deletions(-) diff --git a/packages/coding-agent/src/modes/components/model-selector.ts b/packages/coding-agent/src/modes/components/model-selector.ts index 78ad2a4e0..cbe27eddb 100644 --- a/packages/coding-agent/src/modes/components/model-selector.ts +++ b/packages/coding-agent/src/modes/components/model-selector.ts @@ -16,6 +16,11 @@ function makeInvertedBadge(label: string, color: ThemeColor): string { return `${bgAnsi}\x1b[30m ${label} \x1b[39m\x1b[49m`; } +function formatRoleThinkingModeLabel(thinkingMode: RoleThinkingMode): string { + if (thinkingMode === "default") return "inherit"; + return formatThinkingEffortLabel(thinkingMode); +} + interface ModelItem { provider: string; id: string; @@ -41,10 +46,14 @@ interface MenuRoleAction { role: ModelRole; } -const MENU_ROLE_ACTIONS: MenuRoleAction[] = MODEL_ROLE_IDS.map(role => ({ - label: `Set as ${MODEL_ROLES[role].name}`, - role, -})); +const MENU_ROLE_ACTIONS: MenuRoleAction[] = MODEL_ROLE_IDS.map(role => { + const roleInfo = MODEL_ROLES[role]; + const roleLabel = roleInfo.tag ? `${roleInfo.tag} (${roleInfo.name})` : roleInfo.name; + return { + label: `Set as ${roleLabel}`, + role, + }; +}); const THINKING_MODE_OPTIONS: RoleThinkingMode[] = ["default", "off", "minimal", "low", "medium", "high"]; const ALL_TAB = "ALL"; @@ -403,12 +412,7 @@ export class ModelSelectorComponent extends Container { if (!tag || !assigned || !modelsAreEqual(assigned.model, item.model)) continue; const badge = makeInvertedBadge(tag, color ?? "success"); - if (assigned.thinkingMode === "default") { - roleBadgeTokens.push(badge); - continue; - } - - const thinkingLabel = formatThinkingEffortLabel(assigned.thinkingMode); + const thinkingLabel = formatRoleThinkingModeLabel(assigned.thinkingMode); roleBadgeTokens.push(`${badge} ${theme.fg("dim", `(${thinkingLabel})`)}`); } const badgeText = roleBadgeTokens.length > 0 ? ` ${roleBadgeTokens.join(" ")}` : ""; @@ -503,7 +507,8 @@ export class ModelSelectorComponent extends Container { const optionLines = showingThinking ? thinkingOptions.map((thinkingMode, index) => { const prefix = index === this.#menuSelectedIndex ? ` ${theme.nav.cursor} ` : " "; - return `${prefix}${thinkingMode}`; + const label = formatRoleThinkingModeLabel(thinkingMode); + return `${prefix}${label}`; }) : MENU_ROLE_ACTIONS.map((action, index) => { const prefix = index === this.#menuSelectedIndex ? ` ${theme.nav.cursor} ` : " "; diff --git a/packages/coding-agent/test/agent-session-role-thinking.test.ts b/packages/coding-agent/test/agent-session-role-thinking.test.ts index 7489aa15e..1a05375db 100644 --- a/packages/coding-agent/test/agent-session-role-thinking.test.ts +++ b/packages/coding-agent/test/agent-session-role-thinking.test.ts @@ -124,6 +124,34 @@ describe("AgentSession role model thinking behavior", () => { expect(session.thinkingLevel).toBe("minimal"); }); + it("applies slow role thinking even when plan shares the same model", async () => { + const defaultModel = getAnthropicModelOrThrow("claude-sonnet-4-5"); + const smolModel = getAnthropicModelOrThrow("claude-sonnet-4-6"); + const slowPlanModel = getAnthropicModelOrThrow("claude-opus-4-5"); + + await createSession({ + initialModelId: defaultModel.id, + initialThinkingLevel: "medium", + modelRoles: { + default: `${defaultModel.provider}/${defaultModel.id}`, + smol: `${smolModel.provider}/${smolModel.id}:low`, + slow: `${slowPlanModel.provider}/${slowPlanModel.id}:high`, + plan: `${slowPlanModel.provider}/${slowPlanModel.id}:off`, + }, + }); + + const toSmol = await session.cycleRoleModels(["slow", "default", "smol"]); + expect(toSmol?.role).toBe("smol"); + expect(toSmol?.thinkingLevel).toBe("low"); + expect(session.thinkingLevel).toBe("low"); + + const toSlow = await session.cycleRoleModels(["slow", "default", "smol"]); + expect(toSlow?.role).toBe("slow"); + expect(toSlow?.model.id).toBe(slowPlanModel.id); + expect(toSlow?.thinkingLevel).toBe("high"); + expect(session.thinkingLevel).toBe("high"); + }); + it("preserves explicit role thinking when updating default model despite unresolved previous model", async () => { const defaultModel = getAnthropicModelOrThrow("claude-sonnet-4-5"); const slowModel = getAnthropicModelOrThrow("claude-sonnet-4-6"); diff --git a/packages/coding-agent/test/model-resolver.test.ts b/packages/coding-agent/test/model-resolver.test.ts index a8cabd02c..d9c2485e2 100644 --- a/packages/coding-agent/test/model-resolver.test.ts +++ b/packages/coding-agent/test/model-resolver.test.ts @@ -92,7 +92,34 @@ const mockProviderOverlapModels: Model<"anthropic-messages">[] = [ }, ]; -const allModels = [...mockModels, ...mockOpenRouterModels, ...mockProviderOverlapModels]; +const mockCodexOverlapModels: Model<"anthropic-messages">[] = [ + { + id: "gpt-5.3-codex", + name: "GPT-5.3 Codex", + api: "anthropic-messages", + provider: "openai-codex", + baseUrl: "https://api.openai.com", + reasoning: true, + input: ["text"], + cost: { input: 1.5, output: 6, cacheRead: 0.15, cacheWrite: 1.5 }, + contextWindow: 200000, + maxTokens: 8192, + }, + { + id: "gpt-5.3-codex-spark", + name: "GPT-5.3 Codex Spark", + api: "anthropic-messages", + provider: "openai-codex", + baseUrl: "https://api.openai.com", + reasoning: true, + input: ["text"], + cost: { input: 1, output: 4, cacheRead: 0.1, cacheWrite: 1 }, + contextWindow: 200000, + maxTokens: 8192, + }, +]; + +const allModels = [...mockModels, ...mockOpenRouterModels, ...mockProviderOverlapModels, ...mockCodexOverlapModels]; describe("parseModelPattern", () => { describe("simple patterns without colons", () => { @@ -298,6 +325,20 @@ describe("resolveModelRoleValue", () => { expect(result.explicitThinkingLevel).toBe(false); expect(result.warning).toBeUndefined(); }); + + test("does not resolve exact codex role values to codex-spark via substring matching", () => { + const providerQualified = resolveModelRoleValue("openai-codex/gpt-5.3-codex:xhigh", allModels); + expect(providerQualified.model?.provider).toBe("openai-codex"); + expect(providerQualified.model?.id).toBe("gpt-5.3-codex"); + expect(providerQualified.thinkingLevel).toBe("xhigh"); + expect(providerQualified.explicitThinkingLevel).toBe(true); + + const idOnly = resolveModelRoleValue("gpt-5.3-codex:xhigh", allModels); + expect(idOnly.model?.provider).toBe("openai-codex"); + expect(idOnly.model?.id).toBe("gpt-5.3-codex"); + expect(idOnly.thinkingLevel).toBe("xhigh"); + expect(idOnly.explicitThinkingLevel).toBe(true); + }); }); describe("resolveModelFromString", () => { test("falls back to pattern parsing for provider/model:thinking when strict provider+id miss", () => { diff --git a/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts b/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts index e7db2a970..f7f56cb27 100644 --- a/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts +++ b/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts @@ -22,7 +22,7 @@ describe("ModelSelector role badge thinking display", () => { initTheme(); }); - test("renders explicit thinking next to role badges and removes details duplication", async () => { + test("renders per-role thinking labels with inherit mode to avoid badge ambiguity", async () => { const model = getBundledModel("anthropic", "claude-sonnet-4-5"); if (!model) throw new Error("Expected bundled model anthropic/claude-sonnet-4-5"); @@ -30,7 +30,8 @@ describe("ModelSelector role badge thinking display", () => { modelRoles: { default: `${model.provider}/${model.id}`, smol: `${model.provider}/${model.id}:minimal`, - slow: `${model.provider}/${model.id}:off`, + slow: `${model.provider}/${model.id}`, + plan: `${model.provider}/${model.id}:high`, commit: `${model.provider}/${model.id}:medium`, }, }); @@ -55,10 +56,19 @@ describe("ModelSelector role badge thinking display", () => { await Bun.sleep(0); const rendered = normalizeRenderedText(selector.render(220).join("\n")); + expect(rendered).toContain("DEFAULT (inherit)"); expect(rendered).toContain("SMOL (min)"); - expect(rendered).toContain("SLOW (off)"); + expect(rendered).toContain("SLOW (inherit)"); + expect(rendered).toContain("PLAN (high)"); expect(rendered).toContain("COMMIT (medium)"); - expect(rendered).not.toContain("DEFAULT ("); expect(rendered).not.toContain("Role Thinking:"); + + selector.handleInput("\n"); + const menuRendered = normalizeRenderedText(selector.render(220).join("\n")); + expect(menuRendered).toContain("Set as DEFAULT (Default)"); + expect(menuRendered).toContain("Set as SMOL (Fast)"); + expect(menuRendered).toContain("Set as SLOW (Thinking)"); + expect(menuRendered).toContain("Set as PLAN (Architect)"); + expect(menuRendered).toContain("Set as COMMIT (Commit)"); }); });