fix(codebuddy): refuse leaked vendor tool-call scaffolding (#4596)

[lidge-jun]

Summary

Carries #4627 by @Ingwannu for #4596, with review changes described below.

The CodeBuddy route launches the vendor CLI with --tools "" and --strict-mcp-config, so the routed model has no native tool channel and writes its tool call as prose. src/adapters/coding-agent/protocol.ts filters frame types but never inspects frame contents, so that markup was forwarded as an ordinary assistant answer — the same single line of trust #4190 found for Qoder.

Qoder's shipped guard does not match this leak. The tags are wrapped in FULLWIDTH VERTICAL LINE (U+FF5C), e.g. <｜｜DSML｜｜ invoke name="functions.exec">, which none of the shipped UNREPAIRABLE_MARKERS cover. A signature extension was required, not a port of the existing filter.

The hard requirement was not over-blocking

Refusing on the marker alone would reject a legitimate answer that merely explains this protocol — a user asking what the tag means, a documentation answer containing a fenced example, or source code carrying the literal. That is a worse failure than the one being fixed, because it breaks correct answers.

Refusal therefore requires the observed two-line grammar, not the marker:

• a calls control line at column zero,
• outside a Markdown fence,
• immediately followed by an invoke line naming a functions.* tool.

Every quotation form carries extra syntax before the tag — inline code, a quoted string, a blockquote, indented source, a fenced block, or surrounding prose — so all of them are forwarded untouched. A lone calls tag with no invoke line is harmless and passes. Both directions are tested.

A detected leak preserves the answer text already proven safe, emits one non-retryable vendor_scaffold_detected error, and suppresses the vendor's later success terminal so the client never sees a completed turn. Markers split across streamed deltas are caught by holding only a bounded suffix that could still complete a control sequence or a fence; unrelated pending text is released at the next mismatch or at the terminal, so normal output is never withheld indefinitely. The reasoning channel is guarded independently of text.

Leaked prose is never promoted into a real tool call. The text channel carries no authenticated call envelope and no validated arguments, so converting it would manufacture execution authority out of model output. Failing closed is the right answer here, matching the Qoder precedent.

Kept CodeBuddy-owned rather than lifted into the shared coding-agent path — the same containment #4234 chose for Qoder, and what #4190's lane packet explicitly asked for: driven by a report, not by symmetry.

Review disposition from #4627

Accepted: the adapter-local placement, the fail-closed non-retryable error contract, the preserved answer prefix, the independent reasoning guard, split-delta buffering, success-terminal suppression, and CodeRabbit's findings on cross-channel terminal ordering, replacement-tail ordering, queuing events behind unresolved tails, and empty-delta slot preservation.

Rejected, with reasons: the original marker-only detector, because it refuses legitimate discussion and code examples and so fails the over-blocking requirement outright; promoting leaked prose into a real tool call; and lifting the filter into the shared coding-agent path.

Closes #4596

Verification

Static source review only, plus hosted CI. No local suite, typecheck, or build was run — the repository owner prohibits local suite execution in this lane after a past local run deleted real ~/.opencodex data. The verification claims in #4627's description are that author's, not re-asserted here.

Static checks performed:

• Read the detector's full state machine against the two verbatim leaks captured in [codebuddy] Routed tool-call markup reaches the client as assistant text (no scaffolding guard, unlike #4190) #4596, including the second turn's empty parameter body.
• Walked the fence/column-zero/prefix-hold logic for each quotation form the over-blocking requirement names, confirming each takes a path that forwards text unchanged.
• Confirmed the adapter event union and call path accept the new filter result, and that the guard is reached on both emit sites the issue identifies.
• Confirmed no existing test was deleted or weakened in this layer.
• git diff --check clean.

Regression coverage in tests/providers/codebuddy-adapter.test.ts, covering both directions:

Refusal — refuses a full-message DSML calls-and-invoke scaffold, detects a DSML control sequence split across streamed text deltas, refuses DSML calls-and-invoke scaffolding from reasoning independently.

Non-refusal (the over-blocking controls) — delivers a lone discussed DSML calls tag unchanged, delivers quoted and inline-code DSML literals unchanged, delivers a fenced DSML source example unchanged across deltas, delivers source strings containing both DSML literals unchanged, delivers an unquoted invoke line when no calls container precedes it.

Ordering and streaming — releases a lone control-line candidate at the terminal, queues later events behind an unresolved marker prefix, keeps an existing pending slot when its channel receives an empty delta, moves a replaced pending marker prefix to its new arrival position.

This is the tip of a stacked lane and deliberately carries no [skip ci]. Under the maintainer-approved DEV-STACK-08 tip-only policy, the CI run on this branch is the gate for every layer beneath it, and this branch contains all of them: codex/pw1-vision-sidecar-truncation (#4752), codex/pw2-deepseek-reasoning-replay (#4769), codex/pw3-kimi-tool-adjacency (#4770), codex/pw4-custom-baseurl-join (#4773), codex/pw5-bounded-tool-wire-names (#4774).

Known limitation: the detector is deliberately narrow and would not catch a future vendor variant that indents the control lines or inserts blank lines between them. Neither shape appears in #4596, and widening it would cost the over-blocking guarantee, so that is left for a report rather than speculation.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. (docs-site provider guide and structure/providers/chat-compat.md both record the refusal contract and the tools-disabled ownership boundary)
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (the change strictly reduces trust: vendor prose can no longer reach the client as an answer, and it is explicitly never converted into an executable call; the error carries no command text)

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• ^dev$
• ^preview$

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f207e159-7df3-4490-9298-08e08061e125

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-16T02:40:56.616887Z	aef65bd	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[lidge-jun]

리뷰 · 우선순위 78 / 80

이 PR은 CodeBuddy 경로에서 벤더 CLI가 도구 채널이 꺼진 채로(--tools "" + --strict-mcp-config) 돌 때, 모델이 도구 호출을 글자(프로즈) 로 써서 클라이언트에 그대로 흘러가던 문제를 막습니다. 이슈 #4596에 찍힌 실제 누출은 전각 세로줄(U+FF5C)로 감싼 DSML calls / invoke name="functions.*" 두 줄 문법인데, Qoder용 UNREPAIRABLE_MARKERS(#4234 / src/adapters/qoder/scaffold-guard.ts)로는 이 태그를 잡지 못합니다. 그래서 공유 coding-agent가 아니라 CodeBuddy 전용 src/adapters/codebuddy/scaffold-guard.ts를 새로 두고, adapter.ts의 emit을 guardCodeBuddyScaffolding(emit)으로 감쌉니다. 지금 dev(HEAD b3035fe29, #4720 Codex account/catalog 레인 착지 직후)에도 CodeBuddy 쪽에는 아직 adapter.ts/profiles.ts만 있고 가드가 없으므로, 이 변경은 현재 헤드 기준으로도 그대로 필요한 보안·신뢰 경계입니다.

중요한 설계 선택은 “마커만 보이면 거절”이 아니라 관측된 두 줄 문법일 때만 거절한다는 점입니다. 줄 맨 앞(column zero)이고, 마크다운 펜스 밖이며, calls 바로 다음에 functions.* invoke가 와야 합니다. 그래서 태그를 설명하는 문장, 인라인 코드, 인용, 펜스 예시, 소스 문자열은 그대로 통과합니다. 거절이 나면 이미 안전한 앞부분은 남기고, 재시도 불가(retryable: false)인 vendor_scaffold_detected 에러 하나만 내며, 뒤따르는 벤더 success done은 막아서 클라이언트가 “성공한 턴”으로 보지 않게 합니다. 유출된 프로즈를 진짜 tool call로 올리는 일은 하지 않습니다 — 인증된 호출 봉투가 없는 텍스트를 실행 권한으로 바꾸면 안 되기 때문입니다. 이 방향은 #4190 레인 패킷이 말한 “보고가 오면 CodeBuddy도 모듈로, 대칭을 위해 미리 올리지 말 것”과도 맞습니다.

원본 기여자 PR #4627(Ingwannu)을 리뷰 반영해 가져왔고, Co-authored-by가 커밋에 남아 있습니다. 파일은 가드·어댑터 한 줄 연결·tests/providers/codebuddy-adapter.test.ts 양방향(거절/비거절/스트리밍 순서)·docs-site providers 가이드·structure/providers/chat-compat.md까지 다섯 개로 범위가 좁습니다. 스택 tip이라 [skip ci]가 없고, 아래 층 #4752→#4769→#4770→#4773→#4774 전체를 tip CI가 덮는 DEV-STACK-08 정책과 맞습니다. 로컬 스위트는 돌리지 않았다는 고지도 #4720 이후 레포 규칙과 일치합니다.

라인 단위로 보면 큰 구멍은 거의 없고, 운영·스택 쪽 주의가 더 큽니다.

src/adapters/codebuddy/scaffold-guard.ts scan() - 의도적으로 좁음. 들여쓴 제어줄이나 calls와 invoke 사이 빈 줄은 안 잡힘. #4596 로그에는 없는 형태라 지금 확장하면 오탐(설명/예시 거절) 위험이 커지므로, 새 리포트 오기 전엔 유지가 맞음.
guardCodeBuddyScaffolding refuse() - 거절 후 closed=true로 이후 done을 버림. success 터미널 억제 계약은 테스트로 고정돼 있음. 상태 코드 502/upstream_error는 Qoder와 같은 계열이라 클라이언트 매핑만 한 번 확인하면 됨.
adapter.ts emit 래핑 - text/thinking만 가드하고 tool_call_* 등은 큐 순서만 맞춤. 지금 라우트가 도구 채널을 안 여는 v1 전제와 맞음.
베이스 codex/pw5-bounded-tool-wire-names (#4774) - dev가 아님. tip CI가 스택 게이트. 또한 현재 origin/dev(b3035fe / #4720)가 이 브랜치 조상이 아니라서, 부모들이 랜딩된 뒤 tip을 dev에 올릴 때 리베이스/충돌 여부를 한 번 봐야 함.
#4627 - 같은 주제의 원본 PR이 아직 OPEN. 이 tip이 머지되면 Landed via #4776 후 superseded로 닫고, 이슈 #4596도 함께 닫을 차례.

메인테이너의 판단이 필요한 지점

• 스택 순서: fix(vision): fail closed on truncated sidecar captions (#4727) #4752 → [WRONG BRANCH] fix(responses): refuse a combo failover that cannot replay mandatory reasoning (#4696) #4769 → [WRONG BRANCH] fix(providers): give Kimi the Responses tool-result adjacency repair (#4726) #4770 → [WRONG BRANCH] fix(catalog): normalize the custom-provider model-discovery join (#4724) #4773 → fix(openai-chat): bound flattened tool wire names for strict gateways (#4679) #4774 → fix(codebuddy): refuse leaked vendor tool-call scaffolding (#4596) #4776(tip) 을 유지할지, chore(lanes): land the Codex account and catalog lane (nine PRs) #4720 착지 이후 tip만 dev에 다시 올릴지.
• #4627을 tip 머지와 동시에 closed/superseded 처리할지(권장), 아니면 tip 전에 duplicate로 먼저 닫을지.
• 좁은 탐지기를 새 DSML 변형 리포트가 오기 전까지 freeze할지.

너의 추천
tip CI(호스티드, 이 브랜치 SHA) 통과를 확인하고, 부모 #4774까지 랜딩한 뒤 #4776을 머지하세요. 머지 직후 #4596을 닫고, #4627에는 Landed via #4776 at <commit> + landed-via-maintainer로 superseded 클로즈하세요. 탐지기 확장은 하지 말고, 들여쓰기/빈줄 변형이 실제로 잡히면 그때 이슈로 받으세요. 지금은 KEEP.

이 댓글은 grok-bot이 작성했습니다

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aef65bde8c

ℹ️ 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".

[chatgpt-codex-connector]

Track the full Markdown fence delimiter

When an answer uses a valid four-backtick outer fence to quote a three-backtick fenced example, this scanner consumes only the first three opening backticks and then treats the inner ```text line as the closing fence, even though it has trailing content. The subsequent DSML example is therefore parsed as live control markup and the turn incorrectly ends with vendor_scaffold_detected, contradicting the intended guarantee that fenced examples pass unchanged. Record the opening delimiter length and only close on a delimiter of at least that length with a valid closing-fence suffix.

Useful? React with 👍 / 👎.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[lidge-jun]

Landing the provider wire-compatibility lane, top layer first. Routed tool-call markup no longer reaches the client as assistant text. The detector is deliberately scoped to the two-line syntax actually reported in #4596; widening it to hypothetical variants would trade away the guarantee that ordinary code and prose are never over-blocked.

Evidence at the verified tip 49f815d (tree cea66da56f10b8d1aee2290fa185d321442eab98), from run 35061163092:

• test 1-4/4 and macos 1-2/2 all completed with conclusion success, confirmed through the check-runs API rather than the check rollup, so the heavy jobs actually executed and were not path-filtered. gates, changes, storage policy, api usage, docker smoke, keyring and npm-global on three platforms, the three service-lifecycle jobs, and the aggregate ci check all succeeded.
• The ci failure at this commit belongs to run 35061161660, which this push superseded; run 35061163092 is the live one and it concluded success.
• The lane absorbed dev at cf6e939 from the bottom layer upward, so each pull request keeps its own layer diff (4 / 9 / 6 / 7 / 7 / 5 files) and no dev commit appears in any layer's diff.
• The single conflict was structure/transports/responses.md, where both sides appended a new section to the same empty base. It was resolved by keeping both: the section count goes from 14 on dev to 15 here, and every dev section name is still present. That loss is the kind CI cannot detect, so the names were compared directly rather than trusting the count.
• The file-size ratchet reports no offenders after the absorption; openai-chat.ts sits exactly at its 822 cap.
• git merge-tree --write-tree origin/dev <tip> reports a clean merge, and origin/dev is itself an ancestor of this tip.
• Ancestry verified so each layer closes as MERGED: pw1 through pw5 are all ancestors of this tip.

Maintainer integration decision under MAINTAINERS.md / AGENTS.md: a maintainer with maintain or admin access may integrate into dev without a second maintainer approval, recording the decision and exact-head CI evidence.