diff --git a/LAWS/CHAT.md b/LAWS/CHAT.md index 917e91747..6687c4345 100644 --- a/LAWS/CHAT.md +++ b/LAWS/CHAT.md @@ -37,3 +37,9 @@ - A session's subagent activity MUST appear in the chat transcript with the subagent identity when known. - A session's subagent activity MUST appear in the chat transcript with the delegated task when known. + +## Session configuration + +- A session’s provider MUST support its model, and its harness MUST support that provider. +- A session MUST have exactly one effective configuration. +- Berd MUST show the configuration that the session uses. diff --git a/src/app/AppShell.berdctl.test.tsx b/src/app/AppShell.berdctl.test.tsx index f85f08521..8cc92f52b 100644 --- a/src/app/AppShell.berdctl.test.tsx +++ b/src/app/AppShell.berdctl.test.tsx @@ -80,6 +80,7 @@ vi.mock("@/app/views/NavigationPanesView", () => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpCreateSession: (...args: unknown[]) => mockAcpCreateSession(...args), acpListSessionsPage: (...args: unknown[]) => mockAcpListSessionsPage(...args), acpLoadSession: (...args: unknown[]) => mockAcpLoadSession(...args), diff --git a/src/app/AppShell.navigation.test.tsx b/src/app/AppShell.navigation.test.tsx index 095c4cb48..b408936d8 100644 --- a/src/app/AppShell.navigation.test.tsx +++ b/src/app/AppShell.navigation.test.tsx @@ -317,6 +317,20 @@ function seedProviderModels( ], ]), ); + // Simulate a successful live inventory response: seeding a provider's + // display candidates alone is advisory and never establishes proof. + useProviderModelCacheStore.setState((state) => { + const providers = new Map(state.providers); + const existing = providers.get(providerId); + if (existing) { + providers.set(providerId, { + ...existing, + provenModelIds: models.map((model) => model.id), + fetchedAt: Date.now(), + }); + } + return { providers }; + }); } vi.mock("@/shared/profile/buildProfile", () => ({ @@ -440,6 +454,7 @@ vi.mock("@/shared/api/acp", () => ({ acpListSessionsPage: (...args: unknown[]) => mockAcpListSessionsPage(...args), acpLoadSession: (...args: unknown[]) => mockAcpLoadSession(...args), discoverAcpProviders: vi.fn().mockResolvedValue([]), + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), })); vi.mock("@/shared/api/acpApi", () => ({ @@ -1412,6 +1427,7 @@ describe("AppShell global navigation", () => { "openai", "~/goose artifacts", expect.any(Object), + expect.objectContaining({ clear: expect.any(Function) }), ); }); expect( @@ -1460,9 +1476,12 @@ describe("AppShell global navigation", () => { expect(mockAcpCreateSession).toHaveBeenCalled(); }); - it("allows a configured concrete provider when the BYO default is missing", async () => { + it("allows a configured concrete target when the BYO default is missing", async () => { requireByoDefaultProviderSetup(); - setResolvingPersona(); + setResolvingPersona("goose-gpt-5-5", "databricks_v2", "databricks_v2"); + seedProviderModels("databricks_v2", [ + { id: "goose-gpt-5-5", name: "GPT-5.5" }, + ]); mockCheckAllProviderStatus.mockResolvedValue([ { providerId: "databricks_v2", isConfigured: true }, ]); @@ -1481,7 +1500,7 @@ describe("AppShell global navigation", () => { "~/goose artifacts", { deferProviderSetup: false, - modelId: undefined, + modelId: "goose-gpt-5-5", projectId: undefined, }, ); @@ -3402,6 +3421,39 @@ describe("AppShell global navigation", () => { }); }); + it("promotes a managed provider and model resolved during draft creation", async () => { + mockAcpCreateSession.mockResolvedValueOnce({ + sessionId: "created-session", + configOptionsSnapshot: { + model: { modelId: "goose-gpt-5-5", modelName: "GPT-5.5" }, + reasoningEffort: null, + }, + resolvedSelection: { + providerId: "databricks_v2", + modelId: "goose-gpt-5-5", + modelName: "GPT-5.5", + }, + }); + const user = userEvent.setup(); + renderAppShell(); + + await user.click(screen.getByRole("button", { name: "Sidebar new chat" })); + + await waitFor(() => { + expect( + useChatSessionStore.getState().getSession("created-session"), + ).toMatchObject({ + executionTarget: { + harnessId: "goose", + modelProviderId: "databricks_v2", + modelId: "goose-gpt-5-5", + modelName: "GPT-5.5", + }, + }); + }); + expect(mockAcpPrepareSession).not.toHaveBeenCalled(); + }); + it("applies the latest pending draft selection before promotion", async () => { const pendingSession = deferred<{ sessionId: string }>(); const pendingPrepare = deferred>(); @@ -3435,6 +3487,7 @@ describe("AppShell global navigation", () => { "codex-acp", "~/goose artifacts", expect.objectContaining({ modelId: "gpt-5.4-mini" }), + expect.objectContaining({ clear: expect.any(Function) }), ); }); act(() => pendingPrepare.resolve({})); @@ -3463,6 +3516,143 @@ describe("AppShell global navigation", () => { expect(getModelSelectionIntent("created-session")).toBeUndefined(); }); + it("does not promote a stale provider-only draft change over a newer provider", async () => { + const pendingSession = deferred<{ sessionId: string }>(); + const pendingProviderB = deferred>(); + mockAcpCreateSession.mockReturnValueOnce(pendingSession.promise); + mockAcpPrepareSession + .mockReturnValueOnce(pendingProviderB.promise) + .mockResolvedValueOnce({}); + const user = userEvent.setup(); + renderAppShell(); + + await user.click(screen.getByRole("button", { name: "Sidebar new chat" })); + await waitFor(() => expect(mockAcpCreateSession).toHaveBeenCalled()); + const draftSessionId = useChatSessionStore.getState().activeSessionId ?? ""; + + act(() => { + beginModelSelectionIntent(draftSessionId, { + requestId: "provider-b", + target: { harnessId: "codex-acp" }, + }); + pendingSession.resolve({ sessionId: "created-session" }); + }); + + await waitFor(() => { + expect(mockAcpPrepareSession).toHaveBeenCalledWith( + "created-session", + "codex-acp", + "~/goose artifacts", + expect.any(Object), + expect.objectContaining({ clear: expect.any(Function) }), + ); + }); + + act(() => { + beginModelSelectionIntent(draftSessionId, { + requestId: "provider-c", + target: { harnessId: "claude-acp" }, + }); + pendingProviderB.resolve({}); + }); + + await waitFor(() => { + expect(mockAcpPrepareSession).toHaveBeenCalledWith( + "created-session", + "claude-acp", + "~/goose artifacts", + expect.any(Object), + expect.objectContaining({ clear: expect.any(Function) }), + ); + }); + await waitFor(() => { + expect(useChatSessionStore.getState().activeSessionId).toBe( + "created-session", + ); + }); + expect( + useChatSessionStore.getState().getSession("created-session"), + ).toMatchObject({ executionTarget: { harnessId: "claude-acp" } }); + expect(getModelSelectionIntent("created-session")).toBeUndefined(); + }); + + it("does not restore stale draft creation ownership after A to B to A", async () => { + const pendingSession = deferred<{ + sessionId: string; + resolvedSelection: { + providerId: string; + modelId: string; + modelName: string; + }; + }>(); + mockAcpCreateSession.mockReturnValueOnce(pendingSession.promise); + const user = userEvent.setup(); + renderAppShell(); + + await user.click(screen.getByRole("button", { name: "Sidebar new chat" })); + await waitFor(() => expect(mockAcpCreateSession).toHaveBeenCalled()); + const draftSessionId = useChatSessionStore.getState().activeSessionId ?? ""; + const originalTarget = useChatSessionStore + .getState() + .getSession(draftSessionId)?.executionTarget; + expect(originalTarget).toBeDefined(); + if (!originalTarget) { + throw new Error( + "Expected the draft to retain its original execution target", + ); + } + + act(() => { + beginModelSelectionIntent(draftSessionId, { + requestId: "newer-b", + target: { + harnessId: "codex-acp", + modelProviderId: "codex-acp", + modelId: "gpt-5.4-mini", + modelName: "GPT-5.4 mini", + }, + }); + beginModelSelectionIntent(draftSessionId, { + requestId: "newer-a", + target: originalTarget, + preferenceAgentId: originalTarget.harnessId, + }); + pendingSession.resolve({ + sessionId: "created-session", + resolvedSelection: { + providerId: "anthropic", + modelId: "claude-fable", + modelName: "Claude Fable", + }, + }); + }); + + await waitFor(() => { + expect(mockAcpPrepareSession).toHaveBeenCalledWith( + "created-session", + "goose", + "~/goose artifacts", + expect.objectContaining({ selectionAlreadyResolved: true }), + expect.objectContaining({ clear: expect.any(Function) }), + ); + }); + await waitFor(() => { + expect( + useChatSessionStore.getState().getSession("created-session") + ?.executionTarget, + ).toMatchObject(originalTarget); + }); + expect( + useChatSessionStore.getState().getSession("created-session") + ?.executionTarget, + ).not.toMatchObject({ modelId: "claude-fable" }); + await waitFor(() => { + expect(useChatSessionStore.getState().activeSessionId).toBe( + "created-session", + ); + }); + }); + it("adopts a repaired pending draft selection before promotion", async () => { const pendingSession = deferred<{ sessionId: string }>(); mockAcpCreateSession.mockReturnValueOnce(pendingSession.promise); @@ -3497,6 +3687,7 @@ describe("AppShell global navigation", () => { "databricks_v2", "~/goose artifacts", expect.objectContaining({ modelId: "goose-gpt-5-5" }), + expect.objectContaining({ clear: expect.any(Function) }), ); }); await waitFor(() => { @@ -5076,7 +5267,7 @@ describe("AppShell global navigation", () => { }); }); - it("uses the normal new-chat target when a persona has no plausible target", async () => { + it("rejects starting an agent whose saved target is invalid", async () => { useDefaultProviderReadinessStore.setState({ readiness: { status: "ready", @@ -5107,15 +5298,10 @@ describe("AppShell global navigation", () => { ); await waitFor(() => { - expect(mockAcpCreateSession).toHaveBeenCalledWith( - "databricks_v2", - "~/goose artifacts", - expect.objectContaining({ modelId: "goose-default" }), - ); + expect(screen.getByTestId("active-view")).toHaveTextContent("agents"); }); - expect( - useChatSessionStore.getState().getSession("created-session"), - ).toMatchObject({ personaId: "persona-unresolved" }); + expect(mockAcpCreateSession).not.toHaveBeenCalled(); + expect(useChatSessionStore.getState().sessions).toHaveLength(0); }); it("tags a Home agent starter in the composer instead of opening a blank chat", async () => { diff --git a/src/app/AppShell.tsx b/src/app/AppShell.tsx index 4606476c0..1d908efe7 100644 --- a/src/app/AppShell.tsx +++ b/src/app/AppShell.tsx @@ -88,7 +88,7 @@ import { } from "@/features/chat/stores/chatSessionSelectors"; import { useAgentStore } from "@/features/agents/stores/agentStore"; import { useProviderSelection } from "@/features/agents/hooks/useProviderSelection"; -import { personaExecutionTarget } from "@/features/agents/lib/personaExecutionTarget"; +import { resolvePersonaExecutionTarget } from "@/features/agents/lib/personaExecutionTarget"; import { useProjectStore } from "@/features/projects/stores/projectStore"; import { selectProjects } from "@/features/projects/stores/projectSelectors"; import { findExistingDraft } from "@/features/chat/lib/newChat"; @@ -179,7 +179,10 @@ import { } from "@/features/providers/providerCatalog"; import { useProviderModelCacheStore } from "@/features/providers/stores/providerModelCacheStore"; import { getBuildFeatureState } from "@/shared/profile/buildProfile"; -import { gooseServeSelectionFromExecutionTarget } from "@/features/chat/lib/gooseServeExecutionTarget"; +import { + executionTargetFromGooseServeBoundary, + gooseServeSelectionFromExecutionTarget, +} from "@/features/chat/lib/gooseServeExecutionTarget"; import { isModelExecutionTarget, materializeSessionExecutionModel, @@ -1807,6 +1810,9 @@ export function AppShell({ liveDraft, sessionExecutionTarget, ); + const requestedTargetIntentId = getModelSelectionIntent( + session.id, + )?.requestId; const creationSelection = gooseServeSelectionFromExecutionTarget(requestedTarget); return acpCreateSession( @@ -1820,12 +1826,14 @@ export function AppShell({ // the backend session as soon as it exists. deferProviderSetup: false, }, - ).then(({ sessionId, configOptionsSnapshot }) => { + ).then(({ sessionId, configOptionsSnapshot, resolvedSelection }) => { createdBackendSessionId = sessionId; return { sessionId, configOptionsSnapshot, + resolvedSelection, sessionExecutionTarget: requestedTarget, + requestedTargetIntentId, workingDir: resolvedWorkingDir, }; }); @@ -1834,7 +1842,9 @@ export function AppShell({ async ({ sessionId, configOptionsSnapshot, + resolvedSelection, sessionExecutionTarget, + requestedTargetIntentId, workingDir, }) => { const sessionStore = useChatSessionStore.getState(); @@ -1847,7 +1857,26 @@ export function AppShell({ ); return; } - let appliedTarget = sessionExecutionTarget; + const creationTarget = executionTargetFromGooseServeBoundary( + resolvedSelection ?? + gooseServeSelectionFromExecutionTarget(sessionExecutionTarget), + sessionExecutionTarget, + ); + let appliedTarget = creationTarget; + if ( + getModelSelectionIntent(session.id)?.requestId === + requestedTargetIntentId && + sameSessionExecutionTarget( + latestSession.executionTarget, + sessionExecutionTarget, + ) && + !sameSessionExecutionTarget( + creationTarget, + sessionExecutionTarget, + ) + ) { + replaceSessionTargetAfterDispatch(session.id, creationTarget); + } let resolvedConfigOptionsSnapshot = configOptionsSnapshot; const reconcileLatestDraftSelection = async () => { while (true) { @@ -1861,6 +1890,9 @@ export function AppShell({ if (sameSessionExecutionTarget(latestTarget, appliedTarget)) { return latestTarget; } + const latestTargetIntentId = getModelSelectionIntent( + session.id, + )?.requestId; const result = await transitionSessionTarget({ sessionId, target: latestTarget, @@ -1896,6 +1928,12 @@ export function AppShell({ ); } } + if ( + getModelSelectionIntent(session.id)?.requestId === + latestTargetIntentId + ) { + return effectiveTarget; + } appliedTarget = effectiveTarget; } }; @@ -1969,6 +2007,8 @@ export function AppShell({ providerId: pendingSelectionIntent.target.modelProviderId, }, ); + } + if (pendingSelectionIntent) { clearCurrentModelSelectionIntent( session.id, pendingSelectionIntent.requestId, @@ -2700,19 +2740,39 @@ export function AppShell({ const persona = agentState.personas.find( (candidate) => candidate.id === agentId, ); - const cachedModels = [ - ...useProviderModelCacheStore.getState().providers, - ].flatMap(([providerId, entry]) => - entry.models.map((model) => ({ + const modelCache = useProviderModelCacheStore.getState(); + const cachedModels = [...modelCache.providers].flatMap( + ([providerId, entry]) => + entry.models.map((model) => ({ + ...model, + providerId: model.providerId ?? providerId, + })), + ); + const provenModels = [...modelCache.providers].flatMap(([providerId]) => + modelCache.getProvenModelsForProvider(providerId).map((model) => ({ ...model, providerId: model.providerId ?? providerId, })), ); - const executionTarget = personaExecutionTarget(persona, { + const personaResolution = resolvePersonaExecutionTarget(persona, { providers: agentState.providers, models: cachedModels, + getProvenModelsForHarness: () => provenModels, + isModelInventoryAuthoritative: (providerId) => + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative(providerId), catalogEntries: getProviderCatalog(), }); + if (personaResolution.status === "invalid") { + setActiveView("agents"); + setAgentsPersonaId(agentId); + return; + } + const executionTarget = + personaResolution.status === "valid" + ? personaResolution.target + : undefined; void createNewTab(DEFAULT_CHAT_TITLE, undefined, { executionTarget, @@ -5277,6 +5337,7 @@ export function AppShell({ onSend={handleGlobalCompose} onExpand={handleGlobalComposerExpand} onDismiss={dismissCenteredGlobalComposer} + onEditAgent={navigateAgents} onHandoffStart={handleGlobalComposerHandoffStart} placement={globalComposerPlacement} mainLeftOffsetPx={sidebarDockedOuterWidth} diff --git a/src/app/lib/chatRuntimeStartup.test.ts b/src/app/lib/chatRuntimeStartup.test.ts index 3bf339aff..e5b8dc1e2 100644 --- a/src/app/lib/chatRuntimeStartup.test.ts +++ b/src/app/lib/chatRuntimeStartup.test.ts @@ -5,6 +5,18 @@ const mockLoadPersistedMessageQueues = vi.hoisted(() => ); const mockGetClient = vi.hoisted(() => vi.fn<() => Promise>()); const mockRefreshAllModelProviders = vi.hoisted(() => vi.fn()); +const mockMigratePersonaTargetIfUnchanged = vi.hoisted(() => vi.fn()); +const mockAgentState = vi.hoisted(() => ({ + personas: [] as Array>, + providers: [] as Array<{ id: string; label?: string }>, +})); +const mockModelCacheState = vi.hoisted(() => ({ + providers: new Map< + string, + { models: Array>; provenModelIds?: string[] } + >(), + runtimeManagedProviderIds: new Set(), +})); // The latch under test wraps startChatRuntime, whose body touches most of the // startup module graph. Everything it reaches is stubbed inert (resolved, @@ -18,6 +30,7 @@ vi.mock("@/features/agents/stores/agentStore", () => ({ setProviders: () => {}, setPersonas: () => {}, setPersonasLoading: () => {}, + ...mockAgentState, }), }, })); @@ -48,12 +61,15 @@ vi.mock("@/features/providers/runtimeProviderConstraints", () => ({ })); vi.mock("@/features/providers/modelCacheRefresh", () => ({ - getModelCacheRefreshProviderIds: () => [], + getModelCacheRefreshProviderIds: () => ["claude-acp"], })); vi.mock("@/features/providers/providerCatalog", () => ({ + canonicalProviderCatalogIdFromEntries: (_entries: unknown, id: string) => id, getModelProviders: () => [], getProviderCatalog: () => [], + resolveAgentProviderCatalogIdStrictFromEntries: () => null, + resolveModelProviderCatalogIdStrictFromEntries: () => null, })); vi.mock("@/features/providers/runtimeProviderConfig", () => ({ @@ -110,8 +126,7 @@ vi.mock("@/features/providers/stores/defaultProviderReadinessStore", () => ({ vi.mock("@/features/providers/stores/providerModelCacheStore", () => ({ useProviderModelCacheStore: { getState: () => ({ - providers: new Map(), - runtimeManagedProviderIds: new Set(), + ...mockModelCacheState, loadPersisted: () => {}, refreshAllModelProviders: (...args: unknown[]) => mockRefreshAllModelProviders(...args), @@ -166,8 +181,9 @@ vi.mock("@/shared/api/distro", () => ({ })); vi.mock("@/shared/api/agents", () => ({ - listPersonas: async () => [], - migratePersonaTargetIfUnchanged: async () => null, + listPersonas: async () => mockAgentState.personas, + migratePersonaTargetIfUnchanged: (...args: unknown[]) => + mockMigratePersonaTargetIfUnchanged(...args), })); function deferred() { @@ -190,6 +206,12 @@ describe("runChatRuntimeStartup", () => { mockGetClient.mockResolvedValue({}); mockRefreshAllModelProviders.mockReset(); mockRefreshAllModelProviders.mockResolvedValue(undefined); + mockMigratePersonaTargetIfUnchanged.mockReset(); + mockMigratePersonaTargetIfUnchanged.mockResolvedValue(null); + mockAgentState.personas = []; + mockAgentState.providers = []; + mockModelCacheState.providers = new Map(); + mockModelCacheState.runtimeManagedProviderIds = new Set(); }); it("collapses concurrent callers onto one startup run", async () => { @@ -221,6 +243,64 @@ describe("runChatRuntimeStartup", () => { inventoryRefresh.resolve(); }); + it("does not migrate a runtime-managed configuration seed before live discovery", async () => { + mockAgentState.providers = [{ id: "claude-acp", label: "Claude Code" }]; + mockAgentState.personas = [ + { + id: "persona-1", + displayName: "Configured Claude", + systemPrompt: "Help.", + provider: "claude-acp", + modelProviderId: "claude-acp", + model: "configured-model", + isBuiltin: false, + writable: true, + }, + ]; + mockModelCacheState.runtimeManagedProviderIds = new Set(["claude-acp"]); + mockModelCacheState.providers = new Map([ + [ + "claude-acp", + { + models: [{ id: "configured-model", providerId: "claude-acp" }], + }, + ], + ]); + + const { runChatRuntimeStartup } = await import("./chatRuntimeStartup"); + await runChatRuntimeStartup(); + + expect(mockMigratePersonaTargetIfUnchanged).not.toHaveBeenCalled(); + }); + + it("leaves an authoritative unsupported persona persisted for explicit repair", async () => { + mockAgentState.providers = [{ id: "claude-acp", label: "Claude Code" }]; + mockAgentState.personas = [ + { + id: "persona-1", + displayName: "Legacy Claude", + systemPrompt: "Help.", + provider: "claude-acp", + modelProviderId: "openai", + model: "gpt-5", + isBuiltin: false, + writable: true, + }, + ]; + mockModelCacheState.providers = new Map([ + ["claude-acp", { models: [], provenModelIds: [] }], + ]); + mockRefreshAllModelProviders.mockImplementation(async () => { + mockModelCacheState.providers = new Map([ + ["claude-acp", { models: [], provenModelIds: [] }], + ]); + }); + + const { runChatRuntimeStartup } = await import("./chatRuntimeStartup"); + await runChatRuntimeStartup(); + expect(mockMigratePersonaTargetIfUnchanged).not.toHaveBeenCalled(); + }); + it("stays latched after a successful run", async () => { const { runChatRuntimeStartup } = await import("./chatRuntimeStartup"); const first = runChatRuntimeStartup(); diff --git a/src/app/lib/chatRuntimeStartup.ts b/src/app/lib/chatRuntimeStartup.ts index 72c74573b..f42469b18 100644 --- a/src/app/lib/chatRuntimeStartup.ts +++ b/src/app/lib/chatRuntimeStartup.ts @@ -259,15 +259,19 @@ async function startChatRuntime( const cachedModels = [...modelState.providers].flatMap( ([providerId, entry]) => authoritativeProviderIds.has(providerId) - ? entry.models.map((model) => ({ - ...model, - providerId: model.providerId ?? providerId, - })) + ? entry.models + .filter((model) => entry.provenModelIds?.includes(model.id)) + .map((model) => ({ + ...model, + providerId: model.providerId ?? providerId, + })) : [], ); const targetContext = { providers: useAgentStore.getState().providers, models: cachedModels, + isModelInventoryAuthoritative: (providerId: string) => + authoritativeProviderIds.has(providerId), catalogEntries: getProviderCatalog(), }; const personas = useAgentStore.getState().personas; @@ -311,13 +315,14 @@ async function startChatRuntime( ); await modelCacheStore.refreshAllModelProviders(refreshProviderIds); const modelState = useProviderModelCacheStore.getState(); - return new Set([ - ...modelState.runtimeManagedProviderIds, - ...refreshProviderIds.filter((providerId) => { + return new Set( + refreshProviderIds.filter((providerId) => { const entry = modelState.providers.get(providerId); - return entry != null && !entry.error; + return ( + entry != null && !entry.error && entry.provenModelIds !== undefined + ); }), - ]); + ); }; const loadSessionState = async () => { diff --git a/src/app/views/__tests__/NavigationPanesView.test.tsx b/src/app/views/__tests__/NavigationPanesView.test.tsx index d40ffed73..36a84cb16 100644 --- a/src/app/views/__tests__/NavigationPanesView.test.tsx +++ b/src/app/views/__tests__/NavigationPanesView.test.tsx @@ -315,6 +315,7 @@ vi.mock("@/features/chat/stores/chatStore", () => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSearchSessions: (...args: unknown[]) => mockAcpSearchSessions(...args), })); diff --git a/src/features/agents/lib/__tests__/personaExecutionTarget.test.ts b/src/features/agents/lib/__tests__/personaExecutionTarget.test.ts index d5e05acc9..3ae457fba 100644 --- a/src/features/agents/lib/__tests__/personaExecutionTarget.test.ts +++ b/src/features/agents/lib/__tests__/personaExecutionTarget.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from "vitest"; import type { ProviderCatalogEntry } from "@/shared/types/providers"; +import { gooseServeSelectionFromExecutionTarget } from "@/features/chat/lib/gooseServeExecutionTarget"; import { personaExecutionTarget, + resolvePersonaExecutionTarget, personaTargetMigration, } from "../personaExecutionTarget"; @@ -18,12 +20,17 @@ const catalog = (id: string, category: "agent" | "model", aliases?: string[]) => const context = ( models: Array<{ id: string; providerId?: string; displayName?: string }> = [], + authoritativeProviderIds: readonly string[] = [], + provenModels = models, ) => ({ providers: [ { id: "goose", label: "Goose" }, { id: "claude-acp", label: "Claude Code" }, ], models, + getProvenModelsForHarness: () => provenModels, + isModelInventoryAuthoritative: (providerId: string) => + authoritativeProviderIds.includes(providerId), catalogEntries: [ catalog("goose", "agent"), catalog("claude-acp", "agent", ["claude"]), @@ -38,6 +45,24 @@ describe("personaExecutionTarget", () => { expect(personaExecutionTarget({}, context())).toBeUndefined(); }); + it("distinguishes absent, valid, and invalid saved configurations", () => { + expect(resolvePersonaExecutionTarget({}, context())).toEqual({ + status: "absent", + }); + expect( + resolvePersonaExecutionTarget( + { provider: "goose", modelProviderId: "openai", model: "gpt-5" }, + context(), + ), + ).toMatchObject({ status: "valid", target: { modelId: "gpt-5" } }); + expect( + resolvePersonaExecutionTarget( + { provider: "missing-provider", model: "retired" }, + context(), + ), + ).toEqual({ status: "invalid" }); + }); + it("returns the complete saved Goose target without requiring inventory", () => { expect( personaExecutionTarget( @@ -52,6 +77,201 @@ describe("personaExecutionTarget", () => { }); }); + it("does not treat an advisory-only display candidate as live proof", () => { + const persona = { + provider: "goose", + modelProviderId: "openai", + model: "advisory-only", + }; + const targetContext = context( + [{ id: "advisory-only", providerId: "openai" }], + ["openai"], + [], + ); + + expect(personaExecutionTarget(persona, targetContext)).toBeUndefined(); + expect(personaTargetMigration(persona, targetContext)).toBeNull(); + }); + + it("rejects an agent harness persisted as a Goose model provider", () => { + const persona = { + provider: "goose", + modelProviderId: "claude-acp", + model: "sonnet", + }; + + expect(personaExecutionTarget(persona, context())).toBeUndefined(); + expect(personaTargetMigration(persona, context())).toEqual({ + provider: null, + modelProviderId: null, + model: null, + }); + }); + + it("never materializes an agent provider as a Goose target without a model", () => { + const persona = { provider: "goose", modelProviderId: "claude-acp" }; + + expect(personaExecutionTarget(persona, context())).toBeUndefined(); + expect(resolvePersonaExecutionTarget(persona, context())).toEqual({ + status: "invalid", + }); + expect(personaTargetMigration(persona, context())).toEqual({ + provider: null, + modelProviderId: null, + model: null, + }); + }); + + it.each([ + { + name: "Goose canonical provider with a supported model", + persona: { provider: "goose", modelProviderId: "openai", model: "gpt-5" }, + models: [{ id: "gpt-5", providerId: "openai" }], + authoritativeProviderIds: ["openai"], + target: { + harnessId: "goose", + modelProviderId: "openai", + modelId: "gpt-5", + modelName: "gpt-5", + }, + migration: null, + }, + { + name: "Goose canonical provider with an unsupported model", + persona: { provider: "goose", modelProviderId: "openai", model: "gpt-5" }, + models: [], + authoritativeProviderIds: ["openai"], + target: undefined, + migration: null, + }, + { + name: "external harness with foreign provider and supported model", + persona: { + provider: "claude-acp", + modelProviderId: "openai", + model: "sonnet", + }, + models: [{ id: "sonnet", displayName: "Sonnet" }], + authoritativeProviderIds: ["claude-acp"], + target: { + harnessId: "claude-acp", + modelProviderId: "claude-acp", + modelId: "sonnet", + modelName: "Sonnet", + }, + migration: { + provider: "claude-acp", + modelProviderId: "claude-acp", + model: "sonnet", + }, + }, + { + name: "external harness with foreign provider and unsupported model", + persona: { + provider: "claude-acp", + modelProviderId: "openai", + model: "gpt-5", + }, + models: [], + authoritativeProviderIds: ["claude-acp"], + target: undefined, + migration: null, + }, + { + name: "external harness with unavailable inventory", + persona: { + provider: "claude-acp", + modelProviderId: "openai", + model: "gpt-5", + }, + models: [], + authoritativeProviderIds: [], + target: { + harnessId: "claude-acp", + modelProviderId: "claude-acp", + modelId: "gpt-5", + modelName: "gpt-5", + }, + migration: { + provider: "claude-acp", + modelProviderId: "claude-acp", + model: "gpt-5", + }, + }, + ])("canonicalizes $name across persisted target, migration, and wire selection", ({ + persona, + models, + authoritativeProviderIds, + target, + migration, + }) => { + const targetContext = context(models, authoritativeProviderIds); + + expect(personaExecutionTarget(persona, targetContext)).toEqual(target); + expect(personaTargetMigration(persona, targetContext)).toEqual(migration); + const actualTarget = personaExecutionTarget(persona, targetContext); + if (!target) { + expect(gooseServeSelectionFromExecutionTarget(actualTarget)).toEqual({}); + return; + } + const wireProviderId = + target.harnessId === "goose" ? target.modelProviderId : target.harnessId; + expect(gooseServeSelectionFromExecutionTarget(actualTarget)).toEqual({ + providerId: wireProviderId, + modelId: target.modelId, + modelName: target.modelName, + }); + }); + + it("owns an external harness model provider and repairs legacy display metadata", () => { + const persona = { + provider: "claude-acp", + modelProviderId: "openai", + model: "sonnet", + }; + const target = personaExecutionTarget( + persona, + context([{ id: "sonnet", displayName: "Sonnet" }]), + ); + + expect(target).toEqual({ + harnessId: "claude-acp", + modelProviderId: "claude-acp", + modelId: "sonnet", + modelName: "Sonnet", + }); + expect(gooseServeSelectionFromExecutionTarget(target)).toEqual({ + providerId: "claude-acp", + modelId: "sonnet", + modelName: "Sonnet", + }); + expect( + personaTargetMigration( + persona, + context([{ id: "sonnet", displayName: "Sonnet" }]), + ), + ).toEqual({ + provider: "claude-acp", + modelProviderId: "claude-acp", + model: "sonnet", + }); + }); + + it("returns no target for an unknown persisted harness", () => { + const persona = { + provider: "deleted-harness", + modelProviderId: "openai", + model: "gpt-5", + }; + + expect(personaExecutionTarget(persona, context())).toBeUndefined(); + expect(personaTargetMigration(persona, context())).toEqual({ + provider: null, + modelProviderId: null, + model: null, + }); + }); + it("uses an external harness as the runtime provider boundary", () => { expect( personaExecutionTarget( diff --git a/src/features/agents/lib/personaExecutionTarget.ts b/src/features/agents/lib/personaExecutionTarget.ts index 44c0fa251..d3b7832d3 100644 --- a/src/features/agents/lib/personaExecutionTarget.ts +++ b/src/features/agents/lib/personaExecutionTarget.ts @@ -28,6 +28,10 @@ export interface PersonaTargetContext { providers: readonly AvailableHarness[]; models: readonly AvailableModel[]; getModelsForHarness?: (harnessId: string) => readonly AvailableModel[]; + /** Live inventory models, separate from display/advisory candidates. */ + getProvenModelsForHarness?: (harnessId: string) => readonly AvailableModel[]; + /** Whether the model inventory for a provider/harness is authoritative. */ + isModelInventoryAuthoritative?: (providerId: string) => boolean; catalogEntries: ProviderCatalogEntry[]; } @@ -47,7 +51,7 @@ function canonicalModelProviderId( return canonicalProviderCatalogIdFromEntries(catalogEntries, providerId); } -function harnessIdForPersona( +export function personaHarnessId( providerId: string | undefined, providers: readonly AvailableHarness[], catalogEntries: ProviderCatalogEntry[], @@ -75,17 +79,37 @@ function harnessIdForPersona( ); } +function isAgentProviderId( + providerId: string, + catalogEntries: ProviderCatalogEntry[], +): boolean { + return ( + resolveAgentProviderCatalogIdStrictFromEntries( + catalogEntries, + providerId, + ) !== null + ); +} + function persistedModelProviderId( persona: Pick, harnessId: string, catalogEntries: ProviderCatalogEntry[], ): string | undefined { - if (persona.modelProviderId?.trim()) { - return canonicalModelProviderId(persona.modelProviderId, catalogEntries); + // A non-Goose harness is its own provider boundary. Its persisted model + // provider is display metadata from an older representation, never an + // independent provider that may be sent to Goose. + if (harnessId !== "goose") return harnessId; + + const persistedProviderId = persona.modelProviderId?.trim(); + if ( + persistedProviderId && + !isAgentProviderId(persistedProviderId, catalogEntries) + ) { + return canonicalModelProviderId(persistedProviderId, catalogEntries); } if ( persona.provider?.trim() && - harnessId === "goose" && (INTERNAL_DATABRICKS_KEYS.has(normalizeProviderKey(persona.provider)) || resolveModelProviderCatalogIdStrictFromEntries( catalogEntries, @@ -94,14 +118,14 @@ function persistedModelProviderId( ) { return canonicalModelProviderId(persona.provider, catalogEntries); } - return harnessId === "goose" ? undefined : harnessId; + return undefined; } /** * Convert canonical saved agent metadata into PR #1085's runtime target. * An incomplete legacy target is no override; callers must leave chat state alone. */ -export function personaExecutionTarget( +function resolvePersonaExecutionTargetValue( persona: | Pick | null @@ -110,10 +134,12 @@ export function personaExecutionTarget( providers, models, getModelsForHarness, + getProvenModelsForHarness, + isModelInventoryAuthoritative, catalogEntries, }: PersonaTargetContext, ): SessionExecutionTarget | undefined { - const harnessId = harnessIdForPersona( + const harnessId = personaHarnessId( persona?.provider, providers, catalogEntries, @@ -121,7 +147,10 @@ export function personaExecutionTarget( if (!harnessId) return undefined; const availableModels = getModelsForHarness?.(harnessId) ?? models; + const provenModels = + getProvenModelsForHarness?.(harnessId) ?? availableModels; const modelId = normalizeConcreteModelId(persona?.model); + if (!modelId) return undefined; let modelProviderId = persistedModelProviderId( persona ?? {}, harnessId, @@ -131,7 +160,7 @@ export function personaExecutionTarget( // Compatibility read until the migration write completes. if (modelId && !modelProviderId && harnessId === "goose") { const matches = new Set( - availableModels.flatMap((model) => + provenModels.flatMap((model) => model.id === modelId && model.providerId ? [canonicalModelProviderId(model.providerId, catalogEntries)] : [], @@ -151,6 +180,21 @@ export function personaExecutionTarget( canonicalModelProviderId(model.providerId, catalogEntries) === modelProviderId), ); + const provenModel = provenModels.find( + (model) => + model.id === modelId && + (!model.providerId || + canonicalModelProviderId(model.providerId, catalogEntries) === + modelProviderId), + ); + const inventoryIsAuthoritative = + isModelInventoryAuthoritative?.(modelProviderId ?? harnessId) ?? false; + + if (modelId && !provenModel && inventoryIsAuthoritative) { + // Preserve the saved configuration for repair, but do not expose an + // incomplete execution target: agents require both provider and model. + return undefined; + } return normalizeSessionExecutionTarget({ harnessId, @@ -160,6 +204,38 @@ export function personaExecutionTarget( }); } +export type PersonaExecutionResolution = + | { status: "absent"; target?: undefined } + | { status: "invalid"; target?: undefined } + | { status: "valid"; target: SessionExecutionTarget }; + +/** Distinguish legacy absence from saved metadata that cannot be invoked safely. */ +export function resolvePersonaExecutionTarget( + persona: + | Pick + | null + | undefined, + context: PersonaTargetContext, +): PersonaExecutionResolution { + const hasSavedTarget = Boolean( + persona?.provider || persona?.modelProviderId || persona?.model, + ); + if (!hasSavedTarget) return { status: "absent" }; + const target = resolvePersonaExecutionTargetValue(persona, context); + return target ? { status: "valid", target } : { status: "invalid" }; +} + +export function personaExecutionTarget( + persona: + | Pick + | null + | undefined, + context: PersonaTargetContext, +): SessionExecutionTarget | undefined { + const resolution = resolvePersonaExecutionTarget(persona, context); + return resolution.status === "valid" ? resolution.target : undefined; +} + /** * Produce the durable repair for legacy agent metadata after provider inventory * has refreshed. `null` means the saved target is already canonical. @@ -185,15 +261,27 @@ export function personaTargetMigration( ); const unknownHarness = Boolean(persona.provider) && - !harnessIdForPersona( + !personaHarnessId( + persona.provider, + context.providers, + context.catalogEntries, + ); + const persistedAgentProviderForGoose = + personaHarnessId( persona.provider, context.providers, context.catalogEntries, + ) === "goose" && + Boolean( + persona.modelProviderId && + isAgentProviderId(persona.modelProviderId, context.catalogEntries), ); // Clear only when the saved data itself proves it cannot form one target. // No inventory match may be a transient availability problem, so preserve // that legacy metadata until a later authoritative refresh can repair it. - return unknownHarness || matchingProviderIds.size > 1 + return unknownHarness || + persistedAgentProviderForGoose || + matchingProviderIds.size > 1 ? { provider: null, modelProviderId: null, model: null } : null; } diff --git a/src/features/berdctl/__tests__/commands/commands.test.ts b/src/features/berdctl/__tests__/commands/commands.test.ts index 0ae4fa33d..3557caeb6 100644 --- a/src/features/berdctl/__tests__/commands/commands.test.ts +++ b/src/features/berdctl/__tests__/commands/commands.test.ts @@ -15,6 +15,8 @@ import { } from "@/features/berdctl/commands/types"; import { useAgentStore } from "@/features/agents/stores/agentStore"; import { DEFAULT_CHAT_TITLE } from "@/features/chat/lib/sessionTitle"; +import { DEFAULT_RUNTIME_CONFIG } from "@/shared/runtime-config/schema"; +import { useRuntimeConfigStore } from "@/shared/runtime-config/runtimeConfigStore"; import { resetSessionTargetCoordinatorsForTests } from "@/features/chat/lib/sessionTargetCoordinator"; import { applyPendingSessionWorkspaceActivation, @@ -32,6 +34,7 @@ import { DEFAULT_PROJECT_COLOR } from "@/features/projects/lib/projectDefaults"; import { DEFAULT_PROJECT_ICON } from "@/features/projects/lib/projectIcons"; import { useProjectStore } from "@/features/projects/stores/projectStore"; import { getModelProviders } from "@/features/providers/providerCatalog"; +import { useProviderCatalogStore } from "@/features/providers/stores/providerCatalogStore"; import { useProviderModelCacheStore } from "@/features/providers/stores/providerModelCacheStore"; import { setMultiWorkspaceEnabled } from "@/features/workspaces/multiWorkspacePreference"; import { resolveSkillPillTone } from "@/features/skills/lib/resolveSkillPillTone"; @@ -74,6 +77,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpCreateSession: (...args: unknown[]) => mocks.acpCreateSession(...args), acpDuplicateSession: (...args: unknown[]) => mocks.acpDuplicateSession(...args), @@ -289,11 +293,22 @@ function seedModelCache(cacheKey: string, modelIds: string[]): void { providerId: cacheKey, models: modelIds.map((id) => ({ id, name: id })), fetchedAt: Date.now(), + // Simulate a successful live inventory response: proof is what keeps + // this cache entry from being treated as stale and re-fetched. + provenModelIds: modelIds, }); return { providers }; }); } +function seedAuthoritativeGooseInventory( + modelsByProvider: Record = {}, +): void { + for (const provider of getModelProviders()) { + seedModelCache(provider.id, modelsByProvider[provider.id] ?? []); + } +} + /** Fresh-but-empty cache entries for every catalog model provider, so goose * aggregation never triggers a real refresh in tests. */ function emptyModelProviderCache(): Map< @@ -356,10 +371,16 @@ beforeEach(() => { hasFetchedProjects: false, }); useAgentStore.setState({ personas: [], agents: [], activeAgentId: null }); + useProviderCatalogStore.setState({ + entries: useProviderCatalogStore.getInitialState().entries, + }); useProviderModelCacheStore.setState({ providers: emptyModelProviderCache(), refreshingProviderIds: new Set(), }); + useRuntimeConfigStore.setState({ + config: DEFAULT_RUNTIME_CONFIG, + }); window.localStorage.clear(); setMultiWorkspaceEnabled(true); @@ -639,6 +660,9 @@ describe("sessions.create", () => { id: "agent-7", displayName: "Reviewer", systemPrompt: "Review the work carefully.", + provider: "goose", + modelProviderId: "databricks_v2", + model: "model-9", isBuiltin: false, writable: true, }, @@ -700,6 +724,117 @@ describe("sessions.create", () => { expect(controller.openSession).not.toHaveBeenCalled(); }); + it("rejects an unconfigured agent before creating or sending", async () => { + mocks.listPersonas.mockResolvedValue([ + { + id: "empty-agent", + displayName: "Empty", + systemPrompt: "", + isBuiltin: false, + writable: true, + }, + ]); + await expectCommandError( + dispatchCommand( + "sessions", + { action: "create", prompt: "hi", agent_id: "empty-agent" }, + ctx, + ), + "agent_configuration_invalid", + ); + expect(mocks.acpCreateSession).not.toHaveBeenCalled(); + expect(mocks.acpSendMessage).not.toHaveBeenCalled(); + expect(useChatStore.getState().queuedMessageBySession).toEqual({}); + }); + + it("creates a model-free non-agent session without refreshing inventory", async () => { + const refreshProviderModels = vi + .fn() + .mockRejectedValue(new Error("inventory unavailable")); + useProviderModelCacheStore.setState({ refreshProviderModels }); + await dispatchCommand( + "sessions", + { action: "create", prompt: "hi", harness_id: "goose" }, + ctx, + ); + expect(refreshProviderModels).not.toHaveBeenCalled(); + useProviderModelCacheStore.setState({ + refreshProviderModels: + useProviderModelCacheStore.getInitialState().refreshProviderModels, + }); + expect(mocks.acpCreateSession).toHaveBeenCalled(); + }); + + it("keeps a model-only Goose override inside the saved provider", async () => { + useProviderModelCacheStore.setState((state) => ({ + providers: new Map(state.providers) + .set("provider-a", { + providerId: "provider-a", + models: [{ id: "shared", name: "A" }], + fetchedAt: Date.now(), + provenModelIds: ["shared"], + }) + .set("provider-b", { + providerId: "provider-b", + models: [{ id: "shared", name: "B" }], + fetchedAt: Date.now(), + provenModelIds: ["shared"], + }), + })); + mocks.listPersonas.mockResolvedValue([ + { + id: "agent-b", + displayName: "B", + systemPrompt: "", + provider: "goose", + modelProviderId: "provider-b", + model: "old", + isBuiltin: false, + writable: true, + }, + ]); + await dispatchCommand( + "sessions", + { + action: "create", + prompt: "hi", + agent_id: "agent-b", + model_id: "shared", + }, + ctx, + ); + expect(mocks.acpCreateSession).toHaveBeenCalledWith( + "provider-b", + "/resolved/cwd", + expect.objectContaining({ modelId: "shared" }), + ); + }); + + it("rejects an agent with an invalid saved target before creating", async () => { + mocks.listPersonas.mockResolvedValue([ + { + id: "invalid-agent", + displayName: "Invalid Agent", + systemPrompt: "Do not invoke.", + provider: "missing-provider", + model: "retired-model", + isBuiltin: false, + writable: true, + }, + ]); + + await expectCommandError( + dispatchCommand( + "sessions", + { action: "create", prompt: "hi", agent_id: "invalid-agent" }, + ctx, + ), + "agent_configuration_invalid", + ); + + expect(mocks.acpCreateSession).not.toHaveBeenCalled(); + }); + it("rejects an unknown agent before creating the session", async () => { await expectCommandError( dispatchCommand( @@ -735,6 +870,37 @@ describe("sessions.create", () => { ); }); + it("uses a valid agent's saved execution target when no explicit target is passed", async () => { + seedModelCache("databricks_v2", ["agent-model"]); + mocks.listPersonas.mockResolvedValue([ + { + id: "configured-agent", + displayName: "Configured Agent", + systemPrompt: "Use the saved target.", + provider: "goose", + modelProviderId: "databricks_v2", + model: "agent-model", + isBuiltin: false, + writable: true, + }, + ]); + + await dispatchCommand( + "sessions", + { action: "create", prompt: "hi", agent_id: "configured-agent" }, + ctx, + ); + + expect(mocks.acpCreateSession).toHaveBeenCalledWith( + "databricks_v2", + "/resolved/cwd", + expect.objectContaining({ + modelId: "agent-model", + personaId: "configured-agent", + }), + ); + }); + it("rejects an unknown harness with harness_not_found before creating", async () => { const error = await expectCommandError( dispatchCommand( @@ -766,7 +932,7 @@ describe("sessions.create", () => { it("resolves a goose model to its owning model provider", async () => { const modelProvider = getModelProviders()[0].id; - seedModelCache(modelProvider, ["model-a"]); + seedAuthoritativeGooseInventory({ [modelProvider]: ["model-a"] }); await dispatchCommand( "sessions", @@ -791,6 +957,213 @@ describe("sessions.create", () => { ); }); + it("does not infer a bare Goose model while an eligible provider inventory is unavailable", async () => { + const [provenProvider] = getModelProviders(); + useProviderCatalogStore.setState((state) => ({ + entries: [ + ...state.entries, + { + id: "unavailable-provider", + displayName: "Unavailable Provider", + category: "model", + description: "Eligible provider with unavailable inventory", + setupMethod: "single_api_key", + group: "additional", + setupCatalogProvider: true, + }, + ], + })); + seedModelCache(provenProvider.id, ["model-a"]); + + await expectCommandError( + dispatchCommand( + "sessions", + { action: "create", prompt: "hi", model_id: "model-a" }, + ctx, + ), + "model_not_found", + ); + expect(mocks.acpCreateSession).not.toHaveBeenCalled(); + }); + + it("rejects an advisory ACP model excluded by authoritative proof", async () => { + useProviderModelCacheStore.setState((state) => ({ + providers: new Map(state.providers).set("codex-acp", { + providerId: "codex-acp", + models: [ + { id: "proven-model", name: "Proven" }, + { id: "advisory-model", name: "Advisory" }, + ], + fetchedAt: Date.now(), + provenModelIds: ["proven-model"], + }), + })); + + await expectCommandError( + dispatchCommand( + "sessions", + { + action: "create", + prompt: "hi", + harness_id: "codex-acp", + model_id: "advisory-model", + }, + ctx, + ), + "model_not_found", + ); + expect(mocks.acpCreateSession).not.toHaveBeenCalled(); + }); + + it("canonicalizes a saved provider alias for a model-only override", async () => { + seedModelCache("databricks_v2", ["alias-model"]); + mocks.listPersonas.mockResolvedValue([ + { + id: "legacy-alias-agent", + displayName: "Legacy Alias", + systemPrompt: "", + provider: "goose", + modelProviderId: "databricks", + model: "old-model", + isBuiltin: false, + writable: true, + }, + ]); + + await dispatchCommand( + "sessions", + { + action: "create", + prompt: "hi", + agent_id: "legacy-alias-agent", + model_id: "alias-model", + }, + ctx, + ); + + expect(mocks.acpCreateSession).toHaveBeenCalledWith( + "databricks_v2", + "/resolved/cwd", + expect.objectContaining({ modelId: "alias-model" }), + ); + }); + + it("ignores an ineligible provider when resolving a bare Goose model", async () => { + const [goose] = useProviderCatalogStore.getState().entries; + useProviderCatalogStore.setState({ + entries: [ + goose, + { + id: "eligible-provider", + displayName: "Eligible Provider", + category: "model", + description: "Runtime eligible", + setupMethod: "none", + group: "default", + }, + { + id: "ineligible-provider", + displayName: "Ineligible Provider", + category: "model", + description: "Excluded by runtime policy", + setupMethod: "none", + group: "default", + }, + ], + }); + useRuntimeConfigStore.setState({ + config: { + ...DEFAULT_RUNTIME_CONFIG, + goose: { + ...DEFAULT_RUNTIME_CONFIG.goose, + modelProviders: [ + { + id: "eligible-provider", + displayName: "Eligible Provider", + models: [{ id: "shared-model", name: "Shared Model" }], + }, + ], + }, + }, + }); + seedModelCache("eligible-provider", ["shared-model"]); + seedModelCache("ineligible-provider", ["shared-model"]); + + await dispatchCommand( + "sessions", + { action: "create", prompt: "hi", model_id: "shared-model" }, + ctx, + ); + + expect(mocks.acpCreateSession).toHaveBeenCalledWith( + "eligible-provider", + "/resolved/cwd", + expect.objectContaining({ modelId: "shared-model" }), + ); + }); + + it("rejects a complete explicit Goose target when its model id is provider-ambiguous", async () => { + const [goose] = useProviderCatalogStore.getState().entries; + const modelProviders = ["first-provider", "second-provider"].map((id) => ({ + id, + displayName: id, + category: "model" as const, + description: "Runtime eligible", + setupMethod: "none" as const, + group: "default" as const, + })); + useProviderCatalogStore.setState({ entries: [goose, ...modelProviders] }); + useRuntimeConfigStore.setState({ + config: { + ...DEFAULT_RUNTIME_CONFIG, + goose: { + ...DEFAULT_RUNTIME_CONFIG.goose, + modelProviders: modelProviders.map(({ id, displayName }) => ({ + id, + displayName, + models: [{ id: "shared-model", name: "Shared Model" }], + })), + }, + }, + }); + seedModelCache("first-provider", ["shared-model"]); + seedModelCache("second-provider", ["shared-model"]); + + await expectCommandError( + dispatchCommand( + "sessions", + { + action: "create", + prompt: "hi", + harness_id: "goose", + model_id: "shared-model", + }, + ctx, + ), + "model_ambiguous", + ); + expect(mocks.acpCreateSession).not.toHaveBeenCalled(); + }); + + it("accepts an explicit model for a ready ACP harness without authoritative inventory", async () => { + await dispatchCommand( + "sessions", + { + action: "create", + prompt: "hi", + harness_id: "codex-acp", + model_id: "unlisted-model", + }, + ctx, + ); + + expect(mocks.acpCreateSession).toHaveBeenCalledWith( + "codex-acp", + "/resolved/cwd", + expect.objectContaining({ modelId: "unlisted-model" }), + ); + }); + it("reports model_not_found when an explicit Goose model has no concrete provider", async () => { await expectCommandError( dispatchCommand( @@ -840,7 +1213,9 @@ describe("sessions.create", () => { expect(mocks.acpCreateSession).toHaveBeenCalledWith( "codex-acp", "/resolved/cwd", - expect.objectContaining({ modelId: "gpt-6" }), + expect.objectContaining({ + modelId: "gpt-6", + }), ); }); @@ -1005,7 +1380,8 @@ describe("sessions.send", () => { "session-1", "codex-acp", "/resolved/cwd", - { modelId: "gpt-6" }, + { modelId: "gpt-6", selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); expect(controller.openSession).not.toHaveBeenCalled(); @@ -1082,7 +1458,8 @@ describe("sessions.send", () => { "session-1", "codex-acp", "/resolved/cwd", - {}, + { selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); expect(getPendingSessionWorkspaceActivation("session-1")).toBeNull(); }); @@ -1131,7 +1508,8 @@ describe("sessions.send", () => { "session-1", "old-provider", "/resolved/cwd", - { modelId: "old-model" }, + { modelId: "old-model", selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); await vi.waitFor(() => { expect(mocks.acpSendMessage).toHaveBeenCalledWith( diff --git a/src/features/berdctl/commands/impl/createSession.ts b/src/features/berdctl/commands/impl/createSession.ts index ffdee39e9..359cdca24 100644 --- a/src/features/berdctl/commands/impl/createSession.ts +++ b/src/features/berdctl/commands/impl/createSession.ts @@ -95,6 +95,16 @@ Result: { berdctlCrossSessionSendOptions }, { GOOSE_PROVIDER_ID }, { normalizeSessionExecutionTarget, targetFromAgentModelSelection }, + { personaHarnessId, resolvePersonaExecutionTarget }, + { useAgentStore }, + { useProviderModelCacheStore }, + { + canonicalProviderCatalogIdFromEntries, + getModelProviders, + getProviderCatalog, + }, + { filterModelProvidersForRuntimeConfig }, + { useRuntimeConfigStore }, { findPersonaOrThrow }, { findProjectOrThrow }, { findReadyHarnessOrThrow, gooseModelOptions, harnessModelOptions }, @@ -106,53 +116,228 @@ Result: import("../runtime/sessionSend"), import("@/shared/api/acpPersonaHandoff"), import("@/features/chat/lib/sessionExecutionTarget"), + import("@/features/agents/lib/personaExecutionTarget"), + import("@/features/agents/stores/agentStore"), + import("@/features/providers/stores/providerModelCacheStore"), + import("@/features/providers/providerCatalog"), + import("@/features/providers/runtimeProviderConstraints"), + import("@/shared/runtime-config/runtimeConfigStore"), import("../runtime/agents"), import("../runtime/projects"), import("../runtime/providers"), ]); - const harnessId = args.harness_id ?? GOOSE_PROVIDER_ID; - // The validation legs are independent I/O; overlap them. - const [project, , models, persona] = await Promise.all([ + // Resolve precedence before validating any target field: a complete + // explicit target replaces saved execution metadata, while partial + // overrides still consume and therefore validate the remaining saved fields. + const [project, persona] = await Promise.all([ args.project_id ? findProjectOrThrow(args.project_id) : null, - args.harness_id ? findReadyHarnessOrThrow(args.harness_id) : null, - args.model_id - ? (harnessId === GOOSE_PROVIDER_ID - ? gooseModelOptions() - : harnessModelOptions(harnessId) - ).catch(() => []) - : null, args.agent_id ? findPersonaOrThrow(args.agent_id) : null, ]); - // Soft model validation: only reject when the harness's model list is - // known and the id is not in it. On goose a model belongs to a model - // provider (anthropic, openai, ...), so a match also resolves the - // provider the session should run against — mirroring the in-app picker. - let modelProviderId = - harnessId === GOOSE_PROVIDER_ID ? undefined : harnessId; - if (args.model_id && models) { - const match = models.find((model) => model.model_id === args.model_id); - if (match) { - modelProviderId = match.provider ?? modelProviderId; - } else if (models.length > 0) { - throw new CommandError( - "model_not_found", - `Model "${args.model_id}" is not available on "${harnessId}"; list models with \`berdctl info models\`.`, - ); - } + const providers = useAgentStore.getState().providers; + const catalogEntries = getProviderCatalog(); + const completeExplicitTarget = Boolean(args.harness_id && args.model_id); + const hasSavedTarget = Boolean( + persona?.provider || persona?.modelProviderId || persona?.model, + ); + const savedHarnessId = persona + ? personaHarnessId(persona.provider, providers, catalogEntries) + : undefined; + if (persona && !hasSavedTarget && !completeExplicitTarget) { + throw new CommandError( + "agent_configuration_invalid", + `Agent "${persona.id}" has no saved provider and model. Configure it or pass both --harness-id and --model-id.`, + ); + } + if (persona?.provider && !savedHarnessId && !completeExplicitTarget) { + throw new CommandError( + "agent_configuration_invalid", + `Agent "${persona.id}" has a saved provider or model that is no longer available. Update the agent configuration before invoking it.`, + ); + } + const harnessId = args.harness_id ?? savedHarnessId ?? GOOSE_PROVIDER_ID; + await findReadyHarnessOrThrow(harnessId); + + const requiresModelValidation = Boolean( + args.model_id || (persona && hasSavedTarget && !completeExplicitTarget), + ); + if (requiresModelValidation) { + if (harnessId === GOOSE_PROVIDER_ID) await gooseModelOptions(); + else await harnessModelOptions(harnessId); + } + const modelCache = useProviderModelCacheStore.getState(); + const eligibleGooseProviderIds = filterModelProvidersForRuntimeConfig( + getModelProviders(), + useRuntimeConfigStore.getState().config, + ).map((provider) => provider.id); + const provenModelsForProviders = (providerIds: Iterable) => + [...providerIds].flatMap((providerId) => + modelCache.isModelInventoryAuthoritative(providerId) + ? modelCache.getProvenModelsForProvider(providerId).map((model) => ({ + ...model, + providerId: model.providerId ?? providerId, + })) + : [], + ); + // Saved targets may reference a provider outside the current runtime + // inference set. Keep all authoritative cache entries available for saved + // target validation, while bare-model inference remains runtime-scoped. + const cachedModels = provenModelsForProviders(modelCache.providers.keys()); + const eligibleGooseModels = provenModelsForProviders( + eligibleGooseProviderIds, + ); + const modelsForHarness = (candidateHarnessId: string) => + candidateHarnessId === GOOSE_PROVIDER_ID + ? cachedModels + : modelCache.getModelsForProvider(candidateHarnessId); + const provenModelsForHarness = (candidateHarnessId: string) => + candidateHarnessId === GOOSE_PROVIDER_ID + ? cachedModels + : modelCache.getProvenModelsForProvider(candidateHarnessId); + + const effectivePersona = persona + ? { + provider: args.harness_id ?? persona.provider, + modelProviderId: args.harness_id + ? args.harness_id === GOOSE_PROVIDER_ID + ? persona.modelProviderId + : args.harness_id + : persona.modelProviderId, + model: args.model_id ?? persona.model, + } + : null; + const personaResolution = + effectivePersona && hasSavedTarget && !completeExplicitTarget + ? resolvePersonaExecutionTarget(effectivePersona, { + providers, + models: cachedModels, + getModelsForHarness: modelsForHarness, + getProvenModelsForHarness: provenModelsForHarness, + isModelInventoryAuthoritative: + modelCache.isModelInventoryAuthoritative, + catalogEntries, + }) + : { status: "absent" as const }; + if (personaResolution.status === "invalid") { + throw new CommandError( + "agent_configuration_invalid", + `Agent "${persona?.id}" has a saved provider or model that is no longer available. Update the agent configuration before invoking it.`, + ); + } + const personaTarget = + personaResolution.status === "valid" + ? personaResolution.target + : undefined; + const explicitModelId = args.model_id; + const explicitModelProviderBoundary = + harnessId === GOOSE_PROVIDER_ID && + persona && + !completeExplicitTarget && + !args.harness_id && + persona.modelProviderId + ? canonicalProviderCatalogIdFromEntries( + catalogEntries, + persona.modelProviderId, + ) + : undefined; + const gooseInventoryIsAuthoritative = eligibleGooseProviderIds.every( + modelCache.isModelInventoryAuthoritative, + ); + const inventoryIsAuthoritative = + harnessId === GOOSE_PROVIDER_ID + ? gooseInventoryIsAuthoritative + : modelCache.isModelInventoryAuthoritative(harnessId); + const inventoryModels = + harnessId === GOOSE_PROVIDER_ID + ? explicitModelProviderBoundary + ? modelCache.isModelInventoryAuthoritative( + explicitModelProviderBoundary, + ) + ? modelCache + .getProvenModelsForProvider(explicitModelProviderBoundary) + .map((model) => ({ + ...model, + providerId: model.providerId ?? explicitModelProviderBoundary, + })) + : modelCache + .getModelsForProvider(explicitModelProviderBoundary) + .map((model) => ({ + ...model, + providerId: model.providerId ?? explicitModelProviderBoundary, + })) + : eligibleGooseModels + : inventoryIsAuthoritative + ? provenModelsForHarness(harnessId) + : modelsForHarness(harnessId); + const matchingExplicitModels = explicitModelId + ? inventoryModels.filter( + (model) => + model.id === explicitModelId && + (!explicitModelProviderBoundary || + model.providerId === explicitModelProviderBoundary), + ) + : []; + // Bare-model inference is runtime-scoped. Authoritative models from an + // ineligible provider cannot make the eligible provider ambiguous. + const matchingExplicitProviders = new Set( + (explicitModelId && + harnessId === GOOSE_PROVIDER_ID && + !explicitModelProviderBoundary + ? eligibleGooseModels + : matchingExplicitModels + ).flatMap((model) => + model.id === explicitModelId && model.providerId + ? [model.providerId] + : [], + ), + ); + if ( + explicitModelId && + harnessId === GOOSE_PROVIDER_ID && + matchingExplicitProviders.size > 1 + ) { + throw new CommandError( + "model_ambiguous", + `Model "${explicitModelId}" is available from multiple Goose providers; select an agent with a provider-qualified model.`, + ); + } + const explicitModel = matchingExplicitModels[0]; + if (explicitModelId && !explicitModel && inventoryIsAuthoritative) { + throw new CommandError( + "model_not_found", + `Model "${explicitModelId}" is not available on "${harnessId}"; list models with \`berdctl info models\`.`, + ); + } + const explicitModelProviderId = + harnessId === GOOSE_PROVIDER_ID ? explicitModel?.providerId : harnessId; + if ( + explicitModelId && + harnessId === GOOSE_PROVIDER_ID && + !explicitModelProviderBoundary && + !gooseInventoryIsAuthoritative + ) { + throw new CommandError( + "model_not_found", + `Could not resolve a provider for model "${explicitModelId}" without authoritative inventory for every eligible Goose provider; list models with \`berdctl info models\` and retry.`, + ); } - if (args.model_id && !modelProviderId && harnessId === GOOSE_PROVIDER_ID) { + if (explicitModelId && !explicitModelProviderId) { throw new CommandError( "model_not_found", - `Could not resolve a provider for model "${args.model_id}"; list models with \`berdctl info models\` and retry.`, + `Could not resolve a provider for model "${explicitModelId}"; list models with \`berdctl info models\` and retry.`, ); } - const executionTarget = args.model_id + const executionTarget = explicitModelId ? targetFromAgentModelSelection(harnessId, { - modelProviderId: modelProviderId ?? harnessId, - modelId: args.model_id, - modelName: args.model_id, + modelProviderId: explicitModelProviderId ?? harnessId, + modelId: explicitModelId, + modelName: + explicitModel?.displayName ?? + explicitModel?.name ?? + explicitModelId, }) - : normalizeSessionExecutionTarget({ harnessId }); + : args.harness_id + ? (personaTarget ?? normalizeSessionExecutionTarget({ harnessId })) + : (personaTarget ?? normalizeSessionExecutionTarget({ harnessId })); const requiresStartupName = Boolean( project && projectRequiresStartupWorkspaceName(project), ); diff --git a/src/features/berdctl/commands/runtime/sessionSend.test.ts b/src/features/berdctl/commands/runtime/sessionSend.test.ts index 864ca273e..99dcc5cc0 100644 --- a/src/features/berdctl/commands/runtime/sessionSend.test.ts +++ b/src/features/berdctl/commands/runtime/sessionSend.test.ts @@ -37,6 +37,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpGetSessionInfo: (...args: unknown[]) => mocks.acpGetSessionInfo(...args), acpLoadSession: (...args: unknown[]) => mocks.acpLoadSession(...args), acpPrepareSession: (...args: unknown[]) => { @@ -580,7 +581,8 @@ describe("sendPromptToExistingSessionInBackground", () => { SESSION_ID, "claude-acp", expect.any(String), - { modelId: "claude-sonnet-4" }, + { modelId: "claude-sonnet-4", selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); expect(mocks.acpSendMessage).toHaveBeenCalledWith( SESSION_ID, @@ -655,7 +657,8 @@ describe("sendPromptToExistingSessionInBackground", () => { SESSION_ID, UPDATED_TARGET.modelProviderId, "/tmp/project", - { modelId: UPDATED_TARGET.modelId }, + { modelId: UPDATED_TARGET.modelId, selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ]); expect(mocks.acpSendMessage).toHaveBeenCalledTimes(1); expect( @@ -720,7 +723,8 @@ describe("sendPromptToExistingSessionInBackground", () => { SESSION_ID, INITIAL_TARGET.modelProviderId, "/tmp/project", - { modelId: INITIAL_TARGET.modelId }, + { modelId: INITIAL_TARGET.modelId, selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ]); expect(mocks.transportProviders).toEqual([INITIAL_TARGET.modelProviderId]); expect( @@ -865,7 +869,8 @@ describe("sendPromptToExistingSessionInBackground", () => { SESSION_ID, INITIAL_TARGET.modelProviderId, "/tmp/project", - { modelId: INITIAL_TARGET.modelId }, + { modelId: INITIAL_TARGET.modelId, selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); expect(mocks.acpPrepareSession.mock.calls.at(-1)).toEqual([ SESSION_ID, @@ -873,8 +878,10 @@ describe("sendPromptToExistingSessionInBackground", () => { "/tmp/project", expect.objectContaining({ modelId: UPDATED_TARGET.modelId, + selectionAlreadyResolved: true, requestId: "select-updated-during-prepare", }), + expect.objectContaining({ clear: expect.any(Function) }), ]); expect(mocks.acpSendMessage).toHaveBeenCalledTimes(1); expect(mocks.transportProviders).toEqual([INITIAL_TARGET.modelProviderId]); @@ -915,7 +922,8 @@ describe("sendPromptToExistingSessionInBackground", () => { SESSION_ID, INITIAL_TARGET.modelProviderId, "/tmp/project", - { modelId: INITIAL_TARGET.modelId }, + { modelId: INITIAL_TARGET.modelId, selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ]); expect(mocks.acpPrepareSession.mock.calls.at(-1)).toEqual([ SESSION_ID, @@ -923,8 +931,10 @@ describe("sendPromptToExistingSessionInBackground", () => { "/tmp/project", expect.objectContaining({ modelId: UPDATED_TARGET.modelId, + selectionAlreadyResolved: true, requestId: "select-updated-during-cwd", }), + expect.objectContaining({ clear: expect.any(Function) }), ]); expect(mocks.acpSendMessage).toHaveBeenCalledTimes(1); expect(mocks.transportProviders).toEqual([INITIAL_TARGET.modelProviderId]); diff --git a/src/features/berdctl/commands/types.ts b/src/features/berdctl/commands/types.ts index 978e442a2..9b2bbed39 100644 --- a/src/features/berdctl/commands/types.ts +++ b/src/features/berdctl/commands/types.ts @@ -73,10 +73,12 @@ export const COMMAND_ERROR_CODES = [ "session_not_found", "project_not_found", "agent_not_found", + "agent_configuration_invalid", "skill_not_found", "harness_not_found", "harness_not_ready", "model_not_found", + "model_ambiguous", "blocked_unsaved_changes", "backend_read_failed", "backend_archive_failed", diff --git a/src/features/chat/hooks/__tests__/useChat.attachments.test.ts b/src/features/chat/hooks/__tests__/useChat.attachments.test.ts index 6dde38e17..6019c0904 100644 --- a/src/features/chat/hooks/__tests__/useChat.attachments.test.ts +++ b/src/features/chat/hooks/__tests__/useChat.attachments.test.ts @@ -9,6 +9,7 @@ const mockAcpCancelSession = vi.fn(); const mockAcpPrepareSession = vi.fn(); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSendMessage: (...args: unknown[]) => { const result = mockAcpSendMessage(...args); const options = args[2] as diff --git a/src/features/chat/hooks/__tests__/useChat.compaction.test.ts b/src/features/chat/hooks/__tests__/useChat.compaction.test.ts index 03f1d53fc..451125154 100644 --- a/src/features/chat/hooks/__tests__/useChat.compaction.test.ts +++ b/src/features/chat/hooks/__tests__/useChat.compaction.test.ts @@ -12,6 +12,7 @@ const mockAcpSendMessage = vi.fn(); const mockAcpLoadSession = vi.fn(); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSendMessage: (...args: unknown[]) => mockAcpSendMessage(...args), acpCancelSession: vi.fn(), acpLoadSession: (...args: unknown[]) => mockAcpLoadSession(...args), diff --git a/src/features/chat/hooks/__tests__/useChat.personaPreparation.test.ts b/src/features/chat/hooks/__tests__/useChat.personaPreparation.test.ts index 37e22ab1f..0c64c6f20 100644 --- a/src/features/chat/hooks/__tests__/useChat.personaPreparation.test.ts +++ b/src/features/chat/hooks/__tests__/useChat.personaPreparation.test.ts @@ -10,6 +10,7 @@ const mockAcpCancelSession = vi.fn(); const mockAcpLoadSession = vi.fn(); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSendMessage: (...args: unknown[]) => { const result = mockAcpSendMessage(...args); const options = args[2] as diff --git a/src/features/chat/hooks/__tests__/useChat.skillChips.test.ts b/src/features/chat/hooks/__tests__/useChat.skillChips.test.ts index 6d506431f..3c8762c9a 100644 --- a/src/features/chat/hooks/__tests__/useChat.skillChips.test.ts +++ b/src/features/chat/hooks/__tests__/useChat.skillChips.test.ts @@ -10,6 +10,7 @@ const mockAcpLoadSession = vi.fn(); const mockAcpPrepareSession = vi.fn(); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSendMessage: (...args: unknown[]) => { const result = mockAcpSendMessage(...args); const options = args[2] as diff --git a/src/features/chat/hooks/__tests__/useChat.test.ts b/src/features/chat/hooks/__tests__/useChat.test.ts index 3a7ebb280..d70bd0591 100644 --- a/src/features/chat/hooks/__tests__/useChat.test.ts +++ b/src/features/chat/hooks/__tests__/useChat.test.ts @@ -24,6 +24,7 @@ const mockAcpPrepareSession = vi.fn(); let mockAcpDispatches = true; vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSendMessage: (...args: unknown[]) => { const result = mockAcpSendMessage(...args); const options = args[2] as diff --git a/src/features/chat/hooks/__tests__/useChatSessionController.targetLeaseCompaction.test.ts b/src/features/chat/hooks/__tests__/useChatSessionController.targetLeaseCompaction.test.ts index 71afa9c07..e2984c356 100644 --- a/src/features/chat/hooks/__tests__/useChatSessionController.targetLeaseCompaction.test.ts +++ b/src/features/chat/hooks/__tests__/useChatSessionController.targetLeaseCompaction.test.ts @@ -31,6 +31,7 @@ function deferred() { } vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpPrepareSession: async (...args: unknown[]) => { const result = await mockAcpPrepareSession(...args); preparedProviderBySession.set(args[0] as string, args[1] as string); diff --git a/src/features/chat/hooks/__tests__/useChatSessionController.test.ts b/src/features/chat/hooks/__tests__/useChatSessionController.test.ts index d12c33d75..0f735c2ed 100644 --- a/src/features/chat/hooks/__tests__/useChatSessionController.test.ts +++ b/src/features/chat/hooks/__tests__/useChatSessionController.test.ts @@ -16,7 +16,10 @@ import { type ChatAttachmentDraft, createUserMessage, } from "@/shared/types/messages"; -import { useChatStore } from "../../stores/chatStore"; +import { + type QueuedMessagePayload, + useChatStore, +} from "../../stores/chatStore"; import { type ChatSession, useChatSessionStore, @@ -115,6 +118,7 @@ function deferred() { } vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpPrepareSession: (...args: unknown[]) => mockAcpPrepareSession(...args), acpSetSessionConfigOption: (...args: unknown[]) => mockAcpSetSessionConfigOption(...args), @@ -255,6 +259,9 @@ vi.mock("../useAgentModelPickerState", () => ({ getModelsForAgent: (agentId: string) => mockPickerState.modelsByAgent.get(agentId) ?? mockPickerState.availableModels, + getProvenModelsForAgent: (agentId: string) => + mockPickerState.modelsByAgent.get(agentId) ?? + mockPickerState.availableModels, isModelInventoryAuthoritative: () => false, modelsLoading: mockPickerState.modelsLoading, modelStatusMessage: mockPickerState.modelStatusMessage, @@ -323,6 +330,7 @@ function expectSessionPreparation({ ...(modelId ? { modelId } : {}), ...(forceConfigRefresh ? { forceConfigRefresh: true } : {}), }), + expect.objectContaining({ clear: expect.any(Function) }), ); } @@ -3099,6 +3107,7 @@ describe("useChatSessionController", () => { displayName: "Codex Planner", systemPrompt: "Plan clearly.", provider: "codex-acp", + model: "codex-model", }), personaFixture({ id: "persona-2", @@ -3195,6 +3204,12 @@ describe("useChatSessionController", () => { personaId: "persona-1", personaName: "Codex Planner", sendOptions: { + sessionSelection: { + harnessId: "codex-acp", + modelProviderId: "codex-acp", + modelId: "codex-model", + modelName: "codex-model", + }, capturedPersonaSystemPrompt: expect.stringContaining("Plan clearly."), telemetrySourceSurface: CHAT_SOURCE_SURFACE.MAIN_CHAT, }, @@ -3204,6 +3219,12 @@ describe("useChatSessionController", () => { personaId: "persona-2", personaName: "Goose Reviewer", sendOptions: { + sessionSelection: { + harnessId: "goose", + modelProviderId: "databricks_v2", + modelId: "goose-claude-opus-4-8", + modelName: "Claude Opus 4.8", + }, capturedPersonaSystemPrompt: expect.stringContaining("Review carefully."), telemetrySourceSurface: CHAT_SOURCE_SURFACE.MAIN_CHAT, @@ -4764,64 +4785,45 @@ describe("useChatSessionController", () => { expect(result.current.currentModelName).toBe("Claude Sonnet 4"); }); - it("applies a persona's provider-only target exactly", async () => { - useProviderCatalogStore.getState().mergeEntries([ - { - id: "openai", - displayName: "OpenAI", - category: "model", - description: "OpenAI", - setupMethod: "single_api_key", - group: "default", - }, - ]); + it("rejects an invalid persona send before acceptance and preserves state", () => { useAgentStore.setState({ personas: [ personaFixture({ - provider: "goose", - modelProviderId: "openai", - model: undefined, + provider: "missing-provider", + model: "missing-model", }), ], + activeAgentId: null, }); + useChatStore.setState((state) => ({ + draftsBySession: { ...state.draftsBySession, "session-1": "keep me" }, + })); + const beforeSession = useChatSessionStore + .getState() + .getSession("session-1"); const { result } = renderHook(() => useChatSessionController({ sessionId: "session-1" }), ); - + let accepted: boolean | Promise = true; act(() => { - result.current.handlePersonaChange("persona-1"); - }); - - await waitFor(() => { - expect( - useChatSessionStore.getState().getSession("session-1"), - ).toMatchObject({ - personaId: "persona-1", - executionTarget: { - harnessId: "goose", - modelProviderId: "openai", - }, - }); + accepted = result.current.handleSend("do not send", "persona-1"); }); - expect(mockAcpPrepareSession).toHaveBeenCalledWith( - "session-1", - "openai", - "/tmp/project", - expect.objectContaining({ requestId: expect.any(String) }), + expect(accepted).toBe(false); + expect(useChatStore.getState().draftsBySession["session-1"]).toBe( + "keep me", ); + expect( + useChatStore.getState().queuedMessageBySession["session-1"], + ).toBeUndefined(); + expect(useChatSessionStore.getState().getSession("session-1")).toEqual( + beforeSession, + ); + expect(useAgentStore.getState().activeAgentId).toBeNull(); + expect(mockUseChatSendMessage).not.toHaveBeenCalled(); + expect(mockAcpPrepareSession).not.toHaveBeenCalled(); }); - it("keeps a provider-only persona target local while session creation is pending", () => { - useProviderCatalogStore.getState().mergeEntries([ - { - id: "openai", - displayName: "OpenAI", - category: "model", - description: "OpenAI", - setupMethod: "single_api_key", - group: "default", - }, - ]); + it("rejects a persona's provider-only target", () => { useAgentStore.setState({ personas: [ personaFixture({ @@ -4831,30 +4833,16 @@ describe("useChatSessionController", () => { }), ], }); - useChatSessionStore.setState((state) => ({ - sessions: state.sessions.map((candidate) => - candidate.id === "session-1" - ? { ...candidate, creationState: "pending" } - : candidate, - ), - })); + const before = useChatSessionStore.getState().getSession("session-1"); const { result } = renderHook(() => useChatSessionController({ sessionId: "session-1" }), ); - act(() => { result.current.handlePersonaChange("persona-1"); }); - - expect( - useChatSessionStore.getState().getSession("session-1"), - ).toMatchObject({ - personaId: "persona-1", - executionTarget: { - harnessId: "goose", - modelProviderId: "openai", - }, - }); + expect(useChatSessionStore.getState().getSession("session-1")).toEqual( + before, + ); expect(mockAcpPrepareSession).not.toHaveBeenCalled(); }); @@ -5183,7 +5171,7 @@ describe("useChatSessionController", () => { }); }); - it("leaves the current target alone when a legacy persona model cannot resolve", async () => { + it("rejects a legacy persona whose saved model cannot resolve", async () => { useAgentStore.setState({ personas: [ personaFixture({ @@ -5214,13 +5202,13 @@ describe("useChatSessionController", () => { expect( useChatSessionStore.getState().getSession("session-1"), ).toMatchObject({ - personaId: "persona-1", executionTarget: { harnessId: "goose", modelProviderId: "openai", modelId: "gpt-4o", }, }); + expect(useAgentStore.getState().activeAgentId).toBeNull(); expect(mockAcpPrepareSession).not.toHaveBeenCalled(); }); @@ -5278,6 +5266,63 @@ describe("useChatSessionController", () => { }); }); + it("replaces captured persona authority when a queued edit removes the persona", () => { + const update = vi.fn( + (_recordId: string, _payload: QueuedMessagePayload) => true, + ); + const staleToken = Symbol("persona-a"); + mockUseMessageQueue.mockReturnValue({ + queuedMessage: null, + enqueue: vi.fn(), + update, + dismiss: vi.fn(), + }); + useAgentStore.setState({ + personas: [ + personaFixture({ + provider: "goose", + model: "goose-claude-fable-5", + }), + ], + }); + + const { result } = renderHook(() => + useChatSessionController({ sessionId: "session-1" }), + ); + + act(() => { + result.current.queue.update("queued-a", { + text: "send without persona", + persona: { kind: "none" }, + sendOptions: { + sessionSelection: { + harnessId: "goose", + modelProviderId: "databricks_v2", + modelId: "goose-claude-fable-5", + modelName: "goose-claude-fable-5", + }, + sessionSelectionToken: staleToken, + displayText: "preserved", + }, + }); + }); + + expect(update).toHaveBeenCalledWith( + "queued-a", + expect.objectContaining({ + persona: { kind: "none" }, + sendOptions: expect.objectContaining({ displayText: "preserved" }), + }), + ); + const updateCall = update.mock.calls[0]; + if (!updateCall) { + throw new Error("Expected the queued message to be recaptured"); + } + const recaptured = updateCall[1].sendOptions; + expect(recaptured).not.toHaveProperty("sessionSelection"); + expect(recaptured).not.toHaveProperty("sessionSelectionToken"); + }); + it("removes the active persona without changing the selected model", async () => { useChatSessionStore.getState().replaceSessionExecutionTarget("session-1", { harnessId: "goose", @@ -5322,7 +5367,7 @@ describe("useChatSessionController", () => { expect(mockAcpPrepareSession).not.toHaveBeenCalled(); }); - it("leaves the current target alone when a persona target cannot resolve", () => { + it("rejects a persona whose saved target cannot resolve", () => { useAgentStore.setState({ personas: [ personaFixture({ @@ -5344,17 +5389,17 @@ describe("useChatSessionController", () => { expect( useChatSessionStore.getState().getSession("session-1"), ).toMatchObject({ - personaId: "persona-1", executionTarget: { harnessId: "goose", modelProviderId: "openai", modelId: "gpt-4o", }, }); + expect(useAgentStore.getState().activeAgentId).toBeNull(); expect(mockAcpPrepareSession).not.toHaveBeenCalled(); }); - it("keeps the Home target when an unresolved persona is selected", async () => { + it("does not carry an unresolved persona into a later Home session", async () => { useAgentStore.setState({ personas: [ personaFixture({ @@ -5391,12 +5436,15 @@ describe("useChatSessionController", () => { expect( useChatSessionStore.getState().getSession("home-unresolved-persona"), ).toMatchObject({ - personaId: "persona-1", executionTarget: { harnessId: "goose", modelProviderId: "openai", }, }); + expect( + useChatSessionStore.getState().getSession("home-unresolved-persona") + ?.personaId, + ).toBeUndefined(); }); expect(mockAcpPrepareSession).not.toHaveBeenCalled(); }); diff --git a/src/features/chat/hooks/__tests__/useResolvedAgentModelPicker.test.ts b/src/features/chat/hooks/__tests__/useResolvedAgentModelPicker.test.ts index dd04d0bbc..b7f2d4276 100644 --- a/src/features/chat/hooks/__tests__/useResolvedAgentModelPicker.test.ts +++ b/src/features/chat/hooks/__tests__/useResolvedAgentModelPicker.test.ts @@ -58,6 +58,7 @@ function renderModelPicker(overrides: Partial = {}) { vi.mock("../useAgentModelPickerState", () => ({ useAgentModelPickerState: (args: unknown) => ({ getModelsForAgent: () => [], + getProvenModelsForAgent: () => [], isModelInventoryAuthoritative: () => false, ...mockUseAgentModelPickerState(args), }), @@ -68,6 +69,7 @@ vi.mock("@/shared/api/acpConnection", () => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpPrepareSession: (...args: unknown[]) => mockPrepareSession(...args), })); @@ -211,6 +213,7 @@ describe("useResolvedAgentModelPicker", () => { "openai", "/w", expect.objectContaining({ modelId: "next" }), + expect.objectContaining({ clear: expect.any(Function) }), ); }); @@ -392,6 +395,55 @@ describe("useResolvedAgentModelPicker", () => { executionTarget: { harnessId: "codex-acp" }, }, ); + const firstSelection = getSessionTargetSelection(session.id); + expect(firstSelection).toMatchObject({ + target: { harnessId: "codex-acp" }, + }); + + act(() => result.current.handleProviderChange("claude-acp")); + + const secondSelection = getSessionTargetSelection(session.id); + expect(secondSelection).toMatchObject({ + target: { harnessId: "claude-acp" }, + }); + expect(secondSelection?.operationId).not.toBe(firstSelection?.operationId); + }); + + it("gives a provider-only A to B to A draft change new ownership", () => { + useChatSessionStore.getState().createDraftSession({ + workingDir: "/tmp/project", + executionTarget: { harnessId: "goose" }, + }); + const session = useChatSessionStore.getState().sessions[0]; + const { result } = renderModelPicker({ + providers: [ + { id: "goose", label: "Goose" }, + { id: "codex-acp", label: "Codex" }, + ], + sessionId: session.id, + session, + }); + + act(() => result.current.handleProviderChange("codex-acp")); + const providerB = getSessionTargetSelection(session.id); + + const liveDraft = useChatSessionStore.getState().getSession(session.id); + const { result: rerendered } = renderModelPicker({ + providers: [ + { id: "goose", label: "Goose" }, + { id: "codex-acp", label: "Codex" }, + ], + selectedProvider: "codex-acp", + sessionId: session.id, + session: liveDraft, + }); + act(() => rerendered.current.handleProviderChange("goose")); + + const providerAAgain = getSessionTargetSelection(session.id); + expect(providerAAgain).toMatchObject({ + target: { harnessId: "goose" }, + }); + expect(providerAAgain?.operationId).not.toBe(providerB?.operationId); }); it("routes explicit concrete model providers through the Goose harness", () => { @@ -469,6 +521,14 @@ describe("useResolvedAgentModelPicker", () => { providerId: "anthropic", }, ], + getProvenModelsForAgent: () => [ + { id: "gpt-5.4", name: "GPT-5.4", providerId: "openai" }, + { + id: "claude-sonnet-4", + name: "Claude Sonnet 4", + providerId: "anthropic", + }, + ], modelsLoading: false, modelStatusMessage: null, handleProviderChange: (providerId: string) => @@ -495,6 +555,35 @@ describe("useResolvedAgentModelPicker", () => { }); }); + it("does not accept an advisory retired model for an existing session", () => { + mockUseAgentModelPickerState.mockImplementation(() => ({ + pickerAgents: [{ id: "goose", label: "Goose" }], + availableModels: [ + { id: "retired", name: "Retired", providerId: "openai" }, + { id: "current", name: "Current", providerId: "openai" }, + ], + getProvenModelsForAgent: () => [ + { id: "current", name: "Current", providerId: "openai" }, + ], + isModelInventoryAuthoritative: (providerId: string) => + providerId === "openai", + modelsLoading: false, + modelStatusMessage: null, + handleProviderChange: vi.fn(), + handleModelChange: vi.fn(), + })); + + const session = makeSession({ + harnessId: "goose", + modelProviderId: "openai", + modelId: "retired", + modelName: "Retired", + }); + const { result } = renderModelPicker({ session }); + + expect(result.current.effectiveModelSelection?.id).not.toBe("retired"); + }); + it("does not synthesize a model for an existing provider-only session", () => { window.localStorage.setItem( "goose:preferredModelsByAgent", @@ -556,6 +645,19 @@ describe("useResolvedAgentModelPicker", () => { recommended: true, }, ], + getProvenModelsForAgent: () => [ + { + id: "gpt-5.4-mini", + name: "GPT Mini 5.4", + providerId: "codex-acp", + }, + { + id: "gpt-5.5", + name: "GPT 5.5", + providerId: "codex-acp", + recommended: true, + }, + ], modelsLoading: false, modelStatusMessage: null, handleProviderChange: vi.fn(), @@ -1149,6 +1251,7 @@ describe("useResolvedAgentModelPicker", () => { recommended: true, }, ], + getProvenModelsForAgent: () => [], isModelInventoryAuthoritative: () => false, modelsLoading: true, modelStatusMessage: null, @@ -1201,6 +1304,14 @@ describe("useResolvedAgentModelPicker", () => { recommended: true, }, ], + getProvenModelsForAgent: () => [ + { + id: "gpt-5.6", + name: "GPT-5.6", + providerId: "openai", + recommended: true, + }, + ], isModelInventoryAuthoritative: (providerId: string) => providerId === "openai", modelsLoading: false, @@ -1220,6 +1331,52 @@ describe("useResolvedAgentModelPicker", () => { }); }); + it("does not auto-select an advisory model while inventory proof is unavailable", () => { + mockUseAgentModelPickerState.mockImplementation(() => ({ + pickerAgents: [{ id: "goose", label: "Goose" }], + availableModels: [ + { id: "advisory", name: "Advisory", recommended: true }, + ], + getProvenModelsForAgent: () => [], + isModelInventoryAuthoritative: () => false, + modelsLoading: true, + modelStatusMessage: null, + handleProviderChange: vi.fn(), + handleModelChange: vi.fn(), + })); + + const { result } = renderModelPicker({ + selectedProvider: "openai", + sessionId: null, + session: undefined, + }); + + expect(result.current.effectiveModelSelection).toBeNull(); + }); + + it("does not auto-select an unqualified advisory model from authoritative empty inventory", () => { + mockUseAgentModelPickerState.mockImplementation(() => ({ + pickerAgents: [{ id: "goose", label: "Goose" }], + availableModels: [ + { id: "advisory", name: "Advisory", recommended: true }, + ], + getProvenModelsForAgent: () => [], + isModelInventoryAuthoritative: () => true, + modelsLoading: false, + modelStatusMessage: null, + handleProviderChange: vi.fn(), + handleModelChange: vi.fn(), + })); + + const { result } = renderModelPicker({ + selectedProvider: "openai", + sessionId: null, + session: undefined, + }); + + expect(result.current.effectiveModelSelection).toBeNull(); + }); + it("ignores a stored model missing from an authoritative populated inventory", () => { window.localStorage.setItem( "goose:preferredModelsByAgent", @@ -1241,6 +1398,9 @@ describe("useResolvedAgentModelPicker", () => { recommended: true, }, ], + getProvenModelsForAgent: () => [ + { id: "gpt-5.6", name: "GPT-5.6", providerId: "openai" }, + ], isModelInventoryAuthoritative: () => true, modelsLoading: true, modelStatusMessage: null, diff --git a/src/features/chat/hooks/useAgentModelPickerState.ts b/src/features/chat/hooks/useAgentModelPickerState.ts index 6293edcf1..f42c15e67 100644 --- a/src/features/chat/hooks/useAgentModelPickerState.ts +++ b/src/features/chat/hooks/useAgentModelPickerState.ts @@ -29,6 +29,7 @@ export function useAgentModelPickerState({ configuredModelProviderIds, modelCacheRefreshProviderIds, getModelsForAgent, + getProvenModelsForAgent, isModelInventoryAuthoritative: isProviderModelInventoryAuthoritative, refreshAllModelProviders, isRefreshingProvider, @@ -212,6 +213,7 @@ export function useAgentModelPickerState({ pickerAgents, availableModels, getModelsForAgent, + getProvenModelsForAgent, isModelInventoryAuthoritative, modelsLoading, modelStatusMessage, diff --git a/src/features/chat/hooks/useChatSessionController.ts b/src/features/chat/hooks/useChatSessionController.ts index 3cbe97a42..f3290eb89 100644 --- a/src/features/chat/hooks/useChatSessionController.ts +++ b/src/features/chat/hooks/useChatSessionController.ts @@ -83,7 +83,10 @@ import { markAgentBuilderSessionPreparationFailed, preSeedDraftAgent, } from "@/features/agents/lib/agentBuilderSession"; -import { personaExecutionTarget } from "@/features/agents/lib/personaExecutionTarget"; +import { + personaHarnessId, + resolvePersonaExecutionTarget, +} from "@/features/agents/lib/personaExecutionTarget"; import { deletePersonaSource } from "@/shared/api/agents"; import type { Persona } from "@/shared/types/agents"; import { @@ -1093,6 +1096,8 @@ export function useChatSessionController({ pickerAgents, availableModels, getModelsForAgent, + getProvenModelsForAgent, + isModelInventoryAuthoritative, modelsLoading, modelStatusMessage, handleProviderChange, @@ -1204,13 +1209,21 @@ export function useChatSessionController({ const resolvePersonaTarget = useCallback( (persona: Persona) => - personaExecutionTarget(persona, { + resolvePersonaExecutionTarget(persona, { providers, models: getModelsForAgent("goose"), getModelsForHarness: getModelsForAgent, + getProvenModelsForHarness: getProvenModelsForAgent, + isModelInventoryAuthoritative, catalogEntries, }), - [catalogEntries, getModelsForAgent, providers], + [ + catalogEntries, + getModelsForAgent, + getProvenModelsForAgent, + isModelInventoryAuthoritative, + providers, + ], ); const prepareSessionForCurrentSelection = useCallback( async ( @@ -1436,7 +1449,16 @@ export function useChatSessionController({ } const persona = personas.find((candidate) => candidate.id === personaId); - const personaTarget = persona ? resolvePersonaTarget(persona) : undefined; + const personaResolution = persona + ? resolvePersonaTarget(persona) + : { status: "absent" as const }; + if (personaResolution.status === "invalid") { + return; + } + const personaTarget = + personaResolution.status === "valid" + ? personaResolution.target + : undefined; if (personaTarget) { const harnessId = personaTarget.harnessId; @@ -1838,14 +1860,25 @@ export function useChatSessionController({ const targetPersona = personas.find( (persona) => persona.id === overridePersona.id, ); + const resolution = targetPersona + ? resolvePersonaTarget(targetPersona) + : undefined; return ( - (targetPersona - ? resolvePersonaTarget(targetPersona)?.harnessId - : undefined) ?? selectedAgentId + (resolution?.status === "valid" + ? resolution.target.harnessId + : targetPersona + ? personaHarnessId( + targetPersona.provider, + providers, + catalogEntries, + ) + : undefined) ?? selectedAgentId ); }, [ + catalogEntries, personas, + providers, resolvePersonaTarget, selectedAgentId, session?.executionTarget?.harnessId, @@ -2413,8 +2446,19 @@ export function useChatSessionController({ availableSkillsCatalogPrompt, ) : undefined; + const personaTarget = queuedPersona + ? resolvePersonaTarget(queuedPersona) + : undefined; + const { + sessionSelection: _previousPersonaSelection, + sessionSelectionToken: _previousPersonaSelectionToken, + ...retainedSendOptions + } = payload.sendOptions ?? {}; const sendOptions = { - ...payload.sendOptions, + ...retainedSendOptions, + ...(personaTarget?.status === "valid" + ? { sessionSelection: personaTarget.target } + : {}), ...(capturedPersonaSystemPrompt !== undefined ? { capturedPersonaSystemPrompt } : {}), @@ -2444,6 +2488,7 @@ export function useChatSessionController({ availableSkillsCatalogPrompt, chatSourceSurface, includedWorkspacesPrompt, + resolvePersonaTarget, selectedPersona, workspaceContextReady, workspaceInstructionsPrompt, @@ -2492,6 +2537,19 @@ export function useChatSessionController({ if (currentPreSendWorkspaceSetup?.status === "creating") { return false; } + const intendedPersona = personaId + ? selectedPersona?.id === personaId + ? selectedPersona + : useAgentStore.getState().getPersonaById(personaId) + : personaId === undefined + ? selectedPersona + : undefined; + if ( + (personaId && !intendedPersona && personaId !== selectedPersonaId) || + (intendedPersona && + resolvePersonaTarget(intendedPersona).status === "invalid") + ) + return false; const personaName = personaId ? selectedPersona?.id === personaId ? selectedPersona.displayName @@ -2725,6 +2783,7 @@ export function useChatSessionController({ queue, readOnly, recordDraftPreservingSubmission, + resolvePersonaTarget, session?.agentBuilderOpen, session?.creationState, session?.intent, diff --git a/src/features/chat/hooks/useMessageQueue.ts b/src/features/chat/hooks/useMessageQueue.ts index cffcdf307..1cd1b71a0 100644 --- a/src/features/chat/hooks/useMessageQueue.ts +++ b/src/features/chat/hooks/useMessageQueue.ts @@ -167,7 +167,10 @@ export function useMessageQueue( if (dispatchReleaseUnsubscribeRef.current) { return false; } - const acquisition = acquireSessionDispatchTarget(sessionId); + const acquisition = acquireSessionDispatchTarget( + sessionId, + queuedMsg.payload.sendOptions?.sessionSelection, + ); if (acquisition.status === "contended") { dispatchReleasePayloadRef.current = payload; dispatchReleaseUnsubscribeRef.current = acquisition.waiter.wait(() => { diff --git a/src/features/chat/hooks/useResolvedAgentModelPicker.ts b/src/features/chat/hooks/useResolvedAgentModelPicker.ts index d21d8a4b9..0e8c7c62a 100644 --- a/src/features/chat/hooks/useResolvedAgentModelPicker.ts +++ b/src/features/chat/hooks/useResolvedAgentModelPicker.ts @@ -34,7 +34,6 @@ import { type SessionExecutionTarget, } from "../lib/sessionExecutionTarget"; import { gooseServeSelectionFromExecutionTarget } from "../lib/gooseServeExecutionTarget"; -import { replaceSessionTargetAfterDispatch } from "../lib/sessionTargetCoordinator"; import type { ModelOption } from "../types"; const MODEL_ALIAS_IDS = new Set(["current", "default"]); @@ -133,6 +132,7 @@ function getPreferredSelectionForAgent( function resolveAvailableSelection( selection: PreferredModelSelection, models: readonly ModelOption[], + provenModels: readonly ModelOption[], selectedModelProviderId: string | null, isInventoryAuthoritative: (providerId: string) => boolean, ): PreferredModelSelection | null { @@ -143,7 +143,10 @@ function resolveAvailableSelection( return null; } - const matchingModel = models.find( + const candidates = isInventoryAuthoritative(selection.modelProviderId) + ? provenModels + : models; + const matchingModel = candidates.find( (model) => model.id === selection.id && (!model.providerId || model.providerId === selection.modelProviderId), @@ -337,6 +340,7 @@ export function useResolvedAgentModelPicker({ pickerAgents, availableModels, getModelsForAgent, + getProvenModelsForAgent, isModelInventoryAuthoritative, modelsLoading, modelStatusMessage, @@ -391,20 +395,18 @@ export function useResolvedAgentModelPicker({ setGlobalSelectedProvider(resolvedRequestedAgentId); } - // A pending draft only has a client-generated id. Keep the selection on - // the draft so startup can apply it after ACP returns the backend id; - // sending a config request now would target a session ACP cannot know. + // A pending draft only has a client-generated id. Keep every target + // mutation under a fresh ownership token so provider-only changes can + // supersede creation/reconciliation work just like model changes do. if (session?.creationState === "pending") { - if (nextTarget.modelId) { - beginModelSelectionIntent(sessionId, { - requestId: createModelSelectionRequestId(), - target: nextTarget, - previousTarget: session.executionTarget, - preferenceAgentId: resolvedRequestedAgentId, - }); - } else { - replaceSessionTargetAfterDispatch(sessionId, nextTarget); - } + beginModelSelectionIntent(sessionId, { + requestId: createModelSelectionRequestId(), + target: nextTarget, + previousTarget: session.executionTarget, + ...(nextTarget.modelId + ? { preferenceAgentId: resolvedRequestedAgentId } + : {}), + }); return; } @@ -695,6 +697,7 @@ export function useResolvedAgentModelPicker({ const availableStoredSelection = resolveAvailableSelection( storedSelection, availableModels, + getProvenModelsForAgent(selectedAgentId), concreteSelectedProviderId, isModelInventoryAuthoritative, ); @@ -723,6 +726,7 @@ export function useResolvedAgentModelPicker({ modelProviderId: defaultModelProviderId, }, availableModels, + getProvenModelsForAgent(selectedAgentId), concreteSelectedProviderId, isModelInventoryAuthoritative, ); @@ -730,6 +734,7 @@ export function useResolvedAgentModelPicker({ availableModels, catalogEntries, concreteSelectedProviderId, + getProvenModelsForAgent, gooseDefaultSelection, isModelInventoryAuthoritative, selectedAgentId, @@ -742,7 +747,13 @@ export function useResolvedAgentModelPicker({ return null; } - const modelsMatchingSessionId = availableModels.filter( + const inventoryAuthoritative = + executionTarget.modelProviderId != null && + isModelInventoryAuthoritative(executionTarget.modelProviderId); + const sessionCandidates = inventoryAuthoritative + ? getProvenModelsForAgent(executionTarget.harnessId) + : availableModels; + const modelsMatchingSessionId = sessionCandidates.filter( (model) => model.id === executionTarget.modelId, ); const exactProviderMatch = @@ -767,7 +778,7 @@ export function useResolvedAgentModelPicker({ }; } - if (isModelAlias(executionTarget.modelId)) { + if (isModelAlias(executionTarget.modelId) || inventoryAuthoritative) { return null; } @@ -777,17 +788,33 @@ export function useResolvedAgentModelPicker({ modelProviderId: executionTarget.modelProviderId, source: "explicit", }; - }, [availableModels, session]); + }, [ + availableModels, + getProvenModelsForAgent, + isModelInventoryAuthoritative, + session, + ]); const availableDefaultModelSelection = useMemo(() => { + const provenModels = getProvenModelsForAgent(selectedAgentId); + const selectableModels = availableModels.filter((model) => { + const providerId = model.providerId ?? concreteSelectedProviderId; + return provenModels.some( + (proven) => + proven.id === model.id && + (!providerId || + !proven.providerId || + proven.providerId === providerId), + ); + }); const compatibleModels = concreteSelectedProviderId - ? availableModels.filter( + ? selectableModels.filter( (model) => !model.providerId || model.providerId === concreteSelectedProviderId, ) - : availableModels; + : selectableModels; const defaultModel = compatibleModels.find((model) => model.recommended) ?? compatibleModels[0]; @@ -802,7 +829,13 @@ export function useResolvedAgentModelPicker({ modelProviderId: defaultModel.providerId ?? selectedProvider, source: defaultModel.recommended ? "default" : "explicit", }; - }, [availableModels, concreteSelectedProviderId, selectedProvider]); + }, [ + availableModels, + concreteSelectedProviderId, + getProvenModelsForAgent, + selectedAgentId, + selectedProvider, + ]); const fallbackModelSelection = session ? null @@ -817,6 +850,8 @@ export function useResolvedAgentModelPicker({ pickerAgents, availableModels, getModelsForAgent, + getProvenModelsForAgent, + isModelInventoryAuthoritative, modelsLoading, modelStatusMessage, handleProviderChange, diff --git a/src/features/chat/lib/__tests__/sessionActivation.test.ts b/src/features/chat/lib/__tests__/sessionActivation.test.ts index 08641cc9f..7a112fe4d 100644 --- a/src/features/chat/lib/__tests__/sessionActivation.test.ts +++ b/src/features/chat/lib/__tests__/sessionActivation.test.ts @@ -37,6 +37,7 @@ const resolvePath = vi.hoisted(() => vi.fn()); const checkDirectoriesExist = vi.hoisted(() => vi.fn()); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpGetSessionInfo: (...args: unknown[]) => acpGetSessionInfo(...args), acpLoadSession: (...args: unknown[]) => acpLoadSession(...args), acpPrepareSession: (...args: unknown[]) => acpPrepareSession(...args), @@ -612,7 +613,8 @@ describe("loadSessionMessages", () => { "s-selection-race", "databricks_v2", "/resolved/existing/session", - { modelId: "goose-gpt-5-6-sol" }, + { modelId: "goose-gpt-5-6-sol", selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); expect(acpLoadSession.mock.invocationCallOrder[0]).toBeLessThan( acpPrepareSession.mock.invocationCallOrder[0], diff --git a/src/features/chat/lib/__tests__/sessionWorkspaceCleanup.test.ts b/src/features/chat/lib/__tests__/sessionWorkspaceCleanup.test.ts index 8216387f1..0c0df1745 100644 --- a/src/features/chat/lib/__tests__/sessionWorkspaceCleanup.test.ts +++ b/src/features/chat/lib/__tests__/sessionWorkspaceCleanup.test.ts @@ -23,6 +23,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpListSessionsPage: mocks.acpListSessionsPage, })); diff --git a/src/features/chat/lib/__tests__/steerCore.test.ts b/src/features/chat/lib/__tests__/steerCore.test.ts index 8f5602679..204458370 100644 --- a/src/features/chat/lib/__tests__/steerCore.test.ts +++ b/src/features/chat/lib/__tests__/steerCore.test.ts @@ -5,6 +5,7 @@ import { MAX_PROMPT_ATTACHMENT_BYTES } from "../attachmentPayloadBudget"; const mockAcpSteerMessage = vi.fn(); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSteerMessage: (...args: unknown[]) => mockAcpSteerMessage(...args), })); diff --git a/src/features/chat/lib/queuedSessionSend.test.ts b/src/features/chat/lib/queuedSessionSend.test.ts index bfdb2777b..cde53a408 100644 --- a/src/features/chat/lib/queuedSessionSend.test.ts +++ b/src/features/chat/lib/queuedSessionSend.test.ts @@ -31,6 +31,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpGetSessionInfo: (...args: unknown[]) => mocks.acpGetSessionInfo(...args), acpLoadSession: (...args: unknown[]) => mocks.acpLoadSession(...args), acpPrepareSession: (...args: unknown[]) => mocks.acpPrepareSession(...args), @@ -86,6 +87,13 @@ const EXECUTION_TARGET = { modelName: "GPT-6 Berd", } as const; +const NEWER_EXECUTION_TARGET = { + harnessId: "goose", + modelProviderId: "anthropic", + modelId: "claude-newer", + modelName: "Claude Newer", +} as const; + const PROJECT: ProjectInfo = { id: "project-1", path: "/tmp/project-source", @@ -196,6 +204,51 @@ describe("sendQueuedPromptToExistingSessionInBackground telemetry", () => { mocks.listSkills.mockResolvedValue([]); }); + it("dispatches a queued target snapshot after the live session changes", async () => { + const queued = releasedRecord({ + persona: { kind: "none" }, + sendOptions: { sessionSelection: EXECUTION_TARGET }, + }); + useChatSessionStore + .getState() + .replaceSessionExecutionTarget(SESSION_ID, NEWER_EXECUTION_TARGET); + let targetAtPromptDispatch: unknown; + let providerAtPromptDispatch: unknown; + mocks.acpSendMessage.mockImplementationOnce((...args: unknown[]) => { + targetAtPromptDispatch = useChatSessionStore + .getState() + .getSession(SESSION_ID)?.executionTarget; + providerAtPromptDispatch = useChatStore + .getState() + .getSessionRuntime(SESSION_ID).pendingAssistantProviderId; + const options = args[2] as + | { onPromptDispatching?: () => void; onPromptDispatched?: () => void } + | undefined; + options?.onPromptDispatching?.(); + options?.onPromptDispatched?.(); + return Promise.resolve(undefined); + }); + + await sendQueuedPromptToExistingSessionInBackground(SESSION_ID, queued); + + expect(mocks.acpPrepareSession).toHaveBeenCalledWith( + SESSION_ID, + EXECUTION_TARGET.modelProviderId, + "/tmp/project", + expect.objectContaining({ + modelId: EXECUTION_TARGET.modelId, + selectionAlreadyResolved: true, + }), + expect.anything(), + ); + expect(targetAtPromptDispatch).toEqual(EXECUTION_TARGET); + expect(providerAtPromptDispatch).toBe(EXECUTION_TARGET.modelProviderId); + expect(mocks.acpSendMessage).toHaveBeenCalledTimes(1); + expect( + useChatSessionStore.getState().getSession(SESSION_ID)?.executionTarget, + ).toEqual(NEWER_EXECUTION_TARGET); + }); + it("emits Session Started and Message Sent exactly once, at the user-message commit", async () => { // Captured inside the transport mock: the commit has not happened yet // when the transport is invoked, so nothing may have fired by then. diff --git a/src/features/chat/lib/queuedSessionSend.ts b/src/features/chat/lib/queuedSessionSend.ts index cd8ba53a5..5b92c3fed 100644 --- a/src/features/chat/lib/queuedSessionSend.ts +++ b/src/features/chat/lib/queuedSessionSend.ts @@ -117,6 +117,7 @@ async function hydrateSessionForBackgroundSend( export async function acquireExistingSessionForBackgroundSend( sessionId: string, + targetOverride?: SessionExecutionTarget, ) { const sessionBeforeHydration = useChatSessionStore .getState() @@ -140,7 +141,7 @@ export async function acquireExistingSessionForBackgroundSend( // other senders see contention and wait for the release instead of // dispatching into it. Hydration under a held lease is expected — the target // coordinator either absorbs a matching observation or defers it to release. - const acquisition = acquireSessionDispatchTarget(sessionId); + const acquisition = acquireSessionDispatchTarget(sessionId, targetOverride); if (acquisition.status === "unresolved") { // The store holds no execution target yet, so there is nothing to lease: // the `session/load` replay is what hydrates the target for a session @@ -151,7 +152,7 @@ export async function acquireExistingSessionForBackgroundSend( if (!(await hydrateSessionForBackgroundSend(sessionId))) { return { status: "session-missing" } as const; } - return acquireSessionDispatchTarget(sessionId); + return acquireSessionDispatchTarget(sessionId, targetOverride); } if (acquisition.status !== "acquired") { return acquisition; @@ -278,7 +279,11 @@ export async function sendQueuedPromptToExistingSessionInBackground( return targetPath; }; assertAgentBuilderPreparationReady(); - const acquisition = await acquireExistingSessionForBackgroundSend(sessionId); + const { payload } = queuedMessage; + const acquisition = await acquireExistingSessionForBackgroundSend( + sessionId, + payload.sendOptions?.sessionSelection, + ); if (acquisition.status === "contended") { throw new SessionDispatchContentionError(acquisition.waiter); } @@ -293,7 +298,6 @@ export async function sendQueuedPromptToExistingSessionInBackground( } const targetLease = acquisition; try { - const { payload } = queuedMessage; const payloadPersonaIntent = payload.persona; const payloadPersona = payloadPersonaIntent.kind === "persona" diff --git a/src/features/chat/lib/sendCore.test.ts b/src/features/chat/lib/sendCore.test.ts index 167bf5476..2f7b47e57 100644 --- a/src/features/chat/lib/sendCore.test.ts +++ b/src/features/chat/lib/sendCore.test.ts @@ -9,6 +9,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSendMessage: (...args: unknown[]) => mocks.acpSendMessage(...args), })); diff --git a/src/features/chat/lib/sessionModelPreference.test.ts b/src/features/chat/lib/sessionModelPreference.test.ts index db4b3d4e6..d561dc3e0 100644 --- a/src/features/chat/lib/sessionModelPreference.test.ts +++ b/src/features/chat/lib/sessionModelPreference.test.ts @@ -95,6 +95,19 @@ describe("resolveSessionModelPreference", () => { }); }); + it("drops a stored model when an authoritative inventory is empty", () => { + expect( + sanitizeSessionModelPreference( + { + providerId: "openai", + modelId: "gpt-5.4", + modelName: "GPT-5.4", + }, + { models: [] }, + ), + ).toEqual({ providerId: "openai" }); + }); + it("drops a stored model when the provider model list no longer contains it", () => { expect( sanitizeSessionModelPreference( diff --git a/src/features/chat/lib/sessionModelPreference.ts b/src/features/chat/lib/sessionModelPreference.ts index 80ab31deb..d0c65a980 100644 --- a/src/features/chat/lib/sessionModelPreference.ts +++ b/src/features/chat/lib/sessionModelPreference.ts @@ -67,10 +67,6 @@ export function sanitizeSessionModelPreference( return preference; } - if (providerModels.models.length === 0) { - return preference; - } - if (providerModels.models.some((model) => model.id === preference.modelId)) { return preference; } diff --git a/src/features/chat/lib/sessionTargetCoordinator.test.ts b/src/features/chat/lib/sessionTargetCoordinator.test.ts index 93854799b..640c63d48 100644 --- a/src/features/chat/lib/sessionTargetCoordinator.test.ts +++ b/src/features/chat/lib/sessionTargetCoordinator.test.ts @@ -16,7 +16,17 @@ import { } from "./sessionTargetCoordinator"; const mockPrepare = vi.fn(); +const configurationIntentEvents: string[] = []; +let nextConfigurationIntent = 0; vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => { + const id = ++nextConfigurationIntent; + configurationIntentEvents.push(`reserve:${id}`); + return { + sequence: id, + clear: () => configurationIntentEvents.push(`clear:${id}`), + }; + }, acpPrepareSession: (...args: unknown[]) => mockPrepare(...args), })); @@ -47,6 +57,8 @@ const reasoningEffort = { describe("session target coordinator", () => { beforeEach(() => { vi.clearAllMocks(); + configurationIntentEvents.length = 0; + nextConfigurationIntent = 0; resetSessionTargetCoordinatorsForTests(); useChatSessionStore.setState({ sessions: [ @@ -126,9 +138,44 @@ describe("session target coordinator", () => { target: target("c"), }); expect(mockPrepare).toHaveBeenCalledTimes(1); - expect(mockPrepare).toHaveBeenCalledWith("s", "openai", "/w", { - modelId: "c", + expect(mockPrepare).toHaveBeenCalledWith( + "s", + "openai", + "/w", + { modelId: "c", selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), + ); + }); + + it("reserves the replacement before releasing superseded transition ownership", async () => { + const firstWire = deferred(); + mockPrepare + .mockReturnValueOnce(firstWire.promise) + .mockResolvedValueOnce(undefined); + + const first = transitionSessionTarget({ + sessionId: "s", + target: target("b"), + workingDir: "/w", + }); + await vi.waitFor(() => expect(mockPrepare).toHaveBeenCalledTimes(1)); + + const replacement = transitionSessionTarget({ + sessionId: "s", + target: target("c"), + workingDir: "/w", }); + + expect(configurationIntentEvents.slice(0, 2)).toEqual([ + "reserve:1", + "reserve:2", + ]); + firstWire.resolve(); + await expect(first).resolves.toMatchObject({ status: "superseded" }); + await expect(replacement).resolves.toMatchObject({ status: "committed" }); + expect(configurationIntentEvents.indexOf("reserve:2")).toBeLessThan( + configurationIntentEvents.indexOf("clear:1"), + ); }); it("prevents an on-wire stale operation from committing over the winner", async () => { @@ -405,10 +452,17 @@ describe("session target coordinator", () => { requireReasoningEffort: true, }); - expect(mockPrepare).toHaveBeenCalledWith("s", "openai", "/w", { - modelId: "a", - forceConfigRefresh: true, - }); + expect(mockPrepare).toHaveBeenCalledWith( + "s", + "openai", + "/w", + { + modelId: "a", + forceConfigRefresh: true, + selectionAlreadyResolved: true, + }, + expect.objectContaining({ clear: expect.any(Function) }), + ); }); it("defers external hydration until the dispatch lease releases", () => { diff --git a/src/features/chat/lib/sessionTargetCoordinator.ts b/src/features/chat/lib/sessionTargetCoordinator.ts index 4720276b2..69f8b855c 100644 --- a/src/features/chat/lib/sessionTargetCoordinator.ts +++ b/src/features/chat/lib/sessionTargetCoordinator.ts @@ -1,4 +1,7 @@ -import { acpPrepareSession } from "@/shared/api/acp"; +import { + acpPrepareSession, + reserveAcpSessionConfiguration, +} from "@/shared/api/acp"; import type { AcpModelConfigSnapshot, AcpReasoningEffortConfigSnapshot, @@ -61,6 +64,7 @@ interface PendingOperation { selectionAtRequest?: SessionTargetSelection; targetAtRequest?: SessionExecutionTarget; settled: boolean; + intent: ReturnType; resolve: (outcome: SessionTargetOutcome) => void; } @@ -268,7 +272,7 @@ async function execute( actor: SessionActor, operation: PendingOperation, ): Promise { - const { request, operationId } = operation; + const { request, operationId, intent } = operation; try { const effective = await resolveEffectiveTarget(request.target); const liveTarget = useChatSessionStore @@ -325,9 +329,10 @@ async function execute( throw new Error("Session execution target requires a provider boundary."); } const forceConfigRefresh = - request.requireReasoningEffort && - !useChatSessionStore.getState().getSession(request.sessionId) - ?.reasoningEffort; + (request.requireReasoningEffort && + !useChatSessionStore.getState().getSession(request.sessionId) + ?.reasoningEffort) || + (request.target.modelId !== undefined && effective.modelId === undefined); const snapshot = await acpPrepareSession( request.sessionId, selection.providerId, @@ -335,10 +340,12 @@ async function execute( { ...(selection.modelId ? { modelId: selection.modelId } : {}), ...(forceConfigRefresh ? { forceConfigRefresh: true } : {}), + selectionAlreadyResolved: true, ...(request.operationId || request.requestId ? { requestId: operationId } : {}), }, + intent, ); if (!currentOperation(actor, operation)) { resolveSuperseded(actor, operation); @@ -356,11 +363,14 @@ async function execute( settleOperation(operation, { status: "session-missing", applied: false }); return; } - const acknowledged = - !effective.modelId && snapshot?.model - ? (materializeSessionExecutionModel(effective, snapshot.model) ?? - effective) - : effective; + // The ACP response is the acknowledgement of the configuration actually + // applied. Always reconcile from it: inventory can change between + // preflight resolution and preparation, and a provider reset can select a + // replacement model. + const acknowledged = snapshot?.model + ? (materializeSessionExecutionModel(effective, snapshot.model) ?? + effective) + : effective; const legacyIntent = actor.selection ? { requestId: actor.selection.operationId, @@ -441,6 +451,8 @@ async function execute( error, fallback, }); + } finally { + intent.clear(); } } @@ -493,8 +505,14 @@ function requestSessionTargetTransition( }); const outcome = new Promise((resolve) => { const previous = actor.latest; - if (previous && previous !== actor.current) + // Reserve the replacement before releasing the superseded operation. The + // registry atomically transfers pending ownership to this intent, so a + // prompt waiter can never observe an unowned A-to-B handoff. + const intent = reserveAcpSessionConfiguration(request.sessionId); + if (previous && previous !== actor.current) { + previous.intent.clear(); settleOperation(previous, { status: "superseded", applied: false }); + } actor.latest = { sequence, request, @@ -502,6 +520,7 @@ function requestSessionTargetTransition( selectionAtRequest, targetAtRequest, settled: false, + intent, resolve, }; }); @@ -1219,6 +1238,7 @@ export function transferSessionTargetOwnership( ), ); for (const operation of pending) { + operation.intent.clear(); settleOperation(operation, { status: "superseded", applied: false }); } source.current = undefined; @@ -1247,6 +1267,7 @@ export function cancelSessionTarget(sessionId: string): void { ), ); for (const operation of pending) { + operation.intent.clear(); settleOperation(operation, { status: "session-missing", applied: false }); } actor.latest = undefined; diff --git a/src/features/chat/lib/sessionTargetTransition.integration.test.ts b/src/features/chat/lib/sessionTargetTransition.integration.test.ts index 43b94ebdc..1512eb505 100644 --- a/src/features/chat/lib/sessionTargetTransition.integration.test.ts +++ b/src/features/chat/lib/sessionTargetTransition.integration.test.ts @@ -5,6 +5,42 @@ const mockLoadSession = vi.fn(); const mockSetProvider = vi.fn(); const mockSetModel = vi.fn(); const mockGetClient = vi.fn(); +const noRequestProviderContext = { + requestId: undefined, + canPublish: expect.any(Function), +}; + +const noRequestModelContext = (providerId: string) => ({ + providerId, + requestId: undefined, + canPublish: expect.any(Function), +}); + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} + +function executionConfigResponse(providerId: string, modelId: string) { + return { + configOptions: [ + { + id: "provider", + kind: { type: "select", currentValue: providerId, options: [] }, + }, + { + id: "model", + category: "model", + kind: { type: "select", currentValue: modelId, options: [] }, + }, + ], + }; +} vi.mock("@/shared/api/acpApi", () => ({ loadSession: (...args: unknown[]) => mockLoadSession(...args), @@ -117,12 +153,12 @@ describe("transitionSessionTarget with managed Goose models", () => { expect(mockSetProvider).toHaveBeenCalledWith( "legacy-session", "databricks_v2", - { requestId: undefined }, + noRequestProviderContext, ); expect(mockSetModel).toHaveBeenCalledWith( "legacy-session", "goose-gpt-5-5", - { providerId: "databricks_v2", requestId: undefined }, + noRequestModelContext("databricks_v2"), ); expect(mockSetModel).not.toHaveBeenCalledWith("legacy-session", "goose"); }); @@ -245,6 +281,11 @@ describe("transitionSessionTarget with managed Goose models", () => { "./sessionTargetCoordinator" ); + mockSetProvider.mockResolvedValueOnce({ + model: { modelId: "backend-fallback", modelName: "Backend fallback" }, + reasoningEffort: null, + }); + await expect( transitionSessionTarget({ sessionId: "no-default-session", @@ -261,18 +302,149 @@ describe("transitionSessionTarget with managed Goose models", () => { resolvedTarget: { harnessId: "goose", modelProviderId: "databricks_v2", + modelId: "backend-fallback", }, }); + expect(mockSetProvider).toHaveBeenCalledWith( + "no-default-session", + "databricks_v2", + noRequestProviderContext, + ); expect(mockSetModel).not.toHaveBeenCalled(); + expect(mockGetClient).toHaveBeenCalledTimes(1); expect( useChatSessionStore.getState().getSession("no-default-session"), ).toMatchObject({ executionTarget: { harnessId: "goose", modelProviderId: "databricks_v2", + modelId: "backend-fallback", + }, + }); + const { requireSessionInvocationSelection } = await import( + "@/shared/api/acpSessionRegistry" + ); + expect(requireSessionInvocationSelection("no-default-session")).toEqual({ + providerId: "databricks_v2", + modelId: "backend-fallback", + }); + }); + + it("suppresses a concurrent load while migration proof is pending", async () => { + const supportedModels = deferred<{ models: string[] }>(); + const supportedModelsList = vi + .fn() + .mockReturnValue(supportedModels.promise); + mockGetClient.mockResolvedValue({ + goose: { GooseUnstableProvidersSupportedModelsList: supportedModelsList }, + }); + mockLoadSession.mockResolvedValueOnce( + executionConfigResponse("legacy-provider", "legacy-model"), + ); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "@/shared/api/acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { resetManagedModelSelectionRepairCacheForTests } = await import( + "@/features/providers/lib/managedModelSelectionRepair" + ); + resetManagedModelSelectionRepairCacheForTests(); + const { acpLoadSession } = await import("@/shared/api/acp"); + const { transitionSessionTarget } = await import( + "./sessionTargetCoordinator" + ); + + const transition = transitionSessionTarget({ + sessionId: "migration-proof-session", + target: { + harnessId: "goose", + modelProviderId: "legacy-provider", + modelId: "legacy-model", + modelName: "Legacy model", + }, + workingDir: "/tmp/project", + }); + await vi.waitFor(() => + expect(supportedModelsList).toHaveBeenCalledTimes(1), + ); + + await acpLoadSession("migration-proof-session", "/tmp/project"); + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + + supportedModels.resolve({ models: ["goose-gpt-5-5"] }); + await expect(transition).resolves.toMatchObject({ + applied: true, + resolvedTarget: { + harnessId: "goose", + modelProviderId: "databricks_v2", + modelId: "goose-gpt-5-5", }, }); + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + }); + + it("materializes the prepared model for a matching provider-only transition", async () => { + const sessionId = "provider-only-prepared-session"; + const { useChatSessionStore } = await import( + "@/features/chat/stores/chatSessionStore" + ); + useChatSessionStore.setState({ + sessions: [ + { + id: sessionId, + title: "Prepared session", + executionTarget: { + harnessId: "goose", + modelProviderId: "databricks_v2", + }, + createdAt: "2026-08-01T00:00:00.000Z", + updatedAt: "2026-08-01T00:00:00.000Z", + messageCount: 1, + }, + ], + }); + const registry = await import("@/shared/api/acpSessionRegistry"); + registry.registerPreparedSession( + sessionId, + "databricks_v2", + "/tmp/project", + "goose-gpt-5-5", + ); + const { transitionSessionTarget } = await import( + "./sessionTargetCoordinator" + ); + + await expect( + transitionSessionTarget({ + sessionId, + target: { + harnessId: "goose", + modelProviderId: "databricks_v2", + }, + workingDir: "/tmp/project", + }), + ).resolves.toMatchObject({ + status: "committed", + target: { + modelProviderId: "databricks_v2", + modelId: "goose-gpt-5-5", + }, + }); + + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + expect( + useChatSessionStore.getState().getSession(sessionId)?.executionTarget, + ).toMatchObject({ + modelProviderId: "databricks_v2", + modelId: "goose-gpt-5-5", + }); + expect(registry.requireSessionInvocationSelection(sessionId)).toEqual({ + providerId: "databricks_v2", + modelId: "goose-gpt-5-5", + }); }); it("finishes on the explicitly selected model instead of the managed default", async () => { @@ -296,13 +468,13 @@ describe("transitionSessionTarget with managed Goose models", () => { expect(mockSetProvider).toHaveBeenCalledWith( "managed-opus-session", "databricks_v2", - { requestId: undefined }, + noRequestProviderContext, ); expect(mockSetModel).toHaveBeenCalledOnce(); expect(mockSetModel).toHaveBeenCalledWith( "managed-opus-session", "goose-claude-opus-4-8", - { providerId: "databricks_v2", requestId: undefined }, + noRequestModelContext("databricks_v2"), ); }); }); diff --git a/src/features/chat/lib/sessionTargetTransition.test.ts b/src/features/chat/lib/sessionTargetTransition.test.ts index 40c6c7d81..958c4c4ef 100644 --- a/src/features/chat/lib/sessionTargetTransition.test.ts +++ b/src/features/chat/lib/sessionTargetTransition.test.ts @@ -5,6 +5,7 @@ import { resetSessionTargetCoordinatorsForTests } from "./sessionTargetCoordinat const mockAcpPrepareSession = vi.fn(); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpPrepareSession: (...args: unknown[]) => mockAcpPrepareSession(...args), })); @@ -38,7 +39,8 @@ describe("transitionSessionTarget", () => { "session-latest", "new-provider", "/new", - { modelId: "new-model" }, + { modelId: "new-model", selectionAlreadyResolved: true }, + expect.objectContaining({ clear: expect.any(Function) }), ); }); @@ -63,8 +65,10 @@ describe("transitionSessionTarget", () => { { modelId: "goose-gpt-5-6-sol", forceConfigRefresh: true, + selectionAlreadyResolved: true, requestId: "request-5-6", }, + expect.objectContaining({ clear: expect.any(Function) }), ); }); }); diff --git a/src/features/chat/stores/__tests__/chatSessionStore.test.ts b/src/features/chat/stores/__tests__/chatSessionStore.test.ts index 61c3842fb..df348d613 100644 --- a/src/features/chat/stores/__tests__/chatSessionStore.test.ts +++ b/src/features/chat/stores/__tests__/chatSessionStore.test.ts @@ -29,6 +29,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpCreateSession: (...args: unknown[]) => mocks.acpCreateSession(...args), acpListSessionsPage: (...args: unknown[]) => mocks.acpListSessionsPage(...args), @@ -700,13 +701,18 @@ describe("chatSessionStore", () => { }); }); - it("does not attach an ACP default model to an unqualified Goose harness", async () => { + it("stores the managed provider and model resolved during direct creation", async () => { mocks.acpCreateSession.mockResolvedValue({ sessionId: "acp-1", configOptionsSnapshot: { - model: { modelId: "gpt-5.5", modelName: "GPT-5.5" }, + model: { modelId: "goose-gpt-5-5", modelName: "GPT-5.5" }, reasoningEffort: null, }, + resolvedSelection: { + providerId: "databricks_v2", + modelId: "goose-gpt-5-5", + modelName: "GPT-5.5", + }, }); const session = await useChatSessionStore.getState().createSession({ @@ -714,7 +720,15 @@ describe("chatSessionStore", () => { workingDir: "/tmp/project", }); - expect(session.executionTarget).toEqual({ harnessId: "goose" }); + expect(session.executionTarget).toEqual({ + harnessId: "goose", + modelProviderId: "databricks_v2", + modelId: "goose-gpt-5-5", + modelName: "GPT-5.5", + }); + expect( + useChatSessionStore.getState().getSession("acp-1")?.executionTarget, + ).toEqual(session.executionTarget); }); it("seeds reasoning effort from ACP session creation config", async () => { diff --git a/src/features/chat/stores/chatSessionStore.ts b/src/features/chat/stores/chatSessionStore.ts index 811c297bd..32407e5d8 100644 --- a/src/features/chat/stores/chatSessionStore.ts +++ b/src/features/chat/stores/chatSessionStore.ts @@ -34,12 +34,14 @@ import { removePersistedChatWorkspaceMetadata, } from "./workspaceAttachmentPersistence"; import { - materializeSessionExecutionModel, normalizeSessionExecutionTarget, sameSessionExecutionTarget, type SessionExecutionTarget, } from "@/features/chat/lib/sessionExecutionTarget"; -import { gooseServeSelectionFromExecutionTarget } from "@/features/chat/lib/gooseServeExecutionTarget"; +import { + executionTargetFromGooseServeBoundary, + gooseServeSelectionFromExecutionTarget, +} from "@/features/chat/lib/gooseServeExecutionTarget"; const RIGHT_RAIL_OPEN_STORAGE_KEY = "goose:right-rail-open"; const LEGACY_CONTEXT_PANEL_OPEN_STORAGE_KEY = "goose:context-panel-open"; @@ -513,29 +515,23 @@ export const useChatSessionStore = create((set, get) => ({ ); const providerId = gooseServeSelection.providerId ?? "goose"; const requestedModelId = requestedExecutionTarget.modelId; - const { sessionId, configOptionsSnapshot } = await acpCreateSession( - providerId, - opts.workingDir, - { + const { sessionId, configOptionsSnapshot, resolvedSelection } = + await acpCreateSession(providerId, opts.workingDir, { personaId: opts.personaId, modelId: requestedModelId, projectId: opts.projectId, deferProviderSetup: opts.deferProviderSetup ?? requestedModelId == null, - }, - ); + }); logReasoningEffortInfo("createSession acp resolved", { sessionId: shortLogId(sessionId), providerId, modelId: requestedModelId ?? null, hasReasoningEffort: Boolean(configOptionsSnapshot?.reasoningEffort), }); - const executionTarget = - !requestedModelId && configOptionsSnapshot?.model - ? (materializeSessionExecutionModel( - requestedExecutionTarget, - configOptionsSnapshot.model, - ) ?? requestedExecutionTarget) - : requestedExecutionTarget; + const executionTarget = executionTargetFromGooseServeBoundary( + resolvedSelection ?? gooseServeSelection, + requestedExecutionTarget, + ); const chatSession: ChatSession = withWorkspaceBackfill({ id: sessionId, title: opts.title ?? DEFAULT_CHAT_TITLE, diff --git a/src/features/chat/ui/AgentModelPicker.tsx b/src/features/chat/ui/AgentModelPicker.tsx index f88687bd2..8ce856b01 100644 --- a/src/features/chat/ui/AgentModelPicker.tsx +++ b/src/features/chat/ui/AgentModelPicker.tsx @@ -55,6 +55,8 @@ interface AgentModelPickerProps { loading?: boolean; isCompact?: boolean; showSelectedModelInTrigger?: boolean; + /** A provider-only target must not synthesize a default model for display. */ + showDefaultModelInTrigger?: boolean; triggerTabIndex?: number; triggerIconOnly?: boolean; open?: boolean; @@ -175,6 +177,7 @@ export function AgentModelPicker({ loading = false, isCompact = false, showSelectedModelInTrigger = true, + showDefaultModelInTrigger = true, triggerTabIndex, triggerIconOnly = false, open: controlledOpen, @@ -275,15 +278,16 @@ export function AgentModelPicker({ displayModelLabel, selectedAgentId, ]); - const triggerLabel = showSelectedModelInTrigger - ? resolvePickerTriggerLabel({ - currentModelId, - currentModelName, - currentModelProviderId, - availableModels: displayedModels, - selectedAgentLabel, - }) - : selectedAgentLabel; + const triggerLabel = + showSelectedModelInTrigger && (currentModelId || showDefaultModelInTrigger) + ? resolvePickerTriggerLabel({ + currentModelId, + currentModelName, + currentModelProviderId, + availableModels: displayedModels, + selectedAgentLabel, + }) + : selectedAgentLabel; const triggerTitle = triggerLabel ?? (loading ? t("toolbar.loading") : undefined); const triggerButtonSize = triggerIconOnly ? "icon-pill-sm" : "sm"; diff --git a/src/features/home/ui/HomeScreen.test.tsx b/src/features/home/ui/HomeScreen.test.tsx index bd56861a7..64ad3b1b5 100644 --- a/src/features/home/ui/HomeScreen.test.tsx +++ b/src/features/home/ui/HomeScreen.test.tsx @@ -127,6 +127,7 @@ vi.mock("@/features/chat/hooks/useMentionHandlers", () => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), discoverAcpProviders: vi.fn().mockResolvedValue([ { id: "goose", label: "Goose" }, { id: "claude-acp", label: "Claude Code" }, diff --git a/src/features/providers/defaultProviderConfig.test.ts b/src/features/providers/defaultProviderConfig.test.ts index ab8e429cd..b72026d89 100644 --- a/src/features/providers/defaultProviderConfig.test.ts +++ b/src/features/providers/defaultProviderConfig.test.ts @@ -63,6 +63,36 @@ describe("reconcileManagedDefaultProviderSelection", () => { mockGetStoredModelPreference.mockReturnValue(null); }); + it("does not wait for model inventory for provider-only defaults", async () => { + useRuntimeConfigStore.setState({ + loaded: true, + config: managedRuntimeConfig, + result: { + status: "ready", + source: "bundledFile", + config: managedRuntimeConfig, + }, + }); + const supportedModelsList = vi.fn().mockReturnValue(new Promise(() => {})); + mockGetClient.mockResolvedValue({ + goose: { + GooseUnstableDefaultsRead: vi.fn().mockResolvedValue({ + providerId: "databricks_v2", + modelId: undefined, + }), + GooseUnstableDefaultsSave: defaultsSave, + GooseUnstableProvidersSupportedModelsList: supportedModelsList, + }, + } as never); + + await expect(reconcileManagedDefaultProviderSelection()).resolves.toEqual({ + providerId: "databricks_v2", + modelId: undefined, + }); + expect(supportedModelsList).not.toHaveBeenCalled(); + expect(defaultsSave).not.toHaveBeenCalled(); + }); + it("repairs a persisted Goose harness sentinel to the managed default", async () => { useRuntimeConfigStore.setState({ loaded: true, @@ -80,6 +110,9 @@ describe("reconcileManagedDefaultProviderSelection", () => { modelId: "goose", }), GooseUnstableDefaultsSave: defaultsSave, + GooseUnstableProvidersSupportedModelsList: vi.fn().mockResolvedValue({ + models: ["goose-gpt-5-5"], + }), }, } as never); @@ -119,6 +152,57 @@ describe("saveDefaultProviderSelection", () => { }); }); + it("does not persist an advisory recommendation excluded from live proof", async () => { + const refreshProviderModels = vi.fn().mockImplementation((providerId) => { + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + providerId, + { + providerId, + fetchedAt: Date.now(), + provenModelIds: [], + models: [{ id: "advisory", name: "Advisory", recommended: true }], + }, + ], + ]), + }); + }); + useProviderModelCacheStore.setState({ refreshProviderModels }); + + await expect(saveDefaultProviderSelection("openai")).rejects.toThrow( + "Could not load models for provider", + ); + expect(defaultsSave).not.toHaveBeenCalled(); + }); + + it("does not save a stale default after forced refresh fails", async () => { + const staleModel = { id: "gpt-4o", name: "gpt-4o", recommended: true }; + const refreshProviderModels = vi.fn().mockImplementation((providerId) => { + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + providerId, + { + providerId, + fetchedAt: Date.now(), + provenModelIds: ["gpt-4o"], + models: [staleModel], + error: "offline", + }, + ], + ]), + }); + }); + useProviderModelCacheStore.setState({ refreshProviderModels }); + + await expect(saveDefaultProviderSelection("openai")).rejects.toThrow( + "Could not prove models for provider", + ); + expect(defaultsSave).not.toHaveBeenCalled(); + expect(mockSetStoredModelPreference).not.toHaveBeenCalled(); + }); + it("saves backend defaults, local goose preference, and readiness", async () => { const refreshProviderModels = vi.fn().mockImplementation((providerId) => { useProviderModelCacheStore.setState({ @@ -128,6 +212,7 @@ describe("saveDefaultProviderSelection", () => { { providerId, fetchedAt: Date.now(), + provenModelIds: ["gpt-4o"], models: [{ id: "gpt-4o", name: "gpt-4o", recommended: true }], }, ], @@ -271,6 +356,7 @@ describe("saveDefaultProviderSelectionFromConfiguredProvider", () => { { providerId, fetchedAt: Date.now(), + provenModelIds: models.map((model) => model.id), models, }, ], diff --git a/src/features/providers/defaultProviderConfig.ts b/src/features/providers/defaultProviderConfig.ts index 58ad083af..552472c7b 100644 --- a/src/features/providers/defaultProviderConfig.ts +++ b/src/features/providers/defaultProviderConfig.ts @@ -137,9 +137,16 @@ export async function saveDefaultProviderSelection( const modelCacheStore = useProviderModelCacheStore.getState(); await modelCacheStore.refreshProviderModels(providerId, { force: true }); - const models = useProviderModelCacheStore - .getState() - .getModelsForProvider(providerId); + const cache = useProviderModelCacheStore.getState(); + if ( + cache.getError(providerId) || + !cache.isModelInventoryAuthoritative(providerId) + ) { + throw new Error( + "Could not prove models for provider. Check provider setup and try again.", + ); + } + const models = cache.getProvenModelsForProvider(providerId); const runtimeDefaultModelId = providerId === getDefaultGooseModelProviderId() ? getDefaultGooseModelId() diff --git a/src/features/providers/hooks/useNewSessionTarget.test.tsx b/src/features/providers/hooks/useNewSessionTarget.test.tsx index 72db5f68a..eaa149365 100644 --- a/src/features/providers/hooks/useNewSessionTarget.test.tsx +++ b/src/features/providers/hooks/useNewSessionTarget.test.tsx @@ -60,6 +60,7 @@ describe("useNewSessionTarget", () => { providerId: "anthropic", }, ], + provenModelIds: ["claude-sonnet-4"], fetchedAt: Date.now(), }, ], @@ -95,6 +96,70 @@ describe("useNewSessionTarget", () => { }); }); + it.each([ + { + name: "proof is absent", + provenModelIds: undefined, + expectedModelId: "removed-model", + }, + { + name: "live proof is empty", + provenModelIds: [], + expectedModelId: "goose-gpt-5-5", + }, + { + name: "live proof omits the stored model", + provenModelIds: ["claude-sonnet-4"], + expectedModelId: "goose-gpt-5-5", + }, + ])("uses $name when resolving a stored model for a new chat", async ({ + provenModelIds, + expectedModelId, + }) => { + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + "anthropic", + { + providerId: "anthropic", + models: [ + { + id: "claude-sonnet-4", + name: "Claude Sonnet 4", + providerId: "anthropic", + }, + ], + ...(provenModelIds !== undefined ? { provenModelIds } : {}), + fetchedAt: Date.now(), + }, + ], + ]), + refreshingProviderIds: new Set(), + }); + window.localStorage.setItem( + "goose:preferredModelsByAgent", + JSON.stringify({ + goose: { + modelId: "removed-model", + modelName: "Removed model", + providerId: "anthropic", + }, + }), + ); + + const { result } = renderHook(() => useNewSessionTarget()); + let target: Awaited> | undefined; + await act(async () => { + target = await result.current(); + }); + + expect(target).toMatchObject({ + status: "ready", + providerId: "goose", + modelId: expectedModelId, + }); + }); + it("drops an unavailable stored model before resolving the new chat", async () => { window.localStorage.setItem( "goose:preferredModelsByAgent", diff --git a/src/features/providers/hooks/useProviderModels.test.tsx b/src/features/providers/hooks/useProviderModels.test.tsx index f0a1da316..498a94cd1 100644 --- a/src/features/providers/hooks/useProviderModels.test.tsx +++ b/src/features/providers/hooks/useProviderModels.test.tsx @@ -104,6 +104,7 @@ describe("useProviderModels", () => { { providerId: "databricks_v2", models, + provenModelIds: models.map((model) => model.id), fetchedAt: Date.now(), }, ], diff --git a/src/features/providers/hooks/useProviderModels.ts b/src/features/providers/hooks/useProviderModels.ts index 869ccfe07..2de572b4f 100644 --- a/src/features/providers/hooks/useProviderModels.ts +++ b/src/features/providers/hooks/useProviderModels.ts @@ -82,6 +82,16 @@ export function useProviderModels() { [providers], ); + const getProvenModelsForProvider = useCallback( + (providerId: string) => { + const entry = providers.get(providerId); + if (!entry?.provenModelIds) return EMPTY_MODELS; + const provenIds = new Set(entry.provenModelIds); + return entry.models.filter((model) => provenIds.has(model.id)); + }, + [providers], + ); + const isModelInventoryAuthoritative = useCallback( (providerId: string) => isCachedModelInventoryAuthoritative(providers.get(providerId)), @@ -123,6 +133,14 @@ export function useProviderModels() { ], ); + const getProvenModelsForAgent = useCallback( + (agentId: string) => + agentId === "goose" + ? configuredModelProviderIds.flatMap(getProvenModelsForProvider) + : getProvenModelsForProvider(agentId), + [configuredModelProviderIds, getProvenModelsForProvider], + ); + const isRefreshingProvider = useCallback( (providerId: string) => refreshingProviderIds.has(providerId), [refreshingProviderIds], @@ -141,6 +159,7 @@ export function useProviderModels() { modelCacheRefreshProviderIds, getModelsForAgent, getModelsForProvider, + getProvenModelsForAgent, isModelInventoryAuthoritative, refreshProviderModels, refreshAllModelProviders, diff --git a/src/features/providers/lib/managedModelSelectionRepair.test.ts b/src/features/providers/lib/managedModelSelectionRepair.test.ts index ecc4a6fdc..f2fd17937 100644 --- a/src/features/providers/lib/managedModelSelectionRepair.test.ts +++ b/src/features/providers/lib/managedModelSelectionRepair.test.ts @@ -11,6 +11,7 @@ import { notifyProviderModelInventoryInvalidated } from "./providerModelInventor vi.mock("@/shared/api/acpConnection", () => ({ getClient: vi.fn(), + invalidateClientConnection: vi.fn().mockResolvedValue(undefined), })); const managedConfig: RuntimeConfig = { @@ -62,6 +63,34 @@ describe("repairManagedGooseModelSelection", () => { }); }); + it("keeps an authoritative-empty same-provider target provider-only", async () => { + vi.mocked(getClient).mockResolvedValue({ + goose: { + GooseUnstableProvidersSupportedModelsList: vi.fn().mockResolvedValue({ + models: [], + }), + }, + } as never); + + await expect( + repairManagedGooseModelSelection( + { providerId: "databricks_v2" }, + "session", + ), + ).resolves.toEqual({ providerId: "databricks_v2", modelId: undefined }); + }); + + it("preserves same-provider model-free intent when live proof cannot be read", async () => { + vi.mocked(getClient).mockRejectedValue(new Error("offline")); + + await expect( + repairManagedGooseModelSelection( + { providerId: "databricks_v2" }, + "session", + ), + ).resolves.toEqual({ providerId: "databricks_v2", modelId: undefined }); + }); + it("repairs any model absent from the live target-provider inventory", async () => { vi.mocked(getClient).mockResolvedValue({ goose: { @@ -172,6 +201,69 @@ describe("repairManagedGooseModelSelection", () => { expect(supportedModelsList).toHaveBeenCalledTimes(2); }); + it("releases same-provider proof after stalled client acquisition", async () => { + vi.useFakeTimers(); + vi.mocked(getClient).mockReturnValue(new Promise(() => {})); + + const repair = repairManagedGooseModelSelection( + { providerId: "databricks_v2", modelId: "future-model" }, + "session", + ); + await vi.advanceTimersByTimeAsync(60_000); + await expect(repair).resolves.toEqual({ + providerId: "databricks_v2", + modelId: "future-model", + }); + + vi.mocked(getClient).mockResolvedValue({ + goose: { + GooseUnstableProvidersSupportedModelsList: vi.fn().mockResolvedValue({ + models: ["future-model"], + }), + }, + } as never); + await expect( + repairManagedGooseModelSelection( + { providerId: "databricks_v2", modelId: "future-model" }, + "session", + ), + ).resolves.toEqual({ + providerId: "databricks_v2", + modelId: "future-model", + }); + }); + + it("releases same-provider proof after stalled inventory RPC", async () => { + vi.useFakeTimers(); + const stalledInventory = new Promise<{ models: string[] }>(() => {}); + const supportedModelsList = vi + .fn() + .mockReturnValueOnce(stalledInventory) + .mockResolvedValueOnce({ models: ["future-model"] }); + vi.mocked(getClient).mockResolvedValue({ + goose: { GooseUnstableProvidersSupportedModelsList: supportedModelsList }, + } as never); + + const repair = repairManagedGooseModelSelection( + { providerId: "databricks_v2", modelId: "future-model" }, + "session", + ); + await vi.advanceTimersByTimeAsync(60_000); + await expect(repair).resolves.toEqual({ + providerId: "databricks_v2", + modelId: "future-model", + }); + await expect( + repairManagedGooseModelSelection( + { providerId: "databricks_v2", modelId: "future-model" }, + "session", + ), + ).resolves.toEqual({ + providerId: "databricks_v2", + modelId: "future-model", + }); + expect(supportedModelsList).toHaveBeenCalledTimes(2); + }); it("preserves the selected model when live inventory cannot be read", async () => { vi.mocked(getClient).mockRejectedValue(new Error("offline")); diff --git a/src/features/providers/lib/managedModelSelectionRepair.ts b/src/features/providers/lib/managedModelSelectionRepair.ts index d4a9b752c..31032fd84 100644 --- a/src/features/providers/lib/managedModelSelectionRepair.ts +++ b/src/features/providers/lib/managedModelSelectionRepair.ts @@ -1,15 +1,19 @@ import packageJson from "../../../../package.json"; -import { getClient } from "@/shared/api/acpConnection"; import { useRuntimeConfigStore } from "@/shared/runtime-config/runtimeConfigStore"; import { resolveAgentProviderCatalogIdStrict } from "@/features/providers/providerCatalog"; -import { subscribeToProviderModelInventoryInvalidation } from "./providerModelInventoryEvents"; import { + providerModelInventoryGeneration, + subscribeToProviderModelInventoryInvalidation, +} from "@/shared/runtime-config/providerModelInventoryInvalidation"; +import { publishProvenModelInventory } from "@/features/providers/stores/providerModelCacheStore"; +import { + readBoundedProvenModelInventory, resolveManagedGooseProviderSelection, + resolveValidatedManagedGooseProviderSelection, type GooseProviderSelection, type ManagedGooseProviderSelection, } from "@/shared/runtime-config/modelProviderPolicy"; -const DATABRICKS_V2_PROVIDER_ID = "databricks_v2"; const VALIDATED_INVENTORY_TTL_MS = 5 * 60 * 1000; const validatedInventories = new Map< string, @@ -19,16 +23,9 @@ const inventoryRequests = new Map< string, Promise | null> >(); -const inventoryGenerations = new Map(); - -function inventoryGeneration(providerId: string): number { - return inventoryGenerations.get(providerId) ?? 0; -} - subscribeToProviderModelInventoryInvalidation((providerId) => { validatedInventories.delete(providerId); inventoryRequests.delete(providerId); - inventoryGenerations.set(providerId, inventoryGeneration(providerId) + 1); }); export type ManagedModelRepairSource = @@ -50,25 +47,24 @@ async function validatedModelIds( const existing = inventoryRequests.get(providerId); if (existing) return existing; - const generationAtStart = inventoryGeneration(providerId); + const generationAtStart = providerModelInventoryGeneration(providerId); let request!: Promise | null>; request = (async () => { try { - const client = await getClient(); - const response = - await client.goose.GooseUnstableProvidersSupportedModelsList({ - providerId, - }); - const modelIds = new Set(response.models as string[]); - if (generationAtStart !== inventoryGeneration(providerId)) { + const modelIds = await readBoundedProvenModelInventory(providerId); + if (generationAtStart !== providerModelInventoryGeneration(providerId)) { return validatedModelIds(providerId); } validatedInventories.set(providerId, { modelIds, fetchedAt: Date.now(), }); + publishProvenModelInventory(providerId, [...modelIds]); return modelIds; } catch (error) { + if (generationAtStart !== providerModelInventoryGeneration(providerId)) { + return validatedModelIds(providerId); + } console.warn("Could not validate managed provider model inventory", { providerId, error: error instanceof Error ? error.message : String(error), @@ -102,11 +98,14 @@ export async function repairManagedGooseModelSelection( const config = useRuntimeConfigStore.getState().config; const initial = resolveManagedGooseProviderSelection(config, selection); if (!initial) return null; + if (initial.providerId !== selection.providerId) { + return resolveValidatedManagedGooseProviderSelection(config, selection); + } + if (!selection.modelId) { + return initial; + } - const targetModelIds = - initial.providerId === DATABRICKS_V2_PROVIDER_ID && selection.modelId - ? await validatedModelIds(initial.providerId) - : null; + const targetModelIds = await validatedModelIds(initial.providerId); const repaired = resolveManagedGooseProviderSelection(config, selection, { ...(targetModelIds ? { targetModelIds } : {}), targetInventoryValidated: targetModelIds !== null, @@ -132,5 +131,4 @@ export async function repairManagedGooseModelSelection( export function resetManagedModelSelectionRepairCacheForTests(): void { validatedInventories.clear(); inventoryRequests.clear(); - inventoryGenerations.clear(); } diff --git a/src/features/providers/lib/providerModelInventoryEvents.ts b/src/features/providers/lib/providerModelInventoryEvents.ts index 3c17f91e8..244fb9aff 100644 --- a/src/features/providers/lib/providerModelInventoryEvents.ts +++ b/src/features/providers/lib/providerModelInventoryEvents.ts @@ -1,19 +1,5 @@ -type ProviderModelInventoryInvalidationListener = (providerId: string) => void; - -const invalidationListeners = - new Set(); - -export function notifyProviderModelInventoryInvalidated( - providerId: string, -): void { - for (const listener of invalidationListeners) { - listener(providerId); - } -} - -export function subscribeToProviderModelInventoryInvalidation( - listener: ProviderModelInventoryInvalidationListener, -): () => void { - invalidationListeners.add(listener); - return () => invalidationListeners.delete(listener); -} +export { + notifyProviderModelInventoryInvalidated, + providerModelInventoryGeneration, + subscribeToProviderModelInventoryInvalidation, +} from "@/shared/runtime-config/providerModelInventoryInvalidation"; diff --git a/src/features/providers/lib/resolveSessionModelPreference.test.ts b/src/features/providers/lib/resolveSessionModelPreference.test.ts index cff241f2c..5c6d3f660 100644 --- a/src/features/providers/lib/resolveSessionModelPreference.test.ts +++ b/src/features/providers/lib/resolveSessionModelPreference.test.ts @@ -13,8 +13,9 @@ const mockCheckAllProviderStatus = vi.mocked(checkAllProviderStatus); function setCachedModels( providerId: string, models: string[], - fetchedAt = Date.now(), + options: { fetchedAt?: number; proven?: boolean } = {}, ) { + const { fetchedAt = Date.now(), proven = true } = options; useProviderModelCacheStore.setState({ providers: new Map([ [ @@ -22,6 +23,7 @@ function setCachedModels( { providerId, models: models.map((id) => ({ id, name: id, providerId })), + ...(proven ? { provenModelIds: models } : {}), fetchedAt, }, ], @@ -63,6 +65,7 @@ describe("resolveSupportedSessionModelPreference", () => { modelId: "gpt-5.4", }, }); + setCachedModels("openai", ["gpt-5.4"]); await expect( resolveSupportedSessionModelPreference("goose"), @@ -204,50 +207,72 @@ describe("resolveSupportedSessionModelPreference", () => { }); }); - it("preserves the selected model when the model cache has no model list", async () => { - setCachedModels("openai", []); - - await expect( - resolveSupportedSessionModelPreference("openai", "gpt-5.4"), - ).resolves.toEqual({ - providerId: "openai", - modelId: "gpt-5.4", - modelName: "gpt-5.4", - }); - }); - - it("drops an unsupported model when populated model cache is available", async () => { - setCachedModels("openai", ["gpt-5.3"]); + it.each([ + { + name: "proof is absent", + models: ["gpt-5.3"], + options: { proven: false }, + expected: { + providerId: "openai", + modelId: "gpt-5.4", + modelName: "gpt-5.4", + }, + }, + { + name: "live proof is empty", + models: [], + options: {}, + expected: { providerId: "openai" }, + }, + { + name: "live proof contains the preferred model", + models: ["gpt-5.4"], + options: {}, + expected: { + providerId: "openai", + modelId: "gpt-5.4", + modelName: "gpt-5.4", + }, + }, + { + name: "live proof omits the preferred model", + models: ["gpt-5.3"], + options: {}, + expected: { providerId: "openai" }, + }, + ])("uses only live proof when $name", async ({ + models, + options, + expected, + }) => { + setCachedModels("openai", models, options); await expect( resolveSupportedSessionModelPreference("openai", "gpt-5.4"), - ).resolves.toEqual({ - providerId: "openai", - }); + ).resolves.toEqual(expected); }); - it("preserves a selected model while a populated cache is provisional", async () => { - setCachedModels("openai", ["gpt-5.3"], 0); - - await expect( - resolveSupportedSessionModelPreference("openai", "gpt-5.4"), - ).resolves.toEqual({ - providerId: "openai", - modelId: "gpt-5.4", - modelName: "gpt-5.4", + it("rejects an advisory display candidate absent from live proof", async () => { + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + "openai", + { + providerId: "openai", + models: [ + { id: "gpt-5.3", name: "gpt-5.3", providerId: "openai" }, + { id: "gpt-5.4", name: "gpt-5.4", providerId: "openai" }, + ], + provenModelIds: ["gpt-5.3"], + fetchedAt: Date.now(), + }, + ], + ]), }); - }); - - it("keeps a supported model when populated model cache is available", async () => { - setCachedModels("openai", ["gpt-5.4"]); await expect( resolveSupportedSessionModelPreference("openai", "gpt-5.4"), - ).resolves.toEqual({ - providerId: "openai", - modelId: "gpt-5.4", - modelName: "gpt-5.4", - }); + ).resolves.toEqual({ providerId: "openai" }); }); it("drops a stored model whose provider is disconnected", async () => { @@ -272,6 +297,7 @@ describe("resolveSupportedSessionModelPreference", () => { modelId: "goose-gpt-5-5", }, }); + setCachedModels("databricks_v2", ["goose-gpt-5-5"]); mockCheckAllProviderStatus.mockResolvedValue([ { providerId: "openai", isConfigured: false }, ]); diff --git a/src/features/providers/lib/resolveSessionModelPreference.ts b/src/features/providers/lib/resolveSessionModelPreference.ts index cd6193511..dd4a5c2af 100644 --- a/src/features/providers/lib/resolveSessionModelPreference.ts +++ b/src/features/providers/lib/resolveSessionModelPreference.ts @@ -112,23 +112,31 @@ export async function resolveSupportedSessionModelPreference( }; } + const modelCache = useProviderModelCacheStore.getState(); + + // A configured default is synthesized intent. Unlike an explicit preference, + // it cannot survive without a successful inventory proof. if (providerId === "goose" && !sessionModelPreference.modelId) { - sessionModelPreference = gooseDefaultPreference() ?? sessionModelPreference; + const fallback = gooseDefaultPreference(); + if (!fallback) return sessionModelPreference; + if (!modelCache.isModelInventoryAuthoritative(fallback.providerId)) { + return { providerId }; + } + return sanitizeSessionModelPreference(fallback, { + models: modelCache.getProvenModelsForProvider(fallback.providerId), + }); } if (!sessionModelPreference.modelId) { return sessionModelPreference; } - const modelCache = useProviderModelCacheStore.getState(); - const models = modelCache.getModelsForProvider( - sessionModelPreference.providerId, - ); + const modelProviderId = sessionModelPreference.providerId; - if ( - modelCache.isModelInventoryAuthoritative(sessionModelPreference.providerId) - ) { - return sanitizeSessionModelPreference(sessionModelPreference, { models }); + if (modelCache.isModelInventoryAuthoritative(modelProviderId)) { + return sanitizeSessionModelPreference(sessionModelPreference, { + models: modelCache.getProvenModelsForProvider(modelProviderId), + }); } if (!(await isProviderDisconnected(sessionModelPreference.providerId))) { @@ -137,12 +145,13 @@ export async function resolveSupportedSessionModelPreference( if (providerId === "goose") { const fallback = gooseDefaultPreference(); - if (fallback && fallback.providerId !== sessionModelPreference.providerId) { - const fallbackModels = useProviderModelCacheStore - .getState() - .getModelsForProvider(fallback.providerId); + if ( + fallback && + fallback.providerId !== sessionModelPreference.providerId && + modelCache.isModelInventoryAuthoritative(fallback.providerId) + ) { return sanitizeSessionModelPreference(fallback, { - models: fallbackModels, + models: modelCache.getProvenModelsForProvider(fallback.providerId), }); } } diff --git a/src/features/providers/modelCacheRefresh.ts b/src/features/providers/modelCacheRefresh.ts index fa99a093c..8777240fc 100644 --- a/src/features/providers/modelCacheRefresh.ts +++ b/src/features/providers/modelCacheRefresh.ts @@ -11,7 +11,10 @@ import { getModelProviders, getModelProvidersFromEntries, } from "./providerCatalog"; -import { runtimeRefreshableModelProviderIds } from "./runtimeProviderConfig"; +import { + runtimeManagedModelProviderIds, + runtimeRefreshableModelProviderIds, +} from "./runtimeProviderConfig"; export function getModelCacheRefreshProviderIds( runtimeConfig: RuntimeConfig | null | undefined, @@ -33,10 +36,13 @@ export function getModelCacheRefreshProviderIds( ? new Set(configuredProviderIds) : null; - for (const providerId of runtimeRefreshableModelProviderIds( - runtimeConfig, - defaultModelInventoryMode, - )) { + for (const providerId of [ + ...runtimeManagedModelProviderIds(runtimeConfig, defaultModelInventoryMode), + ...runtimeRefreshableModelProviderIds( + runtimeConfig, + defaultModelInventoryMode, + ), + ]) { ids.add(providerId); } diff --git a/src/features/providers/runtimeProviderConfig.test.ts b/src/features/providers/runtimeProviderConfig.test.ts index 1a8e6dce9..c6f2b4bc7 100644 --- a/src/features/providers/runtimeProviderConfig.test.ts +++ b/src/features/providers/runtimeProviderConfig.test.ts @@ -436,10 +436,21 @@ describe("getModelCacheRefreshProviderIds", () => { ]); }); - it("excludes runtime-managed model providers from startup refresh", () => { - expect(getModelCacheRefreshProviderIds(DEFAULT_RUNTIME_CONFIG)).toEqual([ - "codex-acp", - ]); + it("includes runtime-managed model providers so live discovery can establish proof", () => { + expect( + getModelCacheRefreshProviderIds({ + ...MANAGED_RUNTIME_CONFIG, + goose: { + ...MANAGED_RUNTIME_CONFIG.goose, + modelProviders: [ + { + ...MANAGED_RUNTIME_CONFIG.goose.modelProviders[0], + modelInventoryMode: "authoritative", + }, + ], + }, + }), + ).toEqual(["databricks_v2", "codex-acp"]); }); it("includes model providers for bundled appDefault refresh", () => { diff --git a/src/features/providers/runtimeProviderConfig.ts b/src/features/providers/runtimeProviderConfig.ts index 55f2beced..06adb5d4c 100644 --- a/src/features/providers/runtimeProviderConfig.ts +++ b/src/features/providers/runtimeProviderConfig.ts @@ -164,16 +164,16 @@ function modelInventoryMode( } export function runtimeManagedModelProviderIds( - runtimeConfig: RuntimeConfig, + runtimeConfig: RuntimeConfig | null | undefined, defaultMode: RuntimeModelInventoryMode = DEFAULT_MODEL_INVENTORY_MODE, ): Set { return new Set( - runtimeConfig.goose.modelProviders + runtimeConfig?.goose.modelProviders .filter( (provider) => modelInventoryMode(provider, defaultMode) === "authoritative", ) - .map((provider) => provider.id), + .map((provider) => provider.id) ?? [], ); } diff --git a/src/features/providers/stores/providerModelCacheStore.test.ts b/src/features/providers/stores/providerModelCacheStore.test.ts index f61cf3848..945ba89a6 100644 --- a/src/features/providers/stores/providerModelCacheStore.test.ts +++ b/src/features/providers/stores/providerModelCacheStore.test.ts @@ -1,7 +1,12 @@ import { waitFor } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import type { ModelOption } from "@/features/chat/types"; -import { useProviderModelCacheStore } from "./providerModelCacheStore"; +import { + isModelSelectionAllowedByCachedInventory, + MODEL_CACHE_TTL_MS, + publishProvenModelInventory, + useProviderModelCacheStore, +} from "./providerModelCacheStore"; const mocks = vi.hoisted(() => ({ getClient: vi.fn(), @@ -41,49 +46,146 @@ describe("providerModelCacheStore", () => { }); }); - it("seeds runtime models as authoritative runtime-managed entries", async () => { + it("keeps runtime-managed configuration seeds advisory until live discovery succeeds", async () => { const model = seededModel({ contextLimit: 128000, recommended: true, featured: true, sortOrder: 0, }); - useProviderModelCacheStore .getState() .seedRuntimeModels(new Map([["databricks_v2", [model]]])); + + expect( + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative("databricks_v2"), + ).toBe(false); + expect( + useProviderModelCacheStore + .getState() + .getProvenModelsForProvider("databricks_v2"), + ).toEqual([]); + + mocks.supportedModelsList.mockResolvedValueOnce({ models: [] }); await useProviderModelCacheStore .getState() - .refreshAllModelProviders(["databricks_v2"]); - await useProviderModelCacheStore - .getState() - .refreshProviderModels("databricks_v2", { force: true }); + .refreshProviderModels("databricks_v2"); const entry = useProviderModelCacheStore .getState() .providers.get("databricks_v2"); expect(entry?.runtimeManaged).toBe(true); + expect(entry?.provenModelIds).toEqual([]); expect( useProviderModelCacheStore .getState() .getModelsForProvider("databricks_v2"), ).toEqual([model]); - expect(mocks.supportedModelsList).not.toHaveBeenCalled(); + expect(mocks.supportedModelsList).toHaveBeenCalledWith({ + providerId: "databricks_v2", + }); }); - it("preserves runtime-managed models after invalidation and forced refresh", async () => { - const model = seededModel({ - contextLimit: 128000, - recommended: true, - featured: true, - sortOrder: 0, + it("lets a newer ACP acknowledgement supersede retained negative proof", () => { + const model = seededModel({ id: "old-model" }); + const proofRevision = 10; + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + "databricks_v2", + { + providerId: "databricks_v2", + models: [model], + provenModelIds: ["old-model"], + proofRevision, + fetchedAt: Date.now() - MODEL_CACHE_TTL_MS - 1, + }, + ], + ]), + }); + + expect( + isModelSelectionAllowedByCachedInventory( + "databricks_v2", + "newly-prepared-model", + ), + ).toBe(false); + expect( + isModelSelectionAllowedByCachedInventory( + "databricks_v2", + "newly-prepared-model", + proofRevision + 1, + ), + ).toBe(true); + + useProviderModelCacheStore.setState((state) => { + const providers = new Map(state.providers); + const entry = providers.get("databricks_v2"); + if (!entry) throw new Error("expected seeded provider"); + providers.set("databricks_v2", { + ...entry, + error: "offline", + }); + return { providers }; }); + expect( + isModelSelectionAllowedByCachedInventory( + "databricks_v2", + "newly-prepared-model", + ), + ).toBe(false); + expect( + isModelSelectionAllowedByCachedInventory( + "databricks_v2", + "newly-prepared-model", + proofRevision + 1, + ), + ).toBe(true); + }); + + it("publishes successful live preflight as the shared fresh authority", () => { + publishProvenModelInventory("databricks_v2", ["new-model"]); + + expect( + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative("databricks_v2"), + ).toBe(true); + expect( + isModelSelectionAllowedByCachedInventory("databricks_v2", "new-model"), + ).toBe(true); + expect( + isModelSelectionAllowedByCachedInventory("databricks_v2", "old-model"), + ).toBe(false); + }); + it("invalidates runtime-managed proof without discarding its display seed", async () => { + const model = seededModel({ recommended: true, featured: true }); useProviderModelCacheStore .getState() .seedRuntimeModels(new Map([["databricks_v2", [model]]])); + mocks.supportedModelsList + .mockResolvedValueOnce({ models: ["goose-gpt-5-5"] }) + .mockResolvedValueOnce({ models: ["goose-gpt-5-5"] }); + + await useProviderModelCacheStore + .getState() + .refreshProviderModels("databricks_v2"); useProviderModelCacheStore.getState().invalidateProvider("databricks_v2"); + const invalidatedEntry = useProviderModelCacheStore + .getState() + .providers.get("databricks_v2"); + expect(invalidatedEntry?.provenModelIds).toBeUndefined(); + expect(invalidatedEntry?.models).toEqual([model]); + expect( + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative("databricks_v2"), + ).toBe(false); + await useProviderModelCacheStore .getState() .refreshProviderModels("databricks_v2", { force: true }); @@ -92,12 +194,16 @@ describe("providerModelCacheStore", () => { .getState() .providers.get("databricks_v2"); expect(entry?.runtimeManaged).toBe(true); + expect(entry?.provenModelIds).toEqual(["goose-gpt-5-5"]); expect( useProviderModelCacheStore .getState() - .getModelsForProvider("databricks_v2"), - ).toEqual([model]); - expect(mocks.supportedModelsList).not.toHaveBeenCalled(); + .getModelsForProvider("databricks_v2") + .map((candidate) => candidate.id), + ).toEqual(["goose-gpt-5-5", "seeded-model"]); + expect(mocks.supportedModelsList).toHaveBeenLastCalledWith({ + providerId: "databricks_v2", + }); }); it("keeps refreshable runtime models provisional until discovery succeeds", async () => { @@ -206,6 +312,12 @@ describe("providerModelCacheStore", () => { expect(models.find((model) => model.id === "goose-gpt-5-6-sol")).toEqual( expect.objectContaining(configuredModel), ); + expect( + useProviderModelCacheStore + .getState() + .getProvenModelsForProvider("databricks_v2") + .map((model) => model.id), + ).toEqual(["goose-gpt-5-5"]); }); it("keeps configured models after a failed refresh and retry", async () => { @@ -239,6 +351,50 @@ describe("providerModelCacheStore", () => { ).toEqual(["goose-gpt-5-5", "goose-gpt-5-6-sol"]); }); + it.each([ + { provenModelIds: ["supported-model"], expected: ["supported-model"] }, + { provenModelIds: [], expected: [] }, + ])("preserves prior proof data and runtime policy but revokes authority after refresh failure: $provenModelIds", async ({ + provenModelIds, + expected, + }) => { + const model = seededModel({ id: "configured-model" }); + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + "databricks_v2", + { + providerId: "databricks_v2", + models: [model], + configuredModels: [model], + provenModelIds, + fetchedAt: 123, + runtimeManaged: true, + }, + ], + ]), + runtimeManagedProviderIds: new Set(["databricks_v2"]), + }); + mocks.supportedModelsList.mockRejectedValueOnce(new Error("offline")); + + await useProviderModelCacheStore + .getState() + .refreshProviderModels("databricks_v2", { force: true }); + + const entry = useProviderModelCacheStore + .getState() + .providers.get("databricks_v2"); + expect(entry?.provenModelIds).toEqual(expected); + expect(entry?.runtimeManaged).toBe(true); + expect(entry?.configuredModels).toEqual([model]); + expect(entry?.fetchedAt).toBe(123); + expect( + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative("databricks_v2"), + ).toBe(false); + }); + it("removes stale runtime-managed providers when runtime config changes", () => { const model = seededModel(); @@ -271,6 +427,56 @@ describe("providerModelCacheStore", () => { ).toBe(true); }); + it("keeps configuration-only runtime seeds provisional across restart", () => { + const model = seededModel({ recommended: true, featured: true }); + useProviderModelCacheStore + .getState() + .seedRuntimeModels(new Map([["databricks_v2", [model]]])); + + useProviderModelCacheStore.setState({ + providers: new Map(), + runtimeManagedProviderIds: new Set(), + }); + useProviderModelCacheStore.getState().loadPersisted(); + + const entry = useProviderModelCacheStore + .getState() + .providers.get("databricks_v2"); + expect(entry?.provenModelIds).toBeUndefined(); + expect( + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative("databricks_v2"), + ).toBe(false); + }); + + it("persists authority only after a successful live response", async () => { + const model = seededModel({ recommended: true, featured: true }); + useProviderModelCacheStore + .getState() + .seedRuntimeModels(new Map([["databricks_v2", [model]]])); + mocks.supportedModelsList.mockResolvedValueOnce({ models: [] }); + + await useProviderModelCacheStore + .getState() + .refreshProviderModels("databricks_v2"); + useProviderModelCacheStore.setState({ + providers: new Map(), + runtimeManagedProviderIds: new Set(), + }); + useProviderModelCacheStore.getState().loadPersisted(); + + const entry = useProviderModelCacheStore + .getState() + .providers.get("databricks_v2"); + expect(entry?.provenModelIds).toEqual([]); + expect( + useProviderModelCacheStore + .getState() + .isModelInventoryAuthoritative("databricks_v2"), + ).toBe(true); + }); + it("runs a forced refresh after an in-flight refresh finishes", async () => { let rejectInitialRefresh!: (error: Error) => void; const initialRefresh = new Promise<{ models: string[] }>( diff --git a/src/features/providers/stores/providerModelCacheStore.ts b/src/features/providers/stores/providerModelCacheStore.ts index b69ab3361..cfaeeb17e 100644 --- a/src/features/providers/stores/providerModelCacheStore.ts +++ b/src/features/providers/stores/providerModelCacheStore.ts @@ -6,15 +6,21 @@ import { getClient } from "@/shared/api/acpConnection"; import { notifyProviderModelInventoryInvalidated } from "../lib/providerModelInventoryEvents"; const MODEL_CACHE_STORAGE_KEY = "goose:providerModelCache:v1"; -const MODEL_CACHE_TTL_MS = 5 * 60 * 1000; +export const MODEL_CACHE_TTL_MS = 5 * 60 * 1000; const inFlightRefreshes = new Map>(); const queuedForceRefreshes = new Map>(); const providerRefreshVersions = new Map(); export interface CachedProviderModels { providerId: string; + /** Display candidates: live models plus configured recommendations. */ models: ModelOption[]; + /** IDs returned by a successful live inventory response; the only proof. */ + provenModelIds?: string[]; + /** Monotonic provenance for comparing proof with a later ACP acknowledgement. */ + proofRevision?: number; fetchedAt: number; + /** Runtime configuration policy; it does not establish model proof. */ runtimeManaged?: boolean; configuredModels?: ModelOption[]; error?: string; @@ -33,6 +39,7 @@ interface ProviderModelCacheActions { options?: { fresh?: boolean; runtimeManagedProviderIds?: Set }, ) => void; getModelsForProvider: (providerId: string) => ModelOption[]; + getProvenModelsForProvider: (providerId: string) => ModelOption[]; isModelInventoryAuthoritative: (providerId: string) => boolean; getError: (providerId: string) => string | null; refreshProviderModels: ( @@ -121,18 +128,86 @@ async function fetchProviderSupportedModels( return response.models; } +function mergeDisplayModels( + discoveredModels: ModelOption[], + configuredModels: ModelOption[], +): ModelOption[] { + const configuredModelsById = new Map( + configuredModels.map((model) => [model.id, model]), + ); + const hasConfiguredFeaturedModel = configuredModels.some( + (model) => model.featured, + ); + const discoveredModelIds = new Set(discoveredModels.map((model) => model.id)); + return [ + ...discoveredModels.map((model) => ({ + ...model, + ...(hasConfiguredFeaturedModel ? { featured: false } : {}), + ...configuredModelsById.get(model.id), + })), + ...configuredModels.filter((model) => !discoveredModelIds.has(model.id)), + ]; +} + +function getProvenModels( + entry: CachedProviderModels | undefined, +): ModelOption[] { + if (!isCachedModelInventoryAuthoritative(entry) || !entry?.provenModelIds) { + return []; + } + const provenIds = new Set(entry.provenModelIds); + return entry.models.filter((model) => provenIds.has(model.id)); +} + export function isCachedModelInventoryAuthoritative( entry: CachedProviderModels | undefined, ): boolean { - return entry != null && (entry.runtimeManaged || entry.fetchedAt > 0); + return ( + entry != null && + Array.isArray(entry.provenModelIds) && + entry.error === undefined && + Date.now() - entry.fetchedAt <= MODEL_CACHE_TTL_MS + ); +} + +/** + * Return whether cached inventory still permits a concrete provider/model pair. + * Retained successful proof can disprove a selection even after it becomes + * stale or a refresh fails. A later ACP acknowledgement supersedes that older + * negative proof for the concrete prepared session only. + */ +export function isModelSelectionAllowedByCachedInventory( + providerId: string, + modelId: string, + acknowledgedProofRevision = 0, +): boolean { + const entry = useProviderModelCacheStore.getState().providers.get(providerId); + if (!entry?.provenModelIds) return true; + if (entry.provenModelIds.includes(modelId)) return true; + // A proof can disprove only selections acknowledged before that proof. A + // newer ACP acknowledgement is stronger evidence for that exact session. + return proofRevision(entry) <= acknowledgedProofRevision; } function isStale(entry: CachedProviderModels | undefined): boolean { if (!entry || !isCachedModelInventoryAuthoritative(entry)) { return true; } - return ( - !entry.runtimeManaged && Date.now() - entry.fetchedAt > MODEL_CACHE_TTL_MS + return Date.now() - entry.fetchedAt > MODEL_CACHE_TTL_MS; +} + +function proofRevision(entry: CachedProviderModels | undefined): number { + return entry?.proofRevision ?? entry?.fetchedAt ?? 0; +} + +function nextProofRevision(entry: CachedProviderModels | undefined): number { + return Math.max(Date.now(), proofRevision(entry) + 1); +} + +/** Provenance of the latest successful inventory observed for a provider. */ +export function getModelInventoryProofRevision(providerId: string): number { + return proofRevision( + useProviderModelCacheStore.getState().providers.get(providerId), ); } @@ -145,6 +220,35 @@ function bumpRefreshVersion(providerId: string): void { notifyProviderModelInventoryInvalidated(providerId); } +/** Publish successful live proof into the one shared inventory authority. */ +export function publishProvenModelInventory( + providerId: string, + modelIds: readonly string[], +): void { + const state = useProviderModelCacheStore.getState(); + const existing = state.providers.get(providerId); + const discoveredModels = providerModelOptionsFromIds(providerId, [ + ...modelIds, + ]); + const configuredModels = existing?.configuredModels ?? []; + // A successful preflight is newer proof than any refresh already in flight. + // Supersede only the store's refresh writer; publishing proof is not an + // invalidation and must not evict the preflight cache that produced it. + providerRefreshVersions.set(providerId, refreshVersion(providerId) + 1); + const providers = new Map(state.providers); + providers.set(providerId, { + providerId, + models: mergeDisplayModels(discoveredModels, configuredModels), + provenModelIds: [...modelIds], + proofRevision: nextProofRevision(existing), + fetchedAt: Date.now(), + ...(existing?.runtimeManaged ? { runtimeManaged: true } : {}), + ...(configuredModels.length > 0 ? { configuredModels } : {}), + }); + useProviderModelCacheStore.setState({ providers }); + persistModels(providers); +} + export const useProviderModelCacheStore = create( (set, get) => ({ ...readPersistedProviderState(), @@ -166,15 +270,25 @@ export const useProviderModelCacheStore = create( for (const providerId of runtimeProviderIds) { bumpRefreshVersion(providerId); - const models = modelsByProviderId.get(providerId) ?? []; + const configuredModels = modelsByProviderId.get(providerId) ?? []; const runtimeManaged = runtimeManagedProviderIds.has(providerId); + const existing = providers.get(providerId); + const provenModels = getProvenModels(existing); + const provenModelIds = existing?.provenModelIds; + const hasLiveProof = Array.isArray(provenModelIds); providers.set(providerId, { providerId, - models, - fetchedAt: runtimeManaged || options.fresh ? Date.now() : 0, - ...(runtimeManaged - ? { runtimeManaged } - : { configuredModels: models }), + // Every runtime seed is advisory, including providers whose + // connection policy is runtime-managed. A prior successful live + // response stays proof, but the seed can neither create nor renew it. + models: mergeDisplayModels(provenModels, configuredModels), + fetchedAt: existing?.fetchedAt ?? 0, + configuredModels, + ...(hasLiveProof ? { provenModelIds } : {}), + ...(existing?.proofRevision !== undefined + ? { proofRevision: existing.proofRevision } + : {}), + ...(runtimeManaged ? { runtimeManaged } : {}), }); if (runtimeManaged) { nextRuntimeManagedProviderIds.add(providerId); @@ -202,6 +316,13 @@ export const useProviderModelCacheStore = create( getModelsForProvider: (providerId) => get().providers.get(providerId)?.models ?? [], + getProvenModelsForProvider: (providerId) => { + const entry = get().providers.get(providerId); + if (!entry?.provenModelIds) return []; + const provenIds = new Set(entry.provenModelIds); + return entry.models.filter((model) => provenIds.has(model.id)); + }, + isModelInventoryAuthoritative: (providerId) => isCachedModelInventoryAuthoritative(get().providers.get(providerId)), @@ -210,12 +331,6 @@ export const useProviderModelCacheStore = create( refreshProviderModels: async (providerId, options = {}) => { const current = get(); const existing = current.providers.get(providerId); - if ( - existing?.runtimeManaged || - current.runtimeManagedProviderIds.has(providerId) - ) { - return; - } if (!options.force && !isStale(existing)) { return; } @@ -260,29 +375,14 @@ export const useProviderModelCacheStore = create( const ids = await fetchProviderSupportedModels(providerId); const discoveredModels = providerModelOptionsFromIds(providerId, ids); const configuredModels = existing?.configuredModels ?? []; - const configuredModelsById = new Map( - configuredModels.map((model) => [model.id, model]), - ); - const hasConfiguredFeaturedModel = configuredModels.some( - (model) => model.featured, - ); - const discoveredModelIds = new Set( - discoveredModels.map((model) => model.id), - ); - const models = [ - ...discoveredModels.map((model) => ({ - ...model, - ...(hasConfiguredFeaturedModel ? { featured: false } : {}), - ...configuredModelsById.get(model.id), - })), - ...configuredModels.filter( - (model) => !discoveredModelIds.has(model.id), - ), - ]; + const models = mergeDisplayModels(discoveredModels, configuredModels); const entry: CachedProviderModels = { providerId, models, fetchedAt: Date.now(), + provenModelIds: ids, + proofRevision: nextProofRevision(existing), + ...(existing?.runtimeManaged ? { runtimeManaged: true } : {}), ...(configuredModels.length > 0 ? { configuredModels } : {}), }; if (versionAtStart !== refreshVersion(providerId)) { @@ -305,6 +405,15 @@ export const useProviderModelCacheStore = create( providerId, models: existing?.models ?? [], fetchedAt: existing?.fetchedAt ?? 0, + ...(existing?.provenModelIds + ? { provenModelIds: existing.provenModelIds } + : isCachedModelInventoryAuthoritative(existing) + ? { provenModelIds: [] } + : {}), + ...(existing?.proofRevision !== undefined + ? { proofRevision: existing.proofRevision } + : {}), + ...(existing?.runtimeManaged ? { runtimeManaged: true } : {}), ...(existing?.configuredModels ? { configuredModels: existing.configuredModels } : {}), @@ -341,13 +450,17 @@ export const useProviderModelCacheStore = create( invalidateProvider: (providerId) => { bumpRefreshVersion(providerId); set((state) => { - if (state.runtimeManagedProviderIds.has(providerId)) { - const existing = state.providers.get(providerId); - if (!existing || existing.runtimeManaged) { - return {}; - } + const existing = state.providers.get(providerId); + if (state.runtimeManagedProviderIds.has(providerId) && existing) { const providers = new Map(state.providers); - providers.set(providerId, { ...existing, runtimeManaged: true }); + // Keep the configured display seed but remove proof: an invalidation + // means the old live response can no longer justify compatibility. + providers.set(providerId, { + ...existing, + models: existing.configuredModels ?? existing.models, + fetchedAt: 0, + provenModelIds: undefined, + }); persistModels(providers); return { providers }; } diff --git a/src/features/sessions/hooks/__tests__/useAutoArchiveSessions.test.ts b/src/features/sessions/hooks/__tests__/useAutoArchiveSessions.test.ts index 543c0cd3e..930349a22 100644 --- a/src/features/sessions/hooks/__tests__/useAutoArchiveSessions.test.ts +++ b/src/features/sessions/hooks/__tests__/useAutoArchiveSessions.test.ts @@ -14,6 +14,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpGetSessionInfo: (...args: unknown[]) => mocks.getSessionInfo(...args), })); diff --git a/src/features/sessions/hooks/__tests__/useSessionSearch.test.ts b/src/features/sessions/hooks/__tests__/useSessionSearch.test.ts index 10d574d72..06d1943fb 100644 --- a/src/features/sessions/hooks/__tests__/useSessionSearch.test.ts +++ b/src/features/sessions/hooks/__tests__/useSessionSearch.test.ts @@ -47,6 +47,7 @@ function sweep( } vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpSearchSessions: (...args: unknown[]) => mockAcpSearchSessions(...args), })); diff --git a/src/features/sessions/ui/__tests__/SessionHistoryView.test.tsx b/src/features/sessions/ui/__tests__/SessionHistoryView.test.tsx index b07a6cd5d..be8035709 100644 --- a/src/features/sessions/ui/__tests__/SessionHistoryView.test.tsx +++ b/src/features/sessions/ui/__tests__/SessionHistoryView.test.tsx @@ -39,6 +39,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/shared/api/acp", () => ({ + reserveAcpSessionConfiguration: () => ({ sequence: 0, clear: () => {} }), acpExportSession: (...args: unknown[]) => mocks.acpExportSession(...args), acpImportSession: (...args: unknown[]) => mocks.acpImportSession(...args), acpSearchSessions: (...args: unknown[]) => mocks.acpSearchSessions(...args), diff --git a/src/shared/api/__tests__/acp.test.ts b/src/shared/api/__tests__/acp.test.ts index 31b8d224e..66a7bb441 100644 --- a/src/shared/api/__tests__/acp.test.ts +++ b/src/shared/api/__tests__/acp.test.ts @@ -20,10 +20,14 @@ const mockAppendSessionSystemPrompt = vi.fn(); const mockForkSession = vi.fn(); const mockRenameSession = vi.fn(); const mockArchiveSession = vi.fn(); -const noRequestProviderContext = { requestId: undefined }; +const noRequestProviderContext = { + requestId: undefined, + canPublish: expect.any(Function), +}; const noRequestModelContext = (providerId: string) => ({ providerId, requestId: undefined, + canPublish: expect.any(Function), }); const managedRuntimeConfig: RuntimeConfig = { @@ -140,6 +144,16 @@ vi.mock("@/features/berdctl/appPreamble", () => ({ getBerdctlPreamble: () => mockGetBerdctlPreamble(), })); +const mockSupportedModelsList = vi.hoisted(() => vi.fn()); +vi.mock("../acpConnection", () => ({ + getClient: () => + Promise.resolve({ + goose: { + GooseUnstableProvidersSupportedModelsList: mockSupportedModelsList, + }, + }), +})); + vi.mock("../acpActiveMessageTracking", () => ({ setActiveMessageId: vi.fn(), clearActiveMessageId: vi.fn(), @@ -170,6 +184,46 @@ describe("acpSteerMessage", () => { expect(mockSteerSession).not.toHaveBeenCalled(); }); + + it("waits for pending configuration before steering the active run", async () => { + const registry = await import("../acpSessionRegistry"); + const { acpSteerMessage, reserveAcpSessionConfiguration } = await import( + "../acp" + ); + const sessionId = "acp-session-steer-transition"; + registry.registerPreparedSession( + sessionId, + "openai", + "/tmp/project", + "gpt-4.1", + ); + mockSteerSession.mockResolvedValueOnce({ + runId: "run-1", + messageId: "steer-message", + }); + const intent = reserveAcpSessionConfiguration(sessionId); + const steering = acpSteerMessage(sessionId, "run-1", "more"); + await Promise.resolve(); + expect(mockSteerSession).not.toHaveBeenCalled(); + await registry.configureSession( + sessionId, + "anthropic", + "/tmp/project", + "claude-fable", + {}, + intent, + ); + await expect(steering).resolves.toEqual({ + runId: "run-1", + messageId: "steer-message", + }); + expect(mockSteerSession).toHaveBeenCalledWith( + sessionId, + [{ type: "text", text: "more" }], + "run-1", + undefined, + ); + }); }); describe("acpSendMessage", () => { @@ -199,6 +253,134 @@ describe("acpSendMessage", () => { expect(mockPrompt).not.toHaveBeenCalled(); }); + it("blocks a prepared model disproved by cached authoritative inventory without network I/O", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const { useProviderModelCacheStore } = await import( + "@/features/providers/stores/providerModelCacheStore" + ); + const sessionRegistry = await import("../acpSessionRegistry"); + const { acpSendMessage } = await import("../acp"); + sessionRegistry.registerPreparedSession( + "acp-session-invalidated-model", + "databricks_v2", + "/tmp/project", + "removed-model", + ); + useProviderModelCacheStore.setState({ + providers: new Map([ + [ + "databricks_v2", + { + providerId: "databricks_v2", + models: [ + { + id: "supported-model", + name: "Supported model", + providerId: "databricks_v2", + }, + ], + provenModelIds: ["supported-model"], + fetchedAt: Date.now(), + }, + ], + ]), + }); + + await expect( + acpSendMessage("acp-session-invalidated-model", "hello"), + ).rejects.toThrow("removed-model is no longer supported"); + + expect(mockSupportedModelsList).not.toHaveBeenCalled(); + expect(mockPrompt).not.toHaveBeenCalled(); + }); + + it("keeps a disproved prepared model blocked after a failed forced refresh", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const { useProviderModelCacheStore } = await import( + "@/features/providers/stores/providerModelCacheStore" + ); + const sessionRegistry = await import("../acpSessionRegistry"); + const { acpSendMessage } = await import("../acp"); + sessionRegistry.registerPreparedSession( + "acp-session-model-disproved-before-refresh-failure", + "databricks_v2", + "/tmp/project", + "removed-model", + ); + mockSupportedModelsList.mockResolvedValueOnce({ + models: ["supported-model"], + }); + await useProviderModelCacheStore + .getState() + .refreshProviderModels("databricks_v2", { force: true }); + + await expect( + acpSendMessage( + "acp-session-model-disproved-before-refresh-failure", + "before failure", + ), + ).rejects.toThrow("removed-model is no longer supported"); + + mockSupportedModelsList.mockRejectedValueOnce(new Error("offline")); + await useProviderModelCacheStore + .getState() + .refreshProviderModels("databricks_v2", { force: true }); + const inventoryCallsBeforeSend = mockSupportedModelsList.mock.calls.length; + + await expect( + acpSendMessage( + "acp-session-model-disproved-before-refresh-failure", + "after failure", + ), + ).rejects.toThrow("removed-model is no longer supported"); + + expect(mockSupportedModelsList).toHaveBeenCalledTimes( + inventoryCallsBeforeSend, + ); + expect(mockPrompt).not.toHaveBeenCalled(); + expect( + useProviderModelCacheStore.getState().providers.get("databricks_v2") + ?.provenModelIds, + ).toEqual(["supported-model"]); + useProviderModelCacheStore.getState().invalidateProvider("databricks_v2"); + }); + + it("admits a managed-provider prompt without reading live model inventory", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const sessionRegistry = await import("../acpSessionRegistry"); + const { acpSendMessage } = await import("../acp"); + sessionRegistry.registerPreparedSession( + "acp-session-managed-send", + "databricks_v2", + "/tmp/project", + "goose-gpt-5-5", + ); + + await acpSendMessage("acp-session-managed-send", "hello"); + + expect(mockSupportedModelsList).not.toHaveBeenCalled(); + expect(mockPrompt).toHaveBeenCalledOnce(); + }); + + it("rejects an out-of-policy provider without reading live model inventory", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const sessionRegistry = await import("../acpSessionRegistry"); + const { acpSendMessage } = await import("../acp"); + sessionRegistry.registerPreparedSession( + "acp-session-outside-policy", + "outside-policy", + "/tmp/project", + "outside-model", + ); + + await expect( + acpSendMessage("acp-session-outside-policy", "hello"), + ).rejects.toThrow("outside the managed Goose provider policy"); + + expect(mockSupportedModelsList).not.toHaveBeenCalled(); + expect(mockPrompt).not.toHaveBeenCalled(); + }); + it("reports dispatch only after ACP setup reaches the transport boundary", async () => { const sessionRegistry = await import("../acpSessionRegistry"); const { acpSendMessage } = await import("../acp"); @@ -752,6 +934,302 @@ describe("acpLoadSession", () => { ); }); + it("does not dispatch a load snapshot when provider-changing prepare is awaiting inventory proof", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const supportedModels = deferred<{ models: string[] }>(); + const loadResponse = deferred>(); + mockSupportedModelsList.mockReturnValueOnce(supportedModels.promise); + mockLoadSession.mockReturnValueOnce(loadResponse.promise); + mockSetProvider.mockResolvedValueOnce({ + model: null, + reasoningEffort: null, + }); + mockSetModel.mockResolvedValueOnce({ model: null, reasoningEffort: null }); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { acpLoadSession, acpPrepareSession } = await import("../acp"); + + const configure = acpPrepareSession( + "acp-session-preflight-race", + "goose", + "/tmp/replay", + { modelId: "other-model" }, + ); + await vi.waitFor(() => expect(mockSupportedModelsList).toHaveBeenCalled()); + + const load = acpLoadSession("acp-session-preflight-race", "/tmp/replay"); + await vi.waitFor(() => expect(mockLoadSession).toHaveBeenCalledTimes(1)); + loadResponse.resolve( + executionConfigResponse("other-managed", "other-model"), + ); + await load; + + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + + supportedModels.resolve({ models: ["goose-gpt-5-5"] }); + await configure; + + expect(mockSetProvider).toHaveBeenCalledWith( + "acp-session-preflight-race", + "databricks_v2", + noRequestProviderContext, + ); + expect(mockSetModel).toHaveBeenCalledWith( + "acp-session-preflight-race", + "goose-gpt-5-5", + noRequestModelContext("databricks_v2"), + ); + }); + + it("publishes a deferred authoritative load when preflight rejects before mutation", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const loadResponse = deferred>(); + const supportedModels = deferred<{ models: string[] }>(); + mockLoadSession.mockReturnValueOnce(loadResponse.promise); + mockSupportedModelsList.mockReturnValueOnce(supportedModels.promise); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { acpLoadSession, acpPrepareSession } = await import("../acp"); + + const load = acpLoadSession( + "acp-session-load-before-rejected-preflight", + "/tmp/replay", + ); + await vi.waitFor(() => expect(mockLoadSession).toHaveBeenCalledTimes(1)); + const configure = acpPrepareSession( + "acp-session-load-before-rejected-preflight", + "goose", + "/tmp/replay", + { modelId: "other-model" }, + ); + await vi.waitFor(() => expect(mockSupportedModelsList).toHaveBeenCalled()); + + loadResponse.resolve( + executionConfigResponse("other-managed", "other-model"), + ); + await load; + supportedModels.reject(new Error("offline")); + await expect(configure).rejects.toThrow( + "Cannot verify models for migrated provider", + ); + await Promise.resolve(); + + expect(applyModelConfigSnapshot).toHaveBeenCalledWith( + "acp-session-load-before-rejected-preflight", + { modelId: "other-model", modelName: "other-model" }, + { origin: "response" }, + ); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + const { requireSessionInvocationSelection } = await import( + "../acpSessionRegistry" + ); + expect( + requireSessionInvocationSelection( + "acp-session-load-before-rejected-preflight", + ), + ).toEqual({ providerId: "other-managed", modelId: "other-model" }); + }); + + it.each([ + { + name: "setProvider", + load: executionConfigResponse("other-managed", "other-model"), + modelId: "other-model", + reject: () => mockSetProvider.mockRejectedValueOnce(new Error("offline")), + expected: () => expect(mockSetProvider).toHaveBeenCalledTimes(1), + }, + { + name: "setModel", + load: executionConfigResponse("databricks_v2", "old-model"), + modelId: "goose-gpt-5-5", + reject: () => mockSetModel.mockRejectedValueOnce(new Error("offline")), + expected: () => expect(mockSetModel).toHaveBeenCalledTimes(1), + }, + ])("does not publish a deferred load after attempted $name fails", async ({ + name, + load, + modelId, + reject, + expected, + }) => { + await setRuntimeConfig(managedRuntimeConfig); + const supportedModels = deferred<{ models: string[] }>(); + mockSupportedModelsList.mockReturnValueOnce(supportedModels.promise); + mockLoadSession.mockResolvedValueOnce(load); + reject(); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const sessionRegistry = await import("../acpSessionRegistry"); + const { acpLoadSession, acpPrepareSession } = await import("../acp"); + const sessionId = `acp-session-failed-${modelId}`; + + const configure = acpPrepareSession(sessionId, "goose", "/tmp/replay", { + modelId, + }); + await vi.waitFor(() => expect(mockSupportedModelsList).toHaveBeenCalled()); + await acpLoadSession(sessionId, "/tmp/replay"); + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + + supportedModels.resolve({ models: ["goose-gpt-5-5"] }); + await expect(configure).rejects.toThrow("offline"); + expected(); + if (name === "setProvider") { + expect(sessionRegistry.isSessionPrepared(sessionId)).toBe(false); + } + await Promise.resolve(); + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + }); + + it("does not let a stale preflight consume a newer preflight intent or publish a load", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const firstInventory = deferred<{ models: string[] }>(); + const secondInventory = deferred<{ models: string[] }>(); + mockSupportedModelsList + .mockReturnValueOnce(firstInventory.promise) + .mockReturnValueOnce(secondInventory.promise); + mockSetProvider.mockResolvedValue({ model: null, reasoningEffort: null }); + mockSetModel.mockResolvedValue({ model: null, reasoningEffort: null }); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { acpLoadSession, acpPrepareSession } = await import("../acp"); + + const first = acpPrepareSession( + "acp-session-two-preflights", + "goose", + "/tmp/replay", + { + modelId: "other-model", + }, + ); + await vi.waitFor(() => + expect(mockSupportedModelsList).toHaveBeenCalledTimes(1), + ); + const second = acpPrepareSession( + "acp-session-two-preflights", + "goose", + "/tmp/replay", + { + modelId: "other-model", + }, + ); + await vi.waitFor(() => + expect(mockSupportedModelsList).toHaveBeenCalledTimes(2), + ); + + firstInventory.resolve({ models: ["goose-gpt-5-5"] }); + await first; + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + + await acpLoadSession("acp-session-two-preflights", "/tmp/replay"); + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + + secondInventory.resolve({ models: ["goose-gpt-5-5"] }); + await second; + expect(mockSetProvider).toHaveBeenCalledTimes(1); + expect(mockSetModel).toHaveBeenCalledTimes(1); + }); + + it("releases only a rejected latest preflight intent", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const firstInventory = deferred<{ models: string[] }>(); + mockSupportedModelsList + .mockReturnValueOnce(firstInventory.promise) + .mockRejectedValueOnce(new Error("offline")); + mockLoadSession.mockResolvedValueOnce( + executionConfigResponse("other-managed", "other-model"), + ); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { acpLoadSession, acpPrepareSession } = await import("../acp"); + + const first = acpPrepareSession( + "acp-session-rejected-latest", + "goose", + "/tmp/replay", + { + modelId: "other-model", + }, + ); + await vi.waitFor(() => + expect(mockSupportedModelsList).toHaveBeenCalledTimes(1), + ); + await expect( + acpPrepareSession("acp-session-rejected-latest", "goose", "/tmp/replay", { + modelId: "other-model", + }), + ).rejects.toThrow("Cannot verify models for migrated provider"); + + firstInventory.resolve({ models: ["goose-gpt-5-5"] }); + await first; + await acpLoadSession("acp-session-rejected-latest", "/tmp/replay"); + + expect(applyModelConfigSnapshot).toHaveBeenCalledTimes(1); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + }); + + it("keeps the latest preflight when it resolves before an older one", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const firstInventory = deferred<{ models: string[] }>(); + const secondInventory = deferred<{ models: string[] }>(); + mockSupportedModelsList + .mockReturnValueOnce(firstInventory.promise) + .mockReturnValueOnce(secondInventory.promise); + mockSetProvider.mockResolvedValue({ model: null, reasoningEffort: null }); + mockSetModel.mockResolvedValue({ model: null, reasoningEffort: null }); + const { acpPrepareSession } = await import("../acp"); + + const first = acpPrepareSession( + "acp-session-reverse-preflights", + "goose", + "/tmp/replay", + { + modelId: "other-model", + }, + ); + await vi.waitFor(() => + expect(mockSupportedModelsList).toHaveBeenCalledTimes(1), + ); + const second = acpPrepareSession( + "acp-session-reverse-preflights", + "goose", + "/tmp/replay", + { + modelId: "other-model", + }, + ); + await vi.waitFor(() => + expect(mockSupportedModelsList).toHaveBeenCalledTimes(2), + ); + + secondInventory.resolve({ models: ["goose-gpt-5-5"] }); + await second; + firstInventory.resolve({ models: ["goose-gpt-5-5"] }); + await first; + + expect(mockSetProvider).toHaveBeenCalledTimes(1); + expect(mockSetModel).toHaveBeenCalledTimes(1); + }); + it("does not dispatch a load snapshot superseded by a UI configuration", async () => { const loadResponse = deferred>(); mockLoadSession.mockReturnValueOnce(loadResponse.promise); @@ -799,6 +1277,8 @@ describe("acpCreateSession", () => { mockSetProvider.mockResolvedValue({ model: null, reasoningEffort: null }); mockSetModel.mockReset(); mockSetModel.mockResolvedValue({ model: null, reasoningEffort: null }); + mockSupportedModelsList.mockReset(); + mockSupportedModelsList.mockResolvedValue({ models: ["goose-gpt-5-5"] }); await setRuntimeConfig(DEFAULT_RUNTIME_CONFIG); }); @@ -820,6 +1300,7 @@ describe("acpCreateSession", () => { model: null, reasoningEffort: null, }, + resolvedSelection: { providerId: "openai", modelId: "gpt-4.1" }, }); expect(mockNewSession).toHaveBeenCalledWith("/tmp/project", { @@ -872,6 +1353,7 @@ describe("acpCreateSession", () => { model: null, reasoningEffort: null, }, + resolvedSelection: { providerId: "goose" }, }); expect(mockNewSession).toHaveBeenCalledWith("/tmp/project", { @@ -1003,7 +1485,39 @@ describe("acpCreateSession", () => { }, reasoningEffort: reasoningEffortSnapshot, }, + resolvedSelection: { + providerId: "openai", + modelId: "gpt-4.1", + modelName: "GPT-4.1", + }, + }); + }); + + it("returns the concrete model acknowledged for provider-only creation", async () => { + mockNewSession.mockResolvedValue({ sessionId: "acp-session-default" }); + mockSetProvider.mockResolvedValueOnce({ + model: { modelId: "gpt-5.5", modelName: "GPT-5.5" }, + reasoningEffort: reasoningEffortSnapshot, + }); + + const sessionRegistry = await import("../acpSessionRegistry"); + const { acpCreateSession } = await import("../acp"); + + await expect(acpCreateSession("openai", "/tmp/project")).resolves.toEqual({ + sessionId: "acp-session-default", + configOptionsSnapshot: { + model: { modelId: "gpt-5.5", modelName: "GPT-5.5" }, + reasoningEffort: reasoningEffortSnapshot, + }, + resolvedSelection: { + providerId: "openai", + modelId: "gpt-5.5", + modelName: "GPT-5.5", + }, }); + expect( + sessionRegistry.requireSessionInvocationSelection("acp-session-default"), + ).toMatchObject({ providerId: "openai", modelId: "gpt-5.5" }); }); it("does not resurrect provider defaults absent from the final model snapshot", async () => { @@ -1029,7 +1543,78 @@ describe("acpCreateSession", () => { model: { modelId: "claude-fable", modelName: "Claude Fable" }, reasoningEffort: null, }, + resolvedSelection: { + providerId: "anthropic", + modelId: "claude-fable", + modelName: "Claude Fable", + }, + }); + }); + + it("applies the complete resolved migration pair for provider-only input", async () => { + await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockResolvedValueOnce({ + models: ["goose-gpt-5-5"], + }); + mockNewSession.mockResolvedValue({ sessionId: "migrated-session" }); + const { acpCreateSession } = await import("../acp"); + + await expect( + acpCreateSession("goose", "/tmp/project"), + ).resolves.toMatchObject({ + sessionId: "migrated-session", + resolvedSelection: { + providerId: "databricks_v2", + modelId: "goose-gpt-5-5", + }, + }); + + expect(mockSetProvider).toHaveBeenCalledWith( + "migrated-session", + "databricks_v2", + ); + expect(mockSetModel).toHaveBeenCalledWith( + "migrated-session", + "goose-gpt-5-5", + noRequestModelContext("databricks_v2"), + ); + }); + + it("does not mutate ACP when managed provider migration cannot prove support", async () => { + await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockRejectedValueOnce(new Error("offline")); + const { acpCreateSession } = await import("../acp"); + + await expect( + acpCreateSession("goose", "/tmp/project", { modelId: "other-model" }), + ).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + + expect(mockNewSession).not.toHaveBeenCalled(); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + }); + + it("uses a proven default instead of an unsupported migrated model", async () => { + await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockResolvedValueOnce({ + models: ["goose-gpt-5-5"], }); + mockNewSession.mockResolvedValue({ sessionId: "migrated-session" }); + const { acpCreateSession } = await import("../acp"); + + await acpCreateSession("goose", "/tmp/project", { modelId: "other-model" }); + + expect(mockSetProvider).toHaveBeenCalledWith( + "migrated-session", + "databricks_v2", + ); + expect(mockSetModel).toHaveBeenCalledWith( + "migrated-session", + "goose-gpt-5-5", + noRequestModelContext("databricks_v2"), + ); }); it("rejects an explicit provider outside managed policy before creating", async () => { @@ -1059,6 +1644,34 @@ describe("acpCreateSession", () => { expect(mockSetModel).not.toHaveBeenCalled(); }); + it("does not send a same-provider model disproved by live inventory", async () => { + await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockResolvedValueOnce({ + models: ["goose-gpt-5-5"], + }); + mockNewSession.mockResolvedValue({ sessionId: "managed-session" }); + const { acpCreateSession } = await import("../acp"); + + await acpCreateSession("databricks_v2", "/tmp/project", { + modelId: "retired-model", + }); + + expect(mockSetProvider).toHaveBeenCalledWith( + "managed-session", + "databricks_v2", + ); + expect(mockSetModel).toHaveBeenCalledWith( + "managed-session", + "goose-gpt-5-5", + noRequestModelContext("databricks_v2"), + ); + expect(mockSetModel).not.toHaveBeenCalledWith( + "managed-session", + "retired-model", + expect.anything(), + ); + }); + it.each([ "claude-acp", "codex-acp", @@ -1177,6 +1790,8 @@ describe("acpPrepareSession", () => { beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); + mockSupportedModelsList.mockReset(); + mockSupportedModelsList.mockResolvedValue({ models: ["goose-gpt-5-5"] }); await setRuntimeConfig(DEFAULT_RUNTIME_CONFIG); }); @@ -1203,6 +1818,162 @@ describe("acpPrepareSession", () => { expect(sessionRegistry.isSessionPrepared("acp-session-1")).toBe(true); }); + it("keeps a caller-owned configuration intent after prepare rejects", async () => { + await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockRejectedValueOnce(new Error("offline")); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { + acpLoadSession, + acpPrepareSession, + reserveAcpSessionConfiguration, + } = await import("../acp"); + const sessionId = "acp-session-caller-owned-intent"; + const intent = reserveAcpSessionConfiguration(sessionId); + + await expect( + acpPrepareSession( + sessionId, + "goose", + "/tmp/project", + { + modelId: "other-model", + }, + intent, + ), + ).rejects.toThrow("Cannot verify models for migrated provider"); + mockLoadSession.mockResolvedValueOnce( + executionConfigResponse("openai", "gpt-5.5"), + ); + await acpLoadSession(sessionId, "/tmp/project"); + expect(applyModelConfigSnapshot).not.toHaveBeenCalled(); + + intent.clear(); + mockLoadSession.mockResolvedValueOnce( + executionConfigResponse("openai", "gpt-5.5"), + ); + await acpLoadSession(sessionId, "/tmp/project"); + expect(applyModelConfigSnapshot).toHaveBeenCalledTimes(1); + }); + + it("releases timed-out inventory intent so fresh load and retry can reconcile", async () => { + await setRuntimeConfig(managedRuntimeConfig); + vi.useFakeTimers(); + const timedOutInventory = deferred<{ models: string[] }>(); + mockSupportedModelsList + .mockReturnValueOnce(timedOutInventory.promise) + .mockResolvedValueOnce({ models: ["goose-gpt-5-5"] }); + mockLoadSession.mockResolvedValue( + executionConfigResponse("openai", "gpt-5.5"), + ); + mockSetProvider.mockResolvedValue({ model: null, reasoningEffort: null }); + mockSetModel.mockResolvedValue({ model: null, reasoningEffort: null }); + const applyModelConfigSnapshot = vi.fn(); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + setSessionConfigSnapshotHandlers({ applyModelConfigSnapshot }); + const { acpLoadSession, acpPrepareSession } = await import("../acp"); + const sessionId = "acp-session-timeout-retry"; + + const timedOutPrepare = acpPrepareSession( + sessionId, + "goose", + "/tmp/project", + { + modelId: "other-model", + }, + ); + const rejectedPrepare = expect(timedOutPrepare).rejects.toThrow( + "Cannot verify models for migrated provider", + ); + await vi.advanceTimersByTimeAsync(60_000); + await rejectedPrepare; + + expect(mockLoadSession).not.toHaveBeenCalled(); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + timedOutInventory.resolve({ models: ["goose-gpt-5-5"] }); + await Promise.resolve(); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + + await acpLoadSession(sessionId, "/tmp/project"); + expect(applyModelConfigSnapshot).toHaveBeenCalledTimes(1); + await acpPrepareSession(sessionId, "goose", "/tmp/project", { + modelId: "other-model", + }); + expect(mockSetProvider).toHaveBeenCalledWith( + sessionId, + "databricks_v2", + noRequestProviderContext, + ); + expect(mockSetModel).toHaveBeenCalledWith( + sessionId, + "goose-gpt-5-5", + noRequestModelContext("databricks_v2"), + ); + vi.useRealTimers(); + }); + + it("rejects invalidated inventory proof and requires a fresh proof before mutation", async () => { + await setRuntimeConfig(managedRuntimeConfig); + const staleInventory = deferred<{ models: string[] }>(); + mockSupportedModelsList + .mockReturnValueOnce(staleInventory.promise) + .mockResolvedValueOnce({ models: ["goose-gpt-5-5"] }); + mockSetProvider.mockResolvedValue({ model: null, reasoningEffort: null }); + mockSetModel.mockResolvedValue({ model: null, reasoningEffort: null }); + const { acpPrepareSession } = await import("../acp"); + const { notifyProviderModelInventoryInvalidated } = await import( + "@/shared/runtime-config/providerModelInventoryInvalidation" + ); + const sessionId = "acp-session-invalidated-proof"; + + const stalePrepare = acpPrepareSession(sessionId, "goose", "/tmp/project", { + modelId: "other-model", + }); + await vi.waitFor(() => + expect(mockSupportedModelsList).toHaveBeenCalledTimes(1), + ); + notifyProviderModelInventoryInvalidated("databricks_v2"); + staleInventory.resolve({ models: ["goose-gpt-5-5"] }); + await expect(stalePrepare).rejects.toThrow( + "Cannot verify models for migrated provider", + ); + + expect(mockLoadSession).not.toHaveBeenCalled(); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + await acpPrepareSession(sessionId, "goose", "/tmp/project", { + modelId: "other-model", + }); + expect(mockSupportedModelsList).toHaveBeenCalledTimes(2); + expect(mockSetProvider).toHaveBeenCalledTimes(1); + expect(mockSetModel).toHaveBeenCalledTimes(1); + }); + + it("does not load or mutate a session when managed migration cannot prove support", async () => { + await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockRejectedValueOnce(new Error("offline")); + const { acpPrepareSession } = await import("../acp"); + + await expect( + acpPrepareSession("legacy-session", "goose", "/tmp/project", { + modelId: "other-model", + }), + ).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + + expect(mockLoadSession).not.toHaveBeenCalled(); + expect(mockSetProvider).not.toHaveBeenCalled(); + expect(mockSetModel).not.toHaveBeenCalled(); + }); + it("rejects a provider outside managed policy before loading the session", async () => { await setRuntimeConfig(managedRuntimeConfig); const { acpPrepareSession } = await import("../acp"); @@ -1231,6 +2002,9 @@ describe("acpPrepareSession", () => { it("allows upstream models omitted from recommendation metadata", async () => { await setRuntimeConfig(managedRuntimeConfig); + mockSupportedModelsList.mockResolvedValueOnce({ + models: ["new-upstream-model"], + }); const { acpPrepareSession } = await import("../acp"); await acpPrepareSession("other-session", "other-managed", "/tmp/project", { diff --git a/src/shared/api/__tests__/acpApi.test.ts b/src/shared/api/__tests__/acpApi.test.ts index 3f5e739fe..bec71cdcc 100644 --- a/src/shared/api/__tests__/acpApi.test.ts +++ b/src/shared/api/__tests__/acpApi.test.ts @@ -1,5 +1,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { setSessionConfigSnapshotHandlers } from "../acpSessionConfigSnapshots"; +import { registerChatSessionConfigSnapshotHandlers } from "@/features/chat/acp/sessionConfigSnapshotAdapter"; +import { resetSessionTargetCoordinatorsForTests } from "@/features/chat/lib/sessionTargetCoordinator"; +import { useChatSessionStore } from "@/features/chat/stores/chatSessionStore"; const mocks = vi.hoisted(() => ({ getClient: vi.fn(), @@ -19,7 +22,7 @@ const includeLastMessageSnippetMeta = { }, }; -function createConfigOptionsResponse() { +function createConfigOptionsResponse(modelId = "claude-opus-4-8") { return { configOptions: [ { @@ -27,10 +30,16 @@ function createConfigOptionsResponse() { category: "model", kind: { type: "select", - currentValue: "claude-opus-4-8", + currentValue: modelId, options: { type: "ungrouped", - values: [{ value: "claude-opus-4-8", name: "Claude Opus 4.8" }], + values: [ + { + value: modelId, + name: + modelId === "claude-opus-4-8" ? "Claude Opus 4.8" : modelId, + }, + ], }, }, }, @@ -54,10 +63,21 @@ function createConfigOptionsResponse() { }; } +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} + vi.mock("../acpConnection", () => ({ getClient: (...args: unknown[]) => mocks.getClient(...args), interceptSessionNotifications: (...args: unknown[]) => mocks.interceptSessionNotifications(...args), + invalidateClientConnection: vi.fn().mockResolvedValue(undefined), })); describe("promptForText", () => { @@ -815,6 +835,163 @@ describe("provider wire translation", () => { ); }); + it.each([ + "provider", + "model", + ] as const)("keeps newer preparation authoritative when a timed-out stale %s response settles", async (kind) => { + vi.useFakeTimers(); + try { + const sessionId = `stale-${kind}-session`; + const appliedModels: string[] = []; + setSessionConfigSnapshotHandlers({ + applyModelConfigSnapshot: (_sessionId, snapshot) => { + appliedModels.push(snapshot.modelId); + }, + }); + const staleResponse = + deferred>(); + mocks.getClient.mockResolvedValue({ + loadSession: vi.fn().mockResolvedValue({ configOptions: [] }), + setSessionConfigOption: mocks.setSessionConfigOption, + }); + mocks.setSessionConfigOption + .mockReset() + .mockReturnValueOnce(staleResponse.promise) + .mockResolvedValueOnce(createConfigOptionsResponse("new-model")); + const registry = await import("../acpSessionRegistry"); + registry.registerPreparedSession( + sessionId, + "old-provider", + "/project", + "old-model", + ); + + const stale = + kind === "provider" + ? registry.prepareSession(sessionId, "stale-provider", "/project") + : registry.applySessionModel(sessionId, "stale-model"); + await vi.waitFor(() => + expect(mocks.setSessionConfigOption).toHaveBeenCalledTimes(1), + ); + const staleRejection = expect(stale).rejects.toThrow( + "ACP operation timed out", + ); + await vi.advanceTimersByTimeAsync(60_000); + await staleRejection; + + await registry.prepareSession(sessionId, "new-provider", "/project"); + expect(registry.requireSessionInvocationSelection(sessionId)).toEqual({ + providerId: "new-provider", + modelId: "new-model", + }); + expect(appliedModels).toEqual(["new-model"]); + + staleResponse.resolve(createConfigOptionsResponse("stale-model")); + await Promise.resolve(); + await Promise.resolve(); + + expect(registry.requireSessionInvocationSelection(sessionId)).toEqual({ + providerId: "new-provider", + modelId: "new-model", + }); + expect(appliedModels).toEqual(["new-model"]); + } finally { + vi.useRealTimers(); + } + }); + + it.each([ + "provider", + "model", + ] as const)("keeps the real chat store and registry newer after a stale %s response settles", async (kind) => { + vi.useFakeTimers(); + try { + const sessionId = `adapter-store-${kind}-session`; + const staleResponse = + deferred>(); + useChatSessionStore.setState({ sessions: [] }); + resetSessionTargetCoordinatorsForTests(); + registerChatSessionConfigSnapshotHandlers(); + useChatSessionStore.getState().addSession({ + id: sessionId, + title: "Chat", + executionTarget: { + harnessId: "goose", + modelProviderId: "old-provider", + modelId: "old-model", + modelName: "old-model", + }, + executionTargetSource: "acp", + createdAt: "2026-04-20T00:00:00.000Z", + updatedAt: "2026-04-20T00:00:00.000Z", + messageCount: 0, + }); + mocks.getClient.mockResolvedValue({ + loadSession: vi.fn().mockResolvedValue({ configOptions: [] }), + setSessionConfigOption: mocks.setSessionConfigOption, + }); + mocks.setSessionConfigOption + .mockReset() + .mockReturnValueOnce(staleResponse.promise) + .mockResolvedValueOnce(createConfigOptionsResponse("new-model")); + + const registry = await import("../acpSessionRegistry"); + registry.registerPreparedSession( + sessionId, + "old-provider", + "/project", + "old-model", + ); + + const stale = + kind === "provider" + ? registry.prepareSession(sessionId, "stale-provider", "/project") + : registry.applySessionModel(sessionId, "stale-model"); + await vi.waitFor(() => + expect(mocks.setSessionConfigOption).toHaveBeenCalledTimes(1), + ); + const staleRejection = expect(stale).rejects.toThrow( + "ACP operation timed out", + ); + const newer = registry.prepareSession( + sessionId, + "new-provider", + "/project", + ); + await vi.advanceTimersByTimeAsync(60_000); + await staleRejection; + await newer; + + expect( + useChatSessionStore.getState().getSession(sessionId)?.executionTarget, + ).toMatchObject({ + modelProviderId: "new-provider", + modelId: "new-model", + }); + expect(registry.requireSessionInvocationSelection(sessionId)).toEqual({ + providerId: "new-provider", + modelId: "new-model", + }); + + staleResponse.resolve(createConfigOptionsResponse("stale-model")); + await Promise.resolve(); + await Promise.resolve(); + + expect( + useChatSessionStore.getState().getSession(sessionId)?.executionTarget, + ).toMatchObject({ + modelProviderId: "new-provider", + modelId: "new-model", + }); + expect(registry.requireSessionInvocationSelection(sessionId)).toEqual({ + providerId: "new-provider", + modelId: "new-model", + }); + } finally { + vi.useRealTimers(); + } + }); + it("warns instead of silently dropping snapshots when no handlers are registered", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); mocks.setSessionConfigOption.mockResolvedValueOnce( diff --git a/src/shared/api/__tests__/acpSessionRegistry.test.ts b/src/shared/api/__tests__/acpSessionRegistry.test.ts index 38c34bb20..5188f923e 100644 --- a/src/shared/api/__tests__/acpSessionRegistry.test.ts +++ b/src/shared/api/__tests__/acpSessionRegistry.test.ts @@ -10,10 +10,14 @@ const mockSetSessionConfigOption = vi.fn(); const mockUpdateWorkingDir = vi.fn(); const mockLoadSession = vi.fn(); const mockInvalidateClientConnection = vi.fn(); -const noRequestProviderContext = { requestId: undefined }; +const noRequestProviderContext = { + requestId: undefined, + canPublish: expect.any(Function), +}; const noRequestModelContext = (providerId: string) => ({ providerId, requestId: undefined, + canPublish: expect.any(Function), }); vi.mock("../acpConnection", () => ({ @@ -305,6 +309,70 @@ describe("applySessionModel", () => { expect(prompt).toHaveBeenCalledWith("anthropic"); }); + it("lets a successful load supersede existing negative proof but not newer proof", async () => { + const { publishProvenModelInventory } = await import( + "@/features/providers/stores/providerModelCacheStore" + ); + publishProvenModelInventory("openai", ["gpt-other"]); + const registry = await importRegistry(); + mockLoadSession.mockResolvedValueOnce( + executionConfigResponse("openai", "gpt-5.5"), + ); + const prompt = vi.fn().mockResolvedValue("complete"); + + await registry.loadSession("session-1", "/project"); + + await expect( + registry.runPreparedSessionPrompt("session-1", prompt), + ).resolves.toBe("complete"); + expect(prompt).toHaveBeenCalledWith("openai"); + + publishProvenModelInventory("openai", ["gpt-newer"]); + + await expect( + registry.runPreparedSessionPrompt("session-1", prompt), + ).rejects.toThrow( + "Session model gpt-5.5 is no longer supported by provider openai", + ); + expect(prompt).toHaveBeenCalledTimes(1); + }); + + it("holds prompt transport behind pending configuration intent until it clears", async () => { + const registry = await importPreparedRegistry("openai", "gpt-4.1"); + const supersession = registry.supersedeSessionMutation("session-1"); + const prompt = vi.fn().mockResolvedValue("complete"); + + const result = registry.runPreparedSessionPrompt("session-1", prompt); + await Promise.resolve(); + expect(prompt).not.toHaveBeenCalled(); + + supersession.clear(); + await expect(result).resolves.toBe("complete"); + expect(prompt).toHaveBeenCalledWith("openai"); + }); + + it("holds prompt transport until pending configuration is consumed", async () => { + const registry = await importPreparedRegistry("openai", "gpt-4.1"); + const supersession = registry.supersedeSessionMutation("session-1"); + const prompt = vi.fn().mockResolvedValue("complete"); + + const result = registry.runPreparedSessionPrompt("session-1", prompt); + const configure = registry.configureSession( + "session-1", + "anthropic", + "/project", + "claude-fable", + {}, + supersession, + ); + await expect(configure).resolves.toEqual({ + model: { modelId: "claude-fable", modelName: "claude-fable" }, + reasoningEffort: null, + }); + await expect(result).resolves.toBe("complete"); + expect(prompt).toHaveBeenCalledWith("anthropic"); + }); + it("does not time out a long-running prompt or admit config work mid-turn", async () => { vi.useFakeTimers(); try { @@ -363,6 +431,63 @@ describe("applySessionModel", () => { } }); + it("does not publish a timed-out prepare after newer preparation succeeds", async () => { + vi.useFakeTimers(); + try { + const registry = await importRegistry(); + const staleLoadResponse = deferred(); + mockLoadSession.mockReturnValueOnce(staleLoadResponse.promise); + mockSetProvider.mockResolvedValueOnce( + modelConfigResponse("new-model", "New Model"), + ); + + const stalePrepare = registry.prepareSession( + "session-1", + "stale-provider", + "/stale-project", + ); + await vi.waitFor(() => expect(mockLoadSession).toHaveBeenCalledTimes(1)); + const staleRejection = expect(stalePrepare).rejects.toThrow( + "ACP operation timed out", + ); + + const newerPrepare = registry.prepareSession( + "session-1", + "new-provider", + "/new-project", + ); + await vi.advanceTimersByTimeAsync(60_000); + + await staleRejection; + await expect(newerPrepare).resolves.toEqual( + modelConfigResponse("new-model", "New Model"), + ); + expect(registry.requireSessionInvocationSelection("session-1")).toEqual({ + providerId: "new-provider", + modelId: "new-model", + }); + + staleLoadResponse.resolve( + modelConfigResponse("stale-model", "Stale Model"), + ); + await Promise.resolve(); + + expect(mockSetProvider).toHaveBeenCalledTimes(1); + expect(mockSetProvider).toHaveBeenCalledWith( + "session-1", + "new-provider", + noRequestProviderContext, + ); + expect(registry.getPreparedProviderId("session-1")).toBe("new-provider"); + expect(registry.requireSessionInvocationSelection("session-1")).toEqual({ + providerId: "new-provider", + modelId: "new-model", + }); + } finally { + vi.useRealTimers(); + } + }); + it("records a superseded load pair for the queued model mutation", async () => { const registry = await importPreparedRegistry("openai", "gpt-4.1"); const loadResponse = deferred>(); @@ -382,6 +507,43 @@ describe("applySessionModel", () => { ); }); + it("keeps a mutation behind an owned preflight configuration after its old tail settles", async () => { + const registry = await importPreparedRegistry("openai", "gpt-4.1"); + const firstProviderResponse = deferred(); + mockSetProvider.mockReturnValueOnce(firstProviderResponse.promise); + + const supersession = registry.supersedeSessionMutation("session-1"); + const first = registry.prepareSession( + "session-1", + "anthropic", + "/project", + {}, + supersession, + ); + + await vi.waitFor(() => expect(mockSetProvider).toHaveBeenCalledTimes(1)); + + // The owned intent was consumed before the real mutation was appended. A + // cleanup registered against the formerly resolved tail must not reclaim + // the queue while this provider call is still in flight. + const second = registry.prepareSession("session-1", "gemini", "/project"); + await Promise.resolve(); + expect(mockSetProvider).toHaveBeenCalledTimes(1); + + firstProviderResponse.resolve( + modelConfigResponse("claude-default", "Claude Default"), + ); + await first; + await second; + + expect(mockSetProvider).toHaveBeenCalledTimes(2); + expect(mockSetProvider).toHaveBeenLastCalledWith( + "session-1", + "gemini", + noRequestProviderContext, + ); + }); + it("does not run a load between one provider and model configuration", async () => { const registry = await importPreparedRegistry("openai", "gpt-4.1"); const providerResponse = deferred(); @@ -420,6 +582,26 @@ describe("applySessionModel", () => { expect(mockSetModel).toHaveBeenCalledTimes(1); }); + it("returns the acknowledged requested model when setModel omits a snapshot", async () => { + const registry = await importPreparedRegistry("openai", "gpt-4.1"); + mockSetProvider.mockResolvedValueOnce( + modelConfigResponse("claude-sonnet", "Claude Sonnet"), + ); + mockSetModel.mockResolvedValueOnce(undefined); + + await expect( + registry.configureSession( + "session-1", + "anthropic", + "/project", + "claude-fable", + ), + ).resolves.toEqual({ + model: { modelId: "claude-fable", modelName: "claude-fable" }, + reasoningEffort: null, + }); + }); + it("returns the final model snapshot without provider-default fields", async () => { const registry = await importPreparedRegistry("openai", "gpt-5.5"); mockSetProvider.mockResolvedValueOnce( diff --git a/src/shared/api/acp.ts b/src/shared/api/acp.ts index 4817b03fb..5d39da7b1 100644 --- a/src/shared/api/acp.ts +++ b/src/shared/api/acp.ts @@ -28,7 +28,10 @@ import { type PersonaHandoffClaim, } from "./acpPersonaHandoff"; import { useRuntimeConfigStore } from "@/shared/runtime-config/runtimeConfigStore"; -import { resolveManagedGooseProviderSelection } from "@/shared/runtime-config/modelProviderPolicy"; +import { + resolveManagedGooseProviderSelection, + resolveValidatedManagedGooseProviderSelection, +} from "@/shared/runtime-config/modelProviderPolicy"; import { getStyleGuidelinesPrompt } from "@/shared/preferences/styleGuidelinesPreference"; import { getBerdctlPreamble } from "@/features/berdctl/appPreamble"; import { INTERACTION_NORMS_PREAMBLE } from "@/shared/api/interactionNorms"; @@ -78,11 +81,31 @@ export interface AcpSessionConfigApplyOptions { modelId?: string | null; /** UI selection intent that owns any response snapshots. */ requestId?: string; + /** The coordinator already resolved this complete selection from inventory. */ + selectionAlreadyResolved?: boolean; +} + +export type AcpSessionConfigurationIntent = + sessionRegistry.SessionMutationSupersession; + +/** + * Reserve a session's configuration ordering before asynchronous resolution. + * The owner must pass it to acpPrepareSession and clear it when finished. + */ +export function reserveAcpSessionConfiguration( + sessionId: string, +): AcpSessionConfigurationIntent { + return sessionRegistry.supersedeSessionMutation(sessionId); } export interface AcpCreateSessionResult { sessionId: string; configOptionsSnapshot: AcpSessionConfigSnapshots; + resolvedSelection: { + providerId: string; + modelId?: string; + modelName?: string; + }; } export type AcpDuplicateSessionOptions = AcpForkSessionOptions; @@ -169,11 +192,25 @@ async function acpSendMessageNow( const sid = sessionId.slice(0, 8); const tStart = performance.now(); - const resolvedProvider = resolveGooseSessionSelection(providerId).providerId; - if (resolvedProvider !== providerId) { - throw new Error( - `Session provider ${providerId} is outside the managed Goose provider policy. Re-prepare the session before prompting.`, - ); + if ( + providerId === "goose" || + CURATED_PROVIDER_CATALOG_BY_ID.get(providerId)?.category !== "agent" + ) { + const runtimeConfigState = useRuntimeConfigStore.getState(); + if (runtimeConfigState.result.status === "unavailable") { + throw new Error( + `Goose provider policy is unavailable: ${runtimeConfigState.result.message}`, + ); + } + const resolvedProvider = resolveManagedGooseProviderSelection( + runtimeConfigState.config, + { providerId }, + )?.providerId; + if (resolvedProvider && resolvedProvider !== providerId) { + throw new Error( + `Session provider ${providerId} is outside the managed Goose provider policy. Re-prepare the session before prompting.`, + ); + } } // Goose owns prompt assembly and accepts a real system prompt via its ACP @@ -298,7 +335,6 @@ export async function acpSteerMessage( "assistantPrompt" | "goose" | "images" > = {}, ): Promise { - sessionRegistry.requireSessionInvocationSelection(sessionId); const { assistantPrompt, goose, images } = options; const content: ContentBlock[] = []; const assistantText = assistantPrompt?.trim(); @@ -316,18 +352,20 @@ export async function acpSteerMessage( } } - return directAcp.steerSession( - sessionId, - content, - expectedRunId, - goose && Object.keys(goose).length > 0 ? { goose } : undefined, + return sessionRegistry.runPreparedSessionInvocation(sessionId, () => + directAcp.steerSession( + sessionId, + content, + expectedRunId, + goose && Object.keys(goose).length > 0 ? { goose } : undefined, + ), ); } -function resolveGooseSessionSelection( +async function resolveGooseSessionSelection( providerId: string, modelId?: string | null, -): { providerId: string; modelId?: string } { +): Promise<{ providerId: string; modelId?: string }> { if (modelId === "goose") { throw new Error(`Invalid model id: ${modelId}`); } @@ -356,7 +394,7 @@ function resolveGooseSessionSelection( providerId, ...(concreteModelId ? { modelId: concreteModelId } : {}), }; - const managedSelection = resolveManagedGooseProviderSelection( + const managedSelection = await resolveValidatedManagedGooseProviderSelection( runtimeConfigState.config, requestedSelection, ); @@ -368,9 +406,36 @@ function resolveGooseSessionSelection( ); } - // A concrete provider is renderer-owned. Policy may validate it, but must - // not replace its provider or inject a different provider's default model. - return requestedSelection; + // A concrete provider stays renderer-owned; validation may remove or replace + // only its model with a proven result. + return managedSelection; +} + +/** Apply a caller-resolved session selection without performing another inventory read. */ +async function applyResolvedSessionSelection( + sessionId: string, + selection: { providerId: string; modelId?: string }, + workingDir: string, + options: AcpSessionConfigApplyOptions, + supersession: AcpSessionConfigurationIntent, +): Promise { + const applyResolvedModel = Boolean(selection.modelId); + return applyResolvedModel && selection.modelId + ? sessionRegistry.configureSession( + sessionId, + selection.providerId, + workingDir, + selection.modelId, + options, + supersession, + ) + : sessionRegistry.prepareSession( + sessionId, + selection.providerId, + workingDir, + options, + supersession, + ); } /** Prepare or warm an ACP session ahead of the first prompt. */ @@ -379,34 +444,37 @@ export async function acpPrepareSession( providerId: string, workingDir: string, options: AcpSessionConfigApplyOptions = {}, + intent?: AcpSessionConfigurationIntent, ): Promise { const sid = sessionId.slice(0, 8); const t0 = performance.now(); perfLog( `[perf:prepare] ${sid} acpPrepareSession start (provider=${providerId})`, ); - const selection = resolveGooseSessionSelection(providerId, options.modelId); - const applyResolvedModel = - Boolean(options.modelId) || selection.providerId !== providerId; - const snapshots = - applyResolvedModel && selection.modelId - ? await sessionRegistry.configureSession( - sessionId, - selection.providerId, - workingDir, - selection.modelId, - options, - ) - : await sessionRegistry.prepareSession( - sessionId, - selection.providerId, - workingDir, - options, - ); - perfLog( - `[perf:prepare] ${sid} acpPrepareSession done in ${(performance.now() - t0).toFixed(1)}ms`, - ); - return snapshots; + const supersession = intent ?? reserveAcpSessionConfiguration(sessionId); + const ownsSupersession = intent === undefined; + try { + const resolvedModelId = normalizeConcreteModelId(options.modelId); + const selection = options.selectionAlreadyResolved + ? { + providerId, + ...(resolvedModelId ? { modelId: resolvedModelId } : {}), + } + : await resolveGooseSessionSelection(providerId, options.modelId); + const snapshots = await applyResolvedSessionSelection( + sessionId, + selection, + workingDir, + options, + supersession, + ); + perfLog( + `[perf:prepare] ${sid} acpPrepareSession done in ${(performance.now() - t0).toFixed(1)}ms`, + ); + return snapshots; + } finally { + if (ownsSupersession) supersession.clear(); + } } export async function acpCreateSession( @@ -414,7 +482,10 @@ export async function acpCreateSession( workingDir: string, options: AcpCreateSessionOptions = {}, ): Promise { - const selection = resolveGooseSessionSelection(providerId, options.modelId); + const selection = await resolveGooseSessionSelection( + providerId, + options.modelId, + ); providerId = selection.providerId; options = { ...options, modelId: selection.modelId }; // Only the "goose" sentinel should rely on backend defaults. Concrete @@ -474,7 +545,18 @@ export async function acpCreateSession( (await sessionRegistry.applySessionModel(sessionId, options.modelId)) ?? configOptionsSnapshot; } - return { sessionId, configOptionsSnapshot }; + const resolvedModel = configOptionsSnapshot.model; + return { + sessionId, + configOptionsSnapshot, + resolvedSelection: resolvedModel + ? { + providerId: selection.providerId, + modelId: resolvedModel.modelId, + modelName: resolvedModel.modelName, + } + : selection, + }; } catch (error) { rollbackSessionRegistration?.(); try { @@ -563,29 +645,37 @@ export async function acpLoadSession( sessionId: shortLogId(sessionId), }); perfLog(`[perf:load] ${sid} acpLoadSession → client.loadSession`); - const { response, isCurrent, executionSelection } = + const { response, isCurrent, deferredCurrent, executionSelection } = await sessionRegistry.loadSession(sessionId, effectiveWorkingDir); + const publish = () => { + const snapshots = readSessionConfigOptionsSnapshots(response); + logReasoningEffortInfo("acpLoadSession response", { + sessionId: shortLogId(sessionId), + hasReasoningEffortSnapshot: Boolean(snapshots.reasoningEffort), + ...reasoningEffortConfigLogFields( + "reasoningEffort", + snapshots.reasoningEffort, + ), + }); + applySessionConfigOptionsSnapshot(sessionId, response, { + origin: "response", + }); + perfLog( + `[perf:load] ${sid} client.loadSession resolved in ${(performance.now() - t0).toFixed(1)}ms`, + ); + }; if (!isCurrent) { + if (deferredCurrent) { + void deferredCurrent.then((becameCurrent) => { + if (becameCurrent) publish(); + }); + } perfLog( - `[perf:load] ${sid} dropped superseded load snapshot in ${(performance.now() - t0).toFixed(1)}ms`, + `[perf:load] ${sid} deferred or dropped superseded load snapshot in ${(performance.now() - t0).toFixed(1)}ms`, ); return undefined; } - const snapshots = readSessionConfigOptionsSnapshots(response); - logReasoningEffortInfo("acpLoadSession response", { - sessionId: shortLogId(sessionId), - hasReasoningEffortSnapshot: Boolean(snapshots.reasoningEffort), - ...reasoningEffortConfigLogFields( - "reasoningEffort", - snapshots.reasoningEffort, - ), - }); - applySessionConfigOptionsSnapshot(sessionId, response, { - origin: "response", - }); - perfLog( - `[perf:load] ${sid} client.loadSession resolved in ${(performance.now() - t0).toFixed(1)}ms`, - ); + publish(); return executionSelection; } diff --git a/src/shared/api/acpApi.ts b/src/shared/api/acpApi.ts index 497de6a33..72085cfed 100644 --- a/src/shared/api/acpApi.ts +++ b/src/shared/api/acpApi.ts @@ -215,7 +215,11 @@ export async function forkSession( export async function setModel( sessionId: string, modelId: string, - context: { providerId?: string; requestId?: string } = {}, + context: { + providerId?: string; + requestId?: string; + canPublish?: () => boolean; + } = {}, ): Promise { const sid = sessionId.slice(0, 8); const tClient = performance.now(); @@ -236,11 +240,14 @@ export async function setModel( snapshots.reasoningEffort, ), }); - applySessionConfigOptionsSnapshot(sessionId, response, { - origin: "response", - ...context, - modelId: snapshots.model?.modelId ?? modelId, - }); + const { canPublish, ...snapshotContext } = context; + if (canPublish?.() !== false) { + applySessionConfigOptionsSnapshot(sessionId, response, { + origin: "response", + ...snapshotContext, + modelId: snapshots.model?.modelId ?? modelId, + }); + } perfLog( `[perf:api] ${sid} setModel(${modelId}) getClient=${(tCall - tClient).toFixed(1)}ms wire=${(performance.now() - tCall).toFixed(1)}ms`, ); @@ -286,7 +293,7 @@ export async function setSessionConfigOption( export async function setProvider( sessionId: string, providerId: string, - context: { requestId?: string } = {}, + context: { requestId?: string; canPublish?: () => boolean } = {}, ): Promise { const sid = sessionId.slice(0, 8); const tClient = performance.now(); @@ -309,12 +316,15 @@ export async function setProvider( snapshots.reasoningEffort, ), }); - applySessionConfigOptionsSnapshot(sessionId, response, { - origin: "response", - ...context, - providerId, - modelId: snapshots.model?.modelId, - }); + const { canPublish, ...snapshotContext } = context; + if (canPublish?.() !== false) { + applySessionConfigOptionsSnapshot(sessionId, response, { + origin: "response", + ...snapshotContext, + providerId, + modelId: snapshots.model?.modelId, + }); + } perfLog( `[perf:api] ${sid} setProvider(${providerId}→${wireProvider}) getClient=${(tCall - tClient).toFixed(1)}ms wire=${(performance.now() - tCall).toFixed(1)}ms`, ); diff --git a/src/shared/api/acpConnection.test.ts b/src/shared/api/acpConnection.test.ts new file mode 100644 index 000000000..631dd2974 --- /dev/null +++ b/src/shared/api/acpConnection.test.ts @@ -0,0 +1,125 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => { + const urlRequests: Array<{ resolve: (url: string) => void }> = []; + const initializations: Array<{ + resolve: () => void; + reject: (error: Error) => void; + }> = []; + const streams: Array<{ writable: { abort: ReturnType } }> = []; + class MockGooseClient { + closed = new Promise(() => {}); + async initialize(): Promise { + await new Promise((resolve, reject) => + initializations.push({ resolve, reject }), + ); + } + } + return { urlRequests, initializations, streams, MockGooseClient }; +}); +vi.mock("@tauri-apps/api/core", () => ({ + invoke: vi.fn( + () => new Promise((resolve) => mocks.urlRequests.push({ resolve })), + ), +})); +vi.mock("@aaif/goose-sdk", () => ({ + DEFAULT_GOOSE_MCP_HOST_CAPABILITIES: {}, + GooseClient: mocks.MockGooseClient, +})); +vi.mock("./createWebSocketStream", () => ({ + createWebSocketStream: (url: string) => { + const stream = { + url, + writable: { abort: vi.fn().mockResolvedValue(undefined) }, + }; + mocks.streams.push(stream); + return stream; + }, +})); +describe("ACP connection lifecycle", () => { + beforeEach(() => { + mocks.urlRequests.length = 0; + mocks.initializations.length = 0; + mocks.streams.length = 0; + vi.resetModules(); + }); + it("rejects a retired URL lookup before opening its transport", async () => { + const connection = await import("./acpConnection"); + const stale = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(1)); + + await connection.invalidateClientConnection(); + const retry = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(2)); + mocks.urlRequests[0]?.resolve("ws://stale"); + await expect(stale).rejects.toThrow("initialization was superseded"); + expect(mocks.streams).toHaveLength(0); + + mocks.urlRequests[1]?.resolve("ws://current"); + await vi.waitFor(() => expect(mocks.initializations).toHaveLength(1)); + mocks.initializations[0]?.resolve(); + await expect(retry).resolves.toBeTruthy(); + expect(mocks.streams).toHaveLength(1); + expect(mocks.streams[0]).toMatchObject({ url: "ws://current" }); + }); + + it("retires and aborts an initializing transport before retry", async () => { + const connection = await import("./acpConnection"); + const stale = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(1)); + mocks.urlRequests[0]?.resolve("ws://goose"); + await vi.waitFor(() => expect(mocks.initializations).toHaveLength(1)); + await connection.invalidateClientConnection(); + expect(mocks.streams[0]?.writable.abort).toHaveBeenCalledOnce(); + const retry = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(2)); + mocks.urlRequests[1]?.resolve("ws://goose-retry"); + await vi.waitFor(() => expect(mocks.initializations).toHaveLength(2)); + mocks.initializations[1]?.resolve(); + const client = await retry; + mocks.initializations[0]?.resolve(); + await expect(stale).rejects.toThrow( + "ACP connection initialization was superseded", + ); + expect(connection.getClientSync()).toBe(client); + expect(mocks.streams[1]?.writable.abort).not.toHaveBeenCalled(); + }); + + it("rejects every waiter for a retired initialization", async () => { + const connection = await import("./acpConnection"); + const first = connection.getClient(); + const second = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(1)); + mocks.urlRequests[0]?.resolve("ws://goose"); + await vi.waitFor(() => expect(mocks.initializations).toHaveLength(1)); + + await connection.invalidateClientConnection(); + mocks.initializations[0]?.resolve(); + + await expect(first).rejects.toThrow("initialization was superseded"); + await expect(second).rejects.toThrow("initialization was superseded"); + expect(mocks.streams[0]?.writable.abort).toHaveBeenCalledOnce(); + }); + + it("aborts a transport when initialization rejects and preserves the error", async () => { + const connection = await import("./acpConnection"); + const failed = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(1)); + mocks.urlRequests[0]?.resolve("ws://goose"); + await vi.waitFor(() => expect(mocks.initializations).toHaveLength(1)); + + mocks.initializations[0]?.reject(new Error("handshake failed")); + + await expect(failed).rejects.toThrow("handshake failed"); + expect(mocks.streams[0]?.writable.abort).toHaveBeenCalledOnce(); + expect(connection.getClientSync()).toBeNull(); + + const retry = connection.getClient(); + await vi.waitFor(() => expect(mocks.urlRequests).toHaveLength(2)); + mocks.urlRequests[1]?.resolve("ws://goose-retry"); + await vi.waitFor(() => expect(mocks.initializations).toHaveLength(2)); + mocks.initializations[1]?.resolve(); + await expect(retry).resolves.toBeTruthy(); + expect(mocks.streams[1]?.writable.abort).not.toHaveBeenCalled(); + }); +}); diff --git a/src/shared/api/acpConnection.ts b/src/shared/api/acpConnection.ts index 0f69884f5..b37071809 100644 --- a/src/shared/api/acpConnection.ts +++ b/src/shared/api/acpConnection.ts @@ -61,6 +61,21 @@ export function setPermissionHandler(handler: PermissionRequestHandler): void { let clientPromise: Promise | null = null; let resolvedClient: GooseClient | null = null; let activeStream: ReturnType | null = null; +let connectionGeneration = 0; + +interface ConnectionAttempt { + generation: number; + stream: ReturnType | null; + streamAborted: boolean; +} + +let currentAttempt: ConnectionAttempt | null = null; + +async function abortAttemptStream(attempt: ConnectionAttempt): Promise { + if (!attempt.stream || attempt.streamAborted) return; + attempt.streamAborted = true; + await attempt.stream.writable.abort(); +} function createClientCallbacks(): () => Client { return () => ({ @@ -95,14 +110,16 @@ function createClientCallbacks(): () => Client { function monitorConnection( client: GooseClient, stream: ReturnType, + attempt: ConnectionAttempt, ): void { const clearCurrentConnection = () => { - if (activeStream !== stream) { + if (currentAttempt !== attempt || activeStream !== stream) { return; } resolvedClient = null; clientPromise = null; activeStream = null; + currentAttempt = null; }; client.closed .then(() => { @@ -125,16 +142,28 @@ function monitorConnection( * safer than allowing later mutations to race work still running remotely. */ export async function invalidateClientConnection(): Promise { - const stream = activeStream; + connectionGeneration += 1; + const attempt = currentAttempt; + currentAttempt = null; + const stream = attempt?.stream ?? activeStream; activeStream = null; resolvedClient = null; clientPromise = null; - if (stream) { + if (attempt) { + await abortAttemptStream(attempt); + } else if (stream) { await stream.writable.abort(); } } -async function initializeConnection(): Promise { +interface InitializedConnection { + client: GooseClient; + stream: ReturnType; +} + +async function initializeConnection( + attempt: ConnectionAttempt, +): Promise { // Dev-only: inject a real failure into startup so the WARP probe runs // for real against kgoose. `VITE_DEV_STARTUP_ERROR=warp just dev` lets // us experience the diagnostic UI with whatever real WARP state the @@ -164,10 +193,18 @@ async function initializeConnection(): Promise { perfLog( `[perf:conn] get_goose_serve_url in ${(performance.now() - tStart).toFixed(1)}ms`, ); + if ( + currentAttempt !== attempt || + attempt.generation !== connectionGeneration + ) { + throw new Error( + "ACP connection initialization was superseded; retry the operation.", + ); + } const tStream = performance.now(); const stream = createWebSocketStream(wsUrl); - activeStream = stream; + attempt.stream = stream; const client = new GooseClient(createClientCallbacks(), stream); perfLog( @@ -194,31 +231,54 @@ async function initializeConnection(): Promise { `[perf:conn] client.initialize in ${(performance.now() - tInit).toFixed(1)}ms (total ${(performance.now() - tStart).toFixed(1)}ms)`, ); - monitorConnection(client, stream); - - return client; + return { client, stream }; } export async function getClient(): Promise { - if (resolvedClient) { - return resolvedClient; - } - + if (resolvedClient) return resolvedClient; if (!clientPromise) { perfLog("[perf:conn] getClient() → initializing new ACP connection"); - clientPromise = initializeConnection() - .then((client) => { - resolvedClient = client; - return client; + const attempt: ConnectionAttempt = { + generation: connectionGeneration, + stream: null, + streamAborted: false, + }; + currentAttempt = attempt; + const initialization = initializeConnection(attempt) + .then(async ({ client, stream }) => { + if ( + currentAttempt === attempt && + attempt.generation === connectionGeneration + ) { + activeStream = stream; + resolvedClient = client; + monitorConnection(client, stream, attempt); + return client; + } + await abortAttemptStream(attempt); + throw new Error( + "ACP connection initialization was superseded; retry the operation.", + ); }) - .catch((error) => { - clientPromise = null; + .catch(async (error) => { + // initializeConnection may fail after opening the transport. Retire it + // before dropping the attempt while preserving the original failure. + await abortAttemptStream(attempt).catch((abortError) => { + console.warn( + "[acp] Failed to abort rejected connection attempt.", + abortError, + ); + }); + if (currentAttempt === attempt) { + currentAttempt = null; + clientPromise = null; + } throw error; }); + clientPromise = initialization; } else { perfLog("[perf:conn] getClient() awaiting in-flight initializeConnection"); } - return clientPromise; } diff --git a/src/shared/api/acpSessionRegistry.ts b/src/shared/api/acpSessionRegistry.ts index 83c6d0c33..82150f7f8 100644 --- a/src/shared/api/acpSessionRegistry.ts +++ b/src/shared/api/acpSessionRegistry.ts @@ -11,11 +11,17 @@ import { shortLogId, } from "@/shared/lib/reasoningEffortDiagnostics"; import { normalizeConcreteModelId } from "@/shared/lib/modelIdentity"; +import { + getModelInventoryProofRevision, + isModelSelectionAllowedByCachedInventory, +} from "@/features/providers/stores/providerModelCacheStore"; export interface AcpSessionExecutionSelection { providerId: string; /** Last model this window observed ACP acknowledge successfully. */ modelId?: string; + /** Shared inventory proof already known when ACP acknowledged this selection. */ + acknowledgedProofRevision?: number; } interface PreparedSession { @@ -30,13 +36,44 @@ interface SessionConfigMutationOptions { const SESSION_MUTATION_TIMEOUT_MS = 60_000; +interface SessionMutationQueue { + latestSequence: number; + tail: Promise; + /** Configuration intent awaiting async preflight before it can enqueue. */ + pendingSupersession?: { + sequence: number; + previousSequence: number; + settled: Promise; + resolve: () => void; + }; +} + +/** Opaque ownership of a configuration intent that is awaiting preflight. */ +export interface SessionMutationSupersession { + readonly sequence: number; + clear(): void; +} + const prepared = new Map(); -const mutationQueues = new Map< - string, - { latestSequence: number; tail: Promise } ->(); +const mutationQueues = new Map(); let nextMutationSequence = 1; +function scheduleQueueCleanup( + sessionId: string, + queue: SessionMutationQueue, +): void { + const tail = queue.tail; + void tail.then(() => { + if ( + mutationQueues.get(sessionId) === queue && + queue.tail === tail && + queue.pendingSupersession === undefined + ) { + mutationQueues.delete(sessionId); + } + }); +} + function clonePreparedSession( entry: PreparedSession | undefined, ): PreparedSession | undefined { @@ -58,12 +95,14 @@ function replaceExecutionSelection( entry.executionSelection = { providerId, ...(modelId ? { modelId } : {}), + acknowledgedProofRevision: getModelInventoryProofRevision(providerId), }; } async function runBoundedSessionMutation( sessionId: string, mutation: Promise, + invalidate: () => void, ): Promise { let timeoutId: ReturnType | undefined; let didTimeOut = false; @@ -73,6 +112,7 @@ async function runBoundedSessionMutation( new Promise((_, reject) => { timeoutId = setTimeout(() => { didTimeOut = true; + invalidate(); reject( new Error( `ACP operation timed out for session ${sessionId.slice(0, 8)}. Reconnect and retry.`, @@ -101,7 +141,12 @@ async function runBoundedSessionMutation( function serializeSessionMutation( sessionId: string, - mutation: (isLatest: () => boolean) => Promise, + mutation: ( + isLatest: () => boolean, + sequence: number, + queue: SessionMutationQueue, + canPublish: () => boolean, + ) => Promise, bounded = true, ): Promise { let queue = mutationQueues.get(sessionId); @@ -112,32 +157,104 @@ function serializeSessionMutation( const sequence = nextMutationSequence++; queue.latestSequence = sequence; - const execute = () => mutation(() => queue?.latestSequence === sequence); + let canPublish = true; + const execute = () => + mutation( + () => + queue?.pendingSupersession === undefined && + queue?.latestSequence === sequence, + sequence, + queue, + () => canPublish && queue?.pendingSupersession === undefined, + ); const result = queue.tail.then(() => - bounded ? runBoundedSessionMutation(sessionId, execute()) : execute(), + bounded + ? runBoundedSessionMutation(sessionId, execute(), () => { + canPublish = false; + }) + : execute(), ); const tail = result.then( () => undefined, () => undefined, ); queue.tail = tail; - void tail.then(() => { - if (mutationQueues.get(sessionId)?.tail === tail) { - mutationQueues.delete(sessionId); - } - }); + scheduleQueueCleanup(sessionId, queue); return result; } +function consumeSessionSupersession( + sessionId: string, + supersession: SessionMutationSupersession | undefined, +): boolean { + if (!supersession) return true; + const queue = mutationQueues.get(sessionId); + if (queue?.pendingSupersession?.sequence !== supersession.sequence) { + return false; + } + const pending = queue.pendingSupersession; + queue.pendingSupersession = undefined; + pending.resolve(); + scheduleQueueCleanup(sessionId, queue); + return true; +} + +export function supersedeSessionMutation( + sessionId: string, +): SessionMutationSupersession { + let queue = mutationQueues.get(sessionId); + if (!queue) { + queue = { latestSequence: 0, tail: Promise.resolve() }; + mutationQueues.set(sessionId, queue); + } + // Retain preflight intent before its authoritative I/O completes so a load + // cannot publish a snapshot that predates the requested configuration. + const sequence = nextMutationSequence++; + const previousSequence = + queue.pendingSupersession?.previousSequence ?? queue.latestSequence; + queue.pendingSupersession?.resolve(); + let resolveSettled!: () => void; + const settled = new Promise((resolve) => { + resolveSettled = resolve; + }); + queue.latestSequence = sequence; + queue.pendingSupersession = { + sequence, + previousSequence, + settled, + resolve: resolveSettled, + }; + scheduleQueueCleanup(sessionId, queue); + + return { + sequence, + clear() { + if (queue?.pendingSupersession?.sequence !== sequence) return; + const pending = queue.pendingSupersession; + queue.pendingSupersession = undefined; + if (queue.latestSequence === sequence) { + queue.latestSequence = pending.previousSequence; + } + pending.resolve(); + scheduleQueueCleanup(sessionId, queue); + }, + }; +} + export async function prepareSession( sessionId: string, providerId: string, workingDir: string, options: SessionConfigMutationOptions = {}, + supersession?: SessionMutationSupersession, ): Promise { - return serializeSessionMutation(sessionId, () => - prepareSessionNow(sessionId, providerId, workingDir, options), + if (!consumeSessionSupersession(sessionId, supersession)) return; + const snapshots = await serializeSessionMutation( + sessionId, + (_isLatest, _sequence, _queue, canPublish) => + prepareSessionNow(sessionId, providerId, workingDir, options, canPublish), ); + return snapshots; } async function prepareSessionNow( @@ -145,6 +262,7 @@ async function prepareSessionNow( providerId: string, workingDir: string, options: SessionConfigMutationOptions, + canPublish: () => boolean, ): Promise { const sid = sessionId.slice(0, 8); const existing = prepared.get(sessionId); @@ -163,6 +281,7 @@ async function prepareSessionNow( }); if (existing.workingDir !== workingDir) { await acpApi.updateWorkingDir(sessionId, workingDir); + if (!canPublish()) return; existing.workingDir = workingDir; changed = true; } @@ -171,27 +290,41 @@ async function prepareSessionNow( try { snapshots = await acpApi.setProvider(sessionId, providerId, { requestId: options.requestId, + canPublish, }); } catch (error) { // Goose can apply the provider and then fail while building the // response snapshot. The complete backend pair is unknown until the // UI selection is prepared again. - existing.executionSelection = undefined; + if (canPublish()) { + existing.executionSelection = undefined; + } throw error; } perfLog( `[perf:prepare] ${sid} reuse setProvider(${providerId}) in ${(performance.now() - tProv).toFixed(1)}ms`, ); - replaceExecutionSelection( - existing, - providerId, - normalizeConcreteModelId(snapshots?.model?.modelId), - ); - changed = true; + if (canPublish()) { + replaceExecutionSelection( + existing, + providerId, + normalizeConcreteModelId(snapshots?.model?.modelId), + ); + changed = true; + } } perfLog( `[perf:prepare] ${sid} reuse existing session (updates=${changed}) in ${(performance.now() - tReuse).toFixed(1)}ms`, ); + if (!snapshots && existing.executionSelection?.modelId) { + return { + model: { + modelId: existing.executionSelection.modelId, + modelName: existing.executionSelection.modelId, + }, + reasoningEffort: null, + }; + } return snapshots; } @@ -201,6 +334,7 @@ async function prepareSessionNow( providerId, }); await acpApi.loadSession(sessionId, workingDir); + if (!canPublish()) return; perfLog( `[perf:prepare] ${sid} registry loadSession ok in ${(performance.now() - tLoad).toFixed(1)}ms`, ); @@ -208,6 +342,7 @@ async function prepareSessionNow( const tProv = performance.now(); const snapshots = await acpApi.setProvider(sessionId, providerId, { requestId: options.requestId, + canPublish, }); perfLog( `[perf:prepare] ${sid} registry setProvider(${providerId}) in ${(performance.now() - tProv).toFixed(1)}ms`, @@ -221,9 +356,12 @@ async function prepareSessionNow( executionSelection: { providerId, ...(acknowledgedModelId ? { modelId: acknowledgedModelId } : {}), + acknowledgedProofRevision: getModelInventoryProofRevision(providerId), }, }; - prepared.set(sessionId, entry); + if (canPublish()) { + prepared.set(sessionId, entry); + } return snapshots; } @@ -241,8 +379,10 @@ export async function applySessionModel( if (!concreteModelId) { throw new Error(`Invalid model id: ${modelId}`); } - return serializeSessionMutation(sessionId, () => - applySessionModelNow(sessionId, concreteModelId, options), + return serializeSessionMutation( + sessionId, + (_isLatest, _sequence, _queue, canPublish) => + applySessionModelNow(sessionId, concreteModelId, options, canPublish), ); } @@ -250,6 +390,7 @@ async function applySessionModelNow( sessionId: string, modelId: string, options: SessionConfigMutationOptions, + canPublish: () => boolean, ): Promise { const sid = sessionId.slice(0, 8); const entry = prepared.get(sessionId); @@ -279,6 +420,7 @@ async function applySessionModelNow( snapshots = await acpApi.setModel(sessionId, modelId, { providerId: executionSelection.providerId, requestId: options.requestId, + canPublish, }); } catch (error) { // Drop the cached value so the next attempt retries over the wire. @@ -314,25 +456,39 @@ export async function configureSession( workingDir: string, modelId?: string, options: SessionConfigMutationOptions = {}, + supersession?: SessionMutationSupersession, ): Promise { const concreteModelId = normalizeConcreteModelId(modelId); if (modelId && !concreteModelId) { throw new Error(`Invalid model id: ${modelId}`); } - return serializeSessionMutation(sessionId, async () => { - let snapshots = await prepareSessionNow( - sessionId, - providerId, - workingDir, - concreteModelId ? {} : options, - ); - if (concreteModelId) { - snapshots = - (await applySessionModelNow(sessionId, concreteModelId, options)) ?? - snapshots; - } - return snapshots; - }); + if (!consumeSessionSupersession(sessionId, supersession)) return; + const snapshots = await serializeSessionMutation( + sessionId, + async (_isLatest, _sequence, _queue, canPublish) => { + let snapshots = await prepareSessionNow( + sessionId, + providerId, + workingDir, + concreteModelId ? {} : options, + canPublish, + ); + if (concreteModelId && canPublish()) { + const modelSnapshots = await applySessionModelNow( + sessionId, + concreteModelId, + options, + canPublish, + ); + snapshots = modelSnapshots ?? { + model: { modelId: concreteModelId, modelName: concreteModelId }, + reasoningEffort: null, + }; + } + return snapshots; + }, + ); + return snapshots; } export function applySessionConfigOption( @@ -365,43 +521,94 @@ export function requireSessionInvocationSelection( "Session requires a configured provider and model before prompting. Re-prepare the session after completing provider setup.", ); } - return { ...selection, modelId: selection.modelId }; + if ( + !isModelSelectionAllowedByCachedInventory( + selection.providerId, + selection.modelId, + selection.acknowledgedProofRevision, + ) + ) { + throw new Error( + `Session model ${selection.modelId} is no longer supported by provider ${selection.providerId}. Re-prepare the session before prompting.`, + ); + } + const { acknowledgedProofRevision: _, ...invocationSelection } = selection; + return { ...invocationSelection, modelId: selection.modelId }; } -/** Run prompt setup and transport without allowing session config to interleave. */ -export function runPreparedSessionPrompt( +/** Run invocation transport without allowing session config to interleave. */ +export async function runPreparedSessionInvocation( sessionId: string, - prompt: (providerId: string) => Promise, + invoke: ( + selection: AcpSessionExecutionSelection & { modelId: string }, + ) => Promise, ): Promise { + let pending = mutationQueues.get(sessionId)?.pendingSupersession; + while (pending) { + await pending.settled; + pending = mutationQueues.get(sessionId)?.pendingSupersession; + } return serializeSessionMutation( sessionId, - () => prompt(requireSessionInvocationSelection(sessionId).providerId), + () => invoke(requireSessionInvocationSelection(sessionId)), false, ); } +/** Run prompt setup and transport without allowing session config to interleave. */ +export function runPreparedSessionPrompt( + sessionId: string, + prompt: (providerId: string) => Promise, +): Promise { + return runPreparedSessionInvocation(sessionId, ({ providerId }) => + prompt(providerId), + ); +} + export async function loadSession( sessionId: string, workingDir: string, ): Promise<{ response: Awaited>; isCurrent: boolean; + deferredCurrent?: Promise; executionSelection?: AcpSessionExecutionSelection; }> { return serializeSessionMutation( sessionId, - async (isLatest) => { + async (isLatest, _sequence, queue) => { const response = await acpApi.loadSession(sessionId, workingDir); + const pendingAtResponse = queue.pendingSupersession; const isCurrentResult = isLatest(); + const deferredCurrent = pendingAtResponse + ? (async () => { + let pending: SessionMutationQueue["pendingSupersession"] = + pendingAtResponse; + while (pending) { + await pending.settled; + pending = queue.pendingSupersession; + } + return isLatest(); + })() + : undefined; const executionSnapshot = readSessionExecutionConfigSnapshot(response); + const executionSelection = executionSnapshot + ? { + ...executionSnapshot, + acknowledgedProofRevision: getModelInventoryProofRevision( + executionSnapshot.providerId, + ), + } + : undefined; prepared.set(sessionId, { workingDir, - executionSelection: executionSnapshot ?? undefined, + executionSelection, }); return { response, isCurrent: isCurrentResult, - executionSelection: executionSnapshot ?? undefined, + ...(deferredCurrent ? { deferredCurrent } : {}), + executionSelection, }; }, false, @@ -421,6 +628,7 @@ export function registerPreparedSession( executionSelection: { providerId, ...(acknowledgedModelId ? { modelId: acknowledgedModelId } : {}), + acknowledgedProofRevision: getModelInventoryProofRevision(providerId), }, }; diff --git a/src/shared/i18n/locales/en/chat.json b/src/shared/i18n/locales/en/chat.json index aa4d49ffa..220869501 100644 --- a/src/shared/i18n/locales/en/chat.json +++ b/src/shared/i18n/locales/en/chat.json @@ -436,6 +436,9 @@ }, "globalPill": { "ariaLabel": "Quick compose", + "invalidAgentConfiguration": "This agent’s saved provider or model is no longer available.", + "editInvalidAgent": "Edit agent", + "continueWithoutAgent": "Continue without this agent", "expand": "Expand to full chat", "placeholder": "Start a conversation", "setupVoiceConversation": "Set up Voice Conversation", diff --git a/src/shared/i18n/locales/es/chat.json b/src/shared/i18n/locales/es/chat.json index 65f824346..aa2f38220 100644 --- a/src/shared/i18n/locales/es/chat.json +++ b/src/shared/i18n/locales/es/chat.json @@ -435,6 +435,9 @@ }, "globalPill": { "ariaLabel": "Redacción rápida", + "invalidAgentConfiguration": "El proveedor o modelo guardado de este agente ya no está disponible.", + "editInvalidAgent": "Editar agente", + "continueWithoutAgent": "Continuar sin este agente", "expand": "Expandir a chat completo", "placeholder": "Inicia una conversación", "setupVoiceConversation": "Configurar conversación por voz", diff --git a/src/shared/runtime-config/modelProviderPolicy.test.ts b/src/shared/runtime-config/modelProviderPolicy.test.ts index 4195073ab..6e937e21c 100644 --- a/src/shared/runtime-config/modelProviderPolicy.test.ts +++ b/src/shared/runtime-config/modelProviderPolicy.test.ts @@ -1,10 +1,28 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { RuntimeConfig } from "./schema"; +import { notifyProviderModelInventoryInvalidated } from "./providerModelInventoryInvalidation"; import { managedGooseSelectionChanged, resolveManagedGooseProviderSelection, + resolveValidatedManagedGooseProviderSelection, } from "./modelProviderPolicy"; +const mockGetClient = vi.hoisted(() => vi.fn()); +const mockInvalidateClientConnection = vi.hoisted(() => vi.fn()); +const mockSupportedModelsList = vi.hoisted(() => vi.fn()); +vi.mock("@/shared/api/acpConnection", () => ({ + getClient: () => mockGetClient(), + invalidateClientConnection: () => mockInvalidateClientConnection(), +})); + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + const managedConfig: RuntimeConfig = { schemaVersion: 1, goose: { @@ -29,6 +47,18 @@ const managedConfig: RuntimeConfig = { }; describe("resolveManagedGooseProviderSelection", () => { + beforeEach(() => { + vi.useRealTimers(); + mockGetClient.mockReset(); + mockInvalidateClientConnection.mockReset(); + mockInvalidateClientConnection.mockResolvedValue(undefined); + mockGetClient.mockResolvedValue({ + goose: { + GooseUnstableProvidersSupportedModelsList: mockSupportedModelsList, + }, + }); + mockSupportedModelsList.mockReset(); + }); it("returns unrestricted for an empty provider list", () => { expect( resolveManagedGooseProviderSelection( @@ -58,7 +88,7 @@ describe("resolveManagedGooseProviderSelection", () => { ).toEqual({ providerId: "databricks_v2", modelId: "shared-model" }); }); - it("repairs the legacy Goose model sentinel without live inventory", () => { + it("keeps a legacy model sentinel model-free without live proof", () => { expect( resolveManagedGooseProviderSelection(managedConfig, { providerId: "databricks", @@ -66,7 +96,7 @@ describe("resolveManagedGooseProviderSelection", () => { }), ).toEqual({ providerId: "databricks_v2", - modelId: "goose-gpt-5-5", + modelId: undefined, }); }); @@ -151,14 +181,183 @@ describe("resolveManagedGooseProviderSelection", () => { }); }); - it("uses the configured default only when no model is selected", () => { + it("keeps a migrated provider model-free without live proof", () => { expect( resolveManagedGooseProviderSelection(managedConfig, { providerId: "databricks", }), ).toEqual({ + providerId: "databricks_v2", + modelId: undefined, + }); + }); + + it("validates a model retained across a provider migration", async () => { + mockSupportedModelsList.mockResolvedValue({ models: ["shared-model"] }); + + await expect( + resolveValidatedManagedGooseProviderSelection(managedConfig, { + providerId: "disallowed", + modelId: "shared-model", + }), + ).resolves.toEqual({ + providerId: "databricks_v2", + modelId: "shared-model", + }); + }); + + it("replaces an unsupported migrated model only with a proven default", async () => { + mockSupportedModelsList.mockResolvedValue({ models: ["goose-gpt-5-5"] }); + + await expect( + resolveValidatedManagedGooseProviderSelection(managedConfig, { + providerId: "disallowed", + modelId: "other-model", + }), + ).resolves.toEqual({ providerId: "databricks_v2", modelId: "goose-gpt-5-5", }); }); + + it("uses a deterministic proven inventory model when a migration has no default model", async () => { + mockSupportedModelsList.mockResolvedValue({ + models: ["z-model", "a-model"], + }); + const configWithoutDefault: RuntimeConfig = { + ...managedConfig, + goose: { ...managedConfig.goose, defaultModelId: undefined }, + }; + + await expect( + resolveValidatedManagedGooseProviderSelection(configWithoutDefault, { + providerId: "disallowed", + }), + ).resolves.toEqual({ + providerId: "databricks_v2", + modelId: "a-model", + }); + }); + + it("rejects a migration with no selected default when target inventory is empty", async () => { + mockSupportedModelsList.mockResolvedValue({ models: [] }); + const configWithoutDefault: RuntimeConfig = { + ...managedConfig, + goose: { ...managedConfig.goose, defaultModelId: undefined }, + }; + + await expect( + resolveValidatedManagedGooseProviderSelection(configWithoutDefault, { + providerId: "disallowed", + }), + ).rejects.toThrow( + "No supported model is available for migrated provider databricks_v2", + ); + }); + + it("times out a never-settling inventory proof without accepting its late result", async () => { + vi.useFakeTimers(); + let resolveInventory!: (value: { models: string[] }) => void; + mockSupportedModelsList.mockReturnValue( + new Promise<{ models: string[] }>((resolve) => { + resolveInventory = resolve; + }), + ); + + const migration = resolveValidatedManagedGooseProviderSelection( + managedConfig, + { + providerId: "disallowed", + }, + ); + const rejectedMigration = expect(migration).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + await vi.advanceTimersByTimeAsync(60_000); + await rejectedMigration; + expect(mockInvalidateClientConnection).not.toHaveBeenCalled(); + resolveInventory({ models: ["goose-gpt-5-5"] }); + await Promise.resolve(); + }); + + it("does not abort a concurrent prompt when inventory proof times out", async () => { + vi.useFakeTimers(); + const prompt = deferred(); + mockSupportedModelsList.mockReturnValue(new Promise(() => {})); + mockGetClient.mockResolvedValue({ + goose: { + GooseUnstableProvidersSupportedModelsList: mockSupportedModelsList, + prompt: () => prompt.promise, + }, + }); + + const client = await mockGetClient(); + const activePrompt = client.goose.prompt(); + const migration = resolveValidatedManagedGooseProviderSelection( + managedConfig, + { providerId: "disallowed" }, + ); + const rejectedMigration = expect(migration).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + await vi.advanceTimersByTimeAsync(60_000); + await rejectedMigration; + + expect(mockInvalidateClientConnection).not.toHaveBeenCalled(); + prompt.resolve("complete"); + await expect(activePrompt).resolves.toBe("complete"); + }); + + it("times out stalled ACP client acquisition", async () => { + vi.useFakeTimers(); + mockGetClient.mockReturnValue(new Promise(() => {})); + + const migration = resolveValidatedManagedGooseProviderSelection( + managedConfig, + { providerId: "disallowed" }, + ); + const rejectedMigration = expect(migration).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + await vi.advanceTimersByTimeAsync(60_000); + await rejectedMigration; + expect(mockInvalidateClientConnection).not.toHaveBeenCalled(); + expect(mockSupportedModelsList).not.toHaveBeenCalled(); + }); + + it("rejects an inventory proof invalidated while it is in flight", async () => { + let resolveInventory!: (value: { models: string[] }) => void; + mockSupportedModelsList.mockReturnValue( + new Promise<{ models: string[] }>((resolve) => { + resolveInventory = resolve; + }), + ); + + const migration = resolveValidatedManagedGooseProviderSelection( + managedConfig, + { + providerId: "disallowed", + }, + ); + await vi.waitFor(() => expect(mockSupportedModelsList).toHaveBeenCalled()); + notifyProviderModelInventoryInvalidated("databricks_v2"); + resolveInventory({ models: ["goose-gpt-5-5"] }); + + await expect(migration).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + }); + + it("rejects a provider migration when support cannot be proved", async () => { + mockSupportedModelsList.mockRejectedValue(new Error("offline")); + + await expect( + resolveValidatedManagedGooseProviderSelection(managedConfig, { + providerId: "disallowed", + modelId: "other-model", + }), + ).rejects.toThrow( + "Cannot verify models for migrated provider databricks_v2", + ); + }); }); diff --git a/src/shared/runtime-config/modelProviderPolicy.ts b/src/shared/runtime-config/modelProviderPolicy.ts index 59e16dc35..c29ccb5e0 100644 --- a/src/shared/runtime-config/modelProviderPolicy.ts +++ b/src/shared/runtime-config/modelProviderPolicy.ts @@ -1,5 +1,7 @@ import type { RuntimeConfig, RuntimeGooseConfig } from "./schema"; import { normalizeConcreteModelId } from "@/shared/lib/modelIdentity"; +import { getClient } from "@/shared/api/acpConnection"; +import { providerModelInventoryGeneration } from "./providerModelInventoryInvalidation"; export interface GooseProviderSelection { providerId?: string | null; @@ -18,7 +20,7 @@ export interface ManagedGooseProviderResolutionContext { targetInventoryValidated?: boolean; } -const DATABRICKS_V2_PROVIDER_ID = "databricks_v2"; +const INVENTORY_PROOF_TIMEOUT_MS = 60_000; /** * Runtime model providers define provider policy and curated model metadata. @@ -48,8 +50,8 @@ function defaultManagedProviderId(goose: RuntimeGooseConfig): string { * - `null` means policy is unrestricted; the caller must preserve its values. * - Allowed providers and all of their upstream-discovered models stay selected. * - Disallowed/missing providers move to the runtime default provider. - * - Existing model selections survive provider migration. A missing model uses - * the configured default, whose inventory entry is recommendation metadata. + * - Existing concrete model selections survive while proof is unavailable. + * - A configured default is synthesized only from successful inventory proof. */ export function resolveManagedGooseProviderSelection( config: Pick, @@ -65,20 +67,102 @@ export function resolveManagedGooseProviderSelection( (provider) => provider.id === selection.providerId, )?.id; const providerId = configuredProviderId ?? defaultManagedProviderId(goose); - let modelId = - normalizeConcreteModelId(selection.modelId) ?? - normalizeConcreteModelId(goose.defaultModelId); + const providerWasMigrated = configuredProviderId === undefined; + const selectedModelId = normalizeConcreteModelId(selection.modelId); + const defaultModelId = normalizeConcreteModelId(goose.defaultModelId); + if (context.targetInventoryValidated === true) { + const provenModelIds = context.targetModelIds ?? new Set(); + const needsModelRepair = providerWasMigrated || selection.modelId != null; + const modelId = + (selectedModelId && provenModelIds.has(selectedModelId) + ? selectedModelId + : undefined) ?? + // A default is a synthesized fallback. Do not turn same-provider, + // provider-only intent into a concrete selection merely because live + // inventory happened to be available. It may repair an existing concrete + // selection (including a legacy sentinel) or a provider migration. + (needsModelRepair && defaultModelId && provenModelIds.has(defaultModelId) + ? defaultModelId + : undefined); + return { providerId, modelId }; + } + + return { providerId, modelId: selectedModelId }; +} - if ( - providerId === DATABRICKS_V2_PROVIDER_ID && - modelId && - context.targetInventoryValidated === true && - !context.targetModelIds?.has(modelId) - ) { - modelId = goose.defaultModelId; +/** + * Read a provider's live model inventory as authoritative evidence for managed + * configuration decisions. Both ACP acquisition and the inventory RPC share + * one deadline. A timed-out proof is abandoned without invalidating the shared + * ACP transport, so unrelated active prompts remain intact. Results from an + * invalidated inventory generation are never accepted. + */ +export async function readBoundedProvenModelInventory( + providerId: string, +): Promise> { + const generationAtStart = providerModelInventoryGeneration(providerId); + let timeoutId: ReturnType | undefined; + try { + const response = await Promise.race([ + getClient().then((client) => + client.goose.GooseUnstableProvidersSupportedModelsList({ providerId }), + ), + new Promise((_, reject) => { + timeoutId = setTimeout(() => { + reject( + new Error(`Timed out proving models for provider ${providerId}.`), + ); + }, INVENTORY_PROOF_TIMEOUT_MS); + }), + ]); + if (generationAtStart !== providerModelInventoryGeneration(providerId)) { + throw new Error( + `Model inventory changed while proving provider ${providerId}.`, + ); + } + return new Set(response.models as string[]); + } finally { + if (timeoutId !== undefined) clearTimeout(timeoutId); } +} + +export async function resolveValidatedManagedGooseProviderSelection( + config: Pick, + selection: GooseProviderSelection, +): Promise { + const resolved = resolveManagedGooseProviderSelection(config, selection); + if (!resolved) return null; - return { providerId, modelId: modelId ?? undefined }; + let supportedModelIds: ReadonlySet; + try { + supportedModelIds = await readBoundedProvenModelInventory( + resolved.providerId, + ); + } catch (error) { + if (resolved.providerId === selection.providerId) return resolved; + throw new Error( + `Cannot verify models for migrated provider ${resolved.providerId}; provider selection was not changed.`, + { cause: error }, + ); + } + + const proven = resolveManagedGooseProviderSelection(config, selection, { + targetModelIds: supportedModelIds, + targetInventoryValidated: true, + }); + if (resolved.providerId === selection.providerId) return proven; + if (proven?.modelId) return proven; + + const provenInventoryFallback = [...supportedModelIds].sort()[0]; + if (provenInventoryFallback) { + return { + providerId: resolved.providerId, + modelId: provenInventoryFallback, + }; + } + throw new Error( + `No supported model is available for migrated provider ${resolved.providerId}; provider selection was not changed.`, + ); } export function managedGooseSelectionChanged( diff --git a/src/shared/runtime-config/providerModelInventoryInvalidation.ts b/src/shared/runtime-config/providerModelInventoryInvalidation.ts new file mode 100644 index 000000000..0ced94916 --- /dev/null +++ b/src/shared/runtime-config/providerModelInventoryInvalidation.ts @@ -0,0 +1,28 @@ +type ProviderModelInventoryInvalidationListener = (providerId: string) => void; + +const invalidationListeners = + new Set(); +const inventoryGenerations = new Map(); + +export function providerModelInventoryGeneration(providerId: string): number { + return inventoryGenerations.get(providerId) ?? 0; +} + +export function notifyProviderModelInventoryInvalidated( + providerId: string, +): void { + inventoryGenerations.set( + providerId, + providerModelInventoryGeneration(providerId) + 1, + ); + for (const listener of invalidationListeners) { + listener(providerId); + } +} + +export function subscribeToProviderModelInventoryInvalidation( + listener: ProviderModelInventoryInvalidationListener, +): () => void { + invalidationListeners.add(listener); + return () => invalidationListeners.delete(listener); +} diff --git a/src/shared/ui/GlobalComposerPill.test.tsx b/src/shared/ui/GlobalComposerPill.test.tsx index 02e8d841b..631722caa 100644 --- a/src/shared/ui/GlobalComposerPill.test.tsx +++ b/src/shared/ui/GlobalComposerPill.test.tsx @@ -24,6 +24,7 @@ const mockNormalizeImageBase64 = vi.fn(); const mockSearchFilesForMentions = vi.fn(); const mockResizeImage = vi.fn(); const mockGetModelsForAgent = vi.fn(); +const mockGetProvenModelsForAgent = vi.fn(); const pendingMentionLoad = new Promise(() => {}); const mockRefreshAllModelProviders = vi.fn(); const mockRefreshAgentProviderStatus = vi.fn(); @@ -89,6 +90,8 @@ vi.mock("@/features/providers/hooks/useProviderModels", () => ({ configuredModelProviderIds: ["openai", "anthropic"], modelCacheRefreshProviderIds: ["openai", "anthropic"], getModelsForAgent: (agentId: string) => mockGetModelsForAgent(agentId), + getProvenModelsForAgent: (agentId: string) => + mockGetProvenModelsForAgent(agentId), isModelInventoryAuthoritative: () => mockProviderModelsState.inventoryAuthoritative, refreshAllModelProviders: (...args: unknown[]) => @@ -255,6 +258,10 @@ describe("GlobalComposerPill", () => { vi.mocked(listSkills).mockImplementation(() => pendingMentionLoad); mockGetModelsForAgent.mockReset(); mockGetModelsForAgent.mockReturnValue([]); + mockGetProvenModelsForAgent.mockReset(); + mockGetProvenModelsForAgent.mockImplementation((agentId: string) => + mockGetModelsForAgent(agentId), + ); mockRefreshAllModelProviders.mockReset(); mockRefreshAllModelProviders.mockResolvedValue(undefined); mockRefreshAgentProviderStatus.mockReset(); @@ -776,7 +783,7 @@ describe("GlobalComposerPill", () => { }); }); - it("keeps the Composer target when a persona has no plausible target", async () => { + it("blocks a persona that has saved model metadata but no plausible target", async () => { const user = userEvent.setup(); useAgentStore.setState({ personas: [ @@ -795,11 +802,64 @@ describe("GlobalComposerPill", () => { }); await user.type(screen.getByRole("textbox"), "Hello"); - await user.click(screen.getByRole("button", { name: /send message/i })); - expectSent(onSend, "Hello", { - personaId: "persona-1", + expect( + screen.getByRole("button", { name: /send message/i }), + ).toBeDisabled(); + expect(onSend).not.toHaveBeenCalled(); + expect( + screen.getByText( + "This agent’s saved provider or model is no longer available.", + ), + ).toBeInTheDocument(); + + expect( + screen.queryByRole("button", { name: "Edit agent" }), + ).not.toBeInTheDocument(); + await user.click( + screen.getByRole("button", { name: "Continue without this agent" }), + ); + + expect( + screen.queryByText( + "This agent’s saved provider or model is no longer available.", + ), + ).not.toBeInTheDocument(); + expect(screen.getByRole("button", { name: /send message/i })).toBeEnabled(); + }); + + it("offers to edit an invalid agent without clearing its saved selection", async () => { + const user = userEvent.setup(); + const onEditAgent = vi.fn(); + useAgentStore.setState({ + personas: [ + { + id: "persona-1", + displayName: "Legacy agent", + systemPrompt: "Help.", + model: "unresolved-model", + isBuiltin: false, + writable: true, + }, + ], + }); + renderGlobalComposer(vi.fn(), { + suggestedPersonaId: "persona-1", + onEditAgent, }); + + await user.click(screen.getByRole("button", { name: "Edit agent" })); + + expect(onEditAgent).toHaveBeenCalledWith("persona-1"); + expect(screen.getByText("Legacy agent")).toBeInTheDocument(); + expect( + screen.getByText( + "This agent’s saved provider or model is no longer available.", + ), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /send message/i }), + ).toBeDisabled(); }); it("applies a legacy persona target when inventory arrives", async () => { @@ -880,6 +940,119 @@ describe("GlobalComposerPill", () => { }); }); + await user.type(screen.getByRole("textbox"), "Hello"); + + // The authoritative inventory disproves this persona's saved model, so it + // has no runnable target until the user explicitly selects a supported one. + expect( + screen.getByRole("button", { name: /send message/i }), + ).toBeDisabled(); + expect(onSend).not.toHaveBeenCalled(); + }); + + it("fails closed when authoritative inventory invalidates the active session model", async () => { + const user = userEvent.setup(); + const activeTarget = { + harnessId: "goose", + modelProviderId: "databricks_v2", + modelId: "model-a", + modelName: "Model A", + }; + mockGetModelsForAgent.mockReturnValue([ + { + id: "model-a", + name: "Model A", + providerId: "databricks_v2", + }, + ]); + mockGetProvenModelsForAgent.mockReturnValue([ + { + id: "model-a", + name: "Model A", + providerId: "databricks_v2", + }, + ]); + const onSend = vi.fn(); + const { rerender } = render( + , + ); + + await user.type(screen.getByRole("textbox"), "Hello"); + expect(screen.getByRole("button", { name: /send message/i })).toBeEnabled(); + expect(screen.getByText("Model A")).toBeInTheDocument(); + + // A successful refresh is now authoritative and excludes model A. + mockGetModelsForAgent.mockReturnValue([ + { + id: "model-b", + name: "Model B", + providerId: "databricks_v2", + }, + ]); + mockGetProvenModelsForAgent.mockReturnValue([ + { + id: "model-b", + name: "Model B", + providerId: "databricks_v2", + }, + ]); + rerender( + , + ); + + expect(screen.queryByText("Model A")).not.toBeInTheDocument(); + const send = screen.getByRole("button", { name: /send message/i }); + expect(send).toBeDisabled(); + await user.click(send); + expect(onSend).not.toHaveBeenCalled(); + }); + + it("does not synthesize an advisory model while inventory proof is unavailable", async () => { + const user = userEvent.setup(); + mockProviderModelsState.inventoryAuthoritative = false; + useAgentStore.setState({ selectedProvider: "databricks_v2" }); + mockGetModelsForAgent.mockReturnValue([ + { id: "advisory", name: "Advisory", recommended: true }, + ]); + mockGetProvenModelsForAgent.mockReturnValue([]); + const onSend = renderGlobalComposer(vi.fn(), { + currentExecutionTarget: { + harnessId: "goose", + modelProviderId: "databricks_v2", + }, + }); + + await user.type(screen.getByRole("textbox"), "Hello"); + await user.click(screen.getByRole("button", { name: /send message/i })); + + expectSent(onSend, "Hello", { + executionTarget: { + harnessId: "goose", + modelProviderId: "databricks_v2", + }, + }); + }); + + it("does not synthesize an unqualified advisory model from authoritative empty inventory", async () => { + const user = userEvent.setup(); + useAgentStore.setState({ selectedProvider: "databricks_v2" }); + mockGetModelsForAgent.mockReturnValue([ + { id: "advisory", name: "Advisory", recommended: true }, + ]); + mockGetProvenModelsForAgent.mockReturnValue([]); + const onSend = renderGlobalComposer(vi.fn(), { + currentExecutionTarget: { + harnessId: "goose", + modelProviderId: "databricks_v2", + }, + }); + await user.type(screen.getByRole("textbox"), "Hello"); await user.click(screen.getByRole("button", { name: /send message/i })); @@ -887,10 +1060,7 @@ describe("GlobalComposerPill", () => { executionTarget: { harnessId: "goose", modelProviderId: "databricks_v2", - modelId: "goose-claude-opus-4-8", - modelName: "goose-claude-opus-4-8", }, - personaId: "persona-1", }); }); @@ -1156,17 +1326,15 @@ describe("GlobalComposerPill", () => { expect(screen.getByText("UX Critic")).toBeInTheDocument(); await user.type(screen.getByRole("textbox"), "Hello"); - await user.click(screen.getByRole("button", { name: /send message/i })); - expectSent(onSend, "Hello", { - executionTarget: { - harnessId: "goose", - modelProviderId: "databricks_v2", - modelId: "goose-default", - modelName: "goose-default", - }, - personaId: "persona-2", - }); + // `goose-default` is absent from the authoritative mock inventory, so the + // newly selected persona cannot dispatch through an implicit fallback. + expect(screen.queryByText("GPT-5.5")).not.toBeInTheDocument(); + expect(screen.getByText("Goose")).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /send message/i }), + ).toBeDisabled(); + expect(onSend).not.toHaveBeenCalled(); }); it("keeps the selected provider/model after clearing the suggested persona", async () => { @@ -1194,13 +1362,10 @@ describe("GlobalComposerPill", () => { await user.type(screen.getByRole("textbox"), "Hello"); await user.click(screen.getByRole("button", { name: /send message/i })); + // The unsupported persona target never becomes a live selection, so + // clearing it retains the composer fallback without stale metadata. expectSent(onSend, "Hello", { - executionTarget: { - harnessId: "claude-acp", - modelProviderId: "claude-acp", - modelId: "claude-sonnet-4", - modelName: "claude-sonnet-4", - }, + executionTarget: { harnessId: "goose" }, }); }); @@ -1577,6 +1742,13 @@ describe("GlobalComposerPill", () => { it("expands with the controlled Home model", async () => { const user = userEvent.setup(); const onExpand = vi.fn().mockResolvedValue(true); + mockGetModelsForAgent.mockReturnValue([ + { + id: "goose-claude-fable", + name: "Claude Fable", + providerId: "anthropic", + }, + ]); renderGlobalComposer(vi.fn(), { onExpand, currentExecutionTarget: { @@ -1705,6 +1877,17 @@ describe("GlobalComposerPill", () => { it("uses a controlled external harness when the global provider differs", async () => { const user = userEvent.setup(); + mockGetModelsForAgent.mockImplementation((agentId: string) => + agentId === "claude-acp" + ? [ + { + id: "claude-opus-4-1", + name: "Claude Opus 4.1", + providerId: "claude-acp", + }, + ] + : [], + ); const onSend = renderGlobalComposer(vi.fn(), { currentExecutionTarget: { harnessId: "claude-acp", diff --git a/src/shared/ui/GlobalComposerPill.tsx b/src/shared/ui/GlobalComposerPill.tsx index 1c9795e82..e2fc8b3b8 100644 --- a/src/shared/ui/GlobalComposerPill.tsx +++ b/src/shared/ui/GlobalComposerPill.tsx @@ -18,7 +18,7 @@ import { useTranslation } from "react-i18next"; import { useProviderSelection } from "@/features/agents/hooks/useProviderSelection"; import { useAgentStore } from "@/features/agents/stores/agentStore"; import { selectPersonas } from "@/features/agents/stores/agentSelectors"; -import { personaExecutionTarget } from "@/features/agents/lib/personaExecutionTarget"; +import { resolvePersonaExecutionTarget } from "@/features/agents/lib/personaExecutionTarget"; import { useAttachmentDropTarget } from "@/features/chat/hooks/useAttachmentDropTarget"; import { useChatInputAttachments } from "@/features/chat/hooks/useChatInputAttachments"; import { useChatInputFilePicker } from "@/features/chat/hooks/useChatInputFilePicker"; @@ -101,6 +101,7 @@ interface GlobalComposerPillProps { payload: GlobalComposerExpandPayload, ) => boolean | undefined | Promise; onDismiss?: () => void; + onEditAgent?: (personaId: string) => void; onHandoffStart?: (rect: GlobalComposerHandoffRect) => void; suggestedPersonaId?: string | null; reasoningEffort?: ChatInputReasoningEffort; @@ -212,6 +213,7 @@ export function GlobalComposerPill({ onSend, onExpand, onDismiss, + onEditAgent, onHandoffStart, suggestedPersonaId = null, reasoningEffort, @@ -454,6 +456,7 @@ export function GlobalComposerPill({ pickerAgents, availableModels, getModelsForAgent, + getProvenModelsForAgent, isModelInventoryAuthoritative, modelsLoading, modelStatusMessage, @@ -497,16 +500,28 @@ export function GlobalComposerPill({ }, }); - const personaTarget = useMemo( + const personaResolution = useMemo( () => - personaExecutionTarget(selectedPersona, { + resolvePersonaExecutionTarget(selectedPersona, { providers, models: getModelsForAgent("goose"), getModelsForHarness: getModelsForAgent, + getProvenModelsForHarness: getProvenModelsForAgent, + isModelInventoryAuthoritative, catalogEntries, }), - [catalogEntries, getModelsForAgent, providers, selectedPersona], + [ + catalogEntries, + getModelsForAgent, + getProvenModelsForAgent, + isModelInventoryAuthoritative, + providers, + selectedPersona, + ], ); + const personaTarget = + personaResolution.status === "valid" ? personaResolution.target : undefined; + const personaConfigurationInvalid = personaResolution.status === "invalid"; useEffect(() => { if (!selectedPersonaId) { @@ -570,10 +585,21 @@ export function GlobalComposerPill({ ? selectedProviderForPicker : null; const defaultModelSelection = useMemo(() => { + const provenModels = getProvenModelsForAgent(selectedAgentId); + const selectableModels = availableModels.filter((model) => { + const providerId = model.providerId ?? concreteSelectedProviderId; + return provenModels.some( + (proven) => + proven.id === model.id && + (!providerId || + !proven.providerId || + proven.providerId === providerId), + ); + }); const storedPreference = getStoredModelPreference(selectedAgentId); if (storedPreference) { const matchingModel = findMatchingModel( - availableModels, + selectableModels, storedPreference.modelId, storedPreference.providerId, ); @@ -609,7 +635,7 @@ export function GlobalComposerPill({ gooseDefaultSelection.modelProviderId === concreteSelectedProviderId) ) { const matchingDefault = findMatchingModel( - availableModels, + selectableModels, gooseDefaultSelection.modelId, gooseDefaultSelection.modelProviderId, ); @@ -619,25 +645,21 @@ export function GlobalComposerPill({ selectedProviderForPicker, ); } - if ( - !isModelInventoryAuthoritative(gooseDefaultSelection.modelProviderId) - ) { - return gooseDefaultSelection; - } } const compatibleModels = concreteSelectedProviderId - ? availableModels.filter( + ? selectableModels.filter( (model) => !model.providerId || model.providerId === concreteSelectedProviderId, ) - : availableModels; + : selectableModels; return getPreferredModel(compatibleModels, selectedProviderForPicker); }, [ availableModels, concreteSelectedProviderId, + getProvenModelsForAgent, gooseDefaultSelection, isModelInventoryAuthoritative, selectedAgentId, @@ -651,20 +673,51 @@ export function GlobalComposerPill({ if (!currentExecutionTarget?.modelId) { return null; } + const modelProviderId = currentExecutionTarget.modelProviderId; + if ( + modelProviderId && + isModelInventoryAuthoritative(modelProviderId) && + !getProvenModelsForAgent(currentExecutionTarget.harnessId).some( + (model) => + model.id === currentExecutionTarget.modelId && + (!model.providerId || model.providerId === modelProviderId), + ) + ) { + return null; + } return { modelProviderId: currentExecutionTarget.modelProviderId, modelId: currentExecutionTarget.modelId, modelName: currentExecutionTarget.modelName, }; - }, [currentExecutionTarget]); + }, [ + currentExecutionTarget, + getProvenModelsForAgent, + isModelInventoryAuthoritative, + ]); const hasLocalExecutionOverride = providerOverride !== null || modelOverride !== null; + const personaSelectionOverridden = + personaOverrideUserOverrideForRef.current === selectedPersonaId; + // A persona target is the configuration sent to the runtime. Materialize the + // picker from that exact target: a provider-only target deliberately has no + // model selection and must not borrow a default model for display. + const personaModelSelection = + !personaSelectionOverridden && personaTarget?.modelId + ? { + modelProviderId: personaTarget.modelProviderId, + modelId: personaTarget.modelId, + modelName: personaTarget.modelName, + } + : null; const effectiveModelSelection = - modelOverride ?? - (!hasLocalExecutionOverride && currentExecutionTarget !== undefined - ? controlledModelSelection - : defaultModelSelection); + !personaSelectionOverridden && personaTarget + ? personaModelSelection + : (modelOverride ?? + (!hasLocalExecutionOverride && currentExecutionTarget !== undefined + ? controlledModelSelection + : defaultModelSelection)); const localExecutionTarget = useMemo( () => hasLocalExecutionOverride || currentExecutionTarget === undefined @@ -682,12 +735,19 @@ export function GlobalComposerPill({ selectedProviderForPicker, ], ); - const personaSelectionOverridden = - personaOverrideUserOverrideForRef.current === selectedPersonaId; + const personaHasSavedExecutionTarget = Boolean( + selectedPersona?.provider || + selectedPersona?.modelProviderId || + selectedPersona?.model, + ); + const controlledTargetInvalidated = + currentExecutionTarget?.modelId != null && controlledModelSelection == null; const effectiveExecutionTarget = - !personaSelectionOverridden && personaTarget + !personaSelectionOverridden && personaHasSavedExecutionTarget ? personaTarget - : (localExecutionTarget ?? currentExecutionTarget ?? undefined); + : (localExecutionTarget ?? + (controlledTargetInvalidated ? undefined : currentExecutionTarget) ?? + undefined); const canSend = hasSendableContent && Boolean(effectiveExecutionTarget) && @@ -1463,6 +1523,35 @@ export function GlobalComposerPill({ + {personaConfigurationInvalid && !personaSelectionOverridden ? ( +
+ {t("globalPill.invalidAgentConfiguration")} +
+ {selectedPersonaId && onEditAgent ? ( + + ) : null} + +
+
+ ) : null} +