diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml index 78e18b52..31728b85 100644 --- a/.github/workflows/live-probes.yml +++ b/.github/workflows/live-probes.yml @@ -42,7 +42,7 @@ jobs: env: CODEBOOST_RUN_AUTH_PROBES: '1' CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - run: npx vitest run --project docker -t '\(live Claude\)$' --reporter=default --reporter=json --outputFile=live-probes.json test/agent-container.test.ts test/agent-question.test.ts test/agent-supervisor.test.ts + run: npx vitest run --project docker -t '\(live Claude\)$' --reporter=default --reporter=json --outputFile=live-probes.json test/agent-container.test.ts test/agent-question.test.ts test/agent-supervisor.test.ts test/agent-planning.test.ts # Vitest exits 0 when it skips every probe, so check that each probe passed and nothing else ran. - name: Check that every probe passed run: node scripts/live-probes.ts check live-probes.json diff --git a/docs/implementation/agent-isolation.md b/docs/implementation/agent-isolation.md index 1ba773c4..10c4fdf5 100644 --- a/docs/implementation/agent-isolation.md +++ b/docs/implementation/agent-isolation.md @@ -42,7 +42,7 @@ Each row is a T9 requirement for the Docker suite. The suite fails if any row fa | Task and scratch byte and inode limits hold | `agent-container`: task capacity; scratch capacity for Codex and Claude (`/tmp`, `HOME`, `CODEX_HOME`, output directory) | | Hard links and alias writes from `.git/config` and objects fail, and metadata stays unchanged | `agent-container`: metadata alias probe in planning, review and execute, with a digest of `.git` before and after | | Mountpoint replacement fails | `agent-container`: metadata and metadata alias probes (`mv` and `rm -rf` of `.git`) | -| The Claude startup probes read the schema and return bounded valid output through their documented channel | `agent-supervisor` live probes (credentials required): Claude in questions through its stdout envelope; Claude in planning returns a schema-constrained answer as bare JSON; `agent-container`: the authenticated Claude startup path. Codex has no live probe while it is refused (#93). | +| The Claude startup probes read the schema and return bounded valid output through their documented channel | `agent-supervisor` live probes (credentials required): Claude in questions through its stdout envelope; Claude in planning returns a schema-constrained answer as bare JSON; `agent-planning`: the planning check (#164) passes with a real sign-in; `agent-container`: the authenticated Claude startup path. Codex has no live probe while it is refused (#93). | | Nothing can be written beneath a gitlink, and execute and fix launch only on a clean tree (#81) | `agent-container`: the gitlink probe (writes, `mkdir`, `rmdir` and `mv` at every gitlink fail; the rest of `/work` stays writable); a pre-populated nested checkout, a restart with an untracked symlink parent and a restart with an occupied add destination are refused; a gitlink name Docker cannot mount, more than 256 gitlinks and an unreadable tree are refused; each check serves one profile over its own storage and head; the validator rejects a gitlink mount that is missing, writable, or of another size or mode; every directory above a nested gitlink is pinned, writable and `nosuid,nodev`, so renaming one fails (#99); the validator rejects a pin that is missing, read-only, at another subpath, from another volume, `volume-nocopy`, or not a volume | | Hostile input stays inside the boundary | `agent-container`: repositories with links that leave the checkout are refused, links inside the checkout still work, oversized repositories fail closed; `agent-policy`: option-like prompts; `agent-proxy`: hostile CONNECT traffic; `agent-supervisor`: hostile output | diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 27524937..bc66b44a 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -10,7 +10,7 @@ settings are not stored in the repository. | --- | --- | --- | --- | | `.github/workflows/ci.yml` | `test` | every pull request; pushes to `main` and `codex/**` | Type check; every top-level `test/*.test.ts` file except the Docker-backed files; the Playwright browser suite | | `.github/workflows/agent-isolation.yml` | `real-docker` | every pull request; pushes to `main` and `codex/**` | Type check; the complete Vitest suite against real Docker | -| `.github/workflows/live-probes.yml` | `live-probes` | started by hand (`workflow_dispatch`); every night at 06:23 UTC | The four live Claude startup probes only | +| `.github/workflows/live-probes.yml` | `live-probes` | started by hand (`workflow_dispatch`); every night at 06:23 UTC | The five live Claude startup probes only | `test` and `real-docker` have no path filters. They run on every pull request, so both can be required checks. A required check that a path filter skips @@ -112,8 +112,9 @@ The job: missing. A missing secret never gives a passing run. 2. Runs the Docker Vitest project with `CODEBOOST_RUN_AUTH_PROBES=1`. It selects only the tests whose titles end in `(live Claude)`, in - `test/agent-container.test.ts`, `test/agent-question.test.ts` and - `test/agent-supervisor.test.ts`. The files run one at a time. + `test/agent-container.test.ts`, `test/agent-question.test.ts`, + `test/agent-supervisor.test.ts` and `test/agent-planning.test.ts` (the + planning check, #164). The files run one at a time. 3. Runs `node scripts/live-probes.ts check` on the Vitest JSON report. Vitest exits with success when it skips every test, so this step fails unless each probe listed in `scripts/live-probes.ts` passed and no other test ran. diff --git a/docs/implementation/plan-library-alignment.md b/docs/implementation/plan-library-alignment.md index 0d2e9bba..10f0ab1e 100644 --- a/docs/implementation/plan-library-alignment.md +++ b/docs/implementation/plan-library-alignment.md @@ -24,7 +24,7 @@ Issue #6 tracked the v1 plan library contract and the runner obligations that ca | Output limits at the exact byte | **This change** (tests; the supervisor already enforced the limits) | Supervisor enforcement: `test/agent-supervisor.test.ts` (real Docker). Decoder behaviour only, not a limit: the Claude decoder handles an envelope of exactly the stdout ceiling, and the 1 MiB parser limit applies to the decoded answer at the exact byte: `test/agent-adapter.test.ts`. At #6 the decoder re-serialized the answer, so the limit applied to the re-encoded text. *Later:* #166 returns the exact text of `structured_output` in the CLI's envelope, so the limit applies to those bytes | | Prompt byte budget before spawn | Done before this change | `MAX_PROMPT_BYTES` in `core/planning-author.ts`; `test/planning-author.test.ts` ("accepts exactly the prompt byte limit and rejects the next byte without truncation") | | Model budget before spawn | Moved to #163. *Later:* done in #163 | `MAX_PLANNING_INPUT_BYTES` and `PLANNING_MAX_TURNS` in `agents/policy.ts`; `test/agent-policy.test.ts` ("accepts a planning request at exactly its model budget and refuses one byte more"); [agent isolation](agent-isolation.md#planning-model-budget) | -| Planning profile probed at startup | Moved to #164 | — | +| Planning profile probed at startup | Moved to #164. *Later:* fixed in #164 (the review-view state is #198), checked at startup, on the first request after the vendor changes and on the first request a minute after a failure | `runner/planning.ts` (`PlanningAgent.probe`, `PlanningCheck`), `web/planning.ts`; `test/planning-production.test.ts` ("the planning check (#164)"), `test/planning-worker.test.ts`, `test/agent-planning.test.ts` (live Claude probe); `docs/implementation/planning-acceptance.md` | | Docker daemon log limits | Moved to #165. *Later:* fixed in #165 with `--log-driver=none`, checked before launch | `agents/container/profile.ts`, `agents/container/run.ts`; `test/agent-container.test.ts` ("creates the agent container with no Docker daemon log", "refuses an agent container with %s at validation") (real Docker), `test/agent-daemon-log.test.ts`. Agent containers only; the runner's other containers (task-storage scripts, the keeper, the egress proxy) keep the daemon's default log | | Claude structured output under the v1 parsing contract | Moved to #166 (found during this change). *Later:* fixed in #166 | `agents/adapters/claude.ts`; `test/agent-adapter.test.ts` ("lets draft validation refuse %s in a structured answer") | diff --git a/docs/implementation/planning-acceptance.md b/docs/implementation/planning-acceptance.md index 37eabaa6..97763d06 100644 --- a/docs/implementation/planning-acceptance.md +++ b/docs/implementation/planning-acceptance.md @@ -82,6 +82,59 @@ 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. +The planning check (#164) proves that the vendor chosen in Settings can run a +planning invocation before any request does. It is one planning job in the same +worker and profile as a real request (read, list and search only, vendor-only +egress, a mounted schema), on the review's code at the snapshot's head, with a +real request's budget, since a process's first job may build the agent image. It +asks for the schema's own answer, `{"ok": true}`, so it covers sign-in, the +schema-constrained answer and the read-only profile (`PlanningAgent.probe`). +`PlanningCheck` starts it when the server starts, before it listens, and again +on the first request after the vendor changes, cancelling the check it replaces. +`productionPlanning`'s `describe` waits for it, at most 30 s in all, before it +reads the issue or records a planning request, and the request checks again just +before it starts, so a vendor chosen during the issue read never runs unchecked. +That last check runs inside the Store's transaction, so it starts no check +itself: it refuses, and the check it needs starts on the next turn. The provider +checks once more when it is invoked, as defence in depth (E3 invokes it in a +microtask right after the recorded action); a refusal there settles the request +as failed. A failed check refuses the request with a `GuardRefusal` that starts +"Planning is unavailable:" and ends with the reason exactly as given (some end +with a command to copy), and a check still running after the wait is refused +with the same prefix; the server records the refusal and starts no agent. +Requests within a minute of a failure are refused with it; the next request +after that checks again. With no vendor chosen nothing is checked, and the +request fails later, as before, with `NO_PLANNING_AGENT`. Once planning requests +have settled, or after the server's 20 s grace for them, shutdown cancels a +running check and waits up to 20 s more for it before closing the worker. A +check that runs before the runner's setup has set the runner-owned repository +fails, on a task with runner commits, with "which is not configured"; the +recheck a minute later passes once the runner's setup has finished, and without +a runner block it fails each time, as every planning request would. A check that +has not settled within a real request's budget and a minute fails, so the +recheck applies to it. A reason that holds until codeboost restarts (an +abandoned worker, cleanup codeboost could not confirm, a database with other +hard links, a missing token) gets no promise of a recheck, and Codex is told to +choose another agent. A recheck joins the worker's one startup recovery, so a +recovery that never ends fails each recheck until codeboost restarts. A passed +check is kept until a check for another vendor replaces it: a worker abandoned +later, cleanup that did not settle (the check's own run included, when its +answer still came back), or a token revoked later, shows as a failed request. A +refusal recorded when a request stopped waiting, or when the Store could not be +read during the wait, is replayed if the browser resends the same action; +keeping a Store error out of the record needs `web/server.ts`. Showing the +check's state in the review view before a request, and the other limits that +need `web/server.ts`, are #198. A check deferred from the Store's transaction +can start during shutdown, before planning closes; close then waits for it +within its bound. If startup fails after planning's setup, the CLI exits, and +the next start's planning recovery removes what the check left. Tests: +`test/planning-production.test.ts` ("the planning check (#164)", with a check +that passes, one that fails and one that does not finish), +`test/planning-worker.test.ts` (the check in planning's worker, including a +wrong answer) and `test/agent-planning.test.ts` (against the vendor, real +Docker: an invalid sign-in fails, and a live probe in the Live probes workflow +passes with a real one). + The Store guarantees revision/snapshot binding and pending-only settlement across processes, and E3 requires the caller's snapshot identity before admission. G4 now composes the browser with production planning setup, lane D's provider boundary, diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 75109e79..22d624fb 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -361,14 +361,14 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina | `POST /api/plan/import` | `Store.importRevision` | expected revision; body of at most 6 MiB + 16 KiB, so every source the 1 MiB parser accepts fits (see `docs/plan-format.md`, #146) | | `POST /api/plan/suggestions` | E3 coordinator start (`beginSuggestions`) | expected revision and snapshot | | `GET /api/plan/suggestions/:id` | `Store.getSuggestions`, with the request's `mode` | reads only the request row; refuses a draft's ID ("Unknown suggestion request.") | -| `POST /api/plan/suggestions/:id/cancel` | `pending`: `handle.cancel(reason)` on the `SuggestionHandle` that `SuggestionCoordinator.start()` returned. E3's coordinator has no cancel-by-ID method, so F keeps a map from request ID to handle, adds each handle when it starts the request, and removes it only after the handle's outcome settles. If the request is `pending` but F holds no handle, F calls `Store.cancelSuggestions`. After a restart this no longer arises 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/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 in the next process's first planning job that can sign in: the planning check at startup when Claude is chosen and `CLAUDE_CODE_OAUTH_TOKEN` is set, unless that check fails before its recovery runs (for example, on a task with runner commits before the runner's setup has set the runner-owned repository, see `planning-acceptance.md`; or when planning's leftover record refuses it, or another process holds planning's lock) (#164, which also takes planning's lock from then until planning closes at shutdown, or until the process exits after an abandoned worker or a recovery stopped part way, and builds the agent image in planning's worker while the runner, if one is configured, builds its own), otherwise the first check a planning request starts (a check whose recovery fails leaves it to the next one); 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 recorded for replay like a refusal (`web/server.ts`; `test/planning-production.test.ts`). 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. +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, after the planning check (#164) has had up to 20 seconds more to stop. A start replays its recorded outcome before reading GitHub; a GitHub failure is a 502 and is recorded for replay like a refusal (`web/server.ts`; `test/planning-production.test.ts`). 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) diff --git a/runner/planning.ts b/runner/planning.ts index d6fe00af..26b177d2 100644 --- a/runner/planning.ts +++ b/runner/planning.ts @@ -1,5 +1,9 @@ +import { randomUUID } from 'node:crypto'; +import { setMaxListeners } from 'node:events'; import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; -import { AgentWorker } from './question-agent.ts'; +import { GuardRefusal } from './lifecycle.ts'; +import { AgentWorker, type Provider } from './question-agent.ts'; +import { StopError } from './question-container.ts'; import { LeftoverLedger, PLANNING_NAMING } from './question-leftovers.ts'; import { PLANNING_BUDGET_MS, planningRun } from './planning-provider.ts'; import type { ReviewService } from './review.ts'; @@ -8,11 +12,30 @@ import type { ReviewService } from './review.ts'; const SETTLE_MARGIN_MS = 5_000; export const NO_PLANNING_AGENT = 'Choose an agent in Settings, then retry.'; +/** + * The planning check (#164): one planning invocation in the profile a real request gets (lane D's "planning" phase: + * read, list and search only, vendor-only egress, the answer constrained by a mounted schema), on the review's code at + * the current snapshot's head. It passes when the agent signs in and returns exactly this schema's answer. + */ +export const PLANNING_PROBE_PROMPT = 'This is codeboost\'s check that planning works. Read no files. ' + + 'Answer with the JSON object {"ok": true} and nothing else.'; +export const PLANNING_PROBE_SCHEMA = JSON.stringify({ type: 'object', properties: { ok: { const: true } }, + required: ['ok'], additionalProperties: false }); +/** + * Whether a planning answer is exactly the check's: the object `{"ok": true}`, as JSON, with that one member written + * once (JSON.parse would let a repeated key's last value win). + */ +export function isPlanningProbeAnswer(text: string): boolean { + return /^[ \t\n\r]*\{[ \t\n\r]*"ok"[ \t\n\r]*:[ \t\n\r]*true[ \t\n\r]*\}[ \t\n\r]*$/.test(text); +} + /** * 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. + * database, and every Docker object it creates carries the database's planning owner. The first job of a process that + * can sign in (in production, web/planning.ts, always a planning check: at startup, or one a request starts) removes + * what earlier sessions left, by that owner only, so Ask's and the runner's agents are never touched. A first job whose + * recovery fails leaves it to the next job. * * 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). @@ -39,6 +62,238 @@ export class PlanningAgent implements AuthorProvider { snapshotId: snapshot.id }, Date.now() + PLANNING_BUDGET_MS - SETTLE_MARGIN_MS); return this.#worker.plan(run, signal); } + /** + * Run the planning check (#164) with `vendor`, in this provider's worker. Rejects with the reason when the agent + * cannot run a planning invocation (no sign-in, Codex, Docker, a refused or wrong answer); resolves when it can. + */ + async probe(vendor: Provider, signal: AbortSignal): Promise { + signal.throwIfAborted(); + const store = this.#service.store, identity = this.#service.config.identity; + const plan = store.getPlan(identity), snapshot = store.getSnapshot(identity), requestId = randomUUID(); + const run = planningRun({ mode: 'suggest', phase: 'planning', access: 'read-only', identity, requestId, issue: plan.issue, + revision: plan.revision, prompt: PLANNING_PROBE_PROMPT, schemaText: PLANNING_PROBE_SCHEMA }, + { vendor, repository: this.#service.reviewRepository().path, head: snapshot.head, snapshotId: snapshot.id }, + // A real request's budget: a process's first job may build the agent image, which takes its time from this. + Date.now() + PLANNING_BUDGET_MS - SETTLE_MARGIN_MS); + const text = await this.#worker.plan({ ...run, taskId: `planning-probe-${requestId}` }, signal); + if (!isPlanningProbeAnswer(text)) throw new Error('The planning agent did not give the check\'s answer.'); + } /** Call only after every request has settled (E3's close). Releases what the worker still owns, then stops it. */ close(): Promise { return this.#worker.close(); } } + +/** + * Starts every refusal from planning admission, such as a failed or unfinished check, a vendor changed since the + * request was admitted, a stopped wait, a Store that cannot be read, and planning stopping. + */ +export const PLANNING_UNAVAILABLE = 'Planning is unavailable:'; +/** How long a failed check refuses requests before the next request checks again. */ +export const PLANNING_RECHECK_MS = 60_000; +/** How long a request waits for a running check before it is refused, so the browser is never held for long. */ +export const PLANNING_ADMIT_WAIT_MS = 30_000; +/** How long close waits for a check that is still running after it is cancelled. */ +export const PLANNING_CHECK_CLOSE_MS = 20_000; + +/** One run of the planning check, for one vendor. `passed` or `failure` is set once it settles. */ +type Check = { vendor: Provider; done: Promise; passed: boolean; failure?: { error: unknown; at: number }; + abort: AbortController }; +const messageOf = (error: unknown) => error instanceof Error ? error.message : String(error); +const STILL_CHECKING = `${PLANNING_UNAVAILABLE} codeboost is still checking the planning agent. A check can take ` + + 'several minutes the first time, while the agent image is built. Retry in a minute.'; +const duration = (ms: number) => ms === 60_000 ? 'a minute' : ms < 1_000 ? 'a moment' + : ms >= 120_000 ? `${Math.round(ms / 60_000)} minutes` : `${Math.round(ms / 1_000)} s`; +// Reasons a recheck cannot help, so only a restart can, once any cause is fixed: an abandoned worker +// (question-agent.ts), unconfirmed cleanup (planning-provider.ts' "off until codeboost restarts"), a database file with +// other hard links (question-leftovers.ts), and a missing token, since codeboost's environment is fixed while it runs. +// Each is codeboost's own message, given before any vendor output. A run whose cleanup did not settle is not among +// them: its storage is retried before the next run, and if planning did go off, the recheck reports that with these +// words. +const UNTIL_RESTART = /until codeboost restarts|needs CLAUDE_CODE_OAUTH_TOKEN|a database file without hard links/; +const ownText = (message: string) => message.split(' Claude said:')[0]!; +/** + * How long one check may take before it fails: a real request's budget, as the probe has, and a minute for the worker + * to settle. No request waits that long (see PLANNING_ADMIT_WAIT_MS): requests after it are refused with the failure + * until the recheck window passes, and the next one after that checks again. A recheck joins the worker's one startup + * recovery, so a recovery that never ends fails each recheck the same way until codeboost restarts. + */ +export const PLANNING_CHECK_TIMEOUT_MS = PLANNING_BUDGET_MS + 60_000; +/** + * Admission for planning requests (#164). The check runs for the vendor chosen in Settings: `start` begins it at + * startup, and `admit`, called before anything about a request is recorded, waits for it. The last check is kept: a + * passed one admits every request for its vendor; a failed one refuses them for `recheckMs`, after which the next + * request checks again. The first request after the vendor changes starts that vendor's check and cancels the one it + * replaces. A request waits for a check for at most `waitMs` in all, then is refused as still checking. With no vendor + * chosen nothing is checked, and the request fails later, as before, with `NO_PLANNING_AGENT`. + */ +export class PlanningCheck { + readonly #probe: (vendor: Provider, signal: AbortSignal) => Promise; + readonly #vendor: () => Provider | null; + readonly #recheckMs: number; + readonly #waitMs: number; + readonly #timeoutMs: number; + readonly #abort = new AbortController(); + readonly #running = new Set>(); + #current?: Check; + // Tells a request waiting on an older check that `#current` was replaced, so it moves to the new one at once. A + // listener, removed when the wait ends, so no settled request stays attached. + readonly #events = new EventTarget(); + constructor(probe: (vendor: Provider, signal: AbortSignal) => Promise, vendor: () => Provider | null, + options: { recheckMs?: number; waitMs?: number; timeoutMs?: number } = {}) { + // One listener per waiting request, each removed when its wait ends: many at once are no leak. (Node 26 warns of + // none on an AbortSignal, which holds one per running check and one per waiting request.) + setMaxListeners(0, this.#events); + this.#probe = probe; this.#vendor = vendor; + this.#recheckMs = options.recheckMs ?? PLANNING_RECHECK_MS; this.#waitMs = options.waitMs ?? PLANNING_ADMIT_WAIT_MS; + this.#timeoutMs = options.timeoutMs ?? PLANNING_CHECK_TIMEOUT_MS; + } + /** + * Begin the check for the chosen vendor, if any, without waiting for it. After close it throws "planning is + * stopping". + */ + start(): void { + const vendor = this.#vendor(); + if (vendor) this.#check(vendor); + } + // The current check if it covers `vendor`: for that vendor, and passed, running, or failed less than `recheckMs` ago. + #covering(vendor: Provider) { + const current = this.#current; + return current && current.vendor === vendor + && !(current.failure && performance.now() - current.failure.at >= this.#recheckMs) ? current : undefined; + } + // The check that covers `vendor`, started now if none does. + #check(vendor: Provider) { + // First: after close nothing is admitted, not even under a check that passed. + if (this.#abort.signal.aborted) throw new GuardRefusal(`${PLANNING_UNAVAILABLE} planning is stopping.`); + const covering = this.#covering(vendor); + if (covering) return covering; + // A check for another vendor no longer admits anything: stop it, so replaced checks do not pile up in the worker. + if (this.#current && !this.#current.passed && !this.#current.failure) { + this.#current.abort.abort(new Error('The planning agent chosen in Settings changed.')); + } + const abort = new AbortController(), stop = () => abort.abort(this.#abort.signal.reason); + this.#abort.signal.addEventListener('abort', stop, { once: true }); + const check: Check = { vendor, done: Promise.resolve(), passed: false, abort }; + let probe: Promise; + // Started now, so close can cancel it; a probe that throws instead of rejecting fails the check the same way. + try { probe = this.#probe(vendor, abort.signal); } catch (error) { probe = Promise.reject(error); } + // Bounded: a check that has not settled in time fails, and is cancelled, so the recheck path applies to it. + let timer: ReturnType | undefined; + const late = new Promise((_, reject) => { + timer = setTimeout(() => { + const error = new Error(`The planning check did not finish within ${duration(this.#timeoutMs)}.`); + reject(error); abort.abort(error); + }, this.#timeoutMs); + timer.unref?.(); + }); + // Never rejects: the outcome is kept on the check, so every waiting request gets it. + const done = Promise.race([probe, late]).then(() => { check.passed = true; }, (error: unknown) => { + check.failure = { error, at: performance.now() }; + }).finally(() => { clearTimeout(timer); this.#abort.signal.removeEventListener('abort', stop); }); + check.done = done; + // Close waits for the probe itself too: a check that timed out has settled, but its job may still be in the worker. + // (A worker that never ends a job adds one such probe each recheck window; close's bound still holds.) + const settled = probe.then(() => undefined, () => undefined); + for (const running of [done, settled]) { + this.#running.add(running); + void running.finally(() => this.#running.delete(running)); + } + this.#current = check; + this.#events.dispatchEvent(new Event('replaced')); + return check; + } + /** + * Resolve once the check for the vendor chosen now has passed, or at once when no vendor is chosen; reject with a + * `GuardRefusal` naming why when it failed, or when no check passed within `waitMs`. `signal` stops only this wait, + * never the check, which other requests may share: an aborted wait rejects with `signal.reason`, and a vendor that + * cannot be read (a Store error) with that error; the caller maps both. + * + * A passed check is kept until a check for another vendor replaces it: a worker that is abandoned later, cleanup that + * does not settle later (the check's own run included, when its answer still came back), or a token revoked later + * shows as a failed request, not as "Planning is unavailable". + */ + async admit(signal: AbortSignal): Promise { + const deadline = performance.now() + this.#waitMs; + for (;;) { + // A request that stopped waiting starts, replaces or cancels no check. + signal.throwIfAborted(); + // After close nothing is admitted, with or without a vendor. + if (this.#abort.signal.aborted) throw new GuardRefusal(`${PLANNING_UNAVAILABLE} planning is stopping.`); + const vendor = this.#vendor(); + if (!vendor) return; + const check = this.#check(vendor); + // A settled check gives its verdict at once. + if (check.passed || check.failure) return this.#verdict(check); + let release!: () => void, moved!: () => void, closing!: () => void, timer: ReturnType | undefined; + const stopped = new Promise<'timeout' | 'replaced' | 'closing'>((resolve, reject) => { + release = () => reject(signal.reason); signal.addEventListener('abort', release, { once: true }); + moved = () => resolve('replaced'); this.#events.addEventListener('replaced', moved, { once: true }); + // Close wakes the wait at once, even when its check's probe does not stop: the loop then refuses it as + // stopping. + closing = () => resolve('closing'); this.#abort.signal.addEventListener('abort', closing, { once: true }); + timer = setTimeout(() => resolve('timeout'), Math.max(0, deadline - performance.now())); + }); + let outcome: void | 'timeout' | 'replaced' | 'closing'; + try { outcome = await Promise.race([check.done, stopped]); } + finally { + signal.removeEventListener('abort', release); this.#events.removeEventListener('replaced', moved); + this.#abort.signal.removeEventListener('abort', closing); clearTimeout(timer); + } + if (outcome === 'closing') continue; + // A vendor changed meanwhile is checked (or, if none is chosen now, the request goes on) within what is left of + // the wait, which has passed: only a check that has already settled gives its verdict. + if (outcome === 'timeout' && this.#vendor() === vendor) throw new GuardRefusal(STILL_CHECKING); + // Settings changed while this request waited, or its check was replaced (the vendor changed and changed back): + // check the vendor it will run with, within the same wait. A replaced check is never `#current` again. + if (this.#vendor() !== vendor || check !== this.#current) continue; + // Settled, so passed or failed. This check's own verdict: calling #check again could start a recheck once the + // window has passed. + return this.#verdict(check); + } + } + /** + * Throw a `GuardRefusal` unless the check for the vendor chosen now has passed (or no vendor is chosen). Requests + * call it again inside the action that starts them, so a vendor chosen after `admit` never runs unchecked. It starts + * no check there (that runs inside the Store's transaction): one it needs is started on the next turn. + */ + assertPassed(): void { + if (this.#abort.signal.aborted) throw new GuardRefusal(`${PLANNING_UNAVAILABLE} planning is stopping.`); + const vendor = this.#vendor(); + if (!vendor) return; + const covering = this.#covering(vendor); + if (covering) return this.#verdict(covering); + // Checks the vendor chosen when this runs, which Settings may have changed since the refusal. + setImmediate(() => { + // Never throws: an exception here would end the process. A Store that cannot be read starts nothing. + try { const now = this.#vendor(); if (now) this.#check(now); } catch { /* stopping, or no Store: nothing to start */ } + }); + throw new GuardRefusal(`${PLANNING_UNAVAILABLE} the planning agent chosen in Settings changed, and codeboost checks ` + + 'it next. Retry in a minute.'); + } + #verdict(check: Check): void { + // After close nothing is admitted, even by a check that passed meanwhile, and no recheck can be promised. + if (this.#abort.signal.aborted) throw new GuardRefusal(`${PLANNING_UNAVAILABLE} planning is stopping.`); + if (check.passed) return; + // The reason comes last and as it is: some end with a command a person may copy. (The Store records a refusal cut + // to 4,000 characters, so a resend of the same action may replay it shortened.) One that holds until a restart + // gets no promise of a recheck, which could not help. + if (check.failure && UNTIL_RESTART.test(ownText(messageOf(check.failure.error)))) + throw new GuardRefusal(`${PLANNING_UNAVAILABLE} codeboost's check of the planning agent failed, and only a restart ` + + `of codeboost can change that; fix the cause first if the reason names one. The check said: ${messageOf(check.failure.error)}`); + // Codex is refused whatever happens (#93): only choosing another agent helps, and the next request checks that + // one. A restart-only reason above keeps its own wording. + if (check.failure && check.vendor === 'codex') + throw new GuardRefusal(`${PLANNING_UNAVAILABLE} codeboost's check of the planning agent failed; choose another ` + + `agent in Settings, and the next planning request checks it. The check said: ${messageOf(check.failure.error)}`); + if (check.failure) throw new GuardRefusal(`${PLANNING_UNAVAILABLE} codeboost's check of the planning agent failed; ` + + `it checks again on the next request after ${duration(this.#recheckMs)}. ` + + `The check said: ${messageOf(check.failure.error)}`); + throw new GuardRefusal(STILL_CHECKING); + } + /** Cancel any running check and wait up to `waitMs` for it to stop. No check starts afterwards. */ + async close(waitMs = PLANNING_CHECK_CLOSE_MS): Promise { + this.#abort.abort(new StopError('Planning stopped.', 'shutdown')); + let timer: ReturnType | undefined; + await Promise.race([Promise.all(this.#running), new Promise(done => { timer = setTimeout(done, waitMs); })]); + clearTimeout(timer); + } +} diff --git a/runner/question-agent.ts b/runner/question-agent.ts index 94b65140..b97b930b 100644 --- a/runner/question-agent.ts +++ b/runner/question-agent.ts @@ -7,6 +7,9 @@ import type { RecoverReply, ReleaseReply, WorkerReply, WorkerRequest } from './q import { ASK_NAMING, createWorkerRoot, removeWorkerRoot, type LeftoverLedger, type WorkerNaming } from './question-leftovers.ts'; export type { Provider } from './question-container.ts'; +/** Why a feature is off after its worker was given up on: planning's check (#164) matches its wording. */ +export const abandonedWorkerText = (naming: WorkerNaming, why: string) => `The agent container worker ${why}. Its containers and storage may still exist, so ${naming.label} is off until codeboost restarts. The first ${naming.job} after the restart removes them.`; + // Leave the worker time to cancel the container and release storage before the review's own timeout fires. const SETTLE_MARGIN_MS = 5_000; // Bounds the final storage removal at shutdown; whatever remains is left for the next process's recovery. @@ -113,7 +116,7 @@ export class AgentWorker { async #abandonOnce(why: string) { const worker = this.worker; this.worker = undefined; - this.crashed ??= new Error(`The agent container worker ${why}. Its containers and storage may still exist, so ${this.naming.label} is off until codeboost restarts. The first ${this.naming.job} after the restart removes them.`); + this.crashed ??= new Error(abandonedWorkerText(this.naming, why)); for (const release of this.releases.values()) release(null); this.releases.clear(); this.#failRecoveries(this.crashed); diff --git a/runner/question-leftovers.ts b/runner/question-leftovers.ts index c3163789..f5f1ed3c 100644 --- a/runner/question-leftovers.ts +++ b/runner/question-leftovers.ts @@ -190,8 +190,9 @@ function parse(text: string, naming: WorkerNaming): LedgerRecord { /** * Durable record of a feature's host copies (worker roots) that outlived their worker, and the review's lock for that * feature. Docker leftovers need no record: the feature labels them with the review's own owner token for it, and the - * first request of each process removes them through lane D's scoped recovery (#51 item 4, #65). Ask and planning each - * have their own ledger, lock and owner, so neither one's recovery or refusals affect the other (#117). + * first job of each process that can sign in and recovers (for planning, usually its startup check, #164) removes them + * through lane D's scoped recovery (#51 item 4, #65). Ask and planning each have their own ledger, lock and owner, so + * neither one's recovery or refusals affect the other (#117). */ export class LeftoverLedger { readonly path: string; diff --git a/scripts/live-probes.ts b/scripts/live-probes.ts index 9547a76a..a2929270 100644 --- a/scripts/live-probes.ts +++ b/scripts/live-probes.ts @@ -24,6 +24,8 @@ export const LIVE_PROBES = [ { file: 'test/agent-supervisor.test.ts', name: 'container invocation supervisor returns a schema-constrained Claude planning answer as bare JSON from the ' + 'mounted schema (live Claude)' }, + { file: 'test/agent-planning.test.ts', + name: 'planning in the agent container passes the planning check with a real sign-in (live Claude)' }, ] as const; export const LIVE_PROBE_FILES = [...new Set(LIVE_PROBES.map(probe => probe.file))]; diff --git a/test/agent-planning.test.ts b/test/agent-planning.test.ts index 6fb0fe1e..a0b1901c 100644 --- a/test/agent-planning.test.ts +++ b/test/agent-planning.test.ts @@ -43,4 +43,33 @@ describe('planning in the agent container', () => { expect(docker(kind, 'ls', ...(kind === 'container' ? ['-a'] : []), '--quiet', '--filter', `label=io.codeboost.runner=${owner}`).stdout.trim(), kind).toBe(''); expect(existsSync(`${realpathSync(service.config.database)}.planning-leftovers.json`)).toBe(false); }, 11 * 60_000); + + it('fails the planning check when the vendor refuses the sign-in, and leaves nothing (#164)', async () => { + const root = mkdtempSync(join(tmpdir(), 'agent-planning-')); roots.push(root); + const service = new ReviewService(createDemo(join(root, 'demo'))); services.push(service); + owners.push(service.store.planningOwnerToken(statSync(realpathSync(service.config.database), { bigint: true }))); + const agent = new PlanningAgent(service, { env: { CLAUDE_CODE_OAUTH_TOKEN: 'codeboost-invalid-test-token' } }); + try { + // The check is a real planning invocation: only one that reached Anthropic comes back with its 401. + await expect(agent.probe('claude', new AbortController().signal)).rejects.toThrow(/Claude could not write the plan.*(401|authenticate)/); + } finally { await agent.close(); } + const owner = owners.at(-1)!; + for (const kind of ['container', 'volume', 'network']) + expect(docker(kind, 'ls', ...(kind === 'container' ? ['-a'] : []), '--quiet', '--filter', `label=io.codeboost.runner=${owner}`).stdout.trim(), kind).toBe(''); + expect(existsSync(`${realpathSync(service.config.database)}.planning-leftovers.json`)).toBe(false); + }, 11 * 60_000); + + // #164: the check's own prompt and schema, answered by the real Claude service. Runs in the Live probes workflow. + it.runIf(process.env.CODEBOOST_RUN_AUTH_PROBES === '1')('passes the planning check with a real sign-in (live Claude)', async () => { + const root = mkdtempSync(join(tmpdir(), 'agent-planning-')); roots.push(root); + const service = new ReviewService(createDemo(join(root, 'demo'))); services.push(service); + owners.push(service.store.planningOwnerToken(statSync(realpathSync(service.config.database), { bigint: true }))); + const agent = new PlanningAgent(service, { env: { CLAUDE_CODE_OAUTH_TOKEN: process.env.CLAUDE_CODE_OAUTH_TOKEN } }); + try { await expect(agent.probe('claude', new AbortController().signal)).resolves.toBeUndefined(); } + finally { await agent.close(); } + const owner = owners.at(-1)!; + for (const kind of ['container', 'volume', 'network']) + expect(docker(kind, 'ls', ...(kind === 'container' ? ['-a'] : []), '--quiet', '--filter', `label=io.codeboost.runner=${owner}`).stdout.trim(), kind).toBe(''); + expect(existsSync(`${realpathSync(service.config.database)}.planning-leftovers.json`)).toBe(false); + }, 11 * 60_000); }); diff --git a/test/browser/plans-production.spec.ts b/test/browser/plans-production.spec.ts index 4d2cf4e0..d8502957 100644 --- a/test/browser/plans-production.spec.ts +++ b/test/browser/plans-production.spec.ts @@ -91,7 +91,7 @@ function dBackedAgent(service: ReviewService, observations: { const snapshot = service.store.getSnapshot(service.config.identity); const provider = createPlanningProvider({ vendor: 'claude', repository: service.reviewRepository().path, head: snapshot.head, snapshotId: snapshot.id, runnerOwner: '0123456789abcdef0123456789abcdef', deps }); - const agent: AuthorProvider & { close(): Promise } = { + const agent: AuthorProvider & Pick = { async invoke(request: AuthorRequest, signal: AbortSignal) { const current = service.store.getPlan(request.identity); const expected = request.mode === 'draft' ? current.revision + 1 : current.revision; @@ -100,6 +100,9 @@ function dBackedAgent(service: ReviewService, observations: { return provider.invoke(request, signal); }, async close() { /* createPlanningProvider has no process-level owner */ }, + // No vendor is chosen in this spec, so the planning check (#164) checks nothing and never calls this; + // planning-production and agent-planning test the check. + async probe() { /* passes */ }, }; return agent as PlanningAgent; } diff --git a/test/fixtures/question-worker-stub.ts b/test/fixtures/question-worker-stub.ts index 386f27d6..2325f1e0 100644 --- a/test/fixtures/question-worker-stub.ts +++ b/test/fixtures/question-worker-stub.ts @@ -41,6 +41,17 @@ parentPort!.on('message', (message: WorkerRequest) => { // A planning job carries a ReadOnlyRun; the stub reads the same fields from either kind (#117). const job = message.type === 'ask' ? message.question : { ...message.run, noteId: message.run.taskId }; const { prompt, provider, noteId, attemptId } = job; + // The planning check (#164) passes with the schema's own answer, given at once and compact, as the real worker returns + // Claude CLI 2.1.281's structured_output (live probe run 39541887939). The stub leaves out cancellation and the + // worker's feature check: it never leaves a check pending, and plan() refuses a non-planning worker first. + if (message.type === 'plan' && message.run.taskId.startsWith('planning-probe-')) { + // The agent answers the check only when asked for it: the prompt names the answer and the schema requires it. + const asked = message.run.prompt.includes('{"ok": true}') && message.run.schemaText.includes('"ok":{"const":true}'); + // With this token the agent answers something else, which must fail the check. + const wrong = !asked || workerData?.credentials?.CLAUDE_CODE_OAUTH_TOKEN === 'probe-wrong-answer'; + parentPort!.postMessage({ id: message.id, attemptId, ok: true, text: wrong ? '{"ok":false}' : '{"ok":true}' }); + return; + } if (prompt === 'crash') throw new Error('stub crashed'); if (prompt === 'leak') { remaining++; diff --git a/test/planning-production.test.ts b/test/planning-production.test.ts index eec89019..b165cdb5 100644 --- a/test/planning-production.test.ts +++ b/test/planning-production.test.ts @@ -1,18 +1,22 @@ import { randomUUID } from 'node:crypto'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { linkSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, expect, it, vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; import { ISSUE_READ_ACTIVE_MS, type IssueAccess, type IssueText } from '../github/issues.ts'; import { createDemo } from '../scripts/demo.ts'; -import type { PlanningAgent } from '../runner/planning.ts'; import { ReviewService, type ReviewConfig } from '../runner/review.ts'; import { productionPlanning } from '../web/planning.ts'; import { DatabaseSync } from 'node:sqlite'; -import { PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; +import { CODEX_PLANNING_REFUSED, PLANNING_BUDGET_MS, PLANNING_FEATURE, PLANNING_LEFTOVERS, planningCredential, + planningOutput } from '../runner/planning-provider.ts'; +import type { InvocationInput } from '../agents/contract.ts'; import { PLANNING_SHUTDOWN_GRACE_MS, startServer, type PlanningDeps } from '../web/server.ts'; import { GuardRefusal } from '../runner/lifecycle.ts'; +import { PLANNING_UNAVAILABLE, PlanningCheck, type PlanningAgent } from '../runner/planning.ts'; +import { LeftoverLedger, PLANNING_NAMING } from '../runner/question-leftovers.ts'; +import { abandonedWorkerText } from '../runner/question-agent.ts'; // Production planning wiring (#117): when it is on, what each request is told, and how the server awaits the issue. vi.setConfig({ testTimeout: 20_000 }); @@ -34,7 +38,9 @@ const text = (number: number): IssueText => ({ number, title: 'Retries ignore th const issues = (issueText: (number: number, options?: { trustedAuthor?: string | null; expectedAccess?: IssueAccess }) => Promise, collaborator = true) => ({ issueAccess: async (number: number) => ({ number, authorLogin: 'outside', collaborator }), issueText, }); -const closable = (close = vi.fn(async () => undefined)) => ({ close, invoke: vi.fn() }) as unknown as PlanningAgent; +const closable = (close = vi.fn(async () => undefined), + probe: (vendor: string, signal: AbortSignal) => Promise = vi.fn(async () => undefined)) => + ({ close, invoke: vi.fn(), probe }) as unknown as PlanningAgent; const verified = { verifyLock: () => undefined }; it('is off in a demo and without a github block, so neither plans', () => { @@ -310,3 +316,529 @@ it('ends a planning request that does not settle after shutdown aborts it by clo expect(order).toEqual(['planning closed', 'server closed']); } finally { vi.useRealTimers(); } }); + +// #164: the planning check, through productionPlanning's setup with a stand-in agent whose `probe` is the check. +describe('the planning check (#164)', () => { + /** The production setup on a review whose Settings chose `vendor`, with a stand-in agent and issue reads. */ + function checked(probe: (vendor: string, signal: AbortSignal) => Promise, vendor: 'claude' | null = 'claude', + recheckMs?: number, admitWaitMs?: number, issueText = vi.fn(async (number: number) => text(number))) { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + if (vendor) service.store.setQuestionProvider(vendor); + const issueAccess = vi.fn(async (number: number) => ({ number, authorLogin: 'outside', collaborator: true })); + const close = vi.fn(async () => undefined), agent = closable(close, vi.fn(probe)); + const deps = productionPlanning(config, { ...verified, issues: { issueAccess, issueText }, agent: () => agent, recheckMs, + admitWaitMs })!(service); + return { deps, service, agent, issueAccess, close, probe: vi.mocked(agent.probe) }; + } + const signal = () => new AbortController().signal; + const thrown = (run: () => unknown) => { try { run(); } catch (error) { return (error as Error).message; } throw new Error('It did not throw.'); }; + // A real sign-in failure, as planningOutput words it for a failed run. Tests that only show a reason passed through + // whole use any text ('Docker is not running.', 'No worker could start.'). Codex's refusal is CODEX_PLANNING_REFUSED, + // which planningCredential throws as it is. + const runContext = { snapshotId: 's', planId: 'p', planRevision: 1, assignmentId: 'a', referencedCodeHash: 'c', stateVersion: 1 }; + const failedRun = (stdout: string) => thrown(() => planningOutput({ attemptId: 'probe', context: runContext, exitCode: 1, + signal: null, stderr: '', stdout }, { attemptId: 'probe', context: runContext } as InvocationInput)); + const signInFailure = failedRun('Invalid API key · Please run /login'); + + it('starts at setup with the vendor chosen in Settings, and that one check serves every later request', async () => { + const { deps, probe, issueAccess } = checked(async () => undefined); + expect(probe).toHaveBeenCalledTimes(1); + expect(probe.mock.calls[0]![0]).toBe('claude'); + await deps.describe(signal()); + await deps.describe(signal()); + expect(probe).toHaveBeenCalledTimes(1); + expect(issueAccess).toHaveBeenCalledTimes(2); + }); + + it('refuses a request with the check\'s reason, before it reads the issue, and does not check again within the recheck window', async () => { + const { deps, probe, issueAccess } = checked(async () => { throw new Error(signInFailure); }); + const refusal = await Promise.resolve(deps.describe(signal())).catch((error: unknown) => error); + expect(refusal).toBeInstanceOf(GuardRefusal); + expect((refusal as Error).message.startsWith(`${PLANNING_UNAVAILABLE} `)).toBe(true); + expect((refusal as Error).message.endsWith(`The check said: ${signInFailure}`)).toBe(true); + // Inside the recheck window, the same failure refuses at once. + await expect(deps.describe(signal())).rejects.toThrow(signInFailure); + expect(probe).toHaveBeenCalledTimes(1); + expect(issueAccess).not.toHaveBeenCalled(); + }); + + it('checks again once the recheck window after a failure is over, and admits once a check passes', async () => { + let fail = true; + const { deps, probe, issueAccess } = checked(async () => { if (fail) throw new Error('Docker is not running.'); }, 'claude', 0); + await expect(deps.describe(signal())).rejects.toThrow('Docker is not running.'); + fail = false; + await expect(deps.describe(signal())).resolves.toMatchObject({ issue: { title: 'Retries ignore the cap' } }); + expect(probe).toHaveBeenCalledTimes(2); + // A passed check is kept: the wait applies only to a failure. + fail = true; + await deps.describe(signal()); + expect(probe).toHaveBeenCalledTimes(2); + expect(issueAccess).toHaveBeenCalledTimes(2); + }); + + it('makes every request wait for the one running check, and a request that stops waiting leaves it running', async () => { + let pass!: () => void; + const { deps, probe } = checked((_vendor, probeSignal) => new Promise((resolve, reject) => { + pass = resolve; probeSignal.addEventListener('abort', () => reject(probeSignal.reason), { once: true }); + })); + const leaving = new AbortController(), left = deps.describe(leaving.signal), waiting = deps.describe(signal()); + leaving.abort(new Error('The browser went away.')); + // A refusal about the wait, never one the server would record as a failed GitHub read. + const stopped = await Promise.resolve(left).catch((error: unknown) => error); + expect(stopped).toBeInstanceOf(GuardRefusal); + expect((stopped as Error).message).toBe(`${PLANNING_UNAVAILABLE} codeboost stopped waiting for the planning check: The browser went away.`); + pass(); + await expect(waiting).resolves.toMatchObject({ issue: { title: 'Retries ignore the cap' } }); + expect(probe).toHaveBeenCalledTimes(1); + expect(probe.mock.calls[0]![1].aborted).toBe(false); + }); + + it('checks nothing while no agent is chosen, then checks the vendor chosen before its first request', async () => { + // As PlanningAgent.probe answers: Claude's check passes here, and Codex's always fails (planningCredential). + const { deps, service, probe, issueAccess } = checked(async vendor => { + if (vendor === 'codex') throw new Error(CODEX_PLANNING_REFUSED); + }, null); + expect(probe).not.toHaveBeenCalled(); + // No vendor: the request goes on, and fails later with NO_PLANNING_AGENT, as before. + await deps.describe(signal()); + expect(probe).not.toHaveBeenCalled(); + service.store.setQuestionProvider('claude'); + await deps.describe(signal()); + // Settings refuse Codex now, but a database from before #75 may still hold it. + vi.spyOn(service.store, 'questionProvider').mockReturnValue('codex'); + await expect(deps.describe(signal())).rejects.toThrow(CODEX_PLANNING_REFUSED); + expect(probe.mock.calls.map(call => call[0])).toEqual(['claude', 'codex']); + expect(issueAccess).toHaveBeenCalledTimes(2); + }); + + it('checks the vendor chosen while a request waited, before it admits the request', async () => { + let pass!: () => void; + const { deps, service, probe } = checked(vendor => vendor === 'claude' ? new Promise(resolve => { pass = resolve; }) + : Promise.reject(new Error(CODEX_PLANNING_REFUSED))); + const waiting = Promise.resolve(deps.describe(signal())); + vi.spyOn(service.store, 'questionProvider').mockReturnValue('codex'); + pass(); + await expect(waiting).rejects.toThrow(CODEX_PLANNING_REFUSED); + expect(probe.mock.calls.map(call => call[0])).toEqual(['claude', 'codex']); + }); + + it('refuses to invoke an agent whose check has not passed for the vendor chosen at the call', async () => { + const made = checked(async () => undefined); + await made.deps.describe(signal()); + const request = {} as Parameters[0], forwarded = signal(); + // Once the check passed, the call goes to the agent as it is. + await made.deps.provider.invoke(request, forwarded); + expect(made.agent.invoke).toHaveBeenCalledWith(request, forwarded); + vi.mocked(made.agent.invoke).mockClear(); + vi.spyOn(made.service.store, 'questionProvider').mockReturnValue('codex'); + // A rejection, as `AuthorProvider.invoke` promises: E3 then settles the request as failed with this reason + // (test/planning-suggestions.test.ts covers a provider whose invoke fails). + await expect(made.deps.provider.invoke(request, signal())).rejects.toThrow('and codeboost checks it next'); + expect(made.agent.invoke).not.toHaveBeenCalled(); + }); + + it('cancels a running check on close, and closes the agent only once it stopped', async () => { + const order: string[] = []; + // The check settles a while after it is cancelled, as a worker job does. + const { deps, probe, close } = checked((_vendor, probeSignal) => new Promise((_, reject) => + probeSignal.addEventListener('abort', () => setTimeout(() => { order.push('check stopped'); reject(probeSignal.reason); }, 50), + { once: true }))); + close.mockImplementation(async () => { order.push('agent closed'); }); + await deps.close!(); + expect(probe.mock.calls[0]![1].aborted).toBe(true); + expect(order).toEqual(['check stopped', 'agent closed']); + }); + + it('fails the check, and refuses with its reason, when the probe throws instead of rejecting', async () => { + const check = new PlanningCheck(() => { throw new Error('No worker could start.'); }, () => 'claude'); + expect(() => check.start()).not.toThrow(); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect(refusal).toBeInstanceOf(GuardRefusal); + expect((refusal as Error).message).toContain('No worker could start.'); + }); + + it('cancels a running check that a check for another vendor replaced', async () => { + let vendor: 'claude' | 'codex' = 'claude'; + const signals: AbortSignal[] = []; + // Claude's check runs until cancelled; Codex's fails at once, as PlanningAgent.probe's does. + const check = new PlanningCheck((probed, probeSignal) => { + signals.push(probeSignal); + return probed === 'codex' ? Promise.reject(new Error(CODEX_PLANNING_REFUSED)) : new Promise(() => undefined); + }, () => vendor, { waitMs: 10 }); + check.start(); + vendor = 'codex'; + await expect(check.admit(signal())).rejects.toThrow('choose another agent in Settings'); + expect(signals.map(probeSignal => probeSignal.aborted)).toEqual([true, false]); + await check.close(10); + }); + + it('keeps a multi-line reason whole, at the end of the refusal', async () => { + // LeftoverLedger.assertClear's text for host copies it could not delete. + const reason = `${PLANNING_NAMING.label} is off: host copies of ${PLANNING_NAMING.code} or credentials from an earlier session could not be deleted. Delete them, then retry:\nrm -rf '/tmp/codeboost-plan-x'`; + const check = new PlanningCheck(async () => { throw new Error(reason); }, () => 'claude'); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect((refusal as Error).message.endsWith(`The check said: ${reason}`)).toBe(true); + }); + + it('keeps waiting, rather than failing, when its check was replaced and the vendor changed back', async () => { + let vendor: 'claude' | 'codex' = 'claude', count = 0; + const check = new PlanningCheck((_probed, probeSignal) => { + count += 1; + // The third check (Claude again) passes; the first two wait until cancelled. + if (count === 3) return Promise.resolve(); + return new Promise((_, reject) => probeSignal.addEventListener('abort', () => reject(probeSignal.reason), { once: true })); + }, () => vendor); + const waiting = check.admit(signal()); + vendor = 'codex'; + const other = check.admit(signal()).catch(() => undefined); + vendor = 'claude'; + await expect(Promise.all([waiting, check.admit(signal())])).resolves.toEqual([undefined, undefined]); + await other; + expect(count).toBe(3); + }); + + it('moves a waiting request to a newer check at once, without waiting for the replaced one to stop', async () => { + let vendor: 'claude' | 'codex' = 'claude', count = 0; + // The first check ignores its cancellation, as a worker job stuck in a synchronous build does; the third passes. + const check = new PlanningCheck(() => { count += 1; return count === 3 ? Promise.resolve() : new Promise(() => undefined); }, + () => vendor, { waitMs: 5_000 }); + const started = performance.now(), waiting = check.admit(signal()); + vendor = 'codex'; + void check.admit(signal()).catch(() => undefined); + vendor = 'claude'; + void check.admit(signal()).catch(() => undefined); + await expect(waiting).resolves.toBeUndefined(); + expect(performance.now() - started).toBeLessThan(2_000); + await check.close(10); + }); + + it('never throws from the deferred check when the Store cannot be read', async () => { + let fail = false; + const probe = vi.fn(async () => undefined); + const check = new PlanningCheck(probe, () => { if (fail) throw new Error('database is closed'); return 'claude'; }); + const uncaught = vi.fn(); + process.on('uncaughtException', uncaught); + try { + expect(() => check.assertPassed()).toThrow('and codeboost checks it next'); + fail = true; + await new Promise(resolve => setImmediate(resolve)); + expect(uncaught).not.toHaveBeenCalled(); + expect(probe).not.toHaveBeenCalled(); + } finally { process.off('uncaughtException', uncaught); } + }); + + it('lets many requests wait on one check without a listener warning', async () => { + let pass!: () => void; + const check = new PlanningCheck(() => new Promise(resolve => { pass = resolve; }), () => 'claude'); + const warnings: string[] = [], warned = (warning: Error) => warnings.push(warning.name); + process.on('warning', warned); + try { + const waiting = Array.from({ length: 20 }, () => check.admit(signal())); + await new Promise(resolve => setTimeout(resolve, 10)); + pass(); + await Promise.all(waiting); + await new Promise(resolve => setImmediate(resolve)); + expect(warnings).not.toContain('MaxListenersExceededWarning'); + } finally { process.off('warning', warned); } + }); + + it('waits 30 s for a check, fails one after the planning budget and a minute, and waits 20 s at close', async () => { + // productionPlanning's own defaults, as the docs give them: nothing here overrides them. The clock is faked too, so each wait is exact. + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'performance'] }); + try { + // The probe never settles and ignores its abort, as a job stuck in the worker would. + const { deps } = checked(() => new Promise(() => undefined)); + let outcome: unknown; + void Promise.resolve(deps.describe(signal())).catch((error: unknown) => { outcome = error; }); + await vi.advanceTimersByTimeAsync(30_000 - 1); + expect(outcome).toBeUndefined(); + await vi.advanceTimersByTimeAsync(1); + expect(String(outcome)).toContain('still checking'); + // The check itself fails once the planning budget and a minute have passed since it started. + // A request waiting from 15 s before then gets that failure, not the end of its own wait. + await vi.advanceTimersByTimeAsync(11 * 60_000 - 30_000 - 15_000); + outcome = undefined; + void Promise.resolve(deps.describe(signal())).catch((error: unknown) => { outcome = error; }); + await vi.advanceTimersByTimeAsync(15_000 - 1); + expect(outcome).toBeUndefined(); + await vi.advanceTimersByTimeAsync(1); + expect(String(outcome)).toContain('did not finish within 11 minutes'); + // Close waits 20 s for the timed-out check's probe, which never stops. + let closed = false; + void deps.close!().then(() => { closed = true; }); + await vi.advanceTimersByTimeAsync(20_000 - 1); + expect(closed).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(closed).toBe(true); + } finally { vi.useRealTimers(); } + }); + + it('lets a request go on when Settings clear the agent while it waits, even once the wait has passed', async () => { + let vendor: 'claude' | null = 'claude'; + const check = new PlanningCheck((_vendor, probeSignal) => new Promise(resolve => + probeSignal.addEventListener('abort', () => resolve(), { once: true })), () => vendor, { waitMs: 50 }); + const waiting = check.admit(signal()); + vendor = null; + // No check replaces Claude's, so only the wait's end sees the change. + await expect(waiting).resolves.toBeUndefined(); + await check.close(2_000); + }); + + it('fails and cancels a check that does not settle in time, and checks again after the recheck window', async () => { + let count = 0, late!: () => void; + const signals: AbortSignal[] = []; + const check = new PlanningCheck((_vendor, probeSignal) => { + count += 1; signals.push(probeSignal); + return count === 1 ? new Promise(resolve => { late = resolve; }) : Promise.resolve(); + }, () => 'claude', { timeoutMs: 50, recheckMs: 2_000, waitMs: 1_000 }); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect((refusal as Error).message).toContain('The planning check did not finish within a moment.'); + expect(signals[0]!.aborted).toBe(true); + expect((signals[0]!.reason as Error).message).toContain('did not finish'); + // A probe that answers after its timeout does not pass the check: within the wait it still refuses. + late(); + await new Promise(resolve => setTimeout(resolve, 10)); + await expect(check.admit(signal())).rejects.toThrow('did not finish'); + expect(count).toBe(1); + await new Promise(resolve => setTimeout(resolve, 2_100)); + await expect(check.admit(signal())).resolves.toBeUndefined(); + expect(count).toBe(2); + }); + + it('keeps the recheck promise when a run\'s cleanup did not settle, which the next run retries', async () => { + // planning-provider.ts' cleanupFailed: the run's own failure, then the cleanup's. + const reason = PLANNING_FEATURE.cleanupFailed([new Error('docker volume rm failed')], { error: new Error('Planning agent timed out.') }).message; + const check = new PlanningCheck(async () => { throw new Error(reason); }, () => 'claude'); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect((refusal as Error).message).toContain('it checks again on the next request after a minute'); + }); + + it('keeps the recheck promise when only the vendor\'s output mentions a restart', async () => { + // planningOutput's text for a failed run whose output says so. + const reason = failedRun('planning is off until codeboost restarts'); + expect(reason).toContain(' Claude said: planning is off until codeboost restarts'); + const check = new PlanningCheck(async () => { throw new Error(reason); }, () => 'claude'); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect((refusal as Error).message).toContain('it checks again on the next request after a minute'); + }); + + it('reports a Store it cannot read, once a wait has begun, as an unavailable check, not a stopped wait', async () => { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + service.store.setQuestionProvider('claude'); + let pass!: () => void; + const deps = productionPlanning(config, { ...verified, issues: issues(async number => text(number)), + agent: () => closable(undefined, (_vendor, probeSignal) => new Promise((resolve, reject) => { + pass = resolve; probeSignal.addEventListener('abort', () => reject(probeSignal.reason), { once: true }); + })) })!(service); + const waiting = Promise.resolve(deps.describe(signal())).catch((error: unknown) => error); + // The read after the wait fails: the database went bad while the check ran. + await new Promise(resolve => setTimeout(resolve, 10)); + vi.spyOn(service.store, 'questionProvider').mockImplementation(() => { throw new Error('database disk image is malformed'); }); + pass(); + const refusal = await waiting; + expect(refusal).toBeInstanceOf(GuardRefusal); + expect((refusal as Error).message).toBe(`${PLANNING_UNAVAILABLE} codeboost could not check the planning agent: database disk image is malformed`); + vi.mocked(service.store.questionProvider).mockRestore(); + await deps.close!(); + }); + + it('refuses with "planning is stopping" after close, never with a recheck promise', async () => { + const check = new PlanningCheck((_vendor, probeSignal) => new Promise((_, reject) => + probeSignal.addEventListener('abort', () => reject(probeSignal.reason), { once: true })), () => 'claude'); + check.start(); + await check.close(); + expect(() => check.assertPassed()).toThrow(`${PLANNING_UNAVAILABLE} planning is stopping.`); + await expect(check.admit(signal())).rejects.toThrow(`${PLANNING_UNAVAILABLE} planning is stopping.`); + }); + + it('tells Codex to choose another agent, not to wait for a recheck', async () => { + const check = new PlanningCheck(async () => { throw new Error(CODEX_PLANNING_REFUSED); }, () => 'codex'); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect((refusal as Error).message).toBe(`${PLANNING_UNAVAILABLE} codeboost's check of the planning agent failed; choose another agent in Settings, and the next planning request checks it. The check said: ${CODEX_PLANNING_REFUSED}`); + }); + + it('waits on close for a timed-out check\'s probe to stop, within its bound', async () => { + const order: string[] = []; + const check = new PlanningCheck((_vendor, probeSignal) => new Promise((_, reject) => { + // The probe stops a while after the timeout cancels it, as a worker job busy in setup does. + probeSignal.addEventListener('abort', () => + setTimeout(() => { order.push('probe stopped'); reject(probeSignal.reason); }, 100), { once: true }); + }), () => 'claude', { timeoutMs: 20 }); + await expect(check.admit(signal())).rejects.toThrow('did not finish'); + await check.close(2_000); + order.push('closed'); + expect(order).toEqual(['probe stopped', 'closed']); + }); + + // Each reason as the code that gives it words it, so a reworded message that no longer matches fails here. + const hardLinked = () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-linked-')); closers.push(() => rmSync(root, { recursive: true, force: true })); + writeFileSync(join(root, 'review.db'), ''); linkSync(join(root, 'review.db'), join(root, 'other.db')); + const ledger = LeftoverLedger.forDatabase(join(root, 'review.db'), PLANNING_NAMING); + return thrown(() => { ledger.acquire(); ledger.release(); }); + }; + it.each([ + ['the sign-in is missing', () => thrown(() => planningCredential('claude', {}))], + ['the worker was given up on', () => abandonedWorkerText(PLANNING_NAMING, 'stopped (exit code 1)')], + ['cleanup could not be confirmed', () => PLANNING_LEFTOVERS.untracked()], + ['the database file has other hard links', hardLinked], + ])('promises no recheck when %s, which only a restart can change', async (_label, reasonOf) => { + const reason = reasonOf(); + const check = new PlanningCheck(async () => { throw new Error(reason); }, () => 'claude'); + const refusal = await check.admit(signal()).catch((error: unknown) => error); + expect((refusal as Error).message).toBe(`${PLANNING_UNAVAILABLE} codeboost's check of the planning agent failed, and only a restart of codeboost can change that; fix the cause first if the reason names one. The check said: ${reason}`); + }); + + it('starts no check after close, not even one deferred before it', async () => { + let vendor: 'claude' | 'codex' = 'claude'; + const probe = vi.fn(async (_probed: string) => undefined), check = new PlanningCheck(probe, () => vendor); + check.start(); + await new Promise(resolve => setImmediate(resolve)); + // A changed vendor defers its check to the next turn; close comes first. + vendor = 'codex'; + expect(() => check.assertPassed()).toThrow('and codeboost checks it next'); + await check.close(); + await new Promise(resolve => setImmediate(resolve)); + expect(() => check.start()).toThrow(`${PLANNING_UNAVAILABLE} planning is stopping.`); + await expect(check.admit(signal())).rejects.toThrow(`${PLANNING_UNAVAILABLE} planning is stopping.`); + expect(() => check.assertPassed()).toThrow(`${PLANNING_UNAVAILABLE} planning is stopping.`); + await new Promise(resolve => setImmediate(resolve)); + expect(probe.mock.calls.map(call => call[0])).toEqual(['claude']); + }); + + it('wakes a request waiting at close at once, though its check\'s probe never stops', async () => { + const check = new PlanningCheck(() => new Promise(() => undefined), () => 'claude'); + const started = performance.now(); + const waiting = check.admit(signal()).catch((error: unknown) => error); + await new Promise(resolve => setImmediate(resolve)); + void check.close(10); + const refusal = await waiting; + expect((refusal as Error).message).toBe(`${PLANNING_UNAVAILABLE} planning is stopping.`); + expect(performance.now() - started).toBeLessThan(5_000); + }); + + it('tells a request waiting at close that planning is stopping, even if the check then passes', async () => { + let pass!: () => void; + // The probe answers after close cancelled it, as an answer already on its way would. + const check = new PlanningCheck(() => new Promise(resolve => { pass = resolve; }), () => 'claude'); + const waiting = check.admit(signal()).catch((error: unknown) => error); + await new Promise(resolve => setImmediate(resolve)); + const closed = check.close(2_000); + pass(); + await closed; + const refusal = await waiting; + expect(refusal).toBeInstanceOf(GuardRefusal); + expect((refusal as Error).message).toBe(`${PLANNING_UNAVAILABLE} planning is stopping.`); + }); + + it.each([['under a check that passed', 'claude'], ['with no agent chosen', null]] as const)( + 'admits nothing after close, %s', async (_label, vendor) => { + const made = checked(async () => undefined, vendor); + await made.deps.describe(signal()); + await made.deps.close!(); + const stopping = `${PLANNING_UNAVAILABLE} planning is stopping.`; + await expect(Promise.resolve(made.deps.describe(signal()))).rejects.toThrow(stopping); + await expect(made.deps.provider.invoke({} as Parameters[0], signal())).rejects.toThrow(stopping); + expect(made.agent.invoke).not.toHaveBeenCalled(); + expect(made.issueAccess).toHaveBeenCalledTimes(1); + }); + + it('bounds a request\'s whole wait, across a vendor change, by one deadline', async () => { + let vendor: 'claude' | 'codex' = 'claude'; + // Claude's check passes at 3 s, after the vendor changed; Codex's never ends. + const check = new PlanningCheck(probed => new Promise(resolve => { + if (probed === 'claude') setTimeout(resolve, 3_000); + }), () => vendor, { waitMs: 3_200 }); + const started = performance.now(), waiting = check.admit(signal()).catch((error: unknown) => error); + await new Promise(resolve => setTimeout(resolve, 100)); + vendor = 'codex'; + // One deadline ends the wait at 3.2 s; a fresh 3.2 s for Codex would end it at 6.2 s. + const refusal = await waiting; + expect(refusal).toBeInstanceOf(GuardRefusal); + expect((refusal as Error).message).toContain('still checking'); + expect(performance.now() - started).toBeLessThan(5_000); + await check.close(10); + }); + + it('waits a bounded time on close for a check that never stops', async () => { + const check = new PlanningCheck(() => new Promise(() => undefined), () => 'claude'); + check.start(); + const started = performance.now(); + await check.close(100); + expect(performance.now() - started).toBeLessThan(2_000); + }); + + it('refuses a request as still checking when the check outlasts its wait, and admits it once the check passed', async () => { + let pass!: () => void; + const { deps, issueAccess } = checked(() => new Promise(resolve => { pass = resolve; }), 'claude', undefined, 50); + const refusal = await Promise.resolve(deps.describe(signal())).catch((error: unknown) => error); + expect(refusal).toBeInstanceOf(GuardRefusal); + expect((refusal as Error).message).toMatch(new RegExp(`^${PLANNING_UNAVAILABLE} codeboost is still checking the planning agent\\. .* Retry in a minute\\.$`)); + expect(issueAccess).not.toHaveBeenCalled(); + pass(); + await expect(deps.describe(signal())).resolves.toMatchObject({ issue: { title: 'Retries ignore the cap' } }); + }); + + it('refuses at the start a vendor chosen while the issue was read, and checks it', async () => { + let service!: ReviewService; + // Settings take Claude while the issue is read, after a request with none chosen was admitted unchecked. + const issueText = vi.fn(async (number: number) => { + service.store.setQuestionProvider('claude'); + return text(number); + }); + // (The provider, too, checks again when it is invoked: "refuses to invoke an agent whose check has not passed …".) + const made = checked(async () => undefined, null, undefined, undefined, issueText); + service = made.service; + const described = await made.deps.describe(signal()); + expect(() => described.validate()).toThrow(`${PLANNING_UNAVAILABLE} the planning agent chosen in Settings changed, and codeboost checks it next.`); + // validate runs inside the Store's transaction, so the new vendor's check starts only on the next turn. + expect(made.probe).not.toHaveBeenCalled(); + await new Promise(resolve => setImmediate(resolve)); + expect(made.probe.mock.calls.map(call => call[0])).toEqual(['claude']); + // Once that check passed, the same request is admitted. + await new Promise(resolve => setTimeout(resolve, 0)); + expect(() => described.validate()).not.toThrow(); + }); + + it('refuses at the start while the check for a vendor chosen during the issue read still runs', async () => { + let service!: ReviewService, pass!: () => void; + const issueText = vi.fn(async (number: number) => { + service.store.setQuestionProvider('claude'); + return text(number); + }); + const made = checked(() => new Promise(resolve => { pass = resolve; }), null, undefined, undefined, issueText); + service = made.service; + const described = await made.deps.describe(signal()); + expect(() => described.validate()).toThrow('and codeboost checks it next'); + await new Promise(resolve => setImmediate(resolve)); + expect(made.probe.mock.calls.map(call => call[0])).toEqual(['claude']); + expect(() => described.validate()).toThrow(`${PLANNING_UNAVAILABLE} codeboost is still checking the planning agent.`); + pass(); + await new Promise(resolve => setTimeout(resolve, 0)); + expect(() => described.validate()).not.toThrow(); + }); + + it('refuses a start through the server with 409, recording no request and starting no agent', async () => { + const config = production(), probe = vi.fn(async () => { throw new Error(signInFailure); }); + const service = new ReviewService(config); service.store.setQuestionProvider('claude'); service.close(); + const agent = closable(undefined, probe), issueAccess = vi.fn(); + const setup = productionPlanning(config, { ...verified, issues: { issueAccess, issueText: vi.fn() }, agent: () => agent })!; + const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, setup); + closers.push(() => app.close()); + const api = async (method: string, path: string, body?: unknown) => { + const response = await fetch(`${new URL(app.url).origin}${path}`, { method, headers: { 'x-codeboost-token': app.token, + ...(body ? { 'content-type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined }); + return { status: response.status, body: await response.json() as Record }; + }; + const view = (await api('GET', '/api/review')).body; + const started = await api('POST', '/api/plan/suggestions', { expectedRevision: view.plan.revision, + snapshotId: view.snapshot.id, feedback: '', actionId: randomUUID() }); + expect(started.status).toBe(409); + expect(started.body.error.startsWith(`${PLANNING_UNAVAILABLE} `)).toBe(true); + expect(started.body.error).toContain(signInFailure); + const db = new DatabaseSync(config.database); + try { expect((db.prepare('SELECT COUNT(*) AS n FROM requests').get() as { n: number }).n).toBe(0); } finally { db.close(); } + expect(agent.invoke).not.toHaveBeenCalled(); + expect(issueAccess).not.toHaveBeenCalled(); + }); +}); diff --git a/test/planning-worker.test.ts b/test/planning-worker.test.ts index 1cb6b9e0..a473b014 100644 --- a/test/planning-worker.test.ts +++ b/test/planning-worker.test.ts @@ -9,7 +9,7 @@ import { createDemo } from '../scripts/demo.ts'; import { AgentWorker } from '../runner/question-agent.ts'; import { ASK_NAMING, createWorkerRoot, LeftoverLedger, PLANNING_NAMING } from '../runner/question-leftovers.ts'; import { CODEX_PLANNING_REFUSED, PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; -import { NO_PLANNING_AGENT, PlanningAgent } from '../runner/planning.ts'; +import { isPlanningProbeAnswer, NO_PLANNING_AGENT, PLANNING_PROBE_PROMPT, PLANNING_PROBE_SCHEMA, PlanningAgent } from '../runner/planning.ts'; import { ReviewService } from '../runner/review.ts'; // Planning's worker, ledger and owner (#117), driven through the stub worker, which reports what reached it. @@ -155,3 +155,43 @@ it.each([ await expect(agent(service).invoke(request(service), new AbortController().signal)).rejects.toThrow(message); expect(acquire).not.toHaveBeenCalled(); }); + +// #164: the planning check runs as a planning job in planning's own worker, and passes only with the schema's answer. +it('passes the planning check with the schema\'s answer from the worker', async () => { + const service = review(); + await expect(agent(service).probe('claude', new AbortController().signal)).resolves.toBeUndefined(); +}); + +it('fails the planning check without a sign-in or with Codex, before the review\'s lock is taken', async () => { + const service = review(), acquire = vi.spyOn(LeftoverLedger.prototype, 'acquire'); + const unsigned = new PlanningAgent(service, { url: STUB, env: {} }); + closers.push(() => unsigned.close()); + await expect(unsigned.probe('claude', new AbortController().signal)).rejects.toThrow('CLAUDE_CODE_OAUTH_TOKEN'); + await expect(agent(service).probe('codex', new AbortController().signal)).rejects.toThrow(CODEX_PLANNING_REFUSED); + expect(acquire).not.toHaveBeenCalled(); +}); + +it('runs the planning check on the review\'s code at the snapshot\'s head, with the check\'s prompt and schema', async () => { + const service = review(), plan = vi.spyOn(AgentWorker.prototype, 'plan'); + await agent(service).probe('claude', new AbortController().signal); + const run = plan.mock.calls[0]![0], snapshot = service.store.getSnapshot(service.config.identity); + expect(run).toMatchObject({ provider: 'claude', repository: service.reviewRepository().path, head: snapshot.head, + prompt: PLANNING_PROBE_PROMPT, schemaText: PLANNING_PROBE_SCHEMA, + context: { snapshotId: snapshot.id, planId: service.config.identity.planId, referencedCodeHash: snapshot.head } }); + expect(run.taskId).toMatch(/^planning-probe-[0-9a-f-]{36}$/); + expect(run.deadline - Date.now()).toBeGreaterThan(PLANNING_BUDGET_MS - 30_000); +}); + +it('fails the planning check when the agent answers anything but the schema\'s answer', async () => { + const service = review(), wrong = new PlanningAgent(service, { url: STUB, env: { CLAUDE_CODE_OAUTH_TOKEN: 'probe-wrong-answer' } }); + closers.push(() => wrong.close()); + await expect(wrong.probe('claude', new AbortController().signal)).rejects.toThrow('did not give the check\'s answer'); +}); + +it('passes the planning check only with exactly the schema\'s answer', () => { + expect(isPlanningProbeAnswer('{"ok":true}')).toBe(true); + expect(isPlanningProbeAnswer(' { "ok": true }\n')).toBe(true); + for (const text of ['', 'ok', 'true', '{"ok":false}', '{"ok":"true"}', '{"ok":true,"extra":1}', '[{"ok":true}]', 'null', '{}', + '{"ok":false,"ok":true}', '{"o\\u006b":true}', '\uFEFF{"ok":true}', '{\u00a0"ok":true}']) + expect(isPlanningProbeAnswer(text)).toBe(false); +}); diff --git a/web/planning.ts b/web/planning.ts index 3385eb3f..6edeb929 100644 --- a/web/planning.ts +++ b/web/planning.ts @@ -1,6 +1,6 @@ import { GhIssueGateway, ISSUE_READ_TIMEOUT_MS, withIssueReadDeadline, type IssueTrustGateway } from '../github/issues.ts'; import { GuardRefusal } from '../runner/lifecycle.ts'; -import { PlanningAgent } from '../runner/planning.ts'; +import { PLANNING_UNAVAILABLE, PlanningAgent, PlanningCheck } from '../runner/planning.ts'; import type { ReviewConfig, ReviewService } from '../runner/review.ts'; import type { PlanningSetup } from './server.ts'; @@ -10,13 +10,19 @@ export { ISSUE_READ_TIMEOUT_MS } from '../github/issues.ts'; * Production planning (#117): on for a review with a github block, never in a demo. It needs no runner block, since * planning only reads code, as Ask does. The issue text is read from GitHub for each request (current collaborators' * comments, or every comment under explicit author-bound trust), bounded like an execute prompt; approved lessons stay - * empty until lessons exist (L1 to L4). + * empty until lessons exist (L1 to L4). The planning check (#164) starts with the server for the vendor chosen in + * Settings (or, with none chosen then, with the first request after one is), and, when a vendor is chosen, must pass + * before a request reads the issue or records a planning request; a failed check refuses the request with its reason. */ 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?: Pick; agent?: (service: ReviewService) => PlanningAgent; + /** How long a failed planning check refuses requests; `PLANNING_RECHECK_MS` by default. */ + recheckMs?: number; + /** How long a request waits for a running check; `PLANNING_ADMIT_WAIT_MS` by default. */ + admitWaitMs?: number; }): PlanningSetup | undefined { const github = config.github; if (config.demo || !github) return undefined; @@ -28,28 +34,51 @@ export function productionPlanning(config: ReviewConfig, options: { options.verifyLock(); service.store.settleInterruptedRequests(); const provider = options.agent?.(service) ?? new PlanningAgent(service); + // Started now, before the server listens. If startup fails after this, the CLI exits (web/cli.ts), and the next start's + // planning recovery removes what the check left (its objects carry planning's owner). Another caller of startServer + // is left with the check running: startServer does not close planning when a later startup step fails (#198). + const check = new PlanningCheck((vendor, signal) => provider.probe(vendor, signal), () => service.store.questionProvider(), + { recheckMs: options.recheckMs, waitMs: options.admitWaitMs }); + check.start(); return { - provider, - describe(signal) { return withIssueReadDeadline(signal, async (readSignal, timeoutMs) => { - const access = await issues.issueAccess(github.issue, { signal: readSignal, timeoutMs }); - const trust = service.store.issueTrust(github.repository, github.issue); - const explicitlyTrusted = trust?.revokedAt === null && trust.authorLogin === access.authorLogin; - if (!access.collaborator && !explicitlyTrusted) throw new GuardRefusal(`Issue #${github.issue} is not trusted for its current author.`); - const validate = () => { - if (!explicitlyTrusted) return; - const current = service.store.issueTrust(github.repository, github.issue); - if (!current || current.revokedAt !== null || current.authorLogin !== access.authorLogin) - throw new GuardRefusal(`Issue #${github.issue} is not trusted for its current author.`); - }; - const text = await issues.issueText(github.issue, { signal: readSignal, timeoutMs, - trustedAuthor: explicitlyTrusted ? access.authorLogin : undefined, expectedAccess: access }); - validate(); - // 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 }, validate }; - }); }, - close: () => provider.close(), + // Checked once more when the provider is invoked, as defence in depth: E3 invokes it in a microtask right after the + // recorded action, where Settings cannot change, but a later caller might not. A refusal here settles the request as + // failed. + provider: { async invoke(request, signal) { check.assertPassed(); return provider.invoke(request, signal); } }, + async describe(signal) { + // Only refusals leave the wait: anything else (an aborted request, a Store that cannot be read) would be recorded + // by the server as a failed GitHub read, which this wait is not. + try { await check.admit(signal); } + catch (error) { + if (error instanceof GuardRefusal) throw error; + const reason = error instanceof Error ? error.message : String(error); + throw new GuardRefusal(error === signal.reason ? `${PLANNING_UNAVAILABLE} codeboost stopped waiting for the planning check: ${reason}` + : `${PLANNING_UNAVAILABLE} codeboost could not check the planning agent: ${reason}`); + } + return withIssueReadDeadline(signal, async (readSignal, timeoutMs) => { + const access = await issues.issueAccess(github.issue, { signal: readSignal, timeoutMs }); + const trust = service.store.issueTrust(github.repository, github.issue); + const explicitlyTrusted = trust?.revokedAt === null && trust.authorLogin === access.authorLogin; + if (!access.collaborator && !explicitlyTrusted) throw new GuardRefusal(`Issue #${github.issue} is not trusted for its current author.`); + const validate = () => { + if (!explicitlyTrusted) return; + const current = service.store.issueTrust(github.repository, github.issue); + if (!current || current.revokedAt !== null || current.authorLogin !== access.authorLogin) + throw new GuardRefusal(`Issue #${github.issue} is not trusted for its current author.`); + }; + const text = await issues.issueText(github.issue, { signal: readSignal, timeoutMs, + trustedAuthor: explicitlyTrusted ? access.authorLogin : undefined, expectedAccess: access }); + validate(); + // 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; + // Run again just before the request starts: Settings may have chosen an unchecked vendor during the read. + const admitted = () => { validate(); check.assertPassed(); }; + return { issue: { number: text.number, title: text.title, body: text.body, comments: [...text.comments] }, + approvedLessons: [], repo: { name: github.repository, baseRef }, validate: admitted }; + }); + }, + // The check runs in the provider's worker, so it stops before the worker does. + close: async () => { await check.close(); await provider.close(); }, }; }; }