enhance(#1549): validate PR-title issue-ref convention at open time (#1576)

* enhance(#1549): validate PR-title issue-ref convention at open time

The release changelog is title-driven: release.yml generates "What's Changed"
from PR titles, then format-github-release-notes.cjs buckets each line by its
conventional-commit prefix and relies on a `(#<issue>)` in the title to render
the issue link. Both rules were enforced only socially, so titles like
`fix(core): ...` (no issue link) and `[security] fix(...): ...` (leading tag
defeats the `^fix` bucket anchor -> mis-filed under Enhancement) silently broke
the changelog, landing on the maintainer as release-time cleanup.

Extract the title matcher into one shared module consumed by BOTH the changelog
classifier and a new PR-title CI gate, so a title that passes the gate cannot
mis-bucket in the changelog (single source of truth).

- scripts/lib/conventional-title.cjs (new): classifyBucket + evaluatePrTitle +
  the anchored regexes. One matcher, two consumers.
- scripts/release-notes/format-github-release-notes.cjs: classifyTitle now
  delegates to classifyBucket (behavior preserved; existing tests green).
- .github/workflows/pr-title-validator.yml (new): runs evaluatePrTitle on
  pull_request opened/edited/reopened/synchronize, for ALL authors (the drift
  came from member PRs). Trusted base-ref checkout; WARN_ONLY knob for rollout.
- tests/conventional-title.test.cjs (new): bucket + gate cases incl. the
  leading-tag mis-bucket (backfills the untested classifyTitle case) and a
  cross-check that the classifier delegates to the shared matcher.
- CONTRIBUTING.md: document the `type(#<issue>):` rule and no-leading-tag.

Claude-Session: https://claude.ai/code/session_01UMV5Qr3H4oFikbuiEauGQk

* fix(#1549): check out the PR in pr-title-validator so the new matcher resolves

The workflow checked out the base branch (next) as a trusted policy source, but
the shared matcher (scripts/lib/conventional-title.cjs) is introduced by this PR
and does not exist on next yet — so require() failed and validate-title errored
on its own introducing PR. Check out the PR's merge ref instead: the matcher
under review is present, the check is self-consistent, and a fork pull_request
runs read-only with no secrets, so running the PR's own pure-string regex is
safe.

* fix(#1549): move conventional-title.cjs out of installed scripts/lib/

bin/install.js bundles every file under scripts/lib/ into the user-installed
payload (the changeset CLI's dependencies), and install.test.cjs (#935) asserts
that exact set. The new matcher is release/CI tooling that must NOT ship to
users, so placing it in scripts/lib/ both broke the install manifest test and
would have shipped dead code. Relocate it next to its consumer in
scripts/release-notes/ (which the installer does not copy) and update the three
require paths (classifier, workflow, test) + the CONTRIBUTING reference.

install.test.cjs now 125/125; conventional-title + release-notes suites green;
lint:ci clean.

* fix(#1549): load title matcher from trusted base ref, not PR code

Addresses review (Solvely-Colin + trek-e): the gate checked out the PR
merge ref and require()'d evaluatePrTitle from PR-controlled code, so any
future PR could edit conventional-title.cjs to return { valid: true } and
wave its own malformed title through — a self-bypassable required check.

Load the matcher from a base-branch checkout instead (ref:
github.event.pull_request.base.ref), the same trusted-policy-source pattern
pr-target-validator.yml already uses. The PR can change its title but not
the ruler that measures it. An existsSync bootstrap guard skips the check
when the matcher isn't on the base branch yet (the introducing PR); every
PR after merge is fully gated. This keeps the single shared matcher (#1549's
whole point) rather than forking the regex into the workflow.

Also per review:
- add tests/conventional-title.property.test.cjs (fast-check): any
  `type(#n): summary` round-trips to valid; evaluatePrTitle/classifyBucket
  are total functions (never throw).
- pin the `fix(#):` zero-digit boundary as missing-issue-ref.

Claude-Session: https://claude.ai/code/session_01VqUHNQCh71pEqjo96zkgQL

---------

Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Rezolv
2026-06-22 22:44:44 -04:00
committed by GitHub
parent fcf44ac02a
commit 652142521b
6 changed files with 491 additions and 3 deletions

144
.github/workflows/pr-title-validator.yml vendored Normal file
View File

@@ -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 `(#<issue>)` 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(#<issue>): summary\`. No need to recreate the PR — this check`,
`re-runs when you edit the title.`,
``,
`<details><summary>Why this is enforced</summary>`,
``,
`The release changelog is built from PR titles. A leading tag mis-files`,
`the entry into the wrong section, and a scope without \`(#<issue>)\` leaves`,
`the changelog line with no link back to the issue. See issue #1549.`,
``,
`</details>`,
].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 = '<!-- pr-title-validator -->';
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}`);
}

View File

@@ -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(#<issue>): 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** — `(#<digits>)`. 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)).

View File

@@ -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:
// <type>[(<scope>)][!]:
// 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(#<issue>): 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,
};

View File

@@ -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);
}
/**

View File

@@ -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));
})
);
});
});

View File

@@ -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);
});
});