Follow-ups for #776: RVM suspendable partial-rule parity, else_bodies metadata safety, json.patch JSON-Pointer validation, AST/RuleInfo semver
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.rs—rule_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; defaulteval_ruleusesRunToCompletion, 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/PartialObjectadvance to the next body unless the next body is markedelse; 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 insrc/rvm/vm/rules.rsvia...unwrap_or(false) - Issue: A deserialized older program (or hand-built
RuleInfo) with absent/shortelse_bodiesdefaults every marker tofalse(= "not else"), so partial-ruleelsebodies can run as independent bodies after a prior body succeeds. - Fix: Validate
else_bodiesshape againstdefinitionsat 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.rs—parse_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-Undefinedresult 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 (
//a→a).
- Invalid escape sequences (
- Fix: Validate
~is followed only by0/1; validate string array indexes against the RFC6902 grammar; add negative test cases (a/b,//a,/~2,/+1,/-0) assertingUndefined.
4. Malformed-patch vs resource-limit ordering
- Location:
src/builtins/json_patch.rsapply(builds fullEditTreebefore validating op shape) +objects.rswrapper - Issue: A malformed patch on a very large target can surface
LimitError(correctly propagated) before the documented malformed-patchUndefined. Low impact. - Fix: Pre-validate operation shape (
op/path/value) before constructing the edit tree, or document that resource limits preempt malformed-patchUndefined. 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.rs—pub 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
RuleBodywhile the ASTversionstays1. Bump/document the schema version, orskip_serializing_ifthe 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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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