feat: isolate review cache roots - #129
Conversation
📝 WalkthroughWalkthroughAdds ChangesReview cache root
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLIAdapter
participant reviewCacheRoot
participant GitSubprocess
participant FileSystem
CLIAdapter->>reviewCacheRoot: Provide target, runtime, and optional cache base
reviewCacheRoot->>GitSubprocess: Query Git root, common directory, and history roots
GitSubprocess-->>reviewCacheRoot: Return repository metadata
reviewCacheRoot->>FileSystem: Validate boundaries and create cache directory
FileSystem-->>reviewCacheRoot: Return resolved cache path
reviewCacheRoot-->>CLIAdapter: Print cache path with null terminator
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
ec4bda4 to
af45337
Compare
Establishes a private, platform-aware cache root outside the contributor checkout for review runs. Squashes the feature commit and its follow-up refactor simplifying the cache platform defaults.
c5d9c2c to
baa4f91
Compare
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/__tests__/review-cache-root.test.mts (2)
137-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache-base platform coverage is Linux-only.
Only the linux
XDG_CACHE_HOMEbranch is exercised here. The stack context describes this layer's dependency as "Selects the platform cache base" and the PR summary claims support for "platform/XDG cache locations" generally — macOS (~/Library/Caches) and Windows (%LOCALAPPDATA%) default-selection branches have no coverage in this file. Consider adding equivalentplatform: 'darwin'/platform: 'win32'cases to lock in that contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/review-cache-root.test.mts` around lines 137 - 147, Add equivalent reviewCacheRoot tests for platform values 'darwin' and 'win32' in review-cache-root.test.mts, covering the default macOS ~/Library/Caches and Windows %LOCALAPPDATA% cache-base selection branches. Use the existing test setup and assertions to verify each platform resolves to its documented cache location without relying on shell HOME expansion.
108-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the multi-root canonicalization assertion.
This only asserts
existsSync(...)istrue, i.e. that the call doesn't throw. It doesn't verify canonicalization actually works (e.g. that the derived identity is stable/deterministic for a repo with multiple unrelated root commits), which is what the test name implies is being covered.♻️ Suggested strengthening
- expect( - existsSync( - reviewCacheRoot(fx.target, physicalTemp('multi-root-runtime'), { - cacheBase: physicalTemp('multi-root-base'), - }), - ), - ).toBe(true); + const cacheBase = physicalTemp('multi-root-base'); + const first = reviewCacheRoot(fx.target, physicalTemp('multi-root-runtime-one'), { cacheBase }); + const second = reviewCacheRoot(fx.target, physicalTemp('multi-root-runtime-two'), { cacheBase }); + expect(second).toBe(first); + expect(existsSync(first)).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/review-cache-root.test.mts` around lines 108 - 135, Strengthen the assertion around reviewCacheRoot in the multi-root test by calling it repeatedly with the same repository and cache inputs and verifying the returned canonical cache root is identical each time. Preserve the existing multi-root setup and ensure the assertion validates the derived identity, not merely that the path exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/__tests__/review-cache-root.test.mts`:
- Around line 195-213: Update the symlink-redirection assertion in the
reviewCacheRoot test to check the redirected cache directory or path that
reviewCacheRoot would create if the symlink were followed, rather than checking
redirected/checkpoint. Keep the existing rejection expectation and verify that
no cache-related path is created under redirected.
In `@cli/lib/ship/review/cache/root.mts`:
- Around line 29-41: Update assertPhysicalDirectory so the
realpathSync(requested) comparison is normalized for Windows path casing and
drive-letter differences, or bypass that equality check on win32. Preserve the
existing directory and symlink validation, error propagation, and
physical-directory failure behavior.
---
Nitpick comments:
In `@cli/__tests__/review-cache-root.test.mts`:
- Around line 137-147: Add equivalent reviewCacheRoot tests for platform values
'darwin' and 'win32' in review-cache-root.test.mts, covering the default macOS
~/Library/Caches and Windows %LOCALAPPDATA% cache-base selection branches. Use
the existing test setup and assertions to verify each platform resolves to its
documented cache location without relying on shell HOME expansion.
- Around line 108-135: Strengthen the assertion around reviewCacheRoot in the
multi-root test by calling it repeatedly with the same repository and cache
inputs and verifying the returned canonical cache root is identical each time.
Preserve the existing multi-root setup and ensure the assertion validates the
derived identity, not merely that the path exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 105578c2-6532-4745-afe7-0f3c339ee9ab
📒 Files selected for processing (2)
cli/__tests__/review-cache-root.test.mtscli/lib/ship/review/cache/root.mts
| it('rejects cache overlap, relative bases, and symlink redirection', () => { | ||
| const fx = initRepository(physicalTemp('unsafe-repo')); | ||
| const temp = physicalTemp('unsafe-runtime'); | ||
| expect(() => reviewCacheRoot(fx.target, temp, { cacheBase: fx.target })).toThrow( | ||
| /must live outside the target Git worktree/, | ||
| ); | ||
| expect(() => reviewCacheRoot(fx.target, temp, { cacheBase: 'relative/cache' })).toThrow( | ||
| /cache base must be an absolute path/, | ||
| ); | ||
|
|
||
| const cacheBase = physicalTemp('symlink-base'); | ||
| const redirected = physicalTemp('symlink-destination'); | ||
| mkdirSync(join(cacheBase, 'devkit')); | ||
| symlinkSync(redirected, join(cacheBase, 'devkit/review'), 'dir'); | ||
| expect(() => reviewCacheRoot(fx.target, temp, { cacheBase })).toThrow( | ||
| /persistent review cache must be a physical directory/, | ||
| ); | ||
| expect(existsSync(join(redirected, 'checkpoint'))).toBe(false); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- test file outline ---\n'
ast-grep outline cli/__tests__/review-cache-root.test.mts --view expanded || true
printf '\n--- relevant test file excerpt ---\n'
sed -n '160,250p' cli/__tests__/review-cache-root.test.mts
printf '\n--- search reviewCacheRoot / ensurePhysicalDirectory ---\n'
rg -n "reviewCacheRoot|ensurePhysicalDirectory|checkpoint" cli -SRepository: norvalbv/devkit
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- locate implementation files ---\n'
fd -a "review-cache-root" cli .
fd -a "physical" cli .
fd -a "directory" cli .
printf '\n--- inspect test file around target lines ---\n'
nl -ba cli/__tests__/review-cache-root.test.mts | sed -n '190,235p'
printf '\n--- inspect implementation symbols if file exists ---\n'
for f in $(git ls-files | rg 'review-cache-root|physical.*directory|ensurePhysicalDirectory|cache root|review cache'); do
echo "### $f"
nl -ba "$f" | sed -n '1,260p' | rg -n "reviewCacheRoot|ensurePhysicalDirectory|physical|checkpoint|symlink|cacheBase|mkdirSync|realpath" -n -A4 -B4 || true
doneRepository: norvalbv/devkit
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test file outline ---'
ast-grep outline cli/__tests__/review-cache-root.test.mts --view expanded || true
echo
echo '--- relevant test file excerpt ---'
nl -ba cli/__tests__/review-cache-root.test.mts | sed -n '160,250p'
echo
echo '--- search reviewCacheRoot / ensurePhysicalDirectory ---'
rg -n "reviewCacheRoot|ensurePhysicalDirectory|checkpoint" cli -SRepository: norvalbv/devkit
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target test slice ---'
sed -n '190,235p' cli/__tests__/review-cache-root.test.mts | cat -n
echo
echo '--- find implementation files ---'
fd -a "review-cache-root" .
fd -a "ensurePhysicalDirectory" .
fd -a "review-cache-root.*" .
fd -a "cache-root" .
echo
echo '--- search symbols ---'
rg -n "reviewCacheRoot|ensurePhysicalDirectory|checkpoint|symlink" cli -SRepository: norvalbv/devkit
Length of output: 25117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target test slice ---'
sed -n '190,235p' cli/__tests__/review-cache-root.test.mts | cat -n
echo
echo '--- find implementation files ---'
fd -a "review-cache-root" .
fd -a "ensurePhysicalDirectory" .
fd -a "cache-root" .
echo
echo '--- search symbols ---'
rg -n "reviewCacheRoot|ensurePhysicalDirectory|checkpoint|symlink" cli -SRepository: norvalbv/devkit
Length of output: 25057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target test slice ---'
sed -n '190,235p' cli/__tests__/review-cache-root.test.mts | cat -n
echo
echo '--- locate implementation files ---'
fd -a "review-cache-root" .
fd -a "ensurePhysicalDirectory" .
fd -a "cache-root" .
echo
echo '--- search symbols in cli ---'
rg -n "reviewCacheRoot|ensurePhysicalDirectory|checkpoint|symlink" cli -SRepository: norvalbv/devkit
Length of output: 25066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- implementation slice: review cache root ---'
sed -n '1,320p' cli/lib/ship/review/cache/root.mts | cat -n
echo
echo '--- focused search within implementation ---'
rg -n "ensurePhysicalDirectory|mkdirSync|realpathSync|checkpoint|writeFileSync|symlink|lstatSync|statSync" cli/lib/ship/review/cache/root.mts cli/lib/fs-helpers.mts -n -A4 -B4Repository: norvalbv/devkit
Length of output: 17882
Assert the redirected cache path, not checkpoint. reviewCacheRoot() never writes a checkpoint here, so existsSync(join(redirected, 'checkpoint')) doesn’t prove the symlink guard. Check the path that would be created under redirected if the link were followed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/__tests__/review-cache-root.test.mts` around lines 195 - 213, Update the
symlink-redirection assertion in the reviewCacheRoot test to check the
redirected cache directory or path that reviewCacheRoot would create if the
symlink were followed, rather than checking redirected/checkpoint. Keep the
existing rejection expectation and verify that no cache-related path is created
under redirected.
| function assertPhysicalDirectory(path: string, label: string): string { | ||
| const requested = resolve(path); | ||
| try { | ||
| const stat = lstatSync(requested); | ||
| if (!stat.isDirectory() || stat.isSymbolicLink() || realpathSync(requested) !== requested) { | ||
| fail(`${label} must be a physical directory: ${requested}`); | ||
| } | ||
| } catch (cause) { | ||
| if (cause instanceof Error && cause.message.startsWith('devkit review:')) throw cause; | ||
| fail(`${label} is not an available physical directory: ${requested}`); | ||
| } | ||
| return requested; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.realpathSync normalize drive letter or path casing differently from path.resolve on Windows?
💡 Result:
Yes, Node.js fs.realpathSync (and its variants like fs.realpathSync.native) can normalize drive letter casing differently from path.resolve on Windows [1][2][3]. The difference stems from how these two methods operate: 1. path.resolve: This method performs string manipulation on the provided path segments and the current working directory [4]. It does not interact with the filesystem [5]. Consequently, if a path is constructed with a lowercase drive letter (e.g., c:...), path.resolve will generally maintain that casing [2][3]. 2. fs.realpathSync: This method interacts with the underlying operating system to resolve the absolute path and handle symbolic links or junctions [1][6]. On Windows, this often involves internal system calls (such as GetFinalPathNameByHandle) that may return a capitalized drive letter (e.g., C:...) even if the input used a lowercase drive letter [1][2][3]. Because of this behavior, inconsistencies can arise in applications that perform strict string comparisons between paths derived from these different methods [2][3]. For example, if one part of an application uses path.resolve to generate a path and another uses fs.realpathSync, they may produce different strings for the same location on disk (e.g., c:\project vs. C:\project), which can cause issues with caching, module resolution, or configuration lookups [2][3][7]. To ensure consistency on Windows, developers often prefer using fs.realpathSync (or fs.realpathSync.native) consistently across an application to ensure all paths are resolved to their canonical, system-provided forms [2][3][6].
Citations:
- 1: On Windows, node inconsistently capitalizes drive letters when resolving junctions nodejs/node#38459
- 2: Use fs.realpathSync() rather than path.resolve() on the config path rollup/rollup#760
- 3: demensky/rollup@ce1c784
- 4: https://nodejs.org/dist/latest/docs/api/path.html
- 5: How to get properly capitalized file path on Windows? nodejs/help#15
- 6: fs: expose realpath(3) bindings nodejs/node#15776
- 7: Drive letter case is different from the
process.cwdvitejs/vite#18468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant file with line numbers and nearby context.
sed -n '1,220p' cli/lib/ship/review/cache/root.mts
# Search for any Windows-specific path handling or realpath comparisons in the repo.
rg -n "realpathSync|lstatSync|resolve\\(|case-sens|win32|platform === 'win32'|HOMEDRIVE|cacheBase" cli/lib -SRepository: norvalbv/devkit
Length of output: 17204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the path utility helpers that may influence Windows canonicalization.
sed -n '1,180p' cli/lib/ship/review/runtime-paths.mts
# Look for tests covering Windows paths, drive-letter casing, or realpath comparisons.
rg -n "realpathSync\\(|win32|drive letter|LOCALAPPDATA|HOMEDRIVE|case" cli -g '*test*' -g '*spec*' -SRepository: norvalbv/devkit
Length of output: 7372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the review cache root tests around the path invariants.
sed -n '1,280p' cli/__tests__/review-cache-root.test.mts
# Inspect any tests that explicitly skip or vary behavior on Windows in nearby review path code.
sed -n '240,320p' cli/__tests__/review-asset-runtime.test.mts
sed -n '240,320p' cli/__tests__/review-setup-runtime.test.mtsRepository: norvalbv/devkit
Length of output: 15475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' cli/__tests__/review-cache-root.test.mtsRepository: norvalbv/devkit
Length of output: 9325
Normalize the physical-directory comparison on Windows. realpathSync(requested) !== requested can reject a real directory when Windows canonicalizes the drive letter or path casing differently from resolve(). Compare normalized canonical paths, or skip this equality check on win32.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/lib/ship/review/cache/root.mts` around lines 29 - 41, Update
assertPhysicalDirectory so the realpathSync(requested) comparison is normalized
for Windows path casing and drive-letter differences, or bypass that equality
check on win32. Preserve the existing directory and symlink validation, error
propagation, and physical-directory failure behavior.
feat: isolate review cache roots
feat: isolate review cache roots
1 participant
What this PR does
Introduces a stable, checkout-external root for persistent review cache data:
Why this slice exists
Convergence caches need to survive disposable review worktrees and be shared by reviews of the same repository, but they must never add bytes to the reviewed target or collide simply because two unrelated repositories use the same folder name.
Review carefully
Explicit non-goals
Stack position
Review bottom-up. These PRs currently target their predecessor branches; do not merge a middle PR directly to main. After review, successors must be retargeted/rebased as predecessors land, unless an explicit stack-collapse landing process is agreed.
Size and evidence
Summary by CodeRabbit
New Features
devkit review.Tests