diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d92cede..7c7d4d12 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,7 +138,14 @@ _you_ on every commit it rewrites, so reach for it only where you are the author of all of them. A commit somebody else wrote has to be signed off by them. An assistant that helped is disclosed separately, with an `Assisted-by:` -trailer, and signs nothing. +trailer, and signs nothing. It does not go in `Co-authored-by:` either, which 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. ## Developer’s Certificate of Origin 1.1 diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index cdbe1726..fa384626 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 aa282392..63974734 100644 --- a/build/shared/commit-message.test.mts +++ b/build/shared/commit-message.test.mts @@ -13,6 +13,7 @@ import { ACTIONS, BODY_MAX, CATEGORIES, + checkSignOff, readTrailers, validateCommitMessage, } from '@openinf/.github/build/commit-message'; @@ -247,6 +248,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'), @@ -369,8 +456,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 ' + '';