F: stamp a publish outcome with the state version the publish last saw (#114)

docs/implementation/pull-request-opening.md

@@ -133,7 +133,7 @@ Some repositories do not support draft PRs (for example private repositories on
 - **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. 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`.
+- **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.
 - **Demo.** A demo never publishes, even with a `github` block or an injected publisher.
 

runner/publish.ts

@@ -22,7 +22,7 @@ export interface PublishConfig {
 /** An earlier opening's outcome is still unknown; nothing new is opened until it settles. Retry later. */
 export class OpeningUnsettled extends Error {}
 export const DEFAULT_SETTLE_MS = 10 * 60_000;
-export type PublishOutcome =
+export type PublishOutcome = (
   /** `leftReady`: the PR opened or was updated, but could not be made a draft although its task is not in review. */
   | { kind: 'opened'; number: number; url: string; draft: boolean; status: string; leftReady?: number }
   /** `leftReady`: the task's earlier PR could not be made a draft because the repository does not support drafts. */
@@ -38,7 +38,14 @@ export type PublishOutcome =
    * The task head is its base: there is nothing to open a PR for. A running task moves to needs human. `leftReady`: the
    * task's earlier PR could not be made a draft because the repository does not support drafts.
    */
-  | { kind: 'no changes'; leftReady?: number };
+  | { kind: 'no changes'; leftReady?: number }) & {
+  /**
+   * The task state version this publish last saw (#114): where a path ends right after a Store write or guarded read,
+   * the version read there, with no await since; where a GitHub call came after its last write, the version from before
+   * that call, since a change someone else made during it was not seen. Required, so every return path decides it.
+   */
+  seenVersion: number;
+};
 
 const marker = (openingId: string) => `<!-- codeboost:opening=${openingId} -->`;
 const pullRequestList = (prs: readonly { number: number }[]) => `Pull requests ${prs.map(pr => `#${pr.number}`).join(' and ')}`;
@@ -225,7 +232,8 @@ export class PullRequestPublisher {
       // A cancelled publish still records a draft change that landed (above), but moves no task.
       signal?.throwIfAborted();
       if (!draft) this.#store.transitionTask(identity, stateVersion, 'needs human');
-      return leftReady === undefined ? { kind: 'no changes' } : { kind: 'no changes', leftReady };
+      const seenVersion = this.#store.getTask(identity).stateVersion;
+      return leftReady === undefined ? { kind: 'no changes', seenVersion } : { kind: 'no changes', leftReady, seenVersion };
     }
     const own = new Set(prs.filter(pr => pr.number !== null && pr.repository.toLowerCase() === this.#config.repository.toLowerCase()).map(pr => pr.number!));
     if (live) own.add(live.number);
@@ -250,13 +258,19 @@ export class PullRequestPublisher {
     const check = this.#store.recordAlreadyFixed(identity, stateVersion, { snapshotId: snapshot.id, reviewVersion, draft, result });
     if (drafted && earlier!.state === 'opened') this.#store.recordPullRequestDraft(identity, earlier!.openingId, drafted.number, drafted.draft, { stateVersion: check.stateVersion, reviewVersion: check.reviewVersion });
     const ready = leftReady === undefined ? {} : { leftReady };
-    if (result.outcome !== 'clear') return draft ? { kind: 'draft skipped', result, ...ready } : { kind: 'possibly already fixed', result, ...ready };
+    if (result.outcome !== 'clear') {
+      // Right after recording the check (and any draft change), with no await since.
+      const seenVersion = this.#store.getTask(identity).stateVersion;
+      return draft ? { kind: 'draft skipped', result, ...ready, seenVersion } : { kind: 'possibly already fixed', result, ...ready, seenVersion };
+    }
     if (earlier && live) {
       // A needs-human task's ready PR becomes a draft first, before anything else about it changes: if the repository
       // has no drafts, the refusal comes while the PR is still exactly as it was (no push, no new description).
       if (draft && !live.draft) {
+        // Read before the call: nothing is written after it on the refusal path (#114).
+        const seenVersion = this.#store.getTask(identity).stateVersion;
         try { await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId) }, signal); }
-        catch (error) { if (error instanceof DraftsUnsupported) return { kind: 'draft unsupported', number: live.number }; throw error; }
+        catch (error) { if (error instanceof DraftsUnsupported) return { kind: 'draft unsupported', number: live.number, seenVersion }; throw error; }
         signal?.throwIfAborted();
       }
       // The push is a refresh's first content write (it moves the open PR's head), so the refresh is recorded before it;
@@ -300,7 +314,7 @@ export class PullRequestPublisher {
       if (error instanceof PullRequestRefused) { this.#store.abandonPullRequestOpening(identity, opening.openingId); throw error; }
       if (!(error instanceof DraftsUnsupported)) throw error;
       this.#store.abandonPullRequestOpening(identity, opening.openingId);
-      return { kind: 'draft unsupported', number: null };
+      return { kind: 'draft unsupported', number: null, seenVersion: this.#store.getTask(identity).stateVersion };
     }
     const status = this.#store.recordPullRequestOpened(identity, opening.openingId, pr);
     return this.#settleHead(identity, opening.openingId, pr, snapshot.head, draft, status, branch, signal);
@@ -315,7 +329,10 @@ export class PullRequestPublisher {
   async #settleHead(identity: PlanIdentity, openingId: string, pr: { number: number; url: string; headSha: string; draft: boolean }, head: string,
     draft: boolean, status: string, branch: string, signal?: AbortSignal): Promise<PublishOutcome> {
     const base = this.#config.baseBranch;
-    const opened = { kind: 'opened' as const, number: pr.number, url: pr.url, draft: pr.draft, status };
+    // The version right after the open or refresh was recorded (no await since); on the leftReady path nothing is written
+    // after the draft call, so a change made during it was not seen (#114).
+    const seenVersion = this.#store.getTask(identity).stateVersion;
+    const opened = { kind: 'opened' as const, number: pr.number, url: pr.url, draft: pr.draft, status, seenVersion };
     // Left ready only for a ready publish whose task is now in review at the pushed head; a draft publish's PR is always
     // a draft, whatever GitHub returned.
     if (pr.draft || (!draft && status === 'in review' && pr.headSha === head)) return opened;
@@ -327,7 +344,8 @@ export class PullRequestPublisher {
     // A fact about the PR, recorded against the versions read right now (no await since).
     this.#store.recordPullRequestDraft(identity, openingId, drafted.number, drafted.draft,
       { stateVersion: this.#store.getTask(identity).stateVersion, reviewVersion: this.#store.reviewVersion(identity) });
-    return { kind: 'opened', number: drafted.number, url: drafted.url, draft: drafted.draft, status: this.#store.getTask(identity).status };
+    const task = this.#store.getTask(identity);
+    return { kind: 'opened', number: drafted.number, url: drafted.url, draft: drafted.draft, status: task.status, seenVersion: task.stateVersion };
   }
 
   /** The task may keep a ready PR: it is in review, approved (GitHub merges no draft) or merged. Read with no await since. */

runner/publishing.ts

@@ -247,19 +247,20 @@ export class TaskPublishing {
       // 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.' })); }
       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<PublishRecord, 'stateVersion' | 'at'>, unsettled = false;
+      let record: Omit<PublishRecord, 'stateVersion' | 'at'>, 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;
       } 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.
         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) };
       }
-      try { this.#write(() => this.#store.recordPublish(identity, record, actionId)); }
+      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))}`); }
       this.#retryIfUnsettled(identity, record, unsettled);
     };