diff --git a/open-sse/services/model.ts b/open-sse/services/model.ts index c9077bf1db..59ba39d253 100644 --- a/open-sse/services/model.ts +++ b/open-sse/services/model.ts @@ -120,7 +120,44 @@ for (const [aliasOrId, models] of Object.entries(PROVIDER_MODELS)) { } } const KNOWN_MODEL_IDS = new Set(MODEL_TO_PROVIDERS.keys()); -export const CODEX_NATIVE_UNPREFIXED_MODELS = new Set(["codex-auto-review"]); +// Bare Codex CLI defaults must always route to the `codex` provider (chatgpt.com +// OAuth) even when other providers that also catalog the model id (e.g. +// `agentrouter`, `openai`) are active. The Codex cookie quota on the user's +// account is the source of truth for capacity, and bare-id requests from +// `codex` (CLI)/`Codex` (web) would otherwise silently fan out to whichever +// provider won the inference race — leaving the user wondering why the +// canonical ChatGPT subscription stopped working. Override per-request by +// prefixing the model id (e.g. `agentrouter/gpt-5.6-sol`, +// `openai/gpt-5.6-sol`) — the prefix path always wins. +export const CODEX_NATIVE_UNPREFIXED_MODELS = new Set([ + "codex-auto-review", + "gpt-5.6-sol", + "gpt-5.6-sol-ultra", + "gpt-5.6-sol-max", + "gpt-5.6-sol-xhigh", + "gpt-5.6-sol-high", + "gpt-5.6-sol-medium", + "gpt-5.6-sol-low", + "gpt-5.6-terra", + "gpt-5.6-terra-ultra", + "gpt-5.6-terra-max", + "gpt-5.6-terra-xhigh", + "gpt-5.6-terra-high", + "gpt-5.6-terra-medium", + "gpt-5.6-terra-low", + "gpt-5.6-luna", + "gpt-5.6-luna-max", + "gpt-5.6-luna-xhigh", + "gpt-5.6-luna-high", + "gpt-5.6-luna-medium", + "gpt-5.6-luna-low", + "gpt-5.5", + "gpt-5.5-xhigh", + "gpt-5.5-high", + "gpt-5.5-medium", + "gpt-5.5-low", + "gpt-5.3-codex-spark", +]); interface ProviderConnectionLike { provider?: unknown; @@ -534,7 +571,19 @@ async function resolveModelByProviderInference(modelId: string, extendedContext: getActiveSyncedProvidersForModel(modelId), getPreferClaudeCodeForUnprefixedClaudeModels(), ]); - const providers = getInferredProvidersForModel(modelId, activeSyncedProviders); + // #FIX: synced catalogs (populated from `/v1/models` per connection) can + // claim ownership of models the provider does not actually serve (e.g. a + // `kiro` upstream briefly advertising `claude-opus-5` before it was + // vendored into the registry). Without this filter the bare-routing path + // would forward traffic to providers that 404 on the upstream call. + // Auto-discovery still wins when no static registry entry exists for the + // model id — only entries that conflict with the static catalog are dropped. + const staticCatalogProviders = MODEL_TO_PROVIDERS.get(modelId) || []; + const validatedSyncedProviders = + staticCatalogProviders.length > 0 + ? activeSyncedProviders.filter((p) => staticCatalogProviders.includes(p)) + : activeSyncedProviders; + const providers = getInferredProvidersForModel(modelId, validatedSyncedProviders); const nonOpenAIProviders = providers.filter((p) => p !== "openai"); // Bare model IDs from Codex CLI do not preserve OmniRoute's `cx/` prefix. diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index da10e9a550..2d94cd141e 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -1326,7 +1326,8 @@ async function handleSingleModelChat( provider, model, lastError, - lastStatus + lastStatus, + resolved.candidateAliases ); const lastFailedConnectionId = excludedConnectionIds.size > 0 diff --git a/src/sse/handlers/chatHelpers.ts b/src/sse/handlers/chatHelpers.ts index acb2b6d279..7faf961975 100644 --- a/src/sse/handlers/chatHelpers.ts +++ b/src/sse/handlers/chatHelpers.ts @@ -569,7 +569,8 @@ export function handleNoCredentials( provider: string, model: string, lastError: string | null, - lastStatus: number | null + lastStatus: number | null, + candidateAliases?: readonly string[] ) { if (credentials?.allRateLimited) { const errorMsg = lastError || credentials.lastError || "Unavailable"; @@ -642,7 +643,22 @@ export function handleNoCredentials( // all disabled. log level is `warn` rather than `error` because zero active // credentials is an expected operator-driven state, not a server fault. log.warn("AUTH", `No active credentials for provider: ${provider}`); - return errorResponse(HTTP_STATUS.NOT_FOUND, `No active credentials for provider: ${provider}`); + // #FIX: surface the candidate aliases (from resolveModelOrError) so the + // operator can pick a working provider/model prefix instead of guessing. + // Without this, "No active credentials for provider: kiro" leaves the + // user staring at a wall — most bugs in this area are actually "wrong + // provider was picked", not "the provider is broken". + const hint = + Array.isArray(candidateAliases) && candidateAliases.length > 0 + ? ` Try one of: ${candidateAliases + .slice(0, 3) + .map((a) => `${a}/${model}`) + .join(", ")}.` + : ""; + return errorResponse( + HTTP_STATUS.NOT_FOUND, + `No active credentials for provider: ${provider}.${hint}` + ); } log.warn("CHAT", "No more accounts available", { provider }); return errorResponse( diff --git a/tests/unit/compression/harness.test.ts b/tests/unit/compression/harness.test.ts index ac7efc8d84..387f8a8edc 100644 --- a/tests/unit/compression/harness.test.ts +++ b/tests/unit/compression/harness.test.ts @@ -77,7 +77,7 @@ describe("compression harness — eval runner (C1)", () => { }); describe("compression harness — tokens-per-task gate (N4)", () => { - const longInput = "lorem ipsum dolor sit amet ".repeat(40); + const longInput = "example content for testing purposes ".repeat(40); it("passes when cost/task matches the baseline", async () => { const corpus = [{ id: "a", input: longInput, task: "chat" }]; diff --git a/tests/unit/fix-bare-model-precedence.test.ts b/tests/unit/fix-bare-model-precedence.test.ts new file mode 100644 index 0000000000..6a963ddf97 --- /dev/null +++ b/tests/unit/fix-bare-model-precedence.test.ts @@ -0,0 +1,78 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + CODEX_NATIVE_UNPREFIXED_MODELS, + getModelInfoCore, +} from "../../open-sse/services/model.ts"; + +// #FIX: bare Codex-default model ids must always route to the `codex` +// provider (chatgpt.com OAuth) when no provider prefix is supplied, even +// when other providers that also catalog the id (e.g. `agentrouter`, +// `openai`) are active. The Codex cookie quota is the source of truth — +// auto-fanning to other providers silently breaks the "default" experience. + +test("CODEX_NATIVE_UNPREFIXED_MODELS includes gpt-5.6-sol tier set", () => { + for (const id of [ + "gpt-5.6-sol", + "gpt-5.6-sol-max", + "gpt-5.6-sol-xhigh", + "gpt-5.6-sol-high", + "gpt-5.6-sol-medium", + "gpt-5.6-sol-low", + "gpt-5.6-terra", + "gpt-5.6-terra-xhigh", + "gpt-5.6-luna", + "gpt-5.6-luna-xhigh", + "gpt-5.5", + "gpt-5.5-xhigh", + "gpt-5.5-medium", + "gpt-5.5-low", + "gpt-5.3-codex-spark", + "codex-auto-review", + ]) { + assert.equal( + CODEX_NATIVE_UNPREFIXED_MODELS.has(id), + true, + `expected CODEX_NATIVE_UNPREFIXED_MODELS to include ${id}` + ); + } +}); + +test("bare gpt-5.6-sol resolves to codex (provider native prefix wins)", async () => { + const info = await getModelInfoCore("gpt-5.6-sol", null); + assert.equal(info.provider, "codex", "bare gpt-5.6-sol must route to codex"); + assert.equal(info.model, "gpt-5.6-sol"); +}); + +test("bare gpt-5.5 resolves to codex", async () => { + const info = await getModelInfoCore("gpt-5.5", null); + assert.equal(info.provider, "codex"); + assert.equal(info.model, "gpt-5.5"); +}); + +test("bare gpt-5.6-sol-max resolves to codex", async () => { + const info = await getModelInfoCore("gpt-5.6-sol-max", null); + assert.equal(info.provider, "codex"); + assert.equal(info.model, "gpt-5.6-sol-max"); +}); + +test("agentrouter/gpt-5.6-sol (explicit prefix) routes to agentrouter", async () => { + const info = await getModelInfoCore("agentrouter/gpt-5.6-sol", null); + assert.equal(info.provider, "agentrouter"); + assert.equal(info.model, "gpt-5.6-sol"); +}); + +test("openai/gpt-5.6-sol (explicit prefix) routes to openai", async () => { + const info = await getModelInfoCore("openai/gpt-5.6-sol", null); + assert.equal(info.provider, "openai"); + assert.equal(info.model, "gpt-5.6-sol"); +}); + +test("codex-auto-review remains in the precedence set (regression guard)", async () => { + // Pre-fix regression: removing/replacing the set would silently break the + // `/review` codepath that ships with the Codex CLI. + assert.equal(CODEX_NATIVE_UNPREFIXED_MODELS.has("codex-auto-review"), true); + const info = await getModelInfoCore("codex-auto-review", null); + assert.equal(info.provider, "codex"); +}); \ No newline at end of file diff --git a/tests/unit/fix-bare-routing-fallback.test.ts b/tests/unit/fix-bare-routing-fallback.test.ts new file mode 100644 index 0000000000..865ac2feb1 --- /dev/null +++ b/tests/unit/fix-bare-routing-fallback.test.ts @@ -0,0 +1,62 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { getModelInfoCore } from "../../open-sse/services/model.ts"; + +// #FIX: end-to-end precedence checks for bare model routing. These guard +// the contract that: +// - Bare Codex-default model ids (gpt-5.6-sol, gpt-5.5, etc.) ALWAYS route +// to `codex`, regardless of which other providers are also active. +// - Bare model ids shared between providers (e.g. claude-opus-5 across +// anthropic/claude/github/agentrouter/etc.) never silently route to a +// provider whose static registry does NOT actually catalog them (the +// kiro-synced-catalog bug). +// - Explicit `provider/model` prefixes always win over the bare inference. + +test("bare gpt-5.6-sol routes to codex (precedence via CODEX_NATIVE_UNPREFIXED_MODELS)", async () => { + const info = await getModelInfoCore("gpt-5.6-sol", null); + assert.equal( + info.provider, + "codex", + "bare gpt-5.6-sol must route to codex — the Codex CLI default" + ); +}); + +test("bare gpt-5.5 routes to codex", async () => { + const info = await getModelInfoCore("gpt-5.5", null); + assert.equal(info.provider, "codex"); +}); + +test("bare gpt-5.6-sol-xhigh (a tier id) routes to codex", async () => { + const info = await getModelInfoCore("gpt-5.6-sol-xhigh", null); + assert.equal(info.provider, "codex"); +}); + +test("explicit prefix overrides bare precedence (agentrouter/gpt-5.6-sol)", async () => { + const info = await getModelInfoCore("agentrouter/gpt-5.6-sol", null); + assert.equal(info.provider, "agentrouter"); +}); + +test("explicit prefix overrides bare precedence (openai/gpt-5.6-sol)", async () => { + const info = await getModelInfoCore("openai/gpt-5.6-sol", null); + assert.equal(info.provider, "openai"); +}); + +test("bare claude-opus-5 never resolves to kiro (synced-catalog validation)", async () => { + // The bug: a kiro connection had claude-opus-5 in its synced /v1/models + // cache (likely from a brief upstream quirk). The bare-routing path + // accepted it as a candidate and routed traffic there, which then 404'd + // because kiro's static registry never cataloged claude-opus-5. + // The fix: validated synced candidates against the static registry. + const info = await getModelInfoCore("claude-opus-5", null); + assert.notEqual( + info.provider, + "kiro", + `kiro must NOT win bare claude-opus-5 routing — it does not catalog the model` + ); +}); + +test("bare claude-opus-4-8 also never resolves to kiro (same fix must apply to all shared models)", async () => { + const info = await getModelInfoCore("claude-opus-4-8", null); + assert.notEqual(info.provider, "kiro"); +}); \ No newline at end of file diff --git a/tests/unit/fix-error-message-candidates.test.ts b/tests/unit/fix-error-message-candidates.test.ts new file mode 100644 index 0000000000..d141da40b0 --- /dev/null +++ b/tests/unit/fix-error-message-candidates.test.ts @@ -0,0 +1,78 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { handleNoCredentials } from "../../src/sse/handlers/chatHelpers.ts"; + +// #FIX: the "No active credentials for provider: X" 404 response used to be +// a wall of silence — operators had no way to know which providers actually +// catalog the model id they requested. Surface a hint listing the top 3 +// candidate aliases (provider/model prefix form) so the operator can +// prefix and route to a working provider on the next request. + +test("handleNoCredentials includes candidate aliases hint when supplied", async () => { + const res = handleNoCredentials( + /* credentials */ {}, + /* excludeConnectionId */ null, + /* provider */ "kiro", + /* model */ "claude-opus-5", + /* lastError */ null, + /* lastStatus */ null, + /* candidateAliases */ ["anthropic", "claude", "agentrouter"] + ); + + assert.equal(res.status, 404); + const body = (await res.json()) as { error?: { message?: string } }; + const message = body?.error?.message ?? ""; + assert.match( + message, + /No active credentials for provider: kiro/, + "must keep the original error prefix" + ); + assert.match( + message, + /Try one of: anthropic\/claude-opus-5, claude\/claude-opus-5, agentrouter\/claude-opus-5/, + "must append a candidate-prefix hint when candidates are provided" + ); +}); + +test("handleNoCredentials omits hint when no candidates supplied", async () => { + const res = handleNoCredentials( + {}, + null, + "kiro", + "claude-opus-5", + null, + null + /* no candidateAliases */ + ); + + assert.equal(res.status, 404); + const body = (await res.json()) as { error?: { message?: string } }; + const message = body?.error?.message ?? ""; + assert.match(message, /No active credentials for provider: kiro/); + assert.doesNotMatch( + message, + /Try one of:/, + "must NOT append a hint when no candidates are provided" + ); +}); + +test("handleNoCredentials trims candidate list to top 3", async () => { + const res = handleNoCredentials( + {}, + null, + "kiro", + "claude-opus-5", + null, + null, + ["anthropic", "claude", "agentrouter", "github", "vertex-partner"] + ); + + const body = (await res.json()) as { error?: { message?: string } }; + const message = body?.error?.message ?? ""; + // Top-3 (anthropic, claude, agentrouter) — github and vertex-partner are + // dropped to keep the hint actionable. + assert.match(message, /Try one of: anthropic\/claude-opus-5, claude\/claude-opus-5, agentrouter\/claude-opus-5/); + assert.doesNotMatch(message, /github\/claude-opus-5/); + assert.doesNotMatch(message, /vertex-partner\/claude-opus-5/); +}); \ No newline at end of file diff --git a/tests/unit/fix-synced-model-validation.test.ts b/tests/unit/fix-synced-model-validation.test.ts new file mode 100644 index 0000000000..9106e15b78 --- /dev/null +++ b/tests/unit/fix-synced-model-validation.test.ts @@ -0,0 +1,85 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { getModelInfoCore } from "../../open-sse/services/model.ts"; + +// #FIX: synced catalogs (populated from `/v1/models` per connection) can +// claim ownership of models the provider does not actually serve. Without +// validating against the static registry, a `kiro` upstream briefly +// advertising `claude-opus-5` (or any other provider mistakenly exposing a +// model it can't dispatch) routes bare traffic to providers that 404 on +// the upstream call. Auto-discovery still wins when no static registry +// entry exists for the model id — only entries that conflict with the +// static catalog are dropped. + +test("bare claude-opus-5 still resolves to a static-registry provider (does not silently route to kiro)", async () => { + const info = await getModelInfoCore("claude-opus-5", null); + + // The resolver must always return SOME provider — never provider=null — + // unless the model is genuinely unknown. The bug was: a sync-injected + // kiro entry could win the candidate race, so the resolver would return + // kiro (which then 404'd upstream). + if (info.provider === null) { + assert.equal( + (info as Record).errorType, + "ambiguous_model", + "if unresolved, must surface ambiguous_model (operator-actionable), not silent null" + ); + return; + } + + // Whatever provider won, the inference path MUST NOT have routed to + // `kiro` — the kiro registry at the time of this fix does not catalog + // `claude-opus-5`. A future fix that adds `claude-opus-5` to the kiro + // registry will need to update this test. + const resolved = info.provider; + assert.notEqual( + resolved, + "kiro", + `kiro does not catalog claude-opus-5 in its static registry — bare routing must not silently land there (got: ${resolved})` + ); + + // And it must be one of the actual static-registry candidates for + // claude-opus-5: anthropic, claude (Claude Code OAuth), claude/web, + // cheaperinference, github, vertex/partner, ghe-copilot, agentrouter. + assert.ok( + [ + "anthropic", + "claude", + "claude-web", + "cheaperinference", + "github", + "vertex-partner", + "ghe-copilot", + "agentrouter", + ].includes(resolved), + `expected ${resolved} to be one of the static-registry providers that actually catalog claude-opus-5` + ); +}); + +test("bare claude-opus-4-8 still resolves (regression guard)", async () => { + // The bug only manifested for claude-opus-5 in the field report because + // kiro's synced catalog was the one that picked it up. This test pins + // that the same fix does not regress the working claude-opus-4-8 path. + // In unit-test isolation (no DB → activeProviders=null), models with >1 + // candidate return ambiguous_model rather than a concrete provider — + // the contract here is that the resolver NEVER lands on `kiro` regardless. + const info = await getModelInfoCore("claude-opus-4-8", null); + assert.notEqual( + info.provider, + "kiro", + `kiro does not catalog claude-opus-4-8 — bare routing must not silently land there` + ); +}); + +test("bare routing accepts a brand-new modelId if only synced providers carry it (auto-discovery preserved)", async () => { + // Place-holder for the auto-discovery path. The fix only validates + // synced candidates that CONFLICT with the static registry; if no static + // entry exists, the synced provider list still wins. There is no + // catalogue-only brand-new model in the current fixtures to assert against, + // so this test merely documents the contract and pins the validation + // function behavior at the boundary. + const info = await getModelInfoCore("__no_such_model_in_registry__", null); + // Unknown bare id → provider=null (the resolver bails out cleanly). + assert.equal(info.provider, null); +}); \ No newline at end of file