Skip to content

feat: add Windows support #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
agent-of-mkmeral wants to merge 2 commits into
base: main
Choose a base branch
from
+80 −28
Open

feat: add Windows support #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd408e9
feat: add Windows support
agent-of-mkmeral Jun 24, 2026
f26bd58
fix(ci): make Windows Python/Rust CI legs pass
agent-of-mkmeral Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v6
Expand All @@ -43,7 +43,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
os: [ubuntu-latest, macos-latest, windows-latest]
python: ["3.10", "3.11", "3.12", "3.13", "3.14"]

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Technically speaking, since this is a rust project the whole CI/CD including maturin could be rust.

@agent-of-mkmeral agent-of-mkmeral Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed in principle — a Rust-driven CI/CD (e.g. an xtask that wraps maturin/napi) would be cleaner and more consistent. This PR keeps the existing shell-based matrix to stay scoped to Windows enablement, but it's a good follow-up; happy to file an issue for a CI-in-Rust pass if you'd like.

runs-on: ${{ matrix.os }}
steps:
Expand All @@ -62,17 +62,27 @@ jobs:
with:
python-version: ${{ matrix.python }}

# Create the venv and put its bin/Scripts dir on PATH so the
# subsequent steps work identically on Unix (.venv/bin) and
# Windows (.venv/Scripts).
- name: Create virtualenv
run: python -m venv .venv
shell: bash
run: |
python -m venv .venv
if [ "$RUNNER_OS" == "Windows" ]; then
echo "$PWD/.venv/Scripts" >> "$GITHUB_PATH"
else
echo "$PWD/.venv/bin" >> "$GITHUB_PATH"
fi

- name: Install build deps
run: .venv/bin/pip install maturin pytest
run: pip install maturin pytest

- name: Build and install wheel
run: .venv/bin/maturin develop --release
run: maturin develop --release

- name: pytest
run: .venv/bin/pytest tests/python -v
run: pytest tests/python -v

audit:
name: Security audit
Expand Down Expand Up @@ -115,7 +125,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
os: [ubuntu-latest, macos-latest, windows-latest]
node: ["20", "22", "24"]
runs-on: ${{ matrix.os }}
steps:
Expand All @@ -134,6 +144,15 @@ jobs:
with:
node-version: ${{ matrix.node }}

# The package.json build scripts use `$(npm run --silent host-triple)`
# command substitution. npm runs script bodies with its configured
# script-shell, which defaults to cmd.exe on Windows (no `$(...)`
# support). Point it at the Git Bash that ships on the windows-latest
# runner so the substitution works identically across platforms.
- name: Use bash as npm script-shell (Windows)
if: runner.os == 'Windows'
run: npm config set script-shell bash

