diff --git a/.github/scripts/orchestrator-helpers.js b/.github/scripts/orchestrator-helpers.js index be74f498b..6a8ab5330 100644 --- a/.github/scripts/orchestrator-helpers.js +++ b/.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, diff --git a/.github/scripts/orchestrator.js b/.github/scripts/orchestrator.js index b48865e4d..7f3be87d1 100644 --- a/.github/scripts/orchestrator.js +++ b/.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); diff --git a/.github/scripts/tests/orchestrator-helpers.test.js b/.github/scripts/tests/orchestrator-helpers.test.js index 3b05ce16b..f63c29697 100644 --- a/.github/scripts/tests/orchestrator-helpers.test.js +++ b/.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); +}); diff --git a/.github/scripts/tests/orchestrator.test.js b/.github/scripts/tests/orchestrator.test.js index 53a845dfa..8b3979b77 100644 --- a/.github/scripts/tests/orchestrator.test.js +++ b/.github/scripts/tests/orchestrator.test.js @@ -21,7 +21,7 @@ function createMocks(eventName = 'issue_comment', commentBody = '/orchestrate') const updateIssueDetails = []; let masterIssueBody = `- [ ] #2\n- [ ] #3`; let currentTaskData = { labels: [], state: 'open' }; - let prData = { labels: [], state: 'open', merged: true, head: { ref: 'opencode/issue2-fix', sha: 'abc' } }; + let prData = { labels: [], state: 'open', merged: true, head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' } }; const github = { rest: { @@ -51,6 +51,13 @@ function createMocks(eventName = 'issue_comment', commentBody = '/orchestrate') return { data: [{ number: 1, labels: [{name: 'kind/orchestrator'}], state: 'open', body: masterIssueBody }] }; } }, + repos: { + // #872's merge gate reads the base tip and the head..base merge-base; + // the default models an up-to-date branch so the merge-path tests still + // reach mergePR. Tests exercising the stale-base gate override these. + getBranch: async () => ({ data: { commit: { sha: 'tipsha' } } }), + compare: async () => ({ data: { merge_base_commit: { sha: 'tipsha' }, commits: [] } }) + }, pulls: { list: async () => { return { data: [] }; // No PRs by default @@ -231,13 +238,69 @@ test('orchestrator.js test suite', async (t) => { assert.ok(calls.updateIssue.includes(1), 'Master issue was updated'); }); + await t.test('Review approve with auto-merge refuses a branch behind master (#872)', async () => { + const { core, github, context, calls, updateIssueDetails, setMasterBody, setTaskData } = createMocks('workflow_run'); + + setMasterBody(``); + setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); + github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); + github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); + github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; + // A branch that does not contain the base tip. + github.rest.repos.getBranch = async () => ({ data: { commit: { sha: 'tipsha' } } }); + github.rest.repos.compare = async () => ({ + data: { + merge_base_commit: { sha: 'oldsha' }, + total_commits: 3, + commits: [{ sha: 'cafebabecafebabecafebabecafebabecafebabe', commit: { message: 'Add JNodeOceanTheme' } }] + } + }); + + await runOrchestrator({ github, context, core }); + + assert.strictEqual(calls.mergePR.length, 0, "a stale branch must not be squash-merged"); + assert.ok(calls.createComment.some(c => c.issue_number === 99 && + c.body.includes('Add JNodeOceanTheme') && c.body.includes('approve again')), + "the refusal names the would-be-reverted commits and the human-approval remedy"); + assert.ok(updateIssueDetails.some(u => u.issue_number === 1 && + u.body.includes('"phase": "HUMAN_REVIEW"')), "the task routes to HUMAN_REVIEW"); + assert.ok(updateIssueDetails.some(u => u.issue_number === 1 && + u.body.includes('merge_refused_base_stale')), "the refusal is in the master history"); + }); + + await t.test('Human approval refuses a branch behind master (#872)', async () => { + const { core, github, context, calls, updateIssueDetails, setMasterBody, setTaskData } = createMocks('pull_request_review'); + + setMasterBody(``); + setTaskData({ labels: [], state: 'open' }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); + github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); + github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; + github.rest.repos.getBranch = async () => ({ data: { commit: { sha: 'tipsha' } } }); + github.rest.repos.compare = async () => ({ + data: { + merge_base_commit: { sha: 'oldsha' }, + commits: [{ sha: 'cafebabecafebabecafebabecafebabecafebabe', commit: { message: 'Add JNodeOceanTheme' } }] + } + }); + + await runOrchestrator({ github, context, core }); + + assert.strictEqual(calls.mergePR.length, 0, "a human approval must not merge a stale branch"); + assert.ok(calls.createComment.some(c => c.issue_number === 99 && + c.body.includes('approve again')), "the refusal says the approval does not carry over"); + assert.ok(updateIssueDetails.some(u => u.issue_number === 1 && + u.body.includes('"phase": "REVIEW"')), "the task falls back to REVIEW"); + }); + await t.test('Review approve with auto-merge and green CI merges and advances', async () => { const { core, github, context, calls, updateIssueDetails, setMasterBody, setTaskData } = createMocks('workflow_run'); setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; @@ -256,7 +319,7 @@ test('orchestrator.js test suite', async (t) => { github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); // The merge call resolves with an empty body; the verification read still // reports an unmerged PR. - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, state: 'open', merged: false } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, state: 'open', merged: false } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; @@ -283,7 +346,7 @@ test('orchestrator.js test suite', async (t) => { const { core, github, context, calls, setMasterBody } = createMocks('pull_request_review'); setMasterBody(``); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, state: 'open', merged: false } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, state: 'open', merged: false } }); await assert.rejects( runOrchestrator({ github, context, core }), @@ -305,7 +368,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('failure') } }) }; @@ -321,7 +384,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'core/src/native/x86/kernel.asm', additions: 3 }] }); // CI defaults to green, so this isolates the diff gate. @@ -422,7 +485,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('failure') } }) }; diff --git a/.github/scripts/tests/ticket-runner.test.js b/.github/scripts/tests/ticket-runner.test.js index d5f0d36a1..b0cac912b 100644 --- a/.github/scripts/tests/ticket-runner.test.js +++ b/.github/scripts/tests/ticket-runner.test.js @@ -160,6 +160,15 @@ function createMocks(eventName, { }; } }, + repos: { + // #872's merge gate reads the base branch tip and the head..base + // merge-base; the default here models an up-to-date branch so existing + // merge-path tests still reach mergePR. + getBranch: async () => ({ data: { commit: { sha: 'tipsha' } } }), + compare: async () => ({ + data: { merge_base_commit: { sha: 'tipsha' }, commits: [] } + }) + }, pulls: { listReviews: async () => ({ data: [] }), list: async () => { @@ -180,6 +189,7 @@ function createMocks(eventName, { data: { number: pull_number, head: currentPRHead, + base: { ref: "master" }, body: currentPRBody, state: currentPRMerged ? "closed" : "open", merged: currentPRMerged @@ -1424,6 +1434,76 @@ test("ticket-runner.js event handling suite", async (t) => { assert.ok(mocks.calls.createComment.some(c => c.issue_number === 99 && c.body.includes("/oc fix Address human review feedback."))); }); + await t.test("/oc approve is refused when the PR branch is behind master (#872)", async () => { + const initialBody = _replaceOrAppendStatus("Task", { + phase: "HUMAN_REVIEW", + pr: 99, + turn: 1, + max_turns: 3, + retries: 0, + started: new Date().toISOString(), + history: [] + }, 42); + + const mocks = createMocks("issue_comment", { + commentBody: "/oc approve", + commentOnPR: true, + issueBody: initialBody + }); + // A branch that does not contain master's tip: the squash would revert it. + mocks.github.rest.repos.getBranch = async () => ({ data: { commit: { sha: "tipsha" } } }); + mocks.github.rest.repos.compare = async () => ({ + data: { + merge_base_commit: { sha: "oldsha" }, + commits: [{ sha: "cafebabecafebabecafebabecafebabecafebabe", commit: { message: "Add JNodeOceanTheme" } }] + } + }); + + await runTicketRunner(mocks); + + assert.strictEqual(mocks.calls.mergePR.length, 0, "a stale branch must not be merged"); + assert.ok(mocks.calls.createComment.some(c => c.issue_number === 99 && + (c.body || "").includes("Add JNodeOceanTheme") && (c.body || "").includes("Merge master into the branch")), + "the refusal names the commits that would be reverted and the remedy"); + assert.ok(mocks.getIssueBody().includes('"phase": "REVIEW"'), + "the ticket falls back to REVIEW so a later approval can retry"); + assert.ok(mocks.getIssueBody().includes("merge_refused_base_stale"), + "the refusal is recorded in the ticket history"); + }); + + await t.test("/oc approve --force still refuses a stale base (#872)", async () => { + // --force exists for a red-CI PR, where the risk is local to the PR. A + // stale base is a master-side data loss, so the ticket's acceptance + // criteria ("cannot be merged by either runner") win: the escape hatch is + // the GitHub UI merge button, a human's explicit act. + const initialBody = _replaceOrAppendStatus("Task", { + phase: "HUMAN_REVIEW", + pr: 99, + turn: 1, + max_turns: 3, + retries: 0, + started: new Date().toISOString(), + history: [] + }, 42); + + const mocks = createMocks("issue_comment", { + commentBody: "/oc approve --force", + commentOnPR: true, + issueBody: initialBody + }); + mocks.github.rest.repos.getBranch = async () => ({ data: { commit: { sha: "tipsha" } } }); + mocks.github.rest.repos.compare = async () => ({ + data: { merge_base_commit: { sha: "oldsha" }, commits: [] } + }); + + await runTicketRunner(mocks); + + assert.strictEqual(mocks.calls.mergePR.length, 0, + "--force must not merge a branch that would revert master commits"); + assert.ok(mocks.getIssueBody().includes('"phase": "REVIEW"'), + "the ticket falls back to REVIEW"); + }); + await t.test("/oc approve on a HUMAN_REVIEW PR merges and closes the issue", async () => { const initialBody = _replaceOrAppendStatus("Task", { phase: "HUMAN_REVIEW", diff --git a/.github/scripts/ticket-runner.js b/.github/scripts/ticket-runner.js index b7bb19d67..1ef699d8a 100644 --- a/.github/scripts/ticket-runner.js +++ b/.github/scripts/ticket-runner.js @@ -814,6 +814,32 @@ module.exports = async ({ github, context, core }) => { core.info("Ticket runner: approval on PR #" + prNumber + " but CI is red; deferring merge."); return; } + // A stale branch must not reach the squash, even under --force: the squash + // applies the branch's whole tree, so it would revert the commits master + // gained since the branch was cut - a master-side data loss, unlike the + // red-CI case --force exists for, where the risk is local to the PR. The + // ticket's acceptance criteria are "cannot be merged by either runner", so + // the escape hatch is GitHub's UI merge button (which bypasses every gate + // and is a human's explicit act), not this command. + var baseInfo = await h.checkBaseCurrency(prNumber); + if (!baseInfo.current) { + state.phase = "REVIEW"; + state.retries = 0; + state.history.push({ + event: "merge_refused_base_stale", + pr: prNumber, + commits: baseInfo.lag.length, + timestamp: new Date().toISOString() + }); + await updateIssueState(issueNumber, state); + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, + body: h.baseCurrencyComment(prNumber, baseInfo, "human") + }); + core.info("Ticket runner: PR #" + prNumber + " is behind its base (" + + baseInfo.lag.length + " commits); refusing to merge."); + return; + } state.phase = "MERGE"; state.history.push({ event: force ? "human_approved_forced" : "human_approved",