[#509] Cross-platform stormtest + serve-this-worktree pathing#516
Open
dsarno wants to merge 4 commits into
Open
[#509] Cross-platform stormtest + serve-this-worktree pathing#516dsarno wants to merge 4 commits into
dsarno wants to merge 4 commits into
Conversation
The stormtest invocation surface assumed POSIX. Make the documented commands identical on every OS so a Windows agent can copy-paste and run. - script/_dev_env.py: shared, stdlib-only, import-safe helpers — venv interpreter resolution (bin/python vs Scripts\python.exe), git worktree/ root-repo resolution, a venv re-exec decision, and cross-platform port freeing (lsof vs netstat/taskkill). - script/stormtest.py: re-exec into the project .venv on launch so `python script/stormtest.py` works everywhere (no .venv/bin/python vs .venv\Scripts\python.exe split). Opt out with SS_NO_REEXEC=1. - script/serve-this-worktree.py: cross-platform (no bash/lsof) companion to the bash serve-this-worktree; extra args like --ws-port pass straight through. The bash script is retained for POSIX. - Host-side path construction in stormtest already goes through tempfile.gettempdir()/os.path.join — confirmed resilient; only the invocation surface needed work. - docs/STRESS_TESTING.md + AGENTS.md: document the OS-agnostic commands and trim the invocation/serve-script parts of the Windows heads-up. The reload-churn caveats (#513, and #514's kill-on-reload product decision) are unrelated and deliberately left in place. Tests: tests/unit/test_dev_env.py covers the platform branches (Windows layout exercised on POSIX CI), port/arg parsing, the re-exec decision, and a compile smoke of all three scripts. https://claude.ai/code/session_01FAB4kCYfQNGP4LQTVbPRTM
Drop the O(n) `not in pids` membership checks in parse_lsof_pids / parse_netstat_pids; dedup once at return, preserving order. https://claude.ai/code/session_01FAB4kCYfQNGP4LQTVbPRTM
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
_port_listening only probed 127.0.0.1, so a dev server bound to ::1 (common on Windows, the platform this targets — see #511) would go undetected and free_port would no-op, leaving the external server unable to bind. Probe both AF_INET and AF_INET6. Adds a test for the listening probe. https://claude.ai/code/session_01FAB4kCYfQNGP4LQTVbPRTM
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the stormtest/dev-server workflow cross-platform by moving OS-specific logic (venv interpreter pathing and port freeing) into a shared stdlib-only helper module, updating the runner to auto re-exec into the project .venv, and adding a Python equivalent of the existing POSIX-only serve-this-worktree bash script. It also updates docs to advertise OS-agnostic invocation and adds unit tests for the new helper behaviors.
Changes:
- Add
script/_dev_env.pywith helpers for worktree/root-repo resolution, venv interpreter resolution, venv re-exec, and cross-platform port freeing. - Update
script/stormtest.pyto re-exec into.venvautomatically sopython script/stormtest.pyworks on all OSes. - Add
script/serve-this-worktree.pyas a cross-platform (non-bash) way to run the dev server from the current worktree’ssrc/, and update docs/tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
script/_dev_env.py |
New shared helper module for cross-platform dev-script behavior (venv pathing, worktree/root-repo detection, port freeing, re-exec). |
script/stormtest.py |
Adds auto re-exec into the project venv before importing third-party deps; updates documented invocation. |
script/serve-this-worktree.py |
New Python entrypoint to launch the dev server from the current worktree with --reload, freeing the chosen port first. |
tests/unit/test_dev_env.py |
New unit tests covering OS-branching logic, parsing helpers, and the venv re-exec decision. |
docs/STRESS_TESTING.md |
Updates invocation instructions to python ... and refreshes Windows/cross-platform guidance. |
AGENTS.md |
Updates the script inventory and stormtest section to reference the new cross-platform invocation/serve script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review feedback on PR #516: - extract_port now raises ValueError on a missing/non-integer --port instead of silently using the default (a typo'd port would otherwise start the server on 8000 while the editor waits on the intended port). serve-this- worktree.py catches it and exits 1 with a clear message. - docs/STRESS_TESTING.md: note the SS_* examples use POSIX inline-env syntax and give the PowerShell equivalent ($env:SS_FOO=...; python ...). https://claude.ai/code/session_01FAB4kCYfQNGP4LQTVbPRTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the
stormtestinvocation surface platform-agnostic so a Windows agent can copy-paste one command and run, removing the POSIX-only steps (.venv/bin/python, bash-onlyserve-this-worktree) called out in #509.script/_dev_env.py— new, stdlib-only, import-safe shared helpers: venv interpreter resolution (bin/pythonvsScripts\python.exe), git worktree/root-repo resolution, a venv re-exec decision, and cross-platform port freeing (lsofvsnetstat/taskkill).script/stormtest.py— re-execs into the project.venvon launch, sopython script/stormtest.pyworks identically on every OS. Opt out withSS_NO_REEXEC=1.script/serve-this-worktree.py— new cross-platform (nobash/lsof) companion to the bashserve-this-worktree; extra args (e.g.--ws-port) pass straight through. The bash script is retained for POSIX.tempfile.gettempdir()/os.path.join— audited and confirmed resilient; only the invocation surface needed work (issue item 3).docs/STRESS_TESTING.md+AGENTS.md— document the OS-agnostic commands and trim the invocation/serve-script parts of the Windows heads-up.Test plan
ruff check+ruff format --checkon all changed/new files — clean.pytest -qfull suite — 1158 passed, 2 skipped.tests/unit/test_dev_env.py(new, 18 tests) — venv layout for both OSes (Windows branch exercised on POSIX via an explicit flag, no globalos.nameflip), worktree/root-repo resolution,--portextraction,lsof/netstatPID parsing, the re-exec decision (no-op when already in-venv / guarded / opted-out / no venv; execs otherwise), and apy_compilesmoke of all three scripts.script/ci-check-gdscript— "All GDScript files OK" (no GDScript touched; run for safety).find_venv_python()resolves the real.venv;serve-this-worktree.pyresolves worktreesrc/+ venv and parses args; re-exec is a no-op when already running the venv interpreter.test_run/ self-update / write-tool-undo smokes — this change is Python tooling + docs only; it touches no plugin/GDScript, self-update, or write-tool code.Deviations / notes
main, notbeta.origin/betais currently stale (v2.4.4 era,9c95fda) and mid-reset (cf.archive/beta-pre-reset-20260531,claude/merge-main-into-beta-*); it does not containscript/stormtest.py(test: add stormtest concurrency/reload stress harness #510) ordocs/STRESS_TESTING.md(docs(stormtest): Windows/cross-platform pathing heads-up (stacked on #510) #512) — the very files stormtest: make runner pathing platform-agnostic (Windows) #509 targets. Basing onbetawould produce a ~1000-line bogus diff re-introducing test: add stormtest concurrency/reload stress harness #510/docs(stormtest): Windows/cross-platform pathing heads-up (stacked on #510) #512.mainis the live branch (v2.5.13) and the state stormtest: make runner pathing platform-agnostic (Windows) #509 references, so this PR targets it. Happy to retarget if a different integration branch is preferred._stop_serverkill-on-reload product decision are out of scope and intentionally left documented as open caveats — they are not pathing problems. The newserve-this-worktree.pydoes incidentally give Windows a non-bash serve path (overlapping stormtest: external-server reload mitigation (serve-this-worktree) doesn't work on Windows #514's Problem 1), but does not change any reload/kill behavior.Closes #509
Generated by Claude Code