From d1db22935048fb1f9cc253f693f495704c59a637 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:05:12 +0000 Subject: [PATCH] chore(spec): scan platform-object definitions for permissive empty states (#3896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. It now scans **/*.object.ts anywhere under packages/ — plugins, platform-objects, metadata-core, and the create-objectstack templates, since 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. 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. 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 — the point being that the answer now lives somewhere other than a maintainer's memory. Three fixes the new surface forced, each the gate being wrong in a way that mattered: - Repudiated prose no longer fires. #3929's own comment reads Deliberately NOT "leave empty to share everything" — the gate flagged the sentence recording why the gate exists. Negation now covers the imperative form as well as the token form (deny-all), and the escape is narrow on purpose: 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 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. - The property-search window is per-surface. A platform-object description: can sit 15+ lines below its field name, while a .zod.ts statement sits beside its property. Widening globally would let .zod.ts narrative be mis-attributed to a distant property — a correct note becoming a wrong failure. Evidence resolution is now honest too: entries are parsed with the liveness ledger's own checkEvidence, so prose works, several paths can be cited, and another repo's path is recorded without being resolved here. The README already promised evidence resolved "like the ledger's"; a raw-path existsSync quietly did not. 6 new unit tests (32 total). spec: 263 files / 6862 tests green, tsc clean, both gates green. No runtime behaviour changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --- .changeset/empty-state-gate-object-surface.md | 56 ++++++ packages/spec/liveness/README.md | 43 ++++- .../scripts/liveness/check-empty-state.mts | 40 ++++- .../scripts/liveness/empty-state-registry.mts | 10 ++ .../spec/scripts/liveness/empty-state.mts | 165 +++++++++++++++--- .../spec/scripts/liveness/empty-state.test.ts | 66 ++++++- 6 files changed, 344 insertions(+), 36 deletions(-) create mode 100644 .changeset/empty-state-gate-object-surface.md 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');