From ebc678d5704d4302d291c83794af5e21263d237f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 00:25:31 +0000 Subject: [PATCH] ci(changeset): report changesets a change modified but did not add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hand-picked `changesets`-style filename can land on one that already exists, and the overwrite is silent in both directions that should catch it: `git status` reports ` M` rather than `??`, so it reads as your own new file landing, and a deleted release declaration is flagged by nothing downstream. The cost lands on a third party — whichever earlier pull request's declaration disappears — and surfaces only when a package fails to bump. New `scripts/check-changeset-overwrite.mjs`, run by a second job in `changeset-guard.yml` (its own checkout at `fetch-depth: 0`, since this one reads a diff). It names every `.changeset/*.md` the change modified or deleted without having added it, prints the declaration that was there and what is now gone from it, and recognizes the release emptying the queue as the non-finding it is. REPORT-ONLY, measured rather than cautious: across all 5281 first-parent commits on `main`, 12 commits modified a pre-existing changeset (19 files) and all 19 were legitimate — bump levels corrected when the pending release line changed, prose corrections, authors amending their own unreleased changeset. Blocking would have failed every one. `OS_CHANGESET_OVERWRITE_ENFORCE=1` flips it for whoever revisits that with a new measurement. The resolver, `git diff` wrapper and frontmatter reader are imported from `check-changeset-presence.mjs` rather than copied a third time; the second copy of that resolver inherited a real defect and had to be fixed to match. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe --- .changeset/6336-changeset-overwrite-guard.md | 21 + .github/workflows/changeset-guard.yml | 73 +++- content/docs/guide/ci-cd-pipeline.md | 51 ++- .../check-changeset-overwrite.test.ts | 359 +++++++++++++++++ scripts/check-changeset-overwrite.mjs | 381 ++++++++++++++++++ scripts/dependabot-merge-gate.mjs | 2 + 6 files changed, 876 insertions(+), 11 deletions(-) create mode 100644 .changeset/6336-changeset-overwrite-guard.md create mode 100644 scripts/__tests__/check-changeset-overwrite.test.ts create mode 100644 scripts/check-changeset-overwrite.mjs diff --git a/.changeset/6336-changeset-overwrite-guard.md b/.changeset/6336-changeset-overwrite-guard.md new file mode 100644 index 000000000..4dd585854 --- /dev/null +++ b/.changeset/6336-changeset-overwrite-guard.md @@ -0,0 +1,21 @@ +--- +--- + +Internal CI-only change: a new report-only `Changeset Overwrite Report` job in +`changeset-guard.yml` names any `.changeset/*.md` a change modified or deleted +without having added it, and prints the release declaration that was there. + +No published package changes, so this releases nothing. + +objectui#6336 — a hand-picked `changesets`-style filename can land on one that +already exists, and the overwrite is silent in both directions that should catch +it: `git status` reports ` M` rather than `??`, and a deleted release declaration +is flagged by nothing downstream. The cost lands on a third party — whichever +earlier pull request's declaration disappears — and surfaces only when a package +fails to bump. + +Report-only is measured, not cautious: across all 5281 first-parent commits on +`main`, every one of the 19 modifications of a pre-existing changeset was +legitimate, so a blocking gate would have failed all of them. +`OS_CHANGESET_OVERWRITE_ENFORCE=1` flips it for whoever revisits that with a new +measurement. diff --git a/.github/workflows/changeset-guard.yml b/.github/workflows/changeset-guard.yml index 814098831..35ca72799 100644 --- a/.github/workflows/changeset-guard.yml +++ b/.github/workflows/changeset-guard.yml @@ -23,8 +23,16 @@ name: Changeset Guard # no per-job path filter either, so the gate lives here, with the inverse # trigger: it runs *only* when `.changeset/**` changes. # -# It needs no install and no build — a checkout plus one `node` call, a few -# seconds — so keep it that way if you add checks to it. +# It needs no install and no build — a checkout plus a `node` call per job, a +# few seconds — so keep it that way if you add checks to it. +# +# ── Two jobs, opposite questions ───────────────────────────────────────────── +# +# `no-major` — the LEVEL a pending changeset declares. Reads the tree. +# `overwrite` — whether this change touched a changeset it did not ADD. Reads +# the diff against the merge base, so it alone needs +# `fetch-depth: 0` (objectui#6336). Report-only; the script's +# header carries the history that chose that over blocking. # # ── Self-coverage (objectui#6321) ──────────────────────────────────────────── # @@ -38,8 +46,9 @@ name: Changeset Guard # `performance-budget.yml`, was closed in #6315). # # The convention those five follow is "list what the `run:` step below actually -# executes" — own YAML plus the invoked script — not the transitive import -# graph. `half-state-patrol.yml` additionally lists `scripts/invoked-as.mjs`, +# executes" — own YAML plus the invoked scripts, both of them — not the +# transitive import graph. `half-state-patrol.yml` additionally lists +# `scripts/invoked-as.mjs`, # but that is a documented one-off (its own header's divergence #2, tied to # objectui#5791's port from objectstack) and NOT the pattern the other three # self-including, script-running gates follow: `node-esm-load-gate.yml`, @@ -51,7 +60,19 @@ name: Changeset Guard # every gate that happens to import it. So this filter matches the # three-of-four majority and does not add it either. # -# Also deliberately NOT listed: `scripts/__tests__/check-changeset-no-major.test.ts`. +# Deliberately NOT listed for the same reason: `scripts/check-changeset-presence.mjs`. +# `check-changeset-overwrite.mjs` imports its base-ref resolver, its `git diff` +# wrapper and its frontmatter reader rather than growing a THIRD copy of them — +# the second copy, in `check-i18n-en-drift.mjs`, inherited a real defect from the +# first draft of that resolver and had to be fixed to match under objectui#3766, +# which is the drift a third copy would reopen. It is an import, not something a +# `run:` step executes, and a change to it is already caught by the root vitest +# suite (`check-changeset-presence.test.ts` and `check-changeset-overwrite.test.ts` +# both exercise it) on any pull request touching `scripts/**` — the same +# `~ partial` reasoning applied to `invoked-as.mjs` just above. +# +# Also deliberately NOT listed: `scripts/__tests__/check-changeset-no-major.test.ts` +# and `scripts/__tests__/check-changeset-overwrite.test.ts`. # That file matches the `~ partial` pattern `published-dist-gate.yml` and # `spec-range-floors.yml` already set for their own gate scripts' `__tests__` # files — the test lives under `scripts/**`, which the `Decide whether this @@ -68,12 +89,14 @@ on: - '.changeset/**' - '.github/workflows/changeset-guard.yml' - 'scripts/check-changeset-no-major.mjs' + - 'scripts/check-changeset-overwrite.mjs' push: branches: [main] paths: - '.changeset/**' - '.github/workflows/changeset-guard.yml' - 'scripts/check-changeset-no-major.mjs' + - 'scripts/check-changeset-overwrite.mjs' concurrency: group: changeset-guard-${{ github.event.pull_request.number || github.ref }} @@ -97,3 +120,43 @@ jobs: # See AGENTS.md §版本号策略, and the script's header for the full rationale. - name: Verify no changeset declares a major bump run: node scripts/check-changeset-no-major.mjs + + # A SECOND job rather than a step in `no-major`, for one reason: this gate + # reads a diff and therefore needs `fetch-depth: 0`, and `no-major` reads the + # tree and wants to stay a depth-1 checkout. Two jobs also run in parallel, + # so the workflow is no slower than its slowest half. + overwrite: + name: Changeset Overwrite Report + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - name: Checkout code + uses: actions/checkout@v7 + with: + # The gate compares this change against its MERGE BASE with the target + # branch, so it needs history — checkout's default is a depth-1 clone + # where `git merge-base` has nothing to find. An unresolvable base is a + # hard failure in the script, never a skip, so getting this wrong is a + # red build rather than a silent pass; spelled out here so it stays + # that way. Same requirement, same reason, as `changeset-presence.yml`. + fetch-depth: 0 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22.x' + + # REPORT-ONLY today, and that is a measured choice rather than caution: + # all 19 modifications of a pre-existing changeset in this repository's + # history were legitimate (bump-level corrections, prose corrections, + # authors amending their own unreleased changeset), so a blocking gate + # would have failed every one of those pull requests. What it catches is + # the overwrite in objectui#6336, whose cost lands on a THIRD PARTY — the + # earlier pull request whose release declaration disappears — and is + # invisible when it happens: `git status` shows ` M`, not `??`. The step + # prints the declaration that was there and what is now gone from it. + # `OS_CHANGESET_OVERWRITE_ENFORCE=1` flips it to blocking for whoever + # revisits this with a new measurement. + - name: Report changesets this change did not add + run: node scripts/check-changeset-overwrite.mjs diff --git a/content/docs/guide/ci-cd-pipeline.md b/content/docs/guide/ci-cd-pipeline.md index 17ff62755..f5b07af70 100644 --- a/content/docs/guide/ci-cd-pipeline.md +++ b/content/docs/guide/ci-cd-pipeline.md @@ -25,7 +25,7 @@ one has its own section below. |---|---|---|---| | `ci.yml` | CI | Push / PR to `main`, `develop`; merge-queue builds | **Yes** — every job but the two coverage-lane jobs (`test-coverage` and `coverage-report`, push only) runs on PRs and on queue builds | | `lint.yml` | Lint | Push / PR to `main`, `develop`; merge-queue builds; manual | **Yes** — ESLint **errors** only | -| `changeset-guard.yml` | Changeset Bump Policy | PR / push touching `.changeset/**` or the gate itself | **Yes** | +| `changeset-guard.yml` | Changeset Bump Policy, Changeset Overwrite Report | PR / push touching `.changeset/**` or either gate itself | **Yes** — the bump policy job only; the overwrite job is report-only | | `changeset-presence.yml` | Changeset Declaration | PR to `main`, `develop` — **no path filter**; merge-queue builds | **Yes** — when a released package's `src/` changed and no changeset was added | | `control-bytes.yml` | Control Byte Scan | Push / PR to `main`, `develop` — **no path filter**; merge-queue builds; manual | **Yes** | | `docs-links.yml` | Internal Docs Link Check | Push / PR to `main`, `develop` — **no path filter**; merge-queue builds; manual | **Yes** | @@ -153,8 +153,8 @@ Two things follow for anyone editing this directory: shapes is just as unrequirable without appearing here. - **Changeset Bump Policy** (`changeset-guard.yml`) — an **inverse** path filter: its `pull_request` trigger declares - `paths: ['.changeset/**', '.github/workflows/changeset-guard.yml', 'scripts/check-changeset-no-major.mjs']`, - so on a PR that touches none of those three the context is never created at all. + `paths: ['.changeset/**', '.github/workflows/changeset-guard.yml', 'scripts/check-changeset-no-major.mjs', 'scripts/check-changeset-overwrite.mjs']`, + so on a PR that touches none of those four neither of its contexts is created at all. - **Bundle Analysis** (`performance-budget.yml`) — an ordinary path filter on the same trigger, with the same consequence for every PR that matches none of its paths. - **Live E2E (informational)** (`live-e2e.yml`) — the job carries `continue-on-error: true`, @@ -1348,11 +1348,17 @@ changeset started nothing at all. Since [#3523](https://github.com/objectstack-a job in them short-circuits, because `.changeset/**` is still on the in-job ignore list. The check that has to read the changeset therefore still lives here. -The same `paths:` list also carries the gate's own YAML and -`scripts/check-changeset-no-major.mjs` ([#6321](https://github.com/objectstack-ai/objectui/issues/6321)) +The same `paths:` list also carries the gate's own YAML and both scripts it runs, +`scripts/check-changeset-no-major.mjs` and `scripts/check-changeset-overwrite.mjs` +([#6321](https://github.com/objectstack-ai/objectui/issues/6321)) — self-coverage, not the inverse trigger above: without it, a PR that edits the gate is not the PR that runs it, and the first real execution lands on someone else's unrelated `.changeset/**` PR. -Deliberately not listed: `scripts/invoked-as.mjs` (a dependency the gate script imports, but a +Deliberately not listed: `scripts/check-changeset-presence.mjs` (the overwrite gate imports its +base-ref resolver, `git diff` wrapper and frontmatter reader rather than growing a third copy — +the second copy, in `check-i18n-en-drift.mjs`, inherited a real defect from that resolver's first +draft and had to be fixed to match under +[#3766](https://github.com/objectstack-ai/objectui/issues/3766); the root vitest suite exercises it +on any PR touching `scripts/**`), `scripts/invoked-as.mjs` (a dependency the gate scripts import, but a widely shared one — 40+ importers under `scripts/` — that `published-dist-gate.yml`, `spec-range-floors.yml` and `node-esm-load-gate.yml` also import without listing; only `half-state-patrol.yml` lists it, as a documented one-off) and the script's own @@ -1370,6 +1376,39 @@ The one release that legitimately bumps the major is the one following `@objects its major; it sets `OBJECTUI_ALLOW_MAJOR=1`. `pnpm test` asserts the same repository state, so the rule survives this workflow being skipped. +#### Second job: Changeset Overwrite Report + +Runs `scripts/check-changeset-overwrite.mjs`, which asks a different question of the same files: +did this change **modify or delete a `.changeset/*.md` that already existed at its merge base** — +a changeset it did not add? It is a separate job because it reads a diff and so needs +`fetch-depth: 0`, which the bump-policy job does not want. + +[#6336](https://github.com/objectstack-ai/objectui/issues/6336) is why it exists. A dev run wrote +its changeset to a hand-picked `changesets`-style name that already existed on `main`, and the +heredoc overwrote an unrelated `@object-ui/plugin-charts: minor`. It was caught before any commit, +but the property that makes it worth a gate is that **the cost lands on a third party and is +invisible at the time it happens**: the agent that picks the colliding name loses nothing, and +whichever earlier PR's release declaration vanishes only discovers it when a package silently +fails to bump. Both signals that should catch it fail — `git status` shows `` M`` rather than +`??`, which reads as your own new file landing, and a deleted release declaration is not something +any later gate flags. With 424 accumulated changesets against an `adjective-animal-verb` name +space, the collision probability is not theoretical. + +**It is report-only, and that is measured rather than cautious.** Across all 5281 first-parent +commits on `main`, 12 commits modified a pre-existing changeset (19 files) and **all 19 were +legitimate** — bump levels corrected when the pending release line changed, a "eleven" corrected +to "ten", a typo'd package name fixed, authors amending their own not-yet-released changeset. A +blocking gate would have failed every one of those PRs. Deletions are dominated by the release +itself (82 of 88 delete changesets alongside a package `CHANGELOG.md`, which is `changeset +version` emptying the queue); the job recognizes that shape and says so instead of reporting it. +`OS_CHANGESET_OVERWRITE_ENFORCE=1` flips the job to blocking for whoever revisits this with a new +measurement. + +⭐ **The convention that makes the hazard impossible**: name a changeset after the issue it +settles — `.changeset/-.md`. The `adjective-animal-verb` names are safe when +`pnpm changeset` allocates them, because it allocates against the files already present; picking +one by hand is what removes that guarantee. + > **A changeset IS now required, by `changeset-presence.yml` — but there is still no > `skip-changeset` mechanism.** Until [#3387](https://github.com/objectstack-ai/objectui/issues/3387) > nothing in CI asked whether a PR had added one, and this note said so at length, because the diff --git a/scripts/__tests__/check-changeset-overwrite.test.ts b/scripts/__tests__/check-changeset-overwrite.test.ts new file mode 100644 index 000000000..2f463875c --- /dev/null +++ b/scripts/__tests__/check-changeset-overwrite.test.ts @@ -0,0 +1,359 @@ +import { afterAll, describe, expect, it } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { declaredEntries } from '../check-changeset-overwrite.mjs'; + +/** + * objectui#6336 — a hand-picked changeset filename can silently overwrite + * another pull request's changeset. + * + * The hazard is not that an agent makes a mistake; it is WHO pays and WHEN. + * The cost lands on a third party (whoever's release declaration disappears) + * and is invisible at the time it happens: `git status` shows ` M`, not `??`, + * so an agent checking "did my new file appear" reads the overwrite as its own + * write landing, and the loss is a deleted release declaration that nothing + * downstream flags. + * + * What this file pins, in the order the gate can fail: + * + * 1. **The workflow can start on the pull request that needs it.** A path + * filter is normally a false-negative surface; here it cannot be, because a + * change that modifies a `.changeset/*.md` touches `.changeset/**` by + * definition. The filter must still carry the gate's OWN closure — its YAML + * and the script it runs (objectui#6321) — or the pull request that changes + * the gate is not the one that runs it. + * 2. **The verdicts**, against throwaway repositories rather than this one's + * history, so they stay decidable when the history moves. Both directions: + * a modification of a pre-existing changeset is REPORTED, and a change that + * only ADDS one stays green with the addition actually SEEN — a green that + * comes from the gate looking at nothing is the failure this whole card is + * about. + * 3. **Report-only is the shipped default, and it is a choice, not an + * accident.** Measured over all 5281 first-parent commits on `main`, all 19 + * modifications of a pre-existing changeset in this repository's history + * were legitimate (bump-level corrections, prose corrections, authors + * amending their own unreleased changeset). A blocking gate would have + * failed every one. `OS_CHANGESET_OVERWRITE_ENFORCE=1` is pinned here so the + * flip stays a decision someone makes with a new measurement. + * 4. **Every missing input fails LOUD.** Report-only means the gate declines to + * fail on its FINDINGS — never that it passes without looking + * (objectstack#4928, objectui#4690). + */ +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const GATE = 'scripts/check-changeset-overwrite.mjs'; +const WORKFLOW = '.github/workflows/changeset-guard.yml'; + +/** + * A workflow's YAML with whole-line comments removed. + * + * Required, not cosmetic: this workflow's header discusses `paths` and its own + * self-coverage at length, so a scan that counted the prose would report filter + * entries the file does not have. Same helper, same reason, as in + * `check-changeset-presence.test.ts` and `merge-queue-reporting.test.ts`. + */ +function withoutComments(yaml: string): string { + return yaml + .split('\n') + .filter((line) => !/^\s*#/.test(line)) + .join('\n'); +} + +const workflowYaml = withoutComments(fs.readFileSync(path.join(repoRoot, WORKFLOW), 'utf8')); + +// ── fixture repositories ───────────────────────────────────────────────────── + +const fixtures: string[] = []; +afterAll(() => { + for (const dir of fixtures) fs.rmSync(dir, { recursive: true, force: true }); +}); + +interface Fixture { + root: string; + git: (...args: string[]) => string; + write: (rel: string, body: string) => void; + remove: (rel: string) => void; + commit: (message: string) => string; +} + +/** + * A throwaway repository carrying two pre-existing changesets and a package + * with a CHANGELOG, which is all this gate reads. + * + * The changesets are named `@fixture/*` on purpose: if the gate ever stopped + * reading declarations out of the files and fell back to a hard-coded + * `@object-ui/*` surface, every "declared at base" assertion below would flip. + */ +function fixtureRepo(label: string): Fixture { + const root = fs.mkdtempSync(path.join(os.tmpdir(), `changeset-overwrite-${label}-`)); + fixtures.push(root); + + const git = (...args: string[]): string => + execFileSync('git', args, { cwd: root, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim(); + + const write = (rel: string, body: string): void => { + fs.mkdirSync(path.join(root, path.dirname(rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), body); + }; + + const remove = (rel: string): void => fs.rmSync(path.join(root, rel)); + + const commit = (message: string): string => { + execFileSync('git', ['add', '-A', '-f'], { cwd: root }); + execFileSync('git', ['commit', '-q', '-m', message], { cwd: root }); + return git('rev-parse', 'HEAD'); + }; + + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 'fixture@example.com'); + git('config', 'user.name', 'Fixture'); + + write('.changeset/config.json', JSON.stringify({ fixed: [['@fixture/alpha']], ignore: [] }, null, 2)); + // Documentation, not a declaration — it matches the `.changeset/*.md` glob and + // must still never be reported. + write('.changeset/README.md', '# Changesets\n\nDocumentation, not a declaration.\n'); + // The shape of the near-miss: somebody else's pending release declaration, + // sitting under a `pnpm changeset` adjective-animal-verb name. + write( + '.changeset/olive-donkeys-smile.md', + '---\n"@fixture/charts": minor\n---\n\nChart widgets render at their resolved height.\n', + ); + write('.changeset/plum-pandas-wave.md', '---\n"@fixture/alpha": patch\n---\n\nAn unrelated pending fix.\n'); + write('packages/alpha/package.json', JSON.stringify({ name: '@fixture/alpha', version: '1.0.0' })); + write('packages/alpha/CHANGELOG.md', '# @fixture/alpha\n\n## 1.0.0\n'); + commit('base'); + + return { root, git, write, remove, commit }; +} + +interface Run { + status: number; + output: string; +} + +/** Runs the real gate against a fixture, capturing status and both streams. */ +function runGate(root: string, args: string[] = [], env: Record = {}): Run { + try { + const stdout = execFileSync('node', [path.join(repoRoot, GATE), '--root', root, ...args], { + cwd: repoRoot, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...process.env, ...env }, + }); + return { status: 0, output: stdout }; + } catch (error) { + const failure = error as { status?: number; stdout?: string; stderr?: string }; + return { status: failure.status ?? -1, output: `${failure.stdout ?? ''}${failure.stderr ?? ''}` }; + } +} + +/** base..head for a fixture's last commit. */ +function lastCommitRange(fixture: Fixture): string[] { + return ['--base', fixture.git('rev-parse', 'HEAD~1'), '--head', fixture.git('rev-parse', 'HEAD')]; +} + +// ── 1. the workflow is reachable, and covers itself ────────────────────────── + +describe('changeset-guard.yml — the gate can start on the pull request that needs it', () => { + it('exists, and runs the gate script', () => { + expect(fs.existsSync(path.join(repoRoot, GATE)), `${GATE} must exist for the workflow to run it`).toBe(true); + expect(workflowYaml).toMatch(new RegExp(`run:\\s*node\\s+${GATE.replace(/[.]/g, '\\.')}`)); + }); + + it('gives the job the history its diff needs', () => { + // The gate compares this change against its MERGE BASE with the target + // branch. checkout's default is a depth-1 clone where `git merge-base` has + // nothing to find, and an unresolvable base is a hard failure in the script + // — so getting this wrong is a red build rather than a silent pass. Pinned + // so it stays that way. + expect(workflowYaml).toMatch(/fetch-depth:\s*0/); + }); + + it("carries the gate's own closure in the paths filter (objectui#6321)", () => { + // Without this, the pull request that edits the gate is not the pull request + // that runs it, and the first real execution lands on somebody else's + // unrelated `.changeset/**` change. `paths:` appears twice — `pull_request` + // and `push` — and both must carry it. + const occurrences = workflowYaml.split(GATE).length - 1; + expect(occurrences, `${GATE} must appear in BOTH paths: filters and the run: step`).toBeGreaterThanOrEqual(3); + expect(workflowYaml.split(WORKFLOW).length - 1, 'the workflow must list its own YAML').toBeGreaterThanOrEqual(2); + }); + + it('still triggers on .changeset/** — the filter this gate depends on', () => { + // A path filter is normally a false-negative surface. It is not here, and + // only because of this entry: a change that modifies or deletes a + // `.changeset/*.md` touches `.changeset/**` BY DEFINITION, so the filter + // cannot skip the case the gate exists for. + expect(workflowYaml).toMatch(/- '\.changeset\/\*\*'/); + }); +}); + +// ── 2. the verdicts ────────────────────────────────────────────────────────── + +describe('a change that only ADDS a changeset', () => { + const fixture = fixtureRepo('add-only'); + fixture.write('.changeset/6336-new-work.md', '---\n"@fixture/alpha": minor\n---\n\nBrand new work.\n'); + fixture.write('packages/alpha/src/index.ts', 'export const alpha = 2;\n'); + fixture.commit('feat: new work with its own changeset'); + const run = runGate(fixture.root, lastCommitRange(fixture)); + + it('is green', () => { + expect(run.status, run.output).toBe(0); + expect(run.output).toContain('No pre-existing changeset was modified or deleted'); + }); + + it('is green because the gate SAW the addition, not because it saw nothing', () => { + // The half of the negative case that is easy to fake. A gate that reports + // "0 changesets added" and exits 0 is green for the wrong reason, and would + // stay green through the overwrite this card is about. + expect(run.output).toContain('1 changeset(s) added, 0 modified, 0 deleted'); + }); + + it('stays green even with enforcement switched on', () => { + const enforced = runGate(fixture.root, lastCommitRange(fixture), { OS_CHANGESET_OVERWRITE_ENFORCE: '1' }); + expect(enforced.status, enforced.output).toBe(0); + }); +}); + +describe("a change that OVERWRITES a pre-existing changeset — the objectui#6336 shape", () => { + const fixture = fixtureRepo('overwrite'); + // Exactly the near-miss: a heredoc onto a name that already existed, carrying + // an unrelated declaration. + fixture.write( + '.changeset/olive-donkeys-smile.md', + '---\n"@fixture/alpha": patch\n---\n\nMy own unrelated fix, written over somebody else.\n', + ); + fixture.commit('fix: something else entirely'); + const run = runGate(fixture.root, lastCommitRange(fixture)); + + it('reports the file', () => { + expect(run.output).toContain('.changeset/olive-donkeys-smile.md'); + expect(run.output).toContain('1 changeset(s) it did not add'); + }); + + it('names the declaration that was there, which is the thing at risk', () => { + expect(run.output).toContain('declared at base: @fixture/charts: minor'); + }); + + it('says which declaration is GONE — the part nothing downstream would flag', () => { + expect(run.output).toContain('GONE from the declaration: @fixture/charts'); + }); + + it('does not fail the build: report-only is the shipped default', () => { + // Measured: all 19 modifications of a pre-existing changeset in this + // repository's history were legitimate. Blocking would have failed every + // one of those pull requests. + expect(run.status, run.output).toBe(0); + expect(run.output).toContain('Report-only'); + }); + + it('DOES fail under OS_CHANGESET_OVERWRITE_ENFORCE=1', () => { + const enforced = runGate(fixture.root, lastCommitRange(fixture), { OS_CHANGESET_OVERWRITE_ENFORCE: '1' }); + expect(enforced.status, enforced.output).toBe(1); + expect(enforced.output).toContain('GONE from the declaration: @fixture/charts'); + }); +}); + +describe('a change that DELETES a pre-existing changeset', () => { + const fixture = fixtureRepo('delete'); + fixture.remove('.changeset/plum-pandas-wave.md'); + fixture.commit('chore: drop a changeset'); + const run = runGate(fixture.root, lastCommitRange(fixture)); + + it('reports it, with what it declared', () => { + expect(run.output).toContain('D .changeset/plum-pandas-wave.md'); + expect(run.output).toContain('declared at base: @fixture/alpha: patch'); + expect(run.status, run.output).toBe(0); + }); +}); + +describe('the release emptying the queue', () => { + const fixture = fixtureRepo('release'); + fixture.remove('.changeset/olive-donkeys-smile.md'); + fixture.remove('.changeset/plum-pandas-wave.md'); + fixture.write('packages/alpha/CHANGELOG.md', '# @fixture/alpha\n\n## 1.1.0\n\n- An unrelated pending fix.\n'); + fixture.write('packages/alpha/package.json', JSON.stringify({ name: '@fixture/alpha', version: '1.1.0' })); + fixture.commit('chore: release packages'); + const run = runGate(fixture.root, lastCommitRange(fixture), { OS_CHANGESET_OVERWRITE_ENFORCE: '1' }); + + it('is not a finding — that is how changesets are consumed', () => { + // Measured: 82 of the 88 commits on `main` that delete a pre-existing + // changeset also modify a package CHANGELOG.md, and every one of those is a + // release. Pinned under ENFORCEMENT, because this is the exemption that has + // to survive the day somebody flips the switch. + expect(run.status, run.output).toBe(0); + expect(run.output).toContain('Release consumption'); + expect(run.output).toContain('2 changeset(s) deleted'); + }); +}); + +describe('.changeset/README.md', () => { + const fixture = fixtureRepo('readme'); + fixture.write('.changeset/README.md', '# Changesets\n\nDocumentation, edited.\n'); + fixture.commit('docs: edit the changeset README'); + const run = runGate(fixture.root, lastCommitRange(fixture), { OS_CHANGESET_OVERWRITE_ENFORCE: '1' }); + + it('is documentation, not a declaration, and is never reported', () => { + expect(run.status, run.output).toBe(0); + expect(run.output).toContain('No pre-existing changeset was modified or deleted'); + }); +}); + +// ── 3. missing inputs fail loud ────────────────────────────────────────────── + +describe('a base that cannot be resolved', () => { + const fixture = fixtureRepo('no-base'); + + it('is a hard failure, never a quiet pass', () => { + // Report-only means the gate declines to fail on its FINDINGS. It must + // still fail when it cannot look — a diff gate that cannot compute its diff + // and exits 0 has reported "clean" while reading nothing (objectstack#4928, + // objectui#4690). + const run = runGate(fixture.root, ['--base', '0000000000000000000000000000000000000000']); + expect(run.status, run.output).toBe(1); + expect(run.output).toContain('Cannot resolve the commit to compare against'); + }); + + it('does not fall through to guessing another base', () => { + // The defect the sibling gate's first draft had: with a fallthrough, + // `--base ` silently judged the change against + // `main`'s tip instead and printed a confident green. + const run = runGate(fixture.root, ['--base', '0000000000000000000000000000000000000000']); + expect(run.output).toContain('named EXPLICITLY'); + }); +}); + +// ── 4. the frontmatter reader ──────────────────────────────────────────────── + +describe('declaredEntries', () => { + it('reads every quoting dialect changesets writes', () => { + expect( + declaredEntries('---\n"@a/one": minor\n\'@a/two\': patch\n@a/three: major\n---\n\nbody\n'), + ).toEqual([ + { name: '@a/one', bump: 'minor' }, + { name: '@a/two', bump: 'patch' }, + { name: '@a/three', bump: 'major' }, + ]); + }); + + it('reads an EMPTY frontmatter as declaring nothing, not as undeclared', () => { + // The repository's explicit "this releases nothing" exemption. It is a real + // declaration with zero entries, and overwriting one still loses a file + // somebody wrote on purpose. + expect(declaredEntries('---\n---\n\nTest-only change.\n')).toEqual([]); + }); + + it('declares nothing when there is no closed frontmatter block at all', () => { + expect(declaredEntries('no frontmatter here\n')).toEqual([]); + expect(declaredEntries('---\n"@a/one": minor\n\nnever closed\n')).toEqual([]); + }); + + it('ignores body lines that merely look like entries', () => { + expect(declaredEntries('---\n"@a/one": minor\n---\n\n"@a/two": patch\n')).toEqual([ + { name: '@a/one', bump: 'minor' }, + ]); + }); +}); diff --git a/scripts/check-changeset-overwrite.mjs b/scripts/check-changeset-overwrite.mjs new file mode 100644 index 000000000..cbb92c408 --- /dev/null +++ b/scripts/check-changeset-overwrite.mjs @@ -0,0 +1,381 @@ +#!/usr/bin/env node +/** + * Reports when a change MODIFIES or DELETES a `.changeset/*.md` that already + * existed at its merge base — a changeset it does not own. + * + * Run: node scripts/check-changeset-overwrite.mjs + * Exit: 0 = nothing pre-existing was touched, the touch is release consumption, + * or REPORT-ONLY mode (the default) declined to fail on the findings + * 1 = the inputs could not be read, or `OS_CHANGESET_OVERWRITE_ENFORCE=1` + * and a pre-existing changeset was modified or deleted + * + * ## The hazard (objectui#6336) + * + * A dev run wrote its changeset to a hand-picked `changesets`-style name, + * `olive-donkeys-smile.md`. That file already existed on `main`, carrying an + * unrelated `@object-ui/plugin-charts: minor`. The heredoc overwrote it. It was + * caught before any commit and no damage occurred — this gate is about the + * hazard, not that incident. + * + * What makes it worth a gate rather than a guideline: **the cost lands on a THIRD + * PARTY and is invisible at the time it happens.** The agent that picks a + * colliding name loses nothing; whichever earlier pull request's release + * declaration vanishes pays, and only discovers it when a package silently fails + * to bump. Both signals that should catch it fail: + * + * 1. `git status` shows ` M` (modified), not `??` (untracked). An agent checking + * "did my new file appear" reads a modification as its own write landing. + * Nothing says *you replaced someone else's file*. + * 2. The loss is a DELETED RELEASE DECLARATION. Nothing downstream flags it — + * the packages simply do not get bumped, with no red anywhere. + * + * The base rate is not small: 424 accumulated `.changeset/*.md` at the time of + * writing, against an `adjective-animal-verb` name space. + * + * What actually caught the near-miss was neither a gate nor a test: + * `check-changeset-presence` reported "0 changeset(s) added", and the dev read + * that gate's own detection logic instead of assuming the gate was wrong. This + * file exists to replace that thread. + * + * ## Why REPORT-ONLY, and what would justify flipping it + * + * Triage ruled "report-only first if the population has legitimate modify cases, + * and measure that before choosing". Measured over all 5281 first-parent commits + * on `main` (2026-01-13 .. 2026-08-25), `--diff-filter=MD` over + * `:(glob).changeset/*.md`: + * + * - 88 commits DELETE a pre-existing changeset (1644 files). 82 of them also + * modify a package `CHANGELOG.md` — release consumption, which is how + * changesets are meant to end. The remaining 6 are five hand deletions plus + * one hand-versioned release; four of the five delete a declaration and add + * a replacement in the same commit. + * - 12 commits MODIFY a pre-existing changeset (19 files), and **all 19 are + * legitimate**: bump-level corrections (`major` to `minor` when the pending + * release line changed, `minor` to `patch` after review — 11 files across 4 + * commits), factual corrections to prose ("eleven" to "ten" locale packs; a + * typo'd package name `@objectstack/console` to `@object-ui/console`), and + * authors amending their own not-yet-released changeset after the change + * grew. + * + * So the premise "a PR modifying a pre-existing changeset is almost always a + * mistake" is NOT true of this repository's history — 19 for 19 against — and a + * blocking gate would have failed every one of those pull requests. Hence + * report-only. `OS_CHANGESET_OVERWRITE_ENFORCE=1` flips it for whoever revisits + * this with a new measurement; the switch is here so that flip is a decision + * rather than a rewrite. + * + * ⭐ The same sweep did find a NARROWER signal that history does not contradict. + * Of those 19 legitimate modifications, 18 keep every package NAME they declared + * at base — a bump level changes, prose changes, names do not disappear. The one + * exception dropped `@objectstack/console`, a name that resolves to no package in + * this workspace, because that was the typo being fixed. An overwrite is the + * opposite shape: the previous declaration's names are simply gone. This gate + * therefore REPORTS lost declarations separately and loudly, and a future + * blocking rule scoped to "a workspace package's declaration disappeared" would + * have been 0-for-19 on this history. Deliberately not built today: one + * explainable hit is not a measurement, and the ruling was report-only first. + * + * ## Why this lives in `changeset-guard.yml` + * + * That workflow's trigger is `paths: ['.changeset/**', ...]` — the inverse filter + * documented in its own header. A path filter is normally a false-negative + * surface, but not here: a change that modifies or deletes a `.changeset/*.md` + * touches `.changeset/**` BY DEFINITION, so the filter cannot miss this gate's + * subject matter. It is a second job rather than a step in `no-major` because it + * reads a diff and therefore needs `fetch-depth: 0`, which that job does not + * want. + * + * ## Never silent + * + * An unreadable input is exit 1, never a pass, even in report-only mode. A diff + * gate that cannot compute its diff and exits 0 has reported "clean" while + * looking at nothing (objectstack#4928, objectui#4690). Report-only means it does + * not fail on FINDINGS; it still fails on not knowing. + */ + +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { isEntrypoint } from './invoked-as.mjs'; +import { + NOT_A_CHANGESET, + changedFiles, + describeDeclaration, + resolveBaseRef, + untrackedChangesets, +} from './check-changeset-presence.mjs'; + +const scriptDir = dirname(fileURLToPath(import.meta.url)); + +/** The declarations, as a pathspec. `.changeset/config.json` is not `.md`. */ +const CHANGESETS = ':(glob).changeset/*.md'; + +/** + * Where a release writes its output. A modification here is what separates + * `changeset version` emptying the queue from a pull request deleting somebody + * else's declaration — measured across the whole history of `main`: 93 commits + * modify a package `CHANGELOG.md`, 92 of them are release commits, and the one + * that is not (`e5537d1cb`, a repo-wide reference rename) deletes no changeset. + */ +const CHANGELOGS = [':(glob)packages/*/CHANGELOG.md', ':(glob)apps/*/CHANGELOG.md']; + +const isDeclaration = (file) => !NOT_A_CHANGESET.has(file.slice(file.lastIndexOf('/') + 1)); + +/** + * The `name: bump` entries a changeset's frontmatter declares. + * + * The same hand-parsed dialect as `describeDeclaration`, which counts these + * entries without naming them, and hand-parsed for the same reason it is there: + * this gate runs on a bare checkout with no `pnpm install`, so it may not import + * a YAML parser or `@changesets/*`. `describeDeclaration` stays the authority on + * whether a file declares anything at all; this only reads the names out of a + * block it has already accepted. + * + * @returns {{ name: string, bump: string }[]} + */ +export function declaredEntries(source) { + if (describeDeclaration(source).kind === 'none') return []; + const lines = source.split(/\r?\n/); + const open = lines.findIndex((line) => line.trim() === '---'); + const entries = []; + for (let i = open + 1; i < lines.length; i++) { + const text = lines[i].trim(); + if (text === '---') break; + const match = /^(?:"([^"]+)"|'([^']+)'|([^:]+?))\s*:\s*(major|minor|patch)\s*$/.exec(text); + if (match) entries.push({ name: match[1] ?? match[2] ?? match[3], bump: match[4] }); + } + return entries; +} + +/** + * One file's content at a revision, or `null` when the path is not there. + * + * `ref === null` means the WORKING TREE, matching `check-changeset-presence`: an + * author running this locally gets the answer before committing. A file deleted + * in the working tree is absent from disk, which is the same `null` a path + * missing from a commit gives — the caller already knows which side it asked + * about. + */ +function contentAt(root, ref, file) { + if (ref === null) { + try { + return readFileSync(join(root, file), 'utf8'); + } catch { + return null; + } + } + try { + return execFileSync('git', ['show', `${ref}:${file}`], { + cwd: root, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch { + return null; + } +} + +/** + * @typedef {object} Hit + * @property {string} file + * @property {{ name: string, bump: string }[]} before what it declared at `base` + * @property {{ name: string, bump: string }[]} after what it declares now (`[]` when deleted) + * @property {string[]} lost package names declared at `base` and gone now + */ + +/** + * @typedef {object} Analysis + * @property {Hit[]} modified + * @property {Hit[]} deleted + * @property {string[]} added + * @property {number} changelogsTouched + * @property {boolean} releaseConsumption + */ + +/** + * What this change does to changesets it did not create. + * + * `base` is the merge base, so "pre-existing" needs no ownership bookkeeping: a + * changeset this change ADDED and then edited on the same branch is an `A` + * against the merge base, never an `M`. Only somebody else's file can be `M`. + * + * Throws on any unreadable input; the CLI turns that into exit 1. + * + * @returns {Analysis} + */ +export function collisions(root, { base, head = null }) { + const list = (filter, pathspecs) => changedFiles(root, { base, head, filter, pathspecs }).filter(isDeclaration); + + const at = (ref, file) => { + const source = contentAt(root, ref, file); + return source === null ? [] : declaredEntries(source); + }; + + const modified = list('M', [CHANGESETS]).map((file) => { + const before = at(base, file); + const after = at(head, file); + const names = new Set(after.map((entry) => entry.name)); + return { file, before, after, lost: before.map((entry) => entry.name).filter((name) => !names.has(name)) }; + }); + + const deleted = list('D', [CHANGESETS]).map((file) => { + const before = at(base, file); + return { file, before, after: [], lost: before.map((entry) => entry.name) }; + }); + + const changelogsTouched = changedFiles(root, { base, head, filter: 'M', pathspecs: CHANGELOGS }).length; + + // Untracked files join the ADDED count for the same reason `check-changeset- + // presence` counts them: `git diff` against the working tree cannot see one, + // and `pnpm changeset` leaves precisely that. Without it an author running + // this locally right after writing a changeset is told "0 added", which is the + // misleading half of the very summary line this gate uses to orient them. + // Empty in CI, where the checkout has no untracked files. It changes no + // verdict — an untracked file cannot be a modification of somebody else's. + const added = [...list('A', [CHANGESETS]), ...(head === null ? untrackedChangesets(root).filter(isDeclaration) : [])]; + + return { + modified, + deleted, + added: [...new Set(added)].sort(), + changelogsTouched, + // A release consumes the queue and writes the CHANGELOGs in one change. + // Modifications are deliberately NOT covered by this: `changeset version` + // deletes changesets, it never edits them, so a modification riding along + // with a release is the same finding as any other. + releaseConsumption: deleted.length > 0 && changelogsTouched > 0, + }; +} + +/** What this gate speaks up about; release consumption is not a finding. */ +export const findings = (analysis) => + analysis.releaseConsumption ? analysis.modified : [...analysis.modified, ...analysis.deleted]; + +/** + * `0` unless enforcement is switched on AND there is something to report. + * + * Report-only is the DEFAULT and the shipped behaviour — see the header for the + * measurement that chose it. Note what this does NOT gate on: an unreadable + * input never reaches here, because the CLI exits 1 before calling it. + */ +export function verdict(analysis, { enforce = false } = {}) { + return enforce && findings(analysis).length > 0 ? 1 : 0; +} + +const describe = (entries) => + entries.length === 0 ? 'nothing (no frontmatter entries)' : entries.map((e) => `${e.name}: ${e.bump}`).join(', '); + +// -- CLI ---------------------------------------------------------------------- + +if (isEntrypoint(import.meta.url)) { + const argOf = (name) => { + const index = process.argv.indexOf(name); + return index > -1 ? process.argv[index + 1] : null; + }; + + // `--root` points the gate at another checkout — a worktree, or one of the + // throwaway repositories `check-changeset-overwrite.test.ts` builds to + // exercise these exit codes end to end. `--base` / `--head` name the two + // commits; both default to "this branch against its merge base with the + // target branch", reading the working tree as the head side. + const root = resolve(argOf('--root') ?? resolve(scriptDir, '..')); + const head = argOf('--head'); + const enforce = process.env.OS_CHANGESET_OVERWRITE_ENFORCE === '1'; + + const base = resolveBaseRef(root, { explicit: argOf('--base') }); + if (!base.ok) { + console.error( + '❌ Cannot resolve the commit to compare against, so there is nothing to diff.\n' + + ` tried: ${base.tried.join(', ')}\n` + + (base.named + ? ' That base was named EXPLICITLY, so it is not guessed around: comparing against\n' + + ' some other commit would answer a question nobody asked, and answer it\n' + + ' confidently. Name a commit that exists in this clone, or pass none.\n' + : base.shallow + ? ' This clone is SHALLOW. In CI, give the checkout `fetch-depth: 0`; locally,\n' + + ' run `git fetch --no-tags origin main` (or `git fetch --unshallow`).\n' + : ' Fetch the base branch (`git fetch --no-tags origin main`) and re-run.\n') + + ' A failure, not a skip: report-only means this gate declines to fail on its\n' + + ' FINDINGS, never that it passes without looking (objectstack#4928, objectui#4690).', + ); + process.exit(1); + } + + let analysis; + try { + analysis = collisions(root, { base: base.ref, head }); + } catch (error) { + console.error( + `❌ ${error.message}\n\n` + + ' Reported as a failure rather than a pass: losing an input means this gate\n' + + ' cannot tell whether anything was overwritten (objectui#4690).', + ); + process.exit(1); + } + + console.log( + `Compared ${head ?? 'the working tree'} with ${base.ref.slice(0, 9)} (${base.how}): ` + + `${analysis.added.length} changeset(s) added, ${analysis.modified.length} modified, ` + + `${analysis.deleted.length} deleted.`, + ); + + if (analysis.releaseConsumption) { + console.log( + `✅ Release consumption: ${analysis.deleted.length} changeset(s) deleted alongside ` + + `${analysis.changelogsTouched} package CHANGELOG.md update(s). That is how the queue is ` + + 'emptied at release time, not a collision.', + ); + } + + const reported = findings(analysis); + if (reported.length === 0) { + console.log('✅ No pre-existing changeset was modified or deleted.'); + process.exit(0); + } + + const say = enforce ? console.error : console.log; + + say( + `\n${enforce ? '❌' : '⚠️'} This change touches ${reported.length} changeset(s) it did not add — ` + + `they already existed at ${base.ref.slice(0, 9)}:\n`, + ); + for (const hit of reported) { + say(` ${analysis.deleted.includes(hit) ? 'D' : 'M'} ${hit.file}`); + say(` declared at base: ${describe(hit.before)}`); + say(` declares now: ${describe(hit.after)}`); + if (hit.lost.length > 0) say(` ⚠️ GONE from the declaration: ${hit.lost.join(', ')}`); + } + + say(` + A changeset is APPEND-ONLY by nature: written once, consumed by the release. A + change that edits or removes one it did not add is usually one of three things. + + 1. You picked a FILENAME THAT ALREADY EXISTED and wrote over somebody else's + declaration (objectui#6336). This is the one that costs a THIRD PARTY a + release: their packages silently do not bump, and nothing downstream says + so. \`git status\` shows \` M\`, not \`??\`, so it reads like your own new file + landing. If the "declared at base" line above names packages your change + has nothing to do with, this is what happened — restore the file with + \`git checkout ${base.ref.slice(0, 9)} -- \` and write yours under a + name that cannot collide. + + 2. You are CORRECTING a declaration on purpose — a bump level after review, a + wrong package name, prose that no longer matches the change. Legitimate, + and the reason this gate reports instead of failing. + + 3. You are SUPERSEDING one with a replacement added in the same change. + Legitimate. + + ⭐ Name a changeset after the issue it settles — \`.changeset/-.md\` — + and case 1 cannot happen. That is the convention this repository already + follows; the \`adjective-animal-verb\` names come from \`pnpm changeset\`, which + allocates them against the files already present, and hand-picking one does not. + + Report-only: this gate does not fail the build on the findings above. See the + header of scripts/check-changeset-overwrite.mjs for the measurement behind that, + and for OS_CHANGESET_OVERWRITE_ENFORCE.`); + + process.exit(verdict(analysis, { enforce })); +} diff --git a/scripts/dependabot-merge-gate.mjs b/scripts/dependabot-merge-gate.mjs index 706f15021..7aede0f0f 100644 --- a/scripts/dependabot-merge-gate.mjs +++ b/scripts/dependabot-merge-gate.mjs @@ -211,6 +211,8 @@ export const NOT_A_GATE = Object.freeze({ 'live-e2e.yml is declared INFORMATIONAL and NON-REQUIRED in its own header and runs `continue-on-error: true`; ci-cd-pipeline.md says in as many words not to add it to required checks.', label: 'labeler.yml applies labels. It is a mutation, not a verdict — nothing about the change is judged by it.', + 'Changeset Overwrite Report': + "changeset-guard.yml's second job is REPORT-ONLY by measurement (objectui#6336): it names any `.changeset/*.md` the change modified or deleted without having added it, and exits 0 whatever it finds — all 19 such modifications in this repository's history were legitimate, so blocking would have failed every one of those pull requests. It goes red only when it cannot compute its diff, which is a fact about the checkout rather than a verdict on the change. Its pull_request trigger is also path-filtered to `.changeset/**` and the two gate scripts, so a Dependabot bump never produces this check at all.", 'Live half-state sweep': 'half-state-patrol.yml is REPORT-ONLY by ruling (objectui#5791): a completed sweep exits 0 whether it found 0 half-states or 40, and the job gates no branch and blocks no queue. It goes red only when the sweep could not RUN — the patrol reporting its own death, which is a fact about the patrol, not a verdict on the pull request. Its pull_request trigger is also path-filtered to the sweeper and the workflow, so a Dependabot bump never produces this check at all.', });