refactor: extract storage error hints

[ndycode]

Summary

• extract storage error-hint formatting out of lib/storage.ts into a dedicated helper module
• keep the existing StorageError and storage-save behavior intact while starting the storage split in a small low-risk slice

What Changed

• added lib/storage/error-hints.ts with formatStorageErrorHint(...)
• updated lib/storage.ts to import and re-export the helper so the existing public surface stays intact
• updated test/storage.test.ts to import the helper from its new module while still exercising the same behavior expectations

Validation

• npm run test -- test/storage.test.ts
• npm run lint
• npm run typecheck
• npm run build

Risk and Rollback

• Risk level: low
• Rollback plan: revert aa77ab7 to restore the inline storage error-hint implementation

Additional Notes

• this begins the storage split after the settings-hub panel extractions in the same isolated worktree and stacked review flow

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 completes a clean, low-risk slice of the storage split: StorageError is moved into lib/errors.ts (now correctly extending CodexError), error-hint logic is extracted into lib/storage/error-hints.ts, and the circular dependency from the previous iteration is fully resolved. the toStorageError factory is now wired into the saveAccountsUnlocked call site, eliminating the dead-code concern from the last review.

key changes:

• StorageError now extends CodexError — upstream instanceof CodexError catches will no longer silently miss storage failures
• circular dependency (error-hints ↔ storage) eliminated — both files now import StorageError from errors.ts directly
• EEMPTY (not a real POSIX code) removed from the switch and its test deleted
• ENOENT hint added — correct guidance for missing parent directory, though the hint is not yet Windows-aware (unlike every other platform-sensitive branch) and has no vitest test coverage for the new branch

Confidence Score: 4/5

• safe to merge — refactor is structurally sound with two minor follow-ups needed before the next storage-split slice
• all four previously flagged issues (circular dep, wrong base class, EEMPTY phantom code, dead toStorageError) are resolved correctly. the two remaining gaps — ENOENT hint not Windows-aware, and missing vitest branch coverage for ENOENT — are style/coverage issues, not runtime blockers. no concurrency or token-safety regressions introduced.
• lib/storage/error-hints.ts — ENOENT hint needs a Windows-aware variant and a corresponding vitest test in test/storage.test.ts

Important Files Changed

Filename	Overview
lib/errors.ts	StorageError correctly moved here and now extends CodexError — hierarchy fix complete, code field inherited, path and hint added as own properties.
lib/storage/error-hints.ts	new module cleanly breaks the cycle; ENOENT branch added but hint is not Windows-aware (missing platform split), and the branch has no vitest test coverage.
lib/storage.ts	call site in saveAccountsUnlocked correctly uses toStorageError; re-exports StorageError and formatStorageErrorHint from their new canonical locations; formatting-only changes elsewhere are clean.
test/storage.test.ts	new toStorageError test added and EEMPTY test correctly removed; ENOENT hint branch added in production but not tested here.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[saveAccountsUnlocked] -->|catch error| B[toStorageError]
    B --> C[extractErrorCode]
    B --> D[formatStorageErrorHint]
    C -->|errno string| D
    D -->|switch on code| E{code?}
    E -->|EACCES / EPERM| F[Windows: antivirus hint\nUnix: chmod hint]
    E -->|EBUSY| G[file-lock hint]
    E -->|ENOENT| H[missing-path hint\n⚠️ not Windows-aware]
    E -->|ENOSPC| I[disk-full hint]
    E -->|default| J[Windows: special-chars hint\nUnix: permissions hint]
    D -->|hint string| B
    B -->|StorageError| A
    A -->|throw StorageError| K[caller]

    subgraph errors.ts
        L[CodexError] --> M[StorageError]
    end

    B -.->|new StorageError| M

Loading

Comments Outside Diff (1)

