diff --git a/packages/onboarding/src/plant-env-credentials.ts b/packages/onboarding/src/plant-env-credentials.ts index 8f20b544b..e63111097 100644 --- a/packages/onboarding/src/plant-env-credentials.ts +++ b/packages/onboarding/src/plant-env-credentials.ts @@ -21,7 +21,11 @@ // one bad or rate-limited key must never stop every other provider from // planting, and must never stop the hub itself from starting. -import { CredentialResponse, paginatedSchema } from "@intx/types"; +import { + CredentialResponse, + paginatedSchema, + ProviderResponse, +} from "@intx/types"; import { inferenceCredentialName, OLLAMA_PLACEHOLDER_SECRET, @@ -155,13 +159,64 @@ export type PlantEnvProviderCredentialsArgs = { seedCatalogFn?: (args: SeedCatalogArgs) => ReturnType; }; -async function findActiveCredential( +/** + * Looks up the provider row a curated provider's connections (env-plant + * and a Settings connect alike) both key their credential to — + * `persistConnectorCredential` and `seedCatalog`'s own `plantCredential` + * both `ensureProvider` this exact `{ name: provider }` pair, so a + * provider row's existence here means some path already connected this + * provider. Read-only: unlike `ensureProvider`, this never creates the + * row, so a provider nobody has connected yet correctly reads back as + * "no active credential" without planting a stub row ahead of a probe + * that might still fail. Paginated the same way `findActiveCredential` + * is — a tenant with enough providers to span a page must not lose a + * match that lands on page two. + */ +async function findProviderId( api: ApiCall, cookies: string[], tenantId: string, provider: SupportedCredentialProvider, -): Promise<{ id: string } | undefined> { - const name = inferenceCredentialName(provider); +): Promise { + let cursor: string | undefined; + do { + const path = + cursor === undefined + ? `/api/tenants/${tenantId}/providers?inherited=false` + : `/api/tenants/${tenantId}/providers?inherited=false&cursor=${encodeURIComponent(cursor)}`; + const listed = await api("GET", path, undefined, cookies); + const page = parseAs( + paginatedSchema(ProviderResponse), + listed.data, + "providers response", + ); + const match = page.data.find((p) => p.name === provider); + if (match !== undefined) return match.id; + cursor = page.nextCursor ?? undefined; + } while (cursor !== undefined); + return undefined; +} + +/** + * An active credential is recognized by the provider row it belongs to + * (`providerId`), not by the credential's own name — a Settings-connected + * credential is named after the connector's `displayName` ("Anthropic"), + * while this module's own plant names its row + * `inferenceCredentialName(provider)` ("anthropic-default"). Both + * resolve to the same provider row (`findProviderId`), so matching on + * `providerId` recognizes either one instead of only the env-plant's own + * naming convention. Restricted to the credential types `seedCatalog` + * itself ever writes for an inference source (`api_key`, `oauth_token`) + * so a non-inference row that happens to share the provider (never + * planted by either path today, but not a case this match should ever + * be fooled by) can't count as the plant. + */ +async function findActiveCredential( + api: ApiCall, + cookies: string[], + tenantId: string, + providerId: string, +): Promise<{ id: string; name: string } | undefined> { let cursor: string | undefined; do { const path = @@ -175,9 +230,12 @@ async function findActiveCredential( "credentials response", ); const match = page.data.find( - (c) => c.name === name && c.status === "active", + (c) => + c.providerId === providerId && + c.status === "active" && + (c.type === "api_key" || c.type === "oauth_token"), ); - if (match !== undefined) return { id: match.id }; + if (match !== undefined) return { id: match.id, name: match.name }; cursor = page.nextCursor ?? undefined; } while (cursor !== undefined); return undefined; @@ -245,14 +303,23 @@ export async function plantEnvProviderCredentials( SupportedCredentialProvider, string, ][]) { - const alreadyActive = await findActiveCredential( + const providerId = await findProviderId( args.api, args.cookies, args.tenantId, provider, ); + const alreadyActive = + providerId !== undefined + ? await findActiveCredential( + args.api, + args.cookies, + args.tenantId, + providerId, + ) + : undefined; if (alreadyActive) { - const name = inferenceCredentialName(provider); + const name = alreadyActive.name; try { await runSeedCatalog( catalogSeedArgs(provider, { @@ -302,13 +369,24 @@ export async function plantEnvProviderCredentials( // same name silently blocks the proven env key from ever being // stored. `findActiveCredential` already ruled out an *active* // credential before the probe; re-checking now is the only way to - // tell "planted" apart from "409-skipped against a dead row". - const nowActive = await findActiveCredential( + // tell "planted" apart from "409-skipped against a dead row". The + // provider row is guaranteed to exist by now — `seedCatalog` just + // `ensureProvider`d it while planting. + const nowProviderId = await findProviderId( args.api, args.cookies, args.tenantId, provider, ); + const nowActive = + nowProviderId !== undefined + ? await findActiveCredential( + args.api, + args.cookies, + args.tenantId, + nowProviderId, + ) + : undefined; if (!nowActive) { const name = inferenceCredentialName(provider); args.log( diff --git a/packages/onboarding/test/plant-env-credentials.test.ts b/packages/onboarding/test/plant-env-credentials.test.ts index 99a4f41fc..0240c86b4 100644 --- a/packages/onboarding/test/plant-env-credentials.test.ts +++ b/packages/onboarding/test/plant-env-credentials.test.ts @@ -13,43 +13,116 @@ function collector() { return { lines, log: (line: string) => lines.push(line) }; } -/** A minimal credentials-list ApiCall: answers - * `GET /api/tenants/:id/credentials` with whatever `active` set holds, - * and 404s everything else — `plantEnvProviderCredentials` never calls - * anything else through `api` itself (the plant/probe indirections are - * always passed as fakes in these tests). */ +/** The curated provider key a fake credential row's name belongs to — + * every fixture in this suite either uses the env-plant's own + * `-default` naming convention or (for the + * Settings-connected-credential tests) supplies its own explicit + * `providerKey`, so a bare fixture name is always `-default`. */ +function providerKeyOf(name: string): string { + return name.replace(/-default$/, ""); +} + +type CredentialFixture = { + readonly name: string; + readonly providerKey?: string; + /** Defaults to "api_key" — the type every fixture in this suite has + * used until the non-inference-type test, which sets this to a type + * `findActiveCredential` must not recognize as the plant. */ + readonly type?: "api_key" | "oauth_token" | "certificate" | "other"; +}; + +function fixture(name: string): CredentialFixture { + return { name }; +} + +function providerRow(providerKey: string) { + return { + id: `prov_${providerKey}`, + tenantId: TENANT_ID, + name: providerKey, + plugin: "http", + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + }; +} + +function credentialRow( + fixture: CredentialFixture, + status: "active" | "revoked", + idPrefix: string, +) { + const providerKey = fixture.providerKey ?? providerKeyOf(fixture.name); + return { + id: `${idPrefix}${fixture.name}`, + tenantId: TENANT_ID, + providerId: `prov_${providerKey}`, + name: fixture.name, + type: fixture.type ?? "api_key", + status, + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + }; +} + +/** A minimal credentials-and-providers-list ApiCall: answers + * `GET /api/tenants/:id/credentials` with whatever `active`/`revoked` + * fixtures hold, `GET /api/tenants/:id/providers` with the provider row + * each fixture's `providerKey` belongs to, and 404s everything else — + * `plantEnvProviderCredentials` never calls anything else through `api` + * itself (the plant/probe indirections are always passed as fakes in + * these tests). */ function credentialsApi( - activeNames: Set, - revokedNames: Set = new Set(), + active: + ReadonlySet | readonly CredentialFixture[], + revoked: + ReadonlySet | readonly CredentialFixture[] = [], ): ApiCall { + // Read `active`/`revoked` fresh on every call rather than snapshotting + // once: several tests mutate the `Set` they passed in (mirroring a + // real `seedCatalog`'s side effect) after the fake is built, and rely + // on the next call seeing that mutation. + function currentFixtures( + entries: + ReadonlySet | readonly CredentialFixture[], + ): CredentialFixture[] { + return [...entries].map((entry) => + typeof entry === "string" ? fixture(entry) : entry, + ); + } return async (method, path) => { if ( method === "GET" && path.startsWith(`/api/tenants/${TENANT_ID}/credentials`) ) { - const active = [...activeNames].map((name) => ({ - id: `cred_${name}`, - tenantId: TENANT_ID, - providerId: `prov_${name}`, - name, - type: "api_key", - status: "active", - createdAt: "2026-01-01T00:00:00.000Z", - updatedAt: "2026-01-01T00:00:00.000Z", - })); - const revoked = [...revokedNames].map((name) => ({ - id: `cred_revoked_${name}`, - tenantId: TENANT_ID, - providerId: `prov_${name}`, - name, - type: "api_key", - status: "revoked", - createdAt: "2026-01-01T00:00:00.000Z", - updatedAt: "2026-01-01T00:00:00.000Z", - })); + const rows = [ + ...currentFixtures(active).map((f) => + credentialRow(f, "active", "cred_"), + ), + ...currentFixtures(revoked).map((f) => + credentialRow(f, "revoked", "cred_revoked_"), + ), + ]; + return { + status: 200, + data: { data: rows, nextCursor: null }, + cookies: [], + }; + } + if ( + method === "GET" && + path.startsWith(`/api/tenants/${TENANT_ID}/providers`) + ) { + const providerKeys = new Set( + [...currentFixtures(active), ...currentFixtures(revoked)].map( + (f) => f.providerKey ?? providerKeyOf(f.name), + ), + ); return { status: 200, - data: { data: [...active, ...revoked], nextCursor: null }, + data: { + data: [...providerKeys].map((key) => providerRow(key)), + nextCursor: null, + }, cookies: [], }; } @@ -61,7 +134,21 @@ function credentialsApi( * the target row lives on page two, so a caller that only reads page * one would never see it. */ function paginatedCredentialsApi(pages: string[][]): ApiCall { + const providerKeys = new Set(pages.flat().map(providerKeyOf)); return async (method, path) => { + if ( + method === "GET" && + path.startsWith(`/api/tenants/${TENANT_ID}/providers`) + ) { + return { + status: 200, + data: { + data: [...providerKeys].map((key) => providerRow(key)), + nextCursor: null, + }, + cookies: [], + }; + } if (!( method === "GET" && path.startsWith(`/api/tenants/${TENANT_ID}/credentials`) @@ -77,16 +164,9 @@ function paginatedCredentialsApi(pages: string[][]): ApiCall { return { status: 200, data: { - data: names.map((name) => ({ - id: `cred_${name}`, - tenantId: TENANT_ID, - providerId: `prov_${name}`, - name, - type: "api_key", - status: "active", - createdAt: "2026-01-01T00:00:00.000Z", - updatedAt: "2026-01-01T00:00:00.000Z", - })), + data: names.map((name) => + credentialRow(fixture(name), "active", "cred_"), + ), nextCursor, }, cookies: [], @@ -94,6 +174,53 @@ function paginatedCredentialsApi(pages: string[][]): ApiCall { }; } +/** A paginated providers-list ApiCall for the findProviderId + * follow-nextCursor test: the target provider row lives on page two, + * paired with a single active credential already sitting on that + * provider so the test can assert the plant is recognized. */ +function paginatedProvidersApi( + pages: string[][], + activeCredentialName: string, +): ApiCall { + return async (method, path) => { + if ( + method === "GET" && + path.startsWith(`/api/tenants/${TENANT_ID}/providers`) + ) { + const url = new URL(path, "http://hub.test"); + const cursor = url.searchParams.get("cursor"); + const pageIndex = cursor === null ? 0 : Number(cursor); + const names = pages[pageIndex] ?? []; + const nextCursor = + pageIndex + 1 < pages.length ? String(pageIndex + 1) : null; + return { + status: 200, + data: { + data: names.map((key) => providerRow(key)), + nextCursor, + }, + cookies: [], + }; + } + if ( + method === "GET" && + path.startsWith(`/api/tenants/${TENANT_ID}/credentials`) + ) { + return { + status: 200, + data: { + data: [ + credentialRow(fixture(activeCredentialName), "active", "cred_"), + ], + nextCursor: null, + }, + cookies: [], + }; + } + throw new Error(`unexpected call: ${method} ${path}`); + }; +} + describe("envProviderKeysFrom", () => { test("reads every curated provider's conventional env var", () => { const keys = envProviderKeysFrom({ @@ -243,6 +370,40 @@ describe("plantEnvProviderCredentials", () => { expect(lines[0]).not.toContain("sk-ant-rotated"); }); + test("a Settings-connected credential under the same provider is recognized, so booting with the env key skips the probe and never creates a second credential row", async () => { + const { log, lines } = collector(); + let probed = false; + const seedCatalogCalls: SeedCatalogArgs[] = []; + const outcomes = await plantEnvProviderCredentials({ + // Named "Anthropic" (the connector's displayName, the way + // `persistConnectorCredential` names a Settings-connected + // credential) rather than this module's own "anthropic-default" — + // both belong to the same `prov_anthropic` provider row. + api: credentialsApi([{ name: "Anthropic", providerKey: "anthropic" }]), + cookies: [], + tenantId: TENANT_ID, + envProviderKeys: { anthropic: "sk-ant-from-env" }, + log, + testCredential: async () => { + probed = true; + return { ok: true }; + }, + seedCatalogFn: async (args) => { + seedCatalogCalls.push(args); + return { hasCompletionCapableModel: true }; + }, + }); + + expect(outcomes).toEqual([{ provider: "anthropic", status: "skipped" }]); + expect(probed).toBe(false); + expect(seedCatalogCalls).toHaveLength(1); + expect(seedCatalogCalls[0]?.existingCredentialId).toBe("cred_Anthropic"); + expect(seedCatalogCalls[0]?.apiKey).toBeUndefined(); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain("Anthropic"); + expect(lines[0]).not.toContain("sk-ant-from-env"); + }); + test("a catalog backfill failure on an already-active credential is reported, never thrown", async () => { const { log, lines } = collector(); const outcomes = await plantEnvProviderCredentials({ @@ -510,4 +671,73 @@ describe("plantEnvProviderCredentials", () => { "cred_anthropic-default", ); }); + + test("findProviderId follows nextCursor instead of reading only page one", async () => { + const { log } = collector(); + let probed = false; + const seedCatalogCalls: SeedCatalogArgs[] = []; + const outcomes = await plantEnvProviderCredentials({ + // The "anthropic" provider row lives on page two only. + api: paginatedProvidersApi( + [["other-provider"], ["anthropic"]], + "anthropic-default", + ), + cookies: [], + tenantId: TENANT_ID, + envProviderKeys: { anthropic: "sk-ant-real" }, + log, + testCredential: async () => { + probed = true; + return { ok: true as const }; + }, + seedCatalogFn: async (args) => { + seedCatalogCalls.push(args); + return { hasCompletionCapableModel: true }; + }, + }); + + expect(outcomes).toEqual([{ provider: "anthropic", status: "skipped" }]); + expect(probed).toBe(false); + expect(seedCatalogCalls[0]?.existingCredentialId).toBe( + "cred_anthropic-default", + ); + }); + + test("a non-inference credential type on the same provider is not recognized as the plant", async () => { + const { log } = collector(); + let probed = false; + const seedCatalogCalls: SeedCatalogArgs[] = []; + // An active "certificate"-typed row already sits on the anthropic + // provider — neither write path plants one of these today, but the + // match must not be fooled by it into skipping the probe. + const active = new Set([ + { + name: "anthropic-legacy-cert", + providerKey: "anthropic", + type: "certificate", + }, + ]); + const outcomes = await plantEnvProviderCredentials({ + api: credentialsApi(active), + cookies: [], + tenantId: TENANT_ID, + envProviderKeys: { anthropic: "sk-ant-real" }, + log, + testCredential: async () => { + probed = true; + return { ok: true as const }; + }, + seedCatalogFn: async (args) => { + seedCatalogCalls.push(args); + // Mirrors the real `seedCatalog`'s side effect: a fresh plant + // actually stores an active `api_key` credential under this name. + active.add({ name: "anthropic-default", providerKey: "anthropic" }); + return { hasCompletionCapableModel: true }; + }, + }); + + expect(probed).toBe(true); + expect(outcomes).toEqual([{ provider: "anthropic", status: "planted" }]); + expect(seedCatalogCalls[0]?.apiKey).toBe("sk-ant-real"); + }); });