Skip to content

fix(responses): repair native exec patch completion before Codex executes - #3701

Merged
lidge-jun merged 7 commits into
devfrom
codex/grok-responses-patch-5598
Sep 5, 2026
Merged

fix(responses): repair native exec patch completion before Codex executes#3701
lidge-jun merged 7 commits into
devfrom
codex/grok-responses-patch-5598

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Repair a complete patch mistakenly emitted as native custom exec before Codex receives input/item completion events, matching the terminal snapshot and Chat bridge behavior.

  • Hold helper-alias and ambiguous raw/JSON-wrapped patch previews until their executable form is known. Pretty-printed and escaped-key wrappers follow the same rule.

  • Preserve unrelated native custom payloads and arbitrary JavaScript byte-for-byte; block-leading JavaScript may wait until completion.

  • Stack layer 3, depends on fix(clients): converge owned Pi and Aside catalogs after selection changes #3699 and fix(clients): honor selected models in Pi and Aside catalogs #3698. A final layer will address the remaining ordinary-function and namespace parity gaps.

  • Replace a blocking test subprocess wait that twice stalled macOS CI with an asynchronous wait, a kill deadline, and bounded cleanup. The same state and exact-PID reaping assertions remain; this follow-up changes only the test helper.

Verification

  • Standalone lifecycle probe verifies held previews, identical input.done/item.done/terminal input, and exactly one canonical patch passed to a recording stub when generated code is executed. No real files were patched by the probe.

  • Added native raw/wrapped/pretty/escaped-key, alias, completion-variant, unrelated-payload, flat-catalog, foreign-namespace and cleanup regressions.

  • Remote focused regression verification and independent review are running. Hosted exact-head CI remains required before merge; no local suites or typecheck were executed.

  • Owner authorized --no-verify pushes and admin merge after verification.

  • The asynchronous probe file passed all 20 cases under pinned Bun 1.4 with --isolate on macmini-cf, including the stalled-child kill/reap case; root typecheck and independent source review passed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Arbitrary executable source is preserved; only existing complete-envelope/helper authorization is reused.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 202289b4-097b-4a70-867b-3fee0ddbd502

📥 Commits

Reviewing files that changed from the base of the PR and between 70b225b and 4893d0d.

📒 Files selected for processing (1)
  • tests/clients/client-connect.test.ts

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


📝 Walkthrough

Walkthrough

Native Responses exec calls now buffer possible patch previews until completion, compile valid patch envelopes into executable tool input, preserve unrelated payloads, and release retained state. Client state probes now use bounded asynchronous child processes. Tests and documentation cover both changes.

Changes

Native exec patch completion

Layer / File(s) Summary
Native exec streaming repair
src/server/responses-custom-tool-repair.ts
Native code-mode exec calls are tracked with helper aliases. Patch-like or ambiguous input is held until completion, while ordinary JavaScript remains byte-exact. Completion resolves and compiles helper input, then releases retained state.
Completed item restoration
src/responses/custom-tool-compat.ts
Unaliased native custom calls without a resolved helper retain raw input. Apply-patch delimiter normalization runs only for configured repair names.
Lifecycle validation and compatibility contracts
tests/responses/responses-custom-tool-repair.test.ts, structure/11_compatibility-contracts.md, docs-site/src/content/docs/guides/codex-integration.md, devlog/_plan/260906_grok_catalog_and_patch/030_responses_patch.md, devlog/_plan/260906_grok_catalog_and_patch/040_native_tool_parity.md
Tests cover fragmented previews, completion boundaries, unchanged inputs, foreign namespaces, cancellation, disposal, and budget cleanup. Documentation and plans record the completion contract and parity scope.

Asynchronous client state probing

Layer / File(s) Summary
Bounded asynchronous state probe
tests/clients/client-connect.test.ts
readStateProbe now uses piped asynchronous child processes, limits captured output to 1 MiB, enforces a deadline and cleanup timer, kills stalled children, and reports failures through ClientStateProbeError. Callers and stall tests now await the probe.

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

Merge Risk: ⚪ Minimal · up to 4893d

Native code-mode exec patch envelopes now resolve into executable apply_patch input at completion while unrelated custom payloads and ordinary JavaScript remain unchanged. No concrete merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant ResponsesStream
  participant responsesCustomToolRepair
  participant CodeModeHelper
  ResponsesStream->>responsesCustomToolRepair: Send native exec input deltas
  responsesCustomToolRepair->>responsesCustomToolRepair: Hold patch-like input
  ResponsesStream->>responsesCustomToolRepair: Send completion event
  responsesCustomToolRepair->>CodeModeHelper: Resolve and compile helper input
  CodeModeHelper-->>responsesCustomToolRepair: Return executable tool input
Loading

Possibly related PRs

  • lidge-jun/opencodex#3498: Adds the earlier raw patch-envelope resolver and streaming repair extended here for native code-mode exec.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: repairing native Responses exec patch completion before Codex executes it.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/grok-responses-patch-5598

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

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

