From 114b0eeaba1d3cb9cb142e4521c79b396d3e439d Mon Sep 17 00:00:00 2001 From: daikaginza Date: Mon, 28 Sep 2026 08:14:11 +0700 Subject: [PATCH 1/3] feat(tooling): one changelog file per pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CONTRIBUTING.md` asks every runtime change to add an entry at the top of `CHANGELOG.md`, so two pull requests in flight write into the same few lines and git has nothing to merge on. Against `main` at `81eb443`, `git merge-tree` over the 85 open pull requests GitHub reports as conflicting that aren't mine puts `CHANGELOG.md` in the conflict set of 41 — more than any other path — and for 6 it is the only one. `docs/CURRENT-STATE.md`, the other file rule 3 names, conflicts for 23, and merges cleanly for 23 of the 41: edits land in the section for the area you touched, so they only collide when the areas do. A pull request now adds `changelog.d/-.md` instead. Distinct filenames never conflict, and the pull request number makes them distinct for free. `npm run changelog` splices the directory into `CHANGELOG.md` and empties it; `npm run changelog:check` reports the batch and changes nothing. Fragment text is copied through unchanged apart from trailing whitespace. `src/annotations/drawTool.test.mjs` reads `CHANGELOG.md` as prose and slices a fixed window after a heading, so an assembler that reflowed an entry could move text across that window. Fragments are deliberately outside `scripts/format-scope.json` for the same reason. The assembler refuses a batch before writing anything when a filename is not `-.md` or two fragments claim one pull request number, and a unit test runs that check over the real `changelog.d/`, so a fragment the release could not assemble fails `npm test` in the pull request that adds it rather than at release time. Refs #781. --- CONTRIBUTING.md | 2 +- changelog.d/README.md | 45 ++++++ docs/MAINTAINER_WORKFLOW.md | 11 +- package.json | 2 + scripts/assemble-changelog.mjs | 120 ++++++++++++++++ scripts/format-scope.json | 4 +- src/tooling/changelogFragments.test.mjs | 178 ++++++++++++++++++++++++ 7 files changed, 359 insertions(+), 3 deletions(-) create mode 100644 changelog.d/README.md create mode 100644 scripts/assemble-changelog.mjs create mode 100644 src/tooling/changelogFragments.test.mjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cdfbb6547..56775e824 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,7 +149,7 @@ ownership and adoption process. 1. Branch off `main`. 2. Keep `npm run build`, `npm test`, and `npm run test:track` green and avoid new console errors, plus the [feature gate](#feature-regression-gates) for the area you touched. -3. If you change runtime behavior, update `docs/CURRENT-STATE.md` and `CHANGELOG.md` in the same PR. +3. If you change runtime behavior, update `docs/CURRENT-STATE.md` in the same PR, and add your changelog entry as `changelog.d/-.md` instead of editing `CHANGELOG.md` — see [changelog.d/README.md](changelog.d/README.md). Distinct filenames don't conflict; a release assembles the directory into `CHANGELOG.md`. A documentation claim that needs a regression test should assert against `docs/CURRENT-STATE.md` or `README.md`, which a PR still edits directly. 4. If you add or change a data source, update [DATA_SOURCES.md](DATA_SOURCES.md) with its license and attribution. **Don't add data you don't have the right to redistribute** — fetch it at runtime instead. 5. Describe what you changed and how you verified it (screenshots welcome for anything visual). diff --git a/changelog.d/README.md b/changelog.d/README.md new file mode 100644 index 000000000..26a7c2e07 --- /dev/null +++ b/changelog.d/README.md @@ -0,0 +1,45 @@ +# Changelog fragments + +One file per entry. A pull request adds a file here instead of editing +`CHANGELOG.md`, and a release concatenates them into it. + +``` +changelog.d/-.md +``` + +`723-atc-decision-layer.md`, for example. The slug is lowercase, words separated +by `-`. Two pull requests never pick the same filename, because the pull request +number is already unique — which is the point: new `CHANGELOG.md` entries go at +the top of the file, so two pull requests in flight write into the same few lines +and git has nothing to merge on. + +Write the file exactly as the entry should read in `CHANGELOG.md`: a `-` bullet, +wrapped the way the surrounding entries are, crediting yourself and the pull +request. + +```markdown +- Report which upstream declined a Street Traffic road load. The layer row now + reads `Overpass rate-limited` instead of a general "Road data temporarily + unavailable" (yourname, #665). +``` + +The text is spliced in unchanged apart from trailing whitespace, so what you +write here is what ships. Fragments are deliberately outside +`scripts/format-scope.json`, so `npm run format` never reflows one. + +At release time: + +``` +npm run changelog:check # list what would move, change nothing +npm run changelog # move every fragment into CHANGELOG.md +``` + +The assembler refuses a batch rather than guessing: a filename that is not +`-.md`, or two fragments claiming one pull request number, +stops the run before anything is written and names the files. + +A documentation claim that needs a regression test — the way +`src/annotations/drawTool.test.mjs` asserts against `CHANGELOG.md` — should +assert against `docs/CURRENT-STATE.md` or `README.md` instead. A pull request +still edits those directly, so the text is in the file while the change is under +review; a fragment does not reach `CHANGELOG.md` until release. diff --git a/docs/MAINTAINER_WORKFLOW.md b/docs/MAINTAINER_WORKFLOW.md index 39c87be4b..632400c9c 100644 --- a/docs/MAINTAINER_WORKFLOW.md +++ b/docs/MAINTAINER_WORKFLOW.md @@ -57,7 +57,8 @@ with a reason; usefulness can also be `decline` or `discuss`. - Inspect removed tests, skipped tests, relaxed assertions, snapshot updates, mocks, and test-runner changes. A green result from weakened tests is not acceptance. - Check applicable documentation updates: runtime changes need `CURRENT-STATE.md` - and `CHANGELOG.md`; source changes need `DATA_SOURCES.md`. + and a `changelog.d/-.md` fragment rather than an edit to + `CHANGELOG.md`; source changes need `DATA_SOURCES.md`. ### 3. Security and supply chain @@ -173,6 +174,14 @@ Before merging: a stale merge. Verify the merged result matches the validated candidate tree and record the resulting commit and PR URL. Stop and report any mismatch. +Pending release notes live in `changelog.d/` as one file per pull request, so an +integration never has to resolve the top of `CHANGELOG.md`. When cutting a release, +run `npm run changelog:check` to see the batch and the order it would take, then +`npm run changelog` to splice it into `CHANGELOG.md` and empty the directory. The +assembler refuses a malformed filename or two fragments claiming one pull request +number before it writes anything, and copies the text through unchanged, so the +contributor's own wording and credit are what ship. + Repository rules enforce only their configured requirements; this document and the skill do not configure branch protection or CI. Maintainers should require CI and review on `main` and dismiss stale approvals when changes are pushed. diff --git a/package.json b/package.json index e48881c6d..4fd753168 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,8 @@ "format:check": "node scripts/format.mjs --check", "layer-token:next": "node scripts/next-layer-state-token.mjs", "layer-token:check": "node scripts/check-layer-state-tokens.mjs", + "changelog": "node scripts/assemble-changelog.mjs --write", + "changelog:check": "node scripts/assemble-changelog.mjs --check", "dev": "vite", "dev:secure": "./scripts/dev-secure.sh", "opensky:import": "./scripts/opensky-import-client.sh", diff --git a/scripts/assemble-changelog.mjs b/scripts/assemble-changelog.mjs new file mode 100644 index 000000000..8f3e854ef --- /dev/null +++ b/scripts/assemble-changelog.mjs @@ -0,0 +1,120 @@ +import { readdir, readFile, writeFile, rm } from 'node:fs/promises'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +/** + * Release-time assembly of changelog fragments. + * + * A pull request adds `changelog.d/-.md` instead of editing + * `CHANGELOG.md`. Distinct filenames never conflict, so two pull requests in + * flight no longer write into the same few lines at the top of one file. A + * release splices the fragments in and empties the directory. + * + * Fragment text is copied through unchanged apart from trailing whitespace: + * `src/annotations/drawTool.test.mjs` reads `CHANGELOG.md` as prose and slices a + * fixed window after a heading, so reflowing an entry here could move text + * across that window. + */ +export const FRAGMENT_DIR = 'changelog.d'; +const FRAGMENT_NAME = /^(\d+)-[a-z0-9]+(?:-[a-z0-9]+)*\.md$/; +const HEADING = '# Changelog\n'; + +/** Fragment file names a release may consume, highest pull request first. */ +export function orderFragmentNames(names) { + const claimed = new Map(); + for (const name of names) { + if (name === 'README.md') continue; + const match = FRAGMENT_NAME.exec(name); + if (!match) { + throw new Error( + `Unexpected file in ${FRAGMENT_DIR}/: ${name}` + + ' — expected -.md with a lowercase slug', + ); + } + const number = Number(match[1]); + const prior = claimed.get(number); + if (prior) { + throw new Error( + `Two fragments claim pull request ${number}: ${prior} and ${name}`, + ); + } + claimed.set(number, name); + } + return [...claimed.entries()] + .sort(([a], [b]) => b - a) + .map(([, name]) => name); +} + +/** Splice fragment bodies above the existing entries, newest batch on top. */ +export function assembleChangelog(changelog, fragments) { + if (!fragments.length) return changelog; + if (!changelog.startsWith(HEADING)) { + throw new Error('CHANGELOG.md must start with "# Changelog"'); + } + const entries = changelog.slice(HEADING.length).replace(/^\n+/, ''); + const batch = fragments.map((text) => text.replace(/\s+$/, '')).join('\n\n'); + return `${HEADING}\n${batch}\n\n${entries}`; +} + +/** + * Report or apply the pending fragments in `/changelog.d`. + * + * The batch is validated in full before anything is written, so a malformed + * filename or a duplicate pull request number leaves the directory untouched. + * + * @param {string} root repository root + * @param {'--check'|'--write'} mode `--check` reports and changes nothing + * @returns {Promise<{ consumed: string[], changelog: string }>} + */ +export async function assembleFragments(root, mode) { + if (!['--check', '--write'].includes(mode)) { + throw new Error( + 'Usage: node scripts/assemble-changelog.mjs --check|--write', + ); + } + const directory = path.join(root, FRAGMENT_DIR); + const changelogFile = path.join(root, 'CHANGELOG.md'); + const consumed = orderFragmentNames(await readdir(directory)); + const bodies = []; + for (const name of consumed) { + bodies.push(await readFile(path.join(directory, name), 'utf8')); + } + const changelog = assembleChangelog( + await readFile(changelogFile, 'utf8'), + bodies, + ); + if (mode === '--write' && consumed.length) { + await writeFile(changelogFile, changelog, 'utf8'); + for (const name of consumed) await rm(path.join(directory, name)); + } + return { consumed, changelog }; +} + +const invoked = process.argv[1] + ? pathToFileURL(path.resolve(process.argv[1])).href + : ''; +if (import.meta.url === invoked) { + try { + if (process.argv.length !== 3) { + throw new Error( + 'Usage: node scripts/assemble-changelog.mjs --check|--write', + ); + } + const mode = process.argv[2]; + const { consumed } = await assembleFragments( + fileURLToPath(new URL('../', import.meta.url)), + mode, + ); + if (!consumed.length) { + console.log(`No fragments in ${FRAGMENT_DIR}/.`); + } else { + console.log( + `${mode === '--write' ? 'Assembled' : 'Would assemble'} ${consumed.length} fragment(s), in order:` + + `\n ${consumed.join('\n ')}`, + ); + } + } catch (error) { + console.error(error.message); + process.exitCode = 1; + } +} diff --git a/scripts/format-scope.json b/scripts/format-scope.json index ef44df03f..974275366 100644 --- a/scripts/format-scope.json +++ b/scripts/format-scope.json @@ -844,5 +844,7 @@ "src/ui/imageryBoxTool.test.mjs", "src/ui/recentImagery.test.mjs", "src/layers/recentImagery/testDoubles.mjs", - "scripts/qa-recent-imagery.mjs" + "scripts/qa-recent-imagery.mjs", + "scripts/assemble-changelog.mjs", + "src/tooling/changelogFragments.test.mjs" ] diff --git a/src/tooling/changelogFragments.test.mjs b/src/tooling/changelogFragments.test.mjs new file mode 100644 index 000000000..880e3318d --- /dev/null +++ b/src/tooling/changelogFragments.test.mjs @@ -0,0 +1,178 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { + mkdtemp, + mkdir, + writeFile, + readFile, + readdir, + rm, +} from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { + FRAGMENT_DIR, + assembleChangelog, + assembleFragments, + orderFragmentNames, +} from '../../scripts/assemble-changelog.mjs'; + +const EXISTING = + '# Changelog\n\n- An entry that was already released (someone, #1).\n'; + +const repositoryRoot = fileURLToPath(new URL('../..', import.meta.url)); + +async function fixture(t, fragments = {}) { + const root = await mkdtemp(path.join(tmpdir(), 'gev-changelog-')); + t.after(() => rm(root, { recursive: true, force: true })); + await mkdir(path.join(root, FRAGMENT_DIR)); + await writeFile( + path.join(root, FRAGMENT_DIR, 'README.md'), + '# How to add one\n', + ); + await writeFile(path.join(root, 'CHANGELOG.md'), EXISTING); + for (const [name, body] of Object.entries(fragments)) { + await writeFile(path.join(root, FRAGMENT_DIR, name), body); + } + return root; +} + +test('the batch reads newest pull request first', () => { + assert.deepEqual(orderFragmentNames(['7-b.md', '112-c.md', '19-a.md']), [ + '112-c.md', + '19-a.md', + '7-b.md', + ]); +}); + +test('the directory README is documentation, never an entry', () => { + assert.deepEqual(orderFragmentNames(['README.md', '5-a.md']), ['5-a.md']); +}); + +test('two fragments claiming one pull request fail loudly, not silently', () => { + assert.throws( + () => orderFragmentNames(['5-first.md', '5-second.md']), + /Two fragments claim pull request 5/, + ); +}); + +test('a name that is not -.md is refused', () => { + for (const name of ['notes.md', '5_underscore.md', '5-Caps.md', '5-a.txt']) { + assert.throws(() => orderFragmentNames([name]), /Unexpected file/, name); + } +}); + +test('fragment text is spliced in unchanged, above the existing entries', () => { + // Wrapped, indented and punctuated the way a real entry is: the continuation + // indent and the blank line inside the entry are the parts a reflow would eat. + const entry = [ + '- A new thing, with `code` and a — dash. The second line is indented the', + ' way the surrounding entries wrap, and it keeps its two spaces.', + '', + ' A second paragraph, because some entries have one (someone, #9).', + ].join('\n'); + const result = assembleChangelog(EXISTING, [`${entry}\n\n\n`]); + assert.equal( + result, + `# Changelog\n\n${entry}\n\n- An entry that was already released (someone, #1).\n`, + ); + // The entry survives byte for byte: drawTool.test.mjs reads this file as + // prose and slices a fixed window, so reflowing would move text across it. + assert.ok(result.includes(entry)); +}); + +test('a CHANGELOG without the heading is refused rather than prepended to', () => { + assert.throws( + () => assembleChangelog('- orphan entry\n', ['- new\n']), + /must start with "# Changelog"/, + ); +}); + +test('assembling consumes the fragments and leaves the README', async (t) => { + const root = await fixture(t, { + '20-second.md': '- Second (someone, #20).', + '100-first.md': '- First (someone, #100).', + }); + const { consumed } = await assembleFragments(root, '--write'); + assert.deepEqual(consumed, ['100-first.md', '20-second.md']); + const changelog = await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'); + assert.match( + changelog, + /# Changelog\n\n- First \(someone, #100\)\.\n\n- Second \(someone, #20\)\.\n\n- An entry that was already released/, + ); + assert.deepEqual(await readdir(path.join(root, FRAGMENT_DIR)), ['README.md']); +}); + +test('a rejected batch leaves the directory and the file untouched', async (t) => { + const root = await fixture(t, { + '8-first.md': '- First (someone, #8).', + '8-again.md': '- Again (someone, #8).', + }); + await assert.rejects( + assembleFragments(root, '--write'), + /Two fragments claim pull request 8/, + ); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); + assert.deepEqual((await readdir(path.join(root, FRAGMENT_DIR))).sort(), [ + '8-again.md', + '8-first.md', + 'README.md', + ]); +}); + +test('check mode changes nothing on disk', async (t) => { + const root = await fixture(t, { '3-a.md': '- Something (someone, #3).' }); + const { consumed } = await assembleFragments(root, '--check'); + assert.deepEqual(consumed, ['3-a.md']); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); + assert.deepEqual((await readdir(path.join(root, FRAGMENT_DIR))).sort(), [ + '3-a.md', + 'README.md', + ]); +}); + +test('an empty directory is a no-op, so a release can always run it', async (t) => { + const root = await fixture(t); + const { consumed } = await assembleFragments(root, '--write'); + assert.deepEqual(consumed, []); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); +}); + +test('a mode that is neither check nor write is refused', async (t) => { + const root = await fixture(t); + await assert.rejects(assembleFragments(root, '--force'), /--check\|--write/); +}); + +// The two below run against the repository itself, so a fragment that the +// release could not assemble fails `npm test` in the pull request that adds it +// rather than at release time, when its author has moved on. + +test('every pending fragment in the repository can be assembled', async () => { + const names = await readdir(path.join(repositoryRoot, FRAGMENT_DIR)); + assert.ok( + names.includes('README.md'), + `${FRAGMENT_DIR}/README.md documents the convention and keeps the directory tracked`, + ); + // Throws on a name that is not -.md, and on two fragments + // claiming one pull request number. Every other file is an entry. + assert.equal(orderFragmentNames(names).length, names.length - 1); +}); + +test('CHANGELOG.md still carries the heading a release splices under', async () => { + const changelog = await readFile( + path.join(repositoryRoot, 'CHANGELOG.md'), + 'utf8', + ); + assert.ok(changelog.startsWith('# Changelog\n')); +}); From d29ff40d04a1ce7de8872488159e38e39f570339 Mon Sep 17 00:00:00 2001 From: daikaginza Date: Mon, 28 Sep 2026 08:17:18 +0700 Subject: [PATCH 2/3] docs: add this pull request's own changelog fragment (#787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dogfoods the convention rather than describing it: the entry for #787 arrives as `changelog.d/787-changelog-fragments.md` and `CHANGELOG.md` is untouched. A fragment needs the pull request number, which only exists once the pull request is open, so it lands in a second commit. `changelog.d/README.md` says to push first and add the fragment after, for the same reason. Checked against the one consumer that constrains the design: assembling this fragment moves the `DISPLAY ▸ Draw` marker in `CHANGELOG.md` by exactly the length of the spliced entry, and the 1200-character window that `src/annotations/drawTool.test.mjs` slices after it is byte-identical. --- changelog.d/787-changelog-fragments.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog.d/787-changelog-fragments.md diff --git a/changelog.d/787-changelog-fragments.md b/changelog.d/787-changelog-fragments.md new file mode 100644 index 000000000..41509c046 --- /dev/null +++ b/changelog.d/787-changelog-fragments.md @@ -0,0 +1,7 @@ +- Changelog entries are now one file per pull request. A change adds + `changelog.d/-.md` instead of editing the top of + `CHANGELOG.md`, which two pull requests in flight could not share — it was the + most frequent merge conflict in the open queue, and for six of them the only + one. `npm run changelog` splices the directory into `CHANGELOG.md` at release + time and empties it; `npm run changelog:check` reports the batch and changes + nothing. Fragment text is copied through unchanged (daikaginza, #787). From 4d42516ffedeb4ebcc24ee30004d4f8ec914790f Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Fri, 16 Oct 2026 11:01:25 +0100 Subject: [PATCH 3/3] Make changelog assembly recoverable after a partial write `--write` wrote CHANGELOG.md and then removed fragments one by one, so a failure after the write left surviving fragments that a rerun spliced in a second time. The write is now staged. The assembled file goes into changelog.d/.assembling/, the fragments are renamed in beside it, and renaming the assembled file over CHANGELOG.md is the single commit point. A run that fails rolls itself back when it can; otherwise the next `npm run changelog` reads the staging directory and either restores the fragments (assembled file still staged) or deletes the leftovers (batch landed). `--check` reports a staged batch instead of reporting past it. A failure-injection test makes every filesystem call in a `--write` fail in turn, both as a one-off error and as a crash with no in-process rollback. It asserts the retry produces exactly the uninterrupted CHANGELOG.md, with each entry once and an empty directory. The test fails against the previous write-then-delete and against delete-first. Co-Authored-By: Claude Opus 5.5 --- .gitignore | 3 + changelog.d/README.md | 7 ++ docs/MAINTAINER_WORKFLOW.md | 4 +- scripts/assemble-changelog.mjs | 98 ++++++++++++++-- src/tooling/changelogFragments.test.mjs | 141 ++++++++++++++++++++++++ 5 files changed, 245 insertions(+), 8 deletions(-) diff --git a/.gitignore b/.gitignore index 14e8d320b..d2f0107c0 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,6 @@ qa-artifacts/ 3d-models/ pinokio/ENVIRONMENT pinokio/.installed + +# Transient: a release interrupted mid-assembly (npm run changelog recovers it) +changelog.d/.assembling/ diff --git a/changelog.d/README.md b/changelog.d/README.md index 26a7c2e07..df17e32f2 100644 --- a/changelog.d/README.md +++ b/changelog.d/README.md @@ -38,6 +38,13 @@ The assembler refuses a batch rather than guessing: a filename that is not `-.md`, or two fragments claiming one pull request number, stops the run before anything is written and names the files. +If `npm run changelog` fails part-way (a full disk, a killed process), run it +again. The batch is staged in `changelog.d/.assembling/` and only lands when the +assembled file replaces `CHANGELOG.md` in a single rename, so the rerun either +finishes that batch or puts its fragments back first. It never adds an entry +twice or drops one. `npm run changelog:check` names a staged batch, if one is +left over, instead of reporting past it. + A documentation claim that needs a regression test — the way `src/annotations/drawTool.test.mjs` asserts against `CHANGELOG.md` — should assert against `docs/CURRENT-STATE.md` or `README.md` instead. A pull request diff --git a/docs/MAINTAINER_WORKFLOW.md b/docs/MAINTAINER_WORKFLOW.md index 5c773f399..ec6cd026d 100644 --- a/docs/MAINTAINER_WORKFLOW.md +++ b/docs/MAINTAINER_WORKFLOW.md @@ -130,7 +130,9 @@ resolves the top of `CHANGELOG.md`. At release, `npm run changelog:check` previe the batch and its order; `npm run changelog` splices it into `CHANGELOG.md` and empties the directory. The assembler refuses a malformed filename or two fragments claiming one PR number before writing, and copies text through unchanged so the -contributor's wording and credit ship. +contributor's wording and credit ship. The write is staged in +`changelog.d/.assembling/`, so if it fails part-way, rerunning `npm run changelog` +finishes or rolls back the batch. It never duplicates or drops an entry. Return this concise record privately unless posting is authorized. Keep sensitive security evidence in the private report: diff --git a/scripts/assemble-changelog.mjs b/scripts/assemble-changelog.mjs index 8f3e854ef..430fca77a 100644 --- a/scripts/assemble-changelog.mjs +++ b/scripts/assemble-changelog.mjs @@ -1,4 +1,4 @@ -import { readdir, readFile, writeFile, rm } from 'node:fs/promises'; +import * as fs from 'node:fs/promises'; import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -14,8 +14,19 @@ import { fileURLToPath, pathToFileURL } from 'node:url'; * `src/annotations/drawTool.test.mjs` reads `CHANGELOG.md` as prose and slices a * fixed window after a heading, so reflowing an entry here could move text * across that window. + * + * A write is staged so that a failure at any point leaves a rerun unable to + * duplicate or lose an entry. The assembled file is written into + * `changelog.d/.assembling/` first, the fragments are renamed in beside it, and + * renaming the assembled file over `CHANGELOG.md` is the single commit point. + * While the assembled file is still staged the batch has not landed, so + * recovery moves the fragments back; once it is gone the batch has landed, and + * the staged fragments are leftovers to delete. Either way they are out of the + * input directory, so they are never assembled twice. */ export const FRAGMENT_DIR = 'changelog.d'; +export const STAGING_DIR = '.assembling'; +const STAGED_CHANGELOG = 'CHANGELOG.md'; const FRAGMENT_NAME = /^(\d+)-[a-z0-9]+(?:-[a-z0-9]+)*\.md$/; const HEADING = '# Changelog\n'; @@ -56,36 +67,109 @@ export function assembleChangelog(changelog, fragments) { return `${HEADING}\n${batch}\n\n${entries}`; } +/** + * Finish or roll back a batch an earlier `--write` left in the staging + * directory. A no-op when there is none. + * + * @returns {Promise<'none'|'rolled-back'|'cleaned-up'>} + */ +export async function recoverInterruptedBatch(root, io = fs) { + const directory = path.join(root, FRAGMENT_DIR); + const staging = path.join(directory, STAGING_DIR); + let staged; + try { + staged = await io.readdir(staging); + } catch (error) { + if (error.code === 'ENOENT') return 'none'; + throw error; + } + if (!staged.includes(STAGED_CHANGELOG)) { + // The assembled file was renamed over CHANGELOG.md: the batch landed. + await io.rm(staging, { recursive: true, force: true }); + return 'cleaned-up'; + } + // The batch never reached CHANGELOG.md, so every fragment goes back. + const pending = new Set(await io.readdir(directory)); + const fragments = staged.filter((name) => name !== STAGED_CHANGELOG); + const clash = fragments.filter((name) => pending.has(name)); + if (clash.length) { + throw new Error( + `Cannot roll back ${FRAGMENT_DIR}/${STAGING_DIR}/: ${clash.join(', ')}` + + ` also exist in ${FRAGMENT_DIR}/ — resolve by hand`, + ); + } + for (const name of fragments) { + await io.rename(path.join(staging, name), path.join(directory, name)); + } + await io.rm(staging, { recursive: true, force: true }); + return 'rolled-back'; +} + /** * Report or apply the pending fragments in `/changelog.d`. * * The batch is validated in full before anything is written, so a malformed * filename or a duplicate pull request number leaves the directory untouched. + * `--write` first recovers any batch an interrupted run left staged. * * @param {string} root repository root * @param {'--check'|'--write'} mode `--check` reports and changes nothing + * @param {typeof fs} [io] filesystem calls, replaceable for failure injection * @returns {Promise<{ consumed: string[], changelog: string }>} */ -export async function assembleFragments(root, mode) { +export async function assembleFragments(root, mode, io = fs) { if (!['--check', '--write'].includes(mode)) { throw new Error( 'Usage: node scripts/assemble-changelog.mjs --check|--write', ); } const directory = path.join(root, FRAGMENT_DIR); + const staging = path.join(directory, STAGING_DIR); const changelogFile = path.join(root, 'CHANGELOG.md'); - const consumed = orderFragmentNames(await readdir(directory)); + if (mode === '--write') { + await recoverInterruptedBatch(root, io); + } + const names = await io.readdir(directory); + if (names.includes(STAGING_DIR)) { + throw new Error( + `${FRAGMENT_DIR}/${STAGING_DIR}/ holds a batch an interrupted release` + + ' left behind — `npm run changelog` finishes or rolls it back', + ); + } + const consumed = orderFragmentNames(names); const bodies = []; for (const name of consumed) { - bodies.push(await readFile(path.join(directory, name), 'utf8')); + bodies.push(await io.readFile(path.join(directory, name), 'utf8')); } const changelog = assembleChangelog( - await readFile(changelogFile, 'utf8'), + await io.readFile(changelogFile, 'utf8'), bodies, ); if (mode === '--write' && consumed.length) { - await writeFile(changelogFile, changelog, 'utf8'); - for (const name of consumed) await rm(path.join(directory, name)); + try { + await io.mkdir(staging); + await io.writeFile( + path.join(staging, STAGED_CHANGELOG), + changelog, + 'utf8', + ); + for (const name of consumed) { + await io.rename(path.join(directory, name), path.join(staging, name)); + } + await io.rename(path.join(staging, STAGED_CHANGELOG), changelogFile); + await io.rm(staging, { recursive: true, force: true }); + } catch (error) { + try { + await recoverInterruptedBatch(root, io); + } catch { + throw new Error( + `${error.message} — the batch is staged in ${FRAGMENT_DIR}/${STAGING_DIR}/,` + + ' and the next `npm run changelog` finishes or rolls it back', + { cause: error }, + ); + } + throw error; + } } return { consumed, changelog }; } diff --git a/src/tooling/changelogFragments.test.mjs b/src/tooling/changelogFragments.test.mjs index 880e3318d..792542f68 100644 --- a/src/tooling/changelogFragments.test.mjs +++ b/src/tooling/changelogFragments.test.mjs @@ -1,5 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; +import * as fs from 'node:fs/promises'; import { mkdtemp, mkdir, @@ -14,9 +15,11 @@ import { fileURLToPath } from 'node:url'; import { FRAGMENT_DIR, + STAGING_DIR, assembleChangelog, assembleFragments, orderFragmentNames, + recoverInterruptedBatch, } from '../../scripts/assemble-changelog.mjs'; const EXISTING = @@ -154,6 +157,144 @@ test('a mode that is neither check nor write is refused', async (t) => { await assert.rejects(assembleFragments(root, '--force'), /--check\|--write/); }); +// Failure injection. Every filesystem call the write path makes is a point +// where a release can die: a full disk, a permissions error, a killed process. + +const IO_CALLS = ['readdir', 'readFile', 'mkdir', 'writeFile', 'rename', 'rm']; +const BATCH = { + '20-second.md': '- Second (someone, #20).', + '100-first.md': '- First (someone, #100).', + '7-third.md': '- Third (someone, #7).', +}; + +/** + * The real filesystem, except that `call` throws the `nth` time it runs. With + * `crash`, every later call throws as well, the way a killed process makes no + * further calls: the in-process rollback cannot run either, so the next + * invocation is the only thing that can recover. + */ +function failing(call, nth, { crash = false } = {}) { + let seen = 0; + let dead = false; + const io = { ...fs }; + for (const name of IO_CALLS) { + io[name] = async (...args) => { + if (dead) throw new Error('process already gone'); + if (name === call && ++seen === nth) { + dead = crash; + throw new Error(`injected ${call} failure #${nth}`); + } + return fs[name](...args); + }; + } + return io; +} + +/** How many times one uninterrupted `--write` makes each filesystem call. */ +async function countCalls(t) { + const root = await fixture(t, BATCH); + const counts = Object.fromEntries(IO_CALLS.map((name) => [name, 0])); + const io = { ...fs }; + for (const name of IO_CALLS) { + io[name] = (...args) => { + counts[name] += 1; + return fs[name](...args); + }; + } + await assembleFragments(root, '--write', io); + return { + counts, + expected: await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + }; +} + +test('a write that fails at any call never lets a retry duplicate or lose an entry', async (t) => { + const { counts, expected } = await countCalls(t); + // The write path itself: one mkdir, one writeFile, a rename per fragment plus + // the commit rename, and the cleanup rm. Fewer would mean a step went missing. + assert.ok(counts.rename >= Object.keys(BATCH).length + 1); + for (const crash of [false, true]) { + for (const call of IO_CALLS) { + for (let nth = 1; nth <= counts[call]; nth += 1) { + const label = `${crash ? 'crash' : 'error'} at ${call} #${nth}`; + const root = await fixture(t, BATCH); + await assert.rejects( + assembleFragments(root, '--write', failing(call, nth, { crash })), + /injected|already gone/, + label, + ); + await assembleFragments(root, '--write'); + const changelog = await readFile( + path.join(root, 'CHANGELOG.md'), + 'utf8', + ); + assert.equal(changelog, expected, label); + for (const pr of [100, 20, 7, 1]) { + assert.equal( + changelog.split(`(someone, #${pr}).`).length - 1, + 1, + `${label}: #${pr} appears exactly once`, + ); + } + assert.deepEqual( + await readdir(path.join(root, FRAGMENT_DIR)), + ['README.md'], + label, + ); + } + } + } +}); + +test('an error before the commit rolls the batch back in the same run', async (t) => { + const root = await fixture(t, BATCH); + // The second rename has already moved one fragment into the staging directory. + await assert.rejects( + assembleFragments(root, '--write', failing('rename', 2)), + /injected rename failure #2/, + ); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); + assert.deepEqual( + (await readdir(path.join(root, FRAGMENT_DIR))).sort(), + [...Object.keys(BATCH), 'README.md'].sort(), + ); +}); + +test('check mode reports a batch an interrupted release left staged', async (t) => { + const root = await fixture(t, BATCH); + await assert.rejects( + assembleFragments(root, '--write', failing('rename', 2, { crash: true })), + ); + await assert.rejects( + assembleFragments(root, '--check'), + new RegExp(`${STAGING_DIR}/ holds a batch an interrupted release`), + ); + assert.equal(await recoverInterruptedBatch(root), 'rolled-back'); + assert.equal(await recoverInterruptedBatch(root), 'none'); +}); + +test('recovery refuses to roll a fragment back over a file of the same name', async (t) => { + const root = await fixture(t, BATCH); + await assert.rejects( + assembleFragments(root, '--write', failing('rename', 2, { crash: true })), + ); + await writeFile( + path.join(root, FRAGMENT_DIR, '100-first.md'), + '- Rewritten.', + ); + await assert.rejects( + assembleFragments(root, '--write'), + /Cannot roll back .*100-first\.md/, + ); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); +}); + // The two below run against the repository itself, so a fragment that the // release could not assemble fails `npm test` in the pull request that adds it // rather than at release time, when its author has moved on.