fix(responses): bundle lane E — combo resend safety, WebSocket replacement, goal helpers, Devin retry delays - #5675
Conversation
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. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (55)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: d166470b9c
ℹ️ 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".
…grant Review finding on #5675. With retryOnReset.replacements set to 2, a dead Codex WebSocket spends the first grant on its HTTP replacement; if that replacement resets before its head, the WebSocket row settled it as the replay refusal at once, so the configured second replacement was never reachable. The reset is the pre-header row again, so the row now asks the same gate (and the send budget) once more and resends only when a grant remains; with the default of one it still settles as the refusal. The loop is bounded by the request's finite allowance.
Carries #5629. The shared retry-delay parser accepts the generated "retry after ~180s" approximation marker after Retry-After, and the bounded Devin replay re-evaluates the delay on every attempt within the existing cumulative ceilings. Folded review fixes: a repeated approximation marker ("~1 minute ~30 seconds") now rejects the hint instead of silently shortening it to the first component, and the cloud-direct comment no longer claims the marker blocks re-parsing. Supersedes #5629. Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>
Carries #5659. Routed create_goal, get_goal and update_goal calls (bare or with a provider-invented default. prefix) are accepted as nested helpers of a genuinely declared code-mode exec and compiled to the matching tools.<helper>(...) call instead of falling through to exec_command. A genuinely declared bare goal tool keeps its identity, and a catalog that declares neither the tool nor exec still fails closed. Folded review fixes: the guard is asserted on the original unrestored wire name, an unlisted helper-like name is proven not admitted, bare-goal precedence is covered through full restoration, the authorization comments in src/types/tools.ts name the goal helpers, and the codex integration guide describes the repair. Closes #5495. Supersedes #5659. Co-authored-by: ingwannu <186453546+Ingwannu@users.noreply.github.com>
…guous replacement Carries #5646. Once a request has spent its retryOnReset replacement, the first send may already have run the turn, so a replacement that answers 200 and then fails with zero output must not be sent again. RequestExecutionBudget now reports ambiguousResendSpent from the one shared grant; combo failover stops when it is spent and settles the answer with the shared settleOperatorReplacement rule (a resendable status becomes the replay refusal, anything else keeps its status with the non-replayable marker). On the direct path the streamed opaque-blob rebuild is skipped once the grant is spent. Carried before #5633 so its WebSocket replacement row is never exposed to the third-send gap. Supersedes #5646. Co-authored-by: Fred Amartey <43480311+FredAmartey@users.noreply.github.com>
…died unanswered Carries #5633. A Codex WebSocket that opens and then closes or errors under its create frame before any Responses event is the same unknown state as an HTTP connection that resets before its head. For a provider that opted into retryOnReset, the request-resend gate may now spend the request's single replacement on it (one replacement per logical request, self-contained body only). The exchange records the stage it reached; silence keeps its 504 and a drop after a relayed event keeps its errored 200. Providers that have not opted in are unchanged. Carried after #5646, so the WebSocket replacement row inherits the spent-grant stop. Folded review fixes: the duplicate settleOperatorReplacement import the pair merge produced is removed, and responses-failover.md states the 2xx replacement contract once as settled for all three replacement rows. Supersedes #5633. Co-authored-by: Fred Amartey <43480311+FredAmartey@users.noreply.github.com>
Carries #5489 (net diff; its upstream/dev merge commit is dropped). When a runTurn adapter's first meaningful event in a combo attempt is a tool call the current request did not declare, the existing fail-closed refusal is projected as a pre-commit 502 so the combo can hop to the next target with the same tool catalog. Chat Completions and Anthropic Messages inbound requests keep their existing behaviour. Folded review fix: the non-streaming path now applies the same boundary as the streaming preflight. An undeclared tool call after a replay-unsafe heartbeat (an adapter-side effect already ran) keeps the refusal on that child instead of sending the turn to another target. New streaming and non-streaming cases prove exactly one dispatch; the non-streaming one fails without the gate. The combos guide gains the hop row in every locale and responses-failover.md records the runTurn boundary. Related to #5407 (covers its Responses path only; the reported Claude Code Anthropic Messages path is unchanged). Supersedes #5489. Co-authored-by: Yu Zhang <34849476+AaronZ345@users.noreply.github.com>
…child Found by the lane's adversarial review of the carried #5646/#5489 changes. - A runTurn adapter that emits a replay-unsafe heartbeat (it already ran a local side effect, as Cursor does) and then errors or ends empty before any output returned a plain 502, so a combo sent the turn to the next target and could repeat the side effect. Streaming and non-streaming paths now mark that 502 non-replayable, and the combo stops on the child. This predates the carried commits; it sits on the same boundary structure/runtime.md states. - A scope derived from a shape-compatible budget that implements claimAmbiguousResend but not ambiguousResendSpent reported "not spent" after it claimed the grant, which would let a combo hop on a zero-output 200 from the replacement (a third send). Grants claimed through the bridge are now latched per bridged parent and visible to every sibling scope. Both are covered by new tests that fail without the fix.
…grant Review finding on #5675. With retryOnReset.replacements set to 2, a dead Codex WebSocket spends the first grant on its HTTP replacement; if that replacement resets before its head, the WebSocket row settled it as the replay refusal at once, so the configured second replacement was never reachable. The reset is the pre-header row again, so the row now asks the same gate (and the send budget) once more and resends only when a grant remains; with the default of one it still settles as the refusal. The loop is bounded by the request's finite allowance.
29fe485 to
056f9cd
Compare
리뷰 · 우선순위 56 / 80이 PR은 같은 요청을 한 번 더 보내지 않게 막는 수정과, 코드 모드 목표 도구, Devin 재시도 시간을 Devin이 코드 모드에서
Codex 웹소켓이 응답 이벤트를 받기 전에 끊기면, 콤보의
메인테이너의 판단이 필요한 지점 목표 도구 세 이름을 네임스페이스 도구의 짧은 이름에서 뺄지입니다. 코드 모드 복구에 필요한 것은 도우미 목록입니다. 짧은 이름 금지는 그 목록과 한 배열을 공유해서 따라왔습니다. 컨트롤 프레임만 온 웹소켓도 HTTP 대체에 넣을지입니다. 프레임이 하나도 없는 끊김과 같은 허용을 탑니다.
너의 추천 짧은 이름 금지 목록에는 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
Bundle lane E (responses and combo resilience) from the 260923 PR consolidation round. One branch, one commit per carried PR, every carried commit credited with
Co-authored-by.fix(devin): accept generated approximate retry delaysretry after ~180smarker after Retry-After. Review fixes: a repeated marker (~1 minute ~30 seconds) now rejects the hint instead of shortening it, and a stale comment is corrected.fix(responses): restore code-mode goal helperscreate_goal/get_goal/update_goal(bare ordefault.-prefixed) compile to the matching nested helper inside a declared code-modeexec. The helper list is closed; a declared bare goal tool keeps its identity; noexecstill fails closed. Review fixes: guard asserted on the raw wire name, unlisted-name and bare-precedence cases, authorization comments, guide sentence.fix(responses): stop combo failover once a request has spent its ambiguous replacementretryOnResetreplacement is spent, a zero-output failure of the replacement's 200 stops the combo (and the direct opaque-blob rebuild) instead of sending the turn a third time. Carried first of the pair.fix(responses): let retryOnReset replace a Codex WebSocket send that died unansweredfix(combo): fail over undeclared zero-output toolsrunTurnadapter's undeclared first tool call in a combo attempt becomes a pre-commit 502 so the combo hops with the same catalog. Review fix: the non-streaming path now respects a replay-unsafe heartbeat like the streaming preflight (test fails without it). Combos guide row added in all 8 locales.fix(combo): keep failures after a replay-unsafe side effect on their childambiguousResendSpentnow reports a grant it claimed, closing a third-send path.Closes #5495.
Supersedes #5629.
Supersedes #5659.
Supersedes #5646.
Supersedes #5633.
Supersedes #5489.
Related to #5407: this covers its Responses path only; the reported Claude Code / Anthropic Messages path is unchanged, so the issue stays open.
Not included, with reasons (details in
devlog/_plan/260923_bundle_lane_e/010_decisions.md):adapter_eof; the final 502 and the cooldown 503 follow existing rules. A wire capture is needed.Co-authored-by: Epinephrine 27862058+luvs01@users.noreply.github.com
Co-authored-by: ingwannu 186453546+Ingwannu@users.noreply.github.com
Co-authored-by: Fred Amartey 43480311+FredAmartey@users.noreply.github.com
Co-authored-by: Yu Zhang 34849476+AaronZ345@users.noreply.github.com
Verification
bun teston 33 files (retry delay, Devin stated reset, code-mode goal helpers and undeclared-tool guard, WebSocket ambiguous resend, replay refusal parity, ambiguous-resend composition, execution-budget permits, routing policy fallback, upstream retry, reset replay, opaque-blob recovery, combo failover e2e, combo zero-output failover, run-turn queue, bridge, combo stream preflight, test layout, file-size ratchet, structure SSOT) — 1003 pass, 0 fail, on the head rebased onto7dd1db22ac.bun x tsc --noEmit(exit 0),bun run structure:check,bun run privacy:scan,git diff --check.bun run testonce after every lane has landed. Exact-head hosted CI on this PR is the merge evidence.Checklist
Summary by CodeRabbit
New Features
execwhen the request declaresexec.Documentation