fix(storage): prevent re-login across PR worktrees#12
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)lib/**⚙️ CodeRabbit configuration file
Files:
test/**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)test/storage.test.ts (2)
🔇 Additional comments (31)
📝 WalkthroughSummaryThis PR resolves a user experience issue where opening a PR worktree incorrectly triggered re-login by changing the storage identity key from per-worktree paths to a canonical repository-root identity. Git linked worktrees now share account storage with their primary repository, while preserving backward compatibility through automatic migration of legacy per-worktree account files on first load. Key ChangesCore Functionality
Public API Additions
Documentation & Migration
Test Coverage & Risk Assessment✓ Regression Tests Exist: Comprehensive coverage added for:
✓ Data Safety: Legacy files are retained on canonical persist failure, ensuring no account loss if migration encounters filesystem errors ⚠ Architectural Consideration: Storage key derivation now depends on worktree detection logic; misconfiguration of No security vulnerabilities or test coverage gaps identified. Walkthroughadds worktree-aware storage identity resolution and automatic legacy worktree→repo-shared storage migration. worktrees that reference a shared git commondir resolve to the shared repo root as the storage identity; legacy worktree-scoped files are detected, merged, and removed only after a successful canonical write. Changes
Sequence Diagram(s)sequenceDiagram
participant app as application
participant paths as identity_resolver
participant fs as filesystem
participant storage as storage_layer
participant legacy as legacy_storage
participant canonical as canonical_storage
app->>storage: setStoragePath(projectRoot)
storage->>paths: resolveProjectStorageIdentityRoot(projectRoot)
paths->>fs: read .git (file or dir)
alt .git is file with gitdir pointer
paths->>fs: resolve gitdir, read commondir
fs-->>paths: shared_repo_root
paths-->>storage: return shared_repo_root
else .git is directory or pointer absent
paths-->>storage: return projectRoot
end
storage->>storage: compute currentStoragePath and legacy candidate paths
storage->>legacy: check candidate legacy paths
alt legacy exists
legacy-->>storage: load legacy storage
storage->>canonical: load canonical storage
storage->>storage: mergeStorageForMigration()
storage->>canonical: write merged storage
alt write succeeds
storage->>legacy: remove legacy file
else write fails
storage-->>legacy: keep legacy file
end
else no legacy
storage->>canonical: load canonical storage
end
storage-->>app: return account_pool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes notes (explicit flags you must review)
Suggested labels
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.ts`:
- Around line 241-257: The code unconditionally calls
loadNormalizedStorageFromPath(currentStoragePath, "current account storage")
before verifying any legacy files exist, causing unnecessary reads; change the
flow to first probe candidatePaths (from
currentLegacyWorktreeStoragePath/currentLegacyProjectStoragePath) with
existsSync to detect any real legacy files and only call
loadNormalizedStorageFromPath when at least one legacyPath exists (or move the
load inside the loop just before migrating that legacyPath), ensuring
candidatePaths de-duplication remains and keeping references to candidatePaths,
legacyPath, loadNormalizedStorageFromPath, and currentStoragePath intact.
In `@test/paths.test.ts`:
- Around line 193-232: Add two regression test cases to the existing spec around
resolveProjectStorageIdentityRoot: one where mockedReadFileSync returns a
relative gitdir pointer (e.g., "gitdir: ../.git\n" or "../..\n" in the
commondir) and another where it returns a Windows backslash-style pointer (e.g.,
"gitdir: C:\\repo\\.git\\worktrees\\pr-8\n" or backslash-heavy commondir
content); update mockedExistsSync/mockedStatSync/mockedReadFileSync behaviors in
those tests to return true/appropriate stats for the candidate paths and assert
that resolveProjectStorageIdentityRoot(projectRoot) still resolves to the
sharedRepoRoot, ensuring normalize logic and relative-path resolution are
exercised for both POSIX and Windows-style inputs.
In `@test/storage.test.ts`:
- Around line 1047-1232: Add deterministic Vitest regression tests that exercise
concurrent migrations and Windows-style gitdir paths: extend the
"worktree-scoped storage migration" suite to include one test that calls
loadAccounts() concurrently (e.g., Promise.all over multiple loadAccounts()
invocations after setStoragePath(worktreeRepo)) against the same worktree
fixture and asserts the canonical file exists, legacy file removed, and merged
accounts without duplicate race artifacts; and another test that uses a
Windows-style worktree fixture (write the worktreeRepo/.git file with a
"gitdir:" line using backslashes and ensure commondir uses backslashes) to
verify getProjectStorageKey/getConfigDir path handling and migration behavior.
Reference functions/helpers: loadAccounts, setStoragePath, getStoragePath,
getConfigDir, getProjectStorageKey, and prepareWorktreeFixture to locate where
to add the new tests and fixture variations. Ensure tests remain deterministic
(use vi mocks only when needed) and use vitest async assertions.
- Around line 797-799: The tests restore environment variables unsafely by
reassigning potentially undefined originals (originalHome, originalUserProfile)
which can leak invalid env state across tests; replace those direct assignments
with the conditional delete pattern used earlier (if originalHome === undefined
then delete process.env.HOME else set it, and same for USERPROFILE) in all four
places where envs are restored around testWorkDir cleanup, and ensure
testWorkDir is removed after env fixups; additionally extend the migration test
suite (the migration tests covering persist/fail paths) to add
parallel/concurrent migration races and Windows-specific filesystem cases
(drive-letter paths and NT separator behaviors) and include tests for concurrent
lock scenarios so the migration code is exercised under race and Windows path
edge conditions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
docs/reference/storage-paths.mddocs/troubleshooting.mddocs/upgrade.mdlib/storage.tslib/storage/paths.tstest/paths.test.tstest/storage.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/upgrade.mddocs/troubleshooting.mddocs/reference/storage-paths.md
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/paths.test.tstest/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/storage/paths.tslib/storage.ts
🧬 Code graph analysis (3)
test/paths.test.ts (1)
lib/storage/paths.ts (2)
getProjectGlobalConfigDir(140-142)resolveProjectStorageIdentityRoot(154-192)
test/storage.test.ts (2)
lib/storage.ts (4)
setStoragePath(179-206)getStoragePath(219-224)setStoragePathDirect(208-213)loadAccounts(612-614)lib/storage/paths.ts (2)
getConfigDir(52-54)getProjectStorageKey(114-122)
lib/storage.ts (2)
lib/storage/paths.ts (3)
resolveProjectStorageIdentityRoot(154-192)getProjectGlobalConfigDir(140-142)getProjectConfigDir(64-66)lib/storage/migrations.ts (1)
AccountStorageV3(59-64)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/paths.test.ts`:
- Around line 275-317: The test currently skips on non-Windows platforms but the
mocked fs calls (mockedExistsSync, mockedStatSync, mockedReadFileSync) and use
of path.win32 make it safe to run cross-platform; remove the early platform
guard (the if (process.platform !== "win32") return;) so the Windows-style
backslash gitdir pointer case always runs. Keep the existing
path.win32.join/normalize usage, the mocked implementations for
mockedExistsSync/mockedStatSync/mockedReadFileSync, and the call to
resolveProjectStorageIdentityRoot so the test validates Windows backslash
handling on all CI environments.
In `@test/storage.test.ts`:
- Around line 1297-1333: The current test only runs on win32 and misses a
cross-platform regression where resolveProjectStorageIdentityRoot() calls
path.resolve() which treats backslashes literally on POSIX; add a new unit test
for parseGitDirPointer() (and/or a variant of the existing worktree fixture
test) that supplies a Windows-style gitdir pointer string containing backslashes
and asserts the returned path is normalized/canonical on POSIX, or alternatively
mock path.resolve() in the test to simulate Windows resolution and validate that
parseGitDirPointer()/resolveProjectStorageIdentityRoot() convert backslashes
into proper separators before resolving; reference parseGitDirPointer,
resolveProjectStorageIdentityRoot, and path.resolve in the test to locate the
code to exercise.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
lib/storage.tstest/paths.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/storage.test.tstest/paths.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.ts
🧬 Code graph analysis (2)
test/storage.test.ts (2)
lib/storage.ts (3)
setStoragePath(179-206)getStoragePath(219-224)setStoragePathDirect(208-213)lib/storage/paths.ts (2)
getConfigDir(52-54)getProjectStorageKey(114-122)
lib/storage.ts (2)
lib/storage/paths.ts (3)
resolveProjectStorageIdentityRoot(154-192)getProjectGlobalConfigDir(140-142)getProjectConfigDir(64-66)lib/storage/migrations.ts (1)
AccountStorageV3(59-64)
🔇 Additional comments (24)
lib/storage.ts (10)
8-15: imports look good.added
resolveProjectStorageIdentityRootimport aligns with the new identity resolution logic. no issues here.
148-148: worktree legacy path tracking is correctly scoped.
currentLegacyWorktreeStoragePathis properly initialized and cleared in all relevant paths:setStoragePath,setStoragePathDirect, and reset tonullwhen appropriate. the conditional atlib/storage.ts:198-199correctly avoids self-referential legacy paths.Also applies to: 183-183, 198-199, 203-203, 211-211
191-199: identity root computation and legacy path setup look correct.
lib/storage.ts:191-192properly derives canonical path via identity root, andlib/storage.ts:194-199computes the previous worktree-scoped path only when it differs from canonical. this prevents migration loops.
237-254: migration preflight now correctly gates on existing files.the past review comment flagged unconditional canonical reads before checking legacy existence.
lib/storage.ts:251-254now filterscandidatePathsviaexistsSyncbefore loading canonical storage, eliminating unnecessary reads and windows file-lock pressure.
256-280: migration loop handles persist failures gracefully.
lib/storage.ts:268-280catches persist errors, logs a warning, and continues with fallback storage. legacy files are only deleted after successful persist. this matches the pr objective of preserving legacy files on failed migration.
282-301: legacy file cleanup with proper error handling.
lib/storage.ts:282-295only unlinks after successful persist, ignoresENOENT, and logs warnings for other errors. logging atlib/storage.ts:297-301includes path and account count but no tokens or emails—good.
304-311: migration return logic handles edge cases.
lib/storage.ts:304-310returns migrated storage when migration succeeded, falls back to target storage if canonical file doesn't exist yet (covers first-load-from-legacy scenario), or returns null. this aligns with expected behavior.
313-336: loadNormalizedStorageFromPath helper is clean.
lib/storage.ts:313-336encapsulates load-and-normalize with proper error handling. schema warnings are logged with path context but no sensitive data.ENOENTis silently ignored, other errors are logged as warnings.
338-356: mergeStorageForMigration correctly concatenates accounts.
lib/storage.ts:346-351merges incoming accounts after current accounts, preserves current's active index, and normalizes the result. if normalization fails, it falls back to current. deduplication happens insidenormalizeAccountStorage, so duplicates across legacy and canonical will be handled.
669-694: loadAccountsInternal migration integration is sound.
lib/storage.ts:670-672calls migration early when persist is available.lib/storage.ts:693-694returns migrated legacy storage onENOENTif canonical file doesn't exist yet—this covers the first-load-after-migration case where canonical was just created.test/storage.test.ts (8)
771-803: worktree storage path identity test is well-structured.
test/storage.test.ts:772-803correctly sets up a worktree fixture with.gitfile pointing togitdir:, verifies both main repo and worktree resolve to the same storage path. env restoration uses conditional delete pattern. cleanup in finally block is proper.
925-929: env restoration fixed per past review.
test/storage.test.ts:925-928now uses the conditional delete pattern instead of direct assignment. this prevents leakingundefinedas a string into the environment.
1016-1020: env restoration fixed in legacy migration suite.same conditional delete pattern applied at
test/storage.test.ts:1016-1019. consistent with other cleanup blocks.
1053-1137: worktree migration test suite setup is thorough.
test/storage.test.ts:1053-1137sets up:
prepareWorktreeFixturehelper withpointerStyleoption for windowsbuildStoragehelper for consistent test data- proper env save/restore with
CODEX_MULTI_AUTH_DIRvi.restoreAllMocks()in afterEach
1139-1168: basic worktree migration test covers happy path.
test/storage.test.ts:1139-1168verifies legacy worktree storage migrates to canonical path, legacy file is removed, and canonical file exists. assertions are clear.
1170-1207: merge test validates both accounts are preserved.
test/storage.test.ts:1170-1207sets up both canonical and legacy files, verifies merge produces 2 accounts with both ids present, and legacy file is removed. good coverage.
1209-1250: persist failure retention test uses targeted mock.
test/storage.test.ts:1230-1241mocksfs.writeFileonly for the WAL path to simulate persist failure. verifies legacy file is retained when persist fails. this matches the pr objective of "no destructive cleanup on failed migration".
1252-1295: concurrent migration test addresses past review concern.
test/storage.test.ts:1252-1295runs 4 parallelloadAccounts()calls against the same worktree fixture. verifies:
- all results have 2 accounts
- persisted file has exactly 2 accounts (no duplicates from race)
- legacy file is removed
this covers the concurrency gap flagged in past reviews.
test/paths.test.ts (6)
5-11: mock setup and helper are clean.
test/paths.test.ts:5-11addsreadFileSyncandstatSyncmocks.test/paths.test.ts:27-38provides a typed helper for stat results. this keeps tests deterministic per coding guidelines.Also applies to: 23-38
167-174: getProjectGlobalConfigDir test relocated into nested describe.
test/paths.test.ts:167-174nests the test properly. assertions verify path contains expected segments.
176-191: standard .git directory test verifies no worktree resolution.
test/paths.test.ts:177-191confirmsresolveProjectStorageIdentityRootreturns project root unchanged when.gitis a directory. verifiesreadFileSyncis not called (no gitdir pointer needed).
193-232: linked worktree test validates shared repo root resolution.
test/paths.test.ts:193-232sets up worktree fixture with absolutegitdir:pointer andcommondir: ../... verifies resolution returns shared repo root. this is the core worktree identity behavior.
234-273: relative gitdir pointer test addresses past review.
test/paths.test.ts:234-273usesgitdir: ../../.git/worktrees/pr-relative(relative path). verifies theresolvelogic inlib/storage/paths.ts:175-177handles relative pointers correctly.
319-336: non-worktree .git file test validates fallback.
test/paths.test.ts:319-335simulates a submodule withgitdir: /repo/.git/modules/submodule(not underworktrees/). verifiesisWorktreeGitDirPathreturns false and project root is kept unchanged.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/paths.ts`:
- Around line 206-217: The code currently accepts any .git pointer under
worktrees by existence alone; update the logic in
resolveGitPath/isWorktreeGitDirPath/readGitCommonDir usage so that after
computing commonGitDir you call fs.realpath on commonGitDir and projectRoot and
only accept candidateRepoRoot when the realpath of candidateRepoRoot is a
subpath of the realpath(projectRoot) (reject otherwise), preventing forged
pointers from aliasing another repo's store; also add a unit test (vitest) that
simulates a forged worktree pointer (including Windows symlink behavior) and
verify rejection, add retry handling for EBUSY/429 in any new IO helpers, and
ensure logging in this flow (in functions above) does not include tokens/emails.
In `@test/paths.test.ts`:
- Around line 176-332: Add a regression test to the
resolveProjectStorageIdentityRoot suite that simulates a ".git" file pointing at
a worktrees directory that is not under the repository's shared git dir and
asserts the function returns the original projectRoot; specifically, create a
test that stubs existsSync/statSync/readFileSync so .git is a file (isFile true)
returning a "gitdir: /some/other/repo/.git/worktrees/xxx" pointer and ensure the
shared .git path is not considered present, then call
resolveProjectStorageIdentityRoot(projectRoot) and expect it to equal
projectRoot to guard the fallback logic in resolveProjectStorageIdentityRoot.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
lib/storage/paths.tstest/paths.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/paths.test.tstest/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/storage/paths.ts
🧬 Code graph analysis (1)
test/paths.test.ts (1)
lib/storage/paths.ts (2)
getProjectGlobalConfigDir(171-173)resolveProjectStorageIdentityRoot(185-221)
🔇 Additional comments (1)
test/storage.test.ts (1)
1252-1330: good migration regression coverage for race + windows paths.
test/storage.test.ts:1252-1330(line 1252-1330) now exercises concurrentloadAccounts()migration and windows-stylegitdirfixtures without platform gating. this is the right integration safety net for the storage identity change.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/paths.ts`:
- Around line 252-258: The current code trusts readGitCommonDir output and
accepts candidateRepoRoot if join(candidateRepoRoot, ".git") exists, which
allows a forged commondir to alias another repo; update the logic in the
function that computes candidateRepoRoot to validate that the resolved
commonGitDir is actually located inside candidateRepoRoot (or is a subpath of
candidateRepoRoot) and that the gitdir/commondir symlink chain resolves to paths
staying within the candidateRepoRoot; if any validation fails (commondir points
outside candidateRepoRoot or symlink resolution escapes the root), return
projectRoot as the safe fallback. Refer to readGitCommonDir, candidateRepoRoot,
existsSync(join(candidateRepoRoot, ".git"))) and projectRoot to locate the
patch. Also add a Vitest regression that creates the described fake worktree
files (.git with gitdir -> attacker path, worktrees/pr-x/gitdir ->
projectRoot/.git, worktrees/pr-x/commondir -> /repo/victim/.git) and assert the
function falls back to projectRoot.
In `@test/paths.test.ts`:
- Around line 330-366: Add a deterministic regression test for the hostile
commondir aliasing scenario by extending or adding a vitest case that calls
resolveProjectStorageIdentityRoot(projectRoot) where the .git file points
(gitdir) to a worktree whose gitdir back-ref matches the project but its
commondir contents point outside the project; ensure mockedExistsSync returns
true for project .git, foreign commondir, foreign gitdir back-ref and victim
.git, mockedStatSync reports the project .git as a file (not dir), and
mockedReadFileSync returns the gitdir line for the project .git, a forged
"../.." for the foreign commondir, and the absolute path for the foreign gitdir
back-ref; finally assert the resolved path equals projectRoot to lock down the
regression.
In `@test/storage.test.ts`:
- Around line 1054-1337: Add a deterministic vitest integration that simulates a
forged worktree commondir alias: using prepareWorktreeFixture create two repos
(original worktreeRepo and a foreign repo), then modify the worktree's
worktreeGitDir/commondir to point at the foreign repo (mimicking a hostile
commondir), write a legacy worktree storage file for the original worktree (via
getProjectStorageKey(worktreeRepo)) and a conflicting canonical file for the
foreign repo, call setStoragePath(worktreeRepo) and loadAccounts(), and assert
that loadAccounts returns only the original worktree's accounts (no
cross-contamination into the foreign repo), the canonical path for the original
project contains the migrated data, and any legacy file behavior (existence or
removal) matches existing migration semantics; place the test inside the
existing "worktree-scoped storage migration" describe block and follow patterns
used by prepareWorktreeFixture, setStoragePath, getStoragePath, getConfigDir,
getProjectStorageKey, and loadAccounts so it remains deterministic and uses
vitest APIs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
lib/storage/paths.tstest/paths.test.tstest/storage.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/paths.ts
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/paths.test.tstest/storage.test.ts
🧬 Code graph analysis (2)
test/paths.test.ts (1)
lib/storage/paths.ts (2)
getProjectGlobalConfigDir(209-211)resolveProjectStorageIdentityRoot(223-262)
test/storage.test.ts (2)
lib/storage.ts (5)
setStoragePath(179-206)getStoragePath(219-224)setStoragePathDirect(208-213)loadAccounts(613-615)normalizeAccountStorage(517-606)lib/storage/paths.ts (2)
getConfigDir(121-123)getProjectStorageKey(183-191)
🔇 Additional comments (2)
lib/storage/paths.ts (1)
33-54: good cross-platform path resolution split.
lib/storage/paths.ts:33-54cleanly separates windows-rooted and posix flows and normalizes delimiters before final comparison paths. this is a solid base for windows filesystem behavior.test/storage.test.ts (1)
1259-1336: nice coverage for concurrency and windows migration paths.
test/storage.test.ts:1259-1336now exercises concurrentloadAccounts()migration and windows-style gitdir fixtures without skips. this is the right direction for race and filesystem portability regressions.
Reject worktree identity aliasing when commondir does not match the worktree gitdir ancestry, and add unit/integration regressions for hostile commondir scenarios. Co-authored-by: Codex <noreply@openai.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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 `@test/paths.test.ts`:
- Around line 285-328: The test covers drive-letter Windows pointers but misses
a UNC-share gitdir pointer case; add a vitest regression that calls
resolveProjectStorageIdentityRoot with a UNC projectRoot (e.g.
"\\\\server\\share\\repo\\worktrees\\pr-8") and extend the mockedExistsSync,
mockedStatSync and mockedReadFileSync branches to recognize the UNC .git,
worktree gitdir, commondir and sharedGitDir equivalents (use
path.win32.join/normalize for comparisons) so the function follows the same
resolution path and the expectation asserts the normalized shared UNC root;
reference resolveProjectStorageIdentityRoot and the existing test block in
test/paths.test.ts to mirror the drive-letter case but using UNC paths.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
lib/storage/paths.tstest/paths.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/paths.test.tstest/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/storage/paths.ts
🧬 Code graph analysis (2)
test/paths.test.ts (1)
lib/storage/paths.ts (2)
getProjectGlobalConfigDir(225-227)resolveProjectStorageIdentityRoot(239-281)
test/storage.test.ts (2)
lib/storage.ts (5)
setStoragePath(179-206)getStoragePath(219-224)setStoragePathDirect(208-213)loadAccounts(613-615)normalizeAccountStorage(517-606)lib/storage/paths.ts (2)
getConfigDir(137-139)getProjectStorageKey(199-207)
🔇 Additional comments (2)
lib/storage/paths.ts (1)
239-277: worktree identity hardening looks good.
lib/storage/paths.ts:260-270now blocks aliasing unless both back-ref ownership andcommondirancestry checks pass, andlib/storage/paths.ts:241-279safely falls back toprojectRooton invalid or hostile layouts. coverage intest/paths.test.ts:330-404andtest/storage.test.ts:1338-1393matches the risk surface.test/storage.test.ts (1)
1054-1394: migration regression coverage is strong here.
test/storage.test.ts:1259-1302(concurrency race),test/storage.test.ts:1304-1336(windows-style pointers), andtest/storage.test.ts:1338-1393(forgedcommondirisolation) directly cover the high-risk paths for this pr.
Add a deterministic Windows UNC pointer regression for storage identity root resolution. Co-authored-by: Codex <noreply@openai.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@greptile review |
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
|
Superseded by #14 — all changes from this PR have been merged into the unified supersede PR. |
Resolves all open Dependabot alerts on package-lock.json: - hono <4.12.14: JSX SSR HTML injection, cookie name bypass, IPv4-mapped IPv6 ipRestriction, setCookie validation, serveStatic repeated-slash bypass, toSSG path traversal (alerts #16, #18, #20, #22, #24, #26) - vite <7.3.2: dev server WebSocket arbitrary file read, optimized deps .map path traversal, server.fs.deny query bypass (alerts #12, #13, #14) Lockfile refreshed via npm install --package-lock-only. Typecheck, lint, and 3418/3418 tests pass.
Resolves all open Dependabot alerts on package-lock.json: - hono <4.12.14: JSX SSR HTML injection, cookie name bypass, IPv4-mapped IPv6 ipRestriction, setCookie validation, serveStatic repeated-slash bypass, toSSG path traversal (alerts #16, #18, #20, #22, #24, #26) - vite <7.3.2: dev server WebSocket arbitrary file read, optimized deps .map path traversal, server.fs.deny query bypass (alerts #12, #13, #14) Lockfile refreshed via npm install --package-lock-only. Typecheck, lint, and 3418/3418 tests pass.
Completes partial fixes flagged in the third review, bumps to v2.2.1. Real bugs (prior fixes were incomplete): - request/fetch-helpers: isUnsupportedCodexModelForChatGpt (the handleErrorResponse path) now also matches NORMALIZED_UNSUPPORTED_MODEL_PATTERN, so a 400 'model not currently available for this chatgpt account' gets the entitlement rewrite, not generic error guidance (#13) - forecast: quota-exhausted accounts are classified 'delayed' (not 'unavailable'), so they slipped the recommendation filter; added an explicit exhausted flag and excluded it, returning null when the whole pool is exhausted (#10) - storage/record-utils: clampIndex guards NaN -> 0 (Math.trunc(NaN) propagated) (#16) - local-client-tokens: debounce lastUsedAt persistence (60s threshold) so the bearer-verify hot path stops writing to disk every request; +chmod 0o700 re-assert on the token-store dir (#12, #11) - mcodex: relay SIGTERM/SIGINT to the spawned child so it isn't orphaned (#15) Test quality: - local-client-tokens: parameterized rename-retry test over ENOTEMPTY/EAGAIN/EACCES (#18) - storage-flagged: clear the H4 deadlock-guard timer on the happy path (#17) - storage: removeWithRetry for suite cleanup (#19) Release: - bump package.json + .codex-plugin/plugin.json to 2.2.1 - add docs/releases/v2.2.1.md; point docs portal + README at it Known follow-ups (documented, deferred — need design, not rushed into a patch): config env-path save is a single-process CAS not a true cross-process lock (#8/#9); verifyLocalClientBearerToken read stays serialized but a fuller lease is future work; runtime proxy routingMutex='enabled' still has a select/commit cursor race (#14) requiring an async refactor of chooseAccount across its call sites. Full suite: 4337 passed, 3 skipped, 0 failed; typecheck + lint clean.
1 participant
Summary
Root cause
perProjectAccountskeyed storage by normalized filesystem path. Each PR worktree path produced a different<project-key>, so account pools looked empty and users had to log in again.Changes
lib/storage/paths.tsresolveProjectStorageIdentityRoot(projectRoot).gitfile +commondirand returns shared repo identity rootlib/storage.tssetStoragePathnow keys project storage by identity root (repo-shared for linked worktrees)test/paths.test.ts: worktree identity resolution casestest/storage.test.ts: shared keying + migration/merge/failure-retention regressionsValidation
npm run typechecknpm run lintnpm testnpm run buildnote: 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 fixes per-project account re-login when switching between git worktrees by resolving storage identity to the shared repository root instead of individual worktree paths.
key changes:
lib/storage/paths.ts: addedresolveProjectStorageIdentityRoot()that detects linked worktrees via.gitfile parsing, validatescommondirandgitdirbackrefs to prevent forged repository aliasing, handles windows unc paths and backslash separatorslib/storage.ts: modifiedsetStoragePath()to use identity root for keying, added legacy worktree-scoped migration that merges old files into canonical storage, retains legacy files on persist failurewindows filesystem safety:
win32module for unc/backslash handlingloadAccounts()calls tested (test/storage.test.ts:1259) - relies onsaveAccountsmutex + deduplication innormalizeAccountStorageto prevent duplicate accountsENOENTto handle concurrent unlink racesEACCES)issue - org rule 637a42e6-7a78-40d6-9ef8-6a45e02e73b6 compliance:
the pr description should explicitly explain:
withStorageLock, reads don't but deduplication prevents duplicates)ENOENTignored at line 288)token leakage:
Confidence Score: 4/5
Important Files Changed
resolveProjectStorageIdentityRoot()with comprehensive worktree detection and security validation (gitdir backref, commondir validation, UNC/Windows path support)Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[setStoragePath called] --> B{findProjectRoot} B --> C[resolveProjectStorageIdentityRoot] C --> D{.git exists?} D -->|no| E[return projectRoot] D -->|yes| F{.git is file?} F -->|no/dir| E F -->|yes| G[parse gitdir pointer] G --> H{contains /.git/worktrees/?} H -->|no| E H -->|yes| I[validate gitdir backref] I -->|invalid| E I -->|valid| J[read commondir] J --> K{gitdir under commondir/worktrees?} K -->|no| E K -->|yes| L[return dirname commondir] L --> M[getProjectGlobalConfigDir identityRoot] E --> M M --> N[storage keyed to shared repo] N --> O[loadAccounts] O --> P[migrateLegacyProjectStorageIfNeeded] P --> Q{legacy paths exist?} Q -->|no| R[load from canonical] Q -->|yes| S[load canonical + legacy] S --> T[mergeStorageForMigration] T --> U[saveAccounts mutex] U --> V[deduplicateAccounts] V --> W[persist to canonical] W --> X{success?} X -->|yes| Y[delete legacy files] X -->|no| Z[keep legacy files]Last reviewed commit: 6d137a8
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)