Skip to content

fix(tests): make run_tests.sh work on native Windows (#67385) - #67387

Closed
Hotragn wants to merge 2 commits into
NousResearch:mainfrom
Hotragn:fix/run-tests-windows-env
Closed

fix(tests): make run_tests.sh work on native Windows (#67385)#67387
Hotragn wants to merge 2 commits into
NousResearch:mainfrom
Hotragn:fix/run-tests-windows-env

Conversation

@Hotragn

@Hotragn Hotragn commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes the canonical test runner usable on native Windows. env -i's allowlist was POSIX-centric: it passed HOME, which CPython on Windows never reads — Path.home()/expanduser resolve via USERPROFILE (or HOMEDRIVE+HOMEPATH), ssl/sockets need SystemRoot, and tempfile needs TEMP/TMP. With those stripped, every test file touching Path.home() died at collection with RuntimeError: Could not determine home directory — all of tests/gateway/ included. Full traceback and analysis in #67385.

Also sets PYTHONUTF8=1: the runner and per-file subprocesses print / glyphs, which crash with UnicodeEncodeError under legacy Windows codepages (cp1252).

Hermetic intent preserved: none of the six passed-through vars carry secrets — credential vars stay stripped. All six are guarded on being set (${VAR:+...}), so the Linux/macOS environment is byte-identical to before; PYTHONUTF8=1 is a no-op there since LANG=C.UTF-8 already forces UTF-8. CI is unaffected either way — the workflows invoke run_tests_parallel.py directly, not this wrapper.

Related Issue

Fixes #67385.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • scripts/run_tests.sh — guarded passthrough of USERPROFILE, HOMEDRIVE, HOMEPATH, SYSTEMROOT, TEMP, TMP in the env -i block + PYTHONUTF8=1, with a comment documenting why each is needed on Windows.

How to Test

  1. Native Windows, Git Bash — no workarounds, ordinary uv/python -m venv .venv (post-consolidation, commit 97ce96b2e): bash scripts/run_tests.sh tests/gateway/test_42039_duplicate_user_message.py tests/agent/test_turn_overlap_tripwire.py
    • Before this PR: error: no virtualenv found at startup (Scripts/ layout unprobed); with HERMES_PYTHON supplied, RuntimeError: Could not determine home directory at collection (0 collected).
    • After: probe finds .venv/Scripts/python.exe, collection succeeds, 16/16 pass, no UnicodeEncodeError.
  2. Linux/macOS: no behavior change — the added vars are unset there (${VAR:+...}-guarded), bin/activate is still probed first, and UTF-8 mode was already in effect via LANG.

Regression guard: the fix is shell-level, so the guard is the reproducible command above rather than a test file. If maintainers want it mechanized, a small CI job running that exact command under windows-latest + Git Bash would lock both failure modes permanently — happy to add it here or as a follow-up.

Checklist

Code

  • I've read the Contributing Guide
  • Conventional Commits
  • I searched existing PRs and issues for prior attempts (none — mechanism + keyword search)
  • Single-topic diff
  • Test suite exercised — see How to Test (this PR is itself about making that possible on Windows)
  • Tested on my platform: Windows 11 (native); Linux env is provably byte-identical (guarded expansions)

Documentation & Housekeeping

  • Documentation — in-file comment added; no docs pages reference the env allowlist — or N/A
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md / AGENTS.md — N/A (behavior now matches what they already document)
  • Cross-platform impact — the entire point; see hermetic-intent note above

)

env -i's allowlist was POSIX-centric: it passed HOME, which CPython on
Windows never reads — Path.home()/expanduser resolve via USERPROFILE
(or HOMEDRIVE+HOMEPATH), and ssl/sockets need SystemRoot, tempfile
needs TEMP/TMP. With those stripped, every test file touching
Path.home() died at collection with 'RuntimeError: Could not determine
home directory' — taking out all of tests/gateway/ on native Windows,
the platform the PowerShell installer itself sets up.

Pass the six Windows vars through guarded on being set, so the
Linux/macOS environment is byte-identical to before; none of them carry
secrets, preserving the hermetic credential-stripping intent. Also set
PYTHONUTF8=1 so the runner's ✓/⚠ output doesn't crash with
UnicodeEncodeError under legacy Windows codepages — a no-op on
Linux/macOS where LANG=C.UTF-8 already forces UTF-8.

Verified on Windows 11: tests/gateway collection goes from RuntimeError
to green (16/16 across the previously-failing files).
@PRATHAMESH75

Copy link
Copy Markdown
Contributor

Reviewed against #67385. This looks correct, minimal, and well-scoped — it fully addresses both failure modes the issue describes.

What's solid

  • Guarded passthrough of the six Windows vars via ${VAR:+VAR="$VAR"}. On Linux/macOS these are all unset, so the expansions produce nothing and the resulting env -i line is byte-identical to before — the hermetic intent (stripping credential-bearing vars) is preserved because only these six non-secret vars are added.
  • Correct root cause: HOME was already passed but ntpath.expanduser/Path.home() on Windows read USERPROFILE (or HOMEDRIVE+HOMEPATH), which env -i was dropping → the RuntimeError: Could not determine home directory. at collection. Adding SYSTEMROOT (ssl/sockets) and TEMP/TMP (tempfile) covers the other Windows-only lookups the same env stripped.
  • PYTHONUTF8=1 fixes the UnicodeEncodeError on the / glyphs under the legacy cp1252 console codepage, and is a no-op where LANG=C.UTF-8 already forces UTF-8.
  • The explanatory comment block is accurate and will save the next reader the archaeology.

Minor notes (non-blocking)

  • Windows env-var casing: Git Bash typically exposes SYSTEMROOT uppercased, and the Windows CRT getenv is case-insensitive, so SYSTEMROOT= is picked up regardless — no change needed, just flagging the assumption.
  • Consider adding HOMEDRIVE/HOMEPATH only matters when USERPROFILE is absent; passing all three is harmless and matches CPython's own fallback order.

No behavior change on POSIX, real fix on native Windows, no new deps. LGTM pending the standard CI gates.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation needs-decision Awaiting maintainer decision before any implementation labels Jul 19, 2026
@alt-glitch

alt-glitch commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #66496 and #63642. The live branch combines native-Windows venv Scripts/python.exe discovery with a guarded Windows environment whitelist and UTF-8 mode; #66496 is broader runner work including the parallel-runner path, while #63642 overlaps the venv-layout repair. These overlapping scopes need a maintainer choice rather than a duplicate link.

@Hotragn

Hotragn commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@PRATHAMESH75 thanks for the careful review — agreed on both notes: Git Bash uppercases the vars and the Windows CRT getenv is case-insensitive, so SYSTEMROOT covers code reading SystemRoot; and HOMEDRIVE/HOMEPATH are there purely to mirror CPython's own fallback order when USERPROFILE is absent.

On alt-glitch's consolidation question — the three open runner PRs fix three independent breakages, and native Windows needs all three before scripts/run_tests.sh actually works end-to-end:

Breakage Symptom #63642 #66496 #67387 (this)
venv probe only checks bin/activate (Windows venvs use Scripts/) error: no virtualenv found at startup
env -i strips USERPROFILE/SYSTEMROOT/TEMP (Windows Python ignores the passed HOME) RuntimeError: Could not determine home directory at collection — kills all of tests/gateway/
/ glyphs vs legacy cp1252 stdio UnicodeEncodeError ✅ (runner-level) ✅ (PYTHONUTF8=1, env-level)

They compose cleanly: #66496's probe fix gets you a Python; this PR's whitelist gets that Python through collection. (#66496's repro file never touches Path.home(), which is why the whitelist gap didn't surface there.) The only overlap is the encoding fix — if maintainers land #66496's runner-level handling, I'm happy to drop PYTHONUTF8=1 from this diff on request, though the two are compatible (env-level covers any future child process, not just the runner's own prints).

Equally happy to fold the whitelist into #66496 instead if @TheSmokeDev and maintainers would rather take the runner work as one PR — whatever makes the merge easiest.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the Windows environment failure. The HOME premise is verified on current main: scripts/run_tests.sh:94-104 uses env -i and preserves only HOME; under a clean environment, ntpath.expanduser("~") remains ~ until USERPROFILE is supplied.

Problems

  • This patch alone does not make ordinary native-Windows venv invocation work: current scripts/run_tests.sh:45-63 only probes bin/activate and bin/python. The documented repro therefore requires HERMES_PYTHON.
  • Open #66496 modifies the same env -i block and adds the missing Scripts/activate / Scripts/python.exe probe. It overlaps this patch's whitelist work.

Suggested changes

  • Consolidate with #66496 (or include its Scripts/ probe here), retaining this PR's TEMP, TMP, and PYTHONUTF8 coverage.

Automated hermes-sweeper review.

Comment thread scripts/run_tests.sh
PATH="$PATH" \
HOME="$HOME" \
TZ=UTC \
LANG=C.UTF-8 \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open #66496 edits this same env -i block and adds the missing Scripts/python.exe native-Windows venv probe. Please consolidate the changes, retaining this PR's TEMP/TMP whitelist, so normal Windows venvs do not still require HERMES_PYTHON.

@alt-glitch alt-glitch removed the needs-decision Awaiting maintainer decision before any implementation label Jul 19, 2026
…HERMES_PYTHON

Consolidation requested by review: adopt NousResearch#66496's native-Windows venv
probe (python.exe and activate live under Scripts/ with no bin/) so the
env-whitelist fix in this branch is reachable with an ordinary
python -m venv / uv venv — no HERMES_PYTHON escape hatch needed.

Probe shape credited to NousResearch#66496.

Co-authored-by: TheSmokeDev <194188786+TheSmokeDev@users.noreply.github.com>
@Hotragn

Hotragn commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Consolidation done as requested: 97ce96b2e adopts #66496's Scripts/ venv probe (probe shape credited in-file and via Co-authored-by: TheSmokeDev), on top of this branch's env whitelist, TEMP/TMP, and PYTHONUTF8 coverage.

One correction to my earlier comparison table: #66496's env -i block already carries the four core vars (USERPROFILE/HOMEDRIVE/HOMEPATH/SYSTEMROOT) — the deltas this branch retains over it are TEMP/TMP and the env-level PYTHONUTF8, per the review.

Verified on Windows 11 with no HERMES_PYTHON and an ordinary uv-created .venv: bash scripts/run_tests.sh tests/gateway/test_42039_duplicate_user_message.py tests/agent/test_turn_overlap_tripwire.py → probe finds .venv/Scripts/python.exe, collection succeeds, 16/16 pass, no UnicodeEncodeError. POSIX path unchanged (bin/activate still probed first; new branch only fires when bin/ is absent).

#66496's run_tests_parallel.py encoding hardening and its test files remain complementary and are untouched here — @TheSmokeDev's PR still stands on its own for those.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 19, 2026
@alt-glitch alt-glitch added the needs-decision Awaiting maintainer decision before any implementation label Jul 19, 2026
@PRATHAMESH75

Copy link
Copy Markdown
Contributor

Follow-up on my earlier review — the new commit 97ce96b2 (native Windows venv Scripts/ layout) has landed since, so re-reviewing that addition specifically.

The venv-discovery change is correct and POSIX-neutral. On Linux/macOS a python -m venv/uv venv writes bin/activate, so the first [ -f "$candidate/bin/activate" ] branch matches and the loop breaks before ever testing Scripts/activate — the new branch is unreachable off Windows, and VENV_PYTHON="$candidate/bin/python" reproduces the previous $VENV/bin/python exactly. On native Windows the venv has no bin/; python.exe and activate live under Scripts/, so probing Scripts/activate and pointing PYTHON at Scripts/python.exe is the right shape (matches the #66496 probe). Threading the interpreter path through VENV_PYTHON instead of re-deriving "$VENV/bin/python" is what makes the two layouts diverge cleanly.

One scope note (non-blocking): this commit fixes venv discovery, which is a distinct Windows failure from the env -i var-stripping the issue #67385 actually describes. It's coherent under the PR's stated goal ("make run_tests.sh work on native Windows") and low-risk, but a maintainer may prefer it as its own commit/PR since it references a different root issue (#66496). Your call — folding it in is defensible.

No behavior change on POSIX, both Windows failure modes (venv layout + env-stripped USERPROFILE/SystemRoot/TEMP) now covered, no new deps. Still LGTM pending the standard CI gates.

@TheSmokeDev

Copy link
Copy Markdown
Contributor

Author of #66496 confirming the consolidation: #67387 is my preferred canonical run_tests.sh fix. The adopted Scripts/python.exe probe is correctly credited, and this branch now carries the complete wrapper-level Windows path (venv discovery, home/system/temp allowlist, and UTF-8 startup) with all required CI green.

Maintainers do not need to choose between competing wrapper diffs on my account. If this lands, I will close #66496 as superseded for the wrapper work. I will keep any direct-run_tests_parallel.py hardening separate only if a maintainer explicitly wants that defense-in-depth follow-up.

@alt-glitch alt-glitch removed the needs-decision Awaiting maintainer decision before any implementation label Jul 24, 2026

@monerostar monerostar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native Windows verification (monerostar)

Physical Windows 11 · Git Bash · Python 3.11.15 (Hermes venv) · PR head 97ce96b2e.

Control on current main (run_tests.sh allowlist = HOME only)

env -i PATH=… HOME=… TZ=UTC LANG=C.UTF-8 …
  python -c "from pathlib import Path; Path.home()"
→ RuntimeError: Could not determine home directory.

Also observed on main when running the canonical runner: progress printing dies with

UnicodeEncodeError: 'charmap' codec can't encode character '\u2713' … (cp1252)

This PR

Check Result
Scripts/ venv probe Present
USERPROFILE / HOMEDRIVE / HOMEPATH / SYSTEMROOT / TEMP / TMP Present (:+ guarded)
PYTHONUTF8=1 Present
Path.home() under PR’s env -i line OKC:\Users\Admin
bash scripts/run_tests.sh tests/test_hermes_constants.py -q Completes — no collection home crash, no glyph UnicodeEncodeError

Out of that file: 105 passed, 6 failed, 15 skipped. The 6 failures are symlink privilege (WinError 1314 / no developer mode symlink) in TestGetHermesDirnot caused by this PR; same class needs @skipif or Developer Mode, separate from #67385.

Relation to siblings

PR Overlap
#66496 Also has Scripts/ + home vars; plus _make_stdio_glyph_safe + encoding=utf-8 on runner self-tests — this PR relies on PYTHONUTF8=1 for glyphs (worked here) but doesn’t harden run_tests_parallel.py subprocess text=True decode
#70813 Asks for LOCALAPPDATA/APPDATA + parallel runner encoding="utf-8", errors="replace" — not in this PR; optional follow-up
#39347 Older / broader; prefer this or #66496

Recommend merge (or merge #66496’s parallel stdio bits into this one). CI green + live Win11 proof. Closes the “CONTRIBUTING says use run_tests.sh but Windows can’t” hole.

@monerostar

Copy link
Copy Markdown
Contributor

Opened a combined native-Windows runner PR that includes this allowlist + Scripts/ probe (rebased on current main, which already has the pytest-in-venv gate): #71591. Happy to close in favor of whichever the maintainers prefer.

@Hotragn

Hotragn commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #71591 (@monerostar) — it does the exact consolidation the needs-decision on this PR was waiting for: it combines this PR's env allowlist + PYTHONUTF8 + Scripts/ probe with #66496's parallel-runner encoding and #70813's LOCALAPPDATA/APPDATA vars, rebased on current main, and credits all three by name. I verified it preserves this PR's fix faithfully (same six guarded vars, same UTF-8 mode, same Scripts/ layout branch). One combined, rebased PR is the right thing to merge here rather than three overlapping ones. Thanks @monerostar for folding these together — #71591 is the one to land.

@Hotragn Hotragn closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: run_tests.sh cannot run tests touching Path.home() on native Windows — env -i strips USERPROFILE

6 participants