From 1f0ba5e1b01b679bd5e7f26ac0b12fda628070f2 Mon Sep 17 00:00:00 2001 From: Wade Baglin Date: Wed, 21 Oct 2026 16:09:51 +1000 Subject: [PATCH 1/2] feat: add pnpm support verifyx detects whether a project uses npm or pnpm and uses that manager: verify:* and test scripts run with it, `verifyx init` installs devDependencies with it, and the generated agent docs name its commands. npm projects behave as before. The manager comes from devEngines.packageManager, then packageManager, then lockfiles, nearest directory first. On pnpm, init requires 11.26+ or 12.4+ and declines skott's unneeded @parcel/watcher build. --- README.md | 50 +++++++-- src/checks/external.ts | 7 +- src/checks/registry.ts | 1 + src/checks/types.ts | 2 + src/commands/registerInit.ts | 43 ++++++-- src/commands/registerUpgradeDocs.ts | 4 +- src/orchestrator/resolveEntries.test.ts | 45 +++++++- src/orchestrator/resolveEntries.ts | 17 ++- src/orchestrator/run.ts | 6 +- src/orchestrator/runAll.ts | 15 ++- src/orchestrator/tests.test.ts | 40 ++++++- src/orchestrator/tests.ts | 6 +- src/scaffold/agentFiles.ts | 26 +++-- src/scaffold/init.test.ts | 71 +++++++++++- src/scaffold/init.ts | 11 +- src/scaffold/installDeps.test.ts | 121 +++++++++++++++++---- src/scaffold/installDeps.ts | 94 ++++++++++++---- src/scaffold/pnpmVersion.test.ts | 40 +++++++ src/scaffold/pnpmVersion.ts | 30 +++++ src/scaffold/writeManaged.test.ts | 20 +++- src/scaffold/writeManaged.ts | 8 +- src/shared/packageManager.test.ts | 139 ++++++++++++++++++++++++ src/shared/packageManager.ts | 121 +++++++++++++++++++++ templates/skills/verify/SKILL.md | 2 +- templates/verify-guidance.md | 2 +- 25 files changed, 818 insertions(+), 103 deletions(-) create mode 100644 src/scaffold/pnpmVersion.test.ts create mode 100644 src/scaffold/pnpmVersion.ts create mode 100644 src/shared/packageManager.test.ts create mode 100644 src/shared/packageManager.ts diff --git a/README.md b/README.md index e90cce6..38e462b 100644 --- a/README.md +++ b/README.md @@ -19,15 +19,17 @@ What makes it worth wiring in: Install it as a **dev dependency** so the version is pinned in `package.json`, so the exact same tool runs on your machine and in CI/CD: ```sh -npm install --save-dev @makerx/verify +npm install --save-dev @makerx/verify # npm +pnpm add -D @makerx/verify # pnpm (add -w at a workspace root) ``` -Requires Node.js >= 24. Invoke it via an npm script or `npx verifyx`. +Requires Node.js >= 24. Invoke it via a package.json script, `npx verifyx` (npm) or `pnpm exec verifyx` (pnpm). The quickest way to wire it into a project: ```sh -npx verifyx init +npx verifyx init # npm +pnpm exec verifyx init # pnpm (needs pnpm 11.26+ or 12.4+) ``` > [!NOTE] @@ -40,6 +42,16 @@ npx verifyx init > and a typed `verifyx`) work identically on macOS, Linux, and Windows. Your npm **script** can still be named > `verify` (that's a script lookup, not command resolution), so `npm run verify` works everywhere. +## Package managers + +verifyx works with npm and pnpm, and uses the project's package manager to run scripts, install tools and write agent docs. It looks in the project directory, then its parents, and goes by the first of these it finds: + +1. `devEngines.packageManager` in `package.json` +2. `packageManager` in `package.json`, e.g. `"pnpm@11.26.0"` +3. a lockfile: `pnpm-lock.yaml` or `pnpm-workspace.yaml` for pnpm, `package-lock.json` for npm + +On pnpm, checks run with `pnpm run`, and skip pnpm's dependency check so parallel checks never install mid-run. Run `pnpm verify` to have pnpm check dependencies once before verifyx starts. + ## How `verifyx` decides what to run A **`verify:*` script** is any npm script in your `package.json` whose name starts with `verify:` (for example `verify:lint`, `verify:complexity`, `verify:custom`). Each one is a single **check**. `verifyx` runs them all **in parallel**, and together they form your project's verification gate. You decide which checks make up that gate by adding these scripts (`verifyx init` scaffolds a starting set). @@ -130,7 +142,7 @@ Flags on the bare `verifyx` command: External checks spawn their tool and **skip gracefully when it is not installed**; `verifyx init` installs the ones you opt into. They run the tool from your local `node_modules/.bin` regardless of how `verifyx` was invoked. `oxlint`/`oxfmt`/`tsc` are resolved if present; the rest are declared as optional `peerDependencies`. -Because checks are named for their function, **on failure** an external check prints the tool it used, the exact argv it ran, and a docs link, so you (or an agent) can add the tool's config (e.g. `knip.json`) without guessing. On success it prints nothing (output is buffered and flushed only on failure, to keep runs quiet and cheap). If you override a check with your own `verify:` script, a failure shows that `npm run verify:` was what ran. +Because checks are named for their function, **on failure** an external check prints the tool it used, the exact argv it ran, and a docs link, so you (or an agent) can add the tool's config (e.g. `knip.json`) without guessing. On success it prints nothing (output is buffered and flushed only on failure, to keep runs quiet and cheap). If you override a check with your own `verify:` script, a failure shows the command that ran (`npm run verify:` or `pnpm run verify:`). **Passing extra arguments through.** When you run an external check directly, anything after `--` is forwarded as literal argv entries to the underlying tool, so you can tweak an invocation without ejecting it: `verifyx circular-deps -- src/index.ts` asks skott to start at that file, while `verifyx lint -- --quiet` passes `--quiet` to oxlint. With no entrypoint, skott scans the current project, so `verifyx init` scaffolds the cross-platform default `verifyx circular-deps` without a shell glob. @@ -264,9 +276,9 @@ verifyx init It first asks how `verify` should run: **run all built-in checks** (`verifyx all`, no `verify:*` scripts) or **pick specific checks** to wire up. Then you multi-select **agent targets** (Claude and/or other agents), and, if you chose to pick, the **checks**. After that it: - writes the selected `verify:*` scripts to `package.json` (never clobbering existing ones), -- installs the external checks' tools as `--save-dev`, **skipping any already declared in `package.json` or present in `node_modules`** (so an existing `typescript`/`oxlint` is never re-installed or version-bumped). If the install hits a conflict (e.g. a peer-dependency clash), it isolates the failing package(s), installs the rest, and reports what to install manually at the end instead of aborting, +- installs the external checks' tools as devDependencies with the project's package manager, **skipping any already declared in `package.json` or present in `node_modules`** (so an existing `typescript`/`oxlint` is never re-installed or version-bumped). On npm, if the install hits a conflict (e.g. a peer-dependency clash), it isolates the failing package(s), installs the rest, and reports what to install manually at the end instead of aborting (for pnpm, see [On pnpm](#on-pnpm)), - writes the **`verify` skill**: the same `SKILL.md` to `.claude/skills/verify/` (Claude) and `.agent-skills/verify/` (cross-vendor), so the integration is identical everywhere, -- appends a one-line pointer to `CLAUDE.md` / `AGENTS.md` (only if not already present; existing content is never rewritten), +- appends a one-line pointer to `CLAUDE.md` / `AGENTS.md` (only if not already present; existing content is never rewritten). Both use the project's command (`npm run verify` or `pnpm verify`), - if `unused-code` is selected, adds the other external tools (`oxlint`/`oxfmt`/`skott`/`jscpd`) to knip's `ignoreDependencies` (verifyx runs them at runtime, so knip can't see them and would otherwise report them as unused), and adds system binaries your scripts invoke (`uv`, `az`, `python`, `terraform`, …) that knip's own global list doesn't cover to `ignoreBinaries`. Merged into `knip.json` or `package.json#knip` (created if neither exists), adding only what's missing; a code-based `knip.ts`/`knip.js` is left for you to edit. The skill auto-triggers on "verify"/"run checks", so agents run the checks proactively; the pointer reinforces it for tools that read `CLAUDE.md`/`AGENTS.md` as standing instructions. @@ -276,6 +288,11 @@ Options: - `--defaults-only`: the non-interactive form of the "run all built-in checks" choice. Does **not** write `verify:*` scripts; wires the top-level `verify` script to `verifyx all` so it runs every built-in (still installs opted-in tools and writes the skill + pointer). - `--yes`: non-interactive; use `--select ` (repeatable), `--no-claude`, `--agents`. +#### On pnpm + +- init needs pnpm 11.26+ or 12.4+, and stops before changing anything on an older version. +- pnpm 11+ stops an install when a dependency has an install script (such as a native build) that the project hasn't allowed or blocked under `allowBuilds` in `pnpm-workspace.yaml`. init blocks the one it knows about: `@parcel/watcher`, which skott pulls in but doesn't need. + ### `verifyx upgrade-docs` Idempotently create/refresh the skill and the `CLAUDE.md`/`AGENTS.md` pointer (created / appended / updated / unchanged; refuses to write through symlinks, never rewrites your instruction files): @@ -334,7 +351,26 @@ jobs: - run: npm run verify ``` -`npm run verify` exits non-zero on the first failing check (failing the job), and its buffered output is flushed so the log shows exactly what failed and how to fix it. To force a mode regardless of environment, use `verifyx --check` or `verifyx --fix`. +The same job on pnpm (`pnpm/action-setup` reads the version from `packageManager`): + +```yaml +name: verify +on: [push, pull_request] +jobs: + verify: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: pnpm/action-setup@v6 + - uses: actions/setup-node@v4 + with: + node-version: 24 + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm verify +``` + +`npm run verify` (or `pnpm verify`) exits non-zero on the first failing check (failing the job), and its buffered output is flushed so the log shows exactly what failed and how to fix it. To force a mode regardless of environment, use `verifyx --check` or `verifyx --fix`. ## Programmatic API diff --git a/src/checks/external.ts b/src/checks/external.ts index d1da8c3..d4fb272 100644 --- a/src/checks/external.ts +++ b/src/checks/external.ts @@ -4,6 +4,7 @@ import path from 'node:path' import { color } from '../shared/color.ts' import { resolveMode } from '../shared/mode.ts' import { emit } from '../shared/output.ts' +import { detectPackageManager, PACKAGE_MANAGER_COMMANDS } from '../shared/packageManager.ts' import { appendArgv, formatArgv, formatShellCommand, runArgvCommand } from '../shared/spawn.ts' import { type MaxWarningsSupport, withinBudget } from './maxWarnings.ts' import type { Check, CheckMode, CheckResult, RunDefaultOptions } from './types.ts' @@ -37,6 +38,8 @@ export type ExternalCheckSpec = { /** Command run in fix mode. When omitted, the check is not fixable and always runs `checkCommand`. */ fixCommand?: readonly string[] devDeps: string[] + /** Dependency build scripts the tool does not need, declined when pnpm installs `devDeps`. */ + declineBuilds?: string[] recommended?: boolean /** Docs / config reference for the underlying tool, surfaced when the check fails. */ docs?: string @@ -114,6 +117,7 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { scaffold: { script: `verifyx ${spec.name}`, devDeps: spec.devDeps, + declineBuilds: spec.declineBuilds, }, // `verifyx eject ` inlines these raw commands into the consumer's verify:* scripts. Lazy so // formatShellCommand's unserializable-arg throw fires on eject only, not when CHECKS is built at import. @@ -125,7 +129,8 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { }, async runDefault({ extraArgs = [], maxWarnings }: RunDefaultOptions = {}): Promise { if (!hasLocalBin(spec.bin)) { - console.log(color.dim(`${spec.name}: ${spec.bin} not installed — skipping (add it with \`npx verifyx init\`)`)) + const cli = PACKAGE_MANAGER_COMMANDS[detectPackageManager(process.cwd())].runCli + console.log(color.dim(`${spec.name}: ${spec.bin} not installed — skipping (add it with \`${cli} init\`)`)) return { name: spec.name, ok: true, skipped: true } } if (spec.canRun && !spec.canRun()) { diff --git a/src/checks/registry.ts b/src/checks/registry.ts index 642c1d3..d53e849 100644 --- a/src/checks/registry.ts +++ b/src/checks/registry.ts @@ -87,6 +87,7 @@ export const CHECKS: Check[] = [ bin: 'skott', checkCommand: ['skott', '--displayMode=raw', '--showCircularDependencies', '--exitCodeOnCircularDependencies=1'], devDeps: ['skott'], + declineBuilds: ['@parcel/watcher'], docs: 'https://github.com/antoine-coulon/skott', }), defineExternalCheck({ diff --git a/src/checks/types.ts b/src/checks/types.ts index 4167564..42d730f 100644 --- a/src/checks/types.ts +++ b/src/checks/types.ts @@ -34,6 +34,8 @@ export type Check = { script: string /** Extra devDependencies the check needs, installed on opt-in. */ devDeps?: string[] + /** Packages whose build scripts the check does not need; pnpm records them as declined at install (npm ignores this). */ + declineBuilds?: string[] } /** * External checks only: the raw tool commands `verifyx eject ` inlines into `verify:` (and diff --git a/src/commands/registerInit.ts b/src/commands/registerInit.ts index 32aa9a7..6311a82 100644 --- a/src/commands/registerInit.ts +++ b/src/commands/registerInit.ts @@ -5,8 +5,11 @@ import { CHECKS, recommendedChecks } from '../checks/registry.ts' import type { AgentTarget } from '../scaffold/agentFiles.ts' import { applyInit, type InitResult } from '../scaffold/init.ts' import { type InstallReport, installDevDeps } from '../scaffold/installDeps.ts' +import { checkPnpmVersion } from '../scaffold/pnpmVersion.ts' import { ACTION_MARK } from '../scaffold/writeManaged.ts' import { color } from '../shared/color.ts' +import { detectPackageManager, PACKAGE_MANAGER_COMMANDS, type PackageManager } from '../shared/packageManager.ts' +import { captureArgvCommand } from '../shared/spawn.ts' export type Choice = { name: string; message: string; enabled?: boolean } @@ -84,8 +87,18 @@ function report(result: InitResult, defaultsOnly: boolean): void { } } +/** How to finish a failed install by hand. pnpm already prints the cause, so for pnpm this only points to that output. */ +function manualInstallAdvice(manager: PackageManager, failed: readonly string[]): string { + if (manager === 'pnpm') return " (see pnpm's output above)" + return ( + `\n init finished; the rest is wired up. Resolve these yourself — e.g. a peer-dependency conflict may need` + + `\n a compatible version or --legacy-peer-deps. Install manually with:` + + `\n ${PACKAGE_MANAGER_COMMANDS.npm.addDevDependencies(failed).join(' ')}` + ) +} + /** Summarise the dependency install: what was skipped, installed, and — last, so it's the takeaway — what failed. */ -function reportInstall(install: InstallReport): void { +function reportInstall(install: InstallReport, manager: PackageManager): void { if (install.skipped.length > 0) { console.log(color.dim(`\nSkipped ${install.skipped.length} already-installed devDependenc(ies): ${install.skipped.join(', ')}`)) } @@ -95,15 +108,17 @@ function reportInstall(install: InstallReport): void { if (install.failed.length > 0) { console.error( color.yellow( - `\n⚠ Failed to install ${install.failed.length} devDependenc(ies): ${install.failed.join(', ')}` + - `\n init finished; the rest is wired up. Resolve these yourself — e.g. a peer-dependency conflict may need` + - `\n a compatible version or --legacy-peer-deps. Install manually with:` + - `\n npm install --save-dev ${install.failed.join(' ')}`, + `\n⚠ Failed to install ${install.failed.length} devDependenc(ies): ${install.failed.join(', ')}${manualInstallAdvice(manager, install.failed)}`, ), ) } } +async function pnpmBlocker(cwd: string): Promise { + const check = checkPnpmVersion(await captureArgvCommand(['pnpm', '--version'], { cwd })) + return check.ok ? undefined : check.message +} + /** `verifyx init` — interactively scaffold checks + agent files into the current project. */ export function registerInit(program: Command): void { program @@ -116,16 +131,28 @@ export function registerInit(program: Command): void { .option('--agents', 'also write .agent-skills/ files (non-interactive)') .action(async (opts: InitCliOptions) => { const cwd = process.cwd() + const manager = detectPackageManager(cwd) + const blocker = manager === 'pnpm' ? await pnpmBlocker(cwd) : undefined + if (blocker) { + console.error(color.yellow(blocker)) + process.exitCode = 1 + return + } const { checks, targets, defaultsOnly } = await resolveSelections(opts) - const result = applyInit({ cwd, checks, targets, defaultsOnly }) + const result = applyInit({ cwd, checks, targets, defaultsOnly, packageManager: manager }) report(result, defaultsOnly) + let install: InstallReport | undefined if (result.devDeps.length > 0) { console.log(color.dim(`\nResolving ${result.devDeps.length} devDependenc(ies): ${result.devDeps.join(', ')}`)) - reportInstall(await installDevDeps(result.devDeps, cwd)) + install = await installDevDeps(result.devDeps, cwd, { packageManager: manager, declineBuilds: result.declineBuilds }) + reportInstall(install, manager) } - console.log(color.dim('\nRun `npm run verify` (or `npx verifyx`) to run your verifications.')) + const { runGate, runCli } = PACKAGE_MANAGER_COMMANDS[manager] + console.log(color.dim(`\nRun \`${runGate}\` (or \`${runCli}\`) to run your verifications.`)) + // pnpm only: a failed add usually leaves every later `pnpm install` failing too. + if (manager === 'pnpm' && (install?.failed.length ?? 0) > 0) process.exitCode = 1 }) } diff --git a/src/commands/registerUpgradeDocs.ts b/src/commands/registerUpgradeDocs.ts index 5ea16ba..2930818 100644 --- a/src/commands/registerUpgradeDocs.ts +++ b/src/commands/registerUpgradeDocs.ts @@ -2,6 +2,7 @@ import type { Command } from 'commander' import { type AgentTarget, writeAgentFiles } from '../scaffold/agentFiles.ts' import { ACTION_MARK, summarise } from '../scaffold/writeManaged.ts' +import { detectPackageManager } from '../shared/packageManager.ts' /** `verifyx upgrade-docs` — refresh the verify skill and append the pointer to CLAUDE.md / AGENTS.md. */ export function registerUpgradeDocs(program: Command): void { @@ -15,7 +16,8 @@ export function registerUpgradeDocs(program: Command): void { if (opts.claude !== false) targets.push('claude') if (opts.agents !== false) targets.push('agents') - const results = writeAgentFiles(process.cwd(), targets) + const cwd = process.cwd() + const results = writeAgentFiles(cwd, targets, detectPackageManager(cwd)) for (const result of results) { if (result.action === 'unchanged') continue console.log(` ${ACTION_MARK[result.action]} ${result.path} (${result.action})`) diff --git a/src/orchestrator/resolveEntries.test.ts b/src/orchestrator/resolveEntries.test.ts index d5c503c..66df982 100644 --- a/src/orchestrator/resolveEntries.test.ts +++ b/src/orchestrator/resolveEntries.test.ts @@ -2,9 +2,13 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { entryCheckName, resolveEntries, resolveOverride, selectEntries, type VerifyEntry } from './resolveEntries.ts' +const runShellCommand = vi.hoisted(() => vi.fn()) + +vi.mock('../shared/spawn.ts', () => ({ runShellCommand })) + +import { entryCheckName, resolveEntries, resolveOverride, runEntryCommand, selectEntries, type VerifyEntry } from './resolveEntries.ts' function entry(name: string): VerifyEntry { return { name, command: `npm run ${name}`, cwd: '.' } @@ -27,12 +31,49 @@ describe('resolveEntries', () => { expect(entries.find((e) => e.name === 'verify:a')?.command).toBe('npm run verify:a') }) + it('builds npm entries with no env key', () => { + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { 'verify:a': 'x' } })) + fs.writeFileSync(path.join(dir, 'package-lock.json'), '{}') + expect(resolveEntries(dir)).toStrictEqual([{ name: 'verify:a', command: 'npm run verify:a', cwd: dir }]) + }) + + it('builds pnpm entries that skip the per-run dependency check', () => { + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { 'verify:a': 'x' } })) + fs.writeFileSync(path.join(dir, 'pnpm-lock.yaml'), '') + expect(resolveEntries(dir)).toEqual([ + { name: 'verify:a', command: 'pnpm run verify:a', cwd: dir, env: { pnpm_config_verify_deps_before_run: 'false' } }, + ]) + }) + + it('detects the manager from the package.json directory, not the start directory', () => { + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { 'verify:a': 'x' } })) + fs.writeFileSync(path.join(dir, 'pnpm-lock.yaml'), '') + // An npm signal between the start dir and the package.json dir, which a walk from the start dir would hit first. + fs.mkdirSync(path.join(dir, 'src')) + fs.writeFileSync(path.join(dir, 'src', 'package-lock.json'), '{}') + const nested = path.join(dir, 'src', 'deep') + fs.mkdirSync(nested) + expect(resolveEntries(nested)[0]).toMatchObject({ command: 'pnpm run verify:a', cwd: dir }) + }) + it('returns [] when there are no verify:* scripts', () => { fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { build: 'x' } })) expect(resolveEntries(dir)).toEqual([]) }) }) +describe('runEntryCommand', () => { + afterEach(() => runShellCommand.mockReset()) + + it('forwards the entry cwd and env to the shell runner, quietly', async () => { + runShellCommand.mockResolvedValueOnce(3) + const env = { pnpm_config_verify_deps_before_run: 'false' } + const code = await runEntryCommand({ name: 'verify:a', command: 'pnpm run verify:a', cwd: dir, env }) + expect(code).toBe(3) + expect(runShellCommand).toHaveBeenCalledWith('pnpm run verify:a', { cwd: dir, env, quiet: true }) + }) +}) + describe('entryCheckName', () => { it('strips the verify: prefix', () => { expect(entryCheckName('verify:lint')).toBe('lint') diff --git a/src/orchestrator/resolveEntries.ts b/src/orchestrator/resolveEntries.ts index 8e60e79..53931c4 100644 --- a/src/orchestrator/resolveEntries.ts +++ b/src/orchestrator/resolveEntries.ts @@ -2,11 +2,24 @@ import fs from 'node:fs' import path from 'node:path' import type { CheckMode } from '../checks/types.ts' +import { detectPackageManager, PACKAGE_MANAGER_COMMANDS } from '../shared/packageManager.ts' +import { runShellCommand } from '../shared/spawn.ts' export type VerifyEntry = { name: string command: string cwd: string + /** Extra env for the child, merged over `process.env`. */ + env?: Record +} + +export function scriptEntry(name: string, dir: string): VerifyEntry { + const { command, env } = PACKAGE_MANAGER_COMMANDS[detectPackageManager(dir)].runScript(name) + return env ? { name, command, cwd: dir, env } : { name, command, cwd: dir } +} + +export function runEntryCommand(entry: VerifyEntry): Promise { + return runShellCommand(entry.command, { cwd: entry.cwd, env: entry.env, quiet: true }) } const VERIFY_PREFIX = 'verify:' @@ -44,13 +57,13 @@ export function loadPackageScripts(cwd: string = process.cwd()): { scripts: Reco } } -/** Collect the project's `verify:*` npm scripts as parallelisable entries, nearest package.json wins. */ +/** Collect the project's `verify:*` scripts as parallelisable entries, nearest package.json wins. */ export function resolveEntries(cwd: string = process.cwd()): VerifyEntry[] { const loaded = loadPackageScripts(cwd) if (!loaded) return [] return Object.keys(loaded.scripts) .filter((name) => name.startsWith(VERIFY_PREFIX)) - .map((name) => ({ name, command: `npm run ${name}`, cwd: loaded.dir })) + .map((name) => scriptEntry(name, loaded.dir)) } /** diff --git a/src/orchestrator/run.ts b/src/orchestrator/run.ts index a7695de..d6f80fe 100644 --- a/src/orchestrator/run.ts +++ b/src/orchestrator/run.ts @@ -1,8 +1,8 @@ import { configureMode, resolveMode } from '../shared/mode.ts' -import { runShellCommand, setVerbose } from '../shared/spawn.ts' +import { setVerbose } from '../shared/spawn.ts' import { type MeasureRecord, printMeasureTable } from './measure.ts' import { chatty, reportOutcomes } from './report.ts' -import { entryCheckName, resolveEntries, selectEntries, type VerifyEntry } from './resolveEntries.ts' +import { entryCheckName, resolveEntries, runEntryCommand, selectEntries, type VerifyEntry } from './resolveEntries.ts' import { resolveTestEntry, TEST_CHECK_NAME } from './tests.ts' export type OrchestrateOptions = { @@ -16,7 +16,7 @@ export type OrchestrateOptions = { async function runEntry(entry: VerifyEntry): Promise { const startTime = Date.now() - const code = await runShellCommand(entry.command, { cwd: entry.cwd, quiet: true }) + const code = await runEntryCommand(entry) return { script: entry.name, code, durationMs: Date.now() - startTime } } diff --git a/src/orchestrator/runAll.ts b/src/orchestrator/runAll.ts index 8cecd68..f7c036c 100644 --- a/src/orchestrator/runAll.ts +++ b/src/orchestrator/runAll.ts @@ -2,10 +2,9 @@ import { CHECKS } from '../checks/registry.ts' import { color, paintRed } from '../shared/color.ts' import { resolveMode } from '../shared/mode.ts' import { installConsoleCapture, runCaptured } from '../shared/output.ts' -import { runShellCommand } from '../shared/spawn.ts' import { type MeasureRecord, printMeasureTable } from './measure.ts' import { reportOutcomes } from './report.ts' -import { entryCheckName, resolveEntries, resolveOverride, selectEntries } from './resolveEntries.ts' +import { entryCheckName, resolveEntries, resolveOverride, runEntryCommand, selectEntries, type VerifyEntry } from './resolveEntries.ts' import { resolveTestEntry, TEST_CHECK_NAME } from './tests.ts' export type RunAllOptions = { measure?: boolean; verbose?: boolean; tests?: boolean } @@ -16,10 +15,10 @@ type Task = { name: string; note?: string; run: () => Promise } function buildTasks(opts: RunAllOptions): Task[] { const entries = resolveEntries() const mode = resolveMode() - const spawn = (name: string, command: string, cwd: string, note?: string): Task => ({ - name, + const spawn = (entry: VerifyEntry, note: string): Task => ({ + name: entry.name, note, - run: async () => (await runShellCommand(command, { cwd, quiet: true })) === 0, + run: async () => (await runEntryCommand(entry)) === 0, }) const tasks: Task[] = CHECKS.map((check) => { @@ -29,7 +28,7 @@ function buildTasks(opts: RunAllOptions): Task[] { name: check.name, note: 'overridden', run: async () => { - const ok = (await runShellCommand(override.command, { cwd: override.cwd, quiet: true })) === 0 + const ok = (await runEntryCommand(override)) === 0 if (!ok) console.error(color.dim(`↳ ${check.name}: ran \`${override.command}\` (override)`)) return ok }, @@ -40,11 +39,11 @@ function buildTasks(opts: RunAllOptions): Task[] { for (const entry of selectEntries(entries, mode)) { const check = entryCheckName(entry.name) if (builtinNames.has(check) || check === TEST_CHECK_NAME) continue - tasks.push(spawn(entry.name, entry.command, entry.cwd, 'custom')) + tasks.push(spawn(entry, 'custom')) } const testEntry = resolveTestEntry({ noTests: opts.tests === false }) - if (testEntry) tasks.push(spawn(testEntry.name, testEntry.command, testEntry.cwd, 'tests')) + if (testEntry) tasks.push(spawn(testEntry, 'tests')) return tasks } diff --git a/src/orchestrator/tests.test.ts b/src/orchestrator/tests.test.ts index 6726051..0e82f21 100644 --- a/src/orchestrator/tests.test.ts +++ b/src/orchestrator/tests.test.ts @@ -1,6 +1,10 @@ -import { describe, expect, it } from 'vitest' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' -import { resolveTestScript } from './tests.ts' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { resolveTestEntry, resolveTestScript } from './tests.ts' describe('resolveTestScript', () => { it('prefers an explicit verify:test locally, then the standard test script', () => { @@ -18,3 +22,35 @@ describe('resolveTestScript', () => { expect(resolveTestScript({}, true)).toBeNull() }) }) + +describe('resolveTestEntry', () => { + let dir: string + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'verify-tests-')) + vi.spyOn(process, 'cwd').mockReturnValue(dir) + vi.stubEnv('CI', '') + }) + afterEach(() => { + vi.restoreAllMocks() + vi.unstubAllEnvs() + fs.rmSync(dir, { recursive: true, force: true }) + }) + + it('runs the test script with npm and no env in an npm project', () => { + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { test: 'vitest' } })) + fs.writeFileSync(path.join(dir, 'package-lock.json'), '{}') + expect(resolveTestEntry()).toStrictEqual({ name: 'test', command: 'npm run test', cwd: dir }) + }) + + it('runs the test script with pnpm and its env in a pnpm project', () => { + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { test: 'vitest' } })) + fs.writeFileSync(path.join(dir, 'pnpm-lock.yaml'), '') + expect(resolveTestEntry()).toEqual({ + name: 'test', + command: 'pnpm run test', + cwd: dir, + env: { pnpm_config_verify_deps_before_run: 'false' }, + }) + }) +}) diff --git a/src/orchestrator/tests.ts b/src/orchestrator/tests.ts index 77650b4..2d04b1a 100644 --- a/src/orchestrator/tests.ts +++ b/src/orchestrator/tests.ts @@ -1,4 +1,4 @@ -import { loadPackageScripts, type VerifyEntry } from './resolveEntries.ts' +import { loadPackageScripts, scriptEntry, type VerifyEntry } from './resolveEntries.ts' /** The `verify:*` check name the tests step owns, so it isn't also run in the normal batch. */ export const TEST_CHECK_NAME = 'test' @@ -6,7 +6,7 @@ export const TEST_CHECK_NAME = 'test' export type TestsOptions = { noTests?: boolean } /** - * Which npm script the automatic tests step runs. Locally it prefers an explicit `verify:test`, falling back + * Which package.json script the automatic tests step runs. Locally it prefers an explicit `verify:test`, falling back * to the standard `test` script. On CI it runs only `test:ci` (CI usually needs a different invocation, e.g. * emitting junit.xml), so a plain `test` never runs there. Returns null when no applicable script exists. */ @@ -25,5 +25,5 @@ export function resolveTestEntry(opts: TestsOptions = {}): VerifyEntry | null { if (!loaded) return null const script = resolveTestScript(loaded.scripts, !!process.env.CI) if (!script) return null - return { name: script, command: `npm run ${script}`, cwd: loaded.dir } + return scriptEntry(script, loaded.dir) } diff --git a/src/scaffold/agentFiles.ts b/src/scaffold/agentFiles.ts index 15ab9e3..d938090 100644 --- a/src/scaffold/agentFiles.ts +++ b/src/scaffold/agentFiles.ts @@ -2,6 +2,7 @@ import fs from 'node:fs' import path from 'node:path' import { fileURLToPath } from 'node:url' +import { PACKAGE_MANAGER_COMMANDS, type PackageManager } from '../shared/packageManager.ts' import { ensurePointer, type ManagedFileResult, writeManaged } from './writeManaged.ts' export type AgentTarget = 'claude' | 'agents' @@ -10,11 +11,15 @@ const moduleDir = path.dirname(fileURLToPath(import.meta.url)) // src/scaffold/*.ts and dist/scaffold/*.mjs both sit two levels below the package root, where templates/ lives. const TEMPLATES_DIR = path.join(moduleDir, '..', '..', 'templates') -// A file already telling agents to run `npm run verify` is treated as covered — the pointer is not re-added. -const POINTER_MARKER = 'npm run verify' +const PLACEHOLDER = /\{\{(\w+)\}\}/g -function readTemplate(relativePath: string): string { - return fs.readFileSync(path.join(TEMPLATES_DIR, ...relativePath.split('/')), 'utf-8') +function renderTemplate(relativePath: string, values: Readonly>): string { + const template = fs.readFileSync(path.join(TEMPLATES_DIR, ...relativePath.split('/')), 'utf-8') + return template.replace(PLACEHOLDER, (placeholder, name: string) => { + const value = values[name] + if (value === undefined) throw new Error(`Unknown placeholder ${placeholder} in template ${relativePath}`) + return value + }) } /** @@ -24,20 +29,21 @@ function readTemplate(relativePath: string): string { * - a one-line pointer is appended to the matching instruction file (`CLAUDE.md` / `AGENTS.md`). * * Skills are CLI-owned (created/updated as a whole). The instruction files are user-owned, so the pointer is - * only appended when absent and existing content is never rewritten. + * only appended when no pointer marker is present and existing content is never rewritten. */ -export function writeAgentFiles(cwd: string, targets: readonly AgentTarget[]): ManagedFileResult[] { +export function writeAgentFiles(cwd: string, targets: readonly AgentTarget[], packageManager: PackageManager): ManagedFileResult[] { const results: ManagedFileResult[] = [] - const skill = readTemplate('skills/verify/SKILL.md') - const guidance = readTemplate('verify-guidance.md') + const { runGate, runCli, pointerMarkers } = PACKAGE_MANAGER_COMMANDS[packageManager] + const skill = renderTemplate('skills/verify/SKILL.md', { runGate, runCli }) + const guidance = renderTemplate('verify-guidance.md', { runGate, runCli }) if (targets.includes('claude')) { writeManaged(path.join(cwd, '.claude', 'skills', 'verify', 'SKILL.md'), skill, results) - ensurePointer(path.join(cwd, 'CLAUDE.md'), guidance, POINTER_MARKER, results) + ensurePointer(path.join(cwd, 'CLAUDE.md'), guidance, pointerMarkers, results) } if (targets.includes('agents')) { writeManaged(path.join(cwd, '.agent-skills', 'verify', 'SKILL.md'), skill, results) - ensurePointer(path.join(cwd, 'AGENTS.md'), guidance, POINTER_MARKER, results) + ensurePointer(path.join(cwd, 'AGENTS.md'), guidance, pointerMarkers, results) } results.sort((a, b) => a.path.localeCompare(b.path)) diff --git a/src/scaffold/init.test.ts b/src/scaffold/init.test.ts index 84cea28..fc40b11 100644 --- a/src/scaffold/init.test.ts +++ b/src/scaffold/init.test.ts @@ -23,7 +23,13 @@ function readScripts(): Record { describe('applyInit', () => { it('writes verify:* scripts, the verify skill, a CLAUDE.md pointer, and collects external devDeps', () => { - const result = applyInit({ cwd: dir, checks: ['complexity', 'unused-code'], targets: ['claude'], defaultsOnly: false }) + const result = applyInit({ + cwd: dir, + checks: ['complexity', 'unused-code'], + targets: ['claude'], + defaultsOnly: false, + packageManager: 'npm', + }) const scripts = readScripts() expect(scripts['verify:complexity']).toBe('verifyx complexity') @@ -38,7 +44,7 @@ describe('applyInit', () => { }) it('defaults-only writes no verify:* scripts, points verify at `verifyx all`, and writes the agents skill + AGENTS.md', () => { - const result = applyInit({ cwd: dir, checks: ['unused-code'], targets: ['agents'], defaultsOnly: true }) + const result = applyInit({ cwd: dir, checks: ['unused-code'], targets: ['agents'], defaultsOnly: true, packageManager: 'npm' }) expect(Object.keys(readScripts())).toEqual(['verify']) expect(readScripts().verify).toBe('verifyx all') @@ -56,6 +62,13 @@ describe('applyInit', () => { expect(result.devDeps).toEqual(expect.arrayContaining(['knip', 'jscpd', 'skott'])) }) + it("collects the selected checks' declined builds, and none when no selected check declines any", () => { + expect(applyInit({ cwd: dir, checks: ['circular-deps', 'unused-code'], targets: [], defaultsOnly: false }).declineBuilds).toEqual([ + '@parcel/watcher', + ]) + expect(applyInit({ cwd: dir, checks: ['unused-code', 'complexity'], targets: [], defaultsOnly: false }).declineBuilds).toEqual([]) + }) + it('does not clobber an existing verify:* script', () => { fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { 'verify:complexity': 'custom' } })) applyInit({ cwd: dir, checks: ['complexity'], targets: [], defaultsOnly: false }) @@ -73,4 +86,58 @@ describe('applyInit', () => { applyInit({ cwd: dir, checks: ['lint', 'format'], targets: [], defaultsOnly: false }) expect(fs.existsSync(path.join(dir, 'knip.json'))).toBe(false) }) + + it('renders the agent docs for npm exactly as before the templates took placeholders', () => { + applyInit({ cwd: dir, checks: [], targets: ['claude'], defaultsOnly: false, packageManager: 'npm' }) + expect(fs.readFileSync(path.join(dir, '.claude', 'skills', 'verify', 'SKILL.md'), 'utf-8')).toBe(NPM_SKILL) + expect(fs.readFileSync(path.join(dir, 'CLAUDE.md'), 'utf-8')).toBe(NPM_GUIDANCE) + }) +}) + +const NPM_SKILL = `--- +name: verify +description: Run this project's code verifications and fix what they report. Use after making code changes, or when asked to "verify", "run checks", or "run verify". +--- + +Run \`npm run verify\` (or \`npx verifyx\` \u2014 the CLI binary is \`verifyx\`, since \`verify\` is a Windows builtin). If it fails, fix every error and run again until it passes. Don't silence checks or game the metrics. +` + +const NPM_GUIDANCE = `## Verification + +After making code changes, run \`npm run verify\` and fix everything it reports before finishing. Don't silence checks or game the metrics. +` + +describe('applyInit with pnpm', () => { + const agentDocs = ['CLAUDE.md', 'AGENTS.md', '.claude/skills/verify/SKILL.md', '.agent-skills/verify/SKILL.md'] + const read = (file: string) => fs.readFileSync(path.join(dir, ...file.split('/')), 'utf-8') + + function expectPnpmDocs(): void { + for (const file of agentDocs) { + expect(read(file)).toContain('pnpm verify') + expect(read(file)).not.toContain('npm run verify') + expect(read(file)).not.toContain('npx verifyx') + } + expect(read('.claude/skills/verify/SKILL.md')).toContain('pnpm exec verifyx') + expect(read('.agent-skills/verify/SKILL.md')).toContain('pnpm exec verifyx') + } + + it('names pnpm commands in every agent doc when the manager is passed', () => { + applyInit({ cwd: dir, checks: [], targets: ['claude', 'agents'], defaultsOnly: false, packageManager: 'pnpm' }) + expectPnpmDocs() + }) + + it('names pnpm commands in every agent doc when pnpm is detected from a lockfile', () => { + fs.writeFileSync(path.join(dir, 'pnpm-lock.yaml'), "lockfileVersion: '9.0'\n") + applyInit({ cwd: dir, checks: [], targets: ['claude', 'agents'], defaultsOnly: false }) + expectPnpmDocs() + }) + + it('leaves a CLAUDE.md that already says `pnpm run verify` unchanged', () => { + fs.writeFileSync(path.join(dir, 'pnpm-lock.yaml'), "lockfileVersion: '9.0'\n") + const original = '# Project\n\nRun `pnpm run verify` before finishing.\n' + fs.writeFileSync(path.join(dir, 'CLAUDE.md'), original) + const result = applyInit({ cwd: dir, checks: [], targets: ['claude'], defaultsOnly: false }) + expect(result.agentFiles.find((file) => file.path === path.join(dir, 'CLAUDE.md'))?.action).toBe('unchanged') + expect(read('CLAUDE.md')).toBe(original) + }) }) diff --git a/src/scaffold/init.ts b/src/scaffold/init.ts index 53695f0..728d304 100644 --- a/src/scaffold/init.ts +++ b/src/scaffold/init.ts @@ -2,6 +2,7 @@ import path from 'node:path' import { getCheck } from '../checks/registry.ts' import type { Check } from '../checks/types.ts' +import { detectPackageManager, type PackageManager } from '../shared/packageManager.ts' import { type AgentTarget, writeAgentFiles } from './agentFiles.ts' import { detectSystemBinaries } from './detectSystemBinaries.ts' import { ensureKnipIgnores } from './knipConfig.ts' @@ -15,31 +16,37 @@ export type InitOptions = { targets: readonly AgentTarget[] /** When true, do not write `verify:*` scripts — rely on `verify`'s built-in defaults. */ defaultsOnly: boolean + /** The manager the agent docs name. Defaults to the one detected from `cwd`. */ + packageManager?: PackageManager } export type InitResult = { addedScripts: string[] /** devDependencies the selected external checks need (deduped). */ devDeps: string[] + /** Dependency builds the selected checks decline (deduped). */ + declineBuilds: string[] agentFiles: ManagedFileResult[] } /** Pure scaffolding step: write package.json scripts + agent files, and report the devDeps to install. */ export function applyInit(opts: InitOptions): InitResult { const devDeps: string[] = [] + const declineBuilds: string[] = [] const scripts: Record = {} for (const name of opts.checks) { const check = getCheck(name) if (!check) continue if (check.scaffold.devDeps) devDeps.push(...check.scaffold.devDeps) + if (check.scaffold.declineBuilds) declineBuilds.push(...check.scaffold.declineBuilds) if (!opts.defaultsOnly) scripts[`verify:${name}`] = check.scaffold.script } // Defaults-only wires the top `verify` script to `verifyx all` so it runs every built-in with no verify:* list. const addedScripts = addVerifyScripts(path.join(opts.cwd, 'package.json'), scripts, opts.defaultsOnly ? 'verifyx all' : 'verifyx') - const agentFiles = writeAgentFiles(opts.cwd, opts.targets) + const agentFiles = writeAgentFiles(opts.cwd, opts.targets, opts.packageManager ?? detectPackageManager(opts.cwd)) // context: with unused-code selected, teach knip to ignore the other external tools verifyx runs at runtime. if (opts.checks.includes('unused-code')) { @@ -51,5 +58,5 @@ export function applyInit(opts: InitOptions): InitResult { ensureKnipIgnores(opts.cwd, [...new Set(toolDeps)], agentFiles, detectSystemBinaries(opts.cwd)) } - return { addedScripts, devDeps: [...new Set(devDeps)], agentFiles } + return { addedScripts, devDeps: [...new Set(devDeps)], declineBuilds: [...new Set(declineBuilds)], agentFiles } } diff --git a/src/scaffold/installDeps.test.ts b/src/scaffold/installDeps.test.ts index 0f82646..f90ea74 100644 --- a/src/scaffold/installDeps.test.ts +++ b/src/scaffold/installDeps.test.ts @@ -16,23 +16,24 @@ afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }) }) -/** A runner that records the commands it saw and returns 0 unless the command mentions a `fail`-listed package. */ -function fakeRunner(failFor: string[] = []): Runner & { commands: string[] } { - const commands: string[] = [] - const run: Runner = (command) => { - commands.push(command) - return Promise.resolve(failFor.some((pkg) => command.includes(pkg)) ? 1 : 0) +function fakeRunner(failFor: string[] = []): Runner & { commands: string[][] } { + const commands: string[][] = [] + const run: Runner = (argv) => { + commands.push([...argv]) + return Promise.resolve(failFor.some((pkg) => argv.includes(pkg)) ? 1 : 0) } return Object.assign(run, { commands }) } -describe('installDevDeps', () => { +describe('installDevDeps (npm)', () => { + const npm = (run: Runner) => ({ packageManager: 'npm' as const, run }) + it('installs missing deps in a single batch on the happy path', async () => { const run = fakeRunner() - const report = await installDevDeps(['oxlint', 'knip'], dir, run) + const report = await installDevDeps(['oxlint', 'knip'], dir, npm(run)) expect(report).toEqual({ skipped: [], installed: ['oxlint', 'knip'], failed: [] }) - expect(run.commands).toEqual(['npm install --save-dev oxlint knip']) + expect(run.commands).toEqual([['npm', 'install', '--save-dev', 'oxlint', 'knip']]) }) it('skips deps already declared in package.json (any dependency field)', async () => { @@ -41,18 +42,18 @@ describe('installDevDeps', () => { JSON.stringify({ devDependencies: { typescript: '^5.0.0' }, peerDependencies: { oxlint: '*' } }), ) const run = fakeRunner() - const report = await installDevDeps(['typescript', 'oxlint', 'knip'], dir, run) + const report = await installDevDeps(['typescript', 'oxlint', 'knip'], dir, npm(run)) expect(report.skipped).toEqual(['typescript', 'oxlint']) expect(report.installed).toEqual(['knip']) - expect(run.commands).toEqual(['npm install --save-dev knip']) + expect(run.commands).toEqual([['npm', 'install', '--save-dev', 'knip']]) }) it('skips deps present in node_modules even when not declared', async () => { fs.mkdirSync(path.join(dir, 'node_modules', 'oxlint'), { recursive: true }) fs.writeFileSync(path.join(dir, 'node_modules', 'oxlint', 'package.json'), '{}') const run = fakeRunner() - const report = await installDevDeps(['oxlint', 'knip'], dir, run) + const report = await installDevDeps(['oxlint', 'knip'], dir, npm(run)) expect(report.skipped).toEqual(['oxlint']) expect(report.installed).toEqual(['knip']) @@ -61,7 +62,7 @@ describe('installDevDeps', () => { it('runs no install command when everything is already present', async () => { fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ devDependencies: { oxlint: '*', knip: '*' } })) const run = fakeRunner() - const report = await installDevDeps(['oxlint', 'knip'], dir, run) + const report = await installDevDeps(['oxlint', 'knip'], dir, npm(run)) expect(report).toEqual({ skipped: ['oxlint', 'knip'], installed: [], failed: [] }) expect(run.commands).toEqual([]) @@ -69,17 +70,95 @@ describe('installDevDeps', () => { it('falls back to per-package installs and isolates the failure when the batch fails', async () => { const run = fakeRunner(['oxlint']) - const report = await installDevDeps(['oxlint', 'knip', 'skott'], dir, run) + const report = await installDevDeps(['oxlint', 'knip', 'skott'], dir, npm(run)) - expect(report.skipped).toEqual([]) - expect(report.installed).toEqual(['knip', 'skott']) - expect(report.failed).toEqual(['oxlint']) + expect(report).toEqual({ skipped: [], installed: ['knip', 'skott'], failed: ['oxlint'] }) // batch first, then one command per package to isolate the culprit expect(run.commands).toEqual([ - 'npm install --save-dev oxlint knip skott', - 'npm install --save-dev oxlint', - 'npm install --save-dev knip', - 'npm install --save-dev skott', + ['npm', 'install', '--save-dev', 'oxlint', 'knip', 'skott'], + ['npm', 'install', '--save-dev', 'oxlint'], + ['npm', 'install', '--save-dev', 'knip'], + ['npm', 'install', '--save-dev', 'skott'], ]) }) + + it('passes no decline flags to npm', async () => { + const run = fakeRunner() + await installDevDeps(['skott'], dir, { ...npm(run), declineBuilds: ['@parcel/watcher'] }) + + expect(run.commands).toEqual([['npm', 'install', '--save-dev', 'skott']]) + }) +}) + +describe('installDevDeps (pnpm)', () => { + const ADD = ['pnpm', 'add', '-D', '--ignore-workspace-root-check'] + const DECLINE = '--allow-build=!@parcel/watcher' + + const pnpm = (run: Runner, cwd = dir) => + installDevDeps(['skott', 'knip'], cwd, { packageManager: 'pnpm', declineBuilds: ['@parcel/watcher'], run }) + + const writeWorkspace = (yaml: string, at = dir) => fs.writeFileSync(path.join(at, 'pnpm-workspace.yaml'), yaml) + + const argvFor = async (cwd = dir) => { + const run = fakeRunner() + await pnpm(run, cwd) + expect(run.commands).toHaveLength(1) + return run.commands[0] + } + + it('runs one add with the workspace-root flag and the decline flag', async () => { + const run = fakeRunner() + const report = await pnpm(run) + + expect(report).toEqual({ skipped: [], installed: ['skott', 'knip'], failed: [] }) + expect(run.commands).toEqual([[...ADD, DECLINE, 'skott', 'knip']]) + }) + + it('does not retry per package after a failed batch', async () => { + const run = fakeRunner(['skott']) + const report = await pnpm(run) + + expect(run.commands).toHaveLength(1) + expect(report).toEqual({ skipped: [], installed: [], failed: ['skott', 'knip'] }) + }) + + it('declines the build when there is no pnpm-workspace.yaml', async () => { + expect(await argvFor()).toContain(DECLINE) + }) + + it.each([ + ['@parcel/watcher', true], + ['@parcel/watcher', false], + ['@parcel/watcher@2.6.0', true], + ])('leaves the decision alone when allowBuilds[%j] is %j', async (key, value) => { + writeWorkspace(`allowBuilds:\n '${key}': ${value}\n`) + expect(await argvFor()).toEqual([...ADD, 'skott', 'knip']) + }) + + it('leaves the decision alone when one version is approved and another declined', async () => { + writeWorkspace("allowBuilds:\n '@parcel/watcher@2.4.1': false\n '@parcel/watcher@2.6.0': true\n") + expect(await argvFor()).toEqual([...ADD, 'skott', 'knip']) + }) + + it.each([ + ['only in onlyBuiltDependencies', "onlyBuiltDependencies:\n - '@parcel/watcher'\n"], + ['only in ignoredBuiltDependencies', "ignoredBuiltDependencies:\n - '@parcel/watcher'\n"], + ['a placeholder in allowBuilds', "allowBuilds:\n '@parcel/watcher': set this to true or false\n"], + ['a version-keyed entry with no version', "allowBuilds:\n '@parcel/watcher@': true\n"], + ['declined only for another version', "allowBuilds:\n '@parcel/watcher@2.4.1': false\n"], + ['an entry for a different package', "allowBuilds:\n '@parcel/watcher-wasm': true\n"], + ['an unparseable file', 'allowBuilds: [\n'], + ])('declines the build when the package is %s', async (_, yaml) => { + writeWorkspace(yaml) + expect(await argvFor()).toContain(DECLINE) + }) + + it('finds the workspace file from a sub-directory', async () => { + writeWorkspace("packages:\n - 'api'\nallowBuilds:\n '@parcel/watcher': false\n") + const api = path.join(dir, 'api') + fs.mkdirSync(api) + fs.writeFileSync(path.join(api, 'package.json'), JSON.stringify({ name: 'api' })) + + expect(await argvFor(api)).toEqual([...ADD, 'skott', 'knip']) + }) }) diff --git a/src/scaffold/installDeps.ts b/src/scaffold/installDeps.ts index 9a53a1a..6c6e772 100644 --- a/src/scaffold/installDeps.ts +++ b/src/scaffold/installDeps.ts @@ -1,20 +1,32 @@ import fs from 'node:fs' import path from 'node:path' -import { runShellCommand } from '../shared/spawn.ts' +import { parse } from 'yaml' -/** Signature of the command runner (defaults to `runShellCommand`); injectable so tests don't shell out. */ -export type Runner = (command: string, opts: { cwd: string }) => Promise +import { detectPackageManager, PACKAGE_MANAGER_COMMANDS, type PackageManager } from '../shared/packageManager.ts' +import { runArgvCommand } from '../shared/spawn.ts' + +export type Runner = (argv: readonly string[], opts: { cwd: string }) => Promise export type InstallReport = { /** Already declared in package.json or present in node_modules — left untouched (no version bump). */ skipped: string[] /** Newly installed successfully. */ installed: string[] - /** Attempted but npm exited non-zero (e.g. a peer-dependency conflict) — surfaced for the user to resolve. */ + /** Attempted but the install exited non-zero (e.g. a peer-dependency conflict); surfaced for the user to resolve. */ failed: string[] } +export type InstallOptions = { + /** Defaults to the manager detected from `cwd`. */ + packageManager?: PackageManager + /** Packages whose build scripts pnpm should record as declined (npm ignores them). */ + declineBuilds?: readonly string[] + run?: Runner +} + +type Outcome = Pick + const DEP_FIELDS = ['dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies'] as const type PackageJson = Partial>> @@ -36,12 +48,63 @@ function isInstalled(name: string, cwd: string, declared: Set): boolean return declared.has(name) || fs.existsSync(path.join(cwd, 'node_modules', name, 'package.json')) } +function nearestAllowBuilds(cwd: string): Record { + for (let dir = path.resolve(cwd); ; dir = path.dirname(dir)) { + const file = path.join(dir, 'pnpm-workspace.yaml') + if (fs.existsSync(file)) { + try { + const allowBuilds: unknown = (parse(fs.readFileSync(file, 'utf-8')) as { allowBuilds?: unknown } | null)?.allowBuilds + return allowBuilds && typeof allowBuilds === 'object' && !Array.isArray(allowBuilds) ? (allowBuilds as Record) : {} + } catch { + return {} + } + } + if (path.dirname(dir) === dir) return {} + } +} + +/** True when `key` is `pkg@` (a scoped name's own leading `@` is part of `pkg`). */ +function isVersionKeyFor(key: string, pkg: string): boolean { + return key.startsWith(`${pkg}@`) && key.length > pkg.length + 1 +} + +/** + * A version-keyed `true` in `allowBuilds` counts as decided, since the decline flag would revoke it; a version-keyed + * `false` does not, as it covers one version. A non-boolean value is not a decision. pnpm 11+ ignores + * `onlyBuiltDependencies` and `ignoredBuiltDependencies`. + */ +function undecidedBuilds(packages: readonly string[], cwd: string): string[] { + const entries = Object.entries(nearestAllowBuilds(cwd)) + return packages.filter( + (pkg) => !entries.some(([key, value]) => (key === pkg && typeof value === 'boolean') || (value === true && isVersionKeyFor(key, pkg))), + ) +} + +/** npm: one batch, then per-package installs on failure (the batch is atomic) so one bad package can't block the rest. */ +async function installWithNpm(toInstall: string[], cwd: string, run: Runner): Promise { + const { addDevDependencies } = PACKAGE_MANAGER_COMMANDS.npm + if ((await run(addDevDependencies(toInstall), { cwd })) === 0) return { installed: toInstall, failed: [] } + + const installed: string[] = [] + const failed: string[] = [] + for (const dep of toInstall) { + ;((await run(addDevDependencies([dep]), { cwd })) === 0 ? installed : failed).push(dep) + } + return { installed, failed } +} + /** - * Install the given devDeps, skipping any already present. Tries one batch install first (fast path); if that - * fails — typically a single peer-dependency conflict — falls back to per-package installs so one bad package - * can't block the rest, and the exact failures are reported instead of aborting `init`. + * pnpm: one batch only. A failed batch has usually written every dep already (e.g. it stopped on an unapproved build), + * and every later `pnpm add` fails the same way, so a per-package retry would only repeat the failure. */ -export async function installDevDeps(devDeps: readonly string[], cwd: string, run: Runner = runShellCommand): Promise { +async function installWithPnpm(toInstall: string[], cwd: string, declineBuilds: readonly string[], run: Runner): Promise { + const argv = PACKAGE_MANAGER_COMMANDS.pnpm.addDevDependencies(toInstall, undecidedBuilds(declineBuilds, cwd)) + return (await run(argv, { cwd })) === 0 ? { installed: toInstall, failed: [] } : { installed: [], failed: toInstall } +} + +/** Install the devDeps not already present. Failures are reported instead of aborting `init`. */ +export async function installDevDeps(devDeps: readonly string[], cwd: string, options: InstallOptions = {}): Promise { + const { packageManager = detectPackageManager(cwd), declineBuilds = [], run = runArgvCommand } = options const declared = declaredDeps(cwd) const skipped: string[] = [] const toInstall: string[] = [] @@ -49,16 +112,7 @@ export async function installDevDeps(devDeps: readonly string[], cwd: string, ru if (toInstall.length === 0) return { skipped, installed: [], failed: [] } - // Fast path: one install for everything missing. - if ((await run(`npm install --save-dev ${toInstall.join(' ')}`, { cwd })) === 0) { - return { skipped, installed: toInstall, failed: [] } - } - - // The batch is atomic, so nothing was written — retry each package alone to isolate the culprit(s). - const installed: string[] = [] - const failed: string[] = [] - for (const dep of toInstall) { - ;((await run(`npm install --save-dev ${dep}`, { cwd })) === 0 ? installed : failed).push(dep) - } - return { skipped, installed, failed } + const outcome = + packageManager === 'pnpm' ? await installWithPnpm(toInstall, cwd, declineBuilds, run) : await installWithNpm(toInstall, cwd, run) + return { skipped, ...outcome } } diff --git a/src/scaffold/pnpmVersion.test.ts b/src/scaffold/pnpmVersion.test.ts new file mode 100644 index 0000000..f41b388 --- /dev/null +++ b/src/scaffold/pnpmVersion.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from 'vitest' + +import { checkPnpmVersion } from './pnpmVersion.ts' + +const version = (stdout: string) => checkPnpmVersion({ code: 0, stdout, stderr: '' }) + +describe('checkPnpmVersion', () => { + it.each(['10.34.7', '11.21.0', '11.25.9', '12.3.4'])('refuses %s with the found version, the minimum and the pin advice', (v) => { + const check = version(`${v}\n`) + expect(check.ok).toBe(false) + if (check.ok) return + expect(check.message).toContain(`found pnpm ${v}`) + expect(check.message).toContain('pnpm 11.26+ or 12.4+') + expect(check.message).toContain('`packageManager` or `devEngines` pin') + }) + + it.each(['11.26.0', '11.28.6', '12.4.0', '12.11.2', '13.0.0'])('accepts %s', (v) => { + expect(version(`${v}\n`)).toEqual({ ok: true }) + }) + + it('parses a version with a pre-release or build suffix', () => { + expect(version('12.4.0-rc.1\n')).toEqual({ ok: true }) + expect(version('11.26.0+build.5')).toEqual({ ok: true }) + expect(version('12.3.0-rc.1').ok).toBe(false) + }) + + it('refuses unparseable output and shows it', () => { + const check = version('Downloading pnpm\n') + expect(check.ok).toBe(false) + if (!check.ok) expect(check.message).toContain('Downloading pnpm') + }) + + it('reports a failed `pnpm --version` with its exit code and stderr', () => { + const check = checkPnpmVersion({ code: 1, stdout: '', stderr: 'ERR_PNPM_BAD_PM_VERSION This project is configured to use pnpm@^9\n' }) + expect(check.ok).toBe(false) + if (check.ok) return + expect(check.message).toContain('exited 1') + expect(check.message).toContain('ERR_PNPM_BAD_PM_VERSION') + }) +}) diff --git a/src/scaffold/pnpmVersion.ts b/src/scaffold/pnpmVersion.ts new file mode 100644 index 0000000..1a61267 --- /dev/null +++ b/src/scaffold/pnpmVersion.ts @@ -0,0 +1,30 @@ +/** The outcome of `pnpm --version`, as `captureArgvCommand` returns it. */ +type VersionOutput = { code: number; stdout: string; stderr: string } + +export type PnpmVersionCheck = { ok: true } | { ok: false; message: string } + +const MINIMUM = 'pnpm 11.26+ or 12.4+' +const PIN_ADVICE = 'A `packageManager` or `devEngines` pin in package.json decides which pnpm runs, so bump the pin if there is one.' + +function isSupported(major: number, minor: number): boolean { + if (major === 11) return minor >= 26 + if (major === 12) return minor >= 4 + return major >= 13 +} + +/** `verifyx init` needs a pnpm where `--allow-build=!` works. A pre-release or build suffix is ignored. */ +export function checkPnpmVersion({ code, stdout, stderr }: VersionOutput): PnpmVersionCheck { + if (code !== 0) { + return { ok: false, message: `verifyx init needs ${MINIMUM}, but \`pnpm --version\` exited ${code}:\n${stderr.trim()}` } + } + const raw = stdout.trim() + const match = /^(\d+)\.(\d+)\.\d+(?:[-+][0-9A-Za-z.+-]*)?$/.exec(raw) + if (!match) { + return { + ok: false, + message: `verifyx init needs ${MINIMUM}, but could not read a version from \`pnpm --version\`: ${JSON.stringify(raw)}`, + } + } + if (isSupported(Number(match[1]), Number(match[2]))) return { ok: true } + return { ok: false, message: `verifyx init needs ${MINIMUM}; found pnpm ${raw}. ${PIN_ADVICE}` } +} diff --git a/src/scaffold/writeManaged.test.ts b/src/scaffold/writeManaged.test.ts index 028f4c4..4157e8d 100644 --- a/src/scaffold/writeManaged.test.ts +++ b/src/scaffold/writeManaged.test.ts @@ -64,7 +64,7 @@ describe('ensurePointer', () => { it('creates the file with the block when missing', () => { const file = path.join(dir, 'CLAUDE.md') const results: ManagedFileResult[] = [] - ensurePointer(file, block, '/verify', results) + ensurePointer(file, block, ['/verify'], results) expect(results[0]?.action).toBe('created') expect(fs.readFileSync(file, 'utf-8')).toBe(block) }) @@ -74,7 +74,17 @@ describe('ensurePointer', () => { const original = '# My project\n\nSome guidance mentioning /verify already.\n' fs.writeFileSync(file, original) const results: ManagedFileResult[] = [] - ensurePointer(file, block, '/verify', results) + ensurePointer(file, block, ['/verify'], results) + expect(results[0]?.action).toBe('unchanged') + expect(fs.readFileSync(file, 'utf-8')).toBe(original) + }) + + it('leaves the file untouched when any one of several markers is present', () => { + const file = path.join(dir, 'CLAUDE.md') + const original = '# My project\n\nRun `pnpm run verify` before finishing.\n' + fs.writeFileSync(file, original) + const results: ManagedFileResult[] = [] + ensurePointer(file, block, ['pnpm verify', 'pnpm run verify'], results) expect(results[0]?.action).toBe('unchanged') expect(fs.readFileSync(file, 'utf-8')).toBe(original) }) @@ -84,7 +94,7 @@ describe('ensurePointer', () => { const original = '# My project\n\nExisting instructions.\n' fs.writeFileSync(file, original) const results: ManagedFileResult[] = [] - ensurePointer(file, block, '/verify', results) + ensurePointer(file, block, ['/verify'], results) expect(results[0]?.action).toBe('appended') const after = fs.readFileSync(file, 'utf-8') expect(after.startsWith(original)).toBe(true) @@ -94,10 +104,10 @@ describe('ensurePointer', () => { it('is idempotent — a second call after appending is a no-op', () => { const file = path.join(dir, 'CLAUDE.md') fs.writeFileSync(file, '# My project\n') - ensurePointer(file, block, '/verify', []) + ensurePointer(file, block, ['/verify'], []) const afterFirst = fs.readFileSync(file, 'utf-8') const results: ManagedFileResult[] = [] - ensurePointer(file, block, '/verify', results) + ensurePointer(file, block, ['/verify'], results) expect(results[0]?.action).toBe('unchanged') expect(fs.readFileSync(file, 'utf-8')).toBe(afterFirst) }) diff --git a/src/scaffold/writeManaged.ts b/src/scaffold/writeManaged.ts index 222fcaf..4a8b90c 100644 --- a/src/scaffold/writeManaged.ts +++ b/src/scaffold/writeManaged.ts @@ -48,13 +48,13 @@ export function writeManaged(file: string, contents: string, results: ManagedFil /** * Ensure a user-owned file (CLAUDE.md / AGENTS.md) contains a pointer block. Creates the file with the block - * when missing; appends the block when the file exists but does not already contain `marker`; otherwise leaves - * the file untouched. Never rewrites existing content. + * when missing; appends the block when the file exists but contains none of `markers`; otherwise leaves the file + * untouched. Never rewrites existing content. */ -export function ensurePointer(file: string, block: string, marker: string, results: ManagedFileResult[]): void { +export function ensurePointer(file: string, block: string, markers: readonly string[], results: ManagedFileResult[]): void { const existing = readIfExists(file) if (existing === null) return createFile(file, block, results) - if (existing.includes(marker)) { + if (markers.some((marker) => existing.includes(marker))) { results.push({ path: file, action: 'unchanged' }) return } diff --git a/src/shared/packageManager.test.ts b/src/shared/packageManager.test.ts new file mode 100644 index 0000000..d2d075a --- /dev/null +++ b/src/shared/packageManager.test.ts @@ -0,0 +1,139 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { detectPackageManager, PACKAGE_MANAGER_COMMANDS } from './packageManager.ts' + +// Detection is memoised per start directory, so every test gets its own fresh temp dir. +let dir: string +beforeEach(() => { + dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'verifyx-pm-'))) +}) +afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }) +}) + +const write = (files: Record) => { + for (const [rel, content] of Object.entries(files)) { + const file = path.join(dir, rel) + fs.mkdirSync(path.dirname(file), { recursive: true }) + fs.writeFileSync(file, typeof content === 'string' ? content : JSON.stringify(content)) + } +} + +describe('detectPackageManager', () => { + it('reads a devEngines.packageManager object', () => { + write({ 'package.json': { devEngines: { packageManager: { name: 'pnpm', version: '^11' } } } }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('uses the first entry of a devEngines.packageManager array', () => { + write({ 'package.json': { devEngines: { packageManager: [{ name: 'pnpm' }, { name: 'npm' }] } } }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('reads packageManager with a version', () => { + write({ 'package.json': { packageManager: 'pnpm@11.21.0+sha512.abc' } }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('reads packageManager without a version', () => { + write({ 'package.json': { packageManager: 'pnpm' } }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('treats a scoped-looking packageManager as an unsupported name', () => { + write({ 'package.json': { packageManager: '@scope/pnpm@1.0.0' }, 'pnpm-lock.yaml': '' }) + expect(detectPackageManager(dir)).toBe('npm') + }) + + it('lets a field beat a lockfile in the same directory', () => { + write({ 'package.json': { packageManager: 'npm@10.0.0' }, 'pnpm-lock.yaml': '' }) + expect(detectPackageManager(dir)).toBe('npm') + }) + + it('lets devEngines beat packageManager', () => { + write({ 'package.json': { devEngines: { packageManager: { name: 'pnpm' } }, packageManager: 'npm@10.0.0' } }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('picks pnpm when both pnpm and npm lockfiles are present', () => { + write({ 'pnpm-lock.yaml': '', 'package-lock.json': '{}' }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('treats pnpm-workspace.yaml and npm-shrinkwrap.json as lockfile signals', () => { + write({ 'a/pnpm-workspace.yaml': '', 'b/npm-shrinkwrap.json': '{}' }) + expect(detectPackageManager(path.join(dir, 'a'))).toBe('pnpm') + expect(detectPackageManager(path.join(dir, 'b'))).toBe('npm') + }) + + it('walks up from a sub-package to the workspace root', () => { + write({ + 'package.json': { name: 'root', packageManager: 'pnpm@11.21.0' }, + 'pnpm-lock.yaml': '', + 'pnpm-workspace.yaml': 'packages:\n - api\n', + 'api/package.json': { name: 'api' }, + }) + expect(detectPackageManager(path.join(dir, 'api'))).toBe('pnpm') + }) + + it('lets the nearest directory win over a farther one', () => { + write({ 'pnpm-lock.yaml': '', 'sub/package-lock.json': '{}' }) + expect(detectPackageManager(path.join(dir, 'sub'))).toBe('npm') + }) + + it('stops the walk at a field naming another manager and resolves to npm', () => { + write({ 'pnpm-lock.yaml': '', 'sub/package.json': { packageManager: 'other@1.0.0' } }) + expect(detectPackageManager(path.join(dir, 'sub'))).toBe('npm') + }) + + it('lets a field naming another manager beat a pnpm lockfile in the same directory', () => { + write({ 'package.json': { devEngines: { packageManager: { name: 'other' } } }, 'pnpm-lock.yaml': '' }) + expect(detectPackageManager(dir)).toBe('npm') + }) + + it('ignores an invalid package.json but still reads its lockfiles', () => { + write({ 'package.json': '{ not json', 'pnpm-lock.yaml': '' }) + expect(detectPackageManager(dir)).toBe('pnpm') + }) + + it('resolves to npm when no directory has a signal', () => { + write({ 'package.json': { name: 'plain' } }) + expect(detectPackageManager(dir)).toBe('npm') + }) +}) + +describe('PACKAGE_MANAGER_COMMANDS', () => { + const { npm, pnpm } = PACKAGE_MANAGER_COMMANDS + + it('builds npm commands', () => { + expect(npm.runScript('verify:lint')).toEqual({ command: 'npm run verify:lint' }) + expect(npm.addDevDependencies(['oxlint', 'knip'], ['@parcel/watcher'])).toEqual(['npm', 'install', '--save-dev', 'oxlint', 'knip']) + expect(npm.runGate).toBe('npm run verify') + expect(npm.runCli).toBe('npx verifyx') + expect(npm.pointerMarkers).toEqual(['npm run verify']) + }) + + it('builds pnpm commands', () => { + expect(pnpm.runScript('verify:lint')).toEqual({ + command: 'pnpm run verify:lint', + env: { pnpm_config_verify_deps_before_run: 'false' }, + }) + expect(pnpm.addDevDependencies(['oxlint', 'skott'], ['@parcel/watcher'])).toEqual([ + 'pnpm', + 'add', + '-D', + '--ignore-workspace-root-check', + '--allow-build=!@parcel/watcher', + 'oxlint', + 'skott', + ]) + expect(pnpm.addDevDependencies(['oxlint'])).toEqual(['pnpm', 'add', '-D', '--ignore-workspace-root-check', 'oxlint']) + expect(pnpm.runGate).toBe('pnpm verify') + expect(pnpm.runCli).toBe('pnpm exec verifyx') + expect(pnpm.pointerMarkers).toEqual(['pnpm verify', 'pnpm run verify']) + }) +}) diff --git a/src/shared/packageManager.ts b/src/shared/packageManager.ts new file mode 100644 index 0000000..b9fb57a --- /dev/null +++ b/src/shared/packageManager.ts @@ -0,0 +1,121 @@ +import fs from 'node:fs' +import path from 'node:path' + +export type PackageManager = 'npm' | 'pnpm' + +type ScriptCommand = { command: string; env?: Record } + +export type PackageManagerCommands = { + runScript: (script: string) => ScriptCommand + /** `declineBuilds` lists packages whose build scripts must not run; npm ignores it. */ + addDevDependencies: (deps: readonly string[], declineBuilds?: readonly string[]) => string[] + /** Doc string: run the verify gate. */ + runGate: string + /** Doc string: run the verifyx CLI. */ + runCli: string + /** Strings whose presence in an agent file means it already points to verify. */ + pointerMarkers: readonly string[] +} + +export const PACKAGE_MANAGER_COMMANDS: Record = { + npm: { + runScript: (script) => ({ command: `npm run ${script}` }), + addDevDependencies: (deps) => ['npm', 'install', '--save-dev', ...deps], + runGate: 'npm run verify', + runCli: 'npx verifyx', + pointerMarkers: ['npm run verify'], + }, + pnpm: { + // Children of a parallel run must not check (and maybe install) dependencies; the outer `pnpm verify` already did. + runScript: (script) => ({ command: `pnpm run ${script}`, env: { pnpm_config_verify_deps_before_run: 'false' } }), + addDevDependencies: (deps, declineBuilds = []) => [ + 'pnpm', + 'add', + '-D', + '--ignore-workspace-root-check', + ...declineBuilds.map((pkg) => `--allow-build=!${pkg}`), + ...deps, + ], + runGate: 'pnpm verify', + runCli: 'pnpm exec verifyx', + pointerMarkers: ['pnpm verify', 'pnpm run verify'], + }, +} + +type Signal = PackageManager | 'unsupported' | undefined + +const PNPM_FILES = ['pnpm-lock.yaml', 'pnpm-workspace.yaml'] +const NPM_FILES = ['package-lock.json', 'npm-shrinkwrap.json'] + +const cache = new Map() + +function managerFromName(name: unknown): Signal { + if (typeof name !== 'string' || name === '') return undefined + return name === 'npm' || name === 'pnpm' ? name : 'unsupported' +} + +function packageManagerName(value: string): string { + const at = value.lastIndexOf('@') + return at > 0 ? value.slice(0, at) : value +} + +function readPackageJson(dir: string): Record | undefined { + try { + const parsed: unknown = JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf-8')) + return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? (parsed as Record) : undefined + } catch { + return undefined + } +} + +function fieldSignal(dir: string): Signal { + const pkg = readPackageJson(dir) + if (!pkg) return undefined + + const devEngines = pkg.devEngines + if (devEngines && typeof devEngines === 'object') { + const entry = (devEngines as { packageManager?: unknown }).packageManager + const first: unknown = Array.isArray(entry) ? entry[0] : entry + if (first && typeof first === 'object') { + const signal = managerFromName((first as { name?: unknown }).name) + if (signal) return signal + } + } + + const packageManager = pkg.packageManager + if (typeof packageManager === 'string') return managerFromName(packageManagerName(packageManager)) + return undefined +} + +function lockfileSignal(dir: string): Signal { + const has = (files: string[]) => files.some((file) => fs.existsSync(path.join(dir, file))) + if (has(PNPM_FILES)) return 'pnpm' + if (has(NPM_FILES)) return 'npm' + return undefined +} + +/** + * The first directory at or above `startDir` with a signal decides, defaulting to npm. Memoised per resolved start + * directory for the process lifetime. + */ +export function detectPackageManager(startDir: string): PackageManager { + const start = path.resolve(startDir) + const cached = cache.get(start) + if (cached) return cached + + let result: PackageManager = 'npm' + let dir = start + for (;;) { + const signal = fieldSignal(dir) ?? lockfileSignal(dir) + if (signal) { + result = signal === 'unsupported' ? 'npm' : signal + break + } + const parent = path.dirname(dir) + if (parent === dir) break + dir = parent + } + + cache.set(start, result) + return result +} diff --git a/templates/skills/verify/SKILL.md b/templates/skills/verify/SKILL.md index 3d5e0f7..1153f7f 100644 --- a/templates/skills/verify/SKILL.md +++ b/templates/skills/verify/SKILL.md @@ -3,4 +3,4 @@ name: verify description: Run this project's code verifications and fix what they report. Use after making code changes, or when asked to "verify", "run checks", or "run verify". --- -Run `npm run verify` (or `npx verifyx` — the CLI binary is `verifyx`, since `verify` is a Windows builtin). If it fails, fix every error and run again until it passes. Don't silence checks or game the metrics. +Run `{{runGate}}` (or `{{runCli}}` — the CLI binary is `verifyx`, since `verify` is a Windows builtin). If it fails, fix every error and run again until it passes. Don't silence checks or game the metrics. diff --git a/templates/verify-guidance.md b/templates/verify-guidance.md index e73171a..945b51c 100644 --- a/templates/verify-guidance.md +++ b/templates/verify-guidance.md @@ -1,3 +1,3 @@ ## Verification -After making code changes, run `npm run verify` and fix everything it reports before finishing. Don't silence checks or game the metrics. +After making code changes, run `{{runGate}}` and fix everything it reports before finishing. Don't silence checks or game the metrics. From 0b3bd86759b0df6fb47338c340343b0bee199290 Mon Sep 17 00:00:00 2001 From: Wade Baglin Date: Thu, 22 Oct 2026 09:53:36 +1000 Subject: [PATCH 2/2] refactor: rename declineBuilds to pnpmDenyBuilds and explain it The field only affects pnpm, and pnpm's own term for blocking a build script is "deny". Rewrite its comment around the skott example so it says why init denies @parcel/watcher's build on pnpm 11+. --- src/checks/external.ts | 6 +++--- src/checks/registry.ts | 2 +- src/checks/types.ts | 11 +++++++++-- src/commands/registerInit.ts | 2 +- src/scaffold/init.test.ts | 6 +++--- src/scaffold/init.ts | 10 +++++----- src/scaffold/installDeps.test.ts | 24 ++++++++++++------------ src/scaffold/installDeps.ts | 14 +++++++------- src/shared/packageManager.ts | 8 ++++---- 9 files changed, 45 insertions(+), 38 deletions(-) diff --git a/src/checks/external.ts b/src/checks/external.ts index d4fb272..2847909 100644 --- a/src/checks/external.ts +++ b/src/checks/external.ts @@ -38,8 +38,8 @@ export type ExternalCheckSpec = { /** Command run in fix mode. When omitted, the check is not fixable and always runs `checkCommand`. */ fixCommand?: readonly string[] devDeps: string[] - /** Dependency build scripts the tool does not need, declined when pnpm installs `devDeps`. */ - declineBuilds?: string[] + /** Packages whose build scripts `init` denies on pnpm. See `Check['scaffold']` for why. */ + pnpmDenyBuilds?: string[] recommended?: boolean /** Docs / config reference for the underlying tool, surfaced when the check fails. */ docs?: string @@ -117,7 +117,7 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { scaffold: { script: `verifyx ${spec.name}`, devDeps: spec.devDeps, - declineBuilds: spec.declineBuilds, + pnpmDenyBuilds: spec.pnpmDenyBuilds, }, // `verifyx eject ` inlines these raw commands into the consumer's verify:* scripts. Lazy so // formatShellCommand's unserializable-arg throw fires on eject only, not when CHECKS is built at import. diff --git a/src/checks/registry.ts b/src/checks/registry.ts index d53e849..a2cfdd3 100644 --- a/src/checks/registry.ts +++ b/src/checks/registry.ts @@ -87,7 +87,7 @@ export const CHECKS: Check[] = [ bin: 'skott', checkCommand: ['skott', '--displayMode=raw', '--showCircularDependencies', '--exitCodeOnCircularDependencies=1'], devDeps: ['skott'], - declineBuilds: ['@parcel/watcher'], + pnpmDenyBuilds: ['@parcel/watcher'], docs: 'https://github.com/antoine-coulon/skott', }), defineExternalCheck({ diff --git a/src/checks/types.ts b/src/checks/types.ts index 42d730f..8d83665 100644 --- a/src/checks/types.ts +++ b/src/checks/types.ts @@ -34,8 +34,15 @@ export type Check = { script: string /** Extra devDependencies the check needs, installed on opt-in. */ devDeps?: string[] - /** Packages whose build scripts the check does not need; pnpm records them as declined at install (npm ignores this). */ - declineBuilds?: string[] + /** + * Dependencies of `devDeps` that run a build script on install, but that the check works without. For example, + * skott depends on `@parcel/watcher`, which compiles native code. + * + * pnpm 11+ fails an install until each such build script is allowed or denied. So on pnpm, `verifyx init` denies + * these by adding `'@parcel/watcher': false` under `allowBuilds` in `pnpm-workspace.yaml`. If the project already + * allows or denies the package, init leaves it alone. + */ + pnpmDenyBuilds?: string[] } /** * External checks only: the raw tool commands `verifyx eject ` inlines into `verify:` (and diff --git a/src/commands/registerInit.ts b/src/commands/registerInit.ts index 6311a82..bc9ab76 100644 --- a/src/commands/registerInit.ts +++ b/src/commands/registerInit.ts @@ -146,7 +146,7 @@ export function registerInit(program: Command): void { let install: InstallReport | undefined if (result.devDeps.length > 0) { console.log(color.dim(`\nResolving ${result.devDeps.length} devDependenc(ies): ${result.devDeps.join(', ')}`)) - install = await installDevDeps(result.devDeps, cwd, { packageManager: manager, declineBuilds: result.declineBuilds }) + install = await installDevDeps(result.devDeps, cwd, { packageManager: manager, pnpmDenyBuilds: result.pnpmDenyBuilds }) reportInstall(install, manager) } diff --git a/src/scaffold/init.test.ts b/src/scaffold/init.test.ts index fc40b11..bec9986 100644 --- a/src/scaffold/init.test.ts +++ b/src/scaffold/init.test.ts @@ -62,11 +62,11 @@ describe('applyInit', () => { expect(result.devDeps).toEqual(expect.arrayContaining(['knip', 'jscpd', 'skott'])) }) - it("collects the selected checks' declined builds, and none when no selected check declines any", () => { - expect(applyInit({ cwd: dir, checks: ['circular-deps', 'unused-code'], targets: [], defaultsOnly: false }).declineBuilds).toEqual([ + it("collects the selected checks' denied builds, and none when no selected check denies any", () => { + expect(applyInit({ cwd: dir, checks: ['circular-deps', 'unused-code'], targets: [], defaultsOnly: false }).pnpmDenyBuilds).toEqual([ '@parcel/watcher', ]) - expect(applyInit({ cwd: dir, checks: ['unused-code', 'complexity'], targets: [], defaultsOnly: false }).declineBuilds).toEqual([]) + expect(applyInit({ cwd: dir, checks: ['unused-code', 'complexity'], targets: [], defaultsOnly: false }).pnpmDenyBuilds).toEqual([]) }) it('does not clobber an existing verify:* script', () => { diff --git a/src/scaffold/init.ts b/src/scaffold/init.ts index 728d304..193801d 100644 --- a/src/scaffold/init.ts +++ b/src/scaffold/init.ts @@ -24,22 +24,22 @@ export type InitResult = { addedScripts: string[] /** devDependencies the selected external checks need (deduped). */ devDeps: string[] - /** Dependency builds the selected checks decline (deduped). */ - declineBuilds: string[] + /** Packages whose build scripts the selected checks deny on pnpm (deduped). */ + pnpmDenyBuilds: string[] agentFiles: ManagedFileResult[] } /** Pure scaffolding step: write package.json scripts + agent files, and report the devDeps to install. */ export function applyInit(opts: InitOptions): InitResult { const devDeps: string[] = [] - const declineBuilds: string[] = [] + const pnpmDenyBuilds: string[] = [] const scripts: Record = {} for (const name of opts.checks) { const check = getCheck(name) if (!check) continue if (check.scaffold.devDeps) devDeps.push(...check.scaffold.devDeps) - if (check.scaffold.declineBuilds) declineBuilds.push(...check.scaffold.declineBuilds) + if (check.scaffold.pnpmDenyBuilds) pnpmDenyBuilds.push(...check.scaffold.pnpmDenyBuilds) if (!opts.defaultsOnly) scripts[`verify:${name}`] = check.scaffold.script } @@ -58,5 +58,5 @@ export function applyInit(opts: InitOptions): InitResult { ensureKnipIgnores(opts.cwd, [...new Set(toolDeps)], agentFiles, detectSystemBinaries(opts.cwd)) } - return { addedScripts, devDeps: [...new Set(devDeps)], declineBuilds: [...new Set(declineBuilds)], agentFiles } + return { addedScripts, devDeps: [...new Set(devDeps)], pnpmDenyBuilds: [...new Set(pnpmDenyBuilds)], agentFiles } } diff --git a/src/scaffold/installDeps.test.ts b/src/scaffold/installDeps.test.ts index f90ea74..3971a64 100644 --- a/src/scaffold/installDeps.test.ts +++ b/src/scaffold/installDeps.test.ts @@ -82,9 +82,9 @@ describe('installDevDeps (npm)', () => { ]) }) - it('passes no decline flags to npm', async () => { + it('passes no deny flags to npm', async () => { const run = fakeRunner() - await installDevDeps(['skott'], dir, { ...npm(run), declineBuilds: ['@parcel/watcher'] }) + await installDevDeps(['skott'], dir, { ...npm(run), pnpmDenyBuilds: ['@parcel/watcher'] }) expect(run.commands).toEqual([['npm', 'install', '--save-dev', 'skott']]) }) @@ -92,10 +92,10 @@ describe('installDevDeps (npm)', () => { describe('installDevDeps (pnpm)', () => { const ADD = ['pnpm', 'add', '-D', '--ignore-workspace-root-check'] - const DECLINE = '--allow-build=!@parcel/watcher' + const DENY = '--allow-build=!@parcel/watcher' const pnpm = (run: Runner, cwd = dir) => - installDevDeps(['skott', 'knip'], cwd, { packageManager: 'pnpm', declineBuilds: ['@parcel/watcher'], run }) + installDevDeps(['skott', 'knip'], cwd, { packageManager: 'pnpm', pnpmDenyBuilds: ['@parcel/watcher'], run }) const writeWorkspace = (yaml: string, at = dir) => fs.writeFileSync(path.join(at, 'pnpm-workspace.yaml'), yaml) @@ -106,12 +106,12 @@ describe('installDevDeps (pnpm)', () => { return run.commands[0] } - it('runs one add with the workspace-root flag and the decline flag', async () => { + it('runs one add with the workspace-root flag and the deny flag', async () => { const run = fakeRunner() const report = await pnpm(run) expect(report).toEqual({ skipped: [], installed: ['skott', 'knip'], failed: [] }) - expect(run.commands).toEqual([[...ADD, DECLINE, 'skott', 'knip']]) + expect(run.commands).toEqual([[...ADD, DENY, 'skott', 'knip']]) }) it('does not retry per package after a failed batch', async () => { @@ -122,8 +122,8 @@ describe('installDevDeps (pnpm)', () => { expect(report).toEqual({ skipped: [], installed: [], failed: ['skott', 'knip'] }) }) - it('declines the build when there is no pnpm-workspace.yaml', async () => { - expect(await argvFor()).toContain(DECLINE) + it('denies the build when there is no pnpm-workspace.yaml', async () => { + expect(await argvFor()).toContain(DENY) }) it.each([ @@ -135,7 +135,7 @@ describe('installDevDeps (pnpm)', () => { expect(await argvFor()).toEqual([...ADD, 'skott', 'knip']) }) - it('leaves the decision alone when one version is approved and another declined', async () => { + it('leaves the decision alone when one version is approved and another denied', async () => { writeWorkspace("allowBuilds:\n '@parcel/watcher@2.4.1': false\n '@parcel/watcher@2.6.0': true\n") expect(await argvFor()).toEqual([...ADD, 'skott', 'knip']) }) @@ -145,12 +145,12 @@ describe('installDevDeps (pnpm)', () => { ['only in ignoredBuiltDependencies', "ignoredBuiltDependencies:\n - '@parcel/watcher'\n"], ['a placeholder in allowBuilds', "allowBuilds:\n '@parcel/watcher': set this to true or false\n"], ['a version-keyed entry with no version', "allowBuilds:\n '@parcel/watcher@': true\n"], - ['declined only for another version', "allowBuilds:\n '@parcel/watcher@2.4.1': false\n"], + ['denied only for another version', "allowBuilds:\n '@parcel/watcher@2.4.1': false\n"], ['an entry for a different package', "allowBuilds:\n '@parcel/watcher-wasm': true\n"], ['an unparseable file', 'allowBuilds: [\n'], - ])('declines the build when the package is %s', async (_, yaml) => { + ])('denies the build when the package is %s', async (_, yaml) => { writeWorkspace(yaml) - expect(await argvFor()).toContain(DECLINE) + expect(await argvFor()).toContain(DENY) }) it('finds the workspace file from a sub-directory', async () => { diff --git a/src/scaffold/installDeps.ts b/src/scaffold/installDeps.ts index 6c6e772..dfa1a23 100644 --- a/src/scaffold/installDeps.ts +++ b/src/scaffold/installDeps.ts @@ -20,8 +20,8 @@ export type InstallReport = { export type InstallOptions = { /** Defaults to the manager detected from `cwd`. */ packageManager?: PackageManager - /** Packages whose build scripts pnpm should record as declined (npm ignores them). */ - declineBuilds?: readonly string[] + /** Packages whose build scripts pnpm should deny under `allowBuilds`. */ + pnpmDenyBuilds?: readonly string[] run?: Runner } @@ -69,7 +69,7 @@ function isVersionKeyFor(key: string, pkg: string): boolean { } /** - * A version-keyed `true` in `allowBuilds` counts as decided, since the decline flag would revoke it; a version-keyed + * A version-keyed `true` in `allowBuilds` counts as decided, since the deny flag would revoke it; a version-keyed * `false` does not, as it covers one version. A non-boolean value is not a decision. pnpm 11+ ignores * `onlyBuiltDependencies` and `ignoredBuiltDependencies`. */ @@ -97,14 +97,14 @@ async function installWithNpm(toInstall: string[], cwd: string, run: Runner): Pr * pnpm: one batch only. A failed batch has usually written every dep already (e.g. it stopped on an unapproved build), * and every later `pnpm add` fails the same way, so a per-package retry would only repeat the failure. */ -async function installWithPnpm(toInstall: string[], cwd: string, declineBuilds: readonly string[], run: Runner): Promise { - const argv = PACKAGE_MANAGER_COMMANDS.pnpm.addDevDependencies(toInstall, undecidedBuilds(declineBuilds, cwd)) +async function installWithPnpm(toInstall: string[], cwd: string, pnpmDenyBuilds: readonly string[], run: Runner): Promise { + const argv = PACKAGE_MANAGER_COMMANDS.pnpm.addDevDependencies(toInstall, undecidedBuilds(pnpmDenyBuilds, cwd)) return (await run(argv, { cwd })) === 0 ? { installed: toInstall, failed: [] } : { installed: [], failed: toInstall } } /** Install the devDeps not already present. Failures are reported instead of aborting `init`. */ export async function installDevDeps(devDeps: readonly string[], cwd: string, options: InstallOptions = {}): Promise { - const { packageManager = detectPackageManager(cwd), declineBuilds = [], run = runArgvCommand } = options + const { packageManager = detectPackageManager(cwd), pnpmDenyBuilds = [], run = runArgvCommand } = options const declared = declaredDeps(cwd) const skipped: string[] = [] const toInstall: string[] = [] @@ -113,6 +113,6 @@ export async function installDevDeps(devDeps: readonly string[], cwd: string, op if (toInstall.length === 0) return { skipped, installed: [], failed: [] } const outcome = - packageManager === 'pnpm' ? await installWithPnpm(toInstall, cwd, declineBuilds, run) : await installWithNpm(toInstall, cwd, run) + packageManager === 'pnpm' ? await installWithPnpm(toInstall, cwd, pnpmDenyBuilds, run) : await installWithNpm(toInstall, cwd, run) return { skipped, ...outcome } } diff --git a/src/shared/packageManager.ts b/src/shared/packageManager.ts index b9fb57a..3cda290 100644 --- a/src/shared/packageManager.ts +++ b/src/shared/packageManager.ts @@ -7,8 +7,8 @@ type ScriptCommand = { command: string; env?: Record } export type PackageManagerCommands = { runScript: (script: string) => ScriptCommand - /** `declineBuilds` lists packages whose build scripts must not run; npm ignores it. */ - addDevDependencies: (deps: readonly string[], declineBuilds?: readonly string[]) => string[] + /** `pnpmDenyBuilds` lists packages whose build scripts pnpm should deny. */ + addDevDependencies: (deps: readonly string[], pnpmDenyBuilds?: readonly string[]) => string[] /** Doc string: run the verify gate. */ runGate: string /** Doc string: run the verifyx CLI. */ @@ -28,12 +28,12 @@ export const PACKAGE_MANAGER_COMMANDS: Record ({ command: `pnpm run ${script}`, env: { pnpm_config_verify_deps_before_run: 'false' } }), - addDevDependencies: (deps, declineBuilds = []) => [ + addDevDependencies: (deps, pnpmDenyBuilds = []) => [ 'pnpm', 'add', '-D', '--ignore-workspace-root-check', - ...declineBuilds.map((pkg) => `--allow-build=!${pkg}`), + ...pnpmDenyBuilds.map((pkg) => `--allow-build=!${pkg}`), ...deps, ], runGate: 'pnpm verify',