Merge PR #8722: fix(ai): strip leaked ```thinking delimiters from Gemini thought summaries (@roboomp)

2026-08-19 01:36:03 +02:00
parent e88fb70afe d06e6a30b6
commit 1037bca4be
4 changed files with 251 additions and 7 deletions
@@ -12,6 +12,7 @@
 ### Changed

 - Send the `omp/<version>` User-Agent on xAI chat (`xai` and `xai-oauth`) unless the request already set its own.
+- Fixed Gemini thought summaries occasionally leaking a raw `` ```thinking `` / `` ``````thinking `` fence delimiter into the reasoning block, so it no longer shows up as fence spam in the thinking display or persisted transcripts ([#8719](https://github.com/can1357/oh-my-pi/issues/8719)).

 ## [17.3.5] - 2026-08-16

@@ -0,0 +1,92 @@
+/**
+ * Strip self-referential reasoning-fence delimiters that a provider leaks
+ * *inside* a structured thinking part.
+ *
+ * The visible-channel healers ({@link ../utils/leaked-thinking-stream},
+ * {@link ./fenced-thinking}) split leaked ` ```thinking ` fences out of the
+ * *text* stream. They never run over parts a provider already flags as
+ * thinking, so when a model (observed on Gemini thought summaries — issue
+ * #8719) emits a bare ` ```thinking ` / ` ``````thinking ` opener line between
+ * summaries, that delimiter reaches display and persistence verbatim as fence
+ * spam inside the reasoning block.
+ *
+ * This stripper removes only a *standalone* reasoning-fence opener line — a line
+ * that is nothing but a run of ≥3 backticks immediately followed by the info
+ * string `thinking` or `reasoning`. Such a line is always redundant inside an
+ * already-structured thinking block and never carries content. Inline mentions
+ * (prose on the same line), language-tagged code fences (` ```rs `), and bare
+ * closers (` ``` `) are left untouched so legitimate fenced code inside the
+ * reasoning survives.
+ *
+ * Streaming-safe: deltas may split a line anywhere. A trailing partial line is
+ * held only while it remains a viable opener prefix; the moment it cannot be an
+ * opener it is flushed and the rest of the line passes through character-level.
+ * Correctness never depends on the prefix heuristic — every held line is
+ * classified strictly on its newline (or on {@link ThinkingFenceStripper.flush})
+ * before it is dropped.
+ */
+
+/**
+ * A complete standalone reasoning-fence opener: ≤3 lead spaces, ≥3 backticks,
+ * `thinking`/`reasoning`, optional trailing spaces, tolerating a trailing CR
+ * from a CRLF newline.
+ */
+const OPENER_LINE = /^ {0,3}`{3,}(?:thinking|reasoning)[ \t]*\r?$/i;
+
+/** Could `line` (a partial, newline-not-yet-seen) still grow into {@link OPENER_LINE}? */
+function couldBeOpenerPrefix(line: string): boolean {
+	// Tolerate a pending CR from a split CRLF.
+	const s = line.endsWith("\r") ? line.slice(0, -1) : line;
+	const m = /^ {0,3}(`*)([\s\S]*)$/.exec(s);
+	if (!m) return false;
+	const ticks = m[1]!.length;
+	const rest = m[2]!;
+	if (rest === "") return true; // still consuming leading spaces / backticks
+	if (ticks < 3) return false; // a non-backtick char appeared before 3 backticks: never a fence
+	const word = rest.replace(/[ \t]+$/, "").toLowerCase();
+	return "thinking".startsWith(word) || "reasoning".startsWith(word);
+}
+
+/**
+ * Stateful, line-oriented stripper for leaked reasoning-fence openers in one
+ * structured thinking block. One instance per thinking block; feed every
+ * thinking delta through {@link push} and drain the tail with {@link flush}.
+ */
+export class ThinkingFenceStripper {
+	/** Buffered content of the current line still being classified. */
+	#carry = "";
+	/** True once the current line is known not to be an opener; passes through until newline. */
+	#passthrough = false;
+
+	/** Consume one thinking delta; returns the sanitized text to emit (may be empty). */
+	push(chunk: string): string {
+		let out = "";
+		for (const ch of chunk) {
+			if (this.#passthrough) {
+				out += ch;
+				if (ch === "\n") this.#passthrough = false;
+				continue;
+			}
+			if (ch === "\n") {
+				if (!OPENER_LINE.test(this.#carry)) out += `${this.#carry}\n`;
+				this.#carry = "";
+				continue;
+			}
+			this.#carry += ch;
+			if (!couldBeOpenerPrefix(this.#carry)) {
+				out += this.#carry;
+				this.#carry = "";
+				this.#passthrough = true;
+			}
+		}
+		return out;
+	}
+
+	/** Drain any held partial line at block end; returns text to emit (may be empty). */
+	flush(): string {
+		const carry = this.#carry;
+		this.#carry = "";
+		this.#passthrough = false;
+		return OPENER_LINE.test(carry) ? "" : carry;
+	}
+}
@@ -6,6 +6,7 @@ import { scheduler } from "node:timers/promises";
 import { calculateCost } from "@oh-my-pi/pi-catalog/models";
 import { readSseJson } from "@oh-my-pi/pi-utils";
 import { renderDemotedThinking } from "../dialect/demotion";
+import { ThinkingFenceStripper } from "../dialect/thinking-fence-strip";
 import * as AIError from "../error";
 import type {
 	Api,
@@ -617,11 +618,23 @@ export async function consumeGoogleStream<T extends GoogleApiType>(args: {
 	const blocks = output.content;
 	const blockIndex = () => blocks.length - 1;
 	let currentBlock: TextContent | ThinkingContent | null = null;
+	// Heals a leaked reasoning-fence opener (```thinking / ``````thinking) that some
+	// Gemini thought summaries emit as a between-summary delimiter (#8719). One
+	// stripper per thinking block; created lazily on first thinking delta.
+	let thinkingStripper: ThinkingFenceStripper | null = null;
 	let firstTokenSeen = false;
 	let sawFinishReason = false;

 	const flushCurrent = () => {
 		if (!currentBlock) return;
+		if (currentBlock.type === "thinking" && thinkingStripper) {
+			const tail = thinkingStripper.flush();
+			if (tail) {
+				currentBlock.thinking += tail;
+				stream.push({ type: "thinking_delta", contentIndex: blockIndex(), delta: tail, partial: output });
+			}
+		}
+		thinkingStripper = null;
 		pushBlockEndEvent(currentBlock, blockIndex(), output, stream);
 	};

@@ -658,17 +671,21 @@ export async function consumeGoogleStream<T extends GoogleApiType>(args: {
 						currentBlock = startTextOrThinkingBlock(isThinking, output, stream);
 					}
 					if (currentBlock.type === "thinking") {
-						currentBlock.thinking += part.text;
+						thinkingStripper ??= new ThinkingFenceStripper();
+						const cleaned = thinkingStripper.push(part.text);
+						currentBlock.thinking += cleaned;
 						currentBlock.thinkingSignature = retainThoughtSignature(
 							currentBlock.thinkingSignature,
 							part.thoughtSignature,
 						);
-						stream.push({
-							type: "thinking_delta",
-							contentIndex: blockIndex(),
-							delta: part.text,
-							partial: output,
-						});
+						if (cleaned) {
+							stream.push({
+								type: "thinking_delta",
+								contentIndex: blockIndex(),
+								delta: cleaned,
+								partial: output,
+							});
+						}
 					} else {
 						currentBlock.text += part.text;
 						if (retainTextSignature) {
@@ -0,0 +1,134 @@
+import { describe, expect, it } from "bun:test";
+import { ThinkingFenceStripper } from "@oh-my-pi/pi-ai/dialect/thinking-fence-strip";
+import { consumeGoogleStream } from "@oh-my-pi/pi-ai/providers/google-shared";
+import type { GenerateContentResponse, Part } from "@oh-my-pi/pi-ai/providers/google-types";
+import type { AssistantMessage, AssistantMessageEvent, Model } from "@oh-my-pi/pi-ai/types";
+import { AssistantMessageEventStream } from "@oh-my-pi/pi-ai/utils/event-stream";
+import { buildModel } from "@oh-my-pi/pi-catalog/build";
+
+// Regression for #8719: Gemini thought summaries occasionally emit a bare
+// ```thinking / ``````thinking opener line as a between-summary delimiter.
+// It must never reach the persisted thinking text or the streamed thinking_delta.
+
+const feed = (stripper: ThinkingFenceStripper, chunks: string[]): string =>
+	chunks.map(chunk => stripper.push(chunk)).join("") + stripper.flush();
+
+describe("ThinkingFenceStripper", () => {
+	it("drops a standalone reasoning-fence opener line (any backtick run ≥3)", () => {
+		expect(feed(new ThinkingFenceStripper(), ["a\n```thinking\nb\n"])).toBe("a\nb\n");
+		expect(feed(new ThinkingFenceStripper(), ["a\n``````thinking\nb\n"])).toBe("a\nb\n");
+		expect(feed(new ThinkingFenceStripper(), ["```reasoning\nx"])).toBe("x");
+	});
+
+	it("strips an opener even when the delimiter is split across deltas", () => {
+		expect(feed(new ThinkingFenceStripper(), ["intro\n``", "````thin", "king\nrest"])).toBe("intro\nrest");
+	});
+
+	it("drops a trailing opener that never gets its newline (flush path)", () => {
+		expect(feed(new ThinkingFenceStripper(), ["done\n``````thinking"])).toBe("done\n");
+	});
+
+	it("preserves language-tagged code fences and bare closers inside reasoning", () => {
+		const body = "look:\n```rs\nlet x = 1;\n```\ndone\n";
+		expect(feed(new ThinkingFenceStripper(), [body])).toBe(body);
+	});
+
+	it("keeps inline mentions of the idiom (prose on the fence line)", () => {
+		const line = "I should emit a ```thinking block here.\n";
+		expect(feed(new ThinkingFenceStripper(), [line])).toBe(line);
+	});
+
+	it("keeps indented content that only resembles a fence", () => {
+		// 4-space indent is a code line, not a fence; must survive verbatim.
+		expect(feed(new ThinkingFenceStripper(), ["    ```thinking\n"])).toBe("    ```thinking\n");
+	});
+});
+
+const vertexModel: Model<"google-vertex"> = buildModel({
+	id: "gemini-3.7-flash",
+	name: "Gemini 3.7 Flash (Vertex)",
+	api: "google-vertex",
+	provider: "google-vertex",
+	baseUrl: "",
+	reasoning: true,
+	input: ["text"],
+	cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+	contextWindow: 200_000,
+	maxTokens: 32_000,
+});
+
+function emptyAssistant(): AssistantMessage {
+	return {
+		role: "assistant",
+		content: [],
+		api: "google-vertex",
+		provider: "google-vertex",
+		model: vertexModel.id,
+		usage: {
+			input: 0,
+			output: 0,
+			cacheRead: 0,
+			cacheWrite: 0,
+			totalTokens: 0,
+			cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 },
+		},
+		stopReason: "stop",
+		timestamp: 0,
+	};
+}
+
+/**
+ * Drive `consumeGoogleStream` over fabricated chunks (each an array of parts)
+ * and return the persisted thinking text plus the concatenated thinking deltas.
+ */
+async function runThinking(chunks: Part[][]): Promise<{ thinking: string; streamed: string }> {
+	const output = emptyAssistant();
+	const stream = new AssistantMessageEventStream();
+	const streamed: string[] = [];
+	const collecting = (async () => {
+		for await (const event of stream as AsyncIterable<AssistantMessageEvent>) {
+			if (event.type === "thinking_delta") streamed.push(event.delta);
+		}
+	})();
+
+	async function* googleStream(): AsyncGenerator<GenerateContentResponse> {
+		for (const parts of chunks) {
+			yield { candidates: [{ content: { parts } }] } as unknown as GenerateContentResponse;
+		}
+		yield { candidates: [{ finishReason: "STOP" }] } as unknown as GenerateContentResponse;
+	}
+
+	await consumeGoogleStream({ googleStream: googleStream(), output, stream, model: vertexModel, options: undefined });
+	stream.end(output);
+	await collecting;
+
+	const block = output.content.find(b => b.type === "thinking");
+	return { thinking: block?.thinking ?? "", streamed: streamed.join("") };
+}
+
+describe("consumeGoogleStream leaked thinking-fence delimiter (#8719)", () => {
+	it("heals a leaked ```thinking delimiter out of persistence and streaming", async () => {
+		const { thinking, streamed } = await runThinking([
+			[{ text: "Investigating the return type.\n", thought: true }],
+			[{ text: "``````thinking\n**Investigating Adapter Host Logic**\n", thought: true }],
+			[{ text: "The host owns the loop.", thought: true }],
+		]);
+		const expected = "Investigating the return type.\n**Investigating Adapter Host Logic**\nThe host owns the loop.";
+		expect(thinking).toBe(expected);
+		expect(thinking).not.toContain("```thinking");
+		expect(streamed).toBe(expected);
+	});
+
+	it("leaves normal thought summaries untouched", async () => {
+		const clean = "Considered options A and B; picked B for latency.";
+		const { thinking, streamed } = await runThinking([[{ text: clean, thought: true }]]);
+		expect(thinking).toBe(clean);
+		expect(streamed).toBe(clean);
+	});
+
+	it("does not strip the same idiom from visible (non-thought) text", async () => {
+		const { thinking } = await runThinking([[{ text: "here:\n```thinking\nx\n", thought: false }]]);
+		// No thinking block at all — the text branch is untouched by the stripper.
+		expect(thinking).toBe("");
+	});
+});