Skip to content

fix: green Windows CI — POSIX display paths after ~/, and home isolation that holds on Windows - #296

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/windows-ci-path-and-home-isolation
Sep 3, 2026
Merged

fix: green Windows CI — POSIX display paths after ~/, and home isolation that holds on Windows#296
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/windows-ci-path-and-home-isolation

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Why

Windows CI has failed on main for every recent run — 8 tests (e.g. run 33706023624 @ b85867c). Two independent root causes. One is a real display bug; the rest are POSIX-only assumptions in tests, one of which is also a hygiene hazard for anyone running the suite on a Windows machine.

1. routing list / show rendered mixed separators — product bug, 4 tests

_display_path abbreviated a home-relative path as f"~/{path.relative_to(Path.home())}". On Windows the remainder stringifies with backslashes, so the separator switched mid-path — in the terminal and in the JSON matrix_file field:

~/.amplifier\routing\openai.yaml
~/.amplifier\cache\amplifier-bundle-routing-matrix-test\routing\balanced.yaml

…while the CLI's own help text spells that location ~/.amplifier/routing/.... ~/ is a POSIX-style abbreviation, so the remainder now renders via as_posix() on every platform. A path outside the home directory isn't abbreviated and stays in native form — unchanged.

test_routing_shadowing.py:249, :405 · test_routing_winner_selection.py:323, :400

2. Two matrix_path assertions were POSIX-specific — test bug, 2 tests

matrix_path is a native absolute path — correctly; a user or tool may open it. .endswith("/.amplifier/routing/openai.yaml") can never hold against C:\...\.amplifier\routing\openai.yaml. Now compared as a Path.

test_routing_shadowing.py:304 · test_routing_winner_selection.py:346

3. monkeypatch.setenv("HOME", tmp_path) isolates nothing on Windows — test bug, 2 failing tests; 47 sites in 5 files with the same latent hazard

Path.home() is os.path.expanduser("~"). On Windows, ntpath.expanduser reads USERPROFILE (then HOMEDRIVE+HOMEPATH) and never consults HOME. So every SessionStore-touching test in these files wrote real records into the runner's — or a Windows developer's — actual ~/.amplifier/projects/<slug>/sessions/, and the two tests asserting not SessionStore().exists(SUB_SESSION_ID) found the record a previous checkpointing test had left there.

Fixed once: tests/conftest.py gains isolated_home (sets HOME and USERPROFILE, returns tmp_path). Each affected module opts in with a 3-line autouse fixture; its per-test HOME lines are removed so there is one mechanism, not two.

file sites
test_timedout_session_resumable.py (the 2 failing) 12
test_session_spawner.py 22
test_resume_preserves_provider_promotion.py 8
test_resume_system_prompt.py 4
test_skills_cli.py (custom path; USERPROFILE added beside it) 1

The three non-failing files had the identical defect — their assertions merely never depended on the isolation holding.

Verification

No Windows host available, so each fix was checked against the exact values the CI log recorded, with Windows path semantics simulated via PureWindowsPath / ntpath.expanduser:

old new
_display_path ~/.amplifier\\routing\\openai.yaml ~/.amplifier/routing/openai.yaml (both CI values; outside-home unchanged)
matrix_path assert .endswith(...)False on the CI value Path ==True (Windows and POSIX)
home isolation HOME only → expanduser('~') = C:\Users\runneradmin HOME+USERPROFILE → the tmp path

Linux: 1647 pass (count unchanged), deterministic and random order. Two touched test files weren't ruff format-clean on main; they were edited surgically (per-test line removals + the fixture) rather than reformatted, so the diff is the change and nothing else. ruff check clean on all 9 touched files; the 5 remaining repo-wide lint findings are pre-existing on main.

This PR should be merged only when the Windows jobs are actually green — that's the point of it.

…lation that holds on Windows

