fix(codex): port five upstream fixes - #32
Merged
Merged
Conversation
The validator, usage text, and docs stopped at xhigh, so the max and ultra tiers supported by the models this plugin drives were unreachable through --effort. Extend the accepted set and the strings that describe it. Ports openai#761.
describeStartedItem assumed item.changes was always an array on a fileChange item/started notification. Codex can emit the start event before the change list is known, so item.changes is undefined and the length read throws a TypeError that aborts the whole turn. Guard the read the same way the other consumer of this field already does, and treat a missing or non-array list as zero changes. Ports openai#775.
waitForSingleJobSnapshot already computed waitTimedOut and timeoutMs on the snapshot, but the status handler discarded both before rendering: text output showed a normal running job and the process exited 0. Callers that poll with `/codex:status <id> --wait` therefore treated a timed-out wait as a finished check. Set a non-zero exit code and print the timeout in both the text and JSON forms, while keeping waitTimedOut in the JSON payload for existing consumers. Ports openai#774.
A connect attempt that never completes (a named pipe or Unix socket stuck accepting) left waitForBrokerEndpoint and the broker app-server client hanging past their readiness budget instead of failing over to a direct app-server. Bound each connect attempt to the remaining readiness budget, and let the broker client's own connect timeout out after connectTimeoutMs, treating ETIMEDOUT like the other broker connection failures that already fall back to a direct app-server. Ports openai#773, with three fixes to the upstream patch: the per-attempt cap now uses the full remaining budget instead of capping at 100ms (which would silently fail a working broker whose connect legitimately takes longer, notably Windows named pipes); each attempt arms one timeout mechanism (a plain setTimeout) instead of both a setTimeout and socket.setTimeout; and the redundant remainingMs <= 0 check duplicating the while condition is dropped.
The Stop hook's own budget (hooks.json, 900s) matched the stop-review child's spawnSync timeout (STOP_REVIEW_TIMEOUT_MS, 15 * 60 * 1000ms). Claude Code killed the hook process at the same instant the child would have hit ETIMEDOUT, so the bypass/manual-review message never made it out and the turn ended silently. Lower the inner timeout to 14 minutes so it fires with headroom to spare before the outer hook budget runs out. Port of openai#772 (upstream openai#766).
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
1 participant
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.
Ports five open upstream fixes into our fork, one commit each so any of them can be reverted alone. Nothing here is a feature. Each upstream pull request is open and unreviewed, so each was reviewed as if written here rather than applied as given, and each test was run against the unfixed code first to confirm it fails for the reason the bug describes.
What now works
Reasoning efforts
maxandultraare accepted. The validator stopped atxhighwhile the models people run support two tiers above it, so those tiers were unreachable from the plugin. The tier list is repeated in the usage text, the error message, the rescue command's argument hint and the runtime skill, and all of them now agree. Upstream openai#761.A file-change event that arrives before its change list is known no longer aborts the run. The code that describes a started item assumed the list was always there and threw a type error, which killed the turn. A missing list now counts as zero changes. Upstream openai#775.
A status check that waits and then times out now says so and exits non-zero. It used to print an ordinary running job and exit zero, so anything polling it read a timed-out wait as a finished check. The machine-readable output still carries the same timeout flag it always did. Upstream openai#774.
A connection attempt to the background broker that never completes now gives up instead of hanging forever, and a timeout is treated like the other connection failures that already fall back to running the server directly. Upstream openai#773.
The stop-time review gate now finishes a minute before the harness kills it, so a slow review reports the timeout and says how to bypass it instead of ending the turn silently. Both timers were set to the same fifteen minutes, so the message could never be delivered. Upstream openai#772.
Deviations from the upstream patches
The broker fix as written upstream capped each connection attempt at a tenth of a second even though the overall budget is two seconds. That would report a working broker as down whenever a connection took longer than that, which is realistic on a loaded machine and on Windows named pipes. Each attempt now uses the remaining budget instead; the hang is still bounded by the overall budget, which is the actual bug. Upstream also armed two separate timers per attempt and repeated the loop's own exit condition inside it; one timer is kept and the repetition is dropped. Upstream left the broker client's new timeout untested, so a test was added for it. Upstream's new test file for the readiness wait is kept as written, and the client test is a second new file, because this repo pairs one test file with one source file and the client lives elsewhere. The new client option is declared in the type definitions alongside the other options.
The stop-gate test upstream pulled the timeout constant out of the source with a pattern and evaluated it as code. That is replaced with a direct numeric comparison, which reads the same values without evaluating anything. It still fails if either the constant or the message drifts from the other.
The crash-fix test upstream also asserted on the exact wording of the runtime's type error. That assertion is dropped: the wording comes from the runtime, not from us, and the check that the run succeeds already fails for the right reason.
New tests that launch the tool use the absolute path of the running interpreter rather than the name
node, because the test helper falls back to a shell on Windows for a relative program name and that mangles arguments.Left out
Upstream openai#770 rewrites the prebuild step so it stops creating a directory literally named
-pon Windows. Our prebuild step no longer creates that directory at all; it was dropped in an earlier commit here, and the type generator creates the directory itself. Verified by deleting the generated directory and running the build, which succeeds. The upstream change cannot apply and the problem it fixes does not exist here.Two further upstream changes were considered and left for a separate decision: openai#768, broker teardown when a live broker misses its readiness probe, and openai#742, a sandbox mode flag, which is a feature rather than a fix and edits contract files that were rewritten here recently.
How it was checked
The full suite and the build were run before and after. Before: 220 tests, 219 pass, 1 skipped, 0 fail, build exits 0. After: 227 tests, 226 pass, 1 skipped, 0 fail, build exits 0. Every pre-existing test still passes. Each fix's test was run against the unfixed code and observed to fail first. All of this ran on macOS; Windows coverage comes from the existing continuous integration job.
One behavior is not covered by a test, stated rather than hidden: adding a timeout to the list of connection failures that fall back to running the server directly. That list lives in a private function reachable only through calls that cannot be given a short timeout, and its two existing entries have no coverage today either. Building that harness was judged out of proportion to a one-word change. The behavior that produces the timeout is covered.
Noticed, not fixed
Two pre-existing problems turned up while reading the code and are untouched here.
The fallback to running the server directly only triggers when the broker address is in the environment. A broker reused from a saved session does not put it there, so a hung connection on that path raises the error instead of falling back. The timeout added in this branch inherits that limit exactly as the two existing connection failures already do, so this widens nothing, but the gap is real. It is in the function that wraps a server call, in the file that holds the Codex client logic.
The usage text for the status command does not list its own wait and timeout options, so the only way to learn them is to read the source.
What to watch after merge
The status change is the one callers will notice: a wait that times out now exits non-zero, in both the readable and the machine-readable forms. Anything scripted around it that treated exit zero as "the check ran" will now see a failure. Also watch for a working broker being reported as unreachable, which would mean the per-attempt budget is still too tight, and for the review gate's timeout message appearing where the turn used to end quietly.