Repository navigation
fix(rvm): propagate strict rule errors and output conflicts - #934
Draft
Anand Krishnamoorthi (anakrish) wants to merge 2 commits into
Draft
Anand Krishnamoorthi (anakrish) wants to merge 2 commits into
Anand Krishnamoorthi (anakrish) wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a5b6b76-47ed-4d59-8b05-4cca089961b9
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical VM issues allow exponential error formatting, hide rule-body errors, and miss conflicting outputs.
3 open issues
What changed in this PR
Updates Regorus’s RVM error handling to address #932 and improve consistency with the interpreter, while preserving ordinary non-strict recovery.
Changes:
- Shares rule-error propagation logic and adds typed errors for conflicting complete-rule and function outputs.
- Records terminal failures across initial and resumed execution while supporting VM reuse.
- Preserves mixed-type relational ordering and expands differential and lifecycle coverage across all three execution paths.
| File | Description |
|---|---|
tests/value_walk_safety.rs |
Uses logical operations to test bounded error diagnostics. |
tests/value_serialization_limits.rs |
Updates malformed-option expectations for strict mode. |
tests/rvm/vm/suites/type_errors.yaml |
Expects mixed-type ordering in both strictness modes. |
tests/rvm/vm/suites/function_calls.yaml |
Expects errors for conflicting function outputs. |
tests/rvm/vm/suites/call_rule.yaml |
Covers conflicts and strict body/destructuring errors. |
tests/rvm/rego/mod.rs |
Adds three-mode execution and aligned interpreter strictness. |
tests/rvm/rego/cases/rule_errors.yaml |
Adds differential error, conflict, recovery, and ordering cases. |
tests/rvm/compiler.rs |
Tests typed conflicts, terminal failures, and VM reuse. |
tests/explanation_corpus/cases/rules.yaml |
Removes obsolete conflict-divergence expectations. |
tests/explanation_corpus/cases/calls_values_and_secrets.yaml |
Removes error divergences and aligns division-error matching. |
src/value/tests.rs |
Updates deep-value comparison and diagnostic checks. |
src/rvm/vm/state.rs |
Records terminal suspendable error states. |
src/rvm/vm/rules.rs |
Centralizes propagation, cleanup, and output-conflict handling. |
src/rvm/vm/execution.rs |
Applies shared propagation policy during suspendable execution. |
src/rvm/vm/execution_model.rs |
Removes obsolete conflict-tracking state. |
src/rvm/vm/errors.rs |
Adds VmError::RuleOutputConflict. |
src/rvm/vm/dispatch.rs |
Removes strict-mode mixed-type comparison rejection. |
src/rvm/vm/comparison.rs |
Expands ordering and undefined-propagation tests. |
docs/rvm/vm-runtime.md |
Documents propagation, terminal failures, and subsequent VM execution. |
Review effort: Max
Preserve constant-head witness short-circuiting without skipping fallible independent definitions, and keep complete/function yield comparisons resource-aware. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a5b6b76-47ed-4d59-8b05-4cca089961b9
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical defects allow cached programs and suspendable lookups to miss rule conflicts, while comparison traversal and error diagnostics remain insufficiently bounded by execution limits.
5 open issues
3 resolved since last review
Review effort: Max
| /// Assert not undefined — fail (return undefined) if register is undefined. | ||
| NotUndefined, | ||
| /// Assert a defined complete-rule output agrees with earlier yields in this body. | ||
| RuleOutput, |
Comment on lines
+109
to
+110
| if !self.needs_checked_comparison() { | ||
| return Ok(compare_unchecked(left, right, equality)); |
| | VmError::InvalidModulo { .. } | ||
| | VmError::InvalidIteration { .. } | ||
| | VmError::HostAwaitResponseMissing { .. } => core::fmt::Display::fmt(self.0, f), | ||
| _ => core::fmt::Debug::fmt(self.0, f), |
| // no need to verify consistency with the rest. | ||
| if rule_info.early_exit_on_first_success { | ||
| break 'outer; | ||
| match self.jump_to(*body_entry_point) { |
| // Returning Ok(false) causes the caller to clear execution_stack and | ||
| // propagate the error, terminating the evaluation entirely. | ||
| if self.is_fatal_vm_error(&err) { | ||
| if self.should_propagate_rule_error(&err) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #932.
elseshort-circuiting and the interpreter's legitimate first-witness shortcut within simple constant-head rule bodies; functions, variable heads, and inner comprehensions retain their distinct interpreter behavior.Non-strict recovery otherwise remains unchanged.
Validation
Fresh debug builds only, with at most two build jobs:
cargo test --locked --test mod -j2 rule_errors -- --nocapture: 2 tests passed; 119 YAML scenarios exercised all three VM paths (357 executions). Two non-strict nested-function-conflict scenarios explicitly document the interpreter's existing incorrect success behavior.--test-threads=1, including the actual bounded execution-state getter. Serial execution avoids interference between process-global memory-limit tests observed in an earlier parallel run. The existing nested-budget test now accumulates many bounded diagnostics rather than relying on one oversized diagnostic.cargo xtask ci-debug --frozenpassed withRUSTFLAGS='--force-warn deprecated_in_future', including all-features builds, no-default-feature tests, Azure RBAC tests, 2,875 OPA cases, and all six feature-combination checks.git diff --checkpassed.Unmodified Rust 1.97 debug CI remains blocked by two pre-existing
deprecated_in_futureerrors atsrc/builtins/azure_policy/template_functions_encoding.rs:177,182.cargo xtask clippyreports seven existing errors: those two deprecations and five unrelated Clippy findings. No source suppressions or unrelated lint fixes were added.Compatibility
Adds
VmError::RuleOutputConflictandGuardMode::RuleOutput; downstream exhaustive matches may need updating. The new guard variant is appended, preserving existing guard discriminants. Recompile previously serialized policies to obtain the new per-yield checks; legacyearly_exit_on_first_successmetadata remains serialized but is no longer honored at runtime.Diagnostic formatting summarizes containers instead of recursively expanding their contents. Non-value error-state variant names remain unchanged.
Follow-up implementation addresses the source-review findings and adds the missing regressions; this description no longer claims that the earlier review was clean. The PR remains a draft.