From 06e6303d9b45c0e523a57ed9ac11d50a27368600 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 3 Oct 2026 20:16:02 -0700 Subject: [PATCH 1/4] F: merge the task's published PR, not github.pullRequest (#121) With a runner block, the guarded merge gate now inspects, merges and polls the task's published pull request: the task's latest opened record in github.baseBranch, which GitHub must show from the task branch, into that base, with its marker, as the only open PR from the branch, and not a draft. github.pullRequest becomes optional with a runner block and blocks merging when it names another PR; without a runner block it is required and used as before. The resolved PR must be the same in every validation pass; admission re-reads the PR record in the transaction that saves the attempt, and the attempt pins its PR so reconciliation and queue polls never re-resolve it. The pre-merge already-fixed check treats every PR the task recorded in the repository as its own. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +- docs/implementation/guarded-merge.md | 21 +- docs/implementation/merge-queue.md | 2 +- docs/implementation/pull-request-opening.md | 5 +- github/merge.ts | 174 ++++++-- github/pull-requests.ts | 2 + github/validate.ts | 13 + runner/merge.ts | 117 ++++- runner/production.ts | 15 +- runner/publish.ts | 4 +- runner/review.ts | 4 +- runner/store.ts | 24 +- test/browser/review.spec.ts | 28 +- test/merge-task-pr.test.ts | 459 ++++++++++++++++++++ test/merge.test.ts | 20 +- test/runner-shutdown.test.ts | 2 +- web/server.ts | 8 +- 17 files changed, 789 insertions(+), 113 deletions(-) create mode 100644 test/merge-task-pr.test.ts diff --git a/README.md b/README.md index 3652ba19..c6840661 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`. When a task is cancelled, codeboost closes the open pull requests it opened for it (the branch is kept). A pull request a person moved is left open and reported, and `close-pull-requests` retries a close that failed. 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`. When a task is cancelled, codeboost closes the open pull requests it opened for it (the branch is kept). A pull request a person moved is left open and reported, and `close-pull-requests` retries a close that failed. 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 878e805e..ccce90fb 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. @@ -128,12 +128,13 @@ Some repositories do not support draft PRs (for example private repositories on `TaskPublishing` (`runner/publishing.ts`) owns publishing in production. `setUpRunner` builds the publisher with the real gateways (`GhAlreadyFixedGateway`, `GhPullRequestGateway`) and a `GitBranchPusher` that pushes from the runner-owned repository to `pushUrl(github.repository)` on `GH_HOST`, never to a configured URL. The pusher may overwrite only commits the task's ledger records as `owned`. Before #22 adds rebasing, nothing in production writes a `foreign` ledger entry (only `recordRebase` does), so codeboost pushes no head the ledger calls foreign. -- **What is published.** The whole task head, `base..head` (#113). Before the runner's first commit, the review records the user's checkout, so a task usually starts from the branch's own commits: a person made them, and the ledger has no entry for them, so they count as foreign (the design: a commit not in the ledger is foreign). The runner's commits come on top. All of them reach the `codeboost/…` branch and the PR, as the design intends. Review shows a foreign commit's changes in the Unplanned row, and the merge gate blocks (`unplanned`) until a person assigns or accepts each one. The gate inspects and merges the PR named in `github.pullRequest`, not yet the task's published PR, so this protects the published PR only when the two are the same; pointing the gate at the published PR is a follow-up. The push may overwrite only an owned commit already on the branch. Mapping commits through a rebase is #22's. +- **What is published.** The whole task head, `base..head` (#113). Before the runner's first commit, the review records the user's checkout, so a task usually starts from the branch's own commits: a person made them, and the ledger has no entry for them, so they count as foreign (the design: a commit not in the ledger is foreign). The runner's commits come on top. All of them reach the `codeboost/…` branch and the PR, as the design intends. Review shows a foreign commit's changes in the Unplanned row, and the merge gate blocks (`unplanned`) until a person assigns or accepts each one. With a runner block, the gate inspects and merges the task's published PR (#121), so this blocker protects that PR. The push may overwrite only an owned commit already on the branch. Mapping commits through a rebase is #22's. - **Base branch.** `github.baseBranch` is required with a runner block. The `gh` calls run with the same environment as the push (its `GH_HOST` and credentials). The CLI checks it before it takes the lock, and `setUpRunner` checks it again before anything is created. - **Mode.** The task's status decides it. A `running` task whose every plan item has completed at the current revision gets a ready PR. A task in `needs human`, by any path (a safety finding, the time budget), gets a draft PR. Its problems say what sent it there now: its current attempt's safety finding (or, when that finding's save failed, the attempt's diagnostic that carries it), else "time budget ran out" when the budget has passed, else that attempt's diagnostic. A budget that ran out after a safety finding does not hide the finding. An earlier attempt's finding, which a person may have dealt with, is not used. Any other status, an active attempt, a run still in progress, or a publish already in progress refuses. A draft waits only for an owed safety finding, not an owed scope pause: a draft invites no review, and a task in needs human cannot record a pause. A ready publish also needs every item of the current plan revision to have completed at that revision. The Store's publish guard checks this before every guarded write, so a revision that adds an item while a publish awaits GitHub refuses it. So does work an earlier run still owes: a safety finding not yet acted on, or a scope pause never recorded (its write failed, or the process stopped). Every plan item has run, so no later run would pay it, and a ready PR would show unreviewed out-of-scope changes. A run's end, startup and a refused `publish` action therefore have the executor pay it first (`ItemExecutor.payOwed`, as a run does before its first item). The task then moves to needs amendment or needs human, and a draft follows for the latter. For a task already in needs human, only an owed safety finding is paid. That draft names the paid finding, which only this process holds. It does so only while the task stays in needs human on the same current attempt. A later run that returns the task to needs human has its own reason. - **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 through `publishOwed`, which first runs the check that the runner lock still names the database, because a publish pushes; if the check fails, nothing starts. A task recorded as `not published` by the v10 upgrade stays unpublished until a person uses the `publish` action, or until a run starts after the upgrade (its end is the first state the upgrade did not decide), or a person's `start` or `resume` moves the task. Otherwise 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. A publish or close that ends with an opening still in flight (its own reply was lost, or it was refused while one settles) is tried again at that opening's own deadline, plus a second (`settleRemaining`; a timer, cleared at shutdown). A short retry, after 30 seconds, follows a publish refused because GitHub's PR list lags behind a recorded PR, one refused by a guard (mostly a race: the task, its review or its head changed during the publish; a refusal that persists is in the mode too, so the retry finds nothing owed), one refused just as an opening's deadline passed, and one whose open or update left work behind. That last case covers GitHub showing another head than the one pushed (the PR is made a draft and the task stays running), and a draft change that failed. Such an `opened` outcome is recorded with `reconcile`, which keeps the publish owed. Retries are bounded: one timer per task, and per chain at most one retry at an opening's deadline (while it is still ahead) and one short retry. A chain ends when a job settles. A new one starts with a run's end, a cancel, a person's action or a restart, never with a retry's own job. Whatever is still unsettled after them is stuck for a reason a person must fix (two PRs carry a marker, the repository changed, GitHub keeps failing every open), so the next publish or close waits for a run's end, a restart or an action. - **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. Its state version is the one the publish last saw (`seenVersion`, #114), which every outcome carries. Where a path ends right after a Store write or guarded read, it is the version read there. Where a GitHub call came after the last write (a draft change that failed and left the PR ready, or one refused because drafts are unsupported), it is the version from before that call. So a change someone else made during the call still leaves a publish owed. A refusal, a failure or a stop is stamped with the current version, and stays owed anyway. A close is stamped with the current version. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. Before it starts, every publish or close (after a run, a cancel, at startup or on an action) records itself as `publishing` or `closing`, its durable in-flight ownership. If that write fails, the publish does not run. A `publishing` or `closing` record left at startup is a publish whose process stopped before it recorded an outcome. Startup runs the job again if one is owed, and otherwise settles the record as `stopped`; the task's PR records show what that job did. When only an action reply is stuck (its job never recorded itself as started), the reply reports the outcome on record, and that record is left as it is. A failure while paying owed work does not stop this. A reply that still says `publishing` (or `closing`) belongs to an action whose process stopped (a crash) before its publish recorded an outcome. Any later outcome of the task settles it too, since one publish or close runs per task. At startup, if no publish is owed, it is settled as `stopped` ("interrupted before it recorded an outcome"), so no replay says `publishing` or `closing` for ever. 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: it stops the publish, and the close below handles what the publish left. +- **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. `test/runner-publishing.test.ts` covers the server wiring (#103) with a local bare repository as the remote and GitHub faked: a completed run opens one ready PR with the branch at the task head, a needs-human task gets a draft with its problems, a branch holding someone else's commit is refused and the `publish` action retries it (and its replay reports the outcome), a refused resume that moves the task to needs human publishes a draft while one on a task already there publishes nothing, start, resume and a second publish are refused while a publish runs, shutdown during the push or during the opening closes the Store only after the publish settled, a PR whose reply arrives just after the abort is still recorded before the write gate closes, and the next start publishes once or recovers the opening by its marker. Removing the startup publish, the after-run publish, the publish after a refused resume, the status-change check, the busy guard, the replay update or the shutdown wait, or moving that wait after the write gate, each fails a test. For #111 it also covers: a cancelled task in review gets its ready PR closed with the branch kept; a cancelled needs-human task gets its draft closed; a cancel during a draft opening is answered at once, stops the publish, and the PR GitHub opened meanwhile is recovered by its marker and closed; a failed close is owed and the `close-pull-requests` action retries it; shutdown during a close leaves it to the next start; a PR GitHub's list does not show yet is closed by its number; a replayed or refused cancel starts nothing; a close refused while the opening settles is retried by itself and closes the PR GitHub showed late; and a task with no PR calls GitHub for nothing. `test/publish.test.ts` checks that the gateway closes only the task's own open PR (branch, repository, marker, number), re-checks right before the close, and requires GitHub to show it closed, and that the publisher closes only a cancelled task's PRs, by number, and nothing once shutdown begins between the read and the close. Removing the cancel hook, the close after a stopped publish, the owed close, the close by number, the replay check, the retry timer or the re-check before the close each fails a test. It also publishes a head that holds the branch's own (foreign) commit, and checks that review shows it as Unplanned and the merge gate blocks on it (#113); removing that blocker fails the test. `test/runner-branch-push.test.ts` replays a leftover push, with the pusher's own arguments against a real bare remote, before, between and after a newer publish's read and push (#112); dropping the lease fails the last two. `test/already-fixed.test.ts` and `test/publish.test.ts` cover each row above. Each guard was also broken on purpose, and a test failed each time: own-PR and own-commit exclusion, repository-qualified exclusion, every bound, the ancestor check, the fail-closed catch, the issue-number boundary, the state-version and head re-read, the status check before the no-changes shortcut, the version-owned status change, the open-state check, the settle time, the branch hash, the environment allowlist, reuse of the earlier PR, lost-opening recovery, adoption on the main path and counting that PR as the task's own, drafting a PR that a task which cannot publish still has ready, adopting an abandoned opening's PR that appears after the task stopped, the abort checks on the no-changes path and in that draft step (a lookup that fails, one that answers, a draft change that fails and one that lands), recording a late PR by adoption alone, an abort during the head-mismatch draft change, the `closing` checks before a push and before a ready change, `close()` aborting a publish in progress, the repository check, the definite 422 refusal, a retargeted own PR and another base's PR from the branch, refusals matched on stderr and GitHub's `errors[].message` only, the configured base in recovery and the head settle, the draft step finding the task's PR in any base, closing-only cross-references, both sides of a manual link, the query text, the fence length, and the description bounds. 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 eed67d00..a2cb9196 100644 --- a/github/pull-requests.ts +++ b/github/pull-requests.ts @@ -99,6 +99,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 task's latest opened + * record in `repository` and `baseBranch`, which GitHub must show 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 latest opened PR in the configured base (#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 fb67a1eb..15d2bf6e 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'; @@ -137,18 +138,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 e9967b7d..f22523bc 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'; @@ -47,7 +47,7 @@ export type PublishOutcome = ( seenVersion: 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 eb38929f..eb2928a6 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) { @@ -353,8 +358,15 @@ 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 latest opened record in its base, 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.'); @@ -374,11 +386,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 b7ef8425..569d9de3 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 afa140eb..b60bf411 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 { OWED_REFUSAL, 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'; @@ -64,7 +65,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; From a4c7ad494ed32b7667f3f447506f149163c96db5 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 3 Oct 2026 20:58:58 -0700 Subject: [PATCH 2/4] F: name the merge target as the opening that began last (#121) Co-Authored-By: Claude Opus 5.5 --- runner/merge.ts | 8 ++++---- runner/store.ts | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/runner/merge.ts b/runner/merge.ts index c5b85e6c..e9bd46e5 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -29,9 +29,9 @@ export interface MergeStatus { } 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 task's latest opened - * record in `repository` and `baseBranch`, which GitHub must show from the task branch, into that base, with that record's - * marker as its first line, as the only open PR from the branch. + * 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; @@ -76,7 +76,7 @@ export class MergeCoordinator { /** * 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 latest opened PR in the configured base (#121). + * 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; diff --git a/runner/store.ts b/runner/store.ts index eb2928a6..416a7cda 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -359,8 +359,9 @@ export class Store { this.#run("UPDATE requests SET state='cancelled',reason=? WHERE id=? AND key=? AND state IN ('pending','ready')", reason.trim(), id, identityKey(identity)); } /** - * `target`: the PR the attempt merges. With an `openingId` (the task's published PR, #121), that opening must still be the - * task's latest opened record in its base, with that number, and no opening or update of the task's PRs may be in flight. + * `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 { From a2c6dbacc8a74a9bba92c7acefad44c45b43ce66 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:09:03 -0700 Subject: [PATCH 3/4] F: give the #113 merge-gate test the PR number and draft flag (#121) Co-Authored-By: Claude Opus 5.5 --- test/runner-publishing.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index d92f787b..d71d5b74 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -418,7 +418,7 @@ describe('publishing a finished task (#103)', () => { const view = app.service.load(); expect(view.segments.filter(segment => segment.row === 'Unplanned').map(segment => segment.path)).toContain('debug.log'); const gateway: MergeGateway = { inspect: async () => ({ base: view.snapshot.base, head: view.snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', - rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear' }), merge: async () => { throw new Error('not merged here'); } }; + rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 100, draft: false }), merge: async () => { throw new Error('not merged here'); } }; const status = await new MergeCoordinator(app.service, gateway).status(view); expect(status).toMatchObject({ ready: false }); expect(status.blockers).toContainEqual(expect.objectContaining({ code: 'unplanned' })); From 6b29a8a36493fc8c34cd7f9587c3bdf9a1226f54 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 19:08:36 -0700 Subject: [PATCH 4/4] F: keep the review's blockers when no merge target resolves; test the configured PR through the server (#121) Review round 2 on PR #126 (independent agent): - displayStatus showed only the target blocker when no PR could be resolved, hiding approval, task-status and merge-attempt blockers. It now shows the attempt's blocker first, then the target's, then the review's own. - The server's passing of github.pullRequest as the configured PR is now tested end to end, and #113's unplanned-blocker test runs the gate in published mode. - guarded-merge.md: the in-flight update case names the record's refresh field, not a blocker code. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- runner/merge.ts | 35 +++++++++++++++++---------- test/merge-task-pr.test.ts | 36 +++++++++++++++++++++++++++- test/runner-publishing.test.ts | 15 ++++++++---- 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index c512769e..fc56f535 100644 --- a/docs/implementation/guarded-merge.md +++ b/docs/implementation/guarded-merge.md @@ -14,7 +14,7 @@ Derived planted-review configurations clear the source GitHub binding because th 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`); +- an opening of the task's PRs is still in flight, or an update of one is (its record's `refresh` field is set); - 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. diff --git a/runner/merge.ts b/runner/merge.ts index e9bd46e5..a692cf0a 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -39,8 +39,8 @@ export interface PublishedTarget { /** `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 {} +/** No PR can be inspected. The display keeps the blockers found before the target was resolved. */ +class MergeTargetUnavailable extends Error { blockers: MergeBlocker[] = []; } /** 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 } @@ -165,7 +165,9 @@ 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 resolved = this.#resolve(this.#attempt()); + let resolved: ResolvedTarget; + try { resolved = this.#resolve(this.#attempt()); } + catch (error) { if (error instanceof MergeTargetUnavailable) error.blockers = blockers; throw error; } 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.'); @@ -196,27 +198,34 @@ export class MergeCoordinator { attempt = this.#attempt(); } const queue = this.#queueStatus(attempt); - if (attempt?.state === 'submitting' || attempt?.state === 'queued') { - blockers.unshift({ code: 'queue-active', message: attempt.state === 'submitting' - ? attempt.reason ? `The merge submission outcome is unknown. ${attempt.reason} Waiting for GitHub reconciliation.` : attempt.kind === 'queue' ? 'The reviewed head is being submitted to the merge queue.' : 'The reviewed head is being submitted for direct merge.' - : 'The reviewed head is queued. Waiting for GitHub to confirm the outcome.' }); - } else if (attempt?.state === 'merged') { - blockers.unshift({ code: 'queue-merged', message: 'GitHub confirmed that the reviewed head was merged.' }); - } else if (attempt && !this.#freshReviewComplete(attempt)) { - blockers.unshift({ code: 'queue-head', message: attempt.reason ?? 'The pull request snapshot changed after the queue attempt. Review the replacement snapshot.' }); - } + const attemptBlocker = this.#attemptBlocker(attempt); + if (attemptBlocker) blockers.unshift(attemptBlocker); const active = attempt?.state === 'submitting' || attempt?.state === 'queued' || attempt?.state === 'merged'; const retry = !!queue?.retryable; const ready = !active && blockers.length === 0; return { status: { available: true, ready, action: ready ? (retry ? 'retry' : 'merge') : null, blockers, remote, queue }, resolved }; } + /** What the merge attempt on record says, shown first. */ + #attemptBlocker(attempt: MergeAttempt | null): MergeBlocker | null { + if (attempt?.state === 'submitting' || attempt?.state === 'queued') return { code: 'queue-active', message: attempt.state === 'submitting' + ? attempt.reason ? `The merge submission outcome is unknown. ${attempt.reason} Waiting for GitHub reconciliation.` : attempt.kind === 'queue' ? 'The reviewed head is being submitted to the merge queue.' : 'The reviewed head is being submitted for direct merge.' + : 'The reviewed head is queued. Waiting for GitHub to confirm the outcome.' }; + if (attempt?.state === 'merged') return { code: 'queue-merged', message: 'GitHub confirmed that the reviewed head was merged.' }; + if (attempt && !this.#freshReviewComplete(attempt)) return { code: 'queue-head', message: attempt.reason ?? 'The pull request snapshot changed after the queue attempt. Review the replacement snapshot.' }; + return null; + } + async displayStatus(view = this.service.load(), signal?: AbortSignal): Promise { try { return await this.status(view, false, signal); } 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() }; + if (error instanceof MergeTargetUnavailable) { + const attempt = this.#attempt(), attemptBlocker = this.#attemptBlocker(attempt); + const blockers = [...(attemptBlocker ? [attemptBlocker] : []), { code: 'pull-request', message: error.message }, ...error.blockers]; + return { available: true, ready: false, action: null, blockers, remote: null, queue: this.#queueStatus(attempt) }; + } 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() }; } } diff --git a/test/merge-task-pr.test.ts b/test/merge-task-pr.test.ts index 9fc8cc19..8282a5c0 100644 --- a/test/merge-task-pr.test.ts +++ b/test/merge-task-pr.test.ts @@ -269,13 +269,25 @@ describe('the merge gate with a runner block (#121)', () => { 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); + // The attempt on record is still shown first. expect((await new MergeCoordinator(service(legacy.store), old.client, undefined, undefined, PUBLISHED).displayStatus()).blockers).toEqual([ + { code: 'queue-active', message: 'The reviewed head is being submitted to the merge queue.' }, { 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('shows the review\'s own blockers beside one that names no PR to inspect', async () => { + const store = runningTask(); toReview(store); + const reviewing = service(store); + (reviewing.load() as unknown as { items: Array<{ state: string }> }).items[0]!.state = 'pending'; + const shown = await new MergeCoordinator(reviewing, github(store).client, undefined, undefined, PUBLISHED).displayStatus(); + expect(shown.blockers).toEqual([ + { code: 'pull-request', message: 'The task has no published pull request into main.' }, + { code: 'approval', message: 'P1 is pending.' }]); + }); + it('keeps the configured PR without a runner block, and pins the PR GitHub reported', async () => { const { store } = published(); const gh = github(store); @@ -453,7 +465,29 @@ describe('the server chooses the merge target (#121)', { timeout: 60_000 }, () = 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.' }]); + expect(view.merge.blockers[0]).toEqual({ code: 'pull-request', message: 'The task has no published pull request into main.' }); + } finally { await app.close(); } + }); + it('blocks when github.pullRequest names another PR than the task\'s published one', async () => { + const { config, issue } = demo(); + // The demo task runs every item and publishes PR 7. + const store = new Store(config.database), id = config.identity; + try { + store.transitionTask(id, store.getTask(id).stateVersion, 'queued'); + for (const { id: item } of store.getPlan(id).items) { + const attempt = store.admitAttempt(id, { expectedStateVersion: store.getTask(id).stateVersion, kind: 'execute', item, expectedContext: store.currentContext(id), deadline: Date.now() + 60_000 }); + store.markRunning(id, attempt.id); store.settleAttempt(id, attempt.id, { firstReason: null, exitCode: 0, valid: true }); + } + const check = store.recordAlreadyFixed(id, store.getTask(id).stateVersion, { snapshotId: store.getSnapshot(id).id, reviewVersion: store.reviewVersion(id), draft: false, result: { outcome: 'clear', baseHead: oid(9) } }); + const head = store.getSnapshot(id).head; + const opening = store.beginPullRequest(id, { checkId: check.id, repository: 'owner/repo', base: 'main', headBranch: BRANCH, headSha: head, draft: false }); + expect(store.recordPullRequestOpened(id, opening.openingId, { number: 7, url: url(7), headSha: head, draft: false }, true)).toBe('in review'); + } finally { store.close(); } + const app = await startServer({ ...config, demo: false, runner: {}, github: { repository: 'owner/repo', issue, baseBranch: 'main', pullRequest: 5 } }, 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[0]).toEqual({ code: 'pull-request', message: 'github.pullRequest is #5, but the task\'s pull request is #7. Remove github.pullRequest from the review configuration.' }); } finally { await app.close(); } }); }); diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index d71d5b74..4e4569f7 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -18,7 +18,7 @@ import { baseBranch } from '../runner/production.ts'; import { MergeCoordinator } from '../runner/merge.ts'; import type { MergeGateway } from '../github/merge.ts'; import { OWED_REFUSAL } from '../runner/publishing.ts'; -import { PullRequestMisplaced, type OpenPullRequestInput, type PullRequestGateway } from '../github/pull-requests.ts'; +import { PullRequestMisplaced, openingMarker, type OpenPullRequestInput, type PullRequestGateway } from '../github/pull-requests.ts'; import type { AlreadyFixedGateway } from '../github/already-fixed.ts'; import type { PlanIdentity } from '../core/identity.ts'; @@ -417,10 +417,17 @@ describe('publishing a finished task (#103)', () => { // Review shows its change in the Unplanned row, and the merge gate blocks until a person assigns or accepts it. const view = app.service.load(); expect(view.segments.filter(segment => segment.row === 'Unplanned').map(segment => segment.path)).toContain('debug.log'); - const gateway: MergeGateway = { inspect: async () => ({ base: view.snapshot.base, head: view.snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', - rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 100, draft: false }), merge: async () => { throw new Error('not merged here'); } }; - const status = await new MergeCoordinator(app.service, gateway).status(view); + // The gate inspects the published PR (#121), which GitHub shows as the task's. + const pr = store.taskPullRequests(identity).find(record => record.state === 'opened')!; + const targets: unknown[] = []; + const gateway: MergeGateway = { inspect: async options => { targets.push(options?.target?.pullRequest); return { base: view.snapshot.base, head: view.snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', + rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: pr.number!, draft: false, + published: { headBranch: pr.headBranch, baseBranch: 'main', crossRepository: false, marker: openingMarker(pr.openingId), branchOpen: [{ number: pr.number!, base: 'main' }] } }; }, + merge: async () => { throw new Error('not merged here'); } }; + const status = await new MergeCoordinator(app.service, gateway, undefined, undefined, { repository: REPO, baseBranch: 'main' }).status(view); + expect(targets).toEqual([pr.number]); expect(status).toMatchObject({ ready: false }); + expect(status.blockers).not.toContainEqual(expect.objectContaining({ code: 'pull-request' })); expect(status.blockers).toContainEqual(expect.objectContaining({ code: 'unplanned' })); });