From 1262bb8477b75d4ea9d8f552b79d5ac92b5e6f36 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Thu, 10 Sep 2026 13:13:02 +0530 Subject: [PATCH 01/17] fix(release): record the head each regeneration discards Closes the loop #342 opened and #350 tracked. Release Please regenerates its branch by force-pushing, GitHub records no `before_commit_id` for it, and the discarded head is then unnameable: an ordinary push later rewritten away leaves no timeline entry, and a head nobody reviewed is named by no review. The review gate cannot prove a finding recorded only on that head was carried forward, so it refuses -- correctly, and permanently. Confirmed live on #337 after #351 landed: conclusion=action_required summary=Review gate a rewrite did not record the head it discarded, and the newest recoverable review state predates that rewrite, so a finding recorded only on the discarded head cannot be ruled out. The workflow performing the rewrite is ours, so it can record what GitHub does not. `run-release-please.mjs` reads the release branch heads immediately before the regeneration, while the head about to be discarded is still addressable, and posts a marker naming both heads when one actually moved. The gate reads that marker and treats the rewrite as identified. This supplies a name, not permission. A head named by a marker is still searched for durable state belonging to that pull request, and a marker pointing at a head with no state leaves the gate exactly as unconvinced as before. That is the distinction from #343, which exempted generated branches from the check outright and was withdrawn for it. Three boundaries, each tested: - Only `github-actions[bot]` markers count. Anyone who can comment can write the text, so the author is the whole of its authority, and no human can post under that login. - A branch created rather than rewritten, or one whose head did not move, discarded nothing and gets no marker. - Two markers naming different discarded heads for one created head are two claims about one rewrite with no basis for preferring either, so the gate refuses instead of choosing. The wrapper's "without contacting GitHub" test asserted a guarantee this feature ends. Rather than delete it, it now stubs `fetch` and asserts exactly which calls happen: two branch-head reads bracketing the regeneration, and no writes when nothing was rewritten. Mutation-tested; every one is caught: accept a rewrite record from any author -> 1 test fails ignore recorded discards entirely -> 1 test fails record a marker when the head did not move -> 1 test fails accept conflicting rewrite records -> 1 test fails Does not retroactively unblock #337: its four rewrites already happened unrecorded. That needs its branch re-created, or the corrections carried on a fresh regeneration once this is live. Co-Authored-By: Claude Opus 5 --- scripts/CLAUDE.md | 13 ++ scripts/lib/release-branch-rewrite.mjs | 32 +++++ scripts/publish-review-gate-check.mjs | 39 +++++- scripts/run-release-please.d.mts | 9 ++ scripts/run-release-please.mjs | 76 +++++++++++ .../scripts/publish-review-gate-check.test.ts | 118 ++++++++++++++++++ tests/scripts/run-release-please.test.ts | 97 ++++++++++++++ 7 files changed, 383 insertions(+), 1 deletion(-) create mode 100644 scripts/lib/release-branch-rewrite.mjs diff --git a/scripts/CLAUDE.md b/scripts/CLAUDE.md index 65a3180d..ecda14e7 100644 --- a/scripts/CLAUDE.md +++ b/scripts/CLAUDE.md @@ -31,6 +31,9 @@ Verified against actual file contents and `.github/workflows/*.yml` on 2026-07-0 - `check-pr-review-gate.mjs` — polls `gh` for PR review state, optionally requiring a named reviewer/strict head review; run by review-gate.yml and locally via `pnpm review:gate`. +- `publish-review-gate-check.mjs` — publishes the `Review gate (scheduled)` check on + every open PR head, carrying durable review state forward across force-pushes + (review-gate-reconcile.yml). ### Local-only (not invoked by any workflow) @@ -45,6 +48,16 @@ Verified against actual file contents and `.github/workflows/*.yml` on 2026-07-0 ### Supporting library +- `lib/codex-review-markers.mjs` — the pattern Codex writes on a clean top-level + review, and the reviewed commit it names. Read by `check-pr-review-gate.mjs` (as a + current-head review) and `publish-review-gate-check.mjs` (as a head this PR had); one + definition because two copies disagreeing about which commits were reviewed is the + discontinuity the gate exists to detect. +- `lib/release-branch-rewrite.mjs` — the marker a generated-branch regeneration writes + to record the head it discarded, which GitHub omits (`before_commit_id` is null). + Written by `run-release-please.mjs`, read by `publish-review-gate-check.mjs`. It + supplies a name, not permission: the named commit is still searched, and only a + marker written by `github-actions[bot]` counts. - `lib/live-run-evidence-redaction-patterns.mjs` (+ `.d.mts`) and `lib/live-run-evidence-redaction.ts` / `live-run-evidence-types.ts` / `live-run-evidence.ts` — shared redaction pattern definitions and types used by the live-evidence and diff --git a/scripts/lib/release-branch-rewrite.mjs b/scripts/lib/release-branch-rewrite.mjs new file mode 100644 index 00000000..b3b6b46a --- /dev/null +++ b/scripts/lib/release-branch-rewrite.mjs @@ -0,0 +1,32 @@ +// The record a generated-branch rewrite leaves behind so review continuity survives it. +// +// GitHub omits `before_commit_id` on the force-pushes Release Please performs, so the head a +// regeneration discards is named nowhere in the pull request's record: an ordinary push that was +// later rewritten away leaves no timeline entry, and a head that was never reviewed is named by no +// review either. The review gate therefore cannot prove that a finding recorded only on that head +// was carried forward, and refuses. +// +// The workflow that performs the rewrite is ours, so it can record what GitHub does not. This is +// the marker it writes, and the reader both sides share. +// +// It supplies evidence; it does not waive anything. A marker names a commit, and the gate still +// searches that commit for state belonging to that pull request. A marker naming a head with no +// state leaves the gate exactly as unconvinced as it was before. +const MARKER_PATTERN = + //iu; + +export function formatReleaseBranchRewriteMarker(before, after) { + if (!/^[0-9a-f]{40}$/iu.test(before ?? "") || !/^[0-9a-f]{40}$/iu.test(after ?? "")) { + throw new Error("A release branch rewrite marker needs two full commit SHAs."); + } + return ``; +} + +// Returns `{ before, after }` for a comment that carries the marker, or null. Callers must check +// the comment's author themselves: only a marker written by the workflow that performed the +// rewrite is evidence, and anyone who can comment can write the text. +export function readReleaseBranchRewriteMarker(body) { + const match = MARKER_PATTERN.exec(body ?? ""); + if (!match) return null; + return { before: match[1].toLowerCase(), after: match[2].toLowerCase() }; +} diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs index 6a8fe110..94afb254 100644 --- a/scripts/publish-review-gate-check.mjs +++ b/scripts/publish-review-gate-check.mjs @@ -11,9 +11,14 @@ import { runGhText, } from "./lib/github-cli-retry.mjs"; import { readCleanTopLevelReviewCommit } from "./lib/codex-review-markers.mjs"; +import { readReleaseBranchRewriteMarker } from "./lib/release-branch-rewrite.mjs"; const CHECK_RUN_NAME = "Review gate (scheduled)"; const REQUIRED_REVIEW_AUTHOR = "chatgpt-codex-connector"; +// Only the workflow that performs a rewrite can attest to what it discarded. Anyone who can +// comment can write the marker text, so the author is the whole of its authority: no human can +// post under this login. +const TRUSTED_REWRITE_RECORDER = "github-actions"; const DURABLE_REVIEW_STATE_PREFIX = "review-gate-state/v1\n"; const MAX_DURABLE_FORCE_PUSH_HISTORY_NODES = 20; const MAX_DURABLE_REVIEW_STATE_BYTES = 60_000; @@ -560,7 +565,37 @@ function durableReviewStateBelongsToPr(state, expectedPrNumber) { return parsed?.version === 1 && parsed.prNumber === expectedPrNumber; } +// Discarded heads a rewrite recorded out-of-band, keyed by the head it created. GitHub omits +// `before_commit_id` for a generated-branch regeneration, and the workflow performing it records +// the pair instead (#350). This supplies the missing name; it does not waive anything, because a +// head named this way is still searched for state belonging to this pull request. +function loadRecordedRewriteDiscards(prNumber) { + const commentPages = JSON.parse( + runGithub( + ["api", "--paginate", "--slurp", `repos/${repo}/issues/${prNumber}/comments?per_page=100`], + "recorded rewrite discovery", + ), + ); + const discards = new Map(); + for (const comment of flattenPages(commentPages)) { + if (normaliseLogin(comment?.user?.login) !== TRUSTED_REWRITE_RECORDER) continue; + const marker = readReleaseBranchRewriteMarker(comment.body); + if (!marker) continue; + // A second marker for the same created head is two different claims about one rewrite, and + // there is no basis for preferring either. + if (discards.has(marker.after) && discards.get(marker.after) !== marker.before) { + throw durableStateRejection( + "a rewrite was recorded twice with different discarded heads, so which commit it discarded is not settled", + "conflicting recorded rewrite discards for one created head", + ); + } + discards.set(marker.after, marker.before); + } + return discards; +} + function loadForcePushedPriorShas(prNumber) { + const recordedDiscards = loadRecordedRewriteDiscards(prNumber); const timelinePages = JSON.parse( runGithub( [ @@ -607,7 +642,9 @@ function loadForcePushedPriorShas(prNumber) { // leaves no timeline entry at all. Recording when that happened is what lets state older than // it be recognised as possibly superseded. if (!/^[0-9a-f]{40}$/iu.test(event.before_commit_id ?? "")) { - unidentifiedDiscardAt = Math.max(unidentifiedDiscardAt ?? -Infinity, createdAt); + const recorded = recordedDiscards.get(String(event.commit_id ?? "").toLowerCase()); + if (recorded) shas.push(recorded); + else unidentifiedDiscardAt = Math.max(unidentifiedDiscardAt ?? -Infinity, createdAt); } rewrites.push({ createdAt, shas }); } diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index c296adf8..6b34458d 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -10,3 +10,12 @@ export function resolveReleaseTargetBranch( ): string; export function serializeGitHubOutput(outputs: Record): string; + +export function recordBranchRewrites(options: { + env: NodeJS.ProcessEnv | Record; + owner: string; + repo: string; + targetBranch: string; + headsBeforeRegeneration: Map; + pullRequests: Array<{ number?: number; headBranchName?: string }>; +}): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 4be4b26c..00b9447b 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -1,4 +1,5 @@ import { appendFile } from "node:fs/promises"; +import { formatReleaseBranchRewriteMarker } from "./lib/release-branch-rewrite.mjs"; import { createRequire } from "node:module"; import { pathToFileURL } from "node:url"; import { randomUUID } from "node:crypto"; @@ -49,7 +50,17 @@ export async function runReleasePlease(env = process.env) { configFile, manifestFile, ); + // Read before the regeneration so the head it is about to discard is still addressable. + const headsBeforeRegeneration = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); const pullRequests = (await pullRequestManifest.createPullRequests()).filter(Boolean); + await recordBranchRewrites({ + env, + owner, + repo, + targetBranch, + headsBeforeRegeneration, + pullRequests, + }); outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { outputs.pr = JSON.stringify(pullRequests[0]); @@ -67,6 +78,71 @@ export async function runReleasePlease(env = process.env) { return outputs; } +// Release Please regenerates by force-pushing, and GitHub records no `before_commit_id` for it, so +// the discarded head is unnameable from the pull request's record afterwards. Recording it here -- +// from the only place that still knows it -- is what lets the review gate establish continuity +// across a regeneration instead of refusing every release pull request (#342, #350). +export async function recordBranchRewrites({ + env, + owner, + repo, + targetBranch, + headsBeforeRegeneration, + pullRequests, +}) { + const headsAfter = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); + for (const pullRequest of pullRequests) { + const branch = pullRequest?.headBranchName; + const number = pullRequest?.number; + if (!branch || !Number.isInteger(number)) continue; + const before = headsBeforeRegeneration.get(branch); + const after = headsAfter.get(branch); + // No prior head means the branch was created rather than rewritten, and an unchanged head + // means nothing was discarded. Neither is a rewrite, and inventing a marker for one would be + // recording something that did not happen. + if (!before || !after || before === after) continue; + await githubRequest(env, `/repos/${owner}/${repo}/issues/${number}/comments`, { + method: "POST", + body: JSON.stringify({ body: formatReleaseBranchRewriteMarker(before, after) }), + }); + console.log(`Recorded release branch rewrite on #${number}: ${before} -> ${after}`); + } +} + +async function readReleaseBranchHeads({ env, owner, repo, targetBranch }) { + const prefix = `heads/release-please--branches--${targetBranch}--`; + const refs = await githubRequest(env, `/repos/${owner}/${repo}/git/matching-refs/${prefix}`); + const heads = new Map(); + if (!Array.isArray(refs)) return heads; + for (const ref of refs) { + const branch = String(ref?.ref ?? "").replace(/^refs\/heads\//u, ""); + const sha = ref?.object?.sha; + if (branch && /^[0-9a-f]{40}$/iu.test(sha ?? "")) heads.set(branch, sha.toLowerCase()); + } + return heads; +} + +async function githubRequest(env, path, init = {}) { + const token = env.RELEASE_PLEASE_TOKEN || env.GITHUB_TOKEN || env.GH_TOKEN; + const apiUrl = env.GITHUB_API_URL || DEFAULT_GITHUB_API_URL; + const response = await globalThis.fetch(`${apiUrl}${path}`, { + ...init, + headers: { + accept: "application/vnd.github+json", + authorization: `Bearer ${token}`, + "x-github-api-version": "2022-11-28", + ...(init.body ? { "content-type": "application/json" } : {}), + ...init.headers, + }, + }); + if (!response.ok) { + // Named rather than swallowed: a rewrite that was performed but not recorded is the case the + // review gate cannot tell apart from one that never happened. + throw new Error(`GitHub request failed: ${init.method ?? "GET"} ${path} -> ${response.status}`); + } + return response.status === 204 ? null : response.json(); +} + export function buildReleaseOutputs(releases) { const createdReleases = releases.filter(Boolean); const outputs = { diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts index 8f154b53..56d450a7 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -908,6 +908,124 @@ describe("PR-head Review gate check publisher", () => { expect(publicationText).not.toContain("could not retrieve durable review state"); }); + // The workflow that rewrites a generated branch records the head it discarded, because GitHub + // does not. That turns an unnamed discard into a named one, so state older than the rewrite is + // usable again -- and the named head is still searched rather than trusted. + it("accepts older state when the rewrite recorded the head it discarded", () => { + const createdSha = "b".repeat(40); + const discardedSha = "c".repeat(40); + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + { [discardedSha]: reviewStateWithDeletedFinding(1, "state-on-recorded-discard") }, + [forcePushEvent(null, "2026-08-17T12:00:00Z", createdSha)], + {}, + 0, + null, + {}, + {}, + {}, + { [discardedSha]: "2026-08-17T11:00:00Z" }, + [ + { + user: { login: "github-actions[bot]" }, + body: ``, + }, + ], + ); + const publicationText = + calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; + + expect(result.status).toBe(0); + expect( + calls.some((call) => call.join(" ").includes(`commits/${discardedSha}/check-runs?`)), + ).toBe(true); + expect(publicationText).toContain("state-on-recorded-discard"); + }); + + // Anyone who can comment can write the marker text, so the author is the whole of its authority. + it("ignores a rewrite record that a trusted workflow did not write", () => { + const createdSha = "b".repeat(40); + const discardedSha = "c".repeat(40); + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + { [discardedSha]: reviewStateWithDeletedFinding(1, "state-behind-untrusted-marker") }, + [forcePushEvent(null, "2026-08-17T12:00:00Z", createdSha)], + {}, + 0, + null, + {}, + {}, + {}, + { [discardedSha]: "2026-08-17T11:00:00Z" }, + [ + { + user: { login: "someone-else" }, + body: ``, + }, + ], + ); + const publicationText = + calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; + + expect(result.status).toBe(0); + expect(publicationText).toContain("conclusion=action_required"); + // The head the untrusted marker names is never made a candidate, so its state is neither + // searched nor published. The refusal that fires is the no-reachable-state one rather than + // the unnamed-discard one, and either way nothing is accepted on that marker's word. + expect( + calls.some((call) => call.join(" ").includes(`commits/${discardedSha}/check-runs?`)), + ).toBe(false); + expect(publicationText).not.toContain("state-behind-untrusted-marker"); + }); + + it("refuses when one rewrite was recorded twice with different discarded heads", () => { + const createdSha = "b".repeat(40); + const discardedSha = "c".repeat(40); + const otherSha = "d".repeat(40); + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + {}, + [forcePushEvent(null, "2026-08-17T12:00:00Z", createdSha)], + {}, + 0, + null, + {}, + {}, + {}, + {}, + [ + { + user: { login: "github-actions[bot]" }, + body: ``, + }, + { + user: { login: "github-actions[bot]" }, + body: ``, + }, + ], + ); + const publicationText = + calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; + + expect(result.status).toBe(0); + expect(publicationText).toContain("recorded twice with different discarded heads"); + }); + it("publishes a durable-state workspace failure without its local path", () => { const directory = mkdtempSync(path.join(tmpdir(), "pack-review-gate-private-path-")); const privatePath = path.join(directory, "not-a-directory"); diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index f55d5753..3ea69053 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -99,6 +99,14 @@ describe("Release Please workflow wrapper", () => { .spyOn(releasePlease.Manifest.prototype, "createPullRequests") .mockImplementation(createPullRequests); const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const fetched: string[] = []; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init: RequestInit = {}) => { + fetched.push(`${String(init.method ?? "GET")} ${new URL(String(url)).pathname}`); + return { ok: true, status: 200, json: async () => [] } as unknown as Response; + }), + ); try { const outputs = await runReleasePlease({ @@ -120,7 +128,14 @@ describe("Release Please workflow wrapper", () => { prs_created: "true", release_created: "true", }); + // The only direct GitHub calls are the two branch-head reads that bracket the + // regeneration. Nothing is written, because this run rewrote no branch. + expect(fetched).toEqual([ + "GET /repos/lamemustafa/pack/git/matching-refs/heads/release-please--branches--master--", + "GET /repos/lamemustafa/pack/git/matching-refs/heads/release-please--branches--master--", + ]); } finally { + vi.unstubAllGlobals(); log.mockRestore(); pullRequestManifest.mockRestore(); releaseManifest.mockRestore(); @@ -128,3 +143,85 @@ describe("Release Please workflow wrapper", () => { } }); }); + +describe("release branch rewrite records", () => { + it("records the discarded head when a regeneration rewrote the branch", async () => { + const before = "b".repeat(40); + const after = "c".repeat(40); + const requests: Array<{ url: string; method: string; body?: string }> = []; + const fetchMock = vi.fn(async (url: string, init: RequestInit = {}) => { + requests.push({ url, method: String(init.method ?? "GET"), body: init.body as string }); + if (String(url).includes("matching-refs")) { + // `recordBranchRewrites` is given the pre-regeneration heads and reads only the current + // ones, so this single call answers with the head the rewrite created. + const sha = after; + return { + ok: true, + status: 200, + json: async () => [ + { + ref: "refs/heads/release-please--branches--master--components--pack", + object: { sha }, + }, + ], + } as unknown as Response; + } + return { ok: true, status: 201, json: async () => ({}) } as unknown as Response; + }); + vi.stubGlobal("fetch", fetchMock); + + const { recordBranchRewrites } = await import("../../scripts/run-release-please.mjs"); + await recordBranchRewrites({ + env: { GITHUB_TOKEN: "t", GITHUB_API_URL: "https://api.github.test" }, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([ + ["release-please--branches--master--components--pack", before], + ]), + pullRequests: [ + { number: 337, headBranchName: "release-please--branches--master--components--pack" }, + ], + }); + + const posted = requests.find((request) => request.method === "POST"); + expect(posted?.url).toContain("/repos/lamemustafa/pack/issues/337/comments"); + expect(posted?.body).toContain(`review-gate-rewrite before=${before} after=${after}`); + vi.unstubAllGlobals(); + }); + + it("records nothing when the head did not move", async () => { + const sha = "b".repeat(40); + const requests: Array<{ method: string }> = []; + const fetchMock = vi.fn(async (_url: string, init: RequestInit = {}) => { + requests.push({ method: String(init.method ?? "GET") }); + return { + ok: true, + status: 200, + json: async () => [ + { ref: "refs/heads/release-please--branches--master--components--pack", object: { sha } }, + ], + } as unknown as Response; + }); + vi.stubGlobal("fetch", fetchMock); + + const { recordBranchRewrites } = await import("../../scripts/run-release-please.mjs"); + await recordBranchRewrites({ + env: { GITHUB_TOKEN: "t", GITHUB_API_URL: "https://api.github.test" }, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([ + ["release-please--branches--master--components--pack", sha], + ]), + pullRequests: [ + { number: 337, headBranchName: "release-please--branches--master--components--pack" }, + ], + }); + + // A branch that was created rather than rewritten, or one whose head did not move, discarded + // nothing. A marker for either would record a rewrite that never happened. + expect(requests.some((request) => request.method === "POST")).toBe(false); + vi.unstubAllGlobals(); + }); +}); From d3bc0692144d7bb6ae4c7b4ba7b84a759c976bad Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Thu, 10 Sep 2026 14:30:54 +0530 Subject: [PATCH 02/17] fix(release): make an interrupted rewrite record recoverable Dispositions all three findings from the Codex review of 1262bb8. Two of them interlock, so this is a redesign of the recording rather than three patches. P1, an interrupted run lost the discarded head permanently. Writing the record after the force-push meant a cancellation in between destroyed the only remaining name for that commit, and no later run could reconstruct it. Recording is now two stages: the discarded head is written while it is still the branch head, and the created head is added once the rewrite has produced one. A record stopped in between names a discard with no replacement, identifies nothing, and is ignored by the gate -- and the next run completes it from the branch head it finds, which is exactly what that rewrite created, because nothing else rewrites the branch and it has not run since. P1, bookkeeping could strand a published release. The head snapshot ran after `createReleases()`, so a failure there aborted the wrapper before it wrote its outputs, and the workflow's prerelease-marking, asset-upload and asset-verify steps never ran. Everything fallible now runs before `createReleases()`, where a failure costs a re-run rather than a release without assets. The closing step must run afterwards, so it no longer throws: it logs and leaves the record open. That is only safe because of the fix above -- the open record is recoverable, and the cost of not throwing is a refusal the gate was already making. P2, a malformed head list was read as "no branches". An indeterminate response would have let a rewrite proceed with nothing recording what it discarded, which is the state this mechanism exists to prevent and which nothing downstream could detect. It now throws before regeneration. Fetching the pull request's comments twice -- once for rewrite records, once for clean top-level review markers -- was a duplicate of a fact the pull request states once, and each `gh` invocation spawns a process. Read once and shared. A single run of the publisher tests went from exceeding a 120-second budget to about 41 seconds, which also accounts for tests that had begun failing intermittently in full-file runs while passing alone. Stated as the likely cause rather than a proven one: no before/after timing was captured deliberately. Co-Authored-By: Claude Opus 5 --- scripts/lib/release-branch-rewrite.mjs | 38 +++- scripts/publish-review-gate-check.mjs | 43 ++-- scripts/run-release-please.d.mts | 10 +- scripts/run-release-please.mjs | 148 ++++++++++--- .../scripts/publish-review-gate-check.test.ts | 44 +++- tests/scripts/run-release-please.test.ts | 199 ++++++++++++------ 6 files changed, 355 insertions(+), 127 deletions(-) diff --git a/scripts/lib/release-branch-rewrite.mjs b/scripts/lib/release-branch-rewrite.mjs index b3b6b46a..fe2befc9 100644 --- a/scripts/lib/release-branch-rewrite.mjs +++ b/scripts/lib/release-branch-rewrite.mjs @@ -12,21 +12,41 @@ // It supplies evidence; it does not waive anything. A marker names a commit, and the gate still // searches that commit for state belonging to that pull request. A marker naming a head with no // state leaves the gate exactly as unconvinced as it was before. +// +// A record is written in two stages because the rewrite is not atomic with the recording of it. +// The `before` head is written first, while it is still the branch head; the `after` head is added +// once the rewrite has produced one. A record stopped in between names a discarded head and no +// replacement, which identifies nothing on its own -- so the gate ignores it, and the next run +// completes it from the branch head it finds, which is precisely the head that rewrite created. const MARKER_PATTERN = - //iu; + //iu; + +const SHA_PATTERN = /^[0-9a-f]{40}$/iu; -export function formatReleaseBranchRewriteMarker(before, after) { - if (!/^[0-9a-f]{40}$/iu.test(before ?? "") || !/^[0-9a-f]{40}$/iu.test(after ?? "")) { - throw new Error("A release branch rewrite marker needs two full commit SHAs."); +export function formatReleaseBranchRewriteMarker({ branch, before, after = null }) { + if (!branch || /\s/u.test(branch)) { + throw new Error("A release branch rewrite marker needs a whitespace-free branch name."); + } + if (!SHA_PATTERN.test(before ?? "")) { + throw new Error("A release branch rewrite marker needs a full discarded-head SHA."); + } + if (after !== null && !SHA_PATTERN.test(after)) { + throw new Error("A release branch rewrite marker needs a full created-head SHA or none."); } - return ``; + const suffix = after === null ? "" : ` after=${after.toLowerCase()}`; + return ``; } -// Returns `{ before, after }` for a comment that carries the marker, or null. Callers must check -// the comment's author themselves: only a marker written by the workflow that performed the -// rewrite is evidence, and anyone who can comment can write the text. +// Returns `{ branch, before, after }` for a comment that carries the marker, or null. `after` is +// null for a record whose rewrite had not produced a head yet. Callers must check the comment's +// author themselves: only a marker written by the workflow that performed the rewrite is evidence, +// and anyone who can comment can write the text. export function readReleaseBranchRewriteMarker(body) { const match = MARKER_PATTERN.exec(body ?? ""); if (!match) return null; - return { before: match[1].toLowerCase(), after: match[2].toLowerCase() }; + return { + branch: match[1], + before: match[2].toLowerCase(), + after: match[3] ? match[3].toLowerCase() : null, + }; } diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs index 94afb254..a17ebdb6 100644 --- a/scripts/publish-review-gate-check.mjs +++ b/scripts/publish-review-gate-check.mjs @@ -237,11 +237,14 @@ function untraceableRewriteError() { } function loadLatestDurableReviewState(pr) { + // Read once and shared: both the rewrite records and the clean top-level review markers live in + // this one list, and fetching it twice was a duplicate of a fact the pull request states once. + const comments = loadIssueComments(pr.number); const { priorHeads: forcePushedPriorShas, hasUntraceableRewrite, unidentifiedDiscardAt, - } = loadForcePushedPriorShas(pr.number); + } = loadForcePushedPriorShas(pr.number, comments); // Reject before consulting any reachable state, not after. A state surviving on a current-line // commit cannot contain a finding that was observed and then deleted only on the head this // rewrite discarded, so returning it would publish success while losing that ask. Continuity @@ -265,7 +268,7 @@ function loadLatestDurableReviewState(pr) { const priorHeads = dedupePriorHeadShas( forcePushedPriorShas, loadReviewedHeadShas(pr.number), - loadTopLevelReviewedHeadShas(pr.number, currentPrShas, forcePushedPriorShas), + loadTopLevelReviewedHeadShas(comments, currentPrShas, forcePushedPriorShas), ); const discardedLineShas = loadDiscardedLineShas(pr, priorHeads, new Set(currentPrShas)); @@ -350,16 +353,10 @@ function loadReviewedHeadShas(prNumber) { // it names was therefore a head of this pull request, so it belongs among the candidate heads -- // otherwise a head reviewed clean and then rewritten away is named in the record but never // searched. -function loadTopLevelReviewedHeadShas(prNumber, currentPrShas, forcePushedPriorHeads) { - const commentPages = JSON.parse( - runGithub( - ["api", "--paginate", "--slurp", `repos/${repo}/issues/${prNumber}/comments?per_page=100`], - "reviewed head marker discovery", - ), - ); +function loadTopLevelReviewedHeadShas(comments, currentPrShas, forcePushedPriorHeads) { const known = [...currentPrShas, ...forcePushedPriorHeads.map((head) => head.sha)]; const prefixes = new Set(); - for (const comment of flattenPages(commentPages)) { + for (const comment of comments) { if (normaliseLogin(comment?.user?.login) !== REQUIRED_REVIEW_AUTHOR) continue; const prefix = readCleanTopLevelReviewCommit(comment.body); // A marker naming a head already in hand needs no lookup, and the common case is the current @@ -569,18 +566,26 @@ function durableReviewStateBelongsToPr(state, expectedPrNumber) { // `before_commit_id` for a generated-branch regeneration, and the workflow performing it records // the pair instead (#350). This supplies the missing name; it does not waive anything, because a // head named this way is still searched for state belonging to this pull request. -function loadRecordedRewriteDiscards(prNumber) { - const commentPages = JSON.parse( - runGithub( - ["api", "--paginate", "--slurp", `repos/${repo}/issues/${prNumber}/comments?per_page=100`], - "recorded rewrite discovery", +function loadIssueComments(prNumber) { + return flattenPages( + JSON.parse( + runGithub( + ["api", "--paginate", "--slurp", `repos/${repo}/issues/${prNumber}/comments?per_page=100`], + "pull request comment discovery", + ), ), ); +} + +function loadRecordedRewriteDiscards(comments) { const discards = new Map(); - for (const comment of flattenPages(commentPages)) { + for (const comment of comments) { if (normaliseLogin(comment?.user?.login) !== TRUSTED_REWRITE_RECORDER) continue; const marker = readReleaseBranchRewriteMarker(comment.body); - if (!marker) continue; + // An open record names a discarded head and no replacement, so it pairs with no rewrite and + // identifies nothing. A record whose heads match says the branch did not move, which is a + // recorded non-event rather than a discard. + if (!marker || marker.after === null || marker.after === marker.before) continue; // A second marker for the same created head is two different claims about one rewrite, and // there is no basis for preferring either. if (discards.has(marker.after) && discards.get(marker.after) !== marker.before) { @@ -594,8 +599,8 @@ function loadRecordedRewriteDiscards(prNumber) { return discards; } -function loadForcePushedPriorShas(prNumber) { - const recordedDiscards = loadRecordedRewriteDiscards(prNumber); +function loadForcePushedPriorShas(prNumber, comments) { + const recordedDiscards = loadRecordedRewriteDiscards(comments); const timelinePages = JSON.parse( runGithub( [ diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 6b34458d..31dc4928 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -11,11 +11,17 @@ export function resolveReleaseTargetBranch( export function serializeGitHubOutput(outputs: Record): string; -export function recordBranchRewrites(options: { +export function openBranchRewriteRecords(options: { + env: NodeJS.ProcessEnv | Record; + owner: string; + repo: string; + targetBranch: string; +}): Promise>; + +export function closeBranchRewriteRecords(options: { env: NodeJS.ProcessEnv | Record; owner: string; repo: string; targetBranch: string; headsBeforeRegeneration: Map; - pullRequests: Array<{ number?: number; headBranchName?: string }>; }): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 00b9447b..8c71d463 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -1,5 +1,8 @@ import { appendFile } from "node:fs/promises"; -import { formatReleaseBranchRewriteMarker } from "./lib/release-branch-rewrite.mjs"; +import { + formatReleaseBranchRewriteMarker, + readReleaseBranchRewriteMarker, +} from "./lib/release-branch-rewrite.mjs"; import { createRequire } from "node:module"; import { pathToFileURL } from "node:url"; import { randomUUID } from "node:crypto"; @@ -41,6 +44,16 @@ export async function runReleasePlease(env = process.env) { configFile, manifestFile, ); + // Before `createReleases()`, because everything here can fail and nothing here is reversible + // once a GitHub release exists. A failure at this point costs a re-run; the same failure after a + // release is published leaves that release without its assets. + const headsBeforeRegeneration = await openBranchRewriteRecords({ + env, + owner, + repo, + targetBranch, + }); + const releases = (await releaseManifest.createReleases()).filter(Boolean); const outputs = buildReleaseOutputs(releases); @@ -50,17 +63,8 @@ export async function runReleasePlease(env = process.env) { configFile, manifestFile, ); - // Read before the regeneration so the head it is about to discard is still addressable. - const headsBeforeRegeneration = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); const pullRequests = (await pullRequestManifest.createPullRequests()).filter(Boolean); - await recordBranchRewrites({ - env, - owner, - repo, - targetBranch, - headsBeforeRegeneration, - pullRequests, - }); + await closeBranchRewriteRecords({ env, owner, repo, targetBranch, headsBeforeRegeneration }); outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { outputs.pr = JSON.stringify(pullRequests[0]); @@ -82,38 +86,122 @@ export async function runReleasePlease(env = process.env) { // the discarded head is unnameable from the pull request's record afterwards. Recording it here -- // from the only place that still knows it -- is what lets the review gate establish continuity // across a regeneration instead of refusing every release pull request (#342, #350). -export async function recordBranchRewrites({ +// +// Opening the record before the rewrite rather than writing it afterwards is what makes an +// interrupted run recoverable: a cancellation between the force-push and the write would otherwise +// lose the discarded SHA permanently, and no later run could reconstruct it. +export async function openBranchRewriteRecords({ env, owner, repo, targetBranch }) { + const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); + for (const [branch, head] of heads) { + const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, repo, branch }); + if (pullRequestNumber === null) continue; + const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); + // A record left open by an interrupted run names a discarded head and no replacement. The + // branch head standing here now is exactly what that rewrite created, because this workflow is + // the only thing that rewrites the branch and it has not run since. + for (const record of records) { + if (record.marker.branch !== branch || record.marker.after !== null) continue; + await completeBranchRewriteRecord({ env, owner, repo, record, after: head }); + } + await githubRequest(env, `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments`, { + method: "POST", + body: JSON.stringify({ + body: formatReleaseBranchRewriteMarker({ branch, before: head }), + }), + }); + console.log(`Opened release branch rewrite record on #${pullRequestNumber}: before=${head}`); + } + return heads; +} + +// Failures here are logged rather than thrown. A release may already exist by this point, and +// failing the job would strand it without its assets. The cost of not throwing is bounded: the +// record stays open, the review gate keeps refusing exactly as it would have, and the next run +// completes it. +export async function closeBranchRewriteRecords({ env, owner, repo, targetBranch, headsBeforeRegeneration, - pullRequests, }) { - const headsAfter = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); - for (const pullRequest of pullRequests) { - const branch = pullRequest?.headBranchName; - const number = pullRequest?.number; - if (!branch || !Number.isInteger(number)) continue; - const before = headsBeforeRegeneration.get(branch); - const after = headsAfter.get(branch); - // No prior head means the branch was created rather than rewritten, and an unchanged head - // means nothing was discarded. Neither is a rewrite, and inventing a marker for one would be - // recording something that did not happen. - if (!before || !after || before === after) continue; - await githubRequest(env, `/repos/${owner}/${repo}/issues/${number}/comments`, { - method: "POST", - body: JSON.stringify({ body: formatReleaseBranchRewriteMarker(before, after) }), - }); - console.log(`Recorded release branch rewrite on #${number}: ${before} -> ${after}`); + try { + const headsAfter = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); + for (const branch of headsBeforeRegeneration.keys()) { + const after = headsAfter.get(branch); + // An unchanged head discarded nothing. Completing the record with `after === before` says + // exactly that, and the gate reads it as the non-event it was. + if (!after) continue; + const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, repo, branch }); + if (pullRequestNumber === null) continue; + const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); + const open = records.find( + (record) => record.marker.branch === branch && record.marker.after === null, + ); + if (!open) continue; + await completeBranchRewriteRecord({ env, owner, repo, record: open, after }); + console.log( + `Closed release branch rewrite record on #${pullRequestNumber}: ${open.marker.before} -> ${after}`, + ); + } + } catch (error) { + console.error( + `Could not close a release branch rewrite record: ${error.message}. The record stays open and the next run completes it.`, + ); + } +} + +async function completeBranchRewriteRecord({ env, owner, repo, record, after }) { + await githubRequest(env, `/repos/${owner}/${repo}/issues/comments/${record.id}`, { + method: "PATCH", + body: JSON.stringify({ + body: formatReleaseBranchRewriteMarker({ + branch: record.marker.branch, + before: record.marker.before, + after, + }), + }), + }); +} + +async function readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }) { + const comments = await githubRequest( + env, + `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments?per_page=100`, + ); + if (!Array.isArray(comments)) { + throw new Error("GitHub returned a malformed comment list for a release pull request."); + } + const records = []; + for (const comment of comments) { + const marker = readReleaseBranchRewriteMarker(comment?.body); + if (marker && Number.isInteger(comment?.id)) records.push({ id: comment.id, marker }); } + return records; +} + +async function findOpenPullRequestNumber({ env, owner, repo, branch }) { + const pulls = await githubRequest( + env, + `/repos/${owner}/${repo}/pulls?state=open&head=${owner}:${branch}`, + ); + if (!Array.isArray(pulls)) { + throw new Error("GitHub returned a malformed pull request list for a release branch."); + } + const number = pulls.find((pull) => Number.isInteger(pull?.number))?.number; + return number ?? null; } async function readReleaseBranchHeads({ env, owner, repo, targetBranch }) { const prefix = `heads/release-please--branches--${targetBranch}--`; const refs = await githubRequest(env, `/repos/${owner}/${repo}/git/matching-refs/${prefix}`); + // An indeterminate response is not an empty branch list. Reading it as one would let a rewrite + // proceed with no record of what it discarded, which is the state this whole mechanism exists to + // prevent and which nothing downstream could detect. + if (!Array.isArray(refs)) { + throw new Error("GitHub returned a malformed release branch head list."); + } const heads = new Map(); - if (!Array.isArray(refs)) return heads; for (const ref of refs) { const branch = String(ref?.ref ?? "").replace(/^refs\/heads\//u, ""); const sha = ref?.object?.sha; diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts index 56d450a7..70d08c18 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -933,7 +933,7 @@ describe("PR-head Review gate check publisher", () => { [ { user: { login: "github-actions[bot]" }, - body: ``, + body: ``, }, ], ); @@ -947,6 +947,42 @@ describe("PR-head Review gate check publisher", () => { expect(publicationText).toContain("state-on-recorded-discard"); }); + // An interrupted run leaves a record naming a discarded head and no replacement. It pairs with + // no rewrite, so it identifies nothing and must not be read as if it did. + it("ignores a rewrite record whose rewrite never produced a head", () => { + const createdSha = "b".repeat(40); + const discardedSha = "c".repeat(40); + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + { [discardedSha]: reviewStateWithDeletedFinding(1, "state-behind-open-record") }, + [forcePushEvent(null, "2026-08-17T12:00:00Z", createdSha)], + {}, + 0, + null, + {}, + {}, + {}, + { [discardedSha]: "2026-08-17T11:00:00Z" }, + [ + { + user: { login: "github-actions[bot]" }, + body: ``, + }, + ], + ); + const publicationText = + calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; + + expect(result.status).toBe(0); + expect(publicationText).toContain("conclusion=action_required"); + expect(publicationText).not.toContain("state-behind-open-record"); + }); + // Anyone who can comment can write the marker text, so the author is the whole of its authority. it("ignores a rewrite record that a trusted workflow did not write", () => { const createdSha = "b".repeat(40); @@ -970,7 +1006,7 @@ describe("PR-head Review gate check publisher", () => { [ { user: { login: "someone-else" }, - body: ``, + body: ``, }, ], ); @@ -1011,11 +1047,11 @@ describe("PR-head Review gate check publisher", () => { [ { user: { login: "github-actions[bot]" }, - body: ``, + body: ``, }, { user: { login: "github-actions[bot]" }, - body: ``, + body: ``, }, ], ); diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 3ea69053..f2e3a879 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -1,6 +1,6 @@ import { readFile } from "node:fs/promises"; import { createRequire } from "node:module"; -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { buildReleaseOutputs, @@ -145,83 +145,156 @@ describe("Release Please workflow wrapper", () => { }); describe("release branch rewrite records", () => { - it("records the discarded head when a regeneration rewrote the branch", async () => { - const before = "b".repeat(40); - const after = "c".repeat(40); - const requests: Array<{ url: string; method: string; body?: string }> = []; - const fetchMock = vi.fn(async (url: string, init: RequestInit = {}) => { - requests.push({ url, method: String(init.method ?? "GET"), body: init.body as string }); - if (String(url).includes("matching-refs")) { - // `recordBranchRewrites` is given the pre-regeneration heads and reads only the current - // ones, so this single call answers with the head the rewrite created. - const sha = after; - return { - ok: true, - status: 200, - json: async () => [ - { - ref: "refs/heads/release-please--branches--master--components--pack", - object: { sha }, - }, - ], - } as unknown as Response; - } - return { ok: true, status: 201, json: async () => ({}) } as unknown as Response; - }); - vi.stubGlobal("fetch", fetchMock); + const branch = "release-please--branches--master--components--pack"; + const env = { GITHUB_TOKEN: "t", GITHUB_API_URL: "https://api.github.test" }; + + function stubGitHub(handlers: { + heads?: string | null; + comments?: Array<{ id: number; body: string }>; + pulls?: Array<{ number: number }>; + malformedHeads?: boolean; + }) { + const calls: Array<{ method: string; path: string; body?: string }> = []; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init: RequestInit = {}) => { + const path = new URL(String(url)).pathname + new URL(String(url)).search; + calls.push({ method: String(init.method ?? "GET"), path, body: init.body as string }); + if (path.includes("matching-refs")) { + const payload = handlers.malformedHeads + ? { message: "something else" } + : handlers.heads + ? [{ ref: `refs/heads/${branch}`, object: { sha: handlers.heads } }] + : []; + return { ok: true, status: 200, json: async () => payload } as unknown as Response; + } + if (path.includes("/pulls?")) { + return { + ok: true, + status: 200, + json: async () => handlers.pulls ?? [{ number: 337 }], + } as unknown as Response; + } + if (path.includes("/issues/") && path.includes("/comments")) { + return { + ok: true, + status: 200, + json: async () => handlers.comments ?? [], + } as unknown as Response; + } + return { ok: true, status: 200, json: async () => ({}) } as unknown as Response; + }), + ); + return calls; + } + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("opens a record naming the head about to be discarded", async () => { + const head = "b".repeat(40); + const calls = stubGitHub({ heads: head }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - const { recordBranchRewrites } = await import("../../scripts/run-release-please.mjs"); - await recordBranchRewrites({ - env: { GITHUB_TOKEN: "t", GITHUB_API_URL: "https://api.github.test" }, + const heads = await openBranchRewriteRecords({ + env, owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([ - ["release-please--branches--master--components--pack", before], - ]), - pullRequests: [ - { number: 337, headBranchName: "release-please--branches--master--components--pack" }, - ], }); - const posted = requests.find((request) => request.method === "POST"); - expect(posted?.url).toContain("/repos/lamemustafa/pack/issues/337/comments"); - expect(posted?.body).toContain(`review-gate-rewrite before=${before} after=${after}`); - vi.unstubAllGlobals(); + expect(heads.get(branch)).toBe(head); + const posted = calls.find((call) => call.method === "POST"); + expect(posted?.path).toContain("/issues/337/comments"); + expect(posted?.body).toContain(`review-gate-rewrite branch=${branch} before=${head}`); + expect(posted?.body).not.toContain("after="); }); - it("records nothing when the head did not move", async () => { - const sha = "b".repeat(40); - const requests: Array<{ method: string }> = []; - const fetchMock = vi.fn(async (_url: string, init: RequestInit = {}) => { - requests.push({ method: String(init.method ?? "GET") }); - return { - ok: true, - status: 200, - json: async () => [ - { ref: "refs/heads/release-please--branches--master--components--pack", object: { sha } }, - ], - } as unknown as Response; + it("completes a record an interrupted run left open, from the head standing now", async () => { + const lostBefore = "c".repeat(40); + const head = "b".repeat(40); + const calls = stubGitHub({ + heads: head, + comments: [ + { id: 99, body: `` }, + ], }); - vi.stubGlobal("fetch", fetchMock); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - const { recordBranchRewrites } = await import("../../scripts/run-release-please.mjs"); - await recordBranchRewrites({ - env: { GITHUB_TOKEN: "t", GITHUB_API_URL: "https://api.github.test" }, + await openBranchRewriteRecords({ + env, owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([ - ["release-please--branches--master--components--pack", sha], - ]), - pullRequests: [ - { number: 337, headBranchName: "release-please--branches--master--components--pack" }, + }); + + // The branch head standing here is exactly what the lost rewrite created, because nothing but + // this workflow rewrites the branch and it has not run since. + const patched = calls.find((call) => call.method === "PATCH"); + expect(patched?.path).toContain("/issues/comments/99"); + expect(patched?.body).toContain(`before=${lostBefore} after=${head}`); + }); + + it("refuses to regenerate when the head list is malformed", async () => { + stubGitHub({ malformedHeads: true }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + // An indeterminate response read as "no branches" would let a rewrite proceed unrecorded, + // which nothing downstream could detect. + await expect( + openBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + }), + ).rejects.toThrow(/malformed release branch head list/iu); + }); + + it("closes the open record with the head the rewrite created", async () => { + const before = "c".repeat(40); + const after = "b".repeat(40); + const calls = stubGitHub({ + heads: after, + comments: [ + { id: 99, body: `` }, ], }); + const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - // A branch that was created rather than rewritten, or one whose head did not move, discarded - // nothing. A marker for either would record a rewrite that never happened. - expect(requests.some((request) => request.method === "POST")).toBe(false); - vi.unstubAllGlobals(); + await closeBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, before]]), + }); + + const patched = calls.find((call) => call.method === "PATCH"); + expect(patched?.body).toContain(`before=${before} after=${after}`); + }); + + it("leaves the record open rather than failing a run that already published a release", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: false, status: 500 }) as unknown as Response), + ); + const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); + const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + // Throwing here would abort the workflow after a GitHub release exists, stranding it without + // its assets. The open record costs a refusal the gate was already making. + await expect( + closeBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, "c".repeat(40)]]), + }), + ).resolves.toBeUndefined(); + expect(errors).toHaveBeenCalledWith(expect.stringContaining("stays open")); + errors.mockRestore(); }); }); From bba8115a417d6576565eefbbdb026c1213442f0b Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Fri, 11 Sep 2026 09:18:19 +0530 Subject: [PATCH 03/17] fix(release): read every page, and refuse a list entry that cannot be read Two findings on the previous head, both correct, and both the same class one level below the fix that prompted them. A response that is an array can still carry an entry with no usable SHA. Skipping it made that branch look absent, and an absent branch is force-pushed with no record of the head it discarded -- the exact state this mechanism exists to prevent. An entry that cannot be read is the same claim as a response that cannot be read, so it is now the same refusal. A release pull request outlives a hundred comments, and the marker just written is the newest one, so a first-page read is precisely what drops it. The record then cannot be closed, and every later run reads the same truncated page, leaving the force-push permanently unpaired. Three call sites each had their own single-page request and their own array check. One reader now pages to exhaustion for all three, and throws rather than returning what it collected when it runs out -- a short answer here is indistinguishable from 'no such branch', and that reading is the failure. --- scripts/run-release-please.mjs | 66 ++++++++++++++++++------ tests/scripts/run-release-please.test.ts | 64 +++++++++++++++++++++-- 2 files changed, 110 insertions(+), 20 deletions(-) diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 8c71d463..e2d8fa0b 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -165,13 +165,11 @@ async function completeBranchRewriteRecord({ env, owner, repo, record, after }) } async function readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }) { - const comments = await githubRequest( + const comments = await githubList( env, - `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments?per_page=100`, + `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments`, + "comment list for a release pull request", ); - if (!Array.isArray(comments)) { - throw new Error("GitHub returned a malformed comment list for a release pull request."); - } const records = []; for (const comment of comments) { const marker = readReleaseBranchRewriteMarker(comment?.body); @@ -181,35 +179,69 @@ async function readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }) } async function findOpenPullRequestNumber({ env, owner, repo, branch }) { - const pulls = await githubRequest( + const pulls = await githubList( env, `/repos/${owner}/${repo}/pulls?state=open&head=${owner}:${branch}`, + "pull request list for a release branch", ); - if (!Array.isArray(pulls)) { - throw new Error("GitHub returned a malformed pull request list for a release branch."); - } const number = pulls.find((pull) => Number.isInteger(pull?.number))?.number; return number ?? null; } async function readReleaseBranchHeads({ env, owner, repo, targetBranch }) { const prefix = `heads/release-please--branches--${targetBranch}--`; - const refs = await githubRequest(env, `/repos/${owner}/${repo}/git/matching-refs/${prefix}`); - // An indeterminate response is not an empty branch list. Reading it as one would let a rewrite - // proceed with no record of what it discarded, which is the state this whole mechanism exists to - // prevent and which nothing downstream could detect. - if (!Array.isArray(refs)) { - throw new Error("GitHub returned a malformed release branch head list."); - } + // An indeterminate response is not an empty branch list. Reading it as one lets a rewrite proceed + // with no record of what it discarded, which is the state this whole mechanism exists to prevent + // and which nothing downstream could detect. An entry this function cannot read is the same + // claim as a response it cannot read, so it is the same refusal: skipping the entry would make + // its branch look absent, which is the reading being guarded against. + const refs = await githubList( + env, + `/repos/${owner}/${repo}/git/matching-refs/${prefix}`, + "release branch head list", + ); const heads = new Map(); for (const ref of refs) { const branch = String(ref?.ref ?? "").replace(/^refs\/heads\//u, ""); const sha = ref?.object?.sha; - if (branch && /^[0-9a-f]{40}$/iu.test(sha ?? "")) heads.set(branch, sha.toLowerCase()); + if (!branch || !/^[0-9a-f]{40}$/iu.test(sha ?? "")) { + throw new Error("GitHub returned a malformed release branch head list."); + } + heads.set(branch, sha.toLowerCase()); } return heads; } +const GITHUB_PAGE_SIZE = 100; +const GITHUB_MAX_PAGES = 20; + +/** + * Every page of a GitHub list endpoint, or a throw. + * + * Three call sites read lists this script's correctness depends on, and each had its own + * single-page request and its own array check. A first page is not a list: a release pull request + * outlives a hundred comments, and the marker this mechanism has just written is the newest one, so + * it is precisely what a first-page read drops. Running out of pages throws rather than returning + * what was collected, because a short answer here is indistinguishable from "no such branch" and + * that reading is what force-pushes a branch with no record of the head it discarded. + */ +async function githubList(env, path, description) { + const items = []; + for (let page = 1; page <= GITHUB_MAX_PAGES; page += 1) { + const separator = path.includes("?") ? "&" : "?"; + const batch = await githubRequest( + env, + `${path}${separator}per_page=${GITHUB_PAGE_SIZE}&page=${page}`, + ); + if (!Array.isArray(batch)) { + throw new Error(`GitHub returned a malformed ${description}.`); + } + items.push(...batch); + if (batch.length < GITHUB_PAGE_SIZE) return items; + } + throw new Error(`GitHub returned more pages of ${description} than this script will read.`); +} + async function githubRequest(env, path, init = {}) { const token = env.RELEASE_PLEASE_TOKEN || env.GITHUB_TOKEN || env.GH_TOKEN; const apiUrl = env.GITHUB_API_URL || DEFAULT_GITHUB_API_URL; diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index f2e3a879..0a7f3a67 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -153,6 +153,8 @@ describe("release branch rewrite records", () => { comments?: Array<{ id: number; body: string }>; pulls?: Array<{ number: number }>; malformedHeads?: boolean; + malformedHeadEntry?: boolean; + commentPages?: Array>; }) { const calls: Array<{ method: string; path: string; body?: string }> = []; vi.stubGlobal( @@ -163,9 +165,11 @@ describe("release branch rewrite records", () => { if (path.includes("matching-refs")) { const payload = handlers.malformedHeads ? { message: "something else" } - : handlers.heads - ? [{ ref: `refs/heads/${branch}`, object: { sha: handlers.heads } }] - : []; + : handlers.malformedHeadEntry + ? [{ ref: `refs/heads/${branch}`, object: {} }] + : handlers.heads + ? [{ ref: `refs/heads/${branch}`, object: { sha: handlers.heads } }] + : []; return { ok: true, status: 200, json: async () => payload } as unknown as Response; } if (path.includes("/pulls?")) { @@ -176,6 +180,14 @@ describe("release branch rewrite records", () => { } as unknown as Response; } if (path.includes("/issues/") && path.includes("/comments")) { + if (handlers.commentPages) { + const page = Number(new URL(String(url)).searchParams.get("page") ?? "1"); + return { + ok: true, + status: 200, + json: async () => handlers.commentPages?.[page - 1] ?? [], + } as unknown as Response; + } return { ok: true, status: 200, @@ -252,6 +264,52 @@ describe("release branch rewrite records", () => { ).rejects.toThrow(/malformed release branch head list/iu); }); + // An entry this function cannot read makes its branch look absent, and an absent branch is + // force-pushed with no record of the head it discarded. That is the same claim as an unreadable + // response, so it is the same refusal rather than a skipped row. + it("refuses to regenerate when one matching-ref entry is unreadable", async () => { + stubGitHub({ malformedHeadEntry: true }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + openBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + }), + ).rejects.toThrow(/malformed release branch head list/iu); + }); + + // A release pull request outlives a hundred comments, and the marker this mechanism just wrote is + // the newest one -- exactly what a first-page read drops. Losing it leaves the force-push + // permanently unpaired, because every later run reads the same truncated page. + it("reads a rewrite record past the first page of comments", async () => { + const before = "c".repeat(40); + const after = "b".repeat(40); + const marker = ``; + const calls = stubGitHub({ + heads: after, + commentPages: [ + Array.from({ length: 100 }, (_unused, index) => ({ id: index + 1, body: "chatter" })), + [{ id: 501, body: marker }], + ], + }); + const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await closeBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, before]]), + }); + + const patched = calls.find((call) => call.method === "PATCH"); + expect(patched?.path).toContain("/issues/comments/501"); + expect(patched?.body).toContain(`after=${after}`); + }); + it("closes the open record with the head the rewrite created", async () => { const before = "c".repeat(40); const after = "b".repeat(40); From 113727816d62dfca584ac75b3df1a4e70653fdc5 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 03:57:50 +0530 Subject: [PATCH 04/17] refactor(release): keep who may write a marker in one place The marker parser told callers to check the comment's author themselves, and named no way to do it. One of the two readers did, with its own constant and its own login normaliser; the other did not, and that omission is a finding on this pull request. An instruction in a comment is not a mechanism. `isTrustedRewriteRecord` now lives beside the parser it qualifies, so both readers ask the same question of a comment, and a third reader has something to reach for rather than a note saying it ought to. The login normaliser moves with it, since the gate's review-author check wants the same normalisation and was the only reason a second copy existed. --- scripts/lib/release-branch-rewrite.mjs | 23 ++++++++++++++++++++--- scripts/publish-review-gate-check.mjs | 20 +++++++------------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/scripts/lib/release-branch-rewrite.mjs b/scripts/lib/release-branch-rewrite.mjs index fe2befc9..28f2b931 100644 --- a/scripts/lib/release-branch-rewrite.mjs +++ b/scripts/lib/release-branch-rewrite.mjs @@ -37,10 +37,27 @@ export function formatReleaseBranchRewriteMarker({ branch, before, after = null return ``; } +// Only the workflow that performs a rewrite can attest to what it discarded. Anyone who can +// comment can write the marker text, so the author is the whole of its authority: no human can post +// under this login. It lives beside the parser because a parser that cannot check authorship and an +// authority check kept somewhere else is how one of the two readers came to skip it. +const TRUSTED_REWRITE_RECORDER = "github-actions"; + +export function normaliseGithubLogin(login) { + return String(login ?? "") + .toLowerCase() + .replace(/\[bot\]$/u, ""); +} + +// A marker is evidence only from this author. Both readers -- the workflow that writes records and +// the gate that reads them -- ask this same question of a comment before trusting its marker. +export function isTrustedRewriteRecord(comment) { + return normaliseGithubLogin(comment?.user?.login) === TRUSTED_REWRITE_RECORDER; +} + // Returns `{ branch, before, after }` for a comment that carries the marker, or null. `after` is -// null for a record whose rewrite had not produced a head yet. Callers must check the comment's -// author themselves: only a marker written by the workflow that performed the rewrite is evidence, -// and anyone who can comment can write the text. +// null for a record whose rewrite had not produced a head yet. This reads the text only: pair it +// with `isTrustedRewriteRecord` before treating a marker as evidence or as recoverable state. export function readReleaseBranchRewriteMarker(body) { const match = MARKER_PATTERN.exec(body ?? ""); if (!match) return null; diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs index a17ebdb6..2c68651c 100644 --- a/scripts/publish-review-gate-check.mjs +++ b/scripts/publish-review-gate-check.mjs @@ -11,14 +11,14 @@ import { runGhText, } from "./lib/github-cli-retry.mjs"; import { readCleanTopLevelReviewCommit } from "./lib/codex-review-markers.mjs"; -import { readReleaseBranchRewriteMarker } from "./lib/release-branch-rewrite.mjs"; +import { + isTrustedRewriteRecord, + normaliseGithubLogin, + readReleaseBranchRewriteMarker, +} from "./lib/release-branch-rewrite.mjs"; const CHECK_RUN_NAME = "Review gate (scheduled)"; const REQUIRED_REVIEW_AUTHOR = "chatgpt-codex-connector"; -// Only the workflow that performs a rewrite can attest to what it discarded. Anyone who can -// comment can write the marker text, so the author is the whole of its authority: no human can -// post under this login. -const TRUSTED_REWRITE_RECORDER = "github-actions"; const DURABLE_REVIEW_STATE_PREFIX = "review-gate-state/v1\n"; const MAX_DURABLE_FORCE_PUSH_HISTORY_NODES = 20; const MAX_DURABLE_REVIEW_STATE_BYTES = 60_000; @@ -357,7 +357,7 @@ function loadTopLevelReviewedHeadShas(comments, currentPrShas, forcePushedPriorH const known = [...currentPrShas, ...forcePushedPriorHeads.map((head) => head.sha)]; const prefixes = new Set(); for (const comment of comments) { - if (normaliseLogin(comment?.user?.login) !== REQUIRED_REVIEW_AUTHOR) continue; + if (normaliseGithubLogin(comment?.user?.login) !== REQUIRED_REVIEW_AUTHOR) continue; const prefix = readCleanTopLevelReviewCommit(comment.body); // A marker naming a head already in hand needs no lookup, and the common case is the current // head naming itself. @@ -386,12 +386,6 @@ function resolveCommitSha(prefix) { return commit.sha; } -function normaliseLogin(login) { - return String(login ?? "") - .toLowerCase() - .replace(/\[bot\]$/u, ""); -} - function cleanReviewState(prNumber) { return JSON.stringify({ version: 1, prNumber, findings: [] }); } @@ -580,7 +574,7 @@ function loadIssueComments(prNumber) { function loadRecordedRewriteDiscards(comments) { const discards = new Map(); for (const comment of comments) { - if (normaliseLogin(comment?.user?.login) !== TRUSTED_REWRITE_RECORDER) continue; + if (!isTrustedRewriteRecord(comment)) continue; const marker = readReleaseBranchRewriteMarker(comment.body); // An open record names a discarded head and no replacement, so it pairs with no rewrite and // identifies nothing. A record whose heads match says the branch did not move, which is a From 80adf4a4fa4b84f8fda0b9dafc7da1c32c117c4f Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 03:57:50 +0530 Subject: [PATCH 05/17] fix(release): refuse three readings that let a rewrite go unrecorded Three list reads in this script decide whether a force-push is recorded, and each accepted an answer it should not have. **A marker from anyone.** Records read back were parsed without asking who wrote them. Anyone who can comment on a release pull request could have their comment taken for this workflow's own open record -- and completing a record rewrites the comment in place, so an unrelated comment would be overwritten, or an uneditable one would abort the run before any release work began. The workflow posts with `github.token`, so the recorder it trusts is the author it writes as. **An unreadable pull request entry read as "no pull request".** The caller then skips opening a record while the regeneration force-pushes the branch anyway. An empty list and an entry that cannot be read are different answers; only the first means there is nothing to record. **A head snapshot too old to say what is being discarded.** The record is opened before `createReleases()` so a failure there costs a re-run rather than a published release with no assets, which leaves a gap: anything reaching the branch between then and the force-push is discarded while the record still names the older head, and the gate accepts that pair and never searches the head that was lost. The record is now brought up to the branch's current head immediately before the rewrite. If it cannot be, the rewrite does not happen -- throwing would strand a published release without its assets, and a pull request that waits for the next run is recoverable where a discarded head is not. The refresh and the close walk the same path and differ only in what they write, so they share it. --- scripts/run-release-please.d.mts | 20 +++-- scripts/run-release-please.mjs | 135 ++++++++++++++++++++++++------- 2 files changed, 119 insertions(+), 36 deletions(-) diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 31dc4928..3ceac929 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -11,17 +11,21 @@ export function resolveReleaseTargetBranch( export function serializeGitHubOutput(outputs: Record): string; -export function openBranchRewriteRecords(options: { +interface ReleaseBranchScope { env: NodeJS.ProcessEnv | Record; owner: string; repo: string; targetBranch: string; -}): Promise>; +} -export function closeBranchRewriteRecords(options: { - env: NodeJS.ProcessEnv | Record; - owner: string; - repo: string; - targetBranch: string; +/** The heads the run snapshotted, which the later two stages bring up to date and close. */ +interface OpenedRewriteRecords extends ReleaseBranchScope { headsBeforeRegeneration: Map; -}): Promise; +} + +export function openBranchRewriteRecords(options: ReleaseBranchScope): Promise>; + +/** `false` when it could not confirm what the regeneration is about to discard. */ +export function refreshBranchRewriteRecords(options: OpenedRewriteRecords): Promise; + +export function closeBranchRewriteRecords(options: OpenedRewriteRecords): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index e2d8fa0b..9b1523ad 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -1,6 +1,7 @@ import { appendFile } from "node:fs/promises"; import { formatReleaseBranchRewriteMarker, + isTrustedRewriteRecord, readReleaseBranchRewriteMarker, } from "./lib/release-branch-rewrite.mjs"; import { createRequire } from "node:module"; @@ -63,7 +64,18 @@ export async function runReleasePlease(env = process.env) { configFile, manifestFile, ); - const pullRequests = (await pullRequestManifest.createPullRequests()).filter(Boolean); + // Immediately before the force-push, not at the snapshot above: see + // `refreshBranchRewriteRecords` for why the gap between the two is the dangerous part. + const recordsNameTheCurrentHeads = await refreshBranchRewriteRecords({ + env, + owner, + repo, + targetBranch, + headsBeforeRegeneration, + }); + const pullRequests = recordsNameTheCurrentHeads + ? (await pullRequestManifest.createPullRequests()).filter(Boolean) + : []; await closeBranchRewriteRecords({ env, owner, repo, targetBranch, headsBeforeRegeneration }); outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { @@ -101,7 +113,7 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch // the only thing that rewrites the branch and it has not run since. for (const record of records) { if (record.marker.branch !== branch || record.marker.after !== null) continue; - await completeBranchRewriteRecord({ env, owner, repo, record, after: head }); + await writeBranchRewriteRecord({ env, owner, repo, record, after: head }); } await githubRequest(env, `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments`, { method: "POST", @@ -126,24 +138,17 @@ export async function closeBranchRewriteRecords({ headsBeforeRegeneration, }) { try { - const headsAfter = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); - for (const branch of headsBeforeRegeneration.keys()) { - const after = headsAfter.get(branch); - // An unchanged head discarded nothing. Completing the record with `after === before` says - // exactly that, and the gate reads it as the non-event it was. - if (!after) continue; - const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, repo, branch }); - if (pullRequestNumber === null) continue; - const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); - const open = records.find( - (record) => record.marker.branch === branch && record.marker.after === null, - ); - if (!open) continue; - await completeBranchRewriteRecord({ env, owner, repo, record: open, after }); - console.log( - `Closed release branch rewrite record on #${pullRequestNumber}: ${open.marker.before} -> ${after}`, - ); - } + await eachOpenBranchRewriteRecord( + { env, owner, repo, targetBranch, branches: headsBeforeRegeneration.keys() }, + async ({ pullRequestNumber, record, head }) => { + // An unchanged head discarded nothing. Completing the record with `after === before` says + // exactly that, and the gate reads it as the non-event it was. + await writeBranchRewriteRecord({ env, owner, repo, record, after: head }); + console.log( + `Closed release branch rewrite record on #${pullRequestNumber}: ${record.marker.before} -> ${head}`, + ); + }, + ); } catch (error) { console.error( `Could not close a release branch rewrite record: ${error.message}. The record stays open and the next run completes it.`, @@ -151,15 +156,78 @@ export async function closeBranchRewriteRecords({ } } -async function completeBranchRewriteRecord({ env, owner, repo, record, after }) { +/** + * Brings every open record up to the head its branch actually carries, and reports whether it + * could. `false` means the regeneration must not proceed. + * + * The record is opened before `createReleases()` so that a failure there costs a re-run rather + * than a published release with no assets. That ordering leaves a gap: anything reaching the + * branch between then and the force-push is discarded while the record still names the older head, + * and the gate accepts that recorded pair and never searches the head that was lost. + * + * Throwing is not available here -- the release already exists and a later workflow step uploads + * its assets -- so the failure is reported instead and the caller skips the rewrite. Discarding a + * head no record names is unrecoverable and is the whole point of this mechanism; a release whose + * pull request waits for the next run is not. + */ +export async function refreshBranchRewriteRecords({ + env, + owner, + repo, + targetBranch, + headsBeforeRegeneration, +}) { + try { + await eachOpenBranchRewriteRecord( + { env, owner, repo, targetBranch, branches: headsBeforeRegeneration.keys() }, + async ({ pullRequestNumber, record, head }) => { + if (head === record.marker.before) return; + await writeBranchRewriteRecord({ env, owner, repo, record, before: head }); + console.log( + `Refreshed release branch rewrite record on #${pullRequestNumber}: before=${head}`, + ); + }, + ); + return true; + } catch (error) { + console.error( + `Could not confirm what the regeneration is about to discard: ${error.message}. Skipping the release pull request so nothing is discarded unrecorded; the next run retries.`, + ); + return false; + } +} + +/** + * The walk the refresh and the close share: each branch's open record, paired with the head its + * branch carries right now. They differ only in what they write to it. + */ +async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, branches }, visit) { + const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); + for (const branch of branches) { + const head = heads.get(branch); + if (!head) continue; + const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, repo, branch }); + if (pullRequestNumber === null) continue; + const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); + const open = records.find( + (record) => record.marker.branch === branch && record.marker.after === null, + ); + if (open) await visit({ pullRequestNumber, record: open, head }); + } +} + +async function writeBranchRewriteRecord({ + env, + owner, + repo, + record, + before = record.marker.before, + after = null, +}) { await githubRequest(env, `/repos/${owner}/${repo}/issues/comments/${record.id}`, { method: "PATCH", body: JSON.stringify({ - body: formatReleaseBranchRewriteMarker({ - branch: record.marker.branch, - before: record.marker.before, - after, - }), + body: formatReleaseBranchRewriteMarker({ branch: record.marker.branch, before, after }), }), }); } @@ -172,6 +240,11 @@ async function readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }) ); const records = []; for (const comment of comments) { + // The author is the whole of a marker's authority. Without this, anyone who can comment on a + // release pull request could have their comment treated as this workflow's own open record -- + // and completing a record rewrites the comment in place, so an unrelated comment would be + // overwritten, or an uneditable one would abort the run before any release work began. + if (!isTrustedRewriteRecord(comment)) continue; const marker = readReleaseBranchRewriteMarker(comment?.body); if (marker && Number.isInteger(comment?.id)) records.push({ id: comment.id, marker }); } @@ -184,8 +257,14 @@ async function findOpenPullRequestNumber({ env, owner, repo, branch }) { `/repos/${owner}/${repo}/pulls?state=open&head=${owner}:${branch}`, "pull request list for a release branch", ); - const number = pulls.find((pull) => Number.isInteger(pull?.number))?.number; - return number ?? null; + // An empty list means no open pull request; an entry that cannot be read means the answer is + // unknown, and the two must not collapse into one. Read as "none", an unreadable entry makes the + // caller skip opening a record while the regeneration force-pushes the branch anyway -- which + // discards a head nothing named, the exact loss this mechanism exists to prevent. + if (pulls.length > 0 && !pulls.some((pull) => Number.isInteger(pull?.number))) { + throw new Error("GitHub returned a malformed pull request list for a release branch."); + } + return pulls.find((pull) => Number.isInteger(pull?.number))?.number ?? null; } async function readReleaseBranchHeads({ env, owner, repo, targetBranch }) { From 261f140913f8a12259263c9ac37ccc6989fdd567 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 03:57:50 +0530 Subject: [PATCH 06/17] test(release): pin the three readings a rewrite record depends on Each fails with its guard removed. The comment fixtures now carry an author, which real ones always do -- that they did not is why the gap was invisible. --- tests/scripts/run-release-please.test.ts | 124 +++++++++++++++++++++-- 1 file changed, 113 insertions(+), 11 deletions(-) diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 0a7f3a67..39dae619 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -12,6 +12,10 @@ import { const require = createRequire(import.meta.url); const releasePlease = require("release-please"); +// A marker is evidence only from the workflow that wrote it, so every fixture comment carrying one +// must say who posted it. Anyone can write the text. +const RECORDER = "github-actions[bot]"; + describe("Release Please workflow wrapper", () => { it("emits root release outputs compatible with release-please-action", () => { const outputs = buildReleaseOutputs([ @@ -128,12 +132,13 @@ describe("Release Please workflow wrapper", () => { prs_created: "true", release_created: "true", }); - // The only direct GitHub calls are the two branch-head reads that bracket the - // regeneration. Nothing is written, because this run rewrote no branch. - expect(fetched).toEqual([ - "GET /repos/lamemustafa/pack/git/matching-refs/heads/release-please--branches--master--", - "GET /repos/lamemustafa/pack/git/matching-refs/heads/release-please--branches--master--", - ]); + // The only direct GitHub calls are three branch-head reads: the snapshot taken before + // anything irreversible happens, the re-read immediately before the rewrite, and the read + // after it. The middle one is the point -- the first is too old to say what the force-push + // is about to discard. Nothing is written, because this run rewrote no branch. + const headRead = + "GET /repos/lamemustafa/pack/git/matching-refs/heads/release-please--branches--master--"; + expect(fetched).toEqual([headRead, headRead, headRead]); } finally { vi.unstubAllGlobals(); log.mockRestore(); @@ -150,11 +155,11 @@ describe("release branch rewrite records", () => { function stubGitHub(handlers: { heads?: string | null; - comments?: Array<{ id: number; body: string }>; + comments?: Array<{ id: number; body: string; user?: { login: string } }>; pulls?: Array<{ number: number }>; malformedHeads?: boolean; malformedHeadEntry?: boolean; - commentPages?: Array>; + commentPages?: Array>; }) { const calls: Array<{ method: string; path: string; body?: string }> = []; vi.stubGlobal( @@ -229,7 +234,11 @@ describe("release branch rewrite records", () => { const calls = stubGitHub({ heads: head, comments: [ - { id: 99, body: `` }, + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, ], }); const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); @@ -248,6 +257,95 @@ describe("release branch rewrite records", () => { expect(patched?.body).toContain(`before=${lostBefore} after=${head}`); }); + it("ignores a marker written by anyone but the workflow", async () => { + // The author is the whole of a marker's authority: anyone who can comment can write the text. + // Treating a stranger's comment as an open record would rewrite that comment in place, or -- + // if it cannot be edited -- abort the run before any release work began. + const calls = stubGitHub({ + heads: "b".repeat(40), + comments: [ + { + id: 99, + user: { login: "a-passer-by" }, + body: ``, + }, + ], + }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await openBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + }); + + expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); + }); + + it("refuses to regenerate when a pull request entry cannot be read", async () => { + // A list that is empty means no open pull request. A list whose entry cannot be read means the + // answer is unknown. Collapsing the second into the first skips the record while the rewrite + // force-pushes anyway, discarding a head nothing named. + stubGitHub({ heads: "b".repeat(40), pulls: [{ id: 1 } as unknown as { number: number }] }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + openBranchRewriteRecords({ env, owner: "lamemustafa", repo: "pack", targetBranch: "master" }), + ).rejects.toThrow(/malformed pull request list/iu); + }); + + it("records the head the branch carries at the rewrite, not at the snapshot", async () => { + // The record is opened before `createReleases()` so a failure there costs a re-run rather than + // a release with no assets. Anything reaching the branch in that gap would otherwise be + // discarded while the record still named the older head, and the gate would never search it. + const snapshot = "b".repeat(40); + const arrivedSince = "e".repeat(40); + // The branch already carries the newer head by the time the refresh reads it. + const calls = stubGitHub({ + heads: arrivedSince, + comments: [ + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, + ], + }); + const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + const proceeded = await refreshBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, snapshot]]), + }); + + expect(proceeded).toBe(true); + const patched = calls.find((call) => call.method === "PATCH"); + expect(patched?.body).toContain(`before=${arrivedSince}`); + expect(patched?.body).not.toContain("after="); + }); + + it("refuses to regenerate when it cannot confirm what is about to be discarded", async () => { + // Throwing is unavailable here: the release exists and a later workflow step uploads its + // assets. So the rewrite is skipped instead -- a pull request that waits for the next run is + // recoverable, a head discarded with no record of it is not. + stubGitHub({ malformedHeads: true }); + const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + const proceeded = await refreshBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, "b".repeat(40)]]), + }); + + expect(proceeded).toBe(false); + }); + it("refuses to regenerate when the head list is malformed", async () => { stubGitHub({ malformedHeads: true }); const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); @@ -292,7 +390,7 @@ describe("release branch rewrite records", () => { heads: after, commentPages: [ Array.from({ length: 100 }, (_unused, index) => ({ id: index + 1, body: "chatter" })), - [{ id: 501, body: marker }], + [{ id: 501, user: { login: RECORDER }, body: marker }], ], }); const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); @@ -316,7 +414,11 @@ describe("release branch rewrite records", () => { const calls = stubGitHub({ heads: after, comments: [ - { id: 99, body: `` }, + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, ], }); const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); From 1f5d4c5ed144dc6b2ce778a1d50732e66fe5986e Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 13:27:51 +0530 Subject: [PATCH 07/17] fix(release): name the right pull request, and the head the rewrite created **The lookup matched on head alone.** A generated branch can carry open pull requests against more than one base, and only the one targeting this run's branch is the release pull request being regenerated. Matched on head, the record could be opened and closed on a different pull request while the force-push rewrote this one, leaving the rewrite that mattered unrecorded. **The close read the branch, not the rewrite.** The review gate looks a record up by the head the force-push *created* -- it keys on the timeline event's `commit_id`. An ordinary commit landing on the branch between the rewrite and this read would have the record name a head no event mentions, and the gate would treat the discard as unidentified: exactly as if nothing had recorded it. I checked whether ancestry saved this, since an ordinary commit leaves the created head as its ancestor. It does not: `loadRecordedRewriteDiscards` keys the map by `marker.after` and the lookup is an exact SHA match, not a walk. The close now reads the created head from the force-push event itself. When no event names it, the record stays open -- which is how this module answers not knowing everywhere else: the gate ignores an open record and the next run completes it. Closing with an uncorroborated head would publish a claim about a rewrite nothing backs. --- scripts/run-release-please.mjs | 75 +++++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 9b1523ad..c46181e3 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -105,7 +105,13 @@ export async function runReleasePlease(env = process.env) { export async function openBranchRewriteRecords({ env, owner, repo, targetBranch }) { const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); for (const [branch, head] of heads) { - const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, repo, branch }); + const pullRequestNumber = await findOpenPullRequestNumber({ + env, + owner, + repo, + branch, + targetBranch, + }); if (pullRequestNumber === null) continue; const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); // A record left open by an interrupted run names a discarded head and no replacement. The @@ -143,9 +149,31 @@ export async function closeBranchRewriteRecords({ async ({ pullRequestNumber, record, head }) => { // An unchanged head discarded nothing. Completing the record with `after === before` says // exactly that, and the gate reads it as the non-event it was. - await writeBranchRewriteRecord({ env, owner, repo, record, after: head }); + if (head === record.marker.before) { + await writeBranchRewriteRecord({ env, owner, repo, record, after: head }); + console.log( + `Closed release branch rewrite record on #${pullRequestNumber}: branch unchanged.`, + ); + return; + } + // Otherwise the head is read from the rewrite itself, not from the branch. The gate looks + // this record up by the head the force-push *created* -- it keys on the timeline event's + // `commit_id` -- so an ordinary commit landing on the branch before this read would have + // the record name a head no event mentions, and the rewrite would stay unidentified + // exactly as if nothing had recorded it. + const created = await readLatestForcePushedHead({ env, owner, repo, pullRequestNumber }); + if (!created) { + // Leaving it open is the established answer to not knowing: the gate ignores an open + // record, and the next run completes it. Closing it with an uncorroborated head would + // publish a claim about a rewrite that no event backs. + console.error( + `Could not name the head the regeneration created on #${pullRequestNumber}. The record stays open and the next run completes it.`, + ); + return; + } + await writeBranchRewriteRecord({ env, owner, repo, record, after: created }); console.log( - `Closed release branch rewrite record on #${pullRequestNumber}: ${record.marker.before} -> ${head}`, + `Closed release branch rewrite record on #${pullRequestNumber}: ${record.marker.before} -> ${created}`, ); }, ); @@ -197,6 +225,31 @@ export async function refreshBranchRewriteRecords({ } } +/** + * The head named by this pull request's most recent force-push, or `null`. + * + * `commit_id` on a `head_ref_force_pushed` event is the head that rewrite created -- the same + * field the review gate keys recorded discards by. Reading it here is what makes the two sides + * agree about which rewrite a record describes. + */ +async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber }) { + const timeline = await githubList( + env, + `/repos/${owner}/${repo}/issues/${pullRequestNumber}/timeline`, + "pull request timeline for a release branch", + ); + let latest = null; + for (const event of timeline) { + if (event?.event !== "head_ref_force_pushed") continue; + const sha = String(event.commit_id ?? "").toLowerCase(); + if (!/^[0-9a-f]{40}$/u.test(sha)) continue; + const at = Date.parse(event.created_at ?? ""); + if (!Number.isFinite(at)) continue; + if (!latest || at > latest.at) latest = { sha, at }; + } + return latest?.sha ?? null; +} + /** * The walk the refresh and the close share: each branch's open record, paired with the head its * branch carries right now. They differ only in what they write to it. @@ -206,7 +259,13 @@ async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, bra for (const branch of branches) { const head = heads.get(branch); if (!head) continue; - const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, repo, branch }); + const pullRequestNumber = await findOpenPullRequestNumber({ + env, + owner, + repo, + branch, + targetBranch, + }); if (pullRequestNumber === null) continue; const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); const open = records.find( @@ -251,10 +310,14 @@ async function readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }) return records; } -async function findOpenPullRequestNumber({ env, owner, repo, branch }) { +async function findOpenPullRequestNumber({ env, owner, repo, branch, targetBranch }) { + // Head *and* base. A generated branch can carry open pull requests against more than one base, + // and only the one targeting this run's branch is the release pull request being regenerated. + // Matched on head alone, the record could be opened and closed on a different pull request + // while the force-push rewrote this one -- leaving the rewrite that mattered unrecorded. const pulls = await githubList( env, - `/repos/${owner}/${repo}/pulls?state=open&head=${owner}:${branch}`, + `/repos/${owner}/${repo}/pulls?state=open&head=${owner}:${branch}&base=${targetBranch}`, "pull request list for a release branch", ); // An empty list means no open pull request; an entry that cannot be read means the answer is From 290757054c030780c0720316810d55fcd6e244da Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 13:27:51 +0530 Subject: [PATCH 08/17] test(release): pin which pull request, and which head, a record names Both fail when their guard is reverted: closing from the branch head fails the two head cases, and dropping the base filter fails the lookup case. --- tests/scripts/run-release-please.test.ts | 98 ++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 39dae619..1c8f0b2a 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -160,6 +160,7 @@ describe("release branch rewrite records", () => { malformedHeads?: boolean; malformedHeadEntry?: boolean; commentPages?: Array>; + forcePushedHeads?: Array<{ commit_id?: string; created_at?: string }>; }) { const calls: Array<{ method: string; path: string; body?: string }> = []; vi.stubGlobal( @@ -184,6 +185,20 @@ describe("release branch rewrite records", () => { json: async () => handlers.pulls ?? [{ number: 337 }], } as unknown as Response; } + if (path.includes("/timeline")) { + // `commit_id` on a force-push event is the head that rewrite created -- the field the + // review gate keys recorded discards by. + return { + ok: true, + status: 200, + json: async () => + (handlers.forcePushedHeads ?? []).map((entry) => ({ + event: "head_ref_force_pushed", + created_at: entry.created_at ?? "2026-09-12T00:00:00Z", + ...entry, + })), + } as unknown as Response; + } if (path.includes("/issues/") && path.includes("/comments")) { if (handlers.commentPages) { const page = Number(new URL(String(url)).searchParams.get("page") ?? "1"); @@ -388,6 +403,7 @@ describe("release branch rewrite records", () => { const marker = ``; const calls = stubGitHub({ heads: after, + forcePushedHeads: [{ commit_id: after }], commentPages: [ Array.from({ length: 100 }, (_unused, index) => ({ id: index + 1, body: "chatter" })), [{ id: 501, user: { login: RECORDER }, body: marker }], @@ -413,6 +429,7 @@ describe("release branch rewrite records", () => { const after = "b".repeat(40); const calls = stubGitHub({ heads: after, + forcePushedHeads: [{ commit_id: after }], comments: [ { id: 99, @@ -435,6 +452,87 @@ describe("release branch rewrite records", () => { expect(patched?.body).toContain(`before=${before} after=${after}`); }); + it("closes with the head the rewrite created, not the one the branch carries now", async () => { + // The gate looks a record up by the head the force-push created -- the timeline event's + // `commit_id`. An ordinary commit landing on the branch before this read would otherwise have + // the record name a head no event mentions, leaving the rewrite as unidentified as if nothing + // had recorded it at all. + const before = "c".repeat(40); + const created = "b".repeat(40); + const landedSince = "e".repeat(40); + const calls = stubGitHub({ + heads: landedSince, + forcePushedHeads: [{ commit_id: created }], + comments: [ + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, + ], + }); + const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await closeBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, before]]), + }); + + const patched = calls.find((call) => call.method === "PATCH"); + expect(patched?.body).toContain(`before=${before} after=${created}`); + expect(patched?.body).not.toContain(landedSince); + }); + + it("leaves the record open when no event names the head the rewrite created", async () => { + // Not knowing is answered the way this module answers it everywhere: the gate ignores an open + // record and the next run completes it. Closing with an uncorroborated head would publish a + // claim about a rewrite nothing backs. + const before = "c".repeat(40); + const calls = stubGitHub({ + heads: "b".repeat(40), + forcePushedHeads: [], + comments: [ + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, + ], + }); + const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await closeBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([[branch, before]]), + }); + + expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); + }); + + it("looks for the release pull request by base branch as well as head", async () => { + // A generated branch can carry open pull requests against more than one base. Matched on head + // alone, the record could be opened on a different pull request while the force-push rewrote + // this one. + const calls = stubGitHub({ heads: "b".repeat(40) }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await openBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + }); + + const lookup = calls.find((call) => call.path.includes("/pulls?")); + expect(lookup?.path).toContain("base=master"); + }); + it("leaves the record open rather than failing a run that already published a release", async () => { vi.stubGlobal( "fetch", From 4dc1e4e8dba8826e3f30f44b75fdd4de7ec8e166 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 15:39:57 +0530 Subject: [PATCH 09/17] fix(release): retain rewrite marker identity --- scripts/run-release-please.d.mts | 11 ++- scripts/run-release-please.mjs | 104 +++++++++++++++++------ tests/scripts/run-release-please.test.ts | 41 +++++++-- 3 files changed, 122 insertions(+), 34 deletions(-) diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 3ceac929..3e287ae9 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -18,12 +18,17 @@ interface ReleaseBranchScope { targetBranch: string; } -/** The heads the run snapshotted, which the later two stages bring up to date and close. */ +/** The heads and durable marker identities opened for this regeneration. */ interface OpenedRewriteRecords extends ReleaseBranchScope { - headsBeforeRegeneration: Map; + headsBeforeRegeneration: Map< + string, + { head: string; recordId: number; pullRequestNumber: number } | string + >; } -export function openBranchRewriteRecords(options: ReleaseBranchScope): Promise>; +export function openBranchRewriteRecords( + options: ReleaseBranchScope, +): Promise>; /** `false` when it could not confirm what the regeneration is about to discard. */ export function refreshBranchRewriteRecords(options: OpenedRewriteRecords): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index c46181e3..2d1a7a73 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -104,6 +104,7 @@ export async function runReleasePlease(env = process.env) { // lose the discarded SHA permanently, and no later run could reconstruct it. export async function openBranchRewriteRecords({ env, owner, repo, targetBranch }) { const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); + const opened = new Map(); for (const [branch, head] of heads) { const pullRequestNumber = await findOpenPullRequestNumber({ env, @@ -114,22 +115,47 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch }); if (pullRequestNumber === null) continue; const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); - // A record left open by an interrupted run names a discarded head and no replacement. The - // branch head standing here now is exactly what that rewrite created, because this workflow is - // the only thing that rewrites the branch and it has not run since. - for (const record of records) { - if (record.marker.branch !== branch || record.marker.after !== null) continue; - await writeBranchRewriteRecord({ env, owner, repo, record, after: head }); + const open = records.find( + (record) => record.marker.branch === branch && record.marker.after === null, + ); + if (open) { + const created = await readLatestForcePushedHead({ + env, + owner, + repo, + pullRequestNumber, + after: open.createdAt, + }); + if (created) { + await writeBranchRewriteRecord({ env, owner, repo, record: open, after: created }); + } else if (head === open.marker.before) { + opened.set(branch, { head, recordId: open.id, pullRequestNumber }); + continue; + } else { + throw new Error( + "GitHub could not correlate an open rewrite record with the branch head now standing; refusing to discard an unrecorded head.", + ); + } } - await githubRequest(env, `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments`, { - method: "POST", - body: JSON.stringify({ - body: formatReleaseBranchRewriteMarker({ branch, before: head }), - }), - }); + const created = await githubRequest( + env, + `/repos/${owner}/${repo}/issues/${pullRequestNumber}/comments`, + { + method: "POST", + body: JSON.stringify({ + body: formatReleaseBranchRewriteMarker({ branch, before: head }), + }), + }, + ); + if (!Number.isInteger(created?.id)) { + throw new Error( + "GitHub did not return an identifier for the opened release branch rewrite record.", + ); + } + opened.set(branch, { head, recordId: created.id, pullRequestNumber }); console.log(`Opened release branch rewrite record on #${pullRequestNumber}: before=${head}`); } - return heads; + return opened; } // Failures here are logged rather than thrown. A release may already exist by this point, and @@ -145,7 +171,7 @@ export async function closeBranchRewriteRecords({ }) { try { await eachOpenBranchRewriteRecord( - { env, owner, repo, targetBranch, branches: headsBeforeRegeneration.keys() }, + { env, owner, repo, targetBranch, expected: headsBeforeRegeneration }, async ({ pullRequestNumber, record, head }) => { // An unchanged head discarded nothing. Completing the record with `after === before` says // exactly that, and the gate reads it as the non-event it was. @@ -207,7 +233,7 @@ export async function refreshBranchRewriteRecords({ }) { try { await eachOpenBranchRewriteRecord( - { env, owner, repo, targetBranch, branches: headsBeforeRegeneration.keys() }, + { env, owner, repo, targetBranch, expected: headsBeforeRegeneration }, async ({ pullRequestNumber, record, head }) => { if (head === record.marker.before) return; await writeBranchRewriteRecord({ env, owner, repo, record, before: head }); @@ -232,7 +258,7 @@ export async function refreshBranchRewriteRecords({ * field the review gate keys recorded discards by. Reading it here is what makes the two sides * agree about which rewrite a record describes. */ -async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber }) { +async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber, after = 0 }) { const timeline = await githubList( env, `/repos/${owner}/${repo}/issues/${pullRequestNumber}/timeline`, @@ -244,7 +270,7 @@ async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber } const sha = String(event.commit_id ?? "").toLowerCase(); if (!/^[0-9a-f]{40}$/u.test(sha)) continue; const at = Date.parse(event.created_at ?? ""); - if (!Number.isFinite(at)) continue; + if (!Number.isFinite(at) || at < after) continue; if (!latest || at > latest.at) latest = { sha, at }; } return latest?.sha ?? null; @@ -254,11 +280,18 @@ async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber } * The walk the refresh and the close share: each branch's open record, paired with the head its * branch carries right now. They differ only in what they write to it. */ -async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, branches }, visit) { +async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, expected }, visit) { const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); - for (const branch of branches) { + for (const [branch, expectedRecord] of expected) { + // String entries keep the exported helper compatible with older callers while production runs + // always retain the marker id returned when the record was opened. + const expectedInfo = + typeof expectedRecord === "string" + ? { head: expectedRecord, recordId: null, pullRequestNumber: null } + : expectedRecord; const head = heads.get(branch); - if (!head) continue; + if (!head) + throw new Error(`Release branch ${branch} disappeared after its rewrite record was opened.`); const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, @@ -266,12 +299,28 @@ async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, bra branch, targetBranch, }); - if (pullRequestNumber === null) continue; + if ( + pullRequestNumber === null || + (expectedInfo.pullRequestNumber !== null && + pullRequestNumber !== expectedInfo.pullRequestNumber) + ) { + throw new Error( + `Release pull request for ${branch} changed after its rewrite record was opened.`, + ); + } const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); const open = records.find( - (record) => record.marker.branch === branch && record.marker.after === null, + (record) => + (expectedInfo.recordId === null || record.id === expectedInfo.recordId) && + record.marker.branch === branch && + record.marker.after === null, ); - if (open) await visit({ pullRequestNumber, record: open, head }); + if (!open) { + throw new Error( + `Release branch ${branch} lost the rewrite record opened for this regeneration.`, + ); + } + await visit({ pullRequestNumber, record: open, head }); } } @@ -305,7 +354,14 @@ async function readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }) // overwritten, or an uneditable one would abort the run before any release work began. if (!isTrustedRewriteRecord(comment)) continue; const marker = readReleaseBranchRewriteMarker(comment?.body); - if (marker && Number.isInteger(comment?.id)) records.push({ id: comment.id, marker }); + const createdAt = Date.parse(comment?.created_at ?? ""); + if (marker && Number.isInteger(comment?.id)) { + records.push({ + id: comment.id, + marker, + createdAt: Number.isFinite(createdAt) ? createdAt : 0, + }); + } } return records; } diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 1c8f0b2a..8ac0a2c9 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -200,6 +200,13 @@ describe("release branch rewrite records", () => { } as unknown as Response; } if (path.includes("/issues/") && path.includes("/comments")) { + if (String(init.method ?? "GET") === "POST") { + return { + ok: true, + status: 201, + json: async () => ({ id: 999 }), + } as unknown as Response; + } if (handlers.commentPages) { const page = Number(new URL(String(url)).searchParams.get("page") ?? "1"); return { @@ -236,18 +243,19 @@ describe("release branch rewrite records", () => { targetBranch: "master", }); - expect(heads.get(branch)).toBe(head); + expect(heads.get(branch)).toMatchObject({ head, pullRequestNumber: 337, recordId: 999 }); const posted = calls.find((call) => call.method === "POST"); expect(posted?.path).toContain("/issues/337/comments"); expect(posted?.body).toContain(`review-gate-rewrite branch=${branch} before=${head}`); expect(posted?.body).not.toContain("after="); }); - it("completes a record an interrupted run left open, from the head standing now", async () => { + it("completes an interrupted record from its force-push event, not the current ref", async () => { const lostBefore = "c".repeat(40); - const head = "b".repeat(40); + const created = "b".repeat(40); + const landedSince = "e".repeat(40); const calls = stubGitHub({ - heads: head, + heads: landedSince, comments: [ { id: 99, @@ -255,6 +263,7 @@ describe("release branch rewrite records", () => { body: ``, }, ], + forcePushedHeads: [{ commit_id: created }], }); const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); @@ -265,11 +274,10 @@ describe("release branch rewrite records", () => { targetBranch: "master", }); - // The branch head standing here is exactly what the lost rewrite created, because nothing but - // this workflow rewrites the branch and it has not run since. const patched = calls.find((call) => call.method === "PATCH"); expect(patched?.path).toContain("/issues/comments/99"); - expect(patched?.body).toContain(`before=${lostBefore} after=${head}`); + expect(patched?.body).toContain(`before=${lostBefore} after=${created}`); + expect(patched?.body).not.toContain(landedSince); }); it("ignores a marker written by anyone but the workflow", async () => { @@ -361,6 +369,25 @@ describe("release branch rewrite records", () => { expect(proceeded).toBe(false); }); + it("refuses to regenerate when its opened marker is no longer present", async () => { + // A successful POST is not durable evidence if a later read cannot find that exact comment. + // Continuing would force-push the head the marker was meant to preserve. + stubGitHub({ heads: "b".repeat(40), comments: [] }); + const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + refreshBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([ + [branch, { head: "b".repeat(40), recordId: 999, pullRequestNumber: 337 }], + ]), + }), + ).resolves.toBe(false); + }); + it("refuses to regenerate when the head list is malformed", async () => { stubGitHub({ malformedHeads: true }); const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); From c8231242c02523562a09ef19869a9d581fe9fc30 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 15:43:09 +0530 Subject: [PATCH 10/17] fix(release): compare generated branch rewrites --- scripts/run-release-please.d.mts | 10 ++++ scripts/run-release-please.mjs | 60 ++++++++++++++++++++- tests/scripts/run-release-please.test.ts | 67 ++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 1 deletion(-) diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 3e287ae9..40599a51 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -34,3 +34,13 @@ export function openBranchRewriteRecords( export function refreshBranchRewriteRecords(options: OpenedRewriteRecords): Promise; export function closeBranchRewriteRecords(options: OpenedRewriteRecords): Promise; + +export function withReleaseBranchRewriteCas( + github: { + repository: { owner: string; repo: string }; + graphql: (query: string, variables: Record) => Promise; + octokit: { git: { updateRef: (request: Record) => Promise } }; + }, + expected: OpenedRewriteRecords["headsBeforeRegeneration"], + operation: () => Promise, +): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 2d1a7a73..1cfe902b 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -74,7 +74,11 @@ export async function runReleasePlease(env = process.env) { headsBeforeRegeneration, }); const pullRequests = recordsNameTheCurrentHeads - ? (await pullRequestManifest.createPullRequests()).filter(Boolean) + ? headsBeforeRegeneration.size > 0 + ? await withReleaseBranchRewriteCas(github, headsBeforeRegeneration, async () => + (await pullRequestManifest.createPullRequests()).filter(Boolean), + ) + : (await pullRequestManifest.createPullRequests()).filter(Boolean) : []; await closeBranchRewriteRecords({ env, owner, repo, targetBranch, headsBeforeRegeneration }); outputs.prs_created = String(pullRequests.length > 0); @@ -237,6 +241,8 @@ export async function refreshBranchRewriteRecords({ async ({ pullRequestNumber, record, head }) => { if (head === record.marker.before) return; await writeBranchRewriteRecord({ env, owner, repo, record, before: head }); + const expectedRecord = headsBeforeRegeneration.get(record.marker.branch); + if (expectedRecord && typeof expectedRecord !== "string") expectedRecord.head = head; console.log( `Refreshed release branch rewrite record on #${pullRequestNumber}: before=${head}`, ); @@ -251,6 +257,58 @@ export async function refreshBranchRewriteRecords({ } } +// Release Please delegates generated-branch updates to an unconditional REST force-push. That +// leaves a race after the marker is refreshed: a commit that arrives before the push is discarded +// even though the marker names the earlier head. GitHub's `updateRefs` mutation accepts `beforeOid`, +// so replace only those already-recorded generated-branch updates with a compare-and-swap. A +// mismatch rejects the release run while its open marker remains durable for recovery. +export async function withReleaseBranchRewriteCas(github, expected, operation) { + const updates = github.octokit?.git?.updateRef; + if (typeof updates !== "function" || typeof github.graphql !== "function") { + throw new Error("Release Please did not expose the GitHub clients needed for rewrite CAS."); + } + const repository = await github.graphql( + "query ReleaseBranchRewriteRepository($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { id } }", + { owner: github.repository.owner, name: github.repository.repo }, + ); + const repositoryId = repository?.repository?.id; + if (typeof repositoryId !== "string" || !repositoryId) { + throw new Error("GitHub did not return a repository id for release branch rewrite CAS."); + } + github.octokit.git.updateRef = async (request) => { + const branch = String(request?.ref ?? "").replace(/^heads\//u, ""); + const record = expected.get(branch); + if (!record || typeof record === "string") return updates.call(github.octokit.git, request); + const afterOid = String(request?.sha ?? "").toLowerCase(); + if (!/^[0-9a-f]{40}$/u.test(afterOid)) { + throw new Error(`Release Please gave rewrite CAS an invalid destination for ${branch}.`); + } + const result = await github.graphql( + "mutation ReleaseBranchRewriteCas($repositoryId: ID!, $refUpdates: [RefUpdate!]!) { updateRefs(input: { repositoryId: $repositoryId, refUpdates: $refUpdates }) { clientMutationId } }", + { + repositoryId, + refUpdates: [ + { + name: `refs/heads/${branch}`, + beforeOid: record.head, + afterOid, + force: true, + }, + ], + }, + ); + if (!result?.updateRefs) { + throw new Error(`GitHub did not confirm the compare-and-swap update for ${branch}.`); + } + return { data: { object: { sha: afterOid } } }; + }; + try { + return await operation(); + } finally { + github.octokit.git.updateRef = updates; + } +} + /** * The head named by this pull request's most recent force-push, or `null`. * diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 8ac0a2c9..3a3b2cd5 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -7,6 +7,7 @@ import { resolveReleaseTargetBranch, runReleasePlease, serializeGitHubOutput, + withReleaseBranchRewriteCas, } from "../../scripts/run-release-please.mjs"; const require = createRequire(import.meta.url); @@ -17,6 +18,72 @@ const releasePlease = require("release-please"); const RECORDER = "github-actions[bot]"; describe("Release Please workflow wrapper", () => { + it("uses beforeOid CAS only for a recorded generated branch", async () => { + const branch = "release-please--branches--master--components--pack"; + const before = "b".repeat(40); + const after = "c".repeat(40); + const originalUpdate = vi.fn().mockResolvedValue({ data: { object: { sha: after } } }); + const graphql = vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockResolvedValueOnce({ updateRefs: { clientMutationId: null } }); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql, + octokit: { git: { updateRef: originalUpdate } }, + }; + + await withReleaseBranchRewriteCas( + github, + new Map([[branch, { head: before, recordId: 1, pullRequestNumber: 2 }]]), + async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }), + ); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(graphql).toHaveBeenLastCalledWith( + expect.stringContaining("updateRefs"), + expect.objectContaining({ + repositoryId: "repo-id", + refUpdates: [ + expect.objectContaining({ + name: `refs/heads/${branch}`, + beforeOid: before, + afterOid: after, + force: true, + }), + ], + }), + ); + expect(github.octokit.git.updateRef).toBe(originalUpdate); + }); + + it("rejects a raced generated-branch update and leaves the caller's record intact", async () => { + const branch = "release-please--branches--master--components--pack"; + const before = "b".repeat(40); + const after = "c".repeat(40); + const originalUpdate = vi.fn(); + const graphql = vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockRejectedValueOnce(new Error("Reference update failed: beforeOid does not match")); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql, + octokit: { git: { updateRef: originalUpdate } }, + }; + const records = new Map([[branch, { head: before, recordId: 1, pullRequestNumber: 2 }]]); + + await expect( + withReleaseBranchRewriteCas(github, records, async () => + github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }), + ), + ).rejects.toThrow(/beforeOid does not match/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(records.get(branch)).toMatchObject({ head: before, recordId: 1 }); + expect(github.octokit.git.updateRef).toBe(originalUpdate); + }); + it("emits root release outputs compatible with release-please-action", () => { const outputs = buildReleaseOutputs([ { From 94d5cd7ddad481ceda3d10bb1ff694e7007f8158 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 15:49:28 +0530 Subject: [PATCH 11/17] fix(release): fail closed on ambiguous rewrites --- scripts/lib/release-branch-rewrite.mjs | 4 +- scripts/run-release-please.d.mts | 2 +- scripts/run-release-please.mjs | 84 ++++++++----- tests/scripts/run-release-please.test.ts | 151 +++++++++++++++++++++-- 4 files changed, 197 insertions(+), 44 deletions(-) diff --git a/scripts/lib/release-branch-rewrite.mjs b/scripts/lib/release-branch-rewrite.mjs index 28f2b931..7ba27e0d 100644 --- a/scripts/lib/release-branch-rewrite.mjs +++ b/scripts/lib/release-branch-rewrite.mjs @@ -16,8 +16,8 @@ // A record is written in two stages because the rewrite is not atomic with the recording of it. // The `before` head is written first, while it is still the branch head; the `after` head is added // once the rewrite has produced one. A record stopped in between names a discarded head and no -// replacement, which identifies nothing on its own -- so the gate ignores it, and the next run -// completes it from the branch head it finds, which is precisely the head that rewrite created. +// replacement. The gate ignores it until a later run can correlate exactly one subsequent +// force-push event; the event's `commit_id`, not whichever ref head exists later, supplies `after`. const MARKER_PATTERN = //iu; diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 40599a51..bb7fd4ed 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -22,7 +22,7 @@ interface ReleaseBranchScope { interface OpenedRewriteRecords extends ReleaseBranchScope { headsBeforeRegeneration: Map< string, - { head: string; recordId: number; pullRequestNumber: number } | string + { head: string; recordId: number; pullRequestNumber: number } >; } diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 1cfe902b..3d564124 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -73,13 +73,21 @@ export async function runReleasePlease(env = process.env) { targetBranch, headsBeforeRegeneration, }); - const pullRequests = recordsNameTheCurrentHeads - ? headsBeforeRegeneration.size > 0 - ? await withReleaseBranchRewriteCas(github, headsBeforeRegeneration, async () => - (await pullRequestManifest.createPullRequests()).filter(Boolean), - ) - : (await pullRequestManifest.createPullRequests()).filter(Boolean) - : []; + let pullRequests = []; + if (recordsNameTheCurrentHeads) { + try { + pullRequests = await withReleaseBranchRewriteCas(github, headsBeforeRegeneration, async () => + (await pullRequestManifest.createPullRequests()).filter(Boolean), + ); + } catch (error) { + // Releases may already exist, and their verified assets are uploaded by later workflow + // steps using the outputs below. A rejected regeneration leaves the branch unchanged and + // must not strand that release; its marker remains available for the next run to retry. + console.error( + `Could not regenerate release pull requests: ${error.message}. Existing release outputs remain available for asset publication.`, + ); + } + } await closeBranchRewriteRecords({ env, owner, repo, targetBranch, headsBeforeRegeneration }); outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { @@ -123,7 +131,7 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch (record) => record.marker.branch === branch && record.marker.after === null, ); if (open) { - const created = await readLatestForcePushedHead({ + const created = await readCorrelatedForcePushedHead({ env, owner, repo, @@ -191,7 +199,13 @@ export async function closeBranchRewriteRecords({ // `commit_id` -- so an ordinary commit landing on the branch before this read would have // the record name a head no event mentions, and the rewrite would stay unidentified // exactly as if nothing had recorded it. - const created = await readLatestForcePushedHead({ env, owner, repo, pullRequestNumber }); + const created = await readCorrelatedForcePushedHead({ + env, + owner, + repo, + pullRequestNumber, + after: record.createdAt, + }); if (!created) { // Leaving it open is the established answer to not knowing: the gate ignores an open // record, and the next run completes it. Closing it with an uncorroborated head would @@ -267,22 +281,30 @@ export async function withReleaseBranchRewriteCas(github, expected, operation) { if (typeof updates !== "function" || typeof github.graphql !== "function") { throw new Error("Release Please did not expose the GitHub clients needed for rewrite CAS."); } - const repository = await github.graphql( - "query ReleaseBranchRewriteRepository($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { id } }", - { owner: github.repository.owner, name: github.repository.repo }, - ); - const repositoryId = repository?.repository?.id; - if (typeof repositoryId !== "string" || !repositoryId) { - throw new Error("GitHub did not return a repository id for release branch rewrite CAS."); - } + let repositoryId = null; github.octokit.git.updateRef = async (request) => { const branch = String(request?.ref ?? "").replace(/^heads\//u, ""); + if (!branch.startsWith("release-please--branches--")) { + return updates.call(github.octokit.git, request); + } const record = expected.get(branch); - if (!record || typeof record === "string") return updates.call(github.octokit.git, request); + if (!record) { + throw new Error(`Release Please tried to rewrite unrecorded generated branch ${branch}.`); + } const afterOid = String(request?.sha ?? "").toLowerCase(); if (!/^[0-9a-f]{40}$/u.test(afterOid)) { throw new Error(`Release Please gave rewrite CAS an invalid destination for ${branch}.`); } + if (repositoryId === null) { + const repository = await github.graphql( + "query ReleaseBranchRewriteRepository($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { id } }", + { owner: github.repository.owner, name: github.repository.repo }, + ); + repositoryId = repository?.repository?.id; + if (typeof repositoryId !== "string" || !repositoryId) { + throw new Error("GitHub did not return a repository id for release branch rewrite CAS."); + } + } const result = await github.graphql( "mutation ReleaseBranchRewriteCas($repositoryId: ID!, $refUpdates: [RefUpdate!]!) { updateRefs(input: { repositoryId: $repositoryId, refUpdates: $refUpdates }) { clientMutationId } }", { @@ -316,22 +338,26 @@ export async function withReleaseBranchRewriteCas(github, expected, operation) { * field the review gate keys recorded discards by. Reading it here is what makes the two sides * agree about which rewrite a record describes. */ -async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber, after = 0 }) { +async function readCorrelatedForcePushedHead({ env, owner, repo, pullRequestNumber, after }) { const timeline = await githubList( env, `/repos/${owner}/${repo}/issues/${pullRequestNumber}/timeline`, "pull request timeline for a release branch", ); - let latest = null; + const candidates = []; for (const event of timeline) { if (event?.event !== "head_ref_force_pushed") continue; const sha = String(event.commit_id ?? "").toLowerCase(); if (!/^[0-9a-f]{40}$/u.test(sha)) continue; const at = Date.parse(event.created_at ?? ""); if (!Number.isFinite(at) || at < after) continue; - if (!latest || at > latest.at) latest = { sha, at }; + candidates.push({ sha, at }); + } + if (candidates.length === 0) return null; + if (candidates.length !== 1) { + throw new Error("GitHub returned multiple force-push events after an open rewrite record."); } - return latest?.sha ?? null; + return candidates[0].sha; } /** @@ -341,12 +367,6 @@ async function readLatestForcePushedHead({ env, owner, repo, pullRequestNumber, async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, expected }, visit) { const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); for (const [branch, expectedRecord] of expected) { - // String entries keep the exported helper compatible with older callers while production runs - // always retain the marker id returned when the record was opened. - const expectedInfo = - typeof expectedRecord === "string" - ? { head: expectedRecord, recordId: null, pullRequestNumber: null } - : expectedRecord; const head = heads.get(branch); if (!head) throw new Error(`Release branch ${branch} disappeared after its rewrite record was opened.`); @@ -357,11 +377,7 @@ async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, exp branch, targetBranch, }); - if ( - pullRequestNumber === null || - (expectedInfo.pullRequestNumber !== null && - pullRequestNumber !== expectedInfo.pullRequestNumber) - ) { + if (pullRequestNumber === null || pullRequestNumber !== expectedRecord.pullRequestNumber) { throw new Error( `Release pull request for ${branch} changed after its rewrite record was opened.`, ); @@ -369,7 +385,7 @@ async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, exp const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); const open = records.find( (record) => - (expectedInfo.recordId === null || record.id === expectedInfo.recordId) && + record.id === expectedRecord.recordId && record.marker.branch === branch && record.marker.after === null, ); diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 3a3b2cd5..aad73f6f 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -84,6 +84,25 @@ describe("Release Please workflow wrapper", () => { expect(github.octokit.git.updateRef).toBe(originalUpdate); }); + it("refuses an unrecorded generated-branch force update", async () => { + const branch = "release-please--branches--master--components--pack"; + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql: vi.fn(), + octokit: { git: { updateRef: originalUpdate } }, + }; + + await expect( + withReleaseBranchRewriteCas(github, new Map(), async () => + github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40), force: true }), + ), + ).rejects.toThrow(/unrecorded generated branch/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(github.graphql).not.toHaveBeenCalled(); + }); + it("emits root release outputs compatible with release-please-action", () => { const outputs = buildReleaseOutputs([ { @@ -214,12 +233,110 @@ describe("Release Please workflow wrapper", () => { getFileContentsOnBranch.mockRestore(); } }); + + it("keeps release outputs when generated-branch CAS rejects the regeneration", async () => { + const branch = "release-please--branches--master--components--pack"; + const before = "b".repeat(40); + const marker = ``; + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack", defaultBranch: "master" }, + graphql: vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockRejectedValueOnce(new Error("Reference update failed: beforeOid does not match")), + octokit: { git: { updateRef: originalUpdate } }, + }; + const createReleases = vi + .fn() + .mockResolvedValue([{ path: ".", tagName: "v0.1.1", version: "0.1.1" }]); + const createPullRequests = vi.fn(async () => + github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40), force: true }), + ); + const create = vi.spyOn(releasePlease.GitHub, "create").mockResolvedValue(github); + const fromManifest = vi + .spyOn(releasePlease.Manifest, "fromManifest") + .mockResolvedValueOnce({ createReleases }) + .mockResolvedValueOnce({ createPullRequests }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + let markerExists = false; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init: RequestInit = {}) => { + const path = new URL(String(url)).pathname; + if (path.includes("matching-refs")) { + return { + ok: true, + status: 200, + json: async () => [{ ref: `refs/heads/${branch}`, object: { sha: before } }], + } as unknown as Response; + } + if (path.includes("/pulls")) { + return { + ok: true, + status: 200, + json: async () => [{ number: 337 }], + } as unknown as Response; + } + if (path.includes("/comments") && String(init.method ?? "GET") === "POST") { + markerExists = true; + return { ok: true, status: 201, json: async () => ({ id: 99 }) } as unknown as Response; + } + if (path.includes("/comments") && String(init.method ?? "GET") === "PATCH") { + return { ok: true, status: 200, json: async () => ({ id: 99 }) } as unknown as Response; + } + if (path.includes("/comments")) { + return { + ok: true, + status: 200, + json: async () => + markerExists + ? [ + { + id: 99, + created_at: "2026-09-12T00:00:00Z", + user: { login: RECORDER }, + body: marker, + }, + ] + : [], + } as unknown as Response; + } + throw new Error(`Unexpected GitHub request: ${path}`); + }), + ); + + try { + const outputs = await runReleasePlease({ + GITHUB_REPOSITORY: "lamemustafa/pack", + GITHUB_TOKEN: "test-token", + GITHUB_API_URL: "https://api.github.test", + }); + + expect(createReleases).toHaveBeenCalledOnce(); + expect(createPullRequests).toHaveBeenCalledOnce(); + expect(outputs).toMatchObject({ release_created: "true", prs_created: "false" }); + expect(originalUpdate).not.toHaveBeenCalled(); + expect(error).toHaveBeenCalledWith( + expect.stringContaining("Existing release outputs remain"), + ); + } finally { + vi.unstubAllGlobals(); + error.mockRestore(); + fromManifest.mockRestore(); + create.mockRestore(); + } + }); }); describe("release branch rewrite records", () => { const branch = "release-please--branches--master--components--pack"; const env = { GITHUB_TOKEN: "t", GITHUB_API_URL: "https://api.github.test" }; + function openedRecords(head: string, recordId = 99) { + return new Map([[branch, { head, recordId, pullRequestNumber: 337 }]]); + } + function stubGitHub(handlers: { heads?: string | null; comments?: Array<{ id: number; body: string; user?: { login: string } }>; @@ -347,6 +464,26 @@ describe("release branch rewrite records", () => { expect(patched?.body).not.toContain(landedSince); }); + it("refuses interrupted recovery when more than one force-push could match its marker", async () => { + const before = "c".repeat(40); + stubGitHub({ + heads: before, + comments: [ + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, + ], + forcePushedHeads: [{ commit_id: "b".repeat(40) }, { commit_id: "d".repeat(40) }], + }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + openBranchRewriteRecords({ env, owner: "lamemustafa", repo: "pack", targetBranch: "master" }), + ).rejects.toThrow(/multiple force-push events/iu); + }); + it("ignores a marker written by anyone but the workflow", async () => { // The author is the whole of a marker's authority: anyone who can comment can write the text. // Treating a stranger's comment as an open record would rewrite that comment in place, or -- @@ -409,7 +546,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, snapshot]]), + headsBeforeRegeneration: openedRecords(snapshot), }); expect(proceeded).toBe(true); @@ -430,7 +567,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, "b".repeat(40)]]), + headsBeforeRegeneration: openedRecords("b".repeat(40)), }); expect(proceeded).toBe(false); @@ -510,7 +647,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, before]]), + headsBeforeRegeneration: openedRecords(before, 501), }); const patched = calls.find((call) => call.method === "PATCH"); @@ -539,7 +676,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, before]]), + headsBeforeRegeneration: openedRecords(before), }); const patched = calls.find((call) => call.method === "PATCH"); @@ -572,7 +709,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, before]]), + headsBeforeRegeneration: openedRecords(before), }); const patched = calls.find((call) => call.method === "PATCH"); @@ -603,7 +740,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, before]]), + headsBeforeRegeneration: openedRecords(before), }); expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); @@ -643,7 +780,7 @@ describe("release branch rewrite records", () => { owner: "lamemustafa", repo: "pack", targetBranch: "master", - headsBeforeRegeneration: new Map([[branch, "c".repeat(40)]]), + headsBeforeRegeneration: openedRecords("c".repeat(40)), }), ).resolves.toBeUndefined(); expect(errors).toHaveBeenCalledWith(expect.stringContaining("stays open")); From abb17361d3b0c588b82b80cfa12e9200b86956aa Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 15:52:44 +0530 Subject: [PATCH 12/17] fix(release): preserve original rewrite heads --- scripts/run-release-please.mjs | 19 +++++------ tests/scripts/run-release-please.test.ts | 43 +++++++++++++++++++----- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 3d564124..fa0f55b2 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -229,13 +229,13 @@ export async function closeBranchRewriteRecords({ } /** - * Brings every open record up to the head its branch actually carries, and reports whether it - * could. `false` means the regeneration must not proceed. + * Confirms every open record still names the head its branch carries. `false` means the + * regeneration must not proceed. * * The record is opened before `createReleases()` so that a failure there costs a re-run rather - * than a published release with no assets. That ordering leaves a gap: anything reaching the - * branch between then and the force-push is discarded while the record still names the older head, - * and the gate accepts that recorded pair and never searches the head that was lost. + * than a published release with no assets. A changed head is not refreshed: replacing `before` + * would erase the only durable name of the earlier head. The later compare-and-swap makes the + * remaining update atomic, so this path only admits an exact snapshot. * * Throwing is not available here -- the release already exists and a later workflow step uploads * its assets -- so the failure is reported instead and the caller skips the rewrite. Discarding a @@ -252,13 +252,10 @@ export async function refreshBranchRewriteRecords({ try { await eachOpenBranchRewriteRecord( { env, owner, repo, targetBranch, expected: headsBeforeRegeneration }, - async ({ pullRequestNumber, record, head }) => { + async ({ record, head }) => { if (head === record.marker.before) return; - await writeBranchRewriteRecord({ env, owner, repo, record, before: head }); - const expectedRecord = headsBeforeRegeneration.get(record.marker.branch); - if (expectedRecord && typeof expectedRecord !== "string") expectedRecord.head = head; - console.log( - `Refreshed release branch rewrite record on #${pullRequestNumber}: before=${head}`, + throw new Error( + `Release branch ${record.marker.branch} advanced after its rewrite record was opened; its original discarded head remains recorded and this regeneration will retry.`, ); }, ); diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index aad73f6f..09d82801 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -522,13 +522,9 @@ describe("release branch rewrite records", () => { ).rejects.toThrow(/malformed pull request list/iu); }); - it("records the head the branch carries at the rewrite, not at the snapshot", async () => { - // The record is opened before `createReleases()` so a failure there costs a re-run rather than - // a release with no assets. Anything reaching the branch in that gap would otherwise be - // discarded while the record still named the older head, and the gate would never search it. + it("refuses an ordinary advance after capture without replacing the recorded head", async () => { const snapshot = "b".repeat(40); const arrivedSince = "e".repeat(40); - // The branch already carries the newer head by the time the refresh reads it. const calls = stubGitHub({ heads: arrivedSince, comments: [ @@ -549,10 +545,39 @@ describe("release branch rewrite records", () => { headsBeforeRegeneration: openedRecords(snapshot), }); - expect(proceeded).toBe(true); - const patched = calls.find((call) => call.method === "PATCH"); - expect(patched?.body).toContain(`before=${arrivedSince}`); - expect(patched?.body).not.toContain("after="); + expect(proceeded).toBe(false); + expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); + expect(calls.find((call) => call.method === "POST")).toBeUndefined(); + }); + + it("refuses a force advance after capture without confusing it for an ordinary one", async () => { + const snapshot = "b".repeat(40); + const arrivedSince = "e".repeat(40); + const calls = stubGitHub({ + heads: arrivedSince, + comments: [ + { + id: 99, + user: { login: RECORDER }, + body: ``, + }, + ], + forcePushedHeads: [{ commit_id: arrivedSince }], + }); + const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + refreshBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: openedRecords(snapshot), + }), + ).resolves.toBe(false); + + expect(calls.find((call) => call.path.includes("/timeline"))).toBeUndefined(); + expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); }); it("refuses to regenerate when it cannot confirm what is about to be discarded", async () => { From c6aaeff39012874d6abc2bdac3ef69432d597ee7 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 16:07:30 +0530 Subject: [PATCH 13/17] fix(release): close rewrites from CAS receipts --- scripts/lib/release-branch-rewrite.mjs | 4 +- scripts/run-release-please.d.mts | 14 ++- scripts/run-release-please.mjs | 125 ++++++----------------- tests/scripts/run-release-please.test.ts | 65 +++++------- 4 files changed, 72 insertions(+), 136 deletions(-) diff --git a/scripts/lib/release-branch-rewrite.mjs b/scripts/lib/release-branch-rewrite.mjs index 7ba27e0d..279e7195 100644 --- a/scripts/lib/release-branch-rewrite.mjs +++ b/scripts/lib/release-branch-rewrite.mjs @@ -16,8 +16,8 @@ // A record is written in two stages because the rewrite is not atomic with the recording of it. // The `before` head is written first, while it is still the branch head; the `after` head is added // once the rewrite has produced one. A record stopped in between names a discarded head and no -// replacement. The gate ignores it until a later run can correlate exactly one subsequent -// force-push event; the event's `commit_id`, not whichever ref head exists later, supplies `after`. +// replacement. The gate ignores it until a successful compare-and-swap records the created head; +// timeline events alone cannot safely attribute a later head to this marker. const MARKER_PATTERN = //iu; diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index bb7fd4ed..6ca26529 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -33,7 +33,15 @@ export function openBranchRewriteRecords( /** `false` when it could not confirm what the regeneration is about to discard. */ export function refreshBranchRewriteRecords(options: OpenedRewriteRecords): Promise; -export function closeBranchRewriteRecords(options: OpenedRewriteRecords): Promise; +export function closeBranchRewriteRecords(options: { + env: ReleaseBranchScope["env"]; + owner: string; + repo: string; + confirmedRewrites: Map< + string, + { record: { id: number; marker: { branch: string; before: string } }; after: string } + >; +}): Promise; export function withReleaseBranchRewriteCas( github: { @@ -42,5 +50,9 @@ export function withReleaseBranchRewriteCas( octokit: { git: { updateRef: (request: Record) => Promise } }; }, expected: OpenedRewriteRecords["headsBeforeRegeneration"], + confirmedRewrites: Map< + string, + { record: { id: number; marker: { branch: string; before: string } }; after: string } + >, operation: () => Promise, ): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index fa0f55b2..fc0d9659 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -74,10 +74,14 @@ export async function runReleasePlease(env = process.env) { headsBeforeRegeneration, }); let pullRequests = []; + const confirmedRewrites = new Map(); if (recordsNameTheCurrentHeads) { try { - pullRequests = await withReleaseBranchRewriteCas(github, headsBeforeRegeneration, async () => - (await pullRequestManifest.createPullRequests()).filter(Boolean), + pullRequests = await withReleaseBranchRewriteCas( + github, + headsBeforeRegeneration, + confirmedRewrites, + async () => (await pullRequestManifest.createPullRequests()).filter(Boolean), ); } catch (error) { // Releases may already exist, and their verified assets are uploaded by later workflow @@ -88,7 +92,7 @@ export async function runReleasePlease(env = process.env) { ); } } - await closeBranchRewriteRecords({ env, owner, repo, targetBranch, headsBeforeRegeneration }); + await closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }); outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { outputs.pr = JSON.stringify(pullRequests[0]); @@ -112,8 +116,8 @@ export async function runReleasePlease(env = process.env) { // across a regeneration instead of refusing every release pull request (#342, #350). // // Opening the record before the rewrite rather than writing it afterwards is what makes an -// interrupted run recoverable: a cancellation between the force-push and the write would otherwise -// lose the discarded SHA permanently, and no later run could reconstruct it. +// interrupted run diagnosable: a cancellation leaves the discarded SHA recorded, but a later run +// must hold if it cannot prove the rewrite's created head from its own CAS receipt. export async function openBranchRewriteRecords({ env, owner, repo, targetBranch }) { const heads = await readReleaseBranchHeads({ env, owner, repo, targetBranch }); const opened = new Map(); @@ -131,23 +135,13 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch (record) => record.marker.branch === branch && record.marker.after === null, ); if (open) { - const created = await readCorrelatedForcePushedHead({ - env, - owner, - repo, - pullRequestNumber, - after: open.createdAt, - }); - if (created) { - await writeBranchRewriteRecord({ env, owner, repo, record: open, after: created }); - } else if (head === open.marker.before) { + if (head === open.marker.before) { opened.set(branch, { head, recordId: open.id, pullRequestNumber }); continue; - } else { - throw new Error( - "GitHub could not correlate an open rewrite record with the branch head now standing; refusing to discard an unrecorded head.", - ); } + throw new Error( + "An interrupted rewrite record no longer matches its branch head; regeneration remains held.", + ); } const created = await githubRequest( env, @@ -174,56 +168,20 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch // failing the job would strand it without its assets. The cost of not throwing is bounded: the // record stays open, the review gate keeps refusing exactly as it would have, and the next run // completes it. -export async function closeBranchRewriteRecords({ - env, - owner, - repo, - targetBranch, - headsBeforeRegeneration, -}) { +export async function closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }) { try { - await eachOpenBranchRewriteRecord( - { env, owner, repo, targetBranch, expected: headsBeforeRegeneration }, - async ({ pullRequestNumber, record, head }) => { - // An unchanged head discarded nothing. Completing the record with `after === before` says - // exactly that, and the gate reads it as the non-event it was. - if (head === record.marker.before) { - await writeBranchRewriteRecord({ env, owner, repo, record, after: head }); - console.log( - `Closed release branch rewrite record on #${pullRequestNumber}: branch unchanged.`, - ); - return; - } - // Otherwise the head is read from the rewrite itself, not from the branch. The gate looks - // this record up by the head the force-push *created* -- it keys on the timeline event's - // `commit_id` -- so an ordinary commit landing on the branch before this read would have - // the record name a head no event mentions, and the rewrite would stay unidentified - // exactly as if nothing had recorded it. - const created = await readCorrelatedForcePushedHead({ - env, - owner, - repo, - pullRequestNumber, - after: record.createdAt, - }); - if (!created) { - // Leaving it open is the established answer to not knowing: the gate ignores an open - // record, and the next run completes it. Closing it with an uncorroborated head would - // publish a claim about a rewrite that no event backs. - console.error( - `Could not name the head the regeneration created on #${pullRequestNumber}. The record stays open and the next run completes it.`, - ); - return; - } - await writeBranchRewriteRecord({ env, owner, repo, record, after: created }); - console.log( - `Closed release branch rewrite record on #${pullRequestNumber}: ${record.marker.before} -> ${created}`, - ); - }, - ); + for (const receipt of confirmedRewrites.values()) { + await writeBranchRewriteRecord({ + env, + owner, + repo, + record: receipt.record, + after: receipt.after, + }); + } } catch (error) { console.error( - `Could not close a release branch rewrite record: ${error.message}. The record stays open and the next run completes it.`, + `Could not close a release branch rewrite record: ${error.message}. The record stays open for review.`, ); } } @@ -273,7 +231,7 @@ export async function refreshBranchRewriteRecords({ // even though the marker names the earlier head. GitHub's `updateRefs` mutation accepts `beforeOid`, // so replace only those already-recorded generated-branch updates with a compare-and-swap. A // mismatch rejects the release run while its open marker remains durable for recovery. -export async function withReleaseBranchRewriteCas(github, expected, operation) { +export async function withReleaseBranchRewriteCas(github, expected, confirmedRewrites, operation) { const updates = github.octokit?.git?.updateRef; if (typeof updates !== "function" || typeof github.graphql !== "function") { throw new Error("Release Please did not expose the GitHub clients needed for rewrite CAS."); @@ -319,6 +277,10 @@ export async function withReleaseBranchRewriteCas(github, expected, operation) { if (!result?.updateRefs) { throw new Error(`GitHub did not confirm the compare-and-swap update for ${branch}.`); } + confirmedRewrites.set(branch, { + record: { id: record.recordId, marker: { branch, before: record.head } }, + after: afterOid, + }); return { data: { object: { sha: afterOid } } }; }; try { @@ -328,35 +290,6 @@ export async function withReleaseBranchRewriteCas(github, expected, operation) { } } -/** - * The head named by this pull request's most recent force-push, or `null`. - * - * `commit_id` on a `head_ref_force_pushed` event is the head that rewrite created -- the same - * field the review gate keys recorded discards by. Reading it here is what makes the two sides - * agree about which rewrite a record describes. - */ -async function readCorrelatedForcePushedHead({ env, owner, repo, pullRequestNumber, after }) { - const timeline = await githubList( - env, - `/repos/${owner}/${repo}/issues/${pullRequestNumber}/timeline`, - "pull request timeline for a release branch", - ); - const candidates = []; - for (const event of timeline) { - if (event?.event !== "head_ref_force_pushed") continue; - const sha = String(event.commit_id ?? "").toLowerCase(); - if (!/^[0-9a-f]{40}$/u.test(sha)) continue; - const at = Date.parse(event.created_at ?? ""); - if (!Number.isFinite(at) || at < after) continue; - candidates.push({ sha, at }); - } - if (candidates.length === 0) return null; - if (candidates.length !== 1) { - throw new Error("GitHub returned multiple force-push events after an open rewrite record."); - } - return candidates[0].sha; -} - /** * The walk the refresh and the close share: each branch's open record, paired with the head its * branch carries right now. They differ only in what they write to it. diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 09d82801..9a145e2a 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -36,6 +36,7 @@ describe("Release Please workflow wrapper", () => { await withReleaseBranchRewriteCas( github, new Map([[branch, { head: before, recordId: 1, pullRequestNumber: 2 }]]), + new Map(), async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }), ); @@ -74,7 +75,7 @@ describe("Release Please workflow wrapper", () => { const records = new Map([[branch, { head: before, recordId: 1, pullRequestNumber: 2 }]]); await expect( - withReleaseBranchRewriteCas(github, records, async () => + withReleaseBranchRewriteCas(github, records, new Map(), async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }), ), ).rejects.toThrow(/beforeOid does not match/iu); @@ -94,7 +95,7 @@ describe("Release Please workflow wrapper", () => { }; await expect( - withReleaseBranchRewriteCas(github, new Map(), async () => + withReleaseBranchRewriteCas(github, new Map(), new Map(), async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40), force: true }), ), ).rejects.toThrow(/unrecorded generated branch/iu); @@ -224,7 +225,7 @@ describe("Release Please workflow wrapper", () => { // is about to discard. Nothing is written, because this run rewrote no branch. const headRead = "GET /repos/lamemustafa/pack/git/matching-refs/heads/release-please--branches--master--"; - expect(fetched).toEqual([headRead, headRead, headRead]); + expect(fetched).toEqual([headRead, headRead]); } finally { vi.unstubAllGlobals(); log.mockRestore(); @@ -434,7 +435,7 @@ describe("release branch rewrite records", () => { expect(posted?.body).not.toContain("after="); }); - it("completes an interrupted record from its force-push event, not the current ref", async () => { + it("holds an interrupted record when its branch changed", async () => { const lostBefore = "c".repeat(40); const created = "b".repeat(40); const landedSince = "e".repeat(40); @@ -451,22 +452,15 @@ describe("release branch rewrite records", () => { }); const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - await openBranchRewriteRecords({ - env, - owner: "lamemustafa", - repo: "pack", - targetBranch: "master", - }); - - const patched = calls.find((call) => call.method === "PATCH"); - expect(patched?.path).toContain("/issues/comments/99"); - expect(patched?.body).toContain(`before=${lostBefore} after=${created}`); - expect(patched?.body).not.toContain(landedSince); + await expect( + openBranchRewriteRecords({ env, owner: "lamemustafa", repo: "pack", targetBranch: "master" }), + ).rejects.toThrow(/interrupted rewrite record no longer matches/iu); + expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); }); - it("refuses interrupted recovery when more than one force-push could match its marker", async () => { + it("does not consult timeline events for an unchanged interrupted record", async () => { const before = "c".repeat(40); - stubGitHub({ + const calls = stubGitHub({ heads: before, comments: [ { @@ -481,7 +475,8 @@ describe("release branch rewrite records", () => { await expect( openBranchRewriteRecords({ env, owner: "lamemustafa", repo: "pack", targetBranch: "master" }), - ).rejects.toThrow(/multiple force-push events/iu); + ).resolves.toBeInstanceOf(Map); + expect(calls.find((call) => call.path.includes("/timeline"))).toBeUndefined(); }); it("ignores a marker written by anyone but the workflow", async () => { @@ -653,7 +648,7 @@ describe("release branch rewrite records", () => { // A release pull request outlives a hundred comments, and the marker this mechanism just wrote is // the newest one -- exactly what a first-page read drops. Losing it leaves the force-push // permanently unpaired, because every later run reads the same truncated page. - it("reads a rewrite record past the first page of comments", async () => { + it("closes only the marker named by a verified CAS receipt", async () => { const before = "c".repeat(40); const after = "b".repeat(40); const marker = ``; @@ -671,8 +666,9 @@ describe("release branch rewrite records", () => { env, owner: "lamemustafa", repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: openedRecords(before, 501), + confirmedRewrites: new Map([ + [branch, { record: { id: 501, marker: { branch, before } }, after }], + ]), }); const patched = calls.find((call) => call.method === "PATCH"); @@ -680,7 +676,7 @@ describe("release branch rewrite records", () => { expect(patched?.body).toContain(`after=${after}`); }); - it("closes the open record with the head the rewrite created", async () => { + it("writes the verified CAS destination into the marker", async () => { const before = "c".repeat(40); const after = "b".repeat(40); const calls = stubGitHub({ @@ -700,15 +696,16 @@ describe("release branch rewrite records", () => { env, owner: "lamemustafa", repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: openedRecords(before), + confirmedRewrites: new Map([ + [branch, { record: { id: 99, marker: { branch, before } }, after }], + ]), }); const patched = calls.find((call) => call.method === "PATCH"); expect(patched?.body).toContain(`before=${before} after=${after}`); }); - it("closes with the head the rewrite created, not the one the branch carries now", async () => { + it("does not replace a CAS receipt with a later branch head", async () => { // The gate looks a record up by the head the force-push created -- the timeline event's // `commit_id`. An ordinary commit landing on the branch before this read would otherwise have // the record name a head no event mentions, leaving the rewrite as unidentified as if nothing @@ -733,8 +730,9 @@ describe("release branch rewrite records", () => { env, owner: "lamemustafa", repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: openedRecords(before), + confirmedRewrites: new Map([ + [branch, { record: { id: 99, marker: { branch, before } }, after: created }], + ]), }); const patched = calls.find((call) => call.method === "PATCH"); @@ -764,8 +762,7 @@ describe("release branch rewrite records", () => { env, owner: "lamemustafa", repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: openedRecords(before), + confirmedRewrites: new Map(), }); expect(calls.find((call) => call.method === "PATCH")).toBeUndefined(); @@ -789,26 +786,20 @@ describe("release branch rewrite records", () => { expect(lookup?.path).toContain("base=master"); }); - it("leaves the record open rather than failing a run that already published a release", async () => { + it("does not close a marker without a CAS receipt", async () => { vi.stubGlobal( "fetch", vi.fn(async () => ({ ok: false, status: 500 }) as unknown as Response), ); - const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); const { closeBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - // Throwing here would abort the workflow after a GitHub release exists, stranding it without - // its assets. The open record costs a refusal the gate was already making. await expect( closeBranchRewriteRecords({ env, owner: "lamemustafa", repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: openedRecords("c".repeat(40)), + confirmedRewrites: new Map(), }), ).resolves.toBeUndefined(); - expect(errors).toHaveBeenCalledWith(expect.stringContaining("stays open")); - errors.mockRestore(); }); }); From fa6d5640a0160687da703a6a1f721c18159fb89e Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 16:19:19 +0530 Subject: [PATCH 14/17] fix(release): guard first generated branch update --- scripts/run-release-please.d.mts | 7 +- scripts/run-release-please.mjs | 42 ++++++-- tests/scripts/run-release-please.test.ts | 124 ++++++++++++++++++++++- 3 files changed, 159 insertions(+), 14 deletions(-) diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 6ca26529..511da5fe 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -47,7 +47,12 @@ export function withReleaseBranchRewriteCas( github: { repository: { owner: string; repo: string }; graphql: (query: string, variables: Record) => Promise; - octokit: { git: { updateRef: (request: Record) => Promise } }; + octokit: { + git: { + createRef: (request: Record) => Promise; + updateRef: (request: Record) => Promise; + }; + }; }, expected: OpenedRewriteRecords["headsBeforeRegeneration"], confirmedRewrites: Map< diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index fc0d9659..feba89ee 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -229,21 +229,42 @@ export async function refreshBranchRewriteRecords({ // Release Please delegates generated-branch updates to an unconditional REST force-push. That // leaves a race after the marker is refreshed: a commit that arrives before the push is discarded // even though the marker names the earlier head. GitHub's `updateRefs` mutation accepts `beforeOid`, -// so replace only those already-recorded generated-branch updates with a compare-and-swap. A -// mismatch rejects the release run while its open marker remains durable for recovery. +// so replace recorded rewrites and the first update after a confirmed generated-branch creation +// with a compare-and-swap. A mismatch rejects the release run while an open marker remains +// durable for recovery. export async function withReleaseBranchRewriteCas(github, expected, confirmedRewrites, operation) { const updates = github.octokit?.git?.updateRef; - if (typeof updates !== "function" || typeof github.graphql !== "function") { + const creates = github.octokit?.git?.createRef; + if ( + typeof updates !== "function" || + typeof creates !== "function" || + typeof github.graphql !== "function" + ) { throw new Error("Release Please did not expose the GitHub clients needed for rewrite CAS."); } let repositoryId = null; + const createdBranches = new Map(); + github.octokit.git.createRef = async (request) => { + const branch = String(request?.ref ?? "").replace(/^refs\/heads\//u, ""); + if (!branch.startsWith("release-please--branches--")) { + return creates.call(github.octokit.git, request); + } + const result = await creates.call(github.octokit.git, request); + const head = String(result?.data?.object?.sha ?? "").toLowerCase(); + if (!/^[0-9a-f]{40}$/u.test(head)) { + throw new Error(`GitHub did not confirm the initial head for generated branch ${branch}.`); + } + createdBranches.set(branch, head); + return result; + }; github.octokit.git.updateRef = async (request) => { const branch = String(request?.ref ?? "").replace(/^heads\//u, ""); if (!branch.startsWith("release-please--branches--")) { return updates.call(github.octokit.git, request); } const record = expected.get(branch); - if (!record) { + const createdHead = createdBranches.get(branch); + if (!record && !createdHead) { throw new Error(`Release Please tried to rewrite unrecorded generated branch ${branch}.`); } const afterOid = String(request?.sha ?? "").toLowerCase(); @@ -267,7 +288,7 @@ export async function withReleaseBranchRewriteCas(github, expected, confirmedRew refUpdates: [ { name: `refs/heads/${branch}`, - beforeOid: record.head, + beforeOid: record?.head ?? createdHead, afterOid, force: true, }, @@ -277,16 +298,19 @@ export async function withReleaseBranchRewriteCas(github, expected, confirmedRew if (!result?.updateRefs) { throw new Error(`GitHub did not confirm the compare-and-swap update for ${branch}.`); } - confirmedRewrites.set(branch, { - record: { id: record.recordId, marker: { branch, before: record.head } }, - after: afterOid, - }); + if (record) { + confirmedRewrites.set(branch, { + record: { id: record.recordId, marker: { branch, before: record.head } }, + after: afterOid, + }); + } return { data: { object: { sha: afterOid } } }; }; try { return await operation(); } finally { github.octokit.git.updateRef = updates; + github.octokit.git.createRef = creates; } } diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index 9a145e2a..b44441a3 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -22,6 +22,7 @@ describe("Release Please workflow wrapper", () => { const branch = "release-please--branches--master--components--pack"; const before = "b".repeat(40); const after = "c".repeat(40); + const originalCreate = vi.fn(); const originalUpdate = vi.fn().mockResolvedValue({ data: { object: { sha: after } } }); const graphql = vi .fn() @@ -30,7 +31,7 @@ describe("Release Please workflow wrapper", () => { const github = { repository: { owner: "lamemustafa", repo: "pack" }, graphql, - octokit: { git: { updateRef: originalUpdate } }, + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, }; await withReleaseBranchRewriteCas( @@ -56,12 +57,14 @@ describe("Release Please workflow wrapper", () => { }), ); expect(github.octokit.git.updateRef).toBe(originalUpdate); + expect(github.octokit.git.createRef).toBe(originalCreate); }); it("rejects a raced generated-branch update and leaves the caller's record intact", async () => { const branch = "release-please--branches--master--components--pack"; const before = "b".repeat(40); const after = "c".repeat(40); + const originalCreate = vi.fn(); const originalUpdate = vi.fn(); const graphql = vi .fn() @@ -70,7 +73,7 @@ describe("Release Please workflow wrapper", () => { const github = { repository: { owner: "lamemustafa", repo: "pack" }, graphql, - octokit: { git: { updateRef: originalUpdate } }, + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, }; const records = new Map([[branch, { head: before, recordId: 1, pullRequestNumber: 2 }]]); @@ -83,15 +86,17 @@ describe("Release Please workflow wrapper", () => { expect(originalUpdate).not.toHaveBeenCalled(); expect(records.get(branch)).toMatchObject({ head: before, recordId: 1 }); expect(github.octokit.git.updateRef).toBe(originalUpdate); + expect(github.octokit.git.createRef).toBe(originalCreate); }); it("refuses an unrecorded generated-branch force update", async () => { const branch = "release-please--branches--master--components--pack"; + const originalCreate = vi.fn(); const originalUpdate = vi.fn(); const github = { repository: { owner: "lamemustafa", repo: "pack" }, graphql: vi.fn(), - octokit: { git: { updateRef: originalUpdate } }, + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, }; await expect( @@ -102,6 +107,116 @@ describe("Release Please workflow wrapper", () => { expect(originalUpdate).not.toHaveBeenCalled(); expect(github.graphql).not.toHaveBeenCalled(); + expect(github.octokit.git.createRef).toBe(originalCreate); + }); + + it("uses CAS for the first update after creating a generated branch", async () => { + const branch = "release-please--branches--master--components--pack"; + const initial = "b".repeat(40); + const after = "c".repeat(40); + const originalCreate = vi.fn().mockResolvedValue({ data: { object: { sha: initial } } }); + const originalUpdate = vi.fn(); + const graphql = vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockResolvedValueOnce({ updateRefs: { clientMutationId: null } }); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql, + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + + await withReleaseBranchRewriteCas(github, new Map(), new Map(), async () => { + await github.octokit.git.createRef({ ref: `refs/heads/${branch}`, sha: initial }); + await github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }); + }); + + expect(originalCreate).toHaveBeenCalledOnce(); + expect(originalUpdate).not.toHaveBeenCalled(); + expect(graphql).toHaveBeenLastCalledWith( + expect.stringContaining("updateRefs"), + expect.objectContaining({ + refUpdates: [expect.objectContaining({ beforeOid: initial, afterOid: after, force: true })], + }), + ); + expect(github.octokit.git.createRef).toBe(originalCreate); + expect(github.octokit.git.updateRef).toBe(originalUpdate); + }); + + it("rejects a raced first generated-branch update after creation", async () => { + const branch = "release-please--branches--master--components--pack"; + const initial = "b".repeat(40); + const originalCreate = vi.fn().mockResolvedValue({ data: { object: { sha: initial } } }); + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql: vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockRejectedValueOnce(new Error("Reference update failed: beforeOid does not match")), + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + + await expect( + withReleaseBranchRewriteCas(github, new Map(), new Map(), async () => { + await github.octokit.git.createRef({ ref: `refs/heads/${branch}`, sha: initial }); + return github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40) }); + }), + ).rejects.toThrow(/beforeOid does not match/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(github.octokit.git.createRef).toBe(originalCreate); + expect(github.octokit.git.updateRef).toBe(originalUpdate); + }); + + it("does not authorize an update when generated-branch creation is unconfirmed", async () => { + const branch = "release-please--branches--master--components--pack"; + const originalCreate = vi.fn().mockResolvedValue({ data: { object: { sha: "unknown" } } }); + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql: vi.fn(), + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + + await expect( + withReleaseBranchRewriteCas(github, new Map(), new Map(), async () => { + await expect( + github.octokit.git.createRef({ ref: `refs/heads/${branch}`, sha: "b".repeat(40) }), + ).rejects.toThrow(/did not confirm the initial head/iu); + return github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40) }); + }), + ).rejects.toThrow(/unrecorded generated branch/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(github.graphql).not.toHaveBeenCalled(); + expect(github.octokit.git.createRef).toBe(originalCreate); + expect(github.octokit.git.updateRef).toBe(originalUpdate); + }); + + it("does not authorize an update when generated-branch creation is refused", async () => { + const branch = "release-please--branches--master--components--pack"; + const originalCreate = vi.fn().mockRejectedValue(new Error("reference already exists")); + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql: vi.fn(), + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + + await expect( + withReleaseBranchRewriteCas(github, new Map(), new Map(), async () => { + await expect( + github.octokit.git.createRef({ ref: `refs/heads/${branch}`, sha: "b".repeat(40) }), + ).rejects.toThrow(/reference already exists/iu); + return github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40) }); + }), + ).rejects.toThrow(/unrecorded generated branch/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(github.graphql).not.toHaveBeenCalled(); + expect(github.octokit.git.createRef).toBe(originalCreate); + expect(github.octokit.git.updateRef).toBe(originalUpdate); }); it("emits root release outputs compatible with release-please-action", () => { @@ -239,6 +354,7 @@ describe("Release Please workflow wrapper", () => { const branch = "release-please--branches--master--components--pack"; const before = "b".repeat(40); const marker = ``; + const originalCreate = vi.fn(); const originalUpdate = vi.fn(); const github = { repository: { owner: "lamemustafa", repo: "pack", defaultBranch: "master" }, @@ -246,7 +362,7 @@ describe("Release Please workflow wrapper", () => { .fn() .mockResolvedValueOnce({ repository: { id: "repo-id" } }) .mockRejectedValueOnce(new Error("Reference update failed: beforeOid does not match")), - octokit: { git: { updateRef: originalUpdate } }, + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, }; const createReleases = vi .fn() From a07395a2d8c514648c42cb94df90e4c3d9f364f1 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 16:37:34 +0530 Subject: [PATCH 15/17] fix(release): fail no-release regeneration errors --- scripts/run-release-please.mjs | 9 ++++- tests/scripts/run-release-please.test.ts | 51 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index feba89ee..3a7965d3 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -75,7 +75,13 @@ export async function runReleasePlease(env = process.env) { }); let pullRequests = []; const confirmedRewrites = new Map(); - if (recordsNameTheCurrentHeads) { + if (!recordsNameTheCurrentHeads) { + const error = new Error("Could not confirm release branch rewrite records for regeneration."); + if (releases.length === 0) throw error; + console.error( + `${error.message} Existing release outputs remain available for asset publication.`, + ); + } else { try { pullRequests = await withReleaseBranchRewriteCas( github, @@ -87,6 +93,7 @@ export async function runReleasePlease(env = process.env) { // Releases may already exist, and their verified assets are uploaded by later workflow // steps using the outputs below. A rejected regeneration leaves the branch unchanged and // must not strand that release; its marker remains available for the next run to retry. + if (releases.length === 0) throw error; console.error( `Could not regenerate release pull requests: ${error.message}. Existing release outputs remain available for asset publication.`, ); diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index b44441a3..b1ba3373 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -350,6 +350,57 @@ describe("Release Please workflow wrapper", () => { } }); + it("propagates regeneration failure when no release was created", async () => { + const configContents = await readFile( + new URL("../../release-please-config.json", import.meta.url), + "utf8", + ); + const manifestContents = await readFile( + new URL("../../.release-please-manifest.json", import.meta.url), + "utf8", + ); + const createReleases = vi.fn().mockResolvedValue([]); + const createPullRequests = vi.fn().mockRejectedValue(new Error("regeneration failed")); + const getFileContentsOnBranch = vi + .spyOn(releasePlease.GitHub.prototype, "getFileContentsOnBranch") + .mockImplementation(async (...args: unknown[]) => { + const [path] = args; + if (path === "release-please-config.json") return { parsedContent: configContents }; + if (path === ".release-please-manifest.json") return { parsedContent: manifestContents }; + throw new Error(`Unexpected release-please file request: ${String(path)}`); + }); + const releaseManifest = vi + .spyOn(releasePlease.Manifest.prototype, "createReleases") + .mockImplementation(createReleases); + const pullRequestManifest = vi + .spyOn(releasePlease.Manifest.prototype, "createPullRequests") + .mockImplementation(createPullRequests); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: true, status: 200, json: async () => [] }) as unknown as Response), + ); + + try { + await expect( + runReleasePlease({ + GITHUB_REPOSITORY: "lamemustafa/pack", + RELEASE_PLEASE_TOKEN: "test-token", + RELEASE_PLEASE_TARGET_BRANCH: "master", + }), + ).rejects.toThrow(/regeneration failed/iu); + + expect(createReleases).toHaveBeenCalledOnce(); + expect(createPullRequests).toHaveBeenCalledOnce(); + } finally { + vi.unstubAllGlobals(); + log.mockRestore(); + pullRequestManifest.mockRestore(); + releaseManifest.mockRestore(); + getFileContentsOnBranch.mockRestore(); + } + }); + it("keeps release outputs when generated-branch CAS rejects the regeneration", async () => { const branch = "release-please--branches--master--components--pack"; const before = "b".repeat(40); From 2ec72e72cc2df1ae749c90ad7402ff76408be731 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 16:55:04 +0530 Subject: [PATCH 16/17] fix(release): close confirmed rewrite receipts --- scripts/run-release-please.d.mts | 6 +- scripts/run-release-please.mjs | 68 +++++--- tests/scripts/run-release-please.test.ts | 204 +++++++++++++++++++++++ 3 files changed, 253 insertions(+), 25 deletions(-) diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 511da5fe..9bf81189 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -22,13 +22,15 @@ interface ReleaseBranchScope { interface OpenedRewriteRecords extends ReleaseBranchScope { headsBeforeRegeneration: Map< string, - { head: string; recordId: number; pullRequestNumber: number } + { head: string; recordId: number | null; pullRequestNumber: number | null } >; } export function openBranchRewriteRecords( options: ReleaseBranchScope, -): Promise>; +): Promise< + Map +>; /** `false` when it could not confirm what the regeneration is about to discard. */ export function refreshBranchRewriteRecords(options: OpenedRewriteRecords): Promise; diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index 3a7965d3..a4d5313d 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -75,31 +75,27 @@ export async function runReleasePlease(env = process.env) { }); let pullRequests = []; const confirmedRewrites = new Map(); - if (!recordsNameTheCurrentHeads) { - const error = new Error("Could not confirm release branch rewrite records for regeneration."); + try { + if (!recordsNameTheCurrentHeads) { + throw new Error("Could not confirm release branch rewrite records for regeneration."); + } + pullRequests = await withReleaseBranchRewriteCas( + github, + headsBeforeRegeneration, + confirmedRewrites, + async () => (await pullRequestManifest.createPullRequests()).filter(Boolean), + ); + } catch (error) { + // A release already exists only when its outputs can carry the later asset publication steps. if (releases.length === 0) throw error; console.error( - `${error.message} Existing release outputs remain available for asset publication.`, + `Could not regenerate release pull requests: ${error.message}. Existing release outputs remain available for asset publication.`, ); - } else { - try { - pullRequests = await withReleaseBranchRewriteCas( - github, - headsBeforeRegeneration, - confirmedRewrites, - async () => (await pullRequestManifest.createPullRequests()).filter(Boolean), - ); - } catch (error) { - // Releases may already exist, and their verified assets are uploaded by later workflow - // steps using the outputs below. A rejected regeneration leaves the branch unchanged and - // must not strand that release; its marker remains available for the next run to retry. - if (releases.length === 0) throw error; - console.error( - `Could not regenerate release pull requests: ${error.message}. Existing release outputs remain available for asset publication.`, - ); - } + } finally { + // A successful CAS can precede another release-please error. Its receipt must be closed before + // that error propagates, so the review gate never sees a completed rewrite as interrupted. + await closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }); } - await closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }); outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { outputs.pr = JSON.stringify(pullRequests[0]); @@ -136,7 +132,13 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch branch, targetBranch, }); - if (pullRequestNumber === null) continue; + if (pullRequestNumber === null) { + // A previous release pull request can close while its generated branch remains. This exact + // head is a same-run CAS precondition only; it is not continuity evidence and has no marker + // to close. + opened.set(branch, { head, recordId: null, pullRequestNumber: null }); + continue; + } const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); const open = records.find( (record) => record.marker.branch === branch && record.marker.after === null, @@ -305,7 +307,7 @@ export async function withReleaseBranchRewriteCas(github, expected, confirmedRew if (!result?.updateRefs) { throw new Error(`GitHub did not confirm the compare-and-swap update for ${branch}.`); } - if (record) { + if (record && record.recordId !== null) { confirmedRewrites.set(branch, { record: { id: record.recordId, marker: { branch, before: record.head } }, after: afterOid, @@ -331,6 +333,26 @@ async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, exp const head = heads.get(branch); if (!head) throw new Error(`Release branch ${branch} disappeared after its rewrite record was opened.`); + if (expectedRecord.recordId === null) { + if (head !== expectedRecord.head) { + throw new Error( + `Retained release branch ${branch} advanced after its exact head was observed; regeneration remains held.`, + ); + } + const pullRequestNumber = await findOpenPullRequestNumber({ + env, + owner, + repo, + branch, + targetBranch, + }); + if (pullRequestNumber !== null) { + throw new Error( + `Retained release branch ${branch} gained a release pull request after observation; regeneration remains held.`, + ); + } + continue; + } const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index b1ba3373..aefeeaf7 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -110,6 +110,61 @@ describe("Release Please workflow wrapper", () => { expect(github.octokit.git.createRef).toBe(originalCreate); }); + it("uses an exact retained-branch snapshot for a generated-branch CAS", async () => { + const branch = "release-please--branches--master--components--pack"; + const before = "b".repeat(40); + const after = "c".repeat(40); + const originalCreate = vi.fn(); + const originalUpdate = vi.fn(); + const graphql = vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockResolvedValueOnce({ updateRefs: { clientMutationId: null } }); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql, + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + const confirmed = new Map(); + + await withReleaseBranchRewriteCas( + github, + new Map([[branch, { head: before, recordId: null, pullRequestNumber: null }]]), + confirmed, + async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }), + ); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(graphql).toHaveBeenLastCalledWith( + expect.stringContaining("updateRefs"), + expect.objectContaining({ refUpdates: [expect.objectContaining({ beforeOid: before })] }), + ); + expect(confirmed).toEqual(new Map()); + }); + + it("does not let one retained-branch snapshot authorize another generated branch", async () => { + const branch = "release-please--branches--master--components--pack"; + const otherBranch = "release-please--branches--master--components--other"; + const originalCreate = vi.fn(); + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack" }, + graphql: vi.fn(), + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + + await expect( + withReleaseBranchRewriteCas( + github, + new Map([[otherBranch, { head: "b".repeat(40), recordId: null, pullRequestNumber: null }]]), + new Map(), + async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40) }), + ), + ).rejects.toThrow(/unrecorded generated branch/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + }); + it("uses CAS for the first update after creating a generated branch", async () => { const branch = "release-please--branches--master--components--pack"; const initial = "b".repeat(40); @@ -495,6 +550,99 @@ describe("Release Please workflow wrapper", () => { create.mockRestore(); } }); + + it("closes a confirmed CAS receipt before propagating a later no-release failure", async () => { + const branch = "release-please--branches--master--components--pack"; + const before = "b".repeat(40); + const after = "c".repeat(40); + const marker = ``; + const originalCreate = vi.fn(); + const originalUpdate = vi.fn(); + const github = { + repository: { owner: "lamemustafa", repo: "pack", defaultBranch: "master" }, + graphql: vi + .fn() + .mockResolvedValueOnce({ repository: { id: "repo-id" } }) + .mockResolvedValueOnce({ updateRefs: { clientMutationId: null } }), + octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, + }; + const createReleases = vi.fn().mockResolvedValue([]); + const createPullRequests = vi.fn(async () => { + await github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }); + throw new Error("later pull request API failure"); + }); + const create = vi.spyOn(releasePlease.GitHub, "create").mockResolvedValue(github); + const fromManifest = vi + .spyOn(releasePlease.Manifest, "fromManifest") + .mockResolvedValueOnce({ createReleases }) + .mockResolvedValueOnce({ createPullRequests }); + let markerExists = false; + let closedBody = ""; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init: RequestInit = {}) => { + const path = new URL(String(url)).pathname; + if (path.includes("matching-refs")) { + return { + ok: true, + status: 200, + json: async () => [{ ref: `refs/heads/${branch}`, object: { sha: before } }], + } as unknown as Response; + } + if (path.includes("/pulls")) { + return { + ok: true, + status: 200, + json: async () => [{ number: 337 }], + } as unknown as Response; + } + if (path.includes("/comments") && String(init.method ?? "GET") === "POST") { + markerExists = true; + return { ok: true, status: 201, json: async () => ({ id: 99 }) } as unknown as Response; + } + if (path.includes("/comments") && String(init.method ?? "GET") === "PATCH") { + closedBody = String(init.body); + return { ok: true, status: 200, json: async () => ({ id: 99 }) } as unknown as Response; + } + if (path.includes("/comments")) { + return { + ok: true, + status: 200, + json: async () => + markerExists + ? [ + { + id: 99, + created_at: "2026-09-12T00:00:00Z", + user: { login: RECORDER }, + body: marker, + }, + ] + : [], + } as unknown as Response; + } + throw new Error(`Unexpected GitHub request: ${path}`); + }), + ); + + try { + await expect( + runReleasePlease({ + GITHUB_REPOSITORY: "lamemustafa/pack", + GITHUB_TOKEN: "test-token", + GITHUB_API_URL: "https://api.github.test", + }), + ).rejects.toThrow(/later pull request API failure/iu); + + expect(originalUpdate).not.toHaveBeenCalled(); + expect(closedBody).toContain(`before=${before}`); + expect(closedBody).toContain(`after=${after}`); + } finally { + vi.unstubAllGlobals(); + fromManifest.mockRestore(); + create.mockRestore(); + } + }); }); describe("release branch rewrite records", () => { @@ -602,6 +750,22 @@ describe("release branch rewrite records", () => { expect(posted?.body).not.toContain("after="); }); + it("captures a retained generated branch without inventing a rewrite marker", async () => { + const head = "b".repeat(40); + const calls = stubGitHub({ heads: head, pulls: [] }); + const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + const records = await openBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + }); + + expect(records.get(branch)).toEqual({ head, recordId: null, pullRequestNumber: null }); + expect(calls.find((call) => call.method === "POST")).toBeUndefined(); + }); + it("holds an interrupted record when its branch changed", async () => { const lostBefore = "c".repeat(40); const created = "b".repeat(40); @@ -712,6 +876,46 @@ describe("release branch rewrite records", () => { expect(calls.find((call) => call.method === "POST")).toBeUndefined(); }); + it("refuses a retained generated branch that advanced after observation", async () => { + const before = "b".repeat(40); + const calls = stubGitHub({ heads: "c".repeat(40), pulls: [] }); + const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + refreshBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([ + [branch, { head: before, recordId: null, pullRequestNumber: null }], + ]), + }), + ).resolves.toBe(false); + + expect(calls.find((call) => call.method === "POST")).toBeUndefined(); + }); + + it("refuses a retained generated branch that gained a pull request after observation", async () => { + const head = "b".repeat(40); + const calls = stubGitHub({ heads: head }); + const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); + + await expect( + refreshBranchRewriteRecords({ + env, + owner: "lamemustafa", + repo: "pack", + targetBranch: "master", + headsBeforeRegeneration: new Map([ + [branch, { head, recordId: null, pullRequestNumber: null }], + ]), + }), + ).resolves.toBe(false); + + expect(calls.find((call) => call.method === "POST")).toBeUndefined(); + }); + it("refuses a force advance after capture without confusing it for an ordinary one", async () => { const snapshot = "b".repeat(40); const arrivedSince = "e".repeat(40); From 4bf1130ae6d808d1e138d9a77c5567f2b3ce2850 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Sat, 12 Sep 2026 17:10:58 +0530 Subject: [PATCH 17/17] fix(release): hold unrecorded retained rewrites --- scripts/run-release-please.d.mts | 8 +- scripts/run-release-please.mjs | 107 +++++++++----------- tests/scripts/run-release-please.test.ts | 121 ++--------------------- 3 files changed, 59 insertions(+), 177 deletions(-) diff --git a/scripts/run-release-please.d.mts b/scripts/run-release-please.d.mts index 9bf81189..a2db5a6a 100644 --- a/scripts/run-release-please.d.mts +++ b/scripts/run-release-please.d.mts @@ -22,15 +22,13 @@ interface ReleaseBranchScope { interface OpenedRewriteRecords extends ReleaseBranchScope { headsBeforeRegeneration: Map< string, - { head: string; recordId: number | null; pullRequestNumber: number | null } + { head: string; recordId: number; pullRequestNumber: number } >; } export function openBranchRewriteRecords( options: ReleaseBranchScope, -): Promise< - Map ->; +): Promise>; /** `false` when it could not confirm what the regeneration is about to discard. */ export function refreshBranchRewriteRecords(options: OpenedRewriteRecords): Promise; @@ -43,7 +41,7 @@ export function closeBranchRewriteRecords(options: { string, { record: { id: number; marker: { branch: string; before: string } }; after: string } >; -}): Promise; +}): Promise; export function withReleaseBranchRewriteCas( github: { diff --git a/scripts/run-release-please.mjs b/scripts/run-release-please.mjs index a4d5313d..1ce3d96a 100644 --- a/scripts/run-release-please.mjs +++ b/scripts/run-release-please.mjs @@ -57,25 +57,25 @@ export async function runReleasePlease(env = process.env) { const releases = (await releaseManifest.createReleases()).filter(Boolean); const outputs = buildReleaseOutputs(releases); - - const pullRequestManifest = await Manifest.fromManifest( - github, - targetBranch, - configFile, - manifestFile, - ); - // Immediately before the force-push, not at the snapshot above: see - // `refreshBranchRewriteRecords` for why the gap between the two is the dangerous part. - const recordsNameTheCurrentHeads = await refreshBranchRewriteRecords({ - env, - owner, - repo, - targetBranch, - headsBeforeRegeneration, - }); let pullRequests = []; const confirmedRewrites = new Map(); + let regenerationError = null; try { + const pullRequestManifest = await Manifest.fromManifest( + github, + targetBranch, + configFile, + manifestFile, + ); + // Immediately before the force-push, not at the snapshot above: see + // `refreshBranchRewriteRecords` for why the gap between the two is the dangerous part. + const recordsNameTheCurrentHeads = await refreshBranchRewriteRecords({ + env, + owner, + repo, + targetBranch, + headsBeforeRegeneration, + }); if (!recordsNameTheCurrentHeads) { throw new Error("Could not confirm release branch rewrite records for regeneration."); } @@ -86,15 +86,21 @@ export async function runReleasePlease(env = process.env) { async () => (await pullRequestManifest.createPullRequests()).filter(Boolean), ); } catch (error) { - // A release already exists only when its outputs can carry the later asset publication steps. - if (releases.length === 0) throw error; + regenerationError = error; + } + const closeErrors = await closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }); + if (releases.length === 0 && (regenerationError || closeErrors.length > 0)) { + const errors = regenerationError ? [regenerationError, ...closeErrors] : closeErrors; + const reasons = errors.map((error) => error?.message ?? String(error)).join("; "); + throw new AggregateError( + errors, + `Could not complete release pull request regeneration: ${reasons}`, + ); + } + if (regenerationError) { console.error( - `Could not regenerate release pull requests: ${error.message}. Existing release outputs remain available for asset publication.`, + `Could not regenerate release pull requests: ${regenerationError.message}. Existing release outputs remain available for asset publication.`, ); - } finally { - // A successful CAS can precede another release-please error. Its receipt must be closed before - // that error propagates, so the review gate never sees a completed rewrite as interrupted. - await closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }); } outputs.prs_created = String(pullRequests.length > 0); if (pullRequests.length > 0) { @@ -132,13 +138,10 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch branch, targetBranch, }); - if (pullRequestNumber === null) { - // A previous release pull request can close while its generated branch remains. This exact - // head is a same-run CAS precondition only; it is not continuity evidence and has no marker - // to close. - opened.set(branch, { head, recordId: null, pullRequestNumber: null }); - continue; - } + if (pullRequestNumber === null) + throw new Error( + `Retained release branch ${branch} has no open release pull request for a durable rewrite record; regeneration remains held.`, + ); const records = await readBranchRewriteRecords({ env, owner, repo, pullRequestNumber }); const open = records.find( (record) => record.marker.branch === branch && record.marker.after === null, @@ -173,13 +176,13 @@ export async function openBranchRewriteRecords({ env, owner, repo, targetBranch return opened; } -// Failures here are logged rather than thrown. A release may already exist by this point, and -// failing the job would strand it without its assets. The cost of not throwing is bounded: the -// record stays open, the review gate keeps refusing exactly as it would have, and the next run -// completes it. +// Every confirmed receipt is attempted even if another close fails. The caller decides whether +// those named failures remain terminal: no-release runs hold, while already-created releases keep +// their asset-publication outputs. export async function closeBranchRewriteRecords({ env, owner, repo, confirmedRewrites }) { - try { - for (const receipt of confirmedRewrites.values()) { + const errors = []; + for (const receipt of confirmedRewrites.values()) { + try { await writeBranchRewriteRecord({ env, owner, @@ -187,12 +190,14 @@ export async function closeBranchRewriteRecords({ env, owner, repo, confirmedRew record: receipt.record, after: receipt.after, }); + } catch (error) { + errors.push(error); + console.error( + `Could not close a release branch rewrite record: ${error.message}. The record remains held for review.`, + ); } - } catch (error) { - console.error( - `Could not close a release branch rewrite record: ${error.message}. The record stays open for review.`, - ); } + return errors; } /** @@ -307,7 +312,7 @@ export async function withReleaseBranchRewriteCas(github, expected, confirmedRew if (!result?.updateRefs) { throw new Error(`GitHub did not confirm the compare-and-swap update for ${branch}.`); } - if (record && record.recordId !== null) { + if (record) { confirmedRewrites.set(branch, { record: { id: record.recordId, marker: { branch, before: record.head } }, after: afterOid, @@ -333,26 +338,6 @@ async function eachOpenBranchRewriteRecord({ env, owner, repo, targetBranch, exp const head = heads.get(branch); if (!head) throw new Error(`Release branch ${branch} disappeared after its rewrite record was opened.`); - if (expectedRecord.recordId === null) { - if (head !== expectedRecord.head) { - throw new Error( - `Retained release branch ${branch} advanced after its exact head was observed; regeneration remains held.`, - ); - } - const pullRequestNumber = await findOpenPullRequestNumber({ - env, - owner, - repo, - branch, - targetBranch, - }); - if (pullRequestNumber !== null) { - throw new Error( - `Retained release branch ${branch} gained a release pull request after observation; regeneration remains held.`, - ); - } - continue; - } const pullRequestNumber = await findOpenPullRequestNumber({ env, owner, diff --git a/tests/scripts/run-release-please.test.ts b/tests/scripts/run-release-please.test.ts index aefeeaf7..e896cf3c 100644 --- a/tests/scripts/run-release-please.test.ts +++ b/tests/scripts/run-release-please.test.ts @@ -110,61 +110,6 @@ describe("Release Please workflow wrapper", () => { expect(github.octokit.git.createRef).toBe(originalCreate); }); - it("uses an exact retained-branch snapshot for a generated-branch CAS", async () => { - const branch = "release-please--branches--master--components--pack"; - const before = "b".repeat(40); - const after = "c".repeat(40); - const originalCreate = vi.fn(); - const originalUpdate = vi.fn(); - const graphql = vi - .fn() - .mockResolvedValueOnce({ repository: { id: "repo-id" } }) - .mockResolvedValueOnce({ updateRefs: { clientMutationId: null } }); - const github = { - repository: { owner: "lamemustafa", repo: "pack" }, - graphql, - octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, - }; - const confirmed = new Map(); - - await withReleaseBranchRewriteCas( - github, - new Map([[branch, { head: before, recordId: null, pullRequestNumber: null }]]), - confirmed, - async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: after, force: true }), - ); - - expect(originalUpdate).not.toHaveBeenCalled(); - expect(graphql).toHaveBeenLastCalledWith( - expect.stringContaining("updateRefs"), - expect.objectContaining({ refUpdates: [expect.objectContaining({ beforeOid: before })] }), - ); - expect(confirmed).toEqual(new Map()); - }); - - it("does not let one retained-branch snapshot authorize another generated branch", async () => { - const branch = "release-please--branches--master--components--pack"; - const otherBranch = "release-please--branches--master--components--other"; - const originalCreate = vi.fn(); - const originalUpdate = vi.fn(); - const github = { - repository: { owner: "lamemustafa", repo: "pack" }, - graphql: vi.fn(), - octokit: { git: { createRef: originalCreate, updateRef: originalUpdate } }, - }; - - await expect( - withReleaseBranchRewriteCas( - github, - new Map([[otherBranch, { head: "b".repeat(40), recordId: null, pullRequestNumber: null }]]), - new Map(), - async () => github.octokit.git.updateRef({ ref: `heads/${branch}`, sha: "c".repeat(40) }), - ), - ).rejects.toThrow(/unrecorded generated branch/iu); - - expect(originalUpdate).not.toHaveBeenCalled(); - }); - it("uses CAS for the first update after creating a generated branch", async () => { const branch = "release-please--branches--master--components--pack"; const initial = "b".repeat(40); @@ -602,7 +547,7 @@ describe("Release Please workflow wrapper", () => { } if (path.includes("/comments") && String(init.method ?? "GET") === "PATCH") { closedBody = String(init.body); - return { ok: true, status: 200, json: async () => ({ id: 99 }) } as unknown as Response; + return { ok: false, status: 500 } as unknown as Response; } if (path.includes("/comments")) { return { @@ -632,7 +577,7 @@ describe("Release Please workflow wrapper", () => { GITHUB_TOKEN: "test-token", GITHUB_API_URL: "https://api.github.test", }), - ).rejects.toThrow(/later pull request API failure/iu); + ).rejects.toThrow(/Could not complete release pull request regeneration/iu); expect(originalUpdate).not.toHaveBeenCalled(); expect(closedBody).toContain(`before=${before}`); @@ -750,19 +695,14 @@ describe("release branch rewrite records", () => { expect(posted?.body).not.toContain("after="); }); - it("captures a retained generated branch without inventing a rewrite marker", async () => { + it("holds a retained generated branch without a durable rewrite marker", async () => { const head = "b".repeat(40); const calls = stubGitHub({ heads: head, pulls: [] }); const { openBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - const records = await openBranchRewriteRecords({ - env, - owner: "lamemustafa", - repo: "pack", - targetBranch: "master", - }); - - expect(records.get(branch)).toEqual({ head, recordId: null, pullRequestNumber: null }); + await expect( + openBranchRewriteRecords({ env, owner: "lamemustafa", repo: "pack", targetBranch: "master" }), + ).rejects.toThrow(/no open release pull request.*durable rewrite record/iu); expect(calls.find((call) => call.method === "POST")).toBeUndefined(); }); @@ -876,46 +816,6 @@ describe("release branch rewrite records", () => { expect(calls.find((call) => call.method === "POST")).toBeUndefined(); }); - it("refuses a retained generated branch that advanced after observation", async () => { - const before = "b".repeat(40); - const calls = stubGitHub({ heads: "c".repeat(40), pulls: [] }); - const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - - await expect( - refreshBranchRewriteRecords({ - env, - owner: "lamemustafa", - repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: new Map([ - [branch, { head: before, recordId: null, pullRequestNumber: null }], - ]), - }), - ).resolves.toBe(false); - - expect(calls.find((call) => call.method === "POST")).toBeUndefined(); - }); - - it("refuses a retained generated branch that gained a pull request after observation", async () => { - const head = "b".repeat(40); - const calls = stubGitHub({ heads: head }); - const { refreshBranchRewriteRecords } = await import("../../scripts/run-release-please.mjs"); - - await expect( - refreshBranchRewriteRecords({ - env, - owner: "lamemustafa", - repo: "pack", - targetBranch: "master", - headsBeforeRegeneration: new Map([ - [branch, { head, recordId: null, pullRequestNumber: null }], - ]), - }), - ).resolves.toBe(false); - - expect(calls.find((call) => call.method === "POST")).toBeUndefined(); - }); - it("refuses a force advance after capture without confusing it for an ordinary one", async () => { const snapshot = "b".repeat(40); const arrivedSince = "e".repeat(40); @@ -1111,10 +1011,9 @@ describe("release branch rewrite records", () => { expect(patched?.body).not.toContain(landedSince); }); - it("leaves the record open when no event names the head the rewrite created", async () => { - // Not knowing is answered the way this module answers it everywhere: the gate ignores an open - // record and the next run completes it. Closing with an uncorroborated head would publish a - // claim about a rewrite nothing backs. + it("does not write a marker without a CAS receipt", async () => { + // A receipt is the only attribution evidence. Closing with an uncorroborated head would + // publish a claim about a rewrite nothing backs. const before = "c".repeat(40); const calls = stubGitHub({ heads: "b".repeat(40), @@ -1171,6 +1070,6 @@ describe("release branch rewrite records", () => { repo: "pack", confirmedRewrites: new Map(), }), - ).resolves.toBeUndefined(); + ).resolves.toEqual([]); }); });