From 734061f736f62bc95d028fb0d8878e7d023c97be 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 12:19:16 -0300 Subject: [PATCH] fix(thinking): header opt-out of marker for reasoning_content clients (#5312 RC-C, #5245) (#5367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrated into release/v3.8.41 — header opt-out of marker (#5312 RC-C, #5245). --- CHANGELOG.md | 1 + open-sse/handlers/chatCore.ts | 23 ++++- open-sse/utils/thinkCloseMarker.ts | 41 +++++++++ .../think-close-marker-suppress-5245.test.ts | 89 ++++++++++++++++++- 4 files changed, 150 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e05cfc6541..2e77ddfdac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ _In development — bullets added per PR; finalized at release._ ### 🔧 Bug Fixes +- **thinking (claude):** let reasoning_content-native clients (e.g. Cursor) opt out of the `` close-marker via the request header `x-omniroute-thinking-marker: off` so it no longer leaks an orphan `` into visible `content` (RC-C of #5312, shared with #5245). The suppression machinery already existed (UA allowlist, #5348); this adds explicit per-request opt-in (also `on`/`keep`). Header absent ⇒ byte-identical default (Claude Code/#4633 clients still get the marker). Regression guard: `tests/unit/think-close-marker-suppress-5245.test.ts`. ([#5312](https://github.com/diegosouzapw/OmniRoute/issues/5312), [#5245](https://github.com/diegosouzapw/OmniRoute/issues/5245)) - **cors:** browser/Electron clients (e.g. Wayland AI) can now use OmniRoute as an OpenAI-compatible provider out-of-the-box. The token-authenticated API surface (`/v1/*`, `/v1beta/*`) now returns a permissive `Access-Control-Allow-Origin` (echoes the request `Origin`, `*` when absent) by default — matching 9router and the OpenAI-compatible ecosystem — so a renderer `fetch` can read the response instead of failing CORS-blocked as "site not found" / empty catalog (while `curl`, which sends no preflight, worked). This is **safe**: those routes auth via `Authorization`/`x-api-key` headers browsers never auto-attach (no credentialed-session/CSRF exposure), and `Access-Control-Allow-Credentials` is never paired with the echo/wildcard. Cookie-authed **MANAGEMENT/dashboard routes stay exactly fail-closed**; `CORS_ALLOW_ALL`/`CORS_ALLOWED_ORIGINS` still take precedence. Regression guards: `tests/unit/cors/origins.test.ts`, `tests/unit/authz/pipeline.test.ts`. (Bug 2 of [#5242](https://github.com/diegosouzapw/OmniRoute/issues/5242) — thanks @jonlwheat2-gif) - **grok-web:** forward the Cloudflare clearance cookies and stop mislabeling IP-reputation blocks as a bad cookie. "Check cookie" returned `Invalid SSO cookie` even with a valid, complete browser session — but the cookie parser was never the problem (it robustly extracts `sso`/`sso-rw` from a full DevTools header). Two real gaps fixed: **(1)** `buildGrokCookieHeader` now forwards `cf_clearance` and `__cf_bm` when pasted (it dropped them before; AIClient2API forwards them too) — strictly additive, a bare `sso` blob still yields exactly `sso=…`; **(2)** when the user supplied a `cf_clearance`, a 401 / invalid-credentials-403 from grok.com is now surfaced as an IP-reputation/anti-bot block (cf_clearance is IP+TLS+UA-pinned and can't be replayed from a different machine) instead of the misleading "Invalid SSO cookie — re-paste". A bare cookie with no clearance still gets the re-paste hint. Regression guards in `web-cookie-auth.test.ts` + `provider-validation-specialty.test.ts`. ([#5350](https://github.com/diegosouzapw/OmniRoute/issues/5350) — thanks @SeaXen) - **cli (serve):** opt-in native **HTTPS/TLS** for `omniroute serve` — so strict-CSP Electron apps and browsers can reach OmniRoute over `https://` instead of plain `http://localhost`. Provide `--tls-cert --tls-key ` (or `OMNIROUTE_TLS_CERT`/`OMNIROUTE_TLS_KEY`) and the standalone server terminates TLS on the same listener (no extra port/proxy); WebSocket upgrade (live dashboard + `/v1` streaming) works over `wss://` unchanged since `https.Server extends http.Server`. With no TLS flags the HTTP path is byte-identical to before; only one of cert/key, or an unreadable path, logs a warning and stays HTTP (never half-enables, never crashes). Auto-generated self-signed certs for localhost are a follow-up; for now provide an explicit cert/key (or front OmniRoute with a TLS terminator). Regression guard: `tests/unit/tls-options.test.ts`. (Bug 1C of [#5242](https://github.com/diegosouzapw/OmniRoute/issues/5242) — thanks @jonlwheat2-gif) diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 4ca9bc928b..fb371aa1cb 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -73,7 +73,10 @@ import { withBodyTimeout, } from "../utils/stream.ts"; import { ensureStreamReadiness } from "../utils/streamReadiness.ts"; -import { shouldSuppressThinkCloseMarker } from "../utils/thinkCloseMarker.ts"; +import { + resolveSuppressThinkClose, + THINKING_MARKER_HEADER, +} from "../utils/thinkCloseMarker.ts"; import { resolveStreamReadinessTimeout } from "../utils/streamReadinessPolicy.ts"; import { createStreamController } from "../utils/streamHandler.ts"; import * as streamFailure from "../utils/streamFailureFinalization.ts"; @@ -788,6 +791,15 @@ export async function handleChatCore({ .filter(Boolean) .join(" "); + // Explicit per-request opt-in/out for the `` close marker + // (#5312 / #5245): `x-omniroute-thinking-marker: off` suppresses it for + // reasoning_content-native clients (e.g. Cursor's OpenAI path) that the UA + // allowlist does not cover; absent the header, the UA policy applies. + const thinkingMarkerHeader = getHeaderValueCaseInsensitive( + clientRawRequest?.headers ?? null, + THINKING_MARKER_HEADER + ); + const explicitStreamAlias = resolveExplicitStreamAlias(body); // Remove non-standard non-stream aliases before provider translation/execution. @@ -4191,8 +4203,13 @@ export async function handleChatCore({ handleStreamFailure, copilotCompatibleReasoning, // Suppress the `` close marker for clients that render it verbatim - // (e.g. OpenCode); preserved for Claude Code / Cursor and unknown clients (#5245). - shouldSuppressThinkCloseMarker(streamUserAgent) + // (e.g. OpenCode by UA; any client via `x-omniroute-thinking-marker: off`); + // preserved for Claude Code / Cursor and unknown clients by default (#5245 / + // #5312). The header wins over the UA allowlist. + resolveSuppressThinkClose({ + userAgent: streamUserAgent, + thinkingMarkerHeader, + }) ); } else { log?.debug?.("STREAM", `Standard passthrough mode`); diff --git a/open-sse/utils/thinkCloseMarker.ts b/open-sse/utils/thinkCloseMarker.ts index 978ebc281b..bea9a75c49 100644 --- a/open-sse/utils/thinkCloseMarker.ts +++ b/open-sse/utils/thinkCloseMarker.ts @@ -14,8 +14,18 @@ * default (preserving #4633 for Claude Code / Cursor and any unrecognized * client), and is suppressed ONLY for known clients that render it literally. * Detection is by inbound `User-Agent`. + * + * Clients that DO render the marker verbatim but are not in the UA allowlist + * (e.g. Cursor's reasoning_content-native OpenAI path — #5312 / #5245) can opt + * in explicitly with the request header `x-omniroute-thinking-marker: off`, + * which suppresses the marker regardless of User-Agent. `on` forces it kept + * (overriding the UA allowlist). The default (header absent) is byte-identical + * to the UA-only policy, so #4633 / #5123 are never regressed. */ +/** Header clients send to explicitly opt in/out of the `` close marker. */ +export const THINKING_MARKER_HEADER = "x-omniroute-thinking-marker"; + // Lowercased User-Agent substrings of clients that render the textual // `` marker verbatim and therefore want it suppressed. const SUPPRESS_THINK_CLOSE_UA_MARKERS = ["opencode"]; @@ -30,3 +40,34 @@ export function shouldSuppressThinkCloseMarker(userAgent: string | null | undefi const ua = userAgent.toLowerCase(); return SUPPRESS_THINK_CLOSE_UA_MARKERS.some((marker) => ua.includes(marker)); } + +/** + * Interpret the explicit `x-omniroute-thinking-marker` request header. + * Returns `true` (suppress the marker), `false` (force-keep the marker), or + * `null` when the header is absent/unrecognized (defer to the UA policy). + */ +export function thinkingMarkerHeaderSignal( + headerValue: string | null | undefined +): boolean | null { + if (typeof headerValue !== "string") return null; + const value = headerValue.trim().toLowerCase(); + if (value === "off" || value === "false" || value === "0" || value === "suppress") return true; + if (value === "on" || value === "true" || value === "1" || value === "keep") return false; + return null; +} + +/** + * Resolve whether the streamed `` close marker should be suppressed for + * this request. An explicit `x-omniroute-thinking-marker` header wins; absent + * that, the conservative User-Agent allowlist policy applies. With no header and + * an unrecognized UA the result is `false` (marker kept), so #4633 / #5123 stay + * byte-identical by default. + */ +export function resolveSuppressThinkClose(opts: { + userAgent?: string | null; + thinkingMarkerHeader?: string | null; +}): boolean { + const headerSignal = thinkingMarkerHeaderSignal(opts.thinkingMarkerHeader); + if (headerSignal !== null) return headerSignal; + return shouldSuppressThinkCloseMarker(opts.userAgent); +} diff --git a/tests/unit/think-close-marker-suppress-5245.test.ts b/tests/unit/think-close-marker-suppress-5245.test.ts index 83462dedea..071ce71b4e 100644 --- a/tests/unit/think-close-marker-suppress-5245.test.ts +++ b/tests/unit/think-close-marker-suppress-5245.test.ts @@ -3,7 +3,12 @@ import assert from "node:assert/strict"; const { claudeToOpenAIResponse } = await import("../../open-sse/translator/response/claude-to-openai.ts"); -const { shouldSuppressThinkCloseMarker } = await import("../../open-sse/utils/thinkCloseMarker.ts"); +const { + shouldSuppressThinkCloseMarker, + thinkingMarkerHeaderSignal, + resolveSuppressThinkClose, + THINKING_MARKER_HEADER, +} = await import("../../open-sse/utils/thinkCloseMarker.ts"); // #5245: when translating a Claude-native stream to OpenAI shape, // claude-to-openai.ts emits a textual `` close marker (by design, for @@ -55,6 +60,42 @@ function runThinkThenText(state: Record) { return out; } +// Drive a thinking-only stream that flushes the deferred at the +// message_delta finish path (no text_delta ever arrives; toolCalls stays empty). +// Exercises the L197-207 finish-flush branch in claude-to-openai.ts. +function runThinkOnlyThenFinish(state: Record) { + const out: unknown[] = []; + const push = (r: unknown) => { + if (Array.isArray(r)) out.push(...r); + else if (r) out.push(r); + }; + push(claudeToOpenAIResponse({ type: "message_start", message: { id: "m1" } }, state)); + push( + claudeToOpenAIResponse( + { type: "content_block_start", index: 0, content_block: { type: "thinking" } }, + state + ) + ); + push( + claudeToOpenAIResponse( + { + type: "content_block_delta", + index: 0, + delta: { type: "thinking_delta", thinking: "plan" }, + }, + state + ) + ); + push(claudeToOpenAIResponse({ type: "content_block_stop", index: 0 }, state)); + push( + claudeToOpenAIResponse( + { type: "message_delta", delta: { stop_reason: "end_turn" }, usage: { output_tokens: 5 } }, + state + ) + ); + return out; +} + function contentChunks(chunks: unknown[]): string[] { return chunks .map( @@ -92,3 +133,49 @@ test("claude-to-openai: suppressThinkClose drops the marker but keeps t assert.ok(!contents.includes(""), "marker must be suppressed"); assert.ok(contents.includes("169"), "real text still emitted"); }); + +// ── finish-flush path (L197-207, toolCalls.size === 0) ─────────────────────── + +test("claude-to-openai: finish-flush emits by default for thinking-only response (#4633)", () => { + const contents = contentChunks(runThinkOnlyThenFinish(newState())); + assert.ok(contents.includes(""), "marker must be emitted at finish by default"); +}); + +test("claude-to-openai: finish-flush suppressed under suppressThinkClose (#5312)", () => { + const contents = contentChunks(runThinkOnlyThenFinish(newState({ suppressThinkClose: true }))); + assert.ok(!contents.includes(""), "marker must be suppressed at finish"); +}); + +// ── header signal resolution (x-omniroute-thinking-marker — #5312) ─────────── + +test("thinkingMarkerHeaderSignal: off → suppress, on → keep, absent/unknown → null", () => { + assert.equal(thinkingMarkerHeaderSignal("off"), true); + assert.equal(thinkingMarkerHeaderSignal("OFF"), true); + assert.equal(thinkingMarkerHeaderSignal(" off "), true); + assert.equal(thinkingMarkerHeaderSignal("on"), false); + assert.equal(thinkingMarkerHeaderSignal("keep"), false); + assert.equal(thinkingMarkerHeaderSignal(""), null); + assert.equal(thinkingMarkerHeaderSignal("weird"), null); + assert.equal(thinkingMarkerHeaderSignal(null), null); + assert.equal(thinkingMarkerHeaderSignal(undefined), null); +}); + +test("resolveSuppressThinkClose: header opts in (Cursor) and overrides the UA allowlist", () => { + // header constant is the documented wire name + assert.equal(THINKING_MARKER_HEADER, "x-omniroute-thinking-marker"); + // Cursor's UA is NOT in the allowlist → marker kept by default (orphan , #5312)… + assert.equal(resolveSuppressThinkClose({ userAgent: "cursor-agent/0.5" }), false); + // …but `off` opts in to suppression regardless of UA. + assert.equal( + resolveSuppressThinkClose({ userAgent: "cursor-agent/0.5", thinkingMarkerHeader: "off" }), + true + ); + // `on` force-keeps even for an allowlisted (OpenCode) UA. + assert.equal( + resolveSuppressThinkClose({ userAgent: "opencode/1.0", thinkingMarkerHeader: "on" }), + false + ); + // No header → defers to UA policy (OpenCode suppressed, unknown kept). + assert.equal(resolveSuppressThinkClose({ userAgent: "opencode/1.0" }), true); + assert.equal(resolveSuppressThinkClose({ userAgent: "claude-code/1.0" }), false); +});