fix(action): allow checkpoints after out-of-diff findings (#1524)

* fix(action): allow checkpoints after out-of-diff findings

* fix(action): keep malformed locations checkpoint-blocking

---------

Co-authored-by: Qiyuanqiii <267806965+Qiyuanqiii@users.noreply.github.com>
This commit is contained in:
祈愿Qiii
2026-09-23 11:50:04 +08:00
committed by GitHub
co-authored by Qiyuanqiii
parent 6fdfb91af0
commit 5f8e5ab328
4 changed files with 175 additions and 54 deletions
+3 -2
View File
@@ -317,8 +317,9 @@ outputs:
checkpoint_after:
description: >-
The head this run recorded as the new checkpoint, or empty when it did not
advance one (incomplete run, a finding failed to post, or the summary did
not publish).
advance one (incomplete run, a blocking publication failure, or the summary did
not publish). Findings proven outside the PR diff and included in the
published summary do not block advancement.
value: ${{ steps.post.outputs.checkpoint_after }}
runs:
+1 -1
View File
@@ -335,7 +335,7 @@ These outputs report what happened. All of them are empty when `checkpoint_range
Three properties are worth knowing before you enable it:
- **Widen-only.** The start of the range only ever moves back. An older checkpoint produces a wider review, never a narrower one, and a checkpoint only advances past a run whose manifest reported `terminal_state: complete`, whose findings all posted, and whose summary comment actually published. A run that fails halfway carries the previous checkpoint forward unchanged rather than skipping the range it did not review — and a run that cannot read the existing marker leaves it in place rather than erasing it.
- **Widen-only.** The start of the range only ever moves back. An older checkpoint produces a wider review, never a narrower one, and a checkpoint only advances past a run whose manifest reported `terminal_state: complete`, with no blocking publication failures, and whose summary comment actually published. Findings proven to be outside the PR diff are included in that summary and do not block the checkpoint, even on the first run; they still count toward `comments_failed` because they could not be posted inline. API failures and unresolved locations without sufficient diff data still block advancement. A run that fails halfway carries the previous checkpoint forward unchanged rather than skipping the range it did not review — and a run that cannot read the existing marker leaves it in place rather than erasing it.
- **Same-head reruns change nothing.** Re-running the workflow without pushing reports `same_head_noop` and leaves the previous run's summary untouched.
- **The sticky summary shows the latest range, not the whole PR.** The summary comment is rewritten on every run, so findings it reported for an earlier range (findings with no line information, routed findings, warnings) are replaced by the new range's; a run that narrowed the range says so in one line at the end of the summary. Inline review comments are separate comments and stay. If you rely on the summary as a running list for the whole PR, use `full_review: 'true'` to rebuild it, or leave `checkpoint_range` off.
+47 -33
View File
@@ -155,6 +155,7 @@ async function runPostReviewComments({
summaryUrl: "",
checkpointAfter: "",
};
let outsideDiffCount = 0;
// Parsed here, before anything can exit early, even though it is only acted
// on after the posting loop. Unknown values fall to "off", which is the right
@@ -175,9 +176,10 @@ async function runPostReviewComments({
// ---- Checkpoint write path (#476) ----
//
// The checkpoint may only move forward past a run that published everything
// it found: terminal_state "complete" AND nothing failed to post AND a real
// 40-hex resolved head. Anything else re-emits the marker this run started
// with (carry-forward), so an unrelated failure never silently resets the PR
// it found: terminal_state "complete", no blocking posting failures, and a
// real 40-hex resolved head. Proven out-of-diff findings are published in the
// summary and do not block advancement. Other failures re-emit the marker
// this run started with, so an unrelated failure never silently resets the PR
// to full reviews, and never silently skips a range that was not reviewed.
//
// The fourth condition — the summary was actually published — is enforced in
@@ -195,7 +197,7 @@ async function runPostReviewComments({
stats.checkpointAfter = "";
if (!checkpointEnabled || !stickySummary) return null;
if (!manifest || manifest.terminal_state !== "complete") return null;
if (stats.failed !== 0) return null;
if (stats.failed !== outsideDiffCount) return null;
// No fingerprint means the resolve step fell over before it computed one
// (its catch path publishes an empty one). A marker without a fingerprint
// can never validate, so writing one here would only overwrite a usable
@@ -480,6 +482,7 @@ async function runPostReviewComments({
});
successCount += r.succeeded;
failedCount += r.failed;
outsideDiffCount += r.failedComments.filter((fc) => fc.outsideDiff === true).length;
for (const fc of r.failedComments) failedComments.push(fc);
batchCounters.attempted++;
if (r.reconciled) batchCounters.reconciled++;
@@ -699,10 +702,10 @@ async function publishBatch({
// through to the per-comment loop, which has its own retry discipline.
// Re-sending a batch into a spam-throttled endpoint would deepen the
// incident rather than fix it.
// * classifyCommentAgainstDiff() is TRI-state. A comment is only dropped
// when the diff inventory is complete AND proves the line is outside
// it. "unknown" (incomplete file list, file present but patch omitted
// for a binary/oversized diff, LEFT-side comment, no line info) keeps
// * classifyCommentAgainstDiff() distinguishes malformed locations from
// locations proven outside the diff. Only the latter can advance a
// checkpoint. "unknown" (incomplete file list, omitted patch data,
// LEFT-side comment, no line info) keeps
// the pre-existing per-comment behavior instead of silently voiding a
// comment that might well post.
if (batchStatus === 422 && toRetry.length > 0 && isLineResolutionFailure(e)) {
@@ -722,9 +725,10 @@ async function publishBatch({
log(`[422-fallback] Failed to fetch PR diff hunks (${hunkErr.message}); proceeding without diff hunk filter.`);
}
// valid -> provably inside the diff, safe to re-batch
// unknown -> cannot prove either way, fall through to the per-comment loop
// invalid -> provably outside the diff, route to the summary
// valid -> provably inside the diff, safe to re-batch
// unknown -> cannot prove either way, try the per-comment loop
// outside_diff -> route to the summary without blocking the checkpoint
// malformed -> route to the summary and keep the checkpoint blocked
const validItems = [];
const unknownItems = [];
for (const item of toRetry) {
@@ -735,11 +739,16 @@ async function publishBatch({
unknownItems.push(item);
} else {
failed++;
const outsideDiff = verdict === "outside_diff";
const reason = outsideDiff ? "outside PR diff hunks" : "malformed comment location";
failedComments.push({
comment: item.comment,
error: `${describeCommentLocation(item.reviewComment)} could not be resolved (outside PR diff hunks)`,
error: `${describeCommentLocation(item.reviewComment)} could not be resolved (${reason})`,
// Only proven placement limitations qualify; malformed metadata
// remains blocking even though its finding is also in the summary.
outsideDiff,
});
log(`[422-fallback] Comment for ${item.reviewComment.path} (${describeCommentLocation(item.reviewComment)}) is outside PR diff hunks; routing to summary failure.`);
log(`[422-fallback] Comment for ${item.reviewComment.path} (${describeCommentLocation(item.reviewComment)}) could not be resolved (${reason}); routing to summary failure.`);
}
}
if (unknownItems.length > 0) {
@@ -2674,12 +2683,12 @@ function parseDiffHunkRanges(patch) {
return parseDiffHunkInventory(patch).ranges;
}
// TRI-STATE classification: "valid" | "invalid" | "unknown".
// Classification: "valid" | "outside_diff" | "malformed" | "unknown".
//
// "invalid" is a claim we must be able to PROVE, because it permanently routes
// a finding to the summary without ever attempting to post it. Missing or
// partial diff metadata is "unknown", not "invalid" — it means we could not
// check, and the caller keeps the pre-existing per-comment behavior.
// "outside_diff" requires valid location metadata and proof from the diff;
// it routes the finding to the summary without blocking checkpoint advancement.
// "malformed" also routes to the summary but remains a blocking failure.
// Missing or partial diff metadata is "unknown": keep the per-comment behavior.
function classifyCommentAgainstDiff(item, diff) {
// No inventory at all, or one we know is truncated: we cannot prove anything.
if (!diff || !diff.complete) return "unknown";
@@ -2687,14 +2696,6 @@ function classifyCommentAgainstDiff(item, diff) {
const { reviewComment } = item;
const path = reviewComment.path;
// File is not among the PR's changed files at all — provably outside the diff.
if (!diff.known.has(path)) return "invalid";
// File IS in the PR but GitHub omitted its `patch` (binary, or a diff over
// the size limit). We know nothing about its lines.
const ranges = diff.files.get(path);
if (!ranges) return "unknown";
// We only model RIGHT-side (new file) lines. The producer builds RIGHT-side
// comments today; if that ever changes, decline to judge rather than drop.
if (reviewComment.side && reviewComment.side !== "RIGHT") return "unknown";
@@ -2703,12 +2704,25 @@ function classifyCommentAgainstDiff(item, diff) {
if (endLine == null) return "unknown";
const startLine = reviewComment.start_line != null ? reviewComment.start_line : endLine;
// A reversed span is malformed and GitHub will reject it.
if (startLine > endLine) return "invalid";
// Malformed metadata is not proof of an out-of-diff location, even if the
// path is absent or its patch is unavailable. Check it before those cases.
if (
typeof path !== "string" || path.length === 0 ||
!Number.isInteger(startLine) || !Number.isInteger(endLine) ||
startLine < 1 || endLine < 1 || startLine > endLine
) return "malformed";
// File is not among the PR's changed files at all — provably outside the diff.
if (!diff.known.has(path)) return "outside_diff";
// File IS in the PR but GitHub omitted its `patch` (binary, or a diff over
// the size limit). We know nothing about its lines.
const ranges = diff.files.get(path);
if (!ranges) return "unknown";
// Both endpoints must fall inside ONE hunk.
const withinOneHunk = ranges.some((r) => startLine >= r.start && endLine <= r.end);
return withinOneHunk ? "valid" : "invalid";
return withinOneHunk ? "valid" : "outside_diff";
}
// Human-readable location for the failure summary. A multi-line comment reports
@@ -2731,8 +2745,8 @@ function describeCommentLocation(reviewComment) {
// known Set<path> — every path in the PR's file list
// complete boolean — the file list was fully enumerated
//
// `complete` is the guard that makes "invalid" provable: a truncated walk means
// an absent path proves nothing. Pagination goes through readWithPacing so this
// `complete` makes "outside_diff" provable: with a truncated walk, an absent
// path proves nothing. Pagination goes through readWithPacing so this
// read honors the same retry/pacing/quota discipline as every other read in
// this file. GitHub caps listFiles at 3000 files, hence MAX_PAGES = 30.
//
@@ -2786,8 +2800,8 @@ async function getPrDiffHunks({ github, owner, repo, prNumber, commitSha, log, c
// necessarily has changed files, so an empty listFiles response is an anomaly
// (diff not yet materialized server-side, or a malformed/empty response body)
// rather than evidence that every commented path sits outside the diff.
// Trusting it would classify EVERY comment "invalid" and discard the whole
// batch without a single posting attempt — the exact outcome the tri-state
// Trusting it would classify EVERY comment "outside_diff" and discard the
// batch without a single posting attempt — the exact outcome the "unknown"
// classification exists to prevent. Note this is the mirror of the truncation
// case above: too many files and zero files are both "cannot judge".
if (known.size === 0) {
@@ -2441,6 +2441,8 @@ async function main() {
await testCheckpointResolveShape();
// Cross-push checkpoints (#476) — write path
await testCheckpointAdvanceGateTable();
await testCheckpointAdvancesAcrossOutOfDiffBatches();
await testCheckpointMalformedRangesRemainBlocking();
await testCheckpointAdvanceRequiresFullSha();
await testManifestHeadPinsEveryReviewPost();
await testLegacyPullRequestEventUsesSnapshotHead();
@@ -2565,21 +2567,28 @@ function testClassifyCommentAgainstDiff() {
// Single line inside a hunk.
assert.strictEqual(at({ path: "foo.js", line: 11 }), "valid");
// Single line outside every hunk.
assert.strictEqual(at({ path: "foo.js", line: 30 }), "invalid");
assert.strictEqual(at({ path: "foo.js", line: 30 }), "outside_diff");
// File not in the PR at all.
assert.strictEqual(at({ path: "bar.js", line: 10 }), "invalid");
assert.strictEqual(at({ path: "bar.js", line: 10 }), "outside_diff");
// Multi-line span wholly inside ONE hunk.
assert.strictEqual(at({ path: "foo.js", start_line: 10, line: 12 }), "valid");
// Span straddling two hunks: both endpoints exist, but not in the same hunk.
// A flat line-set would wrongly call this valid and 422 all over again.
assert.strictEqual(at({ path: "foo.js", start_line: 11, line: 51 }), "invalid");
assert.strictEqual(at({ path: "foo.js", start_line: 11, line: 51 }), "outside_diff");
// Reversed span.
assert.strictEqual(at({ path: "foo.js", start_line: 52, line: 11 }), "invalid");
for (const path of ["foo.js", "bar.js", "binary.png"]) {
assert.strictEqual(at({ path, start_line: 52, line: 11 }), "malformed");
}
for (const line of [0, -1, 1.5, "11", NaN, Infinity]) {
assert.strictEqual(at({ path: "foo.js", line }), "malformed");
assert.strictEqual(at({ path: "foo.js", start_line: line, line: 11 }), "malformed");
}
assert.strictEqual(at({ path: "", line: 11 }), "malformed");
// Span partially overhanging the end of a hunk.
assert.strictEqual(at({ path: "foo.js", start_line: 11, line: 13 }), "invalid");
assert.strictEqual(at({ path: "foo.js", start_line: 11, line: 13 }), "outside_diff");
// ---- "unknown" must never be reported as "invalid" ----
// ---- "unknown" must never be reported as "outside_diff" ----
// File is in the PR but GitHub omitted its patch (binary / oversized diff).
assert.strictEqual(at({ path: "binary.png", line: 3 }), "unknown");
// No line information to check.
@@ -3870,33 +3879,35 @@ function lastSummaryBody(gh) {
}
// K2/C4: the advance is gated on publication completeness. Only a run that is
// terminal-complete, failed nothing, and published a summary may move the
// checkpoint forward.
// terminal-complete, has no blocking publication failures, and published a
// summary may move the checkpoint forward.
async function testCheckpointAdvanceGateTable() {
const terminals = ["complete", "partial", "failed", "skipped", null];
const failures = [0, 1];
const failures = ["none", "outside_diff", "api", "unknown_diff"];
const published = [true, false];
let advancing = 0;
for (const terminal of terminals) {
for (const failed of failures) {
for (const isPublished of published) {
// failed=1 is produced the way production produces it: a finding whose
// line is provably outside the diff, so the 422 fallback can neither
// repost nor reconcile it.
// Only a proven out-of-diff location is non-blocking. API failures and
// unresolvable locations with unavailable diff data must still block.
const result = {
comments: [
failed === 1
failed !== "none"
? { path: "src/a.js", content: "c1", start_line: 90, end_line: 90 }
: { path: "src/a.js", content: "c1", start_line: 1, end_line: 1 },
],
manifest: terminal === null ? undefined : ckManifest({ terminal_state: terminal }),
};
const gh = makeGithub(
failed === 1
failed !== "none"
? {
headSha: terminal === null ? context.payload.pull_request.head.sha : CK_RESOLVED,
files: [{ filename: "src/a.js", patch: "@@ -1,2 +1,2 @@\n a\n b" }],
batchErrorSpec: [{ message: "Line could not be resolved", status: 422 }],
listFilesThrow: failed === "unknown_diff",
batchErrorSpec: [{ message: "Line could not be resolved", status: failed === "api" ? 403 : 422 }],
individualError: "Line could not be resolved (outside PR diff hunks)",
individualErrorStatus: 422,
}
: { headSha: terminal === null ? context.payload.pull_request.head.sha : CK_RESOLVED }
);
@@ -3922,9 +3933,9 @@ async function testCheckpointAdvanceGateTable() {
const label = `terminal=${terminal} failed=${failed} published=${isPublished}`;
// Pin the fixture itself: the "failed" axis must really have failed a
// finding, otherwise the row proves nothing.
assert.strictEqual(outputs.comments_failed, String(failed), `${label}: fixture failure count`);
assert.strictEqual(outputs.comments_failed, failed === "none" ? "0" : "1", `${label}: fixture failure count`);
assert.strictEqual(outputs.summary_comment_url === "", !isPublished, `${label}: fixture publication`);
const expectAdvance = terminal === "complete" && failed === 0 && isPublished;
const expectAdvance = terminal === "complete" && (failed === "none" || failed === "outside_diff") && isPublished;
assert.strictEqual(advanced, expectAdvance, `${label}: marker written=${advanced}`);
if (expectAdvance) {
advancing++;
@@ -3943,7 +3954,102 @@ async function testCheckpointAdvanceGateTable() {
}
}
}
assert.strictEqual(advancing, 1, "exactly one of the 20 cells may advance");
assert.strictEqual(advancing, 2, "only complete, published runs without blocking failures may advance");
}
// Malformed spans are never checkpoint-safe, even on absent paths or alongside
// genuinely out-of-diff findings. Preserve the previous checkpoint if present.
async function testCheckpointMalformedRangesRemainBlocking() {
for (const carry of ["", CARRY]) {
for (const path of ["src/a.js", "src/missing.js", "assets/logo.png"]) {
for (const mixed of [false, true]) {
const comments = [{ path, content: "Malformed finding", start_line: 2, end_line: 1 }];
if (mixed) {
comments.push({ path: "src/a.js", content: "Outside finding", start_line: 90, end_line: 90 });
}
const gh = makeGithub({
headSha: CK_RESOLVED,
files: [
{ filename: "src/a.js", patch: "@@ -1,2 +1,2 @@\n a\n b" },
{ filename: "assets/logo.png" },
],
batchErrorSpec: [{ message: "Line could not be resolved", status: 422 }],
});
const outputs = {};
await runPostReviewComments({
github: gh,
context,
core: { info() {}, setOutput: (k, v) => { outputs[k] = v; } },
fs: mockFs(JSON.stringify({ comments, manifest: ckManifest() }), ""),
...ckRunOptions({ checkpointCarry: carry }),
});
const body = lastSummaryBody(gh);
assert.strictEqual(outputs.checkpoint_after, "", `${path}: malformed ranges must block advancement`);
assert.deepStrictEqual(parseCheckpointMarker(body), carry ? parseCheckpointMarker(carry) : null);
assert.strictEqual(outputs.comments_failed, String(comments.length));
assert.strictEqual(outputs.comments_inline, "0");
assert.strictEqual(gh.createReviewCalls.length, 1, "malformed ranges must not be retried");
assert.ok(body.includes("Malformed finding"));
assert.ok(body.includes("Lines 2-1 could not be resolved (malformed comment location)"));
assert.ok(!body.includes("Lines 2-1 could not be resolved (outside PR diff hunks)"));
if (mixed) assert.ok(body.includes("Line 90 could not be resolved (outside PR diff hunks)"));
}
}
}
}
// #1521: out-of-diff findings must not prevent either the first checkpoint or
// later advances, even across multiple batches. A real failure still blocks.
async function testCheckpointAdvancesAcrossOutOfDiffBatches() {
for (const carry of ["", CARRY]) {
for (const apiFailure of [false, true]) {
const gh = makeGithub({
headSha: CK_RESOLVED,
files: [{ filename: "src/a.js", patch: "@@ -1,2 +1,2 @@\n a\n b" }],
batchErrorSpec: [
{ message: "Line could not be resolved", status: 422 },
{ message: "Line could not be resolved", status: 422 },
apiFailure ? { message: "Forbidden", status: 403 } : null,
],
individualError: "Forbidden",
individualErrorStatus: 403,
});
const outputs = {};
const comments = [
{ path: "src/a.js", content: "First outside finding", start_line: 90, end_line: 90 },
{ path: "src/a.js", content: "Second outside finding", start_line: 91, end_line: 91 },
{ path: "src/b.js", content: "Other finding", start_line: 1, end_line: 1 },
];
await runPostReviewComments({
github: gh,
context,
core: { info() {}, setOutput: (k, v) => { outputs[k] = v; } },
fs: mockFs(JSON.stringify({ comments, manifest: ckManifest() }), ""),
reviewCommentBatchSize: 1,
...ckRunOptions({ checkpointCarry: carry }),
});
const body = lastSummaryBody(gh);
assert.ok(body.includes("First outside finding"));
assert.ok(body.includes("Second outside finding"));
assert.ok(body.includes("outside PR diff hunks"));
assert.strictEqual(outputs.comments_failed, apiFailure ? "3" : "2");
assert.strictEqual(outputs.comments_inline, apiFailure ? "0" : "1");
assert.strictEqual(outputs.checkpoint_after, apiFailure ? "" : CK_RESOLVED);
const marker = parseCheckpointMarker(body);
if (apiFailure) {
assert.deepStrictEqual(marker, carry ? parseCheckpointMarker(carry) : null);
} else {
assert.strictEqual(marker.head, CK_RESOLVED);
const range = await resolveCheckpointRange(ckArgs({
github: ckGithub({ comments: [ckComment({ body })] }),
isAncestor: async (from, to) => from === CK_RESOLVED && to === CK_NEW ? 0 : 1,
}));
assert.strictEqual(range.mode, "checkpoint");
assert.strictEqual(range.from, CK_RESOLVED);
assert.strictEqual(range.to, CK_NEW);
}
}
}
}
// A manifest whose resolved_head is not a full sha cannot identify a range, so