Skip to content

chat/comment: the unified comment still says "Manual Review" for a state that no longer summons anyone #10117

Description

@JSONbored

Follow-up to #10116, split out deliberately rather than widened into it.

#10116 stops an unstable mergeable state from being a manual-review hold: no manual-review label, no eviction from the merge train, no maintainer summoned for a PR whose checks are merely unfinished. The unified comment was left alone in that change, and it is now inconsistent with the disposition it describes.

deriveUnifiedStatus (src/review/unified-comment.ts:397) downgrades an otherwise-ready PR to "held" when readiness.mergeStateHeld is set. That downgrade is correct and must stay — the comment must never claim "safe to merge" while GitHub reports the branch unstable. What is wrong is only the wording that "held" renders as:

  • src/review/unified-comment.ts:436"manual review recommended"
  • src/review/unified-comment.ts:504"**Suggested Action - Manual Review**"

So a PR that the gate has deliberately decided needs no human still tells the reader a human is recommended. It blocks nothing — it is text — but it is the same contradiction #5288 reported in the other direction, and it undercuts the autonomy claim on every in-flight PR.

Why this is not a one-line change

mergeStateHeld is true for dirty and behind as well as unstable, and "held" is additionally reached from at least five unrelated paths (ctx.heldForReview, ctx.mergeBlockedReason, ctx.preflightHeld, ctx.neverClosed, and the failed-count branch). Some of those genuinely are "a maintainer must act" — mergeBlockedReason in particular is the one hold a human must personally clear (#9862). Retitling the whole "held" status would mislabel those.

The fix therefore needs either a distinct status (a "waiting on checks" tier that is neither ready nor held) or a "held"-reason discriminator that picks the heading. That is a real design decision about the comment's status vocabulary, which is why it is not folded into #10116.

Constraint

Whatever shape it takes must keep the #5288 / #ready-needs-mergeable property intact: the comment must not say "safe to merge" while mergeable_state is dirty, behind, or unstable. test/unit/unified-comment.test.ts and test/unit/pr-disposition-invariants.test.ts's cross-surface suite both pin it.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions