From e865f13102a3c241c4a36d9bae38f2a0881e2e61 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Jun 2026 17:05:19 +0000 Subject: [PATCH 1/5] Initial plan From bc2fcc1cee9b7ad8d86195ad6afd64f8579978fa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Jun 2026 19:15:12 +0000 Subject: [PATCH 2/5] Fix import-driven cross-package lookup behavior in interpreter and RVM --- src/interpreter.rs | 121 ++++++++++++++++------ src/languages/rego/compiler/references.rs | 9 ++ tests/engine/mod.rs | 31 ++++++ tests/rvm/rego/cases/imports.yaml | 34 ++++++ 4 files changed, 161 insertions(+), 34 deletions(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index 2028eb941..0f1df75d7 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -586,7 +586,7 @@ impl Interpreter { } else { format!("{ref_path}.{index}.{}", path.join(".")) }; - self.ensure_rule_evaluated(ref_path)?; + self.ensure_matching_rules_for_dynamic_data_index(&ref_path)?; } } @@ -2960,6 +2960,80 @@ impl Interpreter { Ok(()) } + /// Ensures all rule/default-rule paths matching a dynamic data lookup are evaluated. + /// Matches both exact path (`data.a.b`) and descendants (`data.a.b.*`). + fn ensure_matching_rules_for_dynamic_data_index(&mut self, path: &str) -> Result<()> { + self.check_execution_time()?; + let path_prefix = format!("{path}."); + let mut matching_paths: Vec = self + .compiled_policy + .default_rules + .keys() + .chain(self.compiled_policy.rules.keys()) + .filter(|rule_path| *rule_path == path || rule_path.starts_with(&path_prefix)) + .cloned() + .collect(); + matching_paths.sort(); + matching_paths.dedup(); + + for rule_path in matching_paths { + self.ensure_rule_evaluated(rule_path)?; + } + + Ok(()) + } + + /// Builds a canonical `data` path from field components. + /// For an empty field list, returns `"data"`. + fn build_data_path(fields: &[&str]) -> String { + if fields.is_empty() { + "data".to_string() + } else { + format!("data.{}", fields.join(".")) + } + } + + /// Resolves `data.` while preserving correct rule semantics. + /// When a rule is already active, this avoids eager module-wide evaluation to prevent + /// false-positive recursion detection and only evaluates matching rule paths. + fn lookup_data_path(&mut self, fields: &[&str]) -> Result { + if self.is_processed(fields)? { + return Ok(Self::get_value_chained(self.data.clone(), fields)); + } + + // If "data" is used in a query, without any fields, then evaluate all the modules. + if fields.is_empty() && self.active_rules.is_empty() { + for module in self.compiled_policy.modules.clone().iter() { + for rule in &module.policy { + self.eval_rule(module, rule)?; + } + } + } + + // While a rule is active, avoid eagerly evaluating all matching modules. + // This prevents sibling/module re-entry from being misclassified as cyclic recursion. + let requested_path = Self::build_data_path(fields); + if self.active_rules.is_empty() { + self.ensure_module_evaluated(requested_path.clone())?; + } + + for i in (1..=fields.len()).rev() { + let prefix = fields.iter().take(i).copied().collect::>(); + let prefix_path = Self::build_data_path(&prefix); + if self.compiled_policy.rules.contains_key(&prefix_path) + || self + .compiled_policy + .default_rules + .contains_key(&prefix_path) + { + self.ensure_rule_evaluated(prefix_path)?; + break; + } + } + + Ok(Self::get_value_chained(self.data.clone(), fields)) + } + fn is_processed(&self, path: &[&str]) -> Result { let mut obj = &self.processed_paths; for p in path { @@ -3010,39 +3084,7 @@ impl Interpreter { // Ensure that rules are evaluated if name.text() == "data" { - if self.is_processed(fields)? { - return Ok(Self::get_value_chained(self.data.clone(), fields)); - } - - // If "data" is used in a query, without any fields, then evaluate all the modules. - if fields.is_empty() && self.active_rules.is_empty() { - for module in self.compiled_policy.modules.clone().iter() { - for rule in &module.policy { - self.eval_rule(module, rule)?; - } - } - } - - // With modifiers may be used to specify part of a module that that not yet been - // evaluated. Therefore ensure that module is evaluated first. - let requested_path = format!("data.{}", fields.join(".")); - self.ensure_module_evaluated(requested_path.clone())?; - - for i in (1..=fields.len()).rev() { - let prefix = fields.iter().take(i).copied().collect::>(); - let prefix_path = format!("data.{}", prefix.join(".")); - if self.compiled_policy.rules.contains_key(&prefix_path) - || self - .compiled_policy - .default_rules - .contains_key(&prefix_path) - { - self.ensure_rule_evaluated(prefix_path)?; - break; - } - } - - Ok(Self::get_value_chained(self.data.clone(), fields)) + self.lookup_data_path(fields) } else if !self.compiled_policy.modules.is_empty() { let module = self.current_module()?; let parsed_path = Parser::get_path_ref_components(&module.package.refr)?; @@ -3092,6 +3134,17 @@ impl Interpreter { if !found { if let Some(imported_var) = self.compiled_policy.imports.get(&rule_path).cloned() { + if let Ok(import_path) = get_path_string(&imported_var, None) { + if import_path == "data" || import_path.starts_with("data.") { + let combined_path = if fields.is_empty() { + import_path + } else { + format!("{}.{}", import_path, fields.join(".")) + }; + let data_fields: Vec<&str> = combined_path.split('.').skip(1).collect(); + return self.lookup_data_path(&data_fields); + } + } return Ok(Self::get_value_chained( self.eval_expr(&imported_var)?, fields, diff --git a/src/languages/rego/compiler/references.rs b/src/languages/rego/compiler/references.rs index e5bfc7ad2..b7c06cc5a 100644 --- a/src/languages/rego/compiler/references.rs +++ b/src/languages/rego/compiler/references.rs @@ -338,6 +338,15 @@ impl<'a> Compiler<'a> { // No rule found; fall back to module-level imports. let import_key = format!("{}.{}", &self.current_package, root); if let Some(import_expr) = self.policy.inner.imports.get(&import_key) { + if let Ok(mut import_chain) = parse_reference_chain(import_expr) { + if let ReferenceRoot::Variable(import_root) = &import_chain.root { + if import_root == "data" { + import_chain.components.extend(chain.components.clone()); + return self.compile_data_chain(&import_chain, span); + } + } + } + let import_reg = self.compile_rego_expr_with_span(import_expr, import_expr.span(), false)?; if chain.components.is_empty() { diff --git a/tests/engine/mod.rs b/tests/engine/mod.rs index 292bcb9b9..310419ab1 100644 --- a/tests/engine/mod.rs +++ b/tests/engine/mod.rs @@ -197,3 +197,34 @@ fn get_policy_parameters() -> Result<()> { Ok(()) } + +#[test] +fn cross_package_import_lookup_does_not_false_cycle() -> Result<()> { + let mut engine = Engine::new(); + engine.add_policy( + "registry.rego".to_string(), + r#"package registry + import data.registry.packages.package_a + import rego.v1 + + allow_stage1 if package_a.allow_stage1 + "# + .to_string(), + )?; + engine.add_policy( + "package_a.rego".to_string(), + r#"package registry.packages.package_a + import data.registry + import rego.v1 + + allow_stage2 if registry.allow_stage1 + allow_stage1 := true + "# + .to_string(), + )?; + + let result = engine.eval_rule("data.registry.packages.package_a.allow_stage2".to_string())?; + assert_eq!(result, Value::Bool(true)); + + Ok(()) +} diff --git a/tests/rvm/rego/cases/imports.yaml b/tests/rvm/rego/cases/imports.yaml index 198a3928b..12731c5ee 100644 --- a/tests/rvm/rego/cases/imports.yaml +++ b/tests/rvm/rego/cases/imports.yaml @@ -95,3 +95,37 @@ cases: } query: data.rules.present want_result: true + - note: import_cross_package_no_false_cycle + modules: + - | + package registry + import data.registry.packages.package_a + import rego.v1 + + allow_stage1 if package_a.allow_stage1 + - | + package registry.packages.package_a + import data.registry + import rego.v1 + + allow_stage2 if registry.allow_stage1 + allow_stage1 := true + query: data.registry.packages.package_a.allow_stage2 + want_result: true + - note: dynamic_data_index_evaluates_matching_rules + modules: + - | + package registry + import rego.v1 + + allow_stage1 if { + name := "package_a" + data.registry.packages[name].allow_stage2 + } + - | + package registry.packages.package_a + import rego.v1 + + allow_stage2 := true + query: data.registry.allow_stage1 + want_result: true From e8f126d4798bac772788032599580ec8d6f7617c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Jun 2026 19:18:20 +0000 Subject: [PATCH 3/5] Refine module-evaluation deferral logic for active rules --- src/interpreter.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index 0f1df75d7..8fa3b801f 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -2993,6 +2993,27 @@ impl Interpreter { } } + fn path_is_prefix(prefix: &str, path: &str) -> bool { + if path == prefix { + return true; + } + path.get(prefix.len()..) + .is_some_and(|suffix| suffix.starts_with('.')) + } + + fn should_defer_module_eval_for_path(&self, requested_path: &str) -> Result { + for active_rule in &self.active_rules { + let module = self.get_rule_module(active_rule)?; + let module_path = get_path_string(&module.package.refr, Some("data"))?; + let rule_path = get_path_string(Self::get_rule_refr(active_rule), None)?; + let full_rule_path = format!("{}.{}", module_path, rule_path); + if Self::path_is_prefix(requested_path, &full_rule_path) { + return Ok(true); + } + } + Ok(false) + } + /// Resolves `data.` while preserving correct rule semantics. /// When a rule is already active, this avoids eager module-wide evaluation to prevent /// false-positive recursion detection and only evaluates matching rule paths. @@ -3013,7 +3034,9 @@ impl Interpreter { // While a rule is active, avoid eagerly evaluating all matching modules. // This prevents sibling/module re-entry from being misclassified as cyclic recursion. let requested_path = Self::build_data_path(fields); - if self.active_rules.is_empty() { + if self.active_rules.is_empty() + || !self.should_defer_module_eval_for_path(&requested_path)? + { self.ensure_module_evaluated(requested_path.clone())?; } From 6dc3a3e2dcd1853cc23408d491dc2034840c5b7b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Jun 2026 21:04:19 +0000 Subject: [PATCH 4/5] Improve active-rule deferral matching docs and path checks --- src/interpreter.rs | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index 8fa3b801f..44da3d900 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -2993,6 +2993,12 @@ impl Interpreter { } } + /// Returns `true` when `prefix` matches `path` on segment boundaries. + /// + /// Examples: + /// - `path_is_prefix("data.auth", "data.auth") == true` + /// - `path_is_prefix("data.auth", "data.auth.allow") == true` + /// - `path_is_prefix("data.auth", "data.authorization") == false` fn path_is_prefix(prefix: &str, path: &str) -> bool { if path == prefix { return true; @@ -3001,13 +3007,39 @@ impl Interpreter { .is_some_and(|suffix| suffix.starts_with('.')) } + /// Checks whether a `requested_path` can contain values produced by an active rule. + /// + /// The active rule path is logically `module_path.rule_path`, but this check avoids + /// allocating that joined string in tight evaluation loops. + /// + /// Examples: + /// - request `data` matches module `data.authz` (module expansion needed) + /// - request `data.authz` matches rule `allow` + /// - request `data.authz.allow` matches rule `allow` + /// - request `data.auth` does not match module `data.authz` + fn request_matches_active_rule_path( + requested_path: &str, + module_path: &str, + rule_path: &str, + ) -> bool { + if Self::path_is_prefix(requested_path, module_path) { + return true; + } + + requested_path + .strip_prefix(module_path) + .and_then(|suffix| suffix.strip_prefix('.')) + .is_some_and(|requested_rule_prefix| { + Self::path_is_prefix(requested_rule_prefix, rule_path) + }) + } + fn should_defer_module_eval_for_path(&self, requested_path: &str) -> Result { for active_rule in &self.active_rules { let module = self.get_rule_module(active_rule)?; let module_path = get_path_string(&module.package.refr, Some("data"))?; let rule_path = get_path_string(Self::get_rule_refr(active_rule), None)?; - let full_rule_path = format!("{}.{}", module_path, rule_path); - if Self::path_is_prefix(requested_path, &full_rule_path) { + if Self::request_matches_active_rule_path(requested_path, &module_path, &rule_path) { return Ok(true); } } From 6cf77bb5eaa13a6b6d07d0955a3d083b37b54b38 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Jun 2026 21:10:05 +0000 Subject: [PATCH 5/5] Clarify deferral comments and dynamic path docs --- src/interpreter.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index 44da3d900..e13d6eab7 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -2961,7 +2961,7 @@ impl Interpreter { } /// Ensures all rule/default-rule paths matching a dynamic data lookup are evaluated. - /// Matches both exact path (`data.a.b`) and descendants (`data.a.b.*`). + /// Matches both exact path (`data.a.b`) and descendants with the `data.a.b.` prefix. fn ensure_matching_rules_for_dynamic_data_index(&mut self, path: &str) -> Result<()> { self.check_execution_time()?; let path_prefix = format!("{path}."); @@ -3047,14 +3047,14 @@ impl Interpreter { } /// Resolves `data.` while preserving correct rule semantics. - /// When a rule is already active, this avoids eager module-wide evaluation to prevent - /// false-positive recursion detection and only evaluates matching rule paths. + /// When a rule is already active, this avoids eager module-wide evaluation so legitimate + /// cross-package references are not misidentified as cyclic recursion. fn lookup_data_path(&mut self, fields: &[&str]) -> Result { if self.is_processed(fields)? { return Ok(Self::get_value_chained(self.data.clone(), fields)); } - // If "data" is used in a query, without any fields, then evaluate all the modules. + // If "data" is used in a query without any fields, then evaluate all modules. if fields.is_empty() && self.active_rules.is_empty() { for module in self.compiled_policy.modules.clone().iter() { for rule in &module.policy { @@ -3064,7 +3064,8 @@ impl Interpreter { } // While a rule is active, avoid eagerly evaluating all matching modules. - // This prevents sibling/module re-entry from being misclassified as cyclic recursion. + // This prevents re-entry through sibling rules or other modules from being + // misclassified as cyclic recursion. let requested_path = Self::build_data_path(fields); if self.active_rules.is_empty() || !self.should_defer_module_eval_for_path(&requested_path)?