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