Skip to content

Commit 2fedeb0

Browse files
waleedlatif1waleedlatif1
committed
fix(logger, blocks): log nested errors and stop spurious model-selection warnings
Two production defects found in the prod logs, neither release-related. logger: mergeArgs copied object arguments verbatim, so an Error held under a key stayed an Error instance and JSON.stringify rendered it {} — message and stack are non-enumerable on Error.prototype. 399 call sites use the logger.x('msg', { error }) shape and every one logged error: {}, which is why BlockOutputs failures (10k/week) were undiagnosable. The colorized path had the same hole via formatObject. blocks: router, evaluator and agent resolved config.tool through getBaseModelProviders(), which deliberately excludes gateway providers (OpenRouter, vLLM, LiteLLM, Ollama, ...). A valid openrouter/* model therefore threw "Invalid model selected", as did a model still holding an unresolved <variable.x> at serialization time — ~140 warnings/day. The value is cosmetic (every handler re-derives the provider from the resolved model), so the throw bought nothing. - unwrap keyed Errors on both the structured and colorized log paths - keep `error` a plain message string so log queries can group on it - resolve serialized provider ids through getProviderFromModel, the same resolver the executor uses, via one shared helper for all four call sites - drop the unreachable `if (!model)` checks behind `params.model || default`
1 parent a7115e8 commit 2fedeb0

8 files changed

Lines changed: 204 additions & 63 deletions

File tree

‎apps/sim/blocks/blocks/agent.ts‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
getModelCapabilityCondition,
77
getModelOptions,
88
getProviderCredentialSubBlocks,
9+
getSerializedModelProviderId,
910
normalizeFileInput,
1011
RESPONSE_FORMAT_WAND_CONFIG,
1112
} from '@/blocks/utils'
@@ -29,6 +30,9 @@ import { useSubBlockStore } from '@/stores/workflows/subblock/store'
2930
import type { ToolResponse } from '@/tools/types'
3031

3132
const logger = createLogger('AgentBlock')
33+
34+
/** Model the agent block falls back to when `model` is unset or the auto pseudo-model. */
35+
const AGENT_FALLBACK_MODEL = 'claude-sonnet-5'
3236
const MODELS_WITH_REASONING_EFFORT = getModelsWithReasoningEffort()
3337
const MODELS_WITH_VERBOSITY = getModelsWithVerbosity()
3438
const MODELS_WITH_THINKING = getModelsWithThinking()
@@ -521,21 +525,10 @@ Return ONLY the JSON array.`,
521525
],
522526
config: {
523527
tool: (params: Record<string, any>) => {
524-
const model = params.model || 'claude-sonnet-5'
525-
if (!model) {
526-
throw new Error('No model selected')
527-
}
528-
// sim-auto resolves to a concrete pool model at execution time, where
529-
// the agent handler derives the provider from the resolved model and
530-
// never reads this serialized value. Serialization still needs the
531-
// same provider-id shape every other model stores, so look up the
532-
// runtime fallback model's provider.
533-
const lookupModel = isAutoModel(model) ? 'claude-sonnet-5' : model
534-
const tool = getBaseModelProviders()[lookupModel]
535-
if (!tool) {
536-
throw new Error(`Invalid model selected: ${model}`)
537-
}
538-
return tool
528+
const model = params.model || AGENT_FALLBACK_MODEL
529+
// sim-auto has no provider of its own until the pool resolves it at execution time.
530+
const lookupModel = isAutoModel(model) ? AGENT_FALLBACK_MODEL : model
531+
return getSerializedModelProviderId(lookupModel, AGENT_FALLBACK_MODEL)
539532
},
540533
params: (params: Record<string, any>) => {
541534
const normalizedFiles = normalizeFileInput(params.files)

‎apps/sim/blocks/blocks/evaluator.ts‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,9 @@ import type { BlockConfig, ParamType } from '@/blocks/types'
44
import {
55
getModelOptions,
66
getProviderCredentialSubBlocks,
7+
getSerializedModelProviderId,
78
PROVIDER_CREDENTIAL_INPUTS,
89
} from '@/blocks/utils'
9-
import { getBaseModelProviders } from '@/providers/models'
10-
import type { ProviderId } from '@/providers/types'
1110
import type { ToolResponse } from '@/tools/types'
1211

1312
const logger = createLogger('EvaluatorBlock')
@@ -253,17 +252,7 @@ export const EvaluatorBlock: BlockConfig<EvaluatorResponse> = {
253252
'deepseek_reasoner',
254253
],
255254
config: {
256-
tool: (params: Record<string, any>) => {
257-
const model = params.model || 'gpt-4o'
258-
if (!model) {
259-
throw new Error('No model selected')
260-
}
261-
const tool = getBaseModelProviders()[model as ProviderId]
262-
if (!tool) {
263-
throw new Error(`Invalid model selected: ${model}`)
264-
}
265-
return tool
266-
},
255+
tool: (params: Record<string, any>) => getSerializedModelProviderId(params.model),
267256
},
268257
},
269258
inputs: {

‎apps/sim/blocks/blocks/router.ts‎

Lines changed: 3 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,9 @@ import { AuthMode, type BlockConfig } from '@/blocks/types'
33
import {
44
getModelOptions,
55
getProviderCredentialSubBlocks,
6+
getSerializedModelProviderId,
67
PROVIDER_CREDENTIAL_INPUTS,
78
} from '@/blocks/utils'
8-
import { getBaseModelProviders } from '@/providers/models'
9-
import type { ProviderId } from '@/providers/types'
109
import type { ToolResponse } from '@/tools/types'
1110

1211
interface RouterResponse extends ToolResponse {
@@ -215,17 +214,7 @@ export const RouterBlock: BlockConfig<RouterResponse> = {
215214
'deepseek_reasoner',
216215
],
217216
config: {
218-
tool: (params: Record<string, any>) => {
219-
const model = params.model || 'gpt-4o'
220-
if (!model) {
221-
throw new Error('No model selected')
222-
}
223-
const tool = getBaseModelProviders()[model as ProviderId]
224-
if (!tool) {
225-
throw new Error(`Invalid model selected: ${model}`)
226-
}
227-
return tool
228-
},
217+
tool: (params: Record<string, any>) => getSerializedModelProviderId(params.model),
229218
},
230219
},
231220
inputs: {
@@ -325,17 +314,7 @@ export const RouterV2Block: BlockConfig<RouterV2Response> = {
325314
'deepseek_reasoner',
326315
],
327316
config: {
328-
tool: (params: Record<string, any>) => {
329-
const model = params.model || 'gpt-4o'
330-
if (!model) {
331-
throw new Error('No model selected')
332-
}
333-
const tool = getBaseModelProviders()[model as ProviderId]
334-
if (!tool) {
335-
throw new Error(`Invalid model selected: ${model}`)
336-
}
337-
return tool
338-
},
317+
tool: (params: Record<string, any>) => getSerializedModelProviderId(params.model),
339318
},
340319
},
341320
inputs: {

‎apps/sim/blocks/utils.test.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,13 @@ import {
7272
BUILT_IN_TOOL_TYPES,
7373
getApiKeyCondition,
7474
getDependsOnFields,
75+
getSerializedModelProviderId,
7576
getSubBlocksDependingOnChange,
7677
parseOptionalBooleanInput,
7778
parseOptionalJsonInput,
7879
parseOptionalNumberInput,
7980
} from '@/blocks/utils'
81+
import { getProviderFromModel } from '@/providers/utils'
8082

8183
describe('BUILT_IN_TOOL_TYPES', () => {
8284
it('classifies the current File block instead of the legacy File block', () => {
@@ -464,3 +466,46 @@ describe('getSubBlocksDependingOnChange', () => {
464466
).toEqual(['projectId'])
465467
})
466468
})
469+
470+
describe('getSerializedModelProviderId', () => {
471+
const resolver = vi.mocked(getProviderFromModel)
472+
473+
beforeEach(() => {
474+
resolver.mockReset()
475+
resolver.mockImplementation(((model: string) => {
476+
if (model.startsWith('openrouter/')) return 'openrouter'
477+
if (model === 'gpt-4o') return 'openai'
478+
if (model === 'claude-sonnet-5') return 'anthropic'
479+
throw new Error(`No provider found for model: ${model}`)
480+
}) as unknown as typeof getProviderFromModel)
481+
})
482+
483+
it('resolves a gateway model that the base model map deliberately omits', () => {
484+
expect(getSerializedModelProviderId('openrouter/meta-llama/llama-4-maverick')).toBe(
485+
'openrouter'
486+
)
487+
})
488+
489+
it('uses the fallback model when the model is still an unresolved reference', () => {
490+
expect(getSerializedModelProviderId('openrouter/<variable.vllm>')).toBe('openai')
491+
expect(resolver).not.toHaveBeenCalledWith('openrouter/<variable.vllm>')
492+
})
493+
494+
it('honours a caller-supplied fallback model', () => {
495+
expect(getSerializedModelProviderId(undefined, 'claude-sonnet-5')).toBe('anthropic')
496+
})
497+
498+
it('never throws when the resolver rejects the model', () => {
499+
expect(() => getSerializedModelProviderId('totally-unknown-model')).not.toThrow()
500+
expect(getSerializedModelProviderId('totally-unknown-model')).toBe('openai')
501+
})
502+
503+
it('never throws when the resolver rejects every model, including the fallback', () => {
504+
resolver.mockImplementation((() => {
505+
throw new Error('Provider "openai" is not available')
506+
}) as unknown as typeof getProviderFromModel)
507+
508+
expect(() => getSerializedModelProviderId('gpt-4o')).not.toThrow()
509+
expect(getSerializedModelProviderId('gpt-4o')).toBe('openai')
510+
})
511+
})

‎apps/sim/blocks/utils.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
SIM_AUTO_MODEL_ID,
2222
} from '@/providers/models'
2323
import { isPiSupportedModel } from '@/providers/pi-providers'
24+
import type { ProviderId } from '@/providers/types'
2425
import { getProviderFromModel } from '@/providers/utils'
2526
import { useProvidersStore } from '@/stores/providers/store'
2627

@@ -236,6 +237,45 @@ function shouldRequireApiKeyForModel(model: string): boolean {
236237
return true
237238
}
238239

240+
/** Model whose provider is recorded when a block's own `model` cannot be resolved. */
241+
const SERIALIZATION_FALLBACK_MODEL = 'gpt-4o'
242+
243+
/** Last-resort provider for when even {@link SERIALIZATION_FALLBACK_MODEL} cannot be resolved. */
244+
const SERIALIZATION_FALLBACK_PROVIDER: ProviderId = 'openai'
245+
246+
/**
247+
* Provider id a model-driven block records for `model` during serialization.
248+
*
249+
* Serialization runs before variable resolution, and every model block's handler
250+
* re-derives the provider from the *resolved* model without ever reading this
251+
* value — so it only has to be shape-correct, and it must never throw. Two cases
252+
* reach here that {@link getBaseModelProviders} cannot answer: `model` may still
253+
* hold a `<variable.x>` reference, and gateway providers (OpenRouter, vLLM,
254+
* LiteLLM, Ollama, …) are deliberately absent from that map even when the model
255+
* id is perfectly valid. A reference resolves to {@link SERIALIZATION_FALLBACK_MODEL}'s
256+
* provider; anything else is left to `getProviderFromModel`, which defaults an
257+
* unrecognised id to `ollama` rather than failing serialization with an error the
258+
* user cannot act on.
259+
*/
260+
export function getSerializedModelProviderId(
261+
model: unknown,
262+
fallbackModel: string = SERIALIZATION_FALLBACK_MODEL
263+
): ProviderId {
264+
const candidate =
265+
typeof model === 'string' && model && !containsReference(model) ? model : fallbackModel
266+
267+
try {
268+
return getProviderFromModel(candidate)
269+
} catch {
270+
/*
271+
* Not a second resolve: `getProviderFromModel` also throws for a blacklisted
272+
* provider or model, so a deployment that blacklists the fallback would throw
273+
* here too — the one case this helper exists to absorb.
274+
*/
275+
return SERIALIZATION_FALLBACK_PROVIDER
276+
}
277+
}
278+
239279
/**
240280
* Visibility condition for a model-tuning field that only some models accept, such as
241281
* reasoning effort or verbosity. Gates on the capability list, but keeps the field visible

‎apps/sim/providers/utils.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -948,6 +948,15 @@ describe('Provider Management', () => {
948948
expect(getProviderFromModel('unknown-model')).toBe('ollama')
949949
})
950950

951+
it('should resolve gateway models that getBaseModelProviders deliberately omits', () => {
952+
// getBaseModelProviders() filters these providers out entirely, so a model
953+
// block that looked models up there rejected valid ids like these.
954+
expect(getProviderFromModel('openrouter/meta-llama/llama-4-maverick')).toBe('openrouter')
955+
expect(getProviderFromModel('together/some-model')).toBe('together')
956+
expect(getProviderFromModel('fireworks/some-model')).toBe('fireworks')
957+
expect(getBaseModelProviders()['openrouter/meta-llama/llama-4-maverick']).toBeUndefined()
958+
})
959+
951960
it('should be case insensitive', () => {
952961
expect(getProviderFromModel('GPT-4O')).toBe('openai')
953962
expect(getProviderFromModel('CLAUDE-SONNET-4-0')).toBe('anthropic')

‎packages/logger/src/index.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,46 @@ describe('Logger', () => {
263263
expect(parsed.self.self).toBe('[Circular]')
264264
})
265265

266+
test('should render an Error held under a key as its message, not {}', () => {
267+
const error = new Error('boom')
268+
269+
createEnabledLogger().error('failed', { error })
270+
271+
const parsed = JSON.parse(consoleErrorSpy.mock.calls[0][0] as string)
272+
expect(parsed.error).toBe('boom')
273+
expect(parsed.stack).toBe(error.stack)
274+
})
275+
276+
test('should render an Error under a non-conventional key without hijacking stack', () => {
277+
createEnabledLogger().error('failed', { cause: new Error('inner'), stack: 'caller-supplied' })
278+
279+
const parsed = JSON.parse(consoleErrorSpy.mock.calls[0][0] as string)
280+
expect(parsed.cause).toBe('inner')
281+
expect(parsed.stack).toBe('caller-supplied')
282+
})
283+
284+
test('should keep sibling keys alongside an Error value', () => {
285+
createEnabledLogger().error('failed', { error: new Error('boom'), toolId: 'slack_message' })
286+
287+
const parsed = JSON.parse(consoleErrorSpy.mock.calls[0][0] as string)
288+
expect(parsed.error).toBe('boom')
289+
expect(parsed.toolId).toBe('slack_message')
290+
})
291+
292+
test('should unwrap an Error held under a key on the colorized path too', () => {
293+
const colorized = new Logger('Test', {
294+
enabled: true,
295+
colorize: true,
296+
logLevel: LogLevel.DEBUG,
297+
})
298+
299+
colorized.error('failed', { error: new Error('boom') })
300+
301+
const printed = consoleErrorSpy.mock.calls[0].join(' ')
302+
expect(printed).toContain('boom')
303+
expect(printed).not.toContain('"error":{}')
304+
})
305+
266306
test('should emit a line instead of throwing on BigInt metadata', () => {
267307
expect(() => createEnabledLogger().error('boom', { size: 10n })).not.toThrow()
268308
expect(consoleErrorSpy).toHaveBeenCalledTimes(1)

‎packages/logger/src/index.ts‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -130,39 +130,85 @@ const getLogConfig = () => {
130130
}
131131
}
132132

133+
/**
134+
* Renders an error as the plain object `JSON.stringify` cannot produce for it.
135+
*
136+
* `message`, `stack` and `name` are non-enumerable on `Error.prototype`, so a
137+
* plain stringify emits `{}`. Own enumerable properties are copied too — driver
138+
* and HTTP errors carry the useful part (`code`, `status`) there.
139+
*/
140+
const errorToPlainObject = (error: Error, isDev: boolean): Record<string, unknown> => {
141+
const errorObj: Record<string, unknown> = {
142+
message: error.message,
143+
stack: isDev ? error.stack : undefined,
144+
name: error.name,
145+
}
146+
for (const key of Object.keys(error)) {
147+
if (!(key in errorObj)) {
148+
errorObj[key] = (error as unknown as Record<string, unknown>)[key]
149+
}
150+
}
151+
return errorObj
152+
}
153+
133154
/**
134155
* Format objects for logging
156+
*
157+
* Errors held under a key are unwrapped as well as bare ones — `{ error }` is
158+
* the common call shape, and it would otherwise print as `{"error":{}}`.
135159
*/
136160
const formatObject = (obj: unknown, isDev: boolean): string => {
137161
try {
138162
if (obj instanceof Error) {
139-
const errorObj: Record<string, unknown> = {
140-
message: obj.message,
141-
stack: isDev ? obj.stack : undefined,
142-
name: obj.name,
163+
return JSON.stringify(errorToPlainObject(obj, isDev), null, isDev ? 2 : 0)
164+
}
165+
if (obj && typeof obj === 'object' && !Array.isArray(obj)) {
166+
let unwrapped: Record<string, unknown> | undefined
167+
for (const [key, value] of Object.entries(obj as Record<string, unknown>)) {
168+
if (!(value instanceof Error)) continue
169+
unwrapped ??= { ...(obj as Record<string, unknown>) }
170+
unwrapped[key] = errorToPlainObject(value, isDev)
143171
}
144-
for (const key of Object.keys(obj)) {
145-
if (!(key in errorObj)) {
146-
errorObj[key] = (obj as unknown as Record<string, unknown>)[key]
147-
}
172+
if (unwrapped) {
173+
return JSON.stringify(unwrapped, null, isDev ? 2 : 0)
148174
}
149-
return JSON.stringify(errorObj, null, isDev ? 2 : 0)
150175
}
151176
return JSON.stringify(obj, null, isDev ? 2 : 0)
152177
} catch {
153178
return '[Circular or Non-Serializable Object]'
154179
}
155180
}
156181

157-
/** Merges caller-supplied log arguments into the structured entry. */
182+
/**
183+
* Merges caller-supplied log arguments into the structured entry.
184+
*
185+
* `Error.message` and `Error.stack` are non-enumerable, so `JSON.stringify`
186+
* renders an error held under a key as `{}` — and `logger.x('...', { error })`
187+
* is by far the most common call shape, which would otherwise reduce the one
188+
* field worth reading to an empty object. Errors nested in an object argument
189+
* are therefore unwrapped like a bare `Error` argument. `error` stays a plain
190+
* message string so log queries can group on it; richer diagnostics are opt-in
191+
* via `describeError` from `@sim/utils/errors`.
192+
*/
158193
const mergeArgs = (entry: Record<string, unknown>, args: unknown[]): Record<string, unknown> => {
159194
for (const arg of args) {
160195
if (arg === null || arg === undefined) continue
161196
if (arg instanceof Error) {
162197
entry.error = arg.message
163198
entry.stack = arg.stack
164199
} else if (typeof arg === 'object') {
165-
Object.assign(entry, arg)
200+
const source = arg as Record<string, unknown>
201+
for (const key of Object.keys(source)) {
202+
const value = source[key]
203+
if (value instanceof Error) {
204+
entry[key] = value.message
205+
if (key === 'error' && entry.stack === undefined) {
206+
entry.stack = value.stack
207+
}
208+
} else {
209+
entry[key] = value
210+
}
211+
}
166212
} else {
167213
entry.extra = arg
168214
}

0 commit comments

Comments
 (0)