From 0dac8ac3766e71b189470ed9eea0bee12e3aedcf Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Wed, 1 Jul 2026 19:38:10 +0800 Subject: [PATCH] fix(ci): prevent duplicate review posts on retry in ocr-review workflow (#250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ci): add idempotency check to prevent duplicate review posts on retry When the batch createReview fails with a 5xx/408/network error, the request may still have landed on the server. Before retrying per-comment, the workflow now: - Tags each review/comment/summary with a per-run HTML comment ID derived from runId + runAttempt + content hash. - Queries existing reviews and review comments to detect whether the batch actually landed, and only retries the comments that are missing. - Before retrying an individual comment whose request may have reached GitHub, cools down (honoring rate-limit headers) then checks whether the comment already exists, treating it as success instead of posting a duplicate. - Skips posting the summary comment when one with the same run tag already exists. - Adds read-API retry/pacing helpers (withRetry/readWithPacing/readAllPages) with shorter spacing than writes (OCR_READ_SUCCESS_DELAY / OCR_READ_LOW_REMAINING_SPACING) since reads are cheaper but still consume the primary rate limit. Degrades gracefully to the original fallback (accepting duplicate risk) when the idempotency read calls themselves fail. * fix(ci): harden idempotency checks in ocr-review workflow Address code review findings on the GitHub Actions PR auto-review workflow (applied to both .github/workflows and examples copies): - readAllPages: cap pagination at maxPages=50 (default) to prevent unbounded loops, and validate the argument is a positive integer. - getPostedCommentIds: anchor the ID regex to the HTML comment wrapper () with a capture group to avoid false positives from user-generated content. - isCommentAlreadyPosted: return null (unknown) instead of false when the read API fails, so callers do not silently risk duplicates; accept a postedIdsCache to reuse a single paginated walk across retries. - hasIssueCommentWithId: return null (unknown) on read API failure, and match the summary tag with an anchored regex for consistency. - Call sites: handle null by skipping retry/posting to avoid duplicates while surfacing the failure in the summary. * fix(ci): validate env config and document intentional behaviors Address code review findings on the ocr-review workflow (applied to both .github/workflows and examples copies): - parseNonNegInt: add a validation helper for env-var parsing so negative or non-numeric values (e.g. OCR_MAX_RETRIES=-5) fall back to defaults instead of bypassing the `|| default` guard (a negative parseInt result is truthy). All seven retry/pacing config values now use it. - readAllPages: document that the 50-page cap is an intentional safety valve against unbounded loops, not a normal mode; callers that depend on completeness already degrade safely to null (unknown), so a truncated walk does not silently produce duplicates. - commentId: document that the 12-hex-char (48-bit) hash collision scope is a single PR (listReviewComments is PR-scoped) and a single run produces only tens to hundreds of comments, making the birthday-bound collision probability negligible (~1e-7 at 10k). * docs(github_actions): sync README with retry/idempotency features in ocr-review.yml - Add OCR_READ_SUCCESS_DELAY and OCR_READ_LOW_REMAINING_SPACING variables for read API pacing used by the idempotency check - Document the three GitHub rate-limit retry strategies (primary reset, retry-after header, secondary no-header backoff) - Add 'Idempotency: avoiding duplicate review comments' section describing how the workflow detects already-landed comments via per-run HTML tags and skips retrying when the read API is unavailable * fix(ci): use full sha256 hash for review comment idempotency IDs Drop the .slice(0, 12) truncation in commentId() and use the full 64-char (256-bit) sha256 hex digest. The truncated 12-char hash carried a tiny but nonzero collision risk whose failure mode was a silently dropped inline comment (the idempotency check would mistake two distinct comments for duplicates). The full hash makes the collision probability effectively zero with no meaningful downside; the ID regex already used [a-f0-9]+ so it accepts the longer IDs unchanged. * fix(ci): use random per-comment IDs and defer body assembly in review workflow Replace the content-derived commentId() (sha256 of path/line/content) with a random per-comment ID (crypto.randomBytes) and restructure the inline- comment flow around an item struct that carries { comment, id, lines }. This fixes two issues in the idempotency check: 1. ID was recomputed on every failure check. Each inline comment is now assigned one random ID up front and carried on the item struct, so the retry/idempotency logic reads item.id directly. The comment body (which embeds the ID) is assembled only at API-call time in toReviewPayload(), eliminating repeated hash computation. 2. Content-derived IDs collided for distinct comments sharing the same path/line/content. A random ID guarantees two such comments get different IDs, so the idempotency check no longer mistakes the second for a duplicate of the first and silently drops it on retry. formatComment/commentId are removed (no callers remain) and replaced with newCommentId/resolveLines/toReviewPayload/buildBody. The matching regex already used [a-f0-9]+ so it accepts the new random tokens unchanged. README ID-format placeholder updated from to . * docs(ci): correct misleading readAllPages truncation comment The comment claimed 'a truncated walk does not silently produce duplicates' because callers 'degrade safely by returning null on read failures.' That reasoning only holds when the read API THROWS (rate limit, 5xx): isCommentAlreadyPosted/hasIssueCommentWithId then return null (unknown) and the caller skips retrying. A truncated walk does not throw — it returns a partial set silently, so isCommentAlreadyPosted returns false (definitively 'not posted') for comments beyond the cap, and the retry loop reposts them, producing duplicates. Rewrite the comment to state the cap is an intentional safety valve and to explicitly distinguish truncation (partial data, can duplicate) from thrown read failures (null/unknown, safe). No behavior change. * fix(ci): drop stale postedIdsCache to prevent duplicate inline comments isCommentAlreadyPosted reused a single listReviewComments snapshot (postedIdsCache) across all per-comment retries. As comments landed during the loop, the snapshot went stale; a 5xx-landed comment checked against the stale snapshot would be reported as 'not posted' and retried, posting a duplicate. Remove the cache and walk fresh on every check. The extra reads are paced via readAllPages/readWithPacing (with retry honoring retry-after and x-ratelimit-reset) and degrade to null — skip retry — if the read API ultimately fails, so they cannot produce duplicates. The cache provided no real benefit in this path: checked comments are either genuine misses (correctly false) or just-landed (a fresh walk catches them), so hits essentially never occurred. --- .github/workflows/ocr-review.yml | 480 ++++++++++++++++++++++--- examples/github_actions/README.md | 18 +- examples/github_actions/ocr-review.yml | 480 ++++++++++++++++++++++--- 3 files changed, 869 insertions(+), 109 deletions(-) diff --git a/.github/workflows/ocr-review.yml b/.github/workflows/ocr-review.yml index e6bee994..23daa9d1 100644 --- a/.github/workflows/ocr-review.yml +++ b/.github/workflows/ocr-review.yml @@ -37,6 +37,18 @@ # (default: 3; GitHub best practice is to watch the header and slow down). # OCR_LOW_REMAINING_SPACING - Request spacing (ms) used when remaining quota is low # (default: 10000 = 10s). +# OCR_READ_SUCCESS_DELAY - Delay (ms) after a successful read API call (listReviews / +# listReviewComments / listIssueComments) used for the +# idempotency check. Reads are cheaper than writes, so the +# default is shorter (default: 500). +# OCR_READ_LOW_REMAINING_SPACING - Request spacing (ms) for read calls when remaining +# quota is low (default: 5000 = 5s). +# +# Idempotency: +# When the batch createReview fails with a 5xx, the request may still have landed on +# the server. Before retrying per-comment, the workflow queries existing reviews and +# review comments (tagged with a per-run HTML comment) and only retries the comments +# that are actually missing. This prevents duplicate review posts. # # Note: GITHUB_TOKEN is automatically provided by GitHub Actions. # Note: The workflow also configures llm.extra_body to '{"thinking": {"type": "disabled"}}' @@ -116,8 +128,22 @@ jobs: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); + const crypto = require('crypto'); const path = '/tmp/ocr-result.json'; + // Unique tag for this workflow run + attempt. Embedded in review/comment + // bodies as an HTML comment so the idempotency check can detect whether + // a batch createReview actually landed on the server before retrying. + // context.runId / context.runAttempt are numbers from @actions/github's + // Context (parsed from GITHUB_RUN_ID / GITHUB_RUN_ATTEMPT). Use + // Number.isFinite to guard against NaN when the env vars are missing, + // falling back to safe defaults. + const runId = Number.isFinite(context.runId) ? context.runId : 0; + const runAttempt = Number.isFinite(context.runAttempt) ? context.runAttempt : 1; + const RUN_TAG = `${runId}-${runAttempt}`; + const REVIEW_TAG = ``; + const SUMMARY_TAG = ``; + // Read OCR output let result; try { @@ -163,35 +189,23 @@ jobs: const commentsWithoutLine = []; for (const comment of comments) { - const body = formatComment(comment); - // Check if comment has valid line information for inline comment (line >= 1) const hasValidLine = (comment.start_line >= 1) || (comment.end_line >= 1); if (!hasValidLine) { - commentsWithoutLine.push({ comment, body }); + commentsWithoutLine.push({ comment }); continue; } - const reviewComment = { - path: comment.path, - body: body - }; - - // Use line range if available - if (comment.start_line >= 1 && comment.end_line >= 1 && comment.start_line !== comment.end_line) { - reviewComment.start_line = comment.start_line; - reviewComment.line = comment.end_line; - reviewComment.start_side = 'RIGHT'; - reviewComment.side = 'RIGHT'; - } else if (comment.end_line >= 1) { - reviewComment.line = comment.end_line; - reviewComment.side = 'RIGHT'; - } else if (comment.start_line >= 1) { - reviewComment.line = comment.start_line; - reviewComment.side = 'RIGHT'; - } - - reviewComments.push({ comment, reviewComment }); + // Each inline comment becomes an item carrying a random ID + // (assigned once) and its resolved line targeting. The body is + // built from item.id only at API-call time (see toReviewPayload), + // so retry/idempotency logic reads item.id directly instead of + // recomputing it, and distinct comments never share an ID. + reviewComments.push({ + comment, + id: newCommentId(), + lines: resolveLines(comment) + }); } // Submit as a single PR review with all comments @@ -203,11 +217,33 @@ jobs: // Add comments without line info to summary body summaryBody += formatSummaryComments(commentsWithoutLine); + // Prepend the run tag so the idempotency check can detect whether the + // batch review actually landed on the server before retrying. + summaryBody = REVIEW_TAG + '\n' + summaryBody; + // Statistics tracking let successCount = 0; let failedCount = 0; const failedComments = []; + // Retry/pacing configuration (shared by write and read API calls). + // parseNonNegInt guards against nonsensical env values (negative, + // NaN, non-numeric) that `parseInt(...) || default` would let + // through for negative numbers, since a negative parseInt result + // is truthy and would bypass the `|| default` fallback. + function parseNonNegInt(val, defaultVal) { + const n = parseInt(val, 10); + return Number.isFinite(n) && n >= 0 ? n : defaultVal; + } + const MAX_RETRIES = parseNonNegInt(process.env.OCR_MAX_RETRIES, 3); + const SUCCESS_DELAY = parseNonNegInt(process.env.OCR_SUCCESS_DELAY, 2000); // delay after successful write + const FAILURE_DELAY = parseNonNegInt(process.env.OCR_FAILURE_DELAY, 1000); // delay after non-retryable failure + const LOW_REMAINING_THRESHOLD = parseNonNegInt(process.env.OCR_LOW_REMAINING_THRESHOLD, 3); + const LOW_REMAINING_SPACING = parseNonNegInt(process.env.OCR_LOW_REMAINING_SPACING, 10000); + // Read APIs are cheaper and have higher thresholds; use shorter pacing. + const READ_SUCCESS_DELAY = parseNonNegInt(process.env.OCR_READ_SUCCESS_DELAY, 500); + const READ_LOW_REMAINING_SPACING = parseNonNegInt(process.env.OCR_READ_LOW_REMAINING_SPACING, 5000); + try { const batchRes = await github.rest.pulls.createReview({ owner: context.repo.owner, @@ -216,24 +252,51 @@ jobs: commit_id: commitSha, body: summaryBody, event: 'COMMENT', - comments: reviewComments.map(({ reviewComment }) => reviewComment) + comments: reviewComments.map(toReviewPayload) }); successCount = reviewComments.length; console.log(`Successfully posted review with ${successCount} inline comments (${commentsWithoutLine.length} in summary)`); logRateLimitQuota(batchRes, 'after batch createReview'); } catch (e) { console.log('Failed to post review with inline comments:', e.message); - console.log('Falling back to posting comments individually with rate-limit-aware retry...'); - - // Fallback: post comments one by one with delay to avoid secondary rate limits. - // GitHub enforces ~80 content-generating requests per minute; spacing calls - // helps stay under that threshold. Retry/wait durations are derived from the - // rate-limit response headers per GitHub's documented strategy. - const MAX_RETRIES = parseInt(process.env.OCR_MAX_RETRIES, 10) || 3; - const SUCCESS_DELAY = parseInt(process.env.OCR_SUCCESS_DELAY, 10) || 2000; // delay after successful post - const FAILURE_DELAY = parseInt(process.env.OCR_FAILURE_DELAY, 10) || 1000; // delay after non-retryable failure - const LOW_REMAINING_THRESHOLD = parseInt(process.env.OCR_LOW_REMAINING_THRESHOLD, 10) || 3; - const LOW_REMAINING_SPACING = parseInt(process.env.OCR_LOW_REMAINING_SPACING, 10) || 10000; + console.log('Checking whether the batch review actually landed on the server before retrying...'); + + // Idempotency check: the batch createReview may have succeeded on the + // server even though we got a 5xx. Query existing reviews to find out, + // so we only retry the comments that are actually missing. + let existingReview = null; + try { + existingReview = await findExistingBatchReview({ + owner: context.repo.owner, + repo: context.repo.repo, + prNumber, + tag: REVIEW_TAG + }); + } catch (checkErr) { + console.log(`Idempotency check failed (${checkErr.message}). ` + + `Degrading to original fallback (accepting duplicate risk).`); + } + + // Compute the list of inline comments that still need to be posted. + // If the batch review landed, only retry the missing ones; otherwise + // retry all of them. + let toRetry = reviewComments; + if (existingReview && existingReview.found) { + const postedIds = await getPostedCommentIds({ + owner: context.repo.owner, + repo: context.repo.repo, + prNumber + }); + toRetry = reviewComments.filter((item) => + !postedIds.has(item.id) + ); + successCount = reviewComments.length - toRetry.length; + console.log(`Batch review already exists (review_id=${existingReview.review.id}). ` + + `${successCount}/${reviewComments.length} inline comments already posted. ` + + `${toRetry.length} missing, will retry only those.`); + } else { + console.log('Batch review not found on server. Falling back to per-comment posting...'); + } // If the batch itself was rate-limited, honor its rate-limit headers // (retry-after / x-ratelimit-reset) before retrying per-comment, @@ -248,7 +311,8 @@ jobs: await sleep(batchRetry.delayMs); } - for (const { comment, reviewComment } of reviewComments) { + for (const item of toRetry) { + const { comment, id } = item; let posted = false; for (let attempt = 0; attempt <= MAX_RETRIES && !posted; attempt++) { try { @@ -259,14 +323,14 @@ jobs: commit_id: commitSha, body: '', event: 'COMMENT', - comments: [reviewComment] + comments: [toReviewPayload(item)] }); successCount++; posted = true; - console.log(`Successfully posted comment for ${reviewComment.path}`); + console.log(`Successfully posted comment for ${comment.path}`); // Proactive throttle: if remaining quota is low, slow down to // avoid hitting the limit (GitHub best practice: watch the header). - const remaining = logRateLimitQuota(res, `after ${reviewComment.path}`); + const remaining = logRateLimitQuota(res, `after ${comment.path}`); const lowQuota = remaining != null && remaining <= LOW_REMAINING_THRESHOLD; if (lowQuota) { console.log(`[rate-limit] quota low (remaining=${remaining} <= ${LOW_REMAINING_THRESHOLD}); increasing spacing to ${LOW_REMAINING_SPACING}ms.`); @@ -279,23 +343,94 @@ jobs: // rate-limit documentation (retry-after / x-ratelimit-* headers). const retryInfo = computeRetryDelayMs(innerE, attempt); const willRetry = retryInfo != null && attempt < MAX_RETRIES; - if (willRetry) { + // Any error whose request may have reached GitHub (5xx server + // errors, 408 timeout, or network-layer errors with no status) + // can mean the comment was actually created but the response was + // lost. Before retrying (which would post a duplicate) or before + // giving up (which would wrongly list it as failed in the summary), + // we must check whether it already landed. + // + // IMPORTANT: do the check AFTER cooling down, not immediately. + // If the error is rate-limit-related (5xx under load, or a + // network blip), firing read requests right away further + // pressures the already-struggling API. Honor the computed + // retry delay first, then query. + const status = innerE.status; + const maybeReachedServer = + (typeof status === 'number' && (status >= 500 || status === 408)) || + status == null; // network errors (ECONNRESET, ETIMEDOUT, ...) + if (maybeReachedServer) { + // Cool down first: even read requests count against rate + // limits, and querying during an ongoing 5xx/rate-limit + // episode can worsen the situation. Use the retry delay when + // available; for non-retryable errors (retryInfo == null) + // there is no header-derived wait, so use a short fixed cool + // down before the read. + const coolDownMs = retryInfo != null ? retryInfo.delayMs : FAILURE_DELAY; + if (coolDownMs > 0) { + const secs = (coolDownMs / 1000).toFixed(1); + console.log( + `Cooling down ${secs}s before idempotency check for ${comment.path} ` + + `(HTTP ${innerE.status || 'n/a'}, attempt ${attempt + 1}/${MAX_RETRIES + 1}).` + ); + await sleep(coolDownMs); + } + const alreadyPosted = await isCommentAlreadyPosted({ + owner: context.repo.owner, + repo: context.repo.repo, + prNumber, + id + }); + if (alreadyPosted === true) { + successCount++; + posted = true; + console.log(`Comment for ${comment.path} already posted (id=${id}); treating as success.`); + await sleep(SUCCESS_DELAY); + continue; + } + // Unknown (null): the read API is unavailable, so we + // cannot tell whether the comment landed. To avoid a + // duplicate, do NOT retry posting; record as failed so + // the summary surfaces the uncertainty rather than + // silently risking a duplicate. + if (alreadyPosted === null) { + failedCount++; + const reason = 'idempotency check unavailable (read API failed)'; + failedComments.push({ comment, error: `${innerE.message} [${reason}]` }); + console.log(`Cannot verify whether comment for ${comment.path} was posted (${reason}, HTTP ${innerE.status || 'n/a'}); skipping retry to avoid duplicate.`); + await sleep(SUCCESS_DELAY); + break; + } + // Not found on server. If retries are exhausted or the + // error is non-retryable, this is a real failure. + if (!willRetry) { + failedCount++; + failedComments.push({ comment, error: innerE.message }); + const reason = retryInfo == null ? 'non-retryable error' : 'rate-limit retries exhausted'; + console.log(`Failed to post comment for ${comment.path} (${reason}, HTTP ${innerE.status || 'n/a'}): ${innerE.message}`); + await sleep(SUCCESS_DELAY); + break; + } + // willRetry: cool down already consumed above, loop back. + } else if (willRetry) { + // Pure 429/403 rate-limit: the request never reached the + // server, so no duplicate is possible and the idempotency + // check can be skipped. Just honor the retry delay. const secs = (retryInfo.delayMs / 1000).toFixed(1); console.log( - `Rate-limited/transient error on ${reviewComment.path} ` + + `Rate-limited on ${comment.path} ` + `(HTTP ${innerE.status}, attempt ${attempt + 1}/${MAX_RETRIES}). ` + `Waiting ${secs}s via '${retryInfo.source}' (${retryInfo.detail}). ` + `Error: ${innerE.message}` ); await sleep(retryInfo.delayMs); } else { + // Non-retryable error that definitely did not reach the + // server (e.g. 4xx validation error): record as failed. failedCount++; failedComments.push({ comment, error: innerE.message }); - const reason = retryInfo == null ? 'non-retryable error' : 'rate-limit retries exhausted'; - console.log(`Failed to post comment for ${reviewComment.path} (${reason}, HTTP ${innerE.status || 'n/a'}): ${innerE.message}`); - // After exhausting retries use the success-style pace delay; - // for other errors use the shorter failure pace delay. - await sleep(retryInfo == null ? FAILURE_DELAY : SUCCESS_DELAY); + console.log(`Failed to post comment for ${comment.path} (non-retryable error, HTTP ${innerE.status || 'n/a'}): ${innerE.message}`); + await sleep(FAILURE_DELAY); break; } } @@ -319,19 +454,216 @@ jobs: finalBody += formatCommentMarkdown(comment, error); } } - - await github.rest.issues.createComment({ + + // Prepend the summary tag and post only if no summary with this tag + // already exists (idempotency: the batch review may have carried the + // same summary body, in which case we must not duplicate it). + finalBody = SUMMARY_TAG + '\n' + finalBody; + const summaryAlreadyPosted = await hasIssueCommentWithId({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: prNumber, - body: finalBody + issueNumber: prNumber, + id: SUMMARY_TAG }); + if (summaryAlreadyPosted === true) { + console.log('Summary comment with this run tag already exists; skipping.'); + } else if (summaryAlreadyPosted === null) { + // Read API unavailable: cannot tell whether the summary already + // landed. Skip posting to avoid a duplicate; the review content + // is still available via inline comments / batch review. + console.log('Cannot verify whether summary comment already exists (read API failed); skipping to avoid duplicate.'); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: finalBody + }); + } } function sleep(ms) { return new Promise(resolve => setTimeout(resolve, ms)); } + // Retry wrapper shared by write and read API calls. Reuses + // computeRetryDelayMs so rate-limit headers (retry-after / + // x-ratelimit-*) are honored uniformly. Throws on final failure + // so the caller can decide how to degrade. + async function withRetry(tag, fn) { + for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + try { + return await fn(); + } catch (e) { + const retryInfo = computeRetryDelayMs(e, attempt); + const willRetry = retryInfo != null && attempt < MAX_RETRIES; + if (willRetry) { + const secs = (retryInfo.delayMs / 1000).toFixed(1); + console.log( + `[${tag}] transient/rate-limited (HTTP ${e.status}, attempt ${attempt + 1}/${MAX_RETRIES}). ` + + `Waiting ${secs}s via '${retryInfo.source}' (${retryInfo.detail}). ${e.message}` + ); + await sleep(retryInfo.delayMs); + } else { + console.log(`[${tag}] failed after ${attempt + 1} attempts: ${e.message}`); + throw e; + } + } + } + } + + // Read API wrapper with retry + proactive pacing. Read requests are + // cheaper than writes but still consume the primary rate limit and can + // trigger the secondary limit when issued in a tight loop. Use shorter + // delays than writes (READ_SUCCESS_DELAY / READ_LOW_REMAINING_SPACING). + async function readWithPacing(tag, fn) { + const res = await withRetry(tag, fn); + const remaining = logRateLimitQuota(res, tag); + const lowQuota = remaining != null && remaining <= LOW_REMAINING_THRESHOLD; + if (lowQuota) { + console.log(`[rate-limit] quota low after read (${remaining} <= ${LOW_REMAINING_THRESHOLD}); spacing ${READ_LOW_REMAINING_SPACING}ms.`); + await sleep(READ_LOW_REMAINING_SPACING); + } else { + await sleep(READ_SUCCESS_DELAY); + } + return res; + } + + // Paginated helper that walks all pages of a list endpoint with retry + // and pacing. Returns the concatenated array of items. + async function readAllPages(tag, pageFn, maxPages = 50) { + if (!Number.isFinite(maxPages) || maxPages < 1) { + throw new Error(`readAllPages: maxPages must be a positive integer, got ${maxPages}`); + } + const all = []; + let page = 1; + const PER_PAGE = 100; + while (page <= maxPages) { + const res = await readWithPacing(`${tag} (page ${page})`, () => pageFn(page, PER_PAGE)); + const items = res.data || []; + all.push(...items); + if (items.length < PER_PAGE) break; + page++; + } + // NOTE: Truncation here is intentional and acts as a safety + // valve against unbounded loops (e.g. a bug or malicious + // activity), not as a normal operating mode. A PR accumulating + // >5000 review comments is far outside expected usage; in that + // rare case we log a warning and proceed with partial data + // rather than failing the whole review. + // + // Caveat: this is NOT the same as a read failure. When the read + // API throws (rate limit, 5xx), isCommentAlreadyPosted and + // hasIssueCommentWithId catch it and return null (unknown), so + // the caller skips retrying and creates no duplicate. A + // truncated walk does not throw; it returns a partial set + // silently, so isCommentAlreadyPosted returns false (definitively + // "not posted") for any comment beyond the cap, and the retry + // loop will repost it, producing a duplicate. This tradeoff is + // accepted because the trigger is far outside expected usage; if + // that ceiling ever needs to rise, make maxPages configurable. + if (page > maxPages) { + console.log(`[${tag}] reached max page limit (${maxPages}); results may be incomplete.`); + } + return all; + } + + // Idempotency check: find whether a batch review with this run tag + // already exists on the PR. Returns { found, review } or throws on + // final failure (caller degrades to original fallback). + async function findExistingBatchReview({ owner, repo, prNumber, tag }) { + const reviews = await readAllPages('listReviews', (page, per_page) => + github.rest.pulls.listReviews({ owner, repo, pull_number: prNumber, per_page, page }) + ); + for (const r of reviews) { + if ((r.body || '').includes(tag)) { + return { found: true, review: r }; + } + } + return { found: false }; + } + + // Collect the set of comment-level IDs already posted on the PR + // (across all reviews). Uses listReviewComments (PR-level, cross-review) + // so a single paginated walk covers everything, avoiding the O(missing) + // amplification of per-comment lookups. + async function getPostedCommentIds({ owner, repo, prNumber }) { + const comments = await readAllPages('listReviewComments', (page, per_page) => + github.rest.pulls.listReviewComments({ owner, repo, pull_number: prNumber, per_page, page }) + ); + const ids = new Set(); + // Anchor the regex to the HTML comment wrapper () + // so user-generated content or code suggestions cannot trigger + // false positives in the idempotency check. The ID format is + // `ocr--` where RUN_TAG is `-` + // and is a per-comment random hex token. Capture group 1 + // holds the bare ID (ocr--), so we can add it + // directly without stripping comment markers. + const ID_RE = //g; + for (const c of comments) { + const body = c.body || ''; + let m; + while ((m = ID_RE.exec(body)) !== null) { + ids.add(m[1]); + } + } + return ids; + } + + // Check whether a specific comment-level ID has already landed on the + // server. Used by the per-comment retry loop: when a createReview call + // fails with a transient 5xx/408, the request may have reached GitHub + // and succeeded even though the response was lost. Querying before + // retrying prevents posting a duplicate inline comment. + // Returns true/false when the check succeeds, or null when the + // read API is unavailable (rate limit, 5xx, etc.). Returning null + // (rather than defaulting to false) prevents the caller from + // assuming the comment was not posted and risking a duplicate on + // retry. + // + // Each call walks listReviewComments fresh — no cached snapshot. + // A snapshot reused across retries would go stale as comments land + // during the loop, and a stale miss for a 5xx-landed comment would + // trigger a retry that posts a duplicate. Read calls are paced via + // readAllPages/readWithPacing and degrade to null (skip retry) if the + // read API itself fails, so the extra walks cannot produce duplicates. + async function isCommentAlreadyPosted({ owner, repo, prNumber, id }) { + try { + const posted = await getPostedCommentIds({ owner, repo, prNumber }); + return posted.has(id); + } catch (e) { + console.log(`[isCommentAlreadyPosted] check failed for ${id} (${e.message}); treating as unknown to avoid duplicates.`); + return null; + } + } + + // Check whether an issue comment with the given tag already exists. + // Used to avoid posting a duplicate summary comment when the batch + // review already carried the same summary body. + // Returns true/false when the check succeeds, or null when the + // read API is unavailable. Returning null (rather than defaulting + // to false) lets the caller decide whether to skip posting or + // degrade gracefully, instead of silently risking a duplicate + // summary comment. + async function hasIssueCommentWithId({ owner, repo, issueNumber, id }) { + try { + const comments = await readAllPages('listIssueComments', (page, per_page) => + github.rest.issues.listComments({ owner, repo, issue_number: issueNumber, per_page, page }) + ); + // Match the tag anchored to its HTML comment wrapper for + // consistency with getPostedCommentIds and to defend against + // user content that happens to contain the bare tag string. + // `id` is an opaque tag like ``, + // so escape any regex metacharacters before embedding it. + const escaped = id.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const tagRe = new RegExp(''); + return comments.some(c => tagRe.test(c.body || '')); + } catch (e) { + console.log(`[listIssueComments] check failed (${e.message}); treating as unknown to avoid duplicates.`); + return null; + } + } + // Case-insensitive header lookup. Octokit normalizes response headers to // lowercase, but this defensive check also handles original casing so that // quota logging and retry delay computation never silently miss a header. @@ -437,15 +769,55 @@ jobs: } catch (_) { return null; } } - function formatComment(comment) { - let body = comment.content || ''; + // Random per-comment ID, assigned once when the inline-comment item + // is built and carried on the item struct. Random (rather than + // content-derived) so two distinct comments that share the same + // path/line/content still get different IDs and the idempotency + // check never mistakes one for the other (which would silently drop + // the second). Embedded in the comment body as an HTML comment so + // getPostedCommentIds can match it back on retry. + function newCommentId() { + return `ocr-${RUN_TAG}-${crypto.randomBytes(8).toString('hex')}`; + } - // Add code suggestion if available + // Resolve the line-targeting fields for a createReview comment + // payload (start_line/line/start_side/side) from the comment's line + // range. Returned object is spread into the payload in toReviewPayload. + function resolveLines(comment) { + const start = comment.start_line; + const end = comment.end_line; + if (start >= 1 && end >= 1 && start !== end) { + return { start_line: start, line: end, start_side: 'RIGHT', side: 'RIGHT' }; + } else if (end >= 1) { + return { line: end, side: 'RIGHT' }; + } else if (start >= 1) { + return { line: start, side: 'RIGHT' }; + } + return {}; + } + + // Build the createReview payload for an inline-comment item. The + // body is assembled here (at call time) from the item's precomputed + // ID, so retry/idempotency logic works directly off item.id instead + // of recomputing an ID each time it needs to check posting status. + function toReviewPayload(item) { + return { + path: item.comment.path, + body: buildBody(item.comment, item.id), + ...item.lines + }; + } + + // Assemble the visible comment body: the per-comment ID tag (HTML + // comment, invisible when rendered) prepended for idempotency + // matching, plus the code suggestion block if present. + function buildBody(comment, id) { + let body = `\n`; + body += comment.content || ''; if (comment.suggestion_code && comment.existing_code) { body += '\n\n**Suggestion:**\n'; body += fencedBlock(comment.suggestion_code, 'suggestion'); } - return body; } diff --git a/examples/github_actions/README.md b/examples/github_actions/README.md index dccf654b..468c6013 100644 --- a/examples/github_actions/README.md +++ b/examples/github_actions/README.md @@ -93,7 +93,13 @@ Use the `--rule` flag to pass a custom rules JSON file: ### Adjust retry and delay settings -When posting review comments individually (fallback mode), the workflow includes rate-limit handling 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. You can configure the retry and delay behavior via **repository variables** (Settings → Secrets and variables → Actions → Variables): +When posting review comments individually (fallback mode), the workflow includes rate-limit handling 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: + +- **Primary rate limit exhausted** (`x-ratelimit-remaining=0`): wait until `x-ratelimit-reset`. +- **Secondary rate limit with a `retry-after` header**: wait exactly that long. +- **Secondary rate limit with no header**: wait at least one minute, then use exponential backoff on continued failures. + +You can configure the retry and delay behavior via **repository variables** (Settings → Secrets and variables → Actions → Variables): | Variable | Default | Description | |----------|---------|-------------| @@ -104,9 +110,19 @@ When posting review comments individually (fallback mode), the workflow includes | `OCR_FAILURE_DELAY` | `1000` | Delay (ms) after a non-rate-limit failure to pace subsequent requests | | `OCR_LOW_REMAINING_THRESHOLD` | `3` | When x-ratelimit-remaining is at or below this value, proactively increase request spacing to avoid hitting the limit | | `OCR_LOW_REMAINING_SPACING` | `10000` | Request spacing (ms) used when remaining quota is low | +| `OCR_READ_SUCCESS_DELAY` | `500` | Delay (ms) after a successful read API call (`listReviews` / `listReviewComments` / `listIssueComments`) used for the idempotency check. Reads are cheaper than writes, so the default is shorter | +| `OCR_READ_LOW_REMAINING_SPACING` | `5000` | Request spacing (ms) for read calls when remaining quota is low | These variables are optional — if not configured, sensible defaults are used. Consider increasing delays for repositories with many concurrent workflows or large PRs that generate numerous review comments. +#### Idempotency: avoiding duplicate review comments + +When the batch `createReview` call fails with a `5xx` error, the request may still have landed on the GitHub server (the response was simply lost). Before retrying per-comment, the workflow queries existing reviews and review comments — each tagged with a per-run HTML comment (e.g. ``) — and only retries the comments that are actually missing. This prevents duplicate review posts. + +The same idempotency check is applied to the summary comment: before posting, the workflow verifies whether a summary with the same run tag already exists, and skips posting if so. + +If the read API itself is unavailable (rate-limited or `5xx`), the check returns *unknown* rather than assuming the comment was not posted. In that case the workflow **skips retrying** to avoid risking a duplicate, and surfaces the uncertainty in the summary instead of silently producing duplicates. + ### Limit concurrency Adjust the `--concurrency` flag for large PRs to control the number of concurrent LLM requests: diff --git a/examples/github_actions/ocr-review.yml b/examples/github_actions/ocr-review.yml index 360eb2e7..2073ae15 100644 --- a/examples/github_actions/ocr-review.yml +++ b/examples/github_actions/ocr-review.yml @@ -37,6 +37,18 @@ # (default: 3; GitHub best practice is to watch the header and slow down). # OCR_LOW_REMAINING_SPACING - Request spacing (ms) used when remaining quota is low # (default: 10000 = 10s). +# OCR_READ_SUCCESS_DELAY - Delay (ms) after a successful read API call (listReviews / +# listReviewComments / listIssueComments) used for the +# idempotency check. Reads are cheaper than writes, so the +# default is shorter (default: 500). +# OCR_READ_LOW_REMAINING_SPACING - Request spacing (ms) for read calls when remaining +# quota is low (default: 5000 = 5s). +# +# Idempotency: +# When the batch createReview fails with a 5xx, the request may still have landed on +# the server. Before retrying per-comment, the workflow queries existing reviews and +# review comments (tagged with a per-run HTML comment) and only retries the comments +# that are actually missing. This prevents duplicate review posts. # # Note: GITHUB_TOKEN is automatically provided by GitHub Actions. # Note: The workflow also configures llm.extra_body to '{"thinking": {"type": "disabled"}}' @@ -149,8 +161,22 @@ jobs: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); + const crypto = require('crypto'); const path = '/tmp/ocr-result.json'; + // Unique tag for this workflow run + attempt. Embedded in review/comment + // bodies as an HTML comment so the idempotency check can detect whether + // a batch createReview actually landed on the server before retrying. + // context.runId / context.runAttempt are numbers from @actions/github's + // Context (parsed from GITHUB_RUN_ID / GITHUB_RUN_ATTEMPT). Use + // Number.isFinite to guard against NaN when the env vars are missing, + // falling back to safe defaults. + const runId = Number.isFinite(context.runId) ? context.runId : 0; + const runAttempt = Number.isFinite(context.runAttempt) ? context.runAttempt : 1; + const RUN_TAG = `${runId}-${runAttempt}`; + const REVIEW_TAG = ``; + const SUMMARY_TAG = ``; + // Read OCR output let result; try { @@ -209,35 +235,23 @@ jobs: const commentsWithoutLine = []; for (const comment of comments) { - const body = formatComment(comment); - // Check if comment has valid line information for inline comment (line >= 1) const hasValidLine = (comment.start_line >= 1) || (comment.end_line >= 1); if (!hasValidLine) { - commentsWithoutLine.push({ comment, body }); + commentsWithoutLine.push({ comment }); continue; } - const reviewComment = { - path: comment.path, - body: body - }; - - // Use line range if available - if (comment.start_line >= 1 && comment.end_line >= 1 && comment.start_line !== comment.end_line) { - reviewComment.start_line = comment.start_line; - reviewComment.line = comment.end_line; - reviewComment.start_side = 'RIGHT'; - reviewComment.side = 'RIGHT'; - } else if (comment.end_line >= 1) { - reviewComment.line = comment.end_line; - reviewComment.side = 'RIGHT'; - } else if (comment.start_line >= 1) { - reviewComment.line = comment.start_line; - reviewComment.side = 'RIGHT'; - } - - reviewComments.push({ comment, reviewComment }); + // Each inline comment becomes an item carrying a random ID + // (assigned once) and its resolved line targeting. The body is + // built from item.id only at API-call time (see toReviewPayload), + // so retry/idempotency logic reads item.id directly instead of + // recomputing it, and distinct comments never share an ID. + reviewComments.push({ + comment, + id: newCommentId(), + lines: resolveLines(comment) + }); } // Submit as a single PR review with all comments @@ -249,11 +263,33 @@ jobs: // Add comments without line info to summary body summaryBody += formatSummaryComments(commentsWithoutLine); + // Prepend the run tag so the idempotency check can detect whether the + // batch review actually landed on the server before retrying. + summaryBody = REVIEW_TAG + '\n' + summaryBody; + // Statistics tracking let successCount = 0; let failedCount = 0; const failedComments = []; + // Retry/pacing configuration (shared by write and read API calls). + // parseNonNegInt guards against nonsensical env values (negative, + // NaN, non-numeric) that `parseInt(...) || default` would let + // through for negative numbers, since a negative parseInt result + // is truthy and would bypass the `|| default` fallback. + function parseNonNegInt(val, defaultVal) { + const n = parseInt(val, 10); + return Number.isFinite(n) && n >= 0 ? n : defaultVal; + } + const MAX_RETRIES = parseNonNegInt(process.env.OCR_MAX_RETRIES, 3); + const SUCCESS_DELAY = parseNonNegInt(process.env.OCR_SUCCESS_DELAY, 2000); // delay after successful write + const FAILURE_DELAY = parseNonNegInt(process.env.OCR_FAILURE_DELAY, 1000); // delay after non-retryable failure + const LOW_REMAINING_THRESHOLD = parseNonNegInt(process.env.OCR_LOW_REMAINING_THRESHOLD, 3); + const LOW_REMAINING_SPACING = parseNonNegInt(process.env.OCR_LOW_REMAINING_SPACING, 10000); + // Read APIs are cheaper and have higher thresholds; use shorter pacing. + const READ_SUCCESS_DELAY = parseNonNegInt(process.env.OCR_READ_SUCCESS_DELAY, 500); + const READ_LOW_REMAINING_SPACING = parseNonNegInt(process.env.OCR_READ_LOW_REMAINING_SPACING, 5000); + try { const batchRes = await github.rest.pulls.createReview({ owner: context.repo.owner, @@ -262,24 +298,51 @@ jobs: commit_id: commitSha, body: summaryBody, event: 'COMMENT', - comments: reviewComments.map(({ reviewComment }) => reviewComment) + comments: reviewComments.map(toReviewPayload) }); successCount = reviewComments.length; console.log(`Successfully posted review with ${successCount} inline comments (${commentsWithoutLine.length} in summary)`); logRateLimitQuota(batchRes, 'after batch createReview'); } catch (e) { console.log('Failed to post review with inline comments:', e.message); - console.log('Falling back to posting comments individually with rate-limit-aware retry...'); - - // Fallback: post comments one by one with delay to avoid secondary rate limits. - // GitHub enforces ~80 content-generating requests per minute; spacing calls - // helps stay under that threshold. Retry/wait durations are derived from the - // rate-limit response headers per GitHub's documented strategy. - const MAX_RETRIES = parseInt(process.env.OCR_MAX_RETRIES, 10) || 3; - const SUCCESS_DELAY = parseInt(process.env.OCR_SUCCESS_DELAY, 10) || 2000; // delay after successful post - const FAILURE_DELAY = parseInt(process.env.OCR_FAILURE_DELAY, 10) || 1000; // delay after non-retryable failure - const LOW_REMAINING_THRESHOLD = parseInt(process.env.OCR_LOW_REMAINING_THRESHOLD, 10) || 3; - const LOW_REMAINING_SPACING = parseInt(process.env.OCR_LOW_REMAINING_SPACING, 10) || 10000; + console.log('Checking whether the batch review actually landed on the server before retrying...'); + + // Idempotency check: the batch createReview may have succeeded on the + // server even though we got a 5xx. Query existing reviews to find out, + // so we only retry the comments that are actually missing. + let existingReview = null; + try { + existingReview = await findExistingBatchReview({ + owner: context.repo.owner, + repo: context.repo.repo, + prNumber, + tag: REVIEW_TAG + }); + } catch (checkErr) { + console.log(`Idempotency check failed (${checkErr.message}). ` + + `Degrading to original fallback (accepting duplicate risk).`); + } + + // Compute the list of inline comments that still need to be posted. + // If the batch review landed, only retry the missing ones; otherwise + // retry all of them. + let toRetry = reviewComments; + if (existingReview && existingReview.found) { + const postedIds = await getPostedCommentIds({ + owner: context.repo.owner, + repo: context.repo.repo, + prNumber + }); + toRetry = reviewComments.filter((item) => + !postedIds.has(item.id) + ); + successCount = reviewComments.length - toRetry.length; + console.log(`Batch review already exists (review_id=${existingReview.review.id}). ` + + `${successCount}/${reviewComments.length} inline comments already posted. ` + + `${toRetry.length} missing, will retry only those.`); + } else { + console.log('Batch review not found on server. Falling back to per-comment posting...'); + } // If the batch itself was rate-limited, honor its rate-limit headers // (retry-after / x-ratelimit-reset) before retrying per-comment, @@ -294,7 +357,8 @@ jobs: await sleep(batchRetry.delayMs); } - for (const { comment, reviewComment } of reviewComments) { + for (const item of toRetry) { + const { comment, id } = item; let posted = false; for (let attempt = 0; attempt <= MAX_RETRIES && !posted; attempt++) { try { @@ -305,14 +369,14 @@ jobs: commit_id: commitSha, body: '', event: 'COMMENT', - comments: [reviewComment] + comments: [toReviewPayload(item)] }); successCount++; posted = true; - console.log(`Successfully posted comment for ${reviewComment.path}`); + console.log(`Successfully posted comment for ${comment.path}`); // Proactive throttle: if remaining quota is low, slow down to // avoid hitting the limit (GitHub best practice: watch the header). - const remaining = logRateLimitQuota(res, `after ${reviewComment.path}`); + const remaining = logRateLimitQuota(res, `after ${comment.path}`); const lowQuota = remaining != null && remaining <= LOW_REMAINING_THRESHOLD; if (lowQuota) { console.log(`[rate-limit] quota low (remaining=${remaining} <= ${LOW_REMAINING_THRESHOLD}); increasing spacing to ${LOW_REMAINING_SPACING}ms.`); @@ -325,23 +389,94 @@ jobs: // rate-limit documentation (retry-after / x-ratelimit-* headers). const retryInfo = computeRetryDelayMs(innerE, attempt); const willRetry = retryInfo != null && attempt < MAX_RETRIES; - if (willRetry) { + // Any error whose request may have reached GitHub (5xx server + // errors, 408 timeout, or network-layer errors with no status) + // can mean the comment was actually created but the response was + // lost. Before retrying (which would post a duplicate) or before + // giving up (which would wrongly list it as failed in the summary), + // we must check whether it already landed. + // + // IMPORTANT: do the check AFTER cooling down, not immediately. + // If the error is rate-limit-related (5xx under load, or a + // network blip), firing read requests right away further + // pressures the already-struggling API. Honor the computed + // retry delay first, then query. + const status = innerE.status; + const maybeReachedServer = + (typeof status === 'number' && (status >= 500 || status === 408)) || + status == null; // network errors (ECONNRESET, ETIMEDOUT, ...) + if (maybeReachedServer) { + // Cool down first: even read requests count against rate + // limits, and querying during an ongoing 5xx/rate-limit + // episode can worsen the situation. Use the retry delay when + // available; for non-retryable errors (retryInfo == null) + // there is no header-derived wait, so use a short fixed cool + // down before the read. + const coolDownMs = retryInfo != null ? retryInfo.delayMs : FAILURE_DELAY; + if (coolDownMs > 0) { + const secs = (coolDownMs / 1000).toFixed(1); + console.log( + `Cooling down ${secs}s before idempotency check for ${comment.path} ` + + `(HTTP ${innerE.status || 'n/a'}, attempt ${attempt + 1}/${MAX_RETRIES + 1}).` + ); + await sleep(coolDownMs); + } + const alreadyPosted = await isCommentAlreadyPosted({ + owner: context.repo.owner, + repo: context.repo.repo, + prNumber, + id + }); + if (alreadyPosted === true) { + successCount++; + posted = true; + console.log(`Comment for ${comment.path} already posted (id=${id}); treating as success.`); + await sleep(SUCCESS_DELAY); + continue; + } + // Unknown (null): the read API is unavailable, so we + // cannot tell whether the comment landed. To avoid a + // duplicate, do NOT retry posting; record as failed so + // the summary surfaces the uncertainty rather than + // silently risking a duplicate. + if (alreadyPosted === null) { + failedCount++; + const reason = 'idempotency check unavailable (read API failed)'; + failedComments.push({ comment, error: `${innerE.message} [${reason}]` }); + console.log(`Cannot verify whether comment for ${comment.path} was posted (${reason}, HTTP ${innerE.status || 'n/a'}); skipping retry to avoid duplicate.`); + await sleep(SUCCESS_DELAY); + break; + } + // Not found on server. If retries are exhausted or the + // error is non-retryable, this is a real failure. + if (!willRetry) { + failedCount++; + failedComments.push({ comment, error: innerE.message }); + const reason = retryInfo == null ? 'non-retryable error' : 'rate-limit retries exhausted'; + console.log(`Failed to post comment for ${comment.path} (${reason}, HTTP ${innerE.status || 'n/a'}): ${innerE.message}`); + await sleep(SUCCESS_DELAY); + break; + } + // willRetry: cool down already consumed above, loop back. + } else if (willRetry) { + // Pure 429/403 rate-limit: the request never reached the + // server, so no duplicate is possible and the idempotency + // check can be skipped. Just honor the retry delay. const secs = (retryInfo.delayMs / 1000).toFixed(1); console.log( - `Rate-limited/transient error on ${reviewComment.path} ` + + `Rate-limited on ${comment.path} ` + `(HTTP ${innerE.status}, attempt ${attempt + 1}/${MAX_RETRIES}). ` + `Waiting ${secs}s via '${retryInfo.source}' (${retryInfo.detail}). ` + `Error: ${innerE.message}` ); await sleep(retryInfo.delayMs); } else { + // Non-retryable error that definitely did not reach the + // server (e.g. 4xx validation error): record as failed. failedCount++; failedComments.push({ comment, error: innerE.message }); - const reason = retryInfo == null ? 'non-retryable error' : 'rate-limit retries exhausted'; - console.log(`Failed to post comment for ${reviewComment.path} (${reason}, HTTP ${innerE.status || 'n/a'}): ${innerE.message}`); - // After exhausting retries use the success-style pace delay; - // for other errors use the shorter failure pace delay. - await sleep(retryInfo == null ? FAILURE_DELAY : SUCCESS_DELAY); + console.log(`Failed to post comment for ${comment.path} (non-retryable error, HTTP ${innerE.status || 'n/a'}): ${innerE.message}`); + await sleep(FAILURE_DELAY); break; } } @@ -365,19 +500,216 @@ jobs: finalBody += formatCommentMarkdown(comment, error); } } - - await github.rest.issues.createComment({ + + // Prepend the summary tag and post only if no summary with this tag + // already exists (idempotency: the batch review may have carried the + // same summary body, in which case we must not duplicate it). + finalBody = SUMMARY_TAG + '\n' + finalBody; + const summaryAlreadyPosted = await hasIssueCommentWithId({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: prNumber, - body: finalBody + issueNumber: prNumber, + id: SUMMARY_TAG }); + if (summaryAlreadyPosted === true) { + console.log('Summary comment with this run tag already exists; skipping.'); + } else if (summaryAlreadyPosted === null) { + // Read API unavailable: cannot tell whether the summary already + // landed. Skip posting to avoid a duplicate; the review content + // is still available via inline comments / batch review. + console.log('Cannot verify whether summary comment already exists (read API failed); skipping to avoid duplicate.'); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: finalBody + }); + } } function sleep(ms) { return new Promise(resolve => setTimeout(resolve, ms)); } + // Retry wrapper shared by write and read API calls. Reuses + // computeRetryDelayMs so rate-limit headers (retry-after / + // x-ratelimit-*) are honored uniformly. Throws on final failure + // so the caller can decide how to degrade. + async function withRetry(tag, fn) { + for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + try { + return await fn(); + } catch (e) { + const retryInfo = computeRetryDelayMs(e, attempt); + const willRetry = retryInfo != null && attempt < MAX_RETRIES; + if (willRetry) { + const secs = (retryInfo.delayMs / 1000).toFixed(1); + console.log( + `[${tag}] transient/rate-limited (HTTP ${e.status}, attempt ${attempt + 1}/${MAX_RETRIES}). ` + + `Waiting ${secs}s via '${retryInfo.source}' (${retryInfo.detail}). ${e.message}` + ); + await sleep(retryInfo.delayMs); + } else { + console.log(`[${tag}] failed after ${attempt + 1} attempts: ${e.message}`); + throw e; + } + } + } + } + + // Read API wrapper with retry + proactive pacing. Read requests are + // cheaper than writes but still consume the primary rate limit and can + // trigger the secondary limit when issued in a tight loop. Use shorter + // delays than writes (READ_SUCCESS_DELAY / READ_LOW_REMAINING_SPACING). + async function readWithPacing(tag, fn) { + const res = await withRetry(tag, fn); + const remaining = logRateLimitQuota(res, tag); + const lowQuota = remaining != null && remaining <= LOW_REMAINING_THRESHOLD; + if (lowQuota) { + console.log(`[rate-limit] quota low after read (${remaining} <= ${LOW_REMAINING_THRESHOLD}); spacing ${READ_LOW_REMAINING_SPACING}ms.`); + await sleep(READ_LOW_REMAINING_SPACING); + } else { + await sleep(READ_SUCCESS_DELAY); + } + return res; + } + + // Paginated helper that walks all pages of a list endpoint with retry + // and pacing. Returns the concatenated array of items. + async function readAllPages(tag, pageFn, maxPages = 50) { + if (!Number.isFinite(maxPages) || maxPages < 1) { + throw new Error(`readAllPages: maxPages must be a positive integer, got ${maxPages}`); + } + const all = []; + let page = 1; + const PER_PAGE = 100; + while (page <= maxPages) { + const res = await readWithPacing(`${tag} (page ${page})`, () => pageFn(page, PER_PAGE)); + const items = res.data || []; + all.push(...items); + if (items.length < PER_PAGE) break; + page++; + } + // NOTE: Truncation here is intentional and acts as a safety + // valve against unbounded loops (e.g. a bug or malicious + // activity), not as a normal operating mode. A PR accumulating + // >5000 review comments is far outside expected usage; in that + // rare case we log a warning and proceed with partial data + // rather than failing the whole review. + // + // Caveat: this is NOT the same as a read failure. When the read + // API throws (rate limit, 5xx), isCommentAlreadyPosted and + // hasIssueCommentWithId catch it and return null (unknown), so + // the caller skips retrying and creates no duplicate. A + // truncated walk does not throw; it returns a partial set + // silently, so isCommentAlreadyPosted returns false (definitively + // "not posted") for any comment beyond the cap, and the retry + // loop will repost it, producing a duplicate. This tradeoff is + // accepted because the trigger is far outside expected usage; if + // that ceiling ever needs to rise, make maxPages configurable. + if (page > maxPages) { + console.log(`[${tag}] reached max page limit (${maxPages}); results may be incomplete.`); + } + return all; + } + + // Idempotency check: find whether a batch review with this run tag + // already exists on the PR. Returns { found, review } or throws on + // final failure (caller degrades to original fallback). + async function findExistingBatchReview({ owner, repo, prNumber, tag }) { + const reviews = await readAllPages('listReviews', (page, per_page) => + github.rest.pulls.listReviews({ owner, repo, pull_number: prNumber, per_page, page }) + ); + for (const r of reviews) { + if ((r.body || '').includes(tag)) { + return { found: true, review: r }; + } + } + return { found: false }; + } + + // Collect the set of comment-level IDs already posted on the PR + // (across all reviews). Uses listReviewComments (PR-level, cross-review) + // so a single paginated walk covers everything, avoiding the O(missing) + // amplification of per-comment lookups. + async function getPostedCommentIds({ owner, repo, prNumber }) { + const comments = await readAllPages('listReviewComments', (page, per_page) => + github.rest.pulls.listReviewComments({ owner, repo, pull_number: prNumber, per_page, page }) + ); + const ids = new Set(); + // Anchor the regex to the HTML comment wrapper () + // so user-generated content or code suggestions cannot trigger + // false positives in the idempotency check. The ID format is + // `ocr--` where RUN_TAG is `-` + // and is a per-comment random hex token. Capture group 1 + // holds the bare ID (ocr--), so we can add it + // directly without stripping comment markers. + const ID_RE = //g; + for (const c of comments) { + const body = c.body || ''; + let m; + while ((m = ID_RE.exec(body)) !== null) { + ids.add(m[1]); + } + } + return ids; + } + + // Check whether a specific comment-level ID has already landed on the + // server. Used by the per-comment retry loop: when a createReview call + // fails with a transient 5xx/408, the request may have reached GitHub + // and succeeded even though the response was lost. Querying before + // retrying prevents posting a duplicate inline comment. + // Returns true/false when the check succeeds, or null when the + // read API is unavailable (rate limit, 5xx, etc.). Returning null + // (rather than defaulting to false) prevents the caller from + // assuming the comment was not posted and risking a duplicate on + // retry. + // + // Each call walks listReviewComments fresh — no cached snapshot. + // A snapshot reused across retries would go stale as comments land + // during the loop, and a stale miss for a 5xx-landed comment would + // trigger a retry that posts a duplicate. Read calls are paced via + // readAllPages/readWithPacing and degrade to null (skip retry) if the + // read API itself fails, so the extra walks cannot produce duplicates. + async function isCommentAlreadyPosted({ owner, repo, prNumber, id }) { + try { + const posted = await getPostedCommentIds({ owner, repo, prNumber }); + return posted.has(id); + } catch (e) { + console.log(`[isCommentAlreadyPosted] check failed for ${id} (${e.message}); treating as unknown to avoid duplicates.`); + return null; + } + } + + // Check whether an issue comment with the given tag already exists. + // Used to avoid posting a duplicate summary comment when the batch + // review already carried the same summary body. + // Returns true/false when the check succeeds, or null when the + // read API is unavailable. Returning null (rather than defaulting + // to false) lets the caller decide whether to skip posting or + // degrade gracefully, instead of silently risking a duplicate + // summary comment. + async function hasIssueCommentWithId({ owner, repo, issueNumber, id }) { + try { + const comments = await readAllPages('listIssueComments', (page, per_page) => + github.rest.issues.listComments({ owner, repo, issue_number: issueNumber, per_page, page }) + ); + // Match the tag anchored to its HTML comment wrapper for + // consistency with getPostedCommentIds and to defend against + // user content that happens to contain the bare tag string. + // `id` is an opaque tag like ``, + // so escape any regex metacharacters before embedding it. + const escaped = id.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const tagRe = new RegExp(''); + return comments.some(c => tagRe.test(c.body || '')); + } catch (e) { + console.log(`[listIssueComments] check failed (${e.message}); treating as unknown to avoid duplicates.`); + return null; + } + } + // Case-insensitive header lookup. Octokit normalizes response headers to // lowercase, but this defensive check also handles original casing so that // quota logging and retry delay computation never silently miss a header. @@ -483,15 +815,55 @@ jobs: } catch (_) { return null; } } - function formatComment(comment) { - let body = comment.content || ''; + // Random per-comment ID, assigned once when the inline-comment item + // is built and carried on the item struct. Random (rather than + // content-derived) so two distinct comments that share the same + // path/line/content still get different IDs and the idempotency + // check never mistakes one for the other (which would silently drop + // the second). Embedded in the comment body as an HTML comment so + // getPostedCommentIds can match it back on retry. + function newCommentId() { + return `ocr-${RUN_TAG}-${crypto.randomBytes(8).toString('hex')}`; + } - // Add code suggestion if available + // Resolve the line-targeting fields for a createReview comment + // payload (start_line/line/start_side/side) from the comment's line + // range. Returned object is spread into the payload in toReviewPayload. + function resolveLines(comment) { + const start = comment.start_line; + const end = comment.end_line; + if (start >= 1 && end >= 1 && start !== end) { + return { start_line: start, line: end, start_side: 'RIGHT', side: 'RIGHT' }; + } else if (end >= 1) { + return { line: end, side: 'RIGHT' }; + } else if (start >= 1) { + return { line: start, side: 'RIGHT' }; + } + return {}; + } + + // Build the createReview payload for an inline-comment item. The + // body is assembled here (at call time) from the item's precomputed + // ID, so retry/idempotency logic works directly off item.id instead + // of recomputing an ID each time it needs to check posting status. + function toReviewPayload(item) { + return { + path: item.comment.path, + body: buildBody(item.comment, item.id), + ...item.lines + }; + } + + // Assemble the visible comment body: the per-comment ID tag (HTML + // comment, invisible when rendered) prepended for idempotency + // matching, plus the code suggestion block if present. + function buildBody(comment, id) { + let body = `\n`; + body += comment.content || ''; if (comment.suggestion_code && comment.existing_code) { body += '\n\n**Suggestion:**\n'; body += fencedBlock(comment.suggestion_code, 'suggestion'); } - return body; }