Add Windows install/uninstall scripts and venv support - #12
Add Windows install/uninstall scripts and venv support#12ImperatorRuscal wants to merge 4 commits into
Conversation
Adds install.cmd/uninstall.cmd for Windows users, updates _skill_bootstrap.py to resolve the bundled venv's Windows paths (.venv\Scripts\python.exe, .venv\Lib\site-packages), documents the Windows install flow in the README, and ignores the local pip download cache created by install.cmd.
|
|
There was a problem hiding this comment.
Thanks for this — Windows support is a real gap and the batch scripting here is unusually careful. I walked the setlocal/endlocal pairing, delayed-expansion inheritance into the subroutines, parse-time vs. runtime expansion inside every parenthesized block, caret escaping, robocopy's 0–7 success range, and subroutine fall-through, and found no defects in the cmd mechanics. The if not !ERRORLEVEL!==0 idiom and the comment explaining why if errorlevel N misreads negative CRT exit codes are both correct, and the USERPROFILE guard faithfully mirrors the HOME guard's rationale. Structure, naming, and error-message style track install.sh closely.
The problems aren't in the batch — they're in what the _skill_bootstrap.py change activates.
Blocking
os.execv does not block on Windows. Before this PR, _VENV_PY was .venv/bin/python3, which never exists on Windows, so the re-exec branch was dead there and Windows users got a loud ImportError with a nonzero exit — gates failed correctly. Pointing _VENV_PY at .venv\Scripts\python.exe turns that branch on. Windows has no real exec; CPython routes os.execv through the CRT's _wexecv (_wspawnv(_P_OVERLAY, …)), which CreateProcess-es the child and terminates the caller immediately — the caller's waiter sees the parent's exit status (0) while the child runs detached. This is long-standing documented behavior (bpo-19124, "os.exec* on Windows runs the new process in the background").
That matters here because script exit codes are workflow gates: preflight.py, validate-finding.py, validate-fix-plan.py, validate-result.py, validate-triage.py, build_graph.py, language-detect.py, and parse-tier-judgment.py all import _skill_bootstrap first and are consumed for their status, and run-gates.py:294 reads gate rc via subprocess. On Windows those gates would report success unconditionally.
It's also not an occasional-mismatch path. Windows venvs copy python.exe rather than symlinking it, so _same_interpreter() is false for every out-of-venv invocation — including the exact base interpreter that built the venv. Post-merge, the broken path becomes the default Windows path. See the inline notes on _skill_bootstrap.py and install.cmd:121; those two fixes are ordered dependencies and need to land together.
Should fix
preflight.py:382-383 wasn't updated (out of diff, so no inline). It still hardcodes .venv/bin/python3 and install.sh, so on Windows the first branch can never match and the hint falls through to "No installed skill at ~/.claude/skills/vulnhunter-fix/ … bash install.sh". Since the hint only prints when deps genuinely fail to import, the audience is a Windows user with a broken venv on an otherwise-installed skill — told nothing is installed, and told to run bash. That's exactly the dead-end the function's docstring says it exists to prevent. Worth mirroring the os.name == "nt" branch and emitting install.cmd.
While you're in there: the elif os.path.isfile(install_sh) branch at preflight.py:397 is dead on every platform — install.sh lives at repo root and is never copied into the installed skill dir by either installer. Pre-existing, but adjacent.
Minor
_same_interpreter() compares realpath results case-sensitively. On CPython ≥3.8 ntpath.realpath returns canonical on-disk case for existing files, and both operands are guaranteed to exist, so this only bites on the resolution fallback (permissions, exotic paths) — and on Windows the comparison is already false for the copied-exe reason above. os.path.normcase() is still correct hardening, just low priority.
One more class of bug the POSIX scripts don't have: with EnableDelayedExpansion active, a USERPROFILE containing ! corrupts paths during %-expansion inside blocks. Rare enough that I wouldn't block on it.
Testing
The repo has no CI, so nothing lints the batch files or exercises the bootstrap's Windows branch — and that branch is unreachable from CI regardless, since it's gated on os.name == "nt". A cheap improvement would be factoring the path construction into a helper that takes the OS name as a parameter, making both layouts unit-testable on any platform.
On the test plan: bullet 3 ("verified _skill_bootstrap.py's Windows path branch matches the venv layout") can't have exercised what it claims, for the reason in the install.cmd:121 comment — the smoke test cannot currently fail on Windows. Worth re-running the full plan once the exec fix lands.
Also flagging scope, not as a blocker: installing on Windows doesn't yet mean the skills run there. vulnhunter-fix/SKILL.md Step 0b's staleness check is a bash snippet, and harness/ and vulnhunter-agent/ still assume POSIX.
Security
Nothing concerning. Every rmdir /s /q target derives from %USERPROFILE%\.claude\skills\<literal-name> where the name comes from a hardcoded list — no argument, stdin, or other environment input reaches a delete path, and both scripts guard on USERPROFILE being set first. robocopy preserves the deliberate copy-not-symlink decision, and the pip specifiers are the same pinned, bounded set install.sh uses.
Happy to re-review once the exec/smoke-test pair is sorted.
Disclosure: this review was prepared with AI assistance (Claude Code). The Windows-specific runtime claims — os.execv/CRT exec emulation, venv interpreter copy-vs-symlink, PowerShell command discovery — were derived from the code plus documented platform behavior and cross-checked by a second model, but were not executed on Windows. Please verify them against a real Windows run before acting. Everything else was checked directly against this checkout.
os.execv doesn't block on Windows (CPython routes it through the CRT's _wexecv/_wspawnv, which returns control to the caller with the parent's exit status while the child runs detached). Pointing _skill_bootstrap's re-exec at the bundled venv turned this dead-on-POSIX-only branch into the default Windows path, silently making every gate/smoke-test that depends on this script's exit code report success unconditionally. - _skill_bootstrap.py: use a blocking subprocess on Windows instead of os.execv; also skip re-exec for `-c` invocations, which would lose the code argument. - install.cmd: smoke-test the venv's own interpreter directly instead of routing through the (previously broken) re-exec. - install.cmd: move the Python-3.11+ requirement into build_vulnfix_venv so vulnhunt/vulnhunt-fix-verify (which need no venv) aren't blocked by a missing interpreter, restoring parity with install.sh. - install.cmd: dedupe the dep-pin string, check the pip self-upgrade's exit code, normcase the interpreter comparison. - preflight.py: bootstrap hint now branches on os.name=="nt" and points Windows users at install.cmd; also fixes a dead branch (it checked for install.sh under the installed skill dir, which is never copied there by either installer) by checking for SKILL.md instead. - preflight.py: check_memory/check_disk_space used os.sysconf/statvfs, which don't exist on Windows and crashed preflight.py unconditionally there, independent of anything above. Added GlobalMemoryStatusEx/ GetDiskFreeSpaceExW-based implementations via ctypes. - .gitignore, README.md: drop a stray pip/ ignore entry unrelated to anything install.cmd creates, and use .\install.cmd/.\uninstall.cmd so the documented commands work in PowerShell too.
|
Addressed the listed concerns, thank you for the check and feedback. While working through I also found part of preflight that should normally fail on Windows (my system has some existing workarounds that hid it from me initially). So I put a bit of branching in preflight when it checks disk space & RAM to call native MS Win handles in order to get good values. This would have been a block against full execution for most Windows environments (without special workarounds) -- but I have now tested in a vanilla sandbox and can confirm it works on both the install & uninstall path. Asked Claude Code to do a Review based on the Requested Changes / comments from the PR and here are the results of that PR #12 review items — status
Additional fixes beyond the current PR discussionThese were not raised anywhere in the PR #12 review thread — found while verifying the fixes above by actually running
|
schenksj
left a comment
schenksj
left a comment
There was a problem hiding this comment.
Re-reviewed at c9b4b7c. All ten items from the previous round are addressed, and both blockers are fixed correctly and in the right order. The two extra bugs you found while verifying — os.sysconf / os.statvfs being absent on Windows — are real and were genuinely fatal: check_memory's except (ValueError, OSError) doesn't catch AttributeError, and check_disk_space had no guard at all, so main() died before printing anything. Good catches, and exactly the class of thing that only surfaces by running it.
Verified
- 1/2 —
_skill_bootstrap.py. Blockingsubprocess.runon Windows,os.execvretained on POSIX, and the exit code propagated viasys.exit(...returncode). This also makesrun-gates.py:294correct on Windows: the child'ssys.executableis now the venv interpreter, so the per-gate subprocess invocations inherit it as the comment atrun-gates.py:290-292assumes.-cguard is in place. - 3 —
install.cmd:131. Smoke test runs"%venv%\Scripts\python.exe"directly. Bypasses the re-exec entirely, so it can now actually fail. - 4 —
install.cmd:91-97.find_pythonmoved into:build_vulnfix_venvwith a correctendlocal & exit /b 1.install.shparity restored: a Windows box without Python 3.11+ can install the two prompt-only skills again, and the hotfix-branch skip path at line 50 is reachable. - 5/6 —
preflight.py:430-470. Branches onos.name; the deadinstall.shbranch is replaced with aSKILL.mdcheck, and all three hint messages now point at the source checkout rather than a path that was never there. - 7
normcaseadded. 8VULNFIX_DEPSdeduped, pip self-upgrade errorlevel checked. 9.gitignoreentry dropped. 10.\install.cmd.
Re-walked the batch mechanics on the new code. set VULNFIX_DEPS="jsonschema>=4.18" "graphifyy>=0.8.14,<0.9.0" is safe: the > and < sit inside double quotes in the raw line so redirection parsing never sees them, and because the quotes are part of the value, they survive %-expansion and protect the same characters again at line 121 and line 122. setlocal EnableDelayedExpansion at line 87 is a no-op change (the mode was already inherited from line 2) but it's clearer to state it.
Ran preflight.py on macOS / CPython 3.13 as a POSIX regression check for the ctypes refactor — 9 passed, 2 failed (expected: no graphifyy/jsonschema in that env), memory and disk both report real figures, and the bootstrap hint renders the POSIX branch correctly. No regression from the extraction.
Should fix
1. preflight.py:440 hands PowerShell users the bare form that item #10 just removed from the README.
run_install_hint = " install.cmd".\install.cmd is the form that works in both shells — same reasoning as the README fix, same audience.
2. check() throws away detail on the success path, so the disk-space fallback is invisible.
check() only appends detail inside the else branch (preflight.py:31-42). So the new check("Disk space", True, "cannot determine — skipping check") at line 137 prints a bare [ok] Disk space — a Windows user whose GetDiskFreeSpaceExW call fails is told the check passed, with no hint it was skipped. Fold the note into the name, the way check_memory's success path already does:
check("Disk space (could not determine — skipped)", True)Line 183's check("Memory", True, "cannot determine — ...") has the identical problem and predates this PR.
3. GetDiskFreeSpaceExW is reading the wrong output parameter.
free_bytes is passed in the 4th slot — lpTotalNumberOfFreeBytes, total free on the volume. The POSIX side uses f_bavail, which is space available to an unprivileged caller. The direct analogue is the 2nd parameter, lpFreeBytesAvailableToCaller. Under a disk quota the two diverge and the check gets more optimistic than its POSIX twin. One-argument change:
if not ctypes.windll.kernel32.GetDiskFreeSpaceExW(
ctypes.c_wchar_p(os.path.abspath(path)), ctypes.byref(free_bytes), None, None
):Minor
-
preflight.py:165— the lastMEMORYSTATUSEXfield is spelledsullAvailExtendedVirtual; the Win32 name isullAvailExtendedVirtual. Purely cosmetic: the layout is unaffected (4 + 4 + 7×8 = 64, which is thedwLengththe API expects), so the call is correct as written. -
install.sh:68's comment still reads "re-execs under venv python if needed". With the newsys.argv[0] != "-c"guard that smoke test can no longer re-exec by construction, so the comment is now false. -
The Windows smoke test covers strictly less than the POSIX one: it proves the deps landed in the venv, but no longer that
_skill_bootstrapimports at all (a syntax error there would sail through). You can have both — run the venv interpreter and import the bootstrap. Under the venv Python_same_interpreter()is true, so no re-exec fires and there's no detach risk:"%venv%\Scripts\python.exe" -c "import sys; sys.path.insert(0, r'%skill_dir%\scripts'); import _skill_bootstrap; import jsonschema, graphify" >nul 2>&1
-
The dep echo now prints the pins with their quotes (
"jsonschema>=4.18" ...), whereinstall.shprints them bare. Cosmetic drift.
Follow-ups, not blockers
vulnhunt/SKILL.md:331,vulnhunt-fix-verify/SKILL.md:89, andvulnhunter-fix/SKILL.md:123/137/138still instruct users to runinstall.sh/./install.sh— including the Step 0b staleness-check remediation, which is the one a Windows user is most likely to hit.preflight.pygot its Windows branch; these didn't. Docs-only, fine as a separate PR.- Still no CI, and this commit adds two more Windows-only
ctypesblocks that nothing on Linux/macOS can exercise. Same suggestion as last round: parameterizing the OS-dependent bits would at least make them unit-testable everywhere. !in%USERPROFILE%underEnableDelayedExpansion. Still rare enough not to block on.
Security
Unchanged from the last pass and still clean. The new ctypes calls are read-only system queries with no caller-controlled input reaching them; os.path.abspath(".") is the only argument. Every delete target still derives from %USERPROFILE%\.claude\skills\<literal>.
Approving — the blockers are resolved and everything above is small. Thanks for the thorough turnaround, and for going and running it rather than just patching to the review.
Disclosure: prepared with AI assistance (Claude Code). I have no Windows host here, so the batch-parsing and Win32/ctypes claims above are from static reading plus documented platform behavior, not execution. The POSIX regression run of preflight.py, the line references, and the check() behavior were verified directly against this checkout.
schenksj
left a comment
schenksj
left a comment
There was a problem hiding this comment.
Switching to Changes Requested — narrowly, for should-fix item 2 from my previous review. Everything else in that review stands: both original blockers are correctly fixed and I'm satisfied with the rest. This is the one item I'd like to see land before merge, because it's the difference between a Windows user learning a check was skipped and being told it passed.
The fix belongs in check() itself, not at the call sites
preflight.py:31-42 only appends detail inside the failure branch:
if passed:
CHECKS_PASSED += 1
print(f" [ok] {name}")
else:
if not optional:
CHECKS_FAILED += 1
msg = f" [WARN] {name}" if optional else f" [FAIL] {name}"
if detail:
msg += f" — {detail}"
print(msg)So the new check("Disk space", True, "cannot determine — skipping check") at line 137 renders as a bare [ok] Disk space. On a Windows box where GetDiskFreeSpaceExW fails, the operator is told the 5 GB requirement passed when it was never evaluated — and preflight exists precisely to stop the pipeline running into that.
I walked all 29 check() call sites in the file to confirm this is safe to fix centrally. Every one of them either passes False, passes no detail at all, or guards the detail with the file's existing ... if not passed else "" idiom. The only two that pass a non-empty detail alongside passed=True are the two "cannot determine" fallbacks — line 137 and line 183. So mirroring the failure branch changes exactly those two lines of output and nothing else:
def check(name: str, passed: bool, detail: str = "", optional: bool = False):
global CHECKS_PASSED, CHECKS_FAILED
if passed:
CHECKS_PASSED += 1
msg = f" [ok] {name}"
else:
if not optional:
CHECKS_FAILED += 1
msg = f" [WARN] {name}" if optional else f" [FAIL] {name}"
if detail:
msg += f" — {detail}"
print(msg)Fixing it here rather than folding the note into the name argument (which is what I suggested last round) also repairs line 183's Memory fallback, which has had the same silent-skip behaviour since before this PR — and which your AttributeError addition at line 182 makes reachable on Windows for the first time. Two fallbacks, one place.
Worth a line in the test plan too: on a machine where both queries succeed there's no observable difference, so the way to check this is to force the exception path once and confirm the reason actually prints.
The other two should-fix items from the last round — the bare install.cmd hint at line 440, and the GetDiskFreeSpaceExW out-parameter (4th slot is total-free; the POSIX side's f_bavail maps to the 2nd, free-to-caller) — are still worth doing, but I'm not blocking on either. Ping me when this one's in and I'll re-approve.
…nd [ok] check() only appended `detail` on the failure branch, so the disk-space and memory "cannot determine" fallbacks (passed=True with an explanatory detail) rendered as a bare [ok] with no indication the check was actually skipped. Mirror the detail-append logic into the success branch so the reason always reaches the operator.
- The bootstrap hint printed a bare `install.cmd`, which PowerShell can't resolve since it excludes cwd from command discovery (same issue already fixed in README.md). Use `.\install.cmd` to match. - _free_disk_bytes read GetDiskFreeSpaceExW's lpTotalNumberOfFreeBytes (4th out-param, quota-blind) instead of lpFreeBytesAvailableToCaller (2nd out-param), overreporting how much space is actually usable and diverging from the POSIX side's statvfs f_bavail (quota-aware).
|
@schenksj -- I have pushed the fix for the blocking item plus both non-blocking should-fix items from your latest review:
Also added regression tests per your note about forcing the fallback path: TestCheckDetailReporting exercises check() directly and both real call sites with their helpers monkeypatched to raise, and TestDiskSpaceQuotaAware fakes GetDiskFreeSpaceExW to prove the correct out-parameter slot is read (skipped on non-Windows). Ready for re-review whenever you get a chance. |
3 participants
Summary
install.cmd/uninstall.cmdso Windows users can install/removethe skills without WSL or a POSIX shell (mirrors the existing
install.sh/uninstall.shbehavior).vulnhunter-fix/scripts/_skill_bootstrap.pyto resolve thebundled venv's Windows layout (
.venv\Scripts\python.exeand.venv\Lib\site-packages) instead of only checking the POSIX.venv/bin/python3path, so the interpreter/site-packages bootstrapworks correctly on Windows.
Test plan
install.cmdon Windows (cmd.exe), confirmed it copies theskills into
%USERPROFILE%\.claude\skills\and creates thevulnhunter-fixvenv at.venv\Scripts\python.exewith.venv\Lib\site-packagespopulated._skill_bootstrap.py's Windows path branch matches thevenv layout actually produced by the install.
uninstall.cmdon Windows; completed without errors andremoved the installed skills as expected.