Repository navigation
feat(remote): control a Windows daemon over ssh - #242
Conversation
Windows PowerShell started by Win32-OpenSSH cannot read ssh stdin, so the install is three round trips: a PowerShell step creates a staging dir under the install dir, the host's own tar.exe extracts the tar.gz from stdin into it, and a second PowerShell step checks every file's SHA-256 and swaps the set in, renaming a running quil.exe aside. Exit 1 before any byte is now RemedyProbe: cmd and PowerShell answer 1 for a missing command, and so does quil refusing to start. The attach path and background dials probe the host to tell them apart. A Windows install registers the daemon's logon task and records the host's shell with the binary path. Ref #236. Part of #234.
- quild writes its PID file before Start, right after the startup lock and the healthy-peer probe. Start respawns panes before it listens, so a client asking "is a daemon starting?" during a heavy restore always heard no and spawned a second daemon. The file is now removed only while it still names this process, so a losing daemon cannot delete the winner's. - install-logon pins the quild beside quil before any quild on PATH, as the upgrade path already does; an older quild on PATH made the task start a mismatched daemon at every logon. - quil remote setup refuses a PowerShell OpenSSH DefaultShell before any remote write until the stdin pipe to tar.exe is measured there. - The lowered-spawn message no longer claims "limited". - remote-prepare.ps1 creates directories with CreateDirectory ([ and ] are wildcards to New-Item -Path) and clears stale staging dirs. - A test pins New's session-0 check; docs describe the final exit-1 behaviour, the setup shell requirement and bash as DefaultShell. Ref #236. Part of #234.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Reviewed Windows SSH remote support at 17220d6. Three blocking findings: nested-job token handling, per-user scheduled-task identity, and shell reconciliation after the documented cmd-to-PowerShell switch. All three have focused decision-level reproductions in an isolated archive; native Windows execution remains unverified.
Findings: 1 HIGH, 2 MEDIUM. Reviewed head 17220d6185 as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Request changes: one HIGH and two MEDIUM findings, each posted as a separate inline thread.
- Keep an ambiguous nested job on the task/wait/refusal path; it currently bypasses token lowering.
- Give default production logon tasks a per-user identity.
- Reconcile the shell at an unchanged binary path after the documented cmd-to-PowerShell switch.
Security review covered token lowering, job nesting, shell/path injection, archive integrity, staging boundaries, and task ownership. Correctness review covered spawning/readiness, startup locking, remote setup/reconciliation, and Windows path/display behavior. The nested-job finding also violates the Windows privilege rule; no separate rule-only finding was added. Test review found the three missing scenarios reproduced in the inline threads.
Validation at 17220d618596dd94daa2f116bbb809339e2e41ba:
- Nine affected packages passed on Linux:
internal/winjob,internal/daemonspawn,internal/remoteinstall,internal/config,internal/daemon,internal/tui,cmd/quil,cmd/quild,cmd/quil-activate. 3,748 top-level tests and 2,197 subtests passed; nine top-level tests skipped. - Targeted race checks across winjob, daemonspawn, remoteinstall, quild and quil passed: 106 top-level tests and 28 subtests. No race reports.
go vet ./...and Windows/amd64go vet -unsafeptr=false ./...passed. The Windows check used the real, SHA-256-verified bundled ConPTY assets. Context-document size check passed.- Three additional decision-level regression probes fail as described in the inline findings.
- GitHub CI
testis failing:TestClientDispatch_AttachMasterChangeReachesOthersOnlyreceives two workspace frames andTestDefaultCWD_PerClientAndBridgereceives an empty CWD. These tests concern existing client synchronization; this review has not established that this PR caused those failures. CodeQL, changelog and site checks pass.
Native Windows token/task/SSH execution and end-to-end installation were not available on this Linux reviewer. Windows PowerShell attachment and standard-user behavior therefore remain runtime validation gaps. Integration-tag tests were not run.
Agent findings: 0 resolved, 3 open. CI: 6/7 checks passing. Head 17220d6185, reviewed by @code-spire-beaver.
Open findings:
- [security/H-1] HIGH — #242 (comment)
- [code-quality/M-1] MEDIUM — #242 (comment)
- [code-quality/M-2] MEDIUM — #242 (comment)
code-spire-beaver
added
the
review: changes-requested
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #242 +/- ##
==========================================
+ Coverage 74.37% 74.63% +0.26%
==========================================
Files 269 288 +19
Lines 39718 40532 +814
==========================================
+ Hits 29539 30252 +713
- Misses 8244 8299 +55
- Partials 1935 1981 +46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The job query sees only the innermost job. A job with neither KILL_ON_JOB_CLOSE nor BREAKAWAY_OK was read as "not in a job", so an admin's High ssh token started the daemon elevated inside an unseen outer sshd kill-on-close job. JobState now returns a JobInfo (InJob, KillOnClose, BreakawayOK) that maps the flags only. StartDaemon handles the ambiguous job: an above-Medium token (or an unreadable one) spawns with the lowered token and no breakaway flag (Via lowered-in-place); a Medium token keeps today's normal spawn. Ref #236. Part of #234.
Task Scheduler names are machine-wide. The production task on the default home was the plain "Quil daemon" for every user, so a second user's non-elevated /Create /F hit the first user's task (access denied), and a name-only /Query or /Run from an elevated session could pick another user's task. Every name is now "Quil daemon (<daemon> <8 hex of the folded home>)". The default home is per-user, so the names differ per user. Ref #236. Part of #234.
Installing with cmd.exe as DefaultShell and then switching back to PowerShell left the record at shell "cmd". Every attach then sent a cmd-quoted command to PowerShell, which failed before quil started. The probe found the same path, and reconciliation compared only the path, so nothing changed and every attach failed the same way. Both reconciliation paths (the 127 heal and the exit-1 probe) now record the probe's shell beside the unchanged path when a Windows probe reports a different shell, and re-dial once. The re-dial sees an unchanged pair, so the existing handling applies. A failed probe or a POSIX probe changes nothing. Ref #236. Part of #234.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Re-reviewed 37dcb3f against 17220d6. Verified all three fixes and reviewed the complete 23-file follow-up diff across security, correctness, project rules and tests. No new blocking findings. Native Windows execution remains unverified; the lowered-in-place fallback cannot guarantee survival of an unseen outer kill-on-close job.
Findings: none. Reviewed head 37dcb3fa2d as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
✅ APPROVED
Verified all three review findings at 37dcb3fa2de53b7ec8ec3444205adb2b051685e4; the three threads are resolved. No new blocking findings in the 23-file follow-up diff.
- Security: ambiguous job membership is preserved, elevated tokens are lowered in place, and lowering failures cannot fall back to normal spawning. Task/wait paths remain first. Windows syscall wiring and the renamed JobInfo API callers were checked.
- Correctness: production task names now separate per-user default homes. Both remote reconciliation paths update a changed shell at the same binary path and stop retrying when the pair is unchanged.
- Rules: checked updated Windows privilege and remote reconciliation rules, daemon lifecycle, dev isolation, Go conventions/testing, error handling and documentation. No new rule finding.
- Tests: five packages passed with
-race(internal/winjob,internal/daemonspawn,internal/remoteinstall,cmd/quil,cmd/quil-activate): 358 top-level tests and 268 subtests passed, one test skipped. Linuxgo vet ./..., Windows/amd64go vet -unsafeptr=false ./..., and the context-document size check passed.
Residual risk: native Windows SSH/token/task execution was not available. The deliberate lowered-in-place fallback closes the privilege defect but cannot guarantee daemon survival if an unseen outer job kills its members on close. Semgrep was not installed; GitHub CodeQL passes. The main checkout and production Quil state remain unchanged.
All GitHub checks now pass, including the full race and integration suites, builds, CodeQL, coverage, changelog and site checks. Approved.
Agent findings: 3 resolved, 0 open. CI: 9/9 checks passing. Head 37dcb3fa2d, reviewed by @code-spire-beaver.
code-spire-beaver
added
review: approved
For a standard user in an ssh session, tasklist prints "ERROR: Access
denied" and exits 0. processProbe read that as "no such process", so
quil --stdio reported the daemon it had just spawned as dead at once
("daemon did not come up within 30s") and the first attach failed.
LiveDaemonPID was blind the same way.
processProbe now opens the process with PROCESS_QUERY_LIMITED_
INFORMATION: ERROR_INVALID_PARAMETER is the only answer that reads
dead; access denied reads alive with an empty name, which no identity
check treats as quild; an opened handle is alive only while its exit
code is STILL_ACTIVE; the name is the full image path. The mapping
lives in an untagged file with a Linux test; native Windows tests
cover self, a free PID and an exited child whose handle is held.
Ref #236. Part of #234.
For a standard account in an ssh session WTSEnumerateSessions fails with "No more data is available". The answer (no session, so a lowered daemon) is right, but newStartDeps logged it with log.Printf, which in quil --stdio lands on stderr and ssh relays it to the client's terminal above the TUI. The error is no longer logged. docs/remote-windows.md says what this means for a standard account: from ssh the logon task is not used, the daemon starts limited, and the task still starts it at the next desktop logon. Ref #236. Part of #234.
Setup refused a Windows host whose OpenSSH DefaultShell is PowerShell, because the extract step pipes the archive through that shell and it had not been measured. It now has been, on Windows 10: under Windows PowerShell, & 'C:\Windows\System32\tar.exe' -tvzf - reads a 2 MB archive from ssh stdin and exits 0, and & '<quil.exe>' --stdio carries a version round trip. The refusal and its test are gone; a new test runs the whole PowerShell install and checks each command is quoted for PowerShell. docs/remote-windows.md and the remote-transport rule say PowerShell is supported for setup and attach; bash stays untested. Ref #236. Part of #234.
Answering N at "Continue? [y/N]" after "Quil is not installed on <host>" was followed by the full "Cannot reach the Quil daemon ... the problem is reaching the host" block, which contradicts the line the user had just answered: the probe before it reached the host. A decline on either offer path (exit 127, and exit 1 settled by the probe) now prints one "Aborted" line and sets remoteFailureReported, so the gate exits 1 without the link-failure report. Ref #236. Part of #234.
With Windows PowerShell as the OpenSSH DefaultShell and a stale cmd record, the remote command fails with exit 1 (measured). But the shell takes a second or two to start, so the gate gave up first, Close killed ssh, the status became -1 and read as RemedyNone: the probe that heals the record or offers the install never ran, and the user was told the host could not be reached. LinkStatus gains WaitExited(d), which reaps from its own goroutine (pump reaps only after stdout EOF). The gate's dead-link branch and gateExtraVersion's DaemonUnknown arm (dialExtra and the MCP host dial) now wait deadLinkExitGrace (5 s) for a natural exit before LinkErr and Close; a child still running after that is closed exactly as before. The gate also gave up before the far side could answer. A stamped client (every binary dev.sh builds, dev and debug included, carries VERSION, so IsRelease is true) waited only the local 2 s handshake timeout, which a remote still connecting, starting a cold daemon or starting PowerShell exceeds; that is the measured exit=-1. An unstamped build (version "dev", e.g. a plain go build) skipped the request entirely, so the dead-link guard ran microseconds after ssh started. Over ssh the gate now always makes the round trip and waits remoteGateTimeout (= extraDialTimeout); a dead link still ends early because ssh exits and the read fails. Tests show both old behaviours fail, and a slow exit 1 now reaches RemedyProbe. Ref #236. Part of #234.
When the install offered at attach fails for any reason other than a decline (the dev-build refusal, a download, the push), offerRemoteInstall and resolveExitOne print "Install failed: ..." and the gate then also printed the "Cannot reach the Quil daemon ... not anything about Quil" block beneath it, contradicting it. failedRemoteInstall now sets remoteFailureReported too, so the gate exits 1 with the install error alone. The test that pinned the old flag is inverted, and a new one drives the dev-build refusal through the gate on both offer paths. docs/remote-windows.md and the remote-transport rule now name what was measured live under Windows PowerShell as the DefaultShell: setup upgrading an install, a stale cmd record healed on attach, the attach itself, a missing quil offered the install, and a decline that aborts. bash stays untested. Ref #236. Part of #234.
The PowerShell entry in docs/remote-windows.md section 4 named only three of the five facts measured live. It now lists all of them, as the remote-transport rule does: setup upgrading an install, a stale cmd record healed on attach (fails once, probes, re-records, reconnects), the attach, a missing quil offered the install, and a decline that aborts. The generic switch-shell sentence is folded into that list. Ref #236. Part of #234.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Reviewed 5ea39f959504901be52aa33610ebdd4353e96d17 against the previously approved 37dcb3fa2de53b7ec8ec3444205adb2b051685e4 (21 changed files). No new blocking findings; the previous three fixes remain intact.
- Security: checked the new Windows process query's unknown-identity handling, PID/name guards, bounded and sanitized version replies, and PowerShell command quoting. No new finding. Semgrep unavailable; GitHub CodeQL passes.
- Correctness: traced WaitExited/reap/Close ordering, bounded waits for natural SSH exit, foreground and background classification, release and unstamped handshakes, process liveness mapping, and install decline/failure reporting. No new finding.
- Rules: checked daemon lifecycle and remote transport changes against the updated rules, Windows privilege rules, dev isolation, Go conventions/testing, resilience and documentation. No new code/rule finding.
- Validation:
go test -racepassed forcmd/quil,internal/transport,internal/remoteinstall, andinternal/winjob: 441 top-level tests and 305 subtests passed, three tests skipped. Linux vet and Windows/amd64 cross-vet (-unsafeptr=false) passed for those packages. Context-document size check and all GitHub checks pass.
Native Windows execution was not repeated on this Linux reviewer. The documentation now reports live PowerShell and standard-account measurements; the PR description still says PowerShell setup is refused and its old test checklist has not been updated to match.
The existing limitation of lowered-in-place spawning under an unseen outer kill-on-close job remains. Main checkout and production state were not modified.
Findings: none. Reviewed head 5ea39f9595 as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
✅ APPROVED
Reviewed 5ea39f959504901be52aa33610ebdd4353e96d17 against the previously approved 37dcb3fa2de53b7ec8ec3444205adb2b051685e4 (21 changed files). No new blocking findings; the previous three fixes remain intact.
- Security: checked the new Windows process query's unknown-identity handling, PID/name guards, bounded and sanitized version replies, and PowerShell command quoting. No new finding. Semgrep unavailable; GitHub CodeQL passes.
- Correctness: traced WaitExited/reap/Close ordering, bounded waits for natural SSH exit, foreground and background classification, release and unstamped handshakes, process liveness mapping, and install decline/failure reporting. No new finding.
- Rules: checked daemon lifecycle and remote transport changes against the updated rules, Windows privilege rules, dev isolation, Go conventions/testing, resilience and documentation. No new code/rule finding.
- Validation:
go test -racepassed forcmd/quil,internal/transport,internal/remoteinstall, andinternal/winjob: 441 top-level tests and 305 subtests passed, three tests skipped. Linux vet and Windows/amd64 cross-vet (-unsafeptr=false) passed for those packages. Context-document size check and all GitHub checks pass.
Native Windows execution was not repeated on this Linux reviewer. The documentation now reports live PowerShell and standard-account measurements; the PR description still says PowerShell setup is refused and its old test checklist has not been updated to match.
The existing limitation of lowered-in-place spawning under an unseen outer kill-on-close job remains. Main checkout and production state were not modified.
Agent findings: 3 resolved, 0 open. CI: 9/9 checks passing. Head 5ea39f9595, reviewed by @code-spire-beaver.
2 participants
Summary
Phase 2 of the multi-client epic: a TUI on another machine can control a quil daemon that runs on Windows, over the built-in OpenSSH server. The daemon and its panes now survive ssh disconnects, and it is never started with an admin token from ssh.
Ref #236. Part of #234.
What changed
Daemon start inside an ssh session (FR-1, FR-2)
quil --stdioauto-started died with the session (measured, see feat(remote): Windows daemon as an ssh remote (phase 2) #236). An admin account over ssh also gets a full (High) token.internal/winjob: every daemon spawn from inside such a job (--stdio,daemon start/restart,mcp, a TUI overssh -t, the upgrade restart) goes through one decision:quil daemon install-logon [--remove]: registers that task (Task Scheduler XML: no 72 h time limit, normal priority, least privilege). No admin rights needed; from an elevated/ssh session it registers under the lowered token, so a normal desktop shell can remove it.quil-activate.exe start-daemon: the task's windowless launcher (reachable only as argv[1], never from aquil://click).internal/daemonspawn: one shared spawn helper, so the CLI and the launcher cannot drift.<QUIL_HOME>\quild.lockand writes its pid file before it starts restoring. Two daemons starting at the same moment for one home could previously both win (measured: two daemons, each with its own panes).[limited]in the status bar for a daemon in session 0 (no saved Credential Manager logins, invisible pane windows), with a short flash once per attach.Remote setup and attach (FR-3, FR-4, FR-7)
quil remote setupdetects a Windows host (nosh, or Git-BashMINGW) and probes it with an encoded PowerShell script.tar.exe(Windows 10 1803+), in three round trips: prepare, copy, verify each file's SHA-256 and swap (a runningquil.exeis renamed toquil.exe.old). Installs to%LOCALAPPDATA%\Programs\quil, does not touch PATH, then registers the logon task.tasklist(tasklistis denied to a standard user over ssh).Clients (FR-6)
displayBase, recent-folder dedupe by path shape).Docs
docs/remote-windows.md(OpenSSH setup, keys, default shell, Tailscale, setup, logon task,[limited]).docs/features.md,docs/installation.md,docs/roadmap/remote-daemon.md,docs/README.md..claude/CLAUDE.md,.claude/rules/remote-transport.md,windows-pty.md,daemon-lifecycle.md.changelog.d/added-windows-ssh-remote.md.Test plan
golang:1.25:go vet ./...,go test -race ./...,go test -tags=integration -race ./...internal/winjob(lowered child is Medium with admins deny-only; schtasks accepts the task XML),cmd/quildstartup lockgo vetfor every touched packagetar.exeextract from ssh stdin into a path with spaces; a cmd-quoted path with spaces,(x86)and@quil remote setupfresh install and upgrade (files byte-identical, no leftovers, per-user logon task); cold-start attach on the first try; quil removed → "not installed" + install offer; declining aborts cleanlycmdrecord healed on attach; attach; missing quil detected and offered; decline aborts