diff --git a/.changeset/lint-visibility-bare-identifier-gate.md b/.changeset/lint-visibility-bare-identifier-gate.md new file mode 100644 index 0000000000..d56beeb9b9 --- /dev/null +++ b/.changeset/lint-visibility-bare-identifier-gate.md @@ -0,0 +1,58 @@ +--- +"@objectstack/lint": minor +"@objectstack/formula": minor +--- + +feat(lint): view/page 可见性谓词的裸标识符构建期闸门 —— 坏谓词发不出去(#6128) + +新增 **error 级** 规则 `visibility-bare-identifier`:view/page 的可见性谓词 +(`visibleWhen` 及其两个已弃用别名 `visibleOn` / `visibility`)里引用了任何绑定根都解析不到的 +顶层标识符时,`os validate` / `os build` / `os lint` 一律拒收。写成 `status == 'active'` +而不是 `record.status == 'active'` 的谓词,从此发不出去。 + +按 #5149 维护者 2026-08-06 裁决的构建期半边落地(运行时 warn-once 半边已由 objectui#3541 合入)。 +本仓传统的准确表述是:fail-open 或 fail-closed 都可以裁,**静默不可以**。谓词失败仍然 fail-open +(已发货 app 行为不变),但坏谓词不再能进入产物。 + +**为什么现有两道闸都放行**(#5149 Repro 1 实测,已写进规则注释,防后人误并): +ADR-0032 的标识符闸(`validate-expressions.ts`)解析 record 作用域的裸引用,但它的遍历只覆盖 +objects / flows / actions / sharingRules / hooks,**从不走 views 与 pages**;ADR-0089 D3b +只判**有根**的谓词根错层(runtime 面的 `data.`、metadata 面的 `record.`),**无根**的谓词两边都不匹配。 +两闸之间正好漏掉「作者按文档示例写了裸字段名 → 谓词永远解析失败 → 控制台 fail-open 静默显示」。 + +**判定由两个既有 oracle 合成,本包不自建 CEL 环境**(#4812 的教训):声明性判定取 +`@objectstack/formula` 的 `firstUndeclaredReference`(即 `validateExpression` 给 record 作用域 +裸引用定罪的同一个严格环境),AST 取规范入口 `parseCelToAst`。AST 先收集所有处于**接收者位置** +的标识符(`a.b` / `a?.b` / `a['b']` / `a.exists(…)`)并在检查前声明它们,于是只剩「当作裸值引用」 +的标识符会被判 —— 未知**根**(`my_record.x`)交还给 ADR-0089 D3b,不在本规则射程内。 + +**与 #4953(全量 vs 稀疏绑定)的边界**:#4953 实测同一求值器在两种绑定下语义相反 +(`has(record.a)` 全量 true / 稀疏 false;`record.a != null` 全量 false / 稀疏 FAULT)。本规则 +**按构造与该分叉无关** —— 它从不追问某个 KEY 在已绑定的根上是否存在,只追问标识符有没有根, +而无根标识符在两种绑定下都解析不到。`has(record.x)` / `record.x != null` 等守卫写法在本闸门下 +一律绿,无论 #4953 最终怎么裁;已加测试钉住这条边界。 + +**遍历按实测修正,否则规则生来即死**:`os build` 跑 `examples/app-showcase` 得到的唯一一条 +view 表单谓词落在 `views[0].formViews.edit.sections[0].fields[6].visibleWhen` —— 运行时 app 形状下 +`views[]` 条目是**视图容器**(`ViewSchema` 声明的自有键就是 `list` / `form` / `listViews` / +`formViews`),`sections` 在下一层。原遍历只读 `views[].sections`,在这份 stack 上报告「干净」。 +现在覆盖容器的 `form` 与每个 `formViews.`,以及仍然直接携带 `sections` 的 `defineForm` 形状; +pages 改走共享的 `walkPageComponents`(regions、slotted 页的 `slots`、以及 `properties` 里的 +`page:tabs` / `page:accordion` / `page:card` 子树都随之覆盖,source-authored 页按其既有语义跳过)。 +`objects[].views` 明确不读 —— 该键已被 schema 立碑拒绝,读它只会造出一条永不触发的幽灵检查。 +两条既有 ADR-0089 D3b advisory 随遍历一并变得真正可达。 + +注册表 tier `advisory` → `gating`(#5762 的先例):tier 声明并非自述, +`authoring-rule-wiring.test.ts` 会读规则源码核对。 + +已知盲点(已钉测试、方向安全):字段名与 CEL **类型名**相同时(`type` / `int` / `string` / `list` +/ `map` / `timestamp` …)不判 —— CEL 自身声明这些标识符,`type == 'grid'` 到检查器那里是类型 +overload 错误而非未知变量;改读 overload 消息会误杀合法的 `type(record.x) == string`。语法不通过 +的谓词同样不判,交还给拥有该判定的闸门。两者都是漏判,永远不会变成误红。 + +仓内 `app-todo` / `app-crm` / `app-showcase` 三个示例 `os validate` 全部通过、零 visibility finding, +无需修改任何示例内容。 + +`@objectstack/formula` 侧:公开导出 `firstUndeclaredReference`(理由与既有的 +`collectCelRootIdentifiers` 一致 —— 绑定根集合不同的消费方需要的是同一个答案,替代方案是在消费方 +自建严格 `Environment`,而那正是 #4812 从本包消费方手里拿掉的私有前端)。 diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index 58becc8bd0..030801a563 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -15,6 +15,16 @@ export { celEngine, DEFAULT_LIMITS } from './cel-engine'; // (approval `expression` approvers): lint and the runtime pre-check share this // one helper so what they accept can never drift. export { collectCelRootIdentifiers } from './cel-engine'; +// #6128 — the strict-environment "does this identifier resolve?" oracle, the +// same one `validateExpression` gives its `record`-scoped bare-ref verdict from. +// Published for the same reason as `collectCelRootIdentifiers` above: a lint +// rule whose surface declares a DIFFERENT root set (`@objectstack/lint`'s +// view/page visibility gate binds `current_user` / `page` on top of +// SCOPE_ROOTS) needs this exact answer, and the alternative — rebuilding a +// strict `Environment` in the consumer — is the private-front-end mistake +// #4812 removed from that very package. One oracle, one answer to "what +// resolves", whichever surface is asking. +export { firstUndeclaredReference } from './cel-engine'; // #4812 — the canonical parse-to-AST entry. Any consumer that needs the AST of // an authored CEL source takes it from here, so "what parses" has exactly ONE // answer across build, lint and runtime. Building a private `new Environment()` diff --git a/packages/lint/src/authoring-rules.ts b/packages/lint/src/authoring-rules.ts index 24c89d4926..bb4316bce2 100644 --- a/packages/lint/src/authoring-rules.ts +++ b/packages/lint/src/authoring-rules.ts @@ -693,12 +693,20 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ surfaceReason: RUNTIME_NEEDS_FULL_SNAPSHOT, run: (stack) => validateSeedStateMachine(stack), }, - // ADR-0089 D3b — deprecated visibility aliases and a mis-layered binding root. - // Pre-parse: the schema folds `visibleOn`/`visibility` into `visibleWhen` - // during parse, so the alias the author wrote is gone from `result.data`. + // ADR-0089 D3b — deprecated visibility aliases and a mis-layered binding root, + // plus (#6128) the bare-identifier gate. Pre-parse: the schema folds + // `visibleOn`/`visibility` into `visibleWhen` during parse, so the alias the + // author wrote is gone from `result.data`. + // + // `gating` since #6128: `visibility-bare-identifier` emits `error`. The two + // ADR-0089 rules stay advisory findings within it — the tier is a property of + // the RULE FUNCTION (can it emit `error`?), and the per-finding severity is + // what decides whether any given diagnostic gates, exactly as `lintFlowPatterns` + // has worked since #3760. The promotion follows the #5762 precedent: a family + // that gains an `error` finding moves its registry tier in the same edit. { name: 'validateVisibilityPredicates', - tier: 'advisory', + tier: 'gating', input: 'normalized', commands: ALL, source: 'packages/lint/src/validate-visibility-predicates.ts', diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 4428c64c19..27c3f6cabc 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -139,6 +139,7 @@ export { validateVisibilityPredicates, VISIBILITY_ALIAS_DEPRECATED, VISIBILITY_ROOT_MISLAYERED, + VISIBILITY_BARE_IDENTIFIER, } from './validate-visibility-predicates.js'; export type { VisibilityFinding, diff --git a/packages/lint/src/validate-visibility-predicates.test.ts b/packages/lint/src/validate-visibility-predicates.test.ts index 32be87fdec..6ee70282c5 100644 --- a/packages/lint/src/validate-visibility-predicates.test.ts +++ b/packages/lint/src/validate-visibility-predicates.test.ts @@ -5,7 +5,9 @@ import { validateVisibilityPredicates, VISIBILITY_ALIAS_DEPRECATED, VISIBILITY_ROOT_MISLAYERED, + VISIBILITY_BARE_IDENTIFIER, } from './validate-visibility-predicates'; +import { AUTHORING_RULES } from './authoring-rules.js'; describe('validateVisibilityPredicates (ADR-0089 D3b)', () => { it('is clean for canonical `visibleWhen` with a runtime binding root', () => { @@ -185,3 +187,318 @@ describe('validateVisibilityPredicates (ADR-0089 D3b)', () => { expect(validateVisibilityPredicates(recordStack)).toEqual([]); }); }); + +// ───────────────────────────────────────────────────────────────────── +// `visibility-bare-identifier` — #6128 (the build-time half of #5149's +// 2026-08-06 ruling; the runtime warn-once half landed as objectui#3541). +// ───────────────────────────────────────────────────────────────────── + +/** A one-field runtime form view carrying `predicate` on its only field. */ +function formStack(predicate: unknown): Record { + return { views: [{ name: 'task_form', sections: [{ fields: [{ field: 'notes', visibleWhen: predicate }] }] }] }; +} + +/** Only the bare-identifier findings, for assertions that ignore the advisories. */ +function bareFindings(stack: Record, opts?: { layer: 'runtime' | 'metadata' }) { + return validateVisibilityPredicates(stack, opts).filter((f) => f.rule === VISIBILITY_BARE_IDENTIFIER); +} + +describe('visibility-bare-identifier (#6128 / #5149 requirement 3)', () => { + describe('the acceptance pair', () => { + it('#5149 Repro 1 — a bare field name is an ERROR, not an advisory', () => { + const findings = validateVisibilityPredicates(formStack("status == 'active'")); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(VISIBILITY_BARE_IDENTIFIER); + // The whole point of the ruling: `error` gates, so the broken predicate + // cannot be shipped at all ("坏谓词根本发不出去"). + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('views[0].sections[0].fields[0]'); + expect(findings[0].message).toContain('`status`'); + expect(findings[0].hint).toContain('`record.status`'); + }); + + it('the `record.`-prefixed spelling of the SAME predicate is clean', () => { + expect(validateVisibilityPredicates(formStack("record.status == 'active'"))).toEqual([]); + }); + }); + + describe('the whole predicate family, on every carrier the schema declares', () => { + it('flags a bare identifier on a form SECTION', () => { + const stack = { views: [{ name: 'f', sections: [{ visibleWhen: 'approved', fields: [] }] }] }; + expect(bareFindings(stack).map((f) => f.path)).toEqual(['views[0].sections[0]']); + }); + + it('flags a bare identifier on a PAGE COMPONENT', () => { + const stack = { + pages: [{ name: 'p', regions: [{ components: [{ type: 'element:text', visibleWhen: "kind == 'a'" }] }] }], + }; + expect(bareFindings(stack).map((f) => f.path)).toEqual(['pages[0].regions[0].components[0]']); + }); + + it('reads the value through the deprecated `visibleOn` alias too (alias + bare, both reported)', () => { + const stack = { views: [{ name: 'f', sections: [{ visibleOn: "status == 'x'", fields: [] }] }] }; + const rules = validateVisibilityPredicates(stack).map((f) => f.rule).sort(); + expect(rules).toEqual([VISIBILITY_ALIAS_DEPRECATED, VISIBILITY_BARE_IDENTIFIER].sort()); + }); + + it('reads the value through the deprecated page-side `visibility` alias too', () => { + const stack = { + pages: [{ name: 'p', regions: [{ components: [{ type: 'element:text', visibility: 'shown' }] }] }], + }; + expect(bareFindings(stack)).toHaveLength(1); + }); + + it('resolves a `{ dialect, source }` envelope the same as a bare string', () => { + expect(bareFindings(formStack({ dialect: 'cel', source: "status == 'active'" }))).toHaveLength(1); + }); + + it('walks a slotted page\'s `slots` map (single component AND array form)', () => { + const stack = { + pages: [ + { + name: 'record_page', + kind: 'slotted', + slots: { + highlights: { type: 'element:text', visibleWhen: "stage == 'won'" }, + tabs: [ + { type: 'record:related', visibleWhen: 'record.has_children' }, + { type: 'record:related', visibleWhen: 'has_children' }, + ], + }, + }, + ], + }; + expect(bareFindings(stack).map((f) => f.path).sort()).toEqual([ + 'pages[0].slots.highlights', + 'pages[0].slots.tabs[1]', + ]); + }); + + it('walks a component sub-tree hidden in the untyped `properties` bag', () => { + // `page:tabs` / `page:accordion` keep their children at + // `properties.items[].children`; `page:card` at `properties.body`. A + // hand-rolled `regions[].components[]` loop sees none of them — which is + // the dead-rule shape `page-walk.ts` exists to prevent (#3583). + const stack = { + pages: [ + { + name: 'p', + regions: [ + { + components: [ + { + type: 'page:tabs', + properties: { + items: [{ children: [{ type: 'element:text', visibleWhen: "stage == 'won'" }] }], + }, + }, + ], + }, + ], + }, + ], + }; + expect(bareFindings(stack)).toHaveLength(1); + }); + }); + + // ── The traversal the rule was measured against ───────────────────── + // + // `os build` on examples/app-showcase emits its single view-form predicate at + // `views[0].formViews.edit.sections[0].fields[6].visibleWhen`: on the runtime + // app shape a `views[]` entry is a view CONTAINER, and `sections` live under + // `form` / `formViews.`. Reading only `views[].sections` reported clean + // on that stack — an `error`-level gate that cannot fire on the shape #5149's + // own repro used (an object create dialog's `type: 'tabbed'` form view). + describe('reaches the form views a real stack actually carries', () => { + it('a container\'s named `formViews.` — the shape app-showcase emits', () => { + const stack = { + views: [ + { + name: 'showcase_task', + list: { type: 'grid' }, + formViews: { + edit: { type: 'simple', sections: [{ fields: [{ field: 'notes', visibleWhen: "priority == 'urgent'" }] }] }, + }, + }, + ], + }; + expect(bareFindings(stack).map((f) => f.path)).toEqual([ + 'views[0].formViews.edit.sections[0].fields[0]', + ]); + }); + + it('names the sub-container in `where`, so two form views are distinguishable', () => { + const stack = { + views: [ + { + object: 'showcase_task', + formViews: { + edit: { sections: [{ visibleWhen: 'approved', fields: [] }] }, + tabbed: { sections: [{ visibleWhen: 'approved', fields: [] }] }, + }, + }, + ], + }; + // No `name` on the container (the emitted-artifact shape) — it falls back + // to the `object` binding, and the surface tells the two forms apart. + expect(bareFindings(stack).map((f) => f.where).sort()).toEqual([ + 'view "showcase_task" · formViews.edit', + 'view "showcase_task" · formViews.tabbed', + ]); + }); + + it('a container\'s DEFAULT `form`', () => { + const stack = { + views: [{ name: 'v', form: { sections: [{ visibleWhen: 'approved', fields: [] }] } }], + }; + expect(bareFindings(stack).map((f) => f.path)).toEqual(['views[0].form.sections[0]']); + }); + + it('a bare form view whose `sections` sit at the top (the `defineForm` shape)', () => { + const stack = { views: [{ name: 'v', sections: [{ visibleWhen: 'approved', fields: [] }] }] }; + expect(bareFindings(stack).map((f) => f.path)).toEqual(['views[0].sections[0]']); + }); + + it('a name-keyed `views` map reports the KEY, not a synthetic index', () => { + const stack = { + views: { task_form: { formViews: { edit: { sections: [{ visibleWhen: 'approved', fields: [] }] } } } }, + }; + expect(bareFindings(stack).map((f) => f.path)).toEqual([ + 'views.task_form.formViews.edit.sections[0]', + ]); + }); + + it('does NOT read `objects[].views` — the schema tombstones that key', () => { + // `object.zod.ts:1833`: "`views` is not an ObjectSchema field". A branch + // keyed on it could only fire for stacks the schema already rejects by + // name — the phantom check #4984 / #5017 removed elsewhere. + const stack = { + objects: [ + { name: 'task', views: [{ sections: [{ visibleWhen: 'approved', fields: [] }] }] }, + ], + }; + expect(validateVisibilityPredicates(stack)).toEqual([]); + }); + }); + + describe('the layer decides the prescribed root (ADR-0089 D3)', () => { + it('a metadata-editing form is told to write `data.`, not `record.`', () => { + const findings = bareFindings(formStack("layout == 'grid'"), { layer: 'metadata' }); + expect(findings).toHaveLength(1); + expect(findings[0].hint).toContain('`data.layout`'); + expect(findings[0].hint).not.toContain('`record.layout`'); + }); + + it('a runtime surface is told to write `record.`', () => { + expect(bareFindings(formStack("layout == 'grid'"))[0].hint).toContain('`record.layout`'); + }); + }); + + it('a field named after a CEL TYPE (`type`, `string`, …) is a measured blind spot', () => { + // Not a bug to fix here, and not a false negative anyone can close cheaply: + // `type` / `int` / `string` / `list` / `map` / `timestamp` … are identifiers + // CEL itself declares (they denote type values), so `type == 'grid'` is a + // TYPE-overload error to the checker, not an unknown variable — and + // `firstUndeclaredReference` acts only on `Unknown variable`. Widening onto + // the overload message would reject `type(record.x) == string`, which is + // legitimate CEL, so the gate stays conservative: a missed catch, never a + // false build error. Pinned so the next reader sees a decision, not a hole. + expect(bareFindings(formStack("type == 'grid'"))).toEqual([]); + expect(bareFindings(formStack("record.type == 'grid'"))).toEqual([]); + }); + + // ── The #4953 boundary, pinned rather than described ──────────────── + // + // #4953 measured the SAME evaluator giving opposite verdicts on a total vs a + // sparse record binding: `has(record.a)` is true/false and `record.a != null` + // is false/FAULT depending on which the surface binds. Whichever way that + // fork is settled, none of it changes THIS rule's verdict — a rootless + // identifier resolves under neither binding, and a rooted one is never judged + // here. These cases are the pin on that independence: if a later edit widens + // the rule into key-level reasoning, they go red rather than the boundary + // being quietly lost. + describe('shapes that are legal under a SPARSE binding stay green (#4953)', () => { + it.each([ + ['has(record.status)', 'the sparse-binding guard idiom'], + ['record.status != null', 'the TOTAL-binding guard idiom — the opposite spelling'], + ['has(record.a) && has(record.b) && record.a < record.b', 'the #4763 has()-only shape'], + ['record.a != null && record.b != null && record.a < record.b', 'its total-binding counterpart'], + ['!has(record.archived_at)', 'a negated presence test'], + ])('%s stays clean (%s)', (predicate) => { + expect(validateVisibilityPredicates(formStack(predicate))).toEqual([]); + }); + }); + + describe('what the rule deliberately does not reject', () => { + it.each([ + ["record.tags.all(t, t != '')", 'a macro variable used BARE inside the comprehension body'], + ['record.items.exists(i, i.qty > 0)', 'a macro variable used as a receiver'], + ["['a', 'b'].exists(x, x == record.status)", 'a macro over a list literal'], + ['record.lines.filter(l, l.amount > 0).size() > 0', 'a macro chained into a method call'], + ])('%s — %s', (predicate) => { + expect(validateVisibilityPredicates(formStack(predicate))).toEqual([]); + }); + + it.each([ + ["current_user.id == record.owner_id", 'the `current_user` root the schema documents'], + ["'admin' in current_user.positions", 'the ADR-0068 role-membership shape'], + ["page.selectedProjectId != ''", 'page state as `page.`'], + ['previous.status != record.status', 'the `previous` root the evaluator binds'], + ["parent.status == 'paid'", 'a master-detail header injected as `parent`'], + ])('%s — %s', (predicate) => { + expect(validateVisibilityPredicates(formStack(predicate))).toEqual([]); + }); + + it('an UNKNOWN root is left to the wrong-root rules — this one only judges rootless refs', () => { + // `my_record` resolves to nothing either, but it is a root-shaped defect: + // ADR-0089 D3b owns the two directions the spec states, and the legal-root + // list is not yet trustworthy enough to gate on (#6146). Widening here + // would also make the D3b fixture below a false positive of this rule. + expect(bareFindings(formStack('my_record.x == 1'))).toEqual([]); + expect(bareFindings(formStack("record.data == 1"))).toEqual([]); + }); + + it('a predicate the canonical front end will not parse is left to the syntax verdict', () => { + // `===` is not CEL. `parseCelToAst` returns null and this rule stays + // silent rather than inventing a second syntax verdict — the same policy + // `validate-null-guards.ts` states. (Documented gap: nothing validates + // view/page predicate SYNTAX today, so this one is currently un-reported.) + expect(validateVisibilityPredicates(formStack('country === "USA"'))).toEqual([]); + expect(validateVisibilityPredicates(formStack('status =='))).toEqual([]); + }); + + it('an absent / empty predicate is not a finding', () => { + expect(validateVisibilityPredicates(formStack(undefined))).toEqual([]); + expect(validateVisibilityPredicates(formStack(' '))).toEqual([]); + }); + }); + + describe('the finding really gates', () => { + it('the registry entry is `gating`, so `error` reaches all three commands', () => { + // `severity: 'error'` only fails a build because `authoring-rules.ts` + // says this rule family gates and therefore runs on validate/build/lint + // alike. Declared = enforced: without this entry the diagnostic would be + // an `error` nobody runs everywhere (`authoring-rule-wiring.test.ts` + // states the invariant; this is its per-rule pin). + const entry = AUTHORING_RULES.find((r) => r.name === 'validateVisibilityPredicates'); + expect(entry, 'validateVisibilityPredicates must be registered').toBeDefined(); + expect(entry!.tier).toBe('gating'); + expect([...entry!.commands].sort()).toEqual(['build', 'lint', 'validate']); + }); + + it('a bare identifier ANYWHERE in the predicate is caught, not just at the head', () => { + expect(bareFindings(formStack("record.type == 'a' ? record.x > 1 : status == 'b'"))).toHaveLength(1); + expect(bareFindings(formStack('record.done && overdue'))[0].message).toContain('`overdue`'); + }); + + it('an unknown root does not mask a real bare identifier alongside it', () => { + // The declare-then-check order matters: `my_record` is declared as a + // namespace first, so the checker's verdict lands on `status` rather than + // stopping at the root it is not this rule's job to judge. + const findings = bareFindings(formStack("my_record.x == 1 && status == 'a'")); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('`status`'); + }); + }); +}); diff --git a/packages/lint/src/validate-visibility-predicates.ts b/packages/lint/src/validate-visibility-predicates.ts index b3bad84be8..a10dbf24b8 100644 --- a/packages/lint/src/validate-visibility-predicates.ts +++ b/packages/lint/src/validate-visibility-predicates.ts @@ -13,10 +13,15 @@ * to see what the author actually wrote. * * Two advisory rules (both `warning` — nothing is broken, the alias still works - * and a mis-rooted predicate just never matches): + * and a mis-rooted predicate just never matches) plus one **gating** rule + * (`error` — the predicate can never evaluate at all): * * - `visibility-alias-deprecated` — a `visibleOn` / `visibility` key in authored * source. Autofix intent: rename the key to `visibleWhen` (same value). + * - `visibility-bare-identifier` (**error**, #6128 / #5149 requirement 3) — a + * predicate referencing a top-level identifier that no binding root can + * resolve (`status == 'active'` instead of `record.status == 'active'`). See + * the §Bare identifiers block below for the mechanism and the boundaries. * - `visibility-root-mislayered` — a visibility predicate whose binding root does * not match its layer (ADR-0089 D3, §Context). The check is **bidirectional**: * - **runtime** view/page surfaces (`*.view.ts` / `*.page.ts`) bind @@ -28,13 +33,114 @@ * app-lint path (`os validate` / `compile`) always lints runtime surfaces, while a * file-aware caller linting a `*.form.ts` passes `layer: 'metadata'`. * - * Scope: `views` (form `sections` / legacy `groups`, and their `fields`) and - * `pages` (`regions[].components[]`). Data-field `visibleWhen` is already covered - * by `validate-expressions` and is not re-checked here. + * Scope: `views` — every form view reachable from a `views[]` entry (the entry + * itself when it IS a form view, plus the container's `form` and each + * `formViews.`; see {@link formViewSites} for why reading only the first + * shape left this rule reporting clean on real metadata) — and `pages`, through + * the shared `walkPageComponents` traversal. Data-field `visibleWhen` is already + * covered by `validate-expressions` and is not re-checked here. + * + * The predicate family is read off the schema, not guessed: `visibleWhen` is the + * canonical key on all three carriers (`FormFieldBaseSchema` `view.zod.ts:1416`, + * `FormSectionSchema` `view.zod.ts:1510`, `PageComponentSchema` + * `page.zod.ts:143`), `visibleOn` is the view-side deprecated alias + * (`view.zod.ts:1418` / `:1512`) and `visibility` the page-side one + * (`page.zod.ts:145`). There is no fourth spelling on this surface. + * + * ## Bare identifiers — the gap between two gates that both wave it through + * + * `visibility-bare-identifier` exists because #5149 Repro 1 measured a predicate + * written with bare field names (`status == 'active'`) passing **every** gate the + * platform has and then failing OPEN in the console: the identifier resolves to + * nothing, `evalFieldPredicate` returns its `fallback`, and for visibility that + * fallback is `true` — so a predicate that never works is pixel-identical to no + * predicate at all. The maintainer's 2026-08-06 ruling on #5149 kept fail-open + * and closed the silence from both ends instead: warn-once at runtime + * (objectui#3541, requirement 2) and refuse the metadata at build time (this + * rule, requirement 3). + * + * The two gates that already exist each miss it for a structural reason, and + * BOTH reasons must stay written down or this rule reads like a duplicate and + * gets merged away: + * + * - **ADR-0032's identifier gate** (`validate-expressions.ts`) resolves bare + * refs on `record`-scoped sites — but its traversal covers objects, flows, + * actions, sharing rules and hooks. It never walks `views` or `pages`, so a + * view form field's `visibleWhen` is outside it entirely. + * - **ADR-0089 D3b** (the two rules above, same file) walks exactly this + * surface — but it judges the *root* of a predicate that HAS one + * (`data.` in a runtime view, `record.` in a metadata form). A predicate with + * no root at all matches neither direction and falls through clean. + * + * ### How the verdict is decided (two oracles, neither of them ours) + * + * The declaredness verdict comes from `@objectstack/formula`'s + * `firstUndeclaredReference` — the same strict-environment check + * `validateExpression` uses for the `record`-scoped bare-ref error, so "what + * resolves" has one answer across the platform. This rule builds no + * `Environment` of its own: that is the #4812 lesson (a private parse front end + * silently answers a different question), and the AST it does read comes from + * `parseCelToAst`, the canonical entry. + * + * The checker alone is not enough, because cel-js reports every undeclared + * top-level identifier — including the ROOT of a dotted path nobody binds + * (`my_record.x`). Those are deliberately out of scope here: the set of legal + * roots is not yet trustworthy enough to gate on (#6146 measured `current_user` + * as documented-but-unbound at both ends), and the two ADR-0089 rules above + * already own the wrong-root directions the spec DOES state. So the AST is + * walked first for every identifier used in a **receiver position** (`a.b`, + * `a?.b`, `a['b']`, `a.exists(…)`) and those names are declared before the + * check runs. What survives is an identifier used as a bare VALUE — the one + * shape that cannot resolve under any binding convention. + * + * ### What this rule deliberately does NOT reject + * + * - **Comprehension-macro variables.** `record.tags.all(t, t != '')` binds `t` + * inside the macro body; the AST reports it as a top-level id, the strict + * checker does not. Measured, not assumed — which is exactly why the verdict + * is the checker's and not a hand-rolled AST scan. + * - **Shapes that are legal only under a SPARSE binding (#4953).** That issue + * measured the same evaluator giving opposite verdicts on a total vs sparse + * record: `has(record.a)` is `true`/`false` and `record.a != null` is + * `false`/FAULT depending on which one the surface binds. This rule is immune + * to that fork by construction — it never asks whether a KEY is present on a + * bound root, only whether the identifier has a root at all, and a rootless + * identifier resolves under neither binding. `has(record.x)`, + * `record.x != null` and every other guard idiom stay green here, whichever + * way #4953 is eventually settled. + * - **A predicate the canonical front end will not parse.** `parseCelToAst` + * returns `null` for a syntax fault or a `DEFAULT_LIMITS` overrun, and this + * rule then stays silent rather than inventing a second syntax verdict + * (`validate-null-guards.ts` states the same policy for the same reason). + * Worth knowing where that leaves the surface: unlike the object/flow/action + * sites, NOTHING validates view/page predicate syntax today, so a `=` typo is + * still un-diagnosed here. Widening this rule to own that verdict is a + * separate decision about what authors may write, not a wiring gap to close + * in passing. + * - **Nested composite / repeater sub-fields** (`fields[].fields[]`, + * `view.zod.ts:1477`). The traversal stops at a section's direct fields. A + * sub-field of a repeater row is evaluated against a binding this rule cannot + * cite a spec sentence for, and an `error`-level gate must not judge a + * convention it cannot name. + * - **A field whose name collides with a CEL TYPE** — `type`, `int`, `string`, + * `bool`, `double`, `bytes`, `list`, `map`, `timestamp`, `duration`, + * `null_type`. CEL declares those identifiers itself (they denote type + * values), so bare `type == 'grid'` reaches the checker as a type-overload + * error rather than an unknown variable, and only the latter is a verdict + * here. It cannot be closed by reading the overload message instead: + * `type(record.x) == string` is legitimate CEL over the very same names. A + * measured blind spot in the safe direction — a missed catch, never a false + * build error — pinned by a test so it reads as a decision. */ +import { firstUndeclaredReference, parseCelToAst } from '@objectstack/formula'; +import type { CelAstNode } from '@objectstack/formula'; + +import { walkPageComponents } from './page-walk.js'; + export const VISIBILITY_ALIAS_DEPRECATED = 'visibility-alias-deprecated'; export const VISIBILITY_ROOT_MISLAYERED = 'visibility-root-mislayered'; +export const VISIBILITY_BARE_IDENTIFIER = 'visibility-bare-identifier'; export type VisibilitySeverity = 'error' | 'warning'; @@ -52,7 +158,10 @@ export interface VisibilityOptions { } export interface VisibilityFinding { - /** Always `warning` today — both rules are advisory (see module note). */ + /** + * `warning` for the two ADR-0089 D3b advisories; `error` for + * `visibility-bare-identifier`, which gates (see module note). + */ severity: VisibilitySeverity; /** Diagnostic rule id, e.g. `visibility-alias-deprecated`. */ rule: string; @@ -72,11 +181,28 @@ type AnyRec = Record; const CANONICAL = 'visibleWhen'; const ALIASES = ['visibleOn', 'visibility'] as const; -/** Coerce a collection (array or name-keyed map) to an array of records. */ -function asArray(v: unknown): AnyRec[] { - if (Array.isArray(v)) return v as AnyRec[]; +/** + * Every record in a collection authored either as an array or as a name-keyed + * map, each with its config PATH — `pages[2]` for the array shape, + * `pages.my_page` for the map. Findings on this surface are consumed as edit + * targets (`os lint --json`, Studio's finding renderer), so a map-shaped + * collection must not report a synthetic index nobody can look up. The map + * shape also contributes the entry's KEY as its `name`, which is how an + * unnamed-but-keyed view still locates itself in a message. + */ +function collectionEntries(v: unknown, base: string): Array<{ rec: AnyRec; path: string }> { + if (Array.isArray(v)) { + const out: Array<{ rec: AnyRec; path: string }> = []; + for (let i = 0; i < v.length; i++) { + const rec = v[i]; + if (rec && typeof rec === 'object' && !Array.isArray(rec)) out.push({ rec: rec as AnyRec, path: `${base}[${i}]` }); + } + return out; + } if (v && typeof v === 'object') { - return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + return Object.entries(v as AnyRec) + .filter(([, def]) => !!def && typeof def === 'object' && !Array.isArray(def)) + .map(([name, def]) => ({ rec: { name, ...(def as AnyRec) }, path: `${base}.${name}` })); } return []; } @@ -98,6 +224,94 @@ function usesRoot(source: string, root: string): boolean { return new RegExp(`(^|[^.\\w$])${root}\\.\\w`).test(source); } +// ── `visibility-bare-identifier` (#6128) ──────────────────────────── + +/** + * Binding roots this surface exposes BEYOND the generous set + * `@objectstack/formula`'s strict environment already declares (`record`, + * `previous`, `data`, `parent`, `user`, `ctx`, … — `cel-engine.ts` SCOPE_ROOTS, + * which errs toward declaring more precisely because a missing root is a false + * build error while an extra one is only a missed catch). + * + * Both entries are read off the schema rather than chosen here: + * `view.zod.ts:1416` / `:1510` state the runtime form binding as + * `record` + `current_user`, and `page.zod.ts:143` adds page state as + * `page.` for a page component. The runtime side binds `record` + + * `previous` + a caller-supplied extra scope (objectui + * `packages/core/src/evaluator/fieldRules.ts` `evalFieldPredicate`), so the + * union — not either list alone — is the safe set for an `error`-level gate. + * + * `current_user` is declared here even though #6146 measured it as + * documented-but-unbound at both ends: whether that root RESOLVES is that + * issue's verdict to give, and this rule must not pre-empt it by reporting a + * spelling the spec currently tells authors to write. + */ +const VIEW_PAGE_EXTRA_ROOTS = ['current_user', 'page'] as const; + +type AnyNode = { op?: string; args?: unknown }; + +function isNode(v: unknown): v is AnyNode & CelAstNode { + return !!v && typeof v === 'object' && typeof (v as AnyNode).op === 'string'; +} + +/** + * Every identifier the source uses in a **receiver position** — `a.b`, `a?.b`, + * `a['b']`, `a.exists(…)`. The author is treating each of these as a namespace, + * so an unbound one is a wrong-ROOT defect (ADR-0089 D3b's territory, or an + * undocumented root this rule deliberately does not adjudicate), never the bare + * field name #5149 Repro 1 is about. + * + * Declaring them before the strict check is what narrows this rule to bare + * VALUE references. It is uniformly the conservative direction: every name it + * adds can only remove a finding, so the widening costs coverage and can never + * produce a false build error. + */ +function namespaceRoots(node: unknown, out: Set): void { + if (Array.isArray(node)) { + for (const child of node) namespaceRoots(child, out); + return; + } + if (!isNode(node)) return; + const args = node.args; + if (Array.isArray(args)) { + // `.` / `.?` / `[]` hold the receiver first; `rcall` (a receiver-style call + // such as `record.tags.all(t, …)`) holds the method NAME first and the + // receiver second. + const receiver = node.op === 'rcall' ? args[1] : args[0]; + if ((node.op === '.' || node.op === '.?' || node.op === '[]' || node.op === 'rcall') + && isNode(receiver) && receiver.op === 'id' && typeof receiver.args === 'string') { + out.add(receiver.args); + } + } + namespaceRoots(args, out); +} + +/** + * The first identifier in `source` that no binding root can resolve, or `null` + * when every reference is rooted. See the module note for why this is two + * oracles (the canonical AST for namespace roots, the shared strict-environment + * checker for the verdict) and for the shapes it deliberately leaves alone. + */ +function firstBareIdentifier(source: string): string | null { + const ast = parseCelToAst(source); + // Not parseable through the canonical front end (syntax fault, or over + // DEFAULT_LIMITS) — not this rule's verdict to give. + if (!ast) return null; + const rooted = new Set(); + namespaceRoots(ast, rooted); + return firstUndeclaredReference(source, [...VIEW_PAGE_EXTRA_ROOTS, ...rooted]); +} + +/** + * The root an author on this layer should have written. Runtime view/page + * surfaces bind the live record as `record`; a `*.form.ts` metadata-editing + * form binds the row under edit as `data` (ADR-0089 D3, §Context). + */ +const CANONICAL_ROOT_BY_LAYER: Record = { + runtime: 'record', + metadata: 'data', +}; + /** * Per-layer mis-rooted-predicate description. The `runtime` layer forbids the * metadata-editing-form root (`data.`); the `metadata` layer forbids the runtime @@ -176,6 +390,35 @@ function checkElement( hint: rule.hint, }); } + + // (3) #6128 — a reference no binding root can resolve. Unlike (2) this one + // GATES: a mis-rooted predicate is at least a statement about a namespace + // someone binds somewhere, while a bare identifier resolves nowhere, on no + // layer, under neither a total nor a sparse record (#4953) — so there is no + // reading of the metadata under which it was going to work. + if (source) { + const bare = firstBareIdentifier(source); + if (bare) { + const root = CANONICAL_ROOT_BY_LAYER[layer]; + findings.push({ + severity: 'error', + rule: VISIBILITY_BARE_IDENTIFIER, + where, + path, + message: + `visibility predicate references \`${bare}\` as a bare identifier. ` + + `Values are bound under a namespace on this surface — they are never ` + + `flattened to top level — so \`${bare}\` resolves to nothing, the predicate ` + + `can never evaluate, and the console falls OPEN: the element renders ` + + `unconditionally and looks exactly like one with no predicate at all (#5149).`, + hint: + `Write \`${root}.${bare}\` instead of \`${bare}\`` + + (layer === 'runtime' + ? ' (runtime view/page surfaces bind `record` + `current_user`; a page component also exposes page state as `page.`).' + : ' (a `*.form.ts` metadata-editing form binds the row under edit as `data`).'), + }); + } + } } /** A section field entry is either a bare field name or `{ field, visibleWhen, … }`. */ @@ -183,13 +426,66 @@ function isFieldObject(entry: unknown): entry is AnyRec { return !!entry && typeof entry === 'object' && !Array.isArray(entry); } +/** + * Every FORM VIEW reachable from one `views[]` entry, with the path each sits at. + * + * Two shapes, and reading only the first is how this rule was dead on real + * metadata until #6128 measured it. `os build` on `examples/app-showcase` emits + * its one form predicate at + * `views[0].formViews.edit.sections[0].fields[6].visibleWhen` — the traversal + * read `views[0].sections`, found nothing, and reported clean on a stack that + * DOES carry a view-form predicate: + * + * - **View CONTAINER** (the runtime app shape). `ViewSchema` declares exactly + * `name` / `label` / `object` / `list` / `form` / `listViews` / `formViews` + * (`view.zod.ts:1890-1903` — the strict error map spells the container's own + * keys out in prose). Form sections therefore live one level down, under + * `form` and each `formViews.`; `list` / `listViews.` are + * `ObjectListViewSchema` and carry no `sections`, so they are not walked. + * - **A bare FORM VIEW** (`FormViewSchema`, `view.zod.ts:1623-1624`), whose + * `sections` / `groups` sit at the top. This is the `defineForm` shape the + * `*.form.ts` metadata-editing forms use, i.e. the `layer: 'metadata'` caller. + * + * `objects[].views` is deliberately absent: `object.zod.ts:1833` tombstones the + * key ("`views` is not an ObjectSchema field"), so a branch keyed on it could + * only ever fire for stacks the schema already rejects by name — the phantom + * check #4984 / #5017 removed from two neighbouring rules. Object-level + * `listViews` (`object.zod.ts:1616`) is a list view, so it carries none of this + * either. + */ +function formViewSites( + view: AnyRec, + basePath: string, +): Array<{ form: AnyRec; path: string; surface: string }> { + // `surface` names the sub-container in the human-readable `where`. It earns + // its place on exactly the shape this traversal was extended for: a runtime + // container carries neither `name` nor `object` in the emitted artifact, so + // without it every finding under one view reads `view "views[0]"` and the + // author cannot tell the `edit` form from the `tabbed` one. + const sites = [{ form: view, path: basePath, surface: '' }]; + const dflt = view.form; + if (dflt && typeof dflt === 'object' && !Array.isArray(dflt)) { + sites.push({ form: dflt as AnyRec, path: `${basePath}.form`, surface: 'form' }); + } + const named = view.formViews; + if (named && typeof named === 'object' && !Array.isArray(named)) { + for (const [key, sub] of Object.entries(named as AnyRec)) { + if (sub && typeof sub === 'object' && !Array.isArray(sub)) { + sites.push({ form: sub as AnyRec, path: `${basePath}.formViews.${key}`, surface: `formViews.${key}` }); + } + } + } + return sites; +} + /** * Validate conditional-visibility keys across authored views and pages. * * Runs on the **pre-parse** (normalized) stack so it can see the deprecated * `visibleOn` / `visibility` aliases before the schema folds them into - * `visibleWhen`. Returns findings (empty = clean); all advisory (`warning`) — - * the caller must never fail the build on these alone. + * `visibleWhen`. Returns findings (empty = clean). The two ADR-0089 D3b rules + * are advisory (`warning`); `visibility-bare-identifier` is `error` and the + * caller is expected to fail the build on it (#6128). * * The binding-root check is layer-directional (ADR-0089 D3): pass * `opts.layer = 'metadata'` when linting a `*.form.ts` metadata-editing form (so a @@ -204,54 +500,60 @@ export function validateVisibilityPredicates( const layer: VisibilityLayer = opts.layer ?? 'runtime'; const findings: VisibilityFinding[] = []; - // ── Views: form sections / legacy groups, and their fields ────────── - const views = asArray(stack.views); - for (let i = 0; i < views.length; i++) { - const view = views[i]; - if (!view || typeof view !== 'object') continue; - const viewName = typeof view.name === 'string' ? view.name : `(view ${i})`; - const where = `view "${viewName}"`; - - // `sections` (canonical) and `groups` (legacy alias → sections) both hold - // FormSection objects with an optional visibility predicate + `fields`. - for (const bucket of ['sections', 'groups'] as const) { - const sections = Array.isArray(view[bucket]) ? (view[bucket] as unknown[]) : []; - for (let s = 0; s < sections.length; s++) { - const sec = sections[s]; - if (!sec || typeof sec !== 'object') continue; - const secPath = `views[${i}].${bucket}[${s}]`; - checkElement(sec as AnyRec, where, secPath, layer, findings); - - const secFields = Array.isArray((sec as AnyRec).fields) ? ((sec as AnyRec).fields as unknown[]) : []; - for (let f = 0; f < secFields.length; f++) { - const entry = secFields[f]; - if (isFieldObject(entry)) { - checkElement(entry, where, `${secPath}.fields[${f}]`, layer, findings); + // ── Views: every reachable form view's sections / groups, and their fields ── + for (const { rec: view, path: viewPath } of collectionEntries(stack.views, 'views')) { + // A container names itself with `name`, or binds with `object` — and an + // artifact-emitted one may carry neither, so the path is the last resort. + const viewName = typeof view.name === 'string' ? view.name + : typeof view.object === 'string' ? view.object + : viewPath; + + for (const site of formViewSites(view, viewPath)) { + const where = site.surface ? `view "${viewName}" · ${site.surface}` : `view "${viewName}"`; + // `sections` (canonical) and `groups` (legacy alias → sections) both hold + // FormSection objects with an optional visibility predicate + `fields`. + for (const bucket of ['sections', 'groups'] as const) { + const sections = Array.isArray(site.form[bucket]) ? (site.form[bucket] as unknown[]) : []; + for (let s = 0; s < sections.length; s++) { + const sec = sections[s]; + if (!sec || typeof sec !== 'object') continue; + const secPath = `${site.path}.${bucket}[${s}]`; + checkElement(sec as AnyRec, where, secPath, layer, findings); + + const secFields = Array.isArray((sec as AnyRec).fields) ? ((sec as AnyRec).fields as unknown[]) : []; + for (let f = 0; f < secFields.length; f++) { + const entry = secFields[f]; + if (isFieldObject(entry)) { + checkElement(entry, where, `${secPath}.fields[${f}]`, layer, findings); + } } } } } } - // ── Pages: regions[].components[] ─────────────────────────────────── - const pages = asArray(stack.pages); - for (let i = 0; i < pages.length; i++) { - const page = pages[i]; - if (!page || typeof page !== 'object') continue; - const pageName = typeof page.name === 'string' ? page.name : `(page ${i})`; - const where = `page "${pageName}"`; - const regions = Array.isArray(page.regions) ? (page.regions as unknown[]) : []; - for (let r = 0; r < regions.length; r++) { - const region = regions[r]; - const components = region && typeof region === 'object' && Array.isArray((region as AnyRec).components) - ? ((region as AnyRec).components as unknown[]) - : []; - for (let c = 0; c < components.length; c++) { - const comp = components[c]; - if (comp && typeof comp === 'object') { - checkElement(comp as AnyRec, where, `pages[${i}].regions[${r}].components[${c}]`, layer, findings); - } - } + // ── Pages: every component, through the SHARED walk ───────────────── + // + // `walkPageComponents` (#3583) is the one traversal that knows where page + // components actually live: `regions[].components[]`, the slotted-page + // `slots.` map (single component OR array), and the sub-trees hidden in + // the untyped `properties` bag (`page:tabs` / `page:accordion` + // `items[].children`, `page:card` `body` / `footer`). It also skips + // source-authored (`html` / `react` / `jsx`) pages, whose `regions` are a + // derived cache the author never wrote — reporting a gating error against + // that cache would be a build failure over metadata nobody authored. + // + // Taken rather than hand-rolled for the reason that file states in its own + // header: duplicating this walk has already produced one dead rule. A + // hand-rolled `regions[].components[]` loop is exactly the copy that misses + // slots and nested children — and `visibility-bare-identifier` GATES, so + // `authoring-rules.ts`'s standard applies: partial coverage is not a stricter + // check, it is a coin flip. + for (const { rec: page, path: pagePath } of collectionEntries(stack.pages, 'pages')) { + const pageName = typeof page.name === 'string' ? page.name : undefined; + const where = `page "${pageName ?? pagePath}"`; + for (const walked of walkPageComponents(page, pagePath)) { + checkElement(walked.component, where, walked.path, layer, findings); } }