diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d472a76..d8b4076 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -169,6 +169,14 @@ Developer Certificate of Origin, reproduced in full in the pull request template, and only the author can certify it -- an assistant discloses itself with `Assisted-by:` and signs nothing. `git commit -s` writes the line for you. +`Co-authored-by:` is for people, and the check refuses one naming an assistant +or a bot account. Authorship is a claim only a person can make, and an agent +writing its own commit message reaches for that trailer by habit. It refuses on +the `[bot]` suffix, which GitHub reserves so that no person can hold it, on the +addresses the agents commit under, and on a handful of product names. Only the +last can reach a person. If it ever refuses a real co-author, narrow the pattern +in the same pull request rather than dropping the credit. + `pnpm run lint:commits` holds every commit on your branch to all of this, and cross-checks its own reading of the trailers against `git interpret-trailers`, so the rules cannot quietly drift from the tool they describe. Commits written diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index cdbe172..fa38462 100644 --- a/build/shared/commit-message.mts +++ b/build/shared/commit-message.mts @@ -119,6 +119,28 @@ const TRAILER_LINE = /^(?[A-Za-z][\w-]*):[ \t]*(?.*)$/; */ const ASSISTED_BY_VALUE = /^[^\s:]+:\S+( \S+)*$/; +/** + * A `Co-authored-by` value naming something that is not a person. Authorship + * is a claim only a person can make: the Developer Certificate of Origin is + * certified by whoever wrote the code, and a tool certifies nothing. An + * assistant is disclosed with `Assisted-by` instead, which says what was used + * rather than who wrote it. + * + * `[bot]` cannot catch a person: GitHub reserves the suffix and no account may + * be named with it, which is the same fact the commit checker relies on to + * recognize a bot author. The agents' noreply addresses are theirs alone. Only + * the product names can reach a person, and only one of them realistically: + * Claude is a name people have. That is the trade accepted here, since the + * alternative is a tool standing in the history as an author. If it ever + * refuses a real co-author, narrow the pattern rather than drop the credit. + * + * An agent whose integration commits as a `[bot]` account needs no name here. + * Renovate and Dependabot are out of reach either way: their commits are + * skipped whole, so their own `[bot]` co-authors are not this check's business. + */ +const TOOL_COAUTHOR = + /\[bot]|\bnoreply@(?:anthropic|openai)\.com\b|\b(?:aider|chatgpt|claude|codex|copilot|cursor)\b/i; + /** git folds a trailer whose value runs onto an indented line beneath it. */ export const CONTINUATION_LINE = /^\s/; @@ -199,7 +221,25 @@ const trailerBlockOf = (paragraphs: string[][]) => { (line) => TRAILER_LINE.test(line) || CONTINUATION_LINE.test(line) ); - return isBlock ? last.filter((line) => !CONTINUATION_LINE.test(line)) : []; + // A folded trailer is one trailer, not a trailer and a stray line. git + // joins the indented line onto the value, and `git interpret-trailers + // --parse` prints the two back as one, so this does the same. Dropping the + // continuation instead would put whatever got wrapped out of reach of every + // check below -- an address most of all, which is the half that says who a + // trailer names. + return isBlock + ? last.reduce((folded, line) => { + const previous = folded.at(-1); + + if (previous !== undefined && CONTINUATION_LINE.test(line)) { + folded[folded.length - 1] = `${previous} ${line.trim()}`; + } else { + folded.push(line); + } + + return folded; + }, []) + : []; }; /** @@ -371,6 +411,15 @@ const checkTrailers = (lines: string[]) => { ); } + if ( + token.toLowerCase() === 'co-authored-by' && + TOOL_COAUTHOR.test(found?.value ?? '') + ) { + problems.push( + `“Co-authored-by: ${found?.value}” credits a tool with authorship: an assistant is disclosed with “Assisted-by:” and co-authors nothing` + ); + } + // 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. @@ -423,12 +472,12 @@ export function validateCommitMessage(message: string) { problems.push('the line after the subject has to be blank'); } - // Where the trailer block starts, or past the end when there is none. A - // trailer is one line by construction here: git would read a folded value, - // but `readTrailers` keeps only the token line, so wrapping a long - // `Signed-off-by:` to fit would put the author out of reach of the sign-off - // check. The block is exempt from the width limit rather than made to fit - // inside it, which is also what the limit is for -- prose that is read. + // Where the trailer block starts, or past the end when there is none. The + // block is exempt from the width limit rather than made to fit inside it, + // which is what the limit is for -- prose that is read. A trailer long + // enough to need wrapping is wrapped by nobody here, and one that arrives + // wrapped anyway is read whole, since `trailerBlockOf` folds it back the + // way git does. const paragraphs = paragraphsOf(rest); const blockStart = trailerBlockOf(paragraphs).length > 0 diff --git a/build/shared/commit-message.test.mts b/build/shared/commit-message.test.mts index 53ca224..f3ce70e 100644 --- a/build/shared/commit-message.test.mts +++ b/build/shared/commit-message.test.mts @@ -14,6 +14,7 @@ import { ACTIONS, BODY_MAX, CATEGORIES, + checkSignOff, readTrailers, validateCommitMessage, } from './commit-message.mts'; @@ -250,6 +251,92 @@ describe('validateCommitMessage: the trailers', () => { ); }); + test('accepts a person as a co-author', () => { + deepStrictEqual( + validateCommitMessage( + '🏗️🔧：fix it\n\nCo-authored-by: Ada Lovelace ' + ), + [] + ); + }); + + test('rejects an assistant credited as a co-author', () => { + // The mistake this exists to stop. An assistant is disclosed with + // `Assisted-by`, and putting it here instead claims it wrote the code and + // certified the Developer Certificate of Origin, neither of which a tool + // can do. + match( + soleProblem( + '🏗️🔧：fix it\n\nCo-authored-by: Claude Opus 5 \nSigned-off-by: Ada Lovelace ' + ), + /credits a tool with authorship/ + ); + }); + + test('rejects an assistant co-author with no address to give it away', () => { + match( + soleProblem('🏗️🔧：fix it\n\nCo-authored-by: GitHub Copilot'), + /credits a tool with authorship/ + ); + }); + + test('rejects a bot account as a co-author', () => { + match( + soleProblem( + '🏗️🔧：fix it\n\nCo-authored-by: some-app[bot] <1234+some-app[bot]@users.noreply.github.com>' + ), + /credits a tool with authorship/ + ); + }); + + test('leaves a person at one of those companies alone', () => { + // Only the agents' `noreply` address is theirs. Somebody who works there + // and writes part of a change is a co-author like anybody else. + deepStrictEqual( + validateCommitMessage( + '🏗️🔧：fix it\n\nCo-authored-by: Ada Lovelace ' + ), + [] + ); + }); + + test('leaves a person whose name an agent also goes by alone', () => { + // `devin` and `gemini` are names before they are products, and both of + // those integrations commit as `[bot]` accounts anyway, so neither has to + // be named here. `claude` is the one kept, and the one that can misfire. + deepStrictEqual( + validateCommitMessage( + '🏗️🔧：fix it\n\nCo-authored-by: Devin Gemini ' + ), + [] + ); + }); + + test('rejects an assistant whose address is on a folded line', () => { + // git joins the indented line onto the value and reads one trailer, so a + // wrapped address is still the address. Keeping only the token line would + // have made folding a way around the check. + match( + soleProblem( + '🏗️🔧：fix it\n\nCo-authored-by: Some Person\n ' + ), + /credits a tool with authorship/ + ); + }); + + test('reads a folded sign-off as the whole address', () => { + // The same fold, on the trailer whose value the sign-off check compares + // against the author. Dropping the continuation put the author out of + // reach and the sign-off passed for nobody. + deepStrictEqual( + checkSignOff( + '🏗️🔧：fix it\n\nSigned-off-by: Ada Lovelace\n ', + 'Ada Lovelace ' + ), + [] + ); + }); + test('rejects a token this project does not use', () => { match( soleProblem('🏗️🔧：fix it\n\nCloses: https://x/1'), @@ -372,8 +459,9 @@ describe('the vocabulary', () => { describe('the width limit', () => { test('leaves a trailer that cannot be wrapped alone', () => { - // Folding this to fit would put the author on a continuation line, where - // `readTrailers` does not look and so `checkSignOff` could not match it. + // Trailers are metadata rather than prose, and the limit is for prose. + // Folding this one to fit would be read correctly either way, but there + // is no reason to make somebody wrap an address to please a linter. const long = 'Signed-off-by: Christopher Alexander Montgomery ' + '';