Repository navigation
agentHost: avoid subprocesses for Windows endpoint ACLs - #341178
Draft
Dmitriy Vasyura (dmitrivMS) wants to merge 1 commit into
Draft
Dmitriy Vasyura (dmitrivMS) wants to merge 1 commit into
Dmitriy Vasyura (dmitrivMS) wants to merge 1 commit into
Conversation
Use native owner-only ACL setup to avoid whoami.exe and icacls.exe at startup. Requires the companion windows-process-tree API release and dependency update before merging. Fixes #332710 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The native dependency remains unpublished/unpinned, and the regression test misses synchronous subprocess creation.
2 open findings
What changed in this PR
Replaces Windows ACL subprocesses with a native asynchronous API for Agent Host metadata.
Changes:
- Applies owner-only ACLs through
setOwnerOnlyAcl. - Adds subprocess regression coverage.
- Documents native ACL behavior and permits diagnostics-channel imports.
| File | Description |
|---|---|
localAgentHostMetadata.ts |
Uses the native ACL API. |
localAgentHostMetadata.test.ts |
Tests subprocess-free publication. |
LOCAL_ENDPOINT.md |
Documents Windows ACL handling. |
eslint.config.js |
Allows diagnostics_channel imports. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return new Promise((resolve, reject) => { | ||
| execFile(command, [...args], { encoding: 'utf8', windowsHide: true }, (error, stdout) => error ? reject(error) : resolve(String(stdout))); | ||
| }); | ||
| const { setOwnerOnlyAcl } = await import('@vscode/windows-process-tree'); |
| if (process.platform === 'win32') { | ||
| test('publishes metadata without spawning processes', async () => { | ||
| const childProcesses: ChildProcess[] = []; | ||
| const childProcessChannel = channel('child_process'); |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #332710.
Summary
Replace the local agent host's Windows
whoami.exeandicacls.execalls with the asynchronous nativesetOwnerOnlyAclAPI. This avoids startup subprocesses that can trigger security alerts while preserving protected, inheritable full-control grants for the current process user, SYSTEM and Administrators.The shared owner-only directory helper also covers the Dev Container certificate cache. Add a regression test that observes child-process creation and asserts metadata publication spawns none, plus documentation and the Node diagnostics-channel import allowance.
Dependency / merge blocker
Depends on microsoft/vscode-windows-process-tree#104. This draft was validated with that package built and linked locally. Do not merge until the native APIs are published and this PR updates the package dependency and lockfile. Neither a machine-local dependency path nor an unpublished version is committed. Without the companion package, a clean checkout does not yet have the new API.
Validation
Windows x64, locally linked native package:
npm run transpile-client— passed..\scripts\test.bat --run src\vs\platform\agentHost\test\node\localAgentHostMetadata.test.ts --run src\vs\platform\agentHost\test\node\devContainerAgentHostService.test.ts --grep 'Local Agent Host Endpoint Metadata|writes certificates to an owner-only cache'— passed, 11 Electron tests, including zero child processes during metadata publication and concurrent certificate-cache writes.npm run eslint -- src\vs\platform\agentHost\node\localAgentHostMetadata.ts src\vs\platform\agentHost\test\node\localAgentHostMetadata.test.ts— passed.git diff --check— passed.Companion package: native build, TypeScript compilation, all 39 tests and lint passed. ACL tests validate exact permissions, ownership preservation, inheritance, old-grant replacement, Unicode paths, concurrent calls and errors.
Full VS Code typecheck and full application build were not run. This draft retains the tested branch base; it has not been rebased onto the latest main.