From 9d937091f2912de74e6034c2ea1b0843dae8e9da Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 5 Oct 2026 22:58:30 +0900 Subject: [PATCH 1/9] 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 78640c88a32f81bb28feafabd87210d2dec63006 Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 5 Oct 2026 22:59:19 +0900 Subject: [PATCH 2/9] 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 bff335cef..d66dd36c6 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -276,8 +276,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 b64cc874b..b151cf074 100644 --- a/mise.toml +++ b/mise.toml @@ -258,6 +258,10 @@ depends = ["deno:cache"] 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"] depends = ["deno:cache"] description = "Check for invalid workspace: specifiers without version (*, ^, ~)" From 3ea4027a93e881de1bc0d1ea21131c452958a013 Mon Sep 17 00:00:00 2001 From: co2plant Date: Sat, 10 Oct 2026 15:36:43 +0900 Subject: [PATCH 3/9] Tell missing files from other Git errors The check read previous fragments with `git show` and treated any failure as a missing file, so an operational failure could make it treat historical entries as introduced. Check with `git ls-tree` whether the path exists first, and rethrow other Git errors, rather than matching error messages, which Git can localize. Report such errors as a one-line message with exit code 2 instead of a stack trace. https://github.com/fedify-dev/fedify/pull/1247#discussion_r4185591936 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 | 44 +++++++++++++++++++++++-- scripts/check_changelog_pr_refs.ts | 29 ++++++++++------ 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts index d40ce1914..3bd746eb7 100644 --- a/scripts/check_changelog_pr_refs.test.ts +++ b/scripts/check_changelog_pr_refs.test.ts @@ -1,6 +1,11 @@ -import { deepStrictEqual, match, strictEqual } from "node:assert/strict"; +import { + deepStrictEqual, + match, + rejects, + strictEqual, +} from "node:assert/strict"; import { describe, it } from "node:test"; -import { dirname, join } from "@std/path"; +import { dirname, fromFileUrl, join } from "@std/path"; import { checkChangelogPrRefs, checkFragment, @@ -590,6 +595,41 @@ describe("checkChangelogPrRefs()", () => { deepStrictEqual(result, { fragments: [path], violations: [] }); }); + it("reports errors without a stack trace", async () => { + const { code, stderr } = await new Deno.Command(Deno.execPath(), { + args: [ + "run", + "--allow-read", + "--allow-run=git", + fromFileUrl(new URL("./check_changelog_pr_refs.ts", import.meta.url)), + "--pr", + "1", + "--base", + "no-such-revision", + ], + stdout: "null", + stderr: "piped", + }).output(); + const message = new TextDecoder().decode(stderr); + strictEqual(code, 2); + match(message, /^error: git merge-base no-such-revision HEAD failed/); + strictEqual(/^\s+at /m.test(message), false, message); + }); + + it("propagates Git errors other than missing files", async () => { + await rejects( + withRepository(async (root) => { + await commit(root, { [historicalPath]: withNewEntry("", "[[#123]]") }); + // Lose the historical fragment's content at the merge base: + const blob = await git(root, "rev-parse", `main:${historicalPath}`); + await Deno.remove( + join(root, ".git", "objects", blob.slice(0, 2), blob.slice(2)), + ); + }), + /git show .* failed/, + ); + }); + it("ignores fragments deleted by a merge", async () => { const result = await withRepository(async (root) => { await git(root, "switch", "--quiet", "-c", "maintenance", "main"); diff --git a/scripts/check_changelog_pr_refs.ts b/scripts/check_changelog_pr_refs.ts index b2b59e047..28f8e1bac 100644 --- a/scripts/check_changelog_pr_refs.ts +++ b/scripts/check_changelog_pr_refs.ts @@ -307,11 +307,11 @@ export async function checkChangelogPrRefs( ): 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. - } + // Listing the path first tells a missing file from other Git errors: + const listed = await run("ls-tree", "--name-only", revision, "--", path); + return listed.trim() === "" + ? null + : await run("show", `${revision}:${path}`); }; const mergeBase = (await run("merge-base", options.base, options.head)) .trim(); @@ -381,11 +381,20 @@ if (import.meta.main) { Deno.exit(2); } const projectRoot = resolve(dirname(fromFileUrl(import.meta.url)), ".."); - const { fragments, violations } = await checkChangelogPrRefs(projectRoot, { - pullRequest, - base: values.base, - head: values.head, - }); + let result: Awaited>; + try { + result = await checkChangelogPrRefs(projectRoot, { + pullRequest, + base: values.base, + head: values.head, + }); + } catch (error) { + console.error( + `error: ${error instanceof Error ? error.message : String(error)}`, + ); + Deno.exit(2); + } + const { fragments, violations } = result; if (fragments.length < 1) { console.log("This pull request introduces no changelog entries."); } From 1cf64493c682cbe20770e0370fc954cddb72adb3 Mon Sep 17 00:00:00 2001 From: co2plant Date: Sat, 10 Oct 2026 15:36:56 +0900 Subject: [PATCH 4/9] Keep edited historical entries excluded Changing only the first line of a historical entry, for example to fix a typo, made the check require the current PR number for it. Also treat an entry as historical when it has the same non-empty trailing references as a previous entry in the same fragment, regardless of their order. The first line is compared before the references, so a new entry that copies the references of an entry edited in the same PR is still checked. https://github.com/fedify-dev/fedify/pull/1247#discussion_r4192214545 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 | 63 +++++++++++++++++++++++-- scripts/check_changelog_pr_refs.ts | 25 ++++++---- 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts index 3bd746eb7..c445a433f 100644 --- a/scripts/check_changelog_pr_refs.test.ts +++ b/scripts/check_changelog_pr_refs.test.ts @@ -227,6 +227,55 @@ describe("findIntroducedEntries()", () => { ); }); + it("treats entries keeping their trailing references as existing", () => { + deepStrictEqual( + findIntroducedEntries(" - Fixed a typo. [[#7]]\n", [ + " - Fixed a bug. [[#7]]\n", + ]), + new Set(), + ); + }); + + it("ignores the order of trailing references", () => { + deepStrictEqual( + findIntroducedEntries(" - Fixed a typo. [[#7], [#5]]\n", [ + " - Fixed a bug. [[#5], [#7]]\n", + ]), + new Set(), + ); + }); + + it("pairs edited entries by their first line before their references", () => { + // A new entry copying the trailing references of an entry edited in the + // same pull request must not take the edited entry's place: + deepStrictEqual( + findIntroducedEntries( + " - Added baz. [[#5], [#7]]\n\n" + + " - Fixed a bug in foo.\n Details. [[#5], [#7]]\n", + [" - Fixed a bug in foo.\n Detail. [[#5], [#7]]\n"], + ), + new Set([0]), + ); + }); + + it("reports one of the entries sharing references with a new one", () => { + // Entries sharing their references are paired in order, so when edited + // entries and a new one share them, one of them is still left unpaired: + const introduced = findIntroducedEntries( + " - Changed foo. [[#5], [#7]]\n\n - Added baz. [[#5], [#7]]\n\n" + + " - Changed bar. [[#5], [#7]]\n", + [" - Fixed foo. [[#5], [#7]]\n\n - Fixed bar. [[#5], [#7]]\n"], + ); + strictEqual(introduced instanceof Set && introduced.size, 1); + }); + + it("does not pair entries without trailing references", () => { + deepStrictEqual( + findIntroducedEntries(" - Fixed bar.\n", [" - Fixed foo.\n"]), + new Set([0]), + ); + }); + 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"; @@ -434,15 +483,21 @@ describe("checkChangelogPrRefs()", () => { } }); - it("checks historical entries whose first line is edited", async () => { + it("excludes 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\]\)/); + deepStrictEqual(result, { fragments: [], violations: [] }); + + const path = "changes.d/cli/one-line.md"; + const oneLine = `---\nlinks:\n '#7': ${pull(7)}\n---\n` + + " - Fixed a bug. [[#7]]\n"; + const typo = await withRepository(async (root) => { + await commit(root, { [path]: oneLine.replace("a bug", "a typo") }); + }, { [path]: oneLine }); + deepStrictEqual(typo, { fragments: [], violations: [] }); }); it("checks new entries replacing historical ones", async () => { diff --git a/scripts/check_changelog_pr_refs.ts b/scripts/check_changelog_pr_refs.ts index 28f8e1bac..2a86c9bc6 100644 --- a/scripts/check_changelog_pr_refs.ts +++ b/scripts/check_changelog_pr_refs.ts @@ -43,8 +43,10 @@ function isFragmentPath(path: string): boolean { /** * 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. + * that, if it edits a previous entry with the same first line or the same + * trailing references. So edits to historical entries are not introduced, + * while an entry citing the current pull request or a mispredicted number + * is. * * @param content The fragment content at the head of the pull request. * @param previous The contents the fragment had before the pull request: at @@ -68,9 +70,9 @@ export function findIntroducedEntries( const after = extractEntries(content); const matched = new Set(); const existing = new Set(); - for (const key of ["paragraph", "firstLine"] as const) { + for (const key of ["paragraph", "firstLine", "references"] as const) { after.forEach((entry, index) => { - if (existing.has(index)) return; + if (existing.has(index) || entry[key] == null) return; const origin = before.findIndex((old, i) => !matched.has(i) && old[key] === entry[key] ); @@ -88,6 +90,8 @@ interface Entry { readonly firstLine: string; /** The first paragraph of the item, with its whitespace collapsed. */ readonly paragraph: string; + /** The sorted trailing references of the item, such as `#5 #7`, if any. */ + readonly references: string | null; } /** The pieces of a changelog fragment relevant to this check. */ @@ -142,12 +146,17 @@ function extractEntries(content: string): Entry[] { index > 0 && (line === "" || /^(?:[-*+]|\d+[.)]) /.test(line)) ); const collapse = (text: string) => text.replace(/\s+/g, " ").trim(); + const paragraph = collapse( + trimmed.slice(0, end < 0 ? undefined : end).join(" ") + .replace(/^[-*+] +/, ""), + ); + const references = extractTrailingReferences(paragraph); return { firstLine: collapse(trimmed[0]), - paragraph: collapse( - trimmed.slice(0, end < 0 ? undefined : end).join(" ") - .replace(/^[-*+] +/, ""), - ), + paragraph, + references: references == null || references.length < 1 + ? null + : references.sort((a, b) => a - b).map((n) => `#${n}`).join(" "), }; }); } From 89daa98c7538fd60f4ff48e10e2d8e201531ea49 Mon Sep 17 00:00:00 2001 From: co2plant Date: Sat, 10 Oct 2026 15:36:58 +0900 Subject: [PATCH 5/9] Exclude entries restored by sacho carry `sacho release` consumes fragments into a released section, so an entry that `sacho carry` restores after the release tag is merged lives in a fragment that exists in neither the merge base nor the merged release, and the check required the current PR number for it. Also compare entries with CHANGES.md at the merge base and on the merged branches: an entry whose first paragraph matches a changelog entry with trailing references is historical. A carried-from-.md fragment is also compared with that version's section by first line and references, so carried entries edited afterward stay excluded. CHANGES.md at the head is not used, since its unreleased section holds the PR's own entries. https://github.com/fedify-dev/fedify/pull/1247#discussion_r4192214558 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 | 103 ++++++++++++++++++++ scripts/check_changelog_pr_refs.ts | 119 ++++++++++++++++++------ 2 files changed, 192 insertions(+), 30 deletions(-) diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts index c445a433f..6e37f7db5 100644 --- a/scripts/check_changelog_pr_refs.test.ts +++ b/scripts/check_changelog_pr_refs.test.ts @@ -236,6 +236,27 @@ describe("findIntroducedEntries()", () => { ); }); + it("treats entries in previous changelogs as existing", () => { + const changelog = "Version 1.0.1\n-------------\n\n" + + " - Fixed a bug. [[#5], [#7]]\n\n - Fixed foo. [[#8]]\n"; + deepStrictEqual( + findIntroducedEntries(" - Fixed a bug.\n [[#5], [#7]]\n", [], [ + changelog, + ]), + new Set(), + ); + // Only the same first paragraph counts, since changelog entries often + // share their trailing references: + deepStrictEqual( + findIntroducedEntries( + " - Fixed a bug. [[#5], [#7]]\n\n - Fixed bar. [[#8]]\n", + [], + [changelog], + ), + new Set([1]), + ); + }); + it("ignores the order of trailing references", () => { deepStrictEqual( findIntroducedEntries(" - Fixed a typo. [[#7], [#5]]\n", [ @@ -269,6 +290,15 @@ describe("findIntroducedEntries()", () => { strictEqual(introduced instanceof Set && introduced.size, 1); }); + it("does not treat changelog entries without references as existing", () => { + deepStrictEqual( + findIntroducedEntries(" - Updated dependencies.\n", [], [ + "Version 1.0.0\n-------------\n\n - Updated dependencies.\n", + ]), + "all", + ); + }); + it("does not pair entries without trailing references", () => { deepStrictEqual( findIntroducedEntries(" - Fixed bar.\n", [" - Fixed foo.\n"]), @@ -650,6 +680,79 @@ describe("checkChangelogPrRefs()", () => { deepStrictEqual(result, { fragments: [path], violations: [] }); }); + it("excludes entries restored by sacho carry", async () => { + const links = [ + "---", + "links:", + " '#5': https://github.com/fedify-dev/fedify/issues/5", + ` '#7': ${pull(7)}`, + "---", + "", + ].join("\n"); + const released = [ + "Changelog", + "=========", + "", + "Version 1.0.1", + "-------------", + "", + "Released on October 2, 2026.", + "", + " - Fixed a bug. [[#5], [#7]]", + "", + "[#5]: https://github.com/fedify-dev/fedify/issues/5", + `[#7]: ${pull(7)}`, + "", + ].join("\n"); + // `sacho carry` rewraps the entry it restores: + const carried = `${links} - Fixed a bug.\n [[#5], [#7]]\n`; + const carriedPath = "changes.d/carried-from-1.0.1.md"; + const check = (content: string) => + withRepository(async (root) => { + await git(root, "switch", "--quiet", "-c", "maintenance", "main"); + await commit(root, { + "changes.d/cli/fix-foo.md": + `${links} - Fixed a bug. [[#5], [#7]]\n`, + }); + // `sacho release` consumes the fragment into a released section: + await commit(root, { + "changes.d/cli/fix-foo.md": null, + "CHANGES.md": released, + }); + await git(root, "tag", "1.0.1"); + await git(root, "switch", "--quiet", "pr"); + await git(root, "merge", "--quiet", "--no-ff", "-m", "m", "1.0.1"); + await commit(root, { [carriedPath]: content }); + }); + + deepStrictEqual(await check(carried), { fragments: [], violations: [] }); + // Sacho suggests editing carried fragments as the branch needs: + deepStrictEqual(await check(carried.replace("a bug", "a typo")), { + fragments: [], + violations: [], + }); + const extended = await check(`${carried}\n - Added baz. [[#123]]\n`); + deepStrictEqual(extended.fragments, [carriedPath]); + strictEqual(extended.violations.length, 2); + match(extended.violations[1].message, /entry 2 \(\[#123\]\)/); + }); + + it("does not treat the unreleased section of the changelog as history", async () => { + // Sacho materializes the fragments, including new ones, in the changelog, + // whatever its unreleased heading says: + for (const heading of ["To be released.", "Unreleased."]) { + const result = await withRepository(async (root) => { + await commit(root, { + "changes.d/cli/baz.md": " - Added baz. [[#123]]\n", + "CHANGES.md": `Version 1.1.0\n-------------\n\n${heading}\n\n` + + " - Added baz. [[#123]]\n", + }); + }); + deepStrictEqual(result.fragments, ["changes.d/cli/baz.md"]); + strictEqual(result.violations.length, 2); + } + }); + it("reports errors without a stack trace", async () => { const { code, stderr } = await new Deno.Command(Deno.execPath(), { args: [ diff --git a/scripts/check_changelog_pr_refs.ts b/scripts/check_changelog_pr_refs.ts index 2a86c9bc6..c7b71b2df 100644 --- a/scripts/check_changelog_pr_refs.ts +++ b/scripts/check_changelog_pr_refs.ts @@ -5,10 +5,11 @@ * 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. + * pull request, that is, in the fragment or the changelog at the merge base + * of the base and head revisions or on the branches that the pull request + * merged in (see {@link findIntroducedEntries}). Entries that `sacho carry` + * restores from a released section are compared with that section. A pull + * request without any changelog fragment passes. * * Usage: * @@ -24,6 +25,9 @@ import { parse as parseYaml } from "@std/yaml"; /** The directory that holds changelog fragments, relative to the root. */ export const FRAGMENTS_DIRECTORY = "changes.d"; +/** The changelog that Sacho compiles the fragments into. */ +export const CHANGELOG_PATH = "CHANGES.md"; + /** The URL prefix of pull requests in the upstream repository. */ export const PULL_REQUEST_URL_PREFIX = "https://github.com/fedify-dev/fedify/pull/"; @@ -42,34 +46,42 @@ function isFragmentPath(path: string): boolean { /** * 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 or the same - * trailing references. So edits to historical entries are not introduced, - * while an entry citing the current pull request or a mispredicted number - * is. + * existed before if the same first paragraph appeared in the fragment or the + * changelog before, or failing that, if it edits a previous entry of the + * fragment with the same first line or the same trailing references. So + * edits to historical entries and entries that `sacho carry` restores from + * released sections are not introduced, while an entry citing the current + * pull request or a mispredicted number is. * * @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. + * @param changelogs The contents of the changelog at the same revisions. + * Only entries with trailing references and the same first + * paragraph are matched against them, since many changelog + * entries share their trailing references or lack them. + * @returns `"all"` if neither the fragment nor any of its entries existed + * before; otherwise, the 0-based indices of the top-level list items + * it introduces. */ export function findIntroducedEntries( content: string, previous: readonly string[], + changelogs: 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 logged = new Set( + changelogs.flatMap(extractEntries).map((entry) => entry.paragraph), + ); + const before = previous.flatMap(extractEntries); const matched = new Set(); - const existing = new Set(); + const existing = new Set( + [...after.keys()].filter((index) => + after[index].references != null && logged.has(after[index].paragraph) + ), + ); + if (before.length < 1 && existing.size < 1) return "all"; for (const key of ["paragraph", "firstLine", "references"] as const) { after.forEach((entry, index) => { if (existing.has(index) || entry[key] == null) return; @@ -77,13 +89,41 @@ export function findIntroducedEntries( !matched.has(i) && old[key] === entry[key] ); if (origin < 0) return; - matched.add(origin); + // The same entry usually appears in several previous revisions: + for (const [i, old] of before.entries()) { + if (old.paragraph === before[origin].paragraph) matched.add(i); + } existing.add(index); }); } return new Set([...after.keys()].filter((index) => !existing.has(index))); } +/** + * Split a changelog into its version sections. + * + * @param changelog The changelog content. + * @returns The content of each section, keyed by its version. + */ +function extractVersionSections(changelog: string): Map { + const lines = normalizeNewlines(changelog).split("\n"); + const sections = new Map(); + let current: string[] = []; + for (let i = 0; i < lines.length; i++) { + const heading = /^Version (\S+)\s*$/.exec(lines[i]); + if (heading != null && /^-{3,}\s*$/.test(lines[i + 1] ?? "")) { + current = []; + sections.set(heading[1], current); + i++; // Skip the underline. + } else { + current.push(lines[i]); + } + } + return new Map( + [...sections].map(([version, body]) => [version, body.join("\n")]), + ); +} + /** A top-level list item of a changelog fragment. */ interface Entry { /** The line holding the list marker, with its whitespace collapsed. */ @@ -344,17 +384,36 @@ export async function checkChangelogPrRefs( "--", FRAGMENTS_DIRECTORY, ); + // Each line is a status followed by the path, or by the old and new paths of + // a rename: + const changes = diff.split("\n") + .map((line) => line.split("\t")) + .filter(([status, ...paths]) => + !status.startsWith("D") && paths.length > 0 && + isFragmentPath(paths[paths.length - 1]) + ) + .map(([, ...paths]) => ({ path: paths[paths.length - 1], paths })); + if (changes.length < 1) return { fragments: [], violations: [] }; + // Only revisions before the pull request count, since the changelog at the + // head also holds the pull request's own entries: + const changelogs: string[] = []; + const sections = new Map(); + for (const revision of [mergeBase, ...mergedIn]) { + const changelog = await readFile(revision, CHANGELOG_PATH); + if (changelog == null) continue; + changelogs.push(changelog); + for (const [version, section] of extractVersionSections(changelog)) { + if (!sections.has(version)) sections.set(version, section); + } + } 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; - } + for (const { path, paths } of changes) { const previous: string[] = []; + // `sacho carry ` restores the entries of a released section: + const carried = /(?:^|\/)carried-from-(.+)\.md$/.exec(path); + const section = carried == null ? undefined : sections.get(carried[1]); + if (section != null) previous.push(section); for (const revision of [mergeBase, ...mergedIn]) { for (const oldPath of new Set(paths)) { const content = await readFile(revision, oldPath); @@ -362,7 +421,7 @@ export async function checkChangelogPrRefs( } } const content = await run("show", `${options.head}:${path}`); - const entries = findIntroducedEntries(content, previous); + const entries = findIntroducedEntries(content, previous, changelogs); if (entries !== "all" && entries.size < 1) continue; fragments.push(path); violations.push( From dd346d744ac473d087319694356f910a7f87af59 Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 26 Oct 2026 22:52:29 +0900 Subject: [PATCH 6/9] Accept non-numeric trailing references Entries for security fixes end with reference groups such as `\[[GHSA-4qhx-hhwf-8685], [#1318], [#1328]]`: the opening bracket may be escaped, and the group may include advisories besides issue and PR numbers. The check only recognized groups of numeric references, so it reported the entry in #1328 as having no trailing references, and could not pair a carried entry citing only an advisory with its original once the entry was reworded. Accept any bracketed reference and an escaped opening bracket, and compare all references when pairing entries. Only numeric references count for the PR number itself. https://github.com/fedify-dev/fedify/pull/1328 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 | 52 +++++++++++++++++++++++++ scripts/check_changelog_pr_refs.ts | 38 +++++++++++++----- 2 files changed, 80 insertions(+), 10 deletions(-) diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts index 6e37f7db5..73df1f265 100644 --- a/scripts/check_changelog_pr_refs.test.ts +++ b/scripts/check_changelog_pr_refs.test.ts @@ -177,6 +177,32 @@ describe("checkFragment()", () => { } }); + it("accepts references to advisories and escaped brackets", () => { + const content = [ + "---", + "links:", + ` '#456': ${pull(456)}`, + "---", + " - Fixed a bug where foo would bar.", + " \\[[GHSA-4qhx-hhwf-8685], [#123], [#456]]", + "", + ].join("\n"); + deepStrictEqual(checkFragment(PATH, content, 456), []); + }); + + it("reports references to advisories without the pull request", () => { + const content = fragment( + { "#456": pull(456) }, + "[[GHSA-4qhx-hhwf-8685]]", + ); + const violations = checkFragment(PATH, content, 456); + strictEqual(violations.length, 1); + match( + violations[0].message, + /references of the entry \(\[GHSA-4qhx-hhwf-8685\]\) do not include/, + ); + }); + it("reports malformed frontmatter", () => { const content = "---\nlinks: [\n---\n - Added foo. [[#456]]\n"; const violations = checkFragment(PATH, content, 456); @@ -290,6 +316,32 @@ describe("findIntroducedEntries()", () => { strictEqual(introduced instanceof Set && introduced.size, 1); }); + it("treats changelog entries citing only advisories as existing", () => { + const changelog = "Version 1.0.1\n-------------\n\n" + + " - Fixed foo. \\[[GHSA-r48c-xx2h-fv4g]]\n"; + deepStrictEqual( + findIntroducedEntries( + " - Fixed foo.\n \\[[GHSA-r48c-xx2h-fv4g]]\n", + [], + [ + changelog, + ], + ), + new Set(), + ); + }); + + it("pairs edited entries citing only advisories by their references", () => { + // As when a carried security fix is reworded afterward: + deepStrictEqual( + findIntroducedEntries( + " - Fixed an SSRF vulnerability.\n [[GHSA-r48c-xx2h-fv4g]]\n", + [" - Fixed a server-side request forgery.\n [[GHSA-r48c-xx2h-fv4g]]\n"], + ), + new Set(), + ); + }); + it("does not treat changelog entries without references as existing", () => { deepStrictEqual( findIntroducedEntries(" - Updated dependencies.\n", [], [ diff --git a/scripts/check_changelog_pr_refs.ts b/scripts/check_changelog_pr_refs.ts index c7b71b2df..1bc702db5 100644 --- a/scripts/check_changelog_pr_refs.ts +++ b/scripts/check_changelog_pr_refs.ts @@ -130,7 +130,10 @@ interface Entry { readonly firstLine: string; /** The first paragraph of the item, with its whitespace collapsed. */ readonly paragraph: string; - /** The sorted trailing references of the item, such as `#5 #7`, if any. */ + /** + * The sorted trailing references of the item, such as `#5 #7 GHSA-…`, if + * any. + */ readonly references: string | null; } @@ -194,24 +197,39 @@ function extractEntries(content: string): Entry[] { return { firstLine: collapse(trimmed[0]), paragraph, - references: references == null || references.length < 1 + references: references == null ? null - : references.sort((a, b) => a - b).map((n) => `#${n}`).join(" "), + : references.toSorted((a, b) => + a.localeCompare(b, "en", { numeric: true }) + ) + .join(" "), }; }); } /** * Matches the trailing reference group of a paragraph, such as - * `[[#123], [#456] by John Doe]`. + * `[[#123], [#456] by John Doe]` or `\[[GHSA-xxxx-xxxx-xxxx], [#456]]`. */ const TRAILING_REFERENCES_PATTERN = - /\[(\[#\d+\](?:\s*,\s*\[#\d+\])*)(?:\s+by\s+[^\[\]]+)?\]\s*$/; + /\\?\[(\[[^\[\]]+\](?:\s*,\s*\[[^\[\]]+\])*)(?:\s+by\s+[^\[\]]+)?\]\s*$/; -function extractTrailingReferences(paragraph: string): number[] | null { +/** + * Return the trailing references of a paragraph, such as `#123` or + * `GHSA-xxxx-xxxx-xxxx`, or `null` if it has none. + */ +function extractTrailingReferences(paragraph: string): string[] | 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 Array.from(match[1].matchAll(/\[([^\[\]]+)\]/g), (m) => m[1]); +} + +/** Return the issue and pull request numbers among trailing references. */ +function toNumbers(references: readonly string[]): number[] { + return references.flatMap((reference) => { + const match = /^#(\d+)$/.exec(reference); + return match == null ? [] : [Number(match[1])]; + }); } /** Return the numbers of the pull requests that `links` points to. */ @@ -258,7 +276,7 @@ export function checkFragment( // Other entries' pull requests are not wrong predictions: const cited = new Set( selected.flatMap(({ entry }) => - extractTrailingReferences(entry.paragraph) ?? [] + toNumbers(extractTrailingReferences(entry.paragraph) ?? []) ), ); const otherPullRequests = parsed.links == null @@ -302,12 +320,12 @@ export function checkFragment( `paragraph with the accepted issue and the pull request, e.g., ` + `[[#123], [${ref}]].`, }); - } else if (!references.includes(pullRequest)) { + } else if (!toNumbers(references).includes(pullRequest)) { violations.push({ path, message: `the trailing references of the ${entry} (${ - references.map((n) => `[#${n}]`).join(", ") + references.map((reference) => `[${reference}]`).join(", ") }) do not include the current pull request. Add [${ref}] to ` + `them.${predictionHint}`, }); From 3c4c1a2aada4e42f102897f2f7d26488cc0ac3c2 Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 26 Oct 2026 23:18:23 +0900 Subject: [PATCH 7/9] Keep test repositories from packing objects The Git error test removes a loose object from a temporary repository to cause a failure, so it would break if Git packed the objects first. Disable automatic garbage collection in the test repositories so that their objects stay loose. 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts index 73df1f265..52e899942 100644 --- a/scripts/check_changelog_pr_refs.test.ts +++ b/scripts/check_changelog_pr_refs.test.ts @@ -397,6 +397,9 @@ describe("checkChangelogPrRefs()", () => { "commit.gpgSign=false", "-c", "core.hooksPath=/dev/null", + // Keep objects loose, since a test removes one of them: + "-c", + "gc.auto=0", ...args, ], cwd: root, From 01879891e0a1772ebdbc457769887935e617ef77 Mon Sep 17 00:00:00 2001 From: co2plant Date: Mon, 26 Oct 2026 23:18:41 +0900 Subject: [PATCH 8/9] Document carried sections as previous contents `findIntroducedEntries()` also receives the released section that `sacho carry` restored a fragment from as one of its previous contents, but its documentation only mentioned the merge base and the merged branches. https://github.com/fedify-dev/fedify/issues/1236 Changelog: none Assisted-by: Claude Code:claude-opus-5-5 --- scripts/check_changelog_pr_refs.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/check_changelog_pr_refs.ts b/scripts/check_changelog_pr_refs.ts index 1bc702db5..2c919b4d5 100644 --- a/scripts/check_changelog_pr_refs.ts +++ b/scripts/check_changelog_pr_refs.ts @@ -56,7 +56,9 @@ function isFragmentPath(path: string): boolean { * @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. + * in. For a fragment that `sacho carry` restored, also the + * released section it was restored from. Empty if none of + * them exist. * @param changelogs The contents of the changelog at the same revisions. * Only entries with trailing references and the same first * paragraph are matched against them, since many changelog From ac176107e862c089e49b95a1c8aaf90c27bd3b17 Mon Sep 17 00:00:00 2001 From: co2plant Date: Tue, 27 Oct 2026 22:58:32 +0900 Subject: [PATCH 9/9] Disable tag signing in test repositories With tag.gpgSign enabled in the global Git configuration, `git tag` creates a signed annotated tag and opens an editor for its message, so the test of entries restored by sacho carry hung. Disable tag signing in the test's Git helper, as it already does for commits, so that the test creates a lightweight tag regardless of the configuration. https://github.com/fedify-dev/fedify/pull/1247#discussion_r4385210326 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 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/check_changelog_pr_refs.test.ts b/scripts/check_changelog_pr_refs.test.ts index 52e899942..8ab566fa8 100644 --- a/scripts/check_changelog_pr_refs.test.ts +++ b/scripts/check_changelog_pr_refs.test.ts @@ -396,6 +396,8 @@ describe("checkChangelogPrRefs()", () => { "-c", "commit.gpgSign=false", "-c", + "tag.gpgSign=false", + "-c", "core.hooksPath=/dev/null", // Keep objects loose, since a test removes one of them: "-c",