Skip to content

Commit cfed452

Browse files
Gavin Williamsclaude
Gavin Williams
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 d80899a commit cfed452

4 files changed

Lines changed: 160 additions & 10 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
@@ -29,14 +29,19 @@ const SINGLE_REVIEW: sourcebot_file_diff_review[] = [
2929
},
3030
];
3131

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

‎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: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,17 @@ const SINGLE_REVIEW: sourcebot_file_diff_review[] = [
3434
},
3535
];
3636

37-
function makeMockClient(discussionResult: 'resolve' | 'reject' = 'resolve') {
37+
function makeMockClient(discussionResult: 'resolve' | 'reject' = 'resolve', existingNotes: { id: number; body: string }[] = []) {
3838
return {
3939
MergeRequestDiscussions: {
4040
create: discussionResult === 'resolve'
4141
? vi.fn().mockResolvedValue({})
4242
: vi.fn().mockRejectedValue(new Error('400 Bad Request')),
4343
},
4444
MergeRequestNotes: {
45+
all: vi.fn().mockResolvedValue(existingNotes),
4546
create: vi.fn().mockResolvedValue({}),
47+
edit: vi.fn().mockResolvedValue({}),
4648
},
4749
} as any;
4850
}
@@ -177,11 +179,63 @@ describe('gitlabPushMrReviews', () => {
177179
test('does not throw when both discussion and note creation fail', async () => {
178180
const client = {
179181
MergeRequestDiscussions: { create: vi.fn().mockRejectedValue(new Error('500')) },
180-
MergeRequestNotes: { create: vi.fn().mockRejectedValue(new Error('500')) },
182+
MergeRequestNotes: { all: vi.fn().mockResolvedValue([]), create: vi.fn().mockRejectedValue(new Error('500')), edit: vi.fn() },
181183
} as any;
182184

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

‎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)