Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 22 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -41,6 +47,7 @@ jobs:
fmt:
name: Check formatting
needs: check
timeout-minutes: 20
runs-on: ubuntu-latest

steps:
Expand All @@ -57,7 +64,9 @@ jobs:

test:
name: Run tests (${{ matrix.name }})
timeout-minutes: 45
strategy:
fail-fast: false
matrix:
include:
- name: ubuntu
Expand All @@ -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 }}

Expand Down Expand Up @@ -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
Expand All @@ -105,6 +128,7 @@ jobs:
xtask:
name: Run xtask checks
needs: check
timeout-minutes: 20
runs-on: ubuntu-latest

steps:
Expand Down
10 changes: 10 additions & 0 deletions md/design/common-issues.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
9 changes: 9 additions & 0 deletions md/design/running-tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
37 changes: 30 additions & 7 deletions src/hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> {
if dirs.is_empty() {
return None;
}
let existing = std::env::var_os("PATH").unwrap_or_default();
let mut entries: Vec<PathBuf> = 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,
Expand Down Expand Up @@ -905,6 +914,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_<name>
Expand Down
41 changes: 35 additions & 6 deletions src/predicate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,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
Expand Down Expand Up @@ -1162,6 +1172,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()
}

#[test]
fn evaluate_custom_predicate_pass() {
let (mut ctx, _scripts) = ctx_with_exit_codes(vec![("foo", 0)]);
Expand Down Expand Up @@ -1222,8 +1251,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();

Expand Down Expand Up @@ -1259,8 +1288,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();

Expand Down Expand Up @@ -1293,8 +1322,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();

Expand Down
5 changes: 3 additions & 2 deletions src/workspace_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
71 changes: 53 additions & 18 deletions symposium-testlib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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('\\', "/")
}
}

Expand All @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ depends-on = ["*"]

[[installations]]
name = "my-checker"
executable = "$TEST_DIR/cross-checker.sh"
script = "$TEST_DIR/cross-checker.sh"

[[predicate]]
name = "my_check"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Loading
Loading