From f080db2f7b5b826e7c2ea665e307a3d8ee8a0206 Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 7 May 2026 13:37:36 +0530 Subject: [PATCH 1/4] refactor(policy): use most-specific-pattern-wins evaluation model --- crates/forge_domain/src/policies/config.rs | 20 +- crates/forge_domain/src/policies/engine.rs | 274 +++++++++++++++++---- crates/forge_domain/src/policies/policy.rs | 15 ++ crates/forge_domain/src/policies/rule.rs | 30 +++ crates/forge_domain/src/policies/types.rs | 13 + 5 files changed, 293 insertions(+), 59 deletions(-) diff --git a/crates/forge_domain/src/policies/config.rs b/crates/forge_domain/src/policies/config.rs index 694ecf5140..f5cfb22133 100644 --- a/crates/forge_domain/src/policies/config.rs +++ b/crates/forge_domain/src/policies/config.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeSet; use std::fmt::{Display, Formatter}; use schemars::JsonSchema; @@ -10,22 +9,29 @@ use super::types::Permission; use crate::Rule; /// Collection of policies +/// +/// Policies are stored as an ordered list to preserve the order in which they +/// appear in `permissions.yaml`. The [`PolicyEngine`] evaluates them in that +/// order using a *first-matching-policy-wins* model, so the user's declared +/// order is the priority order. +/// +/// [`PolicyEngine`]: crate::PolicyEngine #[derive(Default, Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct PolicyConfig { - /// Set of policies to evaluate - #[serde(default, skip_serializing_if = "BTreeSet::is_empty")] - pub policies: BTreeSet, + /// Ordered list of policies to evaluate + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub policies: Vec, } impl PolicyConfig { /// Create a new empty policies collection pub fn new() -> Self { - Self { policies: BTreeSet::new() } + Self { policies: Vec::new() } } - /// Add a policy to the collection + /// Append a policy to the collection, preserving declaration order pub fn add_policy(mut self, policy: Policy) -> Self { - self.policies.insert(policy); + self.policies.push(policy); self } diff --git a/crates/forge_domain/src/policies/engine.rs b/crates/forge_domain/src/policies/engine.rs index b89747a906..952cb09c66 100644 --- a/crates/forge_domain/src/policies/engine.rs +++ b/crates/forge_domain/src/policies/engine.rs @@ -25,64 +25,42 @@ impl<'a> PolicyEngine<'a> { self.evaluate_policies(operation) } - /// Internal helper function to evaluate policies for a given operation - /// Returns permission result, defaults to Confirm if no policies match + /// Internal helper function to evaluate policies for a given operation. + /// Returns the permission of the first policy whose rule matches the + /// operation, or [`Permission::Confirm`] if no policy matches. fn evaluate_policies(&self, operation: &PermissionOperation) -> Permission { - let has_policies = !self.policies.policies.is_empty(); - - if !has_policies { - return Permission::Confirm; - } - - let mut last_allow: Option = None; - - // Evaluate all policies in order: workflow policies first, then extended - // policies - - if let Some(permission) = self.evaluate_policy_set(self.policies.policies.iter(), operation) - { - match permission { - Permission::Deny | Permission::Confirm => { - // Return immediately for denials or confirmations - return permission; - } - Permission::Allow => { - // Keep track of the last allow - last_allow = Some(permission); - } - } - } - - // Return last allow if found, otherwise default to Confirm - last_allow.unwrap_or(Permission::Confirm) - } - - /// Helper function to evaluate a set of policies - /// Returns the first non-Allow result, or the last Allow result if all are - /// Allow + self.evaluate_policy_set(self.policies.policies.iter(), operation) + .unwrap_or(Permission::Confirm) + } + + /// Helper function to evaluate a set of policies using a + /// *most-specific-pattern-wins* model. + /// + /// Among all policies whose rule matches the operation, the one with + /// the highest [`Policy::specificity`] takes effect. Ties are broken by + /// preferring the more restrictive permission (`Deny > Confirm > Allow`), + /// which keeps the engine safe-by-default when two equally-specific + /// rules disagree. + /// + /// Declaration order in `permissions.yaml` does not affect the outcome, + /// so users can express both "allow specific, deny everything else" and + /// "deny everything, allow specific" without worrying about ordering. + /// + /// Returns `None` if no policy matches. fn evaluate_policy_set<'p, I: IntoIterator>( &self, policies: I, operation: &PermissionOperation, ) -> Option { - let mut last_allow: Option = None; - - for policy in policies { - if let Some(permission) = policy.eval(operation) { - match permission { - Permission::Deny | Permission::Confirm => { - // Return immediately for denials or confirmations - return Some(permission); - } - Permission::Allow => { - // Keep track of the last allow - last_allow = Some(permission); - } - } - } - } - - last_allow + policies + .into_iter() + .filter_map(|policy| { + policy + .eval(operation) + .map(|permission| (permission, policy.specificity())) + }) + .max_by_key(|(permission, specificity)| (*specificity, permission.restrictiveness())) + .map(|(permission, _)| permission) } } @@ -187,6 +165,198 @@ mod tests { assert_eq!(actual, Permission::Allow); } + /// Regression test for https://github.com/tailcallhq/forgecode/issues/3085 + /// + /// The documented `permissions.yaml` example claims to allow writes to + /// one kind of file (`**/*.rs`) while denying writes to everything else + /// (`**/*`). The original engine always short-circuited on the first + /// matching `Deny`, so the broad `deny write` rule silently overrode + /// the more-specific `allow write`. + /// + /// Under the *most-specific-pattern-wins* model, `**/*.rs` (3 literal + /// chars) is more specific than `**/*` (0 literals), so writes to `.rs` + /// files are correctly allowed regardless of declaration order. + #[test] + fn test_policy_engine_specific_allow_should_win_over_broad_deny() { + // policies: + // - permission: allow + // rule: + // read: "**/*" + // - permission: allow + // rule: + // write: "**/*.rs" // specificity=3 (l: "", "/*/", ".rs") + // - permission: deny + // rule: + // write: "**/*" // specificity=0 (all wildcards) + // + // operation: write "test.rs" + // → matches allow "**/*.rs" (spec=3) + // → matches deny "**/*" (spec=0) + // → 3 > 0 → allow wins + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Read(ReadRule { read: "**/*".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "**/*.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "**/*".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let operation = PermissionOperation::Write { + path: std::path::PathBuf::from("test.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.rs".to_string(), + }; + + let actual = fixture.can_perform(&operation); + let expected = Permission::Allow; + + assert_eq!(actual, expected); + } + + /// Order-independence companion: same allow/deny pair as the issue + /// scenario but with the broad deny declared *first*. Under + /// most-specific-wins the result must be identical — declaration order + /// must not change the outcome. + #[test] + fn test_policy_engine_specific_allow_wins_regardless_of_order() { + // policies: + // - permission: deny + // rule: + // write: "**/*" // specificity=0 + // - permission: allow + // rule: + // write: "**/*.rs" // specificity=3 + // + // operation: write "test.rs" + // → matches deny "**/*" (spec=0) + // → matches allow "**/*.rs" (spec=3) + // → 3 > 0 → allow wins (same result as issue scenario) + // + // operation: write "test.py" + // → matches deny "**/*" only (spec=0) + // → only rule is deny → deny wins + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "**/*".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "**/*.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let rs_op = PermissionOperation::Write { + path: std::path::PathBuf::from("test.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.rs".to_string(), + }; + let py_op = PermissionOperation::Write { + path: std::path::PathBuf::from("test.py"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.py".to_string(), + }; + + // .rs matches both rules; the more-specific allow wins. + assert_eq!(fixture.can_perform(&rs_op), Permission::Allow); + // .py only matches the broad deny, so it stays denied. + assert_eq!(fixture.can_perform(&py_op), Permission::Deny); + } + + /// Carve-out exception: a broad allow with a narrow deny exception. + /// Under most-specific-wins the deny on the literal filename + /// (`secret.rs`, all literals) outranks the broader allow on + /// `**/*.rs`, so writes to `secret.rs` are blocked while other `.rs` + /// files remain writable. + #[test] + fn test_policy_engine_specific_deny_carves_out_exception_in_broad_allow() { + // policies: + // - permission: allow + // rule: + // write: "**/*.rs" // specificity=3 + // - permission: deny + // rule: + // write: "secret.rs" // specificity=9 (all literals) + // + // operation: write "secret.rs" + // → matches allow "**/*.rs" (spec=3) + // → matches deny "secret.rs" (spec=9) + // → 9 > 3 → deny wins (carve-out respected) + // + // operation: write "main.rs" + // → matches allow "**/*.rs" (spec=3) + // → no deny match + // → allow wins + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "**/*.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "secret.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let secret_op = PermissionOperation::Write { + path: std::path::PathBuf::from("secret.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: secret.rs".to_string(), + }; + let other_op = PermissionOperation::Write { + path: std::path::PathBuf::from("main.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: main.rs".to_string(), + }; + + assert_eq!(fixture.can_perform(&secret_op), Permission::Deny); + assert_eq!(fixture.can_perform(&other_op), Permission::Allow); + } + + /// Tie-break test: when two equally-specific rules disagree, the more + /// restrictive permission wins. Both rules use the literal pattern + /// `secret.rs`, so specificity is identical; `Deny` must override + /// `Allow` for safety. + #[test] + fn test_policy_engine_equal_specificity_prefers_deny() { + // policies: + // - permission: allow + // rule: + // write: "secret.rs" // specificity=9 + // - permission: deny + // rule: + // write: "secret.rs" // specificity=9 + // + // operation: write "secret.rs" + // → matches both rules (spec=9 each) + // → specificity tie → restrictiveness breaks it + // → Deny (2) > Allow (0) → deny wins (safe default) + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "secret.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "secret.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let operation = PermissionOperation::Write { + path: std::path::PathBuf::from("secret.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: secret.rs".to_string(), + }; + + let actual = fixture.can_perform(&operation); + let expected = Permission::Deny; + + assert_eq!(actual, expected); + } + #[test] fn test_policy_engine_can_perform_net_fetch() { let fixture_workflow = fixture_workflow_with_net_fetch_policy(); diff --git a/crates/forge_domain/src/policies/policy.rs b/crates/forge_domain/src/policies/policy.rs index 36ac5a344f..3aee6add01 100644 --- a/crates/forge_domain/src/policies/policy.rs +++ b/crates/forge_domain/src/policies/policy.rs @@ -111,6 +111,21 @@ impl Policy { _ => None, } } + + /// Returns the specificity score for this policy. + /// + /// For [`Policy::Simple`], this delegates to [`Rule::specificity`]. + /// For composite policies (`All`/`Any`/`Not`), the maximum specificity + /// across all child rules is used so that nested specific rules still + /// outrank shallower broad ones. + pub fn specificity(&self) -> usize { + match self { + Policy::Simple { permission: _, rule } => rule.specificity(), + Policy::All { all } => all.iter().map(Policy::specificity).max().unwrap_or(0), + Policy::Any { any } => any.iter().map(Policy::specificity).max().unwrap_or(0), + Policy::Not { not } => not.specificity(), + } + } } impl Display for Policy { diff --git a/crates/forge_domain/src/policies/rule.rs b/crates/forge_domain/src/policies/rule.rs index 652dbab8a9..6fcb1aa35b 100644 --- a/crates/forge_domain/src/policies/rule.rs +++ b/crates/forge_domain/src/policies/rule.rs @@ -97,6 +97,36 @@ impl Rule { _ => false, } } + + /// Returns a specificity score for this rule's glob pattern(s). + /// + /// Specificity is the count of *literal* characters in the pattern + /// (i.e. characters that aren't glob wildcards `*` or `?`). Higher + /// scores indicate a narrower, more specific match. The optional `dir` + /// component contributes its own literal-character count to the total. + /// + /// This is used by [`PolicyEngine`] to break ambiguity when multiple + /// rules match the same operation: the rule with the highest + /// specificity wins, regardless of declaration order. + /// + /// [`PolicyEngine`]: crate::PolicyEngine + pub fn specificity(&self) -> usize { + fn count_literals(pattern: &str) -> usize { + pattern + .chars() + .filter(|c| !matches!(c, '*' | '?')) + .count() + } + + let (primary, dir) = match self { + Rule::Write(r) => (r.write.as_str(), r.dir.as_deref()), + Rule::Read(r) => (r.read.as_str(), r.dir.as_deref()), + Rule::Execute(r) => (r.command.as_str(), r.dir.as_deref()), + Rule::Fetch(r) => (r.url.as_str(), r.dir.as_deref()), + }; + + count_literals(primary) + dir.map_or(0, count_literals) + } } /// Helper function to match a glob pattern against a path or string diff --git a/crates/forge_domain/src/policies/types.rs b/crates/forge_domain/src/policies/types.rs index 00433f5f7f..7688df19aa 100644 --- a/crates/forge_domain/src/policies/types.rs +++ b/crates/forge_domain/src/policies/types.rs @@ -15,6 +15,19 @@ pub enum Permission { Confirm, } +impl Permission { + /// Restrictiveness score used as a tie-breaker when multiple policies + /// match an operation with the same specificity. Higher values are more + /// restrictive: `Deny > Confirm > Allow`. + pub(crate) fn restrictiveness(&self) -> u8 { + match self { + Permission::Deny => 2, + Permission::Confirm => 1, + Permission::Allow => 0, + } + } +} + impl Display for Permission { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { From 166e16512cd963fd35df4b87900e24330b0989a0 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 08:14:24 +0000 Subject: [PATCH 2/4] [autofix.ci] apply automated fixes --- crates/forge_domain/src/policies/config.rs | 2 +- crates/forge_domain/src/policies/engine.rs | 39 +++++++--------------- crates/forge_domain/src/policies/rule.rs | 5 +-- 3 files changed, 14 insertions(+), 32 deletions(-) diff --git a/crates/forge_domain/src/policies/config.rs b/crates/forge_domain/src/policies/config.rs index f5cfb22133..3bcee09183 100644 --- a/crates/forge_domain/src/policies/config.rs +++ b/crates/forge_domain/src/policies/config.rs @@ -115,7 +115,7 @@ mod tests { assert_eq!(policies.policies.len(), 3); // Test first policy - get first policy from the set - let first_policy = policies.policies.iter().next().unwrap(); + let first_policy = policies.policies.first().unwrap(); if let Policy::Simple { permission, rule } = first_policy { assert_eq!(permission, &Permission::Allow); if let Rule::Read(rule) = rule { diff --git a/crates/forge_domain/src/policies/engine.rs b/crates/forge_domain/src/policies/engine.rs index 952cb09c66..db8f0dec91 100644 --- a/crates/forge_domain/src/policies/engine.rs +++ b/crates/forge_domain/src/policies/engine.rs @@ -179,15 +179,11 @@ mod tests { #[test] fn test_policy_engine_specific_allow_should_win_over_broad_deny() { // policies: - // - permission: allow - // rule: - // read: "**/*" - // - permission: allow - // rule: - // write: "**/*.rs" // specificity=3 (l: "", "/*/", ".rs") - // - permission: deny - // rule: - // write: "**/*" // specificity=0 (all wildcards) + // - permission: allow rule: read: "**/*" + // - permission: allow rule: write: "**/*.rs" // specificity=3 (l: "", + // "/*/", ".rs") + // - permission: deny rule: write: "**/*" // specificity=0 (all + // wildcards) // // operation: write "test.rs" // → matches allow "**/*.rs" (spec=3) @@ -226,12 +222,8 @@ mod tests { #[test] fn test_policy_engine_specific_allow_wins_regardless_of_order() { // policies: - // - permission: deny - // rule: - // write: "**/*" // specificity=0 - // - permission: allow - // rule: - // write: "**/*.rs" // specificity=3 + // - permission: deny rule: write: "**/*" // specificity=0 + // - permission: allow rule: write: "**/*.rs" // specificity=3 // // operation: write "test.rs" // → matches deny "**/*" (spec=0) @@ -276,12 +268,9 @@ mod tests { #[test] fn test_policy_engine_specific_deny_carves_out_exception_in_broad_allow() { // policies: - // - permission: allow - // rule: - // write: "**/*.rs" // specificity=3 - // - permission: deny - // rule: - // write: "secret.rs" // specificity=9 (all literals) + // - permission: allow rule: write: "**/*.rs" // specificity=3 + // - permission: deny rule: write: "secret.rs" // specificity=9 (all + // literals) // // operation: write "secret.rs" // → matches allow "**/*.rs" (spec=3) @@ -324,12 +313,8 @@ mod tests { #[test] fn test_policy_engine_equal_specificity_prefers_deny() { // policies: - // - permission: allow - // rule: - // write: "secret.rs" // specificity=9 - // - permission: deny - // rule: - // write: "secret.rs" // specificity=9 + // - permission: allow rule: write: "secret.rs" // specificity=9 + // - permission: deny rule: write: "secret.rs" // specificity=9 // // operation: write "secret.rs" // → matches both rules (spec=9 each) diff --git a/crates/forge_domain/src/policies/rule.rs b/crates/forge_domain/src/policies/rule.rs index 6fcb1aa35b..59803bffb7 100644 --- a/crates/forge_domain/src/policies/rule.rs +++ b/crates/forge_domain/src/policies/rule.rs @@ -112,10 +112,7 @@ impl Rule { /// [`PolicyEngine`]: crate::PolicyEngine pub fn specificity(&self) -> usize { fn count_literals(pattern: &str) -> usize { - pattern - .chars() - .filter(|c| !matches!(c, '*' | '?')) - .count() + pattern.chars().filter(|c| !matches!(c, '*' | '?')).count() } let (primary, dir) = match self { From c620d55ac7a4a409f82175ae9b9315cebfb57692 Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 7 May 2026 14:01:19 +0530 Subject: [PATCH 3/4] test(policy): add test for path prefix specificity behavior --- crates/forge_domain/src/policies/engine.rs | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/crates/forge_domain/src/policies/engine.rs b/crates/forge_domain/src/policies/engine.rs index db8f0dec91..6730c4acb7 100644 --- a/crates/forge_domain/src/policies/engine.rs +++ b/crates/forge_domain/src/policies/engine.rs @@ -342,6 +342,57 @@ mod tests { assert_eq!(actual, expected); } + #[test] + fn test_policy_engine_path_prefix_specificity() { + // Specificity is measured by literal-character count in the glob + // pattern — not by semantic narrowness. This test verifies that a + // pattern with more path segments and literal chars outranks a + // shorter, seemingly "simpler" pattern, even when human intuition + // might consider the shorter one more restrictive. + // + // policies: + // - permission: allow + // rule: + // write: "src/**/*.rs" // specificity=7 ("s","r","c","/","/",".","r","s") + // - permission: deny + // rule: + // write: "*.rs" // specificity=3 (".","r","s") + // + // operation: write "src/utils/helper.rs" + // → matches allow "src/**/*.rs" (spec=7) + // → matches deny "*.rs" (spec=3) + // → 7 > 3 → allow wins + // + // operation: write "test.rs" (outside src/) + // → matches deny "*.rs" only (spec=3) + // → deny wins + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "src/**/*.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "*.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let inside_src = PermissionOperation::Write { + path: std::path::PathBuf::from("src/utils/helper.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: src/utils/helper.rs".to_string(), + }; + let outside_src = PermissionOperation::Write { + path: std::path::PathBuf::from("test.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.rs".to_string(), + }; + + // Path "narrowness" (src/ prefix) wins because it has more literals. + assert_eq!(fixture.can_perform(&inside_src), Permission::Allow); + // File outside src/ only matches the broad deny. + assert_eq!(fixture.can_perform(&outside_src), Permission::Deny); + } + #[test] fn test_policy_engine_can_perform_net_fetch() { let fixture_workflow = fixture_workflow_with_net_fetch_policy(); From 5ccb1cb092ea8671e230a98b19dd0e98214959d2 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 08:33:38 +0000 Subject: [PATCH 4/4] [autofix.ci] apply automated fixes --- crates/forge_domain/src/policies/engine.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/forge_domain/src/policies/engine.rs b/crates/forge_domain/src/policies/engine.rs index 6730c4acb7..b02a1f1a14 100644 --- a/crates/forge_domain/src/policies/engine.rs +++ b/crates/forge_domain/src/policies/engine.rs @@ -351,12 +351,10 @@ mod tests { // might consider the shorter one more restrictive. // // policies: - // - permission: allow - // rule: - // write: "src/**/*.rs" // specificity=7 ("s","r","c","/","/",".","r","s") - // - permission: deny - // rule: - // write: "*.rs" // specificity=3 (".","r","s") + // - permission: allow rule: write: "src/**/*.rs" // specificity=7 + // ("s","r","c","/","/",".","r","s") + // - permission: deny rule: write: "*.rs" // specificity=3 + // (".","r","s") // // operation: write "src/utils/helper.rs" // → matches allow "src/**/*.rs" (spec=7)