Merge pull request #4423 from JSONbored/claude/fix-third-party-check-manual-hold

fix(github): hold non-required third-party action_required checks instead of auto-closing

Author: JSONbored

src/github/backfill.ts

@@ -2775,6 +2775,8 @@ async function reduceLiveCiAggregate(
   // 1) Check-runs (GitHub Actions jobs, CodeQL, app checks). Deduped by check-run identity first, so a
   // re-run job's stale duplicate entry can never contribute its own failingDetails/pending signal alongside the
   // current one, without collapsing unrelated checks that merely share a display name.
+  const checkRunSummary = (run: LiveCiCheckRun): string | undefined =>
+    [run.output?.title, run.output?.summary].find((value): value is string => typeof value === "string" && value.trim().length > 0)?.trim().slice(0, 200);
   for (const run of dedupeLatestCheckRunsByIdentity(checkRuns)) {
     seenContextNames.add(run.name); // mark BEFORE bot-check skip: a bot-owned required context is "seen"
     const appSlug = (run.app?.slug ?? "").toLowerCase();
@@ -2784,13 +2786,25 @@ async function reduceLiveCiAggregate(
     const conclusion = (run.conclusion ?? "").toLowerCase();
     const status = (run.status ?? "").toLowerCase();
     // A THIRD-PARTY app's OWN action_required verdict on an already-COMPLETED check-run (for example, a
-    // security/check tool asking for human review) is a settled, terminal adverse result. This is NOT the
-    // github-actions "awaiting maintainer Approve and run" case the action_required exclusion above exists for:
-    // non-Actions apps use their own conclusion as a policy signal, so fail closed instead of treating it as
-    // green CI. Conservative: an unknown/absent app slug is NOT treated as third-party here.
-    const isThirdPartyActionRequiredFailure = conclusion === "action_required" && status === "completed" && appSlug !== "" && appSlug !== "github-actions";
-    if (isThirdPartyActionRequiredFailure || (conclusion ? CI_FAILING_CONCLUSIONS.has(conclusion) : false)) {
-      const summary = [run.output?.title, run.output?.summary].find((value): value is string => typeof value === "string" && value.trim().length > 0)?.trim().slice(0, 200);
+    // security/check tool asking for human review) is a settled, terminal adverse result -- but ONLY when that
+    // check is actually a REQUIRED context. A non-required third-party check must never hard-fail/auto-close the
+    // PR on its own say-so: the same app can post multiple check-runs (e.g. a required "X Security Scan" plus a
+    // separate, NEVER-required "X Contributor trust" advisory check), and treating either one's action_required
+    // the same way conflates them (#4414 regressed exactly this -- a non-required advisory check started
+    // auto-closing real contributor PRs). This is NOT the github-actions "awaiting maintainer Approve and run"
+    // case the action_required exclusion above exists for: non-Actions apps use their own conclusion as a policy
+    // signal. Conservative: an unknown/absent app slug is NOT treated as third-party here.
+    const isThirdPartyActionRequired = conclusion === "action_required" && status === "completed" && appSlug !== "" && appSlug !== "github-actions";
+    if (isThirdPartyActionRequired && isRequired(run.name)) {
+      const summary = checkRunSummary(run);
+      failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) });
+    } else if (isThirdPartyActionRequired) {
+      // Non-required: visible (never silently folded into "passed" either, unlike the pre-#4414 behavior) but
+      // non-blocking -- routed to nonRequiredFailingDetails, which never feeds ciState or a close decision.
+      const summary = checkRunSummary(run);
+      nonRequiredFailingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) });
+    } else if (conclusion ? CI_FAILING_CONCLUSIONS.has(conclusion) : false) {
+      const summary = checkRunSummary(run);
       failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) });
     } else if (conclusion ? CI_PASSING_CONCLUSIONS.has(conclusion) : status === "completed") {
       // concluded and not failing → passing

src/queue/processors.ts

@@ -10599,13 +10599,23 @@ async function maybePublishPrPublicSurface(
           ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}),
         }),
       );
