Skip to content

fix(review): re-verify duplicate-close justification live before it executes #3930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 7, 2026
Merged

fix(review): re-verify duplicate-close justification live before it executes #3930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 34 additions & 8 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3322,6 +3322,7 @@ async function reReviewStoredPullRequest(
repo,
settings,
advisory,
otherOpenPullRequests,
{
deliveryId,
baseSha: live?.base?.sha ?? null,
Expand Down Expand Up @@ -5881,6 +5882,7 @@ async function processGitHubWebhook(
repo,
settings,
advisory,
otherOpenPullRequests,
{
deliveryId,
authorType: payloadPullRequest.user?.type,
Expand Down Expand Up @@ -7676,6 +7678,12 @@ async function maybePublishPrPublicSurface(
repo: Awaited<ReturnType<typeof getRepository>>,
settings: Awaited<ReturnType<typeof getRepositorySettings>>,
advisory: Awaited<ReturnType<typeof buildPullRequestAdvisory>>,
// The SAME live-reconciled open siblings the gate's own duplicate-close decision uses (reconcileLiveDuplicateSiblings,
// built by the caller before this function runs — see reReviewStoredPullRequest / handlePullRequestWebhook /
// buildAuthorizedPrActionAdvisory). Threaded in rather than re-derived so the duplicate-winner election below
// (#dup-winner) agrees with the gate BY CONSTRUCTION instead of computing its own, separately-stale answer
// from a raw, un-reconciled DB read (#dup-winner-slop-drift).
otherOpenPullRequests: PullRequestRecord[],
webhook: {
deliveryId: string;
authorType?: string | undefined;
Expand Down Expand Up @@ -8305,13 +8313,16 @@ async function maybePublishPrPublicSurface(
registryEverSynced,
);
// Duplicate-winner adjudication (#dup-winner): compute the winner ONCE for this review run from the SAME
// open-only sibling source the gate uses, and thread the flag/result consistently into readiness, the slop
// penalty (below), and the public panel builders (further down) so they agree by construction. Flag-OFF
// (default) ⇒ duplicateWinnerEnabled is false and isDupWinner is false ⇒ every guard short-circuits
// (byte-identical).
// live-RECONCILED open-only sibling source the gate's own close decision uses (otherOpenPullRequests, threaded
// in by the caller via reconcileLiveDuplicateSiblings — NOT repoPullRequests, a raw un-reconciled read of the
// cached `state` column that can still say "open" for a sibling GitHub already closed, e.g. a missed/delayed
// webhook), and thread the flag/result consistently into readiness, the slop penalty (below), and the public
// panel builders (further down) so they agree by construction with the actual close decision
// (#dup-winner-slop-drift). Flag-OFF (default) ⇒ duplicateWinnerEnabled is false and isDupWinner is false ⇒
// every guard short-circuits (byte-identical).
const linkedDuplicatePrsForGate = linkedIssueDuplicatePullRequestRecordsForGate(
pr,
repoPullRequests,
otherOpenPullRequests,
);
const duplicateWinnerEnabled = env.GITTENSORY_DUPLICATE_WINNER === "true";
const isDupWinner =
Expand Down Expand Up @@ -10638,7 +10649,7 @@ async function maybeProcessPrPanelRetrigger(
return true;
}

const { repo, advisory } = await buildAuthorizedPrActionAdvisory(
const { repo, advisory, otherOpenPullRequests } = await buildAuthorizedPrActionAdvisory(
env,
repoFullName,
pr,
Expand Down Expand Up @@ -10694,6 +10705,7 @@ async function maybeProcessPrPanelRetrigger(
repo,
settings,
advisory,
otherOpenPullRequests,
{
deliveryId,
action: "manual_retrigger",
Expand Down Expand Up @@ -10825,11 +10837,25 @@ export async function buildAuthorizedPrActionAdvisory(
): Promise<{
repo: Awaited<ReturnType<typeof getRepository>>;
advisory: ReturnType<typeof buildPullRequestAdvisory>;
// Reconciled open siblings (#dup-winner-staleness), returned so a caller that also needs to invoke
// maybePublishPrPublicSurface (the panel-retrigger path) can reuse the SAME reconciled set instead of
// re-deriving it from a second, un-reconciled read.
otherOpenPullRequests: PullRequestRecord[];
}> {
const [repo, otherOpenPullRequests] = await Promise.all([
const [repo, cachedOtherOpenPullRequests] = await Promise.all([
getRepository(env, repoFullName),
listOtherOpenPullRequests(env, repoFullName, pr.number),
]);
// #dup-winner-staleness: reconcile the cached open-PR read against GitHub's live state, same as the main
// webhook path (reReviewStoredPullRequest / handlePullRequestWebhook) -- an authorized PR action (gate-
// override / panel retrigger) must see the same ground truth, not a second, independently-stale snapshot.
const otherOpenPullRequests = await reconcileLiveDuplicateSiblings(
env,
repo?.installationId ?? null,
repoFullName,
pr,
cachedOtherOpenPullRequests,
);
// Mirror the main webhook path: thread linked-issue authors + the open-reference check so an authorized PR
// action (gate-override / panel retrigger) honors the same self-authored-linked-issue block AND stale-
// issue-link countermeasure. installationId comes from the repo record. (#self-authored-parity, #unlinked-issue-guardrail-followup)
Expand All @@ -10847,7 +10873,7 @@ export async function buildAuthorizedPrActionAdvisory(
confirmedNoOpenLinkedIssue,
linkedIssueAuthorLogins,
});
return { repo, advisory };
return { repo, advisory, otherOpenPullRequests };
}

function isCheckedPrPanelRetrigger(body: string | null | undefined): boolean {
Expand Down
33 changes: 29 additions & 4 deletions src/services/agent-action-executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { isAuthorBlacklisted } from "../settings/contributor-blacklist";
import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure";
import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord";
import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app";
import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill";
import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill";
import { githubRateLimitAdmissionKeyForToken } from "../github/client";
import { ensurePullRequestAssignee } from "../github/assignees";
import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels";
Expand Down Expand Up @@ -388,18 +388,31 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
// "Resolve conversation" on GitHub during a slow review pass clears it before this mutation runs, same as
// an unrelated PR clearing a base conflict. Same immediate, same-pass execution path gap as #3863 had.
const requiresLiveThreadRecheck = action.actionClass === "close" && action.closeKind === "heuristic" && action.closeRequiresThreadResolved === true;
if (requiresLiveCiRecheck || requiresLiveMergeableRecheck || requiresLiveThreadRecheck) {
// #dup-winner-staleness: a duplicate-justified heuristic close (closeRequiresDuplicateStillOpen === true) is
// likewise read from the planning-pass snapshot -- otherOpenPullRequests is reconciled ONCE up front
// (reconcileLiveDuplicateSiblings), before the often-slow AI-review/gate-evaluation pass runs, and never
// re-verified before this mutation. Unlike a conflict, the fact that can go stale here lives on a SIBLING
// PR (it can be closed/merged independently, asynchronously, any time after this pass started), so only a
// close that named a SPECIFIC winning sibling (duplicateWinnerPrNumber) has a cheap single-PR live signal
// to re-check; one that didn't (flag off, or an ambiguous election) has no equivalently cheap re-derivation
// and is left as a no-op here, matching closeRequiresMergeableState's own "false ⇒ skip" scoping above.
const requiresLiveDuplicateRecheck =
action.actionClass === "close" && action.closeKind === "heuristic" && action.closeRequiresDuplicateStillOpen === true && action.duplicateWinnerPrNumber !== undefined;
if (requiresLiveCiRecheck || requiresLiveMergeableRecheck || requiresLiveThreadRecheck || requiresLiveDuplicateRecheck) {
const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined);
const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId);
// mergeRequiredCiContexts(null, ...) -- no live branch-protection re-fetch here, just the maintainer's own
// configured expectedCiContexts (or null/fold-all when unset), matching the "no branch protection" arm of
// the planning pass's own merge (mergeRequiredCiContexts is pure and already exported for that call site).
const [liveCi, liveMergeableState, liveThreadBlockers] = await Promise.all([
const [liveCi, liveMergeableState, liveThreadBlockers, liveWinnerState] = await Promise.all([
requiresLiveCiRecheck
? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, mergeRequiredCiContexts(null, ctx.expectedCiContexts), admissionKey)
: Promise.resolve(undefined),
requiresLiveMergeableRecheck ? fetchLivePullRequestMergeState(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined),
requiresLiveThreadRecheck ? fetchLiveReviewThreadBlockers(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined),
requiresLiveDuplicateRecheck
? fetchLivePullRequestState(env, ctx.repoFullName, action.duplicateWinnerPrNumber!, ciToken, admissionKey).catch(() => undefined)
: Promise.resolve(undefined),
]);
// The planner itself only ever stages a merge when ciState === "passed" exactly (reviewGood in
// agent-actions.ts; "pending" short-circuits to no actions at all upstream) -- the live re-check must
Expand Down Expand Up @@ -430,7 +443,14 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
requiresLiveThreadRecheck && liveThreadBlockers !== undefined && liveThreadBlockers.length === 0
? "the review thread(s) that justified this close are now all resolved"
: null;
const staleReason = ciStaleReason ?? mergeableStaleReason ?? threadStaleReason;
// Only a CONFIRMED non-"open" clears a duplicate-justified close -- a failed/ambiguous fetch (undefined)
// fails open exactly like the mergeable-state recheck above, so a transient GitHub hiccup never wrongly
// spares a close that is, in fact, still justified.
const duplicateStaleReason =
requiresLiveDuplicateRecheck && liveWinnerState !== undefined && liveWinnerState !== "open"
? `duplicate-cluster winner #${action.duplicateWinnerPrNumber} is no longer open`
: null;
const staleReason = ciStaleReason ?? mergeableStaleReason ?? threadStaleReason ?? duplicateStaleReason;
if (staleReason) {
await audit("denied", `${staleReason} — action not executed`);
continue;
Expand Down Expand Up @@ -881,6 +901,11 @@ export function actionParams(action: PlannedAgentAction): AgentPendingActionPara
// Round-trip the review-thread dependency likewise: only a thread-justified close needs the accept-time /
// pre-mutation live thread-blocker recheck (see the field's doc comment on AgentPendingActionParams).
...(action.closeRequiresThreadResolved !== undefined ? { closeRequiresThreadResolved: action.closeRequiresThreadResolved } : {}),
// Round-trip the duplicate-PR dependency likewise: only a duplicate-justified close needs the live
// duplicate-still-open recheck (#dup-winner-staleness, see the field's doc comment on AgentPendingActionParams).
...(action.closeRequiresDuplicateStillOpen !== undefined ? { closeRequiresDuplicateStillOpen: action.closeRequiresDuplicateStillOpen } : {}),
// Round-trip the named winning sibling so the recheck re-verifies THAT PR specifically on replay too.
...(action.duplicateWinnerPrNumber !== undefined ? { duplicateWinnerPrNumber: action.duplicateWinnerPrNumber } : {}),
// Round-trip the concrete-evidence tag so the breaker's exemption still applies when a staged close accepts.
...(action.closeConcreteEvidence !== undefined ? { closeConcreteEvidence: action.closeConcreteEvidence } : {}),
};
Expand Down
Loading