Skip to content

Commit 45b90b6

Browse files
claude[bot]claude
andauthored
fix(cli): evaluate os lint's duplicate-name advisory per package (#17952)
Fixes #17821 `os lint`'s `naming/namespace-prefix` advisory read ONE flattened array per collection key — `lint.ts:428`, `const items: any[] = Array.isArray(stack[key]) ? stack[key] : [];` — with no package boundary, while its own comment at `lint.ts:362` asserted "We only see one package's config here". On a composed multi-package project, which is exactly the shape `composeStacks(…, { manifest: 'preserve' })` emits, that array carries every package's items. So two packages that each legitimately declare `home` were reported as one package declaring it twice, prescribing a rename of a name that was already correct — with the OTHER package's namespace as the suggested prefix — under a closing sentence saying distinct packages may reuse a name freely. ## What this does ADR-0130 D4/D5 already ruled that artifacts register per package, and `compile.ts` step 3b-ii runs the author-time rule table once per package through `artifactPackages` + `packageBodyAsStack`. This ports that ruled shape into the entry that missed it; it does not invent a new one. Those two functions move verbatim to `packages/cli/src/utils/artifact-packages.ts`, and `lintConfig` calls the same pair. The alternative to a shared module was importing one oclif command from another — which would pull `compile.ts`' whole module graph, the lowerer and the docs sweep included, into every `os lint` invocation — or a second copy of the package-id rule, free to drift from the one the runtime registers under. ⛔ Not a narrowing, an opt-out, or a skip. `compile.ts:118`'s fence — "This is NOT 'skip the site per package' — that would silence the gate" — binds here too. Every finding the advisory exists to raise is still raised, on the package that owns it. What changed is that a name is judged against its OWN package's declarations instead of against the union of every package's. The defect was missing attribution, never the signal. Fewer than two packages keeps today's read verbatim: a stack with no `packages[]` is one package by definition, and with exactly one the flattened top level carries that package's collections and nothing else, in its own order. With two or more, the flattened top level mixes packages, so the path is written whole (`packages[1].manifest.apps[1].name`) and resolves in BOTH ADR-0130 D4 shapes — a bare `apps[1]` would, on the additive shape, point at a different item than the one being reported. ## Measured — both directions, both stack shapes Through the real binary (`node packages/cli/bin/run.js lint`), `os lint` exit **0** on every row. Before: `origin/main` `84e6b05b6`. After: this branch at `7ccfdd189`. Count is `naming/namespace-prefix` warnings. | fixture | stack shape | before | after | | |:--|:--|--:|--:|:--| | two packages each declaring `home` (the card's repro) | additive | 1 | 0 | false positive, removed | | two packages each declaring `home` (the card's repro) | `packages[]`-only | 1 | 0 | false positive, removed | | two `home` apps inside ONE package | additive | 1 | 1 | true positive, kept | | two `home` apps inside ONE package | `packages[]`-only | 1 | 1 | true positive, kept | | one `log_call` action per package, same `objectName` | additive | 1 | 0 | false positive, removed | | two `log_call` actions on one `objectName`, one package | additive | 1 | 1 | true positive, kept | ⛔ The true-positive rows are not decoration: a run of the false-positive side alone is satisfied by an advisory that was simply switched off, and that is the outcome the fence above names. The prescription is fixed in the same motion. The surviving true positive used to read `rename one, e.g. "alpha_portal"` — `alpha` being the namespace of the package that does not own the reported item — and now reads `"beta_portal"`, because `ns` is read off the same package stack the items came from. Both shapes are live today: #17528 landed (`8305ad6df`) and folds an option-B stack before the hand-written checks run, so the card's "after #17528" column is the present tense and repairing only the additive shape would have been half a fix. ## Ablations Both legs mutate `packages/cli/src/commands/lint.ts` on disk, prove the bytes changed (`git hash-object` before vs after, and an anchor occurrence count), run the suites, then restore with `git checkout HEAD -- …` and prove the restore (`git diff HEAD` empty, blob hash equal to `HEAD:packages/cli/src/commands/lint.ts`). The mutated module is imported by the tests through a relative specifier into `src/`, so no `dist/` is on this consumption path. 1. **Revert the per-package dispatch** (`packageEntries.length > 1` becomes `> 999`, i.e. always the union leg): 12 of the 15 new pins go red, and BOTH pre-existing files — `lint-namespace-prefix.test.ts` and `lint-handwritten-checks-package-fold.test.ts` — stay green. That green is why the new file had to exist: the old pins are all single-package and cannot see this axis at all. 2. **Silence the advisory** (early `return issues;`): every TRUE-POSITIVE row goes red while every "THE CARD'S REPRO" row stays GREEN. That is the signature of the outcome the fence forbids, and the pair is what detects it. ## Verification - `pnpm --filter '@objectstack/cli^...' build` — exit 0. - `pnpm --filter @objectstack/cli typecheck` — exit 0 (includes `check:test-typecheck`). - `pnpm --filter @objectstack/cli exec vitest run --project unit` — **205 files / 2950 tests passed**. The `integration` tier is declared to CI: the diff touches no spawn entry, no `test/helpers/serve-process.ts`, and no driver/kernel boot path. - `pnpm lint` (repo-wide, `eslint . --no-inline-config`) — exit 0, 96s. Not narrowed. - Gate families derived with `node scripts/pm/dispatch-gates.mjs` from this tree: **62 derived, 62 run, 0 NOT MEASURED, 0 UNRUN**, every one recording exit 0 (reconciled with `--ran`). - ⚠️ NOT MEASURED: `origin/main` has moved to `bd25e897d` since this branch's base, and `scripts/check-cli-test-child-env.mjs` changed there — the local run read this tree's copy. CI runs the current one. Clause-②: no `packages/cli/src/utils/artifact-packages.ts` is an internal util. It is re-exported by none of the package's three published entry points (`.`, `./console`, `./hook-body`), and `pnpm check:published-files` confirms no resolvable surface moved. ## Acceptance notes Observations found while working here, none filed, each with the reason: - `packages/cli/src/utils/nav-contribution-groups.ts` exports `artifactPackagesOf`, a second implementation of the same package-id rule (`manifest.id`, then `name`, then the positional spelling) whose own docblock says it "Mirrors `compile.ts`' `artifactPackages` id rule". Folding it onto the module this PR adds is mechanical, but it is a different defect class from this card and the file carries its own pins — noted, not filed. Prospective handler: the next PR in `nav-contribution-groups.ts`. - With two or more packages, a declaration sitting at the flattened top level that no `packages[]` entry carries is outside every registration unit and is judged by neither leg. `composeStacks` cannot emit that shape (its flattened top level is exactly the union of the packages it composed), so it is a hand-authored-only shape. Stated in the code comment rather than filed; prospective handler: none. - Every other check in `lintConfig` reports an index into the folded stack, which on a `packages[]`-only project is a synthetic coordinate that resolves nowhere in the author's file. That is pre-existing (it arrived with the #17528 fold) and untouched here; this advisory is now the one rule whose multi-package path resolves. Prospective handler: none — noted, not filed. --- _Generated by [Claude Code](https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 38472ce commit 45b90b6

