fix(comment): say "waiting on checks" when a hold is not asking anyone to act

src/review/unified-comment.ts

@@ -426,14 +426,58 @@ 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":
       return "approve/merge recommended";
     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) {

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<typeof renderUnifiedReviewComment>[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<UnifiedReviewInput>[] = [{ 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");
+  });
+});