diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index c303851a..109ac7d3 100644 --- a/docs/implementation/guarded-merge.md +++ b/docs/implementation/guarded-merge.md @@ -8,7 +8,7 @@ Derived planted-review configurations clear the source GitHub binding because th Merging blocks when any plan item is unreviewed or stale, an attributed file is outside that item's declared plan scope, an Ambiguous or Unplanned segment remains, a current change request is open, or a `cmd:` acceptance check lacks a passing result for the current head. Approval does not override an out-of-scope file; the plan must be amended. Historical change requests from an earlier revision or snapshot remain visible but do not block the current revision. -The GitHub adapter reads the current PR base/head, effective branch rulesets, classic branch protection, required check contexts and app identities, and issue cross-references. It ignores review records that are not required checks. Cross-referenced PRs are read in one GraphQL request; repository identity is preserved with each PR number, and cross-repository references fail closed. More than 100 references also fail closed as unknown. Malformed pagination or PR references, partial referenced-PR records, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. An unreadable or incomplete rule source, a pending/missing/failing check, another open or merged PR for the issue, a conflict, or a closed PR blocks merging. +The GitHub adapter reads the current PR base/head, effective branch rulesets, classic branch protection, required check contexts and app identities, and issue cross-references. It ignores review records that are not required checks. The already-fixed check is the pre-PR check, `GhAlreadyFixedGateway` in `github/already-fixed.ts`, with the same rules (see “The check” in `pull-request-opening.md`): a closed issue, another open or merged PR that would close the issue or is linked manually, or a new commit on the base branch that mentions the issue. A PR that only mentions the issue does not count, in this repository or another; when the PR's base is not the default branch, a PR in this repository into that same base that references the issue counts too (decided 2026-09-30). The merge passes this PR as the task's own PR, matched by repository and number: it is excluded while open and counts once merged. Unlike the pre-PR check, the merge does not know the task's earlier PRs, so another open PR of the same task still counts. The commit scan starts at the PR's base commit on its base branch. Merging requires that commit to equal the reviewed snapshot's base, which is the base the pre-PR check scanned from. The merge passes no own commits, because the task's commits reach the base branch only by this merge. Any result the check cannot complete is unknown and blocks merging, including a base branch name outside the pattern the check accepts. The check starts as soon as the PR is read, alongside the rule reads, and stops 0.65 seconds before the inspection's deadline (`MERGE_CHECK_SETTLE_MS` in `github/merge.ts`: both grace periods of the merge's `gh` runner, which the check also uses, plus a margin), so its processes settle before the inspection ends; a check stopped that way is unknown. If the rule reads fail, the check is stopped and awaited before the inspection rejects. When the PR read ends too late to leave that much time, the check is not started and is unknown. A check answer that arrives after the check was stopped is unknown, and an inspection whose caller aborted or whose deadline passed rejects even when its reads answer late. An injected check must name the merge repository. The blocker names what the check found (at most five closes, PRs and commits, by repository, number, state and short SHA, with a closing commit's SHA shortened too; never a commit message) or why it could not finish. Once this PR has merged, the check counts it as the fix, so the coordinator shows only the `pr-state` blocker, not an already-fixed one. Malformed pagination, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. A caller's own abort during the already-fixed check waits for its processes like any other `gh` call of the gateway, up to 0.4 seconds. An unreadable or incomplete rule source, a pending/missing/failing check, an already-fixed match or unknown result, a conflict, or a closed PR blocks merging. Automatic merging also requires strict required status checks as a server-enforced current-base policy. A client-side fetch cannot supply that guarantee. Merge-queue branches were blocked in this increment because `gh pr merge` can report a successful enqueue before the pull request is merged. Queue support has since landed (#24; see `merge-queue.md`): a queue-capable adapter tracks each attempt until GitHub confirms the merge, removal or failure, and queue rules count as the server-side current-base guard. An adapter without queue inspection still fails closed with a `merge-queue` blocker. The coordinator performs the complete gate twice without using the display cache, verifies the same base/head pair, then re-reads the local review generation immediately before invoking `gh pr merge --match-head-commit` with literal argv. Each `gh` process gets only the allowlisted variables in `github/gh-env.ts`, plus fixed settings that turn off prompts, the pager, colour and update checks. No other server variables are passed on. A timeout or abort sends the `gh` process SIGTERM, then SIGKILL after a quarter of a second, and the call returns only after that process has exited (or 0.15 s after it exits, if a process it started keeps its output open). So a merge click aborted at its 14-second deadline settles by 14.4 s, inside the 14.5-second shutdown drain and below the 15-second request budget. Stopping `gh` cannot recall a merge request it had already sent: GitHub can still apply it, so a cancelled merge is reported as unknown and its attempt stays in flight, with the action disabled. A direct attempt is settled only when GitHub shows the pull request merged; a queued attempt also settles when GitHub reports it removed or failed (`merge-queue.md`). The adapter invalidates its pre-action cache before the command and after either success or refusal. A generation guard prevents inspections started before or during the command from restoring stale cache entries afterward. After command success, the server returns “merge submitted” even if its best-effort status reload fails, and the browser keeps the action disabled until Refresh confirms GitHub state. GitHub's command refusal text is returned to the local UI, and a failed attempt also remains disabled until Refresh loads fresh state. There is no “merge anyway” path around missing atomic protection. Demo mode never constructs the coordinator, including when a gateway is injected. @@ -16,8 +16,8 @@ Shutdown sets a terminal admission flag before inspecting active work and checks ## Deferred runner work -This slice does not mutate Git history or run plan commands. If the base moved, the gate sends the task back to review. A plan item with a `cmd:` acceptance check remains blocked because the current review service has no trusted container result. Issue #22 tracks rebasing through the ledger, recomputing review state, running commands in the container, and persisting results against the exact head. +This slice does not mutate Git history or run plan commands. If the base moved, the gate sends the task back to review. A plan item with a `cmd:` acceptance check remains blocked because the current review service has no trusted container result. Issue #22 tracks rebasing through the ledger, recomputing review state, running commands in the container, and persisting results against the exact head. The already-fixed commit scan starts at the PR's current base commit; once a rebase moves that base, the scan must still cover the base-branch commits the rebase brought under the PR since the last check. ## Validation -Unit and integration tests cover local review blockers; current and stale base/head pairs; PR state and mergeability; known, unknown, empty, pending, and failing check requirements; app-bound and malformed app identities; server base protection; merge-queue refusal for adapters without queue inspection; runtime merge-method validation; already-fixed results; a requirement changing between validation passes; deadline cancellation; post-action cache invalidation; and the exact `gh` merge argv. Browser coverage proves blockers are visible, the ready action uses the reviewed head, and duplicate clicks start only one merge. +Unit and integration tests cover local review blockers; current and stale base/head pairs; PR state and mergeability; known, unknown, empty, pending, and failing check requirements; app-bound and malformed app identities; server base protection; merge-queue refusal for adapters without queue inspection; runtime merge-method validation; the already-fixed check run through `GhAlreadyFixedGateway` (closing-only cross-references, the non-default-base rule, manual links, the closed state, base-branch commits, this PR excluded only while open, the merge inputs, fail-closed results and caller cancellation); a requirement changing between validation passes; deadline cancellation; post-action cache invalidation; and the exact `gh` merge argv. Browser coverage proves blockers are visible, the ready action uses the reviewed head, and duplicate clicks start only one merge. diff --git a/docs/implementation/pull-request-opening.md b/docs/implementation/pull-request-opening.md index 8c729123..e8f9dd47 100644 --- a/docs/implementation/pull-request-opening.md +++ b/docs/implementation/pull-request-opening.md @@ -33,6 +33,8 @@ The check matches when any of these is true: | Another open or merged PR links to the issue. | Cross-reference events that would close the issue (`willCloseTarget`: a closing keyword such as `Fixes #12`). GitHub closes issues only from PRs into the default branch, so `willCloseTarget` is false for every PR into another branch; when the task's base is not the default branch, a PR in this repository into that same base that references the issue counts too (decided 2026-09-30). Manual links: "connected" and "disconnected" events replayed in order. Both sides of a manual link are read, because which side GitHub reports as the subject depends on where the link was made; the linked PR is the side that is a PR, and a link between two PRs or to an unknown type makes the check `unknown`. | The task's own open PRs, matched by repository and number (its own merged PR is a match). Closed, unmerged PRs. A manual link whose latest event is a disconnect. A PR that only mentions the issue, in this repository or another (decided 2026-09-30: on cli/cli a third of open issues had such mentions, mostly merged PRs in unrelated repositories). | | A new commit on the base branch mentions the issue. | The commits from the task's base to the current base branch head. | Own commits. `#123` when the issue is `#12`. `other/repo#12`. A token inside a URL path or a longer path-like token (`https://example.com/GH-12`, `mirror/owner/repo#12`). | +The pre-merge check (`GhMergeGateway`, see `guarded-merge.md`) runs this same check, with the task's PR as its only own PR and the PR's base commit as the task base. + A commit mentions the issue with `#12`, `GH-12`, `owner/repo#12`, or the issue URL. A PR in another repository that would close the issue, or is linked manually, counts as a match. It is not excluded by number, because its number belongs to another repository. GitHub turns `willCloseTarget` false once the issue is closed, so a merged PR that closed the issue is reported through the closed state instead. An abandoned opening's PR has no recorded number, so the check cannot exclude it by number. The main path looks the branch up first and adds a visible PR's number to the own PRs. If GitHub's PR list still does not show that PR after the 10-minute settle time but the issue timeline already does, the check counts the task's own PR as another PR, and the task moves to possibly already fixed. That fails closed: a person sees the PR and continues. @@ -123,7 +125,6 @@ Some repositories do not support draft PRs (for example private repositories on - **Push.** `BranchPusher` is injected. The real push needs D's commit export (#66) and a runner-owned host repository. - **Continue from possibly already fixed.** The Continue and Cancel actions are user actions for a later slice. - **Close the draft on cancel.** The design closes the draft PR when a person cancels a needs-human task. The PR record is kept for that. -- **The pre-merge check.** `GhMergeGateway` keeps its own check for now. F6 moves it onto this module. The merge check treats a PR in another repository as `unknown`; this check treats it as a match. ## Tests diff --git a/github/merge.ts b/github/merge.ts index 81b5d58c..7b19d8cc 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,5 +1,7 @@ +import { GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedMatch } from './already-fixed.ts'; import { ghEnvironment } from './gh-env.ts'; import { runWithInput } from './run-with-input.ts'; +import { REPOSITORY, SHA } from './validate.ts'; /** * How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after @@ -8,6 +10,11 @@ import { runWithInput } from './run-with-input.ts'; * drain and below the 15-second serving request budget, and an inspection (at most 12 s) within 12.4 s. */ export const MERGE_KILL_GRACE_MS = 250, MERGE_PIPE_GRACE_MS = 150; +/** + * How long before an inspection's deadline the already-fixed check stops: both grace periods of this gateway's runner, + * which the check also uses, plus a margin. Its processes then settle before the deadline. + */ +export const MERGE_CHECK_SETTLE_MS = MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS + 250; /** The longest deadline of any inspection, and the default for merge-state and queue-state inspections. */ export const MERGE_INSPECTION_TIMEOUT_MS = 12_000; @@ -27,6 +34,8 @@ export interface RemoteMergeState { mergeQueue: boolean; requiredChecks: RequiredCheck[]; alreadyFixed: 'clear' | 'found' | 'unknown'; + /** Why the check is `found` (its matches) or `unknown` (its reason), for display. */ + alreadyFixedDetail?: string; /** The pull request's web URL, when GitHub reports a valid one. Display only; never used to decide readiness. */ url?: string; } @@ -62,8 +71,20 @@ export interface GhMergeConfig { method?: 'merge' | 'squash' | 'rebase'; } +type BranchRules = Pick; export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal }) => Promise; +/** + * The already-fixed matches as display text. Only validated fields are used (repository names, numbers, states, SHAs and + * the check's own close description), never a commit message, and at most five are named. + */ +function describeMatches(matches: readonly AlreadyFixedMatch[]): string { + const named = matches.slice(0, 5).map(match => match.kind === 'closed' ? `the issue was closed by ${match.by.replace(/^commit ([a-f0-9]{12})[a-f0-9]{28}\b/, 'commit $1')}` + : match.kind === 'pull request' ? `${match.repository}#${match.number} (${match.state.toLowerCase()}${match.draft ? ', draft' : ''})` + : `commit ${match.sha.slice(0, 12)} on the base branch`); + return named.join('; ') + (matches.length > 5 ? `; and ${matches.length - 5} more` : ''); +} + function confirmedMergeRefusal(message: string): boolean { return /required (?:approving )?review|required status check|branch protection|merge conflict|not mergeable|head (?:branch |commit )?(?:was )?(?:modified|changed)|does not match.*head|pull request.*(?:closed|draft)|merge method.*not allowed/i.test(message); } @@ -87,15 +108,20 @@ function timestamp(value: unknown, label: string): string { export class GhMergeGateway implements MergeGateway, MergeQueueGateway { readonly config: GhMergeConfig; readonly run: RunGh; + readonly checks: AlreadyFixedGateway; #cache: { expiresAt: number; state: RemoteMergeState } | null = null; #generation = 0; #inflight: { generation: number; promise: Promise } | null = null; - constructor(config: GhMergeConfig, run?: RunGh) { - if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(config.repository) || !Number.isSafeInteger(config.pullRequest) || config.pullRequest < 1 || !Number.isSafeInteger(config.issue) || config.issue < 1) + /** `checks` defaults to the pre-PR check's GitHub adapter for the same repository, using this gateway's runner. */ + constructor(config: GhMergeConfig, run?: RunGh, checks?: AlreadyFixedGateway) { + if (!REPOSITORY.test(config.repository) || !Number.isSafeInteger(config.pullRequest) || config.pullRequest < 1 || !Number.isSafeInteger(config.issue) || config.issue < 1) throw new Error('A GitHub repository, pull request, and issue are required for merging.'); if (config.method !== undefined && !['merge','squash','rebase'].includes(config.method)) throw new Error('GitHub merge method must be merge, squash, or rebase.'); + if (checks && checks.repository?.toLowerCase() !== config.repository.toLowerCase()) throw new Error('The already-fixed check must name and read the merge repository.'); this.config = config; this.run = run ?? ((args, options) => runWithInput('gh', args, { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment(), killGraceMs: MERGE_KILL_GRACE_MS, pipeGraceMs: MERGE_PIPE_GRACE_MS })); + // The check uses this gateway's runner, so its stopped `gh` calls settle within the same grace periods as the merge's. + this.checks = checks ?? new GhAlreadyFixedGateway({ repository: config.repository }, this.run); } async #json(args: readonly string[], signal?: AbortSignal): Promise { @@ -112,47 +138,63 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } } - async #alreadyFixed(signal?: AbortSignal): Promise<'clear' | 'found' | 'unknown'> { + /** + * The pre-merge "already fixed" check runs the pre-PR check (`github/already-fixed.ts`) with the same rules. This PR is + * the task's own PR: excluded while open, a match once merged. The base-branch scan starts at the PR's base commit, the + * base the merge is validated against; the task has no own commits on the base branch before its merge. + * The check stops early enough that its `gh` processes, which may need both grace periods after an abort, settle before + * the inspection's deadline; stopped that way it is `unknown`. + */ + async #alreadyFixed(baseBranch: string, base: string, deadlineAt: number, signal?: AbortSignal): Promise> { + // A check started with no settle time left could leave processes running past the inspection's deadline. + const runFor = deadlineAt - MERGE_CHECK_SETTLE_MS - Date.now(); + if (runFor <= 0) return { alreadyFixed: 'unknown', alreadyFixedDetail: 'No time was left to run the check.' }; + const stop = new AbortController(); + const timer = setTimeout(() => stop.abort(new Error('The already-fixed check did not finish in time.')), runFor); try { - const timeline = flattenPages(await this.#json(['api','--paginate','--slurp','-H','Accept: application/vnd.github+json',`repos/${this.config.repository}/issues/${this.config.issue}/timeline`], signal)); - const referenced = new Set(); - for (const event of timeline) { - if (!event || typeof event !== 'object' || Array.isArray(event)) throw new Error('GitHub returned a malformed timeline event.'); - const source = (event as { source?: { issue?: { number?: unknown; pull_request?: unknown; repository_url?: unknown } } }).source?.issue; - if (!source?.pull_request) continue; - if (source.repository_url !== `https://api.github.com/repos/${this.config.repository}`) throw new Error('GitHub returned a cross-repository or incomplete pull request reference.'); - if (!Number.isSafeInteger(source.number) || (source.number as number) < 1) throw new Error('GitHub returned an invalid pull request reference.'); - if (source.number !== this.config.pullRequest) referenced.add(source.number as number); - } - const numbers = [...referenced]; - if (numbers.length > 100) return 'unknown'; - if (!numbers.length) return 'clear'; - const [owner, name] = this.config.repository.split('/') as [string, string]; - const selections = numbers.map((number, index) => `p${index}: pullRequest(number:${number}) { state mergedAt }`).join(' '); - const response = await this.#json(['api','graphql','-f',`query=query { repository(owner:${JSON.stringify(owner)}, name:${JSON.stringify(name)}) { ${selections} } }`], signal) as { data?: { repository?: Record }; errors?: unknown }; - if (Object.hasOwn(response, 'errors') && (!Array.isArray(response.errors) || response.errors.length > 0)) return 'unknown'; - const pulls = response.data?.repository; - if (!pulls || Object.keys(pulls).length !== numbers.length) return 'unknown'; - for (let index = 0; index < numbers.length; index++) { - const pr = pulls[`p${index}`]; - if (!pr) return 'unknown'; - if (!['OPEN','CLOSED','MERGED'].includes(String(pr.state)) || !Object.hasOwn(pr, 'mergedAt') || (pr.mergedAt !== null && typeof pr.mergedAt !== 'string')) return 'unknown'; - if ((pr.state === 'OPEN' || pr.state === 'CLOSED') && pr.mergedAt !== null) return 'unknown'; - if (pr.state === 'MERGED' && typeof pr.mergedAt !== 'string') return 'unknown'; - if (pr.state === 'OPEN' || pr.state === 'MERGED') return 'found'; - } - return 'clear'; + const result = await this.checks.check({ issue: this.config.issue, taskBase: base, baseBranch, ownPullRequests: [this.config.pullRequest], ownCommits: new Set() }, + signal ? AbortSignal.any([signal, stop.signal]) : stop.signal); + // An answer that arrives after the check was stopped is not used. (One after a caller's abort is refused by the + // inspection itself, once the rule reads have ended.) + if (stop.signal.aborted) return { alreadyFixed: 'unknown', alreadyFixedDetail: 'The check did not finish in time.' }; + // `clear` removes this blocker, so a clear or found answer without its base head is malformed and counts as unknown. + const based = (result.outcome === 'clear' || result.outcome === 'found') && typeof result.baseHead === 'string' && SHA.test(result.baseHead); + if (based && result.outcome === 'clear') return { alreadyFixed: 'clear' }; + if (based && result.outcome === 'found' && Array.isArray(result.matches) && result.matches.length) return { alreadyFixed: 'found', alreadyFixedDetail: describeMatches(result.matches) }; + return { alreadyFixed: 'unknown', alreadyFixedDetail: result.outcome === 'unknown' && typeof result.reason === 'string' ? result.reason.slice(0, 300) : 'The check returned an invalid result.' }; } catch (error) { if (signal?.aborted) throw error; - return 'unknown'; - } + return { alreadyFixed: 'unknown', alreadyFixedDetail: stop.signal.aborted ? 'The check did not finish in time.' : 'GitHub could not be read.' }; + } finally { clearTimeout(timer); } } - async #inspectNow(signal?: AbortSignal): Promise { + async #inspectNow(deadlineAt: number, signal?: AbortSignal): Promise { const pr = await this.#json(['pr','view',String(this.config.pullRequest),'--repo',this.config.repository,'--json','baseRefName,baseRefOid,headRefName,headRefOid,state,mergeable,statusCheckRollup,url'], signal) as Record; if (typeof pr.baseRefName !== 'string' || typeof pr.headRefName !== 'string' || !['OPEN','CLOSED','MERGED'].includes(String(pr.state)) || !['MERGEABLE','CONFLICTING','UNKNOWN'].includes(String(pr.mergeable)) || !Array.isArray(pr.statusCheckRollup)) throw new Error('GitHub returned an incomplete pull request state.'); - const branch = encodeURIComponent(pr.baseRefName); + const base = fullSha(pr.baseRefOid, 'base SHA'), head = fullSha(pr.headRefOid, 'head SHA'); + // The already-fixed check needs only the PR's base, so it runs alongside the rule reads. If those fail, the check is + // stopped; it is awaited on every path, so no `gh` process it started outlives the inspection. + const failed = new AbortController(); + const alreadyFixed = this.#alreadyFixed(pr.baseRefName, base, deadlineAt, signal ? AbortSignal.any([signal, failed.signal]) : failed.signal); + alreadyFixed.catch(() => {}); + let rules: BranchRules; + try { rules = await this.#rules(pr.baseRefName, pr.statusCheckRollup as Array>, signal); } + catch (error) { failed.abort(error); await alreadyFixed.catch(() => {}); throw error; } + const fixed = await alreadyFixed; + // The rule reads turn a failed read into rulesKnown: false, so a read that ends after an abort must not yield a state. + signal?.throwIfAborted(); + return { + base, head, + pullRequestState: pr.state as RemoteMergeState['pullRequestState'], mergeable: pr.mergeable as RemoteMergeState['mergeable'], + ...rules, ...fixed, + ...(typeof pr.url === 'string' && pr.url.length <= 2048 && /^https:\/\//.test(pr.url) ? { url: pr.url } : {}), + }; + } + + /** The effective branch rules and the state of each required check on the PR. */ + async #rules(baseRefName: string, observed: Array>, signal?: AbortSignal): Promise { + const branch = encodeURIComponent(baseRefName); let rulesKnown = true; let rules: unknown[] = []; let classic: unknown | null = null; @@ -218,7 +260,6 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } } } - const observed = pr.statusCheckRollup as Array>; const requiredChecks = [...requirements.values()].map(required => { const match = observed.find(check => { const context = typeof check.name === 'string' ? check.name : check.context; @@ -239,12 +280,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } return { ...required, state }; }); - return { - base: fullSha(pr.baseRefOid, 'base SHA'), head: fullSha(pr.headRefOid, 'head SHA'), - pullRequestState: pr.state as RemoteMergeState['pullRequestState'], mergeable: pr.mergeable as RemoteMergeState['mergeable'], - rulesKnown, atomicBaseGuard, mergeQueue, requiredChecks, alreadyFixed: await this.#alreadyFixed(signal), - ...(typeof pr.url === 'string' && pr.url.length <= 2048 && /^https:\/\//.test(pr.url) ? { url: pr.url } : {}), - }; + return { rulesKnown, atomicBaseGuard, mergeQueue, requiredChecks }; } async inspect(options: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal } = {}): Promise { @@ -253,12 +289,13 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { if (!options.fresh && this.#cache && this.#cache.expiresAt > Date.now()) return this.#cache.state; const generation = this.#generation; if (!options.fresh && this.#inflight?.generation === generation) return this.#inflight.promise; - const timeout = new AbortController(); + const timeout = new AbortController(), deadlineAt = Date.now() + timeoutMs; const timer = setTimeout(() => timeout.abort(new Error('GitHub merge-state inspection timed out.')), timeoutMs); const signal = options.signal ? AbortSignal.any([options.signal, timeout.signal]) : timeout.signal; - const attempt = this.#inspectNow(signal).catch(error => { - if (timeout.signal.aborted) throw timeout.signal.reason; - if (options.signal?.aborted) throw options.signal.reason; + const attempt = this.#inspectNow(deadlineAt, signal).catch(error => { + // The combined signal keeps the reason of whichever abort came first, the caller's or the deadline's; a stopped gh + // call can settle after both have fired. + if (signal.aborted) throw signal.reason; throw error; }).then(state => { if (this.#generation === generation) this.#cache = { expiresAt: Date.now() + 5_000, state }; diff --git a/runner/merge.ts b/runner/merge.ts index a27b8858..37894cb0 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -111,8 +111,13 @@ export class MergeCoordinator { if (remote.mergeQueue && !queueGateway(this.gateway)) blockers.push({ code: 'merge-queue', message: 'This GitHub adapter cannot verify the merge-queue lifecycle.' }); if (!remote.atomicBaseGuard) blockers.push({ code: 'base-guard', message: 'GitHub does not expose a server-enforced guard for the validated base.' }); for (const check of remote.requiredChecks) if (check.state !== 'success') blockers.push({ code: 'check', message: `${check.context} is ${check.state}.` }); - if (remote.alreadyFixed === 'found') blockers.push({ code: 'already-fixed', message: 'Another open or merged pull request references this issue.' }); - if (remote.alreadyFixed === 'unknown') blockers.push({ code: 'already-fixed', message: 'The already-fixed check could not be completed.' }); + // Once this PR has merged, the check counts it as the fix; the pr-state blocker already says so. + if (remote.pullRequestState !== 'MERGED' && remote.alreadyFixed === 'found') blockers.push({ code: 'already-fixed', message: remote.alreadyFixedDetail + ? `The issue may already be fixed: ${remote.alreadyFixedDetail}.` + : 'The issue may already be fixed: it is closed, another open or merged pull request refers to it, or a new base-branch commit mentions it.' }); + if (remote.pullRequestState !== 'MERGED' && remote.alreadyFixed === 'unknown') blockers.push({ code: 'already-fixed', message: remote.alreadyFixedDetail + ? `The already-fixed check could not be completed: ${remote.alreadyFixedDetail}` + : 'The already-fixed check could not be completed.' }); let attempt = this.#attempt(); if (attempt?.kind === 'direct' && attempt.state === 'submitting') { diff --git a/test/merge.test.ts b/test/merge.test.ts index bb969ee6..1ede078b 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -2,12 +2,39 @@ import { randomUUID } from 'node:crypto'; import { expect, it, vi } from 'vitest'; import { ReviewService } from '../runner/review.ts'; import { MergeCoordinator, MergeNotApplied, MergeOutcomeUnknown } from '../runner/merge.ts'; -import { GhMergeGateway, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type RemoteMergeState } from '../github/merge.ts'; +import { GhMergeGateway, MERGE_CHECK_SETTLE_MS, MERGE_KILL_GRACE_MS, MERGE_PIPE_GRACE_MS, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type RemoteMergeState } from '../github/merge.ts'; import { Store, mergeActionResponse } from '../runner/store.ts'; +import { GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedInput, type AlreadyFixedResult } from '../github/already-fixed.ts'; import { GuardRefusal } from '../runner/lifecycle.ts'; type ReviewView = ReturnType; const sha = (digit: string) => digit.repeat(40); +const xref = (number: number, willCloseTarget: boolean, over: Record = {}) => ({ __typename: 'CrossReferencedEvent', willCloseTarget, + source: { __typename: 'PullRequest', number, state: 'OPEN', isDraft: false, baseRefName: 'main', repository: { nameWithOwner: 'owner/repo' }, ...over } }); +interface IssueFake { nodes?: unknown[]; state?: 'OPEN' | 'CLOSED'; defaultBranch?: string; totalCount?: number; errors?: unknown; commits?: { sha: string; message: string }[]; compareStatus?: string } +const issueTimeline = (fake: IssueFake = {}) => JSON.stringify({ ...(fake.errors !== undefined ? { errors: fake.errors } : {}), data: { repository: { + nameWithOwner: 'owner/repo', defaultBranchRef: { name: fake.defaultBranch ?? 'main' }, issue: { state: fake.state ?? 'OPEN', + timelineItems: { totalCount: fake.totalCount ?? (fake.nodes ?? []).length, pageInfo: { hasNextPage: false }, nodes: fake.nodes ?? [] } } } } }); +/** Answers the reads of the already-fixed check (`github/already-fixed.ts`): the issue timeline, the base branch ref, and the base comparison. */ +function alreadyFixedReads(args: readonly string[], fake: IssueFake = {}): string | null { + const joined = args.join(' '); + if (args[1] === 'graphql' && joined.includes('timelineItems(first: ')) return issueTimeline(fake); + const ref = /git\/ref\/heads\/(\S+)$/.exec(joined); + if (ref) return JSON.stringify({ ref: `refs/heads/${ref[1]}`, object: { sha: sha('9') } }); + const commits = fake.commits ?? []; + if (joined.includes('/compare/')) return JSON.stringify({ status: fake.compareStatus ?? (commits.length ? 'ahead' : 'identical'), total_commits: commits.length, + commits: commits.map(commit => ({ sha: commit.sha, commit: { message: commit.message } })) }); + return null; +} +/** Answers the merge adapter's own reads: an open PR 7 into main with no rules or protection. */ +function mergeReads(args: readonly string[], pull: Record = {}): string { + const joined = args.join(' '); + if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [], ...pull }); + if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); + if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); + if (/\/branches\/[^/]+$/.test(joined)) return JSON.stringify({ protected: false }); + throw new Error(`Unexpected gh call: ${joined}`); +} function readyView(): ReviewView { return { items: [{ id: 'P1', state: 'approved', outside: [], acceptance: [{ type: 'check', text: 'Works' }], checks: { tests: '– No tests defined' } }], @@ -82,6 +109,18 @@ it.each([ expect(status.blockers.map(blocker => blocker.code)).toContain(code); }); +it('does not report the issue as already fixed by another change once this PR has merged', async () => { + const view = readyView(); + for (const alreadyFixed of ['found', 'unknown'] as const) { + const merged = await new MergeCoordinator(serviceFor(view), gateway([remote(view, { pullRequestState: 'MERGED', alreadyFixed })])).status(view); + expect(merged.blockers.map(blocker => blocker.code)).toEqual(['pr-state']); + const open = await new MergeCoordinator(serviceFor(view), gateway([remote(view, { alreadyFixed })])).status(view); + expect(open.blockers.map(blocker => blocker.code)).toEqual(['already-fixed']); + } + const found = await new MergeCoordinator(serviceFor(view), gateway([remote(view, { alreadyFixed: 'found' })])).status(view); + expect(found.blockers[0]!.message).toMatch(/closed.*pull request refers to it.*commit mentions it/); +}); + it('blocks command acceptance that has no current passing runner result', async () => { const view = readyView(); const changed = { ...view, items: view.items.map((item, index) => index ? item : { ...item, acceptance: [{ type: 'cmd' as const, text: 'npm test' }] }) }; @@ -855,7 +894,7 @@ it('parses required checks from both rule sources and pins the gh merge head', a if (joined.includes('/rules/branches/')) return JSON.stringify([[{ type: 'merge_queue' }, { type: 'required_status_checks', parameters: { strict_required_status_checks_policy: true, required_status_checks: [{ context: 'test', integration_id: 10 }, { context: 'race', integration_id: null }] } }]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: true }); if (joined.includes('/protection')) return JSON.stringify({ required_status_checks: { strict: false, checks: [{ context: 'lint', app_id: null }] } }); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } if (joined.startsWith('pr merge 7')) return ''; throw new Error(`Unexpected gh call: ${joined}`); }; @@ -1086,7 +1125,7 @@ it.each([ if (joined.includes('/rules/branches/')) return JSON.stringify([rules]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: requiredStatusChecks }); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); @@ -1129,7 +1168,7 @@ it.each([[false, true], [true, false]])('treats a protection 404 with protected= if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: protectedBranch }); if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); @@ -1137,53 +1176,6 @@ it.each([[false, true], [true, false]])('treats a protection 404 with protected= expect(state.requiredChecks).toEqual([]); }); -it.each([['feature', 'found'], ['other-branch', 'found']] as const)('classifies a referenced PR on %s as %s', async (referencedBranch, expected) => { - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.startsWith('api graphql')) return JSON.stringify({ data: { repository: { p0: { state: 'OPEN', mergedAt: null, headRefName: referencedBranch } } } }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[{ source: { issue: { number: 7, pull_request: {}, repository_url: 'https://api.github.com/repos/owner/repo' } } }, { source: { issue: { number: 8, pull_request: {}, repository_url: 'https://api.github.com/repos/owner/repo' } } }]]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe(expected); -}); - -it('fails closed for a pull request reference from another repository', async () => { - let graphReads = 0; - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.startsWith('api graphql')) { graphReads++; return JSON.stringify({ data: { repository: {} } }); } - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[{ source: { issue: { number: 7, pull_request: {}, repository_url: 'https://api.github.com/repos/other/repo' } } }]]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); - expect(graphReads).toBe(0); -}); - -it.each([{}, { state: 'CLOSED' }, { state: 'BOGUS', mergedAt: null }, { state: 'CLOSED', mergedAt: 42 }, { state: 'MERGED', mergedAt: null }, { state: 'OPEN', mergedAt: '2026-01-01' }, { state: 'CLOSED', mergedAt: '2026-01-01' }])('fails closed for malformed referenced PR data: %j', async referencedPull => { - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.startsWith('api graphql')) return JSON.stringify({ data: { repository: { p0: referencedPull } } }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[{ source: { issue: { number: 8, pull_request: {}, repository_url: 'https://api.github.com/repos/owner/repo' } } }]]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); -}); - it.each([false, 'required', []])('fails closed for malformed classic protection metadata: %j', async requiredStatusChecks => { const run = async (args: readonly string[]) => { const joined = args.join(' '); @@ -1191,7 +1183,7 @@ it.each([false, 'required', []])('fails closed for malformed classic protection if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: requiredStatusChecks }); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); @@ -1205,7 +1197,7 @@ it('fails closed for a ruleset entry without a type', async () => { if (joined.includes('/rules/branches/')) return JSON.stringify([[{}]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: { strict: true, checks: [] } }); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); @@ -1219,26 +1211,272 @@ it('does not treat empty strict check policies as an atomic base guard', async ( if (joined.includes('/rules/branches/')) return JSON.stringify([[{ type: 'required_status_checks', parameters: { strict_required_status_checks_policy: true, required_status_checks: [] } }]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: { strict: true, checks: [], contexts: [] } }); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); expect(state).toMatchObject({ rulesKnown: true, atomicBaseGuard: false, requiredChecks: [] }); }); -it('fails closed when GraphQL returns referenced PR data with errors', async () => { +function mergeRun(fake: IssueFake = {}, pull: Record = {}) { + const calls: string[][] = []; + const run = async (args: readonly string[]) => { calls.push([...args]); return alreadyFixedReads(args, fake) ?? mergeReads(args, pull); }; + return { calls, alreadyFixed: async () => (await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect()).alreadyFixed }; +} + +it('counts a cross-reference before merging only when it would close the issue', async () => { + // A mention, in this repository or another, is not a fix (decided 2026-09-30). + expect(await mergeRun({ nodes: [xref(8, false), xref(9, false, { state: 'MERGED', repository: { nameWithOwner: 'other/repo' } })] }).alreadyFixed()).toBe('clear'); + expect(await mergeRun({ nodes: [xref(8, true)] }).alreadyFixed()).toBe('found'); + expect(await mergeRun({ nodes: [xref(8, true, { state: 'MERGED' })] }).alreadyFixed()).toBe('found'); + expect(await mergeRun({ nodes: [xref(8, true, { state: 'CLOSED' })] }).alreadyFixed()).toBe('clear'); + // A closing PR in another repository is a match, not unknown. Its number is not this PR's, even when the digits are. + expect(await mergeRun({ nodes: [xref(7, true, { repository: { nameWithOwner: 'other/repo' } })] }).alreadyFixed()).toBe('found'); +}); + +it('excludes this pull request before merging only while it is open', async () => { + expect(await mergeRun({ nodes: [xref(7, true)] }).alreadyFixed()).toBe('clear'); + expect(await mergeRun({ nodes: [xref(7, true, { repository: { nameWithOwner: 'Owner/Repo' } })] }).alreadyFixed()).toBe('clear'); + expect(await mergeRun({ nodes: [xref(7, true, { state: 'MERGED' })] }).alreadyFixed()).toBe('found'); +}); + +it('before merging into a base that is not the default branch, counts a PR here into that base that references the issue', async () => { + const develop = { baseRefName: 'develop' }, intoDevelop = xref(8, false, { baseRefName: 'develop' }); + expect(await mergeRun({ nodes: [intoDevelop] }, develop).alreadyFixed()).toBe('found'); + expect(await mergeRun({ nodes: [xref(8, false)] }, develop).alreadyFixed()).toBe('clear'); + expect(await mergeRun({ nodes: [xref(8, false, { baseRefName: 'develop', repository: { nameWithOwner: 'other/repo' } })] }, develop).alreadyFixed()).toBe('clear'); + // Into the default branch, or when the merge's base is the default branch, a mention stays a mention. + expect(await mergeRun({ nodes: [intoDevelop] }).alreadyFixed()).toBe('clear'); + expect(await mergeRun({ nodes: [intoDevelop], defaultBranch: 'develop' }, develop).alreadyFixed()).toBe('clear'); +}); + +it('before merging, counts a manual link, a close and a new base-branch commit that mentions the issue', async () => { + const linked = { __typename: 'PullRequest', number: 8, state: 'OPEN', isDraft: false, repository: { nameWithOwner: 'owner/repo' } }, issue = { __typename: 'Issue' }; + expect(await mergeRun({ nodes: [{ __typename: 'ConnectedEvent', source: issue, subject: linked }] }).alreadyFixed()).toBe('found'); + expect(await mergeRun({ nodes: [{ __typename: 'ConnectedEvent', source: linked, subject: issue }] }).alreadyFixed()).toBe('found'); + expect(await mergeRun({ nodes: [{ __typename: 'ConnectedEvent', source: issue, subject: linked }, { __typename: 'DisconnectedEvent', source: issue, subject: linked }] }).alreadyFixed()).toBe('clear'); + const closer = { __typename: 'PullRequest', number: 9, repository: { nameWithOwner: 'owner/repo' } }; + expect(await mergeRun({ state: 'CLOSED', nodes: [{ __typename: 'ClosedEvent', closer }] }).alreadyFixed()).toBe('found'); + expect(await mergeRun({ commits: [{ sha: sha('c'), message: 'Fix #21' }] }).alreadyFixed()).toBe('found'); + expect(await mergeRun({ commits: [{ sha: sha('c'), message: 'Fix #210' }] }).alreadyFixed()).toBe('clear'); +}); + +it('runs the merge check from the PR base on its base branch and asks for no field beyond the repo scope', async () => { + const { calls, alreadyFixed } = mergeRun({}, { baseRefName: 'develop', baseRefOid: sha('c') }); + expect(await alreadyFixed()).toBe('clear'); + const paths = calls.map(call => call.find(arg => arg.startsWith('repos/')) ?? ''); + expect(paths).toContain('repos/owner/repo/git/ref/heads/develop'); + expect(paths).toContain(`repos/owner/repo/compare/${sha('c')}...${sha('9')}?per_page=100&page=1`); + expect(paths.some(path => path.includes('/timeline'))).toBe(false); + const graphql = calls.find(call => call[1] === 'graphql')!; + expect(graphql).toContain('number=21'); + // Any field on ProjectV2 needs the read:project scope, and GitHub then refuses the whole query. + expect(graphql.find(arg => arg.startsWith('query='))).not.toMatch(/on ProjectV2/); +}); + +it.each([ + ['GraphQL errors', { errors: [{ message: 'partial' }] }], + ['more timeline events than one page', { totalCount: 101 }], + ['a PR base that is not an ancestor of the base branch', { compareStatus: 'diverged' }], + ['a malformed timeline event', { nodes: [null] }], +] as const)('fails the merge check closed for %s', async (_case, fake) => { + expect(await mergeRun(fake as IssueFake).alreadyFixed()).toBe('unknown'); +}); + +it('gives an injected check the merge inputs and fails closed on an unknown outcome or error', async () => { + const inputs: AlreadyFixedInput[] = []; + const answers: Array<() => AlreadyFixedResult> = [() => ({ outcome: 'clear', baseHead: sha('9') }), () => ({ outcome: 'bogus' } as unknown as AlreadyFixedResult), () => { throw new Error('HTTP 502'); }]; + const checks: AlreadyFixedGateway = { repository: 'Owner/Repo', check: async input => { inputs.push(input); return answers.shift()!(); } }; + const run = async (args: readonly string[]) => mergeReads(args, { baseRefName: 'release/1', baseRefOid: sha('c') }); + const client = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, checks); + expect((await client.inspect({ fresh: true })).alreadyFixed).toBe('clear'); + expect(inputs[0]).toEqual({ issue: 21, taskBase: sha('c'), baseBranch: 'release/1', ownPullRequests: [7], ownCommits: new Set() }); + expect((await client.inspect({ fresh: true })).alreadyFixed).toBe('unknown'); + expect((await client.inspect({ fresh: true })).alreadyFixed).toBe('unknown'); + expect(() => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, { ...checks, repository: 'other/repo' })).toThrow(/merge repository/); + expect(() => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, { check: checks.check })).toThrow(/merge repository/); +}); + +it('keeps the caller cancellation when the merge check is aborted', async () => { + const controller = new AbortController(); + let started!: () => void; + const checking = new Promise(resolve => { started = resolve; }); + const checks: AlreadyFixedGateway = { repository: 'owner/repo', check: (_input, signal) => new Promise((_resolve, reject) => { + started(); signal?.addEventListener('abort', () => reject(new Error('generic runner abort')), { once: true }); + }) }; + const run = async (args: readonly string[]) => mergeReads(args); + const pending = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, checks).inspect({ fresh: true, signal: controller.signal }); + await checking; + controller.abort(new Error('merge request deadline exceeded')); + await expect(pending).rejects.toThrow('merge request deadline exceeded'); +}); + +it('stops the merge check early enough for its processes to settle before the inspection deadline', async () => { + vi.useFakeTimers(); + try { + let checkSignal: AbortSignal | undefined; + const settle = MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS; + // Like the real runner, the check settles only after both grace periods once it is aborted. + const checks: AlreadyFixedGateway = { repository: 'owner/repo', check: (_input, signal) => new Promise((_resolve, reject) => { + checkSignal = signal; + signal?.addEventListener('abort', () => setTimeout(() => reject(new Error('aborted')), settle), { once: true }); + }) }; + const run = async (args: readonly string[]) => mergeReads(args); + const inspection = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, checks).inspect({ fresh: true, timeoutMs: 6_000 }); + const settled = inspection.then(state => ({ state }), error => ({ error })); + await vi.advanceTimersByTimeAsync(6_000 - MERGE_CHECK_SETTLE_MS - 1); + expect(checkSignal?.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(checkSignal?.aborted).toBe(true); + await vi.advanceTimersByTimeAsync(MERGE_CHECK_SETTLE_MS - 1); + // The inspection resolves before its deadline, with the check unknown, instead of overrunning it. + expect(await Promise.race([settled, Promise.resolve('pending')])).toMatchObject({ state: { alreadyFixed: 'unknown', alreadyFixedDetail: 'The check did not finish in time.' } }); + } finally { vi.useRealTimers(); } +}); + +it('gives the default merge check the gateway runner, whose grace periods the early stop is built from', () => { + const config = { repository: 'owner/repo', pullRequest: 7, issue: 21 }; + const plain = new GhMergeGateway(config); + expect(plain.checks).toBeInstanceOf(GhAlreadyFixedGateway); + // The check's own default runner waits 1.5 s after an abort; the merge's waits 0.4 s, and MERGE_CHECK_SETTLE_MS assumes it. + expect((plain.checks as GhAlreadyFixedGateway).run).toBe(plain.run); + const run = async () => ''; + expect((new GhMergeGateway(config, run).checks as GhAlreadyFixedGateway).run).toBe(run); +}); + +it('starts the merge check alongside the rule reads, and stops and awaits it when they fail', async () => { + let checkStarted!: () => void, checkSignal: AbortSignal | undefined, checkSettled = false; + const started = new Promise(resolve => { checkStarted = resolve; }); + const checks: AlreadyFixedGateway = { repository: 'owner/repo', check: (_input, signal) => new Promise((_resolve, reject) => { + checkSignal = signal; checkStarted(); + signal?.addEventListener('abort', () => setTimeout(() => { checkSettled = true; reject(new Error('aborted')); }, 20), { once: true }); + }) }; + const rollup: unknown[] = []; const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.startsWith('api graphql')) return JSON.stringify({ data: { repository: { p0: { state: 'CLOSED', mergedAt: null } } }, errors: [{ message: 'partial' }] }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[{ source: { issue: { number: 8, pull_request: {}, repository_url: 'https://api.github.com/repos/owner/repo' } } }]]); - throw new Error(`Unexpected gh call: ${joined}`); + // The rule read answers only once the check has started: run one after the other, this inspection would hang. + if (args.join(' ').includes('/rules/branches/')) { await started; return JSON.stringify([[{ type: 'required_status_checks', parameters: { strict_required_status_checks_policy: true, required_status_checks: [{ context: 'test', integration_id: null }] } }]]); } + return mergeReads(args, { statusCheckRollup: rollup }); }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); + // A null rollup entry makes the required-check matching throw after the rule reads. + rollup.push(null); + await expect(new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, checks).inspect({ fresh: true })).rejects.toThrow(TypeError); + expect(checkSignal?.aborted).toBe(true); + expect(checkSettled).toBe(true); +}); + +it('reports why the merge check matched or could not finish, without commit messages', async () => { + const closer = { __typename: 'PullRequest', number: 9, repository: { nameWithOwner: 'owner/repo' } }; + const found = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => alreadyFixedReads(args, { + state: 'CLOSED', nodes: [{ __typename: 'ClosedEvent', closer }, xref(8, true, { isDraft: true })], commits: [{ sha: sha('c'), message: 'Fix #21 ' }], + }) ?? mergeReads(args)).inspect(); + expect(found).toMatchObject({ alreadyFixed: 'found', alreadyFixedDetail: `the issue was closed by owner/repo#9; owner/repo#8 (open, draft); commit ${sha('c').slice(0, 12)} on the base branch` }); + const unknown = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => alreadyFixedReads(args, { totalCount: 101 }) ?? mergeReads(args)).inspect(); + expect(unknown).toMatchObject({ alreadyFixed: 'unknown', alreadyFixedDetail: expect.stringMatching(/more than 100 linking events/) }); + // A closing commit is named by its short SHA too. + const byCommit = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => alreadyFixedReads(args, { + state: 'CLOSED', nodes: [{ __typename: 'ClosedEvent', closer: { __typename: 'Commit', oid: sha('d') } }] }) ?? mergeReads(args)).inspect(); + expect(byCommit.alreadyFixedDetail).toBe(`the issue was closed by commit ${sha('d').slice(0, 12)}`); + // At most five matches are named. + const many = Array.from({ length: 7 }, (_, index) => xref(10 + index, true)); + const capped = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => alreadyFixedReads(args, { nodes: many }) ?? mergeReads(args)).inspect(); + expect(capped.alreadyFixedDetail).toBe('owner/repo#10 (open); owner/repo#11 (open); owner/repo#12 (open); owner/repo#13 (open); owner/repo#14 (open); and 2 more'); +}); + +it('names the already-fixed detail in the merge blocker', async () => { + const view = readyView(); + const found = await new MergeCoordinator(serviceFor(view), gateway([remote(view, { alreadyFixed: 'found', alreadyFixedDetail: 'owner/repo#8 (open)' })])).status(view); + expect(found.blockers).toEqual([{ code: 'already-fixed', message: 'The issue may already be fixed: owner/repo#8 (open).' }]); + const unknown = await new MergeCoordinator(serviceFor(view), gateway([remote(view, { alreadyFixed: 'unknown', alreadyFixedDetail: 'The check did not finish in time.' })])).status(view); + expect(unknown.blockers).toEqual([{ code: 'already-fixed', message: 'The already-fixed check could not be completed: The check did not finish in time.' }]); +}); + +it('does not accept a check answer or rule reads that arrive after an abort', async () => { + vi.useFakeTimers(); + try { + // A check that ignores its abort and answers clear after it. + const late: AlreadyFixedGateway = { repository: 'owner/repo', check: (_input, signal) => new Promise(resolve => { + signal?.addEventListener('abort', () => setTimeout(() => resolve({ outcome: 'clear', baseHead: sha('9') }), 10), { once: true }); + }) }; + const stopped = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => mergeReads(args), late).inspect({ fresh: true, timeoutMs: 6_000 }); + const stoppedResult = stopped.then(state => ({ state }), error => ({ error })); + await vi.advanceTimersByTimeAsync(6_000 - MERGE_CHECK_SETTLE_MS + 20); + expect(await stoppedResult).toMatchObject({ state: { alreadyFixed: 'unknown', alreadyFixedDetail: 'The check did not finish in time.' } }); + + // A caller abort: the late clear answer must not turn into a resolved inspection. + const controller = new AbortController(); + const cancelled = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => mergeReads(args), late).inspect({ fresh: true, signal: controller.signal }); + const cancelledResult = cancelled.then(state => ({ state }), error => ({ error })); + await vi.advanceTimersByTimeAsync(100); + controller.abort(new Error('merge request deadline exceeded')); + await vi.advanceTimersByTimeAsync(20); + expect(await cancelledResult).toMatchObject({ error: { message: 'merge request deadline exceeded' } }); + + // Rule reads that answer after the caller aborted, with a check that has already answered. + const quick: AlreadyFixedGateway = { repository: 'owner/repo', check: async () => ({ outcome: 'clear', baseHead: sha('9') }) }; + const rulesAbort = new AbortController(); + const slowRules = async (args: readonly string[]) => { + if (args.join(' ').includes('/rules/branches/')) await new Promise(resolve => setTimeout(resolve, 1_000)); + return mergeReads(args); + }; + const afterRules = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, slowRules, quick).inspect({ fresh: true, signal: rulesAbort.signal }); + const afterRulesResult = afterRules.then(state => ({ state }), error => ({ error })); + await vi.advanceTimersByTimeAsync(100); + rulesAbort.abort(new Error('merge request deadline exceeded')); + await vi.advanceTimersByTimeAsync(1_000); + expect(await afterRulesResult).toMatchObject({ error: { message: 'merge request deadline exceeded' } }); + } finally { vi.useRealTimers(); } +}); + +it('shortens only a closing commit SHA, not a repository name that looks like one', async () => { + const hexRepo = `owner/${'a'.repeat(40)}`; + const closer = { __typename: 'PullRequest', number: 9, repository: { nameWithOwner: hexRepo } }; + const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => alreadyFixedReads(args, { + state: 'CLOSED', nodes: [{ __typename: 'ClosedEvent', closer }] }) ?? mergeReads(args)).inspect(); + expect(state.alreadyFixedDetail).toBe(`the issue was closed by ${hexRepo}#9`); +}); + +it('does not start the merge check when no time is left for its processes to settle', async () => { + vi.useFakeTimers(); + try { + let checks = 0; + const counted: AlreadyFixedGateway = { repository: 'owner/repo', check: async () => { checks++; return { outcome: 'clear', baseHead: sha('9') }; } }; + // The PR read ends after the point where the check would have to stop. + const slowPull = async (args: readonly string[]) => { + if (args.join(' ').startsWith('pr view 7')) await new Promise(resolve => setTimeout(resolve, 6_000 - MERGE_CHECK_SETTLE_MS + 10)); + return mergeReads(args); + }; + const inspection = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, slowPull, counted).inspect({ fresh: true, timeoutMs: 6_000 }); + const result = inspection.then(state => ({ state }), error => ({ error })); + await vi.advanceTimersByTimeAsync(6_000 - MERGE_CHECK_SETTLE_MS + 20); + expect(await result).toMatchObject({ state: { alreadyFixed: 'unknown', alreadyFixedDetail: 'No time was left to run the check.' } }); + expect(checks).toBe(0); + } finally { vi.useRealTimers(); } +}); + +it('fails the merge check closed on a clear or found answer without a valid base head', async () => { + const answers: unknown[] = [{ outcome: 'clear' }, { outcome: 'clear', baseHead: 'main' }, { outcome: 'found', matches: [{ kind: 'commit', sha: sha('c'), subject: 'x' }] }, { outcome: 'clear', baseHead: sha('9') }]; + const checks: AlreadyFixedGateway = { repository: 'owner/repo', check: async () => answers.shift() as AlreadyFixedResult }; + const client = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, async args => mergeReads(args), checks); + for (let index = 0; index < 3; index++) + expect(await client.inspect({ fresh: true })).toMatchObject({ alreadyFixed: 'unknown', alreadyFixedDetail: 'The check returned an invalid result.' }); + expect((await client.inspect({ fresh: true })).alreadyFixed).toBe('clear'); +}); + +it('keeps the caller cancellation reason when the inspection deadline also passes before gh settles', async () => { + vi.useFakeTimers(); + try { + const controller = new AbortController(); + // Like the real runner, a stopped gh call settles only after its grace periods, here past the inspection deadline. + const run = async (_args: readonly string[], options?: { signal?: AbortSignal }) => new Promise((_resolve, reject) => { + options?.signal?.addEventListener('abort', () => setTimeout(() => reject(new Error('generic runner abort')), 400), { once: true }); + }); + const inspection = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect({ fresh: true, timeoutMs: 1_000, signal: controller.signal }); + const result = inspection.then(state => ({ state }), error => ({ error })); + await vi.advanceTimersByTimeAsync(900); + controller.abort(new Error('merge request deadline exceeded')); + await vi.advanceTimersByTimeAsync(500); + expect(await result).toMatchObject({ error: { message: 'merge request deadline exceeded' } }); + } finally { vi.useRealTimers(); } }); it('does not let an inspection started before merge repopulate the cache', async () => { @@ -1252,94 +1490,20 @@ it('does not let an inspection started before merge repopulate the cache', async if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline') && pullReads === 1) { markTimelineStarted(); return delayedTimeline; } - if (joined.includes('/timeline')) return JSON.stringify([[]]); + if (args[1] === 'graphql' && pullReads === 1) { markTimelineStarted(); return delayedTimeline; } + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const client = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run); const staleInspection = client.inspect(); await timelineStarted; await client.merge(sha('b')); - releaseTimeline(JSON.stringify([[]])); + releaseTimeline(issueTimeline()); await staleInspection; await client.inspect(); expect(pullReads).toBe(2); }); -it('blocks the already-fixed check instead of truncating more than 100 references', async () => { - const references = Array.from({ length: 101 }, (_, index) => ({ source: { issue: { number: index + 8, pull_request: {}, repository_url: 'https://api.github.com/repos/owner/repo' } } })); - let referencedViews = 0; - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.startsWith('pr view')) { referencedViews++; return JSON.stringify({ state: 'CLOSED', mergedAt: null, headRefName: 'other' }); } - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([references]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); - expect(referencedViews).toBe(0); -}); - -it('fails closed when a paginated timeline contains a malformed page', async () => { - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([{ source: { issue: { number: 8, pull_request: {} } } }]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); -}); - -it('fails closed when a timeline pull request reference has no valid number', async () => { - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[{ source: { issue: { number: '8', pull_request: {}, repository_url: 'https://api.github.com/repos/owner/repo' } } }]]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); -}); - -it('fails closed when a timeline contains a non-object event', async () => { - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[null]]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); -}); - -it('fails closed when a timeline contains an array event', async () => { - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: false }); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - if (joined.includes('/timeline')) return JSON.stringify([[[]]]); - throw new Error(`Unexpected gh call: ${joined}`); - }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); -}); - it.each([ { strict: true, checks: 'bad', contexts: [] }, { strict: true, checks: [], contexts: ['test'] }, @@ -1352,7 +1516,7 @@ it.each([ if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); if (/branches\/main$/.test(joined)) return JSON.stringify({ protected: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: requiredStatusChecks }); - if (joined.includes('/timeline')) return JSON.stringify([[]]); + { const fixed = alreadyFixedReads(args); if (fixed !== null) return fixed; } throw new Error(`Unexpected gh call: ${joined}`); }; const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect();