Skip to content

Commit 362cac1

Browse files
authored
ci: require maintainer approval for a major changeset (#178)
## Related Issue No issue — this came out of reviewing how `2.0.0` was produced. ## Problem `@pymodel/pythinker-code@2.0.0` is set by exactly one file, `.changeset/remove-built-in-hosted-provider.md`, which declares `major`. It arrived inside a 19-changeset squash, and nothing in CI looked at it. `.agents/skills/gen-changesets/SKILL.md` already says a `major` is never a mechanical decision — "stop, explain, and get explicit user confirmation first". That was a convention with no enforcement, so a `major` can rename the release on its own. npm publishes are irreversible, so the wrong version number is not something a follow-up commit can take back. ## What changed A pull request that **adds** a `major` changeset now fails unless it carries the `breaking-change-approved` label. - `scripts/release/check-major-changeset.mjs` — `parseBumpLevels` reads only the frontmatter, so changelog prose that happens to contain a `key: value` line is not mistaken for a bump. `evaluate` is pure and takes its file reader by injection. - `.github/workflows/changeset-policy.yml` — runs on `labeled`/`unlabeled` as well as pushes, so applying the label re-runs the check instead of leaving a stale failure. - The `breaking-change-approved` label has been created on the repository. Only **added** changesets count. Editing prose in a `major` that already sits on the base branch is not a new decision, and re-gating it would block every follow-up that touches the file. The gate self-tests before it runs (`--self-test`, 16 cases): a silent break in the check that decides what ships is worse than a red pull request. Widening the frontmatter slice to the whole file makes the suite fail, so the cases pin real behaviour rather than restating the implementation. Both workflow inputs reach the script through `env:` and are never interpolated into `run:`. Labels are passed as `toJSON(...)` so a comma inside a label cannot smuggle a second name, and the diff uses `base.sha` rather than the branch name, so no caller-chosen text reaches git. Replaying the pull request that introduced the current `major`: ``` majors: [ '.changeset/remove-built-in-hosted-provider.md' ] ok: false with label ok: true ``` ## Checklist - [x] I have read the [CONTRIBUTING](https://github.com/PyModel/pythinker-code/blob/main/CONTRIBUTING.md) document. - [ ] I have linked a related issue (external PRs: the issue must have a maintainer's `/approve`). - [x] I have added tests that prove my feature works. - [x] Ran `gen-changesets` skill, or this PR needs no changeset. - [x] Ran `gen-docs` skill, or this PR needs no doc update. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated checks for pull requests introducing major release changes. * Major changes now require a `breaking-change-approved` label before merging. * Checks run when pull requests are updated or labels change. * Existing major-version packages are handled appropriately without requiring additional approval. * Added validation and self-tests to ensure release checks operate reliably. * Irrelevant files and deleted changesets are excluded from approval checks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 199eaa9 commit 362cac1

2 files changed

Lines changed: 318 additions & 0 deletions

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
name: Changeset policy
2+
3+
# A major bump renames the release and breaks every pinned consumer, and npm
4+
# publishes are irreversible. `gen-changesets` already says a major is never an
5+
# agent's call; this makes that a merge gate rather than a convention.
6+
#
7+
# `labeled`/`unlabeled` are listed so adding the approval label re-runs the
8+
# check instead of leaving a stale red on the pull request.
9+
on:
10+
pull_request:
11+
types: [opened, synchronize, reopened, labeled, unlabeled]
12+
13+
permissions:
14+
contents: read
15+
16+
jobs:
17+
major-bump-approval:
18+
name: Major bump needs approval
19+
runs-on: ubuntu-latest
20+
timeout-minutes: 5
21+
22+
steps:
23+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # pinned from v6.0.2
24+
with:
25+
# The gate diffs against the base commit, which a shallow clone of
26+
# the merge ref alone does not contain.
27+
fetch-depth: 0
28+
29+
- uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # pinned from v7.0.0
30+
with:
31+
node-version-file: .nvmrc
32+
33+
# The gate decides what ships, so a silent break in the gate is worse
34+
# than a red pull request. Its own cases run first.
35+
- name: Self-test the gate
36+
run: node scripts/release/check-major-changeset.mjs --self-test
37+
38+
# Both values reach the script through the environment and are never
39+
# interpolated into a shell command. `base.sha` is used rather than the
40+
# branch name so no caller-chosen text reaches git at all.
41+
- name: Check for an unapproved major changeset
42+
env:
43+
BASE_SHA: ${{ github.event.pull_request.base.sha }}
44+
PR_LABELS_JSON: ${{ toJSON(github.event.pull_request.labels.*.name) }}
45+
run: node scripts/release/check-major-changeset.mjs
Lines changed: 273 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
1+
/**
2+
* Gate: a `major` changeset needs a human decision, recorded on the pull
3+
* request.
4+
*
5+
* `.agents/skills/gen-changesets/SKILL.md` already says never to choose a
6+
* `major` bump alone — stop and get explicit approval. Nothing enforced it, so
7+
* a `major` could ride into `main` inside a large squash and set the next
8+
* release's version on its own. This turns that rule into a check: a pull
9+
* request that ADDS a `major` changeset fails unless it carries the approval
10+
* label.
11+
*
12+
* Only added files count. Editing prose in a `major` changeset that is already
13+
* on the base branch is not a new decision, and re-gating it would block every
14+
* follow-up touching the same file.
15+
*/
16+
17+
import { readFileSync } from 'node:fs';
18+
import { execFileSync } from 'node:child_process';
19+
20+
export const APPROVAL_LABEL = 'breaking-change-approved';
21+
22+
/**
23+
* Read the bump levels a changeset declares.
24+
*
25+
* The frontmatter is the block between the first two `---` fences; each entry
26+
* reads `"package": level`. Anything outside that block is the changelog prose
27+
* and must not be scanned — a body that mentions the word "major" is not a
28+
* `major` bump.
29+
*
30+
* @param source - Raw changeset file contents.
31+
* @returns The declared levels, lowercased, in file order.
32+
*/
33+
export function parseBumpLevels(source) {
34+
const normalized = source.replaceAll('\r\n', '\n');
35+
if (!normalized.startsWith('---\n')) return [];
36+
const end = normalized.indexOf('\n---', 3);
37+
if (end === -1) return [];
38+
const frontmatter = normalized.slice(4, end + 1);
39+
40+
const levels = [];
41+
for (const line of frontmatter.split('\n')) {
42+
const match = /^\s*(?:"[^"]+"|'[^']+'|[^:]+)\s*:\s*([A-Za-z]+)\s*$/u.exec(line);
43+
if (match !== null) levels.push(match[1].toLowerCase());
44+
}
45+
return levels;
46+
}
47+
48+
/** Changeset paths, ignoring the directory's own README and config. */
49+
export function isChangesetFile(path) {
50+
return path.startsWith('.changeset/') && path.endsWith('.md') && !path.endsWith('/README.md');
51+
}
52+
53+
/**
54+
* Decide whether the gate passes.
55+
*
56+
* A changeset counts against the pull request when it declares `major` now and
57+
* did not already declare it on the base branch. Editing an existing changeset
58+
* up to `major` therefore counts, while merely touching one that was already
59+
* approved does not ask for the label a second time.
60+
*
61+
* @param input.changedFiles - Changeset paths the pull request adds or edits.
62+
* @param input.labels - Label names on the pull request.
63+
* @param input.readFile - Reads one path at the pull request head.
64+
* @param input.readBaseFile - Reads one path on the base branch, or undefined
65+
* when the path does not exist there. Injected so this stays pure.
66+
* @returns The offending changesets and whether they are approved.
67+
*/
68+
export function evaluate(input) {
69+
const majors = input.changedFiles.filter(isChangesetFile).filter((path) => {
70+
if (!parseBumpLevels(input.readFile(path)).includes('major')) return false;
71+
const base = input.readBaseFile(path);
72+
return base === undefined || !parseBumpLevels(base).includes('major');
73+
});
74+
const approved = input.labels.includes(APPROVAL_LABEL);
75+
return { majors, approved, ok: majors.length === 0 || approved };
76+
}
77+
78+
function selfTest() {
79+
const cases = [
80+
{ name: 'major in frontmatter', source: '---\n"@pymodel/pythinker-code": major\n---\n\nDrop it.\n', expected: ['major'] },
81+
{ name: 'minor only', source: '---\n"@pymodel/pythinker-code": minor\n---\n\nAdd it.\n', expected: ['minor'] },
82+
// The body can hold a `key: value` line of its own; only the frontmatter
83+
// declares bumps, so the boundary has to be respected, not just the words.
84+
{ name: 'prose with a colon line', source: '---\n"a": patch\n---\n\nBreaking: major\n', expected: ['patch'] },
85+
{ name: 'crlf frontmatter', source: '---\r\n"a": major\r\n---\r\n\r\nText.\r\n', expected: ['major'] },
86+
{ name: 'multi package', source: '---\n"a": patch\n"b": major\n---\n\nText.\n', expected: ['patch', 'major'] },
87+
{ name: 'no frontmatter', source: 'Just prose about a major change.\n', expected: [] },
88+
{ name: 'unterminated frontmatter', source: '---\n"a": major\n', expected: [] },
89+
];
90+
let failures = 0;
91+
for (const { name, source, expected } of cases) {
92+
const actual = parseBumpLevels(source);
93+
if (JSON.stringify(actual) !== JSON.stringify(expected)) {
94+
console.error(`self-test FAILED: ${name} — expected ${JSON.stringify(expected)}, got ${JSON.stringify(actual)}`);
95+
failures += 1;
96+
}
97+
}
98+
99+
const MAJOR = '---\n"a": major\n---\n\nText.\n';
100+
const MINOR = '---\n"a": minor\n---\n\nText.\n';
101+
const files = { '.changeset/a.md': MAJOR };
102+
const readFile = (path) => files[path];
103+
const absentFromBase = () => undefined;
104+
105+
const gateCases = [
106+
{
107+
name: 'an unlabelled new major is blocked',
108+
input: { changedFiles: ['.changeset/a.md'], labels: [], readFile, readBaseFile: absentFromBase },
109+
ok: false,
110+
majors: 1,
111+
},
112+
{
113+
name: 'a labelled new major passes',
114+
input: {
115+
changedFiles: ['.changeset/a.md'],
116+
labels: [APPROVAL_LABEL],
117+
readFile,
118+
readBaseFile: absentFromBase,
119+
},
120+
ok: true,
121+
majors: 1,
122+
},
123+
{
124+
name: 'the changeset README is not a changeset',
125+
input: {
126+
changedFiles: ['.changeset/README.md'],
127+
labels: [],
128+
readFile: () => MAJOR,
129+
readBaseFile: absentFromBase,
130+
},
131+
ok: true,
132+
majors: 0,
133+
},
134+
// The escape this gate exists to close: the file is not new, so a filter on
135+
// added paths alone would never see the bump rise from minor to major.
136+
{
137+
name: 'editing an existing changeset up to major is blocked',
138+
input: { changedFiles: ['.changeset/a.md'], labels: [], readFile, readBaseFile: () => MINOR },
139+
ok: false,
140+
majors: 1,
141+
},
142+
{
143+
name: 'touching an already-major changeset does not re-ask for the label',
144+
input: { changedFiles: ['.changeset/a.md'], labels: [], readFile, readBaseFile: () => MAJOR },
145+
ok: true,
146+
majors: 0,
147+
},
148+
{
149+
name: 'editing a changeset that stays below major passes',
150+
input: {
151+
changedFiles: ['.changeset/a.md'],
152+
labels: [],
153+
readFile: () => MINOR,
154+
readBaseFile: () => MINOR,
155+
},
156+
ok: true,
157+
majors: 0,
158+
},
159+
];
160+
161+
for (const { name, input, ok, majors } of gateCases) {
162+
const actual = evaluate(input);
163+
if (actual.ok !== ok || actual.majors.length !== majors) {
164+
console.error(
165+
`self-test FAILED: ${name} — expected ok=${ok} majors=${majors}, got ok=${actual.ok} majors=${actual.majors.length}`,
166+
);
167+
failures += 1;
168+
}
169+
}
170+
171+
const labelCases = [
172+
{ name: 'absent', raw: undefined, expected: [] },
173+
{ name: 'empty', raw: '', expected: [] },
174+
{ name: 'json array', raw: '["a","breaking-change-approved"]', expected: ['a', APPROVAL_LABEL] },
175+
{ name: 'not json', raw: 'breaking-change-approved', expected: [] },
176+
{ name: 'not an array', raw: '{"name":"x"}', expected: [] },
177+
{ name: 'non-string members', raw: '[1,"a"]', expected: ['a'] },
178+
];
179+
for (const { name, raw, expected } of labelCases) {
180+
const actual = parseLabels(raw);
181+
if (JSON.stringify(actual) !== JSON.stringify(expected)) {
182+
console.error(`self-test FAILED: labels ${name} — expected ${JSON.stringify(expected)}, got ${JSON.stringify(actual)}`);
183+
failures += 1;
184+
}
185+
}
186+
187+
if (failures > 0) process.exit(1);
188+
console.log(`check-major-changeset: self-test OK (${cases.length + labelCases.length + gateCases.length} cases)`);
189+
}
190+
191+
/**
192+
* Label names as the workflow passes them: a JSON array, so a label containing
193+
* a comma or a newline cannot smuggle in a second name.
194+
*
195+
* @param raw - The `PR_LABELS_JSON` value, or undefined when unset.
196+
* @returns The label names; empty when the value is absent or not an array.
197+
*/
198+
export function parseLabels(raw) {
199+
if (raw === undefined || raw.length === 0) return [];
200+
let parsed;
201+
try {
202+
parsed = JSON.parse(raw);
203+
} catch {
204+
return [];
205+
}
206+
return Array.isArray(parsed) ? parsed.filter((name) => typeof name === 'string') : [];
207+
}
208+
209+
/**
210+
* Changeset paths the pull request adds, edits, or renames into place.
211+
*
212+
* `--diff-filter=d` keeps every status except deletion, so an edit that raises
213+
* an existing changeset to `major` is reported alongside a brand new one. A
214+
* deleted changeset cannot introduce a bump, so it is the only status dropped.
215+
*/
216+
function changedFilesAgainst(baseSha) {
217+
const output = execFileSync(
218+
'git',
219+
['diff', '--name-only', '--diff-filter=d', `${baseSha}...HEAD`, '--', '.changeset'],
220+
{ encoding: 'utf8' },
221+
);
222+
return output.split('\n').filter((line) => line.length > 0);
223+
}
224+
225+
/** The same path on the base branch, or undefined when it is new there. */
226+
function readBaseFile(baseSha, path) {
227+
try {
228+
return execFileSync('git', ['show', `${baseSha}:${path}`], { encoding: 'utf8' });
229+
} catch {
230+
return undefined;
231+
}
232+
}
233+
234+
function main() {
235+
if (process.argv.includes('--self-test')) {
236+
selfTest();
237+
return;
238+
}
239+
240+
const baseSha = process.env['BASE_SHA'];
241+
if (baseSha === undefined || !/^[0-9a-f]{7,40}$/u.test(baseSha)) {
242+
console.error('check-major-changeset: BASE_SHA must be the pull request base commit.');
243+
process.exit(1);
244+
}
245+
246+
const result = evaluate({
247+
changedFiles: changedFilesAgainst(baseSha),
248+
labels: parseLabels(process.env['PR_LABELS_JSON']),
249+
readFile: (path) => readFileSync(path, 'utf8'),
250+
readBaseFile: (path) => readBaseFile(baseSha, path),
251+
});
252+
253+
if (result.ok) {
254+
const note = result.majors.length === 0 ? 'no new major changeset' : 'major approved by label';
255+
console.log(`check-major-changeset: OK (${note})`);
256+
return;
257+
}
258+
259+
console.error('check-major-changeset: FAILED');
260+
console.error('');
261+
console.error('This pull request declares a major changeset:');
262+
for (const path of result.majors) console.error(` - ${path}`);
263+
console.error('');
264+
console.error('A major bump is a product decision, not a mechanical one: it renames the');
265+
console.error('release, breaks every consumer who upgrades to it, and cannot be walked');
266+
console.error('back once published. Either lower the bump to minor or patch, or have a');
267+
console.error(`maintainer add the "${APPROVAL_LABEL}" label to confirm the break is intended.`);
268+
process.exit(1);
269+
}
270+
271+
if (process.argv[1] !== undefined && import.meta.url.endsWith(process.argv[1].replaceAll('\\', '/'))) {
272+
main();
273+
}

0 commit comments

Comments
 (0)