fix(qoder): refuse vendor CLI scaffolding in routed output - #4234
Conversation
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 <system-reminder> enumerating the operator's configured MCP servers, and tool-call markup opened as <functions.exec> and closed as </invoke>, 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 <system-reminder> 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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 785a3a2985
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const lowered = buffer.toLowerCase(); | ||
| for (const marker of ALL_MARKERS) { | ||
| const at = lowered.indexOf(marker); |
There was a problem hiding this comment.
When a Qoder delta contains a length-expanding Unicode character before a marker, such as U+0130 (İ), lowered.indexOf() returns an offset into the longer lowercased string, but that offset is then applied to the original buffer. For example, a run of İ characters before <system-reminder> causes cleared to include the opening tag and part or all of the protected MCP inventory before the turn is refused. Use a case-insensitive search that preserves original-string offsets, or explicitly map matched offsets back to the source string. This defeats the secret-containment purpose of the guard and is therefore release-blocking.
AGENTS.md reference: AGENTS.md:L357-L363
Useful? React with 👍 / 👎.
| 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 }); |
There was a problem hiding this comment.
When a reasoning delta ends with a possible marker prefix such as < and the next event starts answer text, thinkingFilter retains that character while textFilter immediately emits the answer; the terminal flush then emits the retained reasoning after the answer. The bridge consequently sees reasoning, text, then a second reasoning item, changing the provider's event order and replay semantics. Flush a channel's pending suffix before forwarding a later semantic event from another channel, or retain pending fragments in one ordered event buffer.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
리뷰 · 우선순위 70 / 80이 PR은 L6 스트리밍/툴 레인에서 #4190을 닫는 작업입니다. 지금 이 PR은 그 내용을 qoder 어댑터의 마지막 지점에서 막습니다. 새 베이스 브랜치 (
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
… 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:
<system-reminder>a</system-reminder>\n## Connected MCP servers\n- deploy-keys</system-reminder>
Status.<reminder block>\n- deploy-keys\n<functions.exec>...</invoke>
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
…uard 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. "<invoke name=" did not match a bare "<invoke>", so the command inside such a block shipped before the refusal. "<invoke>" is now its own marker, which leaves "<invoked" in prose alone. Separately, the opener is matched without its ">" so a truncated or self-closed tag still suppresses, which made "<system-reminders>" in an ordinary answer open a block and refuse the turn. It now needs a token boundary. Refs #4190
1 participant
Summary
The qoder route is contractually a text and reasoning surface.
buildQoderArgslaunches the CLI with--tools "" --strict-mcp-config --setting-sources "" --max-turns 1 --no-session-persistence, and bothcoding-agent/protocol.tsandcoding-agent/turn.tsstate that Codex retains tool control and vendor tools are never invoked.The vendor CLI does not always honour that. Two things reached the client as assistant text: an MCP lazy-loading
<system-reminder>block enumerating the operator's configured MCP servers from~/.qoder/mcp.jsonand from plugins, and vendor tool-call markup opened as<functions.exec>and closed as</invoke>— 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:
mapRawStreamEventfilters frame types and forwards a text delta's contents unread, so whatever the vendor's agent layer puts in the text channel is relayed. The strongest version of this leak publishes the operator's MCP server inventory to whoever is reading the turn.This adds a streaming scaffolding filter in
src/adapters/qoder/scaffold-guard.tsand wraps theemitcallback the qoder adapter hands torunCodingAgentTurn. 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.Two shapes, two answers:
<system-reminder>…</system-reminder>block is recognizable and self-delimiting, so it is removed and the answer around it survives.<functions.,<invoke name=,</invoke>, or a</system-reminder>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. The error names the marker class and nothing else: echoing the offending text back, into a message a user may paste into an issue, would publish the thing the guard exists to contain.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 non-obvious way:
isContentEventinempty-completion-guard.tscounts only non-emptytext_deltas as content, so an answer swallowed in full and followed bydonewould reach the client as a successful but empty turn instead of the refusal it is. 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.Decisions the issue left open, and how they were called:
tool_useframes are left alone.mapRawStreamEventmaps them totool_call_start. That is a typed frame rather than leaked text, the issue reports the text channel, andprotocol.tsdocuments that seam as deliberately prepared for a future tool bridge.<system-reminder>or<functions.…>syntax will be stripped or refused. That is the accepted cost of failing closed on a route whose leak publishes the operator's MCP inventory.@qoder-ai/qoderclibundle still contains the reminder builder and appears to initialize it despite the flags, so that fix lives in the vendor. This guard is the containment that does not depend on the vendor agreeing.Design notes are in
devlog/_plan/260911_l6_streaming_tools/020_4190_qoder_scaffold_guard.md.Closes #4190
Verification
bun run test,bun run test:changed,bun run typecheckandbun run build:gui: NOT RUN, by operator instruction for this dispatch round. Hosted CI on the exact pushed head is the only product evidence this change offers.tests/providers/qoder-scaffold-guard.test.ts, beside the existingqoder-adapter.test.ts, registered inscripts/test-layout/layout.jsonandtests/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 after a trip, and the wrapper's terminal handling including the flush-before-donerule 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.coding-agent/turn.ts: every client-visible event for this route leaves through the callback passed torunCodingAgentTurn, including the early failures before spawn and the post-reap terminals.Checklist
Stacked on #4232 (targets
codex/260911-l6-streaming-tools); retarget todevonce the parent lands.