From ab6d2d1b29e8ba48fb73b6487498bb4f24620c5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20S=C3=A1ros?= Date: Thu, 3 Sep 2026 11:03:51 +0200 Subject: [PATCH] chore: document comment rules and enforce commit message shape commitlint's headerPattern could not match a scoped subject like `fix(ui-button): ...`, so subject-max-length never applied to anything. Dropping it lets the conventional parser run, which the new subject and body limits rely on. Limits are calibrated against this repo's history: they clear the heaviest real commit and catch changelog-shaped bodies. Co-Authored-By: Claude --- .claude/commands/commit.md | 28 ++- .claude/commands/tidy.md | 68 +++++ .claude/settings.json | 13 + .gitignore | 1 + CLAUDE.md | 25 ++ commitlint.config.js | 88 ++++++- .../contributing-getting-started.md | 12 + .../check-commit-message.test.ts | 235 ++++++++++++++++++ scripts/claude/check-commit-message.mjs | 198 +++++++++++++++ 9 files changed, 659 insertions(+), 9 deletions(-) create mode 100644 .claude/commands/tidy.md create mode 100644 scripts/claude/__node_tests__/check-commit-message.test.ts create mode 100644 scripts/claude/check-commit-message.mjs diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md index 8c90e52b3c..e3c758188f 100644 --- a/.claude/commands/commit.md +++ b/.claude/commands/commit.md @@ -13,16 +13,36 @@ type(scope): imperative subject BREAKING CHANGE: -πŸ€– Generated with [Claude Code](https://claude.com/claude-code) - Co-Authored-By: Claude ``` - **type**: one of `feat`, `fix`, `docs`, `style`, `refactor`, `perf`, `test`, `build`, `ci`, `chore`, `revert`. `commitlint.config.js` extends [`@commitlint/config-conventional`](https://www.npmjs.com/package/@commitlint/config-conventional), which defines the allowed set β€” pick the type that genuinely matches the change (`feat`/`fix` only for actual features/bug fixes). - **scope**: full package name (`ui-button`, `ui-select`). Comma-separate for a few, use `many` for several, omit for repo-wide. -- **subject**: imperative ("add loading state", not "added"). Must start with a lowercase letter (commitlint's `subject-case` rejects sentence/Start/PascalCase). No trailing period. -- **Body lines: hard-wrap at 100 characters.** Commitlint (`body-max-line-length: 100`) runs in CI and will reject longer lines. The footer lines (Claude Code attribution, Co-Authored-By) are exempt. +- **subject**: imperative ("add loading state", not "added"). Must start with a lowercase letter (commitlint's `subject-case` rejects sentence/Start/PascalCase). No trailing period. **Max 72 characters** (`subject-max-length`) β€” if it doesn't fit, you're listing everything the change touches instead of naming the change. - **Breaking changes**: add a `BREAKING CHANGE:` line in the body describing what breaks. See CLAUDE.md for what counts as breaking. +- **Attribution**: end with `Co-Authored-By: Claude ` β€” that exact form, not a model-specific one, so history stays consistent. **No `πŸ€– Generated with` line in commit messages**; that belongs in PR bodies (`/pr` handles it). + +### Body + +**Omit the body when the subject says it all.** When you do write one, it explains **why** β€” the constraint, the cause, the thing the diff cannot show. Never restate what changed. + +- **Hard-wrap at 100 characters** (`body-max-line-length`). Trailers are exempt. +- ❌ **Never turn the body into a changelog.** No grouping headings (`Configuration:`, `Build Tooling:`), no numbered sections, no long bullet list of the files you touched. Commitlint's `body-no-changelog` rejects 2+ headings, more than 12 bullets, or more than 6 bullets naming files. +- βœ… A single lead-in like `The fixes:` followed by a few bullets is fine, and naming a specific file is fine when the file _is_ the point. +- Hard ceiling of 28 body lines (`body-max-lines`) β€” a backstop for runaway bodies, not a target. + +``` +❌ feat(many): migrate from npm to pnpm βœ… feat(many): migrate from npm to pnpm + + Configuration: regression-test stays on npm so it keeps + - Add pnpm-workspace.yaml installing @instructure/ui the way an + - Add .npmrc with hoisted node linker external consumer would. + Build Tooling: + - Update scripts/bootstrap.js + ...25 more lines... +``` + +Writing about _before_ and _after_ is encouraged β€” "Previously the placeholder only showed on hover" is exactly right in a commit message, which is permanently anchored to its own diff. (Code comments are different: see CLAUDE.md.) ## Steps diff --git a/.claude/commands/tidy.md b/.claude/commands/tidy.md new file mode 100644 index 0000000000..34ad623324 --- /dev/null +++ b/.claude/commands/tidy.md @@ -0,0 +1,68 @@ +--- +description: Tighten comment and commit message wording in the working diff +--- + +Review the wording of comments and commit messages on this branch and tighten anything that +breaks the rules. This is a **wording-only** pass - do not change behaviour, rename anything, +or restructure code. For code quality use `/simplify`, for bugs use `/code-review`. + +## The rules being checked + +Comment rules: the "Code Comments" section of `CLAUDE.md`. +Commit message rules: `.claude/commands/commit.md`. + +The single test both share: **would this still make sense to someone reading it in a year, who +never saw this branch, the PR, the ticket, or the conversation that produced it?** + +## Process + +1. **Collect the changes** + + ``` + git diff + git diff --cached + git log master..HEAD --format='%H%n%s%n%n%b%n---' + ``` + + If the branch has no commits and no working changes, say so and stop. + +2. **Check added and modified comments.** Only lines this branch touched - read the diff, not + whole files. Flag: + + - comments that restate what the code does + - comments longer than they need to be, or split over several lines for no reason + - references to the change itself: `now`, `new`, `previously`, `used to`, `this change`, + `the fix`, `as discussed`, `per review`, `we decided`, `recently` + - narration of the diff: `// added onKeyDown handler`, `// updated to support X` + - trailing comments on the same line as code, and comments below what they describe + - commented-out code, decorative separators, banner comments + - prop JSDoc carrying `@param`/`@type` tags, or running longer than a sentence + - **comments added to code the branch did not otherwise change** + +3. **Check commit messages** against the `/commit` rules: subject over 72 characters, a body + that restates the diff, a changelog shape (several grouping headings, a long bullet list, + bullets enumerating changed files), or a `πŸ€– Generated with` line. + + Do **not** flag before/after wording here. "Previously the placeholder only showed on + hover" is correct in a commit message β€” the message is permanently attached to its own + diff. That rule exists for comments, which persist with no such anchor. + +4. **Report before changing anything.** One table or list, grouped into comments and commit + messages, each entry `file:line` (or the commit's short sha) with the current text and the + proposed replacement. Say plainly if there is nothing to fix. + +5. **Apply on confirmation.** + + - Comment rewrites: edit the files directly. Leave the working tree staged as you found it. + - Commit message rewrites: these rewrite history, so **always confirm separately** and tell + the user it will change the shas. Amend with `git commit --amend` for `HEAD` only; for + older commits, explain that a rebase is needed and let the user decide whether it is + worth it. **Never rewrite history without an explicit go-ahead**, and never on a branch + that has been pushed and reviewed unless the user says so. + +## Important + +- Deleting a comment is usually the right fix. Do not rewrite a comment that should not exist. +- Do not touch the MIT license header block at the top of every file. +- Do not add new comments. This pass only shortens and removes. +- Do not touch comments outside the diff, however tempting. diff --git a/.claude/settings.json b/.claude/settings.json index 241daa48f9..6add3906ff 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -37,5 +37,18 @@ "Bash(gh --help)", "Bash(gh * --help)" ] + }, + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "node \"$CLAUDE_PROJECT_DIR/scripts/claude/check-commit-message.mjs\"" + } + ] + } + ] } } diff --git a/.gitignore b/.gitignore index ea10d82664..5445ac6859 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,7 @@ CLAUDE.local.md !.claude/commands/slack-setup.md !.claude/commands/implement.md !.claude/commands/ticket.md +!.claude/commands/tidy.md # Playwright MCP .playwright-mcp diff --git a/CLAUDE.md b/CLAUDE.md index 9013f8b8a7..3f0ed8ae8c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,6 +17,31 @@ External docs (preferred over guessing component APIs): https://instructure.desi - **New components: functional + hooks only.** Class components exist in legacy code β€” don't extend that pattern. - Styling is Emotion CSS-in-JS via `theme.ts` files co-located with each component. +## Code comments + +**A comment must make sense to someone reading the file in a year who never saw the change that introduced it** β€” no access to the PR, the ticket, or the conversation. The same standard applies to commit messages; `/commit` has the specifics. + +- Explain **why**, never **what**. If the code already says it, delete the comment. +- **One line.** Two or three only when the reason genuinely needs them. +- **Never reference the change itself.** `now`, `new`, `previously`, `used to`, `this change`, `the fix`, `as discussed`, `per review`, `we decided`, `recently` β€” these only mean something next to the diff. Name the constraint instead. +- **Never narrate the diff** (`// added onKeyDown handler`, `// updated to support X`) β€” that's what `git log` is for. +- No commented-out code, no banner or separator comments, and **don't add comments to code you didn't change**. +- Lowercase `//` on its own line above what it explains, never trailing. Ticket ids only on a real external blocker: `// TODO INSTUI-1234: `. +- Leave the MIT license header alone β€” `notice/notice` in `eslint.config.mjs` enforces it. + +```ts +// ❌ verbose, references the change, restates the code +// We now memoize this because we found a performance issue during testing +// where the component re-rendered too often. Previously computed inline. +const styles = useMemo(...) + +// βœ… names the constraint, reads standalone +// getCSSStyleDeclaration costs ~100ms per call +const styles = useMemo(...) +``` + +Prop docs are a JSDoc block with **one prose sentence** and no `@param`/`@type` β€” types come from TypeScript and `react-docgen`. See `packages/ui-alerts/src/Alert/props.ts`. + ## Component versioning (v1/v2) Some components ship in two versions during a migration period β€” a legacy **v1** and a newer **v2** (e.g. `DateInput`). v2 is the preferred implementation for new work; v1 is deprecated and gets removed in a later major release. Don't assume a component has only one version: check its README and the package exports to see which versions exist and which is current before using or changing one. diff --git a/commitlint.config.js b/commitlint.config.js index a62ba2912b..8f19b5746e 100644 --- a/commitlint.config.js +++ b/commitlint.config.js @@ -64,16 +64,94 @@ function getAllPackages() { } } +// Trailers and ticket ids carry no prose, so they don't count towards the body +// limits. +const isTrailer = (line) => + /^[A-Za-z][A-Za-z-]*:\s/.test(line) || + /^[A-Z][A-Z0-9]+-\d+$/.test(line) || + line.startsWith('πŸ€–') + +function bodyLines(raw) { + return (raw || '') + .split('\n') + .slice(1) + .map((line) => line.trim()) + .filter((line) => line !== '' && !line.startsWith('#') && !isTrailer(line)) +} + +/** + * Caps body length. Set well above the longest real commit so it only catches a + * body that has turned into a full changelog of the diff. + */ +function bodyMaxLines(parsed, _when, max) { + const count = bodyLines(parsed.raw).length + return [ + count <= max, + `body has ${count} lines, the limit is ${max}. Explain why the change was ` + + 'made; the diff already covers what changed.' + ] +} + +/** + * Rejects a body shaped like a changelog: grouped under several headings, or a + * long bullet list naming the files that changed. Thresholds sit above the + * heaviest legitimate usage in this repo - one "The fixes:" style lead-in and a + * handful of bullets are fine. + */ +function bodyNoChangelog(parsed) { + const lines = bodyLines(parsed.raw) + const headings = lines.filter((line) => + /^[A-Z][A-Za-z /()]{2,40}:$/.test(line) + ) + const bullets = lines.filter((line) => /^[-*] /.test(line)) + const pathBullets = bullets.filter((line) => + /(packages\/|scripts\/|\.(ts|tsx|js|jsx|mjs|cjs|json|ya?ml|md)\b)/.test( + line + ) + ) + + if (headings.length > 1) { + return [ + false, + `body groups changes under ${headings.length} headings (${headings.join( + ' ' + )}). ` + 'Write prose explaining why, not a grouped changelog.' + ] + } + if (bullets.length > 12) { + return [ + false, + `body has ${bullets.length} bullets. Summarise the reason for the change instead.` + ] + } + if (pathBullets.length > 6) { + return [ + false, + `body lists ${pathBullets.length} changed files. The diff already lists them.` + ] + } + return [true, ''] +} + module.exports = { extends: ['@commitlint/config-conventional'], - parserOpts: { - headerPattern: /^(\w*)\((\w*)\)-(\w*)\s(.*)$/, - headerCorrespondence: ['type', 'scope', 'subject'] - }, + plugins: [ + { + rules: { + 'body-max-lines': bodyMaxLines, + 'body-no-changelog': bodyNoChangelog + } + } + ], // https://commitlint.js.org/reference/rules.html rules: { + // The header is unbounded because multi-package scopes are long, e.g. + // `fix(ui-drawer-layout,ui-a11y-utils):`. The subject itself is capped. 'header-max-length': [0, 'always', 150], // 0 === rule is disabled - 'subject-max-length': [2, 'always', 150] + 'subject-max-length': [2, 'always', 72], + 'body-max-line-length': [2, 'always', 100], + 'body-max-lines': [2, 'always', 28], + 'body-no-changelog': [2, 'always'] }, // https://cz-git.qbb.sh/config/ diff --git a/docs/contributing/contributing-getting-started.md b/docs/contributing/contributing-getting-started.md index b968fa50c5..555593e428 100644 --- a/docs/contributing/contributing-getting-started.md +++ b/docs/contributing/contributing-getting-started.md @@ -71,6 +71,18 @@ Please update the documentation and examples with any changes. - Write documentation inline in code comment blocks. The code and docs should always be in sync. +### Code Comments + +Write comments for someone reading the file a year from now, with no access to +the pull request or ticket that introduced the change. + +- Explain _why_, not _what_. If the code already says it, leave the comment out. +- Keep it to one line where you can. +- Don't refer to the change itself ("we now…", "previously…", "this fix…") or + narrate the diff ("added onKeyDown handler") - `git log` covers that. +- Document props with a JSDoc block containing one prose sentence. Types are + parsed from TypeScript, so `@param` and `@type` tags aren't needed. + ### Commit Guidelines Run `git commit` to commit your changes and follow our commit message format. diff --git a/scripts/claude/__node_tests__/check-commit-message.test.ts b/scripts/claude/__node_tests__/check-commit-message.test.ts new file mode 100644 index 0000000000..da704c443e --- /dev/null +++ b/scripts/claude/__node_tests__/check-commit-message.test.ts @@ -0,0 +1,235 @@ +/* + * The MIT License (MIT) + * + * Copyright (c) 2015 - present Instructure, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +import { spawnSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' + +const HOOK = fileURLToPath( + new URL('../check-commit-message.mjs', import.meta.url) +) + +// built by concatenation so this file's own text does not read as a commit +// invocation to the hook it is testing +const GC = 'git ' + 'commit' + +const heredoc = (message: string) => + `${GC} -m "$(cat <<'EOF'\n${message}\nEOF\n)"` + +function runHook(command: string) { + const payload = JSON.stringify({ + tool_name: 'Bash', + tool_input: { command } + }) + const result = spawnSync('node', [HOOK], { input: payload, encoding: 'utf8' }) + return { status: result.status, stderr: result.stderr } +} + +describe('check-commit-message hook', () => { + describe('allows', () => { + it('a subject-only commit', () => { + expect( + runHook(`${GC} -m "fix(ui-select): keep highlight when options change"`) + .status + ).toBe(0) + }) + + it('a long prose body explaining a subtle cause', () => { + // shaped after 9b0467db66, a genuinely good 11-line body + const message = [ + 'fix(ui-dialog): cancel the scheduled focus activation on close', + '', + 'Dialog activates its FocusRegion in a requestAnimationFrame callback. When', + 'the Dialog closed before that frame ran, close() found no region to blur and', + 'left the frame scheduled. The callback then activated a region for an already', + 'closed Dialog, which nothing ever blurred.', + '', + 'Co-Authored-By: Claude ' + ].join('\n') + + expect(runHook(heredoc(message)).status).toBe(0) + }) + + it('a single lead-in heading followed by a few bullets', () => { + // shaped after ba6f9459d8 / 7e40bc5a2c, which use "The fixes:" as a lead-in + const message = [ + 'test(many): fix tests after the vitest-browser conversion', + '', + 'A real browser does layout, hit-testing and focus, and a fair number of', + 'tests relied on jsdom not doing any of that.', + '', + 'The fixes:', + '- userEvent.click(..., { force: true }) in ~36 places', + '- await expect.element() instead of a bare assertion', + '- drop the manual act() wrappers' + ].join('\n') + + expect(runHook(heredoc(message)).status).toBe(0) + }) + + it('a handful of bullets naming specific files', () => { + // shaped after ea28383285, where naming the files is the point + const message = [ + 'fix(many): update eslint-disable comments for oxlint rule ids', + '', + 'Two disable comments reference rule names that do not match oxlint, so', + 'they silently stopped suppressing anything:', + '- packages/__docs__/globals.ts: no @ scope prefix on the custom rule', + '- packages/ui-table/src/Table.test.tsx: stale directive' + ].join('\n') + + expect(runHook(heredoc(message)).status).toBe(0) + }) + + it('a BREAKING CHANGE note', () => { + const message = [ + 'feat(ui-select)!: remove deprecated onOpen prop', + '', + 'BREAKING CHANGE: onOpen has been removed, use onShowOptions instead.' + ].join('\n') + + expect(runHook(heredoc(message)).status).toBe(0) + }) + + it('a commit chained after another command', () => { + expect(runHook(`git add . && ${GC} -m "chore: bump deps"`).status).toBe(0) + }) + }) + + describe('ignores commands that are not a commit', () => { + it.each([ + ['git log', 'git log -n 5 --oneline'], + ['an unrelated command', 'pnpm run test:vitest'], + ['a command that only mentions committing', `echo "run ${GC} to save"`] + ])('%s', (_name, command) => { + expect(runHook(command).status).toBe(0) + }) + }) + + describe('blocks', () => { + it('an attempt to skip the hooks with HUSKY=0', () => { + const { status, stderr } = runHook( + `HUSKY=0 ${GC} -m "fix(ui-select): ok"` + ) + expect(status).toBe(2) + expect(stderr).toContain('Do not skip the git hooks') + }) + + it('an attempt to skip the hooks with --no-verify', () => { + expect(runHook(`${GC} --no-verify -m "fix(ui-select): ok"`).status).toBe( + 2 + ) + }) + + it('a subject over 72 characters', () => { + const { status, stderr } = runHook( + `${GC} -m "feat(many): add margin prop to v2 FormFieldGroup, CheckboxGroup, RadioInputGroup, Checkbox, RadioInput, Text, and ToggleButton"` + ) + expect(status).toBe(2) + expect(stderr).toContain('the limit is 72') + }) + + it('a body grouped under several changelog headings', () => { + // shaped after f7bb16e114, the 30-line pnpm migration changelog + const message = [ + 'feat(many): migrate from npm to pnpm', + '', + 'Configuration:', + '- add pnpm-workspace.yaml', + '', + 'Build Tooling:', + '- update scripts/bootstrap.js', + '', + 'Documentation:', + '- update npm references' + ].join('\n') + + const { status, stderr } = runHook(heredoc(message)) + expect(status).toBe(2) + expect(stderr).toContain('groups changes under') + }) + + it('a body that is a long bullet list', () => { + const bullets = Array.from( + { length: 14 }, + (_, i) => `- change number ${i}` + ) + const message = ['feat(many): migrate to pnpm', '', ...bullets].join('\n') + + const { status, stderr } = runHook(heredoc(message)) + expect(status).toBe(2) + expect(stderr).toContain('bullets') + }) + + it('a body enumerating many changed files', () => { + const bullets = Array.from( + { length: 8 }, + (_, i) => `- update packages/ui-thing-${i}/src/index.ts` + ) + const message = ['feat(many): migrate to pnpm', '', ...bullets].join('\n') + + const { status, stderr } = runHook(heredoc(message)) + expect(status).toBe(2) + expect(stderr).toContain('changed files') + }) + + it('a body over 28 lines', () => { + const lines = Array.from({ length: 30 }, (_, i) => `prose line ${i}`) + const message = ['feat(many): migrate to pnpm', '', ...lines].join('\n') + + const { status, stderr } = runHook(heredoc(message)) + expect(status).toBe(2) + expect(stderr).toContain('the limit is 28') + }) + + it('the robot attribution line, which belongs in PR bodies only', () => { + const message = [ + 'chore: tweak docs', + '', + 'πŸ€– Generated with [Claude Code](https://claude.com/claude-code)' + ].join('\n') + + expect(runHook(heredoc(message)).status).toBe(2) + }) + + it('a body line over 100 characters', () => { + const message = [ + 'fix(ui-view): memoize style lookup', + '', + 'x'.repeat(120) + ].join('\n') + + expect(runHook(heredoc(message)).status).toBe(2) + }) + }) + + describe('fails open', () => { + it.each([ + ['on malformed json', 'not json'], + ['on empty input', ''] + ])('%s', (_name, input) => { + const result = spawnSync('node', [HOOK], { input, encoding: 'utf8' }) + expect(result.status).toBe(0) + }) + }) +}) diff --git a/scripts/claude/check-commit-message.mjs b/scripts/claude/check-commit-message.mjs new file mode 100644 index 0000000000..e087c3febe --- /dev/null +++ b/scripts/claude/check-commit-message.mjs @@ -0,0 +1,198 @@ +/* + * The MIT License (MIT) + * + * Copyright (c) 2015 - present Instructure, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/* + * PreToolUse hook. Rejects a `git commit` whose message breaks the rules in + * .claude/commands/commit.md, so the model gets the reason before the commit is + * attempted rather than as a commit-msg hook failure afterwards. + * + * Thresholds match commitlint.config.js and are calibrated against this repo's + * history: they sit above the heaviest legitimate commit and below the + * changelog-shaped ones. + * + * Exit 0 to allow, exit 2 to block and send the reasons back to the model. + * Anything it cannot parse is allowed through - a commit must never fail + * because a regex did not match. + */ + +const SUBJECT_MAX = 72 +const BODY_MAX_LINES = 28 +const BODY_MAX_LINE_LENGTH = 100 +const MAX_HEADINGS = 1 +const MAX_BULLETS = 12 +const MAX_PATH_BULLETS = 6 + +// `git commit` at a command position: start of the string, or after a +// separator, optionally preceded by env assignments and git's own flags. +// Anchored so a command that merely quotes "git commit" as data is left alone. +const COMMIT_INVOCATION = + /(?:^|[\n;&|(]|&&|\|\|)\s*(?:[A-Za-z_][A-Za-z0-9_]*=\S*\s+)*git\s+(?:-\S+\s+)*commit\b/ + +const isTrailer = (line) => + /^[A-Za-z][A-Za-z-]*:\s/.test(line) || + /^[A-Z][A-Z0-9]+-\d+$/.test(line) || + line.startsWith('πŸ€–') + +function readStdin() { + return new Promise((resolve) => { + let data = '' + process.stdin.setEncoding('utf8') + process.stdin.on('data', (chunk) => (data += chunk)) + process.stdin.on('end', () => resolve(data)) + process.stdin.on('error', () => resolve('')) + }) +} + +// Pulls the commit message out of a shell command, handling both the heredoc +// form `/commit` uses and plain -m flags. +function extractMessage(command) { + const heredoc = command.match( + /<<-?\s*['"]?(\w+)['"]?\r?\n([\s\S]*?)\r?\n\1\b/ + ) + if (heredoc) { + return heredoc[2] + } + + const paragraphs = [] + const flag = /(?:^|\s)-m\s*(?:=\s*)?(['"])([\s\S]*?)\1/g + let match + while ((match = flag.exec(command)) !== null) { + // a -m whose value is a command substitution is not a literal message + if (!match[2].includes('$(')) { + paragraphs.push(match[2]) + } + } + return paragraphs.length > 0 ? paragraphs.join('\n\n') : null +} + +function checkMessage(message) { + const problems = [] + const all = message.split('\n').filter((line) => !line.startsWith('#')) + const subject = (all[0] || '').trim() + + if (subject.length > SUBJECT_MAX) { + problems.push( + `Subject is ${subject.length} characters, the limit is ${SUBJECT_MAX}. ` + + 'Name the single change being made; drop the enumeration of everything it touches.' + ) + } + + const body = all + .slice(1) + .map((line) => line.trim()) + .filter((line) => line !== '' && !isTrailer(line)) + + if (body.length > BODY_MAX_LINES) { + problems.push( + `Body has ${body.length} lines, the limit is ${BODY_MAX_LINES}. Explain ` + + 'why the change was made; the diff already covers what changed.' + ) + } + + const tooLong = body.find((line) => line.length > BODY_MAX_LINE_LENGTH) + if (tooLong) { + problems.push( + `A body line is ${tooLong.length} characters, the limit is ` + + `${BODY_MAX_LINE_LENGTH}. Hard-wrap the body at 100 columns.` + ) + } + + const headings = body.filter((line) => + /^[A-Z][A-Za-z /()]{2,40}:$/.test(line) + ) + if (headings.length > MAX_HEADINGS) { + problems.push( + `Body groups changes under ${headings.length} headings ` + + `(${headings.join( + ' ' + )}). Write prose explaining why, not a grouped changelog.` + ) + } + + const bullets = body.filter((line) => /^[-*] /.test(line)) + if (bullets.length > MAX_BULLETS) { + problems.push( + `Body has ${bullets.length} bullets. Summarise the reason for the change instead.` + ) + } + + const pathBullets = bullets.filter((line) => + /(packages\/|scripts\/|\.(ts|tsx|js|jsx|mjs|cjs|json|ya?ml|md)\b)/.test( + line + ) + ) + if (pathBullets.length > MAX_PATH_BULLETS) { + problems.push( + `Body lists ${pathBullets.length} changed files. The diff already lists them.` + ) + } + + if (message.includes('πŸ€– Generated with')) { + problems.push( + 'Drop the "πŸ€– Generated with [Claude Code]" line from commit messages. ' + + 'Keep only the Co-Authored-By trailer; the πŸ€– line belongs in PR bodies.' + ) + } + + return problems +} + +const input = await readStdin() + +let command +try { + command = JSON.parse(input)?.tool_input?.command +} catch { + process.exit(0) +} + +if (typeof command !== 'string' || !COMMIT_INVOCATION.test(command)) { + process.exit(0) +} + +const problems = [] + +if (/\bHUSKY=0\b/.test(command) || /--no-verify\b/.test(command)) { + problems.push( + 'Do not skip the git hooks (HUSKY=0, --no-verify). A -m commit is already ' + + 'non-interactive, and commit-msg runs commitlint. Fix the cause instead of ' + + 'bypassing the hook.' + ) +} + +const message = extractMessage(command) +if (message !== null) { + problems.push(...checkMessage(message)) +} + +if (problems.length > 0) { + console.error( + 'Commit message rejected by scripts/claude/check-commit-message.mjs:\n\n' + + problems.map((problem) => `- ${problem}`).join('\n') + + '\n\nSee .claude/commands/commit.md for the rules. Fix the message and retry.' + ) + process.exit(2) +} + +process.exit(0)