1. test/storage.test.ts, line 14-30 (link)

   duplicate formatStorageErrorHint import

   formatStorageErrorHint is imported twice in the same scope: once via the ../lib/storage.js barrel (line 14) and again directly from ../lib/storage/error-hints.js (line 30). TypeScript will surface this as a duplicate identifier error and the file won't compile.

   remove the redundant import on line 30 — the barrel re-export on line 14 already gives you the same value:

   actually, drop line 14's formatStorageErrorHint from the barrel import and keep only the direct import on line 30, or vice versa. e.g.:

   import {
   	buildNamedBackupPath,
   	clearAccounts,
   	deduplicateAccounts,
   	deduplicateAccountsByEmail,
   	exportAccounts,
   	exportNamedBackup,
   	findMatchingAccountIndex,
   	getFlaggedAccountsPath,
   	getStoragePath,
   	importAccounts,
   	loadAccounts,
   	loadFlaggedAccounts,
   	normalizeAccountStorage,
   	resolveAccountSelectionIndex,
   	saveFlaggedAccounts,
   	StorageError,
   	saveAccounts,
   	setStoragePath,
   	setStoragePathDirect,
   	withAccountAndFlaggedStorageTransaction,
   	withAccountStorageTransaction,
   } from "../lib/storage.js";
   import { formatStorageErrorHint } from "../lib/storage/error-hints.js";
   

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: test/storage.test.ts
   Line: 14-30
   
   Comment:
   **duplicate `formatStorageErrorHint` import**
   
   `formatStorageErrorHint` is imported twice in the same scope: once via the `../lib/storage.js` barrel (line 14) and again directly from `../lib/storage/error-hints.js` (line 30). TypeScript will surface this as a duplicate identifier error and the file won't compile.
   
   remove the redundant import on line 30 — the barrel re-export on line 14 already gives you the same value:
   
   
   
   actually, drop line 14's `formatStorageErrorHint` from the barrel import and keep only the direct import on line 30, or vice versa. e.g.:
   
   ```
   import {
   	buildNamedBackupPath,
   	clearAccounts,
   	deduplicateAccounts,
   	deduplicateAccountsByEmail,
   	exportAccounts,
   	exportNamedBackup,
   	findMatchingAccountIndex,
   	getFlaggedAccountsPath,
   	getStoragePath,
   	importAccounts,
   	loadAccounts,
   	loadFlaggedAccounts,
   	normalizeAccountStorage,
   	resolveAccountSelectionIndex,
   	saveFlaggedAccounts,
   	StorageError,
   	saveAccounts,
   	setStoragePath,
   	setStoragePathDirect,
   	withAccountAndFlaggedStorageTransaction,
   	withAccountStorageTransaction,
   } from "../lib/storage.js";
   import { formatStorageErrorHint } from "../lib/storage/error-hints.js";
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

   

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/storage/error-hints.ts
Line: 23-24

Comment:
**`ENOENT` hint is not Windows-aware**

every other platform-sensitive branch has a Windows variant — `EACCES`/`EPERM` returns an antivirus-focused message on win32, and the default branch warns about special characters in paths. on Windows, `ENOENT` for a missing `.codex` directory is frequently caused by path-length limits (>260 chars when `LongPathsEnabled` is off) or by UNC path normalisation issues in worktrees — both cases where "create the parent folder" is incorrect guidance. worth adding a `isWindows` split here the same way the other branches do:

```suggestion
		case "ENOENT":
			return isWindows
				? `Path does not exist: ${path}. Create the parent folder or check for path-length limits (enable LongPathsEnabled in Windows registry if the path exceeds 260 characters).`
				: `Path does not exist: ${path}. Create the parent folder and try again.`;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/storage.test.ts
Line: 1239-1240

Comment:
**missing vitest coverage for `ENOENT` hint**

the `ENOENT` branch was added to `formatStorageErrorHint` in this PR but has no corresponding test case. every other error code (`EACCES`, `EPERM`, `EBUSY`, `ENOSPC`) has an explicit assertion; `ENOENT` should too. the repo targets 80%+ branch coverage — this is a new uncovered branch. add a test alongside the others:

```suggestion
			it("should return disk full hint for ENOSPC", () => {
				const err = { code: "ENOSPC" } as NodeJS.ErrnoException;
				const hint = formatStorageErrorHint(err, testPath);

				expect(hint).toContain("Disk is full");
			});

			it("should return missing-path hint for ENOENT", () => {
				const err = { code: "ENOENT" } as NodeJS.ErrnoException;
				const hint = formatStorageErrorHint(err, testPath);

				expect(hint).toContain("does not exist");
				expect(hint).toContain(testPath);
			});

```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "docs: describe stora..."}

[coderabbitai]

📝 Walkthrough

Walkthrough

A new StorageError class was introduced with structured error metadata (code, path, hint), along with utility functions to convert raw filesystem errors into user-facing storage error instances with contextual hints.

Changes

Cohort / File(s)	Summary
Error class definition lib/errors.ts	Added StorageError class extending Error with constructor taking message, code, path, hint, and optional cause; instance properties expose code, path, and hint as read-only strings.
Error formatting utilities lib/storage/error-hints.ts	Introduced formatStorageErrorHint(error, path) to convert filesystem errors to user-facing messages (handles EACCES/EPERM, EBUSY, ENOSPC, EEMPTY cases) and toStorageError(message, error, path) to build StorageError instances with derived codes and formatted hints.
Storage module refactoring lib/storage.ts	Refactored to import StorageError from lib/errors.ts and formatStorageErrorHint from lib/storage/error-hints.ts, then re-export both. Minor formatting adjustments to debug logs and error throws; no behavioral changes.
Test coverage test/storage.test.ts	Added test case validating toStorageError() correctly wraps EBUSY errors with appropriate code, path, hint containing "File is locked", and preserves original error as cause.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Issues flagged

missing regression tests: test case only covers EBUSY. what about coverage for EACCES, EPERM, ENOSPC, EEMPTY, and the "UNKNOWN" fallback at lib/storage/error-hints.ts:16? each errno branch should have a test.

windows edge cases: errno codes differ on windows (e.g., EACCES maps differently). verify that formatStorageErrorHint() gracefully handles windows-specific error codes, or document the platform limitation.

concurrency risks: toStorageError() at lib/storage/error-hints.ts:30 accepts error: unknown. if called from concurrent storage operations, ensure the error object capture is sound and no race conditions exist when accessing error properties.

Suggested labels

bug

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	PR description is largely incomplete against the template. Missing required validation checkmarks, docs/governance checklist entirely absent, and critical blockers noted by greptile not addressed.	complete the validation checklist (explicitly mark which checks passed/failed), add the docs/governance checklist with appropriate marks, and address the three greptile-flagged blockers: (1) duplicate formatStorageErrorHint import in test/storage.test.ts:14-30, (2) ENOENT hint missing Windows-aware variant in lib/storage/error-hints.ts:23-24, (3) ENOENT branch lacks vitest coverage. these must be resolved before merge.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commit format with type 'refactor' and lowercase imperative summary under 72 chars; accurately describes the main change (extracting error hint formatting).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/pr3-storage-error-helpers

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch refactor/pr3-storage-error-helpers

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

Actionable comments posted: 1

🤖 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/storage/error-hints.ts`:
- Around line 27-41: Extract the duplicated code that reads err?.code ||
"UNKNOWN" into a small helper named extractErrorCode(error: unknown): string,
implement it to cast to NodeJS.ErrnoException and return the code or "UNKNOWN",
then replace the inline duplication in toStorageError and the similar logic in
formatStorageErrorHint (or any other occurrences) to call
extractErrorCode(error) so the error-code extraction is centralized and DRY.

🪄 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: 84d89c77-3b3d-4369-8628-b23f7c2994a0

📥 Commits

Reviewing files that changed from the base of the PR and between f63e949 and a7b3cfb.

📒 Files selected for processing (4)

• lib/errors.ts
• lib/storage.ts
• lib/storage/error-hints.ts
• test/storage.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/storage.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/errors.ts
• lib/storage/error-hints.ts
• lib/storage.ts

🔇 Additional comments (9)

test/storage.test.ts (2)

30-30: direct import from internal module is fine here, but note it's not re-exported from the barrel.

toStorageError is imported from lib/storage/error-hints.js directly while formatStorageErrorHint comes from the barrel at lib/storage.js:19. if toStorageError is intended as internal-only, this is correct. if it should be public api, consider re-exporting it from lib/storage.ts.

---

82-96: lgtm. good coverage for ebusy/windows file lock scenario.

test validates toStorageError wraps the cause correctly and preserves error metadata. this aligns with the concurrency/windows filesystem requirements in lib/storage.ts retry paths.

lib/errors.ts (1)

167-189: lgtm. storageerror follows the same pattern as other error classes in this file.

error chaining via { cause } is correct. readonly properties prevent accidental mutation. no sensitive data (tokens/emails) stored in the error structure.

lib/storage/error-hints.ts (2)

1-1: circular dependency correctly broken by importing from errors.js instead of storage.js.

the pr description flagged a cycle between error-hints.ts → storage.ts → error-hints.ts. importing StorageError from lib/errors.ts:171 avoids that issue.

---

3-25: lgtm. platform-aware error hints with ebusy handling.

ebusy at lib/storage/error-hints.ts:14-15 provides actionable guidance for windows file lock scenarios. no tokens or emails leak into the hint strings.

lib/storage.ts (4)

6-6: lgtm. re-exports maintain public api surface after extraction.

StorageError and formatStorageErrorHint are re-exported so existing consumers importing from lib/storage.ts are unaffected. toStorageError is intentionally not re-exported, keeping it internal to the error-hints module.

Also applies to: 15-19

---

132-199: formatting changes only. no logic changes detected.

these are log.debug formatting adjustments. verified no tokens or emails appear in the logged fields (candidatePath, fileName, mtime, error.message, error.stack).

---

1300-1303: minor formatting tweak in selectnewestaccount call.

no logic change. matchingAccount ?? undefined is redundant since matchingAccount is already T | null, but the explicit fallback doesn't hurt.

---

2087-2089: formatting only. transaction rollback logic unchanged.

verified the account storage rollback path at lib/storage.ts:2096-2113 still handles flagged save failures correctly with aggregate error wrapping.

[ndycode]

Closing because this work is now included in main via #318 and #319.