Skip to content

[bindings] Expose Engine.clone() to JS and add explicit Engine.prepare() warm-up API across bindings#722

Draft
Copilot wants to merge 7 commits into
mainfrom
copilot/add-clone-method-to-engine
Draft

[bindings] Expose Engine.clone() to JS and add explicit Engine.prepare() warm-up API across bindings#722
Copilot wants to merge 7 commits into
mainfrom
copilot/add-clone-method-to-engine

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 13, 2026

WASM Engine implemented Clone in Rust but did not expose cloning to JavaScript. Also, cloning before first evaluation incurred heavy one-time preparation cost, requiring users to run a dummy eval; this change formalizes that warm-up path with an explicit prepare API and extends that capability across bindings.

  • Core engine API

    • Added Engine::prepare() in src/engine.rs.
    • prepare() initializes internal evaluation structures without executing a query/rule, enabling explicit warm-up before cloning.
    • Expanded Engine::prepare() docs to clarify:
      • it is optional for correctness,
      • first eval pays the setup cost if prepare() is not called,
      • preparation is invalidated after policy/data changes and re-run on next prepare/eval.
    • Fixed target-aware preparation flow for azure_policy: calling prepare() before compile_for_target() now still performs target resolution/resource-type inference (no longer skipped due to self.prepared short-circuit).
  • WASM binding surface

    • Exposed JS engine.clone() in bindings/wasm/src/lib.rs (js_name = "clone").
    • Exposed JS engine.prepare() in bindings/wasm/src/lib.rs, mapped to core Engine::prepare().
    • Updated wasm clone/prepare docs to clarify performance intent and lifecycle behavior.
  • Other bindings

    • Added prepare exposure beyond wasm for API parity:
      • FFI/C API: regorus_engine_prepare
      • Go: Engine.Prepare()
      • C++: Engine::prepare()
      • C#: Engine.Prepare()
      • Java: Engine.prepare() / nativePrepare
      • Python: Engine.prepare()
      • Ruby: Engine#prepare
  • Behavioral coverage

    • Added focused engine test prepare_then_clone_without_initial_eval in tests/engine/mod.rs to validate prepare-then-clone flow without a dummy initial eval.
    • Added azure_policy regression test prepare_then_compile_for_target in tests/engine/mod.rs to validate prepare() -> compile_for_target() behavior.
    • Strengthened Java binding test to validate prepare + clone behavior during evaluation.
  • WASM docs/examples

    • Updated bindings/wasm/README.md and bindings/wasm/test.js to show recommended usage: load policy/data → prepare()clone() per request.
const template = new regorus.Engine();
template.addPolicy("policy.rego", policy);
template.addDataJson(dataJson);
template.prepare();            // one-time warm-up

const e = template.clone();    // cheap per-request clone
e.setInputJson(inputJson);
const out = e.evalRule("data.pkg.allow");

Copilot AI linked an issue May 13, 2026 that may be closed by this pull request
Agent-Logs-Url: https://github.com/microsoft/regorus/sessions/58419b20-1586-4a23-85fb-5aed5f5d5961

Co-authored-by: anakrish <35780660+anakrish@users.noreply.github.com>
Copilot AI changed the title [WIP] Add clone method to expose Engine.clone in wasm [wasm] Expose Engine.clone() to JS and add explicit Engine.prepare() warm-up API May 13, 2026
Copilot AI requested a review from anakrish May 13, 2026 20:20
Copy link
Copy Markdown
Collaborator

@anakrish anakrish left a comment

Choose a reason for hiding this comment

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

@copilot We will want to address all other bindings as well. Is this only a wasm problem?

Comment thread bindings/wasm/src/lib.rs

/// Clone this engine.
///
/// Useful for creating per-request engines after loading policy/data once.
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.

Remarks on the performance of this. Tahe a deep look. It is intended to avoid deep colies.

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.

Updated the clone docs to clarify performance intent: clone avoids reparsing/reloading immutable policy structures, while mutable evaluation state is copied for isolation. Commit: 730e6de.

Comment thread bindings/wasm/src/lib.rs Outdated
/// Prepare the engine for evaluation.
///
/// This initializes internal evaluation structures so a cloned engine can
/// evaluate without requiring an initial "dummy" evaluation.
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.

Do we need to remark that if we don't call this, the first eval on the engine will pay this cost. Also about adding policies after preparation.

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.

Added explicit prepare semantics in wasm docs: first eval pays this one-time setup if prepare() is not called, and modifying policy/data after prepare invalidates prepared state. Commit: 730e6de.

Comment thread src/engine.rs Outdated
/// Prepare the engine for evaluation without executing a query or rule.
///
/// This parses and initializes internal evaluation data structures so that
/// subsequent evaluations (or cloned engines) can run without paying the
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.

Can we make the documentation more clear about why this is needed, what ahappens if not called, what happens if policies added after this etc.

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.

Expanded core Engine::prepare() docs with why it exists, behavior when omitted (first eval prepares implicitly), and invalidation rules after policy/data changes. Commit: 730e6de.

Agent-Logs-Url: https://github.com/microsoft/regorus/sessions/49ba4462-95d3-42c0-a302-db1b81df4f65

Co-authored-by: anakrish <35780660+anakrish@users.noreply.github.com>
@anakrish anakrish requested a review from Copilot May 14, 2026 13:16
Agent-Logs-Url: https://github.com/microsoft/regorus/sessions/49ba4462-95d3-42c0-a302-db1b81df4f65

Co-authored-by: anakrish <35780660+anakrish@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 14, 2026

@copilot We will want to address all other bindings as well. Is this only a wasm problem?

Not just wasm. I added prepare across bindings: FFI/C API plus Go, C++, C#, Java, Python, and Ruby wrappers, so warm-up-before-clone is available consistently. Commit: 730e6de.

Copilot AI changed the title [wasm] Expose Engine.clone() to JS and add explicit Engine.prepare() warm-up API [bindings] Expose Engine.clone() to JS and add explicit Engine.prepare() warm-up API across bindings May 14, 2026
Copilot AI requested a review from anakrish May 14, 2026 13:20
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an explicit Engine.prepare() warm-up API to the core engine and propagates it to every binding (WASM, Ruby, Python, Java, Go, FFI/C, C++, C#). Also exposes Engine.clone() to JS (WASM) since the underlying Rust Clone impl was not previously bridged. The intended workflow is: load policies/data → prepare() once → clone() per request.

Changes:

  • Adds Engine::prepare() in src/engine.rs (a public wrapper around the existing internal prepare_for_eval(false, false)).
  • Bridges prepare() (and JS clone()) across all bindings, updates docs/examples/tests accordingly.
  • Adds a prepare_then_clone_without_initial_eval integration test.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/engine.rs New public prepare() method delegating to prepare_for_eval.
tests/engine/mod.rs Test validating prepare → clone → independent evals.
bindings/wasm/src/lib.rs Exposes clone and prepare to JS; test uses prepare().
bindings/wasm/test.js Demonstrates prepare + clone workflow.
bindings/wasm/README.md Documents recommended prepare/clone usage.
bindings/ffi/src/engine.rs New regorus_engine_prepare C FFI export.
bindings/cpp/regorus.hpp C++ prepare() wrapper.
bindings/csharp/Regorus/{Engine.cs,NativeMethods.cs} C# Prepare() and P/Invoke binding.
bindings/go/pkg/regorus/mod.go Go Prepare() wrapper.
bindings/java/src/main/java/.../Engine.java Java prepare() method + native declaration.
bindings/java/src/lib.rs JNI nativePrepare implementation.
bindings/java/com_microsoft_regorus_Engine.h JNI header for nativePrepare.
bindings/java/src/test/java/.../EngineTest.java Test exercising prepare() + clone().
bindings/python/src/lib.rs Python prepare() binding.
bindings/python/test.py Adds prepare() before clone in example.
bindings/ruby/ext/regorusrb/src/lib.rs Ruby prepare method + class registration.
bindings/ruby/test/test_regorus.rb Test calls prepare before clone.
Files not reviewed (1)
  • bindings/java/com_microsoft_regorus_Engine.h: Language not supported

Comment thread src/engine.rs
Comment on lines +545 to +547
pub fn prepare(&mut self) -> Result<()> {
self.prepare_for_eval(false, false)
}
@anakrish
Copy link
Copy Markdown
Collaborator

@copilot Review this PR from different perspectives using different agents. Also launch an agent to review the PR using the deep-review skill

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 14, 2026

@copilot Review this PR from different perspectives using different agents. Also launch an agent to review the PR using the deep-review skill

Ran a deep multi-agent review (3 discovery agents, 2 micro-passes, 1 adversarial verifier). Consolidated outcome: 1 confirmed High issue and 1 confirmed Low test gap.

  • High (confirmed): Engine::prepare() can prevent target-aware setup in later compile_for_target() due to self.prepared short-circuit (src/engine.rs:545-547, 687-691, 1129-1177).
  • Low (confirmed): no test currently covers prepare() -> compile_for_target() (existing tests only cover prepare+eval/clone+eval).

No additional findings from adversarial pass. If you want, I can submit a follow-up patch to make target-aware preparation idempotent/safe and add the missing azure-policy regression test.

@anakrish
Copy link
Copy Markdown
Collaborator

@copilot Address all review comments. Respond to the comments in thread as well.

Copilot AI and others added 2 commits May 14, 2026 20:07
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 14, 2026

@copilot Address all review comments. Respond to the comments in thread as well.

Addressed all actionable review comments in code.

Implemented fix for target-aware preparation so prepare() no longer prevents compile_for_target() target resolution/inference, and added an azure_policy regression test for prepare() -> compile_for_target().

Commits: 196b6d6, 4cc82e2.

Validation run completed: targeted tests pass (prepare_then_clone_without_initial_eval, prepare_then_compile_for_target), code review feedback was incorporated, and CodeQL timed out in the validation service.

@anakrish
Copy link
Copy Markdown
Collaborator

@copilot Run a the deep-review skill to review this PR

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 14, 2026

@copilot Run a the deep-review skill to review this PR

Ran deep-review on current PR tip 4cc82e2.

Findings:

  1. High: bindings/java/src/lib.rs nativePrepare dereferences raw pointer without invalid-handle guard (post-close() use can crash/UAF path).
  2. High (medium confidence): src/engine.rs sets self.prepared = true before target resolve/infer; on resolve/infer error, prepared state may persist.
  3. Low: missing negative tests for (1) closed-handle Java lifecycle and (2) prepare() -> compile_for_target() failure/recovery path.

Dropped: Ruby cfg-gating candidate was not introduced by this diff.
No additional findings from adversarial pass.

@anakrish
Copy link
Copy Markdown
Collaborator

@copilot Address the review findings

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 15, 2026

@copilot Address the review findings

Addressed in 639bfe3.

  • Fixed Java closed-handle safety for this API path: Engine now tracks closed state (enginePtr = 0 on close), uses requireOpen() for public calls, makes close() idempotent, and JNI nativePrepare/nativeClone now reject closed handles safely.
  • Fixed core prepare state handling: self.prepared is now set only after successful target-specific resolve/infer, so failed target preparation does not leave stale prepared state.
  • Added regression tests:
    • Java: test_closed_engine_operations_throw
    • Rust (azure_policy): prepare_then_compile_for_target_error_recovery

Validation run passed for targeted Rust + Java tests; parallel validation code review succeeded and CodeQL timed out in the validation service.

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.

[wasm] Expose Engine.clone to bindings

3 participants