test(scripts): enforce that changeset's ignore covers every private package

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",

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', () => {

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)
+    }
+  })
+})

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/<dir>` 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}`)