diff --git a/.claude/rules/sim-ui-copy.md b/.claude/rules/sim-ui-copy.md new file mode 100644 index 00000000000..951e4a15367 --- /dev/null +++ b/.claude/rules/sim-ui-copy.md @@ -0,0 +1,46 @@ +--- +paths: + - "apps/sim/**/*.tsx" + - "apps/sim/components/emcn/**" +--- + +# UI Copy + +**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading. + +This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.md` instead. + +**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections, so it earns its place by the "prevents misunderstanding" test. Keep those descriptions verb-first and one line, per `sim-settings-pages.md`. Everything else on a settings page — inline `

` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above. + +## The default is no description + +```tsx +// ✗ Bad — the subtitle restates the heading +

API Keys

+

Manage your API keys.

+ +// ✗ Bad — decorative filler under a field label + + +// ✓ Good — the label carries the whole meaning +

API Keys

+ +``` + +If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line. + +## When supporting copy earns its place + +Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake: + +- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone." +- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens." +- **A security or access implication** — "This key is shown once and grants full workspace access." +- **A state the user cannot otherwise see** — "Inherited from your organization's policy." +- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com." + +Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted. + +## Component APIs + +Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above. diff --git a/.cursor/rules/sim-ui-copy.mdc b/.cursor/rules/sim-ui-copy.mdc new file mode 100644 index 00000000000..4648eb21e32 --- /dev/null +++ b/.cursor/rules/sim-ui-copy.mdc @@ -0,0 +1,44 @@ +--- +description: UI copy conventions — no default subtitles or helper text under headings, labels, cards, or settings +globs: ["apps/sim/**/*.tsx"] +--- +# UI Copy + +**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading. + +This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.mdc` instead. + +**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections. Everything else on a settings page — inline `

` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above. + +## The default is no description + +```tsx +// ✗ Bad — the subtitle restates the heading +

API Keys

+

Manage your API keys.

+ +// ✗ Bad — decorative filler under a field label + + +// ✓ Good — the label carries the whole meaning +

API Keys

+ +``` + +If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line. + +## When supporting copy earns its place + +Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake: + +- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone." +- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens." +- **A security or access implication** — "This key is shown once and grants full workspace access." +- **A state the user cannot otherwise see** — "Inherited from your organization's policy." +- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com." + +Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted. + +## Component APIs + +Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above. diff --git a/apps/sim/executor/handlers/agent/agent-handler.test.ts b/apps/sim/executor/handlers/agent/agent-handler.test.ts index 0da21a6a431..35dbf52d7f3 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.test.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.test.ts @@ -979,6 +979,49 @@ describe('AgentBlockHandler', () => { ) }) + /** + * A stalled model call reaches here as the runtime's own `TimeoutError`, whose bare + * message ("The operation timed out.") names nothing. It must become a Sim-level + * message WITHOUT discarding the phase detail the provider attached — that detail is + * the only thing distinguishing "never answered" from "body never completed". + */ + it('maps a provider TimeoutError to a Sim message while keeping the phase detail', async () => { + const inputs = { model: 'gpt-4o', userPrompt: 'hi', apiKey: 'test-api-key' } + mockGetProviderFromModel.mockReturnValue('openai') + + // Faithful to production: providers rewrap the transport failure in a + // ProviderError, which overwrites `name` — so only the cause still classifies it. + const transport = new Error( + 'The operation timed out. [phase=reading-response-body elapsedMs=60001 status=200 contentLength=32116]' + ) + transport.name = 'TimeoutError' + const wrapped = new Error(transport.message, { cause: transport }) + wrapped.name = 'ProviderError' + mockExecuteProviderRequest.mockRejectedValueOnce(wrapped) + + const error = await handler.execute(mockContext, mockBlock, inputs).catch((e) => e) + + expect(error.message).toContain('Provider request timed out') + expect(error.message).toContain('phase=reading-response-body') + expect(error.message).toContain('status=200') + }) + + it('maps a provider AbortError the same way', async () => { + const inputs = { model: 'gpt-4o', userPrompt: 'hi', apiKey: 'test-api-key' } + mockGetProviderFromModel.mockReturnValue('openai') + + const aborted = new Error('aborted [phase=awaiting-response-headers elapsedMs=12]') + aborted.name = 'AbortError' + const wrapped = new Error(aborted.message, { cause: aborted }) + wrapped.name = 'ProviderError' + mockExecuteProviderRequest.mockRejectedValueOnce(wrapped) + + const error = await handler.execute(mockContext, mockBlock, inputs).catch((e) => e) + + expect(error.message).toContain('Provider request timed out') + expect(error.message).toContain('phase=awaiting-response-headers') + }) + it('should handle streaming responses with text/event-stream content type', async () => { const mockStreamBody = new ReadableStream({ start(controller) { diff --git a/apps/sim/executor/handlers/agent/agent-handler.ts b/apps/sim/executor/handlers/agent/agent-handler.ts index 8d510d1696e..209443bb7c2 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.ts @@ -71,6 +71,22 @@ import { getToolAsync } from '@/tools/utils.server' const logger = createLogger('AgentBlockHandler') +/** + * True when a failure originated from a transport deadline or abort, at any depth of the + * cause chain. + * + * Providers rewrap transport failures (`ProviderError` overwrites `name`), so a check on + * the top-level `name` alone misses every wrapped case. Bounded to a short walk so a + * self-referential cause cannot loop. + */ +function isTransportTimeout(error: unknown): boolean { + for (let current = error, depth = 0; current instanceof Error && depth < 5; depth++) { + if (current.name === 'AbortError' || current.name === 'TimeoutError') return true + current = current.cause + } + return false +} + /** * Handler for Agent blocks that process LLM requests with optional tools. */ @@ -1299,8 +1315,15 @@ export class AgentBlockHandler implements BlockHandler { timestamp: new Date().toISOString(), }) - if (error.name === 'AbortError') { - throw new Error('Provider request timed out - the API took too long to respond') + /** + * The original message is appended rather than replaced: providers annotate it with + * the request phase they died in, which is the only thing separating a request that + * was never answered from one whose body stalled. + */ + if (isTransportTimeout(error)) { + throw new Error( + `Provider request timed out - the API took too long to respond (${error.message})` + ) } if (error.name === 'TypeError' && error.message.includes('fetch')) { throw new Error( diff --git a/apps/sim/providers/openai/core.response-status.test.ts b/apps/sim/providers/openai/core.response-status.test.ts new file mode 100644 index 00000000000..1ff16b1b0a0 --- /dev/null +++ b/apps/sim/providers/openai/core.response-status.test.ts @@ -0,0 +1,241 @@ +/** + * @vitest-environment node + * + * Pins the non-streaming status/error gate, and pins its `incomplete` policy to the one + * `streamResponsesTurn` applies so the two paths cannot silently diverge. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { executeResponsesProviderRequest } from '@/providers/openai/core' +import type { ProviderRequest, ProviderResponse } from '@/providers/types' + +vi.mock('@/providers', () => ({ MAX_TOOL_ITERATIONS: 5 })) + +vi.mock('@/providers/utils', () => ({ + isFunctionToolCall: () => false, + calculateCost: () => ({ input: 0, output: 0, total: 0 }), + sumToolCosts: () => 0, + enforceStrictSchema: (schema: unknown) => schema, + prepareToolExecution: () => ({ toolParams: {}, executionParams: {} }), + prepareToolsWithUsageControl: (tools: unknown[]) => ({ + tools, + toolChoice: undefined, + forcedTools: [], + hasFilteredTools: false, + }), + trackForcedToolUsage: () => ({ hasUsedForcedTool: false, usedForcedTools: [] }), + supportsReasoningEffort: () => false, +})) + +const { mockExecuteProviderTool } = vi.hoisted(() => ({ + mockExecuteProviderTool: vi.fn(), +})) + +vi.mock('@/providers/runtime-context', () => ({ + executeProviderTool: mockExecuteProviderTool, +})) + +function jsonResponse(body: unknown) { + return { + ok: true, + status: 200, + headers: new Headers(), + json: () => Promise.resolve(body), + } +} + +const USAGE = { input_tokens: 1, output_tokens: 1, total_tokens: 2 } + +function message(text: string) { + return { + type: 'message', + role: 'assistant', + content: [{ type: 'output_text', text }], + } +} + +function functionCall(args: string) { + return { type: 'function_call', call_id: 'call_1', name: 'exa_search', arguments: args } +} + +const COMPLETED_RESPONSE = { + id: 'resp_1', + status: 'completed', + error: null, + incomplete_details: null, + output: [message('hello')], + usage: USAGE, +} + +describe('OpenAI non-streaming response status handling', () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as any + + beforeEach(() => { + vi.clearAllMocks() + mockExecuteProviderTool.mockResolvedValue({ success: true, output: { results: [] } }) + }) + + function run(fetchMock: unknown, request: Partial = {}) { + return executeResponsesProviderRequest( + { apiKey: 'k', model: 'gpt-5.5', messages: [{ role: 'user', content: 'hi' }], ...request }, + { + providerId: 'openai', + providerLabel: 'OpenAI', + modelName: 'gpt-5.5', + endpoint: 'https://api.openai.com/v1/responses', + headers: { Authorization: 'Bearer k' }, + logger, + fetch: fetchMock as typeof fetch, + } + ) + } + + const TOOL_REQUEST: Partial = { + tools: [{ id: 'exa_search', name: 'exa_search', description: 'search', params: {} }], + } + + it('fails the block on a 200 carrying status "failed", surfacing the API error message', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'failed', + error: { code: 'server_error', message: 'The model produced an invalid response.' }, + incomplete_details: null, + output: [], + usage: USAGE, + }) + ) + + await expect(run(fetchMock)).rejects.toThrow('The model produced an invalid response.') + }) + + it('fails the block when error is populated but status is absent', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + error: { code: null, message: 'Upstream provider rejected the request.' }, + incomplete_details: null, + output: [], + usage: USAGE, + }) + ) + + await expect(run(fetchMock)).rejects.toThrow('Upstream provider rejected the request.') + }) + + /** Policy is shared with `streamResponsesTurn` — keep both in step. */ + it('returns the partial content of a max_output_tokens incomplete response instead of failing', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'incomplete', + error: null, + incomplete_details: { reason: 'max_output_tokens' }, + output: [message('a truncated but usable answer')], + usage: USAGE, + }) + ) + + const result = (await run(fetchMock)) as ProviderResponse + expect(result.content).toBe('a truncated but usable answer') + }) + + it('fails the block on an incomplete response whose reason is not max_output_tokens', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'incomplete', + error: null, + incomplete_details: { reason: 'content_filter' }, + output: [message('partial')], + usage: USAGE, + }) + ) + + await expect(run(fetchMock)).rejects.toThrow(/content_filter/) + }) + + /** + * The confusing-failure case: a truncated `function_call` holds half-written JSON. + * Executing it made `parseToolArguments` throw, reporting a tool bug rather than the + * truncation that actually happened. + */ + it('does not execute a tool call from a non-completed response', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'incomplete', + error: null, + incomplete_details: { reason: 'max_output_tokens' }, + output: [functionCall('{"query": "half writ')], + usage: USAGE, + }) + ) + + await expect(run(fetchMock, TOOL_REQUEST)).rejects.toThrow(/max_output_tokens/) + expect(mockExecuteProviderTool).not.toHaveBeenCalled() + }) + + it('leaves a healthy completed response entirely unaffected', async () => { + const fetchMock = vi.fn().mockResolvedValue(jsonResponse(COMPLETED_RESPONSE)) + + const result = (await run(fetchMock)) as ProviderResponse + expect(result.content).toBe('hello') + expect(result.toolCalls).toBeUndefined() + expect(result.tokens?.total).toBe(2) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('still runs the multi-turn tool loop end to end', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + id: 'resp_tool', + status: 'completed', + error: null, + incomplete_details: null, + output: [functionCall('{"query":"sim"}')], + usage: USAGE, + }) + ) + .mockResolvedValueOnce(jsonResponse(COMPLETED_RESPONSE)) + + const result = (await run(fetchMock, TOOL_REQUEST)) as ProviderResponse + + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(mockExecuteProviderTool).toHaveBeenCalledTimes(1) + expect(result.toolCalls).toHaveLength(1) + expect(result.toolCalls?.[0].success).toBe(true) + expect(result.content).toBe('hello') + expect(result.tokens?.total).toBe(4) + }) + + /** The gate lives in `postResponses`, so continuation turns are covered too. */ + it('fails the block when a later tool-loop turn comes back failed', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + id: 'resp_tool', + status: 'completed', + error: null, + incomplete_details: null, + output: [functionCall('{"query":"sim"}')], + usage: USAGE, + }) + ) + .mockResolvedValueOnce( + jsonResponse({ + id: 'resp_2', + status: 'failed', + error: { code: 'server_error', message: 'Second turn blew up.' }, + incomplete_details: null, + output: [], + usage: USAGE, + }) + ) + + await expect(run(fetchMock, TOOL_REQUEST)).rejects.toThrow('Second turn blew up.') + expect(mockExecuteProviderTool).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/sim/providers/openai/core.transport-phase.test.ts b/apps/sim/providers/openai/core.transport-phase.test.ts new file mode 100644 index 00000000000..74459fc40e5 --- /dev/null +++ b/apps/sim/providers/openai/core.transport-phase.test.ts @@ -0,0 +1,212 @@ +/** + * @vitest-environment node + * + * Covers the phase annotation that separates "never answered" from "answered, but the + * body never arrived" — the runtime reports both as a bare `TimeoutError`. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { executeResponsesProviderRequest } from '@/providers/openai/core' +import type { ProviderRequest } from '@/providers/types' + +const { mockSupportsReasoningEffort } = vi.hoisted(() => ({ + mockSupportsReasoningEffort: vi.fn(() => false), +})) + +vi.mock('@/providers', () => ({ MAX_TOOL_ITERATIONS: 5 })) + +vi.mock('@/providers/utils', () => ({ + isFunctionToolCall: () => false, + calculateCost: () => ({ input: 0, output: 0, total: 0 }), + sumToolCosts: () => 0, + enforceStrictSchema: (schema: unknown) => schema, + prepareToolExecution: () => ({ toolParams: {}, executionParams: {} }), + prepareToolsWithUsageControl: (tools: unknown[]) => ({ + tools, + toolChoice: undefined, + forcedTools: [], + hasFilteredTools: false, + }), + trackForcedToolUsage: () => ({ hasUsedForcedTool: false, usedForcedTools: [] }), + supportsReasoningEffort: mockSupportsReasoningEffort, +})) + +vi.mock('@/tools', () => ({ executeTool: vi.fn() })) + +/** + * Exactly what the runtime raises when a fetch deadline fires: a `DOMException`, NOT a + * plain `Error`. The distinction is load-bearing — `DOMException.message` is a readonly + * getter, so annotating by assignment throws a `TypeError` and replaces the real + * failure. Building a plain `Error` here would let that regression pass. + */ +function timeoutError() { + return new DOMException('The operation timed out.', 'TimeoutError') +} + +const COMPLETED = { + id: 'resp_1', + status: 'completed', + output: [{ type: 'message', role: 'assistant', content: [{ type: 'output_text', text: 'ok' }] }], + usage: { input_tokens: 1, output_tokens: 1, total_tokens: 2 }, +} + +describe('OpenAI transport phase annotation', () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as never + + beforeEach(() => { + vi.clearAllMocks() + mockSupportsReasoningEffort.mockReturnValue(false) + }) + + function run(fetchMock: unknown, request: Partial = {}) { + return executeResponsesProviderRequest( + { apiKey: 'k', model: 'gpt-5.5', messages: [{ role: 'user', content: 'hi' }], ...request }, + { + providerId: 'openai', + providerLabel: 'OpenAI', + modelName: 'gpt-5.5', + endpoint: 'https://api.openai.com/v1/responses', + headers: { Authorization: 'Bearer k' }, + logger, + fetch: fetchMock as typeof fetch, + } + ) + } + + /** + * The production case: `/v1/responses` withholds its 200 until generation finishes, so + * a runaway generation is still in the headers phase when the client gives up. + */ + it('names the header phase when the request was never answered', async () => { + const error = await run(vi.fn().mockRejectedValue(timeoutError())).catch((e) => e) + + expect(error.message).toContain('phase=awaiting-response-headers') + expect(error.message).toMatch(/elapsedMs=\d+/) + // No response existed, so no response metadata may be claimed. + expect(error.message).not.toContain('status=') + }) + + it('names the body phase when headers arrived but the body did not', async () => { + const stalled = { + ok: true, + status: 200, + headers: new Headers({ 'content-length': '32116', 'content-encoding': 'br' }), + json: () => Promise.reject(timeoutError()), + } + + const error = await run(vi.fn().mockResolvedValue(stalled)).catch((e) => e) + + expect(error.message).toContain('phase=reading-response-body') + expect(error.message).toContain('status=200') + expect(error.message).toContain('contentLength=32116') + expect(error.message).toContain('contentEncoding=br') + expect(error.message).toMatch(/ttfbMs=\d+/) + }) + + /** The only identifier the provider can trace a failed call by. */ + it('carries the x-request-id of a failed response', async () => { + const stalled = { + ok: true, + status: 200, + headers: new Headers({ 'x-request-id': 'req_abc123' }), + json: () => Promise.reject(timeoutError()), + } + + const error = await run(vi.fn().mockResolvedValue(stalled)).catch((e) => e) + expect(error.message).toContain('requestId=req_abc123') + }) + + it('leaves a self-describing API error untouched', async () => { + const apiError = { + ok: false, + status: 429, + headers: new Headers(), + text: () => Promise.resolve(JSON.stringify({ error: { message: 'Rate limit reached' } })), + } + + const error = await run(vi.fn().mockResolvedValue(apiError)).catch((e) => e) + expect(error.message).toContain('Rate limit reached') + expect(error.message).not.toContain('phase=') + }) + + it('bounds a non-JSON error body instead of pasting a gateway page into the error', async () => { + const htmlError = { + ok: false, + status: 502, + headers: new Headers(), + text: () => Promise.resolve(`${'x'.repeat(5000)}`), + } + + const error = await run(vi.fn().mockResolvedValue(htmlError)).catch((e) => e) + expect(error.message.length).toBeLessThan(700) + }) + + /** + * The bound applies only to non-JSON bodies. A structured provider error must survive + * intact, because the reasoning-summary strip-and-retry fallback matches on its text + * (`message.includes('reasoning.summary')`) — truncating it would silently disable + * that recovery path for any provider whose error message runs long. + */ + it('does not truncate a structured provider error, so the summary fallback still matches', async () => { + // Marker sits past the 500-char bound, so truncation would break the fallback match. + const longMessage = `${'context detail. '.repeat(40)}Invalid value for reasoning.summary: your organization must be verified to use this feature.` + expect(longMessage.indexOf('reasoning.summary')).toBeGreaterThan(500) + + const completed = { + ok: true, + status: 200, + headers: new Headers(), + json: () => Promise.resolve(COMPLETED), + } + const verificationError = { + ok: false, + status: 400, + headers: new Headers(), + text: () => Promise.resolve(JSON.stringify({ error: { message: longMessage } })), + } + const fetchMock = vi + .fn() + .mockResolvedValueOnce(verificationError) + .mockResolvedValueOnce(completed) + // The fallback only applies when the payload actually carried reasoning.summary. + mockSupportsReasoningEffort.mockReturnValue(true) + + await expect(run(fetchMock, { agentEvents: true })).resolves.toMatchObject({ content: 'ok' }) + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + /** + * Reading the error body of a non-OK response can itself hit the deadline or be + * cancelled. Swallowing that would report the HTTP status as the failure and lose both + * the transport detail and the fact that the user aborted. + */ + it('propagates a deadline hit while reading a non-OK error body', async () => { + const unreadable = { + ok: false, + status: 502, + headers: new Headers(), + text: () => Promise.reject(timeoutError()), + } + + const error = await run(vi.fn().mockResolvedValue(unreadable)).catch((e) => e) + + expect(error.message).toContain('The operation timed out.') + expect(error.message).not.toContain('API error') + // The headers already arrived, so this is the body phase despite the 4xx/5xx status. + expect(error.message).toContain('phase=reading-response-body') + expect(error.message).toContain('status=502') + // Annotated exactly once: the outer catch must not append a second, wrong phase. + expect(error.message.match(/phase=/g)).toHaveLength(1) + }) + + it('leaves a healthy response entirely unaffected', async () => { + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers(), + json: () => Promise.resolve(COMPLETED), + }) + + await expect(run(fetchMock)).resolves.toMatchObject({ content: 'ok' }) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/sim/providers/openai/core.ts b/apps/sim/providers/openai/core.ts index 47dd9e18b7c..739d6d21e87 100644 --- a/apps/sim/providers/openai/core.ts +++ b/apps/sim/providers/openai/core.ts @@ -2,6 +2,7 @@ import { createHash } from 'node:crypto' import type { Logger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' +import { truncate } from '@sim/utils/string' import type OpenAI from 'openai' import type { NormalizedBlockOutput, StreamingExecution } from '@/executor/types' import { MAX_TOOL_ITERATIONS } from '@/providers' @@ -33,12 +34,65 @@ import { createReadableStreamFromResponses, extractResponseText, extractResponseToolCalls, + isMaxOutputTokensIncompleteResponse, parseResponsesUsage, type ResponsesInputItem, type ResponsesToolCall, + responseContainsFunctionCall, toResponsesToolChoice, } from './utils' +/** + * Rejects a `/v1/responses` body reporting a generation that did not succeed — the + * endpoint answers HTTP 200 for both `status: 'failed'` and `status: 'incomplete'`. + * + * The tolerated case must stay matched to `streamResponsesTurn`: `incomplete` is accepted + * only when truncated by `max_output_tokens` AND carrying no function call. Truncated + * prose is a usable partial answer, but a truncated `function_call` holds half-written + * JSON that makes `parseToolArguments` throw a confusing tool failure. + * + * An absent `status` is deliberately not treated as a failure: this path is shared with + * Azure OpenAI and OpenAI-compatible gateways. + */ +function assertUsableResponse(response: OpenAI.Responses.Response, providerLabel: string): void { + if (response.error) { + const code = response.error.code ? ` (${response.error.code})` : '' + throw new Error(`${providerLabel} generation failed${code}: ${response.error.message}`) + } + + if (response.status === 'failed') { + throw new Error( + `${providerLabel} generation failed, and the API returned no error detail explaining why.` + ) + } + + if (response.status === 'incomplete') { + const reason = response.incomplete_details?.reason ?? 'unknown' + if (responseContainsFunctionCall(response)) { + throw new Error( + `${providerLabel} generation stopped before completion (${reason}), truncating a tool call mid-argument. Raise the max output tokens or reduce the tool schema size.` + ) + } + if (!isMaxOutputTokensIncompleteResponse(response)) { + throw new Error(`${providerLabel} generation stopped before completion: ${reason}.`) + } + return + } + + if (response.status && response.status !== 'completed') { + throw new Error( + `${providerLabel} returned a response with status "${response.status}", which carries no finished generation.` + ) + } +} + +/** + * Transport failures annotated once already. The error-body read is annotated where the + * phase is known, then rethrown through an outer catch that would otherwise append a + * second, wrong phase to the same message. + */ +const annotatedTransportFailures = new WeakSet() + type PreparedTools = ReturnType type ToolChoice = PreparedTools['toolChoice'] @@ -85,6 +139,9 @@ export async function executeResponsesProviderRequest( logger.info(`Preparing ${config.providerLabel} request`, { model: request.model, + workflowId: request.workflowId, + blockId: request.blockId, + executionId: request.executionId, hasSystemPrompt: !!request.systemPrompt, hasMessages: !!request.messages?.length, hasTools: !!request.tools?.length, @@ -237,14 +294,97 @@ export async function executeResponsesProviderRequest( ...overrides, }) - const parseErrorResponse = async (response: Response): Promise => { - const text = await response.text() + /** + * Names the request phase an opaque transport failure died in. + * + * Bun raises only `TimeoutError: The operation timed out.`, which cannot distinguish + * "never answered" from "answered, but the body never arrived" — opposite owners, + * opposite fixes. undici splits these as `UND_ERR_HEADERS_TIMEOUT` vs + * `UND_ERR_BODY_TIMEOUT`; this records the equivalent for a runtime that reports + * neither. + * + * The phase rides the error message because that reaches the block's trace span, which + * survives when a task has stopped shipping logs; `x-request-id` is the only handle the + * provider can trace the call by. Self-describing API errors are left untouched. + */ + const annotateTransportFailure = ( + error: unknown, + phase: 'awaiting-response-headers' | 'reading-response-body', + startedAt: number, + detail?: Record + ): unknown => { + if (!(error instanceof Error)) return error + if (error.name !== 'TimeoutError' && error.name !== 'AbortError') return error + if (annotatedTransportFailures.has(error)) return error + + const elapsedMs = Date.now() - startedAt + const fields = Object.entries(detail ?? {}) + .filter(([, value]) => value !== null && value !== undefined) + .map(([key, value]) => `${key}=${value}`) + const context = [`phase=${phase}`, `elapsedMs=${elapsedMs}`, ...fields].join(' ') + + logger.error(`${config.providerLabel} request failed in transport`, { + phase, + elapsedMs, + errorName: error.name, + model: config.modelName, + workflowId: request.workflowId, + blockId: request.blockId, + executionId: request.executionId, + ...detail, + }) + + /** + * A new Error rather than a mutation: the runtime raises these as `DOMException`, + * whose `message` is a readonly getter, so assigning to it throws a `TypeError` and + * destroys the very failure being reported. `name` is copied and the original hangs + * off `cause` so the classification survives the `ProviderError` wrapping below, + * which overwrites `name`. + */ + const annotated = new Error(`${error.message} [${context}]`, { cause: error }) + annotated.name = error.name + annotatedTransportFailures.add(annotated) + return annotated + } + + /** + * The response-side facts worth carrying on a transport failure. `x-request-id` is the + * only handle the provider can trace a failed call by. + */ + const describeResponse = (response: Response): Record => ({ + status: response.status, + requestId: response.headers.get('x-request-id'), + contentLength: response.headers.get('content-length'), + contentEncoding: response.headers.get('content-encoding'), + }) + + /** + * A non-JSON body is usually a gateway or CDN error page and reaches the user-facing + * block error, so it is bounded and falls back to `statusText`. A structured provider + * message is returned untruncated on purpose: the reasoning-summary strip-and-retry + * fallback matches on its text. + * + * A failed body read is annotated rather than swallowed: a deadline or a cancellation + * here must stay distinguishable from an error response that simply carried no body. + * The headers already arrived, so this is the body phase even though the status is 4xx. + */ + const parseErrorResponse = async (response: Response, startedAt: number): Promise => { + let text: string try { - const payload = JSON.parse(text) - return payload?.error?.message || text - } catch { - return text + text = await response.text() + } catch (error) { + throw annotateTransportFailure( + error, + 'reading-response-body', + startedAt, + describeResponse(response) + ) } + try { + const payload = JSON.parse(text) + if (payload?.error?.message) return payload.error.message + } catch {} + return truncate(text.trim(), 500) || response.statusText || `HTTP ${response.status}` } /** @@ -272,6 +412,7 @@ export async function executeResponsesProviderRequest( const fetchResponsesWithSummaryFallback = async ( requestedBody: Record, + startedAt: number, abortSignal = request.abortSignal ): Promise => { const body = reasoningSummariesUnavailable @@ -285,7 +426,7 @@ export async function executeResponsesProviderRequest( }) if (response.ok) return response - const message = await parseErrorResponse(response) + const message = await parseErrorResponse(response, startedAt) const strippedBody = isReasoningSummaryVerificationError(response.status, message) ? stripReasoningSummary(body) : null @@ -305,7 +446,7 @@ export async function executeResponsesProviderRequest( signal: abortSignal, }) if (!retryResponse.ok) { - const retryMessage = await parseErrorResponse(retryResponse) + const retryMessage = await parseErrorResponse(retryResponse, startedAt) throw new Error( `${config.providerLabel} API error (${retryResponse.status}): ${retryMessage}` ) @@ -316,8 +457,30 @@ export async function executeResponsesProviderRequest( const postResponses = async ( body: Record ): Promise => { - const response = await fetchResponsesWithSummaryFallback(body) - return response.json() + const startedAt = Date.now() + + let response: Response + try { + response = await fetchResponsesWithSummaryFallback(body, startedAt) + } catch (error) { + throw annotateTransportFailure(error, 'awaiting-response-headers', startedAt) + } + + const responseMeta = { ...describeResponse(response), ttfbMs: Date.now() - startedAt } + + let parsed: OpenAI.Responses.Response + try { + parsed = await response.json() + } catch (error) { + throw annotateTransportFailure(error, 'reading-response-body', startedAt, responseMeta) + } + + /** + * Placed here so every tool-loop turn is covered, and outside the transport `try` so + * a rejected generation is not misreported as a transport failure. + */ + assertUsableResponse(parsed, config.providerLabel) + return parsed } const providerStartTime = Date.now() @@ -355,7 +518,11 @@ export async function executeResponsesProviderRequest( initialToolChoice: responsesToolChoice, forcedTools: preparedTools?.forcedTools, createStream: (input, overrides, abortSignal) => - fetchResponsesWithSummaryFallback(createRequestBody(input, overrides), abortSignal), + fetchResponsesWithSummaryFallback( + createRequestBody(input, overrides), + Date.now(), + abortSignal + ), logger, timeSegments, onComplete: (result) => { @@ -379,7 +546,8 @@ export async function executeResponsesProviderRequest( logger.info(`Using streaming response for ${config.providerLabel} request`) const streamResponse = await fetchResponsesWithSummaryFallback( - createRequestBody(initialInput, { stream: true }) + createRequestBody(initialInput, { stream: true }), + Date.now() ) const streamingResult = createStreamingExecution({ @@ -722,10 +890,14 @@ export async function executeResponsesProviderRequest( throw error } - throw new ProviderError(toError(error).message, { - startTime: providerStartTimeISO, - endTime: providerEndTimeISO, - duration: totalDuration, - }) + throw new ProviderError( + toError(error).message, + { + startTime: providerStartTimeISO, + endTime: providerEndTimeISO, + duration: totalDuration, + }, + { cause: error } + ) } } diff --git a/apps/sim/providers/types.ts b/apps/sim/providers/types.ts index 7c402d66677..e029f830d2c 100644 --- a/apps/sim/providers/types.ts +++ b/apps/sim/providers/types.ts @@ -241,8 +241,17 @@ export class ProviderError extends Error { duration: number } - constructor(message: string, timing: { startTime: string; endTime: string; duration: number }) { - super(message) + /** + * `options.cause` should carry the error being wrapped. `name` is deliberately + * overwritten with `'ProviderError'`, so without a cause every classification the + * original carried — notably a transport `TimeoutError` — is lost to callers. + */ + constructor( + message: string, + timing: { startTime: string; endTime: string; duration: number }, + options?: ErrorOptions + ) { + super(message, options) this.name = 'ProviderError' this.timing = timing }