🤖 refactor: move per-provider Coder catalog merge rules into a tested module

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> = {}): 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 });
+    });
+  });
+});

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<string, unknown> | 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:<custom>/<model> 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 };
+}