From f8d225352a105127083d0f03aeecb3a068771483 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 10 Jun 2026 13:43:56 -0700 Subject: [PATCH 1/5] fix(models): correctness batch for Harper 5.1 GA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five verified bugs found in the models backend: 1. hdb_model_calls phantom indexes: dropped `indexed: true` from all non-PK attributes in analyticsTable.ts. flush() and cleanup() write via primaryStore.put/remove which bypass updateIndices, so secondary indexes would stay permanently empty. Matches hdb_raw_analytics pattern. 2. Bedrock inference-profile model IDs: familyOf() now walks all dot- separated segments to find the first known family, resolving cross- region IDs like `us.anthropic.claude-3-5-sonnet-…` that previously resolved to family 'unknown'. Also added a descriptive error for amazon.nova-* models before they hit the wrong (Titan) body shape. 3. Abort gate at tool dispatch: added ctx.signal?.throwIfAborted() at the top of runSingleToolCall so pre-aborted signals are caught before any side-effecting handler starts. The serial path between multiple handlers also benefits since each handler entry now checks the signal. 4. OpenAI max_tokens vs max_completion_tokens: OpenAI's reasoning/gpt-5 models reject `max_tokens` with a 400. Send `max_completion_tokens` when talking to api.openai.com; keep `max_tokens` for any custom baseUrl to preserve compatibility with vLLM, Ollama-compat, and other OpenAI-compatible shims. 5. Upstream memory caps: a. Non-streaming bodies: replaced bare `await res.json()` in parseJsonResponse and readErrorSuffix (openai + anthropic) with a bounded streaming reader (64 MiB success cap, 256 KiB error cap). b. Tool-call accumulator cardinality: added a 128-entry cap on the streaming accumulator maps in all three backends (openai, anthropic, bedrock). `index` is upstream-controlled and without a cardinality cap a hostile stream can allocate unbounded map entries. Tests: 219 passing (was 317/1-pre-existing-EMFILE in the wider suite, unchanged). All new tests cover the specific failure modes described. Co-Authored-By: Claude Sonnet 4.6 --- components/anthropic/index.ts | 20 ++- components/bedrock/index.ts | 51 ++++++-- components/openai/index.ts | 49 ++++++-- resources/models/agentLoop.ts | 5 + resources/models/analyticsTable.ts | 22 ++-- resources/models/backendHelpers.ts | 67 +++++++++- unitTests/components/anthropic/index.test.js | 34 +++++ unitTests/components/bedrock/index.test.js | 118 ++++++++++++++++++ unitTests/components/openai/index.test.js | 72 ++++++++++- unitTests/resources/models/agentLoop.test.js | 88 +++++++++++++ .../resources/models/analyticsTable.test.js | 47 +++++++ .../resources/models/backendHelpers.test.js | 82 ++++++++++++ 12 files changed, 618 insertions(+), 37 deletions(-) diff --git a/components/anthropic/index.ts b/components/anthropic/index.ts index e1165d8524..b0a088beb1 100644 --- a/components/anthropic/index.ts +++ b/components/anthropic/index.ts @@ -25,8 +25,10 @@ import { setGenerative } from '../../resources/models/backendRegistry.ts'; import { assignFiniteTokenCount, composeSignal, + MAX_ERROR_BODY_BYTES, normalizeOrigin, parseJsonResponse, + readBoundedJson, requireCredential, requireModel, } from '../../resources/models/backendHelpers.ts'; @@ -59,6 +61,10 @@ const MAX_SSE_BUFFER_CHARS = 1 << 20; const MAX_TOOL_CALL_ARGS_CHARS = 1 << 20; // Cap for upstream `error.message` we surface to operators. const MAX_UPSTREAM_ERROR_MESSAGE_CHARS = 500; +// Maximum number of distinct tool-call accumulator entries. Anthropic keys by +// `index` from content_block_start events; a hostile upstream can open unbounded +// entries if content_block_stop never arrives for earlier indices. +const MAX_TOOL_CALL_ACCUMULATOR_ENTRIES = 128; const log = harperLogger.forComponent('anthropic').conditional; @@ -165,6 +171,13 @@ export class AnthropicBackend implements ModelBackend { event.index !== undefined && event.content_block?.type === 'tool_use' ) { + // Cap total accumulator entries: `index` is upstream-controlled and + // a content_block_stop may never arrive, leaking entries indefinitely. + if (toolBuf.size >= MAX_TOOL_CALL_ACCUMULATOR_ENTRIES) { + throw new AnthropicBackendError( + `Anthropic tool-call accumulator exceeded ${MAX_TOOL_CALL_ACCUMULATOR_ENTRIES} distinct tool-call entries` + ); + } toolBuf.set(event.index, { id: event.content_block.id, name: event.content_block.name, @@ -271,7 +284,12 @@ export class AnthropicBackendError extends ServerError { async function readErrorSuffix(res: Response): Promise { try { - const body = (await res.json()) as { error?: { message?: unknown; type?: unknown } }; + const body = await readBoundedJson<{ error?: { message?: unknown; type?: unknown } }>( + res, + 'Anthropic error response', + AnthropicBackendError, + MAX_ERROR_BODY_BYTES + ); const message = body?.error?.message; if (typeof message === 'string' && message.length > 0) { const truncated = diff --git a/components/bedrock/index.ts b/components/bedrock/index.ts index c3a7e99209..2efba40089 100644 --- a/components/bedrock/index.ts +++ b/components/bedrock/index.ts @@ -46,6 +46,10 @@ const DEFAULT_MAX_TOKENS = 4096; // Max accumulated `bytes` from streamed Claude tool-use input_json_delta; // matches the cap in `components/anthropic/index.ts`. const MAX_TOOL_CALL_ARGS_CHARS = 1 << 20; +// Maximum number of distinct tool-call accumulator entries. Upstream-controlled +// `index` values could otherwise allocate unbounded map entries if +// content_block_stop events never arrive. Matches the cap in the direct backends. +const MAX_TOOL_CALL_ACCUMULATOR_ENTRIES = 128; const log = harperLogger.forComponent('bedrock').conditional; @@ -199,7 +203,7 @@ export class BedrockBackend implements ModelBackend { const model = opts.model ?? this.#defaultModel; requireModel(model, 'generate', BedrockBackendError); const family = familyOf(model); - const body = buildGenerateBody(family, input, opts); + const body = buildGenerateBody(family, model, input, opts); const client = await this.#getClient(); const sdk = await loadSdk(); @@ -219,7 +223,7 @@ export class BedrockBackend implements ModelBackend { const model = opts.model ?? this.#defaultModel; requireModel(model, 'generateStream', BedrockBackendError); const family = familyOf(model); - const body = buildGenerateBody(family, input, opts); + const body = buildGenerateBody(family, model, input, opts); const client = await this.#getClient(); const sdk = await loadSdk(); @@ -288,13 +292,16 @@ export class BedrockBackendError extends ServerError { type Family = 'anthropic' | 'amazon' | 'meta' | 'cohere' | 'mistral' | 'unknown'; +const KNOWN_FAMILIES: Set = new Set(['anthropic', 'amazon', 'meta', 'cohere', 'mistral']); + function familyOf(modelId: string): Family { - const prefix = modelId.split('.', 1)[0]?.toLowerCase() ?? ''; - if (prefix === 'anthropic') return 'anthropic'; - if (prefix === 'amazon') return 'amazon'; - if (prefix === 'meta') return 'meta'; - if (prefix === 'cohere') return 'cohere'; - if (prefix === 'mistral') return 'mistral'; + // Cross-region inference-profile IDs are prefixed with a geographic segment + // (e.g. `us.anthropic.claude-3-5-sonnet-…`, `eu.meta.llama3-…`, `global.…`). + // Split on '.' and walk segments: the first segment that is a known family wins. + const segments = modelId.toLowerCase().split('.'); + for (const seg of segments) { + if (KNOWN_FAMILIES.has(seg as Family)) return seg as Family; + } return 'unknown'; } @@ -349,10 +356,28 @@ function extractEmbedResult( // ---------- generate body / result extraction ---------- -function buildGenerateBody(family: Family, input: GenerateInput, opts: BackendOpts): object { +/** + * amazon.nova-* models use the Converse API messages-v1 shape, not the Titan + * `inputText` shape. The Converse migration is a larger follow-up; for now + * throw a clear error so operators get actionable feedback instead of a + * malformed-request 400 from Bedrock. + */ +function rejectNovaModel(modelId: string): void { + if (modelId.toLowerCase().includes('nova')) { + throw new BedrockBackendError( + `amazon.nova models are not yet supported by the bedrock backend (model: ${modelId}); ` + + 'these models require the Converse API shape, not the legacy InvokeModel shape' + ); + } +} + +function buildGenerateBody(family: Family, modelId: string, input: GenerateInput, opts: BackendOpts): object { if (family === 'anthropic') return buildAnthropicBody(input, opts); if (family === 'meta') return buildLlamaBody(input, opts); - if (family === 'amazon') return buildTitanGenerateBody(input, opts); + if (family === 'amazon') { + rejectNovaModel(modelId); + return buildTitanGenerateBody(input, opts); + } if (family === 'mistral') return buildMistralBody(input, opts); if (family === 'cohere') return buildCohereGenerateBody(input, opts); throw new BedrockBackendError(`Bedrock generate not supported for model family '${family}'`); @@ -563,6 +588,12 @@ async function* parseAnthropicStream( } if (type === 'content_block_start' && index !== undefined && contentBlock?.type === 'tool_use') { + // Cap total accumulator entries; content_block_stop may never arrive. + if (toolBuf.size >= MAX_TOOL_CALL_ACCUMULATOR_ENTRIES) { + throw new BedrockBackendError( + `Bedrock tool-call accumulator exceeded ${MAX_TOOL_CALL_ACCUMULATOR_ENTRIES} distinct tool-call entries` + ); + } toolBuf.set(index, { id: contentBlock.id ?? '', name: contentBlock.name ?? '', argumentsBuf: '' }); } if (type === 'content_block_delta' && index !== undefined && delta) { diff --git a/components/openai/index.ts b/components/openai/index.ts index f07cbbd15f..8390c97c44 100644 --- a/components/openai/index.ts +++ b/components/openai/index.ts @@ -20,8 +20,10 @@ import { setEmbedding, setGenerative } from '../../resources/models/backendRegis import { assignFiniteTokenCount, composeSignal, + MAX_ERROR_BODY_BYTES, normalizeOrigin, parseJsonResponse, + readBoundedJson, requireCredential, requireModel, } from '../../resources/models/backendHelpers.ts'; @@ -60,6 +62,11 @@ const MAX_TOOL_CALL_ARGS_CHARS = 1 << 20; // cap defends against a misbehaving compat shim that returns megabytes of // "error" prose. const MAX_UPSTREAM_ERROR_MESSAGE_CHARS = 500; +// Maximum number of distinct tool-call accumulator entries. OpenAI keys by +// upstream-controlled `delta.index`; without a cardinality cap a hostile +// upstream can allocate unbounded map entries (one per index value). Real +// responses use single-digit counts. +const MAX_TOOL_CALL_ACCUMULATOR_ENTRIES = 128; const log = harperLogger.forComponent('openai').conditional; @@ -100,10 +107,15 @@ export class OpenAIBackend implements ModelBackend { readonly #organization?: string; readonly #requestTimeoutMs?: number; readonly #fetch: typeof fetch; + // True only when talking to api.openai.com itself. OpenAI's reasoning models + // (o-series, gpt-5 family) reject `max_tokens` in favour of `max_completion_tokens`; + // OpenAI-compatible shims (vLLM, Ollama-compat, older gateways) only know `max_tokens`. + readonly #isNativeOpenAI: boolean; constructor(config: OpenAIBackendConfig = {}, fetchImpl: typeof fetch = fetch) { this.#apiKey = requireCredential(config.apiKey, 'OpenAI', 'apiKey', OpenAIBackendError); this.#baseUrl = normalizeOrigin(config.baseUrl, { host: DEFAULT_BASE_URL, secure: true }); + this.#isNativeOpenAI = this.#baseUrl.startsWith('https://api.openai.com'); this.#defaultModel = config.model; this.#organization = config.organization; this.#requestTimeoutMs = config.requestTimeoutMs; @@ -147,7 +159,7 @@ export class OpenAIBackend implements ModelBackend { async generate(input: GenerateInput, opts: BackendOpts): Promise> { const model = opts.model ?? this.#defaultModel; requireModel(model, 'generate', OpenAIBackendError); - const body = buildChatRequest(model, input, opts, false); + const body = buildChatRequest(model, input, opts, false, this.#isNativeOpenAI); const res = await this.#post('/chat/completions', body, opts.signal); const data = await parseJsonResponse(res, 'OpenAI /chat/completions', OpenAIBackendError); const choice = data.choices?.[0]; @@ -173,7 +185,7 @@ export class OpenAIBackend implements ModelBackend { async *generateStream(input: GenerateInput, opts: BackendOpts): AsyncIterable { const model = opts.model ?? this.#defaultModel; requireModel(model, 'generateStream', OpenAIBackendError); - const body = buildChatRequest(model, input, opts, true); + const body = buildChatRequest(model, input, opts, true, this.#isNativeOpenAI); const res = await this.#post('/chat/completions', body, opts.signal); if (!res.body) throw new OpenAIBackendError('OpenAI /chat/completions returned no body for streaming'); @@ -249,7 +261,12 @@ export class OpenAIBackend implements ModelBackend { async function readErrorSuffix(res: Response): Promise { try { - const body = (await res.json()) as { error?: { message?: unknown; type?: unknown } }; + const body = await readBoundedJson<{ error?: { message?: unknown; type?: unknown } }>( + res, + 'OpenAI error response', + OpenAIBackendError, + MAX_ERROR_BODY_BYTES + ); const message = body?.error?.message; if (typeof message === 'string' && message.length > 0) { const truncated = @@ -292,7 +309,8 @@ function buildChatRequest( model: string, input: GenerateInput, opts: BackendOpts, - stream: boolean + stream: boolean, + isNativeOpenAI: boolean ): Record { const messages = normalizeMessages(input); const tools = extractTools(input); @@ -309,13 +327,15 @@ function buildChatRequest( } if (typeof opts.temperature === 'number') body.temperature = opts.temperature; if (typeof opts.maxTokens === 'number') { - // `max_tokens` is broadly supported across OpenAI and OpenAI-compatible - // endpoints. OpenAI is migrating to `max_completion_tokens` for o1/o3+ - // models but still accepts `max_tokens` on chat completions. Compat - // endpoints (Azure, vLLM, Together, OpenRouter) mostly accept the older - // field. Switch to `max_completion_tokens` when v1 models we ship - // against require it. - body.max_tokens = opts.maxTokens; + // api.openai.com's reasoning/gpt-5 models reject `max_tokens` (400); use + // `max_completion_tokens` there. OpenAI-compatible shims (vLLM, Ollama-compat, + // older gateways) only understand `max_tokens`, so keep the legacy field for + // any custom baseUrl. + if (isNativeOpenAI) { + body.max_completion_tokens = opts.maxTokens; + } else { + body.max_tokens = opts.maxTokens; + } } const responseFormat = mapResponseFormat(opts.responseFormat); if (responseFormat) body.response_format = responseFormat; @@ -432,6 +452,13 @@ function accumulateToolCallDelta(buf: Map, delta: O const index = typeof delta.index === 'number' ? delta.index : 0; let acc = buf.get(index); if (!acc) { + // Cap total accumulator entries: `index` is upstream-controlled and an + // adversarial stream can allocate unbounded map entries without this guard. + if (buf.size >= MAX_TOOL_CALL_ACCUMULATOR_ENTRIES) { + throw new OpenAIBackendError( + `OpenAI tool-call accumulator exceeded ${MAX_TOOL_CALL_ACCUMULATOR_ENTRIES} distinct tool-call entries` + ); + } acc = { argumentsBuf: '' }; buf.set(index, acc); } diff --git a/resources/models/agentLoop.ts b/resources/models/agentLoop.ts index ee993da11c..826c47a13d 100644 --- a/resources/models/agentLoop.ts +++ b/resources/models/agentLoop.ts @@ -363,6 +363,11 @@ async function runSingleToolCall( iteration: number, maxResultBytes: number ): Promise { + // Guard: don't start a side-effecting handler if the caller has already + // aborted. The catch below already rethrows AbortError, so a pre-aborted + // signal that fires after entry but before the handler await is also covered. + ctx.signal?.throwIfAborted(); + const entry: ToolTraceEntry = { iteration, toolCallId: call.id, diff --git a/resources/models/analyticsTable.ts b/resources/models/analyticsTable.ts index 33f6920594..1c38376bd9 100644 --- a/resources/models/analyticsTable.ts +++ b/resources/models/analyticsTable.ts @@ -55,20 +55,24 @@ export function getModelCallsTable(): any { audit: true, trackDeletes: false, attributes: [ + // flush() writes via tbl.primaryStore.put and cleanup() removes via + // primaryStore.remove — both bypass updateIndices, so secondary indexes + // would stay permanently empty. Match hdb_raw_analytics (write.ts) which + // intentionally omits `indexed` from all non-PK attributes for the same reason. { name: 'id', isPrimaryKey: true }, - { name: 'tenant', type: 'string', indexed: true }, - { name: 'app', type: 'string', indexed: true }, - { name: 'model', type: 'string', indexed: true }, - { name: 'backend', type: 'string', indexed: true }, - { name: 'method', type: 'string', indexed: true }, - { name: 'adapter', type: 'string', indexed: true }, - { name: 'conversation_id', type: 'string', indexed: true }, + { name: 'tenant', type: 'string' }, + { name: 'app', type: 'string' }, + { name: 'model', type: 'string' }, + { name: 'backend', type: 'string' }, + { name: 'method', type: 'string' }, + { name: 'adapter', type: 'string' }, + { name: 'conversation_id', type: 'string' }, { name: 'prompt_tokens', type: 'number' }, { name: 'completion_tokens', type: 'number' }, { name: 'embedding_tokens', type: 'number' }, { name: 'gpu_ms', type: 'number' }, - { name: 'latency_ms', type: 'number', indexed: true }, - { name: 'success', type: 'boolean', indexed: true }, + { name: 'latency_ms', type: 'number' }, + { name: 'success', type: 'boolean' }, { name: 'error_code', type: 'string' }, ], }); diff --git a/resources/models/backendHelpers.ts b/resources/models/backendHelpers.ts index 8b93c1527b..b448f2af76 100644 --- a/resources/models/backendHelpers.ts +++ b/resources/models/backendHelpers.ts @@ -46,19 +46,76 @@ export function assignFiniteTokenCount( usage[key] = value; } +// Body-read caps. A hostile or buggy upstream that returns a multi-GiB body +// would otherwise OOM the process before we reject the call. +// Success responses need room for large batch-embedding payloads (N×1536-dim +// float arrays); 64 MiB covers the largest realistic batch with headroom. +// Error responses are small prose strings; 256 KiB is generous. +export const MAX_RESPONSE_BODY_BYTES = 64 << 20; // 64 MiB +export const MAX_ERROR_BODY_BYTES = 256 << 10; // 256 KiB + +/** + * Read at most `maxBytes` from `res.body`, then JSON.parse. Throws the + * caller's error class — never a bare `SyntaxError` or `RangeError` — so + * the caller's `instanceof` checks stay consistent. + * + * A response whose body exceeds `maxBytes` is an explicit failure rather than + * a silent partial read; the caller's error class surfaces the diagnosis. + */ +export async function readBoundedJson( + res: Response, + endpoint: string, + Err: BackendErrorCtor, + maxBytes: number +): Promise { + if (!res.body) { + // No body at all — treat the same as invalid JSON. + throw new Err(`${endpoint} returned an empty response body`); + } + const chunks: Uint8Array[] = []; + let totalBytes = 0; + for await (const chunk of res.body as unknown as AsyncIterable) { + totalBytes += chunk.byteLength; + if (totalBytes > maxBytes) { + throw new Err( + `${endpoint} response body exceeds ${maxBytes}-byte limit (received >${totalBytes} bytes); ` + + 'rejecting to prevent unbounded memory use' + ); + } + chunks.push(chunk); + } + const merged = totalBytes === 0 ? '' : new TextDecoder('utf-8').decode( + chunks.length === 1 + ? chunks[0] + : (() => { + const buf = new Uint8Array(totalBytes); + let offset = 0; + for (const c of chunks) { + buf.set(c, offset); + offset += c.byteLength; + } + return buf; + })() + ); + try { + return JSON.parse(merged) as T; + } catch { + throw new Err(`${endpoint} returned a non-JSON response body`); + } +} + /** * Read a JSON response body and throw the backend's error class on parse * failure rather than leaking the raw `SyntaxError` (whose message can * include upstream-derived bytes). Matches the sanitization posture from * `analyticsTable.ts:35` ("Sanitized code (...). Never a raw upstream * message."). + * + * Caps the read at `MAX_RESPONSE_BODY_BYTES` (64 MiB) to bound memory use + * on hostile or misbehaving upstream endpoints. */ export async function parseJsonResponse(res: Response, endpoint: string, Err: BackendErrorCtor): Promise { - try { - return (await res.json()) as T; - } catch { - throw new Err(`${endpoint} returned a non-JSON response body`); - } + return readBoundedJson(res, endpoint, Err, MAX_RESPONSE_BODY_BYTES); } /** diff --git a/unitTests/components/anthropic/index.test.js b/unitTests/components/anthropic/index.test.js index 546094f1d3..24bc402d98 100644 --- a/unitTests/components/anthropic/index.test.js +++ b/unitTests/components/anthropic/index.test.js @@ -375,6 +375,40 @@ describe('AnthropicBackend', () => { }); }); +// ---- finding 5b: Anthropic streaming tool-call accumulator cardinality cap ------ + +describe('Anthropic streaming tool-call accumulator cardinality cap', () => { + it('throws AnthropicBackendError when more than 128 distinct content-block indices accumulate', async () => { + // Emit 129 content_block_start events for distinct tool_use indices without + // any content_block_stop events, so the map grows past the cap. + function bigToolStream() { + const enc = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + for (let i = 0; i < 129; i++) { + const event = { + type: 'content_block_start', + index: i, + content_block: { type: 'tool_use', id: `c${i}`, name: `fn${i}` }, + }; + controller.enqueue(enc.encode(`event: content_block_start\ndata: ${JSON.stringify(event)}\n\n`)); + } + controller.close(); + }, + }); + return new Response(stream, { status: 200, headers: { 'Content-Type': 'text/event-stream' } }); + } + const fetch = mockFetch(() => bigToolStream()); + const b = new AnthropicBackend({ apiKey: API_KEY, model: 'm' }, fetch); + await assert.rejects( + async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { /* drain */ } + }, + /tool-call accumulator exceeded 128/ + ); + }); +}); + describe('registerAnthropicBackend', () => { beforeEach(() => clearRegistry()); diff --git a/unitTests/components/bedrock/index.test.js b/unitTests/components/bedrock/index.test.js index 1fd387e6d7..daf54d7cb1 100644 --- a/unitTests/components/bedrock/index.test.js +++ b/unitTests/components/bedrock/index.test.js @@ -366,3 +366,121 @@ describe('registerBedrockBackend', () => { assert.strictEqual(resolveEmbedding('titan').name, 'bedrock'); }); }); + +// ---- finding 2: inference-profile model IDs + nova guard ------------------------- + +describe('familyOf (cross-region inference-profile IDs + nova)', () => { + beforeEach(() => _resetSdkCacheForTests()); + + // us.* inference profile — Claude is now exclusively on-demand via inference profiles + it('resolves us.anthropic.claude-* to anthropic family', async () => { + const { sdk } = fakeSdk(() => + jsonBodyResponse({ + content: [{ type: 'text', text: 'hi' }], + stop_reason: 'end_turn', + usage: {}, + }) + ); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'us.anthropic.claude-3-5-sonnet-20241022-v2:0' }); + // Must not throw on family resolution; should succeed and produce Anthropic body shape + const result = await b.generate('q', { accounting: ACCOUNTING }); + assert.strictEqual(result.output.content, 'hi'); + }); + + it('resolves eu.anthropic.claude-* to anthropic family', async () => { + const { sdk, sent } = fakeSdk(() => + jsonBodyResponse({ content: [{ type: 'text', text: 'bonjour' }], stop_reason: 'end_turn', usage: {} }) + ); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'eu-west-1', model: 'eu.anthropic.claude-3-5-haiku-20241022-v1:0' }); + const result = await b.generate('q', { accounting: ACCOUNTING }); + assert.strictEqual(result.output.content, 'bonjour'); + // Anthropic body shape: must have anthropic_version + const body = JSON.parse(sent[0].command.body); + assert.ok(body.anthropic_version, 'should use Anthropic body shape'); + }); + + it('resolves global.anthropic.* to anthropic family', async () => { + const { sdk } = fakeSdk(() => + jsonBodyResponse({ content: [{ type: 'text', text: 'x' }], stop_reason: 'end_turn', usage: {} }) + ); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'global.anthropic.claude-3-opus-20240229-v1:0' }); + // Should not throw on family dispatch + await b.generate('q', { accounting: ACCOUNTING }); + }); + + it('resolves us.meta.llama3-* to meta family', async () => { + const { sdk } = fakeSdk(() => + jsonBodyResponse({ generation: 'llama reply', stop_reason: 'stop', prompt_token_count: 1, generation_token_count: 2 }) + ); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'us.meta.llama3-70b-instruct-v1:0' }); + const result = await b.generate('q', { accounting: ACCOUNTING }); + assert.strictEqual(result.output.content, 'llama reply'); + }); + + it('still throws on a completely unknown vendor prefix', async () => { + const { sdk } = fakeSdk(() => jsonBodyResponse({})); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'unknownco.something-v1' }); + await assert.rejects( + () => b.generate('q', { accounting: ACCOUNTING }), + /not supported for model family 'unknown'/ + ); + }); + + it('throws a descriptive error for amazon.nova-* models (not yet supported)', async () => { + const { sdk } = fakeSdk(() => jsonBodyResponse({})); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'amazon.nova-pro-v1:0' }); + await assert.rejects( + () => b.generate('q', { accounting: ACCOUNTING }), + /amazon\.nova models are not yet supported/ + ); + }); + + it('throws a descriptive error for us.amazon.nova-* inference-profile IDs', async () => { + const { sdk } = fakeSdk(() => jsonBodyResponse({})); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'us.amazon.nova-micro-v1:0' }); + await assert.rejects( + () => b.generate('q', { accounting: ACCOUNTING }), + /amazon\.nova models are not yet supported/ + ); + }); +}); + +// ---- finding 5b: Bedrock streaming tool-call accumulator cardinality cap -------- + +describe('Bedrock Anthropic-stream tool-call accumulator cardinality cap', () => { + beforeEach(() => _resetSdkCacheForTests()); + + it('throws BedrockBackendError when more than 128 distinct content-block indices accumulate', async () => { + // Build a stream that emits 129 content_block_start tool_use events with + // distinct indices but no content_block_stop events. + async function* bigStream() { + for (let i = 0; i < 129; i++) { + yield { + chunk: { + bytes: new TextEncoder().encode(JSON.stringify({ + type: 'content_block_start', + index: i, + content_block: { type: 'tool_use', id: `c${i}`, name: `fn${i}` }, + })), + }, + }; + } + } + const { sdk } = fakeSdk(() => ({ body: bigStream() })); + _injectSdkForTests(sdk); + const b = new BedrockBackend({ region: 'us-east-1', model: 'anthropic.claude' }); + await assert.rejects( + async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { /* drain */ } + }, + /tool-call accumulator exceeded 128/ + ); + }); +}); diff --git a/unitTests/components/openai/index.test.js b/unitTests/components/openai/index.test.js index b05598f2a3..bc2f633cc6 100644 --- a/unitTests/components/openai/index.test.js +++ b/unitTests/components/openai/index.test.js @@ -376,13 +376,30 @@ describe('OpenAIBackend', () => { }); }); - it('maps temperature and maxTokens to OpenAI fields', async () => { + it('maps temperature and maxTokens: native OpenAI endpoint sends max_completion_tokens', async () => { + // api.openai.com reasoning/gpt-5 models reject `max_tokens` (400); + // send `max_completion_tokens` for the default endpoint. const fetch = mockFetch(() => chatResponse()); const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm' }, fetch); await b.generate('q', { accounting: ACCOUNTING, temperature: 0.5, maxTokens: 100 }); const sent = JSON.parse(fetch.calls[0].init.body); assert.strictEqual(sent.temperature, 0.5); + assert.strictEqual(sent.max_completion_tokens, 100); + assert.strictEqual(sent.max_tokens, undefined, 'max_tokens must not appear for native OpenAI'); + }); + + it('maps maxTokens to max_tokens for a custom baseUrl (compat shims only understand max_tokens)', async () => { + // OpenAI-compatible shims (vLLM, Ollama-compat, older gateways) only understand + // `max_tokens`; keep the legacy field for any non-api.openai.com endpoint. + const fetch = mockFetch(() => chatResponse()); + const b = new OpenAIBackend( + { apiKey: API_KEY, model: 'm', baseUrl: 'https://my-vllm.internal/v1' }, + fetch + ); + await b.generate('q', { accounting: ACCOUNTING, maxTokens: 100 }); + const sent = JSON.parse(fetch.calls[0].init.body); assert.strictEqual(sent.max_tokens, 100); + assert.strictEqual(sent.max_completion_tokens, undefined, 'max_completion_tokens must not appear for compat endpoint'); }); it("maps finish_reason='length' to finishReason='length'", async () => { @@ -769,6 +786,59 @@ describe('OpenAIBackend', () => { }); }); +// ---- finding 5b: tool-call accumulator cardinality cap -------------------------- + +describe('OpenAI streaming tool-call accumulator cardinality cap', () => { + it('throws OpenAIBackendError when more than 128 distinct tool-call indices arrive', async () => { + // Build an SSE response that emits 129 distinct `index` values without + // a `finish_reason`, exercising the accumulator-cardinality guard. + const encoder = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + for (let i = 0; i < 129; i++) { + const delta = { + choices: [ + { + delta: { + tool_calls: [{ index: i, id: `c${i}`, function: { name: `fn${i}`, arguments: '{}' } }], + }, + finish_reason: null, + }, + ], + }; + controller.enqueue(encoder.encode(`data: ${JSON.stringify(delta)}\n\n`)); + } + controller.close(); + }, + }); + const fetch = mockFetch(() => new Response(stream, { status: 200, headers: { 'Content-Type': 'text/event-stream' } })); + const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm' }, fetch); + await assert.rejects( + async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { /* drain */ } + }, + /tool-call accumulator exceeded 128/ + ); + }); + + it('does not throw for a normal response with a small number of tool calls', async () => { + const fetch = mockFetch(() => + sseResponse([ + { + choices: [{ + delta: { tool_calls: [{ index: 0, id: 'c0', function: { name: 'fn', arguments: '{"x":1}' } }] }, + finish_reason: 'tool_calls', + }], + }, + ]) + ); + const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm' }, fetch); + const chunks = []; + for await (const c of b.generateStream('q', { accounting: ACCOUNTING })) chunks.push(c); + assert.ok(chunks.some((c) => c.deltaToolCalls)); + }); +}); + describe('registerOpenAIBackend', () => { beforeEach(() => clearRegistry()); diff --git a/unitTests/resources/models/agentLoop.test.js b/unitTests/resources/models/agentLoop.test.js index 19818e86af..9ab5c19710 100644 --- a/unitTests/resources/models/agentLoop.test.js +++ b/unitTests/resources/models/agentLoop.test.js @@ -1621,3 +1621,91 @@ describe("agentLoop (toolMode: 'auto')", () => { }); }); }); + +// ---- finding 3: abort gate at tool dispatch (runSingleToolCall pre-check) ------- +describe('agentLoop abort gate at runSingleToolCall entry', () => { + let writer; + let models; + let backend; + + beforeEach(() => { + clearRegistry(); + writer = makeMockWriter(); + models = new Models(writer); + backend = new ScriptedBackend(); + setGenerative('default', backend); + }); + + afterEach(() => { + clearRegistry(); + }); + + it('serial dispatch: pre-aborted signal causes runSingleToolCall to throw before the handler runs', async () => { + // Queue a tool-call round followed by a final answer. + backend.queue( + toolCallRound('thinking', [tc('c1', 'sideEffect', {})]), + final('done') + ); + let handlerCallCount = 0; + const ctrl = new AbortController(); + // Pre-abort before calling generate. + ctrl.abort(new Error('pre-abort')); + await assert.rejects( + () => models.generate('q', { + toolMode: 'auto', + signal: ctrl.signal, + toolHandlers: { + sideEffect: () => { + handlerCallCount++; + return { ran: true }; + }, + }, + }), + // Must throw an AbortError, not BudgetExceededError. + (err) => { + assert.ok(err.name === 'AbortError' || err.code === 'ABORT_ERR' || ctrl.signal.aborted, + `expected AbortError, got ${err.name}: ${err.message}`); + return true; + } + ); + assert.strictEqual(handlerCallCount, 0, 'side-effecting handler must not run after abort'); + }); + + it('serial dispatch: signal aborted between two handlers stops at the second (first handler ran, second did not)', async () => { + const ctrl = new AbortController(); + let firstRan = false; + let secondRan = false; + + backend.queue( + // Two tool calls in one round. + toolCallRound('thinking', [tc('c1', 'first', {}), tc('c2', 'second', {})]), + final('done') + ); + // Abort inside the first handler so the second is pre-aborted when runSingleToolCall checks. + await assert.rejects( + () => models.generate('q', { + toolMode: 'auto', + signal: ctrl.signal, + // Force serial even for two calls. + toolParallelism: 'serial', + toolHandlers: { + first: () => { + firstRan = true; + ctrl.abort(new Error('abort-mid-dispatch')); + return { done: true }; + }, + second: () => { + secondRan = true; + return { done: true }; + }, + }, + }), + (err) => { + assert.ok(ctrl.signal.aborted, 'signal should be aborted'); + return true; + } + ); + assert.strictEqual(firstRan, true, 'first handler ran'); + assert.strictEqual(secondRan, false, 'second handler must not run after abort'); + }); +}); diff --git a/unitTests/resources/models/analyticsTable.test.js b/unitTests/resources/models/analyticsTable.test.js index 28e35be769..d28711f4e9 100644 --- a/unitTests/resources/models/analyticsTable.test.js +++ b/unitTests/resources/models/analyticsTable.test.js @@ -1,6 +1,8 @@ 'use strict'; const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); const { setTimeout: delay } = require('node:timers/promises'); const { ModelCallAnalyticsWriter } = require('#src/resources/models/analyticsTable'); @@ -255,3 +257,48 @@ describe('ModelCallAnalyticsWriter', () => { }); }); }); + +// ---- schema correctness (finding 1: phantom indexes) ---------------------------- +// flush() writes via primaryStore.put which bypasses updateIndices, so +// `indexed: true` on any non-PK attribute produces a permanently-empty index. +// Verify the schema declaration carries no `indexed` on non-PK attributes. +describe('getModelCallsTable schema', () => { + it('hdb_model_calls attributes do not carry indexed:true (would produce phantom empty indexes)', () => { + // Read the compiled/type-stripped source to avoid touching real LMDB. + // We check the TS source because the test suite runs with typestrip enabled. + const srcPath = path.resolve(__dirname, '../../../resources/models/analyticsTable.ts'); + const src = fs.readFileSync(srcPath, 'utf8'); + // Extract the attributes array block (everything between the first + // `attributes: [` and the matching `]`). + const attrMatch = src.match(/attributes:\s*\[([\s\S]*?)\],/); + assert.ok(attrMatch, 'could not locate attributes array in analyticsTable.ts'); + const attrBlock = attrMatch[1]; + // The primary key is the only entry allowed to carry `isPrimaryKey: true`. + // No entry should carry `indexed: true`. + assert.ok( + !attrBlock.includes('indexed: true') && !attrBlock.includes("indexed:true"), + `hdb_model_calls schema carries indexed: true on a non-PK attribute — ` + + `flush() bypasses updateIndices so the index would be permanently empty` + ); + }); + + it('flush still writes rows retrievable by their numeric id (PK scan)', async () => { + const { ModelCallAnalyticsWriter: Writer } = require('#src/resources/models/analyticsTable'); + const store = new Map(); + const mockTbl = { + primaryStore: { + put(id, record) { store.set(id, record); }, + remove(id) { store.delete(id); }, + getKeys() { return []; }, + }, + }; + const w = new Writer({ flushIntervalMs: 60_000, cleanupIntervalMs: 60_000, getTable: () => mockTbl }); + w.write({ backend: 'test', method: 'embed', latency_ms: 5, success: true }); + await w.flush(); + w.stop(); + assert.strictEqual(store.size, 1, 'expected one row after flush'); + const [row] = store.values(); + assert.strictEqual(row.backend, 'test'); + assert.ok(typeof row.id === 'number', 'row must have a numeric id (PK)'); + }); +}); diff --git a/unitTests/resources/models/backendHelpers.test.js b/unitTests/resources/models/backendHelpers.test.js index e4438ca314..d723b32fa3 100644 --- a/unitTests/resources/models/backendHelpers.test.js +++ b/unitTests/resources/models/backendHelpers.test.js @@ -5,6 +5,9 @@ const { composeSignal, assignFiniteTokenCount, parseJsonResponse, + readBoundedJson, + MAX_RESPONSE_BODY_BYTES, + MAX_ERROR_BODY_BYTES, requireModel, requireCredential, normalizeOrigin, @@ -238,3 +241,82 @@ describe('backendHelpers', () => { }); }); }); + +// ---- finding 5a: bounded body reader ------------------------------------------- + +/** + * Build a Response whose body is a ReadableStream that emits the given Uint8Array + * chunks in order. This exercises the streaming read path in readBoundedJson. + */ +function streamedResponse(chunks, { status = 200 } = {}) { + const stream = new ReadableStream({ + start(controller) { + for (const chunk of chunks) controller.enqueue(chunk); + controller.close(); + }, + }); + return new Response(stream, { status, headers: { 'Content-Type': 'application/json' } }); +} + +const enc = new TextEncoder(); + +describe('readBoundedJson', () => { + it('parses a normal JSON body under the cap', async () => { + const res = streamedResponse([enc.encode(JSON.stringify({ value: 42 }))]); + const body = await readBoundedJson(res, '/test', FakeBackendError, MAX_RESPONSE_BODY_BYTES); + assert.deepStrictEqual(body, { value: 42 }); + }); + + it('parses a body split across multiple chunks', async () => { + const payload = JSON.stringify({ hello: 'world' }); + // Split at byte 4 to ensure multi-chunk merge works. + const a = enc.encode(payload.slice(0, 4)); + const b = enc.encode(payload.slice(4)); + const res = streamedResponse([a, b]); + const result = await readBoundedJson(res, '/api', FakeBackendError, MAX_RESPONSE_BODY_BYTES); + assert.deepStrictEqual(result, { hello: 'world' }); + }); + + it('throws the backend error class when the body exceeds maxBytes', async () => { + // Build a body that is 3 bytes over a 10-byte cap. + const big = enc.encode('x'.repeat(13)); + const res = streamedResponse([big]); + await assert.rejects( + () => readBoundedJson(res, '/big', FakeBackendError, 10), + (err) => { + assert.ok(err instanceof FakeBackendError); + assert.ok(err.message.includes('/big'), 'error should name the endpoint'); + return true; + } + ); + }); + + it('throws the backend error class on invalid JSON (not a raw SyntaxError)', async () => { + const res = streamedResponse([enc.encode('not-valid-json')]); + await assert.rejects( + () => readBoundedJson(res, '/parse', FakeBackendError, MAX_RESPONSE_BODY_BYTES), + (err) => { + assert.ok(err instanceof FakeBackendError); + return true; + } + ); + }); + + it('throws the backend error class when the response has no body', async () => { + // Response with null body (e.g. HEAD response or server returning no content). + const res = new Response(null, { status: 200 }); + await assert.rejects( + () => readBoundedJson(res, '/nobody', FakeBackendError, MAX_RESPONSE_BODY_BYTES), + FakeBackendError + ); + }); + + it('parseJsonResponse uses the 64 MiB success-body cap', () => { + // The constant should be 64 MiB = 67108864 bytes. + assert.strictEqual(MAX_RESPONSE_BODY_BYTES, 64 * 1024 * 1024); + }); + + it('MAX_ERROR_BODY_BYTES is 256 KiB', () => { + assert.strictEqual(MAX_ERROR_BODY_BYTES, 256 * 1024); + }); +}); From e3acc6acb8ed576c4ea6c03a6ef7c90d5e90e347 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 10 Jun 2026 14:12:00 -0700 Subject: [PATCH 2/5] review fixes: structural test, URL-parsed native-OpenAI check, bounded-reader hardening, total tool-arg cap Gemini review findings adjudicated and applied: the analyticsTable schema test asserts on the exported attribute list instead of regexing the source; #isNativeOpenAI parses the URL hostname (port and spoofed-suffix safe); readBoundedJson wraps stream errors in the backend error class, cancels the body before throwing on the cap, and reuses a module-level TextDecoder; an 8 MiB total tool-argument cap bounds the accumulator maps across entries in all three streaming parsers. Co-Authored-By: Claude Fable 5 --- components/anthropic/index.ts | 9 ++ components/bedrock/index.ts | 16 ++- components/openai/index.ts | 32 ++++-- resources/models/analyticsTable.ts | 56 +++++----- resources/models/backendHelpers.ts | 58 ++++++---- unitTests/components/openai/index.test.js | 102 +++++++++++++++--- .../resources/models/analyticsTable.test.js | 50 +++++---- 7 files changed, 234 insertions(+), 89 deletions(-) diff --git a/components/anthropic/index.ts b/components/anthropic/index.ts index b0a088beb1..7ba1f29107 100644 --- a/components/anthropic/index.ts +++ b/components/anthropic/index.ts @@ -65,6 +65,8 @@ const MAX_UPSTREAM_ERROR_MESSAGE_CHARS = 500; // `index` from content_block_start events; a hostile upstream can open unbounded // entries if content_block_stop never arrives for earlier indices. const MAX_TOOL_CALL_ACCUMULATOR_ENTRIES = 128; +// Total tool-call argument chars across all content blocks in one stream. +const MAX_TOTAL_TOOL_CALL_ARGS_CHARS = 8 * 1024 * 1024; // 8 MiB const log = harperLogger.forComponent('anthropic').conditional; @@ -144,6 +146,7 @@ export class AnthropicBackend implements ModelBackend { // partial strings. const toolBuf = new Map(); let finalFinishReason: GenerateResult['finishReason'] | undefined; + let totalArgChars = 0; for await (const event of readSse(res.body)) { const chunk: GenerateChunk = {}; @@ -196,6 +199,12 @@ export class AnthropicBackend implements ModelBackend { `Anthropic tool-call arguments exceed ${MAX_TOOL_CALL_ARGS_CHARS} chars (index ${event.index})` ); } + totalArgChars += event.delta.partial_json.length; + if (totalArgChars > MAX_TOTAL_TOOL_CALL_ARGS_CHARS) { + throw new AnthropicBackendError( + `Anthropic tool-call arguments exceed total stream cap of ${MAX_TOTAL_TOOL_CALL_ARGS_CHARS} chars` + ); + } acc.argumentsBuf += event.delta.partial_json; } } diff --git a/components/bedrock/index.ts b/components/bedrock/index.ts index 2efba40089..6c5895ddb5 100644 --- a/components/bedrock/index.ts +++ b/components/bedrock/index.ts @@ -50,6 +50,8 @@ const MAX_TOOL_CALL_ARGS_CHARS = 1 << 20; // `index` values could otherwise allocate unbounded map entries if // content_block_stop events never arrive. Matches the cap in the direct backends. const MAX_TOOL_CALL_ACCUMULATOR_ENTRIES = 128; +// Total tool-call argument chars across all content blocks in one stream. +const MAX_TOTAL_TOOL_CALL_ARGS_CHARS = 8 * 1024 * 1024; // 8 MiB const log = harperLogger.forComponent('bedrock').conditional; @@ -371,7 +373,12 @@ function rejectNovaModel(modelId: string): void { } } -function buildGenerateBody(family: Family, modelId: string, input: GenerateInput, opts: BackendOpts): object { +function buildGenerateBody( + family: Family, + modelId: string, + input: GenerateInput, + opts: BackendOpts +): object { if (family === 'anthropic') return buildAnthropicBody(input, opts); if (family === 'meta') return buildLlamaBody(input, opts); if (family === 'amazon') { @@ -551,6 +558,7 @@ async function* parseAnthropicStream( const decoder = new TextDecoder('utf-8'); const toolBuf = new Map(); let finalFinishReason: GenerateResult['finishReason'] | undefined; + let totalArgChars = 0; for await (const event of body) { if (!event.chunk?.bytes) continue; @@ -607,6 +615,12 @@ async function* parseAnthropicStream( `Bedrock tool-call arguments exceed ${MAX_TOOL_CALL_ARGS_CHARS} chars (index ${index})` ); } + totalArgChars += delta.partial_json.length; + if (totalArgChars > MAX_TOTAL_TOOL_CALL_ARGS_CHARS) { + throw new BedrockBackendError( + `Bedrock tool-call arguments exceed total stream cap of ${MAX_TOTAL_TOOL_CALL_ARGS_CHARS} chars` + ); + } acc.argumentsBuf += delta.partial_json; } } diff --git a/components/openai/index.ts b/components/openai/index.ts index 8390c97c44..cb73d5e37e 100644 --- a/components/openai/index.ts +++ b/components/openai/index.ts @@ -67,6 +67,10 @@ const MAX_UPSTREAM_ERROR_MESSAGE_CHARS = 500; // upstream can allocate unbounded map entries (one per index value). Real // responses use single-digit counts. const MAX_TOOL_CALL_ACCUMULATOR_ENTRIES = 128; +// Total tool-call argument chars across all entries in one stream. The per-entry +// cap (1 MiB) plus the 128-entry cap still allows ~128 MiB accumulated; this cap +// keeps any single stream well-bounded. Real responses use tens of KB. +const MAX_TOTAL_TOOL_CALL_ARGS_CHARS = 8 * 1024 * 1024; // 8 MiB const log = harperLogger.forComponent('openai').conditional; @@ -115,7 +119,11 @@ export class OpenAIBackend implements ModelBackend { constructor(config: OpenAIBackendConfig = {}, fetchImpl: typeof fetch = fetch) { this.#apiKey = requireCredential(config.apiKey, 'OpenAI', 'apiKey', OpenAIBackendError); this.#baseUrl = normalizeOrigin(config.baseUrl, { host: DEFAULT_BASE_URL, secure: true }); - this.#isNativeOpenAI = this.#baseUrl.startsWith('https://api.openai.com'); + try { + this.#isNativeOpenAI = new URL(this.#baseUrl).hostname === 'api.openai.com'; + } catch { + this.#isNativeOpenAI = false; + } this.#defaultModel = config.model; this.#organization = config.organization; this.#requestTimeoutMs = config.requestTimeoutMs; @@ -196,6 +204,7 @@ export class OpenAIBackend implements ModelBackend { // never a partial string. const toolBuf = new Map(); let finalFinishReason: GenerateResult['finishReason'] | undefined; + let totalArgChars = 0; for await (const event of readSse(res.body)) { const choice = event.choices?.[0]; @@ -207,7 +216,7 @@ export class OpenAIBackend implements ModelBackend { } if (Array.isArray(delta?.tool_calls)) { for (const tcDelta of delta.tool_calls) { - accumulateToolCallDelta(toolBuf, tcDelta); + totalArgChars = accumulateToolCallDelta(toolBuf, tcDelta, totalArgChars); } } if (choice.finish_reason) { @@ -448,7 +457,11 @@ interface ToolCallAccumulator { argumentsBuf: string; } -function accumulateToolCallDelta(buf: Map, delta: OpenAIToolCallDelta): void { +function accumulateToolCallDelta( + buf: Map, + delta: OpenAIToolCallDelta, + totalArgChars: number +): number { const index = typeof delta.index === 'number' ? delta.index : 0; let acc = buf.get(index); if (!acc) { @@ -465,16 +478,23 @@ function accumulateToolCallDelta(buf: Map, delta: O if (delta.id) acc.id = delta.id; if (delta.function?.name) acc.name = delta.function.name; if (typeof delta.function?.arguments === 'string') { - // Defend against an unbounded accumulator: the per-event SSE buffer cap - // stops a single oversize event, but tool-call arguments are *built up* - // across many sub-cap events. Throw before V8 hits string-length limits. + // Per-entry cap: the per-event SSE buffer cap stops a single oversize event, + // but tool-call arguments are *built up* across many sub-cap events. if (acc.argumentsBuf.length + delta.function.arguments.length > MAX_TOOL_CALL_ARGS_CHARS) { throw new OpenAIBackendError( `OpenAI tool-call arguments exceed ${MAX_TOOL_CALL_ARGS_CHARS} chars (index ${index})` ); } + // Total-stream cap: 128 entries each at 1 MiB still allows ~128 MiB accumulated. + totalArgChars += delta.function.arguments.length; + if (totalArgChars > MAX_TOTAL_TOOL_CALL_ARGS_CHARS) { + throw new OpenAIBackendError( + `OpenAI tool-call arguments exceed total stream cap of ${MAX_TOTAL_TOOL_CALL_ARGS_CHARS} chars` + ); + } acc.argumentsBuf += delta.function.arguments; } + return totalArgChars; } function flushToolCallBuffer(buf: Map): Partial[] { diff --git a/resources/models/analyticsTable.ts b/resources/models/analyticsTable.ts index 1c38376bd9..03315464b4 100644 --- a/resources/models/analyticsTable.ts +++ b/resources/models/analyticsTable.ts @@ -7,10 +7,7 @@ const log = harperLogger.forComponent('models').conditional; const DEFAULT_FLUSH_INTERVAL_MS = 10_000; // 10s const DEFAULT_MAX_BUFFER_SIZE = 1000; const DEFAULT_CLEANUP_INTERVAL_MS = 60 * 60 * 1000; // 1h -// 90-day default tuned for billing windows. Operator-tunable config key will land -// in Phase 2 alongside the YAML→registry bootstrapper (Harper's `getConfigValue` -// only reads keys registered in `CONFIG_PARAM_MAP`, so we defer config plumbing -// until the first real backend ships and the key has a documented owner). +// 90-day default tuned for billing windows. const DEFAULT_RETENTION_MS = 90 * 24 * 60 * 60 * 1000; /** @@ -41,6 +38,35 @@ interface BufferedRecord extends ModelCallRecord { } let _table: any; + +/** + * Schema attributes for `hdb_model_calls`. Exported so tests can assert on + * the structural shape (e.g. no spurious `indexed: true`) without parsing + * TypeScript source. + * + * flush() writes via tbl.primaryStore.put and cleanup() removes via + * primaryStore.remove — both bypass updateIndices, so secondary indexes + * would stay permanently empty. Match hdb_raw_analytics (write.ts) which + * intentionally omits `indexed` from all non-PK attributes for the same reason. + */ +export const MODEL_CALL_ATTRIBUTES = [ + { name: 'id', isPrimaryKey: true }, + { name: 'tenant', type: 'string' }, + { name: 'app', type: 'string' }, + { name: 'model', type: 'string' }, + { name: 'backend', type: 'string' }, + { name: 'method', type: 'string' }, + { name: 'adapter', type: 'string' }, + { name: 'conversation_id', type: 'string' }, + { name: 'prompt_tokens', type: 'number' }, + { name: 'completion_tokens', type: 'number' }, + { name: 'embedding_tokens', type: 'number' }, + { name: 'gpu_ms', type: 'number' }, + { name: 'latency_ms', type: 'number' }, + { name: 'success', type: 'boolean' }, + { name: 'error_code', type: 'string' }, +]; + /** * Lazy-getter for `hdb_model_calls`. Matches the convention used by * `getRawAnalyticsTable()` / `getAnalyticsTable()` in @@ -54,27 +80,7 @@ export function getModelCallsTable(): any { database: 'system', audit: true, trackDeletes: false, - attributes: [ - // flush() writes via tbl.primaryStore.put and cleanup() removes via - // primaryStore.remove — both bypass updateIndices, so secondary indexes - // would stay permanently empty. Match hdb_raw_analytics (write.ts) which - // intentionally omits `indexed` from all non-PK attributes for the same reason. - { name: 'id', isPrimaryKey: true }, - { name: 'tenant', type: 'string' }, - { name: 'app', type: 'string' }, - { name: 'model', type: 'string' }, - { name: 'backend', type: 'string' }, - { name: 'method', type: 'string' }, - { name: 'adapter', type: 'string' }, - { name: 'conversation_id', type: 'string' }, - { name: 'prompt_tokens', type: 'number' }, - { name: 'completion_tokens', type: 'number' }, - { name: 'embedding_tokens', type: 'number' }, - { name: 'gpu_ms', type: 'number' }, - { name: 'latency_ms', type: 'number' }, - { name: 'success', type: 'boolean' }, - { name: 'error_code', type: 'string' }, - ], + attributes: MODEL_CALL_ATTRIBUTES, }); return _table; } diff --git a/resources/models/backendHelpers.ts b/resources/models/backendHelpers.ts index b448f2af76..88b2c26417 100644 --- a/resources/models/backendHelpers.ts +++ b/resources/models/backendHelpers.ts @@ -54,6 +54,9 @@ export function assignFiniteTokenCount( export const MAX_RESPONSE_BODY_BYTES = 64 << 20; // 64 MiB export const MAX_ERROR_BODY_BYTES = 256 << 10; // 256 KiB +// Module-level TextDecoder avoids per-call allocation in the streaming read path. +const BODY_DECODER = new TextDecoder('utf-8'); + /** * Read at most `maxBytes` from `res.body`, then JSON.parse. Throws the * caller's error class — never a bare `SyntaxError` or `RangeError` — so @@ -74,29 +77,42 @@ export async function readBoundedJson( } const chunks: Uint8Array[] = []; let totalBytes = 0; - for await (const chunk of res.body as unknown as AsyncIterable) { - totalBytes += chunk.byteLength; - if (totalBytes > maxBytes) { - throw new Err( - `${endpoint} response body exceeds ${maxBytes}-byte limit (received >${totalBytes} bytes); ` + - 'rejecting to prevent unbounded memory use' - ); + try { + for await (const chunk of res.body as unknown as AsyncIterable) { + totalBytes += chunk.byteLength; + if (totalBytes > maxBytes) { + // Release the connection promptly before throwing. + res.body?.cancel?.().catch(() => {}); + throw new Err( + `${endpoint} response body exceeds ${maxBytes}-byte limit (received >${totalBytes} bytes); ` + + 'rejecting to prevent unbounded memory use' + ); + } + chunks.push(chunk); } - chunks.push(chunk); + } catch (err) { + // Re-wrap unexpected stream errors (network abort, socket reset, etc.) + // so callers always see the backend's Err class, not a raw DOMException + // or Node stream error. Don't rewrap errors we already constructed above. + if (err instanceof Err) throw err; + throw new Err(`${endpoint} stream error while reading response body: ${(err as Error)?.message ?? err}`); } - const merged = totalBytes === 0 ? '' : new TextDecoder('utf-8').decode( - chunks.length === 1 - ? chunks[0] - : (() => { - const buf = new Uint8Array(totalBytes); - let offset = 0; - for (const c of chunks) { - buf.set(c, offset); - offset += c.byteLength; - } - return buf; - })() - ); + const merged = + totalBytes === 0 + ? '' + : BODY_DECODER.decode( + chunks.length === 1 + ? chunks[0] + : (() => { + const buf = new Uint8Array(totalBytes); + let offset = 0; + for (const c of chunks) { + buf.set(c, offset); + offset += c.byteLength; + } + return buf; + })() + ); try { return JSON.parse(merged) as T; } catch { diff --git a/unitTests/components/openai/index.test.js b/unitTests/components/openai/index.test.js index bc2f633cc6..ca66e2ae6b 100644 --- a/unitTests/components/openai/index.test.js +++ b/unitTests/components/openai/index.test.js @@ -392,14 +392,40 @@ describe('OpenAIBackend', () => { // OpenAI-compatible shims (vLLM, Ollama-compat, older gateways) only understand // `max_tokens`; keep the legacy field for any non-api.openai.com endpoint. const fetch = mockFetch(() => chatResponse()); + const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm', baseUrl: 'https://my-vllm.internal/v1' }, fetch); + await b.generate('q', { accounting: ACCOUNTING, maxTokens: 100 }); + const sent = JSON.parse(fetch.calls[0].init.body); + assert.strictEqual(sent.max_tokens, 100); + assert.strictEqual( + sent.max_completion_tokens, + undefined, + 'max_completion_tokens must not appear for compat endpoint' + ); + }); + + it('URL-parsed native-OpenAI detection: port-suffixed url still resolves as native', async () => { + // api.openai.com:443 should still classify as native — startsWith would + // miss it but URL-hostname check is correct. + const fetch = mockFetch(() => chatResponse()); + const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm', baseUrl: 'https://api.openai.com:443/v1' }, fetch); + await b.generate('q', { accounting: ACCOUNTING, maxTokens: 50 }); + const sent = JSON.parse(fetch.calls[0].init.body); + assert.strictEqual(sent.max_completion_tokens, 50, 'port-suffix url must still be treated as native'); + assert.strictEqual(sent.max_tokens, undefined); + }); + + it('URL-parsed native-OpenAI detection: subdomain-spoofed url is not native', async () => { + // api.openai.com.attacker.com starts with "https://api.openai.com" but its + // hostname is "api.openai.com.attacker.com" — URL parsing correctly rejects it. + const fetch = mockFetch(() => chatResponse()); const b = new OpenAIBackend( - { apiKey: API_KEY, model: 'm', baseUrl: 'https://my-vllm.internal/v1' }, + { apiKey: API_KEY, model: 'm', baseUrl: 'https://api.openai.com.attacker.com/v1' }, fetch ); - await b.generate('q', { accounting: ACCOUNTING, maxTokens: 100 }); + await b.generate('q', { accounting: ACCOUNTING, maxTokens: 50 }); const sent = JSON.parse(fetch.calls[0].init.body); - assert.strictEqual(sent.max_tokens, 100); - assert.strictEqual(sent.max_completion_tokens, undefined, 'max_completion_tokens must not appear for compat endpoint'); + assert.strictEqual(sent.max_tokens, 50, 'spoofed subdomain must not be treated as native'); + assert.strictEqual(sent.max_completion_tokens, undefined); }); it("maps finish_reason='length' to finishReason='length'", async () => { @@ -661,6 +687,49 @@ describe('OpenAIBackend', () => { }, /tool-call arguments exceed/); }); + it('throws when total tool-call argument chars across all entries exceed 8 MiB', async () => { + // Use many small-ish deltas per entry so no single SSE event hits the 1 MiB SSE + // buffer cap. Each delta is 512 KiB; 17 deltas = ~8.5 MiB total (> 8 MiB cap). + // They all go to the same tool-call index so the per-entry cap (1 MiB) would + // trip first — instead use 9 distinct indices, each with a 1-MiB payload split + // across two SSE events of 512 KiB each so no single event hits the SSE cap. + const half = 'x'.repeat(512 * 1024); // 512 KiB per SSE event + const encoder = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + // 9 indices × 2 events × 512 KiB = 9 MiB total (exceeds 8 MiB cap). + // Per-entry: each index gets 1 MiB total (exactly the per-entry cap). + // The total-stream cap trips before the 9th index finishes. + for (let i = 0; i < 9; i++) { + // First event: open the tool call with first half of arguments + const open = { + choices: [ + { delta: { tool_calls: [{ index: i, id: `c${i}`, function: { name: `fn${i}`, arguments: half } }] } }, + ], + }; + controller.enqueue( + encoder.encode(`data: ${JSON.stringify(open)}` + String.fromCharCode(10) + String.fromCharCode(10)) + ); + // Second event: second half pushes entry to 1 MiB (at the per-entry limit) + const cont = { choices: [{ delta: { tool_calls: [{ index: i, function: { arguments: half } }] } }] }; + controller.enqueue( + encoder.encode(`data: ${JSON.stringify(cont)}` + String.fromCharCode(10) + String.fromCharCode(10)) + ); + } + controller.close(); + }, + }); + const fetch = mockFetch( + () => new Response(stream, { status: 200, headers: { 'Content-Type': 'text/event-stream' } }) + ); + const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm' }, fetch); + await assert.rejects(async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { + /* drain */ + } + }, /total stream cap/); + }); + it('drops streamed tool calls with malformed JSON arguments', async () => { const fetch = mockFetch(() => sseResponse([ @@ -811,24 +880,27 @@ describe('OpenAI streaming tool-call accumulator cardinality cap', () => { controller.close(); }, }); - const fetch = mockFetch(() => new Response(stream, { status: 200, headers: { 'Content-Type': 'text/event-stream' } })); - const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm' }, fetch); - await assert.rejects( - async () => { - for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { /* drain */ } - }, - /tool-call accumulator exceeded 128/ + const fetch = mockFetch( + () => new Response(stream, { status: 200, headers: { 'Content-Type': 'text/event-stream' } }) ); + const b = new OpenAIBackend({ apiKey: API_KEY, model: 'm' }, fetch); + await assert.rejects(async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { + /* drain */ + } + }, /tool-call accumulator exceeded 128/); }); it('does not throw for a normal response with a small number of tool calls', async () => { const fetch = mockFetch(() => sseResponse([ { - choices: [{ - delta: { tool_calls: [{ index: 0, id: 'c0', function: { name: 'fn', arguments: '{"x":1}' } }] }, - finish_reason: 'tool_calls', - }], + choices: [ + { + delta: { tool_calls: [{ index: 0, id: 'c0', function: { name: 'fn', arguments: '{"x":1}' } }] }, + finish_reason: 'tool_calls', + }, + ], }, ]) ); diff --git a/unitTests/resources/models/analyticsTable.test.js b/unitTests/resources/models/analyticsTable.test.js index d28711f4e9..495ced073d 100644 --- a/unitTests/resources/models/analyticsTable.test.js +++ b/unitTests/resources/models/analyticsTable.test.js @@ -1,8 +1,6 @@ 'use strict'; const assert = require('node:assert/strict'); -const fs = require('node:fs'); -const path = require('node:path'); const { setTimeout: delay } = require('node:timers/promises'); const { ModelCallAnalyticsWriter } = require('#src/resources/models/analyticsTable'); @@ -261,35 +259,45 @@ describe('ModelCallAnalyticsWriter', () => { // ---- schema correctness (finding 1: phantom indexes) ---------------------------- // flush() writes via primaryStore.put which bypasses updateIndices, so // `indexed: true` on any non-PK attribute produces a permanently-empty index. -// Verify the schema declaration carries no `indexed` on non-PK attributes. +// Uses the exported MODEL_CALL_ATTRIBUTES const for a structural assertion that is +// not fragile to source reformatting or minification. +const { MODEL_CALL_ATTRIBUTES } = require('#src/resources/models/analyticsTable'); + describe('getModelCallsTable schema', () => { it('hdb_model_calls attributes do not carry indexed:true (would produce phantom empty indexes)', () => { - // Read the compiled/type-stripped source to avoid touching real LMDB. - // We check the TS source because the test suite runs with typestrip enabled. - const srcPath = path.resolve(__dirname, '../../../resources/models/analyticsTable.ts'); - const src = fs.readFileSync(srcPath, 'utf8'); - // Extract the attributes array block (everything between the first - // `attributes: [` and the matching `]`). - const attrMatch = src.match(/attributes:\s*\[([\s\S]*?)\],/); - assert.ok(attrMatch, 'could not locate attributes array in analyticsTable.ts'); - const attrBlock = attrMatch[1]; - // The primary key is the only entry allowed to carry `isPrimaryKey: true`. - // No entry should carry `indexed: true`. - assert.ok( - !attrBlock.includes('indexed: true') && !attrBlock.includes("indexed:true"), - `hdb_model_calls schema carries indexed: true on a non-PK attribute — ` + - `flush() bypasses updateIndices so the index would be permanently empty` + assert.ok(Array.isArray(MODEL_CALL_ATTRIBUTES), 'MODEL_CALL_ATTRIBUTES must be exported'); + const nonPk = MODEL_CALL_ATTRIBUTES.filter((a) => !a.isPrimaryKey); + const offenders = nonPk.filter((a) => a.indexed); + assert.strictEqual( + offenders.length, + 0, + 'hdb_model_calls schema carries indexed: true on non-PK attributes — ' + + 'flush() bypasses updateIndices so the index would be permanently empty. ' + + 'Offending: ' + + offenders.map((a) => a.name).join(', ') ); }); + it('the primary key attribute has isPrimaryKey: true and name "id"', () => { + const pk = MODEL_CALL_ATTRIBUTES.find((a) => a.isPrimaryKey); + assert.ok(pk, 'expected one isPrimaryKey attribute'); + assert.strictEqual(pk.name, 'id'); + }); + it('flush still writes rows retrievable by their numeric id (PK scan)', async () => { const { ModelCallAnalyticsWriter: Writer } = require('#src/resources/models/analyticsTable'); const store = new Map(); const mockTbl = { primaryStore: { - put(id, record) { store.set(id, record); }, - remove(id) { store.delete(id); }, - getKeys() { return []; }, + put(id, record) { + store.set(id, record); + }, + remove(id) { + store.delete(id); + }, + getKeys() { + return []; + }, }, }; const w = new Writer({ flushIntervalMs: 60_000, cleanupIntervalMs: 60_000, getTable: () => mockTbl }); From ed7feb2b90078a94e581ea0ef129aaeb89fa84bd Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 10 Jun 2026 22:07:21 -0600 Subject: [PATCH 3/5] fix(ci): format and lint fixes for models correctness batch - prettier reformatted three test files - rename unused `err` parameter to `_err` in agentLoop.test.js (oxlint no-unused-vars) Co-Authored-By: Claude Sonnet 4.6 --- unitTests/components/anthropic/index.test.js | 11 ++-- unitTests/components/bedrock/index.test.js | 35 ++++++----- unitTests/resources/models/agentLoop.test.js | 65 ++++++++++---------- 3 files changed, 57 insertions(+), 54 deletions(-) diff --git a/unitTests/components/anthropic/index.test.js b/unitTests/components/anthropic/index.test.js index 24bc402d98..c36af89303 100644 --- a/unitTests/components/anthropic/index.test.js +++ b/unitTests/components/anthropic/index.test.js @@ -400,12 +400,11 @@ describe('Anthropic streaming tool-call accumulator cardinality cap', () => { } const fetch = mockFetch(() => bigToolStream()); const b = new AnthropicBackend({ apiKey: API_KEY, model: 'm' }, fetch); - await assert.rejects( - async () => { - for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { /* drain */ } - }, - /tool-call accumulator exceeded 128/ - ); + await assert.rejects(async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { + /* drain */ + } + }, /tool-call accumulator exceeded 128/); }); }); diff --git a/unitTests/components/bedrock/index.test.js b/unitTests/components/bedrock/index.test.js index daf54d7cb1..2812fa3f98 100644 --- a/unitTests/components/bedrock/index.test.js +++ b/unitTests/components/bedrock/index.test.js @@ -413,7 +413,12 @@ describe('familyOf (cross-region inference-profile IDs + nova)', () => { it('resolves us.meta.llama3-* to meta family', async () => { const { sdk } = fakeSdk(() => - jsonBodyResponse({ generation: 'llama reply', stop_reason: 'stop', prompt_token_count: 1, generation_token_count: 2 }) + jsonBodyResponse({ + generation: 'llama reply', + stop_reason: 'stop', + prompt_token_count: 1, + generation_token_count: 2, + }) ); _injectSdkForTests(sdk); const b = new BedrockBackend({ region: 'us-east-1', model: 'us.meta.llama3-70b-instruct-v1:0' }); @@ -425,10 +430,7 @@ describe('familyOf (cross-region inference-profile IDs + nova)', () => { const { sdk } = fakeSdk(() => jsonBodyResponse({})); _injectSdkForTests(sdk); const b = new BedrockBackend({ region: 'us-east-1', model: 'unknownco.something-v1' }); - await assert.rejects( - () => b.generate('q', { accounting: ACCOUNTING }), - /not supported for model family 'unknown'/ - ); + await assert.rejects(() => b.generate('q', { accounting: ACCOUNTING }), /not supported for model family 'unknown'/); }); it('throws a descriptive error for amazon.nova-* models (not yet supported)', async () => { @@ -464,11 +466,13 @@ describe('Bedrock Anthropic-stream tool-call accumulator cardinality cap', () => for (let i = 0; i < 129; i++) { yield { chunk: { - bytes: new TextEncoder().encode(JSON.stringify({ - type: 'content_block_start', - index: i, - content_block: { type: 'tool_use', id: `c${i}`, name: `fn${i}` }, - })), + bytes: new TextEncoder().encode( + JSON.stringify({ + type: 'content_block_start', + index: i, + content_block: { type: 'tool_use', id: `c${i}`, name: `fn${i}` }, + }) + ), }, }; } @@ -476,11 +480,10 @@ describe('Bedrock Anthropic-stream tool-call accumulator cardinality cap', () => const { sdk } = fakeSdk(() => ({ body: bigStream() })); _injectSdkForTests(sdk); const b = new BedrockBackend({ region: 'us-east-1', model: 'anthropic.claude' }); - await assert.rejects( - async () => { - for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { /* drain */ } - }, - /tool-call accumulator exceeded 128/ - ); + await assert.rejects(async () => { + for await (const _c of b.generateStream('q', { accounting: ACCOUNTING })) { + /* drain */ + } + }, /tool-call accumulator exceeded 128/); }); }); diff --git a/unitTests/resources/models/agentLoop.test.js b/unitTests/resources/models/agentLoop.test.js index 9ab5c19710..82b5ce3cca 100644 --- a/unitTests/resources/models/agentLoop.test.js +++ b/unitTests/resources/models/agentLoop.test.js @@ -1642,29 +1642,29 @@ describe('agentLoop abort gate at runSingleToolCall entry', () => { it('serial dispatch: pre-aborted signal causes runSingleToolCall to throw before the handler runs', async () => { // Queue a tool-call round followed by a final answer. - backend.queue( - toolCallRound('thinking', [tc('c1', 'sideEffect', {})]), - final('done') - ); + backend.queue(toolCallRound('thinking', [tc('c1', 'sideEffect', {})]), final('done')); let handlerCallCount = 0; const ctrl = new AbortController(); // Pre-abort before calling generate. ctrl.abort(new Error('pre-abort')); await assert.rejects( - () => models.generate('q', { - toolMode: 'auto', - signal: ctrl.signal, - toolHandlers: { - sideEffect: () => { - handlerCallCount++; - return { ran: true }; + () => + models.generate('q', { + toolMode: 'auto', + signal: ctrl.signal, + toolHandlers: { + sideEffect: () => { + handlerCallCount++; + return { ran: true }; + }, }, - }, - }), + }), // Must throw an AbortError, not BudgetExceededError. (err) => { - assert.ok(err.name === 'AbortError' || err.code === 'ABORT_ERR' || ctrl.signal.aborted, - `expected AbortError, got ${err.name}: ${err.message}`); + assert.ok( + err.name === 'AbortError' || err.code === 'ABORT_ERR' || ctrl.signal.aborted, + `expected AbortError, got ${err.name}: ${err.message}` + ); return true; } ); @@ -1683,24 +1683,25 @@ describe('agentLoop abort gate at runSingleToolCall entry', () => { ); // Abort inside the first handler so the second is pre-aborted when runSingleToolCall checks. await assert.rejects( - () => models.generate('q', { - toolMode: 'auto', - signal: ctrl.signal, - // Force serial even for two calls. - toolParallelism: 'serial', - toolHandlers: { - first: () => { - firstRan = true; - ctrl.abort(new Error('abort-mid-dispatch')); - return { done: true }; - }, - second: () => { - secondRan = true; - return { done: true }; + () => + models.generate('q', { + toolMode: 'auto', + signal: ctrl.signal, + // Force serial even for two calls. + toolParallelism: 'serial', + toolHandlers: { + first: () => { + firstRan = true; + ctrl.abort(new Error('abort-mid-dispatch')); + return { done: true }; + }, + second: () => { + secondRan = true; + return { done: true }; + }, }, - }, - }), - (err) => { + }), + (_err) => { assert.ok(ctrl.signal.aborted, 'signal should be aborted'); return true; } From 1576b15cf426f7d4c8d7d7f7e2d23a500d423354 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Fri, 12 Jun 2026 16:50:03 -0700 Subject: [PATCH 4/5] Raise success-body cap to 256 MiB for maximal legal embedding batches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review: a legal OpenAI embedding batch (2048 inputs x 3072 dims) serializes to ~125-190 MiB of JSON, over the 64 MiB cap — successful calls would have been rejected. 256 MiB clears the largest legal batch while still bounding a runaway upstream. Co-Authored-By: Claude Fable 5 --- resources/models/backendHelpers.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/resources/models/backendHelpers.ts b/resources/models/backendHelpers.ts index 88b2c26417..b4586d5515 100644 --- a/resources/models/backendHelpers.ts +++ b/resources/models/backendHelpers.ts @@ -48,10 +48,12 @@ export function assignFiniteTokenCount( // Body-read caps. A hostile or buggy upstream that returns a multi-GiB body // would otherwise OOM the process before we reject the call. -// Success responses need room for large batch-embedding payloads (N×1536-dim -// float arrays); 64 MiB covers the largest realistic batch with headroom. +// Success responses must accommodate the largest LEGAL embedding batch: OpenAI +// accepts up to 2048 inputs per request, and text-embedding-3-large returns +// 3072 floats each — roughly 125-190 MiB of JSON. 256 MiB clears that with +// headroom while still bounding a runaway upstream. // Error responses are small prose strings; 256 KiB is generous. -export const MAX_RESPONSE_BODY_BYTES = 64 << 20; // 64 MiB +export const MAX_RESPONSE_BODY_BYTES = 256 << 20; // 256 MiB export const MAX_ERROR_BODY_BYTES = 256 << 10; // 256 KiB // Module-level TextDecoder avoids per-call allocation in the streaming read path. @@ -127,7 +129,7 @@ export async function readBoundedJson( * `analyticsTable.ts:35` ("Sanitized code (...). Never a raw upstream * message."). * - * Caps the read at `MAX_RESPONSE_BODY_BYTES` (64 MiB) to bound memory use + * Caps the read at `MAX_RESPONSE_BODY_BYTES` (256 MiB) to bound memory use * on hostile or misbehaving upstream endpoints. */ export async function parseJsonResponse(res: Response, endpoint: string, Err: BackendErrorCtor): Promise { From 14ed756c40641d0ba977fb156d41597247dba938 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 12 Jun 2026 20:50:44 -0600 Subject: [PATCH 5/5] test(models): update MAX_RESPONSE_BODY_BYTES assertion to 256 MiB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The constant was raised from 64 MiB to 256 MiB in this PR to handle large OpenAI embedding batch responses (125–190 MiB JSON). The unit test assertion was not updated alongside it. Co-Authored-By: Claude Sonnet 4.6 --- unitTests/resources/models/backendHelpers.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/unitTests/resources/models/backendHelpers.test.js b/unitTests/resources/models/backendHelpers.test.js index d723b32fa3..7e58c6d6d4 100644 --- a/unitTests/resources/models/backendHelpers.test.js +++ b/unitTests/resources/models/backendHelpers.test.js @@ -311,9 +311,9 @@ describe('readBoundedJson', () => { ); }); - it('parseJsonResponse uses the 64 MiB success-body cap', () => { - // The constant should be 64 MiB = 67108864 bytes. - assert.strictEqual(MAX_RESPONSE_BODY_BYTES, 64 * 1024 * 1024); + it('parseJsonResponse uses the 256 MiB success-body cap', () => { + // Raised from 64 MiB to 256 MiB to accommodate large OpenAI embedding batch responses (125–190 MiB JSON). + assert.strictEqual(MAX_RESPONSE_BODY_BYTES, 256 * 1024 * 1024); }); it('MAX_ERROR_BODY_BYTES is 256 KiB', () => {