feat: beginner toolkit, safe-mode retries, and backup-safe account workflows#39
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds beginner onboarding and UI helpers, per-account tags/notes and persistence, a retry-budget system and tracking, export/import previews with timestamped backups, refresh-queue telemetry, schema/config additions (beginnerSafeMode, retryProfile, retryBudgetOverrides), many docs and tests; introduces new public types/APIs and storage migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Plugin as "Plugin / index.ts"
participant Storage as "lib/storage.ts"
participant Accounts as "AccountManager"
participant Backup as "createTimestampedBackupPath"
User->>CLI: run codex-import (path, dryRun?)
CLI->>Plugin: invoke import (path, dryRun)
Plugin->>Storage: readAndNormalizeImportFile(path)
Storage-->>Plugin: normalized import preview
Plugin->>CLI: return preview (dryRun=true)
alt dryRun=false
Plugin->>Backup: createTimestampedBackupPath(prefix)
Backup-->>Storage: create backup file
Plugin->>Storage: importAccounts (merged, dedup)
Storage-->>Accounts: persist accounts
Storage-->>Plugin: result {imported,total,skipped,backupStatus}
Plugin-->>CLI: display result
end
sequenceDiagram
participant Client
participant Plugin as "Plugin / index.ts"
participant Retry as "RetryBudgetTracker"
participant RefreshQ as "RefreshQueue"
participant External as "External API"
Client->>Plugin: request needing network/auth
Plugin->>Retry: consume(bucket)
Retry-->>Plugin: allowed / exhausted
alt allowed
Plugin->>RefreshQ: queue or reuse refresh
RefreshQ->>External: perform request/refresh
External-->>Plugin: response
Plugin->>Retry: optionally consume on error
else exhausted
Plugin-->>Client: structured 503 with lastRetryBudgetReason
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 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)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1868
to
+1873
| threadIdCandidate ? `${threadIdCandidate}:${Date.now()}` : undefined, | ||
| ); | ||
| runtimeMetrics.lastRequestAt = Date.now(); | ||
| const retryBudget = new RetryBudgetTracker(retryBudgetLimits); | ||
| const consumeRetryBudget = ( | ||
| bucket: RetryBudgetClass, |
There was a problem hiding this comment.
pre-import backup export runs outside storage lock - on windows, antivirus could lock the backup file during write and fail the export. consider wrapping in try-catch with graceful degradation (warn user but proceed with import) or queuing through storage lock.
no vitest coverage for windows antivirus race on backup creation.
Context Used: Rule from dashboard - What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: index.ts
Line: 1868-1873
Comment:
pre-import backup export runs outside storage lock - on windows, antivirus could lock the backup file during write and fail the export. consider wrapping in try-catch with graceful degradation (warn user but proceed with import) or queuing through storage lock.
no vitest coverage for windows antivirus race on backup creation.
**Context Used:** Rule from `dashboard` - What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... ([source](https://app.greptile.com/review/custom-context?memory=637a42e6-7a78-40d6-9ef8-6a45e02e73b6))
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+5345
to
+5347
| if (backupPath) { | ||
| await exportAccounts(backupPath, true); | ||
| } |
There was a problem hiding this comment.
pre-import backup export runs outside storage lock - on windows, antivirus could lock the backup file during write and fail the export. consider wrapping in try-catch with graceful degradation (warn user but proceed with import) or queuing through storage lock.
no vitest coverage for windows antivirus race on backup creation.
Context Used: Rule from dashboard - What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: index.ts
Line: 5345-5347
Comment:
pre-import backup export runs outside storage lock - on windows, antivirus could lock the backup file during write and fail the export. consider wrapping in try-catch with graceful degradation (warn user but proceed with import) or queuing through storage lock.
no vitest coverage for windows antivirus race on backup creation.
**Context Used:** Rule from `dashboard` - What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... ([source](https://app.greptile.com/review/custom-context?memory=637a42e6-7a78-40d6-9ef8-6a45e02e73b6))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
docs/development/TESTING.md (1)
28-33: Include lint in the pre-release validation command set.Line 28 currently recommends only typecheck + tests. Add lint so the documented release gate matches the project’s stated validation flow.
Suggested doc tweak
Recommended validation command before release: ```bash +npm run lint npm run typecheck npm test</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/development/TESTING.mdaround lines 28 - 33, Update the pre-release
validation command list in TESTING.md to include linting; edit the code block
that currently shows "npm run typecheck" and "npm test" to also run "npm run
lint" (typically before typecheck) so the documented release gate matches the
project's validation flow.</details> </blockquote></details> <details> <summary>lib/schemas.ts (1)</summary><blockquote> `24-31`: **Add integer constraints to retry budget schema fields.** Lines 24–31 currently allow decimal values for retry budgets, which are operationally attempt counts and should accept integers only. The code gracefully normalizes fractional input at runtime via `Math.floor()` (in `resolveRetryBudgetLimits`), but the schema should be explicit about this constraint. <details> <summary>Suggested schema fix</summary> ```diff retryBudgetOverrides: z.object({ - authRefresh: z.number().min(0).optional(), - network: z.number().min(0).optional(), - server: z.number().min(0).optional(), - rateLimitShort: z.number().min(0).optional(), - rateLimitGlobal: z.number().min(0).optional(), - emptyResponse: z.number().min(0).optional(), + authRefresh: z.number().int().min(0).optional(), + network: z.number().int().min(0).optional(), + server: z.number().int().min(0).optional(), + rateLimitShort: z.number().int().min(0).optional(), + rateLimitGlobal: z.number().int().min(0).optional(), + emptyResponse: z.number().int().min(0).optional(), }).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/schemas.ts` around lines 24 - 31, The retryBudgetOverrides schema currently permits non-integer numbers; update each field inside retryBudgetOverrides (authRefresh, network, server, rateLimitShort, rateLimitGlobal, emptyResponse) to require integers by replacing z.number().min(0).optional() with z.number().int().min(0).optional(); keep optionality unchanged and ensure this aligns with runtime normalization in resolveRetryBudgetLimits which floors fractional input.test/accounts.test.ts (1)
1847-1847: Use the captured timestamp for deterministic explainability checks.Passing
nowinstead of a secondDate.now()call makes boundary behavior more stable.Small determinism tweak
- const explainability = manager.getSelectionExplainability("codex", undefined, Date.now()); + const explainability = manager.getSelectionExplainability("codex", undefined, now);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/accounts.test.ts` at line 1847, The test currently calls manager.getSelectionExplainability("codex", undefined, Date.now()) which uses a fresh timestamp and can cause nondeterministic boundary behavior; change the call to use the previously captured timestamp variable (e.g., now) so it becomes manager.getSelectionExplainability("codex", undefined, now) to ensure deterministic explainability checks and stable test boundaries.test/storage.test.ts (2)
182-185: Assertskippedin preview assertions to lock API contract.
previewImportAccounts()returnsimported,total, andskipped; this test currently verifies only two fields.Suggested test assertion
const preview = await previewImportAccounts(exportPath); expect(preview.imported).toBe(1); expect(preview.total).toBe(2); + expect(preview.skipped).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 182 - 185, The test currently asserts only preview.imported and preview.total; update the assertion block in the test that calls previewImportAccounts to also assert preview.skipped equals the expected value (e.g., 1) so the API contract is locked. Locate the call to previewImportAccounts and the variable preview in the test (storage.test.ts) and add an expectation for the skipped field alongside the existing imported and total assertions.
191-196: Strengthen backup-path assertion to include expected base directory.Current checks can still pass with an incorrect directory as long as
"backups"appears somewhere in the string.Suggested assertion hardening
it("creates timestamped backup paths in storage backups directory", () => { const path = createTimestampedBackupPath(); + expect(path).toContain(join(testWorkDir, "backups")); expect(path).toContain("backups"); expect(path).toContain("codex-backup-"); expect(path.endsWith(".json")).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 191 - 196, The test's "backups" assertion is too weak; update the it block using createTimestampedBackupPath to assert the path uses the expected base directory explicitly (e.g., ensure path starts with "backups" followed by a path separator or matches a regex like /^backups[\/\\]/) instead of just toContain("backups"); keep the other assertions (contains "codex-backup-" and endsWith(".json")) and replace the loose expect(path).toContain("backups") with a startsWith or regex check so the path is guaranteed to be inside the backups directory (reference createTimestampedBackupPath).lib/refresh-queue.ts (1)
127-129: Consider centralizing pending-metric synchronization.
pendingmetric updates are duplicated across branches; a tiny helper would reduce drift risk.Refactor sketch
+ private syncPendingMetric(): void { + this.metrics.pending = this.pending.size; + } ... - this.metrics.pending = this.pending.size; + this.syncPendingMetric(); ... - this.metrics.pending = this.pending.size; + this.syncPendingMetric(); ... - this.metrics.pending = this.pending.size; + this.syncPendingMetric(); ... - this.metrics.pending = this.pending.size; + this.syncPendingMetric(); ... - this.metrics.pending = this.pending.size; + this.syncPendingMetric();Also applies to: 142-144, 159-160, 166-167, 278-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/refresh-queue.ts` around lines 127 - 129, The code repeatedly sets this.metrics.pending = this.pending.size in multiple branches (e.g., where this.metrics.deduplicated is incremented and other refresh-queue branches), which risks drift; add a small helper method on the RefreshQueue class (e.g., syncPendingMetric or updatePendingMetric) that sets this.metrics.pending = this.pending.size and call it wherever pending is changed (references: this.pending, this.metrics.pending, places around the deduplication increment and the other branches at the mentioned locations) so all updates are centralized and reduce duplication.lib/config.ts (1)
260-264: Deduplicate retry-budget normalization logic.
normalizeRetryBudgetValuehere duplicates the helper inlib/request/retry-budget.ts. Consider a single shared implementation to avoid drift between config parsing and runtime budget resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/config.ts` around lines 260 - 264, The normalizeRetryBudgetValue implementation in lib/config.ts duplicates logic already in lib/request/retry-budget.ts; remove the local function and instead import and use the shared helper from lib/request/retry-budget.ts (e.g., the exported normalizeRetryBudgetValue or equivalent function) so config parsing calls that single implementation; update any references in this file to use the imported symbol and ensure the import is added to the top of the file.test/index.test.ts (1)
953-958: Strengthen dry-run and timestamped-export tests with collaborator assertions.These tests currently verify message text only; they should also assert the correct storage helper functions were invoked.
✅ Suggested test hardening
it("exports to timestamped path when path is omitted", async () => { mockStorage.accounts = [{ refreshToken: "r1" }]; + const storageModule = await import("../lib/storage.js"); + vi.mocked(storageModule.createTimestampedBackupPath).mockClear(); const result = await plugin.tool["codex-export"].execute({}); expect(result).toContain("Exported"); expect(result).toContain("codex-backup"); + expect(storageModule.createTimestampedBackupPath).toHaveBeenCalled(); }); @@ it("supports dry-run preview mode", async () => { + const storageModule = await import("../lib/storage.js"); + vi.mocked(storageModule.previewImportAccounts).mockClear(); + vi.mocked(storageModule.importAccounts).mockClear(); const result = await plugin.tool["codex-import"].execute({ path: "/tmp/backup.json", dryRun: true, }); expect(result).toContain("Import preview"); + expect(storageModule.previewImportAccounts).toHaveBeenCalledWith("/tmp/backup.json"); + expect(storageModule.importAccounts).not.toHaveBeenCalled(); });Also applies to: 970-976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 953 - 958, Update the two tests (the dry-run and the timestamped-export cases that call plugin.tool["codex-export"].execute) to not only assert on the result text but also assert that the mock storage helper was invoked to persist the export: check that mockStorage's export/save/upload method (the helper your code uses to write backups — e.g., mockStorage.put/mockStorage.upload/mockStorage.save) was called and that the path argument contains the expected "codex-backup" timestamped segment; keep references to mockStorage.accounts setup and the call to plugin.tool["codex-export"].execute when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration.md`:
- Line 115: Update the docs to remove the apparent contradiction by making clear
that the snippet value for retryAllAccountsMaxRetries is an example override and
not the default; explicitly annotate the example block near
"retryAllAccountsMaxRetries": 3 with a short inline note like “(example)” or add
a comment stating “default: Infinity” and ensure the options table entry for
retryAllAccountsMaxRetries still documents the true default as Infinity; apply
the same clarification for the second occurrence at the other location.
In `@docs/index.md`:
- Line 82: Update the table cell under "**Comprehensive Tests**" in
docs/index.md to reflect the current PR run output by replacing the string "1752
tests" with "1,756 tests" (ensure the rest of the cell text "80% coverage
threshold) + integration tests" is preserved); locate the table row that
contains "**Comprehensive Tests**" and edit only the test count value.
In `@docs/README.md`:
- Line 36: Update the test count string in the docs README: replace the current
"Testing: 1752 tests plus integration coverage." entry with the corrected count
"Testing: 1,756 tests plus integration coverage." so the README matches the PR
objective; locate the exact line containing the "Testing: 1752 tests plus
integration coverage." text and update the numeral to 1,756.
In `@index.ts`:
- Around line 2149-2179: The early return path after a network error currently
returns when consumeRetryBudget("network", ...) is false but does not refund the
previously consumed local token; update the failure handling so that before
returning due to exhausted network retry budget you call
accountManager.refundToken(account, modelFamily, model) (and optionally
accountManager.recordFailure(...) and update runtimeMetrics as done in the other
branch) to ensure the local token bucket is restored; locate this logic around
the network error handling where consumeRetryBudget is checked and add the
refund call immediately prior to constructing and returning the 503 Response.
In `@lib/accounts.ts`:
- Around line 444-447: The code currently sets coolingDownUntil from
account.coolingDownUntil but returns cooldownReason unconditionally; update the
logic so cooldownReason is only included when coolingDownUntil is active (i.e.,
defined and > now). Concretely, in the block that computes coolingDownUntil
(using account.coolingDownUntil and now) only assign or return
account.cooldownReason when coolingDownUntil is truthy; otherwise set
cooldownReason to undefined or omit it. Apply the same conditional change for
the other occurrence around the code referencing account.coolingDownUntil at the
477-477 location.
In `@lib/request/retry-budget.ts`:
- Line 91: The constructor for RetryBudgetTracker captures the passed-in
RetryBudgetLimits by reference, making behavior mutable if the caller later
mutates that object; change the constructor in class RetryBudgetTracker to
defensively clone the provided limits (e.g., create a shallow copy of the
RetryBudgetLimits object) and store that copy on the instance instead of the
original reference so subsequent external mutations don't affect tracker
behavior (refer to the constructor signature taking limits: RetryBudgetLimits
and the class RetryBudgetTracker).
In `@lib/schemas.ts`:
- Around line 99-100: The schema currently allows whitespace-only tags and empty
notes; update the accountTags and accountNote Zod definitions to mirror the
persistence normalization by trimming and removing empties using .transform():
for accountTags, use z.array(z.string().transform(s => s.trim()).refine(s =>
s.length > 0)).optional() or alternatively z.array(z.string().transform(s =>
s.trim())).transform(arr => arr.filter(s => s.length > 0)).optional(); for
accountNote, use z.string().transform(s => { const t = s.trim(); return t === ''
? undefined : t }).optional() (or a trim+refine approach that converts empty ->
undefined) so the schema emits trimmed, filtered tags and a trimmed/undefined
note; apply the same pattern for the other fields referenced at the other
location (lines 146-147).
In `@lib/storage.ts`:
- Around line 980-994: The backup filename generation in
formatBackupTimestamp/createTimestampedBackupPath can collide within the same
second and accepts unsafe prefix input; fix by sanitizing the prefix (strip path
separators and parent refs, e.g., use path.basename and replace non-alphanumeric
characters with a safe delimiter) and append stronger uniqueness to the
timestamp (include milliseconds and a short random/unique suffix such as
crypto.randomUUID() or Date.now() + random hex) to the generated name; ensure
the final filename is composed only of safe characters and never includes
directory separators before joining into the backups directory so
createTimestampedBackupPath produces a unique, path-safe file name.
In `@lib/ui/beginner.ts`:
- Line 122: Beginner guidance strings use angle-bracket placeholders (e.g.,
"codex-switch <index>") which are hard to copy; update the relevant command
property values in lib/ui/beginner.ts to use executable explicit argument syntax
instead (for example replace "codex-switch <index>" with "codex-switch index=2"
and similarly change any "<label>" or other placeholders to concrete examples
like 'label="summary"' or label=summary). Locate the objects/entries that define
the command property (the "command" fields at the shown spots and the other
occurrences around the file) and replace the placeholder form with explicit
example arguments consistently (lines referenced: the "command" property at the
shown diff and the other occurrences noted).
In `@README.md`:
- Line 317: The fenced code blocks containing the snippet "codex-label" (and the
other occurrences at the same pattern) are missing a language identifier,
causing markdownlint MD040; update each triple-backtick fence that wraps the
lines like codex-label index=2 label="Work" (occurrences referenced in the
comment) to include an appropriate language tag (e.g., use ```text or another
explicit tag) on the opening fence so each fenced block is ```text codex-label
... ``` instead of just ```.
---
Nitpick comments:
In `@docs/development/TESTING.md`:
- Around line 28-33: Update the pre-release validation command list in
TESTING.md to include linting; edit the code block that currently shows "npm run
typecheck" and "npm test" to also run "npm run lint" (typically before
typecheck) so the documented release gate matches the project's validation flow.
In `@lib/config.ts`:
- Around line 260-264: The normalizeRetryBudgetValue implementation in
lib/config.ts duplicates logic already in lib/request/retry-budget.ts; remove
the local function and instead import and use the shared helper from
lib/request/retry-budget.ts (e.g., the exported normalizeRetryBudgetValue or
equivalent function) so config parsing calls that single implementation; update
any references in this file to use the imported symbol and ensure the import is
added to the top of the file.
In `@lib/refresh-queue.ts`:
- Around line 127-129: The code repeatedly sets this.metrics.pending =
this.pending.size in multiple branches (e.g., where this.metrics.deduplicated is
incremented and other refresh-queue branches), which risks drift; add a small
helper method on the RefreshQueue class (e.g., syncPendingMetric or
updatePendingMetric) that sets this.metrics.pending = this.pending.size and call
it wherever pending is changed (references: this.pending, this.metrics.pending,
places around the deduplication increment and the other branches at the
mentioned locations) so all updates are centralized and reduce duplication.
In `@lib/schemas.ts`:
- Around line 24-31: The retryBudgetOverrides schema currently permits
non-integer numbers; update each field inside retryBudgetOverrides (authRefresh,
network, server, rateLimitShort, rateLimitGlobal, emptyResponse) to require
integers by replacing z.number().min(0).optional() with
z.number().int().min(0).optional(); keep optionality unchanged and ensure this
aligns with runtime normalization in resolveRetryBudgetLimits which floors
fractional input.
In `@test/accounts.test.ts`:
- Line 1847: The test currently calls
manager.getSelectionExplainability("codex", undefined, Date.now()) which uses a
fresh timestamp and can cause nondeterministic boundary behavior; change the
call to use the previously captured timestamp variable (e.g., now) so it becomes
manager.getSelectionExplainability("codex", undefined, now) to ensure
deterministic explainability checks and stable test boundaries.
In `@test/index.test.ts`:
- Around line 953-958: Update the two tests (the dry-run and the
timestamped-export cases that call plugin.tool["codex-export"].execute) to not
only assert on the result text but also assert that the mock storage helper was
invoked to persist the export: check that mockStorage's export/save/upload
method (the helper your code uses to write backups — e.g.,
mockStorage.put/mockStorage.upload/mockStorage.save) was called and that the
path argument contains the expected "codex-backup" timestamped segment; keep
references to mockStorage.accounts setup and the call to
plugin.tool["codex-export"].execute when adding these assertions.
In `@test/storage.test.ts`:
- Around line 182-185: The test currently asserts only preview.imported and
preview.total; update the assertion block in the test that calls
previewImportAccounts to also assert preview.skipped equals the expected value
(e.g., 1) so the API contract is locked. Locate the call to
previewImportAccounts and the variable preview in the test (storage.test.ts) and
add an expectation for the skipped field alongside the existing imported and
total assertions.
- Around line 191-196: The test's "backups" assertion is too weak; update the it
block using createTimestampedBackupPath to assert the path uses the expected
base directory explicitly (e.g., ensure path starts with "backups" followed by a
path separator or matches a regex like /^backups[\/\\]/) instead of just
toContain("backups"); keep the other assertions (contains "codex-backup-" and
endsWith(".json")) and replace the loose expect(path).toContain("backups") with
a startsWith or regex check so the path is guaranteed to be inside the backups
directory (reference createTimestampedBackupPath).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (28)
CHANGELOG.mdREADME.mddocs/README.mddocs/configuration.mddocs/development/ARCHITECTURE.mddocs/development/CONFIG_FIELDS.mddocs/development/TESTING.mddocs/getting-started.mddocs/index.mddocs/troubleshooting.mdindex.tslib/accounts.tslib/config.tslib/refresh-queue.tslib/request/retry-budget.tslib/schemas.tslib/storage.tslib/storage/migrations.tslib/ui/beginner.tstest/accounts.test.tstest/beginner-ui.test.tstest/index-retry.test.tstest/index.test.tstest/plugin-config.test.tstest/refresh-queue.test.tstest/retry-budget.test.tstest/schemas.test.tstest/storage.test.ts
| 9. **Safety-First Account Backup Flow** - Timestamped exports, import dry-run previews, and pre-import snapshots before apply when existing accounts are present. | ||
|
|
||
| **Testing**: 1689 tests plus integration coverage. | ||
| **Testing**: 1752 tests plus integration coverage. |
Contributor
There was a problem hiding this comment.
Sync test count with the latest run.
Line 36 lists 1752 tests, while the PR objective indicates 1,756. Please update for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/README.md` at line 36, Update the test count string in the docs README:
replace the current "Testing: 1752 tests plus integration coverage." entry with
the corrected count "Testing: 1,756 tests plus integration coverage." so the
README matches the PR objective; locate the exact line containing the "Testing:
1752 tests plus integration coverage." text and update the numeral to 1,756.
Comment on lines
+444
to
+447
| const coolingDownUntil = | ||
| typeof account.coolingDownUntil === "number" && account.coolingDownUntil > now | ||
| ? account.coolingDownUntil | ||
| : undefined; |
Contributor
There was a problem hiding this comment.
Avoid reporting stale cooldownReason when cooldown is inactive.
cooldownReason is returned unconditionally, even when coolingDownUntil is not active. That can mislead diagnostics.
🔧 Proposed fix
const coolingDownUntil =
typeof account.coolingDownUntil === "number" && account.coolingDownUntil > now
? account.coolingDownUntil
: undefined;
+ const cooldownReason =
+ coolingDownUntil !== undefined ? account.cooldownReason : undefined;
@@
- cooldownReason: account.cooldownReason,
+ cooldownReason,Also applies to: 477-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts.ts` around lines 444 - 447, The code currently sets
coolingDownUntil from account.coolingDownUntil but returns cooldownReason
unconditionally; update the logic so cooldownReason is only included when
coolingDownUntil is active (i.e., defined and > now). Concretely, in the block
that computes coolingDownUntil (using account.coolingDownUntil and now) only
assign or return account.cooldownReason when coolingDownUntil is truthy;
otherwise set cooldownReason to undefined or omit it. Apply the same conditional
change for the other occurrence around the code referencing
account.coolingDownUntil at the 477-477 location.
Additional Comments (1)
suggest extracting the retry-with-backoff logic into a shared helper or adding the same retry pattern here. no vitest coverage for windows filesystem race on flagged account writes. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
README.md (1)
480-482:⚠️ Potential issue | 🟡 MinorAdd language identifiers to the new fenced code blocks.
Line 480 and Line 562 open fenced blocks with bare ```; please annotate them (e.g.,
text) to avoid MD040-style lint noise.Suggested doc diff
-``` +```text codex-remove-
+text
Codex Accounts (3):[1] Account 1 (user@gmail.com, workspace:Work, tags:work,team-a) active
[2] Account 2 (backup@email.com, tags:backup) ok
[3] Account 3 (personal@email.com) rate-limitedStorage: ~/.opencode/openai-codex-accounts.json
Also applies to: 562-570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 480 - 482, Update the two new fenced code blocks that currently start with bare ``` so they include a language identifier (use "text"); specifically modify the block containing the single token "codex-remove" and the block that lists "Codex Accounts (3): ..." to open with ```text instead of ```, ensuring both fenced blocks are annotated to satisfy MD040 linting.docs/index.md (1)
82-82:⚠️ Potential issue | 🟡 MinorAlign the documented test count with this PR’s reported run.
Line 82 says
1,762 tests, but this PR objective reports1,756 testspassed. Please sync this value to avoid stale release docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.md` at line 82, Update the test count in the docs table row labeled "Comprehensive Tests" so the documented number of tests matches the PR run (change "1,762 tests" to "1,756 tests"); edit the table entry string in docs/index.md where the cell contains "**Comprehensive Tests** | 1,762 tests (80% coverage threshold) + integration tests" to reflect the new value.
🧹 Nitpick comments (1)
test/index.test.ts (1)
978-1022: Add a race condition regression test for pre-import backup and import sequencing.The backup (
exportAccounts) and import (importAccounts) operations use separatewithAccountStorageTransactionboundaries rather than a shared lock. Between the backup commit and the import start, concurrent mutations could occur: the backup captures state S1, but the import later applies changes to the current state (which may be S2 after intervening modifications), leaving the backup out-of-sync with what was actually imported.The three existing tests cover dry-run, empty-account, and backup-failure paths, but none verify behavior when accounts are modified concurrently between backup and apply. A targeted concurrency test would confirm either that the current sequence is safe or that the backup and import need to be wrapped in a unified transaction lock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 978 - 1022, Add a new test "detects race between pre-import backup and import" that uses plugin.tool["codex-import"].execute and the same mocked storage module used elsewhere; set mockStorage.accounts to an initial snapshot S1, then import "../lib/storage.js" and mock storageModule.exportAccounts with an implementation that captures the exported snapshot (so you can assert it saw S1) and mutates mockStorage.accounts to a different snapshot S2 before resolving, then let importAccounts proceed and assert storageModule.exportAccounts was called and captured S1 while storageModule.importAccounts was called afterwards (with the expected path) and the system applied the current state (showing the sequencing/race). Use vi.mocked(...) and mockImplementationOnce to orchestrate the mutation between exportAccounts and importAccounts and include assertions that reveal the race condition (export saw S1, import ran with S2/current state).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 5272-5278: The resolvedExportPath logic always calls
createTimestampedBackupPath even when timestamped is false; update the branch
that handles no filePath so that when shouldTimestamp is false it returns a
non-timestamped default string (e.g., "codex-backup") instead of calling
createTimestampedBackupPath("codex-backup"). Locate the resolvedExportPath
calculation (uses shouldTimestamp, timestamped, filePath,
createTimestampedBackupPath) and change the ternary so: if filePath is provided
use it, otherwise if shouldTimestamp call createTimestampedBackupPath(), else
return the plain non-timestamped default name.
---
Duplicate comments:
In `@docs/index.md`:
- Line 82: Update the test count in the docs table row labeled "Comprehensive
Tests" so the documented number of tests matches the PR run (change "1,762
tests" to "1,756 tests"); edit the table entry string in docs/index.md where the
cell contains "**Comprehensive Tests** | 1,762 tests (80% coverage threshold) +
integration tests" to reflect the new value.
In `@README.md`:
- Around line 480-482: Update the two new fenced code blocks that currently
start with bare ``` so they include a language identifier (use "text");
specifically modify the block containing the single token "codex-remove" and the
block that lists "Codex Accounts (3): ..." to open with ```text instead of ```,
ensuring both fenced blocks are annotated to satisfy MD040 linting.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 978-1022: Add a new test "detects race between pre-import backup
and import" that uses plugin.tool["codex-import"].execute and the same mocked
storage module used elsewhere; set mockStorage.accounts to an initial snapshot
S1, then import "../lib/storage.js" and mock storageModule.exportAccounts with
an implementation that captures the exported snapshot (so you can assert it saw
S1) and mutates mockStorage.accounts to a different snapshot S2 before
resolving, then let importAccounts proceed and assert
storageModule.exportAccounts was called and captured S1 while
storageModule.importAccounts was called afterwards (with the expected path) and
the system applied the current state (showing the sequencing/race). Use
vi.mocked(...) and mockImplementationOnce to orchestrate the mutation between
exportAccounts and importAccounts and include assertions that reveal the race
condition (export saw S1, import ran with S2/current state).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
README.mddocs/README.mddocs/configuration.mddocs/development/TESTING.mddocs/index.mdindex.tslib/accounts.tslib/config.tslib/request/retry-budget.tslib/schemas.tslib/storage.tslib/ui/beginner.tstest/accounts.test.tstest/index.test.tstest/retry-budget.test.tstest/schemas.test.tstest/storage.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/config.ts
- docs/README.md
- docs/development/TESTING.md
- lib/accounts.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
973-994:⚠️ Potential issue | 🟡 MinorMissing Windows EPERM/EBUSY retry logic in
saveFlaggedAccounts.The
saveAccountsUnlockedfunction (lines 744-759) includes retry logic with exponential backoff for WindowsEPERM/EBUSYerrors during rename operations, butsaveFlaggedAccountsperforms a directfs.renamewithout this protection. This can cause sporadic failures on Windows when antivirus software or file indexers temporarily lock files.🔧 Proposed fix: Add retry logic to saveFlaggedAccounts
export async function saveFlaggedAccounts(storage: FlaggedAccountStorageV1): Promise<void> { return withStorageLock(async () => { const path = getFlaggedAccountsPath(); const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; try { await fs.mkdir(dirname(path), { recursive: true }); const content = JSON.stringify(normalizeFlaggedStorage(storage), null, 2); await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); - await fs.rename(tempPath, path); + + // Retry rename with exponential backoff for Windows EPERM/EBUSY + let lastError: NodeJS.ErrnoException | null = null; + for (let attempt = 0; attempt < 5; attempt++) { + try { + await fs.rename(tempPath, path); + return; + } catch (renameError) { + const code = (renameError as NodeJS.ErrnoException).code; + if (code === "EPERM" || code === "EBUSY") { + lastError = renameError as NodeJS.ErrnoException; + await new Promise(r => setTimeout(r, 10 * Math.pow(2, attempt))); + continue; + } + throw renameError; + } + } + if (lastError) throw lastError; } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 973 - 994, saveFlaggedAccounts currently does a direct fs.rename which can fail on Windows with transient EPERM/EBUSY; update saveFlaggedAccounts to use the same retry-with-exponential-backoff rename logic used in saveAccountsUnlocked (or extract/reuse that logic as a helper like retryRenameOnWindows) so rename(tempPath, path) is retried for EPERM/EBUSY on Windows with increasing delays, preserve the existing tempPath unlink cleanup and error logging behavior, and ensure the helper is referenced from saveFlaggedAccounts (keep function name saveFlaggedAccounts intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 4614-4617: The not-found message uses the wrong variable so it can
print "Account undefined not found."; update the return to reference the
resolved index (targetIndex) instead of index in the branch that checks
storage.accounts[targetIndex] (where account is declared) so the error string
reports the actual queried index.
- Around line 2007-2014: The code calls accountManager.refundToken(account,
modelFamily, model) in the auth-refresh failure branch before any token was
consumed, which can incorrectly inflate token capacity; remove that refund from
the if (!consumeRetryBudget(...)) block and instead refund only when a token has
actually been consumed (e.g., move the refund call to the code path immediately
after a successful token consumption or gate it behind a tokenConsumed boolean
that is set when the token is consumed), keeping references to
consumeRetryBudget, accountManager.refundToken, authRefresh, account,
modelFamily, and model to locate and update the logic.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 973-994: saveFlaggedAccounts currently does a direct fs.rename
which can fail on Windows with transient EPERM/EBUSY; update saveFlaggedAccounts
to use the same retry-with-exponential-backoff rename logic used in
saveAccountsUnlocked (or extract/reuse that logic as a helper like
retryRenameOnWindows) so rename(tempPath, path) is retried for EPERM/EBUSY on
Windows with increasing delays, preserve the existing tempPath unlink cleanup
and error logging behavior, and ensure the helper is referenced from
saveFlaggedAccounts (keep function name saveFlaggedAccounts intact).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
README.mddocs/README.mddocs/index.mdindex.tslib/storage.tstest/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/README.md
Comment on lines
+2007
to
+2014
| if ( | ||
| !consumeRetryBudget( | ||
| "authRefresh", | ||
| `Auth refresh failed for account ${account.index + 1}`, | ||
| ) | ||
| ) { | ||
| accountManager.refundToken(account, modelFamily, model); | ||
| return new Response( |
Contributor
There was a problem hiding this comment.
Avoid refunding a token that was never consumed.
In the auth-refresh failure path, accountManager.refundToken(...) runs before token consumption happens, which can inflate local token capacity.
🔧 Proposed fix
if (
!consumeRetryBudget(
"authRefresh",
`Auth refresh failed for account ${account.index + 1}`,
)
) {
- accountManager.refundToken(account, modelFamily, model);
return new Response(
JSON.stringify({
error: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` around lines 2007 - 2014, The code calls
accountManager.refundToken(account, modelFamily, model) in the auth-refresh
failure branch before any token was consumed, which can incorrectly inflate
token capacity; remove that refund from the if (!consumeRetryBudget(...)) block
and instead refund only when a token has actually been consumed (e.g., move the
refund call to the code path immediately after a successful token consumption or
gate it behind a tokenConsumed boolean that is set when the token is consumed),
keeping references to consumeRetryBudget, accountManager.refundToken,
authRefresh, account, modelFamily, and model to locate and update the logic.
Additional Comments (1)
recommend: move backup creation outside withAccountStorageTransaction, or add explicit timeout with graceful degradation. if backup is critical, document the blocking behavior and windows antivirus exclusion requirement. no vitest coverage for windows antivirus locking backup file (EPERM/EBUSY on backup write, not main storage file). |
ndycode
added a commit
that referenced
this pull request
Apr 6, 2026
…rkflows (#39) * feat: add beginner toolkit, safe-mode retries, and backup-safe account workflows * fix: resolve coderabbit and greptile review findings * fix: address latest review comments on import/export flow * fix: address latest bot findings for retries and messaging
ndycode
added a commit
that referenced
this pull request
Apr 6, 2026
…rkflows (#39) * feat: add beginner toolkit, safe-mode retries, and backup-safe account workflows * fix: resolve coderabbit and greptile review findings * fix: address latest review comments on import/export flow * fix: address latest bot findings for retries and messaging
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR ships a beginner-focused operations layer, safer account backup/import workflows, and expanded resilience/retry controls, with full docs + test coverage updates.
What Changed
1) Beginner command toolkit
codex-helptopic-guided command help (setup,switch,health,backup,dashboard,metrics).codex-setupchecklist flow plus wizard mode with graceful fallback when interactive menus are unavailable.codex-doctordiagnostics with actionable findings and optional safe remediation (fix=true).codex-nextto surface a single recommended next action.2) Account metadata + beginner UX improvements
accountTags?: string[]accountNote?: stringcodex-tag(set/clear tags)codex-note(set/clear note)codex-listwithtagfilter support.codex-switch,codex-label, andcodex-removenow support optional index + interactive picker on compatible terminals.3) Backup/import hardening
createTimestampedBackupPath()andpreviewImportAccounts()in storage flow.codex-exportnow supports omitted path with timestamped backup generation.codex-importnow supportsdryRunpreview.No accounts to export; pre-import backup is skipped cleanly.4) Retry/safety behavior
beginnerSafeModeCODEX_AUTH_BEGINNER_SAFE_MODE5) Docs + changelog refresh
Files of Interest
index.tslib/ui/beginner.tslib/request/retry-budget.tslib/storage.ts,lib/storage/migrations.ts,lib/schemas.ts,lib/accounts.ts,lib/config.tstest/index.test.ts,test/index-retry.test.ts,test/storage.test.ts,test/schemas.test.ts,test/plugin-config.test.ts,test/accounts.test.ts,test/refresh-queue.test.ts,test/beginner-ui.test.ts,test/retry-budget.test.tsValidation
Static/automated
npm run lint✅npm run typecheck✅npm test✅ (56files,1756tests)Runtime smoke verification (real plugin runtime)
Executed an isolated runtime harness against built
dist/index.js(not Vitest mocks), covering:codex-helptopic outputcodex-setupwizard fallbackcodex-importdry-run and applycodex-label/codex-tag/codex-notecodex-listtag filteringcodex-doctordeep modecodex-nextcodex-exporttimestamped pathcodex-removeResult: 12/12 checks passed in isolated temp project.
Notes
Unknown env config "_authtoken") during commands; no lint/type/test failures.Summary by CodeRabbit
New Features
Configuration
Improvements
Tests/Docs
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
ships beginner toolkit (codex-help/setup/doctor/next), account tags/notes, backup hardening with windows filesystem safety, and retry budget system.
key changes:
windows filesystem defense:
token safety:
test coverage:
previous threads addressed:
Confidence Score: 4/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Start([User runs codex command]) Start --> CheckAccounts{Accounts exist?} CheckAccounts -->|No| NoAccounts[codex-setup: Add account] CheckAccounts -->|Yes| CheckHealth{Healthy accounts?} CheckHealth -->|No| Doctor[codex-doctor: Diagnose issues] CheckHealth -->|Yes| Operations[Normal operations] Doctor --> Findings{Severity?} Findings -->|Error| Critical[Critical: Re-login required] Findings -->|Warning| Warning[Warning: Rotate/wait] Findings -->|OK| Operations Operations --> Request{Make request} Request --> Budget{Retry budget?} Budget -->|Exhausted| Failed[Request failed] Budget -->|Available| Attempt[Attempt request] Attempt --> Result{Result?} Result -->|Success| Success[Return response] Result -->|Auth failure| ConsumeAuth[Consume authRefresh budget] Result -->|Network error| ConsumeNet[Consume network budget] Result -->|Rate limit| ConsumeRL[Consume rateLimitShort budget] Result -->|Server 5xx| ConsumeServer[Consume server budget] ConsumeAuth --> Budget ConsumeNet --> Budget ConsumeRL --> Budget ConsumeServer --> Budget Operations --> Import{Import accounts?} Import -->|Yes| DryRun{Dry run?} DryRun -->|Yes| Preview[Preview: Show counts only] DryRun -->|No| CheckExisting{Existing accounts?} CheckExisting -->|No| DoImport[Import directly] CheckExisting -->|Yes| Backup[Create timestamped backup] Backup --> BackupMode{Backup mode?} BackupMode -->|Required| BackupOK{Backup success?} BackupMode -->|Best-effort| TryBackup[Try backup, continue on fail] BackupMode -->|None| DoImport BackupOK -->|Yes| DoImport BackupOK -->|No| AbortImport[Abort: Backup failed] TryBackup --> DoImport DoImport --> ImportDone[Import complete] style Critical fill:#ff6b6b style Warning fill:#feca57 style Success fill:#48dbfb style AbortImport fill:#ff6b6bLast reviewed commit: 8e504e7