diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e146fde48..2f502e929 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/services/autoMergeGate.ts b/packages/core/src/services/autoMergeGate.ts index 1f9eb7738..0a617d1be 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 09d2faf0d..895b935f3 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, @@ -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 { @@ -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 +281,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 2cdb99e95..fcb0e9a6a 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,6 +462,87 @@ export async function areAllChecksPassing( } } +/** 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; + +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 + * 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 = headObservationKey(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`, 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: { + /** The PR whose current head is `ref`. */ + prNumber: number; + now?: number; + loadNonBlockingChecks?: (owner: string, repo: string) => Promise; + observations?: HeadObservationStore; + }, +): Promise { + 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. + 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' }; + } + 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' }; + } +} + export interface PRAutoMergeInfo { hasLabel: boolean; hasUltrafixLabel?: boolean; @@ -468,6 +554,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 +739,7 @@ export async function getPRAutoMergeInfo(owner: string, repoName: string, prNumb : 'unknown'; return { + state: prResponse.data.state, hasLabel, hasUltrafixLabel, hasActiveUltrafixLoop: ultrafixState?.active ?? false, @@ -843,13 +932,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 1200036a3..b5bbbcae6 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/packages/core/test/autoMergeGate.test.ts b/packages/core/test/autoMergeGate.test.ts index ba236b1a6..0410a88de 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/src/daemon.ts b/src/daemon.ts index 052d7268e..3d03aa3bb 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 000000000..5f807ba58 --- /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 000000000..bae83804f --- /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 11d28919d..0de2c5e62 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 a72ccd85c..8a31af1da 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 fa4d8b064..01c9b8e48 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 473f1795c..f46ab8f60 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 af5a04835..e9c46de3e 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 000000000..e1c98dd2f --- /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 24e37b3ff..a6edf70af 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 @@ -142,6 +145,8 @@ const { deleteBranch, getCurrentPRHead, areAllChecksPassing, + getCiMergeReadiness, + NO_CI_GRACE_MS, getCheckRunsStatus, getCheckRunsStatusForRepo, getPRAutoMergeInfo, @@ -184,6 +189,8 @@ function resetMocks(): void { mockTriggerNextPendingIssue.mock.resetCalls(); mockEvaluatePullRequestAutoMerge.mock.resetCalls(); mockEvaluatePullRequestAutoMerge.mock.mockImplementation(evaluateCurrentHead); + mockResolveDirectMergeMethod.mock.resetCalls(); + mockResolveDirectMergeMethod.mock.mockImplementation(resolveWithPolicyMethod); resetUltrafixStateRedisForTests(); } @@ -433,6 +440,129 @@ describe('getCheckRunsStatus with nonBlockingChecks (Ultrafix readiness)', () => }); }); +// ============= getCiMergeReadiness Tests ============= +describe('getCiMergeReadiness', () => { + 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')) { + 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'>; 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 { + 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 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 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 different head restarts the grace period', async () => { + resetMocks(); + respond([]); + 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 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([]); + 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 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); + }); + } + + 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 ============= describe('areAllChecksPassing', () => { @@ -1559,6 +1689,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 () => { diff --git a/test/ultrafixAutoMergeFallback.test.ts b/test/ultrafixAutoMergeFallback.test.ts new file mode 100644 index 000000000..f530b3248 --- /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 3e4414d0c..42f3e1975 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 d95f28789..dc8d8c8e3 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 95cf6aea4..1b0cf1a0f 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: () => {}, diff --git a/test/ultrafixRecoveryRedisIntegration.test.ts b/test/ultrafixRecoveryRedisIntegration.test.ts index 854608aab..a23336d16 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 }),