Repository navigation
Conversation
rpc_request raised TimeoutError(method), so a 4s account/rateLimits/read stall stored authHelpText as the bare RPC name and looked like an auth failure. Include the timeout duration in the exception text. Closes omacom#12880
Automated AI review
Verified: On an One issue in the new message and one in the new test case are described below. End of stream is now reported as a timeout
No real Codex binary was run; the stubs emulate the app-server's JSON-RPC stream. Impact: When Codex exits early, for example because it rejects a command-line flag (the situation fixed in #7649), the panel reports a slow 8 s timeout instead of an early exit. That points diagnosis toward load or latency rather than a CLI incompatibility. Suggested change: Give end of stream its own message, for example: line = proc.stdout.readline()
if not line:
raise RuntimeError(f"codex app-server exited before answering {method}")A stub case that exits immediately could assert that text. New EXIT trap drops six cleanup targetsEach earlier section of -trap 'rm -rf "$TEST_HOME" "$PI_HOME" "$OPENCODE_HOME" "$CACHE_HOME" "$FRESH_HOME" "$MALFORMED_HOME" "$UNWRITABLE_HOME" "$INTERRUPTED_HOME"' EXIT # line 548
+trap 'rm -rf "$TEST_HOME" "$PI_HOME" "$TIMEOUT_HOME"' EXIT # line 608Impact: Each run of the test file now leaves six Suggested change: Append Optional context on the timeout itself: In Codex Review informationTest scope: Source review of the pinned head and base, plus isolated runs of the production collector and the scanner test file against synthetic stub app-servers (stall, early exit, error reply and success cases), with base controls. Codex behavior is from reading the AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
Raise a dedicated early-exit error when stdout closes before a reply, restore the cumulative EXIT trap (plus TIMEOUT/EOF homes), and pin XDG_CACHE_HOME for the timeout/EOF collector runs.
|
Thanks for the review — both issues look right. Pushed a follow-up that:
|
Automated AI review
Verified: At
End of stream: resolved
All four cases store Test cleanup: resolvedBoth trap lines in Optional test improvement: The new early-exit stub exits without reading stdin. If it has already exited when the collector writes Optional: The same write-side case can occur in the collector itself, for example when the app-server answers The earlier optional context on the rate-limit timeout (the 5 s reset-credit lookup in Review informationTest scope: Follow-up on AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments of this revision, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
Catch BrokenPipeError on RPC and initialized writes so early app-server exit is reported as exited-before-answering rather than raw EPIPE. Make the EOF test stub read one line first so initialize always hits EOF.
|
Thanks for the verification — prior issues look resolved. Pushed a small follow-up for the optional write-side race:
Happy to leave the rate-limit deadline / |
Automated AI review
Verified: At
Test stub race: resolvedThe early-exit stub now reads one line before exiting, so it cannot exit before the collector's Collector write side: resolved
Without forced ordering, a stub that answers Optional: After a broken pipe, the unsent request stays buffered in Optional test improvement: After this commit, no test reaches the new write-side mapping, because the end-of-stream stub now always takes the read side. This stub fails the #!/bin/bash
read -r _ || exit 0
exec 0<&-
printf '%s\n' '{"id":1,"result":{}}'Review informationTest scope: Follow-up on AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments of this revision, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
Close proc.stdin in fetch_codex_rpc finally so CPython does not emit ignored BrokenPipeError on finalizer flush after a failed write. Add a scanner stub that answers initialize then closes stdin so the initialized notification path is covered.
|
Thanks for the re-verify — both optional notes look right. Pushed a follow-up:
Still leaving rate-limit deadline / |
Automated AI review
Verified: At
Finalizer noise: resolvedThe
The JSON output is identical on both revisions in every case. Success, error-reply, stall (still about 4.07 s) and end-of-stream results are unchanged, and the stub process was gone after every run. Write-side test: resolvedThe new Optional test improvement: The new case reads only stdout, so it still passes if the stdin close is removed (it passes against epipe_result=$(... "$ROOT/bin/omarchy-agent-usage-codex" 2>"$EPIPE_HOME/stderr")
[[ ! -s $EPIPE_HOME/stderr ]] ||
fail "Codex collector leaves no finalizer noise on stderr after a write-side exit" "$(cat "$EPIPE_HOME/stderr")"A narrower Optional context, related open PRs: Four other open PRs change the same collector or test file and differ from this PR, so whichever merges second has to reconcile them (read from their diffs; none was run):
Review informationTest scope: Follow-up on AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
|
Thanks for the re-verify — prior notes look resolved. Pushed a small test follow-up for the optional stderr coverage:
Still leaving rate-limit deadline / |
Capture collector stderr in the EPIPE_HOME case and require it empty so the stdin-close cleanup is covered, not only the authHelpText mapping.
|
Follow-up to the previous note: the stderr assertion is now actually on the branch ( |
|
Thanks — closing in favor of #14049 works for me. Glad the timeout wording is landing there. |
Summary
rpc_requestraisedTimeoutError(method), so a 4saccount/rateLimits/readstall storedauthHelpTextas the bare RPC name. That looked like an auth failure ("Codex limits unavailable" +account/rateLimits/read) instead of a timeout.Include the timeout duration in the exception text.
Test plan
codexthat sleeps onaccount/rateLimits/read:usageStatusTextisCodex limits unavailableandauthHelpTextisaccount/rateLimits/read timed out after 4s.test/shell.d/agent-usage-codex-scanner-test.sh.Closes #12880