Skip to content

Commit 2f145ba

Browse files
committed
fix(config): use tempfile for permission test uniqueness (macOS CI race)
Replace nanosecond-based `temp_store_path()` with `tempfile::TempDir` for all 17 permission test call sites. Root cause: the old helper used `SystemTime::now().as_nanos()` to generate unique temp paths, but on macOS CI runners (coarse-clock virtualized environments) two nextest processes can hit the same nanosecond, producing identical paths. This caused `save()` in one test to overwrite `save()` in another, or a concurrent test's cleanup to rmdir between `save()` and `load()`, leading to intermittent `save_and_load_roundtrip` failures like `left: 0, right: 2`. Fix: delegate uniqueness to the OS via `tempfile::Builder::tempdir()`. The new `temp_store()` returns `(TempDir, PathBuf)`; the caller binds `_tmp` to keep the directory alive, and RAII cleanup replaces the manual `remove_file` + `remove_dir` calls in the four I/O tests. Tests: 34 permission tests all pass on Windows locally; all 5 pre-commit CI gates (fmt / clippy / check workspace / --no-default-features / -F full) pass.
1 parent 80997c1 commit 2f145ba

1 file changed

Lines changed: 46 additions & 49 deletions

File tree

β€Žcrates/config/src/permissions.rsβ€Ž

