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

[Qiyuanqiii]

Description

Summary

• Allow checkpoint_range to create its first checkpoint and advance existing checkpoints when the only inline posting failures are findings proven to be outside the PR diff.
• Keep those findings visible in the published sticky summary, including the reason they could not be posted inline.
• Preserve comments_failed accounting and continue blocking checkpoint advancement for API failures, uncertain diff locations, incomplete reviews, or an unpublished summary.
• Add regression coverage for the publication gate, multiple batches, mixed failures, carry-forward, and the next run's range resolution.
• Document the exception in the action output description and GitHub Actions guide.

Motivation and Scope

Issue #1521 describes repeated full-branch reviews despite checkpoint_range: true: out-of-diff findings cannot be posted inline, but they keep stats.failed nonzero. The checkpoint writer previously required that count to be zero, even after those findings had been included in the sticky summary. Without an existing checkpoint to carry forward, each later push starts another full-range review.

This change implements the issue's proposed exception for out-of-diff comments. A completed review can record its covered head once the summary containing those findings is published. The next push can then review only the commits after that head.

Implementation

• Mark a failure with outsideDiff: true only in the existing HTTP 422 fallback after classifyCommentAgainstDiff returns outside_diff for a valid location proven absent from the available PR diff.
• Distinguish outside_diff from malformed. Reversed ranges, non-positive or non-integer line numbers, and empty paths remain blocking; validate metadata before treating an absent file or omitted patch as placement evidence. The summary gives malformed locations their own failure reason.
• Accumulate that explicitly classified subset across review batches. Checkpoint advancement requires every failed inline comment to belong to that subset; it does not infer eligibility from an API error's text.
• Leave the existing failure count and summary rendering intact. These findings still failed inline placement, so comments_failed continues to report them.
• Retain the existing requirements for an enabled checkpoint, sticky summary, complete manifest, valid resolved head, and configuration fingerprint.
• Keep the marker inside the final summary and expose checkpoint_after only when that summary publishes successfully.
• Preserve the previous marker when a blocking failure occurs, including runs containing both out-of-diff findings and ordinary API failures.

Compatibility and User Impact

Run outcome	Checkpoint behavior
Complete review; findings posted normally	Advance as before
Complete review; only proven out-of-diff inline failures; summary published	Create or advance the checkpoint
Out-of-diff findings mixed with a real API failure	Do not advance; preserve the previous marker if present
Unresolved location with unavailable diff data	Do not advance
Malformed location metadata, including a reversed line span	Do not advance; preserve the previous marker if present
Incomplete review or unpublished summary	Do not advance
Checkpointing disabled or non-sticky summary	Existing behavior remains unchanged

No action inputs, output names, marker schema, or storage locations change. comments_failed retains its meaning; a nonzero value no longer necessarily means that checkpoint advancement was blocked. The sticky summary still describes the latest reviewed range rather than accumulating findings from every earlier run.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

Validation Environment

• OS: Windows/amd64
• Go: go1.26.5 windows/amd64
• Node.js: v24.14.0
• Base: upstream main at 01cf7ff8b94c5087205eaf47a6e67f94dabb2a32
• Validation repeated in an isolated worktree containing only this issue's four-file patch.

Automated Validation

• node scripts/github-actions/post-review-comments.test.js
• node scripts/github-actions/check-translation-sync.test.js
• make check: license headers, English-only source check, module tidy, formatting, and go vet
• make test: complete Go test suite with the race detector
• git diff --check and LF-only verification for all changed files

Regression Coverage

• Expanded the checkpoint gate table to 40 combinations: five manifest states, four failure classes, and both summary publication outcomes.
• Confirmed that the new out-of-diff case fails before the fix because no checkpoint marker is written.
• Verified first-marker creation and replacement of a carried marker across multiple batches.
• Verified that a mixed real API failure still blocks advancement and preserves the old marker.
• Asserted that out-of-diff findings remain visible in the summary and retain their inline-failure counts.
• Added 12 reversed-range publication scenarios covering first/carried checkpoints, present/absent/binary paths, and mixed proven-outside findings. The new regression was also run against the previous implementation and reproduced the incorrect checkpoint advancement.
• Added direct classifier coverage for non-positive, non-integer, non-numeric, and non-finite line metadata and empty paths.
• Read the written summary through resolveCheckpointRange and verified that the following run starts at the newly recorded head.
• Kept an out-of-diff-looking error message on ordinary API failures to verify that error wording alone cannot grant the exception.

