diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index de57d915ea..beec005cf1 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -262,13 +262,19 @@ jobs: // A queue-superseded shard leaves no other trace in the logs. core.info(`Concurrency group: ${process.env.CONCURRENCY_GROUP}`); + // Turn labels reconcile exclusively among themselves: exactly one + // can be true. has-conflicts is deliberately NOT in this group + // (issue #2089) — whether the branch has conflicts is an + // independent fact that can coexist with any turn. It is managed + // only by reconcileConflictFlag below: set on a known dirty read, + // removed on a known read that is not dirty, untouched while + // mergeability is unknown. const stateLabels = [ 'awaiting-author', 'awaiting-coderabbit', 'awaiting-ready', 'awaiting-maintainer', 'awaiting-review', // Legacy label removed during reconciliation. - 'has-conflicts', ]; const labelDefinitions = [ { @@ -364,9 +370,13 @@ jobs: } } - // Strips stateLabels from a PR, optionally keeping one desired label. - // Callers skip this entirely while mergeability is unknown (see the - // hoisted check below), so no keep-list is needed. + // Strips stateLabels (turn labels) from a PR, optionally keeping + // one desired label. Callers skip this entirely while mergeability + // is unknown (see the hoisted check below), so no keep-list is + // needed. has-conflicts lives outside this group: it is the + // independent conflict flag managed by reconcileConflictFlag, so + // turn-label reconciliation — including the outer failure cleanup + // — never strips or sets it. // Stale-clock labels are owned by the stale step below and are never // touched here: stripping one on a transient CI or mergeability flap // would erase a live warning and restart its close grace (issue #2076). @@ -415,6 +425,50 @@ jobs: } } + // Reconciles the independent has-conflicts flag (issue #2089). + // `present` is a mergeability fact, so callers pass a known read + // only: true for a known dirty read, false for a known read that + // is not dirty. While mergeability is unknown no caller runs, so + // the flag freezes together with the turn labels. A conflicted PR + // must never sit in the maintainer queue, so every conflict + // branch pairs this call with reconcileLabels(pr, 'awaiting-author'). + async function reconcileConflictFlag(pr, present) { + if (isReadOnlyRun && isForkPR(pr)) { + core.info(`PR #${pr.number}: fork PR on a read-only run — skipping label mutation`); + return; + } + const hasLabel = pr.labels.some(label => label.name === 'has-conflicts'); + if (present === hasLabel) return; + try { + if (present) { + await github.rest.issues.addLabels({ + owner, repo, issue_number: pr.number, labels: ['has-conflicts'], + }); + if (!pr.labels.some(label => label.name === 'has-conflicts')) { + pr.labels.push({ name: 'has-conflicts' }); + } + } else { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pr.number, name: 'has-conflicts', + }); + pr.labels = pr.labels.filter(label => label.name !== 'has-conflicts'); + } + } catch (err) { + if (err.status === 404 && !present) { + pr.labels = pr.labels.filter(label => label.name !== 'has-conflicts'); + return; + } + // The flag already decided this run's label state, so a + // failed flag write must fail closed: mark the wrapper + // error so the outer cleanup keeps the current state + // labels exactly like the unknown-mergeability freeze + // instead of stripping them. + const error = new Error(`Could not reconcile has-conflicts label: ${err.message}`); + error.preserveStateLabels = true; + throw error; + } + } + async function findReviewGuide(pr) { const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: pr.number, per_page: 100, @@ -772,10 +826,18 @@ jobs: } if (prDetail.mergeable === false && prDetail.mergeable_state === 'dirty') { - core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`); + core.info( + `PR #${pr.number}: has merge conflicts — ` + + 'labeling has-conflicts and awaiting-author' + ); await updateReviewGate(pr, 'conflict', false); await setCodeRabbitReviewActive(pr, false); - await reconcileLabels(pr, 'has-conflicts'); + // Conflicts are an independent flag, and a conflicted PR + // always owes the author action (issue #2089): set the + // flag and force the awaiting-author turn label — never + // has-conflicts alone, and never a maintainer-queue label. + await reconcileConflictFlag(pr, true); + await reconcileLabels(pr, 'awaiting-author'); await reconcileCommunityApproval(pr, 'remove'); await updateReviewGuide(pr, 'conflict', existingGuide); continue; @@ -787,15 +849,17 @@ jobs: // `mergeable_state: "unknown"`, GitHub has not finished computing // the answer, so it says nothing about conflicts and nothing about // whose turn it is. This run then changes NO state label for the - // PR: every state-label reconcile below is skipped. Acting on + // PR: every state-label reconcile below is skipped, and the + // has-conflicts flag freezes with the turn labels. Acting on // unknown previously let the normal reconcile swap labels (for // example has-conflicts to awaiting-author and back on the next // run's `dirty` read), which flapped labels on unchanged PRs. // Known-dirty never reaches here (the conflict check above - // handles it first); known-clean reconciles normally. The - // awaiting-maintainer branch below still refetches mergeability - // before the maintainer handoff, because a push between this - // fetch and labeling can change the answer; a known read there + // handles it first); a known read that is not dirty removes + // the flag in the branches below. The awaiting-maintainer + // branch below still refetches mergeability before the + // maintainer handoff, because a push between this fetch and + // labeling can change the answer; a known read there // re-enables the final reconcile. let mergeabilityUnknown = prDetail.mergeable === null || prDetail.mergeable_state === 'unknown'; @@ -907,8 +971,11 @@ jobs: // mean a conflicting PR got no merge-ref CI run — not that its // conflicts went away. Unknown mergeability changes no state // label in this run, so this branch keeps every existing state - // label (has-conflicts included) instead of stripping them; a - // later run with a known read reconciles them. + // label (the has-conflicts flag included) instead of stripping + // them; a later run with a known read reconciles them. On a + // known read the branch got here only because the PR is not + // dirty (the conflict check handles dirty first), so the + // conflict flag is removed along with the turn labels. if (ciPending || ciFailed) { core.info( `PR #${pr.number}: CI ${ciPending ? 'pending' : 'failed'} — ` + @@ -919,6 +986,7 @@ jobs: await updateReviewGate(pr, ciPending ? 'ci-pending' : 'ci-failed', false); await setCodeRabbitReviewActive(pr, false); if (!mergeabilityUnknown) { + await reconcileConflictFlag(pr, false); await reconcileLabels(pr, null); } await reconcileCommunityApproval(pr, 'remove'); @@ -1147,10 +1215,18 @@ jobs: const headChanged = Boolean(latestPrDetail.head?.sha) && latestPrDetail.head.sha !== pr.head.sha; if (latestPrDetail.mergeable === false && latestPrDetail.mergeable_state === 'dirty') { - core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`); + core.info( + `PR #${pr.number}: has merge conflicts — ` + + 'labeling has-conflicts and awaiting-author' + ); await updateReviewGate(pr, 'conflict', false); await setCodeRabbitReviewActive(pr, false); - await reconcileLabels(pr, 'has-conflicts'); + // Same invariant as the first conflict branch (issue + // #2089): the flag plus the awaiting-author turn label, + // so a conflicted PR can never land in the maintainer + // queue this run was about to grant. + await reconcileConflictFlag(pr, true); + await reconcileLabels(pr, 'awaiting-author'); await reconcileCommunityApproval(pr, 'remove'); await updateReviewGuide(pr, 'conflict', existingGuide); continue; @@ -1212,6 +1288,10 @@ jobs: await updateReviewGuide(pr, phase, existingGuide); } if (!mergeabilityUnknown) { + // The read that re-enabled this reconcile is known and not + // dirty (the conflict branches handle dirty first), so the + // conflict flag comes off whenever the turn labels reconcile. + await reconcileConflictFlag(pr, false); await reconcileLabels(pr, desiredLabel); } await reconcileCommunityApproval(pr, communityDecision); diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index eaab566b23..e4be567420 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -736,6 +736,25 @@ describe("PR review-state workflow", () => { expect(result.setFailed).not.toHaveBeenCalled() }) + it("adds the conflict flag for fork PRs from pull_request_target", async () => { + const result = await runWorkflow({ eventName: "pull_request_target", fork: true, conflict: true }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + expect(result.setFailed).not.toHaveBeenCalled() + }) + + it("removes the conflict flag for fork PRs from pull_request_target", async () => { + const result = await runWorkflow({ + eventName: "pull_request_target", + fork: true, + conflict: false, + labels: ["has-conflicts"], + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "has-conflicts" })) + expect(result.setFailed).not.toHaveBeenCalled() + }) + it("creates missing workflow labels", async () => { const result = await runWorkflow({ existingRepoLabels: [] }) @@ -1113,6 +1132,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) expect(latestGateStatus(result)?.description).toContain("merge conflicts") }) @@ -1282,18 +1302,32 @@ describe("PR review-state workflow", () => { expect(resolution.prNumbers()).toEqual([1437]) expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + // A conflicted PR always owes the author action (issue #2089): the turn + // label lands on awaiting-author, never back on the maintainer queue. + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) }) - it("preserves awaiting-maintainer when adding has-conflicts fails", async () => { + it("preserves awaiting-maintainer when adding the has-conflicts flag fails (#2089)", async () => { const result = await runWorkflow({ conflict: true, labels: ["awaiting-maintainer"], - addLabelsStatus: 500, + addLabelsFailOnceName: "has-conflicts", }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) expect(result.removeLabel).not.toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) - expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("Could not add desired label")) + expect(result.setFailed).toHaveBeenCalledWith( + expect.stringContaining("Could not reconcile has-conflicts label"), + ) + }) + + it("reports a 404 when adding the has-conflicts flag", async () => { + const result = await runWorkflow({ conflict: true, addLabelsStatus: 404 }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + expect(result.setFailed).toHaveBeenCalledWith( + expect.stringContaining("Could not reconcile has-conflicts label"), + ) }) it("rechecks mergeability before tagging a PR awaiting maintainer", async () => { @@ -1318,6 +1352,8 @@ describe("PR review-state workflow", () => { expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + // The recheck-routed conflict branch also forces awaiting-author (#2089). + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) }) // Issue #2089: the first read reports unknown, one retry reads dirty, and the @@ -1335,6 +1371,7 @@ describe("PR review-state workflow", () => { expect(result.getPullRequest).toHaveBeenCalledTimes(2) expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) }) // Issue #2089: unknown on the first read and on every retry means the run @@ -1360,6 +1397,97 @@ describe("PR review-state workflow", () => { } }) + // Issue #2089 steps 3-4: the conflict flag is independent of the turn + // labels, and a conflicted PR always owes the author action. A known dirty + // read therefore sets has-conflicts and awaiting-author together. The + // shared-state harness carries run 1's actual writes into run 2, which + // then rewrites neither label. + it("carries awaiting-author and has-conflicts together, stable across consecutive runs (#2089)", async () => { + const sharedState: SharedState = { + labels: new Set(["awaiting-maintainer"]), + guide: { body: null }, + gate: { status: null }, + } + const first = await runWorkflow({ + sharedState, + conflict: true, + labels: ["awaiting-maintainer"], + }) + + expect(first.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + expect(first.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(first.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) + // Run 2 starts from run 1's real label state, not a hand-written copy. + expect([...sharedState.labels].sort()).toEqual(["awaiting-author", "has-conflicts"]) + + const second = await runWorkflow({ + sharedState, + conflict: true, + labels: [...sharedState.labels], + }) + + expect(second.addLabels).not.toHaveBeenCalled() + expect(second.removeLabel).not.toHaveBeenCalled() + expect(second.setFailed).not.toHaveBeenCalled() + }) + + // Issue #2089: a 404 on the flag removal means the label vanished between + // the read and the write. The removal goal is already met, so the run + // tolerates the 404 and keeps reconciling instead of failing closed. + it("tolerates a 404 when removing the has-conflicts flag on a known clean read (#2089)", async () => { + const result = await runWorkflow({ + labels: ["has-conflicts"], + removeLabelStatus: 404, + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "has-conflicts" })) + expect(result.setFailed).not.toHaveBeenCalled() + // The turn-label reconcile still runs after the tolerated removal. + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + }) + + // Issue #2089: the conflict-flag reconcile must honor the fork read-only + // guard in both directions. A dirty read wants to add the flag. A clean + // read with the flag present wants to remove it. The guard returns early; + // it is not a failure, so the run writes nothing and still succeeds. + it.each([ + { name: "known dirty read with the flag absent", options: { conflict: true } }, + { name: "known clean read with the flag present", options: { labels: ["has-conflicts"] } }, + ])("skips the has-conflicts reconcile for a fork PR on a read-only run: $name (#2089)", async ({ options }) => { + const result = await runWorkflow({ + ...options, + eventName: "pull_request_review", + fork: true, + }) + + expect(result.addLabels).not.toHaveBeenCalled() + expect(result.removeLabel).not.toHaveBeenCalled() + expect(result.setFailed).not.toHaveBeenCalled() + }) + + // Issue #2089 step 3: a known read that is not dirty removes the conflict + // flag on its own; the turn label keeps reconciling independently of it. + it("removes the has-conflicts flag on a known clean read without touching the turn label (#2089)", async () => { + const result = await runWorkflow({ + labels: ["has-conflicts", "awaiting-maintainer"], + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "has-conflicts" })) + // awaiting-maintainer was already present and stays: the flag removal + // neither re-adds nor strips the turn label. + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(result.removeLabel).not.toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) + expect(result.setFailed).not.toHaveBeenCalled() + }) + // Issue #2091: a failed mergeability retry adds no mergeability answer, so // the run keeps every state label exactly like the still-unknown path, // while gate invalidation and failure reporting still happen. @@ -2077,8 +2205,8 @@ describe("PR review-state workflow", () => { it("outer cleanup removes a label added before partial reconciliation failure", async () => { const result = await runWorkflow({ - labels: ["has-conflicts"], - removeLabelFailOnceName: "has-conflicts", + labels: ["awaiting-maintainer"], + removeLabelFailOnceName: "awaiting-maintainer", }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) @@ -2086,6 +2214,25 @@ describe("PR review-state workflow", () => { expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("Remove label failed once")) }) + // Issue #2089: a failed has-conflicts flag write must fail closed like the + // unknown-mergeability freeze — the outer cleanup keeps the current state + // labels instead of stripping them, because no known clean read decided + // anything this run. + it("keeps state labels when removing the has-conflicts flag fails (#2089)", async () => { + const result = await runWorkflow({ + labels: ["has-conflicts", "awaiting-maintainer"], + removeLabelFailOnceName: "has-conflicts", + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "has-conflicts" })) + // The turn-label reconcile never ran: the flag failure happens first. + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + expect(result.removeLabel).not.toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) + expect(result.setFailed).toHaveBeenCalledWith( + expect.stringContaining("Could not reconcile has-conflicts label"), + ) + }) + it("does not exclude a reconcile check from another integration", async () => { const result = await runWorkflow({ requiredContexts: ["reconcile"],