diff --git a/scripts/check-releasable-commit-types.ts b/scripts/check-releasable-commit-types.ts index 199a06e8f..f0638a0ea 100644 --- a/scripts/check-releasable-commit-types.ts +++ b/scripts/check-releasable-commit-types.ts @@ -83,16 +83,43 @@ export function isBreaking(subject: string): boolean { return /^[a-zA-Z]+(?:\([^)]*\))?!:/.test(subject.trim()); } +/** PURE. Is this file's diff nothing but a `"version"` bump in a package manifest (#10286)? + * + * release-please's own release commit is `chore(release): …` and its whole job is to write the new version + * into `/package.json` -- a path {@link publishedSourcePrefixes} matches by construction. So without + * this, the guard fires on EVERY release PR: the one commit shape nobody hand-writes, that a maintainer + * therefore cannot fix by rewording, and whose flagged "would never reach npm" claim is exactly backwards + * (it is the commit that performs the release). That is precisely the ordinary-case firing this file's own + * `isPublishedFile` note warns gets a guard switched off. + * + * Deliberately narrower than matching the `chore(release):` subject: a hand-written commit that borrows the + * subject while editing a dependency range or `exports` map is still a real stranded release, so the + * exemption is keyed on what the diff DID, not on what the subject claims. An empty diff (the default + * accessor, or a caller that cannot supply one) proves nothing and stays flagged. */ +export function isVersionOnlyManifestBump(file: string, diff: string): boolean { + if (!file.endsWith("/package.json")) return false; + const changed = diff + .split("\n") + .filter((line) => /^[+-]/.test(line) && !/^(\+\+\+|---)/.test(line)); + if (changed.length === 0) return false; + return changed.every((line) => /^[+-]\s*"version":\s*"[^"]*",?\s*$/.test(line)); +} + /** * PURE. The commits that change published source under a type release-please will not release. * * A commit carrying a `Release-As:` footer is exempt: that is release-please's own documented mechanism for * forcing a version, so a commit using it has already answered this check's question. + * + * A path whose only change is a manifest version bump is dropped from consideration (#10286) -- see + * {@link isVersionOnlyManifestBump}. A commit left with no other published-source path is release-please's + * own release commit and is not stranded. */ export function findStrandedCommits( commits: readonly CommitUnderReview[], config: ReleasePleaseConfig, bodyOf: (sha: string) => string = () => "", + diffOf: (sha: string, file: string) => string = () => "", ): StrandedCommit[] { const hidden = hiddenCommitTypes(config); const prefixes = publishedSourcePrefixes(config); @@ -102,8 +129,10 @@ export function findStrandedCommits( if (type === null || !hidden.has(type) || isBreaking(commit.subject)) continue; const paths = commit.files.filter((file) => isPublishedFile(file) && prefixes.some((prefix) => file.startsWith(prefix))); if (paths.length === 0) continue; + const releasable = paths.filter((file) => !isVersionOnlyManifestBump(file, diffOf(commit.sha, file))); + if (releasable.length === 0) continue; if (/^\s*Release-As:/im.test(bodyOf(commit.sha))) continue; - stranded.push({ sha: commit.sha, subject: commit.subject, type, paths }); + stranded.push({ sha: commit.sha, subject: commit.subject, type, paths: releasable }); } return stranded; } @@ -165,7 +194,12 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) process.exit(0); } const config = readConfig(); - const stranded = findStrandedCommits(commits, config, (sha) => git(["log", "-1", "--format=%b", sha])); + const stranded = findStrandedCommits( + commits, + config, + (sha) => git(["log", "-1", "--format=%b", sha]), + (sha, file) => git(["show", "--format=", "--unified=0", sha, "--", file]), + ); if (stranded.length === 0) { process.stdout.write(`releasable-commit-types: ${commits.length} commit(s) checked, none would be stranded.\n`); process.exit(0); diff --git a/test/unit/check-releasable-commit-types.test.ts b/test/unit/check-releasable-commit-types.test.ts index f7bcd31a9..24b0769ef 100644 --- a/test/unit/check-releasable-commit-types.test.ts +++ b/test/unit/check-releasable-commit-types.test.ts @@ -7,6 +7,7 @@ import { hiddenCommitTypes, isBreaking, isPublishedFile, + isVersionOnlyManifestBump, publishedSourcePrefixes, readConfig, type CommitUnderReview, @@ -156,6 +157,72 @@ describe("findStrandedCommits", () => { expect(findStrandedCommits([commit()], CONFIG, () => "we could use Release-As: here")).toHaveLength(1); }); + // #10286: release-please's own release commit is `chore(release):` and writes /package.json, which + // publishedSourcePrefixes matches by construction -- so before this, the guard fired on every release PR. + it("REGRESSION: allows release-please's own release commit -- a version-only manifest bump", () => { + const releaseCommit = commit({ + subject: "chore(release): cut ui-kit v1.7.0", + files: ["packages/loopover-ui-kit/package.json"], + }); + const diff = ['- "version": "1.6.0",', '+ "version": "1.7.0",'].join("\n"); + expect(findStrandedCommits([releaseCommit], CONFIG, () => "", () => diff)).toEqual([]); + }); + + it("still flags a chore that changes a manifest BEYOND its version", () => { + // The reason the exemption keys on the diff rather than the `chore(release):` subject: a dependency range + // is part of what a consumer resolves, so stranding one is the very bug this guard exists for. + const depEdit = commit({ + subject: "chore(release): cut ui-kit v1.7.0", + files: ["packages/loopover-ui-kit/package.json"], + }); + const diff = ['- "version": "1.6.0",', '+ "version": "1.7.0",', '- "recharts": "^3.9.0"', '+ "recharts": "^3.10.1"'].join("\n"); + expect(findStrandedCommits([depEdit], CONFIG, () => "", () => diff)).toHaveLength(1); + }); + + it("reports only the paths that are not version-only bumps when a commit mixes both", () => { + const mixed = commit({ + subject: "chore(release): cut ui-kit v1.7.0", + files: ["packages/loopover-ui-kit/package.json", "packages/loopover-ui-kit/src/components/chart.tsx"], + }); + const diffOf = (_sha: string, file: string) => + file.endsWith("package.json") ? '- "version": "1.6.0",\n+ "version": "1.7.0",' : "-old\n+new"; + const [stranded] = findStrandedCommits([mixed], CONFIG, () => "", diffOf); + expect(stranded?.paths).toEqual(["packages/loopover-ui-kit/src/components/chart.tsx"]); + }); + + it("keeps flagging when no diff is available -- an unprovable exemption is not an exemption", () => { + const releaseCommit = commit({ + subject: "chore(release): cut ui-kit v1.7.0", + files: ["packages/loopover-ui-kit/package.json"], + }); + expect(findStrandedCommits([releaseCommit], CONFIG)).toHaveLength(1); + }); +}); + +describe("isVersionOnlyManifestBump", () => { + const bump = '- "version": "1.6.0",\n+ "version": "1.7.0",'; + + it("accepts a manifest whose only changed lines are the version field", () => { + expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", bump)).toBe(true); + }); + + it("ignores the diff header lines rather than counting them as changes", () => { + const withHeader = ["--- a/packages/loopover-ui-kit/package.json", "+++ b/packages/loopover-ui-kit/package.json", bump].join("\n"); + expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", withHeader)).toBe(true); + }); + + it("rejects a non-manifest file however its diff reads", () => { + expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/src/version.ts", bump)).toBe(false); + }); + + it("rejects an empty diff -- proves nothing, so it cannot exempt", () => { + expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", "")).toBe(false); + }); + + it("rejects a manifest diff carrying any non-version change", () => { + expect(isVersionOnlyManifestBump("packages/loopover-ui-kit/package.json", `${bump}\n+ "sideEffects": false,`)).toBe(false); + }); + it("ignores a non-conventional subject rather than guessing at its type", () => { expect(findStrandedCommits([commit({ subject: "merge branch main" })], CONFIG)).toEqual([]); });