Skip to content

Commit 24d6273

Browse files
authored
fix(review): clean up superseded review-started placeholder comments (#710)
* fix(review): clean up superseded review-started placeholders * fix(review): drop lease PATCH reuse; sweep placeholders only at publish * fix(review): sweep placeholders from the pre-mutation comment snapshot
1 parent d7e01ce commit 24d6273

3 files changed

Lines changed: 196 additions & 5 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ checkpoint, and status-only commits are intentionally omitted.
110110

111111
### Fixed
112112

113+
- Stopped stale "review started" placeholder comments from accumulating on reviewed items: publishing the durable review comment now sweeps superseded placeholders.
113114
- Stopped narrow OpenClaw automerge repairs from chasing unrelated full-repository lint and typecheck failures.
114115
- Removed the synthetic Codex write preflight that could block repair before Codex saw the real task.
115116
- Kept exact-review handoff health live when the dashboard serves a stale fleet snapshot, so recovered claims no longer leave the operator rail stuck in a delayed or stalled state.

src/clawsweeper.ts

Lines changed: 102 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19863,6 +19863,7 @@ function issueReviewCommentState(
1986319863
number: number,
1986419864
fallbackBodies: readonly string[] = [],
1986519865
): {
19866+
comments: Record<string, unknown>[];
1986619867
reviewComment: Record<string, unknown> | undefined;
1986719868
leaseComment: Record<string, unknown> | undefined;
1986819869
leaseComments: Record<string, unknown>[];
@@ -19885,6 +19886,7 @@ function issueReviewCommentState(
1988519886
: []),
1988619887
];
1988719888
return {
19889+
comments,
1988819890
reviewComment,
1988919891
leaseComment: leaseComments[0],
1989019892
leaseComments,
@@ -20603,6 +20605,11 @@ function postReviewStartStatusComment(options: {
2060320605
);
2060420606
const body = renderReviewStartStatusComment(leaseOptions);
2060520607
const payload = writeCommentPayload(options.item.number, body);
20608+
// Every acquisition POSTs a fresh comment: the lowest-server-id election
20609+
// needs distinct ids per contender, so refreshing a leftover placeholder in
20610+
// place would let two racing workers both validate ownership of the same
20611+
// comment. Superseded placeholders are swept when the durable review
20612+
// comment is published instead.
2060620613
const createArgs = [
2060720614
"api",
2060820615
`repos/${targetRepo()}/issues/${options.item.number}/comments`,
@@ -20730,6 +20737,71 @@ function reapExpiredDedicatedReviewStartLeases(
2073020737
}
2073120738
}
2073220739

20740+
const REVIEW_PLACEHOLDER_BODY_PATTERN = /^ClawSweeper status: review started\./i;
20741+
20742+
export function supersededReviewPlaceholderCommentIds(options: {
20743+
number: number;
20744+
comments: readonly Record<string, unknown>[];
20745+
keepCommentIds: ReadonlySet<number>;
20746+
nowMs?: number;
20747+
}): number[] {
20748+
const nowMs = options.nowMs ?? Date.now();
20749+
const ids: number[] = [];
20750+
for (const comment of options.comments) {
20751+
const id = commentId(comment);
20752+
if (id === null || options.keepCommentIds.has(id)) continue;
20753+
if (!canPatchReviewComment(comment)) continue;
20754+
const body = (commentBody(comment) ?? "").trimStart();
20755+
// Placeholder bodies start with the status line; the durable review
20756+
// comment never does, and its marker is an extra guard against deletion.
20757+
if (!REVIEW_PLACEHOLDER_BODY_PATTERN.test(body)) continue;
20758+
if (body.includes(reviewCommentMarker(options.number))) continue;
20759+
// An unexpired lease may belong to a racing worker on a newer revision;
20760+
// only provably superseded placeholders (expired lease or marker-less
20761+
// legacy body) are swept after the durable review comment is published.
20762+
const marker = body.match(/<!--\s*clawsweeper-review-status:started\b([^>]*)-->/i);
20763+
if (marker) {
20764+
const expiresAtMs = Date.parse(marker[1]?.match(/\blease_expires_at=([^\s>]+)/i)?.[1] ?? "");
20765+
if (Number.isFinite(expiresAtMs) && expiresAtMs >= nowMs) continue;
20766+
}
20767+
ids.push(id);
20768+
}
20769+
return ids;
20770+
}
20771+
20772+
function cleanupSupersededReviewPlaceholderComments(options: {
20773+
number: number;
20774+
// Pre-mutation snapshot from the apply flow; the sweep must not refetch the
20775+
// comment list after the durable-comment mutation (API-budget invariant).
20776+
comments: readonly Record<string, unknown>[];
20777+
keepCommentIds: ReadonlySet<number>;
20778+
}): void {
20779+
const ids = supersededReviewPlaceholderCommentIds({
20780+
number: options.number,
20781+
comments: options.comments,
20782+
keepCommentIds: options.keepCommentIds,
20783+
});
20784+
for (const id of ids) {
20785+
try {
20786+
ghObservedMutationCommand({
20787+
identity: `review_placeholder_sweep:${options.number}:${id}`,
20788+
args: ["api", `repos/${targetRepo()}/issues/comments/${id}`, "--method", "DELETE"],
20789+
});
20790+
console.error(
20791+
`[apply] deleted superseded review placeholder comment ${id} for #${options.number}`,
20792+
);
20793+
} catch (error) {
20794+
if (error instanceof GitHubRuntimeBudgetError) throw error;
20795+
// A failed sweep must never fail the publish; the next apply retries it.
20796+
console.error(
20797+
`[apply] could not delete superseded review placeholder comment ${id} for #${options.number}: ${
20798+
error instanceof Error ? error.message : String(error)
20799+
}`,
20800+
);
20801+
}
20802+
}
20803+
}
20804+
2073320805
function pullRequestHeadSha(number: number): string {
2073420806
const pull = asRecord(ghJson<unknown>(["api", `repos/${targetRepo()}/pulls/${number}`]));
2073520807
const sha = asRecord(pull.head).sha;
@@ -26891,6 +26963,7 @@ function applyDecisionsCommandInner(args: Args, runtimeBudget: GitHubRuntimeBudg
2689126963
if (!headBefore || headBefore !== headAfter || headAfter !== initialReviewHeadSha) {
2689226964
return {
2689326965
comment: refreshed.reviewComment,
26966+
comments: refreshed.comments,
2689426967
leaseComments: refreshed.leaseComments,
2689526968
headSha: headAfter,
2689626969
lease: null,
@@ -26901,18 +26974,22 @@ function applyDecisionsCommandInner(args: Args, runtimeBudget: GitHubRuntimeBudg
2690126974
if (item.kind === "issue" && reportReviewRevision && headAfter !== reportReviewRevision) {
2690226975
return {
2690326976
comment: refreshed.reviewComment,
26977+
comments: refreshed.comments,
2690426978
leaseComments: refreshed.leaseComments,
2690526979
headSha: headAfter,
2690626980
lease: null,
2690726981
preserve: false,
2690826982
blockReason: `live issue source revision ${headAfter} differs from reviewed revision ${reportReviewRevision}`,
2690926983
};
2691026984
}
26911-
return reviewStartLeaseStateForComments(
26912-
refreshed.leaseComments,
26913-
refreshed.reviewComment,
26914-
headAfter,
26915-
);
26985+
return {
26986+
...reviewStartLeaseStateForComments(
26987+
refreshed.leaseComments,
26988+
refreshed.reviewComment,
26989+
headAfter,
26990+
),
26991+
comments: refreshed.comments,
26992+
};
2691626993
} catch (error) {
2691726994
if (error instanceof GitHubRuntimeBudgetError) throw error;
2691826995
const detail = trimMiddle(
@@ -26921,6 +26998,7 @@ function applyDecisionsCommandInner(args: Args, runtimeBudget: GitHubRuntimeBudg
2692126998
);
2692226999
return {
2692327000
comment: undefined,
27001+
comments: [] as Record<string, unknown>[],
2692427002
leaseComments: [],
2692527003
headSha: "",
2692627004
lease: null,
@@ -28902,6 +28980,25 @@ function applyDecisionsCommandInner(args: Args, runtimeBudget: GitHubRuntimeBudg
2890228980
allowedSelfMutationUpdatedAts.add(syncedCommentUpdatedAt);
2890328981
}
2890428982
syncReasons.push("updated durable Codex review comment");
28983+
// The durable review comment is now published, so stale "review
28984+
// started" placeholders from failed earlier attempts are clutter.
28985+
const placeholderKeepCommentIds = new Set<number>();
28986+
const syncedCommentId = commentId(syncedComment);
28987+
if (syncedCommentId !== null) placeholderKeepCommentIds.add(syncedCommentId);
28988+
// Closures assign the active lease, so read it through a cast to
28989+
// defeat TypeScript's stale null narrowing at this use site.
28990+
const heldMutationLease = activeApplyMutationLease as {
28991+
itemNumber: number;
28992+
lease: AcquiredReviewStartLease;
28993+
} | null;
28994+
if (heldMutationLease?.itemNumber === number) {
28995+
placeholderKeepCommentIds.add(heldMutationLease.lease.commentId);
28996+
}
28997+
cleanupSupersededReviewPlaceholderComments({
28998+
number,
28999+
comments: latestLeaseState.comments,
29000+
keepCommentIds: placeholderKeepCommentIds,
29001+
});
2890529002
} catch (error) {
2890629003
const commentAuthError = isGitHubRequiresAuthenticationError(error);
2890729004
if (!commentAuthError && !isLockedConversationCommentError(error)) throw error;

test/review-comment-rendering.test.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
renderReviewStartStatusComment,
1212
reviewAutomationMarkersFromReport,
1313
reviewStartLeaseWinnerCommentIdForTest,
14+
supersededReviewPlaceholderCommentIds,
1415
shouldPreserveReviewStartLease,
1516
withReviewStartStatusLease,
1617
} from "../dist/clawsweeper.js";
@@ -1234,3 +1235,95 @@ Full review comments:
12341235
assert.match(markers, /clawsweeper-verdict:needs-human/);
12351236
assert.doesNotMatch(markers, /clawsweeper-verdict:pass/);
12361237
});
1238+
1239+
test("superseded review placeholder sweep deletes only stale bot placeholder comments", () => {
1240+
const nowMs = Date.parse("2026-07-18T22:13:00.000Z");
1241+
const bot = { login: "openclaw-clawsweeper[bot]" };
1242+
const expiredPlaceholder = renderReviewStartStatusComment({
1243+
number: 110918,
1244+
kind: "pull_request",
1245+
title: "fix webhook limiter",
1246+
headSha: "0123456789abcdef0123456789abcdef01234567",
1247+
startedAt: "2026-07-18T21:41:00.000Z",
1248+
leaseExpiresAt: "2026-07-18T22:11:00.000Z",
1249+
});
1250+
const freshPlaceholder = renderReviewStartStatusComment({
1251+
number: 110918,
1252+
kind: "pull_request",
1253+
title: "fix webhook limiter",
1254+
headSha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
1255+
startedAt: "2026-07-18T22:06:00.000Z",
1256+
leaseExpiresAt: "2026-07-18T22:36:00.000Z",
1257+
});
1258+
const comments = [
1259+
{
1260+
id: 1,
1261+
user: bot,
1262+
body: "Codex review: keep open.\n\n<!-- clawsweeper-review item=110918 -->",
1263+
},
1264+
{ id: 2, user: bot, body: expiredPlaceholder },
1265+
{ id: 3, user: bot, body: freshPlaceholder },
1266+
{
1267+
id: 4,
1268+
user: bot,
1269+
body: "ClawSweeper status: review started.\n\nLegacy placeholder without a lease marker.",
1270+
},
1271+
{ id: 5, user: { login: "steipete" }, body: expiredPlaceholder },
1272+
{ id: 6, user: bot, body: expiredPlaceholder },
1273+
];
1274+
1275+
assert.deepEqual(
1276+
supersededReviewPlaceholderCommentIds({
1277+
number: 110918,
1278+
comments,
1279+
keepCommentIds: new Set([6]),
1280+
nowMs,
1281+
}),
1282+
[2, 4],
1283+
);
1284+
});
1285+
1286+
test("superseded review placeholder sweep never selects the durable review comment", () => {
1287+
const nowMs = Date.parse("2026-07-18T22:13:00.000Z");
1288+
const comments = [
1289+
{
1290+
id: 7,
1291+
user: { login: "clawsweeper[bot]" },
1292+
body: [
1293+
"ClawSweeper status: review started.",
1294+
"",
1295+
"Codex review: keep open.",
1296+
"",
1297+
"<!-- clawsweeper-review item=110918 -->",
1298+
].join("\n"),
1299+
},
1300+
];
1301+
1302+
assert.deepEqual(
1303+
supersededReviewPlaceholderCommentIds({
1304+
number: 110918,
1305+
comments,
1306+
keepCommentIds: new Set(),
1307+
nowMs,
1308+
}),
1309+
[],
1310+
);
1311+
});
1312+
1313+
test("publishing the durable review comment sweeps superseded placeholders", () => {
1314+
const source = readFileSync("src/clawsweeper.ts", "utf8");
1315+
const functionStart = source.indexOf("function postReviewStartStatusComment");
1316+
const postStart = source.slice(
1317+
functionStart,
1318+
source.indexOf("function deleteOwnedDedicatedReviewStartLease", functionStart),
1319+
);
1320+
// Lease acquisition must keep POSTing a fresh comment per contender: the
1321+
// lowest-server-id election needs distinct ids, so no in-place PATCH reuse.
1322+
assert.match(postStart, /issues\/\$\{options\.item\.number\}\/comments/);
1323+
assert.doesNotMatch(postStart, /"PATCH"/);
1324+
1325+
const applyStart = source.indexOf('syncReasons.push("updated durable Codex review comment")');
1326+
assert.ok(applyStart >= 0);
1327+
const applyWindow = source.slice(applyStart, applyStart + 1200);
1328+
assert.match(applyWindow, /cleanupSupersededReviewPlaceholderComments\(\{/);
1329+
});

0 commit comments

Comments
 (0)