From 53f58ab1ab20bfa8d889d3da5a5a2ae9a822cd0a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 03:19:06 -0700 Subject: [PATCH] fix(comment): say "waiting on checks" when a hold is not asking anyone to act #10116 stopped an unstable merge state from summoning a human: no manual-review label, not evicted from the merge train. The unified comment kept rendering that state as "manual review recommended" / "Suggested Action - Manual Review", so the comment and the disposition contradicted each other on the same PR -- the #5288 contradiction running the other way. `held` is reached from eight places and they split cleanly: an unsettled mergeable state is a WAIT that clears itself, while a guardrail hold, a durable GitHub merge refusal (#9862), an incomplete review, an explicit manual verdict, a close verdict on a never-closed author, and a real finding set all need a person. Only the wording branches, on one predicate defined beside the conditions in deriveUnifiedStatus that produce those cases -- re-deriving "is this actionable?" separately in the headline and the verdict box would be the same drift that let the comment and the disposition disagree to begin with. The status vocabulary is unchanged, so none of the four src readers or the existing assertions move. Deliberately NOT keyed on `ciState !== "passed"`. That vocabulary is passed|failed|unverified with no "pending" member, and `failed` already returns "blocked" -- so the only value that could reach the predicate via CI is `unverified`, which agent-actions.ts lists as a real manualHoldReason ("CI could not be verified"). An earlier draft included it and would have told a maintainer to stand down from a hold that was waiting on them; tsc rejecting a "pending" literal that does not exist is what surfaced it. Conservative by construction: any state the predicate does not recognise keeps today's Manual Review wording. Being wrongly told to look is a much smaller failure than being wrongly told not to. Closes #10117 --- src/review/unified-comment.ts | 52 ++++++++++++++++++++- test/unit/unified-comment.test.ts | 76 +++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) 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"); + }); +});