WIP: Enable usage in AppContainers#27266
WIP: Enable usage in AppContainers#27266SeeminglyScience wants to merge 1 commit intoPowerShell:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR aims to prevent failures when validating or discovering fixed drives and user module paths in restricted environments (e.g., AppContainers), where direct access to certain roots/special folders may be denied.
Changes:
- Replace direct fixed-drive existence checks with a helper that tolerates access-denied scenarios.
- Update default drive initialization to use the new fixed-drive validator.
- Avoid verification when retrieving the “My Documents” special folder path on Windows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/System.Management.Automation/namespaces/FileSystemProvider.cs |
Introduces IsValidFixedDrive and routes fixed-drive validation through it to avoid access-related failures. |
src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs |
Uses Environment.SpecialFolderOption.DoNotVerify when reading MyDocuments to avoid verification failures. |
| catch | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The bare catch will also swallow unexpected/fatal exceptions (e.g., OutOfMemoryException, StackOverflowException can’t be caught, but others like ThreadAbortException in legacy scenarios) and makes failures harder to diagnose. Prefer catching only the expected exception types from File.GetAttributes (e.g., IOException, ArgumentException, NotSupportedException, PathTooLongException) and let truly unexpected exceptions bubble.
| catch | |
| { | |
| return false; | |
| } | |
| catch (IOException) | |
| { | |
| return false; | |
| } | |
| catch (ArgumentException) | |
| { | |
| return false; | |
| } | |
| catch (NotSupportedException) | |
| { | |
| return false; | |
| } | |
| catch (PathTooLongException) | |
| { | |
| return false; | |
| } |
| catch (UnauthorizedAccessException) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
If the intent is to treat "access denied" as "drive is valid" for restricted sandboxes, consider also handling System.Security.SecurityException the same way as UnauthorizedAccessException. Some sandbox/policy configurations can surface as SecurityException, and returning false there could reintroduce the drive being skipped/treated invalid.
| } | |
| } | |
| catch (SecurityException) | |
| { | |
| return true; | |
| } |
|
|
||
| try | ||
| { | ||
| return (File.GetAttributes(rootDirectory) & FileAttributes.Directory) is not 0; |
There was a problem hiding this comment.
The enum flag check using pattern matching (is not 0) is less conventional for bit flags and may be inconsistent with other flag checks in this codebase. Using the standard comparison form (!= 0) tends to be clearer and more idiomatic for FileAttributes bitmasks.
| return (File.GetAttributes(rootDirectory) & FileAttributes.Directory) is not 0; | |
| return (File.GetAttributes(rootDirectory) & FileAttributes.Directory) != 0; |
PR Summary
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header