+      // Non-required-but-red checks (#4414-class advisory holds): surfaced so a flagged check is never silently
+      // invisible, but never folded into failingChecks/failingDetails -- those two drive ciState/close.
+      const nonRequiredFailingDetails: CheckFailureDetail[] = liveCi.nonRequiredFailingDetails.map(
+        (detail) => ({
+          name: detail.name,
+          ...(detail.summary ? { summary: detail.summary } : {}),
+          ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}),
+        }),
+      );
       const mergeReadiness: MergeReadiness = {
         ciState,
         ...(mergeStateLabel ? { mergeStateLabel } : {}),
         ...(failingDetails.length > 0
           ? { failingChecks: failingDetails.map((detail) => detail.name) }
           : {}),
         ...(failingDetails.length > 0 ? { failingDetails } : {}),
+        ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}),
       };
       // The public comment must match the authoritative Gate check-run conclusion.
       const commentGate = gateEvaluation;

src/review/unified-comment.ts

@@ -91,6 +91,12 @@ export interface MergeReadiness {
   ciState: "passed" | "failed" | "unverified";
   failingChecks?: string[];
   failingDetails?: CheckFailureDetail[];
+  /** Checks that reported red (e.g. a third-party app's `action_required` conclusion) but are NOT a
+   *  branch-protection required context -- so they never flip `ciState`/block merge on their own, but must
+   *  still be VISIBLE rather than silently dropped (#4414-class regression: a non-required advisory check must
+   *  neither auto-close the PR nor vanish without a trace). Rendered as its own non-blocking collapsible,
+   *  independent of `ciState`. */
+  nonRequiredFailingDetails?: CheckFailureDetail[];
 }
 
 /** The structured synthesis of the reviewers' notes that drives BOTH the legacy unified comment
@@ -524,6 +530,22 @@ function failingChecksBlock(readiness: MergeReadiness | undefined): string {
   return [...new Set(names)].map((name) => `- ${escapePublicHtmlAngles(name)}`).join("\n");
 }
 
+/** Render non-required-but-red checks (#4414-class advisory holds) as a `name — reason` bullet list, same
+ *  shape/public-safety rules as `failingChecksBlock`. Unlike that one, this is NOT gated on `ciState` -- these
+ *  checks by definition never flip `ciState`, so the section must render purely off the data's own presence. */
+function nonRequiredFailingChecksBlock(readiness: MergeReadiness | undefined): string {
+  const details = readiness?.nonRequiredFailingDetails ?? [];
+  const lines = details
+    .map((detail) => {
+      const name = escapePublicHtmlAngles(detail.name.trim());
+      if (!name) return "";
+      const reason = detail.summary?.trim() ? ` — ${escapePublicHtmlAngles(detail.summary.trim())}` : "";
+      return `- ${name}${reason}`;
+    })
+    .filter((line) => line.length > 0);
+  return lines.join("\n");
+}
+
 function signalTable(input: UnifiedReviewInput, ctx: UnifiedCommentContext): string {
   const blockerCount = (input.blockers ?? []).length;
   const reviewerEvidence =
@@ -651,6 +673,13 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi
   const failingChecks = failingChecksBlock(input.readiness);
   if (failingChecks) blocks.push(`**CI checks failing**\n${failingChecks}`);
 
+  // Non-required-but-red checks (#4414-class advisory holds): visible but never blocking, so this renders
+  // independent of ciState/status -- omitted entirely when nothing was flagged (default) ⇒ byte-identical.
+  const nonRequiredFailingChecks = nonRequiredFailingChecksBlock(input.readiness);
+  if (nonRequiredFailingChecks && verbosity !== "quiet") {
+    blocks.push(details("Flagged checks (non-blocking)", nonRequiredFailingChecks, undefined, collapsiblesOpen));
+  }
+
   blocks.push(signalTable(input, ctx));
 
   // Linked-issue satisfaction advisory (#2174): additive, collapsed section — omitted entirely when the host

test/unit/backfill.test.ts

@@ -4237,6 +4237,99 @@ describe("GitHub backfill", () => {
       expect(aggregate.failingDetails).toEqual([{ name: "Contributor trust" }]);
     });
 
+    it("a third-party app's COMPLETED action_required check-run that IS a required context carries its summary/detailsUrl into failingDetails", async () => {
+      const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
+      vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+        const url = input.toString();
+        if (url.includes("/check-runs?")) {
+          return Response.json({
+            check_runs: [
+              { name: "coverage", status: "completed", conclusion: "success", app: { slug: "github-actions" } },
+              {
+                name: "Contributor trust",
+                status: "completed",
+                conclusion: "action_required",
+                app: { slug: "superagent-security" },
+                output: { title: "Manual review needed" },
+                details_url: "https://superagent.example/checks/contributor-trust",
+              },
+            ],
+          });
+        }
+        if (url.includes("/status?")) return Response.json({ statuses: [] });
+        if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] });
+        return new Response("not found", { status: 404 });
+      });
+
+      const aggregate = await fetchLiveCiAggregate(env, "JSONbored/awesome-claude", "sha4729", "public-token", new Set(["coverage", "Contributor trust"]));
+
+      expect(aggregate.ciState).toBe("failed");
+      expect(aggregate.failingDetails).toEqual([
+        { name: "Contributor trust", summary: "Manual review needed", detailsUrl: "https://superagent.example/checks/contributor-trust" },
+      ]);
+    });
+
+    it("a third-party app's COMPLETED action_required check-run that is NOT a required context is held as a non-blocking advisory, never auto-closing the PR (#4414-regression)", async () => {
+      const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
+      vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+        const url = input.toString();
+        if (url.includes("/check-runs?")) {
+          return Response.json({
+            check_runs: [
+              { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } },
+              { name: "Superagent Security Scan", status: "completed", conclusion: "success", app: { slug: "superagent-security" } },
+              {
+                name: "Contributor trust",
+                status: "completed",
+                conclusion: "action_required",
+                app: { slug: "superagent-security" },
+                output: { title: "Manual review needed" },
+                details_url: "https://superagent.example/checks/contributor-trust",
+              },
+            ],
+          });
+        }
+        if (url.includes("/status?")) return Response.json({ statuses: [] });
+        if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] });
+        return new Response("not found", { status: 404 });
+      });
+
+      // Matches real branch protection: only "validate" + "Superagent Security Scan" are required contexts --
+      // "Contributor trust" is a SEPARATE, never-required check-run posted by the same app.
+      const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "sha9001", "public-token", new Set(["validate", "Superagent Security Scan"]));
+
+      expect(aggregate.ciState).toBe("passed");
+      expect(aggregate.hasPending).toBe(false);
+      expect(aggregate.failingDetails).toEqual([]);
+      expect(aggregate.nonRequiredFailingDetails).toEqual([
+        { name: "Contributor trust", summary: "Manual review needed", detailsUrl: "https://superagent.example/checks/contributor-trust" },
+      ]);
+    });
+
+    it("a non-required third-party action_required check-run with no output/details_url still lands in nonRequiredFailingDetails, bare (name-only)", async () => {
+      const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
+      vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+        const url = input.toString();
+        if (url.includes("/check-runs?")) {
+          return Response.json({
+            check_runs: [
+              { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } },
+              { name: "Contributor trust", status: "completed", conclusion: "action_required", app: { slug: "superagent-security" } },
+            ],
+          });
+        }
+        if (url.includes("/status?")) return Response.json({ statuses: [] });
+        if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] });
+        return new Response("not found", { status: 404 });
+      });
+
+      const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "sha9002", "public-token", new Set(["validate"]));
+
+      expect(aggregate.ciState).toBe("passed");
+      expect(aggregate.failingDetails).toEqual([]);
+      expect(aggregate.nonRequiredFailingDetails).toEqual([{ name: "Contributor trust" }]);
+    });
+
     it("a github-actions workflow awaiting 'Approve and run' (action_required) is still treated as pending, not settled (#fork-action-required)", async () => {
       const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
       vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {