diff --git a/.release-please-manifest.json b/.release-please-manifest.json index effd9a1389..1d401be83f 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,6 +1,6 @@ { "packages/loopover-mcp": "3.14.1", - "packages/loopover-engine": "3.15.0", + "packages/loopover-engine": "3.15.1", "packages/loopover-miner": "3.14.1", "packages/loopover-ui-kit": "1.2.0" } diff --git a/apps/loopover-ui/src/components/site/proof-of-power-stats-model.ts b/apps/loopover-ui/src/components/site/proof-of-power-stats-model.ts index 14033fefaa..e383f428ee 100644 --- a/apps/loopover-ui/src/components/site/proof-of-power-stats-model.ts +++ b/apps/loopover-ui/src/components/site/proof-of-power-stats-model.ts @@ -39,7 +39,10 @@ export type PublicStats = { closePrecisionCiPct?: { lo: number; hi: number } | null; coveragePct?: number | null; decidedCount?: number; - guaranteed?: { close: { alpha: number; lambda: number; coveragePct: number; n: number } | null; merge: { alpha: number; lambda: number; coveragePct: number; n: number } | null }; + guaranteed?: { + close: { alpha: number; lambda: number; coveragePct: number; n: number } | null; + merge: { alpha: number; lambda: number; coveragePct: number; n: number } | null; + }; instanceCount: number; windowDays: number; gamingFlagsCaught: number; diff --git a/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx b/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx index 45d760b709..c11187ddca 100644 --- a/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx +++ b/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx @@ -206,7 +206,13 @@ describe("ProofOfPowerStats", () => { durationMs: 1, data: { ...PAYLOAD, - fleetAccuracy: { accuracyPct: 80, coveragePct: 64.3, instanceCount: 3, windowDays: 90, gamingFlagsCaught: 2 }, + fleetAccuracy: { + accuracyPct: 80, + coveragePct: 64.3, + instanceCount: 3, + windowDays: 90, + gamingFlagsCaught: 2, + }, }, }); renderWithClient(); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 3eed1e71d0..ad9543e0d7 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -5077,9 +5077,18 @@ export async function getLatestAdvisoryForPullRequest(env: Env, repoFullName: st * sweep tick, while a stale non-cacheable row still correctly falls through to a fresh call. * * A PUBLISHED row (`published_at` set by markAiReviewPublished, once the review actually reached the PR) skips - * the `maxAgeMs` staleness check entirely: the cooldown exists to bound reuse BEFORE the first publish (e.g. two + * the `maxAgeMs` staleness check: the cooldown exists to bound reuse BEFORE the first publish (e.g. two * overlapping sweep passes racing the same head), not to force a periodic re-run of an already-surfaced verdict - * for the SAME head+fingerprint — see the migration's doc comment for the production incident this closes. */ + * for the SAME head+fingerprint — see the migration's doc comment for the production incident this closes. + * + * #9019: that publish exemption does NOT extend to a row whose own verdict was INCONCLUSIVE + * (`metadata.inconclusive` — a provider outage, a consensus-disputed roll). `cacheable=0` conflates two + * unrelated things: a dynamic review context (grounding/RAG), where the verdict is conclusive but simply not + * durable across time, and a genuinely inconclusive verdict. Only the latter must stay retryable: without this, + * a transient outage verdict was FINAL for that head once surfaced — the bot never retried, contradicting the + * finding's own "re-evaluates on the next update" text, while a green PR gave the contributor no reason to push + * the commit that would force one. The #2119 anti-churn incident the exemption exists for concerned + * dynamic-context rows, which keep it. */ export async function getCachedAiReview( env: Env, repoFullName: string, @@ -5095,14 +5104,15 @@ export async function getCachedAiReview( .bind(repoFullName, pullNumber, headSha) .first<{ notes: string; reviewerCount: number; mode: string; findingsJson: string | null; metadataJson: string | null; cacheable: number; publishedAt: string | null; createdAt: string }>(); if (!row || row.mode !== mode) return null; + const metadata = parseJson>(row.metadataJson, {}); if (row.cacheable !== 1) { if (!options?.allowNonCacheable) return null; - if (row.publishedAt == null) { + // Published rows skip the cooldown UNLESS the verdict itself was inconclusive (#9019, see above). + if (row.publishedAt == null || metadata.inconclusive === true) { const ageMs = Date.now() - Date.parse(row.createdAt); if (!Number.isFinite(ageMs) || ageMs < 0 || ageMs > (options.maxAgeMs ?? 0)) return null; } } - const metadata = parseJson>(row.metadataJson, {}); if ( expectedInputFingerprint !== undefined && metadata.inputFingerprint !== expectedInputFingerprint @@ -5179,22 +5189,38 @@ export async function getLatestPublishedAiReview( repoFullName: string, pullNumber: number, mode: string, + // #9019: `conclusiveOnly` skips rows whose own verdict was INCONCLUSIVE (`metadata.inconclusive` -- a + // provider outage, a consensus-disputed roll, a lock-contention placeholder). The one-shot cadence caller + // passes it because this lookup is head-AGNOSTIC by design: without it, one inconclusive verdict permanently + // pinned that PR across ALL future heads -- unlike every other cadence, a contributor pushing new code could + // not escape it, since the reused row was never keyed to their old commit. That PR never actually got its + // one real shot, so one-shot must not count it as spent. Deliberately NOT keyed on `cacheable`, which + // conflates this with a dynamic review context (a conclusive verdict that simply isn't durable across time, + // #2119) -- those stay reusable. The manual-review-freeze caller does not pass it: freezing intends to reuse + // whatever was last surfaced, conclusive or not, until a maintainer explicitly retriggers. + options?: { conclusiveOnly?: boolean } | undefined, ): Promise<{ notes: string; reviewerCount: number; findings: AdvisoryFinding[]; headSha?: string | undefined; metadata?: Record | undefined } | null> { - const row = await env.DB + // `inconclusive` lives in the metadata JSON blob, so the filter can't be a portable SQL predicate across + // both backends -- scan the few most-recent published rows and pick in JS. Bounded by the same small limit + // the sibling fingerprint scan uses; a PR accumulates only a handful of review rows over its lifetime. + const { results } = await env.DB .prepare( - "SELECT notes, reviewer_count AS reviewerCount, head_sha AS headSha, findings_json AS findingsJson, metadata_json AS metadataJson FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND ai_review_mode = ? AND published_at IS NOT NULL ORDER BY published_at DESC, rowid DESC LIMIT 1", + "SELECT notes, reviewer_count AS reviewerCount, head_sha AS headSha, findings_json AS findingsJson, metadata_json AS metadataJson FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND ai_review_mode = ? AND published_at IS NOT NULL ORDER BY published_at DESC, rowid DESC LIMIT ?", ) - .bind(repoFullName, pullNumber, mode) - .first<{ notes: string; reviewerCount: number; headSha: string; findingsJson: string | null; metadataJson: string | null }>(); - if (!row) return null; - const metadata = parseJson>(row.metadataJson, {}); - return { - notes: row.notes, - reviewerCount: row.reviewerCount, - findings: parseJson(row.findingsJson, []), - ...(row.headSha ? { headSha: row.headSha } : {}), - ...(Object.keys(metadata).length > 0 ? { metadata } : {}), - }; + .bind(repoFullName, pullNumber, mode, options?.conclusiveOnly ? AI_REVIEW_FINGERPRINT_SCAN_LIMIT : 1) + .all<{ notes: string; reviewerCount: number; headSha: string; findingsJson: string | null; metadataJson: string | null }>(); + for (const row of results ?? []) { + const metadata = parseJson>(row.metadataJson, {}); + if (options?.conclusiveOnly && metadata.inconclusive === true) continue; + return { + notes: row.notes, + reviewerCount: row.reviewerCount, + findings: parseJson(row.findingsJson, []), + ...(row.headSha ? { headSha: row.headSha } : {}), + ...(Object.keys(metadata).length > 0 ? { metadata } : {}), + }; + } + return null; } /** Count distinct PR head SHAs that already received a published AI review — used by diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0825d84b64..18d4c921a5 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2266,6 +2266,16 @@ function closeWithheldReason(args: { protectedAuthor: boolean; closeOwnerAuthors export function agentHoldAuditDetail(args: { planned: PlannedAgentAction[]; breakerOnPlan: PlannedAgentAction[]; + // #9040: each plan transform REPORTS whether it engaged, threaded in from the transform's own call site -- + // this function must never re-derive the cause from a planned-vs-final set difference. The old inference + // ("a terminal action was planned but is not in the final plan ⇒ the precision breaker did it") was + // provably wrong on the live fleet: `breakerOnPlan` was passed the POST-HOLDOUT plan, so every ε-holdout + // adjudication hold (#8831) was attributed to a breaker that had never engaged -- 6 of 6 live + // "precision circuit breaker" rows paired 1:1 (within 20ms) with decision_audit_holdout events, while + // system_flags contained no engaged breaker at all. A wrong reason is worse than none: an operator chases + // a breaker that never fired while the real cause sits one line above in the ledger. + precisionBreakerEngaged: boolean; + closeAuditHoldoutEngaged: boolean; gateConclusion: string; gateBlockerCodes: string[]; ciState: string; @@ -2279,10 +2289,20 @@ export function agentHoldAuditDetail(args: { mergeAutonomy: string; closeAutonomy: string; }): string { + // Specific, transform-reported causes always beat the residual inference below (#9040). Holdout first: + // it consumes the post-breaker plan, so when both somehow engaged in one pass the holdout is the transform + // that actually removed the final close. + if (args.closeAuditHoldoutEngaged) + return "auto-action held for close-audit adjudication (ε-holdout)"; + if (args.precisionBreakerEngaged) + return "auto-action held by precision circuit breaker"; + // Residual (should be unreachable while breaker+holdout are the only terminal-action-removing transforms): + // an HONEST generic reason for any future transform that removes a planned terminal action without + // reporting itself here -- never a false specific attribution. const plannedTerminalAction = args.planned.some((action) => action.actionClass === "merge" || action.actionClass === "close"); const finalTerminalAction = args.breakerOnPlan.some((action) => action.actionClass === "merge" || action.actionClass === "close"); if (plannedTerminalAction && !finalTerminalAction) - return "auto-action held by precision circuit breaker"; + return "auto-action held: a planned terminal action was removed before execution (unreported transform)"; if (args.ciHasPending || args.ciState === "pending") return "auto-action held because CI is still pending"; const protectedAuthor = args.authorIsAutomationBot || args.authorIsOwner || args.authorIsAdmin; @@ -2450,7 +2470,25 @@ async function maybeRunAgentMaintenance( // block); a pass that loses the race defers cleanly — the next webhook/sweep tick is the backstop. Prefers // the SubmissionLock Durable Object when bound; otherwise the transient-cache mutex in transient-locks.ts. const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); - if (!actuationLock.acquired) return; + if (!actuationLock.acquired) { + // #9025: this used to `return` silently -- the job completed "successfully", nothing re-queued the + // disposition, and no audit row recorded that a planned action was abandoned. That silently amplified + // every restart incident: the recovered job re-ran, republished a no-op surface, hit its own dead + // predecessor's orphaned actuation lock here, and lost the disposition a SECOND time with no trace. + // Throwing the retryable error instead (the exact pattern review-evasion.ts's withPrActuationLock + // established for this same condition) gets the queue's fast 5s-backoff retry, so the disposition lands + // once the contending pass (or the orphaned lock's boot-time flush, #9021) releases the PR. The audit row + // makes the contention itself visible even if every retry ultimately exhausts. + await recordAuditEvent(env, { + eventType: "github_app.agent_maintenance_lock_contended", + actor: null, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "queued", + detail: "Another pass holds this PR's actuation lock; the disposition retries instead of being dropped.", + metadata: { deliveryId: args.deliveryId, repoFullName }, + }).catch(() => undefined); + throw new PrActuationLockContendedError(repoFullName, pr.number, "agent-maintenance"); + } try { await runAgentMaintenancePlanAndExecute(env, { installationId, @@ -3427,9 +3465,16 @@ async function runAgentMaintenancePlanAndExecute( // by hand (#selfhost-holdplan-audit). const isContributorAuthor = !authorIsOwner && !authorIsAdmin && !authorIsAutomationBot; const closeEligible = isContributorAuthor || ((authorIsOwner || authorIsAdmin) && settings.closeOwnerAuthors === true); + // #9040: each transform's engagement is derived from ITS OWN before/after pair -- the breaker from + // (planned -> breakerOnPlan), the holdout from (breakerOnPlan -> holdoutOnPlan) -- and passed explicitly, + // so the audit function never guesses a cause from the combined end-to-end set difference again. + const closeAuditHoldoutEngaged = + breakerOnPlan.some((action) => action.actionClass === "close") && !holdoutOnPlan.some((action) => action.actionClass === "close"); const holdDetail = agentHoldAuditDetail({ planned, breakerOnPlan: holdoutOnPlan, + precisionBreakerEngaged: precisionBreakerDirections.length > 0, + closeAuditHoldoutEngaged, gateConclusion: gate.conclusion, gateBlockerCodes, ciState: ciAggregate.ciState, @@ -3856,6 +3901,10 @@ export async function reReviewStoredPullRequest( liveFacts, }), ).catch((error) => { + // #9025: rate-limit / retryable errors (chiefly PrActuationLockContendedError from the maintenance + // lock claim) MUST reach the queue so the disposition retries instead of being logged-and-dropped -- + // the same propagation contract the review pipeline's own catch sites already follow. + if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; console.error( JSON.stringify({ level: "error", @@ -6740,6 +6789,10 @@ async function handlePullRequestWebhookEvent( liveFacts, }), ).catch((error) => { + // #9025: same propagation contract as the sibling maintenance catch above -- a retryable error + // (chiefly the maintenance lock's own PrActuationLockContendedError) must reach the queue's retry + // path instead of being logged-and-dropped, or the disposition is silently lost. + if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; /* v8 ignore next -- best-effort: auto-maintain failures are logged, never surfaced to the gate. */ console.error( JSON.stringify({ @@ -10005,9 +10058,13 @@ async function maybePublishPrPublicSurface( // a PR that's blacklisted/frozen/already-skipped for another reason never shows AI content at all today, // and one-shot mode must not change that. A non-null result here means this PR already had its one-shot // main-review pass, so the fresh call below must be skipped and this reused instead. + // #9019: `conclusiveOnly` -- this lookup is head-AGNOSTIC, so without it a non-cacheable row (a provider + // outage, an inconclusive/disputed roll, a lock-contention placeholder) pinned the PR's verdict across + // every future head, and a contributor pushing new code could not escape it. That PR never got its one + // real shot, so it must not count as spent; the next pass/push retries instead of replaying the outage. const oneShotPriorReview = oneShotCadenceActive && !authorBlacklisted && !isFrozenForManualReview && !autoReviewSkipReason - ? await getLatestPublishedAiReview(env, repoFullName, pr.number, settings.aiReviewMode).catch(() => null) + ? await getLatestPublishedAiReview(env, repoFullName, pr.number, settings.aiReviewMode, { conclusiveOnly: true }).catch(() => null) : null; const aiReviewWillRun = !authorBlacklisted && @@ -10599,6 +10656,13 @@ async function maybePublishPrPublicSurface( /* v8 ignore next -- runAiReviewForAdvisory (the sole path reaching here) always sets metadata on its "ok" returns; the nullish fallback is a type-level (optional field) safeguard, not a reachable runtime path. */ ...(aiReview.metadata ?? {}), inputFingerprint, + // #9019: `cacheable=0` conflates TWO independent, unrelated reasons -- (a) a dynamic review + // context (grounding/RAG), where the verdict itself is perfectly CONCLUSIVE but simply not + // durable across time, and (b) the review's own verdict being inconclusive/consensus- + // disputed. Only (b) should be retried; (a) is correctly reused once published (#2119). + // Recording the review's OWN verdict here keeps the two separable at read time without + // needing a schema migration, since `cacheable` alone can no longer tell them apart. + inconclusive: aiReview.cacheable === false, // Persist line-anchored findings for post-submission MCP readback (#4519). Inline comments // themselves are still only posted on a fresh review (see inlineFindings hoisting above); // this metadata is read-only structured output, not a cache-replay trigger. diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index b27dd9458b..eaf10a7b91 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -338,6 +338,40 @@ describe("AI review cache (#1)", () => { } }); + // #9019: `cacheable=0` conflates a DYNAMIC-CONTEXT verdict (conclusive, just not durable across time -- + // the row above) with a genuinely INCONCLUSIVE one (a provider outage, a consensus-disputed roll). Only the + // latter must stay retryable: without this, a transient outage verdict became FINAL for that head the + // moment it published -- the bot never retried, contradicting the finding's own "re-evaluates on the next + // update" text, while a green PR gave the contributor no reason to push the commit that would force one. + it("#9019: a published INCONCLUSIVE row still expires with the cooldown, so the stuck verdict is retried", async () => { + const env = createTestEnv(); + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date("2026-07-01T00:00:00.000Z")); + await putCachedAiReview(env, "o/r", 41, "sha1", "block", { + notes: "provider outage", + reviewerCount: 0, + cacheable: false, + metadata: { inconclusive: true }, + }); + await markAiReviewPublished(env, "o/r", 41, "sha1"); + + // Inside the cooldown the published row is still reused -- the retry stays bounded to one per window. + vi.setSystemTime(new Date("2026-07-01T00:10:00.000Z")); + expect( + await getCachedAiReview(env, "o/r", 41, "sha1", "block", undefined, { allowNonCacheable: true, maxAgeMs: 30 * 60 * 1000 }), + ).toMatchObject({ notes: "provider outage" }); + + // Past the cooldown it MISSES, so the next pass spends a fresh attempt instead of replaying the outage. + vi.setSystemTime(new Date("2026-07-01T05:00:00.000Z")); + expect( + await getCachedAiReview(env, "o/r", 41, "sha1", "block", undefined, { allowNonCacheable: true, maxAgeMs: 30 * 60 * 1000 }), + ).toBeNull(); + } finally { + vi.useRealTimers(); + } + }); + it("still misses an UNPUBLISHED non-cacheable row past the cooldown (unchanged behavior)", async () => { const env = createTestEnv(); vi.useFakeTimers(); @@ -505,6 +539,46 @@ describe("AI review cache (#1)", () => { .run(); expect(await getLatestPublishedAiReview(env, "o/r", 55, "block")).toEqual({ notes: "empty head", reviewerCount: 1, findings: [] }); }); + + // #9019: this lookup is head-AGNOSTIC by design, so an INCONCLUSIVE row it returns pins the PR's verdict + // across ALL future heads -- a contributor pushing new code cannot escape it, because the reused row was + // never keyed to their old commit. The one-shot cadence caller passes conclusiveOnly so that PR (which + // never actually got its one real shot) is not counted as spent. The freeze caller does not pass it. + it("#9019: conclusiveOnly skips an inconclusive published row and falls through to the newest conclusive one", async () => { + const env = createTestEnv(); + await putCachedAiReview(env, "o/r", 57, "sha1", "block", { + notes: "real verdict", + reviewerCount: 2, + metadata: { inconclusive: false }, + }); + await markAiReviewPublished(env, "o/r", 57, "sha1"); + await putCachedAiReview(env, "o/r", 57, "sha2", "block", { + notes: "provider outage", + reviewerCount: 0, + cacheable: false, + metadata: { inconclusive: true }, + }); + await markAiReviewPublished(env, "o/r", 57, "sha2"); + + // Without the flag (the manual-review-freeze caller) the NEWEST published row wins, inconclusive or not. + expect(await getLatestPublishedAiReview(env, "o/r", 57, "block")).toMatchObject({ notes: "provider outage" }); + // With it (the one-shot caller) the outage row is skipped in favor of the genuine prior verdict. + expect(await getLatestPublishedAiReview(env, "o/r", 57, "block", { conclusiveOnly: true })).toMatchObject({ notes: "real verdict" }); + }); + + it("#9019: conclusiveOnly returns null when EVERY published row is inconclusive — the PR never got its one shot", async () => { + const env = createTestEnv(); + await putCachedAiReview(env, "o/r", 58, "sha1", "block", { + notes: "provider outage", + reviewerCount: 0, + cacheable: false, + metadata: { inconclusive: true }, + }); + await markAiReviewPublished(env, "o/r", 58, "sha1"); + + expect(await getLatestPublishedAiReview(env, "o/r", 58, "block")).toMatchObject({ notes: "provider outage" }); + expect(await getLatestPublishedAiReview(env, "o/r", 58, "block", { conclusiveOnly: true })).toBeNull(); + }); }); describe("countPublishedAiReviewHeads — auto_pause_after_reviewed_commits (#2042)", () => { diff --git a/test/unit/precision-breakers-chain.test.ts b/test/unit/precision-breakers-chain.test.ts index a578a4c824..d15dfe5e6f 100644 --- a/test/unit/precision-breakers-chain.test.ts +++ b/test/unit/precision-breakers-chain.test.ts @@ -140,6 +140,9 @@ describe("agentHoldAuditDetail — durable why-no-action audit reason", () => { const base = { planned: [] as PlannedAgentAction[], breakerOnPlan: [] as PlannedAgentAction[], + // #9040: both transforms now REPORT their own engagement rather than being inferred from a set difference. + precisionBreakerEngaged: false, + closeAuditHoldoutEngaged: false, gateConclusion: "success", gateBlockerCodes: [] as string[], ciState: "passed", @@ -155,16 +158,40 @@ describe("agentHoldAuditDetail — durable why-no-action audit reason", () => { }; it("records that a precision breaker removed the planned terminal action", () => { - expect(agentHoldAuditDetail({ ...base, planned: [mergeAction] })).toBe("auto-action held by precision circuit breaker"); + expect(agentHoldAuditDetail({ ...base, precisionBreakerEngaged: true, planned: [mergeAction] })).toBe("auto-action held by precision circuit breaker"); expect( agentHoldAuditDetail({ ...base, + precisionBreakerEngaged: true, planned: [readyLabel, mergeAction], breakerOnPlan: [{ actionClass: "label", requiresApproval: false, reason: "held", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "add" }], }), ).toBe("auto-action held by precision circuit breaker"); }); + // #9040: the ε-holdout (#8831) and the precision breaker are DIFFERENT causes that both remove a planned + // terminal action. The old code inferred "breaker" purely from `planned` having a terminal action that + // `breakerOnPlan` lacks -- but the call site passes the POST-holdout plan, so every holdout hold was + // misattributed to a breaker that had never engaged (6/6 live rows paired 1:1 with decision_audit_holdout + // events while system_flags contained no engaged breaker at all). + it("#9040: attributes an ε-holdout hold to the holdout, never to the precision breaker", () => { + expect( + agentHoldAuditDetail({ ...base, closeAuditHoldoutEngaged: true, planned: [heuristicClose] }), + ).toBe("auto-action held for close-audit adjudication (ε-holdout)"); + }); + + it("#9040: the holdout wins when both transforms engaged in one pass (it consumes the post-breaker plan)", () => { + expect( + agentHoldAuditDetail({ ...base, precisionBreakerEngaged: true, closeAuditHoldoutEngaged: true, planned: [heuristicClose] }), + ).toBe("auto-action held for close-audit adjudication (ε-holdout)"); + }); + + it("#9040: a terminal action removed with NO transform reporting itself gets an honest generic reason, never a false breaker attribution", () => { + expect(agentHoldAuditDetail({ ...base, planned: [mergeAction] })).toBe( + "auto-action held: a planned terminal action was removed before execution (unreported transform)", + ); + }); + it("records pending CI ahead of mergeability or gate-policy guesses", () => { expect(agentHoldAuditDetail({ ...base, ciState: "pending" })).toBe("auto-action held because CI is still pending"); expect(agentHoldAuditDetail({ ...base, ciHasPending: true })).toBe("auto-action held because CI is still pending"); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 0633211e59..906fa84025 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -6,6 +6,7 @@ import { PR_PANEL_COMMENT_MARKER } from "../../src/github/comments"; import * as backfillModule from "../../src/github/backfill"; import * as rateLimitModule from "../../src/github/rate-limit"; import * as repositoriesModule from "../../src/db/repositories"; +import * as decisionRecordModule from "../../src/review/decision-record"; import * as reviewEffortModule from "../../src/review/review-effort"; import * as repositorySettingsModule from "../../src/settings/repository-settings"; import * as posthogModule from "../../src/selfhost/posthog"; @@ -3227,12 +3228,76 @@ describe("queue processors", () => { // review round 4) — one shared namespace, not a maintenance-only lock. await env.SELFHOST_TRANSIENT_CACHE?.set("pr-actuation-lock:owner/agent-repo#7", "1", 60); - await processJob(env, { type: "agent-regate-pr", deliveryId: "race-sweep", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + // #9025: the contended pass RETRIES rather than returning silently. It used to `return`, which completed + // the job "successfully" while dropping the disposition entirely -- a silent amplifier for every restart + // incident (the recovered job re-ran, hit its own dead predecessor's orphaned lock, and lost the + // disposition a second time with no trace). Throwing the retryable error is the same contract + // review-evasion.ts's withPrActuationLock already used for this exact condition; the queue honors its + // retryAfterMs via consumingRetryDelayMs, so the disposition lands once the contending pass releases. + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "race-sweep", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }), + ).rejects.toMatchObject({ name: "PrActuationLockContendedError", retryKind: "pr_actuation_lock_contended" }); - // The held lock made this pass skip its plan-and-execute critical section entirely — no mutation attempted. + // The held lock still made this pass skip its plan-and-execute critical section entirely — the retry is + // about not LOSING the disposition, never about mutating while another pass owns the PR. expect(mergeCalls).toBe(0); const actionAudits = await env.DB.prepare("select count(*) as n from audit_events where event_type like 'agent.action.%'").first<{ n: number }>(); expect(actionAudits?.n).toBe(0); + // ...and the contention itself is now visible in the ledger instead of leaving no trace at all. + const contendedAudit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ? and target_key = ?") + .bind("github_app.agent_maintenance_lock_contended", "owner/agent-repo#7") + .first<{ outcome: string; detail: string }>(); + expect(contendedAudit?.outcome).toBe("queued"); + + // #9025: the contention audit write is best-effort -- a failing write must never swallow or replace the + // retry itself. The disposition's durability cannot depend on the ledger being writable. + const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; + const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { + if (event.eventType === "github_app.agent_maintenance_lock_contended") throw new Error("audit DB down"); + await originalRecordAuditEvent(auditEnv, event); + }); + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "race-sweep-audit-down", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }), + ).rejects.toMatchObject({ name: "PrActuationLockContendedError" }); + auditSpy.mockRestore(); + }); + + it("#9025: a NON-retryable maintenance failure is still swallowed and logged, never propagated to the queue", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); + await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off", reviewCheckMode: "required" } }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 8, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a8" }, labels: [], body: "Closes #1" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/8/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/8/reviews") && init?.method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/8/reviews")) return Response.json([]); + if (/\/pulls\/8(\?|$)/.test(url)) return Response.json({ number: 8, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a8" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a8/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a8/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + // A plain (non-retryable) failure from inside the maintenance pass: the guard's FALSE arm must still + // swallow-and-log exactly as before #9025, so an ordinary bug never dead-letters a whole webhook job. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + // persistDecisionRecord is awaited UNCAUGHT and called ONLY inside runAgentMaintenancePlanAndExecute, so a + // rejection here surfaces exactly where a real bug in the maintenance critical section would. + const capSpy = vi.spyOn(decisionRecordModule, "persistDecisionRecord").mockRejectedValue(new Error("plain non-retryable failure")); + + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "maintenance-plain-failure", repoFullName: "owner/agent-repo", prNumber: 8, installationId: 9001 }), + ).resolves.toBeUndefined(); + + capSpy.mockRestore(); + expect(errorSpy.mock.calls.some((call) => String(call[0]).includes("agent_maintenance_failed"))).toBe(true); + errorSpy.mockRestore(); }); it("the sweep stamps the marker INLINE when the repo has no installation (audit-only, still converges) (#audit-sweep-fanout)", async () => { diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 456d6b1f33..19a1de8968 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -3082,17 +3082,23 @@ describe("queue processors", () => { return Response.json({}); }); - await processJob(env, { - type: "github-webhook", - deliveryId: "contributor-cap-pr-lock-contended", - eventName: "pull_request", - payload: { - action: "opened", - installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, - repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, - pull_request: { number: 56, title: "Farmer's 3rd PR", state: "open", user: { login: "farmer99" }, head: { sha: "f56" }, labels: [], body: "x", mergeable_state: "clean", reviewDecision: "APPROVED" }, - }, - }); + // #9025: the trailing maintenance pass hits the SAME still-held actuation lock and now throws the + // retryable contention error rather than returning silently, so the disposition retries instead of being + // dropped. The early-cap short-circuit under test here is a different call site and still defers cleanly + // (its own `return false`, unchanged) -- both assertions below are exactly as before. + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "contributor-cap-pr-lock-contended", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 56, title: "Farmer's 3rd PR", state: "open", user: { login: "farmer99" }, head: { sha: "f56" }, labels: [], body: "x", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }), + ).rejects.toMatchObject({ name: "PrActuationLockContendedError" }); // The early close DEFERRED (no PATCH close fired from it) and the pipeline fell through, mirroring the // author-lock contention semantics one namespace over. diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 3e9372c936..1cbdabc9ca 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6251,14 +6251,17 @@ describe("queue processors", () => { expect(missAudit?.n).toBe(1); // only the genuine first-run miss — the forced pass is NOT double-counted here }); - it("#9: a low-activity repo's old open PR NEVER generates a repeated AI review across many sweep ticks once published (#regate-churn)", async () => { - // Superseded policy note: this used to assert a BOUNDED, periodic retry (one fresh attempt per tick once - // AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS elapsed) for a never-durably-cacheable (still-inconclusive) - // outcome. That was itself the incident-mitigation for #1462, but it was still an UNBOUNDED total spend - // over a PR's lifetime (one fresh call every cooldown window, forever, for as long as the PR stayed open - // and inconclusive). The `published_at` marker (this PR) makes ANY review — cacheable or not — immutable - // for its exact head+fingerprint the moment it is actually published: a tick past the cooldown no longer - // buys a fresh attempt at all; only a real content/config change or an explicit maintainer force-rerun does. + it("#9019: a published-but-INCONCLUSIVE review still retries once per cooldown window, but never within one", async () => { + // Policy history, twice revised. Originally: a bounded periodic retry (one fresh attempt per tick once + // AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS elapsed) for a never-durably-cacheable outcome (#1462). + // Then the `published_at` marker made ANY review — cacheable or not — immutable for its exact + // head+fingerprint the moment it published, to bound lifetime spend (#regate-churn). + // #9019 narrows that second change back: immutability is right for a CONCLUSIVE verdict (including a + // dynamic-context one, covered by the sibling grounding test), but wrong for an INCONCLUSIVE one. A + // provider outage or a consensus-disputed roll would otherwise be FINAL for that head forever — the bot + // never retried, directly contradicting the finding's own "re-evaluates on the next update" text, while a + // green PR gave the contributor no reason to push the commit that would force one. The cooldown still + // bounds the spend to at most one attempt per window; only genuinely inconclusive rows are eligible. let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), @@ -6291,23 +6294,27 @@ describe("queue processors", () => { const callsPerAttempt = aiCalls; expect(callsPerAttempt).toBeGreaterThan(0); - // 5 more sweep ticks over a ~10-hour span (the production incident's 6h window had 97 sweep events for one - // repo), each beyond what used to be the 30-minute cooldown — the unchanged PR's published review is now - // reused indefinitely, so NONE of these buy a fresh attempt. + // 5 more sweep ticks over a ~10-hour span, each well BEYOND the 30-minute cooldown. Because the verdict + // is inconclusive, each buys exactly ONE fresh attempt (#9019) -- the stuck PR keeps getting a real + // chance to resolve instead of replaying the outage forever. const tickTimes = ["03:50:00", "05:40:00", "07:30:00", "09:20:00", "11:10:00"]; for (const [index, time] of tickTimes.entries()) { vi.setSystemTime(new Date(`2026-05-28T${time}.000Z`)); await processJob(env, { type: "agent-regate-pr", deliveryId: `low-activity-${index}`, repoFullName: "JSONbored/gittensory", prNumber: 65, installationId: 123 }); } - expect(aiCalls).toBe(callsPerAttempt); // still just the one, original attempt - - // Tighten four more ticks to well INSIDE what used to be the cooldown window — still zero additional spend. - const tightTicks = ["12:00:00", "12:05:00", "12:10:00", "12:15:00"]; + expect(aiCalls).toBe(callsPerAttempt * (1 + tickTimes.length)); + + // Four more ticks well INSIDE one cooldown window buy NOTHING -- the retry stays strictly bounded to one + // attempt per window, which is what keeps lifetime spend finite. This is the half of #regate-churn's + // guarantee that #9019 deliberately preserves. Timed against the LAST attempt above (11:10), not the + // original baseline: every one of these lands inside its 30-minute window. + const callsBeforeTightTicks = aiCalls; + const tightTicks = ["11:20:00", "11:25:00", "11:30:00", "11:35:00"]; for (const [index, time] of tightTicks.entries()) { vi.setSystemTime(new Date(`2026-05-28T${time}.000Z`)); await processJob(env, { type: "agent-regate-pr", deliveryId: `low-activity-tight-${index}`, repoFullName: "JSONbored/gittensory", prNumber: 65, installationId: 123 }); } - expect(aiCalls).toBe(callsPerAttempt); + expect(aiCalls).toBe(callsBeforeTightTicks); }); describe("#regate-churn: production reproductions (#3379, #3383) and the maintainer-gated freeze", () => {