Support Windows remotes in the Agents window SSH transport - #327797
Draft
sandersaares wants to merge 36 commits into
Draft
Support Windows remotes in the Agents window SSH transport#327797sandersaares wants to merge 36 commits into
sandersaares wants to merge 36 commits into
Conversation
Specifies IRemotePlatform, the per-OS strategy that owns remote operations for the SSH agent host, so the transport carries no shell syntax of its own. Covers POSIX (Linux/macOS subclasses) and Windows (PowerShell -EncodedCommand), platform detection ordering, process identity via the CLI supervisor metadata, and the phased delivery and test strategy. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The remoteAgentHostCommand development escape hatch assumes POSIX and does not run detection, so it still resolves a concrete platform and no shell syntax leaks into the service. Records the POSIX-only limitation and the resulting hint, restates the platform-resolution invariant, and notes that retiring the desktop-side PID also fixes orphan accumulation on POSIX. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The agent host supervisor is shared infrastructure that outlives any single invocation, so termination now requires an independently proven dead endpoint plus a verified process identity; a relay failure alone never kills. Process identity is an optional additive field within metadata schema v1, since bumping the version would make the desktop discard valid metadata and spawn duplicate supervisors. Paths become opaque because a plain string cannot distinguish a literal path from a trusted shell expression. Detection absorbs the Windows encoder foundation, as the Windows probe is itself an encoded command. WSL stays out of the abstraction and keeps its primitives re-exported. The Windows ACL work covers the containing directory and repairs legacy token files, and adds the native Windows cargo test run it needs. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The desktop overwrites the supervisor's metadata with a foreground PID that has already exited, so reuse never fires and supervisors accumulate. That masks a second defect: relay failure terminates whatever the lockfile names, on a supervisor the CLI shares with tunnel, WSL and other desktops. Fixing either alone is unsafe, so both are corrected together. Also orders the trailing sections correctly. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
States the security model the Windows work must match, derived from the existing same-user trust boundary and POSIX 0600/0700 modes, so the ACL obligation is parity rather than a new requirement. Replaces the boolean fallback-path check with a parse returning an opaque remote path, making an unvalidated string unusable as a path by construction, and documents what quality, the server data folder name, and launch arguments mean. Drops the unused POSIX subclass split, since the existing commands are already portable across coreutils and BSD. Migrates the test harness to command-matched responses outright and restates the regression guarantee in terms of assertions rather than file contents. Records why the CLI banner PID is deferred, why end-to-end verification stays manual, and that a duplicate supervisor leaks rather than malfunctions. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The commit pin is best-effort: when the pinned download fails the installer runs any usable CLI already on the remote, and that binary is of unknown vintage rather than merely old, since the user may rotate desktop builds and the install root is shared with Remote-SSH. This is why process identity must be an optional field that degrades gracefully, so it is stated rather than left implicit. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Identity checking covers every consumer that terminates an agent host, not just the desktop, so a recycled PID cannot cause an unrelated process to be killed from any entry point. Since metadata written by a CLI of unknown vintage carries no identity, code agent kill reports that it cannot verify the process, drops the stale metadata so it stops being offered for reuse, and leaves the process running; --force restores the previous unconditional behaviour. Also records that the work lands as a single pull request, with the phases sequencing the commits rather than splitting review. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Recovery must not destroy the only record of what it is recovering, so code agent kill now fails without touching metadata when identity cannot be verified; discarding it would strand a supervisor that agent ps and tunnel reuse can no longer find. Process identity becomes versioned and OS-specific, since boot-relative jiffies repeat across reboots and macOS has no /proc. The endpoint liveness probe becomes a platform operation with an explicit indeterminate result, so only a definitive refusal permits cleanup. A failed metadata write now aborts the supervisor before readiness, so ready implies discoverable. Windows payloads propagate native exit codes, since ErrorActionPreference does not catch failing executables. ACL repair covers the metadata file, which is equally token-bearing, and the install root and binary are owner-protected. Adds diagnostics, telemetry and string obligations, and the validation checklist. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The foreground agent host invocation always exits at the readiness sentinel and there is no detach flag, so the description it was derived from is corrected; stale comments in the CLI sources still mention one. Narrows the Rust termination consumers to the two that actually kill by metadata PID, since one sends no signal and the other reaps only its own child handle. Removing the metadata record now requires the process to be observed absent rather than merely the endpoint refusing, so a supervisor with a stale port is not stranded by the very rule meant to protect it. Adds the raw launch member the override path needs, so its shell wrapper lives in a platform. Records that a duplicate supervisor reuses the persisted token, and re-derives the redaction rationale from the Windows payloads this design introduces rather than a POSIX path that cannot leak. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
code agent host already classifies its lockfile and either reuses the live supervisor or spawns a fresh one, and both paths print the same ws:// banner the desktop already parses. The desktop therefore invokes it and consumes that banner instead of maintaining a second copy of the same lifecycle logic. Removing the desktop-side metadata write, reuse probe and cleanup kill also removes the reasons for process identity tokens, endpoint probing, tree termination, metadata schema changes and structural redaction, and resolves both pre-existing defects by deletion: the desktop can no longer record a process that has already exited, nor tear down a supervisor shared with tunnel and WSL clients. Restates the invariants so they hold as written: detection necessarily precedes platform resolution, and direct-child termination was never in scope. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The supervisor is spawned without CREATE_BREAKAWAY_FROM_JOB, so under Win32-OpenSSH the exec channel's job object can reap it moments after it reports ready. The server child already probes for and uses breakaway; the supervisor now does the same, and this is the first thing the manual checklist verifies since everything else assumes it. The human banner always reports loopback regardless of the address the supervisor bound to, so consuming it alone would dial the wrong endpoint for a reused supervisor. The CLI emits a machine-readable endpoint line on both paths, restoring what the retired metadata read supplied. ACL repair moves ahead of trust: the reuse path returns before either writer runs, so a supervisor started before the fix would keep inherited permissions for its whole life. Reuse now validates both files and refuses rather than silently trusting them. Records the launch timeout mismatch, the safe-description contract, and the CLI-internal limits this design depends on but does not fix. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copying the server child's breakaway probe is not enough: when a job forbids breakaway that code simply omits the flag and stays inside the job, which is fatal for a supervisor that must outlive its channel. The fallback is undecided, and the premise that Win32-OpenSSH uses a job per exec channel is inference rather than measurement, so both are recorded as open with the experiment that settles them. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Measured against a Win32-OpenSSH exec channel rather than inferred. The channel runs in a job that kills its members on close, and of two children spawned detached with their handles on NUL, the plain one died with the channel while the one carrying CREATE_BREAKAWAY_FROM_JOB survived. Detachment alone is therefore not enough, and since the job advertises breakaway as permitted but not silent, the flag has to be passed explicitly. That settles the fallback question: none is needed, and a remote that denies breakaway should fail the connection with an actionable error instead of losing its agent host moments later. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ACL validator resolved the token path from the invoking process's data dir, but the token belongs to whichever data dir the running supervisor was started with, and the lockfile records neither. Reusing a supervisor started by another tool would therefore validate a stale file and never inspect the live one, so the launcher root is recorded and validation resolves the token from it. Three Windows behaviours were measured rather than assumed. PowerShell serialises progress records as CLIXML onto redirected stderr, costing 15x on the CLI download and pushing 342KB back over the channel, so progress is silenced in the envelope. Move-Item -Force deletes before moving and fails outright on a held destination, so the install uses a genuine atomic replace. Remove-Item on a locked binary aborts the whole prune pipeline under the envelope's error preference, leaving later candidates undeleted forever, so prune deletes per item and stays best-effort as it is on POSIX. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Recording the supervisor's launcher root is not enough to find its token file, because a connection token file may be placed anywhere; the exact path is recorded instead, and legacy metadata is resolved rather than refused outright so a supervisor the desktop cannot kill does not strand the user. Replacing the installed binary cannot work on Windows: a mapped image resists deletion and ReplaceFile both requires delete access and inherits the old file's permissions. Commit-keyed binaries are immutable, so publication renames without overwrite and validates whoever won. The desktop's readiness budget becomes the outer one rather than merely equal to the CLI's, since it starts timing earlier. Restores the fatal metadata write, which the earlier simplification dropped on the grounds that the desktop no longer read it, overlooking that the CLI's own reuse still does. Specifies the endpoint line as a versioned, redactable wire format and records what IPv6 needs from its consumers. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reconnects are rare and run on an already-established connection, so the extra round trips and the occasional CLI re-download are not worth a fallback path or a kill switch. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The existing harness replaces _startRemoteAgentHost wholesale, so the command that actually launches the agent host was never built by any test and could be changed without anything noticing. These tests drive the production launcher against the mock client instead, pinning the login-shell wrapper, the PID marker, argument escaping for a raw override, both failure paths, and that the connection token is redacted out of the error. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces IRemotePlatform with POSIX and Windows implementations and platform detection, so remote operations stop being POSIX shell strings built inline. The POSIX implementation reproduces the existing commands verbatim, so adopting it changes no behaviour; the Windows one sends every command as an encoded PowerShell payload and treats commit-keyed binaries as immutable rather than replacing them. Additive only: nothing is wired up yet. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The install, discovery and launch paths now ask an IRemotePlatform to perform each operation instead of building POSIX shell strings inline. The POSIX implementation emits the same commands as before, so the existing suites pass unmodified, which is what establishes the extraction was faithful. The detected platform is threaded into the launch rather than assumed, so the command that starts the agent host is rendered by the same platform that installed it. Detection, the lockfile paths and the shared helper module are untouched; WSL and the local lockfile still import those helpers. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Platform detection now probes once with uname -s -m and, when that fails or is unparseable, with an encoded PowerShell probe, returning a concrete platform for Windows as well as Linux and macOS. Previously the two uname calls simply failed on a Windows remote and the resolver rejected anything that was not Linux or Darwin, which is the reported failure. Detection costs one round trip instead of two, so the scripted responses in the connect-flow tests collapse accordingly; every assertion keeps its meaning. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The desktop no longer reads, writes or deletes the agent host lockfile and no longer kills remote processes. code agent host already classifies that lockfile and either reuses the live supervisor or spawns a fresh one, printing the same endpoint banner either way, so connect now simply resolves the CLI, launches, and relays. This removes a second copy of that logic which could not run on a Windows remote at all, recorded a process that had already exited, and on a relay failure could terminate a supervisor shared with tunnel and WSL clients. A relay failure now propagates instead. The helper module keeps exporting those functions for WSL and the local lockfile. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The desktop allowed 60 seconds for the agent host to report its endpoint while the CLI allows itself five minutes to become ready, and the desktop's timer starts earlier still. A first connect over a slow link could therefore be abandoned while the CLI was legitimately still working. The budget is now larger than the CLI's rather than smaller. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Win32-OpenSSH runs each exec channel in a job object carrying KILL_ON_JOB_CLOSE, so the detached supervisor was terminated when the channel closed moments after it reported ready. Detaching is not enough: measured against Windows 11, a child spawned without CREATE_BREAKAWAY_FROM_JOB dies with the channel while one carrying it survives, and the job advertises breakaway as permitted but not silent. The flag is requested only when a probe confirms the job allows it, since requesting it inside a job that forbids it fails the spawn outright. Publishing the agent host lockfile also becomes fatal. That file is the only record of the supervisor, so serving without it produced an invisible process that nothing could find or reuse and every later invocation duplicated. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The lockfile and the token file both carry the agent host connection token, and both were created with owner-only permissions on Unix but left to inherit whatever the parent directory granted on Windows. The parent is created by whichever tool gets there first, so a permissive profile or directory ACL silently exposed the token to any local account. Access is granted to the OWNER RIGHTS SID rather than by account name, since these machines are commonly joined such that the name takes the form AzureAD\\user@example.com. The result is verified against the well-known broad SIDs rather than assumed, because icacls does not remove inherited entries identically on every host. SYSTEM and Administrators may remain, matching Unix, where root can read a 0600 file. Permissions are re-applied whenever the token file is opened, not only when it is written: the common path returns an existing token without rewriting it, so a file created before this change would otherwise keep its inherited ACL for as long as it survived. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A remote with neither uname nor a usable powershell.exe is almost always a Windows host with PowerShell off the PATH, and the override setting is POSIX-only, so pointing it at a Windows remote fails on the login-shell wrapper rather than on anything the user configured. Both now say that instead of surfacing a raw shell error, and the detection failure is localized along with them. The setting description states the limitation. Refs microsoft#327469 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A locally built CLI is unsigned and has no cloud reputation, so Defender ASR refuses to execute it with a bare 'Access is denied'. Note the diagnosis and the one-time exclusion in the validation checklist, and state why Windows detection is marker-based: a remote login shell can prepend banner output that the client cannot suppress. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Launching through buildLaunchCommand against a real Windows 11 remote, a build carrying CREATE_BREAKAWAY_FROM_JOB leaves a supervisor that is still listening after the exec channel closes; the same build without the flag has already lost it while the channel is open. Both print the endpoint banner, so readiness alone does not distinguish them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The human banner always prints ws://localhost:<port>, so a supervisor bound to a specific address or to ::1 left the desktop dialling loopback forever. The CLI now emits a versioned machine-readable line carrying the dial host, port and token on both the fresh-spawn and reuse paths, and the desktop prefers it, falling back to the banner for a CLI too old to emit it. Dialling an IPv6 host means composing one: dial_host maps :: to ::1 rather than IPv4 loopback, SSHConnection keeps the host so relay-only reconnect stops assuming 127.0.0.1, and authorities are bracketed through a shared formatHostPortAuthority. The bridge channel needs that bracketing too, since it receives the same dial host through --agent-host-bridge-host. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reuse returned before either writer ran, so a supervisor started before the lockfile and token file were protected kept its inherited permissions for as long as it lived. Reuse now checks both files first and refuses when either is readable beyond its owner, rather than tightening a file whose contents may already have been read. Finding the right token file needs the path: --connection-token-file can place it anywhere, so the lockfile records it. A lockfile predating that field is matched against the known default roots, requiring the candidate to hold exactly this supervisor's token; a tokenless supervisor has no file to check. Only an unresolved case refuses, because a blanket refusal would strand users behind a supervisor the desktop is forbidden to kill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The shell expands ~ before ls runs, so fallback discovery reported absolute paths while validation only accepted the tilde form — no candidate was ever recognised on a POSIX remote. Match on the directory and file name instead, so both forms resolve. Proven against a real Linux remote, where discovery now finds the installed CLI. The Windows-only Rust code had no job that could run its tests: cargo test runs on Linux only, and the breakaway module was compiled out with cfg(not(windows)). Add a Windows cargo test step and Windows tests for the detach flags, and execute the generated PowerShell payloads against real powershell.exe, directly and through cmd.exe, so a malformed or mis-encoded payload cannot pass as a string assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Four defects a review found in the preceding commits. The banner was printed first and always says ws://localhost, which the desktop also matches, so a chunk carrying the banner but not yet the endpoint line resolved to loopback — nondeterministically dialling the wrong address for a supervisor bound elsewhere. Emit the endpoint line ahead of the banner so any buffer containing the banner already contains it. The endpoint parser anchored on \$ under /m, so a buffer ending mid-line parsed as complete and could latch a truncated port or token. Require the newline. parseFallbackCliPath left the middle of the directory unconstrained while the POSIX platform interpolates the result unquoted into test -x, --version and exec. Re-assert a shell-safe character set. The legacy token search covered only the default launcher root, but the token is minted under --cli-data-dir while the lockfile is pinned to the canonical root — so every supervisor already running in the field would have been refused after an upgrade. Search the caller's root as well. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The launch command quoted every argument as a literal, so the --cli-data-dir value reached the CLI as the text \C:\Users\sasaares\.vscode-server-oss\cli and it failed trying to create a directory by that name. A remote path is a shell expression, not a literal — POSIX carries an unquoted ~ and Windows a \C:\Users\sasaares fragment — so the launch spec now distinguishes the two and each platform emits paths verbatim while still quoting literals. The existing assertion had pinned the broken form, which is why nothing caught it; it now asserts the expandable one, and a payload-execution test confirms the launched program receives a real directory rather than the expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Windows remote support to the Agents window’s SSH transport through platform-specific command strategies and CLI lifecycle/security changes.
Changes:
- Introduces POSIX and Windows remote-platform implementations with detection and tests.
- Adds endpoint reporting, IPv6 handling, and CLI-owned supervisor lifecycle.
- Adds Windows ACL, process breakaway, and CI test coverage.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHost.contribution.ts |
Documents override platform limitations. |
src/vs/server/node/agentHostChannel.ts |
Formats IPv6 WebSocket authorities. |
src/vs/platform/agentHost/test/node/sshRemoteAgentHostHelpers.test.ts |
Tests endpoint parsing and redaction. |
src/vs/platform/agentHost/test/node/sshRemoteAgentHostService.test.ts |
Exercises Windows SSH connection flows. |
src/vs/platform/agentHost/test/node/remotePlatform/windowsRemotePlatformExec.test.ts |
Executes generated PowerShell payloads. |
src/vs/platform/agentHost/test/node/remotePlatform/windowsRemotePlatform.test.ts |
Tests Windows strategy commands. |
src/vs/platform/agentHost/test/node/remotePlatform/remotePlatformDetection.test.ts |
Tests platform detection ordering. |
src/vs/platform/agentHost/test/node/remotePlatform/posixRemotePlatform.test.ts |
Guards POSIX behavior. |
src/vs/platform/agentHost/REMOTE_PLATFORM.md |
Documents the cross-platform design. |
src/vs/platform/agentHost/node/sshRemoteAgentHostService.ts |
Integrates platform strategies and endpoint relay. |
src/vs/platform/agentHost/node/sshRemoteAgentHostHelpers.ts |
Parses and redacts endpoint lines. |
src/vs/platform/agentHost/node/remotePlatform/windowsRemotePlatform.ts |
Implements Windows PowerShell operations. |
src/vs/platform/agentHost/node/remotePlatform/remotePlatformDetection.ts |
Detects POSIX and Windows remotes. |
src/vs/platform/agentHost/node/remotePlatform/remotePlatform.ts |
Defines the platform abstraction. |
src/vs/platform/agentHost/node/remotePlatform/posixRemotePlatform.ts |
Extracts existing POSIX behavior. |
src/vs/platform/agentHost/common/remoteAgentHostMetadata.ts |
Adds token-file metadata. |
src/vs/base/test/common/network.test.ts |
Tests authority formatting. |
src/vs/base/common/network.ts |
Adds host/port URL formatting. |
cli/src/util/file_permissions.rs |
Adds Windows ACL handling. |
cli/src/util/errors.rs |
Adds lifecycle and credential errors. |
cli/src/util/command.rs |
Adds Windows job breakaway support. |
cli/src/util.rs |
Exports permission utilities. |
cli/src/tunnels/agent_host.rs |
Validates credentials and records token paths. |
cli/src/tunnels/agent_host_metadata.rs |
Secures and extends lockfile metadata. |
cli/src/commands/output.rs |
Emits machine-readable endpoints. |
cli/src/commands/agent_host.rs |
Updates supervisor lifecycle and endpoint reporting. |
build/azure-pipelines/win32/product-build-win32-cli.yml |
Runs Windows CLI tests in CI. |
Windows ACL enforcement on the CLI side now goes through the Win32 security APIs instead of parsing `icacls` output. The previous check scanned for five well-known SIDs and matched the English string "SID Found:", so it failed open on a non-zero `icacls` exit and missed any grant to a principal outside that list. The verifier now enumerates the whole DACL and requires every allow ACE to name the owner, SYSTEM or Administrators, treating a NULL DACL as unprotected. The supervisor no longer adopts a connection token that may already have leaked: it samples whether the file was owner-only before tightening it and mints a fresh token when it was not. A caller-supplied `--connection-token-file` is verified and refused when broadly readable, since tightening cannot un-leak a token that was already exposed. Endpoint output reports the address the listener actually bound rather than re-deriving it from the `--host` label, and the refusal remediation no longer suggests `code agent kill`, which reads its PID from the very lockfile that was just rejected. On the desktop side every remote command carries a localized operation description, so a failure names what was being attempted without echoing the encoded payload. The CLI install boundary is restricted before the binary is published, and a loose install that fails its version check is repaired and re-validated. `bind_tcp` and `print_reuse_banner` take their options as structs, which keeps clippy's argument-count lint satisfied under `-D warnings`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`restrict_to_owner` was a no-op on POSIX, documented as relying on callers to create these files with mode 0600. That made two things untrue. The connection token repair path calls it on every invocation precisely so that a token file predating the check is tightened, but on POSIX that repair never happened. And `write_agent_host_metadata` had to carry parallel cfg-gated `set_permissions` calls to get the behaviour the function already promised. It now sets mode 0600 on a file and 0700 on a directory, preserving an execute bit a file already had so an installed binary stays runnable. The metadata writer drops its duplicated cfg blocks and calls it directly. This also fixes three `cargo test` failures that only appear on POSIX, where a fixture writing a token file through `fs::write` gets the ambient umask and the supervisor then correctly refuses to reuse it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`os.tmpdir()` on the Windows CI agent is an 8.3 short path (`C:\Users\RUNNER~1\...`), and PowerShell reports the canonical long form back, so a path built from the short form never compares equal to what the payload returns. Use the runner-provided temp directory, which is a plain long path, matching what sessionPermissions.test.ts already does for the same reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
@roblourens With development here in flux, I suspect this is not something we're ready to take as a contribution yet? |
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.
[Copilot speaking]
Fixes #327469
Contributes to #310166
The problem
Connecting the Agents window to a Windows remote over SSH fails immediately:
uname -sis only the first thing that breaks. The SSH transport is POSIX throughout, using~paths,test -x,ls -1t | xargs,tar -xzandbash -l -c, with no seam where another OS can be expressed. Special-casing the probe would move the failure a few round trips later.The approach
Introduce an
IRemotePlatformstrategy that owns every command the transport sends, with POSIX and Windows implementations behind it. The POSIX implementation reproduces the existing commands verbatim, so the pre-existing path is unchanged by construction.The design is written up in
REMOTE_PLATFORM.md, which records the reasoning, the measurements behind each decision, and the validation checklist.What is in here
Platform abstraction.
IRemotePlatformwithPosixRemotePlatform(Linux and macOS share one implementation, because the commands are already portable) andWindowsRemotePlatform. Paths are a brandedRemotePathso they cannot be built by concatenation, and remote output only re-enters a command through a validating parser.Detection. A single
uname -s -mfirst, then a PowerShell probe only if that fails. The Windows probe is marker-based, because a remote login shell can prepend arbitrary banner output that the client cannot suppress.Windows transport. Commands travel as
-EncodedCommand(UTF-16LE base64), which survives whatever default shellsshdis configured with. The envelope sets$ProgressPreference = 'SilentlyContinue': leaving it default costs 13,125 ms and 342 KB of CLIXML on stderr for a CLI install, against 844 ms and 0 bytes with it set.The supervisor survives its launching channel. Win32-OpenSSH runs each exec channel in a job object with kill-on-close, so a detached supervisor is reaped moments after it reports itself ready.
CREATE_BREAKAWAY_FROM_JOBis included when the job permits it. Measured as an A/B on a real remote:Both builds print the endpoint banner, so readiness alone cannot distinguish them.
The CLI owns the lifecycle.
code agent host's foreground already classifies its lockfile and reuses or spawns, and both paths report the same endpoint, so the desktop invokes it and consumes its output. The desktop therefore never writes agent host metadata and never terminates a remote process. Both matter: the supervisor's own record is authoritative, and the supervisor is shared withcode tunneland WSL, so killing it on a relay failure can tear down an agent host another consumer is using. A relay failure surfaces a retryable error instead.A machine-readable endpoint line. The human banner always prints
ws://localhost:<port>regardless of the bind address, so it cannot describe a supervisor bound elsewhere. The CLI emits a versioned line carrying the real dial host, and the desktop prefers it, falling back to the banner for a CLI too old to emit one. Reaching an IPv6 endpoint also requires bracketing authorities and keeping the host through relay-only reconnect.Secrets are owner-only on Windows. Every permission call in the writers is
#[cfg(not(windows))], so the lockfile and token file inherit whatever the parent directory grants. They are now restricted via theOWNER RIGHTSSID, chosen over an account name because these machines are commonly joined such that the name isAzureAD\user@example.com. Reuse validates both files first and refuses a supervisor whose secrets are readable by other accounts, rather than tightening a file whose contents may already have leaked.Testing
Unit tests cover the emitted commands per platform, the envelope's round trip, detection ordering, and the lifecycle contract. Because string assertions cannot prove a payload is valid PowerShell, the generated payloads are also executed against real
powershell.exe, both directly and throughcmd.exe.cargo testruns only on Linux today, so the Windows-only Rust code has no job that can test it. This adds a Windows CLI test step and Windows tests for the job-object breakaway logic.The section 13 checklist was worked through against a real Windows 11 remote and a real Linux remote: supervisor survival, first connect, reuse, endpoint reporting, both default shells, a profile path containing a space and an apostrophe, relay failure, ACLs, retention with a locked binary, concurrent install, diagnostics, and POSIX regression. Manual end-to-end testing against a Windows 11 remote confirms a working session.
Notes for reviewers
PosixRemotePlatformis intended to be a faithful move of the existing commands. Any behavioural difference there is a bug, not an intent.chat.sshRemoteAgentHostCommandstays POSIX-only, deliberately. It is a development-only escape hatch that points at a locally built agent host and bypasses detection, install and the platform-owned launch entirely, so supporting it on Windows would mean building a second launch path for a setting no end user has. The path still resolves a concretePosixRemotePlatformrather than emitting shell syntax from the service, so nothing about it blocks a later Windows implementation. The setting description states the limitation and the failure is surfaced as a targeted hint rather than a rawbasherror.cargo teststep to the existing Windows CLI job. I could not execute it locally, so it is unvalidated by a real run. It is also the reason the "Prevent engineering system changes in PRs" check is red: that workflow blocks any PR from a contributor without write access that touchesbuild/, and no exception applies here. The step is kept because it is the only CI coverage for the#[cfg(windows)]Rust code this PR adds, which the Linux CLI job cannot run. Drop the file if you would rather apply it separately.