feat(ui): add health summary dashboard#81
feat(ui): add health summary dashboard#81ndycode wants to merge 4 commits intogit-plan/16-retention-doctorfrom
Conversation
📝 WalkthroughWalkthroughadds health summary display to the login menu by computing health/status data internally and rendering it as read-only rows in the auth menu ui. new type definitions, utilities for health computation, and integration across cli and ui layers. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Layer
participant CM as Codex Manager
participant HS as Health Summary<br/>Computation
participant UI as Auth Menu UI
participant User as User
CLI->>CM: promptLoginMode() called
CM->>HS: buildLoginMenuHealthSummary()
HS->>HS: getLastCodexCliSyncRun()
HS->>HS: summarizeLatestCodexCliSyncState()
HS-->>CM: returns AuthMenuReadOnlyRow
CM->>UI: showAuthMenu(options<br/>with healthSummary)
UI->>UI: render health summary<br/>as disabled row
UI->>User: display login menu<br/>with health info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes specific flags to review:
🚥 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 unit tests (beta)
📝 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}
| 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)); |
There was a problem hiding this comment.
latestRun.message in hint without token redaction
CodexCliSyncRun.message is a free-form string | undefined. if an error path (e.g. the catch block in syncAccountStorageFromCodexCli) ever serialises an exception that embeds a refresh-token or auth credential in its message, that raw string flows into hintParts and ends up visible on-screen.
sanitizeTerminalText in auth-menu.ts strips ANSI codes and control characters but does not redact token-shaped values (e.g. strings matching ey…, rt-…, bearer header fragments, or filesystem paths that include account IDs).
at minimum, drop latestRun.message from the terminal-visible hint, or apply a dedicated token-redaction pass before surfacing it. a vitest case that sets message to a mock token string and asserts it doesn't appear in the final label/hint would close this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4254-4258
Comment:
**`latestRun.message` in hint without token redaction**
`CodexCliSyncRun.message` is a free-form `string | undefined`. if an error path (e.g. the `catch` block in `syncAccountStorageFromCodexCli`) ever serialises an exception that embeds a refresh-token or auth credential in its message, that raw string flows into `hintParts` and ends up visible on-screen.
`sanitizeTerminalText` in `auth-menu.ts` strips ANSI codes and control characters but does **not** redact token-shaped values (e.g. strings matching `ey…`, `rt-…`, bearer header fragments, or filesystem paths that include account IDs).
at minimum, drop `latestRun.message` from the terminal-visible hint, or apply a dedicated token-redaction pass before surfacing it. a vitest case that sets `message` to a mock token string and asserts it doesn't appear in the final `label`/`hint` would close this gap.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 4372-4373: The string interpolation is rendering "undefined" when
rollbackPlan.reason is missing; update the code that constructs the UI strings
(the entries containing `Rollback: ${rollbackPlan.reason}` and `Doctor:
${doctorSummary.hint}`) to guard against undefined by either only including the
"Rollback" line when rollbackPlan.reason is present or substituting a safe
fallback (e.g., empty string or "N/A") using a null/undefined check for
rollbackPlan.reason; ensure the same defensive check is applied to
doctorSummary.hint where appropriate so no "undefined" appears in the UI.
- Around line 4892-4900: Wrap the Promise.all call inside
buildLoginMenuHealthSummary with a try-catch that returns a safe fallback health
summary (e.g., { status: "unknown", details: [] } or similar shape used by the
caller) when getActionableNamedBackupRestores or
getLatestCodexCliSyncRollbackPlan reject, and ensure any thrown errors are
logged but not rethrown so promptLoginMode (called at the await site) still
renders; also ensure pendingMenuQuotaRefresh continues running in background (do
not await it) so it cannot block the prompt. Add vitest cases that stub
getActionableNamedBackupRestores to reject with EBUSY/EACCES/ENOENT, stub
getLatestCodexCliSyncRollbackPlan to reject, simulate concurrent
pendingMenuQuotaRefresh callbacks while buildLoginMenuHealthSummary is awaiting,
and assert the menu renders a fallback/unknown health summary and that stale
quota status is shown if the health summary throws.
- Around line 4255-4258: The health-hint assembly is appending latestRun.message
verbatim into hintParts (see hintParts construction) which can leak secrets;
before pushing latestRun.message into hintParts, import and apply the existing
redaction utilities (maskEmail and redactSensitiveReason from lib/forecast.ts or
the module that exposes them) and use the same redaction flow as
summarizeRefreshFailure (e.g., trim the message, pass it through
redactSensitiveReason and maskEmail as appropriate) so the string appended is
sanitized; update the hintParts expression to use the redacted string and add a
regression test that constructs a latestRun.message containing
emails/tokens/paths and asserts the surfaced hint is sanitized.
In `@test/codex-manager-cli.test.ts`:
- Around line 1662-1747: The test is currently not asserting the sync-state
contract so regressions in sync wiring (sourced from getLastCodexCliSyncRun and
surfaced by buildLoginMenuHealthSummary) can slip by; update the test (around
the runCodexMultiAuthCli call in this spec) to explicitly verify
getLastCodexCliSyncRun was invoked and that the healthSummary includes the full
sync fields (e.g., summary.trigger, summary.outcome or the snapshot.name and
reason) by adding expectations that promptLoginModeMock was called with a
healthSummary containing the sync properties returned by getLastCodexCliSyncRun
and add a spy/assertion that getLastCodexCliSyncRunMock was called exactly once
to lock the contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8629f240-da3d-4c5c-911e-d1fb9f9c07b6
📒 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). (2)
- GitHub Check: cubic · AI code reviewer
- 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/ui/copy.tslib/ui/auth-menu.tslib/cli.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 (7)
lib/cli.ts (1)
96-100: health summary option threading looks correct.the optional type + passthrough is clean and keeps existing behavior intact in
lib/cli.ts:98andlib/cli.ts:259.Also applies to: 257-260
lib/ui/copy.ts (1)
12-12: copy key addition is good.
UI_COPY.mainMenu.healthSummaryinlib/ui/copy.ts:12is clear and matches the new ui section.lib/ui/auth-menu.ts (1)
53-61: health summary rendering is clean and safe.the new read-only row type and conditional block in
lib/ui/auth-menu.ts:53andlib/ui/auth-menu.ts:612are straightforward, and terminal sanitization is correctly applied before rendering.Also applies to: 612-635
test/codex-manager-cli.test.ts (4)
32-33: mock lifecycle for latest sync run is wired correctly.good addition of declaration, module export, reset, and default return for
getLastCodexCliSyncRunMock. this keeps tests isolated and deterministic for the sync summary path inlib/codex-manager.ts:4219.Also applies to: 132-133, 293-294, 406-407
647-647: updated restore-call expectation matches the new health-summary flow.
toHaveBeenCalledTimes(1)is consistent with the health summary builder invoking restore assessment inlib/codex-manager.ts:4354, while still keeping startup restore behavior constrained intest/codex-manager-cli.test.ts:605.
1749-1789: ui regression coverage for read-only health row looks solid.this verifies both the “health summary” heading and the following disabled row payload, which is the right contract boundary for
showAuthMenurendering intest/codex-manager-cli.test.ts:1751.
2088-2089: checkpoint key rename assertion is correct.asserting
codex-cli-rollback-checkpointhere keeps doctor json checks aligned with the renamed key contract intest/codex-manager-cli.test.ts:2053.
| `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)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=ts "latestRun\\.message|getLastCodexCliSyncRun\\(|message\\s*:" lib/codex-manager.ts lib/codex-cli/sync.ts
rg -n --type=ts "redact|sanitizeEmail|mask|token" lib/codex-manager.ts lib/codex-cli/sync.ts libRepository: ndycode/codex-multi-auth
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context around line 4257
sed -n '4250,4265p' lib/codex-manager.ts
# Find CodexCliSyncRun type definition
rg -n "type CodexCliSyncRun|interface CodexCliSyncRun" lib/Repository: ndycode/codex-multi-auth
Length of output: 739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the CodexCliSyncRun interface definition
sed -n '123,180p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 1483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where message is set in CodexCliSyncRun or lastCodexCliSyncRun
rg -n "message\s*:" lib/codex-cli/sync.ts | head -20
# Search for where message is assigned/created in sync.ts
rg -n "message:\s*" lib/codex-cli/sync.ts | grep -E "(error|Error|reason|String)"Repository: ndycode/codex-multi-auth
Length of output: 511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context for lines with message assignments in sync.ts
sed -n '795,810p' lib/codex-cli/sync.ts
sed -n '870,880p' lib/codex-cli/sync.ts
sed -n '335,365p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 1804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all places where CodexCliSyncRun message field is created
rg -B5 -A5 'message:\s*(error|Error)' lib/codex-cli/sync.ts
# Check if there's any redaction applied to latestRun.message in codex-manager before display
rg -B3 -A3 'latestRun\.message' lib/codex-manager.ts
# Search for any sanitization/redaction functions applied to messages
rg -n "redact.*message|sanitize.*message|mask.*message" lib/Repository: ndycode/codex-multi-auth
Length of output: 1454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full context of where hintParts is constructed and returned
sed -n '4245,4270p' lib/codex-manager.ts
# Look at forecast.ts for the redaction functions
rg -B5 -A10 "redactSensitiveReason|redactEmails" lib/forecast.ts | head -50
# Check if forecast functions are used in codex-manager for message redaction
rg -n "redactSensitiveReason|redactEmails" lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 2333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are tests for the health hint generation around line 4257
rg -n "hintParts|health.*hint|sync.*message" test/ --type=ts | head -20
# Look for tests related to CodexCliSyncRun message handling
rg -n "CodexCliSyncRun|getLastCodexCliSyncRun" test/ --type=ts | head -15
# Check what error messages could come through from sync operations
rg -n "Email|email|token" lib/codex-cli/sync.ts | grep -i "message\|error" | head -10Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if test directory exists
ls -la test/ 2>&1 | head -20
# Look for vitest config
fd -e 'vitest.config|test.config|vite.config' .
# Check for any test files
fd -e '.test.ts' -e '.spec.ts' . | head -20Repository: ndycode/codex-multi-auth
Length of output: 1374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for tests related to sync messages and health hints
fd -e '.test.ts' test/ -exec rg -l "latestRun|message|health.*hint|CodexCliSyncRun" {} \;
# Check if there are any tests for summarizeCodexCliSyncHint
rg -n "summarizeCodexCliSyncHint|health.*hint" test/ --type=tsRepository: ndycode/codex-multi-auth
Length of output: 347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for tests related to sync messages and health hints
fd '.test.ts' test/ | xargs rg -l "latestRun|message|health.*hint|CodexCliSyncRun" 2>/dev/null || echo "No matches found"
# Check if there are any tests for summarizeCodexCliSyncHint
rg -n "summarizeCodexCliSyncHint|health.*hint" test/ --type=ts 2>/dev/null || echo "No matches found"
# Look at error scenarios in sync.ts to see what messages can be set
sed -n '870,890p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 1612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the codex-cli-sync.test.ts for message handling
rg -n "message|error.*message" test/codex-cli-sync.test.ts | head -30
# Look at what kinds of errors could be included in sync messages
sed -n '280,310p' lib/codex-cli/sync.ts
# Check for any error handling in sync that might contain sensitive data
rg -B2 -A2 "error instanceof Error" lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the codex-cli-sync.test.ts file
head -100 test/codex-cli-sync.test.ts
# Look at error handling in sync.ts more carefully
sed -n '280,310p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 4781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for tests that check message content in CodexCliSyncRun
rg -n "message" test/codex-cli-sync.test.ts | head -20
# Look for tests that verify the health hint output
rg -n "summarizeCodexCliSyncHint|hint|health" test/ --type=ts | head -20
# Check what happens when an error occurs in sync - look for actual error message scenarios
sed -n '260,300p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for all error scenarios in sync that set message field
rg -B10 "message:\s*error" lib/codex-cli/sync.ts
# Check if redactSensitiveReason function is exported and available
rg -n "export.*redact|redactSensitiveReason" lib/forecast.ts
# Check if redaction is applied anywhere in codex-manager around message handling
sed -n '4220,4270p' lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 2037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if redactSensitiveReason is exported or if we need to create a new function
rg -n "^export.*function redact|^function redact" lib/forecast.ts
# See what the full redaction logic looks like
sed -n '49,85p' lib/forecast.ts
# Check if there are any tests for summarizeCodexCliSyncHint
rg -n "summarizeCodexCliSyncHint" test/ --type=tsRepository: ndycode/codex-multi-auth
Length of output: 1378
redact sync messages before surfacing them in health hints.
lib/codex-manager.ts:4257 appends latestRun.message verbatim without redaction. the message field originates from lib/codex-cli/sync.ts:875 where it's set as raw error text (error instanceof Error ? error.message : String(error)). upstream error messages can leak emails, api keys, bearer tokens, and file paths. redaction logic already exists in lib/forecast.ts (maskEmail, redactSensitiveReason) but is not used here. apply redaction before appending to hintParts, similar to how summarizeRefreshFailure handles sensitive reason text. missing regression test coverage for sanitization of this output path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4255 - 4258, The health-hint assembly is
appending latestRun.message verbatim into hintParts (see hintParts construction)
which can leak secrets; before pushing latestRun.message into hintParts, import
and apply the existing redaction utilities (maskEmail and redactSensitiveReason
from lib/forecast.ts or the module that exposes them) and use the same redaction
flow as summarizeRefreshFailure (e.g., trim the message, pass it through
redactSensitiveReason and maskEmail as appropriate) so the string appended is
sanitized; update the hintParts expression to use the redacted string and add a
regression test that constructs a latestRun.message containing
emails/tokens/paths and asserts the surfaced hint is sanitized.
| it("passes a read-only health summary into the login menu", async () => { | ||
| const now = Date.now(); | ||
| loadAccountsMock.mockResolvedValue({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [ | ||
| { | ||
| email: "enabled@example.com", | ||
| accountId: "acc_enabled", | ||
| refreshToken: "refresh-enabled", | ||
| accessToken: "access-enabled", | ||
| expiresAt: now + 3_600_000, | ||
| addedAt: now - 2_000, | ||
| lastUsed: now - 2_000, | ||
| enabled: true, | ||
| }, | ||
| { | ||
| email: "disabled@example.com", | ||
| accountId: "acc_disabled", | ||
| refreshToken: "refresh-disabled", | ||
| accessToken: "access-disabled", | ||
| expiresAt: now + 3_600_000, | ||
| addedAt: now - 1_000, | ||
| lastUsed: now - 1_000, | ||
| enabled: false, | ||
| }, | ||
| ], | ||
| }); | ||
| getActionableNamedBackupRestoresMock.mockResolvedValue({ | ||
| assessments: [{}], | ||
| totalBackups: 2, | ||
| }); | ||
| getLastCodexCliSyncRunMock.mockReturnValue({ | ||
| outcome: "changed", | ||
| runAt: now, | ||
| sourcePath: "/mock/codex/accounts.json", | ||
| targetPath: "/mock/openai-codex-accounts.json", | ||
| summary: { | ||
| sourceAccountCount: 2, | ||
| targetAccountCountBefore: 2, | ||
| targetAccountCountAfter: 2, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 1, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: true, | ||
| }, | ||
| trigger: "manual", | ||
| rollbackSnapshot: { | ||
| name: "sync-snapshot", | ||
| path: "/mock/backups/sync-snapshot.json", | ||
| }, | ||
| }); | ||
| getLatestCodexCliSyncRollbackPlanMock.mockResolvedValue({ | ||
| status: "ready", | ||
| reason: "Rollback checkpoint ready (2 accounts).", | ||
| snapshot: { | ||
| name: "sync-snapshot", | ||
| path: "/mock/backups/sync-snapshot.json", | ||
| }, | ||
| accountCount: 2, | ||
| storage: { | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [], | ||
| }, | ||
| }); | ||
| promptLoginModeMock.mockResolvedValueOnce({ mode: "cancel" }); | ||
|
|
||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "login"]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(promptLoginModeMock).toHaveBeenCalledTimes(1); | ||
| expect(promptLoginModeMock).toHaveBeenCalledWith( | ||
| expect.any(Array), | ||
| expect.objectContaining({ | ||
| healthSummary: expect.objectContaining({ | ||
| label: expect.stringContaining("Pool 1/2 enabled"), | ||
| hint: expect.stringContaining("Rollback: Rollback checkpoint ready"), | ||
| }), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
tighten this regression to assert sync-state contract explicitly.
this test validates pool and rollback text, but it can still pass if sync-state wiring regresses. since sync now depends on getLastCodexCliSyncRun() in lib/codex-manager.ts:4219 and is surfaced by buildLoginMenuHealthSummary in lib/codex-manager.ts:4357, add explicit sync assertions and call verification here (test/codex-manager-cli.test.ts:1695).
proposed test hardening
expect(exitCode).toBe(0);
expect(promptLoginModeMock).toHaveBeenCalledTimes(1);
+ expect(getLastCodexCliSyncRunMock).toHaveBeenCalledTimes(1);
expect(promptLoginModeMock).toHaveBeenCalledWith(
expect.any(Array),
expect.objectContaining({
healthSummary: expect.objectContaining({
label: expect.stringContaining("Pool 1/2 enabled"),
+ label: expect.stringContaining("Sync changed"),
hint: expect.stringContaining("Rollback: Rollback checkpoint ready"),
+ hint: expect.stringContaining("Latest sync changed"),
}),
}),
);as per coding guidelines, test/**: “tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 1662 - 1747, The test is
currently not asserting the sync-state contract so regressions in sync wiring
(sourced from getLastCodexCliSyncRun and surfaced by
buildLoginMenuHealthSummary) can slip by; update the test (around the
runCodexMultiAuthCli call in this spec) to explicitly verify
getLastCodexCliSyncRun was invoked and that the healthSummary includes the full
sync fields (e.g., summary.trigger, summary.outcome or the snapshot.name and
reason) by adding expectations that promptLoginModeMock was called with a
healthSummary containing the sync properties returned by getLastCodexCliSyncRun
and add a spy/assertion that getLastCodexCliSyncRunMock was called exactly once
to lock the contract.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4279">
P2: The out-of-range active-index check is unreachable because `resolveActiveIndex` clamps the value first.</violation>
<violation number="2" location="lib/codex-manager.ts:4892">
P2: Handle failures when building the health summary so the login menu still opens if backup metadata is malformed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const warnings: string[] = []; | ||
| const errors: string[] = []; | ||
| const activeIndex = resolveActiveIndex(storage, "codex"); |
There was a problem hiding this comment.
P2: The out-of-range active-index check is unreachable because resolveActiveIndex clamps the value first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-manager.ts, line 4279:
<comment>The out-of-range active-index check is unreachable because `resolveActiveIndex` clamps the value first.</comment>
<file context>
@@ -4194,6 +4208,179 @@ async function runDoctor(args: string[]): Promise<number> {
+
+ const warnings: string[] = [];
+ const errors: string[] = [];
+ const activeIndex = resolveActiveIndex(storage, "codex");
+ const activeExists =
+ activeIndex >= 0 && activeIndex < storage.accounts.length;
</file context>
| try { | ||
| [actionableRestores, rollbackPlan] = await Promise.all([ | ||
| getActionableNamedBackupRestores({ currentStorage: storage }), | ||
| getLatestCodexCliSyncRollbackPlan(), | ||
| ]); | ||
| } catch (error) { | ||
| console.warn("Failed to build login menu health summary", error); | ||
| } |
There was a problem hiding this comment.
Promise.all silently discards valid results on partial failure
if getActionableNamedBackupRestores succeeds but getLatestCodexCliSyncRollbackPlan throws (or vice versa), Promise.all rejects and both results fall back to the hardcoded defaults — the valid result is thrown away. the user sees Rollback: health summary unavailable even when the rollback plan loaded successfully.
on windows, antivirus locks can easily hit one file but not the other, making this split-failure path realistic.
use Promise.allSettled to preserve whichever call succeeded:
const [restoresResult, rollbackResult] = await Promise.allSettled([
getActionableNamedBackupRestores({ currentStorage: storage }),
getLatestCodexCliSyncRollbackPlan(),
]);
if (restoresResult.status === "fulfilled") {
actionableRestores = restoresResult.value;
} else {
console.warn("Failed to load actionable restores", restoresResult.reason);
}
if (rollbackResult.status === "fulfilled") {
rollbackPlan = rollbackResult.value;
} else {
console.warn("Failed to load rollback plan", rollbackResult.reason);
}there's also no vitest case that stubs only one of the two calls to reject — the existing fallback test only exercises the case where both fail together (because Promise.all short-circuits). a second test covering one-succeeds/one-fails would close this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4366-4373
Comment:
**`Promise.all` silently discards valid results on partial failure**
if `getActionableNamedBackupRestores` succeeds but `getLatestCodexCliSyncRollbackPlan` throws (or vice versa), `Promise.all` rejects and both results fall back to the hardcoded defaults — the valid result is thrown away. the user sees `Rollback: health summary unavailable` even when the rollback plan loaded successfully.
on windows, antivirus locks can easily hit one file but not the other, making this split-failure path realistic.
use `Promise.allSettled` to preserve whichever call succeeded:
```typescript
const [restoresResult, rollbackResult] = await Promise.allSettled([
getActionableNamedBackupRestores({ currentStorage: storage }),
getLatestCodexCliSyncRollbackPlan(),
]);
if (restoresResult.status === "fulfilled") {
actionableRestores = restoresResult.value;
} else {
console.warn("Failed to load actionable restores", restoresResult.reason);
}
if (rollbackResult.status === "fulfilled") {
rollbackPlan = rollbackResult.value;
} else {
console.warn("Failed to load rollback plan", rollbackResult.reason);
}
```
there's also no vitest case that stubs only one of the two calls to reject — the existing fallback test only exercises the case where both fail together (because `Promise.all` short-circuits). a second test covering one-succeeds/one-fails would close this gap.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Summary by cubic
Adds a compact, read-only Health Summary to the auth/login menu. It shows pool state, latest Codex CLI sync, backup restore readiness, rollback availability, and doctor status, with safe fallbacks if reads fail.
New Features
healthSummary.AuthMenuReadOnlyRowand menu copy; tests cover summary computation, injection, and disabled-row rendering.Bug Fixes
Written for commit 954ceb8. Summary will update on new commits.
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 a compact, read-only health summary row to the auth/login menu that surfaces pool state, latest codex cli sync outcome, backup restore readiness, rollback availability, and a lightweight doctor scan — all computed in
buildLoginMenuHealthSummaryand rendered as a disabledAuthMenuReadOnlyRow.Promise.allinbuildLoginMenuHealthSummarysilently discards successful results when either async call fails; usePromise.allSettledto handle each independently and preserve valid data on partial failure — realistic on windows where antivirus can lock one file but not the othersummarizeReadOnlyDoctorStatecopies full refresh token strings into aSet<string>for duplicate detection; prefer hashed values to minimise in-memory credential exposurePromise.allshort-circuit) but not the one-succeeds/one-fails path; adding that case would give confidence the fix in bullet 1 is regression-proofrollback-checkpointis correctly updated tocodex-cli-rollback-checkpointto match the actual implementation keyConfidence Score: 3/5
Promise.allpartial-failure bug should be fixed before relying on health data accuracyPromise.allpartial-failure issue means valid rollback or restore state can silently appear as "unavailable", which erodes the diagnostic value of the feature; the raw-token Set is a code hygiene concern; no critical path is brokenPromise.allinbuildLoginMenuHealthSummaryand theseenRefreshTokensSet insummarizeReadOnlyDoctorStateImportant Files Changed
buildLoginMenuHealthSummary,summarizeLatestCodexCliSyncState, andsummarizeReadOnlyDoctorState; thePromise.allinbuildLoginMenuHealthSummarydiscards valid results on partial failure, and the doctor-state function stores raw refresh tokens in a SetAuthMenuReadOnlyRowinterface and renders the health summary as a disabled heading + row;sanitizeTerminalTextis applied before display; change is well-scoped and safehealthSummarycopy string; trivial and correctlistAccountSnapshotswhich filters named backups byAUTO_SNAPSHOT_NAME_PATTERN; used inrunDoctor; straightforward and safehealthSummaryfield toLoginMenuOptionsand threads it through topromptLoginMode; change is minimal and correctrollback-checkpointkey tocodex-cli-rollback-checkpoint; fallback test only exercises the both-fail path viaPromise.allsemantics — no coverage for one-succeeds/one-failsSequence Diagram
sequenceDiagram participant AL as runAuthLogin participant BHS as buildLoginMenuHealthSummary participant GR as getActionableNamedBackupRestores participant RP as getLatestCodexCliSyncRollbackPlan participant SS as summarizeLatestCodexCliSyncState participant DS as summarizeReadOnlyDoctorState participant UI as showAuthMenu AL->>BHS: await buildLoginMenuHealthSummary(currentStorage) BHS->>GR: Promise.all call 1 BHS->>RP: Promise.all call 2 GR-->>BHS: assessments + totalBackups RP-->>BHS: rollbackPlan BHS->>SS: summarizeLatestCodexCliSyncState() SS-->>BHS: label + hint BHS->>DS: summarizeReadOnlyDoctorState(storage) DS-->>BHS: severity + label + hint BHS-->>AL: DashboardHealthSummary AL->>UI: promptLoginMode with healthSummary UI-->>AL: menuResultPrompt To Fix All With AI
Last reviewed commit: 954ceb8
Context used: