feat(cli): add codex auth why-selected and verify --paths commands#410
feat(cli): add codex auth why-selected and verify --paths commands#410
Conversation
Closes: Phase 1 §9 F1, F2 (plan PR-P). - Extend lib/rotation.ts with selectHybridAccountTraced: non-mutating variant that mirrors the hybrid scoring logic of selectHybridAccount and returns a per-candidate breakdown (health, tokens, freshness, capability boost, pid bonus, score). selectHybridAccount signature is unchanged and a parity test asserts both agree on the winning index. - Add codex auth why-selected [--now|--last] [--json]: live recomputation of the current selection decision with per-candidate reasons. --last additionally consults the persisted runtime observability snapshot when available; no persistent selection tracker exists yet (trace-mode approach), so --last falls back to live recomputation with a user-visible note. - Add codex auth verify [--paths|--flagged|--all] [--json]: --paths walks the storage path resolution chain (cwd -> findProjectRoot -> resolveProjectStorageIdentityRoot -> storage key -> project config dir -> global project dir) and self-tests resolvePath() with known-good (home, tmp) and known-bad (UNC / /etc/shadow) inputs to confirm the PR-A sandbox guard is working. --flagged delegates to the existing verify-flagged command; --all runs both. The verify-flagged top-level verb is kept as a back-compat alias. - Update help usage block and wire both verbs into runCodexMultiAuthCli. - Tests: 40 new tests across rotation, command, parser, and dispatcher layers. Full suite: 3458 pass (up from 3418).
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function buildSelectAccountTraced(): ( | ||
| storage: AccountStorageV3, | ||
| ) => ReturnType<typeof selectHybridAccountTraced> { | ||
| return (storage: AccountStorageV3) => { | ||
| const now = Date.now(); | ||
| const healthTracker = getHealthTracker(); | ||
| const tokenTracker = getTokenTracker(); | ||
| const accountsWithMetrics: AccountWithMetrics[] = storage.accounts.map( | ||
| (account, index) => { | ||
| const enabled = account?.enabled !== false; | ||
| const rateLimits = account?.rateLimitResetTimes ?? {}; | ||
| let rateLimited = false; | ||
| for (const value of Object.values(rateLimits)) { | ||
| if (typeof value === "number" && value > now) { | ||
| rateLimited = true; | ||
| break; | ||
| } | ||
| } | ||
| const coolingDown = | ||
| typeof account?.coolingDownUntil === "number" && | ||
| account.coolingDownUntil > now; | ||
| const isAvailable = enabled && !rateLimited && !coolingDown; | ||
| return { | ||
| index, | ||
| trackerKey: account?.accountId ?? index, | ||
| isAvailable, | ||
| lastUsed: account?.lastUsed ?? 0, | ||
| }; | ||
| }, | ||
| ); | ||
| return selectHybridAccountTraced({ | ||
| accounts: accountsWithMetrics, | ||
| healthTracker, | ||
| tokenTracker, | ||
| }); | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
isAvailable parity gap vs production
buildSelectAccountTraced() computes isAvailable from enabled, rateLimitResetTimes, and coolingDownUntil, but the production path in lib/accounts.ts (getCurrentOrNextForFamilyHybrid) also gates on hasEnabledWorkspaces(account) and isCircuitAvailable(account) (circuit-breaker state), and uses the family-specific isRateLimitedForFamily(account, family, model) rather than scanning all rate-limit keys. A circuit-open account shows as available: true in the diagnostic but would never be selected in production, making the "why-selected" output misleading exactly when it matters most (accounts under failure pressure).
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 3252-3289
Comment:
**`isAvailable` parity gap vs production**
`buildSelectAccountTraced()` computes `isAvailable` from `enabled`, `rateLimitResetTimes`, and `coolingDownUntil`, but the production path in `lib/accounts.ts` (`getCurrentOrNextForFamilyHybrid`) also gates on `hasEnabledWorkspaces(account)` and `isCircuitAvailable(account)` (circuit-breaker state), and uses the family-specific `isRateLimitedForFamily(account, family, model)` rather than scanning all rate-limit keys. A circuit-open account shows as `available: true` in the diagnostic but would never be selected in production, making the "why-selected" output misleading exactly when it matters most (accounts under failure pressure).
How can I resolve this? If you propose a fix, please make it concise.
| import { describe, expect, it, vi } from "vitest"; | ||
| import { | ||
| parseWhySelectedArgs, | ||
| printWhySelectedUsage, | ||
| runWhySelectedCommand, |
There was a problem hiding this comment.
buildSelectAccountTraced adapter parity
the parity test in rotation.test.ts verifies selectHybridAccountTraced agrees with selectHybridAccount given the same inputs, but there's no test that exercises buildSelectAccountTraced() in codex-manager.ts with a real AccountStorageV3 that has a circuit-breaker-open or workspace-disabled account. that's the gap where the diagnostic diverges from production. adding one integration-style test here (or in a new codex-manager-build-account-traced.test.ts) would catch the parity drift described in the inline comment on buildSelectAccountTraced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/codex-manager-why-selected-command.test.ts
Line: 1-5
Comment:
**Missing coverage: `buildSelectAccountTraced` adapter parity**
the parity test in `rotation.test.ts` verifies `selectHybridAccountTraced` agrees with `selectHybridAccount` given the same inputs, but there's no test that exercises `buildSelectAccountTraced()` in `codex-manager.ts` with a real `AccountStorageV3` that has a circuit-breaker-open or workspace-disabled account. that's the gap where the diagnostic diverges from production. adding one integration-style test here (or in a new `codex-manager-build-account-traced.test.ts`) would catch the parity drift described in the inline comment on `buildSelectAccountTraced`.
How can I resolve this? If you propose a fix, please make it concise.
Addresses oracle audit HIGH findings on PR 410: - verify --paths now resets storage state on entry (setStoragePath(null)) matching the convention used by doctor/check commands - sandbox-reject-escape probe no longer false-fails when cwd is an ancestor of the probe path (e.g. POSIX cwd=/) - Added regression test for cwd=/ edge case - Documented why-selected and verify commands in: - docs/reference/commands.md (full flag + JSON output reference) - README.md (Advanced and Repair sections)
1 participant
Implements PR-P from Phase 1 roadmap (docs/audits/MASTER_AUDIT.md §9 F1, F2).
Summary
Two new diagnostic CLI commands:
codex auth why-selected [--now|--last] [--json]— surfaces live per-candidate scoring breakdown of the hybrid account selection algorithmcodex auth verify [--paths|--flagged|--all] [--json]— self-tests the storage path resolution chain and the PR-AresolvePathsandbox guardDesign Decision: Trace Mode Over Persistent Tracker
No persistent selection tracker exists in the codebase. Rather than building a new ring buffer file,
why-selectedextendsselectHybridAccountwith a non-mutatingselectHybridAccountTracedsibling that mirrors the production scoring logic exactly (same weights, same availability gating, same PID offset, same capability boost) and emits the per-candidate breakdown. A parity test asserts both functions agree on the winning index.--lastconsults the existingloadPersistedRuntimeObservabilitySnapshotfor a generated-at timestamp but falls back to live recomputation; a user-visible note explains this in text mode.Verify --paths Coverage
Walks the full path chain:
process.cwd→findProjectRoot→resolveProjectStorageIdentityRoot→getProjectStorageKey→getProjectConfigDir→getProjectGlobalConfigDir.Sandbox probes:
sandbox-accept-home— home-dir path acceptedsandbox-accept-tmp— tmpdir path acceptedsandbox-reject-escape— out-of-sandbox path rejected (UNC on Windows,/etc/shadowon unix)Confirms the PR-A lookalike-prefix fix is live.
Backward Compatibility
selectHybridAccountsignature unchanged;selectHybridAccountTracedis a new non-mutating siblingverify-flaggedverb kept as alias (delegates torunRepairVerifyFlagged)--allruns both--pathsand--flaggedin sequenceTests
test/codex-manager-why-selected-command.test.tstest/codex-manager-verify-command.test.tstest/rotation.test.tstest/codex-manager-cli.test.tsnpm run typecheck,npm run lint,npm run build: cleanHands-on QA
Verified the shipping binary
codex-multi-auth auth why-selected --jsonandcodex-multi-auth auth verify --paths --jsonproduce structured JSON output matching the documented schema. Bad-flag handling returns clear error + usage print.Closes
Phase 1 §9 F1 (why-selected) and F2 (verify --paths).
See docs/audits/MASTER_AUDIT.md §9.
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
adds two diagnostic CLI commands:
why-selectedsurfaces per-candidate hybrid scoring via a non-mutatingselectHybridAccountTracedsibling, andverify --pathswalks the full storage path chain plus sandbox probes. thepickEscapeProbePathhardening correctly handles the pathologicalcwd=/edge case flagged in the earlier audit, and the parity test between the traced and production selectors is solid.Confidence Score: 5/5
safe to merge — all remaining findings are P2 style/documentation issues with no correctness impact
all three comments are P2: a missing --all entry in the per-command help, a dead resolveActiveIndex dep in the why-selected interface, and a fragile string match for sandbox rejection detection. none affect runtime behavior. prior P1 concerns from the previous thread are not addressed but were already noted; no new regressions introduced.
lib/codex-manager/commands/verify.ts (printVerifyUsage missing --all) and lib/codex-manager/commands/why-selected.ts (resolveActiveIndex unused in interface)
Important Files Changed
resolveActiveIndexdeclared in deps interface but never called — dead coupling--allmissing from printVerifyUsage; sandbox rejection identified by fragile error-message regexselectHybridAccountTracedas a non-mutating sibling to production selection; parity test added; no tracker side-effects confirmedwhy-selectedandverifycommands;buildSelectAccountTracedadapter correctly maps storage toAccountWithMetricsSequence Diagram
sequenceDiagram participant CLI as codex-multi-auth CLI participant CM as codex-manager.ts participant WS as why-selected.ts participant VF as verify.ts participant RT as rotation.ts participant SP as storage/paths.ts participant ST as storage.ts Note over CLI,ST: codex auth why-selected [--now|--last] [--json] CLI->>CM: runCodexMultiAuthCli(["auth","why-selected",...]) CM->>CM: buildSelectAccountTraced() → adapter fn CM->>WS: runWhySelectedCommand(args, deps) WS->>ST: loadAccounts() ST-->>WS: AccountStorageV3 WS->>RT: selectHybridAccountTraced({accounts, healthTracker, tokenTracker}) RT-->>WS: HybridSelectionTraceResult (candidates + scores, no mutation) WS-->>CLI: JSON or human-readable output Note over CLI,ST: codex auth verify --paths [--json] CLI->>CM: runCodexMultiAuthCli(["auth","verify","--paths"]) CM->>VF: runVerifyCommand(args, deps) VF->>SP: getCwd → findProjectRoot → resolveProjectStorageIdentityRoot SP-->>VF: identity root path VF->>SP: getProjectStorageKey → getProjectConfigDir → getProjectGlobalConfigDir SP-->>VF: storage key + config dirs VF->>SP: resolvePath(home probe) → accept VF->>SP: resolvePath(tmp probe) → accept VF->>SP: resolvePath(escape probe) → throw Access denied SP-->>VF: sandbox confirmed VF-->>CLI: VerifyPathsReport (steps + sandboxTests)Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(cli): harden verify --paths sandbox ..." | Re-trigger Greptile