Skip to content

Fix Windows parent directory ownership using PowerShell Set-Acl#64

Merged
travis-hoover-glean merged 3 commits into
mainfrom
thoov/fix-windows-parent-dir-ownership
Apr 8, 2026
Merged

Fix Windows parent directory ownership using PowerShell Set-Acl#64
travis-hoover-glean merged 3 commits into
mainfrom
thoov/fix-windows-parent-dir-ownership

Conversation

@travis-hoover-glean
Copy link
Copy Markdown
Contributor

@travis-hoover-glean travis-hoover-glean commented Apr 8, 2026

Summary

  • Replaces icacls /setowner with PowerShell Set-Acl for setting Windows file/directory ownership — icacls was silently failing on directories because it doesn't explicitly enable SeRestorePrivilege, while .NET's ACL APIs do
  • Batches all ownership changes into a single PowerShell call per user to avoid spawning ~25 processes
  • Adds parent directory ownership verification to the E2E config test

Test plan

  • npm test — all 147 unit tests pass
  • npm run build — builds successfully for all platforms
  • Windows CI: E2E config test now verifies both file and directory ownership

Replace icacls /setowner with PowerShell Set-Acl for setting Windows
file and directory ownership. icacls was silently failing on directories
because it doesn't explicitly enable SeRestorePrivilege in the process
token. PowerShell's Set-Acl uses .NET's SetAccessControl which properly
enables the privilege, making it reliable for both files and directories.

All paths are batched into a single PowerShell call per user to avoid
the performance penalty of spawning multiple processes.

Also adds parent directory ownership verification to the E2E config test.
${p} was being interpreted as a JavaScript template literal variable
instead of a PowerShell variable reference, causing the entire batch
ownership command to fail with "ReferenceError: p is not defined".
@travis-hoover-glean travis-hoover-glean marked this pull request as ready for review April 8, 2026 21:35
@travis-hoover-glean travis-hoover-glean requested a review from a team as a code owner April 8, 2026 21:35
@travis-hoover-glean travis-hoover-glean merged commit b4707fe into main Apr 8, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants