Skip to content

Commit a6df727

Browse files
committed
feat(core): inventory ActionDef's keys and warn on the ones nothing reads (objectstack#4075 step 1)
`ActionDef` ends with `[key: string]: any`, so it accepts any key of any type. Deleting `ActionDef.execute` produced ZERO compile errors even though the field had just been removed (objectui#2990), and stale metadata still authoring `execute: 'markDone'` type-checks today. The same deletion against `@object-ui/types`' `ActionSchema` — no index signature — correctly produced TS2353 at the authoring site. One reader can catch a retired key; the other is structurally incapable. Step 1 is the non-breaking half: make the key set visible, warn on anything outside it, change no types. What the inventory found, which is step 2's worklist: - 18 keys the SPEC owns that `ActionDef` never declared — `visible`, `locations`, `icon`, `variant`, `order`, `component`, `bulkEnabled`, `requiredPermissions` and 10 more. `ActionEngine` reads two of them through `as any` casts (`(ra.action as any).visible`, `(action as any).locations`); those casts exist only because the field is undeclared. - 17 keys `ActionDef` declares that the spec does not own — `actionType`, `api`, `chain`, `onClick`, `toast` and others, several already marked legacy. This is objectui's own dialect and needs the #4115 treatment: documented as deliberate, or retired. - `execute` is not simply gone from the spec — it is a live TOMBSTONE, still in `ActionSchema` so the parser can reject it BY NAME with the rename prescription. So it is warned about separately and more loudly than an unknown key: an unknown key is probably a typo, a retired key is metadata that used to work. - `to` / `external` / `newTab` / `replace` are a real objectui dialect — the `navigation` alias's own spelling, read off the action when no nested `navigate` object is present. Declared here rather than left to trip the warning, with a tripwire test that fails if the spec ever adopts one. The lists are data because TypeScript types do not survive to runtime, and `keyof ActionDef` cannot substitute — the index signature widens it to `string | number`, which is the problem itself. `actionKeys.pin.test.ts` re-derives each list from its real source (the interface via the AST, the spec via its schema) so a hand-maintained list cannot drift from what it mirrors. The spec key list is pinned rather than resolved at runtime: `ActionSchema` is a lazy proxy that does not forward `.shape`, so reading it means walking zod internals — fine in a test, wrong in shipped code. `@object-ui/types` made the same call for `ActionComponent`. Discrimination proof: dropping any entry from `ACTION_DEF_KEYS` fails with "ActionDef declares keys the inventory is missing: <name>"; the warning tests assert it stays silent on a well-formed action, names `targt`, gives `execute` its rename prescription, warns once rather than per click, and says nothing in production. Core type-checks clean and all 207 existing action tests still pass. One pin is deliberately inverted: the suite asserts the index signature is STILL THERE. The day that fails, step 3 has landed, `tsc` catches unknown keys itself, and this warning plus `executeScript`'s rename branch can retire together. Refs objectstack#4075, objectstack#3903, objectui#2990, #3856, #3855, #2169, AGENTS.md PD #12 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rvv6qysks2dRLaGfpTdgEy
1 parent ea84651 commit a6df727

4 files changed

Lines changed: 428 additions & 0 deletions

File tree

packages/core/src/actions/ActionRunner.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import type { RunnableActionType } from '@object-ui/types';
2525
import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator';
2626
import { globalUndoManager, type UndoableOperation } from './UndoManager';
27+
import { warnOnUnknownActionKeys } from './actionKeys';
2728

2829
export interface ActionResult {
2930
success: boolean;
@@ -475,6 +476,12 @@ export class ActionRunner {
475476

476477
async execute(action: ActionDef): Promise<ActionResult> {
477478
try {
479+
// `ActionDef` accepts any key of any type, so a typo (`targt`) and a
480+
// retired key (`execute`) both reach here having type-checked. Neither
481+
// binds a handler, and binding no handler silently is the #2169 "Mark Done
482+
// does nothing" shape. Dev-only, warn-once, changes nothing (#4075 step 1).
483+
warnOnUnknownActionKeys(action);
484+
478485
// Resolve the action type
479486
const actionType = action.type || action.actionType || action.name || '';
480487

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/**
2+
* Pins the key inventory in `actionKeys.ts` to the two things it mirrors —
3+
* `ActionDef`'s own declarations and `@objectstack/spec`'s `ActionSchema`
4+
* (objectstack#4075 step 1).
5+
*
6+
* Why a test rather than a comment: a hand-maintained list that silently drifts
7+
* from the interface it claims to mirror is the exact "declared ≠ enforced"
8+
* failure this work is about. `ActionDef` cannot be enumerated at runtime — its
9+
* `[key: string]: any` widens `keyof` to `string | number` — so the list is data,
10+
* and this file is what makes the data true. Adding a field to `ActionDef`
11+
* without adding it here fails, by name.
12+
*
13+
* Discrimination proof for the guard below: with `ACTION_DEF_KEYS` complete these
14+
* pass; dropping any single entry (e.g. `target`) fails with
15+
* "ActionDef declares keys the inventory is missing: target".
16+
*/
17+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
18+
import { readFileSync } from 'node:fs';
19+
import { fileURLToPath } from 'node:url';
20+
import { dirname, join } from 'node:path';
21+
import ts from 'typescript';
22+
import * as SpecUI from '@objectstack/spec/ui';
23+
import {
24+
ACTION_DEF_KEYS,
25+
SPEC_ACTION_KEYS,
26+
NAVIGATION_ALIAS_KEYS,
27+
RETIRED_ACTION_KEYS,
28+
KNOWN_ACTION_KEYS,
29+
classifyActionKeys,
30+
warnOnUnknownActionKeys,
31+
resetActionKeyWarnings,
32+
} from '../actionKeys';
33+
34+
const RUNNER = join(dirname(fileURLToPath(import.meta.url)), '..', 'ActionRunner.ts');
35+
36+
/** `ActionDef`'s declared property names, read off the interface itself. */
37+
function declaredActionDefKeys(): string[] {
38+
const sf = ts.createSourceFile(RUNNER, readFileSync(RUNNER, 'utf8'), ts.ScriptTarget.Latest, true);
39+
for (const stmt of sf.statements) {
40+
if (!ts.isInterfaceDeclaration(stmt) || stmt.name.text !== 'ActionDef') continue;
41+
return stmt.members
42+
.filter(ts.isPropertySignature)
43+
.map((m) => (m.name && (ts.isIdentifier(m.name) || ts.isStringLiteral(m.name)) ? m.name.text : null))
44+
.filter((n): n is string => n !== null);
45+
}
46+
throw new Error('ActionDef interface not found in ActionRunner.ts');
47+
}
48+
49+
/**
50+
* The spec's `ActionSchema` keys. `ActionSchema` is a lazy proxy that does not
51+
* forward `.shape`, so this walks zod internals to reach the object shape —
52+
* acceptable HERE (a test pins a fact) and deliberately not done in shipped code.
53+
*/
54+
function specActionKeys(): string[] {
55+
const seen = new Set<unknown>();
56+
const walk = (schema: unknown, depth = 0): string[] | null => {
57+
if (!schema || depth > 8 || seen.has(schema)) return null;
58+
seen.add(schema);
59+
const s = schema as Record<string, any>;
60+
if (s.shape) return Object.keys(s.shape);
61+
const def = s._def ?? s.def;
62+
if (!def) return null;
63+
if (def.shape) return Object.keys(def.shape);
64+
for (const key of ['in', 'out', 'innerType', 'schema', 'left', 'right']) {
65+
const found = def[key] ? walk(def[key], depth + 1) : null;
66+
if (found) return found;
67+
}
68+
return null;
69+
};
70+
const keys = walk(SpecUI.ActionSchema);
71+
if (!keys) throw new Error('could not resolve @objectstack/spec/ui ActionSchema shape');
72+
return keys;
73+
}
74+
75+
describe('action key inventory (objectstack#4075 step 1)', () => {
76+
it('ActionDef still has the index signature this inventory compensates for', () => {
77+
// The day this fails, step 3 has landed: `tsc` catches unknown keys itself
78+
// and the dev-mode warning (plus `executeScript`'s rename branch) can retire.
79+
expect(readFileSync(RUNNER, 'utf8')).toContain('[key: string]: any');
80+
});
81+
82+
it('lists every key ActionDef declares', () => {
83+
const declared = declaredActionDefKeys();
84+
const missing = declared.filter((k) => !(ACTION_DEF_KEYS as readonly string[]).includes(k));
85+
const stale = (ACTION_DEF_KEYS as readonly string[]).filter((k) => !declared.includes(k));
86+
expect({ missing, stale }).toEqual({ missing: [], stale: [] });
87+
});
88+
89+
it('lists every key the spec ActionSchema declares', () => {
90+
const spec = specActionKeys();
91+
const missing = spec.filter((k) => !(SPEC_ACTION_KEYS as readonly string[]).includes(k));
92+
const stale = (SPEC_ACTION_KEYS as readonly string[]).filter((k) => !spec.includes(k));
93+
// `missing` means the spec grew a key objectui does not know about; `stale`
94+
// means it dropped one. Either way the inventory has to be re-derived, and
95+
// the diff names exactly which key moved.
96+
expect({ missing, stale }).toEqual({ missing: [], stale: [] });
97+
});
98+
99+
it('`execute` is still a live spec tombstone, so it must not count as known', () => {
100+
const parsed = SpecUI.ActionSchema.safeParse({
101+
name: 'mark_done',
102+
label: 'Mark Done',
103+
type: 'script',
104+
execute: 'markDone',
105+
});
106+
expect(parsed.success).toBe(false);
107+
expect(RETIRED_ACTION_KEYS).toHaveProperty('execute');
108+
expect(KNOWN_ACTION_KEYS.has('execute')).toBe(false);
109+
});
110+
111+
it('keeps the navigation alias out of the spec vocabulary it is not part of', () => {
112+
// If the spec ever adopts one of these, it stops being objectui dialect and
113+
// this fails — naming the alias to retire, the same tripwire shape as
114+
// `ObjectUiLocalActionType`.
115+
const spec = specActionKeys();
116+
expect(NAVIGATION_ALIAS_KEYS.filter((k) => spec.includes(k))).toEqual([]);
117+
});
118+
});
119+
120+
describe('unknown-key warning', () => {
121+
let warn: ReturnType<typeof vi.spyOn>;
122+
123+
beforeEach(() => {
124+
resetActionKeyWarnings();
125+
warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
126+
});
127+
afterEach(() => warn.mockRestore());
128+
129+
it('says nothing about an action built only from recognized keys', () => {
130+
warnOnUnknownActionKeys({ name: 'save', type: 'api', target: '/api/v1/save', locations: ['record_header'] });
131+
expect(warn).not.toHaveBeenCalled();
132+
});
133+
134+
it('names a typo that the compiler cannot see', () => {
135+
// `targt` type-checks today: ActionDef's index signature accepts it. Nothing
136+
// reads it, so the action runs and does nothing — #2169's shape.
137+
warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'saveRecord' });
138+
expect(warn).toHaveBeenCalledTimes(1);
139+
expect(warn.mock.calls[0][0]).toContain('`targt`');
140+
});
141+
142+
it('gives a retired key its rename prescription, not a bare "unknown"', () => {
143+
warnOnUnknownActionKeys({ name: 'mark_done', type: 'script', execute: 'markDone' });
144+
expect(warn).toHaveBeenCalledTimes(1);
145+
expect(warn.mock.calls[0][0]).toContain('rename the key to `target`');
146+
});
147+
148+
it('warns once per key, not once per click', () => {
149+
for (let i = 0; i < 5; i++) warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'x' });
150+
expect(warn).toHaveBeenCalledTimes(1);
151+
});
152+
153+
it('is silent in production', () => {
154+
const prev = process.env.NODE_ENV;
155+
process.env.NODE_ENV = 'production';
156+
try {
157+
warnOnUnknownActionKeys({ name: 'save', type: 'script', targt: 'x' });
158+
expect(warn).not.toHaveBeenCalled();
159+
} finally {
160+
process.env.NODE_ENV = prev;
161+
}
162+
});
163+
164+
it('classifies unknown and retired keys separately', () => {
165+
expect(classifyActionKeys({ type: 'script', execute: 'a', targt: 'b' })).toEqual({
166+
unknown: ['targt'],
167+
retired: ['execute'],
168+
});
169+
});
170+
});

0 commit comments

Comments
 (0)