test(predicates): extend the null-guard sweep to the arguments of stdlib calls - #1366
Merged
Merged
Conversation
The repo-wide predicate sweep greps the operands of ORDERING comparisons. A predicate whose only null-sensitive site is a function ARGUMENT has no ordering operator at all, so it passed vacuously — green, having checked nothing. daysBetween(null, ...) reaches BigInt(NaN) and throws inside the function, which the engine reports as `predicate failed to evaluate` and, from 17.0.0-rc.2, turns into a REJECTED ordinary save. Adds a balanced-paren scan of every function call in every authored predicate, deny-by-default against a measured null-tolerant allowlist, with a vacuity guard that asserts the matcher reaches a non-zero set of call sites and both fixture directions of the matcher itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMzCeNC4SZcPNBpE2zCVCg
Test-only diff, so the frontmatter is deliberately empty — the sanctioned "releases nothing" declaration, verified against the diff rather than assumed: git diff origin/main touches test/ and .changeset/ only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMzCeNC4SZcPNBpE2zCVCg
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
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.
Fixes #1115
The repo-wide predicate guard,
test/object-validation-predicates.test.ts, greps the operands of ordering comparisons for a!= nullguard. A predicate whose only null-sensitive site is a function argument has no ordering operator at all, so it passed vacuously — green, having checked nothing. This adds the missing half.The third abort route, measured in PR #1110:
has()does not cover it: a key that is present and null clearshas(record.f)and detonates the call anyway. That is why this is the!= nullfamily of guard, not thehas()family.⭐ Proven RED before it was accepted — the non-negotiable
Instrument named by the card: take
period_end_matches_calendar_periodoncrm_forecast, strip its!= nullguards, and the new rule must go red on it. Full log:ablate.sh, run under the shared verify lock.Mutation leg — proven to have reached disk before anything was measured:
RED, and red for the right reason — the rule names the predicate and both fields:
Restore leg — proven restored, not assumed:
The mutation was applied to a committed implementation, so the restore leg had a real reference point; the script carries
trap restore EXIT INT TERMwith an absoluteREPO_ROOT, and an empty hash is treated as failure rather than "nothing to compare".The measurement that justifies the whole card
Under that same mutation, 77 tests still passed — including every engine-driven totality test in the file. The new sweep was the only thing in the repo that caught the stripped predicate.
Why the engine half stays green, measured rather than assumed: with every field null,
record.period == "month"is false andrecord.period == "quarter"is false, so&&short-circuits anddaysBetween(…)is never reached. Only the mixed shape —periodandperiod_startset,period_endnull — reaches the call, and no sweep in this file constructs that shape. That is the card's claim, now a measurement.A matcher that finds zero call sites is green forever and worthless — and from the outside it is indistinguishable from a clean tree, which is the exact defect this card records. So the new sweep asserts its own reach first:
daysBetween,addDaysandaddMonthsby function;['period_end', 'period_start']. An innermost-only[^()]*match would seeaddMonths(…)and missrecord.period_end, the argument that actually detonates;How the rule decides
Deny by default. Every field read inside a function call must carry
record.f != nullin the same expression, unless the function is on a short list measured not to abort on null:has,isBlank,coalesce— the measurement is the table already in this file's HOUSE RULE block. The direction is the point: the next stdlib function someone reaches for is guarded before anyone measures it.isBlankhas to be exempt or the house rule's own "f holds no value" shape,(!has(record.f) || isBlank(record.f)), becomes an offender in 13 shipped predicates.String-literal contents are blanked (length preserved, offsets intact) so a regex literal like
"^[0-9]{4}-(01|04|07|10)-01"cannot confuse the paren balance and arecord.spelled inside a literal is not a phantom field read.No exemption list was needed: the tree is clean as it ships.
Scope
One thing for the reviewer to judge
src/objects/forecast.object.tscarries a comment saying this predicate "has no ordering operator, soobject-validation-predicates.test.ts's unguarded-comparison sweep does not cover it — the pin lives in this card's own test file instead." That sentence stays literally true (the unguarded-comparison sweep still does not cover it; this is a different sweep), but its implication is now weaker, since the class is covered repo-wide. I left it alone rather than widen a test-only diff intosrc/on my own judgement. Say the word and it is a two-line follow-up.Verification
pnpm verifygreen — all eight stages ran (validate,typecheck,lint,lint:i18n-gate,hygiene,hygiene:tokens,build,test): 145 test files, 3033 passed, 1 skipped, verdictcommand-exit 0from the shared verify lock.Re-run on the shipping sha
6dd49bdwith a clean tree:✓ source hygiene clean,✓ source token ratchet clean, and the sweep file78 passed (78)— up from 75, the three new tests.Changeset frontmatter is empty, and that was verified rather than assumed:
git diff origin/main --stattouchestest/and.changeset/only, so this PR ships nothing to HotCRM users.Generated by Claude Code