From 6a31dc7714e7b87fb15ad40ab688003c9b519ae1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 23:27:08 +0000 Subject: [PATCH 1/2] ci: gate each action renderer's forward whitelist against the keys the runtime reads (#4050) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 while the payload is dropped one hop before the runner. Six instances shipped that way, each found by a human reading the lists side by side (bodyExtra objectstack#6837, bodyShape #6938, resultDialog objectui#3646, openIn / locations / undoable, label + description #4192). objectstack#6975 stalled because one half of the diff looked underivable: a grep over the runner and the console handlers cannot separate a body-path key a renderer must forward from a mechanic the runner resolves itself. Two changes make it derivable — read the consumers with the compiler API, scoped to the binding the def arrives as (so `a.locations` off the AUTHORED list is not confused with `action.target` off the FORWARDED def), and intersect that with what the surface may be AUTHORED with: owed(surface) = authorable(surface) ∩ runtime-read − retired Both inputs come from their real declarations — the spec's own zod shapes plus `@object-ui/types`' renderer view, and the consumers' ASTs — so neither can be a stale hand copy. The surface-class split #6975 flagged is derived rather than registered: `element:button` gets `InlineActionSchema`'s narrower vocabulary and is green with no exemption at all. What is left over is real, and is declared in two ratcheted tables: JUSTIFIED ("correctly omitted" — unreachable behind a `rowRecord` guard, or consumed by the renderer itself) and KNOWN_GAPS ("really dropped, filed, not fixed here", #4202). Each entry carries file:line evidence, and an entry that excuses nothing is itself a failure, so neither table can outlive the code it excuses — the answer to #6975's objection that the registry becomes the drift-prone list one level up. Extraction failure is RED, never a silent pass, per the ruling: an unresolvable spread, a renamed binding, a moved `execute()` site, a missing consumer, an empty owed set. Each throws rather than returning a clean verdict, and each is driven from a synthetic repo in the pin test, so no fix that makes the real tree green can leave the rule unexercised. Wired as `pnpm check:action-forward-parity` in ci.yml's `type-check` job beside `check:spec-symbols` — it needs the install (it resolves @objectstack/spec) but nothing built. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .github/workflows/ci.yml | 14 + content/docs/guide/ci-cd-pipeline.md | 2 +- package.json | 1 + .../check-action-forward-parity.test.ts | 499 +++++++++++ scripts/check-action-forward-parity.mjs | 780 ++++++++++++++++++ 5 files changed, 1295 insertions(+), 1 deletion(-) create mode 100644 scripts/__tests__/check-action-forward-parity.test.ts create mode 100644 scripts/check-action-forward-parity.mjs 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/scripts/__tests__/check-action-forward-parity.test.ts b/scripts/__tests__/check-action-forward-parity.test.ts new file mode 100644 index 000000000..db39b8616 --- /dev/null +++ b/scripts/__tests__/check-action-forward-parity.test.ts @@ -0,0 +1,499 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// Plain-JS CI helper. Its types are INFERRED from the .mjs source by +// `tsconfig.scripts.json` (`allowJs`), so no `@ts-expect-error` here — see +// objectui#3494. +import { + analyze, + ExtractionError, + forwardedKeys, + runtimeReadKeys, + retiredKeys, + uiActionViewKeys, + JUSTIFIED, + KNOWN_GAPS, + OPAQUE_SPREADS, + SURFACES, + RUNTIME_CONSUMERS, +} from '../check-action-forward-parity.mjs'; + +/** + * objectui#4050 — the behaviour test for `scripts/check-action-forward-parity.mjs`. + * + * The gate answers "does this renderer forward every authored key the runtime + * will read off the def it is handed?", which nothing else in the repo can: + * `actionKeys.pin.test.ts` pins the ActionDef INTERFACE, and the renderers' + * own tests drive one renderer each. Six keys shipped dropped one at a time — + * `bodyExtra` (objectstack#6837), `bodyShape` (#6938), `resultDialog` + * (objectui#3646), `label`/`description` (objectui#4192) — every one of them + * green, because a dropped key parses, publishes and reads as honoured. + * + * Two properties are pinned here, and they fail for different reasons: + * + * 1. The VERDICTS, against synthetic repos. Every red the gate can produce is + * driven from a fixture rather than from whatever `main` happens to contain + * today — so a fix that makes the real tree green cannot also make the rule + * unexercised. That is the objectui#4690 anti-pattern this card's ruling + * names outright: "extraction failure is red, never a silent pass". + * + * 2. NON-VACUITY on the real repo. A gate whose inputs collapse to empty sets + * passes everything: if the owed set were empty, or the runtime-read set + * were, the whole diff would be `∅ − x = ∅` on every surface forever. The + * last describe holds the real measurements above the floor where that + * becomes possible. + * + * The extraction-failure cases are the ones worth reading before widening the + * gate. Each is a way the gate could have been made to pass while reading + * NOTHING — an unresolved spread taken for an empty object, a renamed binding + * taken for "the runtime reads no keys", a moved `execute()` call taken for "no + * payload, nothing owed" — and each is asserted to throw rather than return a + * clean verdict. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); + +/** The two authorable vocabularies, injected so fixtures need no spec package. */ +const SPEC = { + declared: ['name', 'type', 'target', 'undoable', 'bodyShape', 'objectName'], + inline: ['name', 'type', 'target'], +}; + +interface Fixture { + /** `path -> 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); +} From da20bddc0c1fc22a8dc3eafeab56eb640edf645d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 23:27:36 +0000 Subject: [PATCH 2/2] fix(components): action:menu, :icon and :group forward the keys that title a param dialog and reveal a result (#4192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Which renderer a declared action gets is decided by `action:bar`'s `maxVisible` split (3 desktop, 1 mobile) and by `systemActions`, which are always in the overflow menu. The four declared renderers' forward whitelists had drifted, so the same declaration behaved differently by viewport width. `label` and `description` are what the console's param-collection handler titles its dialog from (`title: action?.label || action?.title`, `description: actionDescription(…, action?.description)` — useConsoleActionRuntime.tsx:205-207). Dropped, an overflow action with declared `params` opened a dialog titled "Action parameters" while the SAME declaration rendered inline named itself. #4192 measured this on `action:menu`; the gate from the previous commit found it on `action:icon` and `action:group` too, and found that those two also drop `resultDialog` — the one-shot reveal spec whose loss is objectui#3646, still live on two of the four surfaces. `undoable` and `recordIdField` are deliberately NOT added, which re-scopes half of #4192. Both are read only under a `rowRecord` guard (useConsoleActionRuntime.tsx:377, :398), and `rowRecord` is `params._rowRecord` — written only by the spread-based hosts (DeclaredActionsBar.tsx:215, RelatedRecordActionsBridge.tsx:164, ObjectGrid.tsx:1721/:2034, containers.tsx:1287-1289), none of which dispatch through these renderers. `action:button` forwards them on this path INERTLY, so the menu was never "losing Undo" relative to the button; adding them would have shipped a second inert copy. That verdict is carried, with its evidence, in the gate's JUSTIFIED table, which fails if it ever stops being true. The behavioural pin drives the real renderers through the real runner and asserts what the param-collection handler is HANDED, so the two halves fail for different reasons: delete a key from a whitelist and both the gate and the pin go red; break the runner's param-collection dispatch and only the pin does. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- ...tion-forward-whitelist-parity-4050-4192.md | 13 ++ .../__tests__/action-forward-parity.test.tsx | 164 ++++++++++++++++++ .../src/renderers/action/action-group.tsx | 9 + .../src/renderers/action/action-icon.tsx | 9 + .../src/renderers/action/action-menu.tsx | 10 ++ 5 files changed, 205 insertions(+) create mode 100644 .changeset/action-forward-whitelist-parity-4050-4192.md create mode 100644 packages/components/src/renderers/action/__tests__/action-forward-parity.test.tsx 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/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