From ad91fe97e20cd10119f8a74cf0f2e99c6d2f33cf Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Thu, 10 Sep 2026 06:09:59 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9Afix?= =?UTF-8?q?=20four=20faults=20in=20landing=20a=20pull=20request?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Porting this machinery into OpenINF/sdk put it under review again, and four things came out of that. All of them are here as well, which is where they were written, so they are fixed here first. A branch of more than thirty commits could not land at all. The commits are asked of the API with `--paginate` and a `--jq` filter, and those two together filter each page and print the results one after another, so anything past the first page handed `JSON.parse` several arrays in a row. What came back was a parse error rather than a reason. `--slurp` gathers the pages into one array instead, and the filtering that `--jq` was doing happens in the task. The 72-column limit was applied to trailers, which cannot be wrapped to meet it: git would read a folded value, but `readTrailers` keeps only the token line, so folding a long `Signed-off-by:` moves the author out of reach of `checkSignOff` and fails a different way. Anyone whose name and address ran past 57 characters could not write a commit this would accept. The trailer block is exempt now; prose is not, including a last paragraph that only looks like trailers and so holds no trailers at all. A folded trailer was split while a landing message was composed. Each line was read on its own, so the token line went to the trailers and the indented continuation stayed in the body: `Co-authored-by:` written over two lines landed with the address left behind. The message still validated, so the queue merged it and the attribution was quietly lost. A continuation is attached to the trailer above it now, rather than pushed as an entry of its own, which would have let the sort move it away from what it belongs to. The pull request template told contributors to paste the emoji into the description. It belongs in the title, which is the subject that lands and the only place anything reads it. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- build/shared/commit-message.mts | 22 ++++++++++++-- build/shared/commit-message.test.mts | 33 ++++++++++++++++++++ build/shared/landing.mts | 27 +++++++++++++++-- build/shared/landing.test.mts | 45 ++++++++++++++++++++++++++++ build/tasks/land-pull-request.mts | 28 +++++++++++------ 6 files changed, 141 insertions(+), 16 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 87b695bab..1db3bb824 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -40,7 +40,7 @@ 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 +_Copy and paste one of the following emoji into the title_ -- copy rather than type, since some have a lookalike spelling that is not the one recognized here. diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index 1ee7b90e4..cdbe1726f 100644 --- a/build/shared/commit-message.mts +++ b/build/shared/commit-message.mts @@ -120,7 +120,7 @@ const TRAILER_LINE = /^(?[A-Za-z][\w-]*):[ \t]*(?.*)$/; const ASSISTED_BY_VALUE = /^[^\s:]+:\S+( \S+)*$/; /** git folds a trailer whose value runs onto an indented line beneath it. */ -const CONTINUATION_LINE = /^\s/; +export const CONTINUATION_LINE = /^\s/; /** * Each known token with the pattern that also matches it misspelt with a @@ -423,7 +423,19 @@ export function validateCommitMessage(message: string) { problems.push('the line after the subject has to be blank'); } - for (const line of rest) { + // 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. + const paragraphs = paragraphsOf(rest); + const blockStart = + trailerBlockOf(paragraphs).length > 0 + ? rest.length - (paragraphs.at(-1)?.length ?? 0) + : rest.length; + + for (const [index, line] of rest.entries()) { // 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. @@ -439,7 +451,11 @@ export function validateCommitMessage(message: string) { // No grapheme is fewer than one code unit, so a line of BODY_MAX code // units or fewer is within the limit whatever it is made of, and the // segmenter need not see it. - if (line.length > BODY_MAX && /\s/.test(line.trim())) { + if ( + index < blockStart && + line.length > BODY_MAX && + /\s/.test(line.trim()) + ) { const width = countGraphemes(line); if (width > BODY_MAX) { diff --git a/build/shared/commit-message.test.mts b/build/shared/commit-message.test.mts index 67ec3ea17..44fd73dd6 100644 --- a/build/shared/commit-message.test.mts +++ b/build/shared/commit-message.test.mts @@ -11,6 +11,7 @@ import { readFile } from 'node:fs/promises'; import { describe, test } from 'node:test'; import { ACTIONS, + BODY_MAX, CATEGORIES, readTrailers, validateCommitMessage, @@ -366,6 +367,38 @@ 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. + const long = + 'Signed-off-by: Christopher Alexander Montgomery ' + + ''; + ok(long.length > BODY_MAX); + deepStrictEqual( + validateCommitMessage(`🏗️🔧：fix it\n\nA body line.\n\n${long}`), + [] + ); + }); + + test('still holds prose to it, in the same message', () => { + const problems = validateCommitMessage( + `🏗️🔧：fix it\n\n${'word '.repeat(20)}end\n\n` + + 'Signed-off-by: Derek Lewis ' + ); + deepStrictEqual(problems.length, 1); + match(problems[0] ?? '', /the limit is 72/); + }); + + test('holds a long line in a paragraph that only looks like trailers', () => { + // Not a block, so git reads no trailers in it and it is prose after all. + const problems = validateCommitMessage( + `🏗️🔧：fix it\n\nSigned-off-by: Derek Lewis \n${'word '.repeat(20)}end` + ); + ok(problems.some((problem) => /the limit is 72/.test(problem))); + }); +}); + describe('a message written to be slow', () => { // A commit message comes from whoever opened the pull request, and the // commit queue reads it holding credentials that can write here. How long diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 2017880fb..3fc414449 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -11,7 +11,11 @@ * the repositories in this organization do not agree on that yet. */ -import { linesOf, TRAILER_ORDER } from '@openinf/portal/build/commit-message'; +import { + CONTINUATION_LINE, + linesOf, + TRAILER_ORDER, +} from '@openinf/portal/build/commit-message'; /** One commit's message, split into the parts a landed message reuses. */ export type CommitParts = { @@ -34,9 +38,26 @@ export function partsOfMessage(message: string): CommitParts { const body: string[] = []; const trailers: string[] = []; + // A folded trailer travels with the line it belongs to. Read on its own an + // indented continuation carries no token, so it would be filed as body and + // stranded there -- and `Co-authored-by:` folded over two lines would land + // with the address left behind, which is attribution quietly lost. + let inTrailer = false; + for (const line of rest) { - if (TRAILER_ORDER.includes(tokenOf(line))) trailers.push(line); - else body.push(line); + if (TRAILER_ORDER.includes(tokenOf(line))) { + trailers.push(line); + inTrailer = true; + } else if ( + inTrailer && + CONTINUATION_LINE.test(line) && + line.trim() !== '' + ) { + trailers[trailers.length - 1] += `\n${line}`; + } else { + body.push(line); + inTrailer = false; + } } while (body.at(-1)?.trim() === '') body.pop(); diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index e1fb0d25e..d82b4b8af 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -344,3 +344,48 @@ describe('checksVerdict and its own run', () => { ); }); }); + +describe('a folded trailer', () => { + const folded = [ + '🏗️🔧：fix it', + '', + 'A body line.', + '', + 'Co-authored-by: Jane Doe', + ' ', + 'Signed-off-by: Derek Lewis ', + ].join('\n'); + + test('is one trailer, not a trailer and a stranded line', () => { + const parts = partsOfMessage(folded); + deepStrictEqual(parts.body, ['A body line.']); + deepStrictEqual(parts.trailers, [ + 'Co-authored-by: Jane Doe\n ', + 'Signed-off-by: Derek Lewis ', + ]); + }); + + test('lands with its value still attached to it', () => { + const message = composeLandingMessage( + [partsOfMessage(folded)], + 'https://example.com/pull/1' + ); + deepStrictEqual(message.split('\n'), [ + 'A body line.', + '', + 'Co-authored-by: Jane Doe', + ' ', + 'Signed-off-by: Derek Lewis ', + 'PR-URL: https://example.com/pull/1', + ]); + deepStrictEqual(validateCommitMessage(`🏗️🔧：fix it\n\n${message}`), []); + }); + + test('does not swallow an indented body line after prose', () => { + const parts = partsOfMessage( + '🏗️🔧：fix it\n\nA body line.\n an indented one\n\nPR-URL: x' + ); + deepStrictEqual(parts.body, ['A body line.', ' an indented one']); + deepStrictEqual(parts.trailers, ['PR-URL: x']); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index b1d77a93a..9acf29211 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -232,15 +232,25 @@ try { // do with a stranger's commits is to not have them on disk at all. // GitHub returns them oldest first, which is the order to read them in, // and merges are dropped since their messages say nothing. - const messages: string[] = JSON.parse( - gh( - 'api', - '--paginate', - `repos/${repository()}/pulls/${number}/commits`, - '--jq', - '[.[] | select(.parents | length < 2) | .commit.message]' - ) - ); + // + // `--jq` beside `--paginate` filters each page and prints the results + // one after another, so a branch past the first page of commits would + // hand `JSON.parse` several arrays in a row and land nothing. `--slurp` + // gathers the pages themselves into one array, and the filtering that + // `--jq` was doing happens here instead. + const pages: { parents: unknown[]; commit: { message: string } }[][] = + JSON.parse( + gh( + 'api', + '--paginate', + '--slurp', + `repos/${repository()}/pulls/${number}/commits` + ) + ); + const messages = pages + .flat() + .filter((commit) => commit.parents.length < 2) + .map((commit) => commit.commit.message); const parts = messages.map((message) => partsOfMessage(message)); const message = composeLandingMessage( parts,