From 5f7b9d8652b3dc21e1b7e58f91eb1366b2be1314 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 10 May 2026 03:14:29 +0200 Subject: [PATCH] fix(api): provider-chain GET preserves disabled entries (H2 code-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GET response used to surface the resolveProviderChain()-filtered list, which DROPS disabled entries upstream. The Settings UI then seeded `enabled: true` for every returned row, so disabling an entry made it disappear on next refetch and lose its toggle position — the toggle was functionally a "remove from chain" button. Changed GET to read the *persisted* chain via parseProviderChain() so disabled rows still round-trip with their flag. The active-provider summary still uses resolveProviderChain so the displayed "Active: …" line continues to reflect the actual runner ordering (enabled-only). The Settings UI seeds entries with the wire-supplied `enabled` value instead of always-true, so re-enabling becomes possible without a page reload. 4 unit tests pin the GET shape (persisted-chain ordering, disabled entries surfaced, cached-active overlay, default-chain fallback). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../provider-chain/__tests__/route.test.ts | 87 +++++++++++++++++-- src/app/api/insights/provider-chain/route.ts | 33 +++++-- src/components/settings/ai-section.tsx | 23 +++-- 3 files changed, 126 insertions(+), 17 deletions(-) diff --git a/src/app/api/insights/provider-chain/__tests__/route.test.ts b/src/app/api/insights/provider-chain/__tests__/route.test.ts index 40d60752c..102f21e95 100644 --- a/src/app/api/insights/provider-chain/__tests__/route.test.ts +++ b/src/app/api/insights/provider-chain/__tests__/route.test.ts @@ -14,6 +14,11 @@ vi.mock("@/lib/api-handler", () => ({ user: { id: "u-1" }, session: { id: "s-1" }, })), + HttpError: class HttpError extends Error { + constructor(public status: number, message: string) { + super(message); + } + }, })); vi.mock("@/lib/ai/provider", () => ({ @@ -28,25 +33,47 @@ vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn(), })); +vi.mock("@/lib/db", () => ({ + prisma: { + user: { + findUnique: vi.fn(async () => ({ aiProviderChain: null })), + }, + }, +})); + import { GET } from "../route"; import { resolveProviderChain } from "@/lib/ai/provider"; import { getLastWorkingProvider } from "@/lib/ai/provider-runner"; +import { prisma } from "@/lib/db"; interface Envelope { data?: { activeProvider: string | null; cachedActiveProvider: string | null; - configuredChain: { providerType: string; available: boolean }[]; + configuredChain: { + providerType: string; + enabled: boolean; + available: boolean; + }[]; }; error?: string; } beforeEach(() => { vi.clearAllMocks(); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + aiProviderChain: null, + } as never); }); describe("GET /api/insights/provider-chain", () => { - it("returns the resolved chain in priority order", async () => { + it("returns the persisted chain (with enabled flags) and the active resolver winner", async () => { + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + aiProviderChain: [ + { providerType: "codex", priority: 1, enabled: true }, + { providerType: "admin-openai", priority: 2, enabled: true }, + ], + } as never); vi.mocked(resolveProviderChain).mockResolvedValue([ { providerType: "codex", @@ -64,12 +91,50 @@ describe("GET /api/insights/provider-chain", () => { const body = (await res.json()) as Envelope; expect(body.data?.activeProvider).toBe("codex"); expect(body.data?.configuredChain).toEqual([ - { providerType: "codex", available: true }, - { providerType: "admin-openai", available: true }, + { providerType: "codex", enabled: true, available: true }, + { providerType: "admin-openai", enabled: true, available: true }, ]); }); + it("surfaces disabled entries on the wire so the user can re-enable them", async () => { + // v1.4.16 phase D reconcile (code-review H2) — the GET response + // used to filter through resolveProviderChain which DROPS disabled + // entries; the UI then lost them on next refetch. The persisted + // chain is the source of truth on the wire. + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + aiProviderChain: [ + { providerType: "codex", priority: 1, enabled: true }, + { providerType: "openai", priority: 2, enabled: false }, + { providerType: "admin-openai", priority: 3, enabled: true }, + ], + } as never); + vi.mocked(resolveProviderChain).mockResolvedValue([ + { providerType: "codex", instance: { type: "codex" } as never }, + { + providerType: "admin-openai", + instance: { type: "admin-key" } as never, + }, + ]); + vi.mocked(getLastWorkingProvider).mockReturnValue(null); + + const res = await (GET as () => Promise)(); + const body = (await res.json()) as Envelope; + expect(body.data?.configuredChain).toEqual([ + { providerType: "codex", enabled: true, available: true }, + { providerType: "openai", enabled: false, available: true }, + { providerType: "admin-openai", enabled: true, available: true }, + ]); + // Active still reflects the resolver winner (enabled-only). + expect(body.data?.activeProvider).toBe("codex"); + }); + it("surfaces the cached last-working provider when available", async () => { + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + aiProviderChain: [ + { providerType: "codex", priority: 1, enabled: true }, + { providerType: "openai", priority: 2, enabled: true }, + ], + } as never); vi.mocked(resolveProviderChain).mockResolvedValue([ { providerType: "codex", instance: { type: "codex" } as never }, { @@ -88,13 +153,23 @@ describe("GET /api/insights/provider-chain", () => { expect(body.data?.activeProvider).toBe("codex"); }); - it("returns an empty chain + null active when nothing is configured", async () => { + it("falls back to the default chain when nothing is configured", async () => { + // parseProviderChain(null) returns PROVIDER_CHAIN_DEFAULT — every + // user therefore renders a populated list, not an empty one. The + // active line still reflects the resolver, which can be empty if + // no credentials are configured. + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + aiProviderChain: null, + } as never); vi.mocked(resolveProviderChain).mockResolvedValue([]); vi.mocked(getLastWorkingProvider).mockReturnValue(null); const res = await (GET as () => Promise)(); const body = (await res.json()) as Envelope; expect(body.data?.activeProvider).toBeNull(); - expect(body.data?.configuredChain).toEqual([]); + expect(body.data?.configuredChain.length).toBeGreaterThan(0); + expect(body.data?.configuredChain.every((e) => e.enabled === true)).toBe( + true, + ); }); }); diff --git a/src/app/api/insights/provider-chain/route.ts b/src/app/api/insights/provider-chain/route.ts index 5860eea01..24036d1f1 100644 --- a/src/app/api/insights/provider-chain/route.ts +++ b/src/app/api/insights/provider-chain/route.ts @@ -7,7 +7,10 @@ import { annotate } from "@/lib/logging/context"; import { resolveProviderChain } from "@/lib/ai/provider"; import { getLastWorkingProvider } from "@/lib/ai/provider-runner"; import { prisma } from "@/lib/db"; -import { PROVIDER_CHAIN_TYPES } from "@/lib/ai/provider-chain"; +import { + parseProviderChain, + PROVIDER_CHAIN_TYPES, +} from "@/lib/ai/provider-chain"; /** * v1.4.16 phase B5b — read-only chain summary for the Settings → AI @@ -34,23 +37,41 @@ import { PROVIDER_CHAIN_TYPES } from "@/lib/ai/provider-chain"; */ export const GET = apiHandler(async () => { const { user } = await requireAuth(); - const chain = await resolveProviderChain(user.id); + + // v1.4.16 phase D reconcile (code-review H2) — the GET response used + // to surface the resolveProviderChain()-filtered list, which DROPS + // disabled entries. The Settings UI seeded `enabled: true` for every + // returned row, so disabling an entry made it disappear from the + // list and lose its toggle position. Read the *persisted* chain + // directly via parseProviderChain() so disabled rows still round- + // trip and the user can re-enable them. The active-provider summary + // still uses resolveProviderChain so the displayed "active = …" + // matches the actual runner ordering (enabled entries only). + const userRow = await prisma.user.findUnique({ + where: { id: user.id }, + select: { aiProviderChain: true }, + }); + const persisted = parseProviderChain(userRow?.aiProviderChain ?? null); + + const resolved = await resolveProviderChain(user.id); const cached = getLastWorkingProvider(user.id); annotate({ action: { name: "insights.provider_chain.get" }, meta: { - chain_length: chain.length, - chain_active: chain[0]?.providerType ?? null, + chain_length: persisted.length, + chain_enabled_count: persisted.filter((e) => e.enabled).length, + chain_active: resolved[0]?.providerType ?? null, chain_cached_active: cached, }, }); return apiSuccess({ - activeProvider: chain[0]?.providerType ?? null, + activeProvider: resolved[0]?.providerType ?? null, cachedActiveProvider: cached, - configuredChain: chain.map((entry) => ({ + configuredChain: persisted.map((entry) => ({ providerType: entry.providerType, + enabled: entry.enabled, available: true, })), }); diff --git a/src/components/settings/ai-section.tsx b/src/components/settings/ai-section.tsx index 072010890..9bf81fcb1 100644 --- a/src/components/settings/ai-section.tsx +++ b/src/components/settings/ai-section.tsx @@ -83,7 +83,11 @@ interface ChainEntry { interface ProviderChainData { activeProvider: ProviderType | null; cachedActiveProvider: ProviderType | null; - configuredChain: { providerType: ProviderType; available: boolean }[]; + configuredChain: { + providerType: ProviderType; + enabled: boolean; + available: boolean; + }[]; } const DEFAULT_CHAIN: readonly ChainEntry[] = [ @@ -1257,7 +1261,11 @@ function FallbackChainCard({ selected, onSelect, }: { - chain: { providerType: ProviderType; available: boolean }[]; + chain: { + providerType: ProviderType; + enabled: boolean; + available: boolean; + }[]; selected: ProviderType; onSelect: (next: ProviderType) => void; }) { @@ -1267,17 +1275,22 @@ function FallbackChainCard({ // Local working copy. Server-confirmed values arrive via `chain` // prop; we keep our own state so the user can shuffle multiple rows // before clicking "Save chain order". - const seededKey = chain.map((c) => c.providerType).join(","); + // v1.4.16 phase D reconcile (code-review H2) — `enabled` from the + // wire is now the canonical state. The GET endpoint surfaces the + // raw persisted chain so a disabled entry survives the round-trip. + const seededKey = chain + .map((c) => `${c.providerType}:${c.enabled ? 1 : 0}`) + .join(","); const [seeded, setSeeded] = useState(null); const [entries, setEntries] = useState(() => - chain.map((c) => ({ providerType: c.providerType, enabled: true })), + chain.map((c) => ({ providerType: c.providerType, enabled: c.enabled })), ); if (seededKey !== seeded) { setSeeded(seededKey); setEntries( chain.map((c) => ({ providerType: c.providerType, - enabled: true, + enabled: c.enabled, })), ); }