fix(windows): canonicalize permission path variants#493
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors Windows path normalization to resolve variant spellings into canonical forms before permission validation. It introduces ChangesWindows Path Canonicalization and Tool Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances Windows path normalization by introducing normalizeWindowsPath and WindowsPathOptions, which provide support for extended-length paths and improved resolution of rooted-driveless paths. These improvements are integrated into various tools to ensure consistent path handling. The review feedback suggests reducing code duplication by reusing the existing windowsPath function, removing redundant normalization calls, and making platform-dependent logic injectable to facilitate cross-platform testing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/opencode/src/tool/grep.ts (1)
55-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
assertExternalDirectoryEffectreturn value is not captured — ripgrep uses the pre-canonical pathEvery other tool changed in this PR (read, write, edit, glob, lsp, apply_patch) captures the canonical path returned by
assertExternalDirectoryEffectand uses it for downstream operations.grepis the only tool that still ignores the return value. As a result,cwdandfile(and therefore the ripgrep call) are computed from the unnormalizedsearch, which defeats the Windows canonicalization intent of this PR.🐛 Proposed fix
- yield* assertExternalDirectoryEffect(ctx, search, { + const canonSearch = (yield* assertExternalDirectoryEffect(ctx, search, { kind: info?.type === "Directory" ? "directory" : "file", - }) + })) ?? search - const cwd = info?.type === "Directory" ? search : path.dirname(search) - const file = info?.type === "Directory" ? undefined : [path.relative(cwd, search)] + const cwd = info?.type === "Directory" ? canonSearch : path.dirname(canonSearch) + const file = info?.type === "Directory" ? undefined : [path.relative(cwd, canonSearch)]Then update the
rg.searchcall to use the already-derivedcwd/file(no change needed there since they now derive fromcanonSearch).🤖 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/opencode/src/tool/grep.ts` around lines 55 - 74, The call to assertExternalDirectoryEffect in grep.ts currently ignores its return value so cwd/file are computed from the pre-canonical search path; capture the returned canonical path (e.g., const canonSearch = yield* assertExternalDirectoryEffect(...)) and then compute cwd and file from canonSearch (use path.dirname/canonSearch and path.relative as done elsewhere), and pass those derived cwd/file into rg.search so ripgrep runs against the canonicalized path.packages/opencode/src/tool/external-directory.ts (1)
21-44:⚠️ Potential issue | 🟠 MajorFix
grep.tsto capture and use the normalized path fromassertExternalDirectoryEffect.At line 64,
grep.tscallsassertExternalDirectoryEffectbut discards its return value, continuing with the pre-normalizationsearchfor downstreamrg.search()operations. This re-introduces the variant-spelling bug this PR fixes.Change:
yield* assertExternalDirectoryEffect(ctx, search, { kind: info?.type === "Directory" ? "directory" : "file", })To:
search = (yield* assertExternalDirectoryEffect(ctx, search, { kind: info?.type === "Directory" ? "directory" : "file", })) ?? searchAll other callers (
write.ts,glob.ts,read.ts,apply_patch.ts,lsp.ts,edit.ts) correctly capture and use the return value.🤖 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/opencode/src/tool/external-directory.ts` around lines 21 - 44, The call to assertExternalDirectoryEffect in grep.ts currently discards its return value, so downstream rg.search() is still using the un-normalized search path; change the call to capture and use the returned normalized path by reassigning search to the effect's result (fallback to the original search if the effect returns undefined) — this references the function assertExternalDirectoryEffect and the local variable search used before invoking rg.search().
🧹 Nitpick comments (3)
packages/core/src/filesystem.ts (2)
257-269: 💤 Low valueAmbiguity check ignores caller-supplied drive root duplicates.
resolveRootedWindowsVariantacceptsoptions.driveRootsverbatim. If callers pass duplicates (e.g.["C:\\", "c:\\"]), the same on-disk file can be probed twice and pushed tomatchestwice, triggering the ambiguity throw on a path that is not actually ambiguous. The internalwindowsDriveRoots()already de-dupes viaseen, so the gap only exists for caller-supplied roots. A cheap normalization (new Set(roots.map(r => r.toUpperCase()))) before the loop closes this off and keeps the contract symmetric with the internal source.🤖 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/filesystem.ts` around lines 257 - 269, resolveRootedWindowsVariant can push duplicate candidate paths when caller-supplied options.driveRoots contains case-duplicates (e.g. "C:\\" and "c:\\") causing a false ambiguity; normalize and de-duplicate the driveRoots before iterating (e.g. derive a set from options.driveRoots or windowsDriveRoots() using a case-normalization like toUpperCase()) so the loop over roots only probes unique normalized roots and the matches array cannot contain duplicate identical candidates; update references to options.driveRoots and windowsDriveRoots() in resolveRootedWindowsVariant accordingly.
243-277: 💤 Low valueApproach LGTM; the ambiguity throw is a plain
Error.The pipeline ordering is correct (
normalizeWindowsShellInputonly matches/-prefixed forms,stripExtendedLengthPrefixonly matches\\?\forms, so they don't fight),isRootedDrivelesscorrectly excludes UNC via the negative lookahead, and the existing-match → base-bound → cwd-bound fallback chain matches the PR objective.One small note:
throw new Error("Ambiguous Windows path …")here is a plainError, not anAppFileSystem.FileSystemError. SincenormalizePath/normalizeWindowsPathare now invoked fromEffect.fncallers in the opencode tool layer (see comment onexternal-directory.ts), a typed error class would round-trip more cleanly through Effect's error channel. Not blocking — the throw site itself is correct.🤖 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/filesystem.ts` around lines 243 - 277, In resolveRootedWindowsVariant, replace the plain throw new Error(...) with a typed filesystem error (e.g., throw new AppFileSystem.FileSystemError(...)) so callers (including Effect.fn in external-directory.ts) can round-trip the error through Effect's error channel; update the resolveRootedWindowsVariant function to construct the FileSystemError with the same descriptive message (including the ambiguous path p) and add the necessary import for AppFileSystem.FileSystemError at the top of the module.packages/opencode/src/tool/external-directory.ts (1)
24-32: ⚡ Quick winAmbiguous-path throw becomes a defect inside
Effect.fn.
AppFileSystem.normalizePath/normalizePathPatternthrow a plainError("Ambiguous Windows path …")when multiple drive roots match. Because these are synchronous throws insideEffect.fn, the failure surfaces as an unchecked defect and the tool effect dies rather than producing a clean, user-visible error — which is the new failure mode the PR explicitly introduces ("ambiguous multi-drive matches now fail with a clear error instead of picking the first drive"). Consider trapping the throw so it crosses the Effect boundary as a typed failure onctx.ask/ the tool's error channel.♻️ Sketch: convert the sync throw into a typed Effect failure
- const full = process.platform === "win32" ? AppFileSystem.normalizePath(target, { base: ins.directory }) : target + const full = + process.platform === "win32" + ? yield* Effect.try({ + try: () => AppFileSystem.normalizePath(target, { base: ins.directory }), + catch: (cause) => new SomeToolError({ method: "normalizePath", cause }), + }) + : target if (options?.bypass) return full if (Instance.containsPath(full, ins)) return full const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) - const glob = - process.platform === "win32" - ? AppFileSystem.normalizePathPattern(path.join(dir, "*"), { base: ins.directory }) - : path.join(dir, "*").replaceAll("\\", "/") + const glob = + process.platform === "win32" + ? yield* Effect.try({ + try: () => AppFileSystem.normalizePathPattern(path.join(dir, "*"), { base: ins.directory }), + catch: (cause) => new SomeToolError({ method: "normalizePathPattern", cause }), + }) + : path.join(dir, "*").replaceAll("\\", "/")As per coding guidelines: "Use
Schema.TaggedErrorClassfor typed errors in Effect schemas".🤖 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/opencode/src/tool/external-directory.ts` around lines 24 - 32, The calls to AppFileSystem.normalizePath and AppFileSystem.normalizePathPattern can synchronously throw an "Ambiguous Windows path" Error inside Effect.fn causing an unchecked defect; wrap those calls so any thrown Error is caught and converted into a typed Effect failure (use a Schema.TaggedErrorClass, e.g. AmbiguousPathError) and return Effect.fail (or otherwise surface via the tool's ctx.ask/error channel) instead of letting the throw escape; update the logic around the code that computes full and glob (referencing AppFileSystem.normalizePath, AppFileSystem.normalizePathPattern, the Effect.fn wrapper around this code, and the surrounding flow that uses Instance.containsPath and options?.bypass) to catch the sync throw and map it to the tagged error.
🤖 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/opencode/test/tool/external-directory.test.ts`:
- Around line 25-31: The withWin32Platform helper currently overrides
process.platform using Object.defineProperty without setting configurable, which
prevents restoring and throws when redefining; update withWin32Platform to
capture the original property descriptor (via
Object.getOwnPropertyDescriptor(process, "platform")), then call
Object.defineProperty(process, "platform", { value: "win32", configurable: true
}) and in the finally block restore the original descriptor (or the original
value and flags) using the saved descriptor to reliably revert process.platform
without errors.
---
Outside diff comments:
In `@packages/opencode/src/tool/external-directory.ts`:
- Around line 21-44: The call to assertExternalDirectoryEffect in grep.ts
currently discards its return value, so downstream rg.search() is still using
the un-normalized search path; change the call to capture and use the returned
normalized path by reassigning search to the effect's result (fallback to the
original search if the effect returns undefined) — this references the function
assertExternalDirectoryEffect and the local variable search used before invoking
rg.search().
In `@packages/opencode/src/tool/grep.ts`:
- Around line 55-74: The call to assertExternalDirectoryEffect in grep.ts
currently ignores its return value so cwd/file are computed from the
pre-canonical search path; capture the returned canonical path (e.g., const
canonSearch = yield* assertExternalDirectoryEffect(...)) and then compute cwd
and file from canonSearch (use path.dirname/canonSearch and path.relative as
done elsewhere), and pass those derived cwd/file into rg.search so ripgrep runs
against the canonicalized path.
---
Nitpick comments:
In `@packages/core/src/filesystem.ts`:
- Around line 257-269: resolveRootedWindowsVariant can push duplicate candidate
paths when caller-supplied options.driveRoots contains case-duplicates (e.g.
"C:\\" and "c:\\") causing a false ambiguity; normalize and de-duplicate the
driveRoots before iterating (e.g. derive a set from options.driveRoots or
windowsDriveRoots() using a case-normalization like toUpperCase()) so the loop
over roots only probes unique normalized roots and the matches array cannot
contain duplicate identical candidates; update references to options.driveRoots
and windowsDriveRoots() in resolveRootedWindowsVariant accordingly.
- Around line 243-277: In resolveRootedWindowsVariant, replace the plain throw
new Error(...) with a typed filesystem error (e.g., throw new
AppFileSystem.FileSystemError(...)) so callers (including Effect.fn in
external-directory.ts) can round-trip the error through Effect's error channel;
update the resolveRootedWindowsVariant function to construct the FileSystemError
with the same descriptive message (including the ambiguous path p) and add the
necessary import for AppFileSystem.FileSystemError at the top of the module.
In `@packages/opencode/src/tool/external-directory.ts`:
- Around line 24-32: The calls to AppFileSystem.normalizePath and
AppFileSystem.normalizePathPattern can synchronously throw an "Ambiguous Windows
path" Error inside Effect.fn causing an unchecked defect; wrap those calls so
any thrown Error is caught and converted into a typed Effect failure (use a
Schema.TaggedErrorClass, e.g. AmbiguousPathError) and return Effect.fail (or
otherwise surface via the tool's ctx.ask/error channel) instead of letting the
throw escape; update the logic around the code that computes full and glob
(referencing AppFileSystem.normalizePath, AppFileSystem.normalizePathPattern,
the Effect.fn wrapper around this code, and the surrounding flow that uses
Instance.containsPath and options?.bypass) to catch the sync throw and map it to
the tagged error.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb12a4f8-3dad-4dc2-ba7b-a665fa903d25
📒 Files selected for processing (11)
packages/core/src/filesystem.tspackages/core/test/filesystem/normalize-path.test.tspackages/opencode/src/tool/apply_patch.tspackages/opencode/src/tool/edit.tspackages/opencode/src/tool/external-directory.tspackages/opencode/src/tool/glob.tspackages/opencode/src/tool/grep.tspackages/opencode/src/tool/lsp.tspackages/opencode/src/tool/read.tspackages/opencode/src/tool/write.tspackages/opencode/test/tool/external-directory.test.ts
|
Follow-up for the new review round:
I did not fold the typed-error/docstring suggestions into this PR because they are not required for #427 correctness and would widen the error-contract surface beyond this path-canonicalization fix. |
1 participant
Summary
Canonicalizes the remaining Windows permission path variants for #427 correctness and makes guarded tools use the same canonical path for permission checks and actual filesystem/LSP/ripgrep operations.
Changes included:
assertExternalDirectoryEffect.read,write,edit,apply_patch,glob,grep, andlspboundaries.Why
#431 fixed the existing rooted-but-driveless case, but #427 still had correctness gaps where Windows path variants could produce different permission patterns or let the permission path diverge from the actual tool path. That means a user could approve one spelling with
alwaysand still be prompted again, or approve coverage for a path that is not the one the tool later uses.This PR fixes the correctness portion at the path contract and tool boundary. It intentionally does not implement drive-root probe caching.
Related Issue
Addresses the correctness portion of #427.
Does not close #427 unless the remaining drive-root probe caching follow-up is split into a separate issue.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
\\?\D:\...,\\?\UNC\server\share\..., non-existing rooted-but-driveless targets, and ambiguous multi-drive matches.assertExternalDirectoryEffectreturns.Risk Notes
How To Verify
Screenshots or Recordings
None. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Refactor
Tests