refactor: extract fix command#163
refactor: extract fix command#163ndycode wants to merge 3 commits intorefactor/pr1-doctor-commandfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughrefactored Changes
Sequence DiagramsequenceDiagram
participant CLI
participant Manager as runFix<br/>(manager.ts)
participant FixCmd as runFixCommand<br/>(fix.ts)
participant Storage
participant Cache as Quota Cache
participant Token as Token/QoS
participant Forecast
participant Output as Formatter
CLI->>Manager: runFix(args)
Manager->>FixCmd: runFixCommand(args, deps)
FixCmd->>FixCmd: parseArgs()
alt --help flag
FixCmd->>Output: printFixUsage()
Output-->>FixCmd: exit 0
else --bad flag
FixCmd->>FixCmd: log error
FixCmd-->>Manager: exit 1
else normal flow
FixCmd->>Storage: loadStorage()
Storage-->>FixCmd: accounts[]
alt --live mode
FixCmd->>Cache: loadAndClone()
Cache-->>FixCmd: quotaCache
end
loop per account
FixCmd->>FixCmd: classify state
alt needs token refresh
FixCmd->>Token: refresh tokens
Token-->>FixCmd: success/failure
alt live mode
FixCmd->>Forecast: probeQuota()
Forecast-->>FixCmd: quota state
end
else live quota check
FixCmd->>Forecast: probeQuota()
Forecast-->>FixCmd: quota state
end
FixCmd->>FixCmd: apply lockout avoidance
end
FixCmd->>FixCmd: summarizeFixReports()
alt not dryRun
FixCmd->>Storage: persist accounts
alt live mode
FixCmd->>Cache: persist quotaCache
end
end
alt json output
FixCmd->>Output: JSON + recommendation
else formatted output
FixCmd->>Output: human-readable + rows
end
FixCmd-->>Manager: exit 0
end
Manager-->>CLI: exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes detailed concernsregression testing gap: test coverage is thin relative to logic density.
concurrency risk: account iteration in the scan loop ( windows edge cases: quota cache path handling and file I/O in cache load/clone/persist steps ( dependency injection opacity: 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/commands/fix.ts`:
- Around line 427-430: The quota-cache is currently saved even during --dry-run;
update both locations that call deps.saveQuotaCache so they only run when
options.json is true AND options.dryRun is false (i.e., change the condition to
check !options.dryRun && options.json && workingQuotaCache && quotaCacheChanged
around the existing saveQuotaCache calls), mirror the same gating for the second
occurrence near the later save, and add/adjust the test
test/codex-manager-fix-command.test.ts to assert that a dry-run does not invoke
deps.saveQuotaCache; additionally, to address Windows IO and concurrency, wrap
the saveQuotaCache invocation (or implement in deps.saveQuotaCache) with a small
retry/backoff/queue that handles EBUSY and HTTP 429 transient errors.
- Around line 176-179: The early return when storage is empty bypasses the
--json output path; change the empty-storage branch in the handler that checks
storage (the block using storage and logInfo) to emit machine-readable JSON when
the json flag is set (e.g., an empty accounts array or a structured result)
instead of calling logInfo and returning immediately—use the same JSON output
helper/format used elsewhere in this command (or check the options.json flag) so
`codex auth fix --json` returns valid JSON on the empty-storage path and then
return the correct exit code.
- Around line 207-264: The live-probe catch currently pushes a
"warning-soft-failure" but then the code immediately appends a second "healthy"
report and continues, preventing the refresh fallback (queuedRefresh) from
running; change the control flow so that when deps.fetchCodexQuotaSnapshot(...)
inside the if (options.live) path throws, you do NOT add the "access token still
valid" healthy report or continue, but instead let execution fall through into
the refresh logic (or explicitly call the queuedRefresh path) so that
queuedRefresh/refresh handling can run; update logic around
deps.fetchCodexQuotaSnapshot, reports.push, deps.updateQuotaCacheForAccount and
deps.hasLikelyInvalidRefreshToken to ensure only one report is emitted per
account, and add/adjust tests (test/codex-manager-fix-command.test.ts:71-92) to
assert the revoked-but-not-expired session goes through queuedRefresh and that
EBUSY/429 retry behavior for refresh queues is covered.
🪄 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: 21cd04c9-9fa2-4c56-aaa3-67dac635bf7f
📒 Files selected for processing (3)
lib/codex-manager.tslib/codex-manager/commands/fix.tstest/codex-manager-fix-command.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 (2)
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-fix-command.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/codex-manager.tslib/codex-manager/commands/fix.ts
🔇 Additional comments (1)
lib/codex-manager.ts (1)
2671-2702: delegation boundary looks clean.lib/codex-manager.ts:2671-2702 passes the storage, quota, auth-refresh, and formatting helpers the extracted command needs, so this stays a thin adapter around the new module. the command-level entry paths are at least smoke-covered in test/codex-manager-fix-command.test.ts:71-92.
ndycode
dismissed
coderabbitai[bot]’s stale review
All review threads are resolved and follow-up commits addressed this stale automated change request.
1 participant
Summary
codex auth fiximplementation into a dedicated command moduleWhat Changed
lib/codex-manager/commands/fix.tsfor auto-fix scanning, refresh/live-probe handling, recommendation output, and JSON/human output pathslib/codex-manager.tssorunFix(...)delegates to the extracted fix command module through injected dependenciestest/codex-manager-fix-command.test.tsfor command-level coverageValidation
npm run test -- test/codex-manager-fix-command.test.ts test/codex-manager-cli.test.tsnpm run lintnpm run typechecknpm run buildRisk and Rollback
7b98866to restore the inline fix implementationAdditional Notes
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
clean extraction of
codex auth fixintolib/codex-manager/commands/fix.tsusing the established dep-injection pattern.codex-manager.tsnow delegates torunFixCommandand the new test file adds five targeted unit cases. the refactor also silently fixes a bug wheresaveQuotaCachewas called in dry-run mode — this behavioral change is correct and tested but should be called out explicitly in the PR description.lib/codex-manager/commands/fix.ts: 561-line extracted command; exportsFixCliOptions,FixAccountReport,FixCommandDeps,summarizeFixReports, andrunFixCommandlib/codex-manager.ts:runFixreduced to a singlereturn runFixCommand(args, { ... })call;TokenFailureimport correctly droppedtest/codex-manager-fix-command.test.ts: covers help, invalid-option, empty-storage (json), live-probe-failure, dry-run no-persist, and human-mode previewsaveQuotaCachewith!options.dryRun— the old code would persist quota cache even during a dry-run, which was a silent data mutation bugsaveAccountsaffirmative write are not directly exercised in the new test file; confirmcodex-manager-cli.test.tscovers them end-to-endConfidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant CLI as codex-manager.ts<br/>runFix(args) participant CMD as fix.ts<br/>runFixCommand(args, deps) participant Store as deps.loadAccounts participant QC as deps.loadQuotaCache participant Refresh as deps.queuedRefresh participant Probe as deps.fetchCodexQuotaSnapshot participant Disk as deps.saveAccounts<br/>deps.saveQuotaCache CLI->>CMD: delegate(args, injected deps) CMD->>Store: loadAccounts() Store-->>CMD: storage | null alt empty storage CMD-->>CLI: 0 (json or plain msg) end CMD->>QC: loadQuotaCache() [if --live] loop each account alt token usable + --live CMD->>Probe: fetchCodexQuotaSnapshot() Probe-->>CMD: snapshot | throws else token expired CMD->>Refresh: queuedRefresh(refreshToken) Refresh-->>CMD: success | failure alt --live after refresh CMD->>Probe: fetchCodexQuotaSnapshot() Probe-->>CMD: snapshot | throws end end end alt !dryRun && changed CMD->>Disk: saveAccounts(storage) end alt !dryRun && quotaCacheChanged CMD->>Disk: saveQuotaCache(workingCache) end CMD-->>CLI: exit code 0Comments Outside Diff (4)
lib/codex-manager/commands/fix.ts, line 691-692 (link)runFixcallers share the same storage file without a locksetStoragePath(null)→loadAccounts()→ mutate accounts in the loop →saveAccounts()is not atomic. on a machine with multiple terminals (common on windows where users run the CLI side-by-side), two concurrentcodex auth fixinvocations can race: both read the same file, both apply their mutations independently, and the second write silently clobbers the first. the hard-disable lockout recovery at line ~904 is particularly dangerous here — account A'senabled = trueanti-lockout write could be lost.this is pre-existing behavior from the inline implementation, but the extraction is a good moment to call it out. consider at minimum adding a comment noting that callers must not be run concurrently, or passing a write-queue/mutex dep (consistent with the
unified-settings.tsEBUSY/EPERM queued-write pattern already in the codebase).Prompt To Fix With AI
lib/codex-manager/commands/fix.ts, line 779-782 (link)silent catch changes the live-probe fallback flow and drops warning report
the original code, when a live probe fails for an account with a usable access token, did two things: (1) pushed a
"warning-soft-failure"report with the message"live probe failed (${message}), trying refresh fallback", then (2) fell through to push a second"healthy"report ("access token still valid") andcontinue-d — it did not callqueuedRefresh.the new code sets
needsRefresh = trueand falls through toqueuedRefresh. this means:"warning-soft-failure"probe-failure entry is silently dropped fromreportsand from json output, reducing observabilityqueuedRefresh) is made for every account whose probe fails but whose access token is still valid — this is new behaviour not present in the originalthe pr description says it's "preserving the existing fix CLI behaviour", but this is a divergence. the new test titled "falls back to refresh when live probe fails" is explicitly testing the new behaviour (the old code never called
queuedRefreshin this branch).if the intent is to fix the double-report bug in the original, that's worth documenting. if the intent is strict parity, restore the warning push and skip the refresh:
lib/codex-manager/commands/fix.ts, line 961-964 (link)new
dryRunguard onsaveQuotaCachein json path is an undocumented behaviour changethe original
runFixdid not guardsaveQuotaCachewith!options.dryRunin the json branch —--dry-run --jsonwould still persist the quota cache. the new code adds&& !options.dryRun, which is arguably the correct behaviour (dry-run should not write anything), but it silently changes observable CLI semantics.this should be called out in the pr description and should have an explicit test. the existing
"does not persist quota cache during dry-run"test only exercises the human (non-json) code path (no json guard), so it doesn't directly cover this branch.add a companion test:
lib/codex-manager/commands/fix.ts, line 450-453 (link)silent behavioral fix not mentioned in PR description
the old inline
runFixincodex-manager.tscalledawait saveQuotaCache(workingQuotaCache)in the--jsonpath without a!options.dryRunguard — meaningcodex auth fix --dry-run --json --livewould mutate the quota cache on disk despite being a preview run. the new code correctly adds&& !options.dryRunhere and at line 551, and the test "does not persist accounts or quota cache during dry-run json mode" validates the fix. this is a real behavior change that should be called out in the PR description, since it's not a pure refactor.Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: preserve live probe fallback report..." | Re-trigger Greptile