ci: add native-Windows test lane + POSIX/Win32 test gating - #31
Conversation
Add a windows-latest CI job (3.11-3.14 matrix) and gate the POSIX-only test constructs that failed — or killed the pytest runner — on native Windows, so the deterministic suite runs green on Windows CI. - ci.yml: test-windows job (matrix) + persist-credentials: false on checkouts - install.py: normalize `\`->`/` in _worktree_local_exclude so git-exclude patterns anchor on Windows (a no-op on POSIX) - tests: cross-platform verify commands (test -f / if exist) and dead-pid spawns; real_signal uses signal.raise_signal (catchable in-process on both OSes — keeps Windows coverage instead of TerminateProcess-killing the runner); HAVE_TMUX excludes win32; USERPROFILE for expanduser; cmd-safe hook_bus command; encoding="utf-8" reads; a .cmd-shim fake-CLI launcher; skipif(win32) on the POSIX-only Unity symlink-priming tests Verified green on native Windows (24 baseline failures -> 0 in-scope) and WSL/Linux (no regression; the gating is a POSIX no-op). ruff clean.
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a Windows CI job, hardens checkout credential handling, normalizes git exclude patterns for cross-platform paths, and updates tests and helpers to use platform-aware commands, launchers, signals, and environment variables. ChangesCross-platform CI and test support
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: This PR adds a native Windows CI lane and updates the test suite to be POSIX/Win32 aware so the deterministic suite stays green on Windows. Changes:
Technical Notes: Verify and plugin hook commands are executed with 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_engine_worktree.py (1)
509-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated platform-detection logic across test files.
_touch_run/_exists_run/_seeded_then_touchre-implement the same Windowsif exist ... (exit 0) else (exit 1)/ POSIXtest -fpattern already introduced as_file_exists_cmdintests/test_engine.py. Consider hoisting a shared "exists"/"touch" command builder intoconftest.pyalongsidewrite_script_launcherso future OS-specific quirks only need to be fixed in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_engine_worktree.py` around lines 509 - 538, The helper logic in _touch_run, _exists_run, and _seeded_then_touch duplicates the same Windows/POSIX file-check patterns already present in _file_exists_cmd, so centralize this OS-specific command building instead of re-implementing it here. Move or reuse a shared exists/touch command helper in conftest.py near write_script_launcher, then update the test_engine_worktree helpers to call that shared utility for all marker/file checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_engine_worktree.py`:
- Around line 509-538: The helper logic in _touch_run, _exists_run, and
_seeded_then_touch duplicates the same Windows/POSIX file-check patterns already
present in _file_exists_cmd, so centralize this OS-specific command building
instead of re-implementing it here. Move or reuse a shared exists/touch command
helper in conftest.py near write_script_launcher, then update the
test_engine_worktree helpers to call that shared utility for all marker/file
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65875908-59ab-4be2-a7c6-097b36d7b742
📒 Files selected for processing (13)
.github/workflows/ci.ymlsrc/automator/install.pytests/conftest.pytests/test_decisions.pytests/test_engine.pytests/test_engine_plugin.pytests/test_engine_worktree.pytests/test_generic_tmux.pytests/test_hook_bus.pytests/test_install.pytests/test_runs.pytests/test_sanitize.pytests/test_tui_data.py
The harness reads/writes UTF-8 files (specs, journals, JSON, reports) but Windows defaults to cp1252, so a plain read_text()/open() throws UnicodeDecodeError on the first non-ASCII byte — ~400 latent sites that only surface on the native-Windows runner, one at a time. Set PYTHONUTF8=1 on the windows CI job (PEP 686's Python 3.15 default) so the runner matches the UTF-8 files under test, and add a conftest guard that fails fast with an actionable message when a local win32 run isn't in UTF-8 mode — so local matches CI instead of dying with cryptic charmap errors deep in an unrelated test. src stays encoding-explicit (0 unencoded I/O), so this does not mask real product encoding bugs.
PosixProcessHost.is_alive uses os.kill(pid, 0); on Windows signal 0 is CTRL_C_EVENT, so the probe sent Ctrl+C to the runner and aborted the suite with KeyboardInterrupt. Use get_process_host() so each platform exercises its real host (psutil-based on win32) while keeping the invariant covered.
|
will need the conflicts cleaned up after the two prior PRs were merged in. |
# Conflicts: # tests/test_process_host.py
|
Please squash, this is ugly... |
push_screen sets app.screen before the modal's children finish mounting, so a screen-type-only wait let the click land on an empty modal on slow CI Windows runners (NoMatches on #ok). Wait for the button itself.
|
This was a test-synchronization race - Textual's push_screen assigns app.screen before the modal's children finish mounting, so on a slow CI Windows runner the pilot.click("#ok") fired before #ok existed — fixed by having the until wait for the button to actually be mounted, not just for the screen type to change. Do we wan't to leave all these python version checks? |
We really should keep them it is the version range we support. If flaky tests on additional PR's still keep being a thing with the ci runner, for windows only we can then consider removing the previous versions and add some disclaimer that while supported on the linux/macos/wsl side the windows side is not officially tested for the older versions. |
|
Then I will push more safeguards for until(screen is Modal) → immediate click here.
|
…een type push_screen swaps app.screen the instant a modal is pushed — before its children mount (query NoMatches) or get a layout region (click OutOfBounds, region still 0). A screen-type-only until let query_one/click race that on slow CI Windows runners. Add a ready() helper that waits for a real on-screen region and route all 10 modal interactions in this file through it; a modal's widgets mount and lay out together, so one gate covers every field.
c64673a to
3b6fe46
Compare
Setting a reactive Switch.value posts a Switch.Changed message that on_switch_changed handles on a later tick, mutating a *different* widget (Input.disabled / Collapsible.display). A single pause() can return before that message is delivered on a slow CI Windows runner, so the derived-state assert races. Gate both spots on the derived condition via until(), matching the modal mount/layout fixes.
3b6fe46 to
7559c31
Compare
pbean
left a comment
There was a problem hiding this comment.
Validation ✅ — no Linux/macOS regression, approach is sound. Two small asks before I approve.
Went through this focused on whether it risks the Linux/macOS core. It doesn't — confirmed three ways:
- All 4 existing Linux jobs (py3.11–3.14) stay green alongside the new Windows lane.
- The
os.kill(getpid())→signal.raise_signalswap is equivalent on POSIX (single-threadedengine.run(), same main-thread handler) and stopsTerminateProcessfrom killing the Windows runner. - Every gated test's non-win32 branch is byte-identical or differs only by no-op quoting /
exit 0↔true(equivalent under/bin/sh).
And the _worktree_local_exclude fix is a real one — str(relative_to(repo_root)) renders os.sep, so seed_globs matches became .claude\skills\<name> on Windows and git never anchored them. Good catch. The ready()/until() TUI gates also cut flakiness on slow Linux runners — nice.
Two things I'd like tightened before I approve/merge:
-
Scope the exclude normalization to its actual source. The blanket
patterns = [p.replace("\\", "/") for p in patterns]at the top of_worktree_local_excludeis broader than needed — only the glob-derived path picks upos.sep. Please normalize at the source instead —seeded.append(rel.as_posix())atinstall.py:314— and drop the loop.skill_tree/config_path/seed_filesare already POSIX-slash config strings, so this keeps the fix precise and never rewrites a pattern that could (pathologically) carry a legitimate backslash on POSIX. -
De-dupe the platform helpers.
_file_exists_cmd,_touch_run/_exists_run/_seeded_then_touch,_OK/_RUN, etc. repeat the samesys.platform == "win32"branching across test files (CodeRabbit flagged this too). Please hoist the shared ones intoconftest.py(next towrite_script_launcher) so there's a single platform-detection spot.
Everything else looks great — the per-gate comments and the UTF-8 guard especially. Once those two land I'll approve; please squash on merge.
|
Will address this tomorrow |
Three more spots shared the push_screen swap-before-mount race the modal
fixes already addressed: the deferred-entry and validate-output modals read
query("Static")/query("Label") right after a screen-type until, so the body
could still be unmounted (empty query -> failed assert) on a slow CI runner.
Route them through ready(), matching the other modal interactions.
SettingsScreen is pushed the same way, so open_settings() returned before the
form fields mounted and every caller's first query_one could NoMatches. The
fields mount as one batch, so waiting for any Input in the shared helper gates
all ~20 settings tests at a single point.
…helpers _worktree_local_exclude no longer blanket-rewrites every pattern's separators; only the glob-derived rel can carry os.sep, so it's normalized at the source via rel.as_posix(). skill_tree/config_path/seed_files are already POSIX config strings, so the fix stays precise and never touches a pattern that could pathologically hold a legitimate backslash on POSIX. Hoist the host-shell verify/lifecycle stub helpers (_OK, _RUN, _file_exists_cmd, _touch_run, _exists_run, _seeded_then_touch) into conftest so the sys.platform branching lives in one spot instead of being re-derived per test file; the two engine test modules import them and drop their now-unused sys import.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_tui_app.py (1)
564-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent click-ready pattern; some sites still split
ready()frompilot.click().Sites like Line 767 (
await ready(pilot, "#ok")thenawait pilot.click("#ok")), Line 841-843, Line 865-867, and Line 681-682 use the split form, while sibling sites in the same file (Line 796, 811, 819, 871, 904, 1113) use the combinedawait pilot.click(await ready(pilot, "#ok"))form recommended in review discussion for this PR. The split form is not currently unsafe here (noawaitoccurs betweenready()returning and the subsequentclick()), but the mixed styles obscure the intended safeguard and could silently regress if a yielding call is inserted between the two lines later.Standardize on the combined form throughout for consistency and defense-in-depth against future edits.
♻️ Example consolidation for the dry-run test (Lines 865-871)
- await ready(pilot, "`#ok`") - app.screen.query_one("`#dry-run`", Checkbox).value = True - await pilot.click("`#ok`") + await ready(pilot, "`#ok`") + app.screen.query_one("`#dry-run`", Checkbox).value = True + await pilot.click(await ready(pilot, "`#ok`"))As per PR objectives follow-up: "replace
await pilot.click(\"#ok\")withawait pilot.click(await ready(pilot, \"#ok\"))" as an additional safeguard.Also applies to: 611-617, 678-684, 764-770, 793-799, 823-844, 882-885, 901-906, 1110-1114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tui_app.py` around lines 564 - 567, The click-ready usage in the test helpers is inconsistent: several places still call ready() and pilot.click() as separate steps even though sibling tests use the combined pattern. Update the affected click sites in the test_tui_app.py flows to use the combined await pilot.click(await ready(pilot, "...")) form everywhere, including the helper-driven checks around ready() and pilot.click(), so the intended safeguard is consistent and future edits can’t insert a yield between them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_tui_app.py`:
- Around line 564-567: The click-ready usage in the test helpers is
inconsistent: several places still call ready() and pilot.click() as separate
steps even though sibling tests use the combined pattern. Update the affected
click sites in the test_tui_app.py flows to use the combined await
pilot.click(await ready(pilot, "...")) form everywhere, including the
helper-driven checks around ready() and pilot.click(), so the intended safeguard
is consistent and future edits can’t insert a yield between them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca0b7120-541a-40f6-91b8-5e58e36cac90
📒 Files selected for processing (6)
src/automator/install.pytests/conftest.pytests/test_engine.pytests/test_engine_worktree.pytests/test_tui_app.pytests/test_tui_settings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/automator/install.py
- tests/test_engine.py
|
I ran a rerun, and I think I caught if not all, then at least the most critical tests that are candidates for flakiness. |
The decision-modal choice was the one spot still gating with a standalone ready() immediately followed by pilot.click() on the same selector; fold it into pilot.click(await ready(...)) so no future edit can slip a yield between the mount gate and the click. The other ready() sites keep their standalone gate on purpose — they guard the .value= sets (or a query read) that sit between the gate and the interaction.
pbean
left a comment
There was a problem hiding this comment.
Approved ✅ — both asks verified landed.
Re-validated the head (ba12f18) against my earlier review:
- Exclude normalization scoped to source — the blanket
\\→/loop in_worktree_local_excludeis gone (net diff vs main is comment-only there); the fix is now exactlyseeded.append(rel.as_posix())at the glob source ininstall.py. Precise, and a provable no-op on POSIX. - Platform helpers de-duped —
_OK/_RUN/_file_exists_cmd/_touch_run/_exists_run/_seeded_then_touchhoisted intoconftest.pynext towrite_script_launcher; the engine test modules import them, sosys.platformbranching lives in one spot. (Also closes CodeRabbit's duplication nitpick.)
Re-confirmed Linux/macOS safety on the final diff: every gated test's POSIX branch is byte-identical or shell-equivalent, and the extra git status --short regression assertion in test_install.py is a nice guard for the exclude fix. All 10 checks green on ba12f18 (4 Linux + 4 Windows lanes, lint, version-sync).
Squash-merging as requested. Thanks for the fast, thorough turnaround — the Unity symlink-priming follow-up stays tracked separately.
…iew) test_cleanup.py's _dead_pid has never had a call site: 62818ed created the file by copying the helper out of test_runs.py — `Popen(["true"])` body and all — but never wrote one, and the count has stayed at the bare `def` ever since. So it was not part of bmad-code-org#293's failure set: an uncalled helper cannot resolve `true` on any PATH, which is also why bmad-code-org#31 fixed only the live copy when it added the native-Windows lane, and why three weeks of that lane never flagged it. Porting it would re-apply bmad-code-org#31's fix to code that never runs and re-sync a duplicate of test_runs.py:42 — recreating the duplication that let it drift. Delete it and its now-unused `import subprocess` instead; the coreutils dependency goes away either way. Also bound the new cleanup `wait()` in test_stop_run_signals_live_process, which every other wait in that file and in test_process_host.py already does.
What
Add a native
windows-latestCI job and the POSIX/Win32 test-gating that makes the suite green on it. Source is already POSIX-decoupled; this is the test-layer + CI half (the follow-up flagged in the win32-process-liveness PR).Why
The suite carried POSIX-only test constructs that failed — or killed the pytest runner mid-suite — on native Windows, and there was no Windows CI lane to catch regressions. A verified native-Windows baseline was 24 failures (WSL, being Linux, had masked the Windows-only ones). One real product bug hid among them: worktree git-exclude patterns were written with
\separators, which git never anchors.How
test-windows(windows-latest, py3.13) job running the deterministic suite;persist-credentials: falseon checkouts._worktree_local_excludenormalizes\→/before writing.git/info/exclude(git patterns are POSIX-slash on every platform; a no-op on POSIX).test -f/if exist) and dead-PID spawns ([sys.executable, "-c", ""]vs["true"]);real_signalusessignal.raise_signal(catchable in-process on both OSes — keeps Windows coverage instead ofTerminateProcess-killing the runner);HAVE_TMUXexcludes win32;USERPROFILEfor expanduser;echo hi& exit 7for cmd;encoding="utf-8"reads; a.cmd-shim launcher for fake CLIs;skipif(win32)on the 3 POSIX-only Unity symlink-priming tests.Testing
test_portability_guard.py(the existing regression guard) stays green. Full suite verified on native Windows (24 baseline failures → 0 in-scope) and WSL/Linux (1236 passed — no regression; ports are POSIX no-ops). Ruff clean. Remaining local-only failures (test_module_skills_sync) are workspace.claude/.agentsdrift that skips in CI and is out of scope.Follow-up
Native-Windows Unity Library symlink-priming (the 3 skipped tests) — tracked separately.
Summary by CodeRabbit