From 1a096c4b3a0ddae5cc196a8ed0aa6956e242174b Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Mon, 29 Jun 2026 03:47:47 -0300 Subject: [PATCH] fix(sse): omit Command Code max_tokens when client sends a non-positive value (#5166) (#5304) Integrated into release/v3.8.40 (CHANGELOG re-resolved against post-#5294 tip; code identical to the green commit) --- CHANGELOG.md | 1 + open-sse/executors/commandCode.ts | 15 ++-- ...mmand-code-maxtokens-negative-5166.test.ts | 76 +++++++++++++++++++ 3 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 tests/unit/command-code-maxtokens-negative-5166.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 05eccb6dfb..03d79167da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ _In development β€” bullets added per PR; finalized at release._ ### πŸ”§ Bug Fixes +- **command-code:** treat a non-positive `max_tokens`/`max_completion_tokens` (e.g. Zoo Code's `-1` "let the server choose") as "no limit" β€” omit the field instead of forcing it to `1`. `clampMaxTokens` previously did `Math.max(1, …)`, so a client `-1` was sent upstream as `max_tokens: 1`, truncating the response to a single token (the observed `completion_tokens: 1`, `content: null`, `reasoning_content: "The"` with `finish_reason: stop`). Now any value `≀ 0` is dropped so Command Code applies the model's native default; positive values are still floored and clamped to the 200k ceiling. Regression guard: `tests/unit/command-code-maxtokens-negative-5166.test.ts` ([#5166](https://github.com/diegosouzapw/OmniRoute/issues/5166) β€” thanks @Stazyu) - **fix(auth): compare-and-swap guard on the OAuth refresh persist** β€” under multi-agent load, the per-connection refresh mutex makes `[network refresh + DB write]` atomic for **one** connection, but it does not protect against a **third** writer (a sibling request, a concurrent HealthCheck, or a replica) landing a fresher `refresh_token` rotation on the same `connection_id` between the staleness read and the persist. Overwriting that fresher row reverts the sibling's rotation; the next caller then loads the now-consumed token, Auth0/Anthropic flag it as `refresh_token_reused`, and the whole token family gets revoked (the 1352Γ— claude/`aa5dd5cf` invalidation storm). `getAccessToken` now re-reads the row's current `refresh_token` immediately before persisting (inside the mutex) and **skips the write** when it has rotated past the token the caller presented β€” the caller still receives the freshly-issued access token, only the DB overwrite is skipped. Opt-in via `runWithCasGuard` (no active guard β‡’ byte-identical behavior); skip/persist counters exposed via `getCasGuardStats()`. Regression guard: `tests/unit/token-refresh-cas-guard-4038.test.ts`. ([#4038](https://github.com/diegosouzapw/OmniRoute/issues/4038) β€” thanks @KooshaPari for the root-cause diagnosis) - **mcp:** break the `schemas/tools.ts ↔ schemas/toolSearch.ts` import cycle introduced when the `tool_search` defs (#5269) were extracted into their own module β€” `toolSearch.ts` imported `McpToolDefinition` from `tools.ts` while `tools.ts` imported `toolSearchTool` from `toolSearch.ts`, failing `check:cycles` on `release/v3.8.40`. The shared `AuditLevel` + `McpToolDefinition` types now live in a leaf `schemas/toolDefinition.ts` that both import; `tools.ts` re-exports them for backward compatibility. - **compression (analytics):** record attempted-but-no-op compression runs so Stacked is no longer invisible when it saves nothing. Previously a `compression_analytics` row was written only on a net-positive saving, so a Stacked (RTKβ†’Caveman) pipeline that ran on already-compact context produced no row β€” indistinguishable from "never dispatched" (`byMode.stacked.count` stayed flat while Ultra climbed). Such runs are now recorded with `skip_reason` and surfaced as a per-mode `skipped` count plus `totalSkipped`/`bySkipReason` in the analytics summary and the Mode Breakdown; the existing net-saving totals/averages are unchanged (skip rows are excluded from them) (#4268 β€” thanks @abdulkadirozyurt, @androw) diff --git a/open-sse/executors/commandCode.ts b/open-sse/executors/commandCode.ts index a18100780c..70bebeedf1 100644 --- a/open-sse/executors/commandCode.ts +++ b/open-sse/executors/commandCode.ts @@ -148,14 +148,17 @@ function convertMessages(messages: unknown): { system: string; messages: unknown // Clamp a client-supplied max_tokens to the endpoint ceiling, mirroring the // provider-driven clamp in antigravity.ts: we only intervene when the value is -// present AND would otherwise be rejected (> 200_000). A valid value is -// returned floored; anything absent or non-numeric returns undefined so the -// caller can OMIT the field entirely and let Command Code's upstream apply the -// model's own native default (rather than us inventing a number). +// present, positive AND would otherwise be rejected (> 200_000). A valid value +// is returned floored; anything absent, non-numeric or non-positive returns +// undefined so the caller can OMIT the field entirely and let Command Code's +// upstream apply the model's own native default (rather than us inventing a +// number). A non-positive value such as Zoo Code's max_tokens:-1 ("let the +// server choose") must be omitted, NOT forced to 1 β€” the old Math.max(1,...) +// truncated output to a single token (#5166). function clampMaxTokens(value: unknown): number | undefined { const numeric = numberValue(value); - if (numeric === undefined) return undefined; - return Math.max(1, Math.min(Math.floor(numeric), MAX_COMMAND_CODE_TOKENS)); + if (numeric === undefined || numeric <= 0) return undefined; + return Math.min(Math.floor(numeric), MAX_COMMAND_CODE_TOKENS); } // Reasoning/thinking fields that payload rules or clients may inject and that diff --git a/tests/unit/command-code-maxtokens-negative-5166.test.ts b/tests/unit/command-code-maxtokens-negative-5166.test.ts new file mode 100644 index 0000000000..e155aa72f0 --- /dev/null +++ b/tests/unit/command-code-maxtokens-negative-5166.test.ts @@ -0,0 +1,76 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// #5166: Zoo Code sends `max_tokens: -1` to mean "let the server choose". The +// old clampMaxTokens did `Math.max(1, ...)`, forcing -1 β†’ 1 and truncating +// output to a single token (the observed `completion_tokens: 1`, `content:null`, +// `reasoning_content:"The"` symptom). A non-positive limit must be OMITTED so +// Command Code's upstream applies the model's own native default. + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-cc-maxtokens-5166-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { getExecutor } = await import("../../open-sse/executors/index.ts"); +const core = await import("../../src/lib/db/core.ts"); + +const originalFetch = globalThis.fetch; + +type FetchCall = { url: string; init: Record; body?: any }; + +function commandCodeStream(lines: unknown[]) { + const text = lines.map((line) => `${JSON.stringify(line)}\n`).join(""); + return new Response(text, { status: 200, headers: { "Content-Type": "application/x-ndjson" } }); +} + +test.afterEach(() => { + globalThis.fetch = originalFetch; +}); + +test.after(() => { + globalThis.fetch = originalFetch; + core.resetDbInstance(); +}); + +async function captureParams(body: Record): Promise { + const calls: FetchCall[] = []; + globalThis.fetch = async (url: any, init: any = {}) => { + calls.push({ url: String(url), init, body: JSON.parse(String(init.body)) }); + return commandCodeStream([{ type: "text-delta", text: "ok" }, { type: "finish" }]); + }; + await getExecutor("command-code").execute({ + model: "deepseek/deepseek-v4-pro", + stream: false, + credentials: { apiKey: "cc_test_key" }, + body: { messages: [{ role: "user", content: "Hi" }], ...body }, + }); + return calls[0]; +} + +test("Command Code omits max_tokens when the client sends max_tokens: -1 (#5166)", async () => { + const call = await captureParams({ max_tokens: -1 }); + assert.ok( + !("max_tokens" in call.body.params), + `max_tokens:-1 must be omitted, got params.max_tokens=${call.body.params.max_tokens}` + ); +}); + +test("Command Code omits max_tokens when the client sends max_completion_tokens: -1 (#5166)", async () => { + const call = await captureParams({ max_completion_tokens: -1 }); + assert.ok( + !("max_tokens" in call.body.params), + `max_completion_tokens:-1 must be omitted, got params.max_tokens=${call.body.params.max_tokens}` + ); +}); + +test("Command Code omits max_tokens when the client sends 0 (#5166)", async () => { + const call = await captureParams({ max_tokens: 0 }); + assert.ok(!("max_tokens" in call.body.params), "max_tokens:0 must be omitted"); +}); + +test("Command Code still honors a positive client max_tokens after the #5166 fix", async () => { + const call = await captureParams({ max_tokens: 2048 }); + assert.equal(call.body.params.max_tokens, 2048); +});