From 0a54652e07d69f9bcdac8ca85bf4116988370947 Mon Sep 17 00:00:00 2001 From: Nothing Chan Date: Thu, 9 Jul 2026 07:24:39 +0800 Subject: [PATCH] fix(core): configurable vision bridge timeout + retry with fresh budget (#6541) * fix(core): configurable vision bridge timeout + retry with fresh budget The vision bridge capped image transcription at a hardcoded 30s. On a slow or proxied vision endpoint one latency spike permanently lost the image: the retry inside the side query shared the same abort signal, so a second attempt inherited whatever seconds were left of the first attempt's budget. Add a visionBridgeTimeoutMs setting (per attempt; unset keeps 30s, non-positive values are ignored) and retry a timed-out attempt once at the bridge level with a freshly created timeout signal. Non-timeout failures still fail immediately, and user cancellation is still reported as skipped. Fixes #6524 Co-Authored-By: Claude Fable 5 * fix(core): harden visionBridgeTimeoutMs against invalid timer values Maintainer E2E review found that fractional or out-of-range values such as 30000.5 and 4294967296 could pass the old number-typed config path and Config's Number.isFinite && > 0 guard. Node rejects fractional AbortSignal.timeout values with RangeError and can degrade oversized timer values to a 1ms timeout, which made image turns fail before any model request. Tighten the Config guard to positive integers within the supported 32-bit timer ceiling, make visionBridgeTimeoutMs a bounded integer setting so /config and the generated JSON schema reject bad values up front, and move AbortSignal.timeout/any creation inside the bridge try block so any future bad value becomes a safe failure result instead of an escaped rejection. Also mark the setting requiresRestart because it is read once in the Config constructor. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Fable 5 --- docs/users/configuration/settings.md | 6 + packages/cli/src/config/config.ts | 1 + .../cli/src/config/settingsSchema.test.ts | 13 ++ packages/cli/src/config/settingsSchema.ts | 20 +- .../cli/src/ui/commands/config-command.ts | 9 +- packages/cli/src/utils/settingsUtils.test.ts | 26 +++ packages/cli/src/utils/settingsUtils.ts | 9 + packages/core/src/config/config.test.ts | 59 ++++++ packages/core/src/config/config.ts | 29 +++ .../vision-bridge-service.test.ts | 110 ++++++++++- .../visionBridge/vision-bridge-service.ts | 184 ++++++++++-------- .../schemas/settings.schema.json | 6 + scripts/generate-settings-schema.ts | 13 +- 13 files changed, 394 insertions(+), 91 deletions(-) diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index 948d2af6b0..789979c865 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -262,6 +262,12 @@ The `extra_body` field allows you to add custom parameters to the request body s | ------------- | ------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------- | | `visionModel` | string | Image-capable model used as the vision bridge: when a text-only main model receives an image, it is transcribed by this model first. Leave empty to auto-pick a same-provider vision model. Can also be set via `/model --vision`. | `""` | +#### visionBridgeTimeoutMs + +| Setting | Type | Description | Default | +| ----------------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------- | +| `visionBridgeTimeoutMs` | integer | Per-attempt timeout in milliseconds for the vision bridge image transcription call (positive integer up to 2147483647; the bridge retries a timed-out attempt once with a fresh timeout). Unset uses the built-in 30s. Raise for slow or proxied vision endpoints. | unset | + #### voiceModel | Setting | Type | Description | Default | diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 4cfc402b01..92cc0ed927 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -2196,6 +2196,7 @@ export async function loadCliConfig( memoryAgentTimeoutMinutes: settings.memory?.agentTimeoutMinutes, fastModel: settings.fastModel || undefined, visionModel: settings.visionModel || undefined, + visionBridgeTimeoutMs: settings.visionBridgeTimeoutMs, modelFallbacks: resolveModelFallbacks( argv.fallbackModel, settings.modelFallbacks, diff --git a/packages/cli/src/config/settingsSchema.test.ts b/packages/cli/src/config/settingsSchema.test.ts index 04e6dd56d0..7517a162d8 100644 --- a/packages/cli/src/config/settingsSchema.test.ts +++ b/packages/cli/src/config/settingsSchema.test.ts @@ -179,6 +179,19 @@ describe('SettingsSchema', () => { expect(voiceModel.showInDialog).toBe(false); }); + it('should define visionBridgeTimeoutMs as a restart-required bounded integer', () => { + const timeout = getSettingsSchema().visionBridgeTimeoutMs; + + expect(timeout).toBeDefined(); + expect(timeout.type).toBe('integer'); + expect(timeout.category).toBe('Model'); + expect(timeout.default).toBeUndefined(); + expect(timeout.minimum).toBe(1); + expect(timeout.maximum).toBe(2_147_483_647); + expect(timeout.requiresRestart).toBe(true); + expect(timeout.showInDialog).toBe(false); + }); + it('should define stopHookBlockingCap schema override as a positive integer', () => { expect( getSettingsSchema().stopHookBlockingCap.jsonSchemaOverride, diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 2afb3959d9..9c323850d3 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -34,6 +34,7 @@ export type SettingsType = | 'boolean' | 'string' | 'number' + | 'integer' | 'array' | 'object' | 'enum'; @@ -88,9 +89,9 @@ export interface SettingDefinition { options?: readonly SettingEnumOption[]; /** Schema for array items when type is 'array' */ items?: SettingItemDefinition; - /** Minimum value for number-type settings. */ + /** Minimum value for number/integer-type settings. */ minimum?: number; - /** Maximum value for number-type settings. */ + /** Maximum value for number/integer-type settings. */ maximum?: number; /** * Primitive shapes a field accepted before it was expanded to its current @@ -1276,6 +1277,21 @@ const SETTINGS_SCHEMA = { showInDialog: true, }, + visionBridgeTimeoutMs: { + type: 'integer', + label: 'Vision Bridge Timeout (ms)', + category: 'Model', + // Read once in the Config constructor with no setter, so a mid-session + // change only takes effect on restart. + requiresRestart: true, + default: undefined as number | undefined, + minimum: 1, + maximum: 2_147_483_647, + description: + 'Per-attempt timeout in milliseconds for the vision bridge image transcription call (a positive integer up to 2147483647). Unset uses the built-in 30s. Raise for slow or proxied vision endpoints.', + showInDialog: false, + }, + modelFallbacks: { type: 'string', label: 'Model Fallbacks', diff --git a/packages/cli/src/ui/commands/config-command.ts b/packages/cli/src/ui/commands/config-command.ts index 0feb1d8970..2a1644ac7a 100644 --- a/packages/cli/src/ui/commands/config-command.ts +++ b/packages/cli/src/ui/commands/config-command.ts @@ -114,11 +114,13 @@ function coerceValue( }; } - case 'number': { + case 'number': + case 'integer': { if (!rawValue || rawValue.trim() === '') { return { value: undefined, - error: t('Invalid number value: "{{value}}".', { + error: t('Invalid {{type}} value: "{{value}}".', { + type: def.type, value: String(rawValue), }), }; @@ -127,7 +129,8 @@ function coerceValue( if (Number.isNaN(parsed) || !Number.isFinite(parsed)) { return { value: undefined, - error: t('Invalid number value: "{{value}}".', { + error: t('Invalid {{type}} value: "{{value}}".', { + type: def.type, value: String(rawValue), }), }; diff --git a/packages/cli/src/utils/settingsUtils.test.ts b/packages/cli/src/utils/settingsUtils.test.ts index c91db236f1..69ab8e0394 100644 --- a/packages/cli/src/utils/settingsUtils.test.ts +++ b/packages/cli/src/utils/settingsUtils.test.ts @@ -100,6 +100,17 @@ describe('SettingsUtils', () => { description: 'A number field with a maximum.', showInDialog: true, }, + integerWithBounds: { + type: 'integer', + label: 'Integer With Bounds', + category: 'Basic', + requiresRestart: false, + default: 1, + minimum: 1, + maximum: 10, + description: 'An integer field with bounds.', + showInDialog: true, + }, advanced: { type: 'object', label: 'Advanced', @@ -258,6 +269,21 @@ describe('SettingsUtils', () => { 'Value must be <= 10', ); }); + + it('validates integer settings', () => { + const definition = getSettingDefinition('integerWithBounds'); + expect(definition).toBeDefined(); + + expect(validateSettingValue(definition!, 1)).toBeUndefined(); + expect(validateSettingValue(definition!, 10)).toBeUndefined(); + expect(validateSettingValue(definition!, 1.5)).toBe( + 'Value must be an integer', + ); + expect(validateSettingValue(definition!, 0)).toBe('Value must be >= 1'); + expect(validateSettingValue(definition!, 11)).toBe( + 'Value must be <= 10', + ); + }); }); describe('requiresRestart', () => { diff --git a/packages/cli/src/utils/settingsUtils.ts b/packages/cli/src/utils/settingsUtils.ts index 565fc8846e..952e5aa93b 100644 --- a/packages/cli/src/utils/settingsUtils.ts +++ b/packages/cli/src/utils/settingsUtils.ts @@ -327,6 +327,15 @@ export function validateSettingValue( if (def.maximum !== undefined && value > def.maximum) return `Value must be <= ${def.maximum}`; break; + case 'integer': + if (typeof value !== 'number' || !Number.isFinite(value)) + return 'Value must be a finite integer'; + if (!Number.isInteger(value)) return 'Value must be an integer'; + if (def.minimum !== undefined && value < def.minimum) + return `Value must be >= ${def.minimum}`; + if (def.maximum !== undefined && value > def.maximum) + return `Value must be <= ${def.maximum}`; + break; case 'string': if (typeof value !== 'string') return 'Value must be a string'; if (value.length > MAX_SETTING_STRING_VALUE_LENGTH) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 732e33b567..05dba1be7a 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -557,6 +557,65 @@ describe('Server Config (config.ts)', () => { }); }); + describe('getVisionBridgeTimeoutMs', () => { + it('returns undefined when unset', () => { + expect(new Config(baseParams).getVisionBridgeTimeoutMs()).toBeUndefined(); + }); + + it('passes through positive values', () => { + expect( + new Config({ + ...baseParams, + visionBridgeTimeoutMs: 120_000, + }).getVisionBridgeTimeoutMs(), + ).toBe(120_000); + }); + + it('treats non-positive values as unset (schema validation is bypassed on load)', () => { + expect( + new Config({ + ...baseParams, + visionBridgeTimeoutMs: 0, + }).getVisionBridgeTimeoutMs(), + ).toBeUndefined(); + expect( + new Config({ + ...baseParams, + visionBridgeTimeoutMs: -5000, + }).getVisionBridgeTimeoutMs(), + ).toBeUndefined(); + }); + + it('rejects values AbortSignal.timeout cannot take (fractional, over 2^31-1, non-finite)', () => { + // These pass the number-typed schema's `minimum: 1` via /config but would + // make AbortSignal.timeout throw RangeError or degrade to a 1ms timer. + for (const bad of [ + 30_000.5, + 2_147_483_648, + 4_294_967_296, + 1e300, + Number.NaN, + Number.POSITIVE_INFINITY, + ]) { + expect( + new Config({ + ...baseParams, + visionBridgeTimeoutMs: bad, + }).getVisionBridgeTimeoutMs(), + ).toBeUndefined(); + } + }); + + it('accepts the maximum supported integer timeout', () => { + expect( + new Config({ + ...baseParams, + visionBridgeTimeoutMs: 2_147_483_647, + }).getVisionBridgeTimeoutMs(), + ).toBe(2_147_483_647); + }); + }); + describe('getMaxSubagentDepth', () => { it('defaults to 5 when unset', () => { expect(new Config(baseParams).getMaxSubagentDepth()).toBe(5); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 33af6f236f..e26e098684 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1156,6 +1156,12 @@ export interface ConfigParameters { * (configurable via `/model --vision`). */ visionModel?: string; + /** + * Per-attempt timeout in milliseconds for the vision bridge transcription + * call. Unset → built-in 30s. Corresponds to the `visionBridgeTimeoutMs` + * setting; useful for slow or proxied vision endpoints. + */ + visionBridgeTimeoutMs?: number; /** * Ordered list of fallback model IDs to try when the primary model hits * capacity errors (429/503/529). At most 3 entries; duplicate fallback @@ -1720,6 +1726,7 @@ export class Config { private readonly memoryAgentTimeoutMinutes: number | undefined; private fastModel?: string; private visionModel?: string; + private readonly visionBridgeTimeoutMs: number | undefined; private readonly modelFallbacks: string[]; private readonly disableAllHooks: boolean; private readonly stopHookBlockingCap: number; @@ -2049,6 +2056,19 @@ export class Config { : undefined; this.fastModel = params.fastModel || undefined; this.visionModel = params.visionModel || undefined; + // Guard: nothing validates settings.json on the load path, so this is the + // only real gate. `AbortSignal.timeout()` requires an integer in + // [0, 2^31-1] — a fractional or out-of-range value (which the number-typed + // schema still accepts via /config) would throw RangeError or silently + // degrade to a 1ms timeout, killing every bridge turn. Reject anything the + // timer can't take and fall back to the built-in default. + this.visionBridgeTimeoutMs = + params.visionBridgeTimeoutMs !== undefined && + Number.isInteger(params.visionBridgeTimeoutMs) && + params.visionBridgeTimeoutMs > 0 && + params.visionBridgeTimeoutMs <= 2_147_483_647 + ? params.visionBridgeTimeoutMs + : undefined; this.modelFallbacks = normalizeModelFallbacks(params.modelFallbacks); this.disableAllHooks = params.disableAllHooks ?? false; this.stopHookBlockingCap = resolveStopHookBlockingCap( @@ -3434,6 +3454,15 @@ export class Config { ); } + /** + * Per-attempt timeout in milliseconds for the vision bridge transcription + * call. Resolves the `visionBridgeTimeoutMs` setting; `undefined` means the + * bridge's built-in default applies. + */ + getVisionBridgeTimeoutMs(): number | undefined { + return this.visionBridgeTimeoutMs; + } + /** * Set model programmatically (e.g., VLM auto-switch, fallback). * Delegates to ModelsConfig. diff --git a/packages/core/src/services/visionBridge/vision-bridge-service.test.ts b/packages/core/src/services/visionBridge/vision-bridge-service.test.ts index a732ce8201..b5a5e90907 100644 --- a/packages/core/src/services/visionBridge/vision-bridge-service.test.ts +++ b/packages/core/src/services/visionBridge/vision-bridge-service.test.ts @@ -391,13 +391,16 @@ describe('runVisionBridge', () => { expect(result.error).toBeUndefined(); }); - it('classifies a timeout (user did not cancel) as a failed result with a safe reason', async () => { - // Control the bridge's internal timeout signal so we can fire it (the user - // signal stays un-aborted — this is the timeout-only path, not a cancel). - const timeoutCtl = new AbortController(); + it('classifies a timeout on every attempt (user did not cancel) as a failed result with a safe reason', async () => { + // Control the bridge's internal timeout signals so we can fire them (the + // user signal stays un-aborted — this is the timeout-only path, not a + // cancel). One controller per attempt: the bridge retries a timeout once + // with a fresh timeout signal. + const timeoutCtls = [new AbortController(), new AbortController()]; const timeoutSpy = vi .spyOn(AbortSignal, 'timeout') - .mockReturnValue(timeoutCtl.signal); + .mockReturnValueOnce(timeoutCtls[0].signal) + .mockReturnValueOnce(timeoutCtls[1].signal); mockSideQuery.mockImplementation( (_config: unknown, opts: { abortSignal: AbortSignal }) => new Promise((_resolve, reject) => { @@ -412,7 +415,9 @@ describe('runVisionBridge', () => { parts: ['look', image()], signal: signal(), // user never cancels }); - timeoutCtl.abort(); // fire the 30s timeout + timeoutCtls[0].abort(); // fire attempt 1's timeout → retry + await vi.waitFor(() => expect(mockSideQuery).toHaveBeenCalledTimes(2)); + timeoutCtls[1].abort(); // fire attempt 2's timeout → give up const result = await pending; expect(result.status).toBe('failed'); expect(result.error).toMatch(/timed out/); @@ -424,6 +429,99 @@ describe('runVisionBridge', () => { } }); + it('retries a timed-out attempt once with a fresh timeout and can still succeed', async () => { + const timeoutCtl = new AbortController(); + const timeoutSpy = vi + .spyOn(AbortSignal, 'timeout') + .mockReturnValueOnce(timeoutCtl.signal) + .mockReturnValueOnce(new AbortController().signal); + mockSideQuery + .mockImplementationOnce( + (_config: unknown, opts: { abortSignal: AbortSignal }) => + new Promise((_resolve, reject) => { + opts.abortSignal.addEventListener('abort', () => + reject(new DOMException('aborted', 'AbortError')), + ); + }), + ) + .mockResolvedValueOnce({ text: 'recovered description' }); + try { + const pending = runVisionBridge({ + config, + parts: ['look', image()], + signal: signal(), + }); + timeoutCtl.abort(); // attempt 1 times out + const result = await pending; + expect(result.status).toBe('ok'); + expect(textOf(result.parts)).toContain('recovered description'); + expect(mockSideQuery).toHaveBeenCalledTimes(2); + // Fresh timeout budget per attempt, not one shared signal. + expect(timeoutSpy).toHaveBeenCalledTimes(2); + } finally { + timeoutSpy.mockRestore(); + } + }); + + it('does not retry non-timeout failures', async () => { + mockSideQuery.mockRejectedValue(new Error('HTTP 401 unauthorized')); + const result = await runVisionBridge({ + config, + parts: ['look', image()], + signal: signal(), + }); + expect(result.status).toBe('failed'); + expect(mockSideQuery).toHaveBeenCalledOnce(); + }); + + it('honors the configured visionBridgeTimeoutMs for each attempt', async () => { + const timeoutSpy = vi.spyOn(AbortSignal, 'timeout'); + mockSideQuery.mockResolvedValue({ text: 'desc' }); + try { + await runVisionBridge({ + config: { + getDefaultVisionBridgeModel: () => ({ id: 'qwen3-vl-plus' }), + getVisionBridgeTimeoutMs: () => 120_000, + } as unknown as Config, + parts: ['look', image()], + signal: signal(), + }); + expect(timeoutSpy).toHaveBeenCalledWith(120_000); + } finally { + timeoutSpy.mockRestore(); + } + }); + + it('turns an unusable timeout value into a failure instead of throwing', async () => { + // Config normally rejects such values, but if one ever reaches the bridge + // (a future caller, a direct call), AbortSignal.timeout throws RangeError. + // Its creation lives inside the try, so it must surface as failure() — the + // TUI caller has no try/catch and would otherwise swallow the whole turn. + const timeoutSpy = vi + .spyOn(AbortSignal, 'timeout') + .mockImplementation(() => { + throw new RangeError('timeout value is out of range'); + }); + try { + const result = await runVisionBridge({ + config: { + getDefaultVisionBridgeModel: () => ({ id: 'qwen3-vl-plus' }), + getVisionBridgeTimeoutMs: () => 30_000.5, + } as unknown as Config, + parts: ['look', image()], + signal: signal(), + }); + expect(result.status).toBe('failed'); + expect(result.egressOccurred).toBe(true); + // Classified as a generic failure, not a timeout. + expect(textOf(result.parts)).not.toMatch(/timed out/i); + // No model call — the signal blew up before dispatch. + expect(mockSideQuery).not.toHaveBeenCalled(); + } finally { + timeoutSpy.mockRestore(); + } + }); + it('bounds bridge output and skips output-language preference injection', async () => { mockSideQuery.mockResolvedValue({ text: 'desc' }); diff --git a/packages/core/src/services/visionBridge/vision-bridge-service.ts b/packages/core/src/services/visionBridge/vision-bridge-service.ts index d0c22c63b7..7d7b0f7ef4 100644 --- a/packages/core/src/services/visionBridge/vision-bridge-service.ts +++ b/packages/core/src/services/visionBridge/vision-bridge-service.ts @@ -21,6 +21,9 @@ import { VISION_BRIDGE_MAX_IMAGES } from './vision-bridge-constants.js'; const debugLogger = createDebugLogger('VISION_BRIDGE'); const BRIDGE_MAX_OUTPUT_TOKENS = 2048; const VISION_BRIDGE_TIMEOUT_MS = 30_000; +// One retry on timeout, with a fresh timeout budget per attempt: a transient +// latency spike on the vision endpoint shouldn't permanently drop the image. +const VISION_BRIDGE_MAX_ATTEMPTS = 2; // Cap intent so @-file contents in nonImageParts aren't dumped to the bridge model. const BRIDGE_INTENT_MAX_CHARS = 2000; @@ -314,9 +317,8 @@ export async function runVisionBridge(params: { ); } - // The vision call gets its own timeout, linked to the turn's abort signal. - const timeoutSignal = AbortSignal.timeout(VISION_BRIDGE_TIMEOUT_MS); - const combinedSignal = AbortSignal.any([signal, timeoutSignal]); + const timeoutMs = + config.getVisionBridgeTimeoutMs?.() ?? VISION_BRIDGE_TIMEOUT_MS; const requestContents: Content[] = [ { role: 'user', parts: [...toConvert, { text: buildIntentPart(intent) }] }, ]; @@ -328,89 +330,115 @@ export async function runVisionBridge(params: { ...(modelEndpoint && { modelEndpoint }), } as const; - try { - debugLogger.debug(`calling ${modelId} for ${toConvert.length} image(s)`); - const { text } = await runSideQuery(config, { - contents: requestContents, - abortSignal: combinedSignal, - model: modelForApi, - systemInstruction: BRIDGE_SYSTEM_INSTRUCTION, - purpose: 'vision-bridge', - maxAttempts: 2, - skipOutputLanguagePreference: true, - config: { maxOutputTokens: BRIDGE_MAX_OUTPUT_TOKENS }, - // Fail closed: if the pinned/auto-selected vision model's generator can't - // be created (e.g. a missing cross-provider credential), throw here rather - // than letting BaseLlmClient fall back to the main generator — that would - // send image payloads to the text-only primary while the egress notice - // names a different endpoint. The catch below turns this into a failure. - failClosed: true, - }); + for (let attempt = 1; attempt <= VISION_BRIDGE_MAX_ATTEMPTS; attempt++) { + // The vision call gets its own timeout, linked to the turn's abort signal. + // Declared here so the catch can classify a timeout, but created INSIDE the + // try: `AbortSignal.timeout` throws on a value the timer can't take, and we + // want that to become a failure() rather than an escaped rejection — the TUI + // caller has no try/catch and would otherwise swallow the whole turn. Fresh + // per attempt so a retry starts with a full budget instead of the few + // seconds left over from the attempt that just timed out. + let timeoutSignal: AbortSignal | undefined; + let combinedSignal: AbortSignal | undefined; - const description = stripThinkTags(text ?? ''); - if (description.length === 0) { - debugLogger.warn(`${modelId} returned an empty description`); - return failure( - 'the vision model returned no description', - parts, - omittedCount, - { modelId, ...egress }, - ); - } + try { + timeoutSignal = AbortSignal.timeout(timeoutMs); + combinedSignal = AbortSignal.any([signal, timeoutSignal]); + debugLogger.debug(`calling ${modelId} for ${toConvert.length} image(s)`); + const { text } = await runSideQuery(config, { + contents: requestContents, + abortSignal: combinedSignal, + model: modelForApi, + systemInstruction: BRIDGE_SYSTEM_INSTRUCTION, + purpose: 'vision-bridge', + maxAttempts: 2, + skipOutputLanguagePreference: true, + config: { maxOutputTokens: BRIDGE_MAX_OUTPUT_TOKENS }, + // Fail closed: if the pinned/auto-selected vision model's generator can't + // be created (e.g. a missing cross-provider credential), throw here rather + // than letting BaseLlmClient fall back to the main generator — that would + // send image payloads to the text-only primary while the egress notice + // names a different endpoint. The catch below turns this into a failure. + failClosed: true, + }); - // The transcription often carries sensitive screen contents (tokens, PII, - // private code), and debug logs can end up in shared support bundles — so - // log only metadata (model + length), never the raw text. Trace a wrong - // primary-model answer via the length/model here, not the content. - debugLogger.debug( - `vision bridge transcription via ${modelId} (${description.length} chars)`, - ); - - return { - applied: true, - status: 'ok', - // Stand the transcription in the first image's slot (right after its - // "Content from :" prefix) so the primary model reads it as that - // file's content instead of re-reading the image with a tool. - parts: replaceImagesWithText( - parts, - buildInterpretationBlock( - modelId, - description, - toConvert.length, + const description = stripThinkTags(text ?? ''); + if (description.length === 0) { + debugLogger.warn(`${modelId} returned an empty description`); + return failure( + 'the vision model returned no description', + parts, omittedCount, - ), - ), - convertedCount: toConvert.length, - omittedCount, - modelId, - ...egress, - }; - } catch (error) { - if (signal.aborted) { - debugLogger.debug(`conversion cancelled via ${modelId}`); + { modelId, ...egress }, + ); + } + + // The transcription often carries sensitive screen contents (tokens, PII, + // private code), and debug logs can end up in shared support bundles — so + // log only metadata (model + length), never the raw text. Trace a wrong + // primary-model answer via the length/model here, not the content. + debugLogger.debug( + `vision bridge transcription via ${modelId} (${description.length} chars)`, + ); + return { - applied: false, - status: 'skipped', - convertedCount: 0, + applied: true, + status: 'ok', + // Stand the transcription in the first image's slot (right after its + // "Content from :" prefix) so the primary model reads it as that + // file's content instead of re-reading the image with a tool. + parts: replaceImagesWithText( + parts, + buildInterpretationBlock( + modelId, + description, + toConvert.length, + omittedCount, + ), + ), + convertedCount: toConvert.length, omittedCount, modelId, ...egress, }; + } catch (error) { + if (signal.aborted) { + debugLogger.debug(`conversion cancelled via ${modelId}`); + return { + applied: false, + status: 'skipped', + convertedCount: 0, + omittedCount, + modelId, + ...egress, + }; + } + // `?.` because AbortSignal creation itself can throw (a bad timeout + // value) before these are assigned — that lands here as a non-timeout + // failure, which is the safe classification. + const timedOut = !!combinedSignal?.aborted && !!timeoutSignal?.aborted; + if (timedOut && attempt < VISION_BRIDGE_MAX_ATTEMPTS) { + debugLogger.warn( + `conversion attempt ${attempt} via ${modelId} timed out after ${timeoutMs}ms; retrying`, + ); + continue; + } + const reason = timedOut + ? `timed out after ${timeoutMs}ms (${VISION_BRIDGE_MAX_ATTEMPTS} attempts)` + : error instanceof Error + ? error.message + : String(error); + debugLogger.warn(`conversion failed via ${modelId}: ${reason}`); + return failure(reason, parts, omittedCount, { + modelId, + // The timeout message is safe to show; an arbitrary provider error is not + // (it can carry a signed URL or token), so keep it generic for the model. + noteReason: timedOut ? reason : 'the vision model request failed', + ...egress, + }); } - const timedOut = combinedSignal.aborted && timeoutSignal.aborted; - const reason = timedOut - ? `timed out after ${VISION_BRIDGE_TIMEOUT_MS}ms` - : error instanceof Error - ? error.message - : String(error); - debugLogger.warn(`conversion failed via ${modelId}: ${reason}`); - return failure(reason, parts, omittedCount, { - modelId, - // The timeout message is safe to show; an arbitrary provider error is not - // (it can carry a signed URL or token), so keep it generic for the model. - noteReason: timedOut ? reason : 'the vision model request failed', - ...egress, - }); } + // Unreachable: every loop iteration returns. Keeps TS's control-flow analysis + // satisfied without widening the return type. + throw new Error('vision bridge: exhausted attempts without a result'); } diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index bb623e907c..cbea4e3c36 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -550,6 +550,12 @@ "type": "string", "default": "" }, + "visionBridgeTimeoutMs": { + "description": "Per-attempt timeout in milliseconds for the vision bridge image transcription call (a positive integer up to 2147483647). Unset uses the built-in 30s. Raise for slow or proxied vision endpoints.", + "type": "integer", + "minimum": 1, + "maximum": 2147483647 + }, "modelFallbacks": { "description": "Ordered list of fallback model IDs (comma-separated, max 3) to try when the primary model hits capacity errors (429/503/529). Example: \"qwen-plus,qwen-turbo\". Set via CLI with --fallback-model.", "type": "string", diff --git a/scripts/generate-settings-schema.ts b/scripts/generate-settings-schema.ts index 45f6eb36a7..f762396a43 100644 --- a/scripts/generate-settings-schema.ts +++ b/scripts/generate-settings-schema.ts @@ -128,6 +128,9 @@ function convertSettingToJsonSchema( case 'number': schema.type = 'number'; break; + case 'integer': + schema.type = 'integer'; + break; case 'array': schema.type = 'array'; if (setting.items) { @@ -186,10 +189,16 @@ function convertSettingToJsonSchema( } } - if (setting.type === 'number' && setting.minimum !== undefined) { + if ( + (setting.type === 'number' || setting.type === 'integer') && + setting.minimum !== undefined + ) { schema.minimum = setting.minimum; } - if (setting.type === 'number' && setting.maximum !== undefined) { + if ( + (setting.type === 'number' || setting.type === 'integer') && + setting.maximum !== undefined + ) { schema.maximum = setting.maximum; }