From 9e845556e90048fa4c03354dc63c36c40cf6ac7d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 08:10:03 +0000 Subject: [PATCH] refactor: move per-provider catalog merge rules into a tested module --- .../services/coderGatewayCatalogMerge.test.ts | 200 ++++++++++++++++++ src/node/services/coderGatewayCatalogMerge.ts | 162 ++++++++++++++ src/node/services/coderOauthService.ts | 118 ++--------- 3 files changed, 375 insertions(+), 105 deletions(-) create mode 100644 src/node/services/coderGatewayCatalogMerge.test.ts create mode 100644 src/node/services/coderGatewayCatalogMerge.ts diff --git a/src/node/services/coderGatewayCatalogMerge.test.ts b/src/node/services/coderGatewayCatalogMerge.test.ts new file mode 100644 index 00000000000..bb418258ef2 --- /dev/null +++ b/src/node/services/coderGatewayCatalogMerge.test.ts @@ -0,0 +1,200 @@ +import { describe, expect, it } from "bun:test"; + +import type { CoderGatewayProvider } from "@/common/constants/coderOAuth"; +import { + readPriorCoderCatalog, + reconcileProviderCatalog, + type PriorCoderCatalog, +} from "./coderGatewayCatalogMerge"; + +const anthropic: CoderGatewayProvider = { name: "anthropic", type: "anthropic" }; +const custom: CoderGatewayProvider = { name: "corp", type: "openai" }; + +function prior(overrides: Partial = {}): PriorCoderCatalog { + return { known: true, discovered: [], stale: [], providers: [], ...overrides }; +} + +describe("readPriorCoderCatalog", () => { + it("reads the catalog markers and keeps only string IDs", () => { + expect( + readPriorCoderCatalog({ + discoveredModels: ["anthropic/a", 7, null, "openai/b"], + staleDiscoveredModels: ["corp/c", { id: "x" }], + discoveredProviders: [custom, { name: 3 }], + }) + ).toEqual({ + known: true, + discovered: ["anthropic/a", "openai/b"], + stale: ["corp/c"], + providers: [custom], + }); + }); + + it("treats a missing or non-array catalog as unknown", () => { + expect(readPriorCoderCatalog(undefined)).toEqual({ + known: false, + discovered: [], + stale: [], + providers: [], + }); + expect(readPriorCoderCatalog({ discoveredModels: "anthropic/a" }).known).toBe(false); + }); + + it("knows an empty catalog: an empty list is an authoritative answer", () => { + expect(readPriorCoderCatalog({ discoveredModels: [] }).known).toBe(true); + }); +}); + +describe("reconcileProviderCatalog", () => { + describe("ok", () => { + it("replaces the provider's entries and persists no metadata under a listing", () => { + expect( + reconcileProviderCatalog( + anthropic, + { kind: "ok", ids: ["anthropic/new"] }, + prior({ discovered: ["anthropic/old"] }), + true + ) + ).toEqual({ ids: ["anthropic/new"], providers: [], inconclusive: false }); + }); + + it("on the probe path persists the stored metadata over the probe default", () => { + const stored = { name: "corp", type: "anthropic" }; + const probed = { name: "corp", type: "corp" }; + expect( + reconcileProviderCatalog( + probed, + { kind: "ok", ids: ["corp/m"] }, + prior({ providers: [stored] }), + false + ).providers + ).toEqual([stored]); + expect( + reconcileProviderCatalog(probed, { kind: "ok", ids: [] }, prior(), false).providers + ).toEqual([probed]); + }); + }); + + describe("error", () => { + it("carries prior entries under the provider prefix forward", () => { + expect( + reconcileProviderCatalog( + anthropic, + { kind: "error" }, + prior({ discovered: ["anthropic/a", "openai/b", "anthropicx/c"] }), + true + ) + ).toEqual({ ids: ["anthropic/a"], providers: [], inconclusive: false }); + }); + + it("counts stored metadata as prior state even without prior entries", () => { + expect( + reconcileProviderCatalog(custom, { kind: "error" }, prior({ providers: [custom] }), true) + ).toEqual({ ids: [], providers: [], inconclusive: false }); + }); + + it("keeps the stored metadata on the probe path", () => { + expect( + reconcileProviderCatalog( + custom, + { kind: "error" }, + prior({ providers: [custom], discovered: ["corp/m"] }), + false + ) + ).toEqual({ ids: ["corp/m"], providers: [custom], inconclusive: false }); + }); + + it("makes the catalog inconclusive without prior state and keeps stale entries", () => { + expect( + reconcileProviderCatalog( + anthropic, + { kind: "error" }, + prior({ known: false, stale: ["anthropic/s", "openai/t"] }), + true + ) + ).toEqual({ ids: ["anthropic/s"], providers: [], inconclusive: true }); + }); + + it("is inconclusive for a provider the prior catalog never named", () => { + expect( + reconcileProviderCatalog( + anthropic, + { kind: "error" }, + prior({ discovered: ["openai/b"] }), + true + ) + ).toEqual({ ids: [], providers: [], inconclusive: true }); + }); + + it("drops entries of a provider whose type changed under a listing", () => { + const retyped = { name: "corp", type: "anthropic" }; + expect( + reconcileProviderCatalog( + retyped, + { kind: "error" }, + prior({ providers: [custom], discovered: ["corp/m"], stale: ["corp/s"] }), + true + ) + ).toEqual({ ids: [], providers: [], inconclusive: true }); + }); + + it("treats a name without stored metadata as its own type", () => { + // No stored metadata: the effective prior type is the name, so a listed + // instance named "anthropic" of another type has no carry-forward state. + expect( + reconcileProviderCatalog( + { name: "anthropic", type: "openai" }, + { kind: "error" }, + prior({ discovered: ["anthropic/a"] }), + true + ) + ).toEqual({ ids: [], providers: [], inconclusive: true }); + }); + }); + + describe("unavailable", () => { + it("drops the provider under an authoritative listing", () => { + expect( + reconcileProviderCatalog( + custom, + { kind: "unavailable" }, + prior({ providers: [custom], discovered: ["corp/m"] }), + true + ) + ).toEqual({ ids: [], providers: [], inconclusive: false }); + }); + + it("drops a probe-default name without stored metadata", () => { + expect( + reconcileProviderCatalog( + anthropic, + { kind: "unavailable" }, + prior({ discovered: ["anthropic/a"] }), + false + ) + ).toEqual({ ids: [], providers: [], inconclusive: false }); + }); + + it("keeps a recorded instance and its prior entries on the probe path", () => { + expect( + reconcileProviderCatalog( + custom, + { kind: "unavailable" }, + prior({ providers: [custom], discovered: ["corp/m", "openai/x"] }), + false + ) + ).toEqual({ ids: ["corp/m"], providers: [custom], inconclusive: false }); + }); + + it("keeps only stale entries, inconclusively, when the prior catalog is unknown", () => { + expect( + reconcileProviderCatalog( + custom, + { kind: "unavailable" }, + prior({ known: false, providers: [custom], stale: ["corp/s"] }), + false + ) + ).toEqual({ ids: ["corp/s"], providers: [custom], inconclusive: true }); + }); + }); +}); diff --git a/src/node/services/coderGatewayCatalogMerge.ts b/src/node/services/coderGatewayCatalogMerge.ts new file mode 100644 index 00000000000..8196cc56ae8 --- /dev/null +++ b/src/node/services/coderGatewayCatalogMerge.ts @@ -0,0 +1,162 @@ +/** + * Pure per-provider merge rules for a Coder AI Gateway catalog refresh. + * + * CoderOauthService.refreshBridgeModelsSerializedEffect fetches every + * provider's catalog, then commits the merged result inside the providers-file + * lock. The rules that decide what each provider contributes live here so they + * can be tested without the OAuth service, the lock or the network. + */ +import { parseCoderGatewayProviders } from "@/common/constants/coderOAuth"; +import type { CoderGatewayProvider } from "@/common/constants/coderOAuth"; + +/** One provider catalog request, classified (see fetchProviderCatalogEffect). */ +export type CoderProviderCatalogResult = + | { kind: "ok"; ids: string[] } + | { kind: "unavailable" } + | { kind: "error" }; + +/** Catalog state persisted before this refresh, read INSIDE the locked mutation. */ +export interface PriorCoderCatalog { + /** True when an authoritative `discoveredModels` list exists. */ + known: boolean; + /** Model IDs of the authoritative catalog (empty when not known). */ + discovered: string[]; + /** + * Catalog IDs carried through an earlier inconclusive refresh (the + * authoritative catalog was deleted then; this marker is all that remains + * of it). Never authoritative prior state — it only carries a provider's + * entries forward across consecutive failed refreshes. + */ + stale: string[]; + /** Provider metadata persisted by earlier discovery. */ + providers: CoderGatewayProvider[]; +} + +/** What one provider contributes to the next catalog write. */ +export interface ProviderCatalogContribution { + /** Model IDs to persist for this provider. */ + ids: string[]; + /** Provider metadata to persist on the probe path: zero or one entry. */ + providers: CoderGatewayProvider[]; + /** True when this provider makes the whole catalog inconclusive. */ + inconclusive: boolean; +} + +function stringEntries(value: unknown): string[] { + return Array.isArray(value) ? value.filter((id): id is string => typeof id === "string") : []; +} + +export function readPriorCoderCatalog( + section: Record | undefined +): PriorCoderCatalog { + return { + known: Array.isArray(section?.discoveredModels), + discovered: stringEntries(section?.discoveredModels), + stale: stringEntries(section?.staleDiscoveredModels), + providers: parseCoderGatewayProviders(section?.discoveredProviders), + }; +} + +function idsUnderProvider(ids: string[], providerName: string): string[] { + return ids.filter((id) => id.startsWith(`${providerName}/`)); +} + +/** + * Per-provider conclusiveness: "ok" replaces the provider's entries, + * "unavailable" (404) conclusively clears them, and a transient "error" + * carries the provider's previous entries forward — one provider whose + * upstream rejects /models (Bedrock) or merely flaked must not poison + * every other provider's refresh, and a recovered provider heals on the + * next refresh instead of the next login. Carry-forward requires PRIOR + * STATE for that provider (a prior catalog naming it, or prior entries + * under its prefix): an errored provider without prior state — fresh + * login, or an instance the admin just added — makes the whole catalog + * inconclusive, because persisting the other providers' lists would + * read as an authoritative catalog that blocks every model of the + * failed provider until a later successful refresh. + * + * `authoritative` is true when the provider set came from the admin listing, + * false when it came from probing default and known names. + */ +export function reconcileProviderCatalog( + provider: CoderGatewayProvider, + result: CoderProviderCatalogResult, + prior: PriorCoderCatalog, + authoritative: boolean +): ProviderCatalogContribution { + const previous = prior.providers.find((prev) => prev.name === provider.name); + // Prior state must also TYPE-match under an authoritative listing: + // when the admin changes an instance's type (same name), the old + // type's model IDs are not valid carry-forward state — the write + // persists the NEW type, so carried entries would stay selectable and + // be sent over the new wire protocol until a successful refresh. + // A name with no stored metadata previously resolved via the + // name === type default, so that default is its effective prior type. + // The probe path persists `previous` (not the probe default), so no + // type change can occur there and the check is skipped. + const previousType = previous?.type ?? provider.name; + const typeUnchanged = !authoritative || previousType === provider.type; + const hasPriorState = + prior.known && + typeUnchanged && + (previous !== undefined || prior.discovered.some((id) => id.startsWith(`${provider.name}/`))); + if (result.kind === "ok") { + // The probe confirmed the route exists; prefer previously stored + // metadata (its type may come from an earlier authoritative + // listing) over the probe-derived name === type default. + return { + ids: result.ids, + providers: authoritative ? [] : [previous ?? provider], + inconclusive: false, + }; + } + if (result.kind === "error") { + if (!hasPriorState) { + // Consecutive failed refreshes: after an inconclusive refresh the + // authoritative catalog is gone (prior.known false), so a + // transiently failing provider has no carry-forward state — but + // its stale entries must not vanish (a later successful refresh + // would otherwise start from nothing). The catalog stays + // inconclusive, so these IDs land back in staleDiscoveredModels, + // never in discoveredModels. Type-gated like authoritative + // carry-forward. + return { + ids: typeUnchanged ? idsUnderProvider(prior.stale, provider.name) : [], + providers: [], + inconclusive: true, + }; + } + return { + ids: idsUnderProvider(prior.discovered, provider.name), + providers: !authoritative && previous ? [previous] : [], + inconclusive: false, + }; + } + if (!authoritative && previous !== undefined) { + // "unavailable" on the PROBE path for an instance recorded by an + // earlier authoritative listing: the 404 proves only that the + // upstream serves no /models route — not that the admin removed + // the gateway instance (only an authoritative listing can say + // that). Dropping it would leave manually configured + // coder:/ entries unresolvable until a listing + // succeeds again, so keep the metadata and carry prior entries + // forward like a transient error. Without an authoritative prior + // catalog, stale entries must not be promoted into a conclusive write. + return prior.known + ? { + ids: idsUnderProvider(prior.discovered, provider.name), + providers: [previous], + inconclusive: false, + } + : { + ids: idsUnderProvider(prior.stale, provider.name), + providers: [previous], + inconclusive: true, + }; + } + // "unavailable" otherwise: the authoritative listing already names + // every existing instance (a listed instance whose /models 404s + // conclusively serves no catalog), and probe-default names carry no + // recorded metadata — the route is conclusively absent; drop it. + return { ids: [], providers: [], inconclusive: false }; +} diff --git a/src/node/services/coderOauthService.ts b/src/node/services/coderOauthService.ts index 79962f8a4c3..dd8d31395c2 100644 --- a/src/node/services/coderOauthService.ts +++ b/src/node/services/coderOauthService.ts @@ -40,6 +40,10 @@ import type { ProviderService } from "@/node/services/providerService"; import type { WindowService } from "@/node/services/windowService"; import { log } from "@/node/services/log"; import { AsyncMutex } from "@/node/utils/concurrency/asyncMutex"; +import { + readPriorCoderCatalog, + reconcileProviderCatalog, +} from "@/node/services/coderGatewayCatalogMerge"; import { isCoderOauthAuthExpired, parseCoderOauthAuth, @@ -2586,111 +2590,15 @@ export class CoderOauthService { } // Prior catalog state must be read INSIDE the locked mutation (a // concurrent login/refresh may have changed it since discovery began). - const previousKnown = Array.isArray(section?.discoveredModels); - const previousDiscovered = Array.isArray(section?.discoveredModels) - ? section.discoveredModels.filter((id): id is string => typeof id === "string") - : []; - // Catalog IDs carried through an earlier inconclusive refresh (the - // authoritative catalog was deleted then; this marker is all that - // remains of it). Never authoritative prior state — it only carries a - // provider's entries forward across consecutive failed refreshes. - const staleDiscovered = Array.isArray(section?.staleDiscoveredModels) - ? section.staleDiscoveredModels.filter((id): id is string => typeof id === "string") - : []; - const previousProviders = parseCoderGatewayProviders(section?.discoveredProviders); - - // Per-provider conclusiveness: "ok" replaces the provider's entries, - // "unavailable" (404) conclusively clears them, and a transient "error" - // carries the provider's previous entries forward — one provider whose - // upstream rejects /models (Bedrock) or merely flaked must not poison - // every other provider's refresh, and a recovered provider heals on the - // next refresh instead of the next login. Carry-forward requires PRIOR - // STATE for that provider (a prior catalog naming it, or prior entries - // under its prefix): an errored provider without prior state — fresh - // login, or an instance the admin just added — makes the whole catalog - // inconclusive, because persisting the other providers' lists would - // read as an authoritative catalog that blocks every model of the - // failed provider until a later successful refresh. - const modelIds: string[] = []; - const nextProviders: CoderGatewayProvider[] = []; - for (const { provider, result } of results) { - const previous = previousProviders.find((prev) => prev.name === provider.name); - // Prior state must also TYPE-match under an authoritative listing: - // when the admin changes an instance's type (same name), the old - // type's model IDs are not valid carry-forward state — the write - // persists the NEW type, so carried entries would stay selectable and - // be sent over the new wire protocol until a successful refresh. - // A name with no stored metadata previously resolved via the - // name === type default, so that default is its effective prior type. - // The probe path persists `previous` (not the probe default), so no - // type change can occur there and the check is skipped. - const previousType = previous?.type ?? provider.name; - const typeUnchanged = !authoritative || previousType === provider.type; - const hasPriorState = - previousKnown && - typeUnchanged && - (previous !== undefined || - previousDiscovered.some((id) => id.startsWith(`${provider.name}/`))); - if (result.kind === "ok") { - modelIds.push(...result.ids); - if (!authoritative) { - // The probe confirmed the route exists; prefer previously stored - // metadata (its type may come from an earlier authoritative - // listing) over the probe-derived name === type default. - nextProviders.push(previous ?? provider); - } - } else if (result.kind === "error") { - if (!hasPriorState) { - catalogInconclusive = true; - // Consecutive failed refreshes: after an inconclusive refresh the - // authoritative catalog is gone (previousKnown false), so a - // transiently failing provider has no carry-forward state — but - // its stale entries must not vanish (a later successful refresh - // would otherwise start from nothing). The catalog stays - // inconclusive, so these IDs land back in staleDiscoveredModels, - // never in discoveredModels. Type-gated like authoritative - // carry-forward. - if (typeUnchanged) { - modelIds.push( - ...staleDiscovered.filter((id) => id.startsWith(`${provider.name}/`)) - ); - } - continue; - } - modelIds.push( - ...previousDiscovered.filter((id) => id.startsWith(`${provider.name}/`)) - ); - if (!authoritative && previous) { - nextProviders.push(previous); - } - } else if (!authoritative && previous !== undefined) { - // "unavailable" on the PROBE path for an instance recorded by an - // earlier authoritative listing: the 404 proves only that the - // upstream serves no /models route — not that the admin removed - // the gateway instance (only an authoritative listing can say - // that). Dropping it would leave manually configured - // coder:/ entries unresolvable until a listing - // succeeds again, so keep the metadata and carry prior entries - // forward like a transient error. - nextProviders.push(previous); - if (previousKnown) { - modelIds.push( - ...previousDiscovered.filter((id) => id.startsWith(`${provider.name}/`)) - ); - } else { - // No authoritative prior catalog: stale entries must not be - // promoted into a conclusive write. - catalogInconclusive = true; - modelIds.push( - ...staleDiscovered.filter((id) => id.startsWith(`${provider.name}/`)) - ); - } - } - // "unavailable" otherwise: the authoritative listing already names - // every existing instance (a listed instance whose /models 404s - // conclusively serves no catalog), and probe-default names carry no - // recorded metadata — the route is conclusively absent; drop it. - } + const prior = readPriorCoderCatalog(section); + // Per-provider rules (carry-forward, type gating, conclusiveness): + // see reconcileProviderCatalog. + const contributions = results.map(({ provider, result }) => + reconcileProviderCatalog(provider, result, prior, authoritative) + ); + const modelIds = contributions.flatMap((contribution) => contribution.ids); + const nextProviders = contributions.flatMap((contribution) => contribution.providers); + catalogInconclusive = contributions.some((contribution) => contribution.inconclusive); if (catalogInconclusive) { // The catalog cannot be written as authoritative. Keep/flip it to