diff --git a/packages/testing/code-health/README.md b/packages/testing/code-health/README.md index 222de07adaa..3d7127f5d89 100644 --- a/packages/testing/code-health/README.md +++ b/packages/testing/code-health/README.md @@ -43,7 +43,8 @@ git commit -m "chore: update code-health baseline" ## Output -All output is JSON. Exit code 1 if new violations are found, 0 if clean. +Rule output is JSON (the single-instance subcommands below print plain text). Exit code 1 if new +violations are found, 0 if clean. ```json { @@ -55,6 +56,41 @@ All output is JSON. Exit code 1 if new violations are found, 0 if clean. } ``` +## Single-instance dependency checks + +Separate subcommands (not rules) verify that the curated single-instance libraries in +`src/single-instance/libs.ts` resolve to exactly one physical copy — a second copy breaks +`instanceof`, module singletons and cross-package schema composition at runtime. + +```bash +# Verify an already-installed closure (e.g. a pruned production tree) +pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-closure + +# Reproduce the `npm install` graph of published tarballs and verify that +pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install ... + +# Scopes CI uses: packages changed since a ref, or every publishable package. +# `--report-only` downgrades a finding to a warning and exits 0 (what CI passes today, while the +# curated-lib backlog is worked down). +pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install --changed=origin/master +pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install --all --report-only +``` + +`verify-npm-install` packs each target with `pnpm pack` and installs it with npm, because root +`pnpm.overrides` — which hide duplication locally and in `pnpm deploy` — do not travel in a +published tarball. + +On a finding it prints every physical copy with the package that pulled it in, then the fix options +that apply to those requirers: move the library to `peerDependencies` (`catalog:`) in one of our +packages, align a version in a package the peer rule exempts, bump/replace the third-party package +pinning an incompatible range, or, as a last resort, add a documented `EXPECTED_DUPLICATES` entry in +`src/single-instance/collect-copies.ts`. In CI the same findings also go to the job summary, so a +finding is visible without opening the log. + +A blocking run keeps its scratch install so you can walk the full requirer chain with `npm ls`; a +`--report-only` run deletes it (it is a complete `node_modules`), so re-run the same targets locally +when you need that chain. + ## Adding rules Rules extend `BaseRule` from `@n8n/rules-engine`. See `src/rules/catalog-violations.rule.ts` for the pattern. Register new rules in `src/index.ts`. diff --git a/packages/testing/code-health/src/rules/single-instance-libs.rule.ts b/packages/testing/code-health/src/rules/single-instance-libs.rule.ts index 4edcd7eabc7..6152cc2a423 100644 --- a/packages/testing/code-health/src/rules/single-instance-libs.rule.ts +++ b/packages/testing/code-health/src/rules/single-instance-libs.rule.ts @@ -2,12 +2,7 @@ import { BaseRule } from '@n8n/rules-engine'; import type { Violation } from '@n8n/rules-engine'; import type { CodeHealthContext } from '../context.js'; -import { - CURATED_LIBS, - FRONTEND_PATH_PREFIXES, - HOST_PACKAGES, - PEER_LIBS, -} from '../single-instance/libs.js'; +import { CURATED_LIBS, isPeerRuleExempt, PEER_LIBS } from '../single-instance/libs.js'; import { REQUIRED_CURATED_PEERS } from '../single-instance/required-peers.js'; import { findPackageJsonFiles, @@ -52,7 +47,7 @@ export class SingleInstanceLibsRule extends BaseRule { ), }); - if (this.isExempt(pkg.packageName, relativeDir(rootDir, file))) continue; + if (isPeerRuleExempt(pkg.packageName, relativeDir(rootDir, file))) continue; for (const dep of pkg.deps) { if (PEER_LIBS.includes(dep.name) && RUNTIME_SECTIONS.has(dep.section)) { @@ -106,11 +101,4 @@ export class SingleInstanceLibsRule extends BaseRule { return violations; } - - private isExempt(packageName: string, relDir: string): boolean { - return ( - HOST_PACKAGES.includes(packageName) || - FRONTEND_PATH_PREFIXES.some((prefix) => relDir.startsWith(prefix)) - ); - } } diff --git a/packages/testing/code-health/src/single-instance/collect-copies.ts b/packages/testing/code-health/src/single-instance/collect-copies.ts index 7751f8f5891..fe988f64359 100644 --- a/packages/testing/code-health/src/single-instance/collect-copies.ts +++ b/packages/testing/code-health/src/single-instance/collect-copies.ts @@ -12,6 +12,8 @@ export interface DuplicateGroup { name: string; isCurated: boolean; allowed: boolean; + /** Why the duplicate is tolerated, when it is allowlisted. */ + reason?: string; copies: Copy[]; } @@ -155,6 +157,7 @@ export function analyze( name, isCurated: CURATED_LIBS.includes(name), allowed: Object.hasOwn(allowlist, name), + reason: allowlist[name], copies: distinct, }); } diff --git a/packages/testing/code-health/src/single-instance/explain-duplicates.test.ts b/packages/testing/code-health/src/single-instance/explain-duplicates.test.ts new file mode 100644 index 00000000000..3f38c92488e --- /dev/null +++ b/packages/testing/code-health/src/single-instance/explain-duplicates.test.ts @@ -0,0 +1,341 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { analyze, collectCopies } from './collect-copies.js'; +import { + attributeCopy, + describeOrigin, + explainDuplicates, + formatCopyLines, + formatCuratedReport, + formatRemediation, + formatStepSummary, +} from './explain-duplicates.js'; + +// Planted on disk: nesting is how npm records "this package forced this copy", so attribution is +// exercised against a real tree rather than a hand-built path string. +let ROOT: string; + +const WORKSPACE = new Set(['@n8n/api-types', 'n8n']); +const NO_EXEMPTIONS = new Set(); + +function pkg(relDir: string, manifest: Record): void { + const dir = join(ROOT, relDir); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'package.json'), JSON.stringify(manifest)); +} + +beforeAll(() => { + ROOT = mkdtempSync(join(tmpdir(), 'explain-duplicates-')); + // hoisted copy npm picked for the root project + pkg('node_modules/zod', { name: 'zod', version: '4.4.3' }); + // a workspace package of ours pinning its own copy -> fix is a manifest change in this repo + pkg('node_modules/@n8n/api-types', { + name: '@n8n/api-types', + version: '1.0.0', + dependencies: { zod: '^3.25.0' }, + }); + pkg('node_modules/@n8n/api-types/node_modules/zod', { name: 'zod', version: '3.25.76' }); + // a third-party package pinning an incompatible range -> fix is upstream / the catalog + pkg('node_modules/third-party', { + name: 'third-party', + version: '2.0.0', + peerDependencies: { zod: '~3.20.0' }, + }); + pkg('node_modules/third-party/node_modules/zod', { name: 'zod', version: '3.20.1' }); + // nested under a package that does not declare the lib itself + pkg('node_modules/silent', { name: 'silent', version: '1.0.0' }); + pkg('node_modules/silent/node_modules/zod', { name: 'zod', version: '3.1.0' }); +}); + +afterAll(() => rmSync(ROOT, { recursive: true, force: true })); + +const attribute = (relPath: string, version: string, lib = 'zod') => + attributeCopy(ROOT, lib, { realPath: join(ROOT, relPath), version }, WORKSPACE); + +const explainAll = () => explainDuplicates(ROOT, analyze(collectCopies(ROOT)).failures, WORKSPACE); + +describe('attributeCopy', () => { + it('marks a top-level copy as hoisted', () => { + const copy = attribute('node_modules/zod', '4.4.3'); + expect(copy.requiredBy).toBeNull(); + expect(describeOrigin(copy)).toBe('hoisted at the top level'); + }); + + it('attributes a nested copy to the package that declares the range', () => { + const copy = attribute('node_modules/third-party/node_modules/zod', '3.20.1'); + expect(copy).toMatchObject({ requiredBy: 'third-party', range: '~3.20.0', isWorkspace: false }); + expect(describeOrigin(copy)).toBe('required by third-party (peerDependencies "~3.20.0")'); + }); + + // Which section declared it decides the fix, so attribution has to keep it. + it('records the section the range came from', () => { + expect(attribute('node_modules/@n8n/api-types/node_modules/zod', '3.25.76').section).toBe( + 'dependencies', + ); + expect(attribute('node_modules/third-party/node_modules/zod', '3.20.1').section).toBe( + 'peerDependencies', + ); + expect(attribute('node_modules/silent/node_modules/zod', '3.1.0').section).toBeNull(); + }); + + it('flags a requirer that is one of our own packages', () => { + const copy = attribute('node_modules/@n8n/api-types/node_modules/zod', '3.25.76'); + expect(copy).toMatchObject({ requiredBy: '@n8n/api-types', isWorkspace: true }); + expect(describeOrigin(copy)).toContain('[workspace package]'); + }); + + it('falls back to the enclosing package when no ancestor declares the lib', () => { + const copy = attribute('node_modules/silent/node_modules/zod', '3.1.0'); + expect(copy).toMatchObject({ requiredBy: 'silent', range: null }); + expect(describeOrigin(copy)).toBe('nested under silent (no direct declaration)'); + }); + + // A store key is not a requirer, so naming it as one would be confidently wrong. + it('reports a copy inside a pnpm virtual store as such, not as hoisted', () => { + // Its own root: a store entry in the shared tree would count as another copy of zod. + const storeRoot = join(ROOT, 'store'); + const relPath = 'node_modules/.pnpm/zod@3.25.76/node_modules/zod'; + pkg(join('store', relPath), { name: 'zod', version: '3.25.76' }); + + const copy = attributeCopy( + storeRoot, + 'zod', + { realPath: join(storeRoot, relPath), version: '3.25.76' }, + WORKSPACE, + ); + + expect(copy.requiredBy).toBeNull(); + expect(describeOrigin(copy)).toBe( + 'in the pnpm virtual store (requirer not derivable from the path)', + ); + }); + + // devDependencies are not installed for a consumer, so they cannot nest a copy — a dev-only + // declaration must not be reported as the requirer. + it('ignores a declaration that cannot nest a copy', () => { + const devRoot = join(ROOT, 'dev-only'); + pkg('dev-only/node_modules/zod', { name: 'zod', version: '4.4.3' }); + pkg('dev-only/node_modules/tool', { + name: 'tool', + version: '1.0.0', + devDependencies: { zod: '^3.0.0' }, + }); + pkg('dev-only/node_modules/tool/node_modules/zod', { name: 'zod', version: '3.25.76' }); + + const copy = attributeCopy( + devRoot, + 'zod', + { realPath: join(devRoot, 'node_modules/tool/node_modules/zod'), version: '3.25.76' }, + WORKSPACE, + ); + + expect(copy).toMatchObject({ requiredBy: 'tool', range: null }); + }); +}); + +describe('explainDuplicates', () => { + it('attributes every copy of a real collected duplicate', () => { + const [zod] = explainAll(); + + expect(zod.name).toBe('zod'); + expect(zod.copies.map((c) => c.requiredBy)).toEqual([ + null, + '@n8n/api-types', + 'silent', + 'third-party', + ]); + }); + + it('prints the version, the origin and the path of each copy', () => { + const output = formatCopyLines(explainAll()[0].copies).join('\n'); + + expect(output).toContain('v3.20.1'); + expect(output).toContain('required by third-party (peerDependencies "~3.20.0")'); + expect(output).toContain('node_modules/third-party/node_modules/zod'); + }); +}); + +describe('formatCuratedReport', () => { + it('reports every curated library, and the caller renders the failing copies', () => { + const found = collectCopies(ROOT); + const { duplicates } = analyze(found); + const output = formatCuratedReport(found, duplicates, () => [' ']).join('\n'); + + expect(output).toContain('zod: FAIL — 4 copies, expected 1:'); + expect(output).toContain(''); + // A curated lib absent from the closure is still listed, so a run always states the verdict + // for the full enforced set rather than only for what happened to be installed. + expect(output).toContain('reflect-metadata: not present'); + }); + + // Which copies an entry covers is what tells you whether it is still accurate, so an allowlisted + // duplicate lists them too rather than only its reason. + it('prints the copies of an allowlisted duplicate and why it is tolerated', () => { + const found = collectCopies(ROOT); + const { duplicates } = analyze(found, { allowlist: { zod: 'tolerated for ' } }); + const output = formatCuratedReport(found, duplicates, () => [' ']).join('\n'); + + expect(output).toContain('zod: ALLOWED DUP'); + expect(output).toContain(''); + expect(output).toContain('allowlisted: tolerated for '); + }); +}); + +describe('formatRemediation', () => { + it('tells our own packages to move the library to peerDependencies', () => { + const output = formatRemediation(explainAll(), { exemptPackages: NO_EXEMPTIONS }).join('\n'); + expect(output).toContain('- zod <- @n8n/api-types (dependencies "^3.25.0")'); + expect(output).toContain('"peerDependencies"'); + }); + + // The `single-instance-libs` rule exempts host packages, so telling their owner to move a + // curated lib to peerDependencies would send them into a change that rule rejects. + it('does not propose a peer move for a package the peer rule exempts', () => { + pkg('host/node_modules/zod', { name: 'zod', version: '4.4.3' }); + pkg('host/node_modules/n8n', { + name: 'n8n', + version: '1.0.0', + dependencies: { zod: '^3.0.0' }, + }); + pkg('host/node_modules/n8n/node_modules/zod', { name: 'zod', version: '3.25.76' }); + const hostRoot = join(ROOT, 'host'); + + const explained = explainDuplicates( + hostRoot, + analyze(collectCopies(hostRoot)).failures, + WORKSPACE, + ); + const output = formatRemediation(explained, { exemptPackages: new Set(['n8n']) }).join('\n'); + + expect(output).toContain('legitimately own a copy'); + expect(output).toContain('- zod <- n8n (dependencies "^3.0.0")'); + expect(output).not.toContain('"peerDependencies"'); + }); + + // The compliant shape: @n8n/api-types, n8n-core and n8n-workflow all declare zod only as a peer, + // so they are the likeliest requirers in the real closure — and there is nothing for them to move. + it('does not propose a peer move for a package that already declares the peer', () => { + const peerRoot = join(ROOT, 'peer'); + pkg('peer/node_modules/zod', { name: 'zod', version: '4.4.3' }); + pkg('peer/node_modules/@n8n/api-types', { + name: '@n8n/api-types', + version: '1.0.0', + peerDependencies: { zod: '3.25.76' }, + }); + pkg('peer/node_modules/@n8n/api-types/node_modules/zod', { name: 'zod', version: '3.25.76' }); + + const explained = explainDuplicates( + peerRoot, + analyze(collectCopies(peerRoot)).failures, + WORKSPACE, + ); + const output = formatRemediation(explained, { exemptPackages: NO_EXEMPTIONS }).join('\n'); + + expect(output).toContain('already declare the library as a peer'); + expect(output).toContain('- zod <- @n8n/api-types (peerDependencies "3.25.76")'); + expect(output).not.toContain('Move it to "peerDependencies"'); + }); + + // reflect-metadata is curated but pin-only, so the peer rule does not cover it either. + it('does not propose a peer move for a curated library outside the peer rule', () => { + const copy = attribute( + 'node_modules/@n8n/api-types/node_modules/zod', + '3.25.76', + 'reflect-metadata', + ); + const explained = [ + { + name: 'reflect-metadata', + copies: [{ ...copy, requiredBy: '@n8n/api-types', range: '^0.2.0', isWorkspace: true }], + }, + ]; + + const output = formatRemediation(explained, { exemptPackages: NO_EXEMPTIONS }).join('\n'); + expect(output).toContain('legitimately own a copy'); + expect(output).not.toContain('"peerDependencies"'); + }); + + it('names the third-party requirer and its range', () => { + expect(formatRemediation(explainAll(), { exemptPackages: NO_EXEMPTIONS }).join('\n')).toContain( + '- zod <- third-party (peerDependencies "~3.20.0")', + ); + }); + + it('numbers the steps consecutively whichever ones apply', () => { + const output = formatRemediation(explainAll(), { + exemptPackages: new Set(['@n8n/api-types']), + }).join('\n'); + expect(output).toContain(' 1. Our own packages that legitimately own a copy'); + expect(output).toContain(' 2. Third-party packages'); + expect(output).toContain(' 3. Copies nested under a package that does not declare'); + expect(output).toContain(' 4. If the split cannot be removed yet'); + }); + + it('points at the allowlist as the last resort', () => { + expect(formatRemediation(explainAll(), { exemptPackages: NO_EXEMPTIONS }).join('\n')).toContain( + 'EXPECTED_DUPLICATES', + ); + }); + + it('mentions the kept install tree only when one is kept', () => { + const options = { exemptPackages: NO_EXEMPTIONS }; + expect( + formatRemediation(explainAll(), { ...options, scratch: '/tmp/kept' }).join('\n'), + ).toContain('npm ls --all --prefix /tmp/kept zod'); + expect(formatRemediation(explainAll(), options).join('\n')).not.toContain('--prefix'); + }); +}); + +describe('formatStepSummary', () => { + it('renders a table row per copy and marks an advisory run as such', () => { + const summary = formatStepSummary(explainAll(), { + reportOnly: true, + exemptPackages: NO_EXEMPTIONS, + }); + + expect(summary).toContain('(advisory)'); + expect(summary).toContain('| Library | Version | Pulled in by | Path |'); + expect(summary.match(/^\| zod \|/gm)).toHaveLength(4); + }); + + it('does not mark a blocking run as advisory, and carries the kept tree', () => { + const summary = formatStepSummary(explainAll(), { + reportOnly: false, + exemptPackages: NO_EXEMPTIONS, + scratch: '/tmp/kept', + }); + + expect(summary).not.toContain('advisory'); + expect(summary).toContain('npm ls --all --prefix /tmp/kept'); + }); + + // A `|` ends a table cell even inside a code span, and `||` ranges are common on curated libs. + it('escapes dependency-controlled text so a range cannot break the table', () => { + const summary = formatStepSummary( + [ + { + name: 'zod', + copies: [ + { + version: '3.20.1', + path: 'node_modules/pkg/node_modules/zod', + requiredBy: 'pkg', + range: '^3.22.0 || ^4.0.0', + section: 'dependencies' as const, + isWorkspace: false, + inPnpmStore: false, + }, + ], + }, + ], + { reportOnly: true, exemptPackages: NO_EXEMPTIONS }, + ); + const row = summary.split('\n').find((line) => line.startsWith('| zod |')); + + expect(row).toContain('\\|\\|'); + expect(row?.split(/(? `['a', 'zod']`. + */ +function packageChain(relPath: string): string[] { + return toPosix(relPath) + .replace(/^node_modules\//, '') + .split('/node_modules/'); +} + +type Manifest = Record | undefined>; + +function isManifest(value: unknown): value is Manifest { + return typeof value === 'object' && value !== null; +} + +interface Declaration { + section: Section; + range: string; +} + +function readDeclaration(dir: string, lib: string): Declaration | null { + let parsed: unknown; + try { + parsed = JSON.parse(readFileSync(join(dir, 'package.json'), 'utf8')); + } catch { + return null; + } + if (!isManifest(parsed)) return null; + for (const section of PUBLISHED_SECTIONS) { + const range = parsed[section]?.[lib]; + if (typeof range === 'string' && range) return { section, range }; + } + return null; +} + +/** + * Explain where a physical copy came from. npm nests a copy inside the package that forced it, so + * the nearest enclosing package that declares the lib is the one to fix — that attribution is the + * difference between "zod has 5 copies" and "this dependency pins an incompatible range". + * + * Nesting only carries that meaning in an `npm install` tree. A pnpm virtual store nests every + * package under a `@` store key instead, so the path names no requirer and this + * reports the copy as unattributed rather than presenting a store directory as one. + */ +export function attributeCopy( + root: string, + lib: string, + copy: Copy, + workspaceNames: Set, +): AttributedCopy { + // `collectCopies` records realpaths, so the root has to be resolved too — otherwise a symlinked + // tmpdir (every macOS `/var` path) makes the install path relative to nothing useful. + const resolvedRoot = resolveRealPath(root); + const path = toPosix(relative(resolvedRoot, resolveRealPath(copy.realPath))); + const chain = packageChain(path); + const inPnpmStore = path.startsWith('node_modules/.pnpm/'); + const ancestors = inPnpmStore ? [] : chain.slice(0, -1); + const base = { version: copy.version, path, inPnpmStore }; + + let dir = resolvedRoot; + const ancestorDirs = ancestors.map((name) => (dir = join(dir, 'node_modules', name))); + + // Nearest ancestor first: the innermost declaration is the one that forced this copy. + for (let i = ancestorDirs.length - 1; i >= 0; i--) { + const declared = readDeclaration(ancestorDirs[i], lib); + if (declared) { + const requiredBy = ancestors[i]; + return { + ...base, + requiredBy, + ...declared, + isWorkspace: workspaceNames.has(requiredBy), + }; + } + } + + // Nested with no declaring ancestor (npm nesting for a deeper conflict), the hoisted copy, or a + // tree shape whose nesting names no requirer. + const nearest = ancestors.at(-1) ?? null; + return { + ...base, + requiredBy: nearest, + range: null, + section: null, + isWorkspace: nearest !== null && workspaceNames.has(nearest), + }; +} + +export interface ExplainedDuplicate { + name: string; + copies: AttributedCopy[]; +} + +export function explainDuplicates( + root: string, + groups: DuplicateGroup[], + workspaceNames: Set, +): ExplainedDuplicate[] { + return groups.map((group) => ({ + name: group.name, + copies: group.copies + .map((copy) => attributeCopy(root, group.name, copy, workspaceNames)) + // Hoisted copy first — it is the version npm picked, so every nested copy below it reads + // as "and this package refused that one". + .sort( + (a, b) => Number(!isHoisted(a)) - Number(!isHoisted(b)) || a.path.localeCompare(b.path), + ), + })); +} + +function isHoisted(copy: AttributedCopy): boolean { + return copy.requiredBy === null && !copy.inPnpmStore; +} + +/** One-line origin for a copy, e.g. `required by @langchain/community ("~0.1.29")`. */ +export function describeOrigin(copy: AttributedCopy): string { + if (copy.inPnpmStore) return 'in the pnpm virtual store (requirer not derivable from the path)'; + if (copy.requiredBy === null) return 'hoisted at the top level'; + const marker = copy.isWorkspace ? ' [workspace package]' : ''; + return copy.range === null + ? `nested under ${copy.requiredBy} (no direct declaration)${marker}` + : `required by ${copy.requiredBy} (${copy.section} "${copy.range}")${marker}`; +} + +/** One indented block per physical copy: version, what pulled it in, and where it landed. */ +export function formatCopyLines(copies: AttributedCopy[]): string[] { + return copies.flatMap((copy) => [ + ` v${copy.version} ${describeOrigin(copy)}`, + ` ${copy.path}`, + ]); +} + +/** + * Per-curated-library verdict, in `CURATED_LIBS` order: the enforced list every run prints, + * whatever closure it inspected. `renderCopies` is the caller's detail for a failing library — an + * npm-install run can name the requirer of each copy, a pnpm-shaped closure can only list paths. + */ +export function formatCuratedReport( + found: Map, + duplicates: DuplicateGroup[], + renderCopies: (dup: DuplicateGroup) => string[], +): string[] { + const byName = new Map(duplicates.map((d) => [d.name, d])); + return CURATED_LIBS.flatMap((lib) => { + const dup = byName.get(lib); + if (!dup) { + const copies = distinctCopies(found.get(lib) ?? []); + return [ + ` ${lib}: ${copies.length === 0 ? 'not present' : `OK (1 copy, v${copies[0].version})`}`, + ]; + } + if (dup.allowed) { + return [ + ` ${lib}: ALLOWED DUP — ${dup.copies.length} physical copies:`, + ...renderCopies(dup), + ` allowlisted: ${dup.reason}`, + ]; + } + return [` ${lib}: FAIL — ${dup.copies.length} copies, expected 1:`, ...renderCopies(dup)]; + }); +} + +/** The verdict sentence, shared so both commands phrase a finding identically. */ +export function describeFailureCount(count: number): string { + return count === 1 + ? '1 curated library resolves to more than one physical copy' + : `${count} curated libraries each resolve to more than one physical copy`; +} + +const LIBS_FILE = 'packages/testing/code-health/src/single-instance/libs.ts'; +const ALLOWLIST_FILE = 'packages/testing/code-health/src/single-instance/collect-copies.ts'; + +interface Requirer { + lib: string; + requiredBy: string; + range: string | null; + section: Section | null; + isWorkspace: boolean; +} + +function describeRequirer({ lib, requiredBy, range, section }: Requirer): string { + return ` - ${lib} <- ${requiredBy}${range === null ? '' : ` (${section} "${range}")`}`; +} + +/** Which remediation a requirer needs. One bucket per fix, so every copy is accounted for. */ +type Bucket = 'peerMove' | 'peerConflict' | 'workspaceExempt' | 'thirdParty' | 'indirect'; + +function bucketFor(requirer: Requirer, exemptPackages: Set): Bucket { + // No declared range means the requirer of record does not ask for the lib itself — something + // deeper in its own graph does, and none of the manifest fixes below apply to it. + if (requirer.range === null) return 'indirect'; + if (!requirer.isWorkspace) return 'thirdParty'; + // Already a peer: the shape the peer rule asks for. Nothing to move — the copy exists because + // the graph resolved a version outside this range, which is a pin problem elsewhere. + if (requirer.section === 'peerDependencies') return 'peerConflict'; + // A lib the peer rule does not cover (pin-only, e.g. reflect-metadata) or a package it exempts + // is legitimately allowed its own dependency; only a version split is left to fix. + return PEER_LIBS.includes(requirer.lib) && !exemptPackages.has(requirer.requiredBy) + ? 'peerMove' + : 'workspaceExempt'; +} + +/** + * Remediation steps, ordered by what the findings actually show. `exemptPackages` are the workspace + * packages `isPeerRuleExempt` covers — telling their owner to move a curated lib to + * `peerDependencies` would send them into a change that rule then rejects. + */ +export function formatRemediation( + explained: ExplainedDuplicate[], + { exemptPackages, scratch }: { exemptPackages: Set; scratch?: string }, +): string[] { + // Keyed, not per copy: npm installs the same requirer at several nesting depths, and the fix is + // one manifest edit however many copies it produced. + const requirers = new Map(); + for (const dup of explained) { + for (const { requiredBy, range, section, isWorkspace } of dup.copies) { + if (requiredBy === null) continue; + requirers.set(`${dup.name}|${requiredBy}|${section}|${range}`, { + lib: dup.name, + requiredBy, + range, + section, + isWorkspace, + }); + } + } + const inBucket = (bucket: Bucket): Requirer[] => + [...requirers.values()].filter((r) => bucketFor(r, exemptPackages) === bucket); + const peerMove = inBucket('peerMove'); + const peerConflict = inBucket('peerConflict'); + const workspaceExempt = inBucket('workspaceExempt'); + const thirdParty = inBucket('thirdParty'); + const indirect = inBucket('indirect'); + + const lines = [ + ' A curated library must resolve to ONE physical copy per process — a second copy breaks', + ` instanceof, module singletons and cross-package schema composition (${LIBS_FILE}).`, + '', + ]; + let step = 0; + if (peerMove.length > 0) { + lines.push( + ` ${++step}. Our own packages declaring the library as a runtime dependency:`, + ...peerMove.map(describeRequirer), + ' Move it to "peerDependencies" with "catalog:" in that package.json, and keep it in', + ' devDependencies (also "catalog:") so local builds still resolve it.', + '', + ); + } + if (peerConflict.length > 0) { + lines.push( + ` ${++step}. Our own packages that already declare the library as a peer:`, + ...peerConflict.map(describeRequirer), + ' Nothing to move — this copy exists because something else in the graph resolved a', + ' version outside that range. Align the "catalog:" pin, or fix the requirer that forced', + ' the other version (it is listed under one of the other steps).', + '', + ); + } + if (workspaceExempt.length > 0) { + lines.push( + ` ${++step}. Our own packages that legitimately own a copy (host package, or a library the`, + ' peer rule does not cover):', + ...workspaceExempt.map(describeRequirer), + ' A peer move is not the fix here — align the declared version with "catalog:" so this', + ' copy and the rest of the graph resolve to the same one.', + '', + ); + } + if (thirdParty.length > 0) { + lines.push( + ` ${++step}. Third-party packages pinning an incompatible range:`, + ...thirdParty.map(describeRequirer), + ' Bump or replace that dependency, or move the catalog version in pnpm-workspace.yaml to a', + ' version its range accepts. Root "pnpm.overrides" will NOT help: they do not travel in a', + ' published tarball, which is exactly what this check reproduces.', + '', + ); + } + if (indirect.length > 0) { + lines.push( + ` ${++step}. Copies nested under a package that does not declare the library itself:`, + ...indirect.map(describeRequirer), + " The requirer is deeper in that package's own dependency graph — find it with", + ' "npm ls --all " in the install tree before picking a fix.', + '', + ); + } + lines.push( + ` ${++step}. If the split cannot be removed yet, add an EXPECTED_DUPLICATES entry in`, + ` ${ALLOWLIST_FILE} documenting why it is tolerated and what removes it.`, + '', + ' Reproduce locally (packs + installs the same closure, no CI needed):', + ' pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install ...', + ' (or --changed=origin/master to reproduce the scope this job used)', + ); + if (scratch) { + lines.push( + '', + ` The install tree is kept at ${scratch} — for a full requirer chain, run:`, + ` npm ls --all --prefix ${scratch} ${explained.map((d) => d.name).join(' ')}`, + ); + } + return lines; +} + +/** + * Table cells carry dependency-controlled text (names, semver ranges, paths). In GFM a `|` ends the + * cell even inside a code span — and `||` ranges are common on curated libs — so a raw range would + * shift every following column. Newlines would end the row outright. + */ +function cell(text: string): string { + return text.replace(/[\\|`]/g, '\\$&').replace(/\s*[\r\n]+\s*/g, ' '); +} + +// One backtick more than any fence a dependency's own text could contain, so the fix block cannot +// be closed early and the rest of the summary reinterpreted as markdown. +const FENCE = '````'; + +/** GitHub job-summary markdown — the findings surface without opening a 5k-line log. */ +export function formatStepSummary( + explained: ExplainedDuplicate[], + { + reportOnly, + exemptPackages, + scratch, + }: { reportOnly: boolean; exemptPackages: Set; scratch?: string }, +): string { + const rows = explained.flatMap((dup) => + dup.copies.map( + (copy) => + `| ${[dup.name, copy.version, describeOrigin(copy), copy.path].map(cell).join(' | ')} |`, + ), + ); + return [ + `### Single-instance deps: duplicates found${reportOnly ? ' (advisory)' : ''}`, + '', + `${describeFailureCount(explained.length)} in the \`npm install\` graph.`, + '', + '| Library | Version | Pulled in by | Path |', + '| --- | --- | --- | --- |', + ...rows, + '', + '
How to fix', + '', + FENCE, + ...formatRemediation(explained, { exemptPackages, scratch }), + FENCE, + '', + '
', + '', + ].join('\n'); +} diff --git a/packages/testing/code-health/src/single-instance/libs.ts b/packages/testing/code-health/src/single-instance/libs.ts index 754d531dd02..1ad1fa6788e 100644 --- a/packages/testing/code-health/src/single-instance/libs.ts +++ b/packages/testing/code-health/src/single-instance/libs.ts @@ -7,6 +7,16 @@ * `verify-npm-install` CLI subcommands. */ +/** + * Manifest sections that follow the publish graph: what a consumer installs, and therefore what can + * nest a second physical copy. devDependencies don't ship, so they can't. + */ +export const PUBLISHED_SECTIONS = [ + 'dependencies', + 'peerDependencies', + 'optionalDependencies', +] as const; + /** Libraries a single process must resolve to exactly one physical copy of. */ export const CURATED_LIBS = ['zod', 'form-data', '@langchain/core', 'reflect-metadata']; @@ -31,3 +41,15 @@ export const FRONTEND_PATH_PREFIXES = ['packages/frontend/']; /** Curated libs subject to the peer rule (pin-only libs are exempt). */ export const PEER_LIBS = CURATED_LIBS.filter((lib) => !PIN_ONLY_LIBS.includes(lib)); + +/** + * Whether the peer rule exempts a package: it provides its own runtime instance, or it bundles. + * The `single-instance-libs` rule skips these, and the duplicate report must not tell their owner + * to make a change that rule then rejects — so both read the exemption from here. + */ +export function isPeerRuleExempt(packageName: string, relDir: string): boolean { + return ( + HOST_PACKAGES.includes(packageName) || + FRONTEND_PATH_PREFIXES.some((prefix) => relDir.startsWith(prefix)) + ); +} diff --git a/packages/testing/code-health/src/single-instance/verify-closure.ts b/packages/testing/code-health/src/single-instance/verify-closure.ts index 278c30efea5..aed89b2476d 100644 --- a/packages/testing/code-health/src/single-instance/verify-closure.ts +++ b/packages/testing/code-health/src/single-instance/verify-closure.ts @@ -1,5 +1,5 @@ -import { analyze, collectCopies, distinctCopies, EXPECTED_DUPLICATES } from './collect-copies.js'; -import { CURATED_LIBS } from './libs.js'; +import { analyze, collectCopies } from './collect-copies.js'; +import { describeFailureCount, formatCuratedReport } from './explain-duplicates.js'; /** * Exit code for "the closure was checked and curated duplicates were found". @@ -21,23 +21,13 @@ export function runVerifyClosure(dir: string): number { const { duplicates, failures } = analyze(found); console.log(`\nSingle-instance dependency verifier — root: ${dir}`); - const curatedDups = new Map(duplicates.filter((d) => d.isCurated).map((d) => [d.name, d])); console.log('\nCurated single-instance libraries (enforced):'); - for (const lib of CURATED_LIBS) { - const dup = curatedDups.get(lib); - if (!dup) { - const copies = distinctCopies(found.get(lib) ?? []); - console.log( - ` ${lib}: ${copies.length === 0 ? 'not present' : `OK (1 copy, v${copies[0].version})`}`, - ); - continue; - } - console.log( - ` ${lib}: ${dup.allowed ? 'ALLOWED DUP' : 'FAIL'} — ${dup.copies.length} physical copies:`, - ); - for (const c of dup.copies) console.log(` v${c.version} ${c.realPath}`); - if (dup.allowed) console.log(` allowlisted: ${EXPECTED_DUPLICATES[lib]}`); - } + // Paths, not requirers: this runs against a pruned `pnpm deploy` closure, whose nesting is a + // virtual store rather than "package X forced this copy". + const report = formatCuratedReport(found, duplicates, (dup) => + dup.copies.map((c) => ` v${c.version} ${c.realPath}`), + ); + for (const line of report) console.log(line); const otherDups = duplicates.filter((d) => !d.isCurated); if (otherDups.length > 0) { @@ -51,8 +41,10 @@ export function runVerifyClosure(dir: string): number { console.log(''); if (failures.length > 0) { - console.error( - `FAIL: curated ${failures.length === 1 ? 'library resolves' : 'libraries resolve'} to multiple physical copies: ${failures.map((f) => f.name).join(', ')}`, + // stdout, not stderr: the streams interleave in CI logs, and build-n8n.mjs pipes stdout — a + // verdict on stderr can land in the middle of the list it summarises, or out of view. + console.log( + `FAIL: ${describeFailureCount(failures.length)}: ${failures.map((f) => f.name).join(', ')}`, ); return EXIT_DUPLICATES_FOUND; } diff --git a/packages/testing/code-health/src/single-instance/verify-npm-install.ts b/packages/testing/code-health/src/single-instance/verify-npm-install.ts index 9ae26fb8a96..1f0af36a90f 100644 --- a/packages/testing/code-health/src/single-instance/verify-npm-install.ts +++ b/packages/testing/code-health/src/single-instance/verify-npm-install.ts @@ -1,10 +1,26 @@ import { execFileSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { + appendFileSync, + mkdirSync, + mkdtempSync, + readdirSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { setTimeout } from 'node:timers/promises'; import { analyze, collectCopies } from './collect-copies.js'; +import { + describeFailureCount, + explainDuplicates, + formatCopyLines, + formatCuratedReport, + formatRemediation, + formatStepSummary, +} from './explain-duplicates.js'; +import { isPeerRuleExempt, PUBLISHED_SECTIONS } from './libs.js'; import type { PackageJsonInfo } from '../utils/package-json-scanner.js'; import { findPackageJsonFiles, @@ -16,8 +32,7 @@ import { // root manifest, or the single-instance tooling itself) — a scoped diff would otherwise miss them. const ROOT_TRIGGERS = ['pnpm-workspace.yaml', 'package.json', 'packages/testing/code-health/']; -// Sections that follow the publish graph — devDependencies don't ship, so they're not packed. -const CLOSURE_SECTIONS = new Set(['dependencies', 'peerDependencies', 'optionalDependencies']); +const CLOSURE_SECTIONS = new Set(PUBLISHED_SECTIONS); /** True when a changed file can shift dependency resolution repo-wide, forcing a full check. */ export function filesTriggerFullRun(files: string[]): boolean { @@ -25,11 +40,41 @@ export function filesTriggerFullRun(files: string[]): boolean { } /** - * Surface a finding in the Actions UI. Report-only runs exit 0, and nobody opens the log of a - * green check — without an annotation the finding is indistinguishable from silence. + * Surface a finding in the Actions UI: `error` when the run blocks, `warning` when it is advisory. + * An advisory run exits 0, and nobody opens the log of a green check — without the annotation the + * finding is indistinguishable from silence. */ -function annotate(message: string): void { - if (process.env.GITHUB_ACTIONS) console.log(`::warning::${message}`); +function annotate(message: string, level: 'warning' | 'error'): void { + if (process.env.GITHUB_ACTIONS) console.log(`::${level}::${message}`); +} + +/** + * Fold a noisy section away — collapsed in Actions, a plain header locally. The npm-install log and + * the non-curated duplicate list are ~600 lines between them, and burying the verdict in them is + * what makes a finding read as "there was an error". + */ +function groupStart(title: string): void { + console.log(process.env.GITHUB_ACTIONS ? `::group::${title}` : `\n--- ${title}`); +} + +function groupEnd(): void { + if (process.env.GITHUB_ACTIONS) console.log('::endgroup::'); +} + +/** Put the findings where they are seen without opening the log at all. */ +function writeStepSummary(markdown: string): void { + const file = process.env.GITHUB_STEP_SUMMARY; + if (!file) return; + try { + appendFileSync(file, markdown); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + console.log(`Could not write the job summary: ${reason}`); + } +} + +function seconds(startedAt: number): string { + return `${Math.round((Date.now() - startedAt) / 1000)}s`; } /** @@ -53,6 +98,12 @@ async function installWithRetry(scratch: string, attempts = 3): Promise { } } +/** Workspace packages the peer model exempts — the repo paths `isPeerRuleExempt` needs live here. */ +function exemptPackages(byName: Map): Set { + const names = [...byName].filter(([name, { relDir }]) => isPeerRuleExempt(name, relDir)); + return new Set(names.map(([name]) => name)); +} + interface WorkspacePkg { dir: string; relDir: string; @@ -158,7 +209,7 @@ export function resolveTargets( console.log('No changed publishable packages; nothing to verify.'); return null; } - console.log(`Changed publishable packages: ${targets.join(', ')}`); + console.log(`Changed publishable packages (${targets.length}): ${targets.join(', ')}`); return targets; } return args.filter((a) => !a.startsWith('--')); @@ -202,6 +253,7 @@ export async function runVerifyNpmInstall(args: string[], rootDir: string): Prom // inspecting — either way, keeping it just leaks a full node_modules into tmpdir per run. let keepScratch = false; try { + const packStartedAt = Date.now(); console.log(`Packing ${toPack.length} workspace package(s) (targets: ${targets.length})...`); const tarballByName: Record = {}; for (const name of toPack) { @@ -231,36 +283,80 @@ export async function runVerifyNpmInstall(args: string[], rootDir: string): Prom ), ); - console.log('Installing into scratch dir with npm...'); - await installWithRetry(scratch); + console.log(`Packed in ${seconds(packStartedAt)}. Installing into scratch dir with npm...`); + const installStartedAt = Date.now(); + // npm prints hundreds of ERESOLVE warnings here; they matter only when the install itself + // fails, so they are folded away rather than dropped. Actions cannot un-collapse a group + // after the fact, so a failure points back at it instead. + groupStart('npm install output'); + try { + await installWithRetry(scratch); + } catch (error) { + groupEnd(); + console.log('npm install failed — its output is in the "npm install output" group above.'); + throw error; + } + groupEnd(); + console.log(`Installed in ${seconds(installStartedAt)}.`); - const { duplicates, failures } = analyze(collectCopies(scratch)); + const found = collectCopies(scratch); + const { duplicates, failures } = analyze(found); + const workspaceNames = new Set(byName.keys()); + const curatedDuplicates = duplicates.filter((d) => d.isCurated); + const attributed = explainDuplicates(scratch, curatedDuplicates, workspaceNames); + const explained = attributed.filter((dup) => failures.some((f) => f.name === dup.name)); - console.log(`\nnpm-install closure — scratch: ${scratch}\n`); - const curatedTag = reportOnly ? 'CURATED DUP (report)' : 'FAIL'; - for (const d of duplicates) { - const tag = d.isCurated ? (d.allowed ? 'ALLOWED DUP' : curatedTag) : 'dup (report)'; - console.log( - ` ${d.name}: ${tag} — ${d.copies.length} copies (${d.copies.map((c) => `v${c.version}`).join(', ')})`, - ); + const nonCurated = duplicates.filter((d) => !d.isCurated); + if (nonCurated.length > 0) { + groupStart(`Other duplicated packages (report-only, NOT enforced — ${nonCurated.length})`); + for (const d of nonCurated) { + console.log( + ` ${d.name}: ${d.copies.length} copies (${d.copies.map((c) => `v${c.version}`).join(', ')})`, + ); + } + groupEnd(); + } + + const attributedCopies = new Map(attributed.map((dup) => [dup.name, dup.copies])); + console.log( + `\nCurated single-instance libraries — npm-install closure of ${targets.length} target(s):\n`, + ); + // Unattributed copies fall back to raw paths rather than rendering a "N copies" header with + // nothing under it. + for (const line of formatCuratedReport(found, duplicates, (dup) => { + const copies = attributedCopies.get(dup.name); + return copies + ? formatCopyLines(copies) + : dup.copies.map((c) => ` v${c.version} ${c.realPath}`); + })) { + console.log(line); } if (failures.length > 0) { keepScratch = !reportOnly; + const scratchHint = keepScratch ? scratch : undefined; + const remediationOptions = { exemptPackages: exemptPackages(byName), scratch: scratchHint }; + console.log('\nHow to fix\n'); + for (const line of formatRemediation(explained, remediationOptions)) console.log(line); + writeStepSummary(formatStepSummary(explained, { reportOnly, ...remediationOptions })); for (const f of failures) { annotate( `${f.name}: ${f.copies.length} copies in the npm-install graph (${f.copies.map((c) => `v${c.version}`).join(', ')})`, + reportOnly ? 'warning' : 'error', ); } - console.error( - `\n${reportOnly ? 'REPORT' : 'FAIL'}: ${failures.length} curated library duplicate(s) in the npm-install graph.`, + // stdout, not stderr: the two streams interleave in the Actions log, which is how the + // verdict ended up printed in the middle of the duplicate list. + console.log( + `\n${reportOnly ? 'REPORT' : 'FAIL'}: ${describeFailureCount(failures.length)}: ${failures.map((f) => f.name).join(', ')}${reportOnly ? ' (advisory — this job does not block yet)' : ''}`, ); return reportOnly ? 0 : 1; } console.log('\nOK: no curated duplicates in the npm-install graph.'); return 0; } finally { - if (keepScratch) console.log(`\nScratch install kept for inspection: ${scratch}`); - else rmSync(work, { recursive: true, force: true }); + // The kept tree is already pointed at from the remediation block, which is the only path + // that keeps one. + if (!keepScratch) rmSync(work, { recursive: true, force: true }); } }