fix(codex): wire the timeout wrapper into the /review and /ship diff passes - #2379
Open
aegixx wants to merge 1 commit into
Open
fix(codex): wire the timeout wrapper into the /review and /ship diff passes#2379aegixx wants to merge 1 commit into
aegixx wants to merge 1 commit into
Conversation
…passes garrytan#1056 added _gstack_codex_timeout_wrapper to bin/gstack-codex-probe and wired it into codex/SKILL.md. The /review and /ship Codex passes never adopted it and still run on a bare 5-minute Bash gate, so the stalls and truncation reported in garrytan#1036 remain reachable there. Measured on codex-cli 0.145.0 / macOS: one truncated /review adversarial pass logged task_started with no completion event, 12 shell commands and 6 web searches, and its last event at 287s of a 300s budget, cut mid-tool-call. The same prompt under a longer cap completed at 336s with turn.completed present. It was 36 seconds short. The cap is the binding constraint, not a pipe deadlock, which also explains the roughly 50% failure rate garrytan#1036 reports: cost straddles the gate, so survival tracks diff size. Changes, for the adversarial and structured-review passes in both skills: - Re-source gstack-codex-probe inside each Codex block. Shell functions do not survive between Bash blocks, so the wrapper is not otherwise in scope at the call site even though /review already sources this same file for its auth probe. - Wrap both calls with _gstack_codex_timeout_wrapper 540. - Raise the Bash gate to 600000. It must sit ABOVE the wrapper so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. - Drop the "timeout doesn't exist on macOS" instruction. The wrapper resolves gtimeout, then timeout, then runs unwrapped, so that case is already handled; the claim is what steered these sites away from the wrapper. - Treat a timeout as missing coverage rather than an informational note. The old text printed "Codex timed out after 5 minutes" and continued, which reads downstream as Codex having reviewed and found nothing. Adds 9 regression assertions to test/codex-hardening.test.ts covering wrapper presence, absence of the macOS claim, and the wrapper-under-gate ordering. They fail on the unpatched tree (29 pass / 9 fail) and pass with the fix (38/38). Deliberately out of scope: three more bare `timeout: 300000` sites in scripts/resolvers/review.ts (second opinion, plan review, and one more). This change does not raise their gates, so their error text is left alone rather than made inconsistent with a cap that did not move. Note for a separate look: codex/SKILL.md has the inverse ordering, a 330s wrapper under a 300s Bash gate, which makes its own exit-124 branch unreachable. Refs garrytan#1036
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #1036.
#1056 added
_gstack_codex_timeout_wrappertobin/gstack-codex-probeand wired it intocodex/SKILL.md. The/reviewand/shipCodex passes never adopted it. Its entire change toreview/SKILL.mdwas appending< /dev/null, the stdin fix from #971/#1045, so those passes still run on a bare 5-minute Bash gate and the stalls #1036 describes remain reachable there.Before this PR, on v1.60.1.0:
300000codex/SKILL.md(+.tmpl)review/SKILL.mdscripts/resolvers/review.tsship/sections/adversarial.mdThe cap is the cause, not the pipe
#1036 attributes the stalls to pipe deadlock and swallowed stderr. Measuring one truncated
/reviewadversarial pass (codex-cli 0.145.0, macOS) points somewhere simpler. Its rollout log showstask_started, no completion event, 12 shell commands and 6 web searches, last event at 287s of a 300s budget, cut mid-tool-call. The same prompt under a longer cap: rc=0 at 336s,turn.completedpresent. It was 36 seconds short, with no stall and no deadlock.That also explains the roughly 50% failure rate #1036 reports: cost straddles the gate, so survival tracks diff size.
--enable web_search_cachedis on at both sites and accounted for about 32s of it.Changes
For the adversarial and structured-review passes in both skills:
gstack-codex-probeinside each Codex block. Shell functions do not survive between Bash blocks, so the wrapper is not in scope at the call site even though/reviewalready sources this same file for its auth probe._gstack_codex_timeout_wrapper 540.600000, deliberately above the wrapper so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill returning nothing.timeoutdoesn't exist on macOS" instruction. The wrapper resolvesgtimeout, thentimeout, then runs unwrapped, so that case is already handled; the claim is what steered these sites away from the wrapper.Tests
9 new assertions in
test/codex-hardening.test.tscovering wrapper presence, absence of the macOS claim, and the wrapper-under-gate ordering. They fail on the unpatched tree (29 pass / 9 fail) and pass with the fix (38/38). Fullbun testgreen;bun run gen:skill-docs --dry-runclean, andreview/SKILL.mdis regenerated rather than hand-edited.Deliberately out of scope
Three more bare
timeout: 300000sites remain inscripts/resolvers/review.ts(second opinion, plan review, and one more). This PR does not raise their gates, so their error text is left alone rather than made inconsistent with a cap that did not move. Happy to fold them in if you want.Separately,
codex/SKILL.mdhas the inverse ordering: a 330s wrapper under a 300s Bash gate, which makes its own exit-124 branch unreachable. Left alone here since it is the one place the wrapper is already wired, but it probably wants its own fix.Also worth documenting
A truncated pass is not lost. Every run writes
~/.codex/sessions/<YYYY>/<MM>/<DD>/rollout-*.jsonlcontaining eachagent_messageit produced, andtask_startedwith no completion event is a reliable truncation signature. Three substantive findings were sitting in that file after the measured run had been written off as "Codex unavailable".Env: macOS, gstack 1.60.1.0, codex-cli 0.145.0,
gtimeout/timeoutboth GNU coreutils 9.11.