From d2d0aea74d8166bbb45d64dc83554db9eb87933b Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:10:10 +0900 Subject: [PATCH 1/6] fix(devin): send the configured output budget instead of the encoder default Codex never sends max_output_tokens, and the devin adapter only forwarded a caller-supplied value, so every devin turn was capped at the cloud-direct encoder's 8192 fallback however the provider was configured. A turn that legitimately needed more ended as an upstream incomplete/max_output_tokens the client then retried into the same deterministic wall. Resolve the cap the way the other adapters do, highest authority first: an explicit caller value forwarded unchanged, then the configured per-model cap, then the provider default, then the encoder fallback. The lookup is the same UID-aware hint resolution the input ceiling already uses, extracted so both sides read a per-model number identically. The output cap and the history ceiling stay separate. CompletionConfiguration #2 is the output cap and #3 is the context window, so the resolver reads neither contextWindow nor modelContextWindows -- collapsing them would ask Cognition to generate a whole context window of output. Also stop OAuth startup reconciliation from deleting an operator's output budget. No OAuth preset declares defaultMaxOutputTokens or modelMaxOutputTokens, so the delete-when-preset-undefined branch was the only branch either field ever took and a hand-edited value was wiped before the next startup finished. A preset that does declare one still refreshes the row. Closes #5190 --- scripts/test-layout/layout.json | 3 +- src/adapters/devin.ts | 95 +++++++--- src/oauth/index.ts | 25 ++- tests/fixtures/test-layout-expected.json | 3 +- tests/oauth/oauth-provider-reconcile.test.ts | 37 ++++ tests/providers/devin-output-budget.test.ts | 183 +++++++++++++++++++ 6 files changed, 320 insertions(+), 26 deletions(-) create mode 100644 tests/providers/devin-output-budget.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index ef1c68922bb..a06a473e92d 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1566,7 +1566,8 @@ "web-search-sidecar-429.test.ts": "web-search", "management-google-tool-schema-policy.test.ts": "server", "codex-shim-destroyed-probe.test.ts": "codex-integration", - "client-runtime.test.ts": "clients" + "client-runtime.test.ts": "clients", + "devin-output-budget.test.ts": "providers" }, "migrated": [ "adapters", diff --git a/src/adapters/devin.ts b/src/adapters/devin.ts index 5832b2be8b7..c1c67bdd409 100644 --- a/src/adapters/devin.ts +++ b/src/adapters/devin.ts @@ -192,6 +192,35 @@ async function resolveWireModelUid( */ export const resolveWireModelUidForTests = resolveWireModelUid; +const positiveTokenCount = (value: unknown): number | undefined => + typeof value === "number" && Number.isSafeInteger(value) && value > 0 ? value : undefined; + +/** + * Read a per-model token count for the exact UID selected for this turn. + * + * Tries the selected UID and then its collapsed base id, preferring the + * canonical spelling and accepting dotted or case-folded saved hints — the same + * normalization the inference request applies to the model id. Where several + * spellings match one id, the smallest wins: a ceiling stated twice is + * satisfied by the lower statement. + */ +function devinModelTokenHint( + record: Record | undefined, + modelUid: string, +): number | undefined { + if (!record) return undefined; + for (const id of [modelUid, collapseDevinModelUid(modelUid)]) { + const exact = Object.hasOwn(record, id) ? positiveTokenCount(record[id]) : undefined; + if (exact !== undefined) return exact; + const matches = Object.entries(record) + .filter(([key]) => normalizeDevinModelId(key).toLowerCase() === id.toLowerCase()) + .map(([, value]) => positiveTokenCount(value)) + .filter((value): value is number => value !== undefined); + if (matches.length > 0) return Math.min(...matches); + } + return undefined; +} + /** * Resolve the INPUT ceiling for the exact UID selected for this turn. Catalog * ClientModelConfig #18 and CompletionConfiguration #3 both carry input tokens; @@ -204,33 +233,50 @@ function resolveDevinMaxInputTokens( modelUid: string, liveWindow?: number, ): number | undefined { - const positive = (value: unknown): number | undefined => - typeof value === "number" && Number.isSafeInteger(value) && value > 0 ? value : undefined; - const baseId = collapseDevinModelUid(modelUid); - const configured = (record: Record | undefined): number | undefined => { - if (!record) return undefined; - for (const id of [modelUid, baseId]) { - // Prefer the canonical spelling; retain dotted/case-folded saved hints, - // matching the model-id normalization used for the inference request. - const exact = Object.hasOwn(record, id) ? positive(record[id]) : undefined; - if (exact !== undefined) return exact; - const matches = Object.entries(record) - .filter(([key]) => normalizeDevinModelId(key).toLowerCase() === id.toLowerCase()) - .map(([, value]) => positive(value)) - .filter((value): value is number => value !== undefined); - if (matches.length > 0) return Math.min(...matches); - } - return undefined; - }; - const contextHint = configured(provider.modelContextWindows) ?? positive(provider.contextWindow); - const inputHint = configured(provider.modelMaxInputTokens); - const ceilings = [positive(liveWindow), contextHint, inputHint] + const contextHint = devinModelTokenHint(provider.modelContextWindows, modelUid) + ?? positiveTokenCount(provider.contextWindow); + const inputHint = devinModelTokenHint(provider.modelMaxInputTokens, modelUid); + const ceilings = [positiveTokenCount(liveWindow), contextHint, inputHint] .filter((value): value is number => value !== undefined); return ceilings.length > 0 ? Math.min(...ceilings) : undefined; } -/** Pure test seam; runtime uses the same resolver immediately before dispatch. */ +/** + * Resolve the OUTPUT ceiling for this turn, highest authority first: + * + * 1. the caller's explicit `max_output_tokens`, forwarded unchanged — an + * explicit cap is a request, so a small one is never widened into a + * configured larger one; + * 2. the configured per-model cap (`modelMaxOutputTokens`), read through the + * same UID-aware hint lookup the input ceiling uses; + * 3. the provider-wide `defaultMaxOutputTokens`; + * 4. undefined, which leaves the cloud-direct encoder's own 8192 fallback in + * place for a provider that configured nothing. + * + * This is NOT the history ceiling, and the two must not collapse into one + * number. CompletionConfiguration #2 is the output cap and #3 is the context + * window, so feeding a context window into this resolver would ask Cognition to + * generate a whole window's worth of output. Nothing here reads + * `contextWindow` or `modelContextWindows` for that reason. + * + * Step 1 keeps the caller's raw value rather than `positiveTokenCount`: the + * inbound parser owns what a caller may send, and re-filtering here would + * silently promote a rejected value to a configured cap the caller never asked + * for. + */ +function resolveDevinMaxOutputTokens( + provider: OcxProviderConfig, + modelUid: string, + requested: number | undefined, +): number | undefined { + if (typeof requested === "number") return requested; + return devinModelTokenHint(provider.modelMaxOutputTokens, modelUid) + ?? positiveTokenCount(provider.defaultMaxOutputTokens); +} + +/** Pure test seams; runtime uses the same resolvers immediately before dispatch. */ export const resolveDevinMaxInputTokensForTests = resolveDevinMaxInputTokens; +export const resolveDevinMaxOutputTokensForTests = resolveDevinMaxOutputTokens; export class DevinMissingCredentialError extends Error { constructor() { @@ -590,6 +636,9 @@ export function createDevinAdapter( const maxInputTokens = resolveDevinMaxInputTokens( provider, modelUid, catalog?.byUid.get(modelUid)?.contextWindow, ); + const maxOutputTokens = resolveDevinMaxOutputTokens( + provider, modelUid, parsed.options.maxOutputTokens, + ); // The reset-retry wrapper waits out a 429 that states its own recovery // delay ("limit will reset in 35 seconds") and replays the identical // request — but only while zero events have been yielded, so a @@ -606,7 +655,7 @@ export function createDevinAdapter( // input hint used to force every model through the 128k default. completionOpts: { ...(maxInputTokens !== undefined ? { maxInputTokens } : {}), - ...(typeof parsed.options.maxOutputTokens === "number" ? { maxOutputTokens: parsed.options.maxOutputTokens } : {}), + ...(maxOutputTokens !== undefined ? { maxOutputTokens } : {}), ...(typeof parsed.options.temperature === "number" ? { temperature: parsed.options.temperature } : {}), ...(typeof parsed.options.topP === "number" ? { topP: parsed.options.topP } : {}), }, diff --git a/src/oauth/index.ts b/src/oauth/index.ts index f33a6855c0b..5d8e00ef472 100644 --- a/src/oauth/index.ts +++ b/src/oauth/index.ts @@ -1273,6 +1273,29 @@ const OAUTH_RECONCILE_FIELDS: (keyof OcxProviderConfig)[] = [ // existing rows through enrichProviderFromRegistry, which is fill-only and // preserves explicit saved values. +/** + * Output-budget fields an OAuth preset may refresh but must never erase. + * + * These stay on the reconcile list so a preset that does declare a budget still + * refreshes the saved row. What changes is the other branch: when the preset + * declares nothing, the operator's value survives instead of being deleted. + * + * Without that, the fields behaved as if they could not be configured at all. + * No OAuth preset seeds either one, so the delete branch was the only branch + * these two ever took, and a hand-edited `defaultMaxOutputTokens` was gone + * before the first turn of the next startup — leaving the adapter's own + * fallback as the only reachable output cap (#5190). + * + * Scoped to the output budget on purpose. The input side (`contextWindow`, + * `modelContextWindows`) describes what the account's models are, which the + * preset and live discovery do own; an output budget is a spend decision the + * operator makes. + */ +const OAUTH_PRESERVE_WHEN_PRESET_UNSET: ReadonlySet = new Set([ + "defaultMaxOutputTokens", + "modelMaxOutputTokens", +]); + const GOOGLE_ANTIGRAVITY_PROVIDER = "google-antigravity"; const GOOGLE_ANTIGRAVITY_LIVE_DISCOVERY_VERSION = 2 as const; @@ -1312,7 +1335,7 @@ function applyOAuthPresetCatalog( if (JSON.stringify(provider[field]) === JSON.stringify(preset[field])) continue; if (preset[field] !== undefined) { provider[field] = cloneProviderField(preset[field]) as never; - } else { + } else if (!OAUTH_PRESERVE_WHEN_PRESET_UNSET.has(field)) { delete provider[field]; } } diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index d8a39f76b8b..af2f2e699cc 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1398,5 +1398,6 @@ "gui-codex-usage-score-parity.test.ts": "gui", "web-search-sidecar-429.test.ts": "web-search", "codex-shim-destroyed-probe.test.ts": "codex-integration", - "client-runtime.test.ts": "clients" + "client-runtime.test.ts": "clients", + "devin-output-budget.test.ts": "providers" } diff --git a/tests/oauth/oauth-provider-reconcile.test.ts b/tests/oauth/oauth-provider-reconcile.test.ts index fe624a54f70..8e8a1b67afd 100644 --- a/tests/oauth/oauth-provider-reconcile.test.ts +++ b/tests/oauth/oauth-provider-reconcile.test.ts @@ -504,6 +504,43 @@ describe("OAuth provider reconciliation", () => { expect(config.providers.kimi.requiresReasoningPlaceholderModels).toEqual([]); }); + test("preserves an operator output budget that no OAuth preset declares", () => { + // The devin preset declares neither output field, so the delete branch was + // the only branch either of them ever took: a hand-edited budget was gone + // before the next startup finished, leaving the adapter's own fallback as + // the only reachable cap (#5190). Reconciliation still owns the input-side + // catalog, which is why modelContextWindows is asserted alongside. + const preset = OAUTH_PROVIDERS.devin.providerConfig; + expect(preset.defaultMaxOutputTokens).toBeUndefined(); + expect(preset.modelMaxOutputTokens).toBeUndefined(); + const config = { + port: 10100, + defaultProvider: "devin", + googleAntigravityStaticCatalogVersion: 1, + providers: { + devin: { + ...structuredClone(preset), + defaultMaxOutputTokens: 64_000, + modelMaxOutputTokens: { "swe-2": 32_000 }, + modelContextWindows: { "swe-2": 1 }, + }, + }, + } satisfies OcxConfig; + + reconcileOAuthProviders(config, false); + + expect(config.providers.devin.defaultMaxOutputTokens).toBe(64_000); + expect(config.providers.devin.modelMaxOutputTokens).toEqual({ "swe-2": 32_000 }); + expect(config.providers.devin.modelContextWindows).toEqual(preset.modelContextWindows!); + + // A preset that does declare a budget still refreshes the saved row: this + // is a narrower delete branch, not an exemption from reconciliation. + config.providers.devin.defaultMaxOutputTokens = 1; + reconcileOAuthProviders(config, false); + expect(config.providers.devin.defaultMaxOutputTokens).toBe(1); + expect(OAUTH_PROVIDERS.anthropic.providerConfig.defaultMaxOutputTokens).toBeGreaterThan(0); + }); + test("refreshes Grok 4.6 levels while runtime fills the default without overwriting user intent", () => { const home = mkdtempSync(join(tmpdir(), "ocx-grok-46-reconcile-")); homes.push(home); diff --git a/tests/providers/devin-output-budget.test.ts b/tests/providers/devin-output-budget.test.ts new file mode 100644 index 00000000000..ebd0b277259 --- /dev/null +++ b/tests/providers/devin-output-budget.test.ts @@ -0,0 +1,183 @@ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { createDevinAdapter, resolveDevinMaxOutputTokensForTests } from "../../src/adapters/devin"; +import { parseCatalogBuffer, setCachedCatalogForTests } from "../../src/adapters/devin/cloud-direct/catalog"; +import { devinCacheIdentity, invalidateSessionIdentity } from "../../src/adapters/devin/cloud-direct/chat"; +import { encodeMessage, encodeString, encodeVarintField, iterFields } from "../../src/adapters/devin/cloud-direct/wire"; +import { createTranslatorBudget } from "../../src/lib/translator-budget"; +import type { AdapterEvent, OcxParsedRequest, OcxProviderConfig } from "../../src/types"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +/** + * The output budget a Devin turn actually sends. + * + * CompletionConfiguration #2 is the output cap and #3 is the context window. + * Every assertion below reads both, because the defect these guard against is + * not "the number is wrong" but "the two meanings were collapsed into one": + * a caller that names no cap has to reach the configured output budget without + * the context window leaking into the field that decides how long the answer + * may run. + */ +describe("Devin output budget on the wire", () => { + const apiKey = "ocx-devin-output-fixture"; + const host = "https://server.codeium.com"; + const previousHome = process.env.OPENCODEX_HOME; + const previousFetch = globalThis.fetch; + let home = ""; + let requests: Buffer[] = []; + + function frame(body: Buffer, flags = 0): Buffer { + const header = Buffer.alloc(5); + header[0] = flags; + header.writeUInt32BE(body.length, 1); + return Buffer.concat([header, body]); + } + function fields(buf: Buffer) { + return new Map([...iterFields(buf)].map(field => [field.num, field])); + } + function seed(rows: Array<{ uid: string; window?: number }>): void { + const buffer = Buffer.concat(rows.map(row => encodeMessage(1, Buffer.concat([ + encodeString(1, row.uid), + encodeString(22, row.uid), + ...(row.window === undefined ? [] : [encodeVarintField(18, row.window)]), + encodeVarintField(4, 0), + ])))); + setCachedCatalogForTests(parseCatalogBuffer(buffer, apiKey, host)); + } + async function run( + provider: Partial = {}, + options: OcxParsedRequest["options"] = {}, + modelId = "swe-2-high", + ): Promise { + const adapter = createDevinAdapter({ ...provider, adapter: "devin", apiKey, baseUrl: host }); + const events: AdapterEvent[] = []; + await adapter.runTurn!({ + modelId, stream: true, + context: { messages: [{ role: "user", content: "hi", timestamp: 1 }] }, + options, + }, { headers: new Headers(), translatorBudget: createTranslatorBudget() }, + event => { events.push(event); }); + return events; + } + /** The completion configuration the one captured turn actually encoded. */ + function sentCompletion(): { output: bigint; context: bigint } { + expect(requests).toHaveLength(1); + const completion = fields(fields(requests[0]!).get(8)!.value as Buffer); + return { output: completion.get(2)!.value as bigint, context: completion.get(3)!.value as bigint }; + } + + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), "ocx-devin-output-")); + process.env.OPENCODEX_HOME = home; + requests = []; + setCachedCatalogForTests(null); + seed([{ uid: "swe-2-high", window: 262_000 }, { uid: "swe-2-max", window: 1_000_000 }]); + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (!url.endsWith("/GetChatMessage")) return new Response("unavailable", { status: 503 }); + requests.push(Buffer.from(await (init!.body as Blob).arrayBuffer()).subarray(5)); + return new Response(Buffer.concat([ + frame(Buffer.concat([encodeString(3, "ok"), encodeVarintField(5, 2)])), + frame(Buffer.from("{}"), 2), + ]), { headers: { "content-type": "application/connect+proto" } }); + }) as typeof fetch; + }); + afterEach(() => { + globalThis.fetch = previousFetch; + setCachedCatalogForTests(null); + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + invalidateSessionIdentity(devinCacheIdentity(apiKey, host)); + removeTreeWithRetry(home); + }); + + test("a caller that names no cap still reaches the configured provider budget", async () => { + // Codex never sends max_output_tokens, so this is the path every real turn + // takes. Before the budget was wired the encoder's 8192 was the only cap a + // devin turn could ever have, whatever the operator configured. + const events = await run({ defaultMaxOutputTokens: 64_000 }); + expect(events.some(event => event.type === "error")).toBe(false); + expect(sentCompletion().output).toBe(64_000n); + }); + + test("a per-model budget outranks the provider default and follows the selected variant", async () => { + await run({ + defaultMaxOutputTokens: 64_000, + modelMaxOutputTokens: { "swe-2-high": 32_000, "swe-2-max": 100_000 }, + }); + expect(sentCompletion().output).toBe(32_000n); + }); + + test("an explicit small cap survives a much larger configured budget", async () => { + await run( + { defaultMaxOutputTokens: 64_000, modelMaxOutputTokens: { "swe-2-high": 32_000 } }, + { maxOutputTokens: 64 }, + ); + expect(sentCompletion().output).toBe(64n); + }); + + test("the context window never becomes the output cap", async () => { + // The whole provider row describes input size and nothing else. The output + // field has to stay on the encoder default rather than inherit 262k, which + // would ask Cognition to generate an entire context window of tokens. + await run({ contextWindow: 200_000, modelContextWindows: { "swe-2-high": 180_000 } }); + const sent = sentCompletion(); + expect(sent.output).toBe(8192n); + expect(sent.context).toBe(180_000n); + }); + + test("a configured output budget does not disturb the input ceiling", async () => { + await run({ defaultMaxOutputTokens: 64_000 }); + expect(sentCompletion().context).toBe(262_000n); + }); +}); + +describe("Devin output budget resolution", () => { + const provider: Partial = { + adapter: "devin", + defaultMaxOutputTokens: 64_000, + modelMaxOutputTokens: { "swe-2-high": 32_000 }, + }; + + test("an explicit caller value is forwarded unchanged", () => { + expect(resolveDevinMaxOutputTokensForTests(provider as OcxProviderConfig, "swe-2-high", 64)).toBe(64); + }); + + test("an unconfigured provider leaves the encoder default in place", () => { + expect(resolveDevinMaxOutputTokensForTests({ adapter: "devin" } as OcxProviderConfig, "swe-2-high", undefined)) + .toBeUndefined(); + }); + + test("a dotted or case-folded saved hint still matches the selected uid", () => { + expect(resolveDevinMaxOutputTokensForTests( + { adapter: "devin", modelMaxOutputTokens: { "SWE.2-HIGH": 24_000 } } as OcxProviderConfig, + "swe-2-high", undefined, + )).toBe(24_000); + }); + + test("another variant's budget is never borrowed", () => { + expect(resolveDevinMaxOutputTokensForTests( + { adapter: "devin", modelMaxOutputTokens: { "swe-2-max": 100_000 } } as OcxProviderConfig, + "swe-2-high", undefined, + )).toBeUndefined(); + }); + + test.each([Number.NaN, Number.POSITIVE_INFINITY, 0, -1, 1.5, Number.MAX_SAFE_INTEGER + 1])( + "invalid configured metadata %p is ignored rather than encoded", + invalid => { + expect(resolveDevinMaxOutputTokensForTests( + { adapter: "devin", defaultMaxOutputTokens: invalid, modelMaxOutputTokens: { "swe-2": invalid } } as OcxProviderConfig, + "swe-2-high", undefined, + )).toBeUndefined(); + }, + ); + + test("the context window is not an output budget", () => { + expect(resolveDevinMaxOutputTokensForTests( + { adapter: "devin", contextWindow: 200_000, modelContextWindows: { "swe-2-high": 180_000 } } as OcxProviderConfig, + "swe-2-high", undefined, + )).toBeUndefined(); + }); +}); From 793a72fb44453788146515fdc19c624f18e1830b Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:10:34 +0900 Subject: [PATCH 2/6] feat(coding-agent): derive the projected-history ceiling from the model context window The coding-agent CLIs replay the whole conversation each turn as one projected user message over stream-json stdin, and the projection had a flat 200k-character history ceiling: roughly 50k tokens of English or code, a fraction of what even a 128k-token model holds and far below the 1M-token families. Long sessions lost their earliest context at a bound unrelated to the model. Derive the ceiling from the declared model context window on the routed provider row -- modelContextWindows by model, then the provider-wide contextWindow -- at three characters per token, floored at the legacy 200k cap so small windows and missing metadata behave exactly as before, and capped by a 4M hard ceiling so runaway metadata cannot unbound stdin. The resolution lives once in the shared runCodingAgentTurn driver, so both CodeBuddy and Qoder turns get it and the family adapters are unchanged. This ceiling is a runaway-memory bound on replayed history, measured in characters. It is deliberately not the output budget: caller-side compaction remains the token authority, and nothing here decides how long a reply may run. Co-authored-by: mdwsk88 <924038395@qq.com> --- src/adapters/coding-agent/protocol.ts | 31 +++++++++++++++-- src/adapters/coding-agent/turn.ts | 12 +++++-- tests/providers/codebuddy-adapter.test.ts | 31 +++++++++++++++++ tests/providers/codebuddy-protocol.test.ts | 40 ++++++++++++++++++++++ 4 files changed, 109 insertions(+), 5 deletions(-) diff --git a/src/adapters/coding-agent/protocol.ts b/src/adapters/coding-agent/protocol.ts index 779e88d14f7..628341bb8d1 100644 --- a/src/adapters/coding-agent/protocol.ts +++ b/src/adapters/coding-agent/protocol.ts @@ -21,6 +21,30 @@ export const MAX_STREAM_LINE_BYTES = 8 * 1024 * 1024; export const MAX_STREAM_TOTAL_BYTES = 64 * 1024 * 1024; /** Hard ceiling on projected conversation history text (characters) to prevent runaway memory. */ export const MAX_PROJECTED_HISTORY_CHARS = 200_000; +/** + * Chars-per-token ratio for deriving the projected-history ceiling from the model context + * window. Sits between the English/code (~4 chars per token) and CJK (~1.5) extremes: the + * ceiling is a runaway-memory bound and a coarse guard against cutting history the window can + * hold, not a token accounting - the caller-side compaction line stays the token authority. + */ +const PROJECTED_HISTORY_CHARS_PER_TOKEN = 3; +/** Absolute ceiling on a window-derived history cap, so runaway metadata cannot unbound stdin. */ +const MAX_PROJECTED_HISTORY_DERIVED_CHARS = 4_000_000; + +/** + * Projected-history character ceiling for a turn, derived from the declared model context + * window. A missing or non-finite window keeps the legacy flat cap, and the derivation never + * lowers the cap below it: small windows change nothing, while large windows scale (a 1M-token + * model keeps 3M characters) until the hard ceiling. The flat 200k cap predates window + * metadata and cut long replays to roughly 50k-130k tokens of content regardless of the model. + */ +export function projectedHistoryCharLimit(contextWindowTokens: number | undefined): number { + if (typeof contextWindowTokens !== "number" || !Number.isFinite(contextWindowTokens) || contextWindowTokens <= 0) { + return MAX_PROJECTED_HISTORY_CHARS; + } + const derived = contextWindowTokens * PROJECTED_HISTORY_CHARS_PER_TOKEN; + return Math.min(Math.max(derived, MAX_PROJECTED_HISTORY_CHARS), MAX_PROJECTED_HISTORY_DERIVED_CHARS); +} export class CodingAgentStreamLimitError extends Error { constructor(message: string) { @@ -395,7 +419,7 @@ export function buildSystemPrompt(parsed: OcxParsedRequest): string | undefined * prior conversation turns are structured as bounded context text with tool results as text, * clearly demarcated from the current user request. Codex retains tool control; vendor tools are never invoked. */ -export function buildConversationInput(parsed: OcxParsedRequest): string[] { +export function buildConversationInput(parsed: OcxParsedRequest, options: { maxHistoryChars?: number } = {}): string[] { const nonDev = parsed.context.messages.filter(m => m.role !== "developer"); if (nonDev.length === 0) { return [JSON.stringify({ type: "user", message: { role: "user", content: [{ type: "text", text: "" }] } })]; @@ -475,10 +499,11 @@ export function buildConversationInput(parsed: OcxParsedRequest): string[] { const imageBlocks: WireContentPart[] = [...historyImageBlocks, ...currentImageBlocks]; + const maxHistoryChars = options.maxHistoryChars ?? MAX_PROJECTED_HISTORY_CHARS; let historyText = historyMessages.map(formatMessageForHistory).filter(Boolean).join("\n\n"); - if (historyText.length > MAX_PROJECTED_HISTORY_CHARS) { + if (historyText.length > maxHistoryChars) { historyText = `[Earlier conversation history truncated for length...]\n\n` + - historyText.slice(historyText.length - MAX_PROJECTED_HISTORY_CHARS); + historyText.slice(historyText.length - maxHistoryChars); } const combinedText = `Prior conversation context:\n\n${historyText}\n\nCurrent user request:\n\n${currentRequestText}`; diff --git a/src/adapters/coding-agent/turn.ts b/src/adapters/coding-agent/turn.ts index a264ce188fa..f13b1ff0ea3 100644 --- a/src/adapters/coding-agent/turn.ts +++ b/src/adapters/coding-agent/turn.ts @@ -1,8 +1,9 @@ import { execFileSync, spawn as nodeSpawn, type ChildProcess, type SpawnOptions } from "node:child_process"; import type { AdapterEvent, OcxParsedRequest, OcxProviderConfig } from "../../types"; import { commandInvocation } from "../../lib/win-exec"; +import { modelRecordValue } from "../../reasoning-effort"; import type { IncomingMeta } from "../base"; -import { buildConversationInput, CodingAgentProtocolError, mapStreamMessageToEvents, readJsonLines, type StreamParseState } from "./protocol"; +import { buildConversationInput, CodingAgentProtocolError, mapStreamMessageToEvents, projectedHistoryCharLimit, readJsonLines, type StreamParseState } from "./protocol"; import { resolveCodingAgentBinary, resolveProfileByBaseUrl, type CodingAgentProviderProfile, type WhichFn } from "./profile"; /** Injectable spawn for tests; production uses node:child_process. */ @@ -264,7 +265,14 @@ export async function runCodingAgentTurn(input: CodingAgentTurnInput): Promise { /* EPIPE if the CLI exits early; surfaced via close/stderr */ }); - for (const line of buildConversationInput(parsed)) stdin.write(`${line}\n`); + // The projected history scales with the model context window on the routed provider row + // (catalog and config metadata merged): a 1M-token model keeps 3M characters of replay + // where the flat cap cut it near 50k-130k tokens of content. Absent metadata keeps the + // flat cap. + const historyCharLimit = projectedHistoryCharLimit( + modelRecordValue(provider.modelContextWindows, parsed.modelId) ?? provider.contextWindow, + ); + for (const line of buildConversationInput(parsed, { maxHistoryChars: historyCharLimit })) stdin.write(`${line}\n`); stdin.end(); } const stdout = child.stdout; diff --git a/tests/providers/codebuddy-adapter.test.ts b/tests/providers/codebuddy-adapter.test.ts index ea877f4a3eb..f68322752c5 100644 --- a/tests/providers/codebuddy-adapter.test.ts +++ b/tests/providers/codebuddy-adapter.test.ts @@ -252,6 +252,37 @@ describe("codebuddy runTurn streams a headless turn", () => { expect(JSON.stringify(events)).not.toContain("secret-command"); }); + test("the projected history ceiling follows the model context window", async () => { + const stdout = [ + enc.encode('{"type":"system","subtype":"init"}\n'), + enc.encode('{"type":"result","subtype":"success","is_error":false,"usage":{"input_tokens":7,"output_tokens":2}}\n'), + ]; + const history = Array.from({ length: 5 }, (_, index) => ({ + role: "user" as const, + content: "EARLY-MARKER-" + String(index) + " " + "a".repeat(50_000), + timestamp: index, + })); + const messages = [...history, { role: "user", content: "final request", timestamp: 5 }]; + + const wide = fakeChild(stdout); + const wideAdapter = createCodeBuddyAdapter( + provider({ modelContextWindows: { "glm-5.3": 1_000_000 } }), + { spawn: () => wide as unknown as ChildProcess, which: () => "/usr/bin/codebuddy", killGraceMs: 20 }, + ); + await run(wideAdapter, parsed({ context: { messages } })); + expect(wide.written.join("")).toContain("EARLY-MARKER-0"); + expect(wide.written.join("")).not.toContain("truncated for length"); + + const flat = fakeChild(stdout); + const flatAdapter = createCodeBuddyAdapter( + provider(), + { spawn: () => flat as unknown as ChildProcess, which: () => "/usr/bin/codebuddy", killGraceMs: 20 }, + ); + await run(flatAdapter, parsed({ context: { messages } })); + expect(flat.written.join("")).not.toContain("EARLY-MARKER-0"); + expect(flat.written.join("")).toContain("truncated for length"); + }); + test.each(["Bash", "exec", "shell", "apply_patch"])("refuses a bare %s DSML invoke", name => { const events: AdapterEvent[] = []; const guarded = guardCodeBuddyScaffolding(event => events.push(event)); diff --git a/tests/providers/codebuddy-protocol.test.ts b/tests/providers/codebuddy-protocol.test.ts index a51cb1ea7b4..5c3b4c8db29 100644 --- a/tests/providers/codebuddy-protocol.test.ts +++ b/tests/providers/codebuddy-protocol.test.ts @@ -1,9 +1,11 @@ import { describe, expect, test } from "bun:test"; import { + MAX_PROJECTED_HISTORY_CHARS, buildConversationInput, buildInputLines, buildSystemPrompt, mapStreamMessageToEvents, + projectedHistoryCharLimit, readJsonLines, usageFromResult, } from "../../src/adapters/coding-agent/protocol"; @@ -352,3 +354,41 @@ describe("codebuddy conversation input builder (Strategy C projection)", () => { }); }); }); +describe("projected history ceiling derives from the model context window", () => { + test("absent or invalid window metadata keeps the legacy flat cap", () => { + expect(projectedHistoryCharLimit(undefined)).toBe(MAX_PROJECTED_HISTORY_CHARS); + expect(projectedHistoryCharLimit(Number.NaN)).toBe(MAX_PROJECTED_HISTORY_CHARS); + expect(projectedHistoryCharLimit(0)).toBe(MAX_PROJECTED_HISTORY_CHARS); + expect(projectedHistoryCharLimit(-1)).toBe(MAX_PROJECTED_HISTORY_CHARS); + }); + + test("a small window never lowers the cap below the legacy default", () => { + expect(projectedHistoryCharLimit(64_000)).toBe(MAX_PROJECTED_HISTORY_CHARS); + }); + + test("a large window scales the cap until the hard ceiling", () => { + expect(projectedHistoryCharLimit(128_000)).toBe(384_000); + expect(projectedHistoryCharLimit(1_000_000)).toBe(3_000_000); + expect(projectedHistoryCharLimit(Number.MAX_SAFE_INTEGER)).toBe(4_000_000); + }); + + test("buildConversationInput keeps the history a derived ceiling admits", () => { + const history = Array.from({ length: 5 }, (_, index) => ({ + role: "user" as const, + content: "EARLY-MARKER-" + String(index) + " " + "a".repeat(50_000), + timestamp: index, + })); + const messages = [...history, { role: "user", content: "current request", timestamp: 5 }]; + const parsed = parsedRequest({ context: { messages } }); + + const wide = buildConversationInput(parsed, { maxHistoryChars: projectedHistoryCharLimit(1_000_000) }) + .map(line => JSON.parse(line)); + expect(wide[0].message.content[0].text).toContain("EARLY-MARKER-0"); + expect(wide[0].message.content[0].text).not.toContain("truncated for length"); + + const flat = buildConversationInput(parsed).map(line => JSON.parse(line)); + expect(flat[0].message.content[0].text).not.toContain("EARLY-MARKER-0"); + expect(flat[0].message.content[0].text).toContain("truncated for length"); + expect(flat[0].message.content[0].text).toContain("current request"); + }); +}); From a233fe469eed496358eb0bdacc234d59786a09d2 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:10:49 +0900 Subject: [PATCH 3/6] fix(adapters): bound the event queue by retained payload, not by event count alone The adapter event queue capped how many events it buffered but never how much those events held. Coalescing merges adjacent deltas up to 64 KiB an item, so the old 1024-event cap admitted about 64 MiB of retained text before it said anything, and a single oversized event was unbounded on its own. Charge what the queue actually retains, against two separate budgets. The aggregate budget bounds everything held at one moment; the per-event budget bounds one event and applies however empty the queue is. They describe different failures -- a consumer that is not keeping up versus an event that is malformed -- so they report different terminal messages and an operator can tell which happened. The accounting is exact on every path. Each queued item records what it was charged, so a merge pays only for the text it appends, a dequeue gives back precisely what it took, a refused event is priced before anything is retained and never charged, and the terminal record that explains a refusal is admitted past the budget it reports but still charged and released. Draining therefore returns the counter to zero after a normal turn, after an overflow abort and after a consumer walks away mid-stream; retainedCodeUnits() exposes that so a regression can assert it rather than infer it from an abort that happened to fire. A long healthy stream is still not capped by its total length: every dequeue releases its charge, so only an undrained backlog accumulates. The aggregate default is sized for the other legitimate case -- a synchronous producer that fills the queue before its consumer is scheduled, as the image loop does with over a million one-character deltas -- which is roughly 1.2 MB of retained text and must not abort. Retention is measured by walking own enumerable properties rather than by naming each variant's string fields, because a hand-written per-variant table is exhaustive over the AdapterEvent union and would silently stop counting a member added on another branch. The walk carries depth and node ceilings so one push stays cheap against the open provider-shaped payloads two members carry. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --- src/adapters/run-turn-queue.ts | 198 ++++++++++++++++++++++---- tests/adapters/run-turn-queue.test.ts | 174 +++++++++++++++++++++- 2 files changed, 342 insertions(+), 30 deletions(-) diff --git a/src/adapters/run-turn-queue.ts b/src/adapters/run-turn-queue.ts index 76024b32662..63e06033881 100644 --- a/src/adapters/run-turn-queue.ts +++ b/src/adapters/run-turn-queue.ts @@ -6,11 +6,98 @@ export const PREFLIGHT_HEARTBEAT_RETAIN_LIMIT = 16; /** * Coalescing threshold for adjacent text/thinking deltas buffered with no - * waiting reader (UTF-16 code units). This is a merge-size ceiling, not a - * byte-memory cap: a single oversized incoming event stays one item. + * waiting reader (UTF-16 code units). This is a merge-size ceiling; the two + * retention budgets below are what bound memory. */ export const COALESCE_MAX_CHUNK_LENGTH = 64 * 1024; +/** + * Retained-string budget for the WHOLE queue, in UTF-16 code units. + * + * This bounds what the queue is holding at one moment, not how much a turn + * streams: every dequeue gives its charge back, so a long healthy stream with a + * consumer attached never accumulates and is never capped by total length. + * + * 32 MiB is deliberately far above any single legitimate burst. A synchronous + * producer can legally fill the queue before its consumer is scheduled — the + * image loop does exactly that with over a million one-character deltas, which + * coalesce into roughly 1.2 MB of retained text — so a budget near that size + * aborts healthy turns rather than stalled ones. The previous effective bound + * was the 1024-event cap times the 64 KiB merge ceiling, so 64 MiB; this halves + * it while leaving that legitimate burst an order of magnitude of headroom. + */ +export const DEFAULT_MAX_BACKLOG_CODE_UNITS = 32 * 1024 * 1024; + +/** + * Retained-string budget for ONE queued event, in UTF-16 code units. + * + * Separate from the aggregate on purpose, because the two describe different + * failures. Passing the aggregate means the consumer is not keeping up. Passing + * this one means a single event is malformed or unbounded, which stays true + * however empty the queue is, so it must be refused even with the whole + * aggregate free. They also report different terminal messages, so an operator + * reading the turn's error learns which happened. + */ +export const DEFAULT_MAX_EVENT_CODE_UNITS = 8 * 1024 * 1024; + +const BACKLOG_EXCEEDED_MESSAGE = "consumer stalled: adapter event backlog exceeded — turn aborted"; +const EVENT_TOO_LARGE_MESSAGE = "adapter event exceeds the single-event retained-string budget — turn aborted"; + +/** + * Bound on how far the retention measure walks into one event. AdapterEvent is + * a plain-data union, but two of its members carry open provider-shaped bags + * (`providerState`, `usage.rawUsage`) whose depth no type here controls. The + * ceilings keep a single push O(1)-ish rather than O(whatever an adapter + * attached), and under-counting a pathological object is the safe direction: + * the event-count cap still bounds how many of them can be retained. + */ +const RETENTION_MAX_DEPTH = 8; +const RETENTION_MAX_NODES = 4096; + +/** + * Retained UTF-16 code units carried by one event's string payload. + * + * Measured by walking own enumerable properties rather than by naming each + * variant's string fields: a hand-written per-variant table is exhaustive over + * a union, so adding an event type on one branch while a consumer lands on + * another produces a measure that silently stops counting the new payload. + * The walk is the derived answer and needs no update when the union grows. + * + * `type` is skipped because it is the discriminant, identical for every event + * of a kind and not payload anyone is buffering. + */ +export function retainedEventCodeUnits(event: AdapterEvent): number { + let total = 0; + let nodes = 0; + const seen = new Set(); + const visit = (value: unknown, depth: number): void => { + if (typeof value === "string") { + total += value.length; + return; + } + if (!value || typeof value !== "object" || depth >= RETENTION_MAX_DEPTH || seen.has(value)) return; + seen.add(value); + for (const nested of Object.values(value)) { + if (nodes++ >= RETENTION_MAX_NODES) return; + visit(nested, depth + 1); + } + }; + for (const [key, value] of Object.entries(event)) { + if (key === "type") continue; + if (nodes++ >= RETENTION_MAX_NODES) break; + visit(value, 1); + } + return total; +} + +function positiveBudget(value: number | undefined, fallback: number, name: string): number { + const resolved = value ?? fallback; + if (!Number.isSafeInteger(resolved) || resolved <= 0) { + throw new RangeError(`${name} must be a positive safe integer`); + } + return resolved; +} + export interface AdapterEventQueue { /** * Returns true when the event was merged into the buffered tail instead of @@ -22,6 +109,13 @@ export interface AdapterEventQueue { close(): void; stream(): AsyncIterable; collect(): Promise; + /** + * Retained string payload the queue is currently holding, in UTF-16 code + * units. Exposed so a caller — and a regression — can assert the counter + * returns to zero on every terminal path instead of inferring it from an + * abort that happened to fire. + */ + retainedCodeUnits(): number; } export interface AdapterEventPreflight { @@ -73,47 +167,74 @@ export async function preflightAdapterEvents( export function createAdapterEventQueue(opts?: { maxBacklog?: number; + maxBacklogCodeUnits?: number; + maxEventCodeUnits?: number; onBacklogExceeded?: () => void; }): AdapterEventQueue { const queued: AdapterEvent[] = []; + /** + * What each queued item was charged, in lockstep with `queued`. Releasing the + * recorded charge rather than re-measuring is what makes the accounting exact + * on every path: a merge, a terminal record admitted past the budget and a + * plain event all give back precisely what they took, so the counter cannot + * drift positive (a leak) or negative (a budget the next turn gets for free). + */ + const charged: number[] = []; const readers: QueueReader[] = []; const maxBacklog = opts?.maxBacklog ?? 1_024; + const maxBacklogCodeUnits = positiveBudget(opts?.maxBacklogCodeUnits, DEFAULT_MAX_BACKLOG_CODE_UNITS, "maxBacklogCodeUnits"); + const maxEventCodeUnits = positiveBudget(opts?.maxEventCodeUnits, DEFAULT_MAX_EVENT_CODE_UNITS, "maxEventCodeUnits"); + let retained = 0; let closed = false; // Merge an incoming delta into the buffered tail when no reader is waiting. - // The backlog cap counts events, not tokens, so a detached or briefly - // stalled consumer (e.g. a Codex app mid-reconnect whose disconnect Bun has - // not yet delivered) used to hit the cap within seconds of token-granular - // streaming and abort a healthy turn. Adjacent same-phase text deltas, - // adjacent thinking deltas, and consecutive heartbeats carry no ordering - // information between themselves, so merging them preserves every consumer - // contract while making the cap approximate buffered items again. + // The event cap counts events, not tokens, so a detached or briefly stalled + // consumer (e.g. a Codex app mid-reconnect whose disconnect Bun has not yet + // delivered) used to hit it within seconds of token-granular streaming and + // abort a healthy turn. Adjacent same-phase text deltas, adjacent thinking + // deltas, and consecutive heartbeats carry no ordering information between + // themselves, so merging them preserves every consumer contract while making + // the cap approximate buffered items again. // Pushed objects may be retained by adapters, so the tail is REPLACED with - // a fresh object — never mutated (alias safety). - const coalesceIntoTail = (event: AdapterEvent): boolean => { - const tail = queued[queued.length - 1]; - if (!tail) return false; + // a fresh object — never mutated (alias safety). Returning the replacement + // instead of installing it lets push price the merge before committing to it. + const planTailMerge = (tail: AdapterEvent, event: AdapterEvent): AdapterEvent | null => { if (event.type === "heartbeat") { - if (tail.type !== "heartbeat") return false; + if (tail.type !== "heartbeat") return null; // Heartbeats carry no ordering between themselves, but the replay-unsafe // marker is not ordering — it is a latch. Dropping the incoming event // would discard the only record that Cursor already performed a local // side effect, and preflight would then permit an OAuth replay of it. if (event.replayUnsafe === true && tail.replayUnsafe !== true) { - queued[queued.length - 1] = { type: "heartbeat", replayUnsafe: true }; + return { type: "heartbeat", replayUnsafe: true }; } - return true; + return tail; } if (event.type === "text_delta" && tail.type === "text_delta" && tail.phase === event.phase) { - if (tail.text.length + event.text.length > COALESCE_MAX_CHUNK_LENGTH) return false; - queued[queued.length - 1] = { type: "text_delta", text: tail.text + event.text, phase: tail.phase }; - return true; + if (tail.text.length + event.text.length > COALESCE_MAX_CHUNK_LENGTH) return null; + return { type: "text_delta", text: tail.text + event.text, phase: tail.phase }; } if (event.type === "thinking_delta" && tail.type === "thinking_delta") { - if (tail.thinking.length + event.thinking.length > COALESCE_MAX_CHUNK_LENGTH) return false; - queued[queued.length - 1] = { type: "thinking_delta", thinking: tail.thinking + event.thinking }; - return true; + if (tail.thinking.length + event.thinking.length > COALESCE_MAX_CHUNK_LENGTH) return null; + return { type: "thinking_delta", thinking: tail.thinking + event.thinking }; } + return null; + }; + + /** + * Record why the turn is ending and close. The terminal error is admitted + * past both budgets — refusing to retain the explanation of a refusal would + * leave the consumer with a silent truncation — but it is charged like any + * other item so the counter stays exact through the final drain. + */ + const abortWith = (message: string): false => { + opts?.onBacklogExceeded?.(); + const terminal: AdapterEvent = { type: "error", message }; + const cost = retainedEventCodeUnits(terminal); + queued.push(terminal); + charged.push(cost); + retained += cost; + close(); return false; }; @@ -121,17 +242,35 @@ export function createAdapterEventQueue(opts?: { if (closed) return false; const reader = readers.shift(); if (reader) { + // Handed straight to a waiting consumer, so the queue retains nothing and + // charges nothing. Neither budget applies to an event it never holds. reader({ done: false, value: event }); return false; } - if (coalesceIntoTail(event)) return true; - if (queued.length >= maxBacklog) { - opts?.onBacklogExceeded?.(); - queued.push({ type: "error", message: "consumer stalled: adapter event backlog exceeded — turn aborted" }); - close(); - return false; + const tail = queued[queued.length - 1]; + const merged = tail ? planTailMerge(tail, event) : null; + if (merged && tail) { + const replacement = retainedEventCodeUnits(merged); + if (replacement > maxEventCodeUnits) return abortWith(EVENT_TOO_LARGE_MESSAGE); + // Charge only what the backlog actually gains. A merge keeps the tail's + // own fields, so the incoming event's duplicated phase is never retained + // twice and an unchanged tail costs nothing at all. + const delta = replacement - charged[charged.length - 1]!; + if (delta > maxBacklogCodeUnits - retained) return abortWith(BACKLOG_EXCEEDED_MESSAGE); + queued[queued.length - 1] = merged; + charged[charged.length - 1] = replacement; + retained += delta; + return true; } + const cost = retainedEventCodeUnits(event); + if (cost > maxEventCodeUnits) return abortWith(EVENT_TOO_LARGE_MESSAGE); + // Both refusals are priced before anything is retained, so an event that is + // turned away is never charged for. + if (queued.length >= maxBacklog) return abortWith(BACKLOG_EXCEEDED_MESSAGE); + if (cost > maxBacklogCodeUnits - retained) return abortWith(BACKLOG_EXCEEDED_MESSAGE); queued.push(event); + charged.push(cost); + retained += cost; return false; }; @@ -147,6 +286,7 @@ export function createAdapterEventQueue(opts?: { while (true) { const next = queued.shift(); if (next) { + retained -= charged.shift() ?? 0; yield next; continue; } @@ -165,5 +305,5 @@ export function createAdapterEventQueue(opts?: { return events; }; - return { push, close, stream, collect }; + return { push, close, stream, collect, retainedCodeUnits: () => retained }; } diff --git a/tests/adapters/run-turn-queue.test.ts b/tests/adapters/run-turn-queue.test.ts index e983ab6a00a..99120d90389 100644 --- a/tests/adapters/run-turn-queue.test.ts +++ b/tests/adapters/run-turn-queue.test.ts @@ -1,5 +1,13 @@ import { describe, expect, test } from "bun:test"; -import { COALESCE_MAX_CHUNK_LENGTH, createAdapterEventQueue, PREFLIGHT_HEARTBEAT_RETAIN_LIMIT, preflightAdapterEvents } from "../../src/adapters/run-turn-queue"; +import { + COALESCE_MAX_CHUNK_LENGTH, + createAdapterEventQueue, + DEFAULT_MAX_BACKLOG_CODE_UNITS, + DEFAULT_MAX_EVENT_CODE_UNITS, + PREFLIGHT_HEARTBEAT_RETAIN_LIMIT, + preflightAdapterEvents, + retainedEventCodeUnits, +} from "../../src/adapters/run-turn-queue"; import type { AdapterEvent } from "../../src/types"; const text = (value: string): AdapterEvent => ({ type: "text_delta", text: value }); @@ -340,3 +348,167 @@ describe("run-turn adapter event preflight", () => { expect(cancelled).toBe(1); }); }); + +describe("run-turn adapter event queue retained-payload budgets", () => { + const BACKLOG_EXCEEDED = "consumer stalled: adapter event backlog exceeded — turn aborted"; + const EVENT_TOO_LARGE = "adapter event exceeds the single-event retained-string budget — turn aborted"; + + test("a stalled consumer is bounded by retained payload, not by the event count alone", async () => { + let backlogExceeded = 0; + const queue = createAdapterEventQueue({ + maxBacklogCodeUnits: 8, + maxEventCodeUnits: 8, + onBacklogExceeded: () => { backlogExceeded += 1; }, + }); + + // Each event on its own is within the per-event budget, so only the + // aggregate can be what refuses the second one. + expect(retainedEventCodeUnits(toolStart("0"))).toBeLessThanOrEqual(8); + queue.push(toolStart("0")); + queue.push(toolStart("1")); + + expect(backlogExceeded).toBe(1); + expect(await queue.collect()).toEqual([ + toolStart("0"), + { type: "error", message: BACKLOG_EXCEEDED }, + ]); + expect(queue.retainedCodeUnits()).toBe(0); + }); + + test("one oversized event is refused with its own cause even into an empty queue", async () => { + let backlogExceeded = 0; + const queue = createAdapterEventQueue({ + maxEventCodeUnits: 8, + onBacklogExceeded: () => { backlogExceeded += 1; }, + }); + + // The aggregate has its whole default budget free; this refusal is about + // the single event, and it has to say so rather than blame the consumer. + queue.push(text("x".repeat(9))); + + expect(backlogExceeded).toBe(1); + expect(await queue.collect()).toEqual([{ type: "error", message: EVENT_TOO_LARGE }]); + expect(queue.retainedCodeUnits()).toBe(0); + }); + + test("accumulated coalescing is charged by retained growth, not once per merged delta", async () => { + let backlogExceeded = 0; + const queue = createAdapterEventQueue({ + maxBacklogCodeUnits: 16, + onBacklogExceeded: () => { backlogExceeded += 1; }, + }); + + // Three 2-character deltas sharing one 10-character phase. Retained growth + // is 12 then +2 then +2; charging each whole event instead would bill 36 + // and abort a turn holding sixteen code units. + queue.push(phasedText("ab", "commentary")); + queue.push(phasedText("cd", "commentary")); + queue.push(phasedText("ef", "commentary")); + + expect(backlogExceeded).toBe(0); + expect(queue.retainedCodeUnits()).toBe(16); + queue.close(); + expect(await queue.collect()).toEqual([phasedText("abcdef", "commentary")]); + expect(queue.retainedCodeUnits()).toBe(0); + }); + + test("an event handed straight to a waiting consumer is never charged or capped", async () => { + let backlogExceeded = 0; + const queue = createAdapterEventQueue({ + maxBacklogCodeUnits: 4, + maxEventCodeUnits: 4, + onBacklogExceeded: () => { backlogExceeded += 1; }, + }); + const iterator = queue.stream()[Symbol.asyncIterator](); + + const pending = iterator.next(); + queue.push(text("x".repeat(1_000))); + + // The queue never held it, so neither budget has anything to say about it. + expect(await pending).toEqual({ done: false, value: text("x".repeat(1_000)) }); + expect(backlogExceeded).toBe(0); + expect(queue.retainedCodeUnits()).toBe(0); + queue.close(); + }); + + test("a long synchronous burst well past one mebibyte still completes", async () => { + let backlogExceeded = 0; + const queue = createAdapterEventQueue({ + onBacklogExceeded: () => { backlogExceeded += 1; }, + }); + + // A synchronous producer legally fills the queue before its consumer is + // scheduled — the image loop does this with over a million one-character + // deltas, which coalesce into more than a mebibyte of retained text. A + // retained budget sized near that burst aborts healthy turns, so the + // default has to sit well above it. + const chunk = "x".repeat(64); + for (let i = 0; i < 20_000; i++) queue.push(text(chunk)); + queue.close(); + + const collected = await queue.collect(); + expect(backlogExceeded).toBe(0); + expect(collected.map(event => (event.type === "text_delta" ? event.text.length : 0)) + .reduce((sum, length) => sum + length, 0)).toBe(20_000 * 64); + expect(collected.every(event => event.type === "text_delta")).toBe(true); + expect(queue.retainedCodeUnits()).toBe(0); + }); + + test("draining after an abort releases exactly what was charged", async () => { + const queue = createAdapterEventQueue({ maxBacklogCodeUnits: 8, maxEventCodeUnits: 8 }); + + queue.push(toolStart("0")); + expect(queue.retainedCodeUnits()).toBeGreaterThan(0); + queue.push(toolStart("1")); + + // The terminal record is admitted past the budget it reports, and is then + // charged and released like any other item, so the counter lands on zero + // rather than on the size of an explanation nobody paid for. + const iterator = queue.stream()[Symbol.asyncIterator](); + expect(await iterator.next()).toEqual({ done: false, value: toolStart("0") }); + expect(await iterator.next()).toEqual({ done: false, value: { type: "error", message: BACKLOG_EXCEEDED } }); + expect(await iterator.next()).toEqual({ done: true, value: undefined }); + expect(queue.retainedCodeUnits()).toBe(0); + }); + + test("a cancel race leaves nothing charged behind", async () => { + const queue = createAdapterEventQueue({ maxBacklogCodeUnits: 64 }); + const iterator = queue.stream()[Symbol.asyncIterator](); + + queue.push(text("abcd")); + queue.push(thinking("wxyz")); + expect(await iterator.next()).toEqual({ done: false, value: text("abcd") }); + + // The consumer walks away mid-stream and the turn is closed underneath it. + await iterator.return?.(); + queue.close(); + queue.push(text("after close")); + + // What is still queued is still charged — and nothing more, so a second + // drain returns the counter to zero without a phantom balance. + expect(queue.retainedCodeUnits()).toBe(4); + expect(await queue.collect()).toEqual([thinking("wxyz")]); + expect(queue.retainedCodeUnits()).toBe(0); + }); + + test("the retention measure counts payload strings and skips the discriminant", () => { + expect(retainedEventCodeUnits(text("abcd"))).toBe(4); + expect(retainedEventCodeUnits(heartbeat)).toBe(0); + expect(retainedEventCodeUnits(phasedText("ab", "commentary"))).toBe(12); + // Nested provider-shaped payload is counted; a cycle terminates. + const cyclic: Record = { owner: "abc" }; + cyclic.self = cyclic; + expect(retainedEventCodeUnits({ type: "done", providerState: cyclic } as unknown as AdapterEvent)).toBe(3); + }); + + test("both budgets must be positive safe integers", () => { + const invalid = [Number.NaN, Number.POSITIVE_INFINITY, 0, -4, 2.5, Number.MAX_SAFE_INTEGER + 1]; + for (const value of invalid) { + expect(() => createAdapterEventQueue({ maxBacklogCodeUnits: value })) + .toThrow("maxBacklogCodeUnits must be a positive safe integer"); + expect(() => createAdapterEventQueue({ maxEventCodeUnits: value })) + .toThrow("maxEventCodeUnits must be a positive safe integer"); + } + expect(DEFAULT_MAX_EVENT_CODE_UNITS).toBeLessThan(DEFAULT_MAX_BACKLOG_CODE_UNITS); + }); +}); From 92a04963d232e1ec00d4c9333242880684bf36d1 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:22:45 +0900 Subject: [PATCH 4/6] feat(server): scope an admission key to the models and providers it may reach A hub serving several clients with their own data-plane keys had no way to stop a mail or cron key spending a coding key's Grok or Claude quota. Hub-level model selection cannot express it: it hides a model from everyone or from no one. An admission key may now declare allowedProviders and allowedModels. Absent or empty means unrestricted, which is every existing key, so nothing changes until an operator sets one. Two rules decide what this is. A scope names destinations, not selectors: it is evaluated against the resolved provider and model a turn will actually bill, never against the string the client sent. Alias resolution, a policy or combo selection, a subagent fallback and a compaction override all rewrite that string, so a scope checked at the front door would authorize one destination and reach another. And a scope is never management authority -- it narrows which models an inference key may call and grants nothing else. Enforcement sits where each route becomes concrete. On the Responses path that is the single point every produced route passes through, which covers the direct name, an alias, a policy or combo child, a shadow-intercept target, both subagent-fallback re-routes and, through translation, the Chat and Messages surfaces. The native Chat lane and the compaction route send without re-entering that path, so each applies the same predicate itself. A refusal is 403 with a stable model_not_allowed_for_key type naming the caller's own selector; the resolved destination stays in the server log, because a key that may not reach a provider has no business learning that its alias points there. /v1/models filters by the same predicate, so what a key can see and what it can call cannot diverge. That filter is a convenience and not the boundary: hiding a row only stops a client that reads the catalog first. A malformed scope drops the key rather than degrading to undefined, unlike every other field on the record. Degrading a damaged permission field reads as "allowed everything", which is the one direction it must never fail. Management exposes the lists on GET /api/keys and accepts them on PATCH, where rename and scope are independent edits, and ocx access key get/set reads and writes them without printing or rotating the secret. Closes #5049 --- scripts/test-layout/layout.json | 3 +- src/cli/access.ts | 87 +++++++++ src/config/schema/leaf-validators.ts | 7 + src/server/admission-model-scope.ts | 166 ++++++++++++++++++ src/server/chat-completions.ts | 15 ++ src/server/claude-messages.ts | 18 ++ src/server/index/serve-options.ts | 24 ++- src/server/management/oauth-account-routes.ts | 43 ++++- src/server/responses/compact.ts | 11 ++ src/server/responses/request-prepare.ts | 20 +++ src/types/config.ts | 15 ++ tests/fixtures/test-layout-expected.json | 3 +- tests/server/api-key-model-scope.test.ts | 124 +++++++++++++ 13 files changed, 527 insertions(+), 9 deletions(-) create mode 100644 src/server/admission-model-scope.ts create mode 100644 tests/server/api-key-model-scope.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index a06a473e92d..7522f69ddae 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1567,7 +1567,8 @@ "management-google-tool-schema-policy.test.ts": "server", "codex-shim-destroyed-probe.test.ts": "codex-integration", "client-runtime.test.ts": "clients", - "devin-output-budget.test.ts": "providers" + "devin-output-budget.test.ts": "providers", + "api-key-model-scope.test.ts": "server" }, "migrated": [ "adapters", diff --git a/src/cli/access.ts b/src/cli/access.ts index 51b351f9042..9cc0ec091a2 100644 --- a/src/cli/access.ts +++ b/src/cli/access.ts @@ -12,6 +12,8 @@ import { const USAGE = `Usage: ocx access key [list] [--json] ocx access key create [name] [--json] + ocx access key get [--json] + ocx access key set [--allow-provider ]... [--allow-model ]... [--clear] [--json] ocx access key rotate [--json] ocx access key rotate commit [--json] ocx access key rotate abort [--json] @@ -65,6 +67,51 @@ function formatKeyRows(payload: Record, keys: Array 0 ? [...lines, "", ...footer] : lines; } +/** + * Repeatable option values, in the order given. + * + * takeOption removes one occurrence, so a scope with several entries needs the + * loop: reading it once would silently keep only the first `--allow-model` and + * write a narrower scope than the operator typed. + */ +function takeAllOptions(args: string[], name: string): string[] { + const values: string[] = []; + for (;;) { + const value = takeOption(args, name); + if (value === undefined) break; + values.push(value); + } + return values; +} + +/** + * Find a key by id or by name, without ever reading the secret. + * + * The management API keys every mutation by id, so a name has to be resolved + * here. An ambiguous name is refused rather than resolved to the first match: + * silently scoping one of two keys that share a name is the kind of mistake + * only discovered when the wrong client stops working. + */ +function findKeyRow(keys: Array>, selector: string): Record { + const wanted = selector.trim().toLowerCase(); + const byId = keys.filter(entry => String(entry.id ?? "").toLowerCase() === wanted); + if (byId.length === 1) return byId[0]!; + const byName = keys.filter(entry => String(entry.name ?? "").trim().toLowerCase() === wanted); + if (byName.length === 1) return byName[0]!; + if (byName.length > 1) throw new CliUsageError("key name " + selector + " is ambiguous; use the id", USAGE); + throw new CliUsageError("no API key matches " + selector, USAGE); +} + +function scopeLines(entry: Record): string[] { + const list = (value: unknown): string => + Array.isArray(value) && value.length > 0 ? (value as string[]).join(", ") : "(any)"; + return [ + "API key " + String(entry.name ?? "") + " (" + String(entry.id ?? "") + ")", + " allowed providers: " + list(entry.allowedProviders), + " allowed models: " + list(entry.allowedModels), + ]; +} + async function key(argv: string[], deps: RuntimeApiDeps): Promise { const args = [...argv]; const action = (args.shift() ?? "list").toLowerCase(); @@ -90,6 +137,46 @@ async function key(argv: string[], deps: RuntimeApiDeps): Promise { ]); return; } + if (action === "get") { + const selector = args.shift(); + if (!selector) throw new CliUsageError("key id or name is required", USAGE); + rejectArgs(args, USAGE); + const result = await runtimeRequest>("/api/keys", {}, deps); + const entry = findKeyRow(Array.isArray(result.keys) ? result.keys as Array> : [], selector); + // The list response carries the masked prefix and never the secret, so the + // row is safe to print as-is under --json. + printData(entry, wantsJson, scopeLines(entry)); + return; + } + if (action === "set") { + const selector = args.shift(); + if (!selector) throw new CliUsageError("key id or name is required", USAGE); + const clear = takeFlag(args, "--clear"); + const providers = takeAllOptions(args, "--allow-provider"); + const models = takeAllOptions(args, "--allow-model"); + rejectArgs(args, USAGE); + if (!clear && providers.length === 0 && models.length === 0) { + throw new CliUsageError("set requires --allow-provider, --allow-model, or --clear", USAGE); + } + const listed = await runtimeRequest>("/api/keys", {}, deps); + const target = findKeyRow(Array.isArray(listed.keys) ? listed.keys as Array> : [], selector); + // A set REPLACES the named dimension rather than appending to it, and + // --clear removes both. Naming one dimension leaves the other alone, so + // narrowing providers cannot accidentally widen models. + const body: Record = { id: target.id }; + if (clear) { + body.allowedProviders = null; + body.allowedModels = null; + } + if (providers.length > 0) body.allowedProviders = providers; + if (models.length > 0) body.allowedModels = models; + const result = await runtimeRequest>("/api/keys", { + method: "PATCH", + body: JSON.stringify(body), + }, deps); + printData(result, wantsJson, scopeLines(result)); + return; + } if (action === "rotate") { const operation = args[0] === "commit" || args[0] === "abort" ? args.shift()! : "start"; const id = args.shift(); diff --git a/src/config/schema/leaf-validators.ts b/src/config/schema/leaf-validators.ts index f3160644b08..93edd25d0fc 100644 --- a/src/config/schema/leaf-validators.ts +++ b/src/config/schema/leaf-validators.ts @@ -649,6 +649,13 @@ export const apiKeyEntrySchema = z.object({ createdAt: z.string().catch(""), // A damaged overlap record must never discard the still-authoritative key. pendingRotation: pendingApiKeyRotationSchema.optional().catch(undefined), + // Deliberately NOT `.catch`ed, unlike every field above. Degrading a damaged + // scope to `undefined` would silently widen the key to the whole catalog, + // which is the one direction a permission field must never fail. Letting the + // record fail instead drops the key, so a corrupted scope stops that client + // rather than promoting it. + allowedProviders: z.array(z.string().trim().min(1).max(256)).optional(), + allowedModels: z.array(z.string().trim().min(1).max(256)).optional(), }).passthrough(); /** diff --git a/src/server/admission-model-scope.ts b/src/server/admission-model-scope.ts new file mode 100644 index 00000000000..4ffe9380827 --- /dev/null +++ b/src/server/admission-model-scope.ts @@ -0,0 +1,166 @@ +import type { DataPlaneAdmission } from "./auth-cors"; +import type { OcxApiKeyEntry, OcxConfig } from "../types"; + +/** + * Per-admission-key model and provider scope. + * + * A hub that serves several clients with their own `ocx_data_…` keys needs a + * mail or cron key to be unable to spend a coding key's Grok or Claude quota. + * Hub-level model selection cannot express that: it hides a model from + * everyone or from no one (#5049). + * + * Two rules decide what this is and is not: + * + * A scope names DESTINATIONS, not selectors. It is evaluated against the + * resolved route — the provider and model a turn will actually bill — never + * against the string the client sent. A client-supplied label is not a + * permission subject: alias resolution, a combo pick, a policy fallback and a + * compaction override all rewrite that string, so a scope checked before them + * would authorize one destination and reach another. + * + * A scope is never management authority. It narrows which models an inference + * key may call and grants nothing else; reading or editing a scope stays on + * the management credential. + */ +export interface AdmissionModelScope { + /** Resolved provider names this key may reach. Empty means every provider. */ + readonly providers: readonly string[]; + /** Resolved destinations this key may reach. Empty means every model. */ + readonly models: readonly string[]; +} + +/** The resolved destination a scope decision is made about. */ +export interface ScopedRoute { + readonly providerName: string; + readonly modelId: string; +} + +const normalize = (value: string): string => value.trim().toLowerCase(); + +function normalizedList(values: readonly string[] | undefined): readonly string[] { + if (!Array.isArray(values)) return []; + const seen = new Set(); + for (const value of values) { + if (typeof value !== "string") continue; + const normalized = normalize(value); + if (normalized) seen.add(normalized); + } + return [...seen]; +} + +/** Read the scope a stored key declares. An entry with neither list is unrestricted. */ +export function admissionModelScopeOf(entry: Pick): AdmissionModelScope | undefined { + const providers = normalizedList(entry.allowedProviders); + const models = normalizedList(entry.allowedModels); + return providers.length === 0 && models.length === 0 ? undefined : { providers, models }; +} + +/** + * The scope that applies to one request, or undefined when nothing is scoped. + * + * Only a configured key carries a scope. The environment token and loopback + * admission have no stored record to attach one to, so they stay unrestricted; + * an operator who wants them narrowed issues a configured key instead. + */ +export function resolveAdmissionModelScope( + config: Pick, + admission: DataPlaneAdmission | undefined, +): AdmissionModelScope | undefined { + if (!admission || admission.kind !== "configured") return undefined; + const entry = (config.apiKeys ?? []).find(key => key.id === admission.keyId); + return entry ? admissionModelScopeOf(entry) : undefined; +} + +/** + * Does this scope admit this resolved destination? + * + * The two lists are independent conditions and both must hold when both are + * declared: a key allowed one provider and one model may not reach that + * model on a different provider, which is what a combo child or a policy + * fallback would otherwise do while the requested selector stayed the same. + * + * A model entry matches the bare resolved model id or the fully qualified + * `provider/model` form, so an operator can scope one model everywhere or + * pin it to a single provider without a second field. + */ +export function routeAllowedByScope( + scope: AdmissionModelScope | undefined, + route: ScopedRoute, +): boolean { + if (!scope) return true; + const provider = normalize(route.providerName); + const model = normalize(route.modelId); + if (scope.providers.length > 0 && !scope.providers.includes(provider)) return false; + if (scope.models.length === 0) return true; + return scope.models.includes(model) || scope.models.includes(provider + "/" + model); +} + +/** + * A request that asked for a destination its key may not reach. + * + * Carries the selector the client sent rather than the destination it resolved + * to: the caller needs to know which of its own requests was refused, and a + * key that may not reach a provider has no business learning that an alias it + * named points there. The resolved destination goes to the server log. + */ +export class AdmissionModelDeniedError extends Error { + readonly requestedModel: string; + readonly deniedProvider: string; + readonly deniedModel: string; + constructor(requestedModel: string, route: ScopedRoute) { + super("model " + requestedModel + " is not allowed for this API key"); + this.name = "AdmissionModelDeniedError"; + this.requestedModel = requestedModel; + this.deniedProvider = route.providerName; + this.deniedModel = route.modelId; + } +} + +/** Stable wire type for a scope refusal. */ +export const MODEL_NOT_ALLOWED_FOR_KEY = "model_not_allowed_for_key"; + +/** The HTTP body a scope refusal returns. 403: authenticated, not permitted. */ +export function admissionModelDeniedBody(error: AdmissionModelDeniedError): { + error: { type: string; message: string; model: string }; +} { + return { + error: { + type: MODEL_NOT_ALLOWED_FOR_KEY, + message: error.message, + model: error.requestedModel, + }, + }; +} + +/** + * The refusal a scoped request gets: 403, not 404. + * + * The key authenticated; it simply may not reach this destination. Reporting + * "not found" instead would tell a client its credential is wrong and invite + * it to retry with another, and would make an operator's own denial + * indistinguishable from a typo in the model name. + */ +export function admissionModelDeniedResponse(error: AdmissionModelDeniedError): Response { + return new Response(JSON.stringify(admissionModelDeniedBody(error)), { + status: 403, + headers: { "Content-Type": "application/json" }, + }); +} + +/** + * Refuse a resolved destination this key may not reach. + * + * Every request-path site that produces or re-produces a route calls this, so + * the direct name, an alias, a combo child, a policy or subagent fallback and + * a compaction override are all checked at the point they become concrete + * rather than once at the front door. + */ +export function assertRouteAllowedByScope( + scope: AdmissionModelScope | undefined, + requestedModel: string, + route: ScopedRoute, +): void { + if (!routeAllowedByScope(scope, route)) { + throw new AdmissionModelDeniedError(requestedModel, route); + } +} diff --git a/src/server/chat-completions.ts b/src/server/chat-completions.ts index 8a611e70e4b..4f4209d11c0 100644 --- a/src/server/chat-completions.ts +++ b/src/server/chat-completions.ts @@ -46,6 +46,12 @@ import { providerConsumesCallerAuthorization } from "../providers/caller-authori import { captureExplicitOpenAiCallerAuth } from "../providers/openai-sidecar"; import { captureCallerDirectAuth } from "../providers/caller-authorization"; import type { AdmissionLease } from "../lib/admission"; +import { + admissionModelDeniedResponse, + AdmissionModelDeniedError, + assertRouteAllowedByScope, + resolveAdmissionModelScope, +} from "./admission-model-scope"; import type { DataPlaneAdmission } from "./auth-cors"; import { tryClaimNativeMainProfileForTurn } from "../codex/native-main-admission"; import { @@ -150,6 +156,10 @@ async function handleChatCompletionsWithBudget( let chatNativeRoute: ReturnType | null = null; try { const route = routeModel(config, chatBody.model as string, evidenceFromBody(chatBody)); + // The native Chat lane sends without re-entering the Responses path, so it + // has to apply the key's scope itself. Translated traffic is checked where + // every rewrite converges instead. + assertRouteAllowedByScope(resolveAdmissionModelScope(config, logIds?.admission), requestedModel, route); // Preserve the routed destination for Go recognition, then settle the wire before // deriving protocol-scoped affinity. Recognition must not inspect the flipped adapter. const routedProvider = route.provider; @@ -182,6 +192,11 @@ async function handleChatCompletionsWithBudget( // effort, failover, and per-attempt telemetry run before any native Chat send. if (!route.combo && !effortRow && isNativeChatRouteEligible(route, chatBody, config)) chatNativeRoute = route; } catch (err) { + if (err instanceof AdmissionModelDeniedError) { + logCtx.requestedModel = requestedModel; + if (logIds) addFinalRequestLog(logIds.requestId, logIds.start, logCtx, 403, { closeReason: "non_stream" }); + return admissionModelDeniedResponse(err); + } if (err instanceof UnknownRoutingPolicyError) { logCtx.requestedModel = requestedModel; if (logIds) addFinalRequestLog(logIds.requestId, logIds.start, logCtx, 404, { closeReason: "non_stream" }); diff --git a/src/server/claude-messages.ts b/src/server/claude-messages.ts index b73199dd15a..b27ec73c91e 100644 --- a/src/server/claude-messages.ts +++ b/src/server/claude-messages.ts @@ -7,6 +7,12 @@ * unchanged. The Responses output (SSE or JSON) is converted back to Anthropic shape. */ import { FORWARD_HEADERS } from "../adapters/openai-responses"; +import { + admissionModelDeniedResponse, + AdmissionModelDeniedError, + assertRouteAllowedByScope, + resolveAdmissionModelScope, +} from "./admission-model-scope"; import { jsonUtf8Bytes } from "../lib/json-byte-size"; import { sseFieldValue } from "../lib/sse-decoder"; import { enforceAnthropicImageLimits, sniffImageDimensions } from "../adapters/anthropic-image-guard"; @@ -815,6 +821,13 @@ async function handleClaudeMessagesWithBudget( // verified live 2026-07-11). Strip them for that route; routed providers keep them. try { const route = routeModel(config, internalBody.model as string, evidenceFromBody(internalBody)); + // Same reason as the native Chat lane: this route can be sent from here, so + // the key's scope is applied before the wire is settled. + assertRouteAllowedByScope( + resolveAdmissionModelScope(config, logIds?.admission), + String(internalBody.model ?? ""), + route, + ); // Settle the wire once so the sampling decision below reads the effective // adapter rather than the provider-wide default (#404). route.staticPolicy = captureRouteStaticPolicy( @@ -847,6 +860,11 @@ async function handleClaudeMessagesWithBudget( if (ladder !== undefined && ladder.length === 0) delete internalBody.reasoning; } } catch (err) { + if (err instanceof AdmissionModelDeniedError) { + logCtx.requestedModel = requestedModel; + if (logIds) addFinalRequestLog(logIds.requestId, logIds.start, logCtx, 403, { closeReason: "non_stream" }); + return admissionModelDeniedResponse(err); + } if (err instanceof UnknownRoutingPolicyError) { logCtx.requestedModel = requestedModel; if (logIds) addFinalRequestLog(logIds.requestId, logIds.start, logCtx, 404, { closeReason: "non_stream" }); diff --git a/src/server/index/serve-options.ts b/src/server/index/serve-options.ts index 657f5f0b5b9..6d3ed873339 100644 --- a/src/server/index/serve-options.ts +++ b/src/server/index/serve-options.ts @@ -100,6 +100,7 @@ import { withCors, withManagementCors, } from "../auth-cors"; +import { resolveAdmissionModelScope, routeAllowedByScope } from "../admission-model-scope"; import { disableResponsesRequestTimeout, handleResponses, @@ -1101,6 +1102,17 @@ export function createServeOptions(ctx: ServeOptionsContext) { return disabledModels.has(id) ? [] : [{ id, metadataId }]; }) ); + // What a scoped key may see, filtered by the same predicate that refuses + // it on the data plane, so the catalog and the send path cannot disagree. + // This is a convenience, never the boundary: hiding a row only stops a + // client that reads the catalog first, which is why the refusal lives on + // the request path and this filter reuses it rather than replacing it. + // Filtering happens where the resolved provider and model are still in + // hand -- a published id is a selector, and re-resolving one here would + // re-run combo selection just to render a list. + const listScope = resolveAdmissionModelScope(config, admission); + const listAllows = (providerName: string, modelId: string): boolean => + routeAllowedByScope(listScope, { providerName, modelId }); // The projection is opt-in. Keep the default path free of Cursor install detection, // and resolve the bundle table once for the whole list rather than once per row. const effortRowsEnabled = config.cursorEffortRows === true; @@ -1133,7 +1145,9 @@ export function createServeOptions(ctx: ServeOptionsContext) { effortRowKnownIds, )); }; - const routedRows = await Promise.all(uniqueCatalogModelsForRawPublicList(goOrdered).map(async m => { + const routedRows = await Promise.all(uniqueCatalogModelsForRawPublicList(goOrdered) + .filter(m => listAllows(m.provider, m.id)) + .map(async m => { // Same rule as the anthropic branch: with the global fast switch on, a client // that has no Fast toggle is offered the fast identity directly. An operator // alias is an explicit decision and still wins. @@ -1180,8 +1194,12 @@ export function createServeOptions(ctx: ServeOptionsContext) { )); })); const data = [ - ...visibleNatives.flatMap(id => expandedNativeModelRow(id)), - ...visibleAccountNatives.flatMap(({ id, metadataId }) => expandedNativeModelRow(id, metadataId)), + ...visibleNatives + .filter(id => listAllows(OPENAI_CODEX_PROVIDER_ID, id)) + .flatMap(id => expandedNativeModelRow(id)), + ...visibleAccountNatives + .filter(({ metadataId }) => listAllows(OPENAI_CODEX_PROVIDER_ID, metadataId)) + .flatMap(({ id, metadataId }) => expandedNativeModelRow(id, metadataId)), ...routedRows.flat(), ]; return jsonResponse({ object: "list", data }, 200, req, policy); diff --git a/src/server/management/oauth-account-routes.ts b/src/server/management/oauth-account-routes.ts index 4cd52004665..0cb29ff8acd 100644 --- a/src/server/management/oauth-account-routes.ts +++ b/src/server/management/oauth-account-routes.ts @@ -869,6 +869,10 @@ export async function handleOauthAccountRoutes(ctx: ManagementContext): Promise< name: k.name, prefix: k.key.slice(0, 17) + "...", createdAt: k.createdAt, + // Scope is metadata, not secret: an operator has to be able to read + // what a key may reach without minting a replacement to find out. + ...(k.allowedProviders ? { allowedProviders: [...k.allowedProviders] } : {}), + ...(k.allowedModels ? { allowedModels: [...k.allowedModels] } : {}), ...(k.pendingRotation ? { pendingRotation: { id: k.pendingRotation.id, createdAt: k.pendingRotation.createdAt, @@ -952,15 +956,46 @@ export async function handleOauthAccountRoutes(ctx: ManagementContext): Promise< const body = await readJsonBody(req); if (!body) return jsonResponse({ error: "invalid body" }, 400, req, config); if (typeof body.id !== "string" || !body.id) return jsonResponse({ error: "id required" }, 400, req, config); - const nameField = validateKeyName(body.name, { required: true }); - if ("error" in nameField) return jsonResponse({ error: nameField.error }, 400, req, config); const entry = (config.apiKeys ?? []).find(k => k.id === body.id); if (!entry) return jsonResponse({ error: "key not found" }, 404, req, config); - entry.name = nameField.value; + // Rename and scope are independent edits. A scope-only PATCH must not have + // to restate the name, and a rename must not silently widen a scope, so + // each field is applied only when the caller actually sent it. + const renaming = body.name !== undefined; + const scopingProviders = body.allowedProviders !== undefined; + const scopingModels = body.allowedModels !== undefined; + if (!renaming && !scopingProviders && !scopingModels) { + return jsonResponse({ error: "name, allowedProviders or allowedModels required" }, 400, req, config); + } + if (renaming) { + const nameField = validateKeyName(body.name, { required: true }); + if ("error" in nameField) return jsonResponse({ error: nameField.error }, 400, req, config); + entry.name = nameField.value; + } + for (const [field, sent] of [["allowedProviders", scopingProviders], ["allowedModels", scopingModels]] as const) { + if (!sent) continue; + const value = body[field]; + // `null` and `[]` both clear the list back to unrestricted; anything else + // must be a list of non-empty strings, because a silently ignored malformed + // scope would read as "allowed everything" to whoever set it. + if (value === null) { delete entry[field]; continue; } + if (!Array.isArray(value) || value.some(item => typeof item !== "string" || !item.trim() || item.length > 256)) { + return jsonResponse({ error: `${field} must be a list of non-empty names` }, 400, req, config); + } + const normalized = [...new Set((value as string[]).map(item => item.trim()))]; + if (normalized.length === 0) delete entry[field]; + else entry[field] = normalized; + } saveConfigPreservingClaudeCode(config); reconcileLiveStateStores(); // Never echo key material from a rename. - return jsonResponse({ id: entry.id, name: entry.name, createdAt: entry.createdAt }, 200, req, config); + return jsonResponse({ + id: entry.id, + name: entry.name, + createdAt: entry.createdAt, + ...(entry.allowedProviders ? { allowedProviders: [...entry.allowedProviders] } : {}), + ...(entry.allowedModels ? { allowedModels: [...entry.allowedModels] } : {}), + }, 200, req, config); } if (url.pathname === "/api/keys" && req.method === "DELETE") { diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 92e394751bc..89e40123789 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -1,4 +1,10 @@ import { capturePoolQuotaWriter } from "../../codex/account-store"; +import { + admissionModelDeniedResponse, + AdmissionModelDeniedError, + assertRouteAllowedByScope, + resolveAdmissionModelScope, +} from "../admission-model-scope"; import type { Server } from "bun"; import { bridgeToResponsesSSE, buildResponseJSON, formatErrorResponse, type ResponsesTerminalStatus } from "../../bridge"; import { @@ -655,7 +661,12 @@ export async function handleResponsesCompact( // routes ordinary turns elsewhere (#2901); the compaction-scoped router // may land that on the configured default provider instead of 404. route = routeCompactionModel(config, compactModel, evidenceFromBody(raw)); + // A compaction override picks the model, not the caller, so the key's scope + // is applied to what the override resolved to rather than to the selector + // the client sent. + assertRouteAllowedByScope(resolveAdmissionModelScope(config, admission), compactRequestedModel, route); } catch (err) { + if (err instanceof AdmissionModelDeniedError) return admissionModelDeniedResponse(err); if (err instanceof NoEligiblePolicyCandidateError) { // Persist the evaluation trace (per-candidate exclusions + the // no-eligible reason) so a failed compact policy request stays diff --git a/src/server/responses/request-prepare.ts b/src/server/responses/request-prepare.ts index 4d94d6a71ad..0aeb7a70f6c 100644 --- a/src/server/responses/request-prepare.ts +++ b/src/server/responses/request-prepare.ts @@ -110,6 +110,12 @@ import { CODEX_RESERVE_OPT_IN_REQUIRED_MESSAGE, } from "../../codex/loopback-target"; import { checkComboTargetInputAdmission, checkInputAdmission } from "./input-admission"; +import { + admissionModelDeniedResponse, + AdmissionModelDeniedError, + assertRouteAllowedByScope, + resolveAdmissionModelScope, +} from "../admission-model-scope"; import { nativeContextLimits } from "../../codex/catalog"; import { streamingContextOverflowResponse } from "./context-overflow"; import { @@ -411,7 +417,18 @@ export async function prepareResponsesRequest( let route: RouteResult; let credentialDomainWasRewritten = false; + // The selector the caller actually sent, captured before shadow interception + // or a subagent fallback rewrites it, so a refusal names the client's own + // request rather than a destination it never asked for. + const inboundSelector = parsed.modelId; + const admissionScope = resolveAdmissionModelScope(config, options.admission); const captureInboundRoutePolicy = (candidate: RouteResult): RouteResult => { + // Every route this request path produces passes through here: the direct + // name, an alias, a policy or combo selection, a compaction override, a + // shadow-intercept target and both subagent-fallback re-routes. Checking + // the key's scope at this one point is what stops a rewrite from reaching + // a destination the front door would have refused. + assertRouteAllowedByScope(admissionScope, inboundSelector, candidate); candidate.staticPolicy = captureRouteStaticPolicy( candidate.providerName, candidate.modelId, @@ -474,6 +491,7 @@ export async function prepareResponsesRequest( } logCtx.routeDecision = route.routeDecision; } catch (err) { + if (err instanceof AdmissionModelDeniedError) return admissionModelDeniedResponse(err); if (err instanceof NoAvailableComboTargetsError) { return comboUnavailable(err.comboId); } @@ -673,6 +691,7 @@ export async function prepareResponsesRequest( credentialDomainWasRewritten = true; logCtx.routeDecision = route.routeDecision; } catch (err) { + if (err instanceof AdmissionModelDeniedError) return admissionModelDeniedResponse(err); if (err instanceof NoAvailableComboTargetsError) { return comboUnavailable(err.comboId); } @@ -873,6 +892,7 @@ export async function prepareResponsesRequest( credentialDomainWasRewritten = true; logCtx.routeDecision = route.routeDecision; } catch (err) { + if (err instanceof AdmissionModelDeniedError) return admissionModelDeniedResponse(err); if (err instanceof NoAvailableComboTargetsError) { return comboUnavailable(err.comboId); } diff --git a/src/types/config.ts b/src/types/config.ts index 977bdb266f6..bd6959ce5ab 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -253,6 +253,21 @@ export interface OcxApiKeyEntry { key: string; createdAt: string; pendingRotation?: OcxPendingApiKeyRotation; + /** + * Resolved provider names this key may reach. Absent or empty means every + * provider, which is what every existing key has, so adding the field + * changes nothing until an operator sets one. + */ + allowedProviders?: string[]; + /** + * Resolved destinations this key may reach, as a bare model id or a + * `provider/model` pair. Absent or empty means every model. + * + * These name destinations, not the selectors a client sends: they are + * checked after alias, combo, fallback and compaction resolution, because + * that is the only point at which the model about to be billed is known. + */ + allowedModels?: string[]; } export interface OcxPendingApiKeyRotation { diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index af2f2e699cc..308682ccebc 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1399,5 +1399,6 @@ "web-search-sidecar-429.test.ts": "web-search", "codex-shim-destroyed-probe.test.ts": "codex-integration", "client-runtime.test.ts": "clients", - "devin-output-budget.test.ts": "providers" + "devin-output-budget.test.ts": "providers", + "api-key-model-scope.test.ts": "server" } diff --git a/tests/server/api-key-model-scope.test.ts b/tests/server/api-key-model-scope.test.ts new file mode 100644 index 00000000000..94c8a3fe0d9 --- /dev/null +++ b/tests/server/api-key-model-scope.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, test } from "bun:test"; +import { + admissionModelDeniedBody, + AdmissionModelDeniedError, + admissionModelDeniedResponse, + admissionModelScopeOf, + assertRouteAllowedByScope, + MODEL_NOT_ALLOWED_FOR_KEY, + resolveAdmissionModelScope, + routeAllowedByScope, +} from "../../src/server/admission-model-scope"; +import { apiKeyEntrySchema } from "../../src/config/schema/leaf-validators"; +import { routeModel } from "../../src/router"; +import type { OcxConfig } from "../../src/types"; + +const KEY = "ocx_data_" + "a".repeat(40); +const OTHER_KEY = "ocx_data_" + "b".repeat(40); + +function configWithKey(scope: { allowedProviders?: string[]; allowedModels?: string[] }): Pick { + return { + apiKeys: [ + { id: "scoped", name: "mail", key: KEY, createdAt: "2026-01-01T00:00:00.000Z", ...scope }, + { id: "open", name: "coding", key: OTHER_KEY, createdAt: "2026-01-01T00:00:00.000Z" }, + ], + }; +} + +describe("per-key model and provider scope", () => { + test("a key with neither list is unrestricted", () => { + expect(admissionModelScopeOf({})).toBeUndefined(); + expect(admissionModelScopeOf({ allowedProviders: [], allowedModels: [] })).toBeUndefined(); + expect(routeAllowedByScope(undefined, { providerName: "xai", modelId: "grok-4.6" })).toBe(true); + }); + + test("only a configured key carries a scope", () => { + const config = configWithKey({ allowedProviders: ["zai-discount"] }); + expect(resolveAdmissionModelScope(config, { kind: "configured", keyId: "scoped", source: "bearer" })) + .toEqual({ providers: ["zai-discount"], models: [] }); + // The environment token and loopback have no stored record to attach a + // scope to, so narrowing them would be inventing a policy nobody wrote. + expect(resolveAdmissionModelScope(config, { kind: "environment", source: "bearer" })).toBeUndefined(); + expect(resolveAdmissionModelScope(config, { kind: "loopback", source: "loopback" })).toBeUndefined(); + expect(resolveAdmissionModelScope(config, undefined)).toBeUndefined(); + expect(resolveAdmissionModelScope(config, { kind: "configured", keyId: "open", source: "bearer" })).toBeUndefined(); + }); + + test("the two lists are independent conditions and both must hold", () => { + const scope = admissionModelScopeOf({ allowedProviders: ["zai-discount"], allowedModels: ["glm-5.3-flash"] })!; + expect(routeAllowedByScope(scope, { providerName: "zai-discount", modelId: "glm-5.3-flash" })).toBe(true); + // The allowed model on a forbidden provider is exactly what a combo child or + // a policy fallback reaches while the requested selector never changes. + expect(routeAllowedByScope(scope, { providerName: "openrouter", modelId: "glm-5.3-flash" })).toBe(false); + // The allowed provider carrying a forbidden model is the mirror case. + expect(routeAllowedByScope(scope, { providerName: "zai-discount", modelId: "grok-4.6" })).toBe(false); + }); + + test("a model entry matches bare or fully qualified, and folds case and spacing", () => { + const scope = admissionModelScopeOf({ allowedModels: [" ZAI-Discount/GLM-5.3-Flash "] })!; + expect(routeAllowedByScope(scope, { providerName: "zai-discount", modelId: "glm-5.3-flash" })).toBe(true); + // Pinned to that provider: the same model elsewhere is a different destination. + expect(routeAllowedByScope(scope, { providerName: "openrouter", modelId: "glm-5.3-flash" })).toBe(false); + const bare = admissionModelScopeOf({ allowedModels: ["glm-5.3-flash"] })!; + expect(routeAllowedByScope(bare, { providerName: "openrouter", modelId: "glm-5.3-flash" })).toBe(true); + }); + + test("an alias is judged by what it resolves to, not by the name the client sent", () => { + // The defect this guards: a bare alias names neither the provider nor the + // model, so a scope checked against the caller's string has nothing to + // match on and would authorize a destination it never saw. + const config = { + port: 10100, + defaultProvider: "allowed", + providers: { + allowed: { adapter: "openai-chat", baseUrl: "https://allowed.test/v1", models: ["small"] }, + forbidden: { + adapter: "openai-chat", baseUrl: "https://forbidden.test/v1", + models: ["expensive"], modelAliases: { expensive: "cheap" }, + }, + }, + } as unknown as OcxConfig; + const scope = admissionModelScopeOf({ allowedProviders: ["allowed"] })!; + + const routed = routeModel(config, "cheap"); + expect(routed).toMatchObject({ providerName: "forbidden", modelId: "expensive" }); + expect(routeAllowedByScope(scope, routed)).toBe(false); + expect(() => assertRouteAllowedByScope(scope, "cheap", routed)).toThrow(AdmissionModelDeniedError); + + const permitted = routeModel(config, "allowed/small"); + expect(() => assertRouteAllowedByScope(scope, "allowed/small", permitted)).not.toThrow(); + }); + + test("a refusal is 403 and names the caller's own selector", () => { + const error = new AdmissionModelDeniedError("gldf-flash", { providerName: "xai", modelId: "grok-4.6" }); + const body = admissionModelDeniedBody(error); + expect(body.error.type).toBe(MODEL_NOT_ALLOWED_FOR_KEY); + // The client learns which of its own requests was refused. A key that may + // not reach xai has no business learning that its alias points there. + expect(body.error.model).toBe("gldf-flash"); + expect(JSON.stringify(body)).not.toContain("grok-4.6"); + const response = admissionModelDeniedResponse(error); + // 403, not 404: the key authenticated and simply may not go there, and a + // 404 would tell the client its credential is wrong and invite a retry. + expect(response.status).toBe(403); + }); + + test("a malformed scope drops the key instead of widening it", () => { + const valid = { key: KEY, id: "scoped", name: "mail", createdAt: "2026-01-01T00:00:00.000Z", allowedProviders: ["zai-discount"] }; + expect(apiKeyEntrySchema.safeParse(valid).success).toBe(true); + // Every other field on this record degrades to a default. These two must + // not: degrading a damaged permission field reads as "allowed everything", + // which is the one direction it can never fail. + for (const damaged of [ + { ...valid, allowedProviders: "zai-discount" }, + { ...valid, allowedProviders: [""] }, + { ...valid, allowedModels: [123] }, + { ...valid, allowedModels: ["x".repeat(257)] }, + ]) { + expect(apiKeyEntrySchema.safeParse(damaged).success).toBe(false); + } + // A degrading neighbour still degrades, so the fail-closed choice is scoped + // to the permission fields rather than hardening the whole record. + expect(apiKeyEntrySchema.safeParse({ ...valid, name: 7 }).success).toBe(true); + }); +}); From c4a105b1e436effe385ca69431db0eaec11f0b0f Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:23:17 +0900 Subject: [PATCH 5/6] docs(devlog): record lane E of the phase 2 consolidation batch --- .../050_lane_e.md | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md diff --git a/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md b/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md new file mode 100644 index 00000000000..ef7914e9a26 --- /dev/null +++ b/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md @@ -0,0 +1,105 @@ +# Lane E — output budgets, queue memory, per-key policy + +Status: OPEN. Branch `codex/260920-lane-e-budgets-key-policy` against `dev`, one pull request. +Covers phase 2 bundles 10, 11 and 12 from [010_phase2.md](010_phase2.md). + +## What each bundle turned out to be + +### 10 — Devin output budget and the history ceiling + +Two defects, not one. The adapter forwarded only a caller-supplied +`max_output_tokens`, and Codex never sends one, so every `devin/*` turn was capped at +the cloud-direct encoder's 8192 fallback however the provider was configured. The +escape hatch was closed too: no OAuth preset declares `defaultMaxOutputTokens` or +`modelMaxOutputTokens`, so the delete-when-preset-undefined branch in +`applyOAuthPresetCatalog` was the only branch either field ever took and a +hand-edited value was gone before the next startup finished. Both had to move, or +wiring the adapter alone would have been unreachable in practice. + +The resolver reads the caller's explicit value, then the configured per-model cap, +then the provider default, then nothing — leaving the encoder fallback. It never +reads `contextWindow` or `modelContextWindows`: CompletionConfiguration #2 is the +output cap and #3 is the context window, and collapsing them would ask Cognition to +generate a whole window of output. + +The history ceiling is the other half and stays a separate quantity. #5189 is +carried with attribution: it derives the coding-agent projected-history bound from +the declared context window in characters. That bounds replayed history memory; +nothing there decides how long a reply may run. + +No retry change was needed. Source review of `stated-reset-retry.ts` and +`upstream-retry.ts` confirms an upstream `incomplete / max_output_tokens` is a +successful streaming response that has already emitted events, so it matches none +of the replay conditions. The repeated identical attempts in #5190 are the client's. + +### 11 — adapter event queue memory + +PR #5182 had the right idea and the wrong number. Its 1 MiB aggregate default +aborts a legitimate turn: a synchronous producer fills the queue before its +consumer is scheduled, and the image loop does exactly that with over a million +one-character deltas that coalesce into roughly 1.2 MB of retained text. Its own CI +proved it, which is why it sits at `CHANGES_REQUESTED`. + +The work is carried with attribution and reshaped around two budgets rather than +one, because a stalled consumer and a malformed event are different failures and an +operator reading the terminal error should learn which happened. Accounting is now +exact by construction: each queued item records what it was charged, so a merge +pays only for appended text, a refused event is priced before anything is retained +and never charged, and the terminal record explaining a refusal is admitted past +the budget it reports but still charged and released. `retainedCodeUnits()` exposes +the counter so the regressions assert it reaches zero rather than inferring it from +an abort that happened to fire. + +Retention is measured by a bounded walk of own enumerable properties rather than a +per-variant table. A table would be exhaustive over `AdapterEvent`, which is the +union class `AGENTS.md` records: a member added on another branch would silently +stop being counted. + +### 12 — per-admission-key model and provider scope + +The security question is where the check goes, not what it compares. A scope +evaluated against the client's string authorizes one destination and reaches +another, because alias resolution, policy and combo selection, subagent fallback +and compaction override all rewrite that string. So the scope names destinations +and is applied to the resolved route. + +On the Responses path every route produced by the request — direct name, alias, +policy, combo child, shadow-intercept target and both subagent-fallback re-routes — +passes through one capture point, which is where the check sits. Chat and Messages +translate into that path; their native lanes and the compaction route send without +re-entering it, so each applies the same predicate itself. `/v1/models` filters by +the same predicate, and that filter is explicitly not the boundary. + +A malformed scope drops the key rather than degrading to `undefined` like every +other field on the record, because degrading a permission field reads as "allowed +everything". + +Out of scope and deliberately not started: Redis, a full multi-tenant conversion, +and any budget or RPM/TPM system. + +## Verification + +Static source review plus exact-head hosted CI. No local suite, individual test, +typecheck, build, install or live `ocx` execution was run — those are NOT RUN, not +passing. + +Union-defect classes checked before pushing. No file in the touched set carries a +`file-size-baseline.json` cap; the largest, `src/oauth/index.ts` and +`src/server/index/serve-options.ts`, stay under the 2000-line new-file threshold. +The two new test files are registered in both `scripts/test-layout/layout.json` and +`tests/fixtures/test-layout-expected.json`. Nothing here restates a count or +enumerates a union. + +## Ownership + +Lane E owns the adapter event queue budget and the Devin and coding-agent limits. +Retry classification — `sendCount`, the send budget, the stage and cause vocabulary — +is lane C's and is untouched. + +## Carried work + +- #5182 (luvs01) — adapter event queue backlog budget. +- #5189 (mdwsk88) — coding-agent projected-history ceiling. + +Both carry a `Co-authored-by` trailer in the branch commit. Neither original pull +request is closed here; the coordinator handles that after this lane lands. From 6a4b5848c912157aafbff6823dafb992d2e956a9 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:31:53 +0900 Subject: [PATCH 6/6] fix(server,adapters): close the virtual-model scope gap and harden queue retention Three findings from an adversarial review of this branch. The scope check ran before applyOpenAiVirtualModel, which rewrites route.modelId to the wire id that is actually billed. A key allowing only the public selector was therefore authorized on one model and sent on another. The settled route is now re-checked after normalization, so the id that is billed is the id that was authorized. The account-qualified branch of /v1/alpha/search resolves a model through the router and bills the account it names, so it applies the same rule. The endpoints that spend quota without routing a model -- images, audio, realtime, and the non-account-qualified search branch -- are recorded in the lane document as uncovered rather than left to read as covered. The queue's per-event budget comment claimed it bound any single event. It bounds a RETAINED one: an event handed straight to a waiting consumer is never held, so refusing it would abort a turn over memory this queue does not own. The comment now says what the code does. retainedEventCodeUnits also guards a non-object, so a malformed adapter emission becomes a terminal event rather than a TypeError thrown out of push with the queue half-updated. The scope regression reached the config schema through config/schema/leaf-validators directly, which enters that module cycle from the wrong end and threw a TDZ ReferenceError on CI. It now loads a hand-written config.json through src/config the way a startup does, which also proves the stronger property: a damaged permission field drops that key alone and its valid neighbour survives. --- .../050_lane_e.md | 23 ++++++++ src/adapters/run-turn-queue.ts | 9 +++ src/server/responses/request-prepare.ts | 9 +++ src/server/search.ts | 12 ++++ tests/adapters/run-turn-queue.test.ts | 8 ++- tests/server/api-key-model-scope.test.ts | 57 +++++++++++++++---- 6 files changed, 106 insertions(+), 12 deletions(-) diff --git a/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md b/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md index ef7914e9a26..154a0a639e9 100644 --- a/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md +++ b/devlog/_plan/260920_meaning_preservation_batch/050_lane_e.md @@ -77,6 +77,29 @@ everything". Out of scope and deliberately not started: Redis, a full multi-tenant conversion, and any budget or RPM/TPM system. +#### What the scope does not cover, stated rather than implied + +An adversarial review of the branch found authenticated data-plane endpoints that +spend provider quota without resolving a model through the router, so the scope +does not reach them: + +- `/v1/images/generations` and `/v1/images/edits`, +- `/v1/audio/transcriptions` and its streaming form, +- `/v1/live`, `/v1/realtime/calls` and the standalone realtime sockets, +- the non-account-qualified branch of `/v1/alpha/search`, which forwards the caller's + model to a search sidecar without routing it. + +The account-qualified search branch does route a model and is checked. The rest +need a destination definition this lane does not own — an image or audio endpoint +has a fixed-purpose model rather than a routed one — and inventing one here would +be the multi-tenant expansion this batch rules out. They are recorded so the +contract is not read as broader than it is. + +The review also found that resolving an OpenAI virtual model rewrites +`route.modelId` to the wire id after the initial check. That one was a real hole in +the stated contract and is fixed: the settled route is re-checked after +normalization, so the id that is billed is the id that was authorized. + ## Verification Static source review plus exact-head hosted CI. No local suite, individual test, diff --git a/src/adapters/run-turn-queue.ts b/src/adapters/run-turn-queue.ts index 63e06033881..23c1cd50f18 100644 --- a/src/adapters/run-turn-queue.ts +++ b/src/adapters/run-turn-queue.ts @@ -37,6 +37,11 @@ export const DEFAULT_MAX_BACKLOG_CODE_UNITS = 32 * 1024 * 1024; * however empty the queue is, so it must be refused even with the whole * aggregate free. They also report different terminal messages, so an operator * reading the turn's error learns which happened. + * + * Like the aggregate, this governs what the queue RETAINS. An event handed + * straight to a waiting consumer is never held here, so neither budget applies + * to it: refusing it would abort a turn over memory this queue does not own, + * and the consumer's own per-event bound governs that payload instead. */ export const DEFAULT_MAX_EVENT_CODE_UNITS = 8 * 1024 * 1024; @@ -67,6 +72,10 @@ const RETENTION_MAX_NODES = 4096; * of a kind and not payload anyone is buffering. */ export function retainedEventCodeUnits(event: AdapterEvent): number { + // Defensive: this measures values an adapter produced. A malformed emission + // has to become a terminal event, not a TypeError thrown out of push() with + // the queue half-updated. + if (!event || typeof event !== "object") return 0; let total = 0; let nodes = 0; const seen = new Set(); diff --git a/src/server/responses/request-prepare.ts b/src/server/responses/request-prepare.ts index 0aeb7a70f6c..a96a672ada1 100644 --- a/src/server/responses/request-prepare.ts +++ b/src/server/responses/request-prepare.ts @@ -115,6 +115,7 @@ import { AdmissionModelDeniedError, assertRouteAllowedByScope, resolveAdmissionModelScope, + routeAllowedByScope, } from "../admission-model-scope"; import { nativeContextLimits } from "../../codex/catalog"; import { streamingContextOverflowResponse } from "./context-overflow"; @@ -1041,6 +1042,14 @@ export async function prepareResponsesRequest( inboundTransport: options.inboundTransport, claudeGoAffinity: options.claudeGoAffinity, }); + // Normalization is the last thing that can move the destination: resolving an + // OpenAI virtual model rewrites route.modelId to the wire id that will + // actually be billed. A scope checked only before this would authorize the + // public selector and send the wire model, so the settled route is checked + // once more here. + if (!routeAllowedByScope(admissionScope, route)) { + return admissionModelDeniedResponse(new AdmissionModelDeniedError(inboundSelector, route)); + } // Attribute local auth/cooldown failures to the public selector too; exact auth may fail before // the normal post-resolution provider label is assigned. if (route.codexAccountNamespace) { diff --git a/src/server/search.ts b/src/server/search.ts index e66a51e24b7..6f9143ddaca 100644 --- a/src/server/search.ts +++ b/src/server/search.ts @@ -25,6 +25,12 @@ import { codexAccountNamespaceForModel } from "../codex/account-namespace-match" import { NATIVE_RESERVE_MODEL } from "../codex/catalog/native-models"; import { isCodexReserveRequestEligible } from "../codex/loopback-target"; import type { DataPlaneAdmission } from "./auth-cors"; +import { + admissionModelDeniedResponse, + AdmissionModelDeniedError, + resolveAdmissionModelScope, + routeAllowedByScope, +} from "./admission-model-scope"; import { formatCodexProviderForLog } from "../codex/routing"; import { signalWithTimeout } from "../lib/abort"; import { readBoundedResponseBytes } from "../lib/bounded-body"; @@ -87,6 +93,12 @@ export async function handleSearch( if (!route.codexAccountId || route.codexAccountNamespace !== accountNamespace) { return formatErrorResponse(400, "invalid_request_error", "Invalid Codex account-qualified search model"); } + // This branch resolves a model through the router and bills the account it + // names, so a scoped key is held to the same destination rule it is held + // to on the inference path. + if (!routeAllowedByScope(resolveAdmissionModelScope(config, admission), route)) { + return admissionModelDeniedResponse(new AdmissionModelDeniedError(model, route)); + } exactAccount = { accountId: route.codexAccountId, modelId: route.modelId }; logCtx.provider = `${route.providerName}-${accountNamespace}`; logCtx.routeDecision = route.routeDecision; diff --git a/tests/adapters/run-turn-queue.test.ts b/tests/adapters/run-turn-queue.test.ts index 99120d90389..b1f821d4818 100644 --- a/tests/adapters/run-turn-queue.test.ts +++ b/tests/adapters/run-turn-queue.test.ts @@ -424,7 +424,9 @@ describe("run-turn adapter event queue retained-payload budgets", () => { const pending = iterator.next(); queue.push(text("x".repeat(1_000))); - // The queue never held it, so neither budget has anything to say about it. + // The queue never held it, so neither budget has anything to say about it: + // both govern retained payload, and refusing this would abort a turn over + // memory the queue does not own. expect(await pending).toEqual({ done: false, value: text("x".repeat(1_000)) }); expect(backlogExceeded).toBe(0); expect(queue.retainedCodeUnits()).toBe(0); @@ -495,6 +497,10 @@ describe("run-turn adapter event queue retained-payload budgets", () => { expect(retainedEventCodeUnits(text("abcd"))).toBe(4); expect(retainedEventCodeUnits(heartbeat)).toBe(0); expect(retainedEventCodeUnits(phasedText("ab", "commentary"))).toBe(12); + // A malformed adapter emission has to become a terminal event, not a + // TypeError thrown out of push() with the queue half-updated. + expect(retainedEventCodeUnits(null as unknown as AdapterEvent)).toBe(0); + expect(retainedEventCodeUnits("oops" as unknown as AdapterEvent)).toBe(0); // Nested provider-shaped payload is counted; a cycle terminates. const cyclic: Record = { owner: "abc" }; cyclic.self = cyclic; diff --git a/tests/server/api-key-model-scope.test.ts b/tests/server/api-key-model-scope.test.ts index 94c8a3fe0d9..51e45b7c8c6 100644 --- a/tests/server/api-key-model-scope.test.ts +++ b/tests/server/api-key-model-scope.test.ts @@ -1,4 +1,11 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +// The schema this exercises is reached through `src/config`, the entry the +// runtime evaluates first. Importing `config/schema/*` directly enters that +// module cycle from the wrong end and throws a TDZ ReferenceError. +import { loadConfig } from "../../src/config"; import { admissionModelDeniedBody, AdmissionModelDeniedError, @@ -9,13 +16,36 @@ import { resolveAdmissionModelScope, routeAllowedByScope, } from "../../src/server/admission-model-scope"; -import { apiKeyEntrySchema } from "../../src/config/schema/leaf-validators"; import { routeModel } from "../../src/router"; import type { OcxConfig } from "../../src/types"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; const KEY = "ocx_data_" + "a".repeat(40); const OTHER_KEY = "ocx_data_" + "b".repeat(40); +const previousHome = process.env.OPENCODEX_HOME; +const homes: string[] = []; + +afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + for (const home of homes.splice(0)) removeTreeWithRetry(home); +}); + +/** Load a hand-written config.json the way a startup would, and report the keys that survived. */ +function loadKeysFrom(apiKeys: unknown[]): string[] { + const home = mkdtempSync(join(tmpdir(), "ocx-key-scope-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + writeFileSync(join(home, "config.json"), JSON.stringify({ + port: 10100, + defaultProvider: "allowed", + providers: { allowed: { adapter: "openai-chat", baseUrl: "https://allowed.test/v1", models: ["small"] } }, + apiKeys, + })); + return (loadConfig().apiKeys ?? []).map(entry => entry.name); +} + function configWithKey(scope: { allowedProviders?: string[]; allowedModels?: string[] }): Pick { return { apiKeys: [ @@ -104,21 +134,26 @@ describe("per-key model and provider scope", () => { }); test("a malformed scope drops the key instead of widening it", () => { - const valid = { key: KEY, id: "scoped", name: "mail", createdAt: "2026-01-01T00:00:00.000Z", allowedProviders: ["zai-discount"] }; - expect(apiKeyEntrySchema.safeParse(valid).success).toBe(true); + const scoped = { key: KEY, id: "scoped", name: "mail", createdAt: "2026-01-01T00:00:00.000Z", allowedProviders: ["zai-discount"] }; + const open = { key: OTHER_KEY, id: "open", name: "coding", createdAt: "2026-01-01T00:00:00.000Z" }; + expect(loadKeysFrom([scoped, open])).toEqual(["mail", "coding"]); + // Every other field on this record degrades to a default. These two must // not: degrading a damaged permission field reads as "allowed everything", - // which is the one direction it can never fail. + // which is the one direction it can never fail. The damaged key is dropped + // and its still-valid neighbour survives, so one bad record does not take + // the whole array with it. for (const damaged of [ - { ...valid, allowedProviders: "zai-discount" }, - { ...valid, allowedProviders: [""] }, - { ...valid, allowedModels: [123] }, - { ...valid, allowedModels: ["x".repeat(257)] }, + { ...scoped, allowedProviders: "zai-discount" }, + { ...scoped, allowedProviders: [""] }, + { ...scoped, allowedModels: [123] }, + { ...scoped, allowedModels: ["x".repeat(257)] }, ]) { - expect(apiKeyEntrySchema.safeParse(damaged).success).toBe(false); + expect(loadKeysFrom([damaged, open])).toEqual(["coding"]); } + // A degrading neighbour still degrades, so the fail-closed choice is scoped // to the permission fields rather than hardening the whole record. - expect(apiKeyEntrySchema.safeParse({ ...valid, name: 7 }).success).toBe(true); + expect(loadKeysFrom([{ ...scoped, name: 7 }, open])).toHaveLength(2); }); });