diff --git a/devlog/_plan/260820_bug_pr_backlog_consolidation/080_residual_dispositions.md b/devlog/_plan/260820_bug_pr_backlog_consolidation/080_residual_dispositions.md index e47d8aa35b..460885e19f 100644 --- a/devlog/_plan/260820_bug_pr_backlog_consolidation/080_residual_dispositions.md +++ b/devlog/_plan/260820_bug_pr_backlog_consolidation/080_residual_dispositions.md @@ -161,6 +161,56 @@ Deliberately NOT copied from omniroute: `x-opencode-project`, `x-opencode-reques conversation-derived session identifier is a privacy-relevant change that needs its own evidence rather than a sibling project's precedent. +### wp16 outcome — and the bug the absorb uncovered + +**PR #2160**, branch `codex/absorb-opencode-free-static-headers`, base `dev`. #2067 closed +with attribution (`issuecomment-5349492578`). + +The audit is the interesting part. The plan as written — add the header to the registry row — +passed my own reading and FAILED the reviewer, correctly. `staticHeaders` is documented at +`registry.ts:149` as "merged into every upstream request for this provider", and that was +false. It was copied at seed time only: `providerConfigSeed` writes the block once, +`enrichProviderFromCatalog` fills it only when the whole block is absent, and nothing merged +it at request time. `rg -n 'headers' src/router.ts` returned zero hits. + +Reproduced directly before accepting the finding: + +| persisted config | `routedProviderConfig("opencode-free", ...).headers` | +|---|---| +| no headers block | `undefined` | +| `{x-opencode-client: desktop}` | unchanged — no UA | +| `{user-agent: custom-agent}` | unchanged — no client marker | + +So the contributor's one-line registry patch would have shipped a header that **no existing +install ever receives**. The management API strips a persisted block that exactly matches the +registry set, which means the most common on-disk state is "no headers at all" — and that +state gained nothing. + +Implementation, three parts: + +1. `mergeRegistryStaticHeaders(staticHeaders, userHeaders)` in `registry.ts` — registry values + fill only names the user has not claimed, compared **case-insensitively**. That last word is + load-bearing: HTTP header names are case-insensitive but object keys are not, so spreading a + registry `User-Agent` over a user's `user-agent` leaves both keys and `Headers` serializes + them as `"custom-agent, opencode"` — a corrupted request wearing the costume of an override. +2. `routedProviderConfig` (`router.ts`) merges at resolve time. +3. `buildModelsRequest` (`oauth/index.ts`) does the same, because a provider identified as + `opencode` when it completes but anonymous when it lists its own models reads as two + different clients to a rate limiter. + +Residual, stated rather than skipped: `validateApiKey` (`key-providers.ts:102`) still sends +only `Authorization`. It is an auth probe by design; widening an auth-path request shape is a +separate change with its own review burden. + +Evidence: 6 new regressions; reverting only `router.ts` + `oauth/index.ts` while keeping the +registry header fails exactly 5 of them (13 pass / 5 fail), which is what makes them delivery +tests rather than restatements of the registry constant. Full suite 13519 pass / 10 skip / +0 fail across 856 files; typecheck and privacy scan clean. + +One existing expectation moved: `tests/management-provider-validation.test.ts` "provider PATCH +clear keeps registry static headers" now asserts the two-header set. That is the same edit +#2067 made, and it is the correct one — the test pins the registry-owned set, which grew. + ## wp17-wp19 — the three that need new code Recorded here as each is decided; each is its own PABCD cycle. diff --git a/src/oauth/index.ts b/src/oauth/index.ts index 0162492f9a..fda4a3ec67 100644 --- a/src/oauth/index.ts +++ b/src/oauth/index.ts @@ -17,7 +17,7 @@ import { loginCommandCode, refreshCommandCodeToken } from "./command-code"; import { ANTIGRAVITY_REQUEST_UA } from "../adapters/google-antigravity-wire"; import { deriveOAuthDefaultModel, deriveOAuthProviderConfig } from "../providers/derive"; import { apiKeyPoolEntryId, sanitizeApiKeyValue } from "../providers/api-keys"; -import { effectiveGoogleMode, getProviderRegistryEntry, providerMatchesRegistryTransport } from "../providers/registry"; +import { effectiveGoogleMode, getProviderRegistryEntry, mergeRegistryStaticHeaders, providerMatchesRegistryTransport } from "../providers/registry"; import { resolveProviderModelDiscoveryUrl } from "../providers/model-discovery"; import { resolveProviderTransport } from "../providers/xai-transport"; import { detectClaudeCodeToken, detectGrokCliToken, hasComparableGrokIdentity, isSameGrokIdentity, shouldAdoptGrokGeneration } from "./local-token-detect"; @@ -828,7 +828,16 @@ export function buildModelsRequest( undefined, copilotApiBaseUrl, ); - const headers: Record = { ...(effectiveProvider.headers ?? {}) }; + // Model discovery is an upstream request like any other, so it carries the same registry + // static headers the inference path does. Without this a provider is identified correctly + // when it answers a completion but anonymously when it lists its own models, which is the + // kind of split fingerprint an upstream rate limiter reads as two different clients. + const registryStaticHeaders = providerMatchesRegistryTransport(providerName, effectiveProvider) + ? getProviderRegistryEntry(providerName)?.staticHeaders + : undefined; + const headers: Record = { + ...(mergeRegistryStaticHeaders(registryStaticHeaders, effectiveProvider.headers) ?? {}), + }; const discoveryUrl = (defaultUrl: string): string => resolveProviderModelDiscoveryUrl( providerName, prov, diff --git a/src/providers/registry.ts b/src/providers/registry.ts index 53922e75ad..de20cfa969 100644 --- a/src/providers/registry.ts +++ b/src/providers/registry.ts @@ -2435,6 +2435,16 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ note: "No key needed — public desktop tier. OpenCode currently advertises about 200 Big Pickle/free-model requests per 5 hours. The same Zen gateway can also short-window rate-limit free models at roughly 15-20 requests/minute, and may return generic 429s without Retry-After (opencodex synthesizes backoff only when that header is omitted). Free models are discovered live from Zen. Data use: per OpenCode's Zen docs (https://opencode.ai/docs/zen/), prompts sent to free models may be retained and used for training/improvement — do not send confidential material through this provider.", dashboardUrl: "https://opencode.ai", staticHeaders: { + // Zen answers a bare runtime User-Agent (Bun/x.y.z) more aggressively than a client + // that identifies itself, which is what the 429 in #2067 traced to. The value is + // deliberately unversioned: a pinned "opencode-cli/" is a claim about an + // install we do not have and goes stale on the vendor's schedule, not ours. + // Corroboration, not authority: OmniRoute — an independent open-source broker against + // the same Zen upstream — defaults to exactly this pair (userAgent "opencode", client + // "desktop") in open-sse/executors/opencode.ts, and got there by RETREATING from its + // own earlier "opencode-cli/1.0.0" pin. An operator can still override either value + // through the provider headers API; user headers win case-insensitively at route time. + "User-Agent": "opencode", "x-opencode-client": "desktop", }, modelReasoningEfforts: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)])), @@ -2591,6 +2601,36 @@ export function getProviderRegistryEntry(id: string): ProviderRegistryEntry | un return PROVIDER_REGISTRY.find(entry => entry.id === id); } +/** + * Merge a registry row's `staticHeaders` beneath a provider's own headers. + * + * The field is documented as "merged into every upstream request for this provider", but that + * was only ever true for a freshly seeded config: `providerConfigSeed` copies the block once + * (`derive.ts`), `enrichProviderFromCatalog` fills it only when the whole block is absent, and + * nothing merged it at request time. So an install that predates a header — or that saved any + * header of its own — never received the new one, which is exactly what #2067 would have + * shipped for every existing opencode-free user. + * + * The comparison is case-insensitive on purpose. HTTP header names are case-insensitive, but a + * plain object spread is not: merging a registry `User-Agent` over a user's `user-agent` + * produces two entries that `Headers` serializes as one comma-joined value + * ("opencode, custom-agent"), which is a corrupted request rather than an override. The user's + * spelling and value both win; the registry only fills names the user has not spoken for. + */ +export function mergeRegistryStaticHeaders( + staticHeaders: Record | undefined, + userHeaders: Record | undefined, +): Record | undefined { + if (!staticHeaders) return userHeaders; + if (!userHeaders) return { ...staticHeaders }; + const claimed = new Set(Object.keys(userHeaders).map(name => name.toLowerCase())); + const merged: Record = { ...userHeaders }; + for (const [name, value] of Object.entries(staticHeaders)) { + if (!claimed.has(name.toLowerCase())) merged[name] = value; + } + return merged; +} + /** Whether this registry row's per-model service-tier evidence applies to one configured target. */ export function registryModelServiceTierCapabilityApplies( entry: Pick, diff --git a/src/router.ts b/src/router.ts index 297795180e..d4839e24e0 100644 --- a/src/router.ts +++ b/src/router.ts @@ -14,6 +14,7 @@ import { assertProviderDestinationAllowed } from "./lib/destination-policy"; import { redactSecretString, redactUrlForLog } from "./lib/redact"; import { PROVIDER_REGISTRY, + mergeRegistryStaticHeaders, providerCodexAccountMode, registryModelServiceTierCapabilityApplies, } from "./providers/registry"; @@ -296,6 +297,11 @@ export function routedProviderConfig(providerName: string, provider: OcxProvider ? mergePositiveNumberCaps(registryEntry.modelContextWindows, provider.modelContextWindows) : mergeRecordFill(registryEntry.modelContextWindows, provider.modelContextWindows); const modelInputModalities = mergeRecordFill(registryEntry.modelInputModalities, provider.modelInputModalities); + // Registry static headers are documented as applying to every upstream request, so they are + // filled at resolve time rather than only at seed time: a config written before a header + // existed, or one carrying any header of its own, would otherwise never receive it. User + // headers win, matched case-insensitively so an override replaces rather than duplicates. + const headers = mergeRegistryStaticHeaders(registryEntry.staticHeaders, provider.headers); const modelMaxInputTokens = providerName === OPENAI_API_PROVIDER_ID ? mergePositiveNumberCaps(registryEntry.modelMaxInputTokens, provider.modelMaxInputTokens) : mergeRecordFill(registryEntry.modelMaxInputTokens, provider.modelMaxInputTokens); @@ -372,6 +378,7 @@ export function routedProviderConfig(providerName: string, provider: OcxProvider authMode: canonicalAuthMode, apiKey: resolvedApiKey, ...(staticModelCatalog ? { liveModels: false } : {}), + ...(headers ? { headers } : {}), // Backfill the Google wire mode + Vertex project/location from the registry when the user // config omits them, so a minimal `google-vertex`/`google-antigravity` entry still routes // through the correct branch (CCA/Vertex) instead of falling back to AI Studio. diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index fd79c750e9..600c040f41 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -2888,11 +2888,18 @@ describe("provider management validation", () => { }; // Clearing user-managed headers must not delete the registry-owned static - // metadata (opencode-free's x-opencode-client marker) the transport relies on. + // metadata (opencode-free's User-Agent and x-opencode-client markers) the transport + // relies on. expect((await patch("opencode-free", { headers: null }))?.status).toBe(200); - expect(liveConfig.providers["opencode-free"].headers).toEqual({ "x-opencode-client": "desktop" }); + expect(liveConfig.providers["opencode-free"].headers).toEqual({ + "User-Agent": "opencode", + "x-opencode-client": "desktop", + }); const saved = JSON.parse(readFileSync(join(TEST_DIR, "config.json"), "utf8")) as OcxConfig; - expect(saved.providers["opencode-free"]?.headers).toEqual({ "x-opencode-client": "desktop" }); + expect(saved.providers["opencode-free"]?.headers).toEqual({ + "User-Agent": "opencode", + "x-opencode-client": "desktop", + }); }); test("concurrent provider PATCHes merge different headers", async () => { if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); diff --git a/tests/opencode-free-provider.test.ts b/tests/opencode-free-provider.test.ts index 097fd98ef9..0db4aafb3e 100644 --- a/tests/opencode-free-provider.test.ts +++ b/tests/opencode-free-provider.test.ts @@ -2,6 +2,8 @@ import { describe, expect, test } from "bun:test"; import { PROVIDER_REGISTRY } from "../src/providers/registry"; import { providerConfigSeed, deriveKeyLoginMap, deriveFeaturedProviderIds } from "../src/providers/derive"; import { createOpenAIChatAdapter } from "../src/adapters/openai-chat"; +import { routedProviderConfig } from "../src/router"; +import { buildModelsRequest } from "../src/oauth"; import type { OcxParsedRequest, OcxProviderConfig } from "../src/types"; function minimalRequest(model = "kimi-k2.7-code"): OcxParsedRequest { @@ -27,14 +29,16 @@ describe("opencode-free provider", () => { expect(entry?.models).toBeUndefined(); }); - test("static headers include only the public client marker", () => { + test("static headers include only the public client markers", () => { expect(entry?.staticHeaders?.["Authorization"]).toBeUndefined(); + expect(entry?.staticHeaders?.["User-Agent"]).toBe("opencode"); expect(entry?.staticHeaders?.["x-opencode-client"]).toBe("desktop"); }); test("providerConfigSeed propagates static headers", () => { const seed = providerConfigSeed(entry!); expect(seed.headers?.["Authorization"]).toBeUndefined(); + expect(seed.headers?.["User-Agent"]).toBe("opencode"); expect(seed.headers?.["x-opencode-client"]).toBe("desktop"); expect(seed.keyOptional).toBe(true); expect(seed.liveModels).toBe(true); @@ -55,6 +59,7 @@ describe("opencode-free provider", () => { const req = adapter.buildRequest(minimalRequest()); const headers = req.headers as Record; expect(headers["Authorization"]).toBeUndefined(); + expect(headers["User-Agent"]).toBe("opencode"); expect(headers["x-opencode-client"]).toBe("desktop"); expect(req.url).toBe("https://opencode.ai/zen/v1/chat/completions"); }); @@ -84,6 +89,66 @@ describe("opencode-free provider", () => { expect(Object.keys(headers)).toContain("Authorization"); }); + // A seeded config is the easy case. The one that actually reaches users is a config written + // BEFORE a static header existed: it is on disk with the old header set (or with none at all, + // because the management API strips a block that exactly matches the registry), and nothing + // rewrites it. If the header only arrives at seed time, every existing install stays on the + // old fingerprint forever — which is what the original #2067 patch would have shipped. + describe("existing installs receive newly added static headers", () => { + const persisted = (headers?: Record): OcxProviderConfig => ({ + adapter: "openai-chat", + baseUrl: "https://opencode.ai/zen/v1", + keyOptional: true, + ...(headers ? { headers } : {}), + }); + + test("a config saved with no header block gains the full registry set", () => { + const routed = routedProviderConfig("opencode-free", persisted()); + expect(routed.headers?.["User-Agent"]).toBe("opencode"); + expect(routed.headers?.["x-opencode-client"]).toBe("desktop"); + }); + + test("a config saved with only the older marker gains the new one", () => { + const routed = routedProviderConfig("opencode-free", persisted({ "x-opencode-client": "desktop" })); + expect(routed.headers?.["User-Agent"]).toBe("opencode"); + expect(routed.headers?.["x-opencode-client"]).toBe("desktop"); + }); + + test("the merged headers reach the wire, not just the resolved config", () => { + const routed = routedProviderConfig("opencode-free", persisted({ "x-opencode-client": "desktop" })); + const req = createOpenAIChatAdapter(routed).buildRequest(minimalRequest()); + expect((req.headers as Record)["User-Agent"]).toBe("opencode"); + }); + + test("a user override wins and does not become a second comma-joined value", () => { + // HTTP header names are case-insensitive but object keys are not: a naive spread would + // leave both "user-agent" and "User-Agent", which `Headers` serializes as + // "custom-agent, opencode" — a corrupted request rather than an override. + const routed = routedProviderConfig("opencode-free", persisted({ "user-agent": "custom-agent" })); + const uaKeys = Object.keys(routed.headers ?? {}).filter(k => k.toLowerCase() === "user-agent"); + expect(uaKeys).toEqual(["user-agent"]); + expect(routed.headers?.["user-agent"]).toBe("custom-agent"); + expect(new Headers(routed.headers as Record).get("user-agent")).toBe("custom-agent"); + // Names the user did not claim are still filled. + expect(routed.headers?.["x-opencode-client"]).toBe("desktop"); + }); + + test("model discovery carries the same fingerprint as inference", () => { + // A provider identified as `opencode` when it completes but anonymous when it lists its + // own models reads as two different clients to an upstream rate limiter. + const req = buildModelsRequest(persisted({ "x-opencode-client": "desktop" }), undefined, "opencode-free"); + expect(req.headers["User-Agent"]).toBe("opencode"); + expect(req.headers["x-opencode-client"]).toBe("desktop"); + }); + + test("model discovery honors a user User-Agent override", () => { + const req = buildModelsRequest(persisted({ "user-agent": "custom-agent" }), undefined, "opencode-free"); + const uaKeys = Object.keys(req.headers).filter(k => k.toLowerCase() === "user-agent"); + expect(uaKeys).toEqual(["user-agent"]); + expect(req.headers["user-agent"]).toBe("custom-agent"); + }); + }); + test("provider note mentions no key needed", () => { expect(entry?.note?.toLowerCase()).toContain("no key needed"); expect(entry?.note?.toLowerCase()).toContain("200");