From 4814064a152044cf146e843d63cca442d6b0423e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 17:36:30 +0000 Subject: [PATCH] fix(ci): the lockfile-dedupe gate reports on pull requests instead of blocking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check-lockfile-dedupe.mjs` takes a LIVE registry reading (`pnpm dedupe --check`), and that reading was measured disagreeing with itself: four green and one red on a byte-identical `pnpm-lock.yaml` inside ninety minutes, the red printing a confident `VERDICT not deduped` that no lockfile edit and no registry publish explained — and then instructing the reader to commit a dedupe to a file every open pull request shares. Per the maintainer's ruling (letter A): - the script gains one explicit flag, `--report-only`, which keeps every reading exactly as it is and changes only the consequence: annotations become `::warning::`, the same reading is written to `$GITHUB_STEP_SUMMARY`, and the process exits 0. `lockfile-dedupe.yml` passes it; - the bare script keeps its 0/1/2 exit codes for hand runs; - the finding text, in BOTH modes, now names the instrument — re-run before acting, dedupe only when the split reproduces — and the old unconditional instruction to commit a dedupe is gone from both. The check keeps its name, its path filter and its `OPTIONAL_CONTEXTS` classification, so nothing in branch protection or the merge queue moves. The relaxation is scoped to the VERDICT: a failed checkout, a broken pnpm setup or a `--self-test` that stops passing still reds this context. Report-only may never flatten the three answers into two. `clean` and `cannot-run` both exit 0 under the flag, so the exit code no longer separates them; the annotation does, and the tests read those two runs against each other rather than in isolation. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_015h79niBMyoB1xcaQje3uiz --- .github/workflows/lockfile-dedupe.yml | 72 +++-- .../__tests__/check-lockfile-dedupe.test.ts | 142 +++++++++- scripts/check-lockfile-dedupe.mjs | 249 ++++++++++++++++-- 3 files changed, 406 insertions(+), 57 deletions(-) diff --git a/.github/workflows/lockfile-dedupe.yml b/.github/workflows/lockfile-dedupe.yml index 27de854534..4dc47d0e77 100644 --- a/.github/workflows/lockfile-dedupe.yml +++ b/.github/workflows/lockfile-dedupe.yml @@ -31,37 +31,49 @@ name: Lockfile Dedupe # ⚠️ The filter is also what makes it UNREQUIRABLE: objectui#3523's rule, # enforced by `scripts/__tests__/dependabot-merge-gate.test.ts`, is that a # REQUIRED context must come from a workflow whose `pull_request` trigger has no -# path filter. It is enrolled as an OPTIONAL context instead — blocking when it -# runs, not waited for when the filter keeps it out. `Bundle Analysis` is -# enrolled on exactly those terms and its filter also includes `pnpm-lock.yaml`. +# path filter. It is enrolled as an OPTIONAL context instead — present it must +# be green, absent it is not waited for. `Bundle Analysis` is enrolled on +# exactly those terms and its filter also includes `pnpm-lock.yaml`. # -# ── ⚠️ It IS a blocking context, and that is a decision, not a default ──────── +# ── ⚠️ Its VERDICT no longer blocks a pull request — a ruling, not a default ── # -# `scripts/dependabot-merge-gate.mjs` classifies `Lockfile Dedupe Check` in -# `OPTIONAL_CONTEXTS`, so a red here stops a Dependabot auto-merge. Its -# neighbour `Lockfile Integrity Check` is deliberately NOT enrolled, and that -# difference is intentional rather than an inconsistency to tidy away: +# objectui#9562 measured this gate returning FOUR GREEN AND ONE RED on a +# byte-identical `pnpm-lock.yaml` inside ninety minutes: same blob at every ref, +# no lockfile edit anywhere, and no registry publish that day to explain it. The +# red printed a confident `VERDICT not deduped` naming an `esbuild` peer split — +# and then told the reader to fix it HERE by committing a dedupe, to a file every +# open pull request shares. ⛔ The mechanism was never identified, and this file +# ⛔ does not adopt one. # -# * #8326's gate reports a duplication and leaves the ANSWER open — re-lock, -# pin, or accept the split — so enrolling it would block pull requests on a -# judgement call with no mechanical remedy. Its header reserves that for the -# maintainer, and ⛔ this file does not touch that reservation. -# * This gate has exactly ONE remedy and pnpm prints it: run `pnpm dedupe` and -# commit the lockfile. No declaration, range or override changes. There is no -# judgement call to reserve. +# The maintainer ruled letter A on 2026-09-17 (card objectui#9562, ruling comment +# 5717182406): a gate that is non-deterministic BY CONSTRUCTION does not block. +# So the job now passes `--report-only` to the checker below. What that changes, +# and ⛔ what it deliberately does not: # -# The cost of enrolling it was measured before it was taken, and it is near zero -# TODAY only because objectui#9215 landed first: on `main` after that collapse -# this gate is green, so nothing currently-mergeable is blocked by it. On `main` -# BEFORE it, the same command reds. ⇒ what this gate really defends is that -# objectui#9215's paydown does not silently accrue again — which is the whole -# reason the bump PR's `pnpm dedupe` is cheap enough to require at all. +# * CHANGED — on a pull request a `not deduped` verdict is a `::warning::` +# annotation plus a step-summary block naming the split, and the step exits +# 0. The finding text, in BOTH modes, now says what the instrument is: +# re-run before acting, and dedupe only when the split reproduces. +# * UNCHANGED — the job keeps its name (`Lockfile Dedupe Check`), its path +# filter and its `OPTIONAL_CONTEXTS` classification, so ⛔ nothing in branch +# protection or the merge queue moves. The BARE script keeps its 0/1/2 exit +# codes for whoever runs it by hand; that is the way to ask for the hard +# verdict, and ⛔ it is not an oversight to tidy away. +# * ⚠️ STILL HAS TEETH, and this is why the classification stays honest rather +# than becoming vestigial: `--report-only` relaxes the VERDICT, not the JOB. +# A red checkout, a failed `ci-setup-pnpm.sh`, or a `--self-test` that stops +# passing still fails this context and still stops a Dependabot auto-merge. +# What no longer reds is a live registry reading disagreeing with itself. # -# ⇒ ⚠️ If this ever needs to stop blocking, the flip is one line: move -# `Lockfile Dedupe Check` from `OPTIONAL_CONTEXTS` to `NOT_A_GATE` in -# `scripts/dependabot-merge-gate.mjs`. The partition test keeps either state -# honest; ⛔ deleting the name from both buckets fails that test rather than -# quietly shrinking the gate. +# ⚠️ `scripts/dependabot-merge-gate.mjs`'s entry for this name still reads +# "Blocking when it runs". After this change that is imprecise for the verdict +# (true for the job, per the bullet above); the classification itself is correct +# and ⛔ was deliberately not edited here — that file is held by another open +# pull request, and prose is not worth the collision. +# +# ⇒ what this gate still defends is objectui#9215's paydown: on `main` after that +# collapse it is green, on `main` before it the same command reds. It now defends +# it by REPORTING the drift rather than by refusing the pull request. # # ── No `merge_group` trigger ───────────────────────────────────────────────── # @@ -154,5 +166,11 @@ jobs: # No `pnpm install`: `pnpm dedupe --check` resolves and reports without # writing a lockfile or a node_modules tree (verified on objectui#8333 — # the lockfile's sha256 is unchanged across repeated runs). + # `--report-only` (objectui#9562, ruling A): the reading is unchanged, the + # consequence is not — `::warning::` + a step summary, exit 0. ⛔ Do not + # drop the flag to "make the gate strict again": the gate is a live + # registry reading that was measured disagreeing with itself, and a + # blocking version of it teaches every seat to re-run on red, which hides + # the real failures of this same check. - name: Check the committed lockfile is deduped - run: node scripts/check-lockfile-dedupe.mjs + run: node scripts/check-lockfile-dedupe.mjs --report-only diff --git a/scripts/__tests__/check-lockfile-dedupe.test.ts b/scripts/__tests__/check-lockfile-dedupe.test.ts index ca8408ffb9..4c83ff8bb0 100644 --- a/scripts/__tests__/check-lockfile-dedupe.test.ts +++ b/scripts/__tests__/check-lockfile-dedupe.test.ts @@ -36,8 +36,15 @@ * 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. + * path-filtered `Lockfile Dedupe Check` context — and this file drives the + * shipped script through a STUBBED `pnpm` instead. + * + * ⚠️ That context is no longer BLOCKING on its verdict. objectui#9562's ruling + * (letter A, comment 5717182406) made it REPORT on pull requests: the workflow + * passes `--report-only`, a `not deduped` verdict becomes a `::warning::` plus a + * step summary, and the step exits 0. The bare script keeps its 0/1/2 exit + * codes. Both halves are pinned below, and ⛔ the relaxation is scoped to the + * VERDICT: a job that fails for any other reason still reds this context. * ⛔ 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. @@ -252,6 +259,14 @@ interface CheckerRun { stderr: string; /** Written by the stub and by nothing else — absent means the real pnpm ran. */ pnpmArgv: string | null; + /** + * What the run appended to `$GITHUB_STEP_SUMMARY`, which is pointed at a file + * of this run's own for EVERY leg — including the legs that must write + * nothing. ⚠️ Without that, an empty summary would be indistinguishable from + * an environment variable that never arrived; the legs that DO write are the + * positive control for the legs that must not. + */ + stepSummary: string; } /** @@ -261,7 +276,7 @@ interface CheckerRun { * 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 { +function runChecker(stub: { stdout?: string; stderr?: string; exit: number }, args: string[] = []): CheckerRun { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'lockfile-dedupe-stub-')); try { const bin = path.join(dir, 'bin'); @@ -285,12 +300,19 @@ function runChecker(stub: { stdout?: string; stderr?: string; exit: number }): C ); fs.chmodSync(pnpm, 0o755); - const proc = spawnSync('node', [SCRIPT], { + const summaryFile = path.join(dir, 'step-summary.md'); + fs.writeFileSync(summaryFile, ''); + + const proc = spawnSync('node', [SCRIPT, ...args], { cwd: repoRoot, encoding: 'utf8', env: { ...process.env, PATH: `${bin}${path.delimiter}${process.env.PATH ?? ''}`, + // ⛔ Overridden rather than inherited: under CI this variable is already + // set, and a checker that appended there would both pollute the real run + // summary and make the assertions below read the wrong file. + GITHUB_STEP_SUMMARY: summaryFile, }, }); return { @@ -298,6 +320,7 @@ function runChecker(stub: { stdout?: string; stderr?: string; exit: number }): C stdout: proc.stdout, stderr: proc.stderr, pnpmArgv: fs.existsSync(argvFile) ? fs.readFileSync(argvFile, 'utf8') : null, + stepSummary: fs.readFileSync(summaryFile, 'utf8'), }; } finally { fs.rmSync(dir, { recursive: true, force: true }); @@ -371,6 +394,25 @@ describe('the lockfile-dedupe checker', () => { expect(run.stderr).toContain('VERDICT could not take a reading'); }); + it('⛔ the instruction to commit a dedupe is gone from the finding — in BOTH modes', () => { + // The sentence objectui#9562 reported: a confident red that told the reader + // to `pnpm dedupe` and commit, to a lockfile every open pull request shares. + // The ruling removes it from the blocking mode too, so this leg is measured + // WITHOUT the flag as well — a fix that only reached the new mode would let + // the bad advice survive wherever the hard verdict is still read. + for (const args of [[], ['--report-only']]) { + const run = runChecker({ stdout: PNPM_RED, exit: 1 }, args); + const report = `${run.stdout}${run.stderr}`; + expect(run.pnpmArgv, `the stub did not serve the ${args.join(' ') || 'bare'} run`).toBe('dedupe --check'); + expect(report, 'the removed instruction is back').not.toContain('Fix it HERE'); + // …and what replaced it names the instrument rather than a remedy. + expect(report).toContain('RE-RUN this check'); + expect(report).toContain('ONLY when'); + // Anti-vacuity: the leg really did produce the finding it is reading. + expect(report).toContain('VERDICT not deduped'); + } + }); + 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 @@ -385,6 +427,78 @@ describe('the lockfile-dedupe checker', () => { }); }); +describe('--report-only — the mode objectui#9562 ruled (letter A, comment 5717182406)', () => { + it('⭐ a not-deduped verdict warns, summarises the split, and exits 0', () => { + const run = runChecker({ stdout: PNPM_RED, exit: 1 }, ['--report-only']); + expect(run.pnpmArgv, 'the stub did not serve this run — a live pnpm reached the registry').toBe('dedupe --check'); + + // The relaxation itself. + expect(run.status, 'the ruled mode exits 0 — the verdict reports, it does not fail').toBe(0); + expect(run.stderr).toContain('::warning title=Lockfile dedupe::'); + expect(run.stderr, 'an error annotation still fails the Files tab reading').not.toContain('::error'); + + // ⛔ and it is a relaxation of the CONSEQUENCE, not of the reading: the same + // verdict, naming the same split, is still printed. + expect(run.stderr).toContain('VERDICT not deduped'); + expect(run.stderr).toContain('zod'); + expect(run.stderr).toContain('@objectstack/spec'); + // A green exit with a warning needs to say so, or it reads as a near-miss. + expect(run.stderr).toContain('REPORT ONLY'); + + // The step-summary block, which is the other half of "reports". + expect(run.stepSummary, 'the ruled step summary was not written').toContain('not deduped'); + expect(run.stepSummary).toContain('zod'); + expect(run.stepSummary).toContain('@objectstack/spec'); + // It carries the instrument caveat too, from the checker's single copy. + expect(run.stepSummary).toContain('RE-RUN this check'); + }); + + it('⭐ leg E survives the flag: "could not look" stays distinguishable from "clean"', () => { + // The thing this change may NOT erase. Under the flag both outcomes exit 0, + // so the exit code no longer separates them — something else has to, and + // these two runs are read against each other rather than in isolation. + const down = runChecker({ stderr: PNPM_REGISTRY_DOWN, exit: 1 }, ['--report-only']); + const clean = runChecker({ stdout: PNPM_GREEN, exit: 0 }, ['--report-only']); + expect(down.pnpmArgv).toBe('dedupe --check'); + expect(clean.pnpmArgv).toBe('dedupe --check'); + + expect(down.status, 'the registry being down may not fail a pull request').toBe(0); + expect(clean.status).toBe(0); + + // ⇒ the discriminators, each asserted in BOTH directions. + expect(down.stderr).toContain('VERDICT could not take a reading'); + expect(down.stderr).toContain('Nothing here was judged'); + expect(`${down.stdout}${down.stderr}`, 'a failed reading reported as a deduped tree').not.toContain( + 'VERDICT deduped', + ); + expect(down.stderr, 'nothing was judged, so nothing may be reported as a finding').not.toContain( + 'VERDICT not deduped', + ); + expect(down.stderr, 'the annotation is the only signal left once the exit code is 0').toContain( + '::warning title=Lockfile dedupe::', + ); + expect(down.stepSummary).toContain('could not take a reading'); + + expect(clean.stdout).toContain('VERDICT deduped'); + expect(clean.stderr, 'a clean tree may not raise an annotation of any kind').not.toContain('::warning'); + expect(clean.stderr).not.toContain('::error'); + expect(clean.stepSummary, 'a clean tree writes no step summary — the block itself is the signal').toBe(''); + }); + + it('CONTROL — the BARE script keeps its exit codes, which is what the flag is for', () => { + // The ruling grants ONE explicit flag and keeps the hand-run contract. This + // is also the control for the three legs above: without it, a checker that + // had simply stopped failing would pass them all. + expect(runChecker({ stdout: PNPM_RED, exit: 1 }).status, 'the bare script stopped reporting findings').toBe(1); + expect(runChecker({ stderr: PNPM_REGISTRY_DOWN, exit: 1 }).status, 'the bare script stopped reporting 2').toBe(2); + expect(runChecker({ stdout: PNPM_GREEN, exit: 0 }).status).toBe(0); + // ⛔ And the mode is never inferred: an unrelated argument must not turn it on. + const unrelated = runChecker({ stdout: PNPM_RED, exit: 1 }, ['--verbose']); + expect(unrelated.pnpmArgv).toBe('dedupe --check'); + expect(unrelated.status, 'report-only was entered by something other than its own flag').toBe(1); + }); +}); + describe('the lockfile-dedupe gate is wired the way its header claims', () => { const workflows = readWorkflows(); const workflow = workflows.find((w) => w.file === WORKFLOW); @@ -454,6 +568,15 @@ describe('the lockfile-dedupe gate is wired the way its header claims', () => { expect(takesLiveReading(maskComments('const dedupe = new Set();'))).toBe(false); }); + it('passes `--report-only`, which is where objectui#9562’s ruling actually lands', () => { + // Comment-stripped: the header explains the mode at length, and prose may + // not satisfy wiring. The flag on the step is the entire deliverable — the + // script supports both modes, so the workflow is what chooses one. + expect(body(), 'the gate is blocking again — objectui#9562, undone').toContain( + `node ${SCRIPT} --report-only`, + ); + }); + 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); @@ -476,10 +599,13 @@ describe('the lockfile-dedupe gate is wired the way its header claims', () => { expect(body()).not.toMatch(/^\s*run:.*corepack/m); }); - it('is classified as a BLOCKING (optional) context, not an alarm', () => { - // ⚠️ The load-bearing assertion. Moving this name to NOT_A_GATE is a real - // option — the workflow header documents it as a one-line flip — but it is - // a decision about what may auto-merge, so it fails here first. + it('stays an OPTIONAL context, because the JOB still gates even though its verdict does not', () => { + // ⚠️ The load-bearing assertion, and objectui#9562 did NOT move it: the + // ruling relaxed the VERDICT, not the job. A failed checkout, a broken + // `ci-setup-pnpm.sh` or a `--self-test` that stops passing still reds this + // context and still stops a Dependabot auto-merge — so the classification + // is live rather than vestigial, and moving this name to NOT_A_GATE remains + // a separate decision about what may auto-merge, which fails here first. expect(Object.keys(OPTIONAL_CONTEXTS)).toContain(CONTEXT); expect(Object.keys(NOT_A_GATE)).not.toContain(CONTEXT); // And it may not be REQUIRED while the path filter stands (objectui#3523). diff --git a/scripts/check-lockfile-dedupe.mjs b/scripts/check-lockfile-dedupe.mjs index 894c56bccb..55c986d823 100644 --- a/scripts/check-lockfile-dedupe.mjs +++ b/scripts/check-lockfile-dedupe.mjs @@ -6,8 +6,36 @@ * `pnpm dedupe` on it may not collapse anything (objectui#8333). * * Run: node scripts/check-lockfile-dedupe.mjs (`pnpm check:lockfile-dedupe`) + * node scripts/check-lockfile-dedupe.mjs --report-only (what CI passes) * node scripts/check-lockfile-dedupe.mjs --self-test * + * ## `--report-only`: the gate REPORTS on pull requests (objectui#9562) + * + * objectui#9562 measured this gate returning four green and one red on a + * byte-identical `pnpm-lock.yaml` inside ninety minutes, the red printing a + * confident `VERDICT not deduped` that no lockfile edit and no registry publish + * explained — and then instructing the reader to commit a dedupe the tree did + * not need, to a lockfile every open pull request shares. The maintainer ruled + * (2026-09-17, letter A) that a gate which is non-deterministic by construction + * does not BLOCK: + * + * - `--report-only` keeps every reading exactly as it is and changes only what + * the process does with it: annotations become `::warning::`, the same text + * is written to `$GITHUB_STEP_SUMMARY`, and the process exits 0. It is the + * flag `lockfile-dedupe.yml` passes. + * - The BARE script keeps its exit codes — 0 / 1 / 2 — for whoever runs it by + * hand. ⛔ That is deliberate and not an oversight to tidy away: the hard + * verdict is still how a human asks this question. + * - ⛔ Report-only may never flatten the three answers into two. `clean` and + * `cannot-run` both exit 0 under the flag, so the EXIT CODE no longer tells + * them apart — the annotation does, and `cannot-run` carries one precisely + * because of that. A report-only mode that cannot say "I could not look" is + * worse than the blocking gate it replaces. + * - The finding text, in BOTH modes, now says what the instrument is: re-run + * before acting, and dedupe only when the split reproduces. ⛔ The old + * unconditional "fix it HERE by running `pnpm dedupe` and committing the + * lockfile" is gone from both — it is the sentence objectui#9562 reported. + * * ## What this is for, and why it is not objectui#8326's gate * * objectui#8326's `check-lockfile-integrity.mjs` reports a DELTA: it compares a @@ -79,6 +107,7 @@ */ import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -98,6 +127,31 @@ export const EXIT_FINDINGS = 1; /** A reading that could not be taken is NOT a deduped lockfile. */ export const EXIT_CANNOT_RUN = 2; +/** + * The one explicit flag objectui#9562's ruling grants, passed by + * `lockfile-dedupe.yml`. ⛔ Not inferred from `process.env.CI` or from + * `GITHUB_ACTIONS`: a mode that changes a verdict's consequences is a decision + * the caller states, so that running the checker by hand cannot silently get a + * different contract than the one being read here. + */ +export const REPORT_ONLY_FLAG = '--report-only'; + +/** + * What this gate IS, said wherever it reports a finding — in both modes and on + * both surfaces (log and step summary), from ONE copy so the two cannot drift. + * + * ⛔ This is not decoration. The defect objectui#9562 recorded is not that the + * gate reds; it is that a confident red arrives with a remedy attached, and the + * remedy writes to a file every open pull request shares. + */ +export const INSTRUMENT_CAVEAT = Object.freeze([ + '⚠️ WHAT THIS GATE IS: a LIVE registry reading, and it can disagree with itself. objectui#9562', + 'recorded four green and one red on a byte-identical `pnpm-lock.yaml` inside ninety minutes, the', + 'red naming a split that no lockfile edit and no registry publish explained.', + '⇒ RE-RUN this check before acting on it, and run `pnpm dedupe` and commit the lockfile ONLY when', + 'the split reproduces. ⛔ One red is not by itself evidence that this tree needs a dedupe.', +]); + /** * The verdict, from pnpm's exit status plus the sentinel. * @@ -141,10 +195,24 @@ export function namesFromOutput(output = '') { } /** - * @param {{ outcome: 'clean'|'findings'|'cannot-run', names?: string[], output?: string }} reading + * The line that tells the reader this job will not fail, printed ONLY under the + * flag. Without it a `::warning::` on a green check reads as an unexplained + * near-miss. + * + * @param {boolean} reportOnly + * @returns {string[]} + */ +function modeLine(reportOnly) { + return reportOnly + ? ['⚠️ REPORT ONLY — this job exits 0 and does not fail your pull request (objectui#9562).'] + : []; +} + +/** + * @param {{ outcome: 'clean'|'findings'|'cannot-run', names?: string[], output?: string, reportOnly?: boolean }} reading * @returns {string[]} lines to print */ -export function renderVerdict({ outcome, names = [], output = '' }) { +export function renderVerdict({ outcome, names = [], output = '', reportOnly = false }) { if (outcome === 'clean') { return ['VERDICT deduped — `pnpm dedupe` would collapse nothing in this lockfile.']; } @@ -153,6 +221,7 @@ export function renderVerdict({ outcome, names = [], output = '' }) { 'VERDICT could not take a reading — `pnpm dedupe --check` did not report a result.', 'This is a fact about the run (registry, toolchain, or a changed pnpm report format),', 'NOT a verdict on this change. Nothing here was judged.', + ...modeLine(reportOnly), ...(output ? ['', '--- pnpm output ---', output.trimEnd()] : []), ]; } @@ -161,21 +230,95 @@ export function renderVerdict({ outcome, names = [], output = '' }) { output.trimEnd(), '', `VERDICT not deduped — \`pnpm dedupe\` would still collapse duplicate copies: ${named}.`, - 'Two physical copies of one package are two real paths in the bundle. Fix it HERE, in this', - 'pull request, by running `pnpm dedupe` and committing the lockfile — so that a downstream', - '`Bundle Analysis` reading measures this change and not resolution collateral left behind by', - 'an earlier one (objectui#8333).', + ...modeLine(reportOnly), + 'Two physical copies of one package are two real paths in the bundle, and a downstream', + '`Bundle Analysis` reading then measures resolution collateral left behind by an earlier', + 'change rather than measuring this one (objectui#8333).', + '', + ...INSTRUMENT_CAVEAT, + '', '⛔ Do NOT satisfy this by pinning a version, adding a `pnpm.overrides` entry or widening a', 'range: those spend a declaration to fix a resolution artefact, and objectui#8333 rejected', 'both. `pnpm dedupe` changes no declaration at all.', ]; } +/** + * The same reading, rendered for `$GITHUB_STEP_SUMMARY` instead of the log. + * + * Empty for a clean tree: a gate with nothing to report writes nothing, so the + * presence of a block is itself the signal on the run summary page. + * + * @param {{ outcome: 'clean'|'findings'|'cannot-run', names?: string[], output?: string }} reading + * @returns {string} markdown, or '' when there is nothing to say + */ +export function renderStepSummary({ outcome, names = [], output = '' }) { + if (outcome === 'clean') return ''; + if (outcome === 'cannot-run') { + return [ + '### ⚠️ Lockfile dedupe — could not take a reading', + '', + '`pnpm dedupe --check` did not report a result, so **nothing here was judged**. That is a fact', + 'about the run — registry, toolchain, or a changed pnpm report format — and not a verdict on', + 'this change. ⛔ It is NOT a deduped lockfile either.', + '', + ...INSTRUMENT_CAVEAT, + '', + ].join('\n'); + } + const named = names.length + ? names.map((name) => `- \`${name}\``) + : ['- _(pnpm named no removable identity — see the job log)_']; + return [ + '### ⚠️ Lockfile dedupe — not deduped', + '', + '`pnpm dedupe` would still collapse duplicate copies of:', + '', + ...named, + '', + ...INSTRUMENT_CAVEAT, + '', + '```text', + output.trimEnd(), + '```', + '', + ].join('\n'); +} + +/** + * Append a step-summary block, when the runner gave us somewhere to put it. + * + * ⛔ Never throws and never changes a verdict: a summary that could not be + * written is a worse report, not a different reading. + * + * @param {string} text + * @param {NodeJS.ProcessEnv} [env] + * @returns {boolean} whether anything was written + */ +export function writeStepSummary(text, env = process.env) { + const target = env.GITHUB_STEP_SUMMARY; + if (!target || !text) return false; + try { + fs.appendFileSync(target, `${text}\n`); + return true; + } catch (error) { + console.error( + `::warning title=Lockfile dedupe::could not write the step summary: ${error instanceof Error ? error.message : String(error)}`, + ); + return false; + } +} + /** * @param {string[]} argv * @returns {number} process exit code */ export function main(argv = process.argv.slice(2)) { + const reportOnly = argv.includes(REPORT_ONLY_FLAG); + /** The annotation LEVEL is the whole difference: same message, same surface. */ + const annotate = (message) => + console.error(`::${reportOnly ? 'warning' : 'error'} title=Lockfile dedupe::${message}`); + const run = spawnSync('pnpm', ['dedupe', '--check'], { cwd: REPO_ROOT, encoding: 'utf8', @@ -183,11 +326,10 @@ export function main(argv = process.argv.slice(2)) { }); if (run.error) { - console.error( - `::error title=Lockfile dedupe::could not run \`pnpm dedupe --check\`: ${run.error.message}`, - ); - for (const line of renderVerdict({ outcome: 'cannot-run' })) console.error(line); - return EXIT_CANNOT_RUN; + annotate(`could not run \`pnpm dedupe --check\`: ${run.error.message}`); + for (const line of renderVerdict({ outcome: 'cannot-run', reportOnly })) console.error(line); + writeStepSummary(renderStepSummary({ outcome: 'cannot-run' })); + return reportOnly ? EXIT_CLEAN : EXIT_CANNOT_RUN; } // pnpm writes its report across both streams; the verdict reads them together. @@ -195,18 +337,32 @@ export function main(argv = process.argv.slice(2)) { const outcome = classify({ status: run.status, output }); const names = outcome === 'findings' ? namesFromOutput(output) : []; - const lines = renderVerdict({ outcome, names, output }); + const lines = renderVerdict({ outcome, names, output, reportOnly }); const write = outcome === 'clean' ? console.log : console.error; for (const line of lines) write(line); if (outcome === 'clean') return EXIT_CLEAN; - if (outcome === 'cannot-run') return EXIT_CANNOT_RUN; - console.error( - `::error title=Lockfile dedupe::${names.join(', ') || 'duplicate copies'} — run \`pnpm dedupe\` and commit the lockfile. ` + - 'This is resolution collateral, not a bundle regression (objectui#8333).', + writeStepSummary(renderStepSummary({ outcome, names, output })); + + if (outcome === 'cannot-run') { + // ⛔ Under the flag the exit code no longer separates "clean" from "could + // not look" — both are 0 — so the ANNOTATION has to, and this is the only + // place it can come from. Without it, report-only would answer a question + // it never asked. + if (!reportOnly) return EXIT_CANNOT_RUN; + annotate( + 'could not take a reading — `pnpm dedupe --check` did not report a result, so nothing here ' + + 'was judged. ⛔ This is NOT a deduped verdict.', + ); + return EXIT_CLEAN; + } + + annotate( + `${names.join(', ') || 'duplicate copies'} — a live registry reading says \`pnpm dedupe\` would collapse these. ` + + 'Re-run before acting; dedupe and commit the lockfile only if the split reproduces (objectui#8333, objectui#9562).', ); - return EXIT_FINDINGS; + return reportOnly ? EXIT_CLEAN : EXIT_FINDINGS; } /** @@ -281,6 +437,49 @@ export function selfTest() { renderVerdict({ outcome: 'cannot-run' }).join('\n').includes('Nothing here was judged'), ); + // ── objectui#9562: the finding text names the INSTRUMENT, in both modes ─── + for (const reportOnly of [false, true]) { + const label = reportOnly ? 'report-only' : 'blocking'; + const finding = renderVerdict({ outcome: 'findings', names, output: RED, reportOnly }).join('\n'); + t(`${label} mode: the finding says to re-run before acting`, finding.includes('RE-RUN this check')); + t( + `${label} mode: ⛔ the unconditional instruction to commit a dedupe is GONE`, + !finding.includes('Fix it HERE'), + ); + t(`${label} mode: the finding still names the packages`, finding.includes('zod')); + } + + t( + 'only report-only mode says the job will not fail', + renderVerdict({ outcome: 'findings', names, output: RED, reportOnly: true }).join('\n').includes('REPORT ONLY') && + !renderVerdict({ outcome: 'findings', names, output: RED }).join('\n').includes('REPORT ONLY'), + ); + t( + 'report-only could-not-run says it will not fail, and still says it judged nothing', + renderVerdict({ outcome: 'cannot-run', reportOnly: true }).join('\n').includes('REPORT ONLY') && + renderVerdict({ outcome: 'cannot-run', reportOnly: true }).join('\n').includes('Nothing here was judged'), + ); + + // ── the step summary: same reading, other surface ──────────────────────── + t('a clean tree writes NO step summary — the block itself is the signal', renderStepSummary({ outcome: 'clean' }) === ''); + const summary = renderStepSummary({ outcome: 'findings', names, output: RED }); + t('the step summary names the split', names.every((name) => summary.includes(name))); + t('the step summary carries pnpm\'s own output', summary.includes(DEDUPE_SENTINEL)); + t('the step summary carries the instrument caveat, from the one copy', summary.includes(INSTRUMENT_CAVEAT[0])); + const cannotSummary = renderStepSummary({ outcome: 'cannot-run' }); + t( + 'the could-not-run summary stays distinguishable from a deduped one', + cannotSummary.includes('could not take a reading') && !cannotSummary.includes('not deduped'), + ); + t( + 'CONTROL — a finding summary still renders when the parse names nothing', + renderStepSummary({ outcome: 'findings', names: [], output: RED }).includes('not deduped'), + ); + t( + 'the step summary is written only where the runner asked for one', + writeStepSummary(summary, {}) === false && writeStepSummary('', { GITHUB_STEP_SUMMARY: '/dev/null' }) === false, + ); + const failed = cases.filter((c) => !c.ok); for (const c of failed) console.error(` ✗ ${c.name}`); if (failed.length) { @@ -289,23 +488,29 @@ export function selfTest() { } console.log( `✓ check-lockfile-dedupe self-test: ${cases.length} cases pass ` + - '(the three verdicts, the crash-vs-finding split, and the controls a name-parser-driven gate would fail).', + '(the three verdicts, the crash-vs-finding split, the report-only rendering, and the controls ' + + 'a name-parser-driven gate would fail).', ); return 0; } if (isEntrypoint(import.meta.url)) { - if (process.argv.includes('--self-test')) { + const argv = process.argv.slice(2); + if (argv.includes('--self-test')) { process.exitCode = selfTest(); } else { try { - process.exitCode = main(); + process.exitCode = main(argv); } catch (error) { + // ⛔ The catch-all obeys the flag too. A mode that reports every measured + // outcome and then reds on a crash of the reporter is still a gate that + // fails a pull request for a reason that is not about it. + const reportOnly = argv.includes(REPORT_ONLY_FLAG); console.error( - `::error::check-lockfile-dedupe could not take a reading: ${error instanceof Error ? error.message : String(error)}`, + `::${reportOnly ? 'warning' : 'error'}::check-lockfile-dedupe could not take a reading: ${error instanceof Error ? error.message : String(error)}`, ); console.error('A reading that could not be taken is NOT a deduped lockfile.'); - process.exitCode = EXIT_CANNOT_RUN; + process.exitCode = reportOnly ? EXIT_CLEAN : EXIT_CANNOT_RUN; } } }