Repository navigation
fix: reject rules with unbound comparison variables at registration - #111
Merged
Merged
Conversation
…108) Rules whose comparisons reference variables bound by no positive body atom registered fine and derived wrong results. Two unsafe shapes, now rejected by a new Check 4 in rule_catalog::validate_rule (the choke point both persistent and session rule registration pass through): 1. An equality that binds a variable used nowhere else in the rule - the mistaken-filter shape from the issue repro: +gold(Name) <- customer(_, Name, _), Tier = "gold" The engine's assignment semantics satisfied it for every row, so the rule silently matched all customers. 2. A non-equality comparison over an unbound variable (Salary > 100000 with Salary bound nowhere), which previously surfaced only as a confusing runtime error (Variable not found in schema) for some shapes and nothing at all for others. The engine's intentional assignment feature is preserved: Y = constant / function / arithmetic still binds Y for use in the head or later comparisons, with fixed-point propagation through variable equalities. Error messages name the variable and the fix. Verified: 2627 lib tests green; 3 new unit tests (both rejections plus three legitimate patterns); live repro rejected at registration against a running server while the assignment-to-head pattern still derives correctly; full snapshot suite 1142 passed, 0 failed - the only two fixtures needing golden updates were the ones explicitly testing unbound-comparison behavior, whose comments already said 'should error'.
…ms, assignment inputs An independent review executed escapes against the first version and held the PR. All three findings fixed: 1. Dead-cluster blindness (blocker): duplicated or mutually-referencing dead equalities alibied each other under the used-elsewhere test - 'Tier = "gold", Tier = "gold"' and even the contradictory 'Tier = "gold", Tier = "silver"' registered and derived every row (the second equality is silently dropped downstream). Replaced with anchored-reachability: anchors are head variables plus all non-comparison predicate variables, propagated to fixed point through comparison co-occurrence; any comparison variable left unanchored is rejected. 2. Nested-term blindness: comparisons now use deep Term::variables() extraction, so 'Salary + 1 > 100' and 'abs_int64(Y) > 5' with unbound variables are rejected instead of reaching the old runtime schema error. 3. Assignment inputs: expression sides of equalities must have bound inputs - 'Z = abs_int64(Y)' with Y bound nowhere is rejected. Behavior decisions from the review, pinned by tests: the no-op alias 'Y = X' (X bound) is accepted again (harmless, ran fine pre-#111, and the previous rejection carried a factually wrong message); the constant-guard shape 'Tier = "gold", !banned(Tier)' stays accepted (well-defined semantics). Persisted pre-fix unsafe rules WARN loudly at catalog load and never brick startup, with a test pinning no-brick through the validation-skipping register() side door. 8 new tests (parser-constructed where hand-built ASTs would mask the bug under test) and snapshot fixture 12_errors/71 as an executable spec of the rejections on the persistent rule path. Lib suite 2635 green, all 35 test binaries green, full snapshot suite 1143 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #108. Rules whose comparisons reference unbound or dead variables registered fine and silently derived wrong results. A new Check 4 in rule_catalog::validate_rule - the single choke point both persistent and session rule registration pass through - rejects three unsafe shapes:
Preserved, pinned by tests as deliberate decisions: the assignment feature (Y = constant/function/arithmetic binding Y for the head or later comparisons, fixed-point propagation through equalities); the no-op alias Y = X to a bound variable; the constant-guard shape Tier = "gold", !banned(Tier).
Startup safety: persisted pre-fix unsafe rules are kept (no KG brick - pinned by a test through the validation-skipping side door) and now WARN loudly at catalog load.
Review process
The first version was held by an independent adversarial review that executed escapes end-to-end (dead clusters, nested terms, assignment inputs); all findings are fixed and each has a regression test. Full history in the commits.
Verified
Known residue (documented, not blocking)
Merge order
Independent - no conflicts with the open train.