diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index 2cf9105121..d63c58e94c 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -302,7 +302,7 @@ export const MODEL_ACCESS_DENIED_PATTERNS = [ // across every target, masking the real "fix your credential" error. When the // text clearly indicates a bad credential, the regex-based model-access detection // is suppressed (structured codes/types like model_not_found are unaffected). -const AUTH_CREDENTIAL_ERROR_PATTERNS = [ +export const AUTH_CREDENTIAL_ERROR_PATTERNS = [ /\b(?:invalid|incorrect|expired|missing|revoked)\s+api[\s_-]?key\b/i, /\bapi[\s_-]?key\s+(?:is\s+)?(?:invalid|incorrect|expired|missing|revoked|not\s+valid)\b/i, /\bauthentication\s+(?:failed|error|required)\b/i, @@ -311,6 +311,45 @@ const AUTH_CREDENTIAL_ERROR_PATTERNS = [ /\bnot\s+authenticated\b/i, ]; +// #10460: strict subset of MODEL_ACCESS_DENIED_PATTERNS that is unambiguously +// PROVIDER-wide — the model does not exist / is not served by this provider at all, so +// no account of that provider could serve it (e.g. "The requested model is not +// supported", "model not found"). Deliberately EXCLUDES the "access"/"permission" +// patterns from MODEL_ACCESS_DENIED_PATTERNS (e.g. "does not have permission to access +// this model", "access denied ... model"): those commonly indicate an ACCOUNT-scoped +// entitlement gap (e.g. PRO vs free tier) where a *different* account of the same +// provider may still have access, so they must keep rotating through the normal +// account-cooldown path — not be treated as provider-wide unsupported. +const PROVIDER_MODEL_UNSUPPORTED_PATTERNS = [ + /\binvalid model\b/i, + /\bmodel.*not.*(?:available|found|supported|accessible)\b/i, + /\bmodel.*(?:does not exist|doesn't exist)\b/i, + /\bmodel\b[\s\S]{0,80}?\b(?:does\s+not\s+support|doesn't\s+support|unsupported)\b/i, + /\b(?:does\s+not\s+support|doesn't\s+support|unsupported)\b[\s\S]{0,80}?\bmodel\b/i, + /\bunsupported\s+model\b/i, + /\bplease select a different model\b/i, +]; + +/** + * #10460: is this 400 an unambiguous, PROVIDER-wide "model not supported" response — + * i.e. would retrying a *different account* of the same provider also fail for the + * same reason? Reuses AUTH_CREDENTIAL_ERROR_PATTERNS (the same bad-credential + * exclusion `checkFallbackError`'s 400 branch applies) so a message like "invalid api + * key for model X" is never misclassified as model-wide. Also excludes the broader, + * ambiguous MODEL_ACCESS_DENIED_PATTERNS access/permission phrasing — those can be + * account-scoped entitlement gaps, not a provider-wide unsupported model — so account + * rotation for those keeps working normally via the regular cooldown path. + * + * Callers that want "should combo keep trying other targets" (not "should this + * specific account keep rotating") should use MODEL_ACCESS_DENIED_PATTERNS / + * isModelScoped400() instead — this helper is deliberately narrower. + */ +export function isProviderModelUnsupported400(status: number, errorText: string): boolean { + if (status !== HTTP_STATUS.BAD_REQUEST) return false; + if (AUTH_CREDENTIAL_ERROR_PATTERNS.some((p) => p.test(errorText))) return false; + return PROVIDER_MODEL_UNSUPPORTED_PATTERNS.some((p) => p.test(errorText)); +} + // Malformed request patterns — the model rejected the message format but a different // provider/model in the combo may accept it. const MALFORMED_REQUEST_PATTERNS = [ diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index 318d762e3c..c9fc007db5 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -43,6 +43,7 @@ import { hasPerModelQuota, getRuntimeProviderProfile, recordModelLockoutFailure, + isProviderModelUnsupported400, } from "@omniroute/open-sse/services/accountFallback.ts"; import { isLocalProvider } from "@omniroute/open-sse/config/providerRegistry.ts"; import { COOLDOWN_MS, RateLimitReason } from "@omniroute/open-sse/config/constants.ts"; @@ -2181,6 +2182,26 @@ export async function markAccountUnavailable( } } + // #10460: model-unsupported 400 — the PROVIDER does not serve this model, not + // this account. Cooling down the account and rotating to the next one wastes an + // upstream call because all accounts share the same model catalog. Return + // shouldFallback: false so the error propagates to the combo layer, which already + // has isModelScoped400() (combo.ts:1827) to advance to the next combo target. + // Uses isProviderModelUnsupported400() — the SAME disambiguation + // (AUTH_CREDENTIAL_ERROR_PATTERNS exclusion) checkFallbackError's 400 branch + // applies, narrowed further to exclude the broader/ambiguous + // MODEL_ACCESS_DENIED_PATTERNS access-/permission-phrased matches (e.g. "does not + // have permission to access this model"), which can be an ACCOUNT-scoped + // entitlement gap (PRO vs free tier) rather than a provider-wide unsupported + // model — those must keep rotating to other accounts normally. + if (isProviderModelUnsupported400(status, errorText)) { + log.info( + "AUTH", + `${connectionId.slice(0, 8)} provider_model_unsupported 400 (${provider}/${model ?? "n/a"}) — skipping account cooldown, letting combo advance` + ); + return { shouldFallback: false, cooldownMs: 0, reason: "provider_model_unsupported" }; + } + const effectiveProviderProfile = providerProfile || (provider ? await getRuntimeProviderProfile(provider) : null); // #4530 follow-up: the combo.ts lockout sites forward the admin-configured diff --git a/tests/unit/account-fallback-service.test.ts b/tests/unit/account-fallback-service.test.ts index 5bd6ca24a8..5bacfbe8fd 100644 --- a/tests/unit/account-fallback-service.test.ts +++ b/tests/unit/account-fallback-service.test.ts @@ -1,11 +1,29 @@ import test from "node:test"; import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// #10460: DATA_DIR must be assigned BEFORE any transitive DB import. The +// accountFallback.ts import below statically imports `@/lib/db/providers`, which +// imports `src/lib/db/core.ts`, whose `DATA_DIR` is a top-level +// `export const DATA_DIR = resolveWritableDataDir(...)` captured once at module-load +// time. Setting `process.env.DATA_DIR` after that first import is a no-op — the DB +// singleton keeps whatever DATA_DIR it resolved at import time, so an isolated test +// directory assigned later is silently never used and the #10460 tests below would +// actually read/write the shared default DATA_DIR instead. +const TEST_DATA_DIR_10460 = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-10460-")); +process.env.DATA_DIR = TEST_DATA_DIR_10460; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "10460-test-secret"; const accountFallback = await import("../../open-sse/services/accountFallback.ts"); const accountSelector = await import("../../open-sse/services/accountSelector.ts"); const { RateLimitReason, COOLDOWN_MS, PROVIDER_PROFILES } = await import("../../open-sse/config/constants.ts"); const { getCircuitBreaker } = await import("../../src/shared/utils/circuitBreaker.ts"); +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const auth = await import("../../src/sse/services/auth.ts"); const { isOAuthInvalidToken, @@ -1573,3 +1591,382 @@ test("isAccountDeactivated matches a custom signal after setCustomBannedSignals" setCustomBannedSignals([]); // cleanup — restore module state for other tests }); + +// ─── #10460: model-unsupported 400 skips account rotation ──────────────────── +// TEST_DATA_DIR_10460 / DATA_DIR / core / providersDb / auth are set up at the top +// of this file, BEFORE the accountFallback.ts import — see the comment there. + +async function resetStorage10460() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR_10460, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR_10460, { recursive: true }); +} + +async function seedConn10460(provider: string): Promise { + const conn = await providersDb.createProviderConnection({ + provider, + authType: "apikey", + apiKey: `${provider}-key-10460`, + isActive: true, + testStatus: "active", + }); + return (conn as Record).id as string; +} + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR_10460, { recursive: true, force: true }); +}); + +test("#10460: model-unsupported 400 returns shouldFallback:false (no account cooldown)", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + const result = await auth.markAccountUnavailable( + connId, + 400, + "The requested model is not supported", + "github", + "claude-fable-5" + ); + + // The guard must prevent account cooldown — the error belongs to the combo layer + assert.strictEqual(result.shouldFallback, false, "must not trigger account rotation"); + assert.strictEqual(result.cooldownMs, 0, "must not cool down the account"); + + // Verify the connection was NOT marked unavailable + const after = await providersDb.getProviderConnectionById(connId); + assert.ok(!after.rateLimitedUntil, "connection must not be rate-limited"); + assert.notStrictEqual(after.testStatus, "unavailable", "connection must stay active"); +}); + +test("#10460: model-unsupported 400 handles various phrasings", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + const phrasings = [ + "The requested model is not supported", + "model claude-fable-5 is not supported", + "invalid_request_error: model is not supported", + "unsupported model: gpt-9", + ]; + + for (const errorText of phrasings) { + await resetStorage10460(); + const id = await seedConn10460("github"); + const result = await auth.markAccountUnavailable(id, 400, errorText, "github", "test-model"); + assert.strictEqual(result.shouldFallback, false, `phrasing "${errorText}" must not rotate`); + // Verify connection stays healthy after each iteration + const conn = await providersDb.getProviderConnectionById(id); + assert.ok(!conn.rateLimitedUntil, `"${errorText}" must not rate-limit connection`); + assert.notStrictEqual(conn.testStatus, "unavailable", `"${errorText}" must not mark unavailable`); + } +}); + +test("#10460: body-specific 400 still goes through normal path (not blocked by model guard)", async () => { + await resetStorage10460(); + const connId = await seedConn10460("openai"); + + const result = await auth.markAccountUnavailable( + connId, + 400, + "Invalid message format: the request body is malformed", + "openai", + "gpt-4" + ); + + // Body-specific 400 does NOT match MODEL_ACCESS_DENIED_PATTERNS — normal path applies + assert.strictEqual(result.shouldFallback, true, "body-specific 400 must still allow fallback"); +}); + +test("#10460: non-400 status with model-unsupported text does NOT trigger guard", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + // 429 + model-unsupported text should go through normal path (guard only fires for status===400) + const result = await auth.markAccountUnavailable( + connId, + 429, + "The requested model is not supported", + "github", + "test-model" + ); + + assert.strictEqual(result.shouldFallback, true, "non-400 must not be short-circuited by model guard"); + // The key assertion: guard returns shouldFallback:false. If we get here with + // shouldFallback:true, the guard did NOT fire (correct behavior). +}); + +test("#10460: empty errorText with status 400 does NOT trigger guard", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + const result = await auth.markAccountUnavailable(connId, 400, "", "github", "test-model"); + + // Empty string matches no patterns — normal path applies + assert.strictEqual(result.shouldFallback, false, "empty errorText is generic 400 → no fallback"); +}); + +test("#10460: auth-credential 400 text does NOT match model-unsupported guard", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + // "Invalid API key provided for model gpt-4o" contains "model" but is NOT a + // model-unsupported error — it's a credential issue. The guard must not fire. + const result = await auth.markAccountUnavailable( + connId, + 400, + "Invalid API key provided for model gpt-4o", + "github", + "gpt-4o" + ); + + // This text does NOT match MODEL_ACCESS_DENIED_PATTERNS (verified by regex test) + // so it falls through to checkFallbackError which returns shouldFallback:false for generic 400 + assert.strictEqual(result.shouldFallback, false, "auth-credential 400 must not be caught by model guard"); + // The generic 400 path returns cooldownMs:0 — same as the guard, but the + // connection was NOT touched (no rateLimitedUntil set). This distinguishes + // it from the normal fallback path which would set a cooldown. + const conn = await providersDb.getProviderConnectionById(connId); + assert.ok(!conn.rateLimitedUntil, "generic 400 must not rate-limit connection"); +}); + +test("#10460: guard early return does not touch DB (distinguishes from normal path)", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + // Guard path: model-unsupported 400 → shouldFallback:false, cooldownMs:0, no DB change + const guardResult = await auth.markAccountUnavailable( + connId, 400, "The requested model is not supported", "github", "test-model" + ); + assert.strictEqual(guardResult.shouldFallback, false); + assert.strictEqual(guardResult.cooldownMs, 0); + const guardConn = await providersDb.getProviderConnectionById(connId); + assert.ok(!guardConn.rateLimitedUntil, "guard path must not touch DB"); + assert.strictEqual(guardConn.testStatus, "active", "guard path must keep connection active"); +}); + +test("#10460: returned result exposes a sanitized provider_model_unsupported reason", async () => { + await resetStorage10460(); + const connId = await seedConn10460("github"); + + const result = await auth.markAccountUnavailable( + connId, + 400, + "The requested model is not supported", + "github", + "claude-fable-5" + ); + + assert.strictEqual(result.shouldFallback, false); + assert.strictEqual( + (result as { reason?: string }).reason, + "provider_model_unsupported", + "the canonical sanitized reason must be exposed on the returned result, not only in logs" + ); +}); + +test("#10460: account-scoped permission/entitlement 400 keeps rotating (not misclassified as provider-wide unsupported)", async () => { + await resetStorage10460(); + const connIds: string[] = []; + for (let i = 0; i < 3; i++) { + connIds.push(await seedConn10460("github")); + } + let calls = 0; + + // Phrased so it matches the broader/ambiguous MODEL_ACCESS_DENIED_PATTERNS + // ("permission" ... "model") but is NOT an unambiguous provider-wide "model not + // supported" response — it reads as an account/key entitlement gap (e.g. this + // key's plan doesn't include this model), where a DIFFERENT account of the same + // provider may still have access. Rotation through all 3 accounts must continue, + // unlike the unambiguous "model is not supported" case above. + for (const connId of connIds) { + calls += 1; + const result = await auth.markAccountUnavailable( + connId, + 400, + "Your API key does not have permission to use model gpt-4o", + "github", + "gpt-4o" + ); + if (!result.shouldFallback) break; + } + + assert.equal( + calls, + 3, + "an account-scoped permission/entitlement 400 must keep rotating through all accounts, " + + "not be short-circuited by the provider-wide model-unsupported guard" + ); +}); + +test("#10460: account-scoped 401 keeps rotating through all 3 accounts (not short-circuited)", async () => { + await resetStorage10460(); + const connIds: string[] = []; + for (let i = 0; i < 3; i++) { + connIds.push(await seedConn10460("github")); + } + let calls = 0; + + for (const connId of connIds) { + calls += 1; + const result = await auth.markAccountUnavailable( + connId, + 401, + "Unauthorized: invalid credentials", + "github", + "claude-fable-5" + ); + if (!result.shouldFallback) break; + } + + assert.equal( + calls, + 3, + "401 account-scoped errors must keep rotating through every account, unlike model-unsupported 400" + ); +}); + +test("#10460: account-scoped 403 keeps rotating through all 3 accounts", async () => { + await resetStorage10460(); + const connIds: string[] = []; + for (let i = 0; i < 3; i++) { + connIds.push(await seedConn10460("github")); + } + let calls = 0; + + for (const connId of connIds) { + calls += 1; + const result = await auth.markAccountUnavailable( + connId, + 403, + "Forbidden: access denied for this account", + "github", + "claude-fable-5" + ); + if (!result.shouldFallback) break; + } + + assert.equal(calls, 3, "403 account-scoped errors must keep rotating through every account"); +}); + +test("#10460: 429 rate limit keeps rotating through all 3 accounts", async () => { + await resetStorage10460(); + const connIds: string[] = []; + for (let i = 0; i < 3; i++) { + connIds.push(await seedConn10460("github")); + } + let calls = 0; + + for (const connId of connIds) { + calls += 1; + const result = await auth.markAccountUnavailable( + connId, + 429, + "Rate limit exceeded", + "github", + "claude-fable-5" + ); + if (!result.shouldFallback) break; + } + + assert.equal(calls, 3, "429 rate-limit errors must keep rotating through every account"); +}); + +// ─── #10460 acceptance criteria: 3-account rotation + combo target advancement ─ +// +// Reproduces the exact regression from issue #10460: a combo with a +// (github/model, 3 accounts) target followed by a sibling target. When GitHub +// returns an unambiguous "model not supported" 400, the account-rotation loop +// must make exactly ONE upstream call (not one per account) and the combo must +// advance to the next target — not just that markAccountUnavailable() in +// isolation returns shouldFallback:false (covered above), but that a realistic +// rotation loop wired to the REAL markAccountUnavailable()/ +// isProviderModelUnsupported400() gating actually stops after account 1 and lets +// handleComboChat() move on. +// +// The inner loop below is a faithful, minimal reproduction of the account-rotation +// contract in src/sse/handlers/chat.ts::handleSingleModelChat step 8 ("Fallback to +// next account", ~line 1936): call markAccountUnavailable(); continue to the next +// connection only while shouldFallback is true, otherwise stop immediately and +// surface the failure. Reimplemented at this scope (rather than driving the full +// handleChat()/route stack) so the test can assert on upstream-call counts and +// per-connection DB state directly, while still exercising the real gating logic +// that decides whether rotation continues. +test("#10460 acceptance: unambiguous model-unsupported 400 makes exactly ONE upstream call across 3 accounts, then the combo advances to the next target", async () => { + await resetStorage10460(); + const { handleComboChat } = await import("../../open-sse/services/combo.ts"); + + const githubConnIds: string[] = []; + for (let i = 0; i < 3; i++) { + githubConnIds.push(await seedConn10460("github")); + } + + let githubUpstreamCalls = 0; + const triedGithubConnections: string[] = []; + const noopLog = { info: () => {}, warn: () => {}, debug: () => {}, error: () => {} }; + + const handleSingleModel = async (_body: unknown, modelStr: string) => { + if (modelStr.startsWith("github/")) { + for (const connId of githubConnIds) { + triedGithubConnections.push(connId); + githubUpstreamCalls += 1; + const result = await auth.markAccountUnavailable( + connId, + 400, + "The requested model is not supported", + "github", + "claude-fable-5" + ); + if (result.shouldFallback) continue; + return new Response( + JSON.stringify({ error: { message: "The requested model is not supported" } }), + { status: 400, headers: { "Content-Type": "application/json" } } + ); + } + // A test-fixture bug (guard not firing) would otherwise silently exhaust + // every account and mask the regression this test exists to catch. + throw new Error("all 3 github accounts were tried — the guard did not fire"); + } + // Second combo target (a different provider) — succeeds immediately. + return new Response(JSON.stringify({ id: "ok", choices: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }; + + const result = await handleComboChat({ + body: { model: "test", messages: [{ role: "user", content: "hi" }] }, + combo: { + name: "test-combo-10460", + strategy: "priority", + models: [{ model: "github/claude-fable-5" }, { model: "openai/gpt-4o-mini" }], + }, + handleSingleModel, + log: noopLog, + settings: {}, + allCombos: [], + }); + + assert.equal( + githubUpstreamCalls, + 1, + `expected exactly ONE upstream call for github/claude-fable-5 across 3 accounts, got ` + + `${githubUpstreamCalls} (tried: ${triedGithubConnections.join(", ")})` + ); + assert.equal(triedGithubConnections.length, 1, "only the first account should have been tried"); + assert.equal(result.status, 200, "the combo must advance to the next target and succeed"); + + // The two untouched accounts must remain completely unaffected — proves the + // guard did not just avoid an upstream call but also never cooled them down, + // so they stay immediately eligible for the next unrelated request. + for (const connId of githubConnIds.slice(1)) { + const conn = await providersDb.getProviderConnectionById(connId); + assert.ok(!conn.rateLimitedUntil, `untried account ${connId} must not be rate-limited`); + assert.notStrictEqual( + conn.testStatus, + "unavailable", + `untried account ${connId} must stay active` + ); + } +}); diff --git a/tests/unit/settings-debugmode-default.test.ts b/tests/unit/settings-debugmode-default.test.ts new file mode 100644 index 0000000000..bd667dc120 --- /dev/null +++ b/tests/unit/settings-debugmode-default.test.ts @@ -0,0 +1,26 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// Isolated DATA_DIR so persisted settings rows don't mask the default. +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-settings-debugmode-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "settings-debugmode-test-secret"; + +const { getSettings } = await import("../../src/lib/db/settings.ts"); + +test("debugMode defaults to false for fresh installs (no persisted setting)", async () => { + const settings = await getSettings(); + assert.equal(settings.debugMode, false, "debugMode should default to false, not true"); +}); + +test("logToolSources defaults to false", async () => { + const settings = await getSettings(); + assert.equal(settings.logToolSources, false, "logToolSources should default to false"); +}); + +test.after(() => { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +});