diff --git a/.changeset/alias-tables-are-claims-about-the-schema.md b/.changeset/alias-tables-are-claims-about-the-schema.md new file mode 100644 index 0000000000..ff8cc9db2c --- /dev/null +++ b/.changeset/alias-tables-are-claims-about-the-schema.md @@ -0,0 +1,53 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): unknown-key suggestions no longer point authors at keys the schema rejects (#5013) + +An `aliases` table on an authoring schema is a **claim about that schema**, in +two halves: that the key it is filed under is one the shape *rejects* (an alias +is consulted only from the `unrecognized_keys` path, so a key the shape declares +can never reach it), and that the key it prescribes is one the shape *accepts*. +Nothing checked either half, and both were false on `main`. + +Writing `filter` on a report produced: + +``` +Unrecognized key(s) on this report: `filter`. … Did you mean `filter` -> `filters`? +``` + +and taking that advice produced a **second** rejection — `ReportSchema` declares +neither `filter` nor `filters`, only `runtimeFilter` — this time with no +suggestion at all. That is the exact failure the unknown-key strictness work +exists to remove, shipped by its own fix, and it is worst for AI authors whose +only signal is whether the parse complained. + +**What changed** — no authorable key was added or removed, so nothing that +parsed before stops parsing; only the guidance an author gets when a key is +rejected: + +- `ReportSchema` — `filter`, `filters`, `where` and `criteria` now all name + `runtimeFilter`, matching `JoinedReportBlockSchema`'s table verbatim so a + report and its sub-reports correct the author identically. +- `EmailTemplateDefinitionSchema` — `html`/`content` name `bodyHtml`, `text` + names `bodyText`, and `from`/`sender` name `fromOverride`. All five previously + named `body` / `fromAddress`, neither of which the schema declares. +- `SkillSchema` — `trigger` no longer renames onto `triggers` (never a key). + It now carries a prescription instead, because the correct answer is a split: + routing intent belongs in `triggerConditions`, natural-language intent in + `description` / `instructions`. A rename would have landed a phrase in an + array-of-conditions slot and been rejected on the value instead of the key. +- Six entries filed under keys their own schema already declares — and which + therefore could never run — are gone: `columns` and `chart` on `ReportSchema`, + `measures` and `filter` on `DatasetSchema`, `body` on `ActionSchema`. + +**What keeps it true** — `strictObject` now records each shape it builds, and +`alias-integrity.test.ts` judges every one of the 235 authoring surfaces in the +package against the runtime `.shape` it makes claims about. Reading the runtime +shape rather than the source is what makes it work: shapes spread +(`...MetadataProtectionFields`), ten alias tables are assembled rather than +written as literals, and two different schemas share the surface string +`'this field group'` — a source-literal reader is wrong or blind on all three. +An alias pointing at a **tombstone** is caught too, which the suggester's own +`knownKeys` filter cannot do, since the alias table is consulted before that +fallback runs. diff --git a/packages/spec/src/ai/skill.zod.ts b/packages/spec/src/ai/skill.zod.ts index 6a053ef051..ae8497e2d2 100644 --- a/packages/spec/src/ai/skill.zod.ts +++ b/packages/spec/src/ai/skill.zod.ts @@ -67,8 +67,27 @@ export const SkillSchema = lazySchema(() => strictObject({ surface: 'this skill', history: 'Until #4001 closed this shape these were dropped silently — the item still registered, minus whatever the key was meant to configure.', - aliases: { prompt: 'instructions', content: 'instructions', body: 'instructions', trigger: 'triggers', tool: 'tools' }, + aliases: { prompt: 'instructions', content: 'instructions', body: 'instructions', tool: 'tools' }, guidance: { + // #5013 — `trigger` used to be an ALIAS pointing at `triggers`, a key this + // schema has never declared: the author was told to write it, wrote it, and + // was rejected a second time with no suggestion left to give. + // + // It is not repointed at `triggerConditions`, because a rename is the wrong + // instrument here. The prescription the `triggerPhrases` tombstone below + // carries is a SPLIT — routing intent goes to `triggerConditions`, natural + // language to `description` / `instructions` — so an author who wrote + // `trigger: 'create a case'` and took a rename would land a phrase in an + // array-of-conditions slot and be rejected on the value instead of the key. + // That is ledger finding 7 exactly: this campaign's own fix signposting the + // way back into the failure mode it exists to kill. + trigger: + '`trigger` is not a skill key, and skills have never been activated by a phrase. ' + + 'Activation is `triggerConditions` (an AND of context field/operator/value) intersected ' + + "with the agent's `skills[]` allowlist, plus explicit /skill-name pinning. If you meant a " + + 'programmatic condition, write `triggerConditions: [{ field: …, operator: …, value: … }]`; ' + + 'if you meant natural-language intent for the LLM to route on, that belongs in ' + + '`description` / `instructions`, which are the strings actually put in front of the model.', permissions: '`permissions` is not a skill key — skill invocation was never permission-gated, ' + 'so this was stripped in silence and the author believed they had a gate. Gate at ' diff --git a/packages/spec/src/shared/alias-integrity.test.ts b/packages/spec/src/shared/alias-integrity.test.ts new file mode 100644 index 0000000000..19f13ad853 --- /dev/null +++ b/packages/spec/src/shared/alias-integrity.test.ts @@ -0,0 +1,397 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5013 — repo-wide alias-table integrity for every `strictObject(` authoring + * surface in `packages/spec`. + * + * ## What an alias table actually is + * + * It is a **claim about the schema**, in two halves: + * + * - the key it is filed under is one the shape **rejects** (an alias only ever + * runs from the `unrecognized_keys` path, so a key the shape *declares* can + * never reach it — that entry is dead code that reads as coverage); + * - the key it prescribes is one the shape **accepts** (ledger finding 12, + * *never suggest a key the schema cannot accept*). + * + * Nothing checked either half, and both were false on `main`. `ReportSchema` + * answered `filter` with *"Did you mean `filter` → `filters`?"* and then + * rejected `filters` too — a second rejection, with no suggestion the second + * time, from the campaign built to remove exactly that experience. The other + * five were dead entries whose keys the shape already declared. + * + * ## Why the judgement runs at RUNTIME + * + * 批 14 shipped this assertion over its own six files by reading the source + * object literals with the TypeScript AST, and hand-mapped each `surface` + * string onto a schema. Three things break that at repo scale, all of them + * present today and all measured, not hypothesised: + * + * 1. **Spreads.** A shape that spreads (`...MetadataProtectionFields`) has keys + * no source-literal reader can see, so the target check has to be suppressed + * for most of the interesting schemas. `.shape` sees them. + * 2. **Assembled tables.** Ten call sites build `aliases` (or `surface`) from + * something other than a literal — `data/field.zod.ts`, `ui/theme.zod.ts`, + * `automation/etl.zod.ts` and others. The AST reads those as empty and + * reports them clean. + * 3. **Colliding surfaces.** `'this field group'` names two different schemas + * (`data/object.zod.ts`, `studio/object-designer.zod.ts`), so the surface + * string is not a key and the hand-map silently judges one against the + * other's shape. + * + * So the table and the shape are both read off the **same runtime node**: + * `strictObject` retains its options under a marker symbol, and this file walks + * the schema graph of every module in `packages/spec/src` to find them. There + * is no second copy of anything, and no per-surface registration to forget. + * + * The AST is still used — for **coverage only**. It enumerates the call sites + * that exist in the source, and the walk must have reached every one of them. + * That is the half that makes absence loud: a table the walk cannot reach is a + * table this gate is not judging, and it fails rather than passing quietly. + * + * ## What it deliberately does not judge + * + * - Tables reaching `strictUnknownKeyError` directly, which carry a transcribed + * `knownKeys` array instead of a shape — measured clean, pinned shrink-only + * below, extension tracked as #5483. + * - Two alias keys in one table colliding under the suggester's own + * `aliasProbe` normalisation, where the later entry silently wins (#5481). + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +import { describe, it, expect, beforeAll } from 'vitest'; +import ts from 'typescript'; + +import { acceptsNothing, strictObjectDeclarations, type StrictObjectDeclaration } from './strict-object'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +const SPEC_SRC = path.resolve(HERE, '..'); + +// --------------------------------------------------------------------------- +// The file set — one list, shared by the runtime walk and the AST coverage scan +// --------------------------------------------------------------------------- + +/** Every non-test TypeScript module under `packages/spec/src`, repo-relative. */ +function specModules(dir = SPEC_SRC, out: string[] = []): string[] { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) specModules(full, out); + else if ( + entry.name.endsWith('.ts') + && !entry.name.endsWith('.test.ts') + && !entry.name.endsWith('.bench.ts') // registers vitest suites on import + && !entry.name.endsWith('.d.ts') + ) { + out.push(full); + } + } + return out; +} + +const MODULES = specModules().sort(); + +// --------------------------------------------------------------------------- +// Forcing walk — build every schema, so the registry is complete +// --------------------------------------------------------------------------- + +const isSchema = (v: unknown): boolean => + v != null + && (typeof v === 'object' || typeof v === 'function') + && typeof (v as { _zod?: { def?: unknown } })._zod?.def === 'object'; + +/** + * Touch every node in the zod graph under `root`. + * + * `strictObject` records a declaration when it RUNS, and `lazySchema` defers + * that until first use — so a schema nobody touches never registers. This walk + * is what makes "nobody touched it" impossible: reading `_zod.def` resolves the + * lazy proxy, and descending the graph reaches nested shapes that only build + * when their parent does. + * + * Deliberately generic over `_zod.def` rather than switch-per-zod-type: a node + * kind this file does not know about (a future wrapper, a pipe, a discriminated + * union arm) must not silently drop the subtree beneath it. The cost is + * visiting some internals twice, which `seen` absorbs. + */ +function force(root: unknown, seen: Set): void { + const visit = (node: unknown, depth: number): void => { + if (depth > 40 || node == null) return; + if (typeof node !== 'object' && typeof node !== 'function') return; + if (seen.has(node)) return; + seen.add(node); + if (!isSchema(node)) return; + + const def = (node as { _zod: { def: Record } })._zod.def; + // Reading `.shape` is what forces an object schema's own lazy members. + if ((def as { type?: string }).type === 'object') void (node as { shape?: unknown }).shape; + + for (const value of Object.values(def)) { + if (typeof value === 'function') { + // `z.lazy()` stores its body as `getter`; calling it is the only way + // into a recursive schema's interior. + if ((def as { type?: string }).type === 'lazy') { + try { visit((value as () => unknown)(), depth + 1); } catch { /* not a getter */ } + } + continue; + } + if (isSchema(value)) { visit(value, depth + 1); continue; } + if (Array.isArray(value)) { + for (const item of value) if (isSchema(item)) visit(item, depth + 1); + continue; + } + if (value && typeof value === 'object') { + // `def.shape` (object), `def.entries` / `def.propValues` (unions), … + for (const inner of Object.values(value as Record)) { + if (isSchema(inner)) visit(inner, depth + 1); + } + } + } + }; + visit(root, 0); +} + +/** Every declaration built by the forcing walk, de-duplicated by content. */ +let SURFACES: StrictObjectDeclaration[] = []; + +beforeAll(async () => { + const seen = new Set(); + for (const file of MODULES) { + let mod: Record; + try { + mod = (await import(pathToFileURL(file).href)) as Record; + } catch (error) { + throw new Error(`could not import ${path.relative(SPEC_SRC, file)}: ${String(error)}`); + } + for (const value of Object.values(mod)) force(value, seen); + } + // A factory (`actionObject()`) called by two schemas runs its `strictObject` + // twice, registering two declarations from ONE call site. Same table, same + // shape, same verdict — collapse them so a failure is reported once. + const unique = new Map(); + for (const d of strictObjectDeclarations()) { + unique.set( + JSON.stringify([d.options.surface, d.options.aliases ?? {}, Object.keys(d.shape).sort()]), + d, + ); + } + SURFACES = [...unique.values()]; +}, 180_000); + +// --------------------------------------------------------------------------- +// AST coverage scan — which call sites EXIST (never what they mean) +// --------------------------------------------------------------------------- + +interface CallSite { + file: string; + line: number; + surface: string | null; + /** Alias entries readable as literals. Partial when the table is assembled. */ + aliases: Record; + hasAliases: boolean; + /** True when `surface` or any alias entry is not a plain literal. */ + assembled: boolean; +} + +function callSites(file: string): CallSite[] { + const source = ts.createSourceFile(file, fs.readFileSync(file, 'utf8'), ts.ScriptTarget.Latest, true); + const literal = (n: ts.Node): string | null => + ts.isStringLiteral(n) || ts.isNoSubstitutionTemplateLiteral(n) ? n.text : null; + const prop = (o: ts.ObjectLiteralExpression, name: string): ts.Expression | null => { + for (const p of o.properties) { + if (ts.isPropertyAssignment(p) && ts.isIdentifier(p.name) && p.name.text === name) return p.initializer; + } + return null; + }; + const out: CallSite[] = []; + const visit = (node: ts.Node): void => { + if ( + ts.isCallExpression(node) + && ts.isIdentifier(node.expression) + && node.expression.text === 'strictObject' + && node.arguments.length === 2 + && ts.isObjectLiteralExpression(node.arguments[0]) + ) { + const opts = node.arguments[0]; + const surfaceNode = prop(opts, 'surface'); + const surface = surfaceNode ? literal(surfaceNode) : null; + const aliasesNode = prop(opts, 'aliases'); + const aliases: Record = {}; + let assembled = surfaceNode != null && surface == null; + if (aliasesNode) { + if (ts.isObjectLiteralExpression(aliasesNode)) { + for (const p of aliasesNode.properties) { + if (!ts.isPropertyAssignment(p)) { assembled = true; continue; } + const key = ts.isIdentifier(p.name) || ts.isStringLiteral(p.name) ? p.name.text : null; + const target = literal(p.initializer); + if (key && target) aliases[key] = target; else assembled = true; + } + } else assembled = true; + } + out.push({ + file: path.relative(SPEC_SRC, file), + line: source.getLineAndCharacterOfPosition(node.getStart()).line + 1, + surface, + aliases, + hasAliases: aliasesNode != null, + assembled, + }); + } + ts.forEachChild(node, visit); + }; + visit(source); + return out; +} + +const CALL_SITES = MODULES.flatMap(callSites); + +// --------------------------------------------------------------------------- +// 1. Coverage — the walk reached every table the source declares +// --------------------------------------------------------------------------- + +describe('alias integrity — coverage', () => { + it('the source really contains alias tables to judge (self-test before the verdict)', () => { + // A verdict over an empty set is the failure mode this whole file exists to + // prevent, so the instrument states its own scale first. + const withAliases = CALL_SITES.filter((c) => c.hasAliases); + expect(CALL_SITES.length).toBeGreaterThan(200); + expect(withAliases.length).toBeGreaterThan(140); + expect(CALL_SITES.some((c) => c.assembled)).toBe(true); + }); + + it('every `strictObject(` call site with an alias table was reached at runtime', () => { + const bySurface = new Map(); + for (const s of SURFACES) { + const list = bySurface.get(s.options.surface) ?? []; + list.push(s); + bySurface.set(s.options.surface, list); + } + const unreached: string[] = []; + for (const site of CALL_SITES) { + if (!site.hasAliases) continue; + // Matched on surface AND on the literal entries the AST could read, so a + // colliding surface string (`'this field group'` names two schemas) still + // resolves to the right declaration. A site whose `surface` is itself + // assembled — `actionTranslationSchema(surface)` is called twice with two + // different strings — is matched on its entries alone. + const candidates = site.surface ? (bySurface.get(site.surface) ?? []) : SURFACES; + const matched = candidates.some((c) => + Object.entries(site.aliases).every(([k, v]) => c.options.aliases?.[k] === v)); + if (!matched) unreached.push(`${site.file}:${site.line} (${site.surface ?? 'assembled surface'})`); + } + expect(unreached, 'these alias tables are not reachable from any module export, so nothing judges them').toEqual([]); + }); + + it('the surface this gate does NOT cover only ever shrinks', () => { + // `strictObject` is not the only way to get an alias table: the pre-helper + // wiring calls `strictUnknownKeyError` directly with a hand-transcribed + // `knownKeys` array, and those tables never reach the registry this gate + // reads. Measured at 44 call sites when the gate was written, and measured + // CLEAN on both criteria at the same time — so this is a coverage boundary, + // not hidden debt. It is pinned shrink-only rather than left implicit + // because an uncovered table that nobody can see growing is precisely the + // "green check over source nothing read" failure the campaign keeps paying + // for: migrating one to `strictObject` is free, adding a NEW one fails here + // and forces the choice to be deliberate. Extending the judgement over them + // is #5483 — they carry a transcribed key list rather than a shape, so it + // is a different measurement, not more of this one. + const uncovered = MODULES.filter((f) => { + const rel = path.relative(SPEC_SRC, f); + return rel !== 'shared/suggestions.zod.ts' && rel !== 'shared/strict-object.ts'; + }).flatMap((f) => { + const source = fs.readFileSync(f, 'utf8'); + return [...source.matchAll(/\bstrictUnknownKeyError\s*\(/g)].map(() => path.relative(SPEC_SRC, f)); + }); + expect(uncovered.length).toBeLessThanOrEqual(44); + }); + + it('the runtime walk sees tables the AST provably cannot read', () => { + // The reason the judgement is not done by AST, asserted rather than argued. + const assembled = CALL_SITES.filter((c) => c.hasAliases && c.assembled && Object.keys(c.aliases).length === 0); + expect(assembled.length).toBeGreaterThan(0); + for (const site of assembled) { + const runtime = SURFACES.filter((s) => s.options.surface === site.surface); + expect(runtime.length, `no runtime table for ${site.file}:${site.line}`).toBeGreaterThan(0); + expect( + runtime.some((s) => Object.keys(s.options.aliases ?? {}).length > 0), + `${site.file}:${site.line} reads as an EMPTY table in source but is non-empty at runtime`, + ).toBe(true); + } + }); +}); + +// --------------------------------------------------------------------------- +// 2. The verdict +// --------------------------------------------------------------------------- + +/** + * `file:line — "surface": \`written\` -> \`target\``. + * + * The location is joined back from the AST scan purely so a failure is + * navigable; it plays no part in the verdict, which is decided entirely by the + * runtime shape. + */ +const entry = (s: StrictObjectDeclaration, written: string, target: string): string => { + const site = CALL_SITES.find( + (c) => c.surface === s.options.surface + && Object.entries(c.aliases).every(([k, v]) => s.options.aliases?.[k] === v), + ); + const where = site ? `${site.file}:${site.line}` : '(location unresolved)'; + return `${where} — "${s.options.surface}": \`${written}\` -> \`${target}\``; +}; + +describe('alias integrity — every table is a true claim about its schema', () => { + it('no alias key is itself a declared key (a dead entry that can never fire)', () => { + // An alias is consulted only from the `unrecognized_keys` path. A key the + // shape declares is recognised, so the entry is unreachable — and it reads + // as coverage for a spelling nobody is actually helped with. + const dead: string[] = []; + for (const s of SURFACES) { + const declared = new Set(Object.keys(s.shape)); + for (const [written, target] of Object.entries(s.options.aliases ?? {})) { + if (declared.has(written)) dead.push(`${entry(s, written, target)} — \`${written}\` is declared here`); + } + } + expect(dead.sort()).toEqual([]); + }); + + it('every alias target is a key the schema really accepts', () => { + // Two ways to fail, and the second is invisible to the helper's own guard: + // `knownKeys` filters tombstones out of the edit-distance candidates, but + // the alias table is consulted BEFORE that fallback and bypasses the filter + // entirely. Pointing an alias at a tombstone is ledger finding 12 exactly — + // the author is told to write the one key guaranteed to be rejected next. + const broken: string[] = []; + for (const s of SURFACES) { + const shape = s.shape; + for (const [written, target] of Object.entries(s.options.aliases ?? {})) { + if (!(target in shape)) { + broken.push(`${entry(s, written, target)} — \`${target}\` is not declared here`); + } else if (acceptsNothing(shape[target])) { + broken.push(`${entry(s, written, target)} — \`${target}\` is a tombstone; it accepts nothing`); + } + } + } + expect(broken.sort()).toEqual([]); + }); + + it('no guidance key is itself a declared key (the same dead entry, other channel)', () => { + // `guidance` is consulted from the same `unrecognized_keys` path, so a + // prescription filed under a key the shape DECLARES is unreachable in + // exactly the way a dead alias is. Measured clean when this gate was + // written — asserted so it stays that way, since a guidance entry is the + // instrument a retirement leans on and a silent one loses the upgrade text. + const dead: string[] = []; + for (const s of SURFACES) { + const declared = new Set(Object.keys(s.shape)); + for (const written of Object.keys(s.options.guidance ?? {})) { + if (declared.has(written)) { + dead.push(`"${s.options.surface}": guidance for \`${written}\`, which is declared here`); + } + } + } + expect(dead.sort()).toEqual([]); + }); +}); diff --git a/packages/spec/src/shared/strict-object.ts b/packages/spec/src/shared/strict-object.ts index 400eab94df..7f78f651bb 100644 --- a/packages/spec/src/shared/strict-object.ts +++ b/packages/spec/src/shared/strict-object.ts @@ -88,7 +88,7 @@ import { strictUnknownKeyError } from './suggestions.zod'; * structural check gets that for free, and keeps working if the tombstone * helper is ever reshaped. */ -function acceptsNothing(schema: unknown, depth = 0): boolean { +export function acceptsNothing(schema: unknown, depth = 0): boolean { if (depth > 6) return false; const def = (schema as { _zod?: { def?: { type?: string; innerType?: unknown } } })._zod?.def; if (!def?.type) return false; @@ -148,6 +148,48 @@ export interface StrictObjectOptions { retiredForms?: Readonly>; } +/** + * One built authoring shape, paired with the options it was declared with, so + * the table can be judged against the schema it makes claims about (#5013). + */ +export interface StrictObjectDeclaration { + /** The authoring metadata the shape was declared with. */ + readonly options: StrictObjectOptions; + /** + * The shape the options make claims about — the very object the error map + * reads `knownKeys` from, so the audit judges exactly what the suggester + * judges rather than a second view of it. + */ + readonly shape: z.ZodRawShape; +} + +const DECLARATIONS: StrictObjectDeclaration[] = []; + +/** + * Every authoring shape {@link strictObject} has built **so far in this + * process** — the audit handle behind `alias-integrity.test.ts` (#5013). + * + * An `aliases` / `guidance` table is a **claim about the schema**, in two + * halves: that the key it is filed under is one the shape *rejects* (an alias + * runs only from the `unrecognized_keys` path, so a declared key can never + * reach it), and that the key it prescribes is one the shape *accepts*. Nothing + * checked either half until #5013, and both were false on `main` — + * `ReportSchema` answered `filter` with *"Did you mean `filter` → `filters`?"* + * and then rejected `filters` too, with no suggestion the second time. + * + * Recorded at construction rather than read back off the built schema, because + * a marker on the instance does not survive the clone `.superRefine()` / + * `.extend()` make — which silently un-audited most of the interesting schemas, + * `ReportSchema` and `DatasetSchema` among them, when this was first written + * that way. "So far in this process" is why the audit walks the schema graph to + * force every `lazySchema` before reading this, and cross-checks the result + * against an AST scan of the call sites: a table nothing constructs is a table + * nothing judges, and that must fail loudly rather than pass quietly. + */ +export function strictObjectDeclarations(): readonly StrictObjectDeclaration[] { + return DECLARATIONS; +} + /** * A `.strict()` object whose unknown-key error names the surface, echoes the * offending key, and suggests the closest declared key — with the candidate @@ -202,5 +244,7 @@ export function strictObject(options: StrictObjectOptio }))(issue); }; + DECLARATIONS.push({ options, shape }); + return z.object(shape, { error }).strict(); } diff --git a/packages/spec/src/system/email-template.zod.ts b/packages/spec/src/system/email-template.zod.ts index 5ddd49e384..293aa9b36a 100644 --- a/packages/spec/src/system/email-template.zod.ts +++ b/packages/spec/src/system/email-template.zod.ts @@ -46,7 +46,20 @@ export const EmailTemplateDefinitionSchema = lazySchema(() => strictObject({ surface: 'this email template', history: 'Until #4001 closed this shape these were dropped silently — the item still registered, minus whatever the key was meant to configure.', - aliases: { title: 'subject', content: 'body', html: 'body', text: 'body', from: 'fromAddress', sender: 'fromAddress' }, + // #5013 — five of these pointed at `body` and `fromAddress`, neither of which + // this schema declares: every one of them prescribed a second rejection. The + // real slots are the two bodies and the per-template From override. + // + // `content` goes to `bodyHtml` rather than `bodyText` deliberately: `bodyHtml` + // is the REQUIRED body and accepts any string, and the service derives the + // plain-text alternative from it when `bodyText` is omitted — so the rename + // produces a template that actually renders whether the author's content was + // markup or prose, which is the property a prescription has to have. + aliases: { + title: 'subject', + content: 'bodyHtml', html: 'bodyHtml', text: 'bodyText', + from: 'fromOverride', sender: 'fromOverride', + }, }, { /** * Stable identifier; used as the `template` key in diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 6b01bc6acd..257c6564ea 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -566,7 +566,10 @@ const actionObject = () => strictObject({ style: 'variant', color: 'variant', appearance: 'variant', placement: 'locations', location: 'locations', position: 'locations', verb: 'method', httpMethod: 'method', - body: 'bodyExtra', payload: 'bodyExtra', + // #5013 — `body` is DECLARED on this schema (the `script` action's L1/L2 + // hook body), so an alias filed under it could never run; `payload` is the + // live spelling that still needs pointing at `bodyExtra`. + payload: 'bodyExtra', llm: 'ai', tool: 'ai', dialog: 'resultDialog', result: 'resultDialog', refresh: 'refreshAfter', reload: 'refreshAfter', diff --git a/packages/spec/src/ui/dataset.zod.ts b/packages/spec/src/ui/dataset.zod.ts index b3c89313ce..fc5c10b7d9 100644 --- a/packages/spec/src/ui/dataset.zod.ts +++ b/packages/spec/src/ui/dataset.zod.ts @@ -249,7 +249,13 @@ export const DatasetSchema = lazySchema(() => strictObject({ surface: 'this dataset', history: 'Until #4001 closed this shape these were dropped silently — the item still registered, minus whatever the key was meant to configure.', - aliases: { source: 'object', objectName: 'object', measures: 'metrics', dimension: 'dimensions', filter: 'filters' }, + // #5013 — `measures` and `filter` are both DECLARED here (the aggregatable + // values and the intrinsic scope filter), so neither entry could ever run: an + // alias is consulted only from the `unrecognized_keys` path, and a declared + // key is recognised. Their targets (`metrics`, `filters`) were not keys of + // this schema either, so had they been reachable they would have prescribed a + // second rejection. + aliases: { source: 'object', objectName: 'object', dimension: 'dimensions' }, }, { /** Identity. */ name: SnakeCaseIdentifierSchema.describe('Dataset unique name'), diff --git a/packages/spec/src/ui/report.test.ts b/packages/spec/src/ui/report.test.ts index 99d8181433..90c5592d8b 100644 --- a/packages/spec/src/ui/report.test.ts +++ b/packages/spec/src/ui/report.test.ts @@ -181,6 +181,73 @@ describe('Report ordering (#3916)', () => { }); }); +/** + * #5013 — the scope-filter prescription, pinned by PARSE rather than by reading + * the alias table. + * + * The structural half (every alias key is one the shape rejects, every target + * one it accepts) is gated package-wide in `shared/alias-integrity.test.ts`. + * What that gate cannot say is what the author actually SEES, which is the + * thing that was broken: `filter` pointed at `filters`, a key `ReportSchema` + * does not declare either, so the fix earned a second rejection carrying no + * suggestion at all. + * + * These assertions guard a KEY verdict — which spelling the rejection names — + * so the prescribed key is proved by a full green parse of an otherwise-valid + * report, not merely by the absence of an `unrecognized_keys` issue. + */ +describe('ReportSchema — scope-filter aliases point at `runtimeFilter` (#5013)', () => { + const VALID = { + name: 'pipeline', label: 'Pipeline', type: 'summary', + dataset: 'sales', rows: ['stage'], values: ['revenue'], + } as const; + + const messageFor = (key: string): string => { + const r = ReportSchema.safeParse({ ...VALID, [key]: { won: true } }); + expect(r.success, `\`${key}\` must still be rejected — it is not a declared key`).toBe(false); + return r.error!.issues.map((i) => i.message).join('\n'); + }; + + it.each(['filter', 'filters', 'where', 'criteria'])( + '`%s` is renamed onto `runtimeFilter`, the key this schema really declares', + (key) => { + const message = messageFor(key); + expect(message).toContain(`\`${key}\` → \`runtimeFilter\``); + // The old prescription, and the reason it was a defect: `filters` is not + // a key of this schema, so being sent there is a second rejection. + expect(message).not.toContain('→ `filters`'); + }, + ); + + it('the prescribed key parses — taking the advice ends the conversation', () => { + // The half that was false before: following the suggestion has to WORK. + const r = ReportSchema.safeParse({ ...VALID, runtimeFilter: { won: true } }); + expect(r.success).toBe(true); + expect(r.data!.runtimeFilter).toEqual({ won: true }); + }); + + it('matches the block table verbatim — a sub-report corrects the author the same way', () => { + const block = JoinedReportBlockSchema.safeParse({ + name: 'b', label: 'B', type: 'summary', dataset: 'sales', rows: ['stage'], values: ['revenue'], + filter: { won: true }, + }); + expect(block.success).toBe(false); + expect(block.error!.issues.map((i) => i.message).join('\n')).toContain('`filter` → `runtimeFilter`'); + }); + + it('`columns` and `chart` are real keys here, so nothing renames them away', () => { + // Both were alias KEYS on this schema until #5013 — entries that could + // never run, because the shape declares both. Pinned from the other side: + // authoring either must simply work. + const r = ReportSchema.safeParse({ + ...VALID, type: 'matrix', columns: ['region'], + chart: { type: 'bar', xAxis: 'stage', yAxis: 'revenue' }, + }); + expect(r.success).toBe(true); + expect(r.data!.columns).toEqual(['region']); + }); +}); + describe('ReportChartSchema', () => { it('requires xAxis + yAxis', () => { expect(ReportChartSchema.parse({ type: 'bar', xAxis: 'stage', yAxis: 'revenue' }).type).toBe('bar'); diff --git a/packages/spec/src/ui/report.zod.ts b/packages/spec/src/ui/report.zod.ts index a959090922..bf48628771 100644 --- a/packages/spec/src/ui/report.zod.ts +++ b/packages/spec/src/ui/report.zod.ts @@ -249,7 +249,29 @@ export const ReportSchema = lazySchema(() => strictObject({ surface: 'this report', history: 'Until #4001 closed this shape these were dropped silently — the item still registered, minus whatever the key was meant to configure.', - aliases: { dataSet: 'dataset', source: 'dataset', fields: 'values', columns: 'values', chart: 'chartConfig', filter: 'filters' }, + // Kept deliberately parallel to `JoinedReportBlockSchema` above: a block is a + // sub-report, so an author who learns one vocabulary must not be corrected + // differently on the other. The scope-filter entries are that table's, + // verbatim. + // + // #5013 — `filter` used to point at `filters`, a key `ReportSchema` does not + // declare either, so taking the advice earned a SECOND rejection and that one + // carried no suggestion at all. Three entries are gone with it: `columns` and + // `chart` are both declared here (the matrix across-axis and the embedded + // chart), so an alias filed under them could never run — an alias is consulted + // only from the `unrecognized_keys` path — and `chartConfig` was not a key + // either. `alias-integrity.test.ts` now proves both halves for every table in + // the package. + aliases: { + dataSet: 'dataset', source: 'dataset', + fields: 'values', + // Scope filter. `runtimeFilter` is camelCase, so the edit-distance fallback + // under-reaches every one of these (#4990) — same as on a block. + filter: 'runtimeFilter', + filters: 'runtimeFilter', + where: 'runtimeFilter', + criteria: 'runtimeFilter', + }, guidance: { // #5022 — the reverse half of a two-way disambiguation. The forward half // lives on `ChartDrillDownSchema` in `ui/chart.zod.ts`, which tells an diff --git a/packages/spec/src/ui/strictness-batch14.test.ts b/packages/spec/src/ui/strictness-batch14.test.ts index bb966c9264..902c4c9946 100644 --- a/packages/spec/src/ui/strictness-batch14.test.ts +++ b/packages/spec/src/ui/strictness-batch14.test.ts @@ -19,10 +19,13 @@ * verdict as ADR-0049 REMOVE and both shapes are gone, so the absence pins * live in `notification-embed-retirement.test.ts` where they can actually * fail. See the block's own header for why they are not restated here. - * 3. **Prescription integrity** — every alias target this batch added is a key - * the schema really accepts (ledger finding 12: *never suggest a key the - * schema cannot accept*), checked by parsing the prescribed key, not by - * reading the table. + * 3. **Prescription integrity** — was here; now package-wide in + * `shared/alias-integrity.test.ts` (#5013), which judges all 235 + * `strictObject` surfaces against their runtime `.shape` instead of the nine + * this batch could hand-map. The six pre-existing defects this file used to + * carry as a reverse-pinned debt list are fixed, so the pin fired and was + * retired with them. What remains here is the parse-level assertion a + * structural gate cannot make. */ import fs from 'node:fs'; @@ -30,17 +33,12 @@ import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, it, expect } from 'vitest'; -import ts from 'typescript'; import { z } from 'zod'; -import { ActionParamSchema, ActionSchema as ActionSchemaForAudit } from './action.zod'; +import { ActionParamSchema } from './action.zod'; import { SharingConfigSchema } from './sharing.zod'; -import { ReportSortSchema, JoinedReportBlockSchema, ReportSchema as ReportSchemaForAudit } from './report.zod'; -import { - DatasetDimensionSchema, - DatasetMeasureSchema, - DatasetSchema as DatasetSchemaForAudit, -} from './dataset.zod'; +import { ReportSortSchema, JoinedReportBlockSchema } from './report.zod'; +import { DatasetDimensionSchema, DatasetMeasureSchema } from './dataset.zod'; import { DashboardWidgetSchema } from './dashboard.zod'; const HERE = path.dirname(fileURLToPath(import.meta.url)); @@ -246,193 +244,34 @@ describe('批 14 — curated prescriptions', () => { }); // --------------------------------------------------------------------------- -// 3. Prescription integrity — every alias target must actually be accepted +// 3. Prescription integrity — SUPERSEDED by the package-wide gate (#5013) // --------------------------------------------------------------------------- /** - * The `aliases` table of every `strictObject(` call in one of this batch's six - * files, read from the SOURCE by AST, keyed by the call's `surface` string. + * This section used to carry two assertions and a debt list: that every alias + * target 批 14 added is a key the schema declares, and a reverse pin naming the + * six pre-existing defects in these files that the batch did not own + * (`ReportSchema`'s `filter`/`columns`/`chart`, `DatasetSchema`'s + * `measures`/`filter`, `ActionSchema`'s `body`). * - * Reading the source is what makes this a real check. The first version of this - * suite hand-listed the prescribed keys beside the assertion — a second copy of - * the truth — and it stayed GREEN under a deliberate sabotage that repointed a - * live alias at a key the schema rejects. That is the failure the ledger keeps - * recording in different instruments (finding 9, finding 19): a measurement - * reporting completeness it does not have. `strictObject` exists precisely to - * collapse two copies into one; a test over it must not reintroduce them. + * Both are gone because both were kept honest: #5013 fixed all six, so the + * reverse pin fired exactly as its comment promised — *"this list cannot + * outlive its debt"* — and the verdict itself now runs package-wide in + * `shared/alias-integrity.test.ts`, over all 235 `strictObject` surfaces rather + * than the nine this batch hand-mapped. + * + * It is deleted rather than emptied, because what would be left is a second, + * WEAKER copy of a live check: the version here read alias tables from the + * source with the TypeScript AST and bound each `surface` string to a schema by + * hand, which cannot see through a shape's spreads, reads the ten + * dynamically-assembled tables as empty, and mis-binds `'this field group'` + * (two schemas share that string). Keeping it would reintroduce exactly the + * two-copies-of-the-truth problem `strictObject` exists to collapse. + * + * The one assertion NOT subsumed stays: a structural gate proves a prescribed + * key is declared, never that the shape it prescribes actually parses. */ -function aliasTablesBySurface(file: string): Map> { - const source = ts.createSourceFile( - file, fs.readFileSync(file, 'utf8'), ts.ScriptTarget.Latest, true, - ); - const out = new Map>(); - const literal = (n: ts.Node): string | null => - ts.isStringLiteral(n) || ts.isNoSubstitutionTemplateLiteral(n) ? n.text : null; - const prop = (o: ts.ObjectLiteralExpression, name: string): ts.Expression | null => { - for (const p of o.properties) { - if (ts.isPropertyAssignment(p) && ts.isIdentifier(p.name) && p.name.text === name) return p.initializer; - } - return null; - }; - const visit = (node: ts.Node): void => { - if ( - ts.isCallExpression(node) - && ts.isIdentifier(node.expression) - && node.expression.text === 'strictObject' - && node.arguments.length === 2 - && ts.isObjectLiteralExpression(node.arguments[0]) - ) { - const opts = node.arguments[0]; - const surfaceNode = prop(opts, 'surface'); - const surface = surfaceNode ? literal(surfaceNode) : null; - if (surface) { - const table: Record = {}; - const aliases = prop(opts, 'aliases'); - if (aliases && ts.isObjectLiteralExpression(aliases)) { - for (const p of aliases.properties) { - if (!ts.isPropertyAssignment(p)) continue; - const key = ts.isIdentifier(p.name) || ts.isStringLiteral(p.name) ? p.name.text : null; - const target = literal(p.initializer); - if (key && target) table[key] = target; - } - } - out.set(surface, table); - } - } - ts.forEachChild(node, visit); - }; - visit(source); - return out; -} - -/** Unwrap lazy / optional / default / array / effects down to a plain object's `.shape`. */ -function shapeOf(schema: unknown, depth = 0): Record | null { - if (depth > 12 || schema == null) return null; - const direct = (schema as { shape?: Record }).shape; - if (direct && typeof direct === 'object') return direct; - const def = (schema as { _zod?: { def?: Record } })._zod?.def; - if (!def) return null; - for (const key of ['innerType', 'element', 'in', 'out', 'schema', 'type'] as const) { - const inner = def[key]; - if (inner && typeof inner === 'object') { - const found = shapeOf(inner, depth + 1); - if (found) return found; - } - } - if (typeof (def as { getter?: unknown }).getter === 'function') { - return shapeOf((def as { getter: () => unknown }).getter(), depth + 1); - } - return null; -} - -describe('批 14 — no prescription points at a key the schema rejects', () => { - /** - * `surface` string → the DECLARED keys of the shape that surface names, - * resolved at RUNTIME rather than from the source object literal. - * - * Runtime, specifically, because a shape can spread (`...MetadataProtectionFields`) - * and a source-literal reader cannot see through that — it has to suppress the - * check for every spreading schema, which is most of the interesting ones. - * `.shape` sees the spread keys. - */ - const declaredKeys = (): Map> => { - const widget = shapeOf(DashboardWidgetSchema)!; - const measure = shapeOf(DatasetMeasureSchema)!; - const entries: Array<[string, Record | null]> = [ - ['this action param option', shapeOf(shapeOf(ActionParamSchema)!.options)], - ['this sharing config', shapeOf(SharingConfigSchema)], - ['this report order key', shapeOf(ReportSortSchema)], - ['this joined report block', shapeOf(JoinedReportBlockSchema)], - ['this dataset dimension', shapeOf(DatasetDimensionSchema)], - ['this dataset measure', measure], - ['this derived-measure spec', shapeOf(measure.derived)], - // #5011: `compareTo` converged from a union to a plain strict object, - // so the arm-unwrapper this entry used is gone with it. - ['this comparison window', shapeOf(widget.compareTo)], - ['this widget layout box', shapeOf(widget.layout)], - ]; - return new Map(entries.map(([surface, shape]) => { - expect(shape, `could not resolve the shape behind "${surface}"`).toBeTruthy(); - return [surface, new Set(Object.keys(shape!))]; - })); - }; - - /** - * Pre-existing defects in tables this batch did NOT write, each already filed. - * Listed rather than skipped so the instrument stays complete over these six - * files: the day one is fixed, its entry here fails and gets deleted. - */ - const KNOWN_DEFECTS: ReadonlyArray = [ - // [surface, alias key, issue] - ['this report', 'columns', '#5013'], - ['this report', 'chart', '#5013'], - ['this report', 'filter', '#5013'], - ['this dataset', 'measures', '#5013'], - ['this dataset', 'filter', '#5013'], - ['this action', 'body', '#5013'], - ]; - - const BATCH14_SURFACES = [ - 'this action param option', 'this sharing config', 'this report order key', - 'this joined report block', 'this dataset dimension', 'this dataset measure', - 'this derived-measure spec', 'this comparison window', 'this widget layout box', - ] as const; - - const FILES = [ - 'ui/action.zod.ts', 'ui/sharing.zod.ts', 'ui/report.zod.ts', - 'ui/dataset.zod.ts', 'ui/dashboard.zod.ts', - ]; - - it('the AST reader really finds this batch\'s tables (self-test before the verdict)', () => { - const found = new Set(); - for (const f of FILES) for (const s of aliasTablesBySurface(path.join(SPEC_SRC, f)).keys()) found.add(s); - for (const surface of BATCH14_SURFACES) { - expect(found, `AST reader lost the table for "${surface}"`).toContain(surface); - } - // And it reads real content, not empty tables. - const sharing = aliasTablesBySurface(path.join(SPEC_SRC, 'ui/sharing.zod.ts')); - expect(sharing.get('this sharing config')!.anonymous).toBe('allowAnonymous'); - }); - - it('every alias TARGET this batch added is a key the schema really declares', () => { - const declared = declaredKeys(); - const failures: string[] = []; - for (const f of FILES) { - for (const [surface, table] of aliasTablesBySurface(path.join(SPEC_SRC, f))) { - const keys = declared.get(surface); - if (!keys) continue; // not a 批 14 surface — covered by the next test - for (const [written, target] of Object.entries(table)) { - if (!keys.has(target)) failures.push(`${surface}: \`${written}\` -> \`${target}\` (not declared)`); - if (keys.has(written)) failures.push(`${surface}: \`${written}\` is itself declared (dead entry)`); - } - } - } - expect(failures).toEqual([]); - }); - - it('the pre-existing defects in these files are exactly the ones already filed', () => { - // A reverse pin, the ADR-0010 debt-list idiom: this list cannot outlive its - // debt. Fix one upstream and this test names it and fails. - const seen: string[] = []; - for (const f of FILES) { - for (const [surface, table] of aliasTablesBySurface(path.join(SPEC_SRC, f))) { - if ((BATCH14_SURFACES as readonly string[]).includes(surface)) continue; - const shape = surface === 'this report' ? shapeOf(ReportSchemaForAudit) - : surface === 'this dataset' ? shapeOf(DatasetSchemaForAudit) - : surface === 'this action' ? shapeOf(ActionSchemaForAudit) - : null; - if (!shape) continue; - const keys = new Set(Object.keys(shape)); - for (const [written, target] of Object.entries(table)) { - if (keys.has(written) || !keys.has(target)) seen.push(`${surface}|${written}`); - } - } - } - expect(seen.sort()).toEqual( - KNOWN_DEFECTS.map(([s, k]) => `${s}|${k}`).sort(), - ); - }); - +describe('批 14 — the prescribed action param option shape really parses', () => { it('the action param option shape accepts exactly the pair it prescribes', () => { const r = ActionParamSchema.safeParse({ name: 'p', options: [{ label: 'A', value: 'a' }] }); expect(r.success).toBe(true);