diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 44cd1a37..2289a2ee 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -324,12 +324,14 @@ This runs before the coordinator opens. ## HTTP and UI contract -**Status reads.** `GET /api/runner` returns only the task and attempt rows plus `available`, `stateVersion`, `retryable`, `startable`, `resumable`, `unresolved` and `stopRequested`. `stopRequested` comes from the in-memory job: null, or `{ attemptId, reason, saved }`, where `saved` is false while the first-reason write has failed. The UI shows "Stopping (not saved yet)" only from this field. It does not change `stateVersion`, so user actions still compare against the durable state version. `unresolved` is computed by the server from the in-memory marker: null, or `{ attemptId, reason: "result-not-saved" | "start-not-saved" | "preparation-not-removed" | "storage-not-removed" }`: the terminal write or the start could not be saved; the host-side preparation files could not be removed (before the terminal write when D never ran, after it once D settled); or the task storage could not be removed after a saved terminal write. The UI shows "Needs restart" with that cause only from this field, because the durable row alone may still look active. It does not rebuild Git history or the full review. +**Status reads.** `GET /api/runner` returns only the task and attempt rows plus `available`, `stateVersion`, `reviewVersion`, `retryable`, `startable`, `resumable`, `unresolved` and `stopRequested`. `stopRequested` comes from the in-memory job: null, or `{ attemptId, reason, saved }`, where `saved` is false while the first-reason write has failed. The UI shows "Stopping (not saved yet)" only from this field. It does not change `stateVersion`, so user actions still compare against the durable state version. `unresolved` is computed by the server from the in-memory marker: null, or `{ attemptId, reason: "result-not-saved" | "start-not-saved" | "preparation-not-removed" | "storage-not-removed" }`: the terminal write or the start could not be saved; the host-side preparation files could not be removed (before the terminal write when D never ran, after it once D settled); or the task storage could not be removed after a saved terminal write. The UI shows "Needs restart" with that cause only from this field, because the durable row alone may still look active. It does not rebuild Git history or the full review. -**Start and resume (#91 part 2).** `POST /api/runner` with `action: "start"` or `"resume"` sends `expectedStateVersion` and an `actionId`, but no attempt ID. Both run the plan through `ItemExecutor`, item by item. Everything up to the first item's admission happens inside the user action's transaction, so a refusal rolls back the action's own writes. +**Start and resume (#91 part 2, #107).** `POST /api/runner` with `action: "start"` or `"resume"` sends `expectedStateVersion`, `expectedReviewVersion` and an `actionId`, but no attempt ID. Both versions are part of the idempotent request and must still match before the action writes. Both run the plan through `ItemExecutor`, item by item. Everything up to the first item's admission happens inside the user action's transaction, so a refusal rolls back the action's own writes. +- Both require an approval for every item at the current plan revision. Before an item completes, those approvals must belong to the current snapshot. After a completed item, approvals survive only from the snapshots that the completed execution prefix actually used (or the current snapshot); the completed-prefix head guard below then distinguishes that expected runner movement from an unrelated moved-and-returned head. Each attribution choice and approval is stamped with the review version at which it was saved, and approvals must be later than the latest applicable choice. The review UI marks any approval this execution gate rejects as stale so the reviewer can replace it. Schema v12 stamps legacy unversioned choices at a finite boundary, clears legacy unversioned approvals so the UI offers reapproval, and advances the review version once so fresh approvals can follow. - `start` runs a task that is `in review` or `queued`, has no execute attempt at its current plan revision, and has no pending requeue (that is `resume`'s). An `in review` task moves to `queued` in the same action. -- `resume` continues a task that is `running` or `queued` and has started running (it has an execute attempt at any plan revision, or recovery left it to requeue). It starts at the first item with no completed execute attempt at the current revision, whether the last item completed, failed or was stopped, and claims `requeue_pending` in the admitting transaction. This is also how a task stopped between items continues. -- Both refuse a closed task, a pending cancel, a runner that cannot run execute attempts, an active attempt, an earlier run still finishing, an active merge, a scope pause (#88), and a plan revised after an earlier revision's items made runner commits (#88: those commits must be reconciled with the new revision first; earlier attempts that committed nothing do not block). During shutdown both answer 503 before any of these, and record nothing. When none of these applies, a finding or scope pause owed from an earlier run is acted on first, and nothing is admitted (`outcome: "settled"`). Only then is the first item admitted: admission refuses a spent task budget, which also moves an idle `running` or `queued` task to `needs human` (a task still `in review` stays there: its move to `queued` rolls back with the refused action). +- `resume` continues a task that is `running` or `queued` and has started running (it has an execute attempt at any plan revision, or recovery left it to requeue). It starts at the first item with no completed execute attempt at the current revision, whether the last item completed, failed or was stopped, and claims `requeue_pending` in the admitting transaction. Start never consumes a recovery claim, including when safety or scope work is owed. A completed prefix must still end at the task's current head; a moved head refuses instead of silently skipping earlier items. When owed work settles without an admission, that same transaction claims and clears `requeue_pending`. This is also how a task stopped between items continues. +- Both refuse a closed task, a pending cancel, a runner that cannot run execute attempts, an active attempt, an earlier run still finishing, an active merge, a scope pause (#88), and a plan revised after an earlier revision's items made runner commits (#88: those commits must be reconciled with the new revision first; earlier attempts that committed nothing do not block). During shutdown both answer 503 before any of these, and record nothing. Before the pending-cancel, active-merge and #88 refusals, a safety finding owed from an earlier run is persisted and acted on, and nothing is admitted (`outcome: "settled"`); this prevents an in-memory finding from being lost at restart. An existing human gate is preserved: schema v13 persists the evidence and an explicit owed marker, restart restores the pending escalation, and it remains owed until the person resolves that gate. An owed scope pause is offered and acted on only when the task status, pending-cancel state and merge state allow `pauseForAmendment` to commit it; otherwise the ordinary blocker is shown. Only then is the first item admitted: admission refuses a spent task budget, which also moves an idle `running` or `queued` task to `needs human` (a task still `in review` stays there: its move to `queued` rolls back with the refused action). +- The action owns the review version for the whole multi-item run. Before each later item is admitted, `ItemExecutor` requires that version to be unchanged and rechecks the current-revision approvals. A choice, approval edit or note saved while an earlier item runs therefore stops the run before the next item; the person must review and resume from fresh state. - `GET /api/runner` adds `startable` and `resumable`, computed by the same checks, so the view never offers an action that would be refused. The view also withholds them while the task budget has run out (the action may still act on owed work, or else lets admission refuse it with its move to `needs human`) and while the runner is shutting down or needs a restart. Retrying an execute attempt is refused: a plan item runs again only through `resume`. **User actions.** Cancel, retry and "run again" requests send `attemptId`, `expectedStateVersion` and an `actionId` idempotency key (see "Feedback-event contract"). A replayed `actionId` returns the saved outcome. The server takes the plan identity from its trusted configuration, never from the request, and every `Store` call is scoped by that identity. A mismatch returns HTTP 409 with the current state. The UI then shows that state and keeps any draft. diff --git a/runner/execution.ts b/runner/execution.ts index 1f97fbc3..4f459825 100644 --- a/runner/execution.ts +++ b/runner/execution.ts @@ -69,13 +69,17 @@ export const SAFETY_VIOLATION = 'Safety violation:'; * them, so agent output (stderr) can never be mistaken for one, and a violation survives a later stale or stop outcome. * They are durable (#87 item 3): each is saved on its attempt before the terminal write, which then sends the task to * needs human in the same transaction, however the attempt was started; startup recovery does the same for an attempt - * a crash interrupted. Only a finding whose save failed is held here, in memory, owed until the executor acts on it. + * a crash interrupted. A finding whose save failed, or whose later save could not cross a human gate, is held here until + * the executor can finish acting on it. */ export class SafetyFindings { #store: Store; #write: (fn: () => T) => T; #unsaved = new Map(); /** `capability` is the coordinator's: the save is a settlement write, so it still lands after the write gate closes. */ - constructor(store: Store, capability?: ShutdownCapability) { this.#store = store; this.#write = settleWith(capability); } + constructor(store: Store, capability?: ShutdownCapability) { + this.#store = store; this.#write = settleWith(capability); + for (const owed of store.owedSafetyFindings()) this.#unsaved.set(owed.attemptId, owed.finding); + } #row(attemptId: string): AttemptRecord | undefined { const key = this.#store.attemptOwner(attemptId); return key ? this.#store.getAttempt(findIdentity(this.#store, { id: attemptId } as AttemptRecord), attemptId) : undefined; @@ -88,14 +92,28 @@ export class SafetyFindings { } catch { /* not saved: held here instead */ } this.#unsaved.set(attemptId, reason); } - /** The finding still owed: one whose save failed, so nothing but the executor will act on it. */ + /** The finding still owed; its evidence may already be durable while a human gate defers the escalation. */ get(attemptId: string): string | undefined { return this.#unsaved.get(attemptId); } /** The attempt's finding, owed or already acted on by the terminal write. */ finding(attemptId: string): string | undefined { return this.#unsaved.get(attemptId) ?? this.#row(attemptId)?.safetyFinding ?? undefined; } - /** Whether the finding is only in memory: the Store knows nothing of it, so the executor must act on it itself. */ - unsaved(attemptId: string): boolean { return this.#unsaved.has(attemptId); } + /** Whether the executor still owes the finding's escalation. */ + owed(attemptId: string): boolean { return this.#unsaved.has(attemptId); } settle(attemptId: string): void { this.#unsaved.delete(attemptId); } - /** The attempts whose finding is owed (only in memory). Usually none, so a check over them costs nothing. */ + /** Retry the durable save and, when its human gate is gone, finish the escalation. */ + persist(attemptId: string, options: { settlement?: boolean } = {}): boolean { + const finding = this.#unsaved.get(attemptId); + if (!finding) return true; + const identity = findIdentity(this.#store, { id: attemptId } as AttemptRecord); + const save = () => this.#store.recordOwedSafetyFinding(identity, attemptId, finding, + options.settlement ? { deferAction: true } : {}); + try { if (!(options.settlement ? this.#write(save) : save())) return false; } + catch (error) { if (error instanceof ShuttingDownError) return false; throw error; } + this.#unsaved.delete(attemptId); + // A strict begin usually runs inside userAction's outer transaction. Restore the in-memory debt if that rolls back. + this.#store.afterCommit(() => undefined, () => this.#unsaved.set(attemptId, finding)); + return true; + } + /** The attempts whose finding is owed, including durable markers restored at construction. Usually none. */ owedAttempts(): string[] { return [...this.#unsaved.keys()]; } } export interface ExecutionResult { head: string; unchanged: boolean; inScope: string[]; outOfScope: string[] } @@ -267,6 +285,7 @@ export type ExecutionOutcome = /** A run in progress: its plan, the item it is on and that item's attempt, and what it finished. */ interface Run { plan: ReturnType; index: number; attempt: AttemptRecord; done: string[]; unchanged: string[]; + reviewVersion?: number; stopped: (item: string, state: string, reason: string | null) => ExecutionOutcome; } const refusal = (error: unknown) => error instanceof GuardRefusal || error instanceof ShuttingDownError; @@ -309,7 +328,7 @@ export class ItemExecutor { * throws (the refusal itself where admission refused), and the caller's own writes roll back with it. A finding or scope * pause owed from an earlier run is acted on instead, as the run's whole outcome, and admits nothing (`attemptId` null). */ - begin(identity: PlanIdentity, options: { fromItem?: string; claimRequeue?: boolean } = {}): { attemptId: string | null; outcome: Promise } { + begin(identity: PlanIdentity, options: { fromItem?: string; claimRequeue?: boolean; expectedReviewVersion?: number } = {}): { attemptId: string | null; outcome: Promise } { const key = identityKey(identity); if (this.#inFlight.has(key)) throw new GuardRefusal('An earlier run of this task is still finishing; try again when that run has ended.'); const begun = this.#begin(identity, options, true); @@ -322,11 +341,13 @@ export class ItemExecutor { * reconciled with (#88), the items an execute attempt completed at the current revision, and the first item none did * (null when every item has run). */ - progress(identity: PlanIdentity): { started: boolean; begun: boolean; earlierCommits: boolean; completed: string[]; next: string | null } { - const plan = this.#store.getPlan(identity), { started, begun, earlierCommits, finished } = this.#store.executeProgress(identity, plan.revision); + progress(identity: PlanIdentity): { started: boolean; begun: boolean; earlierCommits: boolean; completed: string[]; prefixHead: string | null; next: string | null } { + const plan = this.#store.getPlan(identity), { started, begun, earlierCommits, finished, finishedHeads } = this.#store.executeProgress(identity, plan.revision); const done = new Set(finished); + let prefixHead: string | null = null; + for (const item of plan.items) { if (!done.has(item.id)) break; prefixHead = finishedHeads[item.id] ?? null; } return { started, begun, earlierCommits, completed: plan.items.filter(item => done.has(item.id)).map(item => item.id), - next: plan.items.find(item => !done.has(item.id))?.id ?? null }; + prefixHead, next: plan.items.find(item => !done.has(item.id))?.id ?? null }; } /** 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)); } @@ -363,7 +384,10 @@ export class ItemExecutor { const stopped = (item: string, state: string, reason: string | null): ExecutionOutcome => ({ kind: 'stopped', item, state, reason, completed: [] }); for (const earlier of this.#store.getAttempts(identity)) { const finding = this.#findings.get(earlier.id); - if (finding) return this.#escalate(identity, earlier, finding, stopped, [], true); + if (finding) { + this.#findings.persist(earlier.id); + return this.#escalate(identity, this.#store.getAttempt(identity, earlier.id), finding, stopped, [], true); + } } if (options.scope === false) return null; const owed = this.#unpausedScopeFinding(identity); @@ -386,11 +410,12 @@ export class ItemExecutor { * The synchronous start of a run: owed work first, then the first item's admission. With `strict`, a run that cannot * admit its first item throws instead of returning a stopped outcome. */ - #begin(identity: PlanIdentity, options: { fromItem?: string; claimRequeue?: boolean }, strict: boolean): { outcome: ExecutionOutcome } | Run { + #begin(identity: PlanIdentity, options: { fromItem?: string; claimRequeue?: boolean; expectedReviewVersion?: number }, strict: boolean): { outcome: ExecutionOutcome } | Run { const plan = this.#store.getPlan(identity); const start = options.fromItem ? plan.items.findIndex(item => item.id === options.fromItem) : 0; if (start < 0) throw new Error('Unknown plan item.'); - const run: Run = { plan, index: start, attempt: undefined!, done: [], unchanged: [], stopped: undefined! }; + const run: Run = { plan, index: start, attempt: undefined!, done: [], unchanged: [], stopped: undefined!, + ...(options.expectedReviewVersion === undefined ? {} : { reviewVersion: options.expectedReviewVersion }) }; run.stopped = (item, state, reason) => ({ kind: 'stopped', item, state, reason, completed: [...run.done] }); const notStarted = (reason: string, error?: unknown): { outcome: ExecutionOutcome } => { if (strict) throw error ?? new GuardRefusal(reason); @@ -403,11 +428,26 @@ export class ItemExecutor { // A safety finding not yet acted on (a failed write, a human gate at the time) goes to needs human first. for (const earlier of this.#store.getAttempts(identity)) { const finding = this.#findings.get(earlier.id); - if (finding) return this.#settledOrRefused(this.#escalate(identity, earlier, finding, run.stopped, [], true), strict); + if (finding) { + if (options.claimRequeue) this.#store.claimRequeue(identity, this.#store.getTask(identity).stateVersion); + const persisted = this.#findings.persist(earlier.id), current = this.#store.getAttempt(identity, earlier.id); + // A pending cancel can leave the original attempt active. Saving the finding is the durable work owed; its + // terminal write will apply the cancellation (which wins) without losing the safety evidence. + if (persisted && (current.state === 'pending' || current.state === 'running')) + return { outcome: run.stopped(current.item!, 'not started', `${finding} The finding is saved and waits for the active attempt to settle.`) }; + const outcome = this.#escalate(identity, current, finding, run.stopped, [], true); + // Persisting terminal evidence at a human gate intentionally leaves the escalation owed. Return the stopped + // outcome even for strict begin so its outer user action commits that evidence instead of rolling it back. + if (!persisted && HUMAN_GATES.includes(this.#store.getTask(identity).status)) return { outcome }; + return this.#settledOrRefused(outcome, strict); + } } // A scope finding whose pause was never recorded (a failed write, the write gate, a crash) pauses now, before any item. const owed = this.#unpausedScopeFinding(identity); - if (owed) return this.#settledOrRefused(this.#pause(identity, owed.row, owed.result, run.stopped, [], true), strict); + if (owed) { + if (options.claimRequeue) this.#store.claimRequeue(identity, this.#store.getTask(identity).stateVersion); + return this.#settledOrRefused(this.#pause(identity, owed.row, owed.result, run.stopped, [], true), strict); + } // Continuing after a scope pause (plan-format.md: reconcile the executed prefix with the audited head, validate the // remaining items from that checkpoint) is not built yet (#88), so a paused task runs no further items: fail closed. const checkpoint = this.#store.latestCheckpoint(identity); @@ -442,6 +482,16 @@ export class ItemExecutor { } if (expected && !sameContext(current, expected)) return run.stopped(item.id, 'not started', `The task's snapshot or assignment changed during the run; review it before running ${item.id}.`); + if (run.reviewVersion !== undefined) { + const changed = this.#store.reviewVersion(identity) !== run.reviewVersion; + const unapproved = changed ? [] : this.#store.unapprovedExecutionItems(identity, run.plan.revision); + if (changed || unapproved.length) { + const reason = changed ? `The review changed during the run; review it before running ${item.id}.` + : `Approve every plan item before running ${item.id}. Waiting for: ${unapproved.join(', ')}.`; + if (strict) throw new GuardRefusal(reason); + return run.stopped(item.id, 'not started', reason); + } + } try { return this.#runner.start(identity, { expectedStateVersion: this.#store.getTask(identity).stateVersion, kind: 'execute', item: item.id, @@ -462,7 +512,12 @@ export class ItemExecutor { // Only the runner's own audit records a finding; it wins over any later stale or stop outcome. The terminal write // has usually acted on it already, so this reads it whether or not it is still owed. const violation = this.#findings.finding(attempt.id); - if (violation) return this.#escalate(identity, row, violation, stopped, done); + if (violation) { + // The first save can fail before the terminal write. This run still owns settlement, so retry through its + // shutdown capability before escalation can clear the only in-memory copy. + this.#findings.persist(attempt.id, { settlement: true }); + return this.#escalate(identity, this.#store.getAttempt(identity, attempt.id), violation, stopped, done); + } if (row.state !== 'completed') { // Still pending or running: the terminal write failed and the slot is held until restart. const unresolved = this.#runner.status(identity).unresolved; @@ -492,6 +547,7 @@ 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 write = owed ? direct : this.#write; const remember = () => { const current = this.#store.getTask(identity), key = identityKey(identity); if (current.status === 'needs human' && current.currentAttemptId === row.id) @@ -505,17 +561,19 @@ 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)) { - 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.`); + if (!this.#findings.owed(row.id)) { + if (CLOSED_STATUSES.includes(task.status)) return stopped(item, owed ? 'not started' : 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)) { + write(() => this.#store.settleOwedSafetyFinding(identity, row.id)); this.#findings.settle(row.id); return stopped(item, owed ? 'not started' : row.state, `${violation} The task is ${task.status}, so it was not moved to needs human.`); } // Already where the finding sends it: nothing is owed. if (task.status === 'needs human') { + write(() => this.#store.settleOwedSafetyFinding(identity, row.id)); this.#findings.settle(row.id); remember(); return { kind: 'needs human', item, reason: violation, completed: [...done] }; @@ -524,7 +582,10 @@ export class ItemExecutor { return stopped(item, owed ? 'not started' : row.state, `${violation} The task is ${task.status}; it moves to needs human when it next runs.`); // Settling this run's own attempt writes through the shutdown capability; paying an owed finding at the start of a // new run is that run's decision, so it does not, and the closed write gate refuses it like any other. - try { (owed ? direct : this.#write)(() => this.#store.transitionTask(identity, task.stateVersion, 'needs human')); } + try { write(() => { + this.#store.transitionTask(identity, task.stateVersion, 'needs human'); + this.#store.settleOwedSafetyFinding(identity, row.id); + }); } catch (error) { if (!(error instanceof GuardRefusal) && !(owed && error instanceof ShuttingDownError)) throw error; return stopped(item, owed ? 'not started' : row.state, `${violation} The task could not be moved to needs human yet: ${(error as Error).message}`); diff --git a/runner/review.ts b/runner/review.ts index 7d646591..907fc572 100644 --- a/runner/review.ts +++ b/runner/review.ts @@ -84,6 +84,10 @@ export class ReviewService { if (approval && (approval.revision !== plan.revision || approval.snapshotId !== snapshot.id || (mergeAttempt.requiresFreshReview && (approval.reviewVersion === undefined || approval.reviewVersion < mergeAttempt.reviewVersion)))) states[item.id] = 'stale'; } + // The execution gate also orders approvals after attribution choices and binds them to the current or executed-prefix + // snapshots. Reflect that same gate in the review UI so an approval execution would refuse is available to reapprove. + const executionUnapproved = new Set(this.store.unapprovedExecutionItems(identity, plan.revision)); + for (const item of plan.items) if (states[item.id] === 'approved' && executionUnapproved.has(item.id)) states[item.id] = 'stale'; for (const item of plan.items) { if (states[item.id] === 'approved' && (segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale'; } @@ -110,6 +114,7 @@ export class ReviewService { ambiguous: segments.filter(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)).map(reviewedDigest), dependencies: item.depends_on.map(id => { const dependency = plan.items.find(value => value.id === id); return dependency ? staleKey(dependency) : null; }), replacement: replacementReview ? { snapshotId: snapshot.id, revision: plan.revision, requiresFreshReview: mergeAttempt.requiresFreshReview, reviewVersion: mergeAttempt.reviewVersion } : null, + execution: executionUnapproved.has(item.id) ? { snapshotId: snapshot.id, choices: saved.choices } : null, })).digest('hex'); staleKeys.set(item.id, key); return key; @@ -129,6 +134,7 @@ export class ReviewService { if (before && !isDeepStrictEqual(before.item.acceptance, item.acceptance)) reasons.push('Acceptance checks changed'); if (before && owned.some(segment => before.segments.some((old: { path: string; content: string; context: string }) => old.path === segment.path && old.content === segment.content && old.context !== segment.context))) reasons.push('Moved to another function'); for (const dep of item.depends_on) if (states[dep] === 'stale') reasons.push(`Depends on ${dep}, which changed`); + if (executionUnapproved.has(item.id)) reasons.push('Execution approval is out of date'); if (!reasons.length) reasons.push('Code or plan definition changed'); } return { ...item, state: states[item.id], count: owned.length, ambiguousCount: ambiguous, reasons, before, staleKey: staleKey(item), diff --git a/runner/store.ts b/runner/store.ts index 551c69de..bfe43db9 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -8,7 +8,7 @@ import type { Approval, SegmentChoice } from '../core/approvals.ts'; import type { InvocationContext, StopReason } from '../agents/contract.ts'; import type { AlreadyFixedResult } from '../github/already-fixed.ts'; import { - ATTEMPT_PHASES, BadRequest, CLOSED_STATUSES, MERGEABLE_STATUSES, ShuttingDownError, type ShutdownCapability, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES, + ATTEMPT_PHASES, BadRequest, CLOSED_STATUSES, HUMAN_GATES, MERGEABLE_STATUSES, ShuttingDownError, type ShutdownCapability, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES, WRITABLE_KINDS, assertUuidV4, bounded, classifySettlement, requestHash, sameContext, type AttemptKind, type AttemptState, type Classification, type FirstReason, type Settlement, type TaskStatus, } from './lifecycle.ts'; @@ -129,8 +129,8 @@ export class Store { this.#db.exec('PRAGMA foreign_keys=ON; PRAGMA journal_mode=WAL; PRAGMA synchronous=FULL;'); this.#transaction(() => { const version = this.#get('PRAGMA user_version')!.user_version as number; - if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11].includes(version)) throw new Error('Unsupported store schema version.'); - if (version === 11) return; + if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13].includes(version)) throw new Error('Unsupported store schema version.'); + if (version === 13) return; if (version === 0) this.#db.exec(` CREATE TABLE plans (key TEXT PRIMARY KEY, issue INTEGER NOT NULL, revision INTEGER NOT NULL, snapshot_id TEXT); CREATE TABLE revisions (key TEXT NOT NULL REFERENCES plans(key), revision INTEGER NOT NULL, data TEXT NOT NULL, PRIMARY KEY(key,revision)); @@ -192,6 +192,33 @@ export class Store { this.#db.exec("ALTER TABLE requests ADD COLUMN mode TEXT NOT NULL DEFAULT 'suggest'"); this.#db.exec('PRAGMA user_version=11'); } + // Choices written before review-version ordering existed must not become an infinite threshold. Give every + // legacy choice the review version at upgrade. Legacy approvals are already invalid for execution, so remove + // them too: the review UI then renders those items unreviewed and permits the fresh approvals that must follow. + if (version < 12) { + const changed = new Set(); + for (const row of this.#db.prepare('SELECT key,choice_key,data FROM choices').all()) { + const choice = decode(row.data); + if (Number.isSafeInteger(choice.reviewVersion) && choice.reviewVersion! >= 0) continue; + const boundary = this.#current(row.key as string).review_version as number; + this.#run('UPDATE choices SET data=? WHERE key=? AND choice_key=?', encode({ ...choice, reviewVersion: boundary }), row.key!, row.choice_key!); + changed.add(row.key as string); + } + for (const row of this.#db.prepare('SELECT key,item,data FROM approvals').all()) { + const approval = decode(row.data); + if (Number.isSafeInteger(approval.reviewVersion) && approval.reviewVersion! >= 0) continue; + this.#run('DELETE FROM approvals WHERE key=? AND item=?', row.key!, row.item!); + changed.add(row.key as string); + } + for (const key of changed) this.#run('UPDATE plans SET review_version=review_version+1 WHERE key=?', key); + this.#db.exec('PRAGMA user_version=12'); + } + // A finding persisted behind a human gate still owes its escalation after restart. + if (version < 13) { + if (!this.#db.prepare('PRAGMA table_info(attempts)').all().some(column => column.name === 'safety_owed')) + this.#db.exec('ALTER TABLE attempts ADD COLUMN safety_owed INTEGER NOT NULL DEFAULT 0 CHECK (safety_owed IN (0,1))'); + this.#db.exec('PRAGMA user_version=13'); + } }); } catch (error) { this.#db.close(); throw error; } } @@ -617,14 +644,14 @@ export class Store { if (this.#taskClosed(key)) throw new GuardRefusal('A closed task never changes.'); // Approvals and choices must not change the reviewed state while GitHub may still merge it. if (this.#activeMerge(key)) throw new GuardRefusal('A merge is in progress; wait for its outcome.'); - const plan = this.getPlan(identity); + const plan = this.getPlan(identity), reviewVersion = this.reviewVersion(identity); for (const approval of approvals) { if (!plan.items.some(item => item.id === approval.item) || !approval.fingerprint) throw new Error('Invalid approval.'); - this.#run('INSERT OR REPLACE INTO approvals VALUES (?,?,?)', key, approval.item, encode({ ...approval, ...expected })); + this.#run('INSERT OR REPLACE INTO approvals VALUES (?,?,?)', key, approval.item, encode({ ...approval, ...expected, reviewVersion })); } for (const choice of choices) { if (!choice.key || !['assign','accept'].includes(choice.action) || (choice.action === 'assign' ? !plan.items.some(item => item.id === choice.item) : choice.item !== null)) throw new Error('Invalid segment choice.'); - this.#run('INSERT OR REPLACE INTO choices VALUES (?,?,?)', key, choice.key, encode({ ...choice, ...expected })); + this.#run('INSERT OR REPLACE INTO choices VALUES (?,?,?)', key, choice.key, encode({ ...choice, ...expected, reviewVersion })); } this.#run('UPDATE plans SET review_version=review_version+1 WHERE key=?', key); }); @@ -675,6 +702,36 @@ export class Store { choices: this.#db.prepare('SELECT data FROM choices WHERE key=? ORDER BY choice_key').all(key).map(row => decode(row.data)), }; } + /** Before execution, approvals belong to the current snapshot; after a completed item, this revision's approvals survive its own commits. */ + unapprovedExecutionItems(identity: PlanIdentity, revision: number): string[] { + const key = identityKey(identity), plan = this.getPlan(identity, revision); + const snapshotId = this.getSnapshot(identity).id, review = this.getReview(identity); + // An approval inherited across runner commits must come from the context of the completed prefix, not merely from + // this revision. Otherwise A -> unrelated B approval -> A could reuse B's approval when the prefix-head guard passes. + const completedAt: Record = Object.create(null); + for (const row of this.#db.prepare(`SELECT item,context FROM attempts WHERE plan_key=? AND kind='execute' AND state='completed' + AND json_extract(context,'$.planRevision')=? ORDER BY rowid`).all(key, revision)) { + const context = decode(row.context); + if (typeof row.item === 'string' && typeof context.snapshotId === 'string') completedAt[row.item] = context.snapshotId; + } + const prefixSnapshots = new Set(); + for (const item of plan.items) { + const completedSnapshot = completedAt[item.id]; + if (!completedSnapshot) break; + prefixSnapshots.add(completedSnapshot); + } + const reviewedExecutionSnapshot = (value: ReviewState) => value.snapshotId === snapshotId || prefixSnapshots.has(value.snapshotId); + // A later attribution choice changes the material reviewed by at least one item. Without rebuilding Git history on a + // status poll, conservatively require approvals recorded after the latest such choice for this execution context. + let latestChoiceVersion = -1; + for (const choice of review.choices) if (choice.revision === revision && (prefixSnapshots.size > 0 || choice.snapshotId === snapshotId)) { + if (choice.reviewVersion === undefined) latestChoiceVersion = Number.MAX_SAFE_INTEGER; + else if (choice.reviewVersion > latestChoiceVersion) latestChoiceVersion = choice.reviewVersion; + } + const approved = new Set(review.approvals.filter(value => value.revision === revision && reviewedExecutionSnapshot(value) + && (value.reviewVersion ?? -1) > latestChoiceVersion).map(value => value.item)); + return plan.items.filter(item => !approved.has(item.id)).map(item => item.id); + } /** Records evidence from an already completed safety audit; does not authorize execution. */ recordCheckpoint(identity: PlanIdentity, expected: ReviewState, evidence: Omit): Checkpoint { const key = identityKey(identity); @@ -844,6 +901,45 @@ export class Store { return changed; }); } + /** Retry a finding whose original durable save failed. False keeps its escalation owed behind a human gate. */ + recordOwedSafetyFinding(identity: PlanIdentity, id: string, finding: string, options: { deferAction?: boolean } = {}): boolean { + if (typeof finding !== 'string' || !finding.trim()) throw new GuardRefusal('A safety finding needs its reason.'); + const key = identityKey(identity); + return this.#transaction(() => { + const row = this.#get('SELECT state,safety_finding,safety_owed FROM attempts WHERE plan_key=? AND id=?', key, id); + if (!row) return false; + let changed = false; + if (row.safety_finding === null) { + this.#run('UPDATE attempts SET safety_finding=? WHERE plan_key=? AND id=?', bounded(finding), key, id); + changed = true; + } + const before = this.#task(key).status as TaskStatus; + const terminal = TERMINAL_STATES.includes(row.state as AttemptState); + // A run that has just settled first makes the evidence durable, then lets the executor apply every current-state + // precondition (including an active merge) before it clears the debt. Recovery and later runs may act here. + const acted = !terminal || (!options.deferAction && !HUMAN_GATES.includes(before)); + const owed = terminal && !acted ? 1 : 0; + if (row.safety_owed !== owed) { + this.#run('UPDATE attempts SET safety_owed=? WHERE plan_key=? AND id=?', owed, key, id); + changed = true; + } + if (terminal && acted && !options.deferAction) this.#actOnFinding(key); + if (changed || this.#task(key).status !== before) this.#touch(key); + return acted; + }); + } + /** Clear a durable escalation debt only in the same write that successfully acts on it. */ + settleOwedSafetyFinding(identity: PlanIdentity, id: string): void { + const key = identityKey(identity); + this.#transaction(() => { + if (this.#run('UPDATE attempts SET safety_owed=0 WHERE plan_key=? AND id=? AND safety_owed=1', key, id).changes === 1) this.#touch(key); + }); + } + /** Terminal findings whose evidence is durable but whose escalation is still blocked by a human gate. */ + owedSafetyFindings(): { attemptId: string; finding: string }[] { + return this.#db.prepare('SELECT id,safety_finding FROM attempts WHERE safety_owed=1 AND safety_finding IS NOT NULL ORDER BY rowid').all() + .map(row => ({ attemptId: row.id as string, finding: row.safety_finding as string })); + } /** Every durable change to a task or its attempts increases the state version. */ #touch(key: string): void { @@ -896,11 +992,8 @@ export class Store { FROM attempts WHERE plan_key=? ORDER BY rowid DESC LIMIT ?) ORDER BY row_order`).all(identityKey(identity), limit) .map(row => { const { result: _result, ...attempt } = this.#attemptRecord(row); return { ...attempt, hasResult: row.has_result === 1 }; }); } - /** - * What `ItemExecutor.progress` needs about a task's execute attempts, without loading or decoding any attempt's result - * (up to 1 MiB each): a status poll asks for it (#91 part 2). `finished` lists each item completed at `revision` once. - */ - executeProgress(identity: PlanIdentity, revision: number): { started: boolean; begun: boolean; earlierCommits: boolean; finished: string[] } { + /** What `ItemExecutor.progress` needs about execute attempts. It decodes only completed results' small head field. */ + executeProgress(identity: PlanIdentity, revision: number): { started: boolean; begun: boolean; earlierCommits: boolean; finished: string[]; finishedHeads: Record } { const key = identityKey(identity), rev = "json_extract(context,'$.planRevision')"; const row = this.#get(`SELECT COUNT(*) > 0 AS started, COALESCE(SUM(${rev} = ?), 0) > 0 AS begun FROM attempts WHERE plan_key=? AND kind='execute' AND item IS NOT NULL`, revision, key)!; @@ -909,9 +1002,27 @@ export class Store { const earlier = this.#get(`SELECT 1 AS found FROM attempts WHERE plan_key=? AND kind IN (${WRITABLE_KINDS.map(() => '?').join(',')}) AND state='completed' AND ${rev} != ? AND (NOT json_valid(result) OR COALESCE(json_extract(result,'$.unchanged'), 0) = 0) LIMIT 1`, key, ...WRITABLE_KINDS, revision); - const finished = this.#db.prepare(`SELECT DISTINCT item FROM attempts WHERE plan_key=? AND kind='execute' AND item IS NOT NULL - AND state='completed' AND ${rev} = ?`).all(key, revision).map(entry => entry.item as string); - return { started: row.started === 1, begun: row.begun === 1, earlierCommits: !!earlier, finished }; + const finishedRows = this.#db.prepare(`SELECT item,CASE WHEN json_valid(result) + THEN CASE WHEN json_type(result,'$.head')='text' THEN json_extract(result,'$.head') ELSE NULL END ELSE NULL END AS head + FROM attempts WHERE plan_key=? AND kind='execute' AND item IS NOT NULL + AND state='completed' AND ${rev} = ? ORDER BY rowid`).all(key, revision); + const finishedHeads: Record = Object.create(null); + for (const entry of finishedRows) { + const head = typeof entry.head === 'string' && /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/.test(entry.head) ? entry.head : null; + finishedHeads[entry.item as string] = head; + } + return { started: row.started === 1, begun: row.begun === 1, earlierCommits: !!earlier, + finished: Object.keys(finishedHeads), finishedHeads }; + } + /** Claim recovery when a user's Resume settles owed work instead of admitting a replacement attempt. */ + claimRequeue(identity: PlanIdentity, expectedStateVersion: number): void { + const key = identityKey(identity); + this.#transaction(() => { + const task = this.#task(key); + if (task.state_version !== expectedStateVersion) throw new GuardRefusal('Stale task state. Reload before writing.'); + if (task.requeue_pending !== 1) throw new GuardRefusal('The requeue was already claimed.'); + this.#run('UPDATE tasks SET requeue_pending=0 WHERE plan_key=?', key); this.#touch(key); + }); } /** The heads of completed execute attempts that changed files outside their plan item, oldest first. */ scopeFindingHeads(identity: PlanIdentity): string[] { diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index aa121a34..75e8f55e 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -161,7 +161,7 @@ it('migrates a database that already has the mode column below v11', () => { expect(open().getDraft(identity, id).state).toBe('ready'); }); -it.each([9, 10])('migrates a v%i database to v11, reading its existing requests as suggestions', version => { +it.each([9, 10])('migrates a v%i database to the current schema, reading its existing requests as suggestions', version => { const { store, path, open } = fixture(), id = store.beginSuggestions(identity, state(store), 'suggest'); store.close(); stores.splice(stores.indexOf(store), 1); const legacy = new DatabaseSync(path); @@ -170,7 +170,7 @@ it.each([9, 10])('migrates a v%i database to v11, reading its existing requests const migrated = open(); expect(migrated.getSuggestions(identity, id)).toMatchObject({ mode: 'suggest', state: 'pending' }); const db = new DatabaseSync(path); - try { expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); } finally { db.close(); } + try { expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); } finally { db.close(); } }); /** E3 in draft mode, with a provider that answers from `source`. */ diff --git a/test/publish.test.ts b/test/publish.test.ts index e06b4064..a71bc1aa 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -337,7 +337,7 @@ describe('schema v7', () => { expect(upgraded.taskPullRequests(identity)).toEqual([]); expect(upgraded.latestAlreadyFixed(identity)).toBeNull(); const db = new DatabaseSync(path, { readOnly: true }); - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); const names = db.prepare("SELECT name FROM sqlite_master WHERE type IN ('table','index') AND (name LIKE '%pull_requests%' OR name LIKE 'already_fixed_checks%') AND name NOT LIKE 'sqlite_autoindex%' ORDER BY name").all().map(row => row.name); expect(names).toEqual(['already_fixed_checks', 'already_fixed_checks_task', 'task_pull_requests', 'task_pull_requests_number', 'task_pull_requests_opening', 'task_pull_requests_task']); db.close(); diff --git a/test/review.test.ts b/test/review.test.ts index 8a8b1503..5a5d4362 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -28,6 +28,17 @@ it('expires a browser token after another view assigns a segment and recomputes expect(next.items[0]!.checks.scope).toContain('out of scope'); expect(()=>service.act({action:'approve',item:'P1',token:view.token})).toThrow(/Stale/); }); +it('offers reapproval when a later attribution choice invalidates execution approvals',()=>{ + const {service,config}=fixture();let view=service.load(); + for(const item of view.items)view=service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); + expect(service.store.unapprovedExecutionItems(config.identity,view.plan.revision)).toEqual([]); + view=service.act({action:'accept',key:view.segments.find(segment=>segment.row==='Unplanned')!.key,token:view.token}); + expect(view.items.every(item=>item.state==='stale'&&item.reasons.includes('Execution approval is out of date'))).toBe(true); + expect(service.store.unapprovedExecutionItems(config.identity,view.plan.revision)).toEqual(view.plan.items.map(item=>item.id)); + for(const item of view.items)view=service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); + expect(view.items.every(item=>item.state==='approved')).toBe(true); + expect(service.store.unapprovedExecutionItems(config.identity,view.plan.revision)).toEqual([]); +},30000); it('keeps both sides of a declared rename in scope after manual reassignment',()=>{ const {service,config}=fixture(),identity=config.identity,plan=service.store.getPlan(identity); plan.items=[{...plan.items[0]!,files:[{path:'renamed.ts',kind:'rename',renamed_from:'retry.ts',change:'Rename the implementation.'}],depends_on:[]}]; @@ -44,7 +55,7 @@ it('keeps both sides of a declared rename in scope after manual reassignment',() expect(new Set(candidates.map(segment=>segment.operation))).toEqual(new Set(['-','+'])); expect(new Set(candidates.map(segmentPath))).toEqual(new Set(['retry.ts','renamed.ts'])); for(const candidate of candidates){view=service.act({action:'assign',key:candidate.key,item:'P1',token:view.token});expect(view.segments.find(segment=>segment.key===candidate.key)?.scope).toBe('in-scope');} -}); +},30000); it('persists bounded per-item notes without creating a plan revision',()=>{ const {service,config}=fixture();const view=service.load();service.act({action:'note',item:'P1',kind:'question',text:'Why this limit?',token:view.token}); const reopened=new ReviewService(config);services.push(reopened);expect(reopened.load().notes[0]!.text).toBe('Why this limit?');expect(reopened.load().plan.revision).toBe(1); @@ -92,9 +103,9 @@ it('gives each stale state its own stale key, including states whose segments an view=service.act({action:'assign',key:unplanned(),item:'P1',token:view.token}); const second={p1:item('P1').staleKey,p1Segments:own('P1'),p1Reasons:item('P1').reasons}; expect(second.p1).not.toBe(first.p1);expect(item('P3').reasons).toEqual(first.p3Reasons);expect(item('P3').staleKey).not.toBe(first.p3); - // Re-approving P3 while P1 keeps it stale changes only P3's approval. + // Re-approving P3 clears its execution reason, but P1's stale dependency keeps P3 stale with a new key. const p3Before=item('P3').staleKey;view=service.act({action:'approve',item:'P3',confirmNoChange:true,token:view.token}); - expect(item('P3').state).toBe('stale');expect(item('P3').reasons).toEqual(first.p3Reasons);expect(item('P3').staleKey).not.toBe(p3Before); + expect(item('P3').state).toBe('stale');expect(item('P3').reasons).toEqual(['Depends on P1, which changed']);expect(item('P3').staleKey).not.toBe(p3Before); // P1 is approved at its current code, then an ambiguous change it shares makes it stale with the same own segments and reasons as before. view=service.act({action:'approve',item:'P1',token:view.token});expect(item('P1').staleKey).toBeNull(); // P1 adds a line to an existing file and P2 then edits that same line, so the line is ambiguous between them. @@ -116,11 +127,12 @@ it('gives a still-stale item a new stale key when it gains an ambiguous change', service.store.recordHistory(config.identity,{revision:view.plan.revision,snapshotId:snapshot.id},snapshot.base,byP2,[{sha:byP1,owner:'P1',origin:'owned',sourceSha:null},{sha:byP2,owner:'P2',origin:'owned',sourceSha:null}]); view=service.load();expect(view.segments.some(segment=>segment.row==='Ambiguous'&&segment.owners.includes('P1'))).toBe(true); expect(view.segments.filter(segment=>segment.row==='P1').map(segment=>segment.key)).toEqual(before.own);expect(item('P1').reasons).toEqual(before.reasons);expect(item('P1').staleKey).not.toBe(before.key); - // P2 re-commits the shared line with CRLF only. Approvals ignore line endings, so the stale state and its key stay the same. + // P2 re-commits the shared line with CRLF only. The rendered approval ignores line endings, but the execution gate also + // owns the changed snapshot, so the stale state gets a new key even though the reviewed segment is equivalent. const unchanged=item('P1').staleKey;writeFileSync(readme,`${original}Shared note, revised.\r\n`);fixtureGit(config.repository,'commit','-am','P2 line endings');const crlf=fixtureGit(config.repository,'rev-parse','HEAD'),endings=service.store.getSnapshot(config.identity); service.store.recordHistory(config.identity,{revision:view.plan.revision,snapshotId:endings.id},endings.base,crlf,[{sha:crlf,owner:'P2',origin:'owned',sourceSha:null}]); const lf=view.segments.find(segment=>segment.row==='Ambiguous'&&segment.path==='README.md')!;view=service.load();const ending=view.segments.find(segment=>segment.row==='Ambiguous'&&segment.path==='README.md')!; - expect(ending.content).toContain('\r');expect(ending.owners).toEqual(lf.owners);expect(item('P1').staleKey).toBe(unchanged); + expect(ending.content).toContain('\r');expect(ending.owners).toEqual(lf.owners);expect(item('P1').staleKey).not.toBe(unchanged); // P3 then edits the same line back to P2's text: the segment keeps its choice key but gains an owner, so the stale state is new. const shared=()=>view.segments.find(segment=>segment.row==='Ambiguous'&&segment.path==='README.md')!,second={key:item('P1').staleKey,segment:shared()}; writeFileSync(readme,`${original}Shared note, draft.\n`);fixtureGit(config.repository,'commit','-am','P3 draft');const byP3=fixtureGit(config.repository,'rev-parse','HEAD'); diff --git a/test/runner-execution.test.ts b/test/runner-execution.test.ts index 47507744..947f4b91 100644 --- a/test/runner-execution.test.ts +++ b/test/runner-execution.test.ts @@ -94,7 +94,7 @@ function setup(options: { manifests?: Record { await runner.close(); store.close(); }); - return { store, path, workspace, findings, runner, executor: new ItemExecutor(store, runner, sources, findings, { capability }), log, commits, prompts, argv, owners, checks }; + return { store, path, workspace, sources, findings, runner, executor: new ItemExecutor(store, runner, sources, findings, { capability }), log, commits, prompts, argv, owners, checks }; } describe('item execution', () => { @@ -270,6 +270,24 @@ describe('item execution', () => { expect(h.commits.map(c => c.item)).toEqual(['P1']); expect(h.runner.status(identity).unresolved).toBeNull(); }); + it('stops before the next item when review input changes during the run', async () => { + let store!: Store, changed = false; + const h = setup({ onLaunch: attemptId => { + if (changed || store.getAttempt(identity, attemptId).item !== 'P1') return; + changed = true; + const snapshot = store.getSnapshot(identity); + store.saveReview(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, [], + [{ key: 'later-choice', action: 'assign', item: 'P1' }]); + } }); + store = h.store; + const snapshot = store.getSnapshot(identity), current = store.getPlan(identity); + store.saveReview(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, + current.items.map(item => ({ item: item.id, fingerprint: `approved-${item.id}` })), []); + const begun = h.executor.begin(identity, { expectedReviewVersion: store.reviewVersion(identity) }); + expect(await begun.outcome).toMatchObject({ kind: 'stopped', item: 'P2', state: 'not started', completed: ['P1'], + reason: expect.stringMatching(/review changed/) }); + expect(store.getAttempts(identity).map(row => row.item)).toEqual(['P1']); + }); it('still pauses for amendment, bound to where the item ran, when the plan changes after its attempt settled', async () => { let store!: Store; const h = setup({ manifests: { P1: manifest([change('a.ts'), change('extra.ts', { kind: 'add', oldType: undefined })]) }, release: async () => { @@ -346,7 +364,8 @@ describe('item execution', () => { // Admitted synchronously, inside whatever transaction the caller holds. expect(h.store.getAttempts(identity).map(row => [row.id, row.item])).toEqual([[begun.attemptId, 'P1']]); expect(await begun.outcome).toEqual({ kind: 'executed', items: ['P1', 'P2'], unchanged: [] }); - expect(h.executor.progress(identity)).toEqual({ started: true, begun: true, earlierCommits: false, completed: ['P1', 'P2'], next: null }); + expect(h.executor.progress(identity)).toEqual({ started: true, begun: true, earlierCommits: false, completed: ['P1', 'P2'], + prefixHead: oid(101), next: null }); }); it('begin throws the admission refusal itself, where runTask reports it as not started', async () => { const h = setup(); @@ -520,10 +539,29 @@ describe('item execution', () => { store = unsafe.store; expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', reason: expect.stringMatching(/needs approval; it moves to needs human when it next runs/) }); expect(store.getTask(identity).status).toBe('needs approval'); - expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'not started', reason: expect.stringMatching(/needs approval; it moves to needs human/) }); + const [unsafeAttempt] = store.getAttempts(identity); + const retrySave = store.recordOwedSafetyFinding.bind(store); + store.recordOwedSafetyFinding = () => { throw Object.assign(new Error('disk full on retry'), { code: 'ERR_SQLITE_ERROR' }); }; + expect(() => unsafe.executor.begin(identity)).toThrow(/disk full on retry/); + expect(store.getTask(identity).status).toBe('needs approval'); + expect(store.getAttempt(identity, unsafeAttempt!.id).safetyFinding).toMatch(/Git metadata/); + expect(unsafe.findings.get(unsafeAttempt!.id)).toMatch(/Git metadata/); + store.recordOwedSafetyFinding = retrySave; + const gatedBegin = unsafe.executor.begin(identity); + expect(gatedBegin.attemptId).toBeNull(); + expect(await gatedBegin.outcome).toMatchObject({ kind: 'stopped', reason: expect.stringMatching(/needs approval; it moves to needs human/) }); + expect(store.getTask(identity).status).toBe('needs approval'); + expect(store.getAttempt(identity, unsafeAttempt!.id).safetyFinding).toMatch(/Git metadata/); + expect(unsafe.findings.get(unsafeAttempt!.id)).toMatch(/Git metadata/); + // A new process reconstructs the deferred escalation from the durable marker, not the old in-memory map. + unsafe.findings.settle(unsafeAttempt!.id); + const restartedFindings = new SafetyFindings(store), restarted = new ItemExecutor(store, unsafe.runner, unsafe.sources, restartedFindings); + expect(restartedFindings.get(unsafeAttempt!.id)).toMatch(/Git metadata/); + expect(await restarted.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'not started', reason: expect.stringMatching(/needs approval; it moves to needs human/) }); store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); - expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); + expect(restarted.payOwed(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs human'); + expect(new SafetyFindings(store).get(unsafeAttempt!.id)).toBeUndefined(); expect(store.getAttempts(identity)).toHaveLength(1); }); it('never launches an item with a declared link that goes through another link, and sends the task to a person', async () => { @@ -659,6 +697,27 @@ describe('item execution', () => { expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'stopped', state: 'not started' }); expect(store.getAttempts(identity)).toHaveLength(1); }); + it('persists a current run safety finding before clearing its in-memory copy', async () => { + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); + const attemptId = h.store.getAttempts(identity)[0]!.id; + expect(h.findings.get(attemptId)).toBeUndefined(); + const reopened = new Store(h.path); cleanups.push(() => reopened.close()); + expect(reopened.getAttempt(identity, attemptId).safetyFinding).toMatch(/Git metadata/); + expect(reopened.getTask(identity).status).toBe('needs human'); + }); + it('keeps a current run safety finding owed when its durable retry fails', async () => { + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true }); + const retrySave = h.store.recordOwedSafetyFinding.bind(h.store); + h.store.recordOwedSafetyFinding = () => { throw Object.assign(new Error('disk full on retry'), { code: 'ERR_SQLITE_ERROR' }); }; + await expect(h.executor.runTask(identity)).rejects.toThrow(/disk full on retry/); + const attemptId = h.store.getAttempts(identity)[0]!.id; + expect(h.store.getAttempt(identity, attemptId).safetyFinding).toBeNull(); + expect(h.findings.get(attemptId)).toMatch(/Git metadata/); + h.store.recordOwedSafetyFinding = retrySave; + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); + expect(h.store.getAttempts(identity)).toHaveLength(1); + }); it('keeps task storage when a foreign result\'s terminal write fails', async () => { const { runner, executor, log } = setup({ settleError: true, exit: { P1: { attemptId: '00000000-0000-4000-8000-000000000000' } } }); expect(await executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'running' }); @@ -785,11 +844,16 @@ describe('item execution', () => { }); it('escalates through the capability after the shutdown write gate closed', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, capability: s => s.shutdownCapability(), + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, + capability: s => s.shutdownCapability(), release: async () => { store.closeWrites(); } }); store = h.store; expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs human'); + const attemptId = store.getAttempts(identity)[0]!.id; + const reopened = new Store(h.path); cleanups.push(() => reopened.close()); + expect(reopened.getAttempt(identity, attemptId).safetyFinding).toMatch(/Git metadata/); + expect(new SafetyFindings(reopened).get(attemptId)).toBeUndefined(); }); it('settles an owed finding when the task was closed before its next run', async () => { let store!: Store; diff --git a/test/runner-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index 89930c19..1419f331 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -5,6 +5,7 @@ import { randomUUID } from 'node:crypto'; import { DatabaseSync } from 'node:sqlite'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { Store, mergeActionResponse } from '../runner/store.ts'; +import { SafetyFindings } from '../runner/execution.ts'; import { ActionIdReused, GuardRefusal, classifySettlement, requestHash } from '../runner/lifecycle.ts'; import type { Plan, PlanContext } from '../core/plan.ts'; @@ -81,6 +82,33 @@ describe('schema v6', () => { }); }); +describe('schema v12 review ordering', () => { + it('gives legacy choices a finite boundary so a later approval can authorize execution', () => { + const { store, path } = fixture(); + const snapshot = store.getSnapshot(identity); + store.saveReview(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, + [{ item: 'P1', fingerprint: 'legacy-approval' }], + [{ key: 'legacy-choice', action: 'assign', item: 'P1' }]); + store.close(); stores.splice(stores.indexOf(store), 1); + const legacy = new DatabaseSync(path); + legacy.exec(`UPDATE choices SET data=json_remove(data,'$.reviewVersion'); + UPDATE approvals SET data=json_remove(data,'$.reviewVersion'); + PRAGMA user_version=11;`); + legacy.close(); + + const migrated = open(path), migratedVersion = migrated.reviewVersion(identity); + expect(migrated.getReview(identity).choices[0]!.reviewVersion).toBe(migratedVersion - 1); + // Execution already treats an unversioned approval as invalid. Remove it too, so the review UI renders the item + // unreviewed and leaves its approval control available instead of showing an unusable approved state. + expect(migrated.getReview(identity).approvals).toEqual([]); + expect(migrated.unapprovedExecutionItems(identity, 1)).toEqual(['P1']); + migrated.saveReview(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: migratedVersion }, + [{ item: 'P1', fingerprint: 'approved-after-upgrade' }], []); + expect(migrated.unapprovedExecutionItems(identity, 1)).toEqual([]); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); + }); +}); + describe('schema v6 backfill context', () => { it('attributes a backfilled closure to the snapshot that merged, not to a later HEAD observation', () => { const { store, path } = fixture(); @@ -662,7 +690,33 @@ describe('durable safety findings (#87 item 3)', () => { db.exec('ALTER TABLE attempts DROP COLUMN safety_finding; PRAGMA user_version=7;'); db.close(); const reopened = open(path); expect(reopened.getAttempt(identity, attempt.id).safetyFinding).toBeNull(); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); + }); + + it('adds the durable owed marker to a version 12 database', () => { + const { path, store } = queued(); + store.close(); stores.splice(stores.indexOf(store), 1); + const db = new DatabaseSync(path); + db.exec('ALTER TABLE attempts DROP COLUMN safety_owed; PRAGMA user_version=12;'); db.close(); + const reopened = open(path); + expect(reopened.owedSafetyFindings()).toEqual([]); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); + }); + + it('restores and clears an escalation owed behind a human gate across real store reopens', () => { + const { path, store } = queued(), attempt = admit(store); store.markRunning(identity, attempt.id); + settle(store, attempt.id, { exitCode: 1, valid: false }); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs approval'); + expect(store.recordOwedSafetyFinding(identity, attempt.id, 'Safety violation: durable restart')).toBe(false); + store.close(); stores.splice(stores.indexOf(store), 1); + + const reopened = open(path), findings = new SafetyFindings(reopened); + expect(findings.get(attempt.id)).toBe('Safety violation: durable restart'); + reopened.transitionTask(identity, reopened.getTask(identity).stateVersion, 'queued'); + expect(findings.persist(attempt.id)).toBe(true); + expect(reopened.getTask(identity).status).toBe('needs human'); + reopened.close(); stores.splice(stores.indexOf(reopened), 1); + expect(new SafetyFindings(open(path)).get(attempt.id)).toBeUndefined(); }); }); @@ -686,7 +740,7 @@ describe('allocation baseline (#91)', () => { db.exec('ALTER TABLE attempts DROP COLUMN metadata_baseline; ALTER TABLE attempts DROP COLUMN storage_base; PRAGMA user_version=8;'); db.close(); const reopened = open(path); expect(reopened.interruptedAttempts()).toEqual([expect.objectContaining({ id: attempt.id, metadataBaseline: null, storageBase: null })]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); }); }); describe('publish outcomes (#103)', () => { @@ -702,7 +756,7 @@ describe('publish outcomes (#103)', () => { reopened.recordPublish(identity, { outcome: 'opened', draft: false, message: 'Pull request #1 is open.', number: 1, url: 'https://github.com/o/r/pull/1' }); expect(reopened.lastPublish(identity)).toMatchObject({ outcome: 'opened', number: 1 }); expect(reopened.getTask(identity)).toEqual(before); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 13 }); }); it('stamps an outcome with the version the publish saw, and refuses one it cannot have seen (#114)', () => { const { store } = queued(), version = store.getTask(identity).stateVersion; diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index 02cc0f36..6e072d3c 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -21,6 +21,7 @@ import { OWED_REFUSAL } from '../runner/publishing.ts'; import { PullRequestMisplaced, openingMarker, type OpenPullRequestInput, type PullRequestGateway } from '../github/pull-requests.ts'; import type { AlreadyFixedGateway } from '../github/already-fixed.ts'; import type { PlanIdentity } from '../core/identity.ts'; +import { approveItem } from '../core/approvals.ts'; vi.setConfig({ testTimeout: 30_000 }); const roots: string[] = [], cleanups: (() => Promise | void)[] = []; @@ -180,6 +181,12 @@ async function serve(w: World, options: { before?: (service: ReviewService) => v let findings: SafetyFindings | undefined; const config = { ...w.demo, demo: options.demo ?? false }; app = await startServer(config, 0, undefined, undefined, 2_000, undefined, undefined, undefined, async service => { + const identity = service.config.identity, task = service.store.getTask(identity), plan = service.store.getPlan(identity); + if (task.status !== 'merged' && task.status !== 'cancelled' && service.store.unapprovedExecutionItems(identity, plan.revision).length) { + const review = service.load(); + service.store.saveReview(identity, review.expected, + review.items.map(item => approveItem(review.plan, review.segments, item.id, identity, item.count === 0)), []); + } options.before?.(service); const repository = await openRunnerRepository({ runnerRoot: join(w.root, 'runner'), runnerOwner: OWNER, repositoryId: service.config.identity.repositoryId, source: service.config.repository }); await ensureCommit(repository, service.store.getSnapshot(service.config.identity).head); @@ -226,9 +233,10 @@ async function serve(w: World, options: { before?: (service: ReviewService) => v } const view = async (app: App) => (await fetch(`${new URL(app.url).origin}/api/runner`, { headers: { 'x-codeboost-token': app.token } })).json() as Promise>; async function act(app: App, action: string, actionId = randomUUID(), expectedStateVersion?: number) { - const { stateVersion } = await view(app); + const { stateVersion, reviewVersion } = await view(app); const response = await fetch(`${new URL(app.url).origin}/api/runner`, { method: 'POST', headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' }, - body: JSON.stringify({ action, expectedStateVersion: expectedStateVersion ?? stateVersion, actionId }) }); + body: JSON.stringify({ action, expectedStateVersion: expectedStateVersion ?? stateVersion, + ...((action === 'start' || action === 'resume') ? { expectedReviewVersion: reviewVersion } : {}), actionId }) }); return { status: response.status, body: await response.json() as Record }; } const publishSettled = (app: App, identity: PlanIdentity) => vi.waitFor(async () => { @@ -374,14 +382,14 @@ describe('publishing a finished task (#103)', () => { expect(w.github.prs[0]!.body).not.toContain(BUDGET); }); - it.each(Object.keys(FINDINGS) as FindingSource[])('names an unsaved finding from %s in the draft after the budget runs out', async source => { + it.each(Object.keys(FINDINGS) as FindingSource[])('names an initially unsaved finding from %s in the draft after the budget runs out', async source => { const w = world(); const { app, identity, store, findings } = await serve(w, { findingSource: source, startup: false, before: service => { service.store.transitionTask(service.config.identity, service.store.getTask(service.config.identity).stateVersion, 'queued'); } }); expect(await app.executor!.runTask(identity)).toMatchObject({ kind: 'needs human', reason: FINDINGS[source] }); const attempt = store.getAttempts(identity).at(-1)!; // Prove which coordinator path produced the attempt; the downstream draft alone would not distinguish the sources. - expect(attempt).toMatchObject({ state: 'failed', safetyFinding: null, diagnostic: FINDING_DIAGNOSTICS[source] }); + expect(attempt).toMatchObject({ state: 'failed', safetyFinding: FINDINGS[source], diagnostic: FINDING_DIAGNOSTICS[source] }); expect(findings.owedAttempts()).toEqual([]); const now = vi.spyOn(Date, 'now').mockReturnValue(store.getTask(identity).budgetDeadline! + 1); try { diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index 569d9de3..af3c6a2c 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -237,6 +237,8 @@ describe('/api/runner', () => { const version = (await api(app, 'GET', '/api/runner')).body.stateVersion, actionId = randomUUID(); expect(await api(app, 'POST', '/api/runner', { action: 'bogus', expectedStateVersion: version, actionId })).toMatchObject({ status: 400 }); expect(await api(app, 'POST', '/api/runner', { action: 'retry', expectedStateVersion: 'x', actionId })).toMatchObject({ status: 400 }); + expect(await api(app, 'POST', '/api/runner', { action: 'start', expectedStateVersion: version, actionId })).toMatchObject({ status: 400, + body: { error: expect.stringMatching(/expectedReviewVersion/) } }); expect(await api(app, 'POST', '/api/runner', { action: 'cancel-attempt', attemptId: 'x', expectedStateVersion: version, actionId })).toMatchObject({ status: 400 }); expect(await api(app, 'POST', '/api/runner', { action: 'retry', expectedStateVersion: version, actionId })).toMatchObject({ status: 400 }); // Nothing was saved under the key, so the corrected request is evaluated, not answered with ActionIdReused. diff --git a/test/runner-start.test.ts b/test/runner-start.test.ts index 9c769b94..2092b15a 100644 --- a/test/runner-start.test.ts +++ b/test/runner-start.test.ts @@ -11,6 +11,7 @@ import type { ReviewService } from '../runner/review.ts'; import type { RunnerDeps } from '../runner/coordinator.ts'; import { SafetyFindings, type ExecutionSources } from '../runner/execution.ts'; import type { AttemptKind } from '../runner/lifecycle.ts'; +import { approveItem } from '../core/approvals.ts'; vi.setConfig({ testTimeout: 20_000 }); const roots: string[] = [], cleanups: (() => Promise | void)[] = []; @@ -24,10 +25,11 @@ type App = Awaited>; * A server with a runner whose preparation always fails: an admitted item ends `failed` without an agent, which is * enough to see what start and resume admit. `before` shapes the Store before the runner exists, as recovery would. */ -async function serve(options: { kinds?: AttemptKind[]; before?: (service: ReviewService) => void; findings?: (findings: SafetyFindings, service: ReviewService) => void } = {}) { +async function serve(options: { kinds?: AttemptKind[]; approve?: boolean; before?: (service: ReviewService) => void; findings?: (findings: SafetyFindings, service: ReviewService) => void } = {}) { const root = mkdtempSync(join(tmpdir(), 'codeboost-start-')); roots.push(root); const demo = createDemo(join(root, 'demo')); const app = await startServer({ ...demo }, 0, undefined, undefined, 2_000, undefined, undefined, undefined, async service => { + if (options.approve !== false) approvePlan(service); options.before?.(service); const deps: RunnerDeps = { runnerOwner: OWNER, kinds: options.kinds ?? ['execute'], prepare: async () => { throw new Error('no agent here'); }, cleanupPreparation: async () => undefined, start: () => { throw new Error('no agent here'); }, validate: () => null }; @@ -42,13 +44,20 @@ async function serve(options: { kinds?: AttemptKind[]; before?: (service: Review return { app, close, database: demo.database, identity: demo.identity, store: app.service.store, items: app.service.store.getPlan(demo.identity).items.map(item => item.id) }; } const view = async (app: App) => (await fetch(`${new URL(app.url).origin}/api/runner`, { headers: { 'x-codeboost-token': app.token } })).json() as Promise>; -async function act(app: App, action: string, request: { expectedStateVersion?: number; actionId?: string } = {}) { - const { stateVersion } = await view(app); +async function act(app: App, action: string, request: { expectedStateVersion?: number; expectedReviewVersion?: number; actionId?: string } = {}) { + const { stateVersion, reviewVersion } = await view(app); const response = await fetch(`${new URL(app.url).origin}/api/runner`, { method: 'POST', headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' }, - body: JSON.stringify({ action, expectedStateVersion: request.expectedStateVersion ?? stateVersion, actionId: request.actionId ?? randomUUID() }) }); + body: JSON.stringify({ action, expectedStateVersion: request.expectedStateVersion ?? stateVersion, + expectedReviewVersion: request.expectedReviewVersion ?? reviewVersion, actionId: request.actionId ?? randomUUID() }) }); return { status: response.status, body: await response.json() as Record }; } +function approvePlan(service: ReviewService) { + const view = service.load(); + service.store.saveReview(service.config.identity, view.expected, + view.items.map(item => approveItem(view.plan, view.segments, item.id, service.config.identity, item.count === 0)), []); +} + /** Before the runner exists: queue the task and leave one execute attempt of its first item, failed. */ function failedFirstItem(service: ReviewService, budgetMs?: number) { const s = service.store, id = service.config.identity; @@ -77,9 +86,36 @@ function committedFirstItem(service: ReviewService, unchanged = false, outOfScop function revise(service: ReviewService) { const s = service.store, id = service.config.identity, plan = s.getPlan(id); s.importRevision(JSON.stringify({ ...plan, revision: plan.revision + 1, summary: `${plan.summary} (revised)` }), 'json', service.planContext(), plan.revision); + const revised = s.getPlan(id), snapshot = s.getSnapshot(id); + s.saveReview(id, { revision: revised.revision, snapshotId: snapshot.id, reviewVersion: s.reviewVersion(id) }, + revised.items.map(item => approveItem(revised, [], item.id, id, true)), []); } describe('start (#91 part 2)', () => { + it('requires every item of the current plan revision to be approved before start', async () => { + const { app, identity, store } = await serve({ approve: false }); + expect(await view(app)).toMatchObject({ startable: false }); + expect((await act(app, 'start')).body.error).toMatch(/approve every plan item/i); + expect(store.getAttempts(identity)).toEqual([]); + }); + it('requires fresh current-snapshot approvals when the head moves before start', async () => { + const { app, identity, store } = await serve(); + const snapshot = store.getSnapshot(identity); + store.recordHistory(identity, { revision: store.getPlan(identity).revision, snapshotId: snapshot.id, + reviewVersion: store.reviewVersion(identity) }, snapshot.base, 'e'.repeat(40), []); + expect(await view(app)).toMatchObject({ startable: false }); + expect((await act(app, 'start')).body.error).toMatch(/approve every plan item/i); + expect(store.getAttempts(identity)).toEqual([]); + }); + it('requires approvals recorded after a later attribution choice', async () => { + const { app, identity, store } = await serve(); + const plan = store.getPlan(identity), snapshot = store.getSnapshot(identity); + store.saveReview(identity, { revision: plan.revision, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, [], + [{ key: 'later-choice', action: 'assign', item: plan.items[0]!.id }]); + expect(await view(app)).toMatchObject({ startable: false }); + approvePlan(app.service); + expect(await view(app)).toMatchObject({ startable: true }); + }); it('moves a task in review to queued and admits its first item, in one action', async () => { const { app, identity, store, items } = await serve(); expect(store.getTask(identity).status).toBe('in review'); @@ -174,6 +210,15 @@ describe('resume (#91 part 2)', () => { }); describe('start and resume refusals and races (#91 part 2)', () => { + it('guards start with the review version as well as the task state version', async () => { + const { app, identity, store } = await serve(); + const before = await view(app); + store.addReviewNote(identity, { revision: store.getPlan(identity).revision, snapshotId: store.getSnapshot(identity).id, + reviewVersion: before.reviewVersion }, store.getPlan(identity).items[0]!.id, 'question', 'Changed after the runner view.'); + expect((await act(app, 'start', { expectedStateVersion: before.stateVersion, expectedReviewVersion: before.reviewVersion })).body.error) + .toMatch(/Stale review state/); + expect(store.getAttempts(identity)).toEqual([]); + }); it('resumes a revised plan from its first item when the earlier revision committed nothing', async () => { const { app, identity, store, items } = await serve({ before: service => { failedFirstItem(service); revise(service); } }); expect(await view(app)).toMatchObject({ startable: false, resumable: true }); @@ -300,6 +345,106 @@ describe('start and resume refusals and races (#91 part 2)', () => { // The escalation committed, so the finding is no longer owed. expect(held.get(earlier)).toBeUndefined(); }); + it('acts on owed safety work before a scope-pause refusal', async () => { + let earlier = ''; + const { app, identity, store } = await serve({ before: service => { + const s = service.store, id = service.config.identity; + committedFirstItem(service, false, ['other.ts']); + const attempt = s.getAttempts(id)[0]!; + s.pauseForAmendment(id, { revision: attempt.context.planRevision, snapshotId: s.getSnapshot(id).id }, { + item: attempt.item!, completedItems: [attempt.item!], baseEntries: [], outOfScopePaths: ['other.ts'], + }); + earlier = attempt.id; + }, findings: (findings, service) => { + const save = service.store.recordSafetyFinding; + service.store.recordSafetyFinding = () => { throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); }; + findings.record(earlier, 'Safety violation: held after the pause'); + service.store.recordSafetyFinding = save; + } }); + expect((await act(app, 'resume')).body.result).toEqual({ outcome: 'settled' }); + expect(store.getTask(identity).status).toBe('needs amendment'); + expect(store.getAttempt(identity, earlier).safetyFinding).toMatch(/held after the pause/); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + expect((await act(app, 'resume')).body.result).toEqual({ outcome: 'settled' }); + expect(store.getTask(identity).status).toBe('needs human'); + }); + it('persists owed safety work before active-merge and pending-cancel refusals', async () => { + for (const blocker of ['merge', 'cancel'] as const) { + let earlier = ''; + const { app, identity, store } = await serve({ before: service => { + const s = service.store, id = service.config.identity; + if (blocker === 'merge') { + earlier = failedFirstItem(service).id; + s.transitionTask(id, s.getTask(id).stateVersion, 'in review'); + const snapshot = s.getSnapshot(id); + s.beginMergeAttempt(id, { revision: s.getPlan(id).revision, snapshotId: snapshot.id, + reviewVersion: s.reviewVersion(id) }, snapshot.head, null, 'direct'); + } else { + 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.cancelTask(id, s.getTask(id).stateVersion, randomUUID()); earlier = attempt.id; + } + }, findings: (findings, service) => { + const save = service.store.recordSafetyFinding; + service.store.recordSafetyFinding = () => { throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); }; + findings.record(earlier, `Safety violation: ${blocker}`); service.store.recordSafetyFinding = save; + } }); + expect((await act(app, blocker === 'merge' ? 'start' : 'resume')).body.result).toEqual({ outcome: 'settled' }); + expect(store.getAttempt(identity, earlier).safetyFinding).toMatch(new RegExp(blocker)); + } + }); + it('clears a recovery requeue when owed work settles without admitting an attempt', async () => { + let interrupted = ''; + const { app, identity, store } = await serve({ before: service => { + 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.recoverInterrupted(Date.now()); interrupted = attempt.id; + }, findings: (findings, service) => { + const save = service.store.recordSafetyFinding; + service.store.recordSafetyFinding = () => { throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); }; + findings.record(interrupted, 'Safety violation: recovered'); service.store.recordSafetyFinding = save; + } }); + expect(store.getTask(identity).requeuePending).toBe(true); + expect(await view(app)).toMatchObject({ startable: false, resumable: true }); + expect((await act(app, 'start')).body.error).toMatch(/recovery left this task to requeue/i); + expect(store.getTask(identity).requeuePending).toBe(true); + expect(store.getAttempt(identity, interrupted).safetyFinding).toBeNull(); + expect((await act(app, 'resume')).body.result).toEqual({ outcome: 'settled' }); + expect(store.getTask(identity)).toMatchObject({ status: 'needs human', requeuePending: false }); + expect(store.getAttempts(identity)).toHaveLength(1); + }); + it('refuses resume when the completed prefix no longer ends at the current task head', async () => { + const { app, identity, store } = await serve({ before: service => { committedFirstItem(service); } }); + const snapshot = store.getSnapshot(identity); + store.recordHistory(identity, { revision: store.getPlan(identity).revision, snapshotId: snapshot.id, + reviewVersion: store.reviewVersion(identity) }, snapshot.base, 'e'.repeat(40), []); + expect(await view(app)).toMatchObject({ resumable: false }); + expect((await act(app, 'resume')).body.error).toMatch(/completed plan prefix.*current task head/i); + expect(store.getAttempts(identity)).toHaveLength(1); + }); + it('inherits approvals only from snapshots used by the completed execution prefix', async () => { + const inherited = await serve({ before: service => { committedFirstItem(service); } }); + expect(await view(inherited.app)).toMatchObject({ resumable: true }); + + const returned = await serve({ before: service => { committedFirstItem(service, true); } }); + const { app, identity, store } = returned, atA = store.getSnapshot(identity); + store.recordHistory(identity, { revision: 1, snapshotId: atA.id, reviewVersion: store.reviewVersion(identity) }, + atA.base, 'e'.repeat(40), []); + const atB = store.getSnapshot(identity); + expect(atB.head).toBe('e'.repeat(40)); + const approvals = store.getReview(identity).approvals.map(({ item, fingerprint }) => ({ item, fingerprint })); + store.saveReview(identity, { revision: 1, snapshotId: atB.id, reviewVersion: store.reviewVersion(identity) }, approvals, []); + expect(store.getReview(identity).approvals).toHaveLength(store.getPlan(identity).items.length); + expect(store.getReview(identity).approvals.every(approval => approval.snapshotId === atB.id)).toBe(true); + store.recordHistory(identity, { revision: 1, snapshotId: atB.id, reviewVersion: store.reviewVersion(identity) }, + atA.base, atA.head, []); + expect(await view(app)).toMatchObject({ resumable: false }); + expect((await act(app, 'resume')).body.error).toMatch(/approve every plan item/i); + expect(store.getAttempts(identity)).toHaveLength(1); + }); it('pauses for an amendment owed from an earlier run instead of admitting, and reports it settled', async () => { const { app, identity, store } = await serve({ before: service => { committedFirstItem(service, false, ['other.ts']); } }); expect((await act(app, 'resume')).body.result).toEqual({ outcome: 'settled' }); @@ -308,7 +453,24 @@ describe('start and resume refusals and races (#91 part 2)', () => { expect(store.getAttempts(identity)).toHaveLength(1); expect(await view(app)).toMatchObject({ startable: false, resumable: false }); }); - it('moves a task in review to queued and then acts on an owed safety finding, in one action', async () => { + it('does not offer scope-only owed work from a state where the pause cannot be recorded', async () => { + for (const blocker of ['human', 'merge'] as const) { + const { app, identity, store } = await serve({ before: service => { + committedFirstItem(service, false, ['other.ts']); + const s = service.store, id = service.config.identity; + s.transitionTask(id, s.getTask(id).stateVersion, blocker === 'human' ? 'needs human' : 'in review'); + if (blocker === 'merge') { + const snapshot = s.getSnapshot(id); + s.beginMergeAttempt(id, { revision: s.getPlan(id).revision, snapshotId: snapshot.id, + reviewVersion: s.reviewVersion(id) }, snapshot.head, null, 'direct'); + } + } }); + expect(await view(app), blocker).toMatchObject({ startable: false, resumable: false }); + expect((await act(app, 'resume')).status, blocker).toBe(409); + expect(store.latestCheckpoint(identity), blocker).toBeNull(); + } + }); + it('acts on an owed safety finding before moving a task in review to queued', async () => { let earlier = ''; const { app, identity, store } = await serve({ before: service => { earlier = failedFirstItem(service).id; revise(service); @@ -322,9 +484,8 @@ describe('start and resume refusals and races (#91 part 2)', () => { } }); const before = store.getTask(identity).stateVersion, moves = vi.spyOn(store, 'transitionTask'); expect((await act(app, 'start')).body.result).toEqual({ outcome: 'settled' }); - // Queued first, then escalated from queued, both in the one action. - expect(moves.mock.calls.map(call => [call[1], call[2]])).toEqual([[before, 'queued'], [before + 1, 'needs human']]); - expect(store.getTask(identity).status).toBe('needs human'); + expect(moves).not.toHaveBeenCalled(); + expect(store.getTask(identity)).toMatchObject({ status: 'needs human', stateVersion: before + 1 }); expect(store.getAttempts(identity)).toHaveLength(1); }); it('replays an action ID, and refuses a second resume made against the old state', async () => { @@ -336,14 +497,31 @@ describe('start and resume refusals and races (#91 part 2)', () => { expect((await act(app, 'resume', { expectedStateVersion: stateVersion })).body.error).toMatch(/Stale task state/); expect(store.getAttempts(identity)).toHaveLength(2); }); + it('replays a pre-review-version action, but never creates a new action without the review version', async () => { + const actionId = randomUUID(); let stateVersion = 0; + const { app, identity, store } = await serve({ before: service => { + const s = service.store, id = service.config.identity; stateVersion = s.getTask(id).stateVersion; + s.userAction(id, { actionId, kind: 'start', request: { attemptId: undefined, expectedStateVersion: stateVersion } }, + () => ({ outcome: 'settled' })); + } }); + const post = async (id: string) => { + const response = await fetch(`${new URL(app.url).origin}/api/runner`, { method: 'POST', + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' }, + body: JSON.stringify({ action: 'start', expectedStateVersion: stateVersion, actionId: id }) }); + return { status: response.status, body: await response.json() as Record }; + }; + expect(await post(actionId)).toMatchObject({ status: 200, body: { result: { outcome: 'settled' } } }); + expect(await post(randomUUID())).toMatchObject({ status: 400, body: { error: expect.stringMatching(/expectedReviewVersion/) } }); + expect(store.getAttempts(identity)).toEqual([]); + }); it('answers 503 to a start whose body finishes arriving after shutdown began, before any refusal, and records nothing', async () => { // A task start would refuse: during shutdown the 503 still comes first, so no refusal is saved under the action ID. const { app, close, database, identity } = await serve({ before: service => { const s = service.store, id = service.config.identity; s.transitionTask(id, s.getTask(id).stateVersion, 'needs human'); } }); - const { stateVersion } = await view(app), actionId = randomUUID(), url = new URL(app.url); - const text = JSON.stringify({ action: 'start', expectedStateVersion: stateVersion, actionId }); + const { stateVersion, reviewVersion } = await view(app), actionId = randomUUID(), url = new URL(app.url); + const text = JSON.stringify({ action: 'start', expectedStateVersion: stateVersion, expectedReviewVersion: reviewVersion, actionId }); let finish!: () => void; const response = new Promise((resolve, reject) => { const req = httpRequest({ host: url.hostname, port: url.port, path: '/api/runner', method: 'POST', @@ -361,7 +539,8 @@ describe('start and resume refusals and races (#91 part 2)', () => { const store = new Store(database); cleanups.push(() => store.close()); expect(store.getTask(identity).status).toBe('needs human'); expect(store.getAttempts(identity)).toEqual([]); - expect(store.savedAction(identity, { actionId, kind: 'start', request: { attemptId: undefined, expectedStateVersion: stateVersion } })).toBeUndefined(); + expect(store.savedAction(identity, { actionId, kind: 'start', request: { attemptId: undefined, expectedStateVersion: stateVersion, + expectedReviewVersion: reviewVersion } })).toBeUndefined(); }); it('answers 503 to a start once the runner stopped admission, and leaves the task in review', async () => { const { app, identity, store } = await serve(); diff --git a/web/server.ts b/web/server.ts index 6fe9cf62..1628138c 100644 --- a/web/server.ts +++ b/web/server.ts @@ -117,18 +117,31 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (!executor || !progress) throw new GuardRefusal(config.demo ? RUNNER_NOT_IN_DEMO : RUNNER_NOT_CONFIGURED); // Shutdown began: answer 503 before any refusal, so nothing is recorded under the action ID and the UI may resend. if (!forView && runner!.closing) throw new ShuttingDownError(); - const task = service.store.getTask(identity), { started, begun, earlierCommits, next } = progress; + const task = service.store.getTask(identity), { started, begun, earlierCommits, completed, prefixHead, next } = progress; // Checked in the order a person can act on: a closed task first, then what admission would refuse. if (task.status === 'merged' || task.status === 'cancelled') throw new GuardRefusal(`The task is ${task.status}.`); - if (task.cancelRequested !== null) throw new GuardRefusal('The task is being cancelled.'); if (!runner!.runs('execute')) throw new GuardRefusal('The runner cannot run execute attempts yet.'); if (runner!.isActive(identity)) throw new GuardRefusal('An attempt is already active for this task.'); // Between two items no attempt is active, but the run that admits the next one is still going. if (executor.busy(identity)) throw new GuardRefusal('An earlier run of this task is still finishing; try again when that run has ended.'); // A publish reads the task head and pushes it; a new item must not move it meanwhile. if (publishing?.busy(identity)) throw new GuardRefusal('A pull request is being published for this task; try again when it has finished.'); + // Recovery owns this claim for Resume even when owed work will settle instead of admitting an item. Start must not + // consume it and strand the task behind the resulting human gate. + if (action === 'start' && task.requeuePending) throw new GuardRefusal('Recovery left this task to requeue; it cannot start until Resume resolves that claim.'); + // A failed durable save must not let an ordinary refusal strand an in-memory safety finding until restart loses it. + if (executor.owes(identity, { scope: false })) return { fromItem: next ?? service.store.getPlan(identity).items[0]!.id, + claimRequeue: task.requeuePending, queue: false, owed: true }; const merge = service.store.getMergeAttempt(identity); - if (merge && (merge.state === 'submitting' || merge.state === 'queued')) throw new GuardRefusal('A merge is in progress; wait for its outcome.'); + const activeMerge = !!merge && (merge.state === 'submitting' || merge.state === 'queued'); + // Scope-only debt is actionable only where pauseForAmendment can commit it. Otherwise expose the ordinary refusal; + // unlike a safety finding, repeatedly offering an impossible pause cannot preserve or improve durable evidence. + const scopePausable = ['running', 'in review', 'approved but merge blocked', 'queued'].includes(task.status) + && task.cancelRequested === null && !activeMerge; + if (scopePausable && executor.owes(identity)) return { fromItem: next ?? service.store.getPlan(identity).items[0]!.id, + claimRequeue: task.requeuePending, queue: false, owed: true }; + if (task.cancelRequested !== null) throw new GuardRefusal('The task is being cancelled.'); + if (activeMerge) throw new GuardRefusal('A merge is in progress; wait for its outcome.'); // Only the view stops here: the action lets admission refuse, because its refusal also moves the idle task to needs // human (the time-limit mapping), and nothing else would. if (forView && task.budgetDeadline !== null && task.budgetDeadline <= Date.now()) throw new GuardRefusal('The task time budget has run out; it needs a person.'); @@ -144,6 +157,8 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge throw new GuardRefusal('This plan has already started running; resume the task instead.'); if (task.status !== 'in review' && task.status !== 'queued') throw new GuardRefusal(`The task is ${task.status}; start runs a task that is in review or queued.`); if (ran) throw new GuardRefusal(begun ? `This plan revision has already run; the task is ${task.status}.` : 'Recovery left this task to requeue; it cannot start until that is resolved.'); + const unapproved = service.store.unapprovedExecutionItems(identity, service.store.getPlan(identity).revision); + if (unapproved.length) throw new GuardRefusal(`Approve every plan item before running it. Waiting for: ${unapproved.join(', ')}.`); return { fromItem: next!, claimRequeue: false, queue: task.status === 'in review' }; } // As for start: point to start only where start's own checks pass. @@ -155,6 +170,10 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge } if (!started && !task.requeuePending) throw new GuardRefusal(startWould ? toStart : 'No item of this plan has run yet, so there is nothing to resume.'); if (!next) throw new GuardRefusal('Every item of this plan has run.'); + if (completed.length && prefixHead !== service.store.getSnapshot(identity).head) + throw new GuardRefusal('The completed plan prefix no longer ends at the current task head; review the changed head before resuming.'); + const unapproved = service.store.unapprovedExecutionItems(identity, service.store.getPlan(identity).revision); + if (unapproved.length) throw new GuardRefusal(`Approve every plan item before running it. Waiting for: ${unapproved.join(', ')}.`); return { fromItem: next, claimRequeue: task.requeuePending, queue: false }; }; const offered = (action: 'start' | 'resume', progress: ReturnType) => { @@ -176,7 +195,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // One progress read per poll, shared by both flags. const progress = free && executor ? executor.progress(identity) : undefined; return { available: !!runner, task, attempts, startable: !!progress && offered('start', progress), resumable: !!progress && offered('resume', progress), - stateVersion: task.stateVersion, retryable, stopRequested: status.stopRequested, unresolved: status.unresolved, publish: publishView(progress) }; + stateVersion: task.stateVersion, reviewVersion: service.store.reviewVersion(identity), retryable, stopRequested: status.stopRequested, unresolved: status.unresolved, publish: publishView(progress) }; }; /** The task's publishing (#103): in progress, offered (what the publish action would run), and the last outcome. */ const publishView = (progress?: ReturnType) => { @@ -193,11 +212,20 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge .catch(error => console.error(`Runner run failed: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`)) .finally(() => publishing?.actIfOwed(identity)); const runnerAction = (input: Record) => { - const { action, attemptId, expectedStateVersion, actionId } = input; + const { action, attemptId, expectedStateVersion, expectedReviewVersion, 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', '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 === 'start' || action === 'resume') && !Number.isSafeInteger(expectedReviewVersion)) { + // Actions saved before #107 had no review version in their request hash. They remain replayable, but this shape + // can never create a new action now: a miss falls through to the new-field validation below. + const legacy = service.store.savedAction(identity, { actionId: actionId as string, kind: action as string, + request: { attemptId, expectedStateVersion } }); + if (legacy) return legacy.response; + throw new BadRequest('expectedReviewVersion must be an integer for start and resume.'); + } if (action === 'cancel-attempt' || action === 'retry') assertUuidV4(attemptId, 'Attempt ID'); + const request = { attemptId, expectedStateVersion, ...((action === 'start' || action === 'resume') ? { expectedReviewVersion } : {}) }; // A refused start or resume can still move the task to needs human (an expired budget is committed with the refusal), // and a task that moves there is owed a draft PR (#103). Only that move publishes: a person who pressed resume on a // task already in needs human asked for no publish. @@ -224,15 +252,18 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge 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 } }, () => { + function act() { return service.store.userAction(identity, { actionId: actionId as string, kind: action as string, request }, () => { if (service.store.getTask(identity).stateVersion !== expectedStateVersion) throw new GuardRefusal('Stale task state. Reload before writing.'); + if ((action === 'start' || action === 'resume') && service.store.reviewVersion(identity) !== expectedReviewVersion) + throw new GuardRefusal('Stale review state. Reload before writing.'); if (action === 'cancel-task') return { outcome: runner ? runner.cancelTask(identity, expectedStateVersion as number, actionId as string) : service.store.cancelTask(identity, expectedStateVersion as number, actionId as string) }; if (!runner) throw new GuardRefusal(config.demo ? RUNNER_NOT_IN_DEMO : RUNNER_NOT_CONFIGURED); if (action === 'start' || action === 'resume') { const choice = runChoice(action); // In this transaction with the admission: a refused admission rolls the move to queued back with it. if (choice.queue) service.store.transitionTask(identity, expectedStateVersion as number, 'queued'); - const begun = executor!.begin(identity, { fromItem: choice.fromItem, claimRequeue: choice.claimRequeue }); + const begun = executor!.begin(identity, { fromItem: choice.fromItem, claimRequeue: choice.claimRequeue, + expectedReviewVersion: expectedReviewVersion as number }); // The run goes on after this request; its outcome is in the task and attempt rows. An error the run throws (storage, // a missing snapshot) is only logged, and the next start or resume derives what is owed again. Once it ends, the // task's PR is published if the task is finished or needs a person (#103).