@sigitcodereview / sigit / commits / f8ef46a

fix: serialize cwd-mutating tests on ENV_TEST_LOCK

The discovery tests in skills.rs, commands.rs and subagents.rs, plus the subagent end-to-end test in tools.rs, all change the process-wide current directory. cargo runs tests in parallel, so one test could move the cwd while another was mid-discovery. That is what broke CI on macOS x86_64: subagents::discover_and_resolve_roundtrip found 0 agent types because format_agent_types_list_reports_none_found had switched the process to its own empty temp dir at that moment. ENV_TEST_LOCK already serializes the env-var mutating tests, and the cwd is the same kind of process-global state (discovery reads SIGIT_CONFIG_DIR too), so the six cwd-mutating tests now take the same lock. The async test in tools.rs holds the guard across an await on purpose; each tokio::test runs its own single-threaded runtime, so nothing on that executor can contend for it.

paydii committed Jul 11, 2026 at 09:58 UTC f8ef46addc5f24813bb771871ee93e1d9eba04a9

5 files changed +32 -3

src/commands.rs

@@ -342,6 +342,9 @@ mod tests {

342

343 #[test]

344 fn discover_and_resolve_roundtrip() {

345 + let _guard = crate::ENV_TEST_LOCK

346 + .lock()

347 + .unwrap_or_else(|poisoned| poisoned.into_inner());

348 let root = unique_dir("roundtrip");

349 let commands_root = root.join(".sigit").join("commands");

350 fs::create_dir_all(commands_root.join("git")).unwrap();

@@ -379,6 +382,9 @@ mod tests {

382

383 #[test]

384 fn format_commands_list_reports_none_found() {

385 + let _guard = crate::ENV_TEST_LOCK

386 + .lock()

387 + .unwrap_or_else(|poisoned| poisoned.into_inner());

388 let root = unique_dir("empty");

389 fs::create_dir_all(&root).unwrap();

390 let prev = std::env::current_dir().unwrap();

src/main.rs

+5 -3

@@ -48,9 +48,11 @@ mod skills;

48 mod subagents;

49 mod tools;

51 -/// Serializes tests that mutate process-global env vars (`SIGIT_CONFIG_DIR`

52 -/// etc.). `cargo test` runs tests in parallel within a binary, so without this

53 -/// the credentials and settings round-trip tests clobber each other's env.

51 +/// Serializes tests that mutate process-global state: env vars

52 +/// (`SIGIT_CONFIG_DIR` etc.) and the current directory. `cargo test` runs

53 +/// tests in parallel within a binary, so without this the credentials and

54 +/// settings round-trip tests clobber each other's env, and the discovery

55 +/// tests (skills/commands/subagents) yank the cwd out from under each other.

56 #[cfg(test)]

57 pub(crate) static ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

src/skills.rs

@@ -414,6 +414,9 @@ mod tests {

414

415 #[test]

416 fn discover_and_activate_roundtrip() {

417 + let _guard = crate::ENV_TEST_LOCK

418 + .lock()

419 + .unwrap_or_else(|poisoned| poisoned.into_inner());

420 let root = unique_dir("roundtrip");

421 let skills_root = root.join(".sigit").join("skills");

422 let skill_dir = skills_root.join("hello-world");

src/subagents.rs

@@ -282,6 +282,9 @@ mod tests {

282

283 #[test]

284 fn discover_and_resolve_roundtrip() {

285 + let _guard = crate::ENV_TEST_LOCK

286 + .lock()

287 + .unwrap_or_else(|poisoned| poisoned.into_inner());

288 let root = unique_dir("roundtrip");

289 let agents_root = root.join(".sigit").join("agents");

290 fs::create_dir_all(&agents_root).unwrap();

@@ -308,6 +311,9 @@ mod tests {

311

312 #[test]

313 fn format_agent_types_list_reports_none_found() {

314 + let _guard = crate::ENV_TEST_LOCK

315 + .lock()

316 + .unwrap_or_else(|poisoned| poisoned.into_inner());

317 let root = unique_dir("empty");

318 fs::create_dir_all(&root).unwrap();

319 let prev = std::env::current_dir().unwrap();

src/tools.rs

+12

@@ -3602,6 +3602,7 @@ mod tests {

3602 }

3603

3604 #[tokio::test]

3605 + #[allow(clippy::await_holding_lock)] // see the ENV_TEST_LOCK comment inside

3606 async fn test_task_runs_subagent_end_to_end() {

3607 // A file only the subagent's read_file call can surface.

3608 let dir = std::env::temp_dir().join("sigit_test_subagent_e2e");

@@ -3711,6 +3712,17 @@ mod tests {

3712 )

3713 .unwrap();

3714

3715 + // Serialize the cwd change against the other discovery tests

3716 + // (skills/commands/subagents) — the cwd is process-global and cargo

3717 + // runs tests in parallel. The guard is held across the `.await`

3718 + // below on purpose: the cwd must stay pinned while `execute_tool`

3719 + // runs agent-type discovery. That's safe here — each `#[tokio::test]`

3720 + // gets its own single-threaded runtime on its own test thread, so no

3721 + // other task on this executor can contend for the lock — hence the

3722 + // `await_holding_lock` allow on this test.

3723 + let _env_guard = crate::ENV_TEST_LOCK

3724 + .lock()

3725 + .unwrap_or_else(|poisoned| poisoned.into_inner());

3726 let prev_cwd = std::env::current_dir().unwrap();

3727 std::env::set_current_dir(&dir).unwrap();

3728