diff --git a/apps/web/components/diff/use-expandable-diff.test.ts b/apps/web/components/diff/use-expandable-diff.test.ts index b4000ca51b..d2959a07d7 100644 --- a/apps/web/components/diff/use-expandable-diff.test.ts +++ b/apps/web/components/diff/use-expandable-diff.test.ts @@ -168,3 +168,64 @@ describe("useExpandableDiff", () => { expect(result.current.metadata).toBeNull(); }); }); + +describe("useExpandableDiff — content-load retries", () => { + it("retries a transient WebSocket timeout and succeeds on a later attempt", async () => { + // A single 5s WS timeout used to set a terminal error and permanently + // disable expansion for this mount (the auto-load effect only fires while + // there is no error). The load now retries transient failures, so a brief + // backend stall recovers instead of killing expansion. + requestFileContentMock + .mockRejectedValueOnce(new Error("WebSocket request timed out: workspace.file.get")) + .mockResolvedValue({ content: "new", is_binary: false }); + requestFileContentAtRefMock.mockResolvedValue({ content: "old", is_binary: false }); + const reparsed = consistentMeta(); + processFileMock.mockReturnValue(reparsed); + + const { result } = renderHook(() => + useExpandableDiff({ ...baseProps, fileDiffMetadata: PARTIAL }), + ); + await loadAndSettle(result.current.loadContent); + + expect(requestFileContentMock).toHaveBeenCalledTimes(2); + expect(result.current.error).toBeNull(); + expect(result.current.metadata).toBe(reparsed); + expect(result.current.isContentLoaded).toBe(true); + }); + + it("does not retry a permanent error and falls back to the partial metadata", async () => { + // Binary files can never be expanded; retrying would just spin. The load + // surfaces the error on the first attempt and keeps the partial metadata. + requestFileContentMock.mockRejectedValue(new Error("Cannot expand binary files")); + requestFileContentAtRefMock.mockResolvedValue({ content: "old", is_binary: false }); + + const { result } = renderHook(() => + useExpandableDiff({ ...baseProps, fileDiffMetadata: PARTIAL }), + ); + await loadAndSettle(result.current.loadContent); + + expect(requestFileContentMock).toHaveBeenCalledTimes(1); + expect(result.current.error).toBe("Cannot expand binary files"); + expect(result.current.metadata).toBe(PARTIAL); + expect(result.current.canExpand).toBe(false); + }); + + it("surfaces a terminal error after transient retries are exhausted", async () => { + // Four attempts total (initial + 3 retries); when every one times out the + // load gives up with the error so the caller falls back to partial metadata. + requestFileContentMock.mockRejectedValue( + new Error("WebSocket request timed out: workspace.file.get"), + ); + requestFileContentAtRefMock.mockResolvedValue({ content: "old", is_binary: false }); + + const { result } = renderHook(() => + useExpandableDiff({ ...baseProps, fileDiffMetadata: PARTIAL }), + ); + await loadAndSettle(result.current.loadContent); + + expect(requestFileContentMock).toHaveBeenCalledTimes(4); + expect(result.current.error).toContain("timed out"); + expect(result.current.metadata).toBe(PARTIAL); + expect(result.current.canExpand).toBe(false); + }); +}); diff --git a/apps/web/components/diff/use-expandable-diff.ts b/apps/web/components/diff/use-expandable-diff.ts index 7cc40171a9..8ee8026b61 100644 --- a/apps/web/components/diff/use-expandable-diff.ts +++ b/apps/web/components/diff/use-expandable-diff.ts @@ -32,6 +32,34 @@ function isFileNotFoundError(error: string): boolean { return /file not found|not found|no such file|does not exist/i.test(error); } +/** + * Number of extra attempts for a transient content-load failure and the base + * backoff between them. The two file reads go over the WebSocket, which rejects + * with a 5s timeout and no retry of its own; when the backend is briefly slow + * (loaded CI runner, reconnect in flight) a single timeout would otherwise set + * a terminal error and permanently disable expansion for this mount, since the + * auto-load effect only fires while there is no error. Retrying transient + * failures here keeps that from happening. + */ +const CONTENT_LOAD_MAX_RETRIES = 3; +const CONTENT_LOAD_RETRY_BASE_MS = 300; + +/** + * A transient failure is one worth retrying: the WebSocket client not being + * ready yet, or a request that timed out. Permanent failures (binary file, + * genuine backend rejections) are surfaced immediately so we fall back to the + * partial metadata without spinning. + */ +function isTransientLoadError(error: string): boolean { + return /timed out|websocket client not available|not connected|connection (closed|lost)/i.test( + error, + ); +} + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + /** * Validate that a reparsed metadata won't trip @pierre/diffs' iterateOverDiff * trailing-context check (which throws and tears down the renderer). When @@ -114,6 +142,34 @@ async function fetchExpansionContent( return { oldContent, newContent }; } +/** + * Fetch expansion content, retrying transient failures with linear backoff. + * `isCurrent` lets the caller abort the retry loop when the hook's inputs have + * changed (a newer request superseded this one); permanent failures rethrow on + * the first attempt so we don't spin on a binary file or a real backend error. + */ +async function fetchExpansionContentWithRetry( + sessionId: string, + filePath: string, + baseRef: string | undefined, + repo: string | undefined, + isCurrent: () => boolean, +) { + let lastError: unknown; + for (let attempt = 0; attempt <= CONTENT_LOAD_MAX_RETRIES; attempt++) { + try { + return await fetchExpansionContent(sessionId, filePath, baseRef, repo); + } catch (err) { + lastError = err; + const msg = err instanceof Error ? err.message : String(err); + if (!isTransientLoadError(msg) || attempt === CONTENT_LOAD_MAX_RETRIES) throw err; + await delay(CONTENT_LOAD_RETRY_BASE_MS * (attempt + 1)); + if (!isCurrent()) throw err; + } + } + throw lastError; +} + /** * Hook for managing expandable diffs with lazy-loaded file content. * @@ -157,7 +213,13 @@ export function useExpandableDiff({ setError(null); try { - const content = await fetchExpansionContent(sessionId, filePath, baseRef, repo); + const content = await fetchExpansionContentWithRetry( + sessionId, + filePath, + baseRef, + repo, + () => version === requestVersionRef.current, + ); if (version === requestVersionRef.current) setLoadedContent(content); } catch (err) { const msg = err instanceof Error ? err.message : "Failed to load file content"; diff --git a/apps/web/components/task/model-selector.test.ts b/apps/web/components/task/model-selector.test.ts index 7cf9a0587d..448bdd51f3 100644 --- a/apps/web/components/task/model-selector.test.ts +++ b/apps/web/components/task/model-selector.test.ts @@ -4,7 +4,12 @@ import { type ModelSelectorOption, usableConfigOptions, } from "@/components/model-config-selector"; -import type { ConfigOptionEntry } from "@/lib/state/slices/session-runtime/types"; +import { hasCompleteDynamicConfig, requiredConfigKeys } from "@/components/task/model-selector"; +import type { Agent, TaskSession } from "@/lib/types/http"; +import type { + ConfigOptionEntry, + SessionModelEntry, +} from "@/lib/state/slices/session-runtime/types"; function compactTriggerLabel( modelOptions: ModelSelectorOption[], @@ -178,3 +183,99 @@ describe("task model selector compact label", () => { expect(label).toBe(`${providerModelName} / Off`); }); }); + +function makeSession(overrides: Partial = {}): TaskSession { + return { + id: "session-1", + task_id: "task-1", + state: "RUNNING", + started_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + ...overrides, + } as TaskSession; +} + +const flatModelId = "claude-opus-4-8"; + +function flatModelEntries(): SessionModelEntry[] { + return [ + { modelId: flatModelId, name: "Claude Opus 4.8" }, + { modelId: "claude-sonnet-4-8", name: "Claude Sonnet 4.8" }, + ]; +} + +describe("hasCompleteDynamicConfig", () => { + const noAgents: Agent[] = []; + + it("treats a required model key as satisfied by a flat ACP model list", () => { + const session = makeSession({ + metadata: { runtime_config: { config_options: { model: flatModelId } } }, + }); + expect(requiredConfigKeys(session, noAgents)).toEqual(["model"]); + + const hydrated = hasCompleteDynamicConfig( + session, + { currentModelId: flatModelId, models: flatModelEntries(), configOptions: [] }, + noAgents, + ); + + expect(hydrated).toBe(true); + }); + + it("still requires the model config option when there is no flat model list", () => { + const session = makeSession({ + metadata: { runtime_config: { config_options: { model: providerModelId } } }, + }); + + const hydrated = hasCompleteDynamicConfig( + session, + { currentModelId: providerModelId, models: [], configOptions: [] }, + noAgents, + ); + + expect(hydrated).toBe(false); + }); + + it("hydrates a config-option agent once the model option is present", () => { + const session = makeSession({ + metadata: { runtime_config: { config_options: { model: providerModelId } } }, + }); + + const modelOption: ConfigOptionEntry = { + type: "select", + id: "model", + name: "Model", + currentValue: providerModelId, + category: "model", + options: [{ value: providerModelId, name: providerModelName }], + }; + + const hydrated = hasCompleteDynamicConfig( + session, + { currentModelId: providerModelId, models: [], configOptions: [modelOption] }, + noAgents, + ); + + expect(hydrated).toBe(true); + }); + + it("does not let a flat model list satisfy a non-model required key", () => { + const session = makeSession({ + metadata: { + runtime_config: { config_options: { model: flatModelId, reasoning_effort: "high" } }, + }, + }); + + const hydrated = hasCompleteDynamicConfig( + session, + { currentModelId: flatModelId, models: flatModelEntries(), configOptions: [] }, + noAgents, + ); + + expect(hydrated).toBe(false); + }); + + it("is hydrated when there are no required config keys", () => { + expect(hasCompleteDynamicConfig(makeSession(), undefined, noAgents)).toBe(true); + }); +}); diff --git a/apps/web/components/task/model-selector.tsx b/apps/web/components/task/model-selector.tsx index 118456d4e3..4e05030808 100644 --- a/apps/web/components/task/model-selector.tsx +++ b/apps/web/components/task/model-selector.tsx @@ -12,6 +12,7 @@ import { } from "@/components/model-config-selector"; import { useAppStore } from "@/components/state-provider"; import { useToast } from "@/components/toast-provider"; +import { createDebugLogger, isDebug } from "@/lib/debug/log"; import { useAvailableAgents } from "@/hooks/domains/settings/use-available-agents"; import { useSettingsData } from "@/hooks/domains/settings/use-settings-data"; import { setSessionConfigOption, setSessionModel } from "@/lib/api/domains/session-api"; @@ -33,6 +34,8 @@ type ModelSelectorProps = { triggerClassName?: string; }; +const debug = createDebugLogger("model-selector:gate"); + function configValueKeys(value: unknown): string[] { if (!value || typeof value !== "object" || Array.isArray(value)) return []; return Object.entries(value) @@ -40,7 +43,9 @@ function configValueKeys(value: unknown): string[] { .map(([key]) => key); } -function requiredConfigKeys(session: TaskSession | null, agents: Agent[]): string[] { +const MODEL_CONFIG_KEY = "model"; + +export function requiredConfigKeys(session: TaskSession | null, agents: Agent[]): string[] { if (!session) return []; const keys = new Set(configValueKeys(session.agent_profile_snapshot?.config_options)); for (const agent of agents) { @@ -59,7 +64,7 @@ function requiredConfigKeys(session: TaskSession | null, agents: Agent[]): strin return [...keys]; } -function hasCompleteDynamicConfig( +export function hasCompleteDynamicConfig( session: TaskSession | null, sessionModelsData: SessionModelsEntry | undefined, agents: Agent[], @@ -68,7 +73,16 @@ function hasCompleteDynamicConfig( if (required.length === 0) return true; if (!sessionModelsData) return false; const available = new Set(sessionModelsData.configOptions.map((option) => option.id)); - return required.every((key) => available.has(key)); + // Flat-model-list agents (e.g. claude-opus) expose their models via the ACP + // top-level `models` list and switch with session/set_model rather than a + // SessionConfigOption(category="model"). For those the persisted runtime + // config still records a "model" key, but it will never appear in + // configOptions. Treat that key as satisfied when the session has a flat model + // list — the selector renders fine from it. + const hasFlatModelList = !!sessionModelsData.models.length; + return required.every( + (key) => available.has(key) || (key === MODEL_CONFIG_KEY && hasFlatModelList), + ); } function resolveSessionState( @@ -338,6 +352,8 @@ function useModelSelectorState(sessionId: string | null) { configOptions, configBaseline: sessionModelsData?.configBaseline, configHydrated: hasCompleteDynamicConfig(session, sessionModelsData, settingsAgents as Agent[]), + requiredKeys: requiredConfigKeys(session, settingsAgents as Agent[]), + rawConfigOptionIds: (sessionModelsData?.configOptions ?? []).map((o) => o.id), handleModelChange, handleConfigChange, }; @@ -353,6 +369,8 @@ export const ModelSelector = memo(function ModelSelector({ configOptions, configBaseline, configHydrated, + requiredKeys, + rawConfigOptionIds, handleModelChange, handleConfigChange, } = useModelSelectorState(sessionId); @@ -374,6 +392,19 @@ export const ModelSelector = memo(function ModelSelector({ [sessionId, handleConfigChange], ); + if (isDebug()) { + debug("render", { + sessionId: sessionId ?? "", + configHydrated, + currentModel: currentModel ?? "", + hasModelConfig: !!modelConfig, + modelOptionsLen: modelOptions.length, + configOptionIds: configOptions.map((o) => o.id), + rawConfigOptionIds, + requiredKeys, + willHide: !sessionId || !configHydrated || (!currentModel && !modelConfig), + }); + } if (!sessionId || !configHydrated || (!currentModel && !modelConfig)) return null; return ( diff --git a/apps/web/e2e/tests/docker/storage-maintenance.spec.ts b/apps/web/e2e/tests/docker/storage-maintenance.spec.ts index 2acabd0274..47cbee465b 100644 --- a/apps/web/e2e/tests/docker/storage-maintenance.spec.ts +++ b/apps/web/e2e/tests/docker/storage-maintenance.spec.ts @@ -1,6 +1,6 @@ import { execFileSync } from "node:child_process"; import { test, expect } from "../../fixtures/docker-test-base"; -import { E2E_IMAGE_TAG } from "../../fixtures/docker-probe"; +import { E2E_IMAGE_TAG, removeKandevContainers } from "../../fixtures/docker-probe"; import { dockerInspectExists, dockerRemove } from "../../helpers/docker"; function createStoppedContainer(labels: string[]): string { @@ -17,6 +17,14 @@ test("removes only stopped Kandev-labeled containers and gates daemon-wide clean apiClient, seedData, }) => { + // This test asserts the *global* count of kandev.managed=true containers the + // daemon reports ("2 managed containers"). The containers project shares one + // Docker daemon across all specs in the worker and only sweeps managed + // containers at worker teardown, so a sibling Docker-executor spec (or one of + // its retries) can leave managed containers behind and inflate the count — + // the flake seen in CI was "5 managed containers" instead of 2. Sweep any + // stray managed containers first so this test counts only its own fixtures. + removeKandevContainers(); const activeTask = await apiClient.createTask(seedData.workspaceId, "Retain active container", { workflow_id: seedData.workflowId, workflow_step_id: seedData.startStepId, diff --git a/apps/web/e2e/tests/git/diff-expansion.spec.ts b/apps/web/e2e/tests/git/diff-expansion.spec.ts index 6491fac66b..1c410ab07e 100644 --- a/apps/web/e2e/tests/git/diff-expansion.spec.ts +++ b/apps/web/e2e/tests/git/diff-expansion.spec.ts @@ -325,33 +325,46 @@ test.describe("Diff expansion — Pierre Diffs provider", () => { await openChangesTab(testPage); await openExpansionFileDiff(testPage); - await waitForDiffText(testPage, "HUNK_TOP", 15_000); + // Use the default (60s) waitForDiffText timeout, not a tight 15s: on a cold + // CI runner the first diff in a shard pays a 30-40s Shiki/regex-engine JIT + // (see the "renders Pierre Diffs viewer" test) before any diff text renders. + await waitForDiffText(testPage, "HUNK_TOP"); - // Wait for expand buttons to appear in the shadow DOM. They load - // asynchronously after full file content is fetched via WebSocket. - await testPage.waitForFunction( - () => { - const container = document.querySelector("diffs-container"); - const shadow = container?.shadowRoot; - if (!shadow) return false; - return shadow.querySelectorAll("[data-expand-button]").length >= 3; - }, - null, - { timeout: 20_000 }, - ); - - // Click the middle separator's expand-up button to reveal lines from the - // top of the collapsed gap. The middle separator is the only one without - // data-separator-first or data-separator-last attributes. - await testPage.evaluate(() => { - const container = document.querySelector("diffs-container"); - const shadow = container!.shadowRoot!; - const sel = - "[data-separator='line-info']:not([data-separator-first]):not([data-separator-last])"; - const btn = shadow.querySelector(`${sel} [data-expand-up]`); - if (!btn) throw new Error("Middle separator expand-up button not found"); - btn.click(); - }); + // Expand controls render in two async phases: the separators appear once the + // patch is parsed, but their expand buttons are injected only after the full + // file content arrives over WebSocket and @pierre/diffs re-parses it (see + // components/diff/use-expandable-diff.ts). Waiting for a *count* of + // [data-expand-button] elements is insufficient — three buttons can exist + // while the middle separator's [data-expand-up] specifically has not been + // injected yet, which is the flake that failed CI with "Middle separator + // expand-up button not found". The re-parse can also replace the shadow + // subtree, so a separate wait-then-click leaves a window where the button + // vanishes between the wait passing and the click firing. Find the button + // and click it in a single browser evaluation, retried by expect.poll, so + // the lookup and the click are atomic per attempt and the click is only + // considered done once the collapsed lines are revealed. The timeout is + // generous because the WS content fetch + reparse is the slowest phase under + // a loaded runner; the 120s per-test budget leaves ample headroom. + const middleExpandUpSelector = + "[data-separator='line-info']:not([data-separator-first]):not([data-separator-last]) [data-expand-up]"; + await expect + .poll( + () => + testPage.evaluate((selector) => { + const container = document.querySelector("diffs-container"); + const shadow = container?.shadowRoot; + if (!shadow) return false; + // Line 60 sits within the first 20 lines revealed by expanding from + // the top of the gap; if it is already present the click landed. + if (shadow.textContent?.includes("original_060")) return true; + const btn = shadow.querySelector(selector); + if (!btn) return false; + btn.click(); + return shadow.textContent?.includes("original_060") ?? false; + }, middleExpandUpSelector), + { timeout: 40_000 }, + ) + .toBe(true); // Line 60 is within the first 20 lines revealed by expanding from the top hunk. await expect(testPage.getByText("original_060", { exact: false })).toBeVisible({ diff --git a/apps/web/lib/debug/log.ts b/apps/web/lib/debug/log.ts index 85541f8a67..da5aafe4a8 100644 --- a/apps/web/lib/debug/log.ts +++ b/apps/web/lib/debug/log.ts @@ -200,6 +200,19 @@ * fetch.success/error means the PR fetch never * fired (look at useActiveTaskPR plumbing). * + * Model selector pipeline (bug: selector shows then disappears on relaunch) + * [model-selector:ws] session.models_updated handler — payload vs. + * existing entry, isEmpty/populated guard, the + * stale-empty skip decision (willSkip), and the + * partial-update config-options preservation + * decision (preserveConfigOptions). + * [model-selector:gate] ModelSelector render gate — configHydrated, + * currentModel, hasModelConfig, configOptionIds + * (usable/filtered), rawConfigOptionIds (unfiltered + * store entry), requiredKeys (keys the session's + * profile/runtime config demand), and willHide (the + * exact gate expression). + * * Other * [ws:connection] WS hook mount + status transitions * [dockview:*] layout restore / save / env-switch / session-tabs / task-select diff --git a/apps/web/lib/state/hydration/hydrator.test.ts b/apps/web/lib/state/hydration/hydrator.test.ts index 3bfcdb9586..92284c3485 100644 --- a/apps/web/lib/state/hydration/hydrator.test.ts +++ b/apps/web/lib/state/hydration/hydrator.test.ts @@ -5,6 +5,7 @@ import { hydrateState, hydrateUI } from "./hydrator"; import { defaultUIState } from "@/lib/state/slices/ui/ui-slice"; import { defaultState, mergeInitialState } from "@/lib/state/default-state"; import type { AppState } from "@/lib/state/store"; +import type { MCPAttachmentHistory } from "@/lib/state/slices/session-runtime/types"; function makeDraft(): AppState { // hydrateUI only touches UI-slice fields; an empty object cast satisfies @@ -347,6 +348,100 @@ describe("hydrateState — sidebar views from user settings", () => { }); }); +describe("hydrateState — session runtime model state", () => { + const sessionId = "session-1"; + const modelId = "claude-opus-4-8"; + const liveModelId = "live-model"; + + it("force-merges sessionModels over live state so the selector survives resume", () => { + const result = produce(makeAppDraft(), (draft: Draft) => { + draft.sessionModels.bySessionId[sessionId] = { + currentModelId: liveModelId, + models: [{ modelId: liveModelId, name: "Live" }], + configOptions: [], + }; + hydrateState( + draft, + { + sessionModels: { + bySessionId: { + [sessionId]: { + currentModelId: modelId, + models: [{ modelId, name: "Opus" }], + configOptions: [], + }, + }, + }, + } as unknown as Partial, + { activeSessionId: sessionId, forceMergeSessionId: sessionId }, + ); + }); + + expect(result.sessionModels.bySessionId[sessionId]).toEqual({ + currentModelId: modelId, + models: [{ modelId, name: "Opus" }], + configOptions: [], + }); + }); + + it("force-merges sessionMcpStatus over live state", () => { + const bootHistory: MCPAttachmentHistory = { + version: 1, + current: { + attachment_attempt_id: "attempt-boot", + started_at: "2026-06-11T00:00:00.000Z", + servers: [{ name: "fs", status: "connected" }], + }, + }; + const result = produce(makeAppDraft(), (draft: Draft) => { + draft.sessionMcpStatus.bySessionId[sessionId] = { + version: 1, + current: { + attachment_attempt_id: "attempt-live", + started_at: "2026-06-10T00:00:00.000Z", + servers: [{ name: "stale", status: "failed" }], + }, + }; + hydrateState( + draft, + { + sessionMcpStatus: { bySessionId: { [sessionId]: bootHistory } }, + } as unknown as Partial, + { activeSessionId: sessionId, forceMergeSessionId: sessionId }, + ); + }); + + expect(result.sessionMcpStatus.bySessionId[sessionId]).toEqual(bootHistory); + }); + + it("does not overwrite live model state for the active (non-force-merged) session", () => { + const result = produce(makeAppDraft(), (draft: Draft) => { + draft.sessionModels.bySessionId["active-session"] = { + currentModelId: liveModelId, + models: [], + configOptions: [], + }; + hydrateState( + draft, + { + sessionModels: { + bySessionId: { + "active-session": { + currentModelId: "stale-model", + models: [], + configOptions: [], + }, + }, + }, + } as unknown as Partial, + { activeSessionId: "active-session" }, + ); + }); + + expect(result.sessionModels.bySessionId["active-session"].currentModelId).toBe(liveModelId); + }); +}); + describe("hydrateState — system slice", () => { it("leaves the system slice untouched when the caller supplies no system fields", () => { const result = produce(makeAppDraft(), (draft: Draft) => { diff --git a/apps/web/lib/state/hydration/hydrator.ts b/apps/web/lib/state/hydration/hydrator.ts index 47ce439fc2..6d874e44d2 100644 --- a/apps/web/lib/state/hydration/hydrator.ts +++ b/apps/web/lib/state/hydration/hydrator.ts @@ -177,6 +177,15 @@ function hydrateSessionRuntime( activeSessionId: string | null, forceMergeSessionId: string | null, ): void { + // Merge a `{ bySessionId }`-shaped slice, protecting the active session. + const mergeBySession = (key: keyof AppState & keyof HydrationState): void => { + const source = state[key] as { bySessionId?: Record } | undefined; + if (!source) return; + const target = draft[key] as { bySessionId?: Record } | undefined; + if (!target?.bySessionId) return; + mergeSessionMap(target.bySessionId, source.bySessionId, activeSessionId, forceMergeSessionId); + }; + if (state.terminal) deepMerge(draft.terminal, state.terminal); if (state.shell) { mergeSessionMap( @@ -201,26 +210,14 @@ function hydrateSessionRuntime( forceMergeSessionId, ); } - if (state.contextWindow) { - mergeSessionMap( - draft.contextWindow.bySessionId, - state.contextWindow?.bySessionId, - activeSessionId, - forceMergeSessionId, - ); - } + mergeBySession("contextWindow"); if (state.environmentIdBySessionId) { Object.assign(draft.environmentIdBySessionId, state.environmentIdBySessionId); } + mergeBySession("sessionModels"); + mergeBySession("sessionMcpStatus"); if (state.agents) deepMerge(draft.agents, state.agents); - if (state.prepareProgress) { - mergeSessionMap( - draft.prepareProgress.bySessionId, - state.prepareProgress?.bySessionId, - activeSessionId, - forceMergeSessionId, - ); - } + mergeBySession("prepareProgress"); } /** Hydrate UI slices without overwriting active connection state. */ diff --git a/apps/web/lib/ws/handlers/session-models.test.ts b/apps/web/lib/ws/handlers/session-models.test.ts index 3a7098240d..8fd6e388a3 100644 --- a/apps/web/lib/ws/handlers/session-models.test.ts +++ b/apps/web/lib/ws/handlers/session-models.test.ts @@ -7,6 +7,7 @@ import type { SessionModelsPayload } from "@/lib/types/session-runtime-payloads" import { registerSessionModelsHandlers } from "./session-models"; const providerModelId = "gpt-5.6-sol"; +const providerModelName = "GPT-5.6 Sol"; const reasoningOptionId = "reasoning_effort"; const reasoningOptionName = "Reasoning Effort"; const optionDescription = "Controls how much reasoning the model performs."; @@ -155,6 +156,183 @@ describe("session.models_updated handler", () => { }); }); +describe("session.models_updated stale-empty guard", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("ignores a fully empty update when a populated entry already exists", () => { + const store = makeStore({ + sessionModels: { + bySessionId: { + "session-1": { + currentModelId: "gpt-5.5", + models: [{ modelId: "gpt-5.5", name: "GPT-5.5" }], + configOptions: [], + }, + }, + } as AppState["sessionModels"], + }); + const handler = registerSessionModelsHandlers(store)["session.models_updated"]!; + + handler(makeMessage(makePayload("", { models: [], config_options: [] }))); + + expect(store.getState().setSessionModels).not.toHaveBeenCalled(); + expect(store.getState().sessionModels.bySessionId["session-1"]).toEqual({ + currentModelId: "gpt-5.5", + models: [{ modelId: "gpt-5.5", name: "GPT-5.5" }], + configOptions: [], + }); + }); + + it("applies an empty update when no entry exists yet", () => { + const store = makeStore(); + const handler = registerSessionModelsHandlers(store)["session.models_updated"]!; + + handler(makeMessage(makePayload("", { models: [], config_options: [] }))); + + expect(store.getState().setSessionModels).toHaveBeenCalledWith( + "session-1", + expect.objectContaining({ currentModelId: "" }), + ); + }); +}); + +describe("session.models_updated stale-empty guard — payload-derived classification", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("ignores an empty update even when persisted runtime metadata carries a model", () => { + const store = makeStore({ + taskSessions: { + items: { + "session-1": makeTaskSession({ + runtime_config: { model: providerModelId }, + }), + }, + }, + sessionModels: { + bySessionId: { + "session-1": { + currentModelId: providerModelId, + models: [{ modelId: providerModelId, name: providerModelName }], + configOptions: [], + }, + }, + } as AppState["sessionModels"], + }); + const handler = registerSessionModelsHandlers(store)["session.models_updated"]!; + + handler(makeMessage(makePayload("", { models: [], config_options: [] }))); + + expect(store.getState().setSessionModels).not.toHaveBeenCalled(); + expect(store.getState().sessionModels.bySessionId["session-1"].currentModelId).toBe( + providerModelId, + ); + }); + + it("ignores a fully empty update when only config options are populated", () => { + const configOptions = [ + { + type: "select", + id: "model", + name: "Model", + currentValue: providerModelId, + options: [{ value: providerModelId, name: providerModelName }], + }, + ]; + const store = makeStore({ + sessionModels: { + bySessionId: { + "session-1": { + currentModelId: "", + models: [], + configOptions, + }, + }, + } as AppState["sessionModels"], + }); + const handler = registerSessionModelsHandlers(store)["session.models_updated"]!; + + handler(makeMessage(makePayload("", { models: [], config_options: [] }))); + + expect(store.getState().setSessionModels).not.toHaveBeenCalled(); + expect(store.getState().sessionModels.bySessionId["session-1"].configOptions).toEqual( + configOptions, + ); + }); +}); + +describe("session.models_updated stale-empty guard — config preservation", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("still applies a populated update over an existing entry", () => { + const store = makeStore({ + sessionModels: { + bySessionId: { + "session-1": { + currentModelId: "gpt-5.5", + models: [{ modelId: "gpt-5.5", name: "GPT-5.5" }], + configOptions: [], + }, + }, + } as AppState["sessionModels"], + }); + const handler = registerSessionModelsHandlers(store)["session.models_updated"]!; + + handler(makeMessage(makePayload("gpt-5.6-sol"))); + + expect(store.getState().setSessionModels).toHaveBeenCalledWith( + "session-1", + expect.objectContaining({ currentModelId: "gpt-5.6-sol" }), + ); + }); + + it("preserves populated config options when a partial relaunch update drops them", () => { + const existingConfigOptions = [ + { + type: "select", + id: "model", + name: "Model", + currentValue: providerModelId, + options: [{ value: providerModelId, name: providerModelName }], + }, + ]; + const store = makeStore({ + sessionModels: { + bySessionId: { + "session-1": { + currentModelId: providerModelId, + models: [{ modelId: providerModelId, name: providerModelName }], + configOptions: existingConfigOptions, + }, + }, + } as AppState["sessionModels"], + }); + const handler = registerSessionModelsHandlers(store)["session.models_updated"]!; + + handler( + makeMessage( + makePayload(providerModelId, { + models: [{ model_id: providerModelId, name: providerModelName }], + config_options: [], + }), + ), + ); + + expect(store.getState().setSessionModels).toHaveBeenCalledWith( + "session-1", + expect.objectContaining({ configOptions: existingConfigOptions }), + ); + expect(store.getState().sessionModels.bySessionId["session-1"].configOptions).toEqual( + existingConfigOptions, + ); + }); +}); + describe("session.models_updated config metadata", () => { it("stores provider descriptions and the persisted config baseline", () => { const store = makeStore(); diff --git a/apps/web/lib/ws/handlers/session-models.ts b/apps/web/lib/ws/handlers/session-models.ts index 0a685a4b11..1bb26970b8 100644 --- a/apps/web/lib/ws/handlers/session-models.ts +++ b/apps/web/lib/ws/handlers/session-models.ts @@ -2,6 +2,9 @@ import type { StoreApi } from "zustand"; import type { AppState } from "@/lib/state/store"; import type { WsHandlers } from "@/lib/ws/handlers/types"; import type { SessionModelsPayload } from "@/lib/types/backend"; +import { createDebugLogger, isDebug } from "@/lib/debug/log"; + +const debug = createDebugLogger("model-selector:ws"); type SessionModelConfigOption = SessionModelsPayload["config_options"][number]; @@ -80,6 +83,76 @@ function isModelConfigOption(option: SessionModelConfigOption): boolean { return option.id === "model" || option.category === "model"; } +// During an agentctl relaunch (ready -> starting -> ready) the backend can emit +// a transient session.models_updated with no models, no current model, and no +// config options before the fresh agent reports its real capabilities. Since +// setSessionModels is a blind replace, that empty event would wipe the populated +// entry and unmount the model selector until a good event arrives. Skip the +// overwrite when the incoming payload is fully empty but a populated entry +// already exists — the same stale-empty guard used by setSessionCommits. +function isEmptyModelsUpdate( + currentModelId: string, + acpModels: SessionModelsPayload["models"], + configOptions: SessionModelsPayload["config_options"], +): boolean { + return !currentModelId && !acpModels?.length && !configOptions?.length; +} + +function hasPopulatedModels(state: AppState, sessionId: string): boolean { + const existing = state.sessionModels.bySessionId[sessionId]; + return ( + !!existing && + (!!existing.currentModelId || existing.models.length > 0 || existing.configOptions.length > 0) + ); +} + +// During an agentctl relaunch the backend can emit a transient +// session.models_updated that still carries models + a current model (so it is +// not "fully empty") but reports an empty config_options array before the fresh +// agent finishes reporting its dynamic config. Blindly writing that array wipes +// the previously-hydrated config options, which flips the model selector's +// configHydrated gate to false and unmounts it. Preserve the existing populated +// config options (and their baseline) when the payload drops them; a real config +// change always re-sends the non-empty array. +function shouldPreserveExistingConfigOptions( + state: AppState, + sessionId: string, + payload: SessionModelsPayload, +): boolean { + if (payload.config_options?.length) return false; + const existing = state.sessionModels.bySessionId[sessionId]; + return !!existing && existing.configOptions.length > 0; +} + +function debugModelsUpdate( + state: AppState, + sessionId: string, + payload: SessionModelsPayload, + resolved: { + currentModelId: string; + isEmpty: boolean; + populated: boolean; + preserveConfigOptions: boolean; + }, +) { + if (!isDebug()) return; + const existing = state.sessionModels.bySessionId[sessionId]; + debug("models_updated", { + sessionId, + payloadCurrentModelId: payload.current_model_id ?? "", + resolvedCurrentModelId: resolved.currentModelId, + payloadModelsLen: payload.models?.length ?? 0, + payloadConfigOptionIds: (payload.config_options ?? []).map((o) => o.id), + isEmpty: resolved.isEmpty, + existingPopulated: resolved.populated, + existingCurrentModelId: existing?.currentModelId ?? "", + existingModelsLen: existing?.models.length ?? 0, + existingConfigOptionIds: (existing?.configOptions ?? []).map((o) => o.id), + willSkip: resolved.isEmpty && resolved.populated, + preserveConfigOptions: resolved.preserveConfigOptions, + }); +} + function clearStaleContextWindow(state: AppState, sessionId: string, currentModelId: string) { const previousModelId = state.sessionModels.bySessionId[sessionId]?.currentModelId ?? ""; if (previousModelId && currentModelId && previousModelId !== currentModelId) { @@ -116,9 +189,37 @@ export function registerSessionModelsHandlers(store: StoreApi): WsHand const matchesPersisted = payloadMatchesPersistedRuntime(payload, persisted); if (matchesPersisted) hydrated.add(sessionId); const pendingRuntime = matchesPersisted ? {} : persisted; - const currentModelId = pendingRuntime.model || resolveCurrentModelId(payload); + const payloadCurrentModelId = resolveCurrentModelId(payload); + const currentModelId = pendingRuntime.model || payloadCurrentModelId; + // Classify emptiness from the incoming payload alone; persisted metadata + // must not make a genuinely empty relaunch event look populated. + const isEmpty = isEmptyModelsUpdate(payloadCurrentModelId, acpModels, payload.config_options); + const populated = hasPopulatedModels(state, sessionId); + const preserveConfigOptions = shouldPreserveExistingConfigOptions(state, sessionId, payload); + debugModelsUpdate(state, sessionId, payload, { + currentModelId, + isEmpty, + populated, + preserveConfigOptions, + }); + if (isEmpty && populated) { + return; + } clearStaleContextWindow(state, sessionId, currentModelId); + const existingEntry = state.sessionModels.bySessionId[sessionId]; + const configOptions = preserveConfigOptions + ? existingEntry.configOptions + : (payload.config_options ?? []).map((o) => ({ + type: o.type, + id: o.id, + name: o.name, + description: o.description, + currentValue: pendingRuntime.configOptions?.[o.id] ?? o.current_value, + category: o.category, + options: o.options, + })); + state.setSessionModels(sessionId, { currentModelId, models: acpModels.map((m) => ({ @@ -128,15 +229,7 @@ export function registerSessionModelsHandlers(store: StoreApi): WsHand usageMultiplier: m.usage_multiplier, meta: m.meta, })), - configOptions: (payload.config_options ?? []).map((o) => ({ - type: o.type, - id: o.id, - name: o.name, - description: o.description, - currentValue: pendingRuntime.configOptions?.[o.id] ?? o.current_value, - category: o.category, - options: o.options, - })), + configOptions, configBaseline: payload.config_baseline ?? persisted.baseline ??