diff --git a/src/queue/processors.ts b/src/queue/processors.ts index bd30231655..f0b46f2857 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2008,9 +2008,17 @@ function publicCheckFailureDetails(details: LiveCiAggregate["failingDetails"]): * metrics — the caller keeps the `incr()` emit, which reads the gate conclusion this function never sees. * * The two flags exist so the COMMENT agrees with the ACTION the disposition planner will take: - * - `heldForReview` — a clean, green PR whose diff touches a hard-guardrail path is held for owner review by - * `planAgentMaintenanceActions`, never auto-merged, so the comment must not headline "safe to merge" - * (#guarded-hold-comment). Uses the same shared `isGuardrailHit` the planner uses, not a second copy. + * - `heldForReview` — true when EITHER a clean, green PR's diff touches a hard-guardrail path (uses the same + * shared `isGuardrailHit` the planner uses, not a second copy), OR the live PR already carries the configured + * manual-review label. Both cases mean `planAgentMaintenanceActions`/the action executor will never + * merge/approve this pass, so the comment must not headline "safe to merge" (#guarded-hold-comment). The + * label check exists because a manual-review hold, once applied (a guardrail hit, a since-resolved gate + * blocker on a protected author, a migration collision, ...), is DELIBERATELY sticky — only a maintainer + * removing it lifts the hold (agent-action-executor.ts's live-label guard) — but nothing previously reflected + * that live block back into this comment: a PR could clear every OTHER hold reason on a later pass and the + * comment would headline "approve/merge recommended" while the executor kept silently denying the merge/ + * approve action every single time, with no visible explanation anywhere on the PR (confirmed live on PR + * #7994, stuck ~3+ hours with a stale manual-review label from an earlier missing_linked_issue blocker). * - `neverClosed` — the disposition never auto-closes a repo-owner or protected-automation PR, so a gate * "close" verdict on one must headline "held", not "Closed" (#8/#9). */ @@ -2019,9 +2027,10 @@ export function derivePublicCommentMergeFacts(args: { mergeableState: string | null | undefined; authorLogin: string | null | undefined; liveCi: Pick; - settings: Pick; + settings: Pick; unifiedFiles: Awaited>; repoFullName: string; + prLabels: readonly string[]; }): PublicCommentMergeFacts { const mergeStateLabel = args.liveMergeState ?? args.mergeableState ?? undefined; // fail-safe to the stored value const ciState: MergeReadiness["ciState"] = @@ -2038,7 +2047,11 @@ export function derivePublicCommentMergeFacts(args: { ...(failingDetails.length > 0 ? { failingDetails } : {}), ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}), }; - const heldForReview = isGuardrailHit(changedPathsForGuardrail(args.unifiedFiles), resolveHardGuardrailGlobs(args.settings)); + const manualReviewLabel = args.settings.manualReviewLabel === null ? null : (args.settings.manualReviewLabel ?? AGENT_LABEL_NEEDS_REVIEW); + const manualReviewLabelPresent = + manualReviewLabel !== null && args.prLabels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase()); + const heldForReview = + isGuardrailHit(changedPathsForGuardrail(args.unifiedFiles), resolveHardGuardrailGlobs(args.settings)) || manualReviewLabelPresent; const repoOwner = args.repoFullName.includes("/") ? args.repoFullName.slice(0, args.repoFullName.indexOf("/")) : ""; const authorLogin = args.authorLogin ?? ""; const neverClosed = @@ -10876,6 +10889,7 @@ async function maybePublishPrPublicSurface( settings, unifiedFiles, repoFullName, + prLabels: pr.labels, }); // The public comment must match the authoritative Gate check-run conclusion. const commentGate = commentGateEvaluation; diff --git a/test/unit/processors-public-comment-merge-facts.test.ts b/test/unit/processors-public-comment-merge-facts.test.ts index e5b01539b5..fda8730c35 100644 --- a/test/unit/processors-public-comment-merge-facts.test.ts +++ b/test/unit/processors-public-comment-merge-facts.test.ts @@ -11,7 +11,8 @@ const UNGUARDED_FILE = { path: "README.md" } as PullRequestFileRecord; const NO_GUARDRAIL_OVERRIDES = { hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: false, -} as Pick; + manualReviewLabel: undefined, +} as Pick; function facts(overrides: Partial[0]> = {}) { return derivePublicCommentMergeFacts({ @@ -22,6 +23,7 @@ function facts(overrides: Partial, + settings: { + hardGuardrailGlobs: [], + hardGuardrailGlobsOverridesInvariants: true, + manualReviewLabel: undefined, + } as Pick, }).heldForReview, ).toBe(false); }); }); +// #7994-follow-up: a manual-review hold is deliberately sticky (only a maintainer removing the label lifts it — +// see agent-action-executor.ts's live-label guard), but nothing previously reflected that live block back into +// this comment. A PR could clear every OTHER hold reason (guardrail, missing_linked_issue, ...) on a later pass +// and the comment would headline "approve/merge recommended" while the executor kept silently denying merge/ +// approve, with no visible explanation anywhere on the PR — confirmed live on PR #7994, stuck ~3+ hours. +describe("derivePublicCommentMergeFacts() — manual-review label hold (#7994-follow-up)", () => { + it("holds a PR that carries the live manual-review label, even with no guardrail hit", () => { + expect(facts({ unifiedFiles: [UNGUARDED_FILE], prLabels: ["manual-review"] }).heldForReview).toBe(true); + }); + + it("matches the label case-insensitively", () => { + expect(facts({ unifiedFiles: [UNGUARDED_FILE], prLabels: ["Manual-Review"] }).heldForReview).toBe(true); + }); + + it("does not hold when the label is absent", () => { + expect(facts({ unifiedFiles: [UNGUARDED_FILE], prLabels: ["gittensor:bug"] }).heldForReview).toBe(false); + }); + + it("honours a repo-configured custom manual-review label name instead of the default", () => { + const settings = { + hardGuardrailGlobs: [], + hardGuardrailGlobsOverridesInvariants: false, + manualReviewLabel: "needs-maintainer", + } as Pick; + // The default "manual-review" label no longer matters once a custom name is configured. + expect(facts({ unifiedFiles: [UNGUARDED_FILE], settings, prLabels: ["manual-review"] }).heldForReview).toBe(false); + expect(facts({ unifiedFiles: [UNGUARDED_FILE], settings, prLabels: ["needs-maintainer"] }).heldForReview).toBe(true); + }); + + it("disables the label check entirely when manualReviewLabel is explicitly null", () => { + const settings = { + hardGuardrailGlobs: [], + hardGuardrailGlobsOverridesInvariants: false, + manualReviewLabel: null, + } as Pick; + expect(facts({ unifiedFiles: [UNGUARDED_FILE], settings, prLabels: ["manual-review"] }).heldForReview).toBe(false); + }); +}); + describe("derivePublicCommentMergeFacts() — neverClosed (#8/#9, #4607)", () => { it("is true for the repo owner, case-insensitively", () => { expect(facts({ repoFullName: "acme/widgets", authorLogin: "acme" }).neverClosed).toBe(true);