From a1e60c3450ee6c652eaffe360a929f94af24d643 Mon Sep 17 00:00:00 2001 From: enieuwy <121954036+enieuwy@users.noreply.github.com> Date: Sat, 8 Aug 2026 11:47:07 +0800 Subject: [PATCH] refactor(session): fold the pending-fallback surface into servingModel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final review found `pendingRetryFallbackModel` unreachable. `servingModel` returns `undefined` only when the session has no model at all, and the pending getter required one, so the badge term guarding on it could never fire. Its case — a fallback armed before anything has served — is already answered by `servingModel`'s bootstrap, which names the current model and flags it as fallback-routed. Removed, the same duplicate-surface cleanup that removed `retryFallbackModel`. Attribution now anchors on the session id rather than the session file. An unpersisted session has no file, so two `undefined`s compared equal and stale attribution survived `/new` and branch switches there; every real switch mints a new id, persisted or not. The cooldown-expiry restore keeps `#fallbackRouted` when the stored primary selector cannot be parsed. Nothing is restored on that path, so the session is still running on the fallback and its remaining turns are still fallback work; clearing the flag reported them as the configured primary. `executor-prewalk`'s fake session predates this work and never set `servingModel`, so the prewalk hand-off stopped advancing the reported model once the executor began reading attribution from the session. It now mirrors the hand-off the way the other executor fixtures do. --- .../modes/components/agent-hub-renderer.ts | 6 +-- .../coding-agent/src/session/agent-session.ts | 5 --- .../coding-agent/src/session/turn-recovery.ts | 40 +++++++++---------- .../test/agent-hub-ordering.test.ts | 5 ++- .../test/agent-session-retry-fallback.test.ts | 17 ++++---- .../test/task/executor-prewalk.test.ts | 6 +++ 6 files changed, 39 insertions(+), 40 deletions(-) diff --git a/packages/coding-agent/src/modes/components/agent-hub-renderer.ts b/packages/coding-agent/src/modes/components/agent-hub-renderer.ts index a97e7caf3..4e345b09f 100644 --- a/packages/coding-agent/src/modes/components/agent-hub-renderer.ts +++ b/packages/coding-agent/src/modes/components/agent-hub-renderer.ts @@ -109,11 +109,7 @@ export function modelBadge(ref: AgentRef, observed: ObservableSession | undefine const fallbackSelector = (serving?.isFallback ? serving.selector : undefined) ?? (progress?.resolvedModelIsFallback ? progress.resolvedModel : undefined) ?? - (ref.history?.resolvedModelIsFallback ? ref.history.resolvedModel : undefined) ?? - // Nothing has served at all, so there is no earlier work to miscredit and - // the armed candidate is all there is to report — but it is still a - // fallback, and a bare model badge would hide that. - (serving ? undefined : ref.session?.pendingRetryFallbackModel); + (ref.history?.resolvedModelIsFallback ? ref.history.resolvedModel : undefined); if (fallbackSelector) { return `${theme.fg("warning", "fallback →")} ${formatResolvedModelBadge(fallbackSelector, true, liveThinkingLevel)}`; } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 3a8650256..827bdc8e0 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -4003,11 +4003,6 @@ export class AgentSession { return this.#recovery.servingModel; } - /** Selector of a fallback that is armed but has not produced a turn yet. */ - get pendingRetryFallbackModel(): string | undefined { - return this.#recovery.pendingRetryFallbackModel; - } - /** Install the interactive decision surface for reserve-triggered model changes. */ setUsageFallbackConfirmer(confirmer: UsageFallbackConfirmer | undefined): void { this.#usageFallbackConfirmer = confirmer; diff --git a/packages/coding-agent/src/session/turn-recovery.ts b/packages/coding-agent/src/session/turn-recovery.ts index 0049f243a..0cc0fba4b 100644 --- a/packages/coding-agent/src/session/turn-recovery.ts +++ b/packages/coding-agent/src/session/turn-recovery.ts @@ -189,14 +189,20 @@ export class TurnRecovery { #emptyStopRetryCount = 0; #unexpectedStopRetryCount = 0; #acceptTerminalEmptyStopForPrompt = false; + // Three fields sit near the word "serve" and are deliberately distinct: + // `#activeRetryFallback.served` gates the one-shot `retry_fallback_succeeded` + // event for the current arm, `#fallbackRouted` says how the CURRENT model was + // reached, and `#lastServed` is the session's attribution. A fallback flipping + // to served does not by itself move attribution — only a settled turn does. /** * Attribution of the newest turn that produced output, tagged with the - * transcript it belongs to. Anchoring rather than resetting follows - * `#ensurePersistedMessageKeys`: switching sessions in place changes the - * session file, so the stale attribution drops itself and no mutation call - * site has to remember to clear it. + * session it belongs to. Anchoring rather than resetting follows + * `#ensurePersistedMessageKeys`: every real switch mints a new session id, so + * stale attribution drops itself and no mutation call site has to remember to + * clear it. The id — not the file — is the anchor because an unpersisted + * session has no file, and comparing two `undefined`s would never invalidate. */ - #lastServed: { attribution: ServingModel; sessionFile: string | undefined } | undefined; + #lastServed: { attribution: ServingModel; sessionId: string } | undefined; /** * Whether the current model was reached by fallback routing rather than by * the configured primary. Tracked separately from {@link #activeRetryFallback} @@ -245,7 +251,7 @@ export class TurnRecovery { */ get servingModel(): ServingModel | undefined { const served = this.#lastServed; - if (served && served.sessionFile === this.#host.sessionManager.getSessionFile()) return served.attribution; + if (served && served.sessionId === this.#host.sessionManager.getSessionId()) return served.attribution; const model = this.#host.model(); if (!model) return undefined; // Polled per streaming event and per render, so the pre-first-turn window @@ -263,19 +269,6 @@ export class TurnRecovery { return value; } - /** - * Selector of a fallback that is armed but has not served yet. - * - * Nothing has been attributed to it, so it is not the serving model — but - * when the session has produced no output at all there is no earlier work to - * miscredit, and naming it is the only honest thing an observer can show. - */ - get pendingRetryFallbackModel(): string | undefined { - const model = this.#host.model(); - if (!model || !this.#activeRetryFallback || this.#activeRetryFallback.served) return undefined; - return formatRetryFallbackSelector(model, this.#host.thinkingLevel()); - } - /** Resets per-prompt recovery counters and terminal-stop acceptance. */ resetForNewPrompt(): void { this.#emptyStopRetryCount = 0; @@ -304,7 +297,7 @@ export class TurnRecovery { selector: formatRetryFallbackSelector(model, this.#host.thinkingLevel()), isFallback: this.#fallbackRouted, }, - sessionFile: this.#host.sessionManager.getSessionFile(), + sessionId: this.#host.sessionManager.getSessionId(), }; } // Independent of the retry saga below: a usage-aware fallback is applied @@ -1531,7 +1524,12 @@ export class TurnRecovery { } = this.#activeRetryFallback; const originalSelector = parseRetryFallbackSelector(originalSelectorRaw, this.#host.modelRegistry); if (!originalSelector) { - this.clearActiveRetryFallback(); + // Defensive: the stored selector is always produced by + // `formatRetryFallbackSelector`, so it should never fail to parse. If it + // somehow does, nothing is restored and the session keeps running on the + // fallback — so drop the chain record but NOT `#fallbackRouted`, whose + // clearing would report the fallback's remaining turns as the primary. + this.#activeRetryFallback = undefined; return false; } diff --git a/packages/coding-agent/test/agent-hub-ordering.test.ts b/packages/coding-agent/test/agent-hub-ordering.test.ts index df029726e..6aea29399 100644 --- a/packages/coding-agent/test/agent-hub-ordering.test.ts +++ b/packages/coding-agent/test/agent-hub-ordering.test.ts @@ -477,8 +477,9 @@ describe("Agent hub row ordering", () => { const session = { model: { id: "gpt-5.6-sol", thinking: true }, thinkingLevel: "high", - servingModel: undefined, - pendingRetryFallbackModel: "openai-codex/gpt-5.6-sol", + // Nothing served, so the session names what it currently points at — + // still flagged as fallback-routed. + servingModel: { selector: "openai-codex/gpt-5.6-sol", isFallback: true }, } as unknown as AgentSession; agents.register({ id: "UnprovenAgent", displayName: "Unproven Agent", kind: "sub", session }); diff --git a/packages/coding-agent/test/agent-session-retry-fallback.test.ts b/packages/coding-agent/test/agent-session-retry-fallback.test.ts index 191a17af7..29ff8297b 100644 --- a/packages/coding-agent/test/agent-session-retry-fallback.test.ts +++ b/packages/coding-agent/test/agent-session-retry-fallback.test.ts @@ -4241,10 +4241,11 @@ describe("AgentSession retry fallback", () => { selector: `${primaryModel.provider}/${primaryModel.id}`, isFallback: false, }); - // Attribution belongs to the transcript it was earned in. Switching the - // session in place drops it rather than carrying another session's model - // across, leaving only what this session currently points at. - vi.spyOn(session.sessionManager, "getSessionFile").mockReturnValue("/tmp/some-other-session.jsonl"); + // Attribution belongs to the session it was earned in. Every real switch + // mints a new session id — including for an unpersisted session, which has + // no file to compare — so the stale value drops itself, leaving only what + // this session currently points at. + vi.spyOn(session.sessionManager, "getSessionId").mockReturnValue("some-other-session"); expect(session.servingModel).toEqual({ selector: `${fallbackModel.provider}/${fallbackModel.id}`, isFallback: true, @@ -4274,6 +4275,11 @@ describe("AgentSession retry fallback", () => { modelRegistry, }); + // Observers poll this per streaming event and per render. Before anything + // has served the answer is computed rather than stored, so that is the + // window where a fresh allocation per call would show up. + expect(session.servingModel).toBe(session.servingModel); + await session.prompt("Fail over to a working fallback"); await session.waitForIdle(); @@ -4282,9 +4288,6 @@ describe("AgentSession retry fallback", () => { selector: `${fallbackModel.provider}/${fallbackModel.id}`, isFallback: true, }); - // Observers poll this per streaming event and per render, so a steady - // session must not allocate a fresh answer each time. - expect(session.servingModel).toBe(session.servingModel); }); it("keeps attribution on a served fallback while the next candidate is unproven", async () => { diff --git a/packages/coding-agent/test/task/executor-prewalk.test.ts b/packages/coding-agent/test/task/executor-prewalk.test.ts index 6f72d4830..998c35935 100644 --- a/packages/coding-agent/test/task/executor-prewalk.test.ts +++ b/packages/coding-agent/test/task/executor-prewalk.test.ts @@ -30,10 +30,15 @@ function yieldEmittingSession( ): AgentSession { const listeners: Array<(event: AgentSessionEvent) => void> = []; let activeTools = initialTools; + // `servingModel` mirrors the real session: attribution names the model that + // produced output, so a prewalk hand-off moves it along with `model`. + const serving = (model: Model | undefined): { selector: string; isFallback: boolean } | undefined => + model ? { selector: `${model.provider}/${model.id}`, isFallback: false } : undefined; const session = { state: { messages: [] }, agent: { state: { systemPrompt: ["test"] } }, model: modelSwitch?.from, + servingModel: serving(modelSwitch?.from), extensionRunner: undefined, sessionManager: { appendSessionInit: () => {} }, getActiveToolNames: () => activeTools, @@ -52,6 +57,7 @@ function yieldEmittingSession( prompt: async (_text: string, _options?: PromptOptions) => { if (modelSwitch) { session.model = modelSwitch.to; + session.servingModel = serving(modelSwitch.to); for (const listener of listeners) { listener({ type: "notice", level: "info", message: "Prewalk switched", source: "prewalk" }); }