From 6ae197f5930840abb569218f9f6287b7e80be5cc Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 20:44:28 +0500 Subject: [PATCH] ci(root): expand the lockstep group over the packages a release resolves The group was expanded against a listing of packages/, while Changesets expands it against every package it enumerates. A pattern reaching outside that directory therefore read as exact coverage here and widened the group at release time, where the highest version any member carries becomes the version of every member. Enumeration now goes through the library Changesets itself uses, so the two agree by construction rather than by agreement. It resolves the root manifest's workspaces field in preference to pnpm-workspace.yaml, which is the answer the release acts on and not the one the workspace file describes. Expansion and group membership are separate questions and no longer share a list: a package outside packages/ belongs to the workspace and not to the train. Naming one in the group is reported apart from naming a package nothing answers to, because the first succeeds at the wrong version and the second fails outright. --- package.json | 1 + pnpm-lock.yaml | 13 +-- scripts/release/check-changesets.mjs | 62 ++++++++---- scripts/release/check-changesets.test.mjs | 110 +++++++++++++++++----- scripts/release/lib.mjs | 71 +++++++++----- 5 files changed, 186 insertions(+), 71 deletions(-) diff --git a/package.json b/package.json index 25ec657e76..d800cc60a3 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,7 @@ "@changesets/parse": "^0.4.3", "@commitlint/cli": "^20.1.0", "@commitlint/config-conventional": "^20.0.0", + "@manypkg/get-packages": "^1.1.3", "@testing-library/jest-dom": "^6.9.1", "@testing-library/react": "^16.3.0", "@testing-library/user-event": "^14.6.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4c85d34175..5fd20bbc9b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -84,6 +84,9 @@ importers: '@commitlint/config-conventional': specifier: ^20.0.0 version: 20.0.0 + '@manypkg/get-packages': + specifier: ^1.1.3 + version: 1.1.3 '@testing-library/jest-dom': specifier: ^6.9.1 version: 6.9.1 @@ -1933,10 +1936,6 @@ packages: peerDependencies: '@babel/core': ^7.29.6 - '@babel/runtime@7.28.4': - resolution: {integrity: sha512-Q/N6JNWvIvPnLDvjlE1OUBLPQHH6l3CltCEsHIujp45zQUSSh8K+gHnaEX45yAT1nyngnINhvWtzN+Nb9D8RAQ==} - engines: {node: '>=6.9.0'} - '@babel/runtime@7.29.7': resolution: {integrity: sha512-Nq8OhGWiZIZGV6hLHoyAKLLcJihP/xFeBMGJoUrxTX2psI8dCifzLhZISFb+VWS3wFMRDmCGw5R+dOySCqPLhw==} engines: {node: '>=6.9.0'} @@ -9124,8 +9123,6 @@ snapshots: '@babel/core': 7.29.7 '@babel/helper-plugin-utils': 7.27.1 - '@babel/runtime@7.28.4': {} - '@babel/runtime@7.29.7': {} '@babel/template@7.29.7': @@ -10581,14 +10578,14 @@ snapshots: '@manypkg/find-root@1.1.0': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@types/node': 12.20.55 find-up: 4.1.0 fs-extra: 8.1.0 '@manypkg/get-packages@1.1.3': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@changesets/types': 4.1.0 '@manypkg/find-root': 1.1.0 fs-extra: 8.1.0 diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs index b21fd6d510..0c18b79304 100644 --- a/scripts/release/check-changesets.mjs +++ b/scripts/release/check-changesets.mjs @@ -31,7 +31,7 @@ import { fileURLToPath } from "node:url"; import parseChangeset from "@changesets/parse"; import micromatch from "micromatch"; -import { getWorkspacePackageNames } from "./lib.mjs"; +import { getWorkspacePackages } from "./lib.mjs"; const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); @@ -58,11 +58,13 @@ const ALPHA_BUMP = "patch"; * `[["a", "b"]]` and is refused by the release tooling, and an entry that is not * a non-empty string names nothing. * - * Checked here rather than by handing the file to `@changesets/config`, whose - * `parse` needs a resolved workspace from `@manypkg/get-packages` — a second - * dependency, for a rule that is three sentences long. That is the opposite trade - * to the YAML reader, and for the opposite reason: this rule cannot drift, and a - * frontmatter grammar can. + * Checked here rather than by handing the file to `@changesets/config`, and NOT + * because that would cost a dependency — the workspace its `parse` needs is + * resolved a few lines above. Because it would answer a different question. + * Changesets accepts any number of disjoint groups and objects only to a package + * appearing in two of them; "exactly one group" is this repository's rule, which + * upstream will never enforce on its behalf. Delegating the check would silently + * drop the part that matters most here. */ export function fixedGroupShape(config) { const groups = config.fixed; @@ -140,13 +142,21 @@ export function lockstepPackages(configText, workspaceNames) { * config packages. Measuring against "what we publish" would report those four as * errors on every run. * - * Checked in BOTH directions. A name in the group that no package answers to is - * a rename or a deletion that the config still believes in, and Changesets fails - * a release on an unknown package in `fixed`. + * Checked in BOTH directions, and the second direction has two causes that are + * NOT the same fault. A name no workspace package answers to is a rename or a + * deletion the config still believes in, and Changesets refuses the release + * outright. A name that DOES resolve but sits outside `packages/` is the + * opposite: the release proceeds, quietly widened. It matters because the group + * moves every member to the highest version any of them carries, so one package + * versioned on its own line — an app, a harness — decides the number the whole + * train takes. Reporting both under one sentence would tell an author the release + * will fail when it is instead going to succeed at the wrong version. */ -export function groupMatchesWorkspace(packages, workspaceNames) { +export function groupMatchesWorkspace(packages, workspace) { const problems = []; - const absent = workspaceNames.filter(name => !packages.includes(name)); + const absent = workspace.underPackagesDir.filter( + name => !packages.includes(name) + ); if (absent.length > 0) { problems.push( `.changeset/config.json: the \`fixed\` group is missing ${absent.join(", ")}. ` + @@ -154,11 +164,23 @@ export function groupMatchesWorkspace(packages, workspaceNames) { `stranded at an older version by the next release.` ); } - const unknown = packages.filter(name => !workspaceNames.includes(name)); + const outside = packages.filter( + name => + !workspace.underPackagesDir.includes(name) && workspace.all.includes(name) + ); + if (outside.length > 0) { + problems.push( + `.changeset/config.json: the \`fixed\` group reaches ${outside.join(", ")}, ` + + `which is in the workspace but not under packages/. The group takes the ` + + `highest version any member carries, so a package versioned on its own ` + + `line decides the version of the entire release train.` + ); + } + const unknown = packages.filter(name => !workspace.all.includes(name)); if (unknown.length > 0) { problems.push( `.changeset/config.json: the \`fixed\` group names ${unknown.join(", ")}, ` + - `which no package under packages/ answers to. Changesets refuses a release ` + + `which no package in the workspace answers to. Changesets refuses a release ` + `on an unknown package in \`fixed\`.` ); } @@ -195,7 +217,8 @@ export function declaredReleases(fileText) { return undefined; } const releases = new Map(); - for (const release of parsed.releases) releases.set(release.name, release.type); + for (const release of parsed.releases) + releases.set(release.name, release.type); return releases; } @@ -259,8 +282,11 @@ export function problemsWith(path, fileText, packages) { * because the PR that adds a package is often exactly that one — and a stale * group makes every later changeset wrong while each of them passes. */ -export function checkChangesets(paths, readFile, configText, workspaceNames) { - const packages = lockstepPackages(configText, workspaceNames); +export function checkChangesets(paths, readFile, configText, workspace) { + // Expanded against the WHOLE workspace, which is what Changesets expands + // against. Narrowing it here to the packages the group is supposed to contain + // would make a pattern reaching outside `packages/` look like exact coverage. + const packages = lockstepPackages(configText, workspace.all); if (packages.length === 0) { // A config with no fixed group would make every check below vacuous, and a // guard that passes because it found nothing to check is worse than none. @@ -275,7 +301,7 @@ export function checkChangesets(paths, readFile, configText, workspaceNames) { // reading the release tooling does not share. if (shape.length > 0) return shape; return [ - ...groupMatchesWorkspace(packages, workspaceNames), + ...groupMatchesWorkspace(packages, workspace), ...paths.flatMap(path => problemsWith(path, readFile(path), packages)), ]; } @@ -349,7 +375,7 @@ async function main(argv) { paths, path => readFileSync(resolve(REPO_ROOT, path), "utf8"), readFileSync(resolve(REPO_ROOT, ".changeset", "config.json"), "utf8"), - getWorkspacePackageNames() + getWorkspacePackages() ); if (problems.length === 0) { console.log( diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index 58c135e6f2..6595eb1744 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -14,8 +14,21 @@ import { const CONFIG = JSON.stringify({ fixed: [["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]], }); -const WORKSPACE = ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]; -const PACKAGES = lockstepPackages(CONFIG, WORKSPACE); +const MEMBERS = ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]; + +/** A workspace whose every package is one the group is supposed to contain. */ +const WORKSPACE = { all: MEMBERS, underPackagesDir: MEMBERS }; + +/** + * The same workspace plus an app, which is the shape the real repository has: + * in the workspace, versioned on its own line, deliberately NOT on the train. + */ +const WITH_AN_APP = { + all: [...MEMBERS, "playground"], + underPackagesDir: MEMBERS, +}; + +const PACKAGES = lockstepPackages(CONFIG, WORKSPACE.all); /** A changeset naming the given `package: bump` pairs. */ function changeset(pairs, body = "Something changed.") { @@ -34,7 +47,7 @@ describe("reading the group", () => { // A guard that passes because it found nothing to check is worse than no // guard: it reports success on every changeset for as long as the config is // broken. - const problems = checkChangesets(["a.md"], () => COMPLETE, "{}", PACKAGES); + const problems = checkChangesets(["a.md"], () => COMPLETE, "{}", WORKSPACE); expect(problems).toHaveLength(1); expect(problems[0]).toContain("no `fixed` group"); }); @@ -127,17 +140,24 @@ describe("what else a frontmatter can get wrong", () => { describe("the group against the workspace", () => { it("accepts a group that matches", () => { // The control, and the state the repository is in today. - expect(groupMatchesWorkspace(PACKAGES, [...PACKAGES])).toEqual([]); + expect(groupMatchesWorkspace(PACKAGES, WORKSPACE)).toEqual([]); + }); + + it("accepts a group that leaves an app out of the train", () => { + // The other control, and the one the real config relies on: a workspace + // package outside packages/ is not a gap in the group. A check measuring the + // group against the whole workspace would report `playground` on every run. + expect(groupMatchesWorkspace(PACKAGES, WITH_AN_APP)).toEqual([]); }); it("catches a package the config was never told about", () => { // The drift the config cannot see about itself: a PR adds a package under // packages/ and every changeset in it names the old members, so all of them // pass while the new package is stranded on the next train. - const problems = groupMatchesWorkspace(PACKAGES, [ - ...PACKAGES, - "@nextlyhq/newcomer", - ]); + const problems = groupMatchesWorkspace(PACKAGES, { + all: [...MEMBERS, "@nextlyhq/newcomer"], + underPackagesDir: [...MEMBERS, "@nextlyhq/newcomer"], + }); expect(problems).toHaveLength(1); expect(problems[0]).toContain("@nextlyhq/newcomer"); @@ -149,21 +169,43 @@ describe("the group against the workspace", () => { // release on an unknown package in `fixed`. const problems = groupMatchesWorkspace( [...PACKAGES, "@nextlyhq/departed"], - [...PACKAGES] + WORKSPACE ); expect(problems).toHaveLength(1); expect(problems[0]).toContain("@nextlyhq/departed"); + expect(problems[0]).toContain("refuses a release"); + }); + + it("separates a group reaching outside packages/ from a name that resolves to nothing", () => { + // Two different outcomes, so they cannot share a sentence. A name nothing + // answers to fails the release; a name answering to an app succeeds it, at + // whatever version that app happens to carry. + const problems = groupMatchesWorkspace( + [...PACKAGES, "playground", "@nextlyhq/departed"], + WITH_AN_APP + ); + + expect(problems).toHaveLength(2); + const reaches = problems.find(problem => problem.includes("playground")); + expect(reaches).toContain("not under packages/"); + expect(reaches).not.toContain("@nextlyhq/departed"); + + const unknown = problems.find(problem => + problem.includes("@nextlyhq/departed") + ); + expect(unknown).toContain("refuses a release"); + expect(unknown).not.toContain("playground"); }); it("is asked even when the pull request touches no changeset", () => { // The PR that adds a package is often exactly the one with no changeset of // its own, so an early return on an empty list would skip the check that // matters most. - const problems = checkChangesets([], () => "", CONFIG, [ - ...PACKAGES, - "@nextlyhq/newcomer", - ]); + const problems = checkChangesets([], () => "", CONFIG, { + all: [...MEMBERS, "@nextlyhq/newcomer"], + underPackagesDir: [...MEMBERS, "@nextlyhq/newcomer"], + }); expect(problems).toHaveLength(1); expect(problems[0]).toContain("@nextlyhq/newcomer"); @@ -295,7 +337,7 @@ describe("the shape of the fixed group itself", () => { [], () => "", JSON.stringify({ fixed: ["nextly", "@nextlyhq/ui"] }), - ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"] + WORKSPACE ); expect(problems).toHaveLength(1); expect(problems[0]).toContain("array of ARRAYS"); @@ -309,8 +351,8 @@ describe("a group written with globs", () => { // report every matching package as missing and the pattern as unknown, which // on a config written this way rejects every pull request. const globbed = JSON.stringify({ fixed: [["nextly", "@nextlyhq/*"]] }); - expect(lockstepPackages(globbed, WORKSPACE).sort()).toEqual( - [...WORKSPACE].sort() + expect(lockstepPackages(globbed, WORKSPACE.all).sort()).toEqual( + [...MEMBERS].sort() ); }); @@ -319,7 +361,7 @@ describe("a group written with globs", () => { // `@changesets/config` does it. Evaluating each pattern alone and unioning // the results would let `**` put back what `!…` excluded. const negated = JSON.stringify({ fixed: [["**", "!@nextlyhq/builder"]] }); - expect(lockstepPackages(negated, WORKSPACE).sort()).toEqual([ + expect(lockstepPackages(negated, WORKSPACE.all).sort()).toEqual([ "@nextlyhq/ui", "nextly", ]); @@ -327,14 +369,36 @@ describe("a group written with globs", () => { it("reports a pattern that matches nothing by the name it was written as", () => { const globbed = JSON.stringify({ fixed: [["@nowhere/*"]] }); - expect(lockstepPackages(globbed, WORKSPACE)).toEqual(["@nowhere/*"]); + expect(lockstepPackages(globbed, WORKSPACE.all)).toEqual(["@nowhere/*"]); }); it("does not report the workspace as missing when a glob covers it", () => { const globbed = JSON.stringify({ fixed: [["nextly", "@nextlyhq/*"]] }); - expect( - checkChangesets([], () => "", globbed, WORKSPACE) - ).toEqual([]); + expect(checkChangesets([], () => "", globbed, WORKSPACE)).toEqual([]); + }); + + it("expands a broad pattern over the packages a release resolves, not one directory", () => { + // The gap this closes. `**` reaches every package Changesets enumerates, + // which includes the ones outside packages/. Expanding it against the + // packages/ listing alone answers with exact coverage, so a changeset naming + // only those packages passes here and the release widens underneath it. + const globbed = JSON.stringify({ fixed: [["**"]] }); + + expect(lockstepPackages(globbed, WITH_AN_APP.all).sort()).toEqual( + [...WITH_AN_APP.all].sort() + ); + + const problems = checkChangesets([], () => "", globbed, WITH_AN_APP); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("playground"); + expect(problems[0]).toContain("not under packages/"); + }); + + it("still accepts a broad pattern in a workspace that is only packages/", () => { + // The other direction, so the case above cannot pass by rejecting `**` on + // sight: with nothing outside packages/ to reach, the same group is correct. + const globbed = JSON.stringify({ fixed: [["**"]] }); + expect(checkChangesets([], () => "", globbed, WORKSPACE)).toEqual([]); }); }); @@ -403,7 +467,7 @@ describe("what the check is pointed at", () => { ["good.md", "bad.md"], path => files[path], CONFIG, - PACKAGES + WORKSPACE ); expect(problems).toHaveLength(1); @@ -413,6 +477,6 @@ describe("what the check is pointed at", () => { it("passes when it is given nothing", () => { // The ordinary PR touches no changeset — a test-only or docs-only one gets // none at all — and that must not be a failure. - expect(checkChangesets([], () => "", CONFIG, PACKAGES)).toEqual([]); + expect(checkChangesets([], () => "", CONFIG, WORKSPACE)).toEqual([]); }); }); diff --git a/scripts/release/lib.mjs b/scripts/release/lib.mjs index 30607c749a..4fa6fec6cb 100644 --- a/scripts/release/lib.mjs +++ b/scripts/release/lib.mjs @@ -9,6 +9,8 @@ import { existsSync, readdirSync, readFileSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { getPackagesSync } from "@manypkg/get-packages"; + const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); const PACKAGES_DIR = join(REPO_ROOT, "packages"); const PRE_STATE_PATH = join(REPO_ROOT, ".changeset", "pre.json"); @@ -78,6 +80,53 @@ export function readPreState() { return state.mode === "pre" ? state : null; } +/** + * The workspace as the release tooling sees it, in two lists that are NOT the + * same and must not be swapped: + * + * - `all` — every package Changesets resolves, private ones included. This is + * what a `fixed` entry is expanded against, so a pattern here has to match + * against exactly this list or the group this repository checks is not the + * group the release builds. + * - `underPackagesDir` — those sitting directly in `packages/`. This is what the + * group is supposed to CONTAIN, which is a different question: `apps/*` is in + * the workspace and deliberately outside the release train. + * + * Both come from one walk rather than two, because they only mean anything + * relative to each other: enumerated separately they could disagree about which + * packages exist, and every comparison below would then be reading two different + * workspaces. + * + * Resolved by `@manypkg/get-packages` rather than by reading a directory or a + * workspace file, because that is the library Changesets itself enumerates + * through — so what expands here and what expands at release time agree by + * construction. The distinction is not academic in this repository: the root + * manifest carries a `workspaces` field, which that library prefers over + * `pnpm-workspace.yaml`, so the two files describe different sets and only one + * of them is the one Changesets acts on. + * + * A package whose manifest has no `name` throws rather than being skipped: it is + * a member of the workspace that no list here can report on, and dropping it + * would make every check below quietly answer a smaller question. + */ +export function getWorkspacePackages() { + const { packages } = getPackagesSync(REPO_ROOT); + const all = []; + const underPackagesDir = []; + + for (const entry of packages) { + const name = entry.packageJson.name; + all.push(name); + if (dirname(entry.dir) === PACKAGES_DIR) underPackagesDir.push(name); + } + + const byName = (a, b) => a.localeCompare(b); + return { + all: all.sort(byName), + underPackagesDir: underPackagesDir.sort(byName), + }; +} + /** * Every workspace package that `changeset publish` is expected to push to npm: * the contents of `packages/*` minus anything marked private. Private packages @@ -88,28 +137,6 @@ export function readPreState() { * A manifest that exists but cannot be parsed is an error rather than a silent * skip: dropping it here would also drop it from every check below. */ -/** - * Every package name under `packages/`, private ones included. - * - * Deliberately wider than {@link getReleaseManifest}, which answers "what do we - * publish". Changesets versions private workspace packages too unless told - * otherwise, so the set it has to be told about is every package in the - * directory rather than only the publishable ones — and a checker comparing the - * `fixed` group against the narrower list would report the config-only packages - * as errors on every run. - */ -export function getWorkspacePackageNames() { - const names = []; - for (const entry of readdirSync(PACKAGES_DIR, { withFileTypes: true })) { - if (!entry.isDirectory()) continue; - const manifestPath = join(PACKAGES_DIR, entry.name, "package.json"); - if (!existsSync(manifestPath)) continue; - const pkg = readJson(manifestPath); - if (typeof pkg.name === "string") names.push(pkg.name); - } - return names.sort((a, b) => a.localeCompare(b)); -} - export function getReleaseManifest() { const manifest = [];