5 files changed

Lines changed: 591 additions & 201 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/cli': patch
3+
---
4+
5+
`os lint`: evaluate the `naming/namespace-prefix` duplicate advisory per package.
6+
7+
The advisory read one flattened array per collection key with no package boundary, so on a
8+
composed multi-package project two packages that each legitimately declare the same bare name
9+
(e.g. `home`) were reported as one package declaring it twice — prescribing a rename of a name
10+
that was already correct, with the OTHER package's namespace as the suggested prefix, under a
11+
closing sentence saying distinct packages may reuse a name freely. Both ADR-0130 D4 stack shapes
12+
were affected (flattened-plus-`packages[]`, and `packages[]`-only).
13+
14+
ADR-0130 D4/D5 registers artifacts per package, so the advisory now runs once per package —
15+
the same shape `os build` has used for the author-time rule table — and a genuine duplicate
16+
inside one package still warns, with the suggestion taken from that package's own namespace and
17+
a path written whole (`packages[1].manifest.apps[1].name`) so it resolves in either shape. A
18+
single-package project is judged exactly as before.

‎packages/cli/src/commands/compile.ts‎

Lines changed: 1 addition & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
import { loadConfig } from '../utils/config.js';
1717
import { lowerCallables } from '../utils/lower-callables.js';
1818
import { authoringRuleUnionStack } from '../utils/stack-collections.js';
19+
import { artifactPackages, packageBodyAsStack } from '../utils/artifact-packages.js';
1920
import { buildAccessMatrix, diffAccessMatrix } from '@objectstack/lint';
2021
import { runAuthoringRules, splitBySeverity, authoringRulesFor } from '@objectstack/lint';
2122
import { resolveSduiManifest } from '../utils/sdui-manifest.js';
@@ -50,88 +51,6 @@ import { checkProtocolVersionGap } from '../utils/protocol-version-gap.js';
5051
import { findNavGroupDiagnostics } from '../utils/nav-contribution-groups.js';
5152
import type { NavContributionGroupDiagnostic } from '@objectstack/objectql';
5253

53-
/**
54-
* The artifact's package entries, as `{ index, id, body }` (ADR-0130 D4).
55-
*
56-
* Reads the PARSED stack, so what is walked here is exactly what the artifact
57-
* will carry — `ArtifactPackageSchema` has already judged every entry by the
58-
* time this runs, which is why nothing here re-checks the shape.
59-
*/
60-
function artifactPackages(parsed: Record<string, unknown>): Array<{
61-
index: number;
62-
id: string;
63-
body: Record<string, unknown>;
64-
}> {
65-
const declared = parsed.packages;
66-
if (!Array.isArray(declared)) return [];
67-
return declared.map((entry, index) => {
68-
const body = (entry as { manifest?: Record<string, unknown> }).manifest ?? {};
69-
const id = typeof body.id === 'string' && body.id !== ''
70-
? body.id
71-
: (typeof body.name === 'string' ? body.name : `packages[${index}]`);
72-
return { index, id, body };
73-
});
74-
}
75-
76-
/**
77-
* One assembled package body, re-read as the STACK it was assembled from.
78-
*
79-
* The author-time rules read a stack: collections at the top level and the
80-
* package identity under `manifest`. An assembled body is that same content
81-
* with the manifest fields flattened over the top (`{ ...manifest, ...stack }`
82-
* — `AppPlugin`'s shape, declared by `AssembledPackageBodySchema`), so undoing
83-
* the flatten is one key: the body IS its own manifest.
84-
*
85-
* ⛔ No key list is transcribed here on purpose. Splitting the body back into
86-
* "manifest fields" and "collections" would need a second copy of the key set
87-
* `AssembledPackageBodySchema` derives, and the rules do not need the split —
88-
* they read collections off the top level (already there) and identity off
89-
* `manifest`.
90-
*
91-
* That `manifest` is a SUPERSET of a real manifest — it is the whole body — and
92-
* that is safe here for one reason only: nothing parses it. `runAuthoringRules`
93-
* reads fields off this object and never hands it to a schema. ⛔ Do not start
94-
* parsing it against `ManifestSchema`, and do not reach for a widened schema to
95-
* make that possible: `ManifestSchema` is `strictObject` since #14192, so it
96-
* would REFUSE, by name, every collection key this superset deliberately puts
97-
* under `manifest` — and re-opening it to stop the refusal would re-open the
98-
* real manifest surface with it.
99-
*
100-
* ## `packages` — the second key, and the only one (#16611)
101-
*
102-
* The body carries the collections this package OWNS. Judged with nothing else,
103-
* a rule that resolves an object NAME concludes that a name a SIBLING package in
104-
* the same artifact ships exists nowhere: `examples/app-multi-package`'s
105-
* `crm_order.account` → `crm_account` — a lookup that fixture's README
106-
* documents as the point of the fixture — is owned by its `core` package and
107-
* read from its `orders` one. ADR-0130 makes the release artifact the
108-
* co-ownership boundary, so a per-package pass that cannot see a sibling
109-
* package's objects is THIS RUN's defect and not the author's; the ruled fix
110-
* (director seat, decision batch #86) hands the per-package stack the
111-
* artifact's own `packages[]` as RESOLUTION CONTEXT.
112-
*
113-
* Two properties make that safe, and they are the whole design:
114-
*
115-
* 1. ⛔ It changes what a rule can RESOLVE, never what it JUDGES. The
116-
* collections read off the top level are still this package's alone, so
117-
* every per-package finding this leg exists to produce is still produced.
118-
* ⛔ This is NOT "skip the site per package" — that would silence the gate
119-
* on the one command that ships.
120-
* 2. A name no entry of `packages[]` provides resolves in neither run, so a
121-
* genuinely dangling reference still errors here exactly as it does in the
122-
* union run above.
123-
*
124-
* The array is passed through verbatim rather than reduced to a name list: the
125-
* consuming rule owns which of an entry's contents are resolution context, and a
126-
* set computed here would be a second copy of that decision, free to drift.
127-
*/
128-
function packageBodyAsStack(
129-
body: Record<string, unknown>,
130-
artifactPackageEntries: unknown,
131-
): Record<string, unknown> {
132-
return { ...body, manifest: body, packages: artifactPackageEntries };
133-
}
134-
13554
/** Identity of one finding, for the per-package de-duplication below. */
13655
const findingKey = (f: { rule: string; where: string; path: string; message: string }): string =>
13756
`${f.rule}\u0000${f.where}\u0000${f.path}\u0000${f.message}`;

0 commit comments

Comments
 (0)