diff --git a/agents/adapters/claude.ts b/agents/adapters/claude.ts index 37df0b29..ef01af72 100644 --- a/agents/adapters/claude.ts +++ b/agents/adapters/claude.ts @@ -7,15 +7,60 @@ import { setUpProfile } from './setup.ts'; import { assertAdapterRequest, capAdapterInvocationBudget, createAdapterInvocationBudget, type AgentAdapterOptions, type AgentAdapterRequest } from './types.ts'; +/** + * The exact source text of each member of a top-level JSON object, by key. `json` must already have passed + * `JSON.parse` as an object, so only string and nesting boundaries are tracked here; every loop also stops at the end + * of the text, so other input cannot hang it. Refuses a key that appears twice, where `JSON.parse` would silently keep + * the last value. + */ +function topLevelMembers(json: string): Map { + const members = new Map(); + const isSpace = (char: string | undefined) => char === ' ' || char === '\t' || char === '\n' || char === '\r'; + let at = 0; + const skipSpace = () => { while (isSpace(json[at])) at++; }; + const skipString = () => { // `at` is on the opening quote; leaves `at` after the closing one. + for (at++; at < json.length && json[at] !== '"'; at++) if (json[at] === '\\') at++; + at++; + }; + skipSpace(); at++; // '{' + for (skipSpace(); at < json.length && json[at] !== '}'; skipSpace()) { + const keyStart = at; skipString(); + const key = JSON.parse(json.slice(keyStart, at)) as string; + skipSpace(); at++; skipSpace(); // ':' + const valueStart = at; + if (json[at] === '"') skipString(); + else if (json[at] === '{' || json[at] === '[') { + let depth = 0; + do { + if (json[at] === '"') { skipString(); continue; } + if (json[at] === '{' || json[at] === '[') depth++; + else if (json[at] === '}' || json[at] === ']') depth--; + at++; + } while (depth > 0 && at < json.length); + } else while (at < json.length && !isSpace(json[at]) && json[at] !== ',' && json[at] !== '}') at++; + if (members.has(key)) throw new Error('Claude returned a malformed output envelope.'); + members.set(key, json.slice(valueStart, at)); + skipSpace(); if (json[at] === ',') at++; + } + return members; +} + /** * Read Claude's `--output-format json` envelope. A result whose `subtype` is not `success`, or whose `is_error` is * true, is a provider failure; it can omit `result`. With `--json-schema` (planning), a successful answer needs - * `subtype: "success"` and is the schema-validated `structured_output` object, returned as JSON text; `result` then - * holds only Claude's prose. Plain-text envelopes without a `subtype` are still accepted. + * `subtype: "success"` and is the schema-validated `structured_output` object. It is returned as the exact JSON text + * of that member in the CLI's envelope, never re-encoded here, so the plan parser sees any duplicate key or inexact + * integer the envelope holds (#166). Whether the API or CLI resolves those before writing the envelope is not verified. + * With a schema, an envelope that repeats a top-level key is malformed, whatever it reports. `result` then holds only + * Claude's prose. Plain-text envelopes without a `subtype` are still accepted. */ export function parseClaudeOutput(raw: Buffer, structured = false): { text: string; providerFailed: boolean } { - const envelope = JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(raw)) as + const source = new TextDecoder('utf-8', { fatal: true }).decode(raw); + const envelope = JSON.parse(source) as { subtype?: unknown; result?: unknown; is_error?: unknown; structured_output?: unknown }; + // Read every member before classifying: JSON.parse keeps only the last of a repeated key, such as `is_error`. + const members = structured && envelope !== null && typeof envelope === 'object' && !Array.isArray(envelope) + ? topLevelMembers(source) : null; if (typeof envelope.is_error !== 'boolean' || (envelope.result !== undefined && typeof envelope.result !== 'string') || (envelope.subtype !== undefined && typeof envelope.subtype !== 'string')) throw new Error('Claude returned a malformed output envelope.'); @@ -31,7 +76,7 @@ export function parseClaudeOutput(raw: Buffer, structured = false): { text: stri const answer = envelope.structured_output; if (answer === null || typeof answer !== 'object' || Array.isArray(answer)) throw new Error('Claude returned no structured output for a schema-constrained answer.'); - return Object.freeze({ text: JSON.stringify(answer), providerFailed: false }); + return Object.freeze({ text: members!.get('structured_output')!, providerFailed: false }); } /** diff --git a/docs/implementation/agent-isolation.md b/docs/implementation/agent-isolation.md index c3858887..8b2b3fc6 100644 --- a/docs/implementation/agent-isolation.md +++ b/docs/implementation/agent-isolation.md @@ -89,10 +89,15 @@ Use only these entry points to run an agent: - **Planning answers.** Use `startClaudeInvocation` for planning. It reads `schema.json` from the input directory and passes its exact text to Claude as `--json-schema`. The container profile refuses the command if that text differs from the file it mounts. The result's `stdout` is Claude's `structured_output` object as JSON text, with - no prose or Markdown around it. A Claude run that fails its own schema check settles with a nonzero exit code. - Questions answer in plain text and carry no schema flag. The schema file must be a regular file with one link, - at most 64 KiB of UTF-8, and a JSON object with `"type": "object"`; otherwise the start call throws. Leave out a - draft 2020-12 `$schema` line: Claude's `--json-schema` rejects it (see [plan format](../plan-format.md)). + no prose or Markdown around it. It is the exact text of that member, cut from the CLI's envelope and never + re-encoded here, so the plan parser sees any duplicate key or inexact integer the envelope holds and refuses it + (#166). The Claude API and CLI may resolve those before writing the envelope; that is not verified, because the + only real-CLI probes need credentials and do not run in CI. A planning envelope that repeats a top-level key is + refused as malformed, whatever it reports. A Claude run that fails its own schema check settles with a nonzero + exit code. Questions answer in plain text and carry no schema flag. The schema file must be a regular file with + one link, at most 64 KiB of UTF-8, and a JSON object with `"type": "object"`; otherwise the start call throws. + Leave out a draft 2020-12 `$schema` line: Claude's `--json-schema` rejects it (see [plan + format](../plan-format.md)). - **Codex.** `startCodexInvocation` throws for every phase, and allocates nothing. Use Claude. See [Codex is refused in every phase](#codex-is-refused-in-every-phase). 5. To keep a stopped writable attempt's partial output, call `exportTaskDiff(storage, { base, imageId })` before diff --git a/docs/implementation/plan-library-alignment.md b/docs/implementation/plan-library-alignment.md index 4ffb02c9..10eff824 100644 --- a/docs/implementation/plan-library-alignment.md +++ b/docs/implementation/plan-library-alignment.md @@ -21,12 +21,12 @@ Issue #6 tracked the v1 plan library contract and the runner obligations that ca | Container mounts: Git metadata on its own filesystem, read-only, with no writable hard links or aliases into it | Done before this change | `agents/container/profile.ts`, probe `metadata-alias` in `agents/policy.ts`; `test/agent-container.test.ts` ("keeps Git metadata read-only, on another filesystem, and mounted against replacement", "keeps Git metadata unchanged under link, alias, truncation and replacement attempts during %s"), negative controls in `test/agent-gate.test.ts` (real Docker) | | Process termination: output overflow, deadline and cancellation stop the whole container, including a process that ignores SIGTERM | Done before this change | `agents/adapters/supervisor.ts`; `test/agent-supervisor.test.ts` ("terminates %s at bounded output limits", "enforces a finite wall deadline and force-settles the container", "preserves the first cancellation reason until an ignored SIGTERM fully settles") (real Docker) | | Output shown in the UI bounded: captured output comes only from the supervisor, which bounds stdout and stderr; when a planning run exits with an error, the provider's message quotes its output with whitespace collapsed, cut to at most 300 UTF-16 code units (`runner/planning-provider.ts`) | Done before this change; 300-unit quote tested in this change | `test/agent-supervisor.test.ts` output-limit cases (real Docker); `test/planning-provider.test.ts` ("quotes at most 300 code units of a failed run's output, with whitespace collapsed"). The Docker daemon's own log is #165 | -| 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`. The decoder re-serializes the answer, so the limit applies to the decoded text, not to the provider's bytes; #166 may change that | +| 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 | — | | Planning profile probed at startup | Moved to #164 | — | | Docker daemon log limits | Moved to #165 | — | -| Claude structured output under the v1 parsing contract | Moved to #166 (found during this change) | — | +| 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") | ## Schema loading diff --git a/test/agent-adapter.test.ts b/test/agent-adapter.test.ts index 3b1078ca..2abfe1b7 100644 --- a/test/agent-adapter.test.ts +++ b/test/agent-adapter.test.ts @@ -140,26 +140,75 @@ describe('production agent adapters', () => { expect(parseClaudeOutput(raw, true)).toEqual({ text: answer, providerFailed: false }); }); - it('re-serializes a structured answer, so decoded text can exceed its raw bytes (#166 may change this)', () => { - // Describes today's decoder, which re-serializes the answer (#166 may change that): JSON.parse then JSON.stringify - // writes 1e20 in full, so 4 raw bytes become 21. The supervisor bounds the decoded text itself; see the - // decoded-output limit cases in agent-supervisor.test.ts. - const raw = structuredEnvelope(JSON.stringify({ n: [] }).replace('[]', '[' + Array(1000).fill('1e20').join(',') + ']'), 8 * 1024); - const { text } = parseClaudeOutput(raw, true); - expect(Buffer.byteLength(text)).toBeGreaterThan(raw.length); + it('returns a structured answer as the exact text of structured_output in the envelope, whatever surrounds it', () => { + // Spacing, escapes, number spellings and brackets inside strings all survive, and so do members on either side. + const answer = '{ "summary" : "caf\\u00e9 } ] \\" \\\\", "n": [1e20, 1.0, -0E+0],\n "nested": {"a": [{}, []]} }'; + const raw = Buffer.from('{"type":"result","subtype":"success","is_error":false,"result":"text with } and \\" in it",' + + `"structured_output":\t${answer}\n, "usage": {"input_tokens": 1}, "end": null}`); + expect(parseClaudeOutput(raw, true)).toEqual({ text: answer, providerFailed: false }); + expect(parseClaudeOutput(Buffer.from(`{"subtype":"success","is_error":false,"structured_output":${answer}}`), true).text) + .toBe(answer); + }); + + // #166: re-encoding the answer once let a duplicate key or an inexact integer reach the plan parser already resolved. + const validPlan = { schema_version: 1, issue: 1, revision: 1, summary: 'Plan', questions: [], + items: [{ id: 'P1', title: 'Change', intent: 'Improve', files: [{ path: 'a', kind: 'edit', renamed_from: null, change: 'Change' }], + acceptance: [{ type: 'check', text: 'Works' }], depends_on: [] }] }; + const draft = () => prepareDraft({ context: { identity: { repositoryId: 'repo', taskId: 'task', planId: 'plan' }, issue: 1, + baseEntries: [{ path: 'a', kind: 'file' }], pathKey: path => path, allowedCommands: [] }, requestId: 'request', revision: 1, + repo: { name: 'repo', baseRef: 'main', baseSha: 'a'.repeat(40), paths: ['a'] }, + issue: { number: 1, title: 'Fix', body: '', comments: [] }, approvedLessons: [], feedback: '' }); + it.each([ + ['a duplicate key', JSON.stringify(validPlan).replace('"summary":"Plan"', '"summary":"Plan","summary":"Other"'), 'Duplicate key: summary'], + ['an inexact integer', JSON.stringify(validPlan).replace('"revision":1', '"revision":1.0000000000000001'), 'Integer is not exactly representable.'], + ])('lets draft validation refuse %s in a structured answer', (_case, answer, refusal) => { + const { text } = parseClaudeOutput(structuredEnvelope(answer, answer.length + 256), true); + expect(text).toBe(answer); + expect(() => draft().validate(text)).toThrow(refusal); + // The same plan without the defect is accepted, so the refusal is the defect's. + const valid = JSON.stringify(validPlan); + expect(draft().validate(parseClaudeOutput(structuredEnvelope(valid, valid.length + 256), true).text).value).toEqual(validPlan); + }); + + it('refuses a structured envelope that repeats a top-level key, before reading what it reports', () => { + for (const envelope of ['{"subtype":"success","is_error":false,"structured_output":{"a":1},"structured_output":{"a":2}}', + '{"subtype":"success","is_error":true,"is_error":false,"structured_output":{"a":1}}', + // The last is_error would otherwise make this a provider failure rather than a refusal. + '{"subtype":"success","is_error":false,"is_error":true,"structured_output":{"a":1}}', + // Keys are compared decoded, so another spelling of the same key is a repeat. + '{"subtype":"success","is_error":false,"structured_output":{"a":1},"structured\\u005foutput":{"a":2}}']) + expect(() => parseClaudeOutput(Buffer.from(envelope), true)).toThrow('malformed'); + // Without a schema the envelope is read as before. + expect(parseClaudeOutput(Buffer.from('{"is_error":false,"result":"a","result":"b"}'))).toEqual({ text: 'b', providerFailed: false }); + }); + + // The base refused these too. They guard the scanner's precondition: it is given only an object envelope. + it.each(['[{"is_error":false}]', '[]', '"text"', 'true'])('refuses a structured envelope that is not an object: %s', envelope => { + expect(() => parseClaudeOutput(Buffer.from(envelope), true)).toThrow('malformed'); + }); + + it('finds structured_output however the envelope is spaced or its key is spelled', () => { + for (const envelope of [' \r\n{ "subtype" : "success" , "is_error" : false , "structured_output" : {"a":1} } \n', + '{"subtype":"success","is_error":false,"structured\\u005foutput":{"a":1}}']) + expect(parseClaudeOutput(Buffer.from(envelope), true).text).toBe('{"a":1}'); + }); + + it('applies the 1 MiB plan parser limit to the bytes of structured_output in the envelope, not to a re-encoding', () => { + // Spacing inside the answer counts: re-encoded, this answer would be far below the limit. + const answer = `{ "summary": "x" ${' '.repeat(MAX_SOURCE_BYTES)}}`; + expect(Buffer.byteLength(JSON.stringify(JSON.parse(answer)))).toBeLessThan(MAX_SOURCE_BYTES); + const { text } = parseClaudeOutput(structuredEnvelope(answer, answer.length + 256), true); + expect(text).toBe(answer); + expect(() => draft().validate(text)).toThrow('Input size exceeds 1 MiB.'); }); it('applies the 1 MiB plan parser limit to the decoded answer at the exact byte in draft validation', () => { - const draft = prepareDraft({ context: { identity: { repositoryId: 'repo', taskId: 'task', planId: 'plan' }, issue: 1, - baseEntries: [{ path: 'a', kind: 'file' }], pathKey: path => path, allowedCommands: [] }, requestId: 'request', revision: 1, - repo: { name: 'repo', baseRef: 'main', baseSha: 'a'.repeat(40), paths: ['a'] }, - issue: { number: 1, title: 'Fix', body: '', comments: [] }, approvedLessons: [], feedback: '' }); const answerOf = (bytes: number) => JSON.stringify({ summary: 'x'.repeat(bytes - '{"summary":""}'.length) }); for (const [bytes, accepted] of [[MAX_SOURCE_BYTES, true], [MAX_SOURCE_BYTES + 1, false]] as const) { const { text } = parseClaudeOutput(structuredEnvelope(answerOf(bytes), bytes + 1024), true); expect(Buffer.byteLength(text)).toBe(bytes); // At the limit the answer parses and fails only later, on its missing revision; one byte more fails in the parser. - expect(() => draft.validate(text)).toThrow(accepted ? 'Response revision mismatch.' : 'Input size exceeds 1 MiB.'); + expect(() => draft().validate(text)).toThrow(accepted ? 'Response revision mismatch.' : 'Input size exceeds 1 MiB.'); } }); diff --git a/test/planning-extraction.test.ts b/test/planning-extraction.test.ts index 9d5094a5..fd583ec5 100644 --- a/test/planning-extraction.test.ts +++ b/test/planning-extraction.test.ts @@ -66,6 +66,7 @@ it.each([ expect(extract(raw)).toMatchObject({ providerFailed: true }); }); +// `structured` writes its envelope with JSON.stringify, so the exact text of the answer is JSON.stringify(answer). it('returns structured_output as JSON text and ignores Claude\'s prose result', () => { const answer = reply(); expect(extract(structured(answer))).toEqual({ text: JSON.stringify(answer), providerFailed: false });