diff --git a/.changeset/not-installed.md b/.changeset/not-installed.md new file mode 100644 index 0000000..c001305 --- /dev/null +++ b/.changeset/not-installed.md @@ -0,0 +1,5 @@ +--- +"uptide": patch +--- + +`fix` and `check` in a checkout whose dependencies are not installed now stop before any analysis with `PACKAGE_NOT_INSTALLED` (exit 2), name the `package.json` that declares the package, say whether nothing is installed or only that package is missing, and give the frozen install for the lockfile (`npm ci`, `pnpm install --frozen-lockfile`, `yarn install --frozen-lockfile`, `yarn install --immutable`). Before, a `node_modules` in a directory above the project counted as the project's install, so a fresh clone under such a directory got past the check and `fix` failed with a misleading "has no upgrade". Uptide still installs nothing in your checkout. `check --json` now answers every refusal on stdout with `{"error": {"code", "message"}}`, as `fix --json` does. When the package is already at or past the target, `fix` says " is already at (target )". diff --git a/docs/commands/check.md b/docs/commands/check.md index 9b6ac70..266a6a2 100644 --- a/docs/commands/check.md +++ b/docs/commands/check.md @@ -28,7 +28,7 @@ whole-repository budgeted mode or `--max-time` flag. Install dependencies first. | `--all-deps` | also analyze `@types/*` and dependencies the repository never imports | | `--html [path]`, `--open` | a shareable report (below) | | `--cwd ` | repository to work on | -| `--json` | the full report as JSON on stdout | +| `--json` | the full report as JSON on stdout; when the run stops before a report, `{"error": {"code", "message"}}` instead (exit 2) | | `--ci` | plain log output: no color, no spinner (`NO_COLOR` is respected too) | While it runs, a terminal shows one live line with the current phase, which disappears when diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 93dea0c..868adf2 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -23,6 +23,19 @@ classic/Berry with the node-modules linker. Deno is not supported. lockfile` once (bun's own flag) so the repository has `bun.lock`, and `check` can read the installed versions. +## "declared in package.json but not installed" + +`check` and `fix` compile your code against the version installed in `node_modules`, so +they stop before any analysis when the package is not there, with the code `PACKAGE_NOT_INSTALLED` +(exit 2; `--json` carries it in `error.code`). "nothing is installed (no node_modules)" +means a fresh clone: install the dependencies, then run the same command again. "node_modules +is there, without it" means a partial install, for example a workspace package installed on +its own. The message names the `package.json` that declares the package, and the next +command is the frozen install for the repository's lockfile: `npm ci`, `pnpm install +--frozen-lockfile`, `yarn install --frozen-lockfile` or `yarn install --immutable`. Uptide +never installs into your checkout. Only the project's own `node_modules` counts: a copy in a +directory above the project root (a stray `~/node_modules`) is not its install. + ## Package manager not available, or the wrong version `fix` runs the package manager the repository names in `packageManager` (corepack's choice), diff --git a/packages/cli/src/__snapshots__/failures.test.ts.snap b/packages/cli/src/__snapshots__/failures.test.ts.snap index 994cd57..1ac2077 100644 --- a/packages/cli/src/__snapshots__/failures.test.ts.snap +++ b/packages/cli/src/__snapshots__/failures.test.ts.snap @@ -1,11 +1,23 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html +exports[`friendly failures: each says what to run next > a copy above the project root is not its install (UPT-136) 1`] = ` +[ + " Next: pnpm install --frozen-lockfile", +] +`; + exports[`friendly failures: each says what to run next > a dependency asked for by name that is not there: exit 2 1`] = ` [ " Next: npx uptide", ] `; +exports[`friendly failures: each says what to run next > a partial install names the package and the workspace that declares it 1`] = ` +[ + " Next: pnpm install --frozen-lockfile", +] +`; + exports[`friendly failures: each says what to run next > a registry error thrown by the engine is the same failure 1`] = ` [ " Next: run the same command again in a minute", @@ -16,6 +28,18 @@ exports[`friendly failures: each says what to run next > a repository without zo exports[`friendly failures: each says what to run next > breaking found and a dependency failed: breaking is the answer (exit 1) 1`] = `[]`; +exports[`friendly failures: each says what to run next > check --json answers a refusal with its code on stdout 1`] = ` +[ + " Next: pnpm install --frozen-lockfile", +] +`; + +exports[`friendly failures: each says what to run next > check --json answers a refusal with its code on stdout 2`] = ` +[ + " Next: npx uptide --json", +] +`; + exports[`friendly failures: each says what to run next > declared but missing from the lockfile 1`] = ` [ " Next: npm install", @@ -42,6 +66,18 @@ exports[`friendly failures: each says what to run next > fix outside git, below ] `; +exports[`friendly failures: each says what to run next > fix stops before any work when nothing is installed 1`] = ` +[ + " Next: pnpm install --frozen-lockfile", +] +`; + +exports[`friendly failures: each says what to run next > fix stops before any work when nothing is installed 2`] = ` +[ + " Next: pnpm install --frozen-lockfile", +] +`; + exports[`friendly failures: each says what to run next > missing ANTHROPIC_API_KEY: fix still runs with the rules 1`] = ` [ " Next: npx uptide fix zod (set ANTHROPIC_API_KEY, OPENAI_API_KEY or GEMINI_API_KEY in your environment first)", diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 372b48d..377e4e2 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -220,7 +220,7 @@ export async function run( } else { if (err instanceof CliError && err.options.next) err.options.next = suggest(err.options.next); - // `fix --json` answers on every exit, a failure included (UPT-107). + // `fix --json` and `check --json` answer on every exit, a failure included (UPT-107, UPT-136). if (flags.json && options.jsonErrors) emit({ ...(err instanceof ReportedError ? err.report : {}), @@ -736,7 +736,7 @@ ${EXIT_CODES( } return EXIT.ok; }, - { quiet: !flags.verbose }, + { quiet: !flags.verbose, jsonErrors: true }, ), ); diff --git a/packages/cli/src/errors.ts b/packages/cli/src/errors.ts index 0f38479..b8cd849 100644 --- a/packages/cli/src/errors.ts +++ b/packages/cli/src/errors.ts @@ -1,4 +1,5 @@ import { + type ErrorCode, IntegrityError, NoTypesError, PackageNotFoundError, @@ -13,6 +14,8 @@ export const EXIT = { ok: 0, breaking: 1, error: 2 } as const; /** A failure the user can act on. `next` is the exact command to run, when there is one. */ export class CliError extends Error { override readonly name = 'CliError'; + /** What `--json` reports; ANALYSIS_FAILED when unset. */ + readonly code: ErrorCode | undefined; constructor( message: string, readonly options: { @@ -20,9 +23,11 @@ export class CliError extends Error { next?: string; literalNext?: boolean; exitCode?: number; + code?: ErrorCode; } = {}, ) { super(message); + this.code = options.code; } } diff --git a/packages/cli/src/failures.test.ts b/packages/cli/src/failures.test.ts index 278689e..2dbfefc 100644 --- a/packages/cli/src/failures.test.ts +++ b/packages/cli/src/failures.test.ts @@ -1,4 +1,4 @@ -import { writeFileSync } from 'node:fs'; +import { rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { type PackageReport, UptideError } from '@uptide/core'; import { describe, expect, it } from 'vitest'; @@ -86,11 +86,84 @@ describe('friendly failures: each says what to run next', () => { ); expect(code).toBe(2); expect(stderr).toContain('✖ Dependencies'); - expect(stderr).toContain('error: zod and stripe are in the lockfile but not installed'); + expect(stderr).toContain( + 'error: zod and stripe are declared in package.json but not installed: nothing is installed (no node_modules)', + ); expect(stderr).toContain(`Next: ${next}\n`); expect(engine.calls).toEqual([]); }); + it('a partial install names the package and the workspace that declares it', async () => { + const cwd = tempRepo({ + 'package.json': '{"name":"mono"}', + 'pnpm-lock.yaml': '', + 'packages/api/package.json': manifest, + 'node_modules/stripe/package.json': '{}', + }); + const engine = fakeEngine({ + workspaces: async () => ['.', 'packages/api'], + declared: async (dir) => + new Map(dir.endsWith('api') ? [['zod', '^3.23.8']] : [['stripe', '14.25.0']]), + }); + const { code, stderr } = await fail(['check', 'zod'], cwd, engine); + expect(code).toBe(2); + expect(stderr).toContain( + 'error: zod is declared in packages/api/package.json but not installed: node_modules is there, without it', + ); + expect(stderr).toContain('Next: pnpm install --frozen-lockfile\n'); + expect(engine.calls).toEqual([]); + }); + + it('a copy above the project root is not its install (UPT-136)', async () => { + const parent = tempRepo({ + 'node_modules/zod/package.json': '{"name":"zod","version":"4.6.5"}', + 'shop/package.json': manifest, + 'shop/pnpm-lock.yaml': '', + }); + const { code, stderr } = await fail(['check', 'zod'], join(parent, 'shop')); + expect(code).toBe(2); + expect(stderr).toContain( + 'error: zod is declared in package.json but not installed: nothing is installed (no node_modules)', + ); + }); + + it('fix stops before any work when nothing is installed', async () => { + const root = pnpmGitRepo(); + rmSync(join(root, 'node_modules'), { recursive: true }); + const { code, stderr, engine } = await fail(['fix', '--only', 'zod'], root); + expect(code).toBe(2); + expect(stderr).toContain('✖ Repository'); + expect(stderr).toContain( + 'error: zod is declared in package.json but not installed: nothing is installed (no node_modules)', + ); + expect(stderr).toContain('Next: pnpm install --frozen-lockfile\n'); + expect(engine.calls).toEqual([]); + const json = await fail(['fix', '--only', 'zod', '--json'], root); + expect(json.code).toBe(2); + expect(JSON.parse(json.stdout).error.code).toBe('PACKAGE_NOT_INSTALLED'); + }); + + it('check --json answers a refusal with its code on stdout', async () => { + const missing = await fail( + ['check', 'zod', '--json'], + tempRepo({ 'package.json': manifest, 'pnpm-lock.yaml': '' }), + ); + expect(missing.code).toBe(2); + expect(JSON.parse(missing.stdout)).toEqual({ + error: { + code: 'PACKAGE_NOT_INSTALLED', + message: + 'zod is declared in package.json but not installed: nothing is installed (no node_modules)', + }, + }); + const engine = fakeEngine({ declared: async () => new Map([['zod', '^3.23.8']]) }); + const absent = await fail(['check', '--only', 'stripe', '--json'], npmRepo(), engine); + expect(absent.code).toBe(2); + expect(JSON.parse(absent.stdout)).toEqual({ + error: { code: 'ANALYSIS_FAILED', message: 'stripe is not a dependency of shop' }, + }); + }); + it('missing Berry installation names the immutable install command', async () => { const { code, stderr } = await fail( ['check', 'zod', 'stripe'], diff --git a/packages/cli/src/failures.ts b/packages/cli/src/failures.ts index 8b80d1b..4a008d7 100644 --- a/packages/cli/src/failures.ts +++ b/packages/cli/src/failures.ts @@ -50,11 +50,23 @@ export function yarnPlugAndPlay(root: string): CliError { }); } -export function missingNodeModules(repo: Repo, missing: readonly string[]): CliError { +/** Each missing dependency with a workspace that declares it. */ +export function missingNodeModules( + repo: Repo, + missing: readonly { name: string; workspace: string }[], +): CliError { + const and = (list: readonly string[]) => names(list).replace(' or ', ' and '); + const one = missing.length === 1; + const manifests = [...new Set(missing.map((m) => join(m.workspace, 'package.json')))]; return new CliError( - `${names(missing).replace(' or ', ' and ')} ${missing.length === 1 ? 'is' : 'are'} in the lockfile but not installed in node_modules`, + `${and(missing.map((m) => m.name))} ${one ? 'is' : 'are'} declared in ${and(manifests)} but not installed: ${ + existsSync(join(repo.root, 'node_modules')) + ? `node_modules is there, without ${one ? 'it' : 'them'}` + : 'nothing is installed (no node_modules)' + }`, { - why: 'The analysis compiles your code against the installed and the target version.', + code: 'PACKAGE_NOT_INSTALLED', + why: 'Install the dependencies first, then run the same command again. The analysis compiles your code against the installed and the target version.', next: repo.manager === 'yarn' && (existsSync(join(dirname(repo.lockfile), '.yarnrc.yml')) || @@ -133,11 +145,16 @@ export function networkFailure(report: CheckResult): CliError | undefined { : undefined; } -/** Whether `name` resolves from `dir` the way Node would look it up. */ -function installedIn(dir: string, name: string): boolean { - for (let current = dir; ; current = dirname(current)) { +/** + * Whether `name` resolves from workspace `w` the way Node would look it up, up to the + * project root: a copy above it (a stray `~/node_modules`) is not this repository's install + * (UPT-136). + */ +function installedIn(repo: Repo, w: string, name: string): boolean { + const top = resolve(repo.root); + for (let current = join(top, w); ; current = dirname(current)) { if (existsSync(join(current, 'node_modules', name, 'package.json'))) return true; - if (dirname(current) === current) return false; + if (current === top || dirname(current) === current) return false; } } @@ -145,9 +162,10 @@ function installedIn(dir: string, name: string): boolean { export function requireInstalled(repo: Repo, dependencies: readonly DependencyStatus[]): void { for (const dep of dependencies) if (dep.workspaces.length > 0 && !dep.installed) throw notInLockfile(repo, dep.name); - const missing = dependencies - .filter((dep) => dep.workspaces.some((w) => !installedIn(join(repo.root, w), dep.name))) - .map((dep) => dep.name); + const missing = dependencies.flatMap((dep) => { + const workspace = dep.workspaces.find((w) => !installedIn(repo, w, dep.name)); + return workspace === undefined ? [] : [{ name: dep.name, workspace }]; + }); if (missing.length > 0) throw missingNodeModules(repo, missing); } @@ -159,12 +177,12 @@ export function requireInstalled(repo: Repo, dependencies: readonly DependencySt export function requireNodeModules(repo: Repo, dependencies: readonly DependencyStatus[]): void { const declared = dependencies.filter((dep) => dep.workspaces.length > 0); const missing = declared.filter((dep) => - dep.workspaces.every((w) => !installedIn(join(repo.root, w), dep.name)), + dep.workspaces.every((w) => !installedIn(repo, w, dep.name)), ); if (declared.length > 0 && missing.length === declared.length) throw missingNodeModules( repo, - missing.slice(0, 3).map((dep) => dep.name), + missing.slice(0, 3).map((dep) => ({ name: dep.name, workspace: dep.workspaces[0] ?? '.' })), ); } diff --git a/packages/core/src/fix/isolate.test.ts b/packages/core/src/fix/isolate.test.ts index e40bb8c..e1ab187 100644 --- a/packages/core/src/fix/isolate.test.ts +++ b/packages/core/src/fix/isolate.test.ts @@ -1291,6 +1291,36 @@ describe('preflight refusals become agent-led drafts with an agent (UPT-78)', () expect(result.agentFallback?.join(';')).toContain('the check returned nothing to analyze'); }); + it('an empty check without an install says so, not "has no upgrade" (UPT-136)', async () => { + const { root, services } = zodFixture(scratch); + const empty = async () => ({ ...(await services.check({ cwd: root })), packages: [] }); + rmSync(join(root, 'node_modules'), { recursive: true }); + const error = await isolatedFix( + { cwd: root, ...agent }, + { ...services, check: empty, agentRunner: runner() }, + ).catch((e: unknown) => e); + expect(error).toMatchObject({ + code: 'PACKAGE_NOT_INSTALLED', + message: + 'zod is declared in package.json but not installed. Install the dependencies first: pnpm install --frozen-lockfile, then run uptide fix zod again.', + }); + }); + + it('a package already at the target says so', async () => { + const { root, services } = zodFixture(scratch); + const empty = async () => ({ ...(await services.check({ cwd: root })), packages: [] }); + writeFileSync( + join(root, 'node_modules/zod/package.json'), + JSON.stringify({ name: 'zod', version: '4.6.5', types: 'index.d.ts' }), + ); + await expect( + isolatedFix( + { cwd: root, only: 'zod', fixer: null, tool: TOOL }, + { ...services, check: empty }, + ), + ).rejects.toThrow(/^zod is already at 4\.6\.5 \(target 4\.6\.5\)$/); + }); + it('never falls back on a safety refusal', async () => { const { root, services } = zodFixture(scratch); const planner = vi diff --git a/packages/core/src/fix/managers/manager.test.ts b/packages/core/src/fix/managers/manager.test.ts index f4b555f..dbf841d 100644 --- a/packages/core/src/fix/managers/manager.test.ts +++ b/packages/core/src/fix/managers/manager.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path'; import { afterAll, expect, it } from 'vitest'; import { versionRange } from '../versions.js'; import { assertLockScope } from './lock-guard.js'; -import { berrySkipBuild, packageManager, updateArgs } from './manager.js'; +import { berrySkipBuild, frozenInstall, packageManager, updateArgs } from './manager.js'; import { pnpmGraph, yarnGraph } from './text-lock.js'; const roots: string[] = []; @@ -18,6 +18,14 @@ function fixture(files: Record) { for (const [f, t] of Object.entries(files)) if (t !== undefined) writeFileSync(join(root, f), t); return root; } +it.each([ + [{ 'package-lock.json': '{"lockfileVersion":3}' }, 'npm ci'], + [{ 'pnpm-lock.yaml': '' }, 'pnpm install --frozen-lockfile'], + [{ 'yarn.lock': '' }, 'yarn install --frozen-lockfile'], + [{ 'yarn.lock': '__metadata:\n version: 8\n' }, 'yarn install --immutable'], +])('the install a person runs for %j is %s', (files, command) => { + expect(frozenInstall(packageManager(fixture(files)))).toBe(command); +}); it.each(['^', '~', ''])('preserves the %s range style', (style) => { expect(versionRange(`${style}3.23.8`, '4.6.5')).toBe(`${style}4.6.5`); }); diff --git a/packages/core/src/fix/managers/manager.ts b/packages/core/src/fix/managers/manager.ts index 1e9c611..4d106d8 100644 --- a/packages/core/src/fix/managers/manager.ts +++ b/packages/core/src/fix/managers/manager.ts @@ -102,6 +102,15 @@ export function packageManager(root: string): PackageManager { 'fix supports npm (lockfile v2/v3), pnpm, and Yarn classic/Berry; no supported lockfile found', ); } +/** The install a person runs to get exactly what the lockfile pins. */ +export function frozenInstall(pm: PackageManager): string { + return { + npm: 'npm ci', + pnpm: 'pnpm install --frozen-lockfile', + 'yarn-classic': 'yarn install --frozen-lockfile', + 'yarn-berry': 'yarn install --immutable', + }[pm.kind]; +} /** Manifests have already been updated, so npm can preserve dependency sections with no add/save flags. */ export function updateArgs(pm: PackageManager, root: string, workspaces: string[]): string[] { const args = managerArgs(pm, root, workspaces); diff --git a/packages/core/src/fix/run.ts b/packages/core/src/fix/run.ts index bd833d3..46e6ea0 100644 --- a/packages/core/src/fix/run.ts +++ b/packages/core/src/fix/run.ts @@ -123,6 +123,7 @@ import { assertNoDowngrades, bumpVersions, declareLead, + frozenInstall, install, packageManager, removeDeclarations, @@ -496,10 +497,23 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise p.installed !== 'unknown' && compareVersions(p.installed, target) < 0, - ); - if (!behind.length) throw new Error(`${pack.name} has no upgrade to ${target}`); + const declared = fallbackReport(root, pack.name, target, '').packages; + // Nothing installed reads as nothing to upgrade; the cause is the install (UPT-136). + const missing = declared.find((p) => p.installed === 'unknown'); + if (missing) + throw new UptideError( + 'PACKAGE_NOT_INSTALLED', + `${pack.name} is declared in ${join(missing.workspace, 'package.json')} but not installed. Install the dependencies first: ${frozenInstall(packageManager(root))}, then run uptide fix ${pack.name} again.`, + ); + const behind = declared.filter((p) => compareVersions(p.installed, target) < 0); + // Everything declared is at or past the target (an alternate lead declares nothing here). + const at = declared.map((p) => p.installed).sort(compareVersions)[0]; + if (!behind.length) + throw new Error( + at + ? `${pack.name} is already at ${at} (target ${target})` + : `${pack.name} is not declared in any package.json of this repository`, + ); const reason = `the check returned nothing to analyze for ${pack.name} ${behind[0]?.installed} → ${target} (no import or pack site found)`; if (!canAssist) throw new UptideError('INCOMPLETE_CHECK', `${reason}. Nothing was changed.`); fallback.push(`Planner refused: ${reason}`); diff --git a/packages/core/src/fix/versions.ts b/packages/core/src/fix/versions.ts index 7db8f59..d17f0d9 100644 --- a/packages/core/src/fix/versions.ts +++ b/packages/core/src/fix/versions.ts @@ -2,7 +2,7 @@ import { assertManagerAvailable } from './managers/availability.js'; import { isolatedUpgrade } from './managers/isolated-install.js'; import type { InstallReport, Upgrade } from './managers/upgrade.js'; -export { packageManager } from './managers/manager.js'; +export { frozenInstall, packageManager } from './managers/manager.js'; import { readFileSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path';