From 9dcd14875445add4fd528a061ea151b91c8b7bd9 Mon Sep 17 00:00:00 2001 From: Abhay Mettu <113219143+abhaymettu@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:29:41 -0600 Subject: [PATCH] fix(server): PR watch wakes call an edited comment edited, not new A comment that already existed at the watch watermark and was then edited was listed under "N new comments", so a bot that rewrites one status comment looked like fresh feedback on every edit. Edits still wake the watch, as #15415 intended, but now arrive as their own "edited comments" change and count toward the same comment-only wake limit. --- .../orchestration-v2/pullRequestWatch.test.ts | 27 +++++++++++++---- .../src/orchestration-v2/pullRequestWatch.ts | 29 ++++++++++++------- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 6ab0191649490..9098847384891 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -273,15 +273,30 @@ describe("evaluatePullRequestWatch", () => { assert.deepEqual(evaluatePullRequestWatch(watching, detail(), [old]).changes, []); const edited = { ...old, editedAt: "2026-10-02T12:06:00Z" }; const report = evaluatePullRequestWatch(watching, detail(), [edited]); - assert.deepEqual(report.changes, [{ kind: "remarks", remarks: [edited] }]); + assert.deepEqual(report.changes, [{ kind: "edits", remarks: [edited] }]); assert.equal(report.next.remarksThrough, edited.editedAt); assert.deepEqual(report.next.remarkIds, [old.id]); assert.isTrue(report.exhausted); + const { text, notification } = pullRequestWatchMessage({ + number: 1, + url: "https://github.com/o/r/pull/1", + baseBranch: "main", + headSha: null, + report, + }); + assert.include(text, "- 1 edited comment:"); + assert.notInclude(text, "new comment"); + assert.equal(notification.summary, "#1: edited comments, stopped watching"); assert.deepEqual(evaluatePullRequestWatch(report.next, detail(), [edited]).changes, []); const late = { ...edited, id: "late" }; - assert.deepEqual(evaluatePullRequestWatch(report.next, detail(), [edited, late]).changes, [ - { kind: "remarks", remarks: [late] }, - ]); + const added = { ...remark("bot", "2026-10-02T12:07:00Z"), editedAt: "2026-10-02T12:08:00Z" }; + assert.deepEqual( + evaluatePullRequestWatch(report.next, detail(), [edited, late, added]).changes, + [ + { kind: "remarks", remarks: [added] }, + { kind: "edits", remarks: [late] }, + ], + ); }); it("wakes once per edit of a remark seen at the watermark, never for the agent's own", () => { @@ -295,12 +310,12 @@ describe("evaluatePullRequestWatch", () => { assert.deepEqual(evaluatePullRequestWatch(watching, detail(), [seen]).changes, []); const edited = { ...seen, editedAt: "2026-10-10T20:04:50.000Z" }; const first = evaluatePullRequestWatch(watching, detail(), [edited]); - assert.deepEqual(first.changes, [{ kind: "remarks", remarks: [edited] }]); + assert.deepEqual(first.changes, [{ kind: "edits", remarks: [edited] }]); assert.strictEqual(first.next.remarksThrough, edited.editedAt); assert.deepEqual(evaluatePullRequestWatch(first.next, detail(), [edited]).changes, []); const again = { ...seen, editedAt: "2026-10-10T20:05:50.000Z" }; const second = evaluatePullRequestWatch(first.next, detail(), [again]); - assert.deepEqual(second.changes, [{ kind: "remarks", remarks: [again] }]); + assert.deepEqual(second.changes, [{ kind: "edits", remarks: [again] }]); assert.deepEqual(evaluatePullRequestWatch(second.next, detail(), [again]).changes, []); const own = { ...edited, author: { login: "agent-user", name: null, avatarUrl: null } }; assert.deepEqual(evaluatePullRequestWatch(watching, detail(), [own]).changes, []); diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index f4c0000847236..432177fff2a09 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -18,6 +18,7 @@ export type PullRequestWatchChange = | { readonly kind: "checks-failed"; readonly failed: ReadonlyArray } | { readonly kind: "checks-passed"; readonly count: number; readonly required: boolean } | { readonly kind: "remarks"; readonly remarks: ReadonlyArray } + | { readonly kind: "edits"; readonly remarks: ReadonlyArray } | { readonly kind: "no-checks"; readonly minutes: number } | { readonly kind: "conflicting" }; @@ -111,14 +112,18 @@ export function evaluatePullRequestWatch( const through = Date.parse(watch.remarksThrough); const activeAt = (remark: PullRequestComment) => remark.editedAt ?? remark.createdAt; // Hosts can stamp remarks to the second, so remarks at the boundary time are told apart by ID. - const fresh = (remarks ?? []).filter((remark) => { - const at = Date.parse(activeAt(remark)); - return ( - (at > through || (at === through && !watch.remarkIds.includes(remark.id))) && - remark.author?.login.toLowerCase() !== own - ); - }); - if (fresh.length > 0) changes.push({ kind: "remarks", remarks: fresh }); + const past = (at: string, id: string) => { + const time = Date.parse(at); + return time < through || (time === through && watch.remarkIds.includes(id)); + }; + const fresh = (remarks ?? []).filter( + (remark) => !past(activeAt(remark), remark.id) && remark.author?.login.toLowerCase() !== own, + ); + // A remark that already existed at the watermark is news only as an edit, not a new comment. + const edits = fresh.filter((remark) => past(remark.createdAt, remark.id)); + const added = fresh.filter((remark) => !edits.includes(remark)); + if (added.length > 0) changes.push({ kind: "remarks", remarks: added }); + if (edits.length > 0) changes.push({ kind: "edits", remarks: edits }); const latest = Math.max(through, ...fresh.map((remark) => Date.parse(activeAt(remark)))); const atLatest = fresh.filter((remark) => Date.parse(activeAt(remark)) === latest); const remarksThrough = latest === through ? watch.remarksThrough : activeAt(atLatest[0]!); @@ -136,7 +141,9 @@ export function evaluatePullRequestWatch( const conflicting = detail.mergeability === "unknown" ? toldConflict : detail.mergeability === "conflicting"; - const commentsOnly = changes.length > 0 && changes.every((change) => change.kind === "remarks"); + const commentsOnly = + changes.length > 0 && + changes.every((change) => change.kind === "remarks" || change.kind === "edits"); const progress = headMoved || (changes.length > 0 && !commentsOnly); const wakes = (progress ? 0 : watch.wakes) + (commentsOnly ? 1 : 0); return { @@ -195,8 +202,9 @@ function changeLines( `- All ${change.count} ${change.required ? "required " : ""}${change.count === 1 ? "check" : "checks"} passed${context.commit}.`, ]; case "remarks": + case "edits": return [ - `- ${change.remarks.length} new ${change.remarks.length === 1 ? "comment" : "comments"}:`, + `- ${change.remarks.length} ${change.kind === "remarks" ? "new" : "edited"} ${change.remarks.length === 1 ? "comment" : "comments"}:`, ...listed(change.remarks, (remark) => { const where = remark.path === null ? "" : ` on ${remark.path}`; const body = snippet(remark.body); @@ -217,6 +225,7 @@ const SUMMARY: Record = { "checks-failed": "checks failed", "checks-passed": "checks passed", remarks: "new comments", + edits: "edited comments", "no-checks": "no checks reported", conflicting: "merge conflict", };