From 2317a674dd04b6736734e05edfd2006bc7b38877 Mon Sep 17 00:00:00 2001 From: "krasnoperov-coding-agent[bot]" <322146550+krasnoperov-coding-agent[bot]@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:49:24 +0200 Subject: [PATCH 1/2] fix: a stack child must not block its parent out of the queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PR whose base is another open PR's branch deadlocked both of them. The parent was admitted, found "unlanded history" shared with its child, and was evicted `policy_blocked`; the child was never admitted because its base is not the base branch. Neither could move, and nothing but a force-push to one of them would break it. The ancestry check guards a real hazard, but only in one direction. A stack child's candidate carries its parent's commits, which GitHub reviewed against the parent's branch rather than against main, so landing the child would bypass a review gate and it must wait. A parent's candidate carries only its own commits: the history it shares with the child is the parent's own, it is in the parent's diff, and it was reviewed there. Nothing is bypassed. The existing guard did cover a child that is current with its parent, through `isAncestor(parentHead, childHead)`. It misses a child that sits on an earlier parent commit — which is the ordinary state of a stack the moment the parent pushes again. Ancestry cannot recover the relationship at that point, so this reads the declared base, and `listOpenPRs` now returns it. A missing base never counts as a match, so an API that stopped returning the field would fail closed rather than disable the check in silence. The second half is getting back: `tryAdmit` refuses a head that matches the latest eviction, which is right for every class that is a fact about the head and wrong for `policy_blocked`, which is a fact about the world around it. A head evicted on environment is offered again by the startup scan and only there — a webhook saying one PR moved is no reason to re-litigate a policy decision, while a restart, which is when the policy itself can have changed, is exactly the moment to. --- .../merge-steward/src/github/pr-client.ts | 12 ++- packages/merge-steward/src/interfaces.ts | 2 +- .../merge-steward/src/open-pr-ancestry.ts | 14 ++++ .../merge-steward/src/reconciler-merge.ts | 1 + .../merge-steward/src/reconciler-prepare.ts | 1 + packages/merge-steward/src/service-queue.ts | 56 +++++++++++-- packages/merge-steward/src/sim/github-sim.ts | 9 ++- .../test/open-pr-ancestry.test.ts | 79 ++++++++++++++++++- .../test/stack-aware-admission.test.ts | 76 +++++++++++++++++- 9 files changed, 235 insertions(+), 15 deletions(-) diff --git a/packages/merge-steward/src/github/pr-client.ts b/packages/merge-steward/src/github/pr-client.ts index 3ea7106e..3e784e80 100644 --- a/packages/merge-steward/src/github/pr-client.ts +++ b/packages/merge-steward/src/github/pr-client.ts @@ -127,12 +127,12 @@ export class GitHubPRClient implements GitHubPRApi { } } - async listOpenPRs(): Promise> { + async listOpenPRs(): Promise> { const result = await exec("gh", [ "pr", "list", "--repo", this.repoFullName, "--state", "open", - "--json", "number,headRefName,headRefOid", + "--json", "number,headRefName,headRefOid,baseRefName", "--limit", "1000", ], { allowNonZero: true, githubRepoFullName: this.repoFullName }); @@ -145,8 +145,14 @@ export class GitHubPRClient implements GitHubPRApi { number: number; headRefName: string; headRefOid: string; + baseRefName: string; }>; - return data.map((pr) => ({ number: pr.number, branch: pr.headRefName, headSha: pr.headRefOid })); + return data.map((pr) => ({ + number: pr.number, + branch: pr.headRefName, + headSha: pr.headRefOid, + baseBranch: pr.baseRefName, + })); } catch (error) { throw new Error("GitHub returned malformed open PR data", { cause: error }); } diff --git a/packages/merge-steward/src/interfaces.ts b/packages/merge-steward/src/interfaces.ts index 9a2e4368..e3102374 100644 --- a/packages/merge-steward/src/interfaces.ts +++ b/packages/merge-steward/src/interfaces.ts @@ -52,7 +52,7 @@ export interface GitHubPRApi { /** Add and/or remove labels on a PR in a single edit. */ setLabels(prNumber: number, opts: { add?: string[]; remove?: string[] }): Promise; /** List open PRs for restart-time eligibility scans. */ - listOpenPRs(): Promise>; + listOpenPRs(): Promise>; /** Find the open PR number for a branch, or null if none exists. */ findPRByBranch(branch: string): Promise; /** Delete the PR's head branch from the remote (best-effort cleanup). */ diff --git a/packages/merge-steward/src/open-pr-ancestry.ts b/packages/merge-steward/src/open-pr-ancestry.ts index 00638f9c..86233bd4 100644 --- a/packages/merge-steward/src/open-pr-ancestry.ts +++ b/packages/merge-steward/src/open-pr-ancestry.ts @@ -15,11 +15,21 @@ export interface OpenPrAncestor { * captures that shared history. If it is absent from current main, landing the * candidate can bypass the other PR's review gates. Explicit stacks become * valid only after their shared parent history has landed in main. + * + * The danger is one-directional. A stack child's candidate carries its + * parent's commits, which GitHub reviewed against the parent's branch rather + * than against main, so the child must wait. A parent's candidate carries only + * its own commits: the history it shares with its child is the parent's own, + * it is in the parent's diff, and it was reviewed there. So a child never + * blocks its parent, and saying so takes the declared base — after the parent + * advances past the commit the child sits on, ancestry alone can no longer + * tell a stack child from an unrelated PR holding the same unlanded history. */ export async function findUnlandedOpenPrAncestors(params: { github: GitHubPRApi; git: GitOperations; currentPrNumber: number; + currentBranch: string; prHeadSha: string; candidateSha: string; baseSha: string; @@ -29,6 +39,10 @@ export async function findUnlandedOpenPrAncestors(params: { for (const openPr of openPrs) { if (openPr.number === params.currentPrNumber) continue; + // Declared onto this PR's branch: a child, whatever its head has done since. + // Both names must be present: a missing base must never read as a match, + // or an API that stopped returning one would disable this check in silence. + if (params.currentBranch !== "" && openPr.baseBranch === params.currentBranch) continue; if ( openPr.headSha !== params.prHeadSha && await params.git.isAncestor(params.prHeadSha, openPr.headSha) diff --git a/packages/merge-steward/src/reconciler-merge.ts b/packages/merge-steward/src/reconciler-merge.ts index 764dfa0c..6328a487 100644 --- a/packages/merge-steward/src/reconciler-merge.ts +++ b/packages/merge-steward/src/reconciler-merge.ts @@ -356,6 +356,7 @@ export async function mergeHead(ctx: ReconcileContext, entry: QueueEntry): Promi github: ctx.github, git: ctx.git, currentPrNumber: entry.prNumber, + currentBranch: entry.branch, prHeadSha: entry.headSha, candidateSha: landingSha, baseSha: currentBase, diff --git a/packages/merge-steward/src/reconciler-prepare.ts b/packages/merge-steward/src/reconciler-prepare.ts index ebd9e5f3..8dd6384d 100644 --- a/packages/merge-steward/src/reconciler-prepare.ts +++ b/packages/merge-steward/src/reconciler-prepare.ts @@ -45,6 +45,7 @@ export async function prepareEntry( github: ctx.github, git: ctx.git, currentPrNumber: entry.prNumber, + currentBranch: entry.branch, prHeadSha: entry.headSha, candidateSha: entry.headSha, baseSha, diff --git a/packages/merge-steward/src/service-queue.ts b/packages/merge-steward/src/service-queue.ts index 136ee749..3d42a49c 100644 --- a/packages/merge-steward/src/service-queue.ts +++ b/packages/merge-steward/src/service-queue.ts @@ -5,7 +5,7 @@ import type { GitHubPolicyCache } from "./github-policy.ts"; import { INVALIDATION_PATCH, selectDownstream } from "./invalidation.ts"; import type { GitHubPRApi, SpeculativeBranchBuilder } from "./interfaces.ts"; import type { QueueStore } from "./store.ts"; -import type { QueueEntry, QueueEntryStatus } from "./types.ts"; +import type { FailureClass, QueueEntry, QueueEntryStatus } from "./types.ts"; import { evaluateCheckPolicy, formatRequiredCheck } from "./check-policy.ts"; function matchGlob(pattern: string, value: string): boolean { @@ -13,6 +13,19 @@ function matchGlob(pattern: string, value: string): boolean { return regex.test(value); } +/** + * Evictions the same head can recover from, because what blocked it was the + * state around the PR rather than anything about the PR. + * + * Every other class is a fact about this head — it conflicts, its checks fail, + * its branch is broken — and re-admitting it unchanged would only reproduce + * the eviction. `policy_blocked` is the exception: it says another open PR's + * unlanded history is in the way, which the head cannot fix and which stops + * being true when that PR lands, closes, retargets, or when the admission + * policy itself changes. Those are re-read on the next startup scan. + */ +const ENVIRONMENTAL_FAILURES: ReadonlySet = new Set(["policy_blocked"]); + function getLatestEvictedEntry(entries: QueueEntry[], prNumber: number): QueueEntry | undefined { const evicted = entries.filter((entry) => entry.prNumber === prNumber && entry.status === "evicted"); evicted.sort((left, right) => { @@ -119,11 +132,22 @@ export class MergeStewardQueueCommands { } } + /** + * A full re-read of what is admissible. + * + * This runs on startup, which is when the admission policy can have changed + * under an entry that a previous version evicted for it. Heads evicted on + * environment are offered again here and nowhere else: a webhook says one PR + * moved, which is no reason to re-litigate a policy decision, while a restart + * is exactly the moment to. + */ async scanEligibleOpenPrs(): Promise<{ scanned: number; admitted: number }> { const open = await this.github.listOpenPRs(); let admitted = 0; for (const pr of open) { - if (await this.tryAdmit(pr.number, pr.branch, pr.headSha)) admitted += 1; + if (await this.tryAdmit(pr.number, pr.branch, pr.headSha, { rereadEnvironmentalEvictions: true })) { + admitted += 1; + } } return { scanned: open.length, admitted }; } @@ -146,7 +170,19 @@ export class MergeStewardQueueCommands { return true; } - async tryAdmit(prNumber: number, branch: string, headSha: string): Promise { + /** Whether this entry's eviction was about the world rather than the head. */ + private evictedOnEnvironment(entry: QueueEntry): boolean { + const incidents = this.store.listIncidents(entry.id); + const latest = incidents[incidents.length - 1]; + return latest !== undefined && ENVIRONMENTAL_FAILURES.has(latest.failureClass); + } + + async tryAdmit( + prNumber: number, + branch: string, + headSha: string, + options?: { rereadEnvironmentalEvictions?: boolean }, + ): Promise { if (this.config.excludeBranches.some((pattern) => matchGlob(pattern, branch))) { this.logger.debug({ prNumber, branch }, "Branch excluded from admission"); return false; @@ -160,11 +196,19 @@ export class MergeStewardQueueCommands { const latestEvicted = getLatestEvictedEntry(this.store.listAll(this.config.repoId), prNumber); if (latestEvicted?.headSha === headSha) { - this.logger.debug( + const rereadable = options?.rereadEnvironmentalEvictions === true + && this.evictedOnEnvironment(latestEvicted); + if (!rereadable) { + this.logger.debug( + { prNumber, headSha, evictedEntryId: latestEvicted.id }, + "PR head matches latest evicted entry, skipping admission until a new push", + ); + return false; + } + this.logger.info( { prNumber, headSha, evictedEntryId: latestEvicted.id }, - "PR head matches latest evicted entry, skipping admission until a new push", + "Re-admitting a head evicted on environment; the blocking condition is re-read from scratch", ); - return false; } try { diff --git a/packages/merge-steward/src/sim/github-sim.ts b/packages/merge-steward/src/sim/github-sim.ts index 454fe2b2..481040ee 100644 --- a/packages/merge-steward/src/sim/github-sim.ts +++ b/packages/merge-steward/src/sim/github-sim.ts @@ -139,10 +139,15 @@ export class GitHubSim implements GitHubPRApi { return []; } - async listOpenPRs(): Promise> { + async listOpenPRs(): Promise> { return [...this.prs.values()] .filter((pr) => !pr.merged && !pr.closed) - .map((pr) => ({ number: pr.number, branch: pr.branch, headSha: pr.headSha })); + .map((pr) => ({ + number: pr.number, + branch: pr.branch, + headSha: pr.headSha, + baseBranch: pr.baseRefName, + })); } async deleteBranch(_prNumber: number): Promise { diff --git a/packages/merge-steward/test/open-pr-ancestry.test.ts b/packages/merge-steward/test/open-pr-ancestry.test.ts index e2ccd90c..88818dbc 100644 --- a/packages/merge-steward/test/open-pr-ancestry.test.ts +++ b/packages/merge-steward/test/open-pr-ancestry.test.ts @@ -20,9 +20,13 @@ function graph(relationships: string[], mergeBases: Record): Git }; } -function openPrApi(prs: Array<{ number: number; branch: string; headSha: string }>): GitHubPRApi { +function openPrApi( + prs: Array<{ number: number; branch: string; headSha: string; baseBranch?: string }>, +): GitHubPRApi { return { - async listOpenPRs() { return prs; }, + async listOpenPRs() { + return prs.map((pr) => ({ ...pr, baseBranch: pr.baseBranch ?? "main" })); + }, } as GitHubPRApi; } @@ -45,6 +49,7 @@ test("finds shared open-PR history absent from current main after the parent adv "descendant|candidate": "candidate", }), currentPrNumber: 30, + currentBranch: "candidate", prHeadSha: "candidate", candidateSha: "candidate", baseSha: "main", @@ -58,6 +63,76 @@ test("finds shared open-PR history absent from current main after the parent adv }]); }); +test("a stack child never blocks its parent, even after the parent advances past it", async () => { + // The child sits on "parent-v1"; the parent has since pushed "parent". The + // shared history is the parent's own commit, which is in the parent's diff + // and was reviewed there, so landing the parent bypasses nothing. + const blockers = await findUnlandedOpenPrAncestors({ + github: openPrApi([ + { number: 30, branch: "parent", headSha: "parent", baseBranch: "main" }, + { number: 40, branch: "child", headSha: "child", baseBranch: "parent" }, + ]), + git: graph([], { "child|parent": "parent-v1" }), + currentPrNumber: 30, + currentBranch: "parent", + prHeadSha: "parent", + candidateSha: "parent", + baseSha: "main", + }); + + assert.deepEqual(blockers, []); +}); + +test("the parent still blocks its stack child", async () => { + // The reverse direction is the one that matters: the child's candidate + // carries the parent's commits, which GitHub reviewed against the parent's + // branch rather than against main. + const blockers = await findUnlandedOpenPrAncestors({ + github: openPrApi([ + { number: 30, branch: "parent", headSha: "parent", baseBranch: "main" }, + { number: 40, branch: "child", headSha: "child", baseBranch: "parent" }, + ]), + git: graph([], { "parent|child": "parent-v1" }), + currentPrNumber: 40, + currentBranch: "child", + prHeadSha: "child", + candidateSha: "child", + baseSha: "main", + }); + + assert.deepEqual(blockers, [{ + prNumber: 30, + branch: "parent", + headSha: "parent", + sharedAncestorSha: "parent-v1", + }]); +}); + +test("a PR that only declares this branch as its base is not enough to be believed about others", async () => { + // The skip is scoped to the declaring PR. An unrelated PR holding unlanded + // history still blocks, whatever some other PR declares. + const blockers = await findUnlandedOpenPrAncestors({ + github: openPrApi([ + { number: 30, branch: "parent", headSha: "parent", baseBranch: "main" }, + { number: 40, branch: "child", headSha: "child", baseBranch: "parent" }, + { number: 50, branch: "unrelated", headSha: "unrelated", baseBranch: "main" }, + ]), + git: graph([], { "child|parent": "parent-v1", "unrelated|parent": "shared" }), + currentPrNumber: 30, + currentBranch: "parent", + prHeadSha: "parent", + candidateSha: "parent", + baseSha: "main", + }); + + assert.deepEqual(blockers, [{ + prNumber: 50, + branch: "unrelated", + headSha: "unrelated", + sharedAncestorSha: "shared", + }]); +}); + test("evicts a hidden stack before candidate selection", async () => { const h = await createHarness({ ciRule: () => "pass" }); await h.gitSim.createBranch("blocked-parent", "main"); diff --git a/packages/merge-steward/test/stack-aware-admission.test.ts b/packages/merge-steward/test/stack-aware-admission.test.ts index b3a27ddc..a61c988e 100644 --- a/packages/merge-steward/test/stack-aware-admission.test.ts +++ b/packages/merge-steward/test/stack-aware-admission.test.ts @@ -36,7 +36,16 @@ function fakeGithub(prs: Map): GitHubPRApi { : []; }, async setLabels() {}, - async listOpenPRs() { return []; }, + async listOpenPRs() { + return [...prs.values()] + .filter((pr) => !pr.merged) + .map((pr) => ({ + number: pr.number, + branch: pr.branch, + headSha: pr.headSha, + baseBranch: pr.baseRefName ?? "main", + })); + }, async findPRByBranch() { return null; }, async deleteBranch() { /* no-op */ }, async listOpenPRsWithLabel() { return []; }, @@ -189,6 +198,71 @@ describe("stack-aware admission", () => { }); }); +describe("re-admission after an environmental eviction", () => { + function evictedQueue() { + const store = new MemoryStore(); + const queue = new MergeStewardQueueCommands( + config, + policy, + store, + fakeGithub(new Map([[100, basePr({ number: 100, branch: "feat-a", baseRefName: "main" })]])), + fakeSpecBuilder(), + noopLogger, + ); + return { store, queue }; + } + + async function evictAdmitted( + store: MemoryStore, + queue: MergeStewardQueueCommands, + failureClass: "policy_blocked" | "integration_conflict", + ) { + await queue.tryAdmit(100, "feat-a", "head-100"); + const entry = store.getEntryByPR("repo", 100)!; + store.insertIncident({ + id: `incident-${failureClass}`, + entryId: entry.id, + at: new Date().toISOString(), + failureClass, + context: { + version: 1, + failureClass, + baseSha: "main", + prHeadSha: entry.headSha, + queuePosition: entry.position, + baseBranch: "main", + branch: entry.branch, + issueKey: null, + retryHistory: [], + }, + outcome: "open", + }); + store.transition(entry.id, "evicted", {}, `evicted: ${failureClass}`); + } + + it("offers a policy-blocked head again on the startup scan", async () => { + const { store, queue } = evictedQueue(); + await evictAdmitted(store, queue, "policy_blocked"); + + // A webhook is no reason to re-litigate a policy decision. + assert.equal(await queue.tryAdmit(100, "feat-a", "head-100"), false); + + // A restart is: the policy that evicted it may not be the policy now. + const { admitted } = await queue.scanEligibleOpenPrs(); + assert.equal(admitted, 1, "the same head is admitted again without a new push"); + assert.equal(store.getEntryByPR("repo", 100)?.status, "queued"); + }); + + it("still holds a head evicted for something only a new push can fix", async () => { + const { store, queue } = evictedQueue(); + await evictAdmitted(store, queue, "integration_conflict"); + + const { admitted } = await queue.scanEligibleOpenPrs(); + assert.equal(admitted, 0, "a conflicting head unchanged would only conflict again"); + assert.equal(store.getEntryByPR("repo", 100), undefined); + }); +}); + function basePr(overrides: Partial & { number: number; branch: string }): PRStatus { return { number: overrides.number, From 5bcb21452411e9fd2f456cc291e23dfeb5b55b4b Mon Sep 17 00:00:00 2001 From: "krasnoperov-coding-agent[bot]" <322146550+krasnoperov-coding-agent[bot]@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:53:18 +0200 Subject: [PATCH 2/2] fix: bound environmental readmission to startup --- packages/merge-steward/src/service-queue.ts | 16 ++++++++-------- .../test/stack-aware-admission.test.ts | 6 ++++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/merge-steward/src/service-queue.ts b/packages/merge-steward/src/service-queue.ts index 3d42a49c..c5eb901f 100644 --- a/packages/merge-steward/src/service-queue.ts +++ b/packages/merge-steward/src/service-queue.ts @@ -123,7 +123,7 @@ export class MergeStewardQueueCommands { async scanStartupAdmissions(): Promise { this.logger.info({ repoId: this.config.repoId }, "Scanning startup admissions"); try { - const { scanned, admitted } = await this.scanEligibleOpenPrs(); + const { scanned, admitted } = await this.scanEligibleOpenPrs({ rereadEnvironmentalEvictions: true }); if (scanned > 0) { this.logger.info({ scanned, admitted }, "Startup scan for eligible open PRs complete"); } @@ -135,17 +135,17 @@ export class MergeStewardQueueCommands { /** * A full re-read of what is admissible. * - * This runs on startup, which is when the admission policy can have changed - * under an entry that a previous version evicted for it. Heads evicted on - * environment are offered again here and nowhere else: a webhook says one PR - * moved, which is no reason to re-litigate a policy decision, while a restart - * is exactly the moment to. + * This runs both periodically and on startup. Only the startup caller asks + * to re-read environmental evictions, because that is when the admission + * policy can have changed under an entry that a previous version evicted. */ - async scanEligibleOpenPrs(): Promise<{ scanned: number; admitted: number }> { + async scanEligibleOpenPrs( + options?: { rereadEnvironmentalEvictions?: boolean }, + ): Promise<{ scanned: number; admitted: number }> { const open = await this.github.listOpenPRs(); let admitted = 0; for (const pr of open) { - if (await this.tryAdmit(pr.number, pr.branch, pr.headSha, { rereadEnvironmentalEvictions: true })) { + if (await this.tryAdmit(pr.number, pr.branch, pr.headSha, options)) { admitted += 1; } } diff --git a/packages/merge-steward/test/stack-aware-admission.test.ts b/packages/merge-steward/test/stack-aware-admission.test.ts index a61c988e..83a3dbf6 100644 --- a/packages/merge-steward/test/stack-aware-admission.test.ts +++ b/packages/merge-steward/test/stack-aware-admission.test.ts @@ -247,9 +247,11 @@ describe("re-admission after an environmental eviction", () => { // A webhook is no reason to re-litigate a policy decision. assert.equal(await queue.tryAdmit(100, "feat-a", "head-100"), false); + // Nor is the same full scan that runs before every periodic queue tick. + assert.deepEqual(await queue.scanEligibleOpenPrs(), { scanned: 1, admitted: 0 }); + // A restart is: the policy that evicted it may not be the policy now. - const { admitted } = await queue.scanEligibleOpenPrs(); - assert.equal(admitted, 1, "the same head is admitted again without a new push"); + await queue.scanStartupAdmissions(); assert.equal(store.getEntryByPR("repo", 100)?.status, "queued"); });