From ada7d2e5b4430748e337a88a6468a81f866f4ed6 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 12:34:33 -0700 Subject: [PATCH 1/2] fix(orb): retry the disposition on lock contention, retry inconclusive verdicts, and stop misattributing holdout holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three disposition-integrity fixes: #9025 -- maybeRunAgentMaintenance returned silently when it lost the per-PR actuation lock: 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, hit its own dead predecessor's orphaned lock, and lost the disposition a SECOND time with no trace at all. Now throws PrActuationLockContendedError (the same contract review-evasion.ts's withPrActuationLock already used for this exact condition; the queue honors its 5s retryAfterMs via consumingRetryDelayMs) and records a named audit event. Both maintenance call sites' catch handlers now re-throw retryable/rate-limit errors instead of logging-and-dropping them, matching the review pipeline's own propagation contract; a plain non-retryable failure is still swallowed and logged. #9019 -- `cacheable=0` conflates two unrelated things: a DYNAMIC review context (grounding/RAG), where the verdict is conclusive but not durable across time, and a genuinely INCONCLUSIVE verdict (a provider outage, a consensus-disputed roll). Because published rows were exempt from the non-cacheable cooldown, a transient outage verdict became FINAL for that head the moment it surfaced -- 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. Worse, the head-AGNOSTIC one-shot lookup pinned that same outage verdict across ALL future heads, so a contributor pushing new code could not escape it either. Records the review's own verdict as metadata.inconclusive and keys both behaviors on it: the publish exemption still applies to dynamic-context rows (#2119 unchanged) but not to inconclusive ones, and the one-shot cadence skips inconclusive rows entirely -- that PR never got its one real shot. The cooldown still bounds retries to at most one attempt per window. #9040 -- every "auto-action held by precision circuit breaker" audit row was wrong. agentHoldAuditDetail inferred the breaker purely from "a terminal action was planned but is not in the final plan", but the call site passes the POST-HOLDOUT plan, so every ε-holdout adjudication hold (#8831) was attributed to a breaker that had never engaged -- 6 of 6 live rows paired 1:1 (within 20ms) with decision_audit_holdout events while system_flags contained no engaged breaker at all. Each transform now REPORTS its own engagement, derived from its own before/after pair, and the holdout gets its own reason string; the residual set-difference case returns an honest generic reason instead of a false specific attribution. Closes #9019 Closes #9025 Closes #9040 --- src/db/repositories.ts | 60 +++++++++++++----- src/queue/processors.ts | 70 +++++++++++++++++++- test/unit/ai-review-cache.test.ts | 74 ++++++++++++++++++++++ test/unit/precision-breakers-chain.test.ts | 29 ++++++++- test/unit/queue-2.test.ts | 69 +++++++++++++++++++- test/unit/queue-3.test.ts | 28 ++++---- test/unit/queue.test.ts | 39 +++++++----- 7 files changed, 319 insertions(+), 50 deletions(-) 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", () => { From f9188bdc9eac5156eea8efe246b4b62c3ab51b31 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 12:42:42 -0700 Subject: [PATCH 2/2] fix(ci): repair two pre-existing main breakages the drift/format gates enforce Both of these are broken on main right now and fail every PR branched from it, including this one: 1. .release-please-manifest.json still pinned packages/loopover-engine at 3.15.0 while its package.json says 3.15.1 -- the manual bump in #9107 (which the engine twin-parity guard required) never synced the release manifest, so release-manifest:sync:check has failed on every commit since. Regenerated via `npm run release-manifest:sync`. 2. apps/loopover-ui's prettier gate failed on two files last touched by #8848 (proof-of-power-stats-model.ts and proof-of-power-stats.test.tsx) -- two over-long object literals prettier wants wrapped. Applied `prettier --write`; pure formatting, no behavior change. --- .release-please-manifest.json | 2 +- .../src/components/site/proof-of-power-stats-model.ts | 5 ++++- .../src/components/site/proof-of-power-stats.test.tsx | 8 +++++++- 3 files changed, 12 insertions(+), 3 deletions(-) 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();