diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 9f1c252b98..c7f9471c8e 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -19,10 +19,11 @@ ## Linked Issues or Issue Description diff --git a/.github/scripts/check-pr-linked-issue.mjs b/.github/scripts/check-pr-linked-issue.mjs index 46f3a881e9..d1dd2bb59d 100644 --- a/.github/scripts/check-pr-linked-issue.mjs +++ b/.github/scripts/check-pr-linked-issue.mjs @@ -47,34 +47,111 @@ const TEMPLATE_FIELDS = { ["Why this adapter is useful", "Why it's useful", 'Why useful', 'Use case'], ['How the agent is invoked', 'How it is invoked', "How it's invoked", 'Invocation'], ], + // Labels below match .github/ISSUE_TEMPLATE/enhancement.yml exactly. + enhancement: [ + ['What existing behavior does this improve?', 'What existing behavior does this improve'], + ['Subsystem affected'], + ['Current behavior'], + ['Proposed behavior'], + ['Reason and benefit'], + ['Breaking changes'], + ], + // Labels below match .github/ISSUE_TEMPLATE/docs_issue.yml exactly. The + // template has 4 distinct fields, so it meets the 3-field minimum. A + // "docs"-prefixed PR skips this check; this set helps a non-"docs"-prefixed + // PR that describes a documentation issue inline. + docs: [ + ['Issue type'], + ['Where is the issue?', 'Where is the issue'], + ["What's wrong?", "What's wrong"], + ['Suggested fix'], + ], }; function escapeRegExp(s) { return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } -function countMatchedFields(body, fieldSet) { +// A generic "label line" is a markdown heading (`## Label`) or a bolded label on +// its own line (`**Label**`). The content scan stops at a label line, because +// that line starts a new field. +const LABEL_LINE = /^\s*(?:#{1,6}\s+\S|(?:\*\*|__)[^*_].*(?:\*\*|__)\s*[:?]?\s*$)/; + +// Build the regex that matches one field label on its own line. +function labelLinePattern(label) { + const esc = escapeRegExp(label); + // Accept markdown headings or bolded/plain labels on their own line. + // Examples: "## What happened?", "**Expected behavior**", "Problem:". + return new RegExp( + `^\\s*(?:#{1,6}\\s+|\\*\\*\\s*|__\\s*)?${esc}(?:\\s*[:?])?(?:\\s*\\*\\*|\\s*__)?\\s*$`, + 'i' + ); +} + +// Every known field label from every template, precompiled. The generic +// LABEL_LINE regex sees a heading or a bold label as a field boundary, but not a +// plain "Label:" line. A skeleton of stacked plain labels needs each label to +// act as a boundary. Without this list the scan reads the next label as +// content, so it counts an empty field as filled. +const KNOWN_LABEL_PATTERNS = Object.values(TEMPLATE_FIELDS) + .flat(2) + .map(labelLinePattern); + +// Return true if the line starts a new field. The line is a heading, a bold +// label, or a plain line that equals a known field label. +function isFieldBoundary(line) { + return LABEL_LINE.test(line) || KNOWN_LABEL_PATTERNS.some(p => p.test(line)); +} + +// Return true if the line holds real content, not a bare placeholder. The +// default template skeleton puts a lone "-" under each label, so a label with +// only "-", blank lines, or a "[...]" placeholder does not count as filled. +function lineHasContent(line) { + let text = line.trim(); + if (!text) return false; + // Drop a leading list marker ("- ", "* ", "1. ") before the check. + text = text.replace(/^[-*+]\s*/, '').replace(/^\d+[.)]\s*/, '').trim(); + if (!text) return false; + // Treat a whole-line bracket placeholder ("[describe here]") as empty. + if (/^\[.*\]$/.test(text)) return false; + return true; +} + +// Return true if a label variant appears on its own line AND at least one +// content line follows it before the next label line. +function isFieldFilled(lines, variants) { + const patterns = variants.map(labelLinePattern); + for (let i = 0; i < lines.length; i += 1) { + if (!patterns.some(p => p.test(lines[i]))) continue; + for (let j = i + 1; j < lines.length; j += 1) { + if (isFieldBoundary(lines[j])) break; // next field starts here + if (lineHasContent(lines[j])) return true; + } + } + return false; +} + +function countMatchedFields(lines, fieldSet) { let matched = 0; for (const variants of fieldSet) { - const hasMatch = variants.some(label => { - const esc = escapeRegExp(label); - // Accept markdown headings or bolded/plain labels on their own line. - // Examples: "## What happened?", "**Expected behavior**", "Problem:". - const pattern = new RegExp( - `^\\s*(?:#{1,6}\\s+|\\*\\*\\s*|__\\s*)?${esc}(?:\\s*[:?])?(?:\\s*\\*\\*|\\s*__)?\\s*$`, - 'im' - ); - return pattern.test(body); - }); - if (hasMatch) matched += 1; + if (isFieldFilled(lines, variants)) matched += 1; } return matched; } +// Remove HTML comments. The PR template puts its guidance and its example +// issue links ("Fixes: #123") inside comments, so the gate must not read them +// as author content. +function stripHtmlComments(body) { + return body.replace(//g, ''); +} + export function hasInlineIssueDescription(body) { if (!body || !body.trim()) return false; + // Strip the guidance comments, then scan the body line by line. + const lines = stripHtmlComments(body).split(/\r?\n/); for (const fieldSet of Object.values(TEMPLATE_FIELDS)) { - if (countMatchedFields(body, fieldSet) >= INLINE_DESCRIPTION_MIN_FIELDS) { + if (countMatchedFields(lines, fieldSet) >= INLINE_DESCRIPTION_MIN_FIELDS) { return true; } } @@ -98,7 +175,7 @@ export function checkLinkedIssue(body, prTitle = '') { return { passed: false, failures: ['PR body is empty — please fill out the PR template'] }; } - const linked = ISSUE_PATTERNS.some(p => p.test(body)); + const linked = ISSUE_PATTERNS.some(p => p.test(stripHtmlComments(body))); const inlined = hasInlineIssueDescription(body); const passed = linked || inlined; diff --git a/.github/scripts/tests/check-pr-linked-issue.test.mjs b/.github/scripts/tests/check-pr-linked-issue.test.mjs index 4f6d1a63e1..f081ed9664 100644 --- a/.github/scripts/tests/check-pr-linked-issue.test.mjs +++ b/.github/scripts/tests/check-pr-linked-issue.test.mjs @@ -1,5 +1,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; import { checkLinkedIssue, hasInlineIssueDescription } from '../check-pr-linked-issue.mjs'; // Existing tests with title parameter added (defaults to no prefix, so still required) @@ -221,3 +223,184 @@ None. `; assert.equal(hasInlineIssueDescription(body), true); }); + +// Prose-only description (no template labels) must fail. A good paragraph of +// prose matches zero labels, so the gate rejects it. +test('fails with a prose-only description that has no template labels', () => { + const body = ` +This pull request rewrites the retry loop so the worker gives up after five +attempts instead of looping forever. The previous loop could hang a job when +the upstream service was down. I also added a log line for each retry so an +operator can see the backoff in the run output. +`; + const result = checkLinkedIssue(body, 'feat: bounded retry'); + assert.equal(result.passed, false); + assert.ok(result.failures.length > 0); +}); + +// An author who copies the feature template labels into the PR body must pass. +// The labels use the bold-label-on-its-own-line form the gate accepts. +const FEATURE_BOLD_LABEL_BODY = ` +**Problem or motivation:** +- The gate rejects a good prose description. + +**Proposed solution:** +- Copy the feature template labels into the PR body. + +**Alternatives considered:** +- Lower the field threshold — rejected, it weakens the gate. +`; + +test('passes with the feature template labels (bold labels)', () => { + assert.equal(checkLinkedIssue(FEATURE_BOLD_LABEL_BODY, 'feat: inline feature description').passed, true); +}); + +// Enhancement template set (matches .github/ISSUE_TEMPLATE/enhancement.yml). +const ENHANCEMENT_INLINE_BODY = ` +## What existing behavior does this improve? + +The board task list sort order. + +## Current behavior + +The list sorts by creation time only. + +## Proposed behavior + +The list sorts by priority, then creation time. + +## Reason and benefit + +Users miss high-priority tasks that were created early. +`; + +test('passes with inline enhancement description (4 template fields)', () => { + assert.equal(checkLinkedIssue(ENHANCEMENT_INLINE_BODY, 'feat: sort by priority').passed, true); +}); + +test('hasInlineIssueDescription returns true for ≥3 enhancement fields', () => { + assert.equal(hasInlineIssueDescription(ENHANCEMENT_INLINE_BODY), true); +}); + +// Empty default skeleton must fail. A label with only the bare "-" placeholder +// under it is not filled, so it must not count toward the field minimum. +const EMPTY_SKELETON_BODY = ` +**What happened?** +- + +**Expected behavior:** +- + +**Steps to reproduce:** +- +`; + +test('fails with an empty template skeleton (labels but no content)', () => { + const result = checkLinkedIssue(EMPTY_SKELETON_BODY, 'feat: something'); + assert.equal(result.passed, false); + assert.ok(result.failures.length > 0); +}); + +test('hasInlineIssueDescription returns false for an empty skeleton', () => { + assert.equal(hasInlineIssueDescription(EMPTY_SKELETON_BODY), false); +}); + +// A filled bug skeleton in the bold-label form must pass, even with list-marker +// content. This proves the fix does not reject real author content. +const FILLED_BUG_SKELETON_BODY = ` +**What happened?** +- The login button does nothing. + +**Expected behavior:** +- The login button authenticates the user. + +**Steps to reproduce:** +- Open the app, then click login. +`; + +test('passes with a filled bug skeleton (three filled fields)', () => { + assert.equal(checkLinkedIssue(FILLED_BUG_SKELETON_BODY, 'feat: fix login').passed, true); +}); + +// Stacked plain labels with no content must fail. Each label sits on its own +// line with the next label directly under it. The scan must treat the next +// label as a field boundary, not as content, so every field stays empty. +const STACKED_FEATURE_LABELS = ` +Problem or motivation: +Proposed solution: +Alternatives considered: +Roadmap alignment: +`; + +const STACKED_BUG_LABELS = ` +What happened?: +Expected behavior: +Steps to reproduce: +Paperclip version: +`; + +const STACKED_ENHANCEMENT_LABELS = ` +What existing behavior does this improve? +Subsystem affected +Current behavior +Proposed behavior +Reason and benefit +`; + +const STACKED_DOCS_LABELS = ` +Issue type +Where is the issue? +What's wrong? +Suggested fix +`; + +test('fails with stacked plain feature labels and no content', () => { + assert.equal(checkLinkedIssue(STACKED_FEATURE_LABELS, 'feat: x').passed, false); +}); + +test('fails with stacked plain bug labels and no content', () => { + assert.equal(checkLinkedIssue(STACKED_BUG_LABELS, 'feat: x').passed, false); +}); + +test('fails with stacked plain enhancement labels and no content', () => { + assert.equal(checkLinkedIssue(STACKED_ENHANCEMENT_LABELS, 'feat: x').passed, false); +}); + +test('fails with stacked plain docs labels and no content', () => { + assert.equal(checkLinkedIssue(STACKED_DOCS_LABELS, 'feat: x').passed, false); +}); + +// A plain-label skeleton with real content under each label must still pass. +// The boundary fix must not reject a field that has genuine content. +const FILLED_PLAIN_FEATURE_LABELS = ` +Problem or motivation: +- The gate rejects a good prose description. +Proposed solution: +- Copy the feature template labels into the PR body. +Alternatives considered: +- Lower the field threshold — rejected, it weakens the gate. +`; + +test('passes with plain feature labels and real content under each', () => { + assert.equal(checkLinkedIssue(FILLED_PLAIN_FEATURE_LABELS, 'feat: inline feature').passed, true); +}); + +// The real .github/PULL_REQUEST_TEMPLATE.md, submitted unchanged, must fail the +// gate. Its skeleton labels have no content and its example issue links live in +// HTML comments, so neither the inline path nor the linked path may pass it. +const PR_TEMPLATE_PATH = fileURLToPath( + new URL('../../PULL_REQUEST_TEMPLATE.md', import.meta.url) +); + +test('fails with the unfilled default PR template body', () => { + const body = readFileSync(PR_TEMPLATE_PATH, 'utf8'); + const result = checkLinkedIssue(body, 'feat: unfilled template'); + assert.equal(result.passed, false); +}); + +// An issue link that appears only inside an HTML comment must not satisfy the +// linked-issue check. The template ships such an example ("Fixes: #123"). +test('fails when the only issue link is inside an HTML comment', () => { + const body = '\n\nSome prose with no real link.'; + assert.equal(checkLinkedIssue(body, 'feat: commented link').passed, false); +});