Skip to content

fix(server): PR watch wakes call an edited comment edited, not new #30959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abhaymettu wants to merge 1 commit into from
+40 −16
Closed

fix(server): PR watch wakes call an edited comment edited, not new #30959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions apps/server/src/orchestration-v2/pullRequestWatch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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, []);
Expand Down
29 changes: 19 additions & 10 deletions apps/server/src/orchestration-v2/pullRequestWatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export type PullRequestWatchChange =
| { readonly kind: "checks-failed"; readonly failed: ReadonlyArray<PullRequestCheck> }
| { readonly kind: "checks-passed"; readonly count: number; readonly required: boolean }
| { readonly kind: "remarks"; readonly remarks: ReadonlyArray<PullRequestComment> }
| { readonly kind: "edits"; readonly remarks: ReadonlyArray<PullRequestComment> }
| { readonly kind: "no-checks"; readonly minutes: number }
| { readonly kind: "conflicting" };

Expand Down Expand Up @@ -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));

@aravhawk aravhawk Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question on using createdAt as the "already existed" signal: for inline review comments, reviewThreadConversation (packages/source-control-github/src/server/gitHubPullRequestJson.ts) passes through the GraphQL createdAt, and the review queries don't read publishedAt. As I understand GitHub, a comment drafted in a pending review keeps its draft time as createdAt and is only published at submit. So if a reviewer starts a pending review before the watermark, edits one of the drafts, and submits after, would that comment show up here as "1 edited comment" even though the watch never reported it? The agent would then be told it's a re-edit of feedback it already handled.

If that's real, it seems narrow (the review's own remark uses submittedAt, so the wake still fires) and could be a follow-up. Flagging it because this line is the first place "created before the watermark" is read as "seen before".

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]!);
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -217,6 +225,7 @@ const SUMMARY: Record<PullRequestWatchChange["kind"], string> = {
"checks-failed": "checks failed",
"checks-passed": "checks passed",
remarks: "new comments",
edits: "edited comments",
"no-checks": "no checks reported",
conflicting: "merge conflict",
};
Expand Down
Loading