fix(formula): declare SCOPE_ROOTS as dyn in the strict env so a bare reference behind a root name is judged (#16412) - #17133
Conversation
…are reference behind a root name is judged `firstUndeclaredReference` reads the ONE error cel-js's checker returns and acts only on `Unknown variable: X`. The strict env it builds declared every `SCOPE_ROOTS` member as `map`, while the permissive env `celEngine.compile` type-checks in leaves the same names `dyn`. `map` carries no `==` / `<` / `+` overload, so an ordinary comparison on a root — or on an object field or flow variable sharing a root's name — compiled clean and then faulted `no such overload` in the strict env only. That fault took the single error slot and every undeclared reference behind it in the same source went unjudged: the helper answered `null`, the same value that means "every reference is rooted", and four published call sites read it as the second. The two environments now agree about these names, so the class cannot arise rather than being compensated for downstream. `dyn` is what the list's own doc-comment already claimed the declaration was for (member access, arithmetic and comparison on a root all deferring to runtime); `map` delivered only the first of the three. Measured on the twelve-row probe table the option was ruled against: 11 rows flip, 17 of 17 negative controls stay clean, and the card's five-row regression table is unchanged. The twelfth row is a CEL TYPE name, which CEL declares itself and no declaration here can reach — its strict-env message is byte-identical under a `map` and a `dyn` root declaration. Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift Check1 anchor(s) derived from 2 changed package(s); no hand-written page names any of them. What this run could not see
Coarse fallback — 11 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 31511f86f505461727148d8e7325e91c02598df7 && git checkout 31511f86f505461727148d8e7325e91c02598df7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 702614108578b56e948bb8cd2a3efa646f4876c2 d4e07dac778febe6bf987886f3f88c0c8dd6e28e && git checkout -B drift-repro 702614108578b56e948bb8cd2a3efa646f4876c2 && git merge --no-ff d4e07dac778febe6bf987886f3f88c0c8dd6e28e
node scripts/docs-audit/affected-docs.mjs --json 702614108578b56e948bb8cd2a3efa646f4876c2 |
…rict-env-scope-roots-dyn
Contract review at
|
Contract review (clause ②) — PASS WITH FINDINGS, F1 blocking · head
|
…n it; correct the flow-scope docblock Contract review found the round's control blind by construction. `inferCelType` shares `buildScopedEnv` with `firstUndeclaredReference`, so the `SCOPE_ROOTS` `map` -> `dyn` move lands on the published `inferExpressionType` as well — and the eight probe expressions the round used to check that surface contained no namespace root in an operand position, the only class the declaration governs. A control that cannot fail on the affected class measures nothing about it. Re-measured with a probe set chosen from what the environment governs rather than from what looks representative: the cross-product of every syntactic position where a root's declared type is consulted and both directions the `map` -> `dyn` move can push the answer, over all 27 published roots, plus controls that cannot observe the declaration. 96 probes through the built `dist`, 75 move, and every one of them moves `unknown` -> a concrete type: 37 to boolean, 34 to number, 4 to text. Nothing narrows to `unknown` and no concrete answer becomes a different concrete answer. It is a widening, and it is uniform — 54 of 54 root probes (27 roots x two operand shapes) move. No behaviour changes here. The changeset gains a paragraph declaring the widening on `inferExpressionType` and on `@objectstack/mcp`'s `validate_expression.inferredType`; `validate.test.ts` gains two pins, one on the widened class and one on the four things the declaration does NOT govern (member-access bases, the `map`-side overloads that must not have narrowed, undeclared-identifier neutrality, and a bare root staying unprovable). `flow-variable-scope.ts`'s docblock described the pre-fix behaviour as current and named the very example this change inverts (`config == 'x' && status == 'y' -> []`, now `['status']`). Corrected to what the code does, with the two first-error classes that remain open — a CEL type name and `has()` on a non-select argument — measured on both legs and stated as still losing every shadow in the source. Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU Co-authored-by: Claude <noreply@anthropic.com>
…rict-env-scope-roots-dyn
Standing down at the gate — contract review cannot run at tier right now · head
|
Fixes #16412
Clause-②: yes
Option C of the card's ruling (
5583983008), measure-first: the strict declaredness environment declaresSCOPE_ROOTSasdyninstead ofmap, so the two environments this package builds agree about those names and the widest first-error masking class cannot arise.What was wrong
firstUndeclaredReferenceasks cel-js's checker for the first undeclared identifier. That checker returns exactly ONE error and the helper acts only onUnknown variable: X, so when the first error is of another class every undeclared reference behind it in the same source goes unjudged and the answer isnull— the same value that means "every reference is rooted". Four published call sites read that answer and none can tell the two readings apart.The reachable way in was a disagreement inside this package:
buildScopedEnvdeclared everySCOPE_ROOTSmembermap, while the permissive envcelEngine.compiletype-checks in leaves themdyn.maphas no equality, ordering or arithmetic overload, so an ordinary comparison on a root — or on an object field or flow variable sharing a root's name — compiled clean and then faultedno such overloadin the strict env only, took the single error slot, and silenced everything behind it.An author reaches it by naming a field or a flow variable after a namespace root and reading it bare. On a metadata-editing form that is not a coincidence: ADR-0089 D3 binds the row under edit as
data, which is aSCOPE_ROOTSmember.The measurement
Twelve rows, run through the real consumers on a built tree, before and after. 11 of 12 flipped; 17 of 17 negative controls stayed clean; the card's five-row regression table is unchanged.
visibility-bare-identifierdata == 'x' && status == 'active'visibility-bare-identifierrecord == 'x' && status == 'active'visibility-bare-identifierconfig != null && status == 'active'visibility-bare-identifiertype == 'grid' && status == 'active'config == 'x' && status == 'y'[]['status']data == 'x' && status == 'y'[]['status']result > 1 && status == 'y'[]['status']validate.tsrecord scopedata == 'x' && status == 'qualified'ok=true, 0 errorsok=false, 1 errorvalidate.tsrecord scopeconfig != null && status == 'qualified'ok=trueok=falsevalidate.tsrecord scoperecord == 'x' && status == 'qualified'ok=trueok=falsevalidate.tsflattenedconfig == 'y' && amont == 'x'validate.tsflatteneddata == 'y' && amont == 'x'Why row 4 did not flip — measured here, not assumed
The dispatch predicted this row is out of reach because
typeis not aSCOPE_ROOTSmember. That is true (asserted from the published list, not copied:expect(SCOPE_ROOTS).not.toContain('type')), but the load-bearing measurement is stronger and was taken directly off the checker. Building the strict env twice — once with rootsmap, once with rootsdyn— and reading the raw first error:The offending operand's type in row 4 is
type, CEL's own type-value type, notmap. The message is byte-identical under both declarations, so the row is out of reach because CEL declares that identifier itself — no declaration this package makes touches it. The same holds forstring,intand the rest of the CEL type names. ⇒ 11/12 is the ceiling of option C by construction, and this row is now pinned rather than left to be rediscovered.Negative controls — the "cannot false-positive" property
17 sources in which every reference is rooted, or which are the legitimate CEL the narrowing exists to protect (
type(record.x) == string, comprehension macros including one whose macro variable shadows a field name, both guard idioms, optional chaining, stdlib calls, two roots in one source, a root used as a namespace). Each asserted across four surfaces at once. 17/17 clean before, 17/17 clean after, and zero fields changed on any of them — the comparison is field-by-field on the whole probe record, not just "still zero findings".inferCelTypesharesbuildScopedEnv, so its answers were pinned too: 8 expressions, all 8 unchanged."inferCelTypedoes share the env, and that is exactly why the claim does not follow. The control was blind by construction: none of its eight expressions used a namespace root as a direct operand, which is the only class the changed declaration governs, so it could not have failed on the affected class no matter what the change did. Re-measured properly in the next section.The published answer that DOES move —
inferExpressionType(F1)The change is one declaration: the CEL type of every
SCOPE_ROOTSname insidebuildScopedEnv. A declaration is observable exactly where the name's type is consulted, i.e. wherever a root sits as a direct operand — so the probe set was rebuilt from what the environment governs rather than from what looked representative: the cross-product of every syntactic position that consults a root's declared type and both directions themap->dynmove can push an answer, over all 27 published roots, plus controls that structurally cannot observe it. 96 probes, read through the builtdist, on both legs of the same ablation.75 of 96 move, and every one of them moves
unknown-> a concrete type: 37 toboolean, 34 tonumber, 4 totext. Nothing narrows tounknown, and no concrete answer becomes a different concrete answer. It is uniform across the published list — 54 of 54 root probes (27 roots x two operand shapes) move.result + 1unknownnumberrecord ? 1 : 2unknownnumberrecord - 1,record * 2,record / 2,record % 2unknownnumberrecord + "x",record ? "a" : "b"unknowntextdata == "x" ? "a" : "b",record > 1 ? "a" : "b"unknowntextrecord == "x",record != "x"unknownbooleanrecord < 1,record > 1,record <= 1,record >= 1unknownbooleanrecord && true, `record,!record`Controls that cannot observe the declaration, and do not move:
record.amount > 100(boolean),record.amount + 1(number),daysBetween(record.a, record.b) + 1(number),undeclared_field + 1(unknown), a barerecord(unknown). Controls for the OTHER direction — the overloadsmapdid carry, which a widening-only reading would never have checked — also do not move:size(record)(number),"a" in record(boolean),has(record.a)(boolean),record.all(k, k == "a")(boolean), andrecord == previous(boolean, becausemap == mapalready had an overload).⇒ This is a widening on a published surface, and the round did not declare it.
inferExpressionTypeis re-exported frompackages/formula/src/index.tsand consumed bypackages/mcp/src/mcp-http-tools.tsasvalidate_expression.inferredType. Thedynanswers are the truthful CEL types, so the code is right and does not change; a consumer keying off a concrete type sees strictly more expressions classified, never a different classification. It is now declared in the changeset and pinned on both sides invalidate.test.ts— one pin on the widened class, one on the four things the declaration does not govern.The card's open question, answered: reachability on all three consumers
The card said this was not measured and that it "decides whether this is a live reachability gap on those surfaces or only a latent one". It is LIVE on all three, four call sites, and each has a control.
validate-visibility-predicates.ts(firstBareIdentifier)data == 'x' && status == 'active'published clean; the rule's own message says the console then falls OPEN and the element renders unconditionallystatus == 'active'alone was always 1 finding;record.status == 'active'always 0flow-variable-scope.ts(bareRootsOfviawarnShadowedFieldReads)foundempty, getsnullon iteration 0 and terminates before judging anything, so every shadow in the source is lost, whatever it is named['status']validate.tsrecord-scope bare-ref (:688)celEngine.compile— which passes, because the permissive env leaves the rootsdynok=true, zero errors, and the formula ships to evaluate as null at runtime (#1928's class)status == 'qualified'alone was alwaysok=falsevalidate.tsunknown-field did-you-mean (:713)amont == 'x'alone always warnedWhat an author writes to get there: name an object field or a flow variable after one of the 27
SCOPE_ROOTSmembers —data,config,record,result,item,event,input,user,status-adjacent ordinary words — and read it bare in the first operand of an ordinary comparison. The compile gate does not stand in the way on any of the four, because it type-checks in the environment where those names are alreadydyn.What this does NOT close
⛔ Neither this nor the alternative design closes the
has()class. Rows 2 and 3 of the card's five-row table staynull, and #16118'shas(…)span mask invalidate-visibility-predicates.tsstays load-bearing — it is what makeshas(status) && other == 'x'report at that one surface, and nothing else has one. Not touched, not weakened, and now pinned as such.⛔ The CEL-type class stays open, for the measured reason above. The
type == 'grid'blind-spot pin invalidate-visibility-predicates.test.tswas read before anything near it moved, and it does not move: its rationale is that widening the regex onto the overload message would rejecttype(record.x) == string, which stays true and stays a negative control here.Ablation — both directions, on disk and through
distThe diff changes a checker's verdict, so the new assertions were proven to redden when the change is reverted.
'dyn'reverted to'map'inbuildScopedEnv, proven on disk by grep count before reading any result (registerVariable(root, 'dyn')1 -> 0,registerVariable(root, 'map')2 -> 3), then@objectstack/formularebuilt and its arrival indist/proven withnode scripts/ablation-dist-preflight.mjs— required because@objectstack/lint's tests resolve@objectstack/formulathroughdist/, a registeredKNOWN_UNALIASED_TEST_IMPORTSpair.git checkout HEAD --at the absolute path, blob hash compared against theHEADblob,git diff HEADempty andgit status --porcelainclean, rebuilt, and absence indist/proven with the preflight's--absent.trap … EXIT INT TERMwith absolute paths resolved fromgit rev-parse --show-toplevel.Numbers:
dist/markers (2 built files)validate.test.tsHEADdynback tomap)HEADblobf943fd1b21 of the new assertions redden under the mutation and every one goes green again on restore. The lint suites going red is itself the dist-arrival proof: they resolve
@objectstack/formulathroughdist/, so they could not have moved at all if the mutation had stayed insrc/.ablation-dist-preflight.mjs --absentcannot pass on a mutation leg of a revert-the-fix ablation — it reads a dirty tree as an unfinished restore leg and exits 1 by design. It is written for the restore leg of a delete-a-guard ablation. The restore leg here used it as intended and it passed (✓ dist/: marker present in 2 built files+✓ tree: working tree clean against HEAD); the mutation leg is evidenced by the marker counts in the table instead. First attempt also used a non-discriminating marker (registerVariable(root, "map")has two legitimate hits frombuildTypedEnv); it was re-run withregisterVariable(root, "dyn"), which is unique to the environment being changed.Verification
All at
d4e07dac7, after mergingorigin/mainin withscripts/pm/os-regen-merge.sh(⛔ not a bare merge) — the derivation warned STALE TREE twice, once per round, and was re-run green after each merge.@objectstack/formula29 files / 856 tests,@objectstack/lint103 files / 3692 tests, all passing (854 -> 856 is the F1 round's two pins). Before the new assertions those were 827 and 3666 with the behaviour change already in: the existing 4493 assertions are blind to all twelve differences, which is why each was probed and then written down.tsc --noEmitpluscheck:test-typecheck; the test layer is confirmed in the type-checked set for both, so the new assertions are covered rather than excluded.node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack(no path argument): 58 families, 58 run, reconciled:✓ dispatch-gates --ran: 58 derived famil(ies) accounted for — 58 run, 0 NOT-MEASURED. 56 exit 0. Two exit 3 = PREREQUISITE NOT MET, which is NOT MEASURED, not a pass and not a failure:check:dual-build-cjs-loadsandcheck:type-check-debtboth need a whole-farmpnpm build(83 packages have nodist/here) — declared to CI, not claimed. A third,check:lean-entry-closure, also exited 3 and was repaired locally by building@objectstack/objectql; it then measured green.check-changeset-fixed,check:authz-resolver,check:error-code-casing,check:filter-alias-parity— were run rather than read as silent. All four exit 0.npx eslint . --no-inline-config --format json, 6432 files, 0 errors, 0 warnings, exit 0, re-run at the final commitd4e07dac7(git rev-parse --short HEADfrom that same run) because a late commit moves exactly the reading a ratchet quotes.$?, never across a pipe.Round 2 — the contract review's findings, and what each one changed
The review returned PASS WITH FINDINGS with F1 blocking. The round is declaration-only: the narrowing, the 11/12 ceiling, the reachability reading, the semver level, the ADR-0087 disposition and every test assertion are unchanged.
inferExpressionType, above. Three carriers, no code: a changeset paragraph declaring it, two pins invalidate.test.ts, and the corrected claim in this body.packages/lint/src/flow-variable-scope.ts'sbareRootsOfdocblock described the pre-fix behaviour as current and named the very example this change inverts. Measured on both ablation legs and rewritten to what the code now does:config == 'x' && status == 'y'was[]and is now['status'], with the reverse order['status']on both legs as its control; and the paragraph now separates the class that is closed from the two that stay open, each measured on both legs and still losing every shadow in the source — a CEL type name (type == 'grid' && status == 'y'->[]) andhas()on a non-select argument (has(status) && other == 'x'->[]). ⛔ Nothing else inpackages/lint/srcnon-test files is touched.cel-engine.tsdoc corrections is not locatable inAGENTS.mdor.claude/. The citation was unsourced; the review checked both corrections and found them true, so they stay on their merits and the citation is not repeated.celEngine.evaluatereturns an error result; thenullispackages/objectql/src/engine.ts's mapping of it. One hop of attribution, no carrier change.Proof that this round changed no behaviour
packages/lint/src/flow-variable-scope.tstranspiled withremoveComments, sha256fcd50358687261d99eb377b6f4bcf951133d1c8df4f85c35ef3df63115ed6bc5before and after, identical, while its raw blob hash moves (948eaee2->431b2463). Comment-only, measured rather than asserted.distbyte-identity — the four runtime bundles built from this commit's parent and from this commit:packages/formula/dist/index.js,index.mjs,packages/lint/dist/index.js,index.cjs, 4 of 4 sha256-identical. The only other files this round touches are a changeset and a test file, neither of which is bundled.'dyn'reverted to'map'inbuildScopedEnvthrough a planted named marker so the marker's spelling is byte-identical insrcand indist; injected text present and deleted text absent on disk before anything was read; rebuilt; arrival indistproven bynode scripts/ablation-dist-preflight.mjs @objectstack/formula OS_ABLATION_16412_ROOT_TYPE(✓ marker present in 2 built files, leg classifiedmutate) plus an independent count (registerVariable(root, "dyn")1 -> 0). Under itvalidate.test.tsgoes 9 failed / 121 passed, the new widening pin among them; the new controls pin stays green, which is what a control must do. Restored withgit checkout HEAD -- ..., proven by a whole-treegit status --porcelainbeing empty and the blob hash equal to HEAD's, rebuilt, and re-proven absent (✓ marker absent from all 6 built files,✓ working tree clean against HEAD). Re-measured after restore, the 96-probe record is byte-identical to the pre-ablation reading.Acceptance notes (the card's 验收口径)
visibility-bare-identifiergoes silent for an identifier that also appears inside ahas()in the same predicate #16118'shas(…)mask is not the template here — it is disjoint from this class, it stays, and it is now pinned.inferExpressionTypewidening — re-measured from a probe set chosen by what the environment governs, declared in the changeset, and pinned on both sides. The lesson the review named is worth carrying past this card: when a change alters an environment rather than a call site, the probe set has to come from what that environment governs, not from what looks representative — a control that cannot fail on the affected class measures nothing about it.Out of scope, noted and not filed:
SCOPE_ROOTSclass: the two first-error classes named in the very next paragraph of that same changeset still lose every shadow in a source, whatever it is named. The two paragraphs are correct read together and the review passed them; the docblock this PR corrects now states the distinction exactly. Noted rather than edited, because the seat's order for this round is that the changeset's existing sentences stay and F1 is an addition. Successor: whoever next edits that changeset's prose.mapvsdynasymmetry also exists betweenbuildScopedEnvandbuildTypedEnv's two envs, where the roots staymap. That is deliberate there (a root is a container the type-soundness check declines to reason through, and the typed struct onrecord/previous/inputcarries the field types) and it is now written down in theSCOPE_ROOTSdoc-comment rather than left implicit. Successor: the next PR to touchbuildTypedEnv.🤖 Generated with Claude Code
https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
Generated by Claude Code