microsoft / microsoft/regorus

Follow-ups for #776: RVM suspendable partial-rule parity, else_bodies metadata safety, json.patch JSON-Pointer validation, AST/RuleInfo semver

Open
#781 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
346
Forks
75
Avg merge
1d 3h
Merged PRs (30d)
15

Description

Follow-up items from the review of #776 (json.patch builtin + partial-rule multi-body fix). None of these blocks correctness of normal policy evaluation — #776 passes the full opa.passing suite (2875/2875) and all targeted tests — so the plan is to merge #776 and address these here in a follow-up PR.

Findings surfaced by the repo's code-review and deep-review skills, deduplicated and calibrated below.

Should fix (before RVM suspendable / serialized-program modes are relied upon)

1. RVM suspendable path skips later successful bodies of partial rules
  • Location: src/rvm/vm/rules.rsrule_frame_after_success (current_body_index = definition_bodies.len();)
  • Issue: #776 fixed multi-body partial-rule accumulation in the run-to-completion path (execute_rule_definitions_common) but the suspendable frame path still jumps to the end of the definition after the first successful body, dropping contributions from later independent partial-object/set bodies.
  • Reachability: set_execution_mode(Suspendable) is currently only used in tests; default eval_rule uses RunToCompletion, so this is not a production allow/deny bug today. It is a dual-path divergence that will produce wrong results if suspendable/host-await evaluation is used.
  • Fix: Mirror the run-to-completion logic — for PartialSet/PartialObject advance to the next body unless the next body is marked else; keep first-success for complete/function rules. Add a suspendable-mode test with multiple independent partial bodies.
2. else_bodies metadata safety on deserialized / hand-built programs
  • Location: src/rvm/program/types.rs (#[serde(default)] pub else_bodies), consumed in src/rvm/vm/rules.rs via ...unwrap_or(false)
  • Issue: A deserialized older program (or hand-built RuleInfo) with absent/short else_bodies defaults every marker to false (= "not else"), so partial-rule else bodies can run as independent bodies after a prior body succeeds.
  • Fix: Validate else_bodies shape against definitions at load/execute time and reject incompatible metadata, or bump the serialized program version with a safe compatibility default.

Nice to have / spec-conformance

3. Tighten JSON-Pointer validation in json.patch
  • Location: src/builtins/json_patch.rsparse_path, array_index
  • The leading-/-optional relaxation is intentional (matches OPA, documented in the PR) — keep it. But the following are more lenient than RFC6902 and could yield a wrong non-Undefined result on malformed input:
    • Invalid escape sequences (~2, bare ~) are passed through literally instead of failing.
    • Signed / leading-zero array indices (+1, -0) parse as valid indices.
    • Multiple leading slashes are collapsed (//aa).
  • Fix: Validate ~ is followed only by 0/1; validate string array indexes against the RFC6902 grammar; add negative test cases (a/b, //a, /~2, /+1, /-0) asserting Undefined.
4. Malformed-patch vs resource-limit ordering
  • Location: src/builtins/json_patch.rs apply (builds full EditTree before validating op shape) + objects.rs wrapper
  • Issue: A malformed patch on a very large target can surface LimitError (correctly propagated) before the documented malformed-patch Undefined. Low impact.
  • Fix: Pre-validate operation shape (op/path/value) before constructing the edit tree, or document that resource limits preempt malformed-patch Undefined. Add a large-target malformed-patch memory-limit test.

Semver / schema (safe to defer, crate is pre-1.0 @ 0.11.0)

5. Public RuleInfo gained a required field
  • Location: src/rvm/program/types.rspub else_bodies: Vec<Vec<bool>>
  • Breaks external struct-literal construction. Consider #[non_exhaustive] / constructor-only.
6. AST JSON schema changed without version bump
  • Location: src/ast.rs (RuleBody.is_else), src/engine.rs (get_ast_as_json, version: 1)
  • New serialized field on RuleBody while the AST version stays 1. Bump/document the schema version, or skip_serializing_if the default.

Filed as the tracked follow-up for #776. Skill reports and full traces available in the review comment on the PR.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/rvm/vm/rules.rs and src/rvm/program/types.rs, then inspect the suspendable rule path and else_bodies handling described in the issue. Run or add the suspendable-mode partial-rule test and the listed json.patch negative cases in src/builtins/json_patch.rs. Done means the follow-up behaviors are covered safely, with the AST and RuleInfo compatibility decisions documented or implemented.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design, compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
32/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.