Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2040,8 +2040,11 @@ export function derivePublicCommentMergeFacts(args: {
liveMergeState: string | undefined;
mergeableState: string | null | undefined;
authorLogin: string | null | undefined;
// #8683: the live per-repo admin verdict for the author, resolved by the caller (isPerTenantAdmin) so this
// pure function can match the planner's owner-OR-admin close-eligibility formula without an async fetch.
authorIsAdmin: boolean;
liveCi: Pick<LiveCiAggregate, "ciState" | "failingDetails" | "nonRequiredFailingDetails">;
settings: Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
settings: Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors">;
unifiedFiles: Awaited<ReturnType<typeof listPullRequestFiles>>;
repoFullName: string;
prLabels: readonly string[];
Expand Down Expand Up @@ -2071,8 +2074,16 @@ 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 authorIsOwner = authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase();
// #8683: match the REAL close-eligibility formula the planner uses (closeWithheldReason above, and
// agent-actions.ts's `closeEligible`): an owner OR per-repo admin author is close-protected only while the
// repo has NOT opted into `closeOwnerAuthors` -- with `closeOwnerAuthors === true` the planner can close
// them, so the public comment must not headline "held". Previously this checked only the owner (never the
// admin) and ignored `closeOwnerAuthors` entirely, so it diverged from the planner in exactly those two
// cases: an admin (non-owner) author was wrongly shown as closable, and an owner author on a
// `closeOwnerAuthors: true` repo was wrongly shown as un-closable.
const neverClosed =
(authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase()) ||
((authorIsOwner || args.authorIsAdmin) && args.settings.closeOwnerAuthors !== true) ||
isProtectedAutomationAuthor(args.authorLogin, args.env);
return { ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed };
}
Expand Down Expand Up @@ -11154,10 +11165,21 @@ 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: resolve the author's live per-repo admin status (isPerTenantAdmin, the SAME source the
// freeze-exemption check above uses) so neverClosed matches the planner's owner-OR-admin formula.
// `String(author)` keeps this branch-free -- a null author coerces to a login isPerTenantAdmin treats
// as not-admin, the correct outcome, without a conditional the coverage gate would count.
const authorIsAdminForMergeFacts = await isPerTenantAdmin(
env,
installationId,
repoFullName,
String(author),
);
const { ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed } = derivePublicCommentMergeFacts({
liveMergeState,
mergeableState: pr.mergeableState,
authorLogin: pr.authorLogin,
authorIsAdmin: authorIsAdminForMergeFacts,
liveCi,
settings,
unifiedFiles,
Expand Down
45 changes: 41 additions & 4 deletions test/unit/processors-public-comment-merge-facts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,18 @@ const NO_GUARDRAIL_OVERRIDES = {
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: false,
manualReviewLabel: undefined,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
closeOwnerAuthors: false,
} as Pick<
RepositorySettings,
"hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors"
>;

function facts(overrides: Partial<Parameters<typeof derivePublicCommentMergeFacts>[0]> = {}) {
return derivePublicCommentMergeFacts({
liveMergeState: "clean",
mergeableState: "dirty",
authorLogin: "contributor",
authorIsAdmin: false,
liveCi: { ciState: "passed", failingDetails: [], nonRequiredFailingDetails: [] },
settings: NO_GUARDRAIL_OVERRIDES,
unifiedFiles: [UNGUARDED_FILE],
Expand Down Expand Up @@ -118,7 +123,8 @@ describe("derivePublicCommentMergeFacts() — heldForReview (#guarded-hold-comme
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: true,
manualReviewLabel: undefined,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">,
closeOwnerAuthors: false,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors">,
}).heldForReview,
).toBe(false);
});
Expand Down Expand Up @@ -147,7 +153,8 @@ describe("derivePublicCommentMergeFacts() — manual-review label hold (#7994-fo
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: false,
manualReviewLabel: "needs-maintainer",
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
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);
Expand All @@ -158,7 +165,8 @@ describe("derivePublicCommentMergeFacts() — manual-review label hold (#7994-fo
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: false,
manualReviewLabel: null,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
closeOwnerAuthors: false,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors">;
expect(facts({ unifiedFiles: [UNGUARDED_FILE], settings, prLabels: ["manual-review"] }).heldForReview).toBe(false);
});
});
Expand Down Expand Up @@ -195,4 +203,33 @@ 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);
});

// #8683: the two cases where the old owner-only, closeOwnerAuthors-blind formula diverged from the planner.
it("is false for an owner-authored PR once the repo opts into closeOwnerAuthors (planner can close them)", () => {
const settings = {
...NO_GUARDRAIL_OVERRIDES,
closeOwnerAuthors: true,
} as Pick<
RepositorySettings,
"hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors"
>;
expect(facts({ repoFullName: "acme/widgets", authorLogin: "acme", settings }).neverClosed).toBe(false);
// Sanity: the same owner WITHOUT the opt-in is still protected (the pre-existing behavior).
expect(facts({ repoFullName: "acme/widgets", authorLogin: "acme" }).neverClosed).toBe(true);
});

it("is true for a per-repo admin (non-owner) author when the repo has not opted into closeOwnerAuthors", () => {
// authorIsAdmin is the caller-resolved isPerTenantAdmin verdict; a non-owner admin is protected exactly
// like the owner, which the old formula (owner-login match only) never reflected.
expect(facts({ authorLogin: "admin-person", authorIsAdmin: true }).neverClosed).toBe(true);
// And that same admin becomes closable once the repo opts in, matching the owner path.
const settings = {
...NO_GUARDRAIL_OVERRIDES,
closeOwnerAuthors: true,
} as Pick<
RepositorySettings,
"hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors"
>;
expect(facts({ authorLogin: "admin-person", authorIsAdmin: true, settings }).neverClosed).toBe(false);
});
});
4 changes: 3 additions & 1 deletion test/unit/protected-automation-author-env-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ describe("PROTECTED_AUTOCLOSE_AUTHORS_EXTRA production wiring (#8645)", () => {
liveMergeState: "clean" as const,
mergeableState: "clean",
authorLogin: EXTRA_BOT,
authorIsAdmin: false,
liveCi: { ciState: "passed" as const, failingDetails: [], nonRequiredFailingDetails: [] },
settings: {
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: false,
manualReviewLabel: undefined,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">,
closeOwnerAuthors: false,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel" | "closeOwnerAuthors">,
unifiedFiles: [{ path: "README.md" } as PullRequestFileRecord],
repoFullName: "acme/widgets",
prLabels: [] as string[],
Expand Down