diff --git a/.changeset/lint-action-dedup-composite-key.md b/.changeset/lint-action-dedup-composite-key.md new file mode 100644 index 0000000000..a580188acc --- /dev/null +++ b/.changeset/lint-action-dedup-composite-key.md @@ -0,0 +1,24 @@ +--- +'@objectstack/cli': patch +--- + +`os lint`: dedup actions on their real engine key, not the bare name + +`naming/namespace-prefix` deduplicated every bare-named type on `name` alone. +For actions that is not the key they occupy: the engine registers an action +under `objectName:name` (`ObjectQLPlugin.actionObjectKey`, with the canonical +object-less key `global` since #3913), so a package that declares one +`log_call` per object occupies one distinct key per object and nothing shadows +anything. + +The bare-name dedup therefore flagged that shape as an intra-package duplicate +and the noise grew linearly with the object count — 12 fixed warnings per run +on HotCRM (5 objects x 3 activity actions), where following the "rename one" +prescription would have broken the shared i18n keys the shape depends on. +Actions now dedup on `objectName:name`; the other six types keep bare-name +dedup and their message text verbatim. + +Genuine shadowing is still reported: two actions sharing one `objectName`, two +object-less actions sharing a name, and an action on an object literally named +`global` meeting an object-less one all still warn — with a remedy calibrated +for actions, which offers separating them by `objectName` before renaming. diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index 751c11da72..907af1be6c 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -278,42 +278,92 @@ export function lintConfig(config: any, opts: LintConfigOptions = {}): LintIssue // Bare-named UI/automation types that share the generic registry namespace. // Data-driven so a new bare-named type is one line. - const PREFIXED_TYPES: Array<{ key: string; label: string }> = [ + // + // `registryKey` maps an item to the key it ACTUALLY occupies at runtime, so + // the dedup asks "do these two collapse onto one key?" rather than "do they + // spell the same bare name?". For every type here the two questions coincide + // — except `actions`, whose engine key is composite (see below). + const PREFIXED_TYPES: Array<{ + key: string; + label: string; + registryKey?: (item: any, name: string) => string; + }> = [ { key: 'apps', label: 'App' }, { key: 'pages', label: 'Page' }, { key: 'dashboards', label: 'Dashboard' }, { key: 'flows', label: 'Flow' }, - { key: 'actions', label: 'Action' }, + // An action's engine registration key is `:`, NOT the + // bare name: `ObjectQLPlugin.actionObjectKey` (and the runtime's + // `standaloneActionObjectName`, kept in lockstep with it) resolve the + // object half to `objectName`, falling back to the canonical object-less + // key `'global'` (#3913). So one package legitimately declaring + // `log_call` on each of five objects occupies five distinct keys and + // nothing shadows anything — deduping those on the bare name produced 12 + // fixed false positives per `objectstack lint` run on HotCRM, growing + // linearly with the object count (#5510), and "just rename one" would have + // broken the shared i18n keys that shape depends on (#592). + // + // `'global'` rather than an inert sentinel like `''` is deliberate: it is + // the literal the engine really registers under, so an action declared on + // an object actually NAMED `global` and an object-less action of the same + // name collide for real — and are reported, as they must be. + // + // Only `objectName` is read. `object`/`entity` are rejected outright by + // `ActionSchema`'s strict shape with a rename prescription, so they never + // reach a spec-valid config and a `??` chain here would only fossilize a + // spelling the contract already refuses (Prime Directive #12). + { + key: 'actions', + label: 'Action', + registryKey: (item, name) => + `${typeof item?.objectName === 'string' && item.objectName ? item.objectName : 'global'}:${name}`, + }, { key: 'reports', label: 'Report' }, { key: 'datasets', label: 'Dataset' }, ]; - for (const { key, label } of PREFIXED_TYPES) { + for (const { key, label, registryKey } of PREFIXED_TYPES) { const items: any[] = Array.isArray(config[key]) ? config[key] : []; - // First occurrence of each name → its index, so a later duplicate can point - // back at the original declaration. + // First occurrence of each registry key → its index, so a later duplicate + // can point back at the original declaration. const firstSeen = new Map(); for (let i = 0; i < items.length; i++) { const name = items[i]?.name; if (typeof name !== 'string' || !name) continue; - const original = firstSeen.get(name); + const dedupKey = registryKey ? registryKey(items[i], name) : name; + const original = firstSeen.get(dedupKey); if (original === undefined) { - firstSeen.set(name, i); + firstSeen.set(dedupKey, i); continue; } - // Genuine intra-package duplicate: two items of the same (type, name) - // declared in this package's config. They collapse onto one registry key - // and shadow each other. Renaming one with the package namespace prefix - // (`crm_home`) is the simplest fix; any distinct name works. + // Genuine intra-package duplicate: two items landing on ONE registry key + // in this package's config. They shadow each other. Renaming one with the + // package namespace prefix (`crm_home`) is the simplest fix; any distinct + // name works. const suggestion = ns && !name.startsWith(`${ns}_`) ? `${ns}_${name}` : undefined; + // An action has a second, usually better remedy than renaming: the two + // declarations collide only because they agree on `objectName` (or both + // omit it and fall to `global`), so pointing one at the object it really + // belongs to separates them while keeping the shared name — the exact + // move the bare-name dedup used to punish. + const remedy = + key === 'actions' + ? `give one a distinct \`objectName\` (same-named actions on DIFFERENT objects ` + + `never collide) or rename one${suggestion ? `, e.g. "${suggestion}"` : ''}` + : `rename one${suggestion ? `, e.g. "${suggestion}"` : ''}`; + const collapseText = + key === 'actions' + ? `Two actions sharing one \`objectName\` (or both object-less) collapse onto ` + + `the same \`objectName:name\` engine key and shadow each other` + : `Two items of the same type sharing a bare name within one package ` + + `shadow each other on the registry key`; issues.push({ severity: 'warning', rule: 'naming/namespace-prefix', message: `${label} "${name}" is declared more than once in this package ` + - `(also at ${key}[${original}].name). Two items of the same type sharing ` + - `a bare name within one package shadow each other on the registry key ` + - `(ADR-0048 §3.4) — rename one${suggestion ? `, e.g. "${suggestion}"` : ''}. ` + + `(also at ${key}[${original}].name). ${collapseText} ` + + `(ADR-0048 §3.4) — ${remedy}. ` + `Distinct packages may reuse the same name freely; the namespace prefix ` + `is an optional convention, not a collision-avoidance requirement.`, path: `${key}[${i}].name`, diff --git a/packages/cli/test/lint-namespace-prefix.test.ts b/packages/cli/test/lint-namespace-prefix.test.ts index 2e04e437e1..7c570629e8 100644 --- a/packages/cli/test/lint-namespace-prefix.test.ts +++ b/packages/cli/test/lint-namespace-prefix.test.ts @@ -1,6 +1,8 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, expect, it } from 'vitest'; +import { normalizeStackInput } from '@objectstack/spec'; +import { ActionSchema } from '@objectstack/spec/ui'; import { lintConfig } from '../src/commands/lint'; const RULE = 'naming/namespace-prefix'; @@ -53,6 +55,24 @@ describe('lint — intra-package duplicate-name advisory (ADR-0048 §3.4)', () = expect(issues[0].message).not.toContain('Two packages'); }); + it('keeps the non-action wording verbatim — only actions got a calibrated remedy', () => { + // #5510 rewrote the message assembly to branch on `actions`. The other six + // types must be byte-identical to what they printed before, so this pins + // the full sentence rather than a substring of it. + const issues = prefixIssues({ + manifest: { namespace: 'crm' }, + pages: [{ name: 'home' }, { name: 'home' }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).toBe( + 'Page "home" is declared more than once in this package (also at pages[0].name). ' + + 'Two items of the same type sharing a bare name within one package shadow each ' + + 'other on the registry key (ADR-0048 §3.4) — rename one, e.g. "crm_home". ' + + 'Distinct packages may reuse the same name freely; the namespace prefix is an ' + + 'optional convention, not a collision-avoidance requirement.', + ); + }); + it('detects duplicates per type independently across every bare-named type', () => { const issues = prefixIssues({ manifest: { namespace: 'crm' }, @@ -103,3 +123,142 @@ describe('lint — intra-package duplicate-name advisory (ADR-0048 §3.4)', () = expect(issues).toHaveLength(0); }); }); + +describe('lint — actions dedup on the composite engine key, not the bare name (#5510)', () => { + // The engine registers an action under `:` + // (`ObjectQLPlugin.actionObjectKey`; the runtime's + // `standaloneActionObjectName` is kept in lockstep with it), with the + // canonical object-less key `'global'` (#3913). Deduping on the bare name + // asked a question the registry never asks. + const ACTIVITY_ACTIONS = ['log_call', 'log_meeting', 'schedule_meeting']; + const CRM_OBJECTS = ['crm_lead', 'crm_contact', 'crm_account', 'crm_opportunity', 'crm_case']; + + it('stays silent on the HotCRM shape: one same-named action per object', () => { + // The reported regression, at full size: 5 objects × 3 activity actions = + // 15 declarations occupying 15 distinct keys. rc.2 emitted 12 warnings + // here (first of each name silent, every later one flagged) and its + // "rename one" prescription would have broken the shared i18n keys the + // #592 shape depends on. + const actions = CRM_OBJECTS.flatMap((objectName) => + ACTIVITY_ACTIONS.map((name) => ({ name, objectName, label: name })), + ); + expect(actions).toHaveLength(15); + expect(prefixIssues({ manifest: { namespace: 'crm' }, actions })).toHaveLength(0); + }); + + it('still warns when two actions genuinely share one objectName', () => { + const issues = prefixIssues({ + manifest: { namespace: 'crm' }, + actions: [ + { name: 'log_call', objectName: 'crm_lead' }, + { name: 'log_call', objectName: 'crm_contact' }, + { name: 'log_call', objectName: 'crm_lead' }, + ], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + // Points at the real duplicate (index 2) and back at the first declaration + // that holds `crm_lead:log_call` (index 0) — never at the innocent + // `crm_contact` row in between. + expect(issues[0].path).toBe('actions[2].name'); + expect(issues[0].message).toContain('actions[0].name'); + expect(issues[0].message).not.toContain('actions[1].name'); + }); + + it('still warns when two object-less actions share a name', () => { + const issues = prefixIssues({ + manifest: { namespace: 'crm' }, + actions: [{ name: 'export_all' }, { name: 'export_all' }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('actions[1].name'); + }); + + it('warns when an action on an object literally named `global` meets an object-less one', () => { + // The object half falls back to the LITERAL `'global'` (#3913), not to an + // inert sentinel — so these two really do collapse onto `global:sync` in + // the engine's exact-string map, and the lint must say so. An `objectName + // ?? ''` key would have missed this pair. + const issues = prefixIssues({ + actions: [{ name: 'sync', objectName: 'global' }, { name: 'sync' }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('actions[1].name'); + }); + + it('prescribes objectName separation first, not a bare rename', () => { + const issues = prefixIssues({ + manifest: { namespace: 'crm' }, + actions: [ + { name: 'log_call', objectName: 'crm_lead' }, + { name: 'log_call', objectName: 'crm_lead' }, + ], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain('distinct `objectName`'); + expect(issues[0].message).toContain('DIFFERENT objects'); + expect(issues[0].message).toContain('objectName:name'); + // Renaming stays on offer as the secondary remedy, with its suggestion. + expect(issues[0].message).toContain('or rename one, e.g. "crm_log_call"'); + expect(issues[0].fix).toBe('crm_log_call'); + }); + + it('holds through the real `os lint` seam (normalizeStackInput → lintConfig)', () => { + // The unit cases above feed `lintConfig` a raw object; the command feeds it + // a `normalizeStackInput` result. This pins that the normalization the + // reported repro actually went through neither relocates the top-level + // `actions` array (the warnings were reported at `actions[N].name`) nor + // drops the `objectName` the dedup key now depends on. + const actions = CRM_OBJECTS.flatMap((objectName) => + ACTIVITY_ACTIONS.map((name) => ({ + name, + objectName, + label: name, + type: 'script' as const, + })), + ); + const normalized: any = normalizeStackInput({ manifest: { namespace: 'crm' }, actions } as any); + expect(normalized.actions).toHaveLength(15); + expect(normalized.actions[0].objectName).toBe('crm_lead'); + expect(prefixIssues(normalized)).toHaveLength(0); + }); + + it('reads only `objectName` — `object` is not an authoring surface at all', () => { + // Guarding a KEY's reachability, so the instrument is the schema's own + // verdict on the key: `ActionSchema` rejects `object` outright with a + // rename prescription onto `objectName`. That is why the dedup key reads + // `objectName` alone — a `?? item.object` chain here would only fossilize + // a spelling the contract already refuses (Prime Directive #12). + const parsed = ActionSchema.safeParse({ + name: 'log_call', + label: 'Log call', + type: 'script', + object: 'crm_lead', + }); + expect(parsed.success).toBe(false); + const unrecognized = parsed.success + ? [] + : parsed.error.issues.filter((i: any) => i.code === 'unrecognized_keys'); + expect(unrecognized).toHaveLength(1); + expect((unrecognized[0] as any).keys).toEqual(['object']); + expect(unrecognized[0].message).toContain('objectName'); + }); + + it('does not leak the composite key into the other bare-named types', () => { + // `objectName` is meaningless on a page/flow/report; carrying one must not + // split their dedup. These stay bare-name duplicates and stay warned. + const issues = prefixIssues({ + manifest: { namespace: 'crm' }, + pages: [ + { name: 'home', objectName: 'crm_lead' }, + { name: 'home', objectName: 'crm_contact' }, + ], + flows: [ + { name: 'onboard', objectName: 'crm_lead' }, + { name: 'onboard', objectName: 'crm_contact' }, + ], + }); + expect(issues).toHaveLength(2); + expect(issues.map((i) => i.path)).toEqual(['pages[1].name', 'flows[1].name']); + }); +});