From 96a2e405cf959530a9ee8334263e252d695e78b4 Mon Sep 17 00:00:00 2001 From: os-sam Date: Wed, 9 Sep 2026 12:19:21 +0000 Subject: [PATCH 1/2] fix(formula): declare `SCOPE_ROOTS` as `dyn` in the strict env so a bare reference behind a root name is judged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .changeset/strict-env-scope-roots-dyn.md | 69 +++++++++ packages/formula/src/cel-engine.ts | 112 +++++++++----- packages/formula/src/validate.test.ts | 137 +++++++++++++++++- packages/lint/src/flow-variable-scope.test.ts | 58 ++++++++ .../validate-visibility-predicates.test.ts | 80 ++++++++++ 5 files changed, 420 insertions(+), 36 deletions(-) create mode 100644 .changeset/strict-env-scope-roots-dyn.md diff --git a/.changeset/strict-env-scope-roots-dyn.md b/.changeset/strict-env-scope-roots-dyn.md new file mode 100644 index 0000000000..cf1b7d3119 --- /dev/null +++ b/.changeset/strict-env-scope-roots-dyn.md @@ -0,0 +1,69 @@ +--- +"@objectstack/formula": minor +"@objectstack/lint": minor +--- + +fix(formula): the strict declaredness env declares `SCOPE_ROOTS` as `dyn`, so a bare reference behind a root name is no longer masked (#16412) + + + +**BREAKING** in the accept-set sense — an accept-set narrowing on published +CHECKERS, in the same sense as a route that starts refusing a request it should +always have refused — landing in the launch window as `minor` on both packages (during the window the bump level is +not the carrier of breaking-ness; this paragraph and the disposition above +are). Nothing that was already reported stops being reported, and no source +that is correct starts being reported. + +`firstUndeclaredReference` asks cel-js's checker for the first undeclared +identifier in a source. That checker returns exactly ONE error, and the helper +acts only on `Unknown variable: X`, so whenever the first error is of another +class every undeclared reference behind it in the same source went unjudged and +the helper answered `null` — which is also the value that means "every +reference is rooted". Four published call sites read that answer, and none of +them can tell the two readings apart. + +The widest way to reach that state was a disagreement between two environments +in this package about the same names. The strict env declared every +`SCOPE_ROOTS` member (`data`, `config`, `record`, `result`, `item`, `event`, +`input`, `user`, …) as `map`, while the permissive env that `celEngine.compile` +type-checks in leaves them `dyn`. `map` has no `==`, `<` or `+` overload, so an +ordinary comparison on one of those names compiled clean and then faulted `no +such overload` in the strict env only — taking the single error slot and +silencing everything behind it. An author reaches it by naming an object field +or a flow variable after a namespace root and reading it bare, which on a +metadata-editing form is not even a coincidence: that layer binds the row under +edit as `data`. + +The strict env now declares those roots `dyn`, which is what the list's own +doc-comment already claimed it was for — member access, arithmetic and +comparison on a root all deferring to runtime — and which `map` delivered only +the first of. The two environments agree about these names, so the class cannot +arise rather than being compensated for downstream. + +What starts reporting, measured on each published surface: + +- `@objectstack/formula` `validateExpression` with `scope: 'record'` — a bare + reference behind a root name is the hard error it always was for the same + identifier written first (`ok` was `true` with zero errors; it is now `false`). +- `@objectstack/formula` `validateExpression` with `scope: 'flattened'` — the + did-you-mean warning reaches a misspelled field behind a root name. +- `@objectstack/lint` `visibility-bare-identifier` — a bare identifier behind a + root name in a `visibleWhen` predicate is a finding. Per that rule's own + message the console otherwise falls open and the element renders + unconditionally. +- `@objectstack/lint` flow-variable shadowing — a shadowed field read behind a + root name is warned. That rule's documented blind spot is now name-local, as + its wording always claimed: the colliding name itself is still not reported. + +⛔ Two first-error classes are NOT closed by this, and both stay pinned. A CEL +TYPE name (`type`, `string`, `int`, …) is declared by CEL itself, so no +declaration this package makes can reach it; measured on the strict env, the +message for `type == 'grid'` is byte-identical under a `map` and a `dyn` root +declaration. And `has()` handed a non-select argument still faults its own +class, which `@objectstack/lint`'s visibility rule masks at its own call site +(#16118) and which nothing else masks. + +The narrowing this helper is built on is unchanged: it still acts only on +`Unknown variable`, so `type(record.x) == string`, comprehension macros, guard +idioms, optional chaining and stdlib calls report nothing, and a widening of +that regex onto the overload message remains refused. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 88f74c9c99..f943fd1b57 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -65,11 +65,20 @@ export function buildEnv(now: () => Date, timezone = 'UTC'): Environment { /** * Namespace roots that a `record`-scoped CEL site may legitimately reference. - * Declared as `map` (dyn values) so member access (`record.foo`) and any - * arithmetic/comparison on it defers to runtime — the strict env faults ONLY on - * an *undeclared* top-level identifier, i.e. a bare field reference. Generous on - * purpose: an unknown root is a missed catch, a missing root is a false positive - * that would break the build, so we err toward declaring more. + * Declared so that member access (`record.foo`) and any arithmetic/comparison on + * a root both defer to runtime — the strict env faults ONLY on an *undeclared* + * top-level identifier, i.e. a bare field reference. Generous on purpose: an + * unknown root is a missed catch, a missing root is a false positive that would + * break the build, so we err toward declaring more. + * + * ⚠️ The CEL type each env declares them AS is not uniform, and the difference is + * load-bearing rather than incidental. {@link buildScopedEnv} — the declaredness + * oracle — declares them `dyn`, because only `dyn` delivers BOTH halves of the + * sentence above; `map` delivered member access and faulted the comparison, and + * #16412 measured what that cost (see {@link firstUndeclaredReference}). The + * type-soundness envs keep them `map`: there a root is a container the check + * deliberately declines to reason through, and the typed struct on + * `record`/`previous`/`input` is what carries the field types. * * ## Why this list is PUBLISHED (#6713) * @@ -152,8 +161,28 @@ function buildScopedEnv(knownFields: readonly string[]): Environment { limits: DEFAULT_LIMITS, }); registerStdLib(env, () => new Date(0)); + // Roots are `dyn`, NOT `map`, for exactly the reason `knownFields` are (below) + // and the doc-comment on {@link SCOPE_ROOTS} already claims: member access, + // arithmetic and comparison on a root must all defer to runtime, so that the + // ONLY thing this env faults on is an undeclared top-level identifier. + // + // `map` delivered the member half and not the other two (#16412): `map` has no + // `==` / `<` / `+` overload, so `data == 'x'` — a root, or an object field + // sharing a root's name, in an ordinary comparison — faulted `no such + // overload` HERE while compiling clean in the permissive env, where the same + // names are `dyn`. Because cel-js's checker reports exactly ONE error, that + // fault took the first slot and every undeclared reference behind it in the + // same source went unjudged: `firstUndeclaredReference` answered `null`, the + // value that also means "every reference is rooted", and four consuming rules + // published the source clean. The two environments now agree about these + // names, so the class cannot arise rather than being compensated for + // downstream. + // + // ⛔ This does NOT weaken the check: `dyn` is undeclared-identifier-neutral — + // it changes what is legal ON a declared root, never whether an UNdeclared + // name faults. The `knownFields` loop below has always relied on that. for (const root of SCOPE_ROOTS) { - try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ } + try { env.registerVariable(root, 'dyn'); } catch { /* duplicate — ignore */ } } // `knownFields` are declared as `dyn` so they (and member/arith/compare on // them) never fault — only a genuinely-undeclared top-level identifier does. @@ -194,35 +223,48 @@ let recordScopeEnv: Environment | undefined; * * The masking is POSITIONAL, not name-keyed: the masked name is not the one * that triggered the first error, so excluding the trigger's own name does not - * reach it. Measured on this env: - * - * data == 'x' && status == 'q' -> null first error `no such - * overload: map == - * string`; `status` unjudged - * status == 'q' && data == 'x' -> "status" first error `Unknown - * variable: status` + * reach it. * * ⚠️ {@link celEngine.compile} is not a gate against this, so a caller that * only reaches here on a clean compile is not protected by that gate. `compile` * type-checks in the PERMISSIVE env ({@link CEL_ENV_OPTIONS}, - * `unlistedVariablesAreDyn: true`), and the two error classes that reach the - * first slot from ordinary authored input fault only HERE: - * - * - a {@link SCOPE_ROOTS} member -- or an object field sharing one of those - * names (`data`, `config`, `result`, `item`, `event`, `input`, `user`, …) -- - * as the operand of an operator with no `map` overload, because this env - * declares those roots `map` while the permissive one leaves them `dyn`; - * - a CEL TYPE name (`type`, `string`, `int`, …) in the same position, already - * pinned as a blind spot by `@objectstack/lint`'s `visibility-bare-identifier` - * suite -- pinned there per NAME, while the masking it causes is source-wide. - * - * ⛔ Do not close this by widening the regex onto the overload message: that - * false positive is precisely what the narrowing buys off (`type(record.x) == - * string` is legitimate CEL). Reporting past the first error needs a re-check - * loop over a neutralised source, or a checker entry that returns more than one - * error -- cel-js 8.0.0 has none, its `TypeCheckResult` carries a single - * `error` -- and either one changes what every consuming rule reports. That is - * a design decision, not a patch. + * `unlistedVariablesAreDyn: true`), so a source can compile clean and still + * fault HERE. + * + * ## What is CLOSED, and what is still open (#16412) + * + * ⭐ CLOSED — the {@link SCOPE_ROOTS} class. It was by far the wider of the two + * reachable ones: a root, or an object field sharing a root's name (`data`, + * `config`, `result`, `item`, `event`, `input`, `user`, …), used as the operand + * of an operator with no `map` overload. {@link buildScopedEnv} declared those + * roots `map` while the permissive env left them `dyn`, and that DISAGREEMENT + * was the whole mechanism; the roots are now `dyn` in both, so the class cannot + * arise. Measured on this env, before → after: + * + * data == 'x' && status == 'q' null -> "status" the class, closed + * status == 'q' && data == 'x' "status" unchanged, the control + * + * ⛔ STILL OPEN — every OTHER first-error class, and the mechanism above is + * untouched for them. Two are reachable from authored input: + * + * - a CEL TYPE name (`type`, `string`, `int`, …) in that same position. CEL + * itself declares those names, so no declaration this package controls can + * move them; `type == 'grid' && status == 'q'` still answers `null`. Pinned + * per NAME by `@objectstack/lint`'s `visibility-bare-identifier` suite, while + * the masking it causes is source-wide. + * - `has()` handed a non-select argument (`has(status) && other == 'x'`), which + * faults `has() invalid argument`. `@objectstack/lint`'s + * `validate-visibility-predicates` masks `has(…)` spans at its own call site + * (#16118) and that mask stays load-bearing; no other consumer has one. + * + * ⇒ A `null` from this helper is still "nothing was reported", never "the + * source is clean". ⛔ Do not close the remaining classes by widening the regex + * onto the overload message: that false positive is precisely what the + * narrowing buys off (`type(record.x) == string` is legitimate CEL). Reporting + * past the first error needs a re-check loop over a neutralised source, or a + * checker entry that returns more than one error -- cel-js 8.0.0 has none, its + * `TypeCheckResult` carries a single `error` -- and either one changes what + * every consuming rule reports. That is a design decision, not a patch. */ export function firstUndeclaredReference( source: string, @@ -598,10 +640,10 @@ export function parseCelToAstWithReason( * expression — its raw CEL type name (`'int'`, `'double'`, `'string'`, `'bool'`, * `'google.protobuf.Timestamp'`, `'dyn'`, …) — or `null` when the expression does * not type-check. Reuses the SAME record-scoped, stdlib-registered env as - * {@link firstUndeclaredReference}: namespace roots (`record`, `previous`, …) are - * declared `map` and `knownFields` are declared `dyn`, so both `record.` - * and bare `` references resolve while every stdlib call carries its - * declared return type. + * {@link firstUndeclaredReference}: namespace roots (`record`, `previous`, …) and + * `knownFields` are both declared `dyn`, so both `record.` and bare + * `` references resolve while every stdlib call carries its declared + * return type. * * Deliberately conservative. A member access (`record.amount`) or a bare field is * `dyn`, and an operator over two `dyn` operands stays `dyn` (cel-js cannot prove diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index c651659b57..0c483ddaac 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -7,7 +7,7 @@ import { nearestName, CEL_STDLIB_FUNCTIONS, } from './validate'; -import { buildEnv, firstUndeclaredReference } from './cel-engine'; +import { buildEnv, firstUndeclaredReference, SCOPE_ROOTS } from './cel-engine'; describe('validateExpression (ADR-0032)', () => { describe('predicates (CEL)', () => { @@ -538,6 +538,141 @@ describe('validateExpression (ADR-0032)', () => { it('does not flag a null-guard on a record-qualified field (no type false-positive)', () => { expect(validateExpression('predicate', 'record.lead_score != null && record.lead_score > 100', { scope: 'record' }).ok).toBe(true); }); + + /** + * ── #16412: a bare ref BEHIND a `SCOPE_ROOTS` name is judged ─────── + * + * `firstUndeclaredReference` reads the ONE error cel-js's checker returns, + * and acts only on `Unknown variable: X`. The strict env used to declare + * `SCOPE_ROOTS` as `map` while the permissive compile env leaves the same + * names `dyn`, so `data == 'x'` — a root, or an object field sharing a + * root's name — compiled clean and then faulted `no such overload: + * map == string` HERE. That fault took the single error slot and + * every bare reference behind it in the same source went unjudged: the + * helper answered `null`, which is also the value meaning "every reference + * is rooted", and this hard error was downgraded to silence. + * + * The roots are now `dyn` in both environments, so the class cannot arise. + * These pin the VERDICT, not the mechanism: they go red if the strict env's + * root declaration regresses to `map`, whatever the message then says. + */ + describe('a bare ref behind a `SCOPE_ROOTS` name is still an error (#16412)', () => { + it.each([ + ['data', "data == 'x' && status == 'qualified'"], + ['config', "config != null && status == 'qualified'"], + ['record', "record == 'x' && status == 'qualified'"], + ['result', "result > 1 && status == 'qualified'"], + ['item', "item == 'x' && status == 'qualified'"], + ])('%s as the FIRST operand does not mask `status`', (_root, source) => { + const r = validateExpression('predicate', source, { scope: 'record' }); + expect(r.ok).toBe(false); + expect(r.errors).toHaveLength(1); + expect(r.errors[0].message).toMatch(/bare reference `status`/); + expect(r.errors[0].message).toMatch(/record\.status/); + }); + + it('gives the same verdict in both operand orders — the masking was POSITIONAL', () => { + // The pre-fix asymmetry, and the whole reason the defect was invisible: + // one order reported, the other published clean. Both must report now, + // and the second is the control that was already correct. + const masked = validateExpression('predicate', "data == 'x' && status == 'qualified'", { scope: 'record' }); + const control = validateExpression('predicate', "status == 'qualified' && data == 'x'", { scope: 'record' }); + expect(masked.ok).toBe(false); + expect(control.ok).toBe(false); + expect(masked.errors[0].message).toBe(control.errors[0].message); + }); + + it('reports the BARE name, never the root that used to mask it', () => { + const r = validateExpression('predicate', "config != null && status == 'qualified'", { scope: 'record' }); + expect(r.errors[0].message).not.toMatch(/bare reference `config`/); + expect(r.errors[0].message).toMatch(/bare reference `status`/); + }); + + it('the flattened did-you-mean reaches a typo behind a root name too', () => { + // The same masking, one severity down: `validateExpression`'s flattened + // arm never saw the typo, so a misspelled field shipped unwarned. + const r = validateExpression('predicate', "config == 'y' && amont == 'x'", { + objectName: 'crm_opportunity', fields: ['amount', 'status'], scope: 'flattened', + }); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/`amont` is not a field/); + expect(r.warnings[0].message).toMatch(/did you mean `amount`/); + }); + + /** + * ⛔ The narrowing this helper is built on is NOT relaxed. Every source + * here has every reference rooted, or is legitimate CEL the overload + * message cannot be told apart from — none may start reporting. This is + * the property a widening of the regex onto the overload message would + * destroy, and it is the one most easily broken while closing a false + * negative. + */ + it.each([ + ['record.status == "x"', 'the canonical dotted spelling'], + ['type(record.x) == string', 'legitimate CEL an overload-message widening would reject'], + ['record.a + record.b > 1', 'arithmetic over two rooted members'], + ['previous.status != record.status', 'two roots in one source'], + ['parent.type == "grid" && record.status == "x"', 'a declared root, then a rooted term'], + ['current_user.id != null', 'the ADR-0068 canonical user root'], + ["record.tags.all(t, t != '')", 'a comprehension macro'], + ['record.lines.exists(status, status.ok)', 'a macro variable shadowing a field name'], + ['size(record.tags) > 0', 'a cel-js built-in'], + ['record.?name.orValue("x") == "x"', 'optional chaining'], + ])('%s stays clean (%s)', (source) => { + const r = validateExpression('predicate', source, { scope: 'record' }); + expect(r.ok).toBe(true); + expect(r.errors).toEqual([]); + }); + }); + + /** + * ⛔ #16412 closes the `SCOPE_ROOTS` class ONLY. Every other first-error + * class still masks what is behind it, and pinning that keeps the next + * reader from mistaking a narrowed fix for a general one. + * + * - a CEL TYPE name (`type`, `string`, `int`, …) is declared by CEL itself, + * so no declaration this package makes can reach it. Measured on the + * strict env: `type == 'grid' && …` faults `no such overload: type == + * string` under BOTH a `map` and a `dyn` root declaration — byte-identical + * messages — which is why this row is out of the fix's reach by + * construction rather than by omission. + * - `has()` handed a non-select argument faults `has() invalid argument`. + * `@objectstack/lint`'s visibility rule masks `has(…)` spans at its own + * call site (#16118); nothing here does. + */ + describe('the first-error classes #16412 does NOT close', () => { + it.each([ + ["type == 'grid' && status == 'qualified'", 'a CEL type name — CEL declares it, not `SCOPE_ROOTS`'], + ["string == 'x' && status == 'qualified'", 'the same class, another type name'], + ['has(status) && other == "x"', 'the `has()` class — #16118 masks this at the lint call site'], + ])('%s is still masked (%s)', (source) => { + expect(firstUndeclaredReference(source)).toBeNull(); + }); + + it('`type` is not a `SCOPE_ROOTS` member — read from the list, not copied', () => { + // Reads the published baseline so the claim above cannot go stale + // silently if the list ever gains the name. + expect(SCOPE_ROOTS as readonly string[]).not.toContain('type'); + }); + + /** + * #16412's own five-row probe table, pinned whole rather than by its one + * interesting row. Rows 1 and 5 are what the helper already got right and + * must keep; rows 2 and 3 are the `has()` class, unchanged by this fix; + * row 4 is the positional control that made the defect legible (rows 3 + * and 4 are the same two sub-expressions in the other order). + */ + it.each([ + ['status == "qualified"', 'status'], + ['has(status) && status == "qualified"', null], + ['has(status) && other == "x"', null], + ['other == "x" && has(status)', 'other'], + ['has(record.status) && status == "qualified"', 'status'], + ])('%s -> %s', (source, expected) => { + expect(firstUndeclaredReference(source)).toBe(expected); + }); + }); }); // #1928 tier 3 — flattened flow conditions reference fields bare, so a bare diff --git a/packages/lint/src/flow-variable-scope.test.ts b/packages/lint/src/flow-variable-scope.test.ts index 7a027f9083..896fcf6b5d 100644 --- a/packages/lint/src/flow-variable-scope.test.ts +++ b/packages/lint/src/flow-variable-scope.test.ts @@ -183,6 +183,64 @@ describe('shadowedFieldReads (#14089)', () => { expect(SCOPE_ROOTS.length).toBeGreaterThan(0); expect(shadowedFieldReads(`${root} == "x"`, vars(root), [root])).toEqual([]); }); + + /** + * ── #16412: the blind spot above is name-local again, as written ──── + * + * It used to be far wider than its own wording, and that is what #16412 + * measured. `SCOPE_ROOTS` were declared `map` in the strict env, so a root + * name — or a field/variable sharing one — used with an ordinary operator + * faulted `no such overload` rather than `Unknown variable`. cel-js returns + * ONE error, so that fault took the slot, `bareRootsOf`'s loop got `null` on + * iteration 0 and terminated before judging anything, and EVERY shadow in + * that source was lost whatever it was named. The source still compiled (the + * permissive env leaves those roots `dyn`), so no sibling diagnostic fired + * either. + * + * The roots are `dyn` in both environments now. What survives is exactly what + * the pin above says: the colliding NAME itself is still not reported. + */ + it.each([ + ['config', "config == 'x' && status == 'y'"], + ['data', "data == 'x' && status == 'y'"], + ['result', "result > 1 && status == 'y'"], + ['item', "item == 'x' && status == 'y'"], + ])('a %s-named first operand no longer swallows every other shadow in the source', (root, source) => { + expect(SCOPE_ROOTS as readonly string[]).toContain(root); + expect(shadowedFieldReads(source, vars('status', root), ['status', root])).toEqual(['status']); + }); + + it('the loop reaches shadows in either order — the masking was POSITIONAL', () => { + // Same two sub-expressions, both orders. The second was already correct + // before #16412 and is the control: it is what made the first legible. + const masked = shadowedFieldReads("data == 'x' && status == 'y'", vars('status', 'data'), ['status', 'data']); + const control = shadowedFieldReads("status == 'y' && data == 'x'", vars('status', 'data'), ['status', 'data']); + expect(masked).toEqual(control); + expect(masked).toEqual(['status']); + }); + + it('still collects EVERY shadow behind a root name, not just the first', () => { + const found = shadowedFieldReads( + "config == 'x' && status == 'y' && amount > 1", + vars('status', 'amount', 'config'), + ['status', 'amount', 'config'], + ); + expect(found.sort()).toEqual(['amount', 'status']); + }); + + /** + * ⛔ The under-report direction is preserved: closing the `SCOPE_ROOTS` class + * must not start reporting anything a rooted or macro-bound source contains. + * These are the false positives the pinned oracle exists to avoid. + */ + it.each([ + ['record.status == "x"', 'the dotted spelling this rule prescribes'], + ['record.lines.exists(status, status.ok)', 'a comprehension-macro variable sharing a field name'], + ['size(record.lines) > 0', 'a function name sharing a field name'], + ['data.status == "x" && record.status == "y"', 'a root used as a NAMESPACE, which is correct'], + ])('%s reports nothing (%s)', (source) => { + expect(shadowedFieldReads(source, vars('status', 'size', 'data'), ['status', 'size', 'data'])).toEqual([]); + }); }); describe('shadowedFieldMessage (#14089)', () => { diff --git a/packages/lint/src/validate-visibility-predicates.test.ts b/packages/lint/src/validate-visibility-predicates.test.ts index b5c5681c30..bd6e224603 100644 --- a/packages/lint/src/validate-visibility-predicates.test.ts +++ b/packages/lint/src/validate-visibility-predicates.test.ts @@ -674,6 +674,86 @@ describe('visibility-bare-identifier (#6128 / #5149 requirement 3)', () => { expect(bareFindings(formStack("record.type == 'grid'"))).toEqual([]); }); + /** + * ── #16412: the CEL-type blind spot is NAME-local, and now really is ── + * + * The pin above describes a missed catch on the name it pins. It used to + * cost far more than that, and the same was true of every `SCOPE_ROOTS` + * name: those roots were declared `map` in the strict env while the + * permissive compile env leaves them `dyn`, so an ordinary comparison on one + * faulted `no such overload` HERE and compiled clean THERE. cel-js returns + * exactly ONE error, so that fault took the slot and every bare identifier + * behind it in the same predicate went unjudged — the rule published the + * predicate clean, and per its own message the console then falls OPEN. + * + * The roots are `dyn` in both environments now, which closes the + * `SCOPE_ROOTS` half. ⛔ The CEL-type half above is NOT closed and cannot be + * from here: `type` is not a `SCOPE_ROOTS` member, so no declaration + * `@objectstack/formula` makes reaches it (measured: the strict env's message + * for `type == 'grid'` is byte-identical under a `map` and a `dyn` root + * declaration). The row below pins that remaining cost so it stays visible. + */ + describe('a bare identifier behind a namespace-root name is reported (#16412)', () => { + it.each([ + ['data', "data == 'x' && status == 'active'"], + ['record', "record == 'x' && status == 'active'"], + ['config', "config != null && status == 'active'"], + ['result', "result > 1 && status == 'active'"], + ])('%s as the first operand no longer masks `status`', (_root, predicate) => { + const findings = bareFindings(formStack(predicate)); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('`status`'); + expect(findings[0].hint).toContain('`record.status`'); + }); + + it('names the BARE identifier, never the root that used to mask it', () => { + const findings = bareFindings(formStack("config != null && status == 'active'")); + expect(findings[0].message).not.toContain('`config`'); + }); + + it('emits exactly ONE finding — no double report on a predicate that compiles', () => { + // The predicate type-checks in the permissive env, so no sibling rule + // fires beside this one; the author gets a single verdict, not two. + expect(validateVisibilityPredicates(formStack("data == 'x' && status == 'active'")).map((f) => f.rule)) + .toEqual([VISIBILITY_BARE_IDENTIFIER]); + }); + + it('⛔ the CEL-type half of the masking stays open — pinned, not fixed', () => { + // `type` is CEL's own declaration, out of reach of `SCOPE_ROOTS`. This is + // the one row of #16412's twelve that its fix does not flip, and it is + // recorded here rather than left to be rediscovered. + expect(bareFindings(formStack("type == 'grid' && status == 'active'"))).toEqual([]); + // The control that proves the sentence above is about `type` and not + // about the shape: same shape, a namespace root in the first operand. + expect(bareFindings(formStack("data == 'grid' && status == 'active'"))).toHaveLength(1); + }); + + it('⛔ the `has()` half stays closed by #16118 s call-site mask, not by this', () => { + // The mask is what makes these report; the oracle alone still answers + // `null` for a `has()` first error. If the mask is ever removed these go + // silent again, which is the whole reason it is load-bearing. + expect(bareFindings(formStack("has(status) && other == 'x'"))).toHaveLength(1); + expect(bareFindings(formStack('has(record.status)'))).toEqual([]); + }); + + /** + * ⛔ The narrowing is not relaxed anywhere: every predicate here has each + * reference rooted, or is legitimate CEL, and none may start reporting. + */ + it.each([ + ['record.status == "active"', 'the canonical dotted spelling'], + ['data.status == "x"', 'a root used as a NAMESPACE, which is what roots are for'], + ['type(record.x) == string', 'the legitimate CEL the blind-spot pin protects'], + ['previous.status != record.status', 'two roots in one predicate'], + ['parent.type == "grid" && record.status == "x"', 'a declared root, then a rooted term'], + ["record.tags.all(t, t != '')", 'a comprehension macro'], + ['size(record.tags) > 0', 'a cel-js built-in'], + ['record.?name.orValue("x") == "x"', 'optional chaining'], + ])('%s produces no bare-identifier finding (%s)', (predicate) => { + expect(bareFindings(formStack(predicate))).toEqual([]); + }); + }); + // ── The #4953 boundary, pinned rather than described ──────────────── // // #4953 measured the SAME evaluator giving opposite verdicts on a total vs a From 5087da6dcc6fbcf5ca44e550a7852290a5c88da9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 13:26:58 +0000 Subject: [PATCH 2/2] docs(formula,lint): declare the `inferExpressionType` widening and pin it; correct the flow-scope docblock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .changeset/strict-env-scope-roots-dyn.md | 19 ++++++++++ packages/formula/src/validate.test.ts | 45 ++++++++++++++++++++++++ packages/lint/src/flow-variable-scope.ts | 33 +++++++++++------ 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/.changeset/strict-env-scope-roots-dyn.md b/.changeset/strict-env-scope-roots-dyn.md index cf1b7d3119..a0ca4a33de 100644 --- a/.changeset/strict-env-scope-roots-dyn.md +++ b/.changeset/strict-env-scope-roots-dyn.md @@ -55,6 +55,25 @@ What starts reporting, measured on each published surface: root name is warned. That rule's documented blind spot is now name-local, as its wording always claimed: the colliding name itself is still not reported. +⚠️ One published answer also WIDENS, and it is not a reporting surface. +`inferExpressionType` (`@objectstack/formula`, re-exported from the package +root; read by `@objectstack/mcp` as `validate_expression.inferredType`) infers a +formula's coarse value type through `inferCelType`, which shares this same +strict environment. While the roots were `map` there was no `==`, `<` or `+` +overload for them, so an expression using a namespace root as a DIRECT OPERAND +did not type-check at all and the answer was `'unknown'`. With the roots `dyn` +those expressions type-check and the answer is the truthful CEL type: +`result + 1` and `record ? 1 : 2` → `'number'`, `record == "x"` → `'boolean'`, +`data == "x" ? "a" : "b"` → `'text'`, uniformly for every name on the list. No +answer changes from one concrete type to another and nothing narrows to +`'unknown'` — `size(record)` and `"a" in record` still answer, and a root that +is only the base of a member access (`record.amount > 100`) never consulted this +declaration. A consumer that keys off a concrete type therefore sees strictly +more expressions classified, never a different classification; for the +motivating consumer that means a formula written as `data == "x" ? "a" : "b"` is +now correctly seen as text rather than as unprovable. Pinned on both sides in +`validate.test.ts`. + ⛔ Two first-error classes are NOT closed by this, and both stay pinned. A CEL TYPE name (`type`, `string`, `int`, …) is declared by CEL itself, so no declaration this package makes can reach it; measured on the strict env, the diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index 0c483ddaac..3ce820f10b 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -987,4 +987,49 @@ describe('inferExpressionType — coarse value-type of a formula', () => { expect(inferExpressionType('no_such_fn(amount)', { fields })).toBe('unknown'); // no overload expect(inferExpressionType('undeclared_field + 1')).toBe('unknown'); // bare ref, no fields given }); + + // ## The #16412 widening, pinned on BOTH sides + // + // `inferCelType` shares `buildScopedEnv` with `firstUndeclaredReference`, so + // #16412's `SCOPE_ROOTS` `map` -> `dyn` move lands on THIS surface too — and + // this one is published: `inferExpressionType` is re-exported from the package + // root and read by `@objectstack/mcp` as `validate_expression.inferredType`. + // `map` had no `==` / `<` / `+` overload, so a root used as a DIRECT OPERAND + // failed to type-check at all and every such expression answered `'unknown'`; + // `dyn` propagates, so the answer is now the truthful CEL type. That is a + // WIDENING of a published answer — more expressions get a concrete type, none + // changes from one concrete type to another — and it is pinned here so it + // cannot drift back or drift further unnoticed. + it('types a namespace root used as a direct operand (#16412 widening)', () => { + // Every one of these answered `'unknown'` while the roots were `map`. + expect(inferExpressionType('result + 1')).toBe('number'); + expect(inferExpressionType('record == "x"')).toBe('boolean'); + expect(inferExpressionType('data == "x" ? "a" : "b"')).toBe('text'); + expect(inferExpressionType('record > 1')).toBe('boolean'); + expect(inferExpressionType('record ? 1 : 2')).toBe('number'); + // It is a property of the DECLARATION, not of any one name, so it is uniform + // across the published list — sampled across its distinct provenance groups. + for (const root of ['config', 'item', 'event', 'input', 'user', 'parent', 'current_user']) { + expect(inferExpressionType(`${root} + 1`)).toBe('number'); + } + }); + + it('moved nothing the root declaration does not govern (#16412 controls)', () => { + // A root that is only the BASE of a member access never consults the changed + // declaration: `record.amount` is `dyn` under `map` and under `dyn` alike. + expect(inferExpressionType('record.amount > 100')).toBe('boolean'); + expect(inferExpressionType('record.amount + 1')).toBe('number'); + // The other direction, which a widening-only reading would miss: the + // overloads `map` DID carry must not have narrowed to `unknown`. + expect(inferExpressionType('size(record)')).toBe('number'); + expect(inferExpressionType('"a" in record')).toBe('boolean'); + expect(inferExpressionType('has(record.a)')).toBe('boolean'); + // `dyn` is undeclared-identifier-neutral — it changes what is legal ON a + // declared root, never whether an UNdeclared name faults. This is the + // property the whole #16412 change rests on, asserted on this surface. + expect(inferExpressionType('undeclared_field + 1')).toBe('unknown'); + expect(inferExpressionType('undeclared_field == "x"')).toBe('unknown'); + // And a root alone is still not a provable type: `dyn` maps to `'unknown'`. + expect(inferExpressionType('record')).toBe('unknown'); + }); }); diff --git a/packages/lint/src/flow-variable-scope.ts b/packages/lint/src/flow-variable-scope.ts index 948eaee28f..431b2463f0 100644 --- a/packages/lint/src/flow-variable-scope.ts +++ b/packages/lint/src/flow-variable-scope.ts @@ -275,17 +275,30 @@ const MAX_BARE_ROOTS = 64; * consulting the AST, which is what re-opens the macro-variable false positive. * * ⛔ That blind spot is NOT confined to the colliding name, and reading it as - * name-local understates it (#16412). Those roots are declared `map`, so using - * one as the operand of an operator with no `map` overload makes the checker's - * FIRST error a `no such overload` rather than an `Unknown variable` — the - * oracle returns `null` on iteration 0 and this loop terminates before it has - * judged anything. Every shadow in that source is then lost, whatever it is - * named, and the source still compiles (the permissive env leaves those roots - * `dyn`, so no sibling diagnostic fires either). Measured, `status` and - * `config` both declared variables and both fields: + * name-local understates it — but the WIDEST way it escaped the name is closed + * (#16412). Those roots used to be declared `map` in the oracle's strict env + * while the permissive env left them `dyn`, so using one as the operand of an + * operator with no `map` overload made the checker's FIRST error a `no such + * overload` rather than an `Unknown variable`: the oracle returned `null` on + * iteration 0 and this loop terminated before it had judged anything, losing + * every shadow in that source whatever it was named. The strict env now + * declares those roots `dyn` as well, so the two envs agree and that class + * cannot arise. Measured, `status` and `config` both declared variables and + * both fields: * - * config == 'x' && status == 'y' -> [] `status` LOST - * status == 'y' && config == 'x' -> ['status'] same names, other order + * config == 'x' && status == 'y' -> ['status'] the class, closed + * status == 'y' && config == 'x' -> ['status'] unchanged, the control + * + * ⚠️ The MECHANISM is untouched for every OTHER first-error class, so a `null` + * from the oracle is still "nothing was reported" and never "the source is + * clean", and this loop still terminates on it. Two such classes are reachable + * from authored input, and each still loses every shadow in the source whatever + * it is named — a CEL TYPE name in that same operand position, which CEL + * declares itself so no declaration this package makes can move it, and `has()` + * handed a non-select argument: + * + * type == 'grid' && status == 'y' -> [] `status` still LOST + * has(status) && other == 'x' -> [] `other` still LOST * * The masking is positional, so the loop's own upper bound is not what limits * it. See {@link firstUndeclaredReference}'s false-negative section for the