fix(cli): lint 的 action 去重改用复合注册键,不再按裸 name 误判 (#5510)

.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.

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 `<objectName>:<name>`, 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<string, number>();
     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`,

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 `<objectName>:<name>`
+  // (`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']);
+  });
+});