Skip to content

fix(driver-memory): decide null before comparing in the reference matcher - #13554

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13494-matcher-null-comparands
Aug 30, 2026
Merged

fix(driver-memory): decide null before comparing in the reference matcher#13554
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13494-matcher-null-comparands

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Fixes #13494
Fixes #13495
Fixes #13549

Three cards, one file, one claim. The reference matcher disagreed with the mingo query path beside it on three cells that all turn on a null.

The two roots — measured by executing the file, not inferred from the cards

The dispatch order's A1 hypothesis was that all three share ONE root. Measured: it is two roots, not one. The fold still holds — both roots are in one file, the fixes do not interact, and neither needs the other — but the PR's shape follows the measurement:

What changed

$eq joins the guard's allowlist, for the same reason its complement $ne was already on it: its arm decides the no-value case itself. The exemption is written over the operator, never over "the comparand is null" — see the hold below.

The $between arm decides comparability before it compares: a no-value row is not inside a range with a real bound, a valued row is not inside a range whose bound is absent, and the degenerate range whose both ends are absent selects the no-value rows. A range with one absent end selects nothing rather than everything.

Every moved cell lands on the live path's existing answer

16 matcher cells moved. Zero live cells moved. After the repair the two faces agree on every cell in the sweep except the ones held for the maintainer.

cell matcher before matcher after live path
{$eq: null}, key ABSENT [] ['3'] ['3']
{$eq: undefined}, key ABSENT [] ['3'] ['3']
{$between: [null,null]}, value null ['1','3'] ['3'] ['3']
{$between: [null,null]}, key ABSENT ['1'] [] []
{$between: [null,'z']}, value null ['1','3'] [] []
{$between: ['a',null]}, value null ['1','3'] [] []
{$between: ['a','b']}, value null ['1','3'] ['1'] ['1']
{$between: ['2026-07-01','2026-07-15']}, value null ['1','2','4'] ['1','2'] ['1','2']
{$between: [-1,1]} numeric, value null ['2','3'] ['2'] ['2']
{$between: [null,null]} numeric ['2','3'] ['3'] ['3']

The half-null bound ([null,'z'] / ['a',null]) was named by no card; it is the same arm and the same repair.

Why a comparison-only repair is not enough — the load-bearing measurement

Rewriting the arm as !(value >= min && value <= max) repairs every STRING cell in all three cards and silently leaves the numeric ones broken: null coerces to 0, so null >= -1 && null <= 1 is true and a null-valued row stays inside a numeric range. Every fixture in the three cards is ISO date strings, where null compares against NaN and the naive form looks correct.

Ablated on the committed implementation: the naive form fails exactly 3 of the new suite's 16 cases, all numeric, and passes the other 13. The comparability decision is what those 3 hold to the code.

The #13357 hold (Zone 1, R1) — byte-identical, proved by measurement

$in: [null] / $nin: [null] are needs-user-decision and were not touched, aligned, or pinned. This is exactly why the guard exemption is spelled over the operator $eq: an exemption written over "the comparand is null" would have moved these arms with it, which is the decision this repair must not make.

All six held cells, before and after, on the matcher:

held cell before after
{$in: [null]}, key ABSENT [] []
{$nin: [null]}, key ABSENT ['1','3'] ['1','3']
{$in: [null,'a']}, key ABSENT ['1'] ['1']
{$nin: [null,'a']}, key ABSENT ['3'] ['3']
{$in: null} on the 5-row fixture ['4'] ['4']
{$nin: null} on the 5-row fixture ['1','2','3','5'] ['1','2','3','5']

Their standing disagreement with the live path is preserved unchanged, in both directions. No test in this PR asserts any of them — pinning them in either direction would prejudge the ruling.

PR #13550's 16 pins (Zone 1, R2 · Zone 2, A2) — zero moved

