feat: add Windows support#44
Conversation
Strands Shell did not compile on Windows because src/vfs_kernel.rs used an unconditional Linux-only TOCTOU check (std::os::unix::io::AsRawFd plus /proc/self/fd) in the host-file read path, and two integration tests created symlinks via std::os::unix::fs::symlink. Core fix (src/vfs_kernel.rs): - Gate the /proc/self/fd defense-in-depth TOCTOU verification behind #[cfg(target_os = "linux")]. /proc/self/fd is a Linux-only procfs interface (macOS has no /proc; Windows has no procfs), so the check only ever functioned on Linux. The canonical-path check performed before open_host() remains the primary security boundary on all platforms, so this narrows the platform of an extra layer rather than weakening the guarantee on Linux. - Silence the now-conditionally-unused canon_base arg on non-Linux targets. Tests (tests/shell_integration.rs): - Gate bind_direct_symlink_escape_blocked and bind_direct_dangling_symlink_blocked behind #[cfg(unix)] since std::os::unix::fs::symlink is unavailable on Windows (symlink creation there also requires elevated privileges). CI (.github/workflows/ci.yml): - Add windows-latest to the rust, python, and node test matrices. - Make the python job cross-platform: create the venv and prepend its bin/Scripts dir to GITHUB_PATH instead of hardcoding .venv/bin. - On Windows, set npm script-shell to bash so package.json's $(npm run --silent host-triple) command substitution works. CD (.github/workflows/release.yml, package.json): - Build + publish Windows artifacts: x86_64-pc-windows-msvc Python wheel, node addon, and the strands-agents-shell-win32-x64-msvc npm package. - Update the inspect coverage check (win_amd64 wheel) and the npm pack count guard (5 -> 6 packages). Validated by cross-compiling lib, bins and all tests to x86_64-pc-windows-gnu (clean), cross-checking the python feature with PYO3_CROSS_PYTHON_VERSION, and running the full 1296-test suite on Linux.
Review Loop SummaryThis PR passed the pre-PR fresh-context review gate (
What was independently verified1. Core fix (
2. Test gating — confirmed correct. 3. CI/CD — confirmed correct. 4. Readiness re-run cold (Linux + Windows cross):
(Note: a full windows-gnu DLL link fails locally with mingw Remaining findingsTwo 🟢 nice-to-have doc-comment nits in Automated pre-PR review gate · iteration 1 of max 3 · converged on a clean review. |
Two native-Windows-only failures that the windows-gnu cross-compile gate could not catch (cross-compile never runs tests nor builds the Python module): - tests/shell_integration.rs: the config_file_with_binds_and_creds test interpolated a Windows temp path (C:\Users\...) into a TOML *basic* string, where \U/\A are parsed as invalid escape sequences -> TOML parse error. Switched to a TOML *literal* string (single quotes), which does no escape processing; Windows paths never contain single quotes. - .github/workflows/ci.yml: on Windows the venv Scripts dir was added to GITHUB_PATH using the Git Bash $PWD (an MSYS path like /d/a/shell/shell) that the Windows PATH cannot resolve, so the venv was silently ignored and pytest ran from the host interpreter -> ModuleNotFoundError: strands_shell. Convert with cygpath -w so .venv\Scripts is actually used. Also refresh stale doc-comment counts in release.yml (4->5 platform packages, 5->6 total) now that win32-x64-msvc is included; the enforced expected=6 guard and matrices were already correct. Verified locally: cargo test --workspace --all-targets (1296+ pass), maturin develop + pytest tests/python (44 pass), cargo fmt/clippy/doc.
f26bd58
🔴 Fixed two real native-Windows CI failures (+ addressed review nits)TL;DR: CI was red on the native Root causes & fixes1. Rust —
|
chaynabors flagged the TOCTOU comment block as overly verbose
('self explanatory to a rust engineer'). Condense to a concise
rationale while preserving the essential 'why Linux-only' note.
No functional change.
Summary
Makes Strands Shell build, test, and ship on Windows. Closes the gap identified in the linked research: the crate didn't compile on Windows out of the box.
The only non-portable code in the library was a single Linux-only TOCTOU check in the host-file read path. Everything else was already cleanly
#[cfg]-gated.Root cause
src/vfs_kernel.rs::open_host()(read branch) unconditionally usedstd::os::unix::io::AsRawFd+/proc/self/fd/<fd>to re-verify, afteropen(), that the opened fd still pointed inside the bind mount. This:std::os::unixis absent →E0433/E0599), and/proc), it just happened to compile there because macOS has thestd::os::unixmodule.Changes
Core fix —
src/vfs_kernel.rs/proc/self/fddefense-in-depth TOCTOU verification behind#[cfg(target_os = "linux")]. This is purely a Linux procfs interface, so the check only ever ran on Linux. The primary security boundary is unchanged: the canonical-path containment check performed beforeopen_host()still runs on every platform. This narrows the platform of an extra defense layer rather than weakening Linux's guarantee.canon_baseis now only consumed by that Linux-only block, so it's explicitly marked unused on other targets (no warnings).Tests —
tests/shell_integration.rsbind_direct_symlink_escape_blockedandbind_direct_dangling_symlink_blockedbehind#[cfg(unix)]. They callstd::os::unix::fs::symlink, which doesn't exist on Windows (and symlink creation there needs elevated privileges). They continue to run on Linux/macOS.CI —
.github/workflows/ci.ymlwindows-latestto the rust, python, and node matrices.bin/Scriptsdir toGITHUB_PATHinstead of hardcoding.venv/bin.script-shelltobashsopackage.json's$(npm run --silent host-triple)command substitution works (npm defaults tocmd.exethere).CD —
.github/workflows/release.yml,package.jsonx86_64-pc-windows-msvcto: the Python wheel matrix, the Node addon matrix,napi.targets, and the publish-platform matrix (newstrands-agents-shell-win32-x64-msvcpackage).win_amd64wheel) and bump the npm pack count guard5 → 6.Validation
x86_64-pc-windows-gnu— clean, no warnings.pythonfeature for Windows viaPYO3_CROSS_PYTHON_VERSION.#[cfg(unix)]-gated symlink escape tests.windows-latestCI legs (native MSVC build of Rust/Python/Node) will be the authoritative cross-platform proof once this PR's CI runs.cc @mkmeral — opened as a draft for your review.