From bd070d46febb3b01656ac7c2166d6f2b37b0f804 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 30 Sep 2026 21:21:16 -0700 Subject: [PATCH 1/9] F6 (part): run the pre-merge already-fixed check through GhAlreadyFixedGateway GhMergeGateway's own REST-timeline check counted every same-repository PR that mentioned the issue and returned unknown for any cross-repository reference. The merge now runs the pre-PR check with the same rules (decided 2026-09-30): a cross-reference counts only when it would close the issue; with a non-default base, a PR here into that base that references the issue also counts; manual links, the closed state and new base-branch commits count. This PR stays the task's own PR, excluded only while open. The scan starts at the PR's base commit, and the check stops early enough for its gh processes to settle before the inspection deadline. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 6 +- docs/implementation/pull-request-opening.md | 3 +- github/merge.ts | 65 ++--- runner/merge.ts | 2 +- test/merge.test.ts | 301 +++++++++++--------- 5 files changed, 202 insertions(+), 175 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index c16e77a4..43e54fa2 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 stops 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS`: both grace periods of its `gh` runner plus a margin), so its processes settle before the inspection ends; a check stopped that way is unknown. 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 still waits for its processes, up to 1.5 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. 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 0a0596dd..8baeea07 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,5 +1,6 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; +import { CHECK_KILL_GRACE_MS, CHECK_PIPE_GRACE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway } from './already-fixed.ts'; const runFile = promisify(execFile); @@ -56,6 +57,9 @@ export interface GhMergeConfig { export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal }) => Promise; +/** How long before an inspection's deadline the already-fixed check stops: both grace periods, plus a margin. */ +export const CHECK_SETTLE_MS = CHECK_KILL_GRACE_MS + CHECK_PIPE_GRACE_MS + 250; + 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); } @@ -79,14 +83,18 @@ 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) { + /** `checks` defaults to the pre-PR check's GitHub adapter for the same repository, using `run` when one is given. */ + constructor(config: GhMergeConfig, run?: RunGh, checks?: AlreadyFixedGateway) { 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) 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?.repository !== undefined && checks.repository.toLowerCase() !== config.repository.toLowerCase()) throw new Error('The already-fixed check must read the merge repository.'); this.config = config; + this.checks = checks ?? new GhAlreadyFixedGateway({ repository: config.repository }, run); this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal })).stdout); } @@ -104,43 +112,27 @@ 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 { + const stop = new AbortController(); + const timer = setTimeout(() => stop.abort(new Error('The already-fixed check did not finish in time.')), Math.max(0, deadlineAt - CHECK_SETTLE_MS - Date.now())); 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); + return result.outcome === 'clear' || result.outcome === 'found' ? result.outcome : 'unknown'; } catch (error) { if (signal?.aborted) throw error; return 'unknown'; - } + } 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.'); @@ -231,10 +223,11 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } return { ...required, state }; }); + const base = fullSha(pr.baseRefOid, 'base SHA'); return { - base: fullSha(pr.baseRefOid, 'base SHA'), head: fullSha(pr.headRefOid, 'head SHA'), + base, 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), + rulesKnown, atomicBaseGuard, mergeQueue, requiredChecks, alreadyFixed: await this.#alreadyFixed(pr.baseRefName, base, deadlineAt, signal), ...(typeof pr.url === 'string' && pr.url.length <= 2048 && /^https:\/\//.test(pr.url) ? { url: pr.url } : {}), }; } @@ -245,10 +238,10 @@ 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 => { + const attempt = this.#inspectNow(deadlineAt, signal).catch(error => { if (timeout.signal.aborted) throw timeout.signal.reason; if (options.signal?.aborted) throw options.signal.reason; throw error; diff --git a/runner/merge.ts b/runner/merge.ts index f255fb12..17b0971f 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -109,7 +109,7 @@ 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 === 'found') blockers.push({ code: 'already-fixed', message: 'The issue may already be fixed: it is closed, another open or merged pull request links to it, or a new base-branch commit mentions it.' }); if (remote.alreadyFixed === 'unknown') blockers.push({ code: 'already-fixed', message: 'The already-fixed check could not be completed.' }); let attempt = this.#attempt(); diff --git a/test/merge.test.ts b/test/merge.test.ts index bb969ee6..8c08ad2c 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -2,12 +2,30 @@ 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 { CHECK_SETTLE_MS, GhMergeGateway, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type RemoteMergeState } from '../github/merge.ts'; import { Store, mergeActionResponse } from '../runner/store.ts'; +import { CHECK_KILL_GRACE_MS, CHECK_PIPE_GRACE_MS, 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; +} function readyView(): ReviewView { return { items: [{ id: 'P1', state: 'approved', outside: [], acceptance: [{ type: 'check', text: 'Works' }], checks: { tests: '– No tests defined' } }], @@ -855,7 +873,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 +1104,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 +1147,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 +1155,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 +1162,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 +1176,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,125 +1190,187 @@ 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[]) => { - 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' }] }); + calls.push([...args]); 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 }); + const fixed = alreadyFixedReads(args, fake); + if (fixed !== null) return fixed; 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' } } }]]); + if (/\/branches\/[^/]+$/.test(joined)) return JSON.stringify({ protected: false }); 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'); -}); + return { calls, alreadyFixed: async () => (await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect()).alreadyFixed }; +} -it('does not let an inspection started before merge repopulate the cache', async () => { - let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; - const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); - const delayedTimeline = new Promise(resolve => { releaseTimeline = resolve; }); - const run = async (args: readonly string[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view')) { pullReads++; return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); } - if (joined.startsWith('pr merge')) return ''; - 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([[]]); - 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([[]])); - await staleInspection; - await client.inspect(); - expect(pullReads).toBe(2); +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('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; +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[]) => { 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.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'release/1', baseRefOid: sha('c'), 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([references]); - throw new Error(`Unexpected gh call: ${joined}`); + return JSON.stringify({ protected: false }); }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); - expect(referencedViews).toBe(0); + 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/); }); -it('fails closed when a paginated timeline contains a malformed page', async () => { +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 = { 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[]) => { 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.startsWith('pr view 7')) 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}`); + return JSON.stringify({ protected: false }); }; - const state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); + 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('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('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 = CHECK_KILL_GRACE_MS + CHECK_PIPE_GRACE_MS; + // Like the real runner, the check settles only after both grace periods once it is aborted. + const checks: AlreadyFixedGateway = { 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[]) => { + 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.includes('/rules/branches/')) return JSON.stringify([[]]); + if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); + return JSON.stringify({ protected: false }); + }; + 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 - CHECK_SETTLE_MS - 1); + expect(checkSignal?.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(checkSignal?.aborted).toBe(true); + await vi.advanceTimersByTimeAsync(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' } }); + } finally { vi.useRealTimers(); } }); -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('gives the default merge check its own hardened runner, or the injected one', () => { + const config = { repository: 'owner/repo', pullRequest: 7, issue: 21 }; + const plain = new GhMergeGateway(config); + expect(plain.checks).toBeInstanceOf(GhAlreadyFixedGateway); + expect((plain.checks as GhAlreadyFixedGateway).run).not.toBe(plain.run); + const run = async () => ''; + expect((new GhMergeGateway(config, run).checks as GhAlreadyFixedGateway).run).toBe(run); }); -it('fails closed when a timeline contains an array event', async () => { +it('does not let an inspection started before merge repopulate the cache', async () => { + let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; + const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); + const delayedTimeline = new Promise(resolve => { releaseTimeline = resolve; }); 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.startsWith('pr view')) { pullReads++; return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); } + if (joined.startsWith('pr merge')) return ''; 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([[[]]]); + 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 state = await new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run).inspect(); - expect(state.alreadyFixed).toBe('unknown'); + const client = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run); + const staleInspection = client.inspect(); + await timelineStarted; + await client.merge(sha('b')); + releaseTimeline(issueTimeline()); + await staleInspection; + await client.inspect(); + expect(pullReads).toBe(2); }); it.each([ @@ -1352,7 +1385,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(); From 76c271b2349da37184f1652a109bb6dc378209a0 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:24:48 -0700 Subject: [PATCH 2/9] Close local review round 2 on F6 merge check: run it alongside the rule reads - Start the already-fixed check as soon as the PR is read, in parallel with the rule reads; stop and await it if those reads fail. A fresh inspection fell from about 3.4 s to 2.1 s on cli/cli, leaving the check its time. - Move CHECK_SETTLE_MS next to the grace periods it is built from. - Require an injected check to name the merge repository. - Show no already-fixed blocker once this PR has merged. - Blocker text: "refers to it" also covers a reference into a non-default base. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- github/already-fixed.ts | 2 ++ github/merge.ts | 38 +++++++++++++++--------- runner/merge.ts | 5 ++-- test/merge.test.ts | 44 +++++++++++++++++++++++++--- 5 files changed, 70 insertions(+), 21 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 43e54fa2..aa5c0781 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. 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 stops 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS`: both grace periods of its `gh` runner plus a margin), so its processes settle before the inspection ends; a check stopped that way is unknown. 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 still waits for its processes, up to 1.5 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. +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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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. An injected check must name the merge repository. 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 still waits for its processes, up to 1.5 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. 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. diff --git a/github/already-fixed.ts b/github/already-fixed.ts index 35ec68b6..42e5b905 100644 --- a/github/already-fixed.ts +++ b/github/already-fixed.ts @@ -45,6 +45,8 @@ export const DEFAULT_CHECK_DEADLINE_MS = 12_000; * The deadline plus both stays below the 15-second serving request budget (12 + 1 + 0.5 = 13.5 s). */ export const CHECK_KILL_GRACE_MS = 1_000, CHECK_PIPE_GRACE_MS = 500; +/** A caller that must settle by its own deadline stops the check this long before it: both grace periods, plus a margin. */ +export const CHECK_SETTLE_MS = CHECK_KILL_GRACE_MS + CHECK_PIPE_GRACE_MS + 250; const PAGE = 100; const TIMELINE_QUERY = `query($owner: String!, $name: String!, $number: Int!) { diff --git a/github/merge.ts b/github/merge.ts index 8baeea07..cf0e2608 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,6 +1,6 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; -import { CHECK_KILL_GRACE_MS, CHECK_PIPE_GRACE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway } from './already-fixed.ts'; +import { CHECK_SETTLE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway } from './already-fixed.ts'; const runFile = promisify(execFile); @@ -55,11 +55,9 @@ export interface GhMergeConfig { method?: 'merge' | 'squash' | 'rebase'; } +type BranchRules = Pick; export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal }) => Promise; -/** How long before an inspection's deadline the already-fixed check stops: both grace periods, plus a margin. */ -export const CHECK_SETTLE_MS = CHECK_KILL_GRACE_MS + CHECK_PIPE_GRACE_MS + 250; - 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); } @@ -92,7 +90,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { 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) 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?.repository !== undefined && checks.repository.toLowerCase() !== config.repository.toLowerCase()) throw new Error('The already-fixed check must read the merge repository.'); + 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.checks = checks ?? new GhAlreadyFixedGateway({ repository: config.repository }, run); this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal })).stdout); @@ -136,7 +134,26 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { 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; } + return { + base, head, + pullRequestState: pr.state as RemoteMergeState['pullRequestState'], mergeable: pr.mergeable as RemoteMergeState['mergeable'], + ...rules, alreadyFixed: await alreadyFixed, + ...(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; @@ -202,7 +219,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; @@ -223,13 +239,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } return { ...required, state }; }); - const base = fullSha(pr.baseRefOid, 'base SHA'); - return { - base, 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(pr.baseRefName, base, deadlineAt, 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 { diff --git a/runner/merge.ts b/runner/merge.ts index 17b0971f..47324bef 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -109,8 +109,9 @@ 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: 'The issue may already be fixed: it is closed, another open or merged pull request links to it, or a new base-branch commit mentions it.' }); - 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: '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: '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 8c08ad2c..9b8f1651 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -2,9 +2,9 @@ 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 { CHECK_SETTLE_MS, GhMergeGateway, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type RemoteMergeState } from '../github/merge.ts'; +import { GhMergeGateway, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type RemoteMergeState } from '../github/merge.ts'; import { Store, mergeActionResponse } from '../runner/store.ts'; -import { CHECK_KILL_GRACE_MS, CHECK_PIPE_GRACE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedInput, type AlreadyFixedResult } from '../github/already-fixed.ts'; +import { CHECK_KILL_GRACE_MS, CHECK_PIPE_GRACE_MS, CHECK_SETTLE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedInput, type AlreadyFixedResult } from '../github/already-fixed.ts'; import { GuardRefusal } from '../runner/lifecycle.ts'; type ReviewView = ReturnType; @@ -100,6 +100,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' }] }) }; @@ -1288,13 +1300,14 @@ it('gives an injected check the merge inputs and fails closed on an unknown outc 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 = { check: (_input, signal) => new Promise((_resolve, reject) => { + 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[]) => { @@ -1316,7 +1329,7 @@ it('stops the merge check early enough for its processes to settle before the in let checkSignal: AbortSignal | undefined; const settle = CHECK_KILL_GRACE_MS + CHECK_PIPE_GRACE_MS; // Like the real runner, the check settles only after both grace periods once it is aborted. - const checks: AlreadyFixedGateway = { check: (_input, signal) => new Promise((_resolve, reject) => { + 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 }); }) }; @@ -1348,6 +1361,29 @@ it('gives the default merge check its own hardened runner, or the injected one', 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: rollup }); + // The rule read answers only once the check has started: run one after the other, this inspection would hang. + if (joined.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 }] } }]]); } + if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); + return JSON.stringify({ protected: false }); + }; + // 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('does not let an inspection started before merge repopulate the cache', async () => { let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); From 93e77b75f0e9f85666c3d24a772be3e13689245c Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:26:40 -0700 Subject: [PATCH 3/9] Close local review round 3 on F6 merge check: say why it blocks - Carry the check's matches (closes, PRs, short commit SHAs; at most five, never a commit message) or its unknown reason into the merge blocker. A long-lived PR whose base is more than 250 commits behind now says so. - Validate the merge repository with the shared REPOSITORY pattern. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- github/merge.ts | 28 ++++++++++++++++++----- runner/merge.ts | 8 +++++-- test/merge.test.ts | 33 +++++++++++++++++++++++++++- 4 files changed, 61 insertions(+), 10 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index aa5c0781..81c8707f 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. 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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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. An injected check must name the merge repository. 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 still waits for its processes, up to 1.5 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. +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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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. 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; 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 still waits for its processes, up to 1.5 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. 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. diff --git a/github/merge.ts b/github/merge.ts index cf0e2608..e9c9f4cf 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,6 +1,7 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; -import { CHECK_SETTLE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway } from './already-fixed.ts'; +import { CHECK_SETTLE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedMatch } from './already-fixed.ts'; +import { REPOSITORY } from './validate.ts'; const runFile = promisify(execFile); @@ -20,6 +21,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; } @@ -58,6 +61,17 @@ export interface GhMergeConfig { 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}` + : 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,7 +101,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { #inflight: { generation: number; promise: Promise } | null = null; /** `checks` defaults to the pre-PR check's GitHub adapter for the same repository, using `run` when one is given. */ constructor(config: GhMergeConfig, run?: RunGh, checks?: AlreadyFixedGateway) { - 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) + 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.'); @@ -117,16 +131,18 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { * 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 { + async #alreadyFixed(baseBranch: string, base: string, deadlineAt: number, signal?: AbortSignal): Promise> { const stop = new AbortController(); const timer = setTimeout(() => stop.abort(new Error('The already-fixed check did not finish in time.')), Math.max(0, deadlineAt - CHECK_SETTLE_MS - Date.now())); try { 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); - return result.outcome === 'clear' || result.outcome === 'found' ? result.outcome : 'unknown'; + if (result.outcome === 'clear') return { alreadyFixed: 'clear' }; + if (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); } } @@ -146,7 +162,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { return { base, head, pullRequestState: pr.state as RemoteMergeState['pullRequestState'], mergeable: pr.mergeable as RemoteMergeState['mergeable'], - ...rules, alreadyFixed: await alreadyFixed, + ...rules, ...await alreadyFixed, ...(typeof pr.url === 'string' && pr.url.length <= 2048 && /^https:\/\//.test(pr.url) ? { url: pr.url } : {}), }; } diff --git a/runner/merge.ts b/runner/merge.ts index 47324bef..8e55aff5 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -110,8 +110,12 @@ export class MergeCoordinator { 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}.` }); // 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: '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: 'The already-fixed check could not be completed.' }); + 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 9b8f1651..e90242b6 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -26,6 +26,15 @@ function alreadyFixedReads(args: readonly string[], fake: IssueFake = {}): strin 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[]): 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.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' } }], @@ -1348,7 +1357,7 @@ it('stops the merge check early enough for its processes to settle before the in expect(checkSignal?.aborted).toBe(true); await vi.advanceTimersByTimeAsync(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' } }); + expect(await Promise.race([settled, Promise.resolve('pending')])).toMatchObject({ state: { alreadyFixed: 'unknown', alreadyFixedDetail: 'The check did not finish in time.' } }); } finally { vi.useRealTimers(); } }); @@ -1384,6 +1393,28 @@ it('starts the merge check alongside the rule reads, and stops and awaits it whe 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/) }); + // 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 let an inspection started before merge repopulate the cache', async () => { let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); From d3913059b45e1f5e287460cddb3009d25987d931 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:27:41 -0700 Subject: [PATCH 4/9] Close local review round 4 on F6 merge check: share the merge-read fake in tests Co-Authored-By: Claude Opus 5.5 --- test/merge.test.ts | 46 ++++++++-------------------------------------- 1 file changed, 8 insertions(+), 38 deletions(-) diff --git a/test/merge.test.ts b/test/merge.test.ts index e90242b6..733852b0 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -27,9 +27,9 @@ function alreadyFixedReads(args: readonly string[], fake: IssueFake = {}): strin 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[]): string { +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: [] }); + 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 }); @@ -1220,16 +1220,7 @@ it('does not treat empty strict check policies as an atomic base guard', async ( function mergeRun(fake: IssueFake = {}, pull: Record = {}) { const calls: string[][] = []; - const run = async (args: readonly string[]) => { - calls.push([...args]); 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 }); - const fixed = alreadyFixedReads(args, fake); - if (fixed !== null) return fixed; - 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}`); - }; + 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 }; } @@ -1296,13 +1287,7 @@ it('gives an injected check the merge inputs and fails closed on an unknown outc 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[]) => { - const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'release/1', baseRefOid: sha('c'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); - if (joined.includes('/rules/branches/')) return JSON.stringify([[]]); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - return JSON.stringify({ protected: false }); - }; + 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() }); @@ -1319,13 +1304,7 @@ it('keeps the caller cancellation when the merge check is aborted', async () => 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[]) => { - 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.includes('/rules/branches/')) return JSON.stringify([[]]); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - return JSON.stringify({ protected: false }); - }; + 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')); @@ -1342,13 +1321,7 @@ it('stops the merge check early enough for its processes to settle before the in checkSignal = signal; signal?.addEventListener('abort', () => setTimeout(() => reject(new Error('aborted')), settle), { once: true }); }) }; - 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.includes('/rules/branches/')) return JSON.stringify([[]]); - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - return JSON.stringify({ protected: false }); - }; + 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 - CHECK_SETTLE_MS - 1); @@ -1379,12 +1352,9 @@ it('starts the merge check alongside the rule reads, and stops and awaits it whe }) }; 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: rollup }); // The rule read answers only once the check has started: run one after the other, this inspection would hang. - if (joined.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 }] } }]]); } - if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); - return JSON.stringify({ protected: false }); + 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 }); }; // A null rollup entry makes the required-check matching throw after the rule reads. rollup.push(null); From 164f690de0e8dd1df229324c81fe88cb8b920145 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:57:24 -0700 Subject: [PATCH 5/9] Close Copilot review round 6 on F6 merge check: shorten a closing commit's SHA A close by a commit reached the blocker as "commit <40 hex>", against the documented short-SHA detail. describeMatches now shortens a full SHA in the close description to 12 characters. Co-Authored-By: Claude Opus 5.5 --- github/merge.ts | 2 +- test/merge.test.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/github/merge.ts b/github/merge.ts index e9c9f4cf..bb44b24f 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -66,7 +66,7 @@ export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal } * 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}` + const named = matches.slice(0, 5).map(match => match.kind === 'closed' ? `the issue was closed by ${match.by.replace(/\b([a-f0-9]{12})[a-f0-9]{28}\b/g, '$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` : ''); diff --git a/test/merge.test.ts b/test/merge.test.ts index 733852b0..24431817 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -1371,6 +1371,10 @@ it('reports why the merge check matched or could not finish, without commit mess 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(); From 2eae9d249537c5dbd79371b6c2a4225465c720a1 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 09:18:24 -0700 Subject: [PATCH 6/9] Close Copilot review round 7 on F6 merge check: refuse answers after an abort Copilot (summary only) noted that a check answer arriving after the early stop was still used, and that rule reads answering after an abort still produced a state. Reproduced both with fake-timer tests. A stopped check is now unknown whatever it answers, and the inspection rejects with the abort's reason once its reads end. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- github/merge.ts | 8 +++++- test/merge.test.ts | 37 ++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 81c8707f..55a5b6f7 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. 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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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. 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; 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 still waits for its processes, up to 1.5 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. +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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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. 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; 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 still waits for its processes, up to 1.5 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. 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. diff --git a/github/merge.ts b/github/merge.ts index bb44b24f..61969e6d 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -137,6 +137,9 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { try { 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.' }; if (result.outcome === 'clear') return { alreadyFixed: 'clear' }; if (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.' }; @@ -159,10 +162,13 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { 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, ...await alreadyFixed, + ...rules, ...fixed, ...(typeof pr.url === 'string' && pr.url.length <= 2048 && /^https:\/\//.test(pr.url) ? { url: pr.url } : {}), }; } diff --git a/test/merge.test.ts b/test/merge.test.ts index 24431817..db9f3b21 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -1389,6 +1389,43 @@ it('names the already-fixed detail in the merge blocker', async () => { 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 - 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('does not let an inspection started before merge repopulate the cache', async () => { let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); From 8742c6d95614adee84f7724e443caff5b83d94f1 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 09:35:41 -0700 Subject: [PATCH 7/9] Close Copilot review round 8 on F6 merge check: no late start, exact SHA shortening Copilot (summary only) found two cases, both reproduced with tests: - A PR read that ended past the stop point still started the check, whose processes could then settle after the inspection deadline. With no settle time left, the check is now not started and is unknown. - The closer SHA shortening matched any 40-hex token, so a repository name of 40 hex characters was cut. Only the "commit " form is shortened. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- github/merge.ts | 7 +++++-- test/merge.test.ts | 26 ++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 55a5b6f7..95793f48 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. 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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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. 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; 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 still waits for its processes, up to 1.5 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. +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 1.75 seconds before the inspection's deadline (`CHECK_SETTLE_MS` in `github/already-fixed.ts`: both grace periods of its `gh` runner 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 still waits for its processes, up to 1.5 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. 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. diff --git a/github/merge.ts b/github/merge.ts index 61969e6d..84ddf981 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -66,7 +66,7 @@ export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal } * 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(/\b([a-f0-9]{12})[a-f0-9]{28}\b/g, '$1')}` + 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` : ''); @@ -132,8 +132,11 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { * 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 - 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.')), Math.max(0, deadlineAt - CHECK_SETTLE_MS - Date.now())); + const timer = setTimeout(() => stop.abort(new Error('The already-fixed check did not finish in time.')), runFor); try { 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); diff --git a/test/merge.test.ts b/test/merge.test.ts index db9f3b21..4ac1e32e 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -1426,6 +1426,32 @@ it('does not accept a check answer or rule reads that arrive after an abort', as } 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 - 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 - 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('does not let an inspection started before merge repopulate the cache', async () => { let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); From a3ac0b8a5bbd0da4c10d04e154c8ca002afbac57 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 09:58:22 -0700 Subject: [PATCH 8/9] Close Copilot review round 10 on F6 merge check: require the base head A clear or found answer without a valid baseHead was accepted, though clear removes the merge blocker. It now counts as unknown ("The check returned an invalid result."). Co-Authored-By: Claude Opus 5.5 --- github/merge.ts | 8 +++++--- test/merge.test.ts | 9 +++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/github/merge.ts b/github/merge.ts index 84ddf981..573e1204 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,7 +1,7 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { CHECK_SETTLE_MS, GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedMatch } from './already-fixed.ts'; -import { REPOSITORY } from './validate.ts'; +import { REPOSITORY, SHA } from './validate.ts'; const runFile = promisify(execFile); @@ -143,8 +143,10 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { // 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.' }; - if (result.outcome === 'clear') return { alreadyFixed: 'clear' }; - if (result.outcome === 'found' && Array.isArray(result.matches) && result.matches.length) return { alreadyFixed: 'found', alreadyFixedDetail: describeMatches(result.matches) }; + // `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; diff --git a/test/merge.test.ts b/test/merge.test.ts index 4ac1e32e..b47d0c39 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -1452,6 +1452,15 @@ it('does not start the merge check when no time is left for its processes to set } 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('does not let an inspection started before merge repopulate the cache', async () => { let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; }); From d7bc309e2adf4620648ae5bb51834337d871ced6 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 10:35:17 -0700 Subject: [PATCH 9/9] Close Copilot review round 12 on F6 merge check: keep the first abort reason When the caller aborted and the inspection deadline also passed before a stopped gh call settled, inspect() reported the timeout instead of the caller's reason. It now rethrows the combined signal's reason, which is whichever abort came first. Co-Authored-By: Claude Opus 5.5 --- github/merge.ts | 5 +++-- test/merge.test.ts | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/github/merge.ts b/github/merge.ts index c1125676..7b19d8cc 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -293,8 +293,9 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { 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(deadlineAt, signal).catch(error => { - if (timeout.signal.aborted) throw timeout.signal.reason; - if (options.signal?.aborted) throw options.signal.reason; + // 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/test/merge.test.ts b/test/merge.test.ts index be4fd89b..1ede078b 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -1462,6 +1462,23 @@ it('fails the merge check closed on a clear or found answer without a valid base 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 () => { let pullReads = 0, releaseTimeline!: (value: string) => void, markTimelineStarted!: () => void; const timelineStarted = new Promise(resolve => { markTimelineStarted = resolve; });