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
24 changes: 19 additions & 5 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
*/
Expand All @@ -2019,9 +2027,10 @@ export function derivePublicCommentMergeFacts(args: {
mergeableState: string | null | undefined;
authorLogin: string | null | undefined;
liveCi: Pick<LiveCiAggregate, "ciState" | "failingDetails" | "nonRequiredFailingDetails">;
settings: Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants">;
settings: Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
unifiedFiles: Awaited<ReturnType<typeof listPullRequestFiles>>;
repoFullName: string;
prLabels: readonly string[];
}): PublicCommentMergeFacts {
const mergeStateLabel = args.liveMergeState ?? args.mergeableState ?? undefined; // fail-safe to the stored value
const ciState: MergeReadiness["ciState"] =
Expand All @@ -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 =
Expand Down Expand Up @@ -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;
Expand Down
52 changes: 47 additions & 5 deletions test/unit/processors-public-comment-merge-facts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ const UNGUARDED_FILE = { path: "README.md" } as PullRequestFileRecord;
const NO_GUARDRAIL_OVERRIDES = {
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: false,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants">;
manualReviewLabel: undefined,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;

function facts(overrides: Partial<Parameters<typeof derivePublicCommentMergeFacts>[0]> = {}) {
return derivePublicCommentMergeFacts({
Expand All @@ -22,6 +23,7 @@ function facts(overrides: Partial<Parameters<typeof derivePublicCommentMergeFact
settings: NO_GUARDRAIL_OVERRIDES,
unifiedFiles: [UNGUARDED_FILE],
repoFullName: "acme/widgets",
prLabels: [],
...overrides,
});
}
Expand Down Expand Up @@ -112,15 +114,55 @@ describe("derivePublicCommentMergeFacts() — heldForReview (#guarded-hold-comme
expect(
facts({
unifiedFiles: [GUARDED_FILE],
settings: { hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: true } as Pick<
RepositorySettings,
"hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants"
>,
settings: {
hardGuardrailGlobs: [],
hardGuardrailGlobsOverridesInvariants: true,
manualReviewLabel: undefined,
} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">,
}).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<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
// 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<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants" | "manualReviewLabel">;
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);
Expand Down