Skip to content

Commit 96a2e40

Browse files
os-samclaude
andcommitted
fix(formula): declare SCOPE_ROOTS as dyn in the strict env so a bare 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>
1 parent 9c8b497 commit 96a2e40

5 files changed

Lines changed: 420 additions & 36 deletions

File tree

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
---
2+
"@objectstack/formula": minor
3+
"@objectstack/lint": minor
4+
---
5+
6+
fix(formula): the strict declaredness env declares `SCOPE_ROOTS` as `dyn`, so a bare reference behind a root name is no longer masked (#16412)
7+
8+
<!-- 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. -->
9+
10+
**BREAKING** in the accept-set sense — an accept-set narrowing on published
11+
CHECKERS, in the same sense as a route that starts refusing a request it should
12+
always have refused — landing in the launch window as `minor` on both packages (during the window the bump level is
13+
not the carrier of breaking-ness; this paragraph and the disposition above
14+
are). Nothing that was already reported stops being reported, and no source
15+
that is correct starts being reported.
16+
17+
`firstUndeclaredReference` asks cel-js's checker for the first undeclared
18+
identifier in a source. That checker returns exactly ONE error, and the helper
19+
acts only on `Unknown variable: X`, so whenever the first error is of another
20+
class every undeclared reference behind it in the same source went unjudged and
21+
the helper answered `null` — which is also the value that means "every
22+
reference is rooted". Four published call sites read that answer, and none of
23+
them can tell the two readings apart.
24+
25+
The widest way to reach that state was a disagreement between two environments
26+
in this package about the same names. The strict env declared every
27+
`SCOPE_ROOTS` member (`data`, `config`, `record`, `result`, `item`, `event`,
28+
`input`, `user`, …) as `map`, while the permissive env that `celEngine.compile`
29+
type-checks in leaves them `dyn`. `map` has no `==`, `<` or `+` overload, so an
30+
ordinary comparison on one of those names compiled clean and then faulted `no
31+
such overload` in the strict env only — taking the single error slot and
32+
silencing everything behind it. An author reaches it by naming an object field
33+
or a flow variable after a namespace root and reading it bare, which on a
34+
metadata-editing form is not even a coincidence: that layer binds the row under
35+
edit as `data`.
36+
37+
The strict env now declares those roots `dyn`, which is what the list's own
38+
doc-comment already claimed it was for — member access, arithmetic and
39+
comparison on a root all deferring to runtime — and which `map` delivered only
40+
the first of. The two environments agree about these names, so the class cannot
41+
arise rather than being compensated for downstream.
42+
43+
What starts reporting, measured on each published surface:
44+
45+
- `@objectstack/formula` `validateExpression` with `scope: 'record'` — a bare
46+
reference behind a root name is the hard error it always was for the same
47+
identifier written first (`ok` was `true` with zero errors; it is now `false`).
48+
- `@objectstack/formula` `validateExpression` with `scope: 'flattened'` — the
49+
did-you-mean warning reaches a misspelled field behind a root name.
50+
- `@objectstack/lint` `visibility-bare-identifier` — a bare identifier behind a
51+
root name in a `visibleWhen` predicate is a finding. Per that rule's own
52+
message the console otherwise falls open and the element renders
53+
unconditionally.
54+
- `@objectstack/lint` flow-variable shadowing — a shadowed field read behind a
55+
root name is warned. That rule's documented blind spot is now name-local, as
56+
its wording always claimed: the colliding name itself is still not reported.
57+
58+
⛔ Two first-error classes are NOT closed by this, and both stay pinned. A CEL
59+
TYPE name (`type`, `string`, `int`, …) is declared by CEL itself, so no
60+
declaration this package makes can reach it; measured on the strict env, the
61+
message for `type == 'grid'` is byte-identical under a `map` and a `dyn` root
62+
declaration. And `has()` handed a non-select argument still faults its own
63+
class, which `@objectstack/lint`'s visibility rule masks at its own call site
64+
(#16118) and which nothing else masks.
65+
66+
The narrowing this helper is built on is unchanged: it still acts only on
67+
`Unknown variable`, so `type(record.x) == string`, comprehension macros, guard
68+
idioms, optional chaining and stdlib calls report nothing, and a widening of
69+
that regex onto the overload message remains refused.

packages/formula/src/cel-engine.ts

Lines changed: 77 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,20 @@ export function buildEnv(now: () => Date, timezone = 'UTC'): Environment {
6565

6666
/**
6767
* Namespace roots that a `record`-scoped CEL site may legitimately reference.
68-
* Declared as `map` (dyn values) so member access (`record.foo`) and any
69-
* arithmetic/comparison on it defers to runtime — the strict env faults ONLY on
70-
* an *undeclared* top-level identifier, i.e. a bare field reference. Generous on
71-
* purpose: an unknown root is a missed catch, a missing root is a false positive
72-
* that would break the build, so we err toward declaring more.
68+
* Declared so that member access (`record.foo`) and any arithmetic/comparison on
69+
* a root both defer to runtime — the strict env faults ONLY on an *undeclared*
70+
* top-level identifier, i.e. a bare field reference. Generous on purpose: an
71+
* unknown root is a missed catch, a missing root is a false positive that would
72+
* break the build, so we err toward declaring more.
73+
*
74+
* ⚠️ The CEL type each env declares them AS is not uniform, and the difference is
75+
* load-bearing rather than incidental. {@link buildScopedEnv} — the declaredness
76+
* oracle — declares them `dyn`, because only `dyn` delivers BOTH halves of the
77+
* sentence above; `map` delivered member access and faulted the comparison, and
78+
* #16412 measured what that cost (see {@link firstUndeclaredReference}). The
79+
* type-soundness envs keep them `map`: there a root is a container the check
80+
* deliberately declines to reason through, and the typed struct on
81+
* `record`/`previous`/`input` is what carries the field types.
7382
*
7483
* ## Why this list is PUBLISHED (#6713)
7584
*
@@ -152,8 +161,28 @@ function buildScopedEnv(knownFields: readonly string[]): Environment {
152161
limits: DEFAULT_LIMITS,
153162
});
154163
registerStdLib(env, () => new Date(0));
164+
// Roots are `dyn`, NOT `map`, for exactly the reason `knownFields` are (below)
165+
// and the doc-comment on {@link SCOPE_ROOTS} already claims: member access,
166+
// arithmetic and comparison on a root must all defer to runtime, so that the
167+
// ONLY thing this env faults on is an undeclared top-level identifier.
168+
//
169+
// `map` delivered the member half and not the other two (#16412): `map` has no
170+
// `==` / `<` / `+` overload, so `data == 'x'` — a root, or an object field
171+
// sharing a root's name, in an ordinary comparison — faulted `no such
172+
// overload` HERE while compiling clean in the permissive env, where the same
173+
// names are `dyn`. Because cel-js's checker reports exactly ONE error, that
174+
// fault took the first slot and every undeclared reference behind it in the
175+
// same source went unjudged: `firstUndeclaredReference` answered `null`, the
176+
// value that also means "every reference is rooted", and four consuming rules
177+
// published the source clean. The two environments now agree about these
178+
// names, so the class cannot arise rather than being compensated for
179+
// downstream.
180+
//
181+
// ⛔ This does NOT weaken the check: `dyn` is undeclared-identifier-neutral —
182+
// it changes what is legal ON a declared root, never whether an UNdeclared
183+
// name faults. The `knownFields` loop below has always relied on that.
155184
for (const root of SCOPE_ROOTS) {
156-
try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ }
185+
try { env.registerVariable(root, 'dyn'); } catch { /* duplicate — ignore */ }
157186
}
158187
// `knownFields` are declared as `dyn` so they (and member/arith/compare on
159188
// them) never fault — only a genuinely-undeclared top-level identifier does.
@@ -194,35 +223,48 @@ let recordScopeEnv: Environment | undefined;
194223
*
195224
* The masking is POSITIONAL, not name-keyed: the masked name is not the one
196225
* that triggered the first error, so excluding the trigger's own name does not
197-
* reach it. Measured on this env:
198-
*
199-
* data == 'x' && status == 'q' -> null first error `no such
200-
* overload: map<dyn, dyn> ==
201-
* string`; `status` unjudged
202-
* status == 'q' && data == 'x' -> "status" first error `Unknown
203-
* variable: status`
226+
* reach it.
204227
*
205228
* ⚠️ {@link celEngine.compile} is not a gate against this, so a caller that
206229
* only reaches here on a clean compile is not protected by that gate. `compile`
207230
* type-checks in the PERMISSIVE env ({@link CEL_ENV_OPTIONS},
208-
* `unlistedVariablesAreDyn: true`), and the two error classes that reach the
209-
* first slot from ordinary authored input fault only HERE:
210-
*
211-
* - a {@link SCOPE_ROOTS} member -- or an object field sharing one of those
212-
* names (`data`, `config`, `result`, `item`, `event`, `input`, `user`, …) --
213-
* as the operand of an operator with no `map` overload, because this env
214-
* declares those roots `map` while the permissive one leaves them `dyn`;
215-
* - a CEL TYPE name (`type`, `string`, `int`, …) in the same position, already
216-
* pinned as a blind spot by `@objectstack/lint`'s `visibility-bare-identifier`
217-
* suite -- pinned there per NAME, while the masking it causes is source-wide.
218-
*
219-
* ⛔ Do not close this by widening the regex onto the overload message: that
220-
* false positive is precisely what the narrowing buys off (`type(record.x) ==
221-
* string` is legitimate CEL). Reporting past the first error needs a re-check
222-
* loop over a neutralised source, or a checker entry that returns more than one
223-
* error -- cel-js 8.0.0 has none, its `TypeCheckResult` carries a single
224-
* `error` -- and either one changes what every consuming rule reports. That is
225-
* a design decision, not a patch.
231+
* `unlistedVariablesAreDyn: true`), so a source can compile clean and still
232+
* fault HERE.
233+
*
234+
* ## What is CLOSED, and what is still open (#16412)
235+
*
236+
* ⭐ CLOSED — the {@link SCOPE_ROOTS} class. It was by far the wider of the two
237+
* reachable ones: a root, or an object field sharing a root's name (`data`,
238+
* `config`, `result`, `item`, `event`, `input`, `user`, …), used as the operand
239+
* of an operator with no `map` overload. {@link buildScopedEnv} declared those
240+
* roots `map` while the permissive env left them `dyn`, and that DISAGREEMENT
241+
* was the whole mechanism; the roots are now `dyn` in both, so the class cannot
242+
* arise. Measured on this env, before → after:
243+
*
244+
* data == 'x' && status == 'q' null -> "status" the class, closed
245+
* status == 'q' && data == 'x' "status" unchanged, the control
246+
*
247+
* ⛔ STILL OPEN — every OTHER first-error class, and the mechanism above is
248+
* untouched for them. Two are reachable from authored input:
249+
*
250+
* - a CEL TYPE name (`type`, `string`, `int`, …) in that same position. CEL
251+
* itself declares those names, so no declaration this package controls can
252+
* move them; `type == 'grid' && status == 'q'` still answers `null`. Pinned
253+
* per NAME by `@objectstack/lint`'s `visibility-bare-identifier` suite, while
254+
* the masking it causes is source-wide.
255+
* - `has()` handed a non-select argument (`has(status) && other == 'x'`), which
256+
* faults `has() invalid argument`. `@objectstack/lint`'s
257+
* `validate-visibility-predicates` masks `has(…)` spans at its own call site
258+
* (#16118) and that mask stays load-bearing; no other consumer has one.
259+
*
260+
* ⇒ A `null` from this helper is still "nothing was reported", never "the
261+
* source is clean". ⛔ Do not close the remaining classes by widening the regex
262+
* onto the overload message: that false positive is precisely what the
263+
* narrowing buys off (`type(record.x) == string` is legitimate CEL). Reporting
264+
* past the first error needs a re-check loop over a neutralised source, or a
265+
* checker entry that returns more than one error -- cel-js 8.0.0 has none, its
266+
* `TypeCheckResult` carries a single `error` -- and either one changes what
267+
* every consuming rule reports. That is a design decision, not a patch.
226268
*/
227269
export function firstUndeclaredReference(
228270
source: string,
@@ -598,10 +640,10 @@ export function parseCelToAstWithReason(
598640
* expression — its raw CEL type name (`'int'`, `'double'`, `'string'`, `'bool'`,
599641
* `'google.protobuf.Timestamp'`, `'dyn'`, …) — or `null` when the expression does
600642
* not type-check. Reuses the SAME record-scoped, stdlib-registered env as
601-
* {@link firstUndeclaredReference}: namespace roots (`record`, `previous`, …) are
602-
* declared `map` and `knownFields` are declared `dyn`, so both `record.<field>`
603-
* and bare `<field>` references resolve while every stdlib call carries its
604-
* declared return type.
643+
* {@link firstUndeclaredReference}: namespace roots (`record`, `previous`, …) and
644+
* `knownFields` are both declared `dyn`, so both `record.<field>` and bare
645+
* `<field>` references resolve while every stdlib call carries its declared
646+
* return type.
605647
*
606648
* Deliberately conservative. A member access (`record.amount`) or a bare field is
607649
* `dyn`, and an operator over two `dyn` operands stays `dyn` (cel-js cannot prove

0 commit comments

Comments
 (0)