fix: five correctness bugs (model-wipe, infinite retry loop, type gap)

src/__tests__/shared-handle-retry.test.ts

@@ -16,6 +16,7 @@ import { TalonError } from "../core/errors.js";
 import { registerModels, clearModels } from "../core/models.js";
 import { setChatModel, getChatSettings } from "../storage/chat-settings.js";
 import { resetSession, getSession } from "../storage/sessions.js";
+import type { QueryParams } from "../core/types.js";
 
 beforeEach(() => {
   clearModels();
@@ -153,7 +154,7 @@ describe("shared / applyRetryDecision — reset_and_retry path", () => {
 });
 
 describe("shared / applyRetryDecision — fallback_model path", () => {
-  it("transient-swaps the chat model during recursion and restores after", async () => {
+  it("passes the fallback model via params.model during recursion", async () => {
     registerModels([
       {
         id: "primary",
@@ -170,10 +171,10 @@ describe("shared / applyRetryDecision — fallback_model path", () => {
       },
     ]);
 
-    // Capture the model that was active inside the recursion.
-    let modelDuringRecursion: string | undefined;
-    const recurse = vi.fn(async () => {
-      modelDuringRecursion = getChatSettings("test-chat").model;
+    // Capture the params handed to the recursive call.
+    let paramsDuringRecursion: QueryParams | undefined;
+    const recurse = vi.fn(async (p: QueryParams) => {
+      paramsDuringRecursion = p;
       return {
         text: "ok",
         durationMs: 1,
@@ -195,15 +196,13 @@ describe("shared / applyRetryDecision — fallback_model path", () => {
     });
 
     expect(outcome.retry?.text).toBe("ok");
-    // During recursion, the model was the fallback.
-    expect(modelDuringRecursion).toBe("fallback");
-    // After recursion returned, the chat model is restored to whatever
-    // it was originally (undefined here — the test set it to undefined
-    // in beforeEach).
+    // The fallback model is injected via params.model, not chat settings.
+    expect(paramsDuringRecursion?.model).toBe("fallback");
+    // Chat settings are never mutated — user's model preference is intact.
     expect(getChatSettings("test-chat").model).toBeUndefined();
   });
 
-  it("restores the original chat model even when the recursive retry throws", async () => {
+  it("does not mutate chat settings even when the recursive retry throws", async () => {
     registerModels([
       {
         id: "primary",
@@ -219,7 +218,7 @@ describe("shared / applyRetryDecision — fallback_model path", () => {
         displayName: "Fallback",
       },
     ]);
-    // Pre-set the chat model so we can verify it's restored.
+    // Pre-set the chat model to verify settings survive the fallback attempt.
     setChatModel("test-chat", "user-pinned");
 
     const recurse = vi.fn(async () => {
@@ -238,7 +237,7 @@ describe("shared / applyRetryDecision — fallback_model path", () => {
       }),
     ).rejects.toThrow("retry blew up");
 
-    // Despite the throw, the user's pinned model is back in place.
+    // Chat settings are not touched — user-pinned model is preserved.
     expect(getChatSettings("test-chat").model).toBe("user-pinned");
   });
 

src/backend/codex/handler.ts

@@ -49,7 +49,7 @@ import {
   setSessionId,
   resetSession,
 } from "../../storage/sessions.js";
-import { getChatSettings, setChatModel } from "../../storage/chat-settings.js";
+import { getChatSettings } from "../../storage/chat-settings.js";
 import { log, logError, logWarn } from "../../util/log.js";
 import { traceMessage } from "../../util/trace.js";
 import { incrementCounter, recordHistogram } from "../../util/metrics.js";
@@ -196,9 +196,9 @@ async function probeUsageExhausted(
  * recursion). Returns `undefined` otherwise; the caller falls through
  * to its normal classify/throw path.
  *
- * The retry side-effects are confined here: session reset, transient
- * `setChatModel` flip (restored in `finally`), `_retried = true` on the
- * recursive call.
+ * The retry side-effects are confined here: session reset, passing the
+ * fallback model via params.model to avoid mutating chat settings,
+ * `_retried = true` on the recursive call.
  */
 async function maybeFallbackForChatGptMismatch(
   probeText: string,
@@ -281,13 +281,7 @@ async function maybeFallbackForChatGptMismatch(
         : ``),
   );
   resetSession(chatId);
-  const originalModel = getChatSettings(chatId).model;
-  setChatModel(chatId, fallbackModel);
-  try {
-    return await handleMessage(params, true);
-  } finally {
-    setChatModel(chatId, originalModel);
-  }
+  return await handleMessage({ ...params, model: fallbackModel }, true);
 }
 
 // ── Active session registry ─────────────────────────────────────────────────

src/backend/openai-agents/handler.ts

@@ -44,7 +44,7 @@ import {
   setSessionName,
   resetSession,
 } from "../../storage/sessions.js";
-import { getChatSettings, setChatModel } from "../../storage/chat-settings.js";
+import { getChatSettings } from "../../storage/chat-settings.js";
 import { classify } from "../../core/errors.js";
 import { log, logError, logWarn } from "../../util/log.js";
 import { traceMessage } from "../../util/trace.js";
@@ -360,13 +360,10 @@ export async function handleMessage(
           `[${chatId}] ${classified.reason}, falling back to ${decision.fallbackModelId}`,
         );
         resetSession(chatId);
-        const originalModel = getChatSettings(chatId).model;
-        setChatModel(chatId, decision.fallbackModelId);
-        try {
-          return await handleMessage(params, true);
-        } finally {
-          setChatModel(chatId, originalModel);
-        }
+        return handleMessage(
+          { ...params, model: decision.fallbackModelId },
+          true,
+        );
       }
 
       logError(
@@ -426,15 +423,22 @@ export async function handleMessage(
           trailingText: streamState.lastTrailingText,
           turnTerminated: streamState.turnTerminated,
           deliveredTextNorms: streamState.deliveredTextNorms,
+          toolCalls: streamState.toolCalls,
           retried: _retried,
+          // retryCount must be passed explicitly — otherwise the computed
+          // default of `retried ? 1 : 0` stays at 1 on every recursive
+          // call, making shouldRetry always `1 < maxRetries = true` and
+          // producing an infinite retry loop. Cap at 1 (single-pass).
+          retryCount: _retried ? 1 : 0,
+          maxRetries: 1,
         })
       : ({ violated: false } as const);
 
   if (violation.violated) {
     incrementCounter("scratchpad.trailing_text_dropped");
     log(
       "agent",
-      `[${chatId}] flow violation: trailing prose (${violation.trailing.length} chars) without end_turn/send. ${
+      `[${chatId}] flow violation: ${violation.reason} without end_turn/send. ${
         violation.shouldRetry
           ? "Re-prompting with reminder."
           : "Already retried — accepting silent drop."

src/backend/remote-server/events.ts

@@ -105,7 +105,11 @@ export async function processStreamEvent(
   // Scope to our session only
   const evtSessionID =
     typeof props.sessionID === "string" ? props.sessionID : undefined;
-  if (evtSessionID && evtSessionID !== ctx.sessionId) {
+  // Use !== undefined rather than truthiness so an event that carries an
+  // explicit sessionID for a different session is always filtered out.
+  // A falsy check would treat sessionID="" as "no scope" and let
+  // session.turn.close events with a blank sessionID stop any loop.
+  if (evtSessionID !== undefined && evtSessionID !== ctx.sessionId) {
     return { kind: "stop", reason: "out_of_scope" };
   }
 

src/backend/remote-server/session-helpers.ts

@@ -49,6 +49,8 @@ export const REMOTE_SESSION_MESSAGE_LIMIT = 5000;
  * structural type.
  */
 export interface RemoteAssistantInfo {
+  /** Upstream-assigned message identifier, used for deduplication. */
+  id?: string;
   role?: string;
   finish?: string;
   time?: {
@@ -342,10 +344,8 @@ export async function listSessionMessages(
   const seenMessageIds = new Set<string>();
 
   for (const message of page) {
-    const messageId = (message as Record<string, unknown>)?.info as
-      | { id?: string }
-      | undefined;
-    const id = messageId?.id;
+    const info = (message as { info?: RemoteAssistantInfo })?.info;
+    const id = info?.id;
     if (id && seenMessageIds.has(id)) continue;
     if (id) seenMessageIds.add(id);
     messages.push(message);

src/backend/shared/handle-retry.ts

@@ -26,7 +26,6 @@ import type { QueryParams, QueryResult } from "../../core/types.js";
 import { classify, type TalonError } from "../../core/errors.js";
 import { logWarn } from "../../util/log.js";
 import { incrementCounter } from "../../util/metrics.js";
-import { getChatSettings, setChatModel } from "../../storage/chat-settings.js";
 import { resetSession } from "../../storage/sessions.js";
 import { classifyRetry } from "./model-retry.js";
 
@@ -128,13 +127,20 @@ export async function applyRetryDecision(
       `[${chatId}] ${classified.reason}, falling back to ${decision.fallbackModelId}`,
     );
     resetSession(chatId);
-    const originalModel = getChatSettings(chatId).model;
-    setChatModel(chatId, decision.fallbackModelId);
-    try {
-      return { retry: await recurseWithRetried(params), classified };
-    } finally {
-      setChatModel(chatId, originalModel);
-    }
+    // Pass the fallback model via params.model rather than mutating chat
+    // settings. Mutating settings with setChatModel then restoring via
+    // getChatSettings(chatId).model was broken: after migrateLegacyModelField
+    // runs, settings.model is undefined, so the restore call became
+    // setChatModel(chatId, undefined), which permanently wipes the entire
+    // modelByBackend map. Injecting via params.model is purely in-memory
+    // and leaves chat settings untouched.
+    return {
+      retry: await recurseWithRetried({
+        ...params,
+        model: decision.fallbackModelId,
+      }),
+      classified,
+    };
   }
 
   // `propagate` — caller throws `classified`.