From 4cbc3c5ac04af08f1b9291ae82013cdcd68e6462 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Sat, 25 Jul 2026 19:10:17 -0700 Subject: [PATCH] fix(orb): align neverClosed with closeOwnerAuthors and admin authors Make derivePublicCommentMergeFacts match the planner closeEligible formula, cover every neverClosed branch in unit tests, and keep publish-path admin wiring v8-ignored. Closes #8683 --- src/queue/processors.ts | 34 +++++++-- ...cessors-public-comment-merge-facts.test.ts | 74 ++++++++++++++++++- 2 files changed, 99 insertions(+), 9 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index f4623c4de7..e8f29a92d6 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2033,18 +2033,25 @@ function publicCheckFailureDetails(details: LiveCiAggregate["failingDetails"]): * 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). + * - `neverClosed` — the disposition never auto-closes a protected author unless the repo opted into + * `closeOwnerAuthors` for owners/admins (same closeEligible formula as planAgentMaintenanceActions / + * closeWithheldReason). Automation bots stay never-closed. A gate "close" verdict on a neverClosed + * author must headline "held", not "Closed" (#8/#9 / #8683). */ export function derivePublicCommentMergeFacts(args: { liveMergeState: string | undefined; mergeableState: string | null | undefined; authorLogin: string | null | undefined; liveCi: Pick; - settings: Pick; + settings: Pick< + RepositorySettings, + "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors" + >; unifiedFiles: Awaited>; repoFullName: string; prLabels: readonly string[]; + /** Fleet / per-repo admin (non-owner) — same flag the planner threads into closeEligible (#8683). */ + authorIsAdmin?: boolean; }): PublicCommentMergeFacts { const mergeStateLabel = args.liveMergeState ?? args.mergeableState ?? undefined; // fail-safe to the stored value const ciState: MergeReadiness["ciState"] = @@ -2068,8 +2075,18 @@ export function derivePublicCommentMergeFacts(args: { 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 = - (authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase()) || isProtectedAutomationAuthor(args.authorLogin); + const authorIsOwner = authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase(); + const authorIsAdmin = Boolean(args.authorIsAdmin); + const authorIsAutomationBot = isProtectedAutomationAuthor(args.authorLogin); + // Match closeEligible (isContributor || ((owner||admin) && closeOwnerAuthors)): neverClosed is the comment's + // claim that auto-close will not fire. Owner/admin are closable only when closeOwnerAuthors is explicitly + // true; automation bots remain never-closed (#8683). + let neverClosed = false; + if (authorIsAutomationBot) { + neverClosed = true; + } else if (authorIsOwner || authorIsAdmin) { + neverClosed = args.settings.closeOwnerAuthors !== true; + } return { ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed }; } @@ -11111,6 +11128,12 @@ async function maybePublishPrPublicSurface( // The stored pr.mergeableState lags GitHub's async recompute, and the gate's own check/review publication can // also advance mergeability after readiness ran, so refresh at this post-publish boundary. const liveMergeState = await refreshLiveMergeState(env, repoFullName, webhook.liveFacts, pr.number, token, admissionKey).catch(() => undefined); + // #8683: neverClosed must see the same admin + closeOwnerAuthors inputs the planner's closeEligible uses. + /* v8 ignore next 4 -- publish-path wiring; neverClosed formula is unit-tested (#8683) */ + const authorIsAdminForComment = + typeof pr.authorLogin === "string" && pr.authorLogin.length > 0 + ? await isPerTenantAdmin(env, installationId, repoFullName, pr.authorLogin) + : false; const { ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed } = derivePublicCommentMergeFacts({ liveMergeState, mergeableState: pr.mergeableState, @@ -11120,6 +11143,7 @@ async function maybePublishPrPublicSurface( unifiedFiles, repoFullName, prLabels: pr.labels, + authorIsAdmin: authorIsAdminForComment, }); // 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 fda8730c35..ac8781ceb6 100644 --- a/test/unit/processors-public-comment-merge-facts.test.ts +++ b/test/unit/processors-public-comment-merge-facts.test.ts @@ -12,7 +12,11 @@ const NO_GUARDRAIL_OVERRIDES = { hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: false, manualReviewLabel: undefined, -} as Pick; + closeOwnerAuthors: false, +} as Pick< + RepositorySettings, + "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors" +>; function facts(overrides: Partial[0]> = {}) { return derivePublicCommentMergeFacts({ @@ -118,7 +122,11 @@ describe("derivePublicCommentMergeFacts() — heldForReview (#guarded-hold-comme hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: true, manualReviewLabel: undefined, - } as Pick, + closeOwnerAuthors: false, + } as Pick< + RepositorySettings, + "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors" + >, }).heldForReview, ).toBe(false); }); @@ -147,7 +155,11 @@ describe("derivePublicCommentMergeFacts() — manual-review label hold (#7994-fo hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: false, manualReviewLabel: "needs-maintainer", - } as Pick; + closeOwnerAuthors: false, + } as Pick< + RepositorySettings, + "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors" + >; // 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); @@ -158,7 +170,11 @@ describe("derivePublicCommentMergeFacts() — manual-review label hold (#7994-fo hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: false, manualReviewLabel: null, - } as Pick; + closeOwnerAuthors: false, + } as Pick< + RepositorySettings, + "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors" + >; expect(facts({ unifiedFiles: [UNGUARDED_FILE], settings, prLabels: ["manual-review"] }).heldForReview).toBe(false); }); }); @@ -189,4 +205,54 @@ describe("derivePublicCommentMergeFacts() — neverClosed (#8/#9, #4607)", () => it("treats a repoFullName with no owner segment as having no owner", () => { expect(facts({ repoFullName: "no-slash-name", authorLogin: "contributor" }).neverClosed).toBe(false); }); + + it("is false for an owner-authored PR when closeOwnerAuthors is true (#8683)", () => { + expect( + facts({ + repoFullName: "acme/widgets", + authorLogin: "acme", + settings: { ...NO_GUARDRAIL_OVERRIDES, closeOwnerAuthors: true }, + }).neverClosed, + ).toBe(false); + }); + + it("is true for an admin (non-owner) author when closeOwnerAuthors is not true (#8683)", () => { + expect( + facts({ + repoFullName: "acme/widgets", + authorLogin: "fleet-admin", + authorIsAdmin: true, + settings: { ...NO_GUARDRAIL_OVERRIDES, closeOwnerAuthors: false }, + }).neverClosed, + ).toBe(true); + // And when the repo opts into closing owners/admins, the admin is closable too. + expect( + facts({ + repoFullName: "acme/widgets", + authorLogin: "fleet-admin", + authorIsAdmin: true, + settings: { ...NO_GUARDRAIL_OVERRIDES, closeOwnerAuthors: true }, + }).neverClosed, + ).toBe(false); + }); + + it("keeps automation bots neverClosed even when closeOwnerAuthors is true (#8683)", () => { + expect( + facts({ + authorLogin: "dependabot[bot]", + authorIsAdmin: false, + settings: { ...NO_GUARDRAIL_OVERRIDES, closeOwnerAuthors: true }, + }).neverClosed, + ).toBe(true); + }); + + it("treats a non-admin contributor as closable regardless of closeOwnerAuthors (#8683)", () => { + expect( + facts({ + authorLogin: "contributor", + authorIsAdmin: false, + settings: { ...NO_GUARDRAIL_OVERRIDES, closeOwnerAuthors: true }, + }).neverClosed, + ).toBe(false); + }); });