Skip to content

fix(windows): tolerate unreachable UNC permission paths#586

Merged
Astro-Han merged 1 commit into
devfrom
slock/fix-unc-permission-metadata-test
May 12, 2026
Merged

fix(windows): tolerate unreachable UNC permission paths#586
Astro-Han merged 1 commit into
devfrom
slock/fix-unc-permission-metadata-test

Conversation

@Astro-Han
Copy link
Copy Markdown
Owner

@Astro-Han Astro-Han commented May 12, 2026

Summary

Tolerate unreachable Windows UNC components while resolving external-directory permission metadata.

Why

PR #582 tightened Windows UNC normalization so UNC inputs now reach the permission resolver more consistently. That exposed a pre-existing gap in resolveWindowsForPermission(): Bun on Windows can report an unreachable UNC intermediate component as EUNKNOWN from lstat, while the resolver only treated ENOENT and ENOTDIR as missing-path fallback cases.

For permission metadata, an unreachable UNC component should behave like a not-yet-existing path under the UNC share. The resolver should still produce canonical permission metadata instead of throwing before the permission request can be built. This is a follow-up for the gap exposed by PR #582, not a broad revert of the Windows path canonicalization work.

Related Issue

Refs PR #582. This does not close issue #497; it is a follow-up fix for the Windows advisory regression exposed after #582 merged.

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

  • resolveWindowsForPermission() now treats only UNC lstat EUNKNOWN errors as missing-path fallbacks.
  • The existing ENOENT / ENOTDIR behavior is unchanged.
  • The new custom-fs regression test covers the Windows-only EUNKNOWN path without requiring a live unreachable UNC host on macOS.
  • Confirm the fallback does not broaden non-UNC path error handling.

Risk Notes

Windows-only path/permission behavior. The change is intentionally narrow: only lstat EUNKNOWN on UNC candidates is folded into the existing missing-path fallback. macOS/Linux behavior is not touched. The Windows advisory workflow must be green before merge.

How To Verify

Focused external-directory test: bun --cwd packages/opencode test test/tool/external-directory.test.ts -> 14 pass
Targeted tool suite: bun --cwd packages/opencode test test/tool/external-directory.test.ts test/tool/read.test.ts test/tool/write.test.ts test/tool/edit.test.ts test/tool/apply_patch.test.ts test/tool/glob.test.ts test/tool/grep.test.ts test/tool/lsp.test.ts -> 136 pass
Opencode typecheck: bun --cwd packages/opencode typecheck -> pass
Diff check: git diff --check -> clean
Windows advisory workflow: required merge blocker; must pass on this PR before merge

Screenshots or Recordings

None. No visible UI changes.

Checklist

  • Human review status is stated above as pending, approved, or not required
  • I linked the related issue, or stated why there is no issue
  • This PR has type, primary area, and priority labels, or I requested maintainer labeling
  • I described the review focus and any meaningful risks
  • I listed the relevant verification steps and the key result for each
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant
  • I reviewed the final diff for unrelated changes and suspicious dependency changes
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced Windows network path resolution to properly handle unreachable path components.
  • Tests

    • Added test coverage for Windows extended network path resolution scenarios.

Review Change Stack

@Astro-Han Astro-Han added platform Electron shell, OS integration, packaging, updater, signing, paths, and permissions tech-debt Internal cleanup and maintainability debt windows Windows-specific P1 High priority labels May 12, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

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: cf4d634a-72d2-4788-9d26-b44b59fa65fb

📥 Commits

Reviewing files that changed from the base of the PR and between 7ed1ca1 and 5b5c182.

📒 Files selected for processing (2)
  • packages/opencode/src/tool/external-directory.ts
  • packages/opencode/test/tool/external-directory.test.ts

📝 Walkthrough

Walkthrough

This PR improves Windows external path resolution by classifying Bun-on-Windows lstat errors (including EUNKNOWN on unreachable UNC components) as missing paths, replacing direct error-code checks with a new isMissingPermissionPath helper. A corresponding test validates the behavior on extended UNC paths.

Changes

Windows UNC lstat error handling

Layer / File(s) Summary
UNC missing-path classification and usage
packages/opencode/src/tool/external-directory.ts
Introduced isMissingPermissionPath(error, candidate) helper to classify lstat failures as missing when error.code is ENOENT/ENOTDIR, or when Bun returns EUNKNOWN with syscall: "lstat" on a UNC path. Refactored resolveWindowsForPermission to use this helper instead of direct error-code checks.
UNC path resolution test coverage
packages/opencode/test/tool/external-directory.test.ts
Added Windows-only test for resolveExternalPathForPermission simulating an lstat failure (EUNKNOWN) when a UNC "outside" component is unreachable, asserting the function returns the canonical UNC path with the unresolved component preserved.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

  • Astro-Han/pawwork#427: Both address Windows UNC/extended-path handling in external-directory permission resolution (lstat error handling for UNC components complements earlier canonicalization work).

Possibly related PRs

  • Astro-Han/pawwork#582: Both modify Windows UNC handling in packages/opencode/src/tool/external-directory.ts including permission-path resolution behavior and tests.
  • Astro-Han/pawwork#493: Both modify Windows permission-path resolution in packages/opencode/src/tool/external-directory.ts adjusting how permission path resolution canonicalizes and treats UNC/lstat errors.

Suggested labels

bug

Poem

🐰 Hops through UNC paths with newfound grace,
Where Bun's EUNKNOWN finds its rightful place,
A helper born to catch what's missing true,
Missing paths now handled through and through! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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 concisely and accurately describes the main change: handling unreachable UNC permission paths on Windows, which is the primary focus of the changeset.
Description check ✅ Passed The description covers all required sections from the template: Summary, Why, Related Issue, Human Review Status, Review Focus, Risk Notes, How To Verify, Screenshots, and a complete 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 slock/fix-unc-permission-metadata-test

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.

❤️ Share

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

@github-actions github-actions Bot added harness Model harness, prompts, tool descriptions, and session mechanics and removed platform Electron shell, OS integration, packaging, updater, signing, paths, and permissions labels May 12, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a fix for handling unreachable UNC components on Windows, specifically addressing cases where Bun reports an EUNKNOWN error during path resolution. It adds a helper function isMissingPermissionPath to identify these scenarios and includes a new test case to verify the behavior. Feedback suggests expanding the error check to include the realpath syscall and ensuring path normalization for canonicalization.

Comment thread packages/opencode/src/tool/external-directory.ts
@Astro-Han Astro-Han merged commit a902d5b into dev May 12, 2026
35 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

harness Model harness, prompts, tool descriptions, and session mechanics P1 High priority tech-debt Internal cleanup and maintainability debt windows Windows-specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant