diff --git a/.changeset/check-peers-outdated-range.md b/.changeset/check-peers-outdated-range.md new file mode 100644 index 00000000..def053f2 --- /dev/null +++ b/.changeset/check-peers-outdated-range.md @@ -0,0 +1,23 @@ +--- +'@openzeppelin/ui-dev-cli': minor +--- + +`check-peers` now warns when an adapter's declared `@openzeppelin/ui-*` range does not admit +the version installed beside it. + +The check compared the *minimum* a range admits against the installed version, mirroring +`validatePeerVersions` in `@openzeppelin/ui-utils`. That is right for the failure it was +built for — an installed peer that is too old — but it left the opposite drift invisible: an +adapter declaring `^2.0.0` against an installed `4.0.1` satisfies its own runtime check and +still cannot be installed by a consumer app, because a package manager reads the declared +range and not the baked minimum. Nothing anywhere reported it, which is how the +`openzeppelin-adapters` packages carried v2 ranges for `ui-components`, `ui-react` and +`ui-utils` across two majors of the kit. + +Such pairs are now reported as a `warning` with code `outdated-range`, naming the bump that +fixes each one. `AdapterPeerPair` gains `range` (the range as declared) and `rangeSatisfied` +(whether the range itself admits the installed version, or `null` for range syntax this check +does not model, which is left unreported rather than guessed at). `ok` and the exit code +remain driven by errors alone, so adding this to an existing pipeline does not change its +result; `printAdapterPeerResult` prints warnings on the passing path, where they would +otherwise be invisible. diff --git a/packages/dev-cli/README.md b/packages/dev-cli/README.md index 208db36c..7da0752e 100644 --- a/packages/dev-cli/README.md +++ b/packages/dev-cli/README.md @@ -42,13 +42,21 @@ It scans `node_modules/@openzeppelin` at the repository root and next to every w package declared in `pnpm-workspace.yaml`, so it works whether dependencies are hoisted to the root or installed under an app. For each installed adapter it compares the *minimum* of each declared `@openzeppelin/ui-*` range against the `ui-*` version actually installed, -matching `validatePeerVersions` semantics — a peer newer than the range is fine, because the -adapter only requires `>=` the minimum. +matching `validatePeerVersions` semantics — a peer newer than the range does not fail, +because the adapter only requires `>=` the minimum. + +That floor-only comparison hides the opposite drift, so a peer newer than the declared range +is reported as a **warning** (`outdated-range`) instead of passing silently. An adapter +declaring `^2.0.0` against an installed `4.0.1` satisfies its own runtime check and still +cannot be installed by a consumer app, because a package manager reads the declared range and +not the baked minimum. Ranges whose syntax the check does not model are left unreported rather +than guessed at. Exit code is `1` when a peer is stale, and also when nothing could be checked (no `@openzeppelin` packages, no adapters installed, or no peers resolved). Those are treated as failures on purpose: in CI they mean the install did not run or `--project` points at the -wrong root, and passing would be a false green. +wrong root, and passing would be a false green. An `outdated-range` warning does **not** fail +the command, so adding this to an existing pipeline does not change its exit code. Run it after install in CI: diff --git a/packages/dev-cli/src/lib/adapterPeers.test.ts b/packages/dev-cli/src/lib/adapterPeers.test.ts index 4230bce4..716985b4 100644 --- a/packages/dev-cli/src/lib/adapterPeers.test.ts +++ b/packages/dev-cli/src/lib/adapterPeers.test.ts @@ -10,6 +10,7 @@ import { collectPeerDeclaringManifests, compareSemver, minimumVersionOf, + rangeAdmits, } from './adapterPeers'; const tempRoots: string[] = []; @@ -187,6 +188,42 @@ describe('collectOverriddenPeers', () => { }); }); +describe('rangeAdmits', () => { + it('respects the caret upper bound the range minimum ignores', () => { + expect(rangeAdmits('^2.0.0', '2.9.9')).toBe(true); + expect(rangeAdmits('^2.0.0', '4.0.1')).toBe(false); + expect(rangeAdmits('^2.0.0', '1.9.9')).toBe(false); + }); + + it('pins the leftmost non-zero segment for 0.x carets, as npm does', () => { + expect(rangeAdmits('^0.5.0', '0.5.9')).toBe(true); + expect(rangeAdmits('^0.5.0', '0.6.0')).toBe(false); + expect(rangeAdmits('^0.0.3', '0.0.3')).toBe(true); + expect(rangeAdmits('^0.0.3', '0.0.4')).toBe(false); + }); + + it('reads tildes, inequalities, exact pins and wildcards', () => { + expect(rangeAdmits('~3.5.0', '3.5.9')).toBe(true); + expect(rangeAdmits('~3.5.0', '3.6.0')).toBe(false); + expect(rangeAdmits('>=2.0.0', '4.0.1')).toBe(true); + expect(rangeAdmits('<3.0.0', '4.0.1')).toBe(false); + expect(rangeAdmits('3.5.0', '3.5.0')).toBe(true); + expect(rangeAdmits('3.5.0', '3.5.1')).toBe(false); + expect(rangeAdmits('*', '4.0.1')).toBe(true); + }); + + it('admits a version matching any alternative of a union', () => { + expect(rangeAdmits('^18.0.0 || ^19.0.0', '19.2.0')).toBe(true); + expect(rangeAdmits('^18.0.0 || ^19.0.0', '20.0.0')).toBe(false); + }); + + it('returns null for range syntax it does not model, so nothing is guessed at', () => { + expect(rangeAdmits('workspace:^', '4.0.1')).toBeNull(); + expect(rangeAdmits('>=2.0.0 <5.0.0', '4.0.1')).toBeNull(); + expect(rangeAdmits('^2.0.0 || workspace:*', '4.0.1')).toBeNull(); + }); +}); + describe('checkAdapterPeers', () => { describe('dependencies hoisted to the repository root', () => { it('passes when every installed peer meets the declared minimum', () => { @@ -235,7 +272,7 @@ describe('checkAdapterPeers', () => { expect(checkAdapterPeers(projectRoot).ok).toBe(false); }); - it('accepts a peer newer than the range, mirroring validatePeerVersions', () => { + it('does not fail on a peer newer than the range, mirroring validatePeerVersions', () => { const projectRoot = createProjectRoot(); installAdapter(projectRoot, '', 'adapter-evm', { '@openzeppelin/ui-utils': '^2.0.0' }); installPeer(projectRoot, '', 'ui-utils', '4.0.0'); @@ -243,7 +280,13 @@ describe('checkAdapterPeers', () => { const result = checkAdapterPeers(projectRoot); expect(result.ok).toBe(true); - expect(result.pairs[0]).toMatchObject({ minimum: '2.0.0', installed: '4.0.0' }); + expect(result.pairs[0]).toMatchObject({ + minimum: '2.0.0', + installed: '4.0.0', + satisfied: true, + }); + // Not fatal, but not silent either -- see the outdated declared ranges block. + expect(result.issues.map((issue) => issue.code)).toEqual(['outdated-range']); }); it('ignores non-@openzeppelin/ui-* peers', () => { @@ -377,6 +420,95 @@ describe('checkAdapterPeers', () => { }); }); + describe('outdated declared ranges', () => { + /** + * The openzeppelin-adapters shape as of this change: every adapter declared + * ui-components/react/utils on the v2 line against an installed v3/v4 kit. The + * adapters' own runtime check passed, so nothing anywhere reported it, and it + * survived two majors. + */ + function createOutdatedRangeProject(): string { + const projectRoot = createProjectRoot(); + installAdapter(projectRoot, '', 'adapter-evm', { + '@openzeppelin/ui-components': '^2.0.0', + '@openzeppelin/ui-types': '^3.5.0', + }); + installPeer(projectRoot, '', 'ui-components', '3.9.0'); + installPeer(projectRoot, '', 'ui-types', '3.5.2'); + return projectRoot; + } + + it('warns without failing when the declared range excludes the installed version', () => { + const result = checkAdapterPeers(createOutdatedRangeProject()); + + expect(result.ok).toBe(true); + expect(result.issues).toEqual([ + { + severity: 'warning', + code: 'outdated-range', + message: + '@openzeppelin/adapter-evm declares @openzeppelin/ui-components ^2.0.0, which does not admit the installed 3.9.0. ' + + "The adapter's own runtime check passes, but a package manager reads the range and refuses the install.", + }, + ]); + }); + + it('records the declared range alongside the minimum it admits', () => { + const result = checkAdapterPeers(createOutdatedRangeProject()); + + expect(result.pairs).toEqual([ + expect.objectContaining({ + peer: '@openzeppelin/ui-components', + range: '^2.0.0', + minimum: '2.0.0', + installed: '3.9.0', + satisfied: true, + rangeSatisfied: false, + }), + expect.objectContaining({ + peer: '@openzeppelin/ui-types', + range: '^3.5.0', + installed: '3.5.2', + satisfied: true, + rangeSatisfied: true, + }), + ]); + }); + + it('names the bump that fixes each outdated range', () => { + const guidance = checkAdapterPeers(createOutdatedRangeProject()).remediation.join('\n'); + + expect(guidance).toContain('@openzeppelin/ui-components ^2.0.0 -> ^3.9.0'); + expect(guidance).not.toContain('@openzeppelin/ui-types'); + }); + + it('reports a stale peer once, as the error it is, rather than also as drift', () => { + const projectRoot = createProjectRoot(); + installAdapter(projectRoot, '', 'adapter-evm', { '@openzeppelin/ui-types': '^3.5.0' }); + installPeer(projectRoot, '', 'ui-types', '3.3.0'); + + const result = checkAdapterPeers(projectRoot); + + expect(result.ok).toBe(false); + expect(result.issues.map((issue) => issue.code)).toEqual(['stale-peer']); + expect(result.pairs[0]).toMatchObject({ satisfied: false, rangeSatisfied: false }); + }); + + it('stays quiet on a range whose syntax it cannot model', () => { + const projectRoot = createProjectRoot(); + installAdapter(projectRoot, '', 'adapter-evm', { + '@openzeppelin/ui-utils': '>=2.0.0 <5.0.0', + }); + installPeer(projectRoot, '', 'ui-utils', '4.0.1'); + + const result = checkAdapterPeers(projectRoot); + + expect(result.ok).toBe(true); + expect(result.issues).toEqual([]); + expect(result.pairs[0]).toMatchObject({ rangeSatisfied: null }); + }); + }); + describe('remediation guidance', () => { function createStaleProject(workspaceYaml: string): string { const projectRoot = createProjectRoot(); diff --git a/packages/dev-cli/src/lib/adapterPeers.ts b/packages/dev-cli/src/lib/adapterPeers.ts index 79ed51a9..4c9f0dc8 100644 --- a/packages/dev-cli/src/lib/adapterPeers.ts +++ b/packages/dev-cli/src/lib/adapterPeers.ts @@ -20,11 +20,19 @@ import { collectWorkspacePackageDirs } from './localDev'; * marked failed and never retried, leaving the UI spinning. That is how bumping the * adapters to a new major while leaving the `ui-*` pins behind reaches staging. * - * Semantics deliberately mirror `validatePeerVersions`. Each adapter bakes its peer - * minimums at build time as `range.replace(/^\^/, '')`, so the runtime compares + * Error semantics deliberately mirror `validatePeerVersions`. Each adapter bakes its + * peer minimums at build time as `range.replace(/^\^/, '')`, so the runtime compares * against the *minimum* the range admits, not the range itself: `ui-utils` 4.0.0 * against a declared `^2.0.0` passes, because the adapter only asks for `>=2.0.0`. - * Enforcing the caret strictly here would report failures the adapters do not have. + * Enforcing the caret strictly as an error here would report failures the adapters do + * not have. + * + * That floor-only comparison leaves the opposite drift invisible, which is its own + * defect: an adapter declaring `^2.0.0` against an installed 4.0.1 satisfies its own + * runtime check and still cannot be installed by anything on the current major, because + * a package manager reads the declared range and not the baked minimum. Nothing warned + * about that, so it survived two majors. Such pairs are reported as warnings -- named, + * but not fatal, so an existing `check-peers` in CI keeps its exit code. */ const ADAPTER_PREFIX = 'adapter-'; @@ -38,18 +46,26 @@ export interface AdapterPeerPair { adapter: string; /** Peer being checked, e.g. `@openzeppelin/ui-types`. */ peer: string; + /** Range as the adapter declares it, e.g. `^3.5.0`. */ + range: string; /** Lowest peer version the declared range admits, e.g. `3.5.0`. */ minimum: string; /** Peer version actually installed and therefore loaded at runtime. */ installed: string; satisfied: boolean; + /** + * Whether the declared range itself admits the installed version, as a package + * manager reads it. `null` when the range syntax is not one this module models, so + * an unrecognised range is never reported as drift. + */ + rangeSatisfied: boolean | null; /** Scope directory the adapter was found in, relative to the project root. */ scopeDir: string; } export interface AdapterPeerIssue { severity: 'error' | 'warning'; - code: 'stale-peer' | 'no-scope-dirs' | 'no-adapters' | 'no-peers-resolved'; + code: 'stale-peer' | 'outdated-range' | 'no-scope-dirs' | 'no-adapters' | 'no-peers-resolved'; message: string; } @@ -164,6 +180,92 @@ export function compareSemver(a: string, b: string): number { return 0; } +/** Parsed `major.minor.patch`, prerelease stripped. */ +function segmentsOf(version: string): [number, number, number] | null { + const match = /^(\d+)\.(\d+)\.(\d+)/.exec(version.replace(/^[vV]/, '')); + return match ? [Number(match[1]), Number(match[2]), Number(match[3])] : null; +} + +/** + * Whether one comparator admits `version`, or `null` for syntax this module does not + * model. Covers what the adapters and the kit actually declare: `^`, `~`, exact pins, + * the four inequalities, and the any-version wildcards. + */ +function comparatorAdmits(comparator: string, version: string): boolean | null { + const trimmed = comparator.trim(); + if (trimmed === '' || trimmed === '*' || trimmed === 'x') { + return true; + } + + const installed = segmentsOf(version); + if (!installed) { + return null; + } + + const match = /^(\^|~|>=|<=|>|<|=)?\s*(\d+\.\d+\.\d+[^\s]*)$/.exec(trimmed); + if (!match) { + return null; + } + + const bound = segmentsOf(match[2]); + if (!bound) { + return null; + } + + const order = compareSemver(version, match[2]); + + switch (match[1]) { + case '^': + // npm's caret pins the leftmost non-zero segment: ^0.5.x stays on 0.5, ^0.0.3 + // admits only 0.0.3. + if (order < 0) { + return false; + } + if (bound[0] !== 0) { + return installed[0] === bound[0]; + } + if (bound[1] !== 0) { + return installed[0] === 0 && installed[1] === bound[1]; + } + return order === 0; + case '~': + return order >= 0 && installed[0] === bound[0] && installed[1] === bound[1]; + case '>=': + return order >= 0; + case '<=': + return order <= 0; + case '>': + return order > 0; + case '<': + return order < 0; + default: + return order === 0; + } +} + +/** + * Whether the declared range admits `version` the way a package manager reads it, + * or `null` when the range is not modelled here. + * + * Unlike `minimumVersionOf`, this respects the range's upper bound, which is what + * decides whether an install succeeds. Only `||` unions and single comparators are + * understood; a range this returns `null` for is left unreported rather than guessed at. + */ +export function rangeAdmits(range: string, version: string): boolean | null { + const alternatives = range.split('||'); + let admitted = false; + + for (const alternative of alternatives) { + const verdict = comparatorAdmits(alternative, version); + if (verdict === null) { + return null; + } + admitted = admitted || verdict; + } + + return admitted; +} + /** * Project-relative package.json paths that declare a range for one of `peerNames`. * @@ -293,10 +395,42 @@ function buildRemediation(declaringManifests: string[], overriddenPeers: string[ return lines; } +function buildOutdatedRangeRemediation( + outdatedRangePairs: AdapterPeerPair[], + declaringManifests: string[] +): string[] { + const lines = [ + '', + 'These declared ranges are behind the versions installed beside them. The adapters', + 'run fine here because their runtime check only enforces the range minimum, so this', + 'stays quiet until a consumer app tries to install an adapter against the current', + 'kit and the package manager rejects the peer.', + '', + 'Fix: raise the range in the adapter manifest to the major that is actually installed.', + ]; + + for (const pair of outdatedRangePairs) { + lines.push(` - ${pair.adapter}: ${pair.peer} ${pair.range} -> ^${pair.installed}`); + } + + if (declaringManifests.length > 0) { + lines.push('', 'Also declared in this repository:'); + for (const manifest of declaringManifests) { + lines.push(` - ${manifest}`); + } + } + + return lines; +} + /** * Compares every installed adapter's `@openzeppelin/ui-*` peer minimum against the * `@openzeppelin/ui-*` version actually installed alongside it. * + * An installed version below the minimum is an error, matching what the adapter throws + * at runtime. An installed version the *declared range* does not admit is a warning: + * harmless in this tree, fatal for a consumer whose package manager reads the range. + * * Missing packages are treated as configuration errors rather than a silent pass: in * CI, "no adapters installed" almost always means the install did not run or the * command points at the wrong root, and passing there would be a false green. @@ -350,11 +484,25 @@ export function checkAdapterPeers(projectRootInput: string): AdapterPeerResult { } const stalePairs = pairs.filter((pair) => !pair.satisfied); - const issues: AdapterPeerIssue[] = stalePairs.map((pair) => ({ - severity: 'error' as const, - code: 'stale-peer' as const, - message: `${pair.adapter} requires ${pair.peer} >=${pair.minimum}, but ${pair.installed} is installed.`, - })); + + // Only pairs the floor check already passes: a stale peer is reported once, as the + // error it is, rather than twice. + const outdatedRangePairs = pairs.filter( + (pair) => pair.satisfied && pair.rangeSatisfied === false + ); + + const issues: AdapterPeerIssue[] = [ + ...stalePairs.map((pair) => ({ + severity: 'error' as const, + code: 'stale-peer' as const, + message: `${pair.adapter} requires ${pair.peer} >=${pair.minimum}, but ${pair.installed} is installed.`, + })), + ...outdatedRangePairs.map((pair) => ({ + severity: 'warning' as const, + code: 'outdated-range' as const, + message: `${pair.adapter} declares ${pair.peer} ${pair.range}, which does not admit the installed ${pair.installed}. The adapter's own runtime check passes, but a package manager reads the range and refuses the install.`, + })), + ]; const declaringManifests = collectPeerDeclaringManifests( projectRoot, @@ -362,15 +510,15 @@ export function checkAdapterPeers(projectRootInput: string): AdapterPeerResult { ); return { - ok: issues.length === 0, + ok: issues.every((issue) => issue.severity !== 'error'), projectRoot, scopeDirs: relativeScopeDirs, declaringManifests, overriddenPeers, pairs, issues, - remediation: - stalePairs.length === 0 + remediation: [ + ...(stalePairs.length === 0 ? [] : buildRemediation( collectPeerDeclaringManifests( @@ -378,7 +526,17 @@ export function checkAdapterPeers(projectRootInput: string): AdapterPeerResult { stalePairs.map((pair) => pair.peer) ), overriddenPeers - ), + )), + ...(outdatedRangePairs.length === 0 + ? [] + : buildOutdatedRangeRemediation( + outdatedRangePairs, + collectPeerDeclaringManifests( + projectRoot, + outdatedRangePairs.map((pair) => pair.peer) + ) + )), + ], }; } @@ -425,9 +583,11 @@ function collectPairs(projectRoot: string, scopeDirs: string[]): AdapterPeerPair pairs.set(key, { adapter, peer, + range, minimum, installed, satisfied: compareSemver(installed, minimum) >= 0, + rangeSatisfied: rangeAdmits(range, installed), scopeDir: path.relative(projectRoot, scopeDir), }); } diff --git a/packages/dev-cli/src/utils/logger.test.ts b/packages/dev-cli/src/utils/logger.test.ts new file mode 100644 index 00000000..8827cb37 --- /dev/null +++ b/packages/dev-cli/src/utils/logger.test.ts @@ -0,0 +1,74 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import type { AdapterPeerIssue, AdapterPeerResult } from '../lib/adapterPeers'; +import { printAdapterPeerResult } from './logger'; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +/** Captures what `printAdapterPeerResult` writes, stripped of colour. */ +function capture(result: AdapterPeerResult): string { + const lines: string[] = []; + vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => { + lines.push(String(chunk)); + return true; + }); + + printAdapterPeerResult(result); + + // Strips the ANSI colour picocolors adds. Built from a char code so the escape is not a + // control character inside a regex literal, which `no-control-regex` rejects. + const ansi = new RegExp(`${String.fromCharCode(27)}\\[\\d+m`, 'g'); + return lines.join('').replace(ansi, ''); +} + +function resultWith(overrides: Partial = {}): AdapterPeerResult { + return { + ok: true, + projectRoot: '/app', + scopeDirs: ['node_modules/@openzeppelin'], + declaringManifests: [], + overriddenPeers: [], + pairs: [], + issues: [], + remediation: [], + ...overrides, + }; +} + +const outdatedRange: AdapterPeerIssue = { + severity: 'warning', + code: 'outdated-range', + message: '@openzeppelin/adapter-evm declares @openzeppelin/ui-components ^2.0.0, ...', +}; + +describe('printAdapterPeerResult', () => { + it('prints warnings on the passing path, where they would otherwise be invisible', () => { + const output = capture( + resultWith({ ok: true, issues: [outdatedRange], remediation: ['Fix: raise the range.'] }) + ); + + expect(output).toContain('Adapter peer check passed'); + expect(output).toContain('[warning] outdated-range:'); + expect(output).toContain('Fix: raise the range.'); + }); + + it('says nothing beyond the summary when there is nothing to report', () => { + const output = capture(resultWith({ ok: true })); + + expect(output.trim()).toBe('Adapter peer check passed for /app (0 adapter/peer pairs)'); + }); + + it('still reports an error as a failure', () => { + const output = capture( + resultWith({ + ok: false, + issues: [{ severity: 'error', code: 'stale-peer', message: 'ui-types 3.3.0 installed.' }], + }) + ); + + expect(output).toContain('Adapter peer check failed'); + expect(output).toContain('[error] stale-peer: ui-types 3.3.0 installed.'); + }); +}); diff --git a/packages/dev-cli/src/utils/logger.ts b/packages/dev-cli/src/utils/logger.ts index ecfd518f..caeeff2c 100644 --- a/packages/dev-cli/src/utils/logger.ts +++ b/packages/dev-cli/src/utils/logger.ts @@ -102,12 +102,15 @@ export function printAdapterPeerResult(result: AdapterPeerResult): void { `Adapter peer check passed for ${result.projectRoot} (${result.pairs.length} adapter/peer ${pairLabel})` ) ); - return; + } else { + writeStdout(pc.red(`Adapter peer check failed for ${result.projectRoot}`)); } - writeStdout(pc.red(`Adapter peer check failed for ${result.projectRoot}`)); + // Printed on both paths: a run with only warnings still passes, and printing nothing + // there is how an outdated declared range stays unnoticed. for (const issue of result.issues) { - writeStdout(pc.red(` [${issue.severity}] ${issue.message}`)); + const color = issue.severity === 'error' ? pc.red : pc.yellow; + writeStdout(color(` [${issue.severity}] ${issue.code}: ${issue.message}`)); } if (result.remediation.length > 0) {