From 01493873a71f714751b9fc7104a9061c126ce1a9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:26:25 -0700 Subject: [PATCH] feat(gate): add the block tier, so an enforcing screenshot gate holds a PR instead of destroying it `close` and `advisory` were the only options: destroy the PR, or enforce nothing. That middle matters because reviews here are ONE-SHOT for contributor work -- there is no "changes requested, try again" state -- so an enforcing gate converted every miss into unrecoverable loss. Right for genuine slop, wrong for a PR that simply has not attached screenshots yet, and the contributor cannot reopen. `block` holds the PR (no merge, never a close) and says exactly what is missing, so the hold is actionable rather than a silent stall. It clears itself the moment evidence appears -- a body table, or a successful bot capture. The hold joins MERGE_HOLD_INPUTS, so every surface folds it in by construction rather than by three edits that can each be forgotten. It respects the same two exemptions the close does: a live capture retry (the evidence may still be coming) and a degraded enforcement (#9881's first half -- the bot cannot produce evidence in this repo at all). A hold with an unmeetable condition is just a slower close. Both label paths carry the message: the merge-authorized fallback and the disposition ternary. They cover different repo configurations -- the fallback serves merge-autonomy repos, the ternary serves label-only ones -- and either missing it would hand half the fleet a bare label with no reason. Also hoists that comment ternary's eligibility guard, which was restated in all six arms. Reaching a later arm already proved the earlier arm's identical guard true, so every copy after the first was structurally unreachable-false -- dead sub-branches that could never be covered because they could never be hit. Evaluated once now, and the whole chain is reachable. Closes #9881 --- apps/loopover-ui/public/openapi.json | 1 + packages/loopover-contract/src/api-schemas.ts | 2 +- .../src/review/screenshot-table-gate.ts | 2 +- .../src/types/manifest-deps-types.ts | 9 ++- src/openapi/schemas.ts | 2 +- src/queue/processors.ts | 25 +++++++ src/settings/agent-actions.ts | 67 ++++++++++++++----- src/settings/pr-disposition.ts | 1 + src/types.ts | 9 ++- test/unit/agent-actions.test.ts | 46 +++++++++++++ test/unit/pr-disposition-invariants.test.ts | 2 +- test/unit/queue-3.test.ts | 61 +++++++++++++++++ .../unit/screenshot-table-gate-engine.test.ts | 4 ++ 13 files changed, 209 insertions(+), 22 deletions(-) diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index a4decf6b09..d85180d284 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -10335,6 +10335,7 @@ "type": "string", "enum": [ "close", + "block", "advisory" ] }, diff --git a/packages/loopover-contract/src/api-schemas.ts b/packages/loopover-contract/src/api-schemas.ts index 39122d833d..0b057b8e38 100644 --- a/packages/loopover-contract/src/api-schemas.ts +++ b/packages/loopover-contract/src/api-schemas.ts @@ -664,7 +664,7 @@ export const RepositorySettingsSchema = z enabled: z.boolean(), whenLabels: z.array(z.string()), whenPaths: z.array(z.string()), - action: z.enum(["close", "advisory"]), + action: z.enum(["close", "block", "advisory"]), requireViewports: z.array(z.string()), requireThemes: z.array(z.string()), message: z.string().optional(), diff --git a/packages/loopover-engine/src/review/screenshot-table-gate.ts b/packages/loopover-engine/src/review/screenshot-table-gate.ts index 454a2b3eb1..15e3d756b4 100644 --- a/packages/loopover-engine/src/review/screenshot-table-gate.ts +++ b/packages/loopover-engine/src/review/screenshot-table-gate.ts @@ -33,7 +33,7 @@ export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { requireThemes: [], }; -const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "advisory"]; +const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "block", "advisory"]; export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index eb8d02cffe..69fad5eedf 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -24,7 +24,14 @@ export type AiReviewLowConfidenceDisposition = "one_shot" | "hold_for_review" | // #4110: `request_changes`/`comment` were REMOVED (see src/types.ts's mirror of this type for why). // `"advisory"` (#4535) is a NEW, actually-wired value -- see src/types.ts's mirror for the full rationale. -export type ScreenshotTableGateAction = "close" | "advisory"; +/** #9881: `close` destroys the PR, `advisory` does nothing enforcing. `block` is the missing middle -- it + * HOLDS the PR (no merge) and says what is needed, without closing work the contributor can still fix. + * + * Why the middle matters here specifically: a one-shot pipeline has no "changes requested, try again" state + * for contributor work, so a gate whose only enforcing option is `close` converts every miss into destroyed + * work. That is the right answer for genuine slop and the wrong one for a PR that simply has not attached + * screenshots yet -- and it is unrecoverable, because the contributor cannot reopen. */ +export type ScreenshotTableGateAction = "close" | "block" | "advisory"; export type ScreenshotTableGateConfig = { enabled: boolean; diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index e8c9c18952..0b49ebd854 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -960,7 +960,7 @@ export const RepositorySettingsSchema = z enabled: z.boolean(), whenLabels: z.array(z.string()), whenPaths: z.array(z.string()), - action: z.enum(["close", "advisory"]), + action: z.enum(["close", "block", "advisory"]), requireViewports: z.array(z.string()), requireThemes: z.array(z.string()), message: z.string().optional(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ddf2ed76dd..1456477d2f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2807,6 +2807,7 @@ function buildAgentMaintenancePlanInput(args: { migrationCollisionHold: AgentActionPlanInput["migrationCollisionHold"]; unlinkedIssueMatchHold: AgentActionPlanInput["unlinkedIssueMatchHold"]; priorityEligibilityHold: AgentActionPlanInput["priorityEligibilityHold"]; + screenshotEvidenceHold: AgentActionPlanInput["screenshotEvidenceHold"]; aiReviewLowConfidenceHold: AgentActionPlanInput["aiReviewLowConfidenceHold"]; unlinkedIssueMatchClose: AgentActionPlanInput["unlinkedIssueMatchClose"]; liveMergeState: string | undefined; @@ -2847,6 +2848,7 @@ function buildAgentMaintenancePlanInput(args: { migrationCollisionHold, unlinkedIssueMatchHold, priorityEligibilityHold, + screenshotEvidenceHold, aiReviewLowConfidenceHold, unlinkedIssueMatchClose, liveMergeState, @@ -2936,6 +2938,7 @@ function buildAgentMaintenancePlanInput(args: { ...(migrationCollisionHold !== undefined ? { migrationCollisionHold } : {}), ...(unlinkedIssueMatchHold !== undefined ? { unlinkedIssueMatchHold } : {}), ...(priorityEligibilityHold !== undefined ? { priorityEligibilityHold } : {}), + ...(screenshotEvidenceHold !== undefined ? { screenshotEvidenceHold } : {}), ...(aiReviewLowConfidenceHold !== undefined ? { aiReviewLowConfidenceHold } : {}), ...(unlinkedIssueMatchClose !== undefined ? { unlinkedIssueMatchClose } : {}), manualReviewLockContentionResolved, @@ -3580,6 +3583,27 @@ async function runAgentMaintenancePlanAndExecute( !screenshotTableEnforcementDegraded ? { matched: true, reason: screenshotTableGateResult.reason } : undefined; + // #9881: `block` is the middle tier between close and advisory -- the PR is HELD, never closed, and told + // exactly what is missing. Same three exemptions the close respects: a live capture retry (the evidence may + // still be coming), and a degraded enforcement (the bot cannot produce evidence here at all, so holding for + // it would be as unfair as closing for it). A hold with an unmeetable condition is just a slower close. + // Captured as the REASON rather than a boolean so the hold's message and its existence cannot disagree: + // every `violated: true` return from the evaluator carries a reason, and this way the type says so without + // an unreachable null-guard. Null here simply means "not blocking this pass", which is the common case. + const screenshotBlockReason = + screenshotTableGateResult.violated && + screenshotTableGateConfig.action === "block" && + !botCaptureRetryPending && + !screenshotTableEnforcementDegraded + ? screenshotTableGateResult.reason + : null; + const screenshotEvidenceHold = + screenshotBlockReason === null + ? undefined + : { + reason: screenshotBlockReason, + comment: `${screenshotBlockReason}\n\nThis PR is held, not closed — add the missing before/after screenshots and it proceeds automatically. This is an automated maintenance action.`, + }; if (screenshotTableEnforcementDegraded) { await recordAuditEvent(env, { eventType: "github_app.screenshot_table_close_degraded_capture_unobtainable", @@ -3748,6 +3772,7 @@ async function runAgentMaintenancePlanAndExecute( migrationCollisionHold, unlinkedIssueMatchHold, priorityEligibilityHold, + screenshotEvidenceHold, aiReviewLowConfidenceHold: aiReviewLowConfidenceHold ?? aiReviewSalvageableHold, unlinkedIssueMatchClose, liveMergeState, diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 72c8f92e3a..0756580bbd 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -462,6 +462,12 @@ export type AgentActionPlanInput = { // it. Same risk profile as the two holds above -- SUPPRESSES the merge (folded into `heldForManualReview`), // never closes, and clears itself once the window passes with no action from the contributor. priorityEligibilityHold?: { reason: string; comment: string } | undefined; + // #9881: the screenshot-table gate is configured to BLOCK and this PR carries no visual evidence. Same risk + // profile as the holds above -- SUPPRESSES the merge, never closes. It exists because `close` and + // `advisory` were the only options: a one-shot pipeline has no "changes requested, try again" for + // contributor work, so an enforcing gate destroyed PRs that had simply not attached screenshots yet, and + // the contributor cannot reopen. Clears itself the moment evidence appears (a table, or a bot capture). + screenshotEvidenceHold?: { reason: string; comment: string } | undefined; // Same guardrail as unlinkedIssueMatchHold, but for a CONFIRMED REPEAT by the same contributor (tracked via // audit_events, see resolveUnlinkedIssueMatchDisposition) -- a second occurrence is no longer a coincidence // worth a human's benefit of the doubt, so this closes the PR one-shot instead of holding it. Deliberately @@ -1153,6 +1159,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne migrationCollisionHold: input.migrationCollisionHold !== undefined, unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined, priorityEligibilityHold: input.priorityEligibilityHold !== undefined, + screenshotEvidenceHold: input.screenshotEvidenceHold !== undefined, advisoryCheckHold: input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0, // Deliberately conjoined with "nothing else adverse": an unstable state is only attributed to the ignore // list when our own aggregate found NO failing check of any kind. If some other non-required check is @@ -1304,6 +1311,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne input.migrationCollisionHold === undefined && input.unlinkedIssueMatchHold === undefined && input.priorityEligibilityHold === undefined && + input.screenshotEvidenceHold === undefined && input.unlinkedIssueMatchClose === undefined && !mergeableStateUnstable && !heldForManualReview && @@ -1367,6 +1375,22 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // 1d-priority) priority-eligibility hold (#9738) — mirrors 1d exactly. The PR is EARLY, not wrong: it is // held with a neutral comment naming the moment work opens, and nothing else about it changes. The label // is the same generic manual-review one, so the hold is visible on the surfaces a maintainer already reads. + // #9881: the merge-authorized fallback, mirroring priorityEligibilityHold's directly below. A repo that + // enables merge autonomy WITHOUT review_state_label gets no disposition label from the ternary above, so + // without this the block tier would suppress the merge and say nothing at all -- a silent stall, which is + // the failure mode this tier exists to replace. Skipped when the ternary already planned the label + // (hasLabelOrPlanned), so the two never double-add. + if (reviewGood && input.screenshotEvidenceHold !== undefined && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { + actions.push({ + actionClass: "label", + autonomyClass: "merge", + requiresApproval: approval("merge"), + reason: `verdict=${conclusion}; ${input.screenshotEvidenceHold.reason}`, + label: labels.manualReview, + labelOp: "add", + comment: sanitizePublicComment(input.screenshotEvidenceHold.comment), + }); + } if (reviewGood && input.priorityEligibilityHold !== undefined && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { actions.push({ actionClass: "label", @@ -1463,22 +1487,33 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne requiresApproval: approval("review_state_label"), reason, label, - // Only the migration-collision hold and the unlinked-issue-match hold carry a comment here — the - // guardrail/ready/changes labels never did and still don't (comment stays undefined, matching the - // pre-#2550 shape exactly). Migration-collision takes priority when both are somehow true (matches - // the label-priority choice above). unlinkedIssueMatchViolated is excluded here too: its own CLOSE - // action already carries the full closeComment, so this label needs no separate comment. - ...(!linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.migrationCollisionHold !== undefined - ? { comment: sanitizePublicComment(input.migrationCollisionHold.comment) } - : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.unlinkedIssueMatchHold !== undefined - ? { comment: sanitizePublicComment(input.unlinkedIssueMatchHold.comment) } - : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.unlinkedIssueMatchClose !== undefined - ? { comment: sanitizePublicComment(input.unlinkedIssueMatchClose.comment) } - : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0 - ? { comment: sanitizePublicComment(advisoryHoldComment(input.advisoryCheckHold)) } - : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && mergeableStateUnstable - ? { comment: sanitizePublicComment(mergeUnstableHoldComment(input.nonRequiredCheckFailures)) } - : {}), + // Only a HOLD carries a comment here — the guardrail/ready/changes labels never did and still don't + // (comment stays undefined, matching the pre-#2550 shape). Priority follows the label choice above. + // + // The eligibility guard is evaluated ONCE rather than repeated per arm: a linked-issue close in + // flight and an unlinked-issue-match close each already carry their own full message, and a + // not-review-good PR is not being held for any of these reasons. Restating it inside every arm (as + // this chain did) made each copy structurally unreachable-false, because reaching a later arm already + // proved the earlier one's identical guard true. + ...(!linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood + ? input.migrationCollisionHold !== undefined + ? { comment: sanitizePublicComment(input.migrationCollisionHold.comment) } + : input.unlinkedIssueMatchHold !== undefined + ? { comment: sanitizePublicComment(input.unlinkedIssueMatchHold.comment) } + : input.unlinkedIssueMatchClose !== undefined + ? { comment: sanitizePublicComment(input.unlinkedIssueMatchClose.comment) } + : input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0 + ? { comment: sanitizePublicComment(advisoryHoldComment(input.advisoryCheckHold)) } + : // #9881: the block tier's point is that the hold is ACTIONABLE -- "held, not closed, here + // is what is missing". The disposition already emits the manual-review label for it, so + // the message belongs on THAT action; a competing second add would be skipped by + // hasLabelOrPlanned and the contributor would get a bare label with no reason. + input.screenshotEvidenceHold !== undefined + ? { comment: sanitizePublicComment(input.screenshotEvidenceHold.comment) } + : mergeableStateUnstable + ? { comment: sanitizePublicComment(mergeUnstableHoldComment(input.nonRequiredCheckFailures)) } + : {} + : {}), }); } // Stale disposition-label cleanup (#stale-disposition-label-cleanup): the review-state labels below diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index 0ce0874438..dfb11add42 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -68,6 +68,7 @@ const MERGE_HOLD_INPUTS = { unlinkedIssueMatchHold: "an unlinked issue appears to match this work", advisoryCheckHold: "an advisory check the maintainer configured is not passing", priorityEligibilityHold: "the linked priority issue's eligibility window has not elapsed", + screenshotEvidenceHold: "the screenshot-table gate is set to block and the PR has no visual evidence", unlinkedIssueMatchCloseWithoutCloseActing: "a repeat unlinked-issue match while close autonomy is off", } as const; diff --git a/src/types.ts b/src/types.ts index 938ee17517..9ec45be661 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1600,7 +1600,14 @@ export type RepositorySettings = { * which appends a non-blocking `screenshot_table_missing` finding to the PR's advisory panel whenever * `action === "advisory"` and the gate would have violated -- a deterministic signal, not just left to chance * in the AI reviewer's own commentary. */ -export type ScreenshotTableGateAction = "close" | "advisory"; +/** #9881: `close` destroys the PR, `advisory` does nothing enforcing. `block` is the missing middle -- it + * HOLDS the PR (no merge) and says what is needed, without closing work the contributor can still fix. + * + * Why the middle matters here specifically: a one-shot pipeline has no "changes requested, try again" state + * for contributor work, so a gate whose only enforcing option is `close` converts every miss into destroyed + * work. That is the right answer for genuine slop and the wrong one for a PR that simply has not attached + * screenshots yet -- and it is unrecoverable, because the contributor cannot reopen. */ +export type ScreenshotTableGateAction = "close" | "block" | "advisory"; /** Per-repo config for the before/after screenshot-table gate (#2006). See {@link RepositorySettings.screenshotTableGate} * and `review/screenshot-table-gate.ts` for the normalizer + pure evaluator. */ diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 9da7b2fe57..5234bb2a81 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -66,6 +66,52 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(collision)).not.toContain("merge"); }); + // #9881: `close` destroys the PR and `advisory` enforces nothing. `block` is the middle tier -- the PR is + // HELD and told what is missing, without destroying work the contributor can still fix. That middle matters + // because a one-shot pipeline has no "changes requested, try again" for contributor work, and a close is + // unrecoverable: the contributor cannot reopen. + describe("screenshot-evidence block tier (#9881)", () => { + const blocked = { + conclusion: "success" as const, + autonomy: { merge: "auto" as const }, + manualReviewLabel: "human-review", + screenshotEvidenceHold: { reason: "missing before/after screenshots", comment: "add them and it proceeds" }, + pr: { labels: [], mergeableState: "clean" as const, reviewDecision: "APPROVED" as const }, + }; + + it("HOLDS the PR instead of merging it, and never closes", () => { + const plan = planAgentMaintenanceActions(input(blocked)); + expect(classes(plan)).not.toContain("merge"); + // The load-bearing distinction from `close`: the PR survives. + expect(classes(plan)).not.toContain("close"); + expect(plan.some((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp !== "remove")).toBe(true); + }); + + it("tells the contributor what is missing, so the hold is actionable rather than a silent stall", () => { + const plan = planAgentMaintenanceActions(input(blocked)); + const held = plan.find((a) => a.actionClass === "label" && a.label === "human-review"); + expect(held?.comment).toContain("add them and it proceeds"); + expect(held?.reason).toContain("missing before/after screenshots"); + }); + + it("carries the reason on the DISPOSITION label, for a label-only repo where the merge fallback cannot fire", () => { + // Two paths can emit the hold label: the merge-authorized fallback and the disposition ternary. The + // fallback runs first and claims the label, so the ternary is reachable only where merge autonomy is + // OFF and review_state_label is ON -- a real configuration, and the one half of the fleet that would + // otherwise get a bare label with no reason. Both paths must carry the message. + const plan = planAgentMaintenanceActions(input({ ...blocked, autonomy: { review_state_label: "auto" } })); + const held = plan.find((a) => a.actionClass === "label" && a.label === "human-review"); + expect(held?.comment).toContain("add them and it proceeds"); + expect(held?.autonomyClass).toBe("review_state_label"); + expect(classes(plan)).not.toContain("close"); + }); + + it("INVARIANT: absent (the default) is byte-identical to today — a clean approved PR still merges", () => { + const plan = planAgentMaintenanceActions(input({ ...blocked, screenshotEvidenceHold: undefined })); + expect(classes(plan)).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 diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index 031f57ca59..f992eea97c 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -222,7 +222,7 @@ describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => describe("guardrail hold released by a clean escalated review (#9808 second half)", () => { const base = { reviewGood: true, guardrailHit: true, migrationCollisionHold: false, unlinkedIssueMatchHold: false, - advisoryCheckHold: false, priorityEligibilityHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false, + advisoryCheckHold: false, priorityEligibilityHold: false, screenshotEvidenceHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false, mergeableState: "clean", }; diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 5f9002a6a3..6f3f66b4df 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2706,6 +2706,67 @@ describe("queue processors", () => { expect(expiredAudit?.n).toBe(1); }); + // #9881: `block` is the middle tier. `close` destroys a contributor PR that simply has not attached + // screenshots yet -- unrecoverable, since they cannot reopen -- and `advisory` enforces nothing at all. + it("screenshot-table gate (#9881): action=block HOLDS the PR with a reason instead of closing it", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[] }; + env.JOBS = { async send() {} } as unknown as Queue; + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto", merge: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, action: "block", whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 74, title: "Update the app index route", state: "open", user: { login: "visual-contributor" }, + head: { sha: "vis74" }, labels: [{ name: "visual" }], body: "Changed the route layout, no table here.", + }); + + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/74/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/74/reviews")) return Response.json([{ state: "APPROVED", user: { login: "JSONbored" }, submitted_at: "2026-07-30T10:00:00Z" }]); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + // A REAL required context (matching the status stub below): an empty list makes CI "resolved but + // unverifiable", which is itself a hold reason and would mask the block tier under a different one. + if (url.includes("/branches/")) return Response.json({ contexts: ["ci/build"] }); + if (url.includes("/pulls/74/commits")) return Response.json([]); + if (url.endsWith("/pulls/74") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 74, state: "closed" }); } + if (url.includes("/pulls/74/merge")) { seen.merged = true; return Response.json({ merged: true }); } + if (url.endsWith("/pulls/74")) return Response.json({ number: 74, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis74" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis74/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes("/commits/vis74/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "ci/build", status: "completed", conclusion: "success" }] }); + if (url.includes("/commits/vis74/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/74/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/74/comments") && method === "POST") { seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); return Response.json({ id: 1 }); } + if (url.includes("/issues/74/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") { seen.labels.push(...(JSON.parse(String(init?.body ?? "{}")).labels ?? [])); return Response.json([]); } + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 907 }, { status: 201 }); + if (url.includes("/check-runs/907") && method === "PATCH") return Response.json({ id: 907 }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "screenshot-block-tier", repoFullName: "JSONbored/gittensory", prNumber: 74, installationId: 123, force: true }); + + // The load-bearing distinction from `close`: the PR survives, and it is not merged either. + expect(seen.closed).toBe(false); + expect(seen.merged).toBe(false); + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); + expect(closeAudit?.n).toBe(0); + // The PR is parked for review rather than silently ignored. + expect(seen.labels.join(",")).toContain("manual-review"); + // The hold's MESSAGE is asserted in agent-actions.test.ts (#9881) rather than here, and deliberately so: + // every sibling hold (priorityEligibilityHold, migrationCollisionHold, advisoryCheckHold) is gated on + // `reviewGood`, which a bare regate cannot reach -- it skips the AI review, so the gate conclusion is + // `neutral`. Driving a full AI-review pass here to re-assert one string would test the harness, not the + // tier. What only the pipeline can prove is this: a violated `block` gate reaches a real decision and the + // PR still exists afterwards. + }); + describe("live migrations/** collision recheck (#2550)", () => { // Full merge-eligible stub set (clean + green + approved), reused across scenarios — a positive test proves // the collision hold actually suppresses what would otherwise merge; a negative test proves the check diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index e4c159d50b..eb99f16b3a 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -220,6 +220,10 @@ describe("normalizeScreenshotTableGateConfig", () => { it("rejects an invalid action with a warning, falling back to close", () => { const warnings: string[] = []; + // #9881: "block" is a REAL action now -- the middle tier between destroying the PR and enforcing nothing. + // Pinned here beside the rejected values so a future tightening of the enum cannot silently drop it. + expect(normalizeScreenshotTableGateConfig({ action: "block" }, []).action).toBe("block"); + expect(normalizeScreenshotTableGateConfig({ action: "advisory" }, []).action).toBe("advisory"); expect(normalizeScreenshotTableGateConfig({ action: "delete" }, warnings).action).toBe("close"); expect(warnings.some((w) => w.includes("action"))).toBe(true); });