diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 8955cc545..47f748800 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -426,6 +426,49 @@ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedComme return status; } +/** + * PURE. Is a `held` status merely WAITING, rather than asking a person to act? (#10117) + * + * `held` is reached from two genuinely different situations, and until now both rendered as "Manual Review": + * + * WAITING -- CI has not finished or gone green, or GitHub reports the branch dirty/behind/unstable. Since + * #10116 an unstable state deliberately summons nobody: it resolves itself on the next push, + * and the disposition applies no `manual-review` label. Telling the reader a human is + * recommended contradicts what the gate actually decided, and on an in-flight PR it is simply + * wrong -- the answer is "wait", not "escalate". + * ACTIONABLE -- a guardrail hold, a durable GitHub merge refusal (#9862), an incomplete review, or a close + * verdict on an author who is never auto-closed. Each of these DOES need a person, and each + * must keep saying so. + * + * Defined here, immediately beside the conditions in deriveUnifiedStatus that produce those two cases, and + * consumed by BOTH wording sites -- the headline and the verdict box. Re-deriving "is this actionable?" + * separately in each would be the same drift that let the comment and the disposition disagree in the first + * place. + * + * Deliberately conservative: every actionable reason short-circuits it to false, so a state this does not + * recognise keeps today's "Manual Review" wording. Being wrongly told to look is a smaller failure than being + * wrongly told not to. + */ +function isWaitingRatherThanActionable(input: UnifiedReviewInput, ctx: UnifiedCommentContext): boolean { + if (input.decision === "manual" || input.decision === "close") return false; + if (ctx.heldForReview || ctx.mergeBlockedReason || ctx.preflightHeld || ctx.neverClosed) return false; + // A partial/split/blocking review is a real finding set, not a wait. + const recs = input.recommendations ?? []; + if ((input.consensusBlocker ?? (input.blockers ?? []).length > 0) || (input.failedCount ?? 0) > 0) return false; + if (recs.some((rec) => rec !== "merge")) return false; + // An unsettled MERGE STATE, and only that. + // + // Deliberately NOT `ciState !== "passed"`. The vocabulary is passed|failed|unverified with no "pending" + // member, and `failed` already returned "blocked" further up -- so the only value that could reach here via + // CI is `unverified`, which is emphatically not a wait: agent-actions.ts lists it as a manualHoldReason + // ("CI could not be verified") and the disposition genuinely holds the PR for a person. Softening that to + // "no action needed yet" would tell a maintainer to stand down from a hold that is waiting on them. Caught + // by tsc rejecting a "pending" literal that does not exist. + // + // A merge state GitHub has not settled is the real waiting case, and the one #10116 stopped escalating. + return input.readiness?.mergeStateHeld === true; +} + function headlineLabel(status: UnifiedCommentStatus, input: UnifiedReviewInput, ctx: UnifiedCommentContext): string { switch (status) { case "ready": @@ -433,7 +476,8 @@ function headlineLabel(status: UnifiedCommentStatus, input: UnifiedReviewInput, case "advisory": return "advisory review"; case "held": - return "manual review recommended"; + // #10117: only say a human is recommended when one actually is. See isWaitingRatherThanActionable. + return isWaitingRatherThanActionable(input, ctx) ? "waiting on checks" : "manual review recommended"; case "blocked": return input.decision === "close" && !ctx.neverClosed ? "reject/close recommended" : "fixes required"; } @@ -506,6 +550,12 @@ function verdictLine(status: UnifiedCommentStatus, input: UnifiedReviewInput, ct `\n${actionReasonBullets("The review passed — merge this pull request yourself to complete it.")}`, ); } + // #10117: a hold that is only waiting on checks asks for patience, not a person. Since #10116 an + // unstable merge state summons nobody and gets no manual-review label, so "Manual Review" here + // contradicted the disposition on the same PR -- the #5288 contradiction in the other direction. + if (isWaitingRatherThanActionable(input, ctx)) { + return nestedBox(`**${icon} Suggested Action - Waiting on Checks**${reasons("no action needed yet")}`); + } return nestedBox(`**${icon} Suggested Action - Manual Review**${reasons()}`); case "blocked": if (ctx.neverClosed) { diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 9474f926b..3912793f8 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -1161,3 +1161,79 @@ describe("a GitHub merge refusal must never render as safe to merge (#9862)", () expect(body).not.toContain("safe to merge"); }); }); + +// #10117: `held` is reached from two genuinely different situations, and both used to render as +// "Manual Review". +// +// WAITING -- GitHub has not settled the merge state (dirty/behind/unstable). Since #10116 an unstable +// state summons NOBODY: no manual-review label, not evicted from the merge train. The comment +// saying "manual review recommended" on that same PR is the #5288 contradiction in reverse. +// ACTIONABLE -- a guardrail hold, a durable GitHub merge refusal (#9862), an incomplete review, unverified +// CI, or a close verdict on a never-closed author. Each needs a person and must keep saying so. +// +// The narrowness is the point, and it was arrived at by being wrong first: an earlier version also treated +// `ciState !== "passed"` as waiting. But that vocabulary is passed|failed|unverified -- `failed` already +// returns "blocked", so the only reachable value was `unverified`, which agent-actions.ts lists as a real +// manualHoldReason ("CI could not be verified"). That version would have told a maintainer to stand down from +// a hold that was waiting on them. +describe("held wording: waiting on checks vs manual review (#10117)", () => { + const unsettledMergeState = { ...base, readiness: { ciState: "passed" as const, mergeStateHeld: true } }; + + it("says WAITING, not manual review, when GitHub's merge state is unsettled — the #10116 case", () => { + expect(deriveUnifiedStatus(unsettledMergeState)).toBe("held"); + const md = renderUnifiedReviewComment(unsettledMergeState); + expect(md).toContain("waiting on checks"); + expect(md).toContain("Waiting on Checks"); + expect(md).not.toContain("manual review recommended"); + expect(md).not.toContain("Suggested Action - Manual Review"); + }); + + it("REGRESSION: UNVERIFIED CI is actionable, never a wait", () => { + // The bug the first draft of this change would have shipped. agent-actions.ts holds the PR for a human on + // ciUnverified; the comment must agree rather than say "no action needed yet". + const md = renderUnifiedReviewComment({ ...base, readiness: { ciState: "unverified" } }); + expect(md).not.toContain("Waiting on Checks"); + expect(md).not.toContain("no action needed yet"); + }); + + it("INVARIANT: every ACTIONABLE hold still says Manual Review", () => { + const actionable: Array<[string, Parameters[1]]> = [ + ["guardrail hold", { heldForReview: true }], + ["GitHub refused the merge (#9862)", { mergeBlockedReason: "required status check is expected" }], + ["incomplete review (preflight hold)", { preflightHeld: true }], + ]; + for (const [name, ctx] of actionable) { + const md = renderUnifiedReviewComment(unsettledMergeState, ctx); + expect(md, name).toContain("Manual Review"); + expect(md, name).not.toContain("Waiting on Checks"); + } + }); + + it("INVARIANT: an explicit manual verdict is never softened to waiting", () => { + const md = renderUnifiedReviewComment({ ...unsettledMergeState, decision: "manual" }); + expect(md).toContain("manual review recommended"); + expect(md).not.toContain("waiting on checks"); + }); + + it("INVARIANT: a real finding set is not a wait, even with an unsettled merge state", () => { + // A blocker or a split review is a result, not a pending state -- softening it would hide findings behind + // "no action needed yet". + const findingSets: Partial[] = [{ blockers: ["something broke"] }, { recommendations: ["request_changes"] }, { failedCount: 1 }]; + for (const over of findingSets) { + const md = renderUnifiedReviewComment({ ...unsettledMergeState, ...over }); + expect(md, JSON.stringify(over)).not.toContain("Waiting on Checks"); + } + }); + + it("INVARIANT: a close verdict on a never-closed author stays Manual Review", () => { + const md = renderUnifiedReviewComment({ ...base, decision: "close" }, { neverClosed: true }); + expect(md).toContain("Manual Review"); + expect(md).not.toContain("Waiting on Checks"); + }); + + it("does not change a ready, green, settled PR", () => { + const md = renderUnifiedReviewComment({ ...base, decision: "merge", readiness: { ciState: "passed" } }); + expect(md).not.toContain("Waiting on Checks"); + expect(md).not.toContain("Manual Review"); + }); +});