Lines changed: 46 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -391,15 +391,20 @@ fn days_to_ymd(days: u64) -> (u64, u64, u64) {
391391
mod tests {
392392
use super::*;
393393

394-
fn temp_store_path() -> PathBuf {
395-
let dir = std::env::temp_dir().join(format!(
396-
"crab-perm-test-{}",
397-
std::time::SystemTime::now()
398-
.duration_since(std::time::UNIX_EPOCH)
399-
.unwrap_or_default()
400-
.as_nanos()
401-
));
402-
dir.join("permissions.json")
394+
/// Create a unique temporary directory + `permissions.json` path under it.
395+
///
396+
/// Returns `(TempDir, PathBuf)` β€” the caller MUST keep `TempDir` alive for
397+
/// the duration of the test, otherwise the directory is removed on drop.
398+
/// Uses `tempfile` (OS-level uniqueness) to avoid race conditions when
399+
/// multiple nextest processes hit the same nanosecond-based path on
400+
/// coarse-clock CI runners (observed on macOS).
401+
fn temp_store() -> (tempfile::TempDir, PathBuf) {
402+
let dir = tempfile::Builder::new()
403+
.prefix("crab-perm-test-")
404+
.tempdir()
405+
.expect("create tempdir");
406+
let path = dir.path().join("permissions.json");
407+
(dir, path)
403408
}
404409

405410
fn make_rule(pattern: &str, verdict: RuleVerdict, scope: RuleScope) -> PermissionRule {
@@ -500,7 +505,8 @@ mod tests {
500505

501506
#[test]
502507
fn add_rule_and_list() {
503-
let mut set = PermissionRuleSet::new(temp_store_path());
508+
let (_tmp, path) = temp_store();
509+
let mut set = PermissionRuleSet::new(path);
504510
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
505511
set.add_rule(make_rule("read", RuleVerdict::Allow, RuleScope::Permanent));
506512

@@ -511,7 +517,8 @@ mod tests {
511517

512518
#[test]
513519
fn add_rule_replaces_same_pattern_and_scope() {
514-
let mut set = PermissionRuleSet::new(temp_store_path());
520+
let (_tmp, path) = temp_store();
521+
let mut set = PermissionRuleSet::new(path);
515522
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
516523
set.add_rule(make_rule("bash", RuleVerdict::Deny, RuleScope::Session));
517524

@@ -521,7 +528,8 @@ mod tests {
521528

522529
#[test]
523530
fn add_rule_different_scopes_coexist() {
524-
let mut set = PermissionRuleSet::new(temp_store_path());
531+
let (_tmp, path) = temp_store();
532+
let mut set = PermissionRuleSet::new(path);
525533
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
526534
set.add_rule(make_rule("bash", RuleVerdict::Deny, RuleScope::Permanent));
527535

@@ -530,7 +538,8 @@ mod tests {
530538

531539
#[test]
532540
fn remove_rules_by_pattern() {
533-
let mut set = PermissionRuleSet::new(temp_store_path());
541+
let (_tmp, path) = temp_store();
542+
let mut set = PermissionRuleSet::new(path);
534543
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
535544
set.add_rule(make_rule("bash", RuleVerdict::Deny, RuleScope::Permanent));
536545
set.add_rule(make_rule("read", RuleVerdict::Allow, RuleScope::Session));
@@ -543,7 +552,8 @@ mod tests {
543552

544553
#[test]
545554
fn remove_rules_by_scope() {
546-
let mut set = PermissionRuleSet::new(temp_store_path());
555+
let (_tmp, path) = temp_store();
556+
let mut set = PermissionRuleSet::new(path);
547557
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
548558
set.add_rule(make_rule("bash", RuleVerdict::Deny, RuleScope::Permanent));
549559

@@ -555,13 +565,15 @@ mod tests {
555565

556566
#[test]
557567
fn remove_nonexistent_returns_zero() {
558-
let mut set = PermissionRuleSet::new(temp_store_path());
568+
let (_tmp, path) = temp_store();
569+
let mut set = PermissionRuleSet::new(path);
559570
assert_eq!(set.remove_rules("nonexistent"), 0);
560571
}
561572

562573
#[test]
563574
fn clear_session_rules() {
564-
let mut set = PermissionRuleSet::new(temp_store_path());
575+
let (_tmp, path) = temp_store();
576+
let mut set = PermissionRuleSet::new(path);
565577
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
566578
set.add_rule(make_rule("read", RuleVerdict::Allow, RuleScope::Permanent));
567579
set.add_rule(make_rule("write", RuleVerdict::Deny, RuleScope::Session));
@@ -575,7 +587,8 @@ mod tests {
575587

576588
#[test]
577589
fn check_exact_match() {
578-
let mut set = PermissionRuleSet::new(temp_store_path());
590+
let (_tmp, path) = temp_store();
591+
let mut set = PermissionRuleSet::new(path);
579592
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
580593

581594
let result = set.check("bash");
@@ -585,7 +598,8 @@ mod tests {
585598

586599
#[test]
587600
fn check_glob_match() {
588-
let mut set = PermissionRuleSet::new(temp_store_path());
601+
let (_tmp, path) = temp_store();
602+
let mut set = PermissionRuleSet::new(path);
589603
set.add_rule(make_rule("mcp__*", RuleVerdict::Deny, RuleScope::Permanent));
590604

591605
let result = set.check("mcp__playwright_click");
@@ -595,7 +609,8 @@ mod tests {
595609

596610
#[test]
597611
fn check_exact_takes_priority_over_glob() {
598-
let mut set = PermissionRuleSet::new(temp_store_path());
612+
let (_tmp, path) = temp_store();
613+
let mut set = PermissionRuleSet::new(path);
599614
set.add_rule(make_rule("mcp__*", RuleVerdict::Deny, RuleScope::Permanent));
600615
set.add_rule(make_rule(
601616
"mcp__safe_tool",
@@ -610,7 +625,8 @@ mod tests {
610625

611626
#[test]
612627
fn check_no_match_returns_none() {
613-
let mut set = PermissionRuleSet::new(temp_store_path());
628+
let (_tmp, path) = temp_store();
629+
let mut set = PermissionRuleSet::new(path);
614630
set.add_rule(make_rule("bash", RuleVerdict::Allow, RuleScope::Session));
615631

616632
assert!(set.check("write").is_none());
@@ -620,7 +636,8 @@ mod tests {
620636

621637
#[test]
622638
fn record_audit_entries() {
623-
let mut set = PermissionRuleSet::new(temp_store_path());
639+
let (_tmp, path) = temp_store();
640+
let mut set = PermissionRuleSet::new(path);
624641
set.record_audit("bash", RuleVerdict::Allow, AuditSource::Interactive, None);
625642
set.record_audit(
626643
"write",
@@ -638,7 +655,8 @@ mod tests {
638655

639656
#[test]
640657
fn audit_entry_has_timestamp() {
641-
let mut set = PermissionRuleSet::new(temp_store_path());
658+
let (_tmp, path) = temp_store();
659+
let mut set = PermissionRuleSet::new(path);
642660
set.record_audit("bash", RuleVerdict::Allow, AuditSource::Policy, None);
643661

644662
let entry = &set.audit_log()[0];
@@ -651,7 +669,7 @@ mod tests {
651669

652670
#[test]
653671
fn save_and_load_roundtrip() {
654-
let path = temp_store_path();
672+
let (_tmp, path) = temp_store();
655673
let mut set = PermissionRuleSet::new(path.clone());
656674

657675
// Add rules of both scopes
@@ -663,25 +681,19 @@ mod tests {
663681
set.save().unwrap();
664682

665683
// Load into a fresh rule set
666-
let mut set2 = PermissionRuleSet::new(path.clone());
684+
let mut set2 = PermissionRuleSet::new(path);
667685
set2.load().unwrap();
668686

669687
// Only permanent rules should be loaded
670688
assert_eq!(set2.list_rules().len(), 2);
671689
assert!(set2.list_session_rules().is_empty());
672690
assert_eq!(set2.list_permanent_rules().len(), 2);
673691
assert_eq!(set2.audit_log().len(), 1);
674-
675-
// Cleanup
676-
let _ = std::fs::remove_file(&path);
677-
if let Some(parent) = path.parent() {
678-
let _ = std::fs::remove_dir(parent);
679-
}
680692
}
681693

682694
#[test]
683695
fn load_preserves_session_rules() {
684-
let path = temp_store_path();
696+
let (_tmp, path) = temp_store();
685697

686698
// Save a permanent rule
687699
let store = PermissionStore {
@@ -691,19 +703,14 @@ mod tests {
691703
save_permission_store(&path, &store).unwrap();
692704

693705
// Create a rule set with a session rule, then load
694-
let mut set = PermissionRuleSet::new(path.clone());
706+
let mut set = PermissionRuleSet::new(path);
695707
set.add_rule(make_rule("write", RuleVerdict::Deny, RuleScope::Session));
696708
set.load().unwrap();
697709

698710
// Both should be present
699711
assert_eq!(set.list_rules().len(), 2);
700712
assert!(set.check("bash").is_some());
701713
assert!(set.check("write").is_some());
702-
703-
let _ = std::fs::remove_file(&path);
704-
if let Some(parent) = path.parent() {
705-
let _ = std::fs::remove_dir(parent);
706-
}
707714
}
708715

709716
#[test]
@@ -715,34 +722,24 @@ mod tests {
715722

716723
#[test]
717724
fn save_creates_parent_dirs() {
718-
let path = temp_store_path();
725+
let (_tmp, path) = temp_store();
719726
let set = PermissionRuleSet::new(path.clone());
720727
assert!(set.save().is_ok());
721728
assert!(path.exists());
722-
723-
let _ = std::fs::remove_file(&path);
724-
if let Some(parent) = path.parent() {
725-
let _ = std::fs::remove_dir(parent);
726-
}
727729
}
728730

729731
// ── File I/O helpers ──────────────────────────────────────────────
730732

731733
#[test]
732734
fn load_invalid_json_returns_error() {
733-
let path = temp_store_path();
735+
let (_tmp, path) = temp_store();
734736
if let Some(parent) = path.parent() {
735737
std::fs::create_dir_all(parent).unwrap();
736738
}
737739
std::fs::write(&path, "not json").unwrap();
738740

739741
let result = load_permission_store(&path);
740742
assert!(result.is_err());
741-
742-
let _ = std::fs::remove_file(&path);
743-
if let Some(parent) = path.parent() {
744-
let _ = std::fs::remove_dir(parent);
745-
}
746743
}
747744

748745
// ── Path helpers ──────────────────────────────────────────────────

0 commit comments

Comments
Β (0)