diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 1522b100..916d6094 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -230,6 +230,19 @@ codeboost never writes the user's repository. The commits it makes live in a **r | Review | Once a task has runner commits (a completed execute or fix attempt that made a commit), the review screen and Ask read it from the runner-owned repository at the head the Store recorded. The user's HEAD is no longer observed for it: runner commits move the head only through the Store, with their ledger entries, and observing the user's older HEAD would roll the task back. A task without runner commits still observes the user's HEAD, owned ledger entries or not: a reviewed branch in the user's repository (the demo, a planted experiment) carries them too. | +## Safety findings and partial output (#87 part 2) + +| What | How it works | +|---|---| +| A safety finding | The runner's own audit records it on the attempt (`recordSafetyFinding`), through the shutdown capability, before the terminal write. The terminal write then sends the task to needs human in the same transaction, whatever outcome wins: a stop or a stale context does not undo what the agent did. A pending cancel still closes the task. A task cannot change status while it has an active attempt, so no human gate can hold the finding back. The text stays on the row (`safetyFinding`). | +| A finding a crash left | Startup recovery acts on it the same way, and does not requeue the task. | +| Who started the attempt | It does not matter: the Store acts on the finding, so an attempt started through `/api/runner` start or retry goes to needs human too, and admission refuses new work from needs human. | +| A finding whose save failed | Held in memory (`SafetyFindings`) and acted on by `ItemExecutor`, as before: owed at a human gate until the task leaves it. | +| A failed run | When the agent ends badly on its own (a non-zero exit, a D stop reason, or a result that is not this attempt's, with no first reason), `auditFailed` inspects and audits what it left before the terminal write. A violation, or an inspection that refuses, is a finding. Nothing is committed. A run a person stopped, that went stale, hit the time limit or was shut down is not audited. | +| A `through-link` declared link | Never launched: preparation records a finding and fails, so the task goes to needs human. | +| Partial output | For a writable attempt that did not complete, `exportPartial` saves D's bounded diff (`exportTaskDiff`, 60 s deadline, never cut short by a stop) as `/.diff`, owner-only, written through an exclusive temporary file. The terminal write stores it as `diagnostic_ref`. A failed export adds "Partial output could not be exported: " (quoted) to the diagnostic instead. | +| Retention | Over the cap (256 MiB by default), unreferenced files go first, oldest first. Then the oldest referenced file: its row's `diagnostic_ref` is cleared and "(partial output removed by retention)" is appended, in one transaction, and only after that commit is the file deleted. No row ever points at a missing file. | + ## Shutdown `web/server.ts` `close()` and the runner coordinator follow this order. Part of step 1 and all of steps 2 and 3 already exist. F1 adds the coordinator barrier in step 1, steps 4 and 5, and step 8. diff --git a/runner/coordinator.ts b/runner/coordinator.ts index e35c149f..1e801ec2 100644 --- a/runner/coordinator.ts +++ b/runner/coordinator.ts @@ -49,6 +49,18 @@ export interface RunnerDeps { * saved with `completed` in one transaction. Throw FinishFailure with an actionable diagnostic to fail the attempt. */ finish?(attempt: AttemptRecord, result: InvocationResult, prepared: PreparedAttempt, signal: AbortSignal): Promise<{ value: unknown; history?: HistoryRecord }>; + /** + * Optional, for writable attempts whose agent ended badly on its own (a non-zero exit, a D stop reason, or a result + * that is not this attempt's, with no first reason): check what it left before the terminal write, recording any safety finding durably, so a failed run that + * did something unsafe goes to a person instead of being retried (#87 item 2). It commits nothing and never throws + * past its own failures: the attempt fails either way. + */ + auditFailed?(attempt: AttemptRecord, prepared: PreparedAttempt, signal: AbortSignal): Promise; + /** + * Optional, for writable attempts that did not complete: save their partial output before the terminal write, which + * stores the reference (`diagnostic_ref`). It bounds itself and reports a failure instead of throwing. + */ + exportPartial?(attempt: AttemptRecord, prepared: PreparedAttempt): Promise<{ diagnosticRef?: string; failure?: string }>; /** Optional: remove task storage after the terminal write and before the slot is freed. A failure keeps the slot under a marker. */ release?(attempt: AttemptRecord, prepared: PreparedAttempt): Promise; now?(): number; @@ -317,8 +329,9 @@ export class RunnerCoordinator { // Accept only the result of this exact invocation, as the question path does. Anything else is never validated // or saved: the attempt fails closed. if (result.attemptId !== attempt.id || !result.context || !sameContext(result.context, attempt.context)) { - const foreignSaved = this.#settle(job, { stopReason: 'capture-failure', exitCode: null, signal: null, valid: false, - detail: job.firstReason === 'stale' ? job.staleCause : FOREIGN_RESULT }); + // The result is not this attempt's, but the storage is: what the agent left is audited and kept all the same. + const evidence = await this.#keepEvidence(job, attempt, prepared, job.firstReason === 'stale' ? job.staleCause : FOREIGN_RESULT, true); + const foreignSaved = this.#settle(job, { stopReason: 'capture-failure', exitCode: null, signal: null, valid: false, ...evidence }); job.decided = true; // Task storage and host-side preparation files wait for the terminal write, as on every other path. if (foreignSaved) await this.#release(job, attempt, prepared); @@ -336,7 +349,10 @@ export class RunnerCoordinator { } // A stale stop keeps its own cause; the agent's stderr is not a reason the attempt went stale. if (job.firstReason === 'stale') detail = job.staleCause; - const saved = this.#settle(job, { stopReason: result.stopReason, exitCode: result.exitCode, signal: result.signal, valid, result: value, detail, history }); + // A clean exit was audited by finish already; only a run that ended badly on its own is audited here. + const clean = !job.firstReason && result.exitCode === 0 && !result.stopReason; + const evidence = valid ? { detail } : await this.#keepEvidence(job, attempt, prepared, detail, !clean); + const saved = this.#settle(job, { stopReason: result.stopReason, exitCode: result.exitCode, signal: result.signal, valid, result: value, history, ...evidence }); job.decided = true; // Task storage goes after the terminal write too; a failed removal holds the slot under a marker. if (saved) await this.#release(job, attempt, prepared); @@ -350,6 +366,26 @@ export class RunnerCoordinator { if (this.#jobs.get(job.key) === job) this.#jobs.delete(job.key); } } + /** + * For an attempt that ran and did not complete, before its terminal write: audit what the agent left when it ended + * badly on its own (no first reason), recording any safety finding, and keep its partial output for diagnosis. Returns + * the terminal write's detail and `diagnosticRef`. + */ + async #keepEvidence(job: Job, attempt: AttemptRecord, prepared: PreparedAttempt, detail: string | undefined, audit: boolean) + : Promise<{ detail?: string; diagnosticRef?: string }> { + if (audit && !job.firstReason && this.#deps.auditFailed) { + // The attempt fails whatever this finds. + try { await this.#deps.auditFailed(attempt, prepared, job.controller.signal); } + catch (error) { console.error(`Runner job ${job.attemptId} could not audit its failed run: ${JSON.stringify(message(error))}`); } + } + if (!this.#deps.exportPartial) return { detail }; + let exported: { diagnosticRef?: string; failure?: string }; + try { exported = await this.#deps.exportPartial(attempt, prepared); } + catch (error) { exported = { failure: message(error) }; } + // The reason can quote agent-chosen paths: quoted, like the agent's own text. + return { diagnosticRef: exported.diagnosticRef, detail: exported.failure + ? bounded(`${detail ?? ''} Partial output could not be exported: ${JSON.stringify(exported.failure)}`.trim()) : detail }; + } /** D gave up on cleanup (presence, not length, is the signal): fail closed until restart, as Ask does. */ #noteUnreleased(result: InvocationResult): void { if (result.unreleased === undefined) return; @@ -396,7 +432,7 @@ export class RunnerCoordinator { if (!this.#markers.has(job.key)) this.#markers.set(job.key, { group: job.group, attemptId: job.attemptId, reason: 'storage-not-removed' }); } } - #settle(job: Job, s: { stopReason?: StopReason; exitCode: number | null; signal: string | null; valid: boolean; result?: unknown; detail?: string; history?: HistoryRecord }): Classification | undefined { + #settle(job: Job, s: { stopReason?: StopReason; exitCode: number | null; signal: string | null; valid: boolean; result?: unknown; detail?: string; history?: HistoryRecord; diagnosticRef?: string }): Classification | undefined { try { return this.#write(() => this.#store.settleAttempt(job.identity, job.attemptId, { ...s, firstReason: job.firstReason })); } catch { diff --git a/runner/diagnostics.ts b/runner/diagnostics.ts new file mode 100644 index 00000000..bf48e3d5 --- /dev/null +++ b/runner/diagnostics.ts @@ -0,0 +1,55 @@ +import { randomUUID } from 'node:crypto'; +import { lstatSync, readdirSync, renameSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { isUuidV4 } from './lifecycle.ts'; +import type { Store } from './store.ts'; + +/** The total the diagnostics directory may hold before retention deletes files. */ +export const DEFAULT_DIAGNOSTICS_CAP_BYTES = 256 * 1024 * 1024; +const NAME = /^([0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})\.diff$/; + +/** + * Save a stopped attempt's partial output as `/.diff` (runner-lifecycle.md, the task storage + * row), then apply retention. The file is written through a unique temporary file opened exclusively, owner-only, and + * renamed into place; a failed write leaves nothing. Returns the path the attempt row references as `diagnostic_ref`. + */ +export function saveDiagnostic(store: Store, directory: string, attemptId: string, bytes: Buffer, + capBytes = DEFAULT_DIAGNOSTICS_CAP_BYTES): string { + if (!isUuidV4(attemptId)) throw new Error('Attempt ID must be a UUID v4.'); + if (!Number.isSafeInteger(capBytes) || capBytes < 1) throw new Error('The diagnostics cap must be a positive integer.'); + const path = join(directory, `${attemptId}.diff`), temporary = join(directory, `.${attemptId}.${randomUUID()}.tmp`); + try { + writeFileSync(temporary, bytes, { mode: 0o600, flag: 'wx' }); + renameSync(temporary, path); + } catch (error) { rmSync(temporary, { force: true }); throw error; } + // The saved file stands even if retention cannot run now (the write gate closed): the next save retains again. + try { retain(store, directory, capBytes, path); } + catch (error) { console.error(`Diagnostics retention did not run: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`); } + return path; +} + +/** + * Retention never deletes a file an attempt row still references. Over the cap, it deletes unreferenced files oldest + * first; if that is not enough, it takes the oldest referenced file and, in one transaction, clears that row's + * `diagnostic_ref` and notes the removal in its diagnostic, and only after that commit deletes the file. The file just + * saved (`keep`) is never removed: it is not referenced until the terminal write. + */ +export function retain(store: Store, directory: string, capBytes: number, keep?: string): void { + const files = readdirSync(directory).flatMap(name => { + const match = NAME.exec(name), path = join(directory, name); + if (!match) return []; + const stat = lstatSync(path, { throwIfNoEntry: false }); + return stat?.isFile() ? [{ path, attemptId: match[1]!, size: stat.size, mtime: stat.mtimeMs }] : []; + }).sort((a, b) => a.mtime - b.mtime); + let total = files.reduce((sum, file) => sum + file.size, 0); + const referenced = new Set(store.referencedDiagnostics()); + for (const unreferenced of [true, false]) { + for (const file of files) { + if (total <= capBytes) return; + if (file.path === keep || referenced.has(file.path) === unreferenced) continue; + if (!unreferenced && !store.forgetDiagnostic(file.path)) continue; + rmSync(file.path, { force: true }); + total -= file.size; + } + } +} diff --git a/runner/execution.ts b/runner/execution.ts index 8d482547..fc38eb2e 100644 --- a/runner/execution.ts +++ b/runner/execution.ts @@ -5,9 +5,11 @@ import { prepareExecution } from '../core/execution-prompt.ts'; import { neutralizeMentions, neutralizeReferences } from '../core/pull-request-body.ts'; import { auditRun, type ChangeManifest } from '../core/run-audit.ts'; import type { DeclaredLinkSnapshot } from '../agents/container/changes.ts'; +import { saveDiagnostic } from './diagnostics.ts'; +import { ownerOnlyDirectory } from './runner-repository.ts'; import { FinishFailure, NEEDS_RESTART, PreparationFailure, type PreparedAttempt, type RunnerCoordinator, type RunnerDeps } from './coordinator.ts'; import type { AttemptRecord, Store } from './store.ts'; -import { CLOSED_STATUSES, GuardRefusal, ShuttingDownError, bounded, sameContext, settleWith, type ShutdownCapability } from './lifecycle.ts'; +import { CLOSED_STATUSES, GuardRefusal, HUMAN_GATES, ShuttingDownError, bounded, sameContext, settleWith, type ShutdownCapability } from './lifecycle.ts'; /** * F2b: per-item execution and the runner's commit step (design, "How codeboost runs a plan"; plan-format.md, "After @@ -34,6 +36,11 @@ export interface TaskWorkspace { trailers: Readonly>; digest: string }, signal: AbortSignal): Promise; /** After the terminal write: remove task storage, and drop the commit of an attempt that did not complete. */ release(workspace: WorkspaceRef): Promise; + /** + * A stopped attempt's partial output (#87 item 1): the diff of what the agent left against `baseHead`, bounded by D. + * Read before the terminal write, while the task storage is still there. + */ + exportPartial?(workspace: WorkspaceRef, input: { baseHead: string }, signal: AbortSignal): Promise<{ diff: Buffer; truncated: boolean }>; /** Remove host-side preparation files (the staging clone). Called before the terminal write when D never ran, after it otherwise. */ cleanupPreparation?(attempt: AttemptRecord): Promise; } @@ -51,13 +58,34 @@ export const SAFETY_VIOLATION = 'Safety violation:'; /** * Safety violations the runner's own audit found, by attempt ID. executionDeps records them and ItemExecutor takes * 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. */ export class SafetyFindings { - #found = new Map(); - record(attemptId: string, reason: string): void { this.#found.set(attemptId, reason); } - /** A finding stays owed until its task has been moved to needs human (or closed); only then is it settled. */ - get(attemptId: string): string | undefined { return this.#found.get(attemptId); } - settle(attemptId: string): void { this.#found.delete(attemptId); } + #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); } + #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; + } + record(attemptId: string, reason: string): void { + try { + const identity = findIdentity(this.#store, { id: attemptId } as AttemptRecord); + // False only when the attempt already holds a finding (the first is kept) or is no longer active. + if (this.#write(() => this.#store.recordSafetyFinding(identity, attemptId, reason)) || this.#row(attemptId)?.safetyFinding) return; + } 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. */ + 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); } + settle(attemptId: string): void { this.#unsaved.delete(attemptId); } } export interface ExecutionResult { head: string; unchanged: boolean; inScope: string[]; outOfScope: string[] } interface Private { workspace: WorkspaceRef; prompt: string; baseHead: string; linkSnapshot: DeclaredLinkSnapshot | undefined } @@ -67,8 +95,36 @@ interface Private { workspace: WorkspaceRef; prompt: string; baseHead: string; l * `runnerOwner` is the database's runner token (`Store.runnerOwnerToken`); `workspace` must allocate task storage under it. */ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: AgentLauncher, sources: ExecutionSources, runnerOwner: string, - findings: SafetyFindings): RunnerDeps { + findings: SafetyFindings, options: { diagnostics?: { directory: string; capBytes?: number }; exportDeadlineMs?: number } = {}): RunnerDeps { const identityOf = (attempt: AttemptRecord): PlanIdentity => findIdentity(store, attempt); + /** + * Inspect what the agent left and audit it against the item: a violation, or an inspection or audit that refuses, is + * recorded as a safety finding and thrown as a FinishFailure. Only the stop's own abort comes back as itself. + */ + const audit = async (attempt: AttemptRecord, prepared: PreparedAttempt, signal: AbortSignal) => { + const data = prepared.private as Private, identity = identityOf(attempt); + const item = store.getPlan(identity, attempt.context.planRevision).items.find(entry => entry.id === attempt.item)!; + const violation = (reason: string): never => { + const text = bounded(`${SAFETY_VIOLATION} ${reason}`); + findings.record(attempt.id, text); + throw new FinishFailure(text); + }; + let manifest: ChangeManifest & { digest: string }; + try { manifest = await workspace.inspectChanges(data.workspace, { baseHead: data.baseHead, linkSnapshot: data.linkSnapshot! }, signal); } + catch (error) { + // Only the stop's own abort error is the stop. Any other refusal is a finding, even if a stop is also pending. + if (signal.aborted && (error === signal.reason || (error instanceof Error && error.name === 'AbortError'))) throw error; + // Contract (Publishing step 2): an inspection that refuses sends the task to needs human. + return violation(`The change inspection refused: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`); + } + // The commit step refuses a tree that no longer matches this digest; without one that guard has nothing to check. + if (typeof manifest?.digest !== 'string' || !manifest.digest) return violation('The change report has no digest.'); + let outcome: ReturnType; + try { outcome = auditRun(item, manifest, sources.planContext(identity).pathKey); } + catch (error) { return violation(`The change report could not be audited: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`); } + if (outcome.kind === 'violation') return violation(outcome.violations.join(' ')); + return { manifest, outcome }; + }; return { runnerOwner, async prepare(attempt, signal) { @@ -87,6 +143,15 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag const prepared = { clone: ws.clone, vendor, approvedArgv: request.approvedArgv, private: data }; try { data.linkSnapshot = await workspace.snapshotDeclaredLinks(ws, declaredPaths, signal); } catch (error) { throw new PreparationFailure(error, prepared); } + // A declared link whose way, or target, goes through another link is never launched (plan-format.md: a target never + // goes through another link): a write through it would land somewhere its snapshot does not watch. It is a safety + // finding, so the task goes to a person rather than retrying. + const through = data.linkSnapshot.links.filter(link => link.status === 'through-link').map(link => JSON.stringify(link.link)); + if (through.length) { + const text = bounded(`${SAFETY_VIOLATION} A declared link goes through another link, so a write through it would land outside its target: ${through.slice(0, 5).join(', ')}${through.length > 5 ? ` and ${through.length - 5} more` : ''}.`); + findings.record(attempt.id, text); + throw new PreparationFailure(new Error(text), prepared); + } return prepared; }, // Host-side files only (the staging clone); task storage waits for release. @@ -96,25 +161,7 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag async finish(attempt, _result, prepared, signal) { const data = prepared.private as Private, identity = identityOf(attempt); const plan = store.getPlan(identity, attempt.context.planRevision), item = plan.items.find(entry => entry.id === attempt.item)!; - const violation = (reason: string): never => { - const text = bounded(`${SAFETY_VIOLATION} ${reason}`); - findings.record(attempt.id, text); - throw new FinishFailure(text); - }; - let manifest: ChangeManifest & { digest: string }; - try { manifest = await workspace.inspectChanges(data.workspace, { baseHead: data.baseHead, linkSnapshot: data.linkSnapshot! }, signal); } - catch (error) { - // Only the stop's own abort error is the stop. Any other refusal is a finding, even if a stop is also pending. - if (signal.aborted && (error === signal.reason || (error instanceof Error && error.name === 'AbortError'))) throw error; - // Contract (Publishing step 2): an inspection that refuses sends the task to needs human. - return violation(`The change inspection refused: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`); - } - // The commit step refuses a tree that no longer matches this digest; without one that guard has nothing to check. - if (typeof manifest?.digest !== 'string' || !manifest.digest) return violation('The change report has no digest.'); - let outcome: ReturnType; - try { outcome = auditRun(item, manifest, sources.planContext(identity).pathKey); } - catch (error) { return violation(`The change report could not be audited: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`); } - if (outcome.kind === 'violation') return violation(outcome.violations.join(' ')); + const { manifest, outcome } = await audit(attempt, prepared, signal); if (outcome.unchanged) return { value: { head: data.baseHead, unchanged: true, inScope: [], outOfScope: [] } satisfies ExecutionResult }; // Last check before the commit, after the last await: a stop, shutdown or context change makes nothing. if (signal.aborted) throw signal.reason; @@ -142,6 +189,28 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag history: { base: snapshot.base, head, entries: [{ sha: head, owner: item.id, origin: 'owned', sourceSha: null }] }, }; }, + // The partial output of an attempt that did not complete (#87 item 1), saved where the attempt row will reference it. + // It has its own deadline: a stop must not cut it short, and nothing waits on it past that. + async exportPartial(attempt, prepared) { + const data = prepared.private as Private; + if (!workspace.exportPartial || !options.diagnostics) return {}; + const signal = AbortSignal.timeout(options.exportDeadlineMs ?? 60_000); + try { + const { diff } = await workspace.exportPartial(data.workspace, { baseHead: data.baseHead }, signal); + // Only the current user may write where diagnostics are kept. + ownerOnlyDirectory(options.diagnostics.directory); + return { diagnosticRef: saveDiagnostic(store, options.diagnostics.directory, attempt.id, diff, options.diagnostics.capBytes) }; + } catch (error) { + return { failure: signal.aborted ? 'the export did not finish within its deadline' : (error instanceof Error ? error.message : String(error)) }; + } + }, + // A failed run's work is audited too (#87 item 2): what it found is saved, so the terminal write sends the task to a + // person. Nothing is committed, and the attempt fails whatever this finds. + async auditFailed(attempt, prepared, signal) { + if ((prepared.private as Private).linkSnapshot === undefined) return; + try { await audit(attempt, prepared, signal); } + catch (error) { if (!(error instanceof FinishFailure)) throw error; } + }, async release(_attempt, prepared) { const data = prepared.private as Private; await workspace.release(data.workspace); @@ -168,11 +237,6 @@ export type ExecutionOutcome = const refusal = (error: unknown) => error instanceof GuardRefusal || error instanceof ShuttingDownError; /** A write outside settlement: no shutdown capability. */ const direct = (fn: () => T): T => fn(); -/** - * Statuses that wait for a person; leaving one needs its own user action (runner-lifecycle.md), so a safety finding is - * owed there instead. Review statuses are not gates: a finding moves them to needs human, so the task cannot be merged. - */ -const HUMAN_GATES: readonly string[] = ['needs amendment', 'needs approval', 'possibly already fixed']; /** * Runs a task's plan items in order, one execute attempt each. Stops at the first item that does not complete cleanly: @@ -253,8 +317,9 @@ export class ItemExecutor { } await this.#runner.settled(identity); const row = this.#store.getAttempt(identity, attempt.id); - // Only the runner's own audit records a finding; it wins over any later stale or stop outcome. - const violation = this.#findings.get(attempt.id); + // 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 (row.state !== 'completed') { // Still pending or running: the terminal write failed and the slot is held until restart. @@ -287,6 +352,12 @@ export class ItemExecutor { const unresolved = this.#runner.status(identity).unresolved; return stopped(item, owed ? 'not started' : row.state, `${violation} ${unresolved ? NEEDS_RESTART[unresolved.reason] : 'Needs restart: the attempt\'s outcome could not be saved.'}`); } + // A saved finding: the terminal write acted on it, so the task went to needs human then (or was closed by a pending + // cancel), and where it is now is a person's doing. + if (!this.#findings.unsaved(row.id)) { + return CLOSED_STATUSES.includes(task.status) ? stopped(item, row.state, `${violation} The task is ${task.status}, so it was not moved to needs human.`) + : { kind: 'needs human', item, reason: violation, completed: [...done] }; + } if (CLOSED_STATUSES.includes(task.status)) { 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.`); diff --git a/runner/lifecycle.ts b/runner/lifecycle.ts index 661f476f..bf9f7b17 100644 --- a/runner/lifecycle.ts +++ b/runner/lifecycle.ts @@ -11,6 +11,11 @@ export type TaskStatus = 'queued' | 'running' | 'needs human' | 'needs amendment export const TASK_STATUSES: readonly TaskStatus[] = ['queued', 'running', 'needs human', 'needs amendment', 'needs approval', 'possibly already fixed', 'in review', 'approved but merge blocked', 'merged', 'cancelled']; export const CLOSED_STATUSES: readonly TaskStatus[] = ['merged', 'cancelled']; +/** + * Statuses that wait for a person; leaving one needs its own user action (runner-lifecycle.md), so a safety finding is + * owed there instead. Review statuses are not gates: a finding moves them to needs human, so the task cannot be merged. + */ +export const HUMAN_GATES: readonly TaskStatus[] = ['needs amendment', 'needs approval', 'possibly already fixed']; /** A merge starts only from review; every other status is closed, running, queued or waiting for a person. */ export const MERGEABLE_STATUSES: readonly TaskStatus[] = ['in review', 'approved but merge blocked']; export const TERMINAL_STATES: readonly AttemptState[] = ['completed', 'failed', 'cancelled', 'stale']; diff --git a/runner/store.ts b/runner/store.ts index 1b5b181a..f59ab001 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -59,6 +59,11 @@ export interface AttemptRecord { id: string; kind: AttemptKind; phase: string; item: string | null; state: AttemptState; context: InvocationContext; deadline: number; firstReason: FirstReason | null; stopReason: StopReason | null; exitCode: number | null; signal: string | null; result: unknown; diagnostic: string | null; diagnosticRef: string | null; createdAt: string; startedAt: string | null; settledAt: string | null; + /** + * The runner's own audit found a safety violation (#87 item 3): its text, kept whatever outcome the attempt settled + * with. The terminal write (or startup recovery) acted on it: the task went to needs human, unless it was closed. + */ + safetyFinding: string | null; } export type FeedbackKind = 'reject' | 'change-request' | 'segment-accept' | 'segment-assign' | 'finding-accept' | 'needs-human-guidance' | 'task-closed'; const FEEDBACK_KINDS: readonly FeedbackKind[] = ['reject', 'change-request', 'segment-accept', 'segment-assign', 'finding-accept', 'needs-human-guidance', 'task-closed']; @@ -91,8 +96,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].includes(version)) throw new Error('Unsupported store schema version.'); - if (version === 7) return; + if (![0, 1, 2, 3, 4, 5, 6, 7, 8].includes(version)) throw new Error('Unsupported store schema version.'); + if (version === 8) 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)); @@ -122,6 +127,13 @@ export class Store { PRAGMA user_version=5;`); if (version < 6) this.#migrateV6(); if (version < 7) this.#migrateV7(); + // A safety finding outlives the process that found it (#87 item 3). + // Idempotent, like the v7 tables: a database moved back to an older version keeps the column. + if (version < 8) { + if (!this.#db.prepare('PRAGMA table_info(attempts)').all().some(column => column.name === 'safety_finding')) + this.#db.exec('ALTER TABLE attempts ADD COLUMN safety_finding TEXT'); + this.#db.exec('PRAGMA user_version=8'); + } }); } catch (error) { this.#db.close(); throw error; } } @@ -649,8 +661,33 @@ export class Store { result: row.result === null ? null : decode(row.result), diagnostic: row.diagnostic as string | null, diagnosticRef: row.diagnostic_ref as string | null, createdAt: row.created_at as string, startedAt: row.started_at as string | null, settledAt: row.settled_at as string | null, + safetyFinding: row.safety_finding as string | null, }; } + /** + * A safety finding sends the task to needs human (plan-format.md, "After each run"), in the transaction that settles + * its attempt. A task cannot change status while it has an active attempt, so it is still running here: no human gate + * can hold the finding back. + */ + #actOnFinding(key: string): void { + if (!this.#closed(this.#task(key).status as TaskStatus)) this.#run(`UPDATE tasks SET status='needs human' WHERE plan_key=?`, key); + } + /** + * Record the runner's own safety finding on an active attempt (#87 item 3), before its terminal write, like the first + * reason. The first finding is kept. The terminal write acts on it: never only memory, so a crash, a failed write or + * a start through another caller cannot lose it. + */ + recordSafetyFinding(identity: PlanIdentity, id: string, finding: string): boolean { + if (typeof finding !== 'string' || !finding.trim()) throw new GuardRefusal('A safety finding needs its reason.'); + const key = identityKey(identity); + return this.#transaction(() => { + const changed = this.#run(`UPDATE attempts SET safety_finding=? WHERE plan_key=? AND id=? AND state IN ('pending','running') AND safety_finding IS NULL`, + bounded(finding), key, id).changes === 1; + if (changed) this.#touch(key); + return changed; + }); + } + /** Every durable change to a task or its attempts increases the state version. */ #touch(key: string): void { this.#run('UPDATE tasks SET state_version=state_version+1, updated_at=? WHERE plan_key=?', new Date().toISOString(), key); @@ -851,10 +888,13 @@ export class Store { const context = decode(row.context); this.recordHistory(identity, { revision: context.planRevision, snapshotId: context.snapshotId }, settlement.history.base, settlement.history.head, settlement.history.entries); } - // A pending cancel task wins over everything, including the time limit. - if (task.cancel_requested !== null && !this.#closed(task.status)) this.#closeTask(key, 'cancelled', task.cancel_requested as string); - else { + // A pending cancel task wins over everything, including the time limit and a safety finding. + if (task.cancel_requested !== null && !this.#closed(task.status)) { + this.#closeTask(key, 'cancelled', task.cancel_requested as string); + } else { if (outcome.timeLimit && !this.#closed(task.status)) this.#run(`UPDATE tasks SET status='needs human' WHERE plan_key=?`, key); + // The runner's safety finding wins over the outcome: a stop or a stale context does not undo what the agent did. + if (row.safety_finding !== null) this.#actOnFinding(key); this.#touch(key); } return outcome; @@ -1249,6 +1289,24 @@ export class Store { return !!this.#get(`SELECT 1 AS found FROM attempts WHERE plan_key=? AND kind IN (${WRITABLE_KINDS.map(() => '?').join(',')}) AND state='completed' AND json_valid(result) AND json_extract(result, '$.unchanged') = 0 LIMIT 1`, identityKey(identity), ...WRITABLE_KINDS); } + /** Every partial-output file an attempt row references (`diagnostic_ref`). Retention never deletes one of these. */ + referencedDiagnostics(): string[] { + return this.#db.prepare('SELECT diagnostic_ref FROM attempts WHERE diagnostic_ref IS NOT NULL').all().map(row => row.diagnostic_ref as string); + } + /** + * Retention, over its cap: stop referencing this file, in one transaction that also notes the removal in the row's + * diagnostic. Only after it commits may the file be deleted, so no row ever points at a missing file. + */ + forgetDiagnostic(ref: string): boolean { + return this.#transaction(() => { + const rows = this.#db.prepare('SELECT id, plan_key FROM attempts WHERE diagnostic_ref=?').all(ref); + for (const row of rows) { + this.#run(`UPDATE attempts SET diagnostic_ref=NULL, diagnostic=TRIM(COALESCE(diagnostic,'') || ' (partial output removed by retention)') WHERE id=?`, row.id!); + this.#touch(row.plan_key as string); + } + return rows.length > 0; + }); + } /** "Preparation starting": saved before any preparation subprocess is spawned. */ markPreparationStarting(identity: PlanIdentity, id: string, startedAt: number): void { const key = identityKey(identity); @@ -1306,9 +1364,12 @@ export class Store { this.#run(`UPDATE attempts SET state=?, first_reason=?, diagnostic=?, diagnostic_ref=COALESCE(?, diagnostic_ref), settled_at=? WHERE id=?`, outcome.state, firstReason, bounded(diagnostic ?? ''), exported?.diagnosticRef ?? null, new Date(now).toISOString(), row.id!); let requeued = false; - if (task.cancel_requested !== null && !this.#closed(task.status)) this.#closeTask(key, 'cancelled', task.cancel_requested as string); - else { + if (task.cancel_requested !== null && !this.#closed(task.status)) { + this.#closeTask(key, 'cancelled', task.cancel_requested as string); + } else { if (outcome.timeLimit && !this.#closed(task.status)) this.#run(`UPDATE tasks SET status='needs human' WHERE plan_key=?`, key); + // A finding the process recorded before it stopped still sends the task to a person, and nothing requeues it. + if (row.safety_finding !== null) this.#actOnFinding(key); const status = this.#task(key).status as string; const interrupted = outcome.state === 'failed' && (outcome.reason ?? '').startsWith('Interrupted'); const shutdown = outcome.state === 'cancelled' && firstReason === 'shutdown'; diff --git a/runner/workspace.ts b/runner/workspace.ts index 38cb25fd..19689e9a 100644 --- a/runner/workspace.ts +++ b/runner/workspace.ts @@ -2,7 +2,7 @@ import { randomUUID } from 'node:crypto'; import { lstatSync, mkdirSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { commitTaskChanges, inspectTaskChanges, snapshotDeclaredLinks } from '../agents/container/changes.ts'; -import { prepareTaskFilesystemsAsync, removeTaskFilesystemsAsync, type TaskFilesystems, type TaskStorageLimits } from '../agents/container/storage.ts'; +import { exportTaskDiff, prepareTaskFilesystemsAsync, removeTaskFilesystemsAsync, type TaskFilesystems, type TaskStorageLimits } from '../agents/container/storage.ts'; import type { ProcessGroup } from '../agents/process-group.ts'; import { identityKey, type PlanIdentity } from '../core/identity.ts'; import { createTaskCloneAsync } from '../git/clone.ts'; @@ -84,6 +84,9 @@ export function createTaskWorkspace(options: WorkspaceOptions): TaskWorkspace { await fetchTaskCommit(repository, { bundle: made.bundle, base: input.baseHead, head: made.head, attemptId }, { signal }); return made.head; }, + async exportPartial(workspace, input, signal) { + return exportTaskDiff(held(workspace).filesystems, { base: input.baseHead, imageId, signal, timeoutMs: 60_000 }); + }, async release(workspace) { const { filesystems, attemptId, identity } = held(workspace); // Storage first: a failure here holds the slot under a marker, and nothing below may stop the removal. diff --git a/test/publish.test.ts b/test/publish.test.ts index 21e5018c..5b9d74b4 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -324,7 +324,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: 7 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 8 }); 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/runner-diagnostics.test.ts b/test/runner-diagnostics.test.ts new file mode 100644 index 00000000..f94e739e --- /dev/null +++ b/test/runner-diagnostics.test.ts @@ -0,0 +1,73 @@ +import { existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync, utimesSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import type { Plan, PlanContext } from '../core/plan.ts'; +import { retain, saveDiagnostic } from '../runner/diagnostics.ts'; +import { Store } from '../runner/store.ts'; + +const identity = { repositoryId: 'repo', taskId: 'task', planId: 'plan' }; +const context: PlanContext = { identity, issue: 1, baseEntries: [{ path: 'a', kind: 'file' }], pathKey: p => p, allowedCommands: [] }; +const plan: Plan = { schema_version: 1, revision: 1, issue: 1, summary: 'S', questions: [], items: [{ id: 'P1', title: 'T', intent: 'I', + files: [{ path: 'a', kind: 'edit', renamed_from: null, change: 'x' }], acceptance: [{ type: 'check', text: 'ok' }], depends_on: [] }] }; +const oid = (n: number) => n.toString(16).padStart(40, '0'); +const dirs: string[] = [], stores: Store[] = []; +afterEach(() => { for (const store of stores.splice(0)) store.close(); for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); + +function setup() { + const root = mkdtempSync(join(tmpdir(), 'diagnostics-')); dirs.push(root); + const store = new Store(join(root, 'state.sqlite')); stores.push(store); + store.createPlan(JSON.stringify(plan), 'json', context, oid(1), oid(2)); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + const directory = join(root, 'diagnostics'); mkdirSync(directory, { mode: 0o700 }); + /** A settled failed attempt that references `bytes` as its partial output, written `age` seconds ago. */ + const failed = (bytes: number, age: number) => { + const attempt = store.admitAttempt(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P1', + expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); + store.markRunning(identity, attempt.id); + const path = saveDiagnostic(store, directory, attempt.id, Buffer.alloc(bytes), 1e9); + const when = Date.now() / 1000 - age; utimesSync(path, when, when); + store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 1, valid: false, detail: 'crashed', diagnosticRef: path }); + return { id: attempt.id, path }; + }; + return { store, directory, failed }; +} + +describe('partial-output retention', () => { + it('writes owner-only through a temporary file, leaving only the named file', () => { + const { directory, failed } = setup(); + const { path } = failed(10, 0); + expect(readdirSync(directory)).toEqual([path.split('/').at(-1)]); + }); + + it('deletes unreferenced files first, oldest first, and never one a row still references', () => { + const { store, directory, failed } = setup(); + const kept = failed(40, 300); + const orphan = join(directory, '0b6d3e2a-1c4f-4a7e-9b2d-5f8e6c1a3b9d.diff'); writeFileSync(orphan, Buffer.alloc(40)); + const old = Date.now() / 1000 - 600; utimesSync(orphan, old, old); + retain(store, directory, 50); + expect(existsSync(orphan)).toBe(false); + expect(existsSync(kept.path)).toBe(true); + expect(store.getAttempt(identity, kept.id).diagnosticRef).toBe(kept.path); + }); + + it('past the cap, clears the oldest reference and notes it before deleting its file', () => { + const { store, directory, failed } = setup(); + const first = failed(40, 300), second = failed(40, 100); + retain(store, directory, 50); + expect(existsSync(first.path)).toBe(false); + expect(store.getAttempt(identity, first.id)).toMatchObject({ diagnosticRef: null, diagnostic: 'crashed (partial output removed by retention)' }); + // The newer one fits under the cap and keeps its reference. + expect(existsSync(second.path)).toBe(true); + expect(store.getAttempt(identity, second.id).diagnosticRef).toBe(second.path); + // No row points at a missing file. + for (const ref of store.referencedDiagnostics()) expect(existsSync(ref)).toBe(true); + }); + + it('never removes the file it just saved, even over the cap', () => { + const { store, directory } = setup(); + const id = 'c3a1b2d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d'; + const path = saveDiagnostic(store, directory, id, Buffer.alloc(100), 10); + expect(existsSync(path)).toBe(true); + }); +}); diff --git a/test/runner-execution.test.ts b/test/runner-execution.test.ts index 7d331e78..50eb4211 100644 --- a/test/runner-execution.test.ts +++ b/test/runner-execution.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, rmSync } from 'node:fs'; +import { mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { DatabaseSync } from 'node:sqlite'; import { join } from 'node:path'; @@ -29,7 +29,15 @@ const manifest = (changes: ManifestChange[], over: Partial; exit?: Record>; commit?: (item: string) => Promise; release?: () => Promise; startError?: Error; inspect?: (item: string, signal: AbortSignal) => Promise; snapshotError?: Error; capability?: (store: Store) => ShutdownCapability; settleError?: boolean; - plan?: Plan; commitHead?: string; pathKeyError?: Error; materializeError?: Error } = {}) { + plan?: Plan; commitHead?: string; pathKeyError?: Error; materializeError?: Error; + /** The durable save of a safety finding fails, so the executor must act on it from memory. */ + findingSaveError?: boolean; + /** The workspace's partial-output export: bytes, or an error. */ + partial?: Buffer | Error | 'hang'; diagnosticsCap?: number; exportDeadlineMs?: number; + /** Called by the fake launcher once the agent has started, before its result settles. */ + onLaunch?: (attemptId: string) => void; + /** The fake agent's result names this attempt instead. */ + foreignResult?: boolean } = {}) { const dir = mkdtempSync(join(tmpdir(), 'codeboost-exec-')); dirs.push(dir); const path = join(dir, 'state.sqlite'), store = new Store(path); store.createPlan(JSON.stringify(options.plan ?? plan), 'json', context, oid(1), oid(2)); @@ -39,7 +47,7 @@ function setup(options: { manifests?: Record (ws.storage as { item: string }).item; const workspace: TaskWorkspace = { async materialize(attempt, head) { log.push(`materialize ${attempt.item} @${head.slice(-3)}`); if (options.materializeError) throw options.materializeError; return { clone: { id: `c-${attempt.id}`, taskId: 'task', directory: '/tmp/x', head }, storage: { item: attempt.item, attemptId: attempt.id } }; }, - async snapshotDeclaredLinks(ws, paths) { log.push(`snapshot ${itemOf(ws)} [${paths.join(',')}]`); if (options.snapshotError) throw options.snapshotError; return { item: itemOf(ws) } as never; }, + async snapshotDeclaredLinks(ws, paths) { log.push(`snapshot ${itemOf(ws)} [${paths.join(',')}]`); if (options.snapshotError) throw options.snapshotError; return { item: itemOf(ws), links: [], targets: {} } as never; }, async inspectChanges(ws, input, signal) { log.push(`inspect ${itemOf(ws)} @${input.baseHead.slice(-3)}`); await options.inspect?.(itemOf(ws), signal); return options.manifests?.[itemOf(ws)] ?? manifest([change(itemOf(ws) === 'P1' ? 'a.ts' : 'b.ts')]); @@ -49,6 +57,13 @@ function setup(options: { manifests?: Record ${head.slice(-3)}`); return head; }, + ...options.partial ? { async exportPartial(ws: WorkspaceRef, _input: unknown, signal: AbortSignal) { + log.push(`export ${itemOf(ws)}`); + if (options.partial instanceof Error) throw options.partial; + // Like D's export: it stops when its signal aborts. + if (options.partial === 'hang') return new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true })); + return { diff: options.partial!, truncated: false }; + } } : {}, async release(ws) { const attemptId = (ws.storage as { attemptId: string }).attemptId; log.push(`release ${itemOf(ws)} after ${store.getAttempt(identity, attemptId).state}`); @@ -58,13 +73,16 @@ function setup(options: { manifests?: Record { throw options.pathKeyError; } } : context; const sources: ExecutionSources = { planContext: () => auditContext, issue: () => ({ number: 1, title: 'Issue', body: 'Please fix', comments: [] }), lessons: () => [], vendor: () => 'claude' }; const prompts: string[] = [], argv: (readonly (readonly string[])[])[] = [], owners: string[] = []; - const findings = new SafetyFindings(), capability = options.capability?.(store); + const capability = options.capability?.(store), findings = new SafetyFindings(store, capability); if (options.settleError) store.settleAttempt = () => { throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); }; + if (options.findingSaveError) store.recordSafetyFinding = () => { throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); }; const deps = executionDeps(store, workspace, (input, prompt, ws) => { if (options.startError) throw options.startError; log.push(`start ${itemOf(ws)}`); prompts.push(prompt); argv.push(input.approvedArgv); owners.push(input.runnerOwner); - return { attemptId: input.attemptId, settled: Promise.resolve({ attemptId: input.attemptId, context: input.context, exitCode: 0, signal: null, stdout: 'done', stderr: '', ...options.exit?.[itemOf(ws)] }), cancel: () => undefined }; - }, sources, RUNNER_OWNER, findings); + const settled = Promise.resolve().then(() => options.onLaunch?.(input.attemptId)).then(() => ({ attemptId: options.foreignResult ? randomUUID() : input.attemptId, + context: input.context, exitCode: 0, signal: null, stdout: 'done', stderr: '', ...options.exit?.[itemOf(ws)] })); + return { attemptId: input.attemptId, settled, cancel: () => undefined }; + }, sources, RUNNER_OWNER, findings, { diagnostics: { directory: join(dir, 'diagnostics'), capBytes: options.diagnosticsCap }, exportDeadlineMs: options.exportDeadlineMs }); const runner = new RunnerCoordinator(store, deps, undefined, capability); cleanups.push(async () => { await runner.close(); store.close(); }); return { store, path, workspace, findings, runner, executor: new ItemExecutor(store, runner, sources, findings, { capability }), log, commits, prompts, argv, owners }; @@ -76,8 +94,8 @@ describe('item execution', () => { expect(await executor.runTask(identity)).toEqual({ kind: 'executed', items: ['P1', 'P2'], unchanged: [] }); // Each commit gets the link snapshot its own item took before launch. expect(commits.map(c => [c.item, c.baseHead.slice(-3), c.trailers, c.linkSnapshot, c.message])).toEqual([ - ['P1', '002', { 'Plan-Item': 'P1', 'Plan-Revision': 'r1' }, { item: 'P1' }, 'P1: First'], - ['P2', '064', { 'Plan-Item': 'P2', 'Plan-Revision': 'r1' }, { item: 'P2' }, 'P2: Second']]); + ['P1', '002', { 'Plan-Item': 'P1', 'Plan-Revision': 'r1' }, { item: 'P1', links: [], targets: {} }, 'P1: First'], + ['P2', '064', { 'Plan-Item': 'P2', 'Plan-Revision': 'r1' }, { item: 'P2', links: [], targets: {} }, 'P2: Second']]); expect(store.getLedger(identity)).toEqual(expect.arrayContaining([ { sha: oid(100), owner: 'P1', origin: 'owned', sourceSha: null }, { sha: oid(101), owner: 'P2', origin: 'owned', sourceSha: null }])); expect(store.getSnapshot(identity).head).toBe(oid(101)); @@ -114,11 +132,55 @@ describe('item execution', () => { expect(store.getLedger(identity).some(entry => entry.owner === 'P1')).toBe(false); expect(log).toContain('release P1 after failed'); }); - it('stops on an agent failure without inspecting or committing', async () => { - const { store, executor, log } = setup({ exit: { P1: { exitCode: 1, stderr: 'agent crashed' } } }); + it('stops on an agent failure: what it left is inspected, never committed', async () => { + const { store, executor, log, commits } = setup({ exit: { P1: { exitCode: 1, stderr: 'agent crashed' } } }); expect(await executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'failed', reason: '"agent crashed"' }); - expect(log.some(line => line.startsWith('inspect'))).toBe(false); + expect(log.some(line => line.startsWith('inspect P1'))).toBe(true); + expect(commits).toEqual([]); expect(store.getSnapshot(identity).head).toBe(oid(2)); + expect(store.getTask(identity).status).toBe('running'); + }); + it('sends a failed run that left a safety violation to a person, so it is not retried (#87 item 2)', async () => { + for (const exit of [{ exitCode: 1, stderr: 'agent crashed' }, { exitCode: null, stopReason: 'timeout' as const }]) { + const { store, executor, commits } = setup({ exit: { P1: exit }, manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) } }); + expect(await executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1', reason: expect.stringContaining('Git metadata') }); + expect(commits).toEqual([]); + expect(store.getTask(identity).status).toBe('needs human'); + expect(store.getAttempts(identity)[0]).toMatchObject({ state: 'failed', safetyFinding: expect.stringContaining(SAFETY_VIOLATION) }); + } + // An inspection that refuses on a failed run is a finding too. + const refused = setup({ exit: { P1: { exitCode: 1 } }, inspect: async () => { throw new Error('docker run failed (exit 6): could not read x'); } }); + expect(await refused.executor.runTask(identity)).toMatchObject({ kind: 'needs human', reason: expect.stringContaining('The change inspection refused') }); + }); + it('audits a clean run once: a failed finish is not inspected again', async () => { + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) } }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); + expect(h.log.filter(line => line.startsWith('inspect'))).toHaveLength(1); + const refused = setup({ commit: async () => { throw new Error('work tree changed after the audit'); } }); + await refused.executor.runTask(identity); + expect(refused.log.filter(line => line.startsWith('inspect'))).toHaveLength(1); + }); + it('does not audit a run a person stopped while it ran', async () => { + let runner!: RunnerCoordinator; + const h = setup({ exit: { P1: { exitCode: 1 } }, manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + onLaunch: attemptId => runner.stop(identity, attemptId, 'cancelled') }); + runner = h.runner; + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'cancelled' }); + expect(h.log.some(line => line.startsWith('start P1'))).toBe(true); + expect(h.log.some(line => line.startsWith('inspect'))).toBe(false); + expect(h.store.getTask(identity).status).not.toBe('needs human'); + }); + it('audits and keeps what a run left when its result is not this attempt\'s', async () => { + const h = setup({ foreignResult: true, manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, partial: Buffer.from('d') }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); + expect(h.store.getAttempts(identity)[0]).toMatchObject({ state: 'failed', diagnosticRef: expect.stringMatching(/\.diff$/) }); + }); + it('gives up on an export at its own deadline and settles with that reason', async () => { + const h = setup({ exit: { P1: { exitCode: 1, stderr: 'crashed' } }, partial: 'hang', exportDeadlineMs: 50 }); + const began = performance.now(); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', state: 'failed' }); + expect(performance.now() - began).toBeLessThan(10_000); + expect(h.store.getAttempts(identity)[0]!.diagnostic).toBe('"crashed" Partial output could not be exported: "the export did not finish within its deadline"'); }); it('records no ledger entry when the commit is refused', async () => { const { store, executor } = setup({ commit: async () => { throw new Error('work tree changed after the audit'); } }); @@ -146,9 +208,11 @@ describe('item execution', () => { expect(runner.status(identity).unresolved).toMatchObject({ reason: 'result-not-saved' }); }); it('releases task storage after the terminal write when D settles with another attempt\'s result', async () => { - const { store, executor, log } = setup({ exit: { P1: { attemptId: '00000000-0000-4000-8000-000000000000' } } }); + const { store, executor, log, commits } = setup({ exit: { P1: { attemptId: '00000000-0000-4000-8000-000000000000' } } }); expect(await executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'failed' }); - expect(log.some(line => line.startsWith('inspect'))).toBe(false); + // What the run left is audited, but a result that is not this attempt's is never committed. + expect(log.some(line => line.startsWith('inspect P1'))).toBe(true); + expect(commits).toEqual([]); expect(log).toContain('release P1 after failed'); expect(store.getSnapshot(identity).head).toBe(oid(2)); }); @@ -339,18 +403,39 @@ describe('item execution', () => { store = scoped.store; expect(await scoped.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs approval'); - const unsafe = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, release: toApproval }); + // A finding the executor holds in memory (its save failed) waits at the gate and is escalated once it is left. + const unsafe = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, release: toApproval, findingSaveError: true }); 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'); - // Still at the gate on the next run: the finding stays owed, reported as not started. expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'not started', reason: expect.stringMatching(/needs approval; it moves to needs human/) }); - // A person releases the gate; the owed finding goes to needs human before any item runs again. store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs human'); 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 () => { + const h = setup(); + h.workspace.snapshotDeclaredLinks = async () => ({ links: [{ link: 'a.ts', status: 'through-link' as const, anchor: { path: 'via', type: 'symlink' as const, + mode: '120777', size: 1, ino: 1, ctime: '0', mtime: '0' } }], targets: {} }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1', + reason: expect.stringContaining('A declared link goes through another link, so a write through it would land outside its target: "a.ts"') }); + expect(h.log.some(line => line.startsWith('start '))).toBe(false); + expect(h.store.getTask(identity).status).toBe('needs human'); + // The storage preparation allocated is still released after the terminal write. + expect(h.log.some(line => line.startsWith('release P1 after failed'))).toBe(true); + }); + it('acts on a saved finding at the terminal write, before release can set a gate (#87)', async () => { + let store!: Store, atRelease: string | undefined; + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + release: async () => { atRelease = store.getTask(identity).status; store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs approval'); } }); + store = h.store; + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1', reason: expect.stringContaining(SAFETY_VIOLATION) }); + // Already needs human when release ran, and nothing left owed: the gate after it is a person's doing. + expect(atRelease).toBe('needs human'); + const row = store.getAttempts(identity)[0]!; + expect(row).toMatchObject({ state: 'failed', safetyFinding: expect.stringContaining('Git metadata') }); + }); it('treats an audit that throws as a safety violation', async () => { const { store, executor, commits } = setup({ pathKeyError: new Error('Non-ASCII case-insensitive paths require an adapter.') }); expect(await executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1', @@ -431,7 +516,7 @@ describe('item execution', () => { store = scoped.store; expect(await scoped.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1' }); expect(store.getTask(identity).status).toBe('queued'); - const unsafe = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, release: toQueued }); + const unsafe = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: toQueued }); store = unsafe.store; expect(await unsafe.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs human'); @@ -446,7 +531,7 @@ describe('item execution', () => { }); it('leaves a safety violation\'s task alone when it was cancelled during release', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.cancelTask(identity, store.getTask(identity).stateVersion, randomUUID()); } }); store = h.store; expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', reason: expect.stringMatching(/is cancelled, so it was not moved/) }); @@ -499,7 +584,7 @@ describe('item execution', () => { expect(store.getTask(identity).status).toBe('cancelled'); }); it('keeps a safety finding owed when moving to needs human fails, and escalates it before the next run launches anything', async () => { - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) } }); + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true }); const transition = h.store.transitionTask.bind(h.store); let fail = true; h.store.transitionTask = (...args) => { if (fail && args[2] === 'needs human') { fail = false; throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); } return transition(...args); }; @@ -528,7 +613,7 @@ describe('item execution', () => { }); it('escalates a safety violation over a review status, so the task cannot be merged past it', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.transitionTask(identity, store.getTask(identity).stateVersion, 'in review'); } }); store = h.store; expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); @@ -549,7 +634,7 @@ describe('item execution', () => { }); it('keeps a finding owed when a merge in progress refuses the escalation, and escalates it on the next run', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, release: async () => { + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.transitionTask(identity, store.getTask(identity).stateVersion, 'in review'); const snapshot = store.getSnapshot(identity); store.beginMergeAttempt(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: store.reviewVersion(identity) }, snapshot.head, null, 'direct'); @@ -572,7 +657,7 @@ describe('item execution', () => { }); it('settles an owed finding when the task was closed before its next run', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs approval'); } }); store = h.store; await h.executor.runTask(identity); @@ -635,7 +720,7 @@ describe('item execution', () => { }); it('keeps possibly already fixed as a human gate for a safety finding', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.transitionTask(identity, store.getTask(identity).stateVersion, 'possibly already fixed'); } }); store = h.store; expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', reason: expect.stringMatching(/possibly already fixed; it moves to needs human/) }); @@ -703,7 +788,7 @@ describe('item execution', () => { }); it('keeps needs amendment as a human gate for a safety finding', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs amendment'); } }); store = h.store; expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', reason: expect.stringMatching(/needs amendment; it moves to needs human/) }); @@ -760,7 +845,7 @@ describe('item execution', () => { }); it('does not pay an owed finding through the shutdown capability once the write gate has 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.transitionTask(identity, store.getTask(identity).stateVersion, 'needs approval'); } }); store = h.store; await h.executor.runTask(identity); @@ -786,8 +871,9 @@ describe('item execution', () => { const outcome = await h.executor.runTask(identity) as { kind: string; reason: string }; expect(outcome.kind).toBe('stopped'); expect(outcome.reason).toMatch(/Needs restart: the last result could not be saved\./); - expect(h.findings.get(h.store.getAttempts(identity)[0]!.id)).toBeDefined(); - // The next run finds it owed, reports it for the earlier item, and starts nothing. + // Saved on the still-active attempt: startup recovery acts on it, whatever this process does next. + expect(h.store.getAttempts(identity)[0]).toMatchObject({ state: 'running', safetyFinding: expect.stringContaining('Git metadata') }); + // The next run reports the held slot for the earlier item, and starts nothing. expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'not started', reason: expect.stringMatching(/Needs restart/) }); }); it('ends as the stop, not a refused commit, when a stop lands and the commit then rejects', async () => { @@ -858,7 +944,7 @@ describe('item execution', () => { }); it('pays nothing owed once shutdown began', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs approval'); } }); store = h.store; await h.executor.runTask(identity); @@ -896,23 +982,45 @@ describe('item execution', () => { expect(outcome).toMatchObject({ kind: 'needs amendment', item: 'P2', completed: ['P1', 'P2'] }); expect(h.store.getCheckpoint(identity, outcome.checkpointId)).toMatchObject({ item: 'P2', completedItems: ['P1', 'P2'], outOfScopePaths: ['extra.ts'] }); }); - it('escalates an older owed finding even when a later attempt completed cleanly', async () => { + it('acts on a finding of an attempt started outside the executor, and admits nothing after it (#87 item 4)', async () => { const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) } }); const store = h.store; - // Both attempts start outside the executor, so nothing acts on P1's finding yet. + // Started through the coordinator directly, as /api/runner start or retry would. h.runner.start(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P1', expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); await h.runner.settled(identity); - h.runner.start(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P2', expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); - await h.runner.settled(identity); - expect(store.getAttempts(identity).map(row => row.state)).toEqual(['failed', 'completed']); - expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); - expect(store.getAttempts(identity)).toHaveLength(2); + expect(store.getTask(identity).status).toBe('needs human'); + expect(store.getAttempts(identity)[0]).toMatchObject({ state: 'failed', safetyFinding: expect.stringContaining(SAFETY_VIOLATION) }); + expect(() => h.runner.start(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P2', + expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 })).toThrow(/needs human/); + expect(store.getAttempts(identity)).toHaveLength(1); }); it('throws, rather than reporting stopped, when this run\'s own escalation meets a closed write gate without a capability', async () => { let store!: Store; - const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, release: async () => { store.closeWrites(); } }); + const h = setup({ manifests: { P1: manifest([change('a.ts')], { metadataChanged: true }) }, findingSaveError: true, release: async () => { store.closeWrites(); } }); store = h.store; await expect(h.executor.runTask(identity)).rejects.toBeInstanceOf(ShuttingDownError); expect(h.findings.get(store.getAttempts(identity)[0]!.id)).toBeDefined(); }); + it('keeps the partial output of an attempt that did not complete, referenced from its row (#87 item 1)', async () => { + const failed = setup({ exit: { P1: { exitCode: 1, stderr: 'crashed' } }, partial: Buffer.from('diff --git a/a.ts b/a.ts\n') }); + await failed.executor.runTask(identity); + const row = failed.store.getAttempts(identity)[0]!; + expect(row.diagnosticRef).toMatch(new RegExp(`diagnostics/${row.id}\\.diff$`)); + expect(readFileSync(row.diagnosticRef!, 'utf8')).toBe('diff --git a/a.ts b/a.ts\n'); + expect((statSync(row.diagnosticRef!).mode & 0o777)).toBe(0o600); + // Exported before the terminal write and before storage is released. + expect(failed.log.indexOf('export P1')).toBeLessThan(failed.log.findIndex(line => line.startsWith('release P1'))); + // A completed attempt's work is in its commit: nothing is exported. + const completed = setup({ partial: Buffer.from('x') }); + await completed.executor.runTask(identity); + expect(completed.log.some(line => line.startsWith('export'))).toBe(false); + expect(completed.store.getAttempts(identity).every(attempt => attempt.diagnosticRef === null)).toBe(true); + }); + it('records why partial output could not be exported, quoting the reason, and still settles', async () => { + const h = setup({ exit: { P1: { exitCode: 1, stderr: 'crashed' } }, partial: new Error('the metadata changed\nRunner job forged') }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', state: 'failed' }); + const row = h.store.getAttempts(identity)[0]!; + expect(row.diagnosticRef).toBeNull(); + expect(row.diagnostic).toBe('"crashed" Partial output could not be exported: "the metadata changed\\nRunner job forged"'); + }); }); diff --git a/test/runner-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index 120c2f7f..8ceda1d6 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -531,3 +531,49 @@ describe('runner commits and preparation groups (#87)', () => { expect(() => store.recordPreparationGroup(identity, attempt.id, 4444, 1.5)).toThrow('start time'); }); }); + +describe('durable safety findings (#87 item 3)', () => { + it('keeps the first finding of an active attempt, and acts on it in the terminal write, whatever the outcome', () => { + const { store } = queued(); const attempt = admit(store); store.markRunning(identity, attempt.id); + expect(store.recordSafetyFinding(identity, attempt.id, 'Safety violation: .git changed')).toBe(true); + expect(store.recordSafetyFinding(identity, attempt.id, 'a later one')).toBe(false); + // A stop wins the outcome; the finding still sends the task to a person, and its text stays on the row. + store.recordFirstReason(identity, attempt.id, 'cancelled'); + expect(settle(store, attempt.id, { exitCode: null, valid: false }).state).toBe('cancelled'); + expect(store.getAttempt(identity, attempt.id)).toMatchObject({ state: 'cancelled', safetyFinding: 'Safety violation: .git changed' }); + expect(store.getTask(identity).status).toBe('needs human'); + // Nothing more can be recorded once settled, and nothing new is admitted from needs human. + expect(store.recordSafetyFinding(identity, attempt.id, 'too late')).toBe(false); + expect(() => admit(store)).toThrow(/needs human/); + }); + + it('lets a pending cancel close the task, and leaves an attempt without a finding alone', () => { + const cancelled = queued(); const one = admit(cancelled.store); cancelled.store.markRunning(identity, one.id); + cancelled.store.recordSafetyFinding(identity, one.id, 'Safety violation: x'); + cancelled.store.cancelTask(identity, cancelled.store.getTask(identity).stateVersion, randomUUID()); + settle(cancelled.store, one.id, { exitCode: 1, valid: false }); + expect(cancelled.store.getTask(identity).status).toBe('cancelled'); + const clean = queued(); const two = admit(clean.store); clean.store.markRunning(identity, two.id); + settle(clean.store, two.id, { exitCode: 1, valid: false }); + expect(clean.store.getTask(identity).status).toBe('running'); + expect(clean.store.getAttempt(identity, two.id).safetyFinding).toBeNull(); + }); + + it('acts on a finding a crash left unsettled, and does not requeue its task', () => { + const { store } = queued(); const attempt = admit(store); store.markRunning(identity, attempt.id); + store.recordSafetyFinding(identity, attempt.id, 'Safety violation: link target changed'); + expect(store.recoverInterrupted(Date.now())).toEqual([expect.objectContaining({ attemptId: attempt.id, requeued: false })]); + expect(store.getTask(identity).status).toBe('needs human'); + expect(store.getAttempt(identity, attempt.id).safetyFinding).toBe('Safety violation: link target changed'); + }); + + it('adds the finding column to a version 7 database', () => { + const { path, store } = queued(); const attempt = admit(store); + store.close(); stores.splice(stores.indexOf(store), 1); + const db = new DatabaseSync(path); + 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: 8 }); + }); +}); diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index e2d9e363..acf4e107 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -50,7 +50,7 @@ async function setup(agent: Record) { committer: { name: 'codeboost', email: 'runner@codeboost.invalid' }, now: () => 1_700_000_000_000 }); const sources: ExecutionSources = { planContext: () => context, issue: () => ({ number: 1, title: 'Issue', body: 'Fix', comments: [] }), lessons: () => [], vendor: () => 'claude' }; - const findings = new SafetyFindings(); + const findings = new SafetyFindings(store); // The agent: a container that edits the work volume, mounted as an agent container mounts it (metadata read-only). const deps = executionDeps(store, workspace, (input, _prompt, ws) => { const { workVolume, metadataVolume } = (ws.storage as { filesystems: TaskFilesystems }).filesystems; @@ -119,4 +119,23 @@ describe('real task workspace (#87)', () => { expect(leftovers(s.store)).toEqual([]); expect(existsSync(join(s.runnerRoot, RUNNER_OWNER, 'attempts', s.store.getAttempts(identity)[0]!.id))).toBe(false); }, 600_000); + + it('keeps a scope pause approvable after the review loads it from the runner-owned repository', async () => { + const s = await setup({ P1: 'printf "a2\\n" > a.ts && printf "extra\\n" > extra.ts' }); + const outcome = await s.executor.runTask(identity) as { kind: string; checkpointId: string }; + expect(outcome).toMatchObject({ kind: 'needs amendment', item: 'P1', outOfScope: ['extra.ts'] }); + const checkpoint = s.store.getCheckpoint(identity, outcome.checkpointId); + // The review load reads the runner commit at the recorded head: it does not replace the checkpoint's snapshot. + const review = new ReviewService({ database: join(s.root, 'state.sqlite'), repository: s.source, runnerRepository: s.repository.path, + identity, pathIdentity: { caseSensitive: true, unicodeNormalization: 'none' } }); + try { expect(review.load().snapshot.id).toBe(checkpoint.snapshotId); } finally { review.close(); } + // A person amends the plan to declare the extra file, then approves continuing from the audited checkpoint. + const amended = { ...plan, items: [{ ...plan.items[0]!, files: [...plan.items[0]!.files, { path: 'extra.ts', kind: 'add' as const, renamed_from: null, change: 'z' }] }, plan.items[1]!] }; + s.store.importRevision(JSON.stringify(amended), 'json', { identity, issue: 1, pathKey: p => p, allowedCommands: [], + baseEntries: [{ path: 'a.ts', kind: 'file' }, { path: 'b.ts', kind: 'file' }] }, 1); + const revision = s.store.getPlan(identity).revision; + expect(() => s.store.approveContinuation(identity, outcome.checkpointId, { revision, snapshotId: checkpoint.snapshotId, + reviewVersion: s.store.reviewVersion(identity) })).not.toThrow(); + expect(s.store.continuationRevision(identity, outcome.checkpointId)).toBe(revision); + }, 600_000); });