diff --git a/devlog/_plan/260804_overnight_triage/000_dispositions.md b/devlog/_plan/260804_overnight_triage/000_dispositions.md new file mode 100644 index 000000000..bcb81ea6d --- /dev/null +++ b/devlog/_plan/260804_overnight_triage/000_dispositions.md @@ -0,0 +1,160 @@ +# 000 — Overnight PR triage: nine PRs, four verdicts + +Nine pull requests arrived overnight while the #951–#955 stack was in review. +The question for each: does it duplicate work already on the stack, is it a real +independent defect, or does it belong somewhere else entirely. + +Measured 2026-08-04 against `origin/dev` and the stack head +`codex/915-cooldown-recovery-probe` at `493329df0`. + +## Verdicts + +| PR | Author | Verdict | Action | +|---|---|---|---| +| #967 | @Yuxin-Qiao | **two real defects in OUR #955** | carry onto layer 6 | +| #965 | @Yuxin-Qiao | correct fix for #962 | carry onto layer 6 | +| #963 | @MarcTCruz | duplicate of #965, broader and weaker | close, name #965 | +| #968 | @DevMello | real independent defect | carry onto layer 6 | +| #964 | @Yuxin-Qiao | real independent defect (#956) | leave open, own review | +| #966 | @Yuxin-Qiao | partial fix, fifth falsified design | leave open, request changes | +| #970 | @stephen-drew | real, out of stack scope | leave open | +| #961 | @Yuxin-Qiao | feature, not a bug | leave open | +| #969 | @Wibias | CI governance policy | out of scope | + +## #967 — the one that matters most + +@Yuxin-Qiao reviewed #955 and found two defects **in my own code**. Both verify. + +**P1: monthly-classified snapshots were rejected.** My +`isCompleteCodexQuotaRecoverySnapshot()` picked the required window from the +plan NAME. The parser picks it from the window DURATION +(`isExplicitMonthlyWindow()`, `src/codex/quota.ts:104-109`), and a Team response +whose primary window is explicitly monthly parses to `monthlyPercent` only. So +the probe rejected every successful fresh read for those accounts: + +```console +$ bun run .tmp/probe_967.ts # on the stack head, BEFORE the fix +parsed = {"monthlyPercent":12,"monthlyResetAt":1900000000} +recoverable? = false <-- Team monthly account can never recover +weekly parsed = {"weeklyPercent":12,"weeklyResetAt":1900000000} +recoverable? = true +``` + +That is the same "cooled forever" failure #915 exists to fix, reintroduced for +monthly-window plans. It is also the *third* time this predicate has been wrong +in the same direction — first the plan allowlist, then `prolite`, now the +window-classification mismatch. The lesson has been consistent and I kept +missing it: this predicate must read what the parser actually wrote, never +re-derive the classification itself. + +**P2: the probe's own token refresh looked like a replacement.** +`getValidCodexToken()` refreshes a near-expiry token inside the probe fetch and +advances the credential generation by exactly one +(`src/codex/account-store.ts:415-421`). My settle required the claim-time +generation to match exactly, so a successful fresh read under the refreshed +generation was thrown away and recovery waited another five minutes. + +The fix fences the +1 transition on `replacedAt`, which is the right +discriminator: +`saveCodexAccountCredentialIfGeneration()` **preserves** `replacedAt` +(`:195-217`) while `saveCodexAccountCredential()` **stamps a fresh one** +(`:131-146`). So a probe-owned refresh and an external replacement are +distinguishable even though both bump the generation. + +Verified after the fix — every fail-closed guard still holds: + +```console +credits-only -> false windowless {} -> false null -> false +exhausted 100 -> false go+weeklyOnly -> false +team monthly -> true team weekly -> true +``` + +Red-green: ablating P1 fails 2 tests, ablating P2 fails 1 different test. + +## #963 vs #965 — the duplicate pair + +Both claim "Fixes #962", both edit `src/codex/catalog/provider-fetch.ts`. +**#965 wins.** + +#962 is specifically about a custom row *replacing* a same-slug provider row. +#965 models exactly that: it indexes the rows deduplication will replace and +fills only undefined capability fields from the replaced row, so it also +inherits live `/models` metadata such as normalized `capabilities`. + +#963 instead recomputes `catalogHintsFromProviderConfig()` for **every** custom +row, including custom-only rows with no provider counterpart — broader than +#962 requires. It cannot retain discovered metadata, since it rebuilds from +config rather than inheriting. And it rewrites an existing regression contract +to fit: `tests/catalog-vision-sidecar-modalities.test.ts` changes from "no +registry reasoning metadata leaks onto an unmatched custom override" to +expecting that leak, and drops three `fetch should not be called` guards. + +Changing a test that encodes a deliberate prior decision, in order to make a +broader change pass, is the part that decides this. #965's ablation fails +exactly one test — the #962 regression — which is what a focused fix looks like. + +## #966 — a fifth design, still falsified + +#966 targets #914, which four prior designs already failed at an audit gate +(`devlog/_plan/260803_transport_attribution/000_plan.md`). It is a genuine +advance: it uses the real Bun 1.3.14 error labels including both alternating +ones, it does **not** repeat the hostname-resolution design, and it closes the +redirect counterexample on the pool Responses and Compact paths with manual +redirects. TLS/fake-IP codes correctly stay account-scoped. + +But two falsifications survive, both reproduced live: + +**Mixed 5xx → rejection still loses the attributable failure.** +`fetchWithTransientRetry()` discards prior transient responses when a later +attempt rejects (`src/lib/upstream-retry.ts:220-236`), so a genuine 503 followed +by a connection refusal is recorded as account-neutral. That is precisely the +hole the earlier audit documented. + +**Falsification 3 survives on five expanded surfaces.** Manual redirects were +added only to Responses and Compact; the five sidecar paths #966 newly +classifies still use default-follow fetch, so a credential-bearing sidecar that +receives a 307 to a dead host is misclassified as neutral — after the origin +already read the `Authorization` header: + +```text +redirect:"follow" serverSawAuthorization:"Bearer credential-follow" resolved:false +redirect:"manual" serverSawAuthorization:"Bearer credential-manual" resolved:true 307 +``` + +So #966 is not mergeable as-is, and its sidecar expansion carries the unresolved +hole *beyond* #914's original sites. It does supersede #922 (which misses one +Bun label entirely, keeps default redirects, and bundles unrelated probe-lease +work while sitting at `CHANGES_REQUESTED`). + +Neither lands on our stack. #914 remains open with a fifth design on record. + +## The rest + +**#968** (@DevMello) — the google adapter dropped `tool_choice` entirely: `none`, +`required`, and a forced tool all produced a wire body identical to `auto`, with +only a prose nudge in the system prompt. The wire compiler already validated +`toolConfig.functionCallingConfig`; the adapter simply never built it. Carries +cleanly onto the stack; same author as the already-carried #943. + +**#964** (@Yuxin-Qiao) — the `nvidia` registry entry lacks `noVisionModels`, so +the vision sidecar never activates for NIM text-only models and raw image parts +reach a text-only upstream. Real, but it is a registry/provider change with no +relationship to this stack's theme; it deserves its own review rather than a +ride on a bug stack. + +**#970** (@stephen-drew) — service re-registration during self-update. Real, but +30 files across CLI, GUI, and five docs locales, touching a +permission-sensitive install path. Out of scope here. + +**#961** — provider custom headers via PATCH. A feature, not a bug. + +**#969** (@Wibias, collaborator) — CI policy that auto-drafts contributor PRs +until a checklist is complete. Out of scope: it is a workflow change requiring +security review per `AGENTS.md`, and it encodes a contribution policy that is +the maintainer's call, not a bug fix. Worth noting the history — the #900–#905 +stack was closed on exactly this kind of policy question, not on its mechanics. + +## Layer 6 contents + +Carry, in order: #967 (fixes our own defects), #965 (catalog), #968 (google +tool_choice). Everything else stays where it is, with a reason on record. diff --git a/src/adapters/google.ts b/src/adapters/google.ts index 58374ddcd..70ee585ac 100644 --- a/src/adapters/google.ts +++ b/src/adapters/google.ts @@ -12,7 +12,7 @@ import type { OcxToolCall, OcxUsage, } from "../types"; -import { isAllowedToolChoice, namespacedToolName, toolAllowedByChoice } from "../types"; +import { isAllowedToolChoice, namespacedToolName, resolveToolChoiceWireName, toolAllowedByChoice } from "../types"; import { contentPartsToText, parseDataUrl } from "./image"; import { getVertexAccessToken } from "../lib/gcp-adc"; import { fetchAntigravityWithRetry, fetchVertexWithRetry } from "./google-http"; @@ -232,6 +232,28 @@ function toolsToGeminiFormat(parsed: OcxParsedRequest): unknown[] | undefined { }]; } +/** + * Client tool_choice enforcement on the wire. The catalog nudge states the same contract in + * prose, but without functionCallingConfig the model is free to ignore it. "auto" stays absent + * so the common case is byte-identical. The allowedTools variant already filters the + * declarations in toolsToGeminiFormat; only its "required" half needs a wire mode. + */ +function toolChoiceToGeminiToolConfig(parsed: OcxParsedRequest): Record | undefined { + const choice = parsed.options.toolChoice; + if (!choice || choice === "auto") return undefined; + if (choice === "none") return { functionCallingConfig: { mode: "NONE" } }; + if (choice === "required") return { functionCallingConfig: { mode: "ANY" } }; + if (isAllowedToolChoice(choice)) { + return choice.mode === "required" ? { functionCallingConfig: { mode: "ANY" } } : undefined; + } + return { + functionCallingConfig: { + mode: "ANY", + allowedFunctionNames: [resolveToolChoiceWireName(parsed.context.tools, choice.name)], + }, + }; +} + function usageFromGemini(usage: Record | undefined): OcxUsage | undefined { if (!usage) return undefined; return { @@ -307,6 +329,10 @@ export function createGoogleAdapter(provider: OcxProviderConfig): ProviderAdapte const body: Record = { contents }; if (systemInstruction) body.systemInstruction = systemInstruction; if (tools) body.tools = tools; + // Only meaningful with declarations on the wire: mode ANY with an empty + // catalog is a guaranteed upstream 400. + const toolConfig = tools ? toolChoiceToGeminiToolConfig(parsed) : undefined; + if (toolConfig) body.toolConfig = toolConfig; const generationConfig: Record = {}; if (parsed.options.maxOutputTokens) generationConfig.maxOutputTokens = parsed.options.maxOutputTokens; @@ -358,6 +384,12 @@ export function createGoogleAdapter(provider: OcxProviderConfig): ProviderAdapte const draftRequest: Record = { ...body, sessionId }; // Claude-on-Antigravity forces VALIDATED function calling (the real client always sets it). if (/claude/i.test(wireModelId)) { + // VALIDATED would defeat a client's tool_choice "none": honor it by dropping the + // declarations instead, the wire shape of a tool-less Claude turn. + if (parsed.options.toolChoice === "none") { + delete draftRequest.tools; + delete draftRequest.toolConfig; + } const existing = (draftRequest.toolConfig ?? {}) as Record; const fcc = (existing.functionCallingConfig ?? {}) as Record; draftRequest.toolConfig = { ...existing, functionCallingConfig: { ...fcc, mode: "VALIDATED" } }; diff --git a/src/codex/catalog/provider-fetch.ts b/src/codex/catalog/provider-fetch.ts index 5422e1db5..71bc27227 100644 --- a/src/codex/catalog/provider-fetch.ts +++ b/src/codex/catalog/provider-fetch.ts @@ -782,6 +782,9 @@ async function gatherRoutedModelsUncached( // Enriched (registry-hydrated) provider clones, keyed by name — the same view used above so // custom rows get the same noVisionModels / inputModalities treatment as discovered rows. const enrichedByName = new Map(activeProviders); + // Provider-derived rows keyed by their Codex-facing slug: a custom override replaces the row + // with the same slug below, so that row's provider capability metadata is the inheritance source. + const replacedByRoutedSlug = new Map(all.map(model => [routedSlug(model.provider, model.id), model])); const customModels = (config.customModels ?? []).map(cm => { const rawProvider = config.providers[cm.provider]; const supportsReasoningSummaries = configuredReasoningSummarySupport(rawProvider, cm.modelId); @@ -794,19 +797,38 @@ async function gatherRoutedModelsUncached( ...(cm.inputModalities ? { inputModalities: cm.inputModalities } : {}), ...(typeof supportsReasoningSummaries === "boolean" ? { supportsReasoningSummaries } : {}), }; + // #962: the dedupe below drops the provider-derived row this custom row replaces. Inherit that + // row's provider capability metadata (reasoning ladder, default effort, parallel tool calls, + // context, ...) so the generated catalog keeps advertising what the router actually provides. + // Explicit custom fields win by construction; this only fills gaps. Without it a + // noReasoningModels model loses its empty ladder and the catalog synthesizes the generic one, + // which Codex then rejects for spawn_agent with effort "none". + const replaced = replacedByRoutedSlug.get(routedSlug(cm.provider, cm.modelId)); + const merged: CatalogModel = replaced ? { + ...base, + ...(base.contextWindow === undefined && replaced.contextWindow !== undefined ? { contextWindow: replaced.contextWindow } : {}), + ...(base.maxInputTokens === undefined && replaced.maxInputTokens !== undefined ? { maxInputTokens: replaced.maxInputTokens } : {}), + ...(base.inputModalities === undefined && replaced.inputModalities !== undefined ? { inputModalities: replaced.inputModalities } : {}), + ...(base.reasoningEfforts === undefined && replaced.reasoningEfforts !== undefined ? { reasoningEfforts: replaced.reasoningEfforts } : {}), + ...(base.defaultReasoningEffort === undefined && replaced.defaultReasoningEffort !== undefined ? { defaultReasoningEffort: replaced.defaultReasoningEffort } : {}), + ...(base.parallelToolCalls === undefined && replaced.parallelToolCalls !== undefined ? { parallelToolCalls: replaced.parallelToolCalls } : {}), + ...(base.supportsVerbosity === undefined && replaced.supportsVerbosity !== undefined ? { supportsVerbosity: replaced.supportsVerbosity } : {}), + ...(base.supportsReasoningSummaries === undefined && replaced.supportsReasoningSummaries !== undefined ? { supportsReasoningSummaries: replaced.supportsReasoningSummaries } : {}), + ...(base.capabilities === undefined && replaced.capabilities !== undefined ? { capabilities: replaced.capabilities } : {}), + } : base; // Vision-sidecar coverage ONLY: if the custom model is in the enriched provider's // noVisionModels, advertise image input so the Codex app lets images reach the sidecar // (#349/#344). Deliberately NOT the full applyProviderConfigHints pass — custom rows are a // user override, so their explicit contextWindow / inputModalities / reasoning fields must be // preserved verbatim (the hint pass would cap context and overwrite modalities from registry). const enrichedProvider = enrichedByName.get(cm.provider) ?? rawProvider; - if (enrichedProvider && modelInList(enrichedProvider.noVisionModels, base.id)) { - const current = base.inputModalities ?? ["text"]; + if (enrichedProvider && modelInList(enrichedProvider.noVisionModels, merged.id)) { + const current = merged.inputModalities ?? ["text"]; if (!current.includes("image")) { - return { ...base, inputModalities: [...current, "image"] }; + return { ...merged, inputModalities: [...current, "image"] }; } } - return base; + return merged; }); // Custom rows override discovered rows that encode to the same Codex-facing slug. const customKeys = new Set(customModels.map(c => routedSlug(c.provider, c.id))); diff --git a/src/codex/quota.ts b/src/codex/quota.ts index 739baf986..562e7082e 100644 --- a/src/codex/quota.ts +++ b/src/codex/quota.ts @@ -9,6 +9,14 @@ export type StoredAccountQuota = { weeklyResetAt?: number; monthlyResetAt?: number; resetCredits?: number; + /** + * True when `monthlyPercent` came from an explicitly-monthly PRIMARY window — + * i.e. it is the account's governing quota reading, not a supplementary + * tertiary window. Tertiary-only monthly data lands in the same field but says + * nothing about the weekly quota that actually gates a non-Go/Free account, + * so recovery must be able to tell the two apart (#967 audit). + */ + monthlyIsPrimaryWindow?: boolean; updatedAt: number; }; @@ -96,17 +104,35 @@ export function codexQuotaWindowForPlan(plan?: string | null): "monthly" | "week } export function isCompleteCodexQuotaRecoverySnapshot( - quota: Pick | null, + quota: Pick | null, plan?: string | null, ): boolean { if (!quota || isCodexQuotaExhausted(quota, plan)) return false; // Recovery still fails closed on MISSING EVIDENCE — a credits-only or windowless payload // carries no usage reading at all and must never clear a cooldown. What it does not do is // fail closed on an unfamiliar plan NAME, which only ever meant "cooled forever". - const required = codexQuotaWindowForPlan(plan) === "monthly" - ? quota.monthlyPercent - : quota.weeklyPercent; - return typeof required === "number" && Number.isFinite(required); + // + // The parser classifies windows by DURATION, not by plan name: a Team response whose + // primary window is explicitly monthly parses to monthlyPercent only (no secondary + // window exists), so requiring weeklyPercent because the plan is not go/free would + // strand exactly those accounts until their predicted expiry. Accept whichever window(s) + // the parser actually wrote; Go/Free never carry a weekly value, so monthly-only is + // required there. + // + // Audit correction: "the parser wrote monthlyPercent" is NOT by itself evidence for a + // weekly-quota plan. A tertiary-only response also writes monthlyPercent, and it says + // nothing about the weekly quota that actually gates a Team/Plus account — accepting it + // would clear the cooldown on a reading of a different window. Only an explicitly-monthly + // PRIMARY window is the governing reading, which is what `monthlyIsPrimaryWindow` records. + if (codexQuotaWindowForPlan(plan) === "monthly") { + return finitePercent(quota.monthlyPercent); + } + if (finitePercent(quota.weeklyPercent)) return true; + return quota.monthlyIsPrimaryWindow === true && finitePercent(quota.monthlyPercent); +} + +function finitePercent(value: number | undefined): boolean { + return typeof value === "number" && Number.isFinite(value); } export function normalizeUsagePercent(value: unknown): number | undefined { @@ -180,6 +206,7 @@ export function setAccountQuotaFromParsed( if (existing?.weeklyResetAt !== undefined) next.weeklyResetAt = existing.weeklyResetAt; if (existing?.monthlyPercent !== undefined) next.monthlyPercent = existing.monthlyPercent; if (existing?.monthlyResetAt !== undefined) next.monthlyResetAt = existing.monthlyResetAt; + if (existing?.monthlyIsPrimaryWindow === true) next.monthlyIsPrimaryWindow = true; next.resetCredits = quota.resetCredits; accountQuota.set(accountId, next); schedulePersistAccountQuotas(); @@ -199,9 +226,15 @@ export function setAccountQuotaFromParsed( if (snapshotHasMonthly(quota)) { if (quota.monthlyPercent !== undefined) next.monthlyPercent = quota.monthlyPercent; if (quota.monthlyResetAt !== undefined) next.monthlyResetAt = quota.monthlyResetAt; + // Carry the provenance with the value it describes. Recovery reads `freshQuota` directly, + // so this is not on its path today — but a cached snapshot that kept `monthlyPercent` + // while silently dropping `monthlyIsPrimaryWindow` would look like tertiary-only data to + // any future reader, and that failure would be invisible. + if (quota.monthlyIsPrimaryWindow === true) next.monthlyIsPrimaryWindow = true; } else if (snapshotHasWeekly(quota) && existing?.monthlyPercent !== undefined) { next.monthlyPercent = existing.monthlyPercent; if (existing.monthlyResetAt !== undefined) next.monthlyResetAt = existing.monthlyResetAt; + if (existing.monthlyIsPrimaryWindow === true) next.monthlyIsPrimaryWindow = true; } if (quota.resetCredits !== undefined) next.resetCredits = quota.resetCredits; @@ -234,6 +267,10 @@ export function parseUpstreamQuotaHeaders(headers: Headers): Omit = []; for (const [order, account] of (config.codexAccounts ?? []).entries()) { @@ -473,6 +476,7 @@ export function claimDueCodexQuotaRecoveryProbes( ...(candidate.scope ? { scope: candidate.scope } : {}), health: candidate.health, credentialGeneration: record.generation, + ...(record.replacedAt !== undefined ? { credentialReplacedAt: record.replacedAt } : {}), order, }); } @@ -497,6 +501,9 @@ export function claimDueCodexQuotaRecoveryProbes( leaseId, cooldownGeneration: candidate.health.cooldownGeneration ?? 0, credentialGeneration: candidate.credentialGeneration, + ...(candidate.credentialReplacedAt !== undefined + ? { credentialReplacedAt: candidate.credentialReplacedAt } + : {}), }; }); } @@ -512,10 +519,21 @@ export function settleCodexQuotaRecoveryProbe( ? scopedHealthFor(claim.accountId, claim.scope) : upstreamHealth.get(claim.accountId); if (!health || health.probeLeaseId !== claim.leaseId) return false; + const currentRecord = readCodexAccountRecord(claim.accountId); + const proofGeneration = proof.credentialGeneration; + // A probe-owned token refresh (getValidCodexToken) advances the credential generation by + // exactly one while preserving `replacedAt`; an external credential replacement bumps the + // generation too but stamps a fresh `replacedAt`. Accept the +1 transition only when the + // claim-time lineage is intact AND the generation the fresh quota was proven under is live. + const generationFenced = proofGeneration !== undefined + && (proofGeneration === claim.credentialGeneration + ? isCodexAccountGenerationLive(claim.accountId, proofGeneration) + : proofGeneration === claim.credentialGeneration + 1 + && currentRecord?.replacedAt === claim.credentialReplacedAt + && isCodexAccountGenerationLive(claim.accountId, proofGeneration)); const fenced = (health.cooldownGeneration ?? 0) === claim.cooldownGeneration && (health.probeLeaseGeneration ?? 0) === claim.cooldownGeneration - && claim.credentialGeneration === proof.credentialGeneration - && isCodexAccountGenerationLive(claim.accountId, claim.credentialGeneration); + && generationFenced; if (!recovered || !fenced) { const released = withProbeLeaseReleased(health, now); if (claim.scope) setScopedHealth(claim.accountId, claim.scope, released); diff --git a/tests/codex-catalog.test.ts b/tests/codex-catalog.test.ts index 3a00105a6..2debabd4d 100644 --- a/tests/codex-catalog.test.ts +++ b/tests/codex-catalog.test.ts @@ -848,6 +848,62 @@ describe("configured CatalogModel displayName -> catalog display_name", () => { }); }); +test("a custom row inherits provider reasoning metadata from the provider-derived row it replaces (#962)", async () => { + clearModelCache("ollama"); + const originalFetch = globalThis.fetch; + globalThis.fetch = (() => { throw new Error("fetch should not be called"); }) as typeof fetch; + try { + const models = await gatherRoutedModels({ + port: 10100, + defaultProvider: "ollama", + providers: { + ollama: { + baseUrl: "http://localhost:11434/v1", + adapter: "openai-chat", + authMode: "key", + liveModels: false, + models: ["qwen-coder-3b"], + selectedModels: ["qwen-coder-3b"], + noReasoningModels: ["qwen-coder-3b"], + modelReasoningEfforts: { "qwen-coder-3b": [] }, + }, + }, + customModels: [ + { + id: "cm-962", + provider: "ollama", + modelId: "qwen-coder-3b", + displayName: "Qwen Coder 3B (local)", + contextWindow: 32768, + inputModalities: ["text"], + addedAt: "2026-01-01T00:00:00.000Z", + }, + ], + }); + + // Explicit custom fields stay verbatim; provider capability metadata is inherited from the + // replaced provider-derived row (noReasoningModels -> empty reasoning ladder, openai-chat + // adapter -> parallel tool calls). + const custom = models.find(m => m.provider === "ollama" && m.id === "qwen-coder-3b"); + expect(custom?.displayName).toBe("Qwen Coder 3B (local)"); + expect(custom?.contextWindow).toBe(32768); + expect(custom?.inputModalities).toEqual(["text"]); + expect(custom?.reasoningEfforts).toEqual([]); + expect(custom?.parallelToolCalls).toBe(true); + + const entries = buildCatalogEntries(nativeTemplate(), [], models); + const row = entries.find(e => e.slug === "ollama/qwen-coder-3b"); + expect(row?.display_name).toBe("Qwen Coder 3B (local)"); + // The catalog must expose no reasoning levels and no default reasoning level for this model; + // the generic low..ultra ladder and the medium default must not be synthesized. + expect(row?.supported_reasoning_levels).toEqual([]); + expect(row?.default_reasoning_level).toBeUndefined(); + } finally { + globalThis.fetch = originalFetch; + clearModelCache("ollama"); + } +}); + function openAiApiCatalogConfig(overrides: Record = {}): OcxConfig { return { port: 10100, diff --git a/tests/codex-cooldown-recovery.test.ts b/tests/codex-cooldown-recovery.test.ts index 7ce26fd19..c34eff6c6 100644 --- a/tests/codex-cooldown-recovery.test.ts +++ b/tests/codex-cooldown-recovery.test.ts @@ -8,7 +8,14 @@ import { seedCodexAuthAdmissionForTests, } from "../src/codex/auth-api"; import { saveCodexAccountCredential } from "../src/codex/account-store"; -import { codexQuotaWindowForPlan, isCompleteCodexQuotaRecoverySnapshot } from "../src/codex/quota"; +import { + codexQuotaWindowForPlan, + getAccountQuota, + isCompleteCodexQuotaRecoverySnapshot, + parseUsageQuota, + setAccountQuotaFromParsed, + updateAccountQuota, +} from "../src/codex/quota"; import upstreamModels from "../src/codex/data/upstream-models.json"; import { CODEX_QUOTA_PROBE_INTERVAL_MS, @@ -117,6 +124,78 @@ describe("Codex cooldown recovery worker", () => { expect(routed).toEqual(["b", "b"]); }); + test("recovers a Team account from a duration-classified monthly snapshot", async () => { + // WHAM can legitimately return only an explicitly monthly primary window for a Team + // plan (30.4-day window, no secondary). parseUsageQuota then writes monthlyPercent only, + // so recovery must accept the window the parser actually classified instead of demanding + // a weekly reading because the plan name is not "go"/"free". + const config = makeConfig(["a"]); + saveCredential("a"); + cool(config, "a"); + globalThis.fetch = async () => usageResponse(0, { + plan_type: "team", + rate_limit: { + primary_window: { used_percent: 6, reset_at: 1_900_000_000, limit_window_seconds: 2_628_000 }, + secondary_window: null, + tertiary_window: null, + }, + rate_limit_reset_credits: { available_count: 0 }, + }); + await runCodexCooldownRecoveryProbes(config, due()); + expect(getCodexQuotaHealthSnapshot("a", "shared", due() + 1)).toBeNull(); + }); + + test("does NOT recover a Team account from a tertiary-only monthly snapshot", async () => { + // The mirror image of the case above, and the reason accepting "whatever the parser + // wrote" is too permissive. A tertiary window also lands in monthlyPercent, but it + // describes a different period and says nothing about the WEEKLY quota that actually + // gates a Team account — clearing the cooldown on it would restore traffic to an + // account whose governing window was never read. Only an explicitly-monthly PRIMARY + // window is that reading, which is what monthlyIsPrimaryWindow records. + const config = makeConfig(["a"]); + saveCredential("a"); + cool(config, "a"); + globalThis.fetch = async () => usageResponse(0, { + plan_type: "team", + rate_limit: { + primary_window: null, + secondary_window: null, + tertiary_window: { used_percent: 7, reset_at: 1_900_000_000 }, + }, + rate_limit_reset_credits: { available_count: 0 }, + }); + await runCodexCooldownRecoveryProbes(config, due()); + expect(getCodexQuotaHealthSnapshot("a", "shared", due() + 1)).not.toBeNull(); + }); + + test("recovers when the probe's own token refresh advances the credential generation", async () => { + // A near-expiry access token makes getValidCodexToken() refresh it inside the probe + // fetch, bumping the credential generation from 1 to 2 before WHAM completes. The fresh + // quota is proven under the new live generation, so settling against the claim-time + // generation must accept this probe's own refresh, not treat it as a replacement. + const config = makeConfig(["a"]); + saveCodexAccountCredential("a", { + accessToken: "access-a", + refreshToken: "refresh-a", + expiresAt: Date.now() + 30_000, + chatgptAccountId: "acct-a", + }); + cool(config, "a"); + globalThis.fetch = async input => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; + if (url.includes("/oauth/token")) { + return new Response(JSON.stringify({ + access_token: "access-a-2", + refresh_token: "refresh-a-2", + expires_in: 3600, + }), { status: 200, headers: { "Content-Type": "application/json" } }); + } + return usageResponse(12); + }; + await runCodexCooldownRecoveryProbes(config, due()); + expect(getCodexQuotaHealthSnapshot("a", "shared", due() + 1)).toBeNull(); + }); + test.each([ ["still exhausted", () => usageResponse(100)], ["credits only", () => usageResponse(0, { plan_type: "team", rate_limit_reset_credits: { available_count: 1 } })], @@ -290,15 +369,50 @@ describe("Codex cooldown recovery worker", () => { for (const plan of snapshotPlans) { const monthly = codexQuotaWindowForPlan(plan) === "monthly"; - const filled = monthly ? { monthlyPercent: 12 } : { weeklyPercent: 12 }; - const empty = monthly ? { weeklyPercent: 12 } : { monthlyPercent: 12 }; - expect(isCompleteCodexQuotaRecoverySnapshot(filled, plan)).toBe(true); - // The other window is not evidence for this plan, in either direction. - expect(isCompleteCodexQuotaRecoverySnapshot(empty, plan)).toBe(false); + // The parser classifies windows by duration, so a weekly-billed plan CAN carry a + // monthly-only reading (30-day primary, no secondary) — but only when that reading is + // the primary window. A bare monthlyPercent could equally be a tertiary window, which + // is a different period and no evidence for the weekly quota that gates the account. + expect(isCompleteCodexQuotaRecoverySnapshot({ weeklyPercent: 12 }, plan)).toBe(!monthly); + expect(isCompleteCodexQuotaRecoverySnapshot({ monthlyPercent: 12 }, plan)).toBe(monthly); + expect(isCompleteCodexQuotaRecoverySnapshot({ monthlyPercent: 12, monthlyIsPrimaryWindow: true }, plan)).toBe(true); } + // Monthly-billed Go/Free parse to monthlyPercent only; a weekly-only reading is not + // evidence for them. + expect(isCompleteCodexQuotaRecoverySnapshot({ weeklyPercent: 12 }, "go")).toBe(false); + expect(isCompleteCodexQuotaRecoverySnapshot({ weeklyPercent: 12 }, "free")).toBe(false); + // Absent plan follows the parser's weekly default. expect(isCompleteCodexQuotaRecoverySnapshot({ weeklyPercent: 12 }, undefined)).toBe(true); + // Provenance, not just presence: monthlyPercent alone is evidence for a weekly-quota plan + // ONLY when it came from an explicitly-monthly primary window. + expect(isCompleteCodexQuotaRecoverySnapshot({ monthlyPercent: 12 }, "team")).toBe(false); + expect(isCompleteCodexQuotaRecoverySnapshot({ monthlyPercent: 12, monthlyIsPrimaryWindow: true }, "team")).toBe(true); + // Go/Free are governed by the monthly window either way, so the flag is not required. + expect(isCompleteCodexQuotaRecoverySnapshot({ monthlyPercent: 12 }, "go")).toBe(true); + + // The flag has to survive the cache, or the guard is decorative: a snapshot that keeps + // monthlyPercent while dropping its provenance looks exactly like tertiary-only data. + // setAccountQuotaFromParsed() rebuilds the record field by field, so this is a real + // drop risk rather than a theoretical one. + const parsedMonthly = parseUsageQuota({ + plan_type: "team", + rate_limit: { + primary_window: { used_percent: 12, limit_window_seconds: 2_628_000, reset_at: 1_900_000_000 }, + }, + } as never); + expect(parsedMonthly?.monthlyIsPrimaryWindow).toBe(true); + setAccountQuotaFromParsed("provenance-probe", parsedMonthly); + expect(getAccountQuota("provenance-probe")?.monthlyIsPrimaryWindow).toBe(true); + + // updateAccountQuota() rebuilds the record too. An unrelated weekly update must not + // downgrade a proven reading to unproven, and a caller-supplied monthly value — which + // arrives with no window information at all — must not inherit the proof. + updateAccountQuota("provenance-probe", 20, 111); + expect(getAccountQuota("provenance-probe")?.monthlyIsPrimaryWindow).toBe(true); + updateAccountQuota("provenance-probe", undefined, undefined, 44, 222); + expect(getAccountQuota("provenance-probe")?.monthlyIsPrimaryWindow).toBeUndefined(); // Missing EVIDENCE still fails closed — that is the guard that matters. expect(isCompleteCodexQuotaRecoverySnapshot({}, "plus")).toBe(false); expect(isCompleteCodexQuotaRecoverySnapshot(null, "plus")).toBe(false); diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index be0e8d23f..8b889cb81 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -1160,7 +1160,9 @@ describe("codex routing", () => { rate_limit: { primary_window: { used_percent: 39, reset_at: 3, limit_window_seconds: 2_628_000 }, }, - })).toEqual({ monthlyPercent: 39, monthlyResetAt: 3 }); + // The provenance flag rides with the value: this monthly reading IS the primary window, + // which is what lets recovery tell it apart from a tertiary-only monthly figure (#967). + })).toEqual({ monthlyPercent: 39, monthlyResetAt: 3, monthlyIsPrimaryWindow: true }); }); test("WHAM monthly primary preserves a legacy secondary weekly window", () => { @@ -1175,6 +1177,7 @@ describe("codex routing", () => { weeklyResetAt: 7, monthlyPercent: 39, monthlyResetAt: 30, + monthlyIsPrimaryWindow: true, }); }); diff --git a/tests/google-adapter.test.ts b/tests/google-adapter.test.ts index 4eecd1d48..467cf19c3 100644 --- a/tests/google-adapter.test.ts +++ b/tests/google-adapter.test.ts @@ -189,3 +189,92 @@ describe("google adapter — tool-call ids on the wire", () => { expect(fc).toBe("call_xyz"); }); }); + +describe("google adapter — tool_choice on the wire", () => { + const TOOLS = [ + { name: "get_weather", parameters: { type: "object", properties: {} } }, + { name: "shot", namespace: "mcp__chrome", parameters: { type: "object", properties: {} } }, + ]; + + function parsedWithChoice(toolChoice: unknown, tools: unknown[] | null = TOOLS): OcxParsedRequest { + return { + modelId: "gemini-3-pro", + stream: false, + options: toolChoice === undefined ? {} : { toolChoice }, + context: { messages: [{ role: "user", content: "hi" }], tools: tools ?? undefined }, + } as unknown as OcxParsedRequest; + } + + test('"none" and "required" map to NONE and ANY', async () => { + expect((await geminiBody(parsedWithChoice("none"))).toolConfig) + .toEqual({ functionCallingConfig: { mode: "NONE" } }); + expect((await geminiBody(parsedWithChoice("required"))).toolConfig) + .toEqual({ functionCallingConfig: { mode: "ANY" } }); + }); + + test("a forced tool maps to ANY with its wire name allowed", async () => { + expect((await geminiBody(parsedWithChoice({ name: "get_weather" }))).toolConfig) + .toEqual({ functionCallingConfig: { mode: "ANY", allowedFunctionNames: ["get_weather"] } }); + // Dotted alias resolves to the namespaced declaration name. + expect((await geminiBody(parsedWithChoice({ name: "mcp__chrome.shot" }))).toolConfig) + .toEqual({ functionCallingConfig: { mode: "ANY", allowedFunctionNames: ["mcp__chrome__shot"] } }); + }); + + test('"auto", absent, and allowedTools+auto stay byte-identical (no toolConfig)', async () => { + expect((await geminiBody(parsedWithChoice("auto"))).toolConfig).toBeUndefined(); + expect((await geminiBody(parsedWithChoice(undefined))).toolConfig).toBeUndefined(); + expect((await geminiBody(parsedWithChoice({ allowedTools: ["get_weather"], mode: "auto" }))).toolConfig).toBeUndefined(); + }); + + test("allowedTools with mode required keeps the filtered catalog and adds ANY", async () => { + const body = await geminiBody(parsedWithChoice({ allowedTools: ["get_weather"], mode: "required" })); + const declared = (body.tools as { functionDeclarations: { name: string }[] }[])[0].functionDeclarations.map(d => d.name); + expect(declared).toEqual(["get_weather"]); + expect(body.toolConfig).toEqual({ functionCallingConfig: { mode: "ANY" } }); + }); + + test("no declared tools means no toolConfig even with a choice", async () => { + expect((await geminiBody(parsedWithChoice("none", null))).toolConfig).toBeUndefined(); + expect((await geminiBody(parsedWithChoice({ name: "get_weather" }, []))).toolConfig).toBeUndefined(); + }); + + test('claude-on-antigravity honors "none" by dropping the declarations', async () => { + const ccaProvider = { + adapter: "google", + googleMode: "cloud-code-assist", + baseUrl: "https://daily-cloudcode-pa.googleapis.com", + apiKey: "key", + project: "proj-123", + }; + const claudeParsed = parsedWithChoice("none"); + (claudeParsed as unknown as { modelId: string }).modelId = "claude-opus-4.8"; + const claudeRequest = JSON.parse((await createGoogleAdapter(ccaProvider).buildRequest(claudeParsed)).body).request as Record; + // VALIDATED would defeat NONE, so the declarations go instead; the config matches a tool-less turn. + expect(claudeRequest.tools).toBeUndefined(); + expect(claudeRequest.toolConfig).toEqual({ functionCallingConfig: { mode: "VALIDATED" } }); + + // Gemini on the same route has no VALIDATED override, so NONE rides with the catalog intact. + const geminiParsed = parsedWithChoice("none"); + const geminiRequest = JSON.parse((await createGoogleAdapter(ccaProvider).buildRequest(geminiParsed)).body).request as Record; + const declared = (geminiRequest.tools as { functionDeclarations: { name: string }[] }[])[0].functionDeclarations.map(d => d.name); + expect(declared).toEqual(["get_weather", "mcp__chrome__shot"]); + expect(geminiRequest.toolConfig).toEqual({ functionCallingConfig: { mode: "NONE" } }); + }); + + test("claude-on-antigravity keeps VALIDATED mode over a client choice, allowed names survive", async () => { + const ccaProvider = { + adapter: "google", + googleMode: "cloud-code-assist", + baseUrl: "https://daily-cloudcode-pa.googleapis.com", + apiKey: "key", + project: "proj-123", + }; + const parsed = parsedWithChoice({ name: "get_weather" }); + (parsed as unknown as { modelId: string }).modelId = "claude-opus-4.8"; + const { body } = await createGoogleAdapter(ccaProvider).buildRequest(parsed); + const request = JSON.parse(body).request as Record; + expect(request.toolConfig).toEqual({ + functionCallingConfig: { mode: "VALIDATED", allowedFunctionNames: ["get_weather"] }, + }); + }); +}); diff --git a/tests/rate-limit-reset-credits.test.ts b/tests/rate-limit-reset-credits.test.ts index 9d66dc1f6..122d78c01 100644 --- a/tests/rate-limit-reset-credits.test.ts +++ b/tests/rate-limit-reset-credits.test.ts @@ -128,7 +128,7 @@ describe("rate-limit reset credits", () => { }, rate_limit_reset_credits: { available_count: 0 }, }); - expect(quota).toEqual({ monthlyPercent: 6, monthlyResetAt: 1787336442, resetCredits: 0 }); + expect(quota).toEqual({ monthlyPercent: 6, monthlyResetAt: 1787336442, resetCredits: 0, monthlyIsPrimaryWindow: true }); expect(quota!.weeklyPercent).toBeUndefined(); }); @@ -143,6 +143,7 @@ describe("rate-limit reset credits", () => { expect(quota).toEqual({ monthlyPercent: 39, monthlyResetAt: 1787401330, + monthlyIsPrimaryWindow: true, weeklyPercent: 20, weeklyResetAt: 1787000000, }); @@ -156,7 +157,7 @@ describe("rate-limit reset credits", () => { tertiary_window: { used_percent: 50, reset_at: 1788000000 }, }, }); - expect(quota).toEqual({ monthlyPercent: 39, monthlyResetAt: 1787401330 }); + expect(quota).toEqual({ monthlyPercent: 39, monthlyResetAt: 1787401330, monthlyIsPrimaryWindow: true }); }); it("falls back to tertiary wholesale when a monthly primary has no percent", () => { @@ -179,6 +180,8 @@ describe("rate-limit reset credits", () => { tertiary_window: { used_percent: 50, reset_at: 1788000000 }, }, }); + // No provenance flag on the Go/Free branch: the monthly window governs those plans + // regardless of which window produced the reading, so recovery never consults it. expect(quota).toEqual({ monthlyPercent: 30, monthlyResetAt: 1787401330 }); }); @@ -310,11 +313,12 @@ describe("rate-limit reset credits", () => { secondary_window: null, }, }); - expect(quota).toEqual({ monthlyPercent: 100, monthlyResetAt: 1787401330 }); + expect(quota).toEqual({ monthlyPercent: 100, monthlyResetAt: 1787401330, monthlyIsPrimaryWindow: true }); setAccountQuotaFromParsed("monthly-A", quota!); expect(getAccountQuota("monthly-A")).toEqual({ monthlyPercent: 100, monthlyResetAt: 1787401330, + monthlyIsPrimaryWindow: true, updatedAt: expect.any(Number), }); }); @@ -331,6 +335,7 @@ describe("rate-limit reset credits", () => { expect(getAccountQuota("monthly-A")).toEqual({ monthlyPercent: 100, monthlyResetAt: 1787401330, + monthlyIsPrimaryWindow: true, updatedAt: expect.any(Number), }); }); @@ -381,6 +386,7 @@ describe("rate-limit reset credits", () => { expect(getAccountQuota("team-tertiary")).toEqual({ monthlyPercent: 39, monthlyResetAt: 1787401330, + monthlyIsPrimaryWindow: true, updatedAt: expect.any(Number), }); }); @@ -431,6 +437,7 @@ describe("rate-limit reset credits", () => { expect(getAccountQuota("team-A")).toEqual({ monthlyPercent: 39, monthlyResetAt: 1787401330, + monthlyIsPrimaryWindow: true, weeklyPercent: 20, weeklyResetAt: 1787000000, updatedAt: expect.any(Number),