From 69351f4e81ebebd05b850d126b2524f622670bff Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 14:59:57 +0500 Subject: [PATCH 1/7] ci(root): fail a changeset that misses a lockstep package --- .github/workflows/ci.yml | 26 +++ scripts/release/check-changesets.mjs | 186 ++++++++++++++++++++ scripts/release/check-changesets.test.mjs | 202 ++++++++++++++++++++++ 3 files changed, 414 insertions(+) create mode 100644 scripts/release/check-changesets.mjs create mode 100644 scripts/release/check-changesets.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 68a6fe9d96..e9be65876d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,6 +102,32 @@ jobs: - name: Release script tests run: pnpm test:scripts + # The published packages version in lockstep, so a changeset that names + # only some of them still bumps them all — and the ones it left out get a + # version with no changelog entry beside it. Reviewers caught that twice in + # one afternoon, both times on frontmatter that HAD been generated from the + # config, just before a new package joined the group or before the branch + # merged the commit that added it. + # + # Scoped to the changesets THIS pull request adds or edits. Several hundred + # are pending on `main`, most written before the group grew; rewriting them + # to satisfy a rule they predate would churn the eventual changelog for no + # gain. + # + # `HEAD^1` is what makes the scoping exact. On a pull request the checkout + # is the MERGE commit, whose first parent is the base branch — so diffing + # against it is the pull request's own diff, already relative to the merge + # base. Diffing against the base branch TIP instead would report every + # changeset the branch picked up by merging `main` as one this branch + # wrote, which is how a guard starts failing PRs over other people's files. + - name: Changeset covers the lockstep group + if: github.event_name == 'pull_request' + run: | + set -euo pipefail + { git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md' \ + | grep -v '^\.changeset/README\.md$' || true; } \ + | node scripts/release/check-changesets.mjs + # Enforce the design-token theming contract in the admin + plugin packages # (no hsl(var(--x)) wrappers, no hardcoded colors, no stray !important). # See packages/ui/docs/plugin-ui-authoring.md. Cheap check, fails fast. diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs new file mode 100644 index 0000000000..cd773b5db7 --- /dev/null +++ b/scripts/release/check-changesets.mjs @@ -0,0 +1,186 @@ +// Refuses a changeset that does not cover every package in the lockstep group. +// +// The packages version together, so a release advances all of them whatever a +// single changeset lists. What an incomplete one loses is the CHANGELOG: a +// package left out of the frontmatter gets a version bump with no entry +// explaining it, and the note that should have appeared under it is filed only +// under the packages that were named. The release is still correct; the record +// of it is not. +// +// This existed as a review convention and was caught by a reviewer twice in one +// afternoon, both times on a changeset that HAD been generated from the config — +// once written before a new package joined the group, once written before the +// branch merged the commit that added it. A convention that depends on +// regenerating at the right moment is one a build should check instead. +// +// Scoped to the changesets a branch ADDS or EDITS, never the backlog. Several +// hundred are pending on `main`, most written before the group grew, and +// rewriting them to satisfy a rule they predate would put churn in front of +// every reader of the eventual changelog for no gain. + +import { readFileSync } from "node:fs"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); + +/** + * The bump every changeset uses while the packages are in alpha. + * + * A `minor` or `major` here does not just mislabel one entry: the group versions + * in lockstep, so the largest bump in the release decides the number every + * package gets, and one mislabelled changeset moves the whole train. + */ +const ALPHA_BUMP = "patch"; + +/** Every package that must appear in a changeset, read from the Changesets config. */ +export function lockstepPackages(configText) { + const config = JSON.parse(configText); + return (config.fixed ?? []).flat(); +} + +/** + * The `package: bump` pairs a changeset declares. + * + * Parsed rather than pulled from `@changesets/parse`, because the answer needed + * here is "what does the file SAY", and a parser that tolerates a malformed + * frontmatter by returning an empty release list would report a changeset naming + * nothing as one naming nothing WRONG. A file this cannot read is reported as + * unreadable instead. + */ +export function declaredReleases(fileText) { + const match = /^---\r?\n([\s\S]*?)\r?\n---/.exec(fileText); + if (match === null) return undefined; + const releases = new Map(); + for (const line of match[1].split(/\r?\n/)) { + const trimmed = line.trim(); + if (trimmed === "") continue; + // Both spellings Changesets accepts for a name: quoted, which every scoped + // package needs, and bare, which an unscoped one may use. + const entry = /^(?:"([^"]+)"|([^:\s]+))\s*:\s*(\S+)\s*$/.exec(trimmed); + if (entry === null) return undefined; + releases.set(entry[1] ?? entry[2], entry[3]); + } + return releases; +} + +/** + * What is wrong with one changeset, as a list of sentences, empty when nothing is. + * + * Returns every problem rather than the first, so one push answers all of them. + * A guard that reports one missing package at a time turns a stale frontmatter + * into as many CI rounds as it has gaps. + */ +export function problemsWith(path, fileText, packages) { + const releases = declaredReleases(fileText); + if (releases === undefined) { + return [ + `${path}: the frontmatter is missing or has a line this cannot read. ` + + `A changeset opens with \`---\`, one \`"package": bump\` per line, and closes with \`---\`.`, + ]; + } + const problems = []; + const missing = packages.filter(name => !releases.has(name)); + if (missing.length > 0) { + problems.push( + `${path}: missing ${missing.length} of ${packages.length} lockstep packages — ` + + `${missing.join(", ")}. They version together, so a package left out is bumped ` + + `with no changelog entry. Generate the list from .changeset/config.json.` + ); + } + const unknown = [...releases.keys()].filter(name => !packages.includes(name)); + if (unknown.length > 0) { + problems.push( + `${path}: names ${unknown.join(", ")}, which the lockstep group does not contain. ` + + `A package that has left the group, or a typo, releases nothing.` + ); + } + const wrongBump = [...releases.entries()] + .filter(([, bump]) => bump !== ALPHA_BUMP) + .map(([name, bump]) => `${name}: ${bump}`); + if (wrongBump.length > 0) { + problems.push( + `${path}: uses a bump other than \`${ALPHA_BUMP}\` (${wrongBump.join(", ")}). ` + + `The group takes the largest bump in the release, so one of these moves every package.` + ); + } + return problems; +} + +/** Every problem across the given changeset paths. */ +export function checkChangesets(paths, readFile, configText) { + const packages = lockstepPackages(configText); + 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. + return [ + ".changeset/config.json declares no `fixed` group, so nothing here can be checked.", + ]; + } + return paths.flatMap(path => problemsWith(path, readFile(path), packages)); +} + +/** + * The changeset files to check: arguments when given, otherwise a newline-separated + * list on stdin. + * + * Passed in rather than discovered, because "which changesets does this branch + * add" is a question about git that the caller already has to ask, and asking it + * here would mean this script could only run one way. + * + * Stdin is what the workflow uses, and it is the reason the empty case is not a + * special one: a pipe that produced nothing arrives as an empty string, whereas + * an empty argument list has to survive shell expansion under `set -u` to get + * here at all. + */ +export function pathsToCheck(argv, stdinText) { + const fromArgv = argv.filter(path => path.endsWith(".md")); + if (fromArgv.length > 0) return fromArgv; + return stdinText + .split("\n") + .map(line => line.trim()) + .filter(line => line.endsWith(".md")); +} + +/** + * Everything piped in, as text. + * + * Streamed rather than read with `readFileSync(0)`. A pipe can be open and + * momentarily empty, and the synchronous read answers that with `EAGAIN` instead + * of waiting — which fails whenever the process on the other side is a shade + * slower than this one, and passes when it is not. A guard that depends on + * scheduling is worse than no guard. + * + * A terminal is not read at all: a hand-run with no arguments would otherwise + * wait forever for input nobody is going to type. + */ +async function readStdin() { + if (process.stdin.isTTY) return ""; + const chunks = []; + for await (const chunk of process.stdin) chunks.push(chunk); + return Buffer.concat(chunks).toString("utf8"); +} + +async function main(argv) { + const paths = pathsToCheck(argv, await readStdin()); + if (paths.length === 0) { + console.log("No changesets added or edited; nothing to check."); + return 0; + } + const problems = checkChangesets( + paths, + path => readFileSync(resolve(REPO_ROOT, path), "utf8"), + readFileSync(resolve(REPO_ROOT, ".changeset", "config.json"), "utf8") + ); + if (problems.length === 0) { + console.log(`Checked ${paths.length} changeset(s): all cover the group.`); + return 0; + } + for (const problem of problems) console.error(`✖ ${problem}`); + return 1; +} + +// Only when run directly, so the exported helpers stay importable from a test. +if (process.argv[1] === fileURLToPath(import.meta.url)) { + process.exit(await main(process.argv.slice(2))); +} diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs new file mode 100644 index 0000000000..56113cc29d --- /dev/null +++ b/scripts/release/check-changesets.test.mjs @@ -0,0 +1,202 @@ +import { describe, expect, it } from "vitest"; + +import { + checkChangesets, + declaredReleases, + lockstepPackages, + pathsToCheck, + problemsWith, +} from "./check-changesets.mjs"; + +/** A lockstep group small enough to read, shaped like the real one. */ +const CONFIG = JSON.stringify({ + fixed: [["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]], +}); +const PACKAGES = lockstepPackages(CONFIG); + +/** A changeset naming the given `package: bump` pairs. */ +function changeset(pairs, body = "Something changed.") { + const frontmatter = pairs.map(([name, bump]) => `"${name}": ${bump}`); + return `---\n${frontmatter.join("\n")}\n---\n\n${body}\n`; +} + +const COMPLETE = changeset(PACKAGES.map(name => [name, "patch"])); + +describe("reading the group", () => { + it("flattens the fixed array", () => { + expect(PACKAGES).toEqual(["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]); + }); + + it("refuses to check anything when the config declares no 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, "{}"); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("no `fixed` group"); + }); +}); + +describe("a complete changeset", () => { + it("has nothing wrong with it", () => { + // The control. Without it every assertion below could pass because the check + // rejects everything. + expect(problemsWith("a.md", COMPLETE, PACKAGES)).toEqual([]); + }); + + it("does not care what order the packages are in", () => { + const reordered = changeset( + [...PACKAGES].reverse().map(name => [name, "patch"]) + ); + expect(problemsWith("a.md", reordered, PACKAGES)).toEqual([]); + }); + + it("accepts the unquoted spelling an unscoped package may use", () => { + const mixed = `---\nnextly: patch\n"@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; + expect(problemsWith("a.md", mixed, PACKAGES)).toEqual([]); + }); +}); + +describe("the omission this exists for", () => { + it("names every package that is missing, not just the first", () => { + // Reporting one at a time turns a frontmatter written against an older group + // into as many CI rounds as it has gaps. + const partial = changeset([["nextly", "patch"]]); + const problems = problemsWith("a.md", partial, PACKAGES); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("@nextlyhq/ui"); + expect(problems[0]).toContain("@nextlyhq/builder"); + expect(problems[0]).toContain("missing 2 of 3"); + }); + + it("catches the one-package gap a new group member leaves", () => { + // The exact shape reviewers caught twice: a frontmatter generated from the + // config BEFORE a package joined the group, so it is complete against the + // config it was written from and short against the current one. + const beforeBuilder = changeset([ + ["nextly", "patch"], + ["@nextlyhq/ui", "patch"], + ]); + const problems = problemsWith("a.md", beforeBuilder, PACKAGES); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("@nextlyhq/builder"); + expect(problems[0]).not.toContain("@nextlyhq/ui"); + }); +}); + +describe("what else a frontmatter can get wrong", () => { + it("rejects a package the group does not contain", () => { + const stale = changeset([ + ...PACKAGES.map(name => [name, "patch"]), + ["@nextlyhq/departed", "patch"], + ]); + const problems = problemsWith("a.md", stale, PACKAGES); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("@nextlyhq/departed"); + }); + + it("rejects a bump other than patch", () => { + // The group takes the largest bump in the release, so one `minor` moves every + // package's version, not just the one it is written beside. + const minor = changeset([ + ["nextly", "minor"], + ["@nextlyhq/ui", "patch"], + ["@nextlyhq/builder", "patch"], + ]); + const problems = problemsWith("a.md", minor, PACKAGES); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("nextly: minor"); + }); + + it("reports several problems together", () => { + const bad = changeset([ + ["nextly", "minor"], + ["@nextlyhq/departed", "patch"], + ]); + expect(problemsWith("a.md", bad, PACKAGES)).toHaveLength(3); + }); +}); + +describe("a file this cannot read", () => { + it("is reported rather than treated as naming nothing wrong", () => { + // The failure mode worth naming: a tolerant parser answers "no releases" for + // a broken frontmatter, and "no releases" and "every release" are opposite + // answers that a missing-package check would score the same way. + expect(problemsWith("a.md", "No frontmatter here.\n", PACKAGES)).toEqual([ + expect.stringContaining("frontmatter is missing"), + ]); + }); + + it("is reported when a line inside the frontmatter is malformed", () => { + const broken = `---\n"nextly" patch\n---\n\nBody.\n`; + expect(problemsWith("a.md", broken, PACKAGES)).toEqual([ + expect.stringContaining("cannot read"), + ]); + }); + + it("reads a frontmatter written with CRLF line endings", () => { + // A changeset can be authored on Windows, and a check that only splits on + // `\n` would read `patch\r` as a bump that is not `patch` and reject every + // one of them. + const crlf = COMPLETE.replace(/\n/g, "\r\n"); + expect(declaredReleases(crlf)?.get("nextly")).toBe("patch"); + expect(problemsWith("a.md", crlf, PACKAGES)).toEqual([]); + }); +}); + +describe("where the file list comes from", () => { + it("prefers arguments when it is given them", () => { + expect(pathsToCheck(["a.md", "b.md"], "never.md")).toEqual([ + "a.md", + "b.md", + ]); + }); + + it("reads stdin when there are no arguments", () => { + expect(pathsToCheck([], ".changeset/a.md\n.changeset/b.md\n")).toEqual([ + ".changeset/a.md", + ".changeset/b.md", + ]); + }); + + it("reads an empty pipe as nothing to do", () => { + // The ordinary PR touches no changeset, so the pipe delivers an empty + // string. Reading that as one path named "" would fail every such build on a + // file that does not exist. + expect(pathsToCheck([], "")).toEqual([]); + expect(pathsToCheck([], "\n")).toEqual([]); + }); + + it("ignores anything that is not a changeset", () => { + expect(pathsToCheck([], ".changeset/a.md\npackage.json\n")).toEqual([ + ".changeset/a.md", + ]); + }); +}); + +describe("what the check is pointed at", () => { + it("checks every path it is given", () => { + const files = { + "good.md": COMPLETE, + "bad.md": changeset([["nextly", "patch"]]), + }; + const problems = checkChangesets( + ["good.md", "bad.md"], + path => files[path], + CONFIG + ); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("bad.md"); + }); + + 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)).toEqual([]); + }); +}); From b039a6a51522b2a274774daadb6a6ae79212cb6b Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 15:25:03 +0500 Subject: [PATCH 2/7] ci(root): check the group itself, and stop the gate failing open --- .github/workflows/ci.yml | 11 +- scripts/release/check-changesets.mjs | 155 +++++++++++++++++++--- scripts/release/check-changesets.test.mjs | 95 ++++++++++++- scripts/release/lib.mjs | 22 +++ 4 files changed, 256 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e9be65876d..5ec480da34 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,8 +124,15 @@ jobs: if: github.event_name == 'pull_request' run: | set -euo pipefail - { git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md' \ - | grep -v '^\.changeset/README\.md$' || true; } \ + # `git diff` runs on its own line so a failure — an unavailable `HEAD^1` + # in some other checkout shape, a broken object store — aborts the step + # under `set -e`. Inside a pipeline with a trailing `|| true` it would + # not: the checker would read empty stdin, report nothing to check, and + # the gate would be off while the step stayed green. Only `grep`'s + # no-match status is suppressed, which is the one expected failure here. + touched="$(git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md')" + printf '%s\n' "$touched" \ + | { grep -v '^\.changeset/README\.md$' || true; } \ | node scripts/release/check-changesets.mjs # Enforce the design-token theming contract in the admin + plugin packages diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs index cd773b5db7..385566c2cd 100644 --- a/scripts/release/check-changesets.mjs +++ b/scripts/release/check-changesets.mjs @@ -17,11 +17,19 @@ // hundred are pending on `main`, most written before the group grew, and // rewriting them to satisfy a rule they predate would put churn in front of // every reader of the eventual changelog for no gain. +// +// The GROUP is checked too, and against the workspace rather than against +// itself. A checker that reads `.changeset/config.json` as the source of truth +// cannot see a package added under `packages/` and never added to `fixed`, which +// is the drift that makes every changeset written afterwards wrong while each of +// them passes. import { readFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { getWorkspacePackageNames } from "./lib.mjs"; + const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); /** @@ -40,26 +48,116 @@ export function lockstepPackages(configText) { } /** - * The `package: bump` pairs a changeset declares. + * What is wrong with the GROUP itself, as a list of sentences, empty when nothing is. + * + * The config is what every changeset is generated from, so a checker that treats + * it as the source of truth cannot see the one drift that matters most: a pull + * request adding a package under `packages/` and not adding it to `fixed`. Every + * changeset in that PR names the old members, passes, and the new package is left + * behind on the next train — discovered after a version PR has already merged. * - * Parsed rather than pulled from `@changesets/parse`, because the answer needed - * here is "what does the file SAY", and a parser that tolerates a malformed - * frontmatter by returning an empty release list would report a changeset naming - * nothing as one naming nothing WRONG. A file this cannot read is reported as - * unreadable instead. + * Compared against `packages/` rather than against the publishable subset, + * because Changesets versions private workspace packages too unless told + * otherwise, and four of this group's members are `private: true` build-time + * 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`. + */ +export function groupMatchesWorkspace(packages, workspaceNames) { + const problems = []; + const absent = workspaceNames.filter(name => !packages.includes(name)); + if (absent.length > 0) { + problems.push( + `.changeset/config.json: the \`fixed\` group is missing ${absent.join(", ")}. ` + + `Every package under packages/ versions with the group; one left out is ` + + `stranded at an older version by the next release.` + ); + } + const unknown = packages.filter(name => !workspaceNames.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 ` + + `on an unknown package in \`fixed\`.` + ); + } + return problems; +} + +/** + * One scalar as YAML would read it: quotes stripped, a trailing comment removed. + * + * A quoted value keeps everything inside the quotes, `#` included, because a + * comment cannot start inside a scalar. Only an unquoted value has a comment to + * strip, and only when the `#` is preceded by whitespace — `patch#1` is one + * token, `patch # note` is a value and a note. + */ +function scalar(raw) { + const trimmed = raw.trim(); + const quoted = /^(["'])((?:(?!\1)[\s\S])*)\1\s*(?:#[\s\S]*)?$/.exec(trimmed); + if (quoted !== null) return quoted[2]; + return trimmed.split(/\s+#/)[0].trim(); +} + +/** + * The name and the rest of one `name: bump` line, or `undefined` when it is + * neither that nor something to skip. + */ +function entryOn(line) { + const quotedName = /^(["'])((?:(?!\1)[\s\S])*)\1\s*:([\s\S]*)$/.exec(line); + if (quotedName !== null) return { name: quotedName[2], rest: quotedName[3] }; + const bareName = /^([^:\s'"]+)\s*:([\s\S]*)$/.exec(line); + if (bareName !== null) return { name: bareName[1], rest: bareName[2] }; + return undefined; +} + +/** + * The `package: bump` pairs a changeset declares, or `undefined` when the file + * is not one this can read. + * + * Hand-parsed rather than handed to `@changesets/parse`, which this repository + * does not depend on and which would be a new dependency for a lint step. The + * subset accepted is deliberately the one Changesets itself writes and reads: + * quoted names (single or double, which every scoped package needs), bare names, + * quoted or bare bumps, blank lines and comments. + * + * The two ways a hand-rolled reader goes wrong are both closed explicitly, + * because each fails in a direction that matters: + * + * - Being STRICTER than Changesets blocks a compliant pull request over a + * spelling the release tooling would have accepted. That is why single quotes, + * quoted bumps and comments are read rather than refused. + * - Being LOOSER lets malformed release metadata reach `main`, where it fails + * the CI-only release workflow after a version PR has already merged. That is + * why the closing delimiter must be exactly `---` on its own line, and why a + * duplicate key is refused rather than silently taking the last one. + * + * A file this cannot read is reported as unreadable rather than as declaring + * nothing: "no releases" and "every release" are opposite answers, and a + * missing-package check would score them the same way. */ export function declaredReleases(fileText) { - const match = /^---\r?\n([\s\S]*?)\r?\n---/.exec(fileText); + const match = /^---[ \t]*\r?\n([\s\S]*?)\r?\n---[ \t]*(?:\r?\n|$)/.exec( + fileText + ); if (match === null) return undefined; const releases = new Map(); for (const line of match[1].split(/\r?\n/)) { const trimmed = line.trim(); - if (trimmed === "") continue; - // Both spellings Changesets accepts for a name: quoted, which every scoped - // package needs, and bare, which an unscoped one may use. - const entry = /^(?:"([^"]+)"|([^:\s]+))\s*:\s*(\S+)\s*$/.exec(trimmed); - if (entry === null) return undefined; - releases.set(entry[1] ?? entry[2], entry[3]); + if (trimmed === "" || trimmed.startsWith("#")) continue; + const entry = entryOn(trimmed); + if (entry === undefined) return undefined; + const name = entry.name.trim(); + const bump = scalar(entry.rest); + if (name === "" || bump === "") return undefined; + // A repeated key is not a reading this can choose between. YAML's own answer + // is to take the last, which would let `"nextly": patch` sit above + // `"nextly": major` and report the file as compliant. + if (releases.has(name)) return undefined; + releases.set(name, bump); } return releases; } @@ -107,8 +205,14 @@ export function problemsWith(path, fileText, packages) { return problems; } -/** Every problem across the given changeset paths. */ -export function checkChangesets(paths, readFile, configText) { +/** + * Every problem: the group's own integrity first, then each changeset against it. + * + * The group is checked even when the pull request touches no changeset at all, + * 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); if (packages.length === 0) { // A config with no fixed group would make every check below vacuous, and a @@ -117,7 +221,10 @@ export function checkChangesets(paths, readFile, configText) { ".changeset/config.json declares no `fixed` group, so nothing here can be checked.", ]; } - return paths.flatMap(path => problemsWith(path, readFile(path), packages)); + return [ + ...groupMatchesWorkspace(packages, workspaceNames), + ...paths.flatMap(path => problemsWith(path, readFile(path), packages)), + ]; } /** @@ -163,17 +270,21 @@ async function readStdin() { async function main(argv) { const paths = pathsToCheck(argv, await readStdin()); - if (paths.length === 0) { - console.log("No changesets added or edited; nothing to check."); - return 0; - } + // No early return on an empty list. The group's own integrity still has to be + // checked, and the pull request that adds a package is often the one that + // touches no changeset at all. const problems = checkChangesets( paths, path => readFileSync(resolve(REPO_ROOT, path), "utf8"), - readFileSync(resolve(REPO_ROOT, ".changeset", "config.json"), "utf8") + readFileSync(resolve(REPO_ROOT, ".changeset", "config.json"), "utf8"), + getWorkspacePackageNames() ); if (problems.length === 0) { - console.log(`Checked ${paths.length} changeset(s): all cover the group.`); + console.log( + paths.length === 0 + ? "No changesets added or edited; the lockstep group matches the workspace." + : `Checked ${paths.length} changeset(s): all cover the group.` + ); return 0; } for (const problem of problems) console.error(`✖ ${problem}`); diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index 56113cc29d..3b04f5a001 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { checkChangesets, declaredReleases, + groupMatchesWorkspace, lockstepPackages, pathsToCheck, problemsWith, @@ -31,7 +32,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, "{}"); + const problems = checkChangesets(["a.md"], () => COMPLETE, "{}", PACKAGES); expect(problems).toHaveLength(1); expect(problems[0]).toContain("no `fixed` group"); }); @@ -121,6 +122,93 @@ 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([]); + }); + + 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", + ]); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("@nextlyhq/newcomer"); + expect(problems[0]).toContain("missing"); + }); + + it("catches a name in the group that no package answers to", () => { + // A rename or a deletion the config still believes in. Changesets refuses a + // release on an unknown package in `fixed`. + const problems = groupMatchesWorkspace( + [...PACKAGES, "@nextlyhq/departed"], + [...PACKAGES] + ); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("@nextlyhq/departed"); + }); + + 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", + ]); + + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("@nextlyhq/newcomer"); + }); +}); + +describe("frontmatter spellings Changesets itself accepts", () => { + it("reads single-quoted names", () => { + // Refusing this would block a compliant PR over a spelling the release + // tooling reads without complaint. + const single = `---\n'nextly': patch\n'@nextlyhq/ui': patch\n'@nextlyhq/builder': patch\n---\n\nBody.\n`; + expect(problemsWith("a.md", single, PACKAGES)).toEqual([]); + }); + + it("reads a quoted bump", () => { + const quotedBump = `---\n"nextly": "patch"\n"@nextlyhq/ui": 'patch'\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; + expect(problemsWith("a.md", quotedBump, PACKAGES)).toEqual([]); + }); + + it("reads comments, whole-line and trailing", () => { + const commented = `---\n# why this exists\n"nextly": patch # the core\n"@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; + expect(problemsWith("a.md", commented, PACKAGES)).toEqual([]); + }); +}); + +describe("frontmatter Changesets itself would refuse", () => { + it("refuses a closing delimiter with anything after it", () => { + // `---junk` does not close the block, so everything below it is frontmatter + // as far as Changesets is concerned. A reader that stops at the first three + // dashes reports the file as complete while the release tooling rejects it. + const junk = `---\n"nextly": patch\n"@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---junk\n\nBody.\n`; + expect(declaredReleases(junk)).toBeUndefined(); + }); + + it("refuses a duplicate key rather than taking the last one", () => { + // YAML's own answer is last-wins, which would let a compliant-looking + // `"nextly": patch` sit above a `"nextly": major` that decides the release. + const duplicated = `---\n"nextly": patch\n"nextly": major\n"@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; + expect(declaredReleases(duplicated)).toBeUndefined(); + }); + + it("refuses an entry with no bump at all", () => { + const noBump = `---\n"nextly":\n"@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; + expect(declaredReleases(noBump)).toBeUndefined(); + }); +}); + describe("a file this cannot read", () => { it("is reported rather than treated as naming nothing wrong", () => { // The failure mode worth naming: a tolerant parser answers "no releases" for @@ -187,7 +275,8 @@ describe("what the check is pointed at", () => { const problems = checkChangesets( ["good.md", "bad.md"], path => files[path], - CONFIG + CONFIG, + PACKAGES ); expect(problems).toHaveLength(1); @@ -197,6 +286,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)).toEqual([]); + expect(checkChangesets([], () => "", CONFIG, PACKAGES)).toEqual([]); }); }); diff --git a/scripts/release/lib.mjs b/scripts/release/lib.mjs index 5eef93c3f2..30607c749a 100644 --- a/scripts/release/lib.mjs +++ b/scripts/release/lib.mjs @@ -88,6 +88,28 @@ 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 = []; From eb7a3c0167986659fd43d633860162207045c822 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 15:49:02 +0500 Subject: [PATCH 3/7] ci(root): read changesets with the parser the release uses --- .github/workflows/ci.yml | 12 +- package.json | 1 + pnpm-lock.yaml | 5 +- scripts/release/check-changesets.mjs | 184 ++++++++++++++-------- scripts/release/check-changesets.test.mjs | 92 ++++++++++- 5 files changed, 219 insertions(+), 75 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5ec480da34..8872bd3b04 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -128,12 +128,14 @@ jobs: # in some other checkout shape, a broken object store — aborts the step # under `set -e`. Inside a pipeline with a trailing `|| true` it would # not: the checker would read empty stdin, report nothing to check, and - # the gate would be off while the step stayed green. Only `grep`'s - # no-match status is suppressed, which is the one expected failure here. + # the gate would be off while the step stayed green. + # + # Which of those paths is a changeset is decided by the checker, using + # the same filter `@changesets/read` applies. A `grep` here would be a + # second answer, and it would differ from the release tooling on a + # lowercase `readme.md` or a hidden `.template.md`. touched="$(git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md')" - printf '%s\n' "$touched" \ - | { grep -v '^\.changeset/README\.md$' || true; } \ - | node scripts/release/check-changesets.mjs + printf '%s\n' "$touched" | node scripts/release/check-changesets.mjs # Enforce the design-token theming contract in the admin + plugin packages # (no hsl(var(--x)) wrappers, no hardcoded colors, no stray !important). diff --git a/package.json b/package.json index 0c5ce2f6bc..1c65c404a8 100644 --- a/package.json +++ b/package.json @@ -80,6 +80,7 @@ "@aws-sdk/lib-storage": "^3.1090.0", "@changesets/changelog-github": "^0.5.1", "@changesets/cli": "^2.27.10", + "@changesets/parse": "^0.4.3", "@commitlint/cli": "^20.1.0", "@commitlint/config-conventional": "^20.0.0", "@testing-library/jest-dom": "^6.9.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2158851d96..94a3bcf585 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -75,6 +75,9 @@ importers: '@changesets/cli': specifier: ^2.27.10 version: 2.30.0(@types/node@24.10.0) + '@changesets/parse': + specifier: ^0.4.3 + version: 0.4.3 '@commitlint/cli': specifier: ^20.1.0 version: 20.1.0(@types/node@24.10.0)(typescript@5.9.3) @@ -13398,7 +13401,7 @@ snapshots: '@humanfs/node': 0.16.6 '@humanwhocodes/module-importer': 1.0.1 '@humanwhocodes/retry': 0.4.3 - '@types/estree': 1.0.9 + '@types/estree': 1.0.8 ajv: 6.15.0 chalk: 4.1.2 cross-spawn: 7.0.6 diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs index 385566c2cd..67d5535355 100644 --- a/scripts/release/check-changesets.mjs +++ b/scripts/release/check-changesets.mjs @@ -28,6 +28,8 @@ import { readFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import parseChangeset from "@changesets/parse"; + import { getWorkspacePackageNames } from "./lib.mjs"; const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); @@ -41,6 +43,59 @@ const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); */ const ALPHA_BUMP = "patch"; +/** + * Whether `fixed` has the SHAPE Changesets requires, as a list of sentences. + * + * Flattening answers the same set for `[["a","b"]]` and for `["a","b"]`, so a + * config hand-edited into the flat form produces exactly the expected names here + * and is refused by the release tooling. Same for a package repeated across two + * groups: flattening hides it, `@changesets/config` rejects it. Either way the + * malformed config merges and surfaces only when a release is attempted. + * + * 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 and fully stated by them. + * That is the opposite trade to the YAML reader, and for the opposite reason: + * this rule cannot drift, and a frontmatter grammar can. + */ +export function fixedGroupShape(config) { + const groups = config.fixed; + if (groups === undefined) return []; + if (!Array.isArray(groups)) { + return ['.changeset/config.json: `fixed` must be an array of arrays.']; + } + const problems = []; + const flat = groups.filter(group => !Array.isArray(group)); + if (flat.length > 0) { + problems.push( + '.changeset/config.json: `fixed` must be an array of ARRAYS — a group per ' + + `line, not a flat list of names. Found ${JSON.stringify(flat[0])} where a group was expected.` + ); + return problems; + } + const seen = new Set(); + const repeated = new Set(); + for (const group of groups) { + for (const name of group) { + if (typeof name !== "string" || name === "") { + problems.push( + `.changeset/config.json: \`fixed\` holds ${JSON.stringify(name)}, which is not a package name.` + ); + continue; + } + if (seen.has(name)) repeated.add(name); + seen.add(name); + } + } + if (repeated.size > 0) { + problems.push( + `.changeset/config.json: \`fixed\` lists ${[...repeated].join(", ")} in more than one ` + + `group. The groups must be disjoint; a package cannot version in lockstep with two sets.` + ); + } + return problems; +} + /** Every package that must appear in a changeset, read from the Changesets config. */ export function lockstepPackages(configText) { const config = JSON.parse(configText); @@ -88,77 +143,37 @@ export function groupMatchesWorkspace(packages, workspaceNames) { } /** - * One scalar as YAML would read it: quotes stripped, a trailing comment removed. - * - * A quoted value keeps everything inside the quotes, `#` included, because a - * comment cannot start inside a scalar. Only an unquoted value has a comment to - * strip, and only when the `#` is preceded by whitespace — `patch#1` is one - * token, `patch # note` is a value and a note. - */ -function scalar(raw) { - const trimmed = raw.trim(); - const quoted = /^(["'])((?:(?!\1)[\s\S])*)\1\s*(?:#[\s\S]*)?$/.exec(trimmed); - if (quoted !== null) return quoted[2]; - return trimmed.split(/\s+#/)[0].trim(); -} - -/** - * The name and the rest of one `name: bump` line, or `undefined` when it is - * neither that nor something to skip. - */ -function entryOn(line) { - const quotedName = /^(["'])((?:(?!\1)[\s\S])*)\1\s*:([\s\S]*)$/.exec(line); - if (quotedName !== null) return { name: quotedName[2], rest: quotedName[3] }; - const bareName = /^([^:\s'"]+)\s*:([\s\S]*)$/.exec(line); - if (bareName !== null) return { name: bareName[1], rest: bareName[2] }; - return undefined; -} - -/** - * The `package: bump` pairs a changeset declares, or `undefined` when the file - * is not one this can read. - * - * Hand-parsed rather than handed to `@changesets/parse`, which this repository - * does not depend on and which would be a new dependency for a lint step. The - * subset accepted is deliberately the one Changesets itself writes and reads: - * quoted names (single or double, which every scoped package needs), bare names, - * quoted or bare bumps, blank lines and comments. + * The `package: bump` pairs a changeset declares, or `undefined` when Changesets + * itself would refuse the file. * - * The two ways a hand-rolled reader goes wrong are both closed explicitly, - * because each fails in a direction that matters: + * Handed to `@changesets/parse` rather than read here. A hand-rolled reader was + * tried and corrected twice — once for being STRICTER than Changesets (single + * quotes, quoted bumps and comments are valid YAML it was rejecting, which + * blocks a compliant pull request) and once for being LOOSER (`---junk` as a + * closing delimiter, and duplicate keys, which it accepted and the release + * tooling does not). A third such correction would have been the third instance + * of one shape, so the reading moved to the library that decides the real answer + * instead. Anything this accepts, the release accepts, by construction rather + * than by agreement. * - * - Being STRICTER than Changesets blocks a compliant pull request over a - * spelling the release tooling would have accepted. That is why single quotes, - * quoted bumps and comments are read rather than refused. - * - Being LOOSER lets malformed release metadata reach `main`, where it fails - * the CI-only release workflow after a version PR has already merged. That is - * why the closing delimiter must be exactly `---` on its own line, and why a - * duplicate key is refused rather than silently taking the last one. + * `@changesets/parse` was already in the tree as a transitive dependency of + * `@changesets/cli`; declaring it changes what resolves, not what is installed. * - * A file this cannot read is reported as unreadable rather than as declaring - * nothing: "no releases" and "every release" are opposite answers, and a - * missing-package check would score them the same way. + * A file it refuses is reported as unreadable rather than as declaring nothing: + * "no releases" and "every release" are opposite answers, and a missing-package + * check would score them the same way. That distinction is why the empty case is + * still called out separately below — `parse` returns an empty release list for a + * frontmatter that is well-formed and says nothing. */ export function declaredReleases(fileText) { - const match = /^---[ \t]*\r?\n([\s\S]*?)\r?\n---[ \t]*(?:\r?\n|$)/.exec( - fileText - ); - if (match === null) return undefined; - const releases = new Map(); - for (const line of match[1].split(/\r?\n/)) { - const trimmed = line.trim(); - if (trimmed === "" || trimmed.startsWith("#")) continue; - const entry = entryOn(trimmed); - if (entry === undefined) return undefined; - const name = entry.name.trim(); - const bump = scalar(entry.rest); - if (name === "" || bump === "") return undefined; - // A repeated key is not a reading this can choose between. YAML's own answer - // is to take the last, which would let `"nextly": patch` sit above - // `"nextly": major` and report the file as compliant. - if (releases.has(name)) return undefined; - releases.set(name, bump); + let parsed; + try { + parsed = parseChangeset(fileText); + } catch { + return undefined; } + const releases = new Map(); + for (const release of parsed.releases) releases.set(release.name, release.type); return releases; } @@ -177,6 +192,16 @@ export function problemsWith(path, fileText, packages) { `A changeset opens with \`---\`, one \`"package": bump\` per line, and closes with \`---\`.`, ]; } + if (releases.size === 0) { + // Well-formed and saying nothing. Reported on its own rather than as "missing + // all of them", because the cause is different — an empty frontmatter is a + // changeset someone forgot to fill in, not one generated against an older + // group — and the fix a reader needs is not the same. + return [ + `${path}: declares no packages at all. A changeset that releases nothing ` + + `still consumes a file name and produces no changelog entry.`, + ]; + } const problems = []; const missing = packages.filter(name => !releases.has(name)); if (missing.length > 0) { @@ -221,6 +246,12 @@ export function checkChangesets(paths, readFile, configText, workspaceNames) { ".changeset/config.json declares no `fixed` group, so nothing here can be checked.", ]; } + const shape = fixedGroupShape(JSON.parse(configText)); + // Returned alone. Every check below reads the flattened group, and a group + // whose shape is wrong flattens to something that looks right — so reporting + // the downstream answers beside it would be reporting answers derived from a + // reading the release tooling does not share. + if (shape.length > 0) return shape; return [ ...groupMatchesWorkspace(packages, workspaceNames), ...paths.flatMap(path => problemsWith(path, readFile(path), packages)), @@ -240,13 +271,32 @@ export function checkChangesets(paths, readFile, configText, workspaceNames) { * an empty argument list has to survive shell expansion under `set -u` to get * here at all. */ +/** + * Whether a path names a file Changesets would READ as a changeset. + * + * Copied from `@changesets/read`, whose filter is + * `!file.startsWith(".") && file.endsWith(".md") && !/^README\.md$/i.test(file)`. + * Anything else in `.changeset/` is documentation or a helper — a template, a + * README in any casing — and passing one of those to a frontmatter check rejects + * ordinary docs as malformed. + * + * Applied HERE rather than in the workflow's `grep`, so a hand-run and the build + * agree about what a changeset is. A shell filter is one caller's answer. + */ +function isChangesetFile(path) { + const name = path.slice(path.lastIndexOf("/") + 1); + return ( + !name.startsWith(".") && name.endsWith(".md") && !/^README\.md$/i.test(name) + ); +} + export function pathsToCheck(argv, stdinText) { - const fromArgv = argv.filter(path => path.endsWith(".md")); + const fromArgv = argv.filter(isChangesetFile); if (fromArgv.length > 0) return fromArgv; return stdinText .split("\n") .map(line => line.trim()) - .filter(line => line.endsWith(".md")); + .filter(isChangesetFile); } /** diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index 3b04f5a001..0e6fed0dbc 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { checkChangesets, declaredReleases, + fixedGroupShape, groupMatchesWorkspace, lockstepPackages, pathsToCheck, @@ -168,7 +169,7 @@ describe("the group against the workspace", () => { }); }); -describe("frontmatter spellings Changesets itself accepts", () => { +describe("frontmatter spellings the release tooling accepts", () => { it("reads single-quoted names", () => { // Refusing this would block a compliant PR over a spelling the release // tooling reads without complaint. @@ -187,7 +188,7 @@ describe("frontmatter spellings Changesets itself accepts", () => { }); }); -describe("frontmatter Changesets itself would refuse", () => { +describe("frontmatter the release tooling refuses", () => { it("refuses a closing delimiter with anything after it", () => { // `---junk` does not close the block, so everything below it is frontmatter // as far as Changesets is concerned. A reader that stops at the first three @@ -203,6 +204,25 @@ describe("frontmatter Changesets itself would refuse", () => { expect(declaredReleases(duplicated)).toBeUndefined(); }); + it("refuses inconsistent indentation", () => { + // Valid-looking to any reader that trims each line, and rejected by the + // release tooling as `bad indentation of a mapping entry`. A file like this + // merging is a release that fails after a version PR is already in. + const indented = `---\n"nextly": patch\n "@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; + expect(declaredReleases(indented)).toBeUndefined(); + }); + + it("reports a well-formed changeset that names nothing", () => { + // Distinct from unreadable: the file parses, it just releases nothing. The + // cause is a changeset someone forgot to fill in rather than one generated + // against an older group, so it gets its own sentence. + const empty = `---\n---\n\nBody.\n`; + expect(declaredReleases(empty)?.size).toBe(0); + expect(problemsWith("a.md", empty, PACKAGES)).toEqual([ + expect.stringContaining("declares no packages at all"), + ]); + }); + it("refuses an entry with no bump at all", () => { const noBump = `---\n"nextly":\n"@nextlyhq/ui": patch\n"@nextlyhq/builder": patch\n---\n\nBody.\n`; expect(declaredReleases(noBump)).toBeUndefined(); @@ -236,6 +256,74 @@ describe("a file this cannot read", () => { }); }); +describe("the shape of the fixed group itself", () => { + it("accepts the array of arrays Changesets requires", () => { + expect(fixedGroupShape({ fixed: [["a", "b"], ["c"]] })).toEqual([]); + // No `fixed` at all is not a shape error; the caller reports it separately. + expect(fixedGroupShape({})).toEqual([]); + }); + + it("refuses a flat array", () => { + // Flattening answers the same set for `[["a","b"]]` and `["a","b"]`, so every + // check downstream reports success while the release tooling refuses the file. + const problems = fixedGroupShape({ fixed: ["a", "b"] }); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("array of ARRAYS"); + }); + + it("refuses a package repeated across groups", () => { + const problems = fixedGroupShape({ fixed: [["a", "b"], ["b"]] }); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("more than one"); + expect(problems[0]).toContain("b"); + }); + + it("refuses a group holding something that is not a name", () => { + const problems = fixedGroupShape({ fixed: [["a", 7]] }); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("not a package name"); + }); + + it("reports the shape ALONE, without answers derived from it", () => { + // A group whose shape is wrong flattens to something that looks right, so + // reporting the coverage answers beside it would be reporting a reading the + // release tooling does not share. + const problems = checkChangesets( + [], + () => "", + JSON.stringify({ fixed: ["nextly", "@nextlyhq/ui"] }), + ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"] + ); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("array of ARRAYS"); + }); +}); + +describe("which files count as changesets", () => { + it("skips what Changesets itself skips", () => { + // `@changesets/read` filters on + // `!startsWith(".") && endsWith(".md") && !/^README\.md$/i`. A docs-only PR + // touching any of these must not be told its documentation is malformed + // frontmatter. + expect( + pathsToCheck( + [], + [ + ".changeset/real-one.md", + ".changeset/README.md", + ".changeset/readme.md", + ".changeset/.template.md", + ".changeset/config.json", + ].join("\n") + ) + ).toEqual([".changeset/real-one.md"]); + }); + + it("applies the same rule to arguments", () => { + expect(pathsToCheck([".changeset/readme.md"], "")).toEqual([]); + }); +}); + describe("where the file list comes from", () => { it("prefers arguments when it is given them", () => { expect(pathsToCheck(["a.md", "b.md"], "never.md")).toEqual([ From 1e4549eff1a5783f49f01cffb7f34777676c8fce Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 16:01:10 +0500 Subject: [PATCH 4/7] ci(root): expand fixed-group globs and require a single group --- .github/workflows/ci.yml | 8 +- package.json | 1 + pnpm-lock.yaml | 3 + scripts/release/check-changesets.mjs | 137 +++++++++++++--------- scripts/release/check-changesets.test.mjs | 42 +++++-- 5 files changed, 124 insertions(+), 67 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8872bd3b04..7c9c295fcb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,10 +104,10 @@ jobs: # The published packages version in lockstep, so a changeset that names # only some of them still bumps them all — and the ones it left out get a - # version with no changelog entry beside it. Reviewers caught that twice in - # one afternoon, both times on frontmatter that HAD been generated from the - # config, just before a new package joined the group or before the branch - # merged the commit that added it. + # version with no changelog entry beside it. Generating the frontmatter from + # the config is not enough on its own, because the group grows: a list + # generated before a package joined it is complete against the config it was + # read from and short against the current one. # # Scoped to the changesets THIS pull request adds or edits. Several hundred # are pending on `main`, most written before the group grew; rewriting them diff --git a/package.json b/package.json index 1c65c404a8..9cf7f4ae81 100644 --- a/package.json +++ b/package.json @@ -92,6 +92,7 @@ "husky": "^9.1.7", "jsdom": "^27.1.0", "lint-staged": "^16.2.6", + "micromatch": "^4.0.8", "prettier": "^3.6.2", "publint": "^0.3.18", "sherif": "1.13.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 94a3bcf585..4c85d34175 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -111,6 +111,9 @@ importers: lint-staged: specifier: ^16.2.6 version: 16.2.6 + micromatch: + specifier: ^4.0.8 + version: 4.0.8 prettier: specifier: ^3.6.2 version: 3.6.2 diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs index 67d5535355..ffa8188574 100644 --- a/scripts/release/check-changesets.mjs +++ b/scripts/release/check-changesets.mjs @@ -1,34 +1,35 @@ -// Refuses a changeset that does not cover every package in the lockstep group. +// Refuses a changeset that does not cover every package in the lockstep group, +// and a `fixed` group that no longer describes the workspace. // // The packages version together, so a release advances all of them whatever a // single changeset lists. What an incomplete one loses is the CHANGELOG: a // package left out of the frontmatter gets a version bump with no entry // explaining it, and the note that should have appeared under it is filed only -// under the packages that were named. The release is still correct; the record -// of it is not. +// under the packages that were named. The release is correct; the record of it +// is not. // -// This existed as a review convention and was caught by a reviewer twice in one -// afternoon, both times on a changeset that HAD been generated from the config — -// once written before a new package joined the group, once written before the -// branch merged the commit that added it. A convention that depends on -// regenerating at the right moment is one a build should check instead. +// Generating the frontmatter from `.changeset/config.json` is not by itself +// enough, because the group grows: a list generated before a package joined it, +// or before the branch merged the commit that added it, is complete against the +// config it was read from and short against the current one. Only a check +// against the config as it stands at build time closes that. +// +// The GROUP is checked too, and against the workspace rather than against +// itself. A checker reading the config as the source of truth cannot see a +// package added under `packages/` and never added to `fixed`, which is the drift +// that makes every changeset written afterwards wrong while each of them passes. // // Scoped to the changesets a branch ADDS or EDITS, never the backlog. Several // hundred are pending on `main`, most written before the group grew, and // rewriting them to satisfy a rule they predate would put churn in front of // every reader of the eventual changelog for no gain. -// -// The GROUP is checked too, and against the workspace rather than against -// itself. A checker that reads `.changeset/config.json` as the source of truth -// cannot see a package added under `packages/` and never added to `fixed`, which -// is the drift that makes every changeset written afterwards wrong while each of -// them passes. import { readFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import parseChangeset from "@changesets/parse"; +import micromatch from "micromatch"; import { getWorkspacePackageNames } from "./lib.mjs"; @@ -44,62 +45,87 @@ const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); const ALPHA_BUMP = "patch"; /** - * Whether `fixed` has the SHAPE Changesets requires, as a list of sentences. + * Whether `fixed` has the shape THIS repository requires, as a list of sentences. + * + * Changesets allows several disjoint groups; this repository has exactly one, and + * the difference is not cosmetic. Flattening answers the same set for one group + * and for two, so a config split into two groups reads as complete here while the + * packages no longer version together — the next release can advance one group + * and leave the other behind, which is precisely the outcome every check below + * exists to prevent. * - * Flattening answers the same set for `[["a","b"]]` and for `["a","b"]`, so a - * config hand-edited into the flat form produces exactly the expected names here - * and is refused by the release tooling. Same for a package repeated across two - * groups: flattening hides it, `@changesets/config` rejects it. Either way the - * malformed config merges and surfaces only when a release is attempted. + * The other refusals are shape: a flat `["a", "b"]` flattens identically to + * `[["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 and fully stated by them. - * That is the opposite trade to the YAML reader, and for the opposite reason: - * this rule cannot drift, and a frontmatter grammar can. + * 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. */ export function fixedGroupShape(config) { const groups = config.fixed; if (groups === undefined) return []; if (!Array.isArray(groups)) { - return ['.changeset/config.json: `fixed` must be an array of arrays.']; + return ["`.changeset/config.json`: `fixed` must be an array of arrays."]; } - const problems = []; const flat = groups.filter(group => !Array.isArray(group)); if (flat.length > 0) { - problems.push( - '.changeset/config.json: `fixed` must be an array of ARRAYS — a group per ' + - `line, not a flat list of names. Found ${JSON.stringify(flat[0])} where a group was expected.` - ); - return problems; + return [ + ".changeset/config.json: `fixed` must be an array of ARRAYS — a group per " + + `line, not a flat list of names. Found ${JSON.stringify(flat[0])} where a group was expected.`, + ]; } - const seen = new Set(); - const repeated = new Set(); + if (groups.length > 1) { + return [ + `.changeset/config.json: \`fixed\` declares ${groups.length} groups. This ` + + `repository versions every package as ONE group; split into two, a release ` + + `can advance one and leave the other behind while every changeset still passes.`, + ]; + } + const problems = []; for (const group of groups) { for (const name of group) { if (typeof name !== "string" || name === "") { problems.push( `.changeset/config.json: \`fixed\` holds ${JSON.stringify(name)}, which is not a package name.` ); - continue; } - if (seen.has(name)) repeated.add(name); - seen.add(name); } } - if (repeated.size > 0) { - problems.push( - `.changeset/config.json: \`fixed\` lists ${[...repeated].join(", ")} in more than one ` + - `group. The groups must be disjoint; a package cannot version in lockstep with two sets.` - ); - } return problems; } -/** Every package that must appear in a changeset, read from the Changesets config. */ -export function lockstepPackages(configText) { - const config = JSON.parse(configText); - return (config.fixed ?? []).flat(); +/** + * Every package that must appear in a changeset: the `fixed` group with any glob + * expanded against the workspace. + * + * `fixed` accepts patterns as well as names — `@nextlyhq/*` is a valid way to + * write this group — and `@changesets/config` expands each entry with + * `micromatch.isMatch(packageName, entry)` before it validates anything. A + * checker comparing the raw entries would report every matching package as + * missing and the pattern itself as unknown, which on a config written that way + * means rejecting every pull request. + * + * The same matcher the release tooling uses, for the same reason the frontmatter + * goes through the same parser: a second implementation of someone else's + * grammar is a second answer. + */ +export function lockstepPackages(configText, workspaceNames) { + const entries = (JSON.parse(configText).fixed ?? []).flat(); + const expanded = new Set(); + for (const entry of entries) { + if (typeof entry !== "string") continue; + const matched = workspaceNames.filter(name => + micromatch.isMatch(name, entry) + ); + // An entry matching nothing is kept as written, so the group check reports it + // by the name an author would search for rather than silently dropping it. + if (matched.length === 0) expanded.add(entry); + for (const name of matched) expanded.add(name); + } + return [...expanded]; } /** @@ -146,15 +172,14 @@ export function groupMatchesWorkspace(packages, workspaceNames) { * The `package: bump` pairs a changeset declares, or `undefined` when Changesets * itself would refuse the file. * - * Handed to `@changesets/parse` rather than read here. A hand-rolled reader was - * tried and corrected twice — once for being STRICTER than Changesets (single - * quotes, quoted bumps and comments are valid YAML it was rejecting, which - * blocks a compliant pull request) and once for being LOOSER (`---junk` as a - * closing delimiter, and duplicate keys, which it accepted and the release - * tooling does not). A third such correction would have been the third instance - * of one shape, so the reading moved to the library that decides the real answer - * instead. Anything this accepts, the release accepts, by construction rather - * than by agreement. + * Handed to `@changesets/parse` rather than read here, so that anything this + * accepts the release accepts, by construction rather than by agreement. A + * frontmatter grammar has two ways to be wrong and both matter: reading it more + * STRICTLY than Changesets blocks a compliant pull request over a spelling the + * release tooling takes (single quotes, quoted bumps, comments), and reading it + * more LOOSELY lets malformed metadata merge and fail the CI-only release + * afterwards (`---junk` as a closing delimiter, duplicate keys, inconsistent + * indentation). Neither margin exists when the same parser decides both. * * `@changesets/parse` was already in the tree as a transitive dependency of * `@changesets/cli`; declaring it changes what resolves, not what is installed. @@ -182,7 +207,7 @@ export function declaredReleases(fileText) { * * Returns every problem rather than the first, so one push answers all of them. * A guard that reports one missing package at a time turns a stale frontmatter - * into as many CI rounds as it has gaps. + * into as many build attempts as it has gaps. */ export function problemsWith(path, fileText, packages) { const releases = declaredReleases(fileText); @@ -238,7 +263,7 @@ export function problemsWith(path, fileText, packages) { * group makes every later changeset wrong while each of them passes. */ export function checkChangesets(paths, readFile, configText, workspaceNames) { - const packages = lockstepPackages(configText); + const packages = lockstepPackages(configText, workspaceNames); 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. diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index 0e6fed0dbc..5b04863bab 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -14,7 +14,8 @@ import { const CONFIG = JSON.stringify({ fixed: [["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]], }); -const PACKAGES = lockstepPackages(CONFIG); +const WORKSPACE = ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]; +const PACKAGES = lockstepPackages(CONFIG, WORKSPACE); /** A changeset naming the given `package: bump` pairs. */ function changeset(pairs, body = "Something changed.") { @@ -257,8 +258,8 @@ describe("a file this cannot read", () => { }); describe("the shape of the fixed group itself", () => { - it("accepts the array of arrays Changesets requires", () => { - expect(fixedGroupShape({ fixed: [["a", "b"], ["c"]] })).toEqual([]); + it("accepts the one array of names this repository uses", () => { + expect(fixedGroupShape({ fixed: [["a", "b", "c"]] })).toEqual([]); // No `fixed` at all is not a shape error; the caller reports it separately. expect(fixedGroupShape({})).toEqual([]); }); @@ -271,11 +272,13 @@ describe("the shape of the fixed group itself", () => { expect(problems[0]).toContain("array of ARRAYS"); }); - it("refuses a package repeated across groups", () => { - const problems = fixedGroupShape({ fixed: [["a", "b"], ["b"]] }); + it("refuses more than one group", () => { + // Changesets allows several disjoint groups; this repository has exactly one. + // Flattening answers the same set either way, so a split config reads as + // complete while a release can advance one group and leave the other behind. + const problems = fixedGroupShape({ fixed: [["a", "b"], ["c"]] }); expect(problems).toHaveLength(1); - expect(problems[0]).toContain("more than one"); - expect(problems[0]).toContain("b"); + expect(problems[0]).toContain("2 groups"); }); it("refuses a group holding something that is not a name", () => { @@ -299,6 +302,31 @@ describe("the shape of the fixed group itself", () => { }); }); +describe("a group written with globs", () => { + it("expands a pattern against the workspace", () => { + // `fixed` accepts patterns, and `@changesets/config` expands each entry with + // `micromatch.isMatch` before validating. Comparing the raw entries would + // 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() + ); + }); + + 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/*"]); + }); + + 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([]); + }); +}); + describe("which files count as changesets", () => { it("skips what Changesets itself skips", () => { // `@changesets/read` filters on From 7adeba1ac04399da1acbe696c8b3dec4821c05ff Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 18:44:14 +0500 Subject: [PATCH 5/7] ci(root): expand the fixed group as one pattern list --- scripts/release/check-changesets.mjs | 45 +++++++++++------------ scripts/release/check-changesets.test.mjs | 11 ++++++ 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs index ffa8188574..b21fd6d510 100644 --- a/scripts/release/check-changesets.mjs +++ b/scripts/release/check-changesets.mjs @@ -98,34 +98,31 @@ export function fixedGroupShape(config) { } /** - * Every package that must appear in a changeset: the `fixed` group with any glob - * expanded against the workspace. + * Every package that must appear in a changeset: the `fixed` group expanded + * against the workspace. * - * `fixed` accepts patterns as well as names — `@nextlyhq/*` is a valid way to - * write this group — and `@changesets/config` expands each entry with - * `micromatch.isMatch(packageName, entry)` before it validates anything. A - * checker comparing the raw entries would report every matching package as - * missing and the pattern itself as unknown, which on a config written that way - * means rejecting every pull request. + * `fixed` accepts patterns as well as names, and the whole group goes to + * `micromatch` in ONE call — `micromatch(packageNames, group)` — because that is + * what `@changesets/config` does and the list is not a union of independent + * patterns. A negated entry only means anything in company: `["**", + * "!@nextlyhq/builder"]` excludes builder, while evaluating each pattern alone + * and unioning the results lets `**` put it straight back. * - * The same matcher the release tooling uses, for the same reason the frontmatter - * goes through the same parser: a second implementation of someone else's - * grammar is a second answer. + * A non-negated entry matching nothing is kept as written, so a typo or a + * departed package is reported by the string an author would search for. */ export function lockstepPackages(configText, workspaceNames) { - const entries = (JSON.parse(configText).fixed ?? []).flat(); - const expanded = new Set(); - for (const entry of entries) { - if (typeof entry !== "string") continue; - const matched = workspaceNames.filter(name => - micromatch.isMatch(name, entry) - ); - // An entry matching nothing is kept as written, so the group check reports it - // by the name an author would search for rather than silently dropping it. - if (matched.length === 0) expanded.add(entry); - for (const name of matched) expanded.add(name); - } - return [...expanded]; + const group = (JSON.parse(configText).fixed ?? []) + .flat() + .filter(entry => typeof entry === "string"); + if (group.length === 0) return []; + const matched = micromatch(workspaceNames, group); + const unmatched = group.filter( + entry => + !entry.startsWith("!") && + !workspaceNames.some(name => micromatch.isMatch(name, entry)) + ); + return [...new Set([...matched, ...unmatched])]; } /** diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index 5b04863bab..e3e17cc1e3 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -314,6 +314,17 @@ describe("a group written with globs", () => { ); }); + it("honours a negated entry, which only means anything in company", () => { + // `micromatch` is given the WHOLE group in one call, the way + // `@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([ + "@nextlyhq/ui", + "nextly", + ]); + }); + 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/*"]); From 4a52dee7190782a25a7cde23577ae204a81c7831 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 19:03:50 +0500 Subject: [PATCH 6/7] ci(root): read only the changesets the release reads --- .github/workflows/ci.yml | 7 ++++++- scripts/release/check-changesets.test.mjs | 6 +++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1a8ebb76e4..2cb8804938 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -156,7 +156,12 @@ jobs: # the same filter `@changesets/read` applies. A `grep` here would be a # second answer, and it would differ from the release tooling on a # lowercase `readme.md` or a hidden `.template.md`. - touched="$(git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md')" + # + # The pathspec excludes subdirectories because git matches `*` across + # `/` while `@changesets/read` lists `.changeset` non-recursively — so + # a file under `.changeset/archive/` is something the release never + # reads, and checking it would fail a pull request over a note. + touched="$(git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md' ':(exclude).changeset/*/**')" printf '%s\n' "$touched" | node scripts/release/check-changesets.mjs # Enforce the design-token theming contract in the admin + plugin packages diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index e3e17cc1e3..58c135e6f2 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -74,9 +74,9 @@ describe("the omission this exists for", () => { }); it("catches the one-package gap a new group member leaves", () => { - // The exact shape reviewers caught twice: a frontmatter generated from the - // config BEFORE a package joined the group, so it is complete against the - // config it was written from and short against the current one. + // The shape a growing group produces: a frontmatter generated from the config + // BEFORE a package joined it, so it is complete against the config it was + // written from and short against the current one. const beforeBuilder = changeset([ ["nextly", "patch"], ["@nextlyhq/ui", "patch"], From e2c03a30f73734e944cce50fd67c17e02b868ec3 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 19:50:57 +0500 Subject: [PATCH 7/7] ci(root): see a changeset replaced by a symlink --- .github/workflows/ci.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2cb8804938..ce12291912 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -157,11 +157,17 @@ jobs: # second answer, and it would differ from the release tooling on a # lowercase `readme.md` or a hidden `.template.md`. # + # `T` sits beside `ACMR` because a changeset converted between a + # regular file and a symlink is reported as a TYPE change, and + # `@changesets/read` follows symlinks — so a release would consume a + # replacement this check never saw. `D` stays out: a deleted changeset + # has nothing left to read. + # # The pathspec excludes subdirectories because git matches `*` across # `/` while `@changesets/read` lists `.changeset` non-recursively — so # a file under `.changeset/archive/` is something the release never # reads, and checking it would fail a pull request over a note. - touched="$(git diff --name-only --diff-filter=ACMR HEAD^1 HEAD -- '.changeset/*.md' ':(exclude).changeset/*/**')" + touched="$(git diff --name-only --diff-filter=ACMRT HEAD^1 HEAD -- '.changeset/*.md' ':(exclude).changeset/*/**')" printf '%s\n' "$touched" | node scripts/release/check-changesets.mjs # Enforce the design-token theming contract in the admin + plugin packages