feat(sync): split oc backend primitives#60
feat(sync): split oc backend primitives#60ndycode wants to merge 12 commits intosplit/pr55-cli-settings-harnessfrom
Conversation
📝 WalkthroughWalkthroughadds three new modules for oc-chatgpt multi-auth: named backup export with path validation, account import/merge with deduplication, and target detection. updates storage api to expose named backups. includes test coverage for all features. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Backup as Named Backup Export
participant Storage as Storage Module
participant FS as File System
Client->>Backup: exportNamedBackupFile(name, deps)
Backup->>Backup: normalizeNamedBackupFileName(name)
Backup->>Backup: getNamedBackupRoot(storagePath)
Backup->>Backup: resolveNamedBackupPath(name, storagePath)
Backup->>Backup: assertWithinDirectory(target, root)
Backup->>Storage: exportAccounts(filePath, force)
Storage->>FS: write backup file
FS-->>Storage: file created
Storage-->>Backup: Promise<void>
Backup-->>Client: resolved backup path
sequenceDiagram
actor Client
participant Preview as Import Adapter
participant Normalize as Normalization
participant Match as Matching
participant Result as Result Builder
Client->>Preview: previewOcChatgptImportMerge({source, destination})
Preview->>Normalize: normalizeAccountsForTarget(source)
Normalize-->>Preview: normalized source + skipped
Preview->>Normalize: normalizeAccountsForTarget(destination)
Normalize-->>Preview: normalized destination + skipped
Preview->>Match: matchDestination(account, normalized dest)
Match-->>Preview: match result
Preview->>Result: buildOcChatgptImportPayload(source)
Result-->>Preview: OcChatgptMergePreview
Result-->>Client: toAdd, toUpdate, toSkip, unchanged
sequenceDiagram
actor Client
participant Detector as Target Detection
participant CandidateResolver as Candidate Resolution
participant Prober as Artifact Prober
Client->>Detector: detectOcChatgptMultiAuthTarget({explicitRoot?, projectRoot?})
Detector->>CandidateResolver: buildCandidateRoots(explicit, canonical, project)
CandidateResolver-->>Detector: ordered candidates
Detector->>Prober: probePhase(candidates, "accounts")
Prober-->>Detector: qualified candidates with artifacts
Detector->>Prober: probePhase(candidates, "signals")
Prober-->>Detector: candidates with backup signals
Detector-->>Client: OcChatgptTargetDetectionResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes flagspath traversal & security: windows edge cases: target detection probes filesystem artifacts without explicit windows-path-normalization tests. account deduplication logic: missing regression tests: concurrency risk: 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
5 issues found across 8 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/oc-chatgpt-target-detection.ts">
<violation number="1" location="lib/oc-chatgpt-target-detection.ts:110">
P2: Do not strip the trailing separator from Windows drive roots; converting `C:\\` to `C:` makes root directory scans drive-relative.</violation>
<violation number="2" location="lib/oc-chatgpt-target-detection.ts:202">
P2: Compare against a normalized explicit root; trailing separators currently cause explicit targets to be misclassified as non-explicit.</violation>
</file>
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1">
P2: Use `removeWithRetry` for test cleanup instead of bare `fs.rm` to avoid flaky EBUSY/EPERM/ENOTEMPTY failures on Windows.</violation>
<violation number="2" location="test/storage.test.ts:1">
P2: This placeholder assertion does not test filename migration, so regressions in migration logic will go undetected.</violation>
</file>
<file name="lib/oc-chatgpt-import-adapter.ts">
<violation number="1" location="lib/oc-chatgpt-import-adapter.ts:154">
P1: `activeIndexByFamily` is clamped after deduplication without key-based remapping, so family selection can point to the wrong account when dedup removes earlier entries.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | ||
| import { promises as fs, existsSync } from "node:fs"; | ||
| import { dirname, join } from "node:path"; | ||
| import { existsSync, promises as fs } from "node:fs"; |
There was a problem hiding this comment.
P2: This placeholder assertion does not test filename migration, so regressions in migration logic will go undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/storage.test.ts:
<comment>This placeholder assertion does not test filename migration, so regressions in migration logic will go undetected.</comment>
…d-primitives # Conflicts: # test/dashboard-settings.test.ts # test/unified-settings.test.ts
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="test/storage.test.ts">
<violation number="1">
P2: This test is outside `describe("storage")`, so it skips the suite’s setup/teardown and can touch the default storage path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/named-backup-export.ts`:
- Around line 59-63: The thrown error for invalid backup names doesn't mention
that dots are disallowed by BACKUP_SAFE_NAME_REGEX; update the error text thrown
where baseName is validated to explicitly state the allowed characters and that
dots are not permitted (e.g. "Backup filename may only contain letters, numbers,
hyphens, and underscores; dots (.) are not allowed"), or if dots should be
allowed instead, change BACKUP_SAFE_NAME_REGEX to include a literal dot (e.g.,
add '.' to the character class like /^[a-zA-Z0-9._-]+$/) and update any related
validation/tests accordingly.
In `@lib/oc-chatgpt-import-adapter.ts`:
- Around line 142-156: The function normalizeActiveIndexByFamily declares a dead
parameter defaultIndex that is never used; remove the unused defaultIndex
parameter from normalizeActiveIndexByFamily signature and adjust all call sites
that pass a defaultIndex (update their calls to drop that argument), or
alternatively use defaultIndex as a fallback by passing it into remapActiveIndex
when remapping fails (e.g., set normalized[family] =
remapActiveIndex(originalAccounts, raw, normalizedAccounts) ?? defaultIndex).
Choose one approach and make the corresponding change consistently: either
delete defaultIndex from the function and all callers, or wire defaultIndex into
the remapActiveIndex fallback logic inside normalizeActiveIndexByFamily.
In `@lib/oc-chatgpt-target-detection.ts`:
- Around line 182-184: inferScopeFromRoot currently treats any path containing
the token PROJECTS_DIR_NAME as project scope; instead normalize the path and
split into segments, then detect the canonical sequence where OPENCODE_DIR_NAME
is immediately followed by PROJECTS_DIR_NAME (i.e.,
".../.opencode/projects/...") — e.g., in inferScopeFromRoot call
path.normalize(root), split on path.sep and check for an index i where
segments[i] === OPENCODE_DIR_NAME && segments[i+1] === PROJECTS_DIR_NAME; return
"project" only in that case, otherwise "global".
In `@lib/storage.ts`:
- Around line 1572-1597: exportAccounts currently writes directly with
fs.writeFile which can corrupt exports on crash; change exportAccounts to use
the same atomic write pattern as saveAccounts: write JSON to a temp file in the
same directory (use dirname(resolvedPath) and a unique temp name), fsync the
temp file, set file mode 0o600, then rename the temp file to resolvedPath with a
retry/backoff loop and ensure cleanup of the temp file on failure; preserve
logging (log.info path and count) and reuse resolvePath and
withAccountStorageTransaction/ storage.accounts to generate the content before
performing the atomic write/rename.
- Around line 1328-1357: clearAccounts currently calls
getAccountsBackupRecoveryCandidatesWithDiscovery() while inside withStorageLock,
so a readdir error (EPERM/EBUSY on Windows) can abort before Promise.all runs
and block removal of the main accounts file; move discovery outside the lock by
invoking getAccountsBackupRecoveryCandidatesWithDiscovery(path) before entering
withStorageLock (or, alternatively, call it inside but wrap it in a try/catch
that logs the error and returns an empty array) so that backupPaths is always
defined and the Promise.all([...clearPath]) block inside clearAccounts (which
calls clearPath for path, walPath and backupPaths) still runs under the lock to
remove the main files; update references to backupPaths,
getAccountsBackupRecoveryCandidatesWithDiscovery, withStorageLock, Promise.all,
and clearPath accordingly.
In `@test/named-backup-export.test.ts`:
- Around line 89-116: Add a regression test to cover concurrent export races by
invoking exportNamedBackupFile twice in parallel for the same backup name and
asserting one succeeds while the other fails (to cover EBUSY/Windows semantics);
use Promise.allSettled to run two exportNamedBackupFile("concurrent-test", {
getStoragePath, exportAccounts }) calls after preparing state with saveAccounts,
then assert that at least one result has status "fulfilled" and the other is
rejected with an "already exists" or filesystem error; reference the existing
helpers saveAccounts, exportNamedBackupFile, getStoragePath, exportAccounts and
resolveNamedBackupPath to create the destination and verify behavior without
changing production code.
- Around line 131-159: The test currently validates exported content but doesn't
verify atomic writes; update the test (the spec using exportNamedBackupFile) to
assert atomic write behavior: after calling exportNamedBackupFile, confirm the
final resolved file
(join(dirname(storagePath),"backups","backup-2026-03-09.json")) contains the
exact expected JSON and also assert there are no leftover temp/partial files in
the backups directory (check for common temp patterns like *.tmp or a leading
dot/partial filename) to detect non-atomic writes, or if you prefer not to add
runtime checks, add a short test comment referencing lib/storage.ts
exportAccounts and document the known gap that it currently uses fs.writeFile
instead of temp-file-then-rename so reviewers know why atomicity isn't enforced
by the test.
In `@test/oc-chatgpt-import-adapter.test.ts`:
- Around line 291-319: The inline storage objects `destination` and `source` in
the test should be explicitly typed as AccountStorageV3 to avoid implicit type
inference and catch schema drift; update the test to annotate those constants
(the objects passed into previewOcChatgptImportMerge) with the AccountStorageV3
type so the compiler will validate shape and fields when calling
previewOcChatgptImportMerge and when asserting preview results.
In `@test/oc-chatgpt-target-detection.test.ts`:
- Around line 81-104: Add a regression test that ensures
detectOcChatgptMultiAuthTarget (which uses inferScopeFromRoot) does not
misclassify a global target when the user HOME contains a "projects" path
segment: create a fake home directory path containing "projects" (e.g.,
workDir/home/user/my-projects/nested), set process.env.HOME and
process.env.USERPROFILE to it, create a global ~/.opencode with
openai-codex-accounts.json inside, call detectOcChatgptMultiAuthTarget() with no
projectRoot, and assert the returned target has kind "target" and
descriptor.scope "global" (or equivalent assertions used elsewhere in the test
file) to reproduce and prevent the inferScopeFromRoot bug.
- Around line 14-30: The helper function removeWithRetry is duplicated in tests;
extract it into a shared test helper module (e.g., export async function
removeWithRetry(...) from test/helpers/fs-retry.ts) preserving the same
signature and retry logic, update the tests that currently declare
removeWithRetry to import it from that shared module, and remove the duplicate
local implementations so both tests use the single exported removeWithRetry
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23e73eea-60b6-4c0f-a853-c6c9f9c3b330
📒 Files selected for processing (8)
lib/named-backup-export.tslib/oc-chatgpt-import-adapter.tslib/oc-chatgpt-target-detection.tslib/storage.tstest/named-backup-export.test.tstest/oc-chatgpt-import-adapter.test.tstest/oc-chatgpt-target-detection.test.tstest/storage.test.ts
📜 Review details
🧰 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/oc-chatgpt-import-adapter.test.tstest/oc-chatgpt-target-detection.test.tstest/named-backup-export.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/storage.tslib/named-backup-export.tslib/oc-chatgpt-target-detection.tslib/oc-chatgpt-import-adapter.ts
🔇 Additional comments (14)
test/named-backup-export.test.ts (1)
17-33: solid windows-safe cleanup helper.good use of exponential backoff with retryable error codes for windows filesystem edge cases. covers
EBUSY,EPERM,ENOTEMPTY,EACCES.lib/storage.ts (3)
1593-1596: log statement exposes account count but not tokens - looks safe.
log.info("Exported accounts", { path: resolvedPath, count: storage.accounts.length })doesn't leak tokens or emails. good.
518-534: exportNamedBackup wiring looks correct.correctly passes
getStoragePathandexportAccountsas dependencies toexportNamedBackupFile, forwarding the force option.
760-767: normalizeEmailKey exported for reuse - good.clean extraction for email normalization, handles trim and lowercase consistently.
test/oc-chatgpt-import-adapter.test.ts (2)
1-70: solid test coverage for import payload normalization.tests correctly validate:
- filtering invalid accounts (empty refresh tokens)
- identity-rich duplicate preservation by accountId
- email fallback for id-less entries
deterministic and uses vitest as required.
72-202: comprehensive merge preview test with exact shape assertions.good validation of:
preserve-destinationactive selection behaviormatchedBystrategies (accountId, email)toAdd,toUpdate,toSkip,unchangedDestinationOnlyarraysthe exact equality assertions at lines 160-201 are thorough.
lib/oc-chatgpt-target-detection.ts (2)
144-158: hasRotated uses readdirSync - blocking but acceptable for cli startup.sync filesystem operations block the event loop. acceptable here since target detection runs at cli startup, not in hot paths. the try-catch swallowing unreadable directories is correct.
106-121: normalizeCandidatePath handles windows drive roots correctly.properly preserves trailing backslash for drive roots like
C:\and strips trailing separators from other paths. good windows handling.test/oc-chatgpt-target-detection.test.ts (1)
186-197: good windows drive root preservation test.validates that
C:\is preserved correctly after normalization. usesObject.definePropertyto mock platform - acceptable for this case.lib/named-backup-export.ts (2)
32-66: thorough backup filename validation.covers:
- empty names
- path separators (
/,\)- traversal tokens (
..)- rotation-style sequences (
.rotate.)- temporary suffixes (
.tmp,.wal)- safe character regex
good defense against path traversal attacks.
86-97: exportNamedBackupFile correctly wires force flag.per context snippet 1,
options?.force === trueensures force is only passed when explicitly set. the dependency injection pattern is clean.lib/oc-chatgpt-import-adapter.ts (3)
305-340: three-tier matching strategy is correct.
matchDestinationcorrectly prioritizes:
- accountId match (exact identity)
- email match (when source has email but dest has no accountId)
- refreshToken match (last resort for identity-less accounts)
the
usedIndexesset prevents double-matching. good.
65-90: sanitizeAccount defensively handles undefined/invalid fields.correctly:
- trims strings
- returns null for empty refreshToken
- normalizes addedAt/lastUsed to 0 if not finite
no token/email leakage in this transform.
349-417: previewOcChatgptImportMerge orchestration looks correct.correctly:
- normalizes both source and destination
- clones destination to avoid mutation
- tracks used indexes for matching
- preserves destination active selection
- computes toAdd/toUpdate/toSkip/unchangedDestinationOnly
the
activeSelectionBehavior: "preserve-destination"literal is clear.
| it("fails on an existing backup file by default", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "acct-1", | ||
| refreshToken: "refresh-1", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const destination = resolveNamedBackupPath( | ||
| "backup-2026-03-09", | ||
| storagePath, | ||
| ); | ||
| await fs.mkdir(dirname(destination), { recursive: true }); | ||
| await fs.writeFile(destination, "already-here", "utf-8"); | ||
|
|
||
| await expect( | ||
| exportNamedBackupFile("backup-2026-03-09", { | ||
| getStoragePath, | ||
| exportAccounts, | ||
| }), | ||
| ).rejects.toThrow(/already exists/); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
missing regression test for concurrent export races.
test validates "already exists" behavior but doesn't cover concurrent export attempts or EBUSY during writeFile. per coding guidelines for test/**, demand regression cases for windows filesystem behavior.
add a test that attempts parallel exports to the same backup name and verifies one succeeds while the other fails gracefully:
it("handles concurrent export attempts without corruption", async () => {
await saveAccounts({ version: 3, activeIndex: 0, accounts: [{ accountId: "a", refreshToken: "r", addedAt: 1, lastUsed: 1 }] });
const results = await Promise.allSettled([
exportNamedBackupFile("concurrent-test", { getStoragePath, exportAccounts }),
exportNamedBackupFile("concurrent-test", { getStoragePath, exportAccounts }),
]);
const successes = results.filter(r => r.status === "fulfilled");
expect(successes.length).toBeGreaterThanOrEqual(1);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/named-backup-export.test.ts` around lines 89 - 116, Add a regression
test to cover concurrent export races by invoking exportNamedBackupFile twice in
parallel for the same backup name and asserting one succeeds while the other
fails (to cover EBUSY/Windows semantics); use Promise.allSettled to run two
exportNamedBackupFile("concurrent-test", { getStoragePath, exportAccounts })
calls after preparing state with saveAccounts, then assert that at least one
result has status "fulfilled" and the other is rejected with an "already exists"
or filesystem error; reference the existing helpers saveAccounts,
exportNamedBackupFile, getStoragePath, exportAccounts and resolveNamedBackupPath
to create the destination and verify behavior without changing production code.
| async function removeWithRetry( | ||
| targetPath: string, | ||
| options: { recursive?: boolean; force?: boolean }, | ||
| ): Promise<void> { | ||
| const retryable = new Set(["EBUSY", "EPERM", "ENOTEMPTY", "EACCES"]); | ||
| for (let attempt = 0; attempt < 6; attempt += 1) { | ||
| try { | ||
| await fs.rm(targetPath, options); | ||
| return; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === "ENOENT") return; | ||
| if (!code || !retryable.has(code) || attempt === 5) throw error; | ||
| await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
duplicate removeWithRetry helper - consider extracting to shared test utility.
this exact helper appears in test/named-backup-export.test.ts:17-33. extract to a shared test/helpers/fs-retry.ts to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/oc-chatgpt-target-detection.test.ts` around lines 14 - 30, The helper
function removeWithRetry is duplicated in tests; extract it into a shared test
helper module (e.g., export async function removeWithRetry(...) from
test/helpers/fs-retry.ts) preserving the same signature and retry logic, update
the tests that currently declare removeWithRetry to import it from that shared
module, and remove the duplicate local implementations so both tests use the
single exported removeWithRetry helper.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/storage.ts">
<violation number="1" location="lib/storage.ts:1597">
P2: Use the existing retrying rename helper for the temp-file swap; a single `fs.rename` here can cause intermittent export failures on Windows when antivirus/file-lock contention returns `EPERM`/`EBUSY`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/storage.ts">
<violation number="1" location="lib/storage.ts:1606">
P1: `exportAccounts` uses a global lock-depth check to bypass the mutex, so concurrent unrelated calls can run unlocked and race with active storage transactions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| export type OcChatgptMergePreview = { | ||
| payload: OcChatgptPreviewPayload; | ||
| merged: AccountStorageV3; |
There was a problem hiding this comment.
merged: AccountStorageV3 carries full tokens — same logging risk as payload had
the previous thread addressed payload leaking tokens; payload is now correctly typed as OcChatgptPreviewPayload (only refreshTokenLast4). but merged is still AccountStorageV3, which has raw refreshToken on every account. any caller that serializes or logs the full OcChatgptMergePreview object (e.g. log.debug("merge preview", { preview })) leaks credentials. this is the same class of risk as the prior payload concern.
merged must hold full tokens to be applicable to storage — that's fine. but it needs explicit documentation that it must never be passed to a logger directly. consider:
merged: AccountStorageV3; // WARNING: contains raw refreshToken values — do not log this field directlyno existing vitest coverage logs or serializes previewOcChatgptImportMerge's return value through a mock logger, so this is currently silent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/oc-chatgpt-import-adapter.ts
Line: 27
Comment:
**`merged: AccountStorageV3` carries full tokens — same logging risk as `payload` had**
the previous thread addressed `payload` leaking tokens; `payload` is now correctly typed as `OcChatgptPreviewPayload` (only `refreshTokenLast4`). but `merged` is still `AccountStorageV3`, which has raw `refreshToken` on every account. any caller that serializes or logs the full `OcChatgptMergePreview` object (e.g. `log.debug("merge preview", { preview })`) leaks credentials. this is the same class of risk as the prior `payload` concern.
`merged` must hold full tokens to be applicable to storage — that's fine. but it needs explicit documentation that it must never be passed to a logger directly. consider:
```ts
merged: AccountStorageV3; // WARNING: contains raw refreshToken values — do not log this field directly
```
no existing vitest coverage logs or serializes `previewOcChatgptImportMerge`'s return value through a mock logger, so this is currently silent.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
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="test/oc-chatgpt-target-detection.test.ts">
<violation number="1" location="test/oc-chatgpt-target-detection.test.ts:203">
P2: This test case doesn't exercise a real `projects` path segment, so it can miss the scope-classification regression it's meant to cover.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Pushed follow-up This pass addressed the current high-confidence items still worth fixing on the latest head:
Validated with The PR body was already in good shape after the recent rewrite, so I left it concise rather than expanding it again. |
| let storageLockDepth = 0; | ||
| let activeTransactionStorage: AccountStorageV3 | null | undefined; | ||
|
|
||
| function withStorageLock<T>(fn: () => Promise<T>): Promise<T> { | ||
| const previousMutex = storageMutex; | ||
| let releaseLock: () => void; | ||
| storageMutex = new Promise<void>((resolve) => { | ||
| releaseLock = resolve; | ||
| }); | ||
| return previousMutex.then(fn).finally(() => releaseLock()); | ||
| const previousMutex = storageMutex; | ||
| let releaseLock: () => void; | ||
| storageMutex = new Promise<void>((resolve) => { | ||
| releaseLock = resolve; | ||
| }); | ||
| return previousMutex | ||
| .then(async () => { | ||
| storageLockDepth += 1; | ||
| try { | ||
| return await fn(); | ||
| } finally { | ||
| storageLockDepth -= 1; | ||
| } | ||
| }) | ||
| .finally(() => releaseLock()); |
There was a problem hiding this comment.
storageLockDepth tracked but never used as a guard
storageLockDepth is incremented and decremented inside withStorageLock but is never read anywhere in the codebase. it provides zero actual re-entrancy protection — it's dead counter code.
the practical consequence: saveAccounts, clearAccounts, saveFlaggedAccounts, and clearFlaggedAccounts all call withStorageLock and will still deadlock silently if invoked from inside a withAccountStorageTransaction callback. only exportAccounts has the activeTransactionStorage !== undefined bypass. a future caller that e.g. calls saveAccounts mid-transaction will hit a promise-chain deadlock with no diagnostic from this counter.
either wire the counter up as an actual guard (throw if storageLockDepth > 0 and activeTransactionStorage === undefined), or remove it to avoid misleading readers into thinking protection exists. no vitest coverage exercises the deadlock path for any of the other lock-acquiring exported functions.
| let storageLockDepth = 0; | |
| let activeTransactionStorage: AccountStorageV3 | null | undefined; | |
| function withStorageLock<T>(fn: () => Promise<T>): Promise<T> { | |
| const previousMutex = storageMutex; | |
| let releaseLock: () => void; | |
| storageMutex = new Promise<void>((resolve) => { | |
| releaseLock = resolve; | |
| }); | |
| return previousMutex.then(fn).finally(() => releaseLock()); | |
| const previousMutex = storageMutex; | |
| let releaseLock: () => void; | |
| storageMutex = new Promise<void>((resolve) => { | |
| releaseLock = resolve; | |
| }); | |
| return previousMutex | |
| .then(async () => { | |
| storageLockDepth += 1; | |
| try { | |
| return await fn(); | |
| } finally { | |
| storageLockDepth -= 1; | |
| } | |
| }) | |
| .finally(() => releaseLock()); | |
| let storageMutex: Promise<void> = Promise.resolve(); | |
| let activeTransactionStorage: AccountStorageV3 | null | undefined; | |
| function withStorageLock<T>(fn: () => Promise<T>): Promise<T> { | |
| const previousMutex = storageMutex; | |
| let releaseLock: () => void; | |
| storageMutex = new Promise<void>((resolve) => { | |
| releaseLock = resolve; | |
| }); | |
| return previousMutex | |
| .then(async () => { | |
| try { | |
| return await fn(); | |
| } finally { | |
| } | |
| }) | |
| .finally(() => releaseLock()); | |
| } |
(or keep the counter and actually check it — the suggestion above is the "remove dead code" path; the "guard" path needs explicit thought around which callers need a bypass like exportAccounts has.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 113-131
Comment:
**`storageLockDepth` tracked but never used as a guard**
`storageLockDepth` is incremented and decremented inside `withStorageLock` but is never read anywhere in the codebase. it provides zero actual re-entrancy protection — it's dead counter code.
the practical consequence: `saveAccounts`, `clearAccounts`, `saveFlaggedAccounts`, and `clearFlaggedAccounts` all call `withStorageLock` and will still deadlock silently if invoked from inside a `withAccountStorageTransaction` callback. only `exportAccounts` has the `activeTransactionStorage !== undefined` bypass. a future caller that e.g. calls `saveAccounts` mid-transaction will hit a promise-chain deadlock with no diagnostic from this counter.
either wire the counter up as an actual guard (throw if `storageLockDepth > 0` and `activeTransactionStorage === undefined`), or remove it to avoid misleading readers into thinking protection exists. no vitest coverage exercises the deadlock path for any of the other lock-acquiring exported functions.
```suggestion
let storageMutex: Promise<void> = Promise.resolve();
let activeTransactionStorage: AccountStorageV3 | null | undefined;
function withStorageLock<T>(fn: () => Promise<T>): Promise<T> {
const previousMutex = storageMutex;
let releaseLock: () => void;
storageMutex = new Promise<void>((resolve) => {
releaseLock = resolve;
});
return previousMutex
.then(async () => {
try {
return await fn();
} finally {
}
})
.finally(() => releaseLock());
}
```
(or keep the counter and actually check it — the suggestion above is the "remove dead code" path; the "guard" path needs explicit thought around which callers need a bypass like `exportAccounts` has.)
How can I resolve this? If you propose a fix, please make it concise.
Summary
Why
This branch is the backend foundation for the later sync PRs. Keeping the primitives isolated makes the downstream orchestration and settings flows smaller and easier to review.
Verification
npm exec vitest run test/named-backup-export.test.ts test/oc-chatgpt-import-adapter.test.ts test/oc-chatgpt-target-detection.test.ts test/storage.test.tsnpm run typechecknpm run buildNotes
#58note: 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 splits backend sync primitives into four focused modules — target detection, import/merge preview, named backup export, and storage hardening — and closes several review gaps from the prior thread. the addressed issues (suffix ordering in
normalizeNamedBackupFileName,inferScopeFromRootfalse-positives,renameFileWithRetryinexportAccounts,payloadtoken redaction, deaddefaultIndexparameter) are all correctly handled and have good vitest coverage.key changes:
lib/named-backup-export.ts— new module with path traversal guards, case-insensitive windows path comparison, and suffix validation correctly ordered after.jsonstriplib/oc-chatgpt-target-detection.ts—inferScopeFromRootnow anchors scope inference torelative(canonicalRoot, root)instead of scanning path segments; trailing slash regex now covers both\and/lib/oc-chatgpt-import-adapter.ts—payloadnow typed asOcChatgptPreviewPayload(token-redacted); deaddefaultIndexparameter removed; merge preview logic is correctlib/storage.ts—exportAccountsusesactiveTransactionStoragebypass to avoid deadlock when called insidewithAccountStorageTransaction;renameFileWithRetrynow used for windows-safe atomic renameopen concerns:
storageLockDepthis incremented/decremented inwithStorageLock(lines 113–128) but never read anywhere — it provides no actual re-entrancy protection.saveAccounts,clearAccounts,saveFlaggedAccounts, andclearFlaggedAccountsall still deadlock silently if called inside awithAccountStorageTransactioncallback. the counter should either be wired up as a guard or removed to avoid misleading future readersexportAccountscalled inside a transaction exports the in-flight (pre-persist()) snapshot — intentional and tested, but undocumented; a backup created inside an abandoned transaction will diverge from committed storagesaveAccounts,clearAccounts, etc.) from inside a transactionConfidence Score: 3/5
storageLockDepthis dead code that creates a false sense of re-entrancy safety for downstream callersstorageLockDepthbeing tracked but never checked — is not a runtime regression today but is a correctness trap: any downstream code that callssaveAccounts/clearAccountsinside a transaction will deadlock silently, and the counter gives no indication that a guard exists. the pre-persist export snapshot is intentional but undocumented. both issues are manageable before downstream orchestration PRs build on this base.lib/storage.ts— specifically thestorageLockDepthdead counter and theexportAccountsin-transaction snapshot semanticsImportant Files Changed
exportAccountswith re-entrancy bypass viaactiveTransactionStorageandrenameFileWithRetry;storageLockDepthis tracked but never read, leaving other lock-acquiring exports unguarded against in-transaction calls.jsonstrip), and windows-safe case-insensitive comparison;assertWithinDirectorylogic is soundpayloadviaOcChatgptPreviewPayload;defaultIndexdead parameter removed;normalizeActiveIndexByFamilyandclampIndexlogic looks correct;mergedfield still carries raw tokens (pre-existing open thread)inferScopeFromRootnow correctly anchors torelative(canonicalRoot, root)removing false-positives; trailing slash handling now uses[\\/]+$for windows backslash; scope inference tests cover the edge casesbackup.tmp.json), force-overwrite, and in-transaction export without deadlock; does not cover the case wherepersist()is called beforeexportAccountsinside a transactionsaveAccounts/clearAccountsinside a transaction (deadlock path) or for the Windows lock retry inexportAccountsSequence Diagram
sequenceDiagram participant Caller participant exportNamedBackupFile participant exportAccounts participant withAccountStorageTransaction participant withStorageLock participant fs note over Caller,fs: path A — called outside a transaction Caller->>exportNamedBackupFile: name, deps, options exportNamedBackupFile->>exportAccounts: destination, force note right of exportAccounts: activeTransactionStorage === undefined exportAccounts->>withAccountStorageTransaction: read current withAccountStorageTransaction->>withStorageLock: acquire lock withStorageLock-->>withAccountStorageTransaction: locked withAccountStorageTransaction-->>exportAccounts: storage snapshot exportAccounts->>fs: writeFile(tempPath) exportAccounts->>fs: renameFileWithRetry(temp→dest) exportAccounts-->>exportNamedBackupFile: destination path exportNamedBackupFile-->>Caller: destination path note over Caller,fs: path B — called inside a transaction (re-entrancy bypass) Caller->>withAccountStorageTransaction: callback withAccountStorageTransaction->>withStorageLock: acquire lock withStorageLock-->>withAccountStorageTransaction: locked, sets activeTransactionStorage withAccountStorageTransaction->>exportNamedBackupFile: (inside callback) exportNamedBackupFile->>exportAccounts: destination, force note right of exportAccounts: activeTransactionStorage !== undefined → skip lock exportAccounts->>fs: writeFile(tempPath) exportAccounts->>fs: renameFileWithRetry(temp→dest) exportAccounts-->>exportNamedBackupFile: destination path exportNamedBackupFile-->>withAccountStorageTransaction: destination path withAccountStorageTransaction-->>Caller: resultLast reviewed commit: 428aaea
Context used: