Repository navigation
Add source-aware policy params declaration detection - #826
Conversation
Mark Birger (kusha)
left a comment
Mark Birger (kusha)
left a comment
There was a problem hiding this comment.
Maksym (@maksym-mishchenko) Could we make this check generic instead of hard-coding params? I’d call it HasDeclaredRuleRootedAt(sourcePath, rootName) and use HasDeclaredRuleRootedAt(sourcePath, "params") here. It should keep the current behavior: inspect declarations in that source, count params and its descendants, including defaults, without evaluating the rules.
|
Maksym (@maksym-mishchenko) I second Mark's suggestion. It is better to make this generic. Also, ensure that we can detect rules that have prefixes. e.g |
|
I changed it to |
There was a problem hiding this comment.
🟡 Changes recommended
The implemented generalized API does not match the promised HasPolicyParams(sourcePath) contract.
1 open finding
What changed in this PR
Adds source-scoped, nonexecuting rule-declaration detection while preserving each module’s load-time parser context.
Changes:
- Adds generalized rule-root detection with extensive Rust tests.
- Persists parser context through compilation and engine restoration.
- Exposes the API through FFI and C#, including validation and documentation.
| File | Description |
|---|---|
src/tests/mod.rs |
Registers engine tests. |
src/tests/engine.rs |
Tests declaration detection and parser contexts. |
src/parser.rs |
Captures context and parses selectors. |
src/lib.rs |
Exports the selector error type. |
src/interpreter.rs |
Transfers parser contexts into compiled policies. |
src/engine.rs |
Implements source-scoped declaration detection. |
src/compiled_policy.rs |
Stores module parser contexts. |
bindings/ffi/src/engine.rs |
Adds the FFI endpoint and tests. |
bindings/csharp/Regorus/ResultHelpers.cs |
Maps invalid arguments appropriately. |
bindings/csharp/Regorus/NativeMethods.cs |
Declares the native endpoint. |
bindings/csharp/Regorus/Engine.cs |
Adds the managed API and NUL validation. |
bindings/csharp/Regorus.Tests/RegorusTests.cs |
Tests managed behavior and errors. |
bindings/csharp/API.md |
Documents declaration detection. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Maksym (@maksym-mishchenko) Can you rebase to latest main to resolve the conflicts? |
3127150 to
e711f1b
Compare
| let _ = Parser::get_path_ref_components(head)?; | ||
| let mut matched_components = 0; | ||
| let matches_prefix = rule_head_matches_selector_prefix( | ||
| head.as_ref(), | ||
| &selector_components, |
|
Maksym (@maksym-mishchenko) Can you fix the two unbounded recursions? |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Accept exact dotted declaration prefixes without evaluating rules, preserving source grammar, parser context, resource limits, and native/managed error precedence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
e711f1b to
b891b20
Compare
| `rootName` must be a nonempty path accepted by the module's load-time Rego mode | ||
| and future-keyword imports. It is parsed as a native rule reference: only a | ||
| root identifier followed by zero or more dot-qualified fields is accepted; | ||
| whitespace, comments, bracket selectors, calls, and trailing syntax are not. | ||
| Path components compare exactly, not by string prefix. Invalid selectors | ||
| throw `ArgumentException` with `ParamName` set to `rootName`. A missing or | ||
| ambiguous source, or a rule head that cannot be classified, throws | ||
| `InvalidOperationException`. Null strings throw `ArgumentNullException`; |
| /// <exception cref="InvalidOperationException"> | ||
| /// No unique loaded module matches the source path or a rule head cannot be classified. | ||
| /// </exception> |
Anand Krishnamoorthi (anakrish)
left a comment
Anand Krishnamoorthi (anakrish)
left a comment
There was a problem hiding this comment.
LGTM. The recursive rule-head traversal issues could be argued as pre existing.
Both the recursion and doc comment fixes can be done later.
bc3e134
into
microsoft:main
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89bea552-e7ec-4747-b714-389610965725
Policy loaders need to identify policy-authored rule heads before injecting assignment parameters; evaluating a rule misses conditional declarations. Add a nonexecuting
HasDeclaredRuleRootedAt(sourcePath, rootName)API across Rust, FFI, and C# that inspects only the uniquely matched loaded source.HasDeclaredRuleRootedAt(sourcePath, "params")detectsparamsdeclarations and their descendants; selectors such as"metadata.parameters"match that rule and its children, not unrelated siblings. Defaults, functions, and set rule heads count without evaluating the rules. Missing, ambiguous, or unclassifiable sources are reported as errors.