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/github/backfill.ts b/src/github/backfill.ts index a57ce9b5ab..68eb09632f 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -72,7 +72,7 @@ import { LOOPOVER_GATE_CHECK_NAME, shouldPublishReviewCheck, } from "../review/check-names"; -import { buildReviewThreadBlocker, type ReviewThreadBlocker } from "../review/review-thread-findings"; +import { buildReviewThreadBlocker, unreadableReviewThreadBlocker, type ReviewThreadBlocker } from "../review/review-thread-findings"; import { delayUntil, HISTORICAL_BACKFILL_RESERVED_HEADROOM, LOW_REST_RATE_LIMIT_REMAINING, shouldWaitForGitHubRateLimit } from "./rate-limit"; import { githubRateLimitAdmissionKeyForPublicToken, @@ -3109,6 +3109,14 @@ export async function fetchLiveCiAggregate( } checkRuns.push(...(result.data.check_runs ?? [])); if (!hasNextPage(result.link)) break; + // #9051: exhausting the page cap with `rel="next"` STILL present leaves the set partially read -- exactly + // the state the failed-fetch branch above already fails closed on, and it must behave identically. Before + // this, the loop simply exited with checkRunsIncomplete=false, so reduceLiveCiAggregate treated a truncated + // set as complete: a red check on page 11+ was invisible, ciState resolved to "passed", the planner saw + // reviewGood, and the PR MERGED. The executor's act-boundary recheck calls this same function, so it + // reproduced the same wrong verdict rather than catching it, and the false "passed" was then persisted into + // the durable cross-job CI cache. Every other capped loop in this file already signals at its cap. + if (page === PR_DETAIL_MAX_PAGES) checkRunsIncomplete = true; } // The combined status endpoint caps at 100/page, so accumulate every page before the reducer processes them. const statuses: LiveCiStatus[] = []; @@ -3127,6 +3135,9 @@ export async function fetchLiveCiAggregate( } statuses.push(...(statusResult.data.statuses ?? [])); if (!hasNextPage(statusResult.link)) break; + // #9051: same cap-exhaustion hole as the check-runs loop above — a failing classic status past the cap + // would otherwise be invisible and read as "passed". + if (page === PR_DETAIL_MAX_PAGES) statusIncomplete = true; } return reduceLiveCiAggregate(env, { checkRuns, @@ -3138,14 +3149,27 @@ export async function fetchLiveCiAggregate( // Lazily read the check-SUITES backstop only when the reducer finds the cheaper sources fully settled; a fetch // error returns null so the reducer fails closed exactly as the inline path did. fetchSuites: async () => { - const suitesResult = await githubJsonWithHeaders<{ check_suites?: Array }>( - env, - repoFullName, - `/commits/${headSha}/check-suites?per_page=100`, - token, - githubRateLimitOptions(admissionKey), - ).catch(() => undefined); - return suitesResult ? (suitesResult.data.check_suites ?? []) : null; + // #9051: this read used to be page-1-only with no Link follow, despite being the LAST gate before a + // commit is certified settled -- a first-party suite still running on page 2 was invisible, so anyPending + // was never set and the aggregate stayed "passed". Paginated like its check-runs/status siblings, and + // returning null (which the reducer already fails closed on) if the cap is exhausted with pages left. + const suites: LiveCiSuite[] = []; + for (let page = 1; page <= PR_DETAIL_MAX_PAGES; page += 1) { + const suitesResult = await githubJsonWithHeaders<{ check_suites?: Array }>( + env, + repoFullName, + `/commits/${headSha}/check-suites?per_page=100&page=${page}`, + token, + githubRateLimitOptions(admissionKey), + ).catch(() => undefined); + if (!suitesResult) return null; + suites.push(...(suitesResult.data.check_suites ?? [])); + if (!hasNextPage(suitesResult.link)) return suites; + if (page === PR_DETAIL_MAX_PAGES) return null; + } + /* v8 ignore next -- unreachable: the loop always returns on the last iteration (either the no-next-page + * or the cap-exhausted branch above); this satisfies the compiler's return-path analysis. */ + return null; }, }); } @@ -3177,7 +3201,7 @@ export async function fetchLiveCiAggregateViaGraphQl( if (!headSha || !token) return null; const [owner, name] = repoFullName.split("/"); if (!owner || !name) return null; - const query = `query LoopOverLiveCiRollup { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { object(oid: ${JSON.stringify(headSha)}) { ... on Commit { statusCheckRollup { contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status startedAt detailsUrl title summary checkSuite { databaseId app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } } } } } }`; + const query = `query LoopOverLiveCiRollup { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { object(oid: ${JSON.stringify(headSha)}) { ... on Commit { statusCheckRollup { contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status startedAt detailsUrl title summary checkSuite { databaseId app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } pageInfo { hasNextPage } } } } } }`; const result = await githubGraphQl<{ data?: { repository?: { @@ -3202,7 +3226,7 @@ export async function fetchLiveCiAggregateViaGraphQl( pageInfo?: { hasNextPage?: boolean }; } | null; } | null; - checkSuites?: { nodes?: Array<{ status?: string | null; app?: { slug?: string | null } | null }> }; + checkSuites?: { nodes?: Array<{ status?: string | null; app?: { slug?: string | null } | null }>; pageInfo?: { hasNextPage?: boolean } }; } | null; } | null; }; @@ -3225,6 +3249,10 @@ export async function fetchLiveCiAggregateViaGraphQl( // must carry a well-formed `contexts.nodes` array; a present-but-malformed connection → fall back to REST. if (rollup && !Array.isArray(contexts?.nodes)) return null; if (contexts?.pageInfo?.hasNextPage) return null; // >100 contexts: not fully enumerated → let REST paginate + // #9051: checkSuites had no such guard, unlike its `contexts` sibling one line up. The suites list is the + // settled-commit backstop, so a truncated read (>100 suites) could hide a still-running first-party suite + // and certify the commit as settled. Fall back to REST, which now paginates this too. + if (commit.checkSuites?.pageInfo?.hasNextPage) return null; const checkRuns: LiveCiCheckRun[] = []; const statuses: LiveCiStatus[] = []; for (const node of contexts?.nodes ?? []) { @@ -3913,6 +3941,14 @@ export async function fetchOpenPullRequestNumbersForCommit( return [...new Set(numbers)]; } +/** #9052: returned by {@link fetchLivePullRequestReviewDecision} when GitHub answered 200 but with a top-level + * `errors` array, i.e. the decision could not actually be read. Distinct from `undefined` ("read fine, the PR + * has no decision yet"), because only a real value survives the caller's `?? pr.reviewDecision` fallback -- + * which is what stops a stale stored APPROVED from standing in for a read we know failed. It equals no real + * GitHub reviewDecision enum value, so every `=== "APPROVED"` / `=== "CHANGES_REQUESTED"` comparison is + * correctly false; call sites that must fail closed on uncertainty check it by name. */ +export const REVIEW_DECISION_UNREADABLE = "__loopover_review_decision_unreadable__"; + /** RC1 (idempotent reviews): the PR's LIVE reviewDecision (APPROVED / CHANGES_REQUESTED / REVIEW_REQUIRED) via * GraphQL. The STORED reviewDecision is only written by the open-PR backfill and goes stale, so the action * planner's approve/request-changes dedup was blind and re-posted a review every cycle — the re-review loop. @@ -3929,12 +3965,19 @@ export async function fetchLivePullRequestReviewDecision( const [owner, name] = repoFullName.split("/"); if (!owner || !name) return undefined; const query = `query { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { pullRequest(number: ${prNumber}) { reviewDecision } } }`; - const result = await githubGraphQl<{ data?: { repository?: { pullRequest?: { reviewDecision?: string | null } | null } | null } }>( + const result = await githubGraphQl<{ data?: { repository?: { pullRequest?: { reviewDecision?: string | null } | null } | null }; errors?: unknown[] }>( env, query, token, admissionKey, ).catch(() => undefined); + // #9052: a 200 carrying a top-level `errors` array is a PARTIAL result -- `reviewDecision` comes back null + // even when the PR genuinely has one. Returning plain `undefined` here made the caller fall back to the + // STORED decision (`liveReviewDecision ?? pr.reviewDecision`), so a PR approved at backfill time and later + // flipped to CHANGES_REQUESTED still read as APPROVED -> approvalsSatisfied -> merge over the objection. + // The sentinel is deliberately a VALUE rather than `undefined`: it must survive the `??` fallback so the + // stale stored decision can never be substituted for a read we know we could not complete. + if (Array.isArray(result?.errors) && result.errors.length > 0) return REVIEW_DECISION_UNREADABLE; return result?.data?.repository?.pullRequest?.reviewDecision ?? undefined; } @@ -3969,6 +4012,7 @@ type GitHubReviewThreadResponse = { } | null; } | null; }; + errors?: unknown[]; }; // Bound the reviewThreads GraphQL walk so a PR with a pathologically large number of review threads can't turn @@ -4037,6 +4081,15 @@ export async function fetchLiveReviewThreadBlockers( token, admissionKey, ).catch(() => undefined); + // #9052: GitHub's standard partial-failure shape under load is HTTP 200 with `reviewThreads: null` AND a + // top-level `errors` array. Without this check that yielded `[]` -- indistinguishable from a genuinely + // thread-free PR -- so a maintainer's unresolved blocking thread was silently dropped from the findings and + // the gate could conclude success and MERGE over the open objection. Unlike this function's fail-OPEN + // posture for a transport error (where nothing at all was read), a partial result means we know the answer + // is unreliable, so it fails CLOSED: a synthetic blocker holds the gate for a human. The sibling GraphQL + // readers in this file (fetchLiveCiAggregateViaGraphQl, fetchLinkedIssueClosedByPullRequest) already guard + // this; this reader was the outlier. + if (Array.isArray(result?.errors) && result.errors.length > 0) return [unreadableReviewThreadBlocker()]; const connection: GitHubReviewThreadConnection | null | undefined = result?.data?.repository?.pullRequest?.reviewThreads; if (!connection?.nodes) { if (threads.length === 0) return []; 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/src/review/review-thread-findings.ts b/src/review/review-thread-findings.ts index 42eb2edd95..5608b7404d 100644 --- a/src/review/review-thread-findings.ts +++ b/src/review/review-thread-findings.ts @@ -46,6 +46,19 @@ export function buildReviewThreadBlocker(input: { }; } +/** #9052: the blocker returned when GitHub answered 200 but with a top-level `errors` array, so the review-thread + * set could not actually be read. Fails CLOSED on purpose: an unreadable thread list is NOT evidence of zero + * threads, and treating it as such let an unresolved maintainer objection be merged over. `scannerFinding: false` + * keeps it out of the scanner-specific reporting paths — it is an infrastructure signal, not a code finding. */ +export function unreadableReviewThreadBlocker(): ReviewThreadBlocker { + return { + title: "review threads could not be read from GitHub (partial GraphQL response)", + path: null, + line: null, + scannerFinding: false, + }; +} + export function reviewThreadBlockerFinding(blocker: ReviewThreadBlocker): AdvisoryFinding { const location = reviewThreadLocation(blocker); const actor = blocker.authorLogin ? `${blocker.authorLogin} ` : ""; diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 57612b5f19..5bcd6287ac 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -6,7 +6,7 @@ import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent- import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly, readUntrustworthyRuleCodes } from "../review/outcomes-wire"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, mergeRequiredCiContexts } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, mergeRequiredCiContexts, REVIEW_DECISION_UNREADABLE } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -264,7 +264,15 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // default -- without this gate, a thread- or duplicate-only close would be wrongly superseded as if it were // conflict-justified merely because mergeability happens to read clean (the SAME over-broad-predicate class // the #2478 gate review already caught once for closeRequiresMergeableState). - const mergeableNowCleared = isMergeableRecheck && reviewFetchSucceeded && mergeableState === "clean" && reviewDecision !== "CHANGES_REQUESTED"; + // #9052: an UNREADABLE decision (200-with-errors) is not "confirmed no changes requested" -- without this + // it would satisfy the `!== "CHANGES_REQUESTED"` test and wrongly clear a conflict-justified close on a + // read we know failed, the same fail-open shape reviewFetchSucceeded already guards for a rejected fetch. + const mergeableNowCleared = + isMergeableRecheck && + reviewFetchSucceeded && + mergeableState === "clean" && + reviewDecision !== "CHANGES_REQUESTED" && + reviewDecision !== REVIEW_DECISION_UNREADABLE; // Only a CONFIRMED non-"open" clears a duplicate-justified close -- a rejected/failed fetch (undefined) // fails open exactly like every other live re-check in this function, so a transient GitHub hiccup never // wrongly spares a close that is, in fact, still justified. diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index adef8786c0..06c626ade4 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -63,6 +63,7 @@ import { createPullRequestReview, mergePullRequest } from "../../src/github/pr-a import { ensurePullRequestLabel } from "../../src/github/labels"; import { createInstallationToken } from "../../src/github/app"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts } from "../../src/github/backfill"; +import { REVIEW_DECISION_UNREADABLE } from "../../src/github/backfill"; import { resolveLinkedIssueHardRule } from "../../src/review/linked-issue-hard-rules"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, type AgentActionExecutionContext } from "../../src/services/agent-action-executor"; @@ -805,6 +806,35 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).not.toHaveBeenCalled(); }); + // #9052: an UNREADABLE live review decision (GitHub 200-with-errors) is not "confirmed no changes requested". + // Without the explicit sentinel check it satisfied `!== "CHANGES_REQUESTED"` and wrongly cleared a + // conflict-justified close on a read we know failed -- the same fail-open shape reviewFetchSucceeded already + // guards for a rejected fetch. The close must STAND (execute), not be superseded. + it("#9052: an unreadable live review decision does NOT clear a conflict-justified close", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: { closeComment: "base conflict", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true, expectedHeadSha: "h7" }, + reason: "base branch now conflicts", + }); + vi.mocked(fetchLivePullRequestReviewDecision).mockResolvedValueOnce(REVIEW_DECISION_UNREADABLE); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + // NOT superseded: the same setup with a readable decision supersedes (the #2478 test directly below). + expect(result.status).not.toBe("rejected"); + // toBeFalsy, not toBeNull: the D1 test double resolves a no-row .first() to undefined. + const audit = await env.DB.prepare("select detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ detail: string }>(); + expect(audit).toBeFalsy(); + }); + it("REGRESSION (#2478): accept supersedes a conflict-justified heuristic close when the conflict cleared", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); 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/backfill-2.test.ts b/test/unit/backfill-2.test.ts index 4e2a723cea..3c6d0b3996 100644 --- a/test/unit/backfill-2.test.ts +++ b/test/unit/backfill-2.test.ts @@ -41,6 +41,8 @@ import { fetchLiveBaseBranchAdvancedAt, fetchLiveCiAggregate, fetchLiveReviewThreadBlockers, + fetchLivePullRequestReviewDecision, + REVIEW_DECISION_UNREADABLE, fetchNamedCheckRunConclusion, fetchRequiredStatusContexts, isOwnReviewThreadAuthor, @@ -255,6 +257,87 @@ describe("GitHub backfill", () => { expect(fetchSpy).not.toHaveBeenCalled(); }); + // #9051: a FAILED page fetch already set checkRunsIncomplete, but exhausting the 10-page cap with + // `rel="next"` still present did not -- the loop just exited and the reducer treated a truncated set as + // complete. A red check on page 11+ was therefore invisible: ciState resolved to "passed", the planner saw + // reviewGood, and the PR MERGED. The executor's act-boundary recheck calls this same function, so it + // reproduced the wrong verdict rather than catching it, and "passed" was persisted to the durable CI cache. + it("#9051: exhausting the check-runs page cap with more pages left fails closed to pending, never passed", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + let checkRunPages = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) { + checkRunPages += 1; + // Every page reports another page after it — the cap is reached with `rel="next"` still present. + return Response.json( + { check_runs: [{ name: "green-check", status: "completed", conclusion: "success" }] }, + { headers: { link: '; rel="next"' } }, + ); + } + if (url.includes("/status?")) return Response.json({ statuses: [] }); + if (url.includes("/check-suites")) return Response.json({ check_suites: [] }); + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "capped", "public-token", null); + + expect(checkRunPages).toBe(10); // the cap itself still bounds the walk + // Every check READ was green, so without the fix this would report "passed". + expect(aggregate.ciState).toBe("pending"); + expect(aggregate.hasPending).toBe(true); + // A partial read must not be mistaken for a definitive missing-required-context verdict either. + expect(aggregate.hasMissingRequiredContext).toBe(false); + }); + + it("#9051: exhausting the classic-status page cap likewise fails closed to pending", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) return Response.json({ check_runs: [] }); + if (url.includes("/status?")) { + return Response.json( + { statuses: [{ context: "green-status", state: "success" }] }, + { headers: { link: '; rel="next"' } }, + ); + } + if (url.includes("/check-suites")) return Response.json({ check_suites: [] }); + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "capped-status", "public-token", null); + + expect(aggregate.ciState).toBe("pending"); + expect(aggregate.hasPending).toBe(true); + }); + + // #9051: the check-suites backstop is the LAST gate before a commit is certified settled, yet it read page 1 + // only with no Link follow -- a first-party suite still running on page 2 was invisible, so anyPending was + // never set and the aggregate stayed "passed". Now paginated; an exhausted cap returns null, which the + // reducer already fails closed on. + it("#9051: the check-suites backstop follows Link pages and fails closed when its cap is exhausted", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + let suitePages = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) return Response.json({ check_runs: [{ name: "lint", status: "completed", conclusion: "success" }] }); + if (url.includes("/status?")) return Response.json({ statuses: [] }); + if (url.includes("/check-suites")) { + suitePages += 1; + return Response.json( + { check_suites: [{ status: "completed", app: { slug: "github-actions" } }] }, + { headers: { link: '; rel="next"' } }, + ); + } + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "capped-suites", "public-token", null); + + expect(suitePages).toBe(10); // it paginates now, instead of reading page 1 and stopping + expect(aggregate.ciState).toBe("pending"); // unreadable backstop => not certified settled + }); + it("fails completed non-required red checks while still reporting optional pending visibility", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { @@ -1469,6 +1552,45 @@ describe("GitHub backfill", () => { }); }); + // #9052 (second instance, same class): fetchLivePullRequestReviewDecision had no top-level `errors` guard + // either. Returning plain `undefined` made the caller fall back to the STORED decision + // (`liveReviewDecision ?? pr.reviewDecision`), so a PR approved at backfill time and later flipped to + // CHANGES_REQUESTED still read as APPROVED -> approvalsSatisfied -> merge. The sentinel is a real VALUE + // precisely so it survives that `??` and the stale stored decision can never be substituted. + describe("fetchLivePullRequestReviewDecision — partial-response guard (#9052)", () => { + it("returns the unreadable sentinel on a 200-with-errors response, so the stale stored decision cannot win the ?? fallback", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + return Response.json({ + data: { repository: { pullRequest: { reviewDecision: null } } }, + errors: [{ message: "Something went wrong while executing your query." }], + }); + }); + + const decision = await fetchLivePullRequestReviewDecision(env, "JSONbored/loopover", 7, "public-token"); + + expect(decision).toBe(REVIEW_DECISION_UNREADABLE); + // The whole point: it survives `?? stored` and is not APPROVED, so approvalsSatisfied stays false. + expect(decision ?? "APPROVED").not.toBe("APPROVED"); + // ...and it is not mistaken for an explicit CHANGES_REQUESTED either. + expect(decision).not.toBe("CHANGES_REQUESTED"); + }); + + it("still returns the real decision on a clean response, and undefined when the PR genuinely has none", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + let reviewDecision: string | null = "APPROVED"; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + return Response.json({ data: { repository: { pullRequest: { reviewDecision } } } }); + }); + + expect(await fetchLivePullRequestReviewDecision(env, "JSONbored/loopover", 8, "public-token")).toBe("APPROVED"); + reviewDecision = null; + expect(await fetchLivePullRequestReviewDecision(env, "JSONbored/loopover", 8, "public-token")).toBeUndefined(); + }); + }); + describe("fetchLiveReviewThreadBlockers", () => { it("returns unresolved non-outdated scanner review threads as blockers", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); @@ -1548,6 +1670,27 @@ describe("GitHub backfill", () => { expect(blockers).toEqual([expect.objectContaining({ title: "blocker on the third page", priority: "P1", path: "src/paginated.ts" })]); }); + // #9052: GitHub's standard partial-failure shape under load is HTTP 200 with `reviewThreads: null` AND a + // top-level `errors` array. That yielded `[]` -- indistinguishable from a genuinely thread-free PR -- so a + // maintainer's unresolved blocking thread was silently dropped and the gate could conclude success and MERGE + // over the open objection. Unlike a transport error (nothing read at all -> fail open), a partial result + // means we KNOW the answer is unreliable, so it fails closed. + it("#9052: a 200-with-errors partial GraphQL response fails CLOSED with a blocker, not an empty thread list", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + return Response.json({ + data: { repository: { pullRequest: { reviewThreads: null } } }, + errors: [{ message: "Something went wrong while executing your query." }], + }); + }); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/loopover", 3, "public-token"); + + expect(blockers).toHaveLength(1); + expect(blockers[0]).toMatchObject({ title: expect.stringContaining("could not be read"), scannerFinding: false }); + }); + it("is bounded: a pathological always-hasNextPage response stops at REVIEW_THREAD_MAX_PAGES instead of looping unboundedly (#7454)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); let call = 0; diff --git a/test/unit/graphql-status-rollup.test.ts b/test/unit/graphql-status-rollup.test.ts index c94122b1be..3da5a3caf5 100644 --- a/test/unit/graphql-status-rollup.test.ts +++ b/test/unit/graphql-status-rollup.test.ts @@ -88,6 +88,23 @@ describe("fetchLiveCiAggregateViaGraphQl — verdicts", () => { expect(await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN)).toBeNull(); }); + // #9051: `contexts` had this guard from the start; `checkSuites` did not, despite being the settled-commit + // backstop -- a truncated suites read (>100) could hide a still-running first-party suite and certify the + // commit as settled. Falls back to REST, which now paginates the suites read too. + it("#9051: returns null when checkSuites reports another page (a truncated backstop cannot certify settled)", async () => { + stubGraphql({ + data: { + repository: { + object: { + statusCheckRollup: { contexts: { nodes: [runNode({ name: "build", conclusion: "SUCCESS", status: "COMPLETED" })], pageInfo: { hasNextPage: false } } }, + checkSuites: { nodes: [{ status: "COMPLETED", app: { slug: "github-actions" } }], pageInfo: { hasNextPage: true } }, + }, + }, + }, + }); + expect(await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN)).toBeNull(); + }); + it("passes when a required check-run and status are green", async () => { stubGraphql(graphqlBody({ runs: [{ name: "build", conclusion: "SUCCESS", status: "COMPLETED", appSlug: "github-actions" }], statuses: [{ context: "codecov/patch", state: "SUCCESS" }], suites: [{ status: "COMPLETED", appSlug: "github-actions" }] })); expect(await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN, new Set(["build"]))).toMatchObject({ ciState: "passed", hasPending: false, failingDetails: [] }); 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", () => {