From 268c5c76705c895ffa265170d96e3824ee95f85e Mon Sep 17 00:00:00 2001 From: Ivan Miletic Date: Tue, 22 Sep 2026 14:48:02 +0200 Subject: [PATCH] fix: policy sibling issues --- .../src/policy/blocks/decision_table.rs | 23 +- core/engine/src/policy/queries/dependency.rs | 20 -- core/engine/src/policy/queries/diagnostics.rs | 78 ++--- core/engine/src/workspace/db.rs | 34 ++- .../policy/entities_merge_multi_policy.toml | 18 +- core/engine/tests/data/policy/evaluation.toml | 10 +- .../data/policy/fixtures/multi_main.json | 36 +-- .../data/policy/fixtures/multi_shared.json | 26 ++ .../data/policy/rename_multi_policy.toml | 5 +- core/engine/tests/policy_import_scope.rs | 268 ++++++++++++++++++ core/engine/tests/policy_slot.rs | 10 +- core/engine/tests/policy_table.rs | 51 +++- core/expression/src/isolate.rs | 11 + 13 files changed, 464 insertions(+), 126 deletions(-) create mode 100644 core/engine/tests/policy_import_scope.rs diff --git a/core/engine/src/policy/blocks/decision_table.rs b/core/engine/src/policy/blocks/decision_table.rs index 1f166857..27f15c82 100644 --- a/core/engine/src/policy/blocks/decision_table.rs +++ b/core/engine/src/policy/blocks/decision_table.rs @@ -5,7 +5,7 @@ use fixedbitset::FixedBitSet; use serde::{Deserialize, Serialize}; use zen_expression::intellisense::{ArmTest, IntelliSense, NumberCover}; use zen_expression::variable::{Variable, VariableType}; -use zen_expression::Isolate; +use zen_expression::{Isolate, IsolateError}; use zen_types::decision::{ DecisionTableHitPolicy, DecisionTableInputField, DecisionTableOutputField, }; @@ -1212,7 +1212,26 @@ impl DecisionTableIr { .map_err(|e| cx.expression_error(field, e))?; isolate .run_unary(cell) - .map_err(|e| cx.expression_error(cell, e)) + .map_err(|source| { + let value_type = col_refs[col_idx] + .as_ref() + .map(Variable::type_name) + .unwrap_or("unknown"); + let value_description = if value_type == "null" { + "missing or null" + } else { + value_type + }; + cx.expression_error( + cell, + IsolateError::ContextError { + context: format!( + "Cannot evaluate condition {cell:?} for input {field:?} ({value_description})" + ), + source: Box::new(source), + }, + ) + }) } _ => { let result = isolate diff --git a/core/engine/src/policy/queries/dependency.rs b/core/engine/src/policy/queries/dependency.rs index ae590e71..256dd818 100644 --- a/core/engine/src/policy/queries/dependency.rs +++ b/core/engine/src/policy/queries/dependency.rs @@ -23,10 +23,8 @@ use crate::workspace::types::{BlockRef, Diagnostic, DiagnosticCode, DiagnosticLo #[derive(Debug)] pub struct ShallowAnalyses { pub per_rule: Vec, - pub diagnostics: Vec, by_block: HashMap, rules_by_path: HashMap, std::ops::Range>, - diags_by_path: HashMap, std::ops::Range>, } impl ShallowAnalyses { @@ -42,13 +40,6 @@ impl ShallowAnalyses { .map(|r| &self.per_rule[r.clone()]) .unwrap_or(&[]) } - - pub fn diags_for(&self, path: &Arc) -> &[Diagnostic] { - self.diags_by_path - .get(path) - .map(|r| &self.diagnostics[r.clone()]) - .unwrap_or(&[]) - } } #[derive(Debug, Clone)] @@ -372,25 +363,17 @@ impl Snapshot { pub(crate) fn compute_shallow( base_scope: &VariableType, all_parsed: &HashMap, Arc>, - classifier: &PathClassifier, intellisense: &SharedIntelliSense, cache: &PolicyDerivedCache, ) -> ShallowAnalyses { let mut per_rule: Vec = Vec::new(); - let mut diagnostics: Vec = Vec::new(); let mut rules_by_path: HashMap, std::ops::Range> = HashMap::new(); - let mut diags_by_path: HashMap, std::ops::Range> = HashMap::new(); let mut sorted_paths: Vec<&Arc> = all_parsed.keys().collect(); sorted_paths.sort(); for path in sorted_paths { let p = &all_parsed[path]; let rules_start = per_rule.len(); - let diags_start = diagnostics.len(); - - for rule in p.policy.rules() { - rule.check_single_entity_scope(path, classifier, &mut diagnostics); - } let no_dictionaries: SharedDictionaryTypes = Rc::new(ahash::HashMap::default()); let no_poison: SharedPoisonedPaths = Default::default(); @@ -420,7 +403,6 @@ impl Snapshot { }); per_rule.extend(policy_shallow.iter().cloned()); rules_by_path.insert(path.clone(), rules_start..per_rule.len()); - diags_by_path.insert(path.clone(), diags_start..diagnostics.len()); } let by_block = per_rule @@ -439,10 +421,8 @@ impl Snapshot { ShallowAnalyses { per_rule, - diagnostics, by_block, rules_by_path, - diags_by_path, } } diff --git a/core/engine/src/policy/queries/diagnostics.rs b/core/engine/src/policy/queries/diagnostics.rs index c1161161..3b31168e 100644 --- a/core/engine/src/policy/queries/diagnostics.rs +++ b/core/engine/src/policy/queries/diagnostics.rs @@ -17,10 +17,28 @@ impl Db { out.extend(parsed.diagnostics.iter().cloned()); } - let shallow = self.shallow(); - out.extend(shallow.diags_for(path).iter().cloned()); + let unit = self.unit(path); + if let Some(parsed) = self.parsed(path) { + for rule in parsed.policy.rules() { + rule.check_single_entity_scope(path, &unit.classifier, &mut out); + } + } - out.extend(self.graph_diagnostics(path)); + // Independently valid imports can conflict when composed. Surface those + // conflicts on the importing entry, even if neither writer is local. + let mut scope_diagnostics = self.graph_diagnostics(path); + scope_diagnostics.extend(self.data_model_diagnostics(path)); + scope_diagnostics.extend(self.dictionary_diagnostics(path)); + for mut diagnostic in scope_diagnostics { + if !diagnostic.is_in(path) { + diagnostic.message = format!( + "in imported policy '{}': {}", + diagnostic.location.policy_path, diagnostic.message + ); + diagnostic.location = DiagnosticLocation::policy(path.clone()); + } + out.push(diagnostic); + } let enriched = self.enriched(path); out.extend( @@ -40,10 +58,6 @@ impl Db { out.extend(self.import_diagnostics(path)); - out.extend(self.data_model_diagnostics(path)); - - out.extend(self.dictionary_diagnostics(path)); - out.extend(self.unreachable_reads_diagnostics(path)); out.extend(self.nested_iteration_diagnostics(path)); @@ -178,25 +192,20 @@ impl Db { .and_then(|b| b.kind.write_target(&write.path)); if let Some(matched) = data_model_paths.matches_prefix(&write.path) { - if in_target { - out.push(Diagnostic::error( - DiagnosticCode::InputOverride, - DiagnosticLocation::block( - rule.policy_path.clone(), - rule.block_id.clone(), - ) + out.push(Diagnostic::error( + DiagnosticCode::InputOverride, + DiagnosticLocation::block(rule.policy_path.clone(), rule.block_id.clone()) .maybe_target(wtarget.clone()), - format!( - "cannot write to '{}': '{}' is defined as a DataModel input", - write.path, matched - ), - )); - } + format!( + "cannot write to '{}': '{}' is defined as a DataModel input", + write.path, matched + ), + )); continue; } match first_writer.get(&write.path) { - Some(existing) if in_target => { + Some(existing) => { out.push(Diagnostic::error( DiagnosticCode::DuplicateWriter, DiagnosticLocation::block( @@ -214,7 +223,6 @@ impl Db { ), )); } - Some(_) => {} None => { first_writer.insert(write.path.clone(), block_ref.clone()); } @@ -276,7 +284,11 @@ impl Db { blocks.push((block_ref, *in_t)); } } - let Some((owner, _)) = blocks.iter().find(|(_, in_t)| *in_t) else { + let Some((owner, _)) = blocks + .iter() + .find(|(_, in_t)| *in_t) + .or_else(|| blocks.first()) + else { continue; }; let cross_policy = blocks @@ -305,12 +317,7 @@ impl Db { let graph = &unit.dep_graph; let cyclic = graph.cyclic_paths(); - let target_in_cycle = cyclic.iter().any(|path| { - graph - .writer_for(path) - .is_some_and(|owner| owner.policy_path == *target) - }); - if target_in_cycle { + if !cyclic.is_empty() { out.push(Diagnostic::error( DiagnosticCode::CyclicDependency, DiagnosticLocation::policy(target.clone()), @@ -369,7 +376,7 @@ impl Db { let dm = &entry.ir; let is_global = dm.scope.is_global(); - if !is_global && global_property_names.contains(&dm.name) && policy_path == target { + if !is_global && global_property_names.contains(&dm.name) { out.push(Diagnostic::error( DiagnosticCode::DataModelCollision, DiagnosticLocation::block(policy_path.clone(), block_id.clone()), @@ -381,7 +388,7 @@ impl Db { } for prop in &dm.properties { - if is_global && known_entities.contains(&prop.name) && policy_path == target { + if is_global && known_entities.contains(&prop.name) { out.push(Diagnostic::error( DiagnosticCode::DataModelCollision, DiagnosticLocation::expression( @@ -408,7 +415,7 @@ impl Db { let conflicts = !prop.kind.same_shape_as(&prev_kind) || prev_array != prop.array || prev_optional != prop.optional; - if conflicts && policy_path == target { + if conflicts { let location = if is_global { format!("global property '{}'", prop.name) } else { @@ -490,8 +497,7 @@ impl Db { for entry in &unit.dictionary_blocks { let name = &entry.ir.name; if let Some((prev_policy, prev_block)) = first_by_name.get(name) { - if entry.policy_path == *target { - out.push(Diagnostic::error( + out.push(Diagnostic::error( DiagnosticCode::DataModelCollision, DiagnosticLocation::block( entry.policy_path.clone(), @@ -501,7 +507,6 @@ impl Db { "dictionary '{name}' is already defined in '{prev_policy}' (block '{prev_block}')" ), )); - } continue; } first_by_name.insert( @@ -509,9 +514,6 @@ impl Db { (entry.policy_path.clone(), entry.block_id.clone()), ); - if entry.policy_path != *target { - continue; - } if known_entities.contains(name) { out.push(Diagnostic::error( DiagnosticCode::DataModelCollision, diff --git a/core/engine/src/workspace/db.rs b/core/engine/src/workspace/db.rs index 0df635c3..0e065318 100644 --- a/core/engine/src/workspace/db.rs +++ b/core/engine/src/workspace/db.rs @@ -139,7 +139,7 @@ pub struct Snapshot { pub(crate) shallow: Arc, pub(crate) components: Vec>>, pub(crate) policy_to_component: HashMap, usize>, - pub(crate) units: RefCell>>, + pub(crate) units: RefCell, Arc>>, pub(crate) policy_diagnostics: RefCell, Arc>>>, pub(crate) eval_artifacts: RefCell, Arc>>, pub(crate) path_set: OnceCell>>>, @@ -450,23 +450,33 @@ impl Db { pub fn unit(&self, policy: &str) -> Arc { let snap = self.snapshot(); - let Some(&idx) = snap.policy_to_component.get(policy) else { - return self.cache.unit_or_compute(&[], &[], || { - Snapshot::compute_unit(&[], &snap.all_parsed, &snap.shallow) - }); - }; - if let Some(u) = snap.units.borrow().get(&idx).cloned() { + if let Some(u) = snap.units.borrow().get(policy).cloned() { return u; } - let members = &snap.components[idx]; + // Connectivity is useful for workspace navigation, but an import only + // makes its dependencies visible. Other importers are separate entries. + let mut seen = HashSet::default(); + let mut stack = vec![Arc::::from(policy)]; + while let Some(path) = stack.pop() { + let Some(parsed) = snap.all_parsed.get(&path) else { + continue; + }; + if seen.insert(path) { + stack.extend(parsed.policy.imports().iter().cloned()); + } + } + let mut members: Vec<_> = seen.into_iter().collect(); + members.sort(); let parsed: Vec> = members .iter() .filter_map(|m| snap.all_parsed.get(m).cloned()) .collect(); - let unit = self.cache.unit_or_compute(members, &parsed, || { - Snapshot::compute_unit(members, &snap.all_parsed, &snap.shallow) + let unit = self.cache.unit_or_compute(&members, &parsed, || { + Snapshot::compute_unit(&members, &snap.all_parsed, &snap.shallow) }); - snap.units.borrow_mut().insert(idx, unit.clone()); + snap.units + .borrow_mut() + .insert(Arc::from(policy), unit.clone()); unit } @@ -771,11 +781,9 @@ impl Snapshot { let entity_sources = Self::compute_entity_sources(&all_parsed); let base_scope = Self::compute_base_scope(&all_parsed, &entity_sources); - let classifier = Self::compute_path_classifier(&all_parsed); let shallow = Arc::new(Self::compute_shallow( &base_scope, &all_parsed, - &classifier, intellisense, cache, )); diff --git a/core/engine/tests/data/policy/entities_merge_multi_policy.toml b/core/engine/tests/data/policy/entities_merge_multi_policy.toml index 1cf734b1..97e9a03c 100644 --- a/core/engine/tests/data/policy/entities_merge_multi_policy.toml +++ b/core/engine/tests/data/policy/entities_merge_multi_policy.toml @@ -54,9 +54,7 @@ entity = "customer" no_duplicate_fields = true # ── Visibility from policy B's perspective ─────────────────────────────────── -# v1 visibility was directional (B does not import A → B saw only its own -# fields). v2 units are import components, so A and B share one unit and B -# sees A's fields too, with source attribution preserved. +# Imports are directional: B must not see fields declared by its importer A. [[test]] name = "entities(B): has own fields" @@ -80,17 +78,7 @@ field = "label" property_kind = "computed" [[test]] -name = "entities(B): sees age from A via component visibility" +name = "entities(B): importer fields remain hidden" policy = "merge_policy_b.json" entity = "customer" -field = "age" -property_kind = "input" -source = "merge_policy_a.json" - -[[test]] -name = "entities(B): sees greeting computed by A via component visibility" -policy = "merge_policy_b.json" -entity = "customer" -field = "greeting" -property_kind = "computed" -source = "merge_policy_a.json" +absent_fields = ["age", "greeting"] diff --git a/core/engine/tests/data/policy/evaluation.toml b/core/engine/tests/data/policy/evaluation.toml index 6b06ab08..4dbe0d30 100644 --- a/core/engine/tests/data/policy/evaluation.toml +++ b/core/engine/tests/data/policy/evaluation.toml @@ -265,7 +265,7 @@ kind = "expression" [[test]] name = "cross-policy — eligible high-income → platinum" -policies = ["cross_dep_base.json", "cross_dep_consumer.json"] +policies = ["cross_dep_consumer.json", "cross_dep_base.json"] input = { customer = { name = "Alice", age = 30, income = 120000 } } output = { customer = { isEligible = true, tier = "platinum" } } @@ -279,7 +279,7 @@ matched_rows = [0] [[test]] name = "cross-policy — eligible mid-income → gold" -policies = ["cross_dep_base.json", "cross_dep_consumer.json"] +policies = ["cross_dep_consumer.json", "cross_dep_base.json"] input = { customer = { name = "Bob", age = 25, income = 60000 } } output = { customer = { isEligible = true, tier = "gold" } } @@ -293,7 +293,7 @@ matched_rows = [1] [[test]] name = "cross-policy — eligible low-income → silver" -policies = ["cross_dep_base.json", "cross_dep_consumer.json"] +policies = ["cross_dep_consumer.json", "cross_dep_base.json"] input = { customer = { name = "Carol", age = 40, income = 35000 } } output = { customer = { isEligible = true, tier = "silver" } } @@ -303,7 +303,7 @@ matched_rows = [2] [[test]] name = "cross-policy — ineligible (underage) → rejected" -policies = ["cross_dep_base.json", "cross_dep_consumer.json"] +policies = ["cross_dep_consumer.json", "cross_dep_base.json"] input = { customer = { name = "Dave", age = 16, income = 50000 } } output = { customer = { isEligible = false, tier = "rejected" } } @@ -321,7 +321,7 @@ matched_rows = [3] [[test]] name = "cross-policy — ineligible (low income) → rejected" -policies = ["cross_dep_base.json", "cross_dep_consumer.json"] +policies = ["cross_dep_consumer.json", "cross_dep_base.json"] input = { customer = { name = "Eve", age = 25, income = 20000 } } output = { customer = { isEligible = false, tier = "rejected" } } diff --git a/core/engine/tests/data/policy/fixtures/multi_main.json b/core/engine/tests/data/policy/fixtures/multi_main.json index 726456f4..b7f0f71f 100644 --- a/core/engine/tests/data/policy/fixtures/multi_main.json +++ b/core/engine/tests/data/policy/fixtures/multi_main.json @@ -3,32 +3,6 @@ "multi_shared.json" ], "blocks": [ - { - "id": "dm-customer", - "type": "dataModel", - "props": { - "data": { - "name": "customer", - "properties": [ - { - "id": "p1", - "name": "name", - "type": "string", - "array": false, - "optional": false - }, - { - "id": "p2", - "name": "age", - "type": "number", - "array": false, - "optional": false - } - ] - } - }, - "children": [] - }, { "id": "s1", "type": "expression", @@ -38,6 +12,16 @@ "value": "customer.tier" } } + }, + { + "id": "age-read", + "type": "expression", + "props": { + "data": { + "key": "customer.ageCopy", + "value": "customer.age" + } + } } ] } diff --git a/core/engine/tests/data/policy/fixtures/multi_shared.json b/core/engine/tests/data/policy/fixtures/multi_shared.json index 6f4d73b4..49e97f0d 100644 --- a/core/engine/tests/data/policy/fixtures/multi_shared.json +++ b/core/engine/tests/data/policy/fixtures/multi_shared.json @@ -1,5 +1,31 @@ { "blocks": [ + { + "id": "dm-customer", + "type": "dataModel", + "props": { + "data": { + "name": "customer", + "properties": [ + { + "id": "p1", + "name": "name", + "type": "string", + "array": false, + "optional": false + }, + { + "id": "p2", + "name": "age", + "type": "number", + "array": false, + "optional": false + } + ] + } + }, + "children": [] + }, { "id": "tree1", "type": "match", diff --git a/core/engine/tests/data/policy/rename_multi_policy.toml b/core/engine/tests/data/policy/rename_multi_policy.toml index 773d5fa4..8cbbb92c 100644 --- a/core/engine/tests/data/policy/rename_multi_policy.toml +++ b/core/engine/tests/data/policy/rename_multi_policy.toml @@ -14,7 +14,7 @@ edits = [ ] # customer.age IS referenced in shared policy arm condition "customer.age > 50" -# and defined in multi_main's dm-customer data model. +# and declared in shared's dm-customer data model, with a read in main. [[test]] name = "rename customer.age finds reference in shared policy condition" entity = "customer" @@ -22,7 +22,8 @@ field = "age" new_name = "years" edits = [ { policy = "multi_shared.json", block_id = "tree1", expression_id = "b1" }, - { policy = "multi_main.json", block_id = "dm-customer", target_kind = "fieldName" }, + { policy = "multi_shared.json", block_id = "dm-customer", target_kind = "fieldName" }, + { policy = "multi_main.json", block_id = "age-read", expression_id = "age-read" }, ] [[test]] diff --git a/core/engine/tests/policy_import_scope.rs b/core/engine/tests/policy_import_scope.rs new file mode 100644 index 00000000..00e81699 --- /dev/null +++ b/core/engine/tests/policy_import_scope.rs @@ -0,0 +1,268 @@ +use std::sync::Arc; + +use serde_json::{json, Value}; +use zen_engine::loader::MemoryLoader; +use zen_engine::policy::{EvaluateRequest, PolicyWorkspace, ScopeRequest, Severity}; +use zen_engine::DecisionEngine; + +fn expression(key: &str, value: &str) -> Value { + json!({"id": key, "type": "expression", "props": {"data": {"key": key, "value": value}}}) +} + +fn dictionary(name: &str, value: &str) -> Value { + json!({"id": name, "type": "dictionary", "props": {"data": { + "name": name, "entries": [{"value": value, "label": value}] + }}}) +} + +fn policy(imports: &[&str], blocks: Vec) -> Value { + json!({"contentType": "policy", "imports": imports, "blocks": blocks}) +} + +fn siblings(duplicate: bool) -> Vec<(&'static str, Value)> { + vec![ + ("A", policy(&[], vec![expression("base", "10")])), + ( + "B", + policy( + &["A"], + vec![ + dictionary("choice", "approve"), + expression("decision", "\"approve\""), + ], + ), + ), + ( + "C", + policy( + &["A"], + vec![ + dictionary(if duplicate { "choice" } else { "otherChoice" }, "decline"), + expression(if duplicate { "decision" } else { "other" }, "\"decline\""), + ], + ), + ), + ] +} + +fn workspace(docs: &[(&str, Value)]) -> PolicyWorkspace { + let mut ws = PolicyWorkspace::new(); + for (path, doc) in docs { + ws.set_policy(*path, serde_json::from_value(doc.clone()).unwrap()); + } + ws +} + +fn request(path: &str, input: Value) -> EvaluateRequest { + EvaluateRequest { + policy_path: Arc::from(path), + input: input.into(), + goals: vec![], + trace: true, + } +} + +fn output(ws: &PolicyWorkspace, path: &str) -> Value { + serde_json::to_value(ws.evaluate(&request(path, json!({}))).unwrap().output).unwrap() +} + +#[test] +fn workspace_and_trace_only_execute_outgoing_imports() { + let ws = workspace(&siblings(false)); + for (path, expected, count) in [ + ("A", json!({"base":10}), 1), + ("B", json!({"base":10,"decision":"approve"}), 2), + ("C", json!({"base":10,"other":"decline"}), 2), + ] { + assert_eq!(output(&ws, path), expected); + let enhanced = ws.enhance_trace(&request(path, json!({}))).unwrap(); + assert_eq!(serde_json::to_value(enhanced.output).unwrap(), expected); + assert_eq!(enhanced.trace.unwrap().executions.len(), count); + } +} + +#[tokio::test] +async fn compiled_and_lazy_engines_agree_with_sibling_duplicate_names() { + for duplicate in [false, true] { + let docs = siblings(duplicate); + for precompile in [false, true] { + let loader = Arc::new(MemoryLoader::default()); + for (path, doc) in &docs { + loader.add( + *path, + serde_json::from_value::(doc.clone()) + .unwrap(), + ); + } + let engine = DecisionEngine::default().with_loader(loader); + if precompile { + assert!(engine.compile().is_empty()); + } + // Switching entries repeatedly must never reuse another entry's scope. + for path in ["B", "C", "A", "C", "B"] { + let result = engine.evaluate(path, json!({}).into()).await.unwrap(); + let result = serde_json::to_value(result.result).unwrap(); + let expected = match path { + "B" => json!({"base":10,"decision":"approve"}), + "C" if duplicate => json!({"base":10,"decision":"decline"}), + "C" => json!({"base":10,"other":"decline"}), + _ => json!({"base":10}), + }; + assert_eq!(result, expected, "precompile={precompile}, entry={path}"); + } + } + } +} + +#[test] +fn sibling_dictionaries_and_diagnostics_stay_separate() { + let ws = workspace(&siblings(true)); + for (path, values) in [ + ("A", vec![]), + ("B", vec!["approve"]), + ("C", vec!["decline"]), + ] { + let errors: Vec<_> = ws + .diagnostics(path) + .into_iter() + .filter(|d| d.severity == Severity::Error) + .collect(); + assert!(errors.is_empty(), "{path}: {errors:?}"); + let dictionaries = ws.dictionaries(&ScopeRequest::for_policy(path)); + let actual: Vec<_> = dictionaries + .iter() + .flat_map(|d| d.entries.iter().map(|e| e.value.as_ref())) + .collect(); + assert_eq!(actual, values); + } +} + +#[tokio::test] +async fn explicitly_composing_conflicting_siblings_reports_errors_at_the_entry() { + let mut docs = siblings(true); + docs.push(("D", policy(&["B", "C"], vec![]))); + let ws = workspace(&docs); + let diagnostics = ws.diagnostics("D"); + for code in ["DuplicateWriter", "DataModelCollision"] { + assert!( + diagnostics.iter().any(|d| format!("{:?}", d.code) == code), + "{diagnostics:?}" + ); + } + assert!(diagnostics + .iter() + .all(|d| d.location.policy_path.as_ref() == "D")); + for precompile in [false, true] { + let loader = Arc::new(MemoryLoader::default()); + for (path, doc) in &docs { + loader.add( + *path, + serde_json::from_value::(doc.clone()).unwrap(), + ); + } + let engine = DecisionEngine::default().with_loader(loader); + if precompile { + let failures = engine.compile(); + assert_eq!(failures.len(), 1); + assert_eq!(failures[0].key.as_ref(), "D"); + } + assert!(engine.evaluate("D", json!({}).into()).await.is_err()); + assert!(engine.evaluate("B", json!({}).into()).await.is_ok()); + assert!(engine.evaluate("C", json!({}).into()).await.is_ok()); + } +} + +#[test] +fn diamond_imports_execute_the_shared_dependency_once() { + let mut docs = siblings(false); + docs.push(( + "D", + policy(&["B", "C"], vec![expression("total", "base + 5")]), + )); + let ws = workspace(&docs); + let result = ws.enhance_trace(&request("D", json!({}))).unwrap(); + assert_eq!( + serde_json::to_value(result.output).unwrap(), + json!({"base":10,"decision":"approve","other":"decline","total":15}) + ); + let trace = result.trace.unwrap(); + assert_eq!(trace.executions.len(), 4); + assert_eq!( + trace + .executions + .iter() + .filter(|e| e.block_id.as_ref() == "base") + .count(), + 1 + ); +} + +#[test] +fn scope_updates_after_import_and_sibling_edits() { + let mut ws = workspace(&siblings(false)); + assert_eq!(output(&ws, "B"), json!({"base":10,"decision":"approve"})); + ws.set_policy( + "C", + serde_json::from_value(policy(&["A"], vec![expression("decision", "\"decline\"")])) + .unwrap(), + ); + assert_eq!(output(&ws, "B"), json!({"base":10,"decision":"approve"})); + ws.set_policy( + "B", + serde_json::from_value(policy(&[], vec![expression("decision", "\"approve\"")])).unwrap(), + ); + assert_eq!(output(&ws, "B"), json!({"decision":"approve"})); + ws.set_policy( + "B", + serde_json::from_value(policy(&["C"], vec![expression("more", "base + 1")])).unwrap(), + ); + assert_eq!( + output(&ws, "B"), + json!({"base":10,"decision":"decline","more":11}) + ); +} + +#[test] +fn unrelated_sibling_input_and_entity_scope_cannot_poison_evaluation() { + let docs = vec![ + ("shared", policy(&[], vec![])), + ( + "Test", + policy( + &["shared"], + vec![ + json!({"id":"dm", "type":"dataModel", "props":{"data":{"name":"person", "properties":[{"id":"age", "name":"age", "type":"number"}]}}}), + expression("decision", "age > 50"), + ], + ), + ), + ( + "underwriting", + policy( + &["shared"], + vec![ + json!({"id":"rule", "type":"decisionTable", "props":{"data":{ + "hitPolicy":"first", "inputs":[], "outputs":[ + {"id":"x", "name":"", "field":"person.result"}, + {"id":"y", "name":"", "field":"other.result"} + ], "rules":[{"_id":"r1","x":"1","y":"2"}] + }}}), + ], + ), + ), + ]; + let ws = workspace(&docs); + let errors: Vec<_> = ws + .diagnostics("underwriting") + .into_iter() + .filter(|d| d.severity == Severity::Error) + .collect(); + assert!(errors.is_empty(), "{errors:?}"); + assert_eq!( + output(&ws, "underwriting"), + json!({"person":{"result":1},"other":{"result":2}}) + ); + assert!(ws + .enhance_trace(&request("underwriting", json!({}))) + .is_ok()); +} diff --git a/core/engine/tests/policy_slot.rs b/core/engine/tests/policy_slot.rs index e05ac9e3..96b2a03b 100644 --- a/core/engine/tests/policy_slot.rs +++ b/core/engine/tests/policy_slot.rs @@ -1403,6 +1403,10 @@ fn graph_table_heads_complete_for_outputs_and_fieldless_inputs() { } fn sibling_writer_workspace() -> PolicyWorkspace { + import_scope_workspace(false) +} + +fn import_scope_workspace(import_rule: bool) -> PolicyWorkspace { let shared = json!({ "blocks": [ { @@ -1456,7 +1460,7 @@ fn sibling_writer_workspace() -> PolicyWorkspace { ] }); let entry = json!({ - "imports": ["shared"], + "imports": if import_rule { vec!["shared", "rule"] } else { vec!["shared"] }, "blocks": [ { "id": "ex_triggered", "type": "expression", "props": { "data": { "key": "triggeredRules", "value": "rules.apu.triggered ? [\"APU\"] : []" } } }, { "id": "ex_count", "type": "expression", "props": { "data": { "key": "triggeredCount", "value": "len(triggeredRules)" } } } @@ -1545,8 +1549,8 @@ fn sibling_policy_writes_are_hidden_until_scheduled() { } #[test] -fn upstream_policy_writes_stay_visible_downstream() { - let ws = sibling_writer_workspace(); +fn explicitly_imported_policy_writes_stay_visible_downstream() { + let ws = import_scope_workspace(true); let reader = cursor_in("entry", "ex_triggered", expression("ex_triggered")); let labels = completion_labels(&ws, &reader); diff --git a/core/engine/tests/policy_table.rs b/core/engine/tests/policy_table.rs index db006bff..fa135956 100644 --- a/core/engine/tests/policy_table.rs +++ b/core/engine/tests/policy_table.rs @@ -1,8 +1,8 @@ use serde_json::json; use std::sync::Arc; use zen_engine::policy::{ - CursorTarget, EngineEdit, EvaluateRequest, PolicyWorkspace, RenameTarget, ScopeRequest, - Severity, + CursorTarget, EngineEdit, EvaluateRequest, EvaluationError, PolicyWorkspace, RenameTarget, + ScopeRequest, Severity, }; use zen_expression::variable::{Variable, VariableType}; @@ -72,6 +72,53 @@ fn strict_table_doc() -> serde_json::Value { }) } +#[test] +fn failed_input_comparisons_report_the_field_and_missing_value() { + let ws = workspace_with(strict_table_doc()); + for (input, value_description) in [ + (json!({}), "missing or null"), + (json!({ "order": { "amount": null } }), "missing or null"), + ] { + let error = ws.evaluate(&request(input, true)).unwrap_err(); + let EvaluationError::ExpressionFailed { + block_id, + expression, + source, + .. + } = error + else { + panic!("expected a contextual expression error, got {error:?}"); + }; + assert_eq!(block_id.as_ref(), "dt"); + assert_eq!(expression.as_ref(), ">= 100"); + let serialized = serde_json::to_value(&source).unwrap(); + assert_eq!(serialized["type"], "contextError"); + assert_eq!(serialized["source"]["type"], "vmError"); + let source = source.to_string(); + assert!(source.contains("order.amount"), "{source}"); + assert!(source.contains(">= 100"), "{source}"); + assert!(source.contains(value_description), "{source}"); + assert!(source.contains("Opcode Compare"), "{source}"); + } + // A failed run must not poison the workspace or silently select a fallback rule. + assert_eq!( + evaluate_output(&ws, json!({ "order": { "amount": 100, "region": "US" } })) + .pointer("/order/shippingCost"), + Some(&json!(0)) + ); +} + +#[test] +fn a_condition_that_handles_missing_input_still_evaluates_normally() { + let mut doc = strict_table_doc(); + doc["blocks"][1]["props"]["data"]["rules"][0]["i1"] = json!("null"); + let ws = workspace_with(doc); + assert_eq!( + evaluate_output(&ws, json!({ "order": { "region": "US" } })).pointer("/order/shippingCost"), + Some(&json!(0)) + ); +} + #[test] fn no_match_emits_null_for_scalar_output() { let ws = workspace_with(strict_table_doc()); diff --git a/core/expression/src/isolate.rs b/core/expression/src/isolate.rs index c7c59b6e..a8d0ec31 100644 --- a/core/expression/src/isolate.rs +++ b/core/expression/src/isolate.rs @@ -197,6 +197,12 @@ impl Isolate { /// Errors which happen within isolate or during evaluation #[derive(Debug, Error)] pub enum IsolateError { + #[error("{context}: {source}")] + ContextError { + context: String, + source: Box, + }, + #[error("Lexer error: {source}")] LexerError { source: LexerError }, @@ -227,6 +233,11 @@ impl Serialize for IsolateError { let mut map = serializer.serialize_map(None)?; match &self { + IsolateError::ContextError { context, source } => { + map.serialize_entry("type", "contextError")?; + map.serialize_entry("context", context)?; + map.serialize_entry("source", source)?; + } IsolateError::ReferenceError => { map.serialize_entry("type", "referenceError")?; }