factory: refuse to squash-merge a branch that is behind its base

.github/scripts/orchestrator-helpers.js

@@ -274,6 +274,112 @@ module.exports = function createHelpers({ github, context, core }) {
            testFiles <= MAX_TEST_FILES;
   }
 
+  /**
+   * Is the PR's merge-base still the tip of its base branch?
+   *
+   * A branch cut before a later master merge, squash-merged as-is, DELETES
+   * whatever landed in the meantime: the squash applies the branch's whole tree,
+   * so the PR's own diff looks clean while master silently loses another agent's
+   * feature (JNodeOceanTheme, 270 lines, on #869 - ticket #872). The loss leaves
+   * no CI signal, because the reverted files still compile.
+   *
+   * Compares the merge-base of head and base with the base branch's current tip.
+   * Fail-closed: an unreadable PR or an API error reads as "not current", the
+   * same direction as isDiffSafe and isCIGreen, because a merge is unrecoverable
+   * while a refusal is not.
+   *
+   * @return {Promise<{current: boolean, base: string, lag: Array}>} where lag
+   *   lists the commits present on the base branch but not on the head - the
+   *   ones the squash would revert.
+   */
+  async function checkBaseCurrency(prNumber) {
+    var stale = { current: false, base: "", lag: [] };
+    var pr;
+    try {
+      pr = await github.rest.pulls.get({ owner, repo, pull_number: prNumber });
+    } catch (e) {
+      core.warning("checkBaseCurrency: pulls.get failed for PR #" + prNumber + ": " + e.message);
+      return stale;
+    }
+    var baseRef = pr.data && pr.data.base && pr.data.base.ref;
+    var headSha = pr.data && pr.data.head && pr.data.head.sha;
+    if (!baseRef || !headSha) return stale;
+    stale.base = baseRef;
+
+    var tip;
+    try {
+      var branch = await github.rest.repos.getBranch({ owner, repo, branch: baseRef });
+      tip = branch.data && branch.data.commit && branch.data.commit.sha;
+    } catch (e) {
+      core.warning("checkBaseCurrency: getBranch(" + baseRef + ") failed: " + e.message);
+      return stale;
+    }
+    if (!tip) return stale;
+
+    // compare head...base: the commits it returns are on the base branch but not
+    // on the head - exactly what the squash would revert.
+    try {
+      var compare = await github.rest.repos.compare({
+        owner, repo, base: headSha, head: baseRef
+      });
+      var mergeBase = compare.data && compare.data.merge_base_commit &&
+        compare.data.merge_base_commit.sha;
+      if (!mergeBase) return stale;
+      var lag = [];
+      var commits = compare.data.commits || [];
+      for (var i = 0; i < commits.length; i++) {
+        var c = commits[i] || {};
+        var message = (c.commit && c.commit.message) || "";
+        lag.push({ sha: (c.sha || "").slice(0, 8), subject: message.split("\n")[0] });
+      }
+      // compare paginates at 250 commits; total_commits is the exact count, so
+      // the caller's "N commits" is honest even on a very lagging branch.
+      if (typeof compare.data.total_commits === "number" &&
+          compare.data.total_commits !== lag.length) {
+        lag.push({ sha: "", subject: "... and " +
+          (compare.data.total_commits - (lag.length - 1)) + " more" });
+      }
+      return { current: mergeBase === tip, base: baseRef, lag: lag };
+    } catch (e) {
+      core.warning("checkBaseCurrency: compare failed for PR #" + prNumber + ": " + e.message);
+      return stale;
+    }
+  }
+
+  /**
+   * Human-readable refusal naming the commits a squash would revert.
+   *
+   * @param prNumber the PR
+   * @param info the checkBaseCurrency result
+   * @param entry how the merge was requested - "review" (an agent review the
+   *   ticket-runner re-posts) or "human" (a native GitHub approval). The remedy
+   *   line differs: re-requesting a review does nothing for a human approval,
+   *   which must be given again on the updated branch.
+   */
+  function baseCurrencyComment(prNumber, info, entry) {
+    var base = info.base || "master";
+    var lines = [
+      "Not merging: PR #" + prNumber + "'s branch is behind " + base +
+        ", so a squash would revert those commits."
+    ];
+    if (info.lag && info.lag.length) {
+      lines.push("");
+      for (var i = 0; i < info.lag.length && i < 11; i++) {
+        var item = info.lag[i];
+        lines.push(item.sha ? "- `" + item.sha + "` " + item.subject : "- " + item.subject);
+      }
+      lines.push("");
+      lines.push("Merge " + base + " into the branch (or re-push it on a current base), then " +
+        (entry === "human"
+          ? "approve again on the GitHub PR Review UI - the approval above does not carry over."
+          : "re-request the review.") +
+        " The diff must then contain only this PR's own changes.");
+    } else {
+      lines.push("The base branch state could not be read, so the merge is refused rather than guessed at.");
+    }
+    return lines.join("\n");
+  }
+
   /** True when CI check runs on the PR head SHA show success and no failure. */
   async function isCIGreen(prNumber) {
     var pr;
@@ -634,6 +740,8 @@ module.exports = function createHelpers({ github, context, core }) {
     getAgentReviewVerdict: getAgentReviewVerdict,
     isAutoMergeEligible: isAutoMergeEligible,
     isDiffSafe: isDiffSafe,
+    checkBaseCurrency: checkBaseCurrency,
+    baseCurrencyComment: baseCurrencyComment,
     touchesForbiddenPath: touchesForbiddenPath,
     parkForCI: parkForCI,
     DEFAULT_MAX_TURNS: DEFAULT_MAX_TURNS,

.github/scripts/orchestrator.js

@@ -602,6 +602,10 @@ module.exports = async ({ github, context, core }) => {
                     const diffSafe = await h.isDiffSafe(task.pr);
                     const ciGreen = await h.isCIGreen(task.pr);
                     const forbidden = diffSafe ? false : await h.touchesForbiddenPath(task.pr);
+                    // Only once CI is green: the base check costs API calls and
+                    // a red-CI PR must be parked before anything else.
+                    const baseInfo = ciGreen ? await h.checkBaseCurrency(task.pr)
+                      : { current: true, base: "", lag: [] };
                     if (!ciGreen) {
                       h.parkForCI(task, task.pr, {
                         event: 'merge_deferred',
@@ -613,6 +617,19 @@ module.exports = async ({ github, context, core }) => {
                         owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr,
                         body: "Agent review passed but auto-merge deferred: Java CI is not green yet. Will re-check when CI completes."
                       });
+                    } else if (!baseInfo.current) {
+                      // A stale branch must never reach the squash: it would
+                      // revert the commits master gained since the branch was
+                      // cut, with no CI signal (ticket #872).
+                      task.phase = 'HUMAN_REVIEW';
+                      task.retries = 0;
+                      state.history.push({ event: 'merge_refused_base_stale', task: task.issue,
+                        pr: task.pr, commits: baseInfo.lag.length, timestamp: new Date().toISOString() });
+                      await github.rest.issues.createComment({
+                        owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr,
+                        body: h.baseCurrencyComment(task.pr, baseInfo, "human")
+                      });
+                      core.error(`Task #${task.issue} PR #${task.pr} is behind its base; routing to HUMAN_REVIEW.`);
                     } else if (forbidden) {
                       task.phase = 'HUMAN_REVIEW';
                       task.retries = 0;
@@ -790,6 +807,23 @@ module.exports = async ({ github, context, core }) => {
           core.info(`Human approved PR #${task.pr} but CI is red; deferring merge.`);
           return;
         }
+        // The human-approval path must refuse a stale branch too: a squash
+        // applies the branch's whole tree, reverting the commits master gained
+        // since the branch was cut, with no CI signal (ticket #872).
+        const baseInfo = await h.checkBaseCurrency(task.pr);
+        if (!baseInfo.current) {
+          task.phase = 'REVIEW';
+          task.retries = 0;
+          state.history.push({ event: 'merge_refused_base_stale', task: task.issue,
+            pr: task.pr, commits: baseInfo.lag.length, timestamp: new Date().toISOString() });
+          await updateMasterIssue(masterIssueNumber, state);
+          await github.rest.issues.createComment({
+            owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr,
+            body: h.baseCurrencyComment(task.pr, baseInfo, "human")
+          });
+          core.error(`Human approved PR #${task.pr} but its branch is behind its base; refusing to merge.`);
+          return;
+        }
         task.phase = 'MERGE';
         await mergePR(task.pr);
         state.completed.push(task.issue);

.github/scripts/tests/orchestrator-helpers.test.js

@@ -320,3 +320,85 @@ test("parkForCI appends to a separate history array for the orchestrator shape",
   assert.strictEqual(history[0].event, "review_pending_ci");
   assert.strictEqual(history[0].pr, 99);
 });
+
+// ---------------------------------------------------------------------------
+// #872: a PR branch cut before a later master merge must not be squash-merged,
+// because applying the branch's whole tree reverts what master gained.
+// ---------------------------------------------------------------------------
+
+function githubWithBase({ tip, mergeBase, lagCommits = [], failOn = null, head = "headsha" } = {}) {
+  const { github, calls } = createGithub({});
+  github.rest.pulls.get = async () => ({
+    data: { number: 99, head: { ref: "opencode/issue99-x", sha: head }, base: { ref: "master" } }
+  });
+  github.rest.repos = {
+    getBranch: async (args) => {
+      calls.getBranch = args;
+      if (failOn === "getBranch") throw new Error("getBranch unavailable");
+      return { data: { commit: { sha: tip || "tipsha" } } };
+    },
+    compare: async (args) => {
+      calls.compare = args;
+      if (failOn === "compare") throw new Error("compare unavailable");
+      return {
+        data: {
+          merge_base_commit: { sha: mergeBase || "tipsha" },
+          commits: lagCommits.map((c, i) => ({ sha: c.sha, commit: { message: c.subject } }))
+        }
+      };
+    }
+  };
+  const context = { repo: { owner: "LSantha", repo: "jnode_ai" } };
+  return { h: createHelpers({ github, context, core: createCore() }), calls };
+}
+
+test("checkBaseCurrency: a branch containing the base tip is current", async () => {
+  const { h, calls } = githubWithBase({ tip: "tipsha", mergeBase: "tipsha", lagCommits: [] });
+  const info = await h.checkBaseCurrency(99);
+  assert.strictEqual(info.current, true, "the branch contains the base tip");
+  assert.strictEqual(info.base, "master");
+  assert.strictEqual(info.lag.length, 0, "nothing would be reverted");
+  assert.ok(calls.compare, "the merge-base is read from the compare API");
+});
+
+test("checkBaseCurrency: a lagging branch is not current and names what it reverts", async () => {
+  const { h } = githubWithBase({
+    tip: "tipsha",
+    mergeBase: "oldsha",
+    lagCommits: [
+      { sha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", subject: "Add JNodeOceanTheme" },
+      { sha: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", subject: "Fix FAT32 floor" }
+    ]
+  });
+  const info = await h.checkBaseCurrency(99);
+  assert.strictEqual(info.current, false, "the branch is behind master");
+  assert.strictEqual(info.lag.length, 2, "both commits would be reverted");
+  assert.strictEqual(info.lag[0].sha, "aaaaaaaa", "the sha is shortened for the comment");
+  assert.strictEqual(info.lag[0].subject, "Add JNodeOceanTheme", "the subject is the first line");
+  const comment = h.baseCurrencyComment(99, info, "review");
+  assert.ok(comment.includes("Add JNodeOceanTheme"),
+    "the refusal names the commits that would be reverted");
+  assert.ok(comment.includes("Merge master into the branch") && comment.includes("re-request the review"),
+    "the review-entry remedy says to update and re-review");
+  const human = h.baseCurrencyComment(99, info, "human");
+  assert.ok(human.includes("approve again") && !human.includes("re-request the review"),
+    "the human-entry remedy says to approve again, not re-review");
+});
+
+test("checkBaseCurrency: fail-closed on an unreadable base", async () => {
+  const { h } = githubWithBase({ failOn: "getBranch" });
+  const info = await h.checkBaseCurrency(99);
+  assert.strictEqual(info.current, false, "an unreadable base must block the merge");
+  assert.ok(h.baseCurrencyComment(99, info, "review").includes("could not be read"),
+    "the refusal says why rather than naming nothing");
+});
+
+test("checkBaseCurrency: fail-closed on an unreadable PR", async () => {
+  const { github } = createGithub({});
+  github.rest.pulls.get = async () => { throw new Error("no PR"); };
+  const context = { repo: { owner: "LSantha", repo: "jnode_ai" } };
+  const h = createHelpers({ github, context, core: createCore() });
+  const info = await h.checkBaseCurrency(99);
+  assert.strictEqual(info.current, false);
+  assert.strictEqual(info.lag.length, 0);
+});