From 8f60af153edc16ad7b9a9715477c9cc71fddb87c Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 3 Oct 2026 22:04:55 -0700 Subject: [PATCH 1/8] F: redraft the current plan through lane D as a durable request (#124) - Store schema v11: requests.mode ('suggest' | 'draft'); earlier requests are suggestions. completeSuggestions checks the reply by mode (a draft is a v1 plan at revision n+1 for the plan's issue); getDraft and applyDraft, which imports a ready, still-bound draft as n+1 with every import check. applySuggestion refuses drafts and applyDraft refuses suggestions. - E3: SuggestionCoordinator.start(input, mode). Draft mode prepares with prepareDraft at n+1; stale checks compare the base revision; one active planning invocation per plan for both modes. - Server: POST /api/plan/drafts, GET /api/plan/drafts/:id, and /api/plan/drafts/:id/(cancel|apply), recorded like suggestions; each kind's routes refuse the other's IDs. Co-Authored-By: Claude Opus 5.5 --- core/planning-suggestions.ts | 35 +++-- docs/implementation/runner-lifecycle.md | 6 +- runner/store.ts | 67 ++++++++-- test/planning-drafts-api.test.ts | 93 ++++++++++++++ test/planning-drafts.test.ts | 164 ++++++++++++++++++++++++ test/publish.test.ts | 2 +- test/runner-lifecycle-store.test.ts | 4 +- test/store.test.ts | 5 +- web/server.ts | 39 ++++-- 9 files changed, 373 insertions(+), 42 deletions(-) create mode 100644 test/planning-drafts-api.test.ts create mode 100644 test/planning-drafts.test.ts diff --git a/core/planning-suggestions.ts b/core/planning-suggestions.ts index 295b0754..67ea7e70 100644 --- a/core/planning-suggestions.ts +++ b/core/planning-suggestions.ts @@ -1,16 +1,21 @@ import { identityKey, type PlanIdentity } from './identity.ts'; -import { prepareSuggestions, type AuthorInput, type AuthorProvider } from './planning-author.ts'; +import { prepareDraft, prepareSuggestions, type AuthorInput, type AuthorProvider, type PreparedAuthor } from './planning-author.ts'; import type { Diagnostic, EditReply, Plan } from './plan.ts'; /** Implemented by the existing Store. No SQL or second persistence writer in core. */ export interface SuggestionStore { getPlan(identity: PlanIdentity): Plan; getSnapshot(identity: PlanIdentity): { id: string; base: string; head: string }; - beginSuggestions(identity: PlanIdentity, expected: { revision: number; snapshotId: string }): string; + beginSuggestions(identity: PlanIdentity, expected: { revision: number; snapshotId: string }, mode?: PlanningMode): string; completeSuggestions(identity: PlanIdentity, id: string, reply: unknown): void; settleSuggestion(identity: PlanIdentity, id: string, expected: { revision: number; snapshotId: string }, outcome: { state: 'failed' | 'cancelled' | 'invalidated'; reason: string }): boolean; getSuggestions(identity: PlanIdentity, id: string): { state: string; revision: number; snapshotId: string | null; reply: EditReply | null; reason: string | null }; } +/** + * A planning request (#124): typed edit cards for the current plan, or a whole draft of its next revision. `revision` + * in the input is always the current revision the request is bound to. + */ +export type PlanningMode = 'suggest' | 'draft'; export type SuggestionInput = Omit & { snapshotId: string }; export type SuggestionOutcome = | { state: 'completed'; id: string; warnings: Diagnostic[] } @@ -32,7 +37,10 @@ function retainDiagnostic(reason: string | null, diagnostic: string): string { return reason && reason !== diagnostic ? `${reason} ${diagnostic}` : diagnostic; } -/** One instance per runner. Close it before closing Store. Not a cross-process scheduler. */ +/** + * One instance per runner. Close it before closing Store. Not a cross-process scheduler. Suggestions and drafts share + * one active invocation per plan. + */ export class SuggestionCoordinator { #store: SuggestionStore; #provider: AuthorProvider; @@ -45,7 +53,7 @@ export class SuggestionCoordinator { throw new Error('Suggestion timeout must fit a positive timer interval.'); this.#store = store; this.#provider = provider; this.#timeoutMs = timeoutMs; } - start(input: SuggestionInput): SuggestionHandle { + start(input: SuggestionInput, mode: PlanningMode = 'suggest'): SuggestionHandle { if (this.#closing) throw new Error('Suggestion coordinator is closing.'); const identity = { ...input.context.identity }, key = identityKey(identity); if (this.#active.has(key)) throw new Error('A suggestion invocation is still active for this plan.'); @@ -54,9 +62,12 @@ export class SuggestionCoordinator { if (previousPlan.revision !== input.revision) throw new Error('Stale plan revision.'); if (snapshot.id !== snapshotId || snapshot.base !== input.repo.baseSha) throw new Error('Stale repository snapshot.'); // Validate before allocating a durable request; no await permits local state changes. - const prepared = prepareSuggestions({ ...input, requestId: 'pending', previousPlan }); + const prepared: PreparedAuthor = mode === 'draft' + ? prepareDraft({ ...input, revision: input.revision + 1, requestId: 'pending', previousPlan }) + : prepareSuggestions({ ...input, requestId: 'pending', previousPlan }); const expected = Object.freeze({ revision: input.revision, snapshotId }); - const id = this.#store.beginSuggestions(identity, expected); + const noun = mode === 'draft' ? 'Draft' : 'Suggestion'; + const id = this.#store.beginSuggestions(identity, expected, mode); const request = Object.freeze({ ...prepared.request, requestId: id }); const controller = new AbortController(); let stopped: Omit | undefined; @@ -84,11 +95,11 @@ export class SuggestionCoordinator { catch (error) { stopped.reason += ` Request cleanup failed: ${String(error)}`; } finally { controller.abort(new Error(reason)); } }; - const timer = setTimeout(() => stop('failed', 'Suggestion invocation timed out.'), this.#timeoutMs); + const timer = setTimeout(() => stop('failed', `${noun} invocation timed out.`), this.#timeoutMs); let finish!: (result: SuggestionOutcome) => void; const result = new Promise(resolve => { finish = resolve; }); const handle: SuggestionHandle = Object.freeze({ id, result, - cancel: (reason = 'Suggestion cancelled by user.') => stop('cancelled', reason) }); + cancel: (reason = `${noun} cancelled by user.`) => stop('cancelled', reason) }); this.#active.set(key, { handle, stop }); // Defer invocation until the handle owns its slot, including synchronous provider errors. void Promise.resolve().then(async () => { @@ -101,11 +112,11 @@ export class SuggestionCoordinator { if (afterInvocation) outcome = { id, ...afterInvocation }; else { const current = this.#store.getSuggestions(identity, id); - if (this.#store.getPlan(identity).revision !== request.revision || + if (this.#store.getPlan(identity).revision !== expected.revision || this.#store.getSnapshot(identity).id !== snapshot.id || current.state === 'invalidated') { outcome = { id, state: 'stale', reason: 'Plan revision or snapshot changed during authoring.' }; } else if (current.state !== 'pending') { - outcome = { id, state: current.state === 'cancelled' ? 'cancelled' : 'stale', reason: `Suggestion request is ${current.state}.` }; + outcome = { id, state: current.state === 'cancelled' ? 'cancelled' : 'stale', reason: `${noun} request is ${current.state}.` }; } else { const validated = prepared.validate(source); this.#store.completeSuggestions(identity, id, validated.value); @@ -120,11 +131,11 @@ export class SuggestionCoordinator { if (!stopped) { try { const current = this.#store.getSuggestions(identity, id); - if (current.state === 'invalidated' || this.#store.getPlan(identity).revision !== request.revision || + if (current.state === 'invalidated' || this.#store.getPlan(identity).revision !== expected.revision || this.#store.getSnapshot(identity).id !== snapshot.id) outcome = { id, state: 'stale', reason: `Plan revision or snapshot changed before publication. ${outcome.reason}` }; else if (current.state === 'cancelled') - outcome = { id, state: 'cancelled', reason: `Suggestion request was cancelled before publication. ${outcome.reason}` }; + outcome = { id, state: 'cancelled', reason: `${noun} request was cancelled before publication. ${outcome.reason}` }; } catch { /* Preserve the original error if durable state cannot be read. */ } } } diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index ec673412..f96d7b1e 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -356,8 +356,12 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina | `GET /api/plan/suggestions/:id` | `Store.getSuggestions` | reads only the request row | | `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle (for example after a restart), F calls `Store.cancelSuggestions`. It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | | `POST /api/plan/suggestions/:id/apply` | `Store.applySuggestion` | request `ready` and bound to the current revision and snapshot | +| `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan | expected revision and snapshot; one active planning request per plan, shared with suggestions | +| `GET /api/plan/drafts/:id` | `Store.getDraft` | reads only a draft request's row | +| `POST /api/plan/drafts/:id/cancel` | as suggestion cancel | request is a draft, `pending` or `ready` | +| `POST /api/plan/drafts/:id/apply` | `Store.applyDraft`: imports the draft as revision + 1 with every import check | request is a draft, `ready` and bound to the current revision and snapshot | -Live planning runs for a non-demo review with a `github` block (#117): `web/planning.ts` reads the issue from GitHub before each start, and `runner/planning.ts` runs the request in planning's worker. E3's suggestion timer and the request share one ten-minute budget (`PLANNING_BUDGET_MS`). At shutdown E3 aborts every request; one that has not settled within 20 seconds (`PLANNING_SHUTDOWN_GRACE_MS`, as Ask) is ended by abandoning planning's worker. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is not recorded. Without a `github` block, or in a demo, the server returns "Planning agent not available yet". +Live planning runs for a non-demo review with a `github` block (#117): `web/planning.ts` reads the issue from GitHub before each start, and `runner/planning.ts` runs the request in planning's worker. E3's suggestion timer and the request share one ten-minute budget (`PLANNING_BUDGET_MS`). At shutdown E3 aborts every request; one that has not settled within 20 seconds (`PLANNING_SHUTDOWN_GRACE_MS`, as Ask) is ended by abandoning planning's worker. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is not recorded. Without a `github` block, or in a demo, the server returns "Planning agent not available yet". Drafts follow the same rules (#124). A draft and a suggestion are both rows of `requests`, told apart by `mode` (schema v11); each kind's routes refuse the other's IDs. ## Feedback-event contract (for lane J) diff --git a/runner/store.ts b/runner/store.ts index a9b4cf84..a90f881e 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -2,7 +2,7 @@ import type { DatabaseSync, SQLInputValue, SQLOutputValue } from 'node:sqlite'; import { createRequire } from 'node:module'; import { randomUUID } from 'node:crypto'; import { identityKey, type PlanIdentity } from '../core/identity.ts'; -import { importPlan, applySuggestion, assertEditReply, type Plan, type PlanContext, type EditReply } from '../core/plan.ts'; +import { importPlan, applySuggestion, assertEditReply, assertPlan, type Plan, type PlanContext, type EditReply } from '../core/plan.ts'; import type { Approval, SegmentChoice } from '../core/approvals.ts'; import type { InvocationContext, StopReason } from '../agents/contract.ts'; import type { AlreadyFixedResult } from '../github/already-fixed.ts'; @@ -20,7 +20,12 @@ export function requireSupportedNode(version = process.versions.node): void { export interface Snapshot { id: string; base: string; head: string } export interface ReviewState { revision: number; snapshotId: string; reviewVersion?: number } export type SuggestionState = 'pending' | 'ready' | 'failed' | 'cancelled' | 'invalidated' | 'consumed'; -export interface SuggestionRequest { state: SuggestionState; revision: number; snapshotId: string | null; reply: EditReply | null; reason: string | null } +/** A planning request: card suggestions for the current plan, or a whole draft of its next revision (#124). */ +export type PlanningMode = 'suggest' | 'draft'; +/** `reply` is the suggestion cards; for a draft it is always null here (read it with `getDraft`). */ +export interface SuggestionRequest { mode: PlanningMode; state: SuggestionState; revision: number; snapshotId: string | null; reply: EditReply | null; reason: string | null } +/** A draft request: `revision` is the plan revision it was drafted against; `plan` would become revision + 1. */ +export interface DraftRequest { state: SuggestionState; revision: number; snapshotId: string | null; plan: Plan | null; reason: string | null } export interface SnippetReference { key: string; path: string; side: 'old' | 'new'; start: number; end: number; text: string; head: string; base: string } export interface QuestionAnswer { provider?: 'claude' | 'codex'; attempt: string; contextId?: string; status: 'pending' | 'complete' | 'failed'; expiresAt: number; text?: string; error?: string } export interface ReviewNote { id: string; item: string; kind: 'question' | 'change'; text: string; reference?: SnippetReference; answer?: QuestionAnswer; createdAt: string; revision: number; snapshotId: string } @@ -102,8 +107,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].includes(version)) throw new Error('Unsupported store schema version.'); - if (version === 9) return; + 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 (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)); @@ -148,6 +153,12 @@ export class Store { if (!columns.includes('storage_base')) this.#db.exec('ALTER TABLE attempts ADD COLUMN storage_base TEXT'); this.#db.exec('PRAGMA user_version=9'); } + // Planning requests carry their mode (#124): every earlier request is a suggestion. Idempotent, like v8. + if (version < 11) { + if (!this.#db.prepare('PRAGMA table_info(requests)').all().some(column => column.name === 'mode')) + this.#db.exec("ALTER TABLE requests ADD COLUMN mode TEXT NOT NULL DEFAULT 'suggest'"); + this.#db.exec('PRAGMA user_version=11'); + } }); } catch (error) { this.#db.close(); throw error; } } @@ -296,21 +307,28 @@ export class Store { if (!row) throw new Error('Unknown snapshot.'); return decode(row.data); } - beginSuggestions(identity: PlanIdentity, expected: ReviewState): string { + beginSuggestions(identity: PlanIdentity, expected: ReviewState, mode: PlanningMode = 'suggest'): string { + if (mode !== 'suggest' && mode !== 'draft') throw new Error('Invalid planning request mode.'); const key = identityKey(identity); return this.#transaction(() => { this.#expect(key, expected); const id = randomUUID(); - this.#run("INSERT INTO requests (id,key,revision,state,reply,snapshot_id,reason) VALUES (?,?,?,'pending',NULL,?,NULL)", id, key, expected.revision, expected.snapshotId); return id; + this.#run("INSERT INTO requests (id,key,revision,state,reply,snapshot_id,reason,mode) VALUES (?,?,?,'pending',NULL,?,NULL,?)", id, key, expected.revision, expected.snapshotId, mode); return id; }); } + /** + * Publish a request's validated reply, by the request's own mode: suggestion cards against the current revision, or + * a draft plan of the next revision for the plan's issue. The full plan checks run again when a draft is applied. + */ completeSuggestions(identity: PlanIdentity, id: string, reply: unknown): void { - assertEditReply(reply); const key = identityKey(identity); this.#transaction(() => { const current = this.#current(key); - if (reply.base_revision !== current.revision || this.#run("UPDATE requests SET state='ready',reply=?,reason=NULL WHERE id=? AND key=? AND revision=? AND snapshot_id=? AND state='pending'", encode(reply), id, key, current.revision!, current.snapshot_id!).changes !== 1) - throw new Error('Suggestion request is stale, cancelled, or complete.'); + const row = this.#get('SELECT mode FROM requests WHERE id=? AND key=?', id, key); + const fits = row?.mode === 'draft' ? (assertPlan(reply), reply.revision === (current.revision as number) + 1 && reply.issue === current.issue) + : (assertEditReply(reply), reply.base_revision === current.revision); + if (!fits || this.#run("UPDATE requests SET state='ready',reply=?,reason=NULL WHERE id=? AND key=? AND revision=? AND snapshot_id=? AND state='pending'", encode(reply), id, key, current.revision!, current.snapshot_id!).changes !== 1) + throw new Error(`${row?.mode === 'draft' ? 'Draft' : 'Suggestion'} request is stale, cancelled, or complete.`); }); } settleSuggestion(identity: PlanIdentity, id: string, expected: ReviewState, outcome: { state: 'failed' | 'cancelled' | 'invalidated'; reason: string }): boolean { @@ -422,13 +440,40 @@ export class Store { getSuggestions(identity: PlanIdentity, id: string): SuggestionRequest { const row = this.#get('SELECT * FROM requests WHERE key=? AND id=?', identityKey(identity), id); if (!row) throw new Error('Unknown suggestion request.'); - return { state: row.state as SuggestionState, revision: row.revision as number, snapshotId: row.snapshot_id as string | null, reply: row.reply === null ? null : decode(row.reply), reason: row.reason as string | null }; + const mode = row.mode as PlanningMode; + return { mode, state: row.state as SuggestionState, revision: row.revision as number, snapshotId: row.snapshot_id as string | null, + reply: row.reply === null || mode === 'draft' ? null : decode(row.reply), reason: row.reason as string | null }; + } + /** A draft request (#124). Refuses a suggestion request's ID. */ + getDraft(identity: PlanIdentity, id: string): DraftRequest { + const row = this.#get("SELECT * FROM requests WHERE key=? AND id=? AND mode='draft'", identityKey(identity), id); + if (!row) throw new Error('Unknown draft request.'); + return { state: row.state as SuggestionState, revision: row.revision as number, snapshotId: row.snapshot_id as string | null, + plan: row.reply === null ? null : decode(row.reply), reason: row.reason as string | null }; + } + /** + * Apply a ready draft as the next revision (#124): the draft must still be bound to the current revision and + * snapshot, and it passes every check an import does. The request is then consumed, so a replay is refused. + */ + applyDraft(identity: PlanIdentity, id: string, context: PlanContext): Plan { + const key = identityKey(identity); + return this.#transaction(() => { + this.#context(key, context); + const request = this.#get("SELECT * FROM requests WHERE id=? AND key=? AND state='ready' AND mode='draft'", id, key); + if (!request) throw new Error('Draft is unavailable.'); + const current = this.#current(key); + if (request.revision !== current.revision || request.snapshot_id !== current.snapshot_id) throw new Error('Draft is unavailable.'); + const revision = current.revision as number; + const plan = importPlan(request.reply as string, 'json', context, revision + 1).plan; + this.#savePlan(key, plan, revision); + this.#run("UPDATE requests SET state='consumed',reason=NULL WHERE id=?", id); return plan; + }); } applySuggestion(identity: PlanIdentity, id: string, index: number, context: PlanContext): Plan { const key = identityKey(identity); return this.#transaction(() => { this.#context(key, context); - const request = this.#get("SELECT * FROM requests WHERE id=? AND key=? AND state='ready'", id, key); + const request = this.#get("SELECT * FROM requests WHERE id=? AND key=? AND state='ready' AND mode='suggest'", id, key); if (!request) throw new Error('Suggestion is unavailable.'); const current = this.#current(key); if (request.revision !== current.revision || request.snapshot_id !== current.snapshot_id) throw new Error('Suggestion is unavailable.'); diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts new file mode 100644 index 00000000..0bd05616 --- /dev/null +++ b/test/planning-drafts-api.test.ts @@ -0,0 +1,93 @@ +import { randomUUID } from 'node:crypto'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, expect, it, vi } from 'vitest'; +import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; +import type { EditReply } from '../core/plan.ts'; +import { createDemo } from '../scripts/demo.ts'; +import { startServer, type PlanningDeps } from '../web/server.ts'; + +// The draft API (#124) on the real server and Store, with a stand-in provider. +vi.setConfig({ testTimeout: 20_000 }); +const roots: string[] = [], closers: (() => Promise)[] = []; +afterEach(async () => { + for (const close of closers.splice(0).reverse()) await close(); + roots.splice(0).forEach(root => rmSync(root, { recursive: true, force: true })); +}); +type Answer = (request: AuthorRequest, signal: AbortSignal) => Promise; +type View = { plan: Record; snapshot: { id: string } }; +/** `answer` is made from the review as it was when the server started. */ +async function serve(answer: (view: View) => Answer) { + const root = mkdtempSync(join(tmpdir(), 'codeboost-drafts-api-')); roots.push(root); + const requests: AuthorRequest[] = []; + let answering!: Answer; + const provider: AuthorProvider = { invoke: (request, signal) => { requests.push(request); return answering(request, signal); } }; + let issue = 0; + const planning: PlanningDeps = { provider, describe: () => ({ issue: { number: issue, title: 'Retries', body: '', comments: [] }, + approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' } }) }; + const app = await startServer(createDemo(join(root, 'demo')), 0, async () => 'answer', undefined, 2_000, undefined, undefined, planning); + closers.push(() => app.close()); + const api = async (method: string, path: string, body?: unknown) => { + const response = await fetch(`${new URL(app.url).origin}${path}`, { method, headers: { 'x-codeboost-token': app.token, + ...(body ? { 'content-type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined }); + return { status: response.status, body: await response.json() as Record }; + }; + const view = (await api('GET', '/api/review')).body as View; + issue = view.plan.issue; answering = answer(view); + const start = (kind: 'drafts' | 'suggestions') => api('POST', `/api/plan/${kind}`, { expectedRevision: view.plan.revision, + snapshotId: view.snapshot.id, feedback: 'Make it two items.', actionId: randomUUID() }); + const settled = async (kind: 'drafts' | 'suggestions', id: string) => { + await vi.waitFor(async () => expect((await api('GET', `/api/plan/${kind}/${id}`)).body.state).not.toBe('pending')); + return (await api('GET', `/api/plan/${kind}/${id}`)).body; + }; + return { api, view, requests, start, settled }; +} +/** The review's plan, redrafted as the next revision with a new summary. */ +const redraft = (view: View): Answer => async request => JSON.stringify({ ...view.plan, revision: request.revision, summary: 'Redrafted by Claude' }); +const cards = (revision: number): EditReply => ({ schema_version: 1, base_revision: revision, reply: 'Rename', edits: [{ op: 'set_field', + item: 'P1', summary: 'Rename', reason: 'Clearer', field: 'title', value: 'Clearer title', file: null, check: null, check_index: null, depends_on: null, new_item: null }] }); + +it('drafts the next revision, keeps it until applied, then applies it exactly once', async () => { + const served = await serve(redraft); + const started = await served.start('drafts'); + expect(started).toMatchObject({ status: 200, body: { result: { requestId: expect.any(String) } } }); + const id = started.body.result.requestId as string; + expect(served.requests[0]).toMatchObject({ mode: 'draft', revision: served.view.plan.revision + 1, requestId: id }); + const draft = await served.settled('drafts', id); + expect(draft).toMatchObject({ state: 'ready', revision: served.view.plan.revision, plan: { summary: 'Redrafted by Claude' } }); + // Still the old revision until the user applies the draft. + expect((await served.api('GET', '/api/review')).body.plan.revision).toBe(served.view.plan.revision); + const apply = { actionId: randomUUID() }; + const applied = await served.api('POST', `/api/plan/drafts/${id}/apply`, apply); + expect(applied).toEqual({ status: 200, body: { result: { revision: served.view.plan.revision + 1 } } }); + expect(await served.api('POST', `/api/plan/drafts/${id}/apply`, apply)).toEqual(applied); + expect(await served.api('POST', `/api/plan/drafts/${id}/apply`, { actionId: randomUUID() })) + .toMatchObject({ status: 409, body: { error: 'Draft is unavailable.' } }); + expect((await served.api('GET', '/api/review')).body.plan).toMatchObject({ revision: served.view.plan.revision + 1, summary: 'Redrafted by Claude' }); +}); + +it('keeps draft and suggestion IDs on their own routes', async () => { + const served = await serve(view => async (request, signal) => request.mode === 'draft' ? redraft(view)(request, signal) + : JSON.stringify(cards(view.plan.revision))); + const draft = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', draft); + const suggestion = (await served.start('suggestions')).body.result.requestId as string; + await served.settled('suggestions', suggestion); + expect(await served.api('GET', `/api/plan/suggestions/${draft}`)).toMatchObject({ status: 409, body: { error: 'Unknown suggestion request.' } }); + expect(await served.api('GET', `/api/plan/drafts/${suggestion}`)).toMatchObject({ status: 409, body: { error: 'Unknown draft request.' } }); + expect(await served.api('POST', `/api/plan/suggestions/${draft}/apply`, { index: 0, actionId: randomUUID() })) + .toMatchObject({ status: 409, body: { error: 'Unknown suggestion request.' } }); + expect(await served.api('POST', `/api/plan/drafts/${suggestion}/cancel`, { actionId: randomUUID() })) + .toMatchObject({ status: 409, body: { error: 'Unknown draft request.' } }); + // Both are still ready: nothing above changed them. + expect((await served.api('GET', `/api/plan/drafts/${draft}`)).body.state).toBe('ready'); + expect((await served.api('GET', `/api/plan/suggestions/${suggestion}`)).body).toMatchObject({ mode: 'suggest', state: 'ready' }); +}); + +it('cancels a draft while Claude is still writing it', async () => { + const served = await serve(() => (_, signal) => new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true }))); + const id = (await served.start('drafts')).body.result.requestId as string; + expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, { actionId: randomUUID() })).toEqual({ status: 200, body: { result: { state: 'cancelling' } } }); + expect(await served.settled('drafts', id)).toMatchObject({ state: 'cancelled', plan: null, reason: 'Cancelled by the user.' }); +}); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts new file mode 100644 index 00000000..2b797042 --- /dev/null +++ b/test/planning-drafts.test.ts @@ -0,0 +1,164 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { DatabaseSync } from 'node:sqlite'; +import { afterEach, expect, it } from 'vitest'; +import type { EditReply, Plan, PlanContext } from '../core/plan.ts'; +import type { AuthorRequest } from '../core/planning-author.ts'; +import { SuggestionCoordinator, type SuggestionInput } from '../core/planning-suggestions.ts'; +import { Store } from '../runner/store.ts'; + +// Plan drafts (#124): a whole next revision of the current plan, kept as a durable request until it is applied. +const identity = { repositoryId: 'repo', taskId: 'task', planId: 'plan' }; +const context: PlanContext = { identity, issue: 1, baseEntries: [{ path: 'a', kind: 'file' }, { path: 'b', kind: 'file' }], pathKey: p => p, allowedCommands: [] }; +const plan = (revision = 1, summary = 'Example'): Plan => ({ schema_version: 1, revision, issue: 1, summary, questions: [], + items: [{ id: 'P1', title: 'Change', intent: 'Improve', files: [{ path: 'a', kind: 'edit', renamed_from: null, change: 'Change' }], + acceptance: [{ type: 'check', text: 'Works' }], depends_on: [] }] }); +const redraft = (revision = 2): Plan => ({ ...plan(revision, 'Redrafted'), items: [ + ...plan().items, { id: 'P2', title: 'Second', intent: 'Also', files: [{ path: 'b', kind: 'edit', renamed_from: null, change: 'Change b' }], + acceptance: [{ type: 'check', text: 'Works too' }], depends_on: ['P1'] }] }); +const cards = (revision = 1): EditReply => ({ schema_version: 1, base_revision: revision, reply: 'Suggestion', edits: [{ op: 'set_field', + item: 'P1', summary: 'Rename', reason: 'Clearer', field: 'title', value: 'Updated', file: null, check: null, check_index: null, depends_on: null, new_item: null }] }); +const oid = (n: number) => n.toString(16).padStart(40, '0'); + +const dirs: string[] = [], stores: Store[] = []; +afterEach(() => { stores.splice(0).forEach(store => store.close()); dirs.splice(0).forEach(dir => rmSync(dir, { recursive: true, force: true })); }); +function fixture() { + const dir = mkdtempSync(join(tmpdir(), 'codeboost-drafts-')); dirs.push(dir); + const path = join(dir, 'state.sqlite'), open = () => { const store = new Store(path); stores.push(store); return store; }; + const store = open(); + store.createPlan(JSON.stringify(plan()), 'json', context, oid(1), oid(2)); + return { store, open, path }; +} +const state = (store: Store) => ({ revision: store.getPlan(identity).revision, snapshotId: store.getSnapshot(identity).id }); +function readyDraft(store: Store, reply: unknown = redraft()) { + const id = store.beginSuggestions(identity, state(store), 'draft'); + store.completeSuggestions(identity, id, reply); + return id; +} + +it('applies a ready draft as the next revision once, and keeps the earlier revision', () => { + const { store, open } = fixture(), id = readyDraft(store); + expect(store.getDraft(identity, id)).toEqual({ state: 'ready', revision: 1, snapshotId: store.getSnapshot(identity).id, plan: redraft(), reason: null }); + const reopened = open(); + const applied = reopened.applyDraft(identity, id, context); + expect(applied).toEqual(redraft()); + expect(store.getPlan(identity)).toEqual(redraft()); + expect(store.getPlan(identity, 1)).toEqual(plan()); + expect(store.getDraft(identity, id).state).toBe('consumed'); + expect(() => store.applyDraft(identity, id, context)).toThrow('Draft is unavailable.'); +}); + +it('keeps drafts and suggestions apart: neither ID works on the other\'s path', () => { + const { store } = fixture(), draft = readyDraft(store); + const suggestion = store.beginSuggestions(identity, state(store)); + store.completeSuggestions(identity, suggestion, cards()); + expect(() => store.applySuggestion(identity, draft, 0, context)).toThrow('Suggestion is unavailable.'); + expect(() => store.applyDraft(identity, suggestion, context)).toThrow('Draft is unavailable.'); + expect(() => store.getDraft(identity, suggestion)).toThrow('Unknown draft request.'); + // The shared read names the mode and never decodes a draft as cards. + expect(store.getSuggestions(identity, draft)).toMatchObject({ mode: 'draft', state: 'ready', reply: null }); + expect(store.getSuggestions(identity, suggestion)).toMatchObject({ mode: 'suggest', reply: cards() }); +}); + +it.each([ + ['cards for a draft', cards(), /must have required property 'issue'/, 'draft'], + ['a draft of the current revision', redraft(1), /Draft request is stale/, 'draft'], + ['a draft for another issue', { ...redraft(), issue: 2 }, /Draft request is stale/, 'draft'], + ['a plan for a suggestion', redraft(), /edit-schema|must have required property/, 'suggest'], +] as const)('refuses %s at publication, leaving the request pending', (_, reply, message, mode) => { + const { store } = fixture(), id = store.beginSuggestions(identity, state(store), mode); + expect(() => store.completeSuggestions(identity, id, reply)).toThrow(message); + expect(store.getSuggestions(identity, id).state).toBe('pending'); +}); + +it('refuses a draft made stale by a new revision or snapshot', () => { + const { store } = fixture(), byRevision = readyDraft(store); + store.importRevision(JSON.stringify(plan(1, 'Imported')), 'json', context, 1); + expect(store.getDraft(identity, byRevision)).toMatchObject({ state: 'invalidated', reason: 'Plan revision changed.' }); + expect(() => store.applyDraft(identity, byRevision, context)).toThrow('Draft is unavailable.'); + const bySnapshot = readyDraft(store, redraft(3)); + store.recordHistory(identity, state(store), oid(3), oid(4), []); + expect(() => store.applyDraft(identity, bySnapshot, context)).toThrow('Draft is unavailable.'); + expect(store.getPlan(identity).revision).toBe(2); +}); + +it('checks a draft like an import when it is applied', () => { + const { store } = fixture(); + // Schema-valid, so it is published, but it names a file the base does not have. + const id = readyDraft(store, { ...redraft(), items: [{ ...redraft().items[0]!, files: [{ path: 'missing', kind: 'edit', renamed_from: null, change: 'x' }] }] }); + expect(() => store.applyDraft(identity, id, context)).toThrow(); + expect(store.getPlan(identity).revision).toBe(1); + expect(store.getDraft(identity, id).state).toBe('ready'); +}); + +it.each([9, 10])('migrates a v%i database to v11, reading its existing requests as suggestions', version => { + const { store, path, open } = fixture(), id = store.beginSuggestions(identity, state(store)); + store.close(); stores.splice(stores.indexOf(store), 1); + const legacy = new DatabaseSync(path); + legacy.exec(`ALTER TABLE requests DROP COLUMN mode; PRAGMA user_version=${version};`); + legacy.close(); + 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(); } +}); + +/** E3 in draft mode, with a provider that answers from `source`. */ +function coordinate(store: Store, source: (request: AuthorRequest) => string) { + const requests: AuthorRequest[] = []; + const coordinator = new SuggestionCoordinator(store, { async invoke(request) { requests.push(request); return source(request); } }); + const input: SuggestionInput = { context, revision: 1, snapshotId: store.getSnapshot(identity).id, feedback: 'Split the work.', + repo: { name: 'repo', baseRef: 'main', baseSha: oid(1), paths: ['a', 'b'] }, + issue: { number: 1, title: 'Issue', body: 'Body', comments: [] }, approvedLessons: [] }; + return { coordinator, input, requests }; +} + +it('drafts the next revision through E3, bound to the current one', async () => { + const { store } = fixture(), { coordinator, input, requests } = coordinate(store, () => JSON.stringify(redraft())); + try { + const handle = coordinator.start(input, 'draft'); + expect(await handle.result).toMatchObject({ state: 'completed', id: handle.id }); + expect(requests[0]).toMatchObject({ mode: 'draft', phase: 'planning', access: 'read-only', revision: 2, requestId: handle.id }); + expect(store.getDraft(identity, handle.id)).toMatchObject({ state: 'ready', revision: 1, plan: redraft() }); + expect(store.applyDraft(identity, handle.id, context).revision).toBe(2); + } finally { await coordinator.close(); } +}); + +it('records a draft that is not the next revision as failed, without a new revision', async () => { + const { store } = fixture(), { coordinator, input } = coordinate(store, () => JSON.stringify(redraft(5))); + try { + const handle = coordinator.start(input, 'draft'); + expect(await handle.result).toMatchObject({ state: 'failed' }); + expect(store.getDraft(identity, handle.id)).toMatchObject({ state: 'failed', plan: null }); + expect(store.getPlan(identity).revision).toBe(1); + } finally { await coordinator.close(); } +}); + +it('marks a draft stale when the plan changes while Claude writes it', async () => { + const { store } = fixture(); + const { coordinator, input } = coordinate(store, () => { + store.importRevision(JSON.stringify(plan(1, 'Imported meanwhile')), 'json', context, 1); + return JSON.stringify(redraft()); + }); + try { + const handle = coordinator.start(input, 'draft'); + expect(await handle.result).toMatchObject({ state: 'stale' }); + expect(store.getDraft(identity, handle.id).state).toBe('invalidated'); + expect(store.getPlan(identity).summary).toBe('Imported meanwhile'); + } finally { await coordinator.close(); } +}); + +it('runs one planning invocation per plan, shared by drafts and suggestions', async () => { + const { store } = fixture(); + let release!: () => void; + const held = new Promise(done => { release = done; }); + const { coordinator, input } = coordinate(store, () => JSON.stringify(redraft())); + const slow = new SuggestionCoordinator(store, { async invoke() { await held; return JSON.stringify(redraft()); } }); + try { + const draft = slow.start(input, 'draft'); + expect(() => slow.start(input)).toThrow('A suggestion invocation is still active for this plan.'); + release(); + await draft.result; + } finally { await slow.close(); await coordinator.close(); } +}); diff --git a/test/publish.test.ts b/test/publish.test.ts index 9e32ea54..372a6912 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: 9 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); 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-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index 7e2cf2d3..3ed3ef2d 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -662,7 +662,7 @@ 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: 9 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); }); }); @@ -686,6 +686,6 @@ 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: 9 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); }); }); diff --git a/test/store.test.ts b/test/store.test.ts index d949e2e7..e7e0e76b 100644 --- a/test/store.test.ts +++ b/test/store.test.ts @@ -105,10 +105,11 @@ it('migrates unbound active requests to terminal history instead of reviving the const { store, path } = fixture(); const id = ready(store); close(store); const legacy = new DatabaseSync(path); - legacy.exec('DROP TABLE merge_attempts; ALTER TABLE requests DROP COLUMN reason; ALTER TABLE requests DROP COLUMN snapshot_id; PRAGMA user_version=3;'); + legacy.exec('DROP TABLE merge_attempts; ALTER TABLE requests DROP COLUMN reason; ALTER TABLE requests DROP COLUMN snapshot_id; ALTER TABLE requests DROP COLUMN mode; PRAGMA user_version=3;'); legacy.close(); const recovered = open(path); - expect(recovered.getSuggestions(identity, id)).toEqual({ state: 'invalidated', revision: 1, snapshotId: null, reply: reply(), reason: 'Request predates snapshot binding.' }); + // Requests from before #124 carry no mode: every one of them is a suggestion. + expect(recovered.getSuggestions(identity, id)).toEqual({ mode: 'suggest', state: 'invalidated', revision: 1, snapshotId: null, reply: reply(), reason: 'Request predates snapshot binding.' }); expect(() => recovered.applySuggestion(identity, id, 0, context)).toThrow(/unavailable/); }); it('invalidates pending and ready requests when another connection advances the snapshot', () => { diff --git a/web/server.ts b/web/server.ts index 41c39c4a..b030b3c1 100644 --- a/web/server.ts +++ b/web/server.ts @@ -14,7 +14,7 @@ import { BadRequest, GuardRefusal, ShuttingDownError, assertUuidV4, isUuidV4, sa import { GhIssueGateway, type IssueGateway } from '../github/issues.ts'; import { demoIssueGateway } from '../scripts/demo-issues.ts'; import { IssueBoard } from './issues.ts'; -import { SuggestionCoordinator, type SuggestionHandle, type SuggestionInput, type SuggestionStore } from '../core/planning-suggestions.ts'; +import { SuggestionCoordinator, type PlanningMode, type SuggestionHandle, type SuggestionInput, type SuggestionStore } from '../core/planning-suggestions.ts'; import type { AuthorProvider } from '../core/planning-author.ts'; import { PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; export type PlanningDescription = Pick & { repo: { name: string; baseRef: string } }; @@ -29,6 +29,8 @@ export interface PlanningDeps { /** Releases what the provider owns at shutdown, after every suggestion has settled. */ close?(): Promise; } +/** Start, cancel or apply a suggestion or a draft (#124): `/api/plan/` or `/api/plan///`. */ +const PLANNING_REQUEST = /^\/api\/plan\/(suggestions|drafts)(?:\/([0-9a-f-]{36})\/(cancel|apply))?$/; /** A dependency the server reads from (GitHub) failed: 502, not recorded. */ class UpstreamFailure extends Error {} /** How long a planning request may take to settle after shutdown aborts it, before its worker is abandoned (Ask's grace). */ @@ -69,7 +71,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const store = service.store; const suggestionStore: SuggestionStore = { getPlan: identity => store.getPlan(identity), getSnapshot: identity => store.getSnapshot(identity), - beginSuggestions: (identity, expected) => store.beginSuggestions(identity, expected), + beginSuggestions: (identity, expected, mode) => store.beginSuggestions(identity, expected, mode), completeSuggestions: (identity, id, reply) => capability.run(() => store.completeSuggestions(identity, id, reply)), settleSuggestion: (identity, id, expected, outcome) => capability.run(() => store.settleSuggestion(identity, id, expected, outcome)), getSuggestions: (identity, id) => store.getSuggestions(identity, id), @@ -200,9 +202,11 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const requireAction = (input: Record) => { if (input.actionId === undefined) throw new BadRequest('actionId is required for this action.'); return input.actionId as string; }; const planningAction = async (path: string, input: Record, signal: AbortSignal) => { const actionId = requireAction(input), { actionId: _omit, ...request } = input; - const imported = path === '/api/plan/import', started = path === '/api/plan/suggestions'; - const match = /^\/api\/plan\/suggestions\/([0-9a-f-]{36})\/(cancel|apply)$/.exec(path); - const kind = imported ? 'plan-import' : started ? 'suggestion-start' : `suggestion-${match![2]}`; + // Suggestions are edit cards for the current plan; a draft is a whole next revision (#124). They share one lifecycle. + const imported = path === '/api/plan/import', route = PLANNING_REQUEST.exec(path); + const mode: PlanningMode = route?.[1] === 'drafts' ? 'draft' : 'suggest', noun = mode === 'draft' ? 'draft' : 'suggestion'; + const started = !imported && !route![2]; + const kind = imported ? 'plan-import' : started ? `${noun}-start` : `${noun}-${route![3]}`; // The issue comes from GitHub (#117), so it is read before the recorded action, which runs synchronously. A replay // returns its saved outcome without reading GitHub again, and a start the recorded action would refuse anyway (no // planning, shutdown, stale revision or snapshot) does not read it at all. @@ -229,26 +233,29 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (stopping) throw new ShuttingDownError(); if (!suggestions || !planning) throw new GuardRefusal('Planning agent not available yet.'); const plan = service.store.getPlan(identity), snapshot = service.store.getSnapshot(identity); - if (input.expectedRevision !== plan.revision || input.snapshotId !== snapshot.id) throw new GuardRefusal('Stale plan revision or snapshot. Reload before asking for suggestions.'); + if (input.expectedRevision !== plan.revision || input.snapshotId !== snapshot.id) throw new GuardRefusal(`Stale plan revision or snapshot. Reload before asking for ${mode === 'draft' ? 'a draft' : 'suggestions'}.`); if (typeof input.feedback !== 'string' || input.feedback.length > 4000) throw new BadRequest('feedback must be text of 4000 characters or fewer.'); // Read above whenever the checks before this line pass; they cannot change across the synchronous action. if (!described) throw new GuardRefusal('Planning agent not available yet.'); const context = service.planContext(); const handle = suggestions.start({ context, revision: plan.revision, snapshotId: snapshot.id, issue: described.issue, approvedLessons: described.approvedLessons, feedback: input.feedback, - repo: { ...described.repo, baseSha: snapshot.base, paths: context.baseEntries.map(entry => entry.path) } }); + repo: { ...described.repo, baseSha: snapshot.base, paths: context.baseEntries.map(entry => entry.path) } }, mode); suggestionHandles.set(handle.id, handle); void handle.result.finally(() => suggestionHandles.delete(handle.id)); return { requestId: handle.id }; } - const id = match![1]!; - if (match![2] === 'cancel') { - const handle = suggestionHandles.get(id), current = service.store.getSuggestions(identity, id); + const id = route![2]!, current = service.store.getSuggestions(identity, id); + // An ID is used only on its own kind's routes. + if (current.mode !== mode) throw new Error(`Unknown ${noun} request.`); + if (route![3] === 'cancel') { + const handle = suggestionHandles.get(id); if (current.state === 'pending' && handle) { handle.cancel('Cancelled by the user.'); return { state: 'cancelling' }; } // No handle in this process: the request belonged to a process that ended, and its container's output had no // route back to the Store (planning runs only through D, in that process's worker; runner-lifecycle.md). service.store.cancelSuggestions(identity, id, 'Cancelled by the user.'); return { state: service.store.getSuggestions(identity, id).state }; } + if (mode === 'draft') return { revision: service.store.applyDraft(identity, id, service.planContext()).revision }; if (!Number.isSafeInteger(input.index)) throw new BadRequest('index is required.'); return { revision: service.store.applySuggestion(identity, id, input.index as number, service.planContext()).revision }; }).response; @@ -275,9 +282,15 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (req.method === 'GET' && path === '/api/issues') { json(200, issues.view()); return; } if (req.method === 'GET' && path === '/api/review') { json(200, await load(requestAbort.signal)); return; } if (req.method === 'GET' && path === '/api/runner') { json(200, runnerView()); return; } - const suggestionRead = /^\/api\/plan\/suggestions\/([0-9a-f-]{36})$/.exec(path); - if (req.method === 'GET' && suggestionRead) { json(200, service.store.getSuggestions(identity, suggestionRead[1]!)); return; } - const planningPath = path === '/api/plan/import' || path === '/api/plan/suggestions' || /^\/api\/plan\/suggestions\/[0-9a-f-]{36}\/(cancel|apply)$/.test(path); + const planningRead = /^\/api\/plan\/(suggestions|drafts)\/([0-9a-f-]{36})$/.exec(path); + if (req.method === 'GET' && planningRead) { + const id = planningRead[2]!; + if (planningRead[1] === 'drafts') { json(200, service.store.getDraft(identity, id)); return; } + const request = service.store.getSuggestions(identity, id); + if (request.mode !== 'suggest') throw new Error('Unknown suggestion request.'); + json(200, request); return; + } + const planningPath = path === '/api/plan/import' || PLANNING_REQUEST.test(path); if (req.method !== 'POST' || !(['/api/action','/api/settings','/api/issues','/api/runner'].includes(path) || planningPath) || req.headers['content-type'] !== 'application/json') { json(405, { error: 'Unsupported request.' }); return; } const chunks: Buffer[] = []; let size = 0; activeRequest.readingBody=true; From 94791f1bf456c88042358cfd02ad7776fb6ee523 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 3 Oct 2026 22:10:17 -0700 Subject: [PATCH 2/8] F: address independent review on plan drafts (#124) - beginSuggestions requires its mode, so an adapter that drops it fails to typecheck; one PlanningMode type, from core. - A recorded cancel or apply includes the request ID from the path, so one action ID cannot replay across two suggestions or drafts. - An unknown ID on a draft route is "Unknown draft request.". - Docs: drafts in the user-action and idempotency-only lists; the suggestion GET returns mode and refuses drafts; a GitHub 502 is not recorded. - Tests: apply refused during a merge, on a closed task and under another identity or issue; dismissing a ready draft; mode column already present below v11; one invocation per plan in both orders; feedback reaches the draft prompt; action ID reuse across kinds and across two drafts. Co-Authored-By: Claude Opus 5.5 --- core/planning-suggestions.ts | 2 +- docs/implementation/runner-lifecycle.md | 6 +-- runner/store.ts | 7 +-- test/planning-acceptance.test.ts | 2 +- test/planning-drafts-api.test.ts | 21 ++++++++ test/planning-drafts.test.ts | 67 +++++++++++++++++++++---- test/store.test.ts | 16 +++--- web/server.ts | 10 ++-- 8 files changed, 102 insertions(+), 29 deletions(-) diff --git a/core/planning-suggestions.ts b/core/planning-suggestions.ts index 67ea7e70..9129f6a6 100644 --- a/core/planning-suggestions.ts +++ b/core/planning-suggestions.ts @@ -6,7 +6,7 @@ import type { Diagnostic, EditReply, Plan } from './plan.ts'; export interface SuggestionStore { getPlan(identity: PlanIdentity): Plan; getSnapshot(identity: PlanIdentity): { id: string; base: string; head: string }; - beginSuggestions(identity: PlanIdentity, expected: { revision: number; snapshotId: string }, mode?: PlanningMode): string; + beginSuggestions(identity: PlanIdentity, expected: { revision: number; snapshotId: string }, mode: PlanningMode): string; completeSuggestions(identity: PlanIdentity, id: string, reply: unknown): void; settleSuggestion(identity: PlanIdentity, id: string, expected: { revision: number; snapshotId: string }, outcome: { state: 'failed' | 'cancelled' | 'invalidated'; reason: string }): boolean; getSuggestions(identity: PlanIdentity, id: string): { state: string; revision: number; snapshotId: string | null; reply: EditReply | null; reason: string | null }; diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index f96d7b1e..d825bbcb 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -353,7 +353,7 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina |---|---|---| | `POST /api/plan/import` | `Store.importRevision` | expected revision | | `POST /api/plan/suggestions` | E3 coordinator start (`beginSuggestions`) | expected revision and snapshot | -| `GET /api/plan/suggestions/:id` | `Store.getSuggestions` | reads only the request row | +| `GET /api/plan/suggestions/:id` | `Store.getSuggestions`, with the request's `mode` | reads only the request row; refuses a draft's ID ("Unknown suggestion request.") | | `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle (for example after a restart), F calls `Store.cancelSuggestions`. It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | | `POST /api/plan/suggestions/:id/apply` | `Store.applySuggestion` | request `ready` and bound to the current revision and snapshot | | `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan | expected revision and snapshot; one active planning request per plan, shared with suggestions | @@ -367,7 +367,7 @@ Live planning runs for a non-demo review with a `github` block (#117): `web/plan **What becomes an event.** Only feedback the user wrote or chose. Issue text, issue comments and agent output never become events. -`actionId` identifies the one user action (or confirmed external outcome) that caused the event. **For user actions it is an idempotency key the UI generates** (a UUID) when the user acts. The server validates it before opening any transaction: it must be a lowercase UUID v4 (the same regular expression as attempt IDs, 36 characters), or the request is refused with HTTP 400 and nothing is stored. The UI generates it and resends unchanged if it retries after a lost response. Every writing user action goes through one `user_actions` table, keyed by `(planKey, actionId)`. That covers notes, choice changes, finding acceptance, rejection, cancel, retry, "run again", start and resume, **starting a merge**, and every planning endpoint that writes: plan import, suggestion start, suggestion cancel and suggestion apply. Each planning request carries an `actionId`; its `user_actions` row is inserted in the same transaction as the `Store` call it wraps, and the saved response includes the request ID or plan revision it produced. For a merge, the `user_actions` row is inserted in the same transaction as `beginMergeAttempt`, and it saves the merge attempt ID with the response "in progress". When the merge coordinator records the outcome, the same transaction updates that saved response. A replayed merge click therefore returns the same attempt's current outcome and never starts a second attempt. The row holds the action kind, a hash of the request body and the bounded response. **The first definite outcome is recorded, including a refusal.** If the action applies, its row is inserted in the same transaction. If a guard refuses it (for example HTTP 409 for a stale state version), the refusal and its response are recorded in their own transaction. Only outcomes that applied nothing for a passing reason are not recorded, so the UI may resend them: HTTP 503 during shutdown, and a storage error. A replay is handled before any other guard: +`actionId` identifies the one user action (or confirmed external outcome) that caused the event. **For user actions it is an idempotency key the UI generates** (a UUID) when the user acts. The server validates it before opening any transaction: it must be a lowercase UUID v4 (the same regular expression as attempt IDs, 36 characters), or the request is refused with HTTP 400 and nothing is stored. The UI generates it and resends unchanged if it retries after a lost response. Every writing user action goes through one `user_actions` table, keyed by `(planKey, actionId)`. That covers notes, choice changes, finding acceptance, rejection, cancel, retry, "run again", start and resume, **starting a merge**, and every planning endpoint that writes: plan import, suggestion start, suggestion cancel, suggestion apply, draft start, draft cancel and draft apply (#124). Each planning request carries an `actionId`; its `user_actions` row is inserted in the same transaction as the `Store` call it wraps, and the saved response includes the request ID or plan revision it produced. For a merge, the `user_actions` row is inserted in the same transaction as `beginMergeAttempt`, and it saves the merge attempt ID with the response "in progress". When the merge coordinator records the outcome, the same transaction updates that saved response. A replayed merge click therefore returns the same attempt's current outcome and never starts a second attempt. The row holds the action kind, a hash of the request body and the bounded response. **The first definite outcome is recorded, including a refusal.** If the action applies, its row is inserted in the same transaction. If a guard refuses it (for example HTTP 409 for a stale state version), the refusal and its response are recorded in their own transaction. Only outcomes that applied nothing for a passing reason are not recorded, so the UI may resend them: HTTP 503 during shutdown, HTTP 502 when the issue cannot be read from GitHub before a suggestion or draft starts (#117), and a storage error. A replay is handled before any other guard: - the same `actionId` with the same request hash returns the saved response and applies nothing, even though the state version has since moved on; - the same `actionId` with a different request hash (including a different action kind) is refused with HTTP 409 "Action ID already used" and applies nothing. @@ -387,7 +387,7 @@ So neither an action nor its events can be duplicated, including a retry that wo | Cancel task | `task-closed` (at close, which may come after the attempt settles) | | Confirmed merge (external outcome, not a user action) | `task-closed` | -Every other writing action is **idempotency-only**: it has a `user_actions` row and produces no event. That covers cancel attempt, retry, "run again", settings changes, questions, merge initiation (its event waits for the confirmed outcome), plan import, and suggestion start, cancel and apply. +Every other writing action is **idempotency-only**: it has a `user_actions` row and produces no event. That covers cancel attempt, retry, "run again", settings changes, questions, merge initiation (its event waits for the confirmed outcome), plan import, suggestion start, cancel and apply, and draft start, cancel and apply (#124). For a cancel or apply, the recorded request includes the ID of the suggestion or draft it acts on, so one `actionId` cannot replay across two of them. | Event kind | Source action | `sourceRef` | |---|---|---| diff --git a/runner/store.ts b/runner/store.ts index a90f881e..523593a2 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -3,6 +3,7 @@ import { createRequire } from 'node:module'; import { randomUUID } from 'node:crypto'; import { identityKey, type PlanIdentity } from '../core/identity.ts'; import { importPlan, applySuggestion, assertEditReply, assertPlan, type Plan, type PlanContext, type EditReply } from '../core/plan.ts'; +import type { PlanningMode } from '../core/planning-suggestions.ts'; import type { Approval, SegmentChoice } from '../core/approvals.ts'; import type { InvocationContext, StopReason } from '../agents/contract.ts'; import type { AlreadyFixedResult } from '../github/already-fixed.ts'; @@ -20,8 +21,7 @@ export function requireSupportedNode(version = process.versions.node): void { export interface Snapshot { id: string; base: string; head: string } export interface ReviewState { revision: number; snapshotId: string; reviewVersion?: number } export type SuggestionState = 'pending' | 'ready' | 'failed' | 'cancelled' | 'invalidated' | 'consumed'; -/** A planning request: card suggestions for the current plan, or a whole draft of its next revision (#124). */ -export type PlanningMode = 'suggest' | 'draft'; +export type { PlanningMode } from '../core/planning-suggestions.ts'; /** `reply` is the suggestion cards; for a draft it is always null here (read it with `getDraft`). */ export interface SuggestionRequest { mode: PlanningMode; state: SuggestionState; revision: number; snapshotId: string | null; reply: EditReply | null; reason: string | null } /** A draft request: `revision` is the plan revision it was drafted against; `plan` would become revision + 1. */ @@ -307,7 +307,8 @@ export class Store { if (!row) throw new Error('Unknown snapshot.'); return decode(row.data); } - beginSuggestions(identity: PlanIdentity, expected: ReviewState, mode: PlanningMode = 'suggest'): string { + /** `mode` is required, so a caller cannot record a draft as a suggestion by leaving it out (#124). */ + beginSuggestions(identity: PlanIdentity, expected: ReviewState, mode: PlanningMode): string { if (mode !== 'suggest' && mode !== 'draft') throw new Error('Invalid planning request mode.'); const key = identityKey(identity); return this.#transaction(() => { diff --git a/test/planning-acceptance.test.ts b/test/planning-acceptance.test.ts index 041ee4ab..cf992ef8 100644 --- a/test/planning-acceptance.test.ts +++ b/test/planning-acceptance.test.ts @@ -44,7 +44,7 @@ it.each(['json', 'yaml'] as const)('imports %s as the next revision and invalida }); it('rejects an import for another selected issue without changing the plan or pending request', async () => { const f = fixture(), before = f.store.getPlan(f.context.identity); - const id = f.store.beginSuggestions(f.context.identity, { revision: 1, snapshotId: f.input.snapshotId }); + const id = f.store.beginSuggestions(f.context.identity, { revision: 1, snapshotId: f.input.snapshotId }, 'suggest'); expect(() => f.store.importRevision(JSON.stringify({ ...plan(), issue: 413 }), 'json', f.context, 1)).toThrow(/issue/); expect(f.store.getPlan(f.context.identity)).toEqual(before); expect(f.store.getSuggestions(f.context.identity, id)).toMatchObject({ state: 'pending', revision: 1, reply: null }); diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 0bd05616..24aba5ee 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -91,3 +91,24 @@ it('cancels a draft while Claude is still writing it', async () => { expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, { actionId: randomUUID() })).toEqual({ status: 200, body: { result: { state: 'cancelling' } } }); expect(await served.settled('drafts', id)).toMatchObject({ state: 'cancelled', plan: null, reason: 'Cancelled by the user.' }); }); + +it('refuses an action ID reused across kinds, or across two drafts', async () => { + const served = await serve(redraft); + const actionId = randomUUID(), body = { expectedRevision: served.view.plan.revision, snapshotId: served.view.snapshot.id, feedback: '', actionId }; + const first = (await served.api('POST', '/api/plan/drafts', body)).body.result.requestId as string; + expect(await served.api('POST', '/api/plan/suggestions', body)).toMatchObject({ status: 409, body: { error: 'Action ID already used for a different request.' } }); + await served.settled('drafts', first); + // A second ready draft against the same revision; one apply action ID must not act on both. + const second = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', second); + const apply = { actionId: randomUUID() }; + expect(await served.api('POST', `/api/plan/drafts/${first}/cancel`, apply)).toMatchObject({ status: 200 }); + expect(await served.api('POST', `/api/plan/drafts/${second}/cancel`, apply)).toMatchObject({ status: 409, body: { error: 'Action ID already used for a different request.' } }); + expect((await served.api('GET', `/api/plan/drafts/${second}`)).body.state).toBe('ready'); +}); + +it('names an unknown ID by the route\'s kind', async () => { + const served = await serve(redraft), unknown = randomUUID(); + expect(await served.api('POST', `/api/plan/drafts/${unknown}/apply`, { actionId: randomUUID() })).toMatchObject({ status: 409, body: { error: 'Unknown draft request.' } }); + expect(await served.api('POST', `/api/plan/suggestions/${unknown}/cancel`, { actionId: randomUUID() })).toMatchObject({ status: 409, body: { error: 'Unknown suggestion request.' } }); +}); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index 2b797042..d05eac01 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -51,7 +52,7 @@ it('applies a ready draft as the next revision once, and keeps the earlier revis it('keeps drafts and suggestions apart: neither ID works on the other\'s path', () => { const { store } = fixture(), draft = readyDraft(store); - const suggestion = store.beginSuggestions(identity, state(store)); + const suggestion = store.beginSuggestions(identity, state(store), 'suggest'); store.completeSuggestions(identity, suggestion, cards()); expect(() => store.applySuggestion(identity, draft, 0, context)).toThrow('Suggestion is unavailable.'); expect(() => store.applyDraft(identity, suggestion, context)).toThrow('Draft is unavailable.'); @@ -87,13 +88,49 @@ it('checks a draft like an import when it is applied', () => { const { store } = fixture(); // Schema-valid, so it is published, but it names a file the base does not have. const id = readyDraft(store, { ...redraft(), items: [{ ...redraft().items[0]!, files: [{ path: 'missing', kind: 'edit', renamed_from: null, change: 'x' }] }] }); - expect(() => store.applyDraft(identity, id, context)).toThrow(); + expect(() => store.applyDraft(identity, id, context)).toThrow(/missing/); expect(store.getPlan(identity).revision).toBe(1); expect(store.getDraft(identity, id).state).toBe('ready'); }); +it.each([ + ['while a merge is in progress', (store: Store) => { store.beginMergeAttempt(identity, { ...state(store), reviewVersion: store.reviewVersion(identity) }, oid(2), null, 'direct'); }, + /merge is in progress/], + ['once the task has closed', (store: Store) => { store.cancelTask(identity, store.getTask(identity).stateVersion, randomUUID()); }, + /closed task never changes/], +] as const)('refuses to apply a ready draft %s', (_, block, message) => { + const { store } = fixture(), id = readyDraft(store); + block(store); + expect(() => store.applyDraft(identity, id, context)).toThrow(message); + expect(store.getPlan(identity).revision).toBe(1); +}); + +it('refuses to apply a draft under another plan\'s identity or issue', () => { + const { store } = fixture(), id = readyDraft(store), other = { ...identity, planId: 'other' }; + store.createPlan(JSON.stringify(plan()), 'json', { ...context, identity: other }, oid(1), oid(2)); + expect(() => store.applyDraft(other, id, { ...context, identity: other })).toThrow('Draft is unavailable.'); + expect(() => store.applyDraft(identity, id, { ...context, issue: 2 })).toThrow(); + expect(store.getDraft(identity, id).state).toBe('ready'); +}); + +it('dismisses a ready draft', () => { + const { store } = fixture(), id = readyDraft(store); + store.cancelSuggestions(identity, id, 'Dismissed.'); + expect(store.getDraft(identity, id)).toMatchObject({ state: 'cancelled', reason: 'Dismissed.' }); + expect(() => store.applyDraft(identity, id, context)).toThrow('Draft is unavailable.'); +}); + +it('migrates a database that already has the mode column below v11', () => { + const { store, path, open } = fixture(), id = readyDraft(store); + store.close(); stores.splice(stores.indexOf(store), 1); + const legacy = new DatabaseSync(path); + legacy.exec('PRAGMA user_version=9;'); + legacy.close(); + 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 => { - const { store, path, open } = fixture(), id = store.beginSuggestions(identity, state(store)); + 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); legacy.exec(`ALTER TABLE requests DROP COLUMN mode; PRAGMA user_version=${version};`); @@ -149,16 +186,26 @@ it('marks a draft stale when the plan changes while Claude writes it', async () } finally { await coordinator.close(); } }); -it('runs one planning invocation per plan, shared by drafts and suggestions', async () => { +it.each([['draft', 'suggest'], ['suggest', 'draft']] as const)('runs one planning invocation per plan: a %s blocks a %s', async (first, second) => { const { store } = fixture(); let release!: () => void; const held = new Promise(done => { release = done; }); - const { coordinator, input } = coordinate(store, () => JSON.stringify(redraft())); - const slow = new SuggestionCoordinator(store, { async invoke() { await held; return JSON.stringify(redraft()); } }); + const { input } = coordinate(store, () => ''); + const slow = new SuggestionCoordinator(store, { async invoke(request) { + await held; return JSON.stringify(request.mode === 'draft' ? redraft() : cards()); } }); try { - const draft = slow.start(input, 'draft'); - expect(() => slow.start(input)).toThrow('A suggestion invocation is still active for this plan.'); + const running = slow.start(input, first); + expect(() => slow.start(input, second)).toThrow('A suggestion invocation is still active for this plan.'); release(); - await draft.result; - } finally { await slow.close(); await coordinator.close(); } + expect(await running.result).toMatchObject({ state: 'completed' }); + } finally { await slow.close(); } +}); + +it('sends the user\'s feedback and the current plan to Claude in a draft request', async () => { + const { store } = fixture(), { coordinator, input, requests } = coordinate(store, () => JSON.stringify(redraft())); + try { + await coordinator.start(input, 'draft').result; + expect(requests[0]!.prompt).toContain('Split the work.'); + expect(requests[0]!.prompt).toContain('"summary":"Example"'); + } finally { await coordinator.close(); } }); diff --git a/test/store.test.ts b/test/store.test.ts index e7e0e76b..777d1021 100644 --- a/test/store.test.ts +++ b/test/store.test.ts @@ -23,7 +23,7 @@ function open(path: string) { const store = new Store(path); stores.push(store); function close(store: Store) { store.close(); stores.splice(stores.indexOf(store), 1); } function fixture() { const path = join(directory(), 'state.sqlite'); const store = open(path); store.createPlan(JSON.stringify(plan()), 'json', context, oid(1), oid(2)); return { path, store }; } const state = (store: Store) => ({ revision: store.getPlan(identity).revision, snapshotId: store.getSnapshot(identity).id }); -function ready(store: Store) { const id = store.beginSuggestions(identity, state(store)); store.completeSuggestions(identity, id, reply()); return id; } +function ready(store: Store) { const id = store.beginSuggestions(identity, state(store), 'suggest'); store.completeSuggestions(identity, id, reply()); return id; } it('allocates revisions in SQLite, survives reopen, and keeps old revisions and snapshots immutable', () => { const { store, path } = fixture(); const first = store.getSnapshot(identity); expect(store.getPlan(identity).revision).toBe(1); @@ -39,10 +39,10 @@ it('binds suggestion requests before the reply and rejects cross-plan, cancelled const { store } = fixture(); const id = ready(store), sibling = ready(store); const other = { ...identity, planId: 'other' }; store.createPlan(JSON.stringify(plan()), 'json', { ...context, identity: other }, oid(1), oid(2)); expect(() => store.applySuggestion(other, id, 0, { ...context, identity: other })).toThrow(/unavailable/); - const cancelled = store.beginSuggestions(identity, state(store)); store.cancelSuggestions(identity, cancelled); + const cancelled = store.beginSuggestions(identity, state(store), 'suggest'); store.cancelSuggestions(identity, cancelled); expect(store.getSuggestions(identity, cancelled)).toMatchObject({ state: 'cancelled', reason: 'Suggestion cancelled.' }); expect(() => store.completeSuggestions(identity, cancelled, reply())).toThrow(/stale|cancelled/); - const delayed = store.beginSuggestions(identity, state(store)); + const delayed = store.beginSuggestions(identity, state(store), 'suggest'); expect(store.applySuggestion(identity, id, 0, context).revision).toBe(2); expect(() => store.applySuggestion(identity, id, 0, context)).toThrow(/unavailable/); expect(() => store.applySuggestion(identity, sibling, 0, context)).toThrow(/unavailable/); @@ -51,7 +51,7 @@ it('binds suggestion requests before the reply and rejects cross-plan, cancelled }); it('preserves a completed request when stale pending cleanup loses the race', () => { const { store, path } = fixture(); const other = open(path), expected = state(store); - const id = store.beginSuggestions(identity, expected); + const id = store.beginSuggestions(identity, expected, 'suggest'); other.completeSuggestions(identity, id, reply()); expect(store.settleSuggestion(identity, id, expected, { state: 'failed', reason: 'Provider failed.' })).toBe(false); expect(store.getSuggestions(identity, id)).toMatchObject({ state: 'ready', snapshotId: expected.snapshotId, reason: null, reply: reply() }); @@ -59,7 +59,7 @@ it('preserves a completed request when stale pending cleanup loses the race', () }); it('persists the winning terminal reason and prevents a late completion from reviving it', () => { const { store, path } = fixture(); const other = open(path), expected = state(store); - const id = store.beginSuggestions(identity, expected); + const id = store.beginSuggestions(identity, expected, 'suggest'); expect(store.settleSuggestion(identity, id, expected, { state: 'failed', reason: 'Provider exited.' })).toBe(true); expect(() => other.completeSuggestions(identity, id, reply())).toThrow(/stale|cancelled|complete/); const recovered = open(path); @@ -68,7 +68,7 @@ it('persists the winning terminal reason and prevents a late completion from rev }); it('retains cancellation reasons across restart and never revives cancelled work', () => { const { store, path } = fixture(), expected = state(store); - const id = store.beginSuggestions(identity, expected); + const id = store.beginSuggestions(identity, expected, 'suggest'); expect(store.settleSuggestion(identity, id, expected, { state: 'cancelled', reason: 'Runner shut down.' })).toBe(true); close(store); const recovered = open(path); @@ -114,7 +114,7 @@ it('migrates unbound active requests to terminal history instead of reviving the }); it('invalidates pending and ready requests when another connection advances the snapshot', () => { const { store, path } = fixture(); const other = open(path), expected = state(store); - const pending = store.beginSuggestions(identity, expected), completed = store.beginSuggestions(identity, expected); + const pending = store.beginSuggestions(identity, expected, 'suggest'), completed = store.beginSuggestions(identity, expected, 'suggest'); store.completeSuggestions(identity, completed, reply()); other.recordHistory(identity, expected, oid(1), oid(3), []); expect(store.getSuggestions(identity, pending)).toMatchObject({ state: 'invalidated', snapshotId: expected.snapshotId, reason: 'Repository snapshot changed.', reply: null }); @@ -217,7 +217,7 @@ it('recovers a committed suggestion after abrupt process exit and discards an in const store = new Store(process.argv[1]); const context = {...${JSON.stringify(context)}, pathKey: p => p}; store.createPlan(${JSON.stringify(JSON.stringify(plan()))}, 'json', context, '${oid(1)}', '${oid(2)}'); - const id = store.beginSuggestions(context.identity, {revision: 1, snapshotId: store.getSnapshot(context.identity).id}); + const id = store.beginSuggestions(context.identity, {revision: 1, snapshotId: store.getSnapshot(context.identity).id}, 'suggest'); store.completeSuggestions(context.identity, id, ${JSON.stringify(reply())}); process.stdout.write(id); process.exit(0);`; const id = execFileSync(process.execPath, ['--input-type=module', '-e', source, path], { encoding: 'utf8' }); diff --git a/web/server.ts b/web/server.ts index b030b3c1..26dd1e8a 100644 --- a/web/server.ts +++ b/web/server.ts @@ -201,9 +201,11 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const suggestionHandles = new Map(); const requireAction = (input: Record) => { if (input.actionId === undefined) throw new BadRequest('actionId is required for this action.'); return input.actionId as string; }; const planningAction = async (path: string, input: Record, signal: AbortSignal) => { - const actionId = requireAction(input), { actionId: _omit, ...request } = input; + const actionId = requireAction(input), { actionId: _omit, ...body } = input; // Suggestions are edit cards for the current plan; a draft is a whole next revision (#124). They share one lifecycle. const imported = path === '/api/plan/import', route = PLANNING_REQUEST.exec(path); + // The recorded request names the planning request the path acts on, so one action ID cannot replay across two. + const request = route?.[2] ? { ...body, requestId: route[2] } : body; const mode: PlanningMode = route?.[1] === 'drafts' ? 'draft' : 'suggest', noun = mode === 'draft' ? 'draft' : 'suggestion'; const started = !imported && !route![2]; const kind = imported ? 'plan-import' : started ? `${noun}-start` : `${noun}-${route![3]}`; @@ -244,8 +246,10 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge void handle.result.finally(() => suggestionHandles.delete(handle.id)); return { requestId: handle.id }; } - const id = route![2]!, current = service.store.getSuggestions(identity, id); - // An ID is used only on its own kind's routes. + const id = route![2]!; + // An ID is used only on its own kind's routes; an unknown one is named by the route's kind. + let current: ReturnType; + try { current = service.store.getSuggestions(identity, id); } catch { throw new Error(`Unknown ${noun} request.`); } if (current.mode !== mode) throw new Error(`Unknown ${noun} request.`); if (route![3] === 'cancel') { const handle = suggestionHandles.get(id); From f04cefaf5c93c6f1b7f7369bebc0630b5a685fc7 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:13:40 -0700 Subject: [PATCH 3/8] F: address review round 1 on plan drafts (#124) - Startup fails every planning request an earlier process left pending (Store.settleInterruptedRequests), under the single-runner lock. - A suggestion cancel or apply recorded before #124 (body-only hash) still replays its saved outcome. - "A planning invocation is still active" names both kinds. - Docs: architecture rows for drafts; cancel, one-invocation and startup wording in runner-lifecycle.md. - Tests: startup settling, sibling invalidation on apply, the context mismatch message, draft 502 and 503, one cancel action ID on two suggestions, and the legacy replay. Co-Authored-By: Claude Opus 5.5 --- core/planning-suggestions.ts | 2 +- docs/architecture.md | 4 +- docs/implementation/runner-lifecycle.md | 8 +-- runner/store.ts | 9 +++ test/planning-drafts-api.test.ts | 73 +++++++++++++++++++++++-- test/planning-drafts.test.ts | 21 ++++++- web/server.ts | 15 ++++- 7 files changed, 116 insertions(+), 16 deletions(-) diff --git a/core/planning-suggestions.ts b/core/planning-suggestions.ts index 9129f6a6..3d30807d 100644 --- a/core/planning-suggestions.ts +++ b/core/planning-suggestions.ts @@ -56,7 +56,7 @@ export class SuggestionCoordinator { start(input: SuggestionInput, mode: PlanningMode = 'suggest'): SuggestionHandle { if (this.#closing) throw new Error('Suggestion coordinator is closing.'); const identity = { ...input.context.identity }, key = identityKey(identity); - if (this.#active.has(key)) throw new Error('A suggestion invocation is still active for this plan.'); + if (this.#active.has(key)) throw new Error('A planning invocation is still active for this plan.'); const snapshotId = input.snapshotId; const previousPlan = this.#store.getPlan(identity), snapshot = this.#store.getSnapshot(identity); if (previousPlan.revision !== input.revision) throw new Error('Stale plan revision.'); diff --git a/docs/architecture.md b/docs/architecture.md index 2db2cc8c..4662bba3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -160,7 +160,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Part | Main files | Owns | State | |---|---|---|---| | Command line | `web/cli.ts` | Parses `--demo` or `--config`, takes the single-runner lock, starts the server, releases the lock on Ctrl+C. | In use | -| HTTP server | `web/server.ts` | Binds `127.0.0.1` only. Checks the token, Host and Origin. Bounded JSON bodies. Routes `/api/review`, `/api/action`, `/api/questions`, `/api/merge`, `/api/issues`, `/api/settings`, plus `/api/runner` and `/api/plan/*` when a runner or planning provider is supplied. Ordered shutdown. | In use; `/api/plan/suggestions` runs Claude through lane D for a review with a `github` block (#117) | +| HTTP server | `web/server.ts` | Binds `127.0.0.1` only. Checks the token, Host and Origin. Bounded JSON bodies. Routes `/api/review`, `/api/action`, `/api/questions`, `/api/merge`, `/api/issues`, `/api/settings`, plus `/api/runner` and `/api/plan/*` when a runner or planning provider is supplied. Ordered shutdown. | In use; `/api/plan/suggestions` and `/api/plan/drafts` run Claude through lane D for a review with a `github` block (#117, #124) | | Browser screen | `web/public/*` | Review screen and Issues screen. Plain JavaScript with IBM Plex fonts served locally. Sends item and segment IDs and a state token, never fingerprints or ownership. | Yes | | Review service | `runner/review.ts` | Builds the review view: reads history, links it to the plan, computes approval states and merge blockers. Applies review commands through the store. | Yes | | Ask | `runner/questions.ts`, `runner/question-*.ts` | Answers a question about one plan item with a read-only agent. Runs lane D's synchronous setup in a worker thread so the server stays responsive. Tracks leftover containers. | Yes | @@ -174,7 +174,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Linking engine | `core/linking.ts` | Replays each commit's line changes and assigns every final line to an owner from the ledger. Produces segments and the Unplanned and Ambiguous rows. | Yes | | Approvals | `core/approvals.ts` | Approval fingerprints, dependency staleness, assignments, and accept-as-is choices. | Yes | | Issue ranking | `core/issue-ranking.ts`, `github/issues.ts`, `web/issues.ts` | Deterministic, explainable issue score. Repository-scoped trust of issue authors. | Yes | -| Planning | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself. | Library tested; planning API not connected; no screen | +| Planning | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself. | In use through `/api/plan/*` (#117, #124); no screen yet | | Execution prompt and audit | `core/execution-prompt.ts`, `core/run-audit.ts`, `prompts/execute.md` | Builds the execute and fix prompt with untrusted text only in escaped data blocks. Audits the change manifest after a run: safety violations first (Git metadata, links that leave the repository, agent commits), then scope against the declared files. | Not connected | | Pull request text | `core/pull-request-body.ts` | Title and description with the plan. Plan and agent text go in fenced blocks, and issue references and @-mentions are neutralized, so the text cannot close other issues or notify people. | Not connected | | Git adapter | `git/history.ts`, `git/clone.ts` | Bounded, hardened history reads. Standalone task clones with no links back to your repository. Every Git command runs with an allowlisted environment. | In use | diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 647c8aac..6d2f2887 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -357,12 +357,12 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina | `GET /api/plan/suggestions/:id` | `Store.getSuggestions`, with the request's `mode` | reads only the request row; refuses a draft's ID ("Unknown suggestion request.") | | `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle (for example after a restart), F calls `Store.cancelSuggestions`. It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | | `POST /api/plan/suggestions/:id/apply` | `Store.applySuggestion` | request `ready` and bound to the current revision and snapshot | -| `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan | expected revision and snapshot; one active planning request per plan, shared with suggestions | +| `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan | expected revision and snapshot; one planning invocation per plan at a time in this process, shared with suggestions (several drafts may be ready at once) | | `GET /api/plan/drafts/:id` | `Store.getDraft` | reads only a draft request's row | -| `POST /api/plan/drafts/:id/cancel` | as suggestion cancel | request is a draft, `pending` or `ready` | +| `POST /api/plan/drafts/:id/cancel` | as suggestion cancel | request is a draft; a `pending` or `ready` one is cancelled, any other state is returned unchanged | | `POST /api/plan/drafts/:id/apply` | `Store.applyDraft`: imports the draft as revision + 1 with every import check | request is a draft, `ready` and bound to the current revision and snapshot | -Live planning runs for a non-demo review with a `github` block (#117): `web/planning.ts` reads the issue from GitHub before each start, and `runner/planning.ts` runs the request in planning's worker. E3's suggestion timer and the request share one ten-minute budget (`PLANNING_BUDGET_MS`). At shutdown E3 aborts every request; one that has not settled within 20 seconds (`PLANNING_SHUTDOWN_GRACE_MS`, as Ask) is ended by abandoning planning's worker. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is not recorded. Without a `github` block, or in a demo, the server returns "Planning agent not available yet". Drafts follow the same rules (#124). A draft and a suggestion are both rows of `requests`, told apart by `mode` (schema v11); each kind's routes refuse the other's IDs. +Live planning runs for a non-demo review with a `github` block (#117): `web/planning.ts` reads the issue from GitHub before each start, and `runner/planning.ts` runs the request in planning's worker. E3's suggestion timer and the request share one ten-minute budget (`PLANNING_BUDGET_MS`). At shutdown E3 aborts every request; one that has not settled within 20 seconds (`PLANNING_SHUTDOWN_GRACE_MS`, as Ask) is ended by abandoning planning's worker. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is not recorded. Without a `github` block, or in a demo, the server returns "Planning agent not available yet". Drafts follow the same rules (#124). A draft and a suggestion are both rows of `requests`, told apart by `mode` (schema v11); each kind's routes refuse the other's IDs. At startup, under the single-runner lock, every request an earlier process left `pending` is failed ("The server stopped before this request finished. Ask again."): its provider ran in that process's planning worker, so nothing can complete it. ## Feedback-event contract (for lane J) @@ -388,7 +388,7 @@ So neither an action nor its events can be duplicated, including a retry that wo | Cancel task | `task-closed` (at close, which may come after the attempt settles) | | Confirmed merge (external outcome, not a user action) | `task-closed` | -Every other writing action is **idempotency-only**: it has a `user_actions` row and produces no event. That covers cancel attempt, retry, "run again", settings changes, questions, merge initiation (its event waits for the confirmed outcome), plan import, suggestion start, cancel and apply, and draft start, cancel and apply (#124). For a cancel or apply, the recorded request includes the ID of the suggestion or draft it acts on, so one `actionId` cannot replay across two of them. +Every other writing action is **idempotency-only**: it has a `user_actions` row and produces no event. That covers cancel attempt, retry, "run again", settings changes, questions, merge initiation (its event waits for the confirmed outcome), plan import, suggestion start, cancel and apply, and draft start, cancel and apply (#124). For a cancel or apply, the recorded request includes the ID of the suggestion or draft it acts on, so one `actionId` cannot replay across two of them. A suggestion cancel or apply recorded before #124 hashed the body alone; its replay still returns the saved outcome. | Event kind | Source action | `sourceRef` | |---|---|---| diff --git a/runner/store.ts b/runner/store.ts index 1e392e68..4bf15b3a 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -473,6 +473,15 @@ export class Store { return { mode, state: row.state as SuggestionState, revision: row.revision as number, snapshotId: row.snapshot_id as string | null, reply: row.reply === null || mode === 'draft' ? null : decode(row.reply), reason: row.reason as string | null }; } + /** + * Fail every request still pending from an earlier process. Call once at startup, under the single-runner lock, before + * any request starts: a pending request's provider ran in that process's planning worker, so nothing can complete it. + * Returns how many were settled. + */ + settleInterruptedRequests(): number { + return this.#run("UPDATE requests SET state='failed', reason=? WHERE state='pending'", + 'The server stopped before this request finished. Ask again.').changes as number; + } /** A draft request (#124). Refuses a suggestion request's ID. */ getDraft(identity: PlanIdentity, id: string): DraftRequest { const row = this.#get("SELECT * FROM requests WHERE key=? AND id=? AND mode='draft'", identityKey(identity), id); diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 24aba5ee..18b33959 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -7,6 +7,8 @@ import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; import type { EditReply } from '../core/plan.ts'; import { createDemo } from '../scripts/demo.ts'; import { startServer, type PlanningDeps } from '../web/server.ts'; +import type { ReviewConfig } from '../runner/review.ts'; +import { Store } from '../runner/store.ts'; // The draft API (#124) on the real server and Store, with a stand-in provider. vi.setConfig({ testTimeout: 20_000 }); @@ -18,16 +20,22 @@ afterEach(async () => { type Answer = (request: AuthorRequest, signal: AbortSignal) => Promise; type View = { plan: Record; snapshot: { id: string } }; /** `answer` is made from the review as it was when the server started. */ -async function serve(answer: (view: View) => Answer) { +async function serve(answer: (view: View) => Answer, options: { describe?: PlanningDeps['describe']; + /** Runs on the review's database before the server starts, as an earlier process would have left it. */ + before?: (config: ReviewConfig) => void } = {}) { const root = mkdtempSync(join(tmpdir(), 'codeboost-drafts-api-')); roots.push(root); + const config = createDemo(join(root, 'demo')); + options.before?.(config); const requests: AuthorRequest[] = []; let answering!: Answer; const provider: AuthorProvider = { invoke: (request, signal) => { requests.push(request); return answering(request, signal); } }; let issue = 0; - const planning: PlanningDeps = { provider, describe: () => ({ issue: { number: issue, title: 'Retries', body: '', comments: [] }, - approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' } }) }; - const app = await startServer(createDemo(join(root, 'demo')), 0, async () => 'answer', undefined, 2_000, undefined, undefined, planning); - closers.push(() => app.close()); + const planning: PlanningDeps = { provider, describe: options.describe ?? (() => ({ issue: { number: issue, title: 'Retries', body: '', comments: [] }, + approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' } })) }; + const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, planning); + let closed = false; + const close = async () => { if (!closed) { closed = true; await app.close(); } }; + closers.push(close); const api = async (method: string, path: string, body?: unknown) => { const response = await fetch(`${new URL(app.url).origin}${path}`, { method, headers: { 'x-codeboost-token': app.token, ...(body ? { 'content-type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined }); @@ -41,7 +49,7 @@ async function serve(answer: (view: View) => Answer) { await vi.waitFor(async () => expect((await api('GET', `/api/plan/${kind}/${id}`)).body.state).not.toBe('pending')); return (await api('GET', `/api/plan/${kind}/${id}`)).body; }; - return { api, view, requests, start, settled }; + return { api, view, requests, start, settled, config, close }; } /** The review's plan, redrafted as the next revision with a new summary. */ const redraft = (view: View): Answer => async request => JSON.stringify({ ...view.plan, revision: request.revision, summary: 'Redrafted by Claude' }); @@ -112,3 +120,56 @@ it('names an unknown ID by the route\'s kind', async () => { expect(await served.api('POST', `/api/plan/drafts/${unknown}/apply`, { actionId: randomUUID() })).toMatchObject({ status: 409, body: { error: 'Unknown draft request.' } }); expect(await served.api('POST', `/api/plan/suggestions/${unknown}/cancel`, { actionId: randomUUID() })).toMatchObject({ status: 409, body: { error: 'Unknown suggestion request.' } }); }); + +it('reports a GitHub failure before a draft as 502, recording nothing', async () => { + const served = await serve(redraft, { describe: async () => { throw new Error('GitHub is unreachable.'); } }); + expect(await served.start('drafts')).toMatchObject({ status: 502, body: { error: 'The issue could not be read from GitHub: GitHub is unreachable.' } }); + expect(served.requests).toHaveLength(0); +}); + +it('refuses a draft with 503 when shutdown begins while the issue is read', async () => { + let resolve!: () => void; + const served = await serve(redraft, { describe: () => new Promise(done => { resolve = () => done({ issue: { number: 0, title: '', body: '', comments: [] }, approvedLessons: [], repo: { name: 'r', baseRef: 'main' } }); }) }); + const started = served.start('drafts'); + await vi.waitFor(() => expect(resolve).toBeTypeOf('function')); + const closing = served.close(); + resolve(); + expect(await started).toMatchObject({ status: 503 }); + await closing; +}); + +it('fails, at startup, a draft an earlier process left pending', async () => { + let pending = ''; + const served = await serve(redraft, { before: config => { + const store = new Store(config.database); + try { pending = store.beginSuggestions(config.identity, { revision: store.getPlan(config.identity).revision, snapshotId: store.getSnapshot(config.identity).id }, 'draft'); } + finally { store.close(); } + } }); + expect((await served.api('GET', `/api/plan/drafts/${pending}`)).body) + .toMatchObject({ state: 'failed', reason: 'The server stopped before this request finished. Ask again.' }); + // A new draft starts normally. + expect((await served.start('drafts')).status).toBe(200); +}); + +it('refuses one cancel action ID on two suggestions', async () => { + const served = await serve(view => async () => JSON.stringify(cards(view.plan.revision))); + const first = (await served.start('suggestions')).body.result.requestId as string; + await served.settled('suggestions', first); + const second = (await served.start('suggestions')).body.result.requestId as string; + await served.settled('suggestions', second); + const cancel = { actionId: randomUUID() }; + expect(await served.api('POST', `/api/plan/suggestions/${first}/cancel`, cancel)).toMatchObject({ status: 200 }); + expect(await served.api('POST', `/api/plan/suggestions/${second}/cancel`, cancel)).toMatchObject({ status: 409, body: { error: 'Action ID already used for a different request.' } }); + expect((await served.api('GET', `/api/plan/suggestions/${second}`)).body.state).toBe('ready'); +}); + +it('replays a suggestion cancel recorded before #124, whose hash had no request ID', async () => { + const served = await serve(redraft), actionId = randomUUID(), id = randomUUID(); + // What an earlier build recorded for this cancel: the body alone. + const store = new Store(served.config.database); + try { store.userAction(served.config.identity, { actionId, kind: 'suggestion-cancel', request: {} }, () => ({ state: 'cancelled' })); } + finally { store.close(); } + expect(await served.api('POST', `/api/plan/suggestions/${id}/cancel`, { actionId })).toEqual({ status: 200, body: { result: { state: 'cancelled' } } }); + // A draft route has no such history: the same action ID is a different request there. + expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, { actionId })).toMatchObject({ status: 409 }); +}); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index d05eac01..f6a931de 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -109,10 +109,27 @@ it('refuses to apply a draft under another plan\'s identity or issue', () => { const { store } = fixture(), id = readyDraft(store), other = { ...identity, planId: 'other' }; store.createPlan(JSON.stringify(plan()), 'json', { ...context, identity: other }, oid(1), oid(2)); expect(() => store.applyDraft(other, id, { ...context, identity: other })).toThrow('Draft is unavailable.'); - expect(() => store.applyDraft(identity, id, { ...context, issue: 2 })).toThrow(); + expect(() => store.applyDraft(identity, id, { ...context, issue: 2 })).toThrow('Plan context identity/issue mismatch.'); expect(store.getDraft(identity, id).state).toBe('ready'); }); +it('invalidates every other ready draft and suggestion when a draft is applied', () => { + const { store } = fixture(), applied = readyDraft(store), sibling = readyDraft(store); + const suggestion = store.beginSuggestions(identity, state(store), 'suggest'); + store.completeSuggestions(identity, suggestion, cards()); + store.applyDraft(identity, applied, context); + expect(store.getDraft(identity, sibling)).toMatchObject({ state: 'invalidated', reason: 'Plan revision changed.' }); + expect(store.getSuggestions(identity, suggestion)).toMatchObject({ state: 'invalidated', reason: 'Plan revision changed.' }); +}); + +it('fails, at startup, every request an earlier process left pending, and nothing else', () => { + const { store } = fixture(), pending = store.beginSuggestions(identity, state(store), 'draft'), ready = readyDraft(store); + expect(store.settleInterruptedRequests()).toBe(1); + expect(store.getDraft(identity, pending)).toMatchObject({ state: 'failed', reason: 'The server stopped before this request finished. Ask again.' }); + expect(store.getDraft(identity, ready).state).toBe('ready'); + expect(store.settleInterruptedRequests()).toBe(0); +}); + it('dismisses a ready draft', () => { const { store } = fixture(), id = readyDraft(store); store.cancelSuggestions(identity, id, 'Dismissed.'); @@ -195,7 +212,7 @@ it.each([['draft', 'suggest'], ['suggest', 'draft']] as const)('runs one plannin await held; return JSON.stringify(request.mode === 'draft' ? redraft() : cards()); } }); try { const running = slow.start(input, first); - expect(() => slow.start(input, second)).toThrow('A suggestion invocation is still active for this plan.'); + expect(() => slow.start(input, second)).toThrow('A planning invocation is still active for this plan.'); release(); expect(await running.result).toMatchObject({ state: 'completed' }); } finally { await slow.close(); } diff --git a/web/server.ts b/web/server.ts index 219e7a1a..34a91086 100644 --- a/web/server.ts +++ b/web/server.ts @@ -11,7 +11,7 @@ import { ItemExecutor } from '../runner/execution.ts'; import { OWED_REFUSAL, TaskPublishing } from '../runner/publishing.ts'; import type { RunnerAssembly } from '../runner/production.ts'; import type { ShutdownCapability } from '../runner/lifecycle.ts'; -import { BadRequest, GuardRefusal, ShuttingDownError, assertUuidV4, isUuidV4, sameContext } from '../runner/lifecycle.ts'; +import { ActionIdReused, BadRequest, GuardRefusal, ShuttingDownError, assertUuidV4, isUuidV4, sameContext } from '../runner/lifecycle.ts'; import { GhIssueGateway, type IssueGateway } from '../github/issues.ts'; import { demoIssueGateway } from '../scripts/demo-issues.ts'; import { IssueBoard } from './issues.ts'; @@ -80,6 +80,8 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge getSuggestions: (identity, id) => store.getSuggestions(identity, id), }; planning = typeof planningInput === 'function' ? planningInput(service) : planningInput; + // A suggestion or draft still pending belonged to a process that ended; nothing can complete it now (#124). + service.store.settleInterruptedRequests(); // One budget for a suggestion, setup included: lane D's cap, which the provider's own deadline stays inside (#117). suggestions = planning ? new SuggestionCoordinator(suggestionStore, planning.provider, PLANNING_BUDGET_MS) : null; } catch (error) { service.close(); throw error; } @@ -274,6 +276,17 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const mode: PlanningMode = route?.[1] === 'drafts' ? 'draft' : 'suggest', noun = mode === 'draft' ? 'draft' : 'suggestion'; const started = !imported && !route![2]; const kind = imported ? 'plan-import' : started ? `${noun}-start` : `${noun}-${route![3]}`; + // A suggestion cancel or apply recorded before #124 hashed the body alone: its replay still returns the saved outcome + // (or refusal). Drafts are new, so none was recorded that way. + if (route?.[2] && mode === 'suggest') { + try { service.store.savedAction(identity, { actionId, kind, request }); } + catch (error) { + if (!(error instanceof ActionIdReused)) throw error; + const legacy = service.store.savedAction(identity, { actionId, kind, request: body }); + if (!legacy) throw error; + return legacy.response; + } + } // The issue comes from GitHub (#117), so it is read before the recorded action, which runs synchronously. A replay // returns its saved outcome without reading GitHub again, and a start the recorded action would refuse anyway (no // planning, shutdown, stale revision or snapshot) does not read it at all. From 6fb687115860e2196c00687f5134188c6a4cda54 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:20:47 -0700 Subject: [PATCH 4/8] F: address review round 2 on plan drafts (#124) - The startup settle of pending requests moves into the production planning setup, after the single-runner lock is verified to name the database (productionPlanning requires verifyLock). - A cancel or apply body may not carry requestId (the path names it), so an older record's hash cannot match another request. - Only a missing request reads as "Unknown ... request." (Store.requestMode); a storage error is no longer recorded as a refusal. - The draft prompt names github.baseBranch when configured (#117 follow-up). - Docs: cancel-without-handle, the requests lifecycle row, the Store rows. - Tests: snapshot change while drafting, verify-before-settle, baseBranch, body requestId refusal, legacy refusal replay, storage error retry, HTTP dismiss and shutdown cancel. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 4 +- docs/implementation/runner-lifecycle.md | 6 +-- runner/store.ts | 4 ++ test/planning-drafts-api.test.ts | 61 +++++++++++++++++-------- test/planning-drafts.test.ts | 14 ++++++ test/planning-production.test.ts | 32 +++++++++++-- web/cli.ts | 2 +- web/planning.ts | 15 ++++-- web/server.ts | 14 +++--- 9 files changed, 112 insertions(+), 40 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 4662bba3..e9c90d2e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -169,7 +169,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Execution | `runner/execution.ts` | Runs plan items in order: fresh workspace, prompt, agent, post-run audit, then the runner's own commit and ledger entry. Pauses in needs amendment on an out-of-scope edit; moves to needs human on a safety violation. | Not connected; uses a test workspace until #66 part 2 and #87 | | Publishing | `runner/publish.ts`, `runner/publishing.ts`, `runner/branch-push.ts` | Runs the already-fixed check, pushes the task head, then opens or reuses the task's pull request (a draft when the task needs a person). Records each opening before calling GitHub, so a lost outcome can be found again by a marker. `TaskPublishing` (#103) publishes when a run ends, on the `publish` runner action, and once at startup, and records the last outcome. On cancel it closes the task's PRs (#111). | With a runner block (needs `github.baseBranch`); never in a demo | | Lock and startup recovery | `runner/recovery.ts` | One runner per database, held as an OS lock on the database file's device and inode. Startup recovery finalizes interrupted attempts and removes leftover containers and storage. | Lock in use; recovery not connected | -| Store | `runner/store.ts` | The only database handle and all SQL. Revision numbers, snapshots, ledger, approvals, choices, notes, suggestion requests, tasks, attempts, user actions, feedback events, merge attempts, pull request openings, already-fixed results. | In use | +| Store | `runner/store.ts` | The only database handle and all SQL. Revision numbers, snapshots, ledger, approvals, choices, notes, suggestion and draft requests, tasks, attempts, user actions, feedback events, merge attempts, pull request openings, already-fixed results. | In use | | Plan logic | `core/plan.ts`, `core/parse-v1.ts`, `schema/` | Schema validation, YAML and JSON import, meaning checks (IDs, dependencies, paths, projected file operations), literal command parsing, suggestion edits. | Yes | | Linking engine | `core/linking.ts` | Replays each commit's line changes and assigns every final line to an owner from the ledger. Produces segments and the Unplanned and Ambiguous rows. | Yes | | Approvals | `core/approvals.ts` | Approval fingerprints, dependency staleness, assignments, and accept-as-is choices. | Yes | @@ -351,7 +351,7 @@ All state is in one SQLite file, opened with WAL and full synchronization. Each | Group | Tables | Notes | |---|---|---| -| Plans | `plans`, `revisions`, `requests` | SQLite allocates revision numbers. Old revisions are never changed. Suggestion requests are bound to a revision and snapshot. | +| Plans | `plans`, `revisions`, `requests` | SQLite allocates revision numbers. Old revisions are never changed. Suggestion and draft requests (`mode`, #124) are bound to a revision and snapshot. | | Code history | `snapshots`, `ledger`, `rewrites` | Ledger entries are immutable. Rebase mappings record which old commit became which new one; foreign stays foreign. | | Review | `approvals`, `choices`, `review_notes`, `checkpoints`, `continuations` | Stored approvals are claims about a past snapshot. Freshness is recomputed every time. | | Runner | `tasks`, `attempts`, `user_actions`, `feedback_events`, `merge_attempts`, `app_settings` | User actions carry idempotency keys. Feedback events are append-only and feed the future learning feature. | diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 6d2f2887..c840d4e4 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -355,14 +355,14 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina | `POST /api/plan/import` | `Store.importRevision` | expected revision | | `POST /api/plan/suggestions` | E3 coordinator start (`beginSuggestions`) | expected revision and snapshot | | `GET /api/plan/suggestions/:id` | `Store.getSuggestions`, with the request's `mode` | reads only the request row; refuses a draft's ID ("Unknown suggestion request.") | -| `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle (for example after a restart), F calls `Store.cancelSuggestions`. It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | +| `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle, F calls `Store.cancelSuggestions`. After a restart this no longer arises in production: startup fails every request an earlier process left `pending` (#124). It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | | `POST /api/plan/suggestions/:id/apply` | `Store.applySuggestion` | request `ready` and bound to the current revision and snapshot | | `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan | expected revision and snapshot; one planning invocation per plan at a time in this process, shared with suggestions (several drafts may be ready at once) | | `GET /api/plan/drafts/:id` | `Store.getDraft` | reads only a draft request's row | | `POST /api/plan/drafts/:id/cancel` | as suggestion cancel | request is a draft; a `pending` or `ready` one is cancelled, any other state is returned unchanged | | `POST /api/plan/drafts/:id/apply` | `Store.applyDraft`: imports the draft as revision + 1 with every import check | request is a draft, `ready` and bound to the current revision and snapshot | -Live planning runs for a non-demo review with a `github` block (#117): `web/planning.ts` reads the issue from GitHub before each start, and `runner/planning.ts` runs the request in planning's worker. E3's suggestion timer and the request share one ten-minute budget (`PLANNING_BUDGET_MS`). At shutdown E3 aborts every request; one that has not settled within 20 seconds (`PLANNING_SHUTDOWN_GRACE_MS`, as Ask) is ended by abandoning planning's worker. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is not recorded. Without a `github` block, or in a demo, the server returns "Planning agent not available yet". Drafts follow the same rules (#124). A draft and a suggestion are both rows of `requests`, told apart by `mode` (schema v11); each kind's routes refuse the other's IDs. At startup, under the single-runner lock, every request an earlier process left `pending` is failed ("The server stopped before this request finished. Ask again."): its provider ran in that process's planning worker, so nothing can complete it. +Live planning runs for a non-demo review with a `github` block (#117): `web/planning.ts` reads the issue from GitHub before each start, and `runner/planning.ts` runs the request in planning's worker. E3's suggestion timer and the request share one ten-minute budget (`PLANNING_BUDGET_MS`). At shutdown E3 aborts every request; one that has not settled within 20 seconds (`PLANNING_SHUTDOWN_GRACE_MS`, as Ask) is ended by abandoning planning's worker. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is not recorded. Without a `github` block, or in a demo, the server returns "Planning agent not available yet". Drafts follow the same rules (#124). A draft and a suggestion are both rows of `requests`, told apart by `mode` (schema v11); each kind's routes refuse the other's IDs. At startup, once the single-runner lock is verified to name the database (`web/planning.ts`), every request an earlier process left `pending` is failed ("The server stopped before this request finished. Ask again."): its provider ran in that process's planning worker, so nothing can complete it. ## Feedback-event contract (for lane J) @@ -449,7 +449,7 @@ A merged v5 task also gets its `task-closed` event, keyed by the merge attempt I | Lifecycle | Current states | Maps to | Gap and owner | |---|---|---|---| | Questions (`runner/questions.ts`, `QuestionAnswer`) | `pending`, `complete`, `failed`; 125 s persisted lease | `running`, `completed`, `failed` | No durable `cancelled` or `stale`; stale is only worked out when the screen renders. The lease allows a second process to start after 125 s. Fixed when F moves Ask onto D's contract after D5. | -| Suggestions (E3, `requests`) | `pending`, `ready`, `consumed`, `failed`, `cancelled`, `invalidated` | `running`, `completed`, `completed` (applied), `failed`, `cancelled`, `stale` | Durable `cancelled` before settlement (allowed for read-only phases; see the exception above). No change. | +| Suggestions (E3, `requests`) | `pending`, `ready`, `consumed`, `failed`, `cancelled`, `invalidated` | `running`, `completed`, `completed` (applied), `failed`, `cancelled`, `stale` | Durable `cancelled` before settlement (allowed for read-only phases; see the exception above). Drafts share this lifecycle (`mode`, #124). At startup, `pending` requests from an earlier process become `failed`. | | Merge attempts (C and K, `merge_attempts`) | `submitting`, `queued`, `merged`, `removed`, `failed` | `running`, `running` (external), `completed`, and `removed`/`failed` as terminal failures for display only | **Excluded from F's generic retry and state machine.** Merge attempts keep their own states and guards in `MergeCoordinator`, including `requiresFreshReview` after `removed`. F's retry rule never applies to them, and the mapping is only for the shared UI vocabulary. When the outcome is unclear, ownership is kept, as AGENTS.md requires. F6 integrates. | ## Required race regressions for the F1 implementation diff --git a/runner/store.ts b/runner/store.ts index 4bf15b3a..47533a6c 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -482,6 +482,10 @@ export class Store { return this.#run("UPDATE requests SET state='failed', reason=? WHERE state='pending'", 'The server stopped before this request finished. Ask again.').changes as number; } + /** The mode of a planning request, or undefined when there is no such request for this plan. */ + requestMode(identity: PlanIdentity, id: string): PlanningMode | undefined { + return this.#get('SELECT mode FROM requests WHERE key=? AND id=?', identityKey(identity), id)?.mode as PlanningMode | undefined; + } /** A draft request (#124). Refuses a suggestion request's ID. */ getDraft(identity: PlanIdentity, id: string): DraftRequest { const row = this.#get("SELECT * FROM requests WHERE key=? AND id=? AND mode='draft'", identityKey(identity), id); diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 18b33959..e809c62e 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -7,7 +7,6 @@ import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; import type { EditReply } from '../core/plan.ts'; import { createDemo } from '../scripts/demo.ts'; import { startServer, type PlanningDeps } from '../web/server.ts'; -import type { ReviewConfig } from '../runner/review.ts'; import { Store } from '../runner/store.ts'; // The draft API (#124) on the real server and Store, with a stand-in provider. @@ -20,12 +19,9 @@ afterEach(async () => { type Answer = (request: AuthorRequest, signal: AbortSignal) => Promise; type View = { plan: Record; snapshot: { id: string } }; /** `answer` is made from the review as it was when the server started. */ -async function serve(answer: (view: View) => Answer, options: { describe?: PlanningDeps['describe']; - /** Runs on the review's database before the server starts, as an earlier process would have left it. */ - before?: (config: ReviewConfig) => void } = {}) { +async function serve(answer: (view: View) => Answer, options: { describe?: PlanningDeps['describe'] } = {}) { const root = mkdtempSync(join(tmpdir(), 'codeboost-drafts-api-')); roots.push(root); const config = createDemo(join(root, 'demo')); - options.before?.(config); const requests: AuthorRequest[] = []; let answering!: Answer; const provider: AuthorProvider = { invoke: (request, signal) => { requests.push(request); return answering(request, signal); } }; @@ -138,19 +134,6 @@ it('refuses a draft with 503 when shutdown begins while the issue is read', asyn await closing; }); -it('fails, at startup, a draft an earlier process left pending', async () => { - let pending = ''; - const served = await serve(redraft, { before: config => { - const store = new Store(config.database); - try { pending = store.beginSuggestions(config.identity, { revision: store.getPlan(config.identity).revision, snapshotId: store.getSnapshot(config.identity).id }, 'draft'); } - finally { store.close(); } - } }); - expect((await served.api('GET', `/api/plan/drafts/${pending}`)).body) - .toMatchObject({ state: 'failed', reason: 'The server stopped before this request finished. Ask again.' }); - // A new draft starts normally. - expect((await served.start('drafts')).status).toBe(200); -}); - it('refuses one cancel action ID on two suggestions', async () => { const served = await serve(view => async () => JSON.stringify(cards(view.plan.revision))); const first = (await served.start('suggestions')).body.result.requestId as string; @@ -173,3 +156,45 @@ it('replays a suggestion cancel recorded before #124, whose hash had no request // A draft route has no such history: the same action ID is a different request there. expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, { actionId })).toMatchObject({ status: 409 }); }); + +it('refuses a cancel or apply body that carries its own request ID', async () => { + const served = await serve(redraft), id = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', id); + expect(await served.api('POST', `/api/plan/drafts/${id}/apply`, { actionId: randomUUID(), requestId: randomUUID() })) + .toMatchObject({ status: 400, body: { error: 'The request ID comes from the path, not the body.' } }); + expect((await served.api('GET', `/api/plan/drafts/${id}`)).body.state).toBe('ready'); +}); + +it('replays a suggestion refusal recorded before #124 as the same refusal', async () => { + const served = await serve(redraft), actionId = randomUUID(); + const store = new Store(served.config.database); + try { + expect(() => store.userAction(served.config.identity, { actionId, kind: 'suggestion-apply', request: { index: 0 } }, + () => { throw new Error('Suggestion is unavailable.'); })).toThrow(); + } finally { store.close(); } + expect(await served.api('POST', `/api/plan/suggestions/${randomUUID()}/apply`, { index: 0, actionId })) + .toMatchObject({ status: 409, body: { error: 'Suggestion is unavailable.' } }); +}); + +it('does not record a storage error on a cancel as a refusal, so the same action ID can retry', async () => { + const served = await serve(redraft), id = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', id); + const failing = vi.spyOn(Store.prototype, 'requestMode').mockImplementationOnce(() => { + throw Object.assign(new Error('disk I/O error'), { code: 'ERR_SQLITE_ERROR' }); }); + const cancel = { actionId: randomUUID() }; + expect((await served.api('POST', `/api/plan/drafts/${id}/cancel`, cancel)).body.error).toBe('disk I/O error'); + failing.mockRestore(); + expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, cancel)).toEqual({ status: 200, body: { result: { state: 'cancelled' } } }); +}); + +it('dismisses a ready draft, and cancels a pending one when the server shuts down', async () => { + const dismissed = await serve(redraft), ready = (await dismissed.start('drafts')).body.result.requestId as string; + await dismissed.settled('drafts', ready); + expect(await dismissed.api('POST', `/api/plan/drafts/${ready}/cancel`, { actionId: randomUUID() })).toEqual({ status: 200, body: { result: { state: 'cancelled' } } }); + const held = await serve(() => (_, signal) => new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true }))); + const pending = (await held.start('drafts')).body.result.requestId as string; + await held.close(); + const store = new Store(held.config.database); + try { expect(store.getDraft(held.config.identity, pending)).toMatchObject({ state: 'cancelled', reason: 'Suggestion coordinator is closing.' }); } + finally { store.close(); } +}); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index f6a931de..3c0e6f6d 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -203,6 +203,20 @@ it('marks a draft stale when the plan changes while Claude writes it', async () } finally { await coordinator.close(); } }); +it('marks a draft stale when the snapshot changes while Claude writes it', async () => { + const { store } = fixture(); + const { coordinator, input } = coordinate(store, () => { + store.recordHistory(identity, state(store), oid(3), oid(4), []); + return JSON.stringify(redraft()); + }); + try { + const handle = coordinator.start(input, 'draft'); + expect(await handle.result).toMatchObject({ state: 'stale' }); + expect(store.getDraft(identity, handle.id)).toMatchObject({ state: 'invalidated', reason: 'Repository snapshot changed.' }); + expect(store.getPlan(identity).revision).toBe(1); + } finally { await coordinator.close(); } +}); + it.each([['draft', 'suggest'], ['suggest', 'draft']] as const)('runs one planning invocation per plan: a %s blocks a %s', async (first, second) => { const { store } = fixture(); let release!: () => void; diff --git a/test/planning-production.test.ts b/test/planning-production.test.ts index bf94dca4..f1b1902f 100644 --- a/test/planning-production.test.ts +++ b/test/planning-production.test.ts @@ -31,26 +31,48 @@ function production(config = demo()): ReviewConfig { } const text = (number: number): IssueText => ({ number, title: 'Retries ignore the cap', body: 'Body with .', comments: ['Collaborator note.'] }); const closable = (close = vi.fn(async () => undefined)) => ({ close, invoke: vi.fn() }) as unknown as PlanningAgent; +const verified = { verifyLock: () => undefined }; it('is off in a demo and without a github block, so neither plans', () => { const config = demo(); - expect(productionPlanning(config)).toBeUndefined(); - expect(productionPlanning({ ...config, demo: false })).toBeUndefined(); - expect(productionPlanning(production(config))).toBeTypeOf('function'); + expect(productionPlanning(config, verified)).toBeUndefined(); + expect(productionPlanning({ ...config, demo: false }, verified)).toBeUndefined(); + expect(productionPlanning(production(config), verified)).toBeTypeOf('function'); }); it('tells each request the GitHub issue, the repository and the base commit, read with a bound', async () => { const config = production(), service = new ReviewService(config); closers.push(() => service.close()); const issueText = vi.fn(async (number: number) => text(number)), signal = new AbortController().signal; - const deps = productionPlanning(config, { issues: { issueText }, agent: () => closable() })!(service); + const deps = productionPlanning(config, { ...verified, issues: { issueText }, agent: () => closable() })!(service); expect(await deps.describe(signal)).toEqual({ issue: text(config.github!.issue), approvedLessons: [], repo: { name: 'acme/retry-service', baseRef: service.store.getSnapshot(config.identity).base } }); expect(issueText).toHaveBeenCalledWith(config.github!.issue, { signal, timeoutMs: ISSUE_READ_TIMEOUT_MS }); }); +it('names the configured base branch, when there is one', async () => { + const base = production(), config = { ...base, github: { ...base.github!, baseBranch: 'release' } }; + const service = new ReviewService(config); closers.push(() => service.close()); + const deps = productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent: () => closable() })!(service); + expect((await deps.describe(new AbortController().signal)).repo).toEqual({ name: 'acme/retry-service', baseRef: 'release' }); +}); + +it('fails requests an earlier process left pending, only after the lock is verified', () => { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + const store = service.store, expected = { revision: store.getPlan(config.identity).revision, snapshotId: store.getSnapshot(config.identity).id }; + const pending = store.beginSuggestions(config.identity, expected, 'draft'); + // A lock that no longer names this database: nothing is settled, and startup stops. + const refused = productionPlanning(config, { verifyLock: () => { throw new Error('The database path changed.'); }, agent: () => closable() })!; + expect(() => refused(service)).toThrow('The database path changed.'); + expect(store.getDraft(config.identity, pending).state).toBe('pending'); + const order: string[] = [], settle = vi.spyOn(store, 'settleInterruptedRequests'); + productionPlanning(config, { verifyLock: () => { order.push('verified'); expect(settle).not.toHaveBeenCalled(); }, agent: () => closable() })!(service); + expect(order).toEqual(['verified']); + expect(store.getDraft(config.identity, pending)).toMatchObject({ state: 'failed', reason: 'The server stopped before this request finished. Ask again.' }); +}); + it('closes the planning agent when the server closes', async () => { const config = production(), close = vi.fn(async () => undefined); - const setup = productionPlanning(config, { issues: { issueText: async number => text(number) }, agent: () => closable(close) })!; + const setup = productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent: () => closable(close) })!; const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, setup); await app.close(); expect(close).toHaveBeenCalledTimes(1); diff --git a/web/cli.ts b/web/cli.ts index 3ff42d13..90b5e8bf 100644 --- a/web/cli.ts +++ b/web/cli.ts @@ -68,7 +68,7 @@ if (values.help || (!values.demo && !values.config && values['release-preparatio for (const signal of ['SIGINT', 'SIGTERM'] as const) process.on(signal, duringStartup); let app: Awaited>; // Planning (#117) is on for any non-demo review with a github block; it needs no runner block. - try { app = await startServer(config, port, undefined, undefined, undefined, undefined, undefined, productionPlanning(config), runnerSetup); } + try { app = await startServer(config, port, undefined, undefined, undefined, undefined, undefined, productionPlanning(config, { verifyLock: () => lock.verify() }), runnerSetup); } catch (error) { lock.release(); // A refused runner startup (no token, a blocked recovery) is the person's to act on: its message, not a stack. diff --git a/web/planning.ts b/web/planning.ts index cfd9f334..045148d2 100644 --- a/web/planning.ts +++ b/web/planning.ts @@ -12,22 +12,29 @@ export const ISSUE_READ_TIMEOUT_MS = 30_000; * only, bounded like an execute prompt's); approved lessons stay empty until lessons exist (L1 to L4). */ export function productionPlanning(config: ReviewConfig, options: { + /** The single-runner lock's check that the database path still names the locked file (runner/recovery.ts). */ + verifyLock: () => void; issues?: { issueText(number: number, options: { signal?: AbortSignal; timeoutMs?: number }): Promise }; agent?: (service: ReviewService) => PlanningAgent; -} = {}): PlanningSetup | undefined { +}): PlanningSetup | undefined { const github = config.github; if (config.demo || !github) return undefined; const issues = options.issues ?? new GhIssueGateway(github.repository); return service => { + // Runs after the Store opens and before the server listens. A request still pending belonged to a process that + // ended (its provider ran in that process's planning worker), so nothing can complete it: fail it (#124). Only under + // a lock verified to name this database, so a live process's requests are never touched. + options.verifyLock(); + service.store.settleInterruptedRequests(); const provider = options.agent?.(service) ?? new PlanningAgent(service); return { provider, async describe(signal) { const text = await issues.issueText(github.issue, { signal, timeoutMs: ISSUE_READ_TIMEOUT_MS }); - // No base branch name is configured yet, so the base is named by its commit. - const base = service.store.getSnapshot(config.identity).base; + // The configured base branch (#103), or the base commit when none is configured. + const baseRef = github.baseBranch ?? service.store.getSnapshot(config.identity).base; return { issue: { number: text.number, title: text.title, body: text.body, comments: [...text.comments] }, - approvedLessons: [], repo: { name: github.repository, baseRef: base } }; + approvedLessons: [], repo: { name: github.repository, baseRef } }; }, close: () => provider.close(), }; diff --git a/web/server.ts b/web/server.ts index 34a91086..69573a3f 100644 --- a/web/server.ts +++ b/web/server.ts @@ -80,8 +80,6 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge getSuggestions: (identity, id) => store.getSuggestions(identity, id), }; planning = typeof planningInput === 'function' ? planningInput(service) : planningInput; - // A suggestion or draft still pending belonged to a process that ended; nothing can complete it now (#124). - service.store.settleInterruptedRequests(); // One budget for a suggestion, setup included: lane D's cap, which the provider's own deadline stays inside (#117). suggestions = planning ? new SuggestionCoordinator(suggestionStore, planning.provider, PLANNING_BUDGET_MS) : null; } catch (error) { service.close(); throw error; } @@ -271,7 +269,9 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const actionId = requireAction(input), { actionId: _omit, ...body } = input; // Suggestions are edit cards for the current plan; a draft is a whole next revision (#124). They share one lifecycle. const imported = path === '/api/plan/import', route = PLANNING_REQUEST.exec(path); - // The recorded request names the planning request the path acts on, so one action ID cannot replay across two. + // The recorded request names the planning request the path acts on, so one action ID cannot replay across two. The + // path alone names it: a body that also carries one could make an older record's hash match another request's. + if (route?.[2] && 'requestId' in body) throw new BadRequest('The request ID comes from the path, not the body.'); const request = route?.[2] ? { ...body, requestId: route[2] } : body; const mode: PlanningMode = route?.[1] === 'drafts' ? 'draft' : 'suggest', noun = mode === 'draft' ? 'draft' : 'suggestion'; const started = !imported && !route![2]; @@ -325,10 +325,10 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge return { requestId: handle.id }; } const id = route![2]!; - // An ID is used only on its own kind's routes; an unknown one is named by the route's kind. - let current: ReturnType; - try { current = service.store.getSuggestions(identity, id); } catch { throw new Error(`Unknown ${noun} request.`); } - if (current.mode !== mode) throw new Error(`Unknown ${noun} request.`); + // An ID is used only on its own kind's routes; an unknown one is named by the route's kind. A storage error is not + // a refusal, so it is never turned into one (it would be recorded under the action ID). + if (service.store.requestMode(identity, id) !== mode) throw new Error(`Unknown ${noun} request.`); + const current = service.store.getSuggestions(identity, id); if (route![3] === 'cancel') { const handle = suggestionHandles.get(id); if (current.state === 'pending' && handle) { handle.cancel('Cancelled by the user.'); return { state: 'cancelling' }; } From ff9f2a2b6c49fae5f6199dbb92cc7d3e80e07fd8 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:28:05 -0700 Subject: [PATCH 5/8] F: address review round 3 on plan drafts (#124) - A cancel or apply looks up its replay first (AGENTS.md), then refuses a body requestId; the legacy body-only fallback never applies to a body that carries one. - A refused planning startup (lock no longer names the database) exits with its message, not a stack, without a runner block too. - Docs: startup order includes the planning settle; the cancel-without-handle note holds only while planning is on; lane D sees a draft as revision n+1. - Tests: CLI start fails a pending request left by an earlier process; the Store is closed when planning setup refuses; the settle covers every plan and no other state; replay before the body check; legacy apply replay. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 6 +++--- test/cli.test.ts | 17 +++++++++++++++++ test/planning-drafts-api.test.ts | 22 ++++++++++++++++++++++ test/planning-drafts.test.ts | 15 +++++++++++++++ test/planning-production.test.ts | 8 ++++++++ web/cli.ts | 8 +++++--- web/server.ts | 17 +++++++++-------- 7 files changed, 79 insertions(+), 14 deletions(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index c840d4e4..9c0057d6 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -320,7 +320,7 @@ This runs before the coordinator opens. 7. **Unowned preparation check.** If any attempt has `preparation_started_at` set and `preparation_pgid` null (see "Launch"), stop here: do not open the coordinator, name each attempt and directory, and wait for `--release-preparation`. Step 4 does not remove those directories. 8. Open the coordinator. -**Production wiring (#91).** The runner is opt-in: the review configuration's `runner` block names its root, an optional diagnostics directory and cap, the committer and optional storage limits (`runner/production.ts`, `parseRunnerConfig`). Without the block, or in a demo, there is no coordinator and runner actions answer that the runner is not configured. With it, `web/cli.ts` takes the lock, `startServer` opens the `Store` and then, before it listens, runs `setUpRunner`: it checks for the github block (the prompt carries the issue) and `CLAUDE_CODE_OAUTH_TOKEN`, calls the lock's `verify()` (decision 1, step 4), reads the runner token for the locked file, and runs `recoverStartup` with D's own `recoverLeftovers`, `exportTaskDiff` and `removeTaskFilesystemsAsync`. The agent image is built after D's recovery has stopped every leftover agent, so no agent keeps writing to its storage during a long build, and before the first export's deadline starts (the build blocks the event loop, so a deadline armed before it would expire as soon as it returned). Any failure closes the `Store`, releases the lock and exits with its message. To export a recovered storage, F saves the storage's `metadataBaseline` and the commit it was seeded from (`attempts.metadata_baseline`, `attempts.storage_base`, schema v9) as soon as D's allocation returns; a row without them gets "Partial output could not be exported: its storage baseline was never saved". Diagnostics are kept per database, in `//diagnostics`: retention reads references only from its own `Store`, so runners of databases sharing a directory must never see each other's files. Recovered diffs are saved through `saveDiagnostic`, with the same owner-only file and retention as the live path; retention keeps every diff this recovery saved, because none is referenced until the finalization transaction. A diff D cut at its byte limit ends with a `codeboost:` line that says so, on this path and the live one. The issue text a prompt carries (title, body and collaborators' comments) is read once and reused for later items for up to five minutes; the base tree listing is read once per base commit. A first Ctrl+C during startup waits for startup to finish; a second stops the process at once, which recovery treats like a crash at the next start. The deps accept execute attempts only (`RunnerDeps.kinds`): admission refuses any other kind before it writes anything. A plan item runs only through `ItemExecutor`, so `/api/runner` refuses to retry an execute attempt on its own; a task's plan runs again through `resume` (below). +**Production wiring (#91).** The runner is opt-in: the review configuration's `runner` block names its root, an optional diagnostics directory and cap, the committer and optional storage limits (`runner/production.ts`, `parseRunnerConfig`). Without the block, or in a demo, there is no coordinator and runner actions answer that the runner is not configured. With it, `web/cli.ts` takes the lock, `startServer` opens the `Store` and then, before it listens, runs the planning setup when planning is on (`web/planning.ts`: verify the lock, then fail every planning request an earlier process left `pending`, #124) and then `setUpRunner`: it checks for the github block (the prompt carries the issue) and `CLAUDE_CODE_OAUTH_TOKEN`, calls the lock's `verify()` (decision 1, step 4), reads the runner token for the locked file, and runs `recoverStartup` with D's own `recoverLeftovers`, `exportTaskDiff` and `removeTaskFilesystemsAsync`. The agent image is built after D's recovery has stopped every leftover agent, so no agent keeps writing to its storage during a long build, and before the first export's deadline starts (the build blocks the event loop, so a deadline armed before it would expire as soon as it returned). Any failure closes the `Store`, releases the lock and exits with its message. To export a recovered storage, F saves the storage's `metadataBaseline` and the commit it was seeded from (`attempts.metadata_baseline`, `attempts.storage_base`, schema v9) as soon as D's allocation returns; a row without them gets "Partial output could not be exported: its storage baseline was never saved". Diagnostics are kept per database, in `//diagnostics`: retention reads references only from its own `Store`, so runners of databases sharing a directory must never see each other's files. Recovered diffs are saved through `saveDiagnostic`, with the same owner-only file and retention as the live path; retention keeps every diff this recovery saved, because none is referenced until the finalization transaction. A diff D cut at its byte limit ends with a `codeboost:` line that says so, on this path and the live one. The issue text a prompt carries (title, body and collaborators' comments) is read once and reused for later items for up to five minutes; the base tree listing is read once per base commit. A first Ctrl+C during startup waits for startup to finish; a second stops the process at once, which recovery treats like a crash at the next start. The deps accept execute attempts only (`RunnerDeps.kinds`): admission refuses any other kind before it writes anything. A plan item runs only through `ItemExecutor`, so `/api/runner` refuses to retry an execute attempt on its own; a task's plan runs again through `resume` (below). ## HTTP and UI contract @@ -355,9 +355,9 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina | `POST /api/plan/import` | `Store.importRevision` | expected revision | | `POST /api/plan/suggestions` | E3 coordinator start (`beginSuggestions`) | expected revision and snapshot | | `GET /api/plan/suggestions/:id` | `Store.getSuggestions`, with the request's `mode` | reads only the request row; refuses a draft's ID ("Unknown suggestion request.") | -| `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle, F calls `Store.cancelSuggestions`. After a restart this no longer arises in production: startup fails every request an earlier process left `pending` (#124). It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | +| `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle, F calls `Store.cancelSuggestions`. After a restart this no longer arises while planning is on (a non-demo review with a `github` block): startup fails every request an earlier process left `pending` (#124). Without planning, such a request stays `pending` until it is cancelled here. It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows | | `POST /api/plan/suggestions/:id/apply` | `Store.applySuggestion` | request `ready` and bound to the current revision and snapshot | -| `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan | expected revision and snapshot; one planning invocation per plan at a time in this process, shared with suggestions (several drafts may be ready at once) | +| `POST /api/plan/drafts` | E3 coordinator start in draft mode (`beginSuggestions(…, 'draft')`, #124): a whole next revision of the current plan. The request lane D sees carries revision n+1, the revision the draft would become; the Store binds it to n | expected revision and snapshot; one planning invocation per plan at a time in this process, shared with suggestions (several drafts may be ready at once) | | `GET /api/plan/drafts/:id` | `Store.getDraft` | reads only a draft request's row | | `POST /api/plan/drafts/:id/cancel` | as suggestion cancel | request is a draft; a `pending` or `ready` one is cancelled, any other state is returned unchanged | | `POST /api/plan/drafts/:id/apply` | `Store.applyDraft`: imports the draft as revision + 1 with every import check | request is a draft, `ready` and bound to the current revision and snapshot | diff --git a/test/cli.test.ts b/test/cli.test.ts index 81aa978b..3b0ac2c3 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -188,3 +188,20 @@ it('refuses with a message, not a stack, when the database path changes during a expect(cli.err().trim()).toBe('The database path changed while opening. Refusing to start.'); } finally { docker.release(); cli.child.kill('SIGKILL'); } }); +it('fails, once startup has verified the lock, a planning request an earlier process left pending (#124)', async () => { + const { fakeDocker, start, database, identity } = review(), docker = fakeDocker(); + const store = new Store(database); + let pending: string; + try { pending = store.beginSuggestions(identity, { revision: store.getPlan(identity).revision, snapshotId: store.getSnapshot(identity).id }, 'draft'); } + finally { store.close(); } + docker.release(); + const cli = start(docker); + try { + await cli.until(() => cli.out().includes('Review ready'), 'the server to open'); + const reopened = new Store(database); + try { expect(reopened.getDraft(identity, pending)).toMatchObject({ state: 'failed', reason: 'The server stopped before this request finished. Ask again.' }); } + finally { reopened.close(); } + cli.child.kill('SIGINT'); + expect(await cli.exited).toBe(0); + } finally { cli.child.kill('SIGKILL'); } +}); diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index e809c62e..bc9e7698 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -198,3 +198,25 @@ it('dismisses a ready draft, and cancels a pending one when the server shuts dow try { expect(store.getDraft(held.config.identity, pending)).toMatchObject({ state: 'cancelled', reason: 'Suggestion coordinator is closing.' }); } finally { store.close(); } }); + +it('replays a recorded cancel before refusing a body request ID, and refuses one on suggestion routes too', async () => { + const served = await serve(view => async () => JSON.stringify(cards(view.plan.revision))); + const id = (await served.start('suggestions')).body.result.requestId as string; + await served.settled('suggestions', id); + const actionId = randomUUID(), first = await served.api('POST', `/api/plan/suggestions/${id}/cancel`, { actionId }); + expect(first).toEqual({ status: 200, body: { result: { state: 'cancelled' } } }); + // A resend that also names the request in its body still replays: replay comes before every other check. + expect(await served.api('POST', `/api/plan/suggestions/${id}/cancel`, { actionId, requestId: id })).toEqual(first); + expect(await served.api('POST', `/api/plan/suggestions/${id}/apply`, { index: 0, actionId: randomUUID(), requestId: id })) + .toMatchObject({ status: 400, body: { error: 'The request ID comes from the path, not the body.' } }); +}); + +it('replays a suggestion apply recorded before #124 with its saved revision', async () => { + const served = await serve(redraft), actionId = randomUUID(); + const store = new Store(served.config.database); + try { store.userAction(served.config.identity, { actionId, kind: 'suggestion-apply', request: { index: 0 } }, () => ({ revision: 7 })); } + finally { store.close(); } + expect(await served.api('POST', `/api/plan/suggestions/${randomUUID()}/apply`, { index: 0, actionId })).toEqual({ status: 200, body: { result: { revision: 7 } } }); + // The body differs from the recorded one, so it is a different request. + expect(await served.api('POST', `/api/plan/suggestions/${randomUUID()}/apply`, { index: 1, actionId })).toMatchObject({ status: 409 }); +}); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index 3c0e6f6d..aa121a34 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -130,6 +130,21 @@ it('fails, at startup, every request an earlier process left pending, and nothin expect(store.settleInterruptedRequests()).toBe(0); }); +it('settles pending requests of every plan in the database, and no other state', () => { + const { store } = fixture(), other = { ...identity, planId: 'other' }; + store.createPlan(JSON.stringify(plan()), 'json', { ...context, identity: other }, oid(1), oid(2)); + const elsewhere = store.beginSuggestions(other, { revision: 1, snapshotId: store.getSnapshot(other).id }, 'suggest'); + const cancelled = readyDraft(store); store.cancelSuggestions(identity, cancelled, 'Dismissed.'); + // Pending until the apply below invalidates it. + const invalidated = store.beginSuggestions(identity, { revision: 1, snapshotId: store.getSnapshot(identity).id }, 'suggest'); + const consumed = readyDraft(store); store.applyDraft(identity, consumed, context); + expect(store.getSuggestions(identity, invalidated).state).toBe('invalidated'); + expect(store.settleInterruptedRequests()).toBe(1); + expect(store.getSuggestions(other, elsewhere).state).toBe('failed'); + expect(store.getDraft(identity, cancelled)).toMatchObject({ state: 'cancelled', reason: 'Dismissed.' }); + expect(store.getDraft(identity, consumed).state).toBe('consumed'); +}); + it('dismisses a ready draft', () => { const { store } = fixture(), id = readyDraft(store); store.cancelSuggestions(identity, id, 'Dismissed.'); diff --git a/test/planning-production.test.ts b/test/planning-production.test.ts index f1b1902f..e6e84494 100644 --- a/test/planning-production.test.ts +++ b/test/planning-production.test.ts @@ -70,6 +70,14 @@ it('fails requests an earlier process left pending, only after the lock is verif expect(store.getDraft(config.identity, pending)).toMatchObject({ state: 'failed', reason: 'The server stopped before this request finished. Ask again.' }); }); +it('closes the Store and starts nothing when the planning setup refuses', async () => { + const config = production(), close = vi.spyOn(ReviewService.prototype, 'close'); + const setup = productionPlanning(config, { verifyLock: () => { throw new Error('The database path changed.'); }, agent: () => closable() })!; + await expect(startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, setup)).rejects.toThrow('The database path changed.'); + expect(close).toHaveBeenCalledTimes(1); + close.mockRestore(); +}); + it('closes the planning agent when the server closes', async () => { const config = production(), close = vi.fn(async () => undefined); const setup = productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent: () => closable(close) })!; diff --git a/web/cli.ts b/web/cli.ts index 90b5e8bf..f8d67a3a 100644 --- a/web/cli.ts +++ b/web/cli.ts @@ -68,11 +68,13 @@ if (values.help || (!values.demo && !values.config && values['release-preparatio for (const signal of ['SIGINT', 'SIGTERM'] as const) process.on(signal, duringStartup); let app: Awaited>; // Planning (#117) is on for any non-demo review with a github block; it needs no runner block. - try { app = await startServer(config, port, undefined, undefined, undefined, undefined, undefined, productionPlanning(config, { verifyLock: () => lock.verify() }), runnerSetup); } + const planningSetup = productionPlanning(config, { verifyLock: () => lock.verify() }); + try { app = await startServer(config, port, undefined, undefined, undefined, undefined, undefined, planningSetup, runnerSetup); } catch (error) { lock.release(); - // A refused runner startup (no token, a blocked recovery) is the person's to act on: its message, not a stack. - if (!runnerSetup) throw error; + // A refused runner or planning startup (no token, a blocked recovery, a lock that no longer names the database) is + // the person's to act on: its message, not a stack. + if (!runnerSetup && !planningSetup) throw error; console.error(error instanceof Error ? error.message : String(error)); process.exit(1); } for (const signal of ['SIGINT', 'SIGTERM'] as const) process.removeListener(signal, duringStartup); diff --git a/web/server.ts b/web/server.ts index 69573a3f..b37260c5 100644 --- a/web/server.ts +++ b/web/server.ts @@ -269,23 +269,24 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const actionId = requireAction(input), { actionId: _omit, ...body } = input; // Suggestions are edit cards for the current plan; a draft is a whole next revision (#124). They share one lifecycle. const imported = path === '/api/plan/import', route = PLANNING_REQUEST.exec(path); - // The recorded request names the planning request the path acts on, so one action ID cannot replay across two. The - // path alone names it: a body that also carries one could make an older record's hash match another request's. - if (route?.[2] && 'requestId' in body) throw new BadRequest('The request ID comes from the path, not the body.'); + // The recorded request names the planning request the path acts on, so one action ID cannot replay across two. const request = route?.[2] ? { ...body, requestId: route[2] } : body; const mode: PlanningMode = route?.[1] === 'drafts' ? 'draft' : 'suggest', noun = mode === 'draft' ? 'draft' : 'suggestion'; const started = !imported && !route![2]; const kind = imported ? 'plan-import' : started ? `${noun}-start` : `${noun}-${route![3]}`; - // A suggestion cancel or apply recorded before #124 hashed the body alone: its replay still returns the saved outcome - // (or refusal). Drafts are new, so none was recorded that way. - if (route?.[2] && mode === 'suggest') { - try { service.store.savedAction(identity, { actionId, kind, request }); } + if (route?.[2]) { + // Replay first (AGENTS.md): the saved outcome, or saved refusal, under this request's hash. + try { const saved = service.store.savedAction(identity, { actionId, kind, request }); if (saved) return saved.response; } catch (error) { - if (!(error instanceof ActionIdReused)) throw error; + // A suggestion cancel or apply recorded before #124 hashed the body alone; its replay still returns its outcome. + // A body carrying a request ID never takes this path, so an older record cannot be aimed at another request. + if (!(error instanceof ActionIdReused) || mode !== 'suggest' || 'requestId' in body) throw error; const legacy = service.store.savedAction(identity, { actionId, kind, request: body }); if (!legacy) throw error; return legacy.response; } + // Only the path names the request. + if ('requestId' in body) throw new BadRequest('The request ID comes from the path, not the body.'); } // The issue comes from GitHub (#117), so it is read before the recorded action, which runs synchronously. A replay // returns its saved outcome without reading GitHub again, and a start the recorded action would refuse anyway (no From 4386d146694a4e8b977ad716b838dd16210ba0cd Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:34:06 -0700 Subject: [PATCH 6/8] F: address review round 4 on plan drafts (#124) - productionPlanning runs inside the CLI's startup try again, so its errors release the lock as before. - E3's closing reason is "Planning coordinator is closing." (drafts too). - The requests lifecycle row says the startup settle runs while planning is on. - Test: a body naming another request never makes the old-hash fallback replay this build's record. Co-Authored-By: Claude Opus 5.5 --- core/planning-suggestions.ts | 4 ++-- docs/implementation/runner-lifecycle.md | 2 +- test/planning-drafts-api.test.ts | 16 +++++++++++++++- test/planning-suggestions.test.ts | 2 +- web/cli.ts | 7 +++++-- 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/core/planning-suggestions.ts b/core/planning-suggestions.ts index 3d30807d..3510c11e 100644 --- a/core/planning-suggestions.ts +++ b/core/planning-suggestions.ts @@ -54,7 +54,7 @@ export class SuggestionCoordinator { this.#store = store; this.#provider = provider; this.#timeoutMs = timeoutMs; } start(input: SuggestionInput, mode: PlanningMode = 'suggest'): SuggestionHandle { - if (this.#closing) throw new Error('Suggestion coordinator is closing.'); + if (this.#closing) throw new Error('Planning coordinator is closing.'); const identity = { ...input.context.identity }, key = identityKey(identity); if (this.#active.has(key)) throw new Error('A planning invocation is still active for this plan.'); const snapshotId = input.snapshotId; @@ -162,7 +162,7 @@ export class SuggestionCoordinator { this.#closing = true; const active = [...this.#active.values()]; this.#closePromise = Promise.all(active.map(job => job.handle.result)).then(() => undefined); - for (const job of active) job.stop('cancelled', 'Suggestion coordinator is closing.'); + for (const job of active) job.stop('cancelled', 'Planning coordinator is closing.'); return this.#closePromise; } } diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 9c0057d6..65dff0de 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -449,7 +449,7 @@ A merged v5 task also gets its `task-closed` event, keyed by the merge attempt I | Lifecycle | Current states | Maps to | Gap and owner | |---|---|---|---| | Questions (`runner/questions.ts`, `QuestionAnswer`) | `pending`, `complete`, `failed`; 125 s persisted lease | `running`, `completed`, `failed` | No durable `cancelled` or `stale`; stale is only worked out when the screen renders. The lease allows a second process to start after 125 s. Fixed when F moves Ask onto D's contract after D5. | -| Suggestions (E3, `requests`) | `pending`, `ready`, `consumed`, `failed`, `cancelled`, `invalidated` | `running`, `completed`, `completed` (applied), `failed`, `cancelled`, `stale` | Durable `cancelled` before settlement (allowed for read-only phases; see the exception above). Drafts share this lifecycle (`mode`, #124). At startup, `pending` requests from an earlier process become `failed`. | +| Suggestions (E3, `requests`) | `pending`, `ready`, `consumed`, `failed`, `cancelled`, `invalidated` | `running`, `completed`, `completed` (applied), `failed`, `cancelled`, `stale` | Durable `cancelled` before settlement (allowed for read-only phases; see the exception above). Drafts share this lifecycle (`mode`, #124). While planning is on, `pending` requests from an earlier process become `failed` at startup. | | Merge attempts (C and K, `merge_attempts`) | `submitting`, `queued`, `merged`, `removed`, `failed` | `running`, `running` (external), `completed`, and `removed`/`failed` as terminal failures for display only | **Excluded from F's generic retry and state machine.** Merge attempts keep their own states and guards in `MergeCoordinator`, including `requiresFreshReview` after `removed`. F's retry rule never applies to them, and the mapping is only for the shared UI vocabulary. When the outcome is unclear, ownership is kept, as AGENTS.md requires. F6 integrates. | ## Required race regressions for the F1 implementation diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index bc9e7698..099c2bd3 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -195,7 +195,7 @@ it('dismisses a ready draft, and cancels a pending one when the server shuts dow const pending = (await held.start('drafts')).body.result.requestId as string; await held.close(); const store = new Store(held.config.database); - try { expect(store.getDraft(held.config.identity, pending)).toMatchObject({ state: 'cancelled', reason: 'Suggestion coordinator is closing.' }); } + try { expect(store.getDraft(held.config.identity, pending)).toMatchObject({ state: 'cancelled', reason: 'Planning coordinator is closing.' }); } finally { store.close(); } }); @@ -220,3 +220,17 @@ it('replays a suggestion apply recorded before #124 with its saved revision', as // The body differs from the recorded one, so it is a different request. expect(await served.api('POST', `/api/plan/suggestions/${randomUUID()}/apply`, { index: 1, actionId })).toMatchObject({ status: 409 }); }); + +it('never replays a record of this build through another request\'s path by naming it in the body', async () => { + const served = await serve(view => async () => JSON.stringify(cards(view.plan.revision))); + const recorded = (await served.start('suggestions')).body.result.requestId as string; + await served.settled('suggestions', recorded); + const other = (await served.start('suggestions')).body.result.requestId as string; + await served.settled('suggestions', other); + const actionId = randomUUID(); + expect((await served.api('POST', `/api/plan/suggestions/${recorded}/cancel`, { actionId })).status).toBe(200); + // The body names the recorded request: its hash would match that record if the old-hash fallback took this body. + expect(await served.api('POST', `/api/plan/suggestions/${other}/cancel`, { actionId, requestId: recorded })) + .toMatchObject({ status: 409, body: { error: 'Action ID already used for a different request.' } }); + expect((await served.api('GET', `/api/plan/suggestions/${other}`)).body.state).toBe('ready'); +}); diff --git a/test/planning-suggestions.test.ts b/test/planning-suggestions.test.ts index 58cb7b8f..18ca12e2 100644 --- a/test/planning-suggestions.test.ts +++ b/test/planning-suggestions.test.ts @@ -109,7 +109,7 @@ it('closes admission first, aborts all jobs and waits for unsettled providers be expect(f.calls.every(call => call.signal.aborted)).toBe(true); await Promise.resolve(); expect(closed).toBe(false); f.pending.reject(new Error('Provider abort')); - for (const job of [first, second]) expect(await job.result).toMatchObject({ state: 'cancelled', reason: 'Suggestion coordinator is closing.' }); + for (const job of [first, second]) expect(await job.result).toMatchObject({ state: 'cancelled', reason: 'Planning coordinator is closing.' }); await closing; expect(closed).toBe(true); expect(f.store.getSuggestions(f.identity, first.id).state).toBe('cancelled'); expect(f.store.getSuggestions(other.context.identity, second.id).state).toBe('cancelled'); diff --git a/web/cli.ts b/web/cli.ts index f8d67a3a..2203dd3a 100644 --- a/web/cli.ts +++ b/web/cli.ts @@ -68,8 +68,11 @@ if (values.help || (!values.demo && !values.config && values['release-preparatio for (const signal of ['SIGINT', 'SIGTERM'] as const) process.on(signal, duringStartup); let app: Awaited>; // Planning (#117) is on for any non-demo review with a github block; it needs no runner block. - const planningSetup = productionPlanning(config, { verifyLock: () => lock.verify() }); - try { app = await startServer(config, port, undefined, undefined, undefined, undefined, undefined, planningSetup, runnerSetup); } + let planningSetup: ReturnType; + try { + planningSetup = productionPlanning(config, { verifyLock: () => lock.verify() }); + app = await startServer(config, port, undefined, undefined, undefined, undefined, undefined, planningSetup, runnerSetup); + } catch (error) { lock.release(); // A refused runner or planning startup (no token, a blocked recovery, a lock that no longer names the database) is From eff2d39bc42b57cde00a8960ebebd9a70c26d7d4 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:40:15 -0700 Subject: [PATCH 7/8] F: address review round 5 on plan drafts (#124) - An empty github.baseBranch falls back to the base commit in the prompt. - Tests: the configured base branch (or the base commit) reaches the prompt through the server; a draft against a stale revision or snapshot is refused and starts nothing. Co-Authored-By: Claude Opus 5.5 --- test/planning-drafts-api.test.ts | 9 +++++++++ test/planning-production.test.ts | 19 ++++++++++++++----- web/planning.ts | 4 ++-- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 099c2bd3..46e13e45 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -234,3 +234,12 @@ it('never replays a record of this build through another request\'s path by nami .toMatchObject({ status: 409, body: { error: 'Action ID already used for a different request.' } }); expect((await served.api('GET', `/api/plan/suggestions/${other}`)).body.state).toBe('ready'); }); + +it('refuses a draft against a stale revision or snapshot, starting nothing', async () => { + const served = await serve(redraft); + for (const stale of [{ expectedRevision: served.view.plan.revision + 1, snapshotId: served.view.snapshot.id }, + { expectedRevision: served.view.plan.revision, snapshotId: randomUUID() }]) + expect(await served.api('POST', '/api/plan/drafts', { ...stale, feedback: '', actionId: randomUUID() })) + .toMatchObject({ status: 409, body: { error: 'Stale plan revision or snapshot. Reload before asking for a draft.' } }); + expect(served.requests).toHaveLength(0); +}); diff --git a/test/planning-production.test.ts b/test/planning-production.test.ts index e6e84494..d6ede2a3 100644 --- a/test/planning-production.test.ts +++ b/test/planning-production.test.ts @@ -49,11 +49,20 @@ it('tells each request the GitHub issue, the repository and the base commit, rea expect(issueText).toHaveBeenCalledWith(config.github!.issue, { signal, timeoutMs: ISSUE_READ_TIMEOUT_MS }); }); -it('names the configured base branch, when there is one', async () => { - const base = production(), config = { ...base, github: { ...base.github!, baseBranch: 'release' } }; - const service = new ReviewService(config); closers.push(() => service.close()); - const deps = productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent: () => closable() })!(service); - expect((await deps.describe(new AbortController().signal)).repo).toEqual({ name: 'acme/retry-service', baseRef: 'release' }); +it.each([['release', 'release'], ['', null]] as const)('names the configured base branch %j, or else the base commit, in the prompt', async (baseBranch, expected) => { + const base = production(), config = { ...base, github: { ...base.github!, baseBranch } }; + const requests: AuthorRequest[] = []; + const agent = () => ({ close: async () => undefined, invoke: async (request: AuthorRequest, signal: AbortSignal) => { + requests.push(request); return new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true })); } }) as unknown as PlanningAgent; + const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, + productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent })!); + closers.push(() => app.close()); + const call = async (method: string, path: string, body?: unknown) => (await fetch(`${new URL(app.url).origin}${path}`, { method, + headers: { 'x-codeboost-token': app.token, ...(body ? { 'content-type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined })).json() as Promise>; + const view = await call('GET', '/api/review'); + await call('POST', '/api/plan/drafts', { expectedRevision: view.plan.revision, snapshotId: view.snapshot.id, feedback: '', actionId: randomUUID() }); + await vi.waitFor(() => expect(requests).toHaveLength(1)); + expect(requests[0]!.prompt).toContain(`"base_ref":${JSON.stringify(expected ?? view.snapshot.base)}`); }); it('fails requests an earlier process left pending, only after the lock is verified', () => { diff --git a/web/planning.ts b/web/planning.ts index 045148d2..a862093d 100644 --- a/web/planning.ts +++ b/web/planning.ts @@ -31,8 +31,8 @@ export function productionPlanning(config: ReviewConfig, options: { provider, async describe(signal) { const text = await issues.issueText(github.issue, { signal, timeoutMs: ISSUE_READ_TIMEOUT_MS }); - // The configured base branch (#103), or the base commit when none is configured. - const baseRef = github.baseBranch ?? service.store.getSnapshot(config.identity).base; + // The configured base branch (#103), or the base commit when none (or an empty one) is configured. + const baseRef = github.baseBranch || service.store.getSnapshot(config.identity).base; return { issue: { number: text.number, title: text.title, body: text.body, comments: [...text.comments] }, approvedLessons: [], repo: { name: github.repository, baseRef } }; }, From e0737426e0489627d92f9ea69d56a70d555ca9e9 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 4 Oct 2026 17:44:21 -0700 Subject: [PATCH 8/8] F: address review round 6 on plan drafts (#124) - Docs: the planning setup runs with or without a runner block. - Tests: a replayed draft start does not read GitHub again; one apply action ID cannot act on two drafts. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 2 +- test/planning-drafts-api.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 65dff0de..44cd1a37 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -320,7 +320,7 @@ This runs before the coordinator opens. 7. **Unowned preparation check.** If any attempt has `preparation_started_at` set and `preparation_pgid` null (see "Launch"), stop here: do not open the coordinator, name each attempt and directory, and wait for `--release-preparation`. Step 4 does not remove those directories. 8. Open the coordinator. -**Production wiring (#91).** The runner is opt-in: the review configuration's `runner` block names its root, an optional diagnostics directory and cap, the committer and optional storage limits (`runner/production.ts`, `parseRunnerConfig`). Without the block, or in a demo, there is no coordinator and runner actions answer that the runner is not configured. With it, `web/cli.ts` takes the lock, `startServer` opens the `Store` and then, before it listens, runs the planning setup when planning is on (`web/planning.ts`: verify the lock, then fail every planning request an earlier process left `pending`, #124) and then `setUpRunner`: it checks for the github block (the prompt carries the issue) and `CLAUDE_CODE_OAUTH_TOKEN`, calls the lock's `verify()` (decision 1, step 4), reads the runner token for the locked file, and runs `recoverStartup` with D's own `recoverLeftovers`, `exportTaskDiff` and `removeTaskFilesystemsAsync`. The agent image is built after D's recovery has stopped every leftover agent, so no agent keeps writing to its storage during a long build, and before the first export's deadline starts (the build blocks the event loop, so a deadline armed before it would expire as soon as it returned). Any failure closes the `Store`, releases the lock and exits with its message. To export a recovered storage, F saves the storage's `metadataBaseline` and the commit it was seeded from (`attempts.metadata_baseline`, `attempts.storage_base`, schema v9) as soon as D's allocation returns; a row without them gets "Partial output could not be exported: its storage baseline was never saved". Diagnostics are kept per database, in `//diagnostics`: retention reads references only from its own `Store`, so runners of databases sharing a directory must never see each other's files. Recovered diffs are saved through `saveDiagnostic`, with the same owner-only file and retention as the live path; retention keeps every diff this recovery saved, because none is referenced until the finalization transaction. A diff D cut at its byte limit ends with a `codeboost:` line that says so, on this path and the live one. The issue text a prompt carries (title, body and collaborators' comments) is read once and reused for later items for up to five minutes; the base tree listing is read once per base commit. A first Ctrl+C during startup waits for startup to finish; a second stops the process at once, which recovery treats like a crash at the next start. The deps accept execute attempts only (`RunnerDeps.kinds`): admission refuses any other kind before it writes anything. A plan item runs only through `ItemExecutor`, so `/api/runner` refuses to retry an execute attempt on its own; a task's plan runs again through `resume` (below). +**Production wiring (#91).** The runner is opt-in: the review configuration's `runner` block names its root, an optional diagnostics directory and cap, the committer and optional storage limits (`runner/production.ts`, `parseRunnerConfig`). Without the block, or in a demo, there is no coordinator and runner actions answer that the runner is not configured. With it, `web/cli.ts` takes the lock, `startServer` opens the `Store` and then, before it listens, runs the planning setup when planning is on (`web/planning.ts`: verify the lock, then fail every planning request an earlier process left `pending`, #124). That step runs with or without a runner block: every start takes the lock, and planning is on for any non-demo review with a `github` block. Then it runs `setUpRunner`: it checks for the github block (the prompt carries the issue) and `CLAUDE_CODE_OAUTH_TOKEN`, calls the lock's `verify()` (decision 1, step 4), reads the runner token for the locked file, and runs `recoverStartup` with D's own `recoverLeftovers`, `exportTaskDiff` and `removeTaskFilesystemsAsync`. The agent image is built after D's recovery has stopped every leftover agent, so no agent keeps writing to its storage during a long build, and before the first export's deadline starts (the build blocks the event loop, so a deadline armed before it would expire as soon as it returned). Any failure closes the `Store`, releases the lock and exits with its message. To export a recovered storage, F saves the storage's `metadataBaseline` and the commit it was seeded from (`attempts.metadata_baseline`, `attempts.storage_base`, schema v9) as soon as D's allocation returns; a row without them gets "Partial output could not be exported: its storage baseline was never saved". Diagnostics are kept per database, in `//diagnostics`: retention reads references only from its own `Store`, so runners of databases sharing a directory must never see each other's files. Recovered diffs are saved through `saveDiagnostic`, with the same owner-only file and retention as the live path; retention keeps every diff this recovery saved, because none is referenced until the finalization transaction. A diff D cut at its byte limit ends with a `codeboost:` line that says so, on this path and the live one. The issue text a prompt carries (title, body and collaborators' comments) is read once and reused for later items for up to five minutes; the base tree listing is read once per base commit. A first Ctrl+C during startup waits for startup to finish; a second stops the process at once, which recovery treats like a crash at the next start. The deps accept execute attempts only (`RunnerDeps.kinds`): admission refuses any other kind before it writes anything. A plan item runs only through `ItemExecutor`, so `/api/runner` refuses to retry an execute attempt on its own; a task's plan runs again through `resume` (below). ## HTTP and UI contract diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 46e13e45..4a9caa72 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -243,3 +243,26 @@ it('refuses a draft against a stale revision or snapshot, starting nothing', asy .toMatchObject({ status: 409, body: { error: 'Stale plan revision or snapshot. Reload before asking for a draft.' } }); expect(served.requests).toHaveLength(0); }); + +it('replays a draft start without reading GitHub again', async () => { + let reads = 0, issue = 0; + const served = await serve(redraft, { describe: () => { reads++; return { issue: { number: issue, title: 'Retries', body: '', comments: [] }, + approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' } }; } }); + issue = served.view.plan.issue; + const body = { expectedRevision: served.view.plan.revision, snapshotId: served.view.snapshot.id, feedback: '', actionId: randomUUID() }; + const first = await served.api('POST', '/api/plan/drafts', body); + expect(first.status).toBe(200); + expect(await served.api('POST', '/api/plan/drafts', body)).toEqual(first); + expect(reads).toBe(1); +}); + +it('refuses one apply action ID on two drafts', async () => { + const served = await serve(redraft); + const first = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', first); + const second = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', second); + const apply = { actionId: randomUUID() }; + expect((await served.api('POST', `/api/plan/drafts/${first}/apply`, apply)).status).toBe(200); + expect(await served.api('POST', `/api/plan/drafts/${second}/apply`, apply)).toMatchObject({ status: 409, body: { error: 'Action ID already used for a different request.' } }); +});