refactor(policy): use most-specific-pattern-wins evaluation model

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<Policy>,
+    /// Ordered list of policies to evaluate
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    pub policies: Vec<Policy>,
 }
 
 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
     }
 
@@ -109,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 {

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<Permission> = 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<Item = &'p Policy>>(
         &self,
         policies: I,
         operation: &PermissionOperation,
     ) -> Option<Permission> {
-        let mut last_allow: Option<Permission> = 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,232 @@ 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_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();

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 {