fix(logger, blocks): log nested errors and stop spurious model-selection warnings - #6706
Conversation
…ion 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`
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Model blocks: Agent, Router, and Evaluator Tests: Coverage for gateway models, variable references, resolver failures, and the production Reviewed by Cursor Bugbot for commit 523a67f. Configure here. |
Greptile SummaryThe PR improves nested
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/logger/src/index.ts | Adds shared Error normalization and applies it to keyed errors in structured and colorized logging paths. |
| apps/sim/blocks/utils.ts | Adds a documented, non-throwing serialization-time model-provider resolver with explicit fallbacks. |
| apps/sim/blocks/blocks/agent.ts | Uses the shared provider resolver while retaining the Agent block’s auto-model fallback behavior. |
| apps/sim/blocks/blocks/evaluator.ts | Replaces base-model-map validation with shared serialization-time provider resolution. |
| apps/sim/blocks/blocks/router.ts | Applies shared provider resolution to both legacy and current Router block configurations. |
Reviews (2): Last reviewed commit: "fix(blocks): move the fallback rationale..." | Re-trigger Greptile
The repo forbids non-TSDoc comments; the explanation for why recovery returns a constant instead of resolving again belongs on the declaration anyway.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 523a67f. Configure here.
Summary
Two chronic defects found while auditing production logs. Neither is release-related — both predate v0.8.1.
1.
logger.x('msg', { error })loggederror: {}at 399 call sites.mergeArgscopied object arguments verbatim, so an Error held under a key stayed an Error instance — andJSON.stringifyrenders that as{}, becausemessageandstackare non-enumerable onError.prototype. A bareErrorargument was handled correctly; an error inside an object was not. This is whyBlockOutputs :: Failed to get tool outputs(10k/week) was undiagnosable. The colorized/dev path had the same hole viaformatObject, so a developer running locally saw the bug too.errorstays a plain message string so log queries canstats count(*) by error; richer diagnostics remain opt-in viadescribeError.2. Router/Evaluator/Agent logged
Invalid model selected~140×/day.config.toolresolved throughgetBaseModelProviders(), which deliberately excludes gateway providers (OpenRouter, vLLM, LiteLLM, Ollama, fireworks, together, baseten). So a fully validopenrouter/meta-llama/llama-4-maverickthrew, as did a model still holding an unresolved<variable.x>at serialization time.The serialized value is cosmetic — every one of these blocks re-derives the provider from the resolved model at execution (
router-handler.ts:112,evaluator-handler.ts:177), andagent.tsalready documented that it "never reads this serialized value". So the throw bought nothing and produced an error users cannot act on.getProviderFromModel, the same resolver the executor uses, so serialization and execution finally agree.if (!model)checks sitting behindparams.model || 'gpt-4o'.Type of Change
Testing
1,189 tests pass across blocks, serializer, providers and executor handlers, plus the logger package; type-check and biome clean.
New tests, including two added specifically because review found the first pass insufficient:
stack; sibling keys survive. Verified they fail without the fix withexpected {} to be 'boom'— the exact production symptom.openrouter/*resolves viamodelPatternsand thatgetBaseModelProviders()omits it — the mocked helper test alone would have passed even if the fix were deleted.Notes
#2 has no user-facing impact— I checked before treating it as one.getToolParamsreturnsundefinedfor both the provider id and theaccess[0]fallback (neither is a registered tool), so the dependent validation was skipped either way. This removes log noise and makes serialization agree with execution; it does not change behavior.Checklist