diff --git a/README.md b/README.md index 41d32414..0703a1f3 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ An existing-store configuration may add a trusted GitHub binding: } ``` -The issue must match the stored plan. The authenticated `gh` account must be able to read the pull request, issue timeline, applicable rulesets, and classic branch protection, and to merge the PR. Codeboost unions required checks from both rule sources, requires strict server-enforced current-base checks, rechecks the base and head immediately before merging, and passes the reviewed head to `gh pr merge --match-head-commit`. Missing permissions and ambiguous rule responses block the merge. When the branch uses a merge queue, codeboost adds the exact reviewed head to the queue and treats the merge as done only when GitHub confirms it merged; a queued pull request is not merged. If GitHub removes it from the queue while the plan, snapshot, review and head are unchanged, Merge offers a retry after the status refresh; if the head changed, it goes back to review first. A moved base and any unexecuted `cmd:` acceptance check remain blocked until [#22](https://github.com/codeabovelab/codeboost/issues/22) adds the runner path. +The issue must match the stored plan. `pullRequest` is required unless the configuration has a `runner` block (see below). The authenticated `gh` account must be able to read the pull request, issue timeline, applicable rulesets, and classic branch protection, and to merge the PR. Codeboost unions required checks from both rule sources, requires strict server-enforced current-base checks, rechecks the base and head immediately before merging, and passes the reviewed head to `gh pr merge --match-head-commit`. Missing permissions and ambiguous rule responses block the merge. When the branch uses a merge queue, codeboost adds the exact reviewed head to the queue and treats the merge as done only when GitHub confirms it merged; a queued pull request is not merged. If GitHub removes it from the queue while the plan, snapshot, review and head are unchanged, Merge offers a retry after the status refresh; if the head changed, it goes back to review first. A moved base and any unexecuted `cmd:` acceptance check remain blocked until [#22](https://github.com/codeabovelab/codeboost/issues/22) adds the runner path. ## Runner (opt-in) @@ -51,7 +51,7 @@ A configuration with a `github` block may also add a `runner` block. The runner - `diagnosticsCapBytes`: the size retention trims that directory back to. It is a target, not a hard limit: the file just saved is always kept, even when it alone passes the cap. The default is 256 MiB. - `limits`: task storage limits (`workBytes`, `workInodes`, `metadataBytes`, `metadataInodes`). -The runner needs Docker and `CLAUDE_CODE_OAUTH_TOKEN`. At start, before the server opens, codeboost recovers what an earlier run left and builds the agent image; a recovery it cannot finish safely stops startup with what to do. `POST /api/runner` with `start` or `resume` runs the plan; the review screen has no buttons for them yet. When a run has completed every item, codeboost pushes the task head to a `codeboost/…` branch with your `gh` credentials and opens a ready pull request into `baseBranch`; a task that needs a person gets a draft pull request with its problems. A publish that was refused (for example, the branch holds a commit codeboost did not make) is retried with the `publish` action, and `GET /api/runner` shows the last outcome under `publish`. Demos never publish. +The runner needs Docker and `CLAUDE_CODE_OAUTH_TOKEN`. At start, before the server opens, codeboost recovers what an earlier run left and builds the agent image; a recovery it cannot finish safely stops startup with what to do. `POST /api/runner` with `start` or `resume` runs the plan; the review screen has no buttons for them yet. When a run has completed every item, codeboost pushes the task head to a `codeboost/…` branch with your `gh` credentials and opens a ready pull request into `baseBranch`; a task that needs a person gets a draft pull request with its problems. A publish that was refused (for example, the branch holds a commit codeboost did not make) is retried with the `publish` action, and `GET /api/runner` shows the last outcome under `publish`. Merge PR then merges that pull request, so `github.pullRequest` can be left out; if it is set, it must name the task's pull request, or merging is blocked. Demos never publish. ## Library diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 109ac7d3..c512769e 100644 --- a/docs/implementation/guarded-merge.md +++ b/docs/implementation/guarded-merge.md @@ -4,11 +4,30 @@ The review screen can now turn a fully approved snapshot into a GitHub merge wit Derived planted-review configurations clear the source GitHub binding because their repository and review identity point at a disposable clone. +## Which pull request (#121) + +| Configuration | The merge targets | `github.pullRequest` | +|---|---|---| +| No runner block | `github.pullRequest` | Required. The server refuses to start without it. | +| A runner block | The task's published PR (see "When the server publishes" in `pull-request-opening.md`) | Optional. If it is set and names another PR, merging is blocked until it is removed. | + +With a runner block, the task's PR is the `opened` record whose opening began last among the task's records in the configured repository with base `github.baseBranch`. (An older opening adopted later is not chosen; if its PR is open, it is a second open PR from the branch, which blocks merging.) Merging is blocked (`pull-request`) when: + +- the task has no such record; +- an opening of the task's PRs is still in flight, or an update of one is (`refresh`); +- GitHub shows the PR from another branch than the record's, from a fork, or into another base; +- the PR's description does not start with that record's marker; +- the PR is open, but another PR is open from the same branch, or GitHub's list of open PRs from the branch does not show it yet. + +For this the inspection also reads the PR's description and fork flag, and lists the open PRs from the task branch (one page of up to 100; a full page fails closed). The list runs alongside the rule reads. A draft PR blocks merging (`draft`) in both configurations, because GitHub does not merge a draft. + +The target is resolved again for each validation pass. Both passes, and the third pass in queue mode, must resolve the same PR from the same opening. Admission then re-reads the record in the same transaction that saves the attempt: it must still be the opened record whose opening began last in its base, with the same number, and nothing may be opening or updating. The attempt records the PR it merges. A status read, reconciliation or queue poll for an attempt that is submitting, queued or merged reads that PR, never the one the gate would choose now. An attempt saved before #121 has no PR: it merged `github.pullRequest`, so that PR is read. With a runner block and no `github.pullRequest`, such an attempt stays blocked until a person sets `github.pullRequest` to the PR it merged. + ## Gate 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 0.65 seconds before the inspection's deadline (`MERGE_CHECK_SETTLE_MS` in `github/merge.ts`: both grace periods of the merge's `gh` runner, which the check also uses, plus a margin), so its processes settle before the inspection ends; a check stopped that way is unknown. If the rule reads fail, the check is stopped and awaited before the inspection rejects. When the PR read ends too late to leave that much time, the check is not started and is unknown. A check answer that arrives after the check was stopped is unknown, and an inspection whose caller aborted or whose deadline passed rejects even when its reads answer late. An injected check must name the merge repository. The blocker names what the check found (at most five closes, PRs and commits, by repository, number, state and short SHA, with a closing commit's SHA shortened too; never a commit message) or why it could not finish. Once this PR has merged, the check counts it as the fix, so the coordinator shows only the `pr-state` blocker, not an already-fixed one. Malformed pagination, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. A caller's own abort during the already-fixed check waits for its processes like any other `gh` call of the gateway, up to 0.4 seconds. An unreadable or incomplete rule source, a pending/missing/failing check, an already-fixed match or unknown result, a conflict, or a closed PR blocks merging. +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, and every other PR the task records in this repository (#121), as the task's own PRs, matched by repository and number: each is excluded while open and counts once merged, as in the pre-PR check. Without a runner block only this PR is passed. The commit scan starts at the PR's base commit on its base branch. Merging requires that commit to equal the reviewed snapshot's base, which is the base the pre-PR check scanned from. The merge passes no own commits, because the task's commits reach the base branch only by this merge. Any result the check cannot complete is unknown and blocks merging, including a base branch name outside the pattern the check accepts. The check starts as soon as the PR is read, alongside the rule reads, and stops 0.65 seconds before the inspection's deadline (`MERGE_CHECK_SETTLE_MS` in `github/merge.ts`: both grace periods of the merge's `gh` runner, which the check also uses, plus a margin), so its processes settle before the inspection ends; a check stopped that way is unknown. If the rule reads fail, the check is stopped and awaited before the inspection rejects. When the PR read ends too late to leave that much time, the check is not started and is unknown. A check answer that arrives after the check was stopped is unknown, and an inspection whose caller aborted or whose deadline passed rejects even when its reads answer late. An injected check must name the merge repository. The blocker names what the check found (at most five closes, PRs and commits, by repository, number, state and short SHA, with a closing commit's SHA shortened too; never a commit message) or why it could not finish. Once this PR has merged, the check counts it as the fix, so the coordinator shows only the `pr-state` blocker, not an already-fixed one. Malformed pagination, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. A caller's own abort during the already-fixed check waits for its processes like any other `gh` call of the gateway, up to 0.4 seconds. An unreadable or incomplete rule source, a pending/missing/failing check, an already-fixed match or unknown result, a conflict, or a closed PR blocks merging. Automatic merging also requires strict required status checks as a server-enforced current-base policy. A client-side fetch cannot supply that guarantee. Merge-queue branches were blocked in this increment because `gh pr merge` can report a successful enqueue before the pull request is merged. Queue support has since landed (#24; see `merge-queue.md`): a queue-capable adapter tracks each attempt until GitHub confirms the merge, removal or failure, and queue rules count as the server-side current-base guard. An adapter without queue inspection still fails closed with a `merge-queue` blocker. The coordinator performs the complete gate twice without using the display cache, verifies the same base/head pair, then re-reads the local review generation immediately before invoking `gh pr merge --match-head-commit` with literal argv. Each `gh` process gets only the allowlisted variables in `github/gh-env.ts`, plus fixed settings that turn off prompts, the pager, colour and update checks. No other server variables are passed on. A timeout or abort sends the `gh` process SIGTERM, then SIGKILL after a quarter of a second, and the call returns only after that process has exited (or 0.15 s after it exits, if a process it started keeps its output open). So a merge click aborted at its 14-second deadline settles by 14.4 s, inside the 14.5-second shutdown drain and below the 15-second request budget. Stopping `gh` cannot recall a merge request it had already sent: GitHub can still apply it, so a cancelled merge is reported as unknown and its attempt stays in flight, with the action disabled. A direct attempt is settled only when GitHub shows the pull request merged; a queued attempt also settles when GitHub reports it removed or failed (`merge-queue.md`). The adapter invalidates its pre-action cache before the command and after either success or refusal. A generation guard prevents inspections started before or during the command from restoring stale cache entries afterward. After command success, the server returns “merge submitted” even if its best-effort status reload fails, and the browser keeps the action disabled until Refresh confirms GitHub state. GitHub's command refusal text is returned to the local UI, and a failed attempt also remains disabled until Refresh loads fresh state. There is no “merge anyway” path around missing atomic protection. Demo mode never constructs the coordinator, including when a gateway is injected. diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index d7c83ef5..5990c90d 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -4,7 +4,7 @@ Queue-backed merging is enabled only when the configured GitHub adapter implemen ## Durable lifecycle -SQLite owns the merge-attempt record and retains its plan revision, snapshot, review version, and exact reviewed head. Legal transitions are: +SQLite owns the merge-attempt record and retains its plan revision, snapshot, review version, exact reviewed head, and the pull request it merges (#121). The queue watermark, enqueue and every later queue inspection use that pull request, never the one the gate would choose now (see "Which pull request" in `guarded-merge.md`). An attempt saved before #121 names none; it queued `github.pullRequest`, which is read instead, and without that setting a poll reports an observation error and leaves the attempt unchanged. Legal transitions are: ```text submitting -> queued -> merged diff --git a/docs/implementation/pull-request-opening.md b/docs/implementation/pull-request-opening.md index 12c30ccd..edf1599d 100644 --- a/docs/implementation/pull-request-opening.md +++ b/docs/implementation/pull-request-opening.md @@ -33,7 +33,7 @@ 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. +The pre-merge check (`GhMergeGateway`, see `guarded-merge.md`) runs this same check, with the PR being merged and the task's other recorded PRs as its own PRs, 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. @@ -134,6 +134,7 @@ Some repositories do not support draft PRs (for example private repositories on - **When.** Once a run started by `start` or `resume` has ended, with no attempt active (`assertPublishable` refuses one). On the `publish` runner action (`expectedStateVersion` and an action ID, like the other runner actions), which retries a refused or failed publish. When a refused `start` or `resume` moves the task to needs human (an expired budget is committed with the refusal); a refused action that leaves the status as it was publishes nothing, so pressing resume on a task already in needs human never pushes. Once at startup, if a publish is owed: the CLI starts it (`publishOwed`) after it has verified that the runner lock still names the database, because a publish pushes. A publish is owed when the task can be published and no publish has settled (`opened`, `possibly already fixed`, `draft skipped`, `draft unsupported`, `no changes`) at the task's current state version. So a refusal, a failure or a stop leaves it owed, and the next start recovers a lost opening by its marker. - **Outcome.** Each publish runs in the background, one per task, and records its outcome in `publish_outcomes` (schema v10): the outcome kind, or `refused` (a guard, `BranchPushRefused`, `OpeningUnsettled`, `PullRequestMisplaced`, `PullRequestRefused`), `failed` or `stopped` (shutdown), with its message (any token value or token shape removed, as for the push) and the task's state version. The record does not move the task's state version. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. The v10 upgrade records every task already `running` or in `needs human` as `not published`, a settled outcome: the first start after the upgrade opens no PR a person did not ask for, and the `publish` action publishes it. `GET /api/runner` shows it as `publish.last`, with `publish.active` and `publish.publishable`. - **Start and resume** refuse while a publish of the task is in progress, so a new item cannot move the head being pushed. A second `publish` is refused too, and the refusal is saved under its action ID. Cancel task is not refused: a draft PR that GitHub opens after a cancel is recorded, and closing it is the later "close the draft on cancel" slice. +- **Merge (#121).** With a runner block, Merge PR merges the task's published PR, not `github.pullRequest`, which becomes optional; if it is set, it must name the task's PR. See "Which pull request" in `guarded-merge.md`. - **Demo.** A demo never publishes, even with a `github` block or an injected publisher. ## Tests diff --git a/github/merge.ts b/github/merge.ts index 47b5c92e..95893a34 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,7 +1,8 @@ import { GhAlreadyFixedGateway, type AlreadyFixedGateway, type AlreadyFixedMatch } from './already-fixed.ts'; import { ghEnvironment } from './gh-env.ts'; +import { markerOf } from './pull-requests.ts'; import { runWithInput } from './run-with-input.ts'; -import { REPOSITORY, SHA } from './validate.ts'; +import { BRANCH, REPOSITORY, SHA } from './validate.ts'; /** * How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after @@ -38,6 +39,27 @@ export interface RemoteMergeState { alreadyFixedDetail?: string; /** The pull request's web URL, when GitHub reports a valid one. Display only; never used to decide readiness. */ url?: string; + /** The pull request this state describes, as GitHub reports it. */ + pullRequest: number; + /** GitHub does not merge a draft. */ + draft: boolean; + /** + * Read only for a published target (#121): where GitHub shows the PR, its description's first line, whether it comes + * from a fork, and every open PR from its head branch. The coordinator decides from these whether it is the task's PR. + */ + published?: PublishedPullRequestState; +} +export interface PublishedPullRequestState { + headBranch: string; baseBranch: string; crossRepository: boolean; marker: string; + branchOpen: Array<{ number: number; base: string }>; +} +/** The pull request one inspection acts on. Without one, the gateway uses its configured `pullRequest`. */ +export interface MergeTarget { + pullRequest: number; + /** The task's own PRs in this repository; the already-fixed check excludes them while open. `pullRequest` is always one. */ + ownPullRequests?: readonly number[]; + /** The task branch of a published target (#121). The inspection then also reads `RemoteMergeState.published`. */ + headBranch?: string; } export interface MergeResult { url: string; } @@ -56,23 +78,29 @@ export type MergeQueueObservation = | { state: 'failed'; reviewedHead: string; entryId: string; reason: string } | { state: 'merged'; reviewedHead: string; mergedAt: string }; export interface MergeQueueGateway { - queueWatermark(expectedHead: string, options?: { signal?: AbortSignal; timeoutMs?: number }): Promise; - inspectQueue(expectedHead: string, options?: { signal?: AbortSignal; timeoutMs?: number; afterCursor?: string | null }): Promise; + queueWatermark(expectedHead: string, options?: { signal?: AbortSignal; timeoutMs?: number; pullRequest?: number }): Promise; + inspectQueue(expectedHead: string, options?: { signal?: AbortSignal; timeoutMs?: number; afterCursor?: string | null; pullRequest?: number }): Promise; } export interface MergeGateway { - inspect(options?: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal }): Promise; - merge(expectedHead: string, options?: { signal?: AbortSignal }): Promise; + inspect(options?: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal; target?: MergeTarget }): Promise; + /** `pullRequest`: the PR to merge; without it, the configured one. */ + merge(expectedHead: string, options?: { signal?: AbortSignal; pullRequest?: number }): Promise; } export interface GhMergeConfig { repository: string; - pullRequest: number; + /** + * The PR to merge. Required without a runner block. With one, the merge targets the task's published PR, and this, if + * set, must name that PR (#121). + */ + pullRequest?: number; issue: number; method?: 'merge' | 'squash' | 'rebase'; /** The branch a task's pull request targets (#103). Required when the review has a runner block. */ baseBranch?: string; } +type ResolvedTarget = { pullRequest: number; ownPullRequests: readonly number[]; headBranch?: string }; type BranchRules = Pick; export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal }) => Promise; @@ -91,6 +119,10 @@ 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); } +function validNumber(value: unknown): value is number { + return Number.isSafeInteger(value) && (value as number) >= 1; +} + function fullSha(value: unknown, label: string): string { if (typeof value !== 'string' || !/^[a-f0-9]{40}$/.test(value)) throw new Error(`GitHub returned an invalid ${label}.`); return value; @@ -111,13 +143,14 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { readonly config: GhMergeConfig; readonly run: RunGh; readonly checks: AlreadyFixedGateway; - #cache: { expiresAt: number; state: RemoteMergeState } | null = null; + /** Keyed by the inspected target, so a state read for one PR is never served for another. */ + #cache: { key: string; expiresAt: number; state: RemoteMergeState } | null = null; #generation = 0; - #inflight: { generation: number; promise: Promise } | null = null; + #inflight: { generation: number; key: string; promise: Promise } | null = null; /** `checks` defaults to the pre-PR check's GitHub adapter for the same repository, using this gateway's runner. */ constructor(config: GhMergeConfig, run?: RunGh, checks?: AlreadyFixedGateway) { - if (!REPOSITORY.test(config.repository) || !Number.isSafeInteger(config.pullRequest) || config.pullRequest < 1 || !Number.isSafeInteger(config.issue) || config.issue < 1) - throw new Error('A GitHub repository, pull request, and issue are required for merging.'); + if (!REPOSITORY.test(config.repository) || (config.pullRequest !== undefined && !validNumber(config.pullRequest)) || !Number.isSafeInteger(config.issue) || config.issue < 1) + throw new Error('A GitHub repository and issue, and a valid pull request number if one is given, are required for merging.'); if (config.method !== undefined && !['merge','squash','rebase'].includes(config.method)) throw new Error('GitHub merge method must be merge, squash, or rebase.'); if (checks && checks.repository?.toLowerCase() !== config.repository.toLowerCase()) throw new Error('The already-fixed check must name and read the merge repository.'); this.config = config; @@ -126,6 +159,22 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { this.checks = checks ?? new GhAlreadyFixedGateway({ repository: config.repository }, this.run); } + /** The PR a call acts on: the one given, else the configured one. */ + #number(pullRequest?: number): number { + const number = pullRequest ?? this.config.pullRequest; + if (number === undefined) throw new Error('No pull request to merge: add github.pullRequest, or publish the task\'s pull request first.'); + if (!validNumber(number)) throw new Error('Invalid pull request number.'); + return number; + } + + #target(target?: MergeTarget): ResolvedTarget { + const pullRequest = this.#number(target?.pullRequest); + const own = target?.ownPullRequests ?? []; + if (own.some(number => !validNumber(number))) throw new Error('Invalid pull request number.'); + if (target?.headBranch !== undefined && (typeof target.headBranch !== 'string' || !BRANCH.test(target.headBranch))) throw new Error('Invalid pull request branch.'); + return { pullRequest, ownPullRequests: [...new Set([pullRequest, ...own])], ...(target?.headBranch !== undefined ? { headBranch: target.headBranch } : {}) }; + } + async #json(args: readonly string[], signal?: AbortSignal): Promise { const output = await this.run(args, { signal }); try { return JSON.parse(output); } @@ -141,20 +190,21 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } /** - * 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 pre-merge "already fixed" check runs the pre-PR check (`github/already-fixed.ts`) with the same rules. This PR and + * the task's other recorded PRs are its own: 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> { + async #alreadyFixed(ownPullRequests: readonly number[], baseBranch: string, base: string, deadlineAt: number, signal?: AbortSignal): Promise> { // A check started with no settle time left could leave processes running past the inspection's deadline. const runFor = deadlineAt - MERGE_CHECK_SETTLE_MS - Date.now(); if (runFor <= 0) return { alreadyFixed: 'unknown', alreadyFixedDetail: 'No time was left to run the check.' }; const stop = new AbortController(); const timer = setTimeout(() => stop.abort(new Error('The already-fixed check did not finish in time.')), runFor); try { - const result = await this.checks.check({ issue: this.config.issue, taskBase: base, baseBranch, ownPullRequests: [this.config.pullRequest], ownCommits: new Set() }, + const result = await this.checks.check({ issue: this.config.issue, taskBase: base, baseBranch, ownPullRequests: [...ownPullRequests], 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.) @@ -170,30 +220,60 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } finally { clearTimeout(timer); } } - 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)) + async #inspectNow(target: ResolvedTarget, deadlineAt: number, signal?: AbortSignal): Promise { + const published = target.headBranch !== undefined; + const fields = 'number,isDraft,baseRefName,baseRefOid,headRefName,headRefOid,state,mergeable,statusCheckRollup,url' + (published ? ',body,isCrossRepository' : ''); + const pr = await this.#json(['pr','view',String(target.pullRequest),'--repo',this.config.repository,'--json',fields], 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) + || typeof pr.isDraft !== 'boolean' || (published && (typeof pr.body !== 'string' || typeof pr.isCrossRepository !== 'boolean'))) throw new Error('GitHub returned an incomplete pull request state.'); + if (pr.number !== target.pullRequest) throw new Error('GitHub returned a different pull request.'); const base = fullSha(pr.baseRefOid, 'base SHA'), head = fullSha(pr.headRefOid, 'head SHA'); - // The already-fixed check needs only the PR's base, so it runs alongside the rule reads. If those fail, the check is - // stopped; it is awaited on every path, so no `gh` process it started outlives the inspection. - const failed = new AbortController(); - const alreadyFixed = this.#alreadyFixed(pr.baseRefName, base, deadlineAt, signal ? AbortSignal.any([signal, failed.signal]) : failed.signal); - alreadyFixed.catch(() => {}); - let rules: BranchRules; - try { rules = await this.#rules(pr.baseRefName, pr.statusCheckRollup as Array>, signal); } - catch (error) { failed.abort(error); await alreadyFixed.catch(() => {}); throw error; } - const fixed = await alreadyFixed; + // The already-fixed check needs only the PR's base, so it runs alongside the rule reads (and, for a published target, + // the branch's PR list). If either read fails, the others are stopped. All three are awaited on every path, so no `gh` + // process they started outlives the inspection. + const failed = new AbortController(), reads = signal ? AbortSignal.any([signal, failed.signal]) : failed.signal; + const stopOthers = (error: unknown) => { failed.abort(error); throw error; }; + const alreadyFixed = this.#alreadyFixed(target.ownPullRequests, pr.baseRefName, base, deadlineAt, reads); + const [ruleRead, listRead] = await Promise.allSettled([ + this.#rules(pr.baseRefName, pr.statusCheckRollup as Array>, reads).catch(stopOthers), + published ? this.#branchOpen(target.headBranch!, reads).catch(stopOthers) : undefined, + ]); + const fixed = await alreadyFixed.catch(error => error instanceof Error ? error : new Error(String(error))); + for (const read of [ruleRead, listRead]) if (read.status === 'rejected') throw read.reason; + if (fixed instanceof Error) throw fixed; + const rules = (ruleRead as PromiseFulfilledResult).value; + const branchOpen = (listRead as PromiseFulfilledResult).value; // 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, + base, head, pullRequest: target.pullRequest, draft: pr.isDraft, pullRequestState: pr.state as RemoteMergeState['pullRequestState'], mergeable: pr.mergeable as RemoteMergeState['mergeable'], ...rules, ...fixed, ...(typeof pr.url === 'string' && pr.url.length <= 2048 && /^https:\/\//.test(pr.url) ? { url: pr.url } : {}), + ...(published ? { published: { headBranch: pr.headRefName, baseBranch: pr.baseRefName, crossRepository: pr.isCrossRepository as boolean, + marker: markerOf(pr.body as string), branchOpen: branchOpen! } } : {}), }; } + /** + * Every open PR from `headBranch` in this repository, with its base. One page is read, as in the publisher's lookup: a + * full page could hide another PR from the branch, so it fails closed, and so does any malformed entry. + */ + async #branchOpen(headBranch: string, signal?: AbortSignal): Promise { + const owner = this.config.repository.split('/')[0]!; + const query = new URLSearchParams({ state: 'open', head: `${owner}:${headBranch}`, per_page: '100' }); + const response = await this.#json(['api','-H','Accept: application/vnd.github+json',`repos/${this.config.repository}/pulls?${query}`], signal); + if (!Array.isArray(response)) throw new Error('GitHub returned an invalid pull request list.'); + if (response.length >= 100) throw new Error(`GitHub lists 100 or more open pull requests from ${headBranch}; codeboost cannot read them all.`); + return response.map(entry => { + const pr = entry as { number?: unknown; head?: { ref?: unknown }; base?: { ref?: unknown } } | null; + if (!pr || typeof pr !== 'object' || !validNumber(pr.number) || pr.head?.ref !== headBranch || typeof pr.base?.ref !== 'string') + throw new Error('GitHub returned an invalid pull request list.'); + return { number: pr.number, base: pr.base.ref }; + }); + } + /** 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); @@ -285,30 +365,32 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { return { rulesKnown, atomicBaseGuard, mergeQueue, requiredChecks }; } - async inspect(options: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal } = {}): Promise { + async inspect(options: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal; target?: MergeTarget } = {}): Promise { const timeoutMs = options.timeoutMs ?? MERGE_INSPECTION_TIMEOUT_MS; if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub inspection timeout.'); - if (!options.fresh && this.#cache && this.#cache.expiresAt > Date.now()) return this.#cache.state; + const target = this.#target(options.target), key = JSON.stringify(target); + if (!options.fresh && this.#cache?.key === key && this.#cache.expiresAt > Date.now()) return this.#cache.state; const generation = this.#generation; - if (!options.fresh && this.#inflight?.generation === generation) return this.#inflight.promise; + if (!options.fresh && this.#inflight?.generation === generation && this.#inflight.key === key) return this.#inflight.promise; 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(deadlineAt, signal).catch(error => { + const attempt = this.#inspectNow(target, deadlineAt, signal).catch(error => { // The combined signal keeps the reason of whichever abort came first, the caller's or the deadline's; a stopped gh // call can settle after both have fired. if (signal.aborted) throw signal.reason; throw error; }).then(state => { - if (this.#generation === generation) this.#cache = { expiresAt: Date.now() + 5_000, state }; + if (this.#generation === generation) this.#cache = { key, expiresAt: Date.now() + 5_000, state }; return state; }).finally(() => { clearTimeout(timer); if (this.#inflight?.promise === attempt) this.#inflight = null; }); - if (!options.fresh) this.#inflight = { generation, promise: attempt }; + if (!options.fresh) this.#inflight = { generation, key, promise: attempt }; return attempt; } - async queueWatermark(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number } = {}): Promise { + async queueWatermark(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number; pullRequest?: number } = {}): Promise { fullSha(expectedHead, 'expected head SHA'); + const number = this.#number(options.pullRequest); const timeoutMs = options.timeoutMs ?? 6_000; if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub queue watermark timeout.'); const [owner, name] = this.config.repository.split('/') as [string, string]; @@ -318,14 +400,14 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { const signal = options.signal ? AbortSignal.any([options.signal, timeout.signal]) : timeout.signal; try { if (options.signal?.aborted) throw options.signal.reason; - const response = await this.#json(['api','graphql','-f',`query=${query}`,'-f',`owner=${owner}`,'-f',`name=${name}`,'-F',`number=${this.config.pullRequest}`], signal) as { + const response = await this.#json(['api','graphql','-f',`query=${query}`,'-f',`owner=${owner}`,'-f',`name=${name}`,'-F',`number=${number}`], signal) as { data?: { repository?: { pullRequest?: { number?: unknown; headRefOid?: unknown; timelineItems?: { edges?: unknown } } | null } | null }; errors?: unknown; }; if (signal.aborted) throw signal.reason; if (Object.hasOwn(response, 'errors') && (!Array.isArray(response.errors) || response.errors.length > 0)) throw new Error('GitHub returned merge-queue watermark data with errors.'); const pull = response.data?.repository?.pullRequest; - if (!pull || pull.number !== this.config.pullRequest || fullSha(pull.headRefOid, 'queue watermark head SHA') !== expectedHead || !Array.isArray(pull.timelineItems?.edges)) + if (!pull || pull.number !== number || fullSha(pull.headRefOid, 'queue watermark head SHA') !== expectedHead || !Array.isArray(pull.timelineItems?.edges)) throw new Error('GitHub returned an incomplete merge-queue watermark.'); const edges = pull.timelineItems.edges; if (edges.length > 1) throw new Error('GitHub returned an invalid merge-queue watermark.'); @@ -343,8 +425,9 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } finally { clearTimeout(timer); } } - async inspectQueue(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number; afterCursor?: string | null } = {}): Promise { + async inspectQueue(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number; afterCursor?: string | null; pullRequest?: number } = {}): Promise { fullSha(expectedHead, 'expected head SHA'); + const number = this.#number(options.pullRequest); const timeoutMs = options.timeoutMs ?? MERGE_INSPECTION_TIMEOUT_MS; if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub queue inspection timeout.'); const correlated = Object.hasOwn(options, 'afterCursor'); @@ -360,13 +443,13 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { let cursor = options.afterCursor ?? null, pull: Record | null = null, stable = '', page = 0; const events: Array<{ id: string; type: unknown; createdAt: string; reason: string | undefined; beforeHead: string | undefined }> = []; while (page++ < 10) { - const args = ['api','graphql','-f',`query=${query}`,'-f',`owner=${owner}`,'-f',`name=${name}`,'-F',`number=${this.config.pullRequest}`]; + const args = ['api','graphql','-f',`query=${query}`,'-f',`owner=${owner}`,'-f',`name=${name}`,'-F',`number=${number}`]; if (cursor !== null) args.push('-f', `after=${cursor}`); const response = await this.#json(args, signal) as { data?: { repository?: { pullRequest?: Record | null } | null }; errors?: unknown }; if (signal.aborted) throw signal.reason; if (Object.hasOwn(response, 'errors') && (!Array.isArray(response.errors) || response.errors.length > 0)) throw new Error('GitHub returned merge-queue data with errors.'); const nextPull = response.data?.repository?.pullRequest; - if (!nextPull || nextPull.number !== this.config.pullRequest) throw new Error('GitHub returned an incomplete merge-queue pull request.'); + if (!nextPull || nextPull.number !== number) throw new Error('GitHub returned an incomplete merge-queue pull request.'); const current = JSON.stringify({ number: nextPull.number, headRefOid: nextPull.headRefOid, state: nextPull.state, mergedAt: nextPull.mergedAt, mergeQueueEntry: nextPull.mergeQueueEntry }); if (stable && current !== stable) throw new Error('GitHub merge-queue state changed during paginated inspection.'); stable = current; pull = nextPull; @@ -392,7 +475,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { if (page === 10 || typeof value.pageInfo.endCursor !== 'string' || !value.pageInfo.endCursor) throw new Error('GitHub merge-queue history exceeded the inspection limit.'); cursor = value.pageInfo.endCursor; } - if (!pull || pull.number !== this.config.pullRequest) throw new Error('GitHub returned an incomplete merge-queue pull request.'); + if (!pull || pull.number !== number) throw new Error('GitHub returned an incomplete merge-queue pull request.'); const reviewedHead = fullSha(pull.headRefOid, 'queue pull request head SHA'); if (reviewedHead !== expectedHead) throw new Error('The pull request head changed after review.'); if (!['OPEN','CLOSED','MERGED'].includes(String(pull.state))) throw new Error('GitHub returned an invalid queue pull request state.'); @@ -419,7 +502,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { if (correlated && attemptEvents.length !== 1) throw new Error('GitHub returned an ambiguous active event sequence for the current enqueue attempt.'); const queueHead = fullSha(value.headCommit?.oid, 'merge-queue head SHA'); const entryHead = fullSha(value.pullRequest?.headRefOid, 'merge-queue entry pull request head SHA'); - if (value.pullRequest?.number !== this.config.pullRequest || queueHead !== expectedHead || entryHead !== expectedHead) throw new Error('The merge-queue entry does not match the reviewed pull request head.'); + if (value.pullRequest?.number !== number || queueHead !== expectedHead || entryHead !== expectedHead) throw new Error('The merge-queue entry does not match the reviewed pull request head.'); if (value.state === 'UNMERGEABLE') return { state: 'failed', reviewedHead, entryId: value.id, reason: 'GitHub reported the merge queue entry as unmergeable.' }; return { state: 'queued', reviewedHead, entryId: value.id, phase: value.state as MergeQueueEntryPhase, @@ -439,15 +522,16 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } finally { clearTimeout(timer); } } - async merge(expectedHead: string, options: { signal?: AbortSignal } = {}): Promise { + async merge(expectedHead: string, options: { signal?: AbortSignal; pullRequest?: number } = {}): Promise { fullSha(expectedHead, 'expected head SHA'); + const number = this.#number(options.pullRequest); const flag = this.config.method === 'squash' ? '--squash' : this.config.method === 'rebase' ? '--rebase' : '--merge'; this.#generation++; this.#cache = null; try { if (options.signal?.aborted) throw options.signal.reason; - await this.run(['pr','merge',String(this.config.pullRequest),'--repo',this.config.repository,flag,'--match-head-commit',expectedHead], { signal: options.signal }); - return { url: `https://github.com/${this.config.repository}/pull/${this.config.pullRequest}` }; + await this.run(['pr','merge',String(number),'--repo',this.config.repository,flag,'--match-head-commit',expectedHead], { signal: options.signal }); + return { url: `https://github.com/${this.config.repository}/pull/${number}` }; } catch (error) { if (error instanceof MergeSubmissionError) throw error; const message = error instanceof Error ? error.message : 'GitHub merge submission failed with an unknown outcome.'; diff --git a/github/pull-requests.ts b/github/pull-requests.ts index 20a38ec0..7077950b 100644 --- a/github/pull-requests.ts +++ b/github/pull-requests.ts @@ -92,6 +92,8 @@ async function draftCall(call: () => Promise): Promise { * agent-controlled, so a marker-shaped string there never identifies a PR. */ export const markerOf = (body: string): string => body.split('\n', 1)[0]!.trim(); +/** The marker of one opening: the first line of the description of the PR it opens. */ +export const openingMarker = (openingId: string): string => ``; /** * One deadline for a whole open, lookup, refresh or draft change, whatever the caller's signal. Stopping `gh` at the * deadline can take up to 6 seconds more (SIGTERM grace and pipe drain), so an operation settles within 66 seconds. diff --git a/github/validate.ts b/github/validate.ts index c3095020..db6ae4c8 100644 --- a/github/validate.ts +++ b/github/validate.ts @@ -3,3 +3,16 @@ export const REPOSITORY = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; export const SHA = /^[a-f0-9]{40}$/; /** A conservative branch name: no leading dash, no `..` or `//`, no trailing `.` or `/`. */ export const BRANCH = /^(?!-)(?!.*\.\.)(?!.*\/\/)[A-Za-z0-9._/-]+(?; type QueueGateway = MergeGateway & MergeQueueGateway; @@ -27,6 +28,21 @@ export interface MergeStatus { remote: RemoteMergeState; queue: MergeQueueStatus | null; } export interface MergeUnavailableStatus { available: true; ready: false; action: null; blockers: MergeBlocker[]; remote: null; queue: MergeQueueStatus | null; } +/** + * With a runner block (#121), the merge targets the task's published PR, not `github.pullRequest`: the opened record + * whose opening began last among the task's records in `repository` and `baseBranch`. GitHub must show it from the task + * branch, into that base, with that record's marker as its first line, as the only open PR from the branch. + */ +export interface PublishedTarget { + repository: string; + baseBranch: string; + /** `github.pullRequest`, if the configuration still names one. It must be the task's PR. */ + configured?: number; +} +/** No PR can be inspected; the message is the merge's only blocker. */ +class MergeTargetUnavailable extends Error {} +/** The PR one status read inspected. `openingId` is set for the task's published PR, which admission re-reads. */ +interface ResolvedTarget { target?: MergeTarget; openingId: string | null; marker?: string; headBranch?: string } function queueGateway(gateway: MergeGateway): gateway is QueueGateway { const queue = gateway as Partial; @@ -44,11 +60,13 @@ export class MergeCoordinator { readonly service: ReviewService; readonly gateway: MergeGateway; readonly operationTimeoutMs: number; + readonly published: PublishedTarget | null; /** Settlement of an irreversible merge keeps its writes after the Store gate closes; request-path reconciliation does not. */ #settle: (fn: () => T) => T; - constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = MERGE_OPERATION_TIMEOUT_MS, capability?: ShutdownCapability) { + constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = MERGE_OPERATION_TIMEOUT_MS, capability?: ShutdownCapability, published?: PublishedTarget) { if (!Number.isSafeInteger(operationTimeoutMs) || operationTimeoutMs < 1 || operationTimeoutMs > MERGE_OPERATION_TIMEOUT_MS) throw new Error('Invalid merge operation deadline.'); - this.service = service; this.gateway = gateway; this.operationTimeoutMs = operationTimeoutMs; + if (published && (published.configured !== undefined && (!Number.isSafeInteger(published.configured) || published.configured < 1))) throw new Error('Invalid configured pull request.'); + this.service = service; this.gateway = gateway; this.operationTimeoutMs = operationTimeoutMs; this.published = published ?? null; this.#settle = settleWith(capability); } @@ -56,6 +74,48 @@ export class MergeCoordinator { return this.service.store?.getMergeAttempt(this.service.config.identity) ?? null; } + /** + * The PR to inspect. An attempt in flight or merged keeps the PR it was started for. Otherwise, without a runner block, + * the gateway's configured PR; with one, the task's opened PR in the configured base whose opening began last (#121). + */ + #resolve(attempt: MergeAttempt | null): ResolvedTarget { + const pinned = attempt && (attempt.state === 'submitting' || attempt.state === 'queued' || attempt.state === 'merged') ? attempt : null; + const published = this.published; + if (!published) return { ...(pinned?.pullRequest ? { target: { pullRequest: pinned.pullRequest } } : {}), openingId: null }; + const rows = this.service.store.taskPullRequests(this.service.config.identity); + const here = rows.filter(pr => pr.repository.toLowerCase() === published.repository.toLowerCase()); + const ownPullRequests = [...new Set(here.flatMap(pr => pr.number === null ? [] : [pr.number]))]; + if (pinned) { + // An attempt saved before #121 has no PR: it merged the configured one. + const number = pinned.pullRequest ?? published.configured; + if (!number) throw new MergeTargetUnavailable('This merge attempt was saved without its pull request, and github.pullRequest is not set. Set github.pullRequest to the pull request it merged.'); + return { target: { pullRequest: number, ownPullRequests }, openingId: null }; + } + if (rows.some(pr => pr.state === 'opening' || pr.refresh !== null)) + throw new MergeTargetUnavailable('The task\'s pull request is being opened or updated. Wait for publishing to finish, then refresh.'); + const latest = here.filter(pr => pr.state === 'opened' && pr.base === published.baseBranch).at(-1); + if (!latest || latest.number === null) throw new MergeTargetUnavailable(`The task has no published pull request into ${published.baseBranch}.`); + if (published.configured !== undefined && published.configured !== latest.number) + throw new MergeTargetUnavailable(`github.pullRequest is #${published.configured}, but the task's pull request is #${latest.number}. Remove github.pullRequest from the review configuration.`); + return { target: { pullRequest: latest.number, ownPullRequests, headBranch: latest.headBranch }, openingId: latest.openingId, + marker: openingMarker(latest.openingId), headBranch: latest.headBranch }; + } + + /** Whether GitHub shows the task's published PR where it was opened, as the only open PR from its branch. */ + #publishedBlockers(remote: RemoteMergeState, resolved: ResolvedTarget): MergeBlocker[] { + const number = resolved.target!.pullRequest, branch = resolved.headBranch!, base = this.published!.baseBranch, seen = remote.published; + const blocker = (message: string) => [{ code: 'pull-request', message }]; + if (remote.pullRequest !== number || !seen) return blocker(`GitHub did not report pull request #${number} as the task's pull request.`); + if (seen.crossRepository || seen.headBranch !== branch || seen.baseBranch !== base) + return blocker(`Pull request #${number} is no longer from ${branch} into ${base}. Restore its branch and base, or close it and publish the task again.`); + if (seen.marker !== resolved.marker) return blocker(`Pull request #${number} no longer starts with its codeboost marker. Restore its first line.`); + if (remote.pullRequestState !== 'OPEN') return []; + const others = seen.branchOpen.filter(pr => pr.number !== number); + if (others.length) return blocker(`Pull request #${number} must be the only open pull request from ${branch}. Close ${others.map(pr => `#${pr.number} (into ${pr.base})`).join(', ')}.`); + if (!seen.branchOpen.some(pr => pr.number === number && pr.base === base)) return blocker(`GitHub's pull request list does not show #${number} yet. Refresh later.`); + return []; + } + #current(attempt: MergeAttempt): boolean { const { store, config } = this.service; if (!store || !config) return false; @@ -84,6 +144,10 @@ export class MergeCoordinator { } async status(view = this.service.load(), fresh = false, signal?: AbortSignal): Promise { + return (await this.#status(view, fresh, signal)).status; + } + + async #status(view: ReviewView, fresh: boolean, signal?: AbortSignal): Promise<{ status: MergeStatus; resolved: ResolvedTarget }> { const blockers: MergeBlocker[] = []; for (const item of view.items) { if (item.state !== 'approved') blockers.push({ code: 'approval', message: `${item.id} is ${item.state}.` }); @@ -101,8 +165,12 @@ export class MergeCoordinator { const task = this.service.store.getTask(this.service.config.identity); if (task.status !== 'merged' && !MERGEABLE_STATUSES.includes(task.status)) blockers.push({ code: 'task', message: `The task is ${task.status}; merge it from review.` }); } - const remote = await this.gateway.inspect({ fresh, timeoutMs: fresh ? 6_000 : undefined, signal }); + const resolved = this.#resolve(this.#attempt()); + const remote = await this.gateway.inspect({ fresh, timeoutMs: fresh ? 6_000 : undefined, signal, ...(resolved.target ? { target: resolved.target } : {}) }); if (signal?.aborted) throw signal.reason; + if (resolved.target && remote.pullRequest !== resolved.target.pullRequest) throw new Error('GitHub returned a different pull request.'); + if (resolved.headBranch !== undefined) blockers.push(...this.#publishedBlockers(remote, resolved)); + if (remote.draft) blockers.push({ code: 'draft', message: `Pull request #${remote.pullRequest} is a draft; GitHub does not merge a draft.` }); if (remote.pullRequestState !== 'OPEN') blockers.push({ code: 'pr-state', message: `Pull request is ${remote.pullRequestState.toLowerCase()}.` }); if (remote.base !== view.snapshot.base) blockers.push({ code: 'base', message: 'The base branch moved. Rebase and review the resulting snapshot.' }); if (remote.head !== view.snapshot.head) blockers.push({ code: 'head', message: 'The pull request head moved. Refresh the review.' }); @@ -121,7 +189,8 @@ export class MergeCoordinator { let attempt = this.#attempt(); if (attempt?.kind === 'direct' && attempt.state === 'submitting') { - if (remote.pullRequestState === 'MERGED' && remote.head === attempt.reviewedHead) + // Only a read of the attempt's own PR settles it (#121); an attempt saved before #121 merged the PR read here. + if (remote.pullRequestState === 'MERGED' && remote.head === attempt.reviewedHead && (attempt.pullRequest == null || attempt.pullRequest === remote.pullRequest)) // Reconciling a lost response: keep GitHub's PR URL, so a replayed click reports it as the first response would. this.service.store.finishMergeAttempt(this.service.config.identity, attempt.id, { state: 'merged', ...(remote.url ? { url: remote.url } : {}) }); attempt = this.#attempt(); @@ -139,7 +208,7 @@ export class MergeCoordinator { const active = attempt?.state === 'submitting' || attempt?.state === 'queued' || attempt?.state === 'merged'; const retry = !!queue?.retryable; const ready = !active && blockers.length === 0; - return { available: true, ready, action: ready ? (retry ? 'retry' : 'merge') : null, blockers, remote, queue }; + return { status: { available: true, ready, action: ready ? (retry ? 'retry' : 'merge') : null, blockers, remote, queue }, resolved }; } async displayStatus(view = this.service.load(), signal?: AbortSignal): Promise { @@ -147,6 +216,7 @@ export class MergeCoordinator { catch (error) { if (error instanceof ShuttingDownError) throw error; if (signal?.aborted) throw signal.reason; + if (error instanceof MergeTargetUnavailable) return { available: true, ready: false, action: null, blockers: [{ code: 'pull-request', message: error.message }], remote: null, queue: this.#queueStatus() }; return { available: true, ready: false, action: null, blockers: [{ code: 'github', message: `Could not read GitHub merge state. ${error instanceof Error ? error.message : 'Unknown error.'}` }], remote: null, queue: this.#queueStatus() }; } } @@ -254,12 +324,16 @@ export class MergeCoordinator { const taskStateVersion = this.service.store && this.service.config ? this.service.store.getTask(this.service.config.identity).stateVersion : null; let view = this.service.load(); if (view.token !== token) throw new Error('Stale review state. Refresh before merging.'); - const status = await this.#statusForMerge(view, signal); + const { status, resolved } = await this.#statusForMerge(view, signal); if (!status.ready) throw new Error(status.blockers[0]?.message ?? 'Merge is blocked.'); view = this.service.load(); if (view.token !== token) throw new Error('Review changed during merge validation. Refresh before merging.'); - const finalStatus = await this.#statusForMerge(view, signal); - if (finalStatus.remote.base !== status.remote.base || finalStatus.remote.head !== status.remote.head) throw new Error('The pull request changed during merge validation. Refresh before merging.'); + const { status: finalStatus, resolved: finalTarget } = await this.#statusForMerge(view, signal); + // The same PR, opened by the same opening, in every pass (#121). + const samePullRequest = (other: { status: MergeStatus; resolved: ResolvedTarget }) => + other.status.remote.pullRequest === status.remote.pullRequest && other.resolved.openingId === resolved.openingId; + if (finalStatus.remote.base !== status.remote.base || finalStatus.remote.head !== status.remote.head || !samePullRequest({ status: finalStatus, resolved: finalTarget })) + throw new Error('The pull request changed during merge validation. Refresh before merging.'); if (finalStatus.remote.mergeQueue !== status.remote.mergeQueue) throw new Error('Merge-queue requirements changed during validation. Refresh before merging.'); if (!finalStatus.ready) throw new Error(`Merge requirements changed during validation. ${finalStatus.blockers[0]!.message}`); let commandStatus = finalStatus; @@ -267,9 +341,10 @@ export class MergeCoordinator { if (status.remote.mergeQueue) { if (!queueGateway(this.gateway)) throw new Error('This GitHub adapter cannot verify the merge-queue lifecycle.'); if (view.expected.reviewVersion === undefined) throw new Error('A current review version is required for merging.'); - queueWatermark = await this.gateway.queueWatermark(status.remote.head, { signal, timeoutMs: 6_000 }); - commandStatus = await this.#statusForMerge(view, signal); - if (commandStatus.remote.base !== finalStatus.remote.base || commandStatus.remote.head !== finalStatus.remote.head || commandStatus.remote.mergeQueue !== finalStatus.remote.mergeQueue) + queueWatermark = await this.gateway.queueWatermark(status.remote.head, { signal, timeoutMs: 6_000, pullRequest: status.remote.pullRequest }); + const command = await this.#statusForMerge(view, signal); + commandStatus = command.status; + if (commandStatus.remote.base !== finalStatus.remote.base || commandStatus.remote.head !== finalStatus.remote.head || commandStatus.remote.mergeQueue !== finalStatus.remote.mergeQueue || !samePullRequest(command)) throw new Error('Merge-queue requirements changed after queue correlation. Refresh before merging.'); if (!commandStatus.ready) throw new Error(`Merge requirements changed after queue correlation. ${commandStatus.blockers[0]!.message}`); } @@ -278,7 +353,7 @@ export class MergeCoordinator { if (this.service.store && this.service.config && view.expected.reviewVersion !== undefined) { const { store, config } = this.service, reviewVersion = view.expected.reviewVersion; const begin = () => store.beginMergeAttempt(config.identity, { ...view.expected, reviewVersion }, commandStatus.remote.head, queueWatermark, - commandStatus.remote.mergeQueue ? 'queue' : 'direct', actionId ?? null, taskStateVersion); + commandStatus.remote.mergeQueue ? 'queue' : 'direct', actionId ?? null, taskStateVersion, { pullRequest: commandStatus.remote.pullRequest, openingId: resolved.openingId }); let begun: MergeAttempt | null = null; // The attempt and the click's saved response commit in one transaction, or neither does. if (action) store.userAction(config.identity, action, () => mergeActionResponse(begun = begin())); @@ -287,7 +362,7 @@ export class MergeCoordinator { if (!begun) throw new Error('This merge click was already submitted. Refresh to see its outcome.'); queueAttempt = begun; } - const result = await this.gateway.merge(commandStatus.remote.head, { signal }); + const result = await this.gateway.merge(commandStatus.remote.head, { signal, pullRequest: commandStatus.remote.pullRequest }); if (queueAttempt) { // The enqueue command has already committed externally. A local refresh failure must not // report that action as failed; the durable submitting record is recoverable by polling. @@ -342,9 +417,9 @@ export class MergeCoordinator { } } - #statusForMerge(view: ReviewView, signal: AbortSignal): Promise { + #statusForMerge(view: ReviewView, signal: AbortSignal): Promise<{ status: MergeStatus; resolved: ResolvedTarget }> { if (signal.aborted) return Promise.reject(signal.reason); - return this.status(view, true, signal); + return this.#status(view, true, signal); } async pollQueue(): Promise { @@ -354,9 +429,12 @@ export class MergeCoordinator { if (!attempt || (attempt.state !== 'submitting' && attempt.state !== 'queued')) return this.#queueStatus(attempt); if (attempt.kind === 'direct') return this.#queueStatus(attempt); if (!queueGateway(this.gateway)) return this.#queueStatus(attempt, 'This GitHub adapter cannot verify the merge-queue lifecycle.'); + // The attempt's own PR; an attempt saved before #121 queued the configured one. + const pullRequest = attempt.pullRequest ?? this.published?.configured; + if (this.published && !pullRequest) return this.#queueStatus(attempt, 'This merge attempt was saved without its pull request, and github.pullRequest is not set. Set github.pullRequest to the pull request it queued.'); const abort = new AbortController(); this.#queueAbort = abort; - const poll = this.#pollQueue(attempt, abort.signal).finally(() => { + const poll = this.#pollQueue(attempt, pullRequest ?? undefined, abort.signal).finally(() => { if (this.#queuePoll === poll) this.#queuePoll = null; if (this.#queueAbort === abort) this.#queueAbort = null; }); @@ -366,9 +444,10 @@ export class MergeCoordinator { queueSnapshot(): MergeQueueStatus | null { return this.#queueStatus(); } - async #pollQueue(attempt: MergeAttempt, signal: AbortSignal): Promise { + async #pollQueue(attempt: MergeAttempt, pullRequest: number | undefined, signal: AbortSignal): Promise { try { - const observation = await (this.gateway as QueueGateway).inspectQueue(attempt.reviewedHead, { signal, timeoutMs: MERGE_INSPECTION_TIMEOUT_MS, afterCursor: attempt.queueWatermark ?? null }); + const observation = await (this.gateway as QueueGateway).inspectQueue(attempt.reviewedHead, { signal, timeoutMs: MERGE_INSPECTION_TIMEOUT_MS, afterCursor: attempt.queueWatermark ?? null, + ...(pullRequest !== undefined ? { pullRequest } : {}) }); this.#publishQueueObservation(attempt, observation); return this.#queueStatus(); } catch (error) { diff --git a/runner/production.ts b/runner/production.ts index e8a6e850..5965d6f1 100644 --- a/runner/production.ts +++ b/runner/production.ts @@ -8,6 +8,7 @@ import { startClaudeInvocation } from '../agents/adapters/claude.ts'; import { GhAlreadyFixedGateway } from '../github/already-fixed.ts'; import { GhIssueGateway, type IssueText } from '../github/issues.ts'; import { GhPullRequestGateway } from '../github/pull-requests.ts'; +import { baseBranch } from '../github/validate.ts'; import { identityKey } from '../core/identity.ts'; import type { RunnerDeps } from './coordinator.ts'; import { DEFAULT_DIAGNOSTICS_CAP_BYTES } from './diagnostics.ts'; @@ -136,18 +137,8 @@ export function recoveryWarnings(report: Pick `Unknown entry in the runner's attempt directory, left in place: ${quoteForTerminal(entry)}`)]; } -/** - * The branch a task's PR targets: `github.baseBranch`, required with a runner block (#103). Checked before anything is - * created; Git's own refusal would come only after a run, at the push or the opening. - */ -export function baseBranch(github: { baseBranch?: unknown } | undefined): string { - const name = github?.baseBranch; - // A short branch name, as GitHub's pull request API takes it: not a full ref (`refs/heads/main`) and not HEAD. - if (typeof name !== 'string' || !/^[A-Za-z0-9][A-Za-z0-9._/-]{0,199}$/.test(name) || /\.\.|\/\/|\/\.|@\{|\.lock(?:\/|$)|[./]$/.test(name) - || name === 'HEAD' || name.startsWith('refs/')) - throw new Error('The runner publishes pull requests: add github.baseBranch, the branch they target (for example "main"), to the review configuration.'); - return name; -} +/** Moved to github/validate.ts so the server can check it without loading the runner (#121). */ +export { baseBranch }; export interface RunnerAssembly { readonly deps: RunnerDeps; diff --git a/runner/publish.ts b/runner/publish.ts index 4b7319ff..1e49c92b 100644 --- a/runner/publish.ts +++ b/runner/publish.ts @@ -2,7 +2,7 @@ import { createHash } from 'node:crypto'; import { identityKey, type PlanIdentity } from '../core/identity.ts'; import { pullRequestBody, pullRequestTitle } from '../core/pull-request-body.ts'; import type { AlreadyFixedGateway, AlreadyFixedResult } from '../github/already-fixed.ts'; -import { DraftsUnsupported, PullRequestMisplaced, PullRequestRefused, type OpenedPullRequest, type PullRequestGateway } from '../github/pull-requests.ts'; +import { DraftsUnsupported, PullRequestMisplaced, PullRequestRefused, openingMarker, type OpenedPullRequest, type PullRequestGateway } from '../github/pull-requests.ts'; import { GuardRefusal, MERGEABLE_STATUSES, ShuttingDownError } from './lifecycle.ts'; import type { Store, TaskPullRequest } from './store.ts'; @@ -40,7 +40,7 @@ export type PublishOutcome = */ | { kind: 'no changes'; leftReady?: number }; -const marker = (openingId: string) => ``; +const marker = openingMarker; const pullRequestList = (prs: readonly { number: number }[]) => `Pull requests ${prs.map(pr => `#${pr.number}`).join(' and ')}`; /** * Tasks with a publish in progress, per Store, shared by every publisher over that Store. One publish per task at a diff --git a/runner/review.ts b/runner/review.ts index c05c224c..7d646591 100644 --- a/runner/review.ts +++ b/runner/review.ts @@ -15,7 +15,9 @@ import type { GhMergeConfig } from '../github/merge.ts'; export interface ReviewConfig { database: string; repository: string; /** The runner-owned repository (#87) holding the commits codeboost makes; required once the task has any. */ runnerRepository?: string; - identity: PlanIdentity; pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }; demo?: boolean; github?: GhMergeConfig } + identity: PlanIdentity; pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }; demo?: boolean; github?: GhMergeConfig; + /** The runner block, parsed by `parseRunnerConfig`. Its presence also makes the merge target the task's published PR (#121). */ + runner?: unknown } export class ReviewService { store: Store; config: ReviewConfig; diff --git a/runner/store.ts b/runner/store.ts index 8f8fe7ed..5fa42153 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -45,6 +45,11 @@ export interface MergeAttempt { occurredAt: string | null; createdAt: string; updatedAt: string; /** The user action that started this attempt; its saved replay response follows the attempt. */ actionId?: string | null; + /** + * The PR this attempt merges (#121). Reconciliation and queue polls read this PR, never one resolved again. Attempts saved + * before #121 have none: they targeted the configured `github.pullRequest`. + */ + pullRequest?: number | null; } /** Saved replay response for the user action that started a merge; refreshed on every attempt change. */ export function mergeActionResponse(attempt: MergeAttempt) { @@ -349,8 +354,16 @@ export class Store { if (typeof reason !== 'string' || !reason.trim() || reason.length > 4000) throw new Error('Invalid cancellation reason.'); this.#run("UPDATE requests SET state='cancelled',reason=? WHERE id=? AND key=? AND state IN ('pending','ready')", reason.trim(), id, identityKey(identity)); } - beginMergeAttempt(identity: PlanIdentity, expected: ReviewState & { reviewVersion: number }, reviewedHead: string, queueWatermark: string | null = null, kind: MergeAttempt['kind'] = 'queue', actionId: string | null = null, expectedTaskStateVersion: number | null = null): MergeAttempt { + /** + * `target`: the PR the attempt merges. With an `openingId` (the task's published PR, #121), that opening must still be + * the task's opened record in its base whose opening began last, with that number, and no opening or update of the + * task's PRs may be in flight. + */ + beginMergeAttempt(identity: PlanIdentity, expected: ReviewState & { reviewVersion: number }, reviewedHead: string, queueWatermark: string | null = null, kind: MergeAttempt['kind'] = 'queue', actionId: string | null = null, expectedTaskStateVersion: number | null = null, + target: { pullRequest: number; openingId: string | null } | null = null): MergeAttempt { sha(reviewedHead); + if (target !== null && (!Number.isSafeInteger(target.pullRequest) || target.pullRequest < 1 || (target.openingId !== null && typeof target.openingId !== 'string'))) + throw new Error('Invalid merge target.'); if (actionId !== null) assertUuidV4(actionId, 'Action ID'); if (!['queue','direct'].includes(kind)) throw new Error('Invalid merge attempt kind.'); if (queueWatermark !== null && (typeof queueWatermark !== 'string' || !queueWatermark || queueWatermark.length > 512)) throw new Error('Invalid merge-queue event cursor.'); @@ -370,11 +383,19 @@ export class Store { const current = this.getMergeAttempt(identity); if (current?.state === 'submitting' || current?.state === 'queued') throw new Error('A merge-queue attempt is already active.'); if (current?.state === 'merged') throw new Error('The reviewed pull request is already merged.'); + if (target?.openingId) { + if (this.#get("SELECT 1 FROM task_pull_requests WHERE plan_key=? AND (state='opening' OR refresh_head IS NOT NULL)", key)) + throw new GuardRefusal('The task\'s pull request is being opened or updated. Wait for publishing to finish, then refresh.'); + const row = this.#get('SELECT rowid, state, number, base, repository FROM task_pull_requests WHERE plan_key=? AND opening_id=?', key, target.openingId); + const newer = row && this.#get("SELECT 1 FROM task_pull_requests WHERE plan_key=? AND state='opened' AND rowid>? AND base=? AND lower(repository)=lower(?)", + key, row.rowid as number, row.base as string, row.repository as string); + if (row?.state !== 'opened' || row.number !== target.pullRequest || newer) throw new GuardRefusal('The task\'s pull request changed during merge validation. Refresh before merging.'); + } const now = new Date().toISOString(); const attempt: MergeAttempt = { id: randomUUID(), kind, state: 'submitting', revision: expected.revision, snapshotId: expected.snapshotId, reviewVersion: expected.reviewVersion, reviewedHead, queueWatermark, url: null, reason: null, requiresFreshReview: false, - entryId: null, phase: null, position: null, occurredAt: null, createdAt: now, updatedAt: now, actionId, + entryId: null, phase: null, position: null, occurredAt: null, createdAt: now, updatedAt: now, actionId, pullRequest: target?.pullRequest ?? null, }; this.#run('INSERT INTO merge_attempts VALUES (?,?,?)', attempt.id, key, encode(attempt)); this.#touch(key); diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index e4b0af0b..e7fd7491 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -23,7 +23,7 @@ test.beforeEach(async () => { root=mkdtempSync(join(tmpdir(),'codeboost-browser- test.afterEach(async () => { await app.close();rmSync(root,{recursive:true,force:true}); }); test('resends the same merge key after a 503, a lost response, an unreadable body or an unknown outcome, and a new key after a definite answer',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config); - const gateway:MergeGateway={inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>{throw new Error('The browser test answers every merge request itself.');}}; + const gateway:MergeGateway={inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async()=>{throw new Error('The browser test answers every merge request itself.');}}; app=await startServer(config,0,undefined,gateway); let view=app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); const keys:string[]=[],answers:Array<'503'|'lost'|'garbled'|'unknown'|'409'>=['503','lost','garbled','unknown','409','409']; @@ -38,7 +38,7 @@ test('resends the same merge key after a 503, a lost response, an unreadable bod }); test('mints a new merge key for a retry once the attempt started by a retained key has ended',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config); - const gateway:MergeGateway={inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>{throw new Error('The browser test answers every merge request itself.');}}; + const gateway:MergeGateway={inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async()=>{throw new Error('The browser test answers every merge request itself.');}}; app=await startServer(config,0,undefined,gateway); let view=app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); const keys:string[]=[]; @@ -66,7 +66,7 @@ test('answers 503 when the merge deadline stops a click, so the browser may rese }); test('resends a retained merge key with the request it was made for, even after a review edit and Refresh',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config); - const gateway:MergeGateway={inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>{throw new Error('The browser test answers every merge request itself.');}}; + const gateway:MergeGateway={inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async()=>{throw new Error('The browser test answers every merge request itself.');}}; app=await startServer(config,0,undefined,gateway); let view=app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); const sent:{actionId:string;token:string}[]=[]; @@ -159,7 +159,7 @@ test('requires private credentials and rejects foreign origins',async({request}) test('shows merge blockers and submits one exact-head merge',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let mergeCalls:string[]=[];let release!:()=>void;const held=new Promise(resolve=>{release=resolve;}); const gateway:MergeGateway={ - inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};}, + inspect:async()=>{const snapshot=app.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, merge:async head=>{mergeCalls.push(head);await held;app.service.load=()=>{throw new Error('post-command reload failed');};app.service.store.getMergeAttempt=()=>{throw new Error('post-command queue read failed');};return {url:'https://github.com/example/repo/pull/21'};}, }; app=await startServer(config,0,undefined,gateway);await page.goto(app.url); @@ -171,7 +171,7 @@ test('shows merge blockers and submits one exact-head merge',async({page})=>{ test('keeps the reviewed head queued until confirmed merged and preserves current input',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app,phase:'queued'|'merged'='queued',queueReads=0; const gateway:MergeGateway&MergeQueueGateway={ - inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear'};}, + inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, queueWatermark:async()=>null, merge:async()=>({url:'https://github.com/example/repo/pull/24'}), inspectQueue:async head=>{queueReads++;return phase==='queued'?{state:'queued',reviewedHead:head,entryId:'MQE_1',phase:'AWAITING_CHECKS',position:2,enqueuedAt:'2026-09-24T08:00:00Z',queueHead:head}:{state:'merged',reviewedHead:head,mergedAt:'2026-09-24T08:10:00Z'};}, @@ -184,14 +184,14 @@ test('keeps the reviewed head queued until confirmed merged and preserves curren test('backs off repeated merge-queue polling',async({page})=>{ await page.addInitScript(()=>{const delays:number[]=[];(window as typeof window&{__mergePollDelays:number[]}).__mergePollDelays=delays;const native=window.setTimeout.bind(window);window.setTimeout=((handler:TimerHandler,timeout?:number,...args:unknown[])=>{if(typeof timeout==='number')delays.push(timeout);return native(handler,timeout,...args);}) as typeof window.setTimeout;}); const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app,queueReads=0; - const gateway:MergeGateway&MergeQueueGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear'};},queueWatermark:async()=>null,merge:async()=>({url:'https://github.com/example/repo/pull/24'}),inspectQueue:async head=>{queueReads++;return{state:'queued',reviewedHead:head,entryId:'MQE_1',phase:'QUEUED',position:1,enqueuedAt:'2026-09-24T08:00:00Z',queueHead:head};}}; + const gateway:MergeGateway&MergeQueueGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},queueWatermark:async()=>null,merge:async()=>({url:'https://github.com/example/repo/pull/24'}),inspectQueue:async head=>{queueReads++;return{state:'queued',reviewedHead:head,entryId:'MQE_1',phase:'QUEUED',position:1,enqueuedAt:'2026-09-24T08:00:00Z',queueHead:head};}}; app=appRef=await startServer(config,0,undefined,gateway);let view=app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); await page.goto(app.url);page.on('dialog',dialog=>dialog.accept());await page.getByRole('button',{name:'Merge PR',exact:true}).click();await expect.poll(()=>queueReads,{timeout:10000}).toBeGreaterThanOrEqual(2); const delays=await page.evaluate(()=>(window as typeof window&{__mergePollDelays:number[]}).__mergePollDelays.filter(value=>value>=500));expect(delays.slice(0,2)).toEqual([2000,4000]); }); test('does not queue-poll an ambiguous direct merge attempt',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app,polls=0; - const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>({url:''})}; + const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async()=>({url:''})}; app=appRef=await startServer(config,0,undefined,gateway);const view=app.service.load();app.service.store.beginMergeAttempt(config.identity,{...view.expected,reviewVersion:view.expected.reviewVersion!},view.snapshot.head,null,'direct'); await page.route('**/api/merge',async route=>{polls++;await route.continue();});await page.goto(app.url);await expect(page.getByRole('button',{name:'Submitting…',exact:true})).toBeDisabled();await page.waitForTimeout(2500);expect(polls).toBe(0); }); @@ -199,7 +199,7 @@ test('surfaces queue removal and retries only the same reviewed head',async({pag test.slow(); const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app,mergeCalls=0; const gateway:MergeGateway&MergeQueueGateway={ - inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear'};}, + inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, queueWatermark:async()=>null, merge:async()=>{mergeCalls++;return {url:'https://github.com/example/repo/pull/24'};}, inspectQueue:async head=>mergeCalls===1?{state:'removed',reviewedHead:head,removedAt:'2026-09-24T08:05:00Z',reason:'Required check failed.'}:{state:'queued',reviewedHead:head,entryId:'MQE_2',phase:'QUEUED',position:1,enqueuedAt:'2026-09-24T08:06:00Z',queueHead:head}, @@ -211,7 +211,7 @@ test('surfaces queue removal and retries only the same reviewed head',async({pag test('ignores a merge poll started before a newer review action',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app; const gateway:MergeGateway&MergeQueueGateway={ - inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear'};}, + inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, queueWatermark:async()=>null, merge:async()=>({url:'https://github.com/example/repo/pull/24'}), inspectQueue:async head=>({state:'removed',reviewedHead:head,removedAt:'2026-09-24T08:05:00Z',reason:'Old poll result.'}), @@ -226,7 +226,7 @@ test('ignores a merge poll started before a newer review action',async({page})=> test('requires fresh review instead of retry when GitHub replaces the queued head',async({page})=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app; const gateway:MergeGateway&MergeQueueGateway={ - inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear'};}, + inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, queueWatermark:async()=>null, merge:async()=>({url:'https://github.com/example/repo/pull/24'}),inspectQueue:async()=>{throw new Error('The pull request head changed after review.');}, }; @@ -234,7 +234,7 @@ test('requires fresh review instead of retry when GitHub replaces the queued hea await page.goto(app.url);page.on('dialog',dialog=>dialog.accept());await page.getByRole('button',{name:'Merge PR',exact:true}).click();await expect(page.locator('#banner')).toContainText('The pull request head changed after review.',{timeout:10000});await expect(page.getByRole('button',{name:'Retry merge',exact:true})).toHaveCount(0);await expect(page.locator('#merge')).toBeDisabled(); }); test('keeps stale merge failures disabled until refresh',async({page})=>{ - const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app;const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>{throw new Error('head changed');}};app=appRef=await startServer(config,0,undefined,gateway); + const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app;const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async()=>{throw new Error('head changed');}};app=appRef=await startServer(config,0,undefined,gateway); let view=app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); await page.goto(app.url);page.on('dialog',dialog=>dialog.accept());await page.getByRole('button',{name:'Merge PR',exact:true}).click();await expect(page.locator('#banner')).toContainText('Merge blocked. head changed');await expect(page.locator('#merge')).toBeDisabled(); }); @@ -565,7 +565,7 @@ test('drains an in-flight question request before closing its agent manager',asy test('drains an admitted merge request before closing its coordinator',async()=>{ const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app,started!:(value?:void)=>void,release!:(value?:void)=>void,commandSignal:AbortSignal|undefined; const commandStarted=new Promise(resolve=>{started=resolve;}),held=new Promise(resolve=>{release=resolve;}); - const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async(_head,options)=>{commandSignal=options?.signal;started();await held;return {url:'https://github.com/example/repo/pull/24'};}}; + const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async(_head,options)=>{commandSignal=options?.signal;started();await held;return {url:'https://github.com/example/repo/pull/24'};}}; app=appRef=await startServer(config,0,undefined,gateway);let view=app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); const endpoint=new URL('/api/action',app.url),body=JSON.stringify({action:'merge',token:view.token,actionId:randomUUID()});let status=0; const completed=new Promise((resolve,reject)=>{const req=httpRequest(endpoint,{method:'POST',headers:{'x-codeboost-token':app.token,'content-type':'application/json','content-length':Buffer.byteLength(body)}},res=>{status=res.statusCode??0;res.resume();res.on('end',resolve);});req.on('error',reject);req.end(body);}); @@ -574,7 +574,7 @@ test('drains an admitted merge request before closing its coordinator',async()=> test('bounds shutdown draining before aborting active queue polling',async()=>{ const config={...app.service.config,demo:false};await app.close();let appRef:typeof app,started!:(value?:void)=>void,settled=false; const pollingStarted=new Promise(resolve=>{started=resolve;}); - const gateway:MergeGateway&MergeQueueGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear'};},queueWatermark:async()=>null,merge:async()=>({url:'https://github.com/example/repo/pull/24'}),inspectQueue:async(_head,options)=>new Promise((_resolve,reject)=>{started();options?.signal?.addEventListener('abort',()=>{settled=true;reject(options.signal?.reason);},{once:true});})}; + const gateway:MergeGateway&MergeQueueGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},queueWatermark:async()=>null,merge:async()=>({url:'https://github.com/example/repo/pull/24'}),inspectQueue:async(_head,options)=>new Promise((_resolve,reject)=>{started();options?.signal?.addEventListener('abort',()=>{settled=true;reject(options.signal?.reason);},{once:true});})}; app=appRef=await startServer(config,0,undefined,gateway,50);const view=app.service.load(),attempt=app.service.store.beginMergeAttempt(config.identity,{...view.expected,reviewVersion:view.expected.reviewVersion!},view.snapshot.head);app.service.store.queueMergeAttempt(config.identity,attempt.id,'https://github.com/example/repo/pull/24'); const response=fetch(new URL('/api/merge',app.url),{headers:{'x-codeboost-token':app.token}});await pollingStarted;await app.close();expect(settled).toBe(true);expect((await response).status).toBe(409);app=await startServer(config,0); }); @@ -594,7 +594,7 @@ test('destroys a partial request body after the shutdown drain',async()=>{ }); test('blocks a partially received merge request when shutdown starts',async()=>{ const config={...app.service.config,demo:false};await app.close();let mergeCalls=0;let appRef:typeof app; - const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>{mergeCalls++;return {url:''};}}; + const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};},merge:async()=>{mergeCalls++;return {url:''};}}; app=appRef=await startServer(config,0,undefined,gateway);let view=app.service.load(); for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token}); for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); diff --git a/test/merge-task-pr.test.ts b/test/merge-task-pr.test.ts new file mode 100644 index 00000000..9fc8cc19 --- /dev/null +++ b/test/merge-task-pr.test.ts @@ -0,0 +1,459 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { randomUUID } from 'node:crypto'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { Store } from '../runner/store.ts'; +import { GuardRefusal } from '../runner/lifecycle.ts'; +import { MergeCoordinator, type PublishedTarget } from '../runner/merge.ts'; +import type { ReviewService } from '../runner/review.ts'; +import { GhMergeGateway, type MergeGateway, type MergeQueueGateway, type MergeTarget, type RemoteMergeState } from '../github/merge.ts'; +import { openingMarker } from '../github/pull-requests.ts'; +import type { AlreadyFixedGateway, AlreadyFixedInput } from '../github/already-fixed.ts'; +import type { Plan, PlanContext } from '../core/plan.ts'; +import { createDemo } from '../scripts/demo.ts'; +import { startServer } from '../web/server.ts'; + +// #121: with a runner block, the merge gate targets the task's published PR, not github.pullRequest. +const oid = (n: number) => n.toString(16).padStart(40, '0'); +const identity = { repositoryId: 'repo', taskId: 'Task_42', planId: 'plan' }; +const plan: Plan = { schema_version: 1, issue: 12, revision: 1, summary: 'Stop the crash', questions: [], items: [ + { id: 'P1', title: 'Guard input', intent: 'Reject empty input', files: [{ path: 'a.ts', kind: 'edit', renamed_from: null, change: 'Check it' }], acceptance: [{ type: 'check', text: 'Works' }], depends_on: [] }] }; +const context: PlanContext = { identity, issue: 12, baseEntries: [{ path: 'a.ts', kind: 'file' }], pathKey: p => p, allowedCommands: [] }; +const BRANCH = 'codeboost/issue-12-task-42-0123456789abcdef'; +const PUBLISHED: PublishedTarget = { repository: 'owner/repo', baseBranch: 'main' }; +const url = (n: number) => `https://github.com/owner/repo/pull/${n}`; + +const stores: Store[] = [], roots: string[] = []; +afterEach(() => { for (const store of stores.splice(0)) store.close(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); + +/** A running task with head `oid(2)` over `oid(1)`, its last attempt settled. */ +function runningTask(): Store { + const store = new Store(':memory:'); stores.push(store); + store.createPlan(JSON.stringify(plan), 'json', context, oid(1), oid(2)); + store.recordHistory(identity, { revision: 1, snapshotId: store.getSnapshot(identity).id }, oid(1), oid(2), [{ sha: oid(2), owner: 'P1', origin: 'owned', sourceSha: null }]); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + const attempt = store.admitAttempt(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P1', expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); + store.markRunning(identity, attempt.id); + store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 0, valid: true }); + return store; +} +const clear = (store: Store) => store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: oid(9) } }); +const begin = (store: Store, base = 'main') => store.beginPullRequest(identity, { checkId: clear(store).id, repository: 'owner/repo', base, headBranch: BRANCH, headSha: oid(2), draft: false }); +const opened = (store: Store, openingId: string, number: number, mayReview = true) => + store.recordPullRequestOpened(identity, openingId, { number, url: url(number), headSha: oid(2), draft: false }, mayReview); +const toReview = (store: Store) => store.transitionTask(identity, store.getTask(identity).stateVersion, 'in review'); + +/** The runner published PR 7 for the task, which is now in review. */ +function published() { + const store = runningTask(); + const opening = begin(store); + expect(opened(store, opening.openingId, 7)).toBe('in review'); + return { store, opening }; +} +/** + * PR 7 is opened and running; a later opening was abandoned, so a test can adopt it as PR 8 (newer than 7) at any point. + * The task is then in review. + */ +function withLaterOpening() { + const store = runningTask(); + const first = begin(store); opened(store, first.openingId, 7, false); + const later = begin(store); store.abandonPullRequestOpening(identity, later.openingId); + toReview(store); + const adopt = () => store.adoptOpening(identity, later.openingId, { number: 8, url: url(8), draft: false }, { stateVersion: store.getTask(identity).stateVersion, reviewVersion: store.reviewVersion(identity) }); + return { store, first, later, adopt }; +} + +function service(store: Store) { + const snapshot = store.getSnapshot(identity); + const view = { + items: [{ id: 'P1', state: 'approved', outside: [], acceptance: [{ type: 'check', text: 'Works' }], checks: { tests: '– No tests defined' } }], + plan: { revision: 1 }, segments: [], notes: [], snapshot: { id: snapshot.id, base: oid(1), head: oid(2) }, token: 'review-token', + expected: { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, + }; + return { store, config: { identity }, load: vi.fn(() => view) } as unknown as ReviewService; +} + +/** + * GitHub as the published PR's record says it is: open, ready, from the task branch into main, with its marker, the only + * open PR from the branch. `change` overrides what GitHub shows for a PR number. + */ +function github(store: Store, change: (number: number) => Partial = () => ({})) { + const targets: Array = [], merged: Array = [], queued: Array = []; + const client: MergeGateway & MergeQueueGateway & { before?: () => void } = { + inspect: vi.fn(async options => { + client.before?.(); + targets.push(options?.target); + const number = options?.target?.pullRequest ?? 99; + const record = store.taskPullRequests(identity).find(pr => pr.number === number); + return { base: oid(1), head: oid(2), pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], + alreadyFixed: 'clear', pullRequest: number, draft: false, + ...(options?.target?.headBranch ? { published: { headBranch: options.target.headBranch, baseBranch: 'main', crossRepository: false, + marker: record ? openingMarker(record.openingId) : '', branchOpen: [{ number, base: 'main' }] } } : {}), + ...change(number) } as RemoteMergeState; + }), + merge: vi.fn(async (_head, options) => { merged.push(options?.pullRequest); return { url: url(options?.pullRequest ?? 99) }; }), + queueWatermark: vi.fn(async () => 'CURSOR'), + inspectQueue: vi.fn(async (head, options) => { queued.push(options?.pullRequest); return { state: 'queued' as const, reviewedHead: head, entryId: 'E', phase: 'QUEUED' as const, position: 0, enqueuedAt: '2026-10-03T00:00:00Z', queueHead: head }; }), + }; + return { client, targets, merged, queued }; +} + +describe('the merge gate with a runner block (#121)', () => { + it('inspects and merges the task\'s published PR and pins it on the attempt', async () => { + const { store } = published(); + const gh = github(store); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + const status = await merges.status(); + expect(status.blockers).toEqual([]); + expect(gh.targets[0]).toEqual({ pullRequest: 7, ownPullRequests: [7], headBranch: BRANCH }); + await merges.merge('review-token'); + expect(gh.merged).toEqual([7]); + expect(store.getMergeAttempt(identity)).toMatchObject({ pullRequest: 7, state: 'merged' }); + }); + + it('refuses when github.pullRequest names another PR, and accepts it when it names the task\'s PR', async () => { + const { store } = published(); + const gh = github(store); + const wrong = new MergeCoordinator(service(store), gh.client, undefined, undefined, { ...PUBLISHED, configured: 5 }); + const shown = await wrong.displayStatus(); + expect(shown.ready).toBe(false); + expect(shown.blockers).toEqual([{ code: 'pull-request', message: 'github.pullRequest is #5, but the task\'s pull request is #7. Remove github.pullRequest from the review configuration.' }]); + const actionId = randomUUID(); + await expect(wrong.merge('review-token', actionId)).rejects.toThrow(/github.pullRequest is #5/); + expect(gh.client.inspect).not.toHaveBeenCalled(); + const right = new MergeCoordinator(service(store), gh.client, undefined, undefined, { ...PUBLISHED, configured: 7 }); + expect((await right.status()).ready).toBe(true); + // A definite refusal is saved: a resend replays it, even to a coordinator that would now merge. + await expect(right.merge('review-token', actionId)).rejects.toThrow(/github.pullRequest is #5/); + expect(gh.merged).toEqual([]); + }); + + it('blocks while the task has no PR into the configured base, or while one is being opened', async () => { + const store = runningTask(); + const gh = github(store); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + const opening = begin(store); toReview(store); + expect((await merges.displayStatus()).blockers).toEqual([{ code: 'pull-request', message: 'The task\'s pull request is being opened or updated. Wait for publishing to finish, then refresh.' }]); + store.abandonPullRequestOpening(identity, opening.openingId); + expect((await merges.displayStatus()).blockers).toEqual([{ code: 'pull-request', message: 'The task has no published pull request into main.' }]); + // A PR into another base is the task's, but not one the merge can target. + store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); + const other = runningTask(); const elsewhere = begin(other, 'release'); opened(other, elsewhere.openingId, 9); + expect((await new MergeCoordinator(service(other), github(other).client, undefined, undefined, PUBLISHED).displayStatus()).blockers) + .toEqual([{ code: 'pull-request', message: 'The task has no published pull request into main.' }]); + expect(gh.client.inspect).not.toHaveBeenCalled(); + }); + + const faults: Array<[string, (n: number, marker: string) => Partial, RegExp]> = [ + ['a draft', () => ({ draft: true }), /^Pull request #7 is a draft/], + ['an edited first line', () => ({ published: { headBranch: BRANCH, baseBranch: 'main', crossRepository: false, marker: 'Fixes #12', branchOpen: [{ number: 7, base: 'main' }] } }), /no longer starts with its codeboost marker/], + ['a renamed branch', (_, marker) => ({ published: { headBranch: 'other', baseBranch: 'main', crossRepository: false, marker, branchOpen: [{ number: 7, base: 'main' }] } }), /no longer from codeboost\/.* into main/], + ['a retargeted base', (_, marker) => ({ published: { headBranch: BRANCH, baseBranch: 'release', crossRepository: false, marker, branchOpen: [{ number: 7, base: 'release' }] } }), /no longer from .* into main/], + ['a fork', (_, marker) => ({ published: { headBranch: BRANCH, baseBranch: 'main', crossRepository: true, marker, branchOpen: [{ number: 7, base: 'main' }] } }), /no longer from/], + ['another open PR from the branch', (_, marker) => ({ published: { headBranch: BRANCH, baseBranch: 'main', crossRepository: false, marker, branchOpen: [{ number: 7, base: 'main' }, { number: 8, base: 'release' }] } }), /must be the only open pull request from .*Close #8 \(into release\)/], + ['a list that does not show it yet', (_, marker) => ({ published: { headBranch: BRANCH, baseBranch: 'main', crossRepository: false, marker, branchOpen: [] } }), /does not show #7 yet/], + ['no published fields', () => ({ published: undefined }), /did not report pull request #7/], + ]; + for (const [name, change, message] of faults) it(`blocks a published PR with ${name}`, async () => { + const { store, opening } = published(); + const gh = github(store, n => change(n, openingMarker(opening.openingId))); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + const status = await merges.status(); + expect(status.ready).toBe(false); + expect(status.blockers.map(blocker => blocker.message)).toEqual([expect.stringMatching(message)]); + await expect(merges.merge('review-token')).rejects.toThrow(message); + expect(gh.merged).toEqual([]); + }); + + it('does not ask a merged PR to still be open on its branch', async () => { + const { store, opening } = published(); + const gh = github(store, () => ({ pullRequestState: 'MERGED', published: { headBranch: BRANCH, baseBranch: 'main', crossRepository: false, marker: openingMarker(opening.openingId), branchOpen: [] } })); + const status = await new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED).status(); + expect(status.blockers.map(blocker => blocker.code)).toEqual(['pr-state']); + }); + + it('excludes every PR the task recorded in this repository from the already-fixed check', async () => { + const { store, adopt } = withLaterOpening(); + adopt(); + // A PR in another repository has a number of that repository: never one of this repository's own PRs. + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + const settled = store.admitAttempt(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P1', expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); + store.markRunning(identity, settled.id); store.settleAttempt(identity, settled.id, { firstReason: null, exitCode: 0, valid: true }); + const foreign = store.beginPullRequest(identity, { checkId: clear(store).id, repository: 'fork/repo', base: 'main', headBranch: BRANCH, headSha: oid(2), draft: false }); + opened(store, foreign.openingId, 3, false); + toReview(store); + const gh = github(store); + await new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED).status(); + expect(gh.targets[0]).toEqual({ pullRequest: 8, ownPullRequests: [7, 8], headBranch: BRANCH }); + }); + + it('refuses when the task\'s PR changes between the validation passes', async () => { + const { store, adopt } = withLaterOpening(); + const gh = github(store); + // Recorded during the first pass's GitHub read, so the second pass resolves the newer PR. + let reads = 0; + gh.client.before = () => { if (++reads === 1) adopt(); }; + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + await expect(merges.merge('review-token')).rejects.toThrow('The pull request changed during merge validation. Refresh before merging.'); + expect(gh.targets.map(target => target?.pullRequest)).toEqual([7, 8]); + expect(gh.merged).toEqual([]); + expect(store.getMergeAttempt(identity)).toBeNull(); + }); + + it('in queue mode, reads the watermark of the resolved PR and refuses a PR change found by the third pass', async () => { + const { store, adopt } = withLaterOpening(); + const gh = github(store, () => ({ mergeQueue: true })); + let reads = 0; + gh.client.before = () => { if (++reads === 2) adopt(); }; + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + await expect(merges.merge('review-token')).rejects.toThrow('Merge-queue requirements changed after queue correlation. Refresh before merging.'); + expect(gh.client.queueWatermark).toHaveBeenCalledWith(oid(2), expect.objectContaining({ pullRequest: 7 })); + expect(gh.targets.map(target => target?.pullRequest)).toEqual([7, 7, 8]); + expect(gh.merged).toEqual([]); + }); + + it('does not settle an attempt for another PR that was admitted while this status read was in flight', async () => { + const { store } = published(); + const view = service(store).load(); + // No PR is pinned when the read starts; the attempt for PR 8 is admitted by another coordinator during the read. + const gh = github(store, () => ({ pullRequest: 99, pullRequestState: 'MERGED' })); + gh.client.before = () => { store.beginMergeAttempt(identity, view.expected as never, oid(2), null, 'direct', null, null, { pullRequest: 8, openingId: null }); }; + await new MergeCoordinator(service(store), gh.client).status(); + expect(store.getMergeAttempt(identity)).toMatchObject({ state: 'submitting', pullRequest: 8 }); + }); + + it('blocks a draft without a runner block too', async () => { + const { store } = published(); + const status = await new MergeCoordinator(service(store), github(store, () => ({ draft: true })).client).status(); + expect(status.blockers).toEqual([{ code: 'draft', message: 'Pull request #99 is a draft; GitHub does not merge a draft.' }]); + }); + + it('settles a direct attempt only from its own PR, even after a newer PR is recorded', async () => { + const { store, first, adopt } = withLaterOpening(); + const view = service(store).load(); + const attempt = store.beginMergeAttempt(identity, view.expected as never, oid(2), null, 'direct', null, null, { pullRequest: 7, openingId: first.openingId }); + // A newer PR recorded while the attempt is in flight; the attempt keeps the PR it was started for. + adopt(); + const gh = github(store, n => (n === 7 ? { pullRequestState: 'MERGED' } : {})); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + await merges.status(); + expect(gh.targets[0]).toEqual({ pullRequest: 7, ownPullRequests: [7, 8] }); + expect(store.getMergeAttempt(identity)).toMatchObject({ id: attempt.id, state: 'merged', pullRequest: 7 }); + }); + + it('never settles an attempt from the state of another PR', async () => { + const { store, opening: first } = published(); + const view = service(store).load(); + const attempt = store.beginMergeAttempt(identity, view.expected as never, oid(2), null, 'direct', null, null, { pullRequest: 7, openingId: first.openingId }); + const gh = github(store, () => ({ pullRequest: 8, pullRequestState: 'MERGED' })); + const shown = await new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED).displayStatus(); + expect(shown.blockers[0]).toMatchObject({ code: 'github', message: expect.stringMatching(/different pull request/) }); + expect(store.getMergeAttempt(identity)).toMatchObject({ id: attempt.id, state: 'submitting' }); + }); + + it('polls the queue for the attempt\'s own PR; an attempt saved without one uses github.pullRequest or reports why it cannot', async () => { + const { store, opening: first } = published(); + const view = service(store).load(); + store.beginMergeAttempt(identity, view.expected as never, oid(2), 'CURSOR', 'queue', null, null, { pullRequest: 7, openingId: first.openingId }); + const gh = github(store); + await new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED).pollQueue(); + expect(gh.queued).toEqual([7]); + + const legacy = published(); + const legacyView = service(legacy.store).load(); + legacy.store.beginMergeAttempt(identity, legacyView.expected as never, oid(2), 'CURSOR', 'queue'); + expect(legacy.store.getMergeAttempt(identity)!.pullRequest).toBeNull(); + const old = github(legacy.store); + const before = legacy.store.getMergeAttempt(identity); + const unknown = await new MergeCoordinator(service(legacy.store), old.client, undefined, undefined, PUBLISHED).pollQueue(); + expect(unknown?.observationError).toMatch(/saved without its pull request/); + expect(legacy.store.getMergeAttempt(identity)).toEqual(before); + expect((await new MergeCoordinator(service(legacy.store), old.client, undefined, undefined, PUBLISHED).displayStatus()).blockers).toEqual([ + { code: 'pull-request', message: expect.stringMatching(/saved without its pull request/) }]); + expect(old.queued).toEqual([]); + await new MergeCoordinator(service(legacy.store), old.client, undefined, undefined, { ...PUBLISHED, configured: 5 }).pollQueue(); + expect(old.queued).toEqual([5]); + }); + + it('keeps the configured PR without a runner block, and pins the PR GitHub reported', async () => { + const { store } = published(); + const gh = github(store); + const merges = new MergeCoordinator(service(store), gh.client); + await merges.merge('review-token'); + expect(gh.targets).toEqual([undefined, undefined]); + expect(gh.merged).toEqual([99]); + expect(store.getMergeAttempt(identity)!.pullRequest).toBe(99); + }); +}); + +describe('admission re-reads the task\'s PR record (#121)', () => { + const admit = (store: Store, target: { pullRequest: number; openingId: string | null }) => + store.beginMergeAttempt(identity, service(store).load().expected as never, oid(2), null, 'direct', null, null, target); + it('refuses a PR that is no longer the task\'s latest, or another number', () => { + const { store, first, later, adopt } = withLaterOpening(); + expect(() => admit(store, { pullRequest: 9, openingId: first.openingId })).toThrow(GuardRefusal); + adopt(); + expect(() => admit(store, { pullRequest: 7, openingId: first.openingId })).toThrow('The task\'s pull request changed during merge validation. Refresh before merging.'); + expect(admit(store, { pullRequest: 8, openingId: later.openingId }).pullRequest).toBe(8); + }); + it('refuses while an opening is in flight, or when the record was abandoned; a newer PR elsewhere does not refuse', () => { + const { store, first, later } = withLaterOpening(); + // Another base and another repository are not this base's latest. + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + const rerun = () => { const a = store.admitAttempt(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P1', expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); + store.markRunning(identity, a.id); store.settleAttempt(identity, a.id, { firstReason: null, exitCode: 0, valid: true }); }; + rerun(); opened(store, begin(store, 'release').openingId, 10, false); + opened(store, store.beginPullRequest(identity, { checkId: clear(store).id, repository: 'fork/repo', base: 'main', headBranch: BRANCH, headSha: oid(2), draft: false }).openingId, 11, false); + const inFlight = begin(store); + toReview(store); + expect(() => admit(store, { pullRequest: 7, openingId: first.openingId })).toThrow('The task\'s pull request is being opened or updated. Wait for publishing to finish, then refresh.'); + store.abandonPullRequestOpening(identity, inFlight.openingId); + expect(() => admit(store, { pullRequest: 8, openingId: later.openingId })).toThrow('The task\'s pull request changed during merge validation. Refresh before merging.'); + expect(admit(store, { pullRequest: 7, openingId: first.openingId }).pullRequest).toBe(7); + }); + it('refuses while an update of the task\'s PR is in flight', () => { + const store = runningTask(); + const opening = begin(store); opened(store, opening.openingId, 7, false); + store.beginRefresh(identity, { checkId: clear(store).id, openingId: opening.openingId, headSha: oid(2), draft: false }); + toReview(store); + expect(() => admit(store, { pullRequest: 7, openingId: opening.openingId })).toThrow('The task\'s pull request is being opened or updated. Wait for publishing to finish, then refresh.'); + }); +}); + +describe('GhMergeGateway with a target (#121)', () => { + const marker = openingMarker('0f0e0d0c-0b0a-4908-8706-050403020100'); + function fake(over: { pull?: Record; list?: unknown } = {}) { + const calls: string[][] = [], checks: AlreadyFixedInput[] = []; + const run = async (args: readonly string[]) => { + calls.push([...args]); const joined = args.join(' '); + if (args[0] === 'pr' && args[1] === 'view') return JSON.stringify({ number: Number(args[2]), isDraft: false, baseRefName: 'main', baseRefOid: oid(1), headRefName: BRANCH, headRefOid: oid(2), state: 'OPEN', + mergeable: 'MERGEABLE', statusCheckRollup: [], body: `${marker}\r\nFixes #12`, isCrossRepository: false, ...over.pull }); + if (joined.includes('/pulls?')) return JSON.stringify(over.list ?? [{ number: 7, head: { ref: BRANCH }, base: { ref: 'main' } }]); + 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 }); + if (args[0] === 'pr' && args[1] === 'merge') return ''; + throw new Error(`Unexpected gh call: ${joined}`); + }; + const check: AlreadyFixedGateway = { repository: 'owner/repo', check: async input => { checks.push(input); return { outcome: 'clear', baseHead: oid(9) }; } }; + return { calls, checks, client: new GhMergeGateway({ repository: 'owner/repo', issue: 12 }, run, check) }; + } + const target: MergeTarget = { pullRequest: 7, ownPullRequests: [3], headBranch: BRANCH }; + + it('reads where the published PR is, its first line and the open PRs from its branch', async () => { + const { calls, checks, client } = fake(); + const state = await client.inspect({ target }); + expect(state).toMatchObject({ pullRequest: 7, draft: false, published: { headBranch: BRANCH, baseBranch: 'main', crossRepository: false, marker, branchOpen: [{ number: 7, base: 'main' }] } }); + expect(calls[0]).toEqual(['pr', 'view', '7', '--repo', 'owner/repo', '--json', 'number,isDraft,baseRefName,baseRefOid,headRefName,headRefOid,state,mergeable,statusCheckRollup,url,body,isCrossRepository']); + expect(calls).toContainEqual(['api', '-H', 'Accept: application/vnd.github+json', `repos/owner/repo/pulls?state=open&head=owner%3A${encodeURIComponent(BRANCH)}&per_page=100`]); + expect(checks[0]!.ownPullRequests).toEqual([7, 3]); + }); + + it('reads no description or branch list for a configured PR', async () => { + const { calls, client } = fake(); + await expect(client.inspect()).rejects.toThrow(/No pull request to merge/); + const state = await client.inspect({ target: { pullRequest: 7 } }); + expect(state.published).toBeUndefined(); + expect(calls.some(args => args.join(' ').includes('/pulls?'))).toBe(false); + expect(calls.find(args => args[1] === 'view')!.at(-1)).toBe('number,isDraft,baseRefName,baseRefOid,headRefName,headRefOid,state,mergeable,statusCheckRollup,url'); + }); + + const malformed: Array<[string, Parameters[0], RegExp]> = [ + ['another PR number', { pull: { number: 8 } }, /different pull request/], + ['no draft flag', { pull: { isDraft: undefined } }, /incomplete pull request state/], + ['no description', { pull: { body: undefined } }, /incomplete pull request state/], + ['no fork flag', { pull: { isCrossRepository: undefined } }, /incomplete pull request state/], + ['a full page of PRs from the branch', { list: Array.from({ length: 100 }, (_, i) => ({ number: i + 1, head: { ref: BRANCH }, base: { ref: 'main' } })) }, /100 or more/], + ['a listed PR from another branch', { list: [{ number: 7, head: { ref: 'other' }, base: { ref: 'main' } }] }, /invalid pull request list/], + ['a listed PR without a base', { list: [{ number: 7, head: { ref: BRANCH } }] }, /invalid pull request list/], + ['a list that is not a list', { list: { message: 'Not Found' } }, /invalid pull request list/], + ['a null list entry', { list: [null] }, /invalid pull request list/], + ['a listed PR without a valid number', { list: [{ number: '7', head: { ref: BRANCH }, base: { ref: 'main' } }] }, /invalid pull request list/], + ]; + for (const [name, over, message] of malformed) it(`fails closed on ${name}`, async () => { + await expect(fake(over).client.inspect({ target })).rejects.toThrow(message); + }); + + it('stops and awaits the rule reads before rejecting when the branch list fails', async () => { + let release!: () => void, stopped: AbortSignal | undefined; + const held = new Promise(resolve => { release = resolve; }); + const base = fake({ list: { message: 'not a list' } }); + const run = async (args: readonly string[], options?: { signal?: AbortSignal }) => { + if (args.join(' ').includes('/rules/branches/')) { stopped = options?.signal; await held; throw new Error('stopped'); } + return (base.client.run)(args, options); + }; + const client = new GhMergeGateway({ repository: 'owner/repo', issue: 12 }, run, base.client.checks); + let settled = false; + const inspection = client.inspect({ target }).finally(() => { settled = true; }); + inspection.catch(() => {}); + await vi.waitFor(() => expect(stopped?.aborted).toBe(true)); + await new Promise(resolve => setTimeout(resolve, 20)); + expect(settled).toBe(false); + release(); + await expect(inspection).rejects.toThrow(/invalid pull request list/); + }); + + it('shares an inspection in flight only with a request for the same PR', async () => { + const { calls, client } = fake(); + const [seven, again, eight] = await Promise.all([client.inspect({ target: { pullRequest: 7 } }), client.inspect({ target: { pullRequest: 7 } }), client.inspect({ target: { pullRequest: 8 } })]); + expect([seven.pullRequest, again.pullRequest, eight.pullRequest]).toEqual([7, 7, 8]); + expect(calls.filter(args => args[1] === 'view').map(args => args[2])).toEqual(['7', '8']); + }); + + it('reports a draft as GitHub shows it', async () => { + expect((await fake({ pull: { isDraft: true } }).client.inspect({ target: { pullRequest: 7 } })).draft).toBe(true); + }); + + it('does not serve one PR\'s cached state for another', async () => { + const { calls, client } = fake(); + await client.inspect({ target: { pullRequest: 7 } }); + const other = await client.inspect({ target: { pullRequest: 8 } }); + expect(other.pullRequest).toBe(8); + expect(calls.filter(args => args[1] === 'view').map(args => args[2])).toEqual(['7', '8']); + }); + + it('merges and reads the queue of the PR it is given', async () => { + const calls: string[][] = []; + const run = async (args: readonly string[]) => { calls.push([...args]); if (args[1] === 'graphql') throw new Error('stop'); return ''; }; + const client = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 5, issue: 12 }, run); + await client.merge(oid(2), { pullRequest: 8 }); + expect(calls[0]).toEqual(['pr', 'merge', '8', '--repo', 'owner/repo', '--merge', '--match-head-commit', oid(2)]); + await expect(client.queueWatermark(oid(2), { pullRequest: 8 })).rejects.toThrow('stop'); + await expect(client.inspectQueue(oid(2), { pullRequest: 8 })).rejects.toThrow('stop'); + expect(calls.slice(1).map(args => args.find(arg => arg.startsWith('number=')))).toEqual(['number=8', 'number=8']); + await client.merge(oid(2)); + expect(calls.at(-1)![2]).toBe('5'); + }); + + it('accepts a configuration without a PR, but not an invalid one', () => { + expect(() => new GhMergeGateway({ repository: 'owner/repo', issue: 12 })).not.toThrow(); + expect(() => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 0, issue: 12 })).toThrow(/valid pull request number/); + }); +}); + +describe('the server chooses the merge target (#121)', { timeout: 60_000 }, () => { + function demo() { + const root = mkdtempSync(join(tmpdir(), 'codeboost-merge-target-')); roots.push(root); + const config = createDemo(join(root, 'demo')); + const store = new Store(config.database); const issue = store.getPlan(config.identity).issue; store.close(); + return { config, issue }; + } + it('requires github.pullRequest without a runner block', async () => { + const { config, issue } = demo(); + await expect(startServer({ ...config, demo: false, github: { repository: 'owner/repo', issue } }, 0)).rejects.toThrow(/Add github.pullRequest/); + }); + it('requires github.baseBranch with a runner block', async () => { + const { config, issue } = demo(); + await expect(startServer({ ...config, demo: false, runner: {}, github: { repository: 'owner/repo', issue } }, 0)).rejects.toThrow(/github.baseBranch/); + }); + it('targets the task\'s published PR with a runner block, without github.pullRequest', async () => { + const { config, issue } = demo(); + const app = await startServer({ ...config, demo: false, runner: {}, github: { repository: 'owner/repo', issue, baseBranch: 'main' } }, 0); + try { + const response = await fetch(`${new URL(app.url).origin}/api/review`, { headers: { 'x-codeboost-token': app.token } }); + const view = await response.json() as { merge: { blockers: unknown[] } }; + expect(view.merge.blockers).toEqual([{ code: 'pull-request', message: 'The task has no published pull request into main.' }]); + } finally { await app.close(); } + }); +}); diff --git a/test/merge.test.ts b/test/merge.test.ts index 1ede078b..336c04c0 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -29,7 +29,7 @@ function alreadyFixedReads(args: readonly string[], fake: IssueFake = {}): strin /** Answers the merge adapter's own reads: an open PR 7 into main with no rules or protection. */ function mergeReads(args: readonly string[], pull: Record = {}): string { const joined = args.join(' '); - if (joined.startsWith('pr view 7')) return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [], ...pull }); + if (joined.startsWith('pr view 7')) return JSON.stringify({ number: 7, isDraft: false, 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 }); @@ -44,7 +44,7 @@ function readyView(): ReviewView { function serviceFor(view: ReviewView): ReviewService { return { load: vi.fn(() => view) } as unknown as ReviewService; } function remote(view: ReviewView, change: Partial = {}): RemoteMergeState { - return { base: view.snapshot.base, head: view.snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', ...change }; + return { base: view.snapshot.base, head: view.snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false, ...change }; } function gateway(states: RemoteMergeState[]): MergeGateway & { heads: string[] } { @@ -886,7 +886,7 @@ it('parses required checks from both rule sources and pins the gh merge head', a let pullReads = 0; const run = async (args: readonly string[]) => { calls.push([...args]); const joined = args.join(' '); - if (joined.startsWith('pr view 7')) { pullReads++; return JSON.stringify({ baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [ + if (joined.startsWith('pr view 7')) { pullReads++; return JSON.stringify({ number: 7, isDraft: false, baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [ { name: 'test', status: 'COMPLETED', conclusion: 'SUCCESS', app: { databaseId: 10 } }, { name: 'race', status: 'IN_PROGRESS', conclusion: 'SUCCESS' }, { context: 'lint', state: 'SUCCESS' }, @@ -1121,7 +1121,7 @@ it.each([ ] as const)('fails closed for an invalid %s check app identity', async (_source, rules, requiredStatusChecks) => { 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: [{ name: 'test', status: 'COMPLETED', conclusion: 'SUCCESS', app: { databaseId: 10 } }] }); + if (joined.startsWith('pr view')) return JSON.stringify({ number: 7, isDraft: false, baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [{ name: 'test', status: 'COMPLETED', conclusion: 'SUCCESS', app: { databaseId: 10 } }] }); 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 }); @@ -1164,7 +1164,7 @@ it('preserves caller cancellation while reading merge status', async () => { it.each([[false, true], [true, false]])('treats a protection 404 with protected=%s as rulesKnown=%s', async (protectedBranch, expectedKnown) => { 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')) return JSON.stringify({ number: 7, isDraft: false, 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: protectedBranch }); if (joined.endsWith('/protection')) throw new Error('HTTP 404: Not Found'); @@ -1179,7 +1179,7 @@ it.each([[false, true], [true, false]])('treats a protection 404 with protected= it.each([false, 'required', []])('fails closed for malformed classic protection metadata: %j', async requiredStatusChecks => { 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')) return JSON.stringify({ number: 7, isDraft: false, 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: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: requiredStatusChecks }); @@ -1193,7 +1193,7 @@ it.each([false, 'required', []])('fails closed for malformed classic protection it('fails closed for a ruleset entry without a type', 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.startsWith('pr view')) return JSON.stringify({ number: 7, isDraft: false, 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: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: { strict: true, checks: [] } }); @@ -1207,7 +1207,7 @@ it('fails closed for a ruleset entry without a type', async () => { it('does not treat empty strict check policies as an atomic base guard', 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.startsWith('pr view')) return JSON.stringify({ number: 7, isDraft: false, baseRefName: 'main', baseRefOid: sha('a'), headRefName: 'feature', headRefOid: sha('b'), state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [] }); 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: [] } }); @@ -1485,7 +1485,7 @@ it('does not let an inspection started before merge repopulate the cache', async 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 view')) { pullReads++; return JSON.stringify({ number: 7, isDraft: false, 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 }); @@ -1512,7 +1512,7 @@ it.each([ ])('fails closed for inconsistent classic protection fields: %j', async requiredStatusChecks => { 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')) return JSON.stringify({ number: 7, isDraft: false, 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: true }); if (joined.endsWith('/protection')) return JSON.stringify({ required_status_checks: requiredStatusChecks }); diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index 25765855..d5395a63 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -88,7 +88,7 @@ describe('merge coordinator after the gate closes', () => { store.createPlan(JSON.stringify(plan), 'json', { identity, issue: 1, baseEntries: [{ path: 'a', kind: 'file' }], pathKey: p => p, allowedCommands: [] }, oid(1), oid(2)); const view = { items: [{ id: 'P1', state: 'approved', outside: [], acceptance: [], checks: {} }], plan: { revision: 1 }, segments: [], notes: [], snapshot: { id: store.getSnapshot(identity).id, base: oid(1), head: oid(2) }, token: 't', expected: { revision: 1, snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity) } } as unknown as ReturnType; const service = { store, config: { identity }, load: vi.fn(() => view) } as unknown as ReviewService; - const state: RemoteMergeState = { base: oid(1), head: oid(2), pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', ...remote }; + const state: RemoteMergeState = { base: oid(1), head: oid(2), pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false, ...remote }; const gateway: MergeGateway & MergeQueueGateway = { inspect: vi.fn(async () => state), merge: vi.fn(async () => ({ url: 'https://github.example/pr/1' })), queueWatermark: vi.fn(async () => 'c'), inspectQueue: vi.fn(async () => ({ state: 'merged', reviewedHead: oid(2), mergedAt: '2026-09-26T00:00:00Z' }) as never), diff --git a/web/server.ts b/web/server.ts index fa14b5fd..25031913 100644 --- a/web/server.ts +++ b/web/server.ts @@ -10,6 +10,7 @@ import { RunnerCoordinator, type RunnerDeps } from '../runner/coordinator.ts'; import { ItemExecutor } from '../runner/execution.ts'; import { TaskPublishing } from '../runner/publishing.ts'; import type { RunnerAssembly } from '../runner/production.ts'; +import { baseBranch } from '../github/validate.ts'; import type { ShutdownCapability } from '../runner/lifecycle.ts'; import { BadRequest, GuardRefusal, ShuttingDownError, assertUuidV4, isUuidV4, sameContext } from '../runner/lifecycle.ts'; import { GhIssueGateway, type IssueGateway } from '../github/issues.ts'; @@ -50,7 +51,12 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // Issue retrieval is read-only, so demos may show it; they use a local fixture and never contact GitHub. issues = new IssueBoard(issueGateway ?? (config.demo ? demoIssueGateway() : config.github ? new GhIssueGateway(config.github.repository) : null), 'Issue ranking needs a GitHub repository. Add a github block with a repository to the review configuration.'); - merges = !config.demo && (mergeGateway || config.github) ? new MergeCoordinator(service, mergeGateway ?? new GhMergeGateway(config.github!), MERGE_OPERATION_TIMEOUT_MS, capability) : null; + // With a runner block the merge targets the task's published PR (#121); without one, github.pullRequest is required. + const published = !config.demo && config.runner !== undefined && config.github + ? { repository: config.github.repository, baseBranch: baseBranch(config.github), ...(config.github.pullRequest !== undefined ? { configured: config.github.pullRequest } : {}) } : undefined; + if (!config.demo && config.github && !published && !mergeGateway && config.github.pullRequest === undefined) + throw new Error('Add github.pullRequest, the pull request to merge, to the review configuration, or add a runner block so codeboost publishes its own.'); + merges = !config.demo && (mergeGateway || config.github) ? new MergeCoordinator(service, mergeGateway ?? new GhMergeGateway(config.github!), MERGE_OPERATION_TIMEOUT_MS, capability, published) : null; if (runnerDeps && runnerSetup) throw new Error('Pass runner deps or a runner setup, not both.'); // Without a runner (no runner block, or a demo), runner actions report that it is not configured. runner = runnerDeps ? new RunnerCoordinator(service.store, runnerDeps, undefined, capability) : null;