From 785a3a2985e18b3537aa58eae6ffab22a1806805 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 08:01:26 +0900 Subject: [PATCH 1/3] fix(qoder): refuse vendor CLI scaffolding in routed output The qoder route is contractually a text and reasoning surface: the CLI is launched with its own tools and MCP servers disabled and Codex owns tool control. The vendor CLI does not always honour that. An MCP lazy-loading enumerating the operator's configured MCP servers, and tool-call markup opened as and closed as , have both reached the client as assistant text, because the shared stream-json parser filters frame types and forwards a text delta's contents unread. Add a streaming scaffolding filter and wrap the emit callback the qoder adapter hands to runCodingAgentTurn. A complete block is recognizable and self-delimiting, so it is removed and the answer around it survives. Anything else carrying a scaffolding signature is not repairable by guesswork, so the turn fails closed with an error that names the marker class and nothing else -- echoing the offending text back would publish the server inventory this guard exists to contain. Markers can be split across deltas, so a tail that is still a possible marker prefix is held and released on flush. The terminal event flushes first: an answer swallowed in full and followed by done would otherwise reach the empty-completion guard as a successful but empty turn rather than a refusal. The suppressed block is discarded as it arrives, retaining only enough trailing bytes to spot a split closer, with a 64 KiB ceiling on how much one unterminated block may swallow before the turn is refused. Closes #4190 --- .../020_4190_qoder_scaffold_guard.md | 97 ++++++++++ scripts/test-layout/layout.json | 1 + src/adapters/qoder/adapter.ts | 70 ++++++- src/adapters/qoder/scaffold-guard.ts | 173 ++++++++++++++++++ tests/fixtures/test-layout-expected.json | 1 + tests/providers/qoder-scaffold-guard.test.ts | 155 ++++++++++++++++ 6 files changed, 496 insertions(+), 1 deletion(-) create mode 100644 devlog/_plan/260911_l6_streaming_tools/020_4190_qoder_scaffold_guard.md create mode 100644 src/adapters/qoder/scaffold-guard.ts create mode 100644 tests/providers/qoder-scaffold-guard.test.ts diff --git a/devlog/_plan/260911_l6_streaming_tools/020_4190_qoder_scaffold_guard.md b/devlog/_plan/260911_l6_streaming_tools/020_4190_qoder_scaffold_guard.md new file mode 100644 index 0000000000..f24b36fd9b --- /dev/null +++ b/devlog/_plan/260911_l6_streaming_tools/020_4190_qoder_scaffold_guard.md @@ -0,0 +1,97 @@ +# 020 — #4190: refuse vendor scaffolding on the qoder route + +Unit: `devlog/_plan/260911_l6_streaming_tools`. Lane L6, work-phase 2. +Issue: [#4190](https://github.com/lidge-jun/opencodex/issues/4190). + +## The contract that was being broken + +The qoder route is documented as a text and reasoning surface. `buildQoderArgs` launches the +CLI with `--tools "" --strict-mcp-config --setting-sources "" --max-turns 1 +--no-session-persistence`, and both `coding-agent/protocol.ts` and `coding-agent/turn.ts` +state that Codex retains tool control and vendor tools are never invoked. + +The reporter saw two things reach the client as assistant text anyway: an MCP lazy-loading +`` block enumerating the local machine's configured MCP servers from +`~/.qoder/mcp.json` and from plugins, and vendor tool-call markup opened as +`` and closed as `` — mismatched, which is what a model emitting +remembered markup looks like rather than a serializer's output. + +The proxy-side hole is one line of trust. `mapRawStreamEvent` forwards a `text_delta` +verbatim; frame *types* are filtered, frame *contents* are not. So whatever the vendor's agent +layer puts in the text channel is relayed, and the strongest version of this leak publishes +the operator's MCP server inventory to whoever is reading the turn. + +## What was implemented + +`src/adapters/qoder/scaffold-guard.ts`, a streaming filter, and `guardQoderScaffolding` in +the adapter, which wraps the `emit` callback handed to `runCodingAgentTurn`. That wrapper is +the last qoder-specific point in the path, which is why the guard sits there rather than in +the parser every coding-agent CLI shares — CodeBuddy runs the same turn code and is not part +of this report. + +Two shapes, two answers, per the lane packet's recorded decision: + +- A complete `…` block is recognizable and + self-delimiting. It is removed and the answer around it survives. +- Anything else carrying a scaffolding signature — ``, or a `` with no opener — fails the turn closed. A partial + tool-call block has no reliable end, and the prose around it may be the vendor's own agent + narration rather than the model's answer, so repairing it would be guesswork. + +It is a stream filter, not a regex over a finished string. A marker can be split across +deltas, so a tail that is still a possible marker prefix is held rather than emitted, and the +terminal event flushes both channels first. That flush is load-bearing in a way that is not +obvious: `isContentEvent` in `empty-completion-guard.ts` counts only non-empty +`text_delta`s as content, so an answer swallowed in full and followed by `done` would reach +the client as a successful but empty turn instead of as the refusal it is. + +The suppressed block is discarded as it arrives; only the trailing bytes needed to spot a +split closer are kept, so an unclosed reminder cannot grow memory. A 64 KiB ceiling bounds +how much of a turn one unterminated block may swallow before the turn is refused. + +## Decisions this issue left open + +**Qoder only, not the shared coding-agent path.** The lane owns `src/adapters/qoder/` and +not `src/adapters/coding-agent/protocol.ts`, and the leak is reported only for Qoder. If +CodeBuddy turns out to do the same thing, the filter is a pure module and lifting it is a +small change — but it should be driven by a report, not by symmetry. + +**Fail closed rather than strip, for tool-call markup.** The issue's own review lists both +options. Silently deleting markup leaves the user with a mutilated answer and no signal that +the route's contract was violated; the error names the marker class and says why. + +**Not retryable.** The leak is intermittent, so a retry would often succeed. It is still +marked `retryable: false`: an automatic retry spends the operator's vendor credits on a +contract violation the proxy cannot influence, and hiding an intermittent violation is how it +stays unfixed. + +**Vendor tool-call frames are left alone.** `mapRawStreamEvent` maps a `tool_use` block to +`tool_call_start`. That is arguably also a contract violation, but it is a typed frame rather +than leaked text, the issue reports the text channel, and `protocol.ts` documents that seam +as deliberately prepared for a future tool bridge. + +**Known false positive.** A turn that legitimately discusses `` or +`` syntax will be stripped or refused. That is the cost of failing closed on a +route whose leak publishes the operator's MCP inventory, and it is the direction the packet +recorded. + +**Not touched: making the CLI actually run with MCP disabled.** The issue's first suggested +direction is to fix the spawn so the vendor agent layer never initializes. The installed +`@qoder-ai/qodercli` bundle still contains the reminder builder and appears to initialize it +despite the flags, so that fix lives in the vendor, not here. This guard is the containment +that does not depend on the vendor agreeing. + +## Verification + +Focused regression test at `tests/providers/qoder-scaffold-guard.test.ts`, beside the +existing `qoder-adapter.test.ts`, registered in `scripts/test-layout/layout.json` and +`tests/fixtures/test-layout-expected.json`. It covers block removal, a marker split across +three deltas, the held tail released on flush, fail-closed on tool-call markup and on a stray +closer, the unterminated-block case, the latch, and the wrapper's terminal handling including +the flush-before-`done` rule and forwarding a vendor error rather than replacing it. Two +cases assert that neither the leaked server list nor the leaked shell command appears in the +refusal message. + +Local suite, typecheck and build: NOT RUN, by operator instruction for this dispatch round. +Hosted CI on the pushed head is the evidence. + diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index cee1514e55..4056d8bc26 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -997,6 +997,7 @@ "proxy-liveness.test.ts": "server", "qoder-adapter.test.ts": "providers", "qoder-live-models.test.ts": "providers", + "qoder-scaffold-guard.test.ts": "providers", "quota-401-recovery-runtime.test.ts": "usage", "quota-401-recovery.test.ts": "usage", "quota-bars-rows.test.ts": "gui", diff --git a/src/adapters/qoder/adapter.ts b/src/adapters/qoder/adapter.ts index 20c0b5581b..1bb8821b0b 100644 --- a/src/adapters/qoder/adapter.ts +++ b/src/adapters/qoder/adapter.ts @@ -4,6 +4,7 @@ import { mapReasoningEffort } from "../../reasoning-effort"; import { buildSystemPrompt } from "../coding-agent/protocol"; import { baseScopedEnv, runCodingAgentTurn, type CodingAgentDeps } from "../coding-agent/turn"; import { QODER_PROFILES, type QoderProfile } from "./profiles"; +import { QoderScaffoldFilter, QODER_SCAFFOLD_ERROR_CODE, qoderScaffoldErrorMessage } from "./scaffold-guard"; export type QoderAdapterDeps = CodingAgentDeps; @@ -31,6 +32,73 @@ export function buildQoderArgs(parsed: OcxParsedRequest, provider: OcxProviderCo return args; } +/** + * Wrap the turn's outbound channel with the scaffolding guard (#4190). + * + * The vendor CLI can put its own agent layer into the text channel despite being launched + * with tools and MCP disabled, and the shared stream-json parser forwards a text delta + * without inspecting it. This is the last point that is still qoder-specific, so the guard + * sits here rather than in the parser every coding-agent CLI shares. + * + * A terminal event flushes both channels first. The held tail is text the filter could not + * yet prove was not the start of a marker; dropping it would truncate a legitimate answer, + * and swallowing an entire response before forwarding a "done" reads downstream as an empty + * completion rather than as the refusal it is. + */ +export function guardQoderScaffolding(emit: (event: AdapterEvent) => void): (event: AdapterEvent) => void { + const textFilter = new QoderScaffoldFilter(); + const thinkingFilter = new QoderScaffoldFilter(); + let closed = false; + + const refuse = (reason: string): void => { + if (closed) return; + closed = true; + emit({ + type: "error", + message: qoderScaffoldErrorMessage(reason), + status: 502, + errorType: "upstream_error", + code: QODER_SCAFFOLD_ERROR_CODE, + // Intermittent, but a silent retry spends the operator's vendor credits on a + // contract violation the proxy cannot influence. Surface it instead. + retryable: false, + }); + }; + + return (event: AdapterEvent): void => { + if (closed) return; + if (event.type === "text_delta") { + const cleaned = textFilter.push(event.text); + if (cleaned.text) emit({ ...event, text: cleaned.text }); + if (cleaned.fail) refuse(cleaned.fail); + return; + } + if (event.type === "thinking_delta") { + const cleaned = thinkingFilter.push(event.thinking); + if (cleaned.text) emit({ ...event, thinking: cleaned.text }); + if (cleaned.fail) refuse(cleaned.fail); + return; + } + if (event.type === "done" || event.type === "error" || event.type === "incomplete") { + const tail = textFilter.flush(); + const reasoning = thinkingFilter.flush(); + if (tail.text) emit({ type: "text_delta", text: tail.text }); + if (reasoning.text) emit({ type: "thinking_delta", thinking: reasoning.text }); + const fail = tail.fail ?? reasoning.fail; + // A vendor error already carries the better explanation for why the turn ended; + // only a terminal that claims success is replaced. + if (fail && event.type !== "error") { + refuse(fail); + return; + } + closed = true; + emit(event); + return; + } + emit(event); + }; +} + export function createQoderAdapter(provider: OcxProviderConfig, deps: QoderAdapterDeps = {}): ProviderAdapter { return { name: "qoder", @@ -60,7 +128,7 @@ export function createQoderAdapter(provider: OcxProviderConfig, deps: QoderAdapt provider, parsed, incoming, - emit, + emit: guardQoderScaffolding(emit), buildArgs: (_profile, req, prov) => buildQoderArgs(req, prov), buildEnv: (profile, apiKey) => buildQoderChildEnv(profile as QoderProfile, apiKey), deps, diff --git a/src/adapters/qoder/scaffold-guard.ts b/src/adapters/qoder/scaffold-guard.ts new file mode 100644 index 0000000000..88c93d3e6d --- /dev/null +++ b/src/adapters/qoder/scaffold-guard.ts @@ -0,0 +1,173 @@ +/** + * Vendor-scaffolding guard for the qoder route (#4190). + * + * The qoder route is contractually a text and reasoning surface: the CLI is spawned with + * `--tools "" --strict-mcp-config --setting-sources ""`, and Codex keeps tool ownership. + * The vendor CLI does not always honour that. It has been observed emitting its own agent + * layer into the assistant text channel — an MCP lazy-loading `` block + * listing the local machine's configured MCP servers, and framework tool-call markup with a + * mismatched closer. Both reached the client verbatim, because the shared stream-json parser + * forwards a text delta without inspecting it. + * + * Two shapes, two answers. A complete `` block is recognizable and + * self-delimiting, so it is removed and the surrounding answer survives. Anything else that + * carries a scaffolding signature is not repairable by guesswork — a partial tool-call block + * has no reliable end, and the text around it may already be the vendor's own agent + * narration rather than the model's answer — so the turn fails closed instead. + * + * The filter is a stream, not a regex over a finished string: a marker can be split across + * deltas, so a tail that is still a possible marker prefix is held back rather than emitted. + * Callers must therefore `flush()` before forwarding a terminal event, or a legitimate + * answer ending in "<" would lose its last character. + */ + +/** Error code for a turn refused because vendor scaffolding reached the text channel. */ +export const QODER_SCAFFOLD_ERROR_CODE = "vendor_scaffold_detected"; + +const REMINDER_OPEN = "` opened and `` closed — mismatched, which is what + * a model emitting remembered markup looks like, and exactly why reconstructing the intended + * text is not possible. A stray `` with no opener is in the same class: + * the block it belonged to was already partly forwarded, or never existed. + */ +const UNREPAIRABLE_MARKERS = ["", REMINDER_CLOSE] as const; + +/** Every marker the scanner must be able to recognize mid-split. */ +const ALL_MARKERS = [REMINDER_OPEN, ...UNREPAIRABLE_MARKERS] as const; + +const MAX_MARKER_LENGTH = Math.max(...ALL_MARKERS.map(marker => marker.length)); + +/** + * Ceiling on a suppressed block before it is treated as unterminated. + * + * The block itself is discarded as it arrives, so this is not a memory bound — only the + * trailing bytes needed to spot a split closer are retained. It bounds how much of a turn a + * single unclosed reminder is allowed to swallow silently before the turn is refused. + */ +const MAX_SUPPRESSED_CHARS = 64 * 1024; + +/** Result of feeding one chunk: the text safe to forward, and a refusal reason once tripped. */ +export interface ScaffoldFilterResult { + /** Text cleared for the client. Empty when everything in the chunk was held or dropped. */ + text: string; + /** Non-null exactly once, on the chunk that trips the guard. */ + fail: string | null; +} + +/** Longest suffix of `text` that could still grow into one of the markers. */ +function heldSuffixLength(text: string): number { + const limit = Math.min(MAX_MARKER_LENGTH - 1, text.length); + for (let length = limit; length > 0; length--) { + const suffix = text.slice(text.length - length).toLowerCase(); + for (const marker of ALL_MARKERS) { + if (marker.length > length && marker.startsWith(suffix)) return length; + } + } + return 0; +} + +/** + * Streaming scaffolding filter for one channel (text or reasoning). + * + * One instance per channel: the two never share suppression state, so a reminder block + * opened in reasoning cannot swallow the answer text. + */ +export class QoderScaffoldFilter { + private mode: "pass" | "suppress" = "pass"; + private pending = ""; + private suppressedTail = ""; + private suppressedChars = 0; + private failed = false; + + push(chunk: string): ScaffoldFilterResult { + if (this.failed || !chunk) return { text: "", fail: null }; + let cleared = ""; + let buffer = this.mode === "pass" ? this.pending + chunk : chunk; + this.pending = ""; + + for (;;) { + if (this.mode === "suppress") { + const scan = this.suppressedTail + buffer; + const close = scan.toLowerCase().indexOf(REMINDER_CLOSE); + if (close < 0) { + this.suppressedChars += buffer.length; + if (this.suppressedChars > MAX_SUPPRESSED_CHARS) { + return this.fail(cleared, `an unterminated ${REMINDER_OPEN}> block`); + } + // The block is discarded as it arrives; only enough tail to spot a split closer is kept. + this.suppressedTail = scan.slice(Math.max(0, scan.length - (REMINDER_CLOSE.length - 1))); + return { text: cleared, fail: null }; + } + buffer = scan.slice(close + REMINDER_CLOSE.length); + this.mode = "pass"; + this.suppressedTail = ""; + this.suppressedChars = 0; + continue; + } + + let earliest = -1; + let found = ""; + const lowered = buffer.toLowerCase(); + for (const marker of ALL_MARKERS) { + const at = lowered.indexOf(marker); + if (at < 0) continue; + // A closer sitting exactly where an opener starts cannot happen, so ties are impossible. + if (earliest < 0 || at < earliest) { + earliest = at; + found = marker; + } + } + + if (earliest < 0) { + const held = heldSuffixLength(buffer); + cleared += held > 0 ? buffer.slice(0, buffer.length - held) : buffer; + this.pending = held > 0 ? buffer.slice(buffer.length - held) : ""; + return { text: cleared, fail: null }; + } + + // Text produced before the scaffolding is the model's own answer; it is kept either way. + cleared += buffer.slice(0, earliest); + if (found !== REMINDER_OPEN) return this.fail(cleared, `vendor tool-call markup (${found})`); + this.mode = "suppress"; + this.suppressedTail = ""; + this.suppressedChars = 0; + buffer = buffer.slice(earliest + REMINDER_OPEN.length); + } + } + + /** Release the held tail. Call before forwarding a terminal event, never mid-stream. */ + flush(): ScaffoldFilterResult { + if (this.failed) return { text: "", fail: null }; + if (this.mode === "suppress") return this.fail("", `an unterminated ${REMINDER_OPEN}> block`); + const text = this.pending; + this.pending = ""; + return { text, fail: null }; + } + + private fail(cleared: string, reason: string): ScaffoldFilterResult { + this.failed = true; + this.pending = ""; + this.suppressedTail = ""; + return { text: cleared, fail: reason }; + } +} + +/** + * Message for a refused turn. + * + * It names the marker class and nothing else. The leaked reminder in the report enumerated + * the operator's own MCP servers, so echoing the offending text back — into an error the + * client renders, and that a user may paste into an issue — would publish the thing this + * guard exists to contain. + */ +export function qoderScaffoldErrorMessage(reason: string): string { + return `Qoder CLI emitted ${reason} in the assistant text channel. This route runs the CLI with` + + " its own tools and MCP servers disabled and Codex owns tool control, so the turn was refused" + + " rather than forwarding vendor agent scaffolding to the client."; +} diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 486a6de544..ba4339c178 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -832,6 +832,7 @@ "proxy-liveness.test.ts": "server", "qoder-adapter.test.ts": "providers", "qoder-live-models.test.ts": "providers", + "qoder-scaffold-guard.test.ts": "providers", "quota-401-recovery-runtime.test.ts": "usage", "quota-401-recovery.test.ts": "usage", "quota-bars-rows.test.ts": "gui", diff --git a/tests/providers/qoder-scaffold-guard.test.ts b/tests/providers/qoder-scaffold-guard.test.ts new file mode 100644 index 0000000000..21ac6789d5 --- /dev/null +++ b/tests/providers/qoder-scaffold-guard.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, test } from "bun:test"; +import type { AdapterEvent } from "../../src/types"; +import { guardQoderScaffolding } from "../../src/adapters/qoder/adapter"; +import { + QoderScaffoldFilter, + QODER_SCAFFOLD_ERROR_CODE, +} from "../../src/adapters/qoder/scaffold-guard"; + +/** + * #4190: the qoder route is documented as a text and reasoning surface with the vendor CLI's + * own tools and MCP servers disabled, yet an MCP lazy-loading reminder listing the operator's + * configured servers, and vendor tool-call markup with a mismatched closer, reached the + * client as assistant text. + */ + +const REMINDER = "MCP lazy-loading is active.\n## Connected MCP servers\n" + + "- internal-notes\n- deploy-keys\nUse mcp_list / mcp_get / mcp_call."; + +const TOOL_MARKUP = "\ncd /srv/private && git status\n"; + +function collect(): { events: AdapterEvent[]; emit: (event: AdapterEvent) => void } { + const events: AdapterEvent[] = []; + return { events, emit: event => { events.push(event); } }; +} + +function textOf(events: AdapterEvent[]): string { + return events.map(event => (event.type === "text_delta" ? event.text : "")).join(""); +} + +describe("QoderScaffoldFilter", () => { + test("removes a complete reminder block and keeps the answer around it", () => { + const filter = new QoderScaffoldFilter(); + const first = filter.push(`Before.${REMINDER}After.`); + expect(first.fail).toBeNull(); + expect(first.text + filter.flush().text).toBe("Before.After."); + }); + + test("catches a marker split across deltas", () => { + const filter = new QoderScaffoldFilter(); + // The opening tag arrives in three pieces; a per-delta scan would miss it entirely. + const parts = ["Answer. secret server list Done."]; + const out = parts.map(part => filter.push(part)); + expect(out.every(result => result.fail === null)).toBe(true); + expect(out.map(result => result.text).join("") + filter.flush().text).toBe("Answer. Done."); + expect(out.map(result => result.text).join("")).not.toContain("secret server list"); + }); + + test("releases a held tail that never became a marker", () => { + const filter = new QoderScaffoldFilter(); + // "<" is a live marker prefix, so it cannot be forwarded until the stream ends. + const pushed = filter.push("compare a < b and a { + const filter = new QoderScaffoldFilter(); + const result = filter.push(`Checking the repositories.\n${TOOL_MARKUP}`); + expect(result.text).toBe("Checking the repositories.\n"); + expect(result.fail).toContain(" { + // The block it belonged to was already partly forwarded, or never existed. + expect(new QoderScaffoldFilter().push("tail").fail).toContain(""); + }); + + test("fails closed when a reminder is never terminated", () => { + const filter = new QoderScaffoldFilter(); + expect(filter.push("ok listing servers").fail).toBeNull(); + expect(filter.flush().fail).toContain("unterminated"); + }); + + test("latches: nothing more escapes after the guard trips", () => { + const filter = new QoderScaffoldFilter(); + expect(filter.push(TOOL_MARKUP).fail).not.toBeNull(); + expect(filter.push("more vendor narration")).toEqual({ text: "", fail: null }); + expect(filter.flush()).toEqual({ text: "", fail: null }); + }); +}); + +describe("guardQoderScaffolding", () => { + test("strips the reminder and still completes the turn", () => { + const { events, emit } = collect(); + const guarded = guardQoderScaffolding(emit); + guarded({ type: "text_delta", text: `Here is the status.${REMINDER}` }); + guarded({ type: "done", stopReason: "stop" }); + expect(textOf(events)).toBe("Here is the status."); + expect(textOf(events)).not.toContain("mcp_call"); + expect(events[events.length - 1]!.type).toBe("done"); + }); + + test("flushes the held tail before the terminal event", () => { + const { events, emit } = collect(); + const guarded = guardQoderScaffolding(emit); + // Without the flush this answer would arrive truncated, and an answer that is entirely + // held back would reach the empty-completion guard as a successful but empty turn. + guarded({ type: "text_delta", text: "1 < 2" }); + guarded({ type: "done", stopReason: "stop" }); + expect(textOf(events)).toBe("1 < 2"); + expect(events[events.length - 1]!.type).toBe("done"); + }); + + test("refuses the turn when tool-call markup leaks, and swallows the vendor's success", () => { + const { events, emit } = collect(); + const guarded = guardQoderScaffolding(emit); + guarded({ type: "text_delta", text: `Checking.\n${TOOL_MARKUP}` }); + guarded({ type: "done", stopReason: "stop" }); + expect(textOf(events)).toBe("Checking.\n"); + const terminal = events[events.length - 1]!; + expect(terminal.type).toBe("error"); + if (terminal.type !== "error") throw new Error("expected an error terminal"); + expect(terminal.code).toBe(QODER_SCAFFOLD_ERROR_CODE); + expect(terminal.status).toBe(502); + expect(terminal.retryable).toBe(false); + expect(terminal.message).not.toContain("git status"); + expect(terminal.message).not.toContain("mcp_call"); + expect(events.filter(event => event.type === "done")).toHaveLength(0); + }); + + test("guards the reasoning channel independently of the text channel", () => { + const { events, emit } = collect(); + const guarded = guardQoderScaffolding(emit); + guarded({ type: "thinking_delta", thinking: `Planning.${REMINDER}Continue.` }); + guarded({ type: "text_delta", text: "Answer." }); + guarded({ type: "done", stopReason: "stop" }); + const thinking = events.filter(event => event.type === "thinking_delta") + .map(event => event.type === "thinking_delta" ? event.thinking : "").join(""); + expect(thinking).toBe("Planning.Continue."); + expect(textOf(events)).toBe("Answer."); + }); + + test("forwards the vendor's own error rather than replacing it", () => { + const { events, emit } = collect(); + const guarded = guardQoderScaffolding(emit); + guarded({ type: "text_delta", text: "partial never closed" }); + guarded({ type: "error", message: "Qoder CLI exited with code 118", status: 429 }); + const terminal = events[events.length - 1]!; + expect(terminal.type).toBe("error"); + if (terminal.type !== "error") throw new Error("expected an error terminal"); + // The vendor said why the turn ended; the guard's job here was only to drop the block. + expect(terminal.message).toBe("Qoder CLI exited with code 118"); + expect(textOf(events)).toBe("partial "); + }); + + test("passes unrelated events through untouched", () => { + const { events, emit } = collect(); + const guarded = guardQoderScaffolding(emit); + guarded({ type: "tool_call_start", id: "call_1", name: "exec" }); + guarded({ type: "done", stopReason: "stop" }); + expect(events.map(event => event.type)).toEqual(["tool_call_start", "done"]); + }); +}); From 9cdcb43fea2d58757cf5a2d08e011700da842da8 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 09:09:23 +0900 Subject: [PATCH 2/3] fix(qoder): stop forwarding the region between a suppressed block and a refusal The guard keeps the text that precedes a scaffolding marker, on the reasoning that it is the model's own answer. That holds for the first marker in a turn. It does not hold once a reminder block has already been suppressed on that channel: the text between that block and the next unrepairable marker is the region the vendor was narrating in, and in the reported leak it carries the MCP server list. Two inputs published it and then refused the turn, which is the worst of both: a\n## Connected MCP servers\n- deploy-keys Status.\n- deploy-keys\n... The first forwarded "## Connected MCP servers\n- deploy-keys", the second "\n- deploy-keys\n", before emitting vendor_scaffold_detected. The prefix is now kept only while the channel has not yet suppressed a block, so an answer that simply precedes a leak still survives and a post-block region does not. Both inputs have a case. Refs #4190 --- src/adapters/qoder/scaffold-guard.ts | 12 +++++++++-- tests/providers/qoder-scaffold-guard.test.ts | 21 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/adapters/qoder/scaffold-guard.ts b/src/adapters/qoder/scaffold-guard.ts index 88c93d3e6d..f7a485f0d2 100644 --- a/src/adapters/qoder/scaffold-guard.ts +++ b/src/adapters/qoder/scaffold-guard.ts @@ -84,6 +84,8 @@ export class QoderScaffoldFilter { private suppressedTail = ""; private suppressedChars = 0; private failed = false; + /** True once a reminder block has been suppressed on this channel. */ + private suppressedBlock = false; push(chunk: string): ScaffoldFilterResult { if (this.failed || !chunk) return { text: "", fail: null }; @@ -131,9 +133,15 @@ export class QoderScaffoldFilter { return { text: cleared, fail: null }; } - // Text produced before the scaffolding is the model's own answer; it is kept either way. - cleared += buffer.slice(0, earliest); + // Text produced before the scaffolding is the model's own answer, and it is kept — but + // only while this channel has not already suppressed a block. Once it has, the text + // between that block and an unrepairable marker is not an answer that happens to + // precede a leak; it is the region the vendor was narrating in, and in the reported + // case it carries the MCP server list. Forwarding it on the way to a refusal would + // publish exactly what the refusal exists to contain. + if (found === REMINDER_OPEN || !this.suppressedBlock) cleared += buffer.slice(0, earliest); if (found !== REMINDER_OPEN) return this.fail(cleared, `vendor tool-call markup (${found})`); + this.suppressedBlock = true; this.mode = "suppress"; this.suppressedTail = ""; this.suppressedChars = 0; diff --git a/tests/providers/qoder-scaffold-guard.test.ts b/tests/providers/qoder-scaffold-guard.test.ts index 21ac6789d5..014387bdf6 100644 --- a/tests/providers/qoder-scaffold-guard.test.ts +++ b/tests/providers/qoder-scaffold-guard.test.ts @@ -67,6 +67,27 @@ describe("QoderScaffoldFilter", () => { expect(new QoderScaffoldFilter().push("tail").fail).toContain(""); }); + test("does not forward the region between a suppressed block and a refusal", () => { + // The text before the FIRST marker is the model's answer and is kept. The text after a + // block this filter already swallowed is the vendor's own narration, and in the reported + // leak that region is the MCP server list itself. + const filter = new QoderScaffoldFilter(); + const result = filter.push( + `a\n## Connected MCP servers\n- deploy-keys`, + ); + expect(result.text).toBe(""); + expect(result.text).not.toContain("deploy-keys"); + expect(result.fail).toContain(""); + }); + + test("does not forward vendor narration that sits between a reminder and tool markup", () => { + const filter = new QoderScaffoldFilter(); + const result = filter.push(`Status.${REMINDER}\n- deploy-keys\n${TOOL_MARKUP}`); + expect(result.text).toBe("Status."); + expect(result.text).not.toContain("deploy-keys"); + expect(result.fail).toContain(" { const filter = new QoderScaffoldFilter(); expect(filter.push("ok listing servers").fail).toBeNull(); From 99028b618a7872b2d31c53a8ee2d39dc25e3dd9e Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 09:18:18 +0900 Subject: [PATCH 3/3] fix(qoder): close three more scaffolding leaks in the routed-output guard An adversarial read of the filter found three inputs that still put MCP server names or a vendor command into the text channel. A nested reminder ended at the inner closer, so the outer block's remaining body was forwarded as the model's answer and the turn SUCCEEDED. Suppression now counts depth and only the closer that unwinds the last level ends it; the retained tail widened to the longest marker so a split opener is still seen. A closer with no opener kept the text ahead of it. That text is the lost block's body, not an answer that happened to precede a leak, so a closing marker no longer contributes a prefix. "", so the command inside such a block shipped before the refusal. "" is now its own marker, which leaves "" so a truncated or self-closed tag still suppresses, which made "" in an ordinary answer open a block and refuse the turn. It now needs a token boundary. Refs #4190 --- src/adapters/qoder/scaffold-guard.ts | 62 ++++++++++++++++++-- tests/providers/qoder-scaffold-guard.test.ts | 56 ++++++++++++++++++ 2 files changed, 113 insertions(+), 5 deletions(-) diff --git a/src/adapters/qoder/scaffold-guard.ts b/src/adapters/qoder/scaffold-guard.ts index f7a485f0d2..8a1b6df620 100644 --- a/src/adapters/qoder/scaffold-guard.ts +++ b/src/adapters/qoder/scaffold-guard.ts @@ -36,13 +36,29 @@ const REMINDER_CLOSE = ""; * text is not possible. A stray `` with no opener is in the same class: * the block it belonged to was already partly forwarded, or never existed. */ -const UNREPAIRABLE_MARKERS = ["", REMINDER_CLOSE] as const; +const UNREPAIRABLE_MARKERS = ["", "", REMINDER_CLOSE] as const; + +/** Markers that end a block rather than start one; their prefix is never an answer. */ +const CLOSING_MARKERS = new Set(["", REMINDER_CLOSE]); /** Every marker the scanner must be able to recognize mid-split. */ const ALL_MARKERS = [REMINDER_OPEN, ...UNREPAIRABLE_MARKERS] as const; const MAX_MARKER_LENGTH = Math.max(...ALL_MARKERS.map(marker => marker.length)); +/** + * True when `` so a truncated or self-closed tag still suppresses, + * which means `` in an ordinary answer would otherwise open a block and + * refuse the turn. A stem running to the end of the buffer still counts: more text may be + * arriving, and reading it as prose is the one reading that could release the block body. + */ +function reminderOpensHere(lowered: string, at: number): boolean { + const after = lowered[at + REMINDER_OPEN.length]; + return after === undefined || /[\s/>]/.test(after); +} + /** * Ceiling on a suppressed block before it is treated as unterminated. * @@ -86,6 +102,8 @@ export class QoderScaffoldFilter { private failed = false; /** True once a reminder block has been suppressed on this channel. */ private suppressedBlock = false; + /** Open reminder blocks; only the closer that unwinds the last one ends suppression. */ + private suppressDepth = 0; push(chunk: string): ScaffoldFilterResult { if (this.failed || !chunk) return { text: "", fail: null }; @@ -96,14 +114,40 @@ export class QoderScaffoldFilter { for (;;) { if (this.mode === "suppress") { const scan = this.suppressedTail + buffer; - const close = scan.toLowerCase().indexOf(REMINDER_CLOSE); + const scanned = scan.toLowerCase(); + // Unwind nesting rather than ending at the first closer. A reminder containing another + // reminder would otherwise hand the outer block's remaining body — the MCP server list + // in the reported leak — to the client as the model's answer, with a successful + // terminal and nothing to signal that anything had gone wrong. + let cursor = 0; + let close = -1; + for (;;) { + const nextClose = scanned.indexOf(REMINDER_CLOSE, cursor); + if (nextClose < 0) break; + let nextOpen = scanned.indexOf(REMINDER_OPEN, cursor); + while (nextOpen >= 0 && !reminderOpensHere(scanned, nextOpen)) { + nextOpen = scanned.indexOf(REMINDER_OPEN, nextOpen + 1); + } + if (nextOpen >= 0 && nextOpen < nextClose) { + this.suppressDepth += 1; + cursor = nextOpen + REMINDER_OPEN.length; + continue; + } + this.suppressDepth -= 1; + cursor = nextClose + REMINDER_CLOSE.length; + if (this.suppressDepth === 0) { + close = nextClose; + break; + } + } if (close < 0) { this.suppressedChars += buffer.length; if (this.suppressedChars > MAX_SUPPRESSED_CHARS) { return this.fail(cleared, `an unterminated ${REMINDER_OPEN}> block`); } // The block is discarded as it arrives; only enough tail to spot a split closer is kept. - this.suppressedTail = scan.slice(Math.max(0, scan.length - (REMINDER_CLOSE.length - 1))); + // The tail must cover a split opener too, now that nesting is counted. + this.suppressedTail = scan.slice(Math.max(0, scan.length - (MAX_MARKER_LENGTH - 1))); return { text: cleared, fail: null }; } buffer = scan.slice(close + REMINDER_CLOSE.length); @@ -117,7 +161,10 @@ export class QoderScaffoldFilter { let found = ""; const lowered = buffer.toLowerCase(); for (const marker of ALL_MARKERS) { - const at = lowered.indexOf(marker); + let at = lowered.indexOf(marker); + while (at >= 0 && marker === REMINDER_OPEN && !reminderOpensHere(lowered, at)) { + at = lowered.indexOf(marker, at + 1); + } if (at < 0) continue; // A closer sitting exactly where an opener starts cannot happen, so ties are impossible. if (earliest < 0 || at < earliest) { @@ -139,10 +186,15 @@ export class QoderScaffoldFilter { // precede a leak; it is the region the vendor was narrating in, and in the reported // case it carries the MCP server list. Forwarding it on the way to a refusal would // publish exactly what the refusal exists to contain. - if (found === REMINDER_OPEN || !this.suppressedBlock) cleared += buffer.slice(0, earliest); + // A closer with no opener never keeps its prefix either: the block it belonged to was + // already partly forwarded or never existed, so the text ahead of it is that body. + if (!CLOSING_MARKERS.has(found) && (found === REMINDER_OPEN || !this.suppressedBlock)) { + cleared += buffer.slice(0, earliest); + } if (found !== REMINDER_OPEN) return this.fail(cleared, `vendor tool-call markup (${found})`); this.suppressedBlock = true; this.mode = "suppress"; + this.suppressDepth = 1; this.suppressedTail = ""; this.suppressedChars = 0; buffer = buffer.slice(earliest + REMINDER_OPEN.length); diff --git a/tests/providers/qoder-scaffold-guard.test.ts b/tests/providers/qoder-scaffold-guard.test.ts index 014387bdf6..65f06d7bc1 100644 --- a/tests/providers/qoder-scaffold-guard.test.ts +++ b/tests/providers/qoder-scaffold-guard.test.ts @@ -88,6 +88,62 @@ describe("QoderScaffoldFilter", () => { expect(result.fail).toContain(" { + // Ending at the first closer handed the outer block's remaining body to the client as + // the model's answer, with a successful terminal and no signal that anything was wrong. + const filter = new QoderScaffoldFilter(); + const result = filter.push( + "outerinner\n## Connected MCP servers\n- deploy-keys", + ); + expect(result.text).toBe(""); + const flushed = filter.flush(); + expect(flushed.text).not.toContain("deploy-keys"); + expect(flushed.fail).toContain("unterminated"); + }); + + test("keeps the answer after a nested reminder that does close", () => { + const filter = new QoderScaffoldFilter(); + const result = filter.push( + "oi- deploy-keys Done.", + ); + expect(result.text).toBe(" Done."); + expect(result.fail).toBeNull(); + }); + + test("counts nesting even when the tags are split across deltas", () => { + const filter = new QoderScaffoldFilter(); + const parts = ["oi- deploy-keys Done."]; + const out = parts.map(part => filter.push(part)); + expect(out.map(result => result.text).join("")).toBe(" Done."); + expect(out.every(result => result.fail === null)).toBe(true); + }); + + test("a closer with no opener forwards nothing ahead of it", () => { + // The prefix of a stray closer is the lost block's body, not an answer that preceded it. + const filter = new QoderScaffoldFilter(); + const result = filter.push("## Connected MCP servers\n- deploy-keys"); + expect(result.text).toBe(""); + expect(result.fail).toContain(""); + expect(new QoderScaffoldFilter().push("cd /srv/private && git status").text).toBe(""); + }); + + test("catches an invoke block that carries no attributes", () => { + // "", so the command shipped ahead of the refusal. + const filter = new QoderScaffoldFilter(); + const result = filter.push("Checking.\n\ncd /srv/private && git status\n"); + expect(result.text).toBe("Checking.\n"); + expect(result.text).not.toContain("git status"); + expect(result.fail).toContain(""); + }); + + test("does not open a block on a word that merely starts with the tag name", () => { + // The opener is matched without its ">", so it needs a token boundary of its own. + const filter = new QoderScaffoldFilter(); + const result = filter.push("the are documented"); + expect(result.text + filter.flush().text).toBe("the are documented"); + expect(result.fail).toBeNull(); + }); + test("fails closed when a reminder is never terminated", () => { const filter = new QoderScaffoldFilter(); expect(filter.push("ok listing servers").fail).toBeNull();