diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index d74f53adb6..2b2eb39a8e 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -2,4 +2,5 @@ - [ ] Added a changelog fragment (see `changes/README.md`), or no release notes are needed and a maintainer should apply `changelog:skip`. - [ ] Added tests for the fix or feature. +- [ ] Gave every commit a description below its subject, saying why it is made (see "Commit messages" in `CONTRIBUTING.md`). - [ ] Signed off every commit for the DCO (`git commit -s`). diff --git a/.github/workflows/semantic_diff.yml b/.github/workflows/semantic_diff.yml index a4c28f2632..784bc616e4 100644 --- a/.github/workflows/semantic_diff.yml +++ b/.github/workflows/semantic_diff.yml @@ -5,7 +5,9 @@ name: Semantic Diff # Markdown (scripts/recordMarkdown.js) with the page's link. A pull request # has one such comment, which each push updates in place to hold every # commit, so the pull request shows its current records in one place; the -# comment marks the commits the last push added or edited. +# comment marks the commits the last push added or edited. After commenting, +# the job fails if a commit's message has no description below its subject, +# naming those commits. # # A pull request from a branch of this repository runs on pull_request, with # the workflow and renderer of its merge into the base branch: GitHub takes @@ -107,3 +109,7 @@ jobs: comment-tag: semantic-diff mode: upsert file-path: semantic-diff/comment.md + + # Last, so that the page and the comment show the records either way. + - name: Check commit descriptions + run: node scripts/commitRecords.js --check "$(git merge-base "$BASE_SHA" "pr-${PR_NUMBER}")..pr-${PR_NUMBER}" diff --git a/AGENTS.md b/AGENTS.md index 829792fb1f..726eb41425 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -78,6 +78,10 @@ relevant guidance from each. - Write each commit message as a record in the format of [`.agents/skills/records/SKILL.md`](.agents/skills/records/SKILL.md), and render review pages from records with `make semantic-diff` +- Every commit needs a description below its subject: a line or two of + prose or a `Why:` section. This includes small follow-ups like adding a + changelog fragment or fixing CI; a subject alone fails the Semantic Diff + check (`node scripts/commitRecords.js --check origin/master..HEAD`) ### Stacked pull requests diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a9d8eaf89a..1d367f291f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -570,6 +570,21 @@ Javascript Code Note that there is one design goal to keep in mind, never introduce any meaningless symbol unless necessary, we do optimizations, however, it should also compile readable output code. +## Commit messages + +Every commit message has a subject, a description and a sign-off. The description says why the commit is made, as a line or two of prose or a `Why:` section; this applies to small follow-up commits too, such as adding a changelog fragment. Pull requests are squash-merged with their commit messages, so these descriptions end up in `master`'s history. + + Warn about unused open statements + + An open whose module contributes no name silently widens the scope; + the new warning points at it. + + Signed-off-by: Random J. Developer + +The Semantic Diff workflow draws each pull request's commit messages as a review page and comments them on the pull request. It fails if a commit has no description and names that commit; add one with `git rebase -i`, marking the commit `reword`, and force-push. Run the same check locally with `node scripts/commitRecords.js --check origin/master..HEAD`. + +A message can also show what the commit changes, such as the parse tree, generated JavaScript or an error message, Before and After; [`.agents/skills/records/SKILL.md`](.agents/skills/records/SKILL.md) describes the format. + ## PR target branch Target branch `master` for development of new (breaking) features (v12). diff --git a/changes/commit-description-check.internal.md b/changes/commit-description-check.internal.md new file mode 100644 index 0000000000..f5cd271716 --- /dev/null +++ b/changes/commit-description-check.internal.md @@ -0,0 +1 @@ +- Fail the Semantic Diff check when a commit message has no description below its subject. diff --git a/scripts/commitRecords.js b/scripts/commitRecords.js index c106c1a0ba..6676430e89 100644 --- a/scripts/commitRecords.js +++ b/scripts/commitRecords.js @@ -16,13 +16,22 @@ * the commits' authors, drawn in place of their names. With * --previous , the pull request's head before the last push, the * comment marks the commits that push added or whose message it edited. + * --check fails when a record has no description, a paragraph of prose + * or a `Why:` section, naming those records; it needs no --out. */ import { execFileSync } from "node:child_process"; import { readFileSync, writeFileSync } from "node:fs"; import { categories, parseFragment } from "./changelogFragments.js"; import { commentMarkdown } from "./recordMarkdown.js"; -import { CSS, esc, pageBody, pageTitle, parseRecord } from "./recordRender.js"; +import { + CSS, + described, + esc, + pageBody, + pageTitle, + parseRecord, +} from "./recordRender.js"; /** @import { CommitRecord, Group } from "./recordRender.js" */ /** @import { ChangelogEntry } from "./recordMarkdown.js" */ @@ -183,7 +192,8 @@ const options = {}; /** @type {string[]} */ const targets = []; for (let i = 0; i < args.length; i++) { - if ( + if (args[i] === "--check") options.check = "true"; + else if ( [ "--out", "--pr", @@ -199,9 +209,9 @@ for (let i = 0; i < args.length; i++) { else targets.push(args[i]); } const outFile = options.out; -if (!outFile) { +if (!outFile && !options.check) { console.error( - "usage: commitRecords.js --out [--pr ] [--title ] [--markdown [--url ] [--previous ]] [--logins ] ...", + "usage: commitRecords.js (--out [--pr ] [--title ] [--markdown [--url ] [--previous ]] [--logins ] | --check) ...", ); process.exit(1); } @@ -218,27 +228,56 @@ const heading = { title: options.title ?? "", }; const groups = targets.map(group); -const body = pageBody(groups, heading); -if (options.markdown) { - const [base, tip] = targets[targets.length - 1].split(".."); - const records = groups.flatMap(g => g.records); + +/** Draws the page and, with --markdown, the comment. @param {string} outFile */ +function draw(outFile) { + const body = pageBody(groups, heading); + if (options.markdown) { + const [base, tip] = targets[targets.length - 1].split(".."); + const records = groups.flatMap(g => g.records); + writeFileSync( + options.markdown, + commentMarkdown(groups, { + url: options.url, + ...(tip ? { changelog: changelog(base, tip) } : {}), + ...(options.previous && tip + ? marks(records, base, options.previous) + : {}), + }), + ); + console.log(options.markdown); + } + const head = `${esc(pageTitle(heading))} + +`; writeFileSync( - options.markdown, - commentMarkdown(groups, { - url: options.url, - ...(tip ? { changelog: changelog(base, tip) } : {}), - ...(options.previous && tip - ? marks(records, base, options.previous) - : {}), - }), + outFile, + `\n\n${head}${body}\n`, ); - console.log(options.markdown); + console.log(outFile); } -const head = `${esc(pageTitle(heading))} - -`; -writeFileSync( - outFile, - `\n\n${head}${body}\n`, -); -console.log(outFile); + +/** + * Names the records without a description and fails, telling how to add + * one. A pull request's check runs this after its page and comment are + * drawn, so the comment shows the records as they are. + */ +function check() { + const missing = groups.flatMap(g => g.records).filter(r => !described(r)); + if (!missing.length) return; + console.error( + missing.length === 1 + ? "A commit has no description:\n" + : `${missing.length} commits have no description:\n`, + ); + for (const r of missing) console.error(` ${r.sha} ${r.subject}`); + console.error(` +Below its subject, a commit message says why the commit is made, as a line +or two of prose or a \`Why:\` section (see "Commit messages" in +CONTRIBUTING.md). Add one with \`git rebase -i\`, marking these commits +\`reword\`, or squash them into a commit that has one, and force-push.`); + process.exit(1); +} + +if (outFile) draw(outFile); +if (options.check) check(); diff --git a/scripts/commitRecords.test.js b/scripts/commitRecords.test.js index 705a14ad06..4594d7b884 100644 --- a/scripts/commitRecords.test.js +++ b/scripts/commitRecords.test.js @@ -1026,3 +1026,50 @@ test("--previous marks the commits the last push added or edited", async () => { has(marked("Added"), "✨ Added"); has(marked("Added"), " · new"); }); + +test("--check names the commits without a description and fails", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "commit-records-")); + const git = (/** @type {string[]} */ ...a) => + execFileSync("git", a, { cwd: dir, encoding: "utf8" }).trim(); + git("init", "-q", "-b", "main"); + git("config", "user.name", "A Person"); + git("config", "user.email", "a@example.com"); + await commit(git, dir, { "a.txt": "a\n" }, "base"); + const base = git("rev-parse", "HEAD"); + const check = () => + spawnSync("node", [script, "--check", `${base}..HEAD`], { + cwd: dir, + encoding: "utf8", + }); + await commit(git, dir, {}, `Prose\n\nA line of why.\n\n${SIGNED}`); + await commit(git, dir, {}, `Why block\n\nWhy:\n The reason.\n\n${SIGNED}`); + await commit( + git, + dir, + {}, + `Why on its line\n\nWhy: the reason.\nReview: a question?\n\n${SIGNED}`, + ); + let r = check(); + assert.equal(r.status, 0, r.stderr); + await commit(git, dir, {}, `Title only\n\n${SIGNED}`); + await commit( + git, + dir, + {}, + `Trailers only\n\nCo-Authored-By: B \n${SIGNED}`, + ); + await commit( + git, + dir, + {}, + `Blocks only\n\nJS modified:\nBefore (js):\n a;\nAfter (js):\n b;\n\nReview: a question?\n\n${SIGNED}`, + ); + await commit(git, dir, {}, `Empty why\n\nWhy:\n\n${SIGNED}`); + r = check(); + assert.equal(r.status, 1); + assert.match(r.stderr, /^4 commits have no description/); + for (const s of ["Title only", "Trailers only", "Blocks only", "Empty why"]) + assert.ok(r.stderr.includes(` ${s}\n`), `stderr lacks ${s}`); + for (const s of ["Prose", "Why block", "Why on its line"]) + assert.ok(!r.stderr.includes(` ${s}\n`), `stderr has ${s}`); +}); diff --git a/scripts/recordRender.js b/scripts/recordRender.js index a7c7eb80d6..6fea80299b 100644 --- a/scripts/recordRender.js +++ b/scripts/recordRender.js @@ -193,6 +193,19 @@ export function parseRecord(commit) { return { ...commit, subject, sections, coauthors: [...new Set(coauthors)] }; } +/** + * Whether a record says why its commit is made, as the format requires: a + * paragraph of prose or a `Why:` section with content. A subject with only + * trailers, object blocks or code has no description. + * @param {CommitRecord} r + */ +export const described = r => + r.sections.some( + s => + (s.name === "" && s.text.length > 0) || + (s.name === "Why" && (s.text.length > 0 || s.block.some(l => l.trim()))), + ); + /** * Who made a commit, as `A Person + Claude`: its author, by the GitHub login * when known, and its co-authors, each name drawn by `name` and the login