diff --git a/src/visibility.rs b/src/visibility.rs index 8a197da..c9b4bb2 100644 --- a/src/visibility.rs +++ b/src/visibility.rs @@ -88,6 +88,19 @@ fn matches_pattern(candidate: &str, pattern: &str) -> bool { } } +/// Whether `candidate` matches one declared private-branch `pattern`. +/// +/// The single per-pattern semantic for `private_branches`. Both callers go +/// through here, so adjudication and export cannot answer differently for +/// the same policy. +fn branch_matches(candidate: &str, pattern: &str) -> bool { + let pattern = pattern.trim().trim_start_matches('/'); + !pattern.is_empty() + && (matches_pattern(candidate, pattern) + || candidate == pattern + || candidate.contains(pattern)) +} + impl VisibilityPolicy { /// Load a visibility policy from a TOML configuration file. pub fn load(path: &Path) -> anyhow::Result { @@ -104,15 +117,13 @@ impl VisibilityPolicy { } /// Whether a branch name matches any declared private branch pattern. + /// + /// Export filtering and adjudication both go through here, so a branch + /// cannot be cloaked at adjudication and still published at export. pub fn branch_is_private(&self, branch: &str) -> bool { - self.private_branches.iter().any(|pb| { - let pb = pb.trim_start_matches('/'); - matches_pattern(branch, pb) - || branch == pb - || branch.starts_with(&format!("{pb}/")) - || branch.ends_with(&format!("/{pb}")) - || branch.contains(&format!("/{pb}/")) - }) + self.private_branches + .iter() + .any(|pb| branch_matches(branch, pb)) } /// Whether a touched path matches any private-path fragment. @@ -199,17 +210,15 @@ impl VisibilityPolicy { } } - // Check private branches + // Check private branches. Each pattern is tested against the change's + // name and refs by `branch_matches`, the same predicate export + // filtering uses, so one policy cannot mean two things. for branch in &self.private_branches { let branch_clean = branch.trim(); if !branch_clean.is_empty() { - let pb = branch_clean.trim_start_matches('/'); - let matched = matches_pattern(&change.name, pb) - || matches_pattern(&change.head_ref, pb) - || matches_pattern(&change.base_ref, pb) - || change.name.contains(pb) - || change.head_ref.contains(pb) - || change.base_ref.contains(pb); + let matched = [&change.name, &change.head_ref, &change.base_ref] + .iter() + .any(|field| branch_matches(field, branch_clean)); if matched { out.push(Dispute { id: format!("V{:03}", n), diff --git a/tests/export_visibility_test.rs b/tests/export_visibility_test.rs index 00d7900..fecdde4 100644 --- a/tests/export_visibility_test.rs +++ b/tests/export_visibility_test.rs @@ -243,6 +243,75 @@ fn test_policy_change_invalidates_export_cache() { let _ = std::fs::remove_dir_all(&tmp); } +/// A branch adjudication cloaks must not survive export. `private_branches` +/// had two independent matchers: export filtered on whole-segment equality, +/// adjudication also matched the bare substring. `private_branches = ["audit"]` +/// therefore cloaked a change on `feature/audit-fixes` and then published +/// that same branch. One predicate, one answer. +#[test] +fn test_cloaked_branch_is_not_published_by_export() { + let tmp = std::env::temp_dir().join(format!("oot-branch-{}-a", std::process::id())); + let _ = std::fs::remove_dir_all(&tmp); + let src = tmp.join("src"); + let proj = tmp.join("proj"); + let out = tmp.join("out"); + std::fs::create_dir_all(&proj).unwrap(); + + build_fixture(&src); + git(&src, &["checkout", "-b", "feature/audit-fixes"]); + std::fs::write(src.join("README.md"), "v2\n").unwrap(); + git(&src, &["add", "."]); + git(&src, &["commit", "-m", "audit work"]); + + std::fs::write( + proj.join("visibility.toml"), + "private_paths = []\nprivate_branches = [\"audit\"]\n", + ) + .unwrap(); + + assert!(oot(&["init"], &proj).0); + + // Adjudication cloaks it: the matcher must still see the pattern in a + // longer branch name, or this test asserts nothing. + let (cloaked, msg) = oot( + &[ + "adjudicate", + "--change", + "feature/audit-fixes", + "--base-ref", + "main", + "--head-ref", + "feature/audit-fixes", + "--repo", + src.to_str().unwrap(), + "--visibility", + proj.join("visibility.toml").to_str().unwrap(), + ], + &proj, + ); + assert!(!cloaked, "adjudication must still cloak the branch: {msg}"); + assert!( + msg.contains("private branch audit"), + "cloak must name the pattern: {msg}" + ); + + assert!(oot(&["import", "--repo", src.to_str().unwrap()], &proj).0); + let (ok, msg) = oot(&["export", "--out", out.to_str().unwrap()], &proj); + assert!(ok, "export failed: {msg}"); + + let refs = git(&out, &["for-each-ref", "--format=%(refname)", "refs/heads"]); + assert!( + !refs.contains("refs/heads/feature/audit-fixes"), + "a cloaked branch was published anyway: {refs}" + ); + assert!( + refs.contains("refs/heads/main"), + "the public branch must still export: {refs}" + ); + + let _ = std::fs::remove_dir_all(&tmp); +} + #[test] fn test_filtered_export_skips_empty_rebuilt_commits() { let tmp = std::env::temp_dir().join(format!("oot-empty-{}", std::process::id()));