diff --git a/docs/architecture/release-pipeline.md b/docs/architecture/release-pipeline.md index 97028a772..88ef4fd59 100644 --- a/docs/architecture/release-pipeline.md +++ b/docs/architecture/release-pipeline.md @@ -27,7 +27,7 @@ chosen deliberately by bumping `package.json`. | GitHub release body | (tag build) | via `extract-changelog-section.mjs --public` | | Telegram announcement | `release:notes:telegram` | stdout | | Reddit announcement | `release:notes:reddit` | stdout | -| Highlight cards | `release:cards:generate` | `dist/release-highlight-cards//` | +| Highlight cards | `release:cards:generate` | `dist/release-highlight-cards/v/` | | Screenshots | `release:screenshots` | `apps/website/public/blog//screenshots/` | ### The ordering constraint that matters @@ -100,7 +100,11 @@ card into the website tree is a deliberate manual act. A release with no `highlight:` notes is not an error: the hero card is still rendered and the run exits 0. An internal-only release has nothing public to -put on a card and exits 0 having written nothing. +put on a card and exits 0, first clearing any cards an earlier run of the same +version left behind. An **empty** `.changes/` directory is a different thing +and does fail: it almost always means this step ran after `--consume`, and +reporting that as "internal-only" would hide the one ordering mistake the +pipeline is built to prevent. ## Draft verification diff --git a/tools/release/generate-highlight-cards.mjs b/tools/release/generate-highlight-cards.mjs index 85c788176..6628de4c1 100644 --- a/tools/release/generate-highlight-cards.mjs +++ b/tools/release/generate-highlight-cards.mjs @@ -140,6 +140,29 @@ async function prepareShotOverlay(screenshotPath) { return { input: cropped, left: SHOT_LEFT, top: SHOT_TOP }; } +/** + * Removes the cards a previous run of THIS generator left in `outputDir`, so a + * renamed, dropped, or newly-internal highlight cannot leave a stale image + * sitting there waiting to be published. Only files matching what this tool + * writes are touched; anything else in the directory is left alone. + * + * @param {string} outputDir + * @returns {string[]} removed filenames + */ +export function removeStaleCards(outputDir) { + if (!existsSync(outputDir)) { + return []; + } + + const stale = readdirSync(outputDir).filter(isOwnedCardFile); + + for (const entry of stale) { + rmSync(path.join(outputDir, entry)); + } + + return stale; +} + /** * @param {{ feature: object[], hero: object }} plan * @param {string} version @@ -148,16 +171,7 @@ async function prepareShotOverlay(screenshotPath) { */ export async function renderCards(plan, version, outputDir) { mkdirSync(outputDir, { recursive: true }); - - // Remove the cards a previous run of THIS generator left behind, so a - // renamed or dropped highlight cannot leave a stale image sitting beside - // the current set waiting to be published. Only files matching what this - // tool writes are touched; anything else in the directory is left alone. - for (const entry of readdirSync(outputDir)) { - if (isOwnedCardFile(entry)) { - rmSync(path.join(outputDir, entry)); - } - } + removeStaleCards(outputDir); const written = []; @@ -210,6 +224,22 @@ async function main() { process.exit(1); } + // An empty directory is not an internal-only release: it is almost always + // this step running AFTER `--consume` deleted the notes, which is the one + // ordering mistake the whole pipeline warns about. Reporting it as + // "internal-only" would hide the mistake and quietly ship no cards at all. + if (notes.length === 0) { + console.error( + [ + `No notes found in ${path.relative(workspaceRoot, notesDir)}/.`, + 'Cards are built from `highlight:`, which exists only in the note', + 'files — if `build-release-notes.mjs --consume` already ran, that', + 'metadata is gone. Render cards before consuming.', + ].join(' ') + ); + process.exit(1); + } + const slug = releaseSlug(options.version); const screenshotsDir = path.join( workspaceRoot, @@ -223,6 +253,14 @@ async function main() { screenshotsDir, theme: options.theme, }); + // Keyed by the exact version, not the minor slug: 0.24.0 and 0.24.1 share + // a blog post but not a card set, and mixing them in one directory invites + // publishing the previous patch's card. + const outputDir = path.resolve( + workspaceRoot, + options.out ?? + path.join('dist/release-highlight-cards', `v${options.version}`) + ); // Neither shape below is an error: an internal-only release is legal, and // a release nobody marked a highlight on still deserves its hero card. @@ -232,6 +270,19 @@ async function main() { 'Internal-only release: no public change to put on a card.' ); + // A release that turned internal-only after cards were already made + // for this exact version must not leave them behind. `--dry-run` + // deletes nothing. + if (options.mode === 'generate') { + const removed = removeStaleCards(outputDir); + + if (removed.length > 0) { + console.log( + `Removed ${removed.length} card(s) left by an earlier run of v${options.version}.` + ); + } + } + return; } @@ -256,15 +307,6 @@ async function main() { process.exit(1); } - // Keyed by the exact version, not the minor slug: 0.24.0 and 0.24.1 share - // a blog post but not a card set, and mixing them in one directory invites - // publishing the previous patch's card. - const outputDir = path.resolve( - workspaceRoot, - options.out ?? - path.join('dist/release-highlight-cards', `v${options.version}`) - ); - console.log(`${plan.feature.length} highlight card(s) + hero for v${options.version}:`); for (const job of plan.feature) { const shot = job.screenshotPath @@ -280,14 +322,14 @@ async function main() { return; } - if (existsSync(outputDir)) { - const stale = readdirSync(outputDir).filter(isOwnedCardFile); + const stale = existsSync(outputDir) + ? readdirSync(outputDir).filter(isOwnedCardFile) + : []; - if (stale.length > 0) { - console.log( - `Replacing ${stale.length} card(s) from a previous run in the same directory.` - ); - } + if (stale.length > 0) { + console.log( + `Replacing ${stale.length} card(s) from a previous run in the same directory.` + ); } const written = await renderCards(plan, options.version, outputDir); diff --git a/tools/release/highlight-cards.mjs b/tools/release/highlight-cards.mjs index 149b9152a..fde1c13f1 100644 --- a/tools/release/highlight-cards.mjs +++ b/tools/release/highlight-cards.mjs @@ -119,12 +119,23 @@ export function planHighlightCards(notes, options) { .flatMap((group) => group.notes); const highlights = ordered.filter((note) => note.highlight); + const takenSlugs = new Set(); + const feature = highlights.map((note) => { // Named after the note file, never the screenshot slug: filenames are // unique within `.changes/`, while two highlights may legitimately // point at the same manifest shot — naming cards after it would let // one silently overwrite the other. - const slug = path.basename(note.sourcePath, '.md').toLowerCase(); + const base = cardSlug(path.basename(note.sourcePath, '.md')); + // Normalizing can collapse two distinct names onto one, so uniqueness + // is re-established here rather than assumed. + let slug = base; + + for (let suffix = 2; takenSlugs.has(slug); suffix += 1) { + slug = `${base}-${suffix}`; + } + + takenSlugs.add(slug); return { slug, @@ -169,6 +180,24 @@ export function isOwnedCardFile(fileName) { ); } +/** + * Every emitted filename must satisfy isOwnedCardFile(), or a later run cannot + * reclaim the card it wrote. Note filenames are conventionally lowercase slugs + * but nothing enforces it, so normalize rather than trust: `player_new-ui.md` + * would otherwise produce a card no cleanup pass can ever remove. + * + * @param {string} noteBaseName note filename without its `.md` extension + * @returns {string} + */ +export function cardSlug(noteBaseName) { + const normalized = noteBaseName + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, ''); + + return normalized || 'note'; +} + function backgroundDefs() { return [ '', diff --git a/tools/release/highlight-cards.test.mjs b/tools/release/highlight-cards.test.mjs index c82cb2415..90e906601 100644 --- a/tools/release/highlight-cards.test.mjs +++ b/tools/release/highlight-cards.test.mjs @@ -9,6 +9,8 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; import { after, describe, it } from 'node:test'; import sharp from 'sharp'; @@ -26,6 +28,7 @@ import { } from './highlight-cards.mjs'; import { parseCardArguments, + removeStaleCards, renderCards, } from './generate-highlight-cards.mjs'; @@ -178,6 +181,32 @@ describe('planHighlightCards', () => { ); }); + it('emits only filenames the ownership predicate can reclaim', () => { + // Nothing enforces the lowercase-slug filename convention, and a card + // the cleanup pass cannot recognize is a card that lives forever. + const plan = planHighlightCards( + [ + note({ + highlight: 'New UI', + sourcePath: '.changes/Player_New-UI.md', + }), + note({ + highlight: 'Other', + sourcePath: '.changes/player.new.ui.md', + }), + ], + planOptions + ); + + assert.deepEqual( + plan.feature.map((job) => job.fileName), + ['card-player-new-ui.png', 'card-player-new-ui-2.png'] + ); + for (const job of plan.feature) { + assert.equal(isOwnedCardFile(job.fileName), true, job.fileName); + } + }); + it('claims only the files it writes', () => { for (const owned of ['card-playback-up-next.png', 'hero.png', 'hero.jpg']) { assert.equal(isOwnedCardFile(owned), true, owned); @@ -400,6 +429,23 @@ describe('renderCards', () => { } }); + it('removeStaleCards tolerates a missing directory and reports what it took', () => { + const outputDir = path.join(makeTempDir(), 'never-created'); + + assert.deepEqual(removeStaleCards(outputDir), []); + + mkdirSync(outputDir, { recursive: true }); + writeFileSync(path.join(outputDir, 'card-gone.png'), 'x'); + writeFileSync(path.join(outputDir, 'hero.jpg'), 'x'); + writeFileSync(path.join(outputDir, 'keep.txt'), 'x'); + + assert.deepEqual(removeStaleCards(outputDir).sort(), [ + 'card-gone.png', + 'hero.jpg', + ]); + assert.deepEqual(readdirSync(outputDir), ['keep.txt']); + }); + it('removes its own stale cards but leaves other files alone', async () => { const outputDir = path.join(makeTempDir(), 'cards'); @@ -424,3 +470,97 @@ describe('renderCards', () => { ]); }); }); + +describe('generate-highlight-cards CLI', () => { + const CLI = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + 'generate-highlight-cards.mjs' + ); + + function runCli(args) { + const result = spawnSync(process.execPath, [CLI, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + + return { + status: result.status ?? 1, + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + }; + } + + it('fails on an empty notes directory instead of calling it internal-only', () => { + // The realistic cause is running this step after `--consume`, which + // deleted the notes; silently reporting "internal-only" would hide it. + const emptyDir = makeTempDir(); + const result = runCli([ + '--generate', + '--version', + '0.24.0', + '--dir', + emptyDir, + '--out', + path.join(makeTempDir(), 'cards'), + ]); + + assert.equal(result.status, 1); + assert.match(result.stderr, /No notes found/); + assert.match(result.stderr, /Render cards before consuming/); + assert.doesNotMatch(result.stderr, /Internal-only/); + }); + + it('clears stale cards when a release becomes internal-only', () => { + const notesDir = makeTempDir(); + const outputDir = path.join(makeTempDir(), 'cards'); + + writeFileSync( + path.join(notesDir, 'deps-bump.md'), + '---\ntype: internal\narea: deps\n---\n\nBumped a parser.\n', + 'utf8' + ); + mkdirSync(outputDir, { recursive: true }); + writeFileSync(path.join(outputDir, 'card-old-feature.png'), 'stale'); + writeFileSync(path.join(outputDir, 'keep.txt'), 'mine'); + + const result = runCli([ + '--generate', + '--version', + '0.24.0', + '--dir', + notesDir, + '--out', + outputDir, + ]); + + assert.equal(result.status, 0); + assert.match(result.stderr, /Internal-only release/); + assert.deepEqual(readdirSync(outputDir), ['keep.txt']); + }); + + it('leaves stale cards untouched in dry-run mode', () => { + const notesDir = makeTempDir(); + const outputDir = path.join(makeTempDir(), 'cards'); + + writeFileSync( + path.join(notesDir, 'deps-bump.md'), + '---\ntype: internal\narea: deps\n---\n\nBumped a parser.\n', + 'utf8' + ); + mkdirSync(outputDir, { recursive: true }); + writeFileSync(path.join(outputDir, 'card-old-feature.png'), 'stale'); + + const result = runCli([ + '--dry-run', + '--version', + '0.24.0', + '--dir', + notesDir, + '--out', + outputDir, + ]); + + assert.equal(result.status, 0); + assert.deepEqual(readdirSync(outputDir), ['card-old-feature.png']); + }); +});