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. |
📝 Walkthroughwalkthroughthis pr adds a read-only health summary to the auth login flow. the cli builds a health summary from persisted sync/rollback and doctor storage reads and passes it into the ui prompt, which may render it as a disabled "health summary" row before accounts. changes
sequence diagram(s)sequenceDiagram
autonumber
actor user
participant cli as "cli\n(lib/cli.ts)"
participant codex as "codex-manager\n(lib/codex-manager.ts)"
participant storage as "storage"
participant ui as "auth-menu\n(lib/ui/auth-menu.ts)"
user->>cli: start auth login
cli->>codex: buildLoginMenuHealthSummary(currentStorage)
codex->>storage: read backups / rollbacks / last sync / doctor state
storage-->>codex: returns results (or errors)
codex-->>cli: healthSummary (or undefined/fallback)
cli->>ui: promptLoginMode(..., healthSummary)
ui->>user: render menu (includes disabled health row if provided)
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes suggested labels
notes for reviewer
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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}
![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/ui/auth-menu.ts`:
- Around line 614-637: Add a focused test that passes adversarial control/ANSI
characters into the healthSummary prop and asserts sanitizeTerminalText strips
or neutralizes them before rendering: call the code path that constructs
healthSummaryLabel/healthSummaryHint (which use sanitizeTerminalText) and render
the menu, then assert the output for the disabled health-summary row (label from
UI_COPY.mainMenu.healthSummary and the disabled row created after
healthSummaryLabel) contains no raw control sequences (e.g., "\x1b[31m", "\n",
"\x00") and only the sanitized text; target the existing spec around the health
summary rendering and ensure the test fails if sanitizeTerminalText does not
remove/escape those characters.
In `@test/codex-manager-cli.test.ts`:
- Line 788: Remove the duplicate mock default by deleting the later call to
getLastCodexCliSyncRunMock.mockReturnValue(null); so the test relies on the
existing default already established earlier (keep the original mock setup and
eliminate this redundant reassignment to avoid fixture noise).
- Around line 4450-4507: The test currently only asserts pool count and rollback
hint in the health summary; add assertions to also verify the sync and restore
sections so regressions in those slices fail. After invoking
runCodexMultiAuthCli and before finishing the test, extend the
promptLoginModeMock expectation to check the passed healthSummary includes
sync-related fields (from getLastCodexCliSyncRunMock) and restore-related fields
(from getActionableNamedBackupRestoresMock /
getLatestCodexCliSyncRollbackPlanMock) — e.g. use expect.objectContaining for
healthSummary.sync (or label containing "Sync" and counts/added/updated details)
and healthSummary.restore (or label containing "Restore"/"Backups" and
totalBackups/assessments info) when verifying promptLoginModeMock was called.
Ensure you reference promptLoginModeMock and the mocked fixtures
getActionableNamedBackupRestoresMock, getLastCodexCliSyncRunMock, and
getLatestCodexCliSyncRollbackPlanMock so the assertions map to the mocked
values.
🪄 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: 10f40d66-1cbf-43d4-ad9f-11a4f2edf758
📒 Files selected for processing (5)
lib/cli.tslib/codex-manager.tslib/ui/auth-menu.tslib/ui/copy.tstest/codex-manager-cli.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)
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/cli.tslib/ui/copy.tslib/ui/auth-menu.tslib/codex-manager.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.ts
🔇 Additional comments (8)
lib/ui/copy.ts (1)
12-12: looks good for the new main menu copy key.
lib/ui/copy.ts:12(line [12]) cleanly adds the label and keeps the copy structure consistent. no windows io, concurrency, or sensitive logging surface is introduced here.lib/cli.ts (1)
7-11: health summary plumbing is clean and backward-compatible.
lib/cli.ts:98(line [98]) keepshealthSummaryoptional, andlib/cli.ts:273-276forwards it without mutating state. this is a safe pass-through with no added windows fs edge, queue behavior, or concurrency risk.Also applies to: 96-99, 273-276
lib/ui/auth-menu.ts (1)
52-60: typed api shape for read-only health rows is solid.
lib/ui/auth-menu.ts:52-60(line [52] and line [59]) introduces a clear, narrow contract and keeps the option optional for compatibility.test/codex-manager-cli.test.ts (3)
3629-3631: good contract pin for omitted health summary.
test/codex-manager-cli.test.ts:3629-3631andtest/codex-manager-cli.test.ts:4580-4584lock the new second-arg contract tohealthSummary: undefinedinstead of letting the option bag disappear silently.Also applies to: 4580-4584
3977-3977: good i/o guard rails around the health-summary reads.
test/codex-manager-cli.test.ts:3977,test/codex-manager-cli.test.ts:4271, andtest/codex-manager-cli.test.ts:4575-4578pin the single backup scan and the skipped rollback/sync reads. that is the right regression surface for this login flow.Also applies to: 4271-4271, 4575-4578
4510-4560: good windows redaction and ui-row coverage.
test/codex-manager-cli.test.ts:4510-4560protects the warning sanitization path for windows-style rollback locations, andtest/codex-manager-cli.test.ts:4587-4627pins the summary row as disabled directly under the heading.Also applies to: 4587-4627
lib/codex-manager.ts (2)
3411-3417: good privacy boundary in the read-only doctor path.the summary stays read-only and only uses a short hash for duplicate-token detection, so the login flow never needs to surface raw refresh tokens or mutate storage just to render health. lib/codex-manager.ts:3411-3417, lib/codex-manager.ts:4007-4094. As per coding guidelines, check for logging that leaks tokens or emails.
Also applies to: 4007-4094
4355-4363: the prompt wiring is nicely isolated.keeping
healthSummaryoptional here preserves the existing login menu behavior when summary generation returnsnull. lib/codex-manager.ts:4355-4363.
| function formatLoginMenuRollbackHint( | ||
| rollbackPlan: Awaited<ReturnType<typeof getLatestCodexCliSyncRollbackPlan>>, | ||
| rollbackPlanLoadFailed = false, | ||
| ): string { | ||
| if (rollbackPlanLoadFailed) { | ||
| return "rollback state unavailable"; | ||
| } | ||
| if (rollbackPlan.status === "ready") { | ||
| const accountCount = rollbackPlan.accountCount; | ||
| if ( | ||
| typeof accountCount === "number" && | ||
| Number.isFinite(accountCount) && | ||
| accountCount >= 0 | ||
| ) { | ||
| return `checkpoint ready for ${Math.trunc(accountCount)} account(s)`; | ||
| } | ||
| return "checkpoint ready"; | ||
| } | ||
| return rollbackPlan.snapshot ? "checkpoint unavailable" : "no rollback checkpoint available"; | ||
| } | ||
|
|
||
| async function buildLoginMenuHealthSummary( | ||
| storage: AccountStorageV3, | ||
| ): Promise<DashboardHealthSummary | null> { | ||
| if (storage.accounts.length === 0) { | ||
| return null; | ||
| } | ||
| const enabledCount = storage.accounts.filter( | ||
| (account) => account.enabled !== false, | ||
| ).length; | ||
| const disabledCount = storage.accounts.length - enabledCount; | ||
| let actionableRestores: Awaited< | ||
| ReturnType<typeof getActionableNamedBackupRestores> | ||
| > = { | ||
| assessments: [], | ||
| allAssessments: [], | ||
| totalBackups: 0, | ||
| }; | ||
| let rollbackPlan: Awaited< | ||
| ReturnType<typeof getLatestCodexCliSyncRollbackPlan> | ||
| > = { | ||
| status: "unavailable", | ||
| reason: "rollback state unavailable", | ||
| snapshot: null, | ||
| }; | ||
| let rollbackPlanLoadFailed = false; | ||
| let syncSummary = { | ||
| label: "unknown", | ||
| hint: "Sync state unavailable.", | ||
| }; | ||
| let doctorSummary: DoctorReadOnlySummary = { | ||
| severity: "warn", | ||
| label: "unknown", | ||
| hint: "Doctor state unavailable.", | ||
| }; | ||
| try { | ||
| actionableRestores = await getActionableNamedBackupRestores({ | ||
| currentStorage: storage, | ||
| }); | ||
| } catch (error) { | ||
| logLoginMenuHealthSummaryWarning( | ||
| "Failed to load login menu restore health summary state", | ||
| error, | ||
| ); | ||
| } | ||
| try { | ||
| rollbackPlan = await getLatestCodexCliSyncRollbackPlan(); | ||
| } catch (error) { | ||
| rollbackPlanLoadFailed = true; | ||
| logLoginMenuHealthSummaryWarning( | ||
| "Failed to load login menu rollback health summary state", | ||
| error, | ||
| ); | ||
| } | ||
| try { | ||
| syncSummary = summarizeLatestCodexCliSyncState(); | ||
| } catch (error) { | ||
| logLoginMenuHealthSummaryWarning( | ||
| "Failed to summarize login menu sync state", | ||
| error, | ||
| ); | ||
| } | ||
| try { | ||
| doctorSummary = summarizeReadOnlyDoctorState(storage); | ||
| } catch (error) { | ||
| logLoginMenuHealthSummaryWarning( | ||
| "Failed to summarize login menu doctor state", | ||
| error, | ||
| ); | ||
| } | ||
| const restoreLabel = | ||
| actionableRestores.totalBackups > 0 | ||
| ? `${actionableRestores.assessments.length}/${actionableRestores.totalBackups} ready` | ||
| : "none"; | ||
| const rollbackLabel = rollbackPlan.status === "ready" ? "ready" : "none"; | ||
| const accountLabel = | ||
| disabledCount > 0 | ||
| ? `${enabledCount}/${storage.accounts.length} enabled` | ||
| : `${storage.accounts.length} active`; | ||
| const hintParts = [ | ||
| `Accounts: ${enabledCount} enabled / ${disabledCount} disabled / ${storage.accounts.length} total`, | ||
| `Sync: ${syncSummary.hint}`, | ||
| `Restore backups: ${actionableRestores.assessments.length} actionable of ${actionableRestores.totalBackups} total`, | ||
| `Rollback: ${formatLoginMenuRollbackHint(rollbackPlan, rollbackPlanLoadFailed)}`, | ||
| `Doctor: ${doctorSummary.hint}`, | ||
| ]; | ||
|
|
||
| return { | ||
| label: | ||
| `Pool ${accountLabel} | Sync ${syncSummary.label} | Restore ${restoreLabel} | ` + | ||
| `Rollback ${rollbackLabel} | Doctor ${doctorSummary.label}`, | ||
| hint: hintParts.join(" | "), |
There was a problem hiding this comment.
don't collapse unreadable restore/rollback state into none.
getLatestCodexCliSyncRollbackPlan() already returns status: "unavailable" with a reason when rollback history cannot be read, but this path still renders rollback none. the restore catch path does the same thing by falling back to zero counts. on windows ebusy/eacces, the health row will tell users there is nothing to restore or roll back when the state is actually unknown. please carry an explicit unavailable state through the label and hint, and add a vitest regression for the unreadable-filesystem branch. lib/codex-manager.ts:4104-4215, lib/codex-cli/sync.ts:567-587. As per coding guidelines, focus on auth rotation, windows filesystem io, and concurrency.
possible fix
function formatLoginMenuRollbackHint(
rollbackPlan: Awaited<ReturnType<typeof getLatestCodexCliSyncRollbackPlan>>,
rollbackPlanLoadFailed = false,
): string {
if (rollbackPlanLoadFailed) {
return "rollback state unavailable";
}
if (rollbackPlan.status === "ready") {
const accountCount = rollbackPlan.accountCount;
if (
typeof accountCount === "number" &&
Number.isFinite(accountCount) &&
accountCount >= 0
) {
return `checkpoint ready for ${Math.trunc(accountCount)} account(s)`;
}
return "checkpoint ready";
}
- return rollbackPlan.snapshot ? "checkpoint unavailable" : "no rollback checkpoint available";
+ const reason = collapseWhitespace(rollbackPlan.reason);
+ if (reason.toLowerCase().includes("unavailable")) {
+ return reason;
+ }
+ return rollbackPlan.snapshot ? "checkpoint unavailable" : "no rollback checkpoint available";
}
async function buildLoginMenuHealthSummary(
storage: AccountStorageV3,
): Promise<DashboardHealthSummary | null> {
@@
let rollbackPlanLoadFailed = false;
+ let restoreStateUnavailable = false;
@@
try {
actionableRestores = await getActionableNamedBackupRestores({
currentStorage: storage,
});
} catch (error) {
+ restoreStateUnavailable = true;
logLoginMenuHealthSummaryWarning(
- "Failed to load login menu restore health summary state",
+ "Failed to load login menu restore health summary state",
error,
);
}
@@
+ const rollbackHint = formatLoginMenuRollbackHint(
+ rollbackPlan,
+ rollbackPlanLoadFailed,
+ );
const restoreLabel =
- actionableRestores.totalBackups > 0
+ restoreStateUnavailable
+ ? "unavailable"
+ : actionableRestores.totalBackups > 0
? `${actionableRestores.assessments.length}/${actionableRestores.totalBackups} ready`
: "none";
- const rollbackLabel = rollbackPlan.status === "ready" ? "ready" : "none";
+ const rollbackLabel =
+ rollbackPlan.status === "ready"
+ ? "ready"
+ : rollbackHint.toLowerCase().includes("unavailable")
+ ? "unavailable"
+ : "none";
@@
- `Rollback: ${formatLoginMenuRollbackHint(rollbackPlan, rollbackPlanLoadFailed)}`,
+ `Rollback: ${rollbackHint}`,
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.ts`:
- Around line 4096-4102: Replace the raw console.warn in
logLoginMenuHealthSummaryWarning with the module's structured logger (the
createLogger instance used elsewhere, e.g., log.warn) to keep observability
consistent: call log.warn and pass the same message built from context and
getRedactedFilesystemErrorLabel(error) and include the original error (or safe
metadata) as structured metadata so the warning is emitted through the central
logger instead of console.warn.
- Around line 3960-4005: The test suite lacks coverage for
summarizeLatestCodexCliSyncState and the file-read path doesn't retry on Windows
EBUSY errors; add a Vitest unit test exercising summarizeLatestCodexCliSyncState
(covering null latest run, various outcome labels, counts, and message trimming)
and modify readLatestSyncHistorySync to retry on EBUSY similar to the existing
retryable logic (use the same retry/backoff strategy and reuse the
RETRYABLE_REMOVE_CODES pattern), ensuring
getLastCodexCliSyncRun/summarizeLatestCodexCliSyncState callpaths are exercised
and the in-memory cache behavior remains unchanged.
In `@test/codex-manager-cli.test.ts`:
- Around line 4496-4505: The test only asserts the aggregated "Doctor 3
warnings" label; update the test that uses promptLoginModeMock to also assert
the doctor-specific detail inside healthSummary.hint by adding a fixture that
shares a refresh token (simulate the duplicate refresh-token fingerprint branch)
and then expect.objectContaining({ healthSummary: expect.objectContaining({
hint: expect.stringMatching(/Doctor: .*duplicate refresh
token|doctor.*refresh-token/iu) }) }) to ensure the hint contains the
doctor-specific message; locate the assertion around promptLoginModeMock in
test/codex-manager-cli.test.ts and modify the fixture setup to make one account
share a refresh token, keep vitest determinism, and avoid mocking real secrets.
- Around line 4525-4547: The mocks throw plain Error instances so the sanitizer
never sees errno-style errors and the Windows path redaction branch isn't
exercised; update the mocked failures (getActionableNamedBackupRestoresMock,
getLatestCodexCliSyncRollbackPlanMock, getLastCodexCliSyncRunMock) to produce
errno-style errors (i.e., Error objects with a code property set to "EBUSY" or
use a NodeJS.ErrnoException shape) and include the Windows path in the error
message (e.g., "EBUSY rollback at C:\\sensitive\\rollback.json") so
runCodexMultiAuthCli's warning sanitizer is forced to strip the path but keep
the errno code when invoked by the test that imports runCodexMultiAuthCli and
calls ["auth","login"] with promptLoginModeMock resolving to cancel. Ensure
mocks use vitest mockResolvedValue/mockRejectedValue or mockImplementation
consistently so the assertions about warningOutput containing the [UNKNOWN]
alternative and not containing the raw path/EBUSY code remain valid.
🪄 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: cff20fb2-d6ee-408a-b7ab-1cd6c92cf26b
📒 Files selected for processing (2)
lib/codex-manager.tstest/codex-manager-cli.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-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/codex-manager.ts
🔇 Additional comments (8)
lib/codex-manager.ts (8)
4125-4236:buildLoginMenuHealthSummaryaddresses past review, but verify regression test.the implementation now correctly:
- uses
Promise.allSettledatlib/codex-manager.ts:4160-4165for parallel i/o- tracks
restoreStateUnavailableandrollbackPlanLoadFailedflags- surfaces "unavailable" labels instead of "none" when filesystem reads fail (
lib/codex-manager.ts:4204-4215)this addresses the past review comment. however, the past review also requested "add a vitest regression for the unreadable-filesystem branch"—please confirm this test exists in
test/codex-manager-cli.test.ts. the pr objectives mention "error fallback" tests, but verify they cover the ebusy/eacces path specifically. as per coding guidelines, verify every change cites affected tests (vitest) and that new queues handle ebusy/429 scenarios.#!/bin/bash # verify test coverage for unavailable/error fallback states rg -n -C3 'unavailable|restoreStateUnavailable|rollbackPlanLoadFailed|EBUSY|EACCES' test/codex-manager-cli.test.ts
1-1: lgtm on the crypto import.using
node:cryptofor sha256 fingerprinting is the right call here—avoids rolling your own hash.
3358-3367: types look clean.
DashboardHealthSummaryandDoctorReadOnlySummaryare well-scoped for the health-summary slice.
3411-3417: sha256 fingerprinting is appropriate here.12 hex chars (48 bits) is plenty for local duplicate detection. critically, this avoids logging or exposing raw refresh tokens in the health summary. as per coding guidelines, focus on auth rotation, windows filesystem io, and concurrency—no token leakage detected.
4007-4094: read-only doctor checks look solid.
lib/codex-manager.ts:4046-4055uses fingerprinted tokens for duplicate detection, avoiding raw token exposure. the severity escalation logic (ok → warn → error) is correct.one note: the pr summary mentions greptile flagged "uncovered branches in new summarize helpers"—confirm test coverage for the error/warn paths in
test/codex-manager-cli.test.ts.
4104-4123: rollback hint handles unavailable state correctly.
lib/codex-manager.ts:4108-4110now returns"rollback state unavailable"whenrollbackPlanLoadFailedis true, rather than collapsing to "none". this addresses the concern from the past review about windows ebusy/eacces scenarios.
3375-3384: integration wiring looks correct.
lib/codex-manager.ts:3375awaitsbuildLoginMenuHealthSummaryand passes it topromptLoginModeat line 3381. the?? undefinedcoercion ensures type compatibility with the optional parameter.
3952-3958: simple helper, no issues.
formatCountNounis a straightforward pluralization utility. works as expected.
| function summarizeLatestCodexCliSyncState(): { | ||
| label: string; | ||
| hint: string; | ||
| } { | ||
| const latestRun = getLastCodexCliSyncRun(); | ||
| if (!latestRun) { | ||
| return { | ||
| label: "none", | ||
| hint: "No Codex CLI sync result recorded yet.", | ||
| }; | ||
| } | ||
|
|
||
| const outcomeLabel = | ||
| latestRun.outcome === "changed" | ||
| ? "changed" | ||
| : latestRun.outcome === "noop" | ||
| ? "noop" | ||
| : latestRun.outcome === "disabled" | ||
| ? "disabled" | ||
| : latestRun.outcome === "unavailable" | ||
| ? "unavailable" | ||
| : "error"; | ||
| const when = formatRelativeDateShort(latestRun.runAt); | ||
| const counts = [ | ||
| latestRun.summary.addedAccountCount > 0 | ||
| ? `add ${latestRun.summary.addedAccountCount}` | ||
| : null, | ||
| latestRun.summary.updatedAccountCount > 0 | ||
| ? `update ${latestRun.summary.updatedAccountCount}` | ||
| : null, | ||
| latestRun.summary.destinationOnlyPreservedCount > 0 | ||
| ? `preserve ${latestRun.summary.destinationOnlyPreservedCount}` | ||
| : null, | ||
| latestRun.summary.selectionChanged ? "selection changed" : null, | ||
| ].filter((value): value is string => value !== null); | ||
| const hintParts = [ | ||
| `Latest sync ${outcomeLabel}${when ? ` ${when}` : ""}`, | ||
| counts.length > 0 ? counts.join(" | ") : "no account deltas recorded", | ||
| latestRun.message?.trim() || undefined, | ||
| ].filter((value): value is string => Boolean(value)); | ||
|
|
||
| return { | ||
| label: `${outcomeLabel}${when ? ` ${when}` : ""}`, | ||
| hint: hintParts.join(" | "), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if getLastCodexCliSyncRun does synchronous file I/O
ast-grep --pattern 'function getLastCodexCliSyncRun($$$) {
$$$
}'
# also check for readFileSync or other sync patterns
rg -n 'getLastCodexCliSyncRun|readFileSync' --type=ts lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Get implementation of getLastCodexCliSyncRun
sed -n '320,360p' lib/codex-cli/sync.ts
# Check what data structure stores the sync run and how it's populated
rg -n 'latestCodexCliSyncRun|lastCodexCliSyncRun' lib/codex-cli/sync.ts -A 2 -B 2
# Look for readFileSync or other blocking file operations in the file
rg -n 'readFileSync|readSync|fs\.' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 2919
🏁 Script executed:
#!/bin/bash
# Find readLatestSyncHistorySync implementation
rg -n 'readLatestSyncHistorySync' lib/codex-cli/sync.ts -A 15
# Also check where it's called from
rg -n 'readLatestSyncHistorySync' lib/Repository: ndycode/codex-multi-auth
Length of output: 1497
🏁 Script executed:
#!/bin/bash
# Get implementation of readLatestSyncHistorySync
sed -n '259,290p' lib/sync-history.ts
# Also check for any readFileSync calls in sync-history.ts
rg -n 'readFileSync|readSync|fs\.read' lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 1473
🏁 Script executed:
#!/bin/bash
# Check for tests covering getLastCodexCliSyncRun
rg -n 'getLastCodexCliSyncRun' test/
# Check for tests of readLatestSyncHistorySync
rg -n 'readLatestSyncHistorySync' test/
# Check error handling for windows file scenarios
sed -n '259,280p' lib/sync-history.ts
# Look at the coding guidelines context - check for EBUSY handling in sync code
rg -n 'EBUSY|EAGAIN' lib/codex-cli/sync.ts lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 3215
🏁 Script executed:
#!/bin/bash
# Check if this function is new in the change or existing
git log --oneline -1 lib/codex-manager.ts
# Check the context around line 3960-4005 in git to see if this is new code
git show --stat lib/codex-manager.ts | head -20
# Look for any tests added for summarizeLatestCodexCliSyncState specifically
rg -n 'summarizeLatestCodexCliSyncState' test/Repository: ndycode/codex-multi-auth
Length of output: 457
🏁 Script executed:
#!/bin/bash
# Check if summarizeLatestCodexCliSyncState has test coverage
rg -n 'summarizeLatestCodexCliSyncState' test/
# Check if the function is directly tested or only used in integration tests
rg -B5 -A10 'summarizeLatestCodexCliSyncState' lib/codex-manager.ts | head -40
# Look for windows-specific file locking tests in sync-history
rg -n 'windows|EBUSY' test/sync-history.test.tsRepository: ndycode/codex-multi-auth
Length of output: 808
add test coverage for summarizeLatestCodexCliSyncState and retry on file lock errors.
blocking file i/o confirmed: summarizeLatestCodexCliSyncState() calls getLastCodexCliSyncRun() which calls readLatestSyncHistorySync() at lib/sync-history.ts:261 using readFileSync(). in-memory cache at lib/codex-cli/sync.ts:153 prevents repeated blocks after first call, and fire-and-forget async load at lib/codex-cli/sync.ts:330 refills it. however, no regression tests exist for summarizeLatestCodexCliSyncState() itself, and readLatestSyncHistorySync() doesn't retry on EBUSY despite windows file lock risks—compare to RETRYABLE_REMOVE_CODES at lib/sync-history.ts:13. add vitest coverage for this function and implement EBUSY retry logic in readLatestSyncHistorySync().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 3960 - 4005, The test suite lacks coverage
for summarizeLatestCodexCliSyncState and the file-read path doesn't retry on
Windows EBUSY errors; add a Vitest unit test exercising
summarizeLatestCodexCliSyncState (covering null latest run, various outcome
labels, counts, and message trimming) and modify readLatestSyncHistorySync to
retry on EBUSY similar to the existing retryable logic (use the same
retry/backoff strategy and reuse the RETRYABLE_REMOVE_CODES pattern), ensuring
getLastCodexCliSyncRun/summarizeLatestCodexCliSyncState callpaths are exercised
and the in-memory cache behavior remains unchanged.
1 participant
Summary
#101devfrom fresh branchclean/pr101-dashboard-health-summaryWhat Changed
test/codex-manager-cli.test.tsdevbehavior while resolving the drift that made the old PR hard to review safelyValidation
npx vitest run test/index.test.ts test/index-retry.test.ts test/codex-manager-cli.test.ts test/cli-auth-menu.test.ts test/storage.test.ts test/paths.test.ts test/codex-cli-sync.test.ts test/live-account-sync.test.ts test/documentation.test.tsnpm run lintnpm run typechecknpm testnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
9d2bf18Additional Notes
#101.5files instead of the old43-file PR.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 adds a read-only health-summary row to the dashboard login menu, surfacing pool size, sync state, restore readiness, rollback status, and a lightweight doctor scan on every menu repaint. the implementation is well-scoped: io is parallelized via
Promise.allSettledandPromise.all, errors are isolated and redacted before logging (important on windows where paths likeC:\sensitive\rollback.jsoncan appear in errno messages), and asanitizeTerminalTextpass protects the tui from ansi/control-character injection.key changes:
buildLoginMenuHealthSummaryassembles the five-part label/hint from parallel async reads with per-component fallbackssummarizeReadOnlyDoctorStateadds a read-only pass over token fingerprints (sha-256, 12-char hex) to count duplicates without exposing raw tokensreadLatestSyncHistorySyncgains a 3-attempt retry loop usingAtomics.wait-basedsleepSyncfor transient windows file locksshowAuthMenurenders the summary as a disabled, sanitized row under a new "Health Summary" headingtwo items to address before merge:
EACCESshould be removed fromRETRYABLE_LATEST_READ_CODES— it is a permanent permission denial on windows, not a transient lock, so retrying wastes 70 ms of main-thread blocking on every menu iteration for affected machinessleepSync/Atomics.waitmain-thread block is acceptable for a cli sync function but should be noted — persistent locks will add up to 70 ms of event-loop freeze per menu keystrokeConfidence Score: 3/5
Important Files Changed
Atomics.waitsleep for EBUSY/EPERM/EACCES onreadLatestSyncHistorySync; EACCES should be removed from the retryable set as it is not transient on windows, and the main-thread blocking warrants explicit documentationbuildLoginMenuHealthSummarywith parallelized I/O viaPromise.allSettled, SHA-256 fingerprinting for read-only duplicate-token detection, redacted filesystem error logging, and parallelPromise.allin the menu loop — solid implementation with comprehensive fallback handlingAuthMenuReadOnlyRowinterface and conditionally renders a sanitized disabled health-summary row with heading separator inshowAuthMenu; control-character sanitization viasanitizeTerminalTextis correctly applied to both label and hintgetActionableNamedBackupRestoresis missing theallAssessmentsfield in one testvi.doMock("node:fs")test that verifies EBUSY retry succeeds on second attempt; correct use ofvi.resetModulesin finally block to avoid test pollutionSequence Diagram
sequenceDiagram participant RL as runAuthLogin (while loop) participant FA as loadFlaggedAccounts() participant BHS as buildLoginMenuHealthSummary() participant GANB as getActionableNamedBackupRestores() participant GLRP as getLatestCodexCliSyncRollbackPlan() participant SLCS as summarizeLatestCodexCliSyncState() participant RLSH as readLatestSyncHistorySync() participant SRDS as summarizeReadOnlyDoctorState() participant PM as promptLoginMode → showAuthMenu RL->>+FA: Promise.all([...]) RL->>+BHS: Promise.all([...]) BHS->>+GANB: Promise.allSettled([...]) BHS->>+GLRP: Promise.allSettled([...]) GANB-->>-BHS: restoresResult (fulfilled | rejected) GLRP-->>-BHS: rollbackResult (fulfilled | rejected) BHS->>+SLCS: try/catch SLCS->>+RLSH: readLatestSyncHistorySync() note over RLSH: retry ×3 on EBUSY/EPERM<br/>sleepSync(10→20→40 ms)<br/>blocks event loop RLSH-->>-SLCS: SyncHistoryEntry | null SLCS-->>-BHS: {label, hint} BHS->>+SRDS: try/catch note over SRDS: SHA-256 fingerprint<br/>duplicate-token check SRDS-->>-BHS: DoctorReadOnlySummary BHS-->>-RL: DashboardHealthSummary | null FA-->>-RL: flaggedStorage RL->>+PM: promptLoginMode({healthSummary, flaggedCount}) PM-->>-RL: MenuResultComments Outside Diff (1)
lib/sync-history.ts, line 9 (link)EACCESis not transient on Windows — retrying it wastes 70 msEBUSYandEPERMcan represent transient exclusive locks on Windows (antivirus, indexer, another process mid-write), which is whyRETRYABLE_REMOVE_CODESincludes them.EACCESis different: it signals a permanent permission denial (Access is denied.). Unlike the write-path set (RETRYABLE_REMOVE_CODES) which deliberately omitsEACCES, the new read-path set includes it. On any machine where the sync-history file has incorrect ACLs the retry loop will burn the full10 + 20 + 40 = 70 msof synchronous main-thread blocking on every health-summary build — and every menu repaint inside thewhile (true)loop.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "fix(login-menu): pre..."