diff --git a/docs/architecture.md b/docs/architecture.md index 542d7ddb..6cf90248 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -43,8 +43,8 @@ Writing standard: plain language, ISO 24495-1:2023 - **In use:** plan and linking library, SQLite store, review screen, plan display/import/authoring screen, Ask (questions to a Claude agent), guarded merge with merge-queue support, ranked Issues screen, agent isolation boundary, and the single-runner lock. - **In use for a non-demo review with a `github` block:** planning. `/api/plan/suggestions` and `/api/plan/drafts` ask Claude, through lane D, for suggestion cards or a whole next plan revision (#117, #124). Nothing becomes a revision until you apply it. - **In use with an opt-in `runner` block in `review.json`:** the whole loop after planning. `start` and `resume` on `/api/runner` run a task's plan item by item with a Claude agent (#91). They require current plan-item approvals, bind both the task state and review version, recheck approvals before each later item, and refuse when a completed prefix no longer ends at the task's current head (#107). After an out-of-scope pause, an approved amended plan can reconcile the completed prefix and resume only the unfinished suffix (#88, PR #134). When a run ends, codeboost publishes the task's pull request, a draft if the task needs a person (#103). `prepare-merge` refreshes the published PR's exact base/head, rebases through the durable F3/F4 path, recomputes attribution and approvals, pushes a rewritten head whose review is still clear over exactly the PR head it rewrote, and runs allowlisted `cmd:` checks in a credential-free read-only container bound to the resulting head (#22). Approve & merge merges that pull request (#121). Cancelling a task closes its pull requests (#111). These are API actions; the screen has no buttons for them yet. - - **Partly built for pre-merge automation:** the trusted local rebase engine records one-to-one commit mappings, verifies the resulting checkout byte for byte, owns its subprocesses durably, and recovers interrupted work (#22, PR #136). Its production conflict path gives a sandboxed agent only the bounded conflict files and imports only audited results for both foreign and plan-owned commits (PRs #138, #140 and #144). `prepare-merge` now invokes that path after current pull-request base/head reads, refreshes attribution and approvals, pushes the rewritten head behind a durable push marker (F6a), and runs exact-head `cmd:` checks; required-check refresh, the already-fixed rerun, and the one-click guarded merge handoff remain for F6b. - - **Not built:** screens for the runner actions, the required-check refresh and one-click merge handoff (F6b), review rounds, and the Queue and Learning screens. + - **Partly built for pre-merge automation:** the trusted local rebase engine records one-to-one commit mappings, verifies the resulting checkout byte for byte, owns its subprocesses durably, and recovers interrupted work (#22, PR #136). Its production conflict path gives a sandboxed agent only the bounded conflict files and imports only audited results for both foreign and plan-owned commits (PRs #138, #140 and #144). `prepare-merge` now invokes that path after current pull-request base/head reads, refreshes attribution and approvals, pushes the rewritten head behind a durable push marker (F6a), and runs exact-head `cmd:` checks. With a runner, Approve & merge runs one merge job that prepares, waits up to 30 minutes for GitHub's required checks, and hands the exact pair to the guarded merge, whose already-fixed check now covers every base commit after the task's original base (F6b, `docs/implementation/merge-handoff.md`). + - **Not built:** screens for the runner actions, the merge job's full step list and Cancel button, review rounds, and the Queue and Learning screens. ## Terms used @@ -357,7 +357,7 @@ The diagram shows the designed transitions. With the `runner` block, running, ne ### Stored data -All state is in one SQLite file, opened with WAL and full synchronization. Each write takes an immediate write lock. The schema version is `PRAGMA user_version`, and migrations run in explicit steps (version 18 today); an unknown version fails. +All state is in one SQLite file, opened with WAL and full synchronization. Each write takes an immediate write lock. The schema version is `PRAGMA user_version`, and migrations run in explicit steps (version 19 today); an unknown version fails. | Group | Tables | Notes | |---|---|---| @@ -410,7 +410,7 @@ These are the decisions that shape the structure. The full list, with the reason | C, K | Guarded merge, merge queue | Done | | D | Agent isolation boundary | Done. Since the isolation gate: bounded diff export (#76), task-change inspection and the runner's commit as a bounded `git bundle` (#66, PRs #85 and #90), owner-scoped recovery for Ask (#95), the plan schema for Claude (#97), gitlink mounts and the pre-launch tree check (#81, #99, PR #100), and Codex refused in every phase (#93, PR #106). The runner-contract follow-ups (#51) are closed: the last one tests export and removal through a recovery handle after a real restart (PR #109). | | E | Planning library and suggestions | Done. E4 (PR #45) added the acceptance fixtures and recorded Claude output that replays through validation and the Store. | -| F | Runner | Merged: F1 (lifecycle store, coordinator, shutdown wiring, startup recovery and the lock, planning API, feedback events, and stops that wait for their commit: #53, #74, #57, #59, #60, #96); F2a–F2b (execute prompts, audit, per-item execution, real workspace, durable safety findings: #67, #68, #92, #94, #128); F2d (already-fixed check, PR opening, branch pusher, production publishing, closing PRs on cancel: #84, #101, #110, #115, #116, #119, #123); production wiring and `start`/`resume` (#91, PRs #102, #105); planning through lane D (#117, #124, PRs #118, #120, #125); merging the task's own PR (#121, PR #126); start/resume approval, review-version, recovery and completed-prefix guards (#107, PR #130); continuation after a scope pause (#88, PR #134); the F3 durable trusted local-rebase foundation (#22, PR #136); the F4 foreign-conflict engine plus production sandbox adapter (#22, PRs #138 and #140); and sandboxed owned-commit conflict resolution (#22, PR #144). F5 (PR #149) added current base/head refresh, rebase admission, post-rebase attribution and approval refresh, and head-bound `cmd:` execution. This branch adds F6a: the durable push of the rewritten head. F6b owns fresh required-check refresh, the already-fixed rerun, and the one-click exact-pair guarded-merge handoff; F6c owns common CI. | +| F | Runner | Merged: F1 (lifecycle store, coordinator, shutdown wiring, startup recovery and the lock, planning API, feedback events, and stops that wait for their commit: #53, #74, #57, #59, #60, #96); F2a–F2b (execute prompts, audit, per-item execution, real workspace, durable safety findings: #67, #68, #92, #94, #128); F2d (already-fixed check, PR opening, branch pusher, production publishing, closing PRs on cancel: #84, #101, #110, #115, #116, #119, #123); production wiring and `start`/`resume` (#91, PRs #102, #105); planning through lane D (#117, #124, PRs #118, #120, #125); merging the task's own PR (#121, PR #126); start/resume approval, review-version, recovery and completed-prefix guards (#107, PR #130); continuation after a scope pause (#88, PR #134); the F3 durable trusted local-rebase foundation (#22, PR #136); the F4 foreign-conflict engine plus production sandbox adapter (#22, PRs #138 and #140); and sandboxed owned-commit conflict resolution (#22, PR #144). F5 (PR #149) added current base/head refresh, rebase admission, post-rebase attribution and approval refresh, and head-bound `cmd:` execution. F6a (PR #153) added the durable push of the rewritten head. This branch adds F6b: the one-click merge job (preparation, the required-check wait, the exact-pair guarded merge) and an already-fixed scan covering every base commit after the task's original base; F6c owns common CI. | | Hardening | Allowlisted subprocess environments | Done: every Git command (#82, #83) and every `gh` runner (#84, #86). | | Tests | Test suite reliability | Done. The full suite runs ordinary tests first and Docker-backed files serially; CI and the real-Docker gate passed on PR #132 (#129). | | G | Planning screen | Done (#145). The screen displays/imports the current plan, requests and renders a next-revision draft or suggestion cards, and applies results with revision-bound exact ambiguous replay. G4 composes the browser with the production planning setup, D-backed provider boundary, F endpoints and SQLite persistence; focused adapter, recorded-output and exact-head real-Docker planning suites cover the lower boundary. | @@ -419,7 +419,7 @@ These are the decisions that shape the structure. The full list, with the reason ## Known limits -- `prepare-merge` invokes the trusted rebase and sandboxed conflict machinery, refreshes the review, pushes the rewritten head, and runs head-bound `cmd:` checks. Until F6b of #22 lands, it does not wait for fresh required checks, rerun the already-fixed check, or hand the exact pair to guarded merge; a person clicks Merge once GitHub's checks on the pushed head pass. +- With a runner, Approve & merge runs one merge job in the background: preparation, a wait of up to 30 minutes for GitHub's required checks, then the guarded merge. A failing or slow check, or GitHub's refusal, leaves the task approved, merge blocked; the browser shows only the job's current step, not the design's full step list, and has no Cancel button for the job yet (cancel the task instead). - Linking is line-based. It cannot see an unrelated edit inside a declared file; only the review agent and you can catch that. - The container's guarantees are those of Docker. The agent can always reach its own vendor account. - `cmd:` acceptance checks run during `prepare-merge`, and their passing evidence is bound to the exact reviewed head. A plan item without current passing evidence still blocks merging. diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index fc56f535..f2ef50a8 100644 --- a/docs/implementation/guarded-merge.md +++ b/docs/implementation/guarded-merge.md @@ -27,7 +27,7 @@ The target is resolved again for each validation pass. Both passes, and the thir 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, 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. +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. Without a runner block, the commit scan starts at the PR's base commit on its base branch, and no own commits are passed. For a runner-published PR, it starts at the base-branch tip that the pre-PR already-fixed check of the task's first opening into the current base branch scanned through (each opening records its check from schema v19; for an earlier first opening, the task's oldest clear check stands in) when that check scanned from the task's original base, and otherwise at the task's original base, so base commits that a pre-merge rebase brought under the PR are covered too, whichever of the task's openings the merge follows (F6b of #22, `merge-handoff.md`); the ledger's owned commits are passed as the task's own, as in the pre-PR check. Inspections that follow a submitting, queued or merged attempt's pinned pull request scan from the PR's base, as without a runner block; they only show status, since no merge is asked for while an attempt is active. 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. @@ -35,7 +35,7 @@ Shutdown sets a terminal admission flag before inspecting active work and checks ## Deferred runner work -This slice does not mutate Git history or run plan commands. If the base moved, the gate sends the task back to review. A plan item with a `cmd:` acceptance check remains blocked because the current review service has no trusted container result. Issue #22 tracks rebasing through the ledger, recomputing review state, running commands in the container, and persisting results against the exact head. The already-fixed commit scan starts at the PR's current base commit; once a rebase moves that base, the scan must still cover the base-branch commits the rebase brought under the PR since the last check. +This slice does not mutate Git history or run plan commands. If the base moved, the gate sends the task back to review. A plan item with a `cmd:` acceptance check remains blocked because the current review service has no trusted container result. Issue #22 tracks rebasing through the ledger, recomputing review state, running commands in the container, and persisting results against the exact head. F6b closes the already-fixed gap: once a rebase moves the base, the scan still covers every base commit after the task's original base. ## Validation diff --git a/docs/implementation/merge-handoff.md b/docs/implementation/merge-handoff.md new file mode 100644 index 00000000..e18a1493 --- /dev/null +++ b/docs/implementation/merge-handoff.md @@ -0,0 +1,86 @@ +# One-click merge handoff (F6b of #22) + +**Audience:** people who build or review codeboost's runner and merge code. +**Purpose:** say exactly what happens between a click on **Merge PR** and GitHub merging the task's pull request, and what stops it. + +## Summary + +With a runner, **Merge PR** no longer merges at once. It starts one **merge job** and returns. The job: + +1. prepares the merge (refresh, rebase, review check, push, `cmd:` checks), as `prepare-merge` does; +2. waits for GitHub's required checks on the pushed head, for up to 30 minutes; +3. merges through the existing guarded merge, which reruns the already-fixed check and pins the exact head. + +Any change that needs a person (a stale approval, a moved pull request, a failing check) stops the job and says why. A task cancelled while the job runs ends it `cancelled`, however the job learns of it. A failing or slow required check, or a refusal from GitHub, moves the task to **approved, merge blocked**; **Retry merge** starts the whole job again. The browser follows the job by polling. + +Without a runner, **Merge PR** works as before. + +## Terms + +| Term | Meaning | +|---|---| +| Merge job | The background work one click starts. One per task at a time. | +| Preparation | `PreMergeCoordinator.start`: refresh, rebase, review check, push of the rewritten head, `cmd:` checks. | +| Readiness | The record preparation leaves when it succeeds. It binds the task state version, review version, snapshot, base, head and command policy. The guarded merge refuses without it. | +| Required checks | The union of the checks GitHub's rulesets and classic branch protection require. | +| Guarded merge | `MergeCoordinator.merge`: two or more fresh inspections, then `gh pr merge --match-head-commit`. | +| Already-fixed check | The check that the issue is not fixed elsewhere: closed, another PR closes it, or a base-branch commit mentions it. | + +## The job, step by step + +| Step | What happens | It continues when | It stops when | +|---|---|---|---| +| Admission | The click's review token must be current, the task mergeable, the issue trusted (read from GitHub, as for `prepare-merge`), and no merge job, runner attempt, publish or preparation active. The job record is saved under the click's action ID and the click returns the review with the job (`running`, step `preparing`). | always | the token is stale, the issue is not trusted, or something is busy. The refusal is saved under the click's action ID, so a resend replays it; a refusal because the server is shutting down (HTTP 503, also when shutdown stops the click's trust read), or because trust could not be read, saves nothing and may be sent again | +| Prepare | The job records its own `prepare-merge` action and runs preparation. | preparation is `ready` | preparation needs review (job: `review-required`) or fails (job: `failed`) | +| Wait for checks | The job inspects the pull request (fresh) at once, then again after 10 s, with the wait doubling up to 60 s. | every required check succeeds, or the required set was read and is empty, and nothing else blocks | a required check fails, or 30 minutes pass (job: `blocked`, if the preparation still applies: the block re-checks readiness in its transaction, and a review edit or other task change meanwhile ends the job `failed` instead); the pull request moved, another blocker appears, or GitHub cannot be read (job: `failed`; one failed read fails closed rather than retrying) | +| Merge | The job calls the guarded merge with the current review token. Its inspections rerun the already-fixed check. | GitHub merged or queued the head (job: `merged` or `queued`) | GitHub refused (job: `blocked`; `failed` when GitHub refused because the head changed, which needs a fresh review, or when the review, snapshot or plan revision changed while GitHub was asked: the block is for the review the merge attempt was admitted for), validation refused (job: `failed`), or the outcome is unknown (job: `submitted`; the merge attempt reconciles as before) | + +While waiting, an unknown mergeability (GitHub is still computing it) and a pending or missing required check keep the job waiting. No inspection starts after the 30 minutes, and one still running when they end is stopped, so only checks seen passing within the limit count. The guarded merge that follows inspects again and may ask GitHub to merge shortly after the limit; it refuses anything no longer clear. Unreadable branch rules never pass. Another blocker outranks a failing check: checks that fail on a head a collaborator pushed say nothing about this review, so the job ends `failed` and the task's status is unchanged. + +## Outcomes + +| Job state | Task status afterwards | What the person does | +|---|---|---| +| `merged` | merged | nothing | +| `queued` | unchanged (in review, or approved, merge blocked on a retry) while GitHub's queue holds it | nothing | +| `submitted` | unchanged | refresh; the merge attempt settles from GitHub | +| `blocked` | approved, merge blocked | fix the check or GitHub's reason, then **Retry merge**. After GitHub refused a merge, a retry whose preparation rebases makes a new snapshot, and that needs every item approved again (the replacement-snapshot rule of `merge-queue.md`): the items show stale, the retry ends `review-required`, and the merge status asks for every item to be approved again. To change the task's code, use the scope-amendment flow, as from review: it accepts a blocked task. | +| `review-required` | unchanged | review the refreshed pull request, then merge again | +| `failed` | unchanged | read the reason, then merge again | +| `cancelled` | cancelled | nothing | + +Moving the task to **approved, merge blocked** advances its state version, so the next job prepares again from the start. + +## The already-fixed range + +A rebase moves the pull request's base. The guarded merge would then scan only the base commits after the new base and miss the ones the rebase brought in. The scan must cover every base commit after the task's original base (the base of its first snapshot), since a rebase can bring any of them under the pull request. Each opening records the pre-PR check it followed (schema v19). That check of the task's **first opening into the current base branch** scanned the branch from its snapshot's base to the branch's tip then, and found nothing. When it scanned from the task's original base, each merge inspection scans from that tip to the branch's tip now. A later rebase onto the branch can bring in any commit after the original base: those up to that tip were scanned by that check, and later ones are in the merge's scan, so the scan covers them whichever of the task's openings into that branch the merge follows (after a re-publish recorded a newer check from a rebased base, and after a person closed the pull request and a new one was opened from a rebased base). A newer check is not a safe start: it scanned only from a rebased base, and a compare range excludes its own start commit. An opening into another base branch, left by a change of the `github.baseBranch` setting, says nothing about this branch and is not used. A first opening recorded before schema v19 has no check; the task's oldest clear check stands in. When there is no such check, or it scanned from a rebased base (the base branch setting changed after a rebase), the scan starts at the task's original base. If that start is not on the base branch (for example, it was force-pushed), the answer is unknown and merging stays blocked. The check reads at most 250 base commits: if more landed since the scan's start, its answer is unknown and merging stays blocked, as for a pull request never rebased. There is no person override yet (the Continue action of `pull-request-opening.md` is a later slice). The task's own commits, the ledger's owned entries, are excluded, as in the pre-PR check. The status shown while a merge attempt is submitting, queued or merged scans from the pull request's base, as before: those inspections follow the attempt's pinned pull request, and no merge is asked for while an attempt is active. + +## Cancel, shutdown and restart + +| Event | Effect | +|---|---| +| Cancel during preparation | as for `prepare-merge`: the preparation stops, and the task closes when its work has settled | +| Cancel while waiting | the wait stops at once; the task closes; the job ends `cancelled` | +| Cancel during the guarded merge | before GitHub is asked, the merge stops and the job ends `cancelled`; once a merge attempt is submitting, a cancel is refused, as before | +| Shutdown | in shutdown's first, synchronous step, before requests drain, the job is told to stop: a wait ends at once, and nothing after it asks GitHub to merge. A preparation in progress stops only when preparation does, after the drain, as for a `prepare-merge` click; the job then stops without merging. The job is awaited before the store closes. While preparing or waiting, it stays `running` on disk. A guarded merge in progress stops as a click's does: if GitHub was not asked yet, the job stays `running`; if it was, the job ends as the merge attempt records it: `blocked` if GitHub refused (`failed` if the head changed, or the review changed while GitHub was asked), otherwise `submitted`, and the attempt reconciles as before | +| Restart | a `running` job that had not asked GitHub to merge becomes `failed` ("The server restarted before the merge finished"). One that had (its merge action ID is saved before the guarded merge starts) ends as its merge attempt records it: `merged`, `queued`, `submitted`, or `blocked` after a refusal, which also moves the task to approved, merge blocked (`failed`, task unchanged, when the head changed or the review changed while GitHub was asked). A `prepare-merge` action and a push marker are settled as before | + +## Storage + +Schema v19 adds `task_pull_requests.check_id` (the pre-PR check each opening followed) and `merge_jobs`: one row per job or refused click (`action_id`, task key, `state`, `step`, `reason`, required-check names and states, the merge result URL, times). A refused click's row is marked `refused`: it is replayed to that click and never shown as the task's job. Writing it never changes the task's state version, so the readiness preparation recorded stays valid while the job waits. A resend of the same click returns its row; another click while a job runs is refused. + +## API and browser + +- `POST /api/action` with `merge` returns `{ job }` at once when a runner is configured. +- `GET /api/merge` returns `{ queue, job }`: `job` is the task's latest job. A saved refusal is not a job; it is replayed only to its own click. +- Merge PR is offered only while the task is in a mergeable status (read after the inspection) and the only blockers left are ones the job resolves or reports: preparation, a moved base (with a merge conflict it causes), `cmd:` checks, required checks (the job waits for pending ones and ends `blocked` on failing ones, so a person can retry after re-running CI), and mergeability GitHub is still computing. +- While a job runs, the Merge button keeps keyboard focus: it is marked `aria-disabled`, not disabled, and a repeat activation starts nothing. +- The browser polls `GET /api/merge` with the existing 2 s to 30 s backoff while a job runs, shows its step in the merge banner, and lists waiting checks in the blocker dialog. When the job ends, the page updates only the merge control and banner (the outcome, and "Refresh to see the result"), so a person's selection and input survive; after **Refresh**, the page offers **Retry merge** when the job is `blocked`. The design's full step list and a Cancel button come later. + +## Tests + +`test/merge-job.test.ts` runs the job against the real Store and guarded merge with a faked GitHub and preparation. It covers the three wait cases the design names (zero required checks, one check pending then passing, a timeout), a failing check, GitHub still computing mergeability, preparation ending for review or failing, another blocker while waiting, a failed GitHub read while waiting, GitHub's refusal, an unknown merge outcome, a merge queue (`queued`), cancel while waiting (told or not) and during the guarded merge's validation, shutdown while waiting and during the guarded merge, startup settlement, resends, a second click, a stale token, a busy runner, and which blockers keep Merge PR available (including Retry merge after `blocked`). `test/pre-merge.test.ts` runs the #22 acceptance cases through the job with real preparation: a base move that leaves approvals valid merges; a rebase that makes an approval stale stops for review; a breaking command after a rebase fails the job; a collaborator push stops for review; a cancel during a command check ends the job `cancelled`. Server tests cover the merge route, its trust admission (an untrusted click refused and replayed, a failed read saving nothing), `GET /api/merge`, the `prepare-merge` refusal, cancel closing the pull requests, and shutdown; a production test covers startup settlement; a browser test follows a job to merged. + +## Check this document with a reader + +Before merging, ask someone who did not write it to answer, from this page alone: what one click does, when the task becomes approved, merge blocked, and why a job survives a review edit only by failing. diff --git a/github/merge.ts b/github/merge.ts index 95893a34..8260bc3d 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -60,6 +60,13 @@ export interface MergeTarget { ownPullRequests?: readonly number[]; /** The task branch of a published target (#121). The inspection then also reads `RemoteMergeState.published`. */ headBranch?: string; + /** + * Where the already-fixed commit scan starts, so it covers every base commit after the task's original base, which a + * rebase can bring under the PR (F6b of #22; the runner picks it, `merge-handoff.md`). Default: the PR's base commit. + */ + alreadyFixedSince?: string; + /** Commits the task made; the already-fixed scan does not count them. Default: none. */ + ownCommits?: readonly string[]; } export interface MergeResult { url: string; } @@ -100,7 +107,7 @@ export interface GhMergeConfig { baseBranch?: string; } -type ResolvedTarget = { pullRequest: number; ownPullRequests: readonly number[]; headBranch?: string }; +type ResolvedTarget = { pullRequest: number; ownPullRequests: readonly number[]; headBranch?: string; alreadyFixedSince?: string; ownCommits?: readonly string[] }; type BranchRules = Pick; export type RunGh = (args: readonly string[], options?: { signal?: AbortSignal }) => Promise; @@ -172,7 +179,11 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { 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 } : {}) }; + if (target?.alreadyFixedSince !== undefined && (typeof target.alreadyFixedSince !== 'string' || !SHA.test(target.alreadyFixedSince))) throw new Error('Invalid already-fixed scan start.'); + if (target?.ownCommits !== undefined && (!Array.isArray(target.ownCommits) || target.ownCommits.some(sha => typeof sha !== 'string' || !SHA.test(sha)))) throw new Error('Invalid own commit.'); + return { pullRequest, ownPullRequests: [...new Set([pullRequest, ...own])], ...(target?.headBranch !== undefined ? { headBranch: target.headBranch } : {}), + ...(target?.alreadyFixedSince !== undefined ? { alreadyFixedSince: target.alreadyFixedSince } : {}), + ...(target?.ownCommits !== undefined ? { ownCommits: [...new Set(target.ownCommits)].sort() } : {}) }; } async #json(args: readonly string[], signal?: AbortSignal): Promise { @@ -191,20 +202,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 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 task's other recorded PRs are its own: excluded while open, a match once merged. The base-branch scan starts at + * `target.alreadyFixedSince` when given (a runner-published PR), else at the PR's base commit; `target.ownCommits`, the + * task's own commits a rebase may have rewritten, are not counted. * 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(ownPullRequests: readonly number[], baseBranch: string, base: string, deadlineAt: number, signal?: AbortSignal): Promise> { + async #alreadyFixed(target: ResolvedTarget, 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: [...ownPullRequests], ownCommits: new Set() }, + const result = await this.checks.check({ issue: this.config.issue, taskBase: target.alreadyFixedSince ?? base, baseBranch, + ownPullRequests: [...target.ownPullRequests], ownCommits: new Set(target.ownCommits ?? []) }, 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.) @@ -234,7 +246,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { // 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 alreadyFixed = this.#alreadyFixed(target, 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, diff --git a/runner/merge-job.ts b/runner/merge-job.ts new file mode 100644 index 00000000..00b794f0 --- /dev/null +++ b/runner/merge-job.ts @@ -0,0 +1,281 @@ +import { randomUUID } from 'node:crypto'; +import { GuardRefusal, MERGEABLE_STATUSES, ShuttingDownError, settleWith, type ShutdownCapability } from './lifecycle.ts'; +import { MergeOutcomeUnknown, type MergeCoordinator, type MergeStatus } from './merge.ts'; +import type { PreMergeCoordinator } from './pre-merge.ts'; +import type { ReviewService } from './review.ts'; +import type { MergeJob, MergeJobState } from './store.ts'; + +/** How long a job waits for GitHub's required checks on the pushed head (docs/implementation/merge-handoff.md). */ +export const REQUIRED_CHECK_WAIT_MS = 30 * 60_000; +/** The wait between required-check inspections starts here and doubles up to the maximum. */ +export const REQUIRED_CHECK_FIRST_DELAY_MS = 10_000; +export const REQUIRED_CHECK_MAX_DELAY_MS = 60_000; + +/** A job's answer to the browser: everything but the click's review token. */ +export type MergeJobView = Omit; +export const mergeJobView = ({ token: _token, ...job }: MergeJob): MergeJobView => job; + +export interface MergeJobTiming { checkWaitMs?: number; firstDelayMs?: number; maxDelayMs?: number } + +/** Why the wait for required checks ended. */ +type CheckVerdict = { kind: 'ready' } | { kind: 'wait'; checks: { context: string; state: string }[] } + | { kind: 'blocked'; reason: string } | { kind: 'failed'; reason: string } | { kind: 'cancelled' }; + +/** + * The one-click merge (F6b of #22): preparation, the wait for GitHub's required checks on the pushed head, then the + * guarded merge. One job per task at a time; its progress is saved in `merge_jobs`, never on the task, so the + * preparation's readiness record stays bound while the job waits. + */ +export class MergeJobCoordinator { + readonly service: ReviewService; + readonly preMerge: PreMergeCoordinator; + readonly merges: MergeCoordinator; + readonly checkWaitMs: number; readonly firstDelayMs: number; readonly maxDelayMs: number; + readonly settle: (fn: () => T) => T; + #active: Promise | null = null; + #abort: AbortController | null = null; + #closing = false; + + constructor(service: ReviewService, preMerge: PreMergeCoordinator, merges: MergeCoordinator, capability?: ShutdownCapability, + timing: MergeJobTiming = {}) { + this.service = service; this.preMerge = preMerge; this.merges = merges; + this.checkWaitMs = timing.checkWaitMs ?? REQUIRED_CHECK_WAIT_MS; + this.firstDelayMs = timing.firstDelayMs ?? REQUIRED_CHECK_FIRST_DELAY_MS; + this.maxDelayMs = timing.maxDelayMs ?? REQUIRED_CHECK_MAX_DELAY_MS; + for (const value of [this.checkWaitMs, this.firstDelayMs, this.maxDelayMs]) + if (!Number.isSafeInteger(value) || value < 0) throw new Error('Invalid merge job timing.'); + this.settle = settleWith(capability); + } + + get active(): boolean { return this.#active !== null; } + /** The running job's settlement, for the server's after-work hooks; null when none runs. */ + get current(): Promise | null { return this.#active; } + /** Whether this click already has a saved job or refusal, which a resend replays without new admission reads. */ + saved(actionId: string): boolean { return this.service.store.getMergeJob(this.service.config.identity, actionId) !== null; } + latest(): MergeJobView | null { + const job = this.service.store.latestMergeJob(this.service.config.identity); + return job ? mergeJobView(job) : null; + } + + /** + * Admit the click and start its job after the caller's transaction commits. A resend of the same click returns its + * job without starting another. A definite refusal is saved under the click's action ID and replayed on resend; only a + * refusal during shutdown saves nothing, so that click may be sent again. + */ + start(token: unknown, actionId: string, busy: () => string | null = () => null): MergeJobView { + const { store, config } = this.service, identity = config.identity; + const saved = store.getMergeJob(identity, actionId); + if (saved) { + const { job } = store.beginMergeJob(identity, { actionId, token: token as string }); + // A refused click replays its refusal; anything else is the job this click started. + if (job.refused) throw new GuardRefusal(job.reason ?? 'The merge was refused.'); + return mergeJobView(job); + } + // Shutdown decides nothing about this click: nothing is saved, and the route answers 503 so the same click is resent. + if (this.#closing) throw new ShuttingDownError(); + const refusal = this.#admissionRefusal(token, busy); + if (refusal !== null) { + // A definite refusal is this click's outcome; a resend replays it (as the guarded merge does). + if (typeof token === 'string' && token) store.recordMergeJobRefusal(identity, { actionId, token, reason: refusal }); + throw new GuardRefusal(refusal); + } + const { job } = store.beginMergeJob(identity, { actionId, token: token as string }); + const abort = new AbortController(); this.#abort = abort; + const run = Promise.resolve().then(() => this.#run(job, abort.signal)).catch(error => { + // Anything unexpected ends the job, unless shutdown stopped it: startup then settles it. + if (!abort.signal.aborted || !this.#closing) this.#finish(job.id, 'failed', error instanceof Error ? error.message : String(error)); + }).finally(() => { if (this.#active === run) { this.#active = null; this.#abort = null; } }); + this.#active = run; + return mergeJobView(job); + } + + /** Why this click may not start a job now, or null. A preparation stopping for shutdown throws instead (not definite). */ + #admissionRefusal(token: unknown, busy: () => string | null): string | null { + const { store, config } = this.service, identity = config.identity; + if (this.#active) return 'A merge is already running for this task.'; + if (typeof token !== 'string' || this.service.load().token !== token) return 'Stale review state. Refresh before merging.'; + const task = store.getTask(identity); + if (!MERGEABLE_STATUSES.includes(task.status)) return `The task is ${task.status}; merge it from review.`; + if (task.cancelRequested !== null) return 'The task is being cancelled.'; + const reason = busy(); + if (reason) return reason; + try { this.preMerge.assertStartable(); } + catch (error) { + if (error instanceof GuardRefusal && /shutting down/i.test(error.message)) throw new ShuttingDownError(); + if (!(error instanceof GuardRefusal)) throw error; + return error.message; + } + return null; + } + + /** The task was cancelled: stop waiting at once. Preparation and the guarded merge stop through their own routes. */ + cancel(): void { this.#abort?.abort(Object.assign(new Error('Task cancelled.'), { code: 'CANCELLED' })); } + + /** Shutdown's first, synchronous step: no job starts, and a running one stops before it can ask GitHub to merge. */ + stop(): void { + this.#closing = true; + this.#abort?.abort(new Error('Server shutdown.')); + } + async close(): Promise { + this.stop(); + await this.#active; + } + + #finish(id: string, state: Exclude, reason: string | null = null, url: string | null = null): void { + try { this.settle(() => this.service.store.finishMergeJob(this.service.config.identity, id, { state, reason, url })); } + catch { /* Startup settles a job that could not be ended now. */ } + } + + async #run(job: MergeJob, signal: AbortSignal): Promise { + const { store, config } = this.service, identity = config.identity; + // 1. Prepare: refresh, rebase, review check, push, cmd: checks. + let view = this.service.load(); + const prepareId = randomUUID(), task = store.getTask(identity); + store.userAction(identity, { actionId: prepareId, kind: 'prepare-merge', + request: { expectedStateVersion: task.stateVersion, expectedReviewVersion: view.expected.reviewVersion, mergeJob: job.id } }, + () => ({ outcome: 'preparing' })); + const prepared = await this.preMerge.start({ stateVersion: task.stateVersion, reviewVersion: view.expected.reviewVersion!, + snapshotId: view.snapshot.id, base: view.snapshot.base, head: view.snapshot.head, actionId: prepareId }); + if (signal.aborted || this.#cancelled()) return this.#end(job, signal); + if (prepared.state !== 'ready') + return this.#finish(job.id, prepared.state === 'review-required' ? 'review-required' : 'failed', prepared.reason); + // 2. Wait for GitHub's required checks on the pushed head. + this.settle(() => store.updateMergeJob(identity, job.id, { step: 'checks' })); + const verdict = await this.#awaitChecks(job, signal); + if (verdict.kind === 'cancelled') return this.#end(job, signal); + // A failing or slow check blocks only while the readiness it was read for still holds (in the block's transaction). + if (verdict.kind === 'blocked') return this.#block(job, verdict.reason, true); + if (verdict.kind === 'failed') return this.#finish(job.id, 'failed', verdict.reason); + // 3. Merge through the guarded merge, which re-validates everything and reruns the already-fixed check. + // The merge's action ID is saved first: after a restart, the attempt carrying it gives the job's outcome. + const mergeId = randomUUID(); + this.settle(() => store.updateMergeJob(identity, job.id, { step: 'merging', checks: [], mergeActionId: mergeId })); + view = this.service.load(); + try { + // The job's signal stops the guarded merge as shutdown or a deadline stops a click: before GitHub is asked, nothing + // is applied; after, the outcome is unknown. + const merged = await this.merges.merge(view.token, mergeId, signal); + const attempt = store.getMergeAttempt(identity); + this.#finish(job.id, attempt?.kind === 'queue' && attempt.state !== 'merged' ? 'queued' : 'merged', null, merged.result.url || null); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + // Once this job's merge attempt exists, GitHub was asked: its recorded state is the answer, whatever else (a cancel, + // shutdown) happened meanwhile. A refusal blocks (design: "GitHub refused the merge"); anything else is unknown. + const attempt = store.getMergeAttempt(identity); + if (attempt?.actionId === mergeId) { + // A refusal because the head changed needs a fresh review, not a retry: the task is not "approved" any more. + if ((attempt.state === 'failed' || attempt.state === 'removed') && attempt.requiresFreshReview) + return this.#finish(job.id, 'failed', `GitHub refused the merge: ${attempt.reason ?? reason} Review the new head, then merge again.`); + if (attempt.state === 'failed' || attempt.state === 'removed') return this.#block(job, `GitHub refused the merge: ${attempt.reason ?? reason}`); + if (attempt.state === 'merged') return this.#finish(job.id, 'merged', null, attempt.url); + if (attempt.state === 'queued') return this.#finish(job.id, 'queued', null, attempt.url); + return this.#finish(job.id, 'submitted', reason); + } + if (error instanceof MergeOutcomeUnknown) return this.#finish(job.id, 'submitted', reason); + // A cancel or shutdown, whichever way the merge reported it (stopped, or refused because the task was cancelled). + if (signal.aborted || this.#cancelled()) return this.#end(job, signal); + this.#finish(job.id, 'failed', reason); + } + } + + #cancelled(): boolean { + const task = this.service.store.getTask(this.service.config.identity); + return task.status === 'cancelled' || task.cancelRequested !== null; + } + /** A cancelled task ends the job, however the job learned of it; shutdown leaves it running on disk for startup. */ + #end(job: MergeJob, signal: AbortSignal): void { + if ((signal.reason as { code?: string } | undefined)?.code === 'CANCELLED' || this.#cancelled()) + this.#finish(job.id, 'cancelled', 'Task cancelled.'); + } + + /** Approved, merge blocked: the task's next merge prepares again from the start. One Store transaction. */ + #block(job: MergeJob, reason: string, whileReady = false): void { + const readyFor = whileReady ? { commandPolicyDigest: this.service.commandPolicyDigest() } : undefined; + try { this.settle(() => this.service.store.blockMergeJob(this.service.config.identity, job.id, reason, readyFor)); } + catch { /* Startup settles a job that could not be ended now. */ } + } + + async #awaitChecks(job: MergeJob, signal: AbortSignal): Promise { + const { store, config } = this.service, identity = config.identity; + const until = performance.now() + this.checkWaitMs; + let delay = this.firstDelayMs, waiting: { context: string; state: string }[] = []; + const timedOut = (): CheckVerdict => ({ kind: 'blocked', reason: `Required checks did not pass within ${Math.round(this.checkWaitMs / 60_000)} minutes: ${ + waiting.length ? waiting.map(check => `${check.context} is ${check.state}`).join(', ') : 'GitHub has not determined mergeability'}.` }); + for (;;) { + if (signal.aborted) return { kind: 'cancelled' }; + const task = store.getTask(identity); + if (task.status === 'cancelled' || task.cancelRequested !== null) return { kind: 'cancelled' }; + // No inspection starts after the wait's limit, and each one is stopped when the limit arrives. + const left = Math.floor(until - performance.now()); + if (left < 1) return timedOut(); + const limit = AbortSignal.timeout(left); + let status: MergeStatus; + try { status = await this.merges.status(this.service.load(), true, AbortSignal.any([signal, limit])); } + catch (error) { + if (signal.aborted) return { kind: 'cancelled' }; + if (limit.aborted) return timedOut(); + return { kind: 'failed', reason: error instanceof Error ? error.message : String(error) }; + } + const verdict = classify(status); + if (verdict.kind !== 'wait') return verdict; + waiting = verdict.checks; + this.settle(() => store.updateMergeJob(identity, job.id, { checks: waiting })); + const remaining = until - performance.now(); + if (remaining <= 0) return timedOut(); + await sleep(Math.min(delay, remaining), signal).catch(() => undefined); + delay = Math.min(delay * 2, this.maxDelayMs); + } + } + +} + +/** Blockers the merge job resolves itself: it prepares (rebase, push, cmd: checks) and waits for required checks. */ +const JOB_RESOLVES = new Set(['preparation', 'base', 'acceptance', 'check']); + +/** + * The merge status the browser sees when merging runs as a job: ready when only blockers the job resolves remain (or + * GitHub is still computing mergeability), and never while a job or merge attempt is in flight. After a blocked job the + * action is a retry. The latest job rides along. + */ +export function withMergeJob(status: T, job: MergeJobView | null, taskStatus: string): T & { job: MergeJobView | null } { + const running = job?.state === 'running'; + const attempt = status.queue?.state === 'submitting' || status.queue?.state === 'queued' || status.queue?.state === 'merged'; + const computing = status.remote?.mergeable === 'UNKNOWN'; + // A conflict after the base moved is the job's too: its preparation rebases, through the sandboxed conflict path. + const rebaseResolves = status.remote?.mergeable === 'CONFLICTING' && status.blockers.some(blocker => blocker.code === 'base'); + const personal = status.blockers.filter(blocker => !JOB_RESOLVES.has(blocker.code) && !(blocker.code === 'mergeable' && (computing || rebaseResolves))); + // The task's status is read after the inspection that produced `status`: a task cancelled or merged meanwhile is never + // offered Merge PR, even if that inspection had no task blocker. + const ready = !running && !attempt && status.remote !== null && personal.length === 0 + && MERGEABLE_STATUSES.includes(taskStatus as (typeof MERGEABLE_STATUSES)[number]); + return { ...status, ready, action: ready ? (taskStatus === 'approved but merge blocked' || status.action === 'retry' ? 'retry' : 'merge') : null, job }; +} + +/** + * One fresh inspection's answer for the wait: a failing required check blocks; pending or missing ones, and GitHub + * still computing mergeability, wait; any other blocker (the PR moved, readiness lost, the already-fixed check, rules) + * ends the job for a person to act. + */ +export function classify(status: MergeStatus): CheckVerdict { + // Another blocker comes first: checks failing on a head nobody reviewed (a collaborator pushed) say nothing about this + // review, so the job ends for a person to look, rather than blocking on those checks. + const computing = status.remote.mergeable === 'UNKNOWN'; + const other = status.blockers.filter(blocker => blocker.code !== 'check' && !(blocker.code === 'mergeable' && computing)); + if (other.length) return { kind: 'failed', reason: other[0]!.message }; + const failing = status.remote.requiredChecks.filter(check => check.state === 'failure'); + if (failing.length) return { kind: 'blocked', reason: `Required check${failing.length === 1 ? '' : 's'} failed: ${failing.map(check => check.context).join(', ')}.` }; + const waiting = status.remote.requiredChecks.filter(check => check.state !== 'success') + .map(check => ({ context: check.context, state: check.state })); + if (waiting.length || computing) return { kind: 'wait', checks: waiting }; + return status.ready ? { kind: 'ready' } : { kind: 'failed', reason: status.blockers[0]?.message ?? 'Merge is blocked.' }; +} + +function sleep(ms: number, signal: AbortSignal): Promise { + return new Promise((resolve, reject) => { + if (signal.aborted) { reject(signal.reason); return; } + const timer = setTimeout(() => { signal.removeEventListener('abort', stop); resolve(); }, ms); + const stop = () => { clearTimeout(timer); reject(signal.reason); }; + signal.addEventListener('abort', stop, { once: true }); + }); +} diff --git a/runner/merge.ts b/runner/merge.ts index e1eae8d6..0d96b02e 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -1,5 +1,5 @@ import type { ReviewService } from './review.ts'; -import { mergeActionResponse, type MergeAttempt } from './store.ts'; +import { mergeActionResponse, type MergeAttempt, type TaskPullRequest } from './store.ts'; import { ActionIdReused, GuardRefusal, MERGEABLE_STATUSES, ShuttingDownError, assertUuidV4, settleWith, type ShutdownCapability } from './lifecycle.ts'; import { MERGE_INSPECTION_TIMEOUT_MS, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type MergeResult, type MergeTarget, type RemoteMergeState } from '../github/merge.ts'; import { openingMarker } from '../github/pull-requests.ts'; @@ -112,8 +112,30 @@ export class MergeCoordinator { 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 }; + return { target: { pullRequest: latest.number, ownPullRequests, headBranch: latest.headBranch, ...this.#alreadyFixedRange(here, published.baseBranch) }, + openingId: latest.openingId, marker: openingMarker(latest.openingId), headBranch: latest.headBranch }; + } + + /** + * The already-fixed scan of a runner-published PR (F6b of #22). It must cover every base commit after the task's + * original base (its first snapshot's), since a rebase can bring any of them under the PR. The pre-PR check of the + * task's first opening into the current base branch scanned that branch from its snapshot's base to its tip and found + * nothing; when that base is the task's original base, the scan starts at that tip: commits a later rebase onto the + * branch brings in up to that tip were scanned by that check, and later ones are in this scan, whichever of the + * task's openings the merge follows. An opening into another base branch says nothing about this one. A first + * opening recorded before schema v19 has no check: the task's oldest clear check stands in. Otherwise (no such check, or one that scanned from a rebased base), the scan starts at the + * task's original base. The task's own commits are excluded. + */ + #alreadyFixedRange(here: TaskPullRequest[], base: string): { alreadyFixedSince: string; ownCommits: string[] } { + const { store, config } = this.service, identity = config.identity; + const original = store.firstSnapshot(identity).base; + const first = here.find(pr => pr.base === base); + const check = first?.checkId ? store.getAlreadyFixed(identity, first.checkId) : store.oldestClearAlreadyFixed(identity); + const ownCommits = store.getLedger(identity).filter(entry => entry.origin === 'owned').map(entry => entry.sha); + if (check?.result.outcome !== 'clear') return { alreadyFixedSince: original, ownCommits }; + let scannedFrom: string | null = null; + try { scannedFrom = store.getSnapshot(identity, check.snapshotId).base; } catch { /* unknown: scan from the original base */ } + return { alreadyFixedSince: scannedFrom === original ? check.result.baseHead : original, ownCommits }; } /** Whether GitHub shows the task's published PR where it was opened, as the only open PR from its branch. */ @@ -257,7 +279,18 @@ export class MergeCoordinator { ? 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.' }; + if (attempt && !this.#freshReviewComplete(attempt)) { + // A new snapshot or plan revision after an attempt (a rebase, for example) needs every item approved again for it; + // the attempt's own reason (often GitHub's refusal) would not say so. + const { store, config } = this.service; + const after = attempt.reason ? ` (${attempt.reason})` : ''; + const message = store.getSnapshot(config.identity).id !== attempt.snapshotId + ? `The pull request changed after the last merge attempt${after}. Approve every item again for the replacement snapshot, then merge.` + : store.getPlan(config.identity).revision !== attempt.revision + ? `The plan changed after the last merge attempt${after}. Approve every item again for the new plan revision, then merge.` + : attempt.reason ?? 'The pull request snapshot changed after the queue attempt. Review the replacement snapshot.'; + return { code: 'queue-head', message }; + } return null; } @@ -279,7 +312,7 @@ export class MergeCoordinator { * `actionId` is the click's idempotency key. A resend with the same key (after a lost response) returns the saved * attempt's current outcome and never submits again. */ - async merge(token: unknown, actionId?: unknown): Promise<{ status: MergeStatus | MergeUnavailableStatus; result: MergeResult }> { + async merge(token: unknown, actionId?: unknown, caller?: AbortSignal): Promise<{ status: MergeStatus | MergeUnavailableStatus; result: MergeResult }> { if (actionId !== undefined) assertUuidV4(actionId, 'Action ID'); // A resend of the click still in progress joins it, so it gets the final result rather than a saved interim state. if (this.#active && typeof actionId === 'string' && this.#activeClick?.actionId === actionId) { @@ -299,9 +332,13 @@ export class MergeCoordinator { if (typeof token !== 'string') throw new Error('Stale review state. Refresh before merging.'); const abort = new AbortController(); const timer = setTimeout(() => abort.abort(new Error('Merge request deadline exceeded.')), this.operationTimeoutMs); + // A caller that owns the merge (the one-click merge job) stops it as shutdown or a deadline would. + const stop = () => abort.abort(caller!.reason); + if (caller?.aborted) stop(); else caller?.addEventListener('abort', stop, { once: true }); this.#abort = abort; const attempt = this.#merge(token, abort.signal, actionId as string | undefined).finally(() => { clearTimeout(timer); + caller?.removeEventListener('abort', stop); if (this.#active === attempt) { this.#active = null; this.#activeClick = null; } if (this.#abort === abort) this.#abort = null; }); diff --git a/runner/production.ts b/runner/production.ts index 2d307e60..cf015259 100644 --- a/runner/production.ts +++ b/runner/production.ts @@ -264,6 +264,7 @@ export async function setUpRunner(o: { service: ReviewService; capability: Shutd deps: o.recovery ? o.recovery(image) : dRecoveryDeps(image, { abort: async (attemptId, resultHead, state) => (await getRebaser()).abort(attemptId, resultHead, state) }, rebasePlanKey) }); service.store.settleInterruptedPreMergeActions(identity); + service.store.settleInterruptedMergeJobs(identity); const repository = await getRepository(); const imageId = image(); const workspace = createTaskWorkspace({ store: service.store, runnerRoot: config.root, runnerOwner, repository, imageId, diff --git a/runner/store.ts b/runner/store.ts index 632801ef..8b501f7f 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -40,6 +40,8 @@ export interface TaskPullRequest { ownerReviewVersion: number; /** An update of this open PR that started and has not been confirmed; the PR may already show it. */ refresh: { head: string; draft: boolean; stateVersion: number } | null; + /** The clear already-fixed check this opening followed; null for an opening recorded before schema v19. */ + checkId: string | null; } export interface AlreadyFixedCheck { id: string; snapshotId: string; result: AlreadyFixedResult; stateVersion: number; reviewVersion: number; checkedAt: string } export type MergeAttemptState = 'submitting' | 'queued' | 'merged' | 'removed' | 'failed'; @@ -129,6 +131,27 @@ export interface PushMarker { } /** What a read of the branch showed after a push: `to`, still `from`, or anything else. */ export type PushOutcome = 'pushed' | 'not-pushed' | 'moved'; +/** A one-click merge job (F6b of #22; docs/implementation/merge-handoff.md). */ +export type MergeJobState = 'running' | 'merged' | 'queued' | 'submitted' | 'blocked' | 'review-required' | 'failed' | 'cancelled'; +export type MergeJobStep = 'preparing' | 'checks' | 'merging'; +export interface MergeJob { + /** The click's action ID. */ + id: string; + /** The action ID of the guarded merge this job started, once it reaches that step: its attempt carries it. */ + mergeActionId: string | null; + /** The click was refused at admission; a resend replays the refusal (`reason`). */ + refused?: boolean; + /** The review token the click was made with; a resend must carry the same one. */ + token: string; + state: MergeJobState; + step: MergeJobStep | null; + reason: string | null; + /** Required checks that have not succeeded yet, as the last inspection read them. */ + checks: { context: string; state: string }[]; + url: string | null; + startedAt: string; + updatedAt: string; +} export interface AttemptRecord { id: string; kind: AttemptKind; phase: string; item: string | null; state: AttemptState; context: InvocationContext; deadline: number; firstReason: FirstReason | null; stopReason: StopReason | null; exitCode: number | null; signal: string | null; result: unknown; @@ -206,8 +229,8 @@ export class Store { this.#db.exec('PRAGMA foreign_keys=ON; PRAGMA journal_mode=WAL; PRAGMA synchronous=FULL;'); this.#transaction(() => { const version = this.#get('PRAGMA user_version')!.user_version as number; - if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18].includes(version)) throw new Error('Unsupported store schema version.'); - if (version === 18) return; + if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19].includes(version)) throw new Error('Unsupported store schema version.'); + if (version === 19) return; if (version === 0) this.#db.exec(` CREATE TABLE plans (key TEXT PRIMARY KEY, issue INTEGER NOT NULL, revision INTEGER NOT NULL, snapshot_id TEXT); CREATE TABLE revisions (key TEXT NOT NULL REFERENCES plans(key), revision INTEGER NOT NULL, data TEXT NOT NULL, PRIMARY KEY(key,revision)); @@ -329,6 +352,16 @@ export class Store { this.#db.exec('ALTER TABLE tasks ADD COLUMN push_in_progress TEXT'); this.#db.exec('PRAGMA user_version=18'); } + // One-click merge jobs (F6b of #22). Kept apart from tasks, so a job's progress never changes the task state version. + if (version < 19) { + this.#db.exec(`CREATE TABLE IF NOT EXISTS merge_jobs ( + id TEXT PRIMARY KEY, plan_key TEXT NOT NULL REFERENCES plans(key), data TEXT NOT NULL); + CREATE INDEX IF NOT EXISTS merge_jobs_plan ON merge_jobs(plan_key);`); + // The pre-PR already-fixed check each opening followed: the merge's scan starts at that check's base. + if (!this.#db.prepare('PRAGMA table_info(task_pull_requests)').all().some(column => column.name === 'check_id')) + this.#db.exec('ALTER TABLE task_pull_requests ADD COLUMN check_id TEXT'); + this.#db.exec('PRAGMA user_version=19'); + } }); } catch (error) { this.#db.close(); throw error; } } @@ -473,6 +506,12 @@ export class Store { this.#run("UPDATE requests SET state='invalidated',reason='Repository snapshot changed.' WHERE key=? AND state IN ('pending','ready')", key); return snapshot; } + /** The task's first snapshot: its base is where the task began, before any rebase. */ + firstSnapshot(identity: PlanIdentity): Snapshot { + const row = this.#get('SELECT data FROM snapshots WHERE key=? ORDER BY rowid ASC LIMIT 1', identityKey(identity)); + if (!row) throw new Error('Unknown snapshot.'); + return decode(row.data); + } getSnapshot(identity: PlanIdentity, id?: string): Snapshot { const key = identityKey(identity); const row = this.#get('SELECT data FROM snapshots WHERE key=? AND id=?', key, id ?? this.#current(key).snapshot_id!); @@ -2080,6 +2119,161 @@ export class Store { const saved = decode<{ ok?: unknown; value?: { outcome?: unknown }; preMergeReadiness?: PreMergeReadiness }>(row.response); return saved.ok === true && saved.value?.outcome === 'ready' && stable(saved.preMergeReadiness) === stable(readiness); } + /** + * Admit a merge job for this click. A resend of the same click returns its job; another click while a job runs, or a + * reused action ID with another token, is refused. Never touches the task. + */ + beginMergeJob(identity: PlanIdentity, input: { actionId: string; token: string; now?: string }): { job: MergeJob; resent: boolean } { + assertUuidV4(input.actionId, 'Action ID'); + if (typeof input.token !== 'string' || !input.token || input.token.length > 512) throw new Error('A review token is required.'); + const key = identityKey(identity); + return this.#transaction(() => { + const task = this.#task(key); + const existing = this.#get('SELECT data FROM merge_jobs WHERE id=?', input.actionId); + if (existing) { + const job = decode(existing.data); + if (job.token !== input.token || this.#get('SELECT plan_key FROM merge_jobs WHERE id=?', input.actionId)!.plan_key !== key) + throw new ActionIdReused('Action ID already used for a different request.'); + return { job, resent: true }; + } + if (this.#closed(task.status as TaskStatus)) throw new GuardRefusal(`The task is ${task.status}; it cannot be merged.`); + if (this.runningMergeJob(identity)) throw new GuardRefusal('A merge is already running for this task.'); + const now = input.now ?? new Date().toISOString(); + const job: MergeJob = { id: input.actionId, mergeActionId: null, token: input.token, state: 'running', step: 'preparing', reason: null, checks: [], + url: null, startedAt: now, updatedAt: now }; + this.#run('INSERT INTO merge_jobs VALUES (?,?,?)', job.id, key, encode(job)); + return { job, resent: false }; + }); + } + /** + * Save a definite admission refusal under the click's action ID, so a resend replays it rather than starting a job. + * A resend of the same click returns the saved record; another request under that ID is refused as a reused ID. + */ + recordMergeJobRefusal(identity: PlanIdentity, input: { actionId: string; token: string; reason: string }): MergeJob { + assertUuidV4(input.actionId, 'Action ID'); + if (typeof input.token !== 'string' || !input.token || input.token.length > 512) throw new Error('A review token is required.'); + const key = identityKey(identity); + return this.#transaction(() => { + this.#task(key); + const existing = this.#get('SELECT plan_key, data FROM merge_jobs WHERE id=?', input.actionId); + if (existing) { + const job = decode(existing.data); + if (job.token !== input.token || existing.plan_key !== key) throw new ActionIdReused('Action ID already used for a different request.'); + return job; + } + const now = new Date().toISOString(); + const job: MergeJob = { id: input.actionId, mergeActionId: null, token: input.token, state: 'failed', step: null, + reason: input.reason.slice(0, 2_000), checks: [], url: null, startedAt: now, updatedAt: now, refused: true }; + this.#run('INSERT INTO merge_jobs VALUES (?,?,?)', job.id, key, encode(job)); + return job; + }); + } + /** Move a running job to another step or record what it waits for. A job no longer running is left unchanged. */ + updateMergeJob(identity: PlanIdentity, id: string, patch: { step?: MergeJobStep; reason?: string | null; checks?: { context: string; state: string }[]; + mergeActionId?: string }): MergeJob | null { + return this.#changeMergeJob(identity, id, job => job.state === 'running' ? { ...job, ...patch } : null); + } + /** + * End a running job `blocked` and, in the same transaction, move a task still in review to approved, merge blocked, + * so the job and the task cannot disagree after a crash. A task that moved meanwhile keeps its status. + * With `readyFor`, the block applies only while the preparation's readiness still holds (checked in the same + * transaction): a check result that arrived after a review edit or another task change says nothing about the + * changed review, so the job then ends `failed` and the task keeps its status. + */ + blockMergeJob(identity: PlanIdentity, id: string, reason: string, readyFor?: { commandPolicyDigest: string }): boolean { + const key = identityKey(identity); + return this.#transaction(() => { + const job = this.getMergeJob(identity, id); + if (!job) throw new Error('Unknown merge job.'); + // A job that already ended decides nothing about the task now. + if (job.state !== 'running') return false; + const task = this.#task(key); + if (readyFor) { + const snapshot = this.getSnapshot(identity); + if (!this.preMergeReady(identity, { stateVersion: task.state_version as number, reviewVersion: this.#current(key).review_version as number, + snapshotId: snapshot.id, base: snapshot.base, head: snapshot.head, commandPolicyDigest: readyFor.commandPolicyDigest })) + return this.finishMergeJob(identity, id, { state: 'failed', + reason: 'The review or task changed while waiting for required checks; the preparation no longer applies. Merge again.' }); + } + // GitHub refused the job's own merge attempt: the block applies only to the review that attempt was admitted for. + // A review edit, new snapshot or plan revision while GitHub was asked leaves the changed task as it is. + const attempt = job.mergeActionId ? this.getMergeAttempt(identity) : null; + if (attempt && attempt.actionId === job.mergeActionId) { + const plan = this.#current(key); + if (plan.revision !== attempt.revision || plan.snapshot_id !== attempt.snapshotId || plan.review_version !== attempt.reviewVersion) + return this.finishMergeJob(identity, id, { state: 'failed', + reason: `${reason} The review changed while GitHub was asked to merge; review it, then merge again.` }); + } + if (task.status === 'in review' && !this.#activeAttempt(key) && !this.#activeMerge(key) + && task.rebase_in_progress === null && (task.push_in_progress === null || task.push_in_progress === undefined)) { + this.#run('UPDATE tasks SET status=? WHERE plan_key=?', 'approved but merge blocked', key); + this.#touch(key); + } + return this.finishMergeJob(identity, id, { state: 'blocked', reason }); + }); + } + /** End a running job. Returns false when it had already ended. */ + finishMergeJob(identity: PlanIdentity, id: string, outcome: { state: Exclude; reason?: string | null; url?: string | null }): boolean { + return this.#changeMergeJob(identity, id, job => job.state === 'running' + ? { ...job, state: outcome.state, step: null, reason: outcome.reason ?? null, url: outcome.url ?? job.url } : null) !== null; + } + #changeMergeJob(identity: PlanIdentity, id: string, change: (job: MergeJob) => MergeJob | null): MergeJob | null { + const key = identityKey(identity); + return this.#transaction(() => { + const row = this.#get('SELECT data FROM merge_jobs WHERE id=? AND plan_key=?', id, key); + if (!row) throw new Error('Unknown merge job.'); + const next = change(decode(row.data)); + if (!next) return null; + const job = { ...next, updatedAt: new Date().toISOString() }; + this.#run('UPDATE merge_jobs SET data=? WHERE id=?', encode(job), id); + return job; + }); + } + getMergeJob(identity: PlanIdentity, id: string): MergeJob | null { + const row = this.#get('SELECT data FROM merge_jobs WHERE id=? AND plan_key=?', id, identityKey(identity)); + return row ? decode(row.data) : null; + } + /** The task's latest job. A refused click started none: its refusal is replayed to that click alone, never shown here. */ + latestMergeJob(identity: PlanIdentity): MergeJob | null { + const row = this.#get(`SELECT data FROM merge_jobs WHERE plan_key=? AND json_extract(data,'$.refused') IS NOT 1 + ORDER BY rowid DESC LIMIT 1`, identityKey(identity)); + return row ? decode(row.data) : null; + } + runningMergeJob(identity: PlanIdentity): MergeJob | null { + const row = this.#get(`SELECT data FROM merge_jobs WHERE plan_key=? AND json_extract(data,'$.state')='running' ORDER BY rowid DESC LIMIT 1`, + identityKey(identity)); + return row ? decode(row.data) : null; + } + /** + * At startup: a job the server stopped running cannot continue. If it had asked GitHub to merge, its merge attempt's + * recorded state is its outcome (a refusal blocks the task, as the job would have); otherwise it fails, and the person + * merges again. + */ + settleInterruptedMergeJobs(identity: PlanIdentity): void { + const running = this.runningMergeJob(identity); + if (!running) return; + const attempt = running.mergeActionId ? this.getMergeAttempt(identity) : null; + if (!attempt || attempt.actionId !== running.mergeActionId) { + this.finishMergeJob(identity, running.id, { state: 'failed', reason: 'The server restarted before the merge finished. Merge again.' }); + return; + } + if (attempt.state === 'merged' || attempt.state === 'queued') { + this.finishMergeJob(identity, running.id, { state: attempt.state, url: attempt.url }); + return; + } + if (attempt.state === 'submitting') { + this.finishMergeJob(identity, running.id, { state: 'submitted', + reason: 'The server restarted while GitHub\'s merge outcome was unknown; it reconciles from GitHub.' }); + return; + } + if (attempt.requiresFreshReview) { + // The head changed: the task needs a fresh review, so it is not moved to approved, merge blocked. + this.finishMergeJob(identity, running.id, { state: 'failed', + reason: `GitHub refused the merge: ${attempt.reason ?? 'no reason given.'} Review the new head, then merge again.` }); + return; + } + this.blockMergeJob(identity, running.id, `GitHub refused the merge: ${attempt.reason ?? 'no reason given.'}`); + } /** Startup recovery makes an interrupted preparation definite and replayable before new work is admitted. */ settleInterruptedPreMergeActions(identity: PlanIdentity): void { const snapshot = this.getSnapshot(identity); @@ -2149,6 +2343,7 @@ export class Store { number: row.number as number | null, url: row.url as string | null, createdAt: row.created_at as string, ownerVersion: row.owner_version as number, ownerReviewVersion: row.owner_review_version as number, refresh: row.refresh_head === null ? null : { head: row.refresh_head as string, draft: row.refresh_draft === 1, stateVersion: row.refresh_version as number }, + checkId: (row.check_id as string | null | undefined) ?? null, }; } /** Every PR codeboost opened or started to open for the task, oldest first. */ @@ -2156,6 +2351,19 @@ export class Store { const key = identityKey(identity); this.#task(key); return this.#db.prepare('SELECT * FROM task_pull_requests WHERE plan_key=? ORDER BY rowid').all(key).map(row => this.#pullRequestRecord(row)); } + /** + * The task's first already-fixed check that found nothing: the scan start for an opening recorded before schema v19, + * which did not record its own check. A compare range excludes its start commit, so only a scan from this check's tip + * onward is provably continuous with what was scanned before. + */ + oldestClearAlreadyFixed(identity: PlanIdentity): AlreadyFixedCheck | null { + const row = this.#get(`SELECT * FROM already_fixed_checks WHERE plan_key=? AND outcome='clear' ORDER BY rowid ASC LIMIT 1`, identityKey(identity)); + return row ? { id: row.id as string, snapshotId: row.snapshot_id as string, result: decode(row.result), stateVersion: row.state_version as number, reviewVersion: row.review_version as number, checkedAt: row.checked_at as string } : null; + } + getAlreadyFixed(identity: PlanIdentity, id: string): AlreadyFixedCheck | null { + const row = this.#get('SELECT * FROM already_fixed_checks WHERE plan_key=? AND id=?', identityKey(identity), id); + return row ? { id: row.id as string, snapshotId: row.snapshot_id as string, result: decode(row.result), stateVersion: row.state_version as number, reviewVersion: row.review_version as number, checkedAt: row.checked_at as string } : null; + } latestAlreadyFixed(identity: PlanIdentity): AlreadyFixedCheck | null { const key = identityKey(identity); this.#task(key); const row = this.#get('SELECT * FROM already_fixed_checks WHERE plan_key=? ORDER BY rowid DESC LIMIT 1', key); @@ -2221,9 +2429,9 @@ export class Store { if (this.#get("SELECT 1 FROM task_pull_requests WHERE plan_key=? AND state='opening'", key)) throw new GuardRefusal('A pull request is already being opened; recover it first.'); const openingId = randomUUID(), now = new Date().toISOString(); this.#touch(key); - this.#run(`INSERT INTO task_pull_requests (opening_id,plan_key,repository,base,head_branch,head_sha,draft,owner_version,owner_review_version,state,number,url,created_at,updated_at) - VALUES (?,?,?,?,?,?,?,?,?,'opening',NULL,NULL,?,?)`, openingId, key, input.repository, input.base, input.headBranch, input.headSha, input.draft ? 1 : 0, - this.#task(key).state_version as number, this.#current(key).review_version as number, now, now); + this.#run(`INSERT INTO task_pull_requests (opening_id,plan_key,repository,base,head_branch,head_sha,draft,owner_version,owner_review_version,state,number,url,created_at,updated_at,check_id) + VALUES (?,?,?,?,?,?,?,?,?,'opening',NULL,NULL,?,?,?)`, openingId, key, input.repository, input.base, input.headBranch, input.headSha, input.draft ? 1 : 0, + this.#task(key).state_version as number, this.#current(key).review_version as number, now, now, input.checkId); return this.taskPullRequests(identity).find(pr => pr.openingId === openingId)!; }); } diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index c028ced0..b6306a14 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -629,3 +629,169 @@ test('hides Retry until a timed-out invocation has actually settled',async({page settle('Late answer'); await expect(page.getByRole('button',{name:'Retry answer',exact:true})).toBeVisible({timeout:10000}); }); +/** Whether the element is drawn with the muted surface background a control that cannot act uses (poll it: buttons may animate their background). */ +const looksInactive=(locator:import('@playwright/test').Locator)=>locator.evaluate(el=>{const probe=document.createElement('div');probe.style.background='var(--surface)';document.body.append(probe); + const surface=getComputedStyle(probe).backgroundColor;probe.remove();return getComputedStyle(el).backgroundColor===surface;}); +test('names a failed merge-job poll as a merge-status failure (F6b)',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',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 running={id:'0f6b0000-0000-4000-8000-000000000001',state:'running',step:'checks',reason:null,checks:[],url:null,startedAt:'2026-10-11T00:00:00Z',updatedAt:'2026-10-11T00:00:00Z'}; + await page.route('**/api/action',async route=>{const body=route.request().postDataJSON();if(body.action!=='merge'){await route.continue();return;} + const response=await route.fetch({url:route.request().url().replace('/api/action','/api/review'),method:'GET'}),review=await response.json(); + await route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({...review,merge:{...review.merge,ready:false,action:null,job:running},job:running})});}); + // A job is running and no merge queue exists; GitHub's status cannot be read. + await page.route('**/api/merge',route=>route.fulfill({status:502,contentType:'application/json',body:JSON.stringify({error:'GitHub returned HTTP 502.'})})); + await page.route('**/api/review',async route=>{const response=await route.fetch(),body=await response.json();body.merge={...body.merge,job:null};await route.fulfill({response,json:body});}); + 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')).toHaveText(/^Could not refresh merge status\. GitHub returned HTTP 502\./,{timeout:15_000}); +}); + +test('keeps a draft typed while a merge-job click is in flight (F6b)',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',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 job=(id:string,state:string,step:string|null)=>({id,state,step,reason:state==='blocked'?'Required check failed: ci.':null,checks:[],url:null,startedAt:'2026-10-11T00:00:00Z',updatedAt:'2026-10-11T00:00:00Z'}); + let release!:()=>void;const held=new Promise(resolve=>{release=resolve;}); + await page.route('**/api/action',async route=>{const body=route.request().postDataJSON();if(body.action!=='merge'){await route.continue();return;} + await held;const response=await route.fetch({url:route.request().url().replace('/api/action','/api/review'),method:'GET'}),review=await response.json(),running=job(body.actionId,'running','preparing'); + await route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({...review,merge:{...review.merge,ready:false,action:null,job:running},job:running})});}); + await page.route('**/api/merge',route=>route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({queue:null,job:job('x','running','preparing')})})); + await page.route('**/api/review',async route=>{const response=await route.fetch(),body=await response.json();body.merge={...body.merge,job:null};await route.fulfill({response,json:body});}); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + await page.getByRole('button',{name:/P2 Document retry behavior/}).click(); + await page.getByLabel('Question about this item').fill('Before the click.'); + await page.getByRole('button',{name:'Merge PR',exact:true}).click(); + // The person keeps typing while the click's request is in flight. + await page.getByLabel('Question about this item').fill('Typed while the merge request was in flight.'); + release(); + await expect(page.getByRole('button',{name:'Preparing…',exact:true})).toHaveAttribute('aria-disabled','true'); + await expect(page.getByLabel('Question about this item')).toHaveValue('Typed while the merge request was in flight.'); +}); + +test('starts a new action for Retry merge once the job its retained key started has ended (F6b)',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',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 job=(id:string,state:string,step:string|null)=>({id,state,step,reason:state==='blocked'?'Required check failed: ci.':null,checks:[],url:null,startedAt:'2026-10-11T00:00:00Z',updatedAt:'2026-10-11T00:00:00Z'}); + const clicks:string[]=[];let first:string|null=null; + await page.route('**/api/action',async route=>{const body=route.request().postDataJSON();if(body.action!=='merge'){await route.continue();return;}clicks.push(body.actionId); + if(clicks.length===1){first=body.actionId;await route.fulfill({status:503,contentType:'application/json',body:JSON.stringify({error:'The review server is shutting down.'})});return;} + const response=await route.fetch({url:route.request().url().replace('/api/action','/api/review'),method:'GET'}),review=await response.json(),running=job(body.actionId,'running','preparing'); + await route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({...review,merge:{...review.merge,ready:false,action:null,job:running},job:running})});}); + await page.route('**/api/merge',route=>route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({queue:null,job:job('x','running','preparing')})})); + // The first click's job was saved and has since ended blocked: the review offers Retry merge. + await page.route('**/api/review',async route=>{const response=await route.fetch(),body=await response.json(); + body.merge={...body.merge,job:first?job(first,'blocked',null):null,...(first?{ready:true,action:'retry'}:{})};await route.fulfill({response,json:body});}); + 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.'); + const reloaded=page.waitForResponse(response=>response.url().endsWith('/api/review')); + await page.locator('#review-link').click();await reloaded; + await page.getByRole('button',{name:'Retry merge',exact:true}).click(); + await expect.poll(()=>clicks.length).toBe(2); + // The retry is a new action: resending the first key would only replay the ended job. + expect(clicks[1]).not.toBe(clicks[0]); +}); + +test('starts a new action once another job became the latest after a retained key (F6b)',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',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 job=(id:string,state:string,step:string|null)=>({id,state,step,reason:state==='blocked'?'Required check failed: ci.':null,checks:[],url:null,startedAt:'2026-10-11T00:00:00Z',updatedAt:'2026-10-11T00:00:00Z'}); + const clicks:string[]=[];let first:string|null=null; + await page.route('**/api/action',async route=>{const body=route.request().postDataJSON();if(body.action!=='merge'){await route.continue();return;}clicks.push(body.actionId); + if(clicks.length===1){first=body.actionId;await route.fulfill({status:503,contentType:'application/json',body:JSON.stringify({error:'The review server is shutting down.'})});return;} + const response=await route.fetch({url:route.request().url().replace('/api/action','/api/review'),method:'GET'}),review=await response.json(),running=job(body.actionId,'running','preparing'); + await route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({...review,merge:{...review.merge,ready:false,action:null,job:running},job:running})});}); + await page.route('**/api/merge',route=>route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({queue:null,job:job('x','running','preparing')})})); + // The first click's job ended, then another client's job ran and ended blocked: the latest job is not this key's. + await page.route('**/api/review',async route=>{const response=await route.fetch(),body=await response.json(); + body.merge={...body.merge,job:first?job('0f6b0000-0000-4000-8000-0000000000b2','blocked',null):null,...(first?{ready:true,action:'retry'}:{})};await route.fulfill({response,json:body});}); + 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.'); + const reloaded=page.waitForResponse(response=>response.url().endsWith('/api/review')); + await page.locator('#review-link').click();await reloaded; + await page.getByRole('button',{name:'Retry merge',exact:true}).click(); + await expect.poll(()=>clicks.length).toBe(2); + // The retry is a new action: resending the first key would only replay the ended job. + expect(clicks[1]).not.toBe(clicks[0]); +}); + +test('keeps focus on the Merge button when a merge-job click is refused (F6b)',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',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}); + let mergeClicks=0; + await page.route('**/api/action',async route=>{const body=route.request().postDataJSON();if(body.action!=='merge'){await route.continue();return;}mergeClicks++; + await route.fulfill({status:409,contentType:'application/json',body:JSON.stringify({error:'Issue #1 is not trusted for its current author.'})});}); + // A runner server reports merging as a job (its merge status carries `job`). + await page.route('**/api/review',async route=>{const response=await route.fetch(),body=await response.json();body.merge={...body.merge,job:null};await route.fulfill({response,json:body});}); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + const button=page.locator('#merge'); + await button.focus();await button.press('Enter'); + await expect(page.locator('#banner')).toHaveText(/^Merge blocked\. Issue #1 is not trusted/); + // The refusal leaves the control marked, not disabled, and focused (AGENTS.md, Async review UI). + await expect(button).toHaveAttribute('aria-disabled','true'); + await expect(button).toHaveJSProperty('disabled',false);await expect(button).toBeFocused(); + // It is drawn as a control that cannot act, as a disabled one is. + await expect.poll(()=>looksInactive(button)).toBe(true); + // Activating it again starts nothing: the merge is not ready. + await button.press('Enter'); + expect(mergeClicks).toBe(1); +}); +test('follows a one-click merge job from preparation through the required-check wait to merged (F6b)',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',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 job=(state:string,step:string|null,checks:Array<{context:string;state:string}>=[],reason:string|null=null)=>({id:'0f6b0000-0000-4000-8000-000000000000',state,step,reason,checks,url:null,startedAt:'2026-10-11T00:00:00Z',updatedAt:'2026-10-11T00:00:00Z'}); + const steps=[job('running','checks',[{context:'ci',state:'pending'}]),job('running','merging'),job('merged',null)]; + let polls=0,reviews=0,mergeClicks=0; + await page.route('**/api/action',async route=>{const body=route.request().postDataJSON();if(body.action!=='merge'){await route.continue();return;}mergeClicks++; + const response=await route.fetch({url:route.request().url().replace('/api/action','/api/review'),method:'GET'}),review=await response.json(),running=job('running','preparing'); + await route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({...review,merge:{...review.merge,ready:false,action:null,job:running},job:running})});}); + await page.route('**/api/merge',async route=>{const next=steps[Math.min(polls,steps.length-1)];polls++;await route.fulfill({status:200,contentType:'application/json',body:JSON.stringify({queue:null,job:next})});}); + // A runner server reports merging as a job (its merge status carries `job`). + await page.route('**/api/review',async route=>{reviews++;const response=await route.fetch(),body=await response.json();body.merge={...body.merge,job:null};await route.fulfill({response,json:body});}); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + const button=page.locator('#merge'); + await page.getByRole('button',{name:'Merge PR',exact:true}).click(); + // The focused control keeps focus while its job runs: marked, not disabled (AGENTS.md, Async review UI). + await expect(page.getByRole('button',{name:'Preparing…',exact:true})).toHaveAttribute('aria-disabled','true'); + // Not disabled in the DOM (Playwright's toBeEnabled counts aria-disabled as disabled), and still focused. + await expect(button).toHaveJSProperty('disabled',false);await expect(button).toBeFocused(); + await expect.poll(()=>looksInactive(button)).toBe(true); + await expect(page.locator('#banner')).toContainText('Preparing the merge'); + await expect(page.getByRole('button',{name:'Waiting for checks…',exact:true})).toHaveAttribute('aria-disabled','true'); + await expect(button).toBeFocused(); + await expect(page.locator('#banner')).toContainText('Waiting for required checks: ci is pending.'); + // A repeat activation while the job runs starts nothing. + await button.press('Enter'); + await page.getByRole('button',{name:'Merge status',exact:true}).click(); + await expect(page.locator('dialog')).toContainText('ci: pending'); + await page.keyboard.press('Escape'); + await expect(page.getByRole('button',{name:'Merging…',exact:true})).toHaveAttribute('aria-disabled','true'); + expect(mergeClicks).toBe(1); + const reloadsBeforeEnd=reviews; + // The job's end updates only the merge control (AGENTS.md, Async review UI): no background reload erases the + // person's selection or input. + await expect(page.locator('#banner')).toHaveText('Merged. Refresh to see the result.'); + expect(reviews).toBe(reloadsBeforeEnd); + await expect(button).toHaveAttribute('aria-disabled','true');await expect(button).toHaveJSProperty('disabled',false); + expect(polls).toBe(3); + await page.locator('#merge-details').click(); + await expect(page.locator('dialog')).toContainText('Merged. Refresh to see the result.'); + await page.keyboard.press('Escape'); + // Refresh loads the outcome. + const reloaded=page.waitForResponse(response=>response.url().endsWith('/api/review')); + await page.locator('#review-link').click();await reloaded; + expect(reviews).toBeGreaterThan(reloadsBeforeEnd); +}); diff --git a/test/merge-job.test.ts b/test/merge-job.test.ts new file mode 100644 index 00000000..60e8d9a8 --- /dev/null +++ b/test/merge-job.test.ts @@ -0,0 +1,564 @@ +import { randomUUID } from 'node:crypto'; +import { afterEach, expect, it, vi } from 'vitest'; +import { Store } from '../runner/store.ts'; +import { GuardRefusal, ShuttingDownError } from '../runner/lifecycle.ts'; +import { MergeCoordinator, type PublishedTarget } from '../runner/merge.ts'; +import { MergeJobCoordinator, withMergeJob, type MergeJobView } from '../runner/merge-job.ts'; +import type { PreMergeCoordinator, PreMergeResult } from '../runner/pre-merge.ts'; +import type { ReviewService } from '../runner/review.ts'; +import { MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type RemoteMergeState, type RequiredCheck } from '../github/merge.ts'; +import { openingMarker } from '../github/pull-requests.ts'; +import type { Plan, PlanContext } from '../core/plan.ts'; +import { commandDigest } from '../runner/checks.ts'; + +// F6b of #22: the one-click merge job (docs/implementation/merge-handoff.md). +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', requiresPreparation: true }; +const TIMING = { checkWaitMs: 2_000, firstDelayMs: 5, maxDelayMs: 20 }; +const stores: Store[] = [], jobs: MergeJobCoordinator[] = []; +afterEach(async () => { for (const job of jobs.splice(0)) await job.close(); for (const store of stores.splice(0)) store.close(); }); + +/** A task in review whose runner published PR 7 with head oid(2) over oid(1). */ +function publishedTask(): 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 }); + const check = 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 opening = store.beginPullRequest(identity, { checkId: check.id, repository: 'owner/repo', base: 'main', headBranch: BRANCH, headSha: oid(2), draft: false }); + expect(store.recordPullRequestOpened(identity, opening.openingId, { number: 7, url: 'https://github.com/owner/repo/pull/7', headSha: oid(2), draft: false }, true)).toBe('in review'); + return store; +} +function service(store: Store) { + const view = () => { + const snapshot = store.getSnapshot(identity); + return { + 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: snapshot.base, head: snapshot.head }, token: 'review-token', + expected: { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, + }; + }; + return { store, config: { identity }, load: vi.fn(view), commandPolicyDigest: () => commandDigest([]) } as unknown as ReviewService; +} +/** Preparation as F5/F6a end it: `ready` records exact readiness on the job's own prepare-merge action. */ +function preparation(store: Store, result: (call: number) => PreMergeResult['state'] | Error = () => 'ready') { + const calls: string[] = []; + const fake = { + assertStartable: () => undefined, + start: vi.fn(async (expected: { actionId: string }) => { + calls.push(expected.actionId); + const outcome = result(calls.length), snapshot = store.getSnapshot(identity); + if (outcome instanceof Error) throw outcome; + const readiness = { stateVersion: store.getTask(identity).stateVersion, reviewVersion: store.reviewVersion(identity), + snapshotId: snapshot.id, base: snapshot.base, head: snapshot.head, commandPolicyDigest: commandDigest([]) }; + const settled = { state: outcome, base: snapshot.base, head: snapshot.head, checked: [], reason: outcome === 'ready' ? null : `preparation ${outcome}` }; + return store.settlePreMergeAction(identity, expected.actionId, settled, outcome === 'ready' ? readiness : null); + }), + }; + return { fake: fake as unknown as PreMergeCoordinator, calls }; +} +/** GitHub showing PR 7 as published; `checks(n)` gives the required checks of the n-th inspection. */ +function github(store: Store, options: { checks?: (n: number) => RequiredCheck[]; change?: (n: number) => Partial; + merge?: (signal?: AbortSignal) => Promise<{ url: string }>; queue?: boolean; unreadable?: (n: number) => boolean; + during?: (n: number) => void } = {}) { + let inspections = 0; + const merged: string[] = []; + const client: MergeGateway = { + inspect: vi.fn(async request => { + inspections++; + if (options.unreadable?.(inspections)) throw new Error('GitHub returned HTTP 502'); + // Something that happens locally while GitHub's answer is on its way. + options.during?.(inspections); + const record = store.taskPullRequests(identity).find(pr => pr.number === 7)!; + const snapshot = store.getSnapshot(identity); + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, + mergeQueue: options.queue === true, requiredChecks: options.checks?.(inspections) ?? [], alreadyFixed: 'clear', pullRequest: 7, draft: false, + ...(request?.target?.headBranch ? { published: { headBranch: request.target.headBranch, baseBranch: 'main', crossRepository: false, + marker: openingMarker(record.openingId), branchOpen: [{ number: 7, base: 'main' }] } } : {}), + ...options.change?.(inspections) } as RemoteMergeState; + }), + merge: vi.fn(async (head, request) => { merged.push(head); return options.merge ? options.merge(request?.signal) : { url: 'https://github.com/owner/repo/pull/7' }; }), + ...(options.queue ? { + queueWatermark: vi.fn(async () => 'CURSOR'), + inspectQueue: vi.fn(async (head: string) => ({ state: 'queued' as const, reviewedHead: head, entryId: 'E', phase: 'QUEUED' as const, position: 0, + enqueuedAt: '2026-10-11T00:00:00Z', queueHead: head })), + } satisfies MergeQueueGateway : {}), + }; + return { client, merged, inspections: () => inspections }; +} +function setup(options: Parameters[1] & { prepare?: Parameters[1] } = {}) { + const store = publishedTask(), review = service(store), gh = github(store, options); + const merges = new MergeCoordinator(review, gh.client, undefined, undefined, PUBLISHED); + const prep = preparation(store, options.prepare); + const coordinator = new MergeJobCoordinator(review, prep.fake, merges, undefined, TIMING); jobs.push(coordinator); + return { store, review, gh, merges, prep, coordinator }; +} +const settled = async (coordinator: MergeJobCoordinator) => { await coordinator.current; return coordinator.latest()!; }; +const pending = (context = 'ci'): RequiredCheck => ({ context, appId: null, state: 'pending' }); +const passing = (context = 'ci'): RequiredCheck => ({ context, appId: null, state: 'success' }); + +it('merges at once when the required set was read and is empty', async () => { + const f = setup(); + const started = f.coordinator.start('review-token', randomUUID()); + expect(started).toMatchObject({ state: 'running', step: 'preparing' }); + expect(started).not.toHaveProperty('token'); + expect(await settled(f.coordinator)).toMatchObject({ state: 'merged', step: null, url: 'https://github.com/owner/repo/pull/7' }); + expect(f.gh.merged).toEqual([oid(2)]); + expect(f.prep.calls).toHaveLength(1); + expect(f.store.getTask(identity).status).toBe('merged'); +}); + +it('waits while a required check is pending, then merges once it passes', async () => { + const seen: string[] = []; + const f = setup({ checks: n => (n <= 2 ? [pending()] : [passing()]) }); + const job = f.coordinator.start('review-token', randomUUID()); + const poll = setInterval(() => { const latest = f.coordinator.latest(); if (latest?.checks.length) seen.push(`${latest.step}:${latest.checks[0]!.state}`); }, 1); + const done = await settled(f.coordinator); clearInterval(poll); + expect(done).toMatchObject({ id: job.id, state: 'merged' }); + expect(seen).toContain('checks:pending'); + expect(f.gh.merged).toEqual([oid(2)]); +}); + +it('ends approved, merge blocked when required checks do not pass in time', async () => { + const f = setup({ checks: () => [pending('build'), { context: 'deploy', appId: null, state: 'missing' }] }); + f.coordinator.start('review-token', randomUUID()); + const done = await settled(f.coordinator); + expect(done.state).toBe('blocked'); + expect(done.reason).toMatch(/did not pass within .*build is pending, deploy is missing/); + expect(f.gh.merged).toEqual([]); + expect(f.store.getTask(identity).status).toBe('approved but merge blocked'); +}); + +it('stops at once when a required check fails, without waiting for the time limit', async () => { + const f = setup({ checks: () => [{ context: 'build', appId: null, state: 'failure' }] }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'blocked', reason: 'Required check failed: build.' }); + expect(f.gh.inspections()).toBe(1); + expect(f.store.getTask(identity).status).toBe('approved but merge blocked'); +}); + +it('ends failed, without merging, when GitHub cannot be read while waiting for checks', async () => { + const f = setup({ checks: () => [pending()], unreadable: n => n === 2 }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/HTTP 502/) }); + expect(f.gh.inspections()).toBe(2); + expect(f.gh.merged).toEqual([]); + expect(f.store.getTask(identity).status).toBe('in review'); +}); + +it('ends failed, leaving the task in review, when GitHub refuses because the head changed', async () => { + const f = setup({ merge: async () => { throw new MergeSubmissionError('Head branch was modified. Review and try the merge again.', 'refused'); } }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/^GitHub refused the merge: Head branch was modified.*Review the new head/) }); + expect(f.store.getMergeAttempt(identity)).toMatchObject({ state: 'failed', requiresFreshReview: true }); + expect(f.store.getTask(identity).status).toBe('in review'); +}); + +it('ends failed, leaving the task in review, when the review changed while GitHub was asked to merge and refused', async () => { + let storeRef: Store | null = null; + const f = setup({ merge: async () => { + // A person adds a change request while the merge command is on its way; then GitHub refuses. + const store = storeRef!, snapshot = store.getSnapshot(identity); + store.addReviewNote(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, 'P1', 'change', 'Changed during the merge.'); + throw new MergeSubmissionError('Required status check "ci" is expected.', 'refused'); + } }); + storeRef = f.store; + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/review changed while GitHub was asked to merge/) }); + expect(f.store.getMergeAttempt(identity)).toMatchObject({ state: 'failed' }); + expect(f.store.getTask(identity).status).toBe('in review'); +}); + +it('ends failed, leaving the task in review, when a failing check arrives after the review changed', async () => { + let storeRef: Store | null = null; + const f = setup({ checks: () => [{ context: 'build', appId: null, state: 'failure' }], during: () => { + // A person adds a change request while the job's inspection is reading GitHub. + const store = storeRef!, snapshot = store.getSnapshot(identity); + store.addReviewNote(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, 'P1', 'change', 'Changed during the wait.'); + } }); + storeRef = f.store; + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/changed while waiting for required checks/) }); + expect(f.store.getTask(identity).status).toBe('in review'); + expect(f.gh.merged).toEqual([]); +}); + +it('waits while GitHub is still computing mergeability', async () => { + const f = setup({ change: n => (n <= 2 ? { mergeable: 'UNKNOWN' } : {}) }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'merged' }); +}); + +it('ends for review or as failed when preparation does, and merges nothing', async () => { + for (const [outcome, state] of [['review-required', 'review-required'], ['failed', 'failed']] as const) { + const f = setup({ prepare: () => outcome }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state, reason: `preparation ${outcome}` }); + expect(f.gh.merged).toEqual([]); + expect(f.store.getTask(identity).status).toBe('in review'); + } +}); + +it('ends as failed, for a person, when another blocker appears while waiting', async () => { + const f = setup({ change: () => ({ base: oid(30) }) }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/base branch moved/) }); + expect(f.gh.merged).toEqual([]); +}); + +it('ends approved, merge blocked when GitHub refuses the merge', async () => { + const f = setup({ merge: async () => { throw new MergeSubmissionError('Required status check "ci" is expected.', 'refused'); } }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'blocked', reason: expect.stringMatching(/^GitHub refused the merge: .*expected/) }); + expect(f.store.getTask(identity).status).toBe('approved but merge blocked'); +}); + +it('ends submitted when GitHub\'s merge outcome is unknown; the attempt stays in flight', async () => { + const f = setup({ merge: async () => { throw new MergeSubmissionError('connection reset', 'unknown'); } }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'submitted' }); + expect(f.store.getMergeAttempt(identity)?.state).toBe('submitting'); +}); + +it('stops waiting at once when the task is cancelled', async () => { + const f = setup({ checks: () => [pending()] }); + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.coordinator.latest()?.step).toBe('checks')); + // As the cancel route does: the task's cancel commits first, then the job is told. + expect(f.store.cancelTask(identity, f.store.getTask(identity).stateVersion, randomUUID())).toBe('closed'); + f.coordinator.cancel(); + expect(await settled(f.coordinator)).toMatchObject({ state: 'cancelled' }); + expect(f.gh.merged).toEqual([]); +}); + +it('keeps the readiness preparation recorded valid while the job records its progress', async () => { + let readyAt: number | null = null; + const f = setup({ checks: n => { if (n === 1) readyAt = f.store.getTask(identity).stateVersion; return n <= 3 ? [pending()] : [passing()]; } }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'merged' }); + // The guarded merge admitted the readiness bound at preparation: the job's own writes never advanced the task. + expect(f.store.getMergeAttempt(identity)).toMatchObject({ state: 'merged' }); + expect(readyAt).not.toBeNull(); +}); + +it('replays a resent click, refuses a second click while a job runs, and replays a refusal for a stale token', async () => { + const f = setup({ checks: () => [pending()] }); + const id = randomUUID(), stale = randomUUID(); + expect(() => f.coordinator.start('stale-token', stale)).toThrow(/Stale review state/); + expect(f.store.getMergeJob(identity, stale)).toMatchObject({ state: 'failed', refused: true }); + expect(() => f.coordinator.start('stale-token', stale)).toThrow(/Stale review state/); + const first = f.coordinator.start('review-token', id); + expect(f.coordinator.start('review-token', id)).toMatchObject({ id: first.id, state: 'running' }); + await vi.waitFor(() => expect(f.coordinator.latest()?.step).toBe('checks')); + const second = randomUUID(); + expect(() => f.coordinator.start('review-token', second)).toThrow(/already running/); + expect(f.store.getMergeJob(identity, second)).toMatchObject({ refused: true }); + // The saved refusal is that click's alone: the running job stays the task's job, and Merge PR stays withheld. + expect(f.coordinator.latest()).toMatchObject({ id: first.id, state: 'running', step: 'checks' }); + const resolvable = { ready: false, action: null, blockers: [{ code: 'check', message: 'pending' }], remote: { mergeable: 'MERGEABLE' }, queue: null } as Parameters[0]; + expect(withMergeJob(resolvable, f.coordinator.latest(), 'in review')).toMatchObject({ ready: false, action: null }); + expect(() => f.coordinator.start('other-token', id)).toThrow(/already used/); + expect(f.store.cancelTask(identity, f.store.getTask(identity).stateVersion, randomUUID())).toBe('closed'); + f.coordinator.cancel(); await settled(f.coordinator); + expect(f.prep.calls).toHaveLength(1); +}); + +it('resolves close only once the job has settled, so shutdown can await its last write', async () => { + const f = setup(); + // Preparation that finishes its own work after the shutdown abort (as a push or command check settling does). + let release!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const prepared = f.prep.fake.start.bind(f.prep.fake); + (f.prep.fake as unknown as { start: typeof prepared }).start = async expected => { await held; return prepared(expected); }; + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.coordinator.latest()).toMatchObject({ state: 'running', step: 'preparing' })); + let closed = false; + const closing = f.coordinator.close().then(() => { closed = true; }); + await new Promise(resolve => setTimeout(resolve, 50)); + expect(closed).toBe(false); + release(); await closing; + expect(f.coordinator.active).toBe(false); + expect(f.gh.merged).toEqual([]); +}); + +it('leaves a job running on shutdown, and startup ends it as failed', async () => { + const f = setup({ checks: () => [pending()] }); + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.coordinator.latest()?.step).toBe('checks')); + await f.coordinator.close(); + expect(f.coordinator.latest()).toMatchObject({ state: 'running' }); + f.store.settleInterruptedMergeJobs(identity); + expect(f.coordinator.latest()).toMatchObject({ state: 'failed', reason: expect.stringMatching(/server restarted/) }); + // Not a definite refusal: the route answers 503 and the browser keeps the click's key (nothing was saved). + const id = randomUUID(); + expect(() => f.coordinator.start('review-token', id)).toThrow(ShuttingDownError); + expect(f.store.getMergeJob(identity, id)).toBeNull(); +}); + +it('ends queued when the merge queue accepted the head', async () => { + const f = setup({ queue: true }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'queued' }); + expect(f.store.getMergeAttempt(identity)).toMatchObject({ kind: 'queue', state: 'queued' }); + expect(f.store.getTask(identity).status).toBe('in review'); +}); + +it('leaves the task approved, merge blocked when a retried job is queued', async () => { + const f = setup({ queue: true, checks: n => (n === 1 ? [{ context: 'build', appId: null, state: 'failure' }] : []) }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'blocked' }); + expect(f.store.getTask(identity).status).toBe('approved but merge blocked'); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'queued' }); + expect(f.store.getTask(identity).status).toBe('approved but merge blocked'); +}); + +it('refuses a click while the runner or publisher is busy, and replays that refusal once it is idle', () => { + const f = setup(), id = randomUUID(); + expect(() => f.coordinator.start('review-token', id, () => 'The runner is busy.')).toThrow('The runner is busy.'); + // The response was lost; the same click is resent after the runner became idle: it replays, starting nothing. + expect(() => f.coordinator.start('review-token', id)).toThrow('The runner is busy.'); + expect(() => f.coordinator.start('other-token', id)).toThrow(/already used/); + expect(f.store.getMergeJob(identity, id)).toMatchObject({ state: 'failed', refused: true, reason: 'The runner is busy.' }); + expect(f.coordinator.latest()).toBeNull(); + expect(f.coordinator.active).toBe(false); + expect(f.prep.calls).toEqual([]); +}); + +it('saves nothing for a click refused because the server is shutting down, so it may be sent again', () => { + const f = setup(), id = randomUUID(); + f.coordinator.stop(); + expect(() => f.coordinator.start('review-token', id)).toThrow(/shutting down/); + expect(f.store.getMergeJob(identity, id)).toBeNull(); +}); + +it('ends cancelled when the task was cancelled while the job waited, even if the job was not told', async () => { + const f = setup({ checks: () => [pending()] }); + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.coordinator.latest()?.step).toBe('checks')); + expect(f.store.cancelTask(identity, f.store.getTask(identity).stateVersion, randomUUID())).toBe('closed'); + expect(await settled(f.coordinator)).toMatchObject({ state: 'cancelled' }); +}); + +it('ends cancelled when the task is cancelled during the guarded merge\'s validation', async () => { + let cancelAt = Number.POSITIVE_INFINITY, coordinatorRef: MergeJobCoordinator | null = null, storeRef: Store | null = null; + const f = setup({ change: n => { + if (n === cancelAt) { storeRef!.cancelTask(identity, storeRef!.getTask(identity).stateVersion, randomUUID()); coordinatorRef!.cancel(); } + return {}; + } }); + coordinatorRef = f.coordinator; storeRef = f.store; + // Inspection 1 is the wait; inspections 2+ are the guarded merge's validation passes. + cancelAt = 2; + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'cancelled' }); + expect(f.gh.merged).toEqual([]); +}); + +it('stops a guarded merge in flight on shutdown, so shutdown does not wait for GitHub', async () => { + const f = setup({ merge: signal => new Promise((_resolve, reject) => signal?.addEventListener('abort', () => reject(signal.reason), { once: true })) }); + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.gh.merged).toHaveLength(1)); + const closing = Date.now(); + await f.coordinator.close(); + expect(Date.now() - closing).toBeLessThan(2_000); + // GitHub was already asked, so its outcome is unknown: the job ends submitted and the attempt stays in flight. + expect(f.coordinator.latest()).toMatchObject({ state: 'submitted' }); + expect(f.store.getMergeAttempt(identity)?.state).toBe('submitting'); +}); + +it('offers Merge PR while only blockers the job resolves remain, and a retry after a blocked job', () => { + const remote = { mergeable: 'MERGEABLE' }; + const status = (codes: string[], extra: Partial<{ queue: { state: string } | null; remote: { mergeable: string } | null }> = {}) => + ({ ready: false, action: null, blockers: codes.map(code => ({ code, message: code })), remote, queue: null, ...extra }) as Parameters[0]; + const running = { state: 'running' } as MergeJobView, blocked = { state: 'blocked' } as MergeJobView; + expect(withMergeJob(status(['preparation', 'base', 'acceptance', 'check']), null, 'in review')).toMatchObject({ ready: true, action: 'merge', job: null }); + expect(withMergeJob(status(['mergeable'], { remote: { mergeable: 'UNKNOWN' } }), null, 'in review')).toMatchObject({ ready: true }); + expect(withMergeJob(status(['mergeable']), null, 'in review')).toMatchObject({ ready: false, action: null }); + for (const code of ['approval', 'scope', 'unplanned', 'head', 'already-fixed', 'rules', 'base-guard', 'task']) + expect(withMergeJob(status(['preparation', code]), null, 'in review')).toMatchObject({ ready: false, action: null }); + expect(withMergeJob(status([]), running, 'in review')).toMatchObject({ ready: false, job: running }); + expect(withMergeJob(status([], { queue: { state: 'submitting' } }), null, 'in review')).toMatchObject({ ready: false }); + expect(withMergeJob(status([], { remote: null }), null, 'in review')).toMatchObject({ ready: false }); + expect(withMergeJob(status(['preparation']), blocked, 'approved but merge blocked')).toMatchObject({ ready: true, action: 'retry', job: blocked }); + // The task's status, read after the inspection, wins: a task cancelled or merged meanwhile is never offered Merge PR. + for (const closed of ['cancelled', 'merged', 'running']) + expect(withMergeJob(status([]), null, closed)).toMatchObject({ ready: false, action: null }); + // A conflict after the base moved is the job's rebase to resolve; a conflict on its own is a person's. + expect(withMergeJob(status(['base', 'mergeable'], { remote: { mergeable: 'CONFLICTING' } }), null, 'in review')).toMatchObject({ ready: true }); + expect(withMergeJob(status(['mergeable'], { remote: { mergeable: 'CONFLICTING' } }), null, 'in review')).toMatchObject({ ready: false }); +}); + +it('ends as failed, not blocked, when checks fail on a head that moved while waiting', async () => { + const f = setup({ checks: () => [{ context: 'build', appId: null, state: 'failure' }], change: () => ({ head: oid(40) }) }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/head moved/) }); + expect(f.store.getTask(identity).status).toBe('in review'); +}); + +it('asks GitHub to merge nothing once stopped, even if the checks pass meanwhile', async () => { + let passing = false; + const f = setup({ checks: () => [passing ? { context: 'ci', appId: null, state: 'success' } : pending()] }); + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.coordinator.latest()?.step).toBe('checks')); + f.coordinator.stop(); passing = true; + await f.coordinator.current; + expect(f.gh.merged).toEqual([]); + expect(f.coordinator.latest()).toMatchObject({ state: 'running' }); +}); + +it('ends blocked when GitHub refuses the merge while shutdown stops the job', async () => { + let coordinatorRef: MergeJobCoordinator | null = null; + const f = setup({ merge: async () => { coordinatorRef!.stop(); throw new MergeSubmissionError('Base branch was modified.', 'refused'); } }); + coordinatorRef = f.coordinator; + f.coordinator.start('review-token', randomUUID()); + await f.coordinator.current; + expect(f.coordinator.latest()).toMatchObject({ state: 'blocked', reason: expect.stringMatching(/^GitHub refused the merge/) }); + expect(f.store.getTask(identity).status).toBe('approved but merge blocked'); +}); + +it('settles a job a crash left in its merge step from its merge attempt at startup', async () => { + for (const [settle, expected, status] of [ + [() => undefined, 'submitted', 'in review'], + [(store: Store, id: string) => store.finishMergeAttempt(identity, id, { state: 'merged', url: 'https://github.com/owner/repo/pull/7' }), 'merged', 'merged'], + [(store: Store, id: string) => store.queueMergeAttempt(identity, id, 'https://github.com/owner/repo/pull/7'), 'queued', 'in review'], + [(store: Store, id: string) => store.finishMergeAttempt(identity, id, { state: 'failed', reason: 'Head branch was modified.', requiresFreshReview: true }), 'failed', 'in review'], + // GitHub refused while a person changed the review: the refusal says nothing about the changed review. + [(store: Store, id: string) => { + const snapshot = store.getSnapshot(identity); + store.addReviewNote(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, 'P1', 'change', 'Changed during the merge.'); + store.finishMergeAttempt(identity, id, { state: 'failed', reason: 'Base branch was modified.' }); + }, 'failed', 'in review'], + [(store: Store, id: string) => store.finishMergeAttempt(identity, id, { state: 'failed', reason: 'Base branch was modified.' }), 'blocked', 'approved but merge blocked'], + ] as const) { + // GitHub never answers: the job is in its merge step when the process "crashes". + const f = setup({ merge: signal => new Promise((_resolve, reject) => signal?.addEventListener('abort', () => reject(signal.reason), { once: true })) }); + f.coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(f.gh.merged).toHaveLength(1)); + const job = f.store.runningMergeJob(identity)!, attempt = f.store.getMergeAttempt(identity)!; + expect(job).toMatchObject({ step: 'merging', mergeActionId: attempt.actionId }); + settle(f.store, attempt.id); + f.store.settleInterruptedMergeJobs(identity); + expect(f.store.getMergeJob(identity, job.id)).toMatchObject({ state: expected }); + expect(f.store.getTask(identity).status).toBe(status); + } +}); + +it('blocks the job and the task in one write, leaving a task that moved meanwhile as it is', () => { + const store = publishedTask(), first = randomUUID(), second = randomUUID(); + store.beginMergeJob(identity, { actionId: first, token: 'review-token' }); + const before = store.getTask(identity).stateVersion; + expect(store.blockMergeJob(identity, first, 'Required check failed: ci.')).toBe(true); + // The status change advances the task state version, so the next merge prepares again from the start. + expect(store.getTask(identity).stateVersion).toBe(before + 1); + expect(store.getMergeJob(identity, first)).toMatchObject({ state: 'blocked', reason: 'Required check failed: ci.' }); + expect(store.getTask(identity).status).toBe('approved but merge blocked'); + store.beginMergeJob(identity, { actionId: second, token: 'review-token' }); + expect(store.cancelTask(identity, store.getTask(identity).stateVersion, randomUUID())).toBe('closed'); + expect(store.blockMergeJob(identity, second, 'late')).toBe(true); + expect(store.getTask(identity).status).toBe('cancelled'); +}); + +it('rolls the task back when ending a blocked job fails, and leaves the task alone for a job that already ended', () => { + const store = publishedTask(), id = randomUUID(); + store.beginMergeJob(identity, { actionId: id, token: 'review-token' }); + const finish = store.finishMergeJob.bind(store); + store.finishMergeJob = () => { throw new Error('disk full'); }; + expect(() => store.blockMergeJob(identity, id, 'Required check failed: ci.')).toThrow('disk full'); + // One transaction: the task did not move without its job. + expect(store.getTask(identity).status).toBe('in review'); + store.finishMergeJob = finish; + expect(store.finishMergeJob(identity, id, { state: 'failed', reason: 'The pull request moved.' })).toBe(true); + const before = store.getTask(identity).stateVersion; + expect(store.blockMergeJob(identity, id, 'late')).toBe(false); + expect(store.getTask(identity)).toMatchObject({ status: 'in review', stateVersion: before }); + expect(store.getMergeJob(identity, id)).toMatchObject({ state: 'failed' }); +}); + +it('decides a retried job only from its own merge attempt, never from an earlier refused one', async () => { + // The first job's merge is refused: its attempt stays on record as failed. + let refuse = true, conflictFrom = Number.POSITIVE_INFINITY; + const f = setup({ merge: async () => { if (refuse) throw new MergeSubmissionError('Base branch was modified.', 'refused'); return { url: 'u' }; }, + change: n => (n >= conflictFrom ? { mergeable: 'CONFLICTING' } : {}) }); + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'blocked' }); + expect(f.store.getMergeAttempt(identity)).toMatchObject({ state: 'failed' }); + // Retry: the guarded merge's second validation pass now sees a conflict, so it refuses before asking GitHub. + refuse = false; conflictFrom = f.gh.inspections() + 3; + f.coordinator.start('review-token', randomUUID()); + expect(await settled(f.coordinator)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/merge conflicts/) }); + // At startup too: a job that saved its merge action ID but never got an attempt fails, whatever older attempts say. + const crashed = randomUUID(); + f.store.beginMergeJob(identity, { actionId: crashed, token: 'review-token' }); + f.store.updateMergeJob(identity, crashed, { step: 'merging', mergeActionId: randomUUID() }); + f.store.settleInterruptedMergeJobs(identity); + expect(f.store.getMergeJob(identity, crashed)).toMatchObject({ state: 'failed', reason: expect.stringMatching(/server restarted/) }); +}); + +it('refuses, saves and replays a click on a task that is not mergeable or is being cancelled', () => { + const f = setup(), queued = randomUUID(); + f.store.transitionTask(identity, f.store.getTask(identity).stateVersion, 'queued'); + expect(() => f.coordinator.start('review-token', queued)).toThrow('The task is queued; merge it from review.'); + expect(f.store.getMergeJob(identity, queued)).toMatchObject({ refused: true }); + f.store.transitionTask(identity, f.store.getTask(identity).stateVersion, 'in review'); + // A resend replays the refusal, even though the task is in review again. + expect(() => f.coordinator.start('review-token', queued)).toThrow('The task is queued; merge it from review.'); + // A cancel that waits for a push of the rewritten head to settle keeps the task in review, cancel requested. + const snapshot = f.store.getSnapshot(identity), reviewed = { revision: 1, snapshotId: snapshot.id, reviewVersion: f.store.reviewVersion(identity) }; + f.store.recordRebase(identity, reviewed, oid(10), oid(12), [{ oldSha: oid(2), newSha: oid(12) }]); + const after = f.store.getSnapshot(identity); + f.store.beginPrePush(identity, { revision: 1, snapshotId: after.id, reviewVersion: f.store.reviewVersion(identity) }, f.store.getTask(identity).stateVersion, + { branch: BRANCH, from: oid(2), to: oid(12) }); + expect(f.store.cancelTask(identity, f.store.getTask(identity).stateVersion, randomUUID())).toBe('stopping'); + const cancelling = randomUUID(); + expect(() => f.coordinator.start('review-token', cancelling)).toThrow('The task is being cancelled.'); + expect(f.store.getMergeJob(identity, cancelling)).toMatchObject({ refused: true }); + expect(f.prep.calls).toEqual([]); +}); + +it('answers a preparation stopping for shutdown as not applied, saving nothing', () => { + const f = setup(), id = randomUUID(); + (f.prep.fake as unknown as { assertStartable: () => void }).assertStartable = () => { throw new GuardRefusal('The server is shutting down.'); }; + expect(() => f.coordinator.start('review-token', id)).toThrow(ShuttingDownError); + expect(f.store.getMergeJob(identity, id)).toBeNull(); + expect(f.prep.calls).toEqual([]); +}); + +it('refuses a click while a preparation is already running, and replays that refusal', () => { + const f = setup(), id = randomUUID(); + (f.prep.fake as unknown as { assertStartable: () => void }).assertStartable = () => { throw new GuardRefusal('Pre-merge preparation is already running.'); }; + expect(() => f.coordinator.start('review-token', id)).toThrow('Pre-merge preparation is already running.'); + expect(f.store.getMergeJob(identity, id)).toMatchObject({ refused: true }); + expect(f.prep.calls).toEqual([]); +}); + +it('starts no inspection after the wait limit and stops one that runs into it', async () => { + let inspections = 0, hang = false; + const f = setup({ checks: () => [pending()] }); + const inspect = f.gh.client.inspect; + f.gh.client.inspect = (async (request: Parameters[0]) => { + inspections++; + // From the second inspection on, GitHub does not answer; only the wait's limit (or a cancel) ends the read. + if (hang && request?.signal) await new Promise((_resolve, reject) => request.signal!.addEventListener('abort', () => reject(request.signal!.reason), { once: true })); + return inspect(request); + }) as typeof inspect; + const coordinator = new MergeJobCoordinator(f.review, f.prep.fake, f.merges, undefined, { checkWaitMs: 400, firstDelayMs: 5, maxDelayMs: 5 }); jobs.push(coordinator); + coordinator.start('review-token', randomUUID()); + await vi.waitFor(() => expect(coordinator.latest()?.step).toBe('checks')); + hang = true; + const started = Date.now(); + await coordinator.current; + expect(Date.now() - started).toBeLessThan(3_000); + expect(coordinator.latest()).toMatchObject({ state: 'blocked', reason: expect.stringMatching(/did not pass within/) }); + const after = inspections; + await new Promise(resolve => setTimeout(resolve, 100)); + expect(inspections).toBe(after); + expect(f.gh.merged).toEqual([]); +}); diff --git a/test/merge-task-pr.test.ts b/test/merge-task-pr.test.ts index 4fc4bed5..eb64848e 100644 --- a/test/merge-task-pr.test.ts +++ b/test/merge-task-pr.test.ts @@ -4,6 +4,7 @@ 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 { DatabaseSync } from 'node:sqlite'; import { GuardRefusal } from '../runner/lifecycle.ts'; import { MergeCoordinator, type PublishedTarget } from '../runner/merge.ts'; import type { ReviewService } from '../runner/review.ts'; @@ -30,8 +31,8 @@ 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); +function runningTask(path = ':memory:'): Store { + const store = new Store(path); 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'); @@ -47,8 +48,8 @@ const opened = (store: Store, openingId: string, number: number, mayReview = tru 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(); +function published(path?: string) { + const store = runningTask(path); const opening = begin(store); expect(opened(store, opening.openingId, 7)).toBe('in review'); return { store, opening }; @@ -252,7 +253,7 @@ describe('the merge gate with a runner block (#121)', () => { 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 }); + expect(gh.targets[0]).toEqual({ pullRequest: 7, ownPullRequests: [7], headBranch: BRANCH, alreadyFixedSince: oid(9), ownCommits: [oid(2)] }); await merges.merge('review-token'); expect(gh.merged).toEqual([7]); expect(store.getMergeAttempt(identity)).toMatchObject({ pullRequest: 7, state: 'merged' }); @@ -332,6 +333,124 @@ describe('the merge gate with a runner block (#121)', () => { expect(gh.merged).toEqual([]); }); + it('scans for an earlier fix from the pre-PR check\'s base after a rebase, skipping the task\'s own commits', async () => { + const { store } = published(), gh = github(store); + const reviewed = () => ({ revision: 1, snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity) }); + // A pre-merge rebase moved the reviewed base from oid(1) to oid(10) and rewrote the owned head oid(2) as oid(12). + store.recordRebase(identity, reviewed(), oid(10), oid(12), [{ oldSha: oid(2), newSha: oid(12) }]); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + await merges.status(undefined, true); + expect(gh.targets.at(-1)).toMatchObject({ pullRequest: 7, alreadyFixedSince: oid(9) }); + expect([...gh.targets.at(-1)!.ownCommits!].sort()).toEqual([oid(12), oid(2)].sort()); + }); + + it('keeps scanning from the PR\'s own pre-PR check after a re-publish records a newer check from a rebased base', async () => { + const { store } = published(), gh = github(store); + const reviewed = () => ({ revision: 1, snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity) }); + store.recordRebase(identity, reviewed(), oid(10), oid(12), [{ oldSha: oid(2), newSha: oid(12) }]); + // A re-publish after an amendment: the runner runs again and records a clear check scanned from the rebased base. + 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 }); + expect(store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, + reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: oid(19) } }).snapshotId).toBe(store.getSnapshot(identity).id); + expect(store.getSnapshot(identity).base).toBe(oid(10)); + toReview(store); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + await merges.status(undefined, true); + expect(gh.targets.at(-1)).toMatchObject({ alreadyFixedSince: oid(9) }); + }); + + it('scans from the first opening\'s check for a new pull request opened after a rebase', async () => { + const { store } = published(); + const reviewed = () => ({ revision: 1, snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity) }); + // A pre-merge rebase brought base commits after oid(9) under the task. The person then closed PR 7 and published + // again: the new opening's own pre-PR check scanned only from the rebased base (to oid(19)), and a later check could + // not complete. Base commits between oid(9) and the rebased base were scanned by no check. + store.recordRebase(identity, reviewed(), oid(10), oid(12), [{ oldSha: oid(2), newSha: oid(12) }]); + 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 }); + const check = store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, + reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: oid(19) } }); + const later = store.beginPullRequest(identity, { checkId: check.id, repository: 'owner/repo', base: 'main', headBranch: BRANCH, headSha: store.getSnapshot(identity).head, draft: false }); + store.recordPullRequestOpened(identity, later.openingId, { number: 8, url: url(8), headSha: store.getSnapshot(identity).head, draft: false }, false); + store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'unknown', reason: 'GitHub could not be read.' } }); + toReview(store); + const gh = github(store); + const merges = new MergeCoordinator(service(store), gh.client, undefined, undefined, PUBLISHED); + await merges.status(undefined, true); + expect(gh.targets.at(-1)).toMatchObject({ pullRequest: 8, alreadyFixedSince: oid(9) }); + }); + + it('scans from the oldest clear check for a PR opened before schema v19, whatever later checks found', async () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-v19-')); roots.push(root); + const path = join(root, 'state.sqlite'); + const { store } = published(path); + const reviewed = () => ({ revision: 1, snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity) }); + store.recordRebase(identity, reviewed(), oid(10), oid(12), [{ oldSha: oid(2), newSha: oid(12) }]); + // A later re-publish recorded a clear check scanned from the rebased base, then one that could not complete. A compare + // range excludes its start, so starting at the later clear check's tip could skip the rebased base commit itself. + 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 }); + store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: oid(19) } }); + store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'unknown', reason: 'GitHub could not be read.' } }); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'in review'); + // The opening was recorded before v19: it carries no check. + store.close(); stores.splice(stores.indexOf(store), 1); + const raw = new DatabaseSync(path); raw.exec('UPDATE task_pull_requests SET check_id=NULL'); raw.close(); + const reopened = new Store(path); stores.push(reopened); + expect(reopened.taskPullRequests(identity)[0]!.checkId).toBeNull(); + const gh = github(reopened); + const merges = new MergeCoordinator(service(reopened), gh.client, undefined, undefined, PUBLISHED); + await merges.status(undefined, true); + expect(gh.targets.at(-1)).toMatchObject({ alreadyFixedSince: oid(9) }); + }); + + it('scans from the first opening into the current base branch after the base branch setting changed', async () => { + const { store } = published(); + // PR 7 went into main (its check scanned main to oid(9)). github.baseBranch then became release: the person closed + // PR 7 and published again, into release, after a check that scanned release to oid(29). main's tip says nothing + // about release. + 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 }); + const check = store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, + reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: oid(29) } }); + const release = store.beginPullRequest(identity, { checkId: check.id, repository: 'owner/repo', base: 'release', headBranch: BRANCH, headSha: oid(2), draft: false }); + opened(store, release.openingId, 8, false); + toReview(store); + const gh = github(store); + await new MergeCoordinator(service(store), gh.client, undefined, undefined, { ...PUBLISHED, baseBranch: 'release' }).status(undefined, true); + expect(gh.targets.at(-1)).toMatchObject({ pullRequest: 8, alreadyFixedSince: oid(29) }); + }); + + it('scans from the task\'s original base when the first opening into the base branch followed a rebase', async () => { + const { store } = published(); + const reviewed = () => ({ revision: 1, snapshotId: store.getSnapshot(identity).id, reviewVersion: store.reviewVersion(identity) }); + // PR 7 went into main from the original base oid(1). A pre-merge rebase moved the task onto main's oid(10), then + // github.baseBranch became release (which contains oid(10)); the first opening into release followed a check that + // scanned release only from oid(10). Base commits between oid(1) and oid(10) were scanned on release by no check. + store.recordRebase(identity, reviewed(), oid(10), oid(12), [{ oldSha: oid(2), newSha: oid(12) }]); + 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 }); + const check = store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: store.getSnapshot(identity).id, + reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: oid(29) } }); + const release = store.beginPullRequest(identity, { checkId: check.id, repository: 'owner/repo', base: 'release', headBranch: BRANCH, headSha: store.getSnapshot(identity).head, draft: false }); + store.recordPullRequestOpened(identity, release.openingId, { number: 8, url: url(8), headSha: store.getSnapshot(identity).head, draft: false }, false); + toReview(store); + const gh = github(store); + await new MergeCoordinator(service(store), gh.client, undefined, undefined, { ...PUBLISHED, baseBranch: 'release' }).status(undefined, true); + expect(gh.targets.at(-1)).toMatchObject({ pullRequest: 8, alreadyFixedSince: oid(1) }); + }); + it('refuses pre-merge preparation for a pull request that is no longer open', async () => { const { store } = published(); const gh = github(store, () => ({ pullRequestState: 'CLOSED' })); @@ -358,7 +477,7 @@ describe('the merge gate with a runner block (#121)', () => { 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 }); + expect(gh.targets[0]).toEqual({ pullRequest: 8, ownPullRequests: [7, 8], headBranch: BRANCH, alreadyFixedSince: oid(9), ownCommits: [oid(2)] }); }); it('refuses when the task\'s PR changes between the validation passes', async () => { diff --git a/test/merge.test.ts b/test/merge.test.ts index 336c04c0..42d412f4 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -711,6 +711,8 @@ it('requires current-snapshot approvals after an ordinary queue removal and forc await h.coordinator.pollQueue(); h.replaceHead(sha('c')); expect((await h.coordinator.status(h.view())).action).toBeNull(); + expect((await h.coordinator.status(h.view())).blockers).toEqual(expect.arrayContaining([{ code: 'queue-head', + message: 'The pull request changed after the last merge attempt (Checks failed.). Approve every item again for the replacement snapshot, then merge.' }])); h.store.saveReview(h.identity, h.view().expected, [{ item: 'P1', fingerprint: 'replacement-review' }], []); expect((await h.coordinator.status(h.view())).action).toBe('merge'); } finally { await h.coordinator.close(); h.store.close(); } @@ -723,6 +725,9 @@ it('requires current-revision approvals after a same-snapshot plan amendment', a await h.coordinator.pollQueue(); h.amendPlan(); expect((await h.coordinator.status(h.view())).action).toBeNull(); + // The snapshot is unchanged: the message names the plan, not a replacement snapshot. + expect((await h.coordinator.status(h.view())).blockers).toEqual(expect.arrayContaining([{ code: 'queue-head', + message: 'The plan changed after the last merge attempt (Checks failed.). Approve every item again for the new plan revision, then merge.' }])); h.store.saveReview(h.identity, h.view().expected, [{ item: 'P1', fingerprint: 'amended-plan-review' }], []); expect((await h.coordinator.status(h.view())).action).toBe('merge'); } finally { await h.coordinator.close(); h.store.close(); } @@ -1297,6 +1302,17 @@ it('gives an injected check the merge inputs and fails closed on an unknown outc expect(() => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, { check: checks.check })).toThrow(/merge repository/); }); +it('scans for an earlier fix from the target\'s scan start and skips the task\'s own commits (F6b)', async () => { + const inputs: AlreadyFixedInput[] = []; + const checks: AlreadyFixedGateway = { repository: 'Owner/Repo', check: async input => { inputs.push(input); return { outcome: 'clear', baseHead: sha('9') }; } }; + const run = async (args: readonly string[]) => mergeReads(args, { baseRefName: 'release/1', baseRefOid: sha('c') }); + const client = new GhMergeGateway({ repository: 'owner/repo', pullRequest: 7, issue: 21 }, run, checks); + await client.inspect({ fresh: true, target: { pullRequest: 7, alreadyFixedSince: sha('a'), ownCommits: [sha('e'), sha('d'), sha('e')] } }); + expect(inputs[0]).toEqual({ issue: 21, taskBase: sha('a'), baseBranch: 'release/1', ownPullRequests: [7], ownCommits: new Set([sha('d'), sha('e')]) }); + await expect(client.inspect({ fresh: true, target: { pullRequest: 7, alreadyFixedSince: 'main' } })).rejects.toThrow(/scan start/); + await expect(client.inspect({ fresh: true, target: { pullRequest: 7, ownCommits: ['x'] } })).rejects.toThrow(/own commit/); +}); + it('keeps the caller cancellation when the merge check is aborted', async () => { const controller = new AbortController(); let started!: () => void; diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index 56290fd1..9f57ce3e 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -170,7 +170,7 @@ it.each([9, 10])('migrates a v%i database to the current schema, reading its exi const migrated = open(); expect(migrated.getSuggestions(identity, id)).toMatchObject({ mode: 'suggest', state: 'pending' }); const db = new DatabaseSync(path); - try { expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); } finally { db.close(); } + try { expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); } finally { db.close(); } }); it('adds checkpoint bindings to v14 planning requests without losing their state', () => { const { store, path, open } = fixture(), id = store.beginSuggestions(identity, state(store), 'draft'); @@ -181,7 +181,7 @@ it('adds checkpoint bindings to v14 planning requests without losing their state expect(migrated.getDraft(identity, id).state).toBe('pending'); const db = new DatabaseSync(path); try { - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); expect(db.prepare('PRAGMA table_info(requests)').all().some(column => column.name === 'continuation')).toBe(true); } finally { db.close(); } }); diff --git a/test/pre-merge.test.ts b/test/pre-merge.test.ts index 6be84a5e..890a6a98 100644 --- a/test/pre-merge.test.ts +++ b/test/pre-merge.test.ts @@ -4,7 +4,11 @@ import { join } from 'node:path'; import { randomUUID } from 'node:crypto'; import { afterEach, expect, it, vi } from 'vitest'; import { COMMAND_CHECK_SETTLEMENT_RESERVE_MS, PRE_MERGE_PROCESS_SETTLEMENT_RESERVE_MS, PreMergeCoordinator, - type PreMergeAuthorization } from '../runner/pre-merge.ts'; + type PreMergeAuthorization, type PreMergeResult } from '../runner/pre-merge.ts'; +import { MergeCoordinator } from '../runner/merge.ts'; +import { MergeJobCoordinator } from '../runner/merge-job.ts'; +import { MergeSubmissionError, type MergeGateway } from '../github/merge.ts'; +import { openingMarker } from '../github/pull-requests.ts'; import { ReviewService } from '../runner/review.ts'; import { createDemo } from '../scripts/demo.ts'; import { fixtureGit } from './fixtures/git.ts'; @@ -24,7 +28,17 @@ const unusedPush = { push: async () => { throw new Error('No push expected.'); } vi.setConfig({ testTimeout: 15_000 }); afterEach(() => { vi.restoreAllMocks(); for (const service of services.splice(0)) service.close(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); -const markRunnerOwned = (service: ReviewService) => { +/** The task's PR as the runner publishes it (F6b tests): a clear pre-PR check, then PR 7 from BRANCH into main. */ +const publishPullRequest = (service: ReviewService) => { + const identity = service.config.identity, store = service.store, snapshot = store.getSnapshot(identity); + const check = store.recordAlreadyFixed(identity, store.getTask(identity).stateVersion, { snapshotId: snapshot.id, + reviewVersion: store.reviewVersion(identity), draft: false, result: { outcome: 'clear', baseHead: snapshot.base } }); + const opening = store.beginPullRequest(identity, { checkId: check.id, repository: 'acme/app', base: 'main', headBranch: BRANCH, + headSha: snapshot.head, draft: false }); + expect(store.recordPullRequestOpened(identity, opening.openingId, { number: 7, url: 'https://github.com/acme/app/pull/7', + headSha: snapshot.head, draft: false }, true)).toBe('in review'); +}; +const markRunnerOwned = (service: ReviewService, published = false) => { const identity = service.config.identity; service.store.transitionTask(identity, service.store.getTask(identity).stateVersion, 'queued'); const attempt = service.store.admitAttempt(identity, { expectedStateVersion: service.store.getTask(identity).stateVersion, @@ -32,7 +46,8 @@ const markRunnerOwned = (service: ReviewService) => { service.store.markRunning(identity, attempt.id); service.store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 0, valid: true, result: { unchanged: false, head: service.store.getSnapshot(identity).head } }); - service.store.transitionTask(identity, service.store.getTask(identity).stateVersion, 'in review'); + if (published) publishPullRequest(service); + else service.store.transitionTask(identity, service.store.getTask(identity).stateVersion, 'in review'); service.config.runnerRepository = service.config.repository; }; const markRunnerUnchanged = (service: ReviewService) => { @@ -76,6 +91,10 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: rebaseResult?: (service: ReviewService, input: { oldHead: string; onto: string }, run: number) => string | undefined; /** Branch reads never answer; only their abort signal ends them. */ branchReadHangs?: boolean; + /** The runner published the task's PR 7 (merging then requires preparation, as in production). */ + published?: boolean; + /** Set up everything but start no preparation; `result` is then null. */ + deferStart?: boolean; /** Remote fetches fail once codeboost has pushed. */ fetchFailsAfterPush?: boolean; /** A collaborator pushes this commit to the branch just before codeboost's push, which is then refused. */ @@ -116,7 +135,7 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: service.store.createPlan(JSON.stringify(plan), 'json', context, base, head); let view = service.load(); service.store.recordHistory(identity, view.expected, base, head, [{ sha: head, owner: 'P1', origin: 'owned', sourceSha: null }]); - if (options.runnerCommitted !== false) markRunnerOwned(service); else markRunnerUnchanged(service); + if (options.runnerCommitted !== false) markRunnerOwned(service, options.published === true); else markRunnerUnchanged(service); view = service.load(); view = service.act({ action: 'approve', item: 'P1', token: view.token }); let coordinator!: PreMergeCoordinator, rebaseRuns = 0; let rebaseAborts = 0; @@ -256,7 +275,7 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: }) as RunnerCoordinator['start']; } const task = service.store.getTask(identity); - const result = await coordinator.start({ stateVersion: task.stateVersion, + const result = options.deferStart ? null as unknown as PreMergeResult : await coordinator.start({ stateVersion: task.stateVersion, reviewVersion: view.expected.reviewVersion!, snapshotId: view.snapshot.id, base: view.snapshot.base, head: view.snapshot.head }); return { service, coordinator, runner, result, rebased, checkedHeads, cancelReasons, pushes, remote: () => remotePair, branchReads: () => branchReads, fetchesAfterPush: () => fetchesAfterPush, @@ -1466,3 +1485,109 @@ it('starts no settlement read when less than the process-settlement reserve is l expect(pushMarker(fixture)).toMatchObject({ to: fixture.rebased }); await fixture.coordinator.close(); await fixture.runner.close(); }); + +// F6b of #22: the acceptance cases, run through the one-click merge job with real preparation. +async function mergeJob(rebasedText: string, commandExit?: number, options: Parameters[2] = {}, refuse: () => boolean = () => false) { + const fixture = await rebaseFixture(rebasedText, commandExit, { ...options, deferStart: true, published: true }); + const merged: string[] = []; + // As production builds it with a runner: the task's published PR, which requires durable preparation readiness. + const gateway: MergeGateway = { + inspect: async request => { + const pr = fixture.service.store.taskPullRequests(fixture.service.config.identity).find(row => row.number === request?.target?.pullRequest)!; + return { base: fixture.remote().base, head: fixture.remote().head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', + rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false, + published: { headBranch: request!.target!.headBranch!, baseBranch: 'main', crossRepository: false, marker: openingMarker(pr.openingId), + branchOpen: [{ number: 7, base: 'main' }] } }; + }, + merge: async head => { + merged.push(head); + if (refuse()) throw new MergeSubmissionError('Required status check "ci" is expected.', 'refused'); + return { url: 'https://github.com/acme/app/pull/7' }; + }, + }; + const merges = new MergeCoordinator(fixture.service, gateway, undefined, undefined, + { repository: 'acme/app', baseBranch: 'main', requiresPreparation: true }); + const jobs = new MergeJobCoordinator(fixture.service, fixture.coordinator, merges, undefined, { checkWaitMs: 2_000, firstDelayMs: 5, maxDelayMs: 20 }); + const start = () => jobs.start(fixture.service.load().token, randomUUID()); + const settled = async () => { await jobs.current; return jobs.latest()!; }; + const close = async () => { await jobs.close(); await fixture.coordinator.close(); await fixture.runner.close(); }; + return { ...fixture, merged, merges, jobs, start, settled, close }; +} + +it('merges through the job when a moved base leaves approvals valid, pushing and checking the rewritten head', async () => { + const f = await mergeJob('feature\n', 0); + f.start(); + expect(await f.settled()).toMatchObject({ state: 'merged' }); + expect(f.pushes).toHaveLength(1); + expect(f.checkedHeads).toEqual([f.rebased]); + expect(f.merged).toEqual([f.rebased]); + await f.close(); +}); + +it('stops the job for review when the rebase makes an approval stale, and merges nothing', async () => { + const f = await mergeJob('changed by rebase\n'); + f.start(); + expect(await f.settled()).toMatchObject({ state: 'review-required', reason: expect.stringMatching(/requires refreshed attribution or approval/) }); + expect(f.merged).toEqual([]); + await f.close(); +}); + +it('fails the job when a command check breaks on the rewritten head', async () => { + const f = await mergeJob('feature\n', 1); + f.start(); + expect(await f.settled()).toMatchObject({ state: 'failed', reason: expect.stringMatching(/command checks did not pass/) }); + expect(f.merged).toEqual([]); + await f.close(); +}); + +it('stops the job for review when a collaborator pushed to the pull request', async () => { + const f = await mergeJob('feature\n'); + const repository = f.service.config.repository; + writeFileSync(join(repository, 'late.txt'), 'collaborator\n'); fixtureGit(repository, 'add', 'late.txt'); fixtureGit(repository, 'commit', '-qm', 'collaborator'); + f.moveRemote({ base: f.remote().base, head: fixtureGit(repository, 'rev-parse', 'HEAD') }); + f.start(); + expect(await f.settled()).toMatchObject({ state: 'review-required', reason: expect.stringMatching(/head moved/) }); + expect(f.pushes).toEqual([]); + expect(f.merged).toEqual([]); + await f.close(); +}); + +it('stops a retry for review when its preparation rebases after GitHub refused the merge', async () => { + let refuse = true; + const f = await mergeJob('feature\n', 0, { + // The retry's rebase replays the already-rebased head onto the moved base, keeping its content. + rebaseResult: (service, input, run) => run < 2 ? undefined : fixtureGit(service.config.repository, 'commit-tree', + fixtureGit(service.config.repository, 'rev-parse', `${input.oldHead}^{tree}`), '-p', input.onto, '-m', 'feature, rebased again'), + }, () => refuse); + f.start(); + expect(await f.settled()).toMatchObject({ state: 'blocked', reason: expect.stringMatching(/^GitHub refused the merge/) }); + expect(f.service.store.getTask(f.service.config.identity).status).toBe('approved but merge blocked'); + // The base moves again before the person retries: the retry's preparation rebases, making a new snapshot. + refuse = false; + const repository = f.service.config.repository, base = f.remote().base; + const moved = fixtureGit(repository, 'commit-tree', fixtureGit(repository, 'rev-parse', `${base}^{tree}`), '-p', base, '-m', 'base moved after the refusal'); + f.moveRemote({ base: moved, head: f.remote().head }); + f.start(); + // After a merge attempt, a replacement snapshot needs every item approved again (#24): the retry stops for review. + expect(await f.settled()).toMatchObject({ state: 'review-required', reason: expect.stringMatching(/requires refreshed attribution or approval/) }); + expect(f.service.load().items.every(item => item.state === 'stale')).toBe(true); + expect((await f.merges.status()).blockers).toEqual(expect.arrayContaining([ + expect.objectContaining({ code: 'queue-head', message: expect.stringMatching(/Approve every item again for the replacement snapshot/) })])); + expect(f.merged).toHaveLength(1); + await f.close(); +}, 60_000); + +it('ends the job cancelled when the task is cancelled while a command check runs', async () => { + const f = await mergeJob('feature\n', 0, { commandWaitsForCancellation: true }); + f.start(); + await vi.waitFor(() => expect(f.runner.isActive(f.service.config.identity)).toBe(true)); + const identity = f.service.config.identity; + // As the cancel route does: the task's cancel commits (it waits on the running check), then the job is told. + expect(f.coordinator.cancelTask(f.service.store.getTask(identity).stateVersion, randomUUID())).toBe('stopping'); + f.jobs.cancel(); + expect(await f.settled()).toMatchObject({ state: 'cancelled' }); + expect(new Set(f.cancelReasons)).toEqual(new Set(['cancelled'])); + expect(f.merged).toEqual([]); + await vi.waitFor(() => expect(f.service.store.getTask(identity).status).toBe('cancelled')); + await f.close(); +}); diff --git a/test/publish.test.ts b/test/publish.test.ts index 8322ffab..3f347e5f 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -358,7 +358,7 @@ describe('schema v7', () => { expect(upgraded.taskPullRequests(identity)).toEqual([]); expect(upgraded.latestAlreadyFixed(identity)).toBeNull(); const db = new DatabaseSync(path, { readOnly: true }); - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); const names = db.prepare("SELECT name FROM sqlite_master WHERE type IN ('table','index') AND (name LIKE '%pull_requests%' OR name LIKE 'already_fixed_checks%') AND name NOT LIKE 'sqlite_autoindex%' ORDER BY name").all().map(row => row.name); expect(names).toEqual(['already_fixed_checks', 'already_fixed_checks_task', 'task_pull_requests', 'task_pull_requests_number', 'task_pull_requests_opening', 'task_pull_requests_task']); db.close(); diff --git a/test/runner-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index 21a4e9c0..b22d5772 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -105,7 +105,7 @@ describe('schema v12 review ordering', () => { migrated.saveReview(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: migratedVersion }, [{ item: 'P1', fingerprint: 'approved-after-upgrade' }], []); expect(migrated.unapprovedExecutionItems(identity, 1)).toEqual([]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); }); @@ -783,7 +783,7 @@ describe('runner commits and preparation groups (#87)', () => { db.exec('ALTER TABLE attempts DROP COLUMN preparation_identity; PRAGMA user_version=15;'); db.close(); const reopened = open(path); expect(reopened.interruptedAttempts()).toEqual([expect.objectContaining({ id: attempt.id, preparationIdentity: null })]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); }); @@ -917,7 +917,7 @@ describe('durable safety findings (#87 item 3)', () => { db.exec('ALTER TABLE attempts DROP COLUMN safety_finding; PRAGMA user_version=7;'); db.close(); const reopened = open(path); expect(reopened.getAttempt(identity, attempt.id).safetyFinding).toBeNull(); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); it('adds the durable owed marker to a version 12 database', () => { @@ -927,7 +927,7 @@ describe('durable safety findings (#87 item 3)', () => { db.exec('ALTER TABLE attempts DROP COLUMN safety_owed; PRAGMA user_version=12;'); db.close(); const reopened = open(path); expect(reopened.owedSafetyFindings()).toEqual([]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); it('restores and clears an escalation owed behind a human gate across real store reopens', () => { @@ -967,7 +967,7 @@ describe('allocation baseline (#91)', () => { db.exec('ALTER TABLE attempts DROP COLUMN metadata_baseline; ALTER TABLE attempts DROP COLUMN storage_base; PRAGMA user_version=8;'); db.close(); const reopened = open(path); expect(reopened.interruptedAttempts()).toEqual([expect.objectContaining({ id: attempt.id, metadataBaseline: null, storageBase: null })]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); }); describe('publish outcomes (#103)', () => { @@ -983,7 +983,7 @@ describe('publish outcomes (#103)', () => { reopened.recordPublish(identity, { outcome: 'opened', draft: false, message: 'Pull request #1 is open.', number: 1, url: 'https://github.com/o/r/pull/1' }); expect(reopened.lastPublish(identity)).toMatchObject({ outcome: 'opened', number: 1 }); expect(reopened.getTask(identity)).toEqual(before); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); it('stamps an outcome with the version the publish saw, and refuses one it cannot have seen (#114)', () => { const { store } = queued(), version = store.getTask(identity).stateVersion; @@ -1023,7 +1023,7 @@ describe('pre-merge push marker (F6a of #22)', () => { const legacy = new DatabaseSync(path); legacy.exec('ALTER TABLE tasks DROP COLUMN push_in_progress; PRAGMA user_version=17;'); legacy.close(); expect(open(path).getTask(identity).pushInProgress).toBeNull(); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); }); it('claims only a push of the current head over a durable rewrite predecessor', () => { diff --git a/test/runner-production.test.ts b/test/runner-production.test.ts index bd5502d0..13265ebc 100644 --- a/test/runner-production.test.ts +++ b/test/runner-production.test.ts @@ -122,6 +122,15 @@ describe('runner startup', () => { expect(await assembly.sources.vendor(service.config.identity)).toBe('claude'); expect(() => assembly.sources.planContext({ ...service.config.identity, planId: 'other' })).toThrow(/only its configured plan/); }); + it('fails a merge job an earlier process left running before it assembles anything (F6b)', async () => { + const { root, service } = fixture(), calls: string[] = []; + const actionId = randomUUID(); + service.store.beginMergeJob(service.config.identity, { actionId, token: 'review-token' }); + await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'sha256:x', recovery: () => recovery(calls) }); + expect(service.store.getMergeJob(service.config.identity, actionId)).toMatchObject({ state: 'failed', + reason: expect.stringMatching(/server restarted/) }); + }); it('pushes a prepared rewrite only under an exact lease on the PR head it rewrote', async () => { const { root, service } = fixture(), calls: string[] = []; const assembly = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock(calls), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index 2939ce91..3e735b05 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, rmSync } from 'node:fs'; +import { chmodSync, mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { randomUUID } from 'node:crypto'; @@ -16,7 +16,7 @@ import { PullRequestPublisher } from '../runner/publish.ts'; import { ensureCommit, openRunnerRepository } from '../runner/runner-repository.ts'; import { baseBranch } from '../runner/production.ts'; import { MergeCoordinator } from '../runner/merge.ts'; -import type { MergeGateway } from '../github/merge.ts'; +import { MergeSubmissionError, type MergeGateway } from '../github/merge.ts'; import { OWED_REFUSAL } from '../runner/publishing.ts'; import { PullRequestMisplaced, openingMarker, type OpenPullRequestInput, type PullRequestGateway } from '../github/pull-requests.ts'; import type { AlreadyFixedGateway } from '../github/already-fixed.ts'; @@ -24,6 +24,8 @@ import type { PlanIdentity } from '../core/identity.ts'; import { approveItem } from '../core/approvals.ts'; import { GhIssueGateway, type IssueTrustGateway } from '../github/issues.ts'; import type { PreMergeCoordinator } from '../runner/pre-merge.ts'; +import { MergeJobCoordinator, type MergeJobTiming } from '../runner/merge-job.ts'; +import type { RemoteMergeState, RequiredCheck } from '../github/merge.ts'; vi.setConfig({ testTimeout: 30_000 }); const roots: string[] = [], cleanups: (() => Promise | void)[] = []; @@ -232,13 +234,16 @@ function world(): World { * earlier process would have left it. */ async function serve(w: World, options: { before?: (service: ReviewService) => void; onPushSpawn?: (n: number, app: () => App, close: () => Promise) => void; demo?: boolean; startup?: boolean; settleMs?: number; env?: NodeJS.ProcessEnv; hold?: Promise; shortRetryMs?: number; findingSource?: FindingSource; - issueGateway?: IssueTrustGateway; preMerge?: (service: ReviewService) => PreMergeCoordinator } = {}) { + issueGateway?: IssueTrustGateway; preMerge?: (service: ReviewService) => PreMergeCoordinator; mergeGateway?: MergeGateway; mergeJobTiming?: MergeJobTiming; + /** A runner block and no github.pullRequest: merging targets the PR the runner published, after preparation. */ + runnerPublished?: boolean } = {}) { let app: App | undefined, spawns = 0, closing: Promise | undefined; const close = () => closing ??= app!.close(); let branchOf: (identity: PlanIdentity) => string = () => ''; let findings: SafetyFindings | undefined; - const config = { ...w.demo, demo: options.demo ?? false, ...(options.issueGateway ? { github: { repository: options.issueGateway.repository, issue: 3, pullRequest: 1, baseBranch: 'main' } } : {}) }; - app = await startServer(config, 0, undefined, undefined, 2_000, options.issueGateway, undefined, undefined, async service => { + const config = { ...w.demo, demo: options.demo ?? false, ...(options.issueGateway ? { github: { repository: options.issueGateway.repository, issue: 3, + ...(options.runnerPublished ? {} : { pullRequest: 1 }), baseBranch: 'main' } } : {}), ...(options.runnerPublished ? { runner: {} } : {}) }; + app = await startServer(config, 0, undefined, options.mergeGateway, 2_000, options.issueGateway, undefined, undefined, async service => { const identity = service.config.identity, task = service.store.getTask(identity), plan = service.store.getPlan(identity); if (task.status !== 'merged' && task.status !== 'cancelled' && service.store.unapprovedExecutionItems(identity, plan.revision).length) { const review = service.load(); @@ -284,7 +289,7 @@ async function serve(w: World, options: { before?: (service: ReviewService) => v return { deps, sources, findings, publisher, ...(options.preMerge ? { preMerge: () => options.preMerge!(service) } : {}), ...(options.env ? { env: options.env } : {}), ...(options.shortRetryMs ? { shortRetryMs: options.shortRetryMs } : {}), recovery: { finalized: [], requeue: [], removedDirectories: [], unknownEntries: [], unmatchedStorage: [], repairedMerges: [] } }; - }); + }, options.mergeJobTiming ? { mergeJobTiming: options.mergeJobTiming } : {}); cleanups.push(close); // As the CLI does once it has verified the lock. if (options.startup !== false) app.publishOwed(() => undefined); @@ -1723,3 +1728,305 @@ describe('closing a cancelled task\'s pull requests (#111)', () => { expect(await act(app, 'publish')).toMatchObject({ status: 409, body: { error: 'The task is cancelled; its pull requests are closed, not published.' } }); }); }); + +describe('the one-click merge job (F6b of #22)', () => { + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { return { number, authorLogin: 'member', collaborator: true }; }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + /** Preparation as F5/F6a end it: readiness recorded exactly on its own action. */ + const preparation = (service: ReviewService) => ({ + get active() { return false; }, get last() { return null; }, assertStartable() {}, settlePendingPush: async () => null, + cancelTask: () => 'closed' as const, close: async () => undefined, + start: async (expected: { actionId: string }) => { + const identity = service.config.identity, snapshot = service.store.getSnapshot(identity); + const readiness = { stateVersion: service.store.getTask(identity).stateVersion, reviewVersion: service.store.reviewVersion(identity), + snapshotId: snapshot.id, base: snapshot.base, head: snapshot.head, commandPolicyDigest: service.commandPolicyDigest() }; + return service.store.settlePreMergeAction(identity, expected.actionId, + { state: 'ready', base: snapshot.base, head: snapshot.head, checked: [], reason: null }, readiness); + }, + } as unknown as PreMergeCoordinator); + /** GitHub showing the task's published PR at the reviewed pair; `checks()` gives its required checks now. */ + const gateway = (store: () => Store, checks: () => RequiredCheck[], hang: () => boolean = () => false): + MergeGateway & { merged: string[]; inspections: () => number; hung: AbortSignal[] } => { + const merged: string[] = [], hung: AbortSignal[] = []; + let inspections = 0; + return { merged, hung, inspections: () => inspections, + inspect: async request => { + inspections++; + // From this inspection on, GitHub does not answer; only the caller's abort ends the read. + if (hang() && request?.signal) { + hung.push(request.signal); + await new Promise((_resolve, reject) => request.signal!.addEventListener('abort', () => reject(request.signal!.reason), { once: true })); + } + const identity = createIdentity(), pr = store().taskPullRequests(identity).find(row => row.number === request?.target?.pullRequest); + const snapshot = store().getSnapshot(identity); + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, atomicBaseGuard: true, + mergeQueue: false, requiredChecks: checks(), alreadyFixed: 'clear', pullRequest: request!.target!.pullRequest, draft: false, + published: { headBranch: request!.target!.headBranch!, baseBranch: 'main', crossRepository: false, marker: openingMarker(pr!.openingId), + branchOpen: [{ number: request!.target!.pullRequest, base: 'main' }] } } as RemoteMergeState; + }, + merge: async head => { merged.push(head); return { url: 'https://github.com/acme/app/pull/100' }; } }; + }; + let createIdentity: () => PlanIdentity = () => { throw new Error('not set'); }; + /** A review a person could merge: the demo's out-of-scope mode change undone, its unplanned changes accepted, all approved. */ + const mergeable = (w: World) => { + chmodSync(join(w.demo.repository, 'run.sh'), 0o644); git(w.demo.repository, 'commit', '-qam', 'Restore declared scope'); + }; + const acceptAndApprove = (service: ReviewService) => { + let view = service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) if (item.state !== 'approved') + view = service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + }; + const review = async (app: App) => (await fetch(`${new URL(app.url).origin}/api/review`, { headers: { 'x-codeboost-token': app.token } })).json() as Promise>; + const merge = async (app: App, token: string, actionId = randomUUID()) => { + const response = await fetch(`${new URL(app.url).origin}/api/action`, { method: 'POST', headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' }, + body: JSON.stringify({ action: 'merge', token, actionId }) }); + return { status: response.status, body: await response.json() as Record }; + }; + const mergeState = async (app: App) => (await fetch(`${new URL(app.url).origin}/api/merge`, { headers: { 'x-codeboost-token': app.token } })).json() as Promise>; + + it('offers Merge PR while only blockers the job resolves remain, then waits for checks and merges, refusing prepare-merge meanwhile', async () => { + const w = world(); mergeable(w); + let checks: RequiredCheck[] = [{ context: 'ci', appId: null, state: 'pending' }], storeRef: Store | null = null; + const gh = gateway(() => storeRef!, () => checks); + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, preMerge: preparation, mergeGateway: gh, runnerPublished: true, + mergeJobTiming: { checkWaitMs: 20_000, firstDelayMs: 20, maxDelayMs: 50 } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + const before = await review(served.app); + // Not prepared yet, and a required check pends: both are the job's to resolve. + expect(before.merge.blockers.map((blocker: { code: string }) => blocker.code)).toEqual(expect.arrayContaining(['preparation', 'check'])); + expect(before.merge).toMatchObject({ ready: true, action: 'merge', job: null }); + const started = await merge(served.app, before.token); + expect(started.status).toBe(200); + expect(started.body.job).toMatchObject({ state: 'running' }); + expect(started.body.merge).toMatchObject({ ready: false, job: { state: 'running' } }); + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ step: 'checks', checks: [{ context: 'ci', state: 'pending' }] }), { timeout: 10_000 }); + expect(await act(served.app, 'prepare-merge')).toMatchObject({ status: 409, body: { error: expect.stringMatching(/merge is running/) } }); + checks = [{ context: 'ci', appId: null, state: 'success' }]; + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ state: 'merged' }), { timeout: 10_000 }); + expect(gh.merged).toEqual([served.store.getSnapshot(served.identity).head]); + expect(served.store.getTask(served.identity).status).toBe('merged'); + }, 60_000); + + it('admits issue trust before a merge job starts: an untrusted click is refused and replayed, a failed read saves nothing', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, access: 'untrusted' | 'unreadable' | 'trusted' = 'trusted', reads = 0, prepared = 0; + const gh = gateway(() => storeRef!, () => []); + const gate: IssueTrustGateway = { ...issueGateway, async issueAccess(number) { + reads++; + if (access === 'unreadable') throw new Error('GitHub returned HTTP 502'); + return access === 'trusted' ? { number, authorLogin: 'member', collaborator: true } : { number, authorLogin: 'outside', collaborator: false }; + } }; + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway: gate, mergeGateway: gh, runnerPublished: true, + preMerge: service => { const real = preparation(service); return { ...real, get active() { return false; }, get last() { return null; }, + start: (expected: { actionId: string }) => { prepared++; return (real.start as (value: unknown) => unknown)(expected); } } as unknown as PreMergeCoordinator; } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + const { token } = await review(served.app), id = randomUUID(); + // A failed read decides nothing: nothing is saved, so the same click may be sent again. + access = 'unreadable'; + expect((await merge(served.app, token, id)).status).toBe(502); + expect(served.store.getMergeJob(served.identity, id)).toBeNull(); + access = 'untrusted'; + expect(await merge(served.app, token, id)).toMatchObject({ status: 409, body: { error: expect.stringMatching(/not trusted/) } }); + expect(served.store.getMergeJob(served.identity, id)).toMatchObject({ refused: true }); + // The resend replays the refusal without another read, even once the issue is trusted. + access = 'trusted'; const before = reads; + expect(await merge(served.app, token, id)).toMatchObject({ status: 409, body: { error: expect.stringMatching(/not trusted/) } }); + expect(reads).toBe(before); + expect(prepared).toBe(0); + expect((await mergeState(served.app)).job).toBeNull(); + // A new click on a trusted issue starts the job. + expect((await merge(served.app, token)).status).toBe(200); + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ state: 'merged' }), { timeout: 10_000 }); + expect(prepared).toBe(1); + }, 60_000); + + it('answers 503 and saves nothing when shutdown stops a merge click\'s trust read', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, hang = false, reading = false; + const gh = gateway(() => storeRef!, () => []); + const gate: IssueTrustGateway = { ...issueGateway, issueAccess(number, options = {}) { + if (!hang) return Promise.resolve({ number, authorLogin: 'member', collaborator: true }); + reading = true; + // GitHub does not answer; only the request's own abort (at the end of shutdown's drain) ends the read. + return new Promise((_resolve, reject) => options.signal?.addEventListener('abort', () => reject(options.signal!.reason), { once: true })); + } }; + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway: gate, preMerge: preparation, mergeGateway: gh, runnerPublished: true }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + const { token } = await review(served.app), id = randomUUID(), database = served.app.service.config.database; + hang = true; + const clicked = merge(served.app, token, id); + await vi.waitFor(() => expect(reading).toBe(true), { timeout: 10_000 }); + const closed = served.close(); + expect(await clicked).toMatchObject({ status: 503 }); + await closed; + const reopened = new Store(database); + try { expect(reopened.getMergeJob(served.identity, id)).toBeNull(); } finally { reopened.close(); } + }, 60_000); + + it('answers 503 when shutdown stops the reload after a merge job was saved, keeping the saved job for a resend', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, hanging = false; + // From the click on, GitHub does not answer; only the caller's abort ends each read. + const gh = gateway(() => storeRef!, () => [], () => hanging); + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, preMerge: preparation, mergeGateway: gh, runnerPublished: true }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + const { token } = await review(served.app), id = randomUUID(), database = served.app.service.config.database; + hanging = true; + const clicked = merge(served.app, token, id); + await vi.waitFor(() => expect(served.store.getMergeJob(served.identity, id)).not.toBeNull(), { timeout: 10_000 }); + await vi.waitFor(() => expect(gh.hung.length).toBeGreaterThan(0), { timeout: 10_000 }); + const closed = served.close(); + expect(await clicked).toMatchObject({ status: 503 }); + await closed; + // The job was saved under the click's action ID, which a resend replays (merge-job.test.ts covers the replay). + const reopened = new Store(database); + try { expect(reopened.getMergeJob(served.identity, id)).toMatchObject({ id }); } finally { reopened.close(); } + }, 60_000); + + it('answers a merge click with the job\'s latest record when it ended during the click\'s reload', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, slow = false; + const base = gateway(() => storeRef!, () => []); + // The click's reload reads GitHub's display status (not fresh) slowly; the job's own fresh inspections answer at once. + const gh: MergeGateway = { merge: base.merge, inspect: async request => { + if (slow && !request?.fresh) await new Promise(resolve => setTimeout(resolve, 1_500)); + return base.inspect(request); + } }; + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, preMerge: preparation, mergeGateway: gh, + runnerPublished: true, mergeJobTiming: { checkWaitMs: 20_000, firstDelayMs: 20, maxDelayMs: 50 } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + const { token } = await review(served.app), id = randomUUID(); + slow = true; + const clicked = await merge(served.app, token, id); + // The job merged while the click's response was being prepared: the response says so, not "preparing". + expect(served.store.getMergeJob(served.identity, id)).toMatchObject({ state: 'merged' }); + expect(clicked).toMatchObject({ status: 200, body: { job: { id, state: 'merged' } } }); + }, 60_000); + + it('closes the pull requests of a task whose cancel waited for the merge job\'s preparation to settle', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, release!: () => void, preparing = false; + const held = new Promise(resolve => { release = resolve; }); + const gh = gateway(() => storeRef!, () => []); + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, mergeGateway: gh, runnerPublished: true, + // A preparation caught in its push: it holds a real push marker, so a cancel waits ("stopping") until the push + // settles, and clearing the marker closes the task, as production does. + preMerge: service => { + const identity = service.config.identity; + let marker: { attemptId: string } | null = null; + return { + get active() { return preparing; }, get last() { return null; }, assertStartable() {}, settlePendingPush: async () => null, close: async () => undefined, + cancelTask: (stateVersion: number, actionId: string) => service.store.cancelTask(identity, stateVersion, actionId), + start: async (expected: { actionId: string }) => { + preparing = true; + const reviewed = () => ({ revision: service.store.getPlan(identity).revision, snapshotId: service.store.getSnapshot(identity).id, reviewVersion: service.store.reviewVersion(identity) }); + const head = service.store.getSnapshot(identity).head, rewritten = 'e'.repeat(40); + service.store.recordRebase(identity, reviewed(), 'f'.repeat(40), rewritten, [{ oldSha: head, newSha: rewritten }]); + marker = service.store.beginPrePush(identity, reviewed(), service.store.getTask(identity).stateVersion, + { branch: service.store.taskPullRequests(identity)[0]!.headBranch, from: head, to: rewritten }); + await held; + service.store.finishPrePush(service.store.getTask(identity).planKey, marker.attemptId); + preparing = false; + const snapshot = service.store.getSnapshot(identity); + return service.store.settlePreMergeAction(identity, expected.actionId, + { state: 'failed', base: snapshot.base, head: snapshot.head, checked: [], reason: 'Task cancelled.' }, null); + }, + } as unknown as PreMergeCoordinator; + } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + expect((await merge(served.app, (await review(served.app)).token)).status).toBe(200); + await vi.waitFor(() => expect(served.store.getTask(served.identity).pushInProgress).not.toBeNull(), { timeout: 10_000 }); + expect((await act(served.app, 'cancel-task')).body.result).toEqual({ outcome: 'stopping' }); + expect(served.store.getTask(served.identity)).toMatchObject({ status: 'in review', cancelRequested: expect.any(String) }); + expect(w.github.prs[0]!.open).toBe(true); + release(); + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ state: 'cancelled' }), { timeout: 10_000 }); + await vi.waitFor(() => expect(w.github.prs[0]!.open).toBe(false), { timeout: 20_000 }); + expect(served.store.getTask(served.identity).status).toBe('cancelled'); + }, 60_000); + + it('stops a waiting merge job promptly on shutdown and leaves it running on disk for the next startup', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, hanging = false; + const gh = gateway(() => storeRef!, () => [{ context: 'ci', appId: null, state: 'pending' }], () => hanging); + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, preMerge: preparation, mergeGateway: gh, + runnerPublished: true, mergeJobTiming: { checkWaitMs: 600_000, firstDelayMs: 30, maxDelayMs: 30 } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + expect((await merge(served.app, (await review(served.app)).token)).status).toBe(200); + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ step: 'checks' }), { timeout: 10_000 }); + // GitHub stops answering the job's next inspection; only the job's own abort can end that read. + hanging = true; + await vi.waitFor(() => expect(gh.hung).toHaveLength(1), { timeout: 10_000 }); + const database = served.app.service.config.database, closing = Date.now(); + const closed = served.close(); + // Shutdown's first synchronous turn stops the job, before any drain, so it cannot ask GitHub to merge meanwhile. + expect(gh.hung[0]!.aborted).toBe(true); + await closed; + expect(Date.now() - closing).toBeLessThan(10_000); + const reopened = new Store(database); + try { expect(reopened.latestMergeJob(served.identity)).toMatchObject({ state: 'running', step: 'checks' }); } finally { reopened.close(); } + }, 60_000); + + it('closes the merge job before the store, so a job settling after the shutdown abort records its outcome', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null, merging: AbortSignal | null = null; + const base = gateway(() => storeRef!, () => []); + // GitHub was asked to merge; after the abort it answers, a little later, with a refusal. + const gh: MergeGateway = { inspect: base.inspect, merge: (_head, options) => new Promise((_resolve, reject) => { + merging = options!.signal!; + merging.addEventListener('abort', () => setTimeout(() => reject(new MergeSubmissionError('Base branch was modified.', 'refused')), 300), { once: true }); + }) }; + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, preMerge: preparation, mergeGateway: gh, + runnerPublished: true, mergeJobTiming: { checkWaitMs: 20_000, firstDelayMs: 20, maxDelayMs: 50 } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + expect((await merge(served.app, (await review(served.app)).token)).status).toBe(200); + await vi.waitFor(() => expect(merging).not.toBeNull(), { timeout: 10_000 }); + const database = served.app.service.config.database, order: string[] = []; + const jobClose = MergeJobCoordinator.prototype.close, storeClose = Store.prototype.close; + const jobs = vi.spyOn(MergeJobCoordinator.prototype, 'close').mockImplementation(async function (this: MergeJobCoordinator) { + await jobClose.call(this); order.push('job settled'); + }); + const stores = vi.spyOn(Store.prototype, 'close').mockImplementation(function (this: Store) { order.push('store closed'); return storeClose.call(this); }); + try { await served.close(); } finally { jobs.mockRestore(); stores.mockRestore(); } + // Shutdown closed the job before the store (close awaiting the job is shown in merge-job.test.ts), and the job's last + // write landed: it ended as its merge attempt records it. + expect(order[0]).toBe('job settled'); + expect(order).toContain('store closed'); + const reopened = new Store(database); + try { + expect(reopened.latestMergeJob(served.identity)).toMatchObject({ state: 'blocked', reason: expect.stringMatching(/^GitHub refused the merge/) }); + expect(reopened.getTask(served.identity).status).toBe('approved but merge blocked'); + } finally { reopened.close(); } + }, 60_000); + + it('stops a waiting merge job when the task is cancelled, and closes the task\'s pull requests', async () => { + const w = world(); mergeable(w); + let storeRef: Store | null = null; + const gh = gateway(() => storeRef!, () => [{ context: 'ci', appId: null, state: 'pending' }]); + const served = await serve(w, { before: service => { completeAll(service); acceptAndApprove(service); }, issueGateway, preMerge: preparation, mergeGateway: gh, runnerPublished: true, + // Waits far longer than this test: only the cancel route's own stop can end the job in time. + mergeJobTiming: { checkWaitMs: 600_000, firstDelayMs: 60_000, maxDelayMs: 60_000 } }); + storeRef = served.store; createIdentity = () => served.identity; + await publishSettled(served.app, served.identity); + expect((await merge(served.app, (await review(served.app)).token)).status).toBe(200); + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ step: 'checks' }), { timeout: 10_000 }); + expect((await act(served.app, 'cancel-task')).body.result).toEqual({ outcome: 'closed' }); + await vi.waitFor(async () => expect((await mergeState(served.app)).job).toMatchObject({ state: 'cancelled' }), { timeout: 5_000 }); + await vi.waitFor(() => expect(w.github.prs[0]!.open).toBe(false), { timeout: 20_000 }); + expect(gh.merged).toEqual([]); + }, 60_000); +}); diff --git a/test/store.test.ts b/test/store.test.ts index 18cbf060..4ea407eb 100644 --- a/test/store.test.ts +++ b/test/store.test.ts @@ -121,7 +121,7 @@ it('scopes issue trust to repository and current author, supports revoke, and mi legacy.exec('DROP TABLE issue_trust; ALTER TABLE attempts DROP COLUMN prompt_comments; PRAGMA user_version=16;'); legacy.close(); const migrated = open(path), db = new DatabaseSync(path); try { - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 18 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 19 }); expect(db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='issue_trust'").get()).toEqual({ name: 'issue_trust' }); expect(db.prepare('PRAGMA table_info(attempts)').all().some(column => column.name === 'prompt_comments')).toBe(true); expect(migrated.issueTrust('owner/a', 5)).toBeNull(); diff --git a/web/public/app.js b/web/public/app.js index a79f8c54..86e55694 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -158,7 +158,29 @@ function renderMerge() { if (!merge?.available) return; // Once the attempt this retained key started has ended, a retry is a new action and needs a new key. if (mergeAction && merge.queue?.actionId === mergeAction.actionId && ["merged", "removed", "failed"].includes(merge.queue.state)) mergeAction = null; - $("merge").disabled = !merge.ready; + // Likewise once the merge job this key started has ended (a job is saved under its click's action ID), or another job is + // the latest: one job runs at a time, so this key's job has ended or never started. Either way a click is a new action. + if (mergeAction && merge.job && !(merge.job.id === mergeAction.actionId && merge.job.state === "running")) mergeAction = null; + // With merge jobs (their status carries `job`), a click's answer arrives while the control may hold focus, so it is + // marked, never disabled; activation is ignored while the merge is not ready. + if ("job" in merge) { + $("merge").disabled = false; + if (merge.ready) $("merge").removeAttribute("aria-disabled"); + else $("merge").setAttribute("aria-disabled", "true"); + } else { + $("merge").disabled = !merge.ready; + $("merge").removeAttribute("aria-disabled"); + } + const job = merge.job; + if (job?.state === "running") { + // Keep the control focusable while its job runs; repeat activation is ignored because the merge is not ready. + $("merge").setAttribute("aria-disabled", "true"); + $("merge").textContent = { preparing: "Preparing…", checks: "Waiting for checks…", merging: "Merging…" }[job.step] ?? "Merging…"; + $("merge-details").hidden = false; + $("merge-details").textContent = "Merge status"; + scheduleMergePoll(); + return; + } $("merge").textContent = merge.action === "retry" ? "Retry merge" : merge.queue?.state === "submitting" @@ -177,8 +199,9 @@ function scheduleMergePoll() { if (mergePollTimer) clearTimeout(mergePollTimer); mergePollTimer = null; const queue = data?.merge?.queue; - const state = queue?.state; - if (queue?.kind === "queue" && ["submitting", "queued"].includes(state)) { + const job = data?.merge?.job; + const state = job?.state === "running" ? `job:${job.step}` : queue?.state; + if (job?.state === "running" || (queue?.kind === "queue" && ["submitting", "queued"].includes(state))) { if (state !== mergePollState) { mergePollState = state; mergePollDelay = 2000; @@ -197,6 +220,25 @@ async function pollMergeQueue(generation) { try { const update = await api("/api/merge"); if (generation !== mergeGeneration || !data?.merge?.available) return; + if (data.merge.job?.state === "running") { + // A merge job (F6b): follow its step. Once it ends, update only the merge control, as the merge-queue poll does: + // a background reload would erase the person's selection and input; Refresh shows the outcome. + const job = update.job; + if (job?.state === "running") { + const changed = job.step !== data.merge.job.step || JSON.stringify(job.checks) !== JSON.stringify(data.merge.job.checks); + data = { ...data, merge: { ...data.merge, job } }; + $("banner").textContent = mergeJobText(job); + if (!changed) backOffMergePoll(); + renderMerge(); + return; + } + const ended = `${job ? mergeJobText(job) : "The merge job ended."} Refresh to see the result.`; + const blockers = data.merge.blockers.filter(blocker => !mergeLifecycleBlockers.has(blocker.code)); + data = { ...data, merge: { ...data.merge, ready: false, action: null, job, blockers: [{ code: "job-refresh", message: ended }, ...blockers] } }; + $("banner").textContent = ended; + renderMerge(); + return; + } const queue = update.queue; mergeObservationGeneration++; data = withMergeQueue(data, queue); @@ -205,12 +247,24 @@ async function pollMergeQueue(generation) { renderMerge(); } catch (error) { if (generation !== mergeGeneration || !data?.merge?.available) return; - $("banner").textContent = `Could not refresh merge-queue status. ${error.message}`; + $("banner").textContent = `Could not refresh merge status. ${error.message}`; backOffMergePoll(); scheduleMergePoll(); } } -const mergeLifecycleBlockers = new Set(["merge-submitted", "queue-active", "queue-head", "queue-merged", "queue-refresh", "stale-merge"]); +function mergeJobText(job) { + if (job.state === "running") return job.step === "checks" + ? `Waiting for required checks${job.checks.length ? `: ${job.checks.map(check => `${check.context} is ${check.state}`).join(", ")}` : ""}.` + : job.step === "merging" ? "Merging the reviewed head." : "Preparing the merge: refresh, rebase, push and command checks."; + return { + merged: "Merged.", queued: "Merge queued. Waiting for GitHub to confirm the outcome.", + submitted: `Merge submitted; GitHub's outcome is not confirmed yet. ${job.reason ?? ""}`.trim(), + blocked: `Approved, merge blocked. ${job.reason ?? ""} Fix it, then Retry merge.`.trim(), + "review-required": `Review needed before merging. ${job.reason ?? ""}`.trim(), + failed: `Merge stopped. ${job.reason ?? ""} Merge again when ready.`.trim(), cancelled: "Merge cancelled.", + }[job.state] ?? job.state; +} +const mergeLifecycleBlockers = new Set(["merge-submitted", "queue-active", "queue-head", "queue-merged", "queue-refresh", "stale-merge", "job-refresh"]); function withMergeQueue(view, queue) { const blockers = view.merge.blockers.filter(blocker => !mergeLifecycleBlockers.has(blocker.code)); if (queue?.state === "merged") @@ -522,7 +576,12 @@ $("approve").onclick = () => { $("reload").onclick = refresh; $("merge-details").onclick = () => { if (!data?.merge?.available) return; - const queue = data.merge.queue; + const queue = data.merge.queue, job = data.merge.job; + if (job?.state === "running") { + showDialog(`

Merge status

${esc(job.step ?? job.state)}

${esc(mergeJobText(job))}

${job.checks.length + ? `
    ${job.checks.map(check => `
  • ${esc(check.context)}: ${esc(check.state)}
    • `).join("")}
` : ""}`); + return; + } showDialog( `

${queue ? "Merge status" : "Merge blockers"}

${queue ? `

${esc(queue.state)} · reviewed head ${esc(queue.reviewedHead.slice(0, 12))}

${queue.phase ? `

GitHub phase: ${esc(queue.phase)}${queue.position === null ? "" : ` · position ${queue.position}`}

` : ""}${queue.reason ? `

${esc(queue.reason)}

` : ""}${queue.observationError ? `

${esc(queue.observationError)}

` : ""}` : ""}
    ${data.merge.blockers.map((blocker) => `
  • ${esc(blocker.message)}
    • `).join("")}
`, ); @@ -535,12 +594,23 @@ $("merge").onclick = async () => { mergeGeneration++; mergePollState = null; mergePollDelay = 2000; - $("merge").disabled = true; + // With merge jobs, the click returns at once and the focused control must keep focus: mark it, do not disable it. + if (data.merge && "job" in data.merge) $("merge").setAttribute("aria-disabled", "true"); + else $("merge").disabled = true; try { rememberDraft(); mergeAction ??= { actionId: crypto.randomUUID(), token: data.token }; const updated = await api("/api/action", { action: "merge", token: mergeAction.token, actionId: mergeAction.actionId }); + // The composer stayed editable while the request was in flight: keep what the person typed meanwhile. + rememberDraft(); mergeAction = null; + if (updated.job) { + // A merge job runs in the background (F6b); the page follows it. + data = { ...updated, merge: { ...updated.merge, job: updated.job } }; + render(); + $("banner").textContent = mergeJobText(updated.job); + return; + } const queued = updated.mergeQueue?.state === "queued" || updated.mergeQueue?.state === "submitting"; const blocker = { code: queued ? "queue-active" : "merge-submitted", message: queued ? "The reviewed head is queued. Waiting for GitHub to confirm the outcome." : "Merge was submitted. Refresh to confirm GitHub state." }; data = updated.mergeRefreshRequired @@ -549,6 +619,7 @@ $("merge").onclick = async () => { render(); $("banner").textContent = `${queued ? "Merge queued" : "Merge submitted"}. ${updated.mergeResult.url}`; } catch (error) { + rememberDraft(); // Only a parsed, definite server answer resolves this click. No response, an unreadable body, 503 (nothing // applied) or an admitted attempt with an unknown outcome keeps the key until that attempt ends. if (typeof error.status === "number" && error.status !== 503 && !error.outcomeUnknown) mergeAction = null; diff --git a/web/public/style.css b/web/public/style.css index f767008a..1272ef7d 100644 --- a/web/public/style.css +++ b/web/public/style.css @@ -100,7 +100,7 @@ button[aria-disabled="true"] { } button.primary, #approve, -#merge:not(:disabled) { +#merge:not(:disabled):not([aria-disabled="true"]) { background: var(--primary); color: var(--on-primary); border-color: var(--primary); @@ -112,7 +112,7 @@ button.primary[aria-disabled="true"] { border-color: var(--line); } #approve:hover, -#merge:not(:disabled):hover { +#merge:not(:disabled):not([aria-disabled="true"]):hover { background: var(--hover); } [hidden] { diff --git a/web/server.ts b/web/server.ts index 6012081c..dea583cf 100644 --- a/web/server.ts +++ b/web/server.ts @@ -22,6 +22,7 @@ import type { AuthorProvider } from '../core/planning-author.ts'; import { PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; import { MAX_SOURCE_BYTES } from '../core/parse-v1.ts'; import type { PreMergeCoordinator } from '../runner/pre-merge.ts'; +import { MergeJobCoordinator, withMergeJob, type MergeJobTiming } from '../runner/merge-job.ts'; export type PlanningDescription = Pick & { repo: { name: string; baseRef: string }; /** Revalidates any mutable authority carried by this description at the synchronous prompt-construction boundary. */ @@ -68,12 +69,16 @@ export const RUNNER_NOT_IN_DEMO = 'Demos do not run the runner. Use a review con export interface ServerTestHooks { /** Test-only gate for forcing a response lifecycle transition before the Issues wait is armed. */ beforeIssueRefreshWait?(): Promise | void; + /** Shorter required-check waits for tests of the one-click merge (F6b). */ + mergeJobTiming?: MergeJobTiming; } export async function startServer(config: ReviewConfig, port = 4318, questionAgent?: QuestionAgent, mergeGateway?: MergeGateway, shutdownDrainMs = MAX_SHUTDOWN_DRAIN_MS, issueGateway?: IssueGateway, runnerDeps?: RunnerDeps, planningInput?: PlanningDeps | PlanningSetup, runnerSetup?: RunnerSetup, testHooks: ServerTestHooks = {}) { if (!Number.isSafeInteger(shutdownDrainMs) || shutdownDrainMs < 1 || shutdownDrainMs > MAX_SHUTDOWN_DRAIN_MS) throw new Error('Invalid shutdown drain deadline.'); const service = new ReviewService(config), token = randomBytes(32).toString('hex'); let questions: Questions, merges: MergeCoordinator | null, issues: IssueBoard, runner: RunnerCoordinator | null, suggestions: SuggestionCoordinator | null; let preMerge: PreMergeCoordinator | null = null; + /** The one-click merge (F6b of #22); only with a runner-published PR, which requires preparation. */ + let mergeJobs: MergeJobCoordinator | null = null; let planning: PlanningDeps | undefined, issueSource: IssueGateway | null; /** Runs a task's plan items; one per Store, like the coordinator. Only the production runner has one. */ let executor: ItemExecutor | null = null; @@ -127,6 +132,8 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge validate: () => requireTrustedIssue(access), }; }); + if (preMerge && merges && merges.published?.requiresPreparation) + mergeJobs = new MergeJobCoordinator(service, preMerge, merges, capability, testHooks.mergeJobTiming); executor = new ItemExecutor(service.store, runner, assembly.sources, assembly.findings, { capability }); // A demo never publishes, whatever its github block or an injected setup provides (setUpRunner refuses demos too). if (assembly.publisher && !config.demo) { const coordinator = runner; publishing = new TaskPublishing(service.store, assembly.publisher(() => coordinator.closing), runner, executor, capability, assembly.env, { @@ -139,7 +146,13 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge } catch (error) { service.close(); throw error; } } const loadReview=()=>{const view=service.load();return {...view,notes:view.notes.map(note=>({...note,answerActive:questions.isRunning(note.id)}))};}; - const load=async(signal?:AbortSignal)=>{const view=loadReview();return {...view,merge:merges?await merges.displayStatus(view,signal):{available:false}};}; + const mergeView=async(view:ReturnType,signal?:AbortSignal)=>{ + if(!merges)return {available:false}; + const status=await merges.displayStatus(view,signal); + // With a runner, Merge PR starts a merge job, which itself prepares and waits for required checks (F6b of #22). + return mergeJobs?withMergeJob(status,mergeJobs.latest(),service.store.getTask(identity).status):status; + }; + const load=async(signal?:AbortSignal)=>{const view=loadReview();return {...view,merge:await mergeView(view,signal)};}; const answerStatuses=()=>service.store.getReviewNotes(config.identity) .filter(note=>note.kind==='question') .map(note=>({id:note.id,answer:note.answer,answerActive:questions.isRunning(note.id)})); @@ -408,6 +421,8 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge service.store.afterCommit(() => afterPreMerge(preMerge!.settlePendingPush())); return { outcome: 'stopping' as const }; } + // A merge job stops its wait at once; its preparation and guarded merge stop through their own routes. + if (mergeJobs?.active) service.store.afterCommit(() => mergeJobs!.cancel()); const outcome = preMerge?.active ? preMerge.cancelTask(expectedStateVersion as number, actionId as string) : runner ? runner.cancelTask(identity, expectedStateVersion as number, actionId as string) @@ -422,6 +437,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (action === 'prepare-merge') { if (access) requireTrustedIssue(access); if (!preMerge) throw new GuardRefusal('Pre-merge preparation is not configured.'); + if (mergeJobs?.active) throw new GuardRefusal('A merge is running for this task; it prepares the merge itself.'); if (runner.isActive(identity) || executor?.busy(identity) || publishing?.busy(identity)) throw new GuardRefusal('The runner or publisher is busy; pre-merge preparation cannot start yet.'); preMerge.assertStartable(); @@ -609,7 +625,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (stopping) { json(503, { error: 'The review server is shutting down.' }); return; } if (req.method === 'GET' && path === '/api/settings') { json(200,{questionProvider:service.store.questionProvider()});return; } if (req.method === 'GET' && path === '/api/questions') { json(200,{notes:answerStatuses()});return; } - if (req.method === 'GET' && path === '/api/merge') { if(!merges)throw new Error('Merging is not configured for this review.');json(200,{queue:await merges.pollQueue()});return; } + if (req.method === 'GET' && path === '/api/merge') { if(!merges)throw new Error('Merging is not configured for this review.');json(200,{queue:await merges.pollQueue(),job:mergeJobs?.latest()??null});return; } if (req.method === 'GET' && path === '/api/issues') { json(200, issues.view()); return; } if (req.method === 'GET' && path === '/api/review') { json(200, await load(requestAbort.signal)); return; } if (req.method === 'GET' && path === '/api/runner') { json(200, runnerView()); return; } @@ -689,6 +705,32 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // Every merge click carries its idempotency key; a request without one would bypass replay. if(!isUuidV4(input.actionId)) { json(400,{error:'A merge request needs a UUID v4 actionId.'}); return; } if(!merges)throw new Error('Merging is not configured for this review.'); + // With a runner, one click starts the whole merge job (F6b of #22) and returns at once; the browser follows it + // through GET /api/merge. + if(mergeJobs) { + // Trust is admitted before the job starts, as for prepare-merge: an untrusted issue's click is refused, and the + // refusal saved. A failed read decides nothing: it saves nothing, and the same click may be sent again. + let untrusted: string|null=null; + if(config.github&&!mergeJobs.saved(input.actionId)) { + if(stopping||runner?.closing)throw new ShuttingDownError(); + let access: IssueAccess; + // A read stopped by shutdown decides nothing: 503, and the browser resends the same click. + try { access=await readIssueAccess(requestAbort.signal); } + catch (error) { if(stopping||runner?.closing||requestAbort.signal.aborted)throw new ShuttingDownError(); throw error; } + try { requireTrustedIssue(access); } catch (error) { if(!(error instanceof GuardRefusal))throw error; untrusted=error.message; } + } + const job=mergeJobs.start(input.token,input.actionId,()=>untrusted??(runner?.isActive(identity)||executor?.busy(identity)||publishing?.busy(identity) + ?'The runner or publisher is busy; the merge cannot start yet.':null)); + const running=mergeJobs.current; + if(running)void running.finally(()=>{ if(publishing&&!stopping&&service.store.getTask(identity).status==='cancelled')publishing.taskCancelled(identity); }); + // The job is saved; a reload stopped by shutdown answers 503, and the resent click replays the job. + let loaded: Awaited>; + try { loaded=await load(requestAbort.signal); } + catch (error) { if(stopping||runner?.closing||requestAbort.signal.aborted)throw new ShuttingDownError(); throw error; } + // The job may have moved on (or ended) while the review reloaded: answer its latest record, not the admission's. + const latest=mergeJobs.latest(); + json(200,{...loaded,job:latest?.id===job.id?latest:job});return; + } let merged: Awaited>; // A merge stopped for a passing reason applied nothing, so answer 503 and the browser resends the same key. try { merged=await merges.merge(input.token,input.actionId); } @@ -768,6 +810,8 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge url: `http://127.0.0.1:${address.port}/#${token}`, close: async () => { // Step 1, one synchronous turn: reject new API requests and new runner work. Admitted requests drain (step 2). stopping = true; + // Same turn: a merge job (F6b) stops before it can ask GitHub to merge; its settlement is awaited below. + mergeJobs?.stop(); // Same turn as the admission flag: a request already reading its body must not start a new Ask worker. questions.stopAdmission(); runner?.rejectAdmission(); @@ -789,7 +833,8 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // whole pre-merge lifecycle before the write gate closes, so those settlement writes cannot be refused. const failures: unknown[] = publishingFailure === undefined ? [] : [publishingFailure]; const step = async (run: () => Promise | unknown) => { try { await run(); } catch (error) { failures.push(error); } }; - await step(() => preMerge?.close()); + // A merge job awaits its preparation, so both stop together: the job is aborted while preparation is. + await step(() => Promise.all([mergeJobs?.close(), preMerge?.close()])); // Step 3: after the drain, close the Store write gate. A request-path write still pending after the abort // (for example merge reconciliation after a GitHub await) now fails with 503; settling coordinators keep the capability. service.store.closeWrites();