Skip to content

Commit 9f9e1d7

Browse files
fatmcgavclaude
fatmcgav
and
claude
committed
feat(web): make summary comments idempotent and add created/updated footer
GitHub and GitLab summary posting now checks for an existing comment/note containing a hidden `<!-- sourcebot-review-summary -->` marker before writing. If found, the existing comment is updated in place (`updateComment` / `MergeRequestNotes.edit`); otherwise a new one is created, preventing duplicate summary comments on re-runs. A markdown footer is appended to every summary body showing "Created: <timestamp>" on first post and "Updated: <timestamp>" on subsequent runs, giving reviewers visibility into when the last review was performed. Tests: * mock helpers updated to include `issues.listComments/createComment/ updateComment`` and `MergeRequestNotes.all/edit`. * New test suites added for both GitHub and GitLab handlers covering: * no summary -> no API calls; * first post -> create with marker and "Created:" footer; * re-run -> update with "Updated:" footer and no duplicate create; * API failure -> no throw propagated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 96f375f commit 9f9e1d7

4 files changed

Lines changed: 161 additions & 11 deletions

File tree

‎packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.ts‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,19 @@ const SINGLE_REVIEW: sourcebot_file_diff_review[] = [
3030
},
3131
];
3232

33-
function makeMockOctokit(createReviewCommentResult: 'resolve' | 'reject' = 'resolve') {
33+
function makeMockOctokit(createReviewCommentResult: 'resolve' | 'reject' = 'resolve', existingComments: { id: number; body: string }[] = []) {
3434
return {
3535
rest: {
3636
pulls: {
3737
createReviewComment: createReviewCommentResult === 'resolve'
3838
? vi.fn().mockResolvedValue({})
3939
: vi.fn().mockRejectedValue(new Error('Unprocessable Entity')),
4040
},
41+
issues: {
42+
listComments: vi.fn().mockResolvedValue({ data: existingComments }),
43+
createComment: vi.fn().mockResolvedValue({}),
44+
updateComment: vi.fn().mockResolvedValue({}),
45+
},
4146
},
4247
} as unknown as Octokit;
4348
}
@@ -147,3 +152,62 @@ describe('githubPushPrReviews', () => {
147152
expect(octokit.rest.pulls.createReviewComment).not.toHaveBeenCalled();
148153
});
149154
});
155+
156+
describe('githubPushPrReviews – summary comment', () => {
157+
const SUMMARY_MARKER = '<!-- sourcebot-review-summary -->';
158+
159+
test('does not call issues API when summary is undefined', async () => {
160+
const octokit = makeMockOctokit();
161+
162+
await githubPushPrReviews(octokit, MOCK_PAYLOAD, [], undefined);
163+
164+
expect(octokit.rest.issues.listComments).not.toHaveBeenCalled();
165+
expect(octokit.rest.issues.createComment).not.toHaveBeenCalled();
166+
expect(octokit.rest.issues.updateComment).not.toHaveBeenCalled();
167+
});
168+
169+
test('creates a new comment including the marker when no existing comment found', async () => {
170+
const octokit = makeMockOctokit('resolve', []);
171+
172+
await githubPushPrReviews(octokit, MOCK_PAYLOAD, [], 'Summary text');
173+
174+
expect(octokit.rest.issues.listComments).toHaveBeenCalledWith({
175+
owner: 'my-org',
176+
repo: 'my-repo',
177+
issue_number: 7,
178+
});
179+
expect(octokit.rest.issues.createComment).toHaveBeenCalledOnce();
180+
const body = octokit.rest.issues.createComment.mock.calls[0][0].body as string;
181+
expect(body).toContain(SUMMARY_MARKER);
182+
expect(body).toContain('Summary text');
183+
expect(body).toContain('Created:');
184+
expect(body).not.toContain('Updated:');
185+
expect(octokit.rest.issues.updateComment).not.toHaveBeenCalled();
186+
});
187+
188+
test('updates the existing comment when the marker is already present', async () => {
189+
const existingComments = [{ id: 99, body: `${SUMMARY_MARKER}\nOld summary` }];
190+
const octokit = makeMockOctokit('resolve', existingComments);
191+
192+
await githubPushPrReviews(octokit, MOCK_PAYLOAD, [], 'New summary');
193+
194+
expect(octokit.rest.issues.updateComment).toHaveBeenCalledOnce();
195+
expect(octokit.rest.issues.updateComment).toHaveBeenCalledWith(
196+
expect.objectContaining({ comment_id: 99 }),
197+
);
198+
const body = octokit.rest.issues.updateComment.mock.calls[0][0].body as string;
199+
expect(body).toContain('New summary');
200+
expect(body).toContain('Updated:');
201+
expect(body).not.toContain('Created:');
202+
expect(octokit.rest.issues.createComment).not.toHaveBeenCalled();
203+
});
204+
205+
test('does not throw when listComments fails', async () => {
206+
const octokit = makeMockOctokit();
207+
octokit.rest.issues.listComments = vi.fn().mockRejectedValue(new Error('403 Forbidden'));
208+
209+
await expect(
210+
githubPushPrReviews(octokit, MOCK_PAYLOAD, [], 'Summary text'),
211+
).resolves.not.toThrow();
212+
});
213+
});

‎packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,31 @@ export const githubPushPrReviews = async (octokit: Octokit, pr_payload: sourcebo
88
logger.info("Executing github_push_pr_reviews");
99

1010
if (summary) {
11+
const SUMMARY_MARKER = "<!-- sourcebot-review-summary -->";
1112
try {
12-
await octokit.rest.issues.createComment({
13+
const { data: comments } = await octokit.rest.issues.listComments({
1314
owner: pr_payload.owner,
1415
repo: pr_payload.repo,
1516
issue_number: pr_payload.number,
16-
body: summary,
1717
});
18+
const existing = comments.find(c => c.body?.includes(SUMMARY_MARKER));
19+
const action = existing ? "Updated" : "Created";
20+
const body = `${SUMMARY_MARKER}\n${summary}\n\n---\n*${action}: ${new Date().toUTCString()}*`;
21+
if (existing) {
22+
await octokit.rest.issues.updateComment({
23+
owner: pr_payload.owner,
24+
repo: pr_payload.repo,
25+
comment_id: existing.id,
26+
body,
27+
});
28+
} else {
29+
await octokit.rest.issues.createComment({
30+
owner: pr_payload.owner,
31+
repo: pr_payload.repo,
32+
issue_number: pr_payload.number,
33+
body,
34+
});
35+
}
1836
} catch (error) {
1937
logger.error(`Error posting PR summary comment: ${error}`);
2038
}

‎packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts‎

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,17 @@ const SINGLE_REVIEW: sourcebot_file_diff_review[] = [
3737
},
3838
];
3939

40-
function makeMockClient(discussionResult: 'resolve' | 'reject' = 'resolve') {
40+
function makeMockClient(discussionResult: 'resolve' | 'reject' = 'resolve', existingNotes: { id: number; body: string }[] = []) {
4141
return {
4242
MergeRequestDiscussions: {
4343
create: discussionResult === 'resolve'
4444
? vi.fn().mockResolvedValue({})
4545
: vi.fn().mockRejectedValue(new Error('400 Bad Request')),
4646
},
4747
MergeRequestNotes: {
48+
all: vi.fn().mockResolvedValue(existingNotes),
4849
create: vi.fn().mockResolvedValue({}),
50+
edit: vi.fn().mockResolvedValue({}),
4951
},
5052
} as unknown as GitlabClient;
5153
}
@@ -180,11 +182,63 @@ describe('gitlabPushMrReviews', () => {
180182
test('does not throw when both discussion and note creation fail', async () => {
181183
const client = {
182184
MergeRequestDiscussions: { create: vi.fn().mockRejectedValue(new Error('500')) },
183-
MergeRequestNotes: { create: vi.fn().mockRejectedValue(new Error('500')) },
184-
} as unknown as GitlabClient;
185+
MergeRequestNotes: { all: vi.fn().mockResolvedValue([]), create: vi.fn().mockRejectedValue(new Error('500')), edit: vi.fn() },
186+
} as any;
185187

186188
await expect(
187189
gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, SINGLE_REVIEW),
188190
).resolves.not.toThrow();
189191
});
190192
});
193+
194+
describe('gitlabPushMrReviews – summary note', () => {
195+
const SUMMARY_MARKER = '<!-- sourcebot-review-summary -->';
196+
197+
test('does not call MergeRequestNotes API when summary is undefined', async () => {
198+
const client = makeMockClient();
199+
200+
await gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, [], undefined);
201+
202+
expect(client.MergeRequestNotes.all).not.toHaveBeenCalled();
203+
expect(client.MergeRequestNotes.create).not.toHaveBeenCalled();
204+
expect(client.MergeRequestNotes.edit).not.toHaveBeenCalled();
205+
});
206+
207+
test('creates a new note including the marker when no existing note found', async () => {
208+
const client = makeMockClient('resolve', []);
209+
210+
await gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, [], 'Summary text');
211+
212+
expect(client.MergeRequestNotes.all).toHaveBeenCalledWith(101, 42);
213+
expect(client.MergeRequestNotes.create).toHaveBeenCalledOnce();
214+
const body = client.MergeRequestNotes.create.mock.calls[0][2] as string;
215+
expect(body).toContain(SUMMARY_MARKER);
216+
expect(body).toContain('Summary text');
217+
expect(body).toContain('Created:');
218+
expect(body).not.toContain('Updated:');
219+
expect(client.MergeRequestNotes.edit).not.toHaveBeenCalled();
220+
});
221+
222+
test('updates the existing note when the marker is already present', async () => {
223+
const existingNotes = [{ id: 55, body: `${SUMMARY_MARKER}\nOld summary` }];
224+
const client = makeMockClient('resolve', existingNotes);
225+
226+
await gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, [], 'New summary');
227+
228+
expect(client.MergeRequestNotes.edit).toHaveBeenCalledOnce();
229+
const editOptions = client.MergeRequestNotes.edit.mock.calls[0][3] as { body: string };
230+
expect(editOptions.body).toContain('New summary');
231+
expect(editOptions.body).toContain('Updated:');
232+
expect(editOptions.body).not.toContain('Created:');
233+
expect(client.MergeRequestNotes.create).not.toHaveBeenCalled();
234+
});
235+
236+
test('does not throw when MergeRequestNotes.all fails', async () => {
237+
const client = makeMockClient();
238+
client.MergeRequestNotes.all = vi.fn().mockRejectedValue(new Error('403 Forbidden'));
239+
240+
await expect(
241+
gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, [], 'Summary text'),
242+
).resolves.not.toThrow();
243+
});
244+
});

‎packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,26 @@ export const gitlabPushMrReviews = async (
1414
logger.info("Executing gitlab_push_mr_reviews");
1515

1616
if (summary) {
17+
const SUMMARY_MARKER = "<!-- sourcebot-review-summary -->";
1718
try {
18-
await gitlabClient.MergeRequestNotes.create(
19-
projectId,
20-
prPayload.number,
21-
summary,
22-
);
19+
const notes = await gitlabClient.MergeRequestNotes.all(projectId, prPayload.number);
20+
const existing = notes.find(n => n.body?.includes(SUMMARY_MARKER));
21+
const action = existing ? "Updated" : "Created";
22+
const body = `${SUMMARY_MARKER}\n${summary}\n\n---\n*${action}: ${new Date().toUTCString()}*`;
23+
if (existing) {
24+
await gitlabClient.MergeRequestNotes.edit(
25+
projectId,
26+
prPayload.number,
27+
existing.id,
28+
{ body },
29+
);
30+
} else {
31+
await gitlabClient.MergeRequestNotes.create(
32+
projectId,
33+
prPayload.number,
34+
body,
35+
);
36+
}
2337
} catch (error) {
2438
logger.error(`Error posting MR summary note: ${error}`);
2539
}

0 commit comments

Comments
 (0)