Say what the container-equality fix actually changed, and pin a hash that can move

Review of the fix commit found it correct and under-described, which for a
change that widens what matches is most of the way to being a defect. The
explanation was entirely about decimal scale -- the commit message, the Javadoc,
the spec amendment and all six tests -- and decimal scale is only part of what
moved.

Jackson's node equality is representation equality: it separates IntNode from
DoubleNode from DecimalNode, and from Jackson 3 one DecimalNode scale from
another. Section 2.6.1 puts all of them in one {number} class. So delegating
container equality to Jackson was never the same predicate, and three things
changed when it stopped:

  {a: 100.00} vs {a: 100.0}   unequal -> equal   (Jackson 3 regression)
  {a: 1} vs {a: 1.0}          unequal -> equal   (Jackson 2 did this too)
  {a: NaN} vs {a: NaN}        equal   -> unequal (went the other way)

Only the first was caused by the migration. The second is older and needs no
BigDecimal to reach -- readTree on ordinary JSON produces it -- so the blast
radius is wider than the money example implied, and the previous commit's claim
that DecimalNodes do not come from readTree was true of the reported bug and
misleading about the fix. The third is a narrowing, and it is what the scalar
path always did. All three now answer the way the scalar path answers, which is
the point; the amendment tabulates them rather than leaving a reader to infer
the second from a sentence about money.

That direction matters enough to state twice. This widens what matches, and
section 2.6.1's design exists largely to stop rules quietly matching more than
they say. Correct behaviour arriving unannounced is still unannounced.

Three tests added for the three cases the old set could not have caught:
cross-representation equality from readTree, NaN, and IN/NOT_IN -- the last
because section 2.6.1 defines IN as EQ against each element, so it inherits all
of this through one comparator. It cannot drift, but "cannot drift" is a claim
about today's call graph and IN is the operator an author actually reaches for.

The pinned version hash was also too narrow to do its job. It covered strings
and integers, which are exactly the node kinds whose rendering will not move,
and omitted decimals, escaped and non-ASCII strings, containers, and tags --
tags being the one canonicalisation bug this project has actually shipped, since
canonicalise sorts them through a TreeSet to keep Set.copyOf's per-JVM salt out
of the hash. A pin that cannot catch what it was built to catch reads like
coverage while providing none. The fixture now carries all of them, and
perturbing a decimal's scale or a tag's spelling moves the hash as it should.

Also from the review: nodesEqual documents why it has no `left == right` fast
path, which looks free and is not -- with two NaNs unequal, a reference check
would make a node equal to itself while unequal to a structurally identical
twin, an identity-dependent answer worse than the depth exposure it saves; the
two spellings of the numeric predicate, compareTo() == 0 for containers and
stripTrailingZeros().equals() for scalars, now point at each other, since the
whole fix rests on them agreeing and no test compares them directly; the
performance note no longer implies the walk is free at the leaf, because a
numeric comparison allocates where IntNode.equals was a primitive compare; two
duplicate imports left by the previous commit's re-sort are gone, the sort now
deduplicating as well as ordering; and three comment lines it pushed past the
hundred-column limit are rewrapped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbNTKG6shEcXad7hpeUBkN

Author: wolpert

docs/rule-engine-spec.md

@@ -451,7 +451,19 @@ Three consequences worth stating out loud:
 
 **Type-compatibility classes** for the "wrong type" row: `{number}`, `{string}`, `{boolean}`, `{array}`, `{object}`. Comparison is defined *within* a class only. Two exceptions: `IN`/`NOT_IN` compare a scalar against array *elements* (an array literal is expected, not a mismatch), and `EQ` on two `object`/`array` values is structural — object key order does not matter, array element order does, and **numbers inside a container compare exactly as they do outside one**.
 
-> **Amendment (Jackson 3 migration).** This sentence read "is Jackson's structural `equals`" until the engine moved to Jackson 3. That was accurate while Jackson 2's `DecimalNode.equals` compared with `BigDecimal.compareTo`, which ignores scale, and so happened to agree with this engine's own numeric equality. Jackson 3 switched to `BigDecimal.equals`, which is scale-sensitive — so `{amount: 100.00}` and `{amount: 100.0}` stopped being equal inside a container while remaining equal as scalars. The sentence stayed literally true and quietly meant something else. Delegating equality to a library is only safe while the library agrees with you; `Comparisons` now walks containers itself and compares numbers through `Canonical` at every depth, which is what the `{number}` type class in this section implies. See `ReviewRegressionTest.ContainerNumericEquality`.
+> **Amendment (Jackson 3 migration).** This sentence read "is Jackson's structural `equals`" until the engine moved to Jackson 3, and delegating to a library is only safe while the library agrees with you. Jackson's node equality is *representation* equality — it distinguishes `IntNode` from `DoubleNode` from `DecimalNode`, and (from Jackson 3) one `DecimalNode` scale from another. This section puts all of them in one `{number}` class, so the two definitions were never the same thing; they merely agreed often enough for the difference to stay hidden.
+>
+> Three concrete consequences, all of which the container path now answers the same way the scalar path always did:
+>
+> | | before | now |
+> |---|---|---|
+> | `{a: 100.00}` vs `{a: 100.0}` | unequal *(Jackson 3 only)* | equal |
+> | `{a: 1}` vs `{a: 1.0}`, straight from `readTree` | unequal *(Jackson 2 as well)* | equal |
+> | `{a: NaN}` vs `{a: NaN}` | equal | unequal |
+>
+> Only the first was a Jackson 3 regression. The second is older and had nothing to do with the migration — `1` and `1.0` were already equal as scalars and unequal inside a container, and it needs no `BigDecimal` to reach, just ordinary parsed JSON. The migration exposed a narrow slice of a wider inconsistency and both are fixed together.
+>
+> **Note the direction: this widens what matches**, which is the outcome this section's design is otherwise built to avoid, so it is stated rather than left to be discovered. `Comparisons` walks containers itself and compares numbers through `Canonical` at every depth; `IN`/`NOT_IN` inherit it, being `EQ` against each element. Object key order still does not matter and array element order still does. See `ReviewRegressionTest.ContainerNumericEquality`.
 
 #### 2.6.2 Numeric canonicalization
 

rule-engine-core/src/main/java/com/codeheadsystems/rules/value/Comparisons.java

@@ -118,22 +118,36 @@ private static boolean equal(final JsonNode actual, final JsonNode literal) {
    * Structural equality over two containers, with numbers compared the way §2.6.1 compares them
    * everywhere else.
    *
-   * <p><strong>Not {@code JsonNode.equals}, and the difference is a defect this replaced.</strong>
-   * Jackson 2's {@code DecimalNode.equals} compared with {@code BigDecimal.compareTo}, which ignores
-   * scale; Jackson 3's uses {@code BigDecimal.equals}, which does not. Delegating to Jackson
-   * therefore made {@code 100.00} and {@code 100.0} unequal <em>inside a container</em> while
-   * {@link Canonical} kept them equal as scalars -- the same two numbers answering differently
-   * depending on nesting, which is precisely the kind of silently-wrong match §2.6.1 exists to
-   * prevent. Money written at two scales is the everyday way to hit it.
+   * <p><strong>Not {@code JsonNode.equals}, and a wider fix than the bug that prompted it.</strong>
+   * Jackson's node equality is <em>representation</em> equality: it separates {@code IntNode} from
+   * {@code DoubleNode} from {@code DecimalNode}, and (from Jackson 3) one {@code DecimalNode} scale
+   * from another. §2.6.1 puts all of them in one {@code {number}} class, and {@link Canonical}
+   * implements that for scalars. Delegating containers to Jackson therefore made the engine answer
+   * differently about the same two numbers depending on whether they sat inside an object.
+   *
+   * <p>The scale half of that split arrived with Jackson 3 -- its {@code DecimalNode.equals} uses
+   * {@code BigDecimal.equals} where Jackson 2 used {@code compareTo} -- and money written at two
+   * scales is the everyday way to meet it. <strong>The cross-representation half is older and was
+   * never a Jackson 3 regression at all</strong>: {@code {a: 1}} and {@code {a: 1.0}} straight out
+   * of {@code readTree} were already unequal here while {@code 1} and {@code 1.0} were equal as
+   * scalars. Both are fixed together, because both are the same disagreement.
+   *
+   * <p>Note the direction: this <em>widens</em> what matches, and §2.6.1's design is largely about
+   * not doing that quietly. It is stated in the amendment there, and pinned by
+   * {@code ReviewRegressionTest.ContainerNumericEquality} across scale, representation and NaN.
+   * Non-finite doubles go the other way -- Jackson called two NaNs equal, this does not -- which
+   * again is what the scalar path already did.
    *
    * <p>§2.6.1 originally read "{@code EQ} on two object/array values is Jackson's structural
    * equals". That sentence was true of Jackson 2 and stayed literally true of Jackson 3 while
-   * meaning something else, so the spec is amended rather than the code bent to it: the engine has
-   * one definition of numeric equality and applies it at every depth. The rest of the container
-   * contract is unchanged -- object key order does not matter, array element order does.
+   * meaning something else, so the spec is amended rather than the code bent to it. The rest of the
+   * container contract is unchanged -- object key order does not matter, array element order does.
    *
-   * <p>Only container {@code EQ} pays for this walk, and {@code JsonNode.equals} was already a deep
-   * walk, so the cost is a comparison per leaf rather than an extra traversal.
+   * <p>Cost: no extra traversal, since {@code JsonNode.equals} was already a deep walk, and nothing
+   * here is quadratic -- {@code properties()} is the live entry set and {@code get(key)} the same
+   * hash lookup Jackson already did. Not free at the leaf, though: a numeric comparison allocates
+   * {@code BigDecimal}s and {@code Optional}s where {@code IntNode.equals} was a primitive compare.
+   * That runs per alpha test per insert, not per fire cycle.
    *
    * @param left one container
    * @param right the other, already known to be the same kind
@@ -168,10 +182,25 @@ private static boolean structurallyEqual(final JsonNode left, final JsonNode rig
    * @return whether they are equal
    */
   private static boolean nodesEqual(final JsonNode left, final JsonNode right) {
+    /*
+     * Deliberately no `left == right` fast path. It looks free and is not: two NaNs are unequal
+     * here, so a reference check would make a node equal to ITSELF while unequal to a structurally
+     * identical twin -- an identity-dependent answer, which is worse than the cost it saves. The
+     * consequence is that comparing a node with itself walks; a self-referential node, which
+     * Jackson lets you build, overflows the stack rather than short-circuiting. JsonNode.equals had
+     * the same exposure for two distinct cyclic nodes, and a payload that IS its own constraint
+     * literal is already a contract violation.
+     */
     if (left.isNumber() && right.isNumber()) {
-      // Canonical.compare returns empty for a value BigDecimal cannot represent -- a non-finite
-      // double built in Java. Those compare unequal rather than throwing, matching what the scalar
-      // path does with them.
+      /*
+       * Canonical.compare, where the scalar path uses Canonical.hashKey -- compareTo() == 0 against
+       * stripTrailingZeros().equals(). Two spellings of one predicate, and the whole fix rests on
+       * them agreeing. They do, over every scale, signed zero and exponent form checked; if you
+       * change either, check the other, because no test compares them directly.
+       *
+       * Empty means a value BigDecimal cannot represent -- a non-finite double built in Java. Those
+       * compare unequal rather than throwing, matching what the scalar path does with them.
+       */
       final OptionalInt sign = Canonical.compare(left, right);
       return sign.isPresent() && sign.getAsInt() == 0;
     }

rule-engine-dsl/src/main/java/com/codeheadsystems/rules/dsl/RuleFileReader.java

@@ -136,7 +136,8 @@ private static Optional<Parsed> bind(final RuleSource source, final JsonNode tre
    * Turns a Jackson failure into a located diagnostic.
    *
    * <p><strong>{@code JacksonException} is unchecked under Jackson 3</strong>, where the Jackson 2
-   * exception it replaces here, {@code JsonProcessingException}, was checked. Nothing here changed shape, but the
+   * exception it replaces here, {@code JsonProcessingException}, was checked. Nothing here
+   * changed shape, but the
    * compiler no longer insists: deleting either catch above would now build clean and turn a
    * malformed rule file back into a raw stack trace in somebody's startup log, which is the exact
    * outcome the second catch's comment argues against. The catches are load-bearing on their own

rule-engine-testkit/src/test/java/com/codeheadsystems/rules/testkit/CompilerValidationTest.java

@@ -346,19 +346,35 @@ void versionHashIsPinned() {
      * and that is a decision to be made deliberately (bump COMPILER_VERSION and say why in the
      * commit) rather than discovered by a consumer whose stored version stopped matching.
      */
+    /*
+     * The fixture is chosen for what could MOVE, not for readability. A pin over strings and ints
+     * would have stayed green through exactly the rendering changes it exists to catch, so it
+     * carries: a BigDecimal with trailing zeros and a fractional double (scale and exponent form
+     * are where Jackson's number rendering could plausibly change); a string needing escaping and
+     * one outside ASCII; an object and an array literal (container rendering, and key order); a
+     * two-sided range; and two tags, because canonicalise sorts tags() through a TreeSet
+     * specifically to keep Set.copyOf's per-JVM iteration salt out of the hash -- the one
+     * canonicalisation bug this project has actually shipped, and the fixture had no tags at all.
+     */
     final CompiledRuleSet pinned = RuleCompiler.compile(List.of(Rules.rule("pinned")
         .salience(5)
+        .tag("zebra")
+        .tag("alpha")
         .when("o", "Order", pattern -> pattern
-            .eq("status", "PENDING")
+            .eq("status", "PENDING\t\"quoted\"\n")
+            .eq("note", "sale ends soon \u2014 \u00e9t\u00e9")
+            .eq("breakdown", Facts.obj("net", new java.math.BigDecimal("100.00")))
+            .eq("codes", Facts.array(1, 2.50d))
             .gt("total", 10000)
+            .between("weight", 0.5d, 99.750d)
             .in("region", "EU", "US"))
         .when("c", "Customer", pattern -> pattern.ref("id", "o.customerId"))
         .then(actions -> actions.emit("out", "id", Rules.ref("o.id")))
         .build()));
 
     assertThat(pinned.version())
         .describedAs("rule-set identity is a compatibility surface; see this test's comment")
-        .isEqualTo("sha256:80ead2110b810f97");
+        .isEqualTo("sha256:8049b5f6bd96b20d");
   }
 
   @Test

rule-engine-testkit/src/test/java/com/codeheadsystems/rules/testkit/ReviewRegressionTest.java

@@ -8,15 +8,13 @@
 import com.codeheadsystems.rules.compiler.RuleCompiler;
 import com.codeheadsystems.rules.fact.FactHandle;
 import com.codeheadsystems.rules.listener.RuleEngineListener;
-import com.codeheadsystems.rules.listener.RuleEngineListener;
 import com.codeheadsystems.rules.listener.SuppressReason;
 import com.codeheadsystems.rules.match.ActivationKey;
 import com.codeheadsystems.rules.rhs.RhsErrorHandler;
 import com.codeheadsystems.rules.rhs.StagedEffect;
 import com.codeheadsystems.rules.rule.Constraint;
 import com.codeheadsystems.rules.rule.FieldConstraint;
 import com.codeheadsystems.rules.rule.Operator;
-import com.codeheadsystems.rules.rule.Operator;
 import com.codeheadsystems.rules.rule.RuleDefinition;
 import com.codeheadsystems.rules.session.CollectingEventSink;
 import com.codeheadsystems.rules.session.CompiledRuleSet;
@@ -282,6 +280,55 @@ void scalarAgreesWithContainer() {
           Facts.obj("v", new java.math.BigDecimal("100.0")).get("v"))).isTrue();
     }
 
+    @Test
+    @DisplayName("cross-representation too, which was never a Jackson 3 regression at all")
+    void representationDoesNotDecideEquality() throws Exception {
+      /*
+       * The half of this that predates the migration. Jackson separates IntNode from DoubleNode as
+       * firmly as it separates two DecimalNode scales, so {a: 1} and {a: 1.0} were unequal inside a
+       * container under Jackson 2 as well -- while 1 and 1.0 were equal as scalars, because the
+       * scalar path went through Canonical. No BigDecimal required: readTree on ordinary JSON is
+       * enough, which is why the blast radius is wider than the money example suggests.
+       */
+      assertThat(Comparisons.test(Operator.EQ, Facts.json("{\"a\": 1}"), Facts.json("{\"a\": 1.0}")))
+          .describedAs("parsed {a:1} EQ parsed {a:1.0}").isTrue();
+      assertThat(Comparisons.test(Operator.EQ, Facts.array(1), Facts.array(1.0d)))
+          .describedAs("[1] EQ [1.0]").isTrue();
+      assertThat(Comparisons.test(Operator.EQ,
+          Facts.obj("a", 1), Facts.obj("a", new java.math.BigDecimal("1"))))
+          .describedAs("int against BigDecimal").isTrue();
+    }
+
+    @Test
+    @DisplayName("a non-finite double is not equal to itself, matching the scalar path")
+    void nonFiniteGoesTheOtherWay() {
+      // The one case that got STRICTER. Jackson called two NaNs equal; Canonical cannot represent
+      // one as a BigDecimal, so the scalar path has always said unequal, and the container path
+      // now agrees. Recorded because it moved, not because anyone should rely on it.
+      final ObjectNode left = Facts.obj("v", 1);
+      final ObjectNode right = Facts.obj("v", 1);
+      left.put("v", Double.NaN);
+      right.put("v", Double.NaN);
+
+      assertThat(Comparisons.test(Operator.EQ, left, right))
+          .describedAs("{v: NaN} EQ {v: NaN}").isFalse();
+      assertThat(Comparisons.test(Operator.EQ, left.get("v"), right.get("v")))
+          .describedAs("the scalar path, which always said this").isFalse();
+    }
+
+    @Test
+    @DisplayName("IN and NOT_IN inherit it, being EQ against each element")
+    void membershipInheritsContainerEquality() {
+      // §2.6.1 defines IN as EQ against each element, and in() delegates to the same comparator --
+      // so this cannot drift. Pinned anyway, because "cannot drift" is a claim about today's call
+      // graph and this is the operator an author actually reaches for.
+      final ObjectNode needle = Facts.obj("amount", new java.math.BigDecimal("100.00"));
+      final ArrayNode haystack = Facts.array(Facts.obj("amount", new java.math.BigDecimal("100.0")));
+
+      assertThat(Comparisons.test(Operator.IN, needle, haystack)).isTrue();
+      assertThat(Comparisons.test(Operator.NOT_IN, needle, haystack)).isFalse();
+    }
+
     @Test
     @DisplayName("§2.6.1's other container rules still hold: key order free, element order not")
     void structuralRulesUnchanged() {