fix(windows): canonicalize rooted-but-driveless paths in permission helpers by Astro-Han · Pull Request #431 · Astro-Han/pawwork

Astro-Han · 2026-05-04T08:56:21Z

Summary

assertExternalDirectory and read both rely on AppFileSystem.normalizePath to canonicalize a user-supplied path before deriving permission globs. On Windows that fell through to path.resolve, which silently bound rooted-but-driveless paths (e.g. /users/runner/...) to the cwd's drive. When the file lived on a different drive (typical on GitHub runners: temp on C: vs workspace on D:), the resulting glob and fs.open path pointed at the wrong drive.

Probe each known drive root for an existing path before falling back to win32.resolve. The helper now lives in @opencode-ai/core/filesystem so AppFileSystem.normalizePath and Filesystem.normalizePath share one implementation; opencode's Filesystem thin-wraps it.

Why

#427 carved two real Windows runtime bugs out of the windows-advisory failures (the rest were test-fixture issues, fixed in #429). Without this change tool.assertExternalDirectory > normalizes Windows path variants to one glob and tool.read external_directory permission > normalizes read permission paths on Windows both fail because the permission glob uses the cwd's drive instead of the file's actual drive.

Related Issue

Fixes the rooted-but-driveless cases in #427. UNC \\?\ prefixes, ambiguous multi-drive matches, and write-target (non-existing path) handling are deliberately out of scope — kept open for a follow-up if windows-advisory surfaces them.

Human Review Status

Pending.

Review Focus

The drive-probe heuristic in normalizeWindowsAbsolutePath and windowsDriveRoots. The probe runs sync existsSync per drive; that adds I/O to a previously pure path operation, but only on Windows and only for rooted-but-driveless inputs (a narrow code path).
Removing the duplicate logic in packages/opencode/src/util/filesystem.ts — Filesystem.normalizePath/resolve/etc. are now re-exports of the core helpers. Existing callers (Filesystem.contains, Filesystem.overlaps, LSP, project/instance, plugin/install) keep working through the namespace.
New packages/core/test/filesystem/normalize-path.test.ts covers Git Bash, Cygwin, WSL, and bare driveless variants plus an explicit guard that non-existing rooted-driveless paths still bind to cwd's drive.

Risk Notes

