From f4bc01a9da0297eeda3a40070521344bce7e201b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:32:14 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(lint,formula):=20=E5=AD=97=E6=AE=B5?= =?UTF-8?q?=E7=BA=A7=20*When=20=E7=9A=84=E6=A0=B9=E6=A3=80=E6=9F=A5?= =?UTF-8?q?=E4=BB=8E=E9=BB=91=E5=90=8D=E5=8D=95=E7=BF=BB=E4=B8=BA=E7=99=BD?= =?UTF-8?q?=E5=90=8D=E5=8D=95,=E5=B9=B6=E6=8C=89=E6=A7=BD=E4=BD=8D?= =?UTF-8?q?=E5=88=86=E6=A1=A3=E5=9B=A0=E6=9E=9C=E5=8F=A5=20(#6713,=20#6716?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZgRyPVwi1jLb1mNNuUQ9o --- packages/formula/src/cel-engine.ts | 22 +- packages/formula/src/index.ts | 10 + .../lint/src/validate-expressions.test.ts | 292 ++++++++++++++++++ packages/lint/src/validate-expressions.ts | 247 +++++++++++++-- 4 files changed, 537 insertions(+), 34 deletions(-) diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 0499b7bc73..bde8c72725 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -50,8 +50,28 @@ function buildEnv(now: () => Date, timezone = 'UTC'): Environment { * 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. + * + * ## Why this list is PUBLISHED (#6713) + * + * Exported for the same reason as {@link collectCelRootIdentifiers} and + * {@link firstUndeclaredReference}: a surface that binds a CLOSED set of roots + * has to name the roots it does NOT bind, and that complement is + * `SCOPE_ROOTS` minus its own allowlist. `@objectstack/lint`'s field-level + * `*When` gate is exactly such a surface — it binds `record` / `previous` / + * `parent` and nothing else — and it used to carry a hand-written DENYLIST of + * three roots instead. A denylist structurally cannot track this list: every + * root added here (`current_user` arrived in #6290) is silently unreported at + * that surface until somebody remembers to copy it over, and #6713 measured 21 + * roots sitting in that gap. + * + * Consuming the list is NOT the same as consuming {@link firstUndeclaredReference} + * and the difference is load-bearing. The strict env also declares CEL's own + * TYPE names (`int`, `string`, `bool`, `type`, `map`, …), so + * `type(record.x) == string` reports `string` as a root that "resolves" — + * legitimate CEL a declaredness oracle cannot tell apart from an unbound + * namespace. Membership of THIS list can. */ -const SCOPE_ROOTS = [ +export const SCOPE_ROOTS = [ 'record', 'previous', 'input', 'output', 'os', 'vars', 'variables', 'automation', 'context', 'args', 'item', 'env', 'user', 'step', 'result', 'trigger', 'event', 'payload', 'data', 'params', 'config', 'settings', diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index 030801a563..b1a1e94a4a 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -25,6 +25,16 @@ export { collectCelRootIdentifiers } from './cel-engine'; // #4812 removed from that very package. One oracle, one answer to "what // resolves", whichever surface is asking. export { firstUndeclaredReference } from './cel-engine'; +// #6713 — the namespace-root baseline itself. A surface binding a CLOSED set of +// roots must name the ones it does NOT bind, and that complement is this list +// minus the surface's own allowlist; a hand-copied denylist in the consumer +// cannot track additions here (21 roots were sitting in that gap when +// `@objectstack/lint`'s field-level `*When` gate was measured). Note the +// declaredness oracle above cannot substitute: it also declares CEL's TYPE +// names, so `type(record.x) == string` resolves `string` — legal CEL that +// membership of this list separates from an unbound namespace and the oracle +// does not. +export { SCOPE_ROOTS } 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/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index c6d981eca6..39d28b598b 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -1,6 +1,11 @@ import { readFileSync } from 'node:fs'; import { describe, it, expect } from 'vitest'; +// #6713 — the residual-root table below is generated from the REAL baseline, not +// a copy of it: "a future `SCOPE_ROOTS` member is covered for free" is the whole +// argument for the allowlist, and a hand-copied list would go green on exactly +// the root the rule never saw. +import { SCOPE_ROOTS } from '@objectstack/formula'; import { ExpressionInputSchema, ObjectStackSchema } from '@objectstack/spec'; import { FieldSchema, ObjectSchema, SelectOptionSchema } from '@objectstack/spec/data'; import { SharingRuleSchema } from '@objectstack/spec/security'; @@ -915,6 +920,293 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); }); + /** + * ── Denylist → ALLOWLIST, and the residual roots it uncovers (#6713) ───── + * + * #6584 matched one root, #6585 three. The truth is a three-item WHITELIST + * — `record` / `previous` / `parent` — pinned by three independent anchors + * (server `rule-validator.ts` binds, objectui's five field-level call sites, + * objectui's authoring `FIELD_RULE_ROOTS`). Everything else in `SCOPE_ROOTS` + * was equally unbound, equally faulting, and equally SILENT: a declared root + * resolves in the strict env, so the bare-reference check never fired on it + * either, and the denylist did not know it. + * + * These tests are written against the IMPORTED `SCOPE_ROOTS`, not a copy of + * it, because "a future root is covered for free" is the whole argument for + * the allowlist and a hand-copied list in the test would assert the opposite + * of what it claims — it would go green on a root the rule never saw. + */ + describe('field-level `*When` roots are an ALLOWLIST over SCOPE_ROOTS (#6713)', () => { + /** The three the surface really binds. Everything else must be rejected. */ + const BOUND = ['record', 'previous', 'parent'] as const; + const RESIDUAL = SCOPE_ROOTS.filter((r) => !(BOUND as readonly string[]).includes(r)); + + const fieldIssues = (predicate: string, slot = 'visibleWhen') => + validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { + amount: { type: 'number' }, + // Declared so the field-existence pass has no verdict of its own + // on the comprehension-macro case below. + tags: { type: 'text' }, + gate: { type: 'text', [slot]: predicate }, + }, + }], + }).filter((i) => i.where === `object 'showcase_deal' · field 'gate' ${slot}`); + + it('the residual set is non-empty and much larger than the denylist it replaces', () => { + // Guards the table below from going vacuous: if `SCOPE_ROOTS` ever + // shrank to the three bound roots there would be nothing to reject and + // every `it.each` case would silently disappear. + expect(RESIDUAL.length).toBeGreaterThan(20); + // The three the old denylist held — a small minority of what is unbound. + expect(RESIDUAL).toEqual(expect.arrayContaining(['current_user', 'user', 'ctx'])); + }); + + it.each(RESIDUAL)('rejects `%s` on a field-level `visibleWhen` — unbound at this surface', (root) => { + const hit = fieldIssues(`${root}.some_key == 'x'`); + expect(hit).toHaveLength(1); + expect(hit[0]!.severity).toBe('error'); + expect(hit[0]!.message).toContain(`\`visibleWhen\` reads \`${root}\``); + }); + + it.each(BOUND)('ACCEPTS `%s` — the three roots the evaluators really bind', (root) => { + // `visibleWhen` on purpose: `parent` on `readonlyWhen`/`requiredWhen` + // meets the separate #4889/#4977 master-detail gate, which is a + // different rule with a different verdict and would confuse this pin. + expect(fieldIssues(`${root}.amount > 0`)).toHaveLength(0); + }); + + /** + * The two named cases #6713 called out as credible author typos rather + * than theoretical members of the residual set. + */ + it('rejects `os.user.id` — ADR-0068 D1\'s FOURTH user spelling, the one #6585 left', () => { + const hit = fieldIssues("os.user.id == '1'"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('`visibleWhen` reads `os`'); + }); + + it('rejects `data` — the LEGAL root of this same key on a METADATA form', () => { + const hit = fieldIssues("data.type == 'select'"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('`visibleWhen` reads `data`'); + }); + + /** + * The partition, both halves. The rule judges `SCOPE_ROOTS` membership, + * NOT strict-env declaredness — and that is a measured distinction, not a + * style choice: the strict env also declares CEL's TYPE names, so + * `type(record.x) == string` reports `string` as a root that resolves. + * A declaredness oracle would reject that legitimate predicate. + */ + it('leaves a BARE field reference to the bare-reference check — one issue, not two', () => { + const hit = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { status: { type: 'text' }, gate: { type: 'text', visibleWhen: "status == 'x'" } }, + }], + }).filter((i) => i.where.includes("field 'gate' visibleWhen")); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toMatch(/bare reference `status`/); + // …and NOT the allowlist rule's wording, which prescribes the wrong fix + // for a bare field. + expect(hit[0]!.message).not.toContain('binds only `record`'); + }); + + it('does NOT reject a CEL TYPE name used as a value — `type(record.x) == string`', () => { + // `collectCelRootIdentifiers` reports `string` as a top-level id and the + // strict env declares it, so a declaredness-based rule WOULD reject this + // legal predicate. SCOPE_ROOTS membership is what separates the two. + expect(fieldIssues('type(record.amount) == string')).toHaveLength(0); + }); + + it('does NOT reject a comprehension-macro variable', () => { + expect(fieldIssues("record.tags.all(t, t != '')")).toHaveLength(0); + }); + + /** + * Root-vs-MEMBER discrimination, re-pinned at allowlist width. #6585 + * pinned `record.user_id` / `record.ctx_key`; the allowlist rejects ~20 + * more names, so the number of ordinary field names that LOOK like a + * rejected root grows with it. Confusing the two would reject the most + * ordinary predicates an author writes. + */ + it('does NOT trip on `record` MEMBERS merely spelled like residual roots', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { + data_source: { type: 'text' }, + os_version: { type: 'text' }, + vars_count: { type: 'number' }, + trigger_key: { type: 'text' }, + gate: { + type: 'text', + visibleWhen: + "record.data_source != '' && record.os_version != '' && " + + "record.vars_count > 0 && record.trigger_key == 'x'", + }, + }, + }], + }); + expect(issues).toHaveLength(0); + }); + + it('reports ONE root per slot, preferring the canonical user spelling', () => { + // Two rejected roots in one predicate. The tie-break is documented + // (user roots first, canonical first) so the message is stable rather + // than dependent on AST walk order. + const hit = fieldIssues("data.type == 'select' && 'admin' in current_user.positions"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('reads `current_user`'); + }); + }); + + /** + * ── The prescription tiers by ROOT (#6713 axis 2) ──────────────────────── + * + * The pre-#6713 prescriptions are user-oriented because the pre-#6713 + * denylist held only user roots. Answering `data.type == 'select'` with + * "move it to the option's `visibleWhen`" answers a question nobody asked. + */ + describe('prescription tiers by root (#6713)', () => { + const messageFor = (predicate: string, slot = 'visibleWhen') => + validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { amount: { type: 'number' }, gate: { type: 'text', [slot]: predicate } }, + }], + }).filter((i) => i.where === `object 'showcase_deal' · field 'gate' ${slot}`)[0]!.message; + + it.each(['current_user', 'user', 'ctx', 'os'])( + 'user root `%s` keeps the two USER prescriptions plus the `record` rewrite', + (root) => { + const m = messageFor(`'admin' in ${root}.positions`); + expect(m).toMatch(/option's own `visibleWhen`/); + expect(m).toMatch(/readable: false/); + expect(m).toMatch(/rewrite the predicate against `record`/); + }, + ); + + it('`data` gets the metadata-form explanation — and NOT the user prescriptions', () => { + const m = messageFor("data.type == 'select'"); + expect(m).toMatch(/METADATA form/); + expect(m).toMatch(/`record\.`/); + // The half this tier exists for: the user-oriented advice is absent, + // because it answers a question this author did not ask. + expect(m).not.toMatch(/option's own `visibleWhen`/); + expect(m).not.toMatch(/readable: false/); + }); + + it.each(['vars', 'trigger', 'context', 'input'])( + 'general root `%s` gets the rewrite prescription, not the user one', + (root) => { + const m = messageFor(`${root}.step_one == 'done'`); + expect(m).toMatch(/bound at OTHER evaluation sites/); + expect(m).toMatch(/Rewrite the predicate against `record`/); + expect(m).not.toMatch(/readable: false/); + expect(m).not.toMatch(/METADATA form/); + }, + ); + }); + + /** + * ── The causal sentence tiers by SLOT (#6716) ──────────────────────────── + * + * One sentence used to serve all three slots: "the predicate faults and + * falls back to VISIBLE, leaving the field the test was meant to hide + * showing for everyone". It is precise for exactly one of them. All three + * cells re-measured for this change, at BOTH ends of each slot: + * + * visibleWhen client `fallback: true` ⇒ VISIBLE; server never evaluates + * a FIELD-level `visibleWhen` at all (`hasFieldRules` gates + * on `requiredWhen || readonlyWhen || option visibility`). + * ⇒ the old sentence was RIGHT here. + * readonlyWhen server `isReadonlyWhenLocked` ⇒ LOCKED (#4889's carve-out, + * whose trigger IS the unbound-root case) and + * `stripReadonlyWhenFields` deletes the value from the + * payload; client `fallback: false` ⇒ editable. The two ends + * fault in OPPOSITE directions and ADR-0057 D10 gives it to + * the server. ⇒ the old sentence was BACKWARDS here. + * requiredWhen server logs the unbound root and `continue`s (#4977 did not + * copy the carve-out); client `fallback: false`. Both ends + * fail open and neither is about visibility. ⇒ the old + * sentence named the wrong PROPERTY here. + * + * Honest note on the reverse direction: reverting the per-slot map cannot + * turn the `visibleWhen` assertions red, because the shared sentence WAS the + * `visibleWhen` one. The load-bearing pins are therefore the two negative + * assertions on `readonlyWhen` / `requiredWhen` — those are what a revert + * fails. + */ + describe('per-slot causal sentence (#6716)', () => { + const messageFor = (slot: string) => + validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { gate: { type: 'text', [slot]: "'admin' in current_user.positions" } }, + }], + }).filter((i) => i.where === `object 'showcase_deal' · field 'gate' ${slot}`)[0]!.message; + + it('`visibleWhen` — fail-OPEN to visible, the one slot the shared sentence fitted', () => { + const m = messageFor('visibleWhen'); + expect(m).toMatch(/falls back to VISIBLE/); + expect(m).toMatch(/showing for everyone/); + }); + + it('`readonlyWhen` — says LOCKED, and never says the field stays visible', () => { + const m = messageFor('readonlyWhen'); + expect(m).toMatch(/LOCKED/); + expect(m).toMatch(/#4889/); + // The defect this card was filed for. The server treats the field as + // locked and drops the write; telling the author it is "showing for + // everyone" inverts both the urgency and the troubleshooting direction. + expect(m).not.toMatch(/falls back to VISIBLE/); + expect(m).not.toMatch(/showing for everyone/); + }); + + it('`readonlyWhen` — names the client/server disagreement, not just the server verdict', () => { + // ADR-0057 D10: the form renders the field editable (`fallback: false`) + // while the server locks it. An author who only reads "LOCKED" cannot + // reconcile that with the editable input in front of them. + const m = messageFor('readonlyWhen'); + expect(m).toMatch(/OPPOSITE directions/); + expect(m).toMatch(/editable/); + }); + + it('`requiredWhen` — says the requirement is never enforced, not anything about visibility', () => { + const m = messageFor('requiredWhen'); + expect(m).toMatch(/never enforced/); + expect(m).toMatch(/saves with the field empty/); + expect(m).not.toMatch(/VISIBLE/); + expect(m).not.toMatch(/showing for everyone/); + }); + + it('no two slots share a causal sentence', () => { + const clause = (slot: string) => messageFor(slot).split(' is unbound here, so ')[1]!; + const clauses = ['visibleWhen', 'readonlyWhen', 'requiredWhen'].map(clause); + expect(new Set(clauses).size).toBe(3); + }); + + /** + * The per-slot axis is orthogonal to the per-root axis: a `data` typo on + * `readonlyWhen` needs the metadata-form prescription AND the locked-field + * consequence. A message-per-combination design would have dropped one. + */ + it('the two axes compose — `data` on `readonlyWhen` carries both halves', () => { + const m = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { gate: { type: 'text', readonlyWhen: "data.type == 'select'" } }, + }], + }).filter((i) => i.where.includes("field 'gate' readonlyWhen"))[0]!.message; + expect(m).toMatch(/LOCKED/); // slot axis + expect(m).toMatch(/METADATA form/); // root axis + }); + }); + /** * ── The option-level traversal itself (#6290 half 3) ──────────────────── * diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 3673b5d6b2..998353c0b0 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -78,7 +78,7 @@ * `validate-expressions.test.ts` pins that with no tracked exceptions left. */ -import { validateExpression, collectCelRootIdentifiers } from '@objectstack/formula'; +import { validateExpression, collectCelRootIdentifiers, SCOPE_ROOTS } from '@objectstack/formula'; import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; import type { FlowNodeParsed } from '@objectstack/spec/automation'; @@ -393,14 +393,16 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { /** * A FIELD-level conditional rule (`visibleWhen` / `readonlyWhen` / - * `requiredWhen`) that reaches for the signed-in user — under its canonical - * spelling `current_user` or either of its ADR-0068 D1 aliases, `user` and - * `ctx.user` — the one thing the field level does not bind (#6146, measured - * at both ends: `evalFieldPredicate` / `resolveFieldRuleState` bind `record` - * + `previous` + `parent` and nothing else, and objectui#1582's authoring - * autocomplete pins the same three). The third root is matched WHOLE (any - * `ctx` read, not only `ctx.user`) — see "Why THREE roots" below for the - * measurement that decides it. + * `requiredWhen`) that reaches for a namespace root the field level does not + * bind. The field level binds THREE roots and nothing else — `record`, + * `previous`, and `parent` on a master-detail line item — measured at three + * independent ends: the server (`rule-validator.ts` binds + * `{ record, previous, extra: { parent } }` for `readonlyWhen` and + * `{ record, previous, ...parentScope }` for `requiredWhen`), the client + * (`evalFieldPredicate` binds `record` + `previous` + a caller `scope` that + * is only ever `{ parent }` across objectui's five field-level call sites), + * and the authoring surface (objectui's `FIELD_RULE_ROOTS`, whose comment + * says "nothing else"). * * ## Why this is a rule of its own rather than a missing root * @@ -422,12 +424,57 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { * platform-wide) and the surface that genuinely cannot bind it says so here, * in its own words, with the prescriptions that actually exist. * + * ## Why the membership test is an ALLOWLIST (#6713) + * + * Until #6713 this rule matched a hand-written DENYLIST — one root in #6584, + * three after #6585 (`current_user` / `user` / `ctx`). The denylist was never + * the truth: the three anchors above pin a three-item ALLOWLIST, and + * everything outside it is equally unbound, faults identically, and — this is + * the part that made the hole invisible — is equally SILENT. A root in + * `SCOPE_ROOTS` resolves in the strict env, so the bare-reference check one + * line up never fires on it either. #6713 measured 21 roots living in that + * gap (`input`, `output`, `os`, `vars`, `variables`, `automation`, `context`, + * `args`, `item`, `env`, `step`, `result`, `trigger`, `event`, `payload`, + * `data`, `params`, `config`, `settings`, `features`, `current`), two of them + * highly credible author typos rather than theoretical members: + * + * - `os.user.id` — ADR-0068 D1's FOURTH user spelling. #6585 took three and + * left this one, so the same semantic error stayed silent under one of the + * four names the platform itself mounts the user object under. + * - `data.status == 'x'` — `data` is the LEGAL root of this very + * `visibleWhen` key on a METADATA form (`view.zod.ts`: "Root: `record` … + * in runtime forms, or `data` in metadata forms"). Two form kinds, one key + * name, different roots — and the repo's own `*.form.ts` files are full of + * the `data` spelling for an author to copy. + * + * A denylist cannot track `SCOPE_ROOTS`: every root added there is unreported + * here until somebody remembers to copy it across (`current_user` itself + * arrived in #6290 and needed #6584 to be noticed). The allowlist inverts the + * maintenance burden onto the three roots that are pinned by three anchors and + * change only when the evaluators do. + * + * The membership test is `SCOPE_ROOTS` minus the allowlist, taken from + * `@objectstack/formula` rather than restated here (#6713 published it for + * this consumer) — one list, one definition, no drift. It deliberately is NOT + * `firstUndeclaredReference`, the declaredness oracle the sibling visibility + * rule uses, and the difference is a measured false positive rather than a + * preference: the strict env also declares CEL's TYPE names, so + * `type(record.x) == string` reports `string` as a root that "resolves". + * Judging by declaredness would reject that legitimate predicate; judging by + * `SCOPE_ROOTS` membership does not. Everything the oracle owns and this list + * does not — bare field references, comprehension-macro variables — keeps + * falling to the bare-reference check, which has the right prescription for + * it. The two partitions are disjoint and there is no gap between them. + * * ## Why it is an error and not a warning * - * The failure direction is the worst one available: an unbound identifier - * faults, the fault falls back, and visibility's fallback is `true` — so a - * predicate written to HIDE a field leaves it permanently visible, which is - * the opposite of what the author wrote and the half nobody notices (#6146). + * Every fault direction available here is silent, and two of the three are + * the opposite of what the author declared (see the per-slot table below): + * a `visibleWhen` written to HIDE leaves the field visible to everyone, a + * `readonlyWhen` written to unlock-under-a-condition locks the field on every + * write, and a `requiredWhen` simply never fires. None of the three produces + * a runtime error an author can find; the only signal that exists is this one + * (#6146). * * Verdict scope is the field level only. Per-option `visibleWhen` is checked * by the loop in the field walk and deliberately NOT passed through here: @@ -435,7 +482,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { * `current_user` (ADR-0068 / objectui#2284) — that surface is where such a * predicate belongs, which is why it is also the first prescription below. * - * ## Why THREE roots, and why `ctx` is judged whole-root (#6585) + * ## The user roots, and why `ctx` is judged whole-root (#6585) * * ADR-0068 D1 makes `user` and `ctx.user` ALIASES of `current_user` — one * `EvalUser` object under every spelling (`buildScope` in @@ -444,8 +491,14 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { * identical semantic error got a diagnostic under `current_user` and total * silence under the aliases — which spelling the author picked decided * whether they got the diagnostic, the exact fork AI authors cannot - * self-check. All three roots now share one verdict and one prescription; - * the message names the spelling found, nothing else varies. + * self-check. All four roots now share one verdict and one prescription; + * the message names the spelling found, nothing else varies. (`os` joined the + * set in #6713 — it is the fourth name `buildScope` mounts the same object + * under, and the allowlist would have rejected it anyway; what the user tier + * decides is only WHICH prescription it gets, and the user one is right for + * `os.user`. `os.org` / `os.env` land in the same tier because the option + * surface named by the first prescription binds the whole `os` namespace, not + * only its `user` member.) * * `ctx` is judged as a WHOLE root, not only in `ctx.user` form, because at * this surface that is simply what is true: `buildScope` creates the `ctx` @@ -468,31 +521,159 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { * would re-open the same fork one level down (`ctx["user"].id` silent, * `ctx.user.id` rejected), while leaving a real fail-open fault (`ctx.locale`) * unreported for the sake of a narrower rule NAME. + * + * ## The message tiers on TWO orthogonal axes + * + * The causal half tiers by SLOT (#6716); the prescription half tiers by ROOT + * (#6713). They are independent — `data` on a `readonlyWhen` needs the + * metadata-form prescription AND the locked-field consequence — so one + * skeleton carries both rather than a message per combination. + * + * ### Axis 1 — the consequence, by SLOT (#6716, all three cells measured) + * + * Until #6713/#6716 all three slots shared ONE sentence: "the predicate + * faults and falls back to VISIBLE, leaving the field the test was meant to + * hide showing for everyone". That is precise for exactly one of them. The + * measurement, at both ends of each slot: + * + * - **`visibleWhen` — client-only, fail-OPEN, sentence was correct.** The + * server never evaluates a FIELD-level `visibleWhen` at all: + * `rule-validator.ts`'s `ConditionalFieldDef` has no such member, and + * `hasFieldRules` gates on `requiredWhen || readonlyWhen || + * fieldHasOptionVisibility` — the `visibleWhen` it does evaluate is the + * per-OPTION one. So the only verdict is the renderer's, and + * `resolveFieldRuleState` passes `fallback: true` for visibility. Field + * visible to everyone, exactly as the old sentence said. + * - **`readonlyWhen` — the two ends fault in OPPOSITE directions, and the + * server wins.** Server: `isReadonlyWhenLocked` matches the fault with + * `unknownVariableOf` and returns `true` — "the declared lock is not + * waived because it could not be evaluated" (#4889's carve-out, whose + * trigger is precisely the unbound-ROOT case, not the undeclared-key one) + * — and `stripReadonlyWhenFields` then DELETES the field from the incoming + * payload and lets the rest of the write through. Client: + * `resolveFieldRuleState` passes `fallback: false`, so the form renders the + * field editable. ADR-0057 D10 ("server enforces, client is courtesy") + * resolves the disagreement: the author edits the field, the save reports + * success, and the value silently never lands. The old sentence told this + * author the field would be VISIBLE TO EVERYONE — the opposite failure, and + * the opposite troubleshooting direction. + * - **`requiredWhen` — fail-OPEN at both ends, and never about visibility.** + * Server: the `requiredWhen` block logs `unknownVariableOf`'s name and + * `continue`s — #4977 deliberately did not copy #4889's carve-out, so the + * required-check is skipped for that write. Client: `fallback: false`, so + * the form does not mark the field required either. Both ends agree and + * both do nothing: the requirement is never enforced anywhere, and a record + * saves with the field empty. "Falls back to VISIBLE" was not merely + * imprecise here, it named the wrong property of the field. + * + * `conditionalRequired` also reaches this helper (the field walk still passes + * it). It is a `retiredKey` in `FieldSchema` — the strict schema rejects it by + * name — so the branch is inert on the parsed compile path, and it gets a + * slot-agnostic clause rather than a fabricated fourth measurement. + * + * ### Axis 2 — the prescription, by ROOT (#6713) + * + * The pre-#6713 prescriptions are user-oriented (move to the option level, + * declare permission-set FLS) because the pre-#6713 denylist held only user + * roots. Handed to an author who wrote `data.type == 'select'`, "move it to + * the option's `visibleWhen`" answers a question nobody asked. Three tiers: + * + * - **user roots** (`current_user` / `user` / `ctx` / `os`) keep the existing + * two user-specific prescriptions plus the `record` rewrite; + * - **`data`** gets the metadata-form-vs-runtime-form explanation, because + * that is what the mistake IS — the same key name, the other form kind's + * root; + * - **everything else** gets the general rewrite, phrased without claiming + * which other surface the author copied it from. + */ + /** + * The roots a field-level `*When` predicate binds. Everything else in + * `SCOPE_ROOTS` is rejected — see the allowlist section above. + */ + const FIELD_RULE_BOUND_ROOTS = ['record', 'previous', 'parent'] as const; + /** + * ADR-0068 D1's four user spellings, in the order the message's tie-break + * prefers them (canonical first — the #6585 ordering, with `os` appended so + * the pre-existing three keep their exact precedence). */ - const FIELD_UNBOUND_USER_ROOTS = ['current_user', 'user', 'ctx'] as const; - const checkFieldRuleUserRoot = (where: string, slot: string, raw: unknown): void => { + const FIELD_RULE_USER_ROOTS = ['current_user', 'user', 'ctx', 'os'] as const; + /** + * The slot-agnostic clause — provably true for any slot, specific to none. + * Used where the honest answer is "not measured for this slot", never as a + * softening of a cell that WAS measured. + */ + const FIELD_RULE_SLOT_CONSEQUENCE_GENERIC = + 'the predicate faults, and a faulting rule never produces the verdict you declared — each ' + + 'slot resolves the fault to its own fallback, and none of those fallbacks is yours'; + /** Axis 1 — see "the consequence, by SLOT" above. Every cell is measured. */ + const FIELD_RULE_SLOT_CONSEQUENCE: Record = { + visibleWhen: + 'the predicate faults and the renderer falls back to VISIBLE ' + + '(`resolveFieldRuleState` evaluates visibility with `fallback: true`, and no server-side ' + + 'gate evaluates a field-level `visibleWhen` at all), leaving the field the test was meant ' + + 'to hide showing for everyone (#6146)', + readonlyWhen: + 'the predicate faults — and the two ends fault in OPPOSITE directions. The server treats ' + + 'the field as LOCKED (`isReadonlyWhenLocked` will not waive a declared lock it could not ' + + 'evaluate, #4889) and drops your value from the payload, while the form still renders the ' + + 'field editable (`fallback: false`). Per ADR-0057 D10 the server is the one that decides: ' + + 'the field looks writable, the save reports success, and the value silently never lands', + requiredWhen: + 'the predicate faults and the requirement is never enforced anywhere — the server logs it ' + + 'and SKIPS the check (fail-open, #4977 deliberately did not take #4889\'s carve-out) and ' + + 'the form does not mark the field required either, so a record saves with the field empty', + // Listed rather than left to the `??` below, so the map covers every slot + // the field walk passes and the default stays unreachable. `FieldSchema` + // declares this key only as a `retiredKey`, which rejects it by name, so + // there is no fourth runtime to measure — the honest clause is the generic + // one, not a fabricated fourth cell (#6716). + conditionalRequired: FIELD_RULE_SLOT_CONSEQUENCE_GENERIC, + }; + const checkFieldRuleRoot = (where: string, slot: string, raw: unknown): void => { const source = celSourceOf(raw); if (!source) return; const roots = collectCelRootIdentifiers(source); if (!roots.ok) return; - // One issue per slot even when a predicate reaches for two of them; the - // tie-break is this list's order (canonical spelling first), so the message - // is stable rather than dependent on AST walk order. - const root = FIELD_UNBOUND_USER_ROOTS.find((r) => roots.roots.includes(r)); - if (root === undefined) return; + // Filtered through SCOPE_ROOTS, in SCOPE_ROOTS order — so a bare field + // reference (owned by the bare-reference check one line up) and a CEL type + // name (`type(record.x) == string`) can never land here. + const kept = SCOPE_ROOTS.filter( + (r) => !(FIELD_RULE_BOUND_ROOTS as readonly string[]).includes(r) && roots.roots.includes(r), + ); + if (kept.length === 0) return; + // One issue per slot even when a predicate reaches for two rejected roots. + // User roots win the tie-break (canonical spelling first) so #6585's + // message stays stable; anything else falls back to SCOPE_ROOTS order, + // which is stable too — never AST walk order. + const root = FIELD_RULE_USER_ROOTS.find((r) => kept.includes(r)) ?? kept[0]!; + const prescription = (FIELD_RULE_USER_ROOTS as readonly string[]).includes(root) + ? `To gate the CHOICES of a select by user, move the predicate to the option's own ` + + `\`visibleWhen\` (\`options: [{ …, visibleWhen: … }]\`) — per-option is the one \`*When\` ` + + `surface that binds \`current_user\` and its ADR-0068 aliases. To hide the FIELD by role, ` + + `declare field-level security on a permission set ` + + `(\`fields: { '.': { readable: false } }\`), which the server enforces. ` + + `To gate on record state, rewrite the predicate against \`record\`.` + : root === 'data' + // The `*.form` spelling is deliberately not `*.form.ts`: this is a + // STRING literal, and #5017's receiver scan strips comments but not + // strings, so `form.ts` inside the message registers `form` as a read + // receiver of this rule. Measured — it went red on the first run. + ? `\`data\` is the root of a METADATA form (a \`*.form\` module — the metadata row ` + + `being edited); ` + + `this is an OBJECT field, whose runtime form binds the row as \`record\` — one key name, ` + + `two form kinds, two roots. Rewrite \`data.\` as \`record.\`.` + : `\`${root}\` is declared platform-wide and bound at OTHER evaluation sites (flow, ` + + `automation, screen and action predicates), never at the field level. Rewrite the ` + + `predicate against \`record\` (plus \`previous\`, and \`parent\` on a master-detail line ` + + `item), or move the decision to a surface that binds \`${root}\`.`; issues.push({ where, message: `\`${slot}\` reads \`${root}\`, but a field-level conditional rule binds only ` + `\`record\` (plus \`previous\`, and \`parent\` on a master-detail line item) — ` + - `\`${root}\` is unbound here, so the predicate faults and falls back to VISIBLE, ` + - `leaving the field the test was meant to hide showing for everyone (#6146). ` + - `To gate the CHOICES of a select by user, move the predicate to the option's own ` + - `\`visibleWhen\` (\`options: [{ …, visibleWhen: … }]\`) — per-option is the one \`*When\` ` + - `surface that binds \`current_user\`. To hide the FIELD by role, declare field-level ` + - `security on a permission set (\`fields: { '.': { readable: false } }\`), ` + - `which the server enforces. To gate on record state, rewrite the predicate against ` + - `\`record\`.`, + `\`${root}\` is unbound here, so ` + + `${FIELD_RULE_SLOT_CONSEQUENCE[slot] ?? FIELD_RULE_SLOT_CONSEQUENCE_GENERIC}. ` + + prescription, source, severity: 'error', }); @@ -680,7 +861,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // not enforced = data-integrity hole). #1928 class, same as actions. for (const key of ['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen'] as const) { check(`object '${objectName}' · field '${fname}' ${key}`, (f as AnyRec)[key], objectName, 'record'); - checkFieldRuleUserRoot(`object '${objectName}' · field '${fname}' ${key}`, key, (f as AnyRec)[key]); + checkFieldRuleRoot(`object '${objectName}' · field '${fname}' ${key}`, key, (f as AnyRec)[key]); } // [#6290] Per-OPTION `visibleWhen` — a `select`/`multiselect`/`radio` // option's own predicate (`SelectOptionSchema.visibleWhen`, @@ -697,7 +878,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // `resolveCascadingOptions` against the host's predicate scope, which // binds it (ADR-0068 / objectui#2284), and the showcase's // `'admin' in current_user.positions` is the pinned legal usage — while - // `checkFieldRuleUserRoot` above rejects it one level up, where nothing + // `checkFieldRuleRoot` above rejects it one level up, where nothing // binds it. Same helper, two verdicts, because the two surfaces have two // evaluators; neither verdict is a side effect of a shared root list. for (const [oi, opt] of asArray(f.options).entries()) { From c94b95c1add76bf240fbc3483e84fe4ff2285f2e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:58:55 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs(changeset):=20=E5=AD=97=E6=AE=B5?= =?UTF-8?q?=E7=BA=A7=20*When=20=E6=A0=B9=E7=99=BD=E5=90=8D=E5=8D=95?= =?UTF-8?q?=E4=B8=8E=E6=8C=89=E6=A7=BD=E4=BD=8D=E5=88=86=E6=A1=A3=E7=9A=84?= =?UTF-8?q?=E5=9B=A0=E6=9E=9C=E5=8F=A5=20(#6713,=20#6716)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZgRyPVwi1jLb1mNNuUQ9o --- .changeset/field-when-root-allowlist.md | 75 +++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 .changeset/field-when-root-allowlist.md diff --git a/.changeset/field-when-root-allowlist.md b/.changeset/field-when-root-allowlist.md new file mode 100644 index 0000000000..8a46aec0c9 --- /dev/null +++ b/.changeset/field-when-root-allowlist.md @@ -0,0 +1,75 @@ +--- +"@objectstack/formula": patch +"@objectstack/lint": patch +--- + +字段级 `*When` 的未绑定根检查:黑名单翻成白名单,并把因果句按槽位分档 + +同一段诊断上的两条**正交**分档轴,一次设计通过 —— 分开做会把这段文案写两遍, +且第二遍推翻第一遍。 + +## 轴一:根集合从 3 项黑名单翻成 3 项白名单(#6713) + +字段级 `visibleWhen` / `readonlyWhen` / `requiredWhen` 实测只绑 `record`、 +`previous`、`parent` 三个根,三处独立证据一致:服务端 +`rule-validator.ts` 的两处绑定(`readonlyWhen` 绑 +`{ record, previous, extra: { parent } }`,`requiredWhen` 绑 +`{ record, previous, ...parentScope }`);客户端 `evalFieldPredicate` 绑 +`record` + `previous` + 调用方 `scope`,而 objectui 全部五个字段级调用点 +(`form.tsx` ×3、`WizardForm.tsx`、`GridField.tsx`)传的 `scope` 只可能是 +`undefined` 或 `{ parent }`;作者端 objectui 的 +`FIELD_RULE_ROOTS = ['record', 'previous', 'parent']`,注释明写 "nothing else"。 + +而检查此前是一张**黑名单** —— #6584 一项、#6711 三项 +(`current_user` / `user` / `ctx`)。黑名单在这个面上结构性地追不上 +`SCOPE_ROOTS`:每新增一个根都要有人记得抄过来(`current_user` 自己就是 #6290 +加进去、#6584 才被发现的)。实测有 **21 个根**落在这条缝里,它们同样未绑定、 +同样 fault、而且同样**静默** —— 都在 `SCOPE_ROOTS` 里,所以裸引用检查也从不 +报它们。其中两个是高可信度的作者笔误而非理论成员: + +- `os.user.id` —— ADR-0068 D1 的**第四种**用户拼写(`buildScope` 把同一个 + `EvalUser` 挂在 `current_user` / `user` / `ctx.user` / `os.user` 下),#6711 + 收了三种,`os` 这一支没收; +- `data.status == 'x'` —— `data` 是**元数据表单**里同一个 `visibleWhen` 键的 + **合法**根(`view.zod.ts`:"Root: `record` … in runtime forms, or `data` in + metadata forms"),两种表单同一个键名、不同的根。 + +判定改为 `SCOPE_ROOTS` 成员减去白名单,列表直接从 `@objectstack/formula` 取, +不在消费端重述 —— 因此 `SCOPE_ROOTS` 将来新增的成员自动被覆盖。 + +处方随之**按根分档**:用户根(`current_user` / `user` / `ctx` / `os`)保留原有 +的选项级 `visibleWhen` 与权限集 FLS 两条用户向处方;`data` 给出元数据表单 vs +运行期表单的解释;其余根给出通用的「改写成 `record` 谓词」。此前只有用户向处方, +对写了 `data.type == 'select'` 的作者是答非所问。 + +## 轴二:因果句按槽位分档(#6716) + +三个槽位此前共用一句「falls back to VISIBLE … showing for everyone」,而这句话 +只对其中一个精确。三格全部**实测**,每格量了两端: + +- **`visibleWhen` —— 仅客户端、fail-OPEN,原文案正确。** 服务端根本不评估字段级 + `visibleWhen`(`ConditionalFieldDef` 无此成员,`fieldsNeedPrior` 只看 + `requiredWhen || readonlyWhen ||` 选项可见性),唯一裁决来自渲染端, + `resolveFieldRuleState` 对可见性传 `fallback: true`。 +- **`readonlyWhen` —— 两端方向相反,服务端说了算,原文案是反的。** 服务端 + `isReadonlyWhenLocked` 命中 `unknownVariableOf` 后返回 `true`(#4889 的 + carve-out,其触发条件正是未绑定根这一类),`stripReadonlyWhenFields` 随即把该 + 字段从 payload 中删除;客户端 `resolveFieldRuleState` 传 `fallback: false`, + 表单仍渲染为可编辑。按 ADR-0057 D10(server enforces, client is courtesy)以 + 服务端为准:作者改了字段、保存报成功、值静默不落库。原文案告诉作者「对所有人 + 可见」—— 失败方向与排障方向都相反。 +- **`requiredWhen` —— 两端都 fail-OPEN,且与可见性无关。** 服务端记日志后 + `continue`(#4977 明确没有采用 #4889 的 carve-out),客户端 `fallback: false`, + 两端都不强制,记录带着空字段保存成功。原文案在这里不只是不精确,而是说错了 + 字段的哪个属性。 + +`conditionalRequired` 在 `FieldSchema` 里是 `retiredKey`(按名字拒绝),解析后的 +编译路径上该分支是惰性的,因此给它一条与槽位无关的通用句,而不是编造第四格测量。 + +## `@objectstack/formula` + +`SCOPE_ROOTS` 改为公开导出。一个绑定**封闭**根集合的面,必须能说出它**不**绑定 +的那些根,而那个补集就是 `SCOPE_ROOTS` 减去该面自己的白名单;消费端手抄的列表 +追不上这张表。注意它不能用 `firstUndeclaredReference` 替代:严格环境同时声明了 +CEL 的**类型名**,`type(record.x) == string` 里的 `string` 会被判成「能解析的根」 +—— 实测按可解析性判定会误杀这条合法谓词(1 例),按 `SCOPE_ROOTS` 成员判定不会。