F: run plan suggestions through lane D in planning's own worker (#117, part 2)

.github/workflows/agent-isolation.yml

@@ -8,6 +8,7 @@ on:
       - 'test/agent-*.test.ts'
       - 'runner/question-*.ts'
       - 'runner/questions.ts'
+      - 'runner/planning*.ts'
       - 'runner/workspace.ts'
       - 'runner/runner-repository.ts'
       - 'runner/execution.ts'
@@ -24,6 +25,7 @@ on:
       - 'test/agent-*.test.ts'
       - 'runner/question-*.ts'
       - 'runner/questions.ts'
+      - 'runner/planning*.ts'
       - 'runner/workspace.ts'
       - 'runner/runner-repository.ts'
       - 'runner/execution.ts'
@@ -49,4 +51,4 @@ jobs:
       - run: npm ci --ignore-scripts
       - run: npm run typecheck
       # The Docker suites share one image tag and daemon, so run test files one at a time.
-      - run: npx vitest run --no-file-parallelism test/agent-contract.test.ts test/agent-clone.test.ts test/agent-container.test.ts test/agent-network.test.ts test/agent-policy.test.ts test/agent-proxy.test.ts test/agent-adapter.test.ts test/agent-supervisor.test.ts test/agent-output.test.ts test/agent-gate.test.ts test/agent-question.test.ts test/runner-workspace.test.ts
+      - run: npx vitest run --no-file-parallelism test/agent-contract.test.ts test/agent-clone.test.ts test/agent-container.test.ts test/agent-network.test.ts test/agent-policy.test.ts test/agent-proxy.test.ts test/agent-adapter.test.ts test/agent-supervisor.test.ts test/agent-output.test.ts test/agent-gate.test.ts test/agent-question.test.ts test/agent-planning.test.ts test/runner-workspace.test.ts

.github/workflows/ci.yml

@@ -18,6 +18,6 @@ jobs:
       - run: npm run typecheck
       # The Docker agent suites run one file at a time in the Agent isolation workflow; running them here
       # would put them in parallel against the same image tag and daemon.
-      - run: npm test -- --exclude test/agent-container.test.ts --exclude test/agent-network.test.ts --exclude test/agent-adapter.test.ts --exclude test/agent-supervisor.test.ts --exclude test/agent-gate.test.ts --exclude test/agent-question.test.ts --exclude test/runner-workspace.test.ts
+      - run: npm test -- --exclude test/agent-container.test.ts --exclude test/agent-network.test.ts --exclude test/agent-adapter.test.ts --exclude test/agent-supervisor.test.ts --exclude test/agent-gate.test.ts --exclude test/agent-question.test.ts --exclude test/agent-planning.test.ts --exclude test/runner-workspace.test.ts
       - run: npx playwright install --with-deps chromium
       - run: npm run test:browser

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/runner` and `/api/plan/*` not connected |
+| 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) |
 | 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 |

docs/implementation/planning-acceptance.md

@@ -72,14 +72,15 @@ The provider and Ask share one container runner, `runReadOnlyAgent` in
 limits, credential rule, output check and wording as a `ReadOnlyFeature` (`ASK_FEATURE`, `PLANNING_FEATURE`). Ask's wording
 and cleanup error are unchanged (#117 part 1).
 
-Left for #117 part 2: the provider's image build, clone and storage calls are
-synchronous, so a server must run it off its request thread, as Ask does. That
-worker should also set TMPDIR to a planning root, as Ask's does, so the input and
-auth staging that lane D leaves when it gives up on cleanup is covered. E3's
-default 120-second suggestion timeout is shorter than the provider's ten-minute
-budget. Startup recovery does not yet remove planning's Docker resources after a
-crash, because they are not attempt allocations in the Store. The recording
-script prints its runner label if it leaves any.
+#117 part 2 wires the provider into the server. Each request runs in planning's
+own worker thread (`runner/planning.ts`, `AgentWorker` with `PLANNING_NAMING`),
+whose TMPDIR is a planning root recorded in a ledger beside the review database,
+so lane D's leftover input and auth staging is covered. Its Docker objects carry
+the database's planning owner, apart from Ask's and the runner's, and the first
+request of a process removes what earlier sessions left by that owner only. E3's
+suggestion timer and the request share one ten-minute budget. The recording
+script still runs the provider in its own process and prints its runner label if
+it leaves anything.
 
 F/G still own live API and UI integration. The Store now guarantees revision/snapshot
 binding and pending-only settlement across processes, and E3 requires the caller's

docs/implementation/runner-lifecycle.md

@@ -354,10 +354,10 @@ 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 startup recovery has already stopped the crashed process's provider. A live planning provider runs only through D (G4 after D5), in a container labelled with this database's runner owner, and D's `recoverLeftovers` (startup step 2) stops it before admission opens. Until D5 there is no live planning provider at all ("Planning agent not available yet"). If a planning provider ever runs outside D, this endpoint must refuse such a request instead, until startup has reconciled it. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows |
+| `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle (for example after a restart), F calls `Store.cancelSuggestions`. It is safe to do so because a crashed process's planning container has no route back to the Store: its output went to that process's worker, which ended with it. A live planning provider runs only through D (#117), in planning's own worker, in containers labelled with this database's planning owner (not the runner's or Ask's). Like Ask's, those containers are removed by D's `recoverLeftovers` for that owner before the first planning request of the next process, not at startup; until then a stray container can keep running read-only (its deadline is enforced from the host, which is gone) until Claude exits or that recovery removes it, and its output is discarded. If a planning provider ever runs outside D, or could report into the Store from another process, this endpoint must refuse such a request instead, until it has been reconciled. `ready`: `Store.cancelSuggestions` (dismiss), because no invocation is running. | request is `pending` or `ready`, as `Store.cancelSuggestions` already allows |
 | `POST /api/plan/suggestions/:id/apply` | `Store.applySuggestion` | request `ready` and bound to the current revision and snapshot |
 
-Live planning invocation waits for D5. Until then, the server returns "Planning agent not available yet" instead of using a fake provider.
+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".
 
 ## Feedback-event contract (for lane J)
 

runner/planning-provider.ts

@@ -5,7 +5,7 @@ import type { InvocationInput, InvocationResult, StopReason } from '../agents/co
 import type { TaskStorageLimits } from '../agents/container/storage.ts';
 import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts';
 import { allocations, exceedsStorage, RetainedStorage, runReadOnlyAgent, sameContext, type ContainerDependencies, type LeftoverPolicy, type Provider,
-  type ReadOnlyFeature, type RepositorySize } from './question-container.ts';
+  type ReadOnlyFeature, type ReadOnlyRun, type RepositorySize } from './question-container.ts';
 
 /** Ask's lane D dependencies, less startup recovery, which the runner owns for planning. */
 export type PlanningDependencies = Omit<ContainerDependencies, 'recover'>;
