Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
ndycode
added
passed
needs-human-review
ndycode
removed
the
needs-human-review
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| withAccountStorageTransaction(async () => { | ||
| setStoragePathDirect( | ||
| testStoragePath.replaceAll("/", "\\").toUpperCase(), | ||
| ); | ||
| await exportAccounts(exportPath); | ||
| }), | ||
| ).resolves.toBeUndefined(); | ||
|
|
||
| const exported = JSON.parse(await fs.readFile(exportPath, "utf-8")); | ||
| expect(exported.accounts[0].accountId).toBe("acct-primary"); | ||
| } finally { | ||
| Object.defineProperty(process, "platform", { | ||
| value: originalPlatform, | ||
| configurable: true, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| it("allows symlink-resolved storage paths during export from an active transaction", async () => { | ||
| if (process.platform === "win32") { | ||
| return; | ||
| } | ||
|
|
||
| const realStorageDir = join(testWorkDir, "real-storage"); | ||
| const aliasStorageDir = join(testWorkDir, "alias-storage"); | ||
| const realStoragePath = join(realStorageDir, "accounts.json"); | ||
| const aliasStoragePath = join(aliasStorageDir, "accounts.json"); | ||
|
|
||
| await fs.mkdir(realStorageDir, { recursive: true }); | ||
| await fs.symlink(realStorageDir, aliasStorageDir, "dir"); | ||
|
|
||
| setStoragePathDirect(realStoragePath); | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "acct-primary", | ||
| refreshToken: "refresh-primary", | ||
| addedAt: 1, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| await expect( | ||
| withAccountStorageTransaction(async () => { | ||
| setStoragePathDirect(aliasStoragePath); | ||
| await exportAccounts(exportPath); | ||
| }), | ||
| ).resolves.toBeUndefined(); | ||
|
|
||
| const exported = JSON.parse(await fs.readFile(exportPath, "utf-8")); | ||
| expect(exported.accounts[0].accountId).toBe("acct-primary"); | ||
| }); | ||
|
|
||
| it("reloads fresh storage after a transaction handler throws", async () => { | ||
| await saveAccounts({ |
There was a problem hiding this comment.
normalizeStorageComparisonPath calls resolvePath(path) (which uses Node's platform-native path.resolve) before converting backslashes to forward slashes. On Linux, path.resolve does not treat \ as a path separator, so \TMP\TEST\ACCOUNTS.JSON gets resolved as a relative path component: ${cwd}/\TMP\TEST\ACCOUNTS.JSON. The subsequent .replaceAll("\\", "/").toLowerCase() then produces ${cwd}//tmp/test/accounts.json, which is never equal to the canonical /tmp/test/accounts.json.
Concretely, patching process.platform = "win32" does not patch Node's path.resolve semantics, so areEquivalentStoragePaths returns false for the backslash-upper version even with the win32 branch active. The guard throws and the test fails on any Linux CI runner.
The symlink test already handles this correctly with a platform guard:
| withAccountStorageTransaction(async () => { | |
| setStoragePathDirect( | |
| testStoragePath.replaceAll("/", "\\").toUpperCase(), | |
| ); | |
| await exportAccounts(exportPath); | |
| }), | |
| ).resolves.toBeUndefined(); | |
| const exported = JSON.parse(await fs.readFile(exportPath, "utf-8")); | |
| expect(exported.accounts[0].accountId).toBe("acct-primary"); | |
| } finally { | |
| Object.defineProperty(process, "platform", { | |
| value: originalPlatform, | |
| configurable: true, | |
| }); | |
| } | |
| }); | |
| it("allows symlink-resolved storage paths during export from an active transaction", async () => { | |
| if (process.platform === "win32") { | |
| return; | |
| } | |
| const realStorageDir = join(testWorkDir, "real-storage"); | |
| const aliasStorageDir = join(testWorkDir, "alias-storage"); | |
| const realStoragePath = join(realStorageDir, "accounts.json"); | |
| const aliasStoragePath = join(aliasStorageDir, "accounts.json"); | |
| await fs.mkdir(realStorageDir, { recursive: true }); | |
| await fs.symlink(realStorageDir, aliasStorageDir, "dir"); | |
| setStoragePathDirect(realStoragePath); | |
| await saveAccounts({ | |
| version: 3, | |
| activeIndex: 0, | |
| accounts: [ | |
| { | |
| accountId: "acct-primary", | |
| refreshToken: "refresh-primary", | |
| addedAt: 1, | |
| lastUsed: 2, | |
| }, | |
| ], | |
| }); | |
| await expect( | |
| withAccountStorageTransaction(async () => { | |
| setStoragePathDirect(aliasStoragePath); | |
| await exportAccounts(exportPath); | |
| }), | |
| ).resolves.toBeUndefined(); | |
| const exported = JSON.parse(await fs.readFile(exportPath, "utf-8")); | |
| expect(exported.accounts[0].accountId).toBe("acct-primary"); | |
| }); | |
| it("reloads fresh storage after a transaction handler throws", async () => { | |
| await saveAccounts({ | |
| it("allows equivalent Windows-style storage paths during export from an active transaction", async () => { | |
| if (process.platform !== "win32") { | |
| return; | |
| } | |
| const originalPlatform = process.platform; |
This test should be restricted to actual win32 runs, or the implementation in normalizeStorageComparisonPath needs to use path.win32.resolve when on win32 instead of relying on the ambient path.resolve.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/storage.test.ts
Line: 1183-1240
Comment:
**Windows test broken on Linux CI**
`normalizeStorageComparisonPath` calls `resolvePath(path)` (which uses Node's platform-native `path.resolve`) *before* converting backslashes to forward slashes. On Linux, `path.resolve` does not treat `\` as a path separator, so `\TMP\TEST\ACCOUNTS.JSON` gets resolved as a relative path component: `${cwd}/\TMP\TEST\ACCOUNTS.JSON`. The subsequent `.replaceAll("\\", "/").toLowerCase()` then produces `${cwd}//tmp/test/accounts.json`, which is never equal to the canonical `/tmp/test/accounts.json`.
Concretely, patching `process.platform = "win32"` does not patch Node's `path.resolve` semantics, so `areEquivalentStoragePaths` returns `false` for the backslash-upper version even with the win32 branch active. The guard throws and the test fails on any Linux CI runner.
The symlink test already handles this correctly with a platform guard:
```suggestion
it("allows equivalent Windows-style storage paths during export from an active transaction", async () => {
if (process.platform !== "win32") {
return;
}
const originalPlatform = process.platform;
```
This test should be restricted to actual win32 runs, or the implementation in `normalizeStorageComparisonPath` needs to use `path.win32.resolve` when on win32 instead of relying on the ambient `path.resolve`.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
exportAccountsis called from an active transaction after the storage path driftsValidation
npm exec vitest run test/storage.test.tsnpm run typechecknote: 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 hardens the storage transaction layer against a class of silent data-corruption bugs where
getStoragePath()drifts mid-transaction (common in multi-worktree and test scenarios). it pins all reads and writes inwithAccountStorageTransactionandwithAccountAndFlaggedStorageTransaction— including the migration-persist callback and the rollback write — to the path captured at transaction start. it also marks the transaction snapshot inactive in afinallyblock soexportAccountscalled after the handler exits reloads fresh state rather than re-using a stale snapshot. on theexportAccountsside, the old silent "load from wrong file" fallback is replaced with a fail-fast error when the active path has drifted away from the transaction path, with symlink and windows case-folding equivalence handled viaareEquivalentStoragePaths.key changes:
loadAccountsInternalandsaveAccountsUnlockednow accept an explicitstoragePathparameter (defaulting togetStoragePath()) so callers can pin writes to a specific rootsaveFlaggedAccountsUnlockedsimilarly pinned;flaggedStoragePathcaptured at combined-transaction start and passed through rollback toostate.active = falseinfinallyin both transaction functions; subsequentexportAccountscalls outside the transaction context reload from diskexportAccountsthrows with a descriptive error when called from an active transaction whose storage path no longer matches the active pathnormalizeStorageComparisonPathresolves symlinks and folds case/slashes on win32 for the equivalence checkprocess.platformbut not Node'spath.resolvesemantics, soareEquivalentStoragePathswill returnfalsefor backslash paths on posix runners, causing the guard to throw instead of passConfidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Caller participant T as withAccountStorageTransaction participant L as loadAccountsInternal(storagePath) participant S as saveAccountsUnlocked(storagePath) participant E as exportAccounts participant CTX as transactionSnapshotContext C->>T: call handler T->>T: storagePath = getStoragePath() [pinned] T->>L: load(persistMigration pinned, storagePath) L-->>T: snapshot T->>CTX: run(state{snapshot, storagePath, active:true}) CTX->>C: handler(current, persist) C->>S: persist(storage) → saveAccountsUnlocked(storage, storagePath) S-->>C: ok C->>E: exportAccounts(path) E->>E: activeStoragePath = getStoragePath() E->>E: areEquivalentStoragePaths(state.storagePath, activeStoragePath)? alt paths differ E-->>C: throws "Export blocked by storage path mismatch" else paths equivalent E->>E: use state.snapshot (no extra load) E-->>C: ok end CTX-->>T: handler returns/throws T->>T: finally: state.active = false T-->>C: result / rethrowPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: harden storage path transactions" | Re-trigger Greptile