test(install/windows): verify the PATH guard against the real HKCU registry (#3068)

## Description

Follow-up requested in review of #2972, on top of the merged fix for
#2970 (#2985). Test-only; no
production code is touched and the `HEADROOM_INSTALL_PATH_SCOPE`
mechanism is unchanged.

`test_powershell_installer_does_not_leak_into_user_path` currently
guards the fix by comparing the
entry count of `[Environment]::GetEnvironmentVariable('Path','User')`
across an installer run. That
infers success from the environment variable rather than verifying it,
and it leaves three gaps:

- The .NET getter expands `%USERPROFILE%`-style references, so it cannot
observe a change of the
  registry value kind (`REG_EXPAND_SZ` vs `REG_SZ`) at all.
- A count comparison passes when an entry is replaced or reordered
rather than appended.
- There is no restore path. If the guard regresses, the test reports the
leak and then leaves the
polluted value behind in the contributor's registry, which is precisely
the damage #2970
  described: the test that detects the pollution also causes it.

This PR reads `HKCU\Environment` directly instead, so the assertion
verifies the guard rather than
assuming it.

## Type of Change

- [ ] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [x] Code refactoring (no functional changes)

## Changes Made

- `tests/test_install/test_native_installers.py`: new
`_read_user_path_entry` helper returning the
raw `HKCU\Environment` `Path` value together with its registry kind (or
`None` when the value is
absent), and `_restore_user_path_entry` writing that exact value and
kind back. Both import
`winreg` inside the function body, so the module still imports on
non-Windows hosts.
- `tests/test_install/test_native_installers.py`:
`test_powershell_installer_does_not_leak_into_user_path`
now records the raw value before the run and asserts both that the
throwaway install dir is absent
from the value afterwards (naming the #2970 symptom in the failure
message) and that value and
kind are byte-identical. The PowerShell subprocess that counted PATH
entries is gone, so the test
  also spawns one process fewer.
- `tests/test_install/test_native_installers.py`: the test now runs
under `try/finally`. The
`finally` cleans up the fake docker state, which this test was missing
relative to its sibling

`test_powershell_native_installer_supports_persistent_docker_lifecycle`,
and restores the recorded
registry value only when it actually changed, so a passing run performs
zero registry writes and a
  regressed run cannot leave the contributor's PATH polluted.

The scope allow-list tests added by #2985 (`_ENSURE_PATH_SCOPE_HARNESS`,
`test_path_scope_accepts_process_case_insensitively`,
`test_path_scope_rejects_machine_and_invalid_values`)
are untouched.

## Testing

- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [x] Manual testing performed

### Test Output

```text
$ uv run pytest tests/test_install/test_native_installers.py -q
platform win32 -- Python 3.13.11, pytest-9.0.3, pluggy-1.6.0
collected 5 items

tests\test_install\test_native_installers.py s....                       [100%]

======================== 4 passed, 1 skipped in 23.59s ========================

$ uv run ruff check .
All checks passed!

$ uv run ruff format --check tests/test_install/test_native_installers.py
1 file already formatted

$ uv run mypy headroom --ignore-missing-imports
Success: no issues found in 521 source files
```

The strengthened assertion was proven to detect a regression by
temporarily neutralising the scope
override in `scripts/install.ps1` (`if ($false -and
$env:HEADROOM_INSTALL_PATH_SCOPE)`), so
`Ensure-PathEntry` writes the `User` scope unconditionally again:

```text
$ uv run pytest tests/test_install/test_native_installers.py -q -k does_not_leak_into_user_path
tests\test_install\test_native_installers.py:638: in test_powershell_installer_does_not_leak_into_user_path
    assert str(home) not in (after[0] if after else ""), (
E   AssertionError: installer leaked the throwaway install dir into the real User PATH:
E     C:\Users\<user>\AppData\Local\Temp\pytest-of-<user>\pytest-154\test_powershell_installer_does0\home

======================= 1 failed, 4 deselected in 2.72s =======================
```

## Real Behavior Proof

- Environment: Windows 11 Pro 10.0.26200, PowerShell 7, Python 3.13.11,
pytest 9.0.3, headroom at
  `main` (`a6ab359a`), provider Anthropic
- Exact command / steps: recorded the raw `HKCU\Environment` `Path`
value with
`python -c "import winreg; ...QueryValueEx(k,'Path')"`, capturing its
registry kind, entry count
and a SHA-256 of the value; ran the full installer test file on the
patched tree; re-read the
registry; then neutralised the scope override in `scripts/install.ps1`
as shown above, re-ran the
single leak test, and re-read the registry a third time to confirm the
failure path restored it.
- Observed result: baseline `kind 1 entries 21 sha256 683ee646a95b8a28`.
After the passing run the
value was identical (`kind 1 entries 21 sha256 683ee646a95b8a28`), so a
passing run writes nothing.
With the override neutralised the test failed as quoted above and the
registry read afterwards was
again byte-identical to the recorded backup (compared as an exact
`{value, kind}` match, `True`),
confirming the `finally` restore. After reverting `scripts/install.ps1`,
the full file is back to
  4 passed, 1 skipped with the registry still unchanged.
