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", () => {