refactor(agents): address review feedback on resolveFreshModel

liuhao1024 · liuhao1024 · commit fe421174422a · 2026-06-12T20:05:28.000+08:00
- Remove dead code: use resolveFreshModel()! in getAvailableThinkingLevels
  since the early return guarantees this.model is truthy
- Add JSDoc explaining why resolveFreshModel() does not use
  refreshCurrentModelFromRegistry() (mutation vs transient read)
- Add defensive test for registry returning different provider/id
- Add integration test verifying compaction uses fresh contextWindow
diff --git a/src/agents/sessions/agent-session.resolve-fresh-model.test.ts b/src/agents/sessions/agent-session.resolve-fresh-model.test.ts
@@ -92,4 +92,27 @@ describe("resolveFreshModel", () => {
       high: "3000",
     });
   });
+
+  it("returns registry model even when provider/id differ from snapshot", () => {
+    const snapshot = makeModel({ provider: "openai", id: "gpt-4o" });
+    const fresh = makeModel({ provider: "openai", id: "gpt-4o-mini" });
+    const find = vi.fn().mockReturnValue(fresh);
+
+    const result = resolveFreshModel(snapshot, find);
+    expect(result).toBe(fresh);
+    expect(result?.id).toBe("gpt-4o-mini");
+  });
+
+  it("uses fresh contextWindow for compaction threshold check", () => {
+    const snapshot = makeModel({ contextWindow: 200_000 });
+    const fresh = makeModel({ contextWindow: 1_000_000 });
+    const find = vi.fn().mockReturnValue(fresh);
+
+    const resolved = resolveFreshModel(snapshot, find);
+    const contextWindow = resolved?.contextWindow ?? 0;
+
+    const usageRatio = 180_000 / contextWindow;
+    expect(contextWindow).toBe(1_000_000);
+    expect(usageRatio).toBeLessThan(0.8);
+  });
 });
diff --git a/src/agents/sessions/agent-session.ts b/src/agents/sessions/agent-session.ts
@@ -1722,7 +1722,7 @@ export class AgentSession {
     if (!this.model) {
       return THINKING_LEVELS;
     }
-    return getSupportedThinkingLevels(this.resolveFreshModel() ?? this.model) as ThinkingLevel[];
+    return getSupportedThinkingLevels(this.resolveFreshModel()!) as ThinkingLevel[];
   }
 
   /**
@@ -2265,6 +2265,11 @@ export class AgentSession {
    * Falls back to the snapshot if the registry has no entry for the current model.
    * Ensures post-turn reads (contextWindow, reasoning, thinkingBudgets) reflect
    * the latest registry state after a /model switch.
+   *
+   * Why not call refreshCurrentModelFromRegistry() instead?
+   * That method mutates agent.state.model — a persistent side effect.
+   * Post-turn reads should not change the snapshot mid-turn.
+   * This helper only does a transient read from the registry.
    */
   private resolveFreshModel(): Model | undefined {
     const current = this.model;