- name: npm install
# package-lock.json is gitignored, so `npm ci` can't run; use install.
run: npm install
Expand Down
12 changes: 9 additions & 3 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ jobs:
- os: ubuntu-latest
target: aarch64-unknown-linux-gnu
manylinux: "2_28"
- os: windows-latest
target: x86_64-pc-windows-msvc
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v6
Expand Down Expand Up @@ -158,7 +160,7 @@ jobs:
set -euo pipefail
echo "Dists:"; ls -1 dist/
missing=()
for target in aarch64-apple-darwin x86_64-apple-darwin x86_64-unknown-linux-gnu aarch64-unknown-linux-gnu; do
for target in aarch64-apple-darwin x86_64-apple-darwin x86_64-unknown-linux-gnu aarch64-unknown-linux-gnu x86_64-pc-windows-msvc; do
# Maturin encodes the target in the wheel filename's platform tag:
# aarch64-apple-darwin → macosx_*_arm64
# x86_64-apple-darwin → macosx_*_x86_64
Expand All @@ -169,6 +171,7 @@ jobs:
x86_64-apple-darwin) pattern="macosx_*_x86_64" ;;
x86_64-unknown-linux-gnu) pattern="manylinux_*_x86_64" ;;
aarch64-unknown-linux-gnu) pattern="manylinux_*_aarch64" ;;
x86_64-pc-windows-msvc) pattern="win_amd64" ;;
esac
if ! ls dist/*${pattern}*.whl >/dev/null 2>&1; then
missing+=("$target")
Expand Down Expand Up @@ -228,6 +231,8 @@ jobs:
target: x86_64-unknown-linux-gnu
- os: ubuntu-latest
target: aarch64-unknown-linux-gnu
- os: windows-latest
target: x86_64-pc-windows-msvc
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v6
Expand Down Expand Up @@ -353,11 +358,11 @@ jobs:
# changed the targets to update node-publish-platform's matrix to match.
run: |
set -euo pipefail
expected=5
expected=6

@agent-of-mkmeral agent-of-mkmeral Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟢 nice-to-have (no functional impact — the enforced expected=6 here and the error string are correct). Two doc-comment lines in this file are now stale after adding Windows and could be tidied:

  • Line ~354-355: static matrix of the 4 platform packages (+ the main package = 5 total) → should read 5 platform packages (6 total).
  • Line ~382: Publish the 4 per-platform packages → now 5 per-platform packages.

Purely prose; the actual matrices and the expected=6 guard already reflect the win32-x64-msvc addition. Anchored here because those comment lines are unchanged context and not directly annotatable.

@agent-of-mkmeral agent-of-mkmeral Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f26bd58 — updated all the stale prose counts (4→5 platform packages, 5→6 total) in release.yml. The enforced expected=6 guard and the matrices were already correct, so this was prose-only.

actual=$(ls dist-npm/*.tgz | wc -l | tr -d ' ')
echo "Packed tarballs ($actual):"; ls -1 dist-npm/*.tgz
if [ "$actual" -ne "$expected" ]; then
echo "::error::Expected $expected npm packages (1 main + 4 platform) but packed $actual. If napi.targets changed, update node-publish-platform's matrix to match."
echo "::error::Expected $expected npm packages (1 main + 5 platform) but packed $actual. If napi.targets changed, update node-publish-platform's matrix to match."
exit 1
fi

Expand Down Expand Up @@ -401,6 +406,7 @@ jobs:
- strands-agents-shell-darwin-arm64
- strands-agents-shell-linux-x64-gnu
- strands-agents-shell-linux-arm64-gnu
- strands-agents-shell-win32-x64-msvc
env:
V: ${{ needs.version.outputs.version }}
steps:
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"x86_64-apple-darwin",
"aarch64-apple-darwin",
"x86_64-unknown-linux-gnu",
"aarch64-unknown-linux-gnu"
"aarch64-unknown-linux-gnu",
"x86_64-pc-windows-msvc"
]
},
"engines": {
Expand Down
37 changes: 26 additions & 11 deletions src/vfs_kernel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,20 +121,35 @@ impl VfsKernel {
canon_base: &std::path::Path,
) -> io::Result<Fd> {
if flags.read && !flags.write {
// Open the file, then verify via /proc/self/fd that the
// opened fd still points within the bind mount. This
// eliminates the TOCTOU between canonicalize and read.
let file = std::fs::File::open(host_path)?;
use std::os::unix::io::AsRawFd;
let fd_path = format!("/proc/self/fd/{}", file.as_raw_fd());
if let Ok(real) = std::fs::read_link(&fd_path)
&& !real.starts_with(canon_base)

// Defense-in-depth TOCTOU check: after opening, verify via

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are big comments like these common across the project?

@agent-of-mkmeral agent-of-mkmeral Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

vfs_kernel.rs already leans verbose (≈130 comment lines) since AGENTS.md flags it as security-critical and asks contributors to preserve the reasoning behind each guard. This block explains why the TOCTOU re-check is Linux-only — which isn't obvious from the #[cfg(target_os = "linux")] alone and is exactly the kind of security rationale that file documents. That said, if you'd prefer it trimmed to a one-liner I'm glad to tighten it — your call as maintainer.

// /proc/self/fd that the opened fd still points within the bind
// mount. This closes the race between canonicalize() and open().
//
// This is Linux-only: /proc/self/fd is a Linux procfs interface
// with no portable equivalent (macOS has no /proc; Windows has no
// procfs at all). On other platforms the canonical-path check
// performed before this function is called is the primary security
// boundary, so we simply skip this extra verification there.
#[cfg(target_os = "linux")]
{
return Err(io::Error::new(
io::ErrorKind::PermissionDenied,
"access denied: path escaped bind mount",
));
use std::os::unix::io::AsRawFd;
let fd_path = format!("/proc/self/fd/{}", file.as_raw_fd());
if let Ok(real) = std::fs::read_link(&fd_path)
&& !real.starts_with(canon_base)
{
return Err(io::Error::new(
io::ErrorKind::PermissionDenied,
"access denied: path escaped bind mount",
));
}
}
// `canon_base` is only consumed by the Linux-only check above; on

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is self explanatory to a rust engineer.

@agent-of-mkmeral agent-of-mkmeral Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair — the let _ = canon_base; is self-explanatory to a Rust dev. I can drop the explanatory line above it if you prefer. Will fold this into the trim if you want the comments tightened.

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore that, meant to comment elsewhere.

// other targets it is intentionally unused.
#[cfg(not(target_os = "linux"))]
let _ = canon_base;

use std::io::Read;
let mut data = Vec::new();
let mut file = file;
Expand Down
6 changes: 6 additions & 0 deletions tests/shell_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4277,6 +4277,9 @@ fn builder_bind_direct_readonly_host() {
}));
}

// Uses std::os::unix::fs::symlink — symlink creation differs on Windows
// (requires elevated privileges), so this escape test is Unix-only.
#[cfg(unix)]

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@agent-of-mkmeral agent-of-mkmeral Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

#[test]
fn bind_direct_symlink_escape_blocked() {
let (rt, local) = rt();
Expand Down Expand Up @@ -7381,6 +7384,9 @@ expect!(cmd_sleep_zero, "command sleep 0 && echo ok", "ok");

// ── dangling symlink escape prevention ──────────────────────────────

// Uses std::os::unix::fs::symlink — symlink creation differs on Windows
// (requires elevated privileges), so this escape test is Unix-only.
#[cfg(unix)]

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above.

@agent-of-mkmeral agent-of-mkmeral Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@chaynabors chaynabors Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as before.

#[test]
fn bind_direct_dangling_symlink_blocked() {
let (rt, local) = rt();
Expand Down
Loading