From 43d5d4f4b55924310eacd8b32f322c4b7aa8bd70 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 10:44:54 +0000 Subject: [PATCH 1/2] ci(deps): require the committed lockfile to be deduped A dependency bump can re-resolve part of the peer graph and fork a package that was single-copy, without changing any declaration, range or override. `Bundle Analysis` then reads a bundle that grew and attributes the growth to the bump. Measured on objectui#8333: bumping `better-auth` alone splits `zod` into a third physical copy and pulls `@objectstack/spec` and `ai` into a second peer context each. The remedy is `pnpm dedupe`, but it is not surgical: run on a tree carrying old duplication it collapses all of it, and the reviewer reads the combined delta as the bump's -- the same misattribution aimed at a different pull request. So the remedy is only honest once the tree is already deduped, which is why the ruling was "B then A": objectui#9215 paid the accumulated debt down first, and this is the second half. New gate `scripts/check-lockfile-dedupe.mjs` runs `pnpm dedupe --check` and requires the committed lockfile to have nothing left to collapse. Its verdict is pnpm's exit code, not a parse of pnpm's output; the output is read only to name packages in the annotation, and to tell a finding apart from a crash (both exit non-zero), so a reading that could not be taken exits 2 and is never a pass. `Lockfile Dedupe Check` is classified in `OPTIONAL_CONTEXTS`, so a red stops a Dependabot auto-merge. Its neighbour `Lockfile Integrity Check` stays in `NOT_A_GATE`, untouched: that gate names a duplication and leaves the answer open, a judgement call its header reserves for the maintainer, while this one has a single mechanical remedy that pnpm itself prints. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FhBNJcLRZLe8M87VcUgpKr --- .changeset/8333-lockfile-dedupe-gate.md | 13 + .github/workflows/lockfile-dedupe.yml | 158 +++++++++ content/docs/guide/ci-cd-pipeline.md | 55 ++++ package.json | 1 + .../__tests__/check-lockfile-dedupe.test.ts | 128 +++++++ scripts/check-lockfile-dedupe.mjs | 311 ++++++++++++++++++ scripts/dependabot-merge-gate.mjs | 2 + 7 files changed, 668 insertions(+) create mode 100644 .changeset/8333-lockfile-dedupe-gate.md create mode 100644 .github/workflows/lockfile-dedupe.yml create mode 100644 scripts/__tests__/check-lockfile-dedupe.test.ts create mode 100644 scripts/check-lockfile-dedupe.mjs diff --git a/.changeset/8333-lockfile-dedupe-gate.md b/.changeset/8333-lockfile-dedupe-gate.md new file mode 100644 index 0000000000..89d42d4c02 --- /dev/null +++ b/.changeset/8333-lockfile-dedupe-gate.md @@ -0,0 +1,13 @@ +--- +--- + +Require the committed `pnpm-lock.yaml` to be deduped (objectui#8333). + +New gate `scripts/check-lockfile-dedupe.mjs` + `.github/workflows/lockfile-dedupe.yml` runs +`pnpm dedupe --check` on pull requests that touch the lockfile, and the Dependabot merge gate +classifies the resulting `Lockfile Dedupe Check` as a blocking context. A dependency bump can +re-resolve part of the peer graph and fork a single-copy package without changing any declaration, +range or override; `Bundle Analysis` then reads the growth as the bump's. This makes +`pnpm dedupe` a requirement of the bump pull request instead, so that reading is attributable. + +CI, scripts and docs only; no package is released by this change. diff --git a/.github/workflows/lockfile-dedupe.yml b/.github/workflows/lockfile-dedupe.yml new file mode 100644 index 0000000000..27de854534 --- /dev/null +++ b/.github/workflows/lockfile-dedupe.yml @@ -0,0 +1,158 @@ +name: Lockfile Dedupe + +# objectui#8333 — the committed `pnpm-lock.yaml` must already be DEDUPED, i.e. +# `pnpm dedupe` on it may not collapse anything. The mechanism, what it catches +# and what its green does NOT mean are documented at length in +# `scripts/check-lockfile-dedupe.mjs`. +# +# ── Why this exists next to `lockfile-integrity.yml` rather than inside it ──── +# +# They answer different questions. objectui#8326's gate reports a DELTA against +# the merge base — "did THIS change duplicate something?" — and needs two +# revisions of one file to do it. This one reports a PROPERTY of a single tree — +# "would `pnpm dedupe` still have work to do here?" — and needs no base at all, +# but does need pnpm and the registry. Different subject, different cost, +# different failure modes, so they are separate jobs and each stays readable. +# +# ── Why it is path-filtered ────────────────────────────────────────────────── +# +# Its subject is one file. A pull request that does not touch `pnpm-lock.yaml` +# cannot change whether that lockfile is deduped, so the filter costs no +# coverage. A manifest edit that WOULD change the resolution cannot reach `main` +# without moving the lockfile either, because `ci.yml` installs with +# `--frozen-lockfile` and fails first. Same argument shape as its neighbour. +# +# The filter also lists this gate's RUNTIME CLOSURE — the files the job actually +# executes — which is this repository's convention for a path-filtered gate +# (`performance-budget.yml` states it at length): without that, a change to the +# checker itself ships having never run once, and the wiring bug then surfaces on +# somebody else's next lockfile PR, reading to them as a problem of their own. +# +# ⚠️ 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`. +# +# ── ⚠️ It IS a blocking context, and that is a decision, 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: +# +# * #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 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. +# +# ⇒ ⚠️ 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. +# +# ── No `merge_group` trigger ───────────────────────────────────────────────── +# +# Deliberate, and the same shape as `lockfile-integrity.yml` and +# `performance-budget.yml`: under objectui#3523 a required context that does not +# report on a queue build stalls the queue until the ruleset timeout fails it, so +# a path-filtered, non-required gate must not subscribe one. + +on: + pull_request: + branches: [main, develop] + paths: + - 'pnpm-lock.yaml' + # This gate's runtime closure — the files this job executes. An edit to any + # of them must be able to red this gate on its OWN pull request. + - '.github/workflows/lockfile-dedupe.yml' + - 'scripts/check-lockfile-dedupe.mjs' + # `isEntrypoint` decides whether `main()` runs at all, so a regression in it + # makes this checker exit 0 having measured nothing — the exact silent-green + # failure the rest of this gate exists to prevent. Same reason + # `performance-budget.yml` and `half-state-patrol.yml` list it. + - 'scripts/invoked-as.mjs' + # The toolchain step this job runs before pnpm is on PATH. + - 'scripts/ci-setup-pnpm.sh' + workflow_dispatch: + +concurrency: + group: lockfile-dedupe-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + lockfile-dedupe: + name: Lockfile Dedupe Check + runs-on: ubuntu-latest + + # ── The job ceiling, DERIVED FOR THIS JOB ──────────────────────────────── + # + # ⛔ NOT inherited from `lockfile-integrity.yml`'s 5 or `ci.yml`'s ladder — + # objectui#7048 fences exactly that. The arithmetic below is this job's own, + # and its weakness is stated rather than hidden. + # + # - Population: `pnpm dedupe --check` on a clean `origin/main` worktree, + # n=3 consecutive runs, pnpm 10.31.0 / Node 22.22.2, WARM pnpm metadata + # cache, on a developer box and not a runner: 22.8s / 22.9s / 23.2s, all + # exit 0. Spread 0.4s, so the command itself is not variable. + # - ⚠️ That population is the wrong one for a ceiling and is known to be: + # this workflow has no run history yet, and a runner starts with a COLD + # metadata cache, which is the dominant term. So the max is not measured + # but BOUNDED, pessimistically, at 5 minutes for checkout + Node + the + # Corepack fetch + a cold resolve of ~1,767 identities. + # + # Ceiling = the smallest round number that is both >= 3x that bound (15min) + # and >= bound + 15min (20min); the second leg binds => 20. + # + # ⚠️ Re-derive this from real runs once this workflow has a history — the + # instrument is the Actions jobs endpoint (`completed_at` minus + # `started_at`, per job, never the run total), the same one + # `performance-budget.yml` names. ⛔ Do not copy the number above forward as + # if it were a measurement of this job in CI; it is a bound on one. + timeout-minutes: 20 + + steps: + # No `fetch-depth: 0` here, unlike its neighbour: this gate's subject is a + # single tree, not a delta, so it needs no merge base and no history. + - name: Checkout code + uses: actions/checkout@v7 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22.x' + # No `cache: 'pnpm'` — that action's store probe shells out to pnpm + # before the step below has put it on PATH (objectui#8099 documents + # that exact ordering trap), and this job runs no `pnpm install`, so + # there is no node_modules for a cache to pay for. + + - name: Enable Corepack and download the pinned pnpm + run: bash scripts/ci-setup-pnpm.sh + + - name: Verify pnpm version + run: pnpm --version + + # The checker's own cases, before the checker is trusted to judge anything. + - name: Self-test the checker + run: node scripts/check-lockfile-dedupe.mjs --self-test + + # 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). + - name: Check the committed lockfile is deduped + run: node scripts/check-lockfile-dedupe.mjs diff --git a/content/docs/guide/ci-cd-pipeline.md b/content/docs/guide/ci-cd-pipeline.md index 026e367835..3be7823692 100644 --- a/content/docs/guide/ci-cd-pipeline.md +++ b/content/docs/guide/ci-cd-pipeline.md @@ -46,6 +46,7 @@ one has its own section below. | `governed-surface-guard.yml` | Governed Surface Queue Guard | PR to `main`, `develop` (incl. `ready_for_review`) — **no path filter**; merge-queue builds | **Yes on a queue build only** — a governed-surface diff with no authorized approval record (on any commit) is refused there, and so is any merge group whose queued pull requests still carry `needs:contract-review`; on the pull request itself it is deliberately green and prints an early warning | | `performance-budget.yml` | Bundle Analysis | Push / PR touching `packages/**`, `apps/console/**`, `pnpm-lock.yaml` | **Yes** — the console entry gzip budget | | `lockfile-integrity.yml` | Lockfile Integrity Check | PR to `main`, `develop` touching `pnpm-lock.yaml` or the gate's own two files; manual | No — **deliberately not a blocking context** ([#8326](https://github.com/objectstack-ai/objectui/issues/8326)); it names the packages and the Dependabot merge gate classifies it `NOT_A_GATE` | +| `lockfile-dedupe.yml` | Lockfile Dedupe Check | PR to `main`, `develop` touching `pnpm-lock.yaml` or the gate's own runtime closure; manual | **Yes, when it runs** — classified `OPTIONAL_CONTEXTS`, so a red stops a Dependabot auto-merge ([#8333](https://github.com/objectstack-ai/objectui/issues/8333)) | | `live-e2e.yml` | Live E2E (informational) | PR to `main`, `develop` (code paths); nightly cron `30 6 * * *`; manual | No — informational lane: not in the required-check set, and it declares no `merge_group` trigger | | `labeler.yml` | Auto Label PRs | PR `opened`, `synchronize`, `reopened` | No | | `dependabot-auto-merge.yml` | Dependabot Auto-merge | PR to `main`/`develop` authored by `dependabot[bot]` | No — but it gates *its own* merge, and goes red instead of merging when the check set is not green | @@ -1857,6 +1858,60 @@ build-tooling churn. And it says nothing about bytes; `Bundle Analysis` measures that filter stands. Enrolling it is a maintainer decision with its own cost, written up on #8326's pull request as input. +## Lockfile Dedupe (`lockfile-dedupe.yml`) + +**Trigger:** Pull requests to `main` and `develop` that touch `pnpm-lock.yaml`, this gate's own +runtime closure (`scripts/check-lockfile-dedupe.mjs`, `scripts/invoked-as.mjs`, +`scripts/ci-setup-pnpm.sh`) or the workflow file itself, plus manual dispatch. It appears in the +checks list as **Lockfile Dedupe Check**. + +Runs `scripts/check-lockfile-dedupe.mjs`, which shells out to `pnpm dedupe --check` and requires the +committed lockfile to be **already deduped** — that is, `pnpm dedupe` must have nothing left to +collapse. + +**How it differs from its neighbour.** `Lockfile Integrity Check` reports a **delta** against the +merge base ("did *this* change duplicate something?") and needs two revisions of one file. +This gate reports a **property of one tree** ("would `pnpm dedupe` still have work to do here?") and +needs no base — but it does need pnpm and the registry. The two are independent: a tree can be +delta-clean and still carry years of accumulated duplication, which is the state `main` was in until +[#9215](https://github.com/objectstack-ai/objectui/issues/9215) collapsed 47 identities out of it. + +**Why it exists.** [#8333](https://github.com/objectstack-ai/objectui/issues/8333) measured the +sequence: a dependency bump re-resolves part of the peer graph and forks a package that was +single-copy — no declaration, range or override anywhere in the workspace changes — and +`Bundle Analysis` then reads a bundle that grew and attributes the growth to the bump. The remedy is +`pnpm dedupe`, but `pnpm dedupe` is **not surgical**: run on a tree carrying old duplication it +collapses all of it, and the reviewer reads the combined delta as the bump's — the same +misattribution, aimed at a different pull request. So the remedy is only honest once the tree is +already deduped, which is why #8333's ruling was **B then A**: pay the accumulated debt down in its +own dedicated pull request first (#9215), then require the remedy in bump pull requests. This gate is +that second half, and it is also what keeps the first half true. + +**It IS a blocking context**, and that is a decision rather than a default. +`scripts/dependabot-merge-gate.mjs` classifies it in `OPTIONAL_CONTEXTS`, so a red stops a Dependabot +auto-merge; its path filter makes it unrequirable under +[#3523](https://github.com/objectstack-ai/objectui/issues/3523)'s rule, the same terms +`Bundle Analysis` is enrolled on. It blocks where `Lockfile Integrity Check` deliberately does not +because the two differ in **remedy**: #8326's gate names a duplication and leaves the answer open +(re-lock, pin, or accept), a judgement call its header reserves for the maintainer, while this gate +has exactly one mechanical remedy and pnpm prints it — run `pnpm dedupe` and commit the lockfile, +changing no declaration, range or override. To stop it blocking, move the name to `NOT_A_GATE`; +`scripts/__tests__/check-lockfile-dedupe.test.ts` fails on that demotion so it has to be taken +deliberately. + +**⛔ What its green does not mean.** It does not say the lockfile is *minimal* — packages whose +ranges genuinely do not overlap keep their separate copies and are not findings. It does not measure +bytes; `Bundle Analysis` does. And it is not a check that drifts with the registry: `pnpm dedupe` +collapses copies already in the tree rather than chasing the newest version a range admits, which +#8333 measured directly (`better-auth` declared `^1.7.2`, locked at `1.7.2`, with `1.7.4` published +and admitted by that range — and this gate green on that tree). + +**⛔ What it does not do.** It does not run `pnpm dedupe` for you, and it never edits the lockfile — +`--check` reports without writing. A finding is fixed in the pull request that caused it, by running +the command and committing the result. ⛔ Pinning a version, adding a `pnpm.overrides` entry or +widening a range are **not** acceptable ways to satisfy it: #8333 rejected all three, on the grounds +that they spend a declaration to fix a resolution artefact. + ## Link Checking (`check-links.yml`) **Trigger:** Weekly cron (`17 4 * * 0` — Sundays, off the top of the hour, when the scheduled-run diff --git a/package.json b/package.json index cdc05fa084..c577ee83b6 100644 --- a/package.json +++ b/package.json @@ -88,6 +88,7 @@ "governed": "node scripts/check-governed-queue-guard.mjs --test", "check:merge-queue-head": "node scripts/check-merge-queue-head.mjs --self-test", "check:lockfile-integrity": "node scripts/check-lockfile-integrity.mjs", + "check:lockfile-dedupe": "node scripts/check-lockfile-dedupe.mjs", "merge-queue-head": "node scripts/check-merge-queue-head.mjs", "check:pre-install-import-graph": "node scripts/check-pre-install-import-graph.mjs", "check:test-path-roots": "node scripts/check-test-path-roots.mjs", diff --git a/scripts/__tests__/check-lockfile-dedupe.test.ts b/scripts/__tests__/check-lockfile-dedupe.test.ts new file mode 100644 index 0000000000..e929ac7bdc --- /dev/null +++ b/scripts/__tests__/check-lockfile-dedupe.test.ts @@ -0,0 +1,128 @@ +/** + * 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: + * + * 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; + * 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. + * + * ⚠️ (3) is the half that rots silently. The partition test in + * `dependabot-merge-gate.test.ts` already fails if the name is classified + * NOWHERE — but it passes just as happily if a future change moves the name + * into `NOT_A_GATE`, which would turn this gate into an alarm without anything + * 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 fs from 'node:fs'; +import path from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +import { NOT_A_GATE, OPTIONAL_CONTEXTS, REQUIRED_CONTEXTS } from '../dependabot-merge-gate.mjs'; +import { pullRequestTrigger, readWorkflows, repoRoot, subscribesMergeGroup } from './workflow-checks.js'; + +const SCRIPT = 'scripts/check-lockfile-dedupe.mjs'; +const WORKFLOW = 'lockfile-dedupe.yml'; +const CONTEXT = 'Lockfile Dedupe Check'; + +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' }); + expect(run.status, `${SCRIPT} --self-test failed:\n${run.stdout}\n${run.stderr}`).toBe(0); + expect(run.stdout).toContain('self-test:'); + // Anti-vacuity: a self-test that asserted nothing would also exit 0. + 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); + 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); +}); + +describe('the lockfile-dedupe gate is wired the way its header claims', () => { + const workflows = readWorkflows(); + const workflow = workflows.find((w) => w.file === WORKFLOW); + /** The workflow with comment lines stripped — prose may not satisfy wiring. */ + const body = () => workflow!.lines.join('\n'); + + it('has a workflow that runs the checker', () => { + expect(workflow, `${WORKFLOW} not found — the gate would never run`).toBeDefined(); + // Comment-stripped throughout: a header comment mentioning the script must + // not be able to satisfy a wiring assertion about what the job RUNS. + expect(body()).toContain(`node ${SCRIPT}`); + expect(body()).toContain(`name: ${CONTEXT}`); + }); + + 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); + expect(trigger.filtered, 'a path filter is what keeps it out of the REQUIRED set').toBe(true); + // Without these the checker can ship having never run once, and the wiring + // bug then surfaces on somebody else's next lockfile pull request. + for (const file of ['pnpm-lock.yaml', `.github/workflows/${WORKFLOW}`, SCRIPT, 'scripts/invoked-as.mjs']) { + expect(body(), `${file} missing from the path filter`).toContain(file); + } + }); + + it('declares no merge_group trigger, per objectui#3523', () => { + // A path-filtered, non-required gate that subscribes the queue stalls it + // until the ruleset timeout. + expect(subscribesMergeGroup(workflow!)).toBe(false); + }); + + it('uses the shared pnpm setup rather than a bare corepack call', () => { + expect(body()).toContain('bash scripts/ci-setup-pnpm.sh'); + 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. + 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). + expect(REQUIRED_CONTEXTS as readonly string[]).not.toContain(CONTEXT); + }); + + it('the classification says why it blocks where its neighbour does not', () => { + const reason = OPTIONAL_CONTEXTS[CONTEXT as keyof typeof OPTIONAL_CONTEXTS]; + expect(reason).toBeTruthy(); + expect(reason).toContain('pnpm dedupe'); + // The neighbour's reservation must stay visible and untouched. + expect(Object.keys(NOT_A_GATE)).toContain('Lockfile Integrity Check'); + }); +}); diff --git a/scripts/check-lockfile-dedupe.mjs b/scripts/check-lockfile-dedupe.mjs new file mode 100644 index 0000000000..894c56bccb --- /dev/null +++ b/scripts/check-lockfile-dedupe.mjs @@ -0,0 +1,311 @@ +#!/usr/bin/env node +/** + * Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + * + * The committed `pnpm-lock.yaml` must already be DEDUPED: running + * `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 --self-test + * + * ## 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 + * pull request's lockfile with its merge base and names anything that gained a + * physical copy. It answers "did THIS change duplicate something?". + * + * This gate answers a different question, about one tree and no base: "is the + * lockfile in a state where `pnpm dedupe` would still have work to do?" The two + * are independent — a tree can be delta-clean and still carry years of + * accumulated duplication, which is exactly the state `main` was in until + * objectui#9215 collapsed 47 identities out of it. + * + * ## The defect this closes, in the order it happens + * + * 1. A dependency bump re-resolves part of the peer graph and forks a package + * that was single-copy. Measured on objectui#8333: bumping `better-auth` + * alone splits `zod` into a third physical copy and pulls `@objectstack/spec` + * and `ai` into a second peer context each. No declaration, range or override + * anywhere in the workspace changed — it is partial-re-resolution debt. + * 2. `Bundle Analysis` then reads a bundle that grew, and the growth is + * attributed to the bump. It is not the bump: it is the fork. + * 3. The remedy is `pnpm dedupe`. But `pnpm dedupe` is NOT SURGICAL — it + * collapses every collapsible duplicate in the tree, not just the one the + * bump created. Run inside a bump PR on a tree carrying old duplication, it + * drags all of that along, and the reviewer reads the combined delta as the + * bump's. That is the SAME misattribution, aimed at a different PR. + * + * ⇒ requiring the remedy is only honest once the tree is already deduped. That + * is why objectui#8333's ruling was "B then A": pay the accumulated debt down in + * its own dedicated pull request FIRST (objectui#9215, landed), and only then + * require the remedy in bump PRs. This gate is the second half, and its whole + * value depends on the first half staying true — which is precisely what it + * enforces. Without it `main` drifts back to a duplicated tree and the next bump + * PR's dedupe is expensive and unreadable again. + * + * ## Why the verdict is pnpm's exit code and not a parse of its output + * + * `pnpm dedupe --check` is pnpm's own contract: exit 0 when nothing would + * change, non-zero when something would. This gate does not re-derive that. It + * reads the exit code, and parses the output ONLY to NAME the packages in the + * annotation. A parse that finds nothing still reds on a non-zero exit — the + * names are a convenience and can never change the verdict, so a future change + * to pnpm's report format degrades the message and not the judgement. + * + * The one thing the output IS load-bearing for is telling a FINDING apart from a + * CRASH. Both exit non-zero, and "the registry was unreachable" is not a finding + * about the diff. So a non-zero exit counts as a finding only when the output + * carries pnpm's own remedy sentinel; otherwise this exits 2, "could not take a + * reading", which is never a pass. + * + * ## ⚠️ This gate reads the REGISTRY, unlike its neighbour + * + * `check-lockfile-integrity.mjs` is one `node` call over two text files. + * This one shells out to pnpm, which resolves against `registry.npmjs.org`, so + * it costs a network round trip and can fail for reasons that are about the + * world rather than the diff. That failure lands on exit 2 (see above) with a + * message saying so, rather than being reported as a lockfile finding. + * + * ⚠️ Stated because it bounds what the green means: `pnpm dedupe` does NOT chase + * the newest version admitted by a range — it only collapses copies that are + * already in the tree. Measured on objectui#8333: `better-auth` is declared + * `^1.7.2` and locked at `1.7.2` while the registry already serves `1.7.4`, + * which that range admits, and this gate is green on that tree. So a publish to + * the registry does not by itself turn this gate red, and it is not the + * spontaneously-drifting check that a "resolve against the live registry" gate + * sounds like. That is an argument from one measured instance plus pnpm's + * documented behaviour, ⛔ not a proof over all future pnpm versions; the + * instrument that re-derives it is this gate's own run history. + */ + +import { spawnSync } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { isEntrypoint } from './invoked-as.mjs'; + +const REPO_ROOT = path.resolve(fileURLToPath(import.meta.url), '..', '..'); + +/** + * pnpm's own remedy line, printed only when `--check` found something to + * collapse. It is what separates a FINDING from a CRASH, both of which exit + * non-zero. Captured verbatim from `pnpm@10.31.0`. + */ +export const DEDUPE_SENTINEL = 'Run pnpm dedupe to apply the changes above.'; + +export const EXIT_CLEAN = 0; +export const EXIT_FINDINGS = 1; +/** A reading that could not be taken is NOT a deduped lockfile. */ +export const EXIT_CANNOT_RUN = 2; + +/** + * The verdict, from pnpm's exit status plus the sentinel. + * + * `status === 0` with the sentinel present is contradictory — pnpm would be + * saying both "nothing to do" and "run dedupe" — so it is reported as + * `cannot-run` rather than being resolved in either direction. + * + * @param {{ status: number|null, output: string }} run + * @returns {'clean'|'findings'|'cannot-run'} + */ +export function classify({ status, output = '' }) { + const sentinel = output.includes(DEDUPE_SENTINEL); + if (status === 0) return sentinel ? 'cannot-run' : 'clean'; + return sentinel ? 'findings' : 'cannot-run'; +} + +/** + * Best-effort: the package NAMES whose copies `pnpm dedupe` would collapse. + * + * `--check` lists the snapshot keys it would remove as `- ` lines, + * where an identity is `name@version` optionally followed by a `(peer)` suffix. + * Scoped names carry a leading `@`, so the version separator is the LAST `@` + * before any peer suffix. + * + * ⛔ Never load-bearing: an empty result does not make a non-zero exit clean. + * + * @param {string} output + * @returns {string[]} sorted, unique + */ +export function namesFromOutput(output = '') { + const names = new Set(); + for (const line of output.split('\n')) { + const match = /^-\s+(\S.*?)\s*$/.exec(line); + if (!match) continue; + const base = match[1].split('(')[0]; + const at = base.lastIndexOf('@'); + if (at <= 0) continue; + names.add(base.slice(0, at)); + } + return [...names].sort(); +} + +/** + * @param {{ outcome: 'clean'|'findings'|'cannot-run', names?: string[], output?: string }} reading + * @returns {string[]} lines to print + */ +export function renderVerdict({ outcome, names = [], output = '' }) { + if (outcome === 'clean') { + return ['VERDICT deduped — `pnpm dedupe` would collapse nothing in this lockfile.']; + } + if (outcome === 'cannot-run') { + return [ + '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.', + ...(output ? ['', '--- pnpm output ---', output.trimEnd()] : []), + ]; + } + const named = names.length ? names.join(', ') : '(pnpm named no removable identity — see its output above)'; + return [ + 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).', + '⛔ 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.', + ]; +} + +/** + * @param {string[]} argv + * @returns {number} process exit code + */ +export function main(argv = process.argv.slice(2)) { + const run = spawnSync('pnpm', ['dedupe', '--check'], { + cwd: REPO_ROOT, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }); + + 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; + } + + // pnpm writes its report across both streams; the verdict reads them together. + const output = `${run.stdout ?? ''}${run.stderr ?? ''}`; + const outcome = classify({ status: run.status, output }); + const names = outcome === 'findings' ? namesFromOutput(output) : []; + + const lines = renderVerdict({ outcome, names, output }); + 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).', + ); + return EXIT_FINDINGS; +} + +/** + * Cases built from REAL `pnpm@10.31.0` output captured on objectui#8333: the + * green leg is today's `main`, the red leg is the same tree with `better-auth` + * bumped, which forks `zod`. + * + * @returns {number} + */ +export function selfTest() { + /** @type {{ name: string, ok: boolean }[]} */ + const cases = []; + const t = (name, ok) => cases.push({ name, ok: Boolean(ok) }); + + const 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'); + const GREEN = ' WARN 7 deprecated subdependencies found: glob@7.2.3\nProgress: resolved 1767, done\n'; + + // ── the verdict comes from the exit code, disambiguated by the sentinel ──── + t('a zero exit with no sentinel is clean', classify({ status: 0, output: GREEN }) === 'clean'); + t('a non-zero exit carrying the sentinel is a finding', classify({ status: 1, output: RED }) === 'findings'); + t( + 'a non-zero exit WITHOUT the sentinel is could-not-run, not a finding', + classify({ status: 1, output: 'ERR_PNPM_FETCH_404 GET https://registry.npmjs.org/x' }) === 'cannot-run', + ); + t( + 'a crash with no output at all is could-not-run, never clean', + classify({ status: 137, output: '' }) === 'cannot-run', + ); + t( + 'a contradictory zero-exit-with-sentinel is could-not-run, not silently clean', + classify({ status: 0, output: RED }) === 'cannot-run', + ); + t('a null status (killed by signal) is could-not-run', classify({ status: null, output: '' }) === 'cannot-run'); + + // ── the names are a convenience; they never decide anything ─────────────── + const names = namesFromOutput(RED); + t('names the unscoped package whose copy is removed', names.includes('zod')); + t('names a SCOPED package without eating its leading @', names.includes('@objectstack/spec')); + t('strips the peer suffix rather than reporting it as part of the name', names.includes('ai')); + t('does not report the ADDED identity as removed', !names.includes('zod-validation-error')); + t('reports each name once, sorted', JSON.stringify(names) === JSON.stringify([...new Set(names)].sort())); + t('finds nothing in clean output', namesFromOutput(GREEN).length === 0); + t( + 'CONTROL — a red verdict still reds when the parse names nothing', + renderVerdict({ outcome: 'findings', names: [], output: RED }).some((l) => l.startsWith('VERDICT not deduped')), + ); + + // ── the rendered verdicts say which of the three answers this was ───────── + t('clean renders VERDICT deduped', renderVerdict({ outcome: 'clean' })[0].startsWith('VERDICT deduped')); + t( + 'a finding names the packages and points at `pnpm dedupe`', + renderVerdict({ outcome: 'findings', names, output: RED }).join('\n').includes('`pnpm dedupe`'), + ); + t( + 'a finding forbids the rejected remedies by name', + renderVerdict({ outcome: 'findings', names, output: RED }).join('\n').includes('pnpm.overrides'), + ); + t( + 'could-not-run says it judged nothing', + renderVerdict({ outcome: 'cannot-run' }).join('\n').includes('Nothing here was judged'), + ); + + const failed = cases.filter((c) => !c.ok); + for (const c of failed) console.error(` ✗ ${c.name}`); + if (failed.length) { + console.error(`✗ check-lockfile-dedupe self-test: ${failed.length} of ${cases.length} case(s) failed.`); + return 1; + } + 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).', + ); + return 0; +} + +if (isEntrypoint(import.meta.url)) { + if (process.argv.includes('--self-test')) { + process.exitCode = selfTest(); + } else { + try { + process.exitCode = main(); + } catch (error) { + console.error( + `::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; + } + } +} diff --git a/scripts/dependabot-merge-gate.mjs b/scripts/dependabot-merge-gate.mjs index a2f5ec0e23..58130e140d 100644 --- a/scripts/dependabot-merge-gate.mjs +++ b/scripts/dependabot-merge-gate.mjs @@ -205,6 +205,8 @@ export const OPTIONAL_CONTEXTS = Object.freeze({ "performance-budget.yml filters on paths: packages/**, apps/console/**, pnpm-lock.yaml. Blocking when it runs (console gzip budget); absent on a PR that touches none of them.", 'Changeset Bump Policy': 'changeset-guard.yml filters on paths: .changeset/**. A Dependabot PR carries no changeset, so it normally does not report at all.', + 'Lockfile Dedupe Check': + "lockfile-dedupe.yml (objectui#8333) runs `pnpm dedupe --check`: the committed lockfile must already be deduped, so a dependency bump cannot leave a forked peer group behind for `Bundle Analysis` to misattribute to the bump. Blocking when it runs; its pull_request trigger is path-filtered to `pnpm-lock.yaml` plus its own runtime closure, so a change touching none of them does not report at all. ⚠️ Enrolled as blocking where its neighbour `Lockfile Integrity Check` deliberately is NOT, and the difference is the remedy: #8326's gate names a duplication and leaves the answer open (re-lock, pin, or accept), which is a judgement call its header reserves for the maintainer, while this one has exactly one mechanical remedy that pnpm itself prints — run `pnpm dedupe` and commit the lockfile, changing no declaration, range or override. Cost of enrolling it, measured on objectui#8333 before it was taken: green on `main` as it stands (objectui#9215 collapsed the accumulated duplication first), red on the same tree with `better-auth` bumped. ⇒ nothing currently mergeable is blocked by it, and what it defends is that objectui#9215's paydown does not silently accrue again. To stop it blocking, move this name to `NOT_A_GATE`; ⛔ removing it from both buckets fails the partition test instead.", 'Hook Self-Tests': 'hook-selftests.yml filters on paths: .claude/hooks/**, plus the workflow file itself (objectui#5754). Blocking when it runs (the PreToolUse guard self-test matrices must pass); a Dependabot dependency bump never touches .claude/hooks/**, so it normally does not report at all.', }); From 82cd71da6a70da85066e11d1eecaa7ceff340fb5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 08:12:41 +0000 Subject: [PATCH 2/2] test(ci-doc): declare the two commands the Lockfile Dedupe section names that its workflow does not run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui#9467's inverted-default sweep pins command parity for every workflow section, so the new `lockfile-dedupe.yml` section added by this PR was born pinned and reported two undeclared phantoms. Both are legitimate — the page is right to name them — so each gets an entry with the reason the instrument cannot see it as a command this workflow's jobs run: - `scripts/dependabot-merge-gate.mjs` — the classifier registry the section points at to say this check is enrolled `OPTIONAL_CONTEXTS`. - `scripts/invoked-as.mjs` — named in the Trigger sentence's runtime-closure enumeration; a watched path, not a command. `pnpm dedupe`, which the section names repeatedly as the remedy, is NOT a phantom: it is not a root `package.json` script, so `firstPartyCommands` never reads it as a first-party command and no entry for it is admissible. Nothing is weakened: `undocumentedCommands` is untouched, no assertion relaxed, and the other 21 entries and every other section are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013VGeMu3p6qEFWR6K6GGLaW --- scripts/__tests__/ci-cd-pipeline-doc.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/scripts/__tests__/ci-cd-pipeline-doc.test.ts b/scripts/__tests__/ci-cd-pipeline-doc.test.ts index 1d226fcf88..240c1f8c23 100644 --- a/scripts/__tests__/ci-cd-pipeline-doc.test.ts +++ b/scripts/__tests__/ci-cd-pipeline-doc.test.ts @@ -3455,6 +3455,19 @@ const SWEEP_DECLARED_NON_RUN_COMMANDS = new Map([ 'contrast the right instrument for sizing the class against the differential gate that ' + 'decides a pull request.', ], + [ + 'lockfile-dedupe.yml: scripts/dependabot-merge-gate.mjs', + 'Named as the registry that classifies this check `OPTIONAL_CONTEXTS` — the declaration that ' + + 'makes its red block a Dependabot auto-merge, and the one file a maintainer edits to demote ' + + 'it. A declaration this section points at, not a step this workflow runs.', + ], + [ + 'lockfile-dedupe.yml: scripts/invoked-as.mjs', + "Named inside the section's TRIGGER sentence, which enumerates this path-filtered gate's " + + 'runtime closure (`check-lockfile-dedupe.mjs` imports `isEntrypoint` from it) to say a ' + + 'change to the checker cannot ship without running it. A path the workflow watches, not a ' + + 'command any job invokes.', + ], [ 'lockfile-integrity.yml: scripts/dependabot-merge-gate.mjs', 'Named as the registry that classifies this workflow `NOT_A_GATE`, in the sentence explaining ' +