From afdb83c0326dc547ebae3c83949c0a4ba536642d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 09:14:28 +0000 Subject: [PATCH 1/3] feat(spec,cli): warn the author when a discarded action alias costs them a handler (#3743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3742 made `target` beat the deprecated `execute` alias everywhere and had the `ActionSchema` transform DROP the alias from its output, so "two different scripts for one button" became unrepresentable. What it left behind: an author who declares both slots with different values still loses one of the two handlers they wrote, silently. Per Prime Directive #12 that belongs at authoring time. New advisory rule `action-target-execute-conflict`: an action declaring both slots with different values gets a warning naming both handlers, stating that `target` wins, and giving the one-line fix. Equal values are harmless duplication and stay quiet. It never fails the build — the resulting stack is well-defined; the cost is a handler that never runs. The rule has to run PRE-PARSE, because the parse is what consumes the alias: once `ObjectStackDefinitionSchema` has run there is no `execute` key left to report. #3743 proposed the CLI compile pipeline as its home, between `lowerCallables` and `safeParse`. That window is real but nearly always empty: `defineStack` parses inside the author's own config module, so for a `defineStack` app — i.e. almost every app — the alias is gone before `os build` ever loads the module. A CLI-only lint would have been structurally blind on the dominant authoring path. So the rule lives in `@objectstack/spec` (`lintDeprecatedAliases`, exported from the package root) and is wired into both layers that perform the discard: - `defineStack` warns on the console before parsing, once per distinct conflict per process — the earliest point the conflict exists. - `os build` / `os validate` run a new pre-parse pass over `normalized`, covering stacks that skip strict `defineStack`: a plain object default-export, `strict: false`, and inline function handlers (`target` is `z.string()`, so those cannot pass strict `defineStack` and are lowered by the CLI instead). Both commands lint the same input, so they agree by construction (#3782) and the parity gate covers the new rule. Each layer reports only its own discards, so one authored conflict yields exactly one warning however the stack is compiled. Behaviour fix in the same contract: #3742 fixed compile-time precedence by probing for a CALLABLE `target` first, which left one combination still resolving the alias's way — a string `target` beside a function `execute` bound the alias and then overwrote the canonical ref the author wrote. `target` now wins in every string/function combination, matching the `ActionSchema` transform, so the new warning states one precedence rule that is true everywhere. Verified end to end on both authoring paths: a `defineStack` config warns once from the authoring gate and the CLI pass stays quiet; a plain-object config warns from the CLI pass for both the string/string and string/function pairs, and the emitted artifact keeps the canonical `target` with no `execute` on the wire. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JqC9ckaLbmhmytt65Gxi96 --- .changeset/action-alias-conflict-warning.md | 52 ++++++ packages/cli/src/commands/compile.ts | 49 +++++- packages/cli/src/commands/validate.ts | 17 +- .../cli/src/utils/lower-callables.test.ts | 27 +++ packages/cli/src/utils/lower-callables.ts | 30 +++- packages/spec/src/index.ts | 6 + .../src/shared/deprecated-aliases.test.ts | 158 ++++++++++++++++++ .../spec/src/shared/deprecated-aliases.ts | 147 ++++++++++++++++ packages/spec/src/stack.test.ts | 85 +++++++++- packages/spec/src/stack.zod.ts | 40 ++++- packages/spec/src/ui/action.zod.ts | 3 + 11 files changed, 604 insertions(+), 10 deletions(-) create mode 100644 .changeset/action-alias-conflict-warning.md create mode 100644 packages/spec/src/shared/deprecated-aliases.test.ts create mode 100644 packages/spec/src/shared/deprecated-aliases.ts diff --git a/.changeset/action-alias-conflict-warning.md b/.changeset/action-alias-conflict-warning.md new file mode 100644 index 0000000000..d937625837 --- /dev/null +++ b/.changeset/action-alias-conflict-warning.md @@ -0,0 +1,52 @@ +--- +"@objectstack/spec": minor +"@objectstack/cli": patch +--- + +feat(spec,cli): warn the author when a deprecated action alias is discarded (#3743) + +#3742 made `target` beat the deprecated `execute` alias everywhere and had the +`ActionSchema` transform **drop** the alias from its output, so "two different +scripts for one button" became unrepresentable. What it left behind: an author +who declares both slots with different values still loses one of the two +handlers they wrote, **silently**. Per Prime Directive #12 that belongs at +authoring time, so it is now reported there. + +**New rule — `action-target-execute-conflict` (advisory).** An action declaring +both `target` and `execute` with different values gets a warning naming both +handlers, stating that `target` wins, and giving the one-line fix (delete +`execute`). Identical values in both slots are harmless duplication and stay +quiet. It never fails the build: the resulting stack is well-defined — the cost +is a handler that never runs, not a broken artifact. + +The rule must run **pre-parse**, because the parse is what consumes the alias: +once `ObjectStackDefinitionSchema` has run there is no `execute` key left to +report. It therefore lives in `@objectstack/spec` +(`lintDeprecatedAliases`, exported from the package root) and is wired into +both layers that perform the discard: + +- **`defineStack`** — the dominant authoring path, and the one that consumes the + alias earliest: it parses inside your own config module, so by the time + `os build` loads that module the alias is already gone. It now warns on the + console before parsing (once per distinct conflict per process). +- **`os build` / `os validate`** — a new pre-parse pass covering stacks that + skip strict `defineStack`: a plain object default-export, + `defineStack(…, { strict: false })`, and inline function handlers (`target` is + `z.string()`, so those cannot pass strict `defineStack` and are lowered by the + CLI instead). Both commands lint the same input, so they agree by construction + (#3782). + +Each layer reports only its own discards, so one authored conflict produces +exactly one warning however the stack is compiled. + +**Behaviour fix in the same contract.** #3742 fixed compile-time precedence by +probing for a *callable* `target` first, which left one combination still +resolving the alias's way: a **string** `target` beside a **function** `execute` +bound the alias and then overwrote the canonical ref the author wrote. `target` +now wins in every combination of string/function across the two slots, matching +the `ActionSchema` transform — so the new warning states one precedence rule +that is true everywhere. If you relied on an inline `execute` function winning +over a string `target`, move it into `target`; the warning names the action. + +Authoring is otherwise unchanged: `execute` alone is still accepted, still +lowered into `target`, and still documented. diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 9539ce51be..16521a4875 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -5,7 +5,7 @@ import path from 'path'; import fs from 'fs'; import chalk from 'chalk'; import { ZodError } from 'zod'; -import { ObjectStackDefinitionSchema, normalizeStackInput } from '@objectstack/spec'; +import { ObjectStackDefinitionSchema, normalizeStackInput, lintDeprecatedAliases } from '@objectstack/spec'; import { loadConfig } from '../utils/config.js'; import { lowerCallables } from '../utils/lower-callables.js'; import { validateStackExpressions } from '@objectstack/lint'; @@ -95,6 +95,53 @@ export default class Compile extends Command { if (!flags.json) printStep('Normalizing stack definition...'); const normalized = normalizeStackInput(config as Record); + // 2a. [#3743] PRE-PARSE authoring lint. Everything in the post-parse lint + // block (3d and below) reads `result.data`, which is the wrong side of + // the fence for an alias the pipeline itself consumes: `lowerCallables` + // drops a function-valued `execute` and the `ActionSchema` transform + // drops a string one (#3742), so by then "the author declared both + // slots" is no longer representable. This pass runs on `normalized` — + // before lowering, before parse — where both slots are still as + // written, and it is the same input `os validate` lints, so the two + // surfaces agree by construction (#3782). + // + // It reports the discards THIS pipeline performs. A stack authored + // with strict `defineStack` has already been parsed inside its own + // config module, so its alias was consumed — and warned about — there; + // this pass covers everything that skipped that gate: a plain object + // default-export, `defineStack(…, { strict: false })`, and inline + // function handlers (`target: z.string()` rejects those, so they can + // only reach the pipeline via a non-strict path and are lowered here). + // + // Advisory today; `severity: 'error'` is honoured so a future rule + // gates the build here without further wiring. + if (!flags.json) printStep('Checking deprecated aliases (#3743)...'); + const aliasLint = lintDeprecatedAliases(normalized as Record); + const aliasLintErrors = aliasLint.filter((f) => f.severity === 'error'); + const aliasLintWarnings = aliasLint.filter((f) => f.severity !== 'error'); + if (aliasLintWarnings.length > 0 && !flags.json) { + console.log(''); + for (const f of aliasLintWarnings) { + printWarning(`${f.where}: ${f.message}`); + console.log(chalk.dim(` ${f.hint}`)); + console.log(chalk.dim(` rule: ${f.rule}`)); + } + } + if (aliasLintErrors.length > 0) { + if (flags.json) { + await emitJson({ success: false, error: 'deprecated alias check failed', issues: aliasLintErrors }, 0, { compact: true }); + this.exit(1); + } + console.log(''); + printError(`Deprecated alias check failed (${aliasLintErrors.length} issue${aliasLintErrors.length > 1 ? 's' : ''})`); + for (const f of aliasLintErrors) { + console.log(` • ${f.where}: ${f.message}`); + console.log(chalk.dim(` ${f.hint}`)); + console.log(chalk.dim(` rule: ${f.rule}`)); + } + this.exit(1); + } + // 2b. Lower inline `function` handlers (Hook.handler, top-level // `functions`) to stable string refs BEFORE Zod parse. This // guarantees we extract the user's real function identity (Zod's diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index b3dbcc7ce8..44c582de30 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -6,7 +6,7 @@ import { createRequire } from 'node:module'; import { join, dirname } from 'node:path'; import chalk from 'chalk'; import { ZodError } from 'zod'; -import { ObjectStackDefinitionSchema, normalizeStackInput, type ConversionNotice } from '@objectstack/spec'; +import { ObjectStackDefinitionSchema, normalizeStackInput, lintDeprecatedAliases, type ConversionNotice } from '@objectstack/spec'; import { loadConfig } from '../utils/config.js'; import { validateStackExpressions } from '@objectstack/lint'; import { validateListViewMode } from '@objectstack/lint'; @@ -601,12 +601,25 @@ export default class Validate extends Command { const viewRefErrors = viewRefLint.filter((f) => f.severity === 'error'); const viewRefWarnings = viewRefLint.filter((f) => f.severity !== 'error'); - const authoringLintErrors = [...flowLintErrors, ...autonumberErrors, ...viewRefErrors]; + // Deprecated aliases (#3743). The ONE lint here that reads `normalized` + // rather than `result.data`, and it has to: the parse consumes the alias + // it reports (`ActionSchema` folds `execute` into `target` and drops it), + // so a post-parse read is structurally blind to the conflict. `os build` + // lints the same pre-parse input, so both surfaces see the same findings. + // A stack authored with strict `defineStack` was already parsed — and + // warned about — inside its own config module; this covers the paths that + // reach the CLI with the alias intact. + const aliasLint = lintDeprecatedAliases(normalized as Record); + const aliasLintErrors = aliasLint.filter((f) => f.severity === 'error'); + const aliasLintWarnings = aliasLint.filter((f) => f.severity !== 'error'); + + const authoringLintErrors = [...flowLintErrors, ...autonumberErrors, ...viewRefErrors, ...aliasLintErrors]; const authoringLintWarnings = [ ...flowLintWarnings, ...livenessLint, ...autonumberWarnings, ...viewRefWarnings, + ...aliasLintWarnings, ]; if (authoringLintErrors.length > 0) { if (flags.json) { diff --git a/packages/cli/src/utils/lower-callables.test.ts b/packages/cli/src/utils/lower-callables.test.ts index 76577f6b67..41ae8dc023 100644 --- a/packages/cli/src/utils/lower-callables.test.ts +++ b/packages/cli/src/utils/lower-callables.test.ts @@ -92,6 +92,33 @@ describe('lowerCallables — action handler slot precedence (#3713)', () => { expect(result.count).toBe(0); }); + // #3743: "`target` first" meant "a CALLABLE `target` first", which left one + // combination still resolving the alias's way — a string `target` beside a + // function `execute` bundled the alias and then overwrote the canonical ref + // the author wrote. Same inversion as above, one slot type over. + it('keeps a string `target` when the alias holds an inline function', () => { + const result = lowerCallables({ + actions: [{ + name: 'mixed', + label: 'Mixed', + type: 'script', + target: 'preferredHandler', + execute: function legacyHandler() { return 'legacy'; }, + }], + }); + + const [action] = actionsOf(result); + expect(action.target).toBe('preferredHandler'); + expect(result.count).toBe(0); + // The losing alias still has to go: a function value is not JSON-safe and + // `ActionSchema` expects a string, so leaving it would trade the silent + // discard for a confusing parse error. `lintDeprecatedAliases` is what + // tells the author which handler they lost (#3743). + expect('execute' in action).toBe(false); + expect(() => JSON.stringify(result.lowered)).not.toThrow(); + expect(JSON.stringify(result.lowered)).not.toContain('legacy'); + }); + it('applies the same precedence to actions nested under an object', () => { const result = lowerCallables({ objects: [{ diff --git a/packages/cli/src/utils/lower-callables.ts b/packages/cli/src/utils/lower-callables.ts index 791ee391ee..1de9d6dbf6 100644 --- a/packages/cli/src/utils/lower-callables.ts +++ b/packages/cli/src/utils/lower-callables.ts @@ -175,9 +175,10 @@ export function lowerCallables(input: Record): LoweringResult { /** * Lower a single action definition: detect a callable on `target` or on the - * deprecated `execute` alias (canonical `target` first — #3713), register it, - * optionally extract a metadata body, and drop the alias. Mutates a shallow - * clone, never the input. + * deprecated `execute` alias, register it, optionally extract a metadata body, + * and drop the alias. A declared `target` — string or function — always wins + * over `execute` (#3713 / #3743), matching the `ActionSchema` transform. Mutates + * a shallow clone, never the input. */ function lowerActionCallable( raw: unknown, @@ -197,13 +198,32 @@ function lowerActionCallable( // (which keeps `target` when both are set). An author who inlined a function in // both slots got the `execute` one bundled while `action.target = ref` below // silently overwrote the `target` function they had actually declared. + // + // #3743: "`target` first" means the DECLARED target, not merely a callable one. + // Probing `typeof action.target === 'function'` left one combination still + // resolving the alias's way: a STRING `target` beside a function `execute` fell + // through to the alias, bundled it, and then overwrote the canonical ref the + // author wrote — the same inversion #3713 fixed for the function/function pair, + // one slot type over. `target` now wins in every combination, so the + // `action-target-execute-conflict` warning can state one precedence rule and + // have it be true everywhere. + const targetDeclared = + typeof action.target === 'function' || (typeof action.target === 'string' && action.target.length > 0); const handlerSlot: 'execute' | 'target' | null = typeof action.target === 'function' ? 'target' - : typeof action.execute === 'function' + : !targetDeclared && typeof action.execute === 'function' ? 'execute' : null; - if (!handlerSlot) return action; + if (!handlerSlot) { + // A function-valued alias that lost to a string `target` still has to go: it + // is not JSON-safe (`JSON.stringify` drops it from the artifact without a + // word) and `ActionSchema` expects a string, so leaving it would trade a + // silent discard for a confusing parse error. The lint above has already + // told the author which handler they lost. + if (typeof action.execute === 'function') delete action.execute; + return action; + } const fn = action[handlerSlot] as AnyFn; const ref = uniqueName(baseName, taken); taken.add(ref); diff --git a/packages/spec/src/index.ts b/packages/spec/src/index.ts index 326be1389e..b3ae702eb1 100644 --- a/packages/spec/src/index.ts +++ b/packages/spec/src/index.ts @@ -107,6 +107,12 @@ export { suggestFieldType, findClosestMatches, formatSuggestion } from './shared export { normalizeMetadataCollection, normalizeStackInput, normalizePluginMetadata, MAP_SUPPORTED_FIELDS, METADATA_ALIASES } from './shared/metadata-collection.zod'; export type { MetadataCollectionInput, MapSupportedField, NormalizeStackInputOptions } from './shared/metadata-collection.zod'; +// Pre-parse authoring lint (#3743) — the one window where a deprecated alias is +// still visible, since the parse itself resolves and drops it. `defineStack` +// warns from here; the CLI runs the same rules over stacks that skip it. +export { lintDeprecatedAliases, formatDeprecatedAliasFinding, ACTION_TARGET_EXECUTE_CONFLICT } from './shared/deprecated-aliases'; +export type { DeprecatedAliasFinding } from './shared/deprecated-aliases'; + // Metadata conversion layer (ADR-0087 D2) — old-shape → canonical-shape transforms applied at load. export * from './conversions/index.js'; diff --git a/packages/spec/src/shared/deprecated-aliases.test.ts b/packages/spec/src/shared/deprecated-aliases.test.ts new file mode 100644 index 0000000000..4f1d19deb9 --- /dev/null +++ b/packages/spec/src/shared/deprecated-aliases.test.ts @@ -0,0 +1,158 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { lintDeprecatedAliases, ACTION_TARGET_EXECUTE_CONFLICT } from './deprecated-aliases'; + +// ── #3743: the discarded `execute` alias now has an author-facing warning ── +// +// #3742 made `target` win everywhere and had the `ActionSchema` transform DROP +// `execute` from its output, so "two different scripts for one button" became +// unrepresentable. What it left: when an author declares both slots with +// different values, the losing one is thrown away **silently**. Prime Directive +// #12 wants that surfaced at authoring time, which is why this lint exists — and +// why it runs PRE-parse: the transform it reports on has already erased the +// evidence by the time anything downstream gets to look. + +const action = (extra: Record) => ({ + name: 'convert', + label: 'Convert', + type: 'script', + ...extra, +}); + +describe('lintDeprecatedAliases — quiet paths', () => { + it('says nothing when only the canonical `target` is declared', () => { + expect(lintDeprecatedAliases({ actions: [action({ target: 'preferredHandler' })] })).toEqual([]); + }); + + it('says nothing when only the deprecated `execute` is declared', () => { + // The alias alone is lowered into `target` and works. Deprecation nagging is + // a separate decision from reporting a DISCARDED handler; this rule reports + // the discard only. + expect(lintDeprecatedAliases({ actions: [action({ execute: 'legacyHandler' })] })).toEqual([]); + }); + + it('says nothing when both slots carry the SAME value', () => { + // Redundant, not contradictory — nothing the author wrote is lost. + const stack = { actions: [action({ target: 'sameHandler', execute: 'sameHandler' })] }; + expect(lintDeprecatedAliases(stack)).toEqual([]); + }); + + it('says nothing when both slots hold the very same function reference', () => { + const handler = function sharedHandler() { return 'shared'; }; + const stack = { actions: [action({ target: handler, execute: handler })] }; + expect(lintDeprecatedAliases(stack)).toEqual([]); + }); + + it('treats an empty-string slot as undeclared', () => { + const stack = { actions: [action({ target: '', execute: 'legacyHandler' })] }; + expect(lintDeprecatedAliases(stack)).toEqual([]); + }); + + it('tolerates a stack with no actions at all', () => { + expect(lintDeprecatedAliases({})).toEqual([]); + expect(lintDeprecatedAliases({ objects: [{ name: 'crm_deal' }] })).toEqual([]); + }); +}); + +describe('lintDeprecatedAliases — action-target-execute-conflict', () => { + it('flags the string/string pair from #3713 and names both handlers', () => { + const stack = { actions: [action({ target: 'preferredHandler', execute: 'legacyHandler' })] }; + const findings = lintDeprecatedAliases(stack); + + expect(findings).toHaveLength(1); + const [f] = findings; + expect(f.rule).toBe(ACTION_TARGET_EXECUTE_CONFLICT); + // Advisory: the stack is well-defined and shippable — the cost is a handler + // that never runs, not a broken build. + expect(f.severity).toBe('warning'); + expect(f.where).toBe(`action 'convert'`); + // Both values must appear, or the author cannot tell WHICH handler they lost. + expect(f.message).toContain(`'preferredHandler'`); + expect(f.message).toContain(`'legacyHandler'`); + // …and the precedence has to be stated, not implied. + expect(f.message).toContain(`'target' wins`); + // The one-line fix (#3743 suggested shape). + expect(f.hint).toContain(`Delete 'execute'`); + }); + + it('carries object context for an action nested under its object', () => { + const stack = { + objects: [{ + name: 'crm_deal', + actions: [action({ target: 'preferredHandler', execute: 'legacyHandler' })], + }], + }; + const [f] = lintDeprecatedAliases(stack); + expect(f.where).toBe(`action 'convert' on object 'crm_deal'`); + }); + + it('reports ONE finding for an action that appears both top-level and nested', () => { + // The loader auto-populates `objects[*].actions` from `actions[*].objectName`, + // so the same authored action routinely shows up twice. One mistake, one line. + const both = action({ target: 'preferredHandler', execute: 'legacyHandler' }); + const stack = { + objects: [{ name: 'crm_deal', actions: [both] }], + actions: [both], + }; + const findings = lintDeprecatedAliases(stack); + expect(findings).toHaveLength(1); + // Object-nested is walked first, so the surviving finding keeps the context. + expect(findings[0].where).toBe(`action 'convert' on object 'crm_deal'`); + }); + + it('flags inline functions too — the silent discard is the same', () => { + const stack = { + actions: [action({ + target: function preferredHandler() { return 'preferred'; }, + execute: function legacyHandler() { return 'legacy'; }, + })], + }; + const [f] = lintDeprecatedAliases(stack); + expect(f.rule).toBe(ACTION_TARGET_EXECUTE_CONFLICT); + expect(f.message).toContain('an inline function'); + }); + + it('flags the mixed string-target / function-execute pair', () => { + // The combination #3742 left resolving the alias's way in `lowerCallables`. + // Both halves of #3743 meet here: the precedence fix makes `target` win, and + // this warning is what tells the author the inline function is gone. + const stack = { + actions: [action({ + target: 'preferredHandler', + execute: function legacyHandler() { return 'legacy'; }, + })], + }; + const [f] = lintDeprecatedAliases(stack); + expect(f.message).toContain(`'preferredHandler'`); + expect(f.message).toContain('an inline function'); + }); + + it('reads map-shaped action slots, injecting the map key as the name', () => { + const stack = { + objects: { + crm_deal: { + actions: { + convert: { label: 'Convert', type: 'script', target: 'preferredHandler', execute: 'legacyHandler' }, + }, + }, + }, + }; + const [f] = lintDeprecatedAliases(stack); + expect(f.where).toBe(`action 'convert' on object 'crm_deal'`); + }); + + it('reports every distinct offending action', () => { + const stack = { + actions: [ + action({ name: 'convert', target: 'a', execute: 'b' }), + action({ name: 'archive', target: 'c', execute: 'd' }), + action({ name: 'clean', target: 'e' }), + ], + }; + expect(lintDeprecatedAliases(stack).map((f) => f.where)).toEqual([ + `action 'convert'`, + `action 'archive'`, + ]); + }); +}); diff --git a/packages/spec/src/shared/deprecated-aliases.ts b/packages/spec/src/shared/deprecated-aliases.ts new file mode 100644 index 0000000000..e3f68c805d --- /dev/null +++ b/packages/spec/src/shared/deprecated-aliases.ts @@ -0,0 +1,147 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * PRE-PARSE authoring lint for DEPRECATED ALIASES (#3743). + * + * A deprecated alias is resolved by the parse itself: `ActionSchema`'s transform + * folds `execute` into `target` and DROPS it from the output (#3713 / #3742). So + * every check that runs on parsed metadata — the CLI's `lintFlowPatterns`, + * `lintViewRefs`, `@objectstack/lint`'s validators, a renderer, the runtime — is + * structurally blind to the one thing worth reporting here: that the author + * declared the alias at all, let alone that they put a *different* handler in it. + * + * That leaves exactly one window: **after `normalizeStackInput`, before the + * parse.** This module is the rule set for that window, kept in `spec` rather + * than in a lint package because the two places that own that window both live + * upstream of `@objectstack/lint`: + * + * 1. `defineStack` (`stack.zod.ts`) — the dominant authoring path, and the one + * that consumes the alias EARLIEST. It parses inside the author's own + * config module, so by the time `os build` loads that module the alias is + * already gone. A CLI-only warning would therefore never fire for a + * `defineStack` app — i.e. for almost every app. + * 2. The CLI's `os build` / `os validate` pre-parse pass — which is where a + * stack that did NOT go through strict `defineStack` gets caught: a plain + * object default-export, `defineStack(…, { strict: false })`, or an inline + * function handler (`z.string()` rejects those, so they cannot pass through + * strict `defineStack` at all and are lowered by the CLI instead). + * + * Each layer warns only for the discards IT performs, so an authored conflict + * produces exactly one warning no matter which path the stack takes. + * + * Rules: + * + * action-target-execute-conflict — WARNING + * An action declares BOTH the canonical `target` and its deprecated alias + * `execute`, with different values. `target` wins everywhere (#3742) and + * `execute` is discarded — silently, until now: the author wrote two + * handlers and one of them is thrown away with no signal (Prime Directive + * #12). Advisory rather than fatal, because the resulting stack is + * well-defined and shippable; the cost is a handler that never runs, not a + * broken build. Identical values in both slots are harmless duplication and + * stay quiet. + * + * Advisory-only today, but `severity` is modelled and honoured by every call + * site, so a future rule here can gate the build without rewiring any of them. + */ + +export interface DeprecatedAliasFinding { + /** Author-facing location, e.g. `action 'convert' on object 'crm_deal'`. */ + where: string; + /** What was discarded and why — states the precedence explicitly. */ + message: string; + /** The one-line fix. */ + hint: string; + /** Stable rule id, e.g. {@link ACTION_TARGET_EXECUTE_CONFLICT}. */ + rule: string; + /** + * `'error'` FAILS the build; `'warning'` (the default when absent) prints and + * continues. `os build` and `os validate` both filter on this field, so a rule + * promoted here gates both surfaces at once (#3782). + */ + severity?: 'error' | 'warning'; +} + +type AnyRec = Record; + +/** Normalise a record-or-map metadata slot into an array, injecting `name` from + * the map key (mirrors the helper in the CLI's sibling authoring lints). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + return []; +} + +export const ACTION_TARGET_EXECUTE_CONFLICT = 'action-target-execute-conflict'; + +/** A handler slot counts as declared when it holds a non-empty string ref or an + * inline callable. Both forms are authorable and both resolve by the same + * `target`-wins precedence. */ +function isDeclaredHandler(v: unknown): boolean { + return typeof v === 'function' || (typeof v === 'string' && v.length > 0); +} + +/** Render a handler slot for an author-facing message. An inline function has no + * useful printable value — naming the shape is what makes the message land. */ +function describeHandler(v: unknown): string { + return typeof v === 'function' ? 'an inline function' : `'${String(v)}'`; +} + +/** + * Collect deprecated-alias findings from a NORMALIZED (not yet parsed) stack. + * + * Pure and side-effect free: callers decide how to surface the findings — + * `defineStack` warns on the console, the CLI folds them into its authoring-lint + * block. Safe to call on a stack that still holds inline functions. + */ +export function lintDeprecatedAliases(stack: AnyRec): DeprecatedAliasFinding[] { + const findings: DeprecatedAliasFinding[] = []; + + // An action commonly appears BOTH top-level and nested under its object (the + // loader auto-populates `objects[*].actions` from `actions[*].objectName`), so + // dedupe by identity + both slot values: one authored mistake, one finding. + const seen = new Set(); + + const checkAction = (action: AnyRec, ownerObject?: string) => { + if (!action) return; + const { target, execute } = action; + if (!isDeclaredHandler(target) || !isDeclaredHandler(execute)) return; + // Same value in both slots (equal strings, or the very same function): the + // alias is redundant, not contradictory, and nothing the author wrote is + // lost. Staying quiet keeps the rule's signal-to-noise at 1. + if (target === execute) return; + + const actionName = typeof action.name === 'string' && action.name ? action.name : '(unnamed)'; + const dedupeKey = `${actionName} ${describeHandler(target)} ${describeHandler(execute)}`; + if (seen.has(dedupeKey)) return; + seen.add(dedupeKey); + + findings.push({ + where: ownerObject ? `action '${actionName}' on object '${ownerObject}'` : `action '${actionName}'`, + message: + `Action declares both 'target' (${describeHandler(target)}) and the deprecated alias 'execute' ` + + `(${describeHandler(execute)}). 'target' wins: 'execute' is dropped while the stack is compiled and ` + + `never reaches the runtime or a renderer, so ${describeHandler(execute)} never runs.`, + hint: + `Delete 'execute' — it is a deprecated alias of 'target', not a second handler. If ` + + `${describeHandler(execute)} is the handler you meant to bind, put it in 'target' instead.`, + rule: ACTION_TARGET_EXECUTE_CONFLICT, + severity: 'warning', + }); + }; + + // Object-nested first so the retained (deduped) finding keeps object context. + for (const obj of asArray(stack.objects)) { + const object = typeof obj.name === 'string' ? obj.name : undefined; + for (const action of asArray(obj.actions)) checkAction(action, object); + } + for (const action of asArray(stack.actions)) checkAction(action); + + return findings; +} + +/** One-line console rendering, shared so `defineStack` and any other + * console-bound caller phrase the warning identically. */ +export function formatDeprecatedAliasFinding(f: DeprecatedAliasFinding): string { + return `${f.where}: ${f.message}\n ${f.hint}\n rule: ${f.rule}`; +} diff --git a/packages/spec/src/stack.test.ts b/packages/spec/src/stack.test.ts index 3be7d5b883..41abc58cb8 100644 --- a/packages/spec/src/stack.test.ts +++ b/packages/spec/src/stack.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { ObjectStackDefinitionSchema, defineStack, @@ -1354,3 +1354,86 @@ describe('defineStack — at most one App per package (ADR-0019 D1/D3)', () => { ).not.toThrow(); }); }); + +// ── #3743: the alias the parse eats must be reported before it is eaten ── +// +// `defineStack` runs `ObjectStackDefinitionSchema.safeParse` INSIDE the author's +// own config module, and that parse folds `execute` into `target` and drops it +// (#3713/#3742). So this is the last moment the conflict exists at all: a lint +// anywhere downstream — the CLI's compile pipeline included — reads a stack that +// no longer has an `execute` key to complain about. Advisory, never fatal: the +// stack that comes out is well-defined, the author has just lost one of the two +// handlers they wrote. +describe('defineStack — deprecated alias warnings (#3743)', () => { + const manifest = { id: 'p', version: '1.0.0', type: 'app' as const, name: 'P', namespace: 'demo' }; + const obj = { name: 'demo_task', label: 'Task', fields: { title: { type: 'text' as const } } }; + const stackWith = (action: Record) => ({ + manifest, + objects: [obj], + actions: [{ label: 'A', type: 'script' as const, objectName: 'demo_task', ...action }], + }); + + // `warnDeprecatedAliases` nags once per DISTINCT conflict for the life of the + // process, so every case below uses its own action name / handler values. + let warn: ReturnType; + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + + it('warns — naming both handlers — when the two slots disagree', () => { + expect(() => + defineStack(stackWith({ name: 'conflicting_a', target: 'preferred_a', execute: 'legacy_a' })), + ).not.toThrow(); + + expect(warn).toHaveBeenCalledTimes(1); + const msg = warn.mock.calls[0][0] as string; + expect(msg).toContain('defineStack:'); + expect(msg).toContain(`'preferred_a'`); + expect(msg).toContain(`'legacy_a'`); + expect(msg).toContain('action-target-execute-conflict'); + }); + + it('still drops the alias and keeps `target` — the warning changes nothing', () => { + const stack = defineStack( + stackWith({ name: 'conflicting_b', target: 'preferred_b', execute: 'legacy_b' }), + ); + const action = (stack.actions as Array>)[0]; + expect(action.target).toBe('preferred_b'); + expect('execute' in action).toBe(false); + }); + + it('stays quiet when only one slot is declared', () => { + defineStack(stackWith({ name: 'canonical_only', target: 'preferred_c' })); + defineStack(stackWith({ name: 'alias_only', execute: 'legacy_c' })); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays quiet when both slots carry the same handler', () => { + defineStack(stackWith({ name: 'same_both', target: 'same_d', execute: 'same_d' })); + expect(warn).not.toHaveBeenCalled(); + }); + + it('nags once per distinct conflict, however often the stack is defined', () => { + const config = stackWith({ name: 'conflicting_e', target: 'preferred_e', execute: 'legacy_e' }); + defineStack(config); + defineStack(config); + defineStack(config); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('does not warn in non-strict mode — nothing has been discarded yet', () => { + // With no parse there is no discard: `execute` survives on the returned + // stack, and whichever layer eventually consumes it reports it. Warning here + // too would double-report the same authored mistake. + const stack = defineStack( + stackWith({ name: 'conflicting_f', target: 'preferred_f', execute: 'legacy_f' }), + { strict: false }, + ); + expect(warn).not.toHaveBeenCalled(); + const action = (stack.actions as Array>)[0]; + expect(action.execute).toBe('legacy_f'); + }); +}); diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index 400eb0fcba..c51cce1731 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -10,6 +10,7 @@ import { TranslationBundleSchema, TranslationConfigSchema } from './system/trans import { hasPlatformObjectPrefix } from './system/constants/platform-object-names'; import { objectStackErrorMap, formatZodError } from './shared/error-map.zod'; import { normalizeStackInput, type MetadataCollectionInput, type MapSupportedField } from './shared/metadata-collection.zod'; +import { lintDeprecatedAliases, formatDeprecatedAliasFinding } from './shared/deprecated-aliases'; // Data Protocol import { ObjectSchema, ObjectExtensionSchema } from './data/object.zod'; @@ -1044,6 +1045,29 @@ function validateKnownCapabilities(config: ObjectStackDefinition): string[] { return errors; } +/** + * Findings already reported this process, so a stack that is defined more than + * once (a dev-server reload, a config imported by several test files) nags once + * per distinct conflict. The key carries the action AND both slot values, so a + * genuinely different conflict is never suppressed. Mirrors the warn-once shape + * of `warnGenericPasswordFields` in `object.zod.ts`. + */ +const warnedAliasFindings = new Set(); + +/** + * [#3743] Surface every deprecated-alias conflict the parse is about to resolve + * silently. Advisory: this never throws, because the stack that comes out is + * well-defined — the author just loses one of the two handlers they wrote. + */ +function warnDeprecatedAliases(normalized: Record): void { + for (const finding of lintDeprecatedAliases(normalized)) { + const key = `${finding.rule}${finding.where}${finding.message}`; + if (warnedAliasFindings.has(key)) continue; + warnedAliasFindings.add(key); + console.warn(`defineStack: ${formatDeprecatedAliasFinding(finding)}`); + } +} + export function defineStack( config: ObjectStackDefinitionInput, options?: DefineStackOptions, @@ -1055,10 +1079,24 @@ export function defineStack( const normalized = normalizeStackInput(config as Record); if (!strict) { - // Non-strict mode: skip validation (advanced use cases only) + // Non-strict mode: skip validation (advanced use cases only). + // No alias warning here on purpose: with no parse there is no discard yet — + // the alias survives on the returned stack, and whichever layer eventually + // consumes it (the CLI's pre-parse pass on `os build`/`os validate`) is the + // one that reports it. Each layer warns for its OWN discards, so an authored + // conflict yields exactly one warning however the stack is compiled. return mergeActionsIntoObjects(normalized as ObjectStackDefinition); } + // [#3743] The LAST moment a deprecated alias is still visible. The parse below + // resolves `execute` into `target` and drops it (#3713/#3742), and this parse + // runs inside the author's own config module — so by the time `os build` reads + // the exported stack there is nothing left to lint. Warning here is what makes + // the discard visible to the author who wrote both handlers (Prime Directive + // #12); it is advisory and never blocks, since the resulting stack is + // well-defined — the cost is a handler that never runs. + warnDeprecatedAliases(normalized as Record); + // Strict mode (default): parse with custom error map, then cross-reference validate const result = ObjectStackDefinitionSchema.safeParse(normalized, { error: objectStackErrorMap, diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 66d67e3a95..76f15bc848 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -253,6 +253,9 @@ const TARGET_REQUIRED_TYPES: ReadonlySet = new Set( * If `execute` is provided without `target`, it is lowered into `target` at parse * time. Either way `execute` is **dropped from the parsed output** (#3713), so * every consumer reads one canonical slot; when both are declared, `target` wins. + * Declaring both with *different* values discards the `execute` handler, so it + * raises the advisory `action-target-execute-conflict` warning (#3743) — from + * `defineStack` and from `os build`/`os validate` — rather than passing silently. * * @example Good action names * - 'on_close_deal' From 6976bb906ee5409c8df5ee7d08a231d976f8e18c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 09:21:30 +0000 Subject: [PATCH 2/3] fix(spec): write the warn-once dedupe separator as an escape, not a raw NUL byte `check:nul-bytes` (#3127) caught two literal U+0000 bytes in the `warnDeprecatedAliases` key. A raw NUL makes ripgrep treat the whole file as binary and silently return zero matches, so `stack.zod.ts` would have dropped out of code search and every grep-based lint. Written as the unicode escape it is byte-identical at runtime. Same separator in `deprecated-aliases.ts`'s dedupe key, with a note on why it must stay an escape, and regenerate the spec API-surface snapshot for the four new exports (0 breaking, 4 added). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JqC9ckaLbmhmytt65Gxi96 --- packages/spec/api-surface.json | 4 ++++ packages/spec/src/shared/deprecated-aliases.ts | 5 ++++- packages/spec/src/stack.zod.ts | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 1cb5316e24..edd28227f2 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -1,5 +1,6 @@ { ".": [ + "ACTION_TARGET_EXECUTE_CONFLICT (const)", "ADMIN_FULL_ACCESS (const)", "ALL_CONVERSIONS (const)", "AUDIENCE_ANCHOR_POSITIONS (const)", @@ -35,6 +36,7 @@ "DatasourceMappingRule (type)", "DatasourceMappingRuleSchema (const)", "DefineStackOptions (interface)", + "DeprecatedAliasFinding (interface)", "EVERYONE_POSITION (const)", "EvalUser (type)", "EvalUserInput (type)", @@ -144,11 +146,13 @@ "expandViewContainerWithDiagnostics (function)", "expression (function)", "findClosestMatches (function)", + "formatDeprecatedAliasFinding (function)", "formatSuggestion (function)", "formatZodError (function)", "formatZodIssue (function)", "isAggregatedViewContainer (function)", "isKnownPlatformCapability (function)", + "lintDeprecatedAliases (function)", "mapMembershipRole (function)", "normalizeMetadataCollection (function)", "normalizePluginMetadata (function)", diff --git a/packages/spec/src/shared/deprecated-aliases.ts b/packages/spec/src/shared/deprecated-aliases.ts index e3f68c805d..2ad9d85211 100644 --- a/packages/spec/src/shared/deprecated-aliases.ts +++ b/packages/spec/src/shared/deprecated-aliases.ts @@ -112,7 +112,10 @@ export function lintDeprecatedAliases(stack: AnyRec): DeprecatedAliasFinding[] { if (target === execute) return; const actionName = typeof action.name === 'string' && action.name ? action.name : '(unnamed)'; - const dedupeKey = `${actionName} ${describeHandler(target)} ${describeHandler(execute)}`; + // `\u0000` written as an escape, never as a raw byte: a literal NUL makes + // ripgrep treat the whole file as binary and silently drop it from every + // grep-based lint (`scripts/check-nul-bytes.mjs` enforces this). + const dedupeKey = `${actionName}\u0000${describeHandler(target)}\u0000${describeHandler(execute)}`; if (seen.has(dedupeKey)) return; seen.add(dedupeKey); diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index c51cce1731..f9f58fe73c 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -1061,7 +1061,7 @@ const warnedAliasFindings = new Set(); */ function warnDeprecatedAliases(normalized: Record): void { for (const finding of lintDeprecatedAliases(normalized)) { - const key = `${finding.rule}${finding.where}${finding.message}`; + const key = `${finding.rule}\u0000${finding.where}\u0000${finding.message}`; if (warnedAliasFindings.has(key)) continue; warnedAliasFindings.add(key); console.warn(`defineStack: ${formatDeprecatedAliasFinding(finding)}`); From 99b3498508524cc8e67f62b0c60970dfd913eb34 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 09:22:30 +0000 Subject: [PATCH 3/3] docs(protocol): document the action-target-execute-conflict warning (#3743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The objectui actions reference already stated the precedence — "if an action declares both, `target` wins and `execute` is discarded" — while saying nothing about the author ever finding out. Name the rule, both surfaces that raise it, that it never fails the build, and the one-line fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JqC9ckaLbmhmytt65Gxi96 --- content/docs/protocol/objectui/actions.mdx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/content/docs/protocol/objectui/actions.mdx b/content/docs/protocol/objectui/actions.mdx index 4409d5eca0..c185bf628b 100644 --- a/content/docs/protocol/objectui/actions.mdx +++ b/content/docs/protocol/objectui/actions.mdx @@ -53,6 +53,8 @@ The `type` field selects how an action is dispatched. The complete enum is **`sc `target` is the canonical binding for every non-`script` type. The deprecated `execute` field is lowered into `target` during parsing and then **removed from the parsed metadata**, so every consumer — server dispatch and renderer alike — reads exactly one slot. If an action declares both, `target` wins and `execute` is discarded. +Declaring both with *different* values means one of the two handlers you wrote never runs, so it raises an advisory `action-target-execute-conflict` warning naming both — from `defineStack` when it parses your config, and from `os build` / `os validate` for stacks that skip strict `defineStack`. It never fails the build; the fix is to delete `execute`. The same value in both slots is harmless duplication and stays quiet. + ### Script Actions Run logic with no server round trip via `body` (an L1 CEL expression or L2 sandboxed JS). `body` is only meaningful when `type` is `script`, and declaring one on any other type is a parse-time error — those types dispatch on `target`, so the body would never be invoked.