F: preserve unsaved safety finding reasons (#128)

Author: mchwang

docs/implementation/pull-request-opening.md

@@ -130,7 +130,7 @@ 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. With a runner block, the gate inspects and merges the task's published PR (#121), so this blocker protects that PR. The push may overwrite only an owned commit already on the branch. Mapping commits through a rebase is #22's.
 - **Base branch.** `github.baseBranch` is required with a runner block. The `gh` calls run with the same environment as the push (its `GH_HOST` and credentials). The CLI checks it before it takes the lock, and `setUpRunner` checks it again before anything is created.
-- **Mode.** The task's status decides it. A `running` task whose every plan item has completed at the current revision gets a ready PR. A task in `needs human`, by any path (a safety finding, the time budget), gets a draft PR. Its problems say what sent it there now: its current attempt's safety finding (or, when that finding's save failed, the attempt's diagnostic that carries it), else "time budget ran out" when the budget has passed, else that attempt's diagnostic. A budget that ran out after a safety finding does not hide the finding. An earlier attempt's finding, which a person may have dealt with, is not used. Any other status, an active attempt, a run still in progress, or a publish already in progress refuses. A draft waits only for an owed safety finding, not an owed scope pause: a draft invites no review, and a task in needs human cannot record a pause. A ready publish also needs every item of the current plan revision to have completed at that revision. The Store's publish guard checks this before every guarded write, so a revision that adds an item while a publish awaits GitHub refuses it. So does work an earlier run still owes: a safety finding not yet acted on, or a scope pause never recorded (its write failed, or the process stopped). Every plan item has run, so no later run would pay it, and a ready PR would show unreviewed out-of-scope changes. A run's end, startup and a refused `publish` action therefore have the executor pay it first (`ItemExecutor.payOwed`, as a run does before its first item). The task then moves to needs amendment or needs human, and a draft follows for the latter. For a task already in needs human, only an owed safety finding is paid. That draft names the paid finding, which only this process holds. It does so only while the task stays in needs human on the same current attempt. A later run that returns the task to needs human has its own reason.
+- **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: the reason `ItemExecutor` remembers whenever it escalates the current attempt, then that attempt's saved safety finding, else "time budget ran out" when the budget has passed, else that attempt's diagnostic. The remembered reason covers every finding source (preparation, a successful run's audit, and a failed run's audit) when its Store save failed and exists only while the process does. 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: the remembered reason applies only while the task stays in needs human on the same current attempt. 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. A later run that returns the task to needs human has its own reason.
 - **When.** Once a run started by `start` or `resume` has ended, with no attempt active (`assertPublishable` refuses one). On the `publish` runner action (`expectedStateVersion` and an action ID, like the other runner actions), which retries a refused or failed publish. When a refused `start` or `resume` moves the task to needs human (an expired budget is committed with the refusal); a refused action that leaves the status as it was publishes nothing, so pressing resume on a task already in needs human never pushes. Once at startup, if a publish is owed: the CLI starts it through `publishOwed`, which first runs the check that the runner lock still names the database, because a publish pushes; if the check fails, nothing starts. A task recorded as `not published` by the v10 upgrade stays unpublished until a person uses the `publish` action, or until a run starts after the upgrade (its end is the first state the upgrade did not decide), or a person's `start` or `resume` moves the task. Otherwise a publish is owed when the task can be published and no publish has settled (`opened`, `possibly already fixed`, `draft skipped`, `draft unsupported`, `no changes`) at the task's current state version. So a refusal, a failure or a stop leaves it owed, and the next start recovers a lost opening by its marker. A publish or close that ends with an opening still in flight (its own reply was lost, or it was refused while one settles) is tried again at that opening's own deadline, plus a second (`settleRemaining`; a timer, cleared at shutdown). A short retry, after 30 seconds, follows a publish refused because GitHub's PR list lags behind a recorded PR, one refused by a guard (mostly a race: the task, its review or its head changed during the publish; a refusal that persists is in the mode too, so the retry finds nothing owed), one refused just as an opening's deadline passed, and one whose open or update left work behind. That last case covers GitHub showing another head than the one pushed (the PR is made a draft and the task stays running), and a draft change that failed. Such an `opened` outcome is recorded with `reconcile`, which keeps the publish owed. Retries are bounded: one timer per task, and per chain at most one retry at an opening's deadline (while it is still ahead) and one short retry. A chain ends when a job settles. A new one starts with a run's end, a cancel, a person's action or a restart, never with a retry's own job. Whatever is still unsettled after them is stuck for a reason a person must fix (two PRs carry a marker, the repository changed, GitHub keeps failing every open), so the next publish or close waits for a run's end, a restart or an action.
 - **Outcome.** Each publish runs in the background, one per task, and records its outcome in `publish_outcomes` (schema v10): the outcome kind, or `refused` (a guard, `BranchPushRefused`, `OpeningUnsettled`, `PullRequestMisplaced`, `PullRequestRefused`), `failed` or `stopped` (shutdown), with its message (any token value or token shape removed, as for the push) and the task's state version. The record does not move the task's state version. Its state version is the one the publish last saw (`seenVersion`, #114), which every outcome carries. Where a path ends right after a Store write or guarded read, it is the version read there. Where a GitHub call came after the last write (a draft change that failed and left the PR ready, or one refused because drafts are unsupported), it is the version from before that call. So a change someone else made during the call still leaves a publish owed. A refusal, a failure or a stop is stamped with the current version, and stays owed anyway. A close is stamped with the current version. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. Before it starts, every publish or close (after a run, a cancel, at startup or on an action) records itself as `publishing` or `closing`, its durable in-flight ownership. If that write fails, the publish does not run. A `publishing` or `closing` record left at startup is a publish whose process stopped before it recorded an outcome. Startup runs the job again if one is owed, and otherwise settles the record as `stopped`; the task's PR records show what that job did. When only an action reply is stuck (its job never recorded itself as started), the reply reports the outcome on record, and that record is left as it is. A failure while paying owed work does not stop this. A reply that still says `publishing` (or `closing`) belongs to an action whose process stopped (a crash) before its publish recorded an outcome. Any later outcome of the task settles it too, since one publish or close runs per task. At startup, if no publish is owed, it is settled as `stopped` ("interrupted before it recorded an outcome"), so no replay says `publishing` or `closing` for ever. The v10 upgrade records every task already `running` or in `needs human` as `not published`, a settled outcome: the first start after the upgrade opens no PR a person did not ask for, and the `publish` action publishes it. `GET /api/runner` shows it as `publish.last`, with `publish.active` and `publish.publishable`.
 - **Start and resume** refuse while a publish of the task is in progress, so a new item cannot move the head being pushed. A second `publish` is refused too, and the refusal is saved under its action ID. Cancel task is not refused: it stops the publish, and the close below handles what the publish left.

runner/execution.ts

@@ -280,6 +280,7 @@ const direct = <T>(fn: () => T): T => fn();
  */
 export class ItemExecutor {
   #store: Store; #runner: RunnerCoordinator; #sources: ExecutionSources; #findings: SafetyFindings;
+  #escalations = new Map<string, { reason: string; attemptId: string | null }>();
   #deadlineMs: number;
   /** F2's status changes after an attempt settles are settlement writes: they still land after the shutdown gate closes. */
   #write: <T>(fn: () => T) => T;
@@ -329,6 +330,16 @@ export class ItemExecutor {
   }
   /** Whether a run of this task is still in progress here (its last write may still be to come). */
   busy(identity: PlanIdentity): boolean { return this.#inFlight.has(identityKey(identity)); }
+  /**
+   * The reason this process most recently moved the task to needs human. An unsaved finding exists nowhere else after
+   * #escalate settles it, so publishing reads it here. It applies only while that same attempt still owns the human gate.
+   */
+  escalationReason(identity: PlanIdentity): string | undefined {
+    const key = identityKey(identity), escalation = this.#escalations.get(key), task = this.#store.getTask(identity);
+    if (escalation && task.status === 'needs human' && escalation.attemptId === task.currentAttemptId) return escalation.reason;
+    this.#escalations.delete(key);
+    return undefined;
+  }
   /**
    * Whether an earlier run left work owed that the next run pays before any item: a safety finding not yet acted on, or
    * a scope pause never recorded (a failed write, the write gate, a crash). Reads only.
@@ -481,6 +492,12 @@ export class ItemExecutor {
   #escalate(identity: PlanIdentity, row: AttemptRecord, violation: string,
     stopped: (item: string, state: string, reason: string | null) => ExecutionOutcome, done: string[], owed = false): ExecutionOutcome {
     const item = row.item!, task = this.#store.getTask(identity);
+    const remember = () => {
+      const current = this.#store.getTask(identity), key = identityKey(identity);
+      if (current.status === 'needs human' && current.currentAttemptId === row.id)
+        this.#escalations.set(key, { reason: violation, attemptId: row.id });
+      else this.#escalations.delete(key);
+    };
     // The terminal write failed: the attempt still counts as active, so the move waits for restart; keep the finding owed.
     if (row.state === 'pending' || row.state === 'running') {
       const unresolved = this.#runner.status(identity).unresolved;
@@ -489,8 +506,9 @@ export class ItemExecutor {
     // A saved finding: the terminal write acted on it, so the task went to needs human then (or was closed by a pending
     // cancel), and where it is now is a person's doing.
     if (!this.#findings.unsaved(row.id)) {
-      return CLOSED_STATUSES.includes(task.status) ? stopped(item, row.state, `${violation} The task is ${task.status}, so it was not moved to needs human.`)
-        : { kind: 'needs human', item, reason: violation, completed: [...done] };
+      if (CLOSED_STATUSES.includes(task.status)) return stopped(item, row.state, `${violation} The task is ${task.status}, so it was not moved to needs human.`);
+      remember();
+      return { kind: 'needs human', item, reason: violation, completed: [...done] };
     }
     if (CLOSED_STATUSES.includes(task.status)) {
       this.#findings.settle(row.id);
@@ -499,6 +517,7 @@ export class ItemExecutor {
     // Already where the finding sends it: nothing is owed.
     if (task.status === 'needs human') {
       this.#findings.settle(row.id);
+      remember();
       return { kind: 'needs human', item, reason: violation, completed: [...done] };
     }
     if (HUMAN_GATES.includes(task.status))
@@ -511,7 +530,7 @@ export class ItemExecutor {
       return stopped(item, owed ? 'not started' : row.state, `${violation} The task could not be moved to needs human yet: ${(error as Error).message}`);
     }
     // Inside a caller's transaction (begin, #91 part 2) the move can still roll back; the finding stays owed until it commits.
-    this.#store.afterCommit(() => this.#findings.settle(row.id));
+    this.#store.afterCommit(() => { this.#findings.settle(row.id); remember(); });
     return { kind: 'needs human', item, reason: violation, completed: [...done] };
   }
   /** The earliest completed execute attempt whose out-of-scope files have no checkpoint yet (its pause was lost). */

runner/publishing.ts

@@ -2,7 +2,7 @@ import { identityKey, type PlanIdentity } from '../core/identity.ts';
 import { PullRequestMisplaced, PullRequestRefused } from '../github/pull-requests.ts';
 import { BranchPushRefused, redact, TOKEN_VARIABLES } from './branch-push.ts';
 import type { RunnerCoordinator } from './coordinator.ts';
-import { SAFETY_VIOLATION, type ItemExecutor } from './execution.ts';
+import type { ItemExecutor } from './execution.ts';
 import { GuardRefusal, ShuttingDownError, settleWith, type ShutdownCapability } from './lifecycle.ts';
 import { OpeningUnsettled, type PublishOutcome, type PullRequestPublisher } from './publish.ts';
 import type { PublishRecord, Store } from './store.ts';
@@ -50,11 +50,6 @@ export class TaskPublishing {
    */
   #retried = new Map<string, { deadline: number; short: number }>();
   #shortRetryMs: number;
-  /**
-   * The reason an owed escalation was paid with (a safety finding whose save had failed): the executor settles it in
-   * 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<string, { reason: string; attemptId: string | null }>();
   /** Jobs in progress, by task, from scheduling until the outcome is recorded; `abort` stops a publish on cancel. */
   #running = new Map<string, { job: PullRequestJob; done: Promise<void>; abort: AbortController }>();
   constructor(store: Store, publisher: PullRequestPublisher, runner: RunnerCoordinator, executor: ItemExecutor, capability?: ShutdownCapability,
@@ -224,8 +219,6 @@ export class TaskPublishing {
     try {
       // A task in needs human cannot record a scope pause, and its draft does not wait for one (mode): only a finding.
       const paid = this.#executor.payOwed(identity, { scope: this.#store.getTask(identity).status !== 'needs human' });
-      // Tied to the attempt current once paid: a later run (a new attempt) that ends in needs human again has its own reason.
-      if (paid?.kind === 'needs human') this.#escalations.set(identityKey(identity), { reason: paid.reason, attemptId: this.#store.getTask(identity).currentAttemptId });
       if (paid?.kind === 'stopped') console.error(`Could not settle what an earlier run owes: ${JSON.stringify(paid.reason ?? '')}`);
     } catch (error) { console.error(`Could not settle what an earlier run owes: ${JSON.stringify(message(error, this.#secrets))}`); }
   }
@@ -329,15 +322,12 @@ export class TaskPublishing {
    */
   #problems(identity: PlanIdentity): string[] {
     const task = this.#store.getTask(identity);
-    // An owed finding paid here first: only this process knows its text. Only while the task is still in needs human from
-    // that payment (the same current attempt); once a later run has started, the finding is an earlier attempt's.
-    const key = identityKey(identity), escalated = this.#escalations.get(key);
-    if (escalated && task.status === 'needs human' && escalated.attemptId === task.currentAttemptId) return [escalated.reason];
-    this.#escalations.delete(key);
+    // ItemExecutor remembers every escalation it makes because an unsaved finding exists nowhere else after it is settled.
+    // It returns that reason only while this task is still at the human gate on the same current attempt.
+    const escalated = this.#executor.escalationReason(identity);
+    if (escalated) return [escalated];
     const current = task.currentAttemptId === null ? undefined : this.#store.getAttempt(identity, task.currentAttemptId);
     if (current?.safetyFinding) return [current.safetyFinding];
-    // A finding whose save failed while the run still moved the task: the attempt's diagnostic is the finding's text.
-    if (current?.diagnostic?.startsWith(SAFETY_VIOLATION)) return [current.diagnostic];
     if (task.budgetDeadline !== null && task.budgetDeadline <= Date.now()) return ['The task\'s time budget ran out before its plan finished.'];
     if (current?.diagnostic) return [current.diagnostic];
     return ['The task needs a person.'];

test/runner-execution.test.ts

@@ -570,6 +570,7 @@ describe('item execution', () => {
     expect(atRelease).toBe('needs human');
     const row = store.getAttempts(identity)[0]!;
     expect(row).toMatchObject({ state: 'failed', safetyFinding: expect.stringContaining('Git metadata') });
+    expect(h.executor.escalationReason(identity)).toBeUndefined();
   });
   it('treats an audit that throws as a safety violation', async () => {
     const { store, executor, commits } = setup({ pathKeyError: new Error('Non-ASCII case-insensitive paths require an adapter.') });