Repository navigation
Evaluate dynamic Rego defaults at runtime - #863
Maksym (maksym-mishchenko) wants to merge 1 commit into
Conversation
Compile valid dynamic defaults for runtime fallback while preserving indexed/default-function semantics, namespace/base-data projection, and suspendable virtual lookup. BREAKING CHANGE: RuleInfo struct literals require default_entry_point and default_is_indexed. Runtime/indexed-default artifacts require a version-7-capable reader. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved namespace compilation, dependency-cycle detection, and cross-mode fallback inconsistencies affect policy correctness.
4 open issues
What changed in this PR
This PR moves nonconstant Rego defaults from compile-time evaluation to runtime fallback execution in RVM, including HostAwait suspension and resume, while preserving legacy artifact compatibility.
Changes:
- Compile runtime defaults and support indexed-default object merging.
- Add resumable virtual-document traversal and execution-error cleanup.
- Introduce version 7 artifact metadata, migration documentation, and Rust/C# regression coverage.
| File | Description |
|---|---|
tests/rvm/rego/mod.rs |
Adds runtime-default, namespace, resume, and reuse regressions. |
tests/rvm/rego/cases/registered_host_await.yaml |
Tests registered HostAwait calls inside defaults. |
tests/rvm/rego/cases/imports.yaml |
Enables imported dynamic-default function coverage. |
tests/rvm/rego/cases/host_await.yaml |
Tests raw HostAwait fallback behavior. |
tests/rvm/rego/cases/default_rules.yaml |
Covers dynamic defaults and suppression conditions. |
tests/rvm/fixtures/e39-mark-frozen-dynamic-default-v6.json |
Adds a historical frozen-default fixture. |
tests/rvm/compiler.rs |
Tests artifact compatibility, validation, and compiler diagnostics. |
src/rvm/vm/virtual_data.rs |
Adds resumable lookups and empty-namespace preservation. |
src/rvm/vm/state.rs |
Cleans up failed executions and validates default metadata. |
src/rvm/vm/rules.rs |
Executes fallbacks and merges indexed defaults. |
src/rvm/vm/execution.rs |
Advances virtual lookups and handles strict-error precedence. |
src/rvm/vm/execution_model.rs |
Defines fallback phases and virtual-lookup frames. |
src/rvm/tests/vm.rs |
Initializes new metadata in test rules. |
src/rvm/program/types.rs |
Extends public rule metadata and constructors. |
src/rvm/program/serialization/json.rs |
Adds versioned JSON while retaining legacy output. |
src/rvm/program/serialization/binary.rs |
Supports version 7 and legacy version 6 layouts. |
src/rvm/program/rule_tree.rs |
Registers package namespaces without document rules. |
src/rvm/program/core.rs |
Validates default metadata and advances the format version. |
src/languages/rego/compiler/rules.rs |
Compiles default expressions and checks dependency ancestry. |
src/languages/rego/compiler/program.rs |
Attaches compiled-default metadata to programs. |
src/languages/rego/compiler/mod.rs |
Tracks literal defaults and fallback entry points. |
src/languages/rego/compiler/function_calls.rs |
Resolves qualified and imported default functions. |
src/languages/rego/compiler/core.rs |
Recognizes default functions and their arity. |
src/interpreter.rs |
Shares default validation and removes compile-time evaluation support. |
docs/rvm/vm-runtime.md |
Documents runtime semantics and artifact migration. |
CHANGELOG.md |
Records default support and compatibility changes. |
bindings/ffi/src/rvm.rs |
Adds the native string-result conversion helper. |
bindings/ffi/CHANGELOG.md |
Documents runtime-default support for native consumers. |
bindings/csharp/Regorus.Tests/RvmProgramTests.cs |
Tests runtime defaults and HostAwait through managed bindings. |
Review effort: Balanced
| let current_entry = | ||
| WorklistEntry::new(rule_path.to_string(), self.current_call_stack.clone()); | ||
| if current_entry.would_create_recursion(index) { |
| .default_rules | ||
| .get(rule_path) | ||
| .is_some_and(|defaults| defaults.iter().any(|(_, index)| index.is_some())); | ||
| rule_info.set_default_is_indexed(has_indexed_default); |
| if self.policy.inner.rule_paths.contains(rule_path) | ||
| && self.policy.inner.default_rules.contains_key(rule_path) | ||
| { |
| if matches!(frame_data.phase, RuleFramePhase::ExecutingDefault) { | ||
| if let Some(result) = self.registers.get_mut(usize::from(frame_data.result_reg)) { | ||
| *result = Value::Undefined; | ||
| } |
There was a problem hiding this comment.
OPA-compatibility clarification: please resolve this mismatch by propagating the strict error, not by suppressing it in both modes.
package probe
default value := [result | result := 1 / input.divisor]With input {"divisor":0} and strict builtin errors enabled, OPA must report an evaluation error. In the new run-to-completion fallback at src/rvm/vm/rules.rs:300-319, jump_to(default_entry_point) returns InvalidDivision, but Err(_) => {} discards it because only fatal resource errors are propagated. The result remains Undefined rather than surfacing the strict error. With strict errors disabled, the correct result is the empty comprehension [].
Please propagate strict builtin errors through fallback execution and assert an error explicitly in both VM modes. The new dynamic_default_errors_match_ordinary_expression_in_both_builtin_modes test compares against another RVM expression, so it can pass when both expressions swallow the same error; it is not an OPA oracle.
References: OPA strict builtin-error behavior and OPA propagating the default evaluation error. Source-derived finding, not an executed reproduction.
Anand Krishnamoorthi (anakrish)
left a comment
Anand Krishnamoorthi (anakrish)
left a comment
There was a problem hiding this comment.
OPA-compatibility review: the core runtime-default change is worth doing. OPA explicitly allows runtime-dependent comprehensions in default values, so freezing those values during compilation is a real correctness bug. However, the indexed-parent merge behavior and named default-function parameter binding need the corrections described in the individual comments. I am also adding OPA-specific error-propagation guidance to the existing fallback-error thread.
The reported OPA conformance pass does not by itself validate these RVM changes: tests/opa.rs includes defaultkeyword, functions, and refheads in OPA_TODO_FOLDERS, so their RVM checks are skipped by default unless OPA_UNSKIP_FOLDERS overrides that list. Please add explicit OPA-oracle coverage for the affected constructs and assert the expected outcome in both execution modes; interpreter/RVM agreement alone can preserve an existing Regorus incompatibility.
This review is based on the diff and OPA documentation/compiler/evaluator source; no builds, tests, or OPA evaluations were run.
| u32::try_from(self.program.instructions.len()).map_err(|_| CompilerError::General { | ||
| message: format!("default entry point for '{rule_path}' exceeds u32"), | ||
| })?; | ||
| let value_register = self.compile_rego_expr(value)?; |
There was a problem hiding this comment.
[P2] Bind named default-function parameters before compiling the fallback
The fallback starts a fresh scope and compiles value, but never declares/binds the names in args. For example:
package probe
default f(x) := [y | y := x]
result := f(7)OPA binds the default function's head arguments before evaluating its body, so result is [7]; references inside the comprehension are permitted. Here x has no parameter binding, so resolve_variable instead attempts to resolve a package rule such as data.probe.x. The fallback preparation in both VM paths also fills the register window with Undefined, discarding the actual argument values.
Please reserve and bind the default head parameters and preserve/reinitialize their actual values when entering the fallback in both execution modes. Add a regression that actually uses a named parameter inside the default comprehension; the new tests currently use ignored/unused parameters.
OPA source reference: default function argument setup and argument unification. Source-derived finding, not an executed reproduction.
| package probe | ||
| default values["constant"] := true | ||
| default values["runtime"] := [item | item := input.items[_]] | ||
| values := {"ordinary": "defined"} if input.include_ordinary |
There was a problem hiding this comment.
[P2] Do not use a conflicting complete-parent/child policy as an OPA compatibility regression
This policy combines child defaults values["constant"] / values["runtime"] with a complete parent definition values := {"ordinary": "defined"}. OPA rejects that combination at compilation: the complete parent data.probe.values conflicts with its child rules, regardless of whether input.include_ordinary is true. It does not evaluate this policy to the merged object asserted below.
Ground child defaults themselves are valid Rego. The incompatibility is treating them as generic missing-key overlays on a separately defined complete parent. The existing interpreter already accepting that form does not establish OPA compatibility; this test now locks the behavior into RVM and its serialized artifacts too.
Please use OPA-valid independent child definitions for the conformance regression and preserve per-document default selection. Either reject the complete-parent conflict or explicitly gate/document this merge as a Regorus extension rather than OPA semantics.
OPA source reference: ground parent/child conflict detection and conflict reporting. Source-derived finding, not an executed reproduction.
Summary
I compile nonconstant Rego defaults into RVM fallback code instead of evaluating them during compilation. Defaults can now depend on input, data and other rules, including HostAwait suspension and resume.
Compatibility
Programs requiring the new default metadata use binary version 7 and a versioned JSON envelope. Other programs retain binary version 6 and flat JSON. The new reader accepts legacy artifacts; old readers reject the new formats. Previously compiled defaults retain their historical frozen values until the policy source is recompiled.
Public
RuleInfogainsdefault_entry_pointanddefault_is_indexed. Constructors initialize them; downstream struct literals need the new fields. Migration details are documented indocs/rvm/vm-runtime.md.This PR targets main independently of #862 and does not use its v2 HostAwait APIs. The only shared product-source overlap is the identical seven-line native string-result helper required to build either change on current main. Numeric package versions, releases and deployment remain outside this PR.
Verification
The final namespace-preservation tree passed RVM tests 206/206 standalone and 209/209 with the separate HostAwait change; FFI HostAwait tests passed 18/18 and 20/20 respectively. Managed tests passed 113/113 with the loaded native DLL path and hash verified against the fresh release build. The populated-base-data regression also passed through the native API in both execution modes.
Broader local
cargo xtask ci-debug --frozenand embeddedthumbv7m-none-eabino_std gates passed before the last two-file namespace-preservation correction. Their receipts remain historical, not relabeled as new executions. The final tree also passed the full standardcargo xtask clippymatrix: all-targets/all-features, no-default-features and the FFI/Java/Python/WASM binding manifests.The published commit passed normal commit and pre-push hooks without bypass, including build, formatting, Clippy, documentation tests, embedded no_std checks and OPA conformance (2875 passed, 0 failed).
Regression coverage includes namespace/function/resume-error corrections, both execution modes, strict and non-strict behavior, allocator-budget failure and reuse. Actual compatibility checks cover historical v6 artifacts on the new reader and v7 binary/JSON rejected by the old e39 reader; the namespace correction does not change those formats.
Validation limitations remain explicit:
cargo check --no-default-features --features rvm,no_stdfails on current main and this change: 62 baseline diagnostics versus 61, with no new diagnostics. This is not a passing standalone no-arc check.cargo clippy --features rvm --test mod -- -D warningsinvocation fails onmissing_const_for_fnin the unchanged no-allocator helper insrc/rvm/vm/machine.rs. This is separate from the repository's standard Clippy matrix; no pass or lint suppression is claimed for that extra configuration.