Skip to content

Commit 321ef8f

Browse files
committed
fix(ci): stop releasable-commit-types flagging release-please's own release commit
publishedSourcePrefixes includes <pkg>/package.json, and a release-please release commit is `chore(release): …` whose entire job is to write the new version into exactly that file. So the guard matched by construction on every release PR it ever ran against -- #10283 surfaced it, where the flagged claim ("would merge green and then never reach npm") is exactly backwards: that commit is the one that performs the release, and a maintainer cannot reword it because release-please generates it. This file's own isPublishedFile note already warns that a guard firing on the ordinary case is a guard that gets disabled. The exemption keys on what the diff DID, not on the subject: a commit borrowing the `chore(release):` subject while editing a dependency range or an exports map still strands a real change, so only a manifest whose sole changed lines are the "version" field is dropped. A commit left with no other published-source path is release-please's own and is not stranded; a mixed commit still reports its remaining paths. An empty diff proves nothing and stays flagged, which keeps the default accessor's behaviour unchanged. Closes #10286
1 parent 7f27a0f commit 321ef8f

2 files changed

Lines changed: 103 additions & 2 deletions

File tree

scripts/check-releasable-commit-types.ts

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,16 +83,43 @@ export function isBreaking(subject: string): boolean {
8383
return /^[a-zA-Z]+(?:\([^)]*\))?!:/.test(subject.trim());
8484
}
8585

86+
/** PURE. Is this file's diff nothing but a `"version"` bump in a package manifest (#10286)?
87+
*
88+
* release-please's own release commit is `chore(release): …` and its whole job is to write the new version
89+
* into `<pkg>/package.json` -- a path {@link publishedSourcePrefixes} matches by construction. So without
90+
* this, the guard fires on EVERY release PR: the one commit shape nobody hand-writes, that a maintainer
91+
* therefore cannot fix by rewording, and whose flagged "would never reach npm" claim is exactly backwards
92+
* (it is the commit that performs the release). That is precisely the ordinary-case firing this file's own
93+
* `isPublishedFile` note warns gets a guard switched off.
94+
*
95+
* Deliberately narrower than matching the `chore(release):` subject: a hand-written commit that borrows the
96+
* subject while editing a dependency range or `exports` map is still a real stranded release, so the
97+
* exemption is keyed on what the diff DID, not on what the subject claims. An empty diff (the default
98+
* accessor, or a caller that cannot supply one) proves nothing and stays flagged. */
99+
export function isVersionOnlyManifestBump(file: string, diff: string): boolean {
100+
if (!file.endsWith("/package.json")) return false;
101+
const changed = diff
102+
.split("\n")
103+
.filter((line) => /^[+-]/.test(line) && !/^(\+\+\+|---)/.test(line));
104+
if (changed.length === 0) return false;
105+
return changed.every((line) => /^[+-]\s*"version":\s*"[^"]*",?\s*$/.test(line));
106+
}
107+
86108
/**
87109
* PURE. The commits that change published source under a type release-please will not release.
88110
*
89111
* A commit carrying a `Release-As:` footer is exempt: that is release-please's own documented mechanism for
90112
* forcing a version, so a commit using it has already answered this check's question.
113+
*
114+
* A path whose only change is a manifest version bump is dropped from consideration (#10286) -- see
115+
* {@link isVersionOnlyManifestBump}. A commit left with no other published-source path is release-please's
116+
* own release commit and is not stranded.
91117
*/
92118
export function findStrandedCommits(
93119
commits: readonly CommitUnderReview[],
94120
config: ReleasePleaseConfig,
95121
bodyOf: (sha: string) => string = () => "",
122+
diffOf: (sha: string, file: string) => string = () => "",
96123
): StrandedCommit[] {
97124
const hidden = hiddenCommitTypes(config);
98125
const prefixes = publishedSourcePrefixes(config);
@@ -102,8 +129,10 @@ export function findStrandedCommits(
102129
if (type === null || !hidden.has(type) || isBreaking(commit.subject)) continue;
103130
const paths = commit.files.filter((file) => isPublishedFile(file) && prefixes.some((prefix) => file.startsWith(prefix)));
104131
if (paths.length === 0) continue;
132+
const releasable = paths.filter((file) => !isVersionOnlyManifestBump(file, diffOf(commit.sha, file)));
133+
if (releasable.length === 0) continue;
105134
if (/^\s*Release-As:/im.test(bodyOf(commit.sha))) continue;
106-
stranded.push({ sha: commit.sha, subject: commit.subject, type, paths });
135+
stranded.push({ sha: commit.sha, subject: commit.subject, type, paths: releasable });
107136
}
108137
return stranded;
109138
}
@@ -165,7 +194,12 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href)
165194
process.exit(0);
166195
}
167196
const config = readConfig();
168-
const stranded = findStrandedCommits(commits, config, (sha) => git(["log", "-1", "--format=%b", sha]));
197+
const stranded = findStrandedCommits(
198+
commits,
199+
config,
200+
(sha) => git(["log", "-1", "--format=%b", sha]),
201+
(sha, file) => git(["show", "--format=", "--unified=0", sha, "--", file]),
202+
);
169203
if (stranded.length === 0) {
170204
process.stdout.write(`releasable-commit-types: ${commits.length} commit(s) checked, none would be stranded.\n`);
171205
process.exit(0);

test/unit/check-releasable-commit-types.test.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
hiddenCommitTypes,
88
isBreaking,
99
isPublishedFile,
10+
isVersionOnlyManifestBump,
1011
publishedSourcePrefixes,
1112
readConfig,
1213
type CommitUnderReview,
@@ -156,6 +157,72 @@ describe("findStrandedCommits", () => {
156157
expect(findStrandedCommits([commit()], CONFIG, () => "we could use Release-As: here")).toHaveLength(1);
157158
});
158159

160+
// #10286: release-please's own release commit is `chore(release):` and writes <pkg>/package.json, which
161+
// publishedSourcePrefixes matches by construction -- so before this, the guard fired on every release PR.
162+
it("REGRESSION: allows release-please's own release commit -- a version-only manifest bump", () => {
163+
const releaseCommit = commit({
164+
subject: "chore(release): cut ui-kit v1.7.0",
165+
files: ["packages/loopover-ui-kit/package.json"],
166+
});
167+
const diff = ['- "version": "1.6.0",', '+ "version": "1.7.0",'].join("\n");
168+
expect(findStrandedCommits([releaseCommit], CONFIG, () => "", () => diff)).toEqual([]);
169+
});
170+
171+
it("still flags a chore that changes a manifest BEYOND its version", () => {
172+
// The reason the exemption keys on the diff rather than the `chore(release):` subject: a dependency range
173+
// is part of what a consumer resolves, so stranding one is the very bug this guard exists for.
174+
const depEdit = commit({
175+
subject: "chore(release): cut ui-kit v1.7.0",
176+
files: ["packages/loopover-ui-kit/package.json"],
177+
});
178+
const diff = ['- "version": "1.6.0",', '+ "version": "1.7.0",', '- "recharts": "^3.9.0"', '+ "recharts": "^3.10.1"'].join("\n");
179+
expect(findStrandedCommits([depEdit], CONFIG, () => "", () => diff)).toHaveLength(1);
180+
});
181+
182+
it("reports only the paths that are not version-only bumps when a commit mixes both", () => {
183+
const mixed = commit({
184+
subject: "chore(release): cut ui-kit v1.7.0",
185+
files: ["packages/loopover-ui-kit/package.json", "packages/loopover-ui-kit/src/components/chart.tsx"],
186+
});
187+
const diffOf = (_sha: string, file: string) =>
188+
file.endsWith("package.json") ? '- "version": "1.6.0",\n+ "version": "1.7.0",' : "-old\n+new";
189+
const [stranded] = findStrandedCommits([mixed], CONFIG, () => "", diffOf);
190+
expect(stranded?.paths).toEqual(["packages/loopover-ui-kit/src/components/chart.tsx"]);
191+
});
192+
193+
it("keeps flagging when no diff is available -- an unprovable exemption is not an exemption", () => {
194+
const releaseCommit = commit({
195+
subject: "chore(release): cut ui-kit v1.7.0",
196+
files: ["packages/loopover-ui-kit/package.json"],
197+
});
198+
expect(findStrandedCommits([releaseCommit], CONFIG)).toHaveLength(1);
199+
});
200+
});
201+
202+
describe("isVersionOnlyManifestBump", () => {
203+
const bump = '- "version": "1.6.0",\n+ "version": "1.7.0",';
204+
205+
it("accepts a manifest whose only changed lines are the version field", () => {
206+
expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", bump)).toBe(true);
207+
});
208+
209+
it("ignores the diff header lines rather than counting them as changes", () => {
210+
const withHeader = ["--- a/packages/loopover-ui-kit/package.json", "+++ b/packages/loopover-ui-kit/package.json", bump].join("\n");
211+
expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", withHeader)).toBe(true);
212+
});
213+
214+
it("rejects a non-manifest file however its diff reads", () => {
215+
expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/src/version.ts", bump)).toBe(false);
216+
});
217+
218+
it("rejects an empty diff -- proves nothing, so it cannot exempt", () => {
219+
expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", "")).toBe(false);
220+
});
221+
222+
it("rejects a manifest diff carrying any non-version change", () => {
223+
expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", `${bump}\n+ "sideEffects": false,`)).toBe(false);
224+
});
225+
159226
it("ignores a non-conventional subject rather than guessing at its type", () => {
160227
expect(findStrandedCommits([commit({ subject: "merge branch main" })], CONFIG)).toEqual([]);
161228
});

0 commit comments

Comments
 (0)