Skip to content

Commit 627d45a

Browse files
TomerFicursoragent
TomerFi
and
cursoragent
authored
fix: improve error and handler log serialization (#826)
* fix: improve error and handler log serialization - 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 <tomer@figenblat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix: handle pull_request_review number fallback and Pino log args - 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 <tomer@figenblat.com> Co-authored-by: Cursor <cursoragent@cursor.com> --------- Signed-off-by: Tomer Figenblat <tomer@figenblat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent ccaa944 commit 627d45a

15 files changed

+80
-54
lines changed

‎src/app-runner.js‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,16 @@ export async function handler (req, res) {
3535

3636
res.status(200).send('ok');
3737
} catch (error) {
38-
console.error('Handler error:', error);
38+
const evt = error.event;
39+
const repo = evt?.payload?.repository?.full_name;
40+
const action = evt ? `${evt.name}.${evt.payload?.action}` : 'unknown';
41+
const num = evt?.payload?.number ?? evt?.payload?.pull_request?.number;
42+
43+
console.error(
44+
`Handler error: [repo=${repo}, event=${action}, #${num}]`,
45+
error.message,
46+
...(error.errors ?? []).map(e => `${e.status} ${e.message}`)
47+
);
3948
res.status(500).send('error');
4049
}
4150
}

