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. |
|
|
||
| // Uses std::os::unix::fs::symlink — symlink creation differs on Windows | ||
| // (requires elevated privileges), so this escape test is Unix-only. | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
This may not need conditional compilation. It's often more reliable to check cargo envvars at compile time since linting works on all platforms. Worth double checking.
There was a problem hiding this comment.
Good instinct, but here the gate has to be a compile-time #[cfg] rather than a runtime cfg!(...): the test body calls std::os::unix::fs::symlink, which does not exist in the standard library on Windows. A runtime cfg!(unix) (or a cargo-env check) still compiles the call on every target, so the test would fail to build on windows-msvc with cannot find function symlink in os::unix. #[cfg(unix)] excludes it at compile time, which is what we need. Windows keeps the non-symlink containment / ..-traversal coverage (canonicalize_dotdot, cd_dotdot, the other bind_direct_* tests). Happy to revisit if there is a portable symlink helper you would prefer.
There was a problem hiding this comment.
It looks like the same function but you used an import differently and blocked. A strategy for conditional compilation around the unix import and symlink and cargo env check around the blocking behaviour might reduce this to one function, which could then be inlined ~possibly. Only if it improves readability.
There was a problem hiding this comment.
These two tests call std::os::unix::fs::symlink, which does not exist in the Windows std — so #[cfg(unix)] is a hard compile-time requirement here, not a runtime guard. Without it the test module won't compile on Windows at all. (#[cfg(unix)] is a compile-time check — Windows simply compiles these two fns out.) The rest of the VFS containment suite, including the ..-traversal escape tests, still runs on Windows. Happy to add a Windows-native symlink variant (std::os::windows::fs::symlink_file, needs Developer Mode/elevation) as a follow-up if you want deeper coverage there.
|
|
||
| // Uses std::os::unix::fs::symlink — symlink creation differs on Windows | ||
| // (requires elevated privileges), so this escape test is Unix-only. | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Same reasoning as the other symlink test — std::os::unix::fs::symlink is absent on Windows, so this needs the compile-time #[cfg(unix)] to build at all.
There was a problem hiding this comment.
Same as above — std::os::unix::fs::symlink is Unix-only in std, so #[cfg(unix)] is required for the dangling-symlink test to compile on Windows. The non-symlink containment tests still cover the Windows path.
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.