Automated Code Review

Ran the repository-requested ocr review --audience agent --background ... against the review fixes. Two selected files were reviewed with no critical, high, or medium findings. One low-severity comment identified obsolete "tri-state" wording; that comment has been updated to match the explicit classification.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a personal self-review of the generated code and description (pending author review while this PR is a draft)
• I have added tests that prove the fix is effective
• The comment-publisher tests and complete Go unit suite pass locally
• I have updated the documentation accordingly
• All committers have signed the CLA (confirmed by CLA Assistant on this PR)
• I have personally reviewed the disclosed AI output and will answer maintainer questions from my own understanding (pending author confirmation)

AI / LLM Disclosure

This PR was prepared with OpenAI Codex (GPT-6) for investigation, regression tests, The open-code-review (ocr) CLI, configured with DeepSeek deepseek-v4-flash, was used for automated code review. I have checked my code and take responsibility for it.

Known Limitations

• Full npm run test:github-actions validation is not claimed on Windows: the shell contract harness requires /bin/bash, and the plugin contract tests require symbolic-link creation privileges unavailable in this environment. The directly relevant comment-publisher suite and translation-sync tests pass; Linux CI should validate the remaining contracts.
• GitHub publication and the following checkpoint read are exercised through injected API mocks, not by posting test findings to a live pull request.
• A batch rejected for a reason other than a recognized line-resolution 422 can fall back to individual-comment retries. An out-of-diff finding rejected on that path still blocks checkpoint advancement because this patch does not add diff classification there. The exception covers findings already proven out-of-diff by the existing batch fallback.
• Coverage was not remeasured with make coverage; this patch changes JavaScript action behavior and documentation, with no Go implementation changes.

Related Issues

Closes #1521.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s).

[Qiyuanqiii]

@NanaseInori review

[NanaseInori]

I found one checkpoint-safety boundary that should be tightened.

The new exemption is described as applying only to findings that are proven to be outside the PR diff. However, publishBatch currently treats every classifyCommentAgainstDiff(...) == "invalid" result as outsideDiff: true.

invalid is broader than “outside the diff”. In particular, the classifier already returns invalid for a reversed span:

if (startLine > endLine) return "invalid";

That is malformed location metadata, not evidence that the finding lies outside the PR diff. If such a comment triggers the recognized 422 fallback, it is currently counted in outsideDiffCount, so a complete run can satisfy:

stats.failed === outsideDiffCount

and advance the checkpoint even though the only failure was not actually proven out-of-diff. The summary also reports the misleading “outside PR diff hunks” reason for that case.

I think the checkpoint exemption should be tied to a narrower classification than the existing generic invalid result. For example, distinguish outside_diff from malformed, or return an explicit checkpoint-safe reason and set outsideDiff: true only for paths/line ranges proven absent from the complete PR diff inventory.

Please also add a regression where a reversed range receives the matching 422 response and verify that it remains a blocking failure and preserves any existing checkpoint.

Apart from this boundary, the checkpoint flow looks well covered: incomplete diff inventories, changed PR heads, ordinary API failures, mixed failures, multi-batch accumulation, summary publication, carry-forward, and the next range resolution all remain fail-closed as expected.

One documentation nit: the checkpoint_after description says “a finding failed to post” prevents advancement and then immediately introduces an exception. “a blocking publication failure” would describe the new contract more precisely.

[NanaseInori]

Re-reviewed the latest head (dc4c74b).

The checkpoint-safety issue from my previous review is addressed correctly.

classifyCommentAgainstDiff now distinguishes outside_diff from malformed, and only the former contributes to the non-blocking outsideDiffCount. Reversed ranges and other malformed location metadata therefore remain blocking even when the diff otherwise provides enough information to classify the path.

The regression coverage is also strong here. In particular, the new malformed-range scenarios cover first and carried checkpoints, present/missing/binary paths, and mixtures of malformed and genuinely out-of-diff findings, while verifying that the previous checkpoint is preserved and checkpoint_after remains empty.

The checkpoint_after documentation now also uses “blocking publication failure”, which accurately describes the new exception.

I don't see another code-level blocker in the current implementation. The current CI checks are green.

LGTM from my side.