fix(responses): compile a recognized apply_patch wrapper from the body it validated

[codingbooo]

Summary

resolveCodeModeHelperName recognizes several wrapper shapes for a bare exec body (#4983) — the
{input} function wrapper, exactly one of exec's fallback fields, and one complete outer Markdown
fence — by reading the body through unwrapFreeformToolInput(..., "exec") and testing that reading
for a complete envelope. Compilation was handed the pre-repair text instead, and its
apply_patch branch unwrapped only input and patch. So the wrapper the body arrived in reached
the tool as the patch:

{code} fallback  -> tools.apply_patch("{\"code\":\"*** Begin Patch\\n*** Update File: ...\"}")
outer fence      -> tools.apply_patch("```diff\n*** Begin Patch\n...\n```")

The generated JavaScript passes values as data and never interpolates them as source, so this was a
wrong patch rather than an escape — but it was still the edit the model asked for turning into a
rejected one, and the failure was silent, because recognition had already committed to the
apply_patch helper.

Recognition and compilation now read one body. When the recognizer's own reading is a complete
envelope, that reading is the canonical patch; when it is not, compilation keeps the name-based
path's narrower input/patch unwrap, byte for byte. An envelope test gates that choice
deliberately: {patch} is not an exec fallback key so it is never recognized from the body, and a
lone fallback field that is not a patch keeps its previous answer.

Where I deviated from the review's suggestion, so you can push back. The review recommended
option A — each of the four call sites passing the unwrapped value into the compiler. I put the
canonical reading inside the compile boundary (canonicalApplyPatchBody) instead. Both satisfy
"one canonical repaired body", but option A leaves the boundary open for a fifth caller to
reintroduce exactly this gap by forgetting to pass it down, and forgetting to pass it down is what
this bug is. It unpacks back to option A in place if you prefer the caller-owned shape.

Closes #5046

Verification

Reproduced before touching anything — on dev, four of six accepted shapes compiled the wrapper:

bare canonical       helper=apply_patch  compilesToRawPatch=true
{input} wrapped      helper=apply_patch  compilesToRawPatch=true
{code} fallback      helper=apply_patch  compilesToRawPatch=false
{script} fallback    helper=apply_patch  compilesToRawPatch=false
{content} fallback   helper=apply_patch  compilesToRawPatch=false
outer markdown fence helper=apply_patch  compilesToRawPatch=false

All five required regressions are added to existing files, so no new test file needed registering:

• tests/responses/apply-patch-envelope.test.ts — all seven fallback fields compile to the same
  raw patch; fenced and unfenced compile identically; decorated delimiters normalize on every path
  and not only the bare one; an ambiguous or non-envelope body stays byte-exact; a normal
  code-mode JavaScript body is never compiled; a caller-defined non-code-mode exec is never
  reinterpreted.
• tests/adapters/bridge-legacy-shell-normalization.test.ts — the required bridge-level
  regression. It drives the real bridge, reads the delivered exec body back out of the SSE, and
  runs it with a stub tools, asserting what tools.apply_patch actually received. A
  recognizer-level assertion cannot see this gap, which is why the issue asked for it.

Reverse-verified, so these are not tautologies: with the source change stashed, 7 of the new
tests fail; with it, all pass.

Command	Result
bun x tsc --noEmit	pass (exit 0)
bun scripts/test.ts --changed=upstream/dev	11617 pass, 3 skip, 9 fail
bun scripts/structure-ssot.ts	structure/ SSOT checks passed
bun scripts/privacy-scan.ts	Privacy scan passed
bun scripts/file-size-ratchet.ts	file-size ratchet passed

The 9 failures are not from this change: they are provider management validation (7), a Windows
icacls spill case, and the large history index case. I ran the same three files on a pristine
upstream/dev worktree and got the identical 9, so they are pre-existing on this machine.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. — structure/transports/responses.md
  records the one-body rule at the wrapper-recovery boundary it already documents.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. — Values
  remain data, never interpolated source; the envelope predicate still gates the wider reading,
  so no non-patch body is reinterpreted.

🤖 Generated with Claude Code

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of apply_patch requests wrapped in supported function-call formats or Markdown fences.
  • Ensured recognized patch content is consistently delivered to the patch tool, regardless of the supported wrapper field used.
  • Preserved original content for ambiguous, incomplete, or non-patch inputs.
  • Prevented ordinary JavaScript and caller-defined tool behavior from being incorrectly interpreted as patch requests.

• Documentation

  • Added guidance describing supported wrapper recovery and ambiguity handling.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 6df1ac6f-0879-4fbd-9449-bb6241b43801

📥 Commits

Reviewing files that changed from the base of the PR and between 1be5cb3 and 9b83041.

📒 Files selected for processing (4)

• src/responses/code-mode-helper-compat.ts
• structure/transports/responses.md
• tests/adapters/bridge-legacy-shell-normalization.test.ts
• tests/responses/apply-patch-envelope.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The compiler now consumes the same canonical body that recognizes repaired apply_patch wrappers. Documentation and tests cover fallback fields, input wrappers, fences, decorated delimiters, ordinary JavaScript, caller-defined tools, and bridge execution.

Changes

Apply_patch compatibility

Layer / File(s)	Summary
Canonical body compilation src/responses/code-mode-helper-compat.ts, structure/transports/responses.md	compileCodeModeHelperInput now uses the validated freeform body for complete patch envelopes and retains the narrower input/patch fallback otherwise. The transport documentation describes this shared reading contract.
Wrapper and bridge regression coverage tests/responses/apply-patch-envelope.test.ts, tests/adapters/bridge-legacy-shell-normalization.test.ts	Tests cover fallback fields, wrappers, fences, decorated delimiters, non-envelope bodies, ordinary JavaScript, caller-defined exec, and execution through the bridge.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

• lidge-jun/opencodex#3498: Introduced the apply_patch envelope recognition and freeform input unwrapping used by this compilation fix.

Merge Risk: ⚪ Minimal · up to 9b830

The compatibility change preserves ordinary code and caller-defined tools while correctly forwarding recognized patch wrappers. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: compiling a recognized apply_patch wrapper from the validated body. It matches the stated objectives and changed files.
Linked Issues check	✅ Passed	The PR satisfies the coding requirements in #5046. In src/responses/code-mode-helper-compat.ts, canonicalApplyPatchBody reads argumentsText with unwrapFreeformToolInput(..., "exec") and uses t…
Out of Scope Changes check	✅ Passed	The changed source in src/responses/code-mode-helper-compat.ts directly implements #5046. The regression tests in tests/responses/apply-patch-envelope.test.ts and `tests/adapters/bridge-legacy-she…

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.}

[github-actions]

⏳ DRAFT

• review readiness checklist open (0/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

• ⬜ All CI tests are green on my local testing.
• ⬜ I pushed my PR to the latest dev commit.
• ⬜ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

Hygiene

✅ Deterministic PR hygiene checks passed.

[lidge-jun]

리뷰 · 우선순위 75 / 80

이 PR은 code-mode에서 exec 본문이 apply_patch로 이미 인식된 뒤, 컴파일이 아직 고치기 전 원문을 쓰던 어긋남을 고칩니다. 지금 dev HEAD(1be5cb3d8, tip #5039 fix(registry) deepseek-v4.1-flash natively image-capable, package 2.59.0)의 src/responses/code-mode-helper-compat.ts를 보면, resolveCodeModeHelperName은 unwrapFreeformToolInput(argumentsText, "exec")로 울타리와 fallback 필드를 본 다음 isCompletePatchEnvelope로 판정합니다. 그런데 같은 파일의 compileCodeModeHelperInput apply_patch 분기는 아직 좁은 unwrapPatchInput만 씁니다. 이 함수는 JSON의 input/patch만 보고, 마크다운 fence도 code/content 같은 exec fallback 키도 안 봅니다. 그래서 #4983이 넓혀 둔 인식(fence, fallback)을 통과한 본문이 호스트에 도착할 때는 여전히 감싸진 JSON이나 fence 문자열로 tools.apply_patch(...)에 들어가고, 패치 적용이 거절됩니다. 이슈 #5046이 바로 그 버그입니다.

고치는 위치는 컴파일 경계 안입니다. canonicalApplyPatchBody가 인식과 같은 unwrapFreeformToolInput(..., "exec")를 읽고, 그 결과가 완전한 패치 봉투면 그걸 정규 본문으로 쓰고, 아니면 이름 기반 경로가 쓰던 unwrapPatchInput을 바이트 그대로 유지합니다. 호출처 다섯 곳에 세 번째 인자를 넘기는 방식(옵션 A)이 아니라, 컴파일 함수 안에 정규 읽기를 넣어서 “다섯 번째 호출자가 인자 전달을 잊는” 재발을 막겠다는 선택입니다. PR 본문에 그 편차가 명시되어 있습니다. structure/transports/responses.md에도 “인식과 컴파일이 한 본문을 읽는다”는 한-body 규칙이 문서화됩니다.

테스트가 두 층입니다. tests/responses/apply-patch-envelope.test.ts는 인식·컴파일 쌍과 이름 기반 경로, fence 동일성, 장식 구분자 정규화, 모호/비봉투 본문의 바이트 보존, 일반 JS·비-code-mode exec 비재해석을 잡습니다. tests/adapters/bridge-legacy-shell-normalization.test.ts는 실제 브리지를 돌린 뒤 SSE에서 전달된 exec 본문을 꺼내 실행하고, stub tools.apply_patch가 받은 값이 래퍼가 아닌 raw 패치인지 확인합니다. 인식기만 보면 보이기 어려운 간극을 이슈가 요구한 방식으로 막은 점이 강합니다. 작성자 말에 따르면 소스 변경을 stash하면 신규 테스트 7개가 실패하고, 넣으면 통과합니다. 패치는 여전히 JSON.stringify로 데이터로만 들어가서, unwrap 범위를 넓혀도 provider 텍스트가 실행 JS로 새어 나가지 않습니다.

다만 같은 버그(#5046)를 고치는 열린 PR이 하나 더 있습니다. 메인테이너 본인 PR #5051(fix(responses): compile the apply_patch body recognition accepted)은 unwrapPatchInput을 제거하고 unwrapFreeformToolInput에 wireToolName을 넘기며 호출처를 같이 고칩니다. 이 PR(#5052)은 경계를 닫는 쪽이고, #5051은 호출자가 정규 본문을 넘기는 쪽입니다. types.ts/config.ts 대분할에 무효화될 PR은 아니고, Closes #5046이라 둘 중 하나만 머지되면 이슈는 닫힙니다. 둘 다 남기면 기여자·열린 PR 카운트가 혼란스러워집니다.

라인 canonicalApplyPatchBody - unwrapFreeformToolInput에 "exec"를 하드코딩합니다. 본문 인식 경로와는 맞지만, 이름이 이미 apply_patch로 확정된 이름 기반 경로에서 apply_patch 전용 어휘만 있는 본문({patch:...})은 봉투 판정에 실패하고 unwrapPatchInput으로 떨어집니다. 의도에 맞는 분기여도, #5051의 wireToolName 방식과 달리 “본문이 실제로 도착한 이름”을 컴파일 쪽에 드러내지는 않습니다. 경계를 유지할지 wire를 드러낼지는 메인테이너 선택이 필요합니다.

tests/responses/apply-patch-envelope.test.ts / tests/adapters/bridge-legacy-shell-normalization.test.ts - fallback 키 목록(code/script/js/javascript/command/cmd/content)이 테스트에 하드코딩되어 있습니다. FREEFORM_FALLBACK_KEYS.exec(src/responses/apply-patch-envelope.ts)에 키가 추가되면 테스트 목록도 같이 고쳐야 합니다. 상수에서 읽게 하면 더 튼튼합니다.

tests/adapters/bridge-legacy-shell-normalization.test.ts - AsyncFunction으로 전달 본문을 실행하는 회귀는 간극을 드러내는 데 맞고, 프로덕션 경로가 아니라 테스트 전용입니다. 다만 이 패턴이 다른 테스트로 복사될 때는 입력 출처를 고정된 fixture로만 두어야 합니다.

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

• #5046을 #5051로 닫을지, 이 PR(fix(responses): compile a recognized apply_patch wrapper from the body it validated #5052)로 닫을지 (둘 다 Closes [Bug]: repaired code-mode apply_patch wrappers compile the original malformed body #5046 — 하나만 머지)
• 정규 본문을 컴파일 경계 안에 둘지(fix(responses): compile a recognized apply_patch wrapper from the body it validated #5052), 호출자가 wireToolName을 넘기게 할지(fix(responses): compile the apply_patch body recognition accepted #5051 옵션 A)
• 이 PR의 브리지 실행 회귀를 #5051에 이식할지, 아니면 이 PR을 착륙 후보로 둘지
• hosted CI 초록만으로 머지할지

너의 추천
같은 이슈를 고치는 중복입니다. 메인테이너 PR #5051이 이미 열려 있고 wire 이름을 호출 경계에 드러내는 API가 더 명시적이므로, 기본은 #5051을 착륙하고 이 PR은 landed-via-maintainer / superseded로 닫기를 추천합니다. 다만 브리지에서 전달 본문을 실제 실행해 tools.apply_patch 인자를 검증하는 테스트는 #5051에 이식할 가치가 큽니다. 반대로 경계 소유(canonicalApplyPatchBody)와 브리지 실행 회귀를 한 번에 가져가고 싶으면 이 PR을 머지하고 #5051을 닫으면 됩니다. 어느 쪽이든 #5046은 하나만 Closes로 닫으세요.

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

[lidge-jun]

Landed via #5051 at d55aed2