@github-actions github-actions Bot added the bug Something isn't working label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 64 / 80

이 PR은 스택 세 번째 층입니다. base는 #3699의 codex/grok-owned-catalog-refresh-5598입니다. 카탈로그 두 층(3698·3699)과 달리 Responses 쪽 런타임 수리이고, 의존은 "스택 전달 순서"에 가깝습니다. 그래도 요청된 트레인에서는 3698→3699→3701→3703입니다. 부모 없이 올리지 마세요.

문제: 라우트된 모델이 완전한 apply_patch 봉투를 native code-mode exec 입력 전체에 넣으면, 예전 수리기는 alias가 아닌 same-name native exec를 건너뛰었습니다. 그래서 response.completed만 고치고 input.done/item.done에는 날것 패치가 남아, Codex가 도구를 실행하기 전에 깨질 수 있었습니다. function apply_patch alias는 완성본은 컴파일되지만, 스트리밍 중 raw patch preview가 나갈 수 있었습니다.

고친 핵심은 src/server/responses-custom-tool-repair.ts입니다. customAliasItemNamescustomExecItemNames로 넓혀, bare exec + declaresCodeModeExec도 같은 completion 수리에 넣습니다. 패치가 될 수 있는 입력·{로 시작하는 객체 접두(공백·이스케이프 키·pretty JSON 포함)는 preview를 붙잡고, authoritative done에서 compileCodeModeHelperInput으로 맞춥니다. 평범한 JS는 바이트 그대로 흘리고, 붙잡았던 접두가 JS로 판명되면 미방출 suffix를 한 번에 냅니다. custom-tool-compat.ts는 aliased/helper가 아닌 native custom JSON을 래퍼로 오해해 풀지 않습니다(render_diagram {"input":"literal"} 부정 케이스).

테스트가 lifecycle 경계를 촘촘히 잡습니다. raw/wrapped/pretty/escaped-key native exec, function apply_patch alias, input.done 없는 완성, terminal-only, 임의 JS·미완성 봉투·flat/foreign namespace 보존, failed/incomplete/dispose 시 버퍼 해제와 성공 합성 금지. docs와 structure/11_compatibility-contracts.md에 completion-boundary 패리티를 적었습니다. #3703이 이어 받을 ordinary function·dotted namespace 층과 역할이 갈립니다. types/config 분할과 무관합니다.

방금 dev에 들어온 OAuth 층(#3691/#3688)과 레인이 다릅니다. A-lane SSE→WS→recovery는 이미 ancestry에 있고, 이 PR은 그 위 Responses 도구 완성 계약입니다. 자식 #3703(약 +939)은 이 층 위에만 올립니다. 머지 후 자식 retarget을 auto-deletion보다 먼저 하세요.

경로/심볼 - src/server/responses-custom-tool-repair.ts customExecItemNames / codeModeExec — same-name native exec를 alias와 같은 완성 수리에 편입.
경로/심볼 - delta hold 조건 — mayBecomePatchEnvelope 또는 trimStart { / 빈 접두; pretty·escaped-key 회귀의 이유.
경로/심볼 - src/responses/custom-tool-compat.ts native non-helper early return — JSON-looking native body를 wrapper로 풀지 않음.
경로/심볼 - tests/responses/responses-custom-tool-repair.test.ts — 독립 oracle로 기대 JS를 고정하고 terminal item을 한 번만 실행.
경로/심볼 - structure/11_compatibility-contracts.md "Routed code-mode patch completion".
라인 - brace-leading ordinary JS({ const value = ... })는 완성까지 hold될 수 있음. 문서화된 보수적 선택이나, 긴 블록 스트리밍 UX에 영향 있는지 한 번 보면 좋음.

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

  • object-leading 입력을 전부 hold하는 보수성이 UX상 acceptable인지. 패치 오탐보다 preview 지연을 택한 감사 수정.
  • 스택상 runtime-independent라도 delivery는 3699 다음인지. 권장은 요청 스택 순서 유지.
  • #3703과 경계: 이 PR은 exec/patch completion만; ordinary integer/string·dotted alias는 위 층.

너의 추천
#3698·#3699가 dev ancestry에 들어온 뒤 이 PR을 retarget·머지하세요. responses-custom-tool-repair 회귀와 exact-head CI 통과 후 admin merge. 그다음 #3703 retarget. 부모보다 먼저 합치지 마세요. types/config 분할 무효화 대상 아님.

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

@lidge-jun
lidge-jun marked this pull request as ready for review September 5, 2026 20:30
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 5, 2026 20:30
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026

Copy link
Copy Markdown

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-05T20:34:03.517347Z 70b225b Draft marked ready
ℹ️ 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.

Base automatically changed from codex/grok-owned-catalog-refresh-5598 to dev September 5, 2026 23:31
@lidge-jun
lidge-jun merged commit d5d9845 into dev Sep 5, 2026
37 of 38 checks passed
@lidge-jun
lidge-jun deleted the codex/grok-responses-patch-5598 branch September 5, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant