carry forward remaining unique non-dev PR work

[ndycode]

Summary

• carry forward the remaining graph-unique non-dev work that is not already included in main
• replay and revalidate the surviving patches on top of fresh main
• keep this as the single aggregate follow-up after #318

Source PRs included

• #250
• #251
• #252
• #253
• #254
• #269
• #273
• #278
• #283
• #285
• #290
• #293

Audit note

• #301 was graph-unique during audit but replayed patch-empty against current main, so it is not carried here

Local validation

• npm run clean:repo:check
• npm run typecheck
• npm run build
• npm run lint
• npx vitest run test/settings-persist-utils.test.ts test/account-snapshot.test.ts test/storage-flagged.test.ts test/account-save.test.ts test/account-clear-entry.test.ts test/restore-backup-entry.test.ts test/settings-panels.test.ts test/unified-settings-entry.test.ts test/dashboard-formatters.test.ts test/account-port.test.ts test/named-backups-entry.test.ts

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 carries forward 12 source prs worth of non-dev work onto a fresh main baseline — primarily windows filesystem resilience improvements, type-level refactoring, and test coverage expansion across the storage and settings layers. the changes are well-scoped and all locally validated.

key changes:

• account-save.ts: removes the silent try/catch that swallowed loadExistingStorage() probe errors — transient windows EBUSY hits during the synthetic-fixture safety check now fail the save via createStorageError instead of proceeding blindly
• account-snapshot.ts: EBUSY/EPERM on stat now returns { exists: true } (no bytes/mtimeMs), triggering a 2-attempt metadata retry after load succeeds; persistent locks fall back to bytes: 0, mtimeMs: 0
• flagged-storage-file.ts: adds readFileWithRetry with EBUSY/EAGAIN retry set (3 retries, 10 ms base exponential) before parsing flagged account files
• settings-write-queue.ts: bumps SETTINGS_WRITE_MAX_DELAY_MS from 1 s → 30 s — has no effect on pure filesystem retries (peak backoff ~400 ms) but allows honoring larger http 429 retry-after values for remote settings sync
• experimental-sync-target-entry.ts: narrows retryable read codes — drops ENOTEMPTY (irrelevant for reads) and EACCES (covered by EPERM for windows antivirus locks)
• type refactoring: UnifiedSettingsControllerDeps extracted as a named type; SnapshotStats moved to backup-metadata.ts; duplicate saveFlaggedAccountsEntry/clearFlaggedAccountsEntry copies removed from account-port.ts
• test coverage: new test files for account-save, account-snapshot, dashboard-formatters, and settings-persist-utils; existing suites extended with concurrency, windows-path, and lock-recovery scenarios

Confidence Score: 5/5

• safe to merge — no logic bugs found, all behavioral changes are intentional and well-tested, windows filesystem safety is improved overall
• all p2 comments are tradeoff observations, not defects. the probe-failure change in account-save.ts is semantically correct and the new behavior is tested. the 30 s cap only activates for http 429 with large retry-after, not filesystem retries. test coverage for new behaviors is thorough. concurrency and windows-path tests added. flagged-save-entry.ts verified to exist. no missing required files.
• no files require special attention — the three p2 comments are informational tradeoff notes, not blocking issues

Important Files Changed

Filename	Overview
lib/codex-manager/settings-write-queue.ts	bumps SETTINGS_WRITE_MAX_DELAY_MS from 1 s → 30 s; has no effect on pure filesystem retries (peak backoff 400 ms) but allows honoring large http 429 retry-after values — could stall interactive cli in extreme cases
lib/codex-manager/experimental-sync-target-entry.ts	removes ENOTEMPTY and EACCES from sync-target read retry set; ENOTEMPTY is correct to drop (not relevant for reads), EACCES removal is defensible since EPERM is still present
lib/storage/account-save.ts	removes silent error-swallowing around loadExistingStorage() probe — transient windows locks during probe now abort the save via createStorageError instead of proceeding blindly; safer but may surface new transient failures on windows
lib/storage/account-snapshot.ts	EBUSY/EPERM on stat now returns exists:true with no bytes/mtimeMs, triggering a 2-attempt retry loop after load succeeds; fallback to 0,0 for persistently locked files is reasonable; logWarn made required
lib/storage/flagged-storage-file.ts	adds local readFileWithRetry with EBUSY/EAGAIN retry set (3 retries, 10ms base exponential); correctly excludes EPERM as non-retryable on read; well-tested in storage-flagged.test.ts
lib/storage/flagged-entry.ts	saveFlaggedAccountsEntry now re-exported from flagged-save-entry.ts (file verified to exist); clearFlaggedAccountsEntry remains directly in this module; duplicate copies in account-port.ts correctly removed
lib/storage/account-port.ts	removes saveFlaggedAccountsEntry and clearFlaggedAccountsEntry duplicates — both already existed in flagged-entry.ts/flagged-save-entry.ts; corresponding account-port.test.ts cases also removed
test/account-snapshot.test.ts	new test file; covers statSnapshot (success, ENOENT, EACCES, EBUSY, EPERM) and describeAccountSnapshot (valid, invalid, loader failure, ENOENT race, stat-lock retry, repeated-lock fallback) — thorough windows-lock coverage
test/account-save.test.ts	new test file; covers probe-failure propagation and synthetic-fixture overwrite refusal — directly validates the account-save.ts error handling change
test/account-clear-entry.test.ts	adds concurrent-clear serialization test (via gated Promise queue) and windows-path preservation test — good concurrency coverage

Sequence Diagram

sequenceDiagram
    participant Caller
    participant describeAccountSnapshot
    participant statSnapshot
    participant loadAccountsFromPath

    Caller->>describeAccountSnapshot: (path, kind, deps)
    describeAccountSnapshot->>statSnapshot: stat(path)
    alt EBUSY/EPERM (windows lock)
        statSnapshot-->>describeAccountSnapshot: { exists: true } (no bytes/mtimeMs)
    else success
        statSnapshot-->>describeAccountSnapshot: { exists: true, bytes, mtimeMs }
    else ENOENT
        statSnapshot-->>describeAccountSnapshot: { exists: false }
        describeAccountSnapshot-->>Caller: { exists: false, valid: false }
    end
    describeAccountSnapshot->>loadAccountsFromPath: load(path)
    loadAccountsFromPath-->>describeAccountSnapshot: { normalized, schemaErrors, storedVersion }
    loop up to 2 retries (if bytes/mtimeMs still missing)
        describeAccountSnapshot->>statSnapshot: stat(path) retry
        statSnapshot-->>describeAccountSnapshot: refreshedStats
    end
    note over describeAccountSnapshot: bytes ?? 0, mtimeMs ?? 0 (locked fallback)
    describeAccountSnapshot-->>Caller: BackupSnapshotMetadata (valid/invalid)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/storage/account-save.ts
Line: 40-48

Comment:
**probe failures now abort saves on windows transient locks**

the old code caught all errors from `loadExistingStorage()` and only re-threw synthetic ones, which meant a transient `EBUSY`/`EPERM` hit during the probe silently let the write proceed. the new code lets those propagate to the outer `catch` and wraps them via `createStorageError`, aborting the save.

this is semantically safer — we don't want to overwrite live storage we couldn't probe — but on windows, antivirus-induced `EBUSY` hits on the probe path will now fail the entire save instead of proceeding. callers that rely on `loadExistingStorage` returning `null` for `ENOENT` (which is the correct contract) are fine; the risk is implementations that throw for transient errors instead of returning `null`.

worth adding a note in `loadExistingStorage`'s contract or adding a retry wrapper inside it (like the `readFileWithRetry` pattern used in `flagged-storage-file.ts`) if transient probe failures on windows become a reliability issue in practice.

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

---

This is a comment left during a code review.
Path: lib/codex-manager/settings-write-queue.ts
Line: 3

Comment:
**30s cap only matters for http 429 retry-after, not filesystem retries**

with `SETTINGS_WRITE_BASE_DELAY_MS = 50` and `maxAttempts = 4`, pure exponential backoff peaks at `50 * 2^3 = 400 ms`, well below both the old 1 s and new 30 s caps. so this change only affects cases where `getRetryAfterMs(error)` returns a value greater than 1 000 ms — i.e. http 429 responses from a remote settings sync.

honoring a longer `retry-after` is correct. however, a server that sends `retryAfterMs: 30000` can now block the per-path write queue for ~60 s (two retries × 30 s each before the 4th attempt throws). for interactive cli settings saves that would be a very bad ux stall. consider capping the honor-retry-after value lower (e.g. 5 s) if these writes also come from interactive panels, or document that `withQueuedRetry` is only called from non-interactive paths (background sync).

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

---

This is a comment left during a code review.
Path: lib/codex-manager/experimental-sync-target-entry.ts
Line: 51-55

Comment:
**`EACCES` removed from sync-target read retry — windows antivirus consideration**

`ENOTEMPTY` is irrelevant for reads (directory-removal code), so removing it makes sense. `EACCES` however can be transient on windows when antivirus briefly locks a file during an index pass; the AGENTS.md calls out windows filesystem safety as a primary concern for this repo.

in practice most windows antivirus locks surface as `EBUSY` or `EPERM`, both still present here, so the risk is low. but the project convention (AGENTS.md) is to be conservative with filesystem error codes. a comment explaining why `EACCES` is intentionally excluded would make the decision auditable for future maintainers.

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

_{Reviews (1): Last reviewed commit: "Fix storage import after unique replay" | Re-trigger Greptile}

[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]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 738f23ae-0d8d-4c60-8438-c582cb24618a

📥 Commits

Reviewing files that changed from the base of the PR and between e5ea183 and a6e42ae.

📒 Files selected for processing (21)

• lib/codex-manager/experimental-sync-target-entry.ts
• lib/codex-manager/settings-write-queue.ts
• lib/codex-manager/unified-settings-controller.ts
• lib/codex-manager/unified-settings-entry.ts
• lib/storage/account-port.ts
• lib/storage/account-save.ts
• lib/storage/account-snapshot.ts
• lib/storage/backup-metadata.ts
• lib/storage/flagged-entry.ts
• lib/storage/flagged-storage-file.ts
• lib/storage/snapshot-inspectors.ts
• test/account-clear-entry.test.ts
• test/account-port.test.ts
• test/account-save.test.ts
• test/account-snapshot.test.ts
• test/dashboard-formatters.test.ts
• test/restore-backup-entry.test.ts
• test/settings-panels.test.ts
• test/settings-persist-utils.test.ts
• test/storage-flagged.test.ts
• test/unified-settings-entry.test.ts

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

---

📝 Walkthrough

Summary

This PR carries forward 97 commits of graph-unique, non-dev changes aggregated from PRs #250–#293, hardening settings persistence, retry semantics, and snapshot inspection logic with comprehensive test coverage. The changes are low-risk as they consist primarily of internal refactoring, dependency injection improvements, and enhanced error handling—with new tests adding ~470 lines of coverage for retry behavior, synthetic fixture detection, and snapshot metadata resolution. However, two behavioral modifications warrant careful review: (1) snapshot inspection now treats locked files (EBUSY/EPERM) as existing rather than missing, and (2) synthetic fixture error detection no longer wraps loadExistingStorage in try/catch, potentially narrowing error interception.

Key Changes

Retry & Error Handling:

• flagged-storage-file.ts: Adds readFileWithRetry helper with exponential backoff (3 attempts, 10/20/40ms) for transient lock codes (EBUSY, EAGAIN).
• experimental-sync-target-entry.ts: Removes ENOTEMPTY and EACCES from retryable error codes, leaving only the surrounding retry infrastructure.
• account-snapshot.ts: Locked files (EBUSY/EPERM) now treated as existing ({ exists: true }) and logged as warnings, changing snapshot visibility semantics.
• settings-write-queue.ts: Increases max retry delay from 1s to 30s for exponential backoff and retryAfterMs-based delays.

Architecture & Dependencies:

• Extracts UnifiedSettingsControllerDeps type (48+ lines) from unified-settings-controller.ts and shares it with unified-settings-entry.ts, eliminating inline object type duplication.
• account-port.ts: Removes exported saveFlaggedAccountsEntry and clearFlaggedAccountsEntry (consolidating into flagged-entry.ts and flagged-save-entry.js).
• backup-metadata.ts: Introduces SnapshotStats type (exists, optional bytes/mtimeMs) for centralized snapshot metadata shape.

Storage & Snapshot Inspection:

• account-save.ts: Removes try/catch around loadExistingStorage, now directly throwing synthetic-fixture error if loaded storage contains non-empty accounts, narrowing exception handling.
• account-snapshot.ts: Makes deps.logWarn required (no optional chaining); adds metadata refresh logic to re-stat when bytes or mtimeMs are undefined.

Test Coverage:

• New test files: settings-persist-utils.test.ts (87 lines covering readFileWithRetry backoff), account-snapshot.test.ts (271 lines for locked-file and metadata-refresh scenarios), account-save.test.ts (83 lines for probe/synthetic errors), storage-flagged.test.ts (93 lines for retry sequencing), dashboard-formatters.test.ts (33 lines), account-clear-entry.test.ts (+92 lines), restore-backup-entry.test.ts (+47 lines), settings-panels.test.ts (+155 lines).
• Enhanced test helpers to verify exact call counts, backoff durations, and error propagation paths.

Risks & Considerations

No security or data-loss risks identified. Existing tests pass local validation (clean, typecheck, build, lint, vitest). Behavioral changes to locked-file handling and synthetic-fixture detection are well-covered by new assertions, though snapshot visibility semantics shift (locked files now visible vs. previously absent). The removal of exception wrapping in account-save.ts is narrower in scope but should be validated against existing prod deployment behavior.

Walkthrough

refactored account storage and settings management: removed flagged-account port functions, enhanced snapshot inspection with retry-friendly stat logic and optional log warnings, reorganized settings controller dependencies into shared type, adjusted delay cap and retry codes. test coverage added across snapshot inspection, account persistence, and settings panel logic.

Changes

Cohort / File(s)	Summary
Settings configuration and delays lib/codex-manager/experimental-sync-target-entry.ts, lib/codex-manager/settings-write-queue.ts, lib/codex-manager/unified-settings-controller.ts, lib/codex-manager/unified-settings-entry.ts	Extracted UnifiedSettingsControllerDeps type to standardize settings controller dependencies across modules. Increased SETTINGS_WRITE_MAX_DELAY_MS from 1_000 to 30_000 (30× increase affecting retry backoff ceiling). Removed ENOTEMPTY and EACCES from retryable codes in experimental-sync-target-entry.
Flagged account storage refactor lib/storage/account-port.ts, lib/storage/flagged-entry.ts, lib/storage/flagged-storage-file.ts	Removed saveFlaggedAccountsEntry and clearFlaggedAccountsEntry exports from account-port; re-exported saveFlaggedAccountsEntry from new flagged-save-entry module. Added readFileWithRetry helper to flagged-storage-file with exponential backoff for EBUSY/EAGAIN (3 attempts, sleep param optional).
Account persistence and error handling lib/storage/account-save.ts	Removed try/catch wrapper around synthetic fixture storage detection, now propagates load errors directly instead of rethrowing selectively by error message content.
Account snapshot inspection and types lib/storage/backup-metadata.ts, lib/storage/account-snapshot.ts, lib/storage/snapshot-inspectors.ts	Added SnapshotStats type with optional bytes and mtimeMs. Made logWarn required (no longer optional) in snapshot dependencies. Enhanced statSnapshot to emit warning and return exists: true on EBUSY/EPERM stat failures. Added conditional re-stat retry (up to 3 attempts) in describeAccountSnapshot when metadata values undefined; defaults missing stats to 0.
Account persistence tests test/account-save.test.ts, test/account-snapshot.test.ts, test/account-clear-entry.test.ts	New test suite for saveAccountsToDisk error paths (probe failures, synthetic fixture detection). New suite for statSnapshot (lock handling, missing files) and describeAccountSnapshot (re-stat on transient locks, valid/invalid snapshots, loader failures). Enhanced account-clear tests with concurrency serialization assertions via queued withStorageLock mock and Windows path passthrough verification.
Account import/export tests test/account-port.test.ts, test/restore-backup-entry.test.ts, test/storage-flagged.test.ts	Removed tests for deleted saveFlaggedAccountsEntry and clearFlaggedAccountsEntry. Enhanced restore-backup-entry with realpath dependency injection and Windows path handling. New flagged-storage tests for loadFlaggedAccountsFromFile retry logic and describeFlaggedSnapshot failure paths.
Settings and dashboard tests test/settings-persist-utils.test.ts, test/settings-panels.test.ts, test/dashboard-formatters.test.ts, test/unified-settings-entry.test.ts	New suite for settings-persist-utils covering env var resolution, error formatting, and readFileWithRetry backoff sequencing. Enhanced settings-panels tests with dependency forwarding assertions for all prompt entry points and default value fallbacks. New dashboard-formatters test covering toggle/label/TTL formatting. Enhanced unified-settings-entry tests with controllerDeps helper and error propagation assertions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

---

critical observations

account-save.ts:line 7–13 — the try/catch removal is a semantic shift: errors from loadExistingStorage() now propagate uncaught instead of selectively rethrowing by message content. the new test suite (test/account-save.test.ts) covers both the probe error and synthetic fixture paths, which is solid, but verify downstream callers expect this error flow change.

account-snapshot.ts:line 40–50 — describeAccountSnapshot now re-stats up to 3 times on transient locks (EBUSY/EPERM). this could cause measurable latency if a file is repeatedly locked during inspection. the test (test/account-snapshot.test.ts) validates the retry sequencing and fallback to { bytes: 0, mtimeMs: 0 }, but monitor for user-visible delays in snapshot loading.

settings-write-queue.ts:line 1 — SETTINGS_WRITE_MAX_DELAY_MS increased 30× from 1_000 to 30_000. this is a significant timeout ceiling change for retry backoff. verify that settings writes don't stall UI interactions or block other critical operations for up to 30 seconds.

account-clear-entry.test.ts:line 15–25 — new test validates concurrent clearAccountsEntry calls are serialized via withStorageLock. good coverage, but ensure the actual clearAccountsEntry implementation (not shown in diff) enforces this serialization correctly.

restore-backup-entry.test.ts — windows path handling now tested with realpath injection. verify that the rest of the backup restore pipeline (not shown) handles Windows paths through the full round trip without normalization surprises.

flagged-storage-file.ts:line 5–20 — readFileWithRetry retries only on {"EBUSY","EAGAIN"} with exponential backoff (10, 20, 40 ms). confirm this retry window is appropriate for typical windows file lock durations in your deployment.

unified-settings-entry.ts + unified-settings-controller.ts — type extraction looks clean, but verify that all callers of configureUnifiedSettingsEntry pass the full UnifiedSettingsControllerDeps shape correctly. the intersection type { configureUnifiedSettingsController: ... } & UnifiedSettingsControllerDeps is slightly redundant; consider whether a simpler struct definition would improve clarity.

test coverage gaps — new test files are comprehensive and use fakes/mocks well. no obvious coverage gaps, though the removed saveFlaggedAccountsEntry and clearFlaggedAccountsEntry functions suggest a related module (flagged-save-entry.js) should exist; confirm it's in the codebase and tested separately.

✨ 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 audit/nondev-unique-20260323

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch audit/nondev-unique-20260323

---

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.}