From 94d803cb83014378b898c90c47ca0965b6cd2a8f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 11:45:41 +0000 Subject: [PATCH] test(ci): take the live registry reading out of a REQUIRED context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/__tests__/check-lockfile-dedupe.test.ts` ran `node scripts/check-lockfile-dedupe.mjs` twice — a `pnpm dedupe --check` that resolves against `registry.npmjs.org` — from the `unit` vitest project, which `ci.yml` shards four ways as `Test (shard N/4)`. All four are REQUIRED contexts, so a term outside the repository decided a required verdict. Measured with the lockfile blob held fixed (sha256 identical before and after every leg): warm cache, cold cache and both half-populated cache states all return exit 0 `VERDICT deduped`, while the same bytes with the registry unreachable return exit 2 `VERDICT could not take a reading` — which the checker documents as never a pass. Byte-identical input, two verdicts. The live reading now runs only where this repository already put it: the path-filtered `Lockfile Dedupe Check` workflow, still classified BLOCKING in `OPTIONAL_CONTEXTS`, untouched. This file drives the shipped script through a stubbed `pnpm` instead, and every run asserts the stub served it, so a spawn that reached a live pnpm reds rather than going quiet. Coverage goes up, not down: the two live legs only ever exercised the GREEN path through `main()`. The replacements cover the finding path, the could-not-run path, the crash path and the `dedupe --check` argv that is why the checker cannot rewrite the lockfile it judges. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_015h79niBMyoB1xcaQje3uiz --- .../__tests__/check-lockfile-dedupe.test.ts | 250 +++++++++++++++--- 1 file changed, 218 insertions(+), 32 deletions(-) diff --git a/scripts/__tests__/check-lockfile-dedupe.test.ts b/scripts/__tests__/check-lockfile-dedupe.test.ts index e929ac7bdc..c681a90c15 100644 --- a/scripts/__tests__/check-lockfile-dedupe.test.ts +++ b/scripts/__tests__/check-lockfile-dedupe.test.ts @@ -2,13 +2,54 @@ * objectui#8333 — the lockfile-dedupe gate, pinned against the thing it exists * to prevent and against the ways it could be worthless. * - * The checker's own verdict logic is exercised by `--self-test` inside the - * script (no pnpm, no network). This file adds the three things that cannot - * say: + * ## ⛔ Nothing in this file may take a LIVE reading (objectui#9562) + * + * This file runs in the `unit` vitest project, which `ci.yml` shards four ways + * as `Test (shard N/4)` — and all four shards are REQUIRED contexts in + * `scripts/dependabot-merge-gate.mjs`. A required context may only carry + * readings that are a function of the repository's own bytes. + * + * `scripts/check-lockfile-dedupe.mjs` is not such a reading and never was: it + * shells out to `pnpm dedupe --check`, which resolves against + * `registry.npmjs.org`. Its own header says so, and `lockfile-dedupe.yml` is + * built around it — that workflow is PATH-FILTERED precisely so the live + * reading reaches only the pull requests that can change its subject, which + * under objectui#3523's rule is also what keeps it out of the required set. + * + * Until objectui#9562 this file ran that same live command TWICE on every pull + * request, inside a required context, bypassing the filter that was put there + * to bound it. Measured on objectui#9562, holding the lockfile blob fixed + * (sha256 identical before and after every leg) and varying only what sits + * outside the repository: + * + * - warm metadata cache, cold cache, and both half-populated states + * (pnpm keeps abbreviated and full packuments in SEPARATE caches): + * exit 0, `VERDICT deduped`, four for four; + * - the same bytes with the registry unreachable: exit 2, + * `VERDICT could not take a reading` — which the checker documents as + * "never a pass", so the required context reds. + * + * ⇒ byte-identical repository input, two different verdicts, decided by a term + * the repository does not contain. ⚠️ That reproduces the CLASS objectui#9562 + * reported, ⛔ not the specific red it recorded (that one printed + * `VERDICT not deduped`, and its mechanism is still unidentified — ⛔ do not + * adopt one here). + * + * So the live reading stays where this repository already put it — the + * path-filtered `Lockfile Dedupe Check` context, still BLOCKING, unchanged — + * and this file drives the shipped script through a STUBBED `pnpm` instead. + * ⛔ Do not reintroduce a bare spawn of the checker here: every run below + * asserts the stub served it (`pnpmArgv` is written by the stub and by nothing + * else), so a spawn that reached the real pnpm fails rather than going quiet. + * + * ## What this file covers that the checker's `--self-test` cannot * * 1. the self-test really passes, run as shipped rather than re-implemented; - * 2. it is GREEN on this repository's real committed `pnpm-lock.yaml` — the - * property objectui#9215 established and this gate defends; + * 2. `main()` end to end — the exit-code mapping, the annotations, and the + * `dedupe --check` argv that is WHY the checker cannot rewrite the + * lockfile it judges. ⭐ The live legs this replaced only ever exercised + * the GREEN path; the red and could-not-run paths through `main()` had no + * coverage at all; * 3. it is WIRED — a workflow runs it, that workflow is path-filtered (so it * cannot be REQUIRED under objectui#3523's rule), and the Dependabot merge * gate classifies the check it produces as BLOCKING. @@ -20,21 +61,15 @@ * going red. That demotion is a real option and the workflow header says how to * take it deliberately; this file is what makes it deliberate rather than * incidental. - * - * ⚠️ (2) is deliberately NOT paired with a red over the same corpus here. The - * red direction costs a full re-resolve of a mutated manifest plus a registry - * round trip, which is not a unit test's to spend; it is measured on - * objectui#8333's pull request instead, and the checker's `--self-test` holds - * the red/green discrimination over captured pnpm output. What this file must - * not become is a green-only assertion whose checker could never fail — which - * is why case (1) runs the self-test, whose controls include exactly that. */ -import { execFileSync, spawnSync } from 'node:child_process'; +import { spawnSync } from 'node:child_process'; import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; import { describe, expect, it } from 'vitest'; +import { DEDUPE_SENTINEL } from '../check-lockfile-dedupe.mjs'; import { NOT_A_GATE, OPTIONAL_CONTEXTS, REQUIRED_CONTEXTS } from '../dependabot-merge-gate.mjs'; import { pullRequestTrigger, readWorkflows, repoRoot, subscribesMergeGroup } from './workflow-checks.js'; @@ -42,6 +77,98 @@ const SCRIPT = 'scripts/check-lockfile-dedupe.mjs'; const WORKFLOW = 'lockfile-dedupe.yml'; const CONTEXT = 'Lockfile Dedupe Check'; +/** + * Real `pnpm@10.31.0` output, same capture objectui#8333 took and the checker's + * `--self-test` carries: a tree where `better-auth` has forked `zod`. + */ +const PNPM_RED = [ + '@objectstack/spec@17.4.0(ai@7.0.65(zod@4.6.4))', + '└── zod 4.4.3 → 4.6.4', + '', + '+ zod-validation-error@4.0.2(zod@4.6.4)', + '- @ai-sdk/gateway@4.0.52(zod@4.4.3)', + '- @objectstack/spec@17.4.0(ai@7.0.65(zod@4.4.3))', + '- ai@7.0.65(zod@4.4.3)', + '- zod@4.4.3', + '', + DEDUPE_SENTINEL, + '', +].join('\n'); + +/** Real `pnpm@10.31.0` output on a deduped tree. */ +const PNPM_GREEN = ' WARN 7 deprecated subdependencies found: glob@7.2.3\nProgress: resolved 1767, done\n'; + +/** + * Real `pnpm@10.31.0` output captured on objectui#9562 by pointing the registry + * at a closed port while the lockfile stayed byte-identical. ⭐ This is the + * shape that used to red a REQUIRED context for a reason that was not about the + * pull request — it exits non-zero and carries NO remedy sentinel. + */ +const PNPM_REGISTRY_DOWN = [ + ' WARN GET http://127.0.0.1:9/@dnd-kit%2Fcore error (ECONNREFUSED). Will retry in 10 seconds. 2 retries left.', + ' ERR_PNPM_META_FETCH_FAIL GET http://127.0.0.1:9/@dnd-kit%2Fcore: request to', + ' http://127.0.0.1:9/@dnd-kit%2Fcore failed, reason: connect ECONNREFUSED 127.0.0.1:9', + '', +].join('\n'); + +interface CheckerRun { + status: number | null; + stdout: string; + stderr: string; + /** Written by the stub and by nothing else — absent means the real pnpm ran. */ + pnpmArgv: string | null; +} + +/** + * Run the checker AS SHIPPED with `pnpm` stubbed on `PATH`. + * + * ⛔ The only way this file may invoke the checker. The stub records its argv, + * and every caller asserts that recording exists — that is the control which + * makes "no live reading" a measurement rather than a promise in a comment. + */ +function runChecker(stub: { stdout?: string; stderr?: string; exit: number }): CheckerRun { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'lockfile-dedupe-stub-')); + try { + const bin = path.join(dir, 'bin'); + fs.mkdirSync(bin); + const argvFile = path.join(dir, 'pnpm-argv'); + const stdoutFile = path.join(dir, 'pnpm-stdout'); + const stderrFile = path.join(dir, 'pnpm-stderr'); + fs.writeFileSync(stdoutFile, stub.stdout ?? ''); + fs.writeFileSync(stderrFile, stub.stderr ?? ''); + const pnpm = path.join(bin, 'pnpm'); + fs.writeFileSync( + pnpm, + [ + '#!/usr/bin/env bash', + `printf '%s' "$*" > ${JSON.stringify(argvFile)}`, + `cat ${JSON.stringify(stdoutFile)}`, + `cat ${JSON.stringify(stderrFile)} >&2`, + `exit ${stub.exit}`, + '', + ].join('\n'), + ); + fs.chmodSync(pnpm, 0o755); + + const proc = spawnSync('node', [SCRIPT], { + cwd: repoRoot, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}${path.delimiter}${process.env.PATH ?? ''}`, + }, + }); + return { + status: proc.status, + stdout: proc.stdout, + stderr: proc.stderr, + pnpmArgv: fs.existsSync(argvFile) ? fs.readFileSync(argvFile, 'utf8') : null, + }; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + describe('the lockfile-dedupe checker', () => { it('ships its own self-test, and it passes as shipped', () => { const run = spawnSync('node', [SCRIPT, '--self-test'], { cwd: repoRoot, encoding: 'utf8' }); @@ -51,25 +178,76 @@ describe('the lockfile-dedupe checker', () => { expect(run.stdout).toMatch(/\d+ cases pass/); }); - it('is GREEN on this repository’s real committed lockfile', () => { - // The property objectui#9215 paid for and this gate defends. If this ever - // reds, `main` has re-accrued collapsible duplication and the remedy is - // `pnpm dedupe`, not an edit to this test. - const run = spawnSync('node', [SCRIPT], { cwd: repoRoot, encoding: 'utf8' }); - expect( - run.status, - `${SCRIPT} is not green on the committed lockfile:\n${run.stdout}\n${run.stderr}`, - ).toBe(0); + it('asks pnpm for `dedupe --check`, which is WHY it cannot rewrite the lockfile', () => { + // This replaces a live leg that re-resolved the whole workspace and then + // compared the lockfile's bytes. That leg could only catch a dropped + // `--check` if the run happened to rewrite something; this catches it + // always, and in milliseconds. `--check` is pnpm's own contract for + // "report without installing packages or editing the lockfile". + const run = runChecker({ stdout: PNPM_GREEN, exit: 0 }); + expect(run.pnpmArgv, 'the stub did not serve this run — a live pnpm reached the registry').toBe('dedupe --check'); + }); + + it('reports a deduped tree as clean, on stdout, exit 0', () => { + const run = runChecker({ stdout: PNPM_GREEN, exit: 0 }); + expect(run.pnpmArgv).toBe('dedupe --check'); + expect(run.status).toBe(0); expect(run.stdout).toContain('VERDICT deduped'); - }, 300_000); - - it('does not rewrite the lockfile it judges', () => { - const lockfile = path.join(repoRoot, 'pnpm-lock.yaml'); - const before = fs.readFileSync(lockfile); - expect(before.length, 'the lockfile read back empty — this assertion would be vacuous').toBeGreaterThan(0); - execFileSync('node', [SCRIPT], { cwd: repoRoot, encoding: 'utf8' }); - expect(fs.readFileSync(lockfile).equals(before)).toBe(true); - }, 300_000); + }); + + it('reports a collapsible tree as a finding, names the packages, exit 1', () => { + const run = runChecker({ stdout: PNPM_RED, exit: 1 }); + expect(run.pnpmArgv).toBe('dedupe --check'); + expect(run.status).toBe(1); + expect(run.stderr).toContain('VERDICT not deduped'); + expect(run.stderr).toContain('zod'); + expect(run.stderr).toContain('@objectstack/spec'); + // The annotation is what a reviewer sees on the Files tab. + expect(run.stderr).toContain('::error title=Lockfile dedupe::'); + // A finding is never written to stdout, where a `VERDICT deduped` grep would + // be looking for it. + expect(run.stdout).not.toContain('VERDICT'); + }); + + it('⭐ a registry it cannot reach is NOT a finding and NOT a pass — exit 2', () => { + // The leg objectui#9562 turns on. Before it, this same condition arrived in + // a REQUIRED context as a bare `expect(status).toBe(0)` failure, telling the + // reader to run `pnpm dedupe` and commit the lockfile — advice that is + // wrong here and, because the lockfile is shared by every open pull + // request, advice that spreads. + const run = runChecker({ stderr: PNPM_REGISTRY_DOWN, exit: 1 }); + expect(run.pnpmArgv).toBe('dedupe --check'); + expect(run.status).not.toBe(0); + expect(run.status, 'a crash must not be reported as a lockfile finding').not.toBe(1); + expect(run.status).toBe(2); + expect(run.stderr).toContain('VERDICT could not take a reading'); + expect(run.stderr).toContain('Nothing here was judged'); + // ⛔ It must not print the remedy: there is no finding to remedy. + expect(run.stderr).not.toContain('VERDICT not deduped'); + }); + + it('treats a signal-killed pnpm as could-not-run, never as clean', () => { + // `status === null` is unreachable through a shell stub, so this pins the + // next-worst shape the checker must not read as a pass: a non-zero exit + // with no output at all. + const run = runChecker({ exit: 137 }); + expect(run.pnpmArgv).toBe('dedupe --check'); + expect(run.status).toBe(2); + expect(run.stderr).toContain('VERDICT could not take a reading'); + }); + + it('returns the same verdict and the same bytes for the same input', () => { + // The property objectui#9562 is about, asserted over the half of the input + // this repository controls. ⚠️ It says nothing about the live reading in + // `lockfile-dedupe.yml`, which is still registry-dependent by design. + const first = runChecker({ stdout: PNPM_RED, exit: 1 }); + const second = runChecker({ stdout: PNPM_RED, exit: 1 }); + expect(first.pnpmArgv).toBe('dedupe --check'); + expect(second.pnpmArgv).toBe('dedupe --check'); + expect(second.status).toBe(first.status); + expect(second.stdout).toBe(first.stdout); + expect(second.stderr).toBe(first.stderr); + }); }); describe('the lockfile-dedupe gate is wired the way its header claims', () => { @@ -86,6 +264,14 @@ describe('the lockfile-dedupe gate is wired the way its header claims', () => { expect(body()).toContain(`name: ${CONTEXT}`); }); + it('is the only place the LIVE reading runs (objectui#9562)', () => { + // The live, registry-dependent execution belongs to this path-filtered + // workflow and nowhere else. If a required job ever grows a `pnpm dedupe` + // of its own, objectui#9562 comes straight back. + const live = workflows.filter((w) => w.lines.join('\n').includes(`node ${SCRIPT}`)).map((w) => w.file); + expect(live).toEqual([WORKFLOW]); + }); + it('is path-filtered, and the filter lists the gate’s own runtime closure', () => { const trigger = pullRequestTrigger(workflow!); expect(trigger.subscribes, 'the gate must run on pull requests').toBe(true);