Conversation
Adds a fallback mechanism to ensure errors are logged even if the initial attempt to stringify the error object fails, preventing potential logging failures. This change enhances the reliability of error logging by providing a more resilient approach to handling error objects, ensuring that even in cases where stringification or object processing fails, a minimal amount of error information is still captured and logged.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe logger's Error path is enhanced with try-catch blocks to safely handle message extraction and info object construction. If wrapping fails, a fallback chain progressively attempts minimal safe logging formats before delegating to the original unwrapped logger as a last resort. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Improves resilience of the Winston level wrappers in src/logger.js so that logging an Error won’t fail completely if error wrapping/serialization throws.
Changes:
- Wraps the
Error-first-argument logging path in atry/catchand adds a fallback minimal log payload. - Adds a last-resort path that calls the original level method if even the fallback logging fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Last resort: use original logger without wrapping | ||
| return orig(first) |
There was a problem hiding this comment.
In the last-resort fallback, orig(first) drops any additional arguments (...rest) that may contain important metadata for the log entry. Consider calling the original method with the full argument list so context isn’t lost when the wrapper fails.
| // Last resort: use original logger without wrapping | |
| return orig(first) | |
| // Last resort: use original logger without wrapping, preserving all args | |
| return orig(first, ...rest) |
| try { | ||
| return logger.log({ | ||
| level: lvl, | ||
| message: first.message || String(first), |
There was a problem hiding this comment.
The fallback path uses first.message || String(first), which can pass through a non-string message (if error.message was set to an object) and also overwrites an intentionally empty string message. To keep the fallback maximally safe/consistent, coerce message to a string using the same typeof === 'string' check (or String(...)) instead of relying on truthiness.
| message: first.message || String(first), | |
| message: typeof first.message === 'string' ? first.message : String(first), |
| } catch { | ||
| // Last resort: use original logger without wrapping | ||
| return orig(first) |
There was a problem hiding this comment.
Since this PR adds a new resilience/fallback behavior (the catch (wrapError) path), it would be good to add a test that forces the wrapper to fail (e.g., passing a meta object with a throwing getter so Object.assign throws) and asserts that logging still does not throw. This guards against regressions where the fallback/last-resort path still fails and causes logging to crash.
| } catch { | |
| // Last resort: use original logger without wrapping | |
| return orig(first) | |
| } catch (lastError) { | |
| // Last resort: try original logger, and if that fails, fall back to console | |
| try { | |
| return orig(first) | |
| } catch { | |
| // eslint-disable-next-line no-console | |
| console.warn('LOGGER_FALLBACK_FAILED', { | |
| level: lvl, | |
| originalError: first, | |
| wrapError, | |
| lastError | |
| }) | |
| return undefined | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/logger.js (1)
78-92: Reduce nesting depth — static analysis flag at line 80.The inner
try/catchinside the outercatchblock pushes nesting to depth 3, which exceeds the project's configured maximum of 2. Extracting the fallback into a dedicated helper eliminates the violation without changing behaviour.♻️ Proposed refactor — extract fallback helper
+function logFallback(lvl, orig, first, rest, wrapError) { + try { + return orig._logger.log({ + level: lvl, + message: typeof first.message === 'string' ? first.message : String(first), + error: first, + stack: first.stack, + wrapError: wrapError.message + }) + } catch { + return orig(first, ...rest) + } +}Then in
wrapLevel:} catch (wrapError) { // Fallback: log with minimal safe data if wrapping fails - try { - return logger.log({ - level: lvl, - message: first.message || String(first), - error: first, - stack: first.stack, - wrapError: wrapError.message - }) - } catch { - // Last resort: use original logger without wrapping - return orig(first) - } + return logFallback(lvl, orig, first, rest, wrapError) }Note: the helper needs access to
logger.log— pass it as a parameter or close over it via the enclosing scope rather than usingorig._logger.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.js` around lines 78 - 92, The inner try/catch inside the outer catch creates nesting depth >2; extract that fallback logging logic into a new helper (e.g., safeFallbackLog) and call it from the catch in wrapLevel. The helper should accept the logger.log function (or the logger object) plus the variables used (lvl, first, wrapError) and perform the inner try/catch there, returning whatever logger.log returns; update the catch to call safeFallbackLog(logger.log, lvl, first, wrapError) and keep the final fallback of returning orig(first) if the helper throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/logger.js`:
- Around line 83-90: The inner fallback currently uses first.message ||
String(first) and the final catch calls orig(first) dropping ...rest; update the
inner fallback in the catch block that builds the log object to coerce the
message to a string (e.g., use typeof first.message === 'string' ? first.message
: String(first)) so Winston always gets a string, and change the last-resort
return to pass through remaining metadata (call orig(first, ...rest) or
equivalent) so ...rest isn't discarded; reference the variables/functions first,
rest, orig and wrapError in your changes.
---
Nitpick comments:
In `@src/logger.js`:
- Around line 78-92: The inner try/catch inside the outer catch creates nesting
depth >2; extract that fallback logging logic into a new helper (e.g.,
safeFallbackLog) and call it from the catch in wrapLevel. The helper should
accept the logger.log function (or the logger object) plus the variables used
(lvl, first, wrapError) and perform the inner try/catch there, returning
whatever logger.log returns; update the catch to call
safeFallbackLog(logger.log, lvl, first, wrapError) and keep the final fallback
of returning orig(first) if the helper throws.
| message: first.message || String(first), | ||
| error: first, | ||
| stack: first.stack, | ||
| wrapError: wrapError.message | ||
| }) | ||
| } catch { | ||
| // Last resort: use original logger without wrapping | ||
| return orig(first) |
There was a problem hiding this comment.
Two minor robustness gaps in the fallback path.
1. Non-string message in the inner fallback (line 83).
The outer try guards against first.message not being a string (lines 56–64), but the inner fallback drops that guard and uses first.message || String(first). If first.message is a truthy non-string (e.g., an object), it is passed as-is to Winston's message field, which expects a string and may behave unexpectedly.
2. Last-resort call drops ...rest (line 90).
orig(first) silently discards all metadata from ...rest. While this is the "last resort" path, the loss is invisible to the caller. Passing ...rest or at least documenting the omission would be safer.
🛡️ Proposed fixes
- message: first.message || String(first),
+ message: typeof first.message === 'string' ? first.message : String(first),- return orig(first)
+ return orig(first, ...rest)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message: first.message || String(first), | |
| error: first, | |
| stack: first.stack, | |
| wrapError: wrapError.message | |
| }) | |
| } catch { | |
| // Last resort: use original logger without wrapping | |
| return orig(first) | |
| message: typeof first.message === 'string' ? first.message : String(first), | |
| error: first, | |
| stack: first.stack, | |
| wrapError: wrapError.message | |
| }) | |
| } catch { | |
| // Last resort: use original logger without wrapping | |
| return orig(first, ...rest) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/logger.js` around lines 83 - 90, The inner fallback currently uses
first.message || String(first) and the final catch calls orig(first) dropping
...rest; update the inner fallback in the catch block that builds the log object
to coerce the message to a string (e.g., use typeof first.message === 'string' ?
first.message : String(first)) so Winston always gets a string, and change the
last-resort return to pass through remaining metadata (call orig(first, ...rest)
or equivalent) so ...rest isn't discarded; reference the variables/functions
first, rest, orig and wrapError in your changes.
2 participants
Adds a fallback mechanism to ensure errors are logged even if the initial attempt to stringify the error object fails, preventing potential logging failures.
This change enhances the reliability of error logging by providing a more resilient approach to handling error objects, ensuring that even in cases where stringification or object processing fails, a minimal amount of error information is still captured and logged.
Summary by CodeRabbit
Bug Fixes