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/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. 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/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/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..2ad9d85211 --- /dev/null +++ b/packages/spec/src/shared/deprecated-aliases.ts @@ -0,0 +1,150 @@ +// 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)'; + // `\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); + + 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..f9f58fe73c 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}\u0000${finding.where}\u0000${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'