From 185665707c5053162cf6feec63b0791d21176fd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Choi=20Kijin=20/=20=EC=B5=9C=20=EA=B8=B0=EC=A7=84=20/=20?= =?UTF-8?q?=E3=83=81=E3=83=A7=E3=82=A4=20=E3=82=AD=E3=82=B8=E3=83=B3?= Date: Tue, 28 Apr 2026 21:43:10 +0900 Subject: [PATCH] fix(model-fallback): clone session fallback chains Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../fallback-state-controller.ts | 17 ++++--- src/hooks/model-fallback/hook.test.ts | 51 +++++++------------ 2 files changed, 26 insertions(+), 42 deletions(-) diff --git a/src/hooks/model-fallback/fallback-state-controller.ts b/src/hooks/model-fallback/fallback-state-controller.ts index 87bd28444..9bc3d1102 100644 --- a/src/hooks/model-fallback/fallback-state-controller.ts +++ b/src/hooks/model-fallback/fallback-state-controller.ts @@ -39,7 +39,7 @@ export function createModelFallbackStateController(input: { function setSessionFallbackChain(sessionID: string, fallbackChain: FallbackEntry[] | undefined): void { if (!sessionID) return - sessionFallbackChains.set(sessionID, fallbackChain?.length ? fallbackChain : []) + sessionFallbackChains.set(sessionID, fallbackChain?.length ? [...fallbackChain] : []) } function clearSessionFallbackChain(sessionID: string): void { @@ -47,7 +47,8 @@ export function createModelFallbackStateController(input: { } function getSessionFallbackChain(sessionID: string): FallbackEntry[] | undefined { - return sessionFallbackChains.get(sessionID) + const fallbackChain = sessionFallbackChains.get(sessionID) + return fallbackChain ? [...fallbackChain] : undefined } function setPendingModelFallback( @@ -61,7 +62,7 @@ export function createModelFallbackStateController(input: { const fallbackChain = sessionFallbackChains.get(sessionID) ?? requirements?.fallbackChain if (!fallbackChain?.length) { - log("[model-fallback] No fallback chain for agent: " + agentName + " (key: " + agentKey + ")") + log(`[model-fallback] No fallback chain for agent: ${agentName} (key: ${agentKey})`) return false } @@ -74,12 +75,12 @@ export function createModelFallbackStateController(input: { attemptCount: 0, pending: true, }) - log("[model-fallback] Set pending fallback for session: " + sessionID + ", agent: " + agentName) + log(`[model-fallback] Set pending fallback for session: ${sessionID}, agent: ${agentName}`) return true } if (existing.pending) { - log("[model-fallback] Pending fallback already armed for session: " + sessionID) + log(`[model-fallback] Pending fallback already armed for session: ${sessionID}`) return false } @@ -87,10 +88,10 @@ export function createModelFallbackStateController(input: { existing.modelID = currentModelID existing.pending = true if (existing.attemptCount >= existing.fallbackChain.length) { - log("[model-fallback] Fallback chain exhausted for session: " + sessionID) + log(`[model-fallback] Fallback chain exhausted for session: ${sessionID}`) return false } - log("[model-fallback] Re-armed pending fallback for session: " + sessionID) + log(`[model-fallback] Re-armed pending fallback for session: ${sessionID}`) return true } @@ -101,7 +102,7 @@ export function createModelFallbackStateController(input: { const fallback = getNextReachableFallback(sessionID, state) if (fallback) return fallback - log("[model-fallback] No more fallbacks for session: " + sessionID) + log(`[model-fallback] No more fallbacks for session: ${sessionID}`) pendingModelFallbacks.delete(sessionID) return null } diff --git a/src/hooks/model-fallback/hook.test.ts b/src/hooks/model-fallback/hook.test.ts index de9e66fd7..b12eee1cd 100644 --- a/src/hooks/model-fallback/hook.test.ts +++ b/src/hooks/model-fallback/hook.test.ts @@ -66,6 +66,7 @@ async function importFreshModelFallbackHookModule() { const { clearPendingModelFallback, createModelFallbackHook, + getSessionFallbackChain, setSessionFallbackChain, setPendingModelFallback, } = await importFreshModelFallbackHookModule() @@ -85,7 +86,6 @@ describe("model fallback hook", () => { }) test("applies pending fallback on chat.message by overriding model", async () => { - //#given const hook = modelFallback as unknown as { "chat.message"?: ( input: { sessionID: string }, @@ -110,13 +110,11 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.( { sessionID: "ses_model_fallback_main" }, output, ) - //#then expect(output.message["model"]).toEqual({ providerID: "anthropic", modelID: "claude-opus-4-7", @@ -124,7 +122,6 @@ describe("model fallback hook", () => { }) test("preserves fallback progression across repeated session.error retries", async () => { - //#given const hook = modelFallback as unknown as { "chat.message"?: ( input: { sessionID: string }, @@ -145,16 +142,13 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when - first retry is applied await hook["chat.message"]?.({ sessionID }, firstOutput) - //#then expect(firstOutput.message["model"]).toEqual({ providerID: "anthropic", modelID: "claude-opus-4-7", }) - //#when - second error re-arms fallback and should advance to next entry expect( setPendingModelFallback(modelFallback, sessionID, "Sisyphus - Ultraworker", "anthropic", "claude-opus-4-7"), ).toBe(true) @@ -167,7 +161,6 @@ describe("model fallback hook", () => { } await hook["chat.message"]?.({ sessionID }, secondOutput) - //#then - chain should progress to entry[1], not repeat entry[0] expect(secondOutput.message["model"]).toEqual({ providerID: "opencode-go", modelID: "kimi-k2.5", @@ -176,11 +169,9 @@ describe("model fallback hook", () => { }) test("does not re-arm fallback when one is already pending", () => { - //#given const sessionID = "ses_model_fallback_pending_guard" clearPendingModelFallback(modelFallback, sessionID) - //#when const firstSet = setPendingModelFallback( modelFallback, sessionID, @@ -196,14 +187,28 @@ describe("model fallback hook", () => { "claude-opus-4-7-thinking", ) - //#then expect(firstSet).toBe(true) expect(secondSet).toBe(false) clearPendingModelFallback(modelFallback, sessionID) }) + test("isolates stored fallback chains from caller mutations on set and get", () => { + const sessionID = "ses_model_fallback_defensive_copy" + const originalChain = [ + { providers: ["anthropic"], model: "claude-opus-4-7" }, + ] + + setSessionFallbackChain(modelFallback, sessionID, originalChain) + originalChain.push({ providers: ["google"], model: "gemini-2.5-pro" }) + const retrieved = getSessionFallbackChain(modelFallback, sessionID) + retrieved?.push({ providers: ["openai"], model: "gpt-5.4" }) + + expect(getSessionFallbackChain(modelFallback, sessionID)).toEqual([ + { providers: ["anthropic"], model: "claude-opus-4-7" }, + ]) + }) + test("skips no-op fallback entries that resolve to same provider/model", async () => { - //#given const sessionID = "ses_model_fallback_noop_skip" clearPendingModelFallback(modelFallback, sessionID) @@ -236,10 +241,8 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.({ sessionID }, output) - //#then expect(output.message["model"]).toEqual({ providerID: "opencode", modelID: "kimi-k2.5-free", @@ -248,7 +251,6 @@ describe("model fallback hook", () => { }) test("skips no-op fallback entries even when variant differs", async () => { - //#given const sessionID = "ses_model_fallback_noop_variant_skip" clearPendingModelFallback(modelFallback, sessionID) @@ -282,10 +284,8 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.({ sessionID }, output) - //#then expect(output.message["model"]).toEqual({ providerID: "quotio", modelID: "gpt-5.2", @@ -295,7 +295,6 @@ describe("model fallback hook", () => { }) test("uses connected preferred provider when fallback entry providers are disconnected", async () => { - //#given const sessionID = "ses_model_fallback_preferred_provider" clearPendingModelFallback(modelFallback, sessionID) readConnectedProvidersCacheMock.mockReturnValue(["provider-x"]) @@ -328,10 +327,8 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.({ sessionID }, output) - //#then expect(output.message["model"]).toEqual({ providerID: "provider-x", modelID: "fallback-model", @@ -340,12 +337,10 @@ describe("model fallback hook", () => { }) test("does not fall back to hardcoded agent chain when session explicitly stores no fallback chain [regression #2941]", () => { - //#given const sessionID = "ses_model_fallback_explicit_none" clearPendingModelFallback(modelFallback, sessionID) setSessionFallbackChain(modelFallback, sessionID, undefined) - //#when const set = setPendingModelFallback( modelFallback, sessionID, @@ -354,13 +349,11 @@ describe("model fallback hook", () => { "claude-sonnet-4-6", ) - //#then expect(set).toBe(false) clearPendingModelFallback(modelFallback, sessionID) }) test("shows toast when fallback is applied", async () => { - //#given const toastCalls: Array<{ title: string; message: string }> = [] const hook = createModelFallbackHook({ toast: async ({ title, message }) => { @@ -390,16 +383,13 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.({ sessionID: "ses_model_fallback_toast" }, output) - //#then expect(toastCalls.length).toBe(1) expect(toastCalls[0]?.title).toBe("Model fallback") }) test("transforms model names for github-copilot provider via fallback chain", async () => { - //#given const sessionID = "ses_model_fallback_ghcp" clearPendingModelFallback(modelFallback, sessionID) @@ -410,7 +400,6 @@ describe("model fallback hook", () => { ) => Promise } - // Set a custom fallback chain that routes through github-copilot setSessionFallbackChain(modelFallback, sessionID, [ { providers: ["github-copilot"], model: "claude-sonnet-4-6" }, ]) @@ -431,10 +420,8 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.({ sessionID }, output) - //#then - model name should be transformed from hyphen to dot notation expect(output.message["model"]).toEqual({ providerID: "github-copilot", modelID: "claude-sonnet-4.6", @@ -444,7 +431,6 @@ describe("model fallback hook", () => { }) test("preserves canonical google preview model names via fallback chain", async () => { - //#given const sessionID = "ses_model_fallback_google" clearPendingModelFallback(modelFallback, sessionID) @@ -455,7 +441,6 @@ describe("model fallback hook", () => { ) => Promise } - // Set a custom fallback chain that routes through google setSessionFallbackChain(modelFallback, sessionID, [ { providers: ["google"], model: "gemini-3.1-pro-preview" }, ]) @@ -476,10 +461,8 @@ describe("model fallback hook", () => { parts: [{ type: "text", text: "continue" }], } - //#when await hook["chat.message"]?.({ sessionID }, output) - //#then: model name should remain gemini-3.1-pro-preview because no google transform exists for this ID expect(output.message["model"]).toEqual({ providerID: "google", modelID: "gemini-3.1-pro-preview",