diff --git a/.github/workflows/pr-title-validator.yml b/.github/workflows/pr-title-validator.yml new file mode 100644 index 000000000..008fded5b --- /dev/null +++ b/.github/workflows/pr-title-validator.yml @@ -0,0 +1,144 @@ +name: PR Title Validator + +# Enforce the PR-title convention the release changelog depends on (#1549). +# +# The changelog is entirely title-driven: release.yml runs +# `gh release create --generate-notes` (GitHub builds "What's Changed" from PR +# titles) and scripts/release-notes/format-github-release-notes.cjs reformats +# it. Two independent rules are read off the title: +# 1. Bucket — classifyBucket() anchors on the leading type (^feat / ^fix / +# else Enhancement). A leading tag (e.g. `[security] `) defeats +# the anchor and silently mis-files the entry. +# 2. Issue link — the `(#)` in the title is what renders as a link to +# the issue in the changelog line. +# +# This gate reuses the SAME matcher the changelog uses +# (scripts/release-notes/conventional-title.cjs) — not a forked regex — so a title that +# passes here cannot mis-bucket in the changelog. +# +# Trust boundary: the matcher is loaded from a BASE-branch checkout (the +# already-merged, reviewed copy on the PR's target), exactly as +# pr-target-validator loads its policy. The PR cannot edit the ruler that +# measures its own title, so the gate is not self-bypassable. Until this +# matcher lands on the base branch it does not exist there — the introducing +# PR is skipped (bootstrap); every PR after merge is fully gated. +# +# Unlike pr-target-validator, this runs for ALL authors (including members): +# the changelog drift that motivated #1549 came from member PRs. +# +# Phase-1 rollout: set WARN_ONLY=true to comment without failing the check. +# Shipped enforcing (WARN_ONLY=false); flip to 'true' for a grace period. +# +# See: scripts/release-notes/conventional-title.cjs, CONTRIBUTING.md, issue #1549. + +on: + pull_request: + types: [opened, edited, reopened, synchronize] + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +permissions: + contents: read + pull-requests: write + +jobs: + validate-title: + runs-on: ubuntu-latest + timeout-minutes: 2 + env: + # Phase-1: set to 'true' to warn only. Shipped enforcing. + WARN_ONLY: 'false' + steps: + # Check out the BASE branch (the PR's merge target) as the trusted policy + # source — not the PR head. The matcher that judges the title must be + # already-merged, reviewed code so a PR cannot bypass the gate by editing + # conventional-title.cjs to accept its own malformed title. Mirrors + # pr-target-validator.yml. The introducing PR is handled by the bootstrap + # guard in the script below (the matcher isn't on base yet). + - name: Checkout base branch (trusted policy source) + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.pull_request.base.ref }} + persist-credentials: false + + - name: Validate PR title + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + WARN_ONLY: ${{ env.WARN_ONLY }} + with: + script: | + const fs = require('fs'); + const matcherPath = `${process.env.GITHUB_WORKSPACE}/scripts/release-notes/conventional-title.cjs`; + + const pr = context.payload.pull_request; + const title = pr.title || ''; + const warnOnly = process.env.WARN_ONLY === 'true'; + + // Bootstrap: the matcher is loaded from the base-branch checkout, so it + // is absent on the PR that first introduces it. Skip rather than fail — + // once this lands on the base branch, every subsequent PR is gated. + if (!fs.existsSync(matcherPath)) { + core.info('conventional-title.cjs not on the base branch yet — bootstrap PR, skipping title check.'); + return; + } + const { evaluatePrTitle } = require(matcherPath); + + const result = evaluatePrTitle({ title }); + + if (result.valid) { + core.info(`PR title OK: ${title}`); + return; + } + + const msg = [ + `### PR title needs the issue-ref convention`, + ``, + `\`${title}\``, + ``, + result.message, + ``, + `**How to fix:** click "Edit" next to the PR title above and retitle it`, + `as \`type(#): summary\`. No need to recreate the PR — this check`, + `re-runs when you edit the title.`, + ``, + `
Why this is enforced`, + ``, + `The release changelog is built from PR titles. A leading tag mis-files`, + `the entry into the wrong section, and a scope without \`(#)\` leaves`, + `the changelog line with no link back to the issue. See issue #1549.`, + ``, + `
`, + ].join('\n'); + + // Post or update a sticky comment. + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pr.number, + }); + const marker = ''; + const existing = comments.find(c => c.body && c.body.includes(marker)); + const body = `${marker}\n${msg}`; + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pr.number, + body, + }); + } + + if (warnOnly) { + core.warning(`PR title convention (warning-only mode): ${result.reason} — ${title}`); + } else { + core.setFailed(`PR title does not follow the convention (${result.reason}): ${title}`); + } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1a5735eb4..c2d14a389 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -228,6 +228,33 @@ node scripts/release-notes/format-github-release-notes.cjs \ Omit `--apply` to print the reformatted body to stdout for review without publishing. +### PR title convention (enforced at open time) + +Because the changelog is built from PR titles, your **PR title** must follow: + +``` +type(#): short summary +``` + +- **Start with the type** — `feat`, `fix`, or any other conventional type + (`chore`, `docs`, `refactor`, …). No leading tags or prefixes: a title like + `[security] fix(config): …` defeats the `^fix` bucket anchor and silently + files the entry under the wrong changelog section. +- **Put the linked issue ref in the scope** — `(#)`. This is what + renders as a link to the issue in the changelog line. `fix(core): …` buckets + correctly but produces a changelog entry with **no issue link**. +- A breaking-change marker is fine: `feat(#42)!: …`. + +Examples: `fix(#1542): roadmap rollback`, `feat(#39): milestone-prefixed phase IDs`, +`enhance(#1549): add PR-title validator`. + +**CI enforcement:** `pr-title-validator.yml` checks the title on open/edit and +fails with the required format if it doesn't conform. It reuses the same matcher +the changelog classifier uses (`scripts/release-notes/conventional-title.cjs`), so a title +that passes the check is guaranteed to bucket and link correctly. Fix a flagged +title by editing it in place — the check re-runs on edit, no need to recreate +the PR. + ## Documentation Updates — Update the Relevant Docs If your PR adds, changes, deprecates, or removes user-visible behavior, you **must** update the relevant documentation in `docs/`. CI will fail any PR whose changeset fragment is typed `Added`, `Changed`, `Deprecated`, or `Removed` without also modifying at least one file under `docs/` ([#3213](https://github.com/open-gsd/gsd-core/issues/3213)). diff --git a/scripts/release-notes/conventional-title.cjs b/scripts/release-notes/conventional-title.cjs new file mode 100644 index 000000000..e1955bf44 --- /dev/null +++ b/scripts/release-notes/conventional-title.cjs @@ -0,0 +1,88 @@ +'use strict'; + +/** + * Single source of truth for conventional-commit PR-title parsing. + * + * Consumed by BOTH: + * - the release-notes changelog classifier + * (scripts/release-notes/format-github-release-notes.cjs), and + * - the PR-title CI gate (.github/workflows/pr-title-validator.yml, via + * evaluatePrTitle). + * + * Keeping one matcher here is the point of #1549: a forked copy of the regex + * would let the gate accept a title that the changelog then mis-buckets. Both + * the bucket anchors and the gate must read the title the same way. + */ + +// Bucket anchors. The leading `^` is load-bearing: the changelog buckets on the +// type at the START of the title. Anything before it (e.g. a `[security] ` tag) +// defeats the anchor and silently mis-files the entry — which is exactly the +// drift the PR-title gate below rejects at open time. +const FEATURE_RE = /^feat(?:ure)?\s*(?:\(|!|:)/i; +const FIX_RE = /^fix\s*(?:\(|!|:)/i; + +// A well-formed conventional header at the START of the title: +// [()][!]: +// e.g. `fix(#1542):`, `feat(#39)!:`, `fix:`, `enhance(verify-phase):`. +// Anchored with `^` so a leading tag/prefix fails to match (no `bad-prefix`). +const HEADER_RE = /^([a-z]+)(\([^)]*\))?(!)?:/i; + +// An issue reference inside a scope: `(#123)`, `(#123, core)`, etc. +const ISSUE_REF_IN_SCOPE_RE = /#\d+/; + +/** + * Classify a clean conventional title into a changelog bucket. + * Callers that hold a full changelog bullet line (with a `* ` marker and a + * ` by @author` suffix) must strip those first; this operates on the title. + * + * @param {string} title + * @returns {'Feature'|'Fix'|'Enhancement'} + */ +function classifyBucket(title) { + const t = String(title == null ? '' : title).trim(); + if (FEATURE_RE.test(t)) return 'Feature'; + if (FIX_RE.test(t)) return 'Fix'; + return 'Enhancement'; +} + +const REQUIRED_FORMAT_MESSAGE = [ + 'PR title must follow `type(#): summary`.', + 'The type must come first (no leading tags like `[security]`) and the scope', + 'must carry the linked issue ref so the release changelog links to it.', + 'Examples: `fix(#1542): roadmap rollback`, `feat(#39)!: drop legacy flag`,', + '`enhance(#1549): add PR-title validator`.', +].join(' '); + +/** + * Validate a PR title against the convention the changelog depends on (#1549). + * + * @param {{ title?: string }} input + * @returns {{ valid: true, reason: 'valid' } + * | { valid: false, reason: 'bad-prefix'|'missing-issue-ref', message: string }} + */ +function evaluatePrTitle({ title } = {}) { + const t = String(title == null ? '' : title).trim(); + + const m = HEADER_RE.exec(t); + if (!m) { + // No clean `type[(scope)][!]:` at the start — covers leading tags, + // `Revert "..."`, empty, and freeform titles. + return { valid: false, reason: 'bad-prefix', message: REQUIRED_FORMAT_MESSAGE }; + } + + const scope = m[2]; // includes the parens, e.g. "(#1542)" — or undefined + if (!scope || !ISSUE_REF_IN_SCOPE_RE.test(scope)) { + return { valid: false, reason: 'missing-issue-ref', message: REQUIRED_FORMAT_MESSAGE }; + } + + return { valid: true, reason: 'valid' }; +} + +module.exports = { + FEATURE_RE, + FIX_RE, + HEADER_RE, + classifyBucket, + evaluatePrTitle, + REQUIRED_FORMAT_MESSAGE, +}; diff --git a/scripts/release-notes/format-github-release-notes.cjs b/scripts/release-notes/format-github-release-notes.cjs index 2116f5902..3e487b93e 100644 --- a/scripts/release-notes/format-github-release-notes.cjs +++ b/scripts/release-notes/format-github-release-notes.cjs @@ -5,6 +5,7 @@ const os = require('os'); const fs = require('fs'); const { execFileSync } = require('child_process'); const { runMain, ExitError } = require('../lib/cli-exit.cjs'); +const { classifyBucket } = require('./conventional-title.cjs'); /** * Classify a What's-Changed bullet line into 'Feature', 'Fix', or 'Enhancement'. @@ -19,9 +20,9 @@ function classifyTitle(bulletLine) { const byIdx = withoutMarker.indexOf(' by @'); const title = (byIdx !== -1 ? withoutMarker.slice(0, byIdx) : withoutMarker).trim(); - if (/^feat(?:ure)?\s*(?:\(|!|:)/i.test(title)) return 'Feature'; - if (/^fix\s*(?:\(|!|:)/i.test(title)) return 'Fix'; - return 'Enhancement'; + // Delegate to the shared matcher so the gate and the changelog can never + // disagree on bucketing (#1549 — single source of truth). + return classifyBucket(title); } /** diff --git a/tests/conventional-title.property.test.cjs b/tests/conventional-title.property.test.cjs new file mode 100644 index 000000000..d835b3c64 --- /dev/null +++ b/tests/conventional-title.property.test.cjs @@ -0,0 +1,78 @@ +'use strict'; + +/** + * Property-based tests for conventional-title.cjs + * + * Module: scripts/release-notes/conventional-title.cjs + * Exported: evaluatePrTitle({ title }), classifyBucket(title) + * + * Properties tested: + * (a) round-trip: any `type(#n): summary` (type ∈ [a-z]+, n a positive + * integer, non-empty summary) is accepted by the gate. This is the + * generative complement to the hand-picked cases in + * conventional-title.test.cjs — the convention CONTRIBUTING.md asks + * contributors to follow must never be rejected. + * (b) total function: evaluatePrTitle never throws on any string input. + * (c) classifyBucket never throws and always returns one of the 3 buckets. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('./helpers/fast-check-setup.cjs'); + +const { + evaluatePrTitle, + classifyBucket, +} = require('../scripts/release-notes/conventional-title.cjs'); + +describe('evaluatePrTitle — properties', () => { + test('(a) any well-formed `type(#n): summary` is accepted', () => { + fc.assert( + fc.property( + // type: a lowercase ascii word, e.g. fix / feat / enhance / chore + fc.stringMatching(/^[a-z]+$/).filter((s) => s.length > 0), + // n: a positive issue number + fc.integer({ min: 1, max: 1_000_000 }), + // summary: non-empty, and not all-whitespace (the title is trimmed, + // but the body after the colon is irrelevant to validity anyway) + fc.string({ minLength: 1 }).filter((s) => s.trim().length > 0), + (type, n, summary) => { + const title = `${type}(#${n}): ${summary}`; + assert.deepEqual(evaluatePrTitle({ title }), { valid: true, reason: 'valid' }); + } + ) + ); + }); + + test('(b) never throws on arbitrary string input', () => { + fc.assert( + fc.property(fc.string(), (title) => { + const r = evaluatePrTitle({ title }); + assert.equal(typeof r.valid, 'boolean'); + assert.equal(typeof r.reason, 'string'); + }) + ); + }); + + test('(b) never throws when called with no argument or a non-string title', () => { + fc.assert( + fc.property(fc.anything(), (title) => { + // evaluatePrTitle coerces title via String(...) — any payload is safe. + const r = evaluatePrTitle({ title }); + assert.equal(typeof r.valid, 'boolean'); + }) + ); + assert.equal(evaluatePrTitle().valid, false); + }); +}); + +describe('classifyBucket — properties', () => { + test('(c) always returns one of the three buckets and never throws', () => { + fc.assert( + fc.property(fc.string(), (title) => { + const bucket = classifyBucket(title); + assert.ok(['Feature', 'Fix', 'Enhancement'].includes(bucket)); + }) + ); + }); +}); diff --git a/tests/conventional-title.test.cjs b/tests/conventional-title.test.cjs new file mode 100644 index 000000000..cd410d8bc --- /dev/null +++ b/tests/conventional-title.test.cjs @@ -0,0 +1,150 @@ +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +const { + classifyBucket, + evaluatePrTitle, +} = require('../scripts/release-notes/conventional-title.cjs'); + +// The changelog classifier must consume the SAME matcher (single source of +// truth — see #1549). If someone forks the regex, this cross-check breaks. +const { + classifyTitle, +} = require('../scripts/release-notes/format-github-release-notes.cjs'); + +// --------------------------------------------------------------------------- +// classifyBucket — the shared bucket matcher (operates on a clean title) +// --------------------------------------------------------------------------- + +describe('classifyBucket', () => { + test('feat(#N): -> Feature', () => { + assert.equal(classifyBucket('feat(#39): milestone-prefixed phase IDs'), 'Feature'); + }); + + test('feature(x): -> Feature', () => { + assert.equal(classifyBucket('feature(x): something'), 'Feature'); + }); + + test('feat: -> Feature', () => { + assert.equal(classifyBucket('feat: some feature'), 'Feature'); + }); + + test('fix(#N): -> Fix', () => { + assert.equal(classifyBucket('fix(#1542): roadmap rollback'), 'Fix'); + }); + + test('fix: -> Fix', () => { + assert.equal(classifyBucket('fix: another fix'), 'Fix'); + }); + + test('chore(#N): -> Enhancement (catch-all)', () => { + assert.equal(classifyBucket('chore(#2): some chore'), 'Enhancement'); + }); + + test('untyped title -> Enhancement (catch-all)', () => { + assert.equal(classifyBucket('Main changes'), 'Enhancement'); + }); + + // Documents the mis-bucket #1549 exists to prevent at the gate: a leading + // tag defeats the `^fix` anchor, so a security fix silently files under + // Enhancement. classifyBucket faithfully reproduces this — the FIX is the + // PR-title gate (evaluatePrTitle) rejecting such titles before they land, + // not changing this catch-all (that is out of scope, flagged in #1549). + test('[security] fix(...) mis-buckets to Enhancement (the reason the gate exists)', () => { + assert.equal(classifyBucket('[security] fix(config): the #1534 case'), 'Enhancement'); + }); +}); + +// --------------------------------------------------------------------------- +// Single source of truth: the changelog classifier delegates to the shared +// matcher, so the gate and the changelog can never disagree on bucketing. +// --------------------------------------------------------------------------- + +describe('classifyTitle delegates to classifyBucket', () => { + for (const core of [ + 'feat(#39): x', + 'fix(#1): x', + 'fix(core): x', + '[security] fix(config): x', + 'chore(#2): x', + ]) { + test(`agree on bucket for ${JSON.stringify(core)}`, () => { + // classifyTitle takes a full changelog bullet line (marker + ` by @`). + const bullet = `* ${core} by @someone in https://github.com/open-gsd/gsd-core/pull/1`; + assert.equal(classifyTitle(bullet), classifyBucket(core)); + }); + } +}); + +// --------------------------------------------------------------------------- +// evaluatePrTitle — the PR-title gate (#1549) +// --------------------------------------------------------------------------- + +describe('evaluatePrTitle — valid titles', () => { + for (const title of [ + 'fix(#1542): roadmap rollback', + 'feat(#39): milestone-prefixed phase IDs', + 'enhance(#1549): add PR-title convention validator', + 'docs(#1234): clarify the title rule', + ]) { + test(`accepts ${JSON.stringify(title)}`, () => { + assert.deepEqual(evaluatePrTitle({ title }), { valid: true, reason: 'valid' }); + }); + } +}); + +describe('evaluatePrTitle — rejected titles', () => { + test('component scope without an issue ref -> missing-issue-ref', () => { + const r = evaluatePrTitle({ title: 'fix(core): six PRs like this' }); + assert.equal(r.valid, false); + assert.equal(r.reason, 'missing-issue-ref'); + }); + + test('type with colon but no scope -> missing-issue-ref', () => { + const r = evaluatePrTitle({ title: 'fix: no scope at all' }); + assert.equal(r.valid, false); + assert.equal(r.reason, 'missing-issue-ref'); + }); + + // Boundary: a scope with a `#` but zero digits. `/#\d+/` requires at least + // one digit, so `(#)` is not an issue ref — pin it so a future regex tweak + // can't silently start accepting linkless titles. + test('scope with a hash but no digits -> missing-issue-ref', () => { + const r = evaluatePrTitle({ title: 'fix(#): no digits after the hash' }); + assert.equal(r.valid, false); + assert.equal(r.reason, 'missing-issue-ref'); + }); + + test('leading tag before the type -> bad-prefix (defeats bucketing)', () => { + const r = evaluatePrTitle({ title: '[security] fix(#1534): the doubly-broken case' }); + assert.equal(r.valid, false); + assert.equal(r.reason, 'bad-prefix'); + }); + + test('no clean type prefix (auto-revert title) -> bad-prefix', () => { + const r = evaluatePrTitle({ title: 'Revert "fix(#1): something"' }); + assert.equal(r.valid, false); + assert.equal(r.reason, 'bad-prefix'); + }); + + test('empty title -> bad-prefix', () => { + const r = evaluatePrTitle({ title: '' }); + assert.equal(r.valid, false); + assert.equal(r.reason, 'bad-prefix'); + }); + + test('breaking-change marker feat(#N)!: is accepted', () => { + assert.deepEqual( + evaluatePrTitle({ title: 'feat(#42)!: drop the legacy flag' }), + { valid: true, reason: 'valid' } + ); + }); + + test('invalid results carry a human-facing message', () => { + const r = evaluatePrTitle({ title: 'fix(core): no ref' }); + assert.equal(typeof r.message, 'string'); + assert.ok(r.message.length > 0); + }); +});