From 4a892a9809857d6cd11d85218fb47fc155e58b90 Mon Sep 17 00:00:00 2001 From: Nguyen Khac Trung Kien Date: Wed, 14 Jan 2026 22:15:05 +0700 Subject: [PATCH] fix(sisyphus-task): correct modelInfo.type detection to compare actual resolved model The previous detection checked if parentModelString exists, but the resolution uses a ?? chain where default may win over parent. Now compares actualModel against each source to correctly identify type. Fixes: model toast incorrectly showing 'inherited' when default was used --- src/tools/sisyphus-task/tools.test.ts | 124 +++++++++++++++++++++++++- src/tools/sisyphus-task/tools.ts | 18 ++-- 2 files changed, 133 insertions(+), 9 deletions(-) diff --git a/src/tools/sisyphus-task/tools.test.ts b/src/tools/sisyphus-task/tools.test.ts index 58dfe66d8..87fddc39e 100644 --- a/src/tools/sisyphus-task/tools.test.ts +++ b/src/tools/sisyphus-task/tools.test.ts @@ -4,7 +4,8 @@ import type { CategoryConfig } from "../../config/schema" function resolveCategoryConfig( categoryName: string, - userCategories?: Record + userCategories?: Record, + parentModelString?: string ): { config: CategoryConfig; promptAppend: string } | null { const defaultConfig = DEFAULT_CATEGORIES[categoryName] const userConfig = userCategories?.[categoryName] @@ -17,7 +18,7 @@ function resolveCategoryConfig( const config: CategoryConfig = { ...defaultConfig, ...userConfig, - model: userConfig?.model ?? defaultConfig?.model ?? "anthropic/claude-sonnet-4-5", + model: userConfig?.model ?? parentModelString ?? defaultConfig?.model ?? "anthropic/claude-sonnet-4-5", } let promptAppend = defaultPromptAppend @@ -205,6 +206,47 @@ describe("sisyphus-task", () => { expect(result).not.toBeNull() expect(result!.config.temperature).toBe(0.3) }) + + test("parentModelString is used when no user model and takes precedence over default", () => { + // #given + const categoryName = "visual-engineering" + const parentModelString = "cliproxy/claude-opus-4-5" + + // #when + const result = resolveCategoryConfig(categoryName, undefined, parentModelString) + + // #then + expect(result).not.toBeNull() + expect(result!.config.model).toBe("cliproxy/claude-opus-4-5") + }) + + test("user model takes precedence over parentModelString", () => { + // #given + const categoryName = "visual-engineering" + const userCategories = { + "visual-engineering": { model: "my-provider/my-model" }, + } + const parentModelString = "cliproxy/claude-opus-4-5" + + // #when + const result = resolveCategoryConfig(categoryName, userCategories, parentModelString) + + // #then + expect(result).not.toBeNull() + expect(result!.config.model).toBe("my-provider/my-model") + }) + + test("default model is used when no user model and no parentModelString", () => { + // #given + const categoryName = "visual-engineering" + + // #when + const result = resolveCategoryConfig(categoryName, undefined, undefined) + + // #then + expect(result).not.toBeNull() + expect(result!.config.model).toBe("google/gemini-3-pro-preview") + }) }) describe("category variant", () => { @@ -710,4 +752,82 @@ describe("sisyphus-task", () => { expect(result).toContain("\n\n") }) }) + + describe("modelInfo detection via resolveCategoryConfig", () => { + test("when parentModelString exists but default model wins - modelInfo should report default", () => { + // #given - Bug scenario: parentModelString is passed but userModel is undefined, + // and the resolution order is: userModel ?? parentModelString ?? defaultModel + // If parentModelString matches the resolved model, it's "inherited" + // If defaultModel matches, it's "default" + const categoryName = "ultrabrain" + const parentModelString = undefined + + // #when + const resolved = resolveCategoryConfig(categoryName, undefined, parentModelString) + + // #then - actualModel should be defaultModel, type should be "default" + expect(resolved).not.toBeNull() + const actualModel = resolved!.config.model + const defaultModel = DEFAULT_CATEGORIES[categoryName]?.model + expect(actualModel).toBe(defaultModel) + expect(actualModel).toBe("openai/gpt-5.2") + }) + + test("when parentModelString is used - modelInfo should report inherited", () => { + // #given + const categoryName = "ultrabrain" + const parentModelString = "cliproxy/claude-opus-4-5" + + // #when + const resolved = resolveCategoryConfig(categoryName, undefined, parentModelString) + + // #then - actualModel should be parentModelString, type should be "inherited" + expect(resolved).not.toBeNull() + const actualModel = resolved!.config.model + expect(actualModel).toBe(parentModelString) + }) + + test("when user defines model - modelInfo should report user-defined regardless of parentModelString", () => { + // #given + const categoryName = "ultrabrain" + const userCategories = { "ultrabrain": { model: "my-provider/custom-model" } } + const parentModelString = "cliproxy/claude-opus-4-5" + + // #when + const resolved = resolveCategoryConfig(categoryName, userCategories, parentModelString) + + // #then - actualModel should be userModel, type should be "user-defined" + expect(resolved).not.toBeNull() + const actualModel = resolved!.config.model + const userDefinedModel = userCategories[categoryName]?.model + expect(actualModel).toBe(userDefinedModel) + expect(actualModel).toBe("my-provider/custom-model") + }) + + test("detection logic: actualModel comparison correctly identifies source", () => { + // #given - This test verifies the fix for PR #770 bug + // The bug was: checking `if (parentModelString)` instead of `if (actualModel === parentModelString)` + const categoryName = "ultrabrain" + const parentModelString = "cliproxy/claude-opus-4-5" + const userCategories = { "ultrabrain": { model: "user/model" } } + + // #when - user model wins + const resolved = resolveCategoryConfig(categoryName, userCategories, parentModelString) + const actualModel = resolved!.config.model + const userDefinedModel = userCategories[categoryName]?.model + const defaultModel = DEFAULT_CATEGORIES[categoryName]?.model + + // #then - detection should compare against actual resolved model + const detectedType = actualModel === userDefinedModel + ? "user-defined" + : actualModel === parentModelString + ? "inherited" + : actualModel === defaultModel + ? "default" + : undefined + + expect(detectedType).toBe("user-defined") + expect(actualModel).not.toBe(parentModelString) + }) + }) }) diff --git a/src/tools/sisyphus-task/tools.ts b/src/tools/sisyphus-task/tools.ts index 127560e9e..413d27b0d 100644 --- a/src/tools/sisyphus-task/tools.ts +++ b/src/tools/sisyphus-task/tools.ts @@ -345,13 +345,17 @@ ${textContent || "(No text output)"}` return `❌ Unknown category: "${args.category}". Available: ${Object.keys({ ...DEFAULT_CATEGORIES, ...userCategories }).join(", ")}` } - const userHasDefinedCategory = userCategories?.[args.category]?.model !== undefined - if (userHasDefinedCategory) { - modelInfo = { model: resolved.config.model, type: "user-defined" } - } else if (parentModelString) { - modelInfo = { model: parentModelString, type: "inherited" } - } else if (DEFAULT_CATEGORIES[args.category]?.model) { - modelInfo = { model: DEFAULT_CATEGORIES[args.category].model, type: "default" } + // Determine model source by comparing against the actual resolved model + const actualModel = resolved.config.model + const userDefinedModel = userCategories?.[args.category]?.model + const defaultModel = DEFAULT_CATEGORIES[args.category]?.model + + if (actualModel === userDefinedModel) { + modelInfo = { model: actualModel, type: "user-defined" } + } else if (actualModel === parentModelString) { + modelInfo = { model: actualModel, type: "inherited" } + } else if (actualModel === defaultModel) { + modelInfo = { model: actualModel, type: "default" } } agentToUse = SISYPHUS_JUNIOR_AGENT