Repository navigation
fix(ai): retry incomplete Codex tool calls once - #6348
Conversation
e2e (tester)RED→GREEN differential on head
Evidence: https://github.com/probepark/qa-evidence/blob/incoming/Yeachan-Heo/gajae-code/6348/README.md Verdict: PASS |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR adds a bounded retry for Codex request timeouts during incomplete function-call argument streaming. The final message is reset correctly, but the retry can leave abandoned tool-call progress events visible to public stream consumers. I found no merge-blocking issue; the event-stream concern below is non-blocking because final-message tool dispatch remains guarded and the built-in HTTP gateway disables provider retries.
Findings / Required Changes
- [P2] Do not expose abandoned tool-call events on retry —
packages/ai/src/providers/openai-codex-responses.ts:2586-2587(non-blocking).- On the ordinary public stream or agent-subscriber path,
toolcall_start/toolcall_deltaevents from the first attempt can escape beforerequest_timeout. The new guard exception permits retry despite accumulated output, then resets the in-memory message but cannot retract the already-emitted events. A consumer can therefore observe an orphaned partial call followed by retry events, with no reset marker; the base implementation rejected retry after output had accumulated. - Prefer declining the retry after attempt events have escaped, buffering retry-eligible events until an attempt is accepted, or defining an explicit reset event. This is non-blocking: the built-in agent dispatches from the final reset message, so duplicate tool execution was not established; the built-in HTTP gateway also disables provider retries.
- On the ordinary public stream or agent-subscriber path,
CI / Verification
- Exact-head Dev CI run
37255934007succeeded; the focusedopenai-codex-truncated-toolcall.test.tsjob passed. - Exact-head Public site sync run
37255934074succeeded. - Follow-up Dev CI run
37256173636was skipped for a metadata-only PR edit; it is not product-test evidence. No exact-head failed check was observed. - The review did not execute tests or PR code.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The stated incomplete-argument timeout retry matches the implementation; no independent policy or contract mismatch was found. |
| A2 — Architecture / Correctness / Failure | APPROVED |
The retry is bounded and guarded; finding 1 is a concrete but non-blocking public event-stream consistency issue. |
| A3 — Security / Privacy / Trust | APPROVED |
No new authorization, credential, privacy, or trust-boundary issue was identified. |
| A4 — Verification / Tests / CI | APPROVED |
Focused retry tests and the target exact-head CI job passed; metadata-edit workflow skips were not treated as product verification. |
| A5 — Context / Compatibility / Platform | APPROVED |
Public stream and agent consumers were traced; no other compatibility issue or materially applicable duplicated abstraction was found. |
Limitations
Static review only; no tests or live Codex request were run. Requiredness of skipped CI contexts could not be confirmed because the branch-protection query returned HTTP 401. No intent_projection was available in the accessible PR/session metadata.
|
@probepark this one is approved on its exact head
These are two overlapping recovery paths, not a textual conflict, so I haven't resolved it myself; how the two should compose is your call. Please merge dev in and decide whether both paths stay. I'll merge it once there's an approval on the new head and CI is green. — |
f9ed7b7 to
d54f86e
Compare
Review fixes (coder)No new review findings — this pass rebases onto
Local verification (studio):
|
|
@snowykr your review findings are addressed (see the reply above). Current head: |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The PR adds a timeout-specific retry for Codex streams whose only output is an incomplete tool call. The new path can bypass the existing one-shot partial-output replay guard after partial call events have already been released, allowing an abandoned call and its replacement to appear in the same public stream.
Findings / Required Changes
- [P2] Preserve the one-shot partial replay guard —
packages/ai/src/providers/openai-codex-responses.ts:2682- Compared with the base, the new
allowIncompleteToolCallOutputalternative incanRetryWithOutputbypasses the existing!partialToolCallReplayAttemptedguard. A reachable sequence is: an SSE attempt emits an incomplete tool call and hits a socket reset; the existing generic partial-output retry setspartialToolCallReplayAttempted; that retry emits another incomplete call and then ends withrequest_timeout. The special retry flag is still unset, so the new timeout path can reset and replay output despite the earlier partial replay. - After the first partial replay, event buffering is disabled, so the abandoned call's start/deltas are already public. The OpenAI Chat SSE consumer assigns a new call index for each
toolcall_start; downstream clients cannot retract the partial call and may receive it alongside the replacement. The base guard prevented this second partial replay. The configured retry budget and the new flag bound the number of attempts, but do not preserve the stream contract. - Require the timeout-specific path to honor the existing partial-replay state once events have been released, or keep those events buffered until another replay is no longer possible. Preserve the intended retry for a first incomplete-call timeout.
- Compared with the base, the new
Non-blocking Observations
packages/ai/test/openai-codex-truncated-toolcall.test.ts:231checks the retry count andtoolUseresult, but an additional assertion for the recovered call's identity, complete arguments, and completeness flag would make the regression test stronger. This is a coverage improvement, not a demonstrated defect or merge blocker.
CI / Verification
Exact-head checks for d54f86e856fdfc24312bb3594d371152858bd82e show 18 successes, 8 skipped checks, and no failures or cancellations. The changed AI test/check job and aggregate checks succeeded; skipped checks were conditional, platform-specific, or opt-in. The PR's reported 211/0 local test run was on an earlier head, so its count is not treated as exact-head evidence.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1: new timeout path bypasses the existing replay limit after events are exposed. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 1: socket-reset replay followed by timeout can replay released partial output. |
| A3 — Security / Privacy / Trust | APPROVED |
No new trust-boundary crossing, credential exposure, or unauthorized tool dispatch identified. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head test/check and aggregate CI succeeded; assertion improvement is non-blocking. |
| A5 — Context / Compatibility / Platform | CHANGES_REQUESTED |
Finding 1: emitted OpenAI-compatible stream can contain abandoned and replacement partial calls. |
Limitations
The review was based on pinned source and exact-head CI evidence; no tests or live provider scenarios were executed. The public CI summary did not expose an exact-head test pass count.
This reverts commit 713c4b9.
Review fixes (coder)Addressing review 5411512455 (snowykr, CHANGES_REQUESTED @ d54f86e).
Regression tests (one per sibling path of the changed state), in
Local verification on head
Note: an intermediate commit 713c4b9 added New head: |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR adjusts Codex stream retry eligibility for incomplete tool-call output and uses the existing retry/reset path. The reviewed change preserves the surrounding retry controls and downstream safeguards; no actionable merge-blocking defects were found.
Findings / Required Changes
No blocking or actionable findings.
CI / Verification
The exact-head Dev CI run (37293829528, head 433b01197f45d99039c5ced08b267a61def20b11) reports success. The changed test-file jobs, @gajae-code/ai check, native build, planner, and final affected-path validation succeeded. Conditional/host-scoped skipped jobs are not product failures. The added mocked-SSE tests exercise retry success and relevant rejection cases. No local tests were run as part of this read-only review; detailed GitHub job logs were not available for independent inspection.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The implementation and repository contract align; no intent projection was available for review. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Partial replay, timeout guards, retry budget, reset/reopen ordering, and downstream error handling show no verified regression. |
| A3 — Security / Privacy / Trust | APPROVED |
Partial arguments are not dispatched; existing tool resolution and argument validation remain in the consumer path. |
| A4 — Verification / Tests / CI | APPROVED |
Focused mocked-stream tests and exact-head CI are green; no live-provider e2e was established. |
| A5 — Context / Compatibility / Platform | APPROVED |
The change uses the existing retry abstraction and leaves public APIs and neighboring provider contracts unchanged. |
|
Merged into dev as
— |
Refs #6301
Why
Codex streams on home2 closed with
request_timeoutwhile a tool call's arguments were still streaming (7/7 incidents: argumentsComplete=[false], recentEvents=function_call_arguments.delta x15 -> error, ~15 min after request start; evidence: #6301 (comment)).tryRetryCodexProviderErrorrefuses to retry once any output exists, so the turn ended as prompt_failed even though nothing user-visible had been produced.What
packages/ai/src/providers/openai-codex-responses.tsincompleteToolCallRetryAttempted(one-shot).isCodexIncompleteToolCallTimeoutRetryable: true only whenisCodexTransientStreamClose(error), provider code isrequest_timeout(not an idle stall), the output holds only thinking blocks, empty text blocks, or tool calls whose arguments are not complete, at least one such tool call exists,runtime.finalizedToolCallIds.size === 0, there was no tool-argument correlation failure, and the flag is still unset.recoverCodexStreamErrorcalls it right beforetryRetryCodexProviderError. The flag is set before reconnecting, so a second timeout cannot retry again even if the reconnect fails. The existing reset/reconnect body is reused through a newallowIncompleteToolCallOutputparameter.trySalvageCodexFinalizedToolCallssalvage conditions, theCODEX_MAX_RETRIESdefault andproviderRetryAttemptbudget, idle-timeout constants, the websocket idle-timeout early return, and existing test expectations.packages/ai/test/openai-codex-truncated-toolcall.test.ts: 3 new tests (1 regression, 2 negative).packages/ai/changelog.d/codex-timeout-incomplete-toolcall-retry.md: changelog fragment.Risk
regression-risklow-riskhigh-riskRetry behaviour changes on one narrow provider error path (
request_timeoutwith only incomplete tool-call/thinking output). Every other path is gated by the unchanged conditions.Local tests (command + result)
Run on head f9ed7b70 (work mac):
bun test ./packages/ai/test/openai-codex-truncated-toolcall.test.ts ./packages/ai/test/openai-codex-stream.test.ts ./packages/ai/test/openai-codex-toolcall-increment-guard.test.ts→ 211 pass, 0 fail (rc=0)openai-codex-responses.tsreverted to HEAD~1): truncated-toolcall suite gives 4 pass / 2 fail. The failing tests are "retries a timeout after only an incomplete function call" and "does not retry a second consecutive timeout", which fail because only one request is made. The text-negative test passes both before and after, as expected.bun --cwd=packages/ai run check(Biome + tsc) → passbun scripts/changelog-fragments.ts check→ pass (22 fragments)Local CI gate (prepush)
PREPUSH_CMD rc=0 bun run --workspaces --if-present check:types
PREPUSH_CMD rc=0 bun scripts/telegram-daemon-generation-guard.ts "$(git merge-base origin/dev HEAD)" "$(git rev-parse HEAD)"
Acceptance
request_timeoutduring incomplete tool-call args; the second stream completes with toolUse and 2 requests in total; fails before the fixrecoverCodexStreamError+isCodexIncompleteToolCallTimeoutRetryableretries a timeout after only an incomplete function call(RED before, GREEN after)every(...)text guarddoes not retry a timeout when non-empty text was emittedincompleteToolCallRetryAttemptedset before reconnectdoes not retry a second consecutive timeout(RED before, GREEN after)Needs e2e
None beyond CI. The behaviour is fully covered by mocked SSE streams. A live
request_timeoutcannot be reproduced on demand, so it will be confirmed after deploy through the #6301 incident counter on home2.Agent
The first attempt was gjc on studio and died mid-turn (
ACP prompt was abandoned because the SDK session host closed, code -32603, prompt_abandoned) with WIP at a6ae0ccb. The fallback was omo/cliproxy/gpt-6.1-sol on work: it imported the WIP, reviewed it, fixed the flag ordering (the flag is now set before the reconnect), and produced f9ed7b70.Open questions
None.