Skip to content

Show the Codex CLI's error instead of "initialize" when the usage RPC fails - #8877

Closed
betizzel wants to merge 2 commits into
omacom:quattrofrom
betizzel:fix/issue-8849
Closed

betizzel wants to merge 2 commits into
omacom:quattrofrom
betizzel:fix/issue-8849

Conversation

@betizzel

@betizzel betizzel commented Aug 29, 2026 •

Copy link
Copy Markdown

Fixes: #8849

What happened

The agents bar panel's Codex tab shows the usage status CODEX LIMITS UNAVAILABLE, and the auth-help card reads initialize, it doesn't give the user any indication on what's going on.

Root cause

rpc_request() in bin/omarchy-agent-usage-codex raises TimeoutError(method) when the codex app-server process never answers, and fetch_codex_rpc() stored that exception verbatim:

except Exception as exc:
    result["usageStatusText"] = "Codex limits unavailable"
    result["authHelpText"] = str(exc)   # -> "initialize"

So any RPC failure surfaces the raw method name. The common trigger is a codex-cli upgrade that drops or renames a flag the collector passes (e.g. -a untrusted), removed in codex-cli 0.149 in favour of on-request/never (the flag value itself is fixed separately in #7649). The CLI exits immediately:

error: invalid value 'untrusted' for '--ask-for-approval <APPROVAL_POLICY>'
  [possible values: on-request, never]

initialize never gets a response, rpc_request() raises, and the panel shows initialize.

Change

  • Capture the app-server's stderr to a temp file (a regular file, not a PIPE, so an unread buffer can't deadlock a server that logs while it runs) instead of subprocess.DEVNULL.
  • On RPC failure, codex_rpc_help() picks the most actionable text, in order:
    1. The CLI exited and printed to stderr. Quote it, all non-empty lines, so clap's [possible values: ...] line survives:
      Codex limits unavailable
      codex app-server exited: error: invalid value 'untrusted' for '--ask-for-approval <APPROVAL_POLICY>' [possible values: on-request, never]
      
    2. The CLI exited without a word. The "not logged in" shape, so keep the existing Run \codex login` to authenticate.` hint.
    3. The process is still running. The RPC stalled or returned something the collector couldn't parse. That is not an auth problem, so it must not send the user to 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-server is unchanged here. That is #7649's fix.

Tests

test/shell.d/agent-usage-codex-scanner-test.sh gains cases for each branch:

  • stub codex writes a two-line clap error and exits: asserts both stderr lines reach authHelpText and the bare method name does not.
  • stub codex exits 0 without speaking the protocol: asserts usageStatusText is Codex limits unavailable and authHelpText is the codex login hint.
  • stub codex answers initialize then never answers account/read (process stays up): asserts authHelpText keeps the stalled method name and is not the login hint.
  • stub codex returns a rate-limit window with a non-numeric usedPercent (process stays up): asserts authHelpText keeps the parse error and is not the login hint.

./test/shell and ./test/cli pass.

Fixes #8849.

Copilot AI balanced review requested due to automatic review settings August 29, 2026 06:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at c17d29a against quattro, with every test run on a disposable VM rather than locally. Reviewed by Claude Opus 5 in Claude Code, with an independent second opinion from Codex at xhigh reasoning; below I have separated what each of us contributed and what I checked and dropped.

The flag is already fixed on quattro

Worth stating up front, because four open issues sit under this and a maintainer reading the thread could close more than this PR actually closes. bin/omarchy-agent-usage-codex:531 on origin/quattro already reads -a on-request:

[codex, "-s", "read-only", "-a", "on-request", "app-server"],

That landed in 4cd8a08 ("Fix Codex usage collection on 0.149", #7649). git tag --contains 4cd8a081 comes back empty and v4.0.1 is still the latest release, so what every reporter is running is a package built before the fix. Your description already says the flag is #7649's job and doesn't claim otherwise — I am confirming it, because #8868, #8460 and #8398 all diagnose a stale flag that is no longer stale on the base branch, and what those three need is a release rather than a code change. #8849 is the exception: it asks for either a compatible flag or an auth-help text that explains the real failure, and quattro will have both once this merges. Fixes #8849 is correct, and it is the only one of the four this closes.

What works

Against the real codex-cli 0.150.1 on the worker, codex -s read-only -a untrusted app-server puts its error on the first line of stderr, and the collector now produces exactly:

Codex limits unavailable
codex app-server exited: error: invalid value 'untrusted' for '--ask-for-approval <APPROVAL_POLICY>'

That is the case this PR is built for and it does what it says. The regular-file-not-PIPE choice is right, the parent reads the child's bytes correctly after exit, errors="replace" keeps a non-UTF-8 line from crashing the collector, and stderr_file is closed on every path including the Popen failure.

One defect worth fixing before this merges

Every failure where the app-server is still alive now reports an authentication problem. codex_rpc_help at :535 reads stderr only when proc.poll() is not None, so a live process falls through to AUTH_HELP at :544 — and the except Exception at :590 catches far more than a CLI that refused to start. Two cases I reproduced on the worker, each against your head and against the same tree with only the authHelpText line reverted:

A slow or hung app-server that answers initialize and then goes quiet on account/read:

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_help at :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 authHelpText equals AUTH_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". Asserting usageStatusText == "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>
@betizzel

Copy link
Copy Markdown
Author

Thanks for the thorough pass. Pushed a fix for the live-process finding (now at 2b0acc86).

codex_rpc_help checks proc.poll() first now. If the process is still running, the RPC stalled or returned something the collector couldn't parse, which is not an auth problem, so it keeps str(exc) (the text the panel showed before this PR) instead of AUTH_HELP. Both cases you reproduced, the account/read stall and the non-numeric usedPercent, keep their original text. Only an exited process gets stderr quoted, or the login hint if it exited silently.

Also in this push:

  • Quote every non-empty stderr line, not just the first, so clap's [possible values: on-request, never] line survives.
  • The silent-exit test now also asserts usageStatusText == "Codex limits unavailable", so it can't pass without reaching the error path.
  • Two new tests: a stub that answers initialize then stalls on account/read (process stays up), and a stub that returns an unparseable rate-limit window. Both assert the login hint is not shown.

On the #8073 interaction: I've left the exited-silently branch returning AUTH_HELP. Once #8073's rpc_request raises a sentence instead of TimeoutError(method), that branch should prefer the exception message, but with today's rpc_request it only ever sees the bare word initialize there, which is what this PR is trying to suppress. Happy to add that follow-up in whichever order the two land.

One case where the text is unchanged: an app-server that hangs on initialize itself, process still alive, still shows initialize. That is now accurate rather than misleading, since the process really is stuck on that method.

@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at 2b0acc86. The blocker is fixed, and the fix also dissolves the #8073 collision. Nothing new found in the delta.

The live-process finding is genuinely fixed. Verified by mutation on a disposable worker: deleting the two-line guard at bin/omarchy-agent-usage-codex:540-541 makes test/shell.d/agent-usage-codex-scanner-test.sh:680 fail with exactly "authHelpText":"Run `codex login` to authenticate.", and the same deletion makes the malformed-usedPercent case regress to the login hint too (that one is not visible in the suite output because fail() exits on the first failure, so I ran it as a standalone probe). Restored afterwards. The ordering is right, incidentally: proc.wait(timeout=1) sets the return code if the child is already gone, and poll() then re-checks with a fresh waitpid, so there is no stale return code. The only race is a child exiting in the instant after poll(), which yields the raw exception text rather than the login hint — the safe direction.

No process leak. I checked the case that would actually leak: a stub that installs trap "" TERM and then stalls. proc.terminate() is ignored, proc.wait(timeout=1) raises, and the proc.kill() at line 607 takes it down. No child survived the collector. kill() is not followed by a wait(), so the child is unreaped for the microseconds until this one-shot collector exits — not worth changing.

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 2b0acc86 on the worker and the combined suite is 27 passed, 0 failed, and git merge-tree reports a clean merge. The reason is your new branch, not luck — both of #8073's stubs stay alive (sleep 2) after closing the pipe, so its RuntimeError("Codex app-server exited before initialize") and ...before initialized" are exactly what the live-process branch now preserves as str(exc). The follow-up you offered in your comment is not needed; these two compose as they stand.

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 usageStatusText assertion at line 650 does distinguish the error path from the record's initial "" at line 556, which is what that note asked for.

Two low notes, neither a blocker:

  • ' '.join(lines) at line 551 keeps the first 300 characters, so a chatty stderr buries the fatal error, which a dying process writes last. A 13-line stub renders as codex app-server exited: ...INFO codex_app_server: starting subsystem number 1 of 12 ...2 of 12 ...3 of 12 ... with the error: line truncated away. Not reachable with today's CLI — a normal codex app-server run wrote 0 bytes to stderr in 2 seconds on a clean worker — so this is only worth the one-word change to truncate from the end if you feel like it.
  • The PR's goal is not met for one shape of CLI error, and it is the shape fix: detect early Codex app-server exits #8073 is about to touch. rpc_request returns a matching message without inspecting "error", and fetch_codex_rpc:589-590 turns a missing "result" into {} without raising, so codex_rpc_help never runs. A stub answering {"id":2,"error":{"code":-32601,"message":"method not found: account/read (renamed in codex-cli 0.152)"}} produces usageStatusText:"" and authHelpText:"Run \codex login` to authenticate."— and sinceshell/plugins/agents/Panel.qml:499gates the status card on a non-emptyusageStatusText, the card is hidden entirely: the user sees empty limits and no explanation at all, and the CLI's actual message is discarded. This is pre-existing and unchanged by your diff, so not a regression, but it is the last place initialize`-style silence still hides.

What was run. Disposable worker (destroyed afterwards). test/shell.d/agent-usage-codex-scanner-test.sh through the shell-test wrapper: 25 passed, 0 failed. ./test/cli: 116 passed, 0 failed, exit 0. Plus the mutation, leak, JSON-RPC-error, chatty-stderr and #8073-combination probes described above.

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 2b0acc86 with the head SHA and function body as a gate. On the pinned run it found no defect in the delta and reached the #8073 conclusion independently by reasoning through the two stubs' timing, which is corroboration of a mechanism rather than agreement with a conclusion; it also enumerated the exception types reachable in the try block to confirm str(exc)[:300] or AUTH_HELP cannot fall through to the login hint for a live process. Where it agreed with things already concluded here, note that its independence is not currently guaranteed. Its one claim I did not adopt: that the new usageStatusText assertion is worthless because the old code also set that string — true but beside the point, since the assertion exists to separate the error path from the record's initial value, and it does.

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>
@omarchybot omarchybot added the verified Omarchy Triage has verified that this issue is ready for final review label Oct 1, 2026
@omarchybot

Copy link
Copy Markdown
Collaborator

This had stopped merging into quattro, so I merged quattro into your branch and pushed it as a538c21. Your two commits are unchanged.

The conflict. #13733 and #13770 both rewrote fetch_codex_rpc since the last review. The limits are now asked for before account/read, account/read is a two-second fallback whose timeout is swallowed, and the function takes a home for multi-account. Your codex_rpc_help and quattro's reset_credits both land, and your except branch still calls codex_rpc_help. One test had to follow the new call order: the stalled-server stub now hangs on account/rateLimits/read rather than account/read, and expects that method name back. A hang on account/read no longer fails anything on quattro, so the old stub was testing a path that is gone.

Verified on a disposable worker against quattro c05d901. test/shell.d/agent-usage-codex-scanner-test.sh passes 28 of 28 at a538c21. The same test file run against unmodified quattro fails "surfaces the CLI's own error" with "authHelpText":"initialize", and so does the head with only codex_rpc_help(...) reverted to str(exc). The other agent-usage suites pass, including agent-usage-accounts-test.sh, which exercises the new per-home path. ./test/cli shows 91 ok and 1 failure, "vscode generated theme references current theme file". That failure is identical on unmodified quattro and unrelated to this change.

A competing fix. #13740 changes the same except branch differently: a timeout becomes "Codex didn't answer the usage request in time", and anything else becomes "Couldn't read usage limits from Codex." Neither covers every case alone:

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.

@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 3/5

[Medium risk] Changes error handling in the Codex CLI integration.

The PR should not merge until temporary-file failures preserve collector output and running app-servers’ actionable diagnostics are surfaced.

Findings

  1. P1 Temporary file failure aborts collection ▶
  2. P1 Running server errors stay hidden ▶
  3. P2 Silent exits imply wrong fix ▶
  4. P2 Failure handling delays refreshes ▶
  5. P2 Stderr processed without a bound ▶

Summary

The PR captures Codex app-server stderr and uses it to explain RPC failures when the CLI exits, with tests for error output, silent exit, stalled RPCs, and malformed limits.

  • The exited-with-stderr case now supplies a CLI diagnostic instead of a bare RPC method.
  • Failure handling still loses diagnostics from a running server and can abort before producing JSON if temporary-file creation fails.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[RPC failure] --> B{App-server exited?}
  B -- No --> C[Show exception text]
  B -- Yes --> D{Captured stderr has text?}
  D -- Yes --> E[Show truncated CLI stderr]
  D -- No --> F[Show login hint]
Loading

Reviews (1) · Last reviewed commit: "Merge quattro into fix/issue-8849"

# 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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Temporary file failure aborts collection If the temporary directory is unavailable, full, or unwritable, creating this file raises before the error handler begins. The exception reaches main(), so the collector prints no JSON instead of returning a Codex-unavailable record.

Comment on lines +540 to +541
if proc.poll() is None:
return str(exc)[:300] or AUTH_HELP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Running server errors stay hidden If an app-server writes an error to stderr but remains alive after an RPC failure, this branch ignores that error and shows only the RPC method name. The user still cannot see why the server failed to answer.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent exits imply wrong fix If the app-server exits without stderr for a reason unrelated to authentication, this fallback still tells the user to run codex login. That can send an already signed-in user toward a fix that will not address the exit.

searchable, so keep it.
"""
try:
proc.wait(timeout=1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Failure handling delays refreshes This waits for up to a second even when a live server has already produced a parse error or an RPC has timed out. Each failed account probe can add that delay to a refresh without improving the diagnosis while the server remains running.

Comment on lines +545 to +551
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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stderr processed without a bound If an exited app-server wrote a large amount to stderr, this reads and assembles all of it before limiting the displayed result to 300 characters. Verbose failures can consume substantial temporary storage and collector memory for text the panel will never show.

@omarchybot

Copy link
Copy Markdown
Collaborator

Greptile's five comments on a538c21, checked against the source. None blocks this, and the head is unchanged.

  • Temporary file failure aborts collection (bin/omarchy-agent-usage-codex:579): this holds. tempfile.TemporaryFile is outside the try, and neither main() nor account_limits() catches around fetch_codex_rpc, so a failure there means no JSON at all. It is hard to reach, though. Python falls back from $TMPDIR to /tmp, /var/tmp, /usr/tmp and the working directory, so every one of them has to be unusable before an empty file can't be created. If you want it closed, wrap the call, fall back to stderr=subprocess.DEVNULL with stderr_file = None, and guard the two close() calls. codex_rpc_help already handles None. Low.
  • Running server errors stay hidden (:541): this is accurate, and it is the design the docstring states. Stderr is quoted only once the process has exited. The file is a regular file, so reading it from a live server would be safe, but whether a live server's log lines belong in the help card is a wording decision, and it overlaps Keep RPC method names out of the Codex limits help text #13740. Judgement call, not a defect.
  • Silent exits imply wrong fix (:552): this is the deliberate "exited without a word" branch the docstring describes, raised and settled in the first review. Judgement call.
  • Failure handling delays refreshes (:537): this holds. While the server is still alive, proc.wait(timeout=1) adds a second before the finally block's own terminate-and-wait. For a timeout, that comes on top of the 8 seconds already spent. It is one extra second, and only on a failed probe. Low.
  • Stderr processed without a bound (:551): this holds. The whole file is read before the 300-character cut. The file only holds what a process wrote before it died, though, and a normal codex app-server run writes nothing to stderr. Low. An earlier review here noted that a cut from the end would keep the fatal line that a chatty stderr would otherwise push out.

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 quattro.

@dhh

dhh commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Thanks! The same fix is coming in through #14049 (from #8977, which also shows the CLI's stderr). Closing in favor of that.

@dhh dhh closed this Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working verified Omarchy Triage has verified that this issue is ready for final review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agents panel: Codex limits show 'initialize' — stale --ask-for-approval flag value

4 participants