Timeout wedged Codex exec calls#115
Conversation
|
Codex review: needs maintainer review before merge. Reviewed May 28, 2026, 9:42 AM ET / 13:42 UTC. Summary Reproducibility: yes. from source: current main invokes Codex exec without timeoutMs, while runCommandArgs only enforces termination when timeoutMs is set. The PR comment also supplies terminal output from a shimmed wedged Codex run timing out after 50ms. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused timeout once maintainers accept the 300s default as the intended Codex provider contract, keeping the documented provider-level and Codex-specific overrides. Do we have a high-confidence way to reproduce the issue? Yes from source: current main invokes Codex exec without timeoutMs, while runCommandArgs only enforces termination when timeoutMs is set. The PR comment also supplies terminal output from a shimmed wedged Codex run timing out after 50ms. Is this the best way to solve the issue? Mostly yes: reusing runCommandArgs timeout handling is the narrow implementation and the PR adds docs plus regression coverage. The remaining decision is whether the hard 300s Codex default is the right compatibility contract. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against ed3d5750ff89. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
Added docs in d4ff0db to make the timeout contract explicit: Codex now mirrors Cursor's 300s provider timeout default, with Behavior proof from a shimmed wedged The regression also asserts the feature is marked Checks run: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
[problem]
[solution]
Wire Codex through existing provider timeout handling. Default mirrors Cursor: 300s; override with
CLAWPATCH_CODEX_TIMEOUT_MSorCLAWPATCH_PROVIDER_TIMEOUT_MS.[proof]
errorstate and released locks.