feat(action): opt-in completeness-gated checkpoint ranges (#476) - #26
feat(action): opt-in completeness-gated checkpoint ranges (#476)#26chethanuk wants to merge 3 commits into
Conversation
On every PR update the Action re-invokes OCR over the full
merge-base-to-head range, so later pushes on a long-lived PR get
progressively more expensive even when the newest change is small.
Adds an opt-in `checkpoint_range` mode. A run that reviewed everything
it selected records the head it covered in a `<!-- ocr-checkpoint:v1 -->`
marker inside its sticky summary comment; the next run reviews only
<checkpoint>..<head>. Default behaviour is unchanged: the review step
expands ${RANGE_FROM:-$MERGE_BASE}, so with the input off the argv is
byte-identical to before.
The gate is fail-closed and ordered — twelve distinct reasons, every one
of which selects a full review and reports why. The checkpoint advances
only when terminal_state is complete, no inline post failed, the summary
was actually published, and the head is 40-hex; a canceled, failed,
partial or ambiguously published run never advances it, and a run that
cannot advance re-emits the previous marker byte-for-byte rather than
erasing it.
Two properties carry the safety argument. `ocr review --from A --to B`
reviews merge-base(A,B)..B, so a stale older checkpoint can only widen
the next range, never open a coverage gap. And `git merge-base
--is-ancestor` exit 128 (object not in the local store) is treated as
non-ancestor, so an unknown checkpoint head falls back to full rather
than being trusted.
Checkpoint reads are author-verified before the payload is parsed,
because findSummaryIssueComment matches on the marker substring alone
and would otherwise trust a sticky comment posted by a third party.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughThe action adds checkpoint-based incremental review ranges. It validates checkpoint metadata, configuration fingerprints, authorship, and commit ancestry before narrowing OCR input. It persists or carries checkpoint markers through sticky summaries and exposes range and checkpoint outputs. ChangesCheckpoint-based incremental reviews
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubAction
participant resolveCheckpointRange
participant OCR
participant runPostReviewComments
participant StickySummary
GitHubAction->>resolveCheckpointRange: resolve review range and checkpoint state
resolveCheckpointRange->>StickySummary: read and validate checkpoint marker
resolveCheckpointRange-->>GitHubAction: return selected range
GitHubAction->>OCR: review selected range
OCR-->>GitHubAction: return review results
GitHubAction->>runPostReviewComments: publish results and checkpoint metadata
runPostReviewComments->>StickySummary: advance or carry checkpoint marker
runPostReviewComments-->>GitHubAction: return checkpoint_after
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
User descriptionDescriptionIncremental mode reduces duplicate publication, not review scope. On every PR update the Action recomputes merge-base-to-head and invokes OCR over that whole range ( The measured case from alibaba#476: a push that changed 7 files / 323 lines was reviewed as 63 files / 15,700 lines — 48.6× larger by changed line count, 46:31 elapsed, 3.98M tokens. This adds an opt-in Default behaviour is unchanged. The review step expands The gate is fail-closed and ordered. Twelve distinct reasons, every one of which selects a full review and reports which one fired. Absent, corrupt, partial, failed, canceled, incompatible config, non-ancestor, different base, wrong PR, wrong repo — all fall back to full. A wasted full review is cheap; a coverage gap is a correctness bug. The checkpoint advances only on a run that is both complete and published. Two properties carry the safety argument:
Checkpoint reads are author-verified before the payload is parsed, because ScopeIn: issue behaviors 3 (fallback), 4 and 5 (never advance on incomplete/canceled), 8 (machine-readable mode, reason, and exact from/to SHAs), plus the manual Deferred: behavior 6 (same-head rerun as an observable no-op) and the richer compatibility axes in 2 (model identity, rules) beyond the config fingerprint. They are additive on top of this gate and are easier to review separately. No Go changes, and no second manifest format — this consumes the Limitations
Type of Change
How Has This Been Tested?
Verification for the JS suite rests on Checklist
Related Issuescloses alibaba#476 CodeAnt-AI DescriptionReview only new changes between pushes with fail-safe checkpoints What Changed
Impact
💡 Usage GuideChecking Your Pull RequestEvery time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later. Talking to CodeAnt AIGot a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask: This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code. ExamplePreserve Org Learnings with CodeAntYou can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input: This helps CodeAnt AI learn and adapt to your team's coding style and standards. ExampleRetrigger reviewAsk CodeAnt AI to review the PR again, by typing: Check Your Repository HealthTo analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (p.base_ref !== baseRef || p.merge_base !== mergeBase) return full("base_changed", seen); | ||
| // Model/prompt/rules/version changed: earlier findings are not comparable. | ||
| if (p.fingerprint !== fingerprint) return full("config_changed", seen); | ||
|
|
There was a problem hiding this comment.
Suggestion: The checkpoint gate trusts the caller-supplied fingerprint, but the action fingerprints the rule input path rather than the contents of the rule file. Changing a custom rule file, or a repository/project rule file used when rule is empty, therefore leaves the fingerprint unchanged and allows the next run to review only the new commit range under different effective rules. Include the effective rule-content hash in the fingerprint or reject the checkpoint when it cannot be verified. [api mismatch]
Severity Level: Major ⚠️
- ❌ Changed custom rules do not invalidate prior checkpoints.
- ❌ Previously reviewed code can be skipped under new project rules.
- ⚠️ Review results become inconsistent across PR pushes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/github-actions/post-review-comments.js
**Line:** 2344:2344
**Comment:**
*Api Mismatch: The checkpoint gate trusts the caller-supplied `fingerprint`, but the action fingerprints the `rule` input path rather than the contents of the rule file. Changing a custom rule file, or a repository/project rule file used when `rule` is empty, therefore leaves the fingerprint unchanged and allows the next run to review only the new commit range under different effective rules. Include the effective rule-content hash in the fingerprint or reject the checkpoint when it cannot be verified.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixCodeAnt flagged that the checkpoint fingerprint interpolated the `rule` input's path string. rules.NewResolver reads that JSON file off the workspace at review time, so editing it left the fingerprint unchanged and let the next run narrow its range under rules the earlier commits were never reviewed against. Hash the file's contents into the fingerprint. An unreadable path forces a full review (reason `rule_unreadable`) rather than trusting a stored fingerprint that cannot be shown to mean the same rules. The built-in rule set is embedded in the binary and already moves with the OCR version, so only the custom-rule path needed covering.
|
@coderabbitai review |
|
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
scripts/github-actions/post-review-comments.js (2)
2250-2258: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueThe
isBotCommentcall is redundant here.The second clause requires an exact match against
botLogin. That condition already impliesisBotComment(comment, botLogin), becauseisBotCommentreturnstruewhenevercomment.user.login === botLogin. The first clause therefore never changes the result. Keeping both invites a later edit that removes the strict clause and leaves only the loose name-shaped test.♻️ Proposed simplification
- // isBotComment is the coarse "is this ours" test used by the dedup paths; it - // also accepts any login ending in "github-actions[bot]". That is too loose - // here, so the identity must ALSO match exactly: when this run authenticates - // as a GitHub App, a marker left by the default GITHUB_TOKEN is a different - // writer and must not be trusted. - if (!isBotComment(comment, botLogin) || (comment.user && comment.user.login) !== botLogin) { + // Exact identity match only. isBotComment (the dedup paths' coarse test) also + // accepts any login ending in "github-actions[bot]", which is too loose here: + // when this run authenticates as a GitHub App, a marker left by the default + // GITHUB_TOKEN is a different writer and must not be trusted. + if (!comment.user || comment.user.login !== botLogin) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/github-actions/post-review-comments.js` around lines 2250 - 2258, Remove the redundant isBotComment(comment, botLogin) check from the author validation near the summary-comment checkpoint, and rely solely on the exact comment.user.login === botLogin comparison while preserving the existing author_unverified return behavior.
2160-2170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the marker prefix from
CHECKPOINT_VERSION.
CHECKPOINT_VERSIONis1, butv1is hardcoded twice more: inCHECKPOINT_MARKER_PATTERN(line 2164) and in the template at line 2169. A future bump ofCHECKPOINT_VERSIONalone emits av1marker whose payload saysv: 2. Readers parse it, then reject it withunsupported version, which looks like corruption rather than a version change. Tie the three together so one edit is enough.Note: the reader must keep matching older prefixes to recognize a stale marker, so a version bump also needs the pattern to accept
v\d+rather than only the current version.♻️ Proposed refactor
const CHECKPOINT_VERSION = 1; +const CHECKPOINT_MARKER_PREFIX = `ocr-checkpoint:v${CHECKPOINT_VERSION}`; // Marker shape: an HTML comment (invisible in the rendered summary) carrying a // base64 JSON payload, so payload text can contain "-->" or newlines without // breaking out of the comment. -const CHECKPOINT_MARKER_PATTERN = "<!-- ocr-checkpoint:v1 ([A-Za-z0-9+/]+={0,2}) -->"; +const CHECKPOINT_MARKER_PATTERN = `<!-- ${CHECKPOINT_MARKER_PREFIX} ([A-Za-z0-9+/]+={0,2}) -->`; const CHECKPOINT_SHA_RE = /^[0-9a-f]{40}$/; function buildCheckpointMarker(payload) { const encoded = Buffer.from(JSON.stringify(payload), "utf8").toString("base64"); - return `<!-- ocr-checkpoint:v1 ${encoded} -->`; + return `<!-- ${CHECKPOINT_MARKER_PREFIX} ${encoded} -->`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/github-actions/post-review-comments.js` around lines 2160 - 2170, Update CHECKPOINT_MARKER_PATTERN and buildCheckpointMarker so the marker prefix is derived from CHECKPOINT_VERSION instead of hardcoding v1, while retaining a reader pattern that accepts v followed by any numeric version (v\d+) to recognize stale markers. Ensure a version bump keeps emitted markers and payload versions synchronized.examples/github_actions/README.md (1)
204-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the three consecutive blockquotes.
markdownlint reports MD028 at lines 205 and 207. A blank line between two blockquotes is ambiguous, and some renderers join them into one quote. Insert a non-quote separator, or merge the three notes into one blockquote with
>on the blank lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/github_actions/README.md` around lines 204 - 208, Separate the three consecutive blockquote notes in the README to satisfy MD028: either add a non-quote separator between the blocks or merge them into one continuous blockquote with `>` on blank lines. Preserve all existing caveat content and headings.Source: Linters/SAST tools
scripts/github-actions/post-review-comments.test.js (1)
3735-3754: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the "advance supersedes carry" invariant on the with-findings path too.
testCheckpointAdvancesOnZeroFindingsasserts that an advancing run does not also re-emitCARRY, but only through the zero-findings early return at line 214. The main body path (line 418) is a separate call site ofappendCheckpoint. If a body ever carried two markers,parseCheckpointMarkerreturnsnullfor "two markers in one body", and the next run silently falls back tocorrupt_checkpointforever.Add one case: findings present,
terminal_state: complete, nothing failed, andcheckpointCarry: CARRY. Assert the body contains exactly oneocr-checkpointmarker and thatparseCheckpointMarker(body).head === CK_RESOLVED.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/github-actions/post-review-comments.test.js` around lines 3735 - 3754, Extend the checkpoint advancement tests with a findings-present case using terminal_state complete, no failures, and checkpointCarry set to CARRY. In the new test, verify the summary body contains exactly one ocr-checkpoint marker and that parseCheckpointMarker(body).head equals CK_RESOLVED, covering the main appendCheckpoint path alongside testCheckpointAdvancesOnZeroFindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@action.yml`:
- Around line 292-296: Update the VERSION_ACTUAL assignment in the OCR version
resolution block to handle an empty result explicitly: fall back to the
configured OCR version spec, or otherwise force checkpoint invalidation/full
review. Preserve the guarantee that a successfully changed resolved OCR version
changes the fingerprint, while avoiding an empty value being written to
GITHUB_ENV.
- Around line 324-413: Wrap the checkpoint-range setup and output logic in the
action script’s top-level try/catch, including helper loading,
readCheckpointComment, resolveCheckpointRange, and export/output calls. On any
unexpected error, log a warning and force the full-review fallback by exporting
an empty RANGE_FROM and setting checkpoint outputs consistently, so the
subsequent ocr review still runs.
In `@examples/github_actions/README.md`:
- Around line 198-202: Update the README text introducing the checkpoint
behavior to say “Three properties” instead of “Two properties,” and revise the
phrase “which failed to post nothing” in the Widen-only bullet to clearly state
that the run posted no findings. Preserve the surrounding behavior descriptions
and bullet structure.
---
Nitpick comments:
In `@examples/github_actions/README.md`:
- Around line 204-208: Separate the three consecutive blockquote notes in the
README to satisfy MD028: either add a non-quote separator between the blocks or
merge them into one continuous blockquote with `>` on blank lines. Preserve all
existing caveat content and headings.
In `@scripts/github-actions/post-review-comments.js`:
- Around line 2250-2258: Remove the redundant isBotComment(comment, botLogin)
check from the author validation near the summary-comment checkpoint, and rely
solely on the exact comment.user.login === botLogin comparison while preserving
the existing author_unverified return behavior.
- Around line 2160-2170: Update CHECKPOINT_MARKER_PATTERN and
buildCheckpointMarker so the marker prefix is derived from CHECKPOINT_VERSION
instead of hardcoding v1, while retaining a reader pattern that accepts v
followed by any numeric version (v\d+) to recognize stale markers. Ensure a
version bump keeps emitted markers and payload versions synchronized.
In `@scripts/github-actions/post-review-comments.test.js`:
- Around line 3735-3754: Extend the checkpoint advancement tests with a
findings-present case using terminal_state complete, no failures, and
checkpointCarry set to CARRY. In the new test, verify the summary body contains
exactly one ocr-checkpoint marker and that parseCheckpointMarker(body).head
equals CK_RESOLVED, covering the main appendCheckpoint path alongside
testCheckpointAdvancesOnZeroFindings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33c2b01d-c387-4eda-8875-317df18fd1ac
📒 Files selected for processing (4)
action.ymlexamples/github_actions/README.mdscripts/github-actions/post-review-comments.jsscripts/github-actions/post-review-comments.test.js
| # Resolved version (not the spec, which is usually "latest"). It feeds | ||
| # the checkpoint fingerprint so an OCR upgrade invalidates checkpoints | ||
| # taken by the previous version. | ||
| VERSION_ACTUAL=$(ocr version 2>/dev/null | head -n 1 | tr -d '\r' || true) | ||
| echo "OCR_VERSION_ACTUAL=${VERSION_ACTUAL}" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Handle an empty resolved OCR version.
If ocr version writes to stderr only, or prints nothing, VERSION_ACTUAL becomes empty. The fingerprint then folds in '' for every run, so an OCR upgrade no longer invalidates a checkpoint. The documented guarantee "the resolved OCR version changed → config_changed" silently stops holding. Consider falling back to the version spec, or forcing a full review when the resolved version is empty.
🛡️ Proposed fallback
- VERSION_ACTUAL=$(ocr version 2>/dev/null | head -n 1 | tr -d '\r' || true)
+ VERSION_ACTUAL=$(ocr version 2>/dev/null | head -n 1 | tr -d '\r' || true)
+ # Never fingerprint an empty version: it would make every OCR upgrade
+ # look identical. Fall back to the requested spec.
+ VERSION_ACTUAL="${VERSION_ACTUAL:-spec:${OCR_VERSION}}"
echo "OCR_VERSION_ACTUAL=${VERSION_ACTUAL}" >> "$GITHUB_ENV"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Resolved version (not the spec, which is usually "latest"). It feeds | |
| # the checkpoint fingerprint so an OCR upgrade invalidates checkpoints | |
| # taken by the previous version. | |
| VERSION_ACTUAL=$(ocr version 2>/dev/null | head -n 1 | tr -d '\r' || true) | |
| echo "OCR_VERSION_ACTUAL=${VERSION_ACTUAL}" >> "$GITHUB_ENV" | |
| # Resolved version (not the spec, which is usually "latest"). It feeds | |
| # the checkpoint fingerprint so an OCR upgrade invalidates checkpoints | |
| # taken by the previous version. | |
| VERSION_ACTUAL=$(ocr version 2>/dev/null | head -n 1 | tr -d '\r' || true) | |
| # Never fingerprint an empty version: it would make every OCR upgrade | |
| # look identical. Fall back to the requested spec. | |
| VERSION_ACTUAL="${VERSION_ACTUAL:-spec:${OCR_VERSION}}" | |
| echo "OCR_VERSION_ACTUAL=${VERSION_ACTUAL}" >> "$GITHUB_ENV" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 292 - 296, Update the VERSION_ACTUAL assignment in
the OCR version resolution block to handle an empty result explicitly: fall back
to the configured OCR version spec, or otherwise force checkpoint
invalidation/full review. Preserve the guarantee that a successfully changed
resolved OCR version changes the fingerprint, while avoiding an empty value
being written to GITHUB_ENV.
| script: | | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const crypto = require('crypto'); | ||
| const { spawnSync } = require('child_process'); | ||
| // Same helper lookup as the posting step below. | ||
| const REL = 'scripts/github-actions/post-review-comments.js'; | ||
| const roots = [process.env.GITHUB_ACTION_PATH, process.env.GITHUB_WORKSPACE].filter(Boolean); | ||
| const helper = roots.map(r => path.resolve(r, REL)).find(p => fs.existsSync(p)); | ||
| if (!helper) throw new Error(`Could not locate ${REL}; searched roots: ${roots.join(', ')}`); | ||
| const { resolveCheckpointRange, readCheckpointComment } = require(helper); | ||
|
|
||
| // `rule` names a JSON file that OCR reads off the workspace at review | ||
| // time (rules.NewResolver only touches disk when the path is non-empty; | ||
| // the default rule set is embedded in the binary and so already moves | ||
| // with OCR_VERSION_ACTUAL). Fingerprinting the *path* alone would let an | ||
| // edit to that file narrow the next range under rules the earlier | ||
| // commits were never reviewed against, so hash the contents too. | ||
| let ruleDigest = 'none'; | ||
| let ruleUnverified = false; | ||
| const rulePath = process.env.OCR_RULE_PATH || ''; | ||
| if (rulePath) { | ||
| try { | ||
| const abs = path.resolve(process.env.GITHUB_WORKSPACE || '.', rulePath); | ||
| ruleDigest = crypto.createHash('sha256').update(fs.readFileSync(abs)).digest('hex'); | ||
| } catch (e) { | ||
| // Cannot prove the rules are unchanged -> do not narrow. OCR itself | ||
| // would normally have failed on an unreadable rule file before this | ||
| // step runs, so this is a belt-and-braces path. | ||
| ruleUnverified = true; | ||
| core.warning(`checkpoint: cannot read rule file ${rulePath} (${e.message}); forcing a full review.`); | ||
| } | ||
| } | ||
|
|
||
| const fingerprint = crypto.createHash('sha256') | ||
| .update(`${process.env.OCR_FINGERPRINT_INPUTS || ''}|${process.env.OCR_VERSION_ACTUAL || ''}|${ruleDigest}`) | ||
| .digest('hex') | ||
| .slice(0, 16); | ||
|
|
||
| // git's own ancestry verdict: 0 = ancestor, 1 = not, 128 = the object | ||
| // is not in this clone (shallow fetch, force-push, head_sha override). | ||
| // Anything else (git missing, signal) is a resolver error. Never | ||
| // treated as "ancestor" except on a literal 0. | ||
| const isAncestor = (a, b) => | ||
| spawnSync('git', ['merge-base', '--is-ancestor', a, b], { cwd: process.env.GITHUB_WORKSPACE }).status; | ||
|
|
||
| const common = { | ||
| github, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| prNumber: context.issue.number, | ||
| log: (m) => core.info(m), | ||
| }; | ||
| // One read serves both purposes: the range decision, and the verbatim | ||
| // marker the posting step re-emits on a run that does not advance the | ||
| // checkpoint (the summary body is rewritten wholesale, which would | ||
| // otherwise erase it). Passing it into the resolver as `read` is what | ||
| // keeps this to a single listComments pagination per run. | ||
| const existing = await readCheckpointComment(common); | ||
| const range = await resolveCheckpointRange(Object.assign({}, common, { | ||
| read: existing, | ||
| enabled: true, | ||
| sticky: process.env.OCR_STICKY_SUMMARY === 'true', | ||
| fullReview: process.env.OCR_FULL_REVIEW === 'true', | ||
| headSha: process.env.HEAD_SHA || '', | ||
| baseRef: process.env.BASE_REF || '', | ||
| mergeBase: process.env.MERGE_BASE || '', | ||
| fingerprint, | ||
| isAncestor, | ||
| })); | ||
|
|
||
| // Last gate, applied after the ordered twelve: the rules this run will | ||
| // apply could not be read, so no stored fingerprint can be trusted to | ||
| // mean "same rules". Widening is always safe; narrowing is not. | ||
| if (ruleUnverified && range.mode === 'checkpoint') { | ||
| range.mode = 'full'; | ||
| range.reason = 'rule_unreadable'; | ||
| } | ||
|
|
||
| // Empty RANGE_FROM means "review the full range": the review step | ||
| // expands ${RANGE_FROM:-$MERGE_BASE}, so unset and empty behave alike. | ||
| core.exportVariable('RANGE_FROM', range.mode === 'checkpoint' ? range.from : ''); | ||
| core.exportVariable('OCR_CONFIG_FINGERPRINT', fingerprint); | ||
| core.exportVariable('OCR_CHECKPOINT_CARRY', existing.raw || ''); | ||
| const summary = range.mode === 'checkpoint' | ||
| ? `checkpoint (${range.reason}): ${range.from}..${range.to}` | ||
| : `full (${range.reason})`; | ||
| core.setOutput('range_mode', range.mode); | ||
| core.setOutput('range_summary', summary); | ||
| core.info(`[checkpoint] reviewing ${summary}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap the resolver in a try/catch so an unexpected error still reviews the full range.
The script has no top-level error handling. resolveCheckpointRange catches its own known failures, but any other throw here fails the whole step, and therefore the job, before ocr review runs. Examples: require(helper) fails, context.issue.number is undefined for a trigger without an issue payload, or core.exportVariable receives an unexpected value. Today, without checkpoint_range, none of these paths exist and the review always runs.
The documented contract is "when in doubt, review the full range". Catching here makes the step match that contract.
🛡️ Proposed fix
- const existing = await readCheckpointComment(common);
- const range = await resolveCheckpointRange(Object.assign({}, common, {
- read: existing,
- enabled: true,
- sticky: process.env.OCR_STICKY_SUMMARY === 'true',
- fullReview: process.env.OCR_FULL_REVIEW === 'true',
- headSha: process.env.HEAD_SHA || '',
- baseRef: process.env.BASE_REF || '',
- mergeBase: process.env.MERGE_BASE || '',
- fingerprint,
- isAncestor,
- }));
+ let existing = { reason: 'resolver_error', payload: null, raw: '' };
+ let range = { mode: 'full', reason: 'resolver_error', from: '', to: process.env.HEAD_SHA || '' };
+ try {
+ existing = await readCheckpointComment(common);
+ range = await resolveCheckpointRange(Object.assign({}, common, {
+ read: existing,
+ enabled: true,
+ sticky: process.env.OCR_STICKY_SUMMARY === 'true',
+ fullReview: process.env.OCR_FULL_REVIEW === 'true',
+ headSha: process.env.HEAD_SHA || '',
+ baseRef: process.env.BASE_REF || '',
+ mergeBase: process.env.MERGE_BASE || '',
+ fingerprint,
+ isAncestor,
+ }));
+ } catch (e) {
+ // Widening is always safe; a resolver crash must not stop the review.
+ core.warning(`checkpoint: range resolution failed (${e.message}); reviewing the full range.`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| script: | | |
| const fs = require('fs'); | |
| const path = require('path'); | |
| const crypto = require('crypto'); | |
| const { spawnSync } = require('child_process'); | |
| // Same helper lookup as the posting step below. | |
| const REL = 'scripts/github-actions/post-review-comments.js'; | |
| const roots = [process.env.GITHUB_ACTION_PATH, process.env.GITHUB_WORKSPACE].filter(Boolean); | |
| const helper = roots.map(r => path.resolve(r, REL)).find(p => fs.existsSync(p)); | |
| if (!helper) throw new Error(`Could not locate ${REL}; searched roots: ${roots.join(', ')}`); | |
| const { resolveCheckpointRange, readCheckpointComment } = require(helper); | |
| // `rule` names a JSON file that OCR reads off the workspace at review | |
| // time (rules.NewResolver only touches disk when the path is non-empty; | |
| // the default rule set is embedded in the binary and so already moves | |
| // with OCR_VERSION_ACTUAL). Fingerprinting the *path* alone would let an | |
| // edit to that file narrow the next range under rules the earlier | |
| // commits were never reviewed against, so hash the contents too. | |
| let ruleDigest = 'none'; | |
| let ruleUnverified = false; | |
| const rulePath = process.env.OCR_RULE_PATH || ''; | |
| if (rulePath) { | |
| try { | |
| const abs = path.resolve(process.env.GITHUB_WORKSPACE || '.', rulePath); | |
| ruleDigest = crypto.createHash('sha256').update(fs.readFileSync(abs)).digest('hex'); | |
| } catch (e) { | |
| // Cannot prove the rules are unchanged -> do not narrow. OCR itself | |
| // would normally have failed on an unreadable rule file before this | |
| // step runs, so this is a belt-and-braces path. | |
| ruleUnverified = true; | |
| core.warning(`checkpoint: cannot read rule file ${rulePath} (${e.message}); forcing a full review.`); | |
| } | |
| } | |
| const fingerprint = crypto.createHash('sha256') | |
| .update(`${process.env.OCR_FINGERPRINT_INPUTS || ''}|${process.env.OCR_VERSION_ACTUAL || ''}|${ruleDigest}`) | |
| .digest('hex') | |
| .slice(0, 16); | |
| // git's own ancestry verdict: 0 = ancestor, 1 = not, 128 = the object | |
| // is not in this clone (shallow fetch, force-push, head_sha override). | |
| // Anything else (git missing, signal) is a resolver error. Never | |
| // treated as "ancestor" except on a literal 0. | |
| const isAncestor = (a, b) => | |
| spawnSync('git', ['merge-base', '--is-ancestor', a, b], { cwd: process.env.GITHUB_WORKSPACE }).status; | |
| const common = { | |
| github, | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| prNumber: context.issue.number, | |
| log: (m) => core.info(m), | |
| }; | |
| // One read serves both purposes: the range decision, and the verbatim | |
| // marker the posting step re-emits on a run that does not advance the | |
| // checkpoint (the summary body is rewritten wholesale, which would | |
| // otherwise erase it). Passing it into the resolver as `read` is what | |
| // keeps this to a single listComments pagination per run. | |
| const existing = await readCheckpointComment(common); | |
| const range = await resolveCheckpointRange(Object.assign({}, common, { | |
| read: existing, | |
| enabled: true, | |
| sticky: process.env.OCR_STICKY_SUMMARY === 'true', | |
| fullReview: process.env.OCR_FULL_REVIEW === 'true', | |
| headSha: process.env.HEAD_SHA || '', | |
| baseRef: process.env.BASE_REF || '', | |
| mergeBase: process.env.MERGE_BASE || '', | |
| fingerprint, | |
| isAncestor, | |
| })); | |
| // Last gate, applied after the ordered twelve: the rules this run will | |
| // apply could not be read, so no stored fingerprint can be trusted to | |
| // mean "same rules". Widening is always safe; narrowing is not. | |
| if (ruleUnverified && range.mode === 'checkpoint') { | |
| range.mode = 'full'; | |
| range.reason = 'rule_unreadable'; | |
| } | |
| // Empty RANGE_FROM means "review the full range": the review step | |
| // expands ${RANGE_FROM:-$MERGE_BASE}, so unset and empty behave alike. | |
| core.exportVariable('RANGE_FROM', range.mode === 'checkpoint' ? range.from : ''); | |
| core.exportVariable('OCR_CONFIG_FINGERPRINT', fingerprint); | |
| core.exportVariable('OCR_CHECKPOINT_CARRY', existing.raw || ''); | |
| const summary = range.mode === 'checkpoint' | |
| ? `checkpoint (${range.reason}): ${range.from}..${range.to}` | |
| : `full (${range.reason})`; | |
| core.setOutput('range_mode', range.mode); | |
| core.setOutput('range_summary', summary); | |
| core.info(`[checkpoint] reviewing ${summary}`); | |
| script: | | |
| const fs = require('fs'); | |
| const path = require('path'); | |
| const crypto = require('crypto'); | |
| const { spawnSync } = require('child_process'); | |
| // Same helper lookup as the posting step below. | |
| const REL = 'scripts/github-actions/post-review-comments.js'; | |
| const roots = [process.env.GITHUB_ACTION_PATH, process.env.GITHUB_WORKSPACE].filter(Boolean); | |
| const helper = roots.map(r => path.resolve(r, REL)).find(p => fs.existsSync(p)); | |
| if (!helper) throw new Error(`Could not locate ${REL}; searched roots: ${roots.join(', ')}`); | |
| const { resolveCheckpointRange, readCheckpointComment } = require(helper); | |
| // `rule` names a JSON file that OCR reads off the workspace at review | |
| // time (rules.NewResolver only touches disk when the path is non-empty; | |
| // the default rule set is embedded in the binary and so already moves | |
| // with OCR_VERSION_ACTUAL). Fingerprinting the *path* alone would let an | |
| // edit to that file narrow the next range under rules the earlier | |
| // commits were never reviewed against, so hash the contents too. | |
| let ruleDigest = 'none'; | |
| let ruleUnverified = false; | |
| const rulePath = process.env.OCR_RULE_PATH || ''; | |
| if (rulePath) { | |
| try { | |
| const abs = path.resolve(process.env.GITHUB_WORKSPACE || '.', rulePath); | |
| ruleDigest = crypto.createHash('sha256').update(fs.readFileSync(abs)).digest('hex'); | |
| } catch (e) { | |
| // Cannot prove the rules are unchanged -> do not narrow. OCR itself | |
| // would normally have failed on an unreadable rule file before this | |
| // step runs, so this is a belt-and-braces path. | |
| ruleUnverified = true; | |
| core.warning(`checkpoint: cannot read rule file ${rulePath} (${e.message}); forcing a full review.`); | |
| } | |
| } | |
| const fingerprint = crypto.createHash('sha256') | |
| .update(`${process.env.OCR_FINGERPRINT_INPUTS || ''}|${process.env.OCR_VERSION_ACTUAL || ''}|${ruleDigest}`) | |
| .digest('hex') | |
| .slice(0, 16); | |
| // git's own ancestry verdict: 0 = ancestor, 1 = not, 128 = the object | |
| // is not in this clone (shallow fetch, force-push, head_sha override). | |
| // Anything else (git missing, signal) is a resolver error. Never | |
| // treated as "ancestor" except on a literal 0. | |
| const isAncestor = (a, b) => | |
| spawnSync('git', ['merge-base', '--is-ancestor', a, b], { cwd: process.env.GITHUB_WORKSPACE }).status; | |
| const common = { | |
| github, | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| prNumber: context.issue.number, | |
| log: (m) => core.info(m), | |
| }; | |
| // One read serves both purposes: the range decision, and the verbatim | |
| // marker the posting step re-emits on a run that does not advance the | |
| // checkpoint (the summary body is rewritten wholesale, which would | |
| // otherwise erase it). Passing it into the resolver as `read` is what | |
| // keeps this to a single listComments pagination per run. | |
| let existing = { reason: 'resolver_error', payload: null, raw: '' }; | |
| let range = { mode: 'full', reason: 'resolver_error', from: '', to: process.env.HEAD_SHA || '' }; | |
| try { | |
| existing = await readCheckpointComment(common); | |
| range = await resolveCheckpointRange(Object.assign({}, common, { | |
| read: existing, | |
| enabled: true, | |
| sticky: process.env.OCR_STICKY_SUMMARY === 'true', | |
| fullReview: process.env.OCR_FULL_REVIEW === 'true', | |
| headSha: process.env.HEAD_SHA || '', | |
| baseRef: process.env.BASE_REF || '', | |
| mergeBase: process.env.MERGE_BASE || '', | |
| fingerprint, | |
| isAncestor, | |
| })); | |
| } catch (e) { | |
| // Widening is always safe; a resolver crash must not stop the review. | |
| core.warning(`checkpoint: range resolution failed (${e.message}); reviewing the full range.`); | |
| } | |
| // Last gate, applied after the ordered twelve: the rules this run will | |
| // apply could not be read, so no stored fingerprint can be trusted to | |
| // mean "same rules". Widening is always safe; narrowing is not. | |
| if (ruleUnverified && range.mode === 'checkpoint') { | |
| range.mode = 'full'; | |
| range.reason = 'rule_unreadable'; | |
| } | |
| // Empty RANGE_FROM means "review the full range": the review step | |
| // expands ${RANGE_FROM:-$MERGE_BASE}, so unset and empty behave alike. | |
| core.exportVariable('RANGE_FROM', range.mode === 'checkpoint' ? range.from : ''); | |
| core.exportVariable('OCR_CONFIG_FINGERPRINT', fingerprint); | |
| core.exportVariable('OCR_CHECKPOINT_CARRY', existing.raw || ''); | |
| const summary = range.mode === 'checkpoint' | |
| ? `checkpoint (${range.reason}): ${range.from}..${range.to}` | |
| : `full (${range.reason})`; | |
| core.setOutput('range_mode', range.mode); | |
| core.setOutput('range_summary', summary); | |
| core.info(`[checkpoint] reviewing ${summary}`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 324 - 413, Wrap the checkpoint-range setup and
output logic in the action script’s top-level try/catch, including helper
loading, readCheckpointComment, resolveCheckpointRange, and export/output calls.
On any unexpected error, log a warning and force the full-review fallback by
exporting an empty RANGE_FROM and setting checkpoint outputs consistently, so
the subsequent ocr review still runs.
| Two 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`, which failed to post nothing, 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. | ||
| - **Same-head reruns are empty.** Re-running the workflow without pushing produces a `checkpoint` range with `from == to` — nothing new to review. | ||
| - **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. 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the item count and the double negative.
Line 198 says "Two properties", but three bullets follow. Line 200 contains "which failed to post nothing", which reads as a double negative.
✏️ Proposed fix
-Two properties are worth knowing before you enable it:
+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`, which failed to post nothing, 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.
+- **Widen-only.** The start of the range only ever moves back. An older checkpoint produces a wider review, never a narrower one. A checkpoint advances only 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.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Two 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`, which failed to post nothing, 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. | |
| - **Same-head reruns are empty.** Re-running the workflow without pushing produces a `checkpoint` range with `from == to` — nothing new to review. | |
| - **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. 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. | |
| 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. A checkpoint advances only 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. | |
| - **Same-head reruns are empty.** Re-running the workflow without pushing produces a `checkpoint` range with `from == to` — nothing new to review. | |
| - **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. 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. |
🧰 Tools
🪛 LanguageTool
[grammar] ~200-~200: Ensure spelling is correct
Context: ..._state: complete`, which failed to post nothing, and whose summary comment actually pub...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/github_actions/README.md` around lines 198 - 202, Update the README
text introducing the checkpoint behavior to say “Three properties” instead of
“Two properties,” and revise the phrase “which failed to post nothing” in the
Widen-only bullet to clearly state that the run posted no findings. Preserve the
surrounding behavior descriptions and bullet structure.
Source: Linters/SAST tools
1 participant
Description
Incremental mode reduces duplicate publication, not review scope. On every PR update the Action recomputes merge-base-to-head and invokes OCR over that whole range (
action.yml:239-245,:279), so later pushes on a long-lived PR keep paying for work already reviewed.The measured case from alibaba#476: a push that changed 7 files / 323 lines was reviewed as 63 files / 15,700 lines — 48.6× larger by changed line count, 46:31 elapsed, 3.98M tokens.
This adds an opt-in
checkpoint_rangemode.Default behaviour is unchanged. The review step expands
${RANGE_FROM:-$MERGE_BASE}, so with the input off the argv is byte-identical to before — verified by executing the expansion out of the edited file withRANGE_FROMunset and empty, not by reading the diff.The gate is fail-closed and ordered. Thirteen distinct reasons, every one of which selects a full review and reports which one fired. Absent, corrupt, partial, failed, canceled, incompatible config, unreadable rules, non-ancestor, different base, wrong PR, wrong repo — all fall back to full. A wasted full review is cheap; a coverage gap is a correctness bug.
The checkpoint advances only on a run that is both complete and published.
terminal_state === "complete"andstats.failed === 0andstats.summaryUrl !== ""and a 40-hex head. Two publication signals are needed becausestats.failedcounts only failed inline posts, while findings routed to the summary are published solely in thefinalizeSummarybody. A run that cannot advance re-emits the previous marker byte-for-byte rather than erasing it, so one bad run does not reset the ratchet.Two properties carry the safety argument:
ocr review --from A --to Breviews merge-base(A,B)..B. When A is a verified ancestor of B the reviewed set is exactly A..B, and it strictly contains B'..B for any B' between A and B — so a stale, older checkpoint can only widen the next range, never open a gap. This is why the canceled-run case from Add completeness-gated cross-push range checkpoints to the reusable GitHub Action alibaba/open-code-review#476 is safe: the surviving run covers last-complete-head through latest-head.git merge-base --is-ancestorexits 128 when the object is not in the local store. That is treated as non-ancestor, never as ancestor, so an unknown checkpoint head falls back to full.Checkpoint reads are author-verified before the payload is parsed, because
findSummaryIssueCommentmatches on the marker substring alone and would otherwise trust a sticky comment posted by someone else.Scope
In: issue behaviors 3 (fallback), 4 and 5 (never advance on incomplete/canceled), 8 (machine-readable mode, reason, and exact from/to SHAs), plus the manual
full_reviewoverride (7).Custom rules are fingerprinted by content, not just by path, so editing a
rulefile invalidates the checkpoint instead of narrowing the next range under rules the earlier commits were never reviewed against. An unreadablerulepath forces a full review.Deferred: behavior 6 (same-head rerun as an observable no-op) and the model-identity axis in 2 beyond the config fingerprint. Additive on top of this gate and easier to review separately.
No Go changes, and no second manifest format — this consumes the
terminal_statevocabulary that landed in alibaba#520.Limitations
terminal_state === "complete"is computed bycomputeTerminal(internal/session/manifest.go:941) fromlen(cov.Failed) == 0over the selected set only. Waived items, and anything excluded beforeRegisterSelected, are inside "complete". The gate documents this where it reads the field rather than overclaiming that complete means every change was reviewed..github/workflows/ocr-review.ymltriggers onpull_request_target: types: [opened], so it never emits asynchronizeevent and cannot exercise this mode. It is exercised by consumers that review on update, which is where Add completeness-gated cross-push range checkpoints to the reusable GitHub Action alibaba/open-code-review#476's evidence comes from.Type of Change
How Has This Been Tested?
make testpasses locallynpm run test:github-actionsexits 0 and prints both suites' pass lines;make testandmake checkare unaffected (no Go changes). All 86 pre-existingpost-review-comments.test.jscases are unmodified and green, with new cases added alongside them covering the write/read round trip, each gate reason, the byte-for-byte carry, and the base64 payload boundary.Verification for the JS suite rests on
test:github-actionsinpackage.json:17— worth noting that no workflow under.github/workflowscurrently runs it.Checklist
go fmt,go vet)Related Issues
closes alibaba#476