Skip to content

Commit bbd8517

Browse files
os-zhuangclaude
andcommitted
fix(ADR-0048): rescope os lint naming/namespace-prefix to intra-package duplicates
ADR-0048 §3.4 retired the per-item cross-package collision throw, but the naming/namespace-prefix lint rule was never updated to match. It fired on every bare-named UI/automation item (hotcrm: 63 false positives) and claimed the package would "collide on the registry key and fail at install" — both false. The rule now warns only on a genuine intra-package duplicate (type, name) pair within the linted config (the authoring-hygiene case §3.4 leaves to os lint). Unique bare names produce zero warnings. Message no longer claims an install failure. Runtime/registry behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f1d7bee commit bbd8517

3 files changed

Lines changed: 156 additions & 74 deletions

File tree

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(ADR-0048): rescope the `os lint` `naming/namespace-prefix` rule to intra-package duplicates
6+
7+
ADR-0048 §3.4 retired the per-item cross-package collision throw — two
8+
installed packages may legitimately ship the same bare name (e.g. `page/home`),
9+
stored under distinct composite keys and disambiguated by package-scoped
10+
resolution. The `naming/namespace-prefix` lint rule was never updated to match,
11+
so it still:
12+
13+
- **fired on every bare-named UI/automation item** (apps/pages/dashboards/flows/
14+
actions/reports/datasets) regardless of whether a duplicate existed — a normal
15+
single-package app got dozens of false positives (hotcrm: 63), and
16+
- **claimed the package would "collide on the registry key and fail at install"**,
17+
which is no longer true.
18+
19+
The rule now warns **only on a genuine intra-package duplicate `(type, name)`
20+
pair** within the linted config — the narrow authoring-time hygiene case ADR-0048
21+
§3.4 explicitly leaves to `os lint` ("an author shipping two `page/home` in one
22+
package"). A unique bare name produces zero warnings. The message no longer
23+
claims an install failure; it explains the items shadow each other on the
24+
registry key and that distinct packages may reuse the same name freely (the
25+
namespace prefix is an optional convention). Runtime/registry behavior is
26+
unchanged.

packages/cli/src/commands/lint.ts

Lines changed: 60 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -216,48 +216,68 @@ export function lintConfig(config: any): LintIssue[] {
216216
}
217217
}
218218

