feat(engine): foreach (rfc-016) + native data source resolution (rfc-017)#511
feat(engine): foreach (rfc-016) + native data source resolution (rfc-017)#511anneschuth wants to merge 36 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Engine Correctness
🟠 Significant — combine_add uses f64 accumulator for integer sums
packages/engine/src/operations.rs, combine_add:
let mut sum: f64 = 0.0;
let mut all_int = true;
for v in results {
match v {
Value::Int(i) => sum += *i as f64,
...
}
}
if all_int {
Ok(Value::Int(sum as i64))
}f64 has 53 bits of mantissa; i64 has 63. Any i64 eurocent value above 2^53 (~9 quadrillion eurocents) loses precision on the *i as f64 cast, and the sum as i64 at the end silently returns a wrong amount. With collection aggregation — the primary use case of FOREACH ADD — this is exactly the path where many per-element amounts accumulate. Should use i64::checked_add instead:
let mut sum: i64 = 0;
for v in results {
match v {
Value::Int(i) => sum = sum.checked_add(*i)
.ok_or_else(|| EngineError::InvalidOperation("FOREACH ADD overflow".to_string()))?,
Value::Null => {}
_ => return Err(TypeMismatch { ... }),
}
}
Ok(Value::Int(sum))🟠 Significant — combine_min_max same i64 → f64 → i64 roundtrip
packages/engine/src/operations.rs, combine_min_max:
let mut best: f64 = if is_min { f64::INFINITY } else { f64::NEG_INFINITY };
...
Value::Int(i) => *i as f64,
...
Ok(Value::Int(best as i64))Same precision issue. For MIN/MAX over integers, compare i64 values directly (track Option<i64> instead of f64).
🟡 Minor — combine_add string branch silently uses debug format for non-string values
packages/engine/src/operations.rs, combine_add string branch:
_ => s.push_str(&format!("{:?}", v)),When the first element is a String but a later element is not, the code falls through to format!("{:?}", v), producing Rust debug output (e.g. Int(42)) rather than a type error. Should return Err(EngineError::TypeMismatch { ... }) like the numeric branch does for non-numbers.
🟡 Minor — AND combine over empty results is vacuous truth
packages/engine/src/operations.rs:
Some("AND") => Ok(Value::Bool(
results.is_empty() || results.iter().all(|v| v.to_bool()),
)),When the collection is empty (or all elements are filtered out), AND returns true. This is mathematically correct for a universal quantifier, but in legal execution "no children satisfy the filter → all conditions are satisfied" is likely not the intended semantics. Document the choice explicitly, or consider returning false for the empty case and noting the deviation from vacuous truth in the RFC.
Preview Deployment — harvester-adminYour changes have been deployed to a preview environment: URL: https://harvester-admin-pr511-regel-k4c.rig.prd1.gn2.quattro.rijksapps.nl This deployment will be automatically cleaned up when the PR is closed. |
Preview Deployment — harvester-workerYour changes have been deployed to a preview environment: URL: https://harvester-worker-pr511-regel-k4c.rig.prd1.gn2.quattro.rijksapps.nl This deployment will be automatically cleaned up when the PR is closed. |
Preview Deployment — editor — editorYour changes have been deployed to a preview environment: URL: https://editor-pr511-regel-k4c.rig.prd1.gn2.quattro.rijksapps.nl This deployment will be automatically cleaned up when the PR is closed. |
There was a problem hiding this comment.
Review: feat(engine): implement FOREACH collection operation (RFC-016)
Changes cover article.rs, context.rs, and operations.rs. The FOREACH architecture (child scope via create_child/set_local, depth propagation, untranslatable short-circuit) is sound. create_child and set_local are infallible. The size-limit guard is a good safety measure.
Engine Correctness
🟠 Significant — add_values silently changes behavior of existing ADD when operands are null
operations.rs, the refactored add_values helper (lines touched by this PR):
// Outer match — Value::Null added:
Value::Int(_) | Value::Float(_) | Value::Null => { ... }
// Inner loop — null skipping added:
Value::Null => {} // Skip nulls in sumBefore this PR, the outer match had only Value::Int(_) | Value::Float(_), so a null first operand fell to the _ catch-all and returned a TypeMismatch error. Nulls in the middle of the list also errored in the inner loop's _ => return Err(type_error(...)) arm.
After this PR, ADD(null, 5) → 5, ADD(3, null, 7) → 10, ADD(null) → 0. This is the right semantic for combine_add inside FOREACH, but add_values is also called from execute_add (the regular ADD operation). Law specifications that relied on an error when an input was null will now silently return a partial sum. The fix is to keep the null-tolerant logic only in combine_add and not leak it into execute_add:
fn combine_add(results: &[Value]) -> Result<Value> {
// inline null-skipping here, don't rely on add_values' null handling
}🟠 Significant — AND combine over empty results returns true (vacuous truth)
operations.rs in execute_foreach, the combine match:
Some("AND") => Ok(Value::Bool(
results.is_empty() || results.iter().all(|v| v.to_bool()),
)),When the collection is empty or every element is filtered out, AND returns true. In Dutch law, a check like "alle kwalificerende kinderen voldoen aan voorwaarde X" returning true when there are no qualifying children may incorrectly grant entitlement. This is mathematically standard (vacuous truth), but it needs an explicit comment explaining the choice, and the RFC or law using AND combine must explicitly account for the empty-collection case. Without that, a law author who writes a FOREACH/AND to guard eligibility will get a silent true when the collection is empty.
Missing BDD Tests
🟡 Minor — no Gherkin feature files for FOREACH
The features/ directory has no .feature file covering FOREACH, despite this being a new operation that will appear in real law YAML files. The unit tests in foreach_ops are thorough, but BDD tests would exercise the full pipeline (YAML parsing → engine → output) and serve as regression anchors for law authors. At minimum one scenario using the YAML syntax shown in the PR description would be appropriate.
Minor Issues
🟡 combine: Option<String> is stringly-typed — invalid combine names ("SUM", "AVERAGE") only fail at execution time. An enum (CombineOp) would make schema validation possible and produce clearer parse-time errors. Low urgency since the operation is new, but worth doing before wide adoption.
🟡 combine_add is a single-line pass-through (operations.rs) — if the null-leakage issue above is fixed by inlining null-skipping into combine_add, this wrapper earns its keep. Otherwise it adds indirection with no benefit.
There was a problem hiding this comment.
Review: feat(engine): implement FOREACH collection operation (RFC-016)
Changes cover article.rs, context.rs, and operations.rs. The FOREACH architecture (child scope via create_child/set_local, depth propagation, untranslatable short-circuit) is sound. create_child and set_local are infallible. The size-limit guard is a good safety measure.
Engine Correctness
🟠 Significant — add_values silently changes behavior of existing ADD when operands are null
In operations.rs, the refactored add_values helper has two null-related additions:
// Outer match — Value::Null added to numeric arm:
Value::Int(_) | Value::Float(_) | Value::Null => { ... }
// Inner loop — null skipping added:
Value::Null => {} // Skip nulls in sumBefore this PR, the outer match had only Value::Int(_) | Value::Float(_), so a null first operand fell to the _ catch-all and returned a TypeMismatch error. Nulls in the middle of the list also errored in the inner loop's _ => return Err(type_error(...)) arm.
After this PR, ADD(null, 5) → 5, ADD(3, null, 7) → 10, ADD(null) → 0. The right semantic for combine_add inside FOREACH, but add_values is also the implementation of the regular ADD operation. Law specifications that relied on an error when an input was null will now silently return a partial sum.
Fix: keep null-tolerant logic only in combine_add and not in the shared add_values:
fn combine_add(results: &[Value]) -> Result<Value> {
// null-skipping inline here only; don't change add_values' semantics
}🟠 Significant — AND combine over empty results returns true (vacuous truth)
In execute_foreach in operations.rs:
Some("AND") => Ok(Value::Bool(
results.is_empty() || results.iter().all(|v| v.to_bool()),
)),When the collection is empty or every element is filtered out, AND returns true. In Dutch law, a check like "alle kwalificerende kinderen voldoen aan voorwaarde X" returning true when there are no qualifying children may incorrectly grant entitlement. This is mathematically standard (vacuous truth), but the choice must be documented with a comment and every law using FOREACH/AND must explicitly account for the zero-elements case. Without that, a law author who writes a FOREACH/AND to guard eligibility will get a silent true when the collection is empty.
Missing BDD Tests
🟡 Minor — no Gherkin feature files for FOREACH
The features/ directory has no .feature file covering FOREACH. The unit tests in foreach_ops are thorough, but BDD tests exercise the full pipeline (YAML parsing → engine → output) and serve as regression anchors for law authors. At minimum one scenario using the YAML syntax from the PR description would be appropriate.
Minor Issues
🟡 combine: Option<String> is stringly-typed — invalid names ("SUM", "AVERAGE") only fail at execution time. An enum would allow schema-level validation and clearer parse-time errors. Low urgency while the operation is new, but worth doing before wide adoption in corpus laws.
🟡 combine_add is a single-line pass-through to add_values with no added logic — once the null-leakage issue above is fixed (by putting null handling in combine_add directly), this wrapper becomes meaningful. Until then it adds indirection without benefit.
There was a problem hiding this comment.
Engine Correctness
🟠 Significant — add_values silently changes ADD semantics when first argument is Null
The refactored add_values adds Value::Null to the numeric match arm (Value::Int(_) | Value::Float(_) | Value::Null). Before this PR, execute_add would hit the _ => Err(type_error(...)) fallthrough when the first evaluated argument was Value::Null, returning a TypeMismatch error. After this PR, ADD(null, 5) silently enters numeric mode, skips the null, and returns Int(5).
This is a behavior change to the live ADD operation, not just the new FOREACH combine path. Any law that inadvertently passes a null-first argument to ADD will now silently succeed with a different result instead of surfacing a type error. Since laws produce legally binding decisions, silent semantic drift is a meaningful risk.
The guard in execute_add (empty check + untranslatable check) still fires before calling add_values, but the null-first case reaches add_values unchanged.
If null-skipping in numeric mode is intentional for the ADD operation generally, that should be an explicit decision with a test for ADD(null, number).
Code Quality
🟡 Minor — combine is Option<String>, not an enum
combine: Option<String> in the Foreach variant accepts arbitrary strings and only errors at execution time:
Some(other) => Err(EngineError::InvalidOperation(format!(
"Unknown FOREACH combine operation: {}",
other
)))A typo ("add" instead of "ADD") or an unsupported combiner passes YAML deserialization silently and only fails when the law is executed. Using a dedicated enum would catch this at parse time and produce a cleaner error message for law authors.
Documentation
🟡 Minor — RFC-016: contradictory sentence about filter Untranslatable propagation
The added paragraph contains a contradiction:
"If
filteritself evaluates toUntranslatable, the FOREACH propagates the untranslatable immediately (same as body). Elements that are skipped by the filter do not contribute to untranslatable detection - only elements that pass the filter and whose body producesUntranslatablecause propagation."
The bolded sentence is wrong — the preceding sentence and the code both establish that a filter evaluating to Untranslatable also causes immediate propagation. The second sentence should be removed or rephrased (e.g., "Elements whose filter evaluates to falsy are skipped and do not contribute to untranslatable detection").
Test Coverage
🟡 Minor — No test for filter Untranslatable propagation
The 13 foreach_ops tests cover untranslatable in the collection and in the body, but not in the filter. Given the RFC explicitly calls out this case (and the code handles it), a test would lock in the behavior:
// filter evaluates to Untranslatable → FOREACH propagates immediately
let untranslatable = Value::Untranslatable { ... };
// bind item whose filter expression returns untranslatable
// assert result.is_untranslatable()
Preview Deployment — docs — docsYour changes have been deployed to a preview environment: URL: https://docs-pr511-regel-k4c.rig.prd1.gn2.quattro.rijksapps.nl This deployment will be automatically cleaned up when the PR is closed. |
There was a problem hiding this comment.
Review of feat(engine): implement FOREACH collection operation (RFC-016)
Significant
add_values silently treats Null as zero in numeric mode — behavior change for existing ADD expressions
packages/engine/src/operations.rs (the new add_values function, outer match arm):
Value::Int(_) | Value::Float(_) | Value::Null => {
...
Value::Null => {} // Skip nulls in sumadd_values is now shared between execute_add (the regular ADD operation) and combine_add (FOREACH). In the old execute_add, a Value::Null first element hit the _ arm and returned type_error("number, string, or array", ...). Now ADD(null, 1, 2) silently returns Value::Int(3).
execute_add still has its own untranslatable guard before calling add_values, so untranslatable is safe. But the null-skip is a behaviour change for any existing law that passes a nullable operand as the first value of ADD: what was a runtime type error is now a silent zero-contribution. Given that laws produce legally binding calculations, silent null absorption deserves an explicit decision and a note in the PR.
combine: Option<String> — invalid combine values are not caught until runtime
packages/engine/src/article.rs, ActionOperation::Foreach:
#[serde(default)]
combine: Option<String>,A law file with combine: MULTIPLY (or any invalid value) parses cleanly and only fails when the law is executed:
Some(other) => Err(EngineError::InvalidOperation(format!(
"Unknown FOREACH combine operation: {}",
other
))),Given the existing pattern in the codebase, a typed enum (e.g., CombineOp) would reject this at deserialization and allow just validate to catch it. A runtime error mid-decision is meaningfully worse than a schema rejection.
Minor
Empty as_name bypasses validation
packages/engine/src/operations.rs, execute_foreach:
if !as_name.is_empty()
&& !as_name.chars().enumerate().all(...)An empty as_name satisfies !as_name.is_empty() == false, so the entire guard is skipped. set_local("".to_string(), item) is then called, creating an unreachable binding. The condition should be if as_name.is_empty() || !as_name.chars()... to reject empty strings explicitly.
legal_basis field on Action is undocumented and unrelated to FOREACH
packages/engine/src/article.rs:
/// Legal basis metadata (not used in computation, preserved for traceability)
#[serde(default)]
pub legal_basis: Option<serde_yaml_ng::Value>,This is not mentioned in the PR description and has no connection to the FOREACH implementation. The serde_yaml_ng::Value type also bypasses any type-safe validation for the field. Should be a separate PR with its own rationale, schema entry, and test coverage.
|
@anneschuth |
There was a problem hiding this comment.
PR #511 Review — FOREACH Collection Operation (RFC-016)
Engine Correctness
🟠 Significant — Null body results silently dropped in FOREACH combines
propagates immediately but has no equivalent check for :
// operations.rs ~line 849-856
let result = evaluate_value(body, &child, depth + 1)?;
if result.is_untranslatable() {
return Ok(result);
}
results.push(result); // Null is pushed without any propagation checkDownstream, null is silently discarded in every combine:
- ADD (via
add_values):Value::Null => {}skips the element, reducing the sum - OR/AND:
null.to_bool()evaluates to false, treating unknown as a definitive value - MIN/MAX (
combine_min_max):Value::Null => continueskips the element
With the newly-added lenient variable resolution (missing var → Null), a body expression referencing a missing property now silently contributes 0 to a sum and false to a boolean aggregate. For a law that computes per-child allowances, this produces a lower benefit without any signal. A null body result should propagate the same way as Untranslatable does.
🟠 Significant — Null filter result silently skips elements without propagating uncertainty
// operations.rs ~line 839-845
let filter_result = evaluate_value(filter_expr, &child, depth + 1)?;
if filter_result.is_untranslatable() {
return Ok(filter_result);
}
if !filter_result.to_bool() {
continue; // Skip this element
}When the filter expression evaluates to Null (e.g., a referenced property is absent), there is no explicit null check. The element is silently excluded if Null.to_bool() is false. For a filtering law that asks "does this household member qualify?" — unknown should propagate uncertainty, not silently exclude the member. The RFC specifies Untranslatable propagation from filter but is silent on Null; the implementation should match the intent.
🟠 Significant — Null not handled in string/array modes of add_values
// operations.rs ~line 442-453
Value::String(_) => {
for val in evaluated {
match val {
Value::String(s) => result.push_str(s),
_ => return Err(EngineError::TypeMismatch { ... }) // Null hits this
}
}
}The numeric branch explicitly skips nulls (Value::Null => {}), but the string and array branches have no Null case and return TypeMismatch instead. With lenient null resolution now converting missing variables to Null, a law that string-concatenates fields where one is absent shifts from VariableNotFound to TypeMismatch — a confusing error that obscures the actual cause. Null should either propagate or be skipped consistently across all modes.
Tests
🟡 Minor — test_missing_parameter_returns_null_outputs asserts nothing
// engine.rs
let _result = engine.evaluate(params, "2025-01-01");
// We just verify it doesn't panicThe original test verified a specific error (Err(EngineError::VariableNotFound)). The replacement verifies nothing — "doesn't panic" is too weak for a test covering required-parameter behavior. At minimum it should assert the result is Ok and that the outputs are null.
Artifacts
🟡 Minor — features/tmp should not be committed
An empty file features/tmp was added in the diff. This appears to be an editor artifact; it has no content and no purpose.
Out of scope but worth noting
The data_sources: Vec<DataSourceInput> field added to EvaluateRequest in bin/evaluate.rs is deserialized from JSON but no code in the diff wires it up to service.register_dict_source. If this is intentional scaffolding it should be gated or documented; if not, the evaluate binary can accept data sources in its request body but silently ignores them.
There was a problem hiding this comment.
PR #511 Review — FOREACH Collection Operation (RFC-016)
Engine Correctness
🟠 Significant — Null body results silently dropped in FOREACH combines
execute_foreach propagates Untranslatable immediately but has no equivalent check for Value::Null:
// operations.rs — execute_foreach body loop
let result = evaluate_value(body, &child, depth + 1)?;
if result.is_untranslatable() {
return Ok(result);
}
results.push(result); // Null is pushed without any propagation checkDownstream, null is silently discarded in every combine:
- ADD (via
add_values):Value::Null => {}skips the element, reducing the sum - OR/AND:
null.to_bool()evaluates to false, treating unknown as a definitive false - MIN/MAX (
combine_min_max):Value::Null => continueskips the element
With the new lenient variable resolution (missing var → Null), a body expression referencing a missing property now silently contributes 0 to a sum or false to a boolean aggregate. For a law computing per-child allowances, this produces a lower benefit with no diagnostic signal. A null body result should propagate the same way as Untranslatable.
🟠 Significant — Null filter result silently skips elements
// operations.rs — execute_foreach filter check
let filter_result = evaluate_value(filter_expr, &child, depth + 1)?;
if filter_result.is_untranslatable() {
return Ok(filter_result);
}
if !filter_result.to_bool() {
continue; // Skip this element
}When filter evaluates to Null (e.g., a referenced property is absent), there is no null check. The element is silently excluded because Null.to_bool() is false. For a law that asks "does this household member qualify?" — an unknown filter result should propagate uncertainty, not silently exclude the member. The RFC specifies Untranslatable propagation from filter but is silent on Null; the implementation should be consistent.
🟠 Significant — Null not handled in string/array modes of add_values
// operations.rs — add_values, string branch
Value::String(_) => {
for val in evaluated {
match val {
Value::String(s) => result.push_str(s),
_ => return Err(EngineError::TypeMismatch { ... }) // Null hits this arm
}
}
}The numeric branch explicitly skips nulls (Value::Null => {}), but the string and array branches have no Null case and return TypeMismatch instead. With lenient resolution now converting missing variables to Null, a law concatenating string fields where one is absent gets a confusing TypeMismatch rather than null propagation. Null should be handled consistently across all modes in add_values.
Tests
🟡 Minor — test_missing_parameter_returns_null_outputs asserts nothing
// engine.rs
let _result = engine.evaluate(params, "2025-01-01");
// We just verify it doesn't panicThe original test verified a specific error (Err(EngineError::VariableNotFound)). The replacement verifies nothing — "doesn't panic" is too weak. At minimum it should assert the result is Ok and that the outputs are null, so the test fails if the lenient-resolution contract is broken in future.
Artifacts
🟡 Minor — features/tmp should not be committed
An empty file features/tmp was added. This appears to be an editor artifact with no purpose.
Out of scope but worth noting
data_sources: Vec<DataSourceInput> added to EvaluateRequest in bin/evaluate.rs is deserialized from JSON but no code in the diff wires it to service.register_dict_source. If intentional scaffolding it should be documented; otherwise the binary silently ignores data sources supplied in the request body.
There was a problem hiding this comment.
Review of PR #511 — FOREACH collection operation
Engine correctness
🔴 Critical — FOREACH combine OR/AND treats Null as false, inconsistent with standalone OR/AND
operations.rs (new lines in execute_foreach):
Some("OR") => Ok(Value::Bool(results.iter().any(|v| v.to_bool()))),
Some("AND") => Ok(Value::Bool(
results.is_empty() || results.iter().all(|v| v.to_bool()),
)),Value::Null.to_bool() returns false. This means:
FOREACH … combine ANDwith results[true, Null]returnsBool(false)— wrong; should propagateNull.FOREACH … combine ORwith results[false, Null]returnsBool(false)— wrong; should propagateNull.
The standalone execute_and / execute_or (also modified in this PR) explicitly treat Null as a taint that propagates unless there is a definitive winner. The combines should mirror that contract: check for Null/Untranslatable elements before calling to_bool(), and propagate if no definitive winner exists. In a legal decision context this matters: "all members of a household satisfy condition X" must yield Null (unknown), not false, if any member's evaluation is unknown.
🟠 Significant — FOREACH filter treats Null as false, silently dropping elements
execute_foreach in operations.rs:
if filter_result.is_untranslatable() {
return Ok(filter_result);
}
if !filter_result.to_bool() {
continue; // Skip this element
}If the filter expression evaluates to Null (e.g., a property is missing on some element), the element is silently excluded from the collection. This can produce understated aggregates — e.g., a FOREACH that sums benefits for qualifying children will drop any child whose qualifying condition is unknown, producing a lower total rather than an unknown total. The RFC section added in this PR only specifies Untranslatable propagation from the filter; Null in the filter should also propagate (or at minimum the behaviour needs to be explicit and documented). The fix is to add a filter_result.is_null() branch before the to_bool() check, mirroring the Untranslatable path.
🟠 Significant — add_values silently skips Null in numeric mode, compounding the filter issue
add_values (new helper in operations.rs):
Value::Null => {} // Skip nulls in sumWhen used as combine: ADD in FOREACH, any Null body result is silently ignored rather than tainting the total. Combined with the filter Null issue above, two independent paths can each independently produce a lower-than-correct aggregate without surfacing an error. If an element's body evaluates to Null, the correct behaviour for a legal sum is to propagate Null (the total is unknown), not to treat the element as contributing 0. At minimum this trade-off (SQL-style NULL-skip vs. propagation) must be explicit in the RFC and the code comment; currently the comment only says "skip nulls in sum" without justification.
🟠 Significant — lenient variable resolution converts every VariableNotFound to Null engine-wide
context.rs:
- Err(EngineError::VariableNotFound(path.to_string()))
+ Ok(Value::Null)This is not scoped to FOREACH child-scope lookups; it affects every variable resolution in every law evaluation. A typo in a law YAML (instead of) will now silently produce Null and flow through all downstream operators, potentially reaching output without any error. Previously this was a hard VariableNotFound error that would surface immediately. The PR summary frames this as enabling null-propagation for FOREACH, but the change is global. If lenient resolution is intentional for the whole engine, it needs its own RFC section and every caller that previously relied on VariableNotFound for validation needs to be audited. The FOREACH implementation itself only needs lenient resolution inside child scopes (for the as_name binding); the outer scope resolution does not need to be made globally lenient.
Artifacts
🟡 Minor — features/tmp (empty file) committed
A zero-byte file features/tmp is added. This is an editor/shell artifact and must be removed before merge.
Previously noted (not repeating inline)
The prior review comment on this PR already flagged: weakened to a no-op, and in not wired to . Both remain unresolved in the current diff.
There was a problem hiding this comment.
Review of PR #511 — FOREACH collection operation
Engine correctness
🔴 Critical — FOREACH combine OR/AND treats Null as false, inconsistent with standalone OR/AND
operations.rs (new lines in execute_foreach):
Some("OR") => Ok(Value::Bool(results.iter().any(|v| v.to_bool()))),
Some("AND") => Ok(Value::Bool(
results.is_empty() || results.iter().all(|v| v.to_bool()),
)),Value::Null.to_bool() returns false. This means:
FOREACH … combine ANDwith results[true, Null]returnsBool(false)— wrong; should propagateNull.FOREACH … combine ORwith results[false, Null]returnsBool(false)— wrong; should propagateNull.
The standalone execute_and / execute_or (also modified in this PR) explicitly treat Null as a taint that propagates unless there is a definitive winner. The combines must mirror that contract: check for Null/Untranslatable elements before calling to_bool(), and propagate if no definitive winner exists. In a legal decision context this matters: "all members of a household satisfy condition X" must yield Null (unknown), not false, if any member's evaluation is unknown.
🟠 Significant — FOREACH filter treats Null as false, silently dropping elements
execute_foreach in operations.rs:
if filter_result.is_untranslatable() {
return Ok(filter_result);
}
if !filter_result.to_bool() {
continue; // Skip this element
}If the filter expression evaluates to Null (e.g., a property is missing on some element), the element is silently excluded from the collection. This can produce understated aggregates — e.g., a FOREACH that sums benefits for qualifying children will drop any child whose qualifying condition is unknown, producing a lower total rather than an unknown total. The RFC section added in this PR only specifies Untranslatable propagation from the filter; Null in the filter should also propagate (or at minimum the behaviour needs to be explicit and documented). The fix is to add a filter_result.is_null() branch before the to_bool() check, mirroring the Untranslatable path.
🟠 Significant — add_values silently skips Null in numeric mode, compounding the filter issue
add_values (new helper in operations.rs):
Value::Null => {} // Skip nulls in sumWhen used as combine: ADD in FOREACH, any Null body result is silently ignored rather than tainting the total. Combined with the filter Null issue above, two independent paths can each independently produce a lower-than-correct aggregate without surfacing an error. If an element's body evaluates to Null, the correct behaviour for a legal sum is to propagate Null (the total is unknown), not to treat the element as contributing 0. At minimum this trade-off (SQL-style NULL-skip vs. propagation) must be explicit in the RFC and the code comment; currently the comment only says "skip nulls in sum" without justification.
🟠 Significant — lenient variable resolution converts every VariableNotFound to Null engine-wide
context.rs:
- Err(EngineError::VariableNotFound(path.to_string()))
+ Ok(Value::Null)This is not scoped to FOREACH child-scope lookups; it affects every variable resolution in every law evaluation. A typo in a law YAML ($leeftiid instead of $leeftijd) will now silently produce Null and flow through all downstream operators, potentially reaching output without any error. Previously this was a hard VariableNotFound error that would surface immediately. The PR summary frames this as enabling null-propagation for FOREACH, but the change is global. If lenient resolution is intentional for the whole engine, it needs its own RFC section and every caller that previously relied on VariableNotFound for validation needs to be audited. The FOREACH implementation itself only needs lenient resolution inside child scopes (for the as_name binding); the outer scope resolution does not need to be made globally lenient.
Artifacts
🟡 Minor — features/tmp (empty file) committed
A zero-byte file features/tmp is added. This is an editor/shell artifact and must be removed before merge.
Previously noted (not repeating inline)
The prior review comment on this PR already flagged: test_missing_parameter weakened to a no-op, and data_sources in EvaluateRequest not wired to service.register_dict_source. Both remain unresolved in the current diff.
- Remove _strip_to_datasource: load original YAML into engine
- Engine does cross-law resolution (source.regulation) + DataSourceRegistry (source: {})
- Fix IN/NOT_IN values: $var → value: $var for list definition refs
- 162/340 BDD tests pass (was 138)
- 4x faster (1m28s vs 5m51s) because engine uses native cross-law resolution
Add date-aware comparison: when both operands look like ISO dates
(YYYY-MM-DD) or date objects ({iso: ...}), compare as dates.
Also add lexicographic string comparison for non-date strings.
Previously, string comparisons went through to_number which
returned 0 for non-numeric strings.
…ype safety - Revert is_null() to only match Value::Null (empty arrays/objects are not null) - Restore strict to_number(): reject non-numeric types instead of coercing to 0 - FOREACH combine OR/AND now propagate Null (matching standalone AND/OR semantics) - FOREACH filter propagates Null instead of silently dropping elements - Regular ADD propagates Null instead of silently skipping null operands - FOREACH combine ADD uses SQL-style null skip (separate from regular ADD) - Change combine field from Option<String> to typed CombineOp enum - Reject empty as_name in FOREACH validation - Fix test_missing_parameter to assert expected outputs - Add tests for filter null propagation, combine null handling, empty as_name - Fix RFC-016 contradictory text about filter null/untranslatable propagation - Remove committed artifacts: worktree, mutants.out.old, tmp files - Update .gitignore to prevent re-committing these artifacts
EXISTS returns true if value is not null AND not empty array/object. This matches legal semantics: an empty registration list means no registrations exist, unlike IS_NULL which only checks for Null. 305/340 BDD tests pass (was 270).
Numeric ADD now uses SQL SUM semantics: ADD(1500000, Null, 0, Null) returns 1500000 instead of Null. Also fixes type determination when the first element is Null by finding the first non-Null value. Array and string ADD still propagate Null (unchanged).
When a FOREACH iterates over objects, the body can reference element fields directly ($startdatum) without requiring $current.startdatum notation. This matches the Python engine behavior and fixes laws that use FOREACH with body operations referencing element fields directly.
- Add GET operation: look up a key in a map/object, coercing the key to string for lookup (map keys are always strings) - Fix IN operation for Object values: check if subject is a key in the map instead of wrapping the map in a single-element array - These changes support laws that use map definitions for table lookups (e.g., kostendelersnorm_factoren in participatiewet)
Concatenates evaluated values into a single string. Null values are treated as empty strings. This supports laws that build addresses and descriptions from multiple fields (e.g., wet_brp postadres).
Fix doc comments that contradicted implementation: add_values documents that numeric ADD uses SQL SUM semantics (skip null) by design, CONCAT documents deliberate null-skip for display strings, and combine_add clarifies its relationship to add_values. Make test_missing_parameter panic on unexpected Err instead of silently passing. Add object field flattening section to RFC-016. Fix clippy unwrap warnings in add_values.
Extend the engine so it can fully resolve external data source inputs
from inline YAML source: blocks, eliminating the need for callers to
pre-resolve them in Python or external mapping files.
Schema v0.5.1 (backwards-compatible additions to inputField.source):
- table: data source name to read from
- field: column to project as the input value
- fields: list of columns to bundle into an object input
- select_on: filter criteria (list of {name, value} pairs, value may be
literal or $variable resolved against the law context)
- source_type: optional kind tag (claim, cases, events, AWB, KVK, etc.)
- service: service code that owns the source
DataSourceRegistry — new RecordSetDataSource (data_source.rs):
- Builder pattern with .key_field, .select_on, .alias, .array_field
- Multi-criteria filtering across arbitrary fields
- Field aliases (input name differs from column name)
- Array field support for FOREACH iteration with optional projection
- Loose Int/String/Float criterion comparison
Article/Input struct (article.rs):
- New ExternalSource fields parsed from YAML: table, field, fields,
select_on, source_type, service
- Backwards compatible — old source: {} and source: {regulation, ...}
blocks still parse identically
Resolution (service.rs, context.rs):
- evaluate_external_input_internal reads stored source metadata and
builds the right select_on criteria, applies field aliases, handles
array projection — all without the caller needing to know about it
- Operations: support resolving from RecordSetDataSource via the
registry's existing trait
PyO3 (python.rs):
- New register_record_set_source(name, records, key_field, select_on,
aliases, array_field, array_field_projection) for callers that
prefer to push records imperatively
- The native YAML path is preferred — the imperative API is a fallback
Tests: 11 new unit tests (data_source.rs) covering simple lookup,
multi-criteria, aliases, array field with/without projection,
no-match, registry priority, builder validation, combined options.
Full crate suite passes.
This unblocks the poc-machine-law side: the Python wrapper can now
delete its 600+ lines of pre-resolution glue (_pre_resolve_data_sources,
_register_array_data_source, _OBJECT_FIELD_MAPPING, _FIELD_ALIAS_MAPPING,
_LAW_INPUT_MAPPING, etc.) and rely on the engine to resolve everything
from the YAML.
…es cross-law Two related fixes that unblock pure-Rust orchestration: 1. IN/NOT_IN with an array subject (e.g. from `$arr.field` dot-notation projection) now matches element-wise: returns true if ANY element of the subject is in the candidate list/value/object. Previously the array was compared as a whole against each candidate, which always failed unless the candidate list happened to contain that exact array. Real example from poc-machine-law: handelsregisterwet's `is_actieve_ondernemer` does `$inschrijvingen.rechtsvorm IN $ondernemersvormen`. The projection yields `["EENMANSZAAK"]` and the definition list is `["EENMANSZAAK", "VOF", "MAATSCHAP", "COMMANDITAIRE_VENNOOTSCHAP"]`. Without this fix the IN check returned false and BBZ requirements collapsed. 2. `select_on` $variable references now also resolve against `resolved_inputs`, not just `parameters`. This lets later inputs filter on earlier inputs via select_on — used by precariobelasting where `$vestigingsadres` (a cross-law output from KVK handelsregisterwet/bedrijfsgegevens) is the select_on filter for `is_openbare_weg` lookup in `bgt_terraslocaties`. Tests: 3 new unit tests for the IN array semantics (`test_in_array_subject_any_match_in_values`, `test_in_array_subject_no_match_in_values`, `test_in_array_subject_any_match_in_value_ref`). Full crate suite: 375 passed, 0 failed. poc-machine-law: 340/340 BDD scenarios pass after these fixes.
The engine now reads precision/min/max from TypeSpec on output specs and rounds float results to the configured number of decimals. precision: 0 floors eurocent and round-half-even otherwise; precision: N rounds to N decimals. This eliminates the Python post-processing pass that previously corrected for floating-point noise on cross-law numeric outputs.
Optional inputs sourced from data registry / cross-law that fail to produce a value now receive a type-appropriate default (0 for numbers, false for booleans, [] for arrays, etc) instead of leaving downstream arithmetic to crash on null. String/date inputs are deliberately left null so IS_NULL checks keep working. Required inputs are never defaulted: callers can still distinguish 'data is missing' from 'data is zero'. Hard cross-law errors (LawNotFound, CircularReference) continue to propagate; only soft failures (OutputNotFound, ArticleNotFound, null values) trigger the default. Adds Input.required and ParameterType import to support the check. Replaces the matching Python _fill_input_defaults helper.
The shifted-date plumbing for cross-law inputs already exists, but nothing pinned the end-to-end behaviour. This adds an integration test that loads two versions of a base law (valid 2024-01-01 and 2025-01-01) and verifies that a consumer with temporal.reference: $prev_january_first on calculation date 2025-06-15 picks the v1 (2024) version of the base law. Guards against regressions in resolve_external_input_internal / with_shifted_date.
Adds three integration tests asserting that FOREACH bodies are fully evaluated in the per-item scope: - CONCAT body referencing the iteration variable yields a Vec<String> rather than raw operation dicts. - A bare $outer_definition body resolves through the parent context. - A CONCAT body that mixes a top-level parameter with an iteration field still resolves the parameter via the inherited scope. These behaviours already work because RuleContext::create_child clones the parent's local scope and the engine dispatches CONCAT through execute_concat. The tests guard against regressions when the poc-machine-law Python post-processing layer is removed.
The previous precision implementation made eurocent + precision: 0 floor instead of round, which broke monetary outputs that depend on bankers-rounded cents (e.g. zorgtoeslag 210915.6 → 210916). Eurocent now always rounds half-even to integer, ignoring any precision annotation. precision: 0 without an eurocent unit still floors to match the historic Python helper that previously did the same as a post-processing step on cross-law numeric outputs.
Inputs in an article can reference each other through `$name` refs in `source.parameters`, `source.select_on[*].value`, and `temporal.reference`. The previous linear walk resolved inputs in strict YAML order, so an input referencing a later-declared input saw it as Null (lenient resolution returns Null for unknown names) and fed garbage into cross-law calls or data-source lookups. Real cases: - `laws/alcoholwet/vergunning/gemeenten/GEMEENTE_ROTTERDAM-2024-01-01.yaml`: `leeftijd_exploitant.parameters.bsn: $bsn` where `bsn` is declared later as a data-source input. The same article's `voldoet_aan_nationale_eisen` depends on three later inputs (`$vloeroppervlakte_horecalokaliteit`, `$type_bedrijf`, `$leeftijd_exploitant`). - `laws/kieswet/KIESRAAD-2024-01-01.yaml`: `leeftijd.parameters.referentiedatum` and `gerechtelijke_uitsluiting.temporal.reference` both reference `$verkiezingsdatum`, which is the last input (from a data source). Add a topological pre-pass before input resolution: - Build a dep graph from `$name` refs in `source.parameters`, `source.select_on[*].value`, and `temporal.reference`. Only refs to other inputs in the same article count; refs to runtime parameters or unknown names are ignored. Dot notation (`$obj.field`) depends on the base name. - Iterative DFS topo-sort that preserves YAML order where no edge constrains it. - On cycle, `tracing::warn!` and fall back to YAML order (no panic, no error) so the engine keeps prior behaviour on malformed articles. Uses the existing `SelectOnCriterion` and `Temporal` types introduced by the native data source work; no schema changes required. Unlocks roughly 10 downstream scenarios in poc-machine-law that exercise these input-dependency patterns (alcoholwet, apv_exploitatievergunning, kieswet, wet_inkomstenbelasting, wijziging). Tests: adds `service::tests::topo_sort` module with coverage for linear deps, reverse chains, YAML-order preservation when no deps, select_on edges, temporal.reference edges, runtime-parameter refs being ignored, dot-notation base-name deps, self-cycles, two-node cycles, and the full alcoholwet Rotterdam pattern. Also applies mechanical `cargo fmt` fixes in data_source.rs and operations.rs and adds `#[allow(clippy::too_many_arguments)]` on the Python `register_record_set_source` binding, so `cargo fmt --check --all` and `RUSTFLAGS=-Dwarnings cargo clippy --all-features` are green on this branch.
… sources
When an input declares an explicit cross-law source (`source.regulation`)
or internal reference (`source.output`), the legacy single-key
DataSourceRegistry fallback was matching by input name first and
short-circuiting the intended cross-law call.
Concrete collision: `laws/kieswet/KIESRAAD-2024-01-01.yaml` declares a
boolean `nationaliteit` input sourced from
`wet_brp#heeft_nederlandse_nationaliteit`. A registered `personen` table
exposes a string column `nationaliteit` ("NEDERLANDS"), which the legacy
registry returned as-is, bypassing the boolean cross-law evaluation.
Fix: only consult the legacy registry when the YAML source has no
`regulation` and no `output` set, i.e. a bare `source: {}` with no
native `table` match. Explicit cross-law and internal references now
always go to their dedicated branches.
Update test_data_registry_provides_input to use `source: {}` (its
original intent was the empty-source fallback path). Add
test_cross_law_wins_over_registry_with_same_input_name pinning the new
priority.
…declared
When YAML declares `source: {table: X, field: Y, select_on: [...]}` and the
native lookup finds no matching record, the engine previously fell through to
the legacy single-key DataSourceRegistry path. That legacy path scans every
registered source for any column literally named the same as the input,
which causes cross-table collisions.
Concrete case: wet_brp article 2.7 declares
`source: {table: verblijfplaats, fields: [straat, huisnummer, postcode,
woonplaats, type]}` for its `adres` input (an object). APV exploitatie-
vergunning registers a `horecagebiedsplannen` source with a string `adres`
column. With no `verblijfplaats` registered for that test, the native lookup
missed and the legacy path returned the horecagebiedsplannen string, which
then crashed wet_brp 2.7 at `get_property("type", String)` with a type
mismatch. Same class of bug masked the two failing APV terrassen scenarios.
Fix: when `source.table` is set, treat a native miss as a definitive no-match.
Type-default optional inputs (consistent with the final else branch) and emit
a trace node so the dashboard can show `Native lookup missed: no record in
table 'X' matching select_on`. Also add a defensive `source.table.is_none()`
predicate to the legacy block to make the invariant explicit.
Adds a service-layer unit test that registers two sources — one with an
integer `x` that select_on will miss, one with a string `x` under a
different key — and asserts the type-defaulted 0 comes back, not the
string leaking across tables.
BDD (poc-machine-law, `~@slow`):
- baseline: 333 passed / 8 failed
- with fix: 337 passed / 4 failed (APV exploitatie 19+61 and APV terrassen
74+89 now pass; remaining 4 failures are preexisting and unrelated)
RecordSetDataSource::query_native used to wrap Value::Null into the
result array whenever a matched record did not contain the projected
column at all. Downstream EXISTS([Null]) then evaluated to true and
silently corrupted law outputs.
Concrete breakage: wet_brp input kinderen_gegevens (type array) reads
relaties.kinderen via select_on bsn. A BDD scenario registered a
relaties row without any kinderen column; query_native projected
record.get("kinderen").unwrap_or(Null) and returned [Null]. That made
heeft_kinderen_onder_12 true for a childless person, IACK was added,
and totale_heffingskortingen came out 265000 cents too high.
project_record_value now returns Option<Value>. It distinguishes
"column absent" (contains_key false) from "column present with null"
using contains_key. query_native's array branch filter_maps over the
projections and returns None when the resulting array is empty; the
scalar branch propagates the None so the caller can type-default.
Unit tests cover all five cases: absent+array, present-null+array,
mixed presence, absent+scalar, present-null+scalar.
Review of PR #511: FOREACH (RFC-016) + native data source resolution (RFC-017)This is a large, well-structured PR. The design is sound, the commit messages are clear, and the bug fixes are well-explained. Three issues worth addressing before merge: Significant1. FOREACH, EXISTS, SUBTRACT_DATE, GET, CONCAT not added to the JSON schema
Consequence: any corpus YAML that uses Either bump the schema to v0.5.2 with the new operation definitions, or add them to v0.5.1 — but the current mismatch between engine capability and schema validity is an inconsistency that will bite the first author who tries to use FOREACH in a corpus file. 2. SUBTRACT_DATE year/month calculations use 365.25-day approximation
"years" => Ok(Value::Float(delta_days as f64 / 365.25)),
"months" => Ok(Value::Float(delta_days as f64 / (365.25 / 12.0))),365.25 is an average, not a calendar year. A person employed from 2024-03-01 to 2025-03-01 has been employed for exactly 1 calendar year (366 days, leap year). Dividing 366 / 365.25 = 1.00205 — fine. But a person employed from 2023-03-01 to 2024-03-01 (365 days, non-leap): 365 / 365.25 = 0.99932 — the engine would report less than 1 year. At a "≥ 1 year" boundary this flips the legal determination. The existing Minor3.
|
There was a problem hiding this comment.
Inline on packages/engine/src/operations.rs — execute_subtract_date:
years and months use 365.25-day division. A person employed for exactly 365 days (non-leap year) gets 365 / 365.25 = 0.9993, which fails a >= 1 year boundary check. AGE uses calendar arithmetic; SUBTRACT_DATE with date units should match.
anneschuth
force-pushed
the
feat/foreach-implementation
branch
from
e6ee7d7 to
0f584e9
Compare
Companion to RFC-016. Schema v0.5.1 reduced source: blocks to just regulation/output/parameters, dropping the inline metadata that described how to query a tabular data source. Hosts had to maintain external mapping files to bridge the gap. RFC-017 restores inline native data source binding via additive optional properties: source_type, service, table, field, fields, select_on. Existing v0.5.1 source forms remain valid; the new properties compose with cross-law references and FOREACH (RFC-016). Implementation lands in PR #511 (feat/foreach-implementation): - ExternalSource struct extended in article.rs - RecordSetDataSource added in data_source.rs - Native YAML resolution in service.rs - Schema v0.5.1 additions - PyO3 register_record_set_source binding Eliminates ~600 lines of pre-resolution glue in poc-machine-law's Python wrapper.
anneschuth
force-pushed
the
feat/foreach-operation
branch
from
0e72ffa to
460512c
Compare
2 participants
Summary
Implements RFC-016 (FOREACH collection operations) and RFC-017 (native data source metadata). See PR #510 for the RFCs.
This PR enables pure-Rust orchestration: the engine reads inline
source.{table,field,fields,select_on,source_type,service}metadata from YAML laws and resolves data source inputs itself, eliminating the need for host-side pre-resolution glue.What's in this PR
FOREACH operation (RFC-016)
ActionOperation::Foreachvariant inarticle.rswithover,as,where?,combine?,valuefieldsoperations.rsusing the existing child-context infrastructure for scopingMAX_ARRAY_SIZEandMAX_RECURSION_DEPTHboundsNative data source resolution (RFC-017)
Sourcestruct inarticle.rsextended with optionaltable,field,fields,select_on,source_type,servicefields (all#[serde(default)], parsed automatically by serde)RecordSetDataSourceindata_source.rswith builder pattern:key_fieldfor fast single-key lookupselect_onfor multi-criteria filteringaliasesfor input-name → column-name mappingarray_fieldwith optional projection for FOREACH iterationDataSource::query_nativetrait method (default impl) + concrete impl onRecordSetDataSourceDataSourceRegistry::resolve_nativeandfind_source_by_nameservice.rs::resolve_inputs_with_servicebranches onsource.table.is_some(): buildsVec<SelectOn>from YAML criteria (resolves$paramrefs against runtime parameters AND already-resolved inputs), callsdata_registry.resolve_native(...). Falls back to legacyDataSourceRegistry::resolvefor YAMLs withoutsource.table.IN/NOT_INwith an array subject (e.g. from$arr.fielddot-notation projection) now matches element-wise: returns true if any element of subject is in the candidate list$nameinsource.parameters,source.select_on[*].value, andtemporal.reference. The resolver now topo-sorts inputs before resolving, falling back to YAML order on cycles. Unlocks alcoholwet Rotterdam, kieswet, and ~8 other downstream scenarios where later-declared inputs are referenced by earlier ones. See RFC-017 "Resolution order" section.source.regulationorsource.output, the legacy single-keyDataSourceRegistryfallback is now skipped so a registered data source column with the same name cannot short-circuit the cross-law call. Fixes the kieswetnationaliteitcollision (boolean cross-law vs. string column inpersonen).source.tableis declared and native lookup misses (commit b80352d). Same class of bug as the kieswet fix: wet_brp article 2.7 declaressource: {table: verblijfplaats, fields: [..., type]}for its object-typedadresinput. When the APV exploitatievergunning test does not registerverblijfplaats, the native lookup misses and the old code fell through to the legacy single-key path, which returned a string column namedadresfrom an unrelatedhorecagebiedsplannensource — crashing article 2.7 atget_property("type", String). The engine now treats a native miss as definitive, type-defaults optional inputs, and emits aNative lookup missed: no record in table 'X' matching select_ontrace node. Fixes APV exploitatievergunning scenarios 19+61 and APV terrassen scenarios 74+89.query_nativeresults (commit c85223b).RecordSetDataSource::query_nativeused to returnSome([Null])when a matched record had no column with the requested name at all, becauseproject_record_valuefell through torecord.get(&lc).cloned().unwrap_or(Null)and the array branch wrapped every per-record projection indiscriminately. DownstreamEXISTS([Null])evaluated to true and silently corrupted law outputs. Concrete breakage: wet_brp inputkinderen_gegevens(type array) readsrelaties.kinderenviaselect_on: bsn. A BDD scenario registered arelatiesrow without anykinderencolumn; the engine returned[Null],heeft_kinderen_onder_12became true for a childless person, IACK was added, andtotale_heffingskortingencame out 265000 cents too high.project_record_valuenow returnsOption<Value>and usescontains_keyto distinguish "column absent" from "column present with null". The array branchfilter_maps projections and returnsNonewhen every matched record lacks the column; the scalar branch propagatesNoneso the caller can type-default. Present-with-null values are preserved. Fixes wet_inkomstenbelasting BELASTINGDIENST 2001-01-01 scenarios 10 and 60.PyO3 binding
RegelrechtEngine.register_record_set_source(name, records, key_field, select_on, aliases, array_field, array_field_projection)for hosts that prefer to push records imperatively (the YAML-driven path is preferred)evaluate_with_tracereturns the full execution trace as JSON for callers that want the resolution pathSchema v0.5.1
service,table,field,fields,select_on,source_typeproperties toinputField.sourcesource: {}andsource: {regulation, ...}still validateTest results
query_nativeDownstream verification (poc-machine-law)
After installing the wheel built from this branch, the poc-machine-law BDD suite passes 340/340 scenarios across 80 laws, with the entire
_pre_resolve_data_sourcesPython pre-resolution layer no longer needed for the migrated YAMLs.Test plan
RecordSetDataSource(basic, select_on, aliases, array, projection, edge cases)