diff --git a/core/planning-suggestions.ts b/core/planning-suggestions.ts index 295b0754..3510c11e 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,18 +53,21 @@ 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 { - if (this.#closing) throw new Error('Suggestion coordinator is closing.'); + start(input: SuggestionInput, mode: PlanningMode = 'suggest'): SuggestionHandle { + 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 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.'); 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. */ } } } @@ -151,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/architecture.md b/docs/architecture.md index 2db2cc8c..e9c90d2e 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 | @@ -169,12 +169,12 @@ 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 | | 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 | @@ -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 7c46c81c..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 `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 @@ -354,17 +354,21 @@ 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 | -| `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 | +| `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 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. 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 | -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. 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) **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. @@ -384,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, 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. A suggestion cancel or apply recorded before #124 hashed the body alone; its replay still returns the saved outcome. | Event kind | Source action | `sourceRef` | |---|---|---| @@ -445,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). 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/runner/store.ts b/runner/store.ts index eb38929f..47533a6c 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -2,7 +2,8 @@ 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 { 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,7 +21,11 @@ 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 } +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. */ +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 } @@ -119,8 +124,8 @@ export class Store { this.#db.exec('PRAGMA foreign_keys=ON; PRAGMA journal_mode=WAL; PRAGMA synchronous=FULL;'); this.#transaction(() => { const version = this.#get('PRAGMA user_version')!.user_version as number; - if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10].includes(version)) throw new Error('Unsupported store schema version.'); - if (version === 10) 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)); @@ -176,6 +181,12 @@ export class Store { 'stateVersion',state_version,'at',?) FROM tasks WHERE status IN ('running','needs human')`, new Date().toISOString()); this.#db.exec('PRAGMA user_version=10'); } + // 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; } } @@ -324,21 +335,29 @@ export class Store { if (!row) throw new Error('Unknown snapshot.'); return decode(row.data); } - beginSuggestions(identity: PlanIdentity, expected: ReviewState): 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(() => { 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 { @@ -450,13 +469,53 @@ 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 }; + } + /** + * 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; + } + /** 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); + 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/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-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 new file mode 100644 index 00000000..4a9caa72 --- /dev/null +++ b/test/planning-drafts-api.test.ts @@ -0,0 +1,268 @@ +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'; +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 }); +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, options: { describe?: PlanningDeps['describe'] } = {}) { + const root = mkdtempSync(join(tmpdir(), 'codeboost-drafts-api-')); roots.push(root); + const config = createDemo(join(root, 'demo')); + 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: 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 }); + 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, 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' }); +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.' }); +}); + +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.' } }); +}); + +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('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 }); +}); + +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: 'Planning 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 }); +}); + +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'); +}); + +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); +}); + +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.' } }); +}); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts new file mode 100644 index 00000000..aa121a34 --- /dev/null +++ b/test/planning-drafts.test.ts @@ -0,0 +1,257 @@ +import { randomUUID } from 'node:crypto'; +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), '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.'); + 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(/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('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('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.'); + 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), '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};`); + 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('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; + const held = new Promise(done => { release = done; }); + const { input } = coordinate(store, () => ''); + const slow = new SuggestionCoordinator(store, { async invoke(request) { + await held; return JSON.stringify(request.mode === 'draft' ? redraft() : cards()); } }); + try { + const running = slow.start(input, first); + 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(); } +}); + +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/planning-production.test.ts b/test/planning-production.test.ts index bf94dca4..d6ede2a3 100644 --- a/test/planning-production.test.ts +++ b/test/planning-production.test.ts @@ -31,26 +31,65 @@ 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.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', () => { + 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 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, { 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/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/test/publish.test.ts b/test/publish.test.ts index bace381c..e06b4064 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -337,7 +337,7 @@ describe('schema v7', () => { expect(upgraded.taskPullRequests(identity)).toEqual([]); expect(upgraded.latestAlreadyFixed(identity)).toBeNull(); const db = new DatabaseSync(path, { readOnly: true }); - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 10 }); + 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 58f3d9f9..89930c19 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: 10 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); }); }); @@ -686,7 +686,7 @@ describe('allocation baseline (#91)', () => { db.exec('ALTER TABLE attempts DROP COLUMN metadata_baseline; ALTER TABLE attempts DROP COLUMN storage_base; PRAGMA user_version=8;'); db.close(); const reopened = open(path); expect(reopened.interruptedAttempts()).toEqual([expect.objectContaining({ id: attempt.id, metadataBaseline: null, storageBase: null })]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 10 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); }); }); describe('publish outcomes (#103)', () => { @@ -702,7 +702,7 @@ describe('publish outcomes (#103)', () => { reopened.recordPublish(identity, { outcome: 'opened', draft: false, message: 'Pull request #1 is open.', number: 1, url: 'https://github.com/o/r/pull/1' }); expect(reopened.lastPublish(identity)).toMatchObject({ outcome: 'opened', number: 1 }); expect(reopened.getTask(identity)).toEqual(before); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 10 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 11 }); }); it('stamps an outcome with the version the publish saw, and refuses one it cannot have seen (#114)', () => { const { store } = queued(), version = store.getTask(identity).stateVersion; diff --git a/test/store.test.ts b/test/store.test.ts index d949e2e7..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); @@ -105,15 +105,16 @@ 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', () => { 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 }); @@ -216,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/cli.ts b/web/cli.ts index 3ff42d13..2203dd3a 100644 --- a/web/cli.ts +++ b/web/cli.ts @@ -68,11 +68,16 @@ 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); } + 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 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/planning.ts b/web/planning.ts index cfd9f334..a862093d 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 (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: base } }; + approvedLessons: [], repo: { name: github.repository, baseRef } }; }, close: () => provider.close(), }; diff --git a/web/server.ts b/web/server.ts index afa140eb..b37260c5 100644 --- a/web/server.ts +++ b/web/server.ts @@ -11,11 +11,11 @@ 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'; -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 } }; @@ -30,6 +30,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). */ @@ -72,7 +74,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), @@ -264,10 +266,28 @@ 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 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]}`; + 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]}`; + 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) { + // 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 // planning, shutdown, stale revision or snapshot) does not read it at all. @@ -294,26 +314,31 @@ 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]!; + // 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' }; } // 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; @@ -340,9 +365,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;