From aafe28a0169d6b9ccc9a3bc961ba21b42ead4190 Mon Sep 17 00:00:00 2001 From: Rinalds Uzkalns Date: Tue, 20 Oct 2026 02:28:05 +0200 Subject: [PATCH 1/5] fix(auto-merge): merge plan PRs when GitHub auto-merge is off or there is no CI Epic PR integry/agent-tank-website#43 reached its Ultrafix goal but never merged. GitHub refused to arm auto-merge ("Auto merge is not allowed for this repository"), the failure was only logged, and the direct-merge fallback never ran: it is driven by check events, the PR targets an epic branch the repo's CI does not run on, and a commit with no CI signal was never treated as mergeable. - enableAutoMerge reports `unavailable` when GitHub cannot arm auto-merge at all (repository setting off, or the PR is already in clean status). - After Ultrafix reaches its goal, an unavailable arm falls back to the direct merge path with all its gates, ignoring only the finishing job's own work. - When the repository setting is off, ProPR comments on the PR suggesting "Allow auto-merge" (Ultrafix finish and first PR arming). - The direct merge accepts CI that has finished passing, or no CI at all for a head that is older than a 2-minute grace, so unregistered checks are not mistaken for missing CI. Closed PRs are skipped. - The daemon sweeps open plan PRs every minute through the same gates, since a repository without CI, or CI that finished before Ultrafix did, produces no further event that would merge the PR. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/core/src/index.ts | 4 +- packages/core/src/webhook/checkRunHandler.ts | 32 +++++++--- packages/core/src/webhook/checkRunHelpers.ts | 49 ++++++++++++++- packages/core/src/webhook/webhookHandler.ts | 2 +- src/daemon.ts | 3 + src/daemon/autoMergeSweep.ts | 63 +++++++++++++++++++ src/github/autoMergeAvailability.ts | 33 ++++++++++ src/github/autoMergeOperations.ts | 10 ++- src/jobs/issueJobPostProcessingHelpers.ts | 7 +++ src/jobs/ultrafixLoopContinuationHelpers.ts | 5 +- src/jobs/ultrafixLoopGithub.ts | 24 +++++-- test/autoMergeOperations.test.ts | 12 ++++ test/autoMergeSweep.test.ts | 35 +++++++++++ test/checkRunHandler.test.ts | 45 +++++++++++++ test/ultrafixAutoMergeFallback.test.ts | 66 ++++++++++++++++++++ test/ultrafixCiFailureRecovery.test.ts | 1 + test/ultrafixContinuationEntryPoint.test.ts | 1 + test/ultrafixRecoveryIntake.test.ts | 1 + 18 files changed, 371 insertions(+), 22 deletions(-) create mode 100644 src/daemon/autoMergeSweep.ts create mode 100644 src/github/autoMergeAvailability.ts create mode 100644 test/autoMergeSweep.test.ts create mode 100644 test/ultrafixAutoMergeFallback.test.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e146fde482..2f502e929f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -157,8 +157,8 @@ export type { AutoResolveContext } from './queue/taskQueue.js'; -export { areAllChecksPassing, buildRedisRuntimeConfig, closeUltrafixStateRedis, getCurrentPRHead, getCheckRunsStatus, getCheckRunsStatusForRepo, summarizeCheckSignals, getActiveTasksForPR, hasActiveTasksForPR, type CheckRunsStatus, type ActivePRWork, type ActivePRTask, type ActivePRQueuedJob } from './webhook/checkRunHelpers.js'; -export { handleCheckRunEvent, handleCheckSuiteEvent, handleStatusEvent, reevaluatePRAutoMerge, setUltrafixCheckRunHook, triggerUltrafixCheckRunHook, type StatusEventPayload } from './webhook/checkRunHandler.js'; +export { areAllChecksPassing, getCiMergeReadiness, NO_CI_GRACE_MS, type CiMergeReadiness, buildRedisRuntimeConfig, closeUltrafixStateRedis, getCurrentPRHead, getCheckRunsStatus, getCheckRunsStatusForRepo, summarizeCheckSignals, getActiveTasksForPR, hasActiveTasksForPR, type CheckRunsStatus, type ActivePRWork, type ActivePRTask, type ActivePRQueuedJob } from './webhook/checkRunHelpers.js'; +export { handleCheckRunEvent, handleCheckSuiteEvent, handleStatusEvent, reevaluatePRAutoMerge, setUltrafixCheckRunHook, triggerUltrafixCheckRunHook, type AutoMergeReevaluationOptions, type StatusEventPayload } from './webhook/checkRunHandler.js'; export { recoverCiFailureFollowups } from './webhook/ciFailureRecovery.js'; export * from './webhook/ciFailureFollowup.js'; export * from './webhook/closedPullRequestCi.js'; diff --git a/packages/core/src/webhook/checkRunHandler.ts b/packages/core/src/webhook/checkRunHandler.ts index 09d2faf0d7..043c04f235 100644 --- a/packages/core/src/webhook/checkRunHandler.ts +++ b/packages/core/src/webhook/checkRunHandler.ts @@ -6,7 +6,7 @@ import { deleteBranch, getFirstCommitMessage, getCurrentPRHead, - areAllChecksPassing, + getCiMergeReadiness, getPRAutoMergeInfo, linkedIssueHasAutoMergeLabel, hasActiveTasksForPR, @@ -177,10 +177,15 @@ async function performMergeAndPostActions(ctx: PRMergeContext, mergeMethod: Auto /** * Processes a single PR for potential auto-merge. */ -async function processPRAutoMerge(ctx: PRContext, headSha: string): Promise { +async function processPRAutoMerge(ctx: PRContext, headSha: string, options: { ignoreWorkIds?: readonly string[] } = {}): Promise { const { owner, repoName, prNumber, log } = ctx; const prInfo = await getPRAutoMergeInfo(owner, repoName, prNumber); + if (prInfo.state !== undefined && prInfo.state !== 'open') { + log.debug({ owner, repoName, prNumber, state: prInfo.state }, 'PR is not open, skipping auto-merge'); + return; + } + if (prInfo.isDraft) { log.debug({ owner, repoName, prNumber }, 'PR is a draft, skipping auto-merge'); return; @@ -197,9 +202,10 @@ async function processPRAutoMerge(ctx: PRContext, headSha: string): Promise { - const log = logger.withCorrelation(correlationId); + const log = logger.withCorrelation(options.correlationId); const headSha = await getCurrentPRHead(owner, repoName, prNumber); if (!headSha) { log.debug({ owner, repoName, prNumber }, 'Auto-merge re-evaluation skipped: PR head SHA unavailable'); @@ -266,7 +278,7 @@ export async function reevaluatePRAutoMerge( } const ctx: PRContext = { owner, repoName, prNumber, log }; - await processPRAutoMerge(ctx, headSha); + await processPRAutoMerge(ctx, headSha, options); } /** diff --git a/packages/core/src/webhook/checkRunHelpers.ts b/packages/core/src/webhook/checkRunHelpers.ts index 2cdb99e95c..6a381f43ae 100644 --- a/packages/core/src/webhook/checkRunHelpers.ts +++ b/packages/core/src/webhook/checkRunHelpers.ts @@ -457,6 +457,44 @@ export async function areAllChecksPassing( } } +/** How long a head may go without any CI signal before ProPR treats the repository as having no CI for it. */ +export const NO_CI_GRACE_MS = 2 * 60_000; + +export interface CiMergeReadiness { + ready: boolean; + reason: 'checks_passing' | 'checks_not_passing' | 'no_ci' | 'waiting_for_ci' | 'unavailable'; +} + +/** + * Whether CI allows merging `ref`: every blocking check passed, or no CI ran for + * it at all. A head with no signal counts as having no CI only once it is older + * than {@link NO_CI_GRACE_MS}, so checks that have not registered yet are not + * mistaken for missing CI. + */ +export async function getCiMergeReadiness( + owner: string, + repoName: string, + ref: string, + options: { now?: number; loadNonBlockingChecks?: (owner: string, repo: string) => Promise } = {}, +): Promise { + const { now = Date.now(), loadNonBlockingChecks = getNonBlockingChecksForRepository } = options; + try { + const { checkRuns, commitStatus } = await fetchCheckSignals(owner, repoName, ref); + if (checkRuns.length > 0 || commitStatus.totalCount > 0) { + const summary = summarizeCheckSignals(checkRuns, commitStatus, await loadNonBlockingChecks(owner, repoName)); + return summary.allPassing ? { ready: true, reason: 'checks_passing' } : { ready: false, reason: 'checks_not_passing' }; + } + const octokit = await getAuthenticatedOctokit(); + const commit = await octokit.request('GET /repos/{owner}/{repo}/commits/{ref}', { owner, repo: repoName, ref }); + const committedAt = Date.parse(commit.data.commit.committer?.date ?? ''); + if (!Number.isFinite(committedAt)) return { ready: false, reason: 'unavailable' }; + return now - committedAt >= NO_CI_GRACE_MS ? { ready: true, reason: 'no_ci' } : { ready: false, reason: 'waiting_for_ci' }; + } catch (error) { + logger.warn({ owner, repoName, ref, error: (error as Error).message }, 'Failed to check CI merge readiness'); + return { ready: false, reason: 'unavailable' }; + } +} + export interface PRAutoMergeInfo { hasLabel: boolean; hasUltrafixLabel?: boolean; @@ -468,6 +506,8 @@ export interface PRAutoMergeInfo { headBranch: string; mergeable: boolean | null; mergeableState: string; + /** GitHub PR state (`open` or `closed`); absent when the PR could not be read. */ + state?: string; } const ULTRAFIX_STATE_KEY_PREFIX = 'ultrafix:state'; @@ -651,6 +691,7 @@ export async function getPRAutoMergeInfo(owner: string, repoName: string, prNumb : 'unknown'; return { + state: prResponse.data.state, hasLabel, hasUltrafixLabel, hasActiveUltrafixLoop: ultrafixState?.active ?? false, @@ -843,13 +884,17 @@ export async function getActiveTasksForPR( */ export async function hasActiveTasksForPR( repository: string, - prNumber: number + prNumber: number, + ignoreWorkIds: readonly string[] = [], ): Promise<{ hasActive: boolean; activeTasks: Array<{ taskId: string; state: string }>; queuedJobs: Array<{ jobId: string; state: string }>; }> { - const { activeTasks, queuedJobs } = await getActiveTasksForPR(repository, prNumber); + const ignored = new Set(ignoreWorkIds); + const work = await getActiveTasksForPR(repository, prNumber); + const activeTasks = work.activeTasks.filter(task => !ignored.has(task.taskId)); + const queuedJobs = work.queuedJobs.filter(job => !ignored.has(job.jobId)); return { hasActive: activeTasks.length > 0 || queuedJobs.length > 0, activeTasks, diff --git a/packages/core/src/webhook/webhookHandler.ts b/packages/core/src/webhook/webhookHandler.ts index 1200036a35..b5bbbcae6c 100644 --- a/packages/core/src/webhook/webhookHandler.ts +++ b/packages/core/src/webhook/webhookHandler.ts @@ -206,7 +206,7 @@ async function handleUltrafixLabelRemoval(payload: unknown, eventType: WebhookEv const prNumber = 'pull_request' in event ? event.pull_request.number : event.issue.number; const log = logger.withCorrelation(correlationId); if (await clearStateForCurrentUltrafixLabelRemoval(owner, repo, prNumber, log)) { - await reevaluatePRAutoMerge(owner, repo, prNumber, correlationId); + await reevaluatePRAutoMerge(owner, repo, prNumber, { correlationId }); log.info({ owner, repo, prNumber }, `Cleared ultrafix loop state after ${eventType === 'pull_request' ? 'PR' : 'issue'} ultrafix label removal`); } } diff --git a/src/daemon.ts b/src/daemon.ts index 052d7268e7..3d03aa3bbb 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -52,6 +52,7 @@ import { sweepPushRescues } from './daemon/rescueRefSweep.js'; import { scheduleAgentRunSweeps } from './agentRunScheduler.js'; import { scheduleUltrafixResumeSweep } from './daemon/ultrafixResumeSweep.js'; import { scheduleEpicMergeProgressRetrySweep } from './daemon/epicMergeProgressRetrySweep.js'; +import { scheduleAutoMergeSweep } from './daemon/autoMergeSweep.js'; import { clearUltrafixStateIfCurrent, hasUltrafixAutomaticWork, @@ -243,6 +244,7 @@ async function startDaemon(options: DaemonOptions = {}): Promise { // without the API server and with polling off does not wait for a webhook. const ultrafixResumeSweepInterval = scheduleUltrafixResumeSweep(redisClient); const epicMergeProgressRetryInterval = scheduleEpicMergeProgressRetrySweep(); + const autoMergeSweepInterval = scheduleAutoMergeSweep(); const repos = getRepos(); await reconcileTaskIntentsSafely(redisClient, repos); @@ -462,6 +464,7 @@ async function startDaemon(options: DaemonOptions = {}): Promise { clearInterval(pushRescueSweepInterval); clearInterval(ultrafixResumeSweepInterval); clearInterval(epicMergeProgressRetryInterval); + clearInterval(autoMergeSweepInterval); clearInterval(mergeConflictSweepInterval); stopManagedDiagnostics(); await stopAgentRunSweeps(); diff --git a/src/daemon/autoMergeSweep.ts b/src/daemon/autoMergeSweep.ts new file mode 100644 index 0000000000..5f807ba584 --- /dev/null +++ b/src/daemon/autoMergeSweep.ts @@ -0,0 +1,63 @@ +import { db, generateCorrelationId, logger, PlanIssueStatus, reevaluatePRAutoMerge } from '@propr/core'; + +export const AUTO_MERGE_SWEEP_INTERVAL_MS = 60_000; +/** Plan PRs untouched for longer than this are left to webhooks and humans. */ +export const AUTO_MERGE_SWEEP_LOOKBACK_DAYS = 14; + +export interface AutoMergeSweepCandidate { + repository: string; + prNumber: number; +} + +/** Open plan PRs (linked, not yet merged or closed) updated recently. */ +export async function loadAutoMergeSweepCandidates(now: Date = new Date()): Promise { + const since = new Date(now.getTime() - AUTO_MERGE_SWEEP_LOOKBACK_DAYS * 24 * 60 * 60 * 1000); + const rows: Array<{ repository: string; pr_number: number }> = await db('plan_issues') + .distinct('repository', 'pr_number') + .whereNotNull('pr_number') + .whereNotIn('status', [PlanIssueStatus.MERGED, PlanIssueStatus.CLOSED]) + .where('updated_at', '>=', since.toISOString().replace('T', ' ').replace(/\.\d+Z$/, '')); + return rows.map(row => ({ repository: row.repository, prNumber: Number(row.pr_number) })); +} + +/** + * Merges auto-merge plan PRs that no event will merge: a repository with no CI + * produces no check events, and CI that finished before Ultrafix reached its goal + * will not fire again. Each PR goes through the same gates as a green check + * (labels, Ultrafix state, CI, mergeability, active work, repository policy). + */ +export async function sweepAutoMergeCandidates( + loadCandidates: () => Promise = loadAutoMergeSweepCandidates, + reevaluate: typeof reevaluatePRAutoMerge = reevaluatePRAutoMerge, +): Promise { + for (const { repository, prNumber } of await loadCandidates()) { + const [owner, repo] = repository.split('/'); + if (!owner || !repo) continue; + try { + await reevaluate(owner, repo, prNumber, { correlationId: generateCorrelationId() }); + } catch (error) { + logger.warn({ repository, prNumber, error: (error as Error).message }, 'Auto-merge sweep failed for PR'); + } + } +} + +export function scheduleAutoMergeSweep( + intervalMs: number = AUTO_MERGE_SWEEP_INTERVAL_MS, + sweep: () => Promise = () => sweepAutoMergeCandidates(), +): NodeJS.Timeout { + let running = false; + const tick = async (): Promise => { + if (running) return; + running = true; + try { + await sweep(); + } catch (error) { + logger.warn({ error: (error as Error).message }, 'Auto-merge sweep failed'); + } finally { + running = false; + } + }; + const interval = setInterval(() => { void tick(); }, intervalMs); + interval.unref?.(); + return interval; +} diff --git a/src/github/autoMergeAvailability.ts b/src/github/autoMergeAvailability.ts new file mode 100644 index 0000000000..bae83804fb --- /dev/null +++ b/src/github/autoMergeAvailability.ts @@ -0,0 +1,33 @@ +import { getAuthenticatedOctokit } from '@propr/core'; + +/** GitHub's refusal when the repository's "Allow auto-merge" setting is off. */ +export function isAutoMergeDisabledForRepository(message: string | undefined): boolean { + return /auto merge is not allowed for this repository/i.test(message ?? ''); +} + +/** Comment suggesting the repository setting that lets GitHub merge on its own. */ +export const AUTO_MERGE_DISABLED_COMMENT = [ + 'ℹ️ **GitHub auto-merge is turned off for this repository**, so ProPR merges this PR itself once it is ready ' + + '(checks passed or no CI configured, and the review goal met).', + '', + 'To let GitHub merge it the moment required checks pass, enable **Settings → General → Pull Requests → Allow auto-merge**.', +].join('\n'); + +/** Posts {@link AUTO_MERGE_DISABLED_COMMENT} on the PR; returns the failure instead of throwing. */ +export async function suggestEnablingAutoMerge(owner: string, repoName: string, prNumber: number): Promise { + try { + const octokit = await getAuthenticatedOctokit(); + await octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', { + owner, repo: repoName, issue_number: prNumber, body: AUTO_MERGE_DISABLED_COMMENT, + }); + return null; + } catch (error) { + return error as Error; + } +} + +/** GitHub errors meaning auto-merge cannot be armed, so a direct merge is needed. */ +export function isAutoMergeUnavailableError(message: string): boolean { + return isAutoMergeDisabledForRepository(message) + || /pull request is in clean status/i.test(message); +} diff --git a/src/github/autoMergeOperations.ts b/src/github/autoMergeOperations.ts index 11d28919d4..0de2c5e62c 100644 --- a/src/github/autoMergeOperations.ts +++ b/src/github/autoMergeOperations.ts @@ -1,4 +1,5 @@ import { getAuthenticatedOctokit, logger, handleError } from '@propr/core'; +import { isAutoMergeUnavailableError } from './autoMergeAvailability.js'; /** * Auto-merge method options supported by GitHub. @@ -26,6 +27,12 @@ export interface EnableAutoMergeResult { autoMergeEnabled?: boolean; /** True when the PR's head or base moved since the evaluated snapshot; nothing was armed. */ headChanged?: boolean; + /** + * True when GitHub cannot arm auto-merge for this PR at all: the repository + * disallows it, or the PR is already mergeable so there is nothing to wait for. + * The caller has to merge directly instead. + */ + unavailable?: boolean; } export interface DisableAutoMergeOptions { @@ -190,7 +197,8 @@ export async function enableAutoMerge(options: EnableAutoMergeOptions): Promise< return { success: false, - error: errorMessages + error: errorMessages, + ...(isAutoMergeUnavailableError(errorMessages) && { unavailable: true }), }; } diff --git a/src/jobs/issueJobPostProcessingHelpers.ts b/src/jobs/issueJobPostProcessingHelpers.ts index a72ccd85c3..8a31af1da4 100644 --- a/src/jobs/issueJobPostProcessingHelpers.ts +++ b/src/jobs/issueJobPostProcessingHelpers.ts @@ -18,6 +18,7 @@ import { type SubmissionPayload, } from '@propr/core'; import { enableAutoMerge } from '../github/autoMergeOperations.js'; +import { isAutoMergeDisabledForRepository, suggestEnablingAutoMerge } from '../github/autoMergeAvailability.js'; import type { PostProcessingResult } from './issueJobHelpers.js'; import { redisClient } from './issueJob/config.js'; import { resolveImplementationPrUltrafixTrigger } from './implementationPrUltrafix.js'; @@ -385,4 +386,10 @@ export async function handleCreatedPlanIssuePR(options: { } correlatedLogger.warn({ prNumber, error: autoMergeResult.error }, 'Failed to enable auto-merge on PR'); + // ProPR still merges the PR itself (check events and the daemon's auto-merge + // sweep); point out the setting that lets GitHub do it. + if (isAutoMergeDisabledForRepository(autoMergeResult.error)) { + const suggestionError = await suggestEnablingAutoMerge(issueRef.repoOwner, issueRef.repoName, prNumber); + if (suggestionError) correlatedLogger.warn({ prNumber, error: suggestionError.message }, 'Failed to suggest enabling GitHub auto-merge'); + } } diff --git a/src/jobs/ultrafixLoopContinuationHelpers.ts b/src/jobs/ultrafixLoopContinuationHelpers.ts index fa4d8b064b..01c9b8e482 100644 --- a/src/jobs/ultrafixLoopContinuationHelpers.ts +++ b/src/jobs/ultrafixLoopContinuationHelpers.ts @@ -92,7 +92,10 @@ export async function finishUltrafixLoop(input: { if (!stateCleared) return false; await clearDeferredContinuationIfCurrent(redisClient, identity, workEpoch); await removeUltrafixLabel(owner, repo, pullRequestNumber, correlatedLogger); - await maybeEnableAutoMerge(owner, repo, pullRequestNumber, correlatedLogger); + await maybeEnableAutoMerge( + { owner, repo, pullRequestNumber, correlationId: params.correlationId, currentJobId: params.currentJobId }, + correlatedLogger, + ); return true; }); if (!finishResult) return { continued: false, reason: 'ultrafix_superseded' }; diff --git a/src/jobs/ultrafixLoopGithub.ts b/src/jobs/ultrafixLoopGithub.ts index 473f1795c1..f46ab8f607 100644 --- a/src/jobs/ultrafixLoopGithub.ts +++ b/src/jobs/ultrafixLoopGithub.ts @@ -8,11 +8,13 @@ import { findPlanIssueByRepoAndPR, gateAutoMergeArming, getAuthenticatedOctokit, + reevaluatePRAutoMerge, retryConfigs, safeRemoveLabel, withRetry, } from '@propr/core'; import { enableAutoMerge } from '../github/autoMergeOperations.js'; +import { isAutoMergeDisabledForRepository, suggestEnablingAutoMerge } from '../github/autoMergeAvailability.js'; async function fetchUltrafixLabelPresence(owner: string, repo: string, pullRequestNumber: number): Promise { const octokit = await withRetry( @@ -152,11 +154,10 @@ export async function updatePrComment(options: { } export async function maybeEnableAutoMerge( - owner: string, - repo: string, - pullRequestNumber: number, + pr: { owner: string; repo: string; pullRequestNumber: number; correlationId?: string; currentJobId?: string }, correlatedLogger: Logger, ): Promise { + const { owner, repo, pullRequestNumber } = pr; try { const repository = `${owner}/${repo}`; const planIssue = await findPlanIssueByRepoAndPR(repository, pullRequestNumber); @@ -185,9 +186,22 @@ export async function maybeEnableAutoMerge( owner, repoName: repo, prNumber: pullRequestNumber, mergeMethod: gate.mergeMethod, expectedHead: { headSha: gate.pullRequest.headSha, baseRef: gate.pullRequest.baseRef }, }); - if (!result.success) { - correlatedLogger.warn({ pullRequestNumber, error: result.error }, 'Failed to enable auto-merge after ultrafix success'); + if (result.success) return; + correlatedLogger.warn({ pullRequestNumber, error: result.error }, 'Failed to enable auto-merge after ultrafix success'); + if (!result.unavailable) return; + if (isAutoMergeDisabledForRepository(result.error)) { + const suggestionError = await suggestEnablingAutoMerge(owner, repo, pullRequestNumber); + if (suggestionError) correlatedLogger.warn({ pullRequestNumber, error: suggestionError.message }, 'Failed to suggest enabling GitHub auto-merge'); } + // GitHub cannot wait for this PR (setting off, or already mergeable), and + // CI may have finished before the goal was reached, so no later check + // event would merge it. Merge directly through the same gates instead; + // only this finishing job is excluded from the active-work check. + correlatedLogger.info({ pullRequestNumber }, 'GitHub auto-merge unavailable after ultrafix success, evaluating a direct merge'); + await reevaluatePRAutoMerge(owner, repo, pullRequestNumber, { + correlationId: pr.correlationId ?? `ultrafix-automerge-${owner}-${repo}-${pullRequestNumber}`, + ignoreWorkIds: pr.currentJobId ? [pr.currentJobId] : [], + }); } catch (err) { correlatedLogger.warn({ error: (err as Error).message, pullRequestNumber }, 'Failed to evaluate auto-merge re-enable after ultrafix success'); } diff --git a/test/autoMergeOperations.test.ts b/test/autoMergeOperations.test.ts index af5a048355..e9c46de3e0 100644 --- a/test/autoMergeOperations.test.ts +++ b/test/autoMergeOperations.test.ts @@ -46,3 +46,15 @@ test('a PR retargeted after the evaluation is not armed with the old base policy assert.equal(result.headChanged, true); assert.equal(graphql.mock.callCount(), 0); }); + +test('GitHub refusing to arm at all is reported as unavailable so callers merge directly', async () => { + currentPullRequest = { ...currentPullRequest, head: { sha: 'evaluated-head' }, base: { ref: 'main' } }; + for (const message of ['Auto merge is not allowed for this repository', 'Pull request is in clean status']) { + graphql.mock.mockImplementationOnce(async () => { throw Object.assign(new Error('GraphQL'), { errors: [{ message }] }); }); + const result = await enableAutoMerge({ owner: 'acme', repoName: 'repo', prNumber: 7, expectedHead }); + assert.deepEqual(result, { success: false, error: message, unavailable: true }); + } + graphql.mock.mockImplementationOnce(async () => { throw Object.assign(new Error('GraphQL'), { errors: [{ message: 'Head sha did not match' }] }); }); + const other = await enableAutoMerge({ owner: 'acme', repoName: 'repo', prNumber: 7, expectedHead }); + assert.equal(other.unavailable, undefined); +}); diff --git a/test/autoMergeSweep.test.ts b/test/autoMergeSweep.test.ts new file mode 100644 index 0000000000..e1c98dd2f2 --- /dev/null +++ b/test/autoMergeSweep.test.ts @@ -0,0 +1,35 @@ +import assert from 'node:assert/strict'; +import { mock, test } from 'node:test'; + +await mock.module('@propr/core', { + namedExports: { + db: () => { throw new Error('the sweep test injects its candidates'); }, + generateCorrelationId: () => 'corr', + logger: { warn() {}, info() {}, debug() {}, error() {} }, + PlanIssueStatus: { MERGED: 'merged', CLOSED: 'closed' }, + reevaluatePRAutoMerge: async () => undefined, + }, +}); + +const { sweepAutoMergeCandidates } = await import('../src/daemon/autoMergeSweep.js'); + +test('the sweep re-evaluates every open plan PR through the merge gates', async () => { + const reevaluate = mock.fn(async (owner: string, _repo: string, _pr: number) => { + if (owner === 'broken') throw new Error('GitHub down'); + }); + await sweepAutoMergeCandidates( + async () => [ + { repository: 'integry/agent-tank-website', prNumber: 43 }, + { repository: 'broken/repo', prNumber: 1 }, + { repository: 'not-a-repository', prNumber: 2 }, + { repository: 'integry/forex', prNumber: 703 }, + ], + reevaluate as never, + ); + // One PR failing does not stop the rest; malformed repositories are skipped. + assert.deepEqual(reevaluate.mock.calls.map(call => call.arguments.slice(0, 3)), [ + ['integry', 'agent-tank-website', 43], + ['broken', 'repo', 1], + ['integry', 'forex', 703], + ]); +}); diff --git a/test/checkRunHandler.test.ts b/test/checkRunHandler.test.ts index 24e37b3ff9..f38f59f69a 100644 --- a/test/checkRunHandler.test.ts +++ b/test/checkRunHandler.test.ts @@ -142,6 +142,8 @@ const { deleteBranch, getCurrentPRHead, areAllChecksPassing, + getCiMergeReadiness, + NO_CI_GRACE_MS, getCheckRunsStatus, getCheckRunsStatusForRepo, getPRAutoMergeInfo, @@ -433,6 +435,49 @@ describe('getCheckRunsStatus with nonBlockingChecks (Ultrafix readiness)', () => }); }); +// ============= getCiMergeReadiness Tests ============= +describe('getCiMergeReadiness', () => { + const committedAt = Date.parse('2026-10-19T23:46:00Z'); + function respond(checkRuns: unknown[], statusCount = 0): void { + mockOctokit.request.mock.mockImplementation(async (route: string) => { + if (route.endsWith('/check-runs')) return { data: { check_runs: checkRuns } }; + if (route.endsWith('/status')) return { data: { state: statusCount ? 'success' : 'pending', total_count: statusCount, statuses: [] } }; + if (route === 'GET /repos/{owner}/{repo}/commits/{ref}') return { data: { commit: { committer: { date: '2026-10-19T23:46:00Z' } } } }; + throw new Error(`unexpected ${route}`); + }); + } + + test('passing checks allow the merge', async () => { + resetMocks(); + respond([{ name: 'build', status: 'completed', conclusion: 'success' }]); + assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt, loadNonBlockingChecks: async () => [] }), { ready: true, reason: 'checks_passing' }); + }); + + test('pending checks hold the merge', async () => { + resetMocks(); + respond([{ name: 'build', status: 'in_progress', conclusion: null }]); + assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt + NO_CI_GRACE_MS * 10, loadNonBlockingChecks: async () => [] }), { ready: false, reason: 'checks_not_passing' }); + }); + + test('a head with no CI at all is mergeable once checks had time to register', async () => { + resetMocks(); + respond([]); + assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt + NO_CI_GRACE_MS, loadNonBlockingChecks: async () => [] }), { ready: true, reason: 'no_ci' }); + }); + + test('a fresh head with no CI signal waits for checks to register', async () => { + resetMocks(); + respond([]); + assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt + NO_CI_GRACE_MS - 1, loadNonBlockingChecks: async () => [] }), { ready: false, reason: 'waiting_for_ci' }); + }); + + test('an API failure never allows the merge', async () => { + resetMocks(); + mockOctokit.request.mock.mockImplementation(async () => { throw new Error('boom'); }); + assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt, loadNonBlockingChecks: async () => [] }), { ready: false, reason: 'unavailable' }); + }); +}); + // ============= areAllChecksPassing Tests ============= describe('areAllChecksPassing', () => { diff --git a/test/ultrafixAutoMergeFallback.test.ts b/test/ultrafixAutoMergeFallback.test.ts new file mode 100644 index 0000000000..f530b32488 --- /dev/null +++ b/test/ultrafixAutoMergeFallback.test.ts @@ -0,0 +1,66 @@ +import assert from 'node:assert/strict'; +import { beforeEach, mock, test } from 'node:test'; + +/** + * When Ultrafix reaches its goal, CI has usually finished already, so no later + * check event will merge the PR. If GitHub cannot arm auto-merge, Ultrafix + * merges through ProPR's own gates instead, ignoring only its own job. + */ +const requests: Array<{ route: string; params: Record }> = []; +const octokit = { + request: async (route: string, params: Record) => { + requests.push({ route, params }); + if (route.startsWith('GET /repos/{owner}/{repo}/issues/')) return { data: { labels: [{ name: 'auto-merge' }] } }; + return { data: {} }; + }, +}; +const reevaluatePRAutoMerge = mock.fn(async (..._args: unknown[]) => undefined); +let armResult: Record = { success: true, autoMergeEnabled: true }; +const enableAutoMerge = mock.fn(async () => armResult); + +await mock.module('@propr/core', { + namedExports: { + findPlanIssueByRepoAndPR: async () => ({ issue_number: 23 }), + gateAutoMergeArming: async () => ({ arm: true, mergeMethod: 'SQUASH', pullRequest: { headSha: 'head', baseRef: 'epic' } }), + getAuthenticatedOctokit: async () => octokit, + reevaluatePRAutoMerge, + retryConfigs: { githubApi: {} }, + safeRemoveLabel: async () => undefined, + withRetry: async (fn: () => unknown) => fn(), + }, +}); +await mock.module('../src/github/autoMergeOperations.js', { namedExports: { enableAutoMerge } }); + +const { maybeEnableAutoMerge } = await import('../src/jobs/ultrafixLoopGithub.js'); +const pr = { owner: 'integry', repo: 'agent-tank-website', pullRequestNumber: 43 }; +const log = { info() {}, warn() {}, error() {}, debug() {} } as never; +const comments = () => requests.filter(r => r.route.startsWith('POST /repos/{owner}/{repo}/issues/{issue_number}/comments')); + +beforeEach(() => { + requests.length = 0; + reevaluatePRAutoMerge.mock.resetCalls(); +}); + +test('a repository with auto-merge disabled gets a suggestion and a direct merge', async () => { + armResult = { success: false, unavailable: true, error: 'Auto merge is not allowed for this repository' }; + await maybeEnableAutoMerge({ ...pr, correlationId: 'corr', currentJobId: 'pr-comments-batch-1' }, log); + assert.equal(comments().length, 1); + assert.match(String(comments()[0].params.body), /Allow auto-merge/); + assert.deepEqual(reevaluatePRAutoMerge.mock.calls[0].arguments, ['integry', 'agent-tank-website', 43, { correlationId: 'corr', ignoreWorkIds: ['pr-comments-batch-1'] }]); +}); + +test('an already mergeable PR is merged directly without a settings comment', async () => { + armResult = { success: false, unavailable: true, error: 'Pull request is in clean status' }; + await maybeEnableAutoMerge({ ...pr, correlationId: 'corr', currentJobId: 'job' }, log); + assert.equal(comments().length, 0); + assert.equal(reevaluatePRAutoMerge.mock.callCount(), 1); +}); + +test('armed auto-merge and other failures do not merge directly', async () => { + armResult = { success: true, autoMergeEnabled: true }; + await maybeEnableAutoMerge(pr, log); + armResult = { success: false, headChanged: true, error: 'Pull request head or base changed' }; + await maybeEnableAutoMerge(pr, log); + assert.equal(reevaluatePRAutoMerge.mock.callCount(), 0); + assert.equal(comments().length, 0); +}); diff --git a/test/ultrafixCiFailureRecovery.test.ts b/test/ultrafixCiFailureRecovery.test.ts index 3e4414d0c0..42f3e1975e 100644 --- a/test/ultrafixCiFailureRecovery.test.ts +++ b/test/ultrafixCiFailureRecovery.test.ts @@ -71,6 +71,7 @@ const mockGetCurrentPRHead = mock.fn(async () => prHead); await mock.module('@propr/core', { namedExports: { + reevaluatePRAutoMerge: async () => undefined, AgentRegistry: {}, logger: { info: () => {} }, loadUltrafixEscalationSettings: async () => ({ enabled: false, models: [], patience: 3, maxReasoningLevels: 2 }), diff --git a/test/ultrafixContinuationEntryPoint.test.ts b/test/ultrafixContinuationEntryPoint.test.ts index d95f287892..dc8d8c8e3f 100644 --- a/test/ultrafixContinuationEntryPoint.test.ts +++ b/test/ultrafixContinuationEntryPoint.test.ts @@ -39,6 +39,7 @@ let escalationEnabled = false; await mock.module('@propr/core', { namedExports: { + reevaluatePRAutoMerge: async () => undefined, AgentRegistry: {}, logger: { info: () => {} }, loadUltrafixEscalationSettings: async () => ({ enabled: escalationEnabled, models: [], patience: 3, maxReasoningLevels: 2 }), diff --git a/test/ultrafixRecoveryIntake.test.ts b/test/ultrafixRecoveryIntake.test.ts index 95cf6aea46..1b0cf1a0fc 100644 --- a/test/ultrafixRecoveryIntake.test.ts +++ b/test/ultrafixRecoveryIntake.test.ts @@ -131,6 +131,7 @@ const mockGetCurrentPRHead = mock.fn(async () => prHead); await mock.module('@propr/core', { namedExports: { + reevaluatePRAutoMerge: async () => undefined, AgentRegistry: {}, logger: coreLogger, handleError: () => {}, From 968ca3d66747ad53e0e5a21a152410e586cbf079 Mon Sep 17 00:00:00 2001 From: Rinalds Uzkalns Date: Tue, 20 Oct 2026 10:18:10 +0200 Subject: [PATCH 2/5] test: add reevaluatePRAutoMerge to the Ultrafix Redis integration core mock Co-Authored-By: Claude Opus 5.5 (1M context) --- test/ultrafixRecoveryRedisIntegration.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/ultrafixRecoveryRedisIntegration.test.ts b/test/ultrafixRecoveryRedisIntegration.test.ts index 854608aab0..a23336d168 100644 --- a/test/ultrafixRecoveryRedisIntegration.test.ts +++ b/test/ultrafixRecoveryRedisIntegration.test.ts @@ -58,6 +58,7 @@ let prHead = 'red-head'; await mock.module('@propr/core', { namedExports: { + reevaluatePRAutoMerge: async () => undefined, AgentRegistry: {}, logger: { info: () => {} }, loadUltrafixEscalationSettings: async () => ({ enabled: false, models: [], patience: 3, maxReasoningLevels: 2 }), From e4b7a8cc89227c74e900a770e51e16a100073b70 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:10:57 +0000 Subject: [PATCH 3/5] feat(ai): I've fixed F1 and F2. Both are in `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts`), plus its one caller in `checkRunHandler.ts`. I've fixed F1 and F2. Both are in `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts`), plus its one caller in `checkRunHandler.ts`. **F1: a failed CI request no longer counts as "no CI"** - The shared CI fetcher now also reports whether both requests (check runs and legacy statuses) succeeded. - If no CI shows up and either request failed, the answer is now `unavailable`, which blocks the merge. It no longer returns `no_ci`, and the 2-minute wait doesn't start. - The other callers (`getCheckRunsStatus`, `areAllChecksPassing`) ignore the new flag, so their behaviour is unchanged. **F2: the 2-minute wait starts when ProPR first sees the head** - The commit-date lookup is gone. A new helper, `observePRHead`, saves the head SHA and the time ProPR first saw it with no CI. It is stored in Redis per PR (`auto-merge:head-observed:::`, expiring after 30 days). - A later sweep run, a webhook, or another process reuses the same saved time, so it carries over between sweep runs. - A different head replaces the saved entry and restarts the wait. Returning to an earlier head also starts over rather than reusing the old time. - If two processes write at once, the saved time can only move later, so the wait never gets shorter. A Redis failure blocks the merge as `unavailable`. - `getCiMergeReadiness` now needs the PR number: the call is now `getCiMergeReadiness(owner, repo, ref, { prNumber })`. The Ultrafix fallback and the sweep both go through this one call site. **Tests** (`test/checkRunHandler.test.ts`) - One test each for the check-runs request failing and the status request failing while the other comes back empty. Both get `unavailable` and don't start the wait. With the F1 fix removed, both fail. - An old commit seen as the head for the first time still waits, and no commit lookup is made. - The saved time carries over between runs: still waiting at 2 minutes minus 1 ms, mergeable at 2 minutes. - A different head restarts the wait, and so does returning to an earlier one. - A broken Redis blocks the merge. `checkRunHandler`, the auto-merge fallback, the sweep and the related Ultrafix suites all pass (221 tests). Core lint with zero warnings, core `tsc`, and the root `tsc` all pass. **Side effects to know about:** - If the GitHub App can't read legacy commit statuses on a repo (the "Resource not accessible by integration" response), a repo with no CI now never merges through this path; it stays `unavailable`. That follows the rule that unreadable CI isn't evidence of no CI, but it is stricter than before. - The first sighting is only recorded once the earlier gates (labels, Ultrafix state, head match) pass. So the wait can start later than the actual push. That only makes it more cautious. There's nothing visual in this change, so I didn't capture any previews. PR: #3280 Comment by: @integry (ID: 6287455285) Model: Claude Opus 5.5 --- packages/core/src/webhook/checkRunHandler.ts | 2 +- packages/core/src/webhook/checkRunHelpers.ts | 70 ++++++++++++++---- test/checkRunHandler.test.ts | 77 +++++++++++++++++--- 3 files changed, 121 insertions(+), 28 deletions(-) diff --git a/packages/core/src/webhook/checkRunHandler.ts b/packages/core/src/webhook/checkRunHandler.ts index 043c04f235..de62999887 100644 --- a/packages/core/src/webhook/checkRunHandler.ts +++ b/packages/core/src/webhook/checkRunHandler.ts @@ -203,7 +203,7 @@ async function processPRAutoMerge(ctx: PRContext, headSha: string, options: { ig } // Passing CI, or no CI at all for this head, lets the merge proceed. - const ci = await getCiMergeReadiness(owner, repoName, headSha); + const ci = await getCiMergeReadiness(owner, repoName, headSha, { prNumber }); if (!ci.ready) { log.debug({ owner, repoName, prNumber, reason: ci.reason }, 'CI does not allow merging yet, skipping merge'); return; diff --git a/packages/core/src/webhook/checkRunHelpers.ts b/packages/core/src/webhook/checkRunHelpers.ts index 6a381f43ae..1f03e681ab 100644 --- a/packages/core/src/webhook/checkRunHelpers.ts +++ b/packages/core/src/webhook/checkRunHelpers.ts @@ -246,12 +246,13 @@ async function getCommitStatusInfo(octokit: any, owner: string, repoName: string /** * Fetch both CI signal types for a commit: check runs and legacy commit * statuses. Either may be unavailable; only when both fail is this an error. + * `complete` is false when one of the two requests failed. */ async function fetchCheckSignals( owner: string, repoName: string, ref: string, -): Promise<{ checkRuns: CheckRunRecord[]; commitStatus: CommitStatusInfo }> { +): Promise<{ checkRuns: CheckRunRecord[]; commitStatus: CommitStatusInfo; complete: boolean }> { const octokit = await getAuthenticatedOctokit(); const [checkRunsResult, commitStatusResult] = await Promise.allSettled([ octokit.request('GET /repos/{owner}/{repo}/commits/{ref}/check-runs', { @@ -286,7 +287,11 @@ async function fetchCheckSignals( ); } - return { checkRuns, commitStatus }; + return { + checkRuns, + commitStatus, + complete: checkRunsResult.status === 'fulfilled' && commitStatusResult.status === 'fulfilled', + }; } export interface CheckRunsStatus { @@ -457,38 +462,73 @@ export async function areAllChecksPassing( } } -/** How long a head may go without any CI signal before ProPR treats the repository as having no CI for it. */ +/** How long a PR head may go without any CI signal before ProPR treats the repository as having no CI for it. */ export const NO_CI_GRACE_MS = 2 * 60_000; +const HEAD_OBSERVATION_KEY_PREFIX = 'auto-merge:head-observed'; +const HEAD_OBSERVATION_TTL_SECONDS = 30 * 24 * 60 * 60; + +export type HeadObservationStore = Pick; + +/** + * When ProPR first saw `headSha` as the head of the PR without CI signal. The + * first call for a head records `now`; a different head starts a new + * observation. Concurrent writers can only move the observation later. + */ +export async function observePRHead( + store: HeadObservationStore, + head: { owner: string; repoName: string; prNumber: number; headSha: string }, + now: number, +): Promise { + const { owner, repoName, prNumber, headSha } = head; + const key = `${HEAD_OBSERVATION_KEY_PREFIX}:${owner}:${repoName}:${prNumber}`; + const raw = await store.get(key); + if (raw) { + const observation = JSON.parse(raw) as { sha?: unknown; observedAt?: unknown }; + if (observation.sha === headSha && typeof observation.observedAt === 'number' && Number.isFinite(observation.observedAt)) { + return observation.observedAt; + } + } + await store.set(key, JSON.stringify({ sha: headSha, observedAt: now }), 'EX', HEAD_OBSERVATION_TTL_SECONDS); + return now; +} + export interface CiMergeReadiness { ready: boolean; reason: 'checks_passing' | 'checks_not_passing' | 'no_ci' | 'waiting_for_ci' | 'unavailable'; } /** - * Whether CI allows merging `ref`: every blocking check passed, or no CI ran for - * it at all. A head with no signal counts as having no CI only once it is older - * than {@link NO_CI_GRACE_MS}, so checks that have not registered yet are not - * mistaken for missing CI. + * Whether CI allows merging `ref`, the current head of the PR: every blocking + * check passed, or no CI ran for it at all. No CI counts only when both CI + * endpoints answered, and only once ProPR has seen the head without signal for + * {@link NO_CI_GRACE_MS}, so checks that have not registered yet after a push + * are not mistaken for missing CI. The commit date says nothing about when it + * was pushed, so the grace runs from ProPR's own first observation. */ export async function getCiMergeReadiness( owner: string, repoName: string, ref: string, - options: { now?: number; loadNonBlockingChecks?: (owner: string, repo: string) => Promise } = {}, + options: { + /** The PR whose current head is `ref`. */ + prNumber: number; + now?: number; + loadNonBlockingChecks?: (owner: string, repo: string) => Promise; + observations?: HeadObservationStore; + }, ): Promise { - const { now = Date.now(), loadNonBlockingChecks = getNonBlockingChecksForRepository } = options; + const { prNumber, now = Date.now(), loadNonBlockingChecks = getNonBlockingChecksForRepository } = options; try { - const { checkRuns, commitStatus } = await fetchCheckSignals(owner, repoName, ref); + const { checkRuns, commitStatus, complete } = await fetchCheckSignals(owner, repoName, ref); if (checkRuns.length > 0 || commitStatus.totalCount > 0) { const summary = summarizeCheckSignals(checkRuns, commitStatus, await loadNonBlockingChecks(owner, repoName)); return summary.allPassing ? { ready: true, reason: 'checks_passing' } : { ready: false, reason: 'checks_not_passing' }; } - const octokit = await getAuthenticatedOctokit(); - const commit = await octokit.request('GET /repos/{owner}/{repo}/commits/{ref}', { owner, repo: repoName, ref }); - const committedAt = Date.parse(commit.data.commit.committer?.date ?? ''); - if (!Number.isFinite(committedAt)) return { ready: false, reason: 'unavailable' }; - return now - committedAt >= NO_CI_GRACE_MS ? { ready: true, reason: 'no_ci' } : { ready: false, reason: 'waiting_for_ci' }; + // An unreadable endpoint may hide CI; it is not evidence that there is none. + if (!complete) return { ready: false, reason: 'unavailable' }; + const observedAt = await observePRHead(options.observations ?? getUltrafixStateRedis(), { owner, repoName, prNumber, headSha: ref }, now); + return now - observedAt >= NO_CI_GRACE_MS ? { ready: true, reason: 'no_ci' } : { ready: false, reason: 'waiting_for_ci' }; } catch (error) { logger.warn({ owner, repoName, ref, error: (error as Error).message }, 'Failed to check CI merge readiness'); return { ready: false, reason: 'unavailable' }; diff --git a/test/checkRunHandler.test.ts b/test/checkRunHandler.test.ts index f38f59f69a..0ef67dd42b 100644 --- a/test/checkRunHandler.test.ts +++ b/test/checkRunHandler.test.ts @@ -437,45 +437,98 @@ describe('getCheckRunsStatus with nonBlockingChecks (Ultrafix readiness)', () => // ============= getCiMergeReadiness Tests ============= describe('getCiMergeReadiness', () => { - const committedAt = Date.parse('2026-10-19T23:46:00Z'); - function respond(checkRuns: unknown[], statusCount = 0): void { + const observedAt = Date.parse('2026-10-19T23:46:00Z'); + function respond(checkRuns: unknown[], statusCount = 0, fail: { checkRuns?: boolean; status?: boolean } = {}): void { mockOctokit.request.mock.mockImplementation(async (route: string) => { - if (route.endsWith('/check-runs')) return { data: { check_runs: checkRuns } }; - if (route.endsWith('/status')) return { data: { state: statusCount ? 'success' : 'pending', total_count: statusCount, statuses: [] } }; - if (route === 'GET /repos/{owner}/{repo}/commits/{ref}') return { data: { commit: { committer: { date: '2026-10-19T23:46:00Z' } } } }; + if (route.endsWith('/check-runs')) { + if (fail.checkRuns) throw new Error('check runs unavailable'); + return { data: { check_runs: checkRuns } }; + } + if (route.endsWith('/status')) { + if (fail.status) throw new Error('statuses unavailable'); + return { data: { state: statusCount ? 'success' : 'pending', total_count: statusCount, statuses: [] } }; + } throw new Error(`unexpected ${route}`); }); } + function observationStore(): { get: (key: string) => Promise; set: (key: string, value: string) => Promise<'OK'>; entries: Map } { + const entries = new Map(); + return { + entries, + get: async (key: string) => entries.get(key) ?? null, + set: async (key: string, value: string) => { entries.set(key, value); return 'OK'; }, + }; + } + function readiness(sha: string, now: number, observations = observationStore()): ReturnType { + return getCiMergeReadiness('owner', 'repo', sha, { prNumber: 7, now, loadNonBlockingChecks: async () => [], observations: observations as never }); + } test('passing checks allow the merge', async () => { resetMocks(); respond([{ name: 'build', status: 'completed', conclusion: 'success' }]); - assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt, loadNonBlockingChecks: async () => [] }), { ready: true, reason: 'checks_passing' }); + assert.deepStrictEqual(await readiness('sha', observedAt), { ready: true, reason: 'checks_passing' }); }); test('pending checks hold the merge', async () => { resetMocks(); respond([{ name: 'build', status: 'in_progress', conclusion: null }]); - assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt + NO_CI_GRACE_MS * 10, loadNonBlockingChecks: async () => [] }), { ready: false, reason: 'checks_not_passing' }); + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS * 10), { ready: false, reason: 'checks_not_passing' }); + }); + + test('a head with no CI at all is mergeable once it was seen without checks for the grace period', async () => { + resetMocks(); + respond([]); + const observations = observationStore(); + assert.deepStrictEqual(await readiness('sha', observedAt, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS - 1, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS, observations), { ready: true, reason: 'no_ci' }); + }); + + test('an old commit newly observed as the head still waits for checks to register', async () => { + resetMocks(); + respond([]); + // The commit itself may be days old; only ProPR's observation of the head counts, + // so no commit lookup is made at all. + assert.deepStrictEqual(await readiness('old-commit', observedAt + 10 * 24 * 60 * 60_000), { ready: false, reason: 'waiting_for_ci' }); + assert.ok(!mockOctokit.request.mock.calls.some(call => call.arguments[0] === 'GET /repos/{owner}/{repo}/commits/{ref}')); }); - test('a head with no CI at all is mergeable once checks had time to register', async () => { + test('a different head restarts the grace period', async () => { resetMocks(); respond([]); - assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt + NO_CI_GRACE_MS, loadNonBlockingChecks: async () => [] }), { ready: true, reason: 'no_ci' }); + const observations = observationStore(); + assert.deepStrictEqual(await readiness('first', observedAt, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('second', observedAt + NO_CI_GRACE_MS, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('second', observedAt + 2 * NO_CI_GRACE_MS - 1, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('second', observedAt + 2 * NO_CI_GRACE_MS, observations), { ready: true, reason: 'no_ci' }); + // Returning to an earlier head does not reuse its old observation. + assert.deepStrictEqual(await readiness('first', observedAt + 3 * NO_CI_GRACE_MS, observations), { ready: false, reason: 'waiting_for_ci' }); }); - test('a fresh head with no CI signal waits for checks to register', async () => { + test('an unreadable observation store never allows the merge', async () => { resetMocks(); respond([]); - assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt + NO_CI_GRACE_MS - 1, loadNonBlockingChecks: async () => [] }), { ready: false, reason: 'waiting_for_ci' }); + const broken = { get: async () => { throw new Error('redis down'); }, set: async () => 'OK' }; + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS * 10, broken as never), { ready: false, reason: 'unavailable' }); }); test('an API failure never allows the merge', async () => { resetMocks(); mockOctokit.request.mock.mockImplementation(async () => { throw new Error('boom'); }); - assert.deepStrictEqual(await getCiMergeReadiness('owner', 'repo', 'sha', { now: committedAt, loadNonBlockingChecks: async () => [] }), { ready: false, reason: 'unavailable' }); + assert.deepStrictEqual(await readiness('sha', observedAt), { ready: false, reason: 'unavailable' }); }); + + for (const failed of ['checkRuns', 'status'] as const) { + test(`a failed ${failed} request with the other endpoint empty is not evidence of no CI`, async () => { + resetMocks(); + respond([], 0, { [failed]: true }); + const observations = observationStore(); + assert.deepStrictEqual(await readiness('sha', observedAt, observations), { ready: false, reason: 'unavailable' }); + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS * 10, observations), { ready: false, reason: 'unavailable' }); + // An unreadable endpoint does not start the no-CI grace period either. + assert.equal(observations.entries.size, 0); + }); + } }); // ============= areAllChecksPassing Tests ============= From c1da9a4b37085cbbdb41e9222e16ee074ecc520f Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:41:16 +0000 Subject: [PATCH 4/5] feat(ai): I fixed F3 and F4. The fixes are in the shared direct-merge path, so the check-event merge, the Ultrafix fallback and the daemon sweep all get them. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I fixed F3 and F4. The fixes are in the shared direct-merge path, so the check-event merge, the Ultrafix fallback and the daemon sweep all get them. **F3: merge method.** Direct merges now use the same repository-supported method that arming uses. - `packages/core/src/services/autoMergeGate.ts` has a new export, `resolveDirectMergeMethod`. It returns the policy's method if one is set. Otherwise it uses the same lookup as `gateAutoMergeArming`: squash, unless the repository disallows squash, in which case merge commit, then rebase. - `processPRAutoMerge` in `packages/core/src/webhook/checkRunHandler.ts` calls it before `performMergeAndPostActions`, so a policy without a method no longer falls back to `squash`. - `processPRAutoMerge` is the only place that calls `performMergeAndPostActions`, so no other merge path has the same bug. **F4: stale no-CI observation.** Seeing CI on any head now ends the earlier no-CI grace period. - In `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts`), when the evaluated head has check runs or statuses, the PR's stored head observation is deleted. - So A without CI → B with CI → back to A records A again and waits a fresh 2 minutes. - Two concurrent sweeps can only make the observation later, never earlier, so the grace period can't be cut short. - One behaviour to know: if Redis is unavailable, that delete fails and the PR is reported `unavailable`, so passing checks don't merge either. That matches how an unreadable observation store already blocks merging. **New tests** - In `test/checkRunHandler.test.ts`: - The A (no CI) → B (pending CI) → A (no CI) case. - A check-event merge where the policy gives no method: the resolver's `merge` is what reaches GitHub's merge request. - In `packages/core/test/autoMergeGate.test.ts`: `resolveDirectMergeMethod` returns merge, rebase or squash depending on repository settings, and a configured method takes precedence. I reverted each fix temporarily and the matching new test failed, so they catch the original bugs. **Verification** - `checkRunHandler`: 120 pass. `autoMergeGate`: 30 pass. - `ultrafixAutoMergeFallback`, `autoMergeSweep`, `ultrafixCiFailureRecovery`, `ultrafixContinuationEntryPoint`, `ultrafixRecoveryIntake`, `taskIntentWithdrawal` and `e2e/webhook` all pass. - `tsc` on `packages/core` is clean. ESLint shows no errors or warnings on the changed source files; the test files are outside its config and were skipped, not linted. I found nothing else to report outside F3 and F4, and the change has no visible UI, so there are no previews. PR: #3280 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5 --- packages/core/src/services/autoMergeGate.ts | 17 ++++++++++ packages/core/src/webhook/checkRunHandler.ts | 9 ++++-- packages/core/src/webhook/checkRunHelpers.ts | 11 +++++-- packages/core/test/autoMergeGate.test.ts | 12 ++++++- test/checkRunHandler.test.ts | 33 ++++++++++++++++++-- 5 files changed, 74 insertions(+), 8 deletions(-) diff --git a/packages/core/src/services/autoMergeGate.ts b/packages/core/src/services/autoMergeGate.ts index 1f9eb7738a..0a617d1be2 100644 --- a/packages/core/src/services/autoMergeGate.ts +++ b/packages/core/src/services/autoMergeGate.ts @@ -172,6 +172,23 @@ async function resolveMergeMethod(octokit: AutoMergeGateOctokit, owner: string, return 'SQUASH'; } +/** + * The merge method for a direct merge: the policy's method, otherwise the same + * repository-supported default {@link gateAutoMergeArming} arms with. + */ +export async function resolveDirectMergeMethod(input: { + owner: string; repo: string; method?: AutoMergePolicyMethod; +}, deps: AutoMergeGateDependencies = {}): Promise { + if (input.method) return input.method; + let octokit: AutoMergeGateOctokit; + try { + octokit = await gateOctokit(deps); + } catch { + return 'squash'; + } + return (await resolveMergeMethod(octokit, input.owner, input.repo)).toLowerCase() as AutoMergePolicyMethod; +} + /** Find the task whose timeline records this decision. */ async function resolveDecisionTaskId(database: Knex, repository: string, numbers: number[]): Promise { const row = await database('tasks').where({ repository }).whereIn('issue_number', numbers) diff --git a/packages/core/src/webhook/checkRunHandler.ts b/packages/core/src/webhook/checkRunHandler.ts index de62999887..895b935f3f 100644 --- a/packages/core/src/webhook/checkRunHandler.ts +++ b/packages/core/src/webhook/checkRunHandler.ts @@ -25,7 +25,7 @@ import { import type { CheckRunEvent, CheckSuiteEvent } from '@octokit/webhooks-types'; import { getNonBlockingChecksForRepository } from '../daemon/configLoader.js'; import { isNonBlockingCheck } from './nonBlockingChecks.js'; -import { evaluatePullRequestAutoMerge } from '../services/autoMergeGate.js'; +import { evaluatePullRequestAutoMerge, resolveDirectMergeMethod } from '../services/autoMergeGate.js'; import type { AutoMergePolicyMethod } from '../workflow/autoMergePolicy.js'; export interface StatusEventPayload { @@ -253,9 +253,12 @@ async function processPRAutoMerge(ctx: PRContext, headSha: string, options: { ig return; } - log.info({ owner, repoName, prNumber, headSha, ci: ci.reason }, 'CI allows merging the auto-merge PR, attempting to merge'); + // A policy without a method merges like the arming gate would arm: squash, + // unless the repository disallows it. + const mergeMethod = await resolveDirectMergeMethod({ owner, repo: repoName, method: decision.method }); + log.info({ owner, repoName, prNumber, headSha, ci: ci.reason, mergeMethod }, 'CI allows merging the auto-merge PR, attempting to merge'); // GitHub refuses the merge if the head moved after the evaluation. - await performMergeAndPostActions(mergeCtx, decision.method, pullRequest.headSha); + await performMergeAndPostActions(mergeCtx, mergeMethod, pullRequest.headSha); } export interface AutoMergeReevaluationOptions { diff --git a/packages/core/src/webhook/checkRunHelpers.ts b/packages/core/src/webhook/checkRunHelpers.ts index 1f03e681ab..3c0cc26109 100644 --- a/packages/core/src/webhook/checkRunHelpers.ts +++ b/packages/core/src/webhook/checkRunHelpers.ts @@ -468,7 +468,11 @@ export const NO_CI_GRACE_MS = 2 * 60_000; const HEAD_OBSERVATION_KEY_PREFIX = 'auto-merge:head-observed'; const HEAD_OBSERVATION_TTL_SECONDS = 30 * 24 * 60 * 60; -export type HeadObservationStore = Pick; +export type HeadObservationStore = Pick; + +function headObservationKey(owner: string, repoName: string, prNumber: number): string { + return `${HEAD_OBSERVATION_KEY_PREFIX}:${owner}:${repoName}:${prNumber}`; +} /** * When ProPR first saw `headSha` as the head of the PR without CI signal. The @@ -481,7 +485,7 @@ export async function observePRHead( now: number, ): Promise { const { owner, repoName, prNumber, headSha } = head; - const key = `${HEAD_OBSERVATION_KEY_PREFIX}:${owner}:${repoName}:${prNumber}`; + const key = headObservationKey(owner, repoName, prNumber); const raw = await store.get(key); if (raw) { const observation = JSON.parse(raw) as { sha?: unknown; observedAt?: unknown }; @@ -522,6 +526,9 @@ export async function getCiMergeReadiness( try { const { checkRuns, commitStatus, complete } = await fetchCheckSignals(owner, repoName, ref); if (checkRuns.length > 0 || commitStatus.totalCount > 0) { + // The PR's head now has CI, so any earlier no-CI observation is over: + // a later return to that earlier head must wait out a fresh grace period. + await (options.observations ?? getUltrafixStateRedis()).del(headObservationKey(owner, repoName, prNumber)); const summary = summarizeCheckSignals(checkRuns, commitStatus, await loadNonBlockingChecks(owner, repoName)); return summary.allPassing ? { ready: true, reason: 'checks_passing' } : { ready: false, reason: 'checks_not_passing' }; } diff --git a/packages/core/test/autoMergeGate.test.ts b/packages/core/test/autoMergeGate.test.ts index ba236b1a64..0410a88dee 100644 --- a/packages/core/test/autoMergeGate.test.ts +++ b/packages/core/test/autoMergeGate.test.ts @@ -16,7 +16,7 @@ await mock.module('../src/auth/githubAuth.js', { namedExports: { getAuthenticate await mock.module('../src/config/planIssueManager.js', { namedExports: { findPlanIssueByRepoAndPR: async () => planIssue } }); const { - gateAutoMergeArming, reevaluateArmedAutoMergeOnNewHead, handleAutoMergePolicyPullRequestEvent, loadBaseAutoMergePolicy, recordAutoMergeDecisionEvent, + gateAutoMergeArming, resolveDirectMergeMethod, reevaluateArmedAutoMergeOnNewHead, handleAutoMergePolicyPullRequestEvent, loadBaseAutoMergePolicy, recordAutoMergeDecisionEvent, } = await import('../src/services/autoMergeGate.js'); type GateOctokit = import('../src/services/autoMergeGate.js').AutoMergeGateOctokit; @@ -143,6 +143,16 @@ test('without a configured method the repository default is used', async () => { assert.equal(result.mergeMethod, 'MERGE'); }); +test('a direct merge without a configured method uses the same repository default as arming', async () => { + const mergeOnly = fakeGitHub({ workflowByRef: {}, files: [], headSha: 'h', baseRef: 'main', headRef: 'f', repo: { allow_squash_merge: false, allow_merge_commit: true } }); + assert.equal(await resolveDirectMergeMethod({ owner: 'acme', repo: 'repo' }, { octokit: mergeOnly.octokit }), 'merge'); + const rebaseOnly = fakeGitHub({ workflowByRef: {}, files: [], headSha: 'h', baseRef: 'main', headRef: 'f', repo: { allow_squash_merge: false, allow_merge_commit: false } }); + assert.equal(await resolveDirectMergeMethod({ owner: 'acme', repo: 'repo' }, { octokit: rebaseOnly.octokit }), 'rebase'); + const defaults = fakeGitHub({ workflowByRef: {}, files: [], headSha: 'h', baseRef: 'main', headRef: 'f' }); + assert.equal(await resolveDirectMergeMethod({ owner: 'acme', repo: 'repo' }, { octokit: defaults.octokit }), 'squash'); + assert.equal(await resolveDirectMergeMethod({ owner: 'acme', repo: 'repo', method: 'rebase' }, { octokit: mergeOnly.octokit }), 'rebase', 'a configured method wins'); +}); + test('the skipped event names matching paths and finds the task by issue number', async () => { const github = fakeGitHub({ workflowByRef: {}, files: [{ filename: 'docs/new.md', previous_filename: '.propr/old.md' }], headSha: 'h', baseRef: 'main', headRef: 'f' }); const result = await gateAutoMergeArming({ owner: 'acme', repo: 'repo', prNumber: 70, opportunity: 'ultrafix_goal', issueNumber: 7, log }, { octokit: github.octokit, database }); diff --git a/test/checkRunHandler.test.ts b/test/checkRunHandler.test.ts index 0ef67dd42b..c7360de76a 100644 --- a/test/checkRunHandler.test.ts +++ b/test/checkRunHandler.test.ts @@ -132,8 +132,11 @@ const evaluateCurrentHead = async (input: { owner: string; repo: string; prNumbe pullRequest: { headSha: await getCurrentPRHead(input.owner, input.repo, input.prNumber) } as { headSha: string | null } | null, }); const mockEvaluatePullRequestAutoMerge = mock.fn(evaluateCurrentHead); +// Like the real resolver: the policy's method, else the repository default (squash here). +const resolveWithPolicyMethod = async (input: { method?: 'merge' | 'squash' | 'rebase' }) => input.method ?? 'squash'; +const mockResolveDirectMergeMethod = mock.fn(resolveWithPolicyMethod); await mock.module('../packages/core/src/services/autoMergeGate.js', { - namedExports: { evaluatePullRequestAutoMerge: mockEvaluatePullRequestAutoMerge } + namedExports: { evaluatePullRequestAutoMerge: mockEvaluatePullRequestAutoMerge, resolveDirectMergeMethod: mockResolveDirectMergeMethod } }); // Import the modules under test @@ -186,6 +189,8 @@ function resetMocks(): void { mockTriggerNextPendingIssue.mock.resetCalls(); mockEvaluatePullRequestAutoMerge.mock.resetCalls(); mockEvaluatePullRequestAutoMerge.mock.mockImplementation(evaluateCurrentHead); + mockResolveDirectMergeMethod.mock.resetCalls(); + mockResolveDirectMergeMethod.mock.mockImplementation(resolveWithPolicyMethod); resetUltrafixStateRedisForTests(); } @@ -451,12 +456,13 @@ describe('getCiMergeReadiness', () => { throw new Error(`unexpected ${route}`); }); } - function observationStore(): { get: (key: string) => Promise; set: (key: string, value: string) => Promise<'OK'>; entries: Map } { + function observationStore(): { get: (key: string) => Promise; set: (key: string, value: string) => Promise<'OK'>; del: (key: string) => Promise; entries: Map } { const entries = new Map(); return { entries, get: async (key: string) => entries.get(key) ?? null, set: async (key: string, value: string) => { entries.set(key, value); return 'OK'; }, + del: async (key: string) => Number(entries.delete(key)), }; } function readiness(sha: string, now: number, observations = observationStore()): ReturnType { @@ -505,6 +511,20 @@ describe('getCiMergeReadiness', () => { assert.deepStrictEqual(await readiness('first', observedAt + 3 * NO_CI_GRACE_MS, observations), { ready: false, reason: 'waiting_for_ci' }); }); + test('a head with CI in between restarts the grace period for an earlier head', async () => { + resetMocks(); + const observations = observationStore(); + respond([]); + assert.deepStrictEqual(await readiness('first', observedAt, observations), { ready: false, reason: 'waiting_for_ci' }); + respond([{ name: 'build', status: 'in_progress', conclusion: null }]); + assert.deepStrictEqual(await readiness('second', observedAt + NO_CI_GRACE_MS, observations), { ready: false, reason: 'checks_not_passing' }); + // The branch is reset to the first head after its original grace period passed. + respond([]); + assert.deepStrictEqual(await readiness('first', observedAt + 2 * NO_CI_GRACE_MS, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('first', observedAt + 3 * NO_CI_GRACE_MS - 1, observations), { ready: false, reason: 'waiting_for_ci' }); + assert.deepStrictEqual(await readiness('first', observedAt + 3 * NO_CI_GRACE_MS, observations), { ready: true, reason: 'no_ci' }); + }); + test('an unreadable observation store never allows the merge', async () => { resetMocks(); respond([]); @@ -1657,6 +1677,15 @@ describe('handleCheckRunEvent', () => { await handleCheckRunEvent(createMockCheckRunPayload({ headSha: 'abc123sha' }), 'test-correlation-id'); assert.equal(mergeCalls().length, 1); assert.equal((mergeCalls()[0].arguments as unknown as [string, { merge_method: string }])[1].merge_method, 'rebase'); + + // A policy without a method merges with the repository-supported method the arming gate would use. + resetMocks(); + mockOctokit.request.mock.mockImplementation(mockMergeablePR); + mockResolveDirectMergeMethod.mock.mockImplementation(async () => 'merge'); + await handleCheckRunEvent(createMockCheckRunPayload({ headSha: 'abc123sha' }), 'test-correlation-id'); + assert.deepEqual(mockResolveDirectMergeMethod.mock.calls[0].arguments[0], { owner: 'test-owner', repo: 'test-repo', method: undefined }); + assert.equal(mergeCalls().length, 1); + assert.equal((mergeCalls()[0].arguments as unknown as [string, { merge_method: string }])[1].merge_method, 'merge'); }); test('merges only the head the auto-merge policy evaluated', async () => { From e01f7707a9cb7ee46bc48f6dcef593d22570ee54 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:07:54 +0000 Subject: [PATCH 5/5] feat(ai): I fixed F5: a merge is now blocked whenever either CI endpoint fails to read, even if the other one shows passing checks. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I fixed F5: a merge is now blocked whenever either CI endpoint fails to read, even if the other one shows passing checks. **What was wrong:** in `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts:527`), the check for an incomplete read only ran after the "signals present" branch had already returned. So passing check runs with a failed status request (or the reverse) came back as `checks_passing`. **The fix:** `if (!complete) return { ready: false, reason: 'unavailable' }` now runs straight after the fetch, before both the passing-CI and no-CI checks. I removed the old copy further down. This function is the only CI gate on the direct-merge path, so the check-event merge, the Ultrafix fallback and the daemon sweep all pick up the fix. **Tests:** I added two regressions to the `getCiMergeReadiness` block in `test/checkRunHandler.test.ts`: - passing check runs while the status request fails → `unavailable` - passing statuses while the check-runs request fails → `unavailable` All 122 tests in `test/checkRunHandler.test.ts` pass. With the old source put back, the two new tests fail and nothing else does. The core package's ESLint (zero warnings allowed) and `tsc --noEmit` are clean. To run the suite I had to install dependencies with `npm ci` and build `packages/shared`; neither changes any repo files. **Separate finding, not changed:** two older functions, `areAllChecksPassing` and `getCheckRunsStatus` in the same file, also ignore `complete`. A failed endpoint there can still produce a passing result for the Ultrafix loop and the polling readiness checks. They behaved the same way before this PR and are outside its merge path, so I left them alone. They would need their own finding if you want them fixed. This change has nothing visible, so I made no preview. PR: #3280 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5 --- packages/core/src/webhook/checkRunHelpers.ts | 5 +++-- test/checkRunHandler.test.ts | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/core/src/webhook/checkRunHelpers.ts b/packages/core/src/webhook/checkRunHelpers.ts index 3c0cc26109..fcb0e9a6aa 100644 --- a/packages/core/src/webhook/checkRunHelpers.ts +++ b/packages/core/src/webhook/checkRunHelpers.ts @@ -525,6 +525,9 @@ export async function getCiMergeReadiness( const { prNumber, now = Date.now(), loadNonBlockingChecks = getNonBlockingChecksForRepository } = options; try { const { checkRuns, commitStatus, complete } = await fetchCheckSignals(owner, repoName, ref); + // An unreadable endpoint may hide pending or failing CI, so neither the + // passing signals of the other endpoint nor their absence prove anything. + if (!complete) return { ready: false, reason: 'unavailable' }; if (checkRuns.length > 0 || commitStatus.totalCount > 0) { // The PR's head now has CI, so any earlier no-CI observation is over: // a later return to that earlier head must wait out a fresh grace period. @@ -532,8 +535,6 @@ export async function getCiMergeReadiness( const summary = summarizeCheckSignals(checkRuns, commitStatus, await loadNonBlockingChecks(owner, repoName)); return summary.allPassing ? { ready: true, reason: 'checks_passing' } : { ready: false, reason: 'checks_not_passing' }; } - // An unreadable endpoint may hide CI; it is not evidence that there is none. - if (!complete) return { ready: false, reason: 'unavailable' }; const observedAt = await observePRHead(options.observations ?? getUltrafixStateRedis(), { owner, repoName, prNumber, headSha: ref }, now); return now - observedAt >= NO_CI_GRACE_MS ? { ready: true, reason: 'no_ci' } : { ready: false, reason: 'waiting_for_ci' }; } catch (error) { diff --git a/test/checkRunHandler.test.ts b/test/checkRunHandler.test.ts index c7360de76a..a6edf70af8 100644 --- a/test/checkRunHandler.test.ts +++ b/test/checkRunHandler.test.ts @@ -549,6 +549,18 @@ describe('getCiMergeReadiness', () => { assert.equal(observations.entries.size, 0); }); } + + test('passing check runs do not allow the merge while the status request fails', async () => { + resetMocks(); + respond([{ name: 'build', status: 'completed', conclusion: 'success' }], 0, { status: true }); + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS * 10), { ready: false, reason: 'unavailable' }); + }); + + test('passing statuses do not allow the merge while the check-runs request fails', async () => { + resetMocks(); + respond([], 1, { checkRuns: true }); + assert.deepStrictEqual(await readiness('sha', observedAt + NO_CI_GRACE_MS * 10), { ready: false, reason: 'unavailable' }); + }); }); // ============= areAllChecksPassing Tests =============