Skip to content

Fix keyed partial-set rule accumulation in RVM - #851

Open
snehabandla wants to merge 15 commits into
microsoft:mainfrom
snehabandla:fix/rvm-keyed-partial-sets
Open

snehabandla wants to merge 15 commits into
microsoft:mainfrom
snehabandla:fix/rvm-keyed-partial-sets

Conversation

@snehabandla

Copy link
Copy Markdown
Contributor

Summary

Fix keyed partial-set rule accumulation in RVM. Keyed contains rules currently compile as flat sets, dropping their object keys.

package test

ports[host] contains port if {
  some host, values in {"web": [80, 443, 80], "db": [5432]}
  some port in values
}

For data.test.ports, the interpreter returns {"web": {80, 443}, "db": {5432}}, while the existing RVM returns {80, 443, 5432}.

Changes

  • Classify keyed contains rules as partial objects and preserve keys during accumulation.
  • Add a three-register ObjectSetAdd instruction for set-valued object fields. Omit undefined keys/values, preserve defined false/null values, and retain errors for invalid object/set targets.
  • Share head-key loop planning with the interpreter and fix coupled interpreter cases where missing contributions erase earlier results or empty keyed rules return undefined instead of {}.
  • Preserve static string rule prefixes and legacy Rego set heads.
  • Update instruction display, listing, parsing, and documentation, with serialization round-trip coverage through the existing harness, 18 Rego regression cases, and five bytecode regression cases covering both VM execution modes.

