Skip to content

fix(review): stale manual-review label silently blocks merge with no visible explanation #8067

Description

@JSONbored

What happened

PR #7994 (a maintainer/owner PR) tripped a missing_linked_issue gate blocker early on because its body used "Implements #7981" instead of a GitHub-recognized closing keyword. Since owner PRs are never auto-closed, the disposition planner's fallback hold (src/settings/agent-actions.ts, the manualHoldReason generic fallback around line 1034/1401) applied the manual-review label as a safety hold.

The PR body was then fixed to say "Closes #7981." and the blocker cleared. From that point on, every re-gate correctly recomputed a clean, review-good verdict and the bot's own PR comment said "✅ approve/merge recommended, no blockers" — but the manual-review label, once applied, is deliberately sticky by design (src/services/agent-action-executor.ts's live-label guard, ~line 370-396, the "#3472 split-brain" comment: "Only a maintainer removing the label ... lifts it"). Nothing in the system ever cleared it, and nothing told the maintainer it was still there.

Result: the PR sat fully green (CI + codecov + gate-approved) for 3+ hours with the executor silently denying every merge/approve attempt (agent.action.merge | denied | ... manual-review label "manual-review" is present on the live PR — merge not executed, repeated across a dozen+ regate passes), visible only via direct server-side audit_events inspection — nothing on the PR itself explained why an "approved" PR wasn't merging. It was eventually merged manually by the maintainer.

Root cause

Two independent design choices combine into a blind spot:

  1. manual-review is intentionally maintainer-only-removable (correct — prevents the bot from ever silently overriding a genuine human freeze).
  2. The public PR comment's heldForReview flag (derivePublicCommentMergeFacts, src/queue/processors.ts) only checked for a hard-guardrail-path hit — it never checked whether the live PR already carries the manual-review label. So the comment could say "ready to merge" while the execution layer silently disagreed, indefinitely, with zero visible signal.

Fix (this issue tracks; a PR should follow)

Broaden derivePublicCommentMergeFacts's heldForReview computation to also go true whenever the configured manual-review label (default "manual-review", honoring a repo's manualReviewLabel override/disable) is present on the live PR — not just on a guardrail hit. This makes the public comment correctly headline "manual review recommended" instead of a misleading "approve/merge recommended" any time the execution layer is actually going to refuse to act, regardless of why the label got applied (guardrail, migration collision, unlinked-issue match, or this generic owner/gate-blocker fallback).

This is a visibility-only fix — it does NOT touch the deliberate "only a maintainer removes this label" safety invariant. A maintainer still has to notice and remove the label, but now the PR itself will say so instead of silently claiming to be ready.

Impact

Any owner/admin PR that trips a trivially-fixable gate blocker (wrong closing-keyword phrasing, a stray guardrail-path touch, etc.) can silently self-inflict an indefinite stall — invisible without server log/DB access — even after the underlying issue is fully resolved and CI is green.

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