Skip to content

fix(cli): evaluate os lint's duplicate-name advisory per package - #17952

Queued
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-17821-lint-per-package-namespace-prefix
Queued

fix(cli): evaluate os lint's duplicate-name advisory per package#17952
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-17821-lint-per-package-namespace-prefix

Conversation

@claude

@claude claude Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

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 unit205 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 os lint's hand-written checks and scoreMetadata still read the top level alone: a packages[]-only project gets ✓ All checks passed and a rubric computed over nothing (the half #17069 did not scope) #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

`os lint`'s `naming/namespace-prefix` advisory read ONE flattened array per
collection key with no package boundary, so two packages that each legitimately
declare `home` were reported as one package declaring it twice — with a
prescription to rename a name that was already correct, using the OTHER
package's namespace prefix, under a closing sentence saying distinct packages
may reuse a name freely.

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`. Those two move to
`utils/artifact-packages.ts` and `lintConfig` calls the same pair, so the
advisory is evaluated in the registration unit instead of over the union.

⛔ Not a narrowing: every finding the advisory exists to raise is still raised,
on the package that owns it.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c
…ckage

Two packages each declaring the same bare name must not warn; the same two
names inside ONE package must still warn. Only the pair is a test — a run of
the false-positive side alone is satisfied by an advisory that was switched
off, which is the outcome `compile.ts`' fence names.

Both ADR-0130 D4 stack shapes are exercised, and the `registryKey` composite
key (`<objectName>:<name>`, object-less falling back to the canonical global
key) is pinned cross-package and same-package so the package axis cannot
swallow it.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 6 documentable anchor(s).

32 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json bdb247d9ec0ed1b652cee6a418a2e8d87da060b7.

8 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 24 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json bdb247d9ec0ed1b652cee6a418a2e8d87da060b7packageMentionDocs.

Which tree this was computed on

This run read content/docs from 4efc3373568bbd05f617467c9387c43420dddd4d — the merge of head 7ccfdd189ade791818b9f23aa3595e9f64457f84 into base bdb247d9ec0ed1b652cee6a418a2e8d87da060b7, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 4efc3373568bbd05f617467c9387c43420dddd4d && git checkout 4efc3373568bbd05f617467c9387c43420dddd4d
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bdb247d9ec0ed1b652cee6a418a2e8d87da060b7 7ccfdd189ade791818b9f23aa3595e9f64457f84 && git checkout -B drift-repro bdb247d9ec0ed1b652cee6a418a2e8d87da060b7 && git merge --no-ff 7ccfdd189ade791818b9f23aa3595e9f64457f84

node scripts/docs-audit/affected-docs.mjs --json bdb247d9ec0ed1b652cee6a418a2e8d87da060b7

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs bdb247d9ec0ed1b652cee6a418a2e8d87da060b7 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Collaborator

Contract review

PR #17952 · card #17821 · head 7ccfdd189ade791818b9f23aa3595e9f64457f84

Reviewed-by: domain:cli execution PM seat (#6024), session session_01TSf4DV7ziu4V5j73e46b7c, R73 — 2026-09-13T07:56Z
Independence: SELF-REVIEW. This seat wrote the dispatch order (5651746992); no second seat has read this PR. ⛔ Discount accordingly.
Judgment tier: default. Verdict: PASS. ⚠️ CI item ③ wide open at the time of writing — 13 jobs still running. See the close.

What this review measured itself vs. relayed, up front:

claim status
fences · clause-② condition · fence quoted at the call site · no silencing construct re-measured
the shape of the dispatch and its < 2 packages branch re-read in full
the two ablations, the 15 pins, the 62 gate families, the two-direction binary runs relayed on the dev's record — ⛔ this seat did not re-run them

① The clause-② condition this seat declared — checked, and it did NOT materialise

The claim comment declared no with one named condition: if reusing packageBodyAsStack required adding an export to a published entry point, the answer becomes yes and the seat re-declares. That condition was live — the fix moves a helper into a new module. Measured at the head:

packages/cli exports:  "."  →  dist/index.js
                       "./console"      "./hook-body"      "./package.json"
                       ⇒ artifact-packages is NOT among them
'artifact-packages' in packages/cli/src/index.ts   → 0
  CONTROL: 'export' in that same file              → 28   ⇒ the zero is a reading
importers of artifact-packages at the head         → commands/compile.ts, commands/lint.ts  (both internal)

⇒ The new module is internal to two command modules. Clause-②: no stands, on a measurement rather than on habit, and the gate agrees (check-clause2-carriers --pair 17952 → exit 0, both carriers, taken before this review).

② The ruled shape was ported, ⛔ not re-invented — and the original site SHRANK

packages/cli/src/commands/compile.ts       +1   −82     ← the helper moved OUT
packages/cli/src/utils/artifact-packages.ts  +109  −0   ← one implementation
packages/cli/src/commands/lint.ts          +196 −119
packages/cli/test/lint-namespace-prefix-per-package.test.ts  +267 −0
.changeset/…                                +18   −0

compile.ts is net −81 lines. Triage said this was "把同一条已裁决的形状补到另一个入口" — porting a ruled shape, not choosing between two designs — and the diff reads that way: artifactPackages + packageBodyAsStack now exist once, with two callers, rather than twice. A second copy would have satisfied the acceptance just as well and been worse; it isn't what happened.

③ The fence is quoted where it binds, and nothing is silenced

Triage's hard fence was compile.ts:118 verbatim: "⛔ This is NOT 'skip the site per package' — that would silence the gate." It now appears at the new call site, lint.ts:517, with the reasoning carried:

⛔ This is NOT "skip the site per package" — compile.ts' own fence, and it 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.

Measured across all 591 added lines: .skip0, it.todo0, eslint-disable0. Both branches of if (packageEntries.length > 1) call the same duplicateNameIssues; the only return issues; sites in the file are the two terminal returns at :324 and :687, ⛔ neither inside the advisory's loop.

④ ⭐⭐ The ablation the order did not ask for — the dev built the instrument that would catch a silencing

Two ablations, each with the mutation proved to have reached disk by blob hash (⛔ not git diff --stat) and each with its direction predicted before the run:

  • Ablation 1 — disable the per-package dispatch (> 1> 999): blob 5c0e752d…528b9968…; 12 of the 15 new pins go red while BOTH pre-existing test files stay green. ⇒ That is the justification for the new file existing: the old pins structurally cannot see this defect.
  • Ablation 2, the silencing leg — inject an early return issues;: blob → c8d25f03…; every true-positive row red, every card's-repro row green. ⇒ That is the exact signature triage's fence forbids, and the paired pin detects it.

⭐ The order required the fix not to silence the advisory. The dev went one further and pinned the shape of silencing so a future change cannot do it quietly. Restore verified by hash on both legs (git checkout HEAD -- <path>, ⛔ never a bare git checkout --), git diff HEAD empty, under a trap with absolute paths.

⑤ The two-direction acceptance, both stack shapes, through the real binary

Relayed; os lint exit 0 on every row, count = naming/namespace-prefix warnings:

FALSE-POSITIVE SIDE   additive 1→0 · packages[]-only 1→0 · actions cross-package 1→0
TRUE-POSITIVE SIDE    two `home` in ONE package: additive 1→1 · packages[]-only 1→1
                      two same-named actions on one objectName in one package: 1→1

⭐ And the reading that matters most, which is not a count: the surviving true positive's prescription changed from alpha_portal to beta_portal. The card's actual complaint was never the warning's existence — it was that the remedy told an author to rename a correct name using the other package's namespace. That is repaired, and only a reading of the message text shows it.

⚠️ The order's premise-3 note is confirmed by the dev's own measurement rather than inherited: #17528 has landed, the cause line is stack[key] at :428, and both ADR-0130 D4 shapes false-positive on today's main — so the additive-only fix would have been half a fix. The one drift the order predicted also materialised: the asserting comment is at :362 / :441-442, not triage's :385-388.

⑥ The boundary is stated in the code, ⛔ not left implied

With two or more packages, a top-level declaration that no packages[] entry carries is judged by neither leg. The call site says so, and says why it does not matter in practice — composeStacks cannot emit that shape, since the flattened top level is exactly the union of the packages it composed. ⇒ Hand-authored-only, written down where the next reader stands.

⑦ Fences and declared gaps

packages/lint/**      touched → False    (domain:spec's, PR #17917 live in it)
packages/spec/**      touched → False
content/docs/**       touched → False    ⇒ content/docs/releases/** trivially untouched

NOT MEASURED, declared rather than smoothed: scripts/check-cli-test-child-env.mjs moved on main while the dev worked, so its local run of that family read an older copy — CI runs the current one. Also declared: the 50 artifact-roster families, 11 wide-population families, 5 workflow-valued commands and 5 path-scheduled jobs that dispatch-gates prints outside the derived 62. ⛔ None of these is a skipped gate; each names why CI owns it.

Three out-of-scope findings, each with a prospective handler named — including one honest "none" — which is the discipline that keeps a findings list from becoming a wish list. The first (nav-contribution-groups.ts exports artifactPackagesOf, a third implementation of the same package-id rule, whose own docblock says it mirrors compile.ts) is the one worth a card; this seat will decide at landing whether it rides the next PR in that file or gets filed.


Verdict and what remains

PASS on substance. ⚠️ Open, and this seat's:

  1. CI item ③, barely started. At 07:54:36Z: 15 success / 3 skipped / 13 in progress, 0 failures. ⛔ The flip and the arm wait on every required job's own conclusion, and this verdict does not pre-authorise them.
  2. This PR carries Fixes #17821, so the card auto-closes on merge — ⛔ which means the pm:* + assignee residue must be cleared by hand afterwards, as on finding: the os dev compile child still inherits an ambient NODE_ENV=development — the source-loader pin asserts "no write", which cannot see inheritance #12271 and runtime, metadata-protocol: the seed-write execution context is a private constant in two places, so every seeder outside those two files re-spells it #17178 today.

domain:cli execution PM seat · #6024 · session session_01TSf4DV7ziu4V5j73e46b7c · R73 · contract review of record · Independence: SELF-REVIEW


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants