diff --git a/package.json b/package.json index e6442a0535..85f4ea931b 100644 --- a/package.json +++ b/package.json @@ -130,7 +130,7 @@ "fallow:baseline": "fallow dead-code --save-baseline fallow-baselines/dead-code.json --summary && fallow health --report-only --save-baseline fallow-baselines/health.json --summary", "check:fallow": "fallow audit", "check:affected": "node --experimental-strip-types scripts/check-affected/run.ts", - "check:affected:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/platform-packages.test.ts scripts/check-affected/device-lanes.test.ts scripts/check-affected/run.test.ts", + "check:affected:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/platform-packages.test.ts scripts/check-affected/device-lanes.test.ts scripts/check-affected/lockfile-install-sync.test.ts scripts/check-affected/run.test.ts", "gate": "node --experimental-strip-types scripts/gate/run.ts", "check:gate-manifest": "node --experimental-strip-types scripts/gate/check.ts", "check:gate-manifest:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/gate/*.test.ts", diff --git a/scripts/check-affected/lockfile-install-sync.test.ts b/scripts/check-affected/lockfile-install-sync.test.ts new file mode 100644 index 0000000000..da6076bf12 --- /dev/null +++ b/scripts/check-affected/lockfile-install-sync.test.ts @@ -0,0 +1,85 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'node:test'; +import { mkdtempForTestSync } from '../../src/__tests__/test-utils/tmp-dir.ts'; +import { checkLockfileInstallSync } from './lockfile-install-sync.ts'; + +function writeLockfile(root: string, content: string): void { + fs.writeFileSync(path.join(root, 'pnpm-lock.yaml'), content); +} + +function writeInstalledSnapshot(root: string, content: string): void { + const dir = path.join(root, 'node_modules', '.pnpm'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'lock.yaml'), content); +} + +test('reports in sync when the installed snapshot byte-matches the checked-out lockfile', () => { + const root = mkdtempForTestSync('agent-device-lockfile-sync-match-'); + writeLockfile(root, 'lockfileVersion: 9.0\nimporters:\n .: {}\n'); + writeInstalledSnapshot(root, 'lockfileVersion: 9.0\nimporters:\n .: {}\n'); + + assert.deepEqual(checkLockfileInstallSync(root), { status: 'in-sync' }); +}); + +test('reports stale when the installed snapshot content differs from the checked-out lockfile', () => { + const root = mkdtempForTestSync('agent-device-lockfile-sync-stale-'); + writeLockfile( + root, + 'lockfileVersion: 9.0\nimporters:\n .:\n dependencies:\n newDep: 1.0.0\n', + ); + writeInstalledSnapshot(root, 'lockfileVersion: 9.0\nimporters:\n .: {}\n'); + + assert.deepEqual(checkLockfileInstallSync(root), { status: 'out-of-sync', reason: 'stale' }); +}); + +test('reports install-missing when a source checkout has no installed snapshot', () => { + const root = mkdtempForTestSync('agent-device-lockfile-sync-no-install-'); + writeLockfile(root, 'lockfileVersion: 9.0\n'); + + assert.deepEqual(checkLockfileInstallSync(root), { + status: 'out-of-sync', + reason: 'install-missing', + }); +}); + +test('reports install-missing for a fresh worktree that has no node_modules directory at all', () => { + const root = mkdtempForTestSync('agent-device-lockfile-sync-fresh-worktree-'); + writeLockfile(root, 'lockfileVersion: 9.0\n'); + assert.equal(fs.existsSync(path.join(root, 'node_modules')), false); + + assert.deepEqual(checkLockfileInstallSync(root), { + status: 'out-of-sync', + reason: 'install-missing', + }); +}); + +test('reports no-source-checkout when there is no pnpm-lock.yaml', () => { + const root = mkdtempForTestSync('agent-device-lockfile-sync-packaged-'); + fs.writeFileSync(path.join(root, 'package.json'), '{"name":"agent-device"}\n'); + + assert.deepEqual(checkLockfileInstallSync(root), { status: 'no-source-checkout' }); +}); + +test('reports no-source-checkout even when an installed snapshot exists without a lockfile', () => { + const root = mkdtempForTestSync('agent-device-lockfile-sync-no-lockfile-'); + writeInstalledSnapshot(root, 'lockfileVersion: 9.0\n'); + + assert.deepEqual(checkLockfileInstallSync(root), { status: 'no-source-checkout' }); +}); + +test('checks each worktree against its own lockfile state', () => { + const worktreeA = mkdtempForTestSync('agent-device-lockfile-sync-worktree-a-'); + const worktreeB = mkdtempForTestSync('agent-device-lockfile-sync-worktree-b-'); + writeLockfile(worktreeA, 'lockfileVersion: 9.0\nimporters:\n .: {}\n'); + writeInstalledSnapshot(worktreeA, 'lockfileVersion: 9.0\nimporters:\n .: {}\n'); + writeLockfile( + worktreeB, + 'lockfileVersion: 9.0\nimporters:\n .:\n dependencies:\n newDep: 1.0.0\n', + ); + writeInstalledSnapshot(worktreeB, 'lockfileVersion: 9.0\nimporters:\n .: {}\n'); + + assert.deepEqual(checkLockfileInstallSync(worktreeA), { status: 'in-sync' }); + assert.deepEqual(checkLockfileInstallSync(worktreeB), { status: 'out-of-sync', reason: 'stale' }); +}); diff --git a/scripts/check-affected/lockfile-install-sync.ts b/scripts/check-affected/lockfile-install-sync.ts new file mode 100644 index 0000000000..209ef9b4e0 --- /dev/null +++ b/scripts/check-affected/lockfile-install-sync.ts @@ -0,0 +1,39 @@ +import fs from 'node:fs'; +import path from 'node:path'; + +const LOCKFILE_BASENAME = 'pnpm-lock.yaml'; +const INSTALLED_SNAPSHOT_RELATIVE_PATH = ['node_modules', '.pnpm', 'lock.yaml']; + +export const STALE_NODE_MODULES_MESSAGE = + "node_modules does not match this worktree's pnpm-lock.yaml"; + +export type LockfileInstallSyncResult = + | { readonly status: 'no-source-checkout' } + | { readonly status: 'in-sync' } + | { + readonly status: 'out-of-sync'; + readonly reason: 'install-missing' | 'stale'; + }; + +/** + * Compares pnpm's installed lockfile snapshot with this worktree's lockfile. + * This is intentionally synchronous and subprocess-free: it runs before any gate. + */ +export function checkLockfileInstallSync(repoRoot: string): LockfileInstallSyncResult { + const lockfile = readFileIfExists(path.join(repoRoot, LOCKFILE_BASENAME)); + if (!lockfile) return { status: 'no-source-checkout' }; + + const installedSnapshot = readFileIfExists( + path.join(repoRoot, ...INSTALLED_SNAPSHOT_RELATIVE_PATH), + ); + if (!installedSnapshot) return { status: 'out-of-sync', reason: 'install-missing' }; + + return lockfile.equals(installedSnapshot) + ? { status: 'in-sync' } + : { status: 'out-of-sync', reason: 'stale' }; +} + +function readFileIfExists(filePath: string): Buffer | undefined { + if (!fs.existsSync(filePath)) return undefined; + return fs.readFileSync(filePath); +} diff --git a/scripts/check-affected/run.test.ts b/scripts/check-affected/run.test.ts index ac4a743eaf..58402cb696 100644 --- a/scripts/check-affected/run.test.ts +++ b/scripts/check-affected/run.test.ts @@ -9,6 +9,7 @@ import os from 'node:os'; import path from 'node:path'; import { test } from 'node:test'; import { runCmdSync } from '../../src/utils/exec.ts'; +import { STALE_NODE_MODULES_MESSAGE } from './lockfile-install-sync.ts'; import { CHECK_CATALOG } from './checks.ts'; import { DEFAULT_VITEST_MAX_WORKERS } from '../lib/vitest-concurrency.ts'; import { selectChecks } from './model.ts'; @@ -247,3 +248,111 @@ test('runChecks leaves coverage to CI and runs capped related tests once', async 'the coverage gate is GitHub-authoritative and must not run locally', ); }); + +test('runChecks fails fast on a stale install before running format or any other check', async () => { + const executed: string[][] = []; + const execute: CommandExecutor = async (command) => { + executed.push(command); + return 0; + }; + const plan = selectChecks({ + changedFiles: ['packages/selectors/src/index.ts'], + packageEntryFiles: [], + }); + const code = await runChecks(plan, { scripts: ALL_SCRIPTS }, ARGS, { + execute, + cwd: '.', + checkLockfileSync: () => ({ status: 'out-of-sync', reason: 'stale' }), + }); + assert.equal(code, 1); + assert.deepEqual(executed, [], 'no check — including format — may run against a stale install'); +}); + +test('runChecks fails fast when node_modules was never installed in this checkout', async () => { + const executed: string[][] = []; + const execute: CommandExecutor = async (command) => { + executed.push(command); + return 0; + }; + const plan = selectChecks({ + changedFiles: ['packages/selectors/src/index.ts'], + packageEntryFiles: [], + }); + const code = await runChecks(plan, { scripts: ALL_SCRIPTS }, ARGS, { + execute, + cwd: '.', + checkLockfileSync: () => ({ status: 'out-of-sync', reason: 'install-missing' }), + }); + assert.equal(code, 1); + assert.deepEqual(executed, []); +}); + +test('runChecks does not block when there is no source checkout to compare against', async () => { + const executed: string[][] = []; + const execute: CommandExecutor = async (command) => { + executed.push(command); + return 0; + }; + const plan = selectChecks({ + changedFiles: ['packages/selectors/src/index.ts'], + packageEntryFiles: [], + }); + const code = await runChecks(plan, { scripts: ALL_SCRIPTS }, ARGS, { + execute, + cwd: '.', + checkLockfileSync: () => ({ status: 'no-source-checkout' }), + }); + assert.equal(code, 0); + assert.ok(executed.length > 0, 'checks still run when there is nothing to compare against'); +}); + +test('runChecks proceeds normally when the install is in sync', async () => { + const executed: string[][] = []; + const execute: CommandExecutor = async (command) => { + executed.push(command); + return 0; + }; + const plan = selectChecks({ + changedFiles: ['packages/selectors/src/index.ts'], + packageEntryFiles: [], + }); + const code = await runChecks(plan, { scripts: ALL_SCRIPTS }, ARGS, { + execute, + cwd: '.', + checkLockfileSync: () => ({ status: 'in-sync' }), + }); + assert.equal(code, 0); + assert.ok(executed.some((command) => command.includes('format:check'))); +}); + +test('runChecks names the real cause on stderr instead of surfacing as an unrelated failure', async () => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = ((chunk: string | Uint8Array) => { + stderrChunks.push(chunk.toString()); + return true; + }) as typeof process.stderr.write; + + try { + const plan = selectChecks({ + changedFiles: ['packages/selectors/src/index.ts'], + packageEntryFiles: [], + }); + const code = await runChecks(plan, { scripts: ALL_SCRIPTS }, ARGS, { + execute: async () => 0, + cwd: '.', + checkLockfileSync: () => ({ status: 'out-of-sync', reason: 'stale' }), + }); + assert.equal(code, 1); + assert.ok( + stderrChunks.some((chunk) => chunk.includes(STALE_NODE_MODULES_MESSAGE)), + `expected stderr to name the stale-install cause, got: ${stderrChunks.join('')}`, + ); + const stderr = stderrChunks.join(''); + assert.match(stderr, /Worktree: \.\n/); + assert.match(stderr, /pnpm install --frozen-lockfile/); + assert.doesNotMatch(stderr, /agent-device doctor/); + } finally { + process.stderr.write = originalWrite; + } +}); diff --git a/scripts/check-affected/run.ts b/scripts/check-affected/run.ts index d31f5bf2a9..7c4d3a2759 100644 --- a/scripts/check-affected/run.ts +++ b/scripts/check-affected/run.ts @@ -9,6 +9,11 @@ import fs from 'node:fs'; import path from 'node:path'; import { pathToFileURL } from 'node:url'; import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; +import { + checkLockfileInstallSync, + STALE_NODE_MODULES_MESSAGE, + type LockfileInstallSyncResult, +} from './lockfile-install-sync.ts'; import { parseScriptArgs } from '../lib/cli-args.ts'; import { runEntrypoint } from '../lib/cli-entrypoint.ts'; import { @@ -184,9 +189,21 @@ export async function runChecks( plan: CheckPlan, pkg: PackageJson, args: Args, - options: { cwd?: string; execute?: CommandExecutor; changedFiles?: readonly string[] } = {}, + options: { + cwd?: string; + execute?: CommandExecutor; + changedFiles?: readonly string[]; + checkLockfileSync?: (cwd: string) => LockfileInstallSyncResult; + } = {}, ): Promise { const cwd = options.cwd ?? repoRoot; + const checkLockfileSync = options.checkLockfileSync ?? checkLockfileInstallSync; + // Fail before any gate can misdiagnose a stale worktree install (#1956). + const lockfileSync = checkLockfileSync(cwd); + if (lockfileSync.status === 'out-of-sync') { + reportStaleInstall(lockfileSync.reason, cwd); + return 1; + } const execute = options.execute ?? streamingExecutor; const runnable = plan.checks.map(getCheckSpec).filter((spec: CheckSpec) => spec.localRunnable); const skipped = plan.checks.map(getCheckSpec).filter((spec: CheckSpec) => !spec.localRunnable); @@ -212,6 +229,17 @@ export async function runChecks( return 0; } +function reportStaleInstall(reason: 'install-missing' | 'stale', cwd: string): void { + process.stderr.write(`\ncheck:affected: ${STALE_NODE_MODULES_MESSAGE}\n`); + process.stderr.write( + reason === 'install-missing' + ? '(no node_modules/.pnpm/lock.yaml found — this checkout was never installed)\n' + : '(node_modules/.pnpm/lock.yaml disagrees with pnpm-lock.yaml)\n', + ); + process.stderr.write(`Worktree: ${cwd}\n`); + process.stderr.write('Run `pnpm install --frozen-lockfile` in this worktree, then retry.\n'); +} + // Where a check the local run skips is authoritative. A parked check has no automatic // lane; say so instead of printing an empty job list. function describeOwner(id: CheckId, ciJobs: ReadonlyMap): string {