diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index f21d9dd014..4e1b08e1a3 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -268,6 +268,19 @@ function addRepoFindings(repo: RepositoryRecord, findings: AdvisoryFinding[]): v } } +/** #9129 (host-parity, src/rules/advisory.ts): corroboration for a duplicate-issue-link finding, beyond + * authored body text alone -- see the host copy's own doc comment for the full rationale. Kept in lock-step + * with the host's `hasDuplicateOverlapCorroboration` by the live-gate-parity contract test. */ +function hasDuplicateOverlapCorroboration(pr: PullRequestRecord, otherPr: PullRequestRecord): boolean { + const mineFiles = pr.changedFiles; + const theirsFiles = otherPr.changedFiles; + if (mineFiles && mineFiles.length > 0 && theirsFiles && theirsFiles.length > 0) { + const mine = new Set(mineFiles); + if (theirsFiles.some((path) => mine.has(path))) return true; + } + return Boolean(theirsFiles && theirsFiles.length > 0); +} + function addPullRequestFindings( repo: RepositoryRecord | null, pr: PullRequestRecord, @@ -312,13 +325,30 @@ function addPullRequestFindings( // suppressing duplicate evidence with arbitrary PR-number ordering. // Flag-OFF (default) short-circuits ⇒ the finding is pushed exactly as before (byte-identical). if (overlappingPrs.length > 0 && !(duplicateWinnerEnabled && isDuplicateClusterWinnerByClaim(pr, overlappingPrs))) { - findings.push({ - code: "duplicate_pr_risk", - severity: "warning", - title: "Linked issue overlaps another open PR", - detail: `Other open pull requests reference the same linked issue set: ${overlappingPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`, - action: "Review the related PRs before spending reviewer time on duplicate work.", - }); + // #9129 (host-parity): split by corroboration -- see hasDuplicateOverlapCorroboration + the host's own + // doc comment. A CONCRETE finding code (`duplicate_pr_risk`) is reserved for a sibling with real + // corroborating evidence beyond body text; a PURELY body-text overlap gets the separate, always-non- + // blocking `duplicate_pr_risk_unconfirmed` code (see resolveConfiguredGateMode below). + const corroboratedPrs = overlappingPrs.filter((otherPr) => hasDuplicateOverlapCorroboration(pr, otherPr)); + const uncorroboratedPrs = overlappingPrs.filter((otherPr) => !hasDuplicateOverlapCorroboration(pr, otherPr)); + if (corroboratedPrs.length > 0) { + findings.push({ + code: "duplicate_pr_risk", + severity: "warning", + title: "Linked issue overlaps another open PR with corroborating changes", + detail: `Other open pull requests reference the same linked issue set AND show corroborating changed-file overlap or a non-trivial diff: ${corroboratedPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`, + action: "This looks like a genuine race for the same issue. Coordinate with the other contributor, or wait for a maintainer to triage before assuming priority.", + }); + } + if (uncorroboratedPrs.length > 0) { + findings.push({ + code: "duplicate_pr_risk_unconfirmed", + severity: "info", + title: "Linked issue is also cited by another open PR", + detail: `Other open pull requests cite the same linked issue number, with no corroborating changed-file evidence yet: ${uncorroboratedPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`, + action: "This may be coincidental, or an early race that hasn't produced comparable code yet — verify manually before assuming it's a genuine duplicate.", + }); + } } } // Self-authored linked-issue detection: the PR author also filed the linked issue. Raised when at least @@ -566,6 +596,21 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy warnings: gateWarnings, }; } + // Duplicate-only HOLD (#9129, host-parity): a gate that would otherwise FAIL solely because of a + // same-linked-issue duplicate_pr_risk blocker is HELD for a human instead of closed outright — see the + // host copy's own doc comment for the full rationale. There is no duplicatePrGateMode configuration that + // can close a PR through this finding anymore. A blocker set that mixes it with a genuinely critical + // finding still falls through to the unconditional failure below. + if (blockers.every((blocker) => blocker.code === "duplicate_pr_risk")) { + return { + enabled: true, + conclusion: "neutral", + title: `${LOOPOVER_GATE_CHECK_NAME} — held for manual review`, + summary: blockers.map((finding) => sanitizeForCheckRun(finding.title)).join("; "), + blockers: [], + warnings: [...gateWarnings, ...blockers], + }; + } // Name the exact blocker(s) + fix in the title so the contributor sees WHY at a glance. const firstBlocker = blockers[0]; const titleDetail = blockers.length === 1 && firstBlocker ? sanitizeForCheckRun(firstBlocker.title) : `${blockers.length} blockers`; @@ -603,9 +648,18 @@ function gatePolicyBlocks(mode: GateRuleMode | undefined, defaultMode: GateRuleM function isConfiguredGateBlocker(finding: AdvisoryFinding, policy: GateCheckPolicy): boolean { const code = finding.code; // Missing linked issue defaults to ADVISORY — issues aren't always available, so it only blocks when a - // repo explicitly opts in with linkedIssueGateMode: "block". Duplicates still default to blocking. + // repo explicitly opts in with linkedIssueGateMode: "block". if (code === "missing_linked_issue") return gatePolicyBlocks(policy.linkedIssueGateMode, "advisory"); - if (code === "duplicate_pr_risk") return gatePolicyBlocks(policy.duplicatePrGateMode, "block"); + // #9129 (host-parity): default changed from "block" to "advisory" -- this finding is derived from another + // contributor's own PR body text, so blocking-by-default let anyone force-close a rival's PR for free. A + // maintainer who explicitly opts into "block" still gets real effect: evaluateGateCheckCore HOLDS (never + // closes) a gate that fails solely on this finding (see the duplicate-only hold there). Only ever produced + // for a CORROBORATED overlap (see hasDuplicateOverlapCorroboration) -- a purely body-text overlap uses the + // separate, always-non-blocking duplicate_pr_risk_unconfirmed code below. + if (code === "duplicate_pr_risk") return gatePolicyBlocks(policy.duplicatePrGateMode, "advisory"); + // #9129 (host-parity): an UNCORROBORATED duplicate-issue citation is NEVER a configured gate blocker, + // regardless of duplicatePrGateMode -- an adversary can manufacture this signal for free. + if (code === "duplicate_pr_risk_unconfirmed") return false; // A dual-model AI consensus defect blocks ONLY when the maintainer opted into aiReview: block. It is the // most conservative AI signal (two independent models) but still confirmed-contributor gated by // evaluateGateCheck, and advisory by default. diff --git a/src/github/app.ts b/src/github/app.ts index be7a392e22..b15a7e6c00 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -7,6 +7,7 @@ import { recordGitHubRateLimitObservation, updateInstallationPermissions } from import { recordClockSkewFromResponse } from "../selfhost/clock-skew"; import { clearGitHubResponseCacheForTest, + forcedSelfhostMode, githubHeaders, githubRateLimitAdmissionKeyForInstallation, makeInstallationOctokit, @@ -514,11 +515,15 @@ export async function getGithubUserCreatedAt( /** Sentinel result for cancelInFlightWorkflowRunsForHeadSha (#2462) -- mirrors CheckRunOutcome's shape (a * typed "degraded, not thrown" result) so a missing `actions: write` grant never has to be distinguished - * from a genuine network/API failure by the caller via exception type-narrowing. */ + * from a genuine network/API failure by the caller via exception type-narrowing. + * `suppressed` (#9130): the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch forced this call to no-op -- + * see cancelInFlightWorkflowRunsForHeadSha's own doc comment. Distinct from every other kind: no network call + * was even attempted, so a caller must never treat it as either a success or a failure worth alerting on. */ export type CancelWorkflowRunsOutcome = | { kind: "cancelled"; cancelledCount: number; totalFound: number } | { kind: "permission_missing"; warning: string } - | { kind: "error"; warning: string }; + | { kind: "error"; warning: string } + | { kind: "suppressed" }; // A rate-limit / secondary-limit 403 is NOT a permission gap -- mirrors isCheckRunPermissionError's own // exclusion (src/github/app.ts, isRateLimitedError check) so a burst-load 403 is never misrecorded as a @@ -625,7 +630,16 @@ async function cancelOneWorkflowRun( * Needs `actions: write` (list needs `actions: read`, effectively granted alongside write) -- an * installation that hasn't granted it gets a typed `permission_missing` result, never a thrown error, so * this can run as a best-effort side effect AFTER a close has already succeeded without risking that - * success being misrecorded as a failure. Greenfield: no existing Actions-API wrapper to extend. */ + * success being misrecorded as a failure. Greenfield: no existing Actions-API wrapper to extend. + * + * #9130: this is the last installation-scoped write that ran on raw `timeoutFetch`, entirely OUTSIDE + * `makeInstallationOctokit`'s own suppression hook -- a direct sibling of the #9067 gap. Every other write this + * executor performs is suppressed the instant the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch is set + * (the octokit hook, or -- since #9130 -- the executor's own `mode` never even reaching the "9) live" branch + * that calls this). This one didn't go through either mechanism, so a suppressed instance still cancelled a + * contributor's real CI run. Consulting `forcedSelfhostMode(env)` directly, the SAME chokepoint + * `makeInstallationOctokit` uses, closes that gap even for a future caller that reaches this function without + * going through executeAgentMaintenanceActions's own (now instance-mode-aware) gate. */ export async function cancelInFlightWorkflowRunsForHeadSha( env: Env, installationId: number, @@ -633,6 +647,7 @@ export async function cancelInFlightWorkflowRunsForHeadSha( headSha: string, pullNumber: number, ): Promise { + if (forcedSelfhostMode(env) !== null) return { kind: "suppressed" }; const parsed = parseRepoFullNameStrict(repoFullName); if (!parsed) return { kind: "error", warning: `Invalid repository full name: ${repoFullName}` }; const { owner, repo } = parsed; diff --git a/src/github/client.ts b/src/github/client.ts index 7b38a14e60..17ae5f608f 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -662,6 +662,7 @@ export async function resolveRepoActionMode(env: Env, settings: Pick { +): Promise<{ merged: boolean; sha: string | null; suppressed: boolean }> { const { owner, repo } = splitRepo(repoFullName); return withInstallationTokenRetry(env, installationId, async (token) => { const octokit = makeInstallationOctokit(env, token, "live", githubRateLimitAdmissionKeyForInstallation(installationId)); @@ -116,8 +124,8 @@ export async function mergePullRequest( merge_method: options.mergeMethod, ...(options.sha ? { sha: options.sha } : {}), }); - const data = response.data as { merged?: boolean; sha?: string }; - return { merged: data.merged ?? true, sha: data.sha ?? null }; + const data = response.data as { merged?: boolean; sha?: string; dryRunSuppressed?: boolean }; + return { merged: data.merged ?? true, sha: data.sha ?? null, suppressed: data.dryRunSuppressed === true }; }); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cd106d8e36..d5ff29f06f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -177,6 +177,7 @@ import { removePullRequestLabel, } from "../github/labels"; import { + forcedSelfhostMode, githubRateLimitAdmissionKeyForInstallation, githubRateLimitAdmissionKeyForToken, resolveRepoActionMode, @@ -1451,6 +1452,7 @@ export async function sweepRepoRegate( return; const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), // env brake OR DB kill-switch (#audit-§5.2) + instanceMode: forcedSelfhostMode(env), // #9130: the instance-level kill switch is now a first-class precedence term here agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -1827,6 +1829,7 @@ export async function sweepRepoBacklogConvergence( if (!(isConvergenceRepoAllowed(env, repoFullName) || isAgentConfigured(settings.autonomy))) return; const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -2263,6 +2266,132 @@ export function agentDispositionLabels( return { actionClass, blockerClass: gateBlockerCodes[0] ?? holdReasonCode ?? "none" }; } +/** + * #9132: the reusable core of the main disposition path's precision-breaker + gate-decision-recording sequence + * (the inline block just below `buildAgentMaintenancePlanInput`'s own call site, `applyPrecisionBreakers` through + * `recordPredictedGateCalibration`), scoped to a POLICY close that never went through a real gate evaluation -- + * today, the two review-nag close sites (maybeThrottleReviewNagPing / the monitored-mention nag handler). + * + * #9086 added `"review_nag"` to `CONTENT_INSPECTION_CLOSE_KINDS` (agent-actions.ts) so `downgradeCloseToHold` + * (the close-precision breaker) would cover it — but that fix was INERT: neither review-nag close site ever + * called `applyPrecisionBreakers` or `recordNativeGateDecision` at all; they built a plan with + * `planAgentMaintenanceActions` and handed it straight to `executeAgentMaintenanceActions`. Two consequences: + * the breaker can never engage for a wrong review-nag close no matter how wrong it is, and — because no + * `gate_decision` row is ever written — `queryRuleGateCells`'s `gd JOIN po` later pairs the close's `pr_outcome` + * against whatever the PR's LAST real quality verdict happened to be, scoring a review-nag close as a MERGE + * misprediction and dragging the repo's measured merge precision toward the holdonly floor (#8825's exact bug + * class, whose `policy_close:${closeKind}` reasonCode fix landed only at the main disposition site). + * + * Does NOT write the `decision_records`/`decision_ledger` row itself: #9134 (merged separately, ahead of this + * fix on rebase) hoisted that write into `executeAgentMaintenanceActions` itself via the now-required + * `AgentActionExecutionContext.decisionRecord` field, so every caller of this function already gets a record + * for whatever this function's returned plan actually executes to, by construction, with NO extra call here -- + * an extra call here would double-write. `defaultDecisionRecordReasonCode`'s `policy_close:` fallback + * (the executor's generic case, used whenever a caller's `decisionRecord` context omits `reasonCode`, which + * both call sites below do) is the exact same convention `deriveDecisionReasonCode` computes for a policy close + * below, so the calibration join sees an identical `reasonCode` either way. + * + * A policy close has no gate: no blockers, no AI judgment, no CI state, no base SHA — this deliberately omits + * every gate-specific input the main disposition path also threads through (AI-judgment confidence/salvageability, + * ciState, the decision-replay input) since none of them apply to a policy-only close. `conclusion: "skipped"` + * (the same value `planAgentMaintenanceActions`'s own `conclusion` input already uses for this scenario) stands + * in for "no gate ran" everywhere a `GateCheckConclusion` is required. + * + * Returns the FINAL, breaker-and-holdout-applied plan for the caller to execute via + * `executeAgentMaintenanceActions` — the caller must never execute the pre-breaker `planned` array directly, or + * every one of the fixes below is silently bypassed. + */ +async function finalizePolicyCloseDisposition( + env: Env, + args: { + repoFullName: string; + pullNumber: number; + headSha: string | null | undefined; + authorLogin?: string | null | undefined; + deliveryId: string; + planned: PlannedAgentAction[]; + settings: Awaited>; + }, +): Promise { + const labelSettings: AgentDispositionLabelSettings = { + manualReviewLabel: args.settings.manualReviewLabel, + readyToMergeLabel: args.settings.readyToMergeLabel, + changesRequestedLabel: args.settings.changesRequestedLabel, + migrationCollisionLabel: args.settings.migrationCollisionLabel, + pendingClosureLabel: args.settings.pendingClosureLabel, + }; + // Mirrors the main disposition path's own breakerMinerAuthored derivation exactly (#2352 scope parity) -- + // a review-nag close on a confirmed official miner's PR must join the SAME miner-scoped precision track + // record as every other close, not silently fall back to the non-miner scope. + const breakerMinerAuthored = args.authorLogin + ? ( + await getCachedOfficialMinerDetection(env, args.authorLogin, { + targetKey: `${args.repoFullName}#${args.pullNumber}`, + deliveryId: args.deliveryId, + }) + ).status === "confirmed" + : false; + const breakerOnPlan = applyPrecisionBreakers( + args.planned, + await isHoldOnly(env, args.repoFullName, breakerMinerAuthored), + await isCloseHoldOnly(env, args.repoFullName, breakerMinerAuthored), + labelSettings, + await readUntrustworthyRuleCodes(env), + ); + for (const direction of precisionBreakerDowngradeDirections(args.planned, breakerOnPlan)) { + incr("loopover_precision_breaker_downgrades_total", { direction }); + } + // #9135: holdout diversion only ever targets a `closeKind: "heuristic"` close (holdoutEligibleClose's own + // filter) -- a policy close routed through this function is always some OTHER closeKind (e.g. "review_nag"), + // so `holdout` is structurally always null here and there is no divertedByHoldout to thread onward; only the + // (possibly-diverted, harmlessly-unmodified-in-practice) plan matters to this function's callers. + const { planned: holdoutOnPlan } = await maybeApplyCloseAuditHoldout(env, { + repoFullName: args.repoFullName, + pullNumber: args.pullNumber, + headSha: args.headSha, + planned: breakerOnPlan, + epsilonPct: args.settings.closeAuditHoldoutPct, + closeAutonomyIsAuto: resolveAutonomy(args.settings.autonomy, "close") === "auto", + labelSettings, + }); + const disposition = agentDispositionLabels(holdoutOnPlan, [], null); + incr("loopover_agent_disposition_total", { + repo: args.repoFullName, + action_class: disposition.actionClass, + blocker_class: disposition.blockerClass, + autonomy_level: resolveAutonomy(args.settings.autonomy, disposition.actionClass === "merge" ? "merge" : "close"), + }); + const policyCloseKind = + disposition.actionClass === "close" + ? holdoutOnPlan.find((planned) => planned.actionClass === "close" && planned.closeKind !== undefined)?.closeKind + : undefined; + const reasonCode = deriveDecisionReasonCode(disposition.blockerClass, policyCloseKind ?? null, "skipped"); + await recordNativeGateDecision(env, { + project: args.repoFullName, + pullNumber: args.pullNumber, + headSha: args.headSha, + conclusion: "skipped", + action: disposition.actionClass, + reasonCode, + minerAuthored: breakerMinerAuthored, + }); + await recordContributorGateDecision(env, { + login: args.authorLogin, + project: args.repoFullName, + pullNumber: args.pullNumber, + headSha: args.headSha, + decision: disposition.actionClass, + }); + await recordPredictedGateCalibration(env, { + login: args.authorLogin, + project: args.repoFullName, + pullNumber: args.pullNumber, + headSha: args.headSha, + decision: disposition.actionClass, + }); + return holdoutOnPlan; +} + const AGENT_HOLD_AUDIT_REASON_MAX_LENGTH = 240; function boundAgentHoldAuditReason(reason: string): string { @@ -2889,6 +3018,7 @@ async function maybeCloseForContributorCapOnOpen( if (isNewAccount && resolveAutonomy(settings.autonomy, "review_state_label") === "auto") { const newAccountMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -3283,6 +3413,7 @@ async function runAgentMaintenancePlanAndExecute( if (isNewAccount && resolveAutonomy(settings.autonomy, "review_state_label") === "auto") { const newAccountMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -6514,7 +6645,7 @@ async function maybeHandleIssueCommentCommandWebhookEvent( * rest of this webhook delivery's processing. */ async function recordDraftConversionCiCancelOutcome(env: Env, installationId: number, repoFullName: string, pullNumber: number, headSha: string, settings: RepositorySettings): Promise { const targetKey = `${repoFullName}#${pullNumber}`; - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); if (mode !== "live") { await recordAuditEvent(env, { eventType: "github_app.draft_convert_ci_cancel_skipped", @@ -6539,6 +6670,11 @@ async function recordDraftConversionCiCancelOutcome(env: Env, installationId: nu }).catch(() => undefined); return; } + // #9130: structurally unreachable here in practice -- the mode !== "live" check above already returns before + // ever calling cancelInFlightWorkflowRunsForHeadSha once forcedSelfhostMode(env) makes `mode` non-live -- but + // handled explicitly rather than falling through to the generic error branch below and reading a `.warning` + // field this variant does not have. + if (outcome.kind === "suppressed") return; const eventType = outcome.kind === "permission_missing" ? "github_app.draft_convert_ci_cancel_permission_missing" : "github_app.draft_convert_ci_cancel_failed"; await recordAuditEvent(env, { eventType, actor: "loopover", targetKey, outcome: "error", detail: outcome.warning, metadata: { repoFullName, headSha, reason: outcome.kind } }).catch(() => undefined); } @@ -7263,6 +7399,7 @@ async function handleIssueWebhookEvent( if (resolveAutonomy(issueSettings.autonomy, "review_state_label") === "auto") { const newAccountMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: issueSettings.agentPaused, agentDryRun: issueSettings.agentDryRun, }); @@ -9130,7 +9267,7 @@ async function maybeApplyManifestPolicyGate( // the cost of a maintainer choosing to ask twice). const alreadyTriggered = await hasAuditEventForHeadSha(env, "github_app.e2e_tests_generation", e2eTargetKey, args.pr.headSha); if (!alreadyTriggered) { - const e2eMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: args.settings.agentPaused, agentDryRun: args.settings.agentDryRun }); + const e2eMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: args.settings.agentPaused, agentDryRun: args.settings.agentDryRun }); if (e2eMode === "live") { await runE2eTestGenerationAndDeliver(env, { repoFullName: args.repoFullName, @@ -12684,6 +12821,7 @@ async function maybeProcessGateOverrideCommand( // flipping the live Gate check-run to neutral and posting a real confirmation comment. const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -12849,7 +12987,7 @@ async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: const gate = evaluateGateCheck(advisory, gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null, undefined, undefined, await resolveAutomaticCloseConfidence(env, req.repoFullName, await getAiReviewCloseConfidenceOverride(env, req.repoFullName)))); const selection = selectWarningsForResolve(gate.warnings, findingRef); if (selection.reason === "finding_not_found") { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey, outcome: "completed", detail: selection.reason, metadata: { deliveryId, repoFullName: req.repoFullName, reason: selection.reason } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "skipped", metadata: { reason: selection.reason } }); return true; } - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); if (mode !== "live") { const skipReason = mode === "dry_run" ? "dry_run" : "agent_paused"; await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey, outcome: "completed", detail: skipReason, metadata: { deliveryId, repoFullName: req.repoFullName, reason: skipReason } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "skipped", metadata: { reason: skipReason } }); return true; } const reviewManifest = await loadRepoFocusManifest(env, req.repoFullName).catch(() => null); const reviewMemoryEnabled = shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(reviewManifest)); @@ -12901,7 +13039,7 @@ async function maybeProcessReviewCommand(env: Env, deliveryId: string, payload: } // Same dry-run/paused gate every other action command respects (pause/resolve/explain/gate-override/ // generate-tests) -- a paused or dry-run repo must not dispatch a live re-review or post a confirmation. - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); if (mode !== "live") { await recordReviewCommandSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, mode === "dry_run" ? "dry_run" : "agent_paused"); return true; @@ -13154,7 +13292,7 @@ async function maybeProcessGenerateTestsCommand(env: Env, deliveryId: string, pa } // Same dry-run/paused gate every other action command respects (mirrors maybeProcessResolveCommand's own // resolveAgentActionMode check) — an agent-paused or dry-run repo gets no generated content posted at all. - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); if (mode !== "live") { const skipReason = mode === "dry_run" ? "dry_run" : "agent_paused"; await recordGenerateTestsSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, skipReason); @@ -13366,6 +13504,7 @@ async function maybeProcessConfigurationCommand( } const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -13505,6 +13644,7 @@ async function maybeProcessPlanCommand( // both dry_run and paused, not just paused. const planMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -13912,7 +14052,7 @@ async function maybeProcessPrPanelGenerateTests( await recordGenerateTestsSkip(env, deliveryId, repoFullName, `${repoFullName}#${pr.number}`, actor, "feature_disabled"); return true; } - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); if (mode !== "live") { const skipReason = mode === "dry_run" ? "dry_run" : "agent_paused"; await recordGenerateTestsSkip(env, deliveryId, repoFullName, `${repoFullName}#${pr.number}`, actor, skipReason); @@ -14255,6 +14395,7 @@ async function maybeThrottleReviewNagPing( const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -14325,6 +14466,20 @@ async function maybeThrottleReviewNagPing( return true; } + // #9132: route the plan through the SAME precision-breaker + gate-decision-recording sequence the main + // disposition path uses (finalizePolicyCloseDisposition) before ever executing it -- previously `planned` + // was handed to executeAgentMaintenanceActions directly, so #9086's review_nag breaker coverage never + // actually engaged and no gate_decision row was ever written for this close. + const finalPlanned = await finalizePolicyCloseDisposition(env, { + repoFullName, + pullNumber: pr.number, + headSha: pr.headSha, + authorLogin: pr.authorLogin, + deliveryId, + planned, + settings, + }); + const installation = await getInstallation(env, installationId); await executeAgentMaintenanceActions( env, @@ -14343,7 +14498,7 @@ async function maybeThrottleReviewNagPing( // contributor-disputable close the issue flagged as biasing the risk-control calibration join. decisionRecord: { configDigest: await contentDigest(settings) }, }, - planned, + finalPlanned, ); return true; } @@ -14456,6 +14611,7 @@ async function maybeThrottleMonitoredMentions( const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); @@ -14519,6 +14675,20 @@ async function maybeThrottleMonitoredMentions( return true; } + // #9132: route the plan through the SAME precision-breaker + gate-decision-recording sequence the main + // disposition path uses (finalizePolicyCloseDisposition) before ever executing it -- previously `planned` + // was handed to executeAgentMaintenanceActions directly, so #9086's review_nag breaker coverage never + // actually engaged and no gate_decision row was ever written for this close. + const finalPlanned = await finalizePolicyCloseDisposition(env, { + repoFullName, + pullNumber: pr.number, + headSha: pr.headSha, + authorLogin: pr.authorLogin, + deliveryId, + planned, + settings, + }); + const installation = await getInstallation(env, installationId); await executeAgentMaintenanceActions( env, @@ -14537,7 +14707,7 @@ async function maybeThrottleMonitoredMentions( // all -- the same gap as its comment-thread-cooldown sibling immediately above. decisionRecord: { configDigest: await contentDigest(settings) }, }, - planned, + finalPlanned, ); return true; } @@ -14938,6 +15108,7 @@ async function maybeProcessLoopOverMentionCommand( // card is a live public comment post, same as gate-override's confirmation comment. const mentionMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); diff --git a/src/queue/review-evasion.ts b/src/queue/review-evasion.ts index acf8a27ee0..54f5895c45 100644 --- a/src/queue/review-evasion.ts +++ b/src/queue/review-evasion.ts @@ -20,6 +20,7 @@ import { terminalizeActiveReviewTracking, } from "../db/repositories"; import { getRepositoryCollaboratorPermission } from "../github/app"; +import { forcedSelfhostMode } from "../github/client"; import { ensurePullRequestLabel } from "../github/labels"; import { fetchPullRequestFreshness, pullRequestFreshnessDetail, type PullRequestFreshness } from "../github/pr-freshness"; import { closePullRequest, createIssueComment, getLastCloserLogin, getLastReopenerLogin, reopenPullRequest } from "../github/pr-actions"; @@ -148,6 +149,7 @@ async function evaluateCloseEnforcementGate(args: { const { env, installationId, repoFullName, pr, settings, eventType, targetKey } = args; const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 1703e22858..d2780dc339 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -298,13 +298,19 @@ export function resolveAiReviewSalvageableHold( // DUPLICATE-ONLY blocker codes: findings whose own severity is always "warning" (advisory by nature — a // same-linked-issue overlap is a lead for a human, not proof of a defect) but that a per-repo gate-mode config -// can still escalate into a hard blocker (`duplicate_pr_risk` under `duplicatePrGateMode: "block"`, its ONLY -// escalation path — see isConfiguredGateBlocker). Kept to exactly this code, NOT "every warning-severity finding": -// `missing_linked_issue`, `self_authored_linked_issue`, `manifest_linked_issue_required`, and +// can still escalate into a configured blocker (`duplicate_pr_risk` under `duplicatePrGateMode: "block"`, its +// ONLY escalation path — see isConfiguredGateBlocker). Kept to exactly this code, NOT "every warning-severity +// finding": `missing_linked_issue`, `self_authored_linked_issue`, `manifest_linked_issue_required`, and // `manifest_missing_tests` are ALSO severity "warning" and ALSO block-mode-escalatable via their own maintainer- // configured gate (linkedIssueGateMode / selfAuthoredLinkedIssueGateMode / manifestPolicyGateMode), and a // maintainer who explicitly opted one of THOSE into "block" must have it still close a PR outright — only the -// same-linked-issue overlap concern is meant to downgrade to a hold for a decisive surface-lane merge. +// same-linked-issue overlap concern downgrades to a hold instead. +// #9129: evaluateGateCheckCore itself now downgrades a duplicate-only blocker set straight to a HOLD (neutral), +// generalizing what USED to be a content-lane-only override (content-lane-wire.ts's applySurfaceGate, guard #4) +// to every repo — so a GateCheckEvaluation this module produces can no longer reach `conclusion: "failure"` with +// every blocker in this set; `isDuplicateOnlyFailure` below stays exported/tested for content-lane-wire.ts's own +// (now largely defense-in-depth) narrower use and for any hand-constructed evaluation that bypasses +// evaluateGateCheckCore. export const DUPLICATE_ONLY_BLOCKER_CODES = new Set(["duplicate_pr_risk"]); /** True when the gate FAILED *solely* because of duplicate-only blockers (every blocker is in @@ -800,6 +806,27 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy warnings: gateWarnings, }; } + // Duplicate-only HOLD (#9129): a gate that would otherwise FAIL solely because of a same-linked-issue + // duplicate_pr_risk blocker (escalated by duplicatePrGateMode: "block") is HELD for a human instead of closed + // outright — never a failure, so this can never one-shot-close a contributor PR. Two independent PRs racing + // for the same issue is a maintainer-triage decision, not one ORB should resolve unilaterally using data the + // "losing" PR's own rival controls (the adversarial attack this issue closes: cite the same issue number in a + // throwaway PR, no code required, and force the victim's clean PR to close). Reuses DUPLICATE_ONLY_BLOCKER_CODES + // (previously scoped only to content-lane-wire.ts's narrower surface-gate override, guard #4) so "block" mode's + // meaning for this one finding is "hold both sides", never "close either" — there is no duplicatePrGateMode + // configuration that can close a PR through this finding anymore. A blocker set that MIXES a duplicate finding + // with a genuinely critical one, or another maintainer-configured block-mode finding, is NOT duplicate-only and + // still falls through to the unconditional failure below (a real defect still closes). + if (blockers.every((blocker) => DUPLICATE_ONLY_BLOCKER_CODES.has(blocker.code))) { + return { + enabled: true, + conclusion: "neutral", + title: `${LOOPOVER_GATE_CHECK_NAME} — held for manual review`, + summary: blockers.map((finding) => sanitizeForCheckRun(finding.title)).join("; "), + blockers: [], + warnings: [...gateWarnings, ...blockers], + }; + } // Name the exact blocker(s) + fix in the title so the contributor sees WHY at a glance. const firstBlocker = blockers[0]; const titleDetail = blockers.length === 1 && firstBlocker ? sanitizeForCheckRun(firstBlocker.title) : `${blockers.length} blockers`; @@ -899,6 +926,27 @@ function addRepoFindings(repo: RepositoryRecord, findings: AdvisoryFinding[]): v } } +/** #9129: corroboration for a duplicate-issue-link finding, beyond authored body text alone. `pr.linkedIssues` + * (and therefore the overlap itself) is entirely contributor-controlled body text — an adversary can cite any + * issue number in their own throwaway PR's body for free, with no code required, and force this PR to be seen + * as "duplicated". Corroboration requires EITHER a genuine changed-file-path overlap with the sibling (both + * PRs' `changedFiles` are resolved and share at least one path — they are plausibly touching the same area of + * the codebase, not just citing the same issue number) OR the sibling itself being a non-trivial real change + * (it has at least one resolved changed file at all, ruling out a hollow, no-diff PR that exists purely to cite + * the issue and force a close). Absent `changedFiles` data on either side — the common case today, since the + * open-PR file-collision enrichment (`enrichOpenPullRequestsWithChangedFiles`, #2653) is deliberately scoped + * away from the gate's own `otherOpenPullRequests` input (see its own scoping note in processors.ts) — degrades + * to "uncorroborated", the safe default: it can never manufacture false corroboration from missing data. PURE. */ +function hasDuplicateOverlapCorroboration(pr: PullRequestRecord, otherPr: PullRequestRecord): boolean { + const mineFiles = pr.changedFiles; + const theirsFiles = otherPr.changedFiles; + if (mineFiles && mineFiles.length > 0 && theirsFiles && theirsFiles.length > 0) { + const mine = new Set(mineFiles); + if (theirsFiles.some((path) => mine.has(path))) return true; + } + return Boolean(theirsFiles && theirsFiles.length > 0); +} + function addPullRequestFindings( repo: RepositoryRecord | null, pr: PullRequestRecord, @@ -945,13 +993,33 @@ function addPullRequestFindings( // suppressing duplicate evidence with arbitrary PR-number ordering. // Flag-OFF (default) short-circuits ⇒ the finding is pushed exactly as before (byte-identical). if (overlappingPrs.length > 0 && !(duplicateWinnerEnabled && isDuplicateClusterWinnerByClaim(pr, overlappingPrs))) { - findings.push({ - code: "duplicate_pr_risk", - severity: "warning", - title: "Linked issue overlaps another open PR", - detail: `Other open pull requests reference the same linked issue set: ${overlappingPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`, - action: "Review the related PRs before spending reviewer time on duplicate work.", - }); + // #9129: split by corroboration (hasDuplicateOverlapCorroboration) — see its own doc comment. A CONCRETE + // finding code (`duplicate_pr_risk`) is reserved for a sibling with real corroborating evidence beyond body + // text; it stays configurable via duplicatePrGateMode and evaluateGateCheckCore now HOLDS (never closes) a + // gate that fails solely on it (#9129, see the duplicate-only hold below). A PURELY body-text overlap gets + // a SEPARATE, always-non-blocking code (`duplicate_pr_risk_unconfirmed`, see resolveConfiguredGateMode) — + // an adversary who cites the same issue number in a throwaway PR body, with no code required, can never + // manufacture a hold or a close through this path. + const corroboratedPrs = overlappingPrs.filter((otherPr) => hasDuplicateOverlapCorroboration(pr, otherPr)); + const uncorroboratedPrs = overlappingPrs.filter((otherPr) => !hasDuplicateOverlapCorroboration(pr, otherPr)); + if (corroboratedPrs.length > 0) { + findings.push({ + code: "duplicate_pr_risk", + severity: "warning", + title: "Linked issue overlaps another open PR with corroborating changes", + detail: `Other open pull requests reference the same linked issue set AND show corroborating changed-file overlap or a non-trivial diff: ${corroboratedPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`, + action: "This looks like a genuine race for the same issue. Coordinate with the other contributor, or wait for a maintainer to triage before assuming priority.", + }); + } + if (uncorroboratedPrs.length > 0) { + findings.push({ + code: "duplicate_pr_risk_unconfirmed", + severity: "info", + title: "Linked issue is also cited by another open PR", + detail: `Other open pull requests cite the same linked issue number, with no corroborating changed-file evidence yet: ${uncorroboratedPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`, + action: "This may be coincidental, or an early race that hasn't produced comparable code yet — verify manually before assuming it's a genuine duplicate.", + }); + } } } // Self-authored linked-issue detection: the PR author also filed the linked issue. Raised when at least @@ -1105,9 +1173,23 @@ export const DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE = 0.93; function resolveConfiguredGateMode(finding: AdvisoryFinding, policy: GateCheckPolicy): GateRuleMode { const code = finding.code; // Missing linked issue defaults to ADVISORY — issues aren't always available, so it only blocks when a - // repo explicitly opts in with linkedIssueGateMode: "block". Duplicates still default to blocking. + // repo explicitly opts in with linkedIssueGateMode: "block". if (code === "missing_linked_issue") return gateMode(policy.linkedIssueGateMode ?? "advisory"); - if (code === "duplicate_pr_risk") return gateMode(policy.duplicatePrGateMode ?? "block"); + // #9129: default changed from "block" to "advisory" — the input this finding is derived from (another + // contributor's own PR body text) is adversary-controlled, so blocking-by-default let anyone force-close a + // rival's PR for free. A maintainer who explicitly opts into "block" still gets real effect: evaluateGateCheckCore + // now HOLDS (never closes) a gate that fails solely on this finding (see the duplicate-only hold there, reusing + // DUPLICATE_ONLY_BLOCKER_CODES) — "block" mode's meaning for this one code is "hold both sides for a human", + // never "close either automatically". This code is ALSO only ever produced for a CORROBORATED overlap (real + // changed-file evidence, not just body text — see hasDuplicateOverlapCorroboration); a purely body-text overlap + // uses the separate, always-non-blocking duplicate_pr_risk_unconfirmed code below. + if (code === "duplicate_pr_risk") return gateMode(policy.duplicatePrGateMode ?? "advisory"); + // #9129: an UNCORROBORATED duplicate-issue citation (body-text overlap only) is NEVER a configured gate + // blocker, regardless of duplicatePrGateMode — an adversary can manufacture this signal for free (cite the + // same issue number in a throwaway PR body, no code required). This branch exists to make that guarantee + // explicit rather than relying on the default "off" fallback at the bottom of this function, so it can never + // silently regress if a future edit adds an unrelated case above it. + if (code === "duplicate_pr_risk_unconfirmed") return "off"; // A dual-model AI consensus defect blocks ONLY when the maintainer opted into aiReview: block. It is the // most conservative AI signal (two independent models) but still confirmed-contributor gated by // evaluateGateCheck, and advisory by default. diff --git a/src/selfhost/foreground-liveness.ts b/src/selfhost/foreground-liveness.ts index 14232e5f89..c2db0aee87 100644 --- a/src/selfhost/foreground-liveness.ts +++ b/src/selfhost/foreground-liveness.ts @@ -2,16 +2,29 @@ // agent-regate-pr, agent-regate-sweep, recapture-preview -- everything at or above FOREGROUND_QUEUE_PRIORITY_FLOOR, // see queue-common.ts) must always have a BOUNDED runnable trickle, mirroring the maintenance lane's own // maxDeferAgeMs escape hatch (maintenance-admission.ts). Unlike maintenance jobs, foreground jobs never go through -// an admission gate of their own -- only the GitHub rate-limit admission check (processOne, before consume()) and -// the rate-limit BUDGET sweep (deferPendingJobsForRateLimit) can push a foreground job's run_after into the -// future, and NEITHER exempts foreground priority the way maintenance-admission exempts it entirely: a -// GITHUB_BUDGET_BACKGROUND_TYPES job like agent-regate-pr (a literal "contributor PR review", priority 9, -// foreground) is rate-limited with the SAME conservative headroom as genuine maintenance sweeps -// (MAINTENANCE_RESERVED_HEADROOM, see queue-common.ts's githubRateLimitAdmissionTargetForJob), so a shared REST -// budget drained by a post-deploy catch-up burst can defer it for the full rate-limit reset window (up to -// MAX_GITHUB_RATE_LIMIT_RETRY_MS = 65 minutes) with no floor. Without this module, that lane can silently starve -// entirely: hundreds of pending contributor-PR-review jobs, zero processing, zero runnable, requiring manual -// intervention -- the production incident this module exists to make structurally impossible. +// an admission gate that EXEMPTS them the way maintenance-admission exempts maintenance jobs entirely -- but three +// distinct ADMISSION GATES (not just rate-limiting) can each independently push a foreground job's run_after into +// the future: the GitHub rate-limit admission check (processOne, before consume(), plus the rate-limit BUDGET +// sweep, deferPendingJobsForRateLimit), the per-installation concurrency admission check, and (for jobs sharing a +// priority band with maintenance work) the maintenance-admission check itself. A GITHUB_BUDGET_BACKGROUND_TYPES +// job like agent-regate-pr (a literal "contributor PR review", priority 9, foreground) is rate-limited with the +// SAME conservative headroom as genuine maintenance sweeps (MAINTENANCE_RESERVED_HEADROOM, see queue-common.ts's +// githubRateLimitAdmissionTargetForJob), so a shared REST budget drained by a post-deploy catch-up burst can defer +// it for the full rate-limit reset window (up to MAX_GITHUB_RATE_LIMIT_RETRY_MS = 65 minutes) with no floor. +// Without this module, that lane can silently starve entirely: hundreds of pending contributor-PR-review jobs, +// zero processing, zero runnable, requiring manual intervention -- the production incident this module exists to +// make structurally impossible. +// +// #9127 -- CORRECTION: an earlier version of this comment claimed admission gates were the ONLY way a foreground +// job's run_after could land in the future. That premise was false: enqueue(message, delaySeconds) sets +// run_after = now + delaySeconds*1000 for foreground job types too -- e.g. the linked-issue flag-then-close grace +// window (processors.ts) enqueues a delayed "recapture-preview" verify job the exact same way. Because the +// candidate SELECT below had no provenance filter, it could not tell a deliberate enqueue-time delay from an +// admission-gate defer, and released BOTH -- collapsing a promised multi-minute grace window to one sweep tick +// (as little as FOREGROUND_LIVENESS_CHECK_INTERVAL_MS, 60s default). The `deferred_by` column (set ONLY at the +// three admission-gate defer sites, never by enqueue()) is the provenance filter that fixes this: the candidate +// SELECT now requires `deferred_by IS NOT NULL`, so an enqueue-time delay is structurally ineligible for release +// regardless of age or rate-limit state, no matter how long it is pending. // // The queue backends (pg-queue.ts / sqlite-queue.ts) run releaseStaleForegroundDeferrals() periodically (see // start()) AND once at boot (init()), so a restart/deploy self-heals inherited over-deferral instead of needing diff --git a/src/selfhost/pg-queue.ts b/src/selfhost/pg-queue.ts index f5e4932662..67fd208a80 100644 --- a/src/selfhost/pg-queue.ts +++ b/src/selfhost/pg-queue.ts @@ -197,6 +197,10 @@ ALTER TABLE ${TABLE} ADD COLUMN IF NOT EXISTS claim_sort_key BIGINT NOT NULL DEF ALTER TABLE ${TABLE} ADD COLUMN IF NOT EXISTS is_maintenance INTEGER NOT NULL DEFAULT 0; ALTER TABLE ${TABLE} ADD COLUMN IF NOT EXISTS foreground_lane TEXT; ALTER TABLE ${TABLE} ADD COLUMN IF NOT EXISTS dead_at BIGINT; +-- #9127: provenance tag for releaseStaleForegroundDeferrals' rate-limit-clear recheck. Set ONLY at an admission +-- gate's own defer site (rate-limit / maintenance-admission / installation-concurrency), NEVER by enqueue()'s +-- delaySeconds -- see foreground-liveness.ts's module header for the full rationale. +ALTER TABLE ${TABLE} ADD COLUMN IF NOT EXISTS deferred_by TEXT; DROP INDEX IF EXISTS ${TABLE}_claim; CREATE INDEX IF NOT EXISTS ${TABLE}_claim ON ${TABLE}(status, priority, claim_sort_key, run_after); CREATE INDEX IF NOT EXISTS ${TABLE}_pending_job_key ON ${TABLE}(job_key, status); @@ -779,21 +783,33 @@ export function createPgQueue( } /** See foreground-liveness.ts for the full rationale. A bounded candidate SELECT (foreground-priority, pending, - * not currently due), an eligibility pass, a ramp-up CAP, then a per-row conditional UPDATE only for the - * capped subset -- mirroring reviveEligibleDeadJobs' shape but with the extra ramp-up step. Each candidate is - * ELIGIBLE on EITHER of two independent conditions: it has genuinely been waiting past the age-based trickle - * ceiling (isForegroundDeferralStale, unconditional backstop), OR -- CONDITION-BASED recovery - * (#selfhost-queue-liveness VPS incident) -- re-evaluating rateLimitAdmissionDelayMs against CURRENT - * observations right now says it would be admitted immediately. The age floor alone can leave a job pinned to - * a stale reset timestamp for up to its full original delay (observed up to ~15m) even when a fresher, - * healthier observation arrived moments after it was deferred; the condition check recovers it on the NEXT - * sweep tick instead (bounded by FOREGROUND_LIVENESS_CHECK_INTERVAL_MS, default 60s) whenever the underlying - * rate-limit pressure has actually cleared, regardless of job age. When more jobs are eligible than + * not currently due, ADMISSION-GATE-DEFERRED -- see below), an eligibility pass, a ramp-up CAP, then a per-row + * conditional UPDATE only for the capped subset -- mirroring reviveEligibleDeadJobs' shape but with the extra + * ramp-up step. Each candidate is ELIGIBLE on EITHER of two independent conditions: it has genuinely been + * waiting past the age-based trickle ceiling (isForegroundDeferralStale, unconditional backstop), OR -- + * CONDITION-BASED recovery (#selfhost-queue-liveness VPS incident), restricted to rows an admission gate itself + * deferred for a rate-limit reason (`deferred_by='rate_limit'`) -- re-evaluating rateLimitAdmissionDelayMs + * against CURRENT observations right now says it would be admitted immediately. The age floor alone can leave a + * job pinned to a stale reset timestamp for up to its full original delay (observed up to ~15m) even when a + * fresher, healthier observation arrived moments after it was deferred; the condition check recovers it on the + * NEXT sweep tick instead (bounded by FOREGROUND_LIVENESS_CHECK_INTERVAL_MS, default 60s) whenever the + * underlying rate-limit pressure has actually cleared, regardless of job age. When more jobs are eligible than * maxReleasePerSweep allows, selectForegroundDeferralsToRelease picks the oldest first -- a large inherited * backlog drains gradually over several sweep ticks instead of flooding GitHub with every re-attempt at once. * Logs + records a metric ONCE per sweep (aggregate count), not per row, so a large release batch cannot spam * the log. * + * #9127 -- PROVENANCE filter (`deferred_by IS NOT NULL`, enforced by the candidate SELECT's own WHERE clause, + * not by an application-side check on the returned rows): only a row an admission gate itself deferred + * (rate-limit / maintenance-admission / installation-concurrency -- see the three defer sites' own + * `deferred_by=COALESCE(deferred_by, ...)` writes) is EVER a release candidate. A row whose future run_after + * came from enqueue(message, delaySeconds) -- a deliberate delay, e.g. the linked-issue flag-then-close grace + * window -- never has deferred_by set, so it is structurally excluded here regardless of age or rate-limit + * state. Without this filter, EVERY pending foreground row scheduled in the future qualified, and + * isRateLimitAdmissionNowClear degrades to "clear" for any job with no rate-limit bucket at all (most enqueue- + * time delays), so a deliberate delay was released on the very next sweep tick almost regardless of its + * intended duration -- see foreground-liveness.ts's module header for the full incident writeup. + * * Candidate selection queries an OLDEST window AND a NEWEST window (#selfhost-queue-liveness clear-bucket * starvation fix), not just one oldest-first window. A single `ORDER BY created_at ASC LIMIT` window can be * filled ENTIRELY by older still-rate-limited jobs once the backlog exceeds the limit -- selectForegroundDeferralsToRelease's @@ -809,16 +825,16 @@ export function createPgQueue( const candidateLimit = foregroundLivenessConfig.maxReleasePerSweep; const [oldestRes, newestRes] = await Promise.all([ pool.query( - `SELECT id, payload, created_at FROM ${TABLE} WHERE status='pending' AND priority>=$1 AND run_after>$2 ORDER BY created_at ASC, id ASC LIMIT $3`, + `SELECT id, payload, created_at, deferred_by FROM ${TABLE} WHERE status='pending' AND priority>=$1 AND run_after>$2 AND deferred_by IS NOT NULL ORDER BY created_at ASC, id ASC LIMIT $3`, [FOREGROUND_QUEUE_PRIORITY_FLOOR, now, candidateLimit], ), pool.query( - `SELECT id, payload, created_at FROM ${TABLE} WHERE status='pending' AND priority>=$1 AND run_after>$2 ORDER BY created_at DESC, id DESC LIMIT $3`, + `SELECT id, payload, created_at, deferred_by FROM ${TABLE} WHERE status='pending' AND priority>=$1 AND run_after>$2 AND deferred_by IS NOT NULL ORDER BY created_at DESC, id DESC LIMIT $3`, [FOREGROUND_QUEUE_PRIORITY_FLOOR, now, candidateLimit], ), ]); - const candidateRowsById = new Map(); - for (const row of [...oldestRes.rows, ...newestRes.rows] as Array<{ id: string; payload: string; created_at: number | string }>) { + const candidateRowsById = new Map(); + for (const row of [...oldestRes.rows, ...newestRes.rows] as Array<{ id: string; payload: string; created_at: number | string; deferred_by: string | null }>) { candidateRowsById.set(row.id, row); } const eligible: Array<{ id: string; pendingSinceMs: number; ageStale: boolean; rateLimitClear: boolean }> = []; @@ -826,7 +842,13 @@ export function createPgQueue( for (const row of candidateRowsById.values()) { const pendingSinceMs = Number(row.created_at); const ageStale = isForegroundDeferralStale(foregroundLivenessConfig, pendingSinceMs, now); - const rateLimitClear = await isRateLimitAdmissionNowClear(row.payload, admissionCache); + // The rate-limit-clear recheck only makes sense -- and is only trusted -- for a row an admission gate + // deferred SPECIFICALLY for a rate-limit reason; a maintenance-admission or installation-concurrency + // deferral has no rate-limit bucket either, and isRateLimitAdmissionNowClear degrading to "clear" for it + // would release it on the very next sweep tick regardless of whether its OWN gate has actually cleared + // (the same false-premise bug #9127 fixes for enqueue-time delays, just for a different admission kind). + const rateLimitClear = + row.deferred_by === "rate_limit" ? await isRateLimitAdmissionNowClear(row.payload, admissionCache) : false; if (!ageStale && !rateLimitClear) continue; eligible.push({ id: row.id, pendingSinceMs, ageStale, rateLimitClear }); } @@ -835,8 +857,11 @@ export function createPgQueue( let releasedByAge = 0; let releasedByRateLimitClear = 0; for (const candidate of toRelease) { + // Clears deferred_by on release (not just run_after): the row is no longer deferred by anything, so a + // FUTURE re-defer (of whatever kind next applies) writes its own fresh tag via COALESCE(deferred_by, ...) + // instead of inheriting a stale tag from whichever admission gate deferred it this time. const update = await pool.query( - `UPDATE ${TABLE} SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1`, + `UPDATE ${TABLE} SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1`, [now, candidate.id], ); const rowsChanged = update.rowCount ?? 0; @@ -987,7 +1012,7 @@ export function createPgQueue( `UPDATE ${TABLE} SET payload=$1, run_after=GREATEST(run_after, $2), created_at=$3, priority=GREATEST(priority, $4), job_key=$5, claim_sort_key=CASE WHEN claim_sort_key>0 THEN LEAST(claim_sort_key, $8) ELSE $8 END, - last_error=NULL + last_error=NULL, deferred_by=NULL WHERE id=$6 AND status='pending' AND job_key=$7`, [mergedPayload, runAfter, now, priority, mergedKey, mergeCandidate.id, mergeCandidate.job_key, claimSortKey], ); @@ -1018,7 +1043,7 @@ export function createPgQueue( await pool.query( `UPDATE ${TABLE} SET payload=$1, run_after=GREATEST(run_after, $2), priority=GREATEST(priority, $3), job_key=$4, - foreground_lane=$5, claim_sort_key=CASE WHEN claim_sort_key>0 THEN LEAST(claim_sort_key, $7) ELSE $7 END, last_error=NULL + foreground_lane=$5, claim_sort_key=CASE WHEN claim_sort_key>0 THEN LEAST(claim_sort_key, $7) ELSE $7 END, last_error=NULL, deferred_by=NULL WHERE id=$6`, [payload, runAfter, priority, key, lane, existing.id, claimSortKey], ); @@ -1051,7 +1076,7 @@ export function createPgQueue( await pool.query( `UPDATE ${TABLE} SET payload=$1, run_after=GREATEST(run_after, $2), priority=GREATEST(priority, $3), - foreground_lane=$4, claim_sort_key=CASE WHEN claim_sort_key>0 THEN LEAST(claim_sort_key, $6) ELSE $6 END, last_error=NULL + foreground_lane=$4, claim_sort_key=CASE WHEN claim_sort_key>0 THEN LEAST(claim_sort_key, $6) ELSE $6 END, last_error=NULL, deferred_by=NULL WHERE id=$5`, [payload, runAfter, priority, lane, existing.id, claimSortKey], ); @@ -1268,7 +1293,7 @@ export function createPgQueue( const update = await retryPoolUpdateOrLeaveForReclaim( () => pool.query( - `UPDATE ${TABLE} SET status='pending', run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2) WHERE id=$3`, + `UPDATE ${TABLE} SET status='pending', run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2), deferred_by=COALESCE(deferred_by, 'rate_limit') WHERE id=$3`, [retryAfter, lastError, job.id], ), job.id, @@ -1311,7 +1336,7 @@ export function createPgQueue( const update = await retryPoolUpdateOrLeaveForReclaim( () => pool.query( - `UPDATE ${TABLE} SET status='pending', run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2) WHERE id=$3`, + `UPDATE ${TABLE} SET status='pending', run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2), deferred_by=COALESCE(deferred_by, 'maintenance_admission') WHERE id=$3`, [retryAfter, `maintenance admission deferred: ${decision.reason}`, job.id], ), job.id, @@ -1392,7 +1417,7 @@ export function createPgQueue( const update = await retryPoolUpdateOrLeaveForReclaim( () => pool.query( - `UPDATE ${TABLE} SET status='pending', run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2) WHERE id=$3`, + `UPDATE ${TABLE} SET status='pending', run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2), deferred_by=COALESCE(deferred_by, 'installation_concurrency') WHERE id=$3`, [retryAfter, `installation concurrency admission deferred: ${decision.reason}`, job.id], ), job.id, @@ -1503,7 +1528,7 @@ export function createPgQueue( await recordQueueMetric("loopover_jobs_coalesced_total"); } else { await pool.query( - `UPDATE ${TABLE} SET status='pending', run_after=$1, last_error=$2 WHERE id=$3`, + `UPDATE ${TABLE} SET status='pending', run_after=$1, last_error=$2, deferred_by='rate_limit' WHERE id=$3`, [retryAfter, errMsg, job.id], ); } @@ -1800,7 +1825,7 @@ export function createPgQueue( if (!matchesGitHubRateLimitAdmissionTarget(candidate, blocked)) continue; const runAfter = now + rateLimitRetryDelayWithJitter(delayMs, `${row.job_key ?? ""}:${row.id}:${row.payload}`); const update = await pool.query( - `UPDATE ${TABLE} SET run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2) WHERE id=$3 AND status='pending'`, + `UPDATE ${TABLE} SET run_after=GREATEST(run_after, $1), last_error=COALESCE(last_error, $2), deferred_by=COALESCE(deferred_by, 'rate_limit') WHERE id=$3 AND status='pending'`, [runAfter, "github rate-limit budget deferred", row.id], ); changed += update.rowCount ?? 0; @@ -1846,7 +1871,7 @@ export function createPgQueue( ).rows[0] as { id: string } | undefined; if (!existing) return false; await pool.query( - `UPDATE ${TABLE} SET run_after=GREATEST(run_after, $1), last_error=$2 WHERE id=$3`, + `UPDATE ${TABLE} SET run_after=GREATEST(run_after, $1), last_error=$2, deferred_by=COALESCE(deferred_by, 'rate_limit') WHERE id=$3`, [runAfter, errMsg, existing.id], ); await pool.query(`DELETE FROM ${TABLE} WHERE id=$1`, [job.id]); diff --git a/src/selfhost/sqlite-queue.ts b/src/selfhost/sqlite-queue.ts index 3c6b72f089..bc203e66b6 100644 --- a/src/selfhost/sqlite-queue.ts +++ b/src/selfhost/sqlite-queue.ts @@ -233,6 +233,14 @@ export function createSqliteQueue( } catch { /* column already present */ } + // #9127: provenance tag for releaseStaleForegroundDeferrals' rate-limit-clear recheck. Set ONLY at an admission + // gate's own defer site (rate-limit / maintenance-admission / installation-concurrency), NEVER by enqueue()'s + // delaySeconds -- see foreground-liveness.ts's module header for the full rationale. + try { + driver.exec(`ALTER TABLE ${TABLE} ADD COLUMN deferred_by TEXT`); + } catch { + /* column already present */ + } driver.exec(CLAIM_INDEX_DDL); driver.exec(JOB_KEY_INDEX_DDL); driver.exec(LANE_INDEX_DDL); @@ -389,20 +397,30 @@ export function createSqliteQueue( } /** See foreground-liveness.ts for the full rationale. A bounded candidate SELECT (foreground-priority, pending, - * not currently due), an eligibility pass, a ramp-up CAP, then a per-row conditional UPDATE only for the - * capped subset -- mirroring reviveEligibleDeadJobs' shape but with the extra ramp-up step. Each candidate is - * ELIGIBLE on EITHER of two independent conditions: it has genuinely been waiting past the age-based trickle - * ceiling (isForegroundDeferralStale, unconditional backstop), OR -- CONDITION-BASED recovery - * (#selfhost-queue-liveness VPS incident) -- re-evaluating rate-limit admission against CURRENT observations - * right now says it would be admitted immediately. The age floor alone can leave a job pinned to a stale - * reset timestamp for up to its full original delay (observed up to ~15m) even when a fresher, healthier - * observation arrived moments after it was deferred; the condition check recovers it on the NEXT sweep tick - * instead (bounded by FOREGROUND_LIVENESS_CHECK_INTERVAL_MS, default 60s) whenever the underlying rate-limit - * pressure has actually cleared, regardless of job age. When more jobs are eligible than maxReleasePerSweep - * allows, selectForegroundDeferralsToRelease picks the oldest first -- a large inherited backlog drains - * gradually over several sweep ticks instead of flooding GitHub with every re-attempt at once. Logs + - * records a metric ONCE per sweep (aggregate count), not per row, so a large release batch cannot spam the - * log. + * not currently due, ADMISSION-GATE-DEFERRED -- see below), an eligibility pass, a ramp-up CAP, then a per-row + * conditional UPDATE only for the capped subset -- mirroring reviveEligibleDeadJobs' shape but with the extra + * ramp-up step. Each candidate is ELIGIBLE on EITHER of two independent conditions: it has genuinely been + * waiting past the age-based trickle ceiling (isForegroundDeferralStale, unconditional backstop), OR -- + * CONDITION-BASED recovery (#selfhost-queue-liveness VPS incident), restricted to rows an admission gate itself + * deferred for a rate-limit reason (`deferred_by='rate_limit'`) -- re-evaluating rate-limit admission against + * CURRENT observations right now says it would be admitted immediately. The age floor alone can leave a job + * pinned to a stale reset timestamp for up to its full original delay (observed up to ~15m) even when a + * fresher, healthier observation arrived moments after it was deferred; the condition check recovers it on the + * NEXT sweep tick instead (bounded by FOREGROUND_LIVENESS_CHECK_INTERVAL_MS, default 60s) whenever the + * underlying rate-limit pressure has actually cleared, regardless of job age. When more jobs are eligible than + * maxReleasePerSweep allows, selectForegroundDeferralsToRelease picks the oldest first -- a large inherited + * backlog drains gradually over several sweep ticks instead of flooding GitHub with every re-attempt at once. + * Logs + records a metric ONCE per sweep (aggregate count), not per row, so a large release batch cannot spam + * the log. + * + * #9127 -- PROVENANCE filter (`deferred_by IS NOT NULL`, enforced by the candidate SELECT's own WHERE clause, + * not by an application-side check on the returned rows): only a row an admission gate itself deferred + * (rate-limit / maintenance-admission / installation-concurrency -- see the three defer sites' own + * `deferred_by=coalesce(deferred_by, ...)` writes) is EVER a release candidate. A row whose future run_after + * came from enqueue(message, delaySeconds) -- a deliberate delay, e.g. the linked-issue flag-then-close grace + * window -- never has deferred_by set, so it is structurally excluded here regardless of age or rate-limit + * state. See pg-queue.ts's twin (and foreground-liveness.ts's module header) for the full incident writeup -- + * this and pg-queue.ts's own copy must stay in lockstep (verified twin, see foreground-liveness.ts). * * Candidate selection queries an OLDEST window AND a NEWEST window (#selfhost-queue-liveness clear-bucket * starvation fix), not just one oldest-first window. A single `ORDER BY created_at ASC LIMIT` window can be @@ -418,22 +436,26 @@ export function createSqliteQueue( const now = Date.now(); const candidateLimit = foregroundLivenessConfig.maxReleasePerSweep; const oldest = driver.query( - `SELECT id, payload, created_at FROM ${TABLE} WHERE status='pending' AND priority>=? AND run_after>? ORDER BY created_at ASC, id ASC LIMIT ?`, + `SELECT id, payload, created_at, deferred_by FROM ${TABLE} WHERE status='pending' AND priority>=? AND run_after>? AND deferred_by IS NOT NULL ORDER BY created_at ASC, id ASC LIMIT ?`, [FOREGROUND_QUEUE_PRIORITY_FLOOR, now, candidateLimit], ).rows; const newest = driver.query( - `SELECT id, payload, created_at FROM ${TABLE} WHERE status='pending' AND priority>=? AND run_after>? ORDER BY created_at DESC, id DESC LIMIT ?`, + `SELECT id, payload, created_at, deferred_by FROM ${TABLE} WHERE status='pending' AND priority>=? AND run_after>? AND deferred_by IS NOT NULL ORDER BY created_at DESC, id DESC LIMIT ?`, [FOREGROUND_QUEUE_PRIORITY_FLOOR, now, candidateLimit], ).rows; - const candidateRowsById = new Map(); - for (const row of [...oldest, ...newest] as Array<{ id: number; payload: string; created_at: number }>) { + const candidateRowsById = new Map(); + for (const row of [...oldest, ...newest] as Array<{ id: number; payload: string; created_at: number; deferred_by: string | null }>) { candidateRowsById.set(row.id, row); } const eligible: Array<{ id: number; pendingSinceMs: number; ageStale: boolean; rateLimitClear: boolean }> = []; const admissionCache = new Map(); for (const row of candidateRowsById.values()) { const ageStale = isForegroundDeferralStale(foregroundLivenessConfig, row.created_at, now); - const rateLimitClear = isRateLimitAdmissionNowClear(row.payload, admissionCache); + // See pg-queue.ts's twin: the rate-limit-clear recheck is only trusted for a row deferred SPECIFICALLY for + // a rate-limit reason -- a maintenance-admission or installation-concurrency deferral has no rate-limit + // bucket either, and isRateLimitAdmissionNowClear degrading to "clear" for it would release it on the very + // next sweep tick regardless of whether its OWN gate has actually cleared. + const rateLimitClear = row.deferred_by === "rate_limit" && isRateLimitAdmissionNowClear(row.payload, admissionCache); if (!ageStale && !rateLimitClear) continue; eligible.push({ id: row.id, pendingSinceMs: row.created_at, ageStale, rateLimitClear }); } @@ -442,8 +464,11 @@ export function createSqliteQueue( let releasedByAge = 0; let releasedByRateLimitClear = 0; for (const candidate of toRelease) { + // Clears deferred_by on release too (not just run_after): the row is no longer deferred by anything, so a + // FUTURE re-defer (of whatever kind next applies) writes its own fresh tag via coalesce(deferred_by, ...) + // instead of inheriting a stale tag from whichever admission gate deferred it this time. const { changes } = driver.query( - `UPDATE ${TABLE} SET run_after=? WHERE id=? AND status='pending' AND run_after>?`, + `UPDATE ${TABLE} SET run_after=?, deferred_by=NULL WHERE id=? AND status='pending' AND run_after>?`, [now, candidate.id, now], ); released += changes; @@ -564,7 +589,7 @@ export function createSqliteQueue( `UPDATE ${TABLE} SET payload=?, run_after=max(run_after, ?), created_at=?, priority=max(priority, ?), job_key=?, claim_sort_key=CASE WHEN claim_sort_key>0 THEN min(claim_sort_key, ?) ELSE ? END, - last_error=NULL + last_error=NULL, deferred_by=NULL WHERE id=?`, [mergedPayload, runAfter, now, priority, mergedKey, claimSortKey, claimSortKey, mergeCandidate.id], ); @@ -593,7 +618,7 @@ export function createSqliteQueue( `UPDATE ${TABLE} SET payload=?, run_after=max(run_after, ?), priority=max(priority, ?), job_key=?, foreground_lane=?, claim_sort_key=CASE WHEN claim_sort_key>0 THEN min(claim_sort_key, ?) ELSE ? END, - last_error=NULL + last_error=NULL, deferred_by=NULL WHERE id=?`, [payload, runAfter, priority, key, lane, claimSortKey, claimSortKey, existing.id], ); @@ -625,7 +650,7 @@ export function createSqliteQueue( `UPDATE ${TABLE} SET payload=?, run_after=max(run_after, ?), priority=max(priority, ?), foreground_lane=?, claim_sort_key=CASE WHEN claim_sort_key>0 THEN min(claim_sort_key, ?) ELSE ? END, - last_error=NULL + last_error=NULL, deferred_by=NULL WHERE id=?`, [payload, runAfter, priority, lane, claimSortKey, claimSortKey, existing.id], ); @@ -932,7 +957,7 @@ export function createSqliteQueue( ); const lastError = `github rate-limit ${rateLimitAdmission.kind} admission`; const { changes } = driver.query( - `UPDATE ${TABLE} SET status='pending', run_after=max(run_after, ?), last_error=coalesce(last_error, ?) WHERE id=?`, + `UPDATE ${TABLE} SET status='pending', run_after=max(run_after, ?), last_error=coalesce(last_error, ?), deferred_by=coalesce(deferred_by, 'rate_limit') WHERE id=?`, [retryAfter, lastError, job.id], ); if (changes) { @@ -970,7 +995,7 @@ export function createSqliteQueue( `${job.job_key ?? ""}:${job.id}:${job.payload}`, ); const { changes } = driver.query( - `UPDATE ${TABLE} SET status='pending', run_after=max(run_after, ?), last_error=coalesce(last_error, ?) WHERE id=?`, + `UPDATE ${TABLE} SET status='pending', run_after=max(run_after, ?), last_error=coalesce(last_error, ?), deferred_by=coalesce(deferred_by, 'maintenance_admission') WHERE id=?`, [retryAfter, `maintenance admission deferred: ${decision.reason}`, job.id], ); if (changes) { @@ -1046,7 +1071,7 @@ export function createSqliteQueue( `${job.job_key ?? ""}:${job.id}:${job.payload}`, ); const { changes } = driver.query( - `UPDATE ${TABLE} SET status='pending', run_after=max(run_after, ?), last_error=coalesce(last_error, ?) WHERE id=?`, + `UPDATE ${TABLE} SET status='pending', run_after=max(run_after, ?), last_error=coalesce(last_error, ?), deferred_by=coalesce(deferred_by, 'installation_concurrency') WHERE id=?`, [retryAfter, `installation concurrency admission deferred: ${decision.reason}`, job.id], ); if (changes) { @@ -1117,7 +1142,7 @@ export function createSqliteQueue( recordQueueMetric(driver, "loopover_jobs_coalesced_total"); } else { driver.query( - `UPDATE ${TABLE} SET status='pending', run_after=?, last_error=? WHERE id=?`, + `UPDATE ${TABLE} SET status='pending', run_after=?, last_error=?, deferred_by='rate_limit' WHERE id=?`, [retryAfter, errMsg, job.id], ); } @@ -1573,7 +1598,7 @@ function deferPendingJobsForRateLimit( if (!matchesGitHubRateLimitAdmissionTarget(candidate, blocked)) continue; const runAfter = now + rateLimitRetryDelayWithJitter(delayMs, `${row.job_key ?? ""}:${row.id}:${row.payload}`); const { changes } = driver.query( - `UPDATE ${TABLE} SET run_after=max(run_after, ?), last_error=coalesce(last_error, ?) WHERE id=? AND status='pending'`, + `UPDATE ${TABLE} SET run_after=max(run_after, ?), last_error=coalesce(last_error, ?), deferred_by=coalesce(deferred_by, 'rate_limit') WHERE id=? AND status='pending'`, [runAfter, "github rate-limit budget deferred", row.id], ); changed += changes; @@ -1650,7 +1675,7 @@ function mergeRescheduledJobIntoPending( ).rows[0] as { id: number } | undefined; if (!existing) return false; driver.query( - `UPDATE ${TABLE} SET run_after=max(run_after, ?), last_error=? WHERE id=?`, + `UPDATE ${TABLE} SET run_after=max(run_after, ?), last_error=?, deferred_by=coalesce(deferred_by, 'rate_limit') WHERE id=?`, [runAfter, errMsg, existing.id], ); driver.query(`DELETE FROM ${TABLE} WHERE id=?`, [job.id]); diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 82e99d1331..ec210396f7 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -20,7 +20,7 @@ import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from " import { recordTerminalActionOutcome, resolveDispositionReason } from "../review/outcomes-wire"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../github/backfill"; -import { githubRateLimitAdmissionKeyForToken } from "../github/client"; +import { forcedSelfhostMode, githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestAssignee } from "../github/assignees"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions"; @@ -379,7 +379,7 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE const targetKey = `${ctx.repoFullName}#${ctx.pullNumber}`; // globalPaused folds the env-var brake AND the DB-backed kill-switch (#audit-§5.2) so an operator can halt the // fleet instantly via one DB row, without a redeploy. - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: ctx.agentPaused, agentDryRun: ctx.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: ctx.agentPaused, agentDryRun: ctx.agentDryRun }); for (const action of planned) { // #label-scoping: a `label` action may be authorized by a class OTHER than `label` itself (an anti-abuse @@ -971,6 +971,11 @@ async function recordCiCancelOutcome(env: Env, reasonKind: CiCancelReasonKind, c await auditCiCancelled(env, reasonKind, targetKey, ctx.repoFullName, headSha, outcome); return; } + // #9130: the instance-wide kill switch suppressed this call before any network request was even attempted -- + // never a failure worth a `_ci_cancel_failed` audit/error log, since nothing went wrong. Silent no-op, matching + // how the outer loop already skips a whole action pass under dry_run/paused without a special per-side-effect + // audit here. + if (outcome.kind === "suppressed") return; console.error( JSON.stringify({ level: "error", @@ -1013,7 +1018,7 @@ export type IssueActionExecutionContext = { export async function executeIssueMaintenanceActions(env: Env, ctx: IssueActionExecutionContext, planned: PlannedAgentAction[]): Promise { const outcomes: AgentActionOutcome[] = []; const targetKey = `${ctx.repoFullName}#${ctx.issueNumber}`; - const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), agentPaused: ctx.agentPaused, agentDryRun: ctx.agentDryRun }); + const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: ctx.agentPaused, agentDryRun: ctx.agentDryRun }); for (const action of planned) { // #label-scoping: a `label` action may be authorized by a class OTHER than `label` itself (an anti-abuse @@ -1166,7 +1171,14 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: // staging fails safe with a 409 (→ terminal hold) instead of merging un-reviewed code. A live sweep plans // expectedHeadSha == ctx.headSha, so its behavior is unchanged; the fallback covers any unpinned plan. const mergeSha = action.expectedHeadSha ?? ctx.headSha; - await mergePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, { mergeMethod: action.mergeMethod ?? "squash", ...(mergeSha ? { sha: mergeSha } : {}) }); + const mergeResult = await mergePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, { mergeMethod: action.mergeMethod ?? "squash", ...(mergeSha ? { sha: mergeSha } : {}) }); + // #9130: a suppressed merge (the instance-wide kill switch fired) is a SYNTHETIC shadow response, not a + // real GitHub mutation -- structurally unreachable via the normal call path (the loop above already + // gates on `mode` before ever reaching performAction under dry_run/paused), but defense in depth for any + // future caller that bypasses that gate. Never record it as ground truth: a real later outcome must still + // be able to write this row, which recordTerminalActionOutcome's own first-write-wins probe would + // otherwise permanently block. + if (mergeResult.suppressed) return "merge suppressed by the instance-wide dry-run/paused kill switch — no GitHub write attempted"; // #8823: record ground truth from the action we just completed rather than depending on the inbound // `pull_request.closed` webhook — a delivery this instance never processes used to lose the outcome // permanently, dropping the PR out of fleet calibration entirely. Idempotent against the webhook path. diff --git a/src/services/automation-state.ts b/src/services/automation-state.ts index a258c7bc8d..a88a45fa64 100644 --- a/src/services/automation-state.ts +++ b/src/services/automation-state.ts @@ -4,6 +4,7 @@ // `pendingActionCount` view that `GET /settings` deliberately does not return (settings returns only the // resolved row). Keeping this in one function is what stops the three surfaces from drifting. import { countPendingAgentActions, getInstallation, getRepository, isGlobalAgentFrozen } from "../db/repositories"; +import { forcedSelfhostMode } from "../github/client"; import { resolveRepositorySettings } from "../settings/repository-settings"; import { isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness } from "../settings/agent-execution"; import { AGENT_ACTION_CLASSES, isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy"; @@ -41,6 +42,7 @@ export async function buildAutomationState(env: Env, repoFullName: string): Prom const installation = repo?.installationId ? await getInstallation(env, repo.installationId) : null; const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, }); diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 5eb8577728..bef03d220e 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -242,14 +242,18 @@ function concreteCloseEvidenceCodes(input: AgentActionPlanInput): string[] { * concrete — the concrete signal alone already justifies the close regardless of what else is present. */ 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; + // #9129: a duplicate-PR link is DELIBERATELY excluded from concrete evidence, unlike a base conflict (#3863) or + // red CI. Those two are facts about THIS PR's own state, verifiable by loopover itself. A duplicate-issue-link + // is a fact about a SIBLING PR's author-controlled body text -- any contributor can manufacture it for free by + // citing the same issue number in a throwaway PR, with no code required, and thereby force ANY other PR + // claiming that issue to look "duplicated" from the breaker's point of view. The previous justification here + // reasoned only about the value going STALE between planning and actuation (closeRequiresDuplicateStillOpen's + // live recheck already covers that); it never addressed an ADVERSARY choosing the value in the first place -- + // exactly the gap that let this become an offensive close primitive against a rival contributor. Excluding it + // here lets downgradeCloseToHold's per-rule precision check apply to a duplicate-driven close like any other + // heuristic judgment, instead of exempting it from the one safety net meant to catch a systematically wrong + // call. (In practice evaluateGateCheckCore's own duplicate-only HOLD, #9129, already prevents this finding from + // producing a close outright -- this exclusion is defense-in-depth for any other path that still reaches here.) return concreteCloseEvidenceCodes(input).length > 0; } diff --git a/src/settings/agent-execution.ts b/src/settings/agent-execution.ts index 0a0d81ad0b..79444bfc03 100644 --- a/src/settings/agent-execution.ts +++ b/src/settings/agent-execution.ts @@ -34,10 +34,28 @@ export function isGlobalAgentPause(env: { AGENT_ACTIONS_PAUSED?: string | undefi * THE single gate the action layer (#778) consults before executing any action, alongside resolveAutonomy. * Precedence (safest wins): a global OR per-repo pause halts everything (`paused`); else a per-repo dry-run * logs what would happen without executing (`dry_run`); else `live`. Deny-toward-safety. Pure. - */ -export function resolveAgentActionMode(input: { globalPaused: boolean; agentPaused?: boolean | null | undefined; agentDryRun?: boolean | null | undefined }): AgentActionMode { - if (input.globalPaused || input.agentPaused === true) return "paused"; - if (input.agentDryRun === true) return "dry_run"; + * + * `instanceMode` (#9130) is the INSTANCE-WIDE self-host kill switch (`SELFHOST_DEPLOYMENT_MODE`, resolved by the + * caller via `forcedSelfhostMode(env)` in src/github/client.ts) — the first precedence term, not an HTTP-layer + * afterthought. Before #9130 this switch was consulted ONLY inside `makeInstallationOctokit`, one layer below + * every decision that depends on the mode: the executor computed its OWN mode from `globalPaused`/`agentPaused`/ + * `agentDryRun` alone, believed it was "live" under a dry-run instance, and drove real non-GitHub-API side effects + * (a persisted `pr_outcome` row, Discord/Slack notifications, `maybeEscalateModeration`'s `mode !== "live"` check) + * off that false belief — even though the actual GitHub write was correctly suppressed by the octokit hook. + * Every caller of this function that has `env` in scope now threads `forcedSelfhostMode(env)` through here, so a + * "dry-run"/"disabled" instance is `dry_run`/`paused` EVERYWHERE a mode decision is made, not just at the wire. + * Folded in via the SAME "most restrictive wins" precedence as the other two terms (instanceMode: "paused" beats + * a per-repo dry-run; instanceMode: "dry_run" beats a live per-repo config) — never the reverse, so a per-repo + * override can never re-enable a mutation the instance-wide switch means to suppress. `undefined`/`null`/"live" + * (the cloud Worker never sets SELFHOST_DEPLOYMENT_MODE) behaves exactly as before this parameter existed. */ +export function resolveAgentActionMode(input: { + globalPaused: boolean; + agentPaused?: boolean | null | undefined; + agentDryRun?: boolean | null | undefined; + instanceMode?: AgentActionMode | null | undefined; +}): AgentActionMode { + if (input.globalPaused || input.agentPaused === true || input.instanceMode === "paused") return "paused"; + if (input.agentDryRun === true || input.instanceMode === "dry_run") return "dry_run"; return "live"; } diff --git a/test/fixtures/engine-parity/predicted-gate/duplicate-pr-block.ts b/test/fixtures/engine-parity/predicted-gate/duplicate-pr-block.ts index 6e9e8a1238..7ec3c59d02 100644 --- a/test/fixtures/engine-parity/predicted-gate/duplicate-pr-block.ts +++ b/test/fixtures/engine-parity/predicted-gate/duplicate-pr-block.ts @@ -1,19 +1,24 @@ import { BASE_INPUT, BASE_REPO, definePredictedGateFixture, openIssue, openPr, parseManifest } from "./_shared"; -// Duplicate cluster branch: another OPEN PR already claims the same linked issue. +// Duplicate cluster branch: another OPEN PR cites the same linked issue in its (author-controlled) body text, +// with no corroborating changed-file evidence resolved for either side (openPr never sets changedFiles). #9129: +// an UNCORROBORATED overlap is never a configured gate blocker regardless of duplicatePrGateMode -- an +// adversary could otherwise cite the same issue number in a throwaway sibling PR, no code required, and force +// this exact scenario to block/close the real PR. Only the separate, always-non-blocking +// duplicate_pr_risk_unconfirmed finding fires here; the gate passes. export default definePredictedGateFixture({ id: "duplicate-pr-block", - title: "Duplicate open PR blocks the predicted gate", - branch: "duplicate_pr_risk via another open sibling sharing the linked issue", + title: "An uncorroborated duplicate-issue citation never blocks the predicted gate (#9129)", + branch: "duplicate_pr_risk_unconfirmed via another open sibling citing the same linked issue, no diff corroboration", input: BASE_INPUT, manifest: parseManifest({ gate: { duplicates: "block" } }), repo: BASE_REPO, issues: [openIssue(7, "Uploads should retry on 5xx")], pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7])], expected: { - conclusion: "failure", + conclusion: "success", pack: "gittensor", - blockerCodes: ["duplicate_pr_risk"], + blockerCodes: [], warningCodes: [], funnelPresent: false, }, diff --git a/test/fixtures/engine-parity/predicted-gate/golden/duplicate-pr-block.json b/test/fixtures/engine-parity/predicted-gate/golden/duplicate-pr-block.json index 31192aeffc..cf84e2a185 100644 --- a/test/fixtures/engine-parity/predicted-gate/golden/duplicate-pr-block.json +++ b/test/fixtures/engine-parity/predicted-gate/golden/duplicate-pr-block.json @@ -2,18 +2,11 @@ "predicted": true, "basis": "public_config", "pack": "gittensor", - "conclusion": "failure", - "title": "LoopOver Orb Review Agent: Linked issue overlaps another open PR", - "summary": "Linked issue overlaps another open PR — Review the related PRs before spending reviewer time on duplicate work.", + "conclusion": "success", + "title": "LoopOver Orb Review Agent passed", + "summary": "No configured hard blocker was found. Advisory findings, if any, stay advisory.", "readinessScore": 69, - "blockers": [ - { - "code": "duplicate_pr_risk", - "title": "Linked issue overlaps another open PR", - "detail": "Other open pull requests reference the same linked issue set: #42.", - "action": "Review the related PRs before spending reviewer time on duplicate work." - } - ], + "blockers": [], "warnings": [], "funnel": null, "note": "Predicted from the repo's public .loopover.yml gate config + safe defaults. The maintainer may have private dashboard overrides not reflected here, and the dual-model AI-consensus blocker is only evaluated on a real PR. The slop score is NOT evaluated pre-submission (it needs the diff content) and may still fail the real gate. Provide the PR's changed paths to also predict the focus-manifest path policy, the size/guardrail hold, and any pre-merge check scoped to changed paths; without them only path-independent title/description/label pre-merge checks are predicted. Every author is gated the same: a configured hard blocker fails the gate regardless of confirmed-contributor status (which affects only on-chain scoring)." diff --git a/test/golden-corpus/gate-corpus.json b/test/golden-corpus/gate-corpus.json index 38ba172007..353d159d03 100644 --- a/test/golden-corpus/gate-corpus.json +++ b/test/golden-corpus/gate-corpus.json @@ -1,6 +1,6 @@ { - "$comment": "Golden gate corpus (#8832, epic #8828). Each entry replays the PURE gate evaluation (evaluateGateCheck) against a decision-input snapshot drawn from a REAL production failure archetype, pinning the expected conclusion and blocker set. knownBad entries additionally assert the invariant that they may NEVER evaluate to success \u2014 the exact regression class that motivated the epic. Version bumps when entry semantics change; additions are append-only.", - "version": 1, + "$comment": "Golden gate corpus (#8832, epic #8828). Each entry replays the PURE gate evaluation (evaluateGateCheck) against a decision-input snapshot drawn from a REAL production failure archetype, pinning the expected conclusion and blocker set. knownBad entries additionally assert the invariant that they may NEVER evaluate to success \u2014 the exact regression class that motivated the epic. Version bumps when entry semantics change; additions are append-only. v2 (#9129): duplicate-block-mode-fails renamed duplicate-block-mode-holds and its pinned verdict changed from failure to neutral \u2014 a duplicate_pr_risk finding under duplicatePrGateMode: block now HOLDS the gate instead of closing (the close-authority fix for the offensive-close attack).", + "version": 2, "entries": [ { "id": "clean-pr-succeeds", @@ -150,9 +150,9 @@ } }, { - "id": "duplicate-block-mode-fails", + "id": "duplicate-block-mode-holds", "source": "reversal-class", - "description": "Duplicate-PR risk under block mode fails the gate.", + "description": "#9129: a duplicate_pr_risk finding under block mode now HOLDS the gate (never closes) -- the close-authority fix for the offensive-close attack (cite the same issue number in a rival's throwaway PR body to force a close). duplicatePrGateMode: block still has real effect (a hold for a human), but no configuration lets this finding close a PR outright anymore.", "knownBad": false, "findings": [ { @@ -166,10 +166,8 @@ "duplicatePrGateMode": "block" }, "expected": { - "conclusion": "failure", - "blockerCodes": [ - "duplicate_pr_risk" - ] + "conclusion": "neutral", + "blockerCodes": [] } }, { diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 58f82dcc69..9b97f2900e 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -3,7 +3,7 @@ import * as notifyDiscordModule from "../../src/services/notify-discord"; vi.mock("../../src/github/pr-actions", () => ({ createPullRequestReview: vi.fn(async () => ({ id: 1 })), - mergePullRequest: vi.fn(async () => ({ merged: true, sha: "merged-sha" })), + mergePullRequest: vi.fn(async () => ({ merged: true, sha: "merged-sha", suppressed: false })), closePullRequest: vi.fn(async () => ({ state: "closed" })), closeIssue: vi.fn(async () => ({ state: "closed" })), createIssueComment: vi.fn(async () => ({ id: 2 })), @@ -212,6 +212,81 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect((await auditFor(env, "merge"))?.outcome).toBe("completed"); }); + // #9130: a suppressed merge (the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch, or any future caller + // that reaches performAction without mergePullRequest's own suppression having already been screened out + // upstream) must never be recorded as ground truth -- the whole point of the mergePullRequest.suppressed + // field. Defense-in-depth: production never reaches this via the normal call path (the loop's own mode gate + // already skips performAction under dry_run/paused), but the executor must still handle a suppressed result + // correctly if it's ever reached. + it("#9130: does NOT record a pr_outcome row when mergePullRequest reports suppressed: true", async () => { + const env = createTestEnv({}); + vi.mocked(mergePullRequest).mockResolvedValueOnce({ merged: true, sha: null, suppressed: true }); + + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); + + expect(outcomes.map((o) => o.outcome)).toEqual(["completed"]); + const outcomeRow = await env.DB.prepare("SELECT 1 AS x FROM review_audit WHERE target_id = ? AND event_type = 'pr_outcome' LIMIT 1") + .bind("owner/repo#7") + .first<{ x: number }>(); + // The D1 test double's .first() resolves undefined (not null) for no match -- assert loosely so this test + // isn't coupled to that fake's exact nullish shape. + expect(outcomeRow ?? null).toBeNull(); + }); + + // #9130 INVARIANT: the instance-wide SELFHOST_DEPLOYMENT_MODE=dry-run kill switch, with every PER-REPO signal + // still saying "live" (agentPaused: false, agentDryRun: false) -- the exact split-brain scenario the issue + // describes, where the executor used to believe it was live while the instance switch meant to suppress + // everything. Runs a merge disposition AND a moderation-escalating close (closeKind: "blacklist", with the + // global ban threshold set to 1 -- i.e. this SINGLE close would normally ban+blacklist the author outright) + // in one pass, and asserts every one of the four side effects #9130 names is completely absent: + // 1. zero GitHub writes (mergePullRequest / closePullRequest / ensurePullRequestLabel never called) + // 2. zero pr_outcome rows + // 3. zero Discord/Slack notifications + // 4. zero moderation escalations (no blacklist entry, despite banThreshold: 1) + it("#9130 INVARIANT: SELFHOST_DEPLOYMENT_MODE=dry-run produces ZERO GitHub writes, zero outcome rows, zero notifications, and zero moderation escalations, even though every per-repo setting says live", async () => { + const env = createTestEnv({ SELFHOST_DEPLOYMENT_MODE: "dry-run" }); + await upsertGlobalModerationConfig(env, { enabled: true, banThreshold: 1 }); + const notifySpy = vi.spyOn(notifyDiscordModule, "notifyActionToDiscord").mockResolvedValue(undefined); + const blacklistClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "banned", closeComment: "closing", closeKind: "blacklist" }; + const blacklistLabel: PlannedAgentAction = { + actionClass: "label", + autonomyClass: "close", + requiresApproval: false, + reason: "banned", + label: "banned-contributor", + labelOp: "add", + closeKind: "blacklist", + }; + + const outcomes = await executeAgentMaintenanceActions( + env, + ctx({ authorLogin: "attacker", agentPaused: false, agentDryRun: false }), + [merge, blacklistClose, blacklistLabel], + ); + + // Every action is recorded as a dry-run shadow, never "completed" (which would mean a real mutation ran). + expect(outcomes.map((o) => o.outcome)).toEqual(["dry_run", "dry_run", "dry_run"]); + + // 1) Zero GitHub writes. + expect(mergePullRequest).not.toHaveBeenCalled(); + expect(closePullRequest).not.toHaveBeenCalled(); + expect(ensurePullRequestLabel).not.toHaveBeenCalled(); + expect(createOrUpdateCloseExplanationComment).not.toHaveBeenCalled(); + + // 2) Zero pr_outcome rows. + const outcomeRow = await env.DB.prepare("SELECT 1 AS x FROM review_audit WHERE target_id = ? AND event_type = 'pr_outcome' LIMIT 1") + .bind("owner/repo#7") + .first<{ x: number }>(); + expect(outcomeRow ?? null).toBeNull(); + + // 3) Zero notifications. + expect(notifySpy).not.toHaveBeenCalled(); + + // 4) Zero moderation escalations -- despite banThreshold: 1 (this single close would normally ban+blacklist + // "attacker" outright), the global blacklist stays empty. + expect(await getGlobalContributorBlacklist(env)).toEqual([]); + }); + // #terminal-outcome-audit: a heuristic close's reason is built by joining every blocker title // (planAgentMaintenanceActions), so a PR with many/verbose blockers could otherwise write an unbounded string // into audit_events.detail. Bounded the same way the pre-existing merge_blocked/mergeBlockedReason paths diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 70c125ec4f..3b9fe75b3d 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1840,13 +1840,14 @@ 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 — and IS duplicate-still-open-justified (#dup-winner-staleness)", () => { + it("#9129: a deterministic linked-issue-overlap duplicate (linkedDuplicateCount > 0) is NOT concrete evidence — the breaker must be able to catch it — but 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. - // 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 }); + // #9129: a duplicate-PR link is DELIBERATELY excluded from concrete evidence (see hasConcreteCloseEvidence's + // own doc comment) — it is a fact about a SIBLING PR's author-controlled body text, not something loopover + // itself can verify the way CI/conflict state can. closeRequiresDuplicateStillOpen is STILL 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 still fire for it, independent of the concrete-evidence classification. + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false, closeRequiresMergeableState: false, closeRequiresDuplicateStillOpen: true }); }); it("linkedDuplicateCount absent (nullish ?? 0) does NOT count as concrete on its own, and closeRequiresDuplicateStillOpen is explicitly false (#dup-winner-staleness)", () => { @@ -1854,6 +1855,18 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false, closeRequiresDuplicateStillOpen: false }); }); + // #9129: the whole point of dropping duplicate-link evidence from hasConcreteCloseEvidence -- the + // close-precision breaker can now actually downgrade a duplicate-driven close to a hold, exactly like any + // other non-concrete heuristic close. Before this fix, this close SURVIVED the breaker unconditionally. + it("#9129: the close-precision breaker can now DOWNGRADE a duplicate-link-only close to a hold", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1 } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + const held = downgradeCloseToHold(plan, true); + expect(held.some((a) => a.actionClass === "close")).toBe(false); + const label = held.find((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "add"); + expect(label?.autonomyClass).toBe("close"); + }); + 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 }); @@ -1955,9 +1968,9 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: [] }); }); - it("closeConcreteEvidenceCodes is EMPTY for a duplicate-link-justified concrete evidence", () => { + it("#9129: closeConcreteEvidenceCodes is EMPTY for a duplicate-link close, which is no longer concrete evidence at all", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1 } })); - expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: [] }); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: false, closeConcreteEvidenceCodes: [] }); }); it("closeConcreteEvidenceCodes excludes an AI-judgment code even when a real concrete code is also present", () => { diff --git a/test/unit/agent-execution.test.ts b/test/unit/agent-execution.test.ts index 159831a012..d9989e73f0 100644 --- a/test/unit/agent-execution.test.ts +++ b/test/unit/agent-execution.test.ts @@ -37,6 +37,41 @@ describe("resolveAgentActionMode (#776 safety gate)", () => { expect(agentActionModeExecutes("dry_run")).toBe(false); expect(agentActionModeExecutes("paused")).toBe(false); }); + + // #9130: instanceMode is the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch (forcedSelfhostMode(env), + // threaded in by every caller with env in scope) -- the FIRST precedence term, folded in via the same + // "most restrictive wins" ordering as the other two terms. + describe("instanceMode (#9130 instance-wide kill switch)", () => { + it("instanceMode: 'paused' forces paused even when every per-repo/global signal says live", () => { + expect(resolveAgentActionMode({ globalPaused: false, agentPaused: false, agentDryRun: false, instanceMode: "paused" })).toBe("paused"); + }); + + it("instanceMode: 'paused' beats a per-repo dry-run too (most restrictive wins)", () => { + expect(resolveAgentActionMode({ globalPaused: false, agentDryRun: true, instanceMode: "paused" })).toBe("paused"); + }); + + it("instanceMode: 'dry_run' forces dry_run even when every per-repo/global signal says live", () => { + expect(resolveAgentActionMode({ globalPaused: false, agentPaused: false, agentDryRun: false, instanceMode: "dry_run" })).toBe("dry_run"); + }); + + it("a global or per-repo pause still beats instanceMode: 'dry_run' (paused is the most restrictive term overall)", () => { + expect(resolveAgentActionMode({ globalPaused: true, instanceMode: "dry_run" })).toBe("paused"); + expect(resolveAgentActionMode({ globalPaused: false, agentPaused: true, instanceMode: "dry_run" })).toBe("paused"); + }); + + it("instanceMode: 'live' never forces anything -- behaves exactly like instanceMode absent", () => { + expect(resolveAgentActionMode({ globalPaused: false, instanceMode: "live" })).toBe("live"); + expect(resolveAgentActionMode({ globalPaused: false, agentDryRun: true, instanceMode: "live" })).toBe("dry_run"); + expect(resolveAgentActionMode({ globalPaused: true, instanceMode: "live" })).toBe("paused"); + }); + + it("instanceMode absent/null/undefined is byte-identical to today (the cloud Worker never sets SELFHOST_DEPLOYMENT_MODE)", () => { + expect(resolveAgentActionMode({ globalPaused: false })).toBe("live"); + expect(resolveAgentActionMode({ globalPaused: false, instanceMode: null })).toBe("live"); + expect(resolveAgentActionMode({ globalPaused: false, instanceMode: undefined })).toBe("live"); + expect(resolveAgentActionMode({ globalPaused: false, agentDryRun: true, instanceMode: null })).toBe("dry_run"); + }); + }); }); describe("isGlobalAgentPause", () => { diff --git a/test/unit/gate-golden-corpus.test.ts b/test/unit/gate-golden-corpus.test.ts index 85daee6fd3..74131cbead 100644 --- a/test/unit/gate-golden-corpus.test.ts +++ b/test/unit/gate-golden-corpus.test.ts @@ -39,7 +39,7 @@ function advisoryOf(entry: CorpusEntry): Advisory { describe("golden gate corpus (#8832)", () => { it("corpus file is well-formed: version, unique ids, non-empty archetype coverage", () => { - expect(corpus.version).toBe(1); + expect(corpus.version).toBe(2); expect(corpus.entries.length).toBeGreaterThanOrEqual(14); expect(new Set(corpus.entries.map((entry) => entry.id)).size).toBe(corpus.entries.length); // The corpus must always carry at least one knownBad guard — the never-flips-to-merge invariant is its point. diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 279954f210..1bbf2e8f3b 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -822,6 +822,49 @@ describe("GitHub check runs", () => { expect(cancelledIds.sort()).toEqual([1, 2, 3]); }); + // #9130: this is the last installation-scoped write that ran on raw timeoutFetch, entirely outside + // makeInstallationOctokit's own suppression hook -- a suppressed instance must never even ATTEMPT the + // network call (createInstallationToken/list/cancel), not merely fail to act on its result. + it("#9130: cancelInFlightWorkflowRunsForHeadSha is suppressed under SELFHOST_DEPLOYMENT_MODE=dry-run, with ZERO network calls attempted", async () => { + const privateKey = await generatePrivateKeyPem(); + let fetchCallCount = 0; + vi.stubGlobal("fetch", async () => { + fetchCallCount += 1; + return new Response("not found", { status: 404 }); + }); + + const outcome = await cancelInFlightWorkflowRunsForHeadSha( + createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey, SELFHOST_DEPLOYMENT_MODE: "dry-run" }), + 123, + "owner/repo", + "abc123", + 55, + ); + + expect(outcome).toEqual({ kind: "suppressed" }); + expect(fetchCallCount).toBe(0); + }); + + it("#9130: cancelInFlightWorkflowRunsForHeadSha is suppressed under SELFHOST_DEPLOYMENT_MODE=disabled too", async () => { + const privateKey = await generatePrivateKeyPem(); + let fetchCallCount = 0; + vi.stubGlobal("fetch", async () => { + fetchCallCount += 1; + return new Response("not found", { status: 404 }); + }); + + const outcome = await cancelInFlightWorkflowRunsForHeadSha( + createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey, SELFHOST_DEPLOYMENT_MODE: "disabled" }), + 123, + "owner/repo", + "abc123", + 55, + ); + + expect(outcome).toEqual({ kind: "suppressed" }); + expect(fetchCallCount).toBe(0); + }); + it("cancelInFlightWorkflowRunsForHeadSha only cancels workflow runs attached to the closed PR", async () => { const privateKey = await generatePrivateKeyPem(); const cancelledIds: number[] = []; diff --git a/test/unit/github-client.test.ts b/test/unit/github-client.test.ts index 4093477a7c..f22b374043 100644 --- a/test/unit/github-client.test.ts +++ b/test/unit/github-client.test.ts @@ -158,6 +158,20 @@ describe("resolveRepoActionMode", () => { await setGlobalAgentFrozen(env, false); expect(await resolveRepoActionMode(env, { agentPaused: false, agentDryRun: false })).toBe("live"); }); + + // #9130: SELFHOST_DEPLOYMENT_MODE is now folded in as the FIRST precedence term, not an HTTP-layer + // afterthought -- resolveRepoActionMode is the SAME chokepoint executeAgentMaintenanceActions/ + // executeIssueMaintenanceActions and every other resolveAgentActionMode call site now consults. + it("#9130: SELFHOST_DEPLOYMENT_MODE forces dry_run/paused even when every per-repo/global signal says live", async () => { + const env = createTestEnv(); + expect(await resolveRepoActionMode({ ...env, SELFHOST_DEPLOYMENT_MODE: "dry-run" }, { agentPaused: false, agentDryRun: false })).toBe("dry_run"); + expect(await resolveRepoActionMode({ ...env, SELFHOST_DEPLOYMENT_MODE: "disabled" }, { agentPaused: false, agentDryRun: false })).toBe("paused"); + // A per-repo/global signal that is ALREADY more restrictive than the instance switch still wins. + expect(await resolveRepoActionMode({ ...env, SELFHOST_DEPLOYMENT_MODE: "dry-run", AGENT_ACTIONS_PAUSED: "true" }, { agentPaused: false, agentDryRun: false })).toBe("paused"); + // Unset / "live" behaves exactly as before this switch existed. + expect(await resolveRepoActionMode({ ...env, SELFHOST_DEPLOYMENT_MODE: "live" }, { agentPaused: false, agentDryRun: false })).toBe("live"); + expect(await resolveRepoActionMode(env, { agentPaused: false, agentDryRun: false })).toBe("live"); + }); }); describe("githubRateLimitAdmissionKeyForToken — the single token→admission-key resolver (no duplication, no drift)", () => { diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 45ac9bacba..0dd0ced4a3 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -132,7 +132,7 @@ describe("GitHub PR action primitives (#778)", () => { return new Response("unexpected", { status: 500 }); }); const result = await mergePullRequest(envWithKey(), 123, "owner/repo", 7, { mergeMethod: "squash", sha: "head1" }); - expect(result).toEqual({ merged: true, sha: "abc" }); + expect(result).toEqual({ merged: true, sha: "abc", suppressed: false }); expect(calls[0]).toMatchObject({ method: "PUT", body: { merge_method: "squash", sha: "head1" } }); }); @@ -147,7 +147,29 @@ describe("GitHub PR action primitives (#778)", () => { const result = await mergePullRequest(envWithKey(), 123, "owner/repo", 7, { mergeMethod: "merge" }); expect(sent).toMatchObject({ merge_method: "merge" }); expect(sent).not.toHaveProperty("sha"); - expect(result).toEqual({ merged: true, sha: null }); + expect(result).toEqual({ merged: true, sha: null, suppressed: false }); + }); + + // #9130: a merge issued while the instance-wide kill switch is set never reaches GitHub -- the octokit hook + // returns the synthetic { merged: true, sha: null, dryRunSuppressed: true } shadow, which mergePullRequest + // must surface as suppressed: true rather than letting it look identical to a real merge. + it("#9130: SELFHOST_DEPLOYMENT_MODE=dry-run suppresses the merge write and marks the result suppressed, with ZERO merge-endpoint calls", async () => { + let mergeCallCount = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/pulls/7/merge")) { + mergeCallCount += 1; + return Response.json({ merged: true, sha: "abc" }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = { ...envWithKey(), SELFHOST_DEPLOYMENT_MODE: "dry-run" } as ReturnType; + const result = await mergePullRequest(env, 123, "owner/repo", 7, { mergeMethod: "squash" }); + expect(result).toEqual({ merged: true, sha: null, suppressed: true }); + // The suppression fires inside the octokit request hook, BEFORE the actual merge write ever reaches + // GitHub -- the installation-token mint (a GET) still runs, but the real PUT /merge never does. + expect(mergeCallCount).toBe(0); }); it("closes a PR via PATCH state=closed", async () => { diff --git a/test/unit/maintainer-activation.test.ts b/test/unit/maintainer-activation.test.ts index d334b0ad93..af6cda62a5 100644 --- a/test/unit/maintainer-activation.test.ts +++ b/test/unit/maintainer-activation.test.ts @@ -200,8 +200,9 @@ describe("buildMaintainerActivationPreview", () => { }); const winner = preview.samples.find((sample) => sample.number === 1)!; const loser = preview.samples.find((sample) => sample.number === 2)!; - expect(winner.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk"); - expect(loser.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); + // Neither PR has changedFiles resolved (#9129: the uncorroborated code fires, not the concrete one). + expect(winner.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk_unconfirmed"); + expect(loser.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk_unconfirmed"); }); it("flags every duplicate-cluster member when LOOPOVER_DUPLICATE_WINNER is off (default)", () => { @@ -215,7 +216,8 @@ describe("buildMaintainerActivationPreview", () => { ], generatedAt: "2026-06-14T00:00:00.000Z", }); - expect(preview.findingCodeCounts).toContainEqual({ code: "duplicate_pr_risk", count: 2 }); + // Neither PR has changedFiles resolved (#9129: uncorroborated code). + expect(preview.findingCodeCounts).toContainEqual({ code: "duplicate_pr_risk_unconfirmed", count: 2 }); }); it("ignores closed/merged siblings when detecting duplicate overlap", () => { diff --git a/test/unit/predicted-gate-engine.test.ts b/test/unit/predicted-gate-engine.test.ts index 0b70e31423..a314436631 100644 --- a/test/unit/predicted-gate-engine.test.ts +++ b/test/unit/predicted-gate-engine.test.ts @@ -221,7 +221,10 @@ describe("predicted-gate engine module coverage (#2283)", () => { }, { duplicatePrGateMode: "block" }, ); - expect(evaluation.conclusion).toBe("failure"); + // #9129 (host-parity): a duplicate_pr_risk finding under block mode now HOLDS (neutral), never closes -- + // see gate-advisory.ts's own duplicate-only-hold branch. The sanitizer still runs on the hold's + // title/summary (built from the same blocker findings), so the redaction assertions are unaffected. + expect(evaluation.conclusion).toBe("neutral"); expect(`${evaluation.title} ${evaluation.summary}`).not.toMatch(/likely_duplicate|reviewability/); expect(evaluation.summary).toContain("[context]"); }); @@ -306,7 +309,8 @@ describe("predicted-gate engine module coverage (#2283)", () => { expect(missingRepo.findings.some((f) => f.code === "repo_not_registered")).toBe(true); const missingPr = buildPullRequestAdvisory(REPO, null); expect(missingPr.findings.some((f) => f.code === "pr_not_cached")).toBe(true); - const blocked = evaluateGateCheck( + // #9129 (host-parity): a duplicate_pr_risk finding under block mode now HOLDS (neutral), never closes. + const held = evaluateGateCheck( { id: "a", targetType: "pull_request", @@ -321,7 +325,7 @@ describe("predicted-gate engine module coverage (#2283)", () => { }, { duplicatePrGateMode: "block" }, ); - expect(blocked.conclusion).toBe("failure"); + expect(held.conclusion).toBe("neutral"); }); it("exercises the inactive lane advice branch", () => { diff --git a/test/unit/predicted-gate.test.ts b/test/unit/predicted-gate.test.ts index 7ef7bd8de4..21ae75968c 100644 --- a/test/unit/predicted-gate.test.ts +++ b/test/unit/predicted-gate.test.ts @@ -67,15 +67,35 @@ describe("buildPredictedGateVerdict", () => { expect(result.blockers).toHaveLength(0); }); - it("predicts a BLOCK when a duplicate PR exists and duplicates:block (the default)", () => { - // Another open PR already targets the same linked issue → duplicate_pr_risk. + // #9129: an UNCORROBORATED duplicate citation (no changedFiles resolved on either side, the common case for + // a pre-submission prediction) is never a configured gate blocker, regardless of duplicates:block -- an + // adversary could otherwise cite the same issue number in a throwaway sibling PR, no code required, to + // force-close the real one. Renamed from "...duplicates:block (the default)": duplicatePrGateMode's + // code-level default is now "advisory", so "the default" no longer describes "block" at all. + it("does NOT block on an uncorroborated duplicate PR even with duplicates:block explicitly set (#9129)", () => { + // Another open PR already cites the same linked issue in its body text, but neither side has changedFiles + // resolved (pre-submission prediction never has diff content) -- uncorroborated. const result = verdict({ gate: { duplicates: "block" }, pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7])] }); - expect(result.conclusion).toBe("failure"); - expect(result.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(true); - // Public-safe: blocker text carries a fix and no raw internal markers. + expect(result.conclusion).toBe("success"); + expect(result.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(false); + // Public-safe: text carries no raw internal markers regardless. expect(result.title.toLowerCase()).toContain("loopover orb review agent"); }); + // #9129: once corroborated -- here via the sibling alone being a non-trivial real change (it has resolved + // changed files), the ALTERNATE corroboration arm since predicted-gate's own synthetic PR never carries + // changedFiles pre-submission -- duplicatePrGateMode: "block" now HOLDS the gate (neutral) rather than + // closing it outright ("prefer holding both over closing either"). + it("HOLDS (never closes) a CORROBORATED duplicate PR even with duplicates:block explicitly set (#9129)", () => { + const corroborated = verdict({ + gate: { duplicates: "block" }, + pullRequests: [{ ...openPr(42, "Retry uploads on 5xx responses", [7]), changedFiles: ["src/upload-client.ts"] }], + }); + expect(corroborated.conclusion).toBe("neutral"); + expect(corroborated.blockers).toHaveLength(0); + expect(corroborated.warnings.some((w) => w.code === "duplicate_pr_risk")).toBe(true); + }); + it("does NOT raise duplicate_pr_risk for an open PR in a different repo sharing the same issue number (repo-scoped parity)", () => { const result = verdict({ gate: { duplicates: "block" }, @@ -146,12 +166,15 @@ describe("buildPredictedGateVerdict", () => { }); it("honors public gate.mergeReadiness when predicting blockers", () => { + // #9129: corroborated (the sibling carries resolved changedFiles) so duplicate_pr_risk is the concrete + // code the mergeReadiness composite can escalate at all -- an uncorroborated citation is hard-wired to + // "off" regardless of any gate mode, mergeReadiness included. Corroborated + block now HOLDS (neutral). const result = verdict({ gate: { duplicates: "off", mergeReadiness: "block" }, - pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7])], + pullRequests: [{ ...openPr(42, "Retry uploads on 5xx responses", [7]), changedFiles: ["src/upload-client.ts"] }], }); - expect(result.conclusion).toBe("failure"); - expect(result.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(true); + expect(result.conclusion).toBe("neutral"); + expect(result.warnings.some((w) => w.code === "duplicate_pr_risk")).toBe(true); }); it("surfaces the missing-linked-issue blocker under composite mergeReadiness even when linkedIssue is unset (#merge-readiness-parity)", () => { @@ -172,19 +195,22 @@ describe("buildPredictedGateVerdict", () => { it("matches author history case-insensitively, like the live gate (#audit-§4)", () => { // The merged PR's author is "MINER1" (different case from the contributor "miner1"). The predictor still // counts it as history, but blocker disposition never depends on it (#2411). + // #9129: the duplicate sibling carries resolved changedFiles (corroborated) -- an uncorroborated citation + // has zero gate effect regardless of duplicatePrGateMode, so it wouldn't exercise this path at all. const mixedCase = verdict({ gate: { duplicates: "block" }, pullRequests: [ - openPr(42, "Retry uploads on 5xx responses", [7], "someone-else"), + { ...openPr(42, "Retry uploads on 5xx responses", [7], "someone-else"), changedFiles: ["src/upload-client.ts"] }, { ...openPr(9, "Earlier fix", [], "MINER1"), state: "merged", mergedAt: "2026-06-01T00:00:00.000Z" }, ], }); - expect(mixedCase.conclusion).toBe("failure"); + expect(mixedCase.conclusion).toBe("neutral"); }); it("keeps duplicate blockers for repeat offenders via the closed-unmerged author-count path", () => { // The author has 3 prior CLOSED-unmerged PRs (state === "closed" && !mergedAt) in this repo. Blocker // disposition no longer depends on first-time grace, so the gate blocks either way. + // #9129: the duplicate sibling carries resolved changedFiles (corroborated) -- "block" now holds, never closes. const closedUnmerged = (number: number, title: string): PullRequestRecord => ({ ...openPr(number, title, [], "miner1"), state: "closed", @@ -192,28 +218,29 @@ describe("buildPredictedGateVerdict", () => { const result = verdict({ gate: { duplicates: "block" }, pullRequests: [ - openPr(42, "Retry uploads on 5xx responses", [7], "someone-else"), + { ...openPr(42, "Retry uploads on 5xx responses", [7], "someone-else"), changedFiles: ["src/upload-client.ts"] }, closedUnmerged(11, "Abandoned attempt one"), closedUnmerged(12, "Abandoned attempt two"), closedUnmerged(13, "Abandoned attempt three"), ], }); - expect(result.conclusion).toBe("failure"); - expect(result.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(true); + expect(result.conclusion).toBe("neutral"); + expect(result.warnings.some((w) => w.code === "duplicate_pr_risk")).toBe(true); }); it("counts a closed-but-merged PR as merge history via the mergedAt fallback (not state === merged)", () => { // The prior PR has state "closed" yet carries a mergedAt timestamp, so it is still counted as merge history. // Blocker disposition no longer depends on first-time grace, so the gate blocks either way. + // #9129: the duplicate sibling carries resolved changedFiles (corroborated) -- "block" now holds, never closes. const result = verdict({ gate: { duplicates: "block" }, pullRequests: [ - openPr(42, "Retry uploads on 5xx responses", [7], "someone-else"), + { ...openPr(42, "Retry uploads on 5xx responses", [7], "someone-else"), changedFiles: ["src/upload-client.ts"] }, { ...openPr(9, "Earlier merged fix", [], "miner1"), state: "closed", mergedAt: "2026-06-01T00:00:00.000Z" }, ], }); - expect(result.conclusion).toBe("failure"); - expect(result.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(true); + expect(result.conclusion).toBe("neutral"); + expect(result.warnings.some((w) => w.code === "duplicate_pr_risk")).toBe(true); }); it("predicts a non-confirmed contributor NORMALLY — a blocker → failure, matching the real gate (#gate-nonconfirmed)", () => { diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index fb959a9b4e..49846cbcfb 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -390,7 +390,10 @@ describe("queue processors", () => { }); it("close policy on a PR thread: labels + closes once the threshold is crossed, with no merit review", async () => { - const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + // #9132: LOOPOVER_REVIEW_PARITY_AUDIT: "true" so recordNativeGateDecision/recordContributorGateDecision/ + // recordPredictedGateCalibration (all flag-gated the same way) actually write, letting this test observe + // the gate_decision row #9132's fix now produces for a review-nag close. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_PARITY_AUDIT: "true" }); await upsertInstallation(env, { installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], @@ -420,11 +423,81 @@ describe("queue processors", () => { expect(seen.comments.some((c) => c.includes("chatty") && c.includes("4 times"))).toBe(true); const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); expect(closeAudit?.n).toBeGreaterThanOrEqual(1); - // #9134 REGRESSION: this review-nag close previously wrote NO decision record at all. - const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 203").bind("JSONbored/gittensory").first<{ action: string; reason_code: string }>(); + // #9134 REGRESSION: this review-nag close previously wrote NO decision record at all -- now covered by + // executeAgentMaintenanceActions' own hoisted recordCompletedDecision (the generic, non-managedByCaller + // path this call site's `decisionRecord: { configDigest }` context takes). + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = ?").bind("JSONbored/gittensory", 203).first<{ action: string; reason_code: string }>(); expect(decisionRecord).toMatchObject({ action: "close", reason_code: "policy_close:review_nag" }); const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); + + // #9132 REGRESSION: the review-nag close ALSO records a gate_decision row through the SAME + // applyPrecisionBreakers + recordNativeGateDecision sequence the main disposition path uses -- #9086's + // breaker coverage is now actually reachable, and the decision is no longer invisible to + // queryRuleGateCells (the #8825 misprediction-poisoning bug this closes). This is a DIFFERENT row than + // #9134's decision_records check above: #9134 hoisted decision_records/decision_ledger into the executor + // for every completed merge/close; #9132 is the review_audit `gate_decision` calibration row, which the + // executor's hoist does not write and finalizePolicyCloseDisposition (processors.ts) still records + // directly, breaker-downgrade-and-all, BEFORE the executor ever runs. + const gateDecision = await env.DB.prepare( + "select decision, summary from review_audit where target_id = ? and event_type = 'gate_decision' order by created_at desc limit 1", + ) + .bind("JSONbored/gittensory#203") + .first<{ decision: string; summary: string }>(); + expect(gateDecision?.decision).toBe("close"); + expect(gateDecision?.summary).toBe("policy_close:review_nag"); + }); + + // #9132 REGRESSION: #9086 added "review_nag" to CONTENT_INSPECTION_CLOSE_KINDS so the close-precision + // breaker (downgradeCloseToHold) would cover it -- but neither review-nag close site ever called + // applyPrecisionBreakers, so the breaker could never actually engage no matter how wrong the close was. + // With the project's close-precision breaker engaged (closehold: in system_flags), a review-nag + // close must now be DOWNGRADED TO A HOLD (manual-review label instead of a close) -- proving the close is + // genuinely breaker-eligible, not just carrying the right code in a set no one consults. + it("REGRESSION (#9132): a review-nag close is downgraded to a hold when the project's close-precision breaker is engaged", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_PARITY_AUDIT: "true" }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewNagPolicy: "close", reviewNagMaxPings: 3 } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 204, title: "Breaker-held close", state: "open", user: { login: "chatty" }, head: { sha: "sha204" }, author_association: "NONE", labels: [], body: "" }); + // The close-precision breaker is engaged repo-wide -- mirrors how queue-lifecycle-guards.test.ts and the + // main-disposition-path breaker tests seed the same system_flags row directly. + await env.DB.prepare("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('closehold:JSONbored/gittensory', '1', CURRENT_TIMESTAMP)").run(); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#204", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(204, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-breaker-held", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 204, title: "Breaker-held close", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@loopover help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + // The breaker downgraded the close: never closed, and the manual-review label (not the review-nag label) + // was applied instead. + expect(seen.closed).toBe(false); + expect(seen.labels).toContain("manual-review"); + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); + expect(closeAudit?.n ?? 0).toBe(0); + // The disposition still records as a hold (no close action survived the breaker), still naming the + // review-nag reason code -- so this genuinely-blocked close is STILL visible to calibration, unlike + // before #9132 where it was invisible either way. + const gateDecision = await env.DB.prepare( + "select decision from review_audit where target_id = ? and event_type = 'gate_decision' order by created_at desc limit 1", + ) + .bind("JSONbored/gittensory#204") + .first<{ decision: string }>(); + expect(gateDecision?.decision).toBe("hold"); }); it("REGRESSION (#review-nag-cross-pr-carryover): a contributor who exhausted their pings on PR A carries the count over to a BRAND-NEW PR B instead of resetting to a clean 0/maxPings slate", async () => { @@ -962,7 +1035,10 @@ describe("queue processors", () => { }); it("close policy on a PR thread: labels + closes once the threshold is crossed, reusing reviewNagLabel", async () => { - const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + // #9132: LOOPOVER_REVIEW_PARITY_AUDIT: "true" so recordNativeGateDecision/recordContributorGateDecision/ + // recordPredictedGateCalibration actually write, letting this test observe the gate_decision row #9132's + // fix now produces for the monitored-mention nag close (the SECOND review-nag close site). + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_PARITY_AUDIT: "true" }); await upsertInstallation(env, { installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], @@ -992,11 +1068,23 @@ describe("queue processors", () => { // #label-scoping: close: "auto" alone (no broad label: "auto") is sufficient for the label AND the close. // #9134 REGRESSION: this monitored-mentions close (the @mention-triggered review-nag variant) previously // wrote NO decision record at all -- same closeKind ("review_nag") as its ping-count sibling above, - // since planAgentMaintenanceActions tags both through the same reviewNagMatch field. - const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 305").bind("JSONbored/gittensory").first<{ action: string; reason_code: string }>(); + // since planAgentMaintenanceActions tags both through the same reviewNagMatch field. Now covered by + // executeAgentMaintenanceActions' own hoisted recordCompletedDecision. + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = ?").bind("JSONbored/gittensory", 305).first<{ action: string; reason_code: string }>(); expect(decisionRecord).toMatchObject({ action: "close", reason_code: "policy_close:review_nag" }); const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); + + // #9132 REGRESSION: the SECOND review-nag close site (monitored-mention) also now records a gate_decision + // row through the shared finalizePolicyCloseDisposition sequence -- a DIFFERENT row than #9134's + // decision_records check above (see the ping-count sibling test's comment for why both are expected). + const gateDecision = await env.DB.prepare( + "select decision, summary from review_audit where target_id = ? and event_type = 'gate_decision' order by created_at desc limit 1", + ) + .bind("JSONbored/gittensory#305") + .first<{ decision: string; summary: string }>(); + expect(gateDecision?.decision).toBe("close"); + expect(gateDecision?.summary).toBe("policy_close:review_nag"); }); it("REGRESSION (#review-nag-cross-pr-carryover): a contributor who exhausted their @-mention pings for ONE login on PR A carries that login's count over to a BRAND-NEW PR B", async () => { @@ -2777,9 +2865,14 @@ describe("queue processors", () => { expect(winnerAdvisory?.findings_json ?? "").not.toContain("duplicate_pr_risk"); }); - it("#dup-winner: flag OFF keeps every same-issue sibling blocked (byte-identical) — the winner is also closed-eligible", async () => { - // Same cluster, flag OFF (default). The lowest open PR (#91) STILL gets the duplicate block + finding, - // exactly like today — no winner is spared. + it("#9129 REGRESSION: an uncorroborated same-issue overlap no longer force-closes, even with duplicatePrGateMode: block set explicitly", async () => { + // Same cluster, duplicate-winner flag OFF (default). Before #9129, the lowest open PR (#91) got a hard + // duplicate BLOCK + gate failure here purely from the sibling's own linked-issue body text -- exactly the + // adversarial primitive #9129 closes (an attacker's throwaway PR body, no code required, forcing a rival's + // clean PR closed). Neither PR has changed-file data resolved for the OTHER side (the gate's own + // otherOpenPullRequests input is deliberately not enriched with changedFiles by default -- see + // hasDuplicateOverlapCorroboration's doc comment), so this overlap is UNCORROBORATED: it now surfaces only + // as the non-blocking duplicate_pr_risk_unconfirmed finding, and the gate passes. const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( env, @@ -2823,10 +2916,11 @@ describe("queue processors", () => { }, }); - // Flag OFF: the duplicate block still fires for the lowest sibling — the Gate fails, the finding persists. - expect(gatePatchBody.conclusion).toBe("failure"); + // #9129: an uncorroborated overlap is advisory-only -- the gate passes, and the finding fired is the + // never-blocking unconfirmed code, not the concrete one. + expect(gatePatchBody.conclusion).toBe("success"); const winnerAdvisory = await env.DB.prepare("select findings_json from advisories where target_type = 'pull_request' and repo_full_name = ? and pull_number = ?").bind("JSONbored/gittensory", 91).first<{ findings_json: string }>(); - expect(winnerAdvisory?.findings_json ?? "").toContain("duplicate_pr_risk"); + expect(winnerAdvisory?.findings_json ?? "").toContain("duplicate_pr_risk_unconfirmed"); }); 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 () => { diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index 2e088e8479..dd94254069 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -207,7 +207,9 @@ describe("advisory rules", () => { expect(advisory.findings.map((finding) => finding.code)).toContain("issue_has_linked_prs"); }); - it("flags duplicate risk when another open PR references the same linked issue", () => { + // #9129: a plain body-text overlap (neither side has changedFiles resolved) is UNCORROBORATED -- it gets the + // separate, always-non-blocking duplicate_pr_risk_unconfirmed code, never the concrete duplicate_pr_risk code. + it("flags an UNCONFIRMED duplicate risk when another open PR references the same linked issue with no corroborating diff evidence (#9129)", () => { const pr: PullRequestRecord = { repoFullName: repo.fullName, number: 12, @@ -228,6 +230,64 @@ describe("advisory rules", () => { const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests: [otherPr] }); + expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk_unconfirmed"); + expect(advisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk"); + }); + + // #9129: a genuine changed-file-path overlap CORROBORATES the same scenario -- now the concrete duplicate_pr_risk + // code fires instead, and it stays configurable via duplicatePrGateMode (see the gate-mode tests below). + it("flags a CONFIRMED duplicate risk when the overlapping open PR shares a changed-file path (#9129)", () => { + const pr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 12, + title: "Add registry sync", + state: "open", + authorLogin: "oktofeesh1", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [4], + changedFiles: ["src/registry/sync.ts"], + }; + const otherPr: PullRequestRecord = { + ...pr, + number: 13, + title: "Alternative registry sync", + linkedIssues: [4], + changedFiles: ["src/registry/sync.ts", "src/registry/other.ts"], + }; + + const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests: [otherPr] }); + + expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); + expect(advisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk_unconfirmed"); + }); + + // #9129: the ALTERNATE corroboration path -- the sibling has a resolved, non-empty changed-file set of its own + // (a real diff exists), even though it shares no path with this PR's own changes. + it("flags a CONFIRMED duplicate risk when the sibling PR is a non-trivial real change, even without shared file paths (#9129)", () => { + const pr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 12, + title: "Add registry sync", + state: "open", + authorLogin: "oktofeesh1", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [4], + // This PR's own changedFiles are unresolved -- corroboration must still work off the sibling alone. + }; + const otherPr: PullRequestRecord = { + ...pr, + number: 13, + title: "Alternative registry sync", + linkedIssues: [4], + changedFiles: ["src/registry/other-approach.ts"], + }; + + const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests: [otherPr] }); + expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); }); @@ -268,7 +328,9 @@ describe("advisory rules", () => { const advisory = buildPullRequestAdvisory(repo, loser, { otherOpenPullRequests: [lowerSibling], duplicateWinnerEnabled: true }); - expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); + // Neither PR has changedFiles resolved here, so the finding is the UNCONFIRMED code (#9129) -- the + // duplicate-winner suppression logic itself is what this test covers, orthogonal to corroboration. + expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk_unconfirmed"); }); it("#dup-winner: flag OFF + would-be-winner ⇒ duplicate finding STILL present (byte-identical)", () => { @@ -288,7 +350,8 @@ describe("advisory rules", () => { const advisory = buildPullRequestAdvisory(repo, wouldBeWinner, { otherOpenPullRequests: [higherSibling], duplicateWinnerEnabled: false }); - expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); + // Neither PR has changedFiles resolved here (#9129: uncorroborated code). + expect(advisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk_unconfirmed"); }); it("#dup-winner: flag ON + no overlap ⇒ no duplicate finding (alone in cluster)", () => { @@ -309,6 +372,7 @@ describe("advisory rules", () => { const advisory = buildPullRequestAdvisory(repo, lonePr, { otherOpenPullRequests: [unrelated], duplicateWinnerEnabled: true }); expect(advisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk"); + expect(advisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk_unconfirmed"); }); it("keeps weak queue warnings advisory-only for the opt-in gate", () => { @@ -404,7 +468,7 @@ describe("advisory rules", () => { expect(output.text).toContain("[context]"); }); - it("keeps missing-issue advisory by default, blocks duplicates by default, honoring explicit modes", () => { + it("keeps missing-issue advisory by default, honoring an explicit block mode", () => { const pr: PullRequestRecord = { repoFullName: repo.fullName, number: 21, @@ -424,17 +488,71 @@ describe("advisory rules", () => { expect(evaluateGateCheck(missingIssueAdvisory, { linkedIssueGateMode: "advisory" }).conclusion).toBe("success"); expect(evaluateGateCheck(missingIssueAdvisory, { linkedIssueGateMode: "off" }).conclusion).toBe("success"); expect(evaluateGateCheck(missingIssueAdvisory, { linkedIssueGateMode: "block" }).conclusion).toBe("failure"); + }); - const linkedPr: PullRequestRecord = { ...pr, number: 22, linkedIssues: [44] }; - const duplicateAdvisory = buildPullRequestAdvisory(repo, linkedPr, { - otherOpenPullRequests: [{ ...linkedPr, number: 23, linkedIssues: [44] }], - }); + // #9129 ADVERSARIAL REGRESSION: a PR with an overlapping linked issue but NO corroborating diff overlap must + // never produce a close, under ANY duplicatePrGateMode -- including an explicit "block", which is exactly the + // live-exploited configuration (.loopover.yml sets `duplicates: block` on this very repo). Before this fix, an + // attacker could cite the same issue number in a throwaway PR body (no code required) and force this exact + // scenario to auto-close the victim's PR one-shot. + it("#9129: an overlapping linked issue with NO diff-overlap corroboration never closes, under any duplicatePrGateMode", () => { + const pr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 22, + title: "Add review panel", + state: "open", + authorLogin: "victim", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [44], + // No changedFiles resolved -- the common case, since collision enrichment is scoped away from the gate's + // own otherOpenPullRequests input by default (see hasDuplicateOverlapCorroboration's doc comment). + }; + const attackerPr: PullRequestRecord = { ...pr, number: 23, authorLogin: "attacker", title: "Fixes the same thing" }; + const duplicateAdvisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests: [attackerPr] }); + + expect(duplicateAdvisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk_unconfirmed"); + expect(duplicateAdvisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk"); + for (const duplicatePrGateMode of ["advisory", "off", "block"] as const) { + const gate = evaluateGateCheck(duplicateAdvisory, { duplicatePrGateMode }); + expect(gate.conclusion).toBe("success"); + expect(gate.blockers).toEqual([]); + } + }); + + // #9129: once corroborated (a real changed-file overlap), the finding stays configurable via + // duplicatePrGateMode -- but "block" now HOLDS (neutral) rather than closing, matching the requirement to + // "prefer holding BOTH over closing either automatically". The new code-level default (unset ⇒ "advisory") + // never even holds; a maintainer must opt in to "block" to get the hold behavior at all. + it("#9129: a CORROBORATED duplicate overlap stays advisory by default, and HOLDS (never closes) under an explicit block mode", () => { + const pr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 22, + title: "Add review panel", + state: "open", + authorLogin: "someone", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [44], + changedFiles: ["src/panel.ts"], + }; + const sibling: PullRequestRecord = { ...pr, number: 23, changedFiles: ["src/panel.ts"] }; + const duplicateAdvisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests: [sibling] }); expect(duplicateAdvisory.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); - expect(evaluateGateCheck(duplicateAdvisory).conclusion).toBe("failure"); + + // Code-level default (unset) is now "advisory" (#9129, was "block") -- never blocks, never holds. + expect(evaluateGateCheck(duplicateAdvisory).conclusion).toBe("success"); expect(evaluateGateCheck(duplicateAdvisory, { duplicatePrGateMode: "advisory" }).conclusion).toBe("success"); expect(evaluateGateCheck(duplicateAdvisory, { duplicatePrGateMode: "off" }).conclusion).toBe("success"); - expect(evaluateGateCheck(duplicateAdvisory, { duplicatePrGateMode: "block" }).conclusion).toBe("failure"); + + // Explicit "block" HOLDS both sides for a human instead of closing either. + const held = evaluateGateCheck(duplicateAdvisory, { duplicatePrGateMode: "block" }); + expect(held.conclusion).toBe("neutral"); + expect(held.blockers).toEqual([]); + expect(held.warnings.map((finding) => finding.code)).toContain("duplicate_pr_risk"); }); it("a reviewer SPLIT (ai_review_split) blocks → close, gated like a consensus defect by aiReviewGateMode (#ai-review-split)", () => { @@ -653,22 +771,25 @@ describe("advisory rules", () => { }); it("gates NON-confirmed contributors normally — a real blocker closes them like a confirmed author (#gate-nonconfirmed)", () => { + // Uses missing_linked_issue (not duplicate_pr_risk, #9129): a duplicate-only blocker set now HOLDS instead of + // closing regardless of confirmed-contributor status, which would confound this test's actual subject (that + // confirmed-status itself has no effect on the verdict) with the unrelated duplicate-only hold behavior. const blockingAdvisory = { ...buildPullRequestAdvisory(repo, null), - findings: [{ code: "duplicate_pr_risk", title: "Linked issue overlaps another open PR", severity: "warning" as const, detail: "Duplicate." }], + findings: [{ code: "missing_linked_issue", title: "No linked issue detected", severity: "warning" as const, detail: "No linked issue." }], }; // Non-confirmed author: gated NORMALLY now — a real blocker → failure (one-shot close), no longer forced to a // neutral/held state. Confirmed-status affects only on-chain scoring, never the gate verdict. (#gate-nonconfirmed) - const nonConfirmed = evaluateGateCheck(blockingAdvisory, { duplicatePrGateMode: "block", confirmedContributor: false }); + const nonConfirmed = evaluateGateCheck(blockingAdvisory, { linkedIssueGateMode: "block", confirmedContributor: false }); expect(nonConfirmed.conclusion).toBe("failure"); - expect(nonConfirmed.title).toBe("LoopOver Orb Review Agent: Linked issue overlaps another open PR"); - expect(nonConfirmed.blockers.map((finding) => finding.code)).toEqual(["duplicate_pr_risk"]); + expect(nonConfirmed.title).toBe("LoopOver Orb Review Agent: No linked issue detected"); + expect(nonConfirmed.blockers.map((finding) => finding.code)).toEqual(["missing_linked_issue"]); // Confirmed author with the same blocker: identical verdict. - const confirmed = evaluateGateCheck(blockingAdvisory, { duplicatePrGateMode: "block", confirmedContributor: true }); + const confirmed = evaluateGateCheck(blockingAdvisory, { linkedIssueGateMode: "block", confirmedContributor: true }); expect(confirmed.conclusion).toBe("failure"); - expect(confirmed.blockers.map((finding) => finding.code)).toEqual(["duplicate_pr_risk"]); + expect(confirmed.blockers.map((finding) => finding.code)).toEqual(["missing_linked_issue"]); // A clean PR from a non-confirmed author is a normal success → auto-merges. const cleanNonConfirmed = evaluateGateCheck({ ...buildPullRequestAdvisory(repo, null), findings: [] }, { confirmedContributor: false }); diff --git a/test/unit/self-review-adapter.test.ts b/test/unit/self-review-adapter.test.ts index 1824d51f70..142b9ab7d8 100644 --- a/test/unit/self-review-adapter.test.ts +++ b/test/unit/self-review-adapter.test.ts @@ -160,13 +160,16 @@ describe("runSelfReview", () => { expect(result.predictedGateVerdict).toEqual(direct); }); - it("a genuinely blocked synthetic diff (duplicate PR) is a failure conclusion and passesPredictedGate is false", () => { - const context = baseContext({ pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7])] }); + it("a genuinely held synthetic diff (corroborated duplicate PR) is a neutral conclusion and passesPredictedGate is false", () => { + // #9129: the sibling carries resolved changedFiles (corroborated) -- predicted-gate's own synthetic "self" + // PR never carries changedFiles (pre-submission, no diff on that side), so the sibling-alone corroboration + // arm is the only one reachable here. Corroborated + duplicates:block now HOLDS (neutral), never closes. + const context = baseContext({ pullRequests: [{ ...openPr(42, "Retry uploads on 5xx responses", [7]), changedFiles: ["src/upload.ts"] }] }); const result = runSelfReview(BASE_DIFF_STATE, context, { runSlopAssessment: () => noopSlop }); - expect(result.predictedGateVerdict.conclusion).toBe("failure"); + expect(result.predictedGateVerdict.conclusion).toBe("neutral"); expect(result.passesPredictedGate).toBe(false); - expect(result.predictedGateVerdict.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(true); + expect(result.predictedGateVerdict.warnings.some((w) => w.code === "duplicate_pr_risk")).toBe(true); const direct = buildPredictedGateVerdict({ input: buildSelfReviewPredictedGateInput(BASE_DIFF_STATE), @@ -183,7 +186,8 @@ describe("runSelfReview", () => { const passing = runSelfReview(BASE_DIFF_STATE, baseContext(), { runSlopAssessment: () => noopSlop }); expect(passing.passesPredictedGate).toBe(true); - const blocked = runSelfReview(BASE_DIFF_STATE, baseContext({ pullRequests: [openPr(42, "dup", [7])] }), { + // #9129: corroborated (resolved changedFiles) so this is a genuine HOLD (neutral), not a silent success. + const blocked = runSelfReview(BASE_DIFF_STATE, baseContext({ pullRequests: [{ ...openPr(42, "dup", [7]), changedFiles: ["src/upload.ts"] }] }), { runSlopAssessment: () => noopSlop, }); expect(blocked.predictedGateVerdict.conclusion).not.toBe(SELF_REVIEW_PASSING_CONCLUSION); diff --git a/test/unit/selfhost-pg-queue.test.ts b/test/unit/selfhost-pg-queue.test.ts index 5fcd7690f0..e373a72dac 100644 --- a/test/unit/selfhost-pg-queue.test.ts +++ b/test/unit/selfhost-pg-queue.test.ts @@ -94,9 +94,13 @@ interface MockPool { * githubRateLimitAdmissionTargetForJob returns null for it and the rate-limit-clear OR-condition is * trivially/always true, isolating the AGE condition cleanly for tests that aren't specifically about * rate-limit clearing. Pass an explicit `payload` (e.g. a github-webhook message) plus `setRateLimitRows` - * to test the rate-limit-clear condition itself, or "not valid json" to test the unparseable-payload path. */ + * to test the rate-limit-clear condition itself, or "not valid json" to test the unparseable-payload path. + * `deferred_by` defaults to `"rate_limit"` (#9127): the real candidate SELECT filters on + * `deferred_by IS NOT NULL`, so a row must carry SOME admission-gate provenance tag to be a candidate at all + * -- pass `deferred_by: null` explicitly to model a row deferred by enqueue()'s own delaySeconds instead + * (which must never be released, see the dedicated #9127 provenance tests). */ setForegroundLivenessCandidates( - rows: Array<{ id: string; created_at: number; payload?: string }>, + rows: Array<{ id: string; created_at: number; payload?: string; deferred_by?: string | null }>, updateRowCounts?: number[], ): void; /** Like setForegroundLivenessCandidates, but programs the OLDEST-ordered and NEWEST-ordered candidate windows @@ -108,8 +112,8 @@ interface MockPool { * windows are genuinely independent -- i.e. a candidate present in one window but not the other -- must use * this instead. */ setForegroundLivenessCandidatesByWindow( - oldestRows: Array<{ id: string; created_at: number; payload?: string }>, - newestRows: Array<{ id: string; created_at: number; payload?: string }>, + oldestRows: Array<{ id: string; created_at: number; payload?: string; deferred_by?: string | null }>, + newestRows: Array<{ id: string; created_at: number; payload?: string; deferred_by?: string | null }>, updateRowCounts?: number[], ): void; } @@ -126,8 +130,8 @@ function makePool(): MockPool { let pressureMaintenance: { cnt: number; oldest: number | null } = { cnt: 0, oldest: null }; let pressureBacklogConvergence: { cnt: number } = { cnt: 0 }; let pressureFreshIntake: { cnt: number } = { cnt: 0 }; - let foregroundLivenessOldestCandidates: Array<{ id: string; created_at: number; payload?: string }> = []; - let foregroundLivenessNewestCandidates: Array<{ id: string; created_at: number; payload?: string }> = []; + let foregroundLivenessOldestCandidates: Array<{ id: string; created_at: number; payload?: string; deferred_by?: string | null }> = []; + let foregroundLivenessNewestCandidates: Array<{ id: string; created_at: number; payload?: string; deferred_by?: string | null }> = []; const foregroundLivenessUpdateRowCounts: number[] = []; const DEFAULT_FOREGROUND_LIVENESS_PAYLOAD = JSON.stringify({ type: "recapture-preview", @@ -138,22 +142,28 @@ function makePool(): MockPool { }); const fn = vi.fn().mockImplementation(async (sql: unknown, params?: unknown[]) => { const q = String(sql); - if (q.includes("SELECT id, payload, created_at FROM") && q.includes("priority>=$1 AND run_after>$2")) { + if (q.includes("SELECT id, payload, created_at, deferred_by FROM") && q.includes("priority>=$1 AND run_after>$2")) { // #selfhost-queue-liveness clear-bucket starvation fix: releaseStaleForegroundDeferrals issues an OLDEST- // ordered window and a NEWEST-ordered window as two independent queries -- match on ORDER BY direction so // setForegroundLivenessCandidatesByWindow can program them differently (setForegroundLivenessCandidates // programs both windows identically, matching this repo's other tests that don't care about the split). - const rows = q.includes("ORDER BY created_at DESC") ? foregroundLivenessNewestCandidates : foregroundLivenessOldestCandidates; - return { - rows: rows.map((row) => ({ + const candidates = q.includes("ORDER BY created_at DESC") ? foregroundLivenessNewestCandidates : foregroundLivenessOldestCandidates; + // #9127: default deferred_by to "rate_limit" (so existing tests that don't care about provenance keep + // exercising the age/rate-limit-clear conditions unchanged), then FILTER out anything explicitly tagged + // `deferred_by: null` -- faithfully simulating the real candidate SELECT's own `deferred_by IS NOT NULL` + // predicate, which structurally excludes an enqueue-time delay (never admission-gate-tagged) from ever + // reaching the eligibility pass at all, regardless of age or rate-limit state. + const rows = candidates + .map((row) => ({ id: row.id, payload: row.payload ?? DEFAULT_FOREGROUND_LIVENESS_PAYLOAD, created_at: row.created_at, - })), - rowCount: rows.length, - }; + deferred_by: row.deferred_by === undefined ? "rate_limit" : row.deferred_by, + })) + .filter((row) => row.deferred_by !== null); + return { rows, rowCount: rows.length }; } - if (q.includes("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1")) { + if (q.includes("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1")) { const rowCount = foregroundLivenessUpdateRowCounts.length > 0 ? (foregroundLivenessUpdateRowCounts.shift() ?? 1) : 1; return { rows: [], rowCount }; @@ -2325,11 +2335,11 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(1); expect(m.fn).toHaveBeenCalledWith( - expect.stringContaining("SELECT id, payload, created_at FROM _selfhost_jobs WHERE status='pending' AND priority>=$1 AND run_after>$2"), + expect.stringContaining("SELECT id, payload, created_at, deferred_by FROM _selfhost_jobs WHERE status='pending' AND priority>=$1 AND run_after>$2"), expect.arrayContaining([8]), ); expect(m.fn).toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining(["fg-1"]), ); expect(await renderMetrics()).toContain("loopover_jobs_foreground_liveness_released_total 1"); @@ -2365,7 +2375,7 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(0); expect(m.fn).not.toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining(["fg-fresh"]), ); expect(await renderMetrics()).not.toContain("loopover_jobs_foreground_liveness_released_total"); @@ -2472,7 +2482,7 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(0); expect(m.fn).not.toHaveBeenCalledWith( - expect.stringContaining("SELECT id, payload, created_at FROM _selfhost_jobs WHERE status='pending' AND priority>=$1 AND run_after>$2"), + expect.stringContaining("SELECT id, payload, created_at, deferred_by FROM _selfhost_jobs WHERE status='pending' AND priority>=$1 AND run_after>$2"), expect.anything(), ); expect(await renderMetrics()).not.toContain("loopover_jobs_foreground_liveness_released_total"); @@ -2503,7 +2513,7 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(3); for (const id of ["stuck-1", "stuck-2", "stuck-3"]) { expect(m.fn).toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining([id]), ); } @@ -2547,13 +2557,13 @@ describe("createPgQueue (durable #977)", () => { ); for (const id of ["oldest", "second-oldest"]) { expect(m.fn).toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining([id]), ); } for (const id of ["newer", "newest"]) { expect(m.fn).not.toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining([id]), ); } @@ -2593,12 +2603,12 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(2); for (const id of ["clear-newer", "blocked-oldest"]) { expect(m.fn).toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining([id]), ); } expect(m.fn).not.toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining(["blocked-second"]), ); }); @@ -2651,12 +2661,12 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(2); for (const id of ["clear-newer", "blocked-oldest"]) { expect(m.fn).toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining([id]), ); } expect(m.fn).not.toHaveBeenCalledWith( - expect.stringContaining("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), expect.arrayContaining(["blocked-second"]), ); }); @@ -2690,10 +2700,10 @@ describe("createPgQueue (durable #977)", () => { // than propagate NaN (mirrors init()'s own "handles null rowCount from the recovery query" test). const fn = vi.fn().mockImplementation(async (sql: unknown) => { const q = String(sql); - if (q.includes("SELECT id, payload, created_at FROM") && q.includes("priority>=$1 AND run_after>$2")) { - return { rows: [{ id: "fg-null", created_at: now - 5 * 60_000 }], rowCount: 1 }; + if (q.includes("SELECT id, payload, created_at, deferred_by FROM") && q.includes("priority>=$1 AND run_after>$2")) { + return { rows: [{ id: "fg-null", created_at: now - 5 * 60_000, deferred_by: "rate_limit" }], rowCount: 1 }; } - if (q.includes("SET run_after=$1 WHERE id=$2 AND status='pending' AND run_after>$1")) { + if (q.includes("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1")) { return { rows: [], rowCount: null }; } return { rows: [], rowCount: 0 }; @@ -2705,6 +2715,88 @@ describe("createPgQueue (durable #977)", () => { expect(released).toBe(0); // null ?? 0 -- no metric recorded, no crash expect(await renderMetrics()).not.toContain("loopover_jobs_foreground_liveness_released_total"); }); + + // INVARIANT (#9127): a row with NO admission-gate provenance tag -- e.g. one whose future run_after came from + // enqueue(message, delaySeconds), never from a rate-limit/maintenance-admission/installation-concurrency + // defer -- must never be released, regardless of how age-stale it is. Before #9127 the candidate SELECT had + // no provenance filter at all, so ANY pending foreground row scheduled in the future qualified and + // isRateLimitAdmissionNowClear degraded to "clear" for a job type with no rate-limit bucket (like + // recapture-preview): the very next sweep tick released it regardless of delaySeconds. Uses a created_at far + // older than FOREGROUND_LIVENESS_MAX_DEFER_MS -- old enough that the AGE arm alone would have released it + // pre-fix -- to prove the deferred_by filter excludes it BEFORE either eligibility condition is even + // evaluated, not just that the rate-limit-clear arm happens not to fire. + it("INVARIANT (#9127): a row with no deferred_by provenance tag is never released, even when far past the age-stale ceiling", async () => { + process.env.FOREGROUND_LIVENESS_MAX_DEFER_MS = "60000"; // 1m floor + const m = makePool(); + const now = Date.now(); + // deferred_by: null models a job enqueued with delaySeconds -- never admission-gate-deferred. + m.setForegroundLivenessCandidates([{ id: "delay-not-admission", created_at: now - 30 * 60_000, deferred_by: null }]); + const q = createPgQueue(m.pool, async () => undefined); + + const released = await q.releaseStaleForegroundDeferrals(); + + expect(released).toBe(0); + expect(m.fn).not.toHaveBeenCalledWith( + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.arrayContaining(["delay-not-admission"]), + ); + expect(await renderMetrics()).not.toContain("loopover_jobs_foreground_liveness_released_total"); + }); + + // REGRESSION (#9127): the contributor-facing trigger. processors.ts's linked-issue flag-then-close grace + // window enqueues a "recapture-preview" job via env.JOBS.send(verifyJob, { delaySeconds }) (up to 300s, + // closeDelaySeconds) after applying the pending-closure label -- never through an admission gate, so it must + // never carry a deferred_by tag and must survive a liveness sweep regardless of how aggressively configured. + it("REGRESSION (#9127): the linked-issue flag-then-close grace-window job type survives a liveness sweep even when it is the ONLY candidate", async () => { + process.env.FOREGROUND_LIVENESS_MAX_DEFER_MS = "60000"; // deliberately aggressive + const m = makePool(); + const now = Date.now(); + const graceWindowPayload = JSON.stringify({ + type: "recapture-preview", + deliveryId: "linked-issue-verify:o/r#42", + repoFullName: "o/r", + prNumber: 42, + installationId: 1, + attempt: 0, + }); + m.setForegroundLivenessCandidates([ + { id: "grace-window", created_at: now - 10 * 60_000, payload: graceWindowPayload, deferred_by: null }, + ]); + const q = createPgQueue(m.pool, async () => undefined); + + const released = await q.releaseStaleForegroundDeferrals(); + + expect(released).toBe(0); + }); + + // The rate-limit-clear recheck is scoped to deferred_by==='rate_limit' ONLY (#9127): a row deferred by + // maintenance-admission or installation-concurrency has no rate-limit bucket either, so treating it as + // trivially "clear" would release it on the very next sweep regardless of whether ITS OWN gate cleared -- + // the same false-premise bug this issue fixes, just for a different admission kind. Covers both outcomes: + // the age-stale row (deferred_by='installation_concurrency') releases via the AGE arm alone, and the fresh + // row (deferred_by='maintenance_admission') stays parked since neither arm applies to it. + it("scopes the rate-limit-clear recheck to deferred_by='rate_limit' -- other admission-gate tags only release via the age arm", async () => { + process.env.FOREGROUND_LIVENESS_MAX_DEFER_MS = "60000"; // 1m floor + const m = makePool(); + const now = Date.now(); + m.setForegroundLivenessCandidates([ + { id: "concurrency-stale", created_at: now - 5 * 60_000, deferred_by: "installation_concurrency" }, + { id: "maintenance-fresh", created_at: now - 1_000, deferred_by: "maintenance_admission" }, + ]); + const q = createPgQueue(m.pool, async () => undefined); + + const released = await q.releaseStaleForegroundDeferrals(); + + expect(released).toBe(1); + expect(m.fn).toHaveBeenCalledWith( + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.arrayContaining(["concurrency-stale"]), + ); + expect(m.fn).not.toHaveBeenCalledWith( + expect.stringContaining("SET run_after=$1, deferred_by=NULL WHERE id=$2 AND status='pending' AND run_after>$1"), + expect.arrayContaining(["maintenance-fresh"]), + ); + }); }); describe("processingCount (#selfhost-queue-liveness)", () => { diff --git a/test/unit/selfhost-sqlite-queue.test.ts b/test/unit/selfhost-sqlite-queue.test.ts index 3a0bdfdfef..cd9ad0fa54 100644 --- a/test/unit/selfhost-sqlite-queue.test.ts +++ b/test/unit/selfhost-sqlite-queue.test.ts @@ -2340,19 +2340,24 @@ describe("createSqliteQueue (durable #980)", () => { * "recapture-preview" by default -- a foreground type NOT in GITHUB_BUDGET_BACKGROUND_TYPES and not * "github-webhook"/"agent-regate-pr", so githubRateLimitAdmissionTargetForJob returns null for it and the * rate-limit-clear condition (isRateLimitAdmissionNowClear) is trivially/always true for these rows -- - * isolating the AGE-based condition cleanly for tests that aren't specifically about rate-limit clearing. */ + * isolating the AGE-based condition cleanly for tests that aren't specifically about rate-limit clearing. + * `deferredBy` defaults to 'rate_limit' (#9127): a row must have SOME admission-gate provenance tag to even + * be a release candidate at all (the SELECT's own `deferred_by IS NOT NULL` filter) -- pass `null` explicitly + * to model a row deferred by enqueue()'s own delaySeconds instead, which must NEVER be released regardless + * of age or rate-limit state (see the dedicated #9127 provenance tests below). */ function seedForegroundPendingRow( driver: ReturnType, - opts: { createdAt: number; runAfter: number; priority?: number; type?: string }, + opts: { createdAt: number; runAfter: number; priority?: number; type?: string; deferredBy?: string | null }, ): void { driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, ?, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, ?, NULL, 0, ?)`, [ JSON.stringify({ type: opts.type ?? "recapture-preview", deliveryId: `seed:${opts.createdAt}`, repoFullName: "o/r", prNumber: 1, attempt: 1 }), opts.runAfter, opts.createdAt, opts.priority ?? 9, + opts.deferredBy === undefined ? "rate_limit" : opts.deferredBy, ], ); } @@ -2417,8 +2422,8 @@ describe("createSqliteQueue (durable #980)", () => { const q = createSqliteQueue(driver, async () => undefined); const futureRunAfter = now + 60 * 60_000; driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, [ JSON.stringify({ type: "github-webhook", deliveryId: "still-blocked", eventName: "x", payload: { installation: { id: 123 } } }), futureRunAfter, @@ -2445,8 +2450,8 @@ describe("createSqliteQueue (durable #980)", () => { const futureRunAfter = now + 60 * 60_000; for (const deliveryId of ["fg-fresh-1", "fg-fresh-2"]) { driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, [ JSON.stringify({ type: "github-webhook", @@ -2479,8 +2484,8 @@ describe("createSqliteQueue (durable #980)", () => { const futureRunAfter = now + 60 * 60_000; // No github_rate_limit_observations table/row at all -- rateLimitAdmissionDelayMs degrades to "clear". driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, [JSON.stringify({ type: "github-webhook", deliveryId: "now-clear", eventName: "x", payload: {} }), futureRunAfter, now - 1_000], ); @@ -2500,8 +2505,8 @@ describe("createSqliteQueue (durable #980)", () => { const now = Date.now(); const futureRunAfter = now + 60 * 60_000; driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, ["not valid json", futureRunAfter, now - 1_000], ); @@ -2626,8 +2631,8 @@ describe("createSqliteQueue (durable #980)", () => { ]; for (const row of rows) { driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, [ JSON.stringify({ type: "github-webhook", @@ -2669,8 +2674,8 @@ describe("createSqliteQueue (durable #980)", () => { // "oldest N" window is entirely consumed by these and never reaches the newer row below. for (let i = 0; i < 6; i += 1) { driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, [ JSON.stringify({ type: "github-webhook", deliveryId: `blocked-${i}`, eventName: "x", payload: { installation: { id: 111 } } }), farFuture, @@ -2680,8 +2685,8 @@ describe("createSqliteQueue (durable #980)", () => { } // The single newest pending row, on a different (clear) admission target. driver.query( - `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance) - VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0)`, + `INSERT INTO _selfhost_jobs (payload, status, attempts, run_after, created_at, priority, job_key, is_maintenance, deferred_by) + VALUES (?, 'pending', 0, ?, ?, 10, NULL, 0, 'rate_limit')`, [JSON.stringify({ type: "github-webhook", deliveryId: "clear-newer", eventName: "x", payload: { installation: { id: 222 } } }), farFuture, now - 1_000], ); @@ -2712,7 +2717,7 @@ describe("createSqliteQueue (durable #980)", () => { let armed = false; const realQuery = driver.query.bind(driver); vi.spyOn(driver, "query").mockImplementation((sql: string, params: unknown[]) => { - if (armed && sql.includes("SELECT id, payload, created_at FROM") && sql.includes("priority>=? AND run_after>?")) { + if (armed && sql.includes("SELECT id, payload, created_at, deferred_by FROM") && sql.includes("priority>=? AND run_after>?")) { throw new Error("disk I/O error"); } return realQuery(sql, params); @@ -2735,6 +2740,100 @@ describe("createSqliteQueue (durable #980)", () => { delete process.env.FOREGROUND_LIVENESS_CHECK_INTERVAL_MS; } }); + + // INVARIANT (#9127): enqueue(message, delaySeconds) is the ONLY place a foreground job's run_after is pushed + // into the future by a DELIBERATE delay (as opposed to an admission gate deferring it) -- such a row must + // never be released by the liveness sweep before its delay elapses, no matter how soon after enqueue the + // sweep fires or how long the delay is. Before #9127, this failed: recapture-preview has no rate-limit + // bucket (githubRateLimitAdmissionTargetForJob returns null for it), so isRateLimitAdmissionNowClear degraded + // to "clear" unconditionally, and the pre-fix candidate SELECT had no provenance filter at all -- the very + // next sweep tick released it regardless of delaySeconds. + it("INVARIANT (#9127): a job enqueued with delaySeconds is not runnable before its delay elapses, regardless of liveness-sweep activity", async () => { + const driver = makeDriver(); + const q = createSqliteQueue(driver, async () => undefined); + const now = Date.now(); + const delaySeconds = 300; + await q.binding.send( + { type: "recapture-preview", deliveryId: "delay-test", repoFullName: "o/r", prNumber: 1, installationId: 1, attempt: 0 } as unknown as JobMessage, + { delaySeconds }, + ); + + // Simulate the liveness sweep firing on the very next tick -- as little as one + // FOREGROUND_LIVENESS_CHECK_INTERVAL_MS after enqueue (60s default), long before delaySeconds elapses. + const released = await q.releaseStaleForegroundDeferrals(); + + expect(released).toBe(0); + const row = driver.query("SELECT run_after, deferred_by FROM _selfhost_jobs", []).rows[0] as { + run_after: number; + deferred_by: string | null; + }; + // Untouched: still (approximately) delaySeconds out, and never tagged as admission-gate-deferred. + expect(row.run_after).toBeGreaterThan(now + (delaySeconds - 1) * 1000); + expect(row.deferred_by).toBeNull(); + expect(await renderMetrics()).not.toContain("loopover_jobs_foreground_liveness_released_total"); + }); + + // REGRESSION (#9127): the contributor-facing trigger. processors.ts's linked-issue flag-then-close grace + // window enqueues exactly this "recapture-preview" shape with delaySeconds = closeDelaySeconds (up to 300s) + // after applying the pending-closure label, promising the contributor that many seconds to push a fix before + // Pass 2 re-verifies and closes. The bug collapsed this to one sweep tick (<=60s default); this test proves + // the grace window survives even a very aggressive liveness sweep configuration (tiny maxDeferMs), which + // would previously have released it via the AGE arm too if enqueue-time delays weren't structurally excluded + // from the candidate SELECT. + it("REGRESSION (#9127): the linked-issue flag-then-close grace window survives a liveness sweep", async () => { + process.env.FOREGROUND_LIVENESS_MAX_DEFER_MS = "60000"; // 1m floor -- deliberately aggressive + const driver = makeDriver(); + const q = createSqliteQueue(driver, async () => undefined); + const closeDelaySeconds = 300; + await q.binding.send( + { + type: "recapture-preview", + deliveryId: "linked-issue-verify:o/r#42", + repoFullName: "o/r", + prNumber: 42, + installationId: 1, + attempt: 0, + } as unknown as JobMessage, + { delaySeconds: closeDelaySeconds }, + ); + + const released = await q.releaseStaleForegroundDeferrals(); + + expect(released).toBe(0); + const row = driver.query("SELECT run_after FROM _selfhost_jobs WHERE payload LIKE '%linked-issue-verify%'", []) + .rows[0] as { run_after: number }; + expect(row.run_after).toBeGreaterThan(Date.now()); + }); + + // The rate-limit-clear recheck is scoped to deferred_by==='rate_limit' ONLY (#9127): a row deferred by + // maintenance-admission or installation-concurrency has no rate-limit bucket either, so treating it as + // trivially "clear" would release it on the very next sweep regardless of whether ITS OWN gate cleared. + // Covers both outcomes: the age-stale row (deferred_by='installation_concurrency') releases via the AGE arm + // alone, and the fresh row (deferred_by='maintenance_admission') stays parked since neither arm applies. + it("scopes the rate-limit-clear recheck to deferred_by='rate_limit' -- other admission-gate tags only release via the age arm", async () => { + process.env.FOREGROUND_LIVENESS_MAX_DEFER_MS = "60000"; // 1m floor + const driver = makeDriver(); + const q = createSqliteQueue(driver, async () => undefined); + const now = Date.now(); + const farFuture = now + 60 * 60_000; + seedForegroundPendingRow(driver, { + createdAt: now - 5 * 60_000, + runAfter: farFuture, + deferredBy: "installation_concurrency", + }); + seedForegroundPendingRow(driver, { + createdAt: now - 1_000, + runAfter: farFuture, + deferredBy: "maintenance_admission", + }); + + const released = await q.releaseStaleForegroundDeferrals(); + + expect(released).toBe(1); + const remaining = driver.query(`SELECT COUNT(*) AS c FROM _selfhost_jobs WHERE status='pending' AND run_after>?`, [now]) + .rows[0] as { c: number }; + expect(remaining.c).toBe(1); // the fresh, non-rate-limit-deferred row stays parked + }); }); describe("processingCount (#selfhost-queue-liveness)", () => {