Repository navigation
fix(ai): replay partial Codex tool calls after stream close - #6331
Conversation
f1028c5 to
950b020
Compare
CI triage (coder)
(run https://github.com/Yeachan-Heo/gajae-code/actions/runs/37212208077, job Fix: I rebased the single PR commit onto current New head: Local checks run on the rebased head:
|
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This PR classifies Codex request_timeout stream closures as retryable and replays eligible incomplete-tool-call attempts. The final AssistantMessage is reset before retry, but already-emitted public stream events are not; a successful recovery can therefore expose both the abandoned partial call and the replayed call. One additional retry-budget edge case is non-blocking.
Findings / Required Changes
-
[P1] Do not replay after partial tool-call events have escaped —
packages/ai/src/providers/openai-codex-responses.ts:2564- With provider retries enabled, Codex can emit
toolcall_startand partial argument deltas, then close with a classifiedrequest_timeout. The new gate allows a retry, and resettingcontext.outputcleans only the terminal message; it does not retract events already pushed to stream consumers. This differs from the merge base, where non-empty output vetoed the retry and this successful-but-contaminated replay path was not reachable. - This reaches supported consumers:
openai-responses-server.ts:1112-1116closes the open item on the nexttoolcall_start, andcloseOpenemits its accumulated partial arguments as a completed item (:920-950). The Chat adapter also allocates a new wire index for each start. Thus the streamed response can contain an abandoned/incomplete call alongside the successful replay, even though.result()contains only the replayed call. The added test checks the final result, not the emitted event sequence. - Buffer events for attempts eligible for replay and publish only the accepted attempt, or define and implement an explicit reset/rollback protocol for all public stream consumers. Clearing the final message alone is insufficient.
- With provider retries enabled, Codex can emit
-
[P2, non-blocking] Preserve the partial-replay allowance across empty-output retries —
packages/ai/src/providers/openai-codex-responses.ts:2556-2557- If the first transient close occurs before any output,
.every(...)passes vacuously and the ordinary no-output retry consumespartialToolCallReplayAttempted. If that next attempt emits an incomplete tool call and closes, the flag prevents the partial replay even though the configured retry budget remains. The base also failed this request-timeout sequence, so this is a limited gap in the new recovery path rather than a regression; it is not independently blocking. - Consume the one-shot flag only when an eligible non-empty partial-output replay is actually taken.
- If the first transient close occurs before any output,
CI / Verification
GitHub Actions run 37217342268 is bound to reviewed head 950b0208ec7c58449a253db8604ffcebeaf07fe7 and completed successfully. The focused AI test task, @gajae-code/ai check, Affected path validation, and Virtual integration validation passed; 18 checks succeeded and 8 path-conditional/platform checks were skipped. The added tests cover the final-result replay and key vetoes, but not public incremental-event output. No local tests were run as part of this static review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1: replay resets the final message but violates the already-published stream contract. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 1 is merge-blocking; finding 2 is a non-blocking retry-state edge case. |
| A3 — Security / Privacy / Trust | APPROVED |
No new credential destination or unauthorized tool-dispatch path was established. |
| A4 — Verification / Tests / CI | APPROVED |
Relevant checks passed; the untested event-sequence behavior is captured in finding 1. |
| A5 — Context / Compatibility / Platform | CHANGES_REQUESTED |
Finding 1 is observable through the Responses and Chat stream adapters. |
Limitations
Review was static; no local test/build execution was performed. The requiredness of branch-protection checks could not be independently verified because GitHub returned 401 for that endpoint; a separate Vercel check suite was queued with zero runs. No intent_projection artifact/path was available after a bounded lookup.
e2e (tester)
Note: the replay request arrived ~120s after the RST (proxy log 1791144091.959 → 1791144212.521), so recovery is slow but not terminal. Evidence: https://github.com/probepark/qa-evidence/blob/incoming/Yeachan-Heo/gajae-code/6331/README.md |
Review fixes (coder)Addresses review 5407712185 (@snowykr, CHANGES_REQUESTED on 950b020). Fix commit: 63ab60c; new exact head: b84336d (63ab60c + merge of origin/dev).
Sibling failure paths, each with a regression test asserting the emitted event sequence (not only
Local verification (tank, agent run):
Merge approval red is expected for agent PRs until maintainer exact-head approval. Re-requesting review from @snowykr. |
Retry transient Codex stream closes after refusing incomplete tool-call salvage, while preserving retry vetoes and avoiding partial execution.
Treat ECONNRESET SSE body closes as one-shot partial tool-call replay candidates without widening normal retry behavior.
Keep partial tool-call replay limited to socket resets and existing transient stream closes with an unfinished tool call. Idle SSE stalls are not semantic progress and must remain terminal.
Keep replayable attempt events private until the accepted attempt commits, preserve the one-shot replay allowance across empty retries, and cover transport and cancellation paths.
b84336d to
f9de45a
Compare
CI triage (coder): not PR-causedThe
This PR's diff only touches Action: rebased onto origin/dev with no content changes and no conflicts. The new head is f9de45a804ee3ba810fc43ca0dcab9fb99e5c173 (previously b84336d). Local verification on the new head, in
The open P1/P2 review fixes from 63ab60c are unchanged by the rebase. Merge approval red is expected for agent PRs until maintainer approval. |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The PR adds one-shot replay for incomplete Codex tool calls after transient stream closures. Exact-head review found two reachable event-lifecycle regressions: a tool-choice capability notification is discarded during fallback, and a later SSE retry can expose both abandoned and accepted partial tool-call events after WebSocket-to-SSE fallback. Both should be corrected before merge.
Findings / Required Changes
-
[P2] Preserve the tool-choice incapability event across retry reset —
packages/ai/src/providers/openai-codex-responses.ts:2363-2382- During streaming forced-tool-choice recovery with empty output and retry budget available, the code emits
toolChoiceIncapabilitythroughemitCodexEvent, then discards the buffer while resetting for the fallback retry. The retry can succeed without the requested choice, but the event never reaches the agent loop'sonToolChoiceIncapabilityconsumer. At base this notification was delivered directly, so the new buffer introduces a concrete callback-contract regression. - Keep this control-plane event outside the retractable response buffer or preserve/deliver it before discarding replayable content; cover the retry-enabled in-stream fallback.
- During streaming forced-tool-choice recovery with empty output and retry budget available, the code emits
-
[P2] Do not retry after fallback output has been released —
packages/ai/src/providers/openai-codex-responses.ts:2607-2609, 2616-2634- In a session-backed WebSocket request, a transient WebSocket close can fall back to SSE after visible output. That fallback releases/reset events; a later SSE attempt's unfinalized tool-call events are then pushed directly. The new partial-replay branch does not check
eventsReleased, so another transient SSE close can reset and retry after consumers already received those abandoned starts/deltas. The consumer stream has no attempt-retraction/deduplication, so the abandoned events can appear alongside the accepted retry's events. This does not establish duplicate tool execution. At base, the provider retry was rejected when the current output contained a partial call; the PR makes this additional retry reachable. - Keep the fallback SSE attempt buffered while it remains replayable, or prohibit partial replay once its events have been released.
- In a session-backed WebSocket request, a transient WebSocket close can fall back to SSE after visible output. That fallback releases/reset events; a later SSE attempt's unfinalized tool-call events are then pushed directly. The new partial-replay branch does not check
Non-blocking Observations
- The exhausted-budget/managed-fallback and nonretryable-error tests would be stronger with request-count assertions; current assertions can miss an unintended extra fetch. This is a coverage improvement, not a demonstrated runtime defect.
- For robustness, the event buffer retains an additional event history for eligible output and can grow with continuous deltas from a faulty or administrator-configured custom upstream. No independent external attacker path was established, so this is not treated as a security blocker.
CI / Verification
For exact head f9de45a804ee3ba810fc43ca0dcab9fb99e5c173, GitHub reports 26 check runs: 18 succeeded, 8 skipped, and none failed. The affected packages/ai/test/openai-codex-stream.test.ts test job and check:@gajae-code/ai passed. No local tests were run during this review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1: a public capability-notification callback is lost during the new buffered retry reset. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 2: replay after events are released can expose abandoned and accepted attempt events to the same consumer. |
| A3 — Security / Privacy / Trust | APPROVED |
No new credential, authorization, or external trust-boundary crossing was established; the conditional custom-upstream memory concern is non-blocking. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head targeted test and package check passed; remaining request-count assertions are optional coverage improvements. |
| A5 — Context / Compatibility / Platform | CHANGES_REQUESTED |
Findings 1 and 2 affect actual agent-loop event consumption and streamed updates; no separate persistence or packaging incompatibility was found. |
Limitations
No intent_projection artifact was available. The review did not execute tests or PR code; verification evidence is from GitHub checks bound to the exact reviewed head. The memory-growth observation is conditional on a faulty or configured custom upstream; no external attacker path was established.
Deliver tool-choice incapability outside retractable response buffering and re-arm replay buffering for SSE fallback attempts.
Assert control-plane delivery, clean websocket-to-SSE replay, and request counts for terminal fallback paths.
Review fixes (coder)Addresses review 5408685525 (CHANGES_REQUESTED @ f9de45a). New head:
Non-blocking observation (request-count assertions): the exhausted-budget/managed-fallback and nonretryable-error tests now assert Sibling failure paths of the changed state (buffer / Local verification on exact head
|
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This PR adds buffered, one-shot replay of partial Codex tool-call output after selected stream closes. The exact-head CI checks passed, but the socket-reset message path can override an explicit non-retryable provider-error classification and resend a deterministic failed request.
Findings / Required Changes
- [P2] Preserve non-retryable provider-error vetoes during socket-reset replay —
packages/ai/src/providers/openai-codex-responses.ts:2635- The new socket-reset alternative admits replay based on the error message even when the parsed provider error carries a code classified as non-retryable. For example, after an unfinished tool call, an SSE error with
code: "invalid_request_error"and messageThe socket connection was closed unexpectedlyis rejected by the transient-close classifier but accepted by the new socket-reset branch. That branch did not exist at the merge-base; the provider-code veto did. - With retries enabled, this reaches a second request with the same invalid request/context, adding avoidable latency and provider cost instead of preserving the terminal error. The one-shot/retry-budget guards limit the duplicate to one attempt, and the agent loop does not execute partial calls on an error result; neither guard preserves the explicit non-retryable contract.
- Keep the typed non-retryable provider-error veto authoritative before applying the message heuristic, or restrict socket-reset replay to actual transport errors. Add regression coverage for a non-retryable provider code paired with socket-close wording. This should be corrected before merge because the new branch overrides an existing terminal-error safeguard.
- The new socket-reset alternative admits replay based on the error message even when the parsed provider error carries a code classified as non-retryable. For example, after an unfinished tool call, an SSE error with
Non-blocking Observations
The abort test queues SSE input and aborts before waiting for stream consumption, so it does not specifically exercise cancellation after eligible partial events have been parsed and buffered. The reviewers found no contract violation in that ordering (partial content on abort is supported), but synchronizing that test could better lock down event ordering.
CI / Verification
For exact head fa8ed09a7adbe27addcd1038d39766f09e61cc05, the targeted Codex stream test, AI package check, CLI smoke, affected-path aggregate, GJC state gates, and virtual integration validation passed. Conditional platform/opt-in checks were skipped, not reported as product failures. No local tests were run as part of this read-only review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1: socket-reset replay bypasses the existing non-retryable provider-code policy. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 1: a terminal provider error can trigger a duplicate request; replay remains one-shot and partial calls are not executed on the error path. |
| A3 — Security / Privacy / Trust | APPROVED |
No new authority or trust-boundary crossing was established; the same-provider retry does not dispatch partial tool arguments. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head focused test and package/aggregate checks passed; mixed provider-code/socket-message coverage is requested with Finding 1. |
| A5 — Context / Compatibility / Platform | APPROVED |
Provider-specific buffering matches its consumer boundary; no materially preferable drop-in abstraction or demonstrated platform compatibility defect was found. |
Limitations
No intent_projection artifact was available in the reviewed workspace. The mixed-code/message failure is established by static tracing of the exact-head classifier and provider error parsing; this review did not execute the PR code.
Non-retryable provider errors must not trigger partial tool-call replay even when their message looks like a socket reset.
Classify typed non-retryable provider failures before socket-close heuristics so invalid requests are not resent.
Review fixes (coder)Addresses review @snowykr (CHANGES_REQUESTED @ fa8ed09). New exact head:
Non-blocking observation (abort test ordering): ✅ addressed in 57a9d12. The abort test now waits until the partial events have been read ( Local tests (packages/ai, on the new head)
Exact-head CI on 57a9d12 is still running ( Note: an agent PR's |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR adds bounded replay of incomplete Codex tool-call attempts for classified stream-close failures, while buffering provisional events and preserving retry vetoes. No merge-blocking defect was identified. One limited TTFT telemetry issue is reported as non-blocking.
Findings / Required Changes
- [P3] Refresh the first-token clock after partial replay (non-blocking) —
packages/ai/src/providers/openai-codex-responses.ts:2682- The stream loop snapshots
context.firstTokenTimeinto a local value before processing an attempt. On the new partial-tool replay path, recovery clears the context field but leaves that local timestamp intact. If an attempt emits partial tool-call output, closes, and the replay succeeds, the final response can therefore report TTFT from the abandoned attempt;finalizeCodexResponsepublishes this value asoutput.ttft, which is consumed by GenAI response-time telemetry. Base behavior did not permit this partial-output retry scenario, although the same stale-local pattern existed on an older WebSocket replay path. This affects telemetry accuracy, not response content or retry safety. Refresh the loop-local timestamp after successful recovery or use resettable shared state. Non-blocking.
- The stream loop snapshots
CI / Verification
For reviewed head 57a9d122ef31e5ebfd6abe4ada4740cf2ee0e006, the targeted packages/ai/test/openai-codex-stream.test.ts job, check:@gajae-code/ai, affected-path validation, and virtual integration validation succeeded. The recorded check-run set contained 18 successes and 8 skips, with no failed or pending checks. Skipped platform/opt-in checks were not treated as product failures. No local tests were run during review. The PR description's attribution of a full-suite failure to the environment or unrelated causes was not established by these affected-path checks.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The retry changes follow the provider-stream retry contract and applicable retry policy; no intent-projection artifact was found. |
| A2 — Architecture / Correctness / Failure | APPROVED |
One-shot replay remains bounded by incomplete-call, visibility/finalization, abort, budget, and retry-veto guards; finding 1 is a non-blocking telemetry issue. |
| A3 — Security / Privacy / Trust | APPROVED |
Replay stays with the configured provider and credentials; abandoned incomplete calls do not bypass downstream tool dispatch, argument validation, or authorization. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head focused regression, package, and aggregate CI checks passed; no current-head failed check was observed. |
| A5 — Context / Compatibility / Platform | APPROVED |
No public provider contract changed; downstream transport failure shape is unchanged, and the socket-close predicate reuses the shared utility. |
Limitations
The exact-head affected-path CI did not establish the cause of the full-suite failure mentioned in the PR description; no failed exact-head check was observed.
|
Merged into dev as
— |
Why
Tank evidence from binary
dc0c69486shows Codex relayrequest_timeoutstream closes while function-call arguments are still streaming. Salvage correctly refuses incomplete arguments, but retry classification rejected the closed-stream error and any partial output blocked the existing retry path, producing terminalsdk_prompt_terminal_failed.What
request_timeoutclosed-stream messages as retryable.ECONNRESET/ unexpected socket closes as one-shot partial tool-call replay candidates.Local tests
Re-run on tank at exact head
70d4566d6(clean worktree,bun install --frozen-lockfile):bun test packages/ai/test/openai-codex-stream.test.ts— 193 pass, 0 fail (r3 commitdce55f5efixed the earlier malformed-delta stall failure by not replaying idle stalls).bun test packages/ai/test/openai-codex-stream.test.ts -t ECONNRESET— 4 pass, 0 fail (r2).biome checkon the 2 changed files — clean.bun --cwd=packages/ai run checkreports 2 format/organizeImports errors only inopenai-opencodex-responses{,.test}.ts, which are byte-identical toorigin/dev(pre-existing on dev, not touched by this PR).bun run --workspaces --if-present check:typesrc=0, telegram-daemon-generation-guard rc=0.Acceptance table
CODEX_MAX_RETRIES, retry delays, timeouts, and idle/stall windows unchanged.Agent
Implemented by GJC agent on
g6-partial-toolcall-retry-r2(home2→studio, ECONNRESET replay) and-r3(tank, idle-stall replay veto + dev merge). Wrapper post-push preflight failed only on gh auth inside the run (gh pr list failed), not on a verdict.Risk classification
low-riskregression-riskhigh-riskApproval
Merges to
devrequire one approving GitHub review from a write-access maintainer on the exact current head. An agent PR's redMerge approvalcheck means it is waiting for maintainer exact-head approval.devpackages/<pkg>/changelog.d/(not applicable: internal retry fix)Assumptions
providerRetryAttemptis the existing shared retry counter; the partial-tool replay is one-shot and increments it exactly once.우회: 없음