@@ -63,7 +63,12 @@ export function removePlanningRoot(root: string): void {
 export const PLANNING_STORAGE: TaskStorageLimits = Object.freeze({
   workBytes: 512 * 1024 * 1024, workInodes: 131_072, metadataBytes: 512 * 1024 * 1024, metadataInodes: 131_072,
 });
-const DEFAULT_TIMEOUT_MS = 10 * 60_000;
+/**
+ * The whole budget of one planning request, setup included (#117). Lane D caps the invocation itself at ten minutes,
+ * and E3's suggestion timer uses this same budget, so the request settles inside it.
+ */
+export const PLANNING_BUDGET_MS = 10 * 60_000;
+const DEFAULT_TIMEOUT_MS = PLANNING_BUDGET_MS;
 
 /** Codex reads files only through its shell, and planning runs no process, so Codex cannot plan (#75, #93). */
 export const CODEX_PLANNING_REFUSED = 'Codex cannot write plans yet: it can read the code only by running commands, '
@@ -110,6 +115,22 @@ export const PLANNING_FEATURE: ReadOnlyFeature = Object.freeze({
     : new AggregateError(failures, 'Planning container cleanup did not settle.'),
 });
 
+/** Where a planning request reads the code: the review's repository at the snapshot's head. */
+export interface PlanningSource {
+  readonly vendor: Provider;
+  readonly repository: string;
+  readonly head: string;
+  readonly snapshotId: string;
+}
+/** One planning request as a read-only run, without its owner, which the process running it supplies. */
+export function planningRun(request: AuthorRequest, source: PlanningSource, deadline: number): Omit<ReadOnlyRun, 'runnerOwner'> {
+  if (request.phase !== 'planning' || request.access !== 'read-only') throw new Error('Planning requests must be read-only.');
+  return { provider: source.vendor, repository: source.repository, head: source.head, attemptId: request.requestId,
+    taskId: `planning-${request.requestId}`, deadline, prompt: request.prompt, schemaText: request.schemaText,
+    context: { snapshotId: source.snapshotId, planId: request.identity.planId, planRevision: request.revision,
+      assignmentId: `${request.mode}-${request.issue}`, referencedCodeHash: source.head, stateVersion: 0 } };
+}
+
 /**
  * E2's provider over lane D: each request runs in a fresh container on a read-only copy of `head`, in the "planning"
  * phase (read, list and search only; no commands) with vendor-only egress. The request's schema is the only file in
@@ -129,11 +150,8 @@ export function createPlanningProvider(options: PlanningProviderOptions): Planni
   return { async invoke(request: AuthorRequest, signal: AbortSignal): Promise<string> {
     if (request.phase !== 'planning' || request.access !== 'read-only') throw new Error('Planning requests must be read-only.');
     signal.throwIfAborted();
-    return runReadOnlyAgent(PLANNING_FEATURE, { provider: vendor, repository: options.repository, head: options.head,
-      runnerOwner: options.runnerOwner, attemptId: request.requestId, taskId: `planning-${request.requestId}`,
-      deadline: Date.now() + timeoutMs, prompt: request.prompt, schemaText: request.schemaText,
-      context: { snapshotId: options.snapshotId, planId: request.identity.planId, planRevision: request.revision,
-        assignmentId: `${request.mode}-${request.issue}`, referencedCodeHash: options.head, stateVersion: 0 } },
+    return runReadOnlyAgent(PLANNING_FEATURE, { ...planningRun(request, { vendor, repository: options.repository,
+      head: options.head, snapshotId: options.snapshotId }, Date.now() + timeoutMs), runnerOwner: options.runnerOwner },
       deps, signal, image, retained);
   } };
 }

runner/planning.ts

@@ -0,0 +1,44 @@
+import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts';
+import { AgentWorker } from './question-agent.ts';
+import { LeftoverLedger, PLANNING_NAMING } from './question-leftovers.ts';
+import { PLANNING_BUDGET_MS, planningRun } from './planning-provider.ts';
+import type { ReviewService } from './review.ts';
+
+// Leave the worker time to cancel the container and release storage before E3's own timer fires (as Ask does).
+const SETTLE_MARGIN_MS = 5_000;
+export const NO_PLANNING_AGENT = 'Choose an agent in Settings, then retry.';
+
+/**
+ * The production planning provider (#117): every request runs in planning's own worker thread, so lane D's synchronous
+ * setup never blocks the server. The worker's TMPDIR is a planning root recorded in a ledger beside the review
+ * database, and every Docker object it creates carries the database's planning owner. The first request of a process
+ * removes what earlier sessions left, by that owner only, so Ask's and the runner's agents are never touched.
+ *
+ * The code is the review's repository at the current snapshot's head; E3 discards a reply whose snapshot or revision
+ * changed meanwhile. The vendor is the one chosen in Settings for Ask; Codex is refused (#93).
+ */
+export class PlanningAgent implements AuthorProvider {
+  readonly #service: ReviewService;
+  readonly #worker: AgentWorker;
+  constructor(service: ReviewService, options: { url?: URL; env?: Readonly<Record<string, string | undefined>>;
+    ledger?: LeftoverLedger; abandonAfterDeadlineMs?: number; terminateWaitMs?: number; releaseTimeoutMs?: number } = {}) {
+    this.#service = service;
+    // Beside the review database's canonical path, so a restart of the same review finds what an earlier session left.
+    const ledger = options.ledger ?? LeftoverLedger.forDatabase(service.config.database, PLANNING_NAMING);
+    this.#worker = new AgentWorker(options.url, ledger, { naming: PLANNING_NAMING, env: options.env,
+      abandonAfterDeadlineMs: options.abandonAfterDeadlineMs, terminateWaitMs: options.terminateWaitMs,
+      releaseTimeoutMs: options.releaseTimeoutMs,
+      runnerOwner: () => service.store.planningOwnerToken(ledger.identity!) });
+  }
+  async invoke(request: AuthorRequest, signal: AbortSignal): Promise<string> {
+    signal.throwIfAborted();
+    const store = this.#service.store, vendor = store.questionProvider();
+    if (!vendor) throw new Error(NO_PLANNING_AGENT);
+    const snapshot = store.getSnapshot(request.identity);
+    const run = planningRun(request, { vendor, repository: this.#service.reviewRepository().path, head: snapshot.head,
+      snapshotId: snapshot.id }, Date.now() + PLANNING_BUDGET_MS - SETTLE_MARGIN_MS);
+    return this.#worker.plan(run, signal);
+  }
+  /** Call only after every request has settled (E3's close). Releases what the worker still owns, then stops it. */
+  close(): Promise<void> { return this.#worker.close(); }
+}