From f0187404e7e62089e454f3d74e38da5597afded3 Mon Sep 17 00:00:00 2001 From: tomolom <37050939+tomolom@users.noreply.github.com> Date: Tue, 8 Sep 2026 04:17:20 +0100 Subject: [PATCH] fix(pi): accept ordered system prompt blocks and keep four cache slots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Oh My Pi types `Context.systemPrompt` as `string[]` and hands the array to the provider, so `context.systemPrompt?.trim()` threw — `?.` does not short-circuit a non-nullish value — and every subagent spawn failed before any request was built. `systemPromptText()` flattens string blocks (and `{ type: 'text', text }` blocks) on a blank line, matching how the host's own providers flatten them, so the documentation-paragraph split classifies the same text on either host. Flattening alone only moved the failure: with `/claude-cache` in automatic or hybrid mode this path added a top-level `cache_control` on top of the four block breakpoints it had already placed, sending five cache_control sites in one body — "A maximum of 4 blocks with cache_control may be provided. Found 5." Each mode now places only its own breakpoints, as the OpenCode rewrite path does: automatic clears the block breakpoints and keeps the top-level control (now at 1h, which it never set), hybrid keeps the four blocks at 1h with no top-level control. Breakpoints are located by visiting the objects that can hold one — the body root, system blocks, tools, messages and their content blocks — rather than by walking the body recursively. Tool `input_schema`s and replayed `tool_use.input`s are arbitrary caller data, so a nested field named `cache_control` there is a tool parameter or argument that must not be deleted or given a TTL. Fixes #201 --- packages/pi/src/convert.ts | 124 ++++++++++++++--- packages/pi/src/tests/convert.test.ts | 192 +++++++++++++++++++++++++- 2 files changed, 293 insertions(+), 23 deletions(-) diff --git a/packages/pi/src/convert.ts b/packages/pi/src/convert.ts index d6c2bfec..b2ad0efc 100644 --- a/packages/pi/src/convert.ts +++ b/packages/pi/src/convert.ts @@ -68,7 +68,7 @@ export type AnthropicRequestBody = { | { type: 'enabled'; budget_tokens: number } | { type: 'adaptive'; display: 'summarized' } output_config?: { effort: string } - cache_control?: { type: 'ephemeral' } + cache_control?: { type: 'ephemeral'; ttl?: '1h' } speed?: 'fast' } @@ -386,6 +386,49 @@ function addEphemeralCacheControl(body: AnthropicRequestBody): void { } } +/** + * Flatten a host system prompt to text. + * + * Pi types `Context.systemPrompt` as `string`, but Oh My Pi 18.x types it as + * `string[]` — "ordered system prompt blocks" — and hands that array to the + * provider unflattened, where `.trim()` is not a function and no request was + * ever built (issue #201). + * + * Blocks join on a blank line, which is how the host's own providers flatten + * them (`normalizeSystemPrompts(prompt).join('\n\n')`), so the paragraph split + * below classifies the same text on either host. The host asks providers to + * preserve its entries as distinct blocks, and that is deliberately not done + * here: an unrecognized prompt shape is carried in messages[] precisely because + * placing the host prompt in top-level system[] is the request shape Anthropic + * rejects with 400 "You're out of extra usage" (see splitPiSystemPrompt). + * Joining loses no text and no paragraph boundary; re-emitting the entries as + * system[] blocks would reintroduce that rejection. + * + * Only a `{ type: 'text', text }` block is read. The host declares strings, so + * that branch is for the next drift in this same field: returning '' there + * would silently ship requests with no host prompt at all, which is worse than + * the crash this replaces. `type` is checked rather than reading any object's + * `text` field, so a tool, image, or other structured block never has its + * metadata flattened into the prompt. + */ +function systemPromptText(prompt: unknown): string { + if (typeof prompt === 'string') return prompt + if (!Array.isArray(prompt)) return '' + + const parts: string[] = [] + for (const block of prompt) { + if (typeof block === 'string') { + parts.push(block) + continue + } + const record = block as { type?: unknown; text?: unknown } | null + if (record?.type === 'text' && typeof record.text === 'string') { + parts.push(record.text) + } + } + return parts.join('\n\n') +} + function splitPiSystemPrompt(prompt: string): { systemText?: string messageText: string @@ -431,33 +474,73 @@ function prependCachedPromptBlock( } } +/** + * Visit every object that can legitimately hold an Anthropic cache breakpoint: + * the request root, each `system[]` block, each tool, and each message with its + * content blocks — exactly where addEphemeralCacheControl and + * prependCachedPromptBlock place them. + * + * Deliberately not a deep walk. A tool's `input_schema` and a replayed + * `tool_use.input` are arbitrary caller data, so a nested field named + * `cache_control` there is a tool parameter or argument, not a breakpoint: + * deleting it or writing `ttl` into it would corrupt the tool contract. + */ +function walkCacheControlHolders( + body: AnthropicRequestBody, + visit: (holder: Record) => void, +): void { + const holders: unknown[] = [body] + if (body.system) holders.push(...body.system) + if (body.tools) holders.push(...body.tools) + for (const message of body.messages) { + holders.push(message) + if (Array.isArray(message.content)) holders.push(...message.content) + } + + for (const holder of holders) { + if (!holder || typeof holder !== 'object') continue + const record = holder as Record + const cacheControl = record.cache_control + if (cacheControl && typeof cacheControl === 'object') visit(record) + } +} + +/** + * Anthropic accepts at most four cache breakpoints per request. This provider + * composes its own body and has already placed exactly four + * (addEphemeralCacheControl's last tool, last system block and last user block, + * plus the cached prompt block on the first user message), so the top-level + * control this used to add on top of them made five cache_control sites in one + * body — the shape behind "A maximum of 4 blocks with cache_control may be + * provided. Found 5." on a request that never reached the model (issue #201). + * + * Each mode now places its own breakpoints and nothing else, as the OpenCode + * rewrite path does (`applyAutomaticCache1h` / `applyHybridCache1h` both clear + * every breakpoint first): + * - `automatic`: the top-level control alone, at 1h. + * - `hybrid`: the four block breakpoints extended to 1h, no top-level control. + * That is the placement OpenCode's hybrid anchors reconstruct by hand and + * this converter emits natively. + * - `explicit`: the same four breakpoints, TTL only. + */ function applyCacheMode( body: AnthropicRequestBody, enabled: boolean, mode: Cache1hMode, ): void { if (!enabled) return + if (mode === 'automatic') { - body.cache_control = { type: 'ephemeral' } + walkCacheControlHolders(body, (holder) => { + delete holder.cache_control + }) + body.cache_control = { type: 'ephemeral', ttl: '1h' } return } - const addTtl = (value: unknown): void => { - if (!value || typeof value !== 'object') return - if (Array.isArray(value)) { - for (const item of value) addTtl(item) - return - } - const record = value as Record - const cacheControl = record.cache_control - if (cacheControl && typeof cacheControl === 'object') { - ;(cacheControl as Record).ttl = '1h' - } - for (const child of Object.values(record)) addTtl(child) - } - - if (mode === 'hybrid') body.cache_control = { type: 'ephemeral' } - addTtl(body) + walkCacheControlHolders(body, (holder) => { + ;(holder.cache_control as Record).ttl = '1h' + }) } export async function buildAnthropicRequest( @@ -494,7 +577,8 @@ export async function buildAnthropicRequest( }, { type: 'text', text: CLAUDE_CODE_IDENTITY }, ] - if (context.systemPrompt?.trim()) { + const systemPrompt = systemPromptText(context.systemPrompt) + if (systemPrompt.trim()) { // Pi's prompt cannot sit whole in the top-level system[] array: two lines of // its documentation paragraph (the docs/*.md enumeration and the "follow .md // cross-references" instruction) are each independently sufficient to make @@ -519,7 +603,7 @@ export async function buildAnthropicRequest( // cache_control is set explicitly because addEphemeralCacheControl's // message-level breakpoint only fires for array content on the *last* user // message, which is not this one after the first turn. - const prompt = splitPiSystemPrompt(context.systemPrompt) + const prompt = splitPiSystemPrompt(systemPrompt) if (prompt.systemText) { system.push({ type: 'text', text: prompt.systemText }) } diff --git a/packages/pi/src/tests/convert.test.ts b/packages/pi/src/tests/convert.test.ts index 984b8331..251a1116 100644 --- a/packages/pi/src/tests/convert.test.ts +++ b/packages/pi/src/tests/convert.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from 'bun:test' import { computeCcVersionSuffix } from '@cortexkit/anthropic-auth-core' -import type { Message } from '@earendil-works/pi-ai' +import type { Context, Message } from '@earendil-works/pi-ai' import { buildAnthropicRequest } from '../convert' function userMsg(text: string): Message { @@ -15,10 +15,14 @@ function assistantMsg(text: string): Message { } as Message } -function toolCallMsg(id: string, name: string): Message { +function toolCallMsg( + id: string, + name: string, + args: Record = {}, +): Message { return { role: 'assistant', - content: [{ type: 'toolCall', id, name, arguments: {} }], + content: [{ type: 'toolCall', id, name, arguments: args }], timestamp: 0, } as Message } @@ -1053,3 +1057,185 @@ describe('convertMessages — signed thinking blocks', () => { }) }) }) + +describe('buildAnthropicRequest — host system prompt shapes', () => { + // Oh My Pi types Context.systemPrompt as ordered prompt blocks and hands the + // array straight to the provider, where `.trim()` threw and no request was + // ever built (issue #201). Blocks must classify exactly like the joined text. + // The cast is the point of these cases: the host contradicts the `string` + // declaration in Pi's Context type at runtime. + async function buildBody(systemPrompt: unknown) { + const { body } = await buildAnthropicRequest( + TEST_MODEL_ID, + { + messages: [userMsg('hello')], + systemPrompt, + tools: [], + } as unknown as Context, + undefined, + defaultCache, + ) + return body + } + + test('splits an ordered block array exactly like the joined prompt', async () => { + const blocks = PI_PROMPT.split('\n\n') + expect(blocks).toHaveLength(3) + + const fromBlocks = await buildBody(blocks) + const fromString = await buildBody(PI_PROMPT) + + expect(fromBlocks.system).toEqual(fromString.system) + expect(fromBlocks.messages).toEqual(fromString.messages) + expect(String(fromBlocks.system?.[2]?.text)).toContain('KEEP TWO') + const content = fromBlocks.messages[0]?.content as Array< + Record + > + expect(String(content[0]?.text)).toContain('MOVE THIS') + }) + + test('reads structured text blocks for their text', async () => { + const body = await buildBody( + PI_PROMPT.split('\n\n').map((text) => ({ type: 'text', text })), + ) + expect(String(body.system?.[2]?.text)).toContain('KEEP ONE') + const content = body.messages[0]?.content as Array> + expect(String(content[0]?.text)).toContain('MOVE THIS') + }) + + test('drops non-text blocks instead of flattening their metadata', async () => { + const paragraphs = PI_PROMPT.split('\n\n') + const body = await buildBody([ + { type: 'text', text: paragraphs[0] }, + { type: 'image', text: 'IMAGE METADATA' }, + { type: 'tool_use', name: 'read', text: 'TOOL METADATA' }, + paragraphs[1], + { type: 'text', text: paragraphs[2] }, + ]) + + const sent = JSON.stringify(body) + expect(sent).toContain('KEEP ONE') + expect(sent).toContain('KEEP TWO') + expect(sent).toContain('MOVE THIS') + expect(sent).not.toContain('IMAGE METADATA') + expect(sent).not.toContain('TOOL METADATA') + }) + + test('treats an empty block list as no prompt', async () => { + const body = await buildBody([]) + expect(body.system).toHaveLength(2) + expect(body.messages[0]).toEqual({ role: 'user', content: 'hello' }) + }) +}) + +describe('buildAnthropicRequest — cache breakpoint budget', () => { + // Anthropic accepts at most four cache breakpoints per request, and this + // converter places four itself: the last tool, the last system block, the + // cached prompt block on the first user message, and the last user block. + // Adding the top-level control on top of them made five and Anthropic + // rejected the request before it reached the model (issue #201). + async function buildBody(cache: { + enabled: boolean + mode: 'explicit' | 'automatic' | 'hybrid' + }) { + const { body } = await buildAnthropicRequest( + TEST_MODEL_ID, + { + messages: [userMsg('hello')], + systemPrompt: PI_PROMPT, + tools: [ + { + name: 'read', + description: 'read a file', + parameters: { properties: {}, required: [] }, + }, + ], + } satisfies Context, + undefined, + cache, + ) + return body + } + + const countBreakpoints = (body: unknown) => + JSON.stringify(body).split('"cache_control"').length - 1 + + test('places four breakpoints and no top-level control by default', async () => { + const body = await buildBody({ enabled: false, mode: 'hybrid' }) + expect(countBreakpoints(body)).toBe(4) + expect(body.cache_control).toBeUndefined() + }) + + test('keeps hybrid within the budget by extending those four to 1h', async () => { + const body = await buildBody({ enabled: true, mode: 'hybrid' }) + expect(countBreakpoints(body)).toBe(4) + expect(body.cache_control).toBeUndefined() + expect(body.system?.at(-1)?.cache_control).toEqual({ + type: 'ephemeral', + ttl: '1h', + }) + expect(body.tools?.at(-1)?.cache_control).toEqual({ + type: 'ephemeral', + ttl: '1h', + }) + }) + + test('spends the whole budget on the top-level control in automatic', async () => { + const body = await buildBody({ enabled: true, mode: 'automatic' }) + expect(countBreakpoints(body)).toBe(1) + expect(body.cache_control).toEqual({ type: 'ephemeral', ttl: '1h' }) + expect(body.system?.at(-1)?.cache_control).toBeUndefined() + expect(body.tools?.at(-1)?.cache_control).toBeUndefined() + }) + + // A tool parameter or tool argument that happens to be named cache_control is + // caller data, not a breakpoint. A deep walk would delete it in automatic and + // write ttl into it in hybrid/explicit, silently corrupting the tool contract. + test.each(['explicit', 'automatic', 'hybrid'] as const)( + 'leaves a tool parameter named cache_control untouched in %s', + async (mode) => { + const { body } = await buildAnthropicRequest( + TEST_MODEL_ID, + { + messages: [ + userMsg('hello'), + toolCallMsg('tool_1', 'store', { + cache_control: { type: 'ephemeral' }, + }), + toolResultMsg('tool_1', 'stored'), + ], + systemPrompt: PI_PROMPT, + tools: [ + { + name: 'store', + description: 'store a value', + parameters: { + properties: { + cache_control: { type: 'string', description: 'a header' }, + }, + required: [], + }, + }, + ], + } satisfies Context, + undefined, + { enabled: true, mode }, + ) + + const schema = body.tools?.[0]?.input_schema as { + properties: Record + } + expect(schema.properties.cache_control).toEqual({ + type: 'string', + description: 'a header', + }) + + const assistant = body.messages[1] as { + content: Array> + } + expect(assistant.content[0]?.input).toEqual({ + cache_control: { type: 'ephemeral' }, + }) + }, + ) +})