From d6e07102283745a44aece2222f84c1599eabf90a Mon Sep 17 00:00:00 2001 From: Parideboy Date: Tue, 7 Jul 2026 19:43:57 +0200 Subject: [PATCH] fix(install): pass sc.exe create as raw command line so binPath= quoting survives (#1654) (#1702) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `headroom install apply --preset persistent-service` fails on Windows with `sc.exe` error 1639 ("invalid start= field"). The service install built the `sc.exe create` invocation as an argv list whose `binPath=` token embedded both spaces and inner double quotes (`cmd.exe /c "…run-headroom.cmd"`). Python's `subprocess.list2cmdline` then wrapped that whole token in outer quotes, so the command line `sc.exe` actually received tokenized as `'binPath= cmd.exe /c "…"'` and `'start= auto'` — single glued tokens — instead of the documented `binPath=` `` `start=` `` separate-token pairs. `sc.exe` rejects that with 1639. This PR builds the exact command line as a pre-quoted string and passes it to `subprocess.run` directly; on Windows a string argument goes verbatim to `CreateProcess`, bypassing `list2cmdline` entirely. The `sc.exe failure` / `start` / `stop` / `delete` calls keep the argv-list form since none of their tokens embed quotes. Fixes #1654 ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update - [ ] Refactoring (no functional changes) ## Changes Made - `headroom/install/supervisors.py`: the Windows `SERVICE` branch of `install_supervisor` now builds the `sc.exe create` command as a single pre-quoted string — `sc.exe create binPath= "cmd.exe /c \"\"" start= auto` — and passes it to `subprocess.run` as a string instead of an argv list. - `tests/test_install/test_supervisors.py`: updated the Windows-service assertion to expect the new command-line string (regression test for #1654), verifying the backslash-escaped inner quotes and `start= auto` as a separate trailing pair. ## Testing - [x] Unit tests pass (`tests/test_install/test_supervisors.py`) - [x] Lint/type gates pass (`ruff check`, `ruff format --check`, `mypy`) ``` $ python -m pytest tests/test_install/ -q 94 passed, 1 failed, 1 skipped # the 1 failure is tests/test_install/test_runtime.py::test_runtime_start_lock_blocks_another_process, # which fails identically on a clean upstream/main checkout on this machine (pre-existing local env flake, # unrelated to this change) $ ruff check headroom/install/supervisors.py tests/test_install/test_supervisors.py All checks passed! $ ruff format --check headroom/install/supervisors.py tests/test_install/test_supervisors.py 2 files already formatted $ mypy headroom --ignore-missing-imports # exit 0, notes only ``` ## Real Behavior Proof - Environment: Windows 11 Pro 10.0.26200, Python 3.13, local checkout of this branch. - Exact command / steps: Tokenized both the old (argv-list → `list2cmdline`) and new (pre-quoted string) command lines with `shell32.CommandLineToArgvW` — the same parsing `sc.exe` applies to its received command line — using the exact path from the issue report. Also ran the new string form through `subprocess.run` against the real `sc.exe` (non-elevated). - Observed result: Old form tokenizes to `['sc.exe', 'create', 'headroom-default', 'binPath= cmd.exe /c "C:\\Users\\Adron\\...\\run-headroom.cmd"', 'start= auto']` — `binPath=`/`start=` glued to their values, which `sc.exe` rejects with 1639. New form tokenizes to `['sc.exe', 'create', 'headroom-default', 'binPath=', 'cmd.exe /c "C:\\Users\\Adron\\...\\run-headroom.cmd"', 'start=', 'auto']` — exactly the documented `sc create` token shape. Running the new string against real `sc.exe` non-elevated proceeds past argument parsing to `OpenSCManager FAILED 5: Access is denied` (the expected no-admin outcome per the issue reporter's own non-admin run), with no 1639 syntax error. - Not tested: Full elevated end-to-end `headroom install apply --preset persistent-service` service creation + service start on an Administrator shell (no elevated session available in this environment); behavior on non-English locales other than the tokenization-level verification above. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review **Follow-up candidate (out of scope here)**: the issue also notes that a failed install removes `~/.headroom/deploy//` artifacts, hampering post-mortem debugging — worth a separate issue/PR to preserve or relocate failed-install artifacts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 --- headroom/install/supervisors.py | 13 +++++++++---- tests/test_install/test_supervisors.py | 14 +++++++------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/headroom/install/supervisors.py b/headroom/install/supervisors.py index ef00d0aaf..dad1c2159 100644 --- a/headroom/install/supervisors.py +++ b/headroom/install/supervisors.py @@ -261,11 +261,16 @@ def install_supervisor(manifest: DeploymentManifest) -> list[ArtifactRecord]: return records if _is_windows() and manifest.supervisor_kind == SupervisorKind.SERVICE.value: - service_bin = f'cmd.exe /c "{windows_run_cmd_path(manifest.profile)}"' - subprocess.run( - ["sc.exe", "create", manifest.service_name, f"binPath= {service_bin}", "start= auto"], - check=True, + # sc.exe's binPath= value embeds its own quotes (cmd.exe /c ""). + # Passing this as an argv list lets subprocess.list2cmdline re-quote the + # token and sc.exe mis-tokenizes it (issue #1654), so build the exact + # command line ourselves and hand subprocess a string. + run_cmd = windows_run_cmd_path(manifest.profile) + create_cmd = ( + f"sc.exe create {manifest.service_name} " + f'binPath= "cmd.exe /c \\"{run_cmd}\\"" start= auto' ) + subprocess.run(create_cmd, check=True) subprocess.run( ["sc.exe", "failure", manifest.service_name, "reset= 0", "actions= restart/5000"], check=True, diff --git a/tests/test_install/test_supervisors.py b/tests/test_install/test_supervisors.py index 161cc8629..c43030341 100644 --- a/tests/test_install/test_supervisors.py +++ b/tests/test_install/test_supervisors.py @@ -308,13 +308,13 @@ def test_install_supervisor_darwin_windows_and_unsupported(monkeypatch, tmp_path win_task = install_supervisor(_manifest(supervisor=SupervisorKind.TASK.value)) assert win_service[-1].kind == "windows-service" assert win_task[-2].path.endswith("-startup") - assert [ - "sc.exe", - "create", - "headroom-default", - 'binPath= cmd.exe /c "C:\\tmp\\default\\run-headroom.cmd"', - "start= auto", - ] in calls + # Regression for #1654: the create command must be a single pre-quoted + # string (bypassing list2cmdline) with the inner quotes backslash-escaped + # and `start= auto` as a separate trailing token. + assert ( + "sc.exe create headroom-default " + 'binPath= "cmd.exe /c \\"C:\\tmp\\default\\run-headroom.cmd\\"" start= auto' + ) in calls assert [ "schtasks", "/Create",