diff --git a/package.json b/package.json index 45915c97..5293f4e5 100644 --- a/package.json +++ b/package.json @@ -39,6 +39,7 @@ "devDependencies": { "@a11y-lens/cli": "^0.4.1", "@changesets/cli": "^2.31.1", + "@manypkg/get-packages": "1.1.3", "@vitest/coverage-v8": "^4.1.10", "concurrently": "^9.2.4", "eslint": "^10.8.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ef2dca17..b887c8d8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -30,6 +30,9 @@ importers: '@changesets/cli': specifier: ^2.31.1 version: 2.31.1(@types/node@22.20.1) + '@manypkg/get-packages': + specifier: 1.1.3 + version: 1.1.3 '@vitest/coverage-v8': specifier: ^4.1.10 version: 4.1.10(vitest@4.1.10) diff --git a/scripts/__tests__/changesetGateAccuracy.test.mjs b/scripts/__tests__/changesetGateAccuracy.test.mjs index 3850ddfb..6625e617 100644 --- a/scripts/__tests__/changesetGateAccuracy.test.mjs +++ b/scripts/__tests__/changesetGateAccuracy.test.mjs @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { packagesNamedIn, mixedChangesets, manifestChangeShips, shipsToUsers, packagePublishesAt } from '../check-changeset.mjs' +import { packagesNamedIn, mixedChangesets, ignoredOnlyChangesets, manifestChangeShips, shipsToUsers, packagePublishesAt } from '../check-changeset.mjs' const IGNORED = new Set(['@tapflowio/dashboard', '@tapflowio/playground']) const cs = (body) => `---\n${body}\n---\n\nsome note.\n` @@ -32,11 +32,47 @@ describe('mixing an ignored package with a published one', () => { it('leaves a published-only changeset alone', () => { expect(mixedChangesets(['clean.md'], IGNORED, read)).toEqual([]) }) - it('leaves an ignored-only changeset alone — changesets accepts it', () => { + // Not this check's business — the file is not *mixed*. It is still a defect, and a silent one, which + // is why `ignoredOnlyChangesets` below exists. This assertion used to read as though the case were + // fine; it only means `mixedChangesets` is right to pass it on. + it('leaves an ignored-only changeset to the check below', () => { expect(mixedChangesets(['only-ignored.md'], IGNORED, read)).toEqual([]) }) }) +describe('a changeset that names only ignored packages', () => { + // The quiet half. `changeset version` produces no release for it and deletes the file, so published + // source ships with nothing written about it — and every gate was green. It is the mistake the root + // AGENTS.md warns about by name: a dashboard change must name `@tapflowio/relay`. + const read = (f) => ({ + 'only-ignored.md': cs('"@tapflowio/dashboard": patch'), + 'both-ignored.md': cs('"@tapflowio/dashboard": patch\n"@tapflowio/playground": patch'), + 'clean.md': cs('"@tapflowio/relay": patch'), + 'mixed.md': cs('"@tapflowio/relay": patch\n"@tapflowio/dashboard": patch'), + 'no-frontmatter.md': 'just prose\n', + })[f] + + it('flags a changeset whose only package is ignored', () => { + expect(ignoredOnlyChangesets(['only-ignored.md'], IGNORED, read)).toEqual(['only-ignored.md']) + }) + it('flags one that names several, all ignored', () => { + expect(ignoredOnlyChangesets(['both-ignored.md'], IGNORED, read)).toEqual(['both-ignored.md']) + }) + it('leaves a published changeset alone', () => { + expect(ignoredOnlyChangesets(['clean.md'], IGNORED, read)).toEqual([]) + }) + // A mixed file is already rejected, louder and with both sides named. Flagging it here too would + // report one defect twice and send the author to the wrong instruction. + it('leaves a mixed changeset to the check above', () => { + expect(ignoredOnlyChangesets(['mixed.md'], IGNORED, read)).toEqual([]) + }) + // Names nothing at all, so it cannot be *only* ignored. `packagesNamedIn` returns `[]` and + // `[].every(...)` is true, which would make this the one input that fails for being empty. + it('leaves a changeset with no frontmatter alone', () => { + expect(ignoredOnlyChangesets(['no-frontmatter.md'], IGNORED, read)).toEqual([]) + }) +}) + describe('a package.json edit that ships', () => { const withDev = (dev) => JSON.stringify({ name: 'x', version: '1.0.0', dependencies: { a: '1' }, devDependencies: dev }) it('ignores a devDependencies-only change', () => { diff --git a/scripts/__tests__/changesetIgnoresPrivate.test.mjs b/scripts/__tests__/changesetIgnoresPrivate.test.mjs new file mode 100644 index 00000000..cf8fd959 --- /dev/null +++ b/scripts/__tests__/changesetIgnoresPrivate.test.mjs @@ -0,0 +1,82 @@ +import { describe, it, expect } from 'vitest' +import { getPackages } from '@manypkg/get-packages' +import path from 'node:path' +import { SHIPS_DESPITE_PRIVATE, ignoredPackages } from '../check-changeset.mjs' + +// `.changeset/config.json`'s `ignore` has to name every `private: true` workspace package, and until +// now nothing checked that it did. The list fell behind twice in one week: +// +// - #537 wrote a changeset naming `@tapflowio/test-utils`, absent from `ignore`. `changeset version` +// would have versioned an unpublished package and written it a CHANGELOG while **no published +// package got a release note**, with the CI `changeset` job green. Caught in review. +// - The follow-up added `test-utils` and left `@tapflowio/docs`, also private, also missing. Caught +// by the next review. +// +// Nothing downstream objects on its own: `privatePackages.version` defaults to `true`, so a private +// package that is not ignored appears in `pnpm changeset` like any other, and config validation only +// errors when a *non-private* unskipped package depends on a skipped one. +const REPO = path.resolve(import.meta.dirname, '../..') + +// **The same enumerator changesets uses**, rather than a glob over `pnpm-workspace.yaml`. Two of the +// four private packages (`docs`, `playground`) are not under `packages/`, so a check that walked +// `packages/*` would have declared the list complete while missing half of what it is about — and +// reimplementing the tool's discovery to guard the tool's behaviour is the shape +// `contributing/test-and-guard-coverage.md` §3 calls a floor rather than a fence. `getPackages` +// excludes the workspace root, which is also correct here: the root is `private` and changesets never +// offers it. +const workspace = await getPackages(REPO) +// **Through the gate's own reader, not a second copy of it.** A first draft read the config here +// directly, which is green under exactly the conditions that break the gate: `ignoredPackages` used a +// relative path and a bare catch, so any cwd but the repo root turned the mixed-changeset check off +// with nothing to say so. A guard that duplicates the reader cannot observe the reader failing. +// +// Entries are compared as literal names. `@changesets/config` micromatch-expands them, so a glob +// entry would be reported by both assertions below — loudly and wrongly. No glob is in use and the +// failure direction is safe, so this is a constraint rather than a hole; stated here so the next +// person does not read the assertions as glob-aware. +const byName = new Map(workspace.packages.map((p) => [p.packageJson.name, p.packageJson])) +const ignored = ignoredPackages() +// Truthiness, matching changesets' own `shouldSkipPackage` (`packageJson.private && …`) and this +// repo's `packagePublishesAt` (`!m.private`). `=== true` was stricter than all three: a manifest with +// `"private": "true"` is private to pnpm, to npm publish and to changesets, and would have been +// invisible here while `changeset version` bumped and changelogged something unpublishable. +const privateNames = workspace.packages + .filter((p) => Boolean(p.packageJson.private)) + .map((p) => p.packageJson.name) + +describe('.changeset/config.json ignore covers every private package', () => { + // Found by inspection, not from a list. A hardcoded list of expected names is satisfied by not being + // edited, which is exactly how the two misses above survived: `changesetGateAccuracy.test.mjs` + // carries `const IGNORED = new Set([...])` as a unit fixture and did not move when the real list did. + it('names every private workspace package', () => { + // **Four, not "more than zero".** A floor of zero is satisfied by an enumerator that finds almost + // nothing, and the specific way this check can go vacuous is the one its header warns about: + // swapping `getPackages` for a `packages/*` walk finds 2 of the 4, and every assertion below still + // passes. The measured count is what makes that mutation fail (test-and-guard-coverage.md §3). + expect(privateNames.length).toBeGreaterThanOrEqual(4) + const missing = privateNames.filter((n) => !ignored.has(n)) + expect(missing, `private but not ignored: ${missing.join(', ')}`).toEqual([]) + }) + + // The other direction, and it is not symmetry for its own sake. An entry naming a package that was + // published since, or renamed, or deleted, silently exempts nothing while reading as deliberate — and + // the failure it hides is the expensive one: a package that should get release notes, quietly not + // getting them. + it('names nothing that is not a private workspace package (literal names only)', () => { + const stale = [...ignored].filter((n) => !byName.get(n)?.private) + expect(stale, `ignored but not a private workspace package: ${stale.join(', ')}`).toEqual([]) + }) + + // `check-changeset.mjs` keeps its own list for a different question — which packages still need a + // release note despite being private, because they ship inside another package's tarball. Both + // questions are true of `dashboard` at once: it must be ignored (changesets cannot publish it) *and* + // a change to it must be noted (it is built into the relay's `public/`). Read from that module rather + // than restated, so the two cannot drift; asserted here because if a name there stops being private, + // that set is exempting a package the gate should be asking about. + it('agrees with the ships-despite-private set in check-changeset.mjs', () => { + for (const name of SHIPS_DESPITE_PRIVATE) { + expect(Boolean(byName.get(name)?.private), `${name} is named as shipping despite private, but is not private`).toBe(true) + expect(ignored.has(name), `${name} is private and must be ignored`).toBe(true) + } + }) +}) diff --git a/scripts/check-changeset.mjs b/scripts/check-changeset.mjs index 5519cd11..085d52c3 100644 --- a/scripts/check-changeset.mjs +++ b/scripts/check-changeset.mjs @@ -31,7 +31,9 @@ const NOT_SHIPPED_PACKAGES = ['docs', 'playground'] // reads from the manifest rather than from a list — the list this replaced still named `docs` // and `playground`, which have not been under `packages/` for some time, while // `@tapflowio/test-utils`, added later, was absent and so counted as shipped. -const SHIPS_DESPITE_PRIVATE = ['@tapflowio/dashboard'] +// Exported so `changesetIgnoresPrivate.test.mjs` can check this list against the manifests rather +// than keeping a second copy of the answer. That test is the one that would notice this going stale. +export const SHIPS_DESPITE_PRIVATE = ['@tapflowio/dashboard'] /** * Whether `packages/` published anything, judged by its manifest **at `rev`**. @@ -313,10 +315,14 @@ export function extractReason(body) { * below only asks whether a changeset exists and never opens one. Four such changesets stopped * the v0.18.0 release, written across four different PRs that all went green. */ -function ignoredPackages() { - try { - return new Set(JSON.parse(readFileSync('.changeset/config.json', 'utf8')).ignore ?? []) - } catch { return new Set() } +export function ignoredPackages() { + // **Rooted at this file, and no longer failing open.** A bare `catch` returning an empty set meant + // "nothing is ignored", which switches `mixedChangesets` off entirely — the check that exists + // because four mixed changesets stopped the v0.18.0 release. Combined with the relative path, any + // invocation whose cwd was not the repo root disabled it silently. A gate that cannot read its own + // configuration has to say so rather than wave everything through. + const config = new URL('../.changeset/config.json', import.meta.url) + return new Set(JSON.parse(readFileSync(config, 'utf8')).ignore ?? []) } /** The package names a changeset's frontmatter bumps. */ @@ -336,6 +342,29 @@ export function mixedChangesets(files, ignored, read) { }) } +/** + * Whether the added changesets name a package that will actually get a release note. + * + * The mixed case above throws at `changeset version` and is loud on release day. This one is + * **silent**: `assemble-release-plan` produces no release for an ignored-only changeset and + * `changeset version` deletes the file, so published source ships with nothing written about it and + * every gate stayed green. It is the exact mistake the root AGENTS.md warns about — a dashboard + * change must name `@tapflowio/relay`, because the dashboard is built into that package's `public/` + * and cannot be versioned itself. + * + * Adding the private packages to `ignore` made this quieter rather than louder: before, an + * ignored-only changeset at least bumped an unpublished package and wrote it a CHANGELOG, which is + * visible in the release PR's diff. Now it emits nothing at all. + * + * Returns the offending files, so the caller can name them. + */ +export function ignoredOnlyChangesets(files, ignored, read) { + return files.filter((f) => { + const named = packagesNamedIn(read(f)) + return named.length > 0 && named.every((n) => ignored.has(n)) + }) +} + function main() { // `--audit [since]` walks merges instead of the current branch: the PR gate cannot help with // anything already on main, and that is exactly how #410–#413 slipped through. Run at release @@ -529,6 +558,16 @@ function main() { process.exit(1) } + const ignoredOnly = ignoredOnlyChangesets(added, ignoredPackages(), (f) => readFileSync(f, 'utf8')) + if (ignoredOnly.length === added.length && added.length > 0) { + console.error('Every added changeset names only ignored packages:\n') + for (const f of ignoredOnly) console.error(` ${f}`) + console.error('\n`changeset version` produces no release for these and deletes the file, so this') + console.error('branch would ship with no release note at all. Name the package that actually ships') + console.error('the change — for a dashboard change that is `@tapflowio/relay`.') + process.exit(1) + } + if (added.length > 0) { console.log(`Published source changed and ${added.length} changeset(s) added:`) for (const f of added) console.log(` + ${f}`)