test: Add RVM compiler testing to ACI tests (#509)

code fixes:
  - compiler: add `is_var_bound_in_current_scope` and use it in destructuring so
    only the innermost scope blocks rebinding while still catching duplicates
    within that block.
  - rvm: treat `not` over undefined operands as a successful negation to match
    interpreter semantics.

tests/aci:
   migrate YAML cases to `data.policy.rule` queries with `{x: …}`
  bindings, expand the harness to run interpreter plus RVM (with optional
  skipping), align results to the binding format, add readable timing output,
  and support a `--filter` flag for targeting cases.

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

src/languages/rego/compiler/core.rs

@@ -140,6 +140,14 @@ impl<'a> Compiler<'a> {
         }
     }
 
+    /// Returns true when a variable is already bound in the innermost scope
+    pub fn is_var_bound_in_current_scope(&self, var_name: &str) -> bool {
+        self.scopes
+            .last()
+            .map(|scope| scope.bound_vars.contains_key(var_name))
+            .unwrap_or(false)
+    }
+
     /// Look up a variable in all scopes starting from innermost (like interpreter's lookup_local_var)
     pub fn lookup_local_var(&self, var_name: &str) -> Option<Register> {
         self.scopes

src/languages/rego/compiler/destructuring.rs

@@ -289,7 +289,7 @@ impl<'a> Compiler<'a> {
             return Ok(());
         }
 
-        if self.lookup_local_var(var_name).is_some() {
+        if self.is_var_bound_in_current_scope(var_name) {
             bail!("Variable '{var_name}' already defined in current scope");
         }
 

src/rvm/vm/dispatch.rs

@@ -299,7 +299,10 @@ impl RegoVM {
                 let operand_value = &self.registers[operand as usize];
 
                 if operand_value == &Value::Undefined {
-                    self.registers[dest as usize] = Value::Undefined;
+                    // In Rego, `not expr` succeeds when `expr` has no results.
+                    // When the operand evaluates to undefined we should treat it as
+                    // a successful negation instead of propagating undefined.
+                    self.registers[dest as usize] = Value::Bool(true);
                     return Ok(InstructionOutcome::Continue);
                 }
 

tests/aci/aci.yaml

@@ -11,7 +11,7 @@ cases:
     target: /run/layers/p0-layer0
   data:
     metadata: {}
-  query: data.policy.mount_device=x
+  query: data.policy.mount_device
   want_result: 
   - x:
       allowed: true
@@ -44,7 +44,7 @@ cases:
         "/run/layers/p0-layer3": 41d64cdeb347bf236b4c13b7403b633ff11f1cf94dbc7cf881a44d6da88c5156
         "/run/layers/p0-layer4": 4dedae42847c704da891a28c25d32201a1ae440bce2aecccfa8e6f03b97a6a6c
         "/run/layers/p0-layer5": fe84c9d5bfddd07a2624d00333cf13c1a9c941f3a261f13ead44fc6a93bc0e7a
-  query: data.policy.mount_overlay=x
+  query: data.policy.mount_overlay
   want_result:
   - x:
       allowed: true
@@ -68,7 +68,7 @@ cases:
     target: /mnt/layer6
   data:
     metadata: {}
-  query: data.policy.scratch_mount=x
+  query: data.policy.scratch_mount
   want_result:
   - x:
       allowed: true
@@ -201,7 +201,7 @@ cases:
       scratch_mounts:
         "/mnt/layer6":
           encrypted: true
-  query: data.policy.create_container=x
+  query: data.policy.create_container
   want_result:
   - x:
       allow_stdio_access: false
@@ -257,7 +257,7 @@ cases:
       started:
         container0:
         - {"privileged": false}
-  query: data.policy.shutdown_container=x
+  query: data.policy.shutdown_container
   want_result:
   - x:
       allowed: true
@@ -286,7 +286,7 @@ cases:
       scratch_mounts:
         "/mnt/layer6":
           encrypted: true
-  query: data.policy.scratch_unmount=x
+  query: data.policy.scratch_unmount
   want_result:
   - x:
       allowed: true
@@ -313,7 +313,7 @@ cases:
       overlayTargets:
         "/run/gcs/c/container0/rootfs": true
       scratch_mounts: []
-  query: data.policy.unmount_overlay=x
+  query: data.policy.unmount_overlay
   want_result:
   - x:
       allowed: true
@@ -332,7 +332,7 @@ cases:
     metadata:
       devices:
         "/run/layers/p0-layer0": 1b80f120dbd88e4355d6241b519c3e25290215c469516b49dece9cf07175a766
-  query: data.policy.unmount_device=x
+  query: data.policy.unmount_device
   want_result:
   - x:
       allowed: true
@@ -378,7 +378,7 @@ cases:
     namespace: fragment
   data:
     metadata: {}
-  query: data.policy.load_fragment=x
+  query: data.policy.load_fragment
   want_result:
   - x: 
       add_module: false

tests/aci/main.rs

@@ -1,9 +1,13 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
+use regorus::languages::rego::compiler::Compiler;
+use regorus::rvm::tests::test_utils::test_round_trip_serialization;
+use regorus::rvm::vm::RegoVM;
 use regorus::*;
 
+use std::collections::BTreeMap;
 use std::path::Path;
-use std::time::Instant;
+use std::time::{Duration, Instant};
 
 use anyhow::Result;
 use clap::Parser;
@@ -18,6 +22,8 @@ struct TestCase {
     modules: Vec<String>,
     query: String,
     want_result: Value,
+    #[serde(default)]
+    skip_rvm: bool,
 }
 
 #[derive(Serialize, Deserialize, PartialEq, Debug)]
@@ -25,7 +31,46 @@ struct YamlTest {
     cases: Vec<TestCase>,
 }
 
-fn eval_test_case(dir: &Path, case: &TestCase) -> Result<Value> {
+fn format_duration(duration: Duration) -> String {
+    if duration.as_secs_f64() >= 1.0 {
+        format!("{:.3}s", duration.as_secs_f64())
+    } else if duration.as_millis() >= 1 {
+        format!("{:.3}ms", duration.as_secs_f64() * 1_000.0)
+    } else if duration.as_micros() >= 1 {
+        format!("{:.0}us", duration.as_secs_f64() * 1_000_000.0)
+    } else {
+        format!("{}ns", duration.as_nanos())
+    }
+}
+
+fn is_binding_array(value: &Value) -> bool {
+    match value {
+        Value::Array(entries) => {
+            if entries.is_empty() {
+                return true;
+            }
+
+            let binding_key = Value::from("x");
+            entries.iter().all(|entry| match entry {
+                Value::Object(fields) => fields.contains_key(&binding_key),
+                _ => false,
+            })
+        }
+        _ => false,
+    }
+}
+
+fn align_result_with_expected(result: Value, expected: &Value) -> Value {
+    if is_binding_array(expected) && !is_binding_array(&result) {
+        let mut binding = BTreeMap::new();
+        binding.insert(Value::from("x"), result);
+        return Value::from(vec![Value::from(binding)]);
+    }
+
+    result
+}
+
+fn setup_engine(dir: &Path, case: &TestCase) -> Result<Engine> {
     let mut engine = Engine::new();
     engine.set_rego_v0(true);
 
@@ -42,29 +87,76 @@ fn eval_test_case(dir: &Path, case: &TestCase) -> Result<Value> {
         }
     }
 
-    let mut engine_full = engine.clone();
-    let query_results = engine.eval_query(case.query.clone(), true)?;
-
-    // Ensure that full evaluation produces the same results.
-    let query_results_full = engine_full.eval_query_and_all_rules(case.query.clone(), true)?;
-    assert_eq!(query_results, query_results_full);
-
-    let mut values = vec![];
-    for qr in query_results.result {
-        values.push(if !qr.bindings.as_object()?.is_empty() {
-            qr.bindings.clone()
-        } else if let Some(v) = qr.expressions.last() {
-            v.value.clone()
-        } else {
-            Value::Undefined
-        });
-    }
-    let result = Value::from(values);
-    // Make result json compatible. (E.g: avoid sets).
-    Value::from_json_str(&result.to_string())
+    Ok(engine)
 }
 
-fn run_aci_tests(dir: &Path) -> Result<()> {
+fn eval_test_case_interpreter(dir: &Path, case: &TestCase) -> Result<(Value, Duration)> {
+    let setup_start = Instant::now();
+    let mut engine = setup_engine(dir, case)?;
+    let setup_duration = setup_start.elapsed();
+
+    // Use eval_rule instead of eval_query since we're evaluating specific rules
+    let result = engine.eval_rule(case.query.clone())?;
+
+    // Make result json compatible. (E.g: avoid sets).
+    let value = Value::from_json_str(&result.to_string())?;
+    Ok((value, setup_duration))
+}
+
+fn eval_test_case_rvm(
+    dir: &Path,
+    case: &TestCase,
+) -> Result<(Value, Duration, Duration, Duration)> {
+    let setup_start = Instant::now();
+    let mut engine = setup_engine(dir, case)?;
+
+    // Convert input and data for RVM
+    let input = case.input.clone();
+    let data = case.data.clone();
+
+    // Create CompiledPolicy first (needed for RVM compiler)
+    let rule = Rc::from(case.query.as_str());
+    let compiled_policy = engine.compile_with_entrypoint(&rule)?;
+    let setup_and_compile_duration = setup_start.elapsed();
+
+    // Use RVM compiler to create a program
+    let compile_start = Instant::now();
+    let program = Compiler::compile_from_policy(&compiled_policy, &[&case.query])?;
+
+    let compile_duration = compile_start.elapsed();
+
+    // Test round-trip serialization
+    test_round_trip_serialization(program.as_ref()).map_err(|e| {
+        anyhow::anyhow!(
+            "Round-trip serialization test failed for case '{}': {}",
+            case.note,
+            e
+        )
+    })?;
+
+    // Create RVM and load the program
+    let mut vm = RegoVM::new();
+    vm.load_program(program);
+    vm.set_data(data)?;
+    vm.set_input(input);
+
+    // Execute on RVM
+    let execution_start = Instant::now();
+    let result = vm.execute()?;
+    let execution_duration = execution_start.elapsed();
+
+    // Make result json compatible. (E.g: avoid sets).
+    let value = Value::from_json_str(&result.to_string())?;
+
+    Ok((
+        value,
+        setup_and_compile_duration,
+        compile_duration,
+        execution_duration,
+    ))
+}
+
+fn run_aci_tests(dir: &Path, filter: Option<&str>) -> Result<()> {
     let mut nfailures = 0;
     for entry in WalkDir::new(dir)
         .sort_by_file_name()
@@ -81,32 +173,79 @@ fn run_aci_tests(dir: &Path) -> Result<()> {
         let test: YamlTest = serde_yaml::from_str(&yaml)?;
 
         for case in &test.cases {
-            print!("{:50}", case.note);
-            let start = Instant::now();
-            let results = eval_test_case(dir, case);
-            let duration = start.elapsed();
-
-            match results {
-                Ok(actual) if actual == case.want_result => {
-                    println!("passed    {:?}", duration);
-                }
-                Ok(actual) => {
-                    println!(
-                        "DIFF {}",
-                        prettydiff::diff_chars(
-                            &serde_yaml::to_string(&case.want_result)?,
-                            &serde_yaml::to_string(&actual)?
-                        )
-                    );
-
-                    nfailures += 1;
-                }
-                Err(e) => {
-                    println!("failed    {:?}", duration);
-                    println!("{e}");
-                    nfailures += 1;
+            // Apply filter if specified
+            if let Some(filter_str) = filter {
+                if !case.note.contains(filter_str) {
+                    continue;
                 }
             }
+
+            print!("{:50}", case.note);
+
+            // Test with interpreter
+            let start = Instant::now();
+            let (interpreter_raw, interpreter_setup_duration) =
+                eval_test_case_interpreter(dir, case)?;
+            let interpreter_results =
+                align_result_with_expected(interpreter_raw, &case.want_result);
+            let interpreter_duration = start.elapsed();
+
+            if interpreter_results != case.want_result {
+                println!(
+                    "INTERPRETER DIFF {}",
+                    prettydiff::diff_chars(
+                        &serde_yaml::to_string(&case.want_result)?,
+                        &serde_yaml::to_string(&interpreter_results)?
+                    )
+                );
+                nfailures += 1;
+                continue;
+            }
+            if case.skip_rvm {
+                println!("skipped rvm");
+                continue;
+            }
+
+            // Test with RVM
+            let start = Instant::now();
+            let (rvm_results, setup_and_compile_duration, rvm_compile_duration, execution_duration) =
+                eval_test_case_rvm(dir, case)?;
+            let rvm_results = align_result_with_expected(rvm_results, &case.want_result);
+            let rvm_duration = start.elapsed();
+
+            if interpreter_results != rvm_results {
+                println!("INTERPRETER RESULT:");
+                println!("{}", serde_yaml::to_string(&interpreter_results)?);
+                println!("RVM RESULT:");
+                println!("{}", serde_yaml::to_string(&rvm_results)?);
+                println!(
+                    "INTERPRETER vs RVM DIFF {}",
+                    prettydiff::diff_chars(
+                        &serde_yaml::to_string(&interpreter_results)?,
+                        &serde_yaml::to_string(&rvm_results)?
+                    )
+                );
+                nfailures += 1;
+                continue;
+            }
+
+            println!("ok");
+
+            let interp_total = format_duration(interpreter_duration);
+            let interp_setup = format_duration(interpreter_setup_duration);
+            let rvm_total = format_duration(rvm_duration);
+            let rvm_prep = format_duration(setup_and_compile_duration);
+            let rvm_compile = format_duration(rvm_compile_duration);
+            let rvm_run = format_duration(execution_duration);
+
+            println!(
+                "    Interp total {:>10} (setup {:>10})",
+                interp_total, interp_setup,
+            );
+            println!(
+                "    RVM    total {:>10} (prep {:>10}, compile {:>10}, run {:>10})",
+                rvm_total, rvm_prep, rvm_compile, rvm_run,
+            );
         }
     }
     assert!(nfailures == 0);
@@ -158,8 +297,8 @@ fn run_aci_tests_coverage(dir: &Path) -> Result<()> {
         }
     }
 
-    let report = engine.get_coverage_report()?;
-    println!("{}", report.to_string_pretty()?);
+    //let report = engine.get_coverage_report()?;
+    //println!("{}", report.to_string_pretty()?);
 
     Ok(())
 }
@@ -171,6 +310,10 @@ struct Cli {
     #[arg(long, short)]
     #[clap(default_value = "tests/aci")]
     test_dir: String,
+
+    /// Filter to run only specific test cases (by note field, e.g., "create_container")
+    #[arg(long, short)]
+    filter: Option<String>,
 }
 
 fn main() -> Result<()> {
@@ -179,5 +322,5 @@ fn main() -> Result<()> {
     #[cfg(feature = "coverage")]
     run_aci_tests_coverage(&Path::new(&cli.test_dir))?;
 
-    run_aci_tests(&Path::new(&cli.test_dir))
+    run_aci_tests(&Path::new(&cli.test_dir), cli.filter.as_deref())
 }