diff --git a/.changeset/action-forward-whitelist-parity-4050-4192.md b/.changeset/action-forward-whitelist-parity-4050-4192.md new file mode 100644 index 000000000..111d9f9b2 --- /dev/null +++ b/.changeset/action-forward-whitelist-parity-4050-4192.md @@ -0,0 +1,13 @@ +--- +'@object-ui/components': patch +--- + +An action rendered in the overflow menu, as an icon or inside a group now reaches the runner carrying the same authored keys as the same action rendered inline — `action:menu`, `action:icon` and `action:group` forward `label` and `description`, and the two group/icon surfaces also forward `resultDialog`. + +Every action renderer hands the `ActionRunner` an explicit key WHITELIST rather than the action itself. That is deliberate — a key no renderer honours must not look wired — but the whitelists had drifted, and which renderer a given action gets is decided by `action:bar`'s `maxVisible` split (3 on desktop, 1 on mobile) and by `systemActions`, which are always in the overflow menu. So the same declared action behaved differently depending on the viewport. + +`label` and `description` are what the console's param-collection handler titles its dialog from (`title: action?.label || action?.title`, `description: actionDescription(…, action?.description)`). Dropped, an action with declared `params` opened a dialog titled "Action parameters" while the SAME declaration rendered inline named itself "Create Environment". `resultDialog` is the one-shot reveal spec (a fresh 2FA code, a newly minted OAuth secret): dropped, the runner falls back to the success toast and the value the user was meant to copy is gone — the objectui#3646 defect, still live on two of the four declared surfaces. + +`undoable` and `recordIdField` are deliberately NOT added. Both are read only under a `rowRecord` guard, and `rowRecord` is `params._rowRecord`, written exclusively by the spread-based hosts (`DeclaredActionsBar`, `RelatedRecordActionsBridge`, `ObjectGrid`, `page:header`), none of which dispatch through these renderers. They are unreachable on this path rather than dropped — `action:button` forwards them here inertly — so forwarding them would have added a second inert copy instead of restoring an affordance. + +A new repo gate, `pnpm check:action-forward-parity`, now derives each surface's owed key set (`authorable ∩ runtime-read − retired`) from the spec's own schemas and the consumers' ASTs and fails when a renderer drops one, so the seventh instance of this class fails on the pull request that introduces it rather than shipping green. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e3b080971..4e267f57a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -189,6 +189,20 @@ jobs: if: steps.relevant.outputs.should_run == 'true' run: pnpm check:spec-symbols + # Each action renderer hands the runner an explicit key WHITELIST, so a new + # spec action key is silently dropped until five separate lists are edited — + # and nothing fails while they are not: the key parses, publishes and reads + # as honoured. Six instances were found by hand, one at a time + # (objectstack#6837 `bodyExtra`, #6938 `bodyShape`, objectui#3646 + # `resultDialog`, objectui#4192 `label`/`description`), which is why + # objectui#4050's ruling asks for a gate rather than a seventh review. Same + # placement rationale as the step above: it reads `@objectstack/spec`'s own + # zod shapes and parses the renderers with `typescript`, so it needs the + # install but nothing built. + - name: Verify action renderers forward every key the runtime reads + if: steps.relevant.outputs.should_run == 'true' + run: pnpm check:action-forward-parity + # A key a component asks `t()` for must exist in the `en` pack. # `all-locales-key-parity.test.ts` compares packs to EACH OTHER, so ten # packs identically missing a key is full parity and full parity is green diff --git a/content/docs/guide/ci-cd-pipeline.md b/content/docs/guide/ci-cd-pipeline.md index 2c5e8b196..465caf3e8 100644 --- a/content/docs/guide/ci-cd-pipeline.md +++ b/content/docs/guide/ci-cd-pipeline.md @@ -169,7 +169,7 @@ it green — which is how two of `type-check`'s gates came to be missing from th | Job key | Appears as | What it runs | When | |---|---|---|---| | `changeset-check` | Changeset Fixed Group Check | `scripts/check-changeset-fixed.mjs` — every workspace package must be in the changeset `fixed` group or explicitly ignored. It checks group *membership*; it does **not** check whether the PR added a changeset. | Every run | -| `type-check` | Type Check | `scripts/check-type-check-coverage.mjs`, then `pnpm check:spec-symbols`, then `pnpm check:i18n-keys`, then `pnpm check:i18n-drift`, then `pnpm type-check:scripts`, then `pnpm type-check`, then `pnpm type-check:vitest-setup`. The coverage guard runs first because turbo silently skips packages that have no `type-check` script, so a package without one would otherwise read as passing (#2911). The two locale gates sit in the middle because both parse the sources with `typescript`: they need the install and nothing built. `pnpm check:i18n-keys` fails when a `t()` call site asks for a key the `en` pack does not define ([#3530](https://github.com/objectstack-ai/objectui/issues/3530)); `pnpm check:i18n-drift` fails when a change to an `en` string is not accompanied by the nine translation packs ([#3650](https://github.com/objectstack-ai/objectui/issues/3650)), and it is why this job's checkout sets `fetch-depth: 0` — it diffs against the merge base, which a depth-1 clone cannot resolve. `pnpm type-check:scripts` (`tsconfig.scripts.json`) covers `scripts/**/*.ts`, which `pnpm type-check` cannot reach at all — `scripts/` has no package.json, so turbo never walks it, and the coverage guard decides coverage per *package*. Until [#3494](https://github.com/objectstack-ai/objectui/issues/3494) that left the pin tests in `scripts/__tests__/` — including the one pinning this very page — compiled by nothing. `pnpm type-check:vitest-setup` (`tsconfig.vitest-setup.json`) closes the same gap for the four repo-root `vitest.setup.*` files, uncovered until [#3515](https://github.com/objectstack-ai/objectui/issues/3515); it runs *last*, after `pnpm type-check`, because `vitest.setup.dom.tsx` side-effect-imports four `@object-ui/*` packages and resolves them through the declarations that turbo's `^build` produces. | Every run; on a PR the steps short-circuit when only ignored paths changed | +| `type-check` | Type Check | `scripts/check-type-check-coverage.mjs`, then `pnpm check:spec-symbols`, then `pnpm check:action-forward-parity`, then `pnpm check:i18n-keys`, then `pnpm check:i18n-drift`, then `pnpm type-check:scripts`, then `pnpm type-check`, then `pnpm type-check:vitest-setup`. The coverage guard runs first because turbo silently skips packages that have no `type-check` script, so a package without one would otherwise read as passing (#2911). `pnpm check:action-forward-parity` fails when an action renderer's forward whitelist drops a key the action runtime reads — the class that shipped six times one key at a time, each time green, because the key parses and publishes while the payload is dropped one hop before the runner ([#4050](https://github.com/objectstack-ai/objectui/issues/4050)). The two locale gates sit in the middle because both parse the sources with `typescript`: they need the install and nothing built. `pnpm check:i18n-keys` fails when a `t()` call site asks for a key the `en` pack does not define ([#3530](https://github.com/objectstack-ai/objectui/issues/3530)); `pnpm check:i18n-drift` fails when a change to an `en` string is not accompanied by the nine translation packs ([#3650](https://github.com/objectstack-ai/objectui/issues/3650)), and it is why this job's checkout sets `fetch-depth: 0` — it diffs against the merge base, which a depth-1 clone cannot resolve. `pnpm type-check:scripts` (`tsconfig.scripts.json`) covers `scripts/**/*.ts`, which `pnpm type-check` cannot reach at all — `scripts/` has no package.json, so turbo never walks it, and the coverage guard decides coverage per *package*. Until [#3494](https://github.com/objectstack-ai/objectui/issues/3494) that left the pin tests in `scripts/__tests__/` — including the one pinning this very page — compiled by nothing. `pnpm type-check:vitest-setup` (`tsconfig.vitest-setup.json`) closes the same gap for the four repo-root `vitest.setup.*` files, uncovered until [#3515](https://github.com/objectstack-ai/objectui/issues/3515); it runs *last*, after `pnpm type-check`, because `vitest.setup.dom.tsx` side-effect-imports four `@object-ui/*` packages and resolves them through the declarations that turbo's `^build` produces. | Every run; on a PR the steps short-circuit when only ignored paths changed | | `test` | Test (shard N/4) | `pnpm test --shard=N/4` across a 4-runner matrix with `fail-fast: false`, so every shard reports its own failures. No coverage instrumentation — v8 adds 40–100% overhead. | Pull requests and merge-queue builds (everything but `push`); steps short-circuit on a PR that changed only ignored paths | | `test-coverage` | Test (coverage) | One unsharded `pnpm test:coverage`, uploaded to Codecov. Nothing blocks on it, which is why it is not sharded. | **Push only** | | `e2e` | Build & E2E | Builds the console with `vite build` (`VITE_BASE_PATH=/console/`), verifies the artifact, then `pnpm test:e2e --project=chromium`. Uploads the Playwright report on failure. | Every run; on a PR the steps short-circuit when only ignored paths changed | diff --git a/package.json b/package.json index 9d9245721..23d26dc96 100644 --- a/package.json +++ b/package.json @@ -36,6 +36,7 @@ "type-check:scripts": "tsc -p tsconfig.scripts.json", "type-check:vitest-setup": "tsc -p tsconfig.vitest-setup.json", "check:spec-symbols": "node scripts/check-spec-symbol-derivation.mjs", + "check:action-forward-parity": "node scripts/check-action-forward-parity.mjs", "check:control-bytes": "node scripts/check-control-bytes.mjs", "check:i18n-keys": "node scripts/check-i18n-call-site-keys.mjs", "check:i18n-drift": "node scripts/check-i18n-en-drift.mjs", diff --git a/packages/components/src/renderers/action/__tests__/action-forward-parity.test.tsx b/packages/components/src/renderers/action/__tests__/action-forward-parity.test.tsx new file mode 100644 index 000000000..6d33f41e2 --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-forward-parity.test.tsx @@ -0,0 +1,164 @@ +/** + * 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. + */ + +/** + * objectui#4192 — the same declared action, rendered inline or in the overflow + * menu, must reach the runner carrying the same authored keys. + * + * Which renderer an action gets is decided by `action:bar`'s `maxVisible` split + * (3 desktop, 1 mobile) and by `systemActions`, which are ALWAYS in the menu. So + * a whitelist that has drifted between the two renderers makes the same + * declaration behave differently by VIEWPORT WIDTH — the class of divergence + * #4162 and objectui#4075 each hit on a different key set. + * + * `label` and `description` are the measured instance. The console's param + * collection handler titles its dialog from exactly those two — + * `title: action?.label || action?.title` and + * `description: actionDescription(…, action?.description)` + * (useConsoleActionRuntime.tsx:205-207) — so an overflow action opened an + * untitled dialog while the SAME declaration, rendered inline, named itself. + * + * ## Why this file exists alongside `scripts/check-action-forward-parity.mjs` + * + * The gate (objectui#4050) reads the whitelists statically: it proves the key + * appears in the `execute({…})` literal. It cannot prove the value SURVIVES the + * hop — that the runner receives it and that the handler which titles the dialog + * sees it. These pins drive the real renderers through the real runner and + * assert on what the handler is actually handed, so the two halves fail for + * different reasons: delete a key from the whitelist and both go red; break the + * runner's param-collection dispatch and only this file does. + * + * ## Reachability, deliberately NOT pinned here + * + * `undoable` and `recordIdField` are absent from these payloads on purpose, and + * asserting they arrive would pin a fiction. Both are read only under a + * `rowRecord` guard, and `rowRecord` is `params._rowRecord` — written by the + * spread-based hosts (`DeclaredActionsBar`, `RelatedRecordActionsBridge`, + * `ObjectGrid`, `page:header`), never by these renderers. `action:button` + * forwards them INERTLY on this path; the menu omitting them costs nothing. + * That verdict is carried, with its evidence, in the gate's JUSTIFIED table. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, waitFor } from '@testing-library/react'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import { ActionProvider } from '@object-ui/react'; +// Module-scope side-effect imports — `action:bar` resolves its members and the +// overflow menu through the ComponentRegistry at render time, and the light +// `dom` project does not load the `@object-ui/components` graph. Module scope, +// not a `beforeAll`, per AGENTS.md §测试纪律. +import '../action-bar'; +import '../action-button'; +import '../action-menu'; + +/** Three ordinary actions — enough to fill the desktop `maxVisible: 3`. */ +const FILLERS = [ + { name: 'a1', label: 'a1', type: 'api', locations: ['list_toolbar'] }, + { name: 'a2', label: 'a2', type: 'api', locations: ['list_toolbar'] }, + { name: 'a3', label: 'a3', type: 'api', locations: ['list_toolbar'] }, +]; + +/** + * The card's action. `params` is an ARRAY — a parameter DEFINITION list — so the + * runner opens param collection before executing, which is the path whose dialog + * was going untitled. `autoTrigger` runs `handleExecute`, the identical function + * a click on the menu item calls, without opening the Radix dropdown (whose + * pointerdown-driven portal is flaky to synthesize in happy-dom — see + * `action-group-dropdown-visible.test.tsx`). + */ +const CREATE = { + name: 'create_environment', + label: 'Create Environment', + description: 'Provision a new environment for this project', + type: 'api', + locations: ['list_toolbar'], + params: [{ name: 'title', type: 'text', label: 'Title' }], + autoTrigger: true, +}; + +let api: ReturnType; +let onParamCollection: ReturnType; + +beforeEach(() => { + api = vi.fn(async () => ({ success: true })); + // Cancel collection: this pin is about what the handler is HANDED, and + // cancelling keeps the assertion off everything downstream of the dialog. + onParamCollection = vi.fn(async () => null); +}); + +function Bar({ actions }: { actions: any[] }) { + const C = ComponentRegistry.get('action:bar'); + if (!C) throw new Error('action:bar is not registered'); + // eslint-disable-next-line react-hooks/static-components -- ComponentRegistry.get returns a registered renderer (stable reference), not a component created during render + return ; +} + +const renderBar = (actions: any[]) => + render( + + + , + ); + +/** The action object the param-collection handler was handed. */ +const collectedAction = () => onParamCollection.mock.calls[0]?.[1] as any; + +describe('action:menu forwards the keys that title a param dialog (#4192)', () => { + it('the overflow path hands the dialog the action`s own label and description', async () => { + // Four actions, `maxVisible: 3` → the create action is 4th, so `action:bar` + // routes it to `action:menu`. Before the fix the handler received + // `label: undefined, description: undefined` here and the console titled + // the dialog "Action parameters". + renderBar([...FILLERS, CREATE]); + + await waitFor(() => expect(onParamCollection).toHaveBeenCalledTimes(1)); + expect(collectedAction().label).toBe('Create Environment'); + expect(collectedAction().description).toBe('Provision a new environment for this project'); + }); + + it('is indistinguishable from the inline path — the split must not decide the title', async () => { + // The SAME declaration with nothing to overflow past: `action:bar` keeps it + // inline and it renders through `action:button`. Both renderers must hand + // the handler the same two keys, or the dialog's title becomes a function of + // viewport width. + renderBar([CREATE]); + + await waitFor(() => expect(onParamCollection).toHaveBeenCalledTimes(1)); + const inline = collectedAction(); + expect(inline.label).toBe('Create Environment'); + expect(inline.description).toBe('Provision a new environment for this project'); + }); + + it('still routes an array `params` as the collection definition, not as a payload', async () => { + // The menu passes `params: action.params` where the button routes an array + // to `actionParams`; the runner accepts either (ActionRunner.ts:816), which + // is why #4192 was a wrong TITLE and not a missing dialog. Pinned so the + // divergence stays latent rather than becoming the next defect. + renderBar([...FILLERS, CREATE]); + + await waitFor(() => expect(onParamCollection).toHaveBeenCalledTimes(1)); + const paramDefs = onParamCollection.mock.calls[0][0] as any[]; + expect(paramDefs).toHaveLength(1); + expect(paramDefs[0].name).toBe('title'); + }); + + it('carries the label through to the runner itself, not only to the dialog', async () => { + // No `params`, so nothing to collect and the action runs: the `api` handler + // receives the ActionDef the menu composed. Asserting here as well as at the + // dialog keeps the pin honest if param collection is ever restructured. + const { name, type, locations, label, description } = CREATE; + renderBar([...FILLERS, { name, type, locations, label, description, autoTrigger: true }]); + + await waitFor(() => expect(api).toHaveBeenCalledTimes(1)); + const def = api.mock.calls[0][0] as any; + expect(def.name).toBe('create_environment'); + expect(def.label).toBe('Create Environment'); + expect(def.description).toBe('Provision a new environment for this project'); + }); +}); diff --git a/packages/components/src/renderers/action/action-group.tsx b/packages/components/src/renderers/action/action-group.tsx index 544009b62..db533b560 100644 --- a/packages/components/src/renderers/action/action-group.tsx +++ b/packages/components/src/renderers/action/action-group.tsx @@ -239,6 +239,11 @@ const ActionGroupRenderer = forwardRef( await execute({ type: schema.type, name: schema.name, + // See action-button.tsx — the param-collection dialog reads its title + // and description off these (objectui#4192, measured on `action:menu` + // and found here by `check:action-forward-parity`). + label: schema.label, + description: (schema as any).description, target: schema.target, openIn: (schema as any).openIn, endpoint: schema.endpoint, @@ -87,6 +92,10 @@ const ActionIconRenderer = forwardRef( // Placement declaration — see action-button.tsx (#2210). locations: (schema as any).locations, toast: schema.toast, + // See action-button.tsx — the one-shot reveal spec (2FA setup, fresh + // OAuth secret). Without it the runner falls back to the success + // toast and the value the user was meant to copy is gone. + resultDialog: (schema as any).resultDialog, ...localContext, }); } finally { diff --git a/packages/components/src/renderers/action/action-menu.tsx b/packages/components/src/renderers/action/action-menu.tsx index adbc1fcd5..7ab54ae94 100644 --- a/packages/components/src/renderers/action/action-menu.tsx +++ b/packages/components/src/renderers/action/action-menu.tsx @@ -216,6 +216,16 @@ const ActionMenuRenderer = forwardRef contents`, relative to the synthetic root. */ + readonly files: Record; +} + +/** Builds a throwaway tree and runs the REAL `analyze()` over it. */ +function withRepo(fixture: Fixture, run: (root: string) => T): T { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'check-action-forward-parity-')); + try { + for (const [rel, contents] of Object.entries(fixture.files)) { + const full = path.join(dir, rel); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, contents); + } + return run(dir); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +const UI_VIEW = 'packages/types/src/ui-action.ts'; +const KEYS_MODULE = 'packages/core/src/actions/actionKeys.ts'; +const CONSUMER = 'packages/core/src/actions/ActionRunner.ts'; +const RENDERER = 'packages/components/src/renderers/action/action-x.tsx'; + +/** A synthetic repo whose only variable is the renderer's forward payload. */ +function repoWith( + forwards: string, + overrides: Partial> = {}, +): Fixture { + return { + files: { + [UI_VIEW]: + overrides.view ?? + 'export interface ActionSchema {\n name?: string;\n description?: string;\n}\n', + [KEYS_MODULE]: + overrides.keys ?? "export const RETIRED_ACTION_KEYS = {\n execute: 'renamed to target',\n};\n", + [CONSUMER]: + overrides.consumer ?? + [ + 'export function run(action: any, list: any[]) {', + ' const { target } = action;', + ' if (action.undoable && action.description) return target;', + ' return list.filter((a) => a.locations).map((a) => a.icon);', + '}', + ].join('\n'), + [RENDERER]: `export const R = () => { void execute(${forwards}); };\n`, + }, + }; +} + +const declaredSurface = [{ id: 'action:x', contract: 'declared', file: RENDERER }]; +const inlineSurface = [{ id: 'action:x', contract: 'inline', file: RENDERER }]; + +/** `analyze` over a fixture, with the fixture's own tables. */ +function judge( + fixture: Fixture, + options: Record = {}, +): { errors: string[]; report: { owed: string[]; forwarded: string[]; unexcused: string[] }[] } { + return withRepo(fixture, (root) => + analyze(root, { + spec: SPEC, + surfaces: declaredSurface, + consumers: [{ file: CONSUMER, binding: 'action' }], + justified: {}, + knownGaps: {}, + opaqueSpreads: {}, + uiActionView: UI_VIEW, + actionKeysModule: KEYS_MODULE, + ...options, + }), + ); +} + +// -- the owed set ------------------------------------------------------------- + +describe('owed = authorable ∩ runtime-read − retired', () => { + it('is green when the payload carries every owed key', () => { + const { errors, report } = judge(repoWith('{ target, undoable, description }')); + expect(errors).toEqual([]); + // `name`/`type`/`bodyShape`/`objectName` are authorable but never read here; + // `locations`/`icon` are read off a DIFFERENT binding. Neither is owed. + expect(report[0].owed).toEqual(['description', 'target', 'undoable']); + }); + + it('names the surface and the dropped key when one is missing', () => { + const { errors } = judge(repoWith('{ target, description }')); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('action:x'); + expect(errors[0]).toContain('`undoable`'); + expect(errors[0]).toContain(RENDERER); + }); + + it('counts a key the runtime reads but nobody can author as the runner`s own mechanic', () => { + // `chain` is read off the def and is in no authorable vocabulary: forwarding + // it is nobody's job. This is the half objectstack#6975 called underivable — + // a grep over the consumer would have reported it as owed. + const fixture = repoWith('{ target, undoable, description }', { + consumer: [ + 'export function run(action: any) {', + ' if (action.chain) return action.chain;', + ' const { target } = action;', + ' return action.undoable && action.description ? target : null;', + '}', + ].join('\n'), + }); + const { errors, report } = judge(fixture); + expect(report[0].owed).not.toContain('chain'); + expect(errors).toEqual([]); + }); + + it('never owes a retired key, even when it is authorable and still read', () => { + const fixture = repoWith('{ target, undoable, description }', { + consumer: [ + 'export function run(action: any) {', + ' const { target } = action;', + ' if (action.execute) return action.execute;', + ' return action.undoable && action.description ? target : null;', + '}', + ].join('\n'), + keys: "export const RETIRED_ACTION_KEYS = {\n execute: 'renamed to target',\n};\n", + }); + // Authorable in the fixture's declared vocabulary AND read — owed but for the + // tombstone, which makes authoring it a parse rejection rather than a drop. + const { errors, report } = judge(fixture, { spec: { ...SPEC, declared: [...SPEC.declared, 'execute'] } }); + expect(report[0].owed).not.toContain('execute'); + expect(errors).toEqual([]); + }); + + it('gives an inline surface the narrower vocabulary — the `element:button` split, derived', () => { + // The same tree, the same payload. Declared owes `undoable`; inline does not + // have the key in its vocabulary at all, so it is green without an exemption + // — objectstack#6975 expected this to need a registered exception. + const fixture = repoWith('{ target }'); + expect(judge(fixture, { surfaces: declaredSurface }).errors).toHaveLength(1); + const inline = judge(fixture, { surfaces: inlineSurface }); + expect(inline.errors).toEqual([]); + expect(inline.report[0].owed).toEqual(['target']); + }); + + it('reads the renderer view as authorable too, not only the spec shape', () => { + // `description` is declared by `@object-ui/types`' renderer view and by no + // spec schema — and it is half of objectui#4192. A gate reading only the + // spec would call it unowed and stay green on the real defect. + const fixture = repoWith('{ target, undoable }'); + const { errors } = judge(fixture); + expect(errors[0]).toContain('`description`'); + }); +}); + +// -- binding scope ------------------------------------------------------------ + +describe('reads are scoped to the binding the def arrives as', () => { + it('ignores property reads off a different identifier in the same file', () => { + // The distinction objectstack#6975 measured as impossible: `a.locations` is + // read off the AUTHORED list (to decide what renders), `action.target` off + // the FORWARDED def. Only the second is a forwarding contract, and grep sees + // one set. + const { union } = withRepo(repoWith('{ target }'), (root) => + runtimeReadKeys(root, [{ file: CONSUMER, binding: 'action' }]), + ); + expect([...union].sort()).toEqual(['description', 'target', 'undoable']); + expect(union.has('locations')).toBe(false); + expect(union.has('icon')).toBe(false); + }); + + it('collects destructured reads as well as property access', () => { + const { union } = withRepo( + repoWith('{ target }', { + consumer: 'export function run(action: any) {\n const { toast: t, openIn } = action;\n return [t, openIn];\n}\n', + }), + (root) => runtimeReadKeys(root, [{ file: CONSUMER, binding: 'action' }]), + ); + expect([...union].sort()).toEqual(['openIn', 'toast']); + }); +}); + +// -- the two registries ------------------------------------------------------- + +describe('JUSTIFIED and KNOWN_GAPS are ratcheted, not free passes', () => { + const dropping = repoWith('{ target, description }'); // drops `undoable` + + it('a justified omission is excused', () => { + const { errors } = judge(dropping, { + justified: { 'action:x:undoable': { reason: 'unreachable behind a guard', issue: 1 } }, + }); + expect(errors).toEqual([]); + }); + + it('a known gap is excused', () => { + const { errors } = judge(dropping, { + knownGaps: { 'action:x:undoable': { reason: 'filed, not fixed here', issue: 1 } }, + }); + expect(errors).toEqual([]); + }); + + it('a JUSTIFIED entry that excuses nothing is itself a failure', () => { + // The registry cannot outlive the code it excuses — objectstack#6975's + // objection was that the registry becomes the drift-prone list one level up. + const { errors } = judge(repoWith('{ target, undoable, description }'), { + justified: { 'action:x:undoable': { reason: 'stale — the key is forwarded now', issue: 1 } }, + }); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('excuses nothing'); + expect(errors[0]).toContain('action:x:undoable'); + }); + + it('a KNOWN_GAPS entry that excuses nothing is itself a failure', () => { + const { errors } = judge(repoWith('{ target, undoable, description }'), { + knownGaps: { 'action:x:undoable': { reason: 'stale — closed', issue: 4202 } }, + }); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('no longer a gap'); + expect(errors[0]).toContain('#4202'); + }); + + it('an OPAQUE_SPREADS entry matching no spread is itself a failure', () => { + const { errors } = judge(repoWith('{ target, undoable, description }'), { + opaqueSpreads: { 'action:x:ghost': { reason: 'nothing spreads this' } }, + }); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('matches no spread'); + }); +}); + +// -- extraction failure is red, never a silent pass --------------------------- + +describe('extraction failure throws rather than returning a clean verdict', () => { + it('an unresolvable spread is not treated as an empty object', () => { + // The silent-pass shape the ruling forbids: a spread may carry ANY key, so + // reading it as `{}` would let a renderer forward nothing and pass. + expect(() => judge(repoWith('{ target, ...mystery }'))).toThrow(ExtractionError); + expect(() => judge(repoWith('{ target, ...mystery }'))).toThrow(/cannot resolve the spread/); + }); + + it('but an OPAQUE_SPREADS entry declares one whose source cannot carry action keys', () => { + const { errors } = judge(repoWith('{ target, undoable, description, ...mystery }'), { + opaqueSpreads: { 'action:x:mystery': { reason: 'the host context bag, not the action' } }, + }); + expect(errors).toEqual([]); + }); + + it('resolves a conditional payload fragment and unions BOTH branches', () => { + // `action:button` routes params through exactly this shape; counting only one + // branch would report a forwarded key as dropped. + const fixture: Fixture = { + files: { + ...repoWith('{}').files, + [RENDERER]: + 'export const R = () => {\n' + + ' const frag = cond ? { undoable } : { description };\n' + + ' void execute({ target, ...frag });\n' + + '};\n', + }, + }; + const { errors, report } = judge(fixture); + expect(report[0].forwarded).toEqual(['description', 'target', 'undoable']); + expect(errors).toEqual([]); + }); + + it('a moved or non-literal forward site is red, not "nothing owed"', () => { + expect(() => judge(repoWith('payload'))).toThrow(/expected exactly one/); + expect(() => judge({ files: { ...repoWith('{}').files, [RENDERER]: 'export const R = () => {};\n' } })).toThrow( + /expected exactly one/, + ); + }); + + it('a split surface with two forward sites is red — each payload needs its own entry', () => { + const fixture: Fixture = { + files: { + ...repoWith('{}').files, + [RENDERER]: 'export const R = () => {\n void execute({ target });\n void execute({ undoable });\n};\n', + }, + }; + expect(() => judge(fixture)).toThrow(/found 2/); + }); + + it('a renamed binding is red, not a runtime that reads nothing', () => { + // The whole-gate vacuity case: an empty read set makes every owed set empty + // and every surface green forever. + const fixture = repoWith('{ target }', { + consumer: 'export function run(def: any) {\n return def.target;\n}\n', + }); + expect(() => judge(fixture)).toThrow(/no reads of/); + }); + + it('a consumer that moved is red, not a silently smaller owed set', () => { + expect(() => + judge(repoWith('{ target, undoable, description }'), { + consumers: [ + { file: CONSUMER, binding: 'action' }, + { file: 'packages/app-shell/src/hooks/gone.tsx', binding: 'action' }, + ], + }), + ).toThrow(/does not exist/); + }); + + it('a renderer view with no properties is red', () => { + expect(() => judge(repoWith('{ target }', { view: 'export interface ActionSchema {}\n' }))).toThrow( + /declares no properties/, + ); + }); + + it('a renamed renderer view is red', () => { + expect(() => judge(repoWith('{ target }', { view: 'export interface Other { name?: string }\n' }))).toThrow( + /not found/, + ); + }); + + it('a missing RETIRED_ACTION_KEYS is red — a tombstone would be reported as owed', () => { + expect(() => judge(repoWith('{ target }', { keys: 'export const OTHER = {};\n' }))).toThrow( + /RETIRED_ACTION_KEYS/, + ); + }); + + it('an empty owed set is red — a vacuously green surface is the #4690 shape', () => { + // Authorable and runtime-read do not intersect at all: nothing would ever be + // checked for this surface again. + expect(() => + judge(repoWith('{ target }', { view: 'export interface ActionSchema {\n nothingReadsThis?: string;\n}\n' }), { + spec: { declared: ['norThis'], inline: [] }, + }), + ).toThrow(/owed set is empty/); + }); +}); + +// -- the real repository ------------------------------------------------------ + +describe('the real repository', () => { + const real = analyze(repoRoot); + + it('is green', () => { + expect(real.errors).toEqual([]); + }); + + it('measures a non-empty owed set for every surface — nothing is vacuously green', () => { + expect(real.report).toHaveLength(SURFACES.length); + for (const r of real.report) { + expect(r.owed.length, `${r.surface.id} owes nothing`).toBeGreaterThan(5); + expect(r.forwarded.length, `${r.surface.id} forwards nothing`).toBeGreaterThan(5); + } + }); + + it('reads a plausible number of keys off the real consumers', () => { + expect(RUNTIME_CONSUMERS.length).toBeGreaterThan(2); + expect(real.runtimeRead.size).toBeGreaterThan(20); + for (const consumer of RUNTIME_CONSUMERS) { + expect(real.perFile.get(consumer.file)?.size ?? 0, `${consumer.file} read nothing`).toBeGreaterThan(0); + } + }); + + it('every JUSTIFIED and KNOWN_GAPS entry carries a reason and an issue', () => { + // An entry without one is indistinguishable from the drift being gated. + for (const [key, meta] of [...Object.entries(JUSTIFIED), ...Object.entries(KNOWN_GAPS)]) { + expect(meta.reason.length, `${key} has no reason`).toBeGreaterThan(80); + expect(typeof meta.issue, `${key} cites no issue`).toBe('number'); + } + for (const [key, meta] of Object.entries(OPAQUE_SPREADS)) { + expect(meta.reason.length, `${key} has no reason`).toBeGreaterThan(40); + } + }); + + it('holds objectui#4192: action:menu owes AND forwards the two keys that title a param dialog', () => { + const menu = real.report.find((r) => r.surface.id === 'action:menu'); + expect(menu, 'action:menu is no longer a surface').toBeDefined(); + for (const key of ['label', 'description']) { + expect(menu?.owed, `action:menu no longer owes ${key}`).toContain(key); + expect(menu?.forwarded, `action:menu dropped ${key} again`).toContain(key); + } + }); + + it('holds the inline contract: element:button is green on the narrower vocabulary', () => { + const inline = real.report.find((r) => r.surface.id === 'element:button'); + expect(inline?.owed.length).toBeGreaterThan(5); + expect(inline?.unexcused).toEqual([]); + // Derived, not exempted — the point of the surface-class split. + expect(Object.keys(JUSTIFIED).some((k) => k.startsWith('element:button:'))).toBe(false); + }); + + it('the real extractors read the real files', () => { + expect(retiredKeys(repoRoot)).toContain('execute'); + expect(uiActionViewKeys(repoRoot)).toContain('description'); + for (const surface of SURFACES) { + expect(forwardedKeys(repoRoot, surface).size, `${surface.id} forwards nothing`).toBeGreaterThan(5); + } + }); +}); + +// -- wiring ------------------------------------------------------------------- + +describe('the gate is wired to run', () => { + const pkg = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')) as { + scripts: Record; + }; + const ci = fs.readFileSync(path.join(repoRoot, '.github/workflows/ci.yml'), 'utf8'); + + it('package.json exposes it as a named script', () => { + expect(pkg.scripts['check:action-forward-parity']).toBe('node scripts/check-action-forward-parity.mjs'); + }); + + it('ci.yml runs it after the install it needs (it resolves @objectstack/spec)', () => { + const install = ci.indexOf('pnpm install --frozen-lockfile'); + const step = ci.indexOf('run: pnpm check:action-forward-parity'); + expect(step, 'ci.yml does not run `pnpm check:action-forward-parity`').toBeGreaterThan(-1); + expect(step, 'the check runs before dependencies are installed').toBeGreaterThan(install); + }); + + it('is not hidden from the renderers by ci.yml path filters', () => { + // ci.yml `paths-ignore`s markdown, content/, docs/, apps/site/ and + // .changeset/. None can match `packages/components/src/renderers/**`, so a PR + // that edits a forward whitelist always starts this workflow. + const ignored = (ci.slice(0, ci.indexOf('jobs:')).match(/^\s+- '.*'$/gm) ?? []).map((line) => + line.trim().replace(/^- '/, '').replace(/'$/, ''), + ); + expect(ignored.length, 'the paths-ignore parse found nothing — this pin would be vacuous').toBeGreaterThan(3); + + /** `**` crosses separators, `*` does not — enough for the five patterns above. */ + const matches = (glob: string, file: string): boolean => + new RegExp( + `^${glob + .split('**') + .map((part) => part.replace(/[.+^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '[^/]*')) + .join('.*')}$`, + ).test(file); + + for (const surface of SURFACES) { + for (const glob of ignored) { + expect(matches(glob, surface.file), `${glob} hides ${surface.file}`).toBe(false); + } + // The matcher itself must be able to say yes, or the loop above proves + // nothing — the vacuity trap this whole file is about. + expect(matches('**/*.md', 'content/docs/guide/ci-cd-pipeline.md')).toBe(true); + } + }); +}); diff --git a/scripts/check-action-forward-parity.mjs b/scripts/check-action-forward-parity.mjs new file mode 100644 index 000000000..ef02368bc --- /dev/null +++ b/scripts/check-action-forward-parity.mjs @@ -0,0 +1,780 @@ +#!/usr/bin/env node +/** + * An action renderer must forward every authored key the runtime will read. + * + * The failure class (objectui#4050, carried from objectstack#6975): each action + * renderer hands the `ActionRunner` an explicit key WHITELIST rather than the + * action itself. That is deliberate — a key no renderer honours must not look + * wired — but its cost is that a NEW key stays invisible until five separate + * lists are edited, and **nothing fails while they are not**. The key parses, + * publishes, and reads as honoured: + * + * bodyExtra objectstack#6837, objectui PR #3924 — payload dropped one hop + * before the runner; the action POSTed nothing. + * bodyShape objectstack#6938, objectui PR #3932 — a declared + * `{ wrap: 'data' }` sent a flat body. + * resultDialog objectui#3646 — the one-shot reveal (2FA code, fresh OAuth + * secret) fell back to a toast and the value was lost. + * openIn / locations / undoable — each carries an in-comment note at its + * forward site recording its own instance. + * label / description objectui#4192 — `action:menu`'s param dialog titled + * itself "Action parameters" instead of naming the action. THIS + * gate found the same omission on `action:icon` and + * `action:group`, which #4192 had not measured. + * + * Six of the seven were found by a human reading the lists side by side. This + * is the gate that reads them instead (maintainer ruling, objectui#4050, + * 2026-08-10: "Diff each action renderer's forward whitelist against the keys + * the runtime actually reads … A new spec action key missing from a whitelist + * must be a red check, not a silent drop; extraction failure is red, never a + * silent pass"). + * + * ── Why this is derivable, when objectstack#6975 argued it was not ─────────── + * #6975 stalled on one half of the diff: "the keys the runtime reads" is not + * mechanically derivable, because `git grep 'action\.'` over the runner and the + * console handlers returns `action.name` / `action.api` / `action.method` hits + * with no way to separate "body-path key a renderer must forward" from + * "mechanic the runner resolves itself". That is true OF GREP. Two changes make + * it derivable: + * + * 1. Read the sites with the compiler API, not with grep — property accesses + * and destructurings bound to the ActionDef parameter, per file. `grep` + * cannot tell `action.locations` (read off the AUTHORED list, to decide + * which actions render) from `action.target` (read off the FORWARDED def, + * at execute time); binding-scoped AST extraction can, because they are + * different bindings in different functions. + * 2. INTERSECT that with what the surface may be AUTHORED with. A key the + * runtime reads but no author can write is a runner mechanic (`api`, + * `chain`, `actionType`, `navigate`) and is nobody's to forward. A key the + * author can write AND the runtime reads is exactly the forwardable set. + * + * owed(surface) = authorable(surface) ∩ runtime-read − retired + * + * Both inputs come from their real declarations, so neither can be a stale hand + * copy: `authorable` from `@objectstack/spec`'s own zod shapes plus (for + * declared surfaces) `@object-ui/types`' renderer VIEW of an action, and + * `runtime-read` from the consumers' ASTs. + * + * The surface-class split #6975 flagged — `element:button` deliberately omits + * `bodyShape` because its contract is spec's `InlineActionSchema` pick list, + * not `ActionSchema` — is therefore DERIVED here rather than registered: an + * inline surface gets an inline owed set, so it is green without an exemption. + * That is one fewer hand-maintained list than #6975 feared this gate would need. + * + * ── The justified-omission registry ────────────────────────────────────────── + * What is left over is real: some omissions are load-bearing and CORRECT, and + * #6975's objection to a mechanical diff was precisely that it would report + * them. Two things cannot be read off an AST: + * + * - REACHABILITY UNDER A GUARD. `recordIdParam`, `recordIdField` and + * `undoable` are each read only inside `if (rowRecord && …)`, and + * `rowRecord` is `params._rowRecord` — written ONLY by the spread-based + * hosts, never by these five renderers. The keys are unreachable here, not + * dropped. + * - CONSUMPTION AT THE RENDERER. `disabled` is evaluated by each renderer to + * grey its own control, and `onClick` is invoked by `action:menu` directly. + * A key the renderer itself honours is not one it owes the runner. + * + * So {@link JUSTIFIED} is the declared table for those, one entry per + * (surface, key), each carrying the evidence that made the call — the same + * governance as `check-spec-symbol-derivation.mjs`'s ALLOW: declared, reasoned, + * and RATCHETED. An entry that excuses nothing (the key became owed-and- + * forwarded, or stopped being owed) fails this guard, so the table cannot + * outlive the code it excuses — which is the answer to #6975's "the registry is + * itself the drift-prone list, one level up". + * + * {@link KNOWN_GAPS} is the other half, and the distinction matters: a + * JUSTIFIED entry says "correctly omitted", a KNOWN_GAPS entry says "really + * dropped, filed, not fixed here". Same ratchet, same shrink-only rule as + * `check-spec-symbol-derivation.mjs`'s DEBT map, and for the same reason — this + * gate exists to stop the BLEEDING (a new key missing from a whitelist fails on + * the PR that adds it), not to retro-fix every pre-existing drop at once. + * + * ── Direction ──────────────────────────────────────────────────────────────── + * One-way, deliberately: `owed − forwarded − excused = ∅`. Forwarding a key + * nothing reads is not an error here. It is harmless at runtime (the runner + * ignores it), and the opposite rule would fail every renderer that forwards a + * key defensively — `locations`, for one, which is read only off the AUTHORED + * action by the hosts that decide placement, never off the forwarded def. + * + * ── Shape ──────────────────────────────────────────────────────────────────── + * Exported pure functions over an injectable `root`, with the CLI half behind + * `invokedDirectly` — the same shape as `check-control-bytes.mjs` and + * `check-changeset-presence.mjs`, and for the same reason: a gate whose failure + * paths nothing exercises is the objectui#4690 anti-pattern one level up. Every + * red below is driven from a synthetic repo in + * `scripts/__tests__/check-action-forward-parity.test.ts`. + * + * Extraction failures THROW {@link ExtractionError} rather than returning a + * verdict: a gate that cannot read its inputs has no verdict to give, and + * returning "no errors" there would be the silent pass the ruling forbids. + * + * Run: node scripts/check-action-forward-parity.mjs + * Exit: 0 = every surface forwards what it owes, 1 = a key is dropped, an entry + * is stale, or extraction failed. + */ + +import ts from "typescript"; +import { createRequire } from "module"; +import { readFileSync, existsSync } from "fs"; +import { resolve, dirname } from "path"; +import { fileURLToPath } from "url"; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = resolve(HERE, ".."); + +/** A gate that cannot read its inputs. Never a pass — see the header. */ +export class ExtractionError extends Error { + constructor(message) { + super(message); + this.name = "ExtractionError"; + } +} + +const fail = (message) => { + throw new ExtractionError(message); +}; + +const readFile = (root, rel) => readFileSync(resolve(root, rel), "utf8"); +const parse = (root, rel) => + ts.createSourceFile(rel, readFile(root, rel), ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + +// ── The surfaces ───────────────────────────────────────────────────────────── +// Every renderer that composes an explicit ActionDef payload and hands it to +// `execute(...)`. `contract` picks the authorable half of the owed set: +// +// declared — the renderer receives a full action (spec `ActionSchema`, seen +// through `@object-ui/types`' renderer VIEW of it). +// inline — the renderer receives spec's `InlineActionSchema` pick list, a +// deliberately narrower vocabulary (objectstack#6837's PR pinned +// this: `element:button`'s list mirrors that pick list field for +// field). +// +// Hosts that spread the whole action (`{...action}`) are NOT surfaces: they +// cannot drop a key by construction, which is the entire failure being gated. +// `DeclaredActionsBar`, `RelatedRecordActionsBridge`, `ObjectGrid` and +// `page:header`'s `dispatchHeaderAction` are all that shape. +export const SURFACES = [ + { id: "action:button", contract: "declared", file: "packages/components/src/renderers/action/action-button.tsx" }, + { id: "action:icon", contract: "declared", file: "packages/components/src/renderers/action/action-icon.tsx" }, + { id: "action:group", contract: "declared", file: "packages/components/src/renderers/action/action-group.tsx" }, + { id: "action:menu", contract: "declared", file: "packages/components/src/renderers/action/action-menu.tsx" }, + { id: "element:button", contract: "inline", file: "packages/components/src/renderers/basic/elements.tsx" }, +]; + +// ── The runtime consumers ──────────────────────────────────────────────────── +// Where a forwarded ActionDef is actually read. `binding` is the parameter the +// def arrives as; reads are collected for that identifier only, which is what +// keeps `objectDef.actions.filter(a => a.locations…)` — a read off the AUTHORED +// list, in the same files — out of the set. +export const RUNTIME_CONSUMERS = [ + { file: "packages/core/src/actions/ActionRunner.ts", binding: "action" }, + { file: "packages/app-shell/src/hooks/useConsoleActionRuntime.tsx", binding: "action" }, + { file: "packages/app-shell/src/views/RecordDetailView.tsx", binding: "action" }, +]; + +// ── Justified omissions ────────────────────────────────────────────────────── +// Key format: ":". Every entry states WHY the omission is correct +// and cites the evidence, because an entry without one is indistinguishable +// from the drift this gate exists to catch. Ratcheted: an entry that no longer +// excuses a real omission fails below. +export const JUSTIFIED = { + // ── Unreachable: read only under `rowRecord` ──────────────────────────────── + // `rowRecord` is `params._rowRecord`, and the ONLY writers are the four + // spread-based hosts — DeclaredActionsBar.tsx:215, + // RelatedRecordActionsBridge.tsx:164, ObjectGrid.tsx:1721 and :2034, and + // page:header's dispatchHeaderAction (containers.tsx:1287-1289). None of them + // dispatch THROUGH these renderers; they compose their own payload and call + // `execute` directly. DeclaredActionsBar.tsx:100 says so in prose: it "injects + // the record under `params._rowRecord` — which `action:button` does NOT do". + // + // These renderers forward `params: schema.params`, i.e. the AUTHORED params, + // and `_rowRecord` is a host stash — `isAuthoredParamKey` excludes it by its + // `_` prefix (packages/core/src/actions/actionKeys.ts:326-327). So the guard + // cannot hold on this path and the key is unreachable, not dropped. + ...Object.fromEntries( + [ + ["recordIdParam", "action:button", "action:icon", "action:group", "action:menu"], + ["recordIdField", "action:icon", "action:group", "action:menu"], + ["undoable", "action:icon", "action:group", "action:menu"], + ].flatMap(([key, ...surfaces]) => + surfaces.map((surface) => [ + `${surface}:${key}`, + { + reason: + `Unreachable on this surface, not dropped. \`${key}\` is read only under a ` + + "`rowRecord` guard — useConsoleActionRuntime.tsx:297 " + + "(`if (rowRecord && action.recordIdParam)`), :377 " + + "(`rowRecord?.[action.recordIdField || 'id']`) and :398 " + + "(`action.undoable && obj && recId && rowRecord && …`) — and `rowRecord` is " + + "`params._rowRecord`, written only by the spread-based hosts listed above, " + + "none of which dispatch through this renderer. objectstack#6938 made the same " + + "reachability call for `recordIdParam`; this gate's own measurement extended it " + + "to the two siblings behind the same guard (objectui#4192 had read the omission " + + "as a live Undo loss — it is not: `action:button` forwards `undoable` and " + + "`recordIdField` on this path INERTLY, for want of the same `rowRecord`).", + issue: 4192, + }, + ]) + ) + ), + + // ── Consumed at the renderer ─────────────────────────────────────────────── + ...Object.fromEntries( + ["action:button", "action:icon", "action:group", "action:menu"].map((surface) => [ + `${surface}:disabled`, + { + reason: + "Consumed HERE, not owed to the runner. Every one of these renderers evaluates " + + "`disabled` itself (through `hasDeclaredVisibilityGate` + `useCondition`) and greys " + + "its own control, so the click the runner's own gate would refuse " + + "(ActionRunner.ts:773) cannot be made in the first place. Forwarding it would put " + + "the same predicate through a second evaluator on a path that is already closed " + + "(objectui#3842 ruling on the declared-gate definition, applied by #3849).", + issue: 4050, + }, + ]) + ), + "action:menu:onClick": { + reason: + "Consumed HERE. `handleExecute` invokes `action.onClick()` directly and returns before " + + "reaching `execute` (action-menu.tsx:212) — the documented UI-local escape hatch that " + + "bypasses the ActionEngine. A key the renderer honours itself is not one it owes the " + + "runner.", + issue: 4050, + }, + + // ── Read only to improve a diagnostic ────────────────────────────────────── + ...Object.fromEntries( + ["action:button", "action:icon", "action:group", "action:menu"].map((surface) => [ + `${surface}:body`, + { + reason: + "Forwarding it could not change an outcome. The client runner cannot execute a spec " + + "`body` AT ALL — it reads the key at ActionRunner.ts:1038 for one purpose, to replace " + + '"no script provided" with the error naming the real cause ("Action body must be ' + + 'executed server-side … register a `script` handler"). So the omission costs a better ' + + "message on an action that fails either way, never a behaviour. Forwarded by none of " + + "the five surfaces; revisit if a client-side body executor ever lands.", + issue: 4050, + }, + ]) + ), +}; + +// ── Known gaps ─────────────────────────────────────────────────────────────── +// Real drops, filed, deliberately NOT fixed here — same shrink-only governance +// as check-spec-symbol-derivation.mjs's DEBT map. Ratcheted below: a gap that +// has been closed must be deleted from this table, or it silently re-reserves +// the key for the next drop. +export const KNOWN_GAPS = { + ...Object.fromEntries( + ["action:button", "action:icon", "action:group", "action:menu"].map((surface) => [ + `${surface}:objectName`, + { + reason: + "An action declaring its own `objectName` (a related-list row action retargeting a " + + "CHILD object) is dropped by all four declared surfaces, so the console handler falls " + + "back to the page's object — useConsoleActionRuntime.tsx:370 " + + "(`action.objectName || objApiName`), :151 and :180 (the i18n scope for the param " + + "dialog's labels). Pre-existing on every surface, so it is not this PR's regression " + + "and not its fix.", + issue: 4202, + }, + ]) + ), + ...Object.fromEntries( + ["action:button", "action:icon", "action:group"].map((surface) => [ + `${surface}:onClick`, + { + reason: + "The UI-local escape hatch is honoured by `action:menu` (which calls it, see JUSTIFIED " + + "above) and by nothing else: these three neither invoke nor forward it, so a " + + "code-composed `onClick` is silently inert. Not authorable in metadata (it is a " + + "function), which is why it stayed invisible. Pre-existing; filed with `objectName`.", + issue: 4202, + }, + ]) + ), +}; + +// ── Opaque spreads ─────────────────────────────────────────────────────────── +// A spread this gate cannot resolve to a literal key set is EXTRACTION FAILURE +// and fails — a spread could carry anything, so treating it as empty would be +// the silent pass the ruling forbids. An entry here declares a spread whose +// source is known not to carry authored action keys. +export const OPAQUE_SPREADS = { + "action:button:localContext": { + reason: + "The renderer's own `context` PROP (`{ schema, className, context: localContext, … }`), a " + + "host-supplied runtime context bag spread last — not the authored action, so it cannot " + + "carry an authored spec key and cannot satisfy or violate the owed-set diff.", + }, + "action:icon:localContext": { + reason: "Same `context` prop as action:button — see that entry.", + }, +}; + +// ── 1. authorable(surface) ─────────────────────────────────────────────────── +/** + * A spec zod schema's own shape keys. + * + * `ActionSchema` is exported as a lazy proxy that does not forward `.shape`, so + * this walks zod internals to reach the object shape — the same walk + * `packages/core/src/actions/__tests__/actionKeys.pin.test.ts` does, and + * acceptable for the same reason: a gate pins a fact, shipped code must not. + */ +export function specShapeKeys(schema, label) { + const seen = new Set(); + const walk = (node, depth = 0) => { + if (!node || depth > 8 || seen.has(node)) return null; + seen.add(node); + const s = node; + const shapeOf = (v) => (v && typeof v === "object" ? Object.keys(v) : null); + if (s.shape) return shapeOf(s.shape); + const def = s._def ?? s.def; + if (!def) return null; + 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; + } + return null; + }; + const keys = walk(schema); + if (!keys || keys.length === 0) { + fail( + `could not resolve \`${label}\`'s shape from @objectstack/spec/ui.\n` + + " The schema's internal representation changed. Fix the walk — falling back to a\n" + + " hardcoded key list would make this gate the stale copy it exists to prevent." + ); + } + return keys; +} + +/** `{ declared, inline }` — the two authorable vocabularies, from the spec itself. */ +export function loadSpecSchemas(require = createRequire(import.meta.url)) { + let ui; + try { + ui = require("@objectstack/spec/ui"); + } catch { + fail( + "cannot resolve @objectstack/spec/ui — run `pnpm install` first.\n" + + " This gate reads the spec's own schemas; it has no hardcoded fallback by design." + ); + } + for (const name of ["ActionSchema", "InlineActionSchema"]) { + if (!ui[name]) fail(`@objectstack/spec/ui no longer exports \`${name}\` — the owed set cannot be derived.`); + } + return { + declared: specShapeKeys(ui.ActionSchema, "ActionSchema"), + inline: specShapeKeys(ui.InlineActionSchema, "InlineActionSchema"), + }; +} + +/** + * `@object-ui/types`' renderer VIEW of an action. + * + * Declared surfaces are typed against this, not against the spec schema + * directly — it carries renderer-only fields the spec does not model + * (`description`, `enabled`, `size`, …) while importing the spec-owned parts it + * shares. Unioned into the declared owed set because a key an author can write + * on THIS type and the runtime reads is just as forwardable as a spec one: + * `description` is exactly that, and is half of objectui#4192. + */ +export const UI_ACTION_VIEW = "packages/types/src/ui-action.ts"; +export function uiActionViewKeys(root, file = UI_ACTION_VIEW) { + if (!existsSync(resolve(root, file))) { + fail(`the renderer action view ${file} does not exist — re-point this gate at it.`); + } + const sf = parse(root, file); + for (const stmt of sf.statements) { + if (!ts.isInterfaceDeclaration(stmt) || stmt.name.text !== "ActionSchema") continue; + const keys = stmt.members + .filter(ts.isPropertySignature) + .map((m) => (m.name && (ts.isIdentifier(m.name) || ts.isStringLiteral(m.name)) ? m.name.text : null)) + .filter((n) => n !== null); + if (keys.length === 0) fail(`\`ActionSchema\` in ${file} declares no properties — extraction failed.`); + return keys; + } + fail( + `\`interface ActionSchema\` not found in ${file}.\n` + + " The renderer view moved or was renamed; re-point this gate at it." + ); +} + +/** + * Keys the spec keeps as TOMBSTONES so the parser can reject them by name. + * Read off `RETIRED_ACTION_KEYS` rather than listed here — a second copy would + * drift, and a tombstone is never owed (authoring it is a parse rejection). + */ +export const ACTION_KEYS_MODULE = "packages/core/src/actions/actionKeys.ts"; +export function retiredKeys(root, file = ACTION_KEYS_MODULE) { + if (!existsSync(resolve(root, file))) { + fail(`${file} does not exist — without it a tombstoned key would be reported as owed.`); + } + const sf = parse(root, file); + let found = null; + const visit = (node) => { + if ( + ts.isVariableDeclaration(node) && + ts.isIdentifier(node.name) && + node.name.text === "RETIRED_ACTION_KEYS" && + node.initializer && + ts.isObjectLiteralExpression(node.initializer) + ) { + found = node.initializer.properties + .map((p) => (p.name && (ts.isIdentifier(p.name) || ts.isStringLiteral(p.name)) ? p.name.text : null)) + .filter((n) => n !== null); + } + ts.forEachChild(node, visit); + }; + visit(sf); + if (!found || found.length === 0) { + fail( + `\`RETIRED_ACTION_KEYS\` not found (or empty) in ${file}.\n` + + " Without it a tombstoned key would be reported as owed." + ); + } + return found; +} + +// ── 2. runtime-read ────────────────────────────────────────────────────────── +/** + * Property names read off the ActionDef binding in each consumer. + * + * Binding-scoped on purpose: `RecordDetailView.tsx` reads `a.locations` off the + * AUTHORED action list a few hundred lines from where it reads `action.target` + * off the forwarded def, and only the second is a forwarding contract. Grep + * cannot tell them apart, which is what objectstack#6975 measured and called + * non-derivable. + */ +export function runtimeReadKeys(root, consumers = RUNTIME_CONSUMERS) { + const perFile = new Map(); + for (const consumer of consumers) { + if (!existsSync(resolve(root, consumer.file))) { + fail( + `runtime consumer ${consumer.file} does not exist.\n` + + " A consumer that moved silently shrinks the owed set — re-point this gate at it." + ); + } + const sf = parse(root, consumer.file); + const keys = new Set(); + const visit = (node) => { + if ( + ts.isPropertyAccessExpression(node) && + ts.isIdentifier(node.expression) && + node.expression.text === consumer.binding + ) { + keys.add(node.name.text); + } + // `action!.label` + if ( + ts.isPropertyAccessExpression(node) && + ts.isNonNullExpression(node.expression) && + ts.isIdentifier(node.expression.expression) && + node.expression.expression.text === consumer.binding + ) { + keys.add(node.name.text); + } + // `const { target, method } = action` + if ( + ts.isVariableDeclaration(node) && + node.initializer && + ts.isIdentifier(node.initializer) && + node.initializer.text === consumer.binding && + node.name && + ts.isObjectBindingPattern(node.name) + ) { + for (const el of node.name.elements) { + const p = el.propertyName ?? el.name; + if (ts.isIdentifier(p)) keys.add(p.text); + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + if (keys.size === 0) { + fail( + `no reads of \`${consumer.binding}.*\` found in ${consumer.file}.\n` + + " Either the binding was renamed or the consumer changed shape. An empty read set\n" + + " would make every owed set empty and this whole gate vacuously green — which is\n" + + " the #4690 anti-pattern the ruling names." + ); + } + perFile.set(consumer.file, keys); + } + const union = new Set(); + for (const keys of perFile.values()) for (const k of keys) union.add(k); + return { union, perFile }; +} + +// ── 3. forwarded(surface) ──────────────────────────────────────────────────── +/** + * The keys of the object literal a surface passes to `execute(...)`. + * + * Conditional payload fragments are resolved and UNIONED: `action:button` and + * `element:button` both route params through + * `const paramsPayload = Array.isArray(…) ? { actionParams } : { params }` and + * spread it, so both branches count as forwarded — the runner accepts either as + * the param-collection definition (ActionRunner.ts:816). + */ +export function forwardedKeys(root, surface, opaqueSpreads = OPAQUE_SPREADS) { + if (!existsSync(resolve(root, surface.file))) { + fail(`surface ${surface.id}: ${surface.file} does not exist — re-point this gate at the renderer.`); + } + const sf = parse(root, surface.file); + + // Local `const X = …` initializers, for resolving spreads. + const locals = new Map(); + const collectLocals = (node) => { + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.initializer) { + locals.set(node.name.text, node.initializer); + } + ts.forEachChild(node, collectLocals); + }; + collectLocals(sf); + + const unwrap = (node) => { + let n = node; + while (n && (ts.isAsExpression(n) || ts.isParenthesizedExpression(n) || ts.isSatisfiesExpression?.(n))) { + n = n.expression; + } + return n; + }; + + const calls = []; + const findCalls = (node) => { + if (ts.isCallExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === "execute") { + const arg = node.arguments.length === 1 ? unwrap(node.arguments[0]) : null; + if (arg && ts.isObjectLiteralExpression(arg)) calls.push(arg); + } + ts.forEachChild(node, findCalls); + }; + findCalls(sf); + + if (calls.length !== 1) { + fail( + `surface ${surface.id}: expected exactly one \`execute({…})\` call in ${surface.file}, found ${calls.length}.\n` + + " Zero means the forward site moved or the payload stopped being a literal; more than one\n" + + " means the surface has split and each payload needs its own entry in SURFACES. Either way\n" + + " the forwarded key set cannot be read, and a gate that cannot read it must not pass." + ); + } + + const keys = new Set(); + const absorb = (objLiteral, depth = 0) => { + if (depth > 4) fail(`surface ${surface.id}: spread nesting too deep to resolve in ${surface.file}.`); + for (const prop of objLiteral.properties) { + if (ts.isPropertyAssignment(prop) || ts.isShorthandPropertyAssignment(prop)) { + const name = prop.name; + if (name && (ts.isIdentifier(name) || ts.isStringLiteral(name))) keys.add(name.text); + continue; + } + if (ts.isSpreadAssignment(prop)) { + const expr = unwrap(prop.expression); + if (ts.isObjectLiteralExpression(expr)) { + absorb(expr, depth + 1); + continue; + } + if (ts.isIdentifier(expr)) { + const declared = locals.get(expr.text); + const resolved = declared ? unwrap(declared) : null; + const branches = []; + if (resolved && ts.isConditionalExpression(resolved)) { + branches.push(unwrap(resolved.whenTrue), unwrap(resolved.whenFalse)); + } else if (resolved && ts.isObjectLiteralExpression(resolved)) { + branches.push(resolved); + } + if (branches.length > 0 && branches.every((b) => ts.isObjectLiteralExpression(b))) { + for (const b of branches) absorb(b, depth + 1); + continue; + } + if (opaqueSpreads[`${surface.id}:${expr.text}`]) continue; + fail( + `surface ${surface.id}: cannot resolve the spread \`...${expr.text}\` in ${surface.file}.\n` + + " A spread may carry any key, so treating it as empty would be the silent pass the\n" + + " ruling forbids. Either it resolves to object literals this gate can read, or it\n" + + ` needs an OPAQUE_SPREADS entry keyed "${surface.id}:${expr.text}" saying why its\n` + + " source cannot carry authored action keys." + ); + } + fail( + `surface ${surface.id}: unreadable spread in the \`execute({…})\` payload in ${surface.file}.\n` + + " The forwarded key set cannot be determined; extraction failure is red by ruling." + ); + } + } + }; + absorb(calls[0]); + + if (keys.size === 0) { + fail(`surface ${surface.id}: extracted zero forwarded keys from ${surface.file} — extraction failed.`); + } + return keys; +} + +// ── 4. Diff ────────────────────────────────────────────────────────────────── +/** + * `owed − forwarded − excused = ∅`, per surface. + * + * Returns `{ errors, report, runtimeRead, perFile }`; `errors` is empty when + * every surface is clean. Anything that stops the inputs being READABLE throws + * {@link ExtractionError} instead — see the header. + */ +export function analyze(root = REPO_ROOT, options = {}) { + const { + surfaces = SURFACES, + consumers = RUNTIME_CONSUMERS, + justified = JUSTIFIED, + knownGaps = KNOWN_GAPS, + opaqueSpreads = OPAQUE_SPREADS, + spec = null, + uiActionView = UI_ACTION_VIEW, + actionKeysModule = ACTION_KEYS_MODULE, + } = options; + + const specKeys = spec ?? loadSpecSchemas(); + const uiView = uiActionViewKeys(root, uiActionView); + const retired = new Set(retiredKeys(root, actionKeysModule)); + const { union: runtimeRead, perFile } = runtimeReadKeys(root, consumers); + + const authorable = { + declared: new Set([...specKeys.declared, ...uiView]), + inline: new Set(specKeys.inline), + }; + + const errors = []; + const matchedJustified = new Set(); + const matchedGaps = new Set(); + const report = []; + + for (const surface of surfaces) { + const vocabulary = authorable[surface.contract]; + if (!vocabulary) { + fail(`surface ${surface.id}: unknown contract \`${surface.contract}\` — expected "declared" or "inline".`); + } + const owed = [...vocabulary].filter((k) => runtimeRead.has(k) && !retired.has(k)).sort(); + if (owed.length === 0) { + fail( + `surface ${surface.id}: owed set is empty.\n` + + " Nothing would ever be checked for it — a vacuously green surface is the #4690\n" + + " anti-pattern, so this is red." + ); + } + const forwarded = forwardedKeys(root, surface, opaqueSpreads); + const dropped = owed.filter((k) => !forwarded.has(k)); + + const unexcused = []; + const excusedJustified = []; + const excusedGaps = []; + for (const key of dropped) { + const entry = `${surface.id}:${key}`; + if (justified[entry]) { + matchedJustified.add(entry); + excusedJustified.push(key); + } else if (knownGaps[entry]) { + matchedGaps.add(entry); + excusedGaps.push(key); + } else unexcused.push(key); + } + + report.push({ + surface, + owed, + forwarded: [...forwarded].sort(), + dropped, + justified: excusedJustified, + gaps: excusedGaps, + unexcused, + }); + + if (unexcused.length > 0) { + errors.push( + `${surface.id} (${surface.file}) does not forward ${unexcused.length} key` + + `${unexcused.length === 1 ? "" : "s"} the runtime reads: \`${unexcused.join("`, `")}\`.\n` + + ` Each is authorable on this surface's ${surface.contract === "inline" ? "`InlineActionSchema`" : "`ActionSchema`"} ` + + "contract AND read off the forwarded def at execute time, so it is silently dropped\n" + + " one hop before the runner — the objectstack#6837 / #6938 shape. Add it to the\n" + + " `execute({…})` payload, or — if the omission is CORRECT — add a JUSTIFIED entry\n" + + " naming the guard that makes it unreachable or the renderer that consumes it,\n" + + " with the file:line evidence." + ); + } + } + + // Ratchet — an entry that excuses nothing must go, or it reserves the key for + // a future drop under an exemption nobody re-examined. + for (const [entry, meta] of Object.entries(justified)) { + if (matchedJustified.has(entry)) continue; + errors.push( + `JUSTIFIED entry \`${entry}\` excuses nothing — the key is now forwarded, is no longer\n` + + " owed on that surface, or the surface is gone. Delete it: a stale exemption is how\n" + + " the next silent drop inherits a reason that was never about it.\n" + + ` (reason on file: ${meta.reason.slice(0, 90)}…)` + ); + } + for (const [entry, meta] of Object.entries(knownGaps)) { + if (matchedGaps.has(entry)) continue; + errors.push( + `KNOWN_GAPS entry \`${entry}\` is no longer a gap — delete it (and close #${meta.issue} once\n` + + " its last entry is gone). Left in, it re-reserves the key for a future drop." + ); + } + for (const entry of Object.keys(opaqueSpreads)) { + const surfaceId = entry.slice(0, entry.lastIndexOf(":")); + const ident = entry.slice(entry.lastIndexOf(":") + 1); + const surface = surfaces.find((s) => s.id === surfaceId); + if (!surface || !readFile(root, surface.file).includes(`...${ident}`)) { + errors.push( + `OPAQUE_SPREADS entry \`${entry}\` matches no spread in the surface's file — delete it,\n` + + " so an unreadable spread cannot inherit an exemption written for a different one." + ); + } + } + + return { errors, report, runtimeRead, perFile, authorable }; +} + +// ── CLI ────────────────────────────────────────────────────────────────────── +const invokedDirectly = process.argv[1] && resolve(process.argv[1]) === resolve(fileURLToPath(import.meta.url)); + +if (invokedDirectly) { + let result; + try { + result = analyze(REPO_ROOT); + } catch (error) { + if (!(error instanceof ExtractionError)) throw error; + console.error(`❌ action forward parity: ${error.message}`); + process.exit(1); + } + + const { errors, report, runtimeRead, perFile } = result; + + if (errors.length === 0) { + const gaps = Object.keys(KNOWN_GAPS).length; + const justifiedCount = Object.keys(JUSTIFIED).length; + console.log( + `✅ action forward parity: ${SURFACES.length} surfaces checked against ${runtimeRead.size} runtime-read keys ` + + `from ${perFile.size} consumers; ${justifiedCount} justified omission` + + `${justifiedCount === 1 ? "" : "s"}, ${gaps} known gap${gaps === 1 ? "" : "s"}.` + ); + for (const r of report) { + console.log( + ` ${r.surface.id.padEnd(14)} owes ${String(r.owed.length).padStart(2)}, ` + + `forwards ${String(r.forwarded.length).padStart(2)}` + ); + } + process.exit(0); + } + + console.error("❌ an action renderer drops a key the runtime reads:\n"); + for (const message of errors) console.error(` • ${message}\n`); + console.error( + "Every instance of this class shipped green: the key parses, publishes, and reads as\n" + + "honoured while the payload is dropped one hop before the runner. See\n" + + "https://github.com/objectstack-ai/objectui/issues/4050 for the six that were found by\n" + + "hand before this gate existed." + ); + process.exit(1); +}