diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 7bc7fed9a..87b695bab 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -34,7 +34,15 @@ https://github.com/OpenINF/.github/blob/HEAD/CONTRIBUTING.md. ## Emojis for categorizing pull requests -_Copy and paste one of the following emoji into description_ +The title becomes the subject of the commit that lands, so it has to hold +to the same rules. They are written up, with the reasoning, at +https://open.inf.is/docs/handbook/style/commit-messages/ -- in short: a +category emoji, then optionally an action, then `๏ผš` (U+FF1A), then what +the change does, in 50 characters or fewer and with no `#NNNN` on the end. + +_Copy and paste one of the following emoji into description_ -- copy +rather than type, since some have a lookalike spelling that is not the +one recognized here. ๐Ÿท๏ธ meta ๐Ÿ‹ dev container diff --git a/.github/workflows/lint-and-test.yml b/.github/workflows/lint-and-test.yml index fe45e381a..6035cdb6b 100644 --- a/.github/workflows/lint-and-test.yml +++ b/.github/workflows/lint-and-test.yml @@ -10,6 +10,10 @@ jobs: steps: - name: Check out project repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # `verify.commits` compares against the base branch, which a + # single-commit checkout does not contain. + fetch-depth: 0 - name: Set up Node.js runtime uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: @@ -63,6 +67,11 @@ jobs: - '**/*.yml' - '**/*.yaml' + # Not behind a filter: every pull request has commit messages, whatever + # it touches. + - name: Verify commit messages + run: nps verify.commits + # Use the filter to check if files with a specific file type were changed # in the PR. If they were, run the relevant linters. Otherwise, skip. - name: Verify Browserslist diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts new file mode 100644 index 000000000..65dd3eaf9 --- /dev/null +++ b/build/shared/commit-message.mts @@ -0,0 +1,388 @@ +/** + * @file The commit message format, as rules a message can be checked against. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/shared/commit-message + */ + +/** + * What the change is about. Kept in step with the list in + * .github/PULL_REQUEST_TEMPLATE.md, which is where a contributor reads it; + * a test fails if the two drift apart. + * + * Each is spelt so that it is drawn as an emoji and no more: the characters + * that would otherwise come out as flat text carry U+FE0F, and the ones + * already drawn in colour do not carry one they have no use for. A test holds + * the list to that. + */ +export const CATEGORIES: Record = { + '๐Ÿท๏ธ': 'meta', + '๐Ÿ‹': 'dev container', + '๐Ÿงฉ': 'extension โˆฅ plugin', + '๐Ÿ—๏ธ': 'infrastructure โˆฅ tooling โˆฅ builds โˆฅ CI/CD', + 'โš•๏ธ': 'community health files', + '๐Ÿงช': 'tests', + 'โ„๏ธ': 'flaky tests', + '๐Ÿ’„': 'CSS โˆฅ styling', + 'โ™ฟ': 'accessibility', + '๐ŸŒ': 'internationalization', + '๐Ÿ“–': 'documentation', + '๐Ÿ“ฆ': 'packages & package management', +}; + +/** What is being done to it. Optional: the template allows leaving it off. */ +export const ACTIONS: Record = { + 'โœจ': 'new feature', + '๐Ÿ”ง': 'bug fix', + '๐Ÿ”ฅ': 'P0 fix', + '๐Ÿš€': 'performance improvements', + 'โช': 'reverting a previous change', + 'โ™ป๏ธ': 'refactoring', + '๐Ÿšฎ': 'deleting code', + '๐Ÿฅผ': 'experimental code', +}; + +/** Issue #1539: 50 for the subject, 72 for everything after it. */ +export const SUBJECT_MAX = 50; +export const BODY_MAX = 72; + +/** + * The order the trailers appear in a landed commit, which is what nodejs/node + * produces and worth matching rather than inventing: whatever the branch + * commit already carried comes first, then what the landing adds. + */ +export const TRAILER_ORDER = [ + 'Co-authored-by', + 'Signed-off-by', + 'Assisted-by', + 'PR-URL', + 'Fixes', + 'Refs', + 'Reviewed-by', +]; + +/** U+FF1A, which separates the emoji from the description. */ +const IDEOGRAPHIC_COLON = '๏ผš'; + +/** The invisible character that distinguishes two spellings of one emoji. */ +const EMOJI_SELECTOR = '๏ธ'; + +/** Where the emoji are laid out for copying. */ +const HANDBOOK_URL = 'https://open.inf.is/docs/handbook/style/commit-messages/'; + +const VOCABULARY: Record = { ...CATEGORIES, ...ACTIONS }; + +/** + * Emoji that come out of a keyboard or a picker looking right while being a + * different string from the one the vocabulary uses. The answer is always to + * copy the emoji rather than to reason about which spelling it is. + */ +const NEAR_MISSES = new Map( + Object.keys(VOCABULARY).map((emoji) => + emoji.endsWith(EMOJI_SELECTOR) + ? [emoji.slice(0, -EMOJI_SELECTOR.length), emoji] + : [`${emoji}${EMOJI_SELECTOR}`, emoji] + ) +); + +/** + * Points at the spelling to use, since the two look alike. + * @param {string} cluster What was written. + * @returns {string} What to write instead. + */ +const describeNearMiss = (cluster: string) => { + const intended = NEAR_MISSES.get(cluster) ?? ''; + + return `โ€œ${cluster}โ€ is not the emoji for ${VOCABULARY[intended]}; copy โ€œ${intended}โ€ from ${HANDBOOK_URL}`; +}; + +const countGraphemes = (text: string) => + [...new Intl.Segmenter().segment(text)].length; + +/** + * A trailer is `Token: value` with no whitespace in the token. Written out + * rather than taken from a list of known tokens, so that a line *meant* as a + * trailer is recognised as one and can be reported as misspelt. + */ +const TRAILER_LINE = /^(?[A-Za-z][\w-]*):[ \t]*(?.*)$/; + +/** + * `Assisted-by` names a tool, not a person, and so takes neither a name nor an + * address: the Linux kernel defines it as `AGENT_NAME:MODEL_VERSION` followed + * by any specialised analysis tools, and nodejs/node lands it that way. Basic + * development tools are left out. + */ +const ASSISTED_BY_VALUE = /^\S+:\S+( \S+)*$/; + +/** git folds a trailer whose value runs onto an indented line beneath it. */ +const CONTINUATION_LINE = /^\s/; + +/** + * Splits a message body into paragraphs of non-empty lines. + * @param {string[]} lines Every line after the subject. + * @returns {string[][]} The paragraphs, in order. + */ +const paragraphsOf = (lines: string[]) => + lines + .join('\n') + .split(/\n{2,}/) + .map((paragraph) => paragraph.split('\n').filter(Boolean)) + .filter((paragraph) => paragraph.length > 0); + +/** + * Reads the trailer block out of a message, agreeing with git about whether + * there is one: the last paragraph, every line of it either a trailer or a + * continuation of the one above, and the first of them a trailer. A paragraph + * that merely contains a colon somewhere is prose, and git reads no trailers + * in it -- so neither does this. + * @param {string} message The whole commit message. + * @returns {string[]} The trailer lines, one per trailer, empty if there is no block. + */ +export function readTrailers(message: string) { + const [, ...rest] = message.replace(/[\r\n]+$/, '').split(/\r?\n/); + const last = paragraphsOf(rest).at(-1) ?? []; + const isBlock = + last.length > 0 && + TRAILER_LINE.test(last[0] ?? '') && + last.every( + (line) => TRAILER_LINE.test(line) || CONTINUATION_LINE.test(line) + ); + + return isBlock ? last.filter((line) => !CONTINUATION_LINE.test(line)) : []; +} + +/** + * Checks the subject against the vocabulary and the length limit. + * @param {string} subject The first line of the message. + * @returns {string[]} What is wrong with it, empty if nothing. + */ +const checkSubject = (subject: string) => { + const problems: string[] = []; + const colon = subject.indexOf(IDEOGRAPHIC_COLON); + + if (colon === -1) { + problems.push( + `subject needs an emoji prefix and โ€œ${IDEOGRAPHIC_COLON}โ€ (U+FF1A), as in โ€œ๐Ÿ—๏ธ๐Ÿ”ง${IDEOGRAPHIC_COLON}fix the thingโ€` + ); + } else { + const prefix = subject.slice(0, colon); + const description = subject.slice(colon + IDEOGRAPHIC_COLON.length); + // The variation selector belongs to the character before it, so the + // prefix has to be read as grapheme clusters and not code points. + const clusters = [...new Intl.Segmenter().segment(prefix)].map( + (entry) => entry.segment + ); + + if (clusters.length === 0) { + problems.push('subject has no emoji before the colon'); + } else if (clusters.length > 2) { + problems.push( + `subject has ${clusters.length} emoji before the colon; expected a category and at most one action` + ); + } else { + const [first, second] = clusters; + + if (first !== undefined && !(first in CATEGORIES)) { + if (NEAR_MISSES.has(first)) { + problems.push(describeNearMiss(first)); + } else if (first in ACTIONS) { + problems.push( + `โ€œ${first}โ€ is an action, not a category; a category comes first` + ); + } else { + problems.push(`โ€œ${first}โ€ is not a category emoji`); + } + } + + if (second !== undefined && !(second in ACTIONS)) { + problems.push( + NEAR_MISSES.has(second) + ? describeNearMiss(second) + : `โ€œ${second}โ€ is not an action emoji` + ); + } + } + + if (description.length === 0) { + problems.push('subject has nothing after the colon'); + } else if (description.startsWith(' ')) { + problems.push('subject has a space after the colon'); + } + + if (/\s#\d+$/.test(description)) { + problems.push( + 'subject ends with a pull request number; `PR-URL:` carries that' + ); + } + + if (description.endsWith('.')) { + problems.push('subject ends with a full stop'); + } + } + + const width = countGraphemes(subject); + + if (width > SUBJECT_MAX) { + problems.push( + `subject is ${width} characters; the limit is ${SUBJECT_MAX}` + ); + } + + return problems; +}; + +/** + * Checks the paragraph a trailer block would have to be, which git only ever + * looks for at the very end of the message. + * @param {string[]} lines Every line of the message after the subject. + * @returns {string[]} What is wrong with them, empty if nothing. + */ +const checkTrailers = (lines: string[]) => { + const problems: string[] = []; + const paragraphs = paragraphsOf(lines); + const last = paragraphs.at(-1) ?? []; + + // `PR URL:` is the mistake worth naming outright: the space means git reads + // no trailer there, and one unreadable line disqualifies every trailer + // beside it, so the whole block goes silently missing. Matched against the + // known tokens with their hyphens loosened, since a looser test than that + // flags any body sentence containing a colon. + for (const line of last) { + for (const token of TRAILER_ORDER) { + const spaced = new RegExp(`^${token.replaceAll('-', '[ -]')}:`, 'i'); + + if ( + spaced.test(line) && + !line.toLowerCase().startsWith(`${token.toLowerCase()}:`) + ) { + problems.push( + `โ€œ${line.split(':')[0]}:โ€ is spelt โ€œ${token}:โ€; a space in the token disqualifies the whole block` + ); + } + } + } + + // A trailer above the final paragraph is not a trailer. Co-authored-by is + // the one that costs something: GitHub reads it only at the end, so + // attribution is quietly lost. + for (const paragraph of paragraphs.slice(0, -1)) { + for (const line of paragraph) { + const token = line.match(TRAILER_LINE)?.groups?.token; + + if (token !== undefined && TRAILER_ORDER.includes(token)) { + problems.push( + `โ€œ${token}:โ€ is not in the last paragraph, so git does not read it as a trailer` + ); + } + } + } + + const tokens: string[] = []; + const block = readTrailers(['', ...lines].join('\n')); + + // A last paragraph that is not a clean block is prose, and git reads no + // trailers in it. Saying so is only worth doing for a line that was plainly + // meant as one of ours, since it is going unread. + if (block.length === 0) { + for (const line of last) { + const token = line.match(TRAILER_LINE)?.groups?.token ?? ''; + + if ( + TRAILER_ORDER.some( + (known) => known.toLowerCase() === token.toLowerCase() + ) + ) { + problems.push( + `โ€œ${token}:โ€ sits in a paragraph that is not all trailers, so git reads none of them` + ); + } + } + } else { + for (const line of block) { + const found = line.match(TRAILER_LINE)?.groups; + const token = found?.token ?? ''; + + if ( + token.toLowerCase() === 'assisted-by' && + !ASSISTED_BY_VALUE.test(found?.value ?? '') + ) { + problems.push( + `โ€œAssisted-by: ${found?.value}โ€ names a tool, not a person: write it as agent:model-version, as in โ€œAssisted-by: Claude-Code:claude-opus-5โ€` + ); + } + + // Case is part of the spelling. git and GitHub would match these either + // way, so this is about a history that reads the same throughout rather + // than about being understood. + if (!TRAILER_ORDER.includes(token)) { + const canonical = TRAILER_ORDER.find( + (known) => known.toLowerCase() === token.toLowerCase() + ); + + problems.push( + canonical === undefined + ? `โ€œ${token}:โ€ is not a trailer this project uses` + : `โ€œ${token}:โ€ is spelt โ€œ${canonical}:โ€ here` + ); + } + + tokens.push(token); + } + + const ranks = tokens + .filter((token) => TRAILER_ORDER.includes(token)) + .map((token) => TRAILER_ORDER.indexOf(token)); + + if ( + ranks.some((rank, index) => index > 0 && rank < (ranks[index - 1] ?? 0)) + ) { + problems.push( + `trailers are out of order; this project uses ${TRAILER_ORDER.join(', ')}` + ); + } + } + + return problems; +}; + +/** + * Checks one commit message against the project's format. + * @param {string} message The whole message, subject line onwards. + * @returns {string[]} What is wrong with it, empty if nothing. + */ +export function validateCommitMessage(message: string) { + // A trailing newline is how git hands the message over and says nothing + // about the message itself. Carriage returns say nothing either: git reads + // trailers through them, so a message written on Windows must not be judged + // differently from the same message written anywhere else. + const lines = message.replace(/[\r\n]+$/, '').split(/\r?\n/); + const [subject = '', ...rest] = lines; + const problems = checkSubject(subject); + + if (rest.length > 0 && rest[0] !== '') { + problems.push('the line after the subject has to be blank'); + } + + for (const line of rest) { + // A line of dashes is why the trailers in this project have been going + // unread: `---` is where git stops looking for them, and any longer run + // splits the block in two so that only the half below it counts. + if (/^-{3,}$/.test(line)) { + problems.push( + `โ€œ${line}โ€ separates the trailers from the message; git reads only one side of it` + ); + } + + // An unbreakable line -- a URL, near enough always -- cannot be wrapped, + // and reflowing one to fit would break it. + if (countGraphemes(line) > BODY_MAX && /\s/.test(line.trim())) { + problems.push( + `line is ${countGraphemes(line)} characters; the limit is ${BODY_MAX}: โ€œ${line.slice(0, 40)}โ€ฆโ€` + ); + } + } + + problems.push(...checkTrailers(rest)); + + return problems; +} diff --git a/build/shared/commit-message.test.mts b/build/shared/commit-message.test.mts new file mode 100644 index 000000000..462403697 --- /dev/null +++ b/build/shared/commit-message.test.mts @@ -0,0 +1,367 @@ +/** + * @file Tests for the commit message rules. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/shared/commit-message.test + */ + +import { deepStrictEqual, match, ok } from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { readFile } from 'node:fs/promises'; +import { describe, test } from 'node:test'; +import { + ACTIONS, + CATEGORIES, + readTrailers, + validateCommitMessage, +} from '@openinf/portal/build/commit-message'; + +/** The one problem a message has, when a test expects exactly one. */ +const soleProblem = (message: string) => { + const problems = validateCommitMessage(message); + + deepStrictEqual(problems.length, 1, `expected one problem, got: ${problems}`); + + return problems[0] ?? ''; +}; + +describe('validateCommitMessage: the subject', () => { + test('accepts a category, an action and a description', () => { + deepStrictEqual(validateCommitMessage('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix the thing'), []); + }); + + test('accepts a category on its own, which the template allows', () => { + deepStrictEqual(validateCommitMessage('๐Ÿ“–๏ผšwrite it down'), []); + }); + + test('wants the ideographic colon', () => { + match(soleProblem('๐Ÿ—๏ธ๐Ÿ”ง: fix the thing'), /U\+FF1A/); + }); + + test('knows an action from a category', () => { + match(soleProblem('๐Ÿ”ง๏ผšfix the thing'), /is an action, not a category/); + }); + + test('rejects an emoji outside the vocabulary', () => { + match(soleProblem('๐Ÿฆ„๏ผšfix the thing'), /not a category emoji/); + }); + + // The two emoji below are deliberately the wrong spelling -- they are what + // the check has to catch, so leave them be. + test('points at the emoji to copy when a lookalike is used', () => { + // ๐Ÿ—๐Ÿ”ง means an infrastructure fix as plainly as ๐Ÿ—๏ธ๐Ÿ”ง does, and is a + // different string. Saying so is not worth a paragraph about Unicode: the + // message shows what to copy. + match(soleProblem('๐Ÿ—๐Ÿ”ง๏ผšfix the thing'), /copy โ€œ๐Ÿ—๏ธโ€ from https:/); + }); + + test('does the same for the other direction', () => { + match(soleProblem('โ™ฟ๏ธ๏ผšname the landmarks'), /copy โ€œโ™ฟโ€ from https:/); + }); + + test('counts an emoji as the one character it looks like', () => { + // 48 written characters plus a two-emoji prefix and the colon: over the + // limit by code point, inside it by grapheme, and the limit means what a + // reader sees. + const subject = `๐Ÿ—๏ธ๐Ÿ”ง๏ผš${'x'.repeat(47)}`; + + ok(subject.length > 50); + deepStrictEqual(validateCommitMessage(subject), []); + }); + + test('rejects a subject past fifty characters', () => { + match( + soleProblem(`๐Ÿ—๏ธ๐Ÿ”ง๏ผš${'x'.repeat(48)}`), + /subject is 51 characters; the limit is 50/ + ); + }); + + test('rejects a trailing pull request number', () => { + match(soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix the thing #1803'), /PR-URL:` carries that/); + }); + + test('rejects a trailing full stop', () => { + match(soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix the thing.'), /full stop/); + }); + + test('rejects a space after the colon', () => { + match(soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผš fix the thing'), /space after the colon/); + }); +}); + +describe('validateCommitMessage: the body', () => { + test('wants a blank line under the subject', () => { + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\nstraight into the body'), + /has to be blank/ + ); + }); + + test('rejects a line past seventy-two characters', () => { + match( + soleProblem(`๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\n${'word '.repeat(20)}`), + /the limit is 72/ + ); + }); + + test('leaves an unbreakable line alone', () => { + // Reflowing a URL to fit the margin would break the URL. + const url = `https://example.com/${'p'.repeat(80)}`; + + deepStrictEqual( + validateCommitMessage(`๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\n${url}\n\nWhere it is written up.`), + [] + ); + }); + + test('warns about a bare URL as the last paragraph', () => { + // Not pedantry: `git interpret-trailers` reads that line as a trailer + // called `https`, so a URL parked at the end changes what git sees. + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nhttps://example.com/p'), + /โ€œhttps:โ€ is not a trailer this project uses/ + ); + }); +}); + +describe('validateCommitMessage: the trailers', () => { + test('accepts a block in the documented order', () => { + deepStrictEqual( + validateCommitMessage( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-authored-by: A \nPR-URL: https://x/1\nReviewed-by: B ' + ), + [] + ); + }); + + test('insists on the documented spelling of a token', () => { + // git and GitHub would match this either way; the point is a history that + // reads the same throughout. + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-Authored-By: A '), + /is spelt โ€œCo-authored-by:โ€ here/ + ); + }); + + test('rejects a space in place of a hyphen', () => { + // The mistake that has been costing this project its trailers: git reads + // no trailer on that line, and one unreadable line voids the block. + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nPR URL: https://x/1'), + /is spelt โ€œPR-URL:โ€/ + ); + }); + + test('rejects a line of dashes above the block', () => { + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\n-------\n\nCo-authored-by: A '), + /git reads only one side of it/ + ); + }); + + test('rejects trailers out of order', () => { + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nReviewed-by: B \nPR-URL: https://x/1'), + /out of order/ + ); + }); + + test('rejects a trailer stranded above the last paragraph', () => { + match( + soleProblem( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-authored-by: A \n\nsomething else entirely' + ), + /not in the last paragraph/ + ); + }); + + test('rejects prose mixed in with a trailer', () => { + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nPR-URL: https://x/1\nand one more thing'), + /not all trailers, so git reads none of them/ + ); + }); + + test('leaves a closing paragraph of prose alone', () => { + // git reads no trailers in a paragraph that is not all trailers, so + // neither does this. Rejecting it was a false positive found in review: + // any commit ending on an explanatory line with a colon in it was refused. + deepStrictEqual( + validateCommitMessage( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCloses the loop.\n\nNote: this only affects staging.\nNothing else changes here.' + ), + [] + ); + }); + + test('allows a trailer folded onto an indented line', () => { + // git's own syntax for a long trailer value, and it parses this as one + // trailer. Rejecting it was a false positive found in review. + deepStrictEqual( + validateCommitMessage( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-authored-by: Jane Doe\n ' + ), + [] + ); + }); + + test('judges a message with carriage returns the same way', () => { + // git reads trailers straight through CRLF. Before this, a `\r` made the + // blank line look non-blank and hid every trailer problem behind it. + match( + soleProblem( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\r\n\r\nBody line.\r\n\r\nReviewed-by: A \r\nPR-URL: https://x/1\r\n' + ), + /out of order/ + ); + }); + + test('does not mind a trailer repeated', () => { + deepStrictEqual( + validateCommitMessage( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-authored-by: A \nCo-authored-by: B \nReviewed-by: C \nReviewed-by: D ' + ), + [] + ); + }); + + test('accepts an assistant named the way the kernel defines it', () => { + deepStrictEqual( + validateCommitMessage( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nAssisted-by: Claude-Code:claude-opus-5' + ), + [] + ); + }); + + test('rejects an assistant written as a person', () => { + // What this repository had been carrying. `Assisted-by` names a tool, so + // an address makes a claim about authorship that the trailer exists to + // avoid making. + match( + soleProblem( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nAssisted-by: Claude Opus 5 ' + ), + /names a tool, not a person/ + ); + }); + + test('rejects a token this project does not use', () => { + match( + soleProblem('๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCloses: https://x/1'), + /not a trailer this project uses/ + ); + }); + + test('leaves ordinary prose containing a colon alone', () => { + deepStrictEqual( + validateCommitMessage( + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nWhat went wrong: the glob skipped dot files.' + ), + [] + ); + }); +}); + +describe('validateCommitMessage: against what landed', () => { + test('rejects the shape every recent commit has used', () => { + const problems = validateCommitMessage( + [ + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšstop the verify task rewriting the files it checks #1803', + '', + 'PR URL: https://github.com/OpenINF/openinf.github.io/pull/1803', + 'Reviewed-by: @OpenINFbot', + '', + '-------', + '', + 'Co-authored-by: Claude Sonnet 5 ', + ].join('\n') + ); + + ok(problems.some((problem) => /PR-URL:/.test(problem))); + ok(problems.some((problem) => /one side of it/.test(problem))); + ok(problems.some((problem) => /the limit is 50/.test(problem))); + ok(problems.some((problem) => /PR-URL:` carries that/.test(problem))); + }); +}); + +describe('readTrailers', () => { + test('agrees with git about where the trailers are', () => { + // The rules describe git's behaviour, so git is the thing to check them + // against. Every disagreement found in review is in this table. + const messages = [ + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nPR-URL: https://x/1', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-authored-by: A \nPR-URL: https://x/1', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nCo-authored-by: Jane Doe\n ', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nNote: only staging.\nNothing else changes.', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nPR-URL: https://x/1\nand one more thing', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nrefs: not a trailer\n\nThe body.', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\r\n\r\nBody.\r\n\r\nPR-URL: https://x/1\r\n', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\n-------\n\nPR-URL: https://x/1', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nPR URL: https://x/1\nReviewed-by: A ', + '๐Ÿ—๏ธ๐Ÿ”ง๏ผšfix it\n\nhttps://example.com/p', + ]; + + for (const message of messages) { + const theirs = execFileSync('git', ['interpret-trailers', '--parse'], { + encoding: 'utf8', + input: message, + }) + .split('\n') + .filter(Boolean); + + deepStrictEqual( + readTrailers(message).length, + theirs.length, + `git reads ${theirs.length} trailers in ${JSON.stringify(message)}` + ); + } + }); +}); + +describe('the vocabulary', () => { + test('is spelt so that every entry is drawn as an emoji, and no more', () => { + // Two ways to get this wrong, and both leave a second spelling of one + // symbol: a character that needs U+FE0F to be drawn in colour and does + // not carry it, and one drawn in colour already that carries a selector + // it has no use for. + for (const emoji of [...Object.keys(CATEGORIES), ...Object.keys(ACTIONS)]) { + const [base = ''] = [...emoji]; + const selected = emoji.endsWith('๏ธ'); + const drawnAsEmoji = /\p{Emoji_Presentation}/u.test(base); + + deepStrictEqual( + selected, + !drawnAsEmoji, + drawnAsEmoji + ? `${emoji} carries a selector it does not need` + : `${emoji} is drawn as text without a selector` + ); + } + }); + + test('matches the list contributors are shown', async () => { + // The template is where the emoji are documented, so drift between it and + // the rules is worth failing over rather than discovering in review. + const template = await readFile( + new URL('../../.github/PULL_REQUEST_TEMPLATE.md', import.meta.url), + 'utf8' + ); + const documented = new Set( + [...template.matchAll(/^(\P{ASCII}๏ธ?) \S/gmu)].map( + (found) => found[1] ?? '' + ) + ); + + for (const emoji of [...Object.keys(CATEGORIES), ...Object.keys(ACTIONS)]) { + ok(documented.has(emoji), `${emoji} is not in the pull request template`); + } + + deepStrictEqual( + documented.size, + Object.keys(CATEGORIES).length + Object.keys(ACTIONS).length, + 'the template documents an emoji the rules do not know' + ); + }); +}); diff --git a/build/tasks/verify/verify-commits.mts b/build/tasks/verify/verify-commits.mts new file mode 100644 index 000000000..a4e534072 --- /dev/null +++ b/build/tasks/verify/verify-commits.mts @@ -0,0 +1,89 @@ +/** + * @file Verify commit messages on this branch follow the project's format. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/tasks/verify/verify-commits + */ + +import { execFileSync } from 'node:child_process'; +import { + readTrailers, + validateCommitMessage, +} from '@openinf/portal/build/commit-message'; + +const git = (...args: string[]) => + execFileSync('git', args, { encoding: 'utf8' }).trim(); + +/** + * Finds what to compare against: the base branch of the pull request when a + * workflow says so, and the default branch otherwise. + * @returns {string} A revision, or an empty string if none could be resolved. + */ +const resolveBase = () => { + const candidates = [ + process.env.GITHUB_BASE_REF ? `origin/${process.env.GITHUB_BASE_REF}` : '', + 'origin/live', + 'live', + ].filter(Boolean); + + for (const candidate of candidates) { + try { + return git('rev-parse', '--verify', '--quiet', `${candidate}^{commit}`); + } catch { + // Try the next one; a shallow clone has few of these. + } + } + + return ''; +}; + +const base = resolveBase(); + +if (base === '') { + console.error( + 'Could not resolve a base revision to compare against. In a workflow ' + + 'this means the checkout is too shallow -- `fetch-depth: 0` gives the ' + + 'history this needs.' + ); + process.exitCode = 1; +} else { + const range = `${base}..HEAD`; + const shas = git('rev-list', '--no-merges', range) + .split('\n') + .filter(Boolean); + let failed = 0; + + for (const sha of shas) { + const message = git('log', '-1', '--format=%B', sha); + const problems = validateCommitMessage(message); + + // git has the final say on what counts as a trailer, so the rules above + // are cross-checked against it rather than trusted on their own. A + // disagreement means the rules have drifted from the tool they describe. + const parsed = execFileSync('git', ['interpret-trailers', '--parse'], { + encoding: 'utf8', + input: message, + }) + .split('\n') + .filter(Boolean); + const expected = readTrailers(message); + + if (problems.length === 0 && parsed.length !== expected.length) { + problems.push( + `git reads ${parsed.length} trailers here where the rules read ${expected.length}; the two have drifted apart` + ); + } + + if (problems.length > 0) { + failed += 1; + console.error(`${sha.slice(0, 9)} ${message.split('\n')[0]}`); + for (const problem of problems) console.error(` ${problem}`); + } + } + + console.log( + `Checked ${shas.length} commit${shas.length === 1 ? '' : 's'} in ${range}.` + ); + + if (failed > 0) process.exitCode = 1; +} diff --git a/collections/_docs/handbook/style/commit-messages.md b/collections/_docs/handbook/style/commit-messages.md index 7e99e8f60..a86d50bae 100644 --- a/collections/_docs/handbook/style/commit-messages.md +++ b/collections/_docs/handbook/style/commit-messages.md @@ -3,10 +3,166 @@ title: Commit Messages key_point: How to describe a change in the message that accompanies it. --- -> [!NOTE]\ -> This page is a placeholder. The section below is linked from elsewhere in the -> handbook and is yet to be written. +A commit message is read far more often than it is written, and usually by +someone trying to work out why a line looks the way it does. Write it for them. + +The rules below are checked by `nps verify.commits`, which runs on every pull +request. Nothing here is a matter of taste that the checker is unaware of: if it +passes, the message conforms. + +## Subject + +The first line names a classification and then says what the change does: + +```text +๐Ÿ—๏ธ๐Ÿ”ง๏ผšlet the glob see dot files +``` + +An ideographic colon, `๏ผš` (U+FF1A), separates the two. It is not the ASCII +colon; the wide character keeps the emoji from crowding the words after it. + +- At most **50 characters**, counting an emoji as the one character it looks + like. +- No full stop at the end. It is a title, not a sentence. +- No space after the colon. +- No pull request number. `PR-URL:` carries that, and repeating it costs six of + the fifty characters. + +Write it in the imperative, as an instruction to the codebase: _let the glob see +dot files_, not _lets_ or _letting_ or _fixed_. ## Classification -Yet to be written. +Every subject opens with a category emoji saying what area the change is in. A +second emoji may follow to say what is being done to it; leave it off when no +single action fits. + +**Copy the emoji from the tables below rather than typing them.** Several have +more than one spelling that looks identical, and only the one here is +recognized. + +Categories: + +| Emoji | Area | +| :---- | :----------------------- | +| ๐Ÿท๏ธ | meta | +| ๐Ÿ‹ | dev container | +| ๐Ÿงฉ | extension โˆฅ plugin | +| ๐Ÿ—๏ธ | infrastructure โˆฅ tooling | +| โš•๏ธ | community health files | +| ๐Ÿงช | tests | +| โ„๏ธ | flaky tests | +| ๐Ÿ’„ | CSS โˆฅ styling | +| โ™ฟ | accessibility | +| ๐ŸŒ | internationalization | +| ๐Ÿ“– | documentation | +| ๐Ÿ“ฆ | packages | + +Actions: + +| Emoji | Doing what | +| :---- | :-------------------------- | +| โœจ | new feature | +| ๐Ÿ”ง | bug fix | +| ๐Ÿ”ฅ | P0 fix | +| ๐Ÿš€ | performance improvement | +| โช | reverting a previous change | +| โ™ป๏ธ | refactoring | +| ๐Ÿšฎ | deleting code | +| ๐Ÿฅผ | experimental code | + +The category comes first. `๐Ÿ”ง๏ผš` on its own is rejected: it says what is +happening without saying where. + +If `nps verify.commits` says an emoji is not the right one when it looks right, +that is the lookalike problem โ€” copy it from the table again. + +## Body + +Leave the second line blank, then explain **why**, wrapped at 72 characters. +What the change does is in the diff; what it is for is not. + +A line may run past 72 characters only if it cannot be wrapped, which in +practice means a bare URL. + +## Trailers + +Metadata goes at the end, as git trailers: one paragraph, every line of the form +`Token: value`, nothing else mixed in. In this order: + +| Trailer | What it records | +| :---------------- | :----------------------------------------- | +| `Co-authored-by:` | someone who wrote part of the change | +| `Signed-off-by:` | a Developer Certificate of Origin sign-off | +| `Assisted-by:` | an AI tool that helped write it | +| `PR-URL:` | the pull request the commit landed through | +| `Fixes:` | an issue this closes | +| `Refs:` | an issue or pull request worth reading | +| `Reviewed-by:` | someone who approved it, as `Name ` | + +Spell them exactly as above, including the lowercase `by`. That is what git +generates for `Signed-off-by`, what GitHub documents for `Co-authored-by`, and +what the Linux kernel established for all of them. git and GitHub would match a +token whatever its case, so this is about a history that reads the same +throughout rather than about being understood. + +`PR-URL:` and `Reviewed-by:` are added when the commit lands. The others belong +in the commit as you write it. + +### Disclosing an AI assistant + +An AI tool that helped write a change is disclosed with `Assisted-by:`, and +takes neither a name nor an address, because it names a tool rather than a +person: + +```text +Assisted-by: agent-name:model-version [tool] [tool] +``` + +The trailing tools are for specialized analysis tools, if any were used; +everyday ones โ€” git, a compiler, an editor โ€” are left out. For example: + +```text +Assisted-by: Claude-Code:claude-opus-5 +``` + +`Co-authored-by:` is for people. An assistant does not go there, and does +**not** get a `Signed-off-by:` either: only a human can certify the [Developer +Certificate of Origin][], and saying a tool helped is not a transfer of +responsibility. You are answerable for every line in your pull request, whatever +wrote it. + +> [!WARNING]\ +> git only looks for trailers in the **last** paragraph of the message, and only +> if every line in it is a trailer. Three consequences, each of which has +> silently cost this project its metadata: +> +> - A space in a token โ€” `PR URL:` instead of `PR-URL:` โ€” is not a trailer, and +> one unreadable line disqualifies every trailer beside it. +> - A line of dashes between the message and the metadata splits it in two. git +> reads only one side, and which side depends on whether the line is exactly +> `---`. +> - A `Co-authored-by:` above the final paragraph is not read at all, so the +> co-author goes uncredited. + +Check what git makes of a message rather than assuming: + +```bash +git log -1 --format=%B | git interpret-trailers --parse +``` + +Anything that does not come back is not a trailer. + +## See Also + +For the emoji as they appear when opening a pull request, see the [pull request +template][]. + + + + +[Developer Certificate of Origin]: https://developercertificate.org/ +[pull request template]: https://github.com/OpenINF/openinf.github.io/blob/HEAD/.github/PULL_REQUEST_TEMPLATE.md + + + diff --git a/package-scripts.yml b/package-scripts.yml index 4b855f3da..b7003c256 100644 --- a/package-scripts.yml +++ b/package-scripts.yml @@ -8,6 +8,7 @@ scripts: # produced it. all: 'rc=0; failed=; for i in build/tasks/verify/*.mts; do echo "==> $i"; node "$i" || { rc=1; failed="$failed $i"; }; done; [ -z "$failed" ] || echo "FAILED:$failed" >&2; exit $rc' browserslist: node build/tasks/verify/verify-browserslist.mts + commits: node build/tasks/verify/verify-commits.mts scss: node build/tasks/verify/verify-scss.mts dockerfile: node build/tasks/verify/verify-dockerfile.mts fileModes: node build/tasks/verify/verify-file-modes.mts diff --git a/package.json b/package.json index aa2e72057..affd224a6 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "pnpm": "11.20.0" }, "exports": { + "./build/commit-message": "./build/shared/commit-message.mts", "./build/constants": "./build/shared/constants.mts", "./build/utils": "./build/utils.mts" },