From 2d50c5847bfd2d12bddb810e2ff618c10cd44e62 Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 5 Oct 2026 22:58:30 +0900 Subject: [PATCH 1/2] Add a script checking changelog PR references Changelog entries often omit the current PR number or pin a predicted number that belongs to another PR. Add a script that finds the entries a PR introduces and fails when one lacks the PR number in its trailing references or in the links metadata of its fragment. An entry is introduced unless it existed before the PR, at the merge base or on a branch the PR merged in: an entry with the same first paragraph, or failing that, one editing an entry with the same first line, existed before. This excludes edits to historical entries that keep their first line, as well as entries carried forward through a merge, while still catching new entries. https://github.com/fedify-dev/fedify/issues/1236 Changelog: none Assisted-by: Claude Code:claude-opus-5-5 --- scripts/check_changelog_pr_refs.test.ts | 612 ++++++++++++++++++++++++ scripts/check_changelog_pr_refs.ts | 402 ++++++++++++++++ 2 files changed, 1014 insertions(+) create mode 100644 scripts/check_changelog_pr_refs.test.ts create mode 100644 scripts/check_changelog_pr_refs.ts diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts new file mode 100644 index 000000000..d40ce1914 --- /dev/null +++ b/scripts/check_changelog_pr_refs.test.ts @@ -0,0 +1,612 @@ +import { deepStrictEqual, match, strictEqual } from "node:assert/strict"; +import { describe, it } from "node:test"; +import { dirname, join } from "@std/path"; +import { + checkChangelogPrRefs, + checkFragment, + findIntroducedEntries, +} from "./check_changelog_pr_refs.ts"; + +const PATH = "changes.d/cli/foo.md"; + +function fragment( + links: Record | null, + references: string, +): string { + const frontmatter = links == null ? "" : [ + "---", + "links:", + ...Object.entries(links).map(([key, url]) => ` '${key}': ${url}`), + "---", + "", + ].join("\n"); + return `${frontmatter} - Fixed a bug where foo would bar. ${references}\n`; +} + +const issue = "https://github.com/fedify-dev/fedify/issues/123"; +const pull = (n: number) => `https://github.com/fedify-dev/fedify/pull/${n}`; + +describe("checkFragment()", () => { + it("accepts a fragment that references the current pull request", () => { + const content = fragment( + { "#123": issue, "#456": pull(456) }, + "[[#123], [#456] by John Doe]", + ); + deepStrictEqual(checkFragment(PATH, content, 456), []); + }); + + it("accepts references wrapped across lines", () => { + const content = [ + "---", + "links:", + ` '#123': ${issue}`, + ` '#456': ${pull(456)}`, + "---", + " - Fixed a bug where foo would bar, which needs a long description", + " to wrap. [[#123],", + " [#456]]", + "", + " A second paragraph without references.", + "", + ].join("\n"); + deepStrictEqual(checkFragment(PATH, content, 456), []); + }); + + it("reports a missing pull request reference", () => { + const content = fragment({ "#123": issue }, "[[#123]]"); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 2); + for (const violation of violations) strictEqual(violation.path, PATH); + match(violations[0].message, /no entry for '#456'/); + match( + violations[0].message, + /'#456': https:\/\/github\.com\/.*\/pull\/456/, + ); + match(violations[1].message, /do not include the current pull request/); + match(violations[1].message, /Add \[#456\]/); + }); + + it("reports an entry without trailing references", () => { + const content = fragment({ "#456": pull(456) }, ""); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 1); + match(violations[0].message, /no trailing references/); + }); + + it("reports a fragment without links metadata", () => { + const content = fragment(null, "[[#123], [#456]]"); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 1); + match(violations[0].message, /no entry for '#456'/); + }); + + it("reports a wrong predicted pull request number", () => { + const content = fragment( + { "#123": issue, "#455": pull(455) }, + "[[#123], [#455]]", + ); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 2); + for (const { message } of violations) { + match(message, /links to #455 instead/); + match(message, /replace it with #456/); + } + }); + + it("reports a links URL that does not point to the pull request", () => { + const content = fragment( + { + "#123": issue, + "#456": "https://github.com/fedify-dev/fedify/issues/456", + }, + "[[#123], [#456]]", + ); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 1); + match(violations[0].message, /maps '#456' to .*\/issues\/456/); + match(violations[0].message, /Change it to .*\/pull\/456/); + }); + + it("checks every entry of a fragment", () => { + const content = [ + "---", + "links:", + ` '#456': ${pull(456)}`, + "---", + " - Added foo. [[#456]]", + "", + " - Added bar. [[#123]]", + "", + ].join("\n"); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 1); + match(violations[0].message, /entry 2/); + }); + + it("checks only the given entries", () => { + const content = [ + "---", + "links:", + ` '#7': ${pull(7)}`, + "---", + " - Added foo. [[#7]]", + "", + " - Added bar. [[#123]]", + "", + ].join("\n"); + const violations = checkFragment(PATH, content, 456, new Set([1])); + strictEqual(violations.length, 2); + match(violations[0].message, /no entry for '#456'/); + match(violations[1].message, /entry 2/); + }); + + it("accepts references followed directly by a nested list", () => { + const content = [ + "---", + "links:", + ` '#456': ${pull(456)}`, + "---", + " - Added options. [[#123], [#456]]", + " - `foo` option.", + " - `bar` option.", + "", + ].join("\n"); + deepStrictEqual(checkFragment(PATH, content, 456), []); + }); + + it("hints only at pull requests the checked entries cite", () => { + const content = [ + "---", + "links:", + ` '#7': ${pull(7)}`, + "---", + " - Added foo. [[#7]]", + "", + " - Added bar. [[#123]]", + "", + ].join("\n"); + const violations = checkFragment(PATH, content, 456, new Set([1])); + strictEqual(violations.length, 2); + for (const { message } of violations) { + strictEqual(message.includes("links to #7"), false, message); + } + }); + + it("reports malformed frontmatter", () => { + const content = "---\nlinks: [\n---\n - Added foo. [[#456]]\n"; + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 1); + match(violations[0].message, /could not parse the frontmatter/); + }); +}); + +describe("findIntroducedEntries()", () => { + it("introduces every entry of a new fragment", () => { + strictEqual(findIntroducedEntries(" - Added foo. [[#456]]\n", []), "all"); + }); + + it("introduces only entries that did not exist before", () => { + const before = " - Added foo. [[#1]]\n\n - Added bar. [[#2]]\n"; + const after = " - Added bar. [[#2]]\n\n - Added baz. [[#456]]\n\n" + + " - Added foo. [[#1]]\n"; + deepStrictEqual(findIntroducedEntries(after, [before]), new Set([1])); + }); + + it("treats entries on any previous revision as existing", () => { + const after = " - Added foo. [[#1]]\n\n - Added bar. [[#2]]\n"; + deepStrictEqual( + findIntroducedEntries(after, [ + " - Added foo. [[#1]]\n", + " - Added bar. [[#2]]\n", + ]), + new Set(), + ); + }); + + it("treats entries edited after their first line as existing", () => { + const before = " - Added foo,\n and bar. [[#1]]\n"; + const after = " - Added foo,\n bar, and baz. [[#1]]\n"; + deepStrictEqual(findIntroducedEntries(after, [before]), new Set()); + }); + + it("introduces new entries sharing a first line with an existing one", () => { + const old = " - Fixed a bug.\n Foo failed. [[#1]]\n"; + const twin = " - Fixed a bug.\n Bar failed. [[#2]]\n"; + deepStrictEqual( + findIntroducedEntries(`${twin}\n${old}`, [old]), + new Set([0]), + ); + deepStrictEqual( + findIntroducedEntries(`${old}\n${twin}`, [old, old]), + new Set([1]), + ); + }); + + it("ignores rewrapped entries", () => { + const before = " - Fixed a bug where foo would bar. [[#1]]\n"; + const after = " - Fixed a bug where foo\n would bar. [[#1]]\n"; + deepStrictEqual(findIntroducedEntries(after, [before]), new Set()); + }); +}); + +describe("checkChangelogPrRefs()", () => { + const valid = fragment( + { "#123": issue, "#456": pull(456) }, + "[[#123], [#456]]", + ); + const historical = [ + "---", + "links:", + ` '#7': ${pull(7)}`, + "---", + " - Fixed a bug where foo would bar, which was fixed", + " long ago. [[#7]]", + "", + ].join("\n"); + const historicalPath = "changes.d/cli/historical.md"; + + /** Append a new top-level entry to the historical fragment. */ + const withNewEntry = (links: string, references: string) => + historical.replace("---\n -", `${links}---\n -`) + + `\n - Added baz. ${references}\n`; + + async function git(root: string, ...args: string[]): Promise { + const { code, stdout, stderr } = await new Deno.Command("git", { + args: [ + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "-c", + "commit.gpgSign=false", + "-c", + "core.hooksPath=/dev/null", + ...args, + ], + cwd: root, + stdout: "piped", + stderr: "piped", + }).output(); + const decoder = new TextDecoder(); + if (code !== 0) throw new Error(decoder.decode(stderr)); + return decoder.decode(stdout).trim(); + } + + async function commit( + root: string, + files: Record, + ): Promise { + for (const [path, content] of Object.entries(files)) { + if (content == null) { + await git(root, "rm", "--quiet", path); + } else { + await Deno.mkdir(join(root, dirname(path)), { recursive: true }); + await Deno.writeTextFile(join(root, path), content); + await git(root, "add", path); + } + } + await git(root, "commit", "--quiet", "--allow-empty", "-m", "commit"); + } + + /** + * Set up a repository whose `main` branch contains a historical fragment + * and `files`, check out a `pr` branch from it, and run `scenario` on that + * branch. + */ + async function withRepository( + scenario: (root: string) => Promise, + files: Record = {}, + ): Promise>> { + const root = await Deno.makeTempDir(); + try { + await git(root, "init", "--quiet", "--initial-branch=main"); + await commit(root, { + "changes.d/next.txt": "1.0.0\n", + [historicalPath]: historical, + ...files, + }); + await git(root, "switch", "--quiet", "-c", "pr"); + await scenario(root); + return await checkChangelogPrRefs(root, { + pullRequest: 456, + base: "main", + head: "pr", + }); + } finally { + await Deno.remove(root, { recursive: true }); + } + } + + it("passes a pull request without changelog fragments", async () => { + const result = await withRepository(async (root) => { + await commit(root, { "src/foo.ts": "export {};\n" }); + }); + deepStrictEqual(result, { fragments: [], violations: [] }); + }); + + it("checks fragments added by the pull request", async () => { + const result = await withRepository(async (root) => { + await commit(root, { + "changes.d/cli/valid.md": valid, + "changes.d/fedify/invalid.md": fragment({ "#123": issue }, "[[#123]]"), + }); + }); + deepStrictEqual(result.fragments, [ + "changes.d/cli/valid.md", + "changes.d/fedify/invalid.md", + ]); + deepStrictEqual( + [...new Set(result.violations.map((v) => v.path))], + ["changes.d/fedify/invalid.md"], + ); + }); + + it("checks later edits to fragments added by the pull request", async () => { + const result = await withRepository(async (root) => { + await commit(root, { "changes.d/cli/foo.md": valid }); + await commit(root, { + "changes.d/cli/foo.md": fragment({ "#123": issue }, "[[#123]]"), + }); + }); + deepStrictEqual(result.fragments, ["changes.d/cli/foo.md"]); + strictEqual(result.violations.length, 2); + }); + + it("follows renames of fragments added by the pull request", async () => { + const result = await withRepository(async (root) => { + await commit(root, { "changes.d/cli/foo.md": valid }); + await git(root, "mv", "changes.d/cli/foo.md", "changes.d/cli/bar.md"); + await commit(root, {}); + }); + deepStrictEqual(result, { + fragments: ["changes.d/cli/bar.md"], + violations: [], + }); + }); + + it("checks entries added to historical fragments", async () => { + const invalid = await withRepository(async (root) => { + await commit(root, { [historicalPath]: withNewEntry("", "[[#123]]") }); + }); + deepStrictEqual(invalid.fragments, [historicalPath]); + strictEqual(invalid.violations.length, 2); + match(invalid.violations[0].message, /no entry for '#456'/); + match(invalid.violations[1].message, /entry 2 \(\[#123\]\)/); + + const valid = await withRepository(async (root) => { + await commit(root, { + [historicalPath]: withNewEntry( + ` '#456': ${pull(456)}\n`, + "[[#123], [#456]]", + ), + }); + }); + deepStrictEqual(valid, { fragments: [historicalPath], violations: [] }); + }); + + it("checks later edits to entries added to historical fragments", async () => { + const result = await withRepository(async (root) => { + await commit(root, { [historicalPath]: withNewEntry("", "[[#123]]") }); + await commit(root, { + [historicalPath]: withNewEntry( + ` '#456': ${pull(456)}\n`, + "[[#123], [#456]]", + ), + }); + }); + deepStrictEqual(result, { fragments: [historicalPath], violations: [] }); + }); + + it("excludes historical fragments renamed", async () => { + const result = await withRepository(async (root) => { + await Deno.mkdir(join(root, "changes.d", "fedify")); + await git( + root, + "mv", + historicalPath, + "changes.d/fedify/historical.md", + ); + await commit(root, {}); + }); + deepStrictEqual(result, { fragments: [], violations: [] }); + }); + + it("excludes historical entries edited after their first line", async () => { + for ( + const edited of [ + historical.replace("long ago", "a while ago"), + `${historical}\n - Also fixed baz.\n`, + historical.replace( + "which was fixed\n long", + "which was\n fixed long", + ), + ] + ) { + const result = await withRepository(async (root) => { + await commit(root, { [historicalPath]: edited }); + }); + deepStrictEqual(result, { fragments: [], violations: [] }); + } + }); + + it("checks historical entries whose first line is edited", async () => { + const result = await withRepository(async (root) => { + await commit(root, { + [historicalPath]: historical.replace("foo would bar", "foo would baz"), + }); + }); + deepStrictEqual(result.fragments, [historicalPath]); + strictEqual(result.violations.length, 2); + match(result.violations[1].message, /entry \(\[#7\]\)/); + }); + + it("checks new entries replacing historical ones", async () => { + const result = await withRepository(async (root) => { + await commit(root, { + [historicalPath]: historical.replace( + /---\n -[^]*$/, + "---\n - Fixed a crash in the inbox handler. [[#9]]\n", + ), + }); + }); + deepStrictEqual(result.fragments, [historicalPath]); + strictEqual(result.violations.length, 2); + match(result.violations[0].message, /no entry for '#456'/); + match(result.violations[1].message, /entry \(\[#9\]\)/); + + const path = "changes.d/fedify/method.md"; + const similar = await withRepository(async (root) => { + await commit(root, { + [path]: " - Added `Context.bar()` method. [[#123]]\n", + }); + }, { [path]: " - Added `Federation.foo()` method. [[#7]]\n" }); + deepStrictEqual(similar.fragments, [path]); + strictEqual(similar.violations.length, 2); + match(similar.violations[1].message, /entry \(\[#123\]\)/); + }); + + it("checks new entries sharing the first paragraph of a historical entry", async () => { + const path = "changes.d/fedify/apis.md"; + const entry = (references: string, api: string) => + ` - Added new APIs. ${references}\n\n - \`${api}()\`\n`; + const result = await withRepository(async (root) => { + await commit(root, { + [path]: `${entry("[[#123]]", "bar")}\n${entry("[[#7]]", "foo")}`, + }); + }, { [path]: entry("[[#7]]", "foo") }); + deepStrictEqual(result.fragments, [path]); + strictEqual(result.violations.length, 2); + match(result.violations[1].message, /entry 1 \(\[#123\]\)/); + }); + + it("excludes historical entries moved within a fragment", async () => { + const path = "changes.d/cli/two.md"; + const result = await withRepository(async (root) => { + await commit(root, { + [path]: " - Added bar. [[#2]]\n\n - Added foo. [[#1]]\n", + }); + }, { [path]: " - Added foo. [[#1]]\n\n - Added bar. [[#2]]\n" }); + deepStrictEqual(result, { fragments: [], violations: [] }); + }); + + it("checks new entries sharing the first line of a historical entry", async () => { + const twin = " - Fixed a bug where foo would bar, which was fixed\n" + + " just now. [[#123]]\n"; + const appended = await withRepository(async (root) => { + await commit(root, { [historicalPath]: `${historical}\n${twin}` }); + }); + deepStrictEqual(appended.fragments, [historicalPath]); + strictEqual(appended.violations.length, 2); + match(appended.violations[0].message, /no entry for '#456'/); + match(appended.violations[1].message, /entry 2 \(\[#123\]\)/); + + const prepended = await withRepository(async (root) => { + await commit(root, { + [historicalPath]: historical.replace("---\n -", `---\n${twin}\n -`), + }); + }); + deepStrictEqual(prepended.fragments, [historicalPath]); + strictEqual(prepended.violations.length, 2); + match(prepended.violations[0].message, /no entry for '#456'/); + match(prepended.violations[1].message, /entry 1 \(\[#123\]\)/); + }); + + it("excludes entries carried forward through a merge", async () => { + const result = await withRepository(async (root) => { + await git(root, "switch", "--quiet", "-c", "maintenance", "main"); + await commit(root, { + "changes.d/cli/backport.md": historical, + [historicalPath]: withNewEntry("", "[[#8]]"), + }); + await git(root, "switch", "--quiet", "pr"); + await git( + root, + "merge", + "--quiet", + "--no-ff", + "-m", + "merge", + "maintenance", + ); + await commit(root, { "changes.d/cli/foo.md": valid }); + }); + deepStrictEqual(result, { + fragments: ["changes.d/cli/foo.md"], + violations: [], + }); + }); + + it("follows introduced entries moved by a merge", async () => { + const result = await withRepository(async (root) => { + await git(root, "switch", "--quiet", "-c", "maintenance", "main"); + await commit(root, { + [historicalPath]: historical.replace( + "---\n -", + "---\n - Added qux. [[#8]]\n\n -", + ), + }); + await git(root, "switch", "--quiet", "pr"); + await commit(root, { + [historicalPath]: `${historical}\n - Added baz. [[#123], [#456]]\n`, + }); + await git( + root, + "merge", + "--quiet", + "--no-ff", + "-m", + "merge", + "maintenance", + ); + const merged = await Deno.readTextFile(join(root, historicalPath)); + await commit(root, { + [historicalPath]: merged.replace( + "\n---\n", + `\n '#456': ${pull(456)}\n---\n`, + ), + }); + }); + deepStrictEqual(result, { fragments: [historicalPath], violations: [] }); + }); + + it("excludes entries merged into fragments added by the pull request", async () => { + const path = "changes.d/cli/foo.md"; + const result = await withRepository(async (root) => { + await git(root, "switch", "--quiet", "-c", "maintenance", "main"); + await commit(root, { [path]: " - Added qux. [[#8]]\n" }); + await git(root, "switch", "--quiet", "pr"); + await commit(root, { [path]: valid }); + await git( + root, + "merge", + "--quiet", + "--no-ff", + "--no-commit", + "--strategy=ours", + "maintenance", + ); + await commit(root, { [path]: `${valid}\n - Added qux. [[#8]]\n` }); + }); + deepStrictEqual(result, { fragments: [path], violations: [] }); + }); + + it("ignores fragments deleted by a merge", async () => { + const result = await withRepository(async (root) => { + await git(root, "switch", "--quiet", "-c", "maintenance", "main"); + await commit(root, { [historicalPath]: null }); + await git(root, "switch", "--quiet", "pr"); + await commit(root, { [historicalPath]: withNewEntry("", "[[#123]]") }); + await git( + root, + "merge", + "--quiet", + "--no-ff", + "--no-commit", + "--strategy=ours", + "maintenance", + ); + await commit(root, { [historicalPath]: null }); + }); + deepStrictEqual(result, { fragments: [], violations: [] }); + }); +}); diff --git a/scripts/check_changelog_pr_refs.ts b/scripts/check_changelog_pr_refs.ts new file mode 100644 index 000000000..b2b59e047 --- /dev/null +++ b/scripts/check_changelog_pr_refs.ts @@ -0,0 +1,402 @@ +/** + * This script checks that every changelog entry introduced by a pull request + * references that pull request: the trailing references of the entry have to + * include the pull request number, and the `links` frontmatter of its fragment + * has to map that number to `https://github.com/fedify-dev/fedify/pull/`. + * + * An entry is introduced by the pull request unless it existed before the + * pull request, that is, at the merge base of the base and head revisions or + * on the branches that the pull request merged in (see + * {@link findIntroducedEntries}). A pull request without any changelog + * fragment passes. + * + * Usage: + * + * ~~~~ bash + * deno run --allow-read --allow-run=git scripts/check_changelog_pr_refs.ts \ + * --pr --base [--head ] + * ~~~~ + */ +import { parseArgs } from "node:util"; +import { dirname, fromFileUrl, resolve } from "@std/path"; +import { parse as parseYaml } from "@std/yaml"; + +/** The directory that holds changelog fragments, relative to the root. */ +export const FRAGMENTS_DIRECTORY = "changes.d"; + +/** The URL prefix of pull requests in the upstream repository. */ +export const PULL_REQUEST_URL_PREFIX = + "https://github.com/fedify-dev/fedify/pull/"; + +/** A problem found in a changelog fragment. */ +export interface Violation { + /** The fragment path, relative to the project root. */ + readonly path: string; + /** A human-readable description of the problem and how to correct it. */ + readonly message: string; +} + +function isFragmentPath(path: string): boolean { + return path.startsWith(`${FRAGMENTS_DIRECTORY}/`) && path.endsWith(".md"); +} + +/** + * Determine which entries of a fragment a pull request introduces. An entry + * existed before if an entry with the same first paragraph did, or failing + * that, if it edits a previous entry with the same first line, so edits to + * historical entries that keep their first line are not introduced. + * + * @param content The fragment content at the head of the pull request. + * @param previous The contents the fragment had before the pull request: at + * the merge base and on the branches the pull request merged + * in. Empty if the fragment did not exist before. + * @returns `"all"` if the fragment did not exist before; otherwise, the + * 0-based indices of the top-level list items it introduces. + */ +export function findIntroducedEntries( + content: string, + previous: readonly string[], +): "all" | Set { + if (previous.length < 1) return "all"; + const paragraphs = new Set(); + const before = previous.flatMap(extractEntries).filter((entry) => { + // The same entry usually appears in several previous revisions: + if (paragraphs.has(entry.paragraph)) return false; + paragraphs.add(entry.paragraph); + return true; + }); + const after = extractEntries(content); + const matched = new Set(); + const existing = new Set(); + for (const key of ["paragraph", "firstLine"] as const) { + after.forEach((entry, index) => { + if (existing.has(index)) return; + const origin = before.findIndex((old, i) => + !matched.has(i) && old[key] === entry[key] + ); + if (origin < 0) return; + matched.add(origin); + existing.add(index); + }); + } + return new Set([...after.keys()].filter((index) => !existing.has(index))); +} + +/** A top-level list item of a changelog fragment. */ +interface Entry { + /** The line holding the list marker, with its whitespace collapsed. */ + readonly firstLine: string; + /** The first paragraph of the item, with its whitespace collapsed. */ + readonly paragraph: string; +} + +/** The pieces of a changelog fragment relevant to this check. */ +interface ParsedFragment { + readonly links: Readonly> | null; + readonly entries: readonly Entry[]; +} + +const FRONTMATTER_PATTERN = /^---\n([\s\S]*?)\n---(?:\n|$)/; + +function normalizeNewlines(content: string): string { + return content.replace(/\r\n?/g, "\n"); +} + +function parseFragment(content: string): ParsedFragment | string { + const text = normalizeNewlines(content); + const frontmatter = FRONTMATTER_PATTERN.exec(text); + let links: Record | null = null; + if (frontmatter != null) { + let metadata: unknown; + try { + metadata = parseYaml(frontmatter[1]); + } catch (error) { + return `could not parse the frontmatter: ${ + error instanceof Error ? error.message : String(error) + }`; + } + if (metadata != null && typeof metadata === "object") { + const value = (metadata as Record).links; + if (value != null && typeof value === "object") { + links = value as Record; + } + } + } + return { links, entries: extractEntries(text) }; +} + +/** Return the top-level list items of a fragment, skipping frontmatter. */ +function extractEntries(content: string): Entry[] { + const text = normalizeNewlines(content); + const frontmatter = FRONTMATTER_PATTERN.exec(text); + const body = frontmatter == null ? text : text.slice(frontmatter[0].length); + const items: string[][] = []; + for (const line of body.split("\n")) { + if (/^ {0,3}[-*+] /.test(line)) items.push([line]); + else items.at(-1)?.push(line); + } + return items.map((lines) => { + const trimmed = lines.map((line) => line.trim()); + // The first paragraph ends at a blank line or where a nested list starts: + const end = trimmed.findIndex((line, index) => + index > 0 && (line === "" || /^(?:[-*+]|\d+[.)]) /.test(line)) + ); + const collapse = (text: string) => text.replace(/\s+/g, " ").trim(); + return { + firstLine: collapse(trimmed[0]), + paragraph: collapse( + trimmed.slice(0, end < 0 ? undefined : end).join(" ") + .replace(/^[-*+] +/, ""), + ), + }; + }); +} + +/** + * Matches the trailing reference group of a paragraph, such as + * `[[#123], [#456] by John Doe]`. + */ +const TRAILING_REFERENCES_PATTERN = + /\[(\[#\d+\](?:\s*,\s*\[#\d+\])*)(?:\s+by\s+[^\[\]]+)?\]\s*$/; + +function extractTrailingReferences(paragraph: string): number[] | null { + const match = TRAILING_REFERENCES_PATTERN.exec(paragraph); + if (match == null) return null; + return Array.from(match[1].matchAll(/\[#(\d+)\]/g), (m) => Number(m[1])); +} + +/** Return the numbers of the pull requests that `links` points to. */ +function findLinkedPullRequests( + links: Readonly>, +): number[] { + const numbers: number[] = []; + for (const [key, url] of Object.entries(links)) { + const keyMatch = /^#(\d+)$/.exec(key); + if (keyMatch == null || typeof url !== "string") continue; + if (url === `${PULL_REQUEST_URL_PREFIX}${keyMatch[1]}`) { + numbers.push(Number(keyMatch[1])); + } + } + return numbers; +} + +/** + * Check that the introduced entries of a changelog fragment reference the + * current pull request. + * + * @param path The fragment path, used in the returned violations. + * @param content The fragment content. + * @param pullRequest The number of the current pull request. + * @param entries The entries to check: `"all"`, or the 0-based indices of the + * introduced top-level list items as returned by + * {@link findIntroducedEntries}. + * @returns The problems found, or an empty array if there are none. + */ +export function checkFragment( + path: string, + content: string, + pullRequest: number, + entries: "all" | ReadonlySet = "all", +): Violation[] { + const parsed = parseFragment(content); + if (typeof parsed === "string") return [{ path, message: parsed }]; + const selected = parsed.entries + .map((entry, index) => ({ entry, index })) + .filter(({ index }) => entries === "all" || entries.has(index)); + + const ref = `#${pullRequest}`; + const expectedUrl = `${PULL_REQUEST_URL_PREFIX}${pullRequest}`; + // Other entries' pull requests are not wrong predictions: + const cited = new Set( + selected.flatMap(({ entry }) => + extractTrailingReferences(entry.paragraph) ?? [] + ), + ); + const otherPullRequests = parsed.links == null + ? [] + : findLinkedPullRequests(parsed.links) + .filter((number) => number !== pullRequest && cited.has(number)); + const predictionHint = otherPullRequests.length > 0 + ? ` It links to ${ + otherPullRequests.map((n) => `#${n}`).join(", ") + } instead; if that number was predicted, replace it with ${ref} in both ` + + "the links metadata and the trailing references." + : ""; + + const violations: Violation[] = []; + const linked = parsed.links?.[ref]; + if (linked == null) { + violations.push({ + path, + message: `the links metadata has no entry for '${ref}'. Add ` + + `'${ref}': ${expectedUrl} under links.${predictionHint}`, + }); + } else if (linked !== expectedUrl) { + violations.push({ + path, + message: `the links metadata maps '${ref}' to ${ + typeof linked === "string" ? linked : JSON.stringify(linked) + }. Change it to ${expectedUrl}.`, + }); + } + + if (selected.length < 1) { + violations.push({ path, message: "the fragment has no list entry." }); + } + for (const { entry: { paragraph }, index } of selected) { + const entry = parsed.entries.length > 1 ? `entry ${index + 1}` : "entry"; + const references = extractTrailingReferences(paragraph); + if (references == null) { + violations.push({ + path, + message: `the ${entry} has no trailing references. End its first ` + + `paragraph with the accepted issue and the pull request, e.g., ` + + `[[#123], [${ref}]].`, + }); + } else if (!references.includes(pullRequest)) { + violations.push({ + path, + message: + `the trailing references of the ${entry} (${ + references.map((n) => `[#${n}]`).join(", ") + }) do not include the current pull request. Add [${ref}] to ` + + `them.${predictionHint}`, + }); + } + } + return violations; +} + +async function git(projectRoot: string, args: string[]): Promise { + const { code, stdout, stderr } = await new Deno.Command("git", { + args, + cwd: projectRoot, + stdout: "piped", + stderr: "piped", + }).output(); + const decoder = new TextDecoder(); + if (code !== 0) { + throw new Error( + `git ${args.join(" ")} failed:\n${decoder.decode(stderr)}`, + ); + } + return decoder.decode(stdout); +} + +/** Options for {@link checkChangelogPrRefs}. */ +export interface CheckOptions { + /** The number of the current pull request. */ + readonly pullRequest: number; + /** The base revision of the pull request. */ + readonly base: string; + /** The head revision of the pull request. */ + readonly head: string; +} + +/** + * Check the changelog entries that a pull request introduces. + * + * @param projectRoot The root of the Git working tree. + * @param options The pull request to check. + * @returns The paths of the fragments holding introduced entries and the + * problems found in them. + */ +export async function checkChangelogPrRefs( + projectRoot: string, + options: CheckOptions, +): Promise<{ fragments: string[]; violations: Violation[] }> { + const run = (...args: string[]) => git(projectRoot, args); + const readFile = async (revision: string, path: string) => { + try { + return await run("show", `${revision}:${path}`); + } catch { + return null; // The file does not exist at the revision. + } + }; + const mergeBase = (await run("merge-base", options.base, options.head)) + .trim(); + // The other parents of the merge commits on the pull request branch, such as + // the base branch merged in to resolve conflicts: + const mergedIn = (await run( + "rev-list", + "--first-parent", + "--merges", + "--parents", + `${mergeBase}..${options.head}`, + )).split("\n").flatMap((line) => line.split(" ").slice(2)); + const diff = await run( + "-c", + "core.quotePath=false", + "diff", + "--name-status", + "--find-renames", + mergeBase, + options.head, + "--", + FRAGMENTS_DIRECTORY, + ); + const fragments: string[] = []; + const violations: Violation[] = []; + for (const line of diff.split("\n")) { + // Each line is a status followed by the path, or by the old and new paths + // of a rename: + const [status, ...paths] = line.split("\t"); + const path = paths.at(-1); + if (status.startsWith("D") || path == null || !isFragmentPath(path)) { + continue; + } + const previous: string[] = []; + for (const revision of [mergeBase, ...mergedIn]) { + for (const oldPath of new Set(paths)) { + const content = await readFile(revision, oldPath); + if (content != null) previous.push(content); + } + } + const content = await run("show", `${options.head}:${path}`); + const entries = findIntroducedEntries(content, previous); + if (entries !== "all" && entries.size < 1) continue; + fragments.push(path); + violations.push( + ...checkFragment(path, content, options.pullRequest, entries), + ); + } + return { fragments, violations }; +} + +if (import.meta.main) { + const { values } = parseArgs({ + args: Deno.args, + options: { + pr: { type: "string" }, + base: { type: "string" }, + head: { type: "string", default: "HEAD" }, + }, + }); + const pullRequest = Number(values.pr); + if (!Number.isSafeInteger(pullRequest) || pullRequest < 1 || !values.base) { + console.error( + "Usage: check_changelog_pr_refs.ts --pr --base " + + "[--head ]", + ); + Deno.exit(2); + } + const projectRoot = resolve(dirname(fromFileUrl(import.meta.url)), ".."); + const { fragments, violations } = await checkChangelogPrRefs(projectRoot, { + pullRequest, + base: values.base, + head: values.head, + }); + if (fragments.length < 1) { + console.log("This pull request introduces no changelog entries."); + } + for (const { path, message } of violations) { + console.error(`${path}: ${message}`); + } + if (violations.length > 0) Deno.exit(1); + if (fragments.length > 0) { + console.log( + `All changelog entries introduced in ${fragments.length} fragment(s) ` + + `reference #${pullRequest}.`, + ); + } +} From ddbce7cd0a7e5d571ec1407c1265fddb316a08db Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 5 Oct 2026 22:59:19 +0900 Subject: [PATCH 2/2] Run the changelog PR reference check in CI Add a mise task for the script and run it in the lint job of pull requests, fetching the full history so that the script can find the merge base and the branches the pull request merged in. https://github.com/fedify-dev/fedify/issues/1236 Changelog: none Assisted-by: Claude Code:claude-opus-5-5 --- .github/workflows/main.yaml | 11 +++++++++++ mise.toml | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 6e6a9f149..8ef0849ab 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -253,8 +253,19 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 + with: + # '0' as a string: the number 0 is falsy in expressions + fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} - uses: ./.github/actions/setup-mise - run: mise run check + - if: github.event_name == 'pull_request' && github.repository == 'fedify-dev/fedify' + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: >- + mise run --skip-deps check:changelog-pr-refs -- + --pr "$PR_NUMBER" --base "$BASE_SHA" --head "$HEAD_SHA" release-test: runs-on: ubuntu-latest diff --git a/mise.toml b/mise.toml index 49dd9b531..7834bab11 100644 --- a/mise.toml +++ b/mise.toml @@ -230,6 +230,10 @@ deno run --allow-read --allow-write scripts/check_versions.ts ...$fix_args ...$s description = "Ensure @fedify/fixture is only used in **/*.test.ts files" run = "deno run --allow-read scripts/check_fixture_usage.ts" +[tasks."check:changelog-pr-refs"] +description = "Check that changelog entries added by a pull request reference it" +run = "deno run --allow-read --allow-run=git scripts/check_changelog_pr_refs.ts" + [tasks."check:workspace-protocol"] description = "Check for invalid workspace: specifiers without version (*, ^, ~)" run = "deno run --allow-read scripts/check_workspace_protocol.ts"