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
22 changes: 22 additions & 0 deletions src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3208,6 +3208,28 @@ export async function fetchLivePullRequestState(
return result?.data.state ?? undefined;
}

/** The PR's LIVE `merged_at` via REST `GET /pulls/{n}` (#4818): a webhook whose embedded `pull_request` snapshot
* predates an imminent merge -- e.g. a `pull_request_review`/`pull_request_review_comment`/
* `pull_request_review_thread` fired a few ms before an "approve and merge" action -- can carry `merged_at:
* null` even though the PR is, by the time this pass actually runs, genuinely merged;
* `handlePullRequestWebhookEvent` never re-verifies the webhook-embedded snapshot it built `pr` from. Used
* ONLY to resolve that one ambiguous case in `resolveIssueLabelsForPropagation`
* (`review/linked-issue-label-propagation-fetch.ts`) -- a CLOSED linked issue whose closure can't yet be
* attributed to this PR from the triggering webhook's own (possibly stale) `merged_at` alone. Best-effort:
* returns undefined on any error, distinct from the confirmed `null` of a genuinely-still-open PR, so the
* caller treats a fetch failure as inconclusive rather than folding it into a confirmed negative (never fails
* toward silently stripping a correct label). */
export async function fetchLivePullRequestMergedAt(
env: Env,
repoFullName: string,
prNumber: number,
token: string | undefined,
admissionKey?: GitHubRateLimitAdmissionKey,
): Promise<string | null | undefined> {
const result = await githubJsonWithHeaders<{ merged_at?: string | null }>(env, repoFullName, `/pulls/${prNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined);
return result === undefined ? undefined : (result.data.merged_at ?? null);
}

/** The issue's LIVE state ("open" / "closed") via REST `GET /issues/{n}`. Mirrors {@link fetchLivePullRequestState}
* for issues: the stored open-issue cache lags GitHub, so a sibling closed on GitHub (or elsewhere) can still
* read `open` locally. The per-contributor open-issue cap (#2479 gate finding) confirms each counted sibling's
Expand Down
5 changes: 5 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7620,6 +7620,11 @@ async function maybePublishPrPublicSurface(
// (the standard "Closes #N" auto-close), instead of losing propagation authority the instant
// the merge that's supposed to earn the label also closes its evidence.
prMergedAt: pr.mergedAt ?? null,
// #4818: lets the ambiguous "issue closed but THIS pass's own prMergedAt reads null" case
// (a pull_request_review/_comment/_thread webhook whose embedded snapshot predates an
// imminent merge, delayed behind other queued work) resolve via one fresh live check instead
// of silently downgrading a correct label.
prNumber: pr.number,
})
: { labels: [], inconclusive: false };
// #regression-safe-propagation: an INCONCLUSIVE recheck (the linked issue's facts or the
Expand Down
65 changes: 54 additions & 11 deletions src/review/linked-issue-label-propagation-fetch.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { fetchLinkedIssueFacts, type LinkedIssueFactsFetch, type LinkedIssueFactsResult } from "../github/backfill";
import { fetchLinkedIssueFacts, fetchLivePullRequestMergedAt, type LinkedIssueFactsFetch, type LinkedIssueFactsResult } from "../github/backfill";
import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app";
import { githubRateLimitAdmissionKeyForToken } from "../github/client";
import { githubRateLimitAdmissionKeyForToken, type GitHubRateLimitAdmissionKey } from "../github/client";
import { parseGitHubLoginList } from "../auth/security";
import { errorMessage } from "../utils/json";
import type { LinkedIssueLabelPropagationMapping } from "../types";
Expand Down Expand Up @@ -120,7 +120,14 @@ export type LinkedIssuePropagationLabels = {
* confirmed `not_found` (a proven-nonexistent issue) is likewise NOT inconclusive -- that is real,
* deterministic evidence, not a hiccup. */
async function resolveIssueLabelsForPropagation(
args: { env: Env; repoFullName: string; installationId: number },
args: {
env: Env;
repoFullName: string;
installationId: number;
prNumber: number | undefined;
token: string | undefined;
admissionKey: GitHubRateLimitAdmissionKey | undefined;
},
result: LinkedIssueFactsFetch,
prAuthorLogin: string | undefined,
relaxableLabels: ReadonlySet<string>,
Expand All @@ -136,7 +143,36 @@ async function resolveIssueLabelsForPropagation(
);
return { labels: [], inconclusive: true };
}
if (result.status !== "found" || !isLinkedIssueTrustworthy(result.facts, prMergedAt) || !prAuthorLogin) return { labels: [], inconclusive: false };
if (result.status !== "found" || !prAuthorLogin) return { labels: [], inconclusive: false };
let trustedMergedAt = prMergedAt;
// #4818 (#regression-safe-propagation): a null `prMergedAt` on a CLOSED linked issue is AMBIGUOUS, not a
// confirmed negative -- it means either "this PR genuinely isn't merged yet" (the real anti-gaming case
// #4528 exists to block: an unrelated, already-resolved issue opportunistically cited by a still-open PR)
// OR "this pass's own triggering webhook happened to be a pull_request_review/_comment/_thread whose
// embedded `pull_request` snapshot was taken a few ms before an imminent merge, then this pass got delayed
// in the queue long enough for the real merge (and the issue's consequent auto-close) to land first."
// Those two cases are indistinguishable from `prMergedAt` alone -- `handlePullRequestWebhookEvent` never
// re-verifies the webhook-embedded snapshot it built `pr` from (`src/queue/processors.ts`). Resolve the
// ambiguity with ONE fresh, authoritative read of THIS PR's own live merge state (never inferred from
// whichever webhook happened to trigger this particular pass) before deciding -- only when the issue is
// confirmed closed with a real `closedAt` (a genuinely still-open issue never reaches here at all, per
// {@link isLinkedIssueTrustworthy}'s own open-state short-circuit) and a PR number is available to check.
if (trustedMergedAt === null && result.facts.state !== "open" && result.facts.closedAt !== null && args.prNumber !== undefined) {
const liveMergedAt = await fetchLivePullRequestMergedAt(args.env, args.repoFullName, args.prNumber, args.token, args.admissionKey);
if (liveMergedAt === undefined) {
console.log(
JSON.stringify({
event: "linked_issue_label_propagation_inconclusive",
repoFullName: args.repoFullName,
issueNumber: result.facts.number,
reason: "live_merge_state_check_failed",
}),
);
return { labels: [], inconclusive: true };
}
trustedMergedAt = liveMergedAt;
}
if (!isLinkedIssueTrustworthy(result.facts, trustedMergedAt)) return { labels: [], inconclusive: false };
const allLabels = result.facts.labels;
const issueAuthorLogin = result.facts.authorLogin?.toLowerCase();
const assignees = result.facts.assignees.map((login) => login.toLowerCase());
Expand Down Expand Up @@ -197,14 +233,20 @@ async function resolveIssueLabelsForPropagation(
* either flag) reproduces today's strict author-or-assignee-only behavior exactly.
*
* `prMergedAt` (#4528) is this PR's own `merged_at`, or `null` while unmerged -- the caller's `pr.mergedAt`
* straight from the DB row, no extra fetch.
* straight from the DB row (or webhook payload), no extra fetch in the common case.
*
* `prNumber` (#4818, optional) unlocks ONE extra live fetch, only in the narrow ambiguous case
* {@link resolveIssueLabelsForPropagation} documents (a CLOSED linked issue whose closure this pass's own
* `prMergedAt` reads null): omitting it reproduces the pre-#4818 behavior exactly (a confirmed negative,
* never ambiguity-checked) -- production always passes it (`src/queue/processors.ts`'s `pr.number`).
*
* Returns {@link LinkedIssuePropagationLabels} (#regression-safe-propagation), NOT a bare `string[]`:
* `inconclusive` is true when ANY linked issue's resolution was inconclusive (fetch failure or an errored
* maintainer-permission check), aggregated across every linked issue with a plain OR -- deliberately
* coarse. A caller only needs to distinguish "confirmed: no propagation applies" from "could not fully
* verify this pass" when `labels` came back empty; when even one linked issue resolved with real labels,
* those labels are just as trustworthy as before regardless of a sibling issue's fetch trouble. */
* `inconclusive` is true when ANY linked issue's resolution was inconclusive (fetch failure, an errored
* maintainer-permission check, or an errored live-merge-state recheck), aggregated across every linked
* issue with a plain OR -- deliberately coarse. A caller only needs to distinguish "confirmed: no
* propagation applies" from "could not fully verify this pass" when `labels` came back empty; when even one
* linked issue resolved with real labels, those labels are just as trustworthy as before regardless of a
* sibling issue's fetch trouble. */
export async function fetchLinkedIssueLabelsForPropagation(args: {
env: Env;
repoFullName: string;
Expand All @@ -213,6 +255,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: {
prAuthorLogin: string | null | undefined;
mappings?: readonly LinkedIssueLabelPropagationMapping[] | undefined;
prMergedAt?: string | null | undefined;
prNumber?: number | undefined;
}): Promise<LinkedIssuePropagationLabels> {
if (args.linkedIssues.length === 0) return { labels: [], inconclusive: false };
const linkedIssues = args.linkedIssues.slice(0, MAX_LINKED_ISSUES_TO_FETCH);
Expand Down Expand Up @@ -257,7 +300,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: {
const perIssueResults = await Promise.all(
results.map((result) =>
resolveIssueLabelsForPropagation(
{ env: args.env, repoFullName: args.repoFullName, installationId: args.installationId },
{ env: args.env, repoFullName: args.repoFullName, installationId: args.installationId, prNumber: args.prNumber, token, admissionKey },
result,
prAuthorLogin,
relaxableLabels,
Expand Down
115 changes: 115 additions & 0 deletions test/unit/linked-issue-label-propagation-fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,121 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", (
});
});

describe("webhook-race live-recheck (#4818 — a null prMergedAt from a stale webhook snapshot is ambiguous, not confirmed)", () => {
it("REGRESSION (PR #4818 shape): propagates when prMergedAt reads null (a pull_request_review webhook's stale pre-merge snapshot) but a live check confirms the PR is actually merged at/before the issue's closedAt", async () => {
stubFetch((url) => {
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.endsWith("/issues/2192"))
return Response.json({
number: 2192,
state: "closed",
closed_at: "2026-07-11T02:26:25Z",
user: { login: "owner" },
labels: ["gittensor:feature"],
});
if (url.endsWith("/pulls/4818")) return Response.json({ merged_at: "2026-07-11T02:26:24Z" });
return new Response("not found", { status: 404 });
});
const env = createTestEnv({});
const result = await fetchLinkedIssueLabelsForPropagation({
env,
repoFullName: "owner/repo",
linkedIssues: [2192],
installationId: 123,
prAuthorLogin: "contrib",
mappings: [{ issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true, trustMaintainerAuthoredIssue: true }],
prMergedAt: null,
prNumber: 4818,
});
expectPropagation(result, ["gittensor:feature"]);
});

it("does not propagate when the live recheck confirms the PR is genuinely still unmerged (the real anti-gaming case #4528 protects)", async () => {
stubFetch((url) => {
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.endsWith("/issues/777"))
return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] });
if (url.endsWith("/pulls/42")) return Response.json({ merged_at: null });
return new Response("not found", { status: 404 });
});
const env = createTestEnv({});
const result = await fetchLinkedIssueLabelsForPropagation({
env,
repoFullName: "owner/repo",
linkedIssues: [777],
installationId: 123,
prAuthorLogin: "contrib",
prMergedAt: null,
prNumber: 42,
});
expectPropagation(result, []);
});

it("does not propagate when the live recheck confirms the PR merged AFTER the issue's own independent closedAt (still not this PR's own close)", async () => {
stubFetch((url) => {
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.endsWith("/issues/777"))
return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] });
if (url.endsWith("/pulls/42")) return Response.json({ merged_at: "2026-07-05T00:00:00Z" });
return new Response("not found", { status: 404 });
});
const env = createTestEnv({});
const result = await fetchLinkedIssueLabelsForPropagation({
env,
repoFullName: "owner/repo",
linkedIssues: [777],
installationId: 123,
prAuthorLogin: "contrib",
prMergedAt: null,
prNumber: 42,
});
expectPropagation(result, []);
});

it("flags the result inconclusive (never a confirmed absence) when the live merge-state recheck itself fails to fetch", async () => {
stubFetch((url) => {
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.endsWith("/issues/777"))
return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] });
if (url.endsWith("/pulls/42")) return new Response("server error", { status: 500 });
return new Response("not found", { status: 404 });
});
const env = createTestEnv({});
const result = await fetchLinkedIssueLabelsForPropagation({
env,
repoFullName: "owner/repo",
linkedIssues: [777],
installationId: 123,
prAuthorLogin: "contrib",
prMergedAt: null,
prNumber: 42,
});
expectPropagation(result, [], true);
});

it("does not attempt a live recheck (and stays a confirmed negative, byte-identical to pre-#4818 behavior) when the caller omits prNumber", async () => {
const fetchSpy = vi.fn(async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.endsWith("/issues/777"))
return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] });
return new Response("not found", { status: 404 });
});
vi.stubGlobal("fetch", fetchSpy);
const env = createTestEnv({});
const result = await fetchLinkedIssueLabelsForPropagation({
env,
repoFullName: "owner/repo",
linkedIssues: [777],
installationId: 123,
prAuthorLogin: "contrib",
prMergedAt: null,
});
expectPropagation(result, []);
expect(fetchSpy.mock.calls.some(([input]) => input.toString().includes("/pulls/"))).toBe(false);
});
});

it("does not propagate labels when the PR author is missing", async () => {
stubFetch((url) => {
if (url.includes("/access_tokens"))
Expand Down