- Not tested: non-Windows hosts (the changed test is Windows-only and
already skipped elsewhere;
`scripts/install.sh` is untouched), elevated/admin installs, and the
`Machine` scope, which
  `Ensure-PathEntry` rejects outright.

One open question this change is positioned to catch but does not
resolve: on this host the
`HKCU\Environment` `Path` value is `REG_SZ` (kind `1`), not
`REG_EXPAND_SZ`. A real install persists
through `[Environment]::SetEnvironmentVariable(..., 'User')`, which is
the API class known to rewrite
that value, so it is possible that a production install silently
downgrades an expandable PATH and
freezes `%USERPROFILE%`-style entries. I have not verified whether
headroom's installer caused it on
this machine or whether the value was always `REG_SZ`, and this PR
deliberately does not chase it.
Happy to open a separate issue if that is worth investigating.

## Runtime Rollout Safety

- Rollout-managed feature(s): none (test-only change)
- Minimum rollout channel: n/a
- Stable/default behavior changed: no; no production code path is
modified
- Kill switch / disable path: n/a
- Unsafe override required: no
- Qualification impact: none
- Rollback path: revert this commit; the test returns to the entry-count
comparison

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Parideboy 2026-08-18 05:21:03 +02:00 committed by GitHub
parent 6c9f41e08c
commit c3c921f2f7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -560,6 +560,39 @@ def _powershell_executable() -> str | None:
return shutil.which("pwsh") or shutil.which("powershell") or shutil.which("powershell.exe")
def _read_user_path_entry() -> tuple[str, int] | None:
"""Read the raw ``HKCU\\Environment`` PATH value and its registry kind, if it exists.
Reading the registry directly rather than through
``[Environment]::GetEnvironmentVariable('Path','User')`` keeps two things visible that the
.NET getter hides: the unexpanded value (the getter expands ``%USERPROFILE%``-style
references) and the value kind, so a ``REG_EXPAND_SZ`` -> ``REG_SZ`` downgrade cannot pass
unnoticed.
"""
import winreg
with winreg.OpenKey(winreg.HKEY_CURRENT_USER, "Environment") as key:
try:
value, kind = winreg.QueryValueEx(key, "Path")
except FileNotFoundError:
return None
return str(value), int(kind)
def _restore_user_path_entry(previous: tuple[str, int] | None) -> None:
import winreg
with winreg.OpenKey(winreg.HKEY_CURRENT_USER, "Environment", 0, winreg.KEY_SET_VALUE) as key:
if previous is None:
try:
winreg.DeleteValue(key, "Path")
except FileNotFoundError:
pass
return
value, kind = previous
winreg.SetValueEx(key, "Path", 0, kind, value)
@pytest.mark.skipif(
os.name != "nt" or _powershell_executable() is None,
reason="Windows PowerShell coverage runs on Windows hosts only",
@ -572,37 +605,46 @@ def test_powershell_installer_does_not_leak_into_user_path(tmp_path: Path) -> No
used to leak the temp shim dir into the developer's real PATH. ``_build_env``
now sets ``HEADROOM_INSTALL_PATH_SCOPE=Process`` to keep the update
ephemeral; the real User PATH must be unchanged across the run.
The assertion reads ``HKCU\\Environment`` itself instead of counting the entries the .NET
getter reports, so it verifies the guard rather than trusting the environment variable to
have taken effect: it catches a count-preserving mutation and a change of the value kind,
neither of which an entry count can see.
"""
powershell = _powershell_executable()
assert powershell is not None
count_cmd = [
powershell,
"-NoProfile",
"-Command",
"([Environment]::GetEnvironmentVariable('Path','User') -split ';').Count",
]
before = _run(count_cmd, env=os.environ.copy()).stdout.strip()
home = tmp_path / "home"
(home / ".local").mkdir(parents=True)
env = _build_env(home, tmp_path)
env["HEADROOM_DOCKER_IMAGE"] = "headroom:test-image"
_run(
[
powershell,
"-NoProfile",
"-ExecutionPolicy",
"Bypass",
"-File",
str(REPO_ROOT / "scripts" / "install.ps1"),
],
env=env,
cwd=REPO_ROOT,
)
after = _run(count_cmd, env=os.environ.copy()).stdout.strip()
assert after == before, f"installer leaked into the real User PATH: {before} -> {after}"
before = _read_user_path_entry()
try:
_run(
[
powershell,
"-NoProfile",
"-ExecutionPolicy",
"Bypass",
"-File",
str(REPO_ROOT / "scripts" / "install.ps1"),
],
env=env,
cwd=REPO_ROOT,
)
after = _read_user_path_entry()
assert str(home) not in (after[0] if after else ""), (
f"installer leaked the throwaway install dir into the real User PATH: {home}"
)
assert after == before, "installer mutated the real User PATH"
finally:
_cleanup_fake_docker(env)
# A passing run never writes to the registry; this only fires if the guard regresses, so
# that a failing test cannot leave the developer's PATH polluted.
if _read_user_path_entry() != before:
_restore_user_path_entry(before)
# AST-extract Ensure-PathEntry from install.ps1 and invoke it in isolation under