From 0c2ec448d9a72dc9015d77507f0c116ebaf62af6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:19:27 +0000 Subject: [PATCH 1/6] ci(spec): a spec-named symbol must be derived, not hand-written (objectstack#4115) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui declares local types and consts under the same names as `@objectstack/spec` exports, with doc comments claiming spec canonicity. Every audited instance had either drifted or was one spec release away from it: `ActionType` (hand union, missing `form`), `ChartType` (7 of 19 members), `ActionLocation` + 2 (re-declared union + tuple + `z.enum` under a comment that said "re-export here"), `ActionParamFieldType` (16 of 49 members). The cost is specific to agent-driven development: an agent reads the comment, takes it as ground truth, and builds on it — #2901 was filed with a backwards premise because the fork carried the spec's own symbol name. A wrong canonical-claim is not stale documentation, it is a planted premise. Lands as the check rather than an AGENTS.md paragraph, which is the same "declared != enforced" landmine the thread is about. - Enumerates all 4231 `@objectstack/spec` export names (types AND values, via the compiler's view of each subpath's `.d.ts` — a runtime `import()` sees only values, and the drifted symbols are mostly types). - Flags an exported declaration whose name the spec owns unless it is derived: re-exported from the spec, or declared with a spec import in a STRUCTURAL position. A spec name merely mentioned inside a members block does not count — that is what a fork looks like. Consts do not descend into array/object literals, because a faithful copy passes every value comparison and only reference identity distinguishes a re-export from a fork. - Ledger of the 156 pre-existing collisions names each symbol rather than counting per package: a count is a budget that lets the next fork land as long as an unrelated one was fixed, and it makes the failure point at whichever symbol sorts first instead of the one just written. `--ledger` regenerates it. - Shrink-only both ways: a fresh collision fails by name, and a fixed symbol fails until its ledger entry is deleted. Barrel re-exports through a relative path or a `@object-ui/*` sibling are not counted twice; the declaration site they point at is judged on its own turn. Discrimination proof — three ways it fails, run before landing: 1. reverting `ActionType` to the pre-#2231 hand union names it exactly: "type `ActionType` packages/types/src/ui-action.ts:80 (exported by `@objectstack/spec/ui`)" 2. a new fork in a package with an empty ledger fails on the PR that writes it 3. renaming a ledger symbol to a dialect fails until its entry is deleted Refs objectstack#4115, objectstack#4074, #3003, #3009, #2944, #2911 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy --- .github/workflows/ci.yml | 8 + package.json | 1 + scripts/check-spec-symbol-derivation.mjs | 617 +++++++++++++++++++++++ 3 files changed, 626 insertions(+) create mode 100644 scripts/check-spec-symbol-derivation.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13a2f85f8..af9ca5b16 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -75,6 +75,14 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # A local type/const declared under a `@objectstack/spec` export's NAME reads + # to the next agent as the spec's own definition — four such symbols had + # already drifted from the spec they claimed to be (objectstack#4115). Needs + # the install (it reads the spec's own `.d.ts`) but not the build, so it runs + # before the expensive steps. + - name: Verify spec-named symbols are derived, not hand-written + run: pnpm check:spec-symbols + - name: Turbo Cache uses: actions/cache@v6 with: diff --git a/package.json b/package.json index 05d37caff..3c43cb910 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,7 @@ "lint:coverage": "node scripts/check-lint-coverage.mjs", "type-check": "turbo run type-check", "type-check:coverage": "node scripts/check-type-check-coverage.mjs", + "check:spec-symbols": "node scripts/check-spec-symbol-derivation.mjs", "cli": "node packages/cli/dist/cli.js", "objectui": "node packages/cli/dist/cli.js", "create-plugin": "node packages/create-plugin/dist/index.js", diff --git a/scripts/check-spec-symbol-derivation.mjs b/scripts/check-spec-symbol-derivation.mjs new file mode 100644 index 000000000..d4d1928c1 --- /dev/null +++ b/scripts/check-spec-symbol-derivation.mjs @@ -0,0 +1,617 @@ +#!/usr/bin/env node +/** + * A spec-named symbol must be DERIVED from `@objectstack/spec`, not hand-written. + * + * The failure class (objectstack#4115): objectui declares a local type or const + * under the SAME NAME as a `@objectstack/spec` export, with a doc comment + * claiming spec canonicity — and the declaration has drifted, or is one spec + * release away from drifting. Every audited instance had: + * + * ActionType "canonical definition from @objectstack/spec" → hand union, missing `form` (#2231/#2901) + * ChartType (sibling schema re-exported under spec's name) → 7 of 19 members (objectui#2944) + * ActionLocation + 2 "single source of truth… re-export here" → re-DECLARED union + z.enum (#4074) + * ActionParamFieldType "aligned with the field types available in spec" → 16 of 49 members (#4074) + * + * What makes this class expensive is specific to agent-driven development: an + * agent reads the comment, takes it as ground truth, and builds on it. #2901 was + * filed with a backwards premise because the fork was re-exported under the + * spec's own symbol name. A wrong canonical-claim is not stale documentation — + * it is a planted premise for the next session. + * + * This guard lands the rule AS THE CHECK, not as an AGENTS.md paragraph. A + * prose-only rule is precisely the "declared ≠ enforced" landmine the whole + * thread is about (#4074, objectui#3009 — where a guard file's own header + * falsely claimed its checks were "the real enforcement" for the entire interval + * during which nothing compiled them). + * + * ── What counts as derived ─────────────────────────────────────────────────── + * Both of the repo's sanctioned forms pass, because both bind the local name to + * the spec at compile time: + * + * export type { ActionLocation } from '@objectstack/spec/ui'; // re-export + * export type ActionType = z.infer; // derivation + * + * A declaration is derived only when the spec reference sits in a STRUCTURAL + * position — a type alias's own type node, an interface's `extends`, a const's + * initializer. A spec name merely *mentioned* inside a members block does not + * count, because that is what a hand-written fork looks like: + * + * export interface ActionSchema { locations?: ActionLocation[] } // NOT derived + * + * Consts are stricter still: object and array literals are not descended into. + * `export const ACTION_LOCATIONS = [...SpecLocations]` is a COPY, and a faithful + * copy passes every value comparison — reference identity is the only check that + * distinguishes a re-export from a fork (objectui#3003, and the insight already + * written down in `spec-subschema-parity.test.ts`). + * + * Run: node scripts/check-spec-symbol-derivation.mjs + * Exit: 0 = OK, 1 = a spec-named symbol is hand-written, or the allowlist is stale + */ + +import ts from "typescript"; +import { createRequire } from "module"; +import { readFileSync, readdirSync, statSync } from "fs"; +import { resolve, dirname, join, relative } from "path"; +import { fileURLToPath } from "url"; + +const root = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +// ── Allowlist ──────────────────────────────────────────────────────────────── +// Same governance as `check-type-check-coverage.mjs`'s DEBT map: declared, +// reasoned, shrink-only. An entry that no longer matches a real violation is +// stale and fails this guard, so the list cannot outlive the code it excuses. +// +// A same-name symbol that is narrower or wider ON PURPOSE belongs here with that +// purpose written down — at which point the comment is load-bearing instead of +// decorative. A symbol that is *supposed* to be the spec's should be imported +// instead; that is the whole point of the check. +// +// Key format: ":". +const ALLOW = { + "@object-ui/types:ActionSchema": { + reason: + "Two deliberate objectui-side shapes, both documented at their declaration. " + + "`ui-action.ts` is a renderer VIEW over the spec's action — it carries renderer-only " + + "fields the spec does not model, while importing the spec-owned parts it does share " + + "(`ActionLocation`, `ActionType`). `crud.ts` is the explicitly @deprecated legacy " + + "shape kept for backward compatibility and slated for removal in a future major.", + issue: 4115, + }, +}; + +// ── Untriaged collisions (the ledger) ──────────────────────────────────────── +// Same governance as `check-type-check-coverage.mjs`'s DEBT map: declared, +// shrink-only. These are the same-name symbols that predate this guard and have +// not been triaged one-by-one yet. This check exists to stop the BLEEDING — a +// new fork fails on the PR that writes it — not to retro-fix 166 symbols at once. +// +// Named symbols rather than a per-package COUNT, deliberately: a count is a +// budget. It lets the next fork land as long as an unrelated one was fixed in +// the same PR, and it makes the failure message point at whichever collisions +// happen to sort first instead of at the one just written. The list is longer; +// it is also the thing that names the new symbol at the moment it appears. +// +// To burn one down: import/derive it from the spec, rename it to a declared +// local dialect (`ObjectUiLocal…`, with a tripwire test asserting the spec does +// not own the name), or move it to ALLOW with the reason it deliberately +// differs. Then delete it from this list — leaving it here fails the ratchet. +const DEBT_ISSUE = 4115; +const DEBT = { + "@object-ui/types": [ + "ActionParam", + "AppContextSelectorSchema", + "AppSchema", + "BatchOperationResult", + "BreakpointName", + "CacheStrategy", + "ChartSeries", + "ChartSeriesSchema", + "ConditionalValidation", + "CreateExportJobRequest", + "CreateExportJobResult", + "CrossFieldValidation", + "DashboardSchema", + "DashboardWidgetSchema", + "DatasourceSchema", + "DriverInterface", + "EventHandler", + "ExportJobStatus", + "ExpressionSchema", + "FeedItemType", + "FileMetadata", + "FilterCondition", + "FilterConditionSchema", + "FilterOperator", + "FormField", + "FormFieldSchema", + "FormatValidation", + "GestureConfig", + "GestureType", + "GlobalFilterSchema", + "GroupByNode", + "ImportJobStatus", + "ImportRowResult", + "ImportWriteMode", + "JoinNode", + "JoinStrategy", + "JoinedReportBlock", + "ListViewSchema", + "MutationEvent", + "NavigationArea", + "NavigationAreaSchema", + "NavigationItem", + "NavigationItemSchema", + "ObjectIndex", + "ObjectPermission", + "OfflineConfig", + "PageRegion", + "PageRegionSchema", + "PageSchema", + "PageVariable", + "PageVariableSchema", + "PermissionAction", + "QueryAST", + "QuerySchema", + "ReportSchedule", + "ReportSchema", + "ResponsiveConfig", + "ScriptValidation", + "SelectOption", + "SelectOptionSchema", + "SharingRule", + "SpanSchema", + "StateMachineValidation", + "Theme", + "ThemeSchema", + "ValidationError", + "ValidationRule", + "ValidationRuleSchema", + "WidgetManifest", + "WidgetSource", + "WindowFunction", + ], + "@object-ui/app-shell": [ + "ConversationSummary", + "DecisionOutputDef", + "ExplainDecision", + "ExplainLayer", + "ExplainMatchedRule", + "ExplainRecordAttribution", + "ExternalCatalog", + "ExternalColumn", + "ExternalTable", + "FieldGroup", + "FieldInput", + "FilterCondition", + "FlowEdge", + "FlowNode", + "GenerateDraftOpts", + "InstalledPackage", + "ObjectDraft", + "PackageManifest", + "PageHeaderProps", + "PluginPermissions", + "RemoteTable", + "RuntimeConfig", + "SchemaDiffEntry", + "SchemaDiffEntryKind", + "SchemaValidationResult", + "ScreenFieldSpec", + "ScreenSpec", + "isAggregatedViewContainer", + ], + "@object-ui/core": [ + "ActionHandler", + "CONTEXT_TOKENS", + "ChartSeries", + "CrudAffordances", + "PluginDefinition", + "ResponsiveStyles", + "RowCrudPredicates", + "RowHeight", + "StyleMap", + "ValidationError", + "ValidationResult", + "defineView", + "resolveCrudAffordances", + ], + "@object-ui/auth": [ + "AuthProvider", + "AuthProviderConfig", + "AuthSession", + "AuthUser", + "DelegableScope", + "TenancyPosture", + ], + "@object-ui/components": [ + "Field", + "FilterCondition", + "ShareLink", + "ShareLinkAudience", + "ShareLinkPermission", + "SortItem", + ], + "@object-ui/react": [ + "ConflictResolutionStrategy", + "NavigationConfig", + "OfflineCacheConfig", + "OfflineConfig", + "OfflineStrategy", + "PerformanceConfig", + ], + "@object-ui/data-objectstack": [ + "CacheStats", + "DroppedFieldsEvent", + "MetadataSaveOptions", + "SecurityPolicy", + "ValidationError", + ], + "@object-ui/plugin-chatbot": [ + "MessageContent", + "PendingActionRow", + "PendingActionStatus", + "Tool", + ], + "@object-ui/plugin-list": [ + "ListView", + "UserFilters", + "ViewTab", + ], + "@object-ui/fields": [ + "FieldWidgetProps", + "isFileIdToken", + ], + "@object-ui/layout": [ + "Page", + "PageHeaderProps", + ], + "@object-ui/plugin-detail": [ + "FeedFilterMode", + "ObjectFieldLike", + ], + "@object-ui/plugin-grid": [ + "ColumnSummaryConfig", + "isMultiValueField", + ], + "@object-ui/collaboration": [ + "RealtimeConfig", + ], + "@object-ui/plugin-charts": [ + "ChartConfig", + ], + "@object-ui/plugin-form": [ + "FormSection", + ], + "@object-ui/providers": [ + "Theme", + ], + "@object-ui/runner": [ + "App", + ], + "@object-ui/sdui-parser": [ + "ValidationResult", + ], +}; + +// Files under these paths are not objectui's own authored surface. +// - `ui/` is the Shadcn no-touch zone (AGENTS.md #7): upstream 3rd-party files +// overwritten by sync scripts, so a collision there is not ours to fix. +const SKIP_PATH_SEGMENTS = [`${"components"}/src/ui/`]; + +const isSpecModule = (m) => m === "@objectstack/spec" || m.startsWith("@objectstack/spec/"); + +// ── 1. Enumerate every `@objectstack/spec` export name, per subpath ────────── +// Types AND values: the drifted symbols in the table above are mostly types, and +// a runtime `import()` only sees values. The compiler's own view of each +// subpath's `.d.ts` is the only source that covers both. +function specExportNames() { + const require = createRequire(import.meta.url); + let pkgPath; + try { + pkgPath = require.resolve("@objectstack/spec/package.json"); + } catch { + console.error( + "❌ cannot resolve @objectstack/spec — run `pnpm install` first.\n" + + " This guard reads the spec's own type declarations; it cannot fall back to a\n" + + " hardcoded name list without becoming the stale copy it exists to prevent." + ); + process.exit(1); + } + const pkgDir = dirname(pkgPath); + const pkg = JSON.parse(readFileSync(pkgPath, "utf8")); + + const entries = []; + for (const [sub, cond] of Object.entries(pkg.exports ?? {})) { + if (typeof cond !== "object" || cond === null) continue; + const dts = cond?.import?.types ?? cond?.require?.types; + if (!dts) continue; + entries.push({ sub: sub === "." ? "@objectstack/spec" : `@objectstack/spec${sub.slice(1)}`, file: resolve(pkgDir, dts) }); + } + + const program = ts.createProgram( + entries.map((e) => e.file), + { + noEmit: true, + skipLibCheck: true, + strict: false, + target: ts.ScriptTarget.ESNext, + module: ts.ModuleKind.ESNext, + moduleResolution: ts.ModuleResolutionKind.Bundler, + } + ); + const checker = program.getTypeChecker(); + + const names = new Map(); // name -> Set + for (const entry of entries) { + const sf = program.getSourceFile(entry.file); + if (!sf) continue; + const moduleSymbol = checker.getSymbolAtLocation(sf); + if (!moduleSymbol) continue; + for (const exported of checker.getExportsOfModule(moduleSymbol)) { + const name = exported.getName(); + if (!names.has(name)) names.set(name, new Set()); + names.get(name).add(entry.sub); + } + } + return names; +} + +// ── 2. Scan objectui's authored source for exported declarations ───────────── +function sourceFiles() { + const out = []; + const pkgsDir = resolve(root, "packages"); + for (const pkg of readdirSync(pkgsDir)) { + const srcDir = join(pkgsDir, pkg, "src"); + try { + if (!statSync(srcDir).isDirectory()) continue; + } catch { + continue; + } + walk(srcDir, out); + } + return out; +} + +function walk(dir, out) { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = join(dir, entry.name); + if (entry.isDirectory()) { + // Tests do not publish a surface, so a local type there cannot be mistaken + // for the spec's by an importer. Keeps the signal on the public API. + if (entry.name === "node_modules" || entry.name === "dist" || entry.name === "__tests__") continue; + walk(full, out); + continue; + } + if (!/\.tsx?$/.test(entry.name) || /\.d\.ts$/.test(entry.name)) continue; + if (/\.test\.tsx?$/.test(entry.name) || /\.stories\.tsx?$/.test(entry.name)) continue; + out.push(full); + } +} + +const packageNameCache = new Map(); +function packageNameFor(file) { + const rel = relative(root, file); + const pkgDir = rel.split("/").slice(0, 2).join("/"); + if (!packageNameCache.has(pkgDir)) { + let name = pkgDir; + try { + name = JSON.parse(readFileSync(resolve(root, pkgDir, "package.json"), "utf8")).name ?? pkgDir; + } catch { + /* keep the directory name */ + } + packageNameCache.set(pkgDir, name); + } + return packageNameCache.get(pkgDir); +} + +const hasExportModifier = (node) => + node.modifiers?.some((m) => m.kind === ts.SyntaxKind.ExportKeyword) ?? false; + +/** + * Does this subtree reference a name bound to a spec import? + * + * `skipLiterals` stops the walk at object/array literals and type-literal member + * blocks: a spec name mentioned inside a members block is a hand-written shape + * that merely USES a spec type, which is exactly what a fork looks like. + */ +function referencesSpec(node, bindings, skipLiterals) { + let found = false; + const visit = (n) => { + if (found || !n) return; + if (skipLiterals) { + if (ts.isTypeLiteralNode(n) || ts.isObjectLiteralExpression(n) || ts.isArrayLiteralExpression(n)) return; + } + if (ts.isIdentifier(n) && bindings.has(n.text)) { + found = true; + return; + } + ts.forEachChild(n, visit); + }; + ts.forEachChild(node, visit); + return found; +} + +function scanFile(file, specNames) { + const text = readFileSync(file, "utf8"); + const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, /\.tsx$/.test(file) ? ts.ScriptKind.TSX : ts.ScriptKind.TS); + + // Local names bound to a `@objectstack/spec` import in THIS file. + const specBindings = new Set(); + for (const stmt of sf.statements) { + if (!ts.isImportDeclaration(stmt) || !ts.isStringLiteral(stmt.moduleSpecifier)) continue; + if (!isSpecModule(stmt.moduleSpecifier.text)) continue; + const clause = stmt.importClause; + if (!clause) continue; + if (clause.name) specBindings.add(clause.name.text); + const named = clause.namedBindings; + if (named && ts.isNamespaceImport(named)) specBindings.add(named.name.text); + if (named && ts.isNamedImports(named)) for (const el of named.elements) specBindings.add(el.name.text); + } + + const findings = []; + const record = (name, kind, derived, node) => { + if (!specNames.has(name) || derived) return; + const { line } = sf.getLineAndCharacterOfPosition(node.getStart(sf)); + findings.push({ name, kind, file, line: line + 1, subpaths: [...specNames.get(name)] }); + }; + + for (const stmt of sf.statements) { + // `export { X } from '…'` / `export { X }` / `export type { X } from '…'` + if (ts.isExportDeclaration(stmt)) { + const fromModule = stmt.moduleSpecifier && ts.isStringLiteral(stmt.moduleSpecifier) ? stmt.moduleSpecifier.text : null; + // `export * from '…'` exports names this AST pass cannot attribute. When it + // re-exports the spec it is derivation by definition; when it re-exports a + // sibling, that sibling's own declaration is scanned on its own turn. + if (!stmt.exportClause || !ts.isNamedExports(stmt.exportClause)) continue; + // A barrel re-exporting a module this scan already covers — a relative path + // or a `@object-ui/*` sibling — is not a second finding. Whatever it points + // at gets judged at its own declaration site; reporting the barrel too would + // just make one fork look like four, and would make the fix land in the + // barrel rather than at the declaration. + if (fromModule && (fromModule.startsWith(".") || fromModule.startsWith("@object-ui/"))) continue; + for (const el of stmt.exportClause.elements) { + const exportedName = el.name.text; + const localName = (el.propertyName ?? el.name).text; + const derived = fromModule + ? isSpecModule(fromModule) // re-export straight from the spec + : specBindings.has(localName); // `import { X } from spec; export { X }` + record(exportedName, "re-export", derived, el); + } + continue; + } + + if (!hasExportModifier(stmt)) continue; + + if (ts.isTypeAliasDeclaration(stmt)) { + record(stmt.name.text, "type", referencesSpec(stmt, specBindings, true), stmt); + } else if (ts.isInterfaceDeclaration(stmt)) { + // Only `extends` counts — see the header. + const extendsSpec = (stmt.heritageClauses ?? []).some((h) => referencesSpec(h, specBindings, false)); + record(stmt.name.text, "interface", extendsSpec, stmt); + } else if (ts.isClassDeclaration(stmt) && stmt.name) { + const extendsSpec = (stmt.heritageClauses ?? []).some((h) => referencesSpec(h, specBindings, false)); + record(stmt.name.text, "class", extendsSpec, stmt); + } else if (ts.isVariableStatement(stmt)) { + for (const decl of stmt.declarationList.declarations) { + if (!ts.isIdentifier(decl.name)) continue; + record(decl.name.text, "const", decl.initializer ? referencesSpec(decl, specBindings, true) : false, decl); + } + } else if (ts.isEnumDeclaration(stmt)) { + // An enum cannot be derived from anything — a spec-named one is a fork. + record(stmt.name.text, "enum", false, stmt); + } else if (ts.isFunctionDeclaration(stmt) && stmt.name) { + record(stmt.name.text, "function", false, stmt); + } + } + return findings; +} + +// ── 3. Report ──────────────────────────────────────────────────────────────── +const specNames = specExportNames(); +const files = sourceFiles().filter((f) => !SKIP_PATH_SEGMENTS.some((seg) => f.includes(seg))); + +const violations = []; +for (const file of files) violations.push(...scanFile(file, specNames)); + +const matchedAllowKeys = new Set(); +const unallowed = []; +for (const v of violations) { + const pkg = packageNameFor(v.file); + const key = `${pkg}:${v.name}`; + if (ALLOW[key]) { + matchedAllowKeys.add(key); + continue; + } + unallowed.push({ ...v, pkg, key }); +} + +const byPackage = new Map(); +for (const v of unallowed) { + if (!byPackage.has(v.pkg)) byPackage.set(v.pkg, []); + byPackage.get(v.pkg).push(v); +} + +// `--ledger` regenerates the DEBT block from the working tree, so the list is +// never hand-maintained (and so burning symbols down is a mechanical edit). +if (process.argv.includes("--ledger")) { + const lines = [...byPackage.entries()] + .sort((a, b) => b[1].length - a[1].length || a[0].localeCompare(b[0])) + .map(([pkg, found]) => { + const names = [...new Set(found.map((v) => v.name))].sort(); + return ` ${JSON.stringify(pkg)}: [\n` + names.map((n) => ` ${JSON.stringify(n)},`).join("\n") + `\n ],`; + }); + console.log("const DEBT = {\n" + lines.join("\n") + "\n};"); + process.exit(0); +} + +const errors = []; + +// 1. A collision that is not in the ledger is NEW. This is the half that stops +// the bleeding: a fresh fork fails on the PR that writes it, by name. +for (const [pkg, found] of byPackage) { + const declared = new Set(DEBT[pkg] ?? []); + const fresh = found.filter((v) => !declared.has(v.name)); + if (fresh.length === 0) continue; + errors.push( + `${pkg} declares ${fresh.length} spec-named symbol${fresh.length === 1 ? "" : "s"} the spec already owns:\n` + + fresh + .map( + (v) => + ` ${v.kind} \`${v.name}\` ${relative(root, v.file)}:${v.line} ` + + `(exported by \`${v.subpaths.join("`, `")}\`)` + ) + .join("\n") + + `\n Import it (\`export type { X } from '@objectstack/spec/…'\`), derive it\n` + + ` (\`export type X = z.infer\`), or — if it deliberately differs — rename it\n` + + ` to a declared dialect (\`ObjectUiLocalX\`, with a tripwire test asserting the spec does\n` + + ` not own the name) or add an ALLOW entry with the reason.` + ); +} + +// 2. Ratchet — a ledger entry whose symbol is fixed (or gone) must be deleted. +// Left in, it reserves the name: the next fork under it would land silently. +for (const [pkg, names] of Object.entries(DEBT)) { + const live = new Set((byPackage.get(pkg) ?? []).map((v) => v.name)); + const stale = names.filter((n) => !live.has(n)); + if (stale.length === 0) continue; + errors.push( + `${pkg} lists ${stale.length} symbol${stale.length === 1 ? "" : "s"} in DEBT that no longer collide` + + ` — \`${stale.join("`, `")}\`.\n` + + ` Delete them from scripts/check-spec-symbol-derivation.mjs (\`--ledger\` regenerates the\n` + + ` block) so the names cannot be re-forked silently` + + `${DEBT_ISSUE ? ` (and close #${DEBT_ISSUE} once the ledger is empty)` : ""}.` + ); +} + +// 3. Ratchet — an ALLOW entry that excuses nothing is stale and must go, or it +// silently keeps a name reserved for a future fork. +for (const key of Object.keys(ALLOW)) { + if (!matchedAllowKeys.has(key)) { + errors.push( + `${key} is in ALLOW but no longer collides with a spec export name — the symbol was\n` + + ` renamed, removed, or is now imported from the spec. Delete the entry so the\n` + + ` exemption cannot be inherited by a future fork under the same name.` + ); + } +} + +const outstanding = Object.values(DEBT).reduce((sum, names) => sum + names.length, 0); + +if (errors.length === 0) { + console.log( + `✅ spec symbol derivation: ${files.length} files scanned against ${specNames.size} spec export names; ` + + `${Object.keys(ALLOW).length} declared dialect${Object.keys(ALLOW).length === 1 ? "" : "s"}, ` + + `${outstanding} untriaged collision${outstanding === 1 ? "" : "s"} in ${Object.keys(DEBT).length} packages.` + ); + process.exit(0); +} + +console.error("❌ a spec-named symbol is hand-written, not derived:\n"); +for (const message of errors) console.error(` • ${message}\n`); +console.error( + "A local declaration under a spec export's name is read by the next agent as the spec's own\n" + + "definition — that is how #2901 was filed with a backwards premise. See\n" + + "https://github.com/objectstack-ai/objectstack/issues/4115 for the four symbols that had\n" + + "already drifted when this guard was written." +); +process.exit(1); From ea84651583bd9c093dc1e5bdb410544d076b0002 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:33:35 +0000 Subject: [PATCH 2/6] test(type-check): 12 packages type-check their tests, and the gap is now declared (objectstack#4118) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A package tsconfig excludes test files — correctly, it is the BUILD config and they would emit into `dist`. The hole is that no other `tsc` invocation read them either, so for most packages nothing compiled a test at all. objectui#3009 found what that costs: `spec-derived-unions.test.ts` had built its whole contract on `satisfies` checks that never ran, under a header calling them "the real enforcement". Reverting a derived alias produced zero errors. Tests are where agents encode their understanding of a contract. Unchecked, a wrong understanding accumulates silently and then reads as evidence. Wires up every package measured at zero code-tier errors — 12, not the 8 the issue estimated. The extra four fell out of a better config template: `plugin-calendar` and `plugin-kanban` needed only the jest-dom matcher types, `providers` was already clean, and `sdui-parser`'s 50 TS2304s were env globals exactly as the issue suspected, not real errors. Wiring `sdui-parser` up surfaced a real bug: `render.test.tsx` imports `@object-ui/core` and `@object-ui/react` with neither declared in its package.json. Both were phantom — resolved only through the ROOT tsconfig's `paths` alias and vitest, never as a dependency a consumer would get. Declared as devDependencies, which is also what makes the `paths: {}` in the new config meaningful: with the alias in place an undeclared workspace import still compiles. Every `tsconfig.test.json` follows objectui#3009's template — `noEmit`, `composite: false`, `paths: {}` to kill the TS6059 source-leak noise, and `lib` / `types` only where the tests actually need them (`mobile` reads `Array.prototype.at`, so ES2022; `plugin-report` pulls in a source file that reads `process.env`, so `node`). The guard grows a second half so this cannot silently regress. A package with test files either compiles them or carries a TEST_DEBT entry with a MEASURED count — re-measured against current main, not copied from the issue's table: 240 code-tier errors across 15 packages. It also fails on a `tsconfig.test.json` that nothing runs, which is the objectui#3009 shape itself: a file that looks like enforcement while no compiler reads it. Discrimination proof — three ways it fails, run before landing: 1. unchaining plugin-tree's config: "has a tsconfig.test.json that its type-check script never runs … the exact shape of objectui#3009" 2. deleting the `permissions` TEST_DEBT entry: "has 3 test files that no `tsc` invocation reads" 3. a package listed in both DEBT and TEST_DEBT is rejected as one gap counted twice (this is how `layout` was caught during the sweep) Refs objectstack#4118, objectui#3009, objectstack#4074, #2911, #2901 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy --- packages/mobile/package.json | 2 +- packages/mobile/tsconfig.test.json | 22 +++ packages/plugin-calendar/package.json | 2 +- packages/plugin-calendar/tsconfig.test.json | 24 +++ packages/plugin-designer/package.json | 2 +- packages/plugin-designer/tsconfig.test.json | 18 ++ packages/plugin-kanban/package.json | 2 +- packages/plugin-kanban/tsconfig.test.json | 24 +++ packages/plugin-markdown/package.json | 2 +- packages/plugin-markdown/tsconfig.test.json | 18 ++ packages/plugin-report/package.json | 2 +- packages/plugin-report/tsconfig.test.json | 25 +++ packages/plugin-timeline/package.json | 2 +- packages/plugin-timeline/tsconfig.test.json | 18 ++ packages/plugin-tree/package.json | 2 +- packages/plugin-tree/tsconfig.test.json | 18 ++ packages/providers/package.json | 2 +- packages/providers/tsconfig.test.json | 18 ++ packages/react-runtime/package.json | 2 +- packages/react-runtime/tsconfig.test.json | 18 ++ packages/runner/package.json | 2 +- packages/runner/tsconfig.test.json | 18 ++ packages/sdui-parser/package.json | 7 +- packages/sdui-parser/tsconfig.test.json | 25 +++ pnpm-lock.yaml | 9 +- scripts/check-type-check-coverage.mjs | 183 +++++++++++++++++++- 26 files changed, 449 insertions(+), 18 deletions(-) create mode 100644 packages/mobile/tsconfig.test.json create mode 100644 packages/plugin-calendar/tsconfig.test.json create mode 100644 packages/plugin-designer/tsconfig.test.json create mode 100644 packages/plugin-kanban/tsconfig.test.json create mode 100644 packages/plugin-markdown/tsconfig.test.json create mode 100644 packages/plugin-report/tsconfig.test.json create mode 100644 packages/plugin-timeline/tsconfig.test.json create mode 100644 packages/plugin-tree/tsconfig.test.json create mode 100644 packages/providers/tsconfig.test.json create mode 100644 packages/react-runtime/tsconfig.test.json create mode 100644 packages/runner/tsconfig.test.json create mode 100644 packages/sdui-parser/tsconfig.test.json diff --git a/packages/mobile/package.json b/packages/mobile/package.json index 293749b81..2f2cd3a72 100644 --- a/packages/mobile/package.json +++ b/packages/mobile/package.json @@ -30,7 +30,7 @@ "build": "tsc", "clean": "rm -rf dist", "test": "vitest run", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "peerDependencies": { diff --git a/packages/mobile/tsconfig.test.json b/packages/mobile/tsconfig.test.json new file mode 100644 index 000000000..5af001b05 --- /dev/null +++ b/packages/mobile/tsconfig.test.json @@ -0,0 +1,22 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build is `composite` + `declaration`; this project emits + // nothing, so it must not inherit those. + "composite": false, + // `gesture-spec-parity.test.tsx` reads `mock.calls.at(-1)`. ES2022 is the + // floor for `Array.prototype.at` — the root config's ES2020 is what the + // SOURCE targets, and a test is free to need more than the shipped bundle. + "lib": ["ES2022", "DOM", "DOM.Iterable"], + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-calendar/package.json b/packages/plugin-calendar/package.json index 5db907fa3..8fc97832c 100644 --- a/packages/plugin-calendar/package.json +++ b/packages/plugin-calendar/package.json @@ -27,7 +27,7 @@ "build": "vite build", "test": "vitest run", "test:watch": "vitest", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/plugin-calendar/tsconfig.test.json b/packages/plugin-calendar/tsconfig.test.json new file mode 100644 index 000000000..a98ee894b --- /dev/null +++ b/packages/plugin-calendar/tsconfig.test.json @@ -0,0 +1,24 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // The `toBeInTheDocument` / `toHaveTextContent` matchers these tests use are + // a global augmentation, not an import, and `@testing-library/jest-dom` does + // not live under `@types/` — so it is never picked up automatically and has + // to be named here. Naming `types` at all switches off automatic `@types/*` + // inclusion, which is fine: nothing in these tests touches Node globals. + "types": ["@testing-library/jest-dom"], + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-designer/package.json b/packages/plugin-designer/package.json index 045bd865c..23a95a451 100644 --- a/packages/plugin-designer/package.json +++ b/packages/plugin-designer/package.json @@ -24,7 +24,7 @@ "build": "vite build", "clean": "rm -rf dist", "test": "vitest run", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "peerDependencies": { diff --git a/packages/plugin-designer/tsconfig.test.json b/packages/plugin-designer/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/plugin-designer/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-kanban/package.json b/packages/plugin-kanban/package.json index 92bdbbd55..778e2a8a8 100644 --- a/packages/plugin-kanban/package.json +++ b/packages/plugin-kanban/package.json @@ -27,7 +27,7 @@ "build": "vite build", "test": "vitest run", "test:watch": "vitest", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/plugin-kanban/tsconfig.test.json b/packages/plugin-kanban/tsconfig.test.json new file mode 100644 index 000000000..a98ee894b --- /dev/null +++ b/packages/plugin-kanban/tsconfig.test.json @@ -0,0 +1,24 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // The `toBeInTheDocument` / `toHaveTextContent` matchers these tests use are + // a global augmentation, not an import, and `@testing-library/jest-dom` does + // not live under `@types/` — so it is never picked up automatically and has + // to be named here. Naming `types` at all switches off automatic `@types/*` + // inclusion, which is fine: nothing in these tests touches Node globals. + "types": ["@testing-library/jest-dom"], + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-markdown/package.json b/packages/plugin-markdown/package.json index df168f237..d944dd54d 100644 --- a/packages/plugin-markdown/package.json +++ b/packages/plugin-markdown/package.json @@ -27,7 +27,7 @@ "build": "vite build", "test": "vitest run", "test:watch": "vitest", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/plugin-markdown/tsconfig.test.json b/packages/plugin-markdown/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/plugin-markdown/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-report/package.json b/packages/plugin-report/package.json index 32388b525..79afe34be 100644 --- a/packages/plugin-report/package.json +++ b/packages/plugin-report/package.json @@ -20,7 +20,7 @@ ], "scripts": { "build": "vite build", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "clean": "rm -rf dist", "test": "vitest run", "lint": "eslint ." diff --git a/packages/plugin-report/tsconfig.test.json b/packages/plugin-report/tsconfig.test.json new file mode 100644 index 000000000..be3d460a9 --- /dev/null +++ b/packages/plugin-report/tsconfig.test.json @@ -0,0 +1,25 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing. + "composite": false, + // Naming `types` at all switches off automatic `@types/*` inclusion, so + // BOTH entries are load-bearing: + // - jest-dom: the `toBeInTheDocument` / `toHaveClass` matchers these tests + // use are a global augmentation, not an import, and it does not live + // under `@types/` so it is never picked up automatically. + // - node: the test program pulls in `LegacyReportRenderer.tsx`, which + // reads `process.env`. + "types": ["@testing-library/jest-dom", "node"], + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-timeline/package.json b/packages/plugin-timeline/package.json index 9c023888a..a00d50fbb 100644 --- a/packages/plugin-timeline/package.json +++ b/packages/plugin-timeline/package.json @@ -27,7 +27,7 @@ "build": "vite build", "test": "vitest run", "test:watch": "vitest", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/plugin-timeline/tsconfig.test.json b/packages/plugin-timeline/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/plugin-timeline/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/plugin-tree/package.json b/packages/plugin-tree/package.json index ba8719a77..0ffeeac1b 100644 --- a/packages/plugin-tree/package.json +++ b/packages/plugin-tree/package.json @@ -27,7 +27,7 @@ "build": "vite build", "test": "vitest run", "test:watch": "vitest", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/plugin-tree/tsconfig.test.json b/packages/plugin-tree/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/plugin-tree/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/providers/package.json b/packages/providers/package.json index cdd96d35b..6585fb5ab 100644 --- a/packages/providers/package.json +++ b/packages/providers/package.json @@ -26,7 +26,7 @@ "scripts": { "build": "tsc", "test": "vitest run", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/providers/tsconfig.test.json b/packages/providers/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/providers/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/react-runtime/package.json b/packages/react-runtime/package.json index b834f49e5..1ed85fbac 100644 --- a/packages/react-runtime/package.json +++ b/packages/react-runtime/package.json @@ -21,7 +21,7 @@ "scripts": { "build": "tsc", "test": "vitest run", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/react-runtime/tsconfig.test.json b/packages/react-runtime/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/react-runtime/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/runner/package.json b/packages/runner/package.json index bb17631f8..d2a05842d 100644 --- a/packages/runner/package.json +++ b/packages/runner/package.json @@ -16,7 +16,7 @@ "scripts": { "dev": "vite", "build": "vite build", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint .", "preview": "vite preview", "test": "vitest run" diff --git a/packages/runner/tsconfig.test.json b/packages/runner/tsconfig.test.json new file mode 100644 index 000000000..1edf1cd83 --- /dev/null +++ b/packages/runner/tsconfig.test.json @@ -0,0 +1,18 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/packages/sdui-parser/package.json b/packages/sdui-parser/package.json index 2fad2afc4..2a0320c33 100644 --- a/packages/sdui-parser/package.json +++ b/packages/sdui-parser/package.json @@ -27,8 +27,11 @@ "scripts": { "build": "tsc", "test": "vitest run", - "type-check": "tsc --noEmit", + "type-check": "tsc --noEmit && tsc -p tsconfig.test.json", "lint": "eslint ." }, - "devDependencies": {} + "devDependencies": { + "@object-ui/core": "workspace:*", + "@object-ui/react": "workspace:*" + } } diff --git a/packages/sdui-parser/tsconfig.test.json b/packages/sdui-parser/tsconfig.test.json new file mode 100644 index 000000000..49dbdf50b --- /dev/null +++ b/packages/sdui-parser/tsconfig.test.json @@ -0,0 +1,25 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. + // + // Wiring this up is what revealed that `render.test.tsx` imported + // `@object-ui/core` and `@object-ui/react` with neither declared in + // package.json — phantom dependencies that only resolved through the ROOT + // tsconfig's `paths` alias and vitest. Both are devDependencies now, so the + // test resolves them the way a consumer would. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing. + "composite": false, + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + // This is also what makes the phantom-dependency check above meaningful: + // with the alias in place, an undeclared workspace import still compiles. + "paths": {} + }, + "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2a48e9df1..073234d9e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2638,7 +2638,14 @@ importers: specifier: ^8.1.5 version: 8.1.5(@types/node@26.1.1)(esbuild@0.28.1)(jiti@2.7.0)(tsx@4.23.1)(yaml@2.9.0) - packages/sdui-parser: {} + packages/sdui-parser: + devDependencies: + '@object-ui/core': + specifier: workspace:* + version: link:../core + '@object-ui/react': + specifier: workspace:* + version: link:../react packages/types: dependencies: diff --git a/scripts/check-type-check-coverage.mjs b/scripts/check-type-check-coverage.mjs index c34d81bfe..cf58755df 100644 --- a/scripts/check-type-check-coverage.mjs +++ b/scripts/check-type-check-coverage.mjs @@ -12,6 +12,17 @@ * only shrink: once a package gains the script, its entry has to be deleted or * this guard fails. * + * The same question applies one level down, to TESTS (objectstack#4118): a + * package tsconfig excludes test files — correctly, it is the BUILD config and + * they would emit into `dist` — and for most packages no other `tsc` invocation + * read them either. Tests are where agents encode their understanding of a + * contract, so unchecked tests let a wrong understanding accumulate silently and + * then READ AS EVIDENCE: objectui#3009 found `spec-derived-unions.test.ts` had + * built its whole contract on `satisfies` checks that never ran, under a header + * calling them "the real enforcement". Reverting a derived alias produced zero + * errors. So the second half of this guard: a package with test files either + * type-checks them or carries a TEST_DEBT entry with a measured error count. + * * Run: node scripts/check-type-check-coverage.mjs * Exit: 0 = OK, 1 = coverage regressed or the lists are stale */ @@ -58,9 +69,65 @@ const CHECKED_BY_OWN_BUILD = { }, }; +// ── Known gaps: tests that nothing type-checks ─────────────────────────────── +// Packages whose tests do not compile yet, so they cannot chain a +// `tsconfig.test.json` from `type-check`. Every entry is real debt: fix the +// errors, add the config, then delete the entry. +// +// Counts are MEASURED, not estimated — a temp tsconfig per package lifting only +// the test exclusion, against `main` at the time of the sweep, with the same +// template the wired-up packages use (`noEmit`, `composite: false`, `paths: {}`, +// plus `lib`/`types` where the tests need them). Errors resolvable in that +// config are excluded, so these are code-tier only. +// +// The two dominant codes are not random, and each has a playbook: +// - TS2741/TS2739 "missing required properties" — authoring fixtures typed as +// PARSED OUTPUT, whose `.default()` fields are required. Type them as the +// input surface (`z.input`), the fix objectstack#4074 applied in `types`. +// - TS2339/TS2353 "property does not exist" — implementation wider than the +// type, the dialect problem. Declare the dialect next to the vocabulary it +// extends (see scripts/check-spec-symbol-derivation.mjs), don't widen the +// type to silence it. +const TEST_DEBT = { + "@object-ui/core": { errors: 72, issue: 4118, note: "TS2741x32, TS2322x17 — mostly the input-vs-output fixture confusion" }, + "@object-ui/app-shell": { errors: 53, issue: 4118, note: "TS2339x24 — implementation wider than the type" }, + "@object-ui/components": { errors: 31, issue: 4118, note: "TS7006x12, TS7031x12 — untyped test callback params" }, + "@object-ui/react": { errors: 27, issue: 4118, note: "TS2769x9 — overload mismatch on render helpers" }, + "@object-ui/i18n": { errors: 13, issue: 4118, note: "TS2769x9" }, + "@object-ui/plugin-form": { errors: 12, issue: 4118, note: "TS2322x10" }, + "@object-ui/plugin-dashboard": { errors: 6, issue: 4118 }, + "@object-ui/plugin-list": { errors: 6, issue: 4118, note: "TS2353x3 — dialect keys" }, + "@object-ui/permissions": { errors: 5, issue: 4118, note: "TS2741x5" }, + "@object-ui/plugin-detail": { errors: 5, issue: 4118, note: "TS2353x3 — dialect keys" }, + "@object-ui/plugin-gantt": { errors: 3, issue: 4118 }, + "@object-ui/auth": { errors: 2, issue: 4118 }, + "@object-ui/plugin-chatbot": { errors: 2, issue: 4118 }, + "@object-ui/plugin-grid": { errors: 2, issue: 4118 }, + "@object-ui/plugin-map": { errors: 1, issue: 4118 }, +}; + // ── Collect workspace packages ─────────────────────────────────────────────── const GROUPS = ["packages", "apps", "examples"]; +function countTestFiles(dir) { + let n = 0; + let entries; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch { + return 0; + } + for (const entry of entries) { + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "dist") continue; + n += countTestFiles(join(dir, entry.name)); + } else if (/\.test\.tsx?$/.test(entry.name)) { + n++; + } + } + return n; +} + function collect() { const out = []; for (const group of GROUPS) { @@ -80,19 +147,36 @@ function collect() { } const pkg = JSON.parse(readFileSync(manifest, "utf8")); if (!pkg.name) continue; - let hasTsconfig = true; + let tsconfig = null; try { - statSync(resolve(root, dir, "tsconfig.json")); + tsconfig = readFileSync(resolve(root, dir, "tsconfig.json"), "utf8"); } catch { - hasTsconfig = false; + /* no tsconfig */ } + let testConfig = null; + try { + testConfig = readFileSync(resolve(root, dir, "tsconfig.test.json"), "utf8"); + } catch { + /* no test config */ + } + const typeCheck = pkg.scripts?.["type-check"] ?? ""; out.push({ name: pkg.name, dir, hasScript: Boolean(pkg.scripts?.["type-check"]), + typeCheck, build: pkg.scripts?.build, hasBuild: Boolean(pkg.scripts?.build), - hasTsconfig, + hasTsconfig: tsconfig !== null, + testFiles: countTestFiles(resolve(root, dir, "src")), + // A package tsconfig that never excludes a test glob compiles its tests + // as part of the build program, so they are already covered. + buildExcludesTests: tsconfig !== null && /\*\.test\./.test(tsconfig), + hasTestConfig: testConfig !== null, + testConfig, + // The config existing is not the same as anything running it — that gap + // IS objectui#3009. `type-check` has to chain it. + chainsTestConfig: /-p\s+tsconfig\.test\.json/.test(typeCheck), }); } } @@ -190,12 +274,97 @@ for (const [name, spec] of Object.entries(CHECKED_BY_OWN_BUILD)) { } } +// ── 5. Tests are code too (objectstack#4118) ───────────────────────────────── +// Only asked of packages that are type-checked at all: one whose whole package +// is unchecked is already covered by DEBT above, and stacking a second entry on +// it would just make the same gap look like two. +const testsCovered = (pkg) => !pkg.buildExcludesTests || (pkg.hasTestConfig && pkg.chainsTestConfig); + +for (const pkg of packages) { + if (!pkg.hasScript || pkg.testFiles === 0) continue; + + // 5a. A `tsconfig.test.json` nothing runs is the objectui#3009 shape exactly: + // a file that looks like enforcement while no `tsc` invocation reads it. + if (pkg.hasTestConfig && !pkg.chainsTestConfig) { + errors.push( + `${pkg.name} (${pkg.dir}) has a tsconfig.test.json that its "type-check" script never runs,\n` + + ` so the file looks like enforcement while nothing compiles it — the exact shape of\n` + + ` objectui#3009. Chain it: "type-check": "tsc --noEmit && tsc -p tsconfig.test.json"` + ); + continue; + } + + // 5b. A config that emits, or that does not actually include the tests, would + // pass 5a while checking nothing. + if (pkg.hasTestConfig && pkg.chainsTestConfig) { + if (!/"noEmit"\s*:\s*true/.test(pkg.testConfig)) { + errors.push( + `${pkg.name} (${pkg.dir}): tsconfig.test.json must set "noEmit": true — it is a checking\n` + + ` project, and emitting would put test output in the published dist.` + ); + } + if (!/\*\.test\./.test(pkg.testConfig)) { + errors.push( + `${pkg.name} (${pkg.dir}): tsconfig.test.json does not "include" any test glob, so it\n` + + ` compiles nothing. Add "include": ["src/**/*.test.ts", "src/**/*.test.tsx"]` + ); + } + continue; + } + + // 5c. Undeclared gap — tests exist and nothing reads them. + if (!testsCovered(pkg) && !TEST_DEBT[pkg.name]) { + errors.push( + `${pkg.name} (${pkg.dir}) has ${pkg.testFiles} test file${pkg.testFiles === 1 ? "" : "s"} that no \`tsc\`\n` + + ` invocation reads: its tsconfig.json excludes them (correctly — it is the build config)\n` + + ` and no tsconfig.test.json chains off "type-check". An unchecked test can assert a\n` + + ` contract the compiler never checked, and then read as evidence that the contract holds.\n` + + ` Add a tsconfig.test.json (see packages/types/tsconfig.test.json) and chain it, or add a\n` + + ` TEST_DEBT entry with the measured error count.` + ); + } +} + +// 6. Ratchet — a declared test gap that has been closed must leave the list. +for (const [name, spec] of Object.entries(TEST_DEBT)) { + const pkg = byName.get(name); + if (!pkg) { + errors.push(`${name} is listed in TEST_DEBT but is not a workspace package any more — delete the entry.`); + continue; + } + if (pkg.testFiles === 0) { + errors.push(`${name} is listed in TEST_DEBT but has no test files any more — delete the entry.`); + continue; + } + // A package that is not type-checked AT ALL is already declared in DEBT. A + // second entry here would make one gap read as two, and would survive the day + // DEBT is paid off, so the test gap has to be re-measured then anyway. + if (!pkg.hasScript) { + errors.push( + `${name} is listed in TEST_DEBT but has no "type-check" script, so its whole package is\n` + + ` unchecked and DEBT already declares that. Delete the TEST_DEBT entry; add it back with\n` + + ` a fresh measurement once the package type-checks at all.` + ); + continue; + } + if (testsCovered(pkg)) { + errors.push( + `${name} type-checks its tests now — delete its TEST_DEBT entry so the gap cannot reopen` + + `${spec.issue ? ` (and close #${spec.issue} if the list is empty)` : ""}.` + ); + } +} + // ── Report ─────────────────────────────────────────────────────────────────── const checked = packages.filter((p) => p.hasScript).length; const debtCount = Object.keys(DEBT).length; const debtErrors = Object.values(DEBT).reduce((sum, d) => sum + d.errors, 0); const byBuild = Object.keys(CHECKED_BY_OWN_BUILD).length; +const withTests = packages.filter((p) => p.hasScript && p.testFiles > 0); +const testsChecked = withTests.filter(testsCovered).length; +const testDebtErrors = Object.values(TEST_DEBT).reduce((sum, d) => sum + d.errors, 0); + if (errors.length === 0) { console.log( `✅ type-check coverage: ${checked}/${packages.length} via \`type-check\`, ` + @@ -203,6 +372,10 @@ if (errors.length === 0) { `${debtCount} known-broken (${debtErrors} errors outstanding), ` + `${NOT_COMPILED.length} not compiled.` ); + console.log( + `✅ test type-check coverage: ${testsChecked}/${withTests.length} packages compile their tests, ` + + `${Object.keys(TEST_DEBT).length} declared debt (${testDebtErrors} errors outstanding).` + ); process.exit(0); } @@ -212,6 +385,8 @@ for (const message of errors) { } console.error( "\nA package with no `type-check` script is not passing — turbo skips it and CI sees nothing.\n" + + "An excluded TEST file is not passing either — nothing compiles it, so what it asserts about\n" + + "a contract was never checked (objectstack#4118).\n" + "See https://github.com/objectstack-ai/objectui/issues/2911 for why this guard exists." ); process.exit(1); From a6df72760d5e317e918d4d7d093a35aa67b78827 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:39:07 +0000 Subject: [PATCH 3/6] feat(core): inventory ActionDef's keys and warn on the ones nothing reads (objectstack#4075 step 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ActionDef` ends with `[key: string]: any`, so it accepts any key of any type. Deleting `ActionDef.execute` produced ZERO compile errors even though the field had just been removed (objectui#2990), and stale metadata still authoring `execute: 'markDone'` type-checks today. The same deletion against `@object-ui/types`' `ActionSchema` — no index signature — correctly produced TS2353 at the authoring site. One reader can catch a retired key; the other is structurally incapable. Step 1 is the non-breaking half: make the key set visible, warn on anything outside it, change no types. What the inventory found, which is step 2's worklist: - 18 keys the SPEC owns that `ActionDef` never declared — `visible`, `locations`, `icon`, `variant`, `order`, `component`, `bulkEnabled`, `requiredPermissions` and 10 more. `ActionEngine` reads two of them through `as any` casts (`(ra.action as any).visible`, `(action as any).locations`); those casts exist only because the field is undeclared. - 17 keys `ActionDef` declares that the spec does not own — `actionType`, `api`, `chain`, `onClick`, `toast` and others, several already marked legacy. This is objectui's own dialect and needs the #4115 treatment: documented as deliberate, or retired. - `execute` is not simply gone from the spec — it is a live TOMBSTONE, still in `ActionSchema` so the parser can reject it BY NAME with the rename prescription. So it is warned about separately and more loudly than an unknown key: an unknown key is probably a typo, a retired key is metadata that used to work. - `to` / `external` / `newTab` / `replace` are a real objectui dialect — the `navigation` alias's own spelling, read off the action when no nested `navigate` object is present. Declared here rather than left to trip the warning, with a tripwire test that fails if the spec ever adopts one. The lists are data because TypeScript types do not survive to runtime, and `keyof ActionDef` cannot substitute — the index signature widens it to `string | number`, which is the problem itself. `actionKeys.pin.test.ts` re-derives each list from its real source (the interface via the AST, the spec via its schema) so a hand-maintained list cannot drift from what it mirrors. The spec key list is pinned rather than resolved at runtime: `ActionSchema` is a lazy proxy that does not forward `.shape`, so reading it means walking zod internals — fine in a test, wrong in shipped code. `@object-ui/types` made the same call for `ActionComponent`. Discrimination proof: dropping any entry from `ACTION_DEF_KEYS` fails with "ActionDef declares keys the inventory is missing: "; the warning tests assert it stays silent on a well-formed action, names `targt`, gives `execute` its rename prescription, warns once rather than per click, and says nothing in production. Core type-checks clean and all 207 existing action tests still pass. One pin is deliberately inverted: the suite asserts the index signature is STILL THERE. The day that fails, step 3 has landed, `tsc` catches unknown keys itself, and this warning plus `executeScript`'s rename branch can retire together. Refs objectstack#4075, objectstack#3903, objectui#2990, #3856, #3855, #2169, AGENTS.md PD #12 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy --- packages/core/src/actions/ActionRunner.ts | 7 + .../actions/__tests__/actionKeys.pin.test.ts | 170 ++++++++++++ packages/core/src/actions/actionKeys.ts | 250 ++++++++++++++++++ packages/core/src/actions/index.ts | 1 + 4 files changed, 428 insertions(+) create mode 100644 packages/core/src/actions/__tests__/actionKeys.pin.test.ts create mode 100644 packages/core/src/actions/actionKeys.ts diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index 3e1eaa20e..0a075f489 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -24,6 +24,7 @@ import type { RunnableActionType } from '@object-ui/types'; import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator'; import { globalUndoManager, type UndoableOperation } from './UndoManager'; +import { warnOnUnknownActionKeys } from './actionKeys'; export interface ActionResult { success: boolean; @@ -475,6 +476,12 @@ export class ActionRunner { async execute(action: ActionDef): Promise { try { + // `ActionDef` accepts any key of any type, so a typo (`targt`) and a + // retired key (`execute`) both reach here having type-checked. Neither + // binds a handler, and binding no handler silently is the #2169 "Mark Done + // does nothing" shape. Dev-only, warn-once, changes nothing (#4075 step 1). + warnOnUnknownActionKeys(action); + // Resolve the action type const actionType = action.type || action.actionType || action.name || ''; diff --git a/packages/core/src/actions/__tests__/actionKeys.pin.test.ts b/packages/core/src/actions/__tests__/actionKeys.pin.test.ts new file mode 100644 index 000000000..8f35db67b --- /dev/null +++ b/packages/core/src/actions/__tests__/actionKeys.pin.test.ts @@ -0,0 +1,170 @@ +/** + * Pins the key inventory in `actionKeys.ts` to the two things it mirrors — + * `ActionDef`'s own declarations and `@objectstack/spec`'s `ActionSchema` + * (objectstack#4075 step 1). + * + * Why a test rather than a comment: a hand-maintained list that silently drifts + * from the interface it claims to mirror is the exact "declared ≠ enforced" + * failure this work is about. `ActionDef` cannot be enumerated at runtime — its + * `[key: string]: any` widens `keyof` to `string | number` — so the list is data, + * and this file is what makes the data true. Adding a field to `ActionDef` + * without adding it here fails, by name. + * + * Discrimination proof for the guard below: with `ACTION_DEF_KEYS` complete these + * pass; dropping any single entry (e.g. `target`) fails with + * "ActionDef declares keys the inventory is missing: target". + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; +import ts from 'typescript'; +import * as SpecUI from '@objectstack/spec/ui'; +import { + ACTION_DEF_KEYS, + SPEC_ACTION_KEYS, + NAVIGATION_ALIAS_KEYS, + RETIRED_ACTION_KEYS, + KNOWN_ACTION_KEYS, + classifyActionKeys, + warnOnUnknownActionKeys, + resetActionKeyWarnings, +} from '../actionKeys'; + +const RUNNER = join(dirname(fileURLToPath(import.meta.url)), '..', 'ActionRunner.ts'); + +/** `ActionDef`'s declared property names, read off the interface itself. */ +function declaredActionDefKeys(): string[] { + const sf = ts.createSourceFile(RUNNER, readFileSync(RUNNER, 'utf8'), ts.ScriptTarget.Latest, true); + for (const stmt of sf.statements) { + if (!ts.isInterfaceDeclaration(stmt) || stmt.name.text !== 'ActionDef') continue; + return stmt.members + .filter(ts.isPropertySignature) + .map((m) => (m.name && (ts.isIdentifier(m.name) || ts.isStringLiteral(m.name)) ? m.name.text : null)) + .filter((n): n is string => n !== null); + } + throw new Error('ActionDef interface not found in ActionRunner.ts'); +} + +/** + * The spec's `ActionSchema` keys. `ActionSchema` is a lazy proxy that does not + * forward `.shape`, so this walks zod internals to reach the object shape — + * acceptable HERE (a test pins a fact) and deliberately not done in shipped code. + */ +function specActionKeys(): string[] { + const seen = new Set(); + const walk = (schema: unknown, depth = 0): string[] | null => { + if (!schema || depth > 8 || seen.has(schema)) return null; + seen.add(schema); + const s = schema as Record; + if (s.shape) return Object.keys(s.shape); + const def = s._def ?? s.def; + if (!def) return null; + if (def.shape) return Object.keys(def.shape); + for (const key of ['in', 'out', 'innerType', 'schema', 'left', 'right']) { + const found = def[key] ? walk(def[key], depth + 1) : null; + if (found) return found; + } + return null; + }; + const keys = walk(SpecUI.ActionSchema); + if (!keys) throw new Error('could not resolve @objectstack/spec/ui ActionSchema shape'); + return keys; +} + +describe('action key inventory (objectstack#4075 step 1)', () => { + it('ActionDef still has the index signature this inventory compensates for', () => { + // The day this fails, step 3 has landed: `tsc` catches unknown keys itself + // and the dev-mode warning (plus `executeScript`'s rename branch) can retire. + expect(readFileSync(RUNNER, 'utf8')).toContain('[key: string]: any'); + }); + + it('lists every key ActionDef declares', () => { + const declared = declaredActionDefKeys(); + const missing = declared.filter((k) => !(ACTION_DEF_KEYS as readonly string[]).includes(k)); + const stale = (ACTION_DEF_KEYS as readonly string[]).filter((k) => !declared.includes(k)); + expect({ missing, stale }).toEqual({ missing: [], stale: [] }); + }); + + it('lists every key the spec ActionSchema declares', () => { + const spec = specActionKeys(); + const missing = spec.filter((k) => !(SPEC_ACTION_KEYS as readonly string[]).includes(k)); + const stale = (SPEC_ACTION_KEYS as readonly string[]).filter((k) => !spec.includes(k)); + // `missing` means the spec grew a key objectui does not know about; `stale` + // means it dropped one. Either way the inventory has to be re-derived, and + // the diff names exactly which key moved. + expect({ missing, stale }).toEqual({ missing: [], stale: [] }); + }); + + it('`execute` is still a live spec tombstone, so it must not count as known', () => { + const parsed = SpecUI.ActionSchema.safeParse({ + name: 'mark_done', + label: 'Mark Done', + type: 'script', + execute: 'markDone', + }); + expect(parsed.success).toBe(false); + expect(RETIRED_ACTION_KEYS).toHaveProperty('execute'); + expect(KNOWN_ACTION_KEYS.has('execute')).toBe(false); + }); + + it('keeps the navigation alias out of the spec vocabulary it is not part of', () => { + // If the spec ever adopts one of these, it stops being objectui dialect and + // this fails — naming the alias to retire, the same tripwire shape as + // `ObjectUiLocalActionType`. + const spec = specActionKeys(); + expect(NAVIGATION_ALIAS_KEYS.filter((k) => spec.includes(k))).toEqual([]); + }); +}); + +describe('unknown-key warning', () => { + let warn: ReturnType; + + beforeEach(() => { + resetActionKeyWarnings(); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => warn.mockRestore()); + + it('says nothing about an action built only from recognized keys', () => { + warnOnUnknownActionKeys({ name: 'save', type: 'api', target: '/api/v1/save', locations: ['record_header'] }); + expect(warn).not.toHaveBeenCalled(); + }); + + it('names a typo that the compiler cannot see', () => { + // `targt` type-checks today: ActionDef's index signature accepts it. Nothing + // reads it, so the action runs and does nothing — #2169's shape. + warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'saveRecord' }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('`targt`'); + }); + + it('gives a retired key its rename prescription, not a bare "unknown"', () => { + warnOnUnknownActionKeys({ name: 'mark_done', type: 'script', execute: 'markDone' }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('rename the key to `target`'); + }); + + it('warns once per key, not once per click', () => { + for (let i = 0; i < 5; i++) warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'x' }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('is silent in production', () => { + const prev = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'x' }); + expect(warn).not.toHaveBeenCalled(); + } finally { + process.env.NODE_ENV = prev; + } + }); + + it('classifies unknown and retired keys separately', () => { + expect(classifyActionKeys({ type: 'script', execute: 'a', targt: 'b' })).toEqual({ + unknown: ['targt'], + retired: ['execute'], + }); + }); +}); diff --git a/packages/core/src/actions/actionKeys.ts b/packages/core/src/actions/actionKeys.ts new file mode 100644 index 000000000..d834d8a02 --- /dev/null +++ b/packages/core/src/actions/actionKeys.ts @@ -0,0 +1,250 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * What an action is allowed to carry — objectstack#4075 step 1. + * + * `ActionDef` ends with `[key: string]: any`, so it accepts any key of any type. + * Concretely (objectui#2990): deleting `ActionDef.execute` produced ZERO compile + * errors even though the field had just been removed, and stale metadata still + * authoring `execute: 'markDone'` type-checks today. The same deletion against + * `@object-ui/types`' `ActionSchema` — which has no index signature — correctly + * produced `TS2353` at the authoring site. One of the two readers can catch a + * retired key; the other is structurally incapable. + * + * That asymmetry is the issue. An open key set on a DECLARED METADATA CONTRACT + * is what lets a typo (`targt`, `exectue`) and a tombstoned key (`execute`) both + * sail through to a runner that then silently does nothing — the #2169 "Mark + * Done does nothing" shape. + * + * This module is step 1 of the staged narrowing: it makes the key set VISIBLE + * and warns on anything outside it, without changing a single type. Nothing + * breaks, and an invisible failure becomes an audible one. Steps 2 and 3 — + * promoting the legitimate keys to explicit optional fields, then removing the + * index signature — are what finally let `tsc` catch typos and retired keys. + * + * The lists are pinned by `__tests__/actionKeys.pin.test.ts`, which re-derives + * each one from its actual source. A hand-maintained list that drifts from the + * interface it mirrors would be the same "declared ≠ enforced" trap this whole + * thread is about. + * + * ── What the inventory found, i.e. step 2's worklist ───────────────────────── + * `ActionDef` declares 34 keys, the spec's `ActionSchema` declares 36, and they + * overlap on 17. The two halves of the difference are different problems: + * + * 18 keys the SPEC owns that `ActionDef` never declared — `ai`, `aria`, + * `bodyExtra`, `bodyShape`, `bulkEnabled`, `component`, `icon`, `locations`, + * `mode`, `objectName`, `order`, `recordIdField`, `recordIdParam`, + * `requiredPermissions`, `requiresFeature`, `shortcut`, `variant`, `visible`. + * These are authored today and reach readers through the index signature. + * `ActionEngine` reads two of them (`visible` at the location filter, `locations` + * at registration) through an `as any` cast — a cast that exists ONLY because the + * field is undeclared, and that goes away when step 2 promotes them. + * + * 17 keys `ActionDef` declares that the spec does not own — `actionType`, `api`, + * `chain`, `chainMode`, `close`, `condition`, `confirm`, `endpoint`, `modal`, + * `navigate`, `onClick`, `onFailure`, `onSuccess`, `redirect`, `reload`, `toast`, + * `actionParams`. Several are already marked legacy at their declaration. These + * are objectui's own dialect and need the #4115 treatment: kept and documented as + * deliberate, or retired — but not silently carried. + */ + +/** + * Every property `ActionDef` declares, in declaration order. + * + * Kept as data because TypeScript types do not survive to runtime — and + * `keyof ActionDef` cannot substitute for this list: the index signature widens + * it to `string | number`, which is precisely the problem being worked around. + */ +export const ACTION_DEF_KEYS = [ + 'type', + 'actionType', + 'name', + 'label', + 'confirmText', + 'confirm', + 'condition', + 'disabled', + 'api', + 'endpoint', + 'method', + 'navigate', + 'onClick', + 'reload', + 'close', + 'redirect', + 'toast', + 'successMessage', + 'errorMessage', + 'refreshAfter', + 'undoable', + 'params', + 'actionParams', + 'resultDialog', + 'target', + 'body', + 'openIn', + 'modal', + 'chain', + 'chainMode', + 'onSuccess', + 'onFailure', + 'opensInNewTab', + 'newTabUrl', +] as const; + +/** + * Every property `@objectstack/spec`'s `ActionSchema` declares. + * + * `ActionDef` mirrors that schema, so a key the spec owns is legitimate even + * when `ActionDef` has not declared it yet — several are read today only through + * an `as any` cast (`visible` and `locations` in `ActionEngine`), which is the + * cast this list exists to eventually retire. + * + * Pinned rather than read off the schema at runtime: the spec exports + * `ActionSchema` as a lazy proxy that does not forward `.shape`, so resolving it + * means walking zod internals (`_def.in` / `_def.innerType`). That is fine in a + * test and wrong in shipped code — `@object-ui/types` made the same call for + * `ActionComponent`. The pin test does the walk and fails the day this drifts. + */ +export const SPEC_ACTION_KEYS = [ + 'ai', + 'aria', + 'body', + 'bodyExtra', + 'bodyShape', + 'bulkEnabled', + 'component', + 'confirmText', + 'disabled', + 'errorMessage', + 'execute', + 'icon', + 'label', + 'locations', + 'method', + 'mode', + 'name', + 'newTabUrl', + 'objectName', + 'openIn', + 'opensInNewTab', + 'order', + 'params', + 'recordIdField', + 'recordIdParam', + 'refreshAfter', + 'requiredPermissions', + 'requiresFeature', + 'resultDialog', + 'shortcut', + 'successMessage', + 'target', + 'type', + 'undoable', + 'variant', + 'visible', +] as const; + +/** + * The `navigation` alias's own spelling of a target, read off the action itself + * when no nested `navigate` object is present (`ActionRunner.executeNavigation`). + * + * A declared objectui-side dialect, not spec vocabulary: `replace` is documented + * at its read site as "the one thing the `navigation` shape carries that + * `ActionSchema` has no field for". Listing it here is what keeps it from being + * silent dialect — the alternative is a warning that cries wolf on a shape the + * runner itself supports. + */ +export const NAVIGATION_ALIAS_KEYS = ['to', 'external', 'newTab', 'replace'] as const; + +/** + * Keys the spec has TOMBSTONED: still present in `ActionSchema` so the parser can + * reject them BY NAME with a rename prescription, rather than fail with a bare + * "unrecognized key". + * + * Warned about separately from unknown keys, and more loudly: an unknown key is + * probably a typo, while a retired key is metadata that used to work. That + * distinction is why `executeScript` carries a runtime branch returning the + * rename prescription — a branch that exists solely to compensate for the index + * signature, and that can retire with it in step 3. + */ +export const RETIRED_ACTION_KEYS: Readonly> = { + execute: '`execute` was removed in @objectstack/spec 17 (#3855) — rename the key to `target`. ' + + 'The value (a script name or expression) is unchanged. ' + + 'Run `os migrate meta --from 16` to rewrite it automatically.', +}; + +/** Every key an action may legitimately carry today. */ +export const KNOWN_ACTION_KEYS: ReadonlySet = new Set([ + ...ACTION_DEF_KEYS, + ...SPEC_ACTION_KEYS, + ...NAVIGATION_ALIAS_KEYS, +].filter((key) => !(key in RETIRED_ACTION_KEYS))); + +/** Split an action's own keys into the two things worth saying out loud. */ +export function classifyActionKeys(action: object | null | undefined): { + unknown: string[]; + retired: string[]; +} { + const unknown: string[] = []; + const retired: string[] = []; + if (!action || typeof action !== 'object') return { unknown, retired }; + for (const key of Object.keys(action)) { + if (key in RETIRED_ACTION_KEYS) retired.push(key); + else if (!KNOWN_ACTION_KEYS.has(key)) unknown.push(key); + } + return { unknown, retired }; +} + +// Warn once per key, not once per execution: an unrecognized key usually sits in +// metadata driving a button that gets clicked repeatedly, and a warning that +// floods the console is a warning that gets muted. +const warned = new Set(); + +/** Reset the warn-once memo. Exported for tests. */ +export function resetActionKeyWarnings(): void { + warned.clear(); +} + +const isDev = (): boolean => + (globalThis as { process?: { env?: Record } }).process?.env?.NODE_ENV !== + 'production'; + +/** + * Dev-mode only: name the keys an action carries that no reader will ever look + * at. Non-breaking by construction — it changes no types and rejects nothing. + * + * Silently binding no handler is the #2169 "Mark Done does nothing" shape, and + * the compiler cannot help while the index signature stands. Until it comes + * down, this is the audible half. + */ +export function warnOnUnknownActionKeys(action: object | null | undefined, where = 'ActionRunner'): void { + if (!isDev()) return; + const { unknown, retired } = classifyActionKeys(action); + const name = (action as { name?: unknown; type?: unknown })?.name ?? (action as { type?: unknown })?.type ?? '(unnamed)'; + + for (const key of retired) { + const memo = `retired:${key}`; + if (warned.has(memo)) continue; + warned.add(memo); + console.warn(`[${where}] action "${String(name)}" carries the retired key \`${key}\`. ${RETIRED_ACTION_KEYS[key]}`); + } + + if (unknown.length === 0) return; + const memo = `unknown:${unknown.slice().sort().join(',')}`; + if (warned.has(memo)) return; + warned.add(memo); + console.warn( + `[${where}] action "${String(name)}" carries ${unknown.length === 1 ? 'a key' : 'keys'} no reader ` + + `recognizes: \`${unknown.join('`, `')}\`. Nothing will read ${unknown.length === 1 ? 'it' : 'them'} — ` + + 'check for a typo, or promote the key to an explicit field on `ActionDef` ' + + '(packages/core/src/actions/actionKeys.ts). Warned rather than rejected because `ActionDef` ' + + 'still carries `[key: string]: any`, so the compiler cannot see this (objectstack#4075).', + ); +} diff --git a/packages/core/src/actions/index.ts b/packages/core/src/actions/index.ts index c3c845cb1..12088f266 100644 --- a/packages/core/src/actions/index.ts +++ b/packages/core/src/actions/index.ts @@ -8,6 +8,7 @@ export * from './ActionRunner.js'; export * from './ActionEngine.js'; +export * from './actionKeys.js'; export * from './TransactionManager.js'; export * from './UndoManager.js'; export * from './bulkFastPath.js'; From f422e6a03f4500d4ff0989756c4c70138a5bca60 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:40:30 +0000 Subject: [PATCH 4/6] chore(changeset): declare the core action-key inventory as a minor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also drops the last `no-explicit-any` lint warning from the pin test's zod walk — `Record` with a narrowing helper reads the same internals without the escape hatch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy --- ...n-key-inventory-and-unknown-key-warning.md | 37 +++++++++++++++++++ .../actions/__tests__/actionKeys.pin.test.ts | 10 +++-- 2 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 .changeset/action-key-inventory-and-unknown-key-warning.md diff --git a/.changeset/action-key-inventory-and-unknown-key-warning.md b/.changeset/action-key-inventory-and-unknown-key-warning.md new file mode 100644 index 000000000..3cc7afd24 --- /dev/null +++ b/.changeset/action-key-inventory-and-unknown-key-warning.md @@ -0,0 +1,37 @@ +--- +"@object-ui/core": minor +--- + +feat(core): inventory `ActionDef`'s keys and warn on the ones nothing reads — objectstack#4075 step 1 + +`ActionDef` ends with `[key: string]: any`, so it accepts any key of any type. +Deleting `ActionDef.execute` produced **zero** compile errors even though the +field had just been removed (objectui#2990), and stale metadata still authoring +`execute: 'markDone'` type-checks today. The same deletion against +`@object-ui/types`' `ActionSchema` — which has no index signature — correctly +produced `TS2353` at the authoring site. One of the two readers can catch a +retired key; the other is structurally incapable, which is how a typo (`targt`) +and a tombstoned key both reach a runner that then silently does nothing. + +This is the non-breaking first step of the staged narrowing: it makes the key set +**visible** and warns on anything outside it, without changing a single type. + +New exports from `@object-ui/core`: + +- `ACTION_DEF_KEYS`, `SPEC_ACTION_KEYS`, `NAVIGATION_ALIAS_KEYS`, + `RETIRED_ACTION_KEYS`, `KNOWN_ACTION_KEYS` — the inventory. +- `classifyActionKeys(action)` — splits an action's own keys into `unknown` and + `retired`. +- `warnOnUnknownActionKeys(action)` — dev-mode only, warn-once. Called by + `ActionRunner.execute`, so no consumer wiring is needed. + +A retired key gets a louder, more specific warning than an unknown one: an +unknown key is probably a typo, a retired key is metadata that used to work. +`execute` is not simply gone from the spec — it is a live **tombstone**, still +present in `ActionSchema` so the parser can reject it by name with the rename +prescription. + +Nothing is rejected and no types changed, so existing metadata behaves exactly as +before. Promoting the legitimate keys to explicit optional fields, then removing +the index signature so `tsc` catches both typos and retired keys, are steps 2 and +3 of objectstack#4075. diff --git a/packages/core/src/actions/__tests__/actionKeys.pin.test.ts b/packages/core/src/actions/__tests__/actionKeys.pin.test.ts index 8f35db67b..745ed8d19 100644 --- a/packages/core/src/actions/__tests__/actionKeys.pin.test.ts +++ b/packages/core/src/actions/__tests__/actionKeys.pin.test.ts @@ -56,11 +56,13 @@ function specActionKeys(): string[] { const walk = (schema: unknown, depth = 0): string[] | null => { if (!schema || depth > 8 || seen.has(schema)) return null; seen.add(schema); - const s = schema as Record; - if (s.shape) return Object.keys(s.shape); - const def = s._def ?? s.def; + const s = schema as Record; + const shapeOf = (v: unknown): string[] | null => + v && typeof v === 'object' ? Object.keys(v as object) : null; + if (s.shape) return shapeOf(s.shape); + const def = (s._def ?? s.def) as Record | undefined; if (!def) return null; - if (def.shape) return Object.keys(def.shape); + if (def.shape) return shapeOf(def.shape); for (const key of ['in', 'out', 'innerType', 'schema', 'left', 'right']) { const found = def[key] ? walk(def[key], depth + 1) : null; if (found) return found; From 08b83e045202af9b59644f54369805cf19a522e5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:44:11 +0000 Subject: [PATCH 5/6] chore(spec-check): write the shadcn skip path as a plain string Leftover template-string noise from drafting; `${"components"}/src/ui/` is just "components/src/ui/". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy --- scripts/check-spec-symbol-derivation.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/check-spec-symbol-derivation.mjs b/scripts/check-spec-symbol-derivation.mjs index d4d1928c1..a7b497212 100644 --- a/scripts/check-spec-symbol-derivation.mjs +++ b/scripts/check-spec-symbol-derivation.mjs @@ -296,7 +296,7 @@ const DEBT = { // Files under these paths are not objectui's own authored surface. // - `ui/` is the Shadcn no-touch zone (AGENTS.md #7): upstream 3rd-party files // overwritten by sync scripts, so a collision there is not ours to fix. -const SKIP_PATH_SEGMENTS = [`${"components"}/src/ui/`]; +const SKIP_PATH_SEGMENTS = ["components/src/ui/"]; const isSpecModule = (m) => m === "@objectstack/spec" || m.startsWith("@objectstack/spec/"); From f23dd45e6870a87b2c49d5f06c9da759cf0090c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:45:13 +0000 Subject: [PATCH 6/6] fix(core): warn once per action, not once per key-set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keying the warn-once memo on the unknown keys alone reported the FIRST action carrying `targt` and stayed silent about every other one — sending the author to fix a button that was only the first symptom. Keyed by action name too; the memo is bounded by the number of authored actions either way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy --- .../src/actions/__tests__/actionKeys.pin.test.ts | 11 ++++++++++- packages/core/src/actions/actionKeys.ts | 15 ++++++++++----- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/core/src/actions/__tests__/actionKeys.pin.test.ts b/packages/core/src/actions/__tests__/actionKeys.pin.test.ts index 745ed8d19..c9d695d02 100644 --- a/packages/core/src/actions/__tests__/actionKeys.pin.test.ts +++ b/packages/core/src/actions/__tests__/actionKeys.pin.test.ts @@ -147,11 +147,20 @@ describe('unknown-key warning', () => { expect(warn.mock.calls[0][0]).toContain('rename the key to `target`'); }); - it('warns once per key, not once per click', () => { + it('warns once per action, not once per click', () => { for (let i = 0; i < 5; i++) warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'x' }); expect(warn).toHaveBeenCalledTimes(1); }); + it('still names a SECOND action carrying the same typo', () => { + // Keying the warn-once memo on the keys alone would report `save` and stay + // silent about `remove` — sending the author to fix the first symptom only. + warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'x' }); + warnOnUnknownActionKeys({ name: 'remove', type: 'script', targt: 'y' }); + expect(warn).toHaveBeenCalledTimes(2); + expect(warn.mock.calls[1][0]).toContain('"remove"'); + }); + it('is silent in production', () => { const prev = process.env.NODE_ENV; process.env.NODE_ENV = 'production'; diff --git a/packages/core/src/actions/actionKeys.ts b/packages/core/src/actions/actionKeys.ts index d834d8a02..0d52ec650 100644 --- a/packages/core/src/actions/actionKeys.ts +++ b/packages/core/src/actions/actionKeys.ts @@ -202,9 +202,14 @@ export function classifyActionKeys(action: object | null | undefined): { return { unknown, retired }; } -// Warn once per key, not once per execution: an unrecognized key usually sits in -// metadata driving a button that gets clicked repeatedly, and a warning that -// floods the console is a warning that gets muted. +// Warn once per (action, problem), not once per execution: an unrecognized key +// usually sits in metadata driving a button that gets clicked repeatedly, and a +// warning that floods the console is a warning that gets muted. +// +// Keyed by action name as well as by the keys, deliberately. Keying on the keys +// alone would report the FIRST action carrying `targt` and stay silent about +// every other one — sending the author to fix a button that was only the first +// symptom. The memo is bounded by the number of authored actions either way. const warned = new Set(); /** Reset the warn-once memo. Exported for tests. */ @@ -230,14 +235,14 @@ export function warnOnUnknownActionKeys(action: object | null | undefined, where const name = (action as { name?: unknown; type?: unknown })?.name ?? (action as { type?: unknown })?.type ?? '(unnamed)'; for (const key of retired) { - const memo = `retired:${key}`; + const memo = `retired:${String(name)}:${key}`; if (warned.has(memo)) continue; warned.add(memo); console.warn(`[${where}] action "${String(name)}" carries the retired key \`${key}\`. ${RETIRED_ACTION_KEYS[key]}`); } if (unknown.length === 0) return; - const memo = `unknown:${unknown.slice().sort().join(',')}`; + const memo = `unknown:${String(name)}:${unknown.slice().sort().join(',')}`; if (warned.has(memo)) return; warned.add(memo); console.warn(