diff --git a/.changeset/form-predicate-root-section-binds-current-user.md b/.changeset/form-predicate-root-section-binds-current-user.md new file mode 100644 index 0000000000..b571b04d65 --- /dev/null +++ b/.changeset/form-predicate-root-section-binds-current-user.md @@ -0,0 +1,59 @@ +--- +"@objectstack/metadata-core": patch +"@objectstack/metadata": patch +--- + +fix(metadata-core,metadata): a form SECTION binds `current_user` too, so the unbound-root notice stops flagging one (#13072) + +Second correction to the unbound-root boot notice, and the same defect as the +first one a surface later. The notice judged a SECTION-level predicate against +`record` / `previous` / `parent` / `data`, sourced faithfully from the section +contract prose — which was stale. + +`current_user` and its ADR-0068 alias roots (`user`, `ctx.user`, `os.user`) +**resolve on a section-level `visibleWhen`**: objectui#6110 threads the host +shell's predicate scope into `isSectionVisible` where it used to pass +`undefined`, and objectui#6111 copies the authored `visibleWhen` onto the +`section-divider` pseudo-field whose predicate the SDUI form renderer evaluates +with that scope bound. #12914 re-measured the contract text accordingly. Until +this change, a legacy artifact carrying a legitimate section-level +`current_user.role == "admin"` predicate was reported at boot as an unbound root +that faults open — a notice about a predicate that resolves, which is the +cry-wolf failure the module's own doc forbids and the one that trains operators +to ignore the channel. + +**What changes:** one vocabulary now serves both form-view predicate surfaces — +`record`, `previous`, `parent`, `data`, `current_user`, `user`, `ctx`, `os`. A +section predicate rooted at the `current_user` family is silent; a section +predicate rooted at a bare field identifier is still reported, and the operator +line still prints the rule per surface for the surfaces the findings implicate. + +**Blast radius, stated without inflation:** this is a **notice**, not a refusal +— no parse change, no gate, no behaviour change, and it only runs inside the +versioned window `applyArtifactForwardConversions` opens. The cost it removes is +a false operator signal on legacy artifacts, not a broken runtime. + +**Removed export, with its migration:** `FIELD_ONLY_BOUND_PREDICATE_ROOTS` is +gone from `@objectstack/metadata-core`. The section binding empties it, and an +exported constant named `FIELD_ONLY_…` holding `[]` asserts a per-surface +difference that no renderer makes. FROM → TO: read +`BOUND_FORM_VIEW_PREDICATE_ROOTS` (every root bound on any form-view predicate) +or `BOUND_FORM_FIELD_PREDICATE_ROOTS` (the field question, the same list today). +No consumer can be carrying it: the notice has never shipped — the two +changesets that introduce it are still pending in `.changeset/`, the newest +published `@objectstack/metadata-core` is 17.2.0, and the commit that added +`form-predicate-root-policy.ts` is in no release tag. This was the last moment +at which the removal cost nothing. + +**Why the vocabulary is no longer justified by quoting the contract.** Both +times this list has been wrong, it was wrong by transcribing a correct-looking +sentence that the renderer had already moved past. The prose is a transcription +of a renderer and can only lag one, so membership is now stated as the mechanism +— *a root is bound on a surface iff some renderer threads a scope carrying it +into that surface's evaluator* — with the threading site named per entry, and +the module's test reads the LIVE `.describe()` text of +`FormFieldSchema.visibleWhen` / `FormSectionSchema.visibleWhen` out of +`@objectstack/spec` instead of copying it into a comment. A comment quoting that +sentence goes stale in silence, twice now; an assertion that fetches it cannot. + + diff --git a/packages/metadata-core/src/form-predicate-root-policy.test.ts b/packages/metadata-core/src/form-predicate-root-policy.test.ts index f1d855fe60..60dbab6039 100644 --- a/packages/metadata-core/src/form-predicate-root-policy.test.ts +++ b/packages/metadata-core/src/form-predicate-root-policy.test.ts @@ -12,10 +12,10 @@ */ import { describe, it, expect } from 'vitest'; +import { FormFieldSchema, FormSectionSchema } from '@objectstack/spec/ui'; import { BOUND_FORM_VIEW_PREDICATE_ROOTS, BOUND_FORM_FIELD_PREDICATE_ROOTS, - FIELD_ONLY_BOUND_PREDICATE_ROOTS, detectUnboundFormViewPredicateRoots, unboundRootsInCelSource, } from './form-predicate-root-policy.js'; @@ -44,44 +44,126 @@ function definitionWithFieldPredicate(predicate: unknown, object = 'crm_lead'): }; } +/** The same artifact with the predicate on the SECTION rather than on a field. */ +function definitionWithSectionPredicate(predicate: unknown, object = 'crm_lead'): unknown { + return { + manifest: { id: 'app.test', engines: { protocol: '^17.0.0-rc.1' } }, + views: [ + { + form: { + type: 'simple', + data: { object }, + sections: [ + { name: 'main', visibleWhen: predicate, fields: [{ field: 'name' }] }, + ], + }, + }, + ], + }; +} + const CEL = (source: string) => ({ dialect: 'cel', source }); -describe('the bound vocabulary comes from the contract, not from this module', () => { - it('the shared base — and therefore the SECTION vocabulary — is record / previous / parent / data', () => { - // `packages/spec/src/ui/view.zod.ts`, `FormSectionSchema.visibleWhen`: - // "Root: `record` (+ `previous`, `parent`) in runtime forms, or `data` in - // metadata forms. No `current_user` at section level — it is unbound here - // and the predicate would fault open." - expect([...BOUND_FORM_VIEW_PREDICATE_ROOTS]).toEqual(['record', 'previous', 'parent', 'data']); - expect(BOUND_FORM_VIEW_PREDICATE_ROOTS).not.toContain('current_user'); - }); +/** + * The `current_user` family as ROOT identifiers — `current_user` plus the + * ADR-0068 D1 aliases, whose two-segment spellings (`ctx.user`, `os.user`) put + * `ctx` and `os` in root position. + * + * Spelled out here rather than imported: this list used to BE an export + * (`FIELD_ONLY_BOUND_PREDICATE_ROOTS`), and asserting the module against its + * own constant would have made the cases below agree with any value it took. + */ +const CURRENT_USER_FAMILY_ROOTS = ['current_user', 'user', 'ctx', 'os'] as const; + +/** + * The `visibleWhen` contract sentence for one form-view surface, read LIVE out + * of `@objectstack/spec` instead of copied into a comment in this file. + * + * `FormFieldSchema` / `FormSectionSchema` are `strictObject(...).transform(...)` + * pipes (ADR-0089 D3a), so the authored `.describe()` text hangs off the pipe's + * INPUT shape, reached through zod 4's public `.in`. The guard is the point of + * the helper: if that accessor path ever moves, this file must go RED rather + * than hand every assertion below an `undefined` that quietly matches nothing. + */ +function visibleWhenContract(schema: unknown, label: string): string { + const shape = (schema as { in?: { shape?: Record } }).in?.shape; + const slot = shape?.visibleWhen as { description?: unknown } | undefined; + const prose = slot?.description; + expect( + typeof prose === 'string' && prose.length > 0, + `${label}.visibleWhen description unreadable — the accessor this pin depends ` + + 'on has moved. Fix the accessor; do not delete the pin.', + ).toBe(true); + return prose as string; +} - it('the FIELD vocabulary adds the current_user family (objectui#6010, re-measured by #12930)', () => { - // `FormFieldSchema.visibleWhen`: "`current_user` (and the ADR-0068 aliases - // `user` / `ctx.user` / `os.user`) resolves here since objectui#6010". - // This is the correction: the first version of this policy judged a field - // by the section vocabulary and false-flagged a legitimate predicate. - expect([...BOUND_FORM_FIELD_PREDICATE_ROOTS]).toEqual([ +describe('the bound vocabulary is checked against the LIVE contract, not a copy of it', () => { + /** + * ⚠️ This block used to CARRY the section contract sentence as a comment: + * + * "Root: `record` (+ `previous`, `parent`) in runtime forms, or `data` in + * metadata forms. No `current_user` at section level — it is unbound + * here and the predicate would fault open." + * + * #12914 replaced that sentence — objectui#6110 threads the host scope into + * `isSectionVisible`, objectui#6111 evaluates the section predicate on the + * `section-divider` pseudo-field with that scope bound — and the copy above + * went stale HERE in total silence, because no gate reads a comment. It is + * kept as history, the record of how this file failed, and replaced as a + * MECHANISM by the reads below: the sentence is now fetched from the schema + * at run time, so the next re-measurement of it fails this file instead of + * outliving it. + */ + it('both surfaces bind the current_user family, and the live contract still says so', () => { + for (const [label, schema] of [ + ['FormFieldSchema', FormFieldSchema], + ['FormSectionSchema', FormSectionSchema], + ] as const) { + const prose = visibleWhenContract(schema, label); + expect(prose, label).toMatch(/`current_user`[\s\S]{0,160}resolves here/); + for (const alias of ['`user`', '`ctx.user`', '`os.user`']) { + expect(prose, `${label} / ${alias}`).toContain(alias); + } + } + + expect([...BOUND_FORM_VIEW_PREDICATE_ROOTS]).toEqual([ 'record', 'previous', 'parent', 'data', 'current_user', 'user', 'ctx', 'os', ]); - // The field vocabulary is a strict superset — the base can never drift out - // from under it. - for (const root of BOUND_FORM_VIEW_PREDICATE_ROOTS) { - expect(BOUND_FORM_FIELD_PREDICATE_ROOTS, root).toContain(root); + for (const root of CURRENT_USER_FAMILY_ROOTS) { + expect(BOUND_FORM_VIEW_PREDICATE_ROOTS, root).toContain(root); } }); - it('judges the SAME predicate differently per surface — the whole point of the split', () => { + it('the FIELD and SECTION vocabularies are ONE list, not two that happen to match', () => { + // By identity, not by value: while these were two constants they could + // drift apart silently, which is precisely what happened to the section + // half. Nothing can now update one surface and leave the other behind. + expect(BOUND_FORM_FIELD_PREDICATE_ROOTS).toBe(BOUND_FORM_VIEW_PREDICATE_ROOTS); + }); + + it('judges the SAME predicate identically on both surfaces — the split is empty', () => { + // ⚠️ INVERTED IN PLACE. Was "judges the SAME predicate differently per + // surface — the whole point of the split", expecting `['current_user']` + // from the section vocabulary. The section binds the root since + // objectui#6110 + #6111 (contract landed by #12914), so the section answer + // is now `[]` too, and a finding there would be a boot notice about a + // predicate that resolves. const source = 'current_user.id == record.owner'; expect(unboundRootsInCelSource(source, BOUND_FORM_FIELD_PREDICATE_ROOTS)).toEqual([]); - expect(unboundRootsInCelSource(source, BOUND_FORM_VIEW_PREDICATE_ROOTS)).toEqual(['current_user']); + expect(unboundRootsInCelSource(source, BOUND_FORM_VIEW_PREDICATE_ROOTS)).toEqual([]); }); - it('defaults to the stricter (section) vocabulary, so a forgetful caller fails loudly', () => { - // A missed detection is silent; a false positive is findable. The default - // is chosen to fail in the findable direction — the traversal never uses it. - expect(unboundRootsInCelSource('current_user.id == record.owner')).toEqual(['current_user']); + it('defaults to the whole vocabulary — the verdict the traversal itself gives', () => { + // ⚠️ INVERTED IN PLACE. Was "defaults to the stricter (section) + // vocabulary, so a forgetful caller fails loudly", expecting + // `['current_user']`. There is no stricter vocabulary left to default to, + // and narrowing one purely to preserve that property would manufacture the + // false positive this module exists to avoid. + expect(unboundRootsInCelSource('current_user.id == record.owner')).toEqual([]); + // Non-vacuity: the default still judges — a genuinely unbound root is + // still reported without the caller naming a vocabulary. + expect(unboundRootsInCelSource('status == "unqualified"')).toEqual(['status']); }); }); @@ -171,22 +253,33 @@ describe('detectUnboundFormViewPredicateRoots — traversal', () => { ]); }); - it('stays SILENT on a field predicate rooted at the current_user family', () => { - // The regression this patch exists for: each of these resolves at field - // level (objectui#6010), so flagging one is crying wolf on a legitimate, - // correctly-authored predicate. - for (const root of FIELD_ONLY_BOUND_PREDICATE_ROOTS) { + it('stays SILENT on a current_user-family predicate on EITHER surface', () => { + // Both regressions in one loop. Each of these resolves at FIELD level + // (objectui#6010) and at SECTION level (objectui#6110 + #6111), so + // flagging one is crying wolf on a legitimate, correctly-authored + // predicate — the failure the module doc forbids, once per surface. + for (const root of CURRENT_USER_FAMILY_ROOTS) { const source = root === 'ctx' || root === 'os' ? `${root}.user.role == "admin"` : `${root}.role == "admin"`; expect( detectUnboundFormViewPredicateRoots(definitionWithFieldPredicate(CEL(source))), - source, + `field / ${source}`, + ).toEqual([]); + expect( + detectUnboundFormViewPredicateRoots(definitionWithSectionPredicate(CEL(source))), + `section / ${source}`, ).toEqual([]); } }); - it('still FLAGS the same root at SECTION level, where the contract says it is unbound', () => { + it('says NOTHING about the same root at SECTION level either — it binds there now', () => { + // ⚠️ INVERTED IN PLACE. This case asserted exactly ONE finding — the + // section slot — "where the contract says it is unbound", while the + // identical field predicate stayed silent. #12914 replaced that contract + // sentence, so the two slots now answer alike and the artifact below is + // healthy on both. A finding here would be a boot notice about a predicate + // that resolves, which the module doc names as worse than no notice. const findings = detectUnboundFormViewPredicateRoots({ views: [ { @@ -202,14 +295,28 @@ describe('detectUnboundFormViewPredicateRoots — traversal', () => { }, ], }); - // Exactly one: the section slot. The identical field predicate is silent. - expect(findings).toHaveLength(1); - expect(findings[0]!.surface).toBe('section'); - expect(findings[0]!.root).toBe('current_user'); - expect(findings[0]!.path).toBe('views[0].form.sections[0].visibleWhen'); + expect(findings).toEqual([]); + }); + + it('still flags a genuinely unbound SECTION root — the silence above is not blanket', () => { + // The control the inversion above needs: the section arm of the traversal + // still reports, so "no finding" there is a verdict about `current_user` + // and not a section scan that stopped running. + const findings = detectUnboundFormViewPredicateRoots( + definitionWithSectionPredicate(CEL('stage == "closed"')), + ); + expect(findings).toEqual([ + { + path: 'views[0].form.sections[0].visibleWhen', + view: 'crm_lead', + root: 'stage', + source: 'stage == "closed"', + surface: 'section', + }, + ]); }); - it('tags every finding with the surface that decided its vocabulary', () => { + it('tags every finding with the slot it sits in', () => { const findings = detectUnboundFormViewPredicateRoots({ views: [ { diff --git a/packages/metadata-core/src/form-predicate-root-policy.ts b/packages/metadata-core/src/form-predicate-root-policy.ts index dcc5963d52..596ed4d6cc 100644 --- a/packages/metadata-core/src/form-predicate-root-policy.ts +++ b/packages/metadata-core/src/form-predicate-root-policy.ts @@ -9,18 +9,20 @@ * A form-view predicate binds a fixed scope: `record` (plus `previous`, the * saved record, and `parent` for master-detail line items) in runtime record * forms, and `data` — the row under edit, at every depth, repeater rows - * included — in metadata-editing forms. A FIELD-level predicate additionally - * binds `current_user` and its ADR-0068 aliases (objectui#6010); a - * SECTION-level one does not. The contract states the failure mode beside the - * vocabulary (`packages/spec/src/ui/view.zod.ts`, + * included — in metadata-editing forms. BOTH predicate surfaces additionally + * bind `current_user` and its ADR-0068 aliases — a FIELD since objectui#6010, + * a SECTION since objectui#6110 + #6111. The contract states the failure mode + * beside the vocabulary (`packages/spec/src/ui/view.zod.ts`, * `FormFieldSchema.visibleWhen` / `FormSectionSchema.visibleWhen`): **a bare * identifier is UNBOUND, the predicate faults, and `visibleWhen`'s fault * fallback is `true`** — so a field the predicate was authored to hide renders * for everyone. * - * ⚠️ That per-surface split is load-bearing, not a detail: see - * {@link BOUND_FORM_FIELD_PREDICATE_ROOTS} for why this module quoted the - * contract correctly and was still wrong within a day of landing. + * ⚠️ The two surfaces did once bind different roots, and this module got that + * split wrong TWICE — once per surface, both times by transcribing prose that + * was faithful and stale. See {@link BOUND_FORM_VIEW_PREDICATE_ROOTS} for what + * the list is sourced from instead, and why that source can be refuted rather + * than merely re-read. * * That is quiet on its own, and lethal in combination with the authoring * pattern it exists to serve. Measured on a real deployment: an artifact built @@ -78,9 +80,11 @@ * - **An AST-only envelope passes.** `{ dialect: 'cel', ast }` with no * `source` is opaque at this layer — the same posture the spec's own * `features.*` root scanner takes. - * - **The vocabulary is per surface, because the contract is** — see - * {@link BOUND_FORM_FIELD_PREDICATE_ROOTS}. Judging a field predicate by the - * section vocabulary false-flags a legitimate `current_user` test. + * - **The vocabulary answers to the binding MECHANISM, not to a sentence + * about it** — see {@link BOUND_FORM_VIEW_PREDICATE_ROOTS}. Judging either + * surface against a stale vocabulary false-flags a legitimate `current_user` + * test, which is this list's only measured failure mode: it has now happened + * once on each surface. * - **Per-option `visibleWhen` is out of scope**, deliberately: options are * evaluated by a *different* evaluator (`resolveCascadingOptions`, ADR-0068), * and — unlike either surface scanned here — the write-path rule validator @@ -96,40 +100,86 @@ */ /** - * Roots bound on EVERY form-view predicate surface — and therefore exactly the - * SECTION-level vocabulary. + * Roots bound on EVERY form-view predicate surface — the FIELD slot and the + * SECTION slot alike, which is why one list now serves both. * - * Sourced from the contract prose on `FormSectionSchema.visibleWhen` - * (`packages/spec/src/ui/view.zod.ts`): `record` + `previous` + `parent` in - * runtime record forms, `data` in metadata-editing forms (and inside a - * repeater, where `data` is the ROW but is still spelled `data`). Both kinds - * are admitted together because an artifact's `views` collection carries both - * and the definition does not say which renderer will read a given form — the - * union is the direction that stays silent on a healthy artifact. + * ## ⛔ Do not re-source this list by transcribing the contract sentence * - * `current_user` is absent here and that is CORRECT for a section: the section - * docblock states it is unbound at that level and the predicate faults open. - * ⚠️ It is NOT correct for a field — see - * {@link BOUND_FORM_FIELD_PREDICATE_ROOTS}. + * That is how it has been wrong BOTH times it has been wrong, and the two are + * the same failure one surface apart: + * + * 1. **FIELD.** The first version omitted `current_user`, quoting + * `FormFieldSchema.visibleWhen` faithfully — the root genuinely had been + * unbound there (#6146). objectui#6010 had already bound it and the prose + * had not caught up; #12930 re-measured the prose, and this module needed a + * same-day correction. + * 2. **SECTION.** That correction then split the vocabulary and justified the + * section half by quoting `FormSectionSchema.visibleWhen` — *"`current_user` + * is absent here and that is CORRECT for a section: the section docblock + * states it is unbound at that level and the predicate faults open."* + * Faithful again, and stale again: objectui#6110 + #6111 had bound it, and + * #12914 re-measured the prose. This list is that second correction. + * + * The prose is a transcription of a renderer, so it can only ever LAG one. + * Membership here is therefore decided by the mechanism, stated so a reader can + * go and REFUTE it instead of re-reading a sentence: **a root is bound on a + * surface iff some renderer threads a scope carrying it into the evaluator that + * surface uses.** Each entry names the site that does the threading, which is + * what a re-measurement actually goes and looks at: + * + * - `record` / `previous` / `parent` — the runtime record-form renderer + * evaluates against the record under edit, its saved counterpart, and the + * master-detail parent of a line item. + * - `data` — the metadata-editing form renderer evaluates against the row under + * edit, and inside a repeater `data` is the ROW at every depth (#6254): there + * is no implicit row scope, so a bare identifier is unbound there too. + * - `current_user` and its ADR-0068 D1 alias roots — the aliases are spelled + * `user`, `ctx.user` and `os.user`, so as ROOT identifiers they are `user`, + * `ctx` and `os`. Threaded in by a DIFFERENT renderer per surface, which is + * why the two surfaces bound it a release apart: + * - FIELD — the form renderer passes the host shell's predicate scope into + * `evalFieldPredicate` / `resolveFieldRuleState` (`@object-ui/core`) as + * their `extra` scope (objectui#6010). + * - SECTION — the console form renderer threads that same scope into + * `isSectionVisible`, where it used to pass `undefined` (objectui#6110); + * and `ObjectForm` / `SplitForm` / `ModalForm` / `DrawerForm` copy the + * authored `visibleWhen` onto the `section-divider` pseudo-field, whose + * predicate the SDUI form renderer evaluates with that scope bound + * (objectui#6111). Before those two, the object-view chain dropped the key + * before any renderer saw it — which is what made the retired sentence + * above TRUE on the day it was written. + * + * Admitting the BARE `ctx` / `os` roots rather than only the two-segment alias + * is the deliberate silent direction: this detector reports fault-open risk, + * and a predicate reaching into the host context under either namespace is not + * the class it is hunting. + * + * And the guard against a third round is mechanical rather than editorial: this + * module's test reads the LIVE `.describe()` text of + * `FormFieldSchema.visibleWhen` and `FormSectionSchema.visibleWhen` out of + * `@objectstack/spec/ui` and fails when it stops agreeing with this list. A + * comment quoting that sentence goes stale in silence — twice now, because no + * gate reads a comment; an assertion that fetches the sentence cannot. + * + * Two limits the `current_user` binding does NOT remove — neither changes this + * detector's answer on either surface, and it is worth saying why: + * + * - It is a **rendering rule, never authorization** (nothing server-side + * evaluates a form-view `visibleWhen`, field or section). That is an + * authoring hazard, not a version-drift hazard, so it is not this boot + * notice's business. + * - The scope belongs to the HOST, so on the console's public form route + * (`/f/:slug`) no principal is published, the root is unbound, and the + * predicate faults open there. This notice stays silent on that: it reports + * what faults on the primary hosted routes, and a route-specific unboundness + * that is equally true of a freshly-built current artifact says nothing about + * the artifact's ERA — which is the only thing this notice claims to detect. */ export const BOUND_FORM_VIEW_PREDICATE_ROOTS: readonly string[] = [ 'record', 'previous', 'parent', 'data', -]; - -/** - * Roots bound ONLY on a field-level predicate: the canonical `current_user` - * and its ADR-0068 D1 alias roots. - * - * The aliases are spelled `user`, `ctx.user` and `os.user`, so as ROOT - * identifiers they are `user`, `ctx` and `os`. Admitting the bare `ctx` / `os` - * roots rather than only the two-segment alias is the deliberate silent - * direction: this detector reports fault-open risk, and a predicate reaching - * into the host context under either namespace is not the class it is hunting. - */ -export const FIELD_ONLY_BOUND_PREDICATE_ROOTS: readonly string[] = [ 'current_user', 'user', 'ctx', @@ -137,37 +187,29 @@ export const FIELD_ONLY_BOUND_PREDICATE_ROOTS: readonly string[] = [ ]; /** - * The FIELD-level vocabulary: the shared base plus the `current_user` family. + * The FIELD-level vocabulary — **the same list**, kept under its own name only + * because the operator-facing notice prints a rule per surface. * - * ⚠️ **This is a re-measurement, and the reason this module needed a same-day - * correction.** `current_user` was unbound at field level (#6146) and the first - * version of this policy said so, quoting the contract prose faithfully. It was - * bound by objectui#6010, and three spec text sites still said otherwise until - * #12930 re-measured them — one of those stale sites was the sentence this - * module was written against, and it landed on `main` while the policy's own PR - * was in flight. Judging a field predicate by the section vocabulary - * false-flags a legitimate `current_user.role == 'admin'` test on a legacy - * artifact, which is precisely the cry-wolf failure the module doc forbids. + * There is no field-only root left. This constant used to be spelled + * `[...BOUND_FORM_VIEW_PREDICATE_ROOTS, ...FIELD_ONLY_BOUND_PREDICATE_ROOTS]`; + * the section binding emptied that difference, and an exported constant named + * `FIELD_ONLY_…` holding `[]` would be a name asserting something no renderer + * does. It was REMOVED rather than emptied — nothing had published it yet, so + * this was the last moment at which removing it cost nothing (the changeset + * carries the evidence). * - * Two limits the binding does NOT remove, per the corrected prose — neither - * changes this detector's answer, and it is worth saying why: - * - * - It is a **rendering rule, never authorization** (nothing server-side - * evaluates a field `visibleWhen`). That is an authoring hazard, not a - * version-drift hazard, so it is not this boot notice's business. - * - The scope belongs to the HOST, so on the console's public form route - * (`/f/:slug`) no principal is published, the root is unbound, and the - * predicate faults open there. This notice stays silent on that: it reports - * what faults on the primary hosted routes, and a route-specific unboundness - * that is equally true of a freshly-built current artifact says nothing about - * the artifact's ERA — which is the only thing this notice claims to detect. + * ⚠️ Keeping this name is not a prediction that the split returns. It records + * that the QUESTION is still per surface: "what does a FIELD predicate bind?" + * and "what does a SECTION predicate bind?" are two questions with one answer + * today, and that answer rests on two different renderers (objectui#6010 versus + * objectui#6110 + #6111). Either can move without the other; when one does, + * this is where the divergence goes, and the live-contract assertion in this + * module's test is what makes the day it happens findable. */ -export const BOUND_FORM_FIELD_PREDICATE_ROOTS: readonly string[] = [ - ...BOUND_FORM_VIEW_PREDICATE_ROOTS, - ...FIELD_ONLY_BOUND_PREDICATE_ROOTS, -]; +export const BOUND_FORM_FIELD_PREDICATE_ROOTS: readonly string[] = + BOUND_FORM_VIEW_PREDICATE_ROOTS; -/** Which form-view slot a predicate sits in — the two have different vocabularies. */ +/** Which form-view slot a predicate sits in. Reported, never a vocabulary switch. */ export type FormPredicateSurface = 'field' | 'section'; /** One form-view predicate naming a root that is not bound where it evaluates. */ @@ -180,7 +222,12 @@ export interface UnboundFormPredicateRoot { root: string; /** The predicate's CEL source, verbatim. */ source: string; - /** The slot it sits in, which is what decided the vocabulary it was judged against. */ + /** + * The slot it sits in. Both slots bind the same roots + * ({@link BOUND_FORM_VIEW_PREDICATE_ROOTS}), so this did NOT decide the + * verdict — it is carried because the operator notice names the slot, and + * "field or section" is the first thing an author needs to go and look. + */ surface: FormPredicateSurface; } @@ -211,12 +258,20 @@ const FORM_PREDICATE_KEYS: readonly string[] = ['visibleWhen', 'visibleOn']; * Root identifiers named by one CEL source, minus every bound root, reserved * word and call target. Order-preserving and de-duplicated. * - * `boundRoots` is the vocabulary to judge against, because the two form-view - * slots do not share one — pass {@link BOUND_FORM_FIELD_PREDICATE_ROOTS} for a - * field and {@link BOUND_FORM_VIEW_PREDICATE_ROOTS} for a section. The default - * is the section (base) vocabulary: it is the stricter of the two, so a caller - * that forgets to say gets a false POSITIVE rather than a missed detection — - * loud and findable, instead of silent. The traversal below never relies on it. + * `boundRoots` is the vocabulary to judge against. It stays a parameter now + * that both form-view slots share one list, because the reason for it was never + * "the two differ today" — this function is the judgement, and the caller is + * what supplies a vocabulary to it. The default is + * {@link BOUND_FORM_VIEW_PREDICATE_ROOTS}, the whole vocabulary, so a caller + * that says nothing gets the same verdict the traversal below would give it. + * + * ⚠️ The retired argument for the old default was that it was the STRICTER of + * two lists, so a forgetful caller failed in the findable direction. That + * argument died with the split rather than surviving it: with one vocabulary + * there is no stricter option, and inventing a narrower default purely to keep + * the argument alive would manufacture the false positive this module exists to + * avoid. Recorded because a rationale of that shape otherwise outlives its + * premise — which is the failure this whole module keeps paying for. * * Exported for the pinned false-positive cases in this module's test — the * traversal below is the product surface, this is the judgement under it. @@ -269,8 +324,11 @@ function isPlainObject(value: unknown): value is Record { } /** - * Judge one predicate slot against the vocabulary of the surface it sits in — - * the two differ, so the surface is passed explicitly rather than defaulted. + * Judge one predicate slot, and record which slot it was. + * + * `surface` no longer selects a vocabulary — both slots bind the same roots + * (see {@link BOUND_FORM_VIEW_PREDICATE_ROOTS}) — and is still threaded through + * because every finding reports it. */ function scanPredicateSlot( predicate: unknown, @@ -281,9 +339,7 @@ function scanPredicateSlot( ): void { const source = readCelSource(predicate); if (source === null) return; - const boundRoots = - surface === 'field' ? BOUND_FORM_FIELD_PREDICATE_ROOTS : BOUND_FORM_VIEW_PREDICATE_ROOTS; - for (const root of unboundRootsInCelSource(source, boundRoots)) { + for (const root of unboundRootsInCelSource(source, BOUND_FORM_VIEW_PREDICATE_ROOTS)) { out.push({ path, view, root, source, surface }); } } diff --git a/packages/metadata/src/plugin-unbound-form-predicate-roots.test.ts b/packages/metadata/src/plugin-unbound-form-predicate-roots.test.ts index 0bb8839e02..ff7d68c2f4 100644 --- a/packages/metadata/src/plugin-unbound-form-predicate-roots.test.ts +++ b/packages/metadata/src/plugin-unbound-form-predicate-roots.test.ts @@ -74,9 +74,10 @@ describe('artifact door — unbound form-predicate roots are announced to the op for (const field of gated) expect(field.visibleWhen.source).not.toContain('record.'); // …and the fixture also carries the SILENT controls: a `record.`-rooted // predicate whose string literal contains identifier-shaped text, a - // FIELD-level `current_user` predicate (which resolves at that level - // since objectui#6010 and must never be flagged), and a second view - // that is entirely healthy. + // FIELD-level `current_user` predicate (which resolves there since + // objectui#6010 — and, since objectui#6110 + #6111, resolves at SECTION + // level too — so it must never be flagged), and a second view that is + // entirely healthy. expect(leadFields.at(-2).visibleWhen.source).toBe('record.note != "status unqualified"'); expect(leadFields.at(-1).visibleWhen.source).toBe('current_user.role == "admin"'); expect(fixture.views[1].form.sections[0].fields[1].visibleWhen.source) @@ -165,10 +166,11 @@ describe('artifact door — unbound form-predicate roots are announced to the op }); it('says NOTHING about an old artifact whose only predicates are field-level current_user tests', async () => { - // The regression this patch exists for. `current_user` and its ADR-0068 - // alias roots resolve at FIELD level (objectui#6010), so a legacy - // artifact using them is healthy on this surface — flagging it would be - // the cry-wolf class the card forbids. + // The regression the first correction existed for. `current_user` and + // its ADR-0068 alias roots resolve at FIELD level (objectui#6010), so a + // legacy artifact using them is healthy on this surface — flagging it + // would be the cry-wolf class the card forbids. The section half of the + // same claim is two cases below. for (const source of [ 'current_user.role == "admin"', 'user.roles.size() > 0', @@ -196,9 +198,18 @@ describe('artifact door — unbound form-predicate roots are announced to the op } }); - it('DOES flag the same root at section level, and prints the section vocabulary there', async () => { - // The other half of the split: the contract says `current_user` is - // unbound on a SECTION predicate and faults open. + it('says NOTHING about a SECTION-level current_user predicate either', async () => { + // ⚠️ INVERTED IN PLACE (#13072). This case read "DOES flag the same + // root at section level, and prints the section vocabulary there", and + // asserted one warning quoting `bound roots on a form SECTION: + // 'record', 'previous', 'parent', 'data'`. That vocabulary was derived + // from the section contract sentence #12914 replaced: objectui#6110 + // threads the host shell's predicate scope into `isSectionVisible` + // where it used to pass `undefined`, and objectui#6111 evaluates the + // authored section `visibleWhen` on the `section-divider` pseudo-field + // with the same scope bound. The predicate RESOLVES, so a notice about + // it would be a false operator signal on a healthy legacy artifact — + // the cry-wolf class this suite is deliberately weighted against. const fixture = loadFixture(); fixture.views = [{ form: { @@ -216,11 +227,38 @@ describe('artifact door — unbound form-predicate roots are announced to the op const ctx = fakeCtx(); await plugin._parseAndRegisterArtifact(ctx, fixture, 'fixture-section-cu'); + expect(unboundRootWarnings(ctx)).toEqual([]); + }); + + it('still flags a genuinely unbound SECTION root, and prints the section rule', async () => { + // The control the inversion above needs: the section arm of the scan + // still reports, so "no warning" up there is a verdict about + // `current_user` and not a section traversal that quietly stopped + // running. It also keeps the printed section vocabulary pinned at the + // door, which is where an operator reads it. + const fixture = loadFixture(); + fixture.views = [{ + form: { + type: 'simple', + data: { provider: 'object', object: 'crm_lead' }, + sections: [{ + name: 'lead', + visibleWhen: { dialect: 'cel', source: 'stage == "closed"' }, + fields: [{ field: 'internal_note' }], + }], + }, + }]; + + const plugin = newPlugin(); + const ctx = fakeCtx(); + await plugin._parseAndRegisterArtifact(ctx, fixture, 'fixture-section-bare'); + const warnings = unboundRootWarnings(ctx); expect(warnings).toHaveLength(1); - expect(warnings[0]!).toContain("'current_user'"); + expect(warnings[0]!).toContain("'stage'"); expect(warnings[0]!).toContain( - "bound roots on a form SECTION: 'record', 'previous', 'parent', 'data'", + "bound roots on a form SECTION: 'record', 'previous', 'parent', 'data', " + + "'current_user', 'user', 'ctx', 'os'", ); // No field findings here, so the field rule is not quoted. expect(warnings[0]!).not.toContain('form FIELD'); diff --git a/packages/metadata/src/plugin.ts b/packages/metadata/src/plugin.ts index 62a352661f..d858461b1c 100644 --- a/packages/metadata/src/plugin.ts +++ b/packages/metadata/src/plugin.ts @@ -829,12 +829,16 @@ export class MetadataPlugin implements Plugin { const roots = [...new Set(findings.map((f) => f.root))]; const quote = (list: readonly string[]) => list.map((v) => `'${v}'`).join(', '); - // The bound vocabulary differs between a field slot and a section slot - // (a field also binds the `current_user` family, objectui#6010), so - // print only the rule(s) the findings actually implicate. Printing one - // flat list would either understate the field vocabulary — reading as - // "your legitimate current_user predicate is broken" — or quote a - // section rule at an operator whose artifact has no section findings. + // Print only the rule(s) the findings actually implicate, so an + // operator is never shown a rule their artifact has no instance of. + // ⚠️ The two lists are the SAME list today: a field bound the + // `current_user` family at objectui#6010 and a section did at + // objectui#6110 + #6111, so the per-surface split is currently empty + // (`@objectstack/metadata-core`, `BOUND_FORM_VIEW_PREDICATE_ROOTS`, + // carries the mechanism and the two times this claim went stale). The + // arms stay per surface because the QUESTION is per surface — two + // different renderers bind the family, and either can move without the + // other. const surfaces = new Set(findings.map((f) => f.surface)); const vocabulary = [ surfaces.has('field')