diff --git a/.changeset/empty-state-gate-object-surface.md b/.changeset/empty-state-gate-object-surface.md new file mode 100644 index 0000000000..93ee6e0ff7 --- /dev/null +++ b/.changeset/empty-state-gate-object-surface.md @@ -0,0 +1,56 @@ +--- +"@objectstack/spec": patch +--- + +chore(spec): the empty-state gate now scans platform-object definitions, where #3896 actually happened + +#3945 added a gate requiring any *"empty = permissive"* statement in the spec to +be classified on purpose. It scanned `packages/spec/src/**/*.zod.ts` — and that +scope had a hole big enough to miss the bug it was built for. + +The sentence that shipped #3896, *"leave empty to share every record"*, was the +`description` of `sys_sharing_rule.criteria_json`, which lives in +**`plugin-sharing`**. The gate could not see its own crime scene. + +**Now scans `**/*.object.ts` anywhere under `packages/`** — plugins, +`platform-objects`, `metadata-core`, and the `create-objectstack` templates +(a starter file is the highest-leverage thing a model copies from). 214 → 290 +files. + +**It immediately found a real one.** `sys_user_permission_set.organization_id` +declares *"NULL = applies in every org context"*: a user↔permission-set grant with +no org scope applies everywhere. That is deliberate and load-bearing rather than +an oversight — ADR-0095 D3 / ADR-0068 D2 derive the `platform_admin` posture from +an **unscoped** `admin_full_access` grant specifically, and an org-scoped grant of +the same set must not confer it. So the empty state is not merely wider, it is the +distinguishing input to the highest privilege in the system. Registered `open` +with that rationale and both enforcement sites cited, which is the point: the +answer now lives somewhere other than a maintainer's memory. + +Three fixes the new surface forced, each a case of the gate being wrong in a way +that mattered: + +- **Repudiated prose no longer fires.** #3929's own comment on `criteria_json` + reads *Deliberately NOT "leave empty to share everything"* — the gate flagged the + sentence recording why the gate exists. Negation is now handled for the + imperative form as well as the token form (`deny-all`), and the escape is + deliberately narrow: the negator must be attached to the phrase, not merely + present in the line, because a false negative here is a missed over-share. +- **The owning property is found by nesting, not by a name list.** A field's prose + sits in a nested key, so the first attempt answered `description` for every + platform-object hit; skipping doc slots then answered `required`, the sibling + above it. What separates a field from its own config is indentation, so the + resolver now takes the nearest key at a shallower indent. +- **The property-search window is per-surface.** A platform-object `description:` + can sit 15+ lines below its field name; a `.zod.ts` statement sits beside its + property. Widening globally would let `.zod.ts` narrative be mis-attributed to a + distant property — turning a correct non-failing note into a wrong failure — so + `.object.ts` gets a wider window and the schema surface keeps its tight one. + +Also makes evidence resolution honest: entries are now parsed with the liveness +ledger's own `checkEvidence`, so prose around the paths works, several paths can +be cited, and another repo's path (`objectui: …`) is recorded without being +resolved here. The README already promised evidence resolved "like the ledger's"; +a single raw-path `existsSync` quietly did not. + +6 new unit tests (32 total). No runtime behaviour changes. diff --git a/packages/spec/liveness/README.md b/packages/spec/liveness/README.md index 132be68dc1..b88a4d7d59 100644 --- a/packages/spec/liveness/README.md +++ b/packages/spec/liveness/README.md @@ -337,13 +337,15 @@ Three properties, same syntactic shape (an optional list or predicate that | Property | Empty means | |---|---| | `object.apiMethods` | `undefined` = unrestricted, **`[]` = deny-all** | -| `plugin-runtime.allowedSources` | was *"empty = all allowed"* — corrected | +| `sys_user_permission_set.organization_id` | NULL = the grant applies in **every** org — and is what derives `platform_admin` | +| `plugin-runtime.allowedSources` | was *"empty = all allowed"* — schema since removed (#3950) | | sharing `condition` | nothing is shared (#3929) | Nothing marked which was which. A maintainer knows by memory; a model authoring metadata cannot, and guessing wrong is silent and permissive. So the gate scans -the schema surface for statements declaring an empty state to be permissive, and -requires each to be classified in `../scripts/liveness/empty-state-registry.mts`: +the authorable surface for statements declaring an empty state to be permissive, +and requires each to be classified in +`../scripts/liveness/empty-state-registry.mts`: | `semantics` | Meaning | |---|---| @@ -373,6 +375,41 @@ check is a check nobody reads. A statement that resolves to no property is narrative — a file header explaining a past bug — and is reported as a non-failing note. +### What it scans, and why it is not just `packages/spec` + +Two surfaces: + +- `packages/spec/src/**/*.zod.ts` — the schema surface. +- `**/*.object.ts` anywhere under `packages/` — platform-object definitions, in + plugins, `platform-objects`, `metadata-core`, and the `create-objectstack` + templates (a starter file is the highest-leverage thing a model copies from). + +**The second one is the point.** The sentence that shipped #3896 — *"leave empty +to share every record"* — was the `description` of +`sys_sharing_rule.criteria_json`, which lives in `plugin-sharing`. A gate scoped +to `packages/spec` could not see the crime scene. Extending the scan immediately +surfaced one unclassified access-control default-open that no sweep of the schema +surface would ever have reached (`sys_user_permission_set.organization_id`). + +Two things the two surfaces do NOT share, both learned by getting them wrong: + +- **The property-search window.** A `.zod.ts` statement sits on or beside its + property; a platform-object field is a nested call whose `description:` can be + 15+ lines below the name. The window is therefore per-surface — widening it + globally would let `.zod.ts` narrative be mis-attributed to a distant property, + turning a correct note into a wrong failure. +- **How the owning property is found.** Not by a list of key names. Skipping + `description:` is not enough — the next key up from it is `required:`, equally + not the field. What separates a field from its own config is *nesting*, so the + resolver takes the nearest key at a **shallower indent**. + +Finally, a match whose permissive claim is **negated or explicitly disowned** is +dropped, in both directions: `[]` = `deny-all` is the opposite of the hazard, and +so is #3929's own comment saying *Deliberately NOT "leave empty to share +everything"*. The escape is narrow on purpose — the negator has to be attached to +the phrase, not merely nearby, because a false negative here is a missed +over-share. + ## Files & usage - `.json` — the ledger for a governed metadata type. diff --git a/packages/spec/scripts/liveness/check-empty-state.mts b/packages/spec/scripts/liveness/check-empty-state.mts index d6c8996c90..7a9ae51761 100644 --- a/packages/spec/scripts/liveness/check-empty-state.mts +++ b/packages/spec/scripts/liveness/check-empty-state.mts @@ -17,17 +17,39 @@ import { checkEmptyState } from './empty-state.mts'; const here = dirname(fileURLToPath(import.meta.url)); const specRoot = resolve(here, '../..'); // packages/spec const repoRoot = resolve(specRoot, '../..'); -const srcRoot = join(specRoot, 'src'); - -/** The authorable spec surface: the schema files an author (or a model) reads. */ -function collectSchemaFiles(dir: string, out: string[] = []): string[] { +const packagesRoot = resolve(repoRoot, 'packages'); + +// Build output, dependencies and caches are copies — scanning them would report +// the same statement several times and flag paths nobody edits. +const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', '.turbo', 'coverage', '.git']); + +/** + * The authorable surface: everything an author — very often a model — reads as + * the contract. + * + * TWO kinds of file, and the second is the reason this gate exists at all: + * + * - `packages/spec/src/**\/*.zod.ts` — the schema surface. + * - `**\/*.object.ts` — platform-object definitions, wherever they live (plugins, + * `platform-objects`, `metadata-core`, the `create-objectstack` templates). + * The sentence that shipped #3896 — *"leave empty to share every record"* — was + * the `description` of `sys_sharing_rule.criteria_json`, which lives in + * `plugin-sharing`. A gate scoped to `packages/spec` could not see the crime + * scene. Templates are included deliberately: a starter file is the highest- + * leverage thing a model copies from. + */ +function collectAuthorableFiles(dir: string, out: string[] = []): string[] { for (const entry of readdirSync(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); if (entry.isDirectory()) { - collectSchemaFiles(full, out); - } else if (entry.name.endsWith('.zod.ts') && !entry.name.endsWith('.test.ts')) { - out.push(full); + if (SKIP_DIRS.has(entry.name)) continue; + collectAuthorableFiles(join(dir, entry.name), out); + continue; } + if (entry.name.endsWith('.test.ts')) continue; + const full = join(dir, entry.name); + const rel = relative(repoRoot, full); + if (entry.name.endsWith('.object.ts')) out.push(full); + else if (entry.name.endsWith('.zod.ts') && rel.startsWith(join('packages', 'spec', 'src'))) out.push(full); } return out; } @@ -36,7 +58,7 @@ const args = process.argv.slice(2); const asJson = args.includes('--json'); const dump = args.includes('--dump'); -const files = collectSchemaFiles(srcRoot).sort(); +const files = collectAuthorableFiles(packagesRoot).sort(); const sources = new Map( files.map((f) => [relative(repoRoot, f), readFileSync(f, 'utf8')]), ); diff --git a/packages/spec/scripts/liveness/empty-state-registry.mts b/packages/spec/scripts/liveness/empty-state-registry.mts index 5a25f6ca50..80bda83a6c 100644 --- a/packages/spec/scripts/liveness/empty-state-registry.mts +++ b/packages/spec/scripts/liveness/empty-state-registry.mts @@ -107,6 +107,16 @@ export const EMPTY_STATE_REGISTRY: EmptyStateEntry[] = [ evidence: 'packages/spec/src/security/sharing.zod.ts', }, + { + file: 'packages/plugins/plugin-security/src/objects/sys-user-permission-set.object.ts', + property: 'organization_id', + semantics: 'open', + rationale: + "A NULL organization scope on a user↔permission-set grant means the grant is UNSCOPED — it applies in every org context. Deliberate and load-bearing rather than an oversight: ADR-0095 D3 / ADR-0068 D2 DERIVE the platform_admin posture from an unscoped `admin_full_access` user grant specifically, and an org-SCOPED grant of the same set must not confer it. So the empty state is not merely wider, it is the distinguishing input to the highest privilege in the system — which is exactly why it belongs on this list rather than being left to memory. `explain-engine.ts` recomputes the identical predicate on purpose so the explain panel's posture cannot sit higher than enforcement's.", + evidence: + 'packages/core/src/security/resolve-authz-context.ts (resolveAuthzContext.hasPlatformAdminGrant — the single source of truth) and packages/plugins/plugin-security/src/explain-engine.ts (the identical predicate, replicated so the panel cannot overstate)', + }, + // ---- Scope selectors --------------------------------------------------- { diff --git a/packages/spec/scripts/liveness/empty-state.mts b/packages/spec/scripts/liveness/empty-state.mts index 7e15d736e3..9e4e2e7b46 100644 --- a/packages/spec/scripts/liveness/empty-state.mts +++ b/packages/spec/scripts/liveness/empty-state.mts @@ -17,6 +17,7 @@ // Pure by construction — every entry point takes its inputs, so the unit tests // never touch the filesystem. +import { checkEvidence } from './evidence.mts'; import type { EmptyStateEntry } from './empty-state-registry.mts'; /** A permissive-empty statement found in the spec surface. */ @@ -102,6 +103,21 @@ const LEAVE_EMPTY_RE = new RegExp( // the hazard; dropping it is not a tolerance, it is a correction. const NEGATED_BEFORE_RE = /(?:deny|denies|no|not|never|zero|none)[-\s]*$/i; +// The same correction for the imperative form, which needs a slightly wider +// window because the repudiated phrase is normally QUOTED: +// +// // Deliberately NOT "leave empty to share everything" (#3896) +// +// That comment is the fix for #3896 explaining itself, and flagging it would +// make the gate fire on the very sentence that records the gate's reason. +// +// The window admits only quotes, brackets and space between the negator and the +// phrase — NOT a free character budget. "if not set, empty = all" must still be +// caught: a false negative here is a missed over-share, which is strictly worse +// than a false positive, so the escape stays narrow enough to require that the +// negator be attached to the phrase rather than merely nearby. +const REPUDIATED_BEFORE_RE = /\b(?:not|never|no longer|rather than|instead of)\b[\s"'“‘(]*$/i; + // A match only counts as PROSE. `const empty = {}` is code and must not trip the // gate, so the match has to sit after a comment marker or inside a string. const PROSE_PREFIX_RE = /(\/\/|\/\*|\*|['"`])/; @@ -114,44 +130,126 @@ function isProse(line: string, index: number): boolean { const PROP_RE = /^\s*([A-Za-z_$][\w$]*)\s*:/; const DOC_LINE_RE = /^\s*(\/\*|\*|\/\/)/; -/** How far from the statement to look for the property it describes. */ -const MAX_PROPERTY_DISTANCE = 8; +/** + * How far from the statement to look for the property it describes. + * + * `.zod.ts` keeps a tight window: there, a statement is either on the property + * line or one line off it, and widening the search only creates opportunities to + * attribute narrative to a distant property — turning a correct non-failing note + * into a wrong failing finding. + * + * `*.object.ts` needs a wider one, because a platform-object field is a nested + * call whose prose sits several keys down from the name it belongs to: + * + * criteria_json: Field.textarea({ + * label: 'Criteria', + * required: false, + * widget: 'filter-condition', + * description: 'Which records to share. …', // ← 15+ lines from the name + * }), + */ +export const DEFAULT_MAX_PROPERTY_DISTANCE = 8; +export const OBJECT_FILE_MAX_PROPERTY_DISTANCE = 24; + +/** + * Keys that DOCUMENT a property rather than being one. + * + * These matter only for `*.object.ts`, where the prose lives in a nested key. A + * field's `description:` line matches the property pattern itself, so without + * this the resolver answers `description` for every platform-object hit — a name + * that is not a property, cannot be registered meaningfully, and would key every + * entry in a file to the same string. + */ +const DOC_SLOT_KEYS = new Set([ + 'description', + 'label', + 'helpText', + 'help_text', + 'hint', + 'placeholder', + 'title', + 'summary', + 'note', + 'tooltip', + 'inlineHelpText', +]); /** * Resolve which property a statement belongs to. * * Direction matters and gets this wrong if you pick one: a JSDoc block sits - * ABOVE its property, while a `.describe(...)` argument sits BELOW it. Searching - * forward only mis-attributed `explain.zod.ts`'s effective-predicate describe to - * the *next* property down the file. + * ABOVE its property, while a `.describe(...)` argument (or a nested + * `description:`) sits BELOW it. Searching forward only mis-attributed + * `explain.zod.ts`'s effective-predicate describe to the *next* property down the + * file. */ -export function resolveProperty(lines: string[], matchLine: number): string | null { - const own = lines[matchLine]?.match(PROP_RE); - if (own) return own[1]; +/** Leading-whitespace width, the nesting signal. */ +function indentOf(line: string): number { + return (line.match(/^[ \t]*/)?.[0].length) ?? 0; +} + +export function resolveProperty( + lines: string[], + matchLine: number, + maxDistance: number = DEFAULT_MAX_PROPERTY_DISTANCE, +): string | null { + const matchIndent = indentOf(lines[matchLine] ?? ''); + + const nameAt = (i: number, maxIndent: number): string | null => { + const line = lines[i] ?? ''; + const m = line.match(PROP_RE); + if (!m) return null; + // A documentation slot is never the property being documented. + if (DOC_SLOT_KEYS.has(m[1]!)) return null; + // …and neither is a sibling config key. Skipping `description` alone was not + // enough: backward from a field's `description:` the next key up is + // `required:`, equally not the field. What separates the FIELD from its + // config is nesting — the field name opens the block the keys sit inside, so + // it is the nearest key at a SHALLOWER indent. + return indentOf(line) <= maxIndent ? m[1]! : null; + }; + + // The statement's own line, when it is the property line itself. + const own = nameAt(matchLine, matchIndent); + if (own) return own; const isDoc = DOC_LINE_RE.test(lines[matchLine] ?? ''); + + // A doc comment sits at the SAME indent as the property beneath it (or one + // deeper, for the ` * ` continuation lines of a JSDoc block), so forward search + // allows equality. Backward search must not: a nested statement's siblings are + // at its own indent, and they are exactly what has to be skipped. const forward = () => { - for (let i = matchLine + 1; i <= matchLine + MAX_PROPERTY_DISTANCE && i < lines.length; i++) { - const m = lines[i]?.match(PROP_RE); - if (m) return m[1]; + for (let i = matchLine + 1; i <= matchLine + maxDistance && i < lines.length; i++) { + const n = nameAt(i, matchIndent); + if (n) return n; } return null; }; const backward = () => { - for (let i = matchLine - 1; i >= matchLine - MAX_PROPERTY_DISTANCE && i >= 0; i--) { - const m = lines[i]?.match(PROP_RE); - if (m) return m[1]; + for (let i = matchLine - 1; i >= matchLine - maxDistance && i >= 0; i--) { + const n = nameAt(i, matchIndent - 1); + if (n) return n; } return null; }; // Doc comment → its property is below. Anything else (a describe argument, a - // trailing comment) → above. + // nested `description:`, a trailing comment) → above. return isDoc ? (forward() ?? backward()) : (backward() ?? forward()); } -/** True when the permissive token this match hinged on is negated ("deny-all", "no limit"). */ +/** + * True when the match is negated — the line says the OPPOSITE of the hazard. + * + * Two windows, because the two forms are repudiated differently: a permissive + * TOKEN is negated by an adjacent prefix (`deny-all`), while the imperative + * PHRASE is normally quoted and disowned (`Deliberately NOT "leave empty to …"`). + */ function isNegated(line: string, match: RegExpExecArray): boolean { + const before = line.slice(0, match.index); + if (REPUDIATED_BEFORE_RE.test(before)) return true; + const token = match[2]; if (!token) return false; const tokenAt = match[0].lastIndexOf(token); @@ -159,10 +257,21 @@ function isNegated(line: string, match: RegExpExecArray): boolean { return NEGATED_BEFORE_RE.test(line.slice(0, match.index + tokenAt)); } +/** + * The property-search window for a file, chosen by what kind of surface it is. + * + * Exported so the gate and its tests agree by construction rather than by two + * copies of the same `endsWith` check. + */ +export function maxPropertyDistanceFor(file: string): number { + return file.endsWith('.object.ts') ? OBJECT_FILE_MAX_PROPERTY_DISTANCE : DEFAULT_MAX_PROPERTY_DISTANCE; +} + /** Find every permissive-empty statement in one file. */ -export function scanSource(file: string, source: string): PermissiveEmptyHit[] { +export function scanSource(file: string, source: string, maxDistance?: number): PermissiveEmptyHit[] { const lines = source.split('\n'); const hits: PermissiveEmptyHit[] = []; + const distance = maxDistance ?? maxPropertyDistanceFor(file); for (let i = 0; i < lines.length; i++) { const line = lines[i] ?? ''; @@ -175,7 +284,7 @@ export function scanSource(file: string, source: string): PermissiveEmptyHit[] { file, line: i + 1, text: line.trim(), - property: resolveProperty(lines, i), + property: resolveProperty(lines, i, distance), }); } @@ -237,7 +346,7 @@ export function checkEmptyState({ sources, registry, exists }: CheckInput): Chec line: hit.line, message: `Narrative, not a property contract: "${hit.text}". ` + - `If it does describe a property, move it within ${MAX_PROPERTY_DISTANCE} lines of the declaration so the gate can classify it.`, + `If it does describe a property, move it within ${maxPropertyDistanceFor(hit.file)} lines of the declaration so the gate can classify it.`, }); continue; } @@ -290,20 +399,30 @@ export function checkEmptyState({ sources, registry, exists }: CheckInput): Chec // Access gates must point at where their posture actually lives; scope // selectors and engine output have no enforcement site to cite. + // + // Evidence is parsed with the liveness ledger's own `checkEvidence`, so the + // two surfaces agree: prose around the paths is fine and encouraged, several + // paths may be cited, and a path attributed to another repo (`objectui: …`) + // is recorded without being resolved here. Reusing it also keeps this honest + // — the README promises evidence resolves "like the ledger's", and a + // single-raw-path `exists()` would have quietly not. if (entry.semantics === 'closed' || entry.semantics === 'open') { - if (!entry.evidence?.trim()) { + const scan = checkEvidence(entry.evidence, exists); + if (!entry.evidence?.trim() || (scan.local.length === 0 && scan.foreign.length === 0)) { findings.push({ kind: 'missing-evidence', file: entry.file, property: entry.property, - message: `'${entry.semantics}' is an access-gate classification and must cite where the posture is enforced.`, + message: + `'${entry.semantics}' is an access-gate classification and must cite where the posture is enforced, ` + + `as at least one repo-rooted path (e.g. packages/…/file.ts).`, }); - } else if (!exists(entry.evidence)) { + } else if (scan.missing.length > 0) { findings.push({ kind: 'rotted-evidence', file: entry.file, property: entry.property, - message: `Evidence path does not resolve: ${entry.evidence}`, + message: `Evidence path(s) do not resolve: ${scan.missing.join(', ')}`, }); } } diff --git a/packages/spec/scripts/liveness/empty-state.test.ts b/packages/spec/scripts/liveness/empty-state.test.ts index 1ab0a7a7b8..65919f0b73 100644 --- a/packages/spec/scripts/liveness/empty-state.test.ts +++ b/packages/spec/scripts/liveness/empty-state.test.ts @@ -9,7 +9,7 @@ import { describe, it, expect } from 'vitest'; -import { scanSource, resolveProperty, checkEmptyState } from './empty-state.mts'; +import { scanSource, resolveProperty, checkEmptyState, maxPropertyDistanceFor } from './empty-state.mts'; import { EMPTY_STATE_REGISTRY } from './empty-state-registry.mts'; import type { EmptyStateEntry } from './empty-state-registry.mts'; @@ -113,6 +113,70 @@ describe('resolveProperty — direction matters', () => { }); }); +describe('resolveProperty — platform-object field blocks', () => { + // The shape that broke the first attempt. A field is a nested call, so its + // prose sits several keys below the name it belongs to, and both of those keys + // look exactly like property declarations. + const field = [ + ' organization_id: Field.lookup(\'sys_organization\', {', // 0 + " label: 'Organization',", // 1 + ' required: false,', // 2 + " description: 'Optional organization scope. NULL = applies in every org context.',", // 3 + ' }),', // 4 + ]; + + it('skips the documentation slot the statement lives in', () => { + // Answering `description` would key every entry in a file to one string. + expect(resolveProperty(field, 3, 24)).not.toBe('description'); + }); + + it('skips SIBLING config keys too, and lands on the field', () => { + // `required:` is not a doc slot and not the field — it is a sibling at the + // same indent. Only nesting separates them, which is why the resolver + // compares indentation rather than keeping a list of key names. + expect(resolveProperty(field, 3, 24)).toBe('organization_id'); + }); + + it('reaches a field name further away than the .zod.ts window allows', () => { + const long = [ + ' criteria_json: Field.textarea({', // 0 + ...Array.from({ length: 12 }, (_, i) => ` opt${i}: true,`), // 1..12 + " description: 'Which records to share (empty = all).',", // 13 + ]; + expect(resolveProperty(long, 13, 24)).toBe('criteria_json'); + // With the tight schema-surface window it is simply out of reach — which is + // why the distance is chosen per surface rather than globally widened. + expect(resolveProperty(long, 13, 8)).toBeNull(); + }); + + it('picks the window from the file extension', () => { + expect(maxPropertyDistanceFor('packages/x/y.object.ts')).toBeGreaterThan( + maxPropertyDistanceFor('packages/spec/src/x.zod.ts'), + ); + }); +}); + +describe('scanSource — repudiated prose must not fire the gate', () => { + it('ignores a phrase the comment explicitly disowns', () => { + // This is #3929's own comment on sys_sharing_rule.criteria_json. Flagging it + // would make the gate fire on the sentence recording why the gate exists. + const src = [ + ' // Deliberately NOT "leave empty to share everything" (#3896): an empty', + ' // criteria never shared everything on purpose, it just failed open.', + " description: 'Which records to share. Required.',", + ].join('\n'); + expect(scanSource('x.object.ts', src)).toEqual([]); + }); + + it('still catches a permissive claim that merely CONTAINS a negation earlier', () => { + // The escape must stay attached to the phrase. "not set" here negates + // nothing about the consequence, and a false negative is a missed + // over-share — strictly worse than a false positive. + const src = " description: 'If not set, leave empty to share every record.',"; + expect(scanSource('x.object.ts', src)).toHaveLength(1); + }); +}); + describe('checkEmptyState — the ratchet', () => { const file = 'packages/spec/src/x.zod.ts'; const permissive = [" thing: z.array(z.string()).optional()", " .describe('Scope (empty = all)'),"].join('\n');