From 2218c1c92324be4e68dc71761e79b0032ae346b4 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Wed, 28 Oct 2026 13:40:51 +0000 Subject: [PATCH 1/3] Fail the Semantic Diff check for commits without a description Commits with only a subject, like #9075's, leave their Semantic Diff cards empty. The records format already requires a description; `commitRecords.js --check` now enforces it after the comment is drawn, and CONTRIBUTING.md, the PR template and AGENTS.md say so. Diagnostic added (commit without a description): After (text): A commit has no description: a1bc9e110 Add missing JSX DOM event bindings 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. Review: What counts as a description? A paragraph of prose, or a `Why:` section with content. Object blocks, code and trailers alone don't count. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gJ4f9jQinqHFuszdea Signed-off-by: Christoph Knittel --- .agents/skills/records/SKILL.md | 6 +- .github/pull_request_template.md | 1 + .github/workflows/semantic_diff.yml | 8 +- AGENTS.md | 4 + CONTRIBUTING.md | 15 ++++ changes/commit-description-check.internal.md | 1 + scripts/commitRecords.js | 89 ++++++++++++++------ scripts/commitRecords.test.js | 47 +++++++++++ scripts/recordRender.js | 13 +++ 9 files changed, 156 insertions(+), 28 deletions(-) create mode 100644 changes/commit-description-check.internal.md diff --git a/.agents/skills/records/SKILL.md b/.agents/skills/records/SKILL.md index cebb83d86de..7142dc1bd8e 100644 --- a/.agents/skills/records/SKILL.md +++ b/.agents/skills/records/SKILL.md @@ -63,8 +63,10 @@ Signed-off-by: Name - The subject, a description and `Signed-off-by` are required: the description says why the commit is made, as a line or two of prose or - a `Why:` section, and object blocks then show what it changes. Every - other section is optional. The message ends with its trailers, git's + a `Why:` section, and object blocks then show what it changes. The + Semantic Diff workflow fails a pull request with a commit that has no + description (`node scripts/commitRecords.js --check ..HEAD`). + Every other section is optional. The message ends with its trailers, git's `Key: value` lines (`Co-Authored-By:`, `Fixes:` or any other one-word key), `Signed-off-by` last; the page lists the `Co-Authored-By` names and draws no other trailer. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index d74f53adb6f..3e89e8d23d5 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -3,3 +3,4 @@ - [ ] 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. - [ ] Signed off every commit for the DCO (`git commit -s`). +- [ ] Gave every commit a description below its subject, saying why it is made (see "Commit messages" in `CONTRIBUTING.md`). diff --git a/.github/workflows/semantic_diff.yml b/.github/workflows/semantic_diff.yml index a4c28f2632e..784bc616e45 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 829792fb1f7..726eb41425c 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 a9d8eaf89a1..1d367f291f3 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 00000000000..f5cd271716a --- /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 c106c1a0ba9..6676430e893 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 705a14ad06f..4594d7b884d 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 a7c7eb80d65..6fea80299b5 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 From 4da09b6d938cbc080fefeef8a7bf7552e4fd1eae Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Wed, 28 Oct 2026 13:50:00 +0000 Subject: [PATCH 2/3] Leave the check command out of the records skill The skill already requires a description, and the failing check explains itself, so naming the script there adds nothing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gJ4f9jQinqHFuszdea Signed-off-by: Christoph Knittel --- .agents/skills/records/SKILL.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.agents/skills/records/SKILL.md b/.agents/skills/records/SKILL.md index 7142dc1bd8e..cebb83d86de 100644 --- a/.agents/skills/records/SKILL.md +++ b/.agents/skills/records/SKILL.md @@ -63,10 +63,8 @@ Signed-off-by: Name - The subject, a description and `Signed-off-by` are required: the description says why the commit is made, as a line or two of prose or - a `Why:` section, and object blocks then show what it changes. The - Semantic Diff workflow fails a pull request with a commit that has no - description (`node scripts/commitRecords.js --check ..HEAD`). - Every other section is optional. The message ends with its trailers, git's + a `Why:` section, and object blocks then show what it changes. Every + other section is optional. The message ends with its trailers, git's `Key: value` lines (`Co-Authored-By:`, `Fixes:` or any other one-word key), `Signed-off-by` last; the page lists the `Co-Authored-By` names and draws no other trailer. From c494d71eda5e065fb0349c4203404b9597009184 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Wed, 28 Oct 2026 13:50:37 +0000 Subject: [PATCH 3/3] List the commit description before the sign-off in the PR template The checklist now follows the order of a commit message: the description comes before the Signed-off-by trailer. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gJ4f9jQinqHFuszdea Signed-off-by: Christoph Knittel --- .github/pull_request_template.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 3e89e8d23d5..2b2eb39a8e4 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -2,5 +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. -- [ ] Signed off every commit for the DCO (`git commit -s`). - [ ] 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`).