Windows-only behavior change. macOS/Linux paths are returned unchanged.
The drive-probe scans up to 26 drives via existsSync. On a hung network drive this could block; in practice CI runners only have C/D so the cost is bounded.
For non-existing rooted-driveless paths (e.g. write targets that don't exist yet), the probe finds nothing and falls back to win32.resolve, which still binds to cwd's drive. This matches prior behavior; only the existing-path case is improved.
Drive probing runs windowsDriveRoots() per call (no caching). Hot paths that previously assumed normalizePath was pure (LSP, ripgrep) now do up to 26 existsSync syscalls per Windows call. Cache invalidation is a follow-up.

How To Verify

bun --cwd packages/opencode test test/tool/external-directory.test.ts test/tool/read.test.ts: 41 pass / 0 fail (macOS)
bun --cwd packages/opencode test test/util/filesystem.test.ts test/file/path-traversal.test.ts test/tool/edit.test.ts test/tool/write.test.ts test/tool/glob.test.ts test/tool/grep.test.ts: 133 pass / 0 fail (macOS)
bun --cwd packages/opencode test test/tool/bash.test.ts test/tool/apply_patch.test.ts test/tool/lsp.test.ts test/tool/truncation.test.ts: 66 pass / 0 fail (macOS)
bun --cwd packages/core test test/filesystem/normalize-path.test.ts: 1 pass / 7 skip on macOS, all variants exercised on Windows
windows-advisory CI: triggered manually on this branch (run 25311230257) — must be green before merge

Screenshots or Recordings

None — no UI changes.

Checklist

Summary by CodeRabbit

Bug Fixes
- Safer Windows absolute-path resolution: probe known drive roots for rooted-but-driveless paths before falling back to standard resolution.
Refactor
- Consolidated filesystem path utilities into a shared core implementation; opencode now delegates normalization and resolution to the central filesystem module.
Tests
- Added cross-platform tests covering Windows-specific path normalization variants and pattern-preservation behavior.

…probe assertExternalDirectory and read both call AppFileSystem.normalizePath, but on Windows that fell through to path.resolve which silently bound rooted-but- driveless paths (/users/runner/...) to cwd's drive. When the file lived on a different drive (typical on GitHub runners: temp on C: vs workspace on D:) the resulting permission glob used the wrong drive. Probe each known drive root for an existing path before falling back to win32.resolve. Move the helper into core/filesystem and delegate from opencode/util/filesystem so both paths share a single implementation. Closes #427

coderabbitai · 2026-05-04T08:56:33Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 627dbd79-a253-476f-8e8e-93475ffe307c

📥 Commits

Reviewing files that changed from the base of the PR and between 36a7bfc and af8a4b7.

📒 Files selected for processing (1)

packages/core/test/filesystem/normalize-path.test.ts

✅ Files skipped from review due to trivial changes (1)

packages/core/test/filesystem/normalize-path.test.ts

📝 Walkthrough

Walkthrough

Core filesystem Windows normalization now probes known drive roots for rooted-but-driveless absolute paths before falling back to win32.resolve. The opencode package delegates its path-normalization utilities to core's AppFileSystem. New tests cover Git Bash, Cygwin, WSL, rooted-but-driveless, fallback, and glob-preservation cases.

Changes

Windows Path Normalization Consolidation

Layer / File(s)	Summary
Core imports & wiring `packages/core/src/filesystem.ts`	Added `win32` and `existsSync` imports to enable drive-root probing.
Core normalization entrypoints `packages/core/src/filesystem.ts`	`normalizePath` and `resolve` (Windows branch) now call `normalizeWindowsAbsolutePath(windowsPath(path))` instead of direct `path.resolve`/`pathResolve`.
Core Windows helpers `packages/core/src/filesystem.ts`	Added `normalizeWindowsAbsolutePath`, `resolveRootedWindowsVariant`, and `windowsDriveRoots` to detect rooted-but-driveless inputs, probe drive roots with `existsSync`, and normalize candidates via `win32.normalize`/`win32.resolve`.
Public API delegation `packages/opencode/src/util/filesystem.ts`	Removed local implementations of `normalizePath`, `normalizePathPattern`, `resolve`, and `windowsPath`; replaced them with exported const aliases referencing `@opencode-ai/core/filesystem` (AppFileSystem).
Internal usage update `packages/opencode/src/util/filesystem.ts`	`comparablePath()` on Windows now calls `AppFileSystem.normalizePath(p)` instead of prior local logic.
Tests `packages/core/test/filesystem/normalize-path.test.ts`	Added tests exercising AppFileSystem.normalizePath and normalizePathPattern across Windows variants (Git Bash, Cygwin, WSL), rooted-but-driveless probing, fallback behavior, and glob preservation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

[Bug] external_directory and read permissions don't fully canonicalize Windows path variants #427: Centralizes and improves Windows canonicalization, addressing missing canonicalization for external_directory/read.

Possibly related PRs

test(windows): unblock advisory by fixing 8 fixture incompatibilities #429: Addresses similar Windows path resolution/normalization quirks (drive-prefix and resolve behavior).

Suggested labels

P2

Poem

🐰 I hopped through roots where paths went stray,

probed each drive until the fog gave way.
Core and util now sing in tune,
Windows paths find home by noon.
🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: fixing Windows path canonicalization for rooted-but-driveless paths in permission helpers.
Description check	✅ Passed	The pull request description comprehensively addresses all required template sections including summary, why, related issue, review focus, risk notes, verification steps, and completed checklist.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch claude/fix-windows-permission-paths-427

_{Review rate limit: 9/10 reviews remaining, refill in 6 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist

Code Review

This pull request refactors filesystem path normalization by centralizing the logic within the core package and reusing it across the codebase. It specifically improves Windows path handling for rooted but driveless paths by probing available drive roots. Feedback was provided regarding the performance of the windowsDriveRoots function, suggesting that caching the drive list would be more efficient for bulk operations.

Drive probe only repairs existing rooted-driveless paths; surface that contract as a test asserting non-existent targets still bind to cwd's drive. Cover Git Bash /c/, Cygwin /cygdrive/c/, WSL /mnt/c/ entry points plus the bare /users/... shape that motivated the fix. Tests are no-ops on macOS/Linux where normalizePath is the identity.

Astro-Han · 2026-05-04T09:31:31Z

windows-advisory dispatch run 25311230257 finished green — all 5 shards (changes, app, desktop, config-project, session, server-tools) passed on this branch. Server-tools was the last shard still red on dev (held by the two #427 tests); now confirmed unblocked.

Reviewer-recommended merge gate satisfied:

bun --cwd packages/opencode test test/tool/external-directory.test.ts test/tool/read.test.ts runs as part of unit-windows-opencode-server-tools and passed.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/core/test/filesystem/normalize-path.test.ts (1)
17-33: 🏗️ Heavy lift

Force this case to cross a drive boundary.

On runners where os.tmpdir() shares a drive with process.cwd(), this can still pass even if the new drive-probing logic is broken, because the legacy fallback can produce the same result. Consider pinning the fixture to a different drive or mocking the drive-root probe so this actually exercises the regression path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/test/filesystem/normalize-path.test.ts` around lines 17 - 33,
The test currently can pass on same-drive runners; modify the test for
"normalizePath probes drive roots for rooted-but-driveless paths to existing
files" to force crossing a drive boundary so the drive-probing branch in
AppFileSystem.normalizePath is exercised: either create the temporary fixture on
a different drive than process.cwd() (e.g., build the tmpdir path by prepending
a different drive letter) or mock/stub the internal drive-root probe used by
AppFileSystem.normalizePath (or the helper that enumerates drive roots) so the
function must probe other drives; update the test to assert the normalized path
equals the file only when the probe path is exercised and clean up the fixture
in the finally block.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/test/filesystem/normalize-path.test.ts`:
- Around line 111-120: The test currently only asserts the normalized path has a
drive letter; change it to assert the exact expected fallback path by
constructing the expected string from the current working directory's drive and
the original driveless path. Compute the expected drive prefix from
process.cwd() (first two chars like "C:"), append the driveless path with its
leading slash removed and with appropriate Windows separators (use path.join or
equivalent) and then replace the toMatch assertion with a strict
equality/assertion comparing AppFileSystem.normalizePath(driveless) to that
expected path so the test fails if the wrong drive is chosen.

---

Nitpick comments:
In `@packages/core/test/filesystem/normalize-path.test.ts`:
- Around line 17-33: The test currently can pass on same-drive runners; modify
the test for "normalizePath probes drive roots for rooted-but-driveless paths to
existing files" to force crossing a drive boundary so the drive-probing branch
in AppFileSystem.normalizePath is exercised: either create the temporary fixture
on a different drive than process.cwd() (e.g., build the tmpdir path by
prepending a different drive letter) or mock/stub the internal drive-root probe
used by AppFileSystem.normalizePath (or the helper that enumerates drive roots)
so the function must probe other drives; update the test to assert the
normalized path equals the file only when the probe path is exercised and clean
up the fixture in the finally block.

🪄 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: da36fde7-1cc8-42f9-98c8-b30bfab04638

📥 Commits

Reviewing files that changed from the base of the PR and between 7cbc96f and 36a7bfc.

📒 Files selected for processing (2)

packages/core/src/filesystem.ts
packages/core/test/filesystem/normalize-path.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

packages/core/src/filesystem.ts

toMatch(/^[A-Za-z]:/) accepted any drive letter, so the fallback could land on the wrong drive without failing the test. Construct the expected cwd-drive answer and assert exact equality.

Astro-Han added bug Something isn't working windows Windows-specific P1 High priority app Application behavior and product flows labels May 4, 2026

gemini-code-assist Bot reviewed May 4, 2026

View reviewed changes

Comment thread packages/core/src/filesystem.ts

coderabbitai Bot reviewed May 4, 2026

View reviewed changes

Comment thread packages/core/test/filesystem/normalize-path.test.ts

test(core): assert exact cwd-drive fallback for non-existent paths

af8a4b7

toMatch(/^[A-Za-z]:/) accepted any drive letter, so the fallback could land on the wrong drive without failing the test. Construct the expected cwd-drive answer and assert exact equality.

Astro-Han merged commit 4615cc7 into dev May 4, 2026
22 checks passed

Astro-Han deleted the claude/fix-windows-permission-paths-427 branch May 4, 2026 09:47

This was referenced May 4, 2026

[Bug] external_directory and read permissions don't fully canonicalize Windows path variants #427

Closed

fix(windows): canonicalize permission path variants #493

Merged

coderabbitai Bot mentioned this pull request May 12, 2026

fix(windows): cache rooted path probes and UNC globs #582

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(windows): canonicalize rooted-but-driveless paths in permission helpers#431

fix(windows): canonicalize rooted-but-driveless paths in permission helpers#431
Astro-Han merged 3 commits into
devfrom
claude/fix-windows-permission-paths-427

Astro-Han commented May 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 4, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested labels

Poem

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Astro-Han commented May 4, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Astro-Han commented May 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Related Issue

Human Review Status

Review Focus

Risk Notes

How To Verify

Screenshots or Recordings

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested labels

Poem

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Astro-Han commented May 4, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Astro-Han commented May 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 4, 2026 •

edited

Loading