refactor: add snapshot path context facade#1021
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
nicotsx
force-pushed
the
refactor/snapshot-path-context
branch
2 times, most recently
from
a22fbe1 to
474e140
Compare
nicotsx
force-pushed
the
fix/volume-browser-path-codec
branch
from
e71a6db to
45b54e8
Compare
nicotsx
force-pushed
the
refactor/snapshot-path-context
branch
from
474e140 to
bce5c5c
Compare
nicotsx
force-pushed
the
refactor/snapshot-path-context
branch
from
bce5c5c to
037a98b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 037a98bd5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const canInferWindowsSourceKind = sourcePathKind === undefined && !hasNonRootPosixPaths; | ||
|
|
||
| const shouldUseWindowsCommonAncestor = | ||
| hasWindowsResticPaths && (sourcePathKind === "windows" || canInferWindowsSourceKind); |
There was a problem hiding this comment.
When restore() is called without sourcePathKind (checked the current repository restore flow, which still passes only basePath/include), this inference treats any POSIX snapshot rooted under a single-letter directory as Windows. For example, selecting /C/projects/App/file and /C/projects/app/data from a Linux snapshot makes createRestorePathArgs choose /C/projects/App as the restore root and include data, so the lower-case /C/projects/app branch is outside the restored subtree and gets skipped. Only use the Windows common-ancestor path when the caller has explicitly identified the source as Windows.
Useful? React with 👍 / 👎.
| return { | ||
| restoreArg, | ||
| includePatterns: getIncludePatterns(includes, target, restoreRoot, sourcePathKind), | ||
| excludePatterns: exclude?.length ? getPathPatterns(exclude, target, restoreRoot, sourcePathKind) : [], |
There was a problem hiding this comment.
For callers that pass restic exclude globs or relative patterns (the API already accepts exclude, and restic restore documents --exclude=[] exclude a pattern), this path conversion rewrites the pattern as a snapshot path before passing it to restic. For example, restoring /var/lib/app with exclude: ["*.tmp"] now emits --exclude ../../../*.tmp for a custom target (or /*.tmp for /), where the previous code passed *.tmp, so the exclusion no longer matches the files it was meant to skip.
Useful? React with 👍 / 👎.
| export const toResticSnapshotPath = (value: string): string => | ||
| windowsHostPathToResticSnapshotPath(value) ?? normalizeAbsolutePath(value); |
There was a problem hiding this comment.
When a snapshot contains legal path names that look URL-encoded or contain backslashes, routing every non-Windows-host path through normalizeAbsolutePath rewrites the name before restic sees it. For example, selecting /tmp/foo%2Fbar.txt now plans snapshot:/tmp/foo with include bar.txt (or /tmp/foo/bar.txt for a root target), whereas the existing restore logic would pass the literal %2F path, so files with literal percent-escape sequences or POSIX backslashes can no longer be restored by selection.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/core/src/restic/helpers/snapshot-path-context.ts`:
- Around line 146-160: Keep the Windows query base when original restore is
unavailable by changing the non-Windows branch in snapshot-path-context’s source
plan logic so it disables only original-location restore without falling back to
rootSourcePlan(true, "windows"). Update the hasWindowsPaths / targetPathKind
handling in the path-planning function to preserve the result from
findWindowsResticCommonAncestor(snapshotPaths) as queryBasePath, while setting
canRestoreOriginal to false and keeping the Windows source path kind for
browser.initialQueryPath() and dump.plan().
In `@packages/core/src/restic/helpers/snapshot-paths.ts`:
- Around line 7-10: The Windows snapshot-path detection in
isWindowsResticSnapshotPath is too broad because it treats any normalized path
starting with a single letter segment as Windows, which misclassifies POSIX
paths like /a/foo and affects toWindowsResticSnapshotPath, isWindowsComparison,
findResticCommonAncestor, and createSourcePlan. Tighten the predicate in
snapshot-paths.ts so ambiguous single-letter roots do not opt into Windows
semantics unless the path kind is explicitly known, and update the callers that
currently rely on omitted sourcePathKind to preserve POSIX behavior for
/a/...-style paths.
In `@packages/core/src/utils/path.ts`:
- Around line 1-4: Update isWindowsHostPath and normalizeWindowsHostPath in
path.ts to reject bare drive letters like “C:” instead of treating them as
absolute Windows roots. Tighten the drive-path check so only true rooted forms
such as “C:\” or “C:/” pass, and keep normalizeWindowsHostPath from rewriting
drive-relative input into a root path. Verify the behavior through the existing
path helpers that consume normalizeWindowsHostPath so browse/restore planning
never sees a drive-relative path as `/C`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 330800ee-29b1-44eb-8fbd-ae213fe0dc3d
📒 Files selected for processing (12)
packages/core/src/restic/commands/__tests__/restore.test.tspackages/core/src/restic/commands/restore.tspackages/core/src/restic/helpers/__tests__/restore-paths.test.tspackages/core/src/restic/helpers/__tests__/snapshot-path-context.test.tspackages/core/src/restic/helpers/restore-paths.tspackages/core/src/restic/helpers/snapshot-path-context.tspackages/core/src/restic/helpers/snapshot-paths.tspackages/core/src/restic/index.tspackages/core/src/restic/server.tspackages/core/src/utils/__tests__/path.test.tspackages/core/src/utils/index.tspackages/core/src/utils/path.ts
| if (hasWindowsPaths) { | ||
| if (targetPathKind !== "windows") { | ||
| return rootSourcePlan(true, "windows"); | ||
| } | ||
|
|
||
| const basePath = findWindowsResticCommonAncestor(snapshotPaths); | ||
| if (!basePath) return rootSourcePlan(true, "windows"); | ||
|
|
||
| return { | ||
| sourcePathKind: "windows", | ||
| queryBasePath: basePath, | ||
| originalRestoreBasePath: basePath, | ||
| customRestoreBasePath: "/", | ||
| requiresCustomTarget: false, | ||
| canRestoreOriginal: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the Windows query base even when original restore is unavailable.
When the snapshot paths are Windows but targetPlatform is non-Windows, Line 147 returns rootSourcePlan(true, "windows"), which collapses queryBasePath to /. That makes browser.initialQueryPath() and dump.plan() lose the real common ancestor even though only original-location restore needs to be disabled here.
Proposed fix
if (hasWindowsPaths) {
- if (targetPathKind !== "windows") {
- return rootSourcePlan(true, "windows");
- }
-
const basePath = findWindowsResticCommonAncestor(snapshotPaths);
if (!basePath) return rootSourcePlan(true, "windows");
return {
sourcePathKind: "windows",
queryBasePath: basePath,
originalRestoreBasePath: basePath,
customRestoreBasePath: "/",
- requiresCustomTarget: false,
- canRestoreOriginal: true,
+ requiresCustomTarget: targetPathKind !== "windows",
+ canRestoreOriginal: targetPathKind === "windows",
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (hasWindowsPaths) { | |
| if (targetPathKind !== "windows") { | |
| return rootSourcePlan(true, "windows"); | |
| } | |
| const basePath = findWindowsResticCommonAncestor(snapshotPaths); | |
| if (!basePath) return rootSourcePlan(true, "windows"); | |
| return { | |
| sourcePathKind: "windows", | |
| queryBasePath: basePath, | |
| originalRestoreBasePath: basePath, | |
| customRestoreBasePath: "/", | |
| requiresCustomTarget: false, | |
| canRestoreOriginal: true, | |
| if (hasWindowsPaths) { | |
| const basePath = findWindowsResticCommonAncestor(snapshotPaths); | |
| if (!basePath) return rootSourcePlan(true, "windows"); | |
| return { | |
| sourcePathKind: "windows", | |
| queryBasePath: basePath, | |
| originalRestoreBasePath: basePath, | |
| customRestoreBasePath: "/", | |
| requiresCustomTarget: targetPathKind !== "windows", | |
| canRestoreOriginal: targetPathKind === "windows", |
🤖 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 `@packages/core/src/restic/helpers/snapshot-path-context.ts` around lines 146 -
160, Keep the Windows query base when original restore is unavailable by
changing the non-Windows branch in snapshot-path-context’s source plan logic so
it disables only original-location restore without falling back to
rootSourcePlan(true, "windows"). Update the hasWindowsPaths / targetPathKind
handling in the path-planning function to preserve the result from
findWindowsResticCommonAncestor(snapshotPaths) as queryBasePath, while setting
canRestoreOriginal to false and keeping the Windows source path kind for
browser.initialQueryPath() and dump.plan().
| const windowsResticSnapshotPathPattern = /^\/[A-Za-z](?:\/|$)/; | ||
|
|
||
| export const isWindowsResticSnapshotPath = (value: string): boolean => | ||
| windowsResticSnapshotPathPattern.test(normalizeAbsolutePath(value)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Single-letter POSIX roots are being inferred as Windows.
^/[A-Za-z](?:/|$) also matches valid POSIX paths like /a/foo. Once that happens, toWindowsResticSnapshotPath(), isWindowsComparison(), and findResticCommonAncestor() all switch to Windows rules when sourcePathKind is omitted. For example, /a/foo + /a/Foo/bar currently produce bar and /a/foo instead of the POSIX-correct ../Foo/bar and /a. createSourcePlan() also consumes this predicate on Windows targets, so the wrong source kind can leak into restore/dump planning. Please require explicit path kind for this ambiguous shape, or carry stronger evidence than the normalized /<letter>/... form before enabling Windows semantics.
Also applies to: 28-33, 91-97, 152-168
🤖 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 `@packages/core/src/restic/helpers/snapshot-paths.ts` around lines 7 - 10, The
Windows snapshot-path detection in isWindowsResticSnapshotPath is too broad
because it treats any normalized path starting with a single letter segment as
Windows, which misclassifies POSIX paths like /a/foo and affects
toWindowsResticSnapshotPath, isWindowsComparison, findResticCommonAncestor, and
createSourcePlan. Tighten the predicate in snapshot-paths.ts so ambiguous
single-letter roots do not opt into Windows semantics unless the path kind is
explicitly known, and update the callers that currently rely on omitted
sourcePathKind to preserve POSIX behavior for /a/...-style paths.
| export const isWindowsHostPath = (value: string): boolean => /^[A-Za-z]:(?:[\\/]|$)/.test(value); | ||
|
|
||
| export const normalizeWindowsHostPath = (value: string): string | undefined => { | ||
| if (!isWindowsHostPath(value)) return undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject bare drive letters here.
C: is drive-relative on Windows, not an absolute root. This regex accepts it, and normalizeWindowsHostPath("C:") then rewrites it to C:\. Downstream helpers now treat a drive-relative input as the drive root (/C), so browse/restore planning can target the wrong location.
Proposed fix
-export const isWindowsHostPath = (value: string): boolean => /^[A-Za-z]:(?:[\\/]|$)/.test(value);
+export const isWindowsHostPath = (value: string): boolean => /^[A-Za-z]:[\\/]/.test(value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const isWindowsHostPath = (value: string): boolean => /^[A-Za-z]:(?:[\\/]|$)/.test(value); | |
| export const normalizeWindowsHostPath = (value: string): string | undefined => { | |
| if (!isWindowsHostPath(value)) return undefined; | |
| export const isWindowsHostPath = (value: string): boolean => /^[A-Za-z]:[\\/]/.test(value); | |
| export const normalizeWindowsHostPath = (value: string): string | undefined => { | |
| if (!isWindowsHostPath(value)) return undefined; |
🤖 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 `@packages/core/src/utils/path.ts` around lines 1 - 4, Update isWindowsHostPath
and normalizeWindowsHostPath in path.ts to reject bare drive letters like “C:”
instead of treating them as absolute Windows roots. Tighten the drive-path check
so only true rooted forms such as “C:\” or “C:/” pass, and keep
normalizeWindowsHostPath from rewriting drive-relative input into a root path.
Verify the behavior through the existing path helpers that consume
normalizeWindowsHostPath so browse/restore planning never sees a drive-relative
path as `/C`.
Snapshot browsing, restore planning, dump preparation, and Windows path handling were sharing the same path rules through many helpers and callers. This creates one core facade that owns source-path detection, restic path conversion, display path, restore targets, and dump
nicotsx
force-pushed
the
refactor/snapshot-path-context
branch
from
037a98b to
1a4fb41
Compare
2 participants
Snapshot browsing, restore planning, dump preparation, and Windows path
handling were sharing the same path rules through many helpers and
callers. This creates one core facade that owns source-path detection,
restic path conversion, display path, restore targets, and dump
Summary by CodeRabbit
New Features
Bug Fixes
Tests