From 8e9bf4dfa424f38f16bcf5c255d5249cb1ede453 Mon Sep 17 00:00:00 2001 From: Tao He Date: Mon, 5 Oct 2026 10:26:08 +0800 Subject: [PATCH 1/2] fix(images): cap images per request and recover sessions over the provider limit Inline images stay in persisted history and every request re-sends the whole provider-visible history, so a long session eventually exceeded the provider's per-request image limit ("Too many images in request: 31 > 30"). Every later request, /retry, /compact and a restarted session then failed the same way, each after the full BYOK retry window, behind a generic "temporarily unavailable / Run /retry" message. - Request-time image ceiling: projectAgentMessagesForModel keeps the newest N images across user messages and tool results and replaces older ones with a short placeholder naming the original file. Stored history is never rewritten. N comes from capabilities.max_images_per_request (default 20; api.mistral.ai defaults to its documented 8). Agent turns, checkpoint requests and footprint estimates share the projection. - Deterministic rejections are terminal: an image-count rejection, or an HTTP 400 invalid_request_error, is no longer retried by the BYOK retry-all path (timeouts, network, 408/429/5xx stay retryable). The TUI shows the upstream reason and suggests /compact or /new instead of /retry. - Compaction fallback: when the checkpoint Provider rejects a candidate for too many images, compaction retries the same history with media replaced by text (Hvideo) and continues down the attachment-free ladder. Refs #425 --- docs/examples.md | 17 + .../agent-core/src/pi-turn-runner/agent.ts | 16 + .../agent-core/src/pi-turn-runner/index.ts | 8 + .../src/pi-turn-runner/llm-retry.ts | 12 +- .../outbound-message-normalizer.ts | 12 +- .../src/pi-turn-runner/request-image-limit.ts | 198 ++++++++++ .../unit/pi-turn-runner/llm-retry.test.ts | 102 ++++- .../request-image-limit.test.ts | 356 ++++++++++++++++++ packages/config/src/config.ts | 2 + .../resolution/local-model-resolver.test.ts | 45 +++ .../resolution/local-model-resolver.ts | 14 +- .../model-system/resolution/model-ref.ts | 10 + .../algorithm/compact-context.test.ts | 140 ++++++- .../compaction/algorithm/compact-context.ts | 106 ++++++ .../turn-system/compaction/contracts.ts | 14 + .../execution/checkpoint-provider.test.ts | 77 +++- .../execution/checkpoint-provider.ts | 8 + packages/protocol/src/runtime.ts | 2 + packages/shared/src/llm-error-classifier.ts | 66 +++- .../runtime/runtime-error-presentation.ts | 33 +- ...ntime-error-presentation-rejection.test.ts | 69 ++++ release/public-source.json | 3 + test/vitest-suites.json | 2 + 23 files changed, 1302 insertions(+), 10 deletions(-) create mode 100644 packages/agent-core/src/pi-turn-runner/request-image-limit.ts create mode 100644 packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts create mode 100644 packages/tui/test/unit/runtime-error-presentation-rejection.test.ts diff --git a/docs/examples.md b/docs/examples.md index b732cfc18..ce7525613 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -154,6 +154,23 @@ text-only again, remove `image` from `modalities.input` and remove or set declare image input support. Restart MCode after editing configuration and select the configured model with `/provider`, or use `exec --model` for a single run. +Images stay in the conversation history, so a long session can carry more images +than a provider accepts in one request. MCode sends only the 20 most recent images +per request and replaces older ones with a short text note naming the original +file; the stored history keeps every image. If your provider allows fewer images +per request, set the limit on the model (Mistral's API is limited to 8 +automatically): + +```yaml +custom_provider: + my-vision-provider: + models: + my-vision-model: + capabilities: + support_image: true + max_images_per_request: 8 +``` + After signing in to MiniMax, try a task that explicitly requires search: > Use web_search to find the official Node.js test runner documentation. Summarize how to run tests and include the source URL. If the tool is unavailable, say so. diff --git a/packages/agent-core/src/pi-turn-runner/agent.ts b/packages/agent-core/src/pi-turn-runner/agent.ts index 72442120d..5a7bc1289 100644 --- a/packages/agent-core/src/pi-turn-runner/agent.ts +++ b/packages/agent-core/src/pi-turn-runner/agent.ts @@ -2,6 +2,7 @@ import { Agent } from '@earendil-works/pi-agent-core'; import type { UserMessage } from '@earendil-works/pi-ai'; import type { TurnTerminationReason } from '../event-bridge/types.js'; import { projectAgentMessagesForModel } from './outbound-message-normalizer.js'; +import { resolveMaxImagesPerRequest } from './request-image-limit.js'; import { failureReason } from './terminal.js'; import type { turnState } from './turn.js'; import type { UserMessageInput } from './types.js'; @@ -73,6 +74,21 @@ export function newAgent(turn: turnState): Agent { '[pi-turn-runner] kept provider-bound images whose dimensions could not be determined', ); } + if (outbound.omittedImageCount > 0) { + // History keeps every image; only this request copy drops the oldest + // ones so it stays within the model's per-request image limit (#425). + turn.logger.info( + { + event: 'outbound_images_limited', + session_id: turn.input.sessionId, + turn_id: turn.input.turnId, + provider: turn.llm.model.provider, + omitted_image_count: outbound.omittedImageCount, + max_images_per_request: resolveMaxImagesPerRequest(turn.llm.model), + }, + '[pi-turn-runner] replaced older provider-bound images with placeholders', + ); + } return outbound.messages; }, streamFn: turn.streamFn, diff --git a/packages/agent-core/src/pi-turn-runner/index.ts b/packages/agent-core/src/pi-turn-runner/index.ts index 7d826d5a6..c1bd76e2d 100644 --- a/packages/agent-core/src/pi-turn-runner/index.ts +++ b/packages/agent-core/src/pi-turn-runner/index.ts @@ -73,6 +73,14 @@ export { projectAgentMessagesForModel, removeOrphanToolResults, } from './outbound-message-normalizer.js'; +export { + DEFAULT_MAX_IMAGES_PER_REQUEST, + limitRequestImages, + knownMaxImagesPerRequestForBaseUrl, + normalizeMaxImagesPerRequest, + resolveMaxImagesPerRequest, +} from './request-image-limit.js'; +export type { ModelRequestImageLimit, RequestImageLimitResult } from './request-image-limit.js'; export { toPiUserMessage } from './agent.js'; export { createToolContextHistogramBucketsByName, diff --git a/packages/agent-core/src/pi-turn-runner/llm-retry.ts b/packages/agent-core/src/pi-turn-runner/llm-retry.ts index 29f4d79aa..fb8fa894d 100644 --- a/packages/agent-core/src/pi-turn-runner/llm-retry.ts +++ b/packages/agent-core/src/pi-turn-runner/llm-retry.ts @@ -7,6 +7,7 @@ import type { SimpleStreamOptions, } from '@earendil-works/pi-ai'; import { + isLLMDeterministicRequestRejection, normalizeLLMError, toLLMMetricErrorKind, toLLMProtocolClassification, @@ -579,12 +580,17 @@ function failureResult(input: { explicitAbort: input.final?.stopReason === 'aborted', }); // BYOK retries every pre-output failure because custom gateways report errors - // inconsistently, but a model safety refusal is deterministic: retrying the - // same request only repeats (and may re-bill) the decline. + // inconsistently, but some failures are deterministic: a model safety refusal, + // or a request the provider rejects as invalid (too many images, or an HTTP + // 400 `invalid_request_error`). Retrying those only re-sends the same request, + // repeats (and may re-bill) the rejection and delays the actionable error by + // the whole backoff window. The deterministic check excludes anything that + // also looks transient (timeout, network, 408/429/5xx). const decision = input.retryAllErrors && !normalized.facts.explicitAbort && - !normalized.facts.signals.has('refusal') + !normalized.facts.signals.has('refusal') && + !isLLMDeterministicRequestRejection(normalized) ? { retryable: true, reason: 'network' as const } : toLLMRetryDecision(normalized); if (!decision.retryable) { diff --git a/packages/agent-core/src/pi-turn-runner/outbound-message-normalizer.ts b/packages/agent-core/src/pi-turn-runner/outbound-message-normalizer.ts index ceef328b8..d4b196b1c 100644 --- a/packages/agent-core/src/pi-turn-runner/outbound-message-normalizer.ts +++ b/packages/agent-core/src/pi-turn-runner/outbound-message-normalizer.ts @@ -11,6 +11,7 @@ import type { import { convertToLlm } from '@earendil-works/pi-coding-agent/messages'; import { imageDimensions } from './image-dimensions.js'; +import { limitRequestImages, resolveMaxImagesPerRequest } from './request-image-limit.js'; const PRIOR_THINKING_OPEN = '<|prior-thinking|>'; const PRIOR_THINKING_CLOSE = '<|/prior-thinking|>'; @@ -52,6 +53,11 @@ export function projectAgentMessagesForModel( * cluster is the signal to teach `imageDimensions` another format. */ undeterminedImageMimeTypes: Record; + /** + * Older image blocks replaced by a placeholder so the request stays within + * the model's per-request image limit (see `request-image-limit.ts`). + */ + omittedImageCount: number; } { const providerVisible = messages.filter((message) => !isHostOnlyMessage(message)); const compatible = removeOrphanToolResults( @@ -65,9 +71,13 @@ export function projectAgentMessagesForModel( const projected = normalized .map(projectVideoBlocks) .map((message) => projectUndersizedImageBlocks(message, undetermined)); + // Last, so the ceiling counts exactly the images that would be sent: video + // frames projected to images count, undersized placeholders do not. + const limited = limitRequestImages(projected, resolveMaxImagesPerRequest(targetModel)); return { removedCount: compatible.removedCount, - messages: projected, + messages: limited.messages, + omittedImageCount: limited.omittedCount, undeterminedImageMimeTypes: Object.fromEntries(undetermined), }; } diff --git a/packages/agent-core/src/pi-turn-runner/request-image-limit.ts b/packages/agent-core/src/pi-turn-runner/request-image-limit.ts new file mode 100644 index 000000000..198420aef --- /dev/null +++ b/packages/agent-core/src/pi-turn-runner/request-image-limit.ts @@ -0,0 +1,198 @@ +import type { Api, Message, Model } from '@earendil-works/pi-ai'; + +/** + * Default ceiling on images in one provider request when the model declares + * none (`capabilities.max_images_per_request`). + * + * Inline images live in persisted history and every request re-sends the whole + * provider-visible history, so without a request-level ceiling a long session + * eventually exceeds the provider's per-request image limit. Every following + * request then fails the same way and the session cannot recover (#425: a + * gateway rejected `Too many images in request: 31 > 30`). + * + * 20 stays below the smallest general-purpose limit seen in practice — the + * gateway in #425 rejects more than 30 — with headroom for gateways that count + * images differently, and far below Anthropic (100 per request) and OpenAI + * (500). It still keeps the attachments of the last five turns (at most four + * inline images per user message) in view. Providers with a smaller limit set + * `max_images_per_request` on the model; Mistral's API (8) is recognized by + * host, see `knownMaxImagesPerRequestForBaseUrl`. + */ +export const DEFAULT_MAX_IMAGES_PER_REQUEST = 20; + +/** + * Optional request limit carried on the resolved model object, so every + * request builder that receives the model (agent turns, compaction + * checkpoints, footprint measurement) applies the same ceiling. + */ +export interface ModelRequestImageLimit { + readonly maxImagesPerRequest?: number; +} + +/** Normalize a configured image ceiling; anything but a positive integer is ignored. */ +export function normalizeMaxImagesPerRequest(value: unknown): number | undefined { + const parsed = + typeof value === 'number' + ? value + : typeof value === 'string' && value.trim() + ? Number(value) + : Number.NaN; + return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : undefined; +} + +/** + * Documented per-request image limits for first-party API hosts whose limit is + * below the default. Matched on the exact hostname so gateways that merely + * proxy these models keep the default unless they declare their own. + */ +const KNOWN_HOST_IMAGE_LIMITS: ReadonlyMap = new Map([ + // Mistral Vision FAQ (docs.mistral.ai/capabilities/vision): "The maximum number + // images per request via API is 8." + ['api.mistral.ai', 8], +]); + +/** Known image ceiling for a first-party API base URL, if it is below the default. */ +export function knownMaxImagesPerRequestForBaseUrl(baseUrl: string | undefined): number | undefined { + if (!baseUrl) return undefined; + try { + return KNOWN_HOST_IMAGE_LIMITS.get(new URL(baseUrl).hostname.toLowerCase()); + } catch { + return undefined; + } +} + +/** Image ceiling for requests to `model`: its declared limit, else the default. */ +export function resolveMaxImagesPerRequest(model: Model | undefined): number { + const declared = normalizeMaxImagesPerRequest( + (model as (Model & ModelRequestImageLimit) | undefined)?.maxImagesPerRequest, + ); + return declared ?? DEFAULT_MAX_IMAGES_PER_REQUEST; +} + +export interface RequestImageLimitResult { + readonly messages: Message[]; + /** Number of image blocks replaced by a placeholder in this request copy. */ + readonly omittedCount: number; +} + +/** + * Keep only the newest `maxImages` image blocks across the whole request + * (user messages and tool results) and replace older ones with a short text + * placeholder naming the original file when it is known. + * + * Operates on the temporary provider request copy only; canonical history is + * never rewritten, so raising the limit (or switching to a model with a higher + * one) brings the images back. + */ +export function limitRequestImages( + messages: readonly Message[], + maxImages: number, +): RequestImageLimitResult { + const limit = Math.max(0, Math.floor(maxImages)); + let total = 0; + for (const message of messages) total += imageBlockCount(message); + if (total <= limit) return { messages: [...messages], omittedCount: 0 }; + + let toOmit = total - limit; + const toolCallPaths = collectToolCallPaths(messages); + const projected = messages.map((message) => { + if (toOmit === 0) return message; + const count = imageBlockCount(message); + if (count === 0) return message; + const paths = imagePathsForMessage(message, count, toolCallPaths); + let imageIndex = 0; + const content = (message.content as Array<{ type?: unknown }>).map((block) => { + if (!isImageBlock(block)) return block; + const path = paths[imageIndex]; + imageIndex += 1; + if (toOmit === 0) return block; + toOmit -= 1; + return { type: 'text' as const, text: omittedImageText(limit, path) }; + }); + return { ...message, content } as Message; + }); + return { messages: projected, omittedCount: total - limit }; +} + +function omittedImageText(limit: number, path: string | undefined): string { + const scope = `this request keeps only the ${limit} most recent images`; + return path + ? `[Earlier image omitted: ${scope}. Original file: ${path}]` + : `[Earlier image omitted: ${scope}.]`; +} + +function isImageBlock(block: unknown): boolean { + return Boolean(block) && typeof block === 'object' && (block as { type?: unknown }).type === 'image'; +} + +function imageBlockCount(message: Message): number { + if (!Array.isArray(message.content)) return 0; + let count = 0; + for (const block of message.content as unknown[]) if (isImageBlock(block)) count += 1; + return count; +} + +const ATTACHMENT_TAG_RE = /]*)>/gu; +const ATTRIBUTE_RE = /([a-z_]+)="([^"]*)"/gu; +const PATH_ARGUMENT_KEYS = ['path', 'file_path', 'filePath'] as const; + +function imagePathsForMessage( + message: Message, + imageCount: number, + toolCallPaths: ReadonlyMap, +): Array { + if (message.role === 'toolResult') { + const path = toolCallPaths.get(message.toolCallId); + return imageCount === 1 && path ? [path] : []; + } + if (message.role !== 'user' || !Array.isArray(message.content)) return []; + // Attachment reminders list every attachment of the message in order; inline + // images are the ones sent as image blocks. Only trust the mapping when the + // counts agree, otherwise fall back to a placeholder without a path. + const inlineImagePaths: string[] = []; + for (const block of message.content) { + if (block.type !== 'text') continue; + for (const tag of block.text.matchAll(ATTACHMENT_TAG_RE)) { + const attributes = new Map(); + for (const attribute of (tag[1] ?? '').matchAll(ATTRIBUTE_RE)) { + attributes.set(attribute[1]!, unescapeAttribute(attribute[2]!)); + } + const path = attributes.get('path'); + if (attributes.get('inline') !== 'true' || !path) continue; + const kind = attributes.get('kind'); + const mime = attributes.get('mime') ?? ''; + if (kind === 'image' || (kind === undefined && mime.startsWith('image/'))) { + inlineImagePaths.push(path); + } + } + } + return inlineImagePaths.length === imageCount ? inlineImagePaths : []; +} + +function collectToolCallPaths(messages: readonly Message[]): Map { + const paths = new Map(); + for (const message of messages) { + if (message.role !== 'assistant' || !Array.isArray(message.content)) continue; + for (const block of message.content) { + if (block.type !== 'toolCall' || !block.arguments || typeof block.arguments !== 'object') { + continue; + } + for (const key of PATH_ARGUMENT_KEYS) { + const value: unknown = block.arguments[key]; + if (typeof value === 'string' && value.trim()) { + paths.set(block.id, value.trim()); + break; + } + } + } + } + return paths; +} + +function unescapeAttribute(value: string): string { + return value + .replaceAll('"', '"') + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('&', '&'); +} diff --git a/packages/agent-core/test/unit/pi-turn-runner/llm-retry.test.ts b/packages/agent-core/test/unit/pi-turn-runner/llm-retry.test.ts index 3aee4ffd9..ccfbf42f9 100644 --- a/packages/agent-core/test/unit/pi-turn-runner/llm-retry.test.ts +++ b/packages/agent-core/test/unit/pi-turn-runner/llm-retry.test.ts @@ -7,7 +7,13 @@ import type { Context, Model, } from "@earendil-works/pi-ai"; -import { LLM_ERROR_CODES } from "@mavis/shared/llm-error-classifier"; +import { + LLM_ERROR_CODES, + classifyLLMRequestRejectionMessage, + isLLMDeterministicRequestRejection, + isLLMImageLimitMessage, + normalizeLLMError, +} from "@mavis/shared/llm-error-classifier"; import { DEFAULT_LLM_RETRY_POLICY, LLM_RETRY_REQUEST_SETTLED_OBSERVER, @@ -574,6 +580,48 @@ describe("withLLMRetry", () => { expect(attempts).toBe(6); }); + // #425: a request the provider deterministically rejects (too many images, + // invalid_request_error) fails the same way on every resend, so the BYOK + // retry-all budget only delays the real reason by ~20 seconds. + it.each([ + ["custom_provider:work", "400 Upstream [invalid_request_error] Too many images in request: 31 > 30"], + ["minimax_api", "400 Upstream [invalid_request_error] Too many images in request: 31 > 30"], + ["custom_provider:work", '400 {"type":"invalid_request_error","message":"messages: text content blocks must be non-empty"}'], + ["fake-provider", "400 Too many images in request: 31 > 30"], + ])("does not retry a deterministic request rejection from %s: %s", async (provider, message) => { + let attempts = 0; + const sleep = vi.fn(async () => {}); + const inner = (async () => { + attempts += 1; + return errorStream(message); + }) as StreamFn; + const wrapped = withLLMRetry(inner, retryOptions({ sleep })); + + const events = await collectEvents(await wrapped(fakeModel(provider), CONTEXT, {})); + + expect(attempts).toBe(1); + expect(sleep).not.toHaveBeenCalled(); + const error = events.find((event) => event.type === "error"); + expect(error?.type === "error" ? error.error.errorMessage : undefined).toBe(message); + }); + + it.each([ + "[Error] 400 Bad Request", + "503 Service Unavailable: too many images queued, try again", + "429 rate limited while processing images", + ])("keeps the BYOK retry-all budget for transient or unclassified errors: %s", async (message) => { + let attempts = 0; + const inner = (async () => { + attempts += 1; + return errorStream(message); + }) as StreamFn; + const wrapped = withLLMRetry(inner, retryOptions({ sleep: vi.fn(async () => {}) })); + + await collectEvents(await wrapped(fakeModel("custom_provider:work"), CONTEXT, {})); + + expect(attempts).toBe(6); + }); + it("keeps the exhausted attempt open when returning its terminal failure stream", async () => { const attemptReturns: Array> = []; const inner = (async () => { @@ -1082,3 +1130,55 @@ describe("withLLMRetry", () => { ]); }); }); + +describe("deterministic request rejection classification", () => { + const rejection = (errorMessage: string, statusCode?: number) => + isLLMDeterministicRequestRejection(normalizeLLMError({ errorMessage, statusCode })); + + it("recognises provider image-count limits in common phrasings", () => { + for (const message of [ + "Upstream [invalid_request_error] Too many images in request: 31 > 30", + "A maximum of 8 images may be provided in one request.", + "Number of images exceeds the limit of 20", + "You can only include at most 100 images", + ]) { + expect(isLLMImageLimitMessage(message), message).toBe(true); + expect(classifyLLMRequestRejectionMessage(message), message).toBe("image_limit"); + } + expect(isLLMImageLimitMessage("image too large")).toBe(false); + }); + + it("treats image limits and HTTP 400 invalid_request_error as terminal", () => { + expect(rejection("400 Upstream [invalid_request_error] Too many images in request: 31 > 30")).toBe(true); + expect(rejection("Too many images in request: 31 > 30")).toBe(true); + expect(rejection("Too many images", 413)).toBe(true); + expect(rejection('400 {"type":"invalid_request_error","message":"bad"}')).toBe(true); + expect(classifyLLMRequestRejectionMessage('400 {"type":"invalid_request_error"}')).toBe("invalid_request"); + }); + + it("classifies the BYOK-attributed form the retry loop actually sees", () => { + // local-runtime-v2 wraps BYOK failures before withLLMRetry inspects them. + const byok = (detail: string) => + `BYOK upstream error: ${JSON.stringify({ + errorCode: LLM_ERROR_CODES.LLM_UPSTREAM_ERROR, + message: `BYOK provider custom_provider:fixture upstream error: ${detail}`, + errorSource: "byok_upstream", + errorDetail: detail, + errorProviderId: "custom_provider:fixture", + })}`; + expect(rejection(byok("400 Upstream [invalid_request_error] Too many images in request: 31 > 30"))).toBe(true); + expect(rejection(byok('400 {"type":"invalid_request_error","message":"bad"}'))).toBe(true); + expect(rejection(byok("400 Bad Request"))).toBe(false); + expect(rejection(byok("503 Service Unavailable"))).toBe(false); + }); + + it("keeps transient, unclassified or ambiguous errors retryable", () => { + expect(rejection("400 Bad Request")).toBe(false); + expect(rejection("invalid_request_error: overloaded")).toBe(false); + expect(rejection("Too many images", 503)).toBe(false); + expect(rejection("Too many images", 429)).toBe(false); + expect(rejection("invalid_request_error", 500)).toBe(false); + expect(rejection("401 invalid api key")).toBe(false); + expect(classifyLLMRequestRejectionMessage("invalid_request_error without a status")).toBeUndefined(); + }); +}); diff --git a/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts b/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts new file mode 100644 index 000000000..fec86938f --- /dev/null +++ b/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts @@ -0,0 +1,356 @@ +import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; + +import type { AgentMessage } from "@earendil-works/pi-agent-core"; +import type { AssistantMessage, Message, Model, ToolResultMessage, UserMessage } from "@earendil-works/pi-ai"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { projectAgentMessagesForModel } from "../../../src/pi-turn-runner/outbound-message-normalizer.js"; +import { PiTurnRunner } from "../../../src/pi-turn-runner/pi-turn-runner.js"; +import { + DEFAULT_MAX_IMAGES_PER_REQUEST, + limitRequestImages, + normalizeMaxImagesPerRequest, + resolveMaxImagesPerRequest, +} from "../../../src/pi-turn-runner/request-image-limit.js"; +import { + RuntimeEventStatus, + RuntimeEventType, + type RuntimeEvent, +} from "../../../src/protocol/runtime-event.js"; +import { NORMAL_40X24_PNG, THIN_432X2_JPEG } from "../image-fixtures.js"; + +// Regression coverage for #425: inline images live in persisted history and +// every request re-sends the whole history, so a long session eventually +// exceeded the provider's per-request image limit ("Too many images in +// request: 31 > 30") and every later request — even plain text — failed. + +const IMAGE = { type: "image" as const, data: NORMAL_40X24_PNG, mimeType: "image/png" }; + +function model(overrides: Record = {}): Model { + return { + id: "fixture-model", + name: "fixture-model", + api: "openai-completions", + provider: "custom_provider:fixture", + baseUrl: "https://example.invalid/v1", + reasoning: false, + input: ["text", "image"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 200_000, + maxTokens: 1_024, + ...overrides, + } as Model; +} + +function attachmentTag(path: string, inline: boolean): string { + return `\nUse the exact local path.\n`; +} + +function userWithImages(turn: number, imageCount: number, withReminder = true): UserMessage { + const paths = Array.from({ length: imageCount }, (_, index) => `/tmp/shots/turn${turn}-${index + 1}.png`); + const reminder = withReminder + ? `\n${paths.map((path) => attachmentTag(path, true)).join("\n")}\n\n` + : ""; + return { + role: "user", + content: [{ type: "text", text: `${reminder}turn ${turn}` }, ...paths.map(() => ({ ...IMAGE }))], + timestamp: turn, + }; +} + +function assistantReply(turn: number): AssistantMessage { + return { + role: "assistant", + content: [{ type: "text", text: `reply ${turn}` }], + api: "openai-completions", + provider: "custom_provider:fixture", + model: "fixture-model", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "stop", + timestamp: turn, + } as AssistantMessage; +} + +/** `turns` user turns with `perTurn` inline images each, plus assistant replies. */ +function imageHistory(turns: number, perTurn = 1): AgentMessage[] { + const history: AgentMessage[] = []; + for (let turn = 1; turn <= turns; turn += 1) { + history.push(userWithImages(turn, perTurn), assistantReply(turn)); + } + return history; +} + +function imageCount(messages: readonly Message[]): number { + return messages.reduce( + (total, message) => + total + + (Array.isArray(message.content) + ? message.content.filter((block) => (block as { type?: string }).type === "image").length + : 0), + 0, + ); +} + +function texts(message: Message): string[] { + return Array.isArray(message.content) + ? message.content.flatMap((block) => (block.type === "text" ? [block.text] : [])) + : [message.content]; +} + +describe("limitRequestImages", () => { + it("leaves a request within the limit untouched", () => { + const messages = imageHistory(3) as Message[]; + const result = limitRequestImages(messages, 3); + expect(result.omittedCount).toBe(0); + expect(result.messages).toEqual(messages); + }); + + it("keeps the newest images across the request and replaces older ones with placeholders", () => { + const messages = imageHistory(5, 2) as Message[]; + const result = limitRequestImages(messages, 3); + + expect(result.omittedCount).toBe(7); + expect(imageCount(result.messages)).toBe(3); + // Turns 1-3 lose every image; turn 4 keeps its second image; turn 5 keeps both. + const perUser = result.messages + .filter((message) => message.role === "user") + .map((message) => imageCount([message])); + expect(perUser).toEqual([0, 0, 0, 1, 2]); + const placeholders = texts(result.messages[0]!).slice(1); + expect(placeholders).toEqual([ + "[Earlier image omitted: this request keeps only the 3 most recent images. Original file: /tmp/shots/turn1-1.png]", + "[Earlier image omitted: this request keeps only the 3 most recent images. Original file: /tmp/shots/turn1-2.png]", + ]); + expect(texts(result.messages[6]!)[1]).toContain("/tmp/shots/turn4-1.png"); + }); + + it("never mutates the canonical messages", () => { + const messages = imageHistory(4) as Message[]; + const snapshot = structuredClone(messages); + limitRequestImages(messages, 1); + expect(messages).toEqual(snapshot); + }); + + it("counts tool-result images and names the file from the matching tool call", () => { + const messages: Message[] = [ + userWithImages(1, 1), + { + ...assistantReply(2), + content: [{ type: "toolCall", id: "call-1", name: "read", arguments: { path: "/tmp/shots/screen.png" } }], + stopReason: "toolUse", + } as AssistantMessage, + { + role: "toolResult", + toolCallId: "call-1", + toolName: "read", + content: [{ type: "text", text: "Read image file [image/png]" }, { ...IMAGE }], + isError: false, + timestamp: 3, + } satisfies ToolResultMessage, + { + ...assistantReply(4), + content: [{ type: "toolCall", id: "call-2", name: "browser", arguments: { action: "screenshot" } }], + stopReason: "toolUse", + } as AssistantMessage, + { + role: "toolResult", + toolCallId: "call-2", + toolName: "browser", + content: [{ ...IMAGE }], + isError: false, + timestamp: 5, + } satisfies ToolResultMessage, + userWithImages(6, 1), + ]; + + const result = limitRequestImages(messages, 1); + + expect(result.omittedCount).toBe(3); + expect(imageCount(result.messages)).toBe(1); + expect(imageCount([result.messages[5]!])).toBe(1); + expect(texts(result.messages[2]!)[1]).toBe( + "[Earlier image omitted: this request keeps only the 1 most recent images. Original file: /tmp/shots/screen.png]", + ); + expect(texts(result.messages[4]!)).toEqual([ + "[Earlier image omitted: this request keeps only the 1 most recent images.]", + ]); + }); + + it("omits the path when attachment reminders do not line up with the image blocks", () => { + const messages: Message[] = [userWithImages(1, 2, false), userWithImages(2, 1)]; + const result = limitRequestImages(messages, 1); + expect(texts(result.messages[0]!).slice(1)).toEqual([ + "[Earlier image omitted: this request keeps only the 1 most recent images.]", + "[Earlier image omitted: this request keeps only the 1 most recent images.]", + ]); + }); +}); + +describe("request image ceiling resolution", () => { + it("defaults to a conservative limit below the observed MiniMax gateway limit of 30", () => { + expect(DEFAULT_MAX_IMAGES_PER_REQUEST).toBeLessThanOrEqual(30); + expect(resolveMaxImagesPerRequest(model())).toBe(DEFAULT_MAX_IMAGES_PER_REQUEST); + }); + + it("honours a positive integer declared on the model and ignores invalid values", () => { + expect(resolveMaxImagesPerRequest(model({ maxImagesPerRequest: 8 }))).toBe(8); + expect(resolveMaxImagesPerRequest(model({ maxImagesPerRequest: 0 }))).toBe(DEFAULT_MAX_IMAGES_PER_REQUEST); + expect(resolveMaxImagesPerRequest(model({ maxImagesPerRequest: 2.5 }))).toBe( + DEFAULT_MAX_IMAGES_PER_REQUEST, + ); + expect(normalizeMaxImagesPerRequest("12")).toBe(12); + expect(normalizeMaxImagesPerRequest("many")).toBeUndefined(); + }); +}); + +describe("projectAgentMessagesForModel image ceiling", () => { + it("caps a 31-image history at the default ceiling without touching canonical history", () => { + const history = imageHistory(31); + const snapshot = structuredClone(history); + const projected = projectAgentMessagesForModel(history, model()); + + expect(imageCount(projected.messages)).toBe(DEFAULT_MAX_IMAGES_PER_REQUEST); + expect(projected.omittedImageCount).toBe(31 - DEFAULT_MAX_IMAGES_PER_REQUEST); + expect(history).toEqual(snapshot); + // The newest turn keeps its image; the oldest one is a placeholder naming its file. + expect(imageCount([projected.messages.at(-2)!])).toBe(1); + expect(texts(projected.messages[0]!).at(-1)).toContain("/tmp/shots/turn1-1.png"); + }); + + it("uses the ceiling declared on the model", () => { + const projected = projectAgentMessagesForModel(imageHistory(31), model({ maxImagesPerRequest: 8 })); + expect(imageCount(projected.messages)).toBe(8); + expect(projected.omittedImageCount).toBe(23); + }); + + it("does not count images already replaced for being undersized", () => { + const history = imageHistory(3); + (history[0] as UserMessage).content = [ + { type: "text", text: "thin" }, + { type: "image", data: THIN_432X2_JPEG, mimeType: "image/jpeg" }, + ]; + const projected = projectAgentMessagesForModel(history, model({ maxImagesPerRequest: 2 })); + expect(imageCount(projected.messages)).toBe(2); + expect(projected.omittedImageCount).toBe(0); + }); +}); + +describe("text-only turn after more than 30 historical images (real HTTP transport)", () => { + const LIMIT = 30; + let server: ReturnType; + let baseUrl: string; + let requestImages: number[]; + + function respond(request: IncomingMessage, response: ServerResponse): void { + let raw = ""; + request.setEncoding("utf8"); + request.on("data", (chunk: string) => { + raw += chunk; + }); + request.on("end", () => { + const body = JSON.parse(raw) as { messages?: Array<{ content?: unknown }> }; + const images = (body.messages ?? []).reduce( + (total, message) => + total + + (Array.isArray(message.content) + ? message.content.filter((part: { type?: string }) => part?.type === "image_url").length + : 0), + 0, + ); + requestImages.push(images); + if (images > LIMIT) { + response.writeHead(400, { "content-type": "application/json" }); + response.end( + JSON.stringify({ + error: { + type: "invalid_request_error", + message: `Upstream [invalid_request_error] Too many images in request: ${images} > ${LIMIT}`, + }, + }), + ); + return; + } + response.writeHead(200, { "content-type": "text/event-stream" }); + response.write( + `data: ${JSON.stringify({ + id: "fixture", + object: "chat.completion.chunk", + model: "fixture-model", + choices: [{ index: 0, delta: { content: "PONG" }, finish_reason: null }], + })}\n\n`, + ); + response.end( + `data: ${JSON.stringify({ + choices: [{ index: 0, delta: {}, finish_reason: "stop" }], + usage: { prompt_tokens: 3, completion_tokens: 1, total_tokens: 4 }, + })}\n\ndata: [DONE]\n\n`, + ); + }); + } + + beforeEach(async () => { + requestImages = []; + server = createServer(respond); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", resolve); + }); + baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}/v1`; + }); + + afterEach(async () => { + server.closeAllConnections(); + await new Promise((resolve) => server.close(() => resolve())); + }); + + async function runTextTurn(turnModel: Model, history: AgentMessage[]) { + const events: RuntimeEvent[] = []; + await new PiTurnRunner().runTurn({ + sessionId: "session-425-images", + turnId: "turn-text", + workspaceDir: process.cwd(), + systemPrompt: "You are a test.", + userMessage: { text: "hi" }, + history, + llm: { model: turnModel, apiKey: "synthetic" }, + llmRetry: { policy: { maxRetries: 5, baseDelayMs: 1, maxDelayMs: 1 } }, + eventWriter: { + pushRuntime: (event) => { + events.push(event); + }, + appendEvents: (batch) => { + events.push(...batch); + }, + }, + toolConfig: { tools: [], disableBuiltinToolFallback: true, context: {} as never }, + }); + const status = events.filter((event) => event.type === RuntimeEventType.SESSION_STATUS).at(-1); + return status?.payload; + } + + it("sends at most the ceiling and the turn succeeds", async () => { + const status = await runTextTurn(model({ baseUrl }), imageHistory(44)); + + expect(status?.status).toBe(RuntimeEventStatus.COMPLETED); + expect(requestImages).toEqual([DEFAULT_MAX_IMAGES_PER_REQUEST]); + }); + + it("fails a provider rejection for too many images once, without BYOK retries", async () => { + // A model that declares a higher ceiling than its gateway accepts still + // hits the rejection; it must surface immediately instead of being + // re-sent through the whole BYOK backoff window. + const status = await runTextTurn(model({ baseUrl, maxImagesPerRequest: 100 }), imageHistory(31)); + + expect(status?.status).toBe(RuntimeEventStatus.FAILED); + expect(JSON.stringify(status)).toContain("Too many images in request: 31 > 30"); + expect(requestImages).toEqual([31]); + }); +}); diff --git a/packages/config/src/config.ts b/packages/config/src/config.ts index 702de23db..db95f7f81 100644 --- a/packages/config/src/config.ts +++ b/packages/config/src/config.ts @@ -1076,6 +1076,8 @@ export interface ModelCapabilitiesConfig { max_video_bytes_inline?: number | string; max_request_body_bytes?: number | string; max_attachments_count?: number | string; + /** Most images one provider request may carry; older history images become placeholders. */ + max_images_per_request?: number | string; [key: string]: unknown; } diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts index bdc1b053c..50e6c42db 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts @@ -1610,3 +1610,48 @@ describe('LocalModelResolver custom provider session affinity', () => { expect(headers['x-session-affinity']).toBeUndefined(); }); }); + +describe('LocalModelResolver request image ceiling (#425)', () => { + const resolveWith = async (modelConfig: LocalModelConfig, baseURL = 'https://gateway.example/v1') => { + const resolver = new LocalModelResolver({ + byokConfigGetter: () => ({ + custom_provider: { + gateway: { + api: 'openai-completions', + options: { apiKey: 'gateway-key', baseURL }, + models: { vision: modelConfig }, + }, + }, + }), + }); + const resolved = await resolver.resolveModel({ + sessionId: 'session-image-ceiling', + turnId: 'turn-image-ceiling', + agentConfig: { + ...AGENT_CONFIG, + model: modelRefForModel('custom_provider:gateway', 'vision', modelConfig), + }, + }); + return (resolved.model as { maxImagesPerRequest?: number }).maxImagesPerRequest; + }; + + it('carries a configured max_images_per_request onto the executor model', async () => { + expect(await resolveWith({ capabilities: { support_image: true, max_images_per_request: 6 } })).toBe(6); + expect(await resolveWith({ capabilities: { support_image: true, max_images_per_request: '12' } })).toBe(12); + }); + + it('leaves the shared default in charge when nothing is declared or the value is invalid', async () => { + expect(await resolveWith({ capabilities: { support_image: true } })).toBeUndefined(); + expect(await resolveWith({ capabilities: { support_image: true, max_images_per_request: 0 } })).toBeUndefined(); + }); + + it('applies the documented Mistral API limit by exact host unless the model overrides it', async () => { + expect(await resolveWith({ capabilities: { support_image: true } }, 'https://api.mistral.ai/v1')).toBe(8); + expect( + await resolveWith({ capabilities: { support_image: true, max_images_per_request: 4 } }, 'https://api.mistral.ai/v1'), + ).toBe(4); + expect( + await resolveWith({ capabilities: { support_image: true } }, 'https://gateway.example/api.mistral.ai/v1'), + ).toBeUndefined(); + }); +}); diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts index 21d4eea4a..3d5bce42d 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts @@ -6,6 +6,11 @@ import { type ThinkingLevelMap, } from '@earendil-works/pi-ai'; import type { StreamFn, ThinkingLevel as PiThinkingLevel } from '@earendil-works/pi-agent-core'; +import { + knownMaxImagesPerRequestForBaseUrl, + normalizeMaxImagesPerRequest, + type ModelRequestImageLimit, +} from '@mavis/agent-core/pi-turn-runner'; import { isFirstPartyMinimaxMessagesRoute, resolveProviderAuthMode, @@ -624,7 +629,10 @@ function buildResolvedModel(scope: { const { input, contextWindow, maxTokens, baseUrl, thinking } = scope; const thinkingLevelMap = resolvedThinkingLevelMap(thinking); const compat = resolvedModelCompatibility(input, thinking); - return { + const maxImagesPerRequest = + normalizeMaxImagesPerRequest(input.modelRef.capabilities?.max_images_per_request) ?? + knownMaxImagesPerRequestForBaseUrl(baseUrl); + const model: Model & ModelRequestImageLimit = { id: input.modelId, name: input.modelId, api: input.api, @@ -637,7 +645,11 @@ function buildResolvedModel(scope: { contextWindow, maxTokens, ...(compat ? { compat } : {}), + // Request builders read this from the model so agent turns, compaction + // checkpoints and footprint estimates share one image ceiling (#425). + ...(maxImagesPerRequest === undefined ? {} : { maxImagesPerRequest }), }; + return model; } function resolvedThinkingLevelMap(thinking: ResolvedThinking): ThinkingLevelMap | undefined { diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/model-ref.ts b/packages/local-runtime-v2/src/service/model-system/resolution/model-ref.ts index ff18c4ced..f8ad44191 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/model-ref.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/model-ref.ts @@ -1,5 +1,6 @@ import { isDefaultThinkingModelId, type Api, type ThinkingLevelMap } from '@earendil-works/pi-ai'; import type { ThinkingLevel as PiThinkingLevel } from '@earendil-works/pi-agent-core'; +import { normalizeMaxImagesPerRequest } from '@mavis/agent-core/pi-turn-runner'; import { ThinkingLevel, ThinkingMode, @@ -500,6 +501,7 @@ export function capabilitiesFromModelConfig( modalities.includes('video') || modelConfig?.capabilities?.support_video === true, ...normalizeLocalFileApiCapabilities(modelConfig?.capabilities), ...normalizeLocalMultimodalLimitCapabilities(modelConfig?.capabilities), + ...maxImagesPerRequestCapability(modelConfig?.capabilities?.max_images_per_request), ...(modelConfig?.capabilities?.support_json_object_output === true ? { [SUPPORT_JSON_OBJECT_OUTPUT_CAPABILITY]: true } : {}), @@ -514,6 +516,14 @@ export function capabilitiesFromModelConfig( return capabilities; } +/** Per-request image ceiling (#425); absent unless the model config declares a positive integer. */ +function maxImagesPerRequestCapability( + value: unknown, +): Pick { + const maxImages = normalizeMaxImagesPerRequest(value); + return maxImages === undefined ? {} : { max_images_per_request: maxImages }; +} + export function supportsJsonObjectOutput(modelRef: IModelRef): boolean { const capabilities: LocalModelRefCapabilities | undefined = modelRef.capabilities; return capabilities?.[SUPPORT_JSON_OBJECT_OUTPUT_CAPABILITY] === true; diff --git a/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.test.ts b/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.test.ts index 59c9d75b1..723fe252a 100644 --- a/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.test.ts +++ b/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.test.ts @@ -3,7 +3,10 @@ import { describe, expect, it, vi } from 'vitest'; import type { CheckpointAttemptMetadata } from '../../agent-host/contracts.js'; import { readCompactionCompatibility } from '../compat.js'; -import { CheckpointCandidateTooLargeError } from '../contracts.js'; +import { + CheckpointCandidateMediaRejectedError, + CheckpointCandidateTooLargeError, +} from '../contracts.js'; import { compactContext, type CheckpointSession, @@ -640,6 +643,141 @@ describe('compactContext historical video recovery', () => { }); }); +describe('compactContext provider image-limit recovery (#425)', () => { + function userWithImage(text: string, data: string, timestamp: number): AgentMessage { + return { + role: 'user', + content: [ + { type: 'text', text }, + { type: 'image', data, mimeType: 'image/png' }, + ], + timestamp, + }; + } + + it('falls back to the attachment-free candidate when the Provider rejects too many images', async () => { + const h0 = [ + userWithImage('screenshot one', 'secret-image-1', 0), + assistantText('seen one', 1), + userWithImage('screenshot two', 'secret-image-2', 2), + assistantText('seen two', 3), + Object.assign(user('display wrapper', 4), { genuineUserQueryText: 'latest query' }), + ]; + const generate = vi + .fn() + .mockRejectedValueOnce( + new CheckpointCandidateMediaRejectedError( + new Error('400 Upstream [invalid_request_error] Too many images in request: 31 > 30'), + ), + ) + .mockResolvedValueOnce(generation()); + const attempts: CheckpointAttemptMetadata[] = []; + const input = policyInput(h0, { generate, fits: () => true }); + + const decision = await compactContext({ + ...input, + checkpoint: { ...input.checkpoint, onAttemptSettled: (metadata) => attempts.push(metadata) }, + }); + + expect(decision).toMatchObject({ method: 'llm_checkpoint', generationAttempts: 2 }); + expect(generate).toHaveBeenCalledTimes(2); + const fallback = JSON.stringify(generate.mock.calls[1]?.[0]?.messages); + expect(fallback).not.toContain('secret-image-'); + expect(fallback).not.toMatch(/"type":"image"/); + expect(fallback).toContain('screenshot one'); + expect(attempts.map(({ candidate, attemptNumber, outcome }) => `${candidate}:${attemptNumber}:${outcome}`)).toEqual([ + 'h0:1:failed', + 'hvideo:2:generated', + ]); + // Canonical history is never rewritten. + expect(JSON.stringify(h0)).toContain('secret-image-1'); + }); + + it('continues down the attachment-free ladder when the image-free candidate overflows', async () => { + const h0 = [ + userWithImage('old query', 'secret-image-1', 0), + ...threeRounds(), + Object.assign(user('display wrapper', 7), { genuineUserQueryText: 'latest query' }), + ]; + const generate = vi + .fn() + .mockRejectedValueOnce(new CheckpointCandidateMediaRejectedError(new Error('Too many images'))) + .mockRejectedValueOnce(new CheckpointCandidateTooLargeError('Hvideo overflow')) + .mockResolvedValueOnce(generation()); + const attempts: CheckpointAttemptMetadata[] = []; + const input = policyInput(h0, { + measurePair: async () => footprint(1_000, 301), + generate, + fits: () => true, + }); + + const decision = await compactContext({ + ...input, + checkpoint: { ...input.checkpoint, onAttemptSettled: (metadata) => attempts.push(metadata) }, + }); + + expect(decision).toMatchObject({ method: 'llm_checkpoint' }); + const last = JSON.stringify(generate.mock.calls.at(-1)?.[0]?.messages); + expect(last).not.toContain('secret-image-1'); + // Hall clears the tool bodies, so its attachment-free form is a new, smaller request. + expect(attempts.map(({ candidate }) => candidate)).toEqual(['h0', 'hvideo', 'hvideo']); + }); + + it('does not resend an identical attachment-free history after it overflows', async () => { + const h0 = [ + userWithImage('old query', 'secret-image-1', 0), + assistantText('answer', 1), + Object.assign(user('display wrapper', 2), { genuineUserQueryText: 'latest query' }), + ]; + const generate = vi + .fn() + .mockRejectedValueOnce(new CheckpointCandidateMediaRejectedError(new Error('Too many images'))) + .mockRejectedValueOnce(new CheckpointCandidateTooLargeError('Hvideo overflow')) + .mockResolvedValue(generation()); + const attempts: CheckpointAttemptMetadata[] = []; + const input = policyInput(h0, { generate, fits: () => true }); + + await compactContext({ + ...input, + checkpoint: { ...input.checkpoint, onAttemptSettled: (metadata) => attempts.push(metadata) }, + }).catch(() => undefined); + + expect(attempts.filter(({ candidate }) => candidate === 'hvideo')).toHaveLength(1); + }); + + it('fails as a Provider failure when the rejected candidate carries no media to strip', async () => { + const h0 = [ + user('old query', 0), + assistantText('answer', 1), + Object.assign(user('display wrapper', 2), { genuineUserQueryText: 'latest query' }), + ]; + const generate = vi + .fn() + .mockRejectedValue(new CheckpointCandidateMediaRejectedError(new Error('Too many images'))); + + await expect(compactContext(policyInput(h0, { generate, fits: () => true }))).rejects.toMatchObject({ + code: 'CHECKPOINT_PROVIDER_FAILED', + message: expect.stringContaining('too many images'), + }); + expect(generate).toHaveBeenCalledTimes(1); + }); + + it('fails as a Provider failure when the image-free candidate is rejected again', async () => { + const h0 = [ + userWithImage('old query', 'secret-image-1', 0), + Object.assign(user('display wrapper', 1), { genuineUserQueryText: 'latest query' }), + ]; + const generate = vi + .fn() + .mockRejectedValue(new CheckpointCandidateMediaRejectedError(new Error('Too many images'))); + + await expect(compactContext(policyInput(h0, { generate, fits: () => true }))).rejects.toMatchObject({ + code: 'CHECKPOINT_PROVIDER_FAILED', + }); + expect(generate).toHaveBeenCalledTimes(2); + }); +}); + describe('compactContext output exhaustion', () => { it('advances the same ladder when a candidate exhausts its output budget', async () => { const h0 = [ diff --git a/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.ts b/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.ts index 98ac0199f..172bc5095 100644 --- a/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.ts +++ b/packages/local-runtime-v2/src/service/turn-system/compaction/algorithm/compact-context.ts @@ -6,6 +6,7 @@ import type { CompactionTokenUsage, } from '../../agent-host/contracts.js'; import { + CheckpointCandidateMediaRejectedError, CheckpointCandidateTooLargeError, ContextCompactionError, type ContextCompactionSizeDiagnostics, @@ -354,6 +355,22 @@ async function generateCheckpoint( ...tokenUsageMetadata(input.checkpoint.getTokenUsage?.()), }; } catch (cause) { + if (cause instanceof CheckpointCandidateMediaRejectedError) { + return recoverAfterMediaRejection({ + input, + session, + rejectedMessages: checkpointCandidate.messages, + hallMessages: hall.messages, + // Hall differs from the rejected candidate only when it cleared tool + // results and was not itself the rejected candidate. + hallIsDistinct: + hall.trimmedResultCount > 0 && checkpointCandidate.messages !== hall.messages, + instructions, + counter, + priorOverflow: overflow, + cause, + }); + } if (!(cause instanceof CheckpointCandidateTooLargeError)) throw cause; overflow = cause; } @@ -369,6 +386,92 @@ async function generateCheckpoint( }); } +/** + * The Provider refused a candidate for the images it carries. Every candidate + * before the attachment-free one keeps those images, so skip straight to the + * same history with media replaced by text; if that still overflows, continue + * down the regular attachment-free ladder (Hvideo, Hmid, Hmin) from Hall. + * Without this, /compact on a session over the provider's image limit failed + * with the same rejection the session itself was stuck on (#425). + */ +async function recoverAfterMediaRejection({ + input, + session, + rejectedMessages, + hallMessages, + hallIsDistinct, + instructions, + counter, + priorOverflow, + cause, +}: { + readonly input: CompactContextInput; + readonly session: CheckpointSession; + readonly rejectedMessages: readonly AgentMessage[]; + readonly hallMessages: readonly AgentMessage[]; + readonly hallIsDistinct: boolean; + readonly instructions: string | undefined; + readonly counter: { value: number }; + readonly priorOverflow: CheckpointCandidateTooLargeError | undefined; + readonly cause: CheckpointCandidateMediaRejectedError; +}): ReturnType { + input.signal?.throwIfAborted(); + const attachmentFree = buildAttachmentFreeCandidate(rejectedMessages); + if (attachmentFree.replacedBlockCount === 0) throw mediaRejectedProviderFailure(cause); + let overflow = priorOverflow; + let attachmentFreeAttempted = false; + try { + if (session.fits(checkpointRequest(input, attachmentFree.messages, instructions))) { + attachmentFreeAttempted = true; + try { + const settled = await generateCandidateWithProviderRetry({ + input, + session, + candidate: 'hvideo', + counter, + messages: attachmentFree.messages, + instructions, + }); + return { + generation: settled.generation, + attempts: settled.attemptNumber, + maxOutputTokens: session.maxOutputTokens, + ...tokenUsageMetadata(input.checkpoint.getTokenUsage?.()), + }; + } catch (next) { + if (!(next instanceof CheckpointCandidateTooLargeError)) throw next; + overflow = next; + } + } + return await recoverAfterHall({ + input, + session, + hallMessages, + instructions, + counter, + priorOverflow: overflow, + // Do not send the same attachment-free history twice. + skipHvideo: attachmentFreeAttempted && !hallIsDistinct, + }); + } catch (next) { + if (next instanceof CheckpointCandidateMediaRejectedError) { + throw mediaRejectedProviderFailure(next); + } + throw next; + } +} + +function mediaRejectedProviderFailure( + cause: CheckpointCandidateMediaRejectedError, +): ContextCompactionError { + return new ContextCompactionError( + 'CHECKPOINT_PROVIDER_FAILED', + 'llm_checkpoint', + 'Checkpoint Provider rejected the request for carrying too many images.', + { cause }, + ); +} + function toolResultReplacementCount( candidate: ToolResultCompactionCandidate | ToolTrimCandidate, ): number { @@ -384,6 +487,7 @@ async function recoverAfterHall({ instructions, counter, priorOverflow, + skipHvideo = false, }: { readonly input: CompactContextInput; readonly session: CheckpointSession; @@ -391,6 +495,7 @@ async function recoverAfterHall({ readonly instructions: string | undefined; readonly counter: { value: number }; readonly priorOverflow: CheckpointCandidateTooLargeError | undefined; + readonly skipHvideo?: boolean; }): Promise<{ readonly generation: CheckpointGeneration; readonly attempts: number; @@ -402,6 +507,7 @@ async function recoverAfterHall({ const hvideo = buildAttachmentFreeCandidate(hallMessages); let overflow = priorOverflow; if ( + !skipHvideo && hvideo.replacedBlockCount > 0 && session.fits(checkpointRequest(input, hvideo.messages, instructions)) ) { diff --git a/packages/local-runtime-v2/src/service/turn-system/compaction/contracts.ts b/packages/local-runtime-v2/src/service/turn-system/compaction/contracts.ts index f48a5a08e..fd7e5a560 100644 --- a/packages/local-runtime-v2/src/service/turn-system/compaction/contracts.ts +++ b/packages/local-runtime-v2/src/service/turn-system/compaction/contracts.ts @@ -101,3 +101,17 @@ export class CheckpointCandidateTooLargeError extends Error { ); } } + +/** + * The Provider rejected the checkpoint request for the images it carries (for + * example `Too many images in request: 31 > 30`). Re-sending the same + * candidate fails identically, so the candidate ladder retries the same + * history with attachments replaced by text instead of failing (#425). + */ +export class CheckpointCandidateMediaRejectedError extends Error { + override readonly name = 'CheckpointCandidateMediaRejectedError'; + + constructor(cause: unknown) { + super('Provider rejected checkpoint input because it carried too many images.', { cause }); + } +} diff --git a/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.test.ts b/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.test.ts index 4f7657070..566926657 100644 --- a/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.test.ts +++ b/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.test.ts @@ -15,7 +15,10 @@ import { describe, expect, it, vi } from 'vitest'; import { buildLocalRequestPayloadTransform } from '../../agent-host/assembly/local-turn-payload-transform.js'; import { validateCheckpointGeneration } from '../algorithm/checkpoint-format.js'; -import { CheckpointCandidateTooLargeError } from '../contracts.js'; +import { + CheckpointCandidateMediaRejectedError, + CheckpointCandidateTooLargeError, +} from '../contracts.js'; import { buildCheckpointControl, CHECKPOINT_SYSTEM_PROMPT } from './checkpoint-prompt.js'; import { createCheckpointSession } from './checkpoint-provider.js'; @@ -522,6 +525,78 @@ describe('checkpoint Provider overflow normalization', () => { }); }); +describe('checkpoint Provider image limits (#425)', () => { + const PNG_16X16 = + 'iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAABzklEQVR4nAXB2wFAIABAUQsUkVT83qk8QkXafwDnNKyCTbALDkEQnIJLcAuiIAmy4BG8giL4BFU0rJJNsksOSZCckktyS6IkSbLkkbySIvkkVTasLVvL3nK0hJaz5Wq5W2JLasktT8vbUlq+lto2rB1bx95xdISOs+PquDtiR+rIHU/H21E6vo7aNayKTbErDkVQnIpLcSuiIimy4lG8iqL4FFU1rD1bz95z9ISes+fquXtiT+rJPU/P21N6vp7aN6wD28A+cAyEgXPgGrgH4kAayAPPwDtQBr6BOjSsmk2zaw5N0JyaS3NroiZpsubRvJqi+TRVN6wj28g+coyEkXPkGrlH4kgaySPPyDtSRr6ROjashs2wGw5DMJyGy3AboiEZsuExvIZi+AzVNKwT28Q+cUyEiXPimrgn4kSayBPPxDtRJr6JOjWsls2yWw5LsJyWy3JboiVZsuWxvJZi+SzVNqyOzbE7DkdwnI7LcTuiIzmy43G8juL4HNU1rJ7Ns3sOT/Ccnstze6InebLn8bye4vk81TesM9vMPnPMhJlz5pq5Z+JMmskzz8w7U2a+mTo3rAvbwr5wLISFc+FauBfiQlrIC8/Cu1AWvoW6/LBGnAFkC4sVAAAAAElFTkSuQmCC'; + const IMAGE_LIMIT = '400 Upstream [invalid_request_error] Too many images in request: 31 > 30'; + + function imageModel(): Parameters[0] { + return { ...model(), input: ['text', 'image'] }; + } + + function session(streamFn: StreamFn) { + return createCheckpointSession({ + model: imageModel(), + streamFn, + thinkingLevel: 'off', + maxOutputTokens: 20, + providerInputLimit: 128_000, + }); + } + + it('normalizes an image-count error result to a media rejection', async () => { + const final = assistant({ + stopReason: 'error', + errorMessage: `BYOK upstream error: ${JSON.stringify({ message: `BYOK provider custom_provider:x upstream error: ${IMAGE_LIMIT}` })}`, + }); + const rejection = await (await session(vi.fn(() => completedStream(final)))) + .generate({ messages: [user('context', 1)] }) + .catch((error: unknown) => error); + + expect(rejection).toBeInstanceOf(CheckpointCandidateMediaRejectedError); + expect(rejection).toMatchObject({ cause: final }); + }); + + it('normalizes a thrown image-count error to a media rejection', async () => { + const error = new Error(IMAGE_LIMIT); + const rejection = await ( + await session( + vi.fn(() => { + throw error; + }), + ) + ) + .generate({ messages: [user('context', 1)] }) + .catch((caught: unknown) => caught); + + expect(rejection).toBeInstanceOf(CheckpointCandidateMediaRejectedError); + expect(rejection).toMatchObject({ cause: error }); + }); + + it('caps the images sent in the checkpoint request with the shared target projection', async () => { + const streamFn: StreamFn = vi.fn(() => completedStream(assistant({ content: [] }))); + const messages: AgentMessage[] = Array.from({ length: 31 }, (_, index) => ({ + role: 'user', + content: [ + { type: 'text', text: `screenshot ${index + 1}` }, + { type: 'image', data: PNG_16X16, mimeType: 'image/png' }, + ], + timestamp: index, + })); + + await (await session(streamFn)).generate({ messages }); + + const sent = vi.mocked(streamFn).mock.calls[0]?.[1]?.messages ?? []; + const images = sent.flatMap((message) => + Array.isArray(message.content) ? message.content.filter((block) => block.type === 'image') : [], + ); + expect(images).toHaveLength(20); + expect(JSON.stringify(sent)).toContain('Earlier image omitted'); + // The caller's history is untouched. + expect(messages.every((message) => JSON.stringify(message).includes('"type":"image"'))).toBe(true); + }); +}); + describe('checkpoint Provider output exhaustion', () => { it.each([ ['thinking only', [{ type: 'thinking' as const, thinking: 'private reasoning' }]], diff --git a/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.ts b/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.ts index fd2e0e806..9a8a8926d 100644 --- a/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.ts +++ b/packages/local-runtime-v2/src/service/turn-system/compaction/execution/checkpoint-provider.ts @@ -10,11 +10,13 @@ import { type LLMRequestSettledEvent, } from '@mavis/agent-core/pi-turn-runner'; import { createDefaultTokenEstimator } from '@mavis/context-manager'; +import { isLLMImageLimitMessage } from '@mavis/shared/llm-error-classifier'; import type { CheckpointSession } from '../algorithm/compact-context.js'; import type { PromptSnapshotSource } from '../../agent-host/contracts.js'; import type { CheckpointGeneration } from '../algorithm/checkpoint-format.js'; import { + CheckpointCandidateMediaRejectedError, CheckpointCandidateTooLargeError, type CheckpointResponseContentKind, } from '../contracts.js'; @@ -82,6 +84,9 @@ export async function createCheckpointSession( if (isContextOverflow(final, options.model.contextWindow)) { throw new CheckpointCandidateTooLargeError(final); } + if (final.stopReason === 'error' && isLLMImageLimitMessage(final.errorMessage)) { + throw new CheckpointCandidateMediaRejectedError(final); + } const generation = toGeneration(final); options.onGenerated?.(generation); if (isOutputExhaustedWithoutText(generation)) { @@ -139,6 +144,9 @@ async function requestCheckpoint( usageComplete: false, }); if (isExplicitInputTooLargeError(error)) throw new CheckpointCandidateTooLargeError(error); + if (isLLMImageLimitMessage(error instanceof Error ? error.message : undefined)) { + throw new CheckpointCandidateMediaRejectedError(error); + } throw error; } } diff --git a/packages/protocol/src/runtime.ts b/packages/protocol/src/runtime.ts index 88b03089c..e97e4d8f4 100644 --- a/packages/protocol/src/runtime.ts +++ b/packages/protocol/src/runtime.ts @@ -185,6 +185,8 @@ export interface IModelCapabilities { max_video_bytes_inline?: number | string; max_request_body_bytes?: number | string; max_attachments_count?: number; + /** Most images one provider request may carry; older history images become placeholders. */ + max_images_per_request?: number | string; support_files_api?: boolean; max_video_bytes_files_api?: number | string; files_api_upload_endpoint?: string; diff --git a/packages/shared/src/llm-error-classifier.ts b/packages/shared/src/llm-error-classifier.ts index 77761f39c..3aa02f9f2 100644 --- a/packages/shared/src/llm-error-classifier.ts +++ b/packages/shared/src/llm-error-classifier.ts @@ -60,7 +60,9 @@ export type LLMErrorSignal = | 'refusal' | 'network' | 'empty_response' - | 'length'; + | 'length' + | 'image_limit' + | 'invalid_request'; /** * Model-side safety classifier decline (Messages API `stop_reason: "refusal"`). Providers surface it @@ -142,6 +144,15 @@ const SAFE_TRANSPORT_NETWORK_MESSAGE_RE = /\b(?:ECONNREFUSED|ECONNRESET|ENOTFOUN const CHROMIUM_NETWORK_MESSAGE_RE = /\bnet::ERR_(?:CONNECTION_(?:RESET|CLOSED|REFUSED)|NETWORK_IO_SUSPENDED|NETWORK_CHANGED|NAME_NOT_RESOLVED|INTERNET_DISCONNECTED|ADDRESS_UNREACHABLE|PROXY_CONNECTION_FAILED|HTTP2_PROTOCOL_ERROR|SSL_BAD_RECORD_MAC_ALERT)\b/i; const CHROMIUM_TIMEOUT_MESSAGE_RE = /\bnet::ERR_(?:TIMED_OUT|CONNECTION_TIMED_OUT)\b/i; +/** + * Provider rejected the request for carrying too many images, e.g. + * `Too many images in request: 31 > 30` (#425). Re-sending the same history + * always fails again, so this is never a transient condition. + */ +const LLM_IMAGE_LIMIT_MESSAGE_RE = + /\btoo many images\b|\b(?:max|maximum) (?:number of |of \d+ )?images\b|\bnumber of images\b[^.]{0,40}\bexceed|\bimages? (?:count |limit )?exceed(?:s|ed)?\b|\bat most \d+ images?\b/i; +/** OpenAI/Anthropic-style error type for a request the provider will never accept as sent. */ +const LLM_INVALID_REQUEST_MESSAGE_RE = /\binvalid_request_error\b/i; const LLM_USAGE_LIMIT_UPSTREAM_STATUS_CODES = [2056, 2067] as const; const LLM_USAGE_LIMIT_UPSTREAM_STATUS_CODE_SET = new Set( LLM_USAGE_LIMIT_UPSTREAM_STATUS_CODES, @@ -183,6 +194,14 @@ export function normalizeLLMError(input: LLMErrorInput): NormalizedLLMError { facts.existingProtocolCode ??= legacy.existingProtocolCode; const message = legacy.message ?? visibleMessage; applyMessageSignals(message, facts, signals); + // Payload extraction keeps only the inner `message`, which drops a JSON + // `type` such as `invalid_request_error`; check the raw text for the two + // deterministic-rejection signals as well. + if (visibleMessage && visibleMessage !== message) { + const boundedVisible = sanitizeMessage(visibleMessage); + if (LLM_IMAGE_LIMIT_MESSAGE_RE.test(boundedVisible)) signals.add('image_limit'); + if (LLM_INVALID_REQUEST_MESSAGE_RE.test(boundedVisible)) signals.add('invalid_request'); + } applyNumericSignals(facts, signals); return { facts, ...(message ? { sanitizedMessage: message } : {}) }; } catch { @@ -193,6 +212,49 @@ export function normalizeLLMError(input: LLMErrorInput): NormalizedLLMError { } } +/** True when an LLM error message reports that the request carried too many images. */ +export function isLLMImageLimitMessage(message: string | undefined): boolean { + return typeof message === 'string' && LLM_IMAGE_LIMIT_MESSAGE_RE.test(message); +} + +/** + * Classify a flattened error message (no structured HTTP status available, as + * in UI layers) as a deterministic request rejection. `invalid_request` also + * requires a visible 400 so transient errors that merely mention the type stay + * unclassified. + */ +export function classifyLLMRequestRejectionMessage( + message: string | undefined, +): 'image_limit' | 'invalid_request' | undefined { + if (typeof message !== 'string' || !message) return undefined; + if (LLM_IMAGE_LIMIT_MESSAGE_RE.test(message)) return 'image_limit'; + if (LLM_INVALID_REQUEST_MESSAGE_RE.test(message) && /\b400\b/u.test(message)) { + return 'invalid_request'; + } + return undefined; +} + +/** + * A provider rejection that repeats identically for the same request: an image + * count limit, or an HTTP 400 that names `invalid_request_error`. + * + * BYOK retries every pre-output failure because custom gateways report + * transient errors inconsistently. These rejections are the exception: the + * request is malformed or over a hard limit, so retrying only re-sends it and + * delays the actionable error. Anything that also looks transient — timeout, + * network, rate limiting, overload or a 5xx — stays retryable. + */ +export function isLLMDeterministicRequestRejection(normalized: NormalizedLLMError): boolean { + const { facts } = normalized; + if (facts.explicitAbort || facts.timeout || facts.signals.has('network')) return false; + const status = effectiveHttpStatus(facts); + if (status !== undefined && (status === 408 || status === 429 || status >= 500)) return false; + if (facts.signals.has('image_limit')) { + return status === undefined || status === 400 || status === 413 || status === 422; + } + return status === 400 && facts.signals.has('invalid_request'); +} + export function toLLMMetricErrorKind(facts: LLMErrorFacts): LLMMetricErrorKind { try { if (facts.explicitAbort) return 'abort'; @@ -490,6 +552,8 @@ function applyMessageSignals( ) signals.add('network'); if (/\bempty.?response/i.test(message)) signals.add('empty_response'); + if (LLM_IMAGE_LIMIT_MESSAGE_RE.test(message)) signals.add('image_limit'); + if (LLM_INVALID_REQUEST_MESSAGE_RE.test(message)) signals.add('invalid_request'); const hasStructuredError = facts.existingProtocolCode !== undefined || effectiveHttpStatus(facts) !== undefined; if (!hasStructuredError && /\b(content.?filter|content.?policy|safety|blocked)\b/i.test(message)) diff --git a/packages/tui/src/tui/controller/runtime/runtime-error-presentation.ts b/packages/tui/src/tui/controller/runtime/runtime-error-presentation.ts index 116b0d6a8..9752e10a7 100644 --- a/packages/tui/src/tui/controller/runtime/runtime-error-presentation.ts +++ b/packages/tui/src/tui/controller/runtime/runtime-error-presentation.ts @@ -1,4 +1,7 @@ -import { LLM_ERROR_CODES } from '@mavis/shared/llm-error-classifier'; +import { + classifyLLMRequestRejectionMessage, + LLM_ERROR_CODES, +} from '@mavis/shared/llm-error-classifier'; import { tuiErrorDiagnostic } from '../../../user-facing-failure.js'; const AUTH_ERROR_CODES = new Set([401, LLM_ERROR_CODES.LLM_AUTH_ERROR]); @@ -80,6 +83,20 @@ export function resolveTuiRuntimeFailure( retryable: false, }; } + const rejection = requestRejection(message, metadata.errorDetail); + if (rejection) { + // Deterministic provider rejections (#425): the same history fails the + // same way on every resend, so /retry is never the next step. Show the + // upstream reason, which the generic branches below would hide. + const reasonLine = rejection.reason ? `\nReason: ${rejection.reason}` : ''; + return { + content: + rejection.kind === 'image_limit' + ? `The model provider rejected the request: the conversation carries too many images.${reasonLine}${provider}${code}\nResending fails the same way. Run /compact to summarize earlier history, or /new to start a fresh Session. Your prompt is preserved.` + : `The model provider rejected the request as invalid.${reasonLine}${provider}${code}\nResending the same conversation fails the same way. Run /compact to summarize earlier history, or /new to start a fresh Session. Your prompt is preserved.`, + retryable: false, + }; + } if ( (errorCode !== undefined && RATE_LIMIT_ERROR_CODES.has(errorCode)) || (message && /\b(?:rate.?limit(?:ed)?|too many requests|HTTP\s*429)\b/iu.test(message)) @@ -169,6 +186,20 @@ export function resolveTuiRuntimeFailure( }; } +const BYOK_UPSTREAM_PREFIX_RE = /^BYOK provider .+? upstream error:\s*/u; + +function requestRejection( + message: string | undefined, + rawDetail: string | undefined, +): { readonly kind: 'image_limit' | 'invalid_request'; readonly reason: string } | undefined { + for (const candidate of [rawDetail, message]) { + const kind = classifyLLMRequestRejectionMessage(candidate); + if (!kind || !candidate) continue; + return { kind, reason: tuiErrorDiagnostic(candidate.replace(BYOK_UPSTREAM_PREFIX_RE, '')) }; + } + return undefined; +} + function runtimeFailureDetail(message: string | undefined, rawDetail: string | undefined): string { const detail = rawDetail ? tuiErrorDiagnostic(rawDetail) : ''; return detail && detail !== message ? `\nReason: ${detail}` : ''; diff --git a/packages/tui/test/unit/runtime-error-presentation-rejection.test.ts b/packages/tui/test/unit/runtime-error-presentation-rejection.test.ts new file mode 100644 index 000000000..6c580f467 --- /dev/null +++ b/packages/tui/test/unit/runtime-error-presentation-rejection.test.ts @@ -0,0 +1,69 @@ +import { LLM_ERROR_CODES } from '@mavis/shared/llm-error-classifier'; +import { describe, expect, it } from 'vitest'; + +import { resolveTuiRuntimeFailure } from '../../src/tui/controller/runtime/runtime-error-presentation.js'; + +// #425: a provider that rejects the conversation for carrying too many images +// used to surface as "temporarily unavailable … Run /retry", which can never +// succeed because every resend carries the same history. + +const PROVIDER = 'custom_provider:fixture'; +const IMAGE_LIMIT_DETAIL = + '400 Upstream [invalid_request_error] Too many images in request: 31 > 30'; + +describe('deterministic provider rejection presentation', () => { + it('explains an image-count rejection with the upstream reason and a real next step', () => { + const failure = resolveTuiRuntimeFailure( + `BYOK provider ${PROVIDER} upstream error: ${IMAGE_LIMIT_DETAIL}`, + LLM_ERROR_CODES.LLM_UPSTREAM_ERROR, + { errorSource: 'byok_upstream', errorDetail: IMAGE_LIMIT_DETAIL, errorProviderId: PROVIDER }, + ); + + expect(failure.retryable).toBe(false); + expect(failure.content).toContain('too many images'); + expect(failure.content).toContain(`Reason: ${IMAGE_LIMIT_DETAIL}`); + expect(failure.content).toContain(`Provider: ${PROVIDER}`); + expect(failure.content).toContain('/compact'); + expect(failure.content).toContain('/new'); + expect(failure.content).not.toContain('/retry'); + expect(failure.content).not.toContain('temporarily unavailable'); + }); + + it('strips the BYOK attribution prefix when only the message carries the reason', () => { + const failure = resolveTuiRuntimeFailure( + `BYOK provider ${PROVIDER} upstream error: ${IMAGE_LIMIT_DETAIL}`, + LLM_ERROR_CODES.LLM_UPSTREAM_ERROR, + ); + + expect(failure.retryable).toBe(false); + expect(failure.content).toContain(`Reason: ${IMAGE_LIMIT_DETAIL}`); + expect(failure.content).not.toContain('BYOK provider'); + }); + + it('treats an HTTP 400 invalid_request_error as terminal with the same guidance', () => { + const detail = '400 {"type":"invalid_request_error","message":"messages: text content blocks must be non-empty"}'; + const failure = resolveTuiRuntimeFailure( + `BYOK provider ${PROVIDER} upstream error: ${detail}`, + LLM_ERROR_CODES.LLM_UPSTREAM_ERROR, + { errorSource: 'byok_upstream', errorDetail: detail, errorProviderId: PROVIDER }, + ); + + expect(failure.retryable).toBe(false); + expect(failure.content).toContain('rejected the request as invalid'); + expect(failure.content).toContain('text content blocks must be non-empty'); + expect(failure.content).toContain('/compact'); + expect(failure.content).not.toContain('/retry'); + }); + + it('keeps the retry guidance for an unclassified upstream failure', () => { + const failure = resolveTuiRuntimeFailure( + `BYOK provider ${PROVIDER} upstream error: 502 Bad Gateway`, + LLM_ERROR_CODES.LLM_UPSTREAM_ERROR, + { errorSource: 'byok_upstream', errorDetail: '502 Bad Gateway', errorProviderId: PROVIDER }, + ); + + expect(failure.retryable).toBe(true); + expect(failure.content).toContain('temporarily unavailable'); + expect(failure.content).toContain('/retry'); + }); +}); diff --git a/release/public-source.json b/release/public-source.json index 4a020b9f8..a6ff0aff8 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -113,6 +113,7 @@ "packages/agent-core/src/pi-turn-runner/metrics.ts", "packages/agent-core/src/pi-turn-runner/outbound-message-normalizer.ts", "packages/agent-core/src/pi-turn-runner/pi-turn-runner.ts", + "packages/agent-core/src/pi-turn-runner/request-image-limit.ts", "packages/agent-core/src/pi-turn-runner/terminal.ts", "packages/agent-core/src/pi-turn-runner/tool-context-size.ts", "packages/agent-core/src/pi-turn-runner/tools.ts", @@ -138,6 +139,7 @@ "packages/agent-core/test/unit/image-fixtures.ts", "packages/agent-core/test/unit/pi-turn-runner/llm-retry.test.ts", "packages/agent-core/test/unit/pi-turn-runner/llm-stream-timeout.test.ts", + "packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts", "packages/agent-extension/package.json", "packages/agent-extension/src/context-manager.ts", "packages/agent-extension/src/index.ts", @@ -3320,6 +3322,7 @@ "packages/tui/test/unit/run-coordinator-usage.test.ts", "packages/tui/test/unit/run-coordinator.test.ts", "packages/tui/test/unit/runtime-data-dir.test.ts", + "packages/tui/test/unit/runtime-error-presentation-rejection.test.ts", "packages/tui/test/unit/runtime-event-normalizer.test.ts", "packages/tui/test/unit/runtime-feedback-diagnostic-upload.test.ts", "packages/tui/test/unit/runtime-feedback-service.test.ts", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 57318ce12..88384f231 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -109,6 +109,8 @@ "packages/tui/test/unit/tui/widgets/editor/editor-behavior.test.ts", "packages/agent-core/test/unit/pi-turn-runner/llm-retry.test.ts", "packages/agent-core/test/unit/pi-turn-runner/llm-stream-timeout.test.ts", + "packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts", + "packages/tui/test/unit/runtime-error-presentation-rejection.test.ts", "packages/local-runtime-v2/src/service/turn-system/agent-host/execution/executor.test.ts", "packages/local-runtime-v2/src/services.test.ts", "packages/local-runtime-v2/src/application/session/session-title-policy.test.ts", From 6ca1a8f8d82b5d41d9d0e7231e61aaa2f7b268f9 Mon Sep 17 00:00:00 2001 From: Tao He Date: Mon, 5 Oct 2026 10:41:57 +0800 Subject: [PATCH 2/2] fix(images): parse attachment reminder paths with a linear scan CodeQL flagged the attribute regular expression as polynomial on user-controlled message text (js/polynomial-redos). Parse the attachment tags with indexOf instead and cover escaped paths and adversarial input. Refs #425 --- .../src/pi-turn-runner/request-image-limit.ts | 57 ++++++++++++++++--- .../request-image-limit.test.ts | 29 ++++++++++ 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/packages/agent-core/src/pi-turn-runner/request-image-limit.ts b/packages/agent-core/src/pi-turn-runner/request-image-limit.ts index 198420aef..a5d6c089a 100644 --- a/packages/agent-core/src/pi-turn-runner/request-image-limit.ts +++ b/packages/agent-core/src/pi-turn-runner/request-image-limit.ts @@ -132,8 +132,7 @@ function imageBlockCount(message: Message): number { return count; } -const ATTACHMENT_TAG_RE = /]*)>/gu; -const ATTRIBUTE_RE = /([a-z_]+)="([^"]*)"/gu; +const ATTACHMENT_TAG_OPEN = '(); - for (const attribute of (tag[1] ?? '').matchAll(ATTRIBUTE_RE)) { - attributes.set(attribute[1]!, unescapeAttribute(attribute[2]!)); - } + for (const attributes of attachmentTagAttributes(block.text)) { const path = attributes.get('path'); if (attributes.get('inline') !== 'true' || !path) continue; const kind = attributes.get('kind'); @@ -189,6 +184,54 @@ function collectToolCallPaths(messages: readonly Message[]): Map return paths; } +/** + * Attributes of each `` tag in `text`. A linear scan rather than + * a regular expression, because message text is user-controlled and nested + * quantifiers over it can backtrack polynomially. + */ +function attachmentTagAttributes(text: string): Array> { + const tags: Array> = []; + let from = 0; + for (;;) { + const start = text.indexOf(ATTACHMENT_TAG_OPEN, from); + if (start < 0) break; + const end = text.indexOf('>', start); + // No later tag can be closed either. + if (end < 0) break; + const next = text.charAt(start + ATTACHMENT_TAG_OPEN.length); + if (next === '>' || next === '/' || /\s/u.test(next)) { + tags.push(parseTagAttributes(text.slice(start + ATTACHMENT_TAG_OPEN.length, end))); + } + from = end + 1; + } + return tags; +} + +function parseTagAttributes(source: string): Map { + const attributes = new Map(); + let position = 0; + while (position < source.length) { + const equals = source.indexOf('="', position); + if (equals < 0) break; + const close = source.indexOf('"', equals + 2); + if (close < 0) break; + let nameStart = equals; + while (nameStart > position && isAttributeNameChar(source.charCodeAt(nameStart - 1))) { + nameStart -= 1; + } + if (nameStart < equals) { + attributes.set(source.slice(nameStart, equals), unescapeAttribute(source.slice(equals + 2, close))); + } + position = close + 1; + } + return attributes; +} + +/** `[a-z_]`, matching the attribute names the attachment reminder writes. */ +function isAttributeNameChar(code: number): boolean { + return (code >= 97 && code <= 122) || code === 95; +} + function unescapeAttribute(value: string): string { return value .replaceAll('"', '"') diff --git a/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts b/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts index fec86938f..a1bf60084 100644 --- a/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts +++ b/packages/agent-core/test/unit/pi-turn-runner/request-image-limit.test.ts @@ -184,6 +184,35 @@ describe("limitRequestImages", () => { ]); }); + it("reads escaped attachment paths and stays linear on adversarial reminder text", () => { + const escaped: UserMessage = { + role: "user", + content: [ + { + type: "text", + text: '', + }, + { ...IMAGE }, + ], + timestamp: 1, + }; + const adversarial: UserMessage = { + role: "user", + content: [ + { type: "text", text: ` ${" { const messages: Message[] = [userWithImages(1, 2, false), userWithImages(2, 1)]; const result = limitRequestImages(messages, 1);