diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs index 62515612..2455ea8c 100644 --- a/scripts/publish-review-gate-check.mjs +++ b/scripts/publish-review-gate-check.mjs @@ -347,7 +347,7 @@ function loadForcePushedPriorShas(prNumber) { "force-push discontinuity discovery", ), ); - const priorHeads = new Map(); + const rewrites = []; let hasUntraceableRewrite = false; for (const event of flattenPages(timelinePages)) { @@ -356,28 +356,46 @@ function loadForcePushedPriorShas(prNumber) { if (!Number.isFinite(createdAt)) { throw new Error("force-push event has no valid creation timestamp"); } - if (!/^[0-9a-f]{40}$/iu.test(event.before_commit_id ?? "")) { + // `before_commit_id` is the discarded head and is what continuity wants. GitHub + // omits it for every release-please regeneration, which is what made those + // rewrites look untraceable and blocked release pull requests indefinitely + // (#342). The same event still names `commit_id`, the head the push created, and + // durable state published against either head is a check run addressable by its + // SHA -- so both are candidate heads to search rather than evidence to discard. + // + // This recovers continuity; it does not waive it. State found this way is still + // required to belong to this pull request, and a rewrite naming neither head + // remains untraceable below. + const shas = [event.commit_id, event.before_commit_id].filter((sha) => + /^[0-9a-f]{40}$/iu.test(sha ?? ""), + ); + if (shas.length === 0) { hasUntraceableRewrite = true; continue; } - const existing = priorHeads.get(event.before_commit_id); - if (!existing || createdAt > existing.createdAt) { - priorHeads.set(event.before_commit_id, { sha: event.before_commit_id, createdAt }); - } + rewrites.push({ createdAt, shas }); } - const orderedPriorHeads = [...priorHeads.values()].sort( - (left, right) => right.createdAt - left.createdAt, - ); - for (let index = 1; index < orderedPriorHeads.length; index += 1) { - if (orderedPriorHeads[index - 1].createdAt === orderedPriorHeads[index].createdAt) { + // Ordering is over events, not SHAs. Two SHAs from one event share its timestamp + // and their order is known -- the created head is newer than the discarded one -- + // so only a tie between distinct events is genuinely ambiguous. + rewrites.sort((left, right) => right.createdAt - left.createdAt); + for (let index = 1; index < rewrites.length; index += 1) { + if (rewrites[index - 1].createdAt === rewrites[index].createdAt) { throw new Error("force-push events have ambiguous chronological ordering"); } } - return { - priorHeads: orderedPriorHeads.map(({ sha }) => sha), - hasUntraceableRewrite, - }; + + const seen = new Set(); + const priorHeads = []; + for (const rewrite of rewrites) { + for (const sha of rewrite.shas) { + if (seen.has(sha)) continue; + seen.add(sha); + priorHeads.push(sha); + } + } + return { priorHeads, hasUntraceableRewrite }; } function flattenPages(value) { diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts index acaf2d33..ad08b06e 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -429,6 +429,80 @@ describe("PR-head Review gate check publisher", () => { expect(publicationText).not.toContain(rawUrl); }); + // #342: the decisive case. release-please force-pushes leave `before_commit_id` + // null, so the gate called the rewrite untraceable and refused -- but the same + // timeline event carries `commit_id`, the head after the push, which is where the + // orphaned durable state lives. Blocking every release pull request to protect + // state that was reachable all along is the defect this closes. + it("reaches durable state on a head named only by the force-push commit_id", () => { + const orphanedSha = "b".repeat(40); + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + { [orphanedSha]: cleanDurableState() }, + [forcePushEvent(null, "2026-08-17T00:00:00Z", orphanedSha)], + ); + const publicationText = + calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; + + expect( + calls.some((call) => call.join(" ").includes(`commits/${orphanedSha}/check-runs?`)), + "the orphaned head named by commit_id must be consulted", + ).toBe(true); + expect(result.status).toBe(0); + expect(publicationText).not.toContain("GitHub did not record the prior head"); + expect(publicationText).toContain("conclusion=success"); + }); + + // Continuity is recovered, not waived: state found through a recovered head is + // still required to belong to this pull request. + it("ignores recovered-head state that belongs to another pull request", () => { + const orphanedSha = "b".repeat(40); + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + { [orphanedSha]: cleanDurableState(2) }, + [forcePushEvent(null, "2026-08-17T00:00:00Z", orphanedSha)], + ); + const publicationText = + calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; + + expect( + calls.some((call) => call.join(" ").includes(`commits/${orphanedSha}/check-runs?`)), + ).toBe(true); + expect(result.status).toBe(0); + expect(publicationText).toContain("conclusion=action_required"); + }); + + // The other half of the premise: a rewrite with neither field usable must stay + // untraceable. Whatever widening happens must not make this reachable. + it("stays untraceable when neither before_commit_id nor commit_id is usable", () => { + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], + [pull(1)], + cleanReviewFixture(), + [{ status: 0 }], + null, + [{ status: 0 }], + { ["c".repeat(40)]: cleanDurableState() }, + [forcePushEvent(null, "2026-08-17T00:00:00Z", null)], + ); + 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).toContain("GitHub did not record the prior head"); + }); + it("publishes a re-creation remedy instead of seeding state across an untraceable rewrite", () => { const orphanedSha = "b".repeat(40); const { result, calls } = runScript( @@ -780,10 +854,18 @@ function reviewStateWithDeletedFinding( ); } -function forcePushEvent(beforeCommitId: string | null, createdAt = "2026-08-17T12:00:00Z") { +function forcePushEvent( + beforeCommitId: string | null, + createdAt = "2026-08-17T12:00:00Z", + commitId: string | null = null, +) { + // Real events carry `commit_id` -- the head *after* the push -- even when + // `before_commit_id` is null. Verified on #337, where all three release-please + // force-pushes have a null `before_commit_id` and a populated `commit_id`. return { event: "head_ref_force_pushed", before_commit_id: beforeCommitId, + ...(commitId === null ? {} : { commit_id: commitId }), created_at: createdAt, }; }