From e5d2282b975c50954089e86642bc40863ca0cea5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:26:36 -0700 Subject: [PATCH] fix(review): stop recomputing the type label on review-family webhooks pull_request_review/_comment/_thread events can never change a PR's title or its linked-issue list -- the only two inputs the type-label decision depends on -- yet each was still reaching the recompute with its own independently stale embedded PR snapshot, which is exactly the mechanism #4818 exploited. Exclude all three event families from the type-label block entirely rather than continuing to patch individually-discovered staleness paths; a genuine change is still caught by the next pull_request-native event or the sweep. Closes #4986 --- src/queue/processors.ts | 36 +++++++++-- test/unit/queue-5.test.ts | 129 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 6 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 828e74ff71..0dfed57685 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -644,6 +644,13 @@ const PR_PUBLIC_SURFACE_ACTIONS = new Set([ "edited", ]); const PR_GATE_CLOSED_ACTIONS = new Set(["closed"]); +// #4818 follow-up: the three review-family event names `shouldProcessPullRequestPublicSurface` (below) also +// routes into `maybePublishPrPublicSurface` -- none of them can ever change a PR's title or its own linked-issue +// list (the only two inputs a TYPE-label decision depends on), yet each carries its OWN independently-timed, +// independently-stale embedded `pull_request` webhook snapshot. Used ONLY to skip the type-label recompute +// itself (see `maybePublishPrPublicSurface`'s type-label block) -- every OTHER piece of the public-surface +// publish (gate re-evaluation, comments, screenshots, …) still needs to run on these events same as before. +const PR_TYPE_LABEL_IRRELEVANT_EVENT_NAMES = new Set(["pull_request_review", "pull_request_review_comment", "pull_request_review_thread"]); const ISSUE_PLAN_COOLDOWN_MS = 10 * 60 * 1000; const NOTIFY_EVALUATE_EVENTS_PER_JOB = 100; @@ -5641,6 +5648,7 @@ async function handlePullRequestWebhookEvent( deliveryId, authorType: payloadPullRequest.user?.type, action: payload.action, + eventName, baseSha: payloadPullRequest.base?.sha ?? null, liveFacts, }, @@ -7331,6 +7339,12 @@ async function maybePublishPrPublicSurface( deliveryId: string; authorType?: string | undefined; action?: string | undefined; + // #4818 follow-up: the GitHub webhook event name (`pull_request`, `pull_request_review`, …), distinct from + // `action` above -- `action: "edited"` alone can't tell a `pull_request` title edit apart from a + // `pull_request_review` comment edit, and only the type-label block needs this distinction (see + // `PR_TYPE_LABEL_IRRELEVANT_EVENT_NAMES`). Omitted (sweep / manual-retrigger callers) is never in that + // set, so those paths are unaffected. + eventName?: string | undefined; baseSha?: string | null | undefined; previewPollAttempt?: number | undefined; skipAiReview?: boolean | undefined; @@ -7576,11 +7590,19 @@ async function maybePublishPrPublicSurface( // label, so a type label would violate it same as a comment would. `typeLabelsEnabled` itself is // computed earlier (see its declaration above prelimHasPublicOutput) so gittensor_only's silence // promise can gate the public-surface computation too, not just this label decision (#gate-only-type-labels). + // #4818 follow-up: skip the recompute ENTIRELY (not merely the ambiguous branch) for a review-family + // trigger -- it can't legitimately change the answer, and its embedded PR snapshot is exactly the class of + // stale input that caused #4818. Nothing is lost, only deferred to the next pull_request-native event or the + // periodic sweep (which reaches this same code with `eventName` unset, so it is never excluded). Computed + // once and reused by both the gate below and the skip-reason ternary in the `else` branch, rather than + // re-evaluating `webhook.eventName ?? ""` twice for the identical answer. + const isReviewFamilyEvent = PR_TYPE_LABEL_IRRELEVANT_EVENT_NAMES.has(webhook.eventName ?? ""); if ( typeLabelsEnabled && !settings.agentPaused && decision.skipReason !== "miner_detection_unavailable" && - decision.skipReason !== "not_official_gittensor_miner" + decision.skipReason !== "not_official_gittensor_miner" && + !isReviewFamilyEvent ) { // Per-PR mutual exclusion (#regression-safe-propagation, mirrors the agent-maintenance claim at #2129 // below in maybeRunAgentMaintenance): a merge fans out into a BURST of near-simultaneous webhook @@ -7709,11 +7731,13 @@ async function maybePublishPrPublicSurface( } } } else { - const skipReason = settings.agentPaused - ? "agent_paused" - : decision.skipReason === "miner_detection_unavailable" || decision.skipReason === "not_official_gittensor_miner" - ? decision.skipReason - : "typeLabelsEnabled_false"; + const skipReason = isReviewFamilyEvent + ? "irrelevant_review_family_event" + : settings.agentPaused + ? "agent_paused" + : decision.skipReason === "miner_detection_unavailable" || decision.skipReason === "not_official_gittensor_miner" + ? decision.skipReason + : "typeLabelsEnabled_false"; await logTypeLabelSkip(env, repoFullName, pr.number, skipReason); } diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 9132ff76da..bcde0b412d 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -5832,6 +5832,135 @@ describe("queue processors", () => { expect(events.results).toEqual([{ outcome: "denied", detail: "lock_contended" }]); }); + describe("review-family events never touch the type label (#4818 follow-up)", () => { + const REVIEW_FAMILY_WEBHOOKS: Array<{ eventName: string; action: string; extra?: Record }> = [ + { eventName: "pull_request_review", action: "submitted", extra: { review: { state: "approved", user: { login: "maintainer" }, submitted_at: "2026-07-11T02:26:36.000Z" } } }, + { eventName: "pull_request_review_comment", action: "created", extra: { comment: { id: 1, user: { login: "maintainer" } } } }, + { eventName: "pull_request_review_thread", action: "resolved", extra: { thread: { comments: [] } } }, + ]; + + for (const webhook of REVIEW_FAMILY_WEBHOOKS) { + it(`skips the type-label recompute entirely (never even fetches the linked issue) on a ${webhook.eventName}:${webhook.action} webhook`, async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "label_only", + autoLabelEnabled: true, + createMissingLabel: false, + checkRunMode: "off", + gateCheckMode: "enabled", reviewCheckMode: "required", + linkedIssueGateMode: "off", + aiReviewMode: "off", + linkedIssueLabelPropagation: { + enabled: true, + mode: "exclusive_type_label", + mappings: [{ issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true }], + }, + }); + const seen = { posted: [] as string[], removed: [] as string[], issueFetches: 0 }; + stubPropagationFetch(4818, 2192, seen, () => Response.json({ number: 2192, state: "closed", closed_at: "2026-07-11T02:26:25Z", user: { login: "JSONbored" }, labels: ["gittensor:feature"] })); + + await processJob(env, { + type: "github-webhook", + deliveryId: `review-family-skip-${webhook.eventName}`, + eventName: webhook.eventName, + payload: { + action: webhook.action, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 4818, + title: "feat(ui): confidence-calibration curve card on the analytics dashboard", + state: "open", + // The exact #4818 shape: this event's own embedded snapshot predates the merge (null merged_at) + // even though the linked issue is already closed -- if this reached the label block at all, it + // would hit the ambiguous branch. It must never get that far. + merged_at: null, + user: { login: "andriypolanski" }, + author_association: "NONE", + head: { sha: "sha4818" }, + labels: [], + body: "Closes #2192", + }, + ...(webhook.extra ?? {}), + }, + }); + + expect(seen.issueFetches).toBe(0); + expect(seen.posted).toEqual([]); + expect(seen.removed).toEqual([]); + const events = await env.DB.prepare( + `select outcome, detail from audit_events where event_type = 'github_app.type_label_decision' and target_key = 'JSONbored/gittensory#4818'`, + ).all(); + expect(events.results).toEqual([{ outcome: "denied", detail: "irrelevant_review_family_event" }]); + }); + } + + it("REGRESSION (#4818 shape): a pull_request_review webhook leaves an already-correctly-propagated label untouched, where a same-shaped pull_request webhook would have hit the ambiguous branch", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "widget", full_name: "acme/widget", private: false, owner: { login: "acme" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "acme/widget", + commentMode: "off", + publicSurface: "label_only", + autoLabelEnabled: true, + createMissingLabel: false, + checkRunMode: "off", + gateCheckMode: "enabled", reviewCheckMode: "required", + linkedIssueGateMode: "off", + aiReviewMode: "off", + linkedIssueLabelPropagation: { + enabled: true, + mode: "exclusive_type_label", + mappings: [{ issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true }], + }, + }); + const seen = { posted: [] as string[], removed: [] as string[], issueFetches: 0 }; + stubPropagationFetch(9001, 501, seen, () => Response.json({ number: 501, state: "open", user: { login: "contributor" }, labels: ["gittensor:feature"] })); + + // Pass 1: PR opened while the issue is still open -- propagates gittensor:feature correctly. + await processJob(env, { + type: "github-webhook", + deliveryId: "pass-1-opened", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "acme", id: 1, type: "User" } }, + repository: { name: "widget", full_name: "acme/widget", private: false, owner: { login: "acme" } }, + pull_request: { number: 9001, title: "fix: some bug", state: "open", user: { login: "contributor" }, author_association: "NONE", head: { sha: "sha9001a" }, labels: [], body: "Closes #501" }, + }, + }); + expect(seen.posted).toEqual(["gittensor:feature"]); + // Pass 1's own mutual-exclusivity cleanup (feature applied -> bug/priority removed from the type-label + // set) -- captured here so pass 2's assertions below can prove it added NOTHING further, not just that + // the array happens to be empty. + const removedAfterPass1 = [...seen.removed].sort(); + expect(removedAfterPass1).toEqual(["gittensor:bug", "gittensor:priority"]); + + // Pass 2: a review submitted with a stale, pre-merge embedded snapshot (merged_at: null) arriving after + // the issue has since closed -- the exact #4818 race. Must be skipped entirely, not reach the ambiguous + // branch (which a same-shaped pull_request webhook would). + await processJob(env, { + type: "github-webhook", + deliveryId: "pass-2-stale-review", + eventName: "pull_request_review", + payload: { + action: "submitted", + installation: { id: 123, account: { login: "acme", id: 1, type: "User" } }, + repository: { name: "widget", full_name: "acme/widget", private: false, owner: { login: "acme" } }, + pull_request: { number: 9001, title: "fix: some bug", state: "open", merged_at: null, user: { login: "contributor" }, author_association: "NONE", head: { sha: "sha9001a" }, labels: [], body: "Closes #501" }, + review: { state: "approved", user: { login: "maintainer" }, submitted_at: "2026-07-11T02:26:36.000Z" }, + }, + }); + + // Still exactly the one post + the one cleanup from pass 1 -- pass 2 added nothing further. + expect(seen.posted).toEqual(["gittensor:feature"]); + expect(seen.removed).toEqual(removedAfterPass1); + }); + }); + it("never fetches a linked issue and keeps normal behavior when propagation is left at its default (disabled) (#priority-linked-issue-gate)", async () => { // Deliberately NOT "JSONbored/gittensory" (unlike its two sibling tests above): this repo's own // `.gittensory.yml` now enables propagation for itself (#priority-linked-issue-gate-ownership