From 8d7d184efa4feda56efce831256e7f85d98f2941 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:56:51 -0700 Subject: [PATCH] fix(gate): lift the bot's own manual-review hold, and stop drafts wedging the merge train Two independent reasons an autonomous queue stalls with no human in it, both found on live PRs. 1) The manual-review label was a ONE-WAY LATCH. The sibling-label cleanup deliberately refuses to touch it, because the same label string is also a maintainer's manual freeze and the planner had no way to tell them apart -- and silently undoing a human's freeze is far worse than leaving a stale hold. That caution was right; the missing piece was provenance. Seen on #9935: the label went on when a finding fired against a stale head, a rebase removed the cause, a fresh escalated review returned ZERO findings, and the PR was mergeable, green and clean -- and still would not merge: agent.action.merge | denied | manual-review label "manual-review" is present on the live PR -- merge not executed It took a human removing the label by hand, in the mode that is supposed to have no human in it. The planner now records the head SHA and reason when IT applies the label, and may lift it once manualHoldReason is null (no reason wants a hold this pass, so a label applied for reason A can never be lifted by reason B clearing). A label with NO provenance -- a human's, or one predating this -- is never touched. That asymmetry is the whole safety property. 2) A draft sibling wedged the merge train. The train already evicts conflicted and manual-review-held siblings on the principle that waiting for a PR which is not trying to reach merge buys nothing the 24h cap does not already bound far worse. A DRAFT is the strongest form of that signal: GitHub itself refuses to merge one, so it is not one merge away from landing, it is one author action away from being eligible at all. 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 -- holding every newer overlapping PR behind it, green and approved, for up to the full 24 hours. Marking it draft is the author saying "skip me", and the train now understands that. Eviction removes ONE PR from the queue; it does not disable the gate. A real older sibling behind the draft still holds the line. --- .../0208_manual_review_label_provenance.sql | 19 ++++++ src/db/repositories.ts | 32 ++++++++++ src/db/schema.ts | 6 ++ src/queue/processors.ts | 3 + src/review/merge-train.ts | 16 +++++ src/services/agent-action-executor.ts | 18 ++++++ src/settings/agent-actions.ts | 39 +++++++++++++ src/types.ts | 7 +++ test/unit/agent-action-executor.test.ts | 58 ++++++++++++++++++- test/unit/agent-actions.test.ts | 52 +++++++++++++++++ test/unit/db-persistence.test.ts | 47 +++++++++++++++ test/unit/merge-train.test.ts | 38 ++++++++++++ 12 files changed, 334 insertions(+), 1 deletion(-) create mode 100644 migrations/0208_manual_review_label_provenance.sql 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 }];