‎src/auto-me-bot.js‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,14 @@ const CONFIG_SPEC = Object.freeze({
5353
export default function (probot) {
5454
probot.on(ON_EVENTS, handlersController(CONFIG_SPEC));
5555
probot.onError(async err => {
56-
probot.log.error(err)
56+
const evt = err.event;
57+
const repo = evt?.payload?.repository?.full_name;
58+
const action = evt ? `${evt.name}.${evt.payload?.action}` : 'unknown';
59+
const num = evt?.payload?.number ?? evt?.payload?.pull_request?.number;
60+
probot.log.error(
61+
{ errors: (err.errors ?? []).map(e => `${e.status} ${e.message}`) },
62+
`[repo=${repo}, event=${action}, #${num}] ${err.message}`
63+
);
5764
})
5865
};
5966

@@ -64,7 +71,7 @@ export function handlersController(configSpec) {
6471
context.log.debug(`Payload: ${JSON.stringify(context.payload)}`)
6572
let config = await context.config('auto-me-bot.yml');
6673
if (config == null) {
67-
context.log.info("no config found")
74+
context.log.debug(`no config found for ${context.payload.repository.full_name}`)
6875
return
6976
}
7077
context.log.debug(`Config: ${JSON.stringify(config)}`)

‎src/handlers/pr-auto-approve.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ function match(context) {
1919

2020
// handler for automatic approvals of PRs based on sender login and type
2121
async function run(context, config, startedAt) {
22-
context.log.info(`${running_handler} started`)
22+
const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`;
23+
context.log.info(`${tag} started`)
2324

2425
// create the initial check run and mark it as in_progress
2526
let checkRun = await context.octokit.rest.checks.create(context.repo({
@@ -54,16 +55,15 @@ async function run(context, config, startedAt) {
5455
report.output.title = 'PR approved!';
5556
report.output.summary = `${context.payload.sender.type} was automatically approved`;
5657
} else {
57-
let {status, message} = response;
58-
context.log.error(`${running_handler} got status ${status} with message ${message}`);
58+
context.log.error(`${tag} got unexpected status ${response.status}`);
5959
report.conclusion = 'failure';
6060
report.output.title = 'Failed to approve the PR';
6161
report.output.summary = 'Automatically approval failed';
6262
report.output.text = `Got status ${response.status}.`;
6363
}
6464
})
6565
.catch(error => {
66-
context.log.error(`${running_handler} got error ${error}`);
66+
context.log.error(`${tag} got error ${error.message}`);
6767
report.conclusion = 'failure';
6868
report.output.title = 'Failed to approve the PR';
6969
report.output.summary = 'Automatically approval failed';
@@ -72,7 +72,7 @@ async function run(context, config, startedAt) {
7272
}
7373
}
7474

75-
context.log.debug(`${running_handler} finalizing`);
75+
context.log.debug(`${tag} finalizing`);
7676

7777
// update check run and mark it as completed
7878
await context.octokit.rest.checks.update(context.repo({
@@ -85,5 +85,5 @@ async function run(context, config, startedAt) {
8585
...report
8686
}));
8787

88-
context.log.info(`${running_handler} completed with conclusion ${report.conclusion}`)
88+
context.log.info(`${tag} completed with conclusion ${report.conclusion}`)
8989
}

‎src/handlers/pr-conventional-commits.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ function match(context) {
2424

2525
// handler for verifying commit messages as conventional
2626
async function run(context, config, startedAt) {
27-
context.log.info(`${running_handler} started`)
27+
const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`;
28+
context.log.info(`${tag} started`)
2829

2930
// create the initial check run and mark it as in_progress
3031
let checkRun = await context.octokit.rest.checks.create(context.repo({
@@ -49,11 +50,10 @@ async function run(context, config, startedAt) {
4950
if (response.status === 200) {
5051
commitObjs = response.data;
5152
} else {
52-
let {status, message} = response;
53-
context.log.error(`${running_handler} got status ${status} with message ${message}`);
53+
context.log.error(`${tag} got unexpected status ${response.status}`);
5454
}
5555
})
56-
.catch(error => context.log.error(`${running_handler} got error ${error}`));
56+
.catch(error => context.log.error(`${tag} got error ${error.message}`));
5757
if (commitObjs.length === 0) {
5858
report.conclusion = 'failure'
5959
report.output.title = 'No commits found'
@@ -101,7 +101,7 @@ async function run(context, config, startedAt) {
101101
}
102102
}
103103

104-
context.log.debug(`${running_handler} finalizing`);
104+
context.log.debug(`${tag} finalizing`);
105105

106106
// update check run and mark it as completed
107107
await context.octokit.rest.checks.update(context.repo({
@@ -114,7 +114,7 @@ async function run(context, config, startedAt) {
114114
...report
115115
}));
116116

117-
context.log.info(`${running_handler} completed with conclusion ${report.conclusion}`)
117+
context.log.info(`${tag} completed with conclusion ${report.conclusion}`)
118118
}
119119

120120
// create markdown segments for aggregating the lint status report

‎src/handlers/pr-conventional-title.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ function match(context) {
2424

2525
// handler for verifying pr titles as conventional
2626
async function run(context, config, startedAt) {
27-
context.log.info(`${running_handler} started`)
27+
const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`;
28+
context.log.info(`${tag} started`)
2829

2930
// create the initial check run and mark it as in_progress
3031
let checkRun = await context.octokit.rest.checks.create(context.repo({
@@ -57,7 +58,7 @@ async function run(context, config, startedAt) {
5758
report.output.text = lintReportToMdReport(lintReport);
5859
}
5960

60-
context.log.debug(`${running_handler} finalizing`);
61+
context.log.debug(`${tag} finalizing`);
6162

6263
// update check run and mark it as completed
6364
await context.octokit.rest.checks.update(context.repo({
@@ -70,7 +71,7 @@ async function run(context, config, startedAt) {
7071
...report
7172
}));
7273

73-
context.log.info(`${running_handler} completed with conclusion ${report.conclusion}`)
74+
context.log.info(`${tag} completed with conclusion ${report.conclusion}`)
7475
}
7576

7677
// create markdown report from the lint report

‎src/handlers/pr-lifecycle-labels.js‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ function match(context) {
5050

5151
// handler for labeling pull requests based on lifecycle
5252
async function run(context, config, startedAt) {
53-
context.log.info(`${running_handler} started`)
53+
const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`;
54+
context.log.info(`${tag} started`)
5455

5556
// create the initial check run and mark it as in_progress
5657
let checkRun = await context.octokit.rest.checks.create(context.repo({
@@ -79,10 +80,10 @@ async function run(context, config, startedAt) {
7980
report.output.title = 'Nothing for me to do';
8081
report.output.summary = 'Are you sure your configuration is valid?';
8182
} else {
82-
await workThemLabels(context, config, report);
83+
await workThemLabels(context, config, report, tag);
8384
}
8485

85-
context.log.debug(`${running_handler} finalizing`);
86+
context.log.debug(`${tag} finalizing`);
8687

8788
// update check run and mark it as completed
8889
await context.octokit.rest.checks.update(context.repo({
@@ -95,7 +96,7 @@ async function run(context, config, startedAt) {
9596
...report
9697
}));
9798

98-
context.log.info(`${running_handler} completed with conclusion ${report.conclusion}`)
99+
context.log.info(`${tag} completed with conclusion ${report.conclusion}`)
99100
}
100101

101102

@@ -111,7 +112,7 @@ function getConfiguredLabels(labels) {
111112
return Object.fromEntries(Object.entries(labels).filter(kv => KNOWN_LABELS.includes(kv[0])));
112113
}
113114

114-
async function getLifecycleLabel(context) {
115+
async function getLifecycleLabel(context, tag) {
115116
let action = context.payload.action;
116117
let isMerged = context.payload.pull_request.merged;
117118

@@ -125,11 +126,10 @@ async function getLifecycleLabel(context) {
125126
if (response.status === 200) {
126127
reviews = response.data;
127128
} else {
128-
let {status, message} = response;
129-
context.log.error(`${running_handler} got status ${status} with message ${message}`);
129+
context.log.error(`${tag} got unexpected status ${response.status}`);
130130
}
131131
})
132-
.catch(error => context.log.error(`${running_handler} got error ${error}`));
132+
.catch(error => context.log.error(`${tag} got error ${error.message}`));
133133

134134
if (reviews.length === 0) {
135135
return LABEL_KEYS.REVIEW_REQUIRED;
@@ -173,11 +173,10 @@ async function getLifecycleLabel(context) {
173173
if (response.status === 200){
174174
baseProtections = response.data;
175175
} else {
176-
let {status, message} = response;
177-
context.log.error(`${running_handler} got status ${status} with message ${message}`);
176+
context.log.error(`${tag} got unexpected status ${response.status}`);
178177
}
179178
})
180-
.catch(error => context.log.error(`${running_handler} got error ${error}`));
179+
.catch(error => context.log.error(`${tag} got error ${error.message}`));
181180

182181
let requiredApprovals = baseProtections?.required_pull_request_reviews?.required_approving_review_count || 0;
183182
if (approvals < requiredApprovals) {
@@ -186,9 +185,9 @@ async function getLifecycleLabel(context) {
186185
return LABEL_KEYS.APPROVED;
187186
}
188187

189-
async function workThemLabels(context, config, report) {
188+
async function workThemLabels(context, config, report, tag) {
190189
let configuredLabels = getConfiguredLabels(config.labels);
191-
let lifecycleLabel = await getLifecycleLabel(context);
190+
let lifecycleLabel = await getLifecycleLabel(context, tag);
192191

193192
if (!(lifecycleLabel in configuredLabels)) {
194193
report.output.title = 'Label not configured'
@@ -208,17 +207,16 @@ async function workThemLabels(context, config, report) {
208207
context.octokit.rest.issues.removeLabel(context.repo({issue_number: context.payload.pull_request.number, name: removeLabel}))
209208
.then(response => {
210209
if (response.status !== 200) {
211-
let {status, message} = response;
212-
context.log.error(`${running_handler} got status ${status} with message ${message}`);
210+
context.log.error(`${tag} got unexpected status ${response.status}`);
213211
}
214212
})
215-
.catch(error => context.log.error(`${running_handler} got error ${error}`)));
213+
.catch(error => context.log.error(`${tag} got error ${error.message}`)));
216214

217215
if (!prLabels.includes(addLabel)) {
218216
let labelExist = false;
219217
await context.octokit.rest.issues.getLabel(context.repo({name: addLabel}))
220218
.then(resp => labelExist = resp.status === 200)
221-
.catch(error => context.log.error(`${running_handler} got error ${error}`));
219+
.catch(error => context.log.error(`${tag} got error ${error.message}`));
222220
if (!labelExist) {
223221
report.conclusion = 'failure';
224222
report.output.title = `Label for '${lifecycleLabel}' not found`;
@@ -234,7 +232,7 @@ async function workThemLabels(context, config, report) {
234232
}
235233
})
236234
.catch(error => {
237-
context.log.error(`${running_handler} got error ${error}`);
235+
context.log.error(`${tag} got error ${error.message}`);
238236
report.conclusion = 'failure';
239237
report.output.title = 'Failed to add the label';
240238
report.output.summary = 'This might be a permissions issue';

‎src/handlers/pr-signed-commits.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ function match(context) {
2525

2626
// handler for verifying all commits are sign with the Signed-off-by trailer and a legit email
2727
async function run(context, config, startedAt) {
28-
context.log.info(`${running_handler} started`)
28+
const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`;
29+
context.log.info(`${tag} started`)
2930

3031
// create the initial check run and mark it as in_progress
3132
let checkRun = await context.octokit.rest.checks.create(context.repo({
@@ -50,11 +51,10 @@ async function run(context, config, startedAt) {
5051
if (response.status === 200) {
5152
allCommits = response.data;
5253
} else {
53-
let {status, message} = response;
54-
context.log.error(`${running_handler} got status ${status} with message ${message}`);
54+
context.log.error(`${tag} got unexpected status ${response.status}`);
5555
}
5656
})
57-
.catch(error => context.log.error(`${running_handler} got error ${error}`));
57+
.catch(error => context.log.error(`${tag} got error ${error.message}`));
5858
if (allCommits.length === 0) {
5959
report.conclusion = 'failure'
6060
report.output.title = 'No commits found'
@@ -78,7 +78,7 @@ async function run(context, config, startedAt) {
7878
}
7979
}
8080

81-
context.log.debug(`${running_handler} finalizing`);
81+
context.log.debug(`${tag} finalizing`);
8282

8383
// update check run and mark it as completed
8484
await context.octokit.rest.checks.update(context.repo({
@@ -91,7 +91,7 @@ async function run(context, config, startedAt) {
9191
...report
9292
}));
9393

94-
context.log.info(`${running_handler} completed with conclusion ${report.conclusion}`)
94+
context.log.info(`${tag} completed with conclusion ${report.conclusion}`)
9595
}
9696

9797
/*

‎src/handlers/pr-tasks-list.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ function match(context) {
2222

2323
// handler for verifying PR tasks' list is completed
2424
async function run(context, _config, startedAt) {
25-
context.log.info(`${running_handler} started`)
25+
const tag = `${running_handler} [${context.payload.repository.full_name}#${context.payload.pull_request.number}]`;
26+
context.log.info(`${tag} started`)
2627

2728
// create the initial check run and mark it as in_progress
2829
let checkRun = await context.octokit.rest.checks.create(context.repo({
@@ -63,7 +64,7 @@ async function run(context, _config, startedAt) {
6364
report.output.text = parseTasks(checkedTasks, 'Here\'s a list of your accomplishments');
6465
}
6566

66-
context.log.debug(`${running_handler} finalizing`);
67+
context.log.debug(`${tag} finalizing`);
6768

6869
// update check run and mark it as completed
6970
await context.octokit.rest.checks.update(context.repo({
@@ -76,7 +77,7 @@ async function run(context, _config, startedAt) {
7677
...report
7778
}));
7879

79-
context.log.info(`${running_handler} completed with conclusion ${report.conclusion}`);
80+
context.log.info(`${tag} completed with conclusion ${report.conclusion}`);
8081
}
8182

8283
// recursively extract task items from tokens, including nested lists

‎tests/auto-me-bot.test.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,8 @@ suite('Testing the auto-me-bot export', () => {
9797
fakeContext = {
9898
payload: {
9999
pull_request: {},
100-
action: 'opened'
100+
action: 'opened',
101+
repository: { full_name: 'testOwner/testRepo' }
101102
},
102103
config: configFuncStub,
103104
log: {

‎tests/handlers/pr-auto-approve.test.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,13 @@ suite('Testing the pr-auto-approve handler', () => {
108108
head: {
109109
sha: fakeSha
110110
},
111+
number: fakePRNumber
111112
},
112113
sender: {
113114
login,
114115
type
115-
}
116+
},
117+
repository: { full_name: `${fakeOwner}/${fakeRepository}` }
116118
},
117119
isBot: () => isBot
118120
}

0 commit comments

Comments
 (0)