diff --git a/.github/workflows/opencode-plugin-ci.yml b/.github/workflows/opencode-plugin-ci.yml index 0e26c0e608..f1c99fe426 100644 --- a/.github/workflows/opencode-plugin-ci.yml +++ b/.github/workflows/opencode-plugin-ci.yml @@ -2,11 +2,11 @@ name: opencode-plugin CI on: push: - branches: [main, "release/**"] + branches: [main, release/v3.8.2] paths: - "@omniroute/opencode-plugin/**" pull_request: - branches: [main, "release/**"] + branches: [main, release/v3.8.2] paths: - "@omniroute/opencode-plugin/**" types: [opened, synchronize, reopened, ready_for_review] diff --git a/src/app/api/providers/[id]/test/route.ts b/src/app/api/providers/[id]/test/route.ts index 215771104c..fcffe0a34a 100644 --- a/src/app/api/providers/[id]/test/route.ts +++ b/src/app/api/providers/[id]/test/route.ts @@ -1053,8 +1053,23 @@ export async function testSingleConnection(connectionId: string, validationModel // Unsupported validation capability is neutral: the probe established that // this provider cannot be verified through the generic test surface, not - // that its credential is invalid. Do not mutate persisted credential health. + // that its credential is invalid. Do not mutate persisted credential health + // (testStatus/lastError/etc.) — but DO activate it if it isn't already: a + // connection that can never be health-checked would otherwise stay hidden + // from /v1/models forever under the "only advertise tested connections" + // default (isActive starts false on creation — see POST /api/providers), + // silently regressing every provider without a test surface. if (result.skipped === true) { + if (connection.isActive !== true) { + try { + await updateProviderConnection(connectionId, { isActive: true }); + } catch (activateError) { + console.log( + `[ConnectionTest] Failed to activate unverifiable connection ${connectionId}:`, + (activateError as any)?.message || activateError + ); + } + } return { ...result, latencyMs, @@ -1099,6 +1114,14 @@ export async function testSingleConnection(connectionId: string, validationModel const updateData: Record = { testStatus: clearErrorState ? "active" : result.valid ? connection.testStatus : "error", + // A passing test is the sole activation signal under the "only advertise + // tested-working connections" default — see POST /api/providers, which + // now creates connections isActive:false. Only ever flips ON here: a + // failing test intentionally leaves isActive untouched (a transient + // failure on an already-active, already-working connection must not take + // it out of rotation — that's what the cooldown/rateLimitedUntil below is + // for), so this never deactivates anything. + ...(result.valid ? { isActive: true } : {}), lastError: clearErrorState ? null : result.valid ? connection.lastError : result.error, lastErrorAt: clearErrorState ? null : result.valid ? connection.lastErrorAt : now, lastTested: now, diff --git a/src/app/api/providers/route.ts b/src/app/api/providers/route.ts index 9ed7f10667..08d0b9b64d 100644 --- a/src/app/api/providers/route.ts +++ b/src/app/api/providers/route.ts @@ -43,6 +43,7 @@ import { getModelSyncInternalBaseUrl, } from "@/shared/services/modelSyncScheduler"; import { finalizeValidatedChatGptWebCodexSecrets } from "@omniroute/open-sse/services/chatgptWebCodexAdmin.ts"; +import { testSingleConnection } from "./[id]/test/route"; // GET /api/providers - List all connections export async function GET(request: Request) { @@ -222,7 +223,15 @@ export async function POST(request: Request) { globalPriority: globalPriority || null, defaultModel: defaultModel || null, providerSpecificData, - isActive: true, + // Start inactive: a connection is only advertised via /v1/models (which + // filters on isActive) once a connection test has actually confirmed it + // works. The auto-test fired below flips this to true on success (or on + // an "unsupported" test, which cannot be verified either way and keeps + // the historical trust-it default) — see testSingleConnection in + // ./[id]/test/route.ts. A connection that fails its test, or is never + // tested because auto-test itself errors, simply stays hidden until the + // operator fixes the credential and re-tests it manually. + isActive: false, testStatus: testStatus || "unknown", }); @@ -272,6 +281,20 @@ export async function POST(request: Request) { ); } + // Auto-test the newly created connection so `testStatus` reflects reality + // shortly after creation instead of sitting at "unknown" until the + // operator manually clicks "Test" in the dashboard. Fire-and-forget for + // the same reason as the auto-sync above: the probe can take a few + // seconds (OAuth refresh, upstream round-trip) and must not block the + // 201 response. testSingleConnection() persists testStatus/lastError/etc. + // itself, so nothing further is needed here beyond logging failures. + void testSingleConnection(newConnection.id).catch((testError: unknown) => { + console.log( + `[providers] Auto-test failed for ${newConnection.id}:`, + (testError as { message?: string })?.message || testError + ); + }); + // Note: Gemini model sync is now triggered client-side with progress dialog // Hide sensitive fields diff --git a/tests/integration/provider-journey.contract.test.ts b/tests/integration/provider-journey.contract.test.ts index b346708551..8f88f273c6 100644 --- a/tests/integration/provider-journey.contract.test.ts +++ b/tests/integration/provider-journey.contract.test.ts @@ -171,7 +171,7 @@ test.describe("provider journey — in-process contract (#8330)", () => { const body = await readJsonObject(response); assert.equal(response.status, 201, `add connection failed: ${JSON.stringify(body)}`); - const connection = body.connection as { id?: string; provider?: string }; + const connection = body.connection as { id?: string; provider?: string; isActive?: unknown }; connectionId = connection.id ?? ""; assert.ok(connectionId, "connection must expose an id"); assert.equal(connection.provider, nodeId, "connection must bind to the created node"); @@ -187,6 +187,19 @@ test.describe("provider journey — in-process contract (#8330)", () => { connections.some((c) => c.id === connectionId && c.provider === nodeId), "the created connection must be visible via GET /api/providers" ); + + // #11446: a newly created connection now starts isActive:false until a passing + // connection test verifies it — POST /api/providers fires that test itself, but + // fire-and-forget and against the real network, which is exactly what makes it + // non-deterministic against this suite's stub host (see STEP 3's note on the + // same host). Simulate the operator's test having already passed, the same way + // STEP 3 bypasses the real /sync-models HTTP round-trip. + assert.equal( + connection.isActive, + false, + "a newly created connection must start inactive until verified (#11446)" + ); + await localDb.updateProviderConnection(connectionId, { isActive: true, testStatus: "active" }); }); test("STEP 3: sync models — discovered model is persisted for the connection", async () => { diff --git a/tests/unit/verified-connection-activation-11446.test.ts b/tests/unit/verified-connection-activation-11446.test.ts new file mode 100644 index 0000000000..6564a3094a --- /dev/null +++ b/tests/unit/verified-connection-activation-11446.test.ts @@ -0,0 +1,206 @@ +/** + * #11446 — a newly created connection was `isActive:true` immediately, with + * `testStatus` left at "unknown" until an operator manually clicked "Test" in the + * dashboard. Since `/v1/models` filters on `isActive` (not `testStatus`), an + * untested — or outright invalid — key's models were indistinguishable from a + * provider that actually works. + * + * This suite pins the behavior changes that close that gap: + * 1. POST /api/providers now creates the connection `isActive:false`. + * 2. testSingleConnection() is the sole activation signal — it flips a + * connection to `isActive:true` once a test actually PASSES, or is + * `skipped` as unverifiable/unsupported (trusted like before, since it + * can never be health-checked); a failing test intentionally leaves + * `isActive` untouched. + * + * All outbound provider validation traffic is stubbed (no real network) so the + * suite is deterministic and network-independent, following the same pattern as + * tests/unit/exclusive-lease-connection-test-isolation.test.ts. + */ + +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import { makeManagementSessionRequest } from "../helpers/managementSession.ts"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-connection-activation-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = "connection-activation-test-secret"; +process.env.DISABLE_SQLITE_AUTO_BACKUP = "true"; + +// Stub every outbound provider probe deterministically: no test in this suite may +// depend on real network reachability. Requests to the fake compatible-provider +// `/models` endpoint resolve per-test via `nextModelsProbeStatus`; anything else +// (e.g. the unrelated fire-and-forget model-sync self-fetch triggered by POST +// /api/providers) gets a harmless 404 instead of touching the network. +const VALIDATION_BASE_URL = "https://proxy.activation-11446.example.com/v1"; +let nextModelsProbeStatus: number | null = 200; +const originalFetch = globalThis.fetch; +globalThis.fetch = (async (input: string | URL | Request) => { + const url = + typeof input === "string" ? input : input instanceof Request ? input.url : input.toString(); + if (url === `${VALIDATION_BASE_URL}/models`) { + if (nextModelsProbeStatus === null) { + throw new Error("simulated network failure"); + } + return new Response(JSON.stringify({ data: [] }), { status: nextModelsProbeStatus }); + } + return new Response("not found", { status: 404 }); +}) as typeof fetch; + +const core = await import("../../src/lib/db/core.ts"); +const providerNodesRoute = await import("../../src/app/api/provider-nodes/route.ts"); +const providersRoute = await import("../../src/app/api/providers/route.ts"); +const { testSingleConnection } = await import("../../src/app/api/providers/[id]/test/route.ts"); + +async function readJsonObject(response: Response): Promise> { + const text = await response.text(); + try { + const parsed = JSON.parse(text) as unknown; + return parsed && typeof parsed === "object" ? (parsed as Record) : {}; + } catch { + return {}; + } +} + +async function createCompatibleNode(prefix: string): Promise { + const response = await providerNodesRoute.POST( + await makeManagementSessionRequest("http://localhost/api/provider-nodes", { + method: "POST", + body: { + type: "openai-compatible", + name: `Activation Test Node ${prefix}`, + prefix, + apiType: "chat", + baseUrl: VALIDATION_BASE_URL, + }, + }) + ); + const body = await readJsonObject(response); + assert.equal(response.status, 201, `create provider-node failed: ${JSON.stringify(body)}`); + const node = body.node as { id?: string }; + assert.ok(node.id, "provider node must expose an id"); + return node.id as string; +} + +async function createConnection( + nodeId: string, + name: string +): Promise<{ id: string; isActive: unknown }> { + const response = await providersRoute.POST( + await makeManagementSessionRequest("http://localhost/api/providers", { + method: "POST", + body: { provider: nodeId, apiKey: "sk-activation-test", name }, + }) + ); + const body = await readJsonObject(response); + assert.equal(response.status, 201, `create connection failed: ${JSON.stringify(body)}`); + const connection = body.connection as { id?: string; isActive?: unknown }; + assert.ok(connection.id, "connection must expose an id"); + return { id: connection.id as string, isActive: connection.isActive }; +} + +test.after(() => { + globalThis.fetch = originalFetch; + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("#11446: POST /api/providers creates a new connection inactive until verified", async () => { + const nodeId = await createCompatibleNode("activation-create"); + const { isActive } = await createConnection(nodeId, "Activation Create Connection"); + + assert.equal( + isActive, + false, + "a newly created connection must start isActive:false — /v1/models must not " + + "advertise an untested connection's models" + ); +}); + +test("#11446: testSingleConnection activates a connection once a test actually passes", async () => { + const nodeId = await createCompatibleNode("activation-pass"); + const { id: connectionId, isActive: createdActive } = await createConnection( + nodeId, + "Activation Pass Connection" + ); + assert.equal(createdActive, false, "precondition: connection must start inactive"); + + nextModelsProbeStatus = 200; // the probe will succeed + const result = await testSingleConnection(connectionId); + assert.equal(result.valid, true, `expected a passing test, got ${JSON.stringify(result)}`); + + const row = core + .getDbInstance() + .prepare("SELECT is_active FROM provider_connections WHERE id = ?") + .get(connectionId) as { is_active: number }; + assert.equal( + row.is_active, + 1, + "a passing test must activate a previously-inactive connection — see #11446" + ); +}); + +test("#11446: testSingleConnection never activates a connection whose test genuinely fails", async () => { + const nodeId = await createCompatibleNode("activation-fail"); + const { id: connectionId, isActive: createdActive } = await createConnection( + nodeId, + "Activation Fail Connection" + ); + assert.equal(createdActive, false, "precondition: connection must start inactive"); + + nextModelsProbeStatus = 401; // the probe will report an invalid credential + const result = await testSingleConnection(connectionId); + assert.equal(result.valid, false, `expected a failing test, got ${JSON.stringify(result)}`); + + const row = core + .getDbInstance() + .prepare("SELECT is_active FROM provider_connections WHERE id = ?") + .get(connectionId) as { is_active: number }; + assert.equal( + row.is_active, + 0, + "a failing test must leave an inactive connection inactive — it must never silently " + + "advertise an unverified/invalid connection" + ); +}); + +test("#11446: testSingleConnection activates a connection whose test is skipped as unsupported", async () => { + // A connection whose provider has no registry entry (and matches none of the + // compatible/specialty validators) makes validateProviderApiKey() return + // `unsupported: true` deterministically, with no network call — the same + // "this provider can never be health-checked through the generic test surface" + // signal the fix treats as neutral-but-trusted. It must still be activated, + // otherwise it would stay hidden from /v1/models forever under the "only + // advertise tested connections" default, since it can never produce a passing + // probe result. (Distinct from an exclusive-lease-busy skip, which is a + // temporary "try again later" and must NOT activate — see + // tests/unit/exclusive-lease-connection-test-isolation.test.ts.) + const db = core.getDbInstance(); + db.prepare( + `INSERT INTO provider_connections + (id, provider, auth_type, name, api_key, is_active, test_status, created_at, updated_at) + VALUES (?, ?, 'apikey', ?, ?, 0, 'unknown', ?, ?)` + ).run( + "activation-skip-connection", + "totally-unregistered-test-provider-11446", + "Activation Skip Connection", + "sk-activation-skip", + new Date().toISOString(), + new Date().toISOString() + ); + + const result = await testSingleConnection("activation-skip-connection"); + assert.equal(result.skipped, true, `expected a skipped test, got ${JSON.stringify(result)}`); + + const row = db + .prepare("SELECT is_active FROM provider_connections WHERE id = ?") + .get("activation-skip-connection") as { is_active: number }; + assert.equal( + row.is_active, + 1, + "a skipped/unsupported test must still activate a previously-inactive connection — see #11446" + ); +});