diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 3301aa3388..ef64acd28c 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, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestAssignee } from "../github/assignees"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; @@ -363,9 +363,11 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // guard above only re-checks head SHA/state, not CI. GitHub's own merge endpoint enforces // branch-protection REQUIRED checks server-side, but only as a backstop when a repo actually configures // them; a red-CI close has no server-side check at all. Re-read live CI right before the mutation so a - // check that flipped in this narrow window is never acted on from stale information. Non-CI closes - // (gate verdict, duplicate/slop, linked-issue hard-rule, blacklist) are exempt — their adverse signal - // does not depend on CI still being red. + // check that flipped in this narrow window is never acted on from stale information. Non-CI closes whose + // justification has no cheap live re-derivation (gate verdict, duplicate/slop, linked-issue hard-rule, + // blacklist) are exempt from THIS specific CI recheck — their adverse signal does not depend on CI still + // being red. A base conflict and an unresolved review thread DO have cheap live signals and get their own + // dedicated rechecks below (requiresLiveMergeableRecheck / requiresLiveThreadRecheck) instead. // A heuristic close staged BEFORE #2478 has no closeRequiresCiState at all -- that field didn't exist yet // -- so `undefined` here is genuinely ambiguous (a legacy CI-driven close and a legacy non-CI close are // byte-identical in storage). The planner now ALWAYS sets the field going forward (never omits it), so @@ -381,17 +383,23 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // live re-check for a STAGED close (agent-approval-queue.ts); this is the immediate, same-pass execution // path, which had no equivalent. const requiresLiveMergeableRecheck = action.actionClass === "close" && action.closeKind === "heuristic" && action.closeRequiresMergeableState === true; - if (requiresLiveCiRecheck || requiresLiveMergeableRecheck) { + // #review-thread-staleness: mirrors requiresLiveMergeableRecheck's exact shape (#3863) -- a review-thread- + // justified heuristic close is read from the SAME planning-pass snapshot, and a contributor clicking + // "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) { 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] = await Promise.all([ + const [liveCi, liveMergeableState, liveThreadBlockers] = 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), ]); // 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 @@ -414,7 +422,15 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // resolved, matching the approval-queue's own fail-safe-toward-keeping-the-close precedent (#3863). const mergeableStaleReason = requiresLiveMergeableRecheck && liveMergeableState === "clean" ? "the base-branch conflict that justified this close has since cleared" : null; - const staleReason = ciStaleReason ?? mergeableStaleReason; + // Only a CONFIRMED empty result clears a thread-justified close -- fetchLiveReviewThreadBlockers already + // fails open to [] on its own internal GraphQL error, so `undefined` here means the Promise.resolve(undefined) + // no-op arm (requiresLiveThreadRecheck was false) rather than a genuine "no threads left" signal, matching + // the mergeable-state recheck's own fail-safe-toward-keeping-the-close precedent above. + const threadStaleReason = + requiresLiveThreadRecheck && liveThreadBlockers !== undefined && liveThreadBlockers.length === 0 + ? "the review thread(s) that justified this close are now all resolved" + : null; + const staleReason = ciStaleReason ?? mergeableStaleReason ?? threadStaleReason; if (staleReason) { await audit("denied", `${staleReason} — action not executed`); continue; @@ -862,6 +878,9 @@ export function actionParams(action: PlannedAgentAction): AgentPendingActionPara // Round-trip the mergeable-state dependency likewise: only a conflict-justified close needs the approval // queue's accept-time mergeable-state recheck (see the field's doc comment on AgentPendingActionParams). ...(action.closeRequiresMergeableState !== undefined ? { closeRequiresMergeableState: action.closeRequiresMergeableState } : {}), + // 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 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 dbef88b1f4..86c49cb613 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, mergeRequiredCiContexts } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -200,10 +200,15 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // 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 shouldRecheckLiveDisposition = - pr?.headSha && - (pending.actionClass === "merge" || - (pending.actionClass === "close" && pending.params.closeKind === "heuristic" && pending.params.closeRequiresMergeableState !== false)); + 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 + // pre-existing legacy rows anywhere -- it is introduced in the same change as its only producer, so a + // freshly planned heuristic close ALWAYS sets it explicitly (mirroring closeRequiresMergeableState's own + // "never omitted" discipline). `undefined` here can therefore only mean "not thread-justified", never an + // 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) { const token = await createInstallationToken(env, pending.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, pending.installationId); @@ -211,13 +216,14 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // 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] = await Promise.allSettled([ + 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), + isThreadRecheck ? fetchLiveReviewThreadBlockers(env, pending.repoFullName, pending.pullNumber, 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, @@ -230,6 +236,19 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // below instead of failing open on it (gate review finding). const reviewFetchSucceeded = reviewResult.status === "fulfilled"; 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 + // itself already fails open to [] on a GraphQL error, so a genuinely FULFILLED empty array is the only + // signal that legitimately means "no live blockers left". + 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). + const mergeableNowCleared = isMergeableRecheck && reviewFetchSucceeded && mergeableState === "clean" && reviewDecision !== "CHANGES_REQUESTED"; const staleReason = pending.actionClass === "merge" ? ciState !== undefined && ciState !== "passed" @@ -239,14 +258,18 @@ 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 (see shouldRecheckLiveDisposition above), so - // CI state is irrelevant to this specific close's justification and the only live signal that matters - // is whether the conflict has cleared. 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". - reviewFetchSucceeded && mergeableState === "clean" && reviewDecision !== "CHANGES_REQUESTED" + : // 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. + // 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" - : null; + : threadsNowResolved + ? "the review thread(s) that justified this close are now all resolved" + : null; if (staleReason) { await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); await recordAuditEvent(env, { @@ -255,7 +278,13 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de targetKey, outcome: "denied", detail: `superseded ${pending.actionClass}: ${staleReason} since staging`, - metadata: { ...baseMetadata, ciState: ciState ?? null, mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null }, + metadata: { + ...baseMetadata, + ciState: ciState ?? null, + mergeableState: mergeableState ?? null, + reviewDecision: reviewDecision ?? null, + liveThreadBlockerCount: liveThreadBlockers?.length ?? 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 ae5fb1df39..d484c4e1b4 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -3,6 +3,7 @@ import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckConclusion } from "../rules/ad import { DEFAULT_AUTO_MAINTAIN_POLICY, autonomyRequiresApproval, isActingAutonomyLevel, resolveAutonomy } from "./autonomy"; import { changedPathsHittingGuardrail, isGuardrailHit } from "../signals/change-guardrail"; import { AGENT_LABEL_PENDING_CLOSURE } from "../review/linked-issue-hard-rules"; +import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings"; import { sanitizePublicComment } from "../github/commands"; // High-slop threshold default when a repo hasn't set slopGateMinScore (mirrors the gate's `high` band). @@ -111,6 +112,10 @@ export type PlannedAgentAction = { // AgentPendingActionParams in types.ts for why the approval queue's accept-time recheck is scoped to this // specific case rather than every non-CI heuristic close. ALWAYS set for a heuristic close (never omitted). closeRequiresMergeableState?: boolean; + // True when an unresolved GitHub review thread (REVIEW_THREAD_BLOCKER_CODE) was part of this close's + // 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; // 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. @@ -687,6 +692,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // or review-thread blocker into success once the gate has classified it as blocking. const conclusion: GateCheckConclusion = input.conclusion; const isConflict = input.pr.mergeableState === "dirty"; // conflicts with base — can't merge as-is + // True when an unresolved GitHub review thread is (at least one of) this close's justifications -- the SAME + // staleness class as isConflict above (#3863), just triggered by a contributor clicking "Resolve conversation" + // on GitHub instead of the base branch becoming mergeable again. A mixed blocker set (thread + something else) + // still counts: the thread recheck only re-verifies ITS OWN signal, so it's harmless to also gate on it here. + const isReviewThreadJustified = (input.gateBlockerCodes ?? []).includes(REVIEW_THREAD_BLOCKER_CODE); const isContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; // The owner-close exemption is PER-REPO CONFIGURABLE (#configurable-owner-close): by default the repo owner's // own PRs are exempt from auto-close (closeOwnerAuthors !== true ⇒ merge or manual-hold only), but a maintainer @@ -1154,6 +1164,8 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne closeRequiresCiState: ciFailed ? "failed" : "not_required", // Always explicit (never omitted), mirroring closeRequiresCiState's own discipline above. closeRequiresMergeableState: isConflict, + // Always explicit (never omitted), mirroring closeRequiresCiState's own discipline above. + closeRequiresThreadResolved: isReviewThreadJustified, }); } // else: guarded → manual; not-good OWNER/automation → manual; action-required/unverified → manual; diff --git a/src/types.ts b/src/types.ts index 21bb989871..aef8d21366 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1165,16 +1165,26 @@ 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 -- - // the ONLY non-CI close reason 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) 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 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). // 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; + // True when an unresolved GitHub review thread (REVIEW_THREAD_BLOCKER_CODE) was part of this heuristic + // close's justification -- the SAME staleness class as closeRequiresMergeableState (#3863) but for a + // contributor RESOLVING the thread on GitHub instead of the base branch becoming mergeable again. ALWAYS set + // (never omitted) for a freshly planned heuristic close, mirroring closeRequiresMergeableState's own + // discipline. Unlike closeRequiresMergeableState, this field has NO pre-existing legacy rows -- it is + // introduced alongside its only producer, so `undefined` here unambiguously means "not thread-justified", + // 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; // 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 570a7b58a2..ee271f96ed 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -36,10 +36,13 @@ vi.mock("../../src/github/app", async (importOriginal) => ({ // deterministic; individual tests below override this to exercise the staleness-denial path. // The actuation-time live mergeable-state re-check (#3863) defaults to "dirty" (conflict still present) so no // 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. 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 }]), refreshInstallationHealthForInstallation: vi.fn(async () => null), })); @@ -48,7 +51,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, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; import { actionParams, applyModerationEscalationForRule, @@ -566,6 +569,59 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(closePullRequest).not.toHaveBeenCalled(); }); + it("REGRESSION (#review-thread-staleness): a review-thread-justified heuristic close is DENIED when the live review threads have since all resolved", async () => { + const env = createTestEnv({}); + const threadClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "unresolved review thread", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresThreadResolved: true }; + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce([]); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [threadClose]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("the review thread(s) that justified this close are now all resolved"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + + it("a review-thread-justified heuristic close proceeds when the live review thread is still unresolved (#review-thread-staleness)", async () => { + const env = createTestEnv({}); + const threadClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "unresolved review thread", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresThreadResolved: true }; + // The module mock's default already returns a single still-unresolved blocker. + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [threadClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("a review-thread-justified heuristic close fails open (still proceeds) when the live thread-blocker read is ambiguous/unresolved (#review-thread-staleness)", async () => { + const env = createTestEnv({}); + const threadClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "unresolved review thread", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresThreadResolved: true }; + // fetchLiveReviewThreadBlockers itself never rejects in production (it fails open to [] internally on a + // GraphQL error) -- but the executor's own Promise.all resolution here still must not treat an ambiguous + // undefined result (were one ever to occur) as proof the threads resolved. Simulate that with a resolved + // single-element array standing in for "read succeeded, still unresolved" -- the true fail-open contract is + // that only a CONFIRMED empty array clears the close, covered by the DENIED test above. + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce([{ title: "ambiguous but present", scannerFinding: false }]); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [threadClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("a non-thread heuristic close (closeRequiresThreadResolved omitted/false) skips the live thread-blocker re-check entirely (#review-thread-staleness)", async () => { + const env = createTestEnv({}); + const gateClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "policy gate blocker", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresThreadResolved: false }; + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [gateClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(fetchLiveReviewThreadBlockers).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#review-thread-staleness): closeRequiresThreadResolved round-trips through the persist/replay round trip so a staged thread-justified close still re-checks live thread blockers", async () => { + const env = createTestEnv({}); + const threadClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "unresolved review thread", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresThreadResolved: true }; + const persisted = actionParams(threadClose); + const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: threadClose.reason }); + expect(replayed.closeRequiresThreadResolved).toBe(true); + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce([]); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [replayed]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + 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 a4656212e9..f088481a4a 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -2,6 +2,7 @@ import { readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; import { AGENT_LABEL_CHANGES, AGENT_LABEL_MIGRATION_COLLISION, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, DEFAULT_BLACKLIST_LABEL, DEFAULT_CONTRIBUTOR_CAP_LABEL, DEFAULT_REVIEW_NAG_LABEL, downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, planAgentMaintenanceActions, type AgentActionPlanInput, type PlannedAgentAction } from "../../src/settings/agent-actions"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; +import { REVIEW_THREAD_BLOCKER_CODE } from "../../src/review/review-thread-findings"; import type { GateCheckConclusion } from "../../src/rules/advisory"; // #module-cycle-regression: forces the SAME module-load cycle that broke once (scoring/model.ts -> // db/repositories.ts -> agent-actions.ts -> rules/advisory.ts -> scoring/preview.ts -> scoring/model.ts) to @@ -1087,9 +1088,51 @@ describe("planAgentMaintenanceActions (#778)", () => { closeKind: "heuristic", closeConcreteEvidence: false, closeRequiresMergeableState: false, + closeRequiresThreadResolved: false, }); }); + it("REGRESSION (#review-thread-staleness): CLOSES a review-thread-only blocker with closeRequiresThreadResolved: true, so the actuation-time recheck can catch a since-resolved thread", () => { + const plan = planAgentMaintenanceActions( + input({ + conclusion: "failure", + autonomy: { approve: "auto", merge: "auto", close: "auto" }, + ciState: "passed", + gateBlockerCodes: [REVIEW_THREAD_BLOCKER_CODE], + blockerTitles: ["reviewer review thread unresolved: fix this"], + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + }), + ); + const cls = classes(plan); + expect(cls).not.toContain("approve"); + expect(cls).not.toContain("merge"); + expect(cls).toContain("close"); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ + closeKind: "heuristic", + closeRequiresMergeableState: false, + closeRequiresThreadResolved: true, + }); + }); + + it("REGRESSION (#review-thread-staleness): a mixed blocker set (review thread + something else) still tags closeRequiresThreadResolved: true", () => { + const plan = planAgentMaintenanceActions( + input({ + conclusion: "failure", + autonomy: { approve: "auto", merge: "auto", close: "auto" }, + ciState: "passed", + gateBlockerCodes: [REVIEW_THREAD_BLOCKER_CODE, "ai_consensus_defect"], + blockerTitles: ["reviewer review thread unresolved: fix this", "AI review found a defect"], + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + }), + ); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ closeKind: "heuristic", closeRequiresThreadResolved: true }); + }); + + it("does NOT tag closeRequiresThreadResolved when gateBlockerCodes is absent (nullish ?? [] fallback)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", blockerTitles: ["readiness score too low"], pr: { labels: [] } })); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ closeKind: "heuristic", closeRequiresThreadResolved: false }); + }); + it("CLOSES on already-red CI even while an unrelated check is still pending — red is terminal, it does not need the rest to settle", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto", merge: "auto", close: "auto" }, ciState: "failed", ciHasPending: true, failingCheckNames: ["build"], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); const cls = classes(plan); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index ac794c4831..e6ba0aaad8 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -34,6 +34,9 @@ vi.mock("../../src/github/backfill", async (importOriginal) => ({ fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), fetchLivePullRequestMergeState: vi.fn(async () => "clean"), fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), + // 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 () => []), })); // 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 @@ -49,7 +52,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 } from "../../src/github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, 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"; @@ -842,22 +845,31 @@ 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) is never touched by the mergeable-state recheck", async () => { + 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 () => { 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-justified - // (closeRequiresMergeableState: false) -- a duplicate/slop/blocker close's mergeability 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). + // 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. 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, expectedHeadSha: "h7" }, + params: { + closeComment: "duplicate of another open PR", + closeKind: "heuristic", + closeRequiresCiState: "not_required", + closeRequiresMergeableState: false, + closeRequiresThreadResolved: false, + expectedHeadSha: "h7", + }, reason: "duplicate of another open PR", }); @@ -867,10 +879,162 @@ 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. + // No live recheck was even attempted for this close -- it isn't scoped by closeRequiresMergeableState or + // closeRequiresThreadResolved. expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); expect(fetchLivePullRequestMergeState).not.toHaveBeenCalled(); expect(fetchLivePullRequestReviewDecision).not.toHaveBeenCalled(); + expect(fetchLiveReviewThreadBlockers).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#review-thread-staleness): a review-thread-only close (closeRequiresThreadResolved: true) DOES trigger the live rechecks", 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" }); + // The thread is still unresolved live (the default empty-array mock is overridden here) so this close + // proceeds, but the point of this test is that the fetch was attempted at all -- unlike the exempt + // duplicate/slop close above, a review-thread-justified close IS scoped by closeRequiresThreadResolved. + // Queues exactly 2 responses (Once, not persistent): the accept-time recheck AND the executor's own + // actuation-time recheck each consume one call and must see the SAME still-unresolved state (mirrors the + // #3863 conflict-recheck tests' own "queues exactly 2 responses" comment). + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce([{ title: "fix this", scannerFinding: false }]).mockResolvedValueOnce([{ title: "fix this", scannerFinding: false }]); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "unresolved review thread", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresThreadResolved: true, expectedHeadSha: "h7" }, + reason: "unresolved review thread", + }); + + 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(fetchLiveReviewThreadBlockers).toHaveBeenCalledWith(env, "owner/repo", 7, "test-installation-token", expect.any(String)); + }); + + it("REGRESSION (#review-thread-staleness): accept supersedes a review-thread-justified heuristic close when the thread(s) have since resolved", 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" }); + // fetchLiveReviewThreadBlockers defaults to [] (the module mock above) -- a contributor resolved the + // thread(s) on GitHub since this close was staged. + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "unresolved review thread", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresThreadResolved: true, expectedHeadSha: "h7" }, + reason: "unresolved review thread", + }); + + 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("the review thread(s) that justified this close are now all resolved"); + expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ liveThreadBlockerCount: 0 }); + }); + + it("accept still executes a review-thread-justified heuristic close when the live thread signal remains unresolved", 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 (see the comment on the "DOES trigger the live rechecks" test above) so both + // the accept-time recheck and the executor's own actuation-time recheck see a consistent still-unresolved state. + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce([{ title: "still needs a fix", scannerFinding: false }]).mockResolvedValueOnce([{ title: "still needs a fix", scannerFinding: false }]); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "unresolved review thread", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresThreadResolved: true, expectedHeadSha: "h7" }, + reason: "unresolved review thread", + }); + + 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); + }); + + it("REGRESSION (#review-thread-staleness): a FULFILLED-but-nullish live thread-blocker result (?? 0 branch) reads the same as a confirmed-empty array", async () => { + // Distinct from the "failed live thread-blocker read" test below: there the promise itself REJECTS (fails + // open -- the close proceeds). Here it FULFILLS with a value that is not a real array (defensive: + // fetchLiveReviewThreadBlockers's real contract always resolves to an array, never undefined/null, so this + // exercises the `liveThreadBlockers?.length ?? 0` nullish-fallback arm for a hypothetically-loosened + // contract). A FULFILLED-but-nullish result is treated the SAME as a confirmed empty array (0 blockers), not + // as an ambiguous read -- only a REJECTED promise gets the fail-open treatment. The close is superseded, + // and the executor's own actuation-time recheck is never reached (the row was already rejected here), so + // only ONE response needs to be queued. + 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" }); + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce(undefined as unknown as never); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "unresolved review thread", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresThreadResolved: true, expectedHeadSha: "h7" }, + reason: "unresolved review thread", + }); + + 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("the review thread(s) that justified this close are now all resolved"); + expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ liveThreadBlockerCount: null }); + }); + + it("REGRESSION (#review-thread-staleness): a failed live thread-blocker read fails open instead of masquerading as 'all resolved'", 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" }); + // The live thread-blocker read itself FAILS (transient API error) -- this must fail open (not stale) at the + // approval-queue's own accept-time recheck, not be silently treated as "confirmed all resolved" merely + // because the resolved value would otherwise read as an empty/absent result. The SECOND queued response is + // for the executor's own separate actuation-time recheck (a real fetchLiveReviewThreadBlockers never + // rejects -- it fails open to [] internally -- so this simulates that read still finding the thread + // unresolved, keeping this test's premise about the QUEUE's fail-open path isolated from the executor's). + vi.mocked(fetchLiveReviewThreadBlockers).mockRejectedValueOnce(new Error("GitHub API transient 502")).mockResolvedValueOnce([{ title: "still open", scannerFinding: false }]); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "unresolved review thread", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false, closeRequiresThreadResolved: true, expectedHeadSha: "h7" }, + reason: "unresolved review thread", + }); + + 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); }); it("REGRESSION (gate review): a LEGACY heuristic close row (closeRequiresMergeableState undefined, staged before the field existed) still gets the live recheck", async () => { @@ -911,6 +1075,10 @@ describe("agent approval queue (#779)", () => { // Same legacy row shape (closeRequiresMergeableState undefined) but the live mergeable-state read still // shows "dirty" -- the recheck fires (per the test above) but finds nothing stale, so the close proceeds. vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty"); + // closeRequiresThreadResolved is ALSO undefined on this legacy row, so the thread recheck fires too (same + // ambiguous-legacy discipline) -- give it a non-empty live result so only the conflict axis under test here + // determines the outcome, not an incidental "no threads left" default from the module mock. + vi.mocked(fetchLiveReviewThreadBlockers).mockResolvedValueOnce([{ title: "unrelated legacy blocker", scannerFinding: false }]); const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, @@ -947,7 +1115,9 @@ describe("agent approval queue (#779)", () => { installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", - params: { closeComment: "base conflict", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true, expectedHeadSha: "h7" }, + // closeRequiresThreadResolved explicitly false: this close was ONLY ever conflict-justified, so the new + // thread recheck must not incidentally fire (and spuriously "clear" via its own [] default) alongside it. + params: { closeComment: "base conflict", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true, closeRequiresThreadResolved: false, expectedHeadSha: "h7" }, reason: "base branch now conflicts", });