From e9d1cba829e781aaf534da6a50407ff0ad3e31e4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 20 Jul 2026 12:57:30 -0700 Subject: [PATCH] fix(review): gate_check_run's byte-identical no-op closes #6724's remaining gap (#7514) surfaceContentChanged treated gate_check_run's mere presence in a pass's published outputs as an unconditional change, even when the gate's actual conclusion was byte-identical to the last completed check for that exact head SHA -- #6724 already covered comment/label this way but its own test file flagged gate_check_run as "a separate, not-yet-implemented scope." recordPublishedGateCheckSummary already upserts keyed on [repoFullName, headSha, name], so a prior row for the exact current head SHA only exists if this SAME commit was already gate-checked before -- a genuinely new commit has no prior row and always falls through to "changed." Reads the prior conclusion once, before this pass writes anything, and compares it against whichever conclusion this pass actually recorded at each of the 3 gate-check publish sites -- not re-derived, so the comparison can't drift from what was really written. check_run (the separate advisory check) has no equivalent persisted prior state today, so its presence is still left unconditionally counted, same as before -- extending that would need new persistent storage and is a larger, separate change. Also corrects one existing #6724 test that turned out to rely on an unrelated type-label mechanism's own activity rather than the label output it claimed to test (confirmed via direct instrumentation: decision.willLabel was false throughout that test; the label POSTs came from the separate, unconditional resolvePrTypeLabel path). Closes #7514 --- src/queue/processors.ts | 55 +++++++++++-- test/unit/queue.test.ts | 173 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 206 insertions(+), 22 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3a5d5ac985..5e49b2c7ee 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -8306,6 +8306,31 @@ async function maybePublishPrPublicSurface( willCheckRun: false, }; + // #ops-review-burst: the LAST completed gate-check conclusion recorded for this EXACT head SHA, captured + // BEFORE this pass writes anything -- recordPublishedGateCheckSummary upserts keyed on + // [repoFullName, headSha, name], so a row only exists here at all if THIS SAME COMMIT was already + // gate-checked before (a genuinely new commit has no prior row for its own headSha, and correctly falls + // through to "changed" below). Read once, up front, so the later surfaceContentChanged comparison + // compares against what was true before this pass, not a value this pass's own writes already overwrote. + const priorGateCheckConclusion = + gateEnabled && advisory.headSha + ? await listCheckSummaries(env, repoFullName, pr.number) + .then((checks) => + checks.find( + (check) => + check.name === LOOPOVER_GATE_CHECK_NAME && + check.headSha === advisory.headSha && + check.status === "completed", + ), + ) + .then((check) => check?.conclusion) + .catch(() => undefined) + : undefined; + // Set at whichever of this pass's gate-check publish sites actually fires, to the SAME conclusion value + // passed to recordPublishedGateCheckSummary there -- compared against priorGateCheckConclusion above when + // surfaceContentChanged is computed further down, instead of re-deriving (and risking drifting from) the + // conclusion actually recorded. + let finalGateCheckConclusion: string | null | undefined; let pendingGateCheckRunId: number | undefined; if (gateEnabled) { const pendingGateResult = await createOrUpdatePendingGateCheckRun( @@ -9959,16 +9984,17 @@ async function maybePublishPrPublicSurface( if (gateCheckResult?.kind === "published") { gateFinalized = true; publishedOutputs.push("gate_check_run"); + /* v8 ignore next -- gate-enabled publication always has a gate evaluation. */ + finalGateCheckConclusion = + autoReviewSkipReason && !publicSurfaceSkipped && gateEvaluation?.conclusion === "success" + ? "skipped" + : (gateEvaluation?.conclusion ?? null); await recordPublishedGateCheckSummary(env, { repoFullName, pullNumber: pr.number, headSha: advisory.headSha, checkRunId: gateCheckResult.id, - /* v8 ignore next -- gate-enabled publication always has a gate evaluation. */ - conclusion: - autoReviewSkipReason && !publicSurfaceSkipped && gateEvaluation?.conclusion === "success" - ? "skipped" - : (gateEvaluation?.conclusion ?? null), + conclusion: finalGateCheckConclusion, detailsUrl: gateCheckResult.html_url, deliveryId: webhook.deliveryId, }).catch((error) => { @@ -10008,6 +10034,7 @@ async function maybePublishPrPublicSurface( if (fallbackGateCheckResult?.kind === "published") { gateFinalized = true; publishedOutputs.push("gate_check_run"); + finalGateCheckConclusion = "neutral"; await recordPublishedGateCheckSummary(env, { repoFullName, pullNumber: pr.number, @@ -10049,6 +10076,7 @@ async function maybePublishPrPublicSurface( if (fallbackGateCheckResult?.kind === "published") { gateFinalized = true; publishedOutputs.push("gate_check_run"); + finalGateCheckConclusion = "neutral"; await recordPublishedGateCheckSummary(env, { repoFullName, pullNumber: pr.number, @@ -10785,11 +10813,22 @@ async function maybePublishPrPublicSurface( } } // #6724 (review-burst): only a PUBLISHED-OUTPUTS SET of exactly comment/label (both proven no-ops, or absent - // entirely) counts as a no-op pass. Any other output kind in this pass (gate_check_run, check_run) has no - // cheap no-op signal, so its mere presence keeps this true -- conservative by design, this can only ever + // entirely) counts as a no-op pass. `check_run` (the separate advisory check, distinct from the gate one) has + // no cheap no-op signal, so its mere presence keeps this true -- conservative by design, this can only ever // suppress a pr_public_surface_published record for a pass PROVEN to have changed nothing, never the reverse. + // `gate_check_run` DOES have one (#ops-review-burst): finalGateCheckConclusion/priorGateCheckConclusion above + // -- a stuck-CI-finalize loop that keeps re-reviewing the SAME unchanged head SHA republishes the IDENTICAL + // gate conclusion every pass (confirmed live: repeated "review burst" ops_anomaly alerts, 6+ published + // surfaces in 2h for one PR with no real state change), which this now proves a no-op rather than always + // counting gate_check_run's mere presence as a change. Any doubt -- no gate_check_run this pass, no prior row + // for this exact head (a genuinely new commit), or a differing conclusion -- still counts as changed. + const gateCheckRunContentChanged = + !publishedOutputs.includes("gate_check_run") || + priorGateCheckConclusion === undefined || + finalGateCheckConclusion !== priorGateCheckConclusion; const surfaceContentChanged = - publishedOutputs.some((output) => output !== "comment" && output !== "label") || + publishedOutputs.some((output) => output !== "comment" && output !== "label" && output !== "gate_check_run") || + (publishedOutputs.includes("gate_check_run") && gateCheckRunContentChanged) || (publishedOutputs.includes("comment") && commentContentChanged) || (publishedOutputs.includes("label") && labelContentChanged); return finishPublicSurfacePublication(surfaceContentChanged); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9429f33646..45bde65aed 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6022,11 +6022,8 @@ describe("queue processors", () => { AI_DAILY_NEURON_BUDGET: "100000", }); await seedRegateChurnRepo(env, { publicSurface: "comment_and_label" }); - // reviewCheckMode: "disabled" isolates the comment/label no-op path this test is about -- with the gate - // check-run enabled (as most gateEnabled repos are), gate_check_run publishes fresh every pass regardless - // of comment/label content, which correctly keeps surfaceContentChanged true by this fix's own conservative - // design (only a PUBLISHED-OUTPUTS SET of exactly comment/label can ever be marked unchanged). Extending a - // similar no-op signal to gate_check_run is a separate, not-yet-implemented scope -- see the PR description. + // reviewCheckMode: "disabled" isolates the comment/label no-op path this test is about, independent of + // gate_check_run's own (now also implemented -- see #ops-review-burst below) no-op detection. await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "off", reviewCheckMode: "disabled", aiReviewMode: "block" }, review: { auto_review: { cadence: "continuous" } } }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 71, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: "a71" }, labels: [], body: "Closes #1" }); await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 71, status: "complete", reviewsSyncedAt: new Date().toISOString() }); @@ -6096,7 +6093,7 @@ describe("queue processors", () => { expect(noopAudit?.detail).toContain("no visible change"); }); - it("#6724 (review-burst): a comment-unchanged pass that newly applies a previously-missing label still records a fresh pr_public_surface_published", async () => { + it("#6724 (review-burst): the UNRELATED type-label mechanism acting on its own does not, by itself, count as the tracked review surface changing", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), AI: { run: async () => ({ response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, @@ -6134,10 +6131,14 @@ describe("queue processors", () => { stickyComment.current = { id: 1, body }; return Response.json({ id: 1 }, { status: 200 }); } - // The label is missing on the first pass (proving the FIRST publish is real), then present by the second - // -- but this test simulates it being manually removed again right before the second pass, so the second - // pass's own POST is what flips labelPresent back to true, proving a genuinely NEW label application - // still counts as a real change even though the comment itself renders byte-identical. + // #ops-review-burst (test correction): this repo config never actually reaches decision.willLabel (the + // "gittensor" review-status label is never applied in this test at all -- confirmed by direct + // instrumentation, not asserted here since it's a repo-config detail, not this test's point). Every + // POST to this endpoint across both passes is the UNRELATED type-label mechanism (resolvePrTypeLabel, + // deriving "gittensor:bug" from the title) re-applying itself whenever the GET shows no labels -- it + // runs unconditionally, outside surfaceContentChanged's tracked outputs entirely, both before and after + // this fix. This test's actual point: that activity alone must not count as the TRACKED review surface + // (comment/gittensor-label/gate-check) having changed. if (url.includes("/issues/72/labels") && method === "GET") return Response.json(labelPresent ? [{ name: "gittensor" }] : []); if (url.includes("/issues/72/labels") && method === "POST") { labelPosts += 1; @@ -6150,19 +6151,19 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-pr", deliveryId: "webhook-open", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123 }); expect(labelPosts).toBe(1); - labelPresent = false; // simulate a maintainer/bot removing the label between passes + labelPresent = false; // simulate the type label being manually removed between passes await processJob(env, { type: "agent-regate-pr", deliveryId: "regate-sweep:JSONbored/gittensory#72:1", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123 }); - expect(labelPosts).toBe(2); // label repair re-applied it -- a real change this pass + expect(labelPosts).toBe(2); // the type-label mechanism re-applied it -- real GitHub activity, just not TRACKED activity const published = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") .bind("github_app.pr_public_surface_published", "JSONbored/gittensory#72") .first<{ n: number }>(); - expect(published?.n).toBe(2); // both passes are real publishes -- comment unchanged, but the label DID change + expect(published?.n).toBe(1); // only the first pass is a real publish -- comment AND gate conclusion are both unchanged on the second const noopAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") .bind("github_app.pr_public_surface_republish_noop", "JSONbored/gittensory#72") .first<{ n: number }>(); - expect(noopAudit?.n).toBe(0); + expect(noopAudit?.n).toBe(1); // the type-label POST alone does not defeat the no-op guard }); it("#6724 (review-burst): a null comment-publish result (the createIfMissing:false shape, structurally unreachable at this call site but still part of the return type) defaults commentContentChanged to true, not a false no-op", async () => { @@ -6208,6 +6209,150 @@ describe("queue processors", () => { expect(noopAudit?.n).toBe(0); }); + it("#ops-review-burst: gate_check_run's own byte-identical no-op now suppresses a republish too, closing the gap #6724 left open for gateEnabled repos", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env, { publicSurface: "comment_and_label" }); + // Unlike the genuinely-full-no-op test above (reviewCheckMode: "disabled", isolating comment/label), THIS + // test enables the gate check run itself, exercising gateCheckRunContentChanged directly. + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "block" }, review: { auto_review: { cadence: "continuous" } } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 74, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: "a74" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 74, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + + const stickyComment: { current: { id: number; body: string } | null } = { current: null }; + let commentPatches = 0; + let checkRunWrites = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/pulls/74/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/74")) return Response.json({ number: 74, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: "a74" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a74/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a74/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/74/comments") && method === "GET") { + return Response.json(stickyComment.current ? [{ ...stickyComment.current, user: { login: "loopover-orb[bot]", type: "Bot" } }] : []); + } + if (url.includes("/issues/74/comments") && method === "POST") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/comments/1") && method === "PATCH") { + commentPatches += 1; + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 200 }); + } + // Both labels this PR would get are already present from the first pass onward, same as the genuinely- + // full-no-op test above. + if (url.includes("/issues/74/labels") && method === "GET") return Response.json([{ name: "gittensor" }, { name: "gittensor:bug" }]); + if (url.includes("/issues/74/labels") && method === "POST") return Response.json([]); + // The pending (in_progress) post AND the completed post/patch both land here -- a stable verdict across + // passes means every one of these calls carries the SAME conclusion, which is exactly what this test + // is proving gets recognized as a no-op now. + if (url.includes("/check-runs") && (method === "POST" || method === "PATCH")) { + checkRunWrites += 1; + return Response.json({ id: 501, html_url: "https://github.com/checks/501" }, { status: method === "POST" ? 201 : 200 }); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "webhook-open", repoFullName: "JSONbored/gittensory", prNumber: 74, installationId: 123 }); + expect(aiCalls).toBeGreaterThan(0); + expect(checkRunWrites).toBeGreaterThan(0); // the gate check DID publish this first pass + const publishedAfterFirst = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_public_surface_published", "JSONbored/gittensory#74") + .first<{ n: number }>(); + expect(publishedAfterFirst?.n).toBe(1); // the first pass IS a real publish + const patchesAfterFirst = commentPatches; + + // A later scheduled regate-sweep pass over the SAME unchanged head: AI cache hit, comment byte-identical, + // labels already present, AND the gate re-evaluates to the exact same conclusion as last time. + await processJob(env, { type: "agent-regate-pr", deliveryId: "regate-sweep:JSONbored/gittensory#74:1", repoFullName: "JSONbored/gittensory", prNumber: 74, installationId: 123 }); + + expect(commentPatches).toBe(patchesAfterFirst); // no additional PATCH -- the re-render is byte-identical + const publishedAfterSecond = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_public_surface_published", "JSONbored/gittensory#74") + .first<{ n: number }>(); + expect(publishedAfterSecond?.n).toBe(1); // NOT a second durable publish record -- this fix's exact target + const noopAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_public_surface_republish_noop", "JSONbored/gittensory#74") + .first<{ n: number }>(); + expect(noopAudit?.n).toBe(1); + }); + + it("#ops-review-burst: a genuinely new commit (different head SHA) is never suppressed, even with an identical gate conclusion", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env, { publicSurface: "comment_and_label" }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "block" }, review: { auto_review: { cadence: "continuous" } } }); + let headSha = "b75a"; + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 75, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 75, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + + let checkRunWrites = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes(`/pulls/75/files`)) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/75")) return Response.json({ number: 75, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes(`/commits/${headSha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes(`/commits/${headSha}/status`)) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/75/comments")) return Response.json(method === "GET" ? [] : { id: 1 }, { status: method === "GET" ? 200 : 201 }); + if (url.includes("/issues/comments/1")) return Response.json({ id: 1 }, { status: 200 }); + if (url.includes("/issues/75/labels") && method === "GET") return Response.json([{ name: "gittensor" }, { name: "gittensor:bug" }]); + if (url.includes("/issues/75/labels") && method === "POST") return Response.json([]); + if (url.includes("/check-runs") && (method === "POST" || method === "PATCH")) { + checkRunWrites += 1; + return Response.json({ id: 601, html_url: "https://github.com/checks/601" }, { status: method === "POST" ? 201 : 200 }); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "webhook-open", repoFullName: "JSONbored/gittensory", prNumber: 75, installationId: 123 }); + const publishedAfterFirst = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_public_surface_published", "JSONbored/gittensory#75") + .first<{ n: number }>(); + expect(publishedAfterFirst?.n).toBe(1); + const aiCallsAfterFirst = aiCalls; + const checkRunWritesAfterFirst = checkRunWrites; + + // A genuinely NEW commit -- same AI verdict (still "Looks fine", so the gate conclusion is IDENTICAL to + // the first pass), but a DIFFERENT head SHA, which has no prior recorded gate-check row of its own. + headSha = "c75b"; + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 75, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1" }); + await processJob(env, { type: "agent-regate-pr", deliveryId: "regate-sweep:JSONbored/gittensory#75:1", repoFullName: "JSONbored/gittensory", prNumber: 75, installationId: 123 }); + + expect(aiCalls).toBeGreaterThan(aiCallsAfterFirst); // the new head SHA is never cache-reused against the old one + expect(checkRunWrites).toBeGreaterThan(checkRunWritesAfterFirst); // the new head's gate check is a fresh POST, not a no-op skip + const publishedAfterSecond = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_public_surface_published", "JSONbored/gittensory#75") + .first<{ n: number }>(); + expect(publishedAfterSecond?.n).toBe(2); // the new commit is a real publish, never suppressed + const noopAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_public_surface_republish_noop", "JSONbored/gittensory#75") + .first<{ n: number }>(); + expect(noopAudit?.n).toBe(0); + }); + it("REGRESSION (#6685): a draft PR's repeat fork-CI-completion triggers stop republishing once the head is already current", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); // reviewCheckMode: "disabled" reproduces the live incident exactly -- gateEnabled (shouldPublishReviewCheck