diff --git a/action.yml b/action.yml index 8812e295..181a6ef1 100644 --- a/action.yml +++ b/action.yml @@ -119,6 +119,25 @@ inputs: with route_severity_below to route on either condition. required: false default: '' + checkpoint_range: + description: >- + Cross-push checkpoints. true = a run that reviewed everything it selected + records the head it covered in its sticky summary comment, and the next + run reviews only .. instead of + ... Fail-closed: if anything is in doubt — the + summary is missing or was not posted by this token, the marker is + unreadable, the base moved, the configuration changed, or git cannot prove + the checkpoint is an ancestor of the new head — the full range is reviewed + exactly as it is today. Requires sticky_summary; ignored without it. + required: false + default: 'false' + full_review: + description: >- + Force one full review even when checkpoint_range is enabled (reason + 'manual_full_review'). Use it to re-review a PR from the merge-base + without turning checkpointing off; the run still records a new checkpoint. + required: false + default: 'false' base_ref: description: >- Override the base ref. Provide this (and head_sha) when invoking from a @@ -154,6 +173,24 @@ outputs: summary_comment_url: description: URL of the posted/updated summary comment, if any. value: ${{ steps.post.outputs.summary_comment_url }} + range_mode: + description: >- + 'checkpoint' when this run reviewed only the range since the previous + checkpoint, 'full' when it reviewed from the merge-base. Empty when + checkpoint_range is not enabled. + value: ${{ steps.range.outputs.range_mode }} + range_summary: + description: >- + The reviewed range plus the reason it was chosen, e.g. + "full (base_changed)" or "checkpoint (ok): ..". Empty when + checkpoint_range is not enabled. + value: ${{ steps.range.outputs.range_summary }} + 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). + value: ${{ steps.post.outputs.checkpoint_after }} runs: using: composite @@ -252,6 +289,11 @@ runs: npm install -g "@alibaba-group/open-code-review@${OCR_VERSION}" echo "OpenCodeReview installed:" ocr version || true + # 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" - name: Configure OCR env: @@ -262,6 +304,114 @@ runs: ocr config set llm.extra_body "$OCR_EXTRA_BODY" ocr config set language "$OCR_LANGUAGE" + - name: Resolve review range + if: inputs.checkpoint_range == 'true' + id: range + uses: actions/github-script@v9 + env: + OCR_FULL_REVIEW: ${{ inputs.full_review }} + OCR_STICKY_SUMMARY: ${{ inputs.sticky_summary }} + # Everything that changes what a review would say. Any difference + # invalidates the checkpoint, because findings from the previous run are + # no longer comparable to what this configuration would produce. + OCR_FINGERPRINT_INPUTS: >- + ${{ inputs.llm_url }}|${{ inputs.llm_model }}|${{ inputs.llm_use_anthropic }}|${{ + inputs.language }}|${{ inputs.llm_extra_body }}|${{ inputs.rule }}|${{ + inputs.route_severity_below }}|${{ inputs.route_categories }} + OCR_RULE_PATH: ${{ inputs.rule }} + with: + github-token: ${{ inputs.github_token }} + 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}`); + - name: Run OpenCodeReview env: OCR_LLM_URL: ${{ inputs.llm_url }} @@ -276,7 +426,7 @@ runs: OCR_RULE: ${{ inputs.rule }} shell: bash run: | - ARGS=(--from "${MERGE_BASE}" --to "${HEAD_SHA}" --format json) + ARGS=(--from "${RANGE_FROM:-$MERGE_BASE}" --to "${HEAD_SHA}" --format json) [ -n "$OCR_REVIEW_CONCURRENCY" ] && ARGS+=(--concurrency "$OCR_REVIEW_CONCURRENCY") [ -n "$OCR_BACKGROUND" ] && ARGS+=(--background "$OCR_BACKGROUND") [ -n "$OCR_RULE" ] && ARGS+=(--rule "$OCR_RULE") @@ -316,6 +466,11 @@ runs: OCR_REVIEW_COMMENT_BATCH_SIZE: ${{ inputs.review_comment_batch_size }} OCR_ROUTE_SEVERITY_BELOW: ${{ inputs.route_severity_below }} OCR_ROUTE_CATEGORIES: ${{ inputs.route_categories }} + # Set by the Resolve review range step; empty when checkpointing is off. + OCR_CHECKPOINT_CARRY: ${{ env.OCR_CHECKPOINT_CARRY }} + OCR_CONFIG_FINGERPRINT: ${{ env.OCR_CONFIG_FINGERPRINT }} + OCR_BASE_REF: ${{ env.BASE_REF }} + OCR_MERGE_BASE: ${{ env.MERGE_BASE }} with: github-token: ${{ inputs.github_token }} script: | @@ -346,4 +501,9 @@ runs: reviewCommentBatchSize: parseInt(process.env.OCR_REVIEW_COMMENT_BATCH_SIZE, 10), routeSeverityBelow: process.env.OCR_ROUTE_SEVERITY_BELOW, routeCategories: process.env.OCR_ROUTE_CATEGORIES, + checkpointEnabled: ${{ inputs.checkpoint_range == 'true' }}, + checkpointCarry: process.env.OCR_CHECKPOINT_CARRY || '', + checkpointBaseRef: process.env.OCR_BASE_REF || '', + checkpointMergeBase: process.env.OCR_MERGE_BASE || '', + checkpointFingerprint: process.env.OCR_CONFIG_FINGERPRINT || '', }); diff --git a/examples/github_actions/README.md b/examples/github_actions/README.md index 0236505f..ce4f6b72 100644 --- a/examples/github_actions/README.md +++ b/examples/github_actions/README.md @@ -159,6 +159,54 @@ The action posts a summary issue comment plus inline review comments. Two inputs > `sticky_summary` and `incremental` must be quoted strings (`'true'`/`'false'`); the action compares them as strings, so an unquoted YAML boolean will not match. +### Review only what changed since the last run (checkpoints) + +`incremental` filters the comments a run produces; it still reviews the whole `merge-base..head` diff every time. On a long-lived PR that means re-reading the same 40 commits on every push. `checkpoint_range` fixes the other half: a run that reviewed everything it selected records the head it covered in a hidden marker inside its sticky summary comment, and the next run reviews `..` instead. + +| Input | Default | Description | +|-------|---------|-------------| +| `checkpoint_range` | `'false'` | Review only the range since the last recorded checkpoint. Requires `sticky_summary: 'true'` (the checkpoint lives in that comment). | +| `full_review` | `'false'` | Force one full review even with `checkpoint_range` enabled. The run still records a new checkpoint. | + +```yaml +- uses: alibaba/open-code-review@main + with: + sticky_summary: 'true' + checkpoint_range: 'true' +``` + +**When in doubt, this reviews the full range.** A checkpoint is used only when every one of these holds; otherwise the run reviews `merge-base..head` exactly as it does today, and the reason is reported in the `range_summary` output and the step log: + +| Reason | The run reviewed the full range because | +|--------|------------------------------------------| +| `disabled` | `checkpoint_range` is not `'true'` | +| `sticky_disabled` | `sticky_summary` is not `'true'`, so there is nowhere durable to keep a checkpoint | +| `manual_full_review` | `full_review: 'true'` was requested | +| `no_summary_comment` | the PR has no sticky summary yet (the first run) | +| `author_unverified` | the summary comment was not posted by this workflow's token | +| `corrupt_checkpoint` | the summary carries no readable checkpoint marker (absent, malformed, or two of them) | +| `schema_invalid` | the marker is for another PR, another marker version, or records a run that did not complete | +| `base_changed` | the base ref or the merge-base moved, so the diff basis is no longer the one the checkpoint was taken against | +| `config_changed` | the model, language, `llm_extra_body`, rules, routing inputs, or the resolved OCR version changed | +| `not_ancestor` | the checkpoint is not an ancestor of the new head (force-push, rebase) | +| `unknown_object` | the checkpoint commit is not in this clone, so ancestry could not be checked | +| `rule_unreadable` | a `rule` path was given but could not be read, so no stored fingerprint can be trusted to mean "same rules" | +| `resolver_error` | the comment could not be read, or `git merge-base --is-ancestor` could not run | + +Three outputs report what happened: `range_mode` (`checkpoint` or `full`), `range_summary` (the mode, the reason, and the range), and `checkpoint_after` (the head recorded as the new checkpoint, or empty when the run did not advance one). + +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. + +> **Caveat — what `complete` covers.** `terminal_state: complete` means nothing in the set the run *selected* failed. Items the run waived, or excluded before selection (unsupported files, size limits), are inside that guarantee. So a checkpoint means "everything this configuration chose to review was reviewed", not "every byte of the diff was read". Changing the configuration invalidates the checkpoint (`config_changed`), which is what keeps that promise honest across runs. + +> **Custom rules are fingerprinted by content.** If you pass `rule`, the checkpoint fingerprint covers that file's *contents*, not just its path — editing your rule file invalidates the checkpoint (`config_changed`) so the next run re-reviews from the merge-base under the new rules, rather than narrowing to the newest commits. The built-in rule set is embedded in the binary and moves with `ocr_version`, which is already part of the fingerprint. A `rule` path that cannot be read forces a full review (`rule_unreadable`). + +> **Caveat — the trust boundary is write permission.** The checkpoint is read only from a comment authored by this workflow's own token, verified against the API rather than by matching the `github-actions[bot]` name. That proves who *posted* the comment, not that its body is unmodified: anyone with write permission on the repository can edit a bot comment and move the checkpoint forward, causing a range to be skipped. The boundary this buys is "write-permission holders are trusted" — a fork contributor, who is exactly the untrusted party under `pull_request_target`, cannot plant or alter a marker. If that is not an acceptable assumption for your repository, leave `checkpoint_range` off. + ### Adjust retry and delay settings When posting review comments individually (fallback mode), the action honors GitHub rate-limit headers (`retry-after`, `x-ratelimit-*`) with exponential backoff. The retry strategy follows GitHub's documented guidance for REST API rate limits — see [Rate limits for the REST API](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2026-03-10) for details on primary/secondary rate limits and recommended retry behavior: diff --git a/scripts/github-actions/post-review-comments.js b/scripts/github-actions/post-review-comments.js index 231aad0d..60c3cd7f 100644 --- a/scripts/github-actions/post-review-comments.js +++ b/scripts/github-actions/post-review-comments.js @@ -84,6 +84,17 @@ async function runPostReviewComments({ // (fail-open for the policy itself, upholding I1). routeSeverityBelow = "", routeCategories = "", + // Cross-push checkpoints (#476). Off by default: with checkpointEnabled + // false and an empty carry, every emitted body is byte-identical to today's. + // checkpointCarry is the raw marker string the resolve step read from the + // existing summary; it is re-emitted verbatim on any run that does not + // advance, because the summary body is rewritten wholesale and would + // otherwise erase the checkpoint. + checkpointEnabled = false, + checkpointCarry = "", + checkpointBaseRef = "", + checkpointMergeBase = "", + checkpointFingerprint = "", }) { const log = (msg) => { if (core && typeof core.info === "function") core.info(msg); @@ -113,6 +124,61 @@ async function runPostReviewComments({ failed: 0, routed: 0, summaryUrl: "", + checkpointAfter: "", + }; + + // ---- 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 + // to full reviews, and never silently skips a range that was not reviewed. + // + // The fourth condition — the summary was actually published — is enforced in + // two places: structurally, because the marker lives INSIDE the summary body + // (a summary that never lands carries no checkpoint), and explicitly on the + // checkpoint_after output in setStatsOutputs. + // + // NOTE on terminal_state: per computeTerminal (internal/session/manifest.go:941) + // "complete" means nothing in the SELECTED set failed. Items the run waived or + // excluded before selection are inside that guarantee, so "complete" is + // completeness relative to what the run chose to review — the strongest signal + // the manifest offers, and the reason the checkpoint tracks the run's own + // resolved_head rather than the runner's HEAD_SHA. + const buildAdvanceMarker = (manifest) => { + stats.checkpointAfter = ""; + if (!checkpointEnabled || !stickySummary) return null; + if (!manifest || manifest.terminal_state !== "complete") return null; + if (stats.failed !== 0) return null; + const head = (manifest.input && manifest.input.resolved_head) || ""; + if (!/^[0-9a-f]{40}$/.test(head)) return null; + stats.checkpointAfter = head; + return buildCheckpointMarker({ + v: CHECKPOINT_VERSION, + pr: prNumber, + head, + base_ref: checkpointBaseRef, + merge_base: checkpointMergeBase, + terminal_state: "complete", + fingerprint: checkpointFingerprint, + run: String(context.runId != null ? context.runId : ""), + }); + }; + // Applied at every site that composes a summary body. Advancing supersedes the + // carry (a body must carry at most one marker); with neither, the body is + // exactly what it is today. + const appendCheckpoint = (body, manifest) => { + const marker = buildAdvanceMarker(manifest); + if (marker) return `${body}\n\n${marker}`; + // The carry is gated on the same two conditions as the advance. Without + // checkpointing there is nothing to carry; without a sticky summary each run + // posts a FRESH comment, so re-emitting a marker read from a previous run's + // comment would stamp a checkpoint onto a body that never carried one — and + // the resolver, which reads the newest summary, would then trust a head this + // run did not review. + if (!checkpointEnabled || !stickySummary) return body; + return checkpointCarry ? `${body}\n\n${checkpointCarry}` : body; }; // Read OCR output. @@ -124,7 +190,12 @@ async function runPostReviewComments({ log(`Failed to parse OCR output: ${e.message}`); const stderr = safeRead(fs, stderrPath).trim(); if (stderr) { - const body = `${SUMMARY_MARKER}\n⚠️ **OpenCodeReview** encountered an error:\n${fencedBlock(stderr)}`; + // No manifest exists on this path (the output could not be parsed), so it + // can only ever carry the previous checkpoint forward — never advance it. + const body = appendCheckpoint( + `${SUMMARY_MARKER}\n⚠️ **OpenCodeReview** encountered an error:\n${fencedBlock(stderr)}`, + null + ); const posted = await postSummary({ github, owner, repo, prNumber, body, sticky: stickySummary, log }); stats.summaryUrl = posted.url; } @@ -139,7 +210,8 @@ async function runPostReviewComments({ // No comments: post a "looks good" summary. if (comments.length === 0) { const message = result.message || "No comments generated. Looks good to me."; - const body = `${SUMMARY_MARKER}\n✅ **OpenCodeReview**: ${message}`; + // A clean run is still a complete run: this path advances the checkpoint. + const body = appendCheckpoint(`${SUMMARY_MARKER}\n✅ **OpenCodeReview**: ${message}`, result.manifest); const posted = await postSummary({ github, owner, repo, prNumber, body, sticky: stickySummary, log }); stats.summaryUrl = posted.url; setStatsOutputs(out, stats); @@ -343,7 +415,7 @@ async function runPostReviewComments({ anchor, sticky: stickySummary, tag: SUMMARY_TAG, - body: wrapSummary(summaryBody), + body: wrapSummary(appendCheckpoint(summaryBody, result.manifest)), log, }); if (finalized) stats.summaryUrl = finalized.url; @@ -773,6 +845,11 @@ function setStatsOutputs(out, stats, batchCounters, batchSize) { out("comments_routed", String(stats.routed)); out("comments_failed", String(stats.failed)); out("summary_comment_url", stats.summaryUrl || ""); + // The head this run's checkpoint advanced to, or "" when it did not advance + // (#476). Gated on summaryUrl because the marker lives inside the summary + // comment: a summary that never published carries no checkpoint, so claiming + // one on the output would lie to the caller. + out("checkpoint_after", stats.summaryUrl && stats.checkpointAfter ? stats.checkpointAfter : ""); // Per-batch telemetry (B7). These are additional outputs; the five above are // unchanged so existing consumers of comments_* / summary_comment_url are // unaffected. batch_summary is a single JSON string so a fleet dashboard can @@ -2059,6 +2136,235 @@ async function getPrDiffHunks({ github, owner, repo, prNumber, commitSha, log, c return diff; } +// ---- Cross-push checkpoints (#476) ---- +// +// Opt-in. A run that provably reviewed everything it selected records the head +// it covered in a hidden marker inside its sticky summary comment; the next run +// may then review only .. instead of +// .., so a 40-commit PR is not re-reviewed from scratch +// on every push. +// +// The whole design is fail-closed: resolveCheckpointRange runs an ordered gate +// and ANY doubt — feature off, summary missing, marker unreadable, base moved, +// config changed, ancestry unprovable — returns mode "full", which reviews the +// same range the action reviews today. The narrowed range is only ever taken +// when every condition holds. +// +// TRUST BOUNDARY: the marker is read only from a comment authored by this run's +// own authenticated identity. That proves who POSTED the comment, not that its +// body is unmodified — anyone with write permission on the repository can edit +// a bot comment. So the boundary this buys is "write-permission holders are +// trusted"; a fork contributor (no write permission) cannot plant or alter a +// marker, which is the case that matters for pull_request_target. + +const CHECKPOINT_VERSION = 1; +// 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 = ""; +const CHECKPOINT_SHA_RE = /^[0-9a-f]{40}$/; + +function buildCheckpointMarker(payload) { + const encoded = Buffer.from(JSON.stringify(payload), "utf8").toString("base64"); + return ``; +} + +// Extract the checkpoint payload from a comment body, or null when the body +// carries no readable checkpoint. Null covers BOTH "no marker at all" (the +// normal first run on a PR) and "marker present but unusable"; the caller maps +// both to the same fail-closed reason because neither yields a usable range. +// Two markers in one body are ambiguous and therefore also null. +function parseCheckpointMarker(body) { + if (typeof body !== "string" || body === "") return null; + const re = new RegExp(CHECKPOINT_MARKER_PATTERN, "g"); + const found = []; + let m; + while ((m = re.exec(body)) !== null) found.push(m[1]); + if (found.length !== 1) return null; + let payload; + try { + payload = JSON.parse(Buffer.from(found[0], "base64").toString("utf8")); + } catch (e) { + return null; + } + if (!payload || typeof payload !== "object" || Array.isArray(payload)) return null; + return payload; +} + +// Structural validation of a parsed payload. Returns null when the payload is +// usable, otherwise a short human-readable reason (logged, not branched on). +// +// terminal_state must be "complete". NOTE: per computeTerminal +// (internal/session/manifest.go:941) "complete" means no item in the SELECTED +// set failed — waived items and items excluded before selection are INSIDE +// that guarantee, i.e. "complete" is completeness with respect to what the run +// chose to review, not with respect to the whole diff. That is the strongest +// signal the manifest offers, and it is why the range is only ever narrowed +// past a run that reported it. +function validateCheckpointPayload(payload, { prNumber } = {}) { + if (!payload) return "no payload"; + if (payload.v !== CHECKPOINT_VERSION) return `unsupported version ${JSON.stringify(payload.v)}`; + if (prNumber != null && payload.pr !== prNumber) return `belongs to PR ${JSON.stringify(payload.pr)}`; + if (!CHECKPOINT_SHA_RE.test(payload.head)) return "head is not a 40-hex sha"; + if (payload.terminal_state !== "complete") return `terminal_state ${JSON.stringify(payload.terminal_state)}`; + if (typeof payload.base_ref !== "string" || payload.base_ref === "") return "missing base_ref"; + if (typeof payload.merge_base !== "string" || payload.merge_base === "") return "missing merge_base"; + if (typeof payload.fingerprint !== "string" || payload.fingerprint === "") return "missing fingerprint"; + return null; +} + +// Read the checkpoint payload out of this PR's sticky summary comment, with the +// author check applied. Returns { reason, payload, raw }: +// reason "ok" -> payload is the marker's decoded payload and raw +// is the marker string, byte for byte as read +// "no_summary_comment" -> no sticky summary exists yet +// "author_unverified" -> the summary was not posted by this identity +// "corrupt_checkpoint" -> no readable marker in the body +// "resolver_error" -> the comment could not be listed at all +async function readCheckpointComment({ github, owner, repo, prNumber, log }) { + let comment; + try { + comment = await findSummaryIssueComment({ + github, + owner, + repo, + prNumber, + sticky: true, + tag: "", + log, + }); + } catch (e) { + log(`[checkpoint] cannot list issue comments (${e.message}); reviewing the full range.`); + return { reason: "resolver_error", payload: null, raw: "" }; + } + if (!comment) return { reason: "no_summary_comment", payload: null, raw: "" }; + + const botLogin = await getAuthenticatedLogin(github, log); + // Fail closed on an unresolvable identity: do NOT fall back to matching the + // literal "github-actions[bot]" login (isBotComment's secondary path), since + // a name is not evidence of authorship. + if (!botLogin) { + log("[checkpoint] authenticated identity unavailable; reviewing the full range."); + return { reason: "author_unverified", payload: null, raw: "" }; + } + // 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) { + log("[checkpoint] summary comment was not authored by this token; reviewing the full range."); + return { reason: "author_unverified", payload: null, raw: "" }; + } + + const body = comment.body || ""; + const payload = parseCheckpointMarker(body); + if (!payload) return { reason: "corrupt_checkpoint", payload: null, raw: "" }; + const raw = (new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body) || [""])[0]; + return { reason: "ok", payload, raw }; +} + +// The ordered gate. Returns exactly eight keys: +// mode "checkpoint" | "full" +// reason "ok" | one of the twelve fail-closed reasons +// from the checkpoint head to review from ("" in full mode, so the +// caller's ${RANGE_FROM:-$MERGE_BASE} keeps today's range) +// to headSha +// checkpointBefore the head recorded by the marker that was read, if any +// ancestry "" (not probed) | ancestor | not_ancestor | unknown_object | error +// sourceRun the run id that wrote the marker, if any +// fingerprint the config fingerprint the marker recorded, if any +// +// isAncestor(a, b) must resolve to git's `merge-base --is-ancestor` exit code: +// 0 = a is an ancestor of b, 1 = it is not, 128 = the object is not in this +// clone. 128 is deliberately NOT treated as "not an ancestor": it means we +// could not check, and it is the state a shallow clone or a head_sha override +// (no PR to fetch from) produces, so it gets its own reason to stay greppable. +async function resolveCheckpointRange({ + github, + owner, + repo, + prNumber, + enabled = false, + sticky = true, + fullReview = false, + headSha = "", + baseRef = "", + mergeBase = "", + fingerprint = "", + isAncestor, + // Optional pre-read from readCheckpointComment. The caller needs the raw + // marker string anyway (to carry it forward on a run that does not advance), + // and that read costs a full listComments pagination plus an identity lookup. + // Passing it in keeps the whole feature at one read per run and makes the + // range decision and the carried marker come from the same observation. + read = null, + log = () => {}, +}) { + const full = (reason, seen) => + Object.assign( + { + mode: "full", + reason, + from: "", + to: headSha, + checkpointBefore: "", + ancestry: "", + sourceRun: "", + fingerprint: "", + }, + seen + ); + + if (!enabled) return full("disabled"); + if (!sticky) return full("sticky_disabled"); + if (fullReview) return full("manual_full_review"); + + const seenComment = read || (await readCheckpointComment({ github, owner, repo, prNumber, log })); + if (seenComment.reason !== "ok") return full(seenComment.reason); + + const p = seenComment.payload; + const seen = { + checkpointBefore: typeof p.head === "string" ? p.head : "", + sourceRun: typeof p.run === "string" ? p.run : "", + fingerprint: typeof p.fingerprint === "string" ? p.fingerprint : "", + }; + + const invalid = validateCheckpointPayload(p, { prNumber }); + if (invalid) { + log(`[checkpoint] marker rejected (${invalid}); reviewing the full range.`); + return full("schema_invalid", seen); + } + // The base moved (branch advanced, rebase, different base ref): the diff basis + // is no longer the one the checkpoint was taken against, so nothing about the + // earlier review carries over. + 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); + + let status; + try { + status = await isAncestor(p.head, headSha); + } catch (e) { + log(`[checkpoint] ancestry check failed (${e.message}); reviewing the full range.`); + return full("resolver_error", seen); + } + if (status === 1) return full("not_ancestor", Object.assign({ ancestry: "not_ancestor" }, seen)); + if (status === 128) return full("unknown_object", Object.assign({ ancestry: "unknown_object" }, seen)); + if (status !== 0) return full("resolver_error", Object.assign({ ancestry: "error" }, seen)); + + return { + mode: "checkpoint", + reason: "ok", + from: p.head, + to: headSha, + checkpointBefore: p.head, + ancestry: "ancestor", + sourceRun: seen.sourceRun, + fingerprint: p.fingerprint, + }; +} + module.exports = { runPostReviewComments, postSummary, @@ -2115,4 +2421,10 @@ module.exports = { isLineResolutionFailure, cooldownAndReconcile, getPrDiffHunks, + buildCheckpointMarker, + parseCheckpointMarker, + validateCheckpointPayload, + readCheckpointComment, + resolveCheckpointRange, + CHECKPOINT_VERSION, }; diff --git a/scripts/github-actions/post-review-comments.test.js b/scripts/github-actions/post-review-comments.test.js index a08c258e..9f97b963 100644 --- a/scripts/github-actions/post-review-comments.test.js +++ b/scripts/github-actions/post-review-comments.test.js @@ -16,7 +16,7 @@ const assert = require("assert"); const path = require("path"); -const { runPostReviewComments, safeFence, fencedBlock, lineSpan, sameCommentSpan, overlapsHistory, resolveThreshold, DEFAULT_OVERLAP_THRESHOLD, newCommentId, getPostedCommentIds, computeRetryDelayMs, formatWarnings, resolveBatchSize, sortToSendDeterministically, chunkArray, buildRunTags, DEFAULT_BATCH_SIZE, buildBadge, sanitizeMetadata, buildPolicy, routeComment, formatComment, formatCommentMarkdown, NO_ROUTING, CATEGORIES, SEVERITIES, SEVERITY_RANK, parseDiffHunkRanges, classifyCommentAgainstDiff, describeCommentLocation, isLineResolutionFailure, getPrDiffHunks } = require(path.join(__dirname, "post-review-comments.js")); +const { runPostReviewComments, safeFence, fencedBlock, lineSpan, sameCommentSpan, overlapsHistory, resolveThreshold, DEFAULT_OVERLAP_THRESHOLD, newCommentId, getPostedCommentIds, computeRetryDelayMs, formatWarnings, resolveBatchSize, sortToSendDeterministically, chunkArray, buildRunTags, DEFAULT_BATCH_SIZE, buildBadge, sanitizeMetadata, buildPolicy, routeComment, formatComment, formatCommentMarkdown, NO_ROUTING, CATEGORIES, SEVERITIES, SEVERITY_RANK, parseDiffHunkRanges, classifyCommentAgainstDiff, describeCommentLocation, isLineResolutionFailure, getPrDiffHunks, SUMMARY_MARKER, buildCheckpointMarker, parseCheckpointMarker, validateCheckpointPayload, readCheckpointComment, resolveCheckpointRange } = require(path.join(__dirname, "post-review-comments.js")); // REVIEW_TAG as the production code builds it for this test's hardcoded run // identity (context.runId=undefined -> 0, runAttempt=undefined -> 1). Used as @@ -2210,6 +2210,26 @@ async function main() { await testDiffFetchFailureDegradesToPerComment(); await testAllCommentsFilteredOutAccounting(); await testCrossHunkRangeIsFilteredOut(); + // Cross-push checkpoints (#476) — read path + testCheckpointMarkerRoundTrip(); + testValidateCheckpointPayload(); + await testCheckpointTwelveReasonsAreDistinct(); + await testCheckpointSingleFieldMutationsFailClosed(); + await testCheckpointAuthorVerificationFailsClosed(); + await testCheckpointRejectsDifferentBotIdentity(); + await testCheckpointWidenOnly(); + await testCheckpointSameHeadRerun(); + await testCheckpointResolveShape(); + // Cross-push checkpoints (#476) — write path + await testCheckpointAdvanceGateTable(); + await testCheckpointAdvanceRequiresFullSha(); + await testCheckpointCarryForwardOnEveryBodyPath(); + await testCheckpointAdvancesOnZeroFindings(); + await testCheckpointNeverAdvancesWithoutSticky(); + await testCheckpointCarryIsGatedLikeTheAdvance(); + await testCheckpointResolverUsesPreReadComment(); + await testCheckpointOptOutLeavesBodyUnchanged(); + await testCheckpointWriteThenReadRoundTrip(); console.log("All post-review-comments tests passed."); } function testParseDiffHunkRanges() { @@ -3187,6 +3207,747 @@ async function testCrossHunkRangeIsFilteredOut() { assert.strictEqual(summaryText.includes("Lines 2-51 could not be resolved"), true); } +// --------------------------------------------------------------------------- +// Cross-push checkpoints (#476) — read path +// --------------------------------------------------------------------------- + +const CK_OLD = "1a".repeat(20); +const CK_MID = "2b".repeat(20); +const CK_NEW = "3c".repeat(20); +const CK_MB = "4d".repeat(20); +const CK_MARKER_RE = /^$/; + +function ckPayload(over = {}) { + return Object.assign( + { + v: 1, + pr: 123, + head: CK_OLD, + base_ref: "main", + merge_base: CK_MB, + terminal_state: "complete", + fingerprint: "fp1", + run: "run-1", + }, + over + ); +} + +// A sticky summary comment carrying (or not carrying) a checkpoint marker. +function ckComment({ payload, login = "github-actions[bot]", body } = {}) { + const tail = body != null ? body : payload ? buildCheckpointMarker(payload) : ""; + return { + id: 1, + html_url: "http://ex/1", + user: { login }, + body: `${SUMMARY_MARKER}\nSummary prose\n\n${tail}`, + }; +} + +function ckGithub({ comments, login = "github-actions[bot]", listThrows = false, authThrows = false } = {}) { + return { + rest: { + users: { + getAuthenticated: async () => { + if (authThrows) throw new Error("auth unavailable"); + return { data: { login } }; + }, + }, + issues: { + listComments: async () => { + if (listThrows) throw new Error("listComments 503"); + return { data: comments || [] }; + }, + }, + }, + }; +} + +function ckArgs(over = {}) { + return Object.assign( + { + github: ckGithub({ comments: [ckComment({ payload: ckPayload() })] }), + owner: "owner", + repo: "repo", + prNumber: 123, + enabled: true, + sticky: true, + fullReview: false, + headSha: CK_NEW, + baseRef: "main", + mergeBase: CK_MB, + fingerprint: "fp1", + isAncestor: async () => 0, + log: () => {}, + }, + over + ); +} + +// C10: the marker survives payloads that could break out of an HTML comment, +// and every malformed body reads as "no checkpoint" instead of throwing. +function testCheckpointMarkerRoundTrip() { + const payloads = [ + ckPayload(), + ckPayload({ note: "contains --> a closing sequence" }), + ckPayload({ note: "line one\nline two" }), + ckPayload({ note: "多字节 テキスト ✅" }), + ]; + for (const p of payloads) { + const marker = buildCheckpointMarker(p); + assert.strictEqual(CK_MARKER_RE.test(marker), true, `marker shape for ${JSON.stringify(p.note)}`); + assert.deepStrictEqual(parseCheckpointMarker(marker), p, "round trip must be lossless"); + // Embedded in a real body, surrounded by prose. + assert.deepStrictEqual(parseCheckpointMarker(`${SUMMARY_MARKER}\nprose\n${marker}\n`), p); + } + + const b64 = (s) => Buffer.from(s, "utf8").toString("base64"); + const malformed = [ + ["empty string", ""], + ["non-base64 payload", ""], + ["base64 of non-JSON", ``], + ["base64 of a JSON array", ``], + ["--> inside summary prose", "Summary mentioning --> an arrow, with no marker"], + ["two markers in one body", `${buildCheckpointMarker(ckPayload())}\n${buildCheckpointMarker(ckPayload({ head: CK_MID }))}`], + ]; + for (const [label, body] of malformed) { + assert.strictEqual(parseCheckpointMarker(body), null, `${label} must read as no checkpoint`); + } + // Non-string input must not throw either. + assert.strictEqual(parseCheckpointMarker(undefined), null); + assert.strictEqual(parseCheckpointMarker(null), null); +} + +// C2: each of the twelve fail-closed reasons has its own input class, and every +// one of them reviews the FULL range. +async function testCheckpointTwelveReasonsAreDistinct() { + const cases = [ + ["disabled", { enabled: false }], + ["sticky_disabled", { sticky: false }], + ["manual_full_review", { fullReview: true }], + ["no_summary_comment", { github: ckGithub({ comments: [] }) }], + ["author_unverified", { github: ckGithub({ comments: [ckComment({ payload: ckPayload(), login: "fork-user" })] }) }], + ["corrupt_checkpoint", { github: ckGithub({ comments: [ckComment({ body: "no marker here" })] }) }], + ["schema_invalid", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ v: 2 }) })] }) }], + ["base_changed", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ merge_base: CK_MID }) })] }) }], + ["config_changed", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ fingerprint: "other" }) })] }) }], + ["not_ancestor", { isAncestor: async () => 1 }], + ["unknown_object", { isAncestor: async () => 128 }], + ["resolver_error", { github: ckGithub({ listThrows: true }) }], + ]; + const seen = new Set(); + for (const [reason, over] of cases) { + const r = await resolveCheckpointRange(ckArgs(over)); + assert.strictEqual(r.reason, reason, `expected reason ${reason}, got ${r.reason}`); + assert.strictEqual(r.mode, "full", `${reason} must review the full range`); + assert.strictEqual(r.from, "", `${reason} must not narrow the range`); + seen.add(reason); + } + assert.strictEqual(seen.size, 12, "all twelve reasons must be reachable"); + + // Exit 128 is "could not check", never "is an ancestor": it must not slip + // through as a checkpoint under any other exit code either. + for (const status of [2, -1, 129, null, undefined, "0"]) { + const r = await resolveCheckpointRange(ckArgs({ isAncestor: async () => status })); + assert.strictEqual(r.mode, "full", `ancestry status ${status} must fail closed`); + } + // A throwing resolver is an error, not an ancestor. + const thrown = await resolveCheckpointRange( + ckArgs({ isAncestor: async () => { throw new Error("git missing"); } }) + ); + assert.strictEqual(thrown.reason, "resolver_error"); + assert.strictEqual(thrown.mode, "full"); +} + +// C3: from one fully-valid input, every single-field mutation falls back to the +// full range; only the unmutated row narrows it. +async function testCheckpointSingleFieldMutationsFailClosed() { + const mutations = [ + ["v", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ v: 2 }) })] }) }], + ["pr", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ pr: 999 }) })] }) }], + ["head format", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ head: "not-a-sha" }) })] }) }], + ["terminal_state", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ terminal_state: "partial" }) })] }) }], + ["merge_base", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ merge_base: CK_MID }) })] }) }], + ["base_ref", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ base_ref: "develop" }) })] }) }], + ["fingerprint", { github: ckGithub({ comments: [ckComment({ payload: ckPayload({ fingerprint: "fp2" }) })] }) }], + ["ancestry exit 1", { isAncestor: async () => 1 }], + ["ancestry exit 128", { isAncestor: async () => 128 }], + ["comment author", { github: ckGithub({ comments: [ckComment({ payload: ckPayload(), login: "someone-else" })] }) }], + ["stickySummary:false", { sticky: false }], + ]; + assert.strictEqual(mutations.length, 11, "eleven single-field mutations"); + for (const [label, over] of mutations) { + const r = await resolveCheckpointRange(ckArgs(over)); + assert.strictEqual(r.mode, "full", `mutating ${label} must review the full range`); + assert.notStrictEqual(r.reason, "ok", `mutating ${label} must carry a fail-closed reason`); + } + const control = await resolveCheckpointRange(ckArgs()); + assert.strictEqual(control.mode, "checkpoint"); + assert.strictEqual(control.reason, "ok"); + assert.strictEqual(control.from, CK_OLD, "the unmutated row narrows to the checkpoint head"); + assert.strictEqual(control.to, CK_NEW); +} + +// C9: authorship is verified against the resolved identity only. A well-formed +// marker planted by a fork contributor is ignored, and an unresolvable identity +// never degrades to matching the literal "github-actions[bot]" name. +async function testCheckpointAuthorVerificationFailsClosed() { + const planted = ckComment({ payload: ckPayload(), login: "fork-contributor" }); + const forked = await resolveCheckpointRange(ckArgs({ github: ckGithub({ comments: [planted] }) })); + assert.strictEqual(forked.reason, "author_unverified"); + assert.strictEqual(forked.mode, "full"); + + // getAuthenticatedLogin rejects -> null identity. The comment IS literally + // authored by "github-actions[bot]", so a name-matching fallback would accept + // it; the gate must not. + const unresolved = await resolveCheckpointRange( + ckArgs({ github: ckGithub({ comments: [ckComment({ payload: ckPayload() })], authThrows: true }) }) + ); + assert.strictEqual(unresolved.reason, "author_unverified"); + assert.strictEqual(unresolved.mode, "full"); + + // Same via the lower-level reader, so the fail-closed behavior is pinned at + // the boundary and not only through the gate. + const read = await readCheckpointComment({ + github: ckGithub({ comments: [ckComment({ payload: ckPayload() })], authThrows: true }), + owner: "owner", + repo: "repo", + prNumber: 123, + log: () => {}, + }); + assert.strictEqual(read.reason, "author_unverified"); + assert.strictEqual(read.payload, null); +} + +// C9 (continued): the identity must match EXACTLY. This run authenticates as a +// GitHub App, so a marker left by the default GITHUB_TOKEN +// ("github-actions[bot]") is a different writer. isBotComment's name-shaped +// secondary path would accept it; the checkpoint gate must not. +async function testCheckpointRejectsDifferentBotIdentity() { + const github = ckGithub({ comments: [ckComment({ payload: ckPayload() })], login: "ocr-app[bot]" }); + const gated = await resolveCheckpointRange(ckArgs({ github })); + assert.strictEqual(gated.reason, "author_unverified"); + assert.strictEqual(gated.mode, "full"); + assert.strictEqual(gated.from, ""); + + const read = await readCheckpointComment({ github, owner: "owner", repo: "repo", prNumber: 123, log: () => {} }); + assert.strictEqual(read.reason, "author_unverified"); + assert.strictEqual(read.raw, ""); +} + +// C7: widen-only. An older checkpoint yields a wider range; the resolver never +// moves the start forward past what the marker recorded. +async function testCheckpointWidenOnly() { + // Ancestry chain OLD -> MID -> NEW under the real 0/1/128 exit-code contract. + const chain = [CK_OLD, CK_MID, CK_NEW]; + const isAncestor = async (a, b) => { + const i = chain.indexOf(a); + const j = chain.indexOf(b); + if (i < 0 || j < 0) return 128; + return i <= j ? 0 : 1; + }; + const withHead = async (head) => + resolveCheckpointRange( + ckArgs({ github: ckGithub({ comments: [ckComment({ payload: ckPayload({ head }) })] }), isAncestor }) + ); + + const older = await withHead(CK_OLD); + assert.strictEqual(older.mode, "checkpoint"); + assert.strictEqual(older.from, CK_OLD, "an older checkpoint reviews the wider range"); + const newer = await withHead(CK_MID); + assert.strictEqual(newer.mode, "checkpoint"); + assert.strictEqual(newer.from, CK_MID); + + // An object outside this clone is unprovable, never an ancestor. + const unknown = await withHead("9".repeat(40)); + assert.strictEqual(unknown.reason, "unknown_object"); + assert.strictEqual(unknown.mode, "full"); +} + +// C12: rerunning on the same head is a legal (empty) checkpoint range, not an +// error and not a silent full re-review. +async function testCheckpointSameHeadRerun() { + const r = await resolveCheckpointRange( + ckArgs({ github: ckGithub({ comments: [ckComment({ payload: ckPayload({ head: CK_NEW }) })] }) }) + ); + assert.strictEqual(r.mode, "checkpoint"); + assert.strictEqual(r.reason, "ok"); + assert.strictEqual(r.from, CK_NEW); + assert.strictEqual(r.from, r.to); +} + +// C11: the resolver's contract is a fixed eight-key shape in both modes, so a +// consumer can read every field without existence checks. +async function testCheckpointResolveShape() { + const expected = [ + "ancestry", + "checkpointBefore", + "fingerprint", + "from", + "mode", + "reason", + "sourceRun", + "to", + ]; + const checkpoint = await resolveCheckpointRange(ckArgs()); + assert.deepStrictEqual(Object.keys(checkpoint).sort(), expected); + assert.deepStrictEqual( + [checkpoint.checkpointBefore, checkpoint.ancestry, checkpoint.sourceRun, checkpoint.fingerprint], + [CK_OLD, "ancestor", "run-1", "fp1"] + ); + + const disabled = await resolveCheckpointRange(ckArgs({ enabled: false })); + assert.deepStrictEqual(Object.keys(disabled).sort(), expected); + assert.deepStrictEqual( + [disabled.from, disabled.to, disabled.checkpointBefore, disabled.ancestry, disabled.sourceRun, disabled.fingerprint], + ["", CK_NEW, "", "", "", ""] + ); + + // A marker that was read but rejected still reports what it recorded, so the + // "why was this a full review" question is answerable from the outputs alone. + const rejected = await resolveCheckpointRange(ckArgs({ isAncestor: async () => 1 })); + assert.deepStrictEqual(Object.keys(rejected).sort(), expected); + assert.deepStrictEqual( + [rejected.mode, rejected.checkpointBefore, rejected.ancestry, rejected.from], + ["full", CK_OLD, "not_ancestor", ""] + ); +} + +// validateCheckpointPayload is the schema half of the gate; pin its verdicts +// directly so a future field addition cannot silently loosen them. +function testValidateCheckpointPayload() { + assert.strictEqual(validateCheckpointPayload(ckPayload(), { prNumber: 123 }), null); + const rejects = [ + [null, {}], + [ckPayload({ v: "1" }), { prNumber: 123 }], + [ckPayload({ pr: 124 }), { prNumber: 123 }], + [ckPayload({ head: "ABCDEF0123456789".padEnd(40, "0") }), { prNumber: 123 }], + [ckPayload({ head: CK_OLD.slice(0, 39) }), { prNumber: 123 }], + [ckPayload({ terminal_state: "failed" }), { prNumber: 123 }], + [ckPayload({ terminal_state: "skipped" }), { prNumber: 123 }], + [ckPayload({ base_ref: "" }), { prNumber: 123 }], + [ckPayload({ merge_base: "" }), { prNumber: 123 }], + [ckPayload({ fingerprint: "" }), { prNumber: 123 }], + ]; + for (const [payload, opts] of rejects) { + assert.strictEqual(typeof validateCheckpointPayload(payload, opts), "string"); + } +} + +// --------------------------------------------------------------------------- +// Cross-push checkpoints (#476) — write path (advance / carry-forward) +// --------------------------------------------------------------------------- + +const CK_RESOLVED = "5e".repeat(20); +const CARRY = buildCheckpointMarker(ckPayload({ head: CK_OLD, run: "carried" })); + +function ckManifest(over = {}) { + return Object.assign( + { terminal_state: "complete", input: { mode: "range", resolved_head: CK_RESOLVED } }, + over + ); +} + +function ckRunOptions(over = {}) { + return Object.assign( + { + checkpointEnabled: true, + checkpointCarry: "", + checkpointBaseRef: "main", + checkpointMergeBase: CK_MB, + checkpointFingerprint: "fp1", + }, + over + ); +} + +// Read back the single body this run actually wrote to the sticky summary. +function lastSummaryBody(gh) { + if (gh.updatedComments.length > 0) return gh.updatedComments[gh.updatedComments.length - 1].body; + if (gh.issueComments.length > 0) return gh.issueComments[gh.issueComments.length - 1].body; + return null; +} + +// 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. +async function testCheckpointAdvanceGateTable() { + const terminals = ["complete", "partial", "failed", "skipped", null]; + const failures = [0, 1]; + 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. + const result = { + comments: [ + failed === 1 + ? { 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 + ? { + files: [{ filename: "src/a.js", patch: "@@ -1,2 +1,2 @@\n a\n b" }], + batchErrorSpec: [{ message: "Line could not be resolved", status: 422 }], + } + : {} + ); + // published=false: the summary cannot be written at all (the issue + // comment API is down), so summaryUrl stays empty. + if (!isPublished) { + gh.rest.issues.listComments = async () => { throw makeErr("listComments unavailable", 503); }; + } + const outputs = {}; + await runPostReviewComments( + Object.assign( + { + github: gh, + context, + core: { setOutput: (k, v) => { outputs[k] = v; } }, + fs: mockFs(JSON.stringify(result), ""), + }, + ckRunOptions() + ) + ); + const body = lastSummaryBody(gh) || ""; + const advanced = body.includes("ocr-checkpoint"); + 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.summary_comment_url === "", !isPublished, `${label}: fixture publication`); + const expectAdvance = terminal === "complete" && failed === 0 && isPublished; + assert.strictEqual(advanced, expectAdvance, `${label}: marker written=${advanced}`); + if (expectAdvance) { + advancing++; + // C5: the recorded head is the manifest's resolved head, not the + // runner's HEAD_SHA (which the fixture deliberately differs on). + const payload = parseCheckpointMarker(body); + assert.strictEqual(payload.head, CK_RESOLVED); + assert.notStrictEqual(payload.head, context.payload.pull_request.head.sha); + assert.strictEqual(payload.terminal_state, "complete"); + assert.strictEqual(payload.pr, context.issue.number); + // C11: the advanced head is exported for the caller. + assert.strictEqual(outputs.checkpoint_after, CK_RESOLVED); + } else { + assert.strictEqual(outputs.checkpoint_after, "", `${label}: checkpoint_after must stay empty`); + } + } + } + } + assert.strictEqual(advancing, 1, "exactly one of the 20 cells may advance"); +} + +// A manifest whose resolved_head is not a full sha cannot identify a range, so +// it never advances however complete the run was. +async function testCheckpointAdvanceRequiresFullSha() { + for (const head of ["", "abc123", CK_RESOLVED.toUpperCase(), `${CK_RESOLVED}0`, undefined]) { + const result = { + comments: [], + manifest: ckManifest({ input: { resolved_head: head } }), + }; + const gh = makeGithub({}); + await runPostReviewComments( + Object.assign( + { github: gh, context, core: { setOutput() {} }, fs: mockFs(JSON.stringify(result), "") }, + ckRunOptions() + ) + ); + assert.strictEqual( + (lastSummaryBody(gh) || "").includes("ocr-checkpoint"), + false, + `resolved_head ${JSON.stringify(head)} must not advance the checkpoint` + ); + } +} + +// C6/K7: every body-composition path re-emits the carried marker byte for byte +// when it does not advance, so a rewritten summary never erases the checkpoint. +async function testCheckpointCarryForwardOnEveryBodyPath() { + const paths = [ + [ + "json parse failure", + { raw: "not json", stderr: "ocr blew up", manifest: null }, + ], + [ + "zero findings, incomplete run", + { raw: JSON.stringify({ comments: [], manifest: ckManifest({ terminal_state: "partial" }) }), stderr: "" }, + ], + [ + "findings, incomplete run", + { + raw: JSON.stringify({ + comments: [{ path: "src/a.js", content: "c1", start_line: 1, end_line: 1 }], + manifest: ckManifest({ terminal_state: "failed" }), + }), + stderr: "", + }, + ], + [ + "findings, no manifest at all", + { + raw: JSON.stringify({ comments: [{ path: "src/a.js", content: "c1", start_line: 1, end_line: 1 }] }), + stderr: "", + }, + ], + ]; + for (const [label, spec] of paths) { + const gh = makeGithub({}); + await runPostReviewComments( + Object.assign( + { github: gh, context, core: { setOutput() {} }, fs: mockFs(spec.raw, spec.stderr) }, + ckRunOptions({ checkpointCarry: CARRY }) + ) + ); + const body = lastSummaryBody(gh); + assert.notStrictEqual(body, null, `${label}: a summary must be written`); + assert.strictEqual(body.split(CARRY).length, 2, `${label}: the carried marker must appear exactly once`); + assert.deepStrictEqual( + parseCheckpointMarker(body), + parseCheckpointMarker(CARRY), + `${label}: the carried marker must not be re-encoded` + ); + + // With no carry, the same path writes no checkpoint at all. + const bare = makeGithub({}); + await runPostReviewComments( + Object.assign( + { github: bare, context, core: { setOutput() {} }, fs: mockFs(spec.raw, spec.stderr) }, + ckRunOptions({ checkpointCarry: "" }) + ) + ); + assert.strictEqual( + (lastSummaryBody(bare) || "").includes("ocr-checkpoint"), + false, + `${label}: no carry means no marker` + ); + } +} + +// C8: a clean run with no findings is the most common complete run there is; it +// must advance the checkpoint through the zero-findings early return. +async function testCheckpointAdvancesOnZeroFindings() { + const gh = makeGithub({}); + const outputs = {}; + await runPostReviewComments( + Object.assign( + { + github: gh, + context, + core: { setOutput: (k, v) => { outputs[k] = v; } }, + fs: mockFs(JSON.stringify({ comments: [], manifest: ckManifest() }), ""), + }, + ckRunOptions({ checkpointCarry: CARRY }) + ) + ); + const body = lastSummaryBody(gh); + const payload = parseCheckpointMarker(body); + assert.strictEqual(payload.head, CK_RESOLVED, "the zero-findings path must advance"); + assert.strictEqual(body.includes(CARRY), false, "advancing replaces the carried marker, never duplicates it"); + assert.strictEqual(outputs.checkpoint_after, CK_RESOLVED); +} + +// A non-sticky run has nowhere durable to keep a checkpoint, so it must never +// advance one even when everything else is green. +async function testCheckpointNeverAdvancesWithoutSticky() { + const gh = makeGithub({}); + await runPostReviewComments( + Object.assign( + { + github: gh, + context, + core: { setOutput() {} }, + fs: mockFs(JSON.stringify({ comments: [], manifest: ckManifest() }), ""), + stickySummary: false, + }, + ckRunOptions() + ) + ); + assert.strictEqual((lastSummaryBody(gh) || "").includes("ocr-checkpoint"), false); +} + +// The carry must be gated on exactly the same two conditions as the advance. +// A non-sticky run posts a fresh comment every time, so re-emitting a marker +// read from an older comment would plant a checkpoint on a body that never +// carried one; a run with checkpointing off must never emit a marker at all. +// Both cases are reachable from the action: the resolve step exports +// OCR_CHECKPOINT_CARRY without consulting sticky_summary. +async function testCheckpointCarryIsGatedLikeTheAdvance() { + const cases = [ + ["non-sticky", { stickySummary: false }, ckRunOptions({ checkpointCarry: CARRY })], + ["checkpointing off", {}, { checkpointEnabled: false, checkpointCarry: CARRY }], + ]; + for (const [label, runOver, ckOver] of cases) { + // An otherwise-complete run: only the gate under test can suppress a marker. + const gh = makeGithub({}); + const outputs = {}; + await runPostReviewComments( + Object.assign( + { + github: gh, + context, + core: { setOutput: (k, v) => { outputs[k] = v; } }, + fs: mockFs(JSON.stringify({ comments: [], manifest: ckManifest() }), ""), + }, + runOver, + ckOver + ) + ); + const body = lastSummaryBody(gh) || ""; + assert.strictEqual(body.includes("ocr-checkpoint"), false, `${label}: no marker may be emitted`); + assert.strictEqual(body.includes(CARRY), false, `${label}: the carry must not be re-emitted`); + assert.strictEqual(outputs.checkpoint_after, "", `${label}: nothing advanced`); + } + + // The same run WITH both gates satisfied still carries, so the assertions + // above are pinning the gate rather than a body that never carries anything. + const gh = makeGithub({}); + await runPostReviewComments( + Object.assign( + { + github: gh, + context, + core: { setOutput() {} }, + fs: mockFs(JSON.stringify({ comments: [], manifest: ckManifest({ terminal_state: "partial" }) }), ""), + }, + ckRunOptions({ checkpointCarry: CARRY }) + ) + ); + assert.strictEqual(lastSummaryBody(gh).includes(CARRY), true, "sticky + enabled still carries"); +} + +// The resolver accepts a pre-read comment so the action can list comments once +// instead of twice (the caller needs the raw marker for the carry regardless). +// The pre-read must be the observation the decision is made from — not merely +// tolerated and then re-read. +async function testCheckpointResolverUsesPreReadComment() { + let listCalls = 0; + const counting = ckGithub({ comments: [ckComment({ payload: ckPayload() })] }); + const inner = counting.rest.issues.listComments; + counting.rest.issues.listComments = async (args) => { + listCalls += 1; + return inner(args); + }; + + const read = await readCheckpointComment({ + github: counting, + owner: "owner", + repo: "repo", + prNumber: 123, + log: () => {}, + }); + assert.strictEqual(read.reason, "ok"); + assert.strictEqual(listCalls, 1); + + const range = await resolveCheckpointRange(ckArgs({ github: counting, read })); + assert.strictEqual(range.mode, "checkpoint"); + assert.strictEqual(range.from, CK_OLD); + assert.strictEqual(listCalls, 1, "the pre-read must not be followed by a second listComments"); + + // A pre-read that failed closed still decides the outcome, and still costs no + // further reads. + const rejected = await resolveCheckpointRange( + ckArgs({ github: counting, read: { reason: "author_unverified", payload: null, raw: "" } }) + ); + assert.strictEqual(rejected.mode, "full"); + assert.strictEqual(rejected.reason, "author_unverified"); + assert.strictEqual(listCalls, 1); + + // Omitting it keeps the standalone behavior: the resolver reads for itself. + const standalone = await resolveCheckpointRange(ckArgs({ github: counting })); + assert.strictEqual(standalone.mode, "checkpoint"); + assert.strictEqual(listCalls, 2); +} + +// C1/K3: with today's option set (no checkpoint options at all) nothing about +// the emitted body or the existing outputs changes. +async function testCheckpointOptOutLeavesBodyUnchanged() { + const result = { comments: [], manifest: ckManifest() }; + const outputs = {}; + const gh = makeGithub({}); + await runPostReviewComments({ + github: gh, + context, + core: { setOutput: (k, v) => { outputs[k] = v; } }, + fs: mockFs(JSON.stringify(result), ""), + }); + const body = lastSummaryBody(gh); + assert.strictEqual(body.includes("ocr-checkpoint"), false, "opt-out runs write no checkpoint"); + assert.strictEqual(body, `${SUMMARY_MARKER}\n✅ **OpenCodeReview**: No comments generated. Looks good to me.`); + assert.strictEqual(outputs.comments_total, "0"); + assert.strictEqual(outputs.checkpoint_after, ""); +} + +// The two halves must actually meet: a marker written by a real run has to be +// accepted by the resolver on the next run, and the range it yields has to start +// where the previous run stopped. Tested separately, each half can drift into a +// shape the other rejects (a field the writer omits, a value the reader rejects) +// while both suites stay green. +async function testCheckpointWriteThenReadRoundTrip() { + const gh = makeGithub({}); + await runPostReviewComments( + Object.assign( + { + github: gh, + context, + core: { setOutput() {} }, + fs: mockFs(JSON.stringify({ comments: [], manifest: ckManifest() }), ""), + }, + ckRunOptions() + ) + ); + const written = lastSummaryBody(gh); + + // Next run, same configuration and same base: the marker just written is the + // only input, and the new head descends from it. + const nextHead = "6f".repeat(20); + const range = await resolveCheckpointRange({ + github: ckGithub({ comments: [{ id: 7, user: { login: "github-actions[bot]" }, body: written }] }), + owner: "owner", + repo: "repo", + prNumber: context.issue.number, + enabled: true, + sticky: true, + fullReview: false, + headSha: nextHead, + baseRef: "main", + mergeBase: CK_MB, + fingerprint: "fp1", + isAncestor: async (a, b) => (a === CK_RESOLVED && b === nextHead ? 0 : 1), + log: () => {}, + }); + assert.strictEqual(range.mode, "checkpoint"); + assert.strictEqual(range.reason, "ok"); + assert.strictEqual(range.from, CK_RESOLVED, "the next run starts where this one stopped"); + assert.strictEqual(range.to, nextHead); + + // Same marker, one setting different: the writer's fingerprint is what the + // reader compares, so a configuration change is caught end to end. + const reconfigured = await resolveCheckpointRange({ + github: ckGithub({ comments: [{ id: 7, user: { login: "github-actions[bot]" }, body: written }] }), + owner: "owner", + repo: "repo", + prNumber: context.issue.number, + enabled: true, + sticky: true, + fullReview: false, + headSha: nextHead, + baseRef: "main", + mergeBase: CK_MB, + fingerprint: "fp2", + isAncestor: async () => 0, + log: () => {}, + }); + assert.strictEqual(reconfigured.reason, "config_changed"); + assert.strictEqual(reconfigured.mode, "full"); +} + main().catch((err) => { console.error(err); process.exit(1);