diff --git a/migrations/0208_manual_review_label_provenance.sql b/migrations/0208_manual_review_label_provenance.sql new file mode 100644 index 0000000000..a94a9cd14a --- /dev/null +++ b/migrations/0208_manual_review_label_provenance.sql @@ -0,0 +1,19 @@ +-- #9939: who applied the manual-review label, so the bot can lift what the bot applied. +-- +-- The label is OVERLOADED: it is both the planner's own "held for a human" disposition and a maintainer's +-- manual safety freeze. agent-actions.ts therefore refuses to auto-remove it while it clears every sibling +-- disposition label, because it has no way to tell the two apart -- and auto-removing a human's deliberate +-- freeze would be far worse than leaving a stale one. That caution is correct and stays. +-- +-- The cost of having no provenance is a one-way latch. Observed on #9935: applied when a finding fired +-- against a stale head, then never lifted after a rebase removed the cause and a fresh escalated review +-- returned zero findings. The PR was mergeable, green and clean, and still refused to merge until a human +-- removed the label by hand -- in the mode that is meant to have no human in it. +-- +-- Recording the head SHA the PLANNER applied it at (and the reason it applied it FOR) makes the distinction +-- decidable: bot-applied labels become liftable once that specific reason clears, while a label with no +-- provenance row -- a human's, or one applied before this column existed -- keeps exactly today's behaviour +-- and is never touched automatically. Nullable with no backfill for precisely that reason: absence means +-- "not ours to lift", which is the safe reading for every pre-existing label. +ALTER TABLE pull_requests ADD COLUMN manual_review_label_applied_sha TEXT; +ALTER TABLE pull_requests ADD COLUMN manual_review_label_applied_reason TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index ca76bc48a5..1232119e57 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4731,6 +4731,36 @@ export async function markPullRequestVisualCaptureUnobtainable(env: Env, fullNam .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); } +/** #9939: record that the PLANNER applied the manual-review label at `headSha`, for `reason`. + * + * This is the provenance that makes the label liftable. The same label string is ALSO how a maintainer + * freezes a PR by hand, and agent-actions.ts rightly refuses to auto-remove it without knowing which it is + * looking at -- silently undoing a human's freeze is far worse than leaving a stale hold. Written only on + * the planner's own add, so a human-applied label never acquires provenance and is never auto-removed. + * + * Scoped to headSha like its visual-capture siblings: a new commit is a new decision, and a hold recorded + * against an older head should not license removing a label on a head nobody has re-evaluated. */ +export async function markPullRequestManualReviewLabelApplied(env: Env, fullName: string, number: number, headSha: string, reason: string): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ manualReviewLabelAppliedSha: headSha, manualReviewLabelAppliedReason: boundedString(reason, 300), updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); +} + +/** #9939: clear the provenance once the label is gone (removed by the planner, or by a maintainer). + * + * Leaving it behind would let a LATER human-applied label inherit the bot's provenance and become + * auto-removable -- exactly the override this whole mechanism exists to prevent. Not scoped to headSha: the + * label is gone regardless of which head it was applied at. */ +export async function clearPullRequestManualReviewLabelProvenance(env: Env, fullName: string, number: number): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ manualReviewLabelAppliedSha: null, manualReviewLabelAppliedReason: null, updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number))); +} + /** Release the retry latch: the retry chain that justified it has ended without a successful capture, so the * screenshotTableGate must stop deferring and evaluate the evidence actually present. * @@ -7441,6 +7471,8 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha, visualCaptureRetryPendingSha: row.visualCaptureRetryPendingSha, visualCaptureRetryPendingAt: row.visualCaptureRetryPendingAt, + manualReviewLabelAppliedSha: row.manualReviewLabelAppliedSha, + manualReviewLabelAppliedReason: row.manualReviewLabelAppliedReason, screenshotTablePresenceSatisfied: parseJson<{ headSha: string; evidenceFingerprint: string } | null>(row.screenshotTablePresenceSatisfiedJson, null), }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index de8902da4b..439a891338 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -494,6 +494,12 @@ export const pullRequests = sqliteTable( // a sha with no timestamp reads as EXPIRED, which is both honest (the row predates the column) and what // releases the PRs already stuck behind one. loopover-computed, written with the sha, cleared with it. visualCaptureRetryPendingAt: text("visual_capture_retry_pending_at"), + // #9939: provenance for the manual-review label -- the head SHA the PLANNER applied it at, and the hold + // reason it applied it FOR. The label is overloaded (bot disposition AND maintainer freeze), so without + // this the planner cannot lift its own stale hold without risking overriding a human's. NULL means "not + // ours" -- a human-applied label, or one predating this column -- and is never auto-removed. + manualReviewLabelAppliedSha: text("manual_review_label_applied_sha"), + manualReviewLabelAppliedReason: text("manual_review_label_applied_reason"), // #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable for this repo -- // the deployments read succeeded, found none at all, and the poll budget is spent. The screenshot-table // gate degrades its CLOSE to advisory for that head rather than closing a PR over evidence no diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e93cac82d9..ddf2ed76dd 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2877,6 +2877,9 @@ function buildAgentMaintenancePlanInput(args: { guardrailEscalationModel: settings.guardrailEscalationModel ?? null, guardrailEscalationProvider: settings.guardrailEscalationProvider ?? null, manualReviewLabel: settings.manualReviewLabel, + // #9939: provenance for the label above -- non-null only when the PLANNER applied it, which is what + // lets a later pass lift its own stale hold without ever touching a maintainer's manual freeze. + manualReviewLabelAppliedSha: pr.manualReviewLabelAppliedSha, readyToMergeLabel: settings.readyToMergeLabel, changesRequestedLabel: settings.changesRequestedLabel, migrationCollisionLabel: settings.migrationCollisionLabel, diff --git a/src/review/merge-train.ts b/src/review/merge-train.ts index 95666ff9d6..c14eea9ba2 100644 --- a/src/review/merge-train.ts +++ b/src/review/merge-train.ts @@ -49,6 +49,18 @@ export type MergeTrainSibling = { * processors.ts) rather than inventing a second, possibly-drifting definition here. Absent/undefined ⇒ not * held (unchanged from before this field existed). */ heldForManualReview?: boolean | undefined; + /** True when this sibling is a DRAFT (#9939). The strongest possible "not trying to merge" signal there is: + * GitHub itself refuses to merge a draft, so a draft sibling is not one merge away from landing, it is one + * AUTHOR ACTION away from even being eligible. Same reasoning that evicted the manual-review hold above, + * only more so -- a manual-review hold at least describes a PR someone might unblock, while a draft is the + * author's own declaration that it is not ready. + * + * This is the head-of-line case that hurt in production: a maintainer PR with red CI cannot be auto-closed + * (maintainers are exempt, deliberately, so they can iterate), so it stays open and overlapping for as long + * as the fix takes -- holding every newer overlapping PR behind it for up to the full 24h cap even when + * those are green and approved. Marking it draft now says "skip me" in a way the train understands. + * Absent/undefined ⇒ not a draft (unchanged from before this field existed). */ + isDraft?: boolean | undefined; }; /** How long an older, OVERLAPPING sibling can hold up a newer one before it's excluded from blocking (24 @@ -126,6 +138,10 @@ export function shouldWaitForOlderSiblings(input: ShouldWaitForOlderSiblingsInpu .filter((sibling) => sibling.number !== thisPrNumber) .filter((sibling) => sibling.mergeableState !== "dirty") .filter((sibling) => !sibling.heldForManualReview) + // #9939: a draft is not trying to reach merge -- GitHub will not merge one at all. Evicted outright + // rather than merely capped, exactly like the manual-review hold above: waiting on a PR that cannot + // merge buys nothing, and the 24h cap is far too long to be the only bound on it. + .filter((sibling) => !sibling.isDraft && sibling.mergeableState !== "draft") .filter((sibling) => isOlder(sibling)) .filter((sibling) => overlaps(thisPrLinkedIssues, thisPrChangedFiles, sibling)) .filter((sibling) => { diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index af7e862fa9..804251f003 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -32,6 +32,7 @@ import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy"; import { boundStructuredCloseReasonsForPersistence, buildAgentActionAudit, formatAgentPermissionDenial, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness, type AgentActionMode } from "../settings/agent-execution"; import { AGENT_LABEL_NEEDS_REVIEW, type PlannedAgentAction } from "../settings/agent-actions"; import type { AgentActionClass, AgentPendingActionParams, AutonomyLevel, AutonomyPolicy } from "../types"; +import { clearPullRequestManualReviewLabelProvenance, markPullRequestManualReviewLabelApplied } from "../db/repositories"; import { errorMessage } from "../utils/json"; import { MODERATION_VIOLATION_EVENT_TYPE, @@ -697,6 +698,10 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE linkedIssues: sibling.linkedIssues, changedFiles: pathsByPullNumber.get(sibling.number), heldForManualReview: manualReviewLabel !== null && sibling.labels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase()), + // #9939: a draft sibling never blocks -- GitHub will not merge it, so it is not "about to land". + // `?? false` rather than passing the nullable straight through: the field is optional on the record + // and the gate treats absent as "not a draft", so normalising here keeps the two readings identical. + isDraft: sibling.isDraft ?? false, })), nowMs: Date.now(), }); @@ -1287,8 +1292,21 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: // optional comment (the Pass-1 flag warning, or the resolved note) posted alongside the label mutation. if (action.labelOp === "remove") { await removePullRequestLabel(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.label ?? ""); + // #9939: the label is gone, so its provenance must go with it. Leaving a stale marker behind would let + // a LATER human-applied label inherit the bot's provenance and become auto-removable -- exactly the + // override the provenance exists to prevent. Best-effort: a failed clear only means the next pass + // re-evaluates with a marker for a label that is not there, which the `hasLabel` guard already ignores. + if (ctx.manualReviewLabel && action.label === ctx.manualReviewLabel) { + await clearPullRequestManualReviewLabelProvenance(env, ctx.repoFullName, ctx.pullNumber).catch(() => {}); + } } else { await ensurePullRequestLabel(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.label ?? "", { createMissingLabel: true }); + // #9939: record that the PLANNER (not a maintainer) applied this hold, so a later pass may lift it + // once nothing wants it. Written only here, on the bot's own add, which is what keeps a + // human-applied label provenance-free and therefore untouchable. + if (ctx.manualReviewLabel && action.label === ctx.manualReviewLabel && ctx.headSha) { + await markPullRequestManualReviewLabelApplied(env, ctx.repoFullName, ctx.pullNumber, ctx.headSha, action.reason).catch(() => {}); + } } if (action.comment) await createIssueComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.comment); return; diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 40f5dacd03..08c46226c7 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -286,6 +286,11 @@ export type AgentActionPlanInput = { // Configured manual-review hold label. Undefined uses the default "manual-review"; null disables only the // label, not the guardrail hold. Separate from review_state_label so operators can avoid ready/changes labels. manualReviewLabel?: string | null | undefined; + /** #9939: the head SHA at which the PLANNER previously applied `manualReviewLabel`, or null/absent when the + * bot did not apply it. This is the provenance that makes the label liftable: the same label string is + * also how a maintainer freezes a PR by hand, so without it the planner cannot remove its own stale hold + * without risking silently undoing a human's. Absent ⇒ never auto-removed. */ + manualReviewLabelAppliedSha?: string | null | undefined; // Optional disposition label overrides. Undefined uses generic defaults; null disables that specific label. readyToMergeLabel?: string | null | undefined; changesRequestedLabel?: string | null | undefined; @@ -1271,6 +1276,40 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne }); } + // 1b) #9939: LIFT a manual-review hold the planner itself applied, once nothing wants it any more. + // + // The sibling-label cleanup further down deliberately refuses to touch this label, because the same string + // is also a maintainer's manual freeze and that cleanup has no way to tell them apart. Correct -- but it + // made the label a ONE-WAY LATCH: applied on a pass where a hold was real, and never lifted once the hold + // cleared. Observed on #9935, which was mergeable, green, and re-reviewed to zero findings, and still + // refused to merge until a human removed the label by hand. + // + // `manualReviewLabelAppliedSha` is the missing provenance. Non-null means the PLANNER applied it, so the + // planner may take it back; null means a human applied it (or it predates the column) and it is left + // strictly alone. Guarded on `manualHoldReason === null`, i.e. NO reason wants a hold this pass -- which is + // why this cannot lift a label applied for reason A just because reason B cleared. Deliberately not scoped + // to the recorded head: a rebase that resolves the cause is the single most common way a hold goes stale, + // and refusing to lift it there would leave the exact #9935 case unfixed. + if ( + manualHoldReason === null && + labels.manualReview !== null && + input.manualReviewLabelAppliedSha != null && + hasLabel(input.pr.labels, labels.manualReview) + // No "is an add already planned this pass?" guard: every site that ADDS this label requires a live hold + // (a guardrail hit, the migration-collision fallback, the owner/automation fallback), and all of those + // set manualHoldReason -- so the null check above already excludes every case where an add could be in + // flight. A guard here would be an arm no input can reach. + ) { + actions.push({ + actionClass: "label", + autonomyClass: "merge", + requiresApproval: approval("merge"), + reason: `manual-review hold resolved — clearing the "${labels.manualReview}" label the bot applied`, + label: labels.manualReview, + labelOp: "remove", + }); + } + // 1c) migration-collision manual-review fallback (#manual-review-coverage) — the migration-collision LABEL // itself stays gated on review_state_label below (section 2, unchanged) so an operator's dedicated filter on // that specific label is undisturbed. But when review_state_label is OFF (a one-shot repo that only configures diff --git a/src/types.ts b/src/types.ts index cb88c93c06..938ee17517 100644 --- a/src/types.ts +++ b/src/types.ts @@ -777,6 +777,13 @@ export type PullRequestRecord = { * review/visual/visual-capture-retry-latch.ts for why a sha with no timestamp reads as expired rather than * live. Publish-written alongside the sha; read straight from the row. */ visualCaptureRetryPendingAt?: string | null | undefined; + /** #9939: the head SHA at which the PLANNER applied the manual-review label, and the hold reason it applied + * it for. Together they are the provenance that lets the planner lift its OWN stale hold without ever + * touching a maintainer's manual freeze -- the label is the same string for both. Absent ⇒ not the bot's + * to remove (a human applied it, or it predates this), which is the safe default for every existing label. + * Planner-written; read straight from the row. */ + manualReviewLabelAppliedSha?: string | null | undefined; + manualReviewLabelAppliedReason?: string | null | undefined; /** Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix): the (headSha, * evidenceFingerprint) checkpoint the screenshotTableGate's presence-mode check last satisfied for this PR * (see evaluateScreenshotTableGate's staleness comment). `null`/absent = presence mode has never satisfied diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index e7ad47e0cf..273bc988bb 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -85,7 +85,7 @@ import type { PlannedAgentAction } from "../../src/settings/agent-actions"; import type { DecisionRecord } from "../../src/review/decision-record"; import { STRUCTURED_CLOSE_REASONS_MAX_COUNT } from "../../src/settings/agent-execution"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; -import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, isGlobalAgentFrozen, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFile, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, getPullRequest, isGlobalAgentFrozen, markPullRequestManualReviewLabelApplied, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFile, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import * as repositoriesModule from "../../src/db/repositories"; import * as posthogModule from "../../src/selfhost/posthog"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; @@ -124,6 +124,62 @@ async function auditFor(env: Env, actionClass: string): Promise<{ outcome: strin return env.DB.prepare("select outcome, metadata_json from audit_events where event_type = ? order by created_at desc limit 1").bind(`agent.action.${actionClass}`).first(); } +describe("manual-review label provenance writes (#9939)", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(fetchPullRequestFreshness).mockImplementation(async (_env, args) => ({ + status: "current", + liveHeadSha: args.expectedHeadSha ?? null, + liveState: "open", + liveLabels: [], + })); + clearInstallationHealthRefreshCooldownForTest(); + clearWritePermissionDenialCooldownForTest(); + resetMetrics(); + }); + + const seedPr = async (env: Env) => { + await upsertRepositoryFromGitHub(env, { full_name: "owner/repo", name: "repo", id: 1, private: false } as never, 123); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 7, title: "t", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], created_at: "2026-07-05T10:00:00.000Z", + } as never); + }; + + it("records provenance when the BOT adds the manual-review label", async () => { + const env = createTestEnv(); + await seedPr(env); + const add: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "verdict=success; guarded path", label: "human-review", labelOp: "add" }; + await executeAgentMaintenanceActions(env, { ...ctx(), manualReviewLabel: "human-review" }, [add]); + const stored = await getPullRequest(env, "owner/repo", 7); + expect(stored?.manualReviewLabelAppliedSha).toBe("sha7"); + expect(stored?.manualReviewLabelAppliedReason).toBe("verdict=success; guarded path"); + }); + + it("INVARIANT: writes NOTHING for any other label -- only the manual-review hold is the bot's to take back", async () => { + const env = createTestEnv(); + await seedPr(env); + await executeAgentMaintenanceActions(env, { ...ctx(), manualReviewLabel: "human-review" }, [label]); + expect((await getPullRequest(env, "owner/repo", 7))?.manualReviewLabelAppliedSha).toBeNull(); + }); + + it("clears provenance when the label is removed, so a later human-applied one cannot inherit it", async () => { + const env = createTestEnv(); + await seedPr(env); + await markPullRequestManualReviewLabelApplied(env, "owner/repo", 7, "sha7", "why"); + const remove: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "resolved", label: "human-review", labelOp: "remove" }; + await executeAgentMaintenanceActions(env, { ...ctx(), manualReviewLabel: "human-review" }, [remove]); + expect((await getPullRequest(env, "owner/repo", 7))?.manualReviewLabelAppliedSha).toBeNull(); + }); + + it("INVARIANT: a repo with the label disabled never acquires provenance", async () => { + const env = createTestEnv(); + await seedPr(env); + const add: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "r", label: "human-review", labelOp: "add" }; + await executeAgentMaintenanceActions(env, { ...ctx(), manualReviewLabel: null }, [add]); + expect((await getPullRequest(env, "owner/repo", 7))?.manualReviewLabelAppliedSha).toBeNull(); + }); +}); + describe("executeAgentMaintenanceActions (#778 gate stack)", () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 7f720df045..36dafd4add 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -66,6 +66,58 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(collision)).not.toContain("merge"); }); + // #9939: the manual-review label was a ONE-WAY LATCH. The sibling-label cleanup deliberately refuses to + // touch it (the same string is also a maintainer's manual freeze), so once applied nothing lifted it when + // the hold cleared. Live on #9935: mergeable, green, re-reviewed to zero findings, and still refused to + // merge until a human removed the label by hand -- in the mode that is meant to have no human in it. + describe("manual-review label provenance (#9939)", () => { + const healthy = { + conclusion: "success" as const, + autonomy: { merge: "auto" as const }, + manualReviewLabel: "human-review", + pr: { labels: ["human-review"], mergeableState: "clean" as const, reviewDecision: "APPROVED" as const }, + }; + + it("REGRESSION: lifts a hold the BOT applied once nothing wants it any more", () => { + const plan = planAgentMaintenanceActions(input({ ...healthy, manualReviewLabelAppliedSha: "abc123" })); + expect(plan.some((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp === "remove")).toBe(true); + }); + + it("INVARIANT: never touches a label with NO provenance -- a maintainer's freeze is not the bot's to lift", () => { + // The load-bearing safety property. Silently undoing a human's deliberate hold is far worse than + // leaving a stale one, which is exactly why the cleanup refused to act before provenance existed. + const plan = planAgentMaintenanceActions(input(healthy)); + expect(plan.some((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp === "remove")).toBe(false); + }); + + it("INVARIANT: does not lift while a hold reason STILL wants the label", () => { + // Guarded on manualHoldReason === null, so a label applied for reason A cannot be lifted by reason B + // clearing. Here a guardrail hit still holds the PR, with the bot's own provenance present. + const plan = planAgentMaintenanceActions( + input({ + ...healthy, + manualReviewLabelAppliedSha: "abc123", + changedPaths: ["src/settings/agent-actions.ts"], + hardGuardrailGlobs: ["src/settings/**"], + }), + ); + expect(plan.some((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp === "remove")).toBe(false); + }); + + it("INVARIANT: no removal when the label is not actually on the PR", () => { + const plan = planAgentMaintenanceActions(input({ ...healthy, manualReviewLabelAppliedSha: "abc123", pr: { ...healthy.pr, labels: [] } })); + expect(plan.some((a) => a.actionClass === "label" && a.labelOp === "remove")).toBe(false); + }); + + it("INVARIANT: lifting is authorized by MERGE autonomy, the same class as the add it reverses", () => { + // Removing a hold is as consequential as applying one -- it is what lets the merge proceed -- so it + // must not ride on a weaker autonomy class than the add. + const plan = planAgentMaintenanceActions(input({ ...healthy, manualReviewLabelAppliedSha: "abc123" })); + const lift = plan.find((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp === "remove"); + expect(lift?.autonomyClass).toBe("merge"); + }); + }); + // #9808/#9869: a guardrail hit used to mean ONE thing -- suppress auto-merge and hold for a human -- while // buying no extra scrutiny at all. With an escalation configured, a CLEAN escalated review can now release // that hold instead. These two cases are the whole point of the feature and the only place all three diff --git a/test/unit/db-persistence.test.ts b/test/unit/db-persistence.test.ts index cccee97188..c6897a27f5 100644 --- a/test/unit/db-persistence.test.ts +++ b/test/unit/db-persistence.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from "vitest"; import { + getPullRequest, + markPullRequestManualReviewLabelApplied, + clearPullRequestManualReviewLabelProvenance, getActiveReviewStartedAt, getBounty, getContributorScoringProfile, @@ -605,6 +608,50 @@ describe("active-review tracking (#review-evasion-protection)", () => { }); }); +describe("manual-review label provenance (#9939)", () => { + const seed = async (env: ReturnType) => { + await upsertRepositoryFromGitHub(env, { full_name: "owner/repo", name: "repo", id: 1, private: false } as never, 4242); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 11, title: "t", state: "open", user: { login: "c" }, head: { sha: "head1" }, labels: [], created_at: "2026-07-05T10:00:00.000Z", + } as never); + }; + + it("records the head and reason the PLANNER applied the label at", async () => { + const env = createTestEnv(); + await seed(env); + await markPullRequestManualReviewLabelApplied(env, "owner/repo", 11, "head1", "verdict=success; guarded path"); + const stored = await getPullRequest(env, "owner/repo", 11); + expect(stored?.manualReviewLabelAppliedSha).toBe("head1"); + expect(stored?.manualReviewLabelAppliedReason).toBe("verdict=success; guarded path"); + }); + + it("INVARIANT: the write is head-scoped -- a stale head does not stamp provenance", async () => { + // A hold recorded against an older head must not license removing a label on a head nobody re-evaluated. + const env = createTestEnv(); + await seed(env); + await markPullRequestManualReviewLabelApplied(env, "owner/repo", 11, "some-other-head", "stale"); + expect((await getPullRequest(env, "owner/repo", 11))?.manualReviewLabelAppliedSha).toBeNull(); + }); + + it("REGRESSION: clearing removes BOTH fields, so a later human-applied label cannot inherit the bot's provenance", async () => { + // The subtle failure this prevents: leftover provenance would make a maintainer's own freeze look + // bot-applied on the next pass, and therefore auto-removable -- the exact override it exists to prevent. + const env = createTestEnv(); + await seed(env); + await markPullRequestManualReviewLabelApplied(env, "owner/repo", 11, "head1", "why"); + await clearPullRequestManualReviewLabelProvenance(env, "owner/repo", 11); + const stored = await getPullRequest(env, "owner/repo", 11); + expect(stored?.manualReviewLabelAppliedSha).toBeNull(); + expect(stored?.manualReviewLabelAppliedReason).toBeNull(); + }); + + it("INVARIANT: a PR with no provenance reads as null -- absent means \"not ours to lift\"", async () => { + const env = createTestEnv(); + await seed(env); + expect((await getPullRequest(env, "owner/repo", 11))?.manualReviewLabelAppliedSha).toBeNull(); + }); +}); + describe("loadOrphanRequeueContext (#9870)", () => { it("returns the installation id AND the PR's real creation time", async () => { // The boot sweep heals the tracking row, but re-queueing the pass needs both — and the tracking row diff --git a/test/unit/merge-train.test.ts b/test/unit/merge-train.test.ts index ca7b97799d..9f65e25b17 100644 --- a/test/unit/merge-train.test.ts +++ b/test/unit/merge-train.test.ts @@ -177,6 +177,44 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { }); }); + describe("draft eviction (#9939 -- head-of-line blocking by an un-mergeable maintainer PR)", () => { + // The production shape: a maintainer PR with red CI cannot be auto-closed (maintainers are exempt on + // purpose, so they can iterate), so it stays open and overlapping for as long as the fix takes. Every + // newer overlapping PR queued behind it -- green, approved, ready -- waited on a PR that was not trying + // to merge at all. Marking it draft is the author saying "skip me"; the train now understands that. + it("REGRESSION: an older, OVERLAPPING draft sibling does NOT block -- GitHub will not merge a draft at all", () => { + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], isDraft: true }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + }); + + it("also evicts on GitHub's own mergeableState of \"draft\", for a caller that only resolved that field", () => { + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], mergeableState: "draft" }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + }); + + it("isDraft: false (or absent/undefined) leaves the OVERLAPPING older sibling blocking exactly as before", () => { + const explicit = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], isDraft: false }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", explicit, NOW)).toEqual({ wait: true, blockingPr: 105 }); + const absent: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1] }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", absent, NOW)).toEqual({ wait: true, blockingPr: 105 }); + }); + + it("a draft sibling is skipped over in favor of the next-oldest still-viable overlapping sibling", () => { + // Eviction removes ONE PR from the queue -- it does not disable the gate. A real older sibling behind + // the draft still holds the line, which is what keeps this a fix rather than a bypass. + const siblings: MergeTrainSibling[] = [ + { number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], isDraft: true }, + { number: 107, createdAt: "2026-07-07T10:30:00.000Z", linkedIssues: [1] }, + ]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 107 }); + }); + + it("a draft that is ALSO manual-review-held is evicted once, not twice -- the two rules compose", () => { + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], isDraft: true, heldForManualReview: true }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + }); + }); + describe("manual-review eviction (#9039 -- confirmed #8735 incident)", () => { it("REGRESSION: an older, OVERLAPPING sibling held for manual review does NOT block -- it is evicted, not waited for", () => { const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], heldForManualReview: true }];