Repository navigation
Fix Codex request_timeout salvage diagnostics - #6302
Conversation
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR adds refusal-path Codex timeout diagnostics and a 24-case replay matrix while leaving salvage eligibility and behavior unchanged. Review of head c3c19c8f2be52d3df4786b9c1d946cdcfac2c080 found no actionable defects.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
- The added replay matrix verifies salvage outcomes, but does not directly assert the new debug refusal record. A focused debug-enabled assertion could protect its fields.
- The logged item type originates in the provider response. The diagnostic is opt-in and no material trust-boundary impact was established; allowlisting the value would be optional log-hygiene hardening.
CI / Verification
GitHub Dev CI run 37165385130 is completed successfully for the reviewed head. The affected-path test job for packages/ai/test/openai-codex-stream.test.ts passed. GitHub reports 18 successful and 25 skipped check-runs, with no failed conclusions, on the exact head. Tests were not run as part of this read-only review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Diagnostic-only change matches the issue's conditional request; the existing salvage contract and changelog conventions are preserved. |
| A2 — Architecture / Correctness / Failure | APPROVED |
The added refusal-path log reads existing state and does not alter salvage predicates, result state, retries, or error recovery. |
| A3 — Security / Privacy / Trust | APPROVED |
No actionable boundary crossing found; the new log is debug-gated and does not expose tool arguments or identifiers. |
| A4 — Verification / Tests / CI | APPROVED |
Replay permutations assert observable tool-use results; the affected test shard passed on the reviewed head. The diagnostic itself lacks a direct assertion (optional coverage). |
| A5 — Context / Compatibility / Platform | APPROVED |
No public interface, packaging, platform, or consumer contract changes; existing debug logger abstractions are reused. |
Limitations
This was a read-only review; tests were not independently rerun, so the PR's claim that all 24 variants pass on the current dev baseline was not independently reproduced.
|
Merged into dev as
— |
Fixes #6301
What
Add the complete Codex timeout replay matrix from the issue and log the guard inputs when complete-tool salvage is refused.
Why
All 24 requested replay variants pass on the current
devbaseline, so there is no RED on dev and changing salvage behavior would be unsafe. The refusal-path diagnostics make the production guard decision observable without changing timeout, retry, model, preset, or salvage behavior.Acceptance
todo_writereplay variants cover arguments.done/output_item.done, trailing item kind, and delta correlationpackages/ai/test/openai-codex-stream.test.ts/createCompleteToolTimeoutReplayEventsand replay matrixbun test packages/ai/test/openai-codex-stream.test.tspackages/ai/src/providers/openai-codex-responses.ts/trySalvageCodexFinalizedToolCallspackages/ai/changelog.d/codex-request-timeout-tool-salvage-r2.mdbun scripts/changelog-fragments.ts checkTesting
bun test packages/ai/test/openai-codex-stream.test.ts-> 182 pass, 0 fail, 740 expect() calls.bun --cwd=packages/ai run check:types-> exit 0.bun scripts/changelog-fragments.ts check-> exit 0; 75 fragments across 5 packages validated.bunx biome check packages/ai/src/providers/openai-codex-responses.ts packages/ai/test/openai-codex-stream.test.ts-> exit 0.bun run ci:test:smoke-> exit 0;smoke-test: ok.Risk classification
low-riskregression-riskhigh-riskAssumptions
devbehavior is the intended baseline because every requested replay variant already returnstoolUse.Approval
The PR targets
dev; no merge or force-push is performed by this change. The branch was refreshed by merging currentdev(no force-push) so exact-head CI contains the event base.Agent
prompt_failed(-32603, "Agent run failed after execution started") right after a failedtodo_write, with no commits; this PR was produced by the fallback agent (gpt-6.1-sol).