219-
// ── Namespace-prefix advisory for bare-named metadata (ADR-0048 §3.3) ──
220-
// Cross-package collision detection (ADR-0048) makes a clash between two
221-
// packages' bare-named items (`page`/`flow`/`action`/…) a loud error at
222-
// registration time. This shifts that left: a non-fatal nudge to prefix
223-
// the name with the package namespace at authoring time, so the clash is
224-
// unlikely to ever happen. Objects are already prefix-*enforced* (error)
225-
// in defineStack; views are object-derived; `doc` has its own build lint
226-
// — so they are excluded here. Warning only — prefix stays a recommended
227-
// convention for legacy types, not a retroactive requirement.
219+
// ── Intra-package duplicate-name advisory (ADR-0048 §3.4) ──
220+
// ADR-0048 §3.4 retired the per-item CROSS-package throw: package ids are
221+
// globally unique, so two installed packages shipping the same bare name
222+
// (e.g. `page/home`) legitimately COEXIST under distinct composite keys and
223+
// each caller resolves to its own via package-scoped resolution. A bare name
224+
// is therefore NOT a collision risk and must not warn on its own.
225+
//
226+
// What the lint still earns its keep on is the narrow authoring-time hygiene
227+
// case the ADR explicitly leaves to `os lint`: "an author shipping two
228+
// `page/home` in one package". Two items of the same (type, name) declared
229+
// within ONE package's config share a single composite registry key and
230+
// shadow each other (last-write-wins). We only see one package's config here,
231+
// so the legitimate signal is a genuine duplicate `(type, name)` pair within
232+
// it — never a unique bare name.
233+
//
234+
// Objects are already prefix-*enforced* (error) in defineStack; views are
235+
// object-derived; `doc` has its own build lint — so they are excluded here.
228236
const ns: string | undefined = config.manifest?.namespace;
229-
if (ns) {
230-
// A name is namespace-safe if it IS the namespace (ADR-0019: a package's
231-
// single app is conventionally named after the namespace, e.g. `crm`),
232-
// is prefixed with it (`crm_lead`), or is a platform-reserved `sys_` name.
233-
const isNamespaceSafe = (name: string) =>
234-
name === ns || name.startsWith(`${ns}_`) || name.startsWith('sys_');
235-
236-
// Bare-named UI/automation types subject to the cross-package collision
237-
// (ADR-0048 §1.1). Data-driven so a new bare-named type is one line.
238-
const PREFIXED_TYPES: Array<{ key: string; label: string }> = [
239-
{ key: 'apps', label: 'App' },
240-
{ key: 'pages', label: 'Page' },
241-
{ key: 'dashboards', label: 'Dashboard' },
242-
{ key: 'flows', label: 'Flow' },
243-
{ key: 'actions', label: 'Action' },
244-
{ key: 'reports', label: 'Report' },
245-
{ key: 'datasets', label: 'Dataset' },
246-
];
247-
248-
for (const { key, label } of PREFIXED_TYPES) {
249-
const items: any[] = Array.isArray(config[key]) ? config[key] : [];
250-
for (let i = 0; i < items.length; i++) {
251-
const name = items[i]?.name;
252-
if (typeof name !== 'string' || !name || isNamespaceSafe(name)) continue;
253-
issues.push({
254-
severity: 'warning',
255-
rule: 'naming/namespace-prefix',
256-
message: `${label} "${name}" is not namespace-prefixed. Two packages defining "${name}" collide on the registry key and fail at install (ADR-0048). Rename to "${ns}_${name}".`,
257-
path: `${key}[${i}].name`,
258-
fix: `${ns}_${name}`,
259-
});
237+
238+
// Bare-named UI/automation types that share the generic registry namespace.
239+
// Data-driven so a new bare-named type is one line.
240+
const PREFIXED_TYPES: Array<{ key: string; label: string }> = [
241+
{ key: 'apps', label: 'App' },
242+
{ key: 'pages', label: 'Page' },
243+
{ key: 'dashboards', label: 'Dashboard' },
244+
{ key: 'flows', label: 'Flow' },
245+
{ key: 'actions', label: 'Action' },
246+
{ key: 'reports', label: 'Report' },
247+
{ key: 'datasets', label: 'Dataset' },
248+
];
249+
250+
for (const { key, label } of PREFIXED_TYPES) {
251+
const items: any[] = Array.isArray(config[key]) ? config[key] : [];
252+
// First occurrence of each name → its index, so a later duplicate can point
253+
// back at the original declaration.
254+
const firstSeen = new Map<string, number>();
255+
for (let i = 0; i < items.length; i++) {
256+
const name = items[i]?.name;
257+
if (typeof name !== 'string' || !name) continue;
258+
const original = firstSeen.get(name);
259+
if (original === undefined) {
260+
firstSeen.set(name, i);
261+
continue;
260262
}
263+
// Genuine intra-package duplicate: two items of the same (type, name)
264+
// declared in this package's config. They collapse onto one registry key
265+
// and shadow each other. Renaming one with the package namespace prefix
266+
// (`crm_home`) is the simplest fix; any distinct name works.
267+
const suggestion = ns && !name.startsWith(`${ns}_`) ? `${ns}_${name}` : undefined;
268+
issues.push({
269+
severity: 'warning',
270+
rule: 'naming/namespace-prefix',
271+
message:
272+
`${label} "${name}" is declared more than once in this package ` +
273+
`(also at ${key}[${original}].name). Two items of the same type sharing ` +
274+
`a bare name within one package shadow each other on the registry key ` +
275+
`(ADR-0048 §3.4) — rename one${suggestion ? `, e.g. "${suggestion}"` : ''}. ` +
276+
`Distinct packages may reuse the same name freely; the namespace prefix ` +
277+
`is an optional convention, not a collision-avoidance requirement.`,
278+
path: `${key}[${i}].name`,
279+
...(suggestion ? { fix: suggestion } : {}),
280+
});
261281
}
262282
}
263283

packages/cli/test/lint-namespace-prefix.test.ts

Lines changed: 70 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -6,63 +6,99 @@ import { lintConfig } from '../src/commands/lint';
66
const RULE = 'naming/namespace-prefix';
77
const prefixIssues = (config: any) => lintConfig(config).filter((i) => i.rule === RULE);
88

9-
describe('lint — namespace-prefix advisory (ADR-0048 §3.3)', () => {
10-
it('warns on a bare-named page without the namespace prefix', () => {
9+
describe('lint — intra-package duplicate-name advisory (ADR-0048 §3.4)', () => {
10+
it('stays silent on unique bare names — they are NOT a collision (ADR-0048 §3.4)', () => {
11+
// The cross-package throw was retired: distinct packages coexist on the
12+
// same bare name via package-scoped resolution. A single package with
13+
// unique bare names must produce zero warnings (was 63 false positives
14+
// for hotcrm under the old over-broad rule).
1115
const issues = prefixIssues({
1216
manifest: { namespace: 'crm' },
13-
pages: [{ name: 'home', label: 'Home' }],
17+
apps: [{ name: 'crm' }],
18+
pages: [{ name: 'home' }, { name: 'settings' }],
19+
dashboards: [{ name: 'overview' }],
20+
flows: [{ name: 'onboard' }],
21+
actions: [{ name: 'send' }],
22+
reports: [{ name: 'pipeline' }],
23+
datasets: [{ name: 'sales' }],
24+
});
25+
expect(issues).toHaveLength(0);
26+
});
27+
28+
it('warns on an actual intra-package duplicate (type, name) pair', () => {
29+
const issues = prefixIssues({
30+
manifest: { namespace: 'crm' },
31+
pages: [{ name: 'home', label: 'Home' }, { name: 'home', label: 'Home 2' }],
1432
});
1533
expect(issues).toHaveLength(1);
1634
expect(issues[0].severity).toBe('warning');
17-
expect(issues[0].path).toBe('pages[0].name');
18-
expect(issues[0].fix).toBe('crm_home');
35+
// The warning points at the duplicate occurrence, not the first.
36+
expect(issues[0].path).toBe('pages[1].name');
37+
expect(issues[0].message).toContain('declared more than once');
38+
expect(issues[0].message).toContain('pages[0].name');
1939
expect(issues[0].message).toContain('ADR-0048');
40+
// Suggests a namespace-prefixed rename when a namespace is available.
41+
expect(issues[0].fix).toBe('crm_home');
2042
});
2143

22-
it('accepts a namespace-prefixed name', () => {
23-
expect(
24-
prefixIssues({ manifest: { namespace: 'crm' }, flows: [{ name: 'crm_onboarding' }] }),
25-
).toHaveLength(0);
44+
it('does not claim the package will fail at install', () => {
45+
const issues = prefixIssues({
46+
manifest: { namespace: 'crm' },
47+
flows: [{ name: 'onboard' }, { name: 'onboard' }],
48+
});
49+
expect(issues).toHaveLength(1);
50+
// ADR-0048 §3.4 retired the per-item cross-package throw — the old
51+
// "collide on the registry key and fail at install" claim is false.
52+
expect(issues[0].message).not.toContain('fail at install');
53+
expect(issues[0].message).not.toContain('Two packages');
2654
});
2755

28-
it('exempts an app named after the namespace (ADR-0019 single-app convention)', () => {
29-
// `defineApp({ name: 'crm' })` in namespace `crm` must NOT warn.
30-
expect(
31-
prefixIssues({ manifest: { namespace: 'crm' }, apps: [{ name: 'crm' }] }),
32-
).toHaveLength(0);
56+
it('detects duplicates per type independently across every bare-named type', () => {
57+
const issues = prefixIssues({
58+
manifest: { namespace: 'crm' },
59+
apps: [{ name: 'a' }, { name: 'a' }],
60+
pages: [{ name: 'p' }, { name: 'p' }],
61+
dashboards: [{ name: 'd' }, { name: 'd' }],
62+
flows: [{ name: 'f' }, { name: 'f' }],
63+
actions: [{ name: 'ac' }, { name: 'ac' }],
64+
reports: [{ name: 'r' }, { name: 'r' }],
65+
datasets: [{ name: 'ds' }, { name: 'ds' }],
66+
});
67+
expect(issues).toHaveLength(7);
68+
expect(new Set(issues.map((i) => i.severity))).toEqual(new Set(['warning']));
3369
});
3470

35-
it('exempts platform-reserved sys_ names', () => {
71+
it('treats the same name under different types as distinct (no false positive)', () => {
72+
// `page/home` and `flow/home` live under different registry collections —
73+
// they do not collide, so a shared name across types must not warn.
3674
expect(
37-
prefixIssues({ manifest: { namespace: 'crm' }, pages: [{ name: 'sys_admin' }] }),
75+
prefixIssues({
76+
manifest: { namespace: 'crm' },
77+
pages: [{ name: 'home' }],
78+
flows: [{ name: 'home' }],
79+
}),
3880
).toHaveLength(0);
3981
});
4082

41-
it('covers every bare-named UI/automation type', () => {
83+
it('warns on duplicates even when no namespace is declared (omits the fix)', () => {
84+
// Duplicate names shadow each other regardless of namespace; without a
85+
// namespace there is no prefix to suggest, so no `fix` is offered.
4286
const issues = prefixIssues({
43-
manifest: { namespace: 'crm' },
44-
apps: [{ name: 'other' }],
45-
pages: [{ name: 'home' }],
46-
dashboards: [{ name: 'overview' }],
47-
flows: [{ name: 'onboard' }],
48-
actions: [{ name: 'send' }],
49-
reports: [{ name: 'pipeline' }],
50-
datasets: [{ name: 'sales' }],
87+
pages: [{ name: 'home' }, { name: 'home' }],
5188
});
52-
expect(issues).toHaveLength(7);
53-
expect(new Set(issues.map((i) => i.severity))).toEqual(new Set(['warning']));
54-
});
55-
56-
it('is silent when the package declares no namespace', () => {
57-
// No manifest.namespace → nothing to prefix against; stays quiet.
58-
expect(prefixIssues({ pages: [{ name: 'home' }] })).toHaveLength(0);
89+
expect(issues).toHaveLength(1);
90+
expect(issues[0].fix).toBeUndefined();
91+
expect(issues[0].message).toContain('declared more than once');
5992
});
6093

6194
it('does not touch objects (already prefix-enforced as an error) or views', () => {
6295
const issues = prefixIssues({
6396
manifest: { namespace: 'crm' },
64-
objects: [{ name: 'lead', fields: { id: { type: 'text' } } }],
65-
views: [{ name: 'lead.all' }],
97+
objects: [
98+
{ name: 'lead', fields: { id: { type: 'text' } } },
99+
{ name: 'lead', fields: { id: { type: 'text' } } },
100+
],
101+
views: [{ name: 'lead.all' }, { name: 'lead.all' }],
66102
});
67103
expect(issues).toHaveLength(0);
68104
});

0 commit comments

Comments
 (0)