feat(gate): add the block tier, so an enforcing screenshot gate holds a PR instead of destroying it

apps/loopover-ui/public/openapi.json

@@ -10335,6 +10335,7 @@
                 "type": "string",
                 "enum": [
                   "close",
+                  "block",
                   "advisory"
                 ]
               },

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(),

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);

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;

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(),

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,

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

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;
 

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. */

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

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",
   };