memory-operator-key-clobber.test.ts was confirmed present in the tree before the sweep (the discriminating check that #13550 landed), and its 16 matcherIds(...) assertion sites across 13 test cases were run against BOTH matchers:

  • pre-fix matcher (the file reverted to df18120502): 13 passed (13)
  • post-fix matcher: 13 passed (13)

So A2 holds as measured: the pins sit in the agreeing set and a correct repair moves none of them. No pinned expectation was edited. The suite's header fence, which named #13549's defect as knowingly unrepaired, is updated to record that it is closed (R3) — prose only; the composition-law design that scores the live path against ITSELF is deliberately left as it is, because it is a statement about what a clobber test measures rather than a workaround for the divergence.

Consumers of match() (Zone 2, A3)

match is not exported from the package entry, so it has no cross-package consumers. 13 in-repo consumers, all inside packages/drivers/driver-memory/src/, all tests:

memory-driver-document-not · memory-driver-filter-logic-conformance · memory-empty-field-constraint · memory-exists-has-value-faces · memory-filter-text-conformance · memory-filter-vocabulary-refusal · memory-icontains · memory-like-pattern · memory-matcher-no-value-negated-operators · memory-matcher-not-null-safe · memory-matcher-or-semantics · memory-null-comparand-refusal · memory-operator-key-clobber

memory-driver.ts imports the module too, but takes getValueByPath only — it is the one non-test consumer, and no arm this PR changes is reachable from it. Everything else in the repo naming memory-matcher names it in prose.

Tests

packages/drivers/driver-memory/src/memory-matcher-null-value-and-comparand.test.ts, 16 cases. Every cell asserts the row set on BOTH faces in one call, against a literal expectation — comparing the two faces only to each other would be satisfied by both being wrong together.

Ablation, on the committed implementation, mutation proved on disk by grep of both the injected and the deleted text, restore under a trap with an absolute repo root and proved by a whole-tree git status --porcelain plus a HEAD-blob hash match:

leg new suite clobber pins
matcher reverted to df18120502 12 failed, 4 passed 13 passed
the naive >= / <= repair 3 failed, 13 passed
as committed 16 passed 13 passed

No rebuild leg applies: the tests resolve ./memory-matcher.js to package-local source rather than through exports to dist, demonstrated by the before/after measurements differing with no build between them.

Verified at 1198c18763, after merging origin/main at df18120502 (R5):

  • pnpm --filter @objectstack/driver-memory testTest Files 36 passed (36), Tests 975 passed (975)
  • pnpm --filter @objectstack/driver-memory typecheck — exit 0, and tsc --noEmit --listFiles confirms both edited test files and memory-matcher.ts are in the program (so the green covers the new tests rather than skipping them)
  • pnpm lint — whole repo, eslint . --no-inline-config, exit 0
  • the gate family derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the real change set: 33 of 36 green, including check:where-matcher, check:engine-double-contract, check:driver-conformance, check:cross-package-test-inputs, check:test-source-alias, check:type-check-coverage and check:nul-bytes

Three gates returned exit 3, and each prints its own verdict saying nothing was measured — none is a finding: check:dual-build-cjs-loads ("PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/ ... This is NOT a pass: nothing was measured"), check-test-completeness.mjs ("the local reading for this gate is NOT MEASURED. It is not a red"), and scripts/pm/check-half-states.mjs ("PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential"). CI measures the first two on a built tree.

Out of scope, filed not fixed

#13553 — the same class in the four ORDERING arms: on a NUMERIC column null coerces to 0, so a null-valued row satisfies $gt/$gte/$lt/$lte while the live path excludes it. Found by widening this fold's fixtures from strings to numbers. It is a different arm family with a target the #5332 lane recorded as uncovered, so it is recorded rather than repaired here.


Generated by Claude Code

…cher

The record-at-a-time matcher disagreed with the mingo query path beside it
on three cells that all turn on a null.

`$eq: null` did not match a row whose key was ABSENT, though it matched one
whose value was a stored null: the pre-switch guard short-circuited a missing
key to "no match" before the `$eq` arm ran, so one operator answered the two
readings of "no value" two ways. `$eq` joins the guard's allowlist for the same
reason its complement `$ne` was already on it — its arm decides the case
itself. The exemption is written over the OPERATOR, never over "the comparand
is null", so the held `$in: [null]` / `$nin: [null]` arms keep byte-identical
answers.

The `$between` arm was an exclusion test spelled with `<` and `>`. A relational
comparison against a null is false in both directions, so neither disjunct
fired and a bounded range stopped bounding: `[null, null]` matched every valued
row, and a null-valued row sat inside a well-formed range. The arm decides
comparability before comparing now. A comparison-only repair would have left
the numeric case broken — null coerces to 0, so a null-valued row stays inside
`[-1, 1]` — and that cell is pinned.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json df18120502b449b0b89ca19631cb595c21d1d91bpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 2f0f2b5cfc6c96f01ebd7796b6e92059aed66da2 — the merge of head 1198c1876329a68a207ecc541b9b2d647014aa08 into base df18120502b449b0b89ca19631cb595c21d1d91b, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 2f0f2b5cfc6c96f01ebd7796b6e92059aed66da2 && git checkout 2f0f2b5cfc6c96f01ebd7796b6e92059aed66da2
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin df18120502b449b0b89ca19631cb595c21d1d91b 1198c1876329a68a207ecc541b9b2d647014aa08 && git checkout -B drift-repro df18120502b449b0b89ca19631cb595c21d1d91b && git merge --no-ff 1198c1876329a68a207ecc541b9b2d647014aa08

node scripts/docs-audit/affected-docs.mjs --json df18120502b449b0b89ca19631cb595c21d1d91b

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Copy link
Copy Markdown
Collaborator Author

PM review — ACCEPT. Release held only on CI reaching full green.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Everything below I measured myself on head 1198c187 / base df18120502; I am not restating the report.

The two Zone 1 rulings hold STRUCTURALLY, which is stronger than holding by assertion

R1 — the #13357 hold. Verified two ways, and the second is what makes it durable:

  • git diff over memory-matcher.ts: zero changed lines mention $in / $nin.
  • The guard exemption is spelled op !== '$eq' — over the operator, in the allowlist beside $exists / $null / $ne. A comparand-shaped spelling ("the comparand is null") would have carried $in: [null] and $nin: [null] with it, which is precisely the decision the maintainer holds.

⭐ That is the difference between honouring a hold and being unable to violate it. The six before/after cells are welcome, but the operator-scoped exemption is why they cannot drift later.

R2 / R3 — #13550's pins. The only change to memory-operator-key-clobber.test.ts is prose; no assertion touched. And the decision to keep the composition-law design is right for the stated reason: scoring the live path against ITSELF is a statement about what a clobber test measures, not a workaround for the divergence — so closing the divergence correctly does not change it.

The load-bearing claim, executed

The claim that a comparison-only repair silently leaves numeric columns broken:

null >= -1 && null <= 1   =>  true    ← the naive repair keeps a null row INSIDE a numeric range
null <  -1 || null >  1   =>  false   ← the old exclusion test: neither disjunct fires

Confirmed. Both fixtures in all three cards are ISO date strings, where null compares against NaN and the naive form looks correct — so ablation leg 2 (3 of 16 failing, all numeric) is testing the thing that actually distinguishes the two repairs. This is the PR's strongest argument and it survives.

A1: refuted, and the refutation was the point

Two roots, not one. #13495 + #13549 share the $between arm; #13494 is the pre-switch guard, and the proof offered — $ne was already on the allowlist and correct on both readings throughout — does rule out "a failed comparison" as its mechanism. The fold was a PM efficiency call and the STOP condition asked only whether three PRs are needed; they are not, so it correctly did not dissolve.

Clause ②: I CONCUR it does not fire, and I verified the load-bearing half rather than accepting it

match is not re-exported from packages/drivers/driver-memory/src/index.ts. That is what makes "no public-surface widening" a measurement rather than an assertion, and it is also why the 13 enumerated consumers are all in-package.

One thing I want on the record, because a later reader could misread it as a regression

#13553 discloses that after this lands, $between will EXCLUDE a null-valued numeric row while $gte / $lte still INCLUDE it — even though the live path compiles $between into exactly those two. Before this PR the five arms were consistently wrong on numerics; after it, one is right and four are wrong.

That is correct scope discipline, not an oversight, and it is a strict improvement: one more arm now agrees with the live face, and repairing the other four would have widened the PR past its three cards on the seat's own judgment. It was disclosed rather than buried, and filed. ⛔ Nothing here should be "tidied up" by widening this PR.

Non-blocking, carried to the maintainer rather than settled here

{$between: [null, null]} had no target asserted by #13495. The tie-break adopted — agree with the live face of the same package — does not rule what "no value" means and every moved cell was verified onto an answer the live path already gave, so the platform gains no reading; it loses a disagreement. I checked the refutation of the alternative myself: service-analytics's NULL_CONTROL really does pin '{$between: [null, 5]} → lowered to two bounds', so refusing a null bound at the shape gate would break a pinned consumer. Option A stands.

If the maintainer wants a different reading of the degenerate range, it is one line in valueWithinRange and nothing outside this PR pins it.

Release conditions

CI at review time: 16 success, 2 skipped, 11 in progress, 0 red. ⛔ The bar is EVERY check green, not the required subset. On full green this goes draft → ready with auto-merge armed, and the merge queue does the merging — ⛔ never a hand-merge, never a queue bypass, never by me.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 30, 2026 23:34
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Aug 30, 2026
Merged via the queue into main with commit ff37576 Aug 30, 2026
34 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13494-matcher-null-comparands branch August 30, 2026 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment