diff --git a/.github/scripts/check-pr-dedup-search.mjs b/.github/scripts/check-pr-dedup-search.mjs new file mode 100644 index 00000000..7e216a35 --- /dev/null +++ b/.github/scripts/check-pr-dedup-search.mjs @@ -0,0 +1,71 @@ +#!/usr/bin/env node +/** + * check-pr-dedup-search.mjs + * Checks that the PR body affirms the author searched for similar PRs before + * opening this one. Looks for a checked checklist line matching the dedup + * affirmation in the PR template. Respects the same skip prefixes as the + * linked-issue gate. + * + * Export: checkDedupSearch(prBody, prTitle) → { passed, failures } + */ +import { fileURLToPath } from 'node:url'; + +const SKIP_PREFIXES = ['docs', 'chore', 'build', 'ci', 'style', 'test', 'revert']; + +// Match a markdown checkbox line whose label mentions searching for similar +// PRs. Examples that should match (checked): +// - [x] I searched for similar open/closed PRs and confirmed this is not a duplicate +// - [X] I searched the GitHub PR list (open + recently closed) for similar PRs ... +// * [x] Searched for similar PRs — not a duplicate +const DEDUP_CHECKBOX_RE = + /^\s*[-*]\s*\[\s*([ xX])\s*\][^\n]*search(?:ed)?[^\n]*(?:similar|duplicate|prior)[^\n]*\bprs?\b/im; + +function parsePrefix(title) { + if (!title) return null; + const match = title.match(/^([a-z]+)(?:\([^)]*\))?:/); + return match ? match[1].toLowerCase() : null; +} + +export function checkDedupSearch(body, prTitle = '') { + const prefix = parsePrefix(prTitle); + if (prefix && SKIP_PREFIXES.includes(prefix)) { + return { passed: true, failures: [] }; + } + + if (!body || !body.trim()) { + return { + passed: false, + failures: ['PR body is empty — please fill out the PR template'], + }; + } + + const match = body.match(DEDUP_CHECKBOX_RE); + if (!match) { + return { + passed: false, + failures: [ + 'Add the dedup-search checkbox to your PR description and check it once ' + + 'you have searched the GitHub PR list for similar PRs. See the PR template ' + + 'at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".', + ], + }; + } + + const checked = match[1] === 'x' || match[1] === 'X'; + return { + passed: checked, + failures: checked ? [] : [ + 'Please confirm you searched the GitHub PR list for similar PRs by ' + + 'checking the dedup-search checkbox in your PR description ' + + '(`- [x] I searched ...`). See CONTRIBUTING.md → "Before You Start: Search First".', + ], + }; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const body = process.env.PR_BODY ?? ''; + const title = process.env.PR_TITLE ?? ''; + const result = checkDedupSearch(body, title); + console.log(JSON.stringify(result)); + process.exit(result.passed ? 0 : 1); +} diff --git a/.github/scripts/check-pr-linked-issue.mjs b/.github/scripts/check-pr-linked-issue.mjs index 865b8865..46f3a881 100644 --- a/.github/scripts/check-pr-linked-issue.mjs +++ b/.github/scripts/check-pr-linked-issue.mjs @@ -1,21 +1,86 @@ #!/usr/bin/env node /** * check-pr-linked-issue.mjs - * Checks that a PR body references a GitHub issue. Respects conventional commit - * prefixes — skips check for docs/chore/build/ci/style/test prefixed PRs. - * Export: checkLinkedIssue(prBody: string, prTitle: string) → { passed, failures } + * Checks that a PR body either links an existing issue/PR or inlines an + * issue-template-shaped description. Respects conventional commit prefixes — + * skips check for docs/chore/build/ci/style/test/revert prefixed PRs. + * + * Exports: + * checkLinkedIssue(prBody, prTitle) → { passed, failures } + * hasInlineIssueDescription(prBody) → boolean */ import { fileURLToPath } from 'node:url'; const ISSUE_PATTERNS = [ - /(?:fixes|closes|resolves)\s+#\d+/i, + /(?:fixes|closes|resolves|refs)\s+#\d+/i, /(?:^|[\s(])https:\/\/github\.com\/paperclipai\/paperclip\/issues\/\d+(?=$|[\s),:;!?]|[.](?![\w-]))/i, /(? { + 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; + } + return matched; +} + +export function hasInlineIssueDescription(body) { + if (!body || !body.trim()) return false; + for (const fieldSet of Object.values(TEMPLATE_FIELDS)) { + if (countMatchedFields(body, fieldSet) >= INLINE_DESCRIPTION_MIN_FIELDS) { + return true; + } + } + return false; +} + function parsePrefix(title) { if (!title) return null; const match = title.match(/^([a-z]+)(?:\([^)]*\))?:/); @@ -33,13 +98,18 @@ export function checkLinkedIssue(body, prTitle = '') { return { passed: false, failures: ['PR body is empty — please fill out the PR template'] }; } - const found = ISSUE_PATTERNS.some(p => p.test(body)); + const linked = ISSUE_PATTERNS.some(p => p.test(body)); + const inlined = hasInlineIssueDescription(body); + const passed = linked || inlined; + return { - passed: found, - failures: found ? [] : [ - 'No linked issue found — please add `Fixes #NNN` to your PR description. ' + - 'If no issue exists yet, please file one first: ' + - 'https://github.com/paperclipai/paperclip/issues/new', + passed, + failures: passed ? [] : [ + 'No linked issue or inline issue description found — either tag an existing issue ' + + 'with `Fixes #NNN` / `Closes #NNN` / `Refs #NNN`, or describe the underlying issue ' + + 'inline in the PR body following one of our issue templates ' + + '(https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). ' + + 'See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".', ], }; } diff --git a/.github/scripts/run-quality-gates.mjs b/.github/scripts/run-quality-gates.mjs index a5b6abae..a99786fb 100644 --- a/.github/scripts/run-quality-gates.mjs +++ b/.github/scripts/run-quality-gates.mjs @@ -12,6 +12,7 @@ import { ghFetch } from './get-bot-token.mjs'; import { fetchAllPullRequestFiles } from './fetch-pr-files.mjs'; import { checkTemplate } from './check-pr-template.mjs'; import { checkLinkedIssue } from './check-pr-linked-issue.mjs'; +import { checkDedupSearch } from './check-pr-dedup-search.mjs'; import { checkTestCoverage } from './check-pr-test-coverage.mjs'; import { checkLockfile } from './check-pr-lockfile.mjs'; import { checkDependencies } from './check-pr-dependencies.mjs'; @@ -110,10 +111,11 @@ async function main() { // Run all quality gates (pure functions run sync, deps check is async) const prTitle = pr.title ?? ''; - const [templateResult, issueResult, testResult, lockfileResult, depsResult] = + const [templateResult, issueResult, dedupResult, testResult, lockfileResult, depsResult] = await Promise.all([ Promise.resolve(checkTemplate(prBody)), Promise.resolve(checkLinkedIssue(prBody, prTitle)), + Promise.resolve(checkDedupSearch(prBody, prTitle)), Promise.resolve(checkTestCoverage(files, prTitle)), Promise.resolve(checkLockfile(files, author, branch)), checkDependencies(files, GH_TOKEN, GH_REPO, prNumber, pr.base?.ref), @@ -122,6 +124,7 @@ async function main() { const allFailures = [ ...templateResult.failures, ...issueResult.failures, + ...dedupResult.failures, ...testResult.failures, ...lockfileResult.failures, ]; diff --git a/.github/scripts/tests/check-pr-dedup-search.test.mjs b/.github/scripts/tests/check-pr-dedup-search.test.mjs new file mode 100644 index 00000000..3886e9c7 --- /dev/null +++ b/.github/scripts/tests/check-pr-dedup-search.test.mjs @@ -0,0 +1,93 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { checkDedupSearch } from '../check-pr-dedup-search.mjs'; + +test('passes when dedup checkbox is checked (lowercase x)', () => { + const body = ` +## Checklist + +- [x] I searched the GitHub PR list (open + recently closed) for similar PRs and confirmed this is not a duplicate +- [ ] Other thing +`; + assert.equal(checkDedupSearch(body, 'feat: thing').passed, true); +}); + +test('passes when dedup checkbox is checked (uppercase X)', () => { + const body = `- [X] I have searched GitHub for duplicate or related PRs and linked them above`; + assert.equal(checkDedupSearch(body, 'feat: thing').passed, true); +}); + +test('passes with current PR template wording (master)', () => { + const body = ` +- [x] I have searched GitHub for duplicate or related PRs and linked them above +`; + assert.equal(checkDedupSearch(body, 'feat: thing').passed, true); +}); + +test('fails when dedup checkbox is present but unchecked', () => { + const body = ` +- [ ] I searched for similar open/closed PRs and confirmed this is not a duplicate +- [ ] Other thing +`; + const result = checkDedupSearch(body, 'feat: thing'); + assert.equal(result.passed, false); + assert.ok(result.failures.length > 0); +}); + +test('fails when dedup checkbox is missing entirely', () => { + const body = ` +## Checklist + +- [x] I have run tests locally +- [x] I have updated documentation +`; + const result = checkDedupSearch(body, 'feat: thing'); + assert.equal(result.passed, false); + assert.ok(result.failures.length > 0); +}); + +test('fails when PR body is empty (and no skip prefix)', () => { + const result = checkDedupSearch('', 'feat: thing'); + assert.equal(result.passed, false); + assert.ok(result.failures.length > 0); +}); + +test('skips check for docs: prefix', () => { + assert.equal(checkDedupSearch('', 'docs: update README').passed, true); +}); + +test('skips check for chore: prefix', () => { + assert.equal(checkDedupSearch('', 'chore: bump deps').passed, true); +}); + +test('skips check for ci: prefix', () => { + assert.equal(checkDedupSearch('', 'ci: tweak workflow').passed, true); +}); + +test('skips check for test: prefix', () => { + assert.equal(checkDedupSearch('', 'test: add coverage').passed, true); +}); + +test('requires checkbox for feat: prefix', () => { + assert.equal(checkDedupSearch('Some description without checkbox', 'feat: new thing').passed, false); +}); + +test('does not match unrelated checked items mentioning "PR"', () => { + const body = ` +- [x] I have run tests locally and they pass +- [x] All Paperclip CI gates are green +`; + assert.equal(checkDedupSearch(body, 'feat: thing').passed, false); +}); + +test('matches asterisk bullet style', () => { + const body = `* [x] I searched GitHub for similar PRs and this is not a duplicate`; + assert.equal(checkDedupSearch(body, 'feat: thing').passed, true); +}); + +test('does not match when "pr" only appears inside an unrelated word', () => { + // "approaches" contains the letters "pr" but is not the token "PR/PRs". + // Without a word-boundary anchor the regex would incorrectly pass. + const body = `- [x] I searched for similar approaches to this problem`; + assert.equal(checkDedupSearch(body, 'feat: thing').passed, false); +}); diff --git a/.github/scripts/tests/check-pr-linked-issue.test.mjs b/.github/scripts/tests/check-pr-linked-issue.test.mjs index 6b2745db..4f6d1a63 100644 --- a/.github/scripts/tests/check-pr-linked-issue.test.mjs +++ b/.github/scripts/tests/check-pr-linked-issue.test.mjs @@ -1,6 +1,6 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { checkLinkedIssue } from '../check-pr-linked-issue.mjs'; +import { checkLinkedIssue, hasInlineIssueDescription } from '../check-pr-linked-issue.mjs'; // Existing tests with title parameter added (defaults to no prefix, so still required) @@ -20,6 +20,14 @@ test('passes with "Resolves #NNN"', () => { assert.equal(checkLinkedIssue('Resolves #101', 'fix: something').passed, true); }); +test('passes with "Refs #NNN"', () => { + assert.equal(checkLinkedIssue('Refs #202', 'fix: something').passed, true); +}); + +test('passes with "refs #NNN" (case-insensitive)', () => { + assert.equal(checkLinkedIssue('refs #303', 'fix: something').passed, true); +}); + test('passes with full github.com URL', () => { assert.equal( checkLinkedIssue('See https://github.com/paperclipai/paperclip/issues/202', 'fix: bug').passed, @@ -72,7 +80,7 @@ test('fails when #NNN is part of a word (no space before)', () => { assert.equal(result.passed, false); }); -// New tests for prefix-aware skip behavior +// Prefix-aware skip behavior test('skips check for docs: prefix', () => { assert.equal(checkLinkedIssue('', 'docs: update README').passed, true); @@ -109,3 +117,107 @@ test('requires issue for refactor: prefix', () => { test('requires issue when no prefix (encourages prefix usage)', () => { assert.equal(checkLinkedIssue('No prefix here', 'Add some feature').passed, false); }); + +// Inline issue description (path 2) + +const BUG_INLINE_BODY = ` +## What happened? + +Login button does nothing when clicked. + +## Expected behavior + +Clicking the login button should authenticate the user. + +## Steps to reproduce + +1. Open the app +2. Click login +3. Nothing happens +`; + +const FEATURE_INLINE_BODY = ` +## Problem or motivation + +We don't have a way to bulk-tag issues. + +## Proposed solution + +Add a bulk-tag action to the issues list. + +## Alternatives considered + +Tagging individually — too slow. +`; + +const ADAPTER_INLINE_BODY = ` +## Agent or provider + +Gemini CLI + +## Why this adapter is useful + +Lots of users want Gemini as an alternative model option. + +## How the agent is invoked + +Via the \`gemini\` CLI binary with stdin/stdout JSON. +`; + +test('passes with inline bug description (3 template fields, feat: prefix)', () => { + assert.equal(checkLinkedIssue(BUG_INLINE_BODY, 'feat: fix login button').passed, true); +}); + +test('passes with inline feature description (3 template fields)', () => { + assert.equal(checkLinkedIssue(FEATURE_INLINE_BODY, 'feat: bulk tag').passed, true); +}); + +test('passes with inline adapter description (3 template fields)', () => { + assert.equal(checkLinkedIssue(ADAPTER_INLINE_BODY, 'feat: gemini adapter').passed, true); +}); + +test('fails with only two bug template fields (below threshold)', () => { + const body = ` +## What happened? + +Something broke. + +## Expected behavior + +It should work. +`; + assert.equal(checkLinkedIssue(body, 'feat: fix').passed, false); +}); + +test('fails with a single stray template-like heading', () => { + const body = ` +This is mostly a free-form description but one heading happens to match. + +## Expected behavior + +Everything works. +`; + assert.equal(checkLinkedIssue(body, 'feat: fix').passed, false); +}); + +test('hasInlineIssueDescription returns true for ≥3 bug fields', () => { + assert.equal(hasInlineIssueDescription(BUG_INLINE_BODY), true); +}); + +test('hasInlineIssueDescription returns false for empty body', () => { + assert.equal(hasInlineIssueDescription(''), false); +}); + +test('hasInlineIssueDescription accepts bolded labels with colons', () => { + const body = ` +**Problem:** +We need this. + +**Proposed solution:** +Build it. + +**Alternatives considered:** +None. +`; + assert.equal(hasInlineIssueDescription(body), true); +}); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 67b60be3..2c43a072 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,6 +14,8 @@ Before you start work, **search GitHub** for existing PRs and issues that touch Duplicate PRs create extra work for reviewers and make merging harder. A 60-second search saves hours later. +Affirm that you did this search by checking the dedup-search box in the PR template (`I have searched GitHub for duplicate or related PRs and linked them above`). Commitperclip checks for this checkbox on non-trivial PRs. + ## Two Paths to Get Your Pull Request Accepted ### Path 1: Small, Focused Changes (Fastest way to get merged)