Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -5641,6 +5648,7 @@ async function handlePullRequestWebhookEvent(
deliveryId,
authorType: payloadPullRequest.user?.type,
action: payload.action,
eventName,
baseSha: payloadPullRequest.base?.sha ?? null,
liveFacts,
},
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down
129 changes: 129 additions & 0 deletions test/unit/queue-5.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown> }> = [
{ 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
Expand Down