diff --git a/docs/implementation/pull-request-opening.md b/docs/implementation/pull-request-opening.md index bab22404..6115e433 100644 --- a/docs/implementation/pull-request-opening.md +++ b/docs/implementation/pull-request-opening.md @@ -129,6 +129,7 @@ Some repositories do not support draft PRs (for example private repositories on `TaskPublishing` (`runner/publishing.ts`) owns publishing in production. `setUpRunner` builds the publisher with the real gateways (`GhAlreadyFixedGateway`, `GhPullRequestGateway`) and a `GitBranchPusher` that pushes from the runner-owned repository to `pushUrl(github.repository)` on `GH_HOST`, never to a configured URL. The pusher may overwrite only commits the task's ledger records as `owned`. Before #22 adds rebasing, nothing in production writes a `foreign` ledger entry (only `recordRebase` does), so codeboost pushes no head the ledger calls foreign. +- **What is published.** The whole task head, `base..head` (#113). Before the runner's first commit, the review records the user's checkout, so a task usually starts from the branch's own commits: a person made them, and the ledger has no entry for them, so they count as foreign (the design: a commit not in the ledger is foreign). The runner's commits come on top. All of them reach the `codeboost/…` branch and the PR, as the design intends. Review shows a foreign commit's changes in the Unplanned row, and the merge gate blocks (`unplanned`) until a person assigns or accepts each one. The gate inspects and merges the PR named in `github.pullRequest`, not yet the task's published PR, so this protects the published PR only when the two are the same; pointing the gate at the published PR is a follow-up. The push may overwrite only an owned commit already on the branch. Mapping commits through a rebase is #22's. - **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. @@ -150,7 +151,7 @@ The process group is therefore not recorded, and startup does not look for it (d ## 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. `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. 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/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index 5dabde22..aac2b77a 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -15,6 +15,8 @@ import { GitBranchPusher } from '../runner/branch-push.ts'; import { PullRequestPublisher } from '../runner/publish.ts'; import { ensureCommit, openRunnerRepository } from '../runner/runner-repository.ts'; import { baseBranch } from '../runner/production.ts'; +import { MergeCoordinator } from '../runner/merge.ts'; +import type { MergeGateway } from '../github/merge.ts'; import { OWED_REFUSAL } from '../runner/publishing.ts'; import { PullRequestMisplaced, type OpenPullRequestInput, type PullRequestGateway } from '../github/pull-requests.ts'; import type { AlreadyFixedGateway } from '../github/already-fixed.ts'; @@ -387,6 +389,26 @@ describe('publishing a finished task (#103)', () => { try { expect(store.taskPullRequests(first.identity)).toMatchObject([{ state: 'opened', number: 100 }]); } finally { store.close(); } }); + it('publishes the whole task head, the branch\'s own commits too, which review shows as Unplanned and the merge gate blocks (#113)', async () => { + const w = world(); + const { app, identity, store, branch, head } = await serve(w, { before: completeAll }); + await publishSettled(app, identity); + // The demo branch's last commit was made by a person, not by codeboost: the ledger does not record it. + const owned = store.getLedger(identity).filter(entry => entry.origin === 'owned').map(entry => entry.sha); + expect(owned).not.toContain(head); + // It is published as it is: the PR's branch is the whole head. + expect(w.github.head(branch)).toBe(head); + expect(w.github.prs).toEqual([expect.objectContaining({ open: true, draft: false })]); + // Review shows its change in the Unplanned row, and the merge gate blocks until a person assigns or accepts it. + const view = app.service.load(); + expect(view.segments.filter(segment => segment.row === 'Unplanned').map(segment => segment.path)).toContain('debug.log'); + const gateway: MergeGateway = { inspect: async () => ({ base: view.snapshot.base, head: view.snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', + rulesKnown: true, atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear' }), merge: async () => { throw new Error('not merged here'); } }; + const status = await new MergeCoordinator(app.service, gateway).status(view); + expect(status).toMatchObject({ ready: false }); + expect(status.blockers).toContainEqual(expect.objectContaining({ code: 'unplanned' })); + }); + it('removes the publishing environment\'s token values from a recorded failure', async () => { const w = world(), token = 'not-a-github-shaped-secret-4711'; w.github.onOpen = async () => { throw new Error(`gh failed: Authorization: token ${token}`); };