Windows CI has failed on main for every recent run (8 tests; e.g. run
33706023624 @ b85867c). Two independent root causes, neither a Windows-only
product defect in routing behaviour -- but one IS a real display bug.

1. `routing list`/`show` rendered mixed separators (PRODUCT, 4 tests)

   `_display_path` abbreviated a home-relative path as
   `f"~/{path.relative_to(Path.home())}"`. On Windows the remainder stringifies
   with backslashes, so users saw the separator switch mid-path:

     ~/.amplifier\routing\openai.yaml
     ~/.amplifier\cache\amplifier-bundle-routing-matrix-test\routing\balanced.yaml

   in the terminal AND in the JSON `matrix_file` field -- while the CLI's own
   help text spells the same location `~/.amplifier/routing/...`. `~/` is a
   POSIX-style abbreviation; the remainder now renders via `as_posix()` on every
   platform. A path outside the home directory is not abbreviated and is left in
   its native form, unchanged.

     tests/test_routing_shadowing.py:249, :405
     tests/test_routing_winner_selection.py:323, :400

2. Two `matrix_path` assertions were POSIX-specific (TEST, 2 tests)

   `matrix_path` is a native absolute path -- correctly; a user or tool may open
   it. `.endswith("/.amplifier/routing/openai.yaml")` can never hold against
   `C:\...\.amplifier\routing\openai.yaml`. Compare as a Path.

     tests/test_routing_shadowing.py:304
     tests/test_routing_winner_selection.py:346

3. `monkeypatch.setenv("HOME", tmp_path)` isolates nothing on Windows (TEST,
   2 failing tests -- and 47 sites in 5 files with the same latent hazard)

   `Path.home()` is `os.path.expanduser("~")`. On Windows, `ntpath.expanduser`
   reads USERPROFILE (then HOMEDRIVE+HOMEPATH) and never consults HOME. So every
   SessionStore-touching test in these files wrote real records into the
   runner's -- or a Windows developer's -- ACTUAL
   `~/.amplifier/projects/<slug>/sessions/`, and the two tests asserting
   `not SessionStore().exists(SUB_SESSION_ID)` found the record a previous
   checkpointing test had left there:

     tests/test_timedout_session_resumable.py:393, :670

   Fixed once: `tests/conftest.py` gains `isolated_home` (sets HOME AND
   USERPROFILE, returns tmp_path). Each affected module opts in with a 3-line
   autouse fixture, and its per-test HOME lines are removed so there is one
   mechanism, not two:

     test_timedout_session_resumable.py      12 sites
     test_session_spawner.py                 22 sites
     test_resume_preserves_provider_promotion.py  8 sites
     test_resume_system_prompt.py             4 sites
     test_skills_cli.py                       1 site (custom path; USERPROFILE added beside it)

   The three files that were not failing had the identical defect -- their
   assertions merely never depended on the isolation holding.

VERIFICATION

No Windows host here, so each fix was checked against the EXACT values the CI
log recorded, with Windows path semantics simulated via PureWindowsPath and
ntpath.expanduser:

  _display_path  old '~/.amplifier\\routing\\openai.yaml'  new '~/.amplifier/routing/openai.yaml'
                 (both CI values; outside-home path unchanged)
  matrix_path    old .endswith(...)=False against the CI value; new Path == : True
                 (and still True on POSIX)
  home           HOME only        -> ntpath.expanduser('~') = C:\Users\runneradmin
                 HOME+USERPROFILE -> C:\Users\runneradmin\...\tmp_path

Linux: 1647 pass (unchanged count), deterministic and random order. Two of the
touched test files were not ruff-format clean on main; they were edited
surgically (per-test line removals + the fixture) rather than reformatted, so
the diff is the change and nothing else.
@bkrabach
Brian Krabach (bkrabach) merged commit ab47608 into main Sep 3, 2026
9 checks passed
@bkrabach
Brian Krabach (bkrabach) deleted the fix/windows-ci-path-and-home-isolation branch September 3, 2026 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants