From d5a08b7dee8ad1089c0c4ba6d0855b561a941662 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Wed, 9 Sep 2026 22:04:33 +0530 Subject: [PATCH 1/2] test(review-gate): pin the untraceable-rewrite premise before fixing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #342 blocks every release pull request: release-please force-pushes its branch, GitHub records the event with a null `before_commit_id`, and the gate fails closed on an untraceable rewrite. Two withdrawn attempts (#343, and the exemption argument behind it) tried to bypass that check. This pins the fact that makes a real fix possible instead. The same timeline event carries `commit_id` -- the head *after* the push -- even when `before_commit_id` is null. Verified against #337, where all three release-please force-pushes look like: before_commit_id: null commit_id: abdd12b… github-actions[bot] before_commit_id: null commit_id: f59d877… github-actions[bot] before_commit_id: null commit_id: 79161c9… github-actions[bot] So the orphaned heads are named in the timeline. Durable review state lives in check runs addressable by SHA, and those SHAs are exactly the ones being discarded: `loadForcePushedPriorShas` reads `before_commit_id`, sets `hasUntraceableRewrite`, and drops the event including the usable SHA in it. Two tests, both asserting current behaviour so a fix has something to flip: - state on a head named only by `commit_id` is never queried, and the run is refused. This is the premise; it passes today, which is the point. - a rewrite with neither field usable stays untraceable. Any widening must keep this failing closed. No production change here. Written first deliberately: two designs of mine on this gate have already been withdrawn for resting on unverified models, and this is the assertion that decides whether widening discovery is the right direction or whether #342 needs a storage redesign instead. --- .../scripts/publish-review-gate-check.test.ts | 65 ++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts index acaf2d33..aa8ac4df 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -429,6 +429,61 @@ 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 calls the rewrite untraceable and refuses -- but the same + // timeline event carries `commit_id`, the head after the push, which is where + // the orphaned durable state lives. + // + // This asserts CURRENT behaviour: the state is not found and the run is refused. + // It is the premise of the proposed fix, written before the fix so the fix has + // something to flip. If a change makes this pass without deliberately widening + // discovery, that change is doing something else. + it("cannot reach 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(" ") ?? ""; + + // The orphaned head is never queried, though the timeline names it. + expect( + calls.some((call) => call.join(" ").includes(`commits/${orphanedSha}/check-runs?`)), + "the orphaned head named by commit_id is not consulted today", + ).toBe(false); + expect(result.status).toBe(0); + expect(publicationText).toContain("conclusion=action_required"); + expect(publicationText).toContain("GitHub did not record the prior head"); + }); + + // 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 +835,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, }; } From c50b3de2acb01a98f3caab00880c0eeb16c2d89e Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Wed, 9 Sep 2026 22:14:37 +0530 Subject: [PATCH 2/2] fix(review-gate): recover prior heads a force-push names but does not record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Release-please regenerates its release branch by force-pushing, GitHub records those events with a null `before_commit_id`, and the gate refused every one as an untraceable rewrite. That blocked release pull requests outright: #337 sits with every check green, a clean review at its head, and all threads resolved, and the last release was v0.5.1 on 2026-08-17. The rewrites were never untraceable. The same timeline event carries `commit_id`, the head the push created, and durable review state lives in check runs addressable by SHA. `loadForcePushedPriorShas` read `before_commit_id`, set `hasUntraceableRewrite`, and discarded the event -- including the usable SHA in it. Verified on #337, where all three regenerations look like: before_commit_id: null commit_id: abdd12b… github-actions[bot] before_commit_id: null commit_id: f59d877… github-actions[bot] before_commit_id: null commit_id: 79161c9… github-actions[bot] Both fields are now treated as candidate heads to search. A rewrite naming neither is still untraceable and still fails closed. This recovers continuity rather than waiving it, which is the distinction that sank the earlier attempt (#343, withdrawn). That change exempted generated branches from the check; review showed the durable state it protects is real, and that exempting could publish success while dropping an observed finding. Here the discarded head is searched, so such a finding is found. State reached through a recovered head is still required to belong to the pull request -- dropping that validation fails two tests. Ordering moved from SHAs to events. The ambiguity check threw when two prior heads shared a timestamp, and one event now contributes two SHAs that necessarily share one. Their order is known -- the created head is newer than the discarded one -- so only a tie between distinct events is ambiguous. Left alone, every recovered rewrite would have thrown "ambiguous chronological ordering": a fix that fails differently. Mutation-tested: reverting the widening, never marking untraceable, and dropping the pull-request ownership check each fail. Closes #342. --- scripts/publish-review-gate-check.mjs | 48 +++++++++++++------ .../scripts/publish-review-gate-check.test.ts | 45 ++++++++++++----- 2 files changed, 65 insertions(+), 28 deletions(-) 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 aa8ac4df..ad08b06e 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -430,15 +430,11 @@ describe("PR-head Review gate check publisher", () => { }); // #342: the decisive case. release-please force-pushes leave `before_commit_id` - // null, so the gate calls the rewrite untraceable and refuses -- but the same - // timeline event carries `commit_id`, the head after the push, which is where - // the orphaned durable state lives. - // - // This asserts CURRENT behaviour: the state is not found and the run is refused. - // It is the premise of the proposed fix, written before the fix so the fix has - // something to flip. If a change makes this pass without deliberately widening - // discovery, that change is doing something else. - it("cannot reach durable state on a head named only by the force-push 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"], @@ -453,14 +449,37 @@ describe("PR-head Review gate check publisher", () => { const publicationText = calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs"))?.join(" ") ?? ""; - // The orphaned head is never queried, though the timeline names it. expect( calls.some((call) => call.join(" ").includes(`commits/${orphanedSha}/check-runs?`)), - "the orphaned head named by commit_id is not consulted today", - ).toBe(false); + "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"); - expect(publicationText).toContain("GitHub did not record the prior head"); }); // The other half of the premise: a rewrite with neither field usable must stay