From 5545281f08cd3a4e29a8b6d7e300982c4ff9a174 Mon Sep 17 00:00:00 2001 From: Awesome Rustacean Date: Sun, 26 Jul 2026 00:26:03 +0300 Subject: [PATCH] ci: run test suite on Windows - add windows-latest to test matrix (fail-fast off); put git `sh` on PATH; concurrency+cancel; job timeouts; `shell: bash` on agent-disable - .gitattributes: LF-normalize text, mark binaries, rust/toml diff hints - portability: `sh_path` drive rewrite (predicate.rs); canonicalize both sides (workspace_state test); forward-slash paths in fixture TOML/JSON (testlib); `.sh` fixtures use `script` not `executable` - hook.rs: build PATH via `join_paths` (was hardcoded `:`, wrong on Windows) +test - predicate.rs: warn on missing `sh` instead of silent false - testlib: cross-platform mock cargo via `.cmd`->`sh` shim; normalize_paths matches home-abbreviated (`~/`) config path so snapshots stay stable when temp lives under $HOME (Windows) - un-gate 6 self-update tests; 2 re_exec tests `#[ignore]`d on Windows (compile everywhere, helpers need no cfg) - docs: running-tests, common-issues windows notes Co-authored-by: Claude --- .gitattributes | 22 ++++++ .github/workflows/ci.yml | 24 +++++++ md/design/common-issues.md | 10 +++ md/design/running-tests.md | 9 +++ src/hook.rs | 37 ++++++++-- src/predicate.rs | 41 +++++++++-- src/workspace_state.rs | 5 +- symposium-testlib/src/lib.rs | 71 ++++++++++++++----- .../plugins/provider-plugin/SYMPOSIUM.toml | 2 +- .../plugins/bp-plugin/SYMPOSIUM.toml | 2 +- .../plugins/bp-plugin/SYMPOSIUM.toml | 2 +- tests/init_sync.rs | 17 +++++ 12 files changed, 206 insertions(+), 36 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..a3aac208 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,22 @@ +# Normalize line endings: store LF in the repo, check out LF on every OS. +# Keeps shell scripts and shebang'd fixtures runnable on the Windows CI runner. +* text=auto eol=lf + +# Force text + LF on source files, with language-aware diff hunk headers. +*.rs text eol=lf diff=rust +*.toml text eol=lf diff=toml +Cargo.lock text eol=lf + +# Binary assets: never touch their bytes. +*.png binary +*.jpg binary +*.jpeg binary +*.gif binary +*.ico binary +*.ai binary +*.eps binary +*.pdf binary +*.woff binary +*.woff2 binary +*.ttf binary +*.otf binary diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9248f2f..8f12696c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,9 +9,15 @@ on: env: CARGO_TERM_COLOR: always +# Cancel superseded runs on the same ref (e.g. rapid pushes to a PR branch). +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: check: name: Check compilation + timeout-minutes: 20 strategy: matrix: os: [ubuntu-latest, macos-latest] @@ -41,6 +47,7 @@ jobs: fmt: name: Check formatting needs: check + timeout-minutes: 20 runs-on: ubuntu-latest steps: @@ -57,7 +64,9 @@ jobs: test: name: Run tests (${{ matrix.name }}) + timeout-minutes: 45 strategy: + fail-fast: false matrix: include: - name: ubuntu @@ -67,6 +76,8 @@ jobs: - name: musl os: ubuntu-latest target: x86_64-unknown-linux-musl + - name: windows + os: windows-latest runs-on: ${{ matrix.os }} @@ -96,7 +107,19 @@ jobs: restore-keys: | ${{ matrix.name }}-cargo- + # Tests spawn `sh` to run script-based hooks/predicates. Git for Windows + # ships sh.exe in usr/bin, which is not on the runner's PATH by default. + - name: Put sh on PATH (Windows) + if: runner.os == 'Windows' + shell: bash + run: echo 'C:\Program Files\Git\usr\bin' >> "$GITHUB_PATH" + + - name: Verify sh is available (Windows) + if: runner.os == 'Windows' + run: sh --version + - name: Disable agent tests + shell: bash run: echo 'test-agents = []' > test-agents.toml - name: Test @@ -105,6 +128,7 @@ jobs: xtask: name: Run xtask checks needs: check + timeout-minutes: 20 runs-on: ubuntu-latest steps: diff --git a/md/design/common-issues.md b/md/design/common-issues.md index 285b8f12..b3b56dbb 100644 --- a/md/design/common-issues.md +++ b/md/design/common-issues.md @@ -15,3 +15,13 @@ Copilot sends `toolArgs` as a JSON *string* (not an object). Our `CopilotPreTool ### Gemini `SessionStart` matcher `ensure_gemini_hook_entry` uses `"matcher": ".*"` for all events including `SessionStart`. Per the Gemini reference, lifecycle events use exact-string matchers, not regex. Likely harmless in practice since `".*"` matches anything. + +## Windows portability (tests) + +The test suite runs on `windows-latest`. A few patterns recur when writing tests that touch paths or scripts: + +- **Paths in TOML/JSON string literals.** A Windows path like `C:\Users\...` is invalid inside a TOML or JSON string (the backslashes read as escapes). When substituting a real path into fixture text, convert to forward slashes first; Windows accepts `/` in paths. See `setup_fixture` in `symposium-testlib`. +- **Paths inside `sh` script bodies.** On Windows `sh` is git-bash's MSYS shell, which reads `C:\a\b` as escapes plus an illegal `:`. Rewrite to the `/c/a/b` form and quote the value. See `sh_path` in `predicate.rs` tests. +- **`.sh` files must use `script`, not `executable`.** A shell script cannot be spawned directly as a process on Windows (no shebang support). In fixtures, reference it via `script = "..."` so it is run through `sh`, never `executable = "..."`. +- **Canonicalized paths carry a `\\?\` prefix.** `fs::canonicalize` on Windows returns an extended-length path that `cargo`'s output lacks. Canonicalize both sides before comparing. +- **Snapshot tests and home-abbreviated paths.** `display_path` (in `output.rs`) abbreviates `$HOME` to `~/`. On Windows the test temp dir lives under `$HOME`, so printed config paths come out home-relative, not absolute. `normalize_paths` (in `symposium-testlib`) replaces both the absolute and the `~/` form; a snapshot leaking a random `.tmpXXXX/` path means one form was missed. Do not `UPDATE_EXPECT` your way past it: that bakes the volatile temp path into the snapshot and it fails on the next run. diff --git a/md/design/running-tests.md b/md/design/running-tests.md index f8ef80fd..58ddbffb 100644 --- a/md/design/running-tests.md +++ b/md/design/running-tests.md @@ -58,3 +58,12 @@ cargo test --test hook_agent -- --nocapture ``` On failure, the test's temporary directory is preserved and its path is printed to stderr so you can inspect the fixture state. + +## Windows + +CI runs the full test suite on `windows-latest` as part of the `test` matrix (see `.github/workflows/ci.yml`). To run the tests locally on Windows: + +- Install Git for Windows and make sure `sh` is on `PATH`. Git ships it at `C:\Program Files\Git\usr\bin`. Several tests spawn `sh` to run script-based hooks and predicates, so a missing `sh` shows up as unrelated-looking hook failures. +- The repo's `.gitattributes` normalizes checked-out text files to LF. This keeps shebang'd fixtures and shell scripts runnable regardless of `core.autocrlf`. + +The self-update tests run on Windows: `set_mock_cargo` runs the `#!/bin/sh` mock through `sh` via a one-line `.cmd` shim (production spawns the cargo override directly, so no production code changes). The two `auto_update_re_execs_*` tests are `#[ignore]`d on Windows (`#[cfg_attr(windows, ignore)]`): they overwrite the running binary with a shebang stand-in and re-exec into it, which needs Windows-native process replacement. They still compile on Windows, so they are skipped (not compiled out) and their helpers need no `#[cfg]`. Porting them to run on Windows is a tracked follow-up. diff --git a/src/hook.rs b/src/hook.rs index be70dc72..152ff09c 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -112,19 +112,28 @@ fn build_env(acquired: &[AcquiredInstallation]) -> Vec<(String, String)> { // PATH lookup over requirements' bin dirs. path_prefix.reverse(); - if !path_prefix.is_empty() { - let existing = std::env::var("PATH").unwrap_or_default(); - let joined = if existing.is_empty() { - path_prefix.join(":") - } else { - format!("{}:{}", path_prefix.join(":"), existing) - }; + if let Some(joined) = prepend_to_path(&path_prefix) { env.push(("PATH".to_string(), joined)); } env } +/// Prepend `dirs` to the current `PATH`, using the platform path-list +/// separator (`:` on Unix, `;` on Windows). Returns `None` when `dirs` is +/// empty. `join_paths` picks the separator, so this stays correct on Windows. +fn prepend_to_path(dirs: &[String]) -> Option { + if dirs.is_empty() { + return None; + } + let existing = std::env::var_os("PATH").unwrap_or_default(); + let mut entries: Vec = dirs.iter().map(PathBuf::from).collect(); + entries.extend(std::env::split_paths(&existing).filter(|dir| !dir.as_os_str().is_empty())); + std::env::join_paths(entries) + .ok() + .map(|joined| joined.to_string_lossy().into_owned()) +} + enum SpawnSpec { Exec { path: PathBuf, @@ -906,6 +915,20 @@ mod tests { assert!(path.contains("/cache/rtk/1.0/bin"), "PATH = {path}"); } + #[test] + fn prepend_to_path_uses_platform_separator() { + assert!(prepend_to_path(&[]).is_none(), "empty input yields None"); + + let dirs = vec!["/first".to_string(), "/second".to_string()]; + let joined = prepend_to_path(&dirs).expect("non-empty dirs yield Some"); + + let sep = if cfg!(windows) { ';' } else { ':' }; + assert!( + joined.starts_with(&format!("/first{sep}/second")), + "prepended dirs come first, joined by the platform separator: {joined}" + ); + } + #[test] fn build_env_no_runnable_no_vars() { // Pure-setup installation: no runnable means no SYMPOSIUM_ diff --git a/src/predicate.rs b/src/predicate.rs index 8910ceec..ea6acd13 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -708,6 +708,16 @@ fn run_shell(command: &str) -> bool { ); false } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + // A missing `sh` makes every shell(...) predicate silently false, + // which is confusing to debug. Surface it once at warn level. + tracing::warn!( + command = %command, + "shell predicate evaluated false because `sh` was not found on PATH; \ + add a POSIX shell to PATH to enable shell(...) predicates", + ); + false + } Err(e) => { tracing::trace!(command = %command, error = %e, "shell predicate failed to spawn"); false @@ -1372,6 +1382,25 @@ mod tests { (PredicateContext::with_custom_predicates(&[], map), scripts) } + /// Render `path` for use *inside* a `/bin/sh` script body. On Windows `sh` + /// is git-bash's MSYS shell, which reads `C:\a\b` as escapes; rewrite it to + /// the `/c/a/b` form the shell understands. + #[cfg(windows)] + fn sh_path(path: &Path) -> String { + let slashed = path.to_string_lossy().replace('\\', "/"); + match slashed.split_once(':') { + Some((drive, rest)) if drive.len() == 1 => { + format!("/{}{}", drive.to_ascii_lowercase(), rest) + } + _ => slashed, + } + } + + #[cfg(not(windows))] + fn sh_path(path: &Path) -> String { + path.to_string_lossy().to_string() + } + fn ctx_with_script_entry( name: &str, script_content: &str, @@ -1453,8 +1482,8 @@ mod tests { let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); writeln!( script.as_file(), - "#!/bin/sh\necho x >> {}\nexit 0", - counter_path.display() + "#!/bin/sh\necho x >> \"{}\"\nexit 0", + sh_path(&counter_path) ) .unwrap(); @@ -1490,8 +1519,8 @@ mod tests { let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); writeln!( script.as_file(), - "#!/bin/sh\necho \"$@\" > {}", - output_path.display() + "#!/bin/sh\necho \"$@\" > \"{}\"", + sh_path(&output_path) ) .unwrap(); @@ -1524,8 +1553,8 @@ mod tests { let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); writeln!( script.as_file(), - "#!/bin/sh\necho \"$@\" > {}", - output_path.display() + "#!/bin/sh\necho \"$@\" > \"{}\"", + sh_path(&output_path) ) .unwrap(); diff --git a/src/workspace_state.rs b/src/workspace_state.rs index 70ea3c6e..ffc02e4b 100644 --- a/src/workspace_state.rs +++ b/src/workspace_state.rs @@ -274,8 +274,9 @@ mod tests { fs::write(src.join("lib.rs"), "").unwrap(); let sym = Symposium::from_dir(tmp.path()); - let found = find_workspace_root(&sym, &src); - // Canonicalize expected path to handle macOS /var → /private/var symlink. + // Canonicalize both sides: macOS resolves /var -> /private/var, and on Windows + // canonicalization adds a `\\?\` prefix cargo's output lacks. + let found = find_workspace_root(&sym, &src).map(|pth| fs::canonicalize(pth).unwrap()); let expected = fs::canonicalize(&root).unwrap(); assert_eq!(found, Some(expected)); } diff --git a/symposium-testlib/src/lib.rs b/symposium-testlib/src/lib.rs index 0cd4ef7a..536a1bd7 100644 --- a/symposium-testlib/src/lib.rs +++ b/symposium-testlib/src/lib.rs @@ -468,28 +468,55 @@ impl TestContext { Ok(SubmitResult { hooks, response }) } - /// Point `cargo` invocations at a mock script for the duration of this test. - /// - /// Writes the given shell script into the tempdir and configures `Symposium` - /// to use it instead of the real `cargo`. No environment variables are + /// Point `cargo` invocations at a mock `sh` script for the duration of this + /// test. `script` is a `#!/bin/sh` body. No environment variables are /// mutated, so tests can run in parallel without interference. + /// + /// Windows can't exec a shebang script directly, so the body is run through + /// git-bash's `sh` via a one-line `.cmd` shim. Rust spawns `.cmd` files + /// through the command interpreter, and production spawns the cargo + /// override directly, so this needs no production change. It does require + /// `sh` on PATH (the documented Windows dev/CI requirement). pub fn set_mock_cargo(&mut self, script: &str) { - let script_path = self.tempdir.join("mock-cargo"); - std::fs::write(&script_path, script).unwrap(); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o755)).unwrap(); - } - self.sym.set_cargo_override(script_path); + let cargo_override = self.write_mock_cargo(script); + self.sym.set_cargo_override(cargo_override); + } + + /// Write the mock cargo as a directly-spawnable program; return its path. + #[cfg(not(windows))] + fn write_mock_cargo(&self, script: &str) -> PathBuf { + use std::os::unix::fs::PermissionsExt; + let path = self.tempdir.join("mock-cargo"); + std::fs::write(&path, script).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + path + } + + /// Windows can't exec a shebang script, so run it through `sh` via a + /// one-line `.cmd` shim (Rust spawns `.cmd` through the command interpreter). + #[cfg(windows)] + fn write_mock_cargo(&self, script: &str) -> PathBuf { + let sh_script = self.tempdir.join("mock-cargo.sh"); + std::fs::write(&sh_script, script).unwrap(); + let sh_script_fwd = sh_script.to_string_lossy().replace('\\', "/"); + let cmd_shim = self.tempdir.join("mock-cargo.cmd"); + std::fs::write(&cmd_shim, format!("@sh \"{sh_script_fwd}\" %*\r\n")).unwrap(); + cmd_shim } /// Replace variable content with stable placeholders for snapshot tests. + /// Backslashes are folded to `/` so path snapshots match on Windows. pub fn normalize_paths(&self, output: &str) -> String { - let config_dir = self.sym.config_dir().to_string_lossy().to_string(); + let config_dir = self.sym.config_dir(); + // `display_path` abbreviates $HOME to `~`, so when the test temp dir + // lives under $HOME (always the case on Windows) the printed config + // path is home-relative, not absolute. Replace both forms. + let abbreviated = symposium::output::display_path(config_dir); output - .replace(&config_dir, "$CONFIG_DIR") + .replace(&abbreviated, "$CONFIG_DIR") + .replace(&*config_dir.to_string_lossy(), "$CONFIG_DIR") .replace(symposium::state::CURRENT_VERSION, "$VERSION") + .replace('\\', "/") } } @@ -509,10 +536,18 @@ async fn setup_fixture(fixtures: &[&str]) -> TestContext { let tempdir = tempfile::tempdir().expect("failed to create tempdir"); let root = tempdir.path(); - let test_dir = root.to_str().expect("tempdir path is UTF-8"); - let binary = std::env::var("CARGO_BIN_EXE_cargo-agents").unwrap_or_default(); - - let vars = [("$TEST_DIR", test_dir), ("$BINARY", &binary)]; + // Forward slashes: these paths are substituted into TOML/JSON string + // literals, where a Windows `C:\Users\...` reads as invalid escapes and + // fails to parse. Windows accepts `/` in paths, so this is portable. + let test_dir = root + .to_str() + .expect("tempdir path is UTF-8") + .replace('\\', "/"); + let binary = std::env::var("CARGO_BIN_EXE_cargo-agents") + .unwrap_or_default() + .replace('\\', "/"); + + let vars = [("$TEST_DIR", test_dir.as_str()), ("$BINARY", &binary)]; let mut scan = FixtureScanResult { config_dirs: Vec::new(), diff --git a/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml index a17acc7e..0f68babc 100644 --- a/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml +++ b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml @@ -3,7 +3,7 @@ crates = ["*"] [[installations]] name = "my-checker" -executable = "$TEST_DIR/cross-checker.sh" +script = "$TEST_DIR/cross-checker.sh" [[predicate]] name = "my_check" diff --git a/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml index df386b49..2799029b 100644 --- a/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml +++ b/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml @@ -5,7 +5,7 @@ predicates = ["battery_pack(cli)"] name = "bp-checker" # Script generated at runtime by the test — returns witness JSON # naming bp-crate so that source = "crate" resolution picks it up. -executable = "$TEST_DIR/bp-checker.sh" +script = "$TEST_DIR/bp-checker.sh" [[predicate]] name = "battery_pack" diff --git a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml index 8b337317..72b011f8 100644 --- a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml +++ b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml @@ -5,7 +5,7 @@ predicates = ["battery_pack(cli)"] name = "bp-checker" # Script generated at runtime by the test — allows tests to vary # behavior (exit 0 vs exit 1, witness JSON, etc). -executable = "$TEST_DIR/bp-checker.sh" +script = "$TEST_DIR/bp-checker.sh" [[predicate]] name = "battery_pack" diff --git a/tests/init_sync.rs b/tests/init_sync.rs index 6ca6b026..4acdbbe4 100644 --- a/tests/init_sync.rs +++ b/tests/init_sync.rs @@ -1349,6 +1349,15 @@ async fn agents_syncing_detects_modified_source_skill() { // --------------------------------------------------------------------------- // Self-update / state integration tests // --------------------------------------------------------------------------- +// +// The "query-only" tests below run on all platforms: `set_mock_cargo` runs the +// `#!/bin/sh` mock through `sh` (via a `.cmd` shim on Windows), so self-update +// version detection and throttling are covered everywhere. The two +// `auto_update_re_execs_*` tests are `#[ignore]`d on Windows: they overwrite the +// running binary with a shebang stand-in and re-exec into it, which needs +// Windows-native process replacement. They still compile on Windows (so their +// helpers below need no `#[cfg]` and stay live), just skip at runtime. Porting +// them to actually run on Windows is a tracked follow-up. fn mock_cargo_script(search_version: &str) -> String { format!( @@ -1662,6 +1671,10 @@ fn assert_surprise(output: &std::process::Output) { } #[tokio::test] +#[cfg_attr( + windows, + ignore = "re_exec over a running binary + shebang mock/stand-in need a Windows port (follow-up)" +)] async fn auto_update_re_execs_on_sync() { let fix = setup_auto_update_fixture(); let output = fix @@ -1673,6 +1686,10 @@ async fn auto_update_re_execs_on_sync() { } #[tokio::test] +#[cfg_attr( + windows, + ignore = "re_exec over a running binary + shebang mock/stand-in need a Windows port (follow-up)" +)] async fn auto_update_re_execs_on_hook() { let fix = setup_auto_update_fixture(); let output = fix