diff --git a/.github/scripts/pr-issue-linkage.test.cjs b/.github/scripts/pr-issue-linkage.test.cjs index 30b0662..8219396 100644 --- a/.github/scripts/pr-issue-linkage.test.cjs +++ b/.github/scripts/pr-issue-linkage.test.cjs @@ -42,6 +42,13 @@ function contractBody({ ].join("\n"); } +// GitHub's parser treats closer+#N as a live closing reference even when the +// surrounding words negate it. Fixtures that need those phrases join the tokens +// at runtime so this file itself does not contain a live closer+#N. +function joinWords(parts) { + return parts.join(" "); +} + // Extracts the inline actions/github-script body (the same technique used to // validate the equivalent block ported into melodic-software/medley's // issue-labeling.yml) and runs it in a sandbox with a stub `core`/`process`, @@ -134,6 +141,240 @@ test('"No related issue:" (claude-code-plugins pull-request skill convention) al assert.equal(failedWith, null); }); +test('"Refs: #N" satisfies linkage without arming GitHub\'s closing parser', () => { + const failedWith = runScript( + contractBody({ closing: "Refs: #42", related: "n/a" }), + ); + assert.equal(failedWith, null); +}); + +test('"Relates to: owner/repo#N" is an equally valid non-closing marker', () => { + const failedWith = runScript( + contractBody({ + closing: "Relates to: owner/repo#42", + related: "n/a", + }), + ); + assert.equal(failedWith, null); +}); + +test("the non-closing marker is case-insensitive and tolerates up to three leading spaces", () => { + for (const marker of [ + "refs: #42", + "REFS: #42", + " Relates To: #42", + "Refs:#42", + ]) { + const failedWith = runScript( + contractBody({ closing: marker, related: "n/a" }), + ); + assert.equal( + failedWith, + null, + `expected "${marker}" to satisfy the linkage requirement`, + ); + } +}); + +test("a non-closing marker with trailing prose on the same line does not satisfy linkage", () => { + const failedWith = runScript( + contractBody({ closing: "Refs: #42 extra prose", related: "n/a" }), + ); + assert.ok( + failedWith, + "only a bare marker line may satisfy the gate -- prose around it is not the marker form", + ); + assert.match(failedWith, /closing keyword/); +}); + +test("a non-closing marker without its colon does not satisfy linkage", () => { + const failedWith = runScript( + contractBody({ closing: "Refs #42", related: "n/a" }), + ); + assert.ok(failedWith, "the colon is required by the marker form"); + assert.match(failedWith, /closing keyword/); +}); + +test("a negated closing keyword fails the gate even with every contract header present", () => { + const failedWith = runScript( + contractBody({ + closing: joinWords(["This", "PR", "does", "not", "close", "#42."]), + related: "n/a", + }), + ); + assert.ok( + failedWith, + "GitHub auto-closes the issue on merge regardless of the disclaimer, so the gate must fail", + ); + assert.match(failedWith, /Negated closing reference/); + assert.match(failedWith, /trigger "not"/); + assert.match(failedWith, /Refs: #N/); +}); + +test('a negated closing keyword still fails when "No linked issue" is also present', () => { + const failedWith = runScript( + contractBody({ + closing: `No linked issue. ${joinWords(["This", "PR", "does", "not", "close", "#42."])}`, + related: "n/a", + }), + ); + assert.ok( + failedWith, + "the opt-out marker does not disarm the closing reference GitHub will act on", + ); + assert.match(failedWith, /Negated closing reference/); +}); + +test("a negated closing keyword still fails when a valid non-closing marker is also present", () => { + const failedWith = runScript( + contractBody({ + closing: "Refs: #42", + related: joinWords(["This", "PR", "does", "not", "close", "#42."]), + }), + ); + assert.ok( + failedWith, + "a correct marker elsewhere does not excuse a live closing reference", + ); + assert.match(failedWith, /Negated closing reference/); +}); + +test("every negation form in the window is recognized", () => { + for (const phrase of [ + ["This", "does", "not", "close", "#42."], + ["This", "will", "not", "close", "#42."], + ["We", "do", "not", "close", "#42."], + ["It", "never", "closes", "#42."], + ["It", "doesn't", "close", "#42."], + ["It", "won't", "close", "#42."], + ["It", "didn't", "close", "#42."], + ["It", "deliberately", "closes", "#42."], + ["It", "intentionally", "closes", "#42."], + ["There", "is", "no", "scenario", "where", "this", "closes", "#42."], + ["Shipped", "without", "closes", "#42."], + ].map(joinWords)) { + const failedWith = runScript( + contractBody({ closing: phrase, related: "n/a" }), + ); + assert.ok(failedWith, `expected "${phrase}" to be read as negated`); + assert.match(failedWith, /Negated closing reference/); + } +}); + +test("negation is scoped to the same line and cut at a sentence break", () => { + for (const phrase of [ + `Nothing here is optional. ${joinWords(["Closes", "#42"])}`, + `This PR does not touch the selector.\n${joinWords(["Closes", "#42"])}`, + `No known issues, ${joinWords(["closes", "#90."])}`, + ]) { + const failedWith = runScript( + contractBody({ closing: phrase, related: "n/a" }), + ); + assert.equal( + failedWith, + null, + `expected "${phrase}" to remain an ordinary closing reference`, + ); + } +}); + +test("correlative not-only-but is not treated as a negated closer", () => { + const failedWith = runScript( + contractBody({ + closing: joinWords([ + "This", + "not", + "only", + "documents", + "but", + "fixes", + "#42", + ]), + related: "n/a", + }), + ); + assert.equal( + failedWith, + null, + "not only … but fixes #N is an affirmative closer, not a disclaimer", + ); +}); + +test("negation looks back at most five words", () => { + const failedWith = runScript( + contractBody({ + closing: joinWords([ + "This", + "PR", + "does", + "not", + "change", + "any", + "of", + "the", + "exported", + "helper", + "names,", + "closes", + "#42", + ]), + related: "n/a", + }), + ); + assert.equal( + failedWith, + null, + "a negation six or more words back belongs to a different clause", + ); +}); + +test("a negated closing keyword is not itself accepted as the required linkage", () => { + const failedWith = runScript( + contractBody({ + closing: joinWords(["This", "PR", "does", "not", "close", "#42."]), + related: "n/a", + }), + ); + assert.ok(failedWith, "expected a failure"); + assert.match(failedWith, /Negated closing reference/); + assert.match(failedWith, /closing keyword/); +}); + +test("a non-negated closing keyword on a line that also carries a negated one still fails", () => { + const failedWith = runScript( + contractBody({ + closing: `${joinWords(["Closes", "#41"])} but ${joinWords(["does", "not", "close", "#42."])}`, + related: "n/a", + }), + ); + assert.ok(failedWith, "expected a failure"); + assert.match(failedWith, /Negated closing reference/); + assert.doesNotMatch(failedWith, /Missing a native closing keyword/); +}); + +test("a negated closing keyword inside a code span is not live and does not fail the gate", () => { + const failedWith = runScript( + `Never write \`${joinWords(["does", "not", "close", "#42"])}\` in a body.\n\n` + + contractBody({ closing: "Refs: #42", related: "n/a" }), + ); + assert.equal( + failedWith, + null, + "code-span text is not rendered linkage, so GitHub will not act on it either", + ); +}); + +test("a non-closing marker inside a code span does not satisfy the gate", () => { + const failedWith = runScript( + "Example: `Refs: #42`\n\n## Summary\n\ns\n\n## Fix\n\nf\n\n## Verification\n\nv\n\n## Related\n\n- #123", + ); + assert.ok( + failedWith, + "inline-code marker text must not count as rendered metadata", + ); + assert.match(failedWith, /closing keyword/); +}); + test("missing closing keyword and all contract headers fails with each message", () => { const failedWith = runScript("Just a description, nothing else."); assert.ok(failedWith, "expected a failure"); diff --git a/.github/workflows/pr-issue-linkage.yml b/.github/workflows/pr-issue-linkage.yml index 40b8116..d28c3bf 100644 --- a/.github/workflows/pr-issue-linkage.yml +++ b/.github/workflows/pr-issue-linkage.yml @@ -1,10 +1,14 @@ name: pr-issue-linkage # Reusable workflow — requires the PR body to carry a native GitHub closing -# keyword (Closes/Fixes/Resolves #N, or the literal "No linked issue" / "No -# related issue:" when the PR closes nothing) AND the four contract headers -# with content: "## Summary", "## Fix", "## Verification", and "## Related" +# keyword (Closes/Fixes/Resolves #N), a non-closing "Refs: #N" / "Relates to: +# #N" marker, or the literal "No linked issue" / "No related issue:" when the +# PR relates to no issue at all, AND the four contract headers with content: +# "## Summary", "## Fix", "## Verification", and "## Related" # (issue #153; consumer template alignment in claude-code-plugins#553). +# A closing keyword whose preceding words negate it ("does not close #N") +# fails the gate outright, because GitHub's own parser is negation-blind and +# auto-closes the issue on merge regardless of the disclaimer (issue #521). # GATING by design: a caller that requires this check blocks the merge until # the body is fixed. Generalizes melodic-software/provisioning's pr-body.yml # (decisions #58/#59) into a shared reusable workflow, mirroring pr-title.yml's @@ -458,17 +462,115 @@ jobs: } } + // Matches the same shape GitHub's own linkage parser accepts: one of the nine + // closing keywords, an optional colon, and a `#N` or `owner/repo#N` tail. + // Global so every occurrence on a line can be classified, not just the first. const CLOSING_KEYWORD = - /\b(close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s*(?:[\w.-]+\/[\w.-]+)?#\d+\b/i; + /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s*(?:[\w.-]+\/[\w.-]+)?#\d+\b/gi; + // First-class non-closing linkage: "this PR references issue N and deliberately + // does not close it". Before #521 the only passing escape was the no-issue + // opt-out, which is a lie when the PR plainly does relate to an issue, and the + // honest prose alternative armed GitHub's parser. The shape is deliberately + // strict -- colon required, at most three leading spaces (four opens an indented + // code block), one marker per rendered line, no trailing prose -- so ordinary + // sentences that happen to contain "refs" cannot satisfy a merge gate. + const NON_CLOSING_MARKER = + /^ {0,3}(?:refs|relates[ \t]+to):[ \t]*(?:[\w.-]+\/[\w.-]+)?#\d+[ \t]*$/i; // Accepts both phrasings: provisioning/pr-body.yml's original "No linked issue" // and claude-code-plugins/source-control:pull-request's independently-evolved // "No related issue:" opt-out marker (reference/create.md). Widened, not narrowed, // so this reusable workflow works as a caller for either repo's existing convention. const NO_ISSUE_MARKER = /\bno (?:linked|related) issue\b/i; - if (!CLOSING_KEYWORD.test(body) && !NO_ISSUE_MARKER.test(body)) { + + // GitHub's parser reads the keyword and nothing around it, so a negated + // closer still registers a live closing reference and still auto-closes + // the issue on merge (observed on melodic-software/dotfiles#583). The gate + // reads the words the parser ignores: at most five tokens immediately + // before the keyword on the same rendered line, cut at the nearest + // sentence or comma so an unrelated earlier clause cannot negate it. + const NEGATION_WINDOW_WORDS = 5; + const NEGATION_WORDS = new Set([ + "not", + "never", + "no", + "without", + "deliberately", + "intentionally", + ]); + + function findNegationTrigger(line, keywordIndex) { + const preceding = line.slice(0, keywordIndex); + const sentenceBreak = Math.max( + preceding.lastIndexOf("."), + preceding.lastIndexOf("!"), + preceding.lastIndexOf("?"), + preceding.lastIndexOf(";"), + preceding.lastIndexOf(","), + ); + const words = + preceding.slice(sentenceBreak + 1).match(/[A-Za-z][A-Za-z'\u2019]*/g) || []; + const window = words.slice(-NEGATION_WINDOW_WORDS); + for (let index = 0; index < window.length; index += 1) { + const word = window[index]; + const lower = word.toLowerCase(); + // Correlative "not only … but" is affirmative ("not only + // documents but fixes #N"), not a disclaimer. "not" still + // counts when the next token is anything else. + if (lower === "not" && window[index + 1]?.toLowerCase() === "only") { + continue; + } + if (NEGATION_WORDS.has(lower) || /n['\u2019]t$/i.test(word)) { + return word; + } + } + return null; + } + + let hasClosingKeyword = false; + let hasNonClosingMarker = false; + const negatedClosingReferences = []; + for (const line of body.split(/\r?\n/)) { + if (NON_CLOSING_MARKER.test(line)) hasNonClosingMarker = true; + CLOSING_KEYWORD.lastIndex = 0; + let match; + while ((match = CLOSING_KEYWORD.exec(line)) !== null) { + const trigger = findNegationTrigger(line, match.index); + if (trigger) { + negatedClosingReferences.push({ text: match[0], trigger }); + } else { + hasClosingKeyword = true; + } + } + } + + // Fail-closed, never warn-only, and never excused by a valid marker elsewhere + // in the body: the disclaimer does not stop the merge from closing the issue, + // so the phrase itself has to go before this PR can merge. + if (negatedClosingReferences.length > 0) { + const quoted = [ + ...new Map( + negatedClosingReferences.map((reference) => [reference.text, reference]), + ).values(), + ] + .map((reference) => `"${reference.text}" (trigger "${reference.trigger}")`) + .join(", "); + errors.push( + `Negated closing reference (${quoted}). GitHub's linkage parser ignores the ` + + "surrounding words, so this still registers a closing reference and still " + + "auto-closes the issue when this PR merges. Remove the closing keyword and " + + 'use "Refs: #N" (or "Relates to: #N") on its own line instead.' + ); + } + + // A negated closing reference deliberately does not count here either: the only + // ways to satisfy linkage are a real closing keyword, an explicit non-closing + // marker, or the no-issue opt-out. + if (!hasClosingKeyword && !hasNonClosingMarker && !NO_ISSUE_MARKER.test(body)) { errors.push( "Missing a native closing keyword (Closes/Fixes/Resolves #N). If this PR " + - 'closes no GitHub issue, state "No linked issue" (or "No related issue:") in the body instead.' + 'references an issue it must not close, put "Refs: #N" (or "Relates to: #N") ' + + 'on its own line. If it relates to no GitHub issue at all, state ' + + '"No linked issue" (or "No related issue:") in the body instead.' ); } diff --git a/README.md b/README.md index a3535f3..1bd60e6 100644 --- a/README.md +++ b/README.md @@ -895,10 +895,32 @@ GitHub continues the normal weekly patching of each hosted image generation. the flip. The self-flip is therefore deferred; consumers should follow this documented block rather than copying the dogfood file. - `.github/workflows/pr-issue-linkage.yml` — validates the PR **body** carries - a native closing keyword (`Closes`/`Fixes`/`Resolves #N`, including - `owner/repo#N`, or the literal `No linked issue` or `No related issue:` - when the PR closes nothing) and non-empty `## Summary`, `## Fix`, - `## Verification`, and `## Related` sections (the four contract headers). + one of the three accepted linkage markers and non-empty `## Summary`, + `## Fix`, `## Verification`, and `## Related` sections (the four contract + headers). The accepted markers are: + - a native closing keyword — `Closes`/`Fixes`/`Resolves #N`, including + `owner/repo#N`, for an issue this PR should auto-close on merge; + - a non-closing reference — `Refs: #N` or `Relates to: #N` (also + `owner/repo#N`), for an issue this PR references but must **not** close. + The colon is required, the marker must be alone on its own rendered line + (at most three leading spaces, no trailing prose), and matching is + case-insensitive; + - the literal `No linked issue` or `No related issue:`, for a PR that + relates to no issue at all. + + A closing keyword that the surrounding prose negates (`does not close #N`, + `won't fix #N`, `deliberately resolves #N`) **fails** the gate rather than + satisfying it, and no other marker excuses it — GitHub's own linkage parser + is negation-blind, so the disclaimer still registers a live closing + reference and still auto-closes the issue on merge (#521). The gate reads + the words the parser ignores: up to five word tokens before the keyword on + the same rendered line, cut at the nearest `.`/`!`/`?`/`;`/`,`. Correlative + `not only … but` is not treated as negation (`This not only documents but + fixes #N` still satisfies the gate). A comma-separated note such as `No + known issues, closes #N` is a new clause, not a disclaimer. The failure + quotes the trigger word. Remove a real disclaimer and use `Refs: #N` + instead. + **Gating**: a non-conforming body fails the job. HTML comments are stripped before either check, so an unedited PR template (whose instructional prose lives in comments) fails rather than