Prerequisites and scope

  • The prerequisite loop compiler fix (Fix RVM compiler panic in hoisted rule-head loops #850) and undefined-set fix (Prevent undefined values from entering RVM sets #841) are merged. No unmerged prerequisites.
  • No explanation or partial-evaluation functionality, provenance metadata, tracing instrumentation, bindings, CLI options, WASM, or demo changes are introduced.
  • Append the new opcode to preserve existing instruction discriminants; no new instruction parameter table is needed, and the instruction-size assertion is retained.
  • A general deep-object setter and multiple dynamic key levels are excluded; multiple dynamic key levels remain explicitly unsupported by the compiler.

Previously reported validation

The implementation's validation report records reproducing the mismatch before the fix and passing:

  • cargo +1.95.0 test --test mod keyed_partial_sets -- --nocapture: 18 cases.
  • cargo +1.95.0 test --lib keyed_set -- --nocapture: five bytecode cases through the existing multi-mode runner.
  • cargo +1.95.0 xtask ci-debug: all-features build, default and no-default-features tests, OPA fixtures, and feature-matrix checks.

Rust 1.95.0 matches the repository CI toolchain. These checks have not been rerun as part of this upstream publication.

snehabandla and others added 2 commits October 19, 2026 22:18
Compile keyed contains rules as partial objects and accumulate each leaf with ObjectSetAdd. Preserve interpreter parity for head-key loops, missing contributions, and empty results. Keep existing instruction discriminants and parameter tables unchanged.

Depends on the hoisted output-loop compiler fix. No explanation or partial-evaluation functionality is introduced.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leave static and legacy set-head planning unchanged and cover explicit rejection of multiple dynamic key levels. Document the narrow three-register accumulation instruction.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Classify literal set heads relative to the compiled document, reject mixed rule kinds, and hoist bodyless and empty-query head loops. Treat missing interpreter output collections as empty iterations.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved compilation failures, interpreter/RVM result differences, and target-validation gaps remain.

4 open issues
What changed in this PR

Fixes keyed partial-set accumulation in RVM so Rego rules preserve object keys rather than producing flat sets.

Changes:

  • Adds ObjectSetAdd and compiler support for keyed set contributions.
  • Updates interpreter accumulation and head-key loop planning.
  • Adds regression coverage and instruction documentation.
File Description
tests/​rvm/​vm/​suites/​keyed_set_operations.yaml Adds five instruction regression cases.
tests/​rvm/​rego/​cases/​keyed_partial_sets.yaml Adds 18 Rego regression cases.
src/​rvm/​vm/​dispatch.rs Implements keyed set insertion.
src/​rvm/​tests/​instruction_parser.rs Parses the new instruction.
src/​rvm/​program/​listing.rs Formats instruction listings.
src/​rvm/​instructions/​mod.rs Appends the new opcode.
src/​rvm/​instructions/​display.rs Adds instruction display formatting.
src/​languages/​rego/​compiler/​rules.rs Classifies and extracts keyed contributions.
src/​languages/​rego/​compiler/​queries.rs Emits keyed set accumulation.
src/​languages/​rego/​compiler/​mod.rs Tracks multi-value contexts.
src/​languages/​rego/​compiler/​loops.rs Initializes the new context field.
src/​languages/​rego/​compiler/​comprehensions.rs Initializes the new context field.
src/​interpreter.rs Adjusts keyed-rule accumulation and initialization.
src/​compiler/​hoist.rs Includes keyed heads in loop planning.
docs/​rvm/​instruction-set.md Documents ObjectSetAdd behavior.

Review effort: Balanced

Comment thread src/interpreter.rs
Comment thread src/languages/rego/compiler/rules.rs Outdated
Comment thread src/languages/rego/compiler/rules.rs Outdated
Comment thread src/rvm/vm/dispatch.rs
Do not create interpreter leaf sets when keyed rule bodies fail. Validate existing ObjectSetAdd leaves even when the incoming value is undefined, without creating absent leaves.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
snehabandla and others added 3 commits October 19, 2026 23:18
Reject unsupported keyed-set prefixes rather than dropping path components. Preserve compiler errors during rule reference resolution, reset head-loop hoisting per body, and skip scalar output-loop collections consistently.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Plan output keys and values against an evolving context. Apply interpreter loop-index binding plans in iteration-local scopes. Seed static set leaves once before evaluating rule bodies, preserving empty declarations and prior contributions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve upstream query-local hoisting when resolving shared loop-index binding plans.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Shared loop planning still has an unresolved cross-body defect, and changes spanning both evaluators require human review and fresh validation.

1 open issue
4 resolved since last review

Review effort: Balanced

Comment thread src/compiler/hoist.rs Outdated
Store body-scoped output loops and binding plans and select them in both evaluators. Clear stale compiler loop-register mappings between bodies and align suspendable partial-rule accumulation with run-to-completion execution.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@anakrish

Copy link
Copy Markdown
Collaborator

High: static child reads still omit overlapping dynamic keyed-set contributions

Rechecked against current head 21632f2fc846276711b932d3ef27a87ec7b4e324; this finding still applies.

package test

values.a contains 1
values[k] contains 2 if { k := "a" }
main := values.a

data.test.main should return {1, 2}, but the current compilation path selects the independently compiled data.test.values.a and returns only {1}. Compiling data.test.values.a directly as an entrypoint has the same omission, although evaluating the parent data.test.values combines both contributions.

The local resolver in references.rs:311-326 still chooses the longest registered rule path; the external resolver at lines 165-179 does likewise. Registration puts the static declaration under both parent and child, but the dynamic declaration only under the parent. compile_worklist_rule uses only rules.get(rule_path), so the selected child never receives the dynamic ancestor's contribution. The newer body-specific head-plan and VM body-iteration changes do not change that definition selection.

Please resolve affected child references/entrypoints through an applicable aggregate ancestor and index the requested suffix, or otherwise include the ancestor's applicable contributions without introducing recursion. Add overlapping-key regression cases querying both data.test.main and data.test.values.a, each expecting set!: [1, 2]; the existing child case with dynamic key "b" does not exercise overlap.

This is an incomplete case of the keyed-set support addressed by this PR, rather than a claim that longest-prefix resolution was newly introduced. Verification here is static source tracing; no builds or tests were run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Shared policy-evaluation changes need human validation, and static-child lookups still omit dynamic keyed contributions.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Static child lookups miss dynamic same-key contributions

src/​languages/​rego/​compiler/​references.rs:326

Static-child lookups omit dynamic contributions to the same key. For values.a contains 1, values[k] contains 2 if { k := "a" }, and main := [values, values.a], the parent contains a: {1, 2}, but the direct child lookup returns only {1}. The longest-prefix lookup selects the separately registered values.a rule, whose definitions exclude the dynamic parent rule; direct child entrypoints have the same problem. Resolve these accesses through the keyed parent and then index the child. Add same-key child-reference and entrypoint regressions in both execution modes; the existing child-entrypoint test uses different keys and misses this case.

1 resolved since last review

Review effort: Balanced

Share lookup-only child definition expansions between evaluators and filter projected dynamic contributions by the requested key. Preserve declaration-based override suppression and avoid whole-parent calls that introduce sibling recursion. Resolve each compiled definition in its declaring package.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@snehabandla

Copy link
Copy Markdown
Contributor Author

Fixed in 4f283f6: static-child references and entrypoints now include matching dynamic keyed-set contributions in both evaluators without introducing sibling recursion.

@snehabandla

Copy link
Copy Markdown
Contributor Author

High: static child reads still omit overlapping dynamic keyed-set contributions

Rechecked against current head 21632f2fc846276711b932d3ef27a87ec7b4e324; this finding still applies.

package test

values.a contains 1
values[k] contains 2 if { k := "a" }
main := values.a
data.test.main should return {1, 2}, but the current compilation path selects the independently compiled data.test.values.a and returns only {1}. Compiling data.test.values.a directly as an entrypoint has the same omission, although evaluating the parent data.test.values combines both contributions.

The local resolver in references.rs:311-326 still chooses the longest registered rule path; the external resolver at lines 165-179 does likewise. Registration puts the static declaration under both parent and child, but the dynamic declaration only under the parent. compile_worklist_rule uses only rules.get(rule_path), so the selected child never receives the dynamic ancestor's contribution. The newer body-specific head-plan and VM body-iteration changes do not change that definition selection.

Please resolve affected child references/entrypoints through an applicable aggregate ancestor and index the requested suffix, or otherwise include the ancestor's applicable contributions without introducing recursion. Add overlapping-key regression cases querying both data.test.main and data.test.values.a, each expecting set!: [1, 2]; the existing child case with dynamic key "b" does not exercise overlap.

This is an incomplete case of the keyed-set support addressed by this PR, rather than a claim that longest-prefix resolution was newly introduced. Verification here is static source tracing; no builds or tests were run.

Fixed in 4f283f6: static-child references and direct entrypoints now include matching dynamic keyed-set contributions in both evaluators. The fix projects the applicable contributions rather than evaluating the whole parent, avoiding sibling recursion.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cross-cutting evaluation changes need human validation, with unresolved recursion and memory-growth concerns in child-rule expansion.

2 open issues

Review effort: Balanced

Comment thread src/interpreter.rs Outdated
Comment thread src/interpreter.rs Outdated
Index dynamic definitions once per keyed parent and merge them lazily for static child lookups, avoiding per-child copies. Reject nonmatching projected keys before dependent body expressions when a constant key binding or some-in binding is available.

Add regressions for nonmatching child dependencies in both execution modes, including reversed statement order and some-in bindings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Child filtering can still omit valid contributions or trigger false recursion in the interpreter and RVM.

4 open issues
2 resolved since last review

Review effort: Balanced

Comment thread src/interpreter.rs Outdated
Comment thread src/interpreter.rs
Comment thread src/languages/rego/compiler/queries.rs
Comment thread src/languages/rego/compiler/rules.rs
Defer child filters until declared keys are defined and check hoisted bindings before dependent statements. Track interpreter recursion per projection, reset compiler guard state per body, and propagate genuine projected cycles as typed runtime errors in both VM modes using the existing recursion-check flag.

Cover unification, parent and sibling dependencies, indexed assignments, lookup order, genuine cycles, and the unsupported multi-body set parser boundary. Review the patch and cumulative implementation locally before committing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Child projection filtering still permits incorrect sibling updates and false recursion.

2 open issues
2 previously missed issues

In code that hasn't changed since last review

Medium severity Prioritize scalar unification bindings before dependency evaluation

src/​compiler/​hoist.rs:52

Scalar = bindings are excluded from prioritization. With values.a contains 1 and values[z] contains n if { n := count(values.a); z = "b" }, querying data.test.values.a evaluates the dependency before binding/filtering z, causing false recursion instead of returning {1} in both engines. Accept scalar unification here as well as :=, retaining the existing dependency checks, and add a regression with the dependency preceding the unification.

Medium severity Enforce output-key filtering to prevent projection leaks and recursion

src/​interpreter.rs:2416

Non-variable head keys bypass this filter, and the interpreter has no final output-key check. With values.a contains 1, values[input.key] contains 2, and input {"key":"b"}, evaluating [data.test.values.a, data.test.values.b] with data.test.values.b as {9} lets the a projection modify the overridden sibling, producing [{1}, {2,9}] instead of [{1}, {9}]. A dependent output such as values[input.key] contains count(values.a) instead re-enters the active projection and reports false recursion. Filter resolvable keys before dependent body/output evaluation, and enforce the requested key at output accumulation for every expression shape, including bodyless rules and head loops. Add regressions for both override isolation and nonmatching dependencies.

3 resolved since last review

Review effort: Balanced

Comment thread src/languages/rego/compiler/queries.rs Outdated
Share conservative key readiness across evaluators, filter each output-loop level, and fence actual projected output keys before values and accumulation. Prioritize scalar unification bindings in either operand order.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved projection-state and scheduling issues affect recursion correctness, while repeated child evaluation adds avoidable work.

1 open issue
3 previously missed issues

In code that hasn't changed since last review

Medium severity Prioritize variables used in compound key expressions

src/​compiler/​hoist.rs:70

Restricting prioritization to a bare variable misses bindings needed by compound keys. With values.a contains 1, values[input.keys[z]] contains n if { n := count(values.a); z := "chosen" }, and input {"keys":{"chosen":"b"}}, querying data.test.values.a reaches the dependent lookup before binding z. Both evaluators report recursion instead of returning {1}, even though this definition only contributes to b. Extend safe binding prioritization to variables used inside key expressions, respecting their dependencies, and add interpreter-parity regressions for both VM modes.

Medium severity Honor completion markers for projected child paths

src/​interpreter.rs:3391

Removing the projected definition from processed exposes repeated evaluation of completed local child lookups. lookup_var checks completion for the bare root values, not values.a, and ensure_rule_evaluated does not check the child marker. With values.a contains 1, values[k] contains 2 if { some k in input.keys }, and main := [values.a | some row in input.rows], each row rescans all keys: O(rows × keys) work for an unchanged child result. Honor completed child paths before reevaluating projections, or cache completion by (rule, key). Do not retain the global rule marker, since that would suppress other children's contributions.

Medium severity Evaluate body-independent projected head-key loops early

src/​languages/​rego/​compiler/​queries.rs:72

A projected head-key loop is still evaluated too late when the body depends on the requested child. For values.a contains 1 and values[input.keys[_]] contains n if { n := count(values.a) }, input {"keys":["b"]} should leave data.test.values.a equal to {1}. The early guard defers because _ is unbound, but the head loop runs only after the body. Both RVM and the interpreter therefore re-enter the same projection and report recursion before discovering that its key cannot match. Schedule and filter body-independent head-key loops before dependent body statements in both evaluators, and cover this case in both VM modes.

2 resolved since last review

Review effort: Balanced

Comment thread src/interpreter.rs
Comment on lines +2433 to +2435
if !crate::compiler::hoist::key_expr_is_ready(
&key_expr,
|expr| self.get_loop_var_value(expr).map(|value| value.is_some()),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed in the follow-up deep review at 6c9551d — High severity. The same parent/child trace survives adversarial checking:

package test
values.a contains 1
values[input.keys[_]] contains count(data.test.values.a)

Input {"keys":["b","a"]}, query data.test.values: the parent caches head value "b"; the nested "a" projection inherits that module/expression binding and rejects the dynamic contribution as nonmatching. The child is then marked processed, so the later parent "a" iteration reads the incomplete cached result. Source tracing yields {"a":{1},"b":{1}} in the interpreter instead of the genuine recursion error reached by RVM.

The projection-aware (rule, key_filter) recursion identity intentionally permits entering the child projection; it does not isolate loop_var_values. eval_rule_with_key_filter saves/restores scopes and module context but not those hoisted-expression bindings. Please make them invocation-local and restore the caller's bindings on both successful and failing exits. A parent-entrypoint regression with this input order is needed in addition to existing locally bound-key cycle cases.

No local builds or tests were run; this confirms the source-level finding without claiming an executed reproducer.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review at 6c9551d: the original static-child missing-contribution issue is fixed. Two additional projection-filtering issues are detailed inline. The separate high-severity invocation-cache issue is confirmed in the existing thread: #851 (comment). These findings were source-traced and adversarially checked; no local builds or tests were run.

Comment thread src/compiler/hoist.rs Outdated
Expr::ArrayCompr { .. }
| Expr::SetCompr { .. }
| Expr::ObjectCompr { .. }
| Expr::Call { .. }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium: deferring every call-valued key allows false recursion before a nonmatching projection is filtered

For example:

package test
values.a contains 1
values[lower("B")] contains n if { n := count(values.a) }

Querying data.test.values.a should return {1}: the dynamic definition contributes only to "b". This branch marks even the dependency-ready pure lower("B") call unready, so both evaluators enter the dependent body and recursively read values.a before rejecting the key.

The value-loop variant also reaches recursion before the final output-key guard:

values[concat("", ["b"])] contains [count(data.test.values.a)][_]

hoist_loops_and_emit_context_yield concatenates key and value loops, so the value-loop collection reads the child before emit_context_yield compares the actual key. Please stage actual key matching after key enumeration but before value-only loop collections, and allow safely evaluable dependency-ready key computations before dependent body statements. Add both examples in the interpreter and both VM modes, plus matching-key counterparts that must still reject genuine recursion.

Verification is static source tracing at 6c9551d, not an executed reproducer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e757d00: safe literal lower/concat keys can be filtered early, and projected key matching is staged before value-only loops.

Comment thread src/interpreter.rs
)? {
return Ok(false);
}
Ok(self.eval_expr(&key_expr)? != filter)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium: speculative head arithmetic executes before its false body guard

package test
values.a contains 1
values[input.port + 1] contains 2 if { is_number(input.port) }

With input {"port":"invalid"}, querying data.test.values.a should return {1} because the body rejects the dynamic contribution without evaluating its head. Instead, readiness accepts the available input dependency and this eval_expr runs before the first body statement, producing a numeric-argument error. Normal parent evaluation runs is_number first and avoids the arithmetic. Arithmetic remains an arithmetic AST node, and numeric validation fails before strict-builtin-error handling, so disabling strict builtin errors does not avoid this interpreter failure.

Please distinguish dependency availability from safety of speculative evaluation. Defer ordinary head-expression failures until body success, while continuing to propagate fatal resource-limit failures. Add a child-lookup regression with the invalid guarded input and successful/matching controls; the existing false-body cases use harmless head keys.

Verification is static source tracing at 6c9551d, not an executed reproducer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e757d00: speculative key readiness now defers arithmetic expressions so body guards run before potentially failing head arithmetic.

@anakrish

Copy link
Copy Markdown
Collaborator

Proposed keyed-partial-set regression gates

Scope: source-only test design against 6c9551d728997f17ff6c37d5683e44f174712f79; none of these proposed tests has been executed. No production changes are proposed here.

The original contribution-loss regression is already covered by overlapping_keyed_set_local_child and overlapping_keyed_set_child_entrypoint, including suspendable variants. Preserve those tests. The remaining P0 approval gates correspond to the published findings: invocation-local loop state, deferred call-key filtering, and guarded arithmetic.

Coverage matrix: extend rather than duplicate

Policies below use package test; values.a contains 1 is the static baseline unless stated otherwise. Sets in expected YAML use the actual set! fixture encoding.

Priority / status / placement Named case and concrete addition Required oracle
P0 New, Rego YAML + engine Rust keyed_set_parent_loop_reentrant_cycle: values[input.keys[_]] contains count(data.test.values.a); input {"keys":["b","a"]}; query data.test.values Genuine recursion error, not {a: {1}, b: {1}}. Repeat ["a","b"], direct child data.test.values.a, and reversed declaration order.
P0 Extend, same Same policy, {"keys":["b"]}, {"keys":["b","c"]} and {"keys":[]}; parent then child and child then parent Parent has a: {set!: [1]} plus exactly the supplied sibling keys, each {set!: [1]}; child always {set!: [1]}. Repeat after an error on the same engine/compiled policy/VM.
P0 New, Rego YAML keyed_set_call_key_body_dependency and keyed_set_call_key_value_loop: exact fixtures below Nonmatching "b" returns {1}; matching "a" produces genuine recursion. Both controls are necessary: suppressing all recursive calls is not a fix.
P0 Extend, Rego YAML + engine Rust keyed_set_guarded_arithmetic: values[input.port + 1] contains 2 if { is_number(input.port) }; invalid, missing, then numeric input Invalid/missing: child {1}, parent {a: {1}}; port 80: parent additionally has numeric key 81: {2}. Successful-body invalid arithmetic must still error.
P1 Extend, Rego YAML Original overlap policy plus main := [values.a, values, values.a]; query data.test.main [{1,2}, {a: {1,2}}, {1,2}]; add one multi-entrypoint fixture registering parent, child and main together. Existing lookup-order tests cover related, but different, policies.
P1 New, Rego YAML Comprehension key: values[[k | some k in ["b"]][_]] contains count(values.a); query child a; counterpart replaces "b" with "a" {1} versus recursion. Exercise deferred comprehension keys separately from builtin calls.
P1 New, Rego YAML values[lower("A")] contains 2; query data.test.values.a, then parent {set!: [1,2]} and {a: {set!: [1,2]}}: ready call keys must contribute successfully, not only avoid false cycles.
P1 Extend, Rego YAML Existing shared-index, independent-wildcard, short-value-collection and body-specific partial-object fixtures Keep paired a:{10}, b:{20} versus Cartesian a:{10,20}, b:{10,20} oracles. Add missing opposite-mode variants, not every permutation.
P1 Extend, Rego YAML Existing static-prefix fixture: query data.test.values.fixed; separately query ancestor data.test.values Keep supported child result {a:{1},b:{2}} separate from the existing unsupported ancestor diagnostic. Preserve nested-dynamic rejection and legacy-set compatibility.
P1 Extend, VM YAML + engine/RVM Rust Existing undefined, defined-false/null, dedup, COW and sibling-override fixtures Preserve exact missing-leaf {} versus static-empty {a: {set!: []}}, and defined members such as {set!: [false,null]}. Extend reuse/override cases as specified below.
P2 New, existing Rust test modules keyed_set_registration_scales, keyed_set_limits_preserve_errors Deterministic generated workloads, exact members, bounded instruction growth, typed resource failures, and successful reuse; details below.

Existing coverage also includes multiple definitions/modules, input/index keys, later-bound keys, scalar/missing loop collections, mixed rule-kind conflicts, escaped child names and genuine sibling cycles. Multiple bodies on set heads remain invalid (keyed_set_multiple_bodies_remain_rejected). The output_head_body_specific_* cases are supported partial objects, not permission to accept multiple set bodies.

Paste-ready P0 YAML

Append these entries beneath cases: in tests/rvm/rego/cases/keyed_partial_sets.yaml. Also duplicate each with a _suspendable note suffix and execution_mode: suspendable; the Rego harness does not automatically run both modes.

  - note: keyed_set_parent_loop_reentrant_cycle
    input: {keys: ["b", "a"]}
    modules:
      - |
        package test
        values.a contains 1
        values[input.keys[_]] contains count(data.test.values.a)
    query: data.test.values
    want_error: "Runtime recursion detected"

  - note: keyed_set_parent_loop_reentrant_nonmatching
    input: {keys: ["b"]}
    modules:
      - |
        package test
        values.a contains 1
        values[input.keys[_]] contains count(data.test.values.a)
    query: data.test.values
    want_result:
      a: {set!: [1]}
      b: {set!: [1]}

  - note: keyed_set_call_key_body_dependency
    modules:
      - |
        package test
        values.a contains 1
        values[lower("B")] contains n if { n := count(values.a) }
    query: data.test.values.a
    want_result: {set!: [1]}

  - note: keyed_set_call_key_body_matching_cycle
    modules:
      - |
        package test
        values.a contains 1
        values[lower("A")] contains n if { n := count(values.a) }
    query: data.test.values.a
    want_error: "Runtime recursion detected"

  - note: keyed_set_call_key_value_loop
    modules:
      - |
        package test
        values.a contains 1
        values[concat("", ["b"])] contains [count(data.test.values.a)][_]
    query: data.test.values.a
    want_result: {set!: [1]}

  - note: keyed_set_call_key_value_loop_matching_cycle
    modules:
      - |
        package test
        values.a contains 1
        values[concat("", ["a"])] contains [count(data.test.values.a)][_]
    query: data.test.values.a
    want_error: "Runtime recursion detected"

  - note: keyed_set_guarded_arithmetic_invalid
    input: {port: "invalid"}
    modules:
      - |
        package test
        values.a contains 1
        values[input.port + 1] contains 2 if { is_number(input.port) }
    query: data.test.values.a
    want_result: {set!: [1]}

  - note: keyed_set_guarded_arithmetic_numeric
    input: {port: 80}
    modules:
      - |
        package test
        values.a contains 1
        values[input.port + 1] contains 2 if { is_number(input.port) }
    query: data.test.values
    want_result:
      a: {set!: [1]}
      81: {set!: [2]}

want_error is a positive substring check on the VM error; it does not validate the interpreter's error text. It also accepts matching compilation errors, so retain direct interpreter runtime assertions. Neither relevant YAML harness implements negated-error strings: do not write want_error: "!recursion". A no-error control needs want_result.

Paste-ready interpreter state/error controls

Add to tests/engine/mod.rs, which already imports Result, regorus::* and BTreeSet. Interpreter recursion uses lowercase "recursion detected"; the VM fixtures above intentionally use its different diagnostic.

#[test]
fn keyed_set_parent_loop_restores_state_after_recursion() -> Result<()> {
    let mut engine = Engine::new();
    engine.add_policy(
        "test.rego".into(),
        r#"package test
           values.a contains 1
           values[input.keys[_]] contains count(data.test.values.a)"#.into(),
    )?;
    let compiled = engine.compile_with_entrypoint(&"data.test.values".into())?;
    let one = Value::from(BTreeSet::from([Value::from(1)]));
    let expected = Value::from(std::collections::BTreeMap::from([
        (Value::from("a"), one.clone()),
        (Value::from("b"), one.clone()),
    ]));
    for keys in [r#"["b","a"]"#, r#"["a","b"]"#] {
        let input = Value::from_json_str(&format!(r#"{{"keys":{keys}}}"#))?;
        engine.set_input(input.clone());
        for path in ["data.test.values", "data.test.values.a"] {
            let error = engine.eval_rule(path.into()).expect_err("genuine cycle");
            assert!(error.to_string().contains("recursion detected"), "{error}");
        }
        let error = compiled.eval_with_input(input).expect_err("compiled cycle");
        assert!(error.to_string().contains("recursion detected"), "{error}");

        let input = Value::from_json_str(r#"{"keys":["b"]}"#)?;
        engine.set_input(input.clone());
        assert_eq!(engine.eval_rule("data.test.values.a".into())?, one);
        assert_eq!(engine.eval_rule("data.test.values".into())?, expected);
        assert_eq!(compiled.eval_with_input(input)?, expected);
        assert_eq!(engine.eval_rule("data.test.values.a".into())?, one);
    }
    Ok(())
}

#[test]
fn keyed_set_successful_guard_does_not_swallow_head_errors() -> Result<()> {
    for strict in [false, true] {
        let mut engine = Engine::new();
        engine.set_strict_builtin_errors(strict);
        engine.add_policy(
            "test.rego".into(),
            r#"package test
               values.a contains 1
               values[input.port + 1] contains 2 if { input.enabled }"#.into(),
        )?;
        engine.set_input(Value::from_json_str(
            r#"{"port":"invalid","enabled":true}"#,
        )?);
        for path in ["data.test.values", "data.test.values.a"] {
            assert!(
                engine.eval_rule(path.into()).is_err(),
                "a successful body must preserve the head error: {path}",
            );
        }
        engine.set_input(Value::from_json_str(
            r#"{"port":"invalid","enabled":false}"#,
        )?);
        assert_eq!(
            engine.eval_rule("data.test.values.a".into())?,
            Value::from(BTreeSet::from([Value::from(1)])),
        );
    }
    Ok(())
}

The second test deliberately asserts no invented numeric-error substring. Add a separate builtin control using values[to_number(input.port)] contains 2 if { input.enabled }: strict enabled-invalid evaluation errors; non-strict enabled-invalid evaluation omits that contribution; disabled-invalid evaluation returns only the static leaf in either configuration. These are different from arithmetic type validation, which is not made harmless by non-strict builtin handling.

Execution/oracle contract

Layer Required execution
Rego YAML Interpreter eval_rule plus selected VM mode; explicit expected set/object/error, no interpreter-success/incorrect-behavior escape flags for P0.
Engine Rust eval_rule, reused CompiledPolicy::eval_with_input, error→success sequences and query-order reversal. Extend existing cycle test with both call-key matching policies to assert the interpreter diagnostic directly.
RVM Rust Run-to-completion and suspendable, actual terminal Completed/Error states; add step/resume coverage for one reentrant case. A suspendable program need not actually suspend unless stepping/host-await is enabled.
VM instruction YAML Existing harness automatically compares run-to-completion, suspendable and stepwise runs. It supports separate normal and _strict expectations.

Parity alone misses the shared call-key bug. Expected values must be independently specified, including numeric-key identity, absent leaves, empty sets, false and null. The Rego harness's strict field is currently not applied to engine/VM configuration: use direct Rust setters for strict/non-strict assertions, not a misleading YAML toggle. Input many! is not a reuse sequence in this harness either.

Focused P1/P2 additions using existing tools

  • Serialization and reuse (P1): extend the pattern in namespace_entrypoint_roundtrips_and_reuses_vm_state in tests/rvm/rego/mod.rs. Compile the original overlap policy plus main; register parent/child/main using the existing multi-entrypoint helper. Execute original, JSON-decoded and binary-decoded programs in each mode, not JSON in one mode and binary in the other. Require binary DeserializationResult::Complete. Reuse each VM with ["b"]→["b","a"]→["b"] for the reentrant policy: success→recursion error→success, using both named and indexed entrypoints. Repeatedly assert the fixed literal expectations, retaining an earlier returned value to detect mutation. Existing binary roundtrip checks compare bytes but do not execute the decoded program.
  • Opcode isolation (P1): keep all seven keyed_set_operations.yaml cases. Add a shared initial literal {key: {set!: [false]}}, copy it with Move, add null to only one alias, and return both via ArrayCreate: expected [{key:{false}}, {key:{false,null}}]. This complements the existing COW test that returns only the untouched alias. Include want_result_strict equal to want_result for this case; no new instruction/parser framework.
  • Overrides (P1): extend the two existing keyed-set override engine tests: evaluate unmodified child, overridden array query, then unmodified parent/child again on the same engine. For values.a contains 1; values[input.keys[_]] contains 2, keys ["a","b"], [values.a,values.b] with data.test.values.b as {9} must be [{1,2},{9}]; afterward parent is {a:{1,2},b:{2}}. Put the override in a main rule for compiled-policy/VM coverage. Repeat with keys ["b"] to ensure projection does not invent an "a" contribution.
  • Registration scaling (P2): generate 64 then 128 static leaves values.k0 contains 0, etc., plus values[input.keys[_]] contains 1000. Input ["k0","k0","extra"]; query k0 gives {0,1000}, parent has all static leaves plus extra:{1000}. Check decoded-program execution too. Record program.instructions.len() and require the 128-leaf program to stay below three times the 64-leaf size: a bounded regression guard against quadratic projection expansion, not a timing benchmark.
  • Limits (P2): adapt tests/execution_limits.rs to a large keyed accumulation workload; a tiny set_max_instructions must produce VmError::InstructionLimitExceeded, not undefined/partial success. Restore a generous budget and small input on the same VM; assert complete output. Repeat both modes. For allocator limits, extend tests/memory_limits.rs under its existing feature gates/lock/guard; assert existing typed memory errors, then recovery. Do not require suspendable memory-budget execution: existing tests explicitly reject it. Keep allocator tests separate from semantic YAML and avoid wall-clock scaling assertions.

Commands proposed for later execution — not run here

From the pinned checkout, after adding tests:

cargo test --test mod --features rvm keyed_
cargo test --lib --features rvm keyed_set_operations
cargo test --test execution_limits --features rvm keyed_set_
cargo test --test memory_limits --features rvm,mimalloc,allocator-memory-limits keyed_set_

For serialized/reused-program tests, name them with keyed_set_ so the first command includes them alongside the keyed-partial-set YAML file. No new dependency or framework is needed. Final acceptance requires running these additions; this proposal makes no runtime-validation claim.

snehabandla and others added 2 commits October 20, 2026 18:56
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@snehabandla

Copy link
Copy Markdown
Contributor Author

Public RVM API compatibility note

This change adds Instruction::ObjectSetAdd and VmError::RuleRecursionDetected, and marks both public enums #[non_exhaustive].

Downstream Rust callers that exhaustively match regorus::rvm::Instruction or regorus::rvm::vm::VmError must add a wildcard arm (_ => ...) to continue compiling. The new variants already require exhaustive matches to be updated; #[non_exhaustive] also requires a fallback even when all currently known variants are listed, allowing future additions without repeated match updates.

This source-compatibility change affects direct RVM API consumers, not callers using only the higher-level Engine API or language bindings.

Resolve is_fatal_vm_error conflict by keeping both RuleRecursionDetected
and NumericOverflow as fatal VM errors.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants