Repository navigation
🔊 fix: surface provider, model, and status in summarization error logs - #109
Conversation
…r logs When summarization fails, host applications (e.g. LibreChat's default console formatter) frequently strip metadata from the winston info object, leaving users with only a generic "Summarization LLM call failed" message — no indication of which provider/model misbehaved or why. Fold the critical bits (provider/model label and HTTP status when present) into the log message string itself so they survive any downstream formatter. Keep the structured metadata bag intact for JSON/structured backends. Apply the same treatment at each summarization error site (primary failure, fallback failure, metadata-stub fallback).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0afe3e6a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const status = | ||
| errObj.status ?? errObj.statusCode ?? errObj.response?.status; |
There was a problem hiding this comment.
describeProviderError treats err as an object and immediately reads status/statusCode/response.status; if a provider throws null/undefined (valid JS throw values), this code raises a new TypeError inside the catch path. That aborts the intended fallback behavior and can prevent metadata-stub recovery, turning a handled summarization failure into an unhandled crash. Add a non-null object guard before accessing status properties.
Useful? React with 👍 / 👎.
| fbErr, | ||
| clientConfig.provider, | ||
| clientConfig.modelName |
There was a problem hiding this comment.
The fallback error logger passes clientConfig.provider/modelName (the primary model) into describeProviderError, even though fbErr comes from tryFallbackProviders and may be from a different provider. In mixed-provider setups this mislabels fallback failures, which can send operators to the wrong integration while debugging incidents.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0afe3e6a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fbDescribed = describeProviderError( | ||
| fbErr, | ||
| clientConfig.provider, | ||
| clientConfig.modelName |
There was a problem hiding this comment.
fbErr here comes from tryFallbackProviders, which throws the last fallback provider error after iterating fallbacks (src/llm/invoke.ts). Logging it with clientConfig.provider/clientConfig.modelName always attributes that failure to the primary provider, so operators can be sent to the wrong deployment when diagnosing incidents; the provider/model in this warning should reflect the fallback that actually failed (or be omitted if unavailable).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b6d27a. Added describeFallbackError which labels the log with the list of fallback providers attempted instead of reusing the primary's provider/model.
| const status = | ||
| errObj.status ?? errObj.statusCode ?? errObj.response?.status; |
There was a problem hiding this comment.
describeProviderError assumes the caught value is property-accessible, but catch can receive null/undefined; in that case errObj.status throws a new TypeError and masks the original provider failure. This can break the intended graceful path (including metadata-stub fallback) specifically when an upstream integration throws a nullish value, so status extraction needs a null/object guard or optional chaining on err itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b6d27a. Extracted an extractHttpStatus helper with an explicit err == null || typeof err !== 'object' guard. Good catch — TypeScript's no-unnecessary-condition rule had stripped the optional chaining after lint-fix, which is exactly what you flagged.
…t provider Two issues surfaced by code review on the initial logging pass: 1. `describeProviderError` cast `err` to a non-nullable object and read `.status`/`.statusCode`/`.response?.status`; TypeScript's no-unnecessary- condition rule then stripped the optional chaining on `errObj`. If a provider throws `null` or `undefined` (both legal throw targets in JS), the status extraction would itself raise `TypeError` and mask the original summarization failure. Extract status through a dedicated helper with an explicit `err == null` guard so the intended metadata-stub recovery path is preserved for any thrown value. 2. The fallback-exhausted warning reused the primary provider/model label for the error thrown out of `tryFallbackProviders`. That error actually comes from the last attempted fallback and is not necessarily the primary integration. Introduce `describeFallbackError` which labels the log with the list of fallback providers attempted, so operators are pointed at the right deployment when diagnosing incidents. Reviewed-by: Codex (P1 + P2 findings on PR #109).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b6d27aa7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fallbacks: ReadonlyArray<{ provider: string | Providers }> | ||
| ): { suffix: string; data: Record<string, unknown> } { | ||
| const errMsg = err instanceof Error ? err.message : String(err); | ||
| const providerNames = fallbacks.map((f) => String(f.provider)); |
There was a problem hiding this comment.
describeFallbackError assumes every element in fallbacks is a non-null object and immediately does fallbacks.map((f) => String(f.provider)). When runtime config is malformed (for example fallbacks: [undefined] or a sparse array), tryFallbackProviders can still catch per-fallback failures, but this helper throws inside the surrounding catch (fbErr) block, which prevents the code from reaching the metadata-stub fallback path. In that scenario, a recoverable summarization failure becomes an uncaught exception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in dc5456e. Widened describeFallbackError's parameter type to ReadonlyArray<unknown>, filter out malformed entries (null/undefined/missing provider), and fall back to a no-fallbacks label when none remain. The logging path is now total on any input.
`describeFallbackError` previously trusted that every element in the `fallbacks` array was a non-null object with a `provider` field. If runtime config is malformed (null/undefined entries, missing provider), reading `.provider` from inside the surrounding `catch (fbErr)` block would throw a fresh TypeError, promoting a recoverable summarization failure into an uncaught exception and blocking the metadata-stub fallback. Widen the parameter to `ReadonlyArray<unknown>`, filter out malformed entries, and fall back to `no-fallbacks` label when none remain. The logging path is now total on any input. Reviewed-by: Codex (P2 finding on PR #109, commit 8b6d27a).
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc5456ee42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fallbacks: ReadonlyArray<unknown> | ||
| ): { suffix: string; data: Record<string, unknown> } { | ||
| const errMsg = err instanceof Error ? err.message : String(err); | ||
| const providerNames = fallbacks |
There was a problem hiding this comment.
describeFallbackError assumes fallbacks is an array and calls .map(...), but the caller sources fallbacks from clientOptions via an unchecked cast from unknown. If runtime config provides a malformed non-array value with a truthy length (for example a string), tryFallbackProviders can fail and this catch-path formatter then throws TypeError: fallbacks.map is not a function, which escapes the error handler and prevents the metadata-stub fallback from running.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bff9449. Widened the parameter to unknown and coerced with Array.isArray; also coerce the call-site fallbacks binding so tryFallbackProviders never sees a non-array. The error-logging path is now total on any input shape.
`describeFallbackError` previously assumed `fallbacks` was an array because the type signature said so. In practice the value originates from `clientConfig.clientOptions` via an unchecked `unknown` cast, so a malformed runtime config (e.g. a string with truthy `length`) would reach `.map(...)` and throw `TypeError: fallbacks.map is not a function` — escaping the surrounding `catch (fbErr)` and blocking the metadata-stub recovery path. Widen the parameter to `unknown`, coerce to an empty array when it isn't one. Also coerce the call site's `fallbacks` binding with `Array.isArray` so `tryFallbackProviders` doesn't see a non-array either. Reviewed-by: Codex (P2 finding on PR #109, commit dc5456e).
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
When summarization fails, host applications (e.g. LibreChat's default console formatter) frequently strip metadata from the winston
infoobject — the user ends up seeing only a genericSummarization LLM call failedline with no indication of which provider/model misbehaved or what the underlying error actually was.This PR folds the critical bits (provider/model label, and HTTP status when available) directly into the log message string so they survive any downstream formatter. Structured metadata stays on the event for JSON backends.
Before
After
Changes
describeProviderErrorhelper that produces a log-string suffix ([provider/model]+ optional(HTTP <n>)+ error message) and a structured metadata bag (provider,model,errorName,errorStack,status).Motivation
Downstream discussion: LibreChat-AI/LibreChat#12733
The symptom masks at least three distinct bugs (discussion #12614, issue #12721, and the reported discussion itself). Surfacing the provider, model, and HTTP status in the log message is the single highest-leverage improvement for diagnosing the whole class.
Test plan
npx jest --testPathPatterns 'src/summarization/__tests__'— all 24 existing tests pass.npx tsc --noEmit— no new type errors.