diff --git a/core/parse-v1.ts b/core/parse-v1.ts index a1c74adc..b59923e4 100644 --- a/core/parse-v1.ts +++ b/core/parse-v1.ts @@ -1,6 +1,7 @@ import { isAlias, isMap, isScalar, isSeq, parseDocument } from 'yaml'; -const MAX_BYTES = 1024 * 1024; +/** The largest plan source import parses, in UTF-8 bytes. The import request limit (web/server.ts) is derived from it. */ +export const MAX_SOURCE_BYTES = 1024 * 1024; const number = /^-?(?:0|[1-9][0-9]*)(?:\.[0-9]+)?(?:[eE][+-]?[0-9]+)?$/u; function reject(message: string): never { throw new Error(message); } function numeric(text: string): number { @@ -21,9 +22,9 @@ function numeric(text: string): number { } /** Frozen v1 syntax contract. No runtime I/O or alias/object construction. */ export function parseV1(input: string | Uint8Array, format: 'json' | 'yaml'): unknown { - if (typeof input === 'string' && input.length > MAX_BYTES) reject('Input size exceeds 1 MiB.'); + if (typeof input === 'string' && input.length > MAX_SOURCE_BYTES) reject('Input size exceeds 1 MiB.'); const bytes = typeof input === 'string' ? new TextEncoder().encode(input) : input; - if (bytes.byteLength > MAX_BYTES) reject('Input size exceeds 1 MiB.'); + if (bytes.byteLength > MAX_SOURCE_BYTES) reject('Input size exceeds 1 MiB.'); const source = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(bytes); if (typeof input === 'string' && input !== source) reject('Invalid UTF-8 source string.'); if (format === 'json') return parseJson(source); diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 9b418201..dcb68072 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -358,7 +358,7 @@ F1 owns the production planning endpoints that G4 needs. They wrap E3's coordina | Endpoint | Store or coordinator call | Guard | |---|---|---| -| `POST /api/plan/import` | `Store.importRevision` | expected revision | +| `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 | diff --git a/docs/plan-format.md b/docs/plan-format.md index 36d6ed32..3fcac869 100644 --- a/docs/plan-format.md +++ b/docs/plan-format.md @@ -95,6 +95,8 @@ questions: [] Import accepts UTF-8 JSON or one YAML 1.2 document representing JSON-compatible data. Reject invalid UTF-8, input above 1 MiB, and nesting deeper than 50 containers before producing a plan. Parse into an intermediate syntax tree with duplicate-key detection; do not convert an unchecked YAML graph into application objects. +`POST /api/plan/import` sends the source as one JSON string in a request body of at most 6 MiB + 16 KiB (6,307,840 bytes, `MAX_IMPORT_REQUEST_BYTES` in `web/server.ts`). Any JSON encoding writes each UTF-8 byte of the source as at most 6 bytes: YAML accepts a raw control character in a quoted scalar, and JSON writes it as `\u0001`. So every source within the 1 MiB parser limit fits, with 16 KiB left for the rest of the request. A larger body is refused with HTTP 413 before it is parsed. A body within the limit whose source is above 1 MiB gets the parser's refusal. Every other `POST` keeps its 16 KiB limit (#146). + For both formats, object keys must be strings and unique after decoding at every level; reject duplicates rather than choosing the first or last value. JSON follows RFC 8259 syntax. YAML allows mappings, sequences, strings (including quoted and block strings), the exact plain literals `true`, `false`, and `null`, and numbers spelled using the JSON number grammar. Reject empty implicit values, non-finite values (`.nan`/`.inf`), non-JSON number spellings such as hex/octal or numeric separators, complex/non-string keys, every anchor and alias, merge keys (`<<`), explicit tags (including custom tags), extra documents, and parser warnings/errors before schema validation. Ordinary YAML 1.2 plain strings remain strings; do not infer dates, functions, or application-specific types. Integers used for issue, revision, or indexes must also be exactly representable safe integers in the implementation. Only after these checks convert to JSON-compatible values, select the registered schema, and validate. Never enable alias expansion or custom object construction. Required fixtures: equivalent JSON/YAML produce identical structured plans; duplicate decoded keys in either format, anchors/aliases, merge keys, tags, non-finite/non-JSON numbers, empty values, complex keys, multiple documents, excessive depth, and oversized input all fail before schema validation. An ordinary quoted string containing `<<` or `&` as part of its text is data, not YAML syntax. diff --git a/test/runner-planning-feedback.test.ts b/test/runner-planning-feedback.test.ts index d5be39c3..233a5f30 100644 --- a/test/runner-planning-feedback.test.ts +++ b/test/runner-planning-feedback.test.ts @@ -7,7 +7,8 @@ import { createHash, randomUUID } from 'node:crypto'; import { choiceKeys } from '../core/approvals.ts'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { createDemo } from '../scripts/demo.ts'; -import { startServer, type PlanningDeps } from '../web/server.ts'; +import { MAX_IMPORT_REQUEST_BYTES, MAX_REQUEST_BYTES, startServer, type PlanningDeps } from '../web/server.ts'; +import { MAX_SOURCE_BYTES } from '../core/parse-v1.ts'; import { Store } from '../runner/store.ts'; import { ReviewService } from '../runner/review.ts'; import { fixtureGit } from './fixtures/git.ts'; @@ -144,6 +145,76 @@ describe('planning API for lane G', () => { expect((await review(app)).plan.revision).toBe(view.plan.revision + 1); expect((await api(app, 'POST', '/api/plan/import', { ...request, actionId: randomUUID() })).status).toBe(409); }); + describe('import request size (#146)', () => { + /** The plan with items added up to `count` items. Each new item adds files whose change text is `change`. */ + const grown = (plan: Record, count: number, change: string, files = 40) => ({ ...plan, items: [...plan.items, + ...Array.from({ length: count - plan.items.length }, (_, i) => { + const id = `P${plan.items.length + i + 1}`; + return { id, title: `Item ${id}`, intent: 'Fill the plan.', depends_on: [], acceptance: [{ type: 'check', text: 'Covered.' }], + files: Array.from({ length: files }, (_, k) => ({ path: `import-limit/${id}/f${k}.txt`, kind: 'add', renamed_from: null, change })) }; + })] }); + const post = async (app: App, body: string) => { + const response = await fetch(`${new URL(app.url).origin}/api/plan/import`, { method: 'POST', body, + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' } }); + return { status: response.status, body: await response.json() as Record }; + }; + const envelope = (source: string, revision: number) => JSON.stringify({ source, format: 'json', expectedRevision: revision, actionId: randomUUID() }); + + it('imports a schema-valid plan whose request is larger than the 16 KiB other requests get', async () => { + const { app } = await serve(); + const view = await review(app); + const body = envelope(JSON.stringify(grown(view.plan, 10, 'Write the generated file.')), view.plan.revision); + expect(Buffer.byteLength(body)).toBeGreaterThan(MAX_REQUEST_BYTES); + expect(await post(app, body)).toEqual({ status: 200, body: { result: { revision: view.plan.revision + 1 } } }); + expect((await review(app)).plan.items).toHaveLength(10); + }); + it('imports a 1 MiB YAML source that escapes to almost six times its size, the most JSON can expand it', async () => { + const { app } = await serve(); + const view = await review(app); + // YAML takes a raw control character in a quoted scalar; JSON writes each one as \u0001. + const yaml = (plan: unknown) => `%YAML 1.2\n---\n${JSON.stringify(plan).replaceAll('\\u0001', '\u0001')}\n`; + let files = 1; + while (Buffer.byteLength(yaml(grown(view.plan, 30, '\u0001'.repeat(1200), files + 1))) < MAX_SOURCE_BYTES) files++; + const unpadded = yaml(grown(view.plan, 30, '\u0001'.repeat(1200), files)); + const source = `${unpadded}#${'x'.repeat(MAX_SOURCE_BYTES - Buffer.byteLength(unpadded) - 1)}`; + expect(Buffer.byteLength(source)).toBe(MAX_SOURCE_BYTES); + const body = JSON.stringify({ source, format: 'yaml', expectedRevision: view.plan.revision, actionId: randomUUID() }); + expect(Buffer.byteLength(body)).toBeGreaterThan(5.5 * MAX_SOURCE_BYTES); + expect(Buffer.byteLength(body)).toBeLessThanOrEqual(MAX_IMPORT_REQUEST_BYTES); + expect(await post(app, body)).toEqual({ status: 200, body: { result: { revision: view.plan.revision + 1 } } }); + expect((await review(app)).plan.items[29].files[0].change).toBe('\u0001'.repeat(1200)); + }); + it('passes a request at the limit to the parser, which refuses a source over 1 MiB', async () => { + const { app } = await serve(); + const view = await review(app), empty = Buffer.byteLength(envelope('', view.plan.revision)); + const body = envelope('x'.repeat(MAX_IMPORT_REQUEST_BYTES - empty), view.plan.revision); + expect(Buffer.byteLength(body)).toBe(MAX_IMPORT_REQUEST_BYTES); + const response = await post(app, body); + expect(response).toEqual({ status: 409, body: { error: expect.stringMatching(/exceeds 1 MiB/) } }); + // The refusal is recorded like any other import refusal: the same request replays it. + expect(await post(app, body)).toEqual(response); + expect((await review(app)).plan.revision).toBe(view.plan.revision); + }); + it('refuses a request one byte over the limit before reading it as JSON, and imports nothing', async () => { + const { app } = await serve(); + const view = await review(app), empty = Buffer.byteLength(envelope('', view.plan.revision)); + const body = envelope('x'.repeat(MAX_IMPORT_REQUEST_BYTES - empty + 1), view.plan.revision); + expect(Buffer.byteLength(body)).toBe(MAX_IMPORT_REQUEST_BYTES + 1); + expect(await post(app, body)).toEqual({ status: 413, body: { error: 'Request too large.' } }); + expect((await review(app)).plan.revision).toBe(view.plan.revision); + }); + it('keeps the 16 KiB limit for every other request, the other planning requests included', async () => { + const { app } = await serve(); + const body = JSON.stringify({ pad: 'x'.repeat(MAX_REQUEST_BYTES - 9) }); + expect(Buffer.byteLength(body)).toBe(MAX_REQUEST_BYTES + 1); + for (const path of ['/api/action', '/api/settings', '/api/issues', '/api/runner', '/api/plan/suggestions', '/api/plan/drafts', + `/api/plan/suggestions/${randomUUID()}/apply`, `/api/plan/drafts/${randomUUID()}/cancel`]) { + const response = await fetch(`${new URL(app.url).origin}${path}`, { method: 'POST', body, + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' } }); + expect([path, response.status]).toEqual([path, 413]); + } + }); + }); it('starts, reads, and applies a suggestion exactly once', async () => { const { deps, calls } = planning(); const { app } = await serve({ planning: deps }); diff --git a/web/server.ts b/web/server.ts index 43f3c397..cba4b5a8 100644 --- a/web/server.ts +++ b/web/server.ts @@ -20,6 +20,7 @@ import { IssueBoard } from './issues.ts'; import { SuggestionCoordinator, type PlanningMode, type SuggestionHandle, type SuggestionInput, type SuggestionStore } from '../core/planning-suggestions.ts'; import type { AuthorProvider } from '../core/planning-author.ts'; import { PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; +import { MAX_SOURCE_BYTES } from '../core/parse-v1.ts'; import type { PreMergeCoordinator } from '../runner/pre-merge.ts'; export type PlanningDescription = Pick & { repo: { name: string; baseRef: string }; @@ -44,6 +45,16 @@ export const PLANNING_SHUTDOWN_GRACE_MS = 20_000; /** Production planning is built after the Store opens, from the review it serves (see web/cli.ts). */ export type PlanningSetup = (service: ReviewService) => PlanningDeps; const publicRoot = new URL('./public/', import.meta.url); +/** The largest body of a POST request other than plan import, in bytes. */ +export const MAX_REQUEST_BYTES = 16 * 1024; +/** + * The largest body of `POST /api/plan/import`, in bytes (#146). The body carries the plan source as one JSON string. + * Any JSON encoding writes each UTF-8 byte of that source as at most 6 bytes (`\u0001` for a control character, which + * YAML accepts in a scalar). So the limit admits every source within the parser's limit, inside any envelope that + * fits MAX_REQUEST_BYTES with an empty source. A body within the limit whose source is above the parser's limit gets + * the parser's refusal, not a transport one. + */ +export const MAX_IMPORT_REQUEST_BYTES = 6 * MAX_SOURCE_BYTES + MAX_REQUEST_BYTES; /** The longest shutdown waits for admitted requests to finish before aborting them; below the 15 s request timeout. */ export const MAX_SHUTDOWN_DRAIN_MS = 14_500; /** @@ -589,8 +600,9 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const planningPath = path === '/api/plan/import' || PLANNING_REQUEST.test(path); if (req.method !== 'POST' || !(['/api/action','/api/settings','/api/issues','/api/runner'].includes(path) || planningPath) || req.headers['content-type'] !== 'application/json') { json(405, { error: 'Unsupported request.' }); return; } const chunks: Buffer[] = []; let size = 0; + const limit = path === '/api/plan/import' ? MAX_IMPORT_REQUEST_BYTES : MAX_REQUEST_BYTES; activeRequest.readingBody=true; - try { for await (const chunk of req) { size += chunk.length; if (size > 16384) { json(413, { error: 'Request too large.' }); return; } chunks.push(chunk); } } + try { for await (const chunk of req) { size += chunk.length; if (size > limit) { json(413, { error: 'Request too large.' }); return; } chunks.push(chunk); } } finally { activeRequest.readingBody=false; } const body = new TextDecoder('utf-8', { fatal: true }).decode(Buffer.concat(chunks)); const input=JSON.parse(body);