fix(formula): declare SCOPE_ROOTS as dyn in the strict env so a bare reference behind a root name is judged (#16412)

.changeset/strict-env-scope-roots-dyn.md

@@ -0,0 +1,88 @@
+---
+"@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)
+
+<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is renamed, retired or re-typed: no `packages/spec` key changes its name, its type or its optionality, no stored shape moves, and every view, form, flow and formula parses byte-identically to before. `objectstack migrate meta` therefore has nothing to rewrite, and this changeset carries no rewrite instructions. What narrows is the ACCEPT SET of published CHECKERS at build time: `validateExpression` and two `@objectstack/lint` rules now report a bare field reference they previously left unjudged, which is the same verdict each of them already returns for that identifier when it is written first in the same predicate. The sources that newly report are already broken at RUN time and were before this change: a bare identifier in a record-scoped site resolves to nothing, the expression evaluates to null and a visibility predicate falls open, which is #1928's class. The remedy is per-source and the diagnostic already names it in full, naming the identifier and the namespace it belongs under; there is no authored artifact and no stored representation for a migration to act on. -->
+
+**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.
+
+⚠️ 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
+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.

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<dyn, dyn> ==
- *                                                  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.<field>`
- * and bare `<field>` 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.<field>` and bare
+ * `<field>` 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