diff --git a/README.md b/README.md index 41d32414..3652ba19 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ A configuration with a `github` block may also add a `runner` block. The runner - `diagnosticsCapBytes`: the size retention trims that directory back to. It is a target, not a hard limit: the file just saved is always kept, even when it alone passes the cap. The default is 256 MiB. - `limits`: task storage limits (`workBytes`, `workInodes`, `metadataBytes`, `metadataInodes`). -The runner needs Docker and `CLAUDE_CODE_OAUTH_TOKEN`. At start, before the server opens, codeboost recovers what an earlier run left and builds the agent image; a recovery it cannot finish safely stops startup with what to do. `POST /api/runner` with `start` or `resume` runs the plan; the review screen has no buttons for them yet. When a run has completed every item, codeboost pushes the task head to a `codeboost/…` branch with your `gh` credentials and opens a ready pull request into `baseBranch`; a task that needs a person gets a draft pull request with its problems. A publish that was refused (for example, the branch holds a commit codeboost did not make) is retried with the `publish` action, and `GET /api/runner` shows the last outcome under `publish`. Demos never publish. +The runner needs Docker and `CLAUDE_CODE_OAUTH_TOKEN`. At start, before the server opens, codeboost recovers what an earlier run left and builds the agent image; a recovery it cannot finish safely stops startup with what to do. `POST /api/runner` with `start` or `resume` runs the plan; the review screen has no buttons for them yet. When a run has completed every item, codeboost pushes the task head to a `codeboost/…` branch with your `gh` credentials and opens a ready pull request into `baseBranch`; a task that needs a person gets a draft pull request with its problems. A publish that was refused (for example, the branch holds a commit codeboost did not make) is retried with the `publish` action, and `GET /api/runner` shows the last outcome under `publish`. When a task is cancelled, codeboost closes the open pull requests it opened for it (the branch is kept). A pull request a person moved is left open and reported, and `close-pull-requests` retries a close that failed. Demos never publish. ## Library diff --git a/docs/architecture.md b/docs/architecture.md index d0c9d95c..2db2cc8c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -167,7 +167,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Merge coordinator | `runner/merge.ts` | Runs the full merge gate twice, re-reads the local generation, then merges the exact reviewed head. Tracks merge-queue attempts. | Yes | | Runner coordinator | `runner/coordinator.ts`, `runner/lifecycle.ts` | Attempt admission, concurrency slots, compare-and-swap on results, retries, shutdown order. | Not connected | | Execution | `runner/execution.ts` | Runs plan items in order: fresh workspace, prompt, agent, post-run audit, then the runner's own commit and ledger entry. Pauses in needs amendment on an out-of-scope edit; moves to needs human on a safety violation. | Not connected; uses a test workspace until #66 part 2 and #87 | -| Publishing | `runner/publish.ts`, `runner/publishing.ts`, `runner/branch-push.ts` | Runs the already-fixed check, pushes the task head, then opens or reuses the task's pull request (a draft when the task needs a person). Records each opening before calling GitHub, so a lost outcome can be found again by a marker. `TaskPublishing` (#103) publishes when a run ends, on the `publish` runner action, and once at startup, and records the last outcome. | With a runner block (needs `github.baseBranch`); never in a demo | +| Publishing | `runner/publish.ts`, `runner/publishing.ts`, `runner/branch-push.ts` | Runs the already-fixed check, pushes the task head, then opens or reuses the task's pull request (a draft when the task needs a person). Records each opening before calling GitHub, so a lost outcome can be found again by a marker. `TaskPublishing` (#103) publishes when a run ends, on the `publish` runner action, and once at startup, and records the last outcome. On cancel it closes the task's PRs (#111). | With a runner block (needs `github.baseBranch`); never in a demo | | Lock and startup recovery | `runner/recovery.ts` | One runner per database, held as an OS lock on the database file's device and inode. Startup recovery finalizes interrupted attempts and removes leftover containers and storage. | Lock in use; recovery not connected | | Store | `runner/store.ts` | The only database handle and all SQL. Revision numbers, snapshots, ledger, approvals, choices, notes, suggestion requests, tasks, attempts, user actions, feedback events, merge attempts, pull request openings, already-fixed results. | In use | | Plan logic | `core/plan.ts`, `core/parse-v1.ts`, `schema/` | Schema validation, YAML and JSON import, meaning checks (IDs, dependencies, paths, projected file operations), literal command parsing, suggestion edits. | Yes | diff --git a/docs/implementation/pull-request-opening.md b/docs/implementation/pull-request-opening.md index 77720677..878e805e 100644 --- a/docs/implementation/pull-request-opening.md +++ b/docs/implementation/pull-request-opening.md @@ -58,7 +58,7 @@ Only one publish runs per task at a time; a second one is refused. A publish who - **Its PR is there.** It is recorded as opened, wherever it is. If this was a draft opening, this publish is a draft publish too, and the PR has since been made ready, it is turned back into a draft first: a failure keeps the opening owned, except that drafts being unsupported is definite, so the PR is recorded as it is and publishing continues: the main path returns `draft unsupported`, or refuses a PR it would not accept. The recovered opening ends this publish only when it is this publish's own work (same task state version, same review version, same draft mode) and the main path would accept its PR (in the configured base, the task's only open PR). Otherwise publishing continues: the main path pushes the current head and brings the PR into the current mode, or, for a PR it would not accept, makes all the task's PRs drafts and refuses (step 3). - **Another of the task's PRs is open into the base this opening asked for.** This opening created nothing (GitHub allows one open PR per branch and base), so it is marked `abandoned`. If that other PR's opening was abandoned too, the main path (a running task) or the draft step (a stopped one) adopts it next. - **Nothing.** The request may still be in flight or not yet visible, so publish stops with `OpeningUnsettled` until the opening is 10 minutes old (`settleMs`). After that, it marks the opening `abandoned` and continues. - **Draft what the task cannot keep ready.** Then, if the task is not in review, approved but merge blocked, or merged, and cannot be published as ready now (cancelled, needs human, possibly already fixed, an attempt active, requeue pending, rebase or merge in progress), every branch with a PR recorded as ready, or with an abandoned opening, is looked up. An abandoned opening's PR that has appeared since is adopted, because the main path, which also adopts it, may refuse first. A task in review, approved or merged has no such step, and publish refuses it on status: recovery still records its lost opening's own PR, but a late PR of another abandoned opening is not adopted or drafted for it. A ready PR is then made a draft, after re-reading the task: one approved meanwhile keeps its PR ready. An approved task's PR is left alone, because GitHub does not merge a draft. A draft flag that GitHub already shows is only recorded. The record saying "ready" is what makes the draft owed. So an earlier draft change that failed (`leftReady`), and a PR that recovery has just recorded, are both drafted by the next publish of that task, even though step 2 then refuses. Only a publish fulfils a draft owed: until something publishes the task again, its PR stays ready. Nothing in the runner calls publish for a cancelled task yet; closing the PR on cancel is a later slice (see "What this slice does not do"). A GitHub failure here does not replace step 2's refusal. The refusal names the PR that may still be ready, and the next publish tries again. A PR that has since closed keeps its "ready" record, and an abandoned opening stays abandoned, so each later publish of that task looks its branch up again. + **Draft what the task cannot keep ready.** Then, if the task is not in review, approved but merge blocked, or merged, and cannot be published as ready now (cancelled, needs human, possibly already fixed, an attempt active, requeue pending, rebase or merge in progress), every branch with a PR recorded as ready, or with an abandoned opening, is looked up. An abandoned opening's PR that has appeared since is adopted, because the main path, which also adopts it, may refuse first. A task in review, approved or merged has no such step, and publish refuses it on status: recovery still records its lost opening's own PR, but a late PR of another abandoned opening is not adopted or drafted for it. A ready PR is then made a draft, after re-reading the task: one approved meanwhile keeps its PR ready. An approved task's PR is left alone, because GitHub does not merge a draft. A draft flag that GitHub already shows is only recorded. The record saying "ready" is what makes the draft owed. So an earlier draft change that failed (`leftReady`), and a PR that recovery has just recorded, are both drafted by the next publish of that task, even though step 2 then refuses. Only a publish fulfils a draft owed: until something publishes the task again, its PR stays ready. Nothing calls publish for a cancelled task: cancel closes its PRs instead (see "Closing a cancelled task's pull requests"). A GitHub failure here does not replace step 2's refusal. The refusal names the PR that may still be ready, and the next publish tries again. A PR that has since closed keeps its "ready" record, and an abandoned opening stays abandoned, so each later publish of that task looks its branch up again. 2. **Status.** Before any other GitHub call, refuse unless the task is running (or in needs human, for a draft), with no attempt active, no merge active, no interrupted work waiting to be requeued, and no rebase in progress. 3. **Find the branch PR.** Ask GitHub for the branch's open PR every time, with the markers of the task's opened and abandoned openings for that branch, whatever their base (none if the task has no PR yet). GitHub allows one open PR per branch and base, so every lookup lists all open PRs from the branch. The list is one page of up to 100; a full page fails closed, because a later page could hide the task's PR, and every entry is validated before any is set aside as someone else's. Every draft change in recovery, the draft step and for a moved PR re-reads the task right before it, with no await between: a task in review, approved or merged keeps its PR ready, because GitHub does not merge a draft. (The head settle after an open or update is the one exception: it makes the PR a draft when GitHub shows another head than the one pushed, even for a task that has just moved to in review, because that head was not reviewed.) Two rules follow. **Observing and drafting** (recovery and the draft step in step 1) look for the task's own PRs, those carrying its markers, in any base (`findOwned`): what GitHub shows is recorded, and a PR is made a draft, wherever it is, because both are safe anywhere. So a lost update or opening settles, and a stopped task's PRs are all drafted, even after a base change or a retarget. **Changing content** (push, description update, ready change, opening) happens only on the main path and only in the configured base (`findOpened`): it refuses when the task's own PR is in another base (retargeted by a person, or left by a change of the base setting; retarget it or close it) or when two of the task's PRs are open (close all but one), instead of opening a second PR from the same branch. Anyone else's PR from the branch into another base (a backport, say) is ignored. A PR into the configured base without one of the task's markers is refused as not codeboost's, unless its number is one of the task's recorded PRs: then a person removed its marker, and it is refused as misplaced (restore its first line or close it). GitHub's PR list can lag behind a PR, so when the list shows no PR but the record has an opened one, that PR is read directly (one request per recorded PR, closed ones included): if it is still open on the task branch, into the configured base, with its marker, only the list is behind, and publish stops with `OpeningUnsettled` (retry later) and pushes nothing, because a push would move the open PR's head with no update recorded as in flight. If it is open but a person moved it (renamed its branch, removed its marker, retargeted it), a retry would never see it, so publish refuses with `PullRequestMisplaced`: close it, or restore its branch, base and first line. Before refusing, it makes that PR a draft where it is, while its first line still identifies it; otherwise the refusal says it may still be ready. A marker is read from the description's first line with surrounding white space removed, because GitHub returns a description edited on github.com with CRLF line endings. A record's base is only where the PR was opened. A lost opening counts as having created nothing only when another of the task's PRs is open into the base that opening asked for. Moving the task to in review is a change too: recovery does it only for a PR in the configured base with no other of the task's PRs open. Any other recovered PR is recorded, and the main path's misplaced draft step adopts the task's other open PRs, makes them all drafts and refuses; the task stays as it was. When the main path refuses the task's PRs for their place (another base, or two open), it first makes every one of them a draft, so none stays ready while a person decides. An open PR that carries none of them was not opened by codeboost: publish refuses before anything is pushed. A PR with an opened record's marker but another number is refused too. An abandoned opening whose PR is now visible is adopted here (number, URL and draft state recorded, task status unchanged), whatever the check then says, so the record names every PR the task has on GitHub. When the PR's draft state on GitHub differs from the record, the record is corrected here (for example after a draft change whose record was lost), under the task's state-version guard. This comes before the check: an abandoned opening's PR links the issue and has no recorded number, so only its marker shows it is the task's own, and its number is added to the own PRs for the check. The PR-number mismatch is refused here, before any GitHub change on this branch other than the draft step in step 1, which skips a mismatched PR and only makes PRs safer. If the task head is its base, nothing is published: after the full publish guard (versions, status, no requeue or rebase), an earlier ready PR is turned into a draft (reported as `leftReady` if the repository has no drafts), and a running task moves to needs human. 4. **Check.** Run the check. Record the result, and any status change, in one transaction. That transaction refuses if the task changed during the check. @@ -123,7 +123,6 @@ Some repositories do not support draft PRs (for example private repositories on - **Repository renames and transfers.** Every PR record is kept under the configured repository name. After a rename or transfer, publish fails closed (the task's PRs are refused or reported as unsettled) until the records are moved; nothing here moves them. - **Continue from possibly already fixed.** The Continue and Cancel actions are user actions for a later slice. -- **Close the draft on cancel.** The design closes the draft PR when a person cancels a needs-human task. The PR record is kept for that. ## When the server publishes (#103) @@ -132,26 +131,12 @@ Some repositories do not support draft PRs (for example private repositories on - **What is published.** The whole task head, `base..head` (#113). Before the runner's first commit, the review records the user's checkout, so a task usually starts from the branch's own commits: a person made them, and the ledger has no entry for them, so they count as foreign (the design: a commit not in the ledger is foreign). The runner's commits come on top. All of them reach the `codeboost/…` branch and the PR, as the design intends. Review shows a foreign commit's changes in the Unplanned row, and the merge gate blocks (`unplanned`) until a person assigns or accepts each one. The gate inspects and merges the PR named in `github.pullRequest`, not yet the task's published PR, so this protects the published PR only when the two are the same; pointing the gate at the published PR is a follow-up. The push may overwrite only an owned commit already on the branch. Mapping commits through a rebase is #22's. - **Base branch.** `github.baseBranch` is required with a runner block. The `gh` calls run with the same environment as the push (its `GH_HOST` and credentials). The CLI checks it before it takes the lock, and `setUpRunner` checks it again before anything is created. - **Mode.** The task's status decides it. A `running` task whose every plan item has completed at the current revision gets a ready PR. A task in `needs human`, by any path (a safety finding, the time budget), gets a draft PR. Its problems say what sent it there now: its current attempt's safety finding (or, when that finding's save failed, the attempt's diagnostic that carries it), else "time budget ran out" when the budget has passed, else that attempt's diagnostic. A budget that ran out after a safety finding does not hide the finding. An earlier attempt's finding, which a person may have dealt with, is not used. Any other status, an active attempt, a run still in progress, or a publish already in progress refuses. A draft waits only for an owed safety finding, not an owed scope pause: a draft invites no review, and a task in needs human cannot record a pause. A ready publish also needs every item of the current plan revision to have completed at that revision. The Store's publish guard checks this before every guarded write, so a revision that adds an item while a publish awaits GitHub refuses it. So does work an earlier run still owes: a safety finding not yet acted on, or a scope pause never recorded (its write failed, or the process stopped). Every plan item has run, so no later run would pay it, and a ready PR would show unreviewed out-of-scope changes. A run's end, startup and a refused `publish` action therefore have the executor pay it first (`ItemExecutor.payOwed`, as a run does before its first item). The task then moves to needs amendment or needs human, and a draft follows for the latter. For a task already in needs human, only an owed safety finding is paid. That draft names the paid finding, which only this process holds. It does so only while the task stays in needs human on the same current attempt. A later run that returns the task to needs human has its own reason. -- **When.** Once a run started by `start` or `resume` has ended, with no attempt active (`assertPublishable` refuses one). On the `publish` runner action (`expectedStateVersion` and an action ID, like the other runner actions), which retries a refused or failed publish. When a refused `start` or `resume` moves the task to needs human (an expired budget is committed with the refusal); a refused action that leaves the status as it was publishes nothing, so pressing resume on a task already in needs human never pushes. Once at startup, if a publish is owed: the CLI starts it through `publishOwed`, which first runs the check that the runner lock still names the database, because a publish pushes; if the check fails, nothing starts. A task recorded as `not published` by the v10 upgrade stays unpublished until a person uses the `publish` action, or until a run starts after the upgrade (its end is the first state the upgrade did not decide), or a person's `start` or `resume` moves the task. Otherwise a publish is owed when the task can be published and no publish has settled (`opened`, `possibly already fixed`, `draft skipped`, `draft unsupported`, `no changes`) at the task's current state version. So a refusal, a failure or a stop leaves it owed, and the next start recovers a lost opening by its marker. A publish that ends with an opening still in flight (its own reply was lost, or it was refused while one settles) is tried again at that opening's own deadline, plus a second (`settleRemaining`; a timer, cleared at shutdown). A short retry, after 30 seconds, follows a publish refused because GitHub's PR list lags behind a recorded PR, one refused by a guard (mostly a race: the task, its review or its head changed during the publish; a refusal that persists is in the mode too, so the retry finds nothing owed), one refused just as an opening's deadline passed, and one whose open or update left work behind. That last case covers GitHub showing another head than the one pushed (the PR is made a draft and the task stays running), and a draft change that failed. Such an `opened` outcome is recorded with `reconcile`, which keeps the publish owed. Retries are bounded: one timer per task, and per chain at most one retry at an opening's deadline (while it is still ahead) and one short retry. A chain ends when a publish settles. A new one starts with a run's end, a person's action or a restart, never with a retry's own publish. Whatever is still unsettled after them is stuck for a reason a person must fix (two PRs carry a marker, the repository changed, GitHub keeps failing every open), so the next publish waits for a run's end, a restart or the `publish` action. -- **Outcome.** Each publish runs in the background, one per task, and records its outcome in `publish_outcomes` (schema v10): the outcome kind, or `refused` (a guard, `BranchPushRefused`, `OpeningUnsettled`, `PullRequestMisplaced`, `PullRequestRefused`), `failed` or `stopped` (shutdown), with its message (any token value or token shape removed, as for the push) and the task's state version. The record does not move the task's state version. Its state version is the one the publish last saw (`seenVersion`, #114), which every outcome carries. Where a path ends right after a Store write or guarded read, it is the version read there. Where a GitHub call came after the last write (a draft change that failed and left the PR ready, or one refused because drafts are unsupported), it is the version from before that call. So a change someone else made during the call still leaves a publish owed. A refusal, a failure or a stop is stamped with the current version, and stays owed anyway. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. Before it starts, every publish (after a run, at startup or on the action) records itself as `publishing`, its durable in-flight ownership. If that write fails, the publish does not run. A `publishing` record left at startup is a publish whose process stopped before it recorded an outcome. Startup publishes again if one is owed, and otherwise settles the record as `stopped`. When only an action reply is stuck (its publish never recorded itself as started), the reply reports the outcome on record, and that record is left as it is. A failure while paying owed work does not stop this; the task's PR records show what that publish did. A reply that still says `publishing` belongs to an action whose process stopped (a crash) before its publish recorded an outcome. Any later outcome of the task settles it too, since one publish runs per task. At startup, if no publish is owed, it is settled as `stopped` ("interrupted before it recorded an outcome"), so no replay says `publishing` for ever. The v10 upgrade records every task already `running` or in `needs human` as `not published`, a settled outcome: the first start after the upgrade opens no PR a person did not ask for, and the `publish` action publishes it. `GET /api/runner` shows it as `publish.last`, with `publish.active` and `publish.publishable`. -- **Start and resume** refuse while a publish of the task is in progress, so a new item cannot move the head being pushed. A second `publish` is refused too, and the refusal is saved under its action ID. Cancel task is not refused: a draft PR that GitHub opens after a cancel is recorded, and closing it is the later "close the draft on cancel" slice. +- **When.** Once a run started by `start` or `resume` has ended, with no attempt active (`assertPublishable` refuses one). On the `publish` runner action (`expectedStateVersion` and an action ID, like the other runner actions), which retries a refused or failed publish. When a refused `start` or `resume` moves the task to needs human (an expired budget is committed with the refusal); a refused action that leaves the status as it was publishes nothing, so pressing resume on a task already in needs human never pushes. Once at startup, if a publish is owed: the CLI starts it through `publishOwed`, which first runs the check that the runner lock still names the database, because a publish pushes; if the check fails, nothing starts. A task recorded as `not published` by the v10 upgrade stays unpublished until a person uses the `publish` action, or until a run starts after the upgrade (its end is the first state the upgrade did not decide), or a person's `start` or `resume` moves the task. Otherwise a publish is owed when the task can be published and no publish has settled (`opened`, `possibly already fixed`, `draft skipped`, `draft unsupported`, `no changes`) at the task's current state version. So a refusal, a failure or a stop leaves it owed, and the next start recovers a lost opening by its marker. A publish or close that ends with an opening still in flight (its own reply was lost, or it was refused while one settles) is tried again at that opening's own deadline, plus a second (`settleRemaining`; a timer, cleared at shutdown). A short retry, after 30 seconds, follows a publish refused because GitHub's PR list lags behind a recorded PR, one refused by a guard (mostly a race: the task, its review or its head changed during the publish; a refusal that persists is in the mode too, so the retry finds nothing owed), one refused just as an opening's deadline passed, and one whose open or update left work behind. That last case covers GitHub showing another head than the one pushed (the PR is made a draft and the task stays running), and a draft change that failed. Such an `opened` outcome is recorded with `reconcile`, which keeps the publish owed. Retries are bounded: one timer per task, and per chain at most one retry at an opening's deadline (while it is still ahead) and one short retry. A chain ends when a job settles. A new one starts with a run's end, a cancel, a person's action or a restart, never with a retry's own job. Whatever is still unsettled after them is stuck for a reason a person must fix (two PRs carry a marker, the repository changed, GitHub keeps failing every open), so the next publish or close waits for a run's end, a restart or an action. +- **Outcome.** Each publish runs in the background, one per task, and records its outcome in `publish_outcomes` (schema v10): the outcome kind, or `refused` (a guard, `BranchPushRefused`, `OpeningUnsettled`, `PullRequestMisplaced`, `PullRequestRefused`), `failed` or `stopped` (shutdown), with its message (any token value or token shape removed, as for the push) and the task's state version. The record does not move the task's state version. Its state version is the one the publish last saw (`seenVersion`, #114), which every outcome carries. Where a path ends right after a Store write or guarded read, it is the version read there. Where a GitHub call came after the last write (a draft change that failed and left the PR ready, or one refused because drafts are unsupported), it is the version from before that call. So a change someone else made during the call still leaves a publish owed. A refusal, a failure or a stop is stamped with the current version, and stays owed anyway. A close is stamped with the current version. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. Before it starts, every publish or close (after a run, a cancel, at startup or on an action) records itself as `publishing` or `closing`, its durable in-flight ownership. If that write fails, the publish does not run. A `publishing` or `closing` record left at startup is a publish whose process stopped before it recorded an outcome. Startup runs the job again if one is owed, and otherwise settles the record as `stopped`; the task's PR records show what that job did. When only an action reply is stuck (its job never recorded itself as started), the reply reports the outcome on record, and that record is left as it is. A failure while paying owed work does not stop this. A reply that still says `publishing` (or `closing`) belongs to an action whose process stopped (a crash) before its publish recorded an outcome. Any later outcome of the task settles it too, since one publish or close runs per task. At startup, if no publish is owed, it is settled as `stopped` ("interrupted before it recorded an outcome"), so no replay says `publishing` or `closing` for ever. The v10 upgrade records every task already `running` or in `needs human` as `not published`, a settled outcome: the first start after the upgrade opens no PR a person did not ask for, and the `publish` action publishes it. `GET /api/runner` shows it as `publish.last`, with `publish.active` and `publish.publishable`. +- **Start and resume** refuse while a publish of the task is in progress, so a new item cannot move the head being pushed. A second `publish` is refused too, and the refusal is saved under its action ID. Cancel task is not refused: it stops the publish, and the close below handles what the publish left. - **Demo.** A demo never publishes, even with a `github` block or an injected publisher. -## A push left running by a crash (#112) - -A normal shutdown aborts a publish and awaits its `git push`. If codeboost crashes or is killed instead, the push can keep running, and nothing records its process group, unlike a preparation's. That process is harmless, because its lease (`pushArguments`) pins the exact branch value its own run read. So it can only land while the branch holds that value. - -- **It lands before a newer publish reads the branch.** The newer publish reads the leftover's head. That head is a commit the ledger records as codeboost's, so the publish moves the branch on from it. -- **It lands between the newer publish's read and its push.** The newer push's lease no longer matches, so it is refused (`BranchPushRefused`, "moved while codeboost pushed it"). Nothing is overwritten. The publish is recorded as `refused` and stays owed; the next publish reads the leftover's head and moves on. -- **It lands after the newer publish moved the branch.** Its own lease no longer matches, so the remote refuses it. The newer head stays. -- **The branch has moved back to the value it read.** The lease compares the value, not its history. So if the branch returns to that exact commit, because a person force-pushes it back or a later publish pushes that commit again, the leftover lands and moves the branch to its own head, a codeboost commit. An open PR then shows that head, which is not the one recorded, until the next publish pushes the task head again and refreshes the PR. Nothing starts that publish by itself. The branch must hold exactly the commit the crashed run read, and the leftover can only put back a commit codeboost made. -- **The branch has been deleted, and the run read none.** A run that read no branch pushes with an empty lease. So if a newer publish created the branch and it was deleted since (by a person, or by GitHub after the PR merged), the leftover recreates it at its own head, a codeboost commit. No PR is opened for it, and nothing in codeboost removes it, so a stray `codeboost/…` branch can be left on GitHub until a person deletes it. It overwrites nothing. - -The process group is therefore not recorded, and startup does not look for it (decision on #112). The `gh` calls that open, update, draft or close a PR are not covered by this argument. Their effects are recovered by their markers instead: a lost opening, an update in flight, and a close's read by number. - -## Tests - -`test/runner-publishing.test.ts` covers the server wiring (#103) with a local bare repository as the remote and GitHub faked: a completed run opens one ready PR with the branch at the task head, a needs-human task gets a draft with its problems, a branch holding someone else's commit is refused and the `publish` action retries it (and its replay reports the outcome), a refused resume that moves the task to needs human publishes a draft while one on a task already there publishes nothing, start, resume and a second publish are refused while a publish runs, shutdown during the push or during the opening closes the Store only after the publish settled, a PR whose reply arrives just after the abort is still recorded before the write gate closes, and the next start publishes once or recovers the opening by its marker. Removing the startup publish, the after-run publish, the publish after a refused resume, the status-change check, the busy guard, the replay update or the shutdown wait, or moving that wait after the write gate, each fails a test. It also publishes a head that holds the branch's own (foreign) commit, and checks that review shows it as Unplanned and the merge gate blocks on it (#113); removing that blocker fails the test. `test/runner-branch-push.test.ts` replays a leftover push, with the pusher's own arguments against a real bare remote, before, between and after a newer publish's read and push (#112); dropping the lease fails the last two. `test/already-fixed.test.ts` and `test/publish.test.ts` cover each row above. Each guard was also broken on purpose, and a test failed each time: own-PR and own-commit exclusion, repository-qualified exclusion, every bound, the ancestor check, the fail-closed catch, the issue-number boundary, the state-version and head re-read, the status check before the no-changes shortcut, the version-owned status change, the open-state check, the settle time, the branch hash, the environment allowlist, reuse of the earlier PR, lost-opening recovery, adoption on the main path and counting that PR as the task's own, drafting a PR that a task which cannot publish still has ready, adopting an abandoned opening's PR that appears after the task stopped, the abort checks on the no-changes path and in that draft step (a lookup that fails, one that answers, a draft change that fails and one that lands), recording a late PR by adoption alone, an abort during the head-mismatch draft change, the `closing` checks before a push and before a ready change, `close()` aborting a publish in progress, the repository check, the definite 422 refusal, a retargeted own PR and another base's PR from the branch, refusals matched on stderr and GitHub's `errors[].message` only, the configured base in recovery and the head settle, the draft step finding the task's PR in any base, closing-only cross-references, both sides of a manual link, the query text, the fence length, and the description bounds. +`test/runner-publishing.test.ts` covers the server wiring (#103) with a local bare repository as the remote and GitHub faked: a completed run opens one ready PR with the branch at the task head, a needs-human task gets a draft with its problems, a branch holding someone else's commit is refused and the `publish` action retries it (and its replay reports the outcome), a refused resume that moves the task to needs human publishes a draft while one on a task already there publishes nothing, start, resume and a second publish are refused while a publish runs, shutdown during the push or during the opening closes the Store only after the publish settled, a PR whose reply arrives just after the abort is still recorded before the write gate closes, and the next start publishes once or recovers the opening by its marker. Removing the startup publish, the after-run publish, the publish after a refused resume, the status-change check, the busy guard, the replay update or the shutdown wait, or moving that wait after the write gate, each fails a test. For #111 it also covers: a cancelled task in review gets its ready PR closed with the branch kept; a cancelled needs-human task gets its draft closed; a cancel during a draft opening is answered at once, stops the publish, and the PR GitHub opened meanwhile is recovered by its marker and closed; a failed close is owed and the `close-pull-requests` action retries it; shutdown during a close leaves it to the next start; a PR GitHub's list does not show yet is closed by its number; a replayed or refused cancel starts nothing; a close refused while the opening settles is retried by itself and closes the PR GitHub showed late; and a task with no PR calls GitHub for nothing. `test/publish.test.ts` checks that the gateway closes only the task's own open PR (branch, repository, marker, number), re-checks right before the close, and requires GitHub to show it closed, and that the publisher closes only a cancelled task's PRs, by number, and nothing once shutdown begins between the read and the close. Removing the cancel hook, the close after a stopped publish, the owed close, the close by number, the replay check, the retry timer or the re-check before the close each fails a test. It also publishes a head that holds the branch's own (foreign) commit, and checks that review shows it as Unplanned and the merge gate blocks on it (#113); removing that blocker fails the test. `test/runner-branch-push.test.ts` replays a leftover push, with the pusher's own arguments against a real bare remote, before, between and after a newer publish's read and push (#112); dropping the lease fails the last two. `test/already-fixed.test.ts` and `test/publish.test.ts` cover each row above. Each guard was also broken on purpose, and a test failed each time: own-PR and own-commit exclusion, repository-qualified exclusion, every bound, the ancestor check, the fail-closed catch, the issue-number boundary, the state-version and head re-read, the status check before the no-changes shortcut, the version-owned status change, the open-state check, the settle time, the branch hash, the environment allowlist, reuse of the earlier PR, lost-opening recovery, adoption on the main path and counting that PR as the task's own, drafting a PR that a task which cannot publish still has ready, adopting an abandoned opening's PR that appears after the task stopped, the abort checks on the no-changes path and in that draft step (a lookup that fails, one that answers, a draft change that fails and one that lands), recording a late PR by adoption alone, an abort during the head-mismatch draft change, the `closing` checks before a push and before a ready change, `close()` aborting a publish in progress, the repository check, the definite 422 refusal, a retargeted own PR and another base's PR from the branch, refusals matched on stderr and GitHub's `errors[].message` only, the configured base in recovery and the head settle, the draft step finding the task's PR in any base, closing-only cross-references, both sides of a manual link, the query text, the fence length, and the description bounds. ## Test this document with a reader diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 58a08c76..7c46c81c 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -253,7 +253,7 @@ codeboost never writes the user's repository. The commits it makes live in a **r |---|---|---| | 1 | Set `stopping` on the server and make the runner coordinator reject admission, in the same synchronous turn, before any active-work list is copied. New API requests get HTTP 503, and new runner work throws `ShuttingDownError`. **Admitted requests are not rejected:** AGENTS.md says to drain already-admitted HTTP requests, and to recheck the flag only at irreversible boundaries. So after reading a body, the server rechecks `stopping` only for `merge`. The question coordinator also stops admission in this turn (`questions.stopAdmission()`): a question admitted before shutdown still saves its note, but starts no agent and gets a retryable "Server stopped" answer (`markStopped`). **Changed while implementing F1c:** an earlier revision of this row rejected every admitted write and closed the Store write gate here. That broke the drain rule and the existing browser regression "drains an in-flight question request before closing its agent manager", so the gate now closes in step 3. | server flag, merge recheck and question admission stop: yes. Runner barrier: new. | | 2 | Stop accepting connections, and wait for admitted requests up to the drain limit (at most 14.5 s, below the 15 s request timeout). | yes | -| 2a | Abort every publish in progress (`TaskPublishing.close()`, #103) in the same turn as step 1, and await it here, at the end of the drain, before the write gate closes: the publisher already refuses every later push and opening through the coordinator's `closing` flag, and a publish's last records (an abandoned opening, the outcome) must still land. A publish stopped this way is owed, so the next start publishes it again. | new (#103) | +| 2a | Abort every publish and PR close in progress (`TaskPublishing.close()`, #103, #111) in the same turn as step 1, and await it here, at the end of the drain, before the write gate closes: the publisher already refuses every later push and opening through the coordinator's `closing` flag, and a publish's last records (an abandoned opening, the outcome) must still land. A publish stopped this way is owed, so the next start publishes it again. | new (#103) | | 3 | After the drain limit, abort the signals of the remaining requests, destroy requests that are still reading a body, then **close the `Store` write gate**, then await request-owned work (the merge coordinator). From here on, every `Store` write throws `ShuttingDownError` unless the caller passes the shutdown capability. The server hands that capability at construction only to coordinators' settlement and close code (the runner, questions' `finishAnswer`, merge settlement after an irreversible command, and E3's `settleSuggestion` when it is wired), and to startup recovery; HTTP handlers and `ReviewService` never receive it. So a request-path write still pending after the abort (an aborted `/api/review` whose `load()` reaches `recordHistory`, or merge reconciliation and queue polling after a GitHub await) fails. The server maps `ShuttingDownError` to HTTP 503 "The review server is shutting down.", never to the 409 used for review errors, and there is no partial view. **Today the merge coordinator catches every error in `displayStatus()` and `#pollQueue()`**, which would swallow the gate error and answer 200; F1 makes each of those catch blocks rethrow `ShuttingDownError` first. The next startup checks GitHub again, so a skipped merge observation is recovered. The server also closes idle keep-alive connections once shutdown starts and after each request finishes, so a finished request's socket cannot hold `server.close()` open until its keep-alive timeout. | abort: yes. Gate, capability, rethrow and idle-connection close: new. | | 4 | For every `pending` and `running` attempt that has an in-memory job, record the first reason `shutdown` **only if no first reason is set yet**. A job already marked `cancelled`, `stale` or `time-limit` keeps its reason. For `running`, call `cancel('shutdown')` and await `settled`. For `pending`, await its preparation promise (the D start call itself returns at once, so it is never in progress), remove only its host-side preparation files (task storage waits for the terminal write, as in step 5), and cancel any handle that start returned, then await its `settled` (the "Launch" table). F does not abandon a job after a timer (decision 4). **This ends:** D's supervisor escalates to a forced kill, and retries unfinished container, network or setup cleanup every second for at most 60 seconds after the first failure (`CLEANUP_RETRY_WINDOW_MS`, `agents/adapters/supervisor.ts`). Retries after the first get only what is left of the window, and every cleanup subprocess is killed at its deadline, so settlement ends about 90 seconds after cleanup starts at worst (up to 30 seconds for the first, failed attempt, then the 60-second window). Then `settled` resolves with a `stopReason` and an `unreleased` list, even if Docker is unreachable or hangs. F records `unreleased` durably before it frees the slot. See the prerequisite below. | new | | 5 | For each attempt from step 4, in the order set by the task-storage holder row: the partial-output export (writable attempts), then the terminal state from its first reason (a `shutdown` reason gives `cancelled` "Stopped by shutdown") with its `diagnostic_ref`, then `removeTaskFilesystems`, and only then free its slot. A failed write leaves the row non-terminal, and a failed removal keeps the slot held. Startup recovery handles both. | new | diff --git a/github/pull-requests.ts b/github/pull-requests.ts index 264e87ff..eed67d00 100644 --- a/github/pull-requests.ts +++ b/github/pull-requests.ts @@ -42,6 +42,13 @@ export interface PullRequestGateway { /** Replaces the title and description of an open PR codeboost opened; marks it ready when `ready`, or a draft when `draft`. */ /** `beforeReady` runs after the description update's await and before any ready or draft change; if it throws, no such change is made. */ refresh(number: number, input: OpenPullRequestInput & { ready: boolean; headSha?: string; beforeReady?: () => void }, signal?: AbortSignal): Promise; + /** + * Closes a PR codeboost opened (#111), read by its number (GitHub's list may lag behind it). An open PR must still be from + * `headBranch` in this repository with `marker` on its first line, or it is refused (`PullRequestMisplaced`) and left + * open. Already closed is success. `beforeClose` runs after the read and right before the close; if it throws, nothing + * is closed. The branch is kept. Settles only once GitHub shows the PR closed. + */ + close(number: number, input: { headBranch: string; marker: string; beforeClose?: () => void }, signal?: AbortSignal): Promise<{ number: number; url: string }>; /** Turns an open PR codeboost opened back into a draft; a no-op for a draft. */ markDraft(number: number, input: { base: string; headBranch: string; marker: string }, signal?: AbortSignal): Promise; } @@ -259,6 +266,33 @@ export class GhPullRequestGateway implements PullRequestGateway { return pr; } + async close(number: number, input: { headBranch: string; marker: string; beforeClose?: () => void }, signal?: AbortSignal): Promise<{ number: number; url: string }> { + signal = this.#bounded(signal); + this.#validate(input); + if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid pull request number.'); + const read = (value: unknown) => { + const pr = value as { number?: unknown; html_url?: unknown; state?: unknown; body?: unknown; head?: { ref?: unknown; repo?: { full_name?: unknown } | null } } | null; + if (!pr || pr.number !== number || typeof pr.html_url !== 'string' || !pr.html_url.startsWith('https://') || (pr.state !== 'open' && pr.state !== 'closed') + || (pr.body !== null && typeof pr.body !== 'string') || typeof pr.head?.ref !== 'string') throw new Error('GitHub returned an invalid pull request.'); + const repo = pr.head.repo?.full_name; + const ours = pr.head.ref === input.headBranch && typeof repo === 'string' && repo.toLowerCase() === this.repository.toLowerCase() && markerOf(pr.body ?? '') === input.marker; + return { number, url: pr.html_url, open: pr.state === 'open', ours }; + }; + const url = `repos/${this.repository}/pulls/${number}`; + const before = read(await this.#json(['api', '-H', 'Accept: application/vnd.github+json', url], signal)); + // Closed already, by anyone: nothing is left to do, whatever its description says now. + if (!before.open) return { number, url: before.url }; + // Never someone else's PR: an open one must still be from the task branch in this repository, with the task's marker. + if (!before.ours) throw new PullRequestMisplaced(`Pull request #${number} is open but no longer from ${input.headBranch} with its first line, so codeboost did not close it. Close it, or restore its branch and first line.`); + // The caller's re-check (shutdown, task state) right before the irreversible change, with no await since. + input.beforeClose?.(); + const after = read(await this.#json(['api', '-X', 'PATCH', '-H', 'Accept: application/vnd.github+json', url], signal, { state: 'closed' })); + // A command that exits 0 does not prove the change applied: the answer must show the PR closed. Only its state is + // checked now: the close has happened, so a description edited meanwhile must not turn it into a failure. + if (after.open) throw new Error(`GitHub did not close pull request #${number}.`); + return { number, url: after.url }; + } + /** The caller has just read the PR as ready, so this changes it straight away and reads the result back once. */ async markDraft(number: number, input: { base: string; headBranch: string; marker: string }, signal?: AbortSignal): Promise { signal = this.#bounded(signal); diff --git a/runner/publish.ts b/runner/publish.ts index a52c54a4..e9967b7d 100644 --- a/runner/publish.ts +++ b/runner/publish.ts @@ -118,9 +118,83 @@ export class PullRequestPublisher { finally { inflight.delete(key); this.#running.delete(publish); } } + /** + * Closes every open PR a cancelled task opened (#111; design, "Needs human": cancel closes the draft PR), draft or + * ready, whatever base it is in now. A lost opening or update is settled first, by its marker, so a PR that GitHub + * opened while the task was being cancelled is closed too. The branch is kept. Returns the PRs it closed. + */ + async closeAll(identity: PlanIdentity, signal?: AbortSignal): Promise { + signal?.throwIfAborted(); + this.#assertOpen(); + const key = identityKey(identity); + let inflight = publishing.get(this.#store); + if (!inflight) publishing.set(this.#store, inflight = new Set()); + if (inflight.has(key)) throw new GuardRefusal('A pull request is already being published or closed for this task.'); + inflight.add(key); + const abort = new AbortController(); + const run = { abort, done: this.#closeAll(identity, signal ? AbortSignal.any([signal, abort.signal]) : abort.signal) }; + this.#running.add(run); + try { return await run.done; } + finally { inflight.delete(key); this.#running.delete(run); } + } + + async #closeAll(identity: PlanIdentity, signal: AbortSignal): Promise { + if (this.#store.getTask(identity).status !== 'cancelled') throw new GuardRefusal('Only a cancelled task\'s pull requests are closed.'); + // Each PR that cannot be closed is reported, and the others are still closed: one moved PR must not keep a ready one + // open on another branch. Shutdown and abort end the close at once. + const problems: unknown[] = [], closed: number[] = []; + const keep = (error: unknown) => { if (signal.aborted || error instanceof ShuttingDownError) throw error; problems.push(error); }; + // Settles a lost opening (OpeningUnsettled while GitHub may still show it) and a lost update first. The task was + // cancelled after they began, so neither moves its status. + try { await this.#recover(identity, false, signal); } catch (error) { keep(error); } + const close = async (number: number, headBranch: string, mark: string) => { + try { + // The shutdown flags right before the close, after the gateway's read: no await between them. + await this.#pulls.close(number, { headBranch, marker: mark, beforeClose: () => this.#assertOpen() }, signal); + closed.push(number); + } catch (error) { keep(error); } + }; + const all = this.#store.taskPullRequests(identity), here = (pr: TaskPullRequest) => pr.repository.toLowerCase() === this.#config.repository.toLowerCase(); + // A record in another repository (a rename, a transfer, a changed configuration) cannot be closed from here. It is + // reported, so the close is not recorded as done and stays owed; the records in this repository are still closed. + // A lost opening there is already refused by the recovery above. + for (const pr of all) if (!here(pr) && pr.state !== 'opening') + keep(new PullRequestMisplaced(`${pr.number === null ? 'A pull request the task was opening' : `Pull request #${pr.number}`} was opened in ${pr.repository}, not ${this.#config.repository}, so codeboost cannot close it from here. Close it on GitHub, then close the task's pull requests again.`)); + const prs = all.filter(here); + // An opened record is closed by its number, which nobody can edit: GitHub's list can lag behind a PR, and the gateway + // closes it only while it is still from the task branch with its marker, wherever its base is now. + for (const row of prs) if (row.state === 'opened' && row.number !== null) await close(row.number, row.headBranch, marker(row.openingId)); + // An abandoned opening's PR has no number on record: it is found by its marker, in any base. + for (const headBranch of new Set(prs.filter(pr => pr.state === 'abandoned').map(pr => pr.headBranch))) { + const rows = prs.filter(pr => pr.state === 'abandoned' && pr.headBranch === headBranch); + let owned; + try { owned = await this.#pulls.findOwned({ headBranch, markers: rows.map(row => marker(row.openingId)) }, signal); } + catch (error) { keep(error); continue; } + signal.throwIfAborted(); + for (const row of rows) { + const mine = owned.filter(pr => pr.marker === marker(row.openingId)); + if (!mine.length) continue; + // Two PRs carrying it (a copied description) cannot be told apart, so neither is closed. Once a person has closed + // the one that is not the task's, the next close finds only the task's. + if (mine.length > 1) { keep(new PullRequestMisplaced(`${pullRequestList(mine)} carry the first line of a pull request the task was opening, so codeboost closed neither. Close the one that is not the task's (compare their authors and creation times), then close the pull requests again.`)); continue; } + const pr = mine[0]!; + // Recorded first, so the task's records name every PR it has on GitHub, before the irreversible close. + this.#store.adoptOpening(identity, row.openingId, pr, { stateVersion: this.#store.getTask(identity).stateVersion, reviewVersion: this.#store.reviewVersion(identity) }); + await close(pr.number, headBranch, pr.marker); + } + } + if (!problems.length) return closed; + const text = [closed.length ? `${pullRequestList(closed.map(number => ({ number })))} closed.` : '', + ...problems.map(error => error instanceof Error ? error.message : String(error))].filter(Boolean).join(' '); + // An unsettled opening is retried once it has settled; refusals are for a person; anything else is a failure. + if (problems.some(error => error instanceof OpeningUnsettled)) throw new OpeningUnsettled(text); + if (problems.every(error => error instanceof GuardRefusal || error instanceof PullRequestMisplaced)) throw new PullRequestMisplaced(text); + throw new Error(text); + } + /** * How long until the task's lost opening may be abandoned (0 when it may be now, null when there is none): its own - * deadline, its creation plus the settle time, on the clock recovery uses. + * deadline, its creation plus the settle time, on the clock recovery uses. A close refused meanwhile waits for it. */ settleRemaining(identity: PlanIdentity): number | null { const lost = this.#store.taskPullRequests(identity).find(pr => pr.state === 'opening'); diff --git a/runner/publishing.ts b/runner/publishing.ts index 07d09763..303156a4 100644 --- a/runner/publishing.ts +++ b/runner/publishing.ts @@ -11,8 +11,8 @@ import type { PublishRecord, Store } from './store.ts'; const SETTLED = ['opened', 'possibly already fixed', 'draft skipped', 'draft unsupported', 'no changes', // Recorded by the v10 upgrade for a task that reached its status before publishing existed: only the action publishes it. 'not published']; -/** The record a publish writes before it starts; one left at startup is a publish whose process stopped (#103). */ -const IN_FLIGHT = 'publishing'; +/** The records a publish or close writes before it starts; one left at startup is a job whose process stopped. */ +const IN_FLIGHT = ['publishing', 'closing']; /** The refusal while an earlier run owes work; the publish action that gets it pays that work (web/server.ts). */ export const OWED_REFUSAL = 'An earlier run left a safety finding or a scope pause that has not been acted on yet; it is settled before anything is published.'; /** @@ -22,13 +22,19 @@ export const OWED_REFUSAL = 'An earlier run left a safety finding or a scope pau export const SHORT_RETRY_MS = 30_000; /** Refusals a person acts on; anything else that fails is reported as `failed`. */ const REFUSALS = [GuardRefusal, BranchPushRefused, OpeningUnsettled, PullRequestMisplaced, PullRequestRefused]; +/** A publish stopped because a person cancelled its task (#111); the close that follows does what is left. */ +export class PublishCancelled extends Error {} + +/** What a task's pull request work is now: a publish (ready or draft), or the close of a cancelled task's PRs (#111). */ +export type PullRequestJob = { kind: 'publish'; draft: boolean } | { kind: 'close' }; /** - * Publishes a task's pull request in production (#103): when a run ends, on the `publish` action, and once at startup. - * The task's status decides the mode: a running task whose every plan item has run gets a ready PR, a task in needs human - * a draft PR with its problems. One publish per task at a time, in the background, tracked until it settles; its outcome - * is recorded in the Store, where GET /api/runner reads it. Shutdown aborts every publish and awaits it before the Store's - * write gate closes, so a publish's last records still land. + * Publishes a task's pull request in production (#103), and closes a cancelled task's PRs (#111): when a run ends, when + * a task is cancelled, on the `publish` and `close-pull-requests` actions, and once at startup. The task's status + * decides the job: a running task whose every plan item has run gets a ready PR, a task in needs human a draft PR with + * its problems, a cancelled task's open PRs are closed. One job per task at a time, in the background, tracked until it + * settles; its outcome is recorded in the Store, where GET /api/runner reads it. Shutdown aborts every job and awaits it + * before the Store's write gate closes, so a job's last records still land. */ export class TaskPublishing { #store: Store; #publisher: PullRequestPublisher; #runner: RunnerCoordinator; #executor: ItemExecutor; @@ -49,8 +55,8 @@ export class TaskPublishing { * memory only, so the task's attempt rows do not carry it, and the draft PR would otherwise not say why (#103). */ #escalations = new Map(); - /** Publishes in progress, by task, from scheduling until the outcome is recorded. */ - #running = new Map>(); + /** Jobs in progress, by task, from scheduling until the outcome is recorded; `abort` stops a publish on cancel. */ + #running = new Map; abort: AbortController }>(); constructor(store: Store, publisher: PullRequestPublisher, runner: RunnerCoordinator, executor: ItemExecutor, capability?: ShutdownCapability, env: NodeJS.ProcessEnv = process.env, options: { shortRetryMs?: number } = {}) { this.#store = store; this.#publisher = publisher; this.#runner = runner; this.#executor = executor; this.#write = settleWith(capability); @@ -58,42 +64,49 @@ export class TaskPublishing { this.#secrets = TOKEN_VARIABLES.flatMap(name => env[name] ?? []); } - /** Whether a publish of this task is in progress. */ + /** Whether a publish or close of this task is in progress. */ busy(identity: PlanIdentity): boolean { return this.#running.has(identityKey(identity)); } lastOutcome(identity: PlanIdentity): PublishRecord | null { return this.#store.lastPublish(identity); } - /** Resolves once the task's publish in progress, if any, has recorded its outcome. */ - async settled(identity: PlanIdentity): Promise { await this.#running.get(identityKey(identity)); } + /** Resolves once the task's job in progress, and any close it led to, have recorded their outcomes. */ + async settled(identity: PlanIdentity): Promise { + for (let job = this.#running.get(identityKey(identity)); job; job = this.#running.get(identityKey(identity))) await job.done; + } /** - * What a publish would do now: a ready or a draft PR. Throws the refusal otherwise, with what a person can act on. - * Writes nothing, so the view asks it too and never offers what the action would refuse. + * What a job would do now. Throws the refusal otherwise, with what a person can act on. Writes nothing, so the view + * asks it too and never offers what an action would refuse. */ - mode(identity: PlanIdentity, progress?: { next: string | null }): { draft: boolean } { + mode(identity: PlanIdentity, progress?: { next: string | null }): PullRequestJob { if (this.#closing || this.#runner.closing) throw new ShuttingDownError(); - if (this.busy(identity)) throw new GuardRefusal('A pull request is already being published for this task.'); + if (this.busy(identity)) throw new GuardRefusal(`A pull request is already being ${this.#running.get(identityKey(identity))!.job.kind === 'close' ? 'closed' : 'published'} for this task.`); + const task = this.#store.getTask(identity); + if (task.status === 'cancelled') { + // Only what the task opened, or was opening, can be closed: a task that never began a PR has nothing to do. + if (!this.#store.taskPullRequests(identity).length) throw new GuardRefusal('The task has no pull request to close.'); + return { kind: 'close' }; + } // Publishing reads the task head, so it waits for the run (and the attempt) to end: assertPublishable refuses an active attempt. if (this.#runner.isActive(identity) || this.#executor.busy(identity)) throw new GuardRefusal('A run of this task is still in progress; publish once it has ended.'); - const task = this.#store.getTask(identity); if (task.status === 'running') { const { next } = progress ?? this.#executor.progress(identity); if (next !== null) throw new GuardRefusal(`${next} has not run yet; publish once every plan item has run.`); this.#assertNothingOwed(identity); this.#store.assertPublishableNow(identity, false); - return { draft: false }; + return { kind: 'publish', draft: false }; } if (task.status === 'needs human') { // A draft invites no review, and a scope pause cannot be recorded for a task already in needs human, so only an owed // safety finding (its escalation records it) holds a draft back. this.#assertNothingOwed(identity, false); this.#store.assertPublishableNow(identity, true); - return { draft: true }; + return { kind: 'publish', draft: true }; } throw new GuardRefusal(`The task is ${task.status}; a pull request is published for a running task whose plan items have all run, or a task that needs a person.`); } /** * An earlier run owes a safety escalation or a scope pause (its write failed, or the process stopped): the task is not - * finished work until that is settled, so a ready PR must not show its unreviewed changes. publishIfOwed pays it. Read + * finished work until that is settled, so a ready PR must not show its unreviewed changes. actIfOwed pays it. Read * only for a status that could publish, so a poll of any other task does not scan its attempts. */ #assertNothingOwed(identity: PlanIdentity, scope = true): void { @@ -101,84 +114,104 @@ export class TaskPublishing { } /** - * The publish owed, if any: the task can be published, and no publish has settled since the task last changed. Null - * when nothing is owed. + * The job owed, if any. A publish is owed when the task can be published and no publish has settled since the task last + * changed; a close, when the task is cancelled, has a pull request record, and its PRs have not been closed since. Either + * is owed while an action that asked for it has not recorded an outcome. */ - owed(identity: PlanIdentity, options: { personAsked?: boolean } = {}): { draft: boolean } | null { - let mode; - try { mode = this.mode(identity); } + owed(identity: PlanIdentity, options: { personAsked?: boolean } = {}): PullRequestJob | null { + let job; + try { job = this.mode(identity); } catch (error) { if (error instanceof GuardRefusal || error instanceof ShuttingDownError) return null; throw error; } + // A person asked for this job and its process stopped before it recorded anything (a crash between the action and its + // in-flight record): the request is owed, whatever an earlier job settled, e.g. a close after a PR was reopened. + if (this.#store.hasUnsettledPublishAction(identity)) return job; const last = this.#store.lastPublish(identity); // A task that reached its status before publishing existed (the v10 upgrade's record) is published only when a person // asks, until a run starts after the upgrade: that run's end is the first state the upgrade did not decide. if (last?.outcome === 'not published' && !options.personAsked && !this.#store.getAttempts(identity).some(attempt => attempt.createdAt > last.at)) return null; + if (job.kind === 'close') return last?.outcome === 'closed' ? null : job; // `reconcile`: an open or update that left the PR or the task not where the publish meant them (GitHub showed another // head, or a draft change failed); the next publish reconciles it, so it is still owed. - return !last || !SETTLED.includes(last.outcome) || last.reconcile || last.stateVersion !== this.#store.getTask(identity).stateVersion ? mode : null; + return !last || !SETTLED.includes(last.outcome) || last.reconcile || last.stateVersion !== this.#store.getTask(identity).stateVersion ? job : null; } /** - * The `publish` action: start a publish now, or throw its refusal. Runs after the caller's transaction commits; its - * outcome replaces the action's saved response, so a replay reports it. + * The `publish` or `close-pull-requests` action: start that job now, or throw its refusal. Runs after the caller's + * transaction commits; its outcome replaces the action's saved response, so a replay reports it. */ - request(identity: PlanIdentity, actionId: string): { draft: boolean } { - const mode = this.mode(identity); + request(identity: PlanIdentity, kind: PullRequestJob['kind'], actionId: string): PullRequestJob { + // The action's own refusal comes first: asking to close a running task's PRs is not answered with a publish refusal. + const status = this.#store.getTask(identity).status; + if (kind === 'close' && status !== 'cancelled') throw new GuardRefusal(`The task is ${status}; only a cancelled task's pull requests are closed.`); + if (kind === 'publish' && status === 'cancelled') throw new GuardRefusal('The task is cancelled; its pull requests are closed, not published.'); + const job = this.mode(identity); // A person's action starts a new chain, with its own automatic retries (AGENTS.md: reset on an explicit user action). this.#newChain(identity); - this.#schedule(identity, mode.draft, actionId); - return mode; + this.#schedule(identity, job, actionId); + return job; } /** - * A run of the task ended, the server started, or a person's start or resume moved the task (`personAsked`: it also ends - * the v10 upgrade's `not published` hold, as a run starting would): publish if one is owed. Never throws. + * A run of the task ended, the task was cancelled, the server started, or a person's start or resume moved the task + * (`personAsked`: it also ends the v10 upgrade's `not published` hold, as a run starting would): do the job owed, if + * any. Never throws. */ - publishIfOwed(identity: PlanIdentity, options: { personAsked?: boolean; retry?: boolean } = {}): void { + actIfOwed(identity: PlanIdentity, options: { personAsked?: boolean; retry?: boolean } = {}): void { try { this.#payOwed(identity); - const owed = this.owed(identity, options); - if (!owed) return; - // A run's end, a person's action or a restart is a meaningful lifecycle change: a new chain, with its own automatic - // retries. A retry timer's own publish is not, or the retries would be unbounded again. + const job = this.owed(identity, options); + if (!job) return; + // A run's end, a cancel, a person's action or a restart is a meaningful lifecycle change: a new chain, with its own + // automatic retries. A retry timer's own job is not, or the retries would be unbounded again. if (!options.retry) this.#newChain(identity); - this.#schedule(identity, owed.draft); + this.#schedule(identity, job); } - catch (error) { console.error(`Could not start publishing: ${JSON.stringify(message(error, this.#secrets))}`); } + catch (error) { console.error(`Could not start pull request work: ${JSON.stringify(message(error, this.#secrets))}`); } + } + + /** + * The task was just cancelled (#111): stop its publish in progress, if any, whose own end then closes what it left; + * otherwise close the task's PRs now. The cancel itself never waits for GitHub. + */ + taskCancelled(identity: PlanIdentity): void { + const running = this.#running.get(identityKey(identity)); + if (running?.job.kind === 'publish') running.abort.abort(new PublishCancelled('The task was cancelled; its pull requests are closed instead.')); + else if (!running) this.actIfOwed(identity); } /** - * Startup, once the runner lock is verified: publish if one is owed. Otherwise a publish whose process stopped before - * it recorded an outcome (a crash) is settled now: its in-flight record, and any action reply still saying - * `publishing`, become `stopped`; the task's records (a PR it opened, recovered by its marker on the next publish) - * show what that publish did. Never throws. + * Startup, once the runner lock is verified: do the job owed, if any. Otherwise a publish or close action whose process + * stopped before its job recorded an outcome (a crash) is settled now, so its replay stops saying `publishing` or + * `closing`, and its in-flight record becomes `stopped`; the task's records show what that job did, and the action + * runs it again. Never throws. */ startup(identity: PlanIdentity): void { // A publish of this process is running: its record is its own, not one a crash left. if (this.busy(identity)) return; try { this.#payOwed(identity); - const owed = this.owed(identity); - if (owed) { this.#schedule(identity, owed.draft); return; } - const last = this.#store.lastPublish(identity); - if (!last || last.outcome === IN_FLIGHT) { - if (last || this.#store.hasUnsettledPublishAction(identity)) this.#write(() => this.#store.recordPublish(identity, { outcome: 'stopped', draft: last?.draft ?? false, - message: 'The publish was interrupted before it recorded an outcome (codeboost stopped). Its pull request, if it opened one, is in the task\'s records; the publish action runs it again.' })); - // Only a reply is stuck (its publish never recorded itself as started): it reports the outcome on record, which is - // left as it is, stamp included, so what is owed does not change. + const job = this.owed(identity); + if (job) { this.#schedule(identity, job); return; } + const last = this.#store.lastPublish(identity), closing = this.#store.getTask(identity).status === 'cancelled'; + if (!last || IN_FLIGHT.includes(last.outcome)) { + if (last || this.#store.hasUnsettledPublishAction(identity)) this.#write(() => this.#store.recordPublish(identity, { outcome: 'stopped', draft: last?.draft ?? false, ...(closing ? { action: 'close' as const } : {}), + message: `The ${closing ? 'close' : 'publish'} was interrupted before it recorded an outcome (codeboost stopped). The task's records show what it did; the ${closing ? 'close-pull-requests' : 'publish'} action runs it again.` })); + // Only a reply is stuck (its job never recorded itself as started): it reports the outcome on record, which is left + // as it is, stamp included, so what is owed does not change. } else if (this.#store.hasUnsettledPublishAction(identity)) this.#write(() => this.#store.settleUnsettledPublishReplies(identity, last)); - } catch (error) { console.error(`Could not start publishing: ${JSON.stringify(message(error, this.#secrets))}`); } + } catch (error) { console.error(`Could not start pull request work: ${JSON.stringify(message(error, this.#secrets))}`); } } /** - * Shutdown: refuse new publishes, abort those in progress (the publisher refuses every later push, opening and ready - * change) and await their recorded outcomes. + * Shutdown: refuse new jobs, abort those in progress (the publisher refuses every later push, opening, ready change and + * close) and await their recorded outcomes. */ async close(): Promise { this.#closing = true; for (const timer of this.#retries.values()) clearTimeout(timer); this.#retries.clear(); await this.#publisher.close(); - while (this.#running.size) await Promise.all([...this.#running.values()]); + while (this.#running.size) await Promise.all([...this.#running.values()].map(running => running.done)); } /** Settle what an earlier run owes before deciding what to publish (see mode); a task it moves is then not finished work. */ @@ -198,7 +231,7 @@ export class TaskPublishing { } /** - * A new chain (a run's end, a person's action, a restart): a fresh retry budget, and a retry the old chain armed is + * A new chain (a run's end, a cancel, a person's action, a restart): a fresh retry budget, and a retry the old chain armed is * dropped with it, so it cannot fire later on the new chain's budget. One timer per task at all times. */ #newChain(identity: PlanIdentity): void { @@ -209,7 +242,7 @@ export class TaskPublishing { } /** - * A publish that ended with an opening still in flight (its reply was lost, or it was refused while one settles), + * A publish or close that ended with an opening still in flight (its reply was lost, or it was refused while one settles), * refused because GitHub's list lags behind a PR, or opened without settling (`reconcile`) is tried again by itself: * nothing else would (no run ends, startup has passed). Bounded (AGENTS.md: back off recurring external polling): one * timer per task, and per chain (until a publish settles) at most one retry at an opening's deadline, while it is @@ -220,7 +253,7 @@ export class TaskPublishing { #retryIfUnsettled(identity: PlanIdentity, record: Omit, unsettled: boolean): void { try { const key = identityKey(identity); - if (SETTLED.includes(record.outcome) && !record.reconcile) { this.#retried.delete(key); return; } + if ((SETTLED.includes(record.outcome) || record.outcome === 'closed') && !record.reconcile) { this.#retried.delete(key); return; } if (this.#closing || this.#retries.has(key)) return; const retried = this.#retried.get(key) ?? { deadline: 0, short: 0 }; const remaining = this.#publisher.settleRemaining(identity); @@ -231,43 +264,62 @@ export class TaskPublishing { else if ((unsettled || record.reconcile) && !(remaining !== null && remaining > 0) && retried.short < 1) { retried.short++; wait = this.#shortRetryMs; } if (wait === null) return; this.#retried.set(key, retried); - const timer = setTimeout(() => { this.#retries.delete(key); this.publishIfOwed(identity, { retry: true }); }, wait + 1_000); + const timer = setTimeout(() => { this.#retries.delete(key); this.actIfOwed(identity, { retry: true }); }, wait + 1_000); timer.unref?.(); this.#retries.set(key, timer); } catch (error) { console.error(`Could not arrange a retry of the publish: ${JSON.stringify(message(error, this.#secrets))}`); } } - #schedule(identity: PlanIdentity, draft: boolean, actionId?: string): void { + #schedule(identity: PlanIdentity, job: PullRequestJob, actionId?: string): void { const key = identityKey(identity); // Reserved in this turn, so a second request in the same transaction is refused. - const reserved = Promise.withResolvers(); - this.#running.set(key, reserved.promise); + const reserved = Promise.withResolvers(), abort = new AbortController(); + this.#running.set(key, { job, done: reserved.promise, abort }); const run = async () => { // Durable in-flight ownership before the first external write (AGENTS.md): if this process stops before the outcome // is recorded, startup finds the marker and publishes again or settles it as interrupted. No marker, no publish. - try { this.#write(() => this.#store.recordPublish(identity, { outcome: IN_FLIGHT, draft, message: 'A pull request is being published.' })); } + const draft = job.kind === 'publish' && job.draft; + try { + this.#write(() => this.#store.recordPublish(identity, job.kind === 'close' ? { outcome: 'closing', draft: false, action: 'close', message: 'The task\'s pull requests are being closed.' } + : { outcome: 'publishing', draft, message: 'A pull request is being published.' })); + } catch (error) { console.error(`Could not record the publish as started, so it did not run: ${JSON.stringify(message(error, this.#secrets))}`); return; } let record: Omit, seenVersion: number | undefined, unsettled = false; try { - // The problems are read when the publish starts: the task is in needs human, and its last attempt says why. - const outcome = await this.#publisher.publish(identity, draft ? { problems: this.#problems(identity) } : {}); - record = describe(outcome, draft); - seenVersion = outcome.seenVersion; + if (job.kind === 'close') { + const closed = await this.#publisher.closeAll(identity, abort.signal); + record = { outcome: 'closed', draft: false, action: 'close', message: closed.length + ? `${closed.length === 1 ? 'Pull request' : 'Pull requests'} ${closed.map(number => `#${number}`).join(', ')} closed; the task's branch is kept.` + : 'No open pull request of the task was left to close.' }; + } else { + // The problems are read when the publish starts: the task is in needs human, and its last attempt says why. + const outcome = await this.#publisher.publish(identity, draft ? { problems: this.#problems(identity) } : {}, abort.signal); + record = describe(outcome, draft); + seenVersion = outcome.seenVersion; + } } catch (error) { - // A guard refusal is mostly a race (the task, its review or its head changed during the publish): one short retry - // publishes the new state. One that persists is in mode() too, so the retry finds nothing owed. + // A guard refusal is mostly a race (the task, its review or its head changed during the job): one short retry + // acts on the new state. One that persists is in mode() too, so the retry finds nothing owed. unsettled = error instanceof OpeningUnsettled || error instanceof GuardRefusal; - const stopped = error instanceof ShuttingDownError || (this.#closing && (error as Error)?.name === 'AbortError'); - record = { outcome: stopped ? 'stopped' : REFUSALS.some(type => error instanceof type) ? 'refused' : 'failed', draft, message: message(error, this.#secrets) }; + const stopped = error instanceof ShuttingDownError || error instanceof PublishCancelled || (this.#closing && (error as Error)?.name === 'AbortError'); + record = { outcome: stopped ? 'stopped' : REFUSALS.some(type => error instanceof type) ? 'refused' : 'failed', draft, message: message(error, this.#secrets), + ...(job.kind === 'close' ? { action: 'close' as const } : {}) }; } try { this.#write(() => this.#store.recordPublish(identity, record, actionId, seenVersion)); } - catch (error) { console.error(`Could not record the publish outcome: ${JSON.stringify(message(error, this.#secrets))}`); } + catch (error) { console.error(`Could not record the pull request outcome: ${JSON.stringify(message(error, this.#secrets))}`); } this.#retryIfUnsettled(identity, record, unsettled); }; + const release = () => { + this.#running.delete(key); + // A publish that the task's cancel stopped, or that ended as the task was cancelled, leaves its PRs to close (#111). + // Started before `done` resolves, so settled() and shutdown see the close too. Only then: a refused publish stays + // owed, and starting it again here would repeat it without end. A close never starts another job. + try { if (job.kind === 'publish' && this.#store.getTask(identity).status === 'cancelled') this.actIfOwed(identity); } + catch (error) { console.error(`Could not start closing pull requests: ${JSON.stringify(message(error, this.#secrets))}`); } + reserved.resolve(); + }; // Starts once the caller's transaction (the action's userAction) has committed; a rollback starts nothing. - this.#store.afterCommit(() => { - void run().finally(() => { this.#running.delete(key); reserved.resolve(); }); - }, () => { this.#running.delete(key); reserved.resolve(); }); + this.#store.afterCommit(() => { void run().finally(release); }, () => { this.#running.delete(key); reserved.resolve(); }); } /** diff --git a/runner/store.ts b/runner/store.ts index 730fb9f5..eb38929f 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -57,12 +57,14 @@ export function mergeActionResponse(attempt: MergeAttempt) { */ export interface PublishRecord { outcome: string; draft: boolean; message: string; stateVersion: number; at: string; number?: number; url?: string; + /** `close`: the record is a cancelled task's PR close (#111), whose settled outcome is `closed`. Absent: a publish. */ + action?: 'close'; /** An `opened` outcome that left the PR or the task not where the publish meant them: still owed (#103). */ reconcile?: boolean; } /** What the publish action replays once its publish has settled: the outcome, as `publish.last` shows it. */ export function publishActionResponse(record: PublishRecord) { - return { outcome: record.outcome, draft: record.draft, message: record.message, ...(record.reconcile ? { reconcile: true } : {}), ...(record.number === undefined ? {} : { number: record.number }), + return { outcome: record.outcome, draft: record.draft, message: record.message, ...(record.action ? { action: record.action } : {}), ...(record.reconcile ? { reconcile: true } : {}), ...(record.number === undefined ? {} : { number: record.number }), ...(record.url === undefined ? {} : { url: record.url }) }; } export interface TaskRecord { @@ -1335,23 +1337,23 @@ export class Store { const saved: PublishRecord = { ...record, message: record.message.slice(0, 2000), stateVersion: seenVersion ?? current, at: new Date().toISOString() }; this.#run('INSERT INTO publish_outcomes (plan_key,data) VALUES (?,?) ON CONFLICT(plan_key) DO UPDATE SET data=excluded.data', key, encode(saved)); // The same transaction refreshes the publish action's replay, as for a merge, so a resent click reports this outcome. - // A reply still saying `publishing` is an action whose publish never recorded an outcome: its process stopped (a - // crash) before it could. One publish runs per task, so this outcome is that action's continuation and settles it - // too; otherwise its replay would say `publishing` for ever. - this.#run(`UPDATE user_actions SET response=? WHERE plan_key=? AND kind='publish' AND json_extract(response,'$.ok')=1 - AND (action_id=? OR json_extract(response,'$.value.outcome')='publishing')`, encode({ ok: true, value: publishActionResponse(saved) }), key, actionId ?? null); + // A reply still saying `publishing` or `closing` is an action whose job never recorded an outcome: its process stopped + // (a crash) before it could. One publish or close runs per task, so this outcome is that action's continuation and + // settles it too; otherwise its replay would say `publishing` or `closing` for ever. + this.#run(`UPDATE user_actions SET response=? WHERE plan_key=? AND kind IN ('publish','close-pull-requests') AND json_extract(response,'$.ok')=1 + AND (action_id=? OR json_extract(response,'$.value.outcome') IN ('publishing','closing'))`, encode({ ok: true, value: publishActionResponse(saved) }), key, actionId ?? null); return saved; }); } - /** Settle every publish action reply still saying `publishing` with `record`, the outcome on record, unchanged. */ + /** Settle every publish or close action reply still saying `publishing` or `closing` with `record`, the outcome on record, unchanged. */ settleUnsettledPublishReplies(identity: PlanIdentity, record: PublishRecord): void { - this.#run(`UPDATE user_actions SET response=? WHERE plan_key=? AND kind='publish' AND json_extract(response,'$.ok')=1 - AND json_extract(response,'$.value.outcome')='publishing'`, encode({ ok: true, value: publishActionResponse(record) }), identityKey(identity)); + this.#run(`UPDATE user_actions SET response=? WHERE plan_key=? AND kind IN ('publish','close-pull-requests') AND json_extract(response,'$.ok')=1 + AND json_extract(response,'$.value.outcome') IN ('publishing','closing')`, encode({ ok: true, value: publishActionResponse(record) }), identityKey(identity)); } - /** Whether a publish action's saved reply still says `publishing`: its publish has not recorded an outcome yet. */ + /** Whether a publish or close action's saved reply still says `publishing` or `closing`: its job has not recorded an outcome yet. */ hasUnsettledPublishAction(identity: PlanIdentity): boolean { - return !!this.#get(`SELECT 1 FROM user_actions WHERE plan_key=? AND kind='publish' AND json_extract(response,'$.ok')=1 - AND json_extract(response,'$.value.outcome')='publishing' LIMIT 1`, identityKey(identity)); + return !!this.#get(`SELECT 1 FROM user_actions WHERE plan_key=? AND kind IN ('publish','close-pull-requests') AND json_extract(response,'$.ok')=1 + AND json_extract(response,'$.value.outcome') IN ('publishing','closing') LIMIT 1`, identityKey(identity)); } lastPublish(identity: PlanIdentity): PublishRecord | null { const key = identityKey(identity); this.#task(key); diff --git a/test/publish.test.ts b/test/publish.test.ts index a7466c87..bace381c 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -41,7 +41,7 @@ const baseOf = new WeakMap, Map>( /** `live` is GitHub's set of open PRs by marker; share it between harnesses to model later runs of the same task. */ function harness(store: Store, options: { results?: AlreadyFixedResult[]; open?: (input: OpenPullRequestInput) => Promise; found?: OpenedPullRequest | null; push?: BranchPusher['push']; live?: Map; next?: { value: number }; config?: Partial; draftAfterRefresh?: boolean; - onFind?: () => void; refreshFails?: boolean; closed?: Set; hidden?: Set; openTimesOut?: boolean; draftFails?: boolean; draftsUnsupported?: boolean; onDraft?: () => void; onRefresh?: () => void; closing?: () => boolean; onCheck?: () => void; moved?: Set; unmarked?: Set; onRead?: () => void; copies?: (OpenedPullRequest & { marker: string })[] } = {}) { + onFind?: () => void; refreshFails?: boolean; closed?: Set; hidden?: Set; openTimesOut?: boolean; draftFails?: boolean; draftsUnsupported?: boolean; onDraft?: () => void; onRefresh?: () => void; closing?: () => boolean; onCheck?: () => void; moved?: Set; unmarked?: Set; onRead?: () => void; onCloseRead?: () => void; copies?: (OpenedPullRequest & { marker: string })[] } = {}) { const publishConfig = { ...config, ...options.config }; // The task's branch, as the publisher names it (one task per harness). let publishBranch = ''; @@ -123,6 +123,17 @@ function harness(store: Store, options: { results?: AlreadyFixedResult[]; open?: if (options.draftFails) throw new Error('timeout marking the PR a draft'); const pr = { ...live.get(input.marker)!, draft: true }; live.set(input.marker, pr); return pr; }, + // Like the adapter: read by number (never the list), refuse an open PR that is no longer the task's, then close. + async close(number, input) { + log.push(`close ${number}`); + const [m] = [...live].find(([, pr]) => pr.number === number) ?? [undefined]; + if (m === undefined || closed.has(number)) return { number, url: 'https://github.com/owner/repo/pull/1' }; + if (options.unmarked?.has(number) || options.moved?.has(number) || m !== input.marker) throw new PullRequestMisplaced(`Pull request #${number} is open but no longer the task's.`); + options.onCloseRead?.(); + input.beforeClose?.(); + closed.add(number); + return { number, url: 'https://github.com/owner/repo/pull/1' }; + }, async refresh(number, input) { log.push(`refresh ${number} ${input.ready ? 'ready' : 'draft'}`); opened.push(input); if ((bases.get(input.marker) ?? input.base) !== input.base) throw new Error('GitHub returned a pull request for a different branch.'); @@ -255,7 +266,7 @@ describe('opening the task PR', () => { } }; const opened: string[] = []; const publisher = new PullRequestPublisher(store, { checks: gate, pusher: { async push() {} }, - pulls: { async open() { opened.push('open'); throw new Error('unreachable'); }, async findOpened() { return null; }, async findOwned() { return []; }, async readPull() { throw new Error('unreachable'); }, async refresh() { throw new Error('unreachable'); }, async markDraft() { throw new Error('unreachable'); } } }, config); + pulls: { async open() { opened.push('open'); throw new Error('unreachable'); }, async findOpened() { return null; }, async findOwned() { return []; }, async readPull() { throw new Error('unreachable'); }, async refresh() { throw new Error('unreachable'); }, async markDraft() { throw new Error('unreachable'); }, async close() { throw new Error('unreachable'); } } }, config); await expect(publisher.publish(identity)).rejects.toThrow(GuardRefusal); expect(opened).toEqual([]); }); @@ -1255,6 +1266,108 @@ describe('gh subprocess environment', () => { }); }); +describe('closing a cancelled task\'s pull requests (#111)', () => { + const cancel = (store: Store) => store.cancelTask(identity, store.getTask(identity).stateVersion, crypto.randomUUID()); + it('closes only a cancelled task\'s PRs', async () => { + const store = runningTask(), { publisher, log } = harness(store); + await expect(publisher.closeAll(identity)).rejects.toThrow(/Only a cancelled task/); + expect(log).toEqual([]); + }); + it('closes a recorded PR by its number, though GitHub\'s list does not show it yet', async () => { + const store = runningTask(), live = new Map(), hidden = new Set(); + await harness(store, { live }).publisher.publish(identity); + for (const marker of live.keys()) hidden.add(marker); + cancel(store); + const closed = new Set(), { publisher, log } = harness(store, { live, hidden, closed }); + expect(await publisher.closeAll(identity)).toEqual([100]); + expect(closed).toEqual(new Set([100])); + expect(log).toEqual(['close 100']); + }); + it('refuses, and leaves open, a PR that is no longer the task\'s', async () => { + const store = runningTask(), live = new Map(); + await harness(store, { live }).publisher.publish(identity); + cancel(store); + const closed = new Set(), { publisher } = harness(store, { live, closed, unmarked: new Set([100]) }); + await expect(publisher.closeAll(identity)).rejects.toThrow(PullRequestMisplaced); + expect(closed.size).toBe(0); + }); + it('reports a PR recorded in another repository instead of recording the close as done', async () => { + const store = runningTask(), live = new Map(); + await harness(store, { live }).publisher.publish(identity); + cancel(store); + // The configuration now names another repository (a rename or a transfer). + const { publisher, log } = harness(store, { live, config: { repository: 'owner/renamed' } }); + await expect(publisher.closeAll(identity)).rejects.toThrow(PullRequestMisplaced); + await expect(publisher.closeAll(identity)).rejects.toThrow('Pull request #100 was opened in owner/repo, not owner/renamed'); + expect(log).toEqual([]); + }); + it('reports how long the lost opening still has to settle, on the clock recovery uses', async () => { + const store = runningTask(), live = new Map(); + await expect(harness(store, { live, openTimesOut: true }).publisher.publish(identity)).rejects.toThrow('timeout'); + const created = Date.parse(store.taskPullRequests(identity)[0]!.createdAt); + expect(harness(store, { live, config: { settleMs: 60_000, now: () => created + 59_500 } }).publisher.settleRemaining(identity)).toBe(500); + expect(harness(store, { live, config: { settleMs: 60_000, now: () => created + 61_000 } }).publisher.settleRemaining(identity)).toBe(0); + expect(harness(runningTask()).publisher.settleRemaining(identity)).toBeNull(); + }); + /** A cancelled task whose opening was abandoned (GitHub showed nothing in time) and whose PR GitHub shows now. */ + async function abandonedThenCancelled() { + const store = runningTask(), live = new Map(), hidden = new Set(); + await expect(harness(store, { live, openTimesOut: true }).publisher.publish(identity)).rejects.toThrow('timeout'); + for (const marker of live.keys()) hidden.add(marker); + // The settle time has passed with nothing visible: the opening is abandoned, and the main path goes on (it opens a + // second PR into the base, which GitHub refuses while the hidden one is open; any outcome will do here). + await harness(store, { live, hidden, config: { settleMs: 0 } }).publisher.publish(identity).catch(() => undefined); + expect(store.taskPullRequests(identity).filter(pr => pr.number === null).map(pr => pr.state)).toEqual(expect.arrayContaining(['abandoned'])); + expect(store.taskPullRequests(identity).every(pr => pr.state === 'abandoned')).toBe(true); + cancel(store); + return { store, live, marker: [...live.keys()][0]! }; + } + it('adopts and closes an abandoned opening\'s PR that GitHub shows now', async () => { + const { store, live } = await abandonedThenCancelled(), closed = new Set(); + const { publisher, log } = harness(store, { live, closed }); + expect(await publisher.closeAll(identity)).toContain(100); + expect(closed.has(100)).toBe(true); + expect(log).toContain('close 100'); + expect(store.taskPullRequests(identity)).toContainEqual(expect.objectContaining({ state: 'opened', number: 100 })); + }); + it('closes neither of two PRs that carry an abandoned opening\'s marker, and refuses', async () => { + const { store, live, marker } = await abandonedThenCancelled(), closed = new Set(); + const copy = { number: 999, url: 'https://github.com/owner/repo/pull/999', headSha: oid(8), draft: false, marker }; + const { publisher } = harness(store, { live, closed, copies: [copy] }); + await expect(publisher.closeAll(identity)).rejects.toThrow(/carry the first line of a pull request the task was opening, so codeboost closed neither/); + expect(closed.has(100) || closed.has(999)).toBe(false); + expect(store.taskPullRequests(identity).every(pr => pr.state === 'abandoned')).toBe(true); + }); + it('still closes the next PR when an earlier one cannot be closed, and reports the problem', async () => { + // Two distinct recorded PRs: #100 from the first run, closed by a person, then #101 from the rerun. + const store = runningTask(), live = new Map(), next = { value: 100 }, closed = new Set(); + await harness(store, { live, next, closed }).publisher.publish(identity); + closed.add(100); + rerun(store); + await harness(store, { live, next, closed }).publisher.publish(identity); + expect(store.taskPullRequests(identity).map(pr => pr.number)).toEqual([100, 101]); + // #100 is reopened, but without its first-line marker: it is no longer the task's, so its close is refused. + closed.delete(100); + cancel(store); + const { publisher, log } = harness(store, { live, next, closed, unmarked: new Set([100]) }); + const failure = await publisher.closeAll(identity).then(() => null, (error: unknown) => error); + expect(failure).toBeInstanceOf(PullRequestMisplaced); + expect((failure as Error).message).toMatch(/^Pull requests #101 closed\. Pull request #100 is open but no longer the task's\./); + expect(log).toEqual(['close 100', 'close 101']); + expect(closed.has(101)).toBe(true); + expect(closed.has(100)).toBe(false); + }); + it('closes nothing when shutdown begins between the read and the close', async () => { + const store = runningTask(), live = new Map(); + await harness(store, { live }).publisher.publish(identity); + cancel(store); + let closing = false; + const closed = new Set(), { publisher } = harness(store, { live, closed, closing: () => closing, onCloseRead: () => { closing = true; } }); + await expect(publisher.closeAll(identity)).rejects.toThrow(ShuttingDownError); + expect(closed.size).toBe(0); + }); +}); + describe('a plan revision during a publish (#103 review)', () => { it('refuses a ready publish when a revision adds an item while its first GitHub lookups run', async () => { const store = runningTask(), live = new Map(), hidden = new Set(); @@ -1399,6 +1512,48 @@ describe('GitHub PR adapter', () => { expect(await new GhPullRequestGateway({ repository: 'owner/repo' }, async () => JSON.stringify([response({ body: `${marker}\nplan quoting ${other}` })])) .findOpened({ ...input, markers: [other, marker] })).toMatchObject({ marker }); }); + it('closes the task\'s PR with a PATCH, after checking its branch, repository and marker, and requires GitHub to show it closed (#111)', async () => { + const calls: string[][] = [], inputs: (string | undefined)[] = []; + let state = 'open'; + const gh = new GhPullRequestGateway({ repository: 'owner/repo' }, async (args, options) => { + calls.push([...args]); inputs.push(options?.input); + if (args.includes('PATCH')) state = 'closed'; + return JSON.stringify(response({ state })); + }); + const close = { headBranch: 'codeboost/issue-12-task', marker }; + expect(await gh.close(7, close)).toEqual({ number: 7, url: 'https://github.com/owner/repo/pull/7' }); + expect(calls).toEqual([['api', '-H', 'Accept: application/vnd.github+json', 'repos/owner/repo/pulls/7'], + ['api', '-X', 'PATCH', '-H', 'Accept: application/vnd.github+json', 'repos/owner/repo/pulls/7', '--input', '-']]); + expect(JSON.parse(inputs[1]!)).toEqual({ state: 'closed' }); + // Already closed: success, with no PATCH. + calls.length = 0; + expect(await gh.close(7, close)).toMatchObject({ number: 7 }); + expect(calls).toHaveLength(1); + // GitHub answered the PATCH but still shows the PR open. + await expect(new GhPullRequestGateway({ repository: 'owner/repo' }, async () => JSON.stringify(response())).close(7, close)).rejects.toThrow(/did not close/); + }); + it('never closes a PR that is no longer the task\'s: another branch, a fork, no marker, another number (#111)', async () => { + for (const over of [{ head: { sha: oid(2), ref: 'other', repo: { full_name: 'owner/repo' } } }, { head: { sha: oid(2), ref: 'codeboost/issue-12-task', repo: { full_name: 'fork/repo' } } }, + { head: { sha: oid(2), ref: 'codeboost/issue-12-task', repo: null } }, { body: 'no marker' }, { body: `plan\n${marker}` }, { number: 8 }]) { + const calls: string[][] = []; + const gh = new GhPullRequestGateway({ repository: 'owner/repo' }, async args => { calls.push([...args]); return JSON.stringify(response(over)); }); + await expect(gh.close(7, { headBranch: 'codeboost/issue-12-task', marker }), JSON.stringify(over)).rejects.toThrow(); + expect(calls.some(call => call.includes('PATCH')), JSON.stringify(over)).toBe(false); + } + // A closed PR is done, whoever edited it since. + expect(await new GhPullRequestGateway({ repository: 'owner/repo' }, async () => JSON.stringify(response({ state: 'closed', body: 'edited' }))) + .close(7, { headBranch: 'codeboost/issue-12-task', marker })).toMatchObject({ number: 7 }); + }); + it('re-checks right before the close, and a close that landed is not failed by an edited description (#111)', async () => { + let state = 'open'; + const calls: string[][] = []; + const run = async (args: readonly string[]) => { calls.push([...args]); if (args.includes('PATCH')) state = 'closed'; + return JSON.stringify(response(args.includes('PATCH') ? { state, body: 'edited by a person' } : { state })); }; + await expect(new GhPullRequestGateway({ repository: 'owner/repo' }, run).close(7, { headBranch: 'codeboost/issue-12-task', marker, beforeClose: () => { throw new Error('shutting down'); } })) + .rejects.toThrow('shutting down'); + expect(calls.some(call => call.includes('PATCH'))).toBe(false); + expect(await new GhPullRequestGateway({ repository: 'owner/repo' }, run).close(7, { headBranch: 'codeboost/issue-12-task', marker })).toMatchObject({ number: 7 }); + }); it('runs gh with the environment it was given, so its GH_HOST matches the push\'s', async () => { const dir = mkdtempSync(join(tmpdir(), 'codeboost-gh-')); try { diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index 4cac8fbf..d92f787b 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -6,7 +6,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { fixtureGit as git } from './fixtures/git.ts'; import { createDemo } from '../scripts/demo.ts'; import { startServer } from '../web/server.ts'; -import { Store } from '../runner/store.ts'; +import { Store, type PublishRecord } from '../runner/store.ts'; import { GuardRefusal } from '../runner/lifecycle.ts'; import type { ReviewService } from '../runner/review.ts'; import type { PreparedAttempt, RunnerDeps } from '../runner/coordinator.ts'; @@ -40,6 +40,7 @@ class FakeGitHub { prs: Pr[] = []; calls: string[] = []; onOpen?: (signal?: AbortSignal) => Promise; + onClose?: (number: number, signal?: AbortSignal) => Promise; /** Runs inside a draft change, before it applies; a throw fails the change. */ onDraft?: () => void; /** PRs GitHub's list does not show yet (it lags behind them). */ @@ -77,6 +78,17 @@ class FakeGitHub { findOwned: async input => this.prs.filter(pr => pr.open && !this.hidden.has(pr.number) && pr.headBranch === input.headBranch && input.markers.includes(pr.marker)) .map(pr => ({ ...this.#view(pr), marker: pr.marker, base: pr.base })), readPull: async number => { const pr = this.prs.find(candidate => candidate.number === number)!; return { open: pr.open, headBranch: pr.headBranch, base: pr.base, marker: pr.marker }; }, + // Like the adapter: read by number, closed is done, an open PR must still be the task's, then the re-check and close. + close: async (number, input, signal) => { + this.calls.push(`close ${number}`); + const pr = this.prs.find(candidate => candidate.number === number)!; + if (!pr.open) return { number, url: pr.url }; + if (pr.headBranch !== input.headBranch || pr.marker !== input.marker) throw new PullRequestMisplaced('Not the task\'s pull request.'); + input.beforeClose?.(); + await this.onClose?.(number, signal); + pr.open = false; + return { number, url: pr.url }; + }, markDraft: async number => { this.onDraft?.(); const pr = this.prs.find(candidate => candidate.number === number)!; pr.draft = true; return this.#view(pr); }, refresh: async (number, input) => { this.calls.push(`refresh ${input.draft ? 'draft' : 'ready'}`); @@ -149,7 +161,7 @@ function world(): World { * real publisher and the real pusher into `w.remote`, with GitHub faked. `before` shapes the Store before the runner * exists, as an 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; env?: NodeJS.ProcessEnv; settleMs?: number; shortRetryMs?: number } = {}) { +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 } = {}) { let app: App | undefined, spawns = 0, closing: Promise | undefined; const close = () => closing ??= app!.close(); let branchOf: (identity: PlanIdentity) => string = () => ''; @@ -162,7 +174,8 @@ async function serve(w: World, options: { before?: (service: ReviewService) => v const prepared: PreparedAttempt = { clone: { id: 'clone', taskId: 'task', directory: '/tmp/x', head: 'f'.repeat(40) }, vendor: 'claude', approvedArgv: [] }; const deps: RunnerDeps = { runnerOwner: OWNER, kinds: ['execute'], prepare: async () => prepared, cleanupPreparation: async () => undefined, start: input => ({ attemptId: input.attemptId, cancel: () => undefined, - settled: Promise.resolve({ attemptId: input.attemptId, context: input.context, exitCode: 0, signal: null, stdout: '', stderr: '' }) }), + // `hold`: the attempt keeps running until the test releases it. + settled: (options.hold ?? Promise.resolve()).then(() => ({ attemptId: input.attemptId, context: input.context, exitCode: 0, signal: null, stdout: '', stderr: '' })) }), validate: () => ({ head: service.store.getSnapshot(service.config.identity).head, unchanged: true, inScope: [], outOfScope: [] }) }; const sources: ExecutionSources = { planContext: () => service.planContext(), issue: () => ({ number: 3, title: '', body: '', comments: [] }), lessons: () => [], vendor: () => 'claude' }; const pusher = new GitBranchPusher({ repository, repositoryId: service.config.identity.repositoryId, remote: REPO, url: w.remote, @@ -431,8 +444,11 @@ describe('publishing a finished task (#103)', () => { store.cancelTask(identity, seen, randomUUID()); throw new Error('GitHub timed out.'); }; + const records = vi.spyOn(store, 'recordPublish'); await publishSettled(app, identity); - expect(store.lastPublish(identity)).toMatchObject({ outcome: 'opened', stateVersion: seen, message: expect.stringContaining('may still be ready for review') }); + // The cancel then closes the PR (#111), so the publish's own record is read from what was recorded, not the last one. + const opened = records.mock.results.map(result => result.value as PublishRecord).find(record => record.outcome === 'opened'); + expect(opened).toMatchObject({ stateVersion: seen, message: expect.stringContaining('may still be ready for review') }); expect(store.getTask(identity).stateVersion).toBeGreaterThan(seen); }); @@ -797,11 +813,15 @@ describe('publishing a finished task (#103)', () => { w.github.onOpen = async () => { cancel?.(); }; const first = await serve(w, { before: completeAll, startup: false }); cancel = () => first.store.cancelTask(first.identity, first.store.getTask(first.identity).stateVersion, randomUUID()); + const records = vi.spyOn(first.store, 'recordPublish'); first.app.publishOwed(() => undefined); - await publishSettled(first.app, first.identity); + await first.app.publishing!.settled(first.identity); expect(first.store.getTask(first.identity).status).toBe('cancelled'); - expect(first.store.lastPublish(first.identity)).toMatchObject({ outcome: 'opened' }); - expect(first.store.lastPublish(first.identity)!.reconcile).toBeUndefined(); + // The publish's own outcome carries no reconcile; then, the task being cancelled, its PR is closed (#111). + const opened = records.mock.calls.map(call => call[1]).find(record => record.outcome === 'opened'); + expect(opened).toMatchObject({ outcome: 'opened' }); + expect(opened!.reconcile).toBeUndefined(); + expect(first.store.lastPublish(first.identity)).toMatchObject({ outcome: 'closed', action: 'close' }); // A publish action whose PR GitHub shows at another head: its saved reply, replayed, says reconcile too. const v = world(); v.github.staleHeadOnce = 'a'.repeat(40); @@ -833,10 +853,10 @@ describe('publishing a finished task (#103)', () => { await publishSettled(app, identity); expect(w.github.prs).toEqual([expect.objectContaining({ draft: true, body: expect.stringContaining('The last item wrote outside its workspace.') })]); // A replay of that refusal applies nothing: no payment, no publish. - const pay = vi.spyOn(app.executor!, 'payOwed'), publishIfOwed = vi.spyOn(app.publishing!, 'publishIfOwed'); + const pay = vi.spyOn(app.executor!, 'payOwed'), actIfOwed = vi.spyOn(app.publishing!, 'actIfOwed'); expect(await act(app, 'publish', actionId, version)).toMatchObject({ status: 409, body: { error: OWED_REFUSAL } }); expect(pay).not.toHaveBeenCalled(); - expect(publishIfOwed).not.toHaveBeenCalled(); + expect(actIfOwed).not.toHaveBeenCalled(); }); it('pays a safety finding owed on a needs-human task before its draft, at startup', async () => { const w = world(); @@ -865,7 +885,7 @@ describe('publishing a finished task (#103)', () => { store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 1, valid: false, detail: 'The tests failed in retry.ts.' }); expect(() => admit(store.getTask(identity).budgetDeadline! + 1)).toThrow(/time budget/); expect(store.getTask(identity).status).toBe('needs human'); - app.publishing!.publishIfOwed(identity); + app.publishing!.actIfOwed(identity); await publishSettled(app, identity); expect(w.github.prs).toEqual([expect.objectContaining({ draft: true, body: expect.stringContaining('The tests failed in retry.ts.') })]); expect(w.github.prs[0]!.body).not.toContain('The last item wrote outside its workspace.'); @@ -879,7 +899,7 @@ describe('publishing a finished task (#103)', () => { app.publishOwed(() => undefined); await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject({ outcome: 'failed' }), { timeout: 5_000 }); // A run's end starts a new chain while the first chain's retry is armed. - app.publishing!.publishIfOwed(identity); + app.publishing!.actIfOwed(identity); await new Promise(resolve => setTimeout(resolve, 6_000)); // The first publish, the new chain's, and that chain's one retry; the first chain's timer never fires. expect(publishes()).toBe(3); @@ -957,3 +977,239 @@ describe('github.baseBranch', () => { expect(() => baseBranch(undefined)).toThrow(/github\.baseBranch/); }); }); + +describe('closing a cancelled task\'s pull requests (#111)', () => { + const closedRecord = { outcome: 'closed', action: 'close' }; + it('closes a ready PR when a task in review is cancelled, and keeps the branch', async () => { + const w = world(); + const { app, identity, store, branch, head } = await serve(w, { before: completeAll }); + await publishSettled(app, identity); + expect(store.getTask(identity).status).toBe('in review'); + expect((await act(app, 'cancel-task')).body.result).toEqual({ outcome: 'closed' }); + await app.publishing!.settled(identity); + expect(w.github.prs).toEqual([expect.objectContaining({ number: 100, open: false, draft: false })]); + expect(w.github.head(branch)).toBe(head); + expect(store.getTask(identity).status).toBe('cancelled'); + expect(store.lastPublish(identity)).toMatchObject({ ...closedRecord, message: 'Pull request #100 closed; the task\'s branch is kept.' }); + // Nothing is owed after a close that closed them. + expect((await view(app)).publish).toMatchObject({ active: false, closable: false, publishable: false }); + // Nothing is owed any more: a restart closes nothing. + await app.publishing!.settled(identity); + const calls = [...w.github.calls]; + const again = await serve(w); + expect(await quiet(again.app, w, identity)).toEqual(calls); + }); + + it('closes the draft PR of a cancelled needs-human task', async () => { + const w = world(); + const { app, identity, store } = await serve(w, { before: needsHuman }); + await publishSettled(app, identity); + expect(w.github.prs).toEqual([expect.objectContaining({ draft: true, open: true })]); + await act(app, 'cancel-task'); + await app.publishing!.settled(identity); + expect(w.github.prs).toEqual([expect.objectContaining({ draft: true, open: false })]); + expect(store.lastPublish(identity)).toMatchObject(closedRecord); + }); + + it('cancels at once during a draft opening, stops the publish, and closes the PR GitHub opened meanwhile', async () => { + const w = world(); + const opening = Promise.withResolvers(); + // GitHub creates the PR; the reply never arrives before the cancel aborts the call. + w.github.onOpen = signal => new Promise((_, reject) => { + signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }); + opening.resolve(); + }); + const { app, identity, store } = await serve(w, { before: needsHuman }); + await opening.promise; + expect(store.taskPullRequests(identity)).toMatchObject([{ state: 'opening' }]); + const records = vi.spyOn(store, 'recordPublish'); + // The cancel is not refused and does not wait for GitHub. + expect(await act(app, 'cancel-task')).toMatchObject({ status: 200, body: { result: { outcome: 'closed' } } }); + expect(store.getTask(identity).status).toBe('cancelled'); + await app.publishing!.settled(identity); + // The publish was recorded as stopped by the cancel, then the close: in flight, then done. + expect(records.mock.calls.map(call => [call[1].outcome, call[1].action])).toEqual([['stopped', undefined], ['closing', 'close'], ['closed', 'close']]); + expect(records.mock.calls[0]![1].message).toBe('The task was cancelled; its pull requests are closed instead.'); + // Recovered by its marker, recorded, and closed. + expect(w.github.prs).toEqual([expect.objectContaining({ number: 100, open: false })]); + expect(store.taskPullRequests(identity)).toMatchObject([{ state: 'opened', number: 100 }]); + expect(store.lastPublish(identity)).toMatchObject({ ...closedRecord, message: expect.stringMatching(/^Pull request #100 closed/) }); + expect(store.getTask(identity).status).toBe('cancelled'); + }); + + it('records a failed close as owed, and the close-pull-requests action retries it; its replay reports the outcome', async () => { + const w = world(); + let fail = true; + w.github.onClose = async () => { if (fail) { fail = false; throw new Error('GitHub is down.'); } }; + const { app, identity, store } = await serve(w, { before: needsHuman }); + await publishSettled(app, identity); + await act(app, 'cancel-task'); + await app.publishing!.settled(identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'failed', action: 'close', message: 'GitHub is down.' }); + expect(w.github.prs[0]!.open).toBe(true); + expect((await view(app)).publish).toMatchObject({ closable: true, last: { outcome: 'failed' } }); + const actionId = randomUUID(), version = store.getTask(identity).stateVersion; + expect(await act(app, 'close-pull-requests', actionId, version)).toMatchObject({ status: 200, body: { result: { outcome: 'closing' } } }); + await app.publishing!.settled(identity); + expect(w.github.prs[0]!.open).toBe(false); + expect((await act(app, 'close-pull-requests', actionId, version)).body.result).toMatchObject(closedRecord); + }); + + it('closes the PRs at the next start when shutdown stopped the close', async () => { + const w = world(); + let closeApp: (() => Promise) | undefined, first = true; + w.github.onClose = (_, signal) => { + if (!first) return Promise.resolve(); + first = false; + return new Promise((_, reject) => { signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }); void closeApp!(); }); + }; + const s1 = await serve(w, { before: needsHuman }); + await publishSettled(s1.app, s1.identity); + closeApp = s1.close; + await act(s1.app, 'cancel-task'); + await vi.waitFor(() => expect(first).toBe(false)); + await closeApp(); + const store = new Store(w.demo.database); + try { expect(store.lastPublish(s1.identity)).toMatchObject({ outcome: 'stopped', action: 'close' }); } finally { store.close(); } + expect(w.github.prs[0]!.open).toBe(true); + const s2 = await serve(w); + await s2.app.publishing!.settled(s2.identity); + expect(w.github.prs[0]!.open).toBe(false); + expect(s2.store.lastPublish(s2.identity)).toMatchObject(closedRecord); + }); + + it('closes a recorded PR that GitHub\'s list does not show yet, by its number', async () => { + const w = world(); + const { app, identity, store } = await serve(w, { before: needsHuman }); + await publishSettled(app, identity); + w.github.hidden.add(100); + await act(app, 'cancel-task'); + await app.publishing!.settled(identity); + expect(w.github.prs[0]!.open).toBe(false); + expect(store.lastPublish(identity)).toMatchObject(closedRecord); + }); + + it('starts no close when a cancel is replayed or refused, though a close is owed', async () => { + const w = world(); + w.github.onClose = async () => { throw new Error('GitHub is down.'); }; + const { app, identity, store } = await serve(w, { before: needsHuman }); + await publishSettled(app, identity); + const actionId = randomUUID(), version = store.getTask(identity).stateVersion; + await act(app, 'cancel-task', actionId, version); + await app.publishing!.settled(identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'failed', action: 'close' }); + const calls = [...w.github.calls]; + expect((await act(app, 'cancel-task', actionId, version)).body.result).toEqual({ outcome: 'closed' }); + expect(await act(app, 'cancel-task')).toMatchObject({ status: 409, body: { error: 'The task is already closed.' } }); + expect(app.publishing!.busy(identity)).toBe(false); + expect(w.github.calls).toEqual(calls); + }); + + it('retries a close refused while the cancelled opening settles, and closes the PR GitHub showed late', async () => { + const w = world(); + const opening = Promise.withResolvers(); + let late: Pr | undefined; + // GitHub creates the PR, but its list does not show it until after the first close was refused. + w.github.onOpen = signal => new Promise((_, reject) => { + late = w.github.prs.at(-1); w.github.hidden.add(late!.number); + signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }); + opening.resolve(); + }); + const { app, identity, store } = await serve(w, { before: needsHuman, settleMs: 300 }); + await opening.promise; + await act(app, 'cancel-task'); + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject({ outcome: 'refused', action: 'close' }), { timeout: 10_000 }); + w.github.hidden.delete(late!.number); + // Retried by itself once the opening has settled (300 ms + 1 s here). + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject(closedRecord), { timeout: 10_000, interval: 50 }); + expect(late!.open).toBe(false); + expect(store.taskPullRequests(identity)).toMatchObject([{ state: 'opened', number: late!.number }]); + }); + + it('retries a refused close at the opening\'s own settle deadline, not a fresh settle time from the cancel', async () => { + const w = world(); + const opening = Promise.withResolvers(); + let late: Pr | undefined; + w.github.onOpen = signal => new Promise((_, reject) => { + late = w.github.prs.at(-1); w.github.hidden.add(late!.number); + signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }); + opening.resolve(); + }); + const { app, identity, store } = await serve(w, { before: needsHuman, settleMs: 8_000 }); + await opening.promise; + // The opening is already about 6.5 s old (of its 8 s) when the task is cancelled. + await new Promise(resolve => setTimeout(resolve, 6_500)); + await act(app, 'cancel-task'); + const cancelled = Date.now(); + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject({ outcome: 'refused', action: 'close' }), { timeout: 5_000 }); + w.github.hidden.delete(late!.number); + // About 1.5 s left plus the 1 s margin: closed well before a fresh 8 s window (9 s) would have retried it. + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject(closedRecord), { timeout: 6_000, interval: 50 }); + expect(Date.now() - cancelled).toBeLessThan(7_000); + expect(late!.open).toBe(false); + }); + + it('cancels promptly while an attempt runs, closes nothing until it settles, then closes the PR', async () => { + const w = world(), hold = Promise.withResolvers(); + const { app, identity, store } = await serve(w, { hold: hold.promise, before: service => { + // The first item failed and the task went to a person (no budget involved), so a draft PR is published at startup. + const s = service.store, id = service.config.identity; + s.transitionTask(id, s.getTask(id).stateVersion, 'queued'); + const attempt = s.admitAttempt(id, { expectedStateVersion: s.getTask(id).stateVersion, kind: 'execute', item: s.getPlan(id).items[0]!.id, + expectedContext: s.currentContext(id), deadline: Date.now() + 60_000 }); + s.markRunning(id, attempt.id); + s.settleAttempt(id, attempt.id, { firstReason: null, exitCode: 1, valid: false, detail: 'The tests failed.' }); + s.transitionTask(id, s.getTask(id).stateVersion, 'needs human'); + } }); + await publishSettled(app, identity); + expect(w.github.prs).toEqual([expect.objectContaining({ draft: true, open: true })]); + // A person sends it back to the queue and resumes it; the attempt keeps running. + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + expect((await act(app, 'resume')).body.result).toMatchObject({ outcome: 'started' }); + const calls = [...w.github.calls]; + expect((await act(app, 'cancel-task')).body.result).toEqual({ outcome: 'stopping' }); + // Nothing is closed while the attempt runs: the task is not cancelled until it settles. + expect(store.getTask(identity).status).not.toBe('cancelled'); + expect(app.publishing!.busy(identity)).toBe(false); + expect(w.github.calls).toEqual(calls); + expect(w.github.prs[0]!.open).toBe(true); + hold.resolve(); + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject(closedRecord), { timeout: 20_000, interval: 50 }); + expect(store.getTask(identity).status).toBe('cancelled'); + expect(w.github.prs[0]!.open).toBe(false); + }); + + it('runs a close asked for after a PR was reopened, when its process stopped before it recorded anything', async () => { + const w = world(), actionId = randomUUID(); + const first = await serve(w, { before: needsHuman }); + await publishSettled(first.app, first.identity); + await act(first.app, 'cancel-task'); + await first.app.publishing!.settled(first.identity); + expect(first.store.lastPublish(first.identity)).toMatchObject(closedRecord); + await first.close(); + // A person reopens the PR and asks for a close; that process stops right after saving the action. + w.github.prs[0]!.open = true; + const crashed = new Store(w.demo.database); + let version = -1; + try { + version = crashed.getTask(first.identity).stateVersion; + crashed.userAction(first.identity, { actionId, kind: 'close-pull-requests', request: { attemptId: undefined, expectedStateVersion: version } }, () => ({ outcome: 'closing' })); + } finally { crashed.close(); } + const second = await serve(w); + await second.app.publishing!.settled(second.identity); + expect(w.github.prs[0]!.open).toBe(false); + expect((await act(second.app, 'close-pull-requests', actionId, version)).body.result).toMatchObject({ outcome: 'closed', action: 'close' }); + }); + + it('calls GitHub for nothing when a task without any PR is cancelled, and each action refuses the other\'s status', async () => { + const w = world(); + const { app, identity, store } = await serve(w); + expect(await act(app, 'close-pull-requests')).toMatchObject({ status: 409, body: { error: expect.stringMatching(/only a cancelled task's pull requests are closed/) } }); + await act(app, 'cancel-task'); + expect(app.publishing!.busy(identity)).toBe(false); + expect(w.github.calls).toEqual([]); + expect(store.lastPublish(identity)).toBeNull(); + expect(await act(app, 'close-pull-requests')).toMatchObject({ status: 409, body: { error: 'The task has no pull request to close.' } }); + expect(await act(app, 'publish')).toMatchObject({ status: 409, body: { error: 'The task is cancelled; its pull requests are closed, not published.' } }); + }); +}); diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index 25765855..b7ef8425 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -229,7 +229,7 @@ describe('/api/runner', () => { const first = await api(app, 'POST', '/api/runner', cancel); expect(first).toMatchObject({ status: 200, body: { result: { outcome: 'closed' }, runner: { task: { status: 'cancelled' } } } }); expect((await api(app, 'POST', '/api/runner', cancel)).body.result).toEqual(first.body.result); - expect((await api(app, 'POST', '/api/runner', { ...cancel, action: 'retry', attemptId: randomUUID() }))).toMatchObject({ status: 409, body: { error: /different request/ } }); + expect((await api(app, 'POST', '/api/runner', { ...cancel, action: 'retry', attemptId: randomUUID() }))).toMatchObject({ status: 409, body: { error: expect.stringMatching(/different request/) } }); expect((await api(app, 'POST', '/api/runner', { ...cancel, actionId: 'not-a-uuid' })).status).toBe(400); }); it('answers 400 for a malformed runner request and records nothing under its action ID', async () => { diff --git a/web/server.ts b/web/server.ts index d4376db5..afa140eb 100644 --- a/web/server.ts +++ b/web/server.ts @@ -172,19 +172,22 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge }; /** The task's publishing (#103): in progress, offered (what the publish action would run), and the last outcome. */ const publishView = (progress?: ReturnType) => { - if (!publishing) return { available: false, active: false, publishable: false, draft: false, last: null }; - let mode: { draft: boolean } | null = null; - try { mode = publishing.mode(identity, progress); } catch (error) { if (!(error instanceof GuardRefusal) && !(error instanceof ShuttingDownError)) throw error; } - return { available: true, active: publishing.busy(identity), publishable: mode !== null, draft: mode?.draft ?? false, last: publishing.lastOutcome(identity) }; + if (!publishing) return { available: false, active: false, publishable: false, closable: false, draft: false, last: null }; + let job: ReturnType | null = null; + try { job = publishing.mode(identity, progress); } catch (error) { if (!(error instanceof GuardRefusal) && !(error instanceof ShuttingDownError)) throw error; } + // `closable`: the task is cancelled and a close of its PRs is owed (#111): not after one that closed them. + const last = publishing.lastOutcome(identity); + return { available: true, active: publishing.busy(identity), publishable: job?.kind === 'publish', closable: job?.kind === 'close' && last?.outcome !== 'closed', + draft: job?.kind === 'publish' && job.draft, last }; }; /** A run's outcome is in the task and attempt rows; once it ends, the task's status decides whether a publish is owed. */ const afterRun = (outcome: Promise) => void outcome .catch(error => console.error(`Runner run failed: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`)) - .finally(() => publishing?.publishIfOwed(identity)); + .finally(() => publishing?.actIfOwed(identity)); const runnerAction = (input: Record) => { const { action, attemptId, expectedStateVersion, actionId } = input; // Malformed requests are refused before userAction, so nothing is recorded under their action ID (HTTP 400). - if (!['cancel-attempt', 'retry', 'cancel-task', 'start', 'resume', 'publish'].includes(action as string)) throw new BadRequest('Unsupported runner action.'); + if (!['cancel-attempt', 'retry', 'cancel-task', 'start', 'resume', 'publish', 'close-pull-requests'].includes(action as string)) throw new BadRequest('Unsupported runner action.'); if (!Number.isSafeInteger(expectedStateVersion)) throw new BadRequest('expectedStateVersion must be an integer.'); if (action === 'cancel-attempt' || action === 'retry') assertUuidV4(attemptId, 'Attempt ID'); // A refused start or resume can still move the task to needs human (an expired budget is committed with the refusal), @@ -192,7 +195,14 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // task already in needs human asked for no publish. if (action === 'start' || action === 'resume') { const before = service.store.getTask(identity).status; - try { return act(); } finally { if (service.store.getTask(identity).status !== before) publishing?.publishIfOwed(identity, { personAsked: true }); } + try { return act(); } finally { if (service.store.getTask(identity).status !== before) publishing?.actIfOwed(identity, { personAsked: true }); } + } + // A cancel that closed the task stops its publish in progress and closes its PRs (#111). A cancel that is still + // stopping an attempt closes the task when the attempt settles; the run's end then closes them (afterRun). + // Only this action's own cancel: a replayed or refused one (the task already closed) starts nothing. + if (action === 'cancel-task') { + const before = service.store.getTask(identity).status; + try { return act(); } finally { if (before !== 'cancelled' && service.store.getTask(identity).status === 'cancelled') publishing?.taskCancelled(identity); } } // A publish refused because an earlier run owes a pause or an escalation pays it here, outside the refused transaction // (AGENTS.md: a refusal's durable change is committed outside it); a task it sends to needs human then gets its draft. @@ -203,7 +213,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge try { replay = !!service.store.savedAction(identity, { actionId: actionId as string, kind: 'publish', request: { attemptId, expectedStateVersion } }); } catch { replay = true; } try { return act(); } - catch (error) { if (!replay && error instanceof GuardRefusal && error.message === OWED_REFUSAL) publishing?.publishIfOwed(identity, { personAsked: true }); throw error; } + catch (error) { if (!replay && error instanceof GuardRefusal && error.message === OWED_REFUSAL) publishing?.actIfOwed(identity, { personAsked: true }); throw error; } } return act(); function act() { return service.store.userAction(identity, { actionId: actionId as string, kind: action as string, request: { attemptId, expectedStateVersion } }, () => { @@ -225,7 +235,14 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // Retries a publish that failed or was refused (#103); it runs after this action commits, in the background. if (!publishing) throw new GuardRefusal(config.demo ? 'Demos never publish pull requests.' : 'This runner does not publish pull requests.'); // Replaced by the publish's outcome once it settles (recordPublish), so a replay reports that. - return { outcome: 'publishing', draft: publishing.request(identity, actionId as string).draft }; + const job = publishing.request(identity, 'publish', actionId as string); + return { outcome: 'publishing', draft: job.kind === 'publish' && job.draft }; + } + if (action === 'close-pull-requests') { + // Retries closing a cancelled task's PRs (#111) after a failure, a refusal or a stop; in the background, like publish. + if (!publishing) throw new GuardRefusal(config.demo ? 'Demos never publish pull requests.' : 'This runner does not publish pull requests.'); + publishing.request(identity, 'close', actionId as string); + return { outcome: 'closing' }; } if (action === 'cancel-attempt') { if (!runner.stop(identity, attemptId as string, 'cancelled')) throw new GuardRefusal('That attempt is not running.'); @@ -236,6 +253,10 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (executor && last.kind === 'execute') throw new GuardRefusal('A plan item is run again by resuming the task, not by retrying its attempt.'); const retry = runner.retry(identity, attemptId as string, { expectedStateVersion: expectedStateVersion as number, kind: last.kind, item: last.item, expectedContext: service.store.currentContext(identity), deadline: Date.now() + 10 * 60_000 }); + // A cancel that stops this attempt closes the task when it settles; its PRs are closed then (#111), as after a run. + // Only then: any other settlement asked for no publish, so an owed (refused) one is not started again here. + void runner.settled(identity).then(() => { if (service.store.getTask(identity).status === 'cancelled') publishing?.actIfOwed(identity); }) + .catch(error => console.error(`Could not start closing pull requests: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`)); return { outcome: 'started', attemptId: retry.id }; }).response; } };