fix: improve error and handler log serialization#826
Merged
Conversation
- Deserialize error event payload in catch block to log repo name, event type, and PR number instead of truncated [Object] - Add repo and PR context to all handler start/complete/error logs - Fix API non-200 response logging that referenced undefined `message` - Downgrade "no config found" from info to debug with repo context - Fix probot.onError to properly serialize error details - Update test fixtures with repository and PR number in mock payloads Signed-off-by: Tomer Figenblat <[email protected]> Co-authored-by: Cursor <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughThe PR standardizes and enriches logging across handlers and core modules by adding context-aware "tag" strings (handler, repository, PR/issue) and extracting event metadata in error handlers. Test fixtures were updated to include repository full_name and PR numbers to support the richer logging context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/pr-lifecycle-labels.js (1)
226-233:⚠️ Potential issue | 🟡 MinorMissing log line for
addLabelsnon-200 response — inconsistent with every other non-200 path in this file.All other API non-200 paths in this handler (
listReviews,getBranchProtection,removeLabel) emitcontext.log.error(...). TheaddLabels.then()silently updates the report without any log, making it the only unlogged API failure path. As per coding guidelines, API errors must always be caught and reported.🛠️ Proposed fix
await context.octokit.rest.issues.addLabels(context.repo({issue_number: context.payload.pull_request.number, labels: [addLabel]})) .then(resp => { if (resp.status !== 200) { + context.log.error(`${tag} got unexpected status ${resp.status}`); report.conclusion = 'failure'; report.output.title = 'Failed to add the label'; report.output.summary = 'This might be an internal time out, please try again'; } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/pr-lifecycle-labels.js` around lines 226 - 233, The addLabels success check currently updates the report on non-200 responses but doesn't log the failure; modify the addLabels response handler (the call to context.octokit.rest.issues.addLabels) to emit a context.log.error with a clear message and include response/status details when resp.status !== 200 (mirror other handlers like listReviews/getBranchProtection/removeLabel), ensuring report.conclusion/report.output remain set as before and the log includes identifying info such as the pull_request number and the returned resp/status/body.
🧹 Nitpick comments (2)
src/handlers/pr-conventional-title.js (1)
27-28: Consider extracting the repeatedtagconstruction into a shared helper.All six handlers share the exact same inline template literal. A tiny utility would eliminate the duplication:
// e.g., src/handlers/handler-utils.js +export function buildTag(handlerName, context) { + return `${handlerName} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`; +}Each handler would then become:
+import { buildTag } from './handler-utils.js'; ... -const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`; +const tag = buildTag(running_handler, context);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/pr-conventional-title.js` around lines 27 - 28, Multiple handlers duplicate the same tag construction using the template literal with running_handler and context.payload details; extract that into a shared helper function (e.g., buildHandlerTag or formatHandlerTag) and replace the inline construction in each handler where tag is defined (references: running_handler, context.payload.repository.full_name, context.payload.pull_request.number, and context.log.info) so each handler calls the helper to produce the tag before logging.src/app-runner.js (1)
38-47: Consider extracting the shared event-context helperThe four lines at 38-41 here are byte-for-byte identical to
src/auto-me-bot.jslines 56-59. A shared utility (e.g.extractEventContext(error)) would keep both error-logging paths consistent and make future changes (like the number fallback fix above) a single-site update.♻️ Suggested helper (e.g. in a new
src/log-utils.js)+// src/log-utils.js +export function extractEventContext(error) { + const evt = error?.event; + return { + repo: evt?.payload?.repository?.full_name, + action: evt ? `${evt.name}.${evt.payload?.action}` : 'unknown', + num: evt?.payload?.number ?? evt?.payload?.pull_request?.number, + }; +}Then in both callers:
- const evt = error.event; - const repo = evt?.payload?.repository?.full_name; - const action = evt ? `${evt.name}.${evt.payload?.action}` : 'unknown'; - const num = evt?.payload?.number; + const { repo, action, num } = extractEventContext(error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app-runner.js` around lines 38 - 47, Extract the duplicated event-context logic (evt, repo, action, num) into a shared helper like extractEventContext(error) placed in a new module (e.g., src/log-utils.js), have it accept the error object and return {evt, repo, action, num} or a formatted string; then replace the four identical lines in src/app-runner.js and the matching block in src/auto-me-bot.js to call extractEventContext(error) and use its return values for logging so both callers share the same implementation and future fixes are centralized.
🤖 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/app-runner.js`:
- Around line 39-41: The variable num is set from evt?.payload?.number which is
undefined for pull_request_review events; update the assignment of num (in
src/app-runner.js where evt, repo and action are defined) to fall back to
evt?.payload?.pull_request?.number when evt?.payload?.number is falsy so
pull_request_review events get the correct PR number; ensure you only change the
num initialization to include this fallback and keep the rest of the surrounding
logic intact.
In `@src/auto-me-bot.js`:
- Around line 56-59: The PR number extraction uses evt?.payload?.number which is
undefined for pull_request_review events; update the logic that computes num
(and any logging that uses it) to fall back to evt.payload.pull_request.number
when payload.number is absent, e.g. set num to evt?.payload?.number ??
evt?.payload?.pull_request?.number so pull request review webhooks log the
correct PR number; ensure this change is applied where num is computed alongside
evt, repo, and action in auto-me-bot.js.
- Around line 60-63: The log call drops err.errors because Pino ignores surplus
args; change the probot.log.error call to pass a single object as the first
argument containing contextual fields and the mapped nested errors (e.g.,
include repo, event/action, num, message and an errors array from (err.errors ??
[]).map(...)) and then the formatted message string as the second arg; update
the call site where probot.log.error is invoked so the error details are merged
into the JSON log line rather than spread as extra arguments.
---
Outside diff comments:
In `@src/handlers/pr-lifecycle-labels.js`:
- Around line 226-233: The addLabels success check currently updates the report
on non-200 responses but doesn't log the failure; modify the addLabels response
handler (the call to context.octokit.rest.issues.addLabels) to emit a
context.log.error with a clear message and include response/status details when
resp.status !== 200 (mirror other handlers like
listReviews/getBranchProtection/removeLabel), ensuring
report.conclusion/report.output remain set as before and the log includes
identifying info such as the pull_request number and the returned
resp/status/body.
---
Nitpick comments:
In `@src/app-runner.js`:
- Around line 38-47: Extract the duplicated event-context logic (evt, repo,
action, num) into a shared helper like extractEventContext(error) placed in a
new module (e.g., src/log-utils.js), have it accept the error object and return
{evt, repo, action, num} or a formatted string; then replace the four identical
lines in src/app-runner.js and the matching block in src/auto-me-bot.js to call
extractEventContext(error) and use its return values for logging so both callers
share the same implementation and future fixes are centralized.
In `@src/handlers/pr-conventional-title.js`:
- Around line 27-28: Multiple handlers duplicate the same tag construction using
the template literal with running_handler and context.payload details; extract
that into a shared helper function (e.g., buildHandlerTag or formatHandlerTag)
and replace the inline construction in each handler where tag is defined
(references: running_handler, context.payload.repository.full_name,
context.payload.pull_request.number, and context.log.info) so each handler calls
the helper to produce the tag before logging.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app-runner.jssrc/auto-me-bot.jssrc/handlers/pr-auto-approve.jssrc/handlers/pr-conventional-commits.jssrc/handlers/pr-conventional-title.jssrc/handlers/pr-lifecycle-labels.jssrc/handlers/pr-signed-commits.jssrc/handlers/pr-tasks-list.jstests/auto-me-bot.test.jstests/handlers/pr-auto-approve.test.jstests/handlers/pr-conventional-commits.test.jstests/handlers/pr-conventional-title.test.jstests/handlers/pr-lifecycle-labels.test.jstests/handlers/pr-signed-commits.test.jstests/handlers/pr-tasks-list.test.js
auto-me-bot
bot
added
status: review started
- payload.number is undefined for pull_request_review events; fall back to payload.pull_request.number in both error handlers. - Pino silently drops spread args after the message string; pass nested errors as a merge object instead. Signed-off-by: Tomer Figenblat <[email protected]> Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/auto-me-bot.js (1)
56-59: Both previously flagged issues are addressed — LGTM.The
pull_request_reviewnumber fallback (?? evt?.payload?.pull_request?.number) resolves the past comment aboutpayload.numberbeingundefinedfor review events, and the extraction pattern is now identical tosrc/app-runner.jslines 37–40.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto-me-bot.js` around lines 56 - 59, The extraction of event metadata (variables evt, repo, action, num) is correct now and mirrors src/app-runner.js's pattern; no code changes are required — keep the current fallback for num as evt?.payload?.number ?? evt?.payload?.pull_request?.number and the action computation using `${evt.name}.${evt.payload?.action}` to handle review events consistently.
🧹 Nitpick comments (1)
src/auto-me-bot.js (1)
60-63: Consider omittingerrorsfield when empty to reduce log noise.
{ errors: [] }is always emitted even when there are no nested errors. Conditionally including it keeps log output cleaner.🔧 Suggested tweak
- { errors: (err.errors ?? []).map(e => `${e.status} ${e.message}`) }, + { ...(err.errors?.length && { errors: err.errors.map(e => `${e.status} ${e.message}`) }) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto-me-bot.js` around lines 60 - 63, The current probot.log.error call always includes an errors field even when empty; modify the code around probot.log.error so you build the log payload first (e.g., create a variable like payload or meta), compute const nested = (err.errors ?? []).map(e => `${e.status} ${e.message}`), and only attach payload.errors = nested when nested.length > 0, then call probot.log.error(payload, `[repo=${repo}, event=${action}, #${num}] ${err.message}`) to avoid emitting { errors: [] } noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/auto-me-bot.js`:
- Around line 56-59: The extraction of event metadata (variables evt, repo,
action, num) is correct now and mirrors src/app-runner.js's pattern; no code
changes are required — keep the current fallback for num as evt?.payload?.number
?? evt?.payload?.pull_request?.number and the action computation using
`${evt.name}.${evt.payload?.action}` to handle review events consistently.
---
Nitpick comments:
In `@src/auto-me-bot.js`:
- Around line 60-63: The current probot.log.error call always includes an errors
field even when empty; modify the code around probot.log.error so you build the
log payload first (e.g., create a variable like payload or meta), compute const
nested = (err.errors ?? []).map(e => `${e.status} ${e.message}`), and only
attach payload.errors = nested when nested.length > 0, then call
probot.log.error(payload, `[repo=${repo}, event=${action}, #${num}]
${err.message}`) to avoid emitting { errors: [] } noise.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app-runner.jssrc/auto-me-bot.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-runner.js
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
2 similar comments
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
auto-me-bot
bot
added
status: merged
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #826 +/- ##
==========================================
+ Coverage 99.21% 99.32% +0.10%
==========================================
Files 8 8
Lines 1022 1038 +16
==========================================
+ Hits 1014 1031 +17
+ Misses 8 7 -1 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
probot.onErrorto log repo name, event type, and PR number instead of truncated[Object][owner/repo#number]context to all handler start, complete, and error log lines across all 6 handlersmessagefield from Octokit responsesTest plan
Summary by CodeRabbit
Bug Fixes
Tests