From 708cbd83e330070923244ba590b539867113f5cb Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 06:32:03 -0700 Subject: [PATCH] fix(github): hold non-required third-party action_required checks instead of auto-closing (#4414) A completed action_required check-run from a third-party app was treated as a hard CI failure regardless of whether it was an actual branch-protection required context. Superagent posts "Contributor trust" alongside its own required "Superagent Security Scan" check, but "Contributor trust" itself is never required -- so real contributor PRs were auto-closed on a signal branch protection never asked for. reduceLiveCiAggregate now only hard-fails a third-party action_required check when isRequired() confirms it's an actual required context. A non-required one is routed to the existing nonRequiredFailingDetails bucket instead -- never flipping ciState/blocking merge, but still rendered under its own "Flagged checks (non-blocking)" section in the PR comment so it's never silently dropped either. --- src/github/backfill.ts | 28 +++- src/queue/processors.ts | 10 ++ src/review/unified-comment.ts | 29 ++++ test/unit/backfill.test.ts | 93 +++++++++++ test/unit/queue.test.ts | 259 ++++++++++++++++++++++++++++++ test/unit/unified-comment.test.ts | 56 +++++++ 6 files changed, 468 insertions(+), 7 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index f5b4d4d9fc..aaa3333993 100644 --- a/src/github/backfill.ts +++ b/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 diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d9c3d04aae..88085fc5cf 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -10578,6 +10578,15 @@ 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 } : {}), @@ -10585,6 +10594,7 @@ async function maybePublishPrPublicSurface( ? { 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; diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 7a1ab184f0..ff8a2871ee 100644 --- a/src/review/unified-comment.ts +++ b/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 diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 7d9806e8fd..27726a11c0 100644 --- a/test/unit/backfill.test.ts +++ b/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) => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 6f85caae2e..977ac3d2f5 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -19992,6 +19992,265 @@ describe("queue processors", () => { expect(postedBody).not.toMatch(/wallet|hotkey|reward|trust score/i); }); + // REGRESSION (#4414-class advisory holds): a third-party app's COMPLETED action_required check-run that is + // NOT a branch-protection required context (e.g. Superagent's "Contributor trust", posted alongside its own + // separate, actually-required "Superagent Security Scan") must never flip ciState to "failed" or post under + // "CI checks failing" -- that auto-closes real contributor PRs (#4414's regression). It must still be VISIBLE, + // under its own non-blocking "Flagged checks" section, so a maintainer can act on it without the PR being + // silently waved through OR silently closed. + it("REGRESSION (#4414-class advisory holds): a non-required third-party action_required check renders as a non-blocking 'Flagged checks' note, not a CI failure", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITHUB_PUBLIC_TOKEN: "public-token", GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", + backfillEnabled: true, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 6, + title: "Fix flaky retry test", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "flagged456" }, + base: { ref: "main" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }); + let postedBody = ""; + 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([ + { + uid: 7, + githubUsername: "oktofeesh1", + githubId: "123", + totalPrs: 4, + totalMergedPrs: 3, + totalOpenPrs: 1, + totalClosedPrs: 0, + totalOpenIssues: 0, + totalClosedIssues: 0, + totalSolvedIssues: 0, + totalValidSolvedIssues: 0, + isEligible: true, + credibility: 1, + eligibleRepoCount: 1, + hotkey: "must-not-leak", + }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ + repositories: [ + { + repositoryFullName: "JSONbored/gittensory", + totalPrs: "4", + totalMergedPrs: "3", + totalOpenPrs: "1", + totalClosedPrs: "0", + totalOpenIssues: "0", + totalClosedIssues: "0", + isEligible: true, + credibility: "1.000000", + }, + ], + }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/6/files")) return Response.json([{ filename: "src/retry.ts", additions: 3, deletions: 1, status: "modified", patch: "@@\n+const x = 1;" }]); + // Branch protection requires ONLY "validate" + "Superagent Security Scan" -- NOT "Contributor trust", + // matching the real-world JSONbored/gittensory config that #4414 broke. + if (url.includes("/branches/main/protection/required_status_checks")) return Response.json({ contexts: ["validate", "Superagent Security Scan"] }); + if (url.includes("/check-runs") && method === "GET") { + return Response.json({ + total_count: 3, + 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("/commits/") && url.includes("/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 903 }, { status: 201 }); + if (url.includes("/check-runs/903") && method === "PATCH") return Response.json({ id: 903 }); + if (url.includes("/issues/6/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/6/comments") && method === "POST") { + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-flagged-nonrequired-check", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 6, + title: "Fix flaky retry test", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "flagged456" }, + base: { ref: "main" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }, + }, + }); + + // Never a CI failure -- the non-required check must not flip ciState/block the PR. + expect(postedBody).not.toContain("`CI failing`"); + expect(postedBody).not.toContain("CI checks failing"); + // But never silently invisible either -- surfaced as its own non-blocking note, with its per-check WHY. + expect(postedBody).toContain("Flagged checks (non-blocking)"); + expect(postedBody).toContain("Contributor trust"); + expect(postedBody).toContain("Manual review needed"); + expect(postedBody).not.toMatch(/wallet|hotkey|reward|trust score/i); + }); + + it("REGRESSION (#4414-class advisory holds): a bare non-required action_required check (no output/details_url) still renders under 'Flagged checks', name-only", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITHUB_PUBLIC_TOKEN: "public-token", GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", + backfillEnabled: true, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 7, + title: "Bump lockfile", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "flagged457" }, + base: { ref: "main" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }); + let postedBody = ""; + 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([ + { uid: 7, githubUsername: "oktofeesh1", githubId: "123", totalPrs: 4, totalMergedPrs: 3, totalOpenPrs: 1, totalClosedPrs: 0, totalOpenIssues: 0, totalClosedIssues: 0, totalSolvedIssues: 0, totalValidSolvedIssues: 0, isEligible: true, credibility: 1, eligibleRepoCount: 1, hotkey: "must-not-leak" }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ repositories: [{ repositoryFullName: "JSONbored/gittensory", totalPrs: "4", totalMergedPrs: "3", totalOpenPrs: "1", totalClosedPrs: "0", totalOpenIssues: "0", totalClosedIssues: "0", isEligible: true, credibility: "1.000000" }] }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "package-lock.json", additions: 2, deletions: 2, status: "modified", patch: "@@\n+1" }]); + if (url.includes("/branches/main/protection/required_status_checks")) return Response.json({ contexts: ["validate", "Superagent Security Scan"] }); + if (url.includes("/check-runs") && method === "GET") { + return Response.json({ + total_count: 3, + check_runs: [ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Superagent Security Scan", status: "completed", conclusion: "success", app: { slug: "superagent-security" } }, + // Bare: no output, no details_url -- the common real-world shape for a check-run with nothing to say. + { name: "Contributor trust", status: "completed", conclusion: "action_required", app: { slug: "superagent-security" } }, + ], + }); + } + if (url.includes("/commits/") && url.includes("/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 904 }, { status: 201 }); + if (url.includes("/check-runs/904") && method === "PATCH") return Response.json({ id: 904 }); + if (url.includes("/issues/7/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/7/comments") && method === "POST") { + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-flagged-nonrequired-check-bare", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 7, + title: "Bump lockfile", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "flagged457" }, + base: { ref: "main" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }, + }, + }); + + expect(postedBody).not.toContain("CI checks failing"); + expect(postedBody).toContain("Flagged checks (non-blocking)"); + expect(postedBody).toContain("- Contributor trust"); + }); + it("skips bots and maintainer authors, and keeps explicitly enabled checks minimal", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 6797a5846e..51833a2891 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -351,6 +351,62 @@ describe("renderUnifiedReviewComment", () => { expect(md).not.toContain("broke "); }); + it("renders non-required-but-red checks as a non-blocking 'Flagged checks' section, independent of ciState (#4414-class advisory holds)", () => { + const md = renderUnifiedReviewComment( + { + ...base, + readiness: { + ciState: "passed", + nonRequiredFailingDetails: [{ name: "Contributor trust", summary: "flagged for manual review" }], + }, + }, + {}, + ); + expect(md).toContain("Flagged checks (non-blocking)"); + expect(md).toContain("- Contributor trust — flagged for manual review"); + // Never blocking: ciState stayed "passed" input, and this section must not read as the failing-checks one. + expect(md).not.toContain("**CI checks failing**"); + }); + + it("omits the 'Flagged checks' section when nonRequiredFailingDetails is absent/empty (default, byte-identical)", () => { + expect(renderUnifiedReviewComment({ ...base, readiness: { ciState: "passed" } }, {})).not.toContain("Flagged checks"); + expect(renderUnifiedReviewComment({ ...base, readiness: { ciState: "passed", nonRequiredFailingDetails: [] } }, {})).not.toContain("Flagged checks"); + }); + + it("hides the 'Flagged checks' section under review.comment_verbosity: quiet, matching Nits/linked-issue-satisfaction", () => { + const md = renderUnifiedReviewComment( + { ...base, readiness: { ciState: "passed", nonRequiredFailingDetails: [{ name: "Contributor trust" }] } }, + { commentVerbosity: "quiet" }, + ); + expect(md).not.toContain("Flagged checks"); + }); + + it("angle-escapes a non-required-failing check name + detail (public-safety, mirrors FIX D3)", () => { + const md = renderUnifiedReviewComment( + { ...base, readiness: { ciState: "passed", nonRequiredFailingDetails: [{ name: "check ", summary: "broke " }] } }, + {}, + ); + expect(md).toContain("check <x>"); + expect(md).toContain("broke </details>"); + expect(md).not.toContain("broke "); + }); + + it("drops a non-required-failing entry with a blank/whitespace-only name (defensive), keeping other valid entries", () => { + const md = renderUnifiedReviewComment( + { + ...base, + readiness: { + ciState: "passed", + nonRequiredFailingDetails: [{ name: " " }, { name: "Contributor trust" }], + }, + }, + {}, + ); + expect(md).toContain("Flagged checks (non-blocking)"); + expect(md).toContain("- Contributor trust"); + expect(md).not.toMatch(/-\s*\n/); // the blank-named entry never rendered its own bullet + }); + it("appends an explicit verdict reason across ready (merged + unmerged) and advisory states", () => { const merged = renderUnifiedReviewComment({ ...base, decision: "merge", merged: true, verdictReason: "all checks green" }, {}); expect(merged).toContain("**✅ Suggested Action - Approve/Merge**");