A stack child must not block its parent out of the merge queue

packages/merge-steward/src/github/pr-client.ts

@@ -127,12 +127,12 @@ export class GitHubPRClient implements GitHubPRApi {
     }
   }
 
-  async listOpenPRs(): Promise<Array<{ number: number; branch: string; headSha: string }>> {
+  async listOpenPRs(): Promise<Array<{ number: number; branch: string; headSha: string; baseBranch: string }>> {
     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 });
     }

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<void>;
   /** List open PRs for restart-time eligibility scans. */
-  listOpenPRs(): Promise<Array<{ number: number; branch: string; headSha: string }>>;
+  listOpenPRs(): Promise<Array<{ number: number; branch: string; headSha: string; baseBranch: string }>>;
   /** Find the open PR number for a branch, or null if none exists. */
   findPRByBranch(branch: string): Promise<number | null>;
   /** Delete the PR's head branch from the remote (best-effort cleanup). */

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)

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,

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,

packages/merge-steward/src/service-queue.ts

@@ -5,14 +5,27 @@ 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 {
   const regex = new RegExp("^" + pattern.replace(/\*/g, ".*") + "$");
   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<FailureClass> = new Set<FailureClass>(["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) => {
@@ -110,7 +123,7 @@ export class MergeStewardQueueCommands {
   async scanStartupAdmissions(): Promise<void> {
     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");
       }
@@ -119,11 +132,22 @@ export class MergeStewardQueueCommands {
     }
   }
 
-  async scanEligibleOpenPrs(): Promise<{ scanned: number; admitted: number }> {
+  /**
+   * A full re-read of what is admissible.
+   *
+   * 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(
+    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)) admitted += 1;
+      if (await this.tryAdmit(pr.number, pr.branch, pr.headSha, options)) {
+        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<boolean> {
+  /** 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<boolean> {
     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 {

packages/merge-steward/src/sim/github-sim.ts

@@ -139,10 +139,15 @@ export class GitHubSim implements GitHubPRApi {
     return [];
   }
 
-  async listOpenPRs(): Promise<Array<{ number: number; branch: string; headSha: string }>> {
+  async listOpenPRs(): Promise<Array<{ number: number; branch: string; headSha: string; baseBranch: string }>> {
     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<void> {

packages/merge-steward/test/open-pr-ancestry.test.ts

@@ -20,9 +20,13 @@ function graph(relationships: string[], mergeBases: Record<string, string>): 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");