From 3b29712677a1a31f17b271c62473615b7ccd7878 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 23:59:08 -0700 Subject: [PATCH] fix(review): re-verify duplicate-close justification live before it executes A duplicate-justified heuristic close (linkedDuplicateCount > 0) had no live recheck at all, unlike merge-conflict closes (fixed by #3881/#3863) and CI-failed closes, which both already re-verify right before the mutation. The duplicate-sibling data driving the close is reconciled ONCE at planning time (reconcileLiveDuplicateSiblings), before the often-slow AI-review/gate pass runs, and is never re-checked afterward -- if the blocking sibling PR closes or merges independently during that window, this PR still gets closed for a duplicate reason that no longer holds. The close was also immune to the close-precision circuit breaker, since hasConcreteCloseEvidence treated a duplicate link as concrete, non-judgment evidence with no staleness risk. Add closeRequiresDuplicateStillOpen + duplicateWinnerPrNumber to the planned close action, mirroring closeRequiresMergeableState's own discipline, and add a live recheck in both the immediate execution path (agent-action-executor.ts) and the staged-approval accept path (agent-approval-queue.ts): when a specific winning sibling was named, re-fetch its live state right before acting and deny the close if that PR is no longer open. When no specific winner was named (duplicate-winner election disabled, or an ambiguous election), there's no cheap single-PR signal to check, so the recheck is a no-op for that case -- same scoping precedent as the mergeable- state recheck for a non-conflict close. Left linkedDuplicateCount in hasConcreteCloseEvidence's concrete-evidence set rather than removing it: the breaker exists to catch a systematically WRONG heuristic judgment call, not to guard against an otherwise-correct deterministic fact going stale between planning and actuation -- that's exactly what the new live recheck now handles, the same relationship a base conflict already has with its own live recheck. Also fixes a second, related bug in maybePublishPrPublicSurface: the duplicate-cluster slop penalty (duplicateClusterMembership, weight 15) was computed from a raw, un-reconciled listPullRequests read, completely separate from the properly-reconciled sibling set the gate's own close decision uses -- despite a comment directly above it claiming they were the same source. If a lower-numbered sibling is closed on GitHub but its cached DB row hasn't caught up (a missed/delayed webhook), this path wrongly denied the current PR winner status and applied the duplicate slop penalty, which under slopGateMode: block becomes a real gate blocker independent of duplicatePrGateMode. Thread the same reconciled otherOpenPullRequests into this function (extending buildAuthorizedPrActionAdvisory to reconcile and return it too, for the panel-retrigger call site) instead of re-deriving a second, separately-stale answer, and correct the comment to describe what's actually happening. --- src/queue/processors.ts | 42 +++++-- src/services/agent-action-executor.ts | 33 +++++- src/services/agent-approval-queue.ts | 80 ++++++++----- src/settings/agent-actions.ts | 27 +++++ src/types.ts | 26 +++-- test/unit/agent-action-executor.test.ts | 132 ++++++++++++++++++++- test/unit/agent-actions.test.ts | 22 +++- test/unit/agent-approval-queue.test.ts | 145 ++++++++++++++++++++++-- test/unit/queue.test.ts | 76 +++++++++++++ 9 files changed, 518 insertions(+), 65 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 312a7888e9..3d41f92fab 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3322,6 +3322,7 @@ async function reReviewStoredPullRequest( repo, settings, advisory, + otherOpenPullRequests, { deliveryId, baseSha: live?.base?.sha ?? null, @@ -5881,6 +5882,7 @@ async function processGitHubWebhook( repo, settings, advisory, + otherOpenPullRequests, { deliveryId, authorType: payloadPullRequest.user?.type, @@ -7676,6 +7678,12 @@ async function maybePublishPrPublicSurface( repo: Awaited>, settings: Awaited>, advisory: Awaited>, + // 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; @@ -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 = @@ -10638,7 +10649,7 @@ async function maybeProcessPrPanelRetrigger( return true; } - const { repo, advisory } = await buildAuthorizedPrActionAdvisory( + const { repo, advisory, otherOpenPullRequests } = await buildAuthorizedPrActionAdvisory( env, repoFullName, pr, @@ -10694,6 +10705,7 @@ async function maybeProcessPrPanelRetrigger( repo, settings, advisory, + otherOpenPullRequests, { deliveryId, action: "manual_retrigger", @@ -10825,11 +10837,25 @@ export async function buildAuthorizedPrActionAdvisory( ): Promise<{ repo: Awaited>; advisory: ReturnType; + // 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) @@ -10847,7 +10873,7 @@ export async function buildAuthorizedPrActionAdvisory( confirmedNoOpenLinkedIssue, linkedIssueAuthorLogins, }); - return { repo, advisory }; + return { repo, advisory, otherOpenPullRequests }; } function isCheckedPrPanelRetrigger(body: string | null | undefined): boolean { diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index ef64acd28c..338fc5a37c 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -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"; @@ -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 @@ -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; @@ -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 } : {}), }; diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 86c49cb613..0fb52bbb07 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -6,7 +6,7 @@ import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent- import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -194,12 +194,12 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // For close, scoped to closeRequiresMergeableState !== false -- i.e. `true` (a base-conflict-justified // heuristic close) OR `undefined` (a LEGACY row staged before this field existed, whose original // justification is unknown). NOT the broader closeRequiresCiState === "not_required" (any non-CI reason). - // A duplicate/slop/blocker-only close (closeRequiresMergeableState === false, always explicit per the - // field's own doc comment) has no cheap live re-derivation, so it is intentionally left out of this - // recheck. But `undefined` must NOT be treated the same as `false`: a strict `=== true` comparison would - // silently skip the live recheck for any pre-existing auto_with_approval close row staged before this - // field was introduced, even one that WAS originally conflict-justified -- exactly the safety gap this - // recheck exists to close. Fail toward "revalidate" for the unknown case, not "skip" (gate review finding). + // A slop/blocker-only close (closeRequiresMergeableState === false, always explicit per the field's own doc + // comment) has no cheap live re-derivation, so it is intentionally left out of this recheck. But `undefined` + // must NOT be treated the same as `false`: a strict `=== true` comparison would silently skip the live + // recheck for any pre-existing auto_with_approval close row staged before this field was introduced, even + // one that WAS originally conflict-justified -- exactly the safety gap this recheck exists to close. Fail + // toward "revalidate" for the unknown case, not "skip" (gate review finding). const isMergeableRecheck = pending.actionClass === "close" && pending.params.closeKind === "heuristic" && pending.params.closeRequiresMergeableState !== false; // Mirrors isMergeableRecheck's LIVE-SIGNAL shape (#review-thread-staleness) but deliberately scoped to // `=== true`, not `!== false`: unlike closeRequiresMergeableState, closeRequiresThreadResolved has NO @@ -209,32 +209,48 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // ambiguous legacy row, so there is no equivalent "fail toward revalidate" case to guard against. const isThreadRecheck = pending.actionClass === "close" && pending.params.closeKind === "heuristic" && pending.params.closeRequiresThreadResolved === true; const shouldRecheckLiveDisposition = pr?.headSha && (pending.actionClass === "merge" || isMergeableRecheck || isThreadRecheck); - if (shouldRecheckLiveDisposition) { + // #dup-winner-staleness: a duplicate-justified heuristic close naming a SPECIFIC winning sibling has its own + // cheap live signal (is that PR still open?), independent of the merge/conflict/thread rechecks above -- gated + // separately since a row can be duplicate-justified without also being conflict- or thread-justified (and + // vice versa), and does not depend on pr?.headSha the way the others do (it checks a SIBLING PR's state). + const shouldRecheckLiveDuplicateWinner = + pending.actionClass === "close" && + pending.params.closeKind === "heuristic" && + pending.params.closeRequiresDuplicateStillOpen === true && + pending.params.duplicateWinnerPrNumber !== undefined; + if (shouldRecheckLiveDisposition || shouldRecheckLiveDuplicateWinner) { const token = await createInstallationToken(env, pending.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, pending.installationId); // Promise.allSettled, not Promise.all: each live re-check is independently best-effort (per the comment // above), so ONE transient rejection must fail open on that specific check, not throw the whole accept // out of decidePendingAgentAction. A settled-rejected check is treated the same as "nothing concerning // found" -- exactly what each function's own internal fail-safe catch already resolves to on success. - const [ciResult, mergeableResult, reviewResult, threadResult] = await Promise.allSettled([ - // mergeRequiredCiContexts(null, ...) -- no live branch-protection re-fetch here, just the maintainer's own - // configured expectedCiContexts (or null/fold-all when unset), so this accept-time re-check honors the - // same required-contexts view the original plan was evaluated against (#selfhost-ci-verification). - fetchLiveCiAggregate(env, pending.repoFullName, pr.headSha, token, mergeRequiredCiContexts(null, settings.expectedCiContexts), admissionKey), - fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey), - fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey), + // The CI/mergeable/review calls are no-ops (Promise.resolve(undefined)) when shouldRecheckLiveDisposition is + // false (block entered ONLY for a duplicate-only recheck); the thread/duplicate calls are independently + // gated on their own specific flags, mirroring the executor's own same-pattern conditional-Promise.all in + // agent-action-executor.ts. + const [ciResult, mergeableResult, reviewResult, threadResult, duplicateWinnerResult] = await Promise.allSettled([ + shouldRecheckLiveDisposition + ? // mergeRequiredCiContexts(null, ...) -- no live branch-protection re-fetch here, just the maintainer's own + // configured expectedCiContexts (or null/fold-all when unset), so this accept-time re-check honors the + // same required-contexts view the original plan was evaluated against (#selfhost-ci-verification). + fetchLiveCiAggregate(env, pending.repoFullName, pr!.headSha, token, mergeRequiredCiContexts(null, settings.expectedCiContexts), admissionKey) + : Promise.resolve(undefined), + shouldRecheckLiveDisposition ? fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), + shouldRecheckLiveDisposition ? fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), isThreadRecheck ? fetchLiveReviewThreadBlockers(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), + shouldRecheckLiveDuplicateWinner ? fetchLivePullRequestState(env, pending.repoFullName, pending.params.duplicateWinnerPrNumber!, token, admissionKey) : Promise.resolve(undefined), ]); // A REJECTED promise stays undefined (fail-open — the read itself failed, not a genuine CI signal); a // FULFILLED promise reporting anything other than "passed" (failed, pending, or unverified) is a real, // non-stale-tolerant signal that the staged merge's justification no longer holds (#2126). - const ciState = ciResult.status === "fulfilled" ? ciResult.value.ciState : undefined; + const ciState = ciResult.status === "fulfilled" ? ciResult.value?.ciState : undefined; const mergeableState = mergeableResult.status === "fulfilled" ? mergeableResult.value : undefined; // Tracked separately from reviewDecision's VALUE: a REJECTED promise also resolves reviewDecision to // undefined below, which must not be indistinguishable from "fetched successfully and confirmed not // CHANGES_REQUESTED" -- otherwise a transient read failure silently satisfies the close-staleness check // below instead of failing open on it (gate review finding). - const reviewFetchSucceeded = reviewResult.status === "fulfilled"; + const reviewFetchSucceeded = reviewResult.status === "fulfilled" && shouldRecheckLiveDisposition; const reviewDecision = reviewFetchSucceeded ? reviewResult.value : undefined; // Tracked separately from the VALUE for the same reason as reviewFetchSucceeded above: a REJECTED promise // also resolves to undefined, which must not read as "confirmed no threads remain" -- fetchLiveReviewThreadBlockers @@ -243,12 +259,16 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de const threadFetchSucceeded = threadResult.status === "fulfilled"; const liveThreadBlockers = threadFetchSucceeded ? threadResult.value : undefined; const threadsNowResolved = isThreadRecheck && threadFetchSucceeded && (liveThreadBlockers?.length ?? 0) === 0; - // Gated on isMergeableRecheck explicitly (not just "reached the close branch"): a thread-only close - // (isThreadRecheck true, isMergeableRecheck false) also reaches this branch now, and mergeableState reads - // "clean" for most never-conflicted PRs by default -- without this gate, a thread-only close would be - // wrongly superseded as if it were conflict-justified merely because mergeability happens to read clean - // (the SAME over-broad-predicate class the #2478 gate review already caught once for closeRequiresMergeableState). + // Gated on isMergeableRecheck explicitly (not just "reached the close branch"): a thread- or duplicate-only + // close also reaches this branch now, and mergeableState reads "clean" for most never-conflicted PRs by + // default -- without this gate, a thread- or duplicate-only close would be wrongly superseded as if it were + // conflict-justified merely because mergeability happens to read clean (the SAME over-broad-predicate class + // the #2478 gate review already caught once for closeRequiresMergeableState). const mergeableNowCleared = isMergeableRecheck && reviewFetchSucceeded && mergeableState === "clean" && reviewDecision !== "CHANGES_REQUESTED"; + // Only a CONFIRMED non-"open" clears a duplicate-justified close -- a rejected/failed fetch (undefined) + // fails open exactly like every other live re-check in this function, so a transient GitHub hiccup never + // wrongly spares a close that is, in fact, still justified. + const duplicateWinnerState = duplicateWinnerResult.status === "fulfilled" ? duplicateWinnerResult.value : undefined; const staleReason = pending.actionClass === "merge" ? ciState !== undefined && ciState !== "passed" @@ -258,18 +278,21 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de : reviewDecision === "CHANGES_REQUESTED" ? "a reviewer has since requested changes" : null - : // Only reached when closeRequiresMergeableState !== false or closeRequiresThreadResolved === true (see - // shouldRecheckLiveDisposition above), so CI state is irrelevant to this specific close's justification - // and the only live signals that matter are whether the conflict has cleared or the thread(s) resolved -- - // each gated individually below (mergeableNowCleared / threadsNowResolved) so a close justified by only - // ONE of the two axes is never wrongly cleared by the other axis's unrelated live state. + : // Only reached when closeRequiresMergeableState !== false, closeRequiresThreadResolved === true, or + // closeRequiresDuplicateStillOpen === true (see shouldRecheckLiveDisposition/shouldRecheckLiveDuplicateWinner + // above), so CI state is irrelevant to this specific close's justification and the only live signals + // that matter are whether the conflict cleared, the thread(s) resolved, or the duplicate winner closed -- + // each gated individually below (mergeableNowCleared / threadsNowResolved / the duplicate check) so a + // close justified by only ONE axis is never wrongly cleared by another axis's unrelated live state. // reviewFetchSucceeded is required alongside the value check -- see its own comment above -- so a failed // live-review read fails open instead of masquerading as "confirmed no changes requested". mergeableNowCleared ? "the conflict that justified this close has since cleared" : threadsNowResolved ? "the review thread(s) that justified this close are now all resolved" - : null; + : shouldRecheckLiveDuplicateWinner && duplicateWinnerState !== undefined && duplicateWinnerState !== "open" + ? `duplicate-cluster winner #${pending.params.duplicateWinnerPrNumber} is no longer open` + : null; if (staleReason) { await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); await recordAuditEvent(env, { @@ -284,6 +307,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null, liveThreadBlockerCount: liveThreadBlockers?.length ?? null, + duplicateWinnerState: duplicateWinnerState ?? null, }, }); return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "stale_disposition" }; diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index d484c4e1b4..3f264892e5 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -116,6 +116,19 @@ export type PlannedAgentAction = { // justification -- see the doc comment on AgentPendingActionParams in types.ts. Mirrors // closeRequiresMergeableState's own discipline: ALWAYS set for a heuristic close (never omitted). closeRequiresThreadResolved?: boolean; + // True when a duplicate-PR justification (linkedDuplicateCount > 0) was part of this close's reasons -- + // mirrors closeRequiresMergeableState's discipline: ALWAYS set (never omitted) for a freshly planned + // heuristic close, so `undefined` on a replayed row unambiguously means "legacy row, predates this field". + // Unlike a conflict, this justification depends on a SIBLING PR's live state, not this PR's own -- the + // recheck (agent-action-executor.ts / agent-approval-queue.ts) re-verifies duplicateWinnerPrNumber below, + // when a specific one was named, rather than re-deriving the whole cluster (#dup-winner-staleness). + closeRequiresDuplicateStillOpen?: boolean; + // The specific sibling PR number this close named as the duplicate-cluster winner (dupWinnerLinkedDuplicateWinnerNumber), + // persisted so the recheck above can re-verify THAT PR specifically instead of re-running the full election. + // Absent when the election named no specific winner (GITTENSORY_DUPLICATE_WINNER off, or an ambiguous + // election) even though closeRequiresDuplicateStillOpen is true -- the recheck has no cheap single-PR signal + // in that case and is a no-op, matching the legacy (pre-dup-winner) behavior for that configuration. + duplicateWinnerPrNumber?: number; // For a "heuristic" close: true when the close is backed by CONCRETE, non-judgment evidence — a committed // secret, a failing/red CI run, a base conflict, a deterministic linked-issue-overlap duplicate, or a // rule-based lane/manifest/pre-merge rejection — rather than any AI/model-derived verdict or a fuzzy score. @@ -180,6 +193,13 @@ const CONCRETE_EVIDENCE_BLOCKER_CODES = new Set([ * wrongly classified as concrete). */ function hasConcreteCloseEvidence(input: AgentActionPlanInput, ciFailed: boolean, isConflict: boolean): boolean { if (ciFailed || isConflict) return true; + // A duplicate-PR link stays concrete evidence even now that the close has its own live staleness recheck + // (closeRequiresDuplicateStillOpen, see the field's doc comment) -- exactly the same relationship isConflict + // above already has with #3863's live mergeable-state recheck. The breaker exists to catch a SYSTEMATICALLY + // WRONG judgment call (an AI verdict that's often mistaken), not to guard against an otherwise-correct + // deterministic fact going STALE between planning and actuation -- that staleness risk is what the recheck + // itself closes. A duplicate-issue-link, like a base conflict, is still a deterministic, zero-hallucination + // fact about the linked-issue graph; it just needs to be re-verified fresh, which it now is. if ((input.pr.linkedDuplicateCount ?? 0) > 0) return true; return (input.gateBlockerCodes ?? []).some((code) => CONCRETE_EVIDENCE_BLOCKER_CODES.has(code) && !AI_JUDGMENT_BLOCKER_CODES.has(code)); } @@ -1166,6 +1186,13 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne closeRequiresMergeableState: isConflict, // Always explicit (never omitted), mirroring closeRequiresCiState's own discipline above. closeRequiresThreadResolved: isReviewThreadJustified, + // Always explicit (never omitted), mirroring closeRequiresMergeableState's own discipline -- a duplicate + // justification depends on a SIBLING PR's live state, which can change independently of this PR between + // planning and actuation (#dup-winner-staleness). + closeRequiresDuplicateStillOpen: (input.pr.linkedDuplicateCount ?? 0) > 0, + // Only set when the election named a SPECIFIC winning sibling -- omitted (not null) when there is none, + // matching expectedHeadSha's own "absent, not null" convention for an optional pin. + ...(input.pr.linkedDuplicateWinnerNumber != null ? { duplicateWinnerPrNumber: input.pr.linkedDuplicateWinnerNumber } : {}), }); } // else: guarded → manual; not-good OWNER/automation → manual; action-required/unverified → manual; diff --git a/src/types.ts b/src/types.ts index aef8d21366..8d8532b8b7 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1165,14 +1165,14 @@ export type AgentPendingActionParams = { // so `undefined` unambiguously means a LEGACY row staged before this field existed, not "not CI-driven". closeRequiresCiState?: "failed" | "not_required"; // True when a base conflict (mergeable_state: "dirty") was part of this heuristic close's justification -- - // one of the few non-CI close reasons (alongside closeRequiresThreadResolved below) the approval queue's - // accept-time live recheck has a cheap, reliable live signal for. Other non-CI heuristic reasons (duplicate - // PR, slop score, a gate-verdict blocker not backed by a review thread) have no equivalently cheap live - // re-derivation, so decidePendingAgentAction only reruns its mergeable-state/review-decision staleness check - // when this is true -- gating it on closeRequiresCiState === "not_required" alone (any non-CI reason) instead - // would supersede EVERY duplicate/slop/blocker-only close whose mergeability simply happens to read "clean" - // (which most never-conflicted PRs already are), even though their actual justification never depended on - // mergeability and may still be live (gate review finding). + // one of three non-CI close reasons (alongside closeRequiresThreadResolved and closeRequiresDuplicateStillOpen + // below) the approval queue's accept-time live recheck has a cheap, reliable live signal for. Slop score and a + // gate-verdict blocker not backed by one of the above have no equivalently cheap live re-derivation, so + // decidePendingAgentAction only reruns its mergeable-state/review-decision staleness check when this is true -- + // gating it on closeRequiresCiState === "not_required" alone (any non-CI reason) instead would supersede EVERY + // duplicate/slop/blocker-only close whose mergeability simply happens to read "clean" (which most + // never-conflicted PRs already are), even though their actual justification never depended on mergeability and + // may still be live (gate review finding). // ALWAYS set (never omitted) for a freshly planned heuristic close, mirroring closeRequiresCiState's own // discipline -- so `undefined` unambiguously means a legacy row staged before this field existed. closeRequiresMergeableState?: boolean; @@ -1185,6 +1185,16 @@ export type AgentPendingActionParams = { // not an ambiguous legacy row; the accept-time/actuation-time rechecks below scope on it with a strict // `=== true`, not the broader `!== false` closeRequiresMergeableState needs for its own legacy-row case. closeRequiresThreadResolved?: boolean; + // True when a duplicate-PR justification (linkedDuplicateCount > 0) was part of this heuristic close's + // reasons -- see PlannedAgentAction.closeRequiresDuplicateStillOpen in agent-actions.ts for the full + // rationale (#dup-winner-staleness). ALWAYS set (never omitted) for a freshly planned heuristic close, + // mirroring closeRequiresMergeableState's own discipline. + closeRequiresDuplicateStillOpen?: boolean; + // The specific sibling PR number named as the duplicate-cluster winner, persisted so the recheck can + // re-verify THAT PR specifically instead of re-deriving the whole cluster. See + // PlannedAgentAction.duplicateWinnerPrNumber in agent-actions.ts. Absent when the election named no + // specific winner even though closeRequiresDuplicateStillOpen is true. + duplicateWinnerPrNumber?: number; // Persisted so the close-precision breaker's concrete-evidence exemption (see // PlannedAgentAction.closeConcreteEvidence) still applies correctly when a staged heuristic close is later // accepted -- without this, EVERY staged close would silently fall back to "not concrete" at accept-time and diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index ee271f96ed..a94dcce14b 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -38,11 +38,14 @@ vi.mock("../../src/github/app", async (importOriginal) => ({ // existing test needs to override it; the tests below explicitly set it to exercise the staleness-denial path. // The actuation-time live review-thread re-check (#review-thread-staleness) defaults to a single still-unresolved // blocker for the same reason -- individual tests below override it to exercise the staleness-denial path. +// The actuation-time live duplicate-winner-still-open re-check (#dup-winner-staleness) defaults to "open" (the +// named winning sibling is still open, i.e. the duplicate justification still holds) for the same reason. vi.mock("../../src/github/backfill", async (importOriginal) => ({ ...(await importOriginal()), fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), fetchLivePullRequestMergeState: vi.fn(async () => "dirty" as const), fetchLiveReviewThreadBlockers: vi.fn(async () => [{ title: "still unresolved", scannerFinding: false }]), + fetchLivePullRequestState: vi.fn(async () => "open" as const), refreshInstallationHealthForInstallation: vi.fn(async () => null), })); @@ -51,7 +54,7 @@ import { ensurePullRequestLabel, removePullRequestLabel } from "../../src/github import { ensurePullRequestAssignee } from "../../src/github/assignees"; import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; import { createInstallationToken } from "../../src/github/app"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; import { actionParams, applyModerationEscalationForRule, @@ -622,6 +625,133 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(closePullRequest).not.toHaveBeenCalled(); }); + it("REGRESSION (#dup-winner-staleness): a duplicate-justified heuristic close is DENIED when the named winning sibling PR is no longer open at actuation time", async () => { + const env = createTestEnv({}); + const dupClose: PlannedAgentAction = { + actionClass: "close", + requiresApproval: false, + reason: "duplicate of open PR #42", + closeComment: "closing", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + }; + vi.mocked(fetchLivePullRequestState).mockResolvedValueOnce("closed"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [dupClose]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("duplicate-cluster winner #42 is no longer open"); + expect(closePullRequest).not.toHaveBeenCalled(); + expect(fetchLivePullRequestState).toHaveBeenCalledWith(env, "owner/repo", 42, expect.anything(), expect.anything()); + }); + + it("a duplicate-justified heuristic close proceeds when the named winning sibling PR is still open (#dup-winner-staleness)", async () => { + const env = createTestEnv({}); + const dupClose: PlannedAgentAction = { + actionClass: "close", + requiresApproval: false, + reason: "duplicate of open PR #42", + closeComment: "closing", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + }; + vi.mocked(fetchLivePullRequestState).mockResolvedValueOnce("open"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [dupClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("a duplicate-justified heuristic close fails open (still proceeds) when the live duplicate-winner-state read is ambiguous/unresolved (#dup-winner-staleness)", async () => { + const env = createTestEnv({}); + const dupClose: PlannedAgentAction = { + actionClass: "close", + requiresApproval: false, + reason: "duplicate of open PR #42", + closeComment: "closing", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + }; + // A failed/ambiguous fetch resolves to undefined -- not proof the sibling closed, so the close must not be + // silently blocked by an inconclusive live read (mirrors the mergeable-state recheck's own fail-open case). + vi.mocked(fetchLivePullRequestState).mockRejectedValueOnce(new Error("GitHub API transient 502")); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [dupClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("a duplicate-justified close with NO named winner (closeRequiresDuplicateStillOpen true, duplicateWinnerPrNumber absent) skips the live recheck entirely -- no cheap single-PR signal to check (#dup-winner-staleness)", async () => { + const env = createTestEnv({}); + const dupClose: PlannedAgentAction = { + actionClass: "close", + requiresApproval: false, + reason: "duplicate of another open PR", + closeComment: "closing", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + }; + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [dupClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(fetchLivePullRequestState).not.toHaveBeenCalled(); + }); + + it("a non-duplicate heuristic close (closeRequiresDuplicateStillOpen false) skips the live duplicate-winner recheck entirely (#dup-winner-staleness)", async () => { + const env = createTestEnv({}); + const gateClose: PlannedAgentAction = { + actionClass: "close", + requiresApproval: false, + reason: "policy gate blocker", + closeComment: "closing", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: false, + }; + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [gateClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(fetchLivePullRequestState).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#dup-winner-staleness): closeRequiresDuplicateStillOpen and duplicateWinnerPrNumber round-trip through the persist/replay round trip so a staged duplicate close still re-checks live winner state", async () => { + const env = createTestEnv({}); + const dupClose: PlannedAgentAction = { + actionClass: "close", + requiresApproval: false, + reason: "duplicate of open PR #42", + closeComment: "closing", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + }; + const persisted = actionParams(dupClose); + expect(persisted.closeRequiresDuplicateStillOpen).toBe(true); + expect(persisted.duplicateWinnerPrNumber).toBe(42); + const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: dupClose.reason }); + expect(replayed.closeRequiresDuplicateStillOpen).toBe(true); + expect(replayed.duplicateWinnerPrNumber).toBe(42); + vi.mocked(fetchLivePullRequestState).mockResolvedValueOnce("closed"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [replayed]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + + it("closeRequiresDuplicateStillOpen and duplicateWinnerPrNumber are omitted from persisted params when absent on the planned action (no stray keys)", () => { + const ambiguousClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "verdict failed", closeComment: "closing", closeKind: "heuristic" }; + const persisted = actionParams(ambiguousClose); + expect(persisted).not.toHaveProperty("closeRequiresDuplicateStillOpen"); + expect(persisted).not.toHaveProperty("duplicateWinnerPrNumber"); + }); + it("LIVE merge is denied when live CI has since turned failing (#2128)", async () => { const env = createTestEnv({}); vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index f088481a4a..9d19aedac3 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1560,16 +1560,30 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true, closeRequiresMergeableState: true }); }); - it("a deterministic linked-issue-overlap duplicate (linkedDuplicateCount > 0) is concrete evidence, but NOT conflict-justified", () => { + it("a deterministic linked-issue-overlap duplicate (linkedDuplicateCount > 0) is concrete evidence, but NOT conflict-justified — and IS duplicate-still-open-justified (#dup-winner-staleness)", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1 } })); // Concrete evidence (duplicate) does NOT imply closeRequiresMergeableState -- that field is specifically // about whether a base conflict was part of the reason, not whether the close is "trustworthy" in general. - expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true, closeRequiresMergeableState: false }); + // closeRequiresDuplicateStillOpen IS true here -- this close's justification depends on a sibling PR's live + // state, so the executor/approval-queue's live recheck (#dup-winner-staleness) must fire for it. + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true, closeRequiresMergeableState: false, closeRequiresDuplicateStillOpen: true }); }); - it("linkedDuplicateCount absent (nullish ?? 0) does NOT count as concrete on its own", () => { + it("linkedDuplicateCount absent (nullish ?? 0) does NOT count as concrete on its own, and closeRequiresDuplicateStillOpen is explicitly false (#dup-winner-staleness)", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [] } })); - expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false, closeRequiresDuplicateStillOpen: false }); + }); + + it("a named duplicate-cluster winner (linkedDuplicateWinnerNumber) is persisted as duplicateWinnerPrNumber so the live recheck knows which sibling to re-verify (#dup-winner-staleness)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1, linkedDuplicateWinnerNumber: 42 } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeRequiresDuplicateStillOpen: true, duplicateWinnerPrNumber: 42 }); + }); + + it("an unnamed duplicate winner (linkedDuplicateWinnerNumber null — flag off or an ambiguous election) omits duplicateWinnerPrNumber entirely (no stray key), even though closeRequiresDuplicateStillOpen is true (#dup-winner-staleness)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1, linkedDuplicateWinnerNumber: null } })); + const close = closeOf(plan); + expect(close).toMatchObject({ closeKind: "heuristic", closeRequiresDuplicateStillOpen: true }); + expect(close).not.toHaveProperty("duplicateWinnerPrNumber"); }); it("a committed secret (secret_leak) is concrete evidence via gateBlockerCodes", () => { diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index e6ba0aaad8..2fff622452 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -37,6 +37,9 @@ vi.mock("../../src/github/backfill", async (importOriginal) => ({ // Defaults to "no live blockers left" so the existing accept tests stay deterministic; individual tests below // override this to exercise the thread-staleness supersede path. fetchLiveReviewThreadBlockers: vi.fn(async () => []), + // The duplicate-winner-still-open re-check (#dup-winner-staleness) defaults to "open" (the named winning + // sibling is still open, i.e. the duplicate justification still holds) so existing tests stay deterministic. + fetchLivePullRequestState: vi.fn(async () => "open"), })); // resolveLinkedIssueHardRule defaults to the REAL implementation, which is a safe no-op here: loadLinkedIssueHardRules // (also real, unmocked) always returns the all-off default config, so the real resolver returns undefined (not @@ -52,7 +55,7 @@ vi.mock("../../src/review/linked-issue-hard-rules", async (importOriginal) => { import { createPullRequestReview, mergePullRequest } from "../../src/github/pr-actions"; import { ensurePullRequestLabel } from "../../src/github/labels"; import { createInstallationToken } from "../../src/github/app"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLiveReviewThreadBlockers } from "../../src/github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers } from "../../src/github/backfill"; import { resolveLinkedIssueHardRule } from "../../src/review/linked-issue-hard-rules"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, type AgentActionExecutionContext } from "../../src/services/agent-action-executor"; @@ -845,17 +848,17 @@ describe("agent approval queue (#779)", () => { expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ ciState: "pending", mergeableState: "clean" }); }); - it("REGRESSION (gate review): a duplicate/slop/blocker-only close (no conflict, no review thread) is never touched by the mergeable-state/thread recheck", async () => { + it("REGRESSION (gate review): a slop/blocker-only close (no conflict, no review thread, no duplicate justification) is never touched by the mergeable-state, thread, or duplicate-winner recheck", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); await seedInstallation(env); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); - // mergeableState reads "clean" (the default mock) and this close was NEVER conflict- or thread-justified - // (closeRequiresMergeableState: false, closeRequiresThreadResolved: false) -- a duplicate/slop/blocker - // close's mergeability/review-thread state was never the signal that justified it, so it must execute as - // staged rather than being superseded just because the PR happens to have clean mergeability (the - // gate-review-flagged over-broad-predicate regression). Narrowed by #review-thread-staleness to also cover - // the new review-thread exemption -- this close stays exempt from BOTH live rechecks. + // mergeableState reads "clean" (the default mock) and this close was NEVER conflict-, thread-, or + // duplicate-justified (closeRequiresMergeableState: false, closeRequiresThreadResolved: false, + // closeRequiresDuplicateStillOpen absent) -- a slop/blocker close's mergeability/review-thread/duplicate + // status was never the signal that justified it, so it must execute as staged rather than being superseded + // just because the PR happens to have clean mergeability (the gate-review-flagged over-broad-predicate + // regression). Covers all three live-recheck exemptions in one test since they share the same shape. const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, @@ -863,14 +866,14 @@ describe("agent approval queue (#779)", () => { actionClass: "close", autonomyLevel: "auto_with_approval", params: { - closeComment: "duplicate of another open PR", + closeComment: "slop score too high", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresThreadResolved: false, expectedHeadSha: "h7", }, - reason: "duplicate of another open PR", + reason: "slop score too high", }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); @@ -879,12 +882,13 @@ describe("agent approval queue (#779)", () => { expect(result.executionOutcome).toBe("completed"); const { closePullRequest } = await import("../../src/github/pr-actions"); expect(closePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7); - // No live recheck was even attempted for this close -- it isn't scoped by closeRequiresMergeableState or - // closeRequiresThreadResolved. + // No live recheck was even attempted for this close -- it isn't scoped by closeRequiresMergeableState, + // closeRequiresThreadResolved, or closeRequiresDuplicateStillOpen. expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); expect(fetchLivePullRequestMergeState).not.toHaveBeenCalled(); expect(fetchLivePullRequestReviewDecision).not.toHaveBeenCalled(); expect(fetchLiveReviewThreadBlockers).not.toHaveBeenCalled(); + expect(fetchLivePullRequestState).not.toHaveBeenCalled(); }); it("REGRESSION (#review-thread-staleness): a review-thread-only close (closeRequiresThreadResolved: true) DOES trigger the live rechecks", async () => { @@ -1037,6 +1041,100 @@ describe("agent approval queue (#779)", () => { expect(closePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7); }); + it("REGRESSION (#dup-winner-staleness): a duplicate-justified close naming a specific winning sibling is DENIED (superseded) when that sibling is no longer open at accept time", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + // Queues exactly 2 responses (Once, not a persistent mockResolvedValue): the accept-time recheck here AND + // the executor's own actuation-time recheck each consume one call and must see the SAME "no longer open" + // state for this test's premise to hold (mirrors the #3863 mergeable-state test's own precedent). + vi.mocked(fetchLivePullRequestState).mockResolvedValueOnce("closed").mockResolvedValueOnce("closed"); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { + closeComment: "duplicate of open PR #42", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + expectedHeadSha: "h7", + }, + reason: "duplicate of open PR #42", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + const { closePullRequest } = await import("../../src/github/pr-actions"); + expect(closePullRequest).not.toHaveBeenCalled(); + const audit = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ detail: string; metadata_json: string }>(); + expect(audit?.detail).toContain("duplicate-cluster winner #42 is no longer open"); + expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ duplicateWinnerState: "closed" }); + }); + + it("a duplicate-justified close naming a specific winning sibling proceeds when that sibling is still open at accept time (#dup-winner-staleness)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { + closeComment: "duplicate of open PR #42", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + expectedHeadSha: "h7", + }, + reason: "duplicate of open PR #42", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + const { closePullRequest } = await import("../../src/github/pr-actions"); + expect(closePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7); + expect(fetchLivePullRequestState).toHaveBeenCalledWith(env, "owner/repo", 42, expect.anything(), expect.anything()); + }); + + it("a duplicate-justified close with NO named winner (duplicateWinnerPrNumber absent) skips the duplicate-winner recheck entirely at accept time (#dup-winner-staleness)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "duplicate of another open PR", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresDuplicateStillOpen: true, expectedHeadSha: "h7" }, + reason: "duplicate of another open PR", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + const { closePullRequest } = await import("../../src/github/pr-actions"); + expect(closePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7); + expect(fetchLivePullRequestState).not.toHaveBeenCalled(); + }); + it("REGRESSION (gate review): a LEGACY heuristic close row (closeRequiresMergeableState undefined, staged before the field existed) still gets the live recheck", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); @@ -1404,6 +1502,29 @@ describe("agent approval queue (#779)", () => { expect( actionParams({ actionClass: "close", requiresApproval: false, reason: "x", closeComment: "C", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true }), ).toEqual({ closeComment: "C", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true }); + // closeRequiresDuplicateStillOpen and duplicateWinnerPrNumber must ALSO round-trip (#dup-winner-staleness) + // -- without them, a replayed duplicate-justified close would lose the discriminators the approval queue's + // accept-time duplicate-winner recheck depends on. + expect( + actionParams({ + actionClass: "close", + requiresApproval: false, + reason: "x", + closeComment: "C", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + }), + ).toEqual({ + closeComment: "C", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresDuplicateStillOpen: true, + duplicateWinnerPrNumber: 42, + }); }); it("lists all pending actions unfiltered and stores a null reason when omitted", async () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 153f5aaf0b..ce0930cbdf 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -21591,6 +21591,82 @@ describe("queue processors", () => { expect(winnerAdvisory?.findings_json ?? "").toContain("duplicate_pr_risk"); }); + it("REGRESSION (#dup-winner-slop-drift): maybePublishPrPublicSurface's slop penalty uses the LIVE-reconciled siblings, not a raw stale-cached read — a stale-cached-open lower sibling that is actually CLOSED on GitHub must not deny this PR winner status / slop-penalize it for the cluster", async () => { + // GITTENSORY_DUPLICATE_WINNER ON. PR #95 (this PR, being reviewed) links issue #1; PR #90 (LOWER-numbered, + // same linked issue) is cached `open` in the DB (a missed/delayed `closed` webhook), but GitHub's LIVE state + // for #90 is actually `closed`. Before the fix, maybePublishPrPublicSurface's own duplicate-winner election + // read the raw, un-reconciled `listPullRequests` result (still showing #90 as open) and so wrongly denied + // #95 winner status, applying the duplicateClusterMembership slop penalty (weight 15, persisted slop_band + // "low") even though the gate's OWN reconciled otherOpenPullRequests (used to build the advisory/gate + // disposition) had already correctly dropped #90. After the fix, both paths agree: #95 is the winner (no + // open siblings once reconciled) and carries NO duplicate-cluster slop penalty (slop_band "clean"). + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_DUPLICATE_WINNER: "true" }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload({ "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + duplicatePrGateMode: "block", + slopGateMode: "advisory", + }); + await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 1, title: "Cache the registry sync", state: "open", user: { login: "maintainer" }, author_association: "OWNER", body: "We should cache the registry fetch." }); + // Stale-cached-open sibling: the DB still says #90 is open (the closed webhook was missed/delayed). + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 90, title: "Older attempt at the cache fix", state: "open", user: { login: "other" }, author_association: "CONTRIBUTOR", head: { sha: "sib90" }, labels: [], body: "Fixes #1" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 95, title: "Fix the cache", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "win95" }, labels: [], body: "Fixes #1\n\nValidation: npm test" }); + + let liveStateFetches90 = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + // The LIVE state of the lower sibling #90 is CLOSED, contradicting the stale-cached "open" DB row -- + // reconcileLiveDuplicateSiblings must discover this via a genuine live fetch, not the cache. + if (/\/pulls\/90(?:\?|$)/.test(url)) { + liveStateFetches90 += 1; + return Response.json({ number: 90, state: "closed" }); + } + // Includes a test-file change alongside the code change so missingTestEvidence never confounds the + // duplicateClusterMembership assertion below — this test isolates the ONE slop signal under test. + if (url.includes("/pulls/95/files")) + return Response.json([ + { filename: "src/cache.ts", status: "modified", additions: 12, deletions: 0, changes: 12 }, + { filename: "test/unit/cache.test.ts", status: "modified", additions: 8, deletions: 0, changes: 8 }, + ]); + if (url.includes("/commits/win95/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "PATCH") return Response.json({ id: 970 }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 970 }, { status: 201 }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "dup-winner-slop-drift", + eventName: "pull_request", + payload: { + action: "opened", + 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: 95, title: "Fix the cache", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "win95" }, labels: [], body: "Fixes #1\n\nValidation: npm test" }, + }, + }); + + // A genuine live reconciliation happened (proving the fix reads live state, not the stale cache). + expect(liveStateFetches90).toBeGreaterThan(0); + // #95 is correctly credited as the cluster winner: no duplicateClusterMembership slop penalty persisted. + const winnerPr = await env.DB.prepare("select slop_risk, slop_band from pull_requests where repo_full_name = ? and number = ?").bind("JSONbored/gittensory", 95).first<{ slop_risk: number | null; slop_band: string | null }>(); + expect(winnerPr?.slop_band).toBe("clean"); + expect(winnerPr?.slop_risk).toBe(0); + }); + it("overrides the Gate to neutral for THIS commit only when a real write/admin maintainer runs gate-override", 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);