Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Improves Codex usage failure reporting by surfacing app-server stderr instead of raw RPC method names.
Changes:
- Captures Codex stderr and provides a login fallback.
- Adds regression tests for rejected and silent app-server exits.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
bin/omarchy-agent-usage-codex |
Reports actionable Codex RPC failures. |
test/shell.d/agent-usage-codex-scanner-test.sh |
Tests stderr reporting and silent-exit fallback. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Reviewed at c17d29a against The flag is already fixed on
|
| authHelpText | |
|---|---|
| before | account/read |
| this PR | Run `codex login` to authenticate. |
A healthy, authenticated app-server that returns a rate-limit window the collector cannot parse (non-numeric usedPercent):
| authHelpText | |
|---|---|
| before | could not convert string to float: 'not-a-number' |
| this PR | Run `codex login` to authenticate. |
The second is Codex's find rather than mine, and it is the more convincing of the two: nothing is wrong with the user's login, and they are sent to codex login, where everything will look fine. The old strings were cryptic but true and searchable; the new one is confident and wrong. Reserving AUTH_HELP for the case your docstring describes — the CLI exited and said nothing — and falling back to the exception's own message while the process is still running would keep everything this PR gains. I have not pushed that, because what a user should read there is your call, and the point of your change is precisely to stop showing raw exception text.
Two smaller things
codex_rpc_helpat :541 quotes only the first non-empty stderr line, which drops clap's second line,[possible values: on-request, never]— the half that says what to change. Both lines fit well inside the 300-character budget. Codex raised a stronger version of this, arguing a startup warning could precede the error and hide it entirely; I could not reproduce that against the real CLI on a clean worker, where the error is line one, so I am only passing on the milder point.- The second new test at :625 asserts
authHelpTextequalsAUTH_HELP, which is also the value the record is initialised with at :548, so it cannot distinguish "fell back correctly" from "never reached the error path at all". AssertingusageStatusText == "Codex limits unavailable"as well, the way your first new test does, would close that.
It collides with #8073
#8073 solves the neighbouring problem: it makes rpc_request raise Codex app-server exited before <method> rather than waiting out the timeout. I applied its patch on top of yours on the worker. It merges textually, but its first new test then fails, because its stub exits without writing to stderr and your handler replaces its message with the login hint:
not ok - Codex collector identifies an app server that exits during startup
... "authHelpText":"Run `codex login` to authenticate."
Same root cause as the finding above, and the same change fixes it. Worth knowing before either lands, since together the two of you would cover the failure space properly.
Tests
./test/shell.d/agent-usage-codex-scanner-test.sh through the shell-test wrapper: 23 passed, 0 failed. ./test/cli: 116 passed, exit 0. I mutation-tested your new coverage by reverting authHelpText = codex_rpc_help(proc, stderr_file) to str(exc) and re-running, and the suite fails on "Codex collector surfaces the CLI's own error in the auth-help text" with "authHelpText":"initialize" — so the test genuinely pins the behaviour rather than passing regardless.
What Codex added, and what I dropped
Its independence is not currently guaranteed, since it can read this session's own transcript, so agreement is worth less here than a new mechanism. It contributed the malformed-rate-limit case above, which I had not reasoned about and then reproduced, and the #8073 interaction, which I then verified by applying the patch; it agreed with the live-process finding I had already reached. I dropped two of its findings: that an unwritable $TMPDIR now crashes the collector — tempfile falls through to /tmp, /var/tmp, /usr/tmp and the cwd, and an unwritable TMPDIR runs fine, its repro needing all four unusable, by which point the desktop session is gone anyway — and the leading-warning ordering above.
Waiting on you for the live-process fallback; after that it is the maintainer's call, along with the sequencing against #8073.
fetch_codex_rpc() stored str(exc) in authHelpText, and rpc_request() raises TimeoutError(method), so a codex-cli that exits before answering the RPC (a flag dropped or renamed in a later release) showed up in the agents panel as the bare word "initialize". Capture the app-server's stderr to a temp file (a regular file, so an unread pipe can't deadlock a server that logs while it runs). On failure, codex_rpc_help() now chooses in order: quote the CLI's stderr if it exited and printed something; fall back to the "run codex login" hint if it exited silently; keep the raw exception text if the process is still running, since a stalled or unparseable response is not an auth problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
c17d29a to
2b0acc8
Compare
|
Thanks for the thorough pass. Pushed a fix for the live-process finding (now at
Also in this push:
On the #8073 interaction: I've left the exited-silently branch returning One case where the text is unchanged: an app-server that hangs on |
|
Re-reviewed at The live-process finding is genuinely fixed. Verified by mutation on a disposable worker: deleting the two-line guard at No process leak. I checked the case that would actually leak: a stub that installs The #8073 collision is gone. Last pass, #8073's tests failed when both patches were applied. They no longer do: applied #8073 on top of The other two smaller notes from last pass are closed as well: all non-empty stderr lines now survive (confirmed against the real thing — codex-cli 0.151.0 emits the clap error as three lines totalling well under the 300-char budget), and the Two low notes, neither a blocker:
What was run. Disposable worker (destroyed afterwards). Second opinion. Codex at xhigh reviewed the delta. Its first run read a stale tree — it cited a worktree path that does not exist and reported the already-fixed findings — so it was re-run pinned to Nothing pushed. This is ready as far as I can tell; the merge decision is the maintainer's. |
quattro now asks for account/rateLimits/read before account/read and treats an account/read that never answers as harmless (omacom#13733), so the stalled-app-server test stalls on the limits read, which is the call a hung server now fails on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This had stopped merging into The conflict. #13733 and #13770 both rewrote Verified on a disposable worker against quattro c05d901. A competing fix. #13740 changes the same
The two also conflict line for line. Which wording the panel should use is the maintainer's call, so I have not changed either. Who checked it. Reviewed by Claude Opus 5.5 in Claude Code. The second opinion, Codex at medium, did not run on this head: the bot's daily review budget was spent, so nothing here has been checked by a second model. Waiting on the maintainer: to choose between this and #13740, or ask for one to take the other's case. |
|
| # A regular file, not a PIPE: the app-server can log to stderr while it runs, | ||
| # and an unread pipe would deadlock it once the buffer filled. Only read on | ||
| # failure, and only once the process has exited (see codex_rpc_help). | ||
| stderr_file = tempfile.TemporaryFile("w+", errors="replace") |
There was a problem hiding this comment.
| if proc.poll() is None: | ||
| return str(exc)[:300] or AUTH_HELP |
| lines = [line.strip() for line in detail.splitlines() if line.strip()] | ||
| if lines: | ||
| return f"codex app-server exited: {' '.join(lines)}"[:300] | ||
| return AUTH_HELP |
| searchable, so keep it. | ||
| """ | ||
| try: | ||
| proc.wait(timeout=1) |
There was a problem hiding this comment.
| stderr_file.seek(0) | ||
| detail = stderr_file.read() | ||
| except Exception: | ||
| detail = "" | ||
| lines = [line.strip() for line in detail.splitlines() if line.strip()] | ||
| if lines: | ||
| return f"codex app-server exited: {' '.join(lines)}"[:300] |
There was a problem hiding this comment.
|
Greptile's five comments on a538c21, checked against the source. None blocks this, and the head is unchanged.
Who checked it. Read by Claude Opus 5.5 in Claude Code; nothing was run this pass, so the worker results in the previous comment still stand for this head. The second opinion, Codex at medium, answered a ping but refused the review because the bot's daily review budget was spent. Nothing here has been checked by a second model. Still waiting on the maintainer to choose between this and #13740, which now conflicts with |
Fixes: #8849
What happened
The agents bar panel's Codex tab shows the usage status
CODEX LIMITS UNAVAILABLE, and the auth-help card readsinitialize, it doesn't give the user any indication on what's going on.Root cause
rpc_request()inbin/omarchy-agent-usage-codexraisesTimeoutError(method)when thecodex app-serverprocess never answers, andfetch_codex_rpc()stored that exception verbatim:So any RPC failure surfaces the raw method name. The common trigger is a
codex-cliupgrade that drops or renames a flag the collector passes (e.g.-a untrusted), removed in codex-cli 0.149 in favour ofon-request/never(the flag value itself is fixed separately in #7649). The CLI exits immediately:initializenever gets a response,rpc_request()raises, and the panel showsinitialize.Change
PIPE, so an unread buffer can't deadlock a server that logs while it runs) instead ofsubprocess.DEVNULL.codex_rpc_help()picks the most actionable text, in order:[possible values: ...]line survives:Run \codex login` to authenticate.` hint.codex login; keep the raw exception text (cryptic but true and searchable, and exactly what the panel showed before this change).The flag value passed to
codex app-serveris unchanged here. That is #7649's fix.Tests
test/shell.d/agent-usage-codex-scanner-test.shgains cases for each branch:codexwrites a two-line clap error and exits: asserts both stderr lines reachauthHelpTextand the bare method name does not.codexexits0without speaking the protocol: assertsusageStatusTextisCodex limits unavailableandauthHelpTextis thecodex loginhint.codexanswersinitializethen never answersaccount/read(process stays up): assertsauthHelpTextkeeps the stalled method name and is not the login hint.codexreturns a rate-limit window with a non-numericusedPercent(process stays up): assertsauthHelpTextkeeps the parse error and is not the login hint../test/shelland./test/clipass.Fixes #8849.