diff --git a/action.yml b/action.yml index 8812e2953..d94e0ac74 100644 --- a/action.yml +++ b/action.yml @@ -87,6 +87,20 @@ inputs: Value in (0, 1]; ignored unless incremental is true. required: false default: '0.6' + resolve_outdated: + description: >- + Resolve the action's own outdated inline review threads (threads GitHub + has already marked outdated because their lines no longer exist in the + diff). Threads a human replied to, threads already resolved, and threads + whose lines a finding from the current run still covers are never touched. + One of: 'false' (default, does nothing and makes no API calls), 'report' + (log what would be resolved, change nothing — run this first), 'true' + (actually resolve). 'true' requires the calling workflow to grant + `contents: write` in its own `permissions:` block; the default read-only + token cannot resolve a thread and the action will warn and continue. + Overlap with current findings is decided by incremental_overlap_threshold. + required: false + default: 'false' review_comment_batch_size: description: >- Maximum number of inline comments packed into a single createReview call. @@ -151,6 +165,16 @@ outputs: comments_failed: description: Number of inline comments that failed to post. value: ${{ steps.post.outputs.comments_failed }} + comments_resolved: + description: >- + Number of outdated bot review threads resolved this run. Always 0 unless + resolve_outdated is 'true'. + value: ${{ steps.post.outputs.comments_resolved }} + comments_resolved_preview: + description: >- + Number of threads resolve_outdated 'report' mode would have resolved. + Always 0 in 'false' and 'true' modes. + value: ${{ steps.post.outputs.comments_resolved_preview }} summary_comment_url: description: URL of the posted/updated summary comment, if any. value: ${{ steps.post.outputs.summary_comment_url }} @@ -316,6 +340,7 @@ 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 }} + OCR_RESOLVE_OUTDATED: ${{ inputs.resolve_outdated }} with: github-token: ${{ inputs.github_token }} script: | @@ -346,4 +371,5 @@ runs: reviewCommentBatchSize: parseInt(process.env.OCR_REVIEW_COMMENT_BATCH_SIZE, 10), routeSeverityBelow: process.env.OCR_ROUTE_SEVERITY_BELOW, routeCategories: process.env.OCR_ROUTE_CATEGORIES, + resolveOutdated: process.env.OCR_RESOLVE_OUTDATED, }); diff --git a/examples/github_actions/README.md b/examples/github_actions/README.md index 0236505f5..d48bfa01f 100644 --- a/examples/github_actions/README.md +++ b/examples/github_actions/README.md @@ -147,7 +147,8 @@ The action posts a summary issue comment plus inline review comments. Two inputs |-------|---------|-------------| | `sticky_summary` | `'true'` | Update an existing summary comment in place instead of posting a new one each run. | | `incremental` | `'false'` | Only append inline comments whose `(path, line range)` does not overlap an existing bot review comment. History is never deleted (non-destructive). | -| `incremental_overlap_threshold` | `'0.6'` | IoU threshold `incremental` uses to decide whether a multi-line comment overlaps an existing one. Two single-line comments match on the same line; single- vs multi-line never match. Ignored unless `incremental` is `'true'`. | +| `incremental_overlap_threshold` | `'0.6'` | IoU threshold `incremental` uses to decide whether a multi-line comment overlaps an existing one. Two single-line comments match on the same line; single- vs multi-line never match. Shared with `resolve_outdated`, which uses the same test to decide whether a current finding still covers an outdated thread. Ignored unless `incremental` or `resolve_outdated` is enabled. | +| `resolve_outdated` | `'false'` | Resolve the action's own outdated inline threads. `'false'` does nothing; `'report'` logs what it would resolve; `'true'` resolves them. See below. | ```yaml - uses: alibaba/open-code-review@main @@ -159,6 +160,40 @@ 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. +### Resolve outdated review threads + +Over a long PR, the bot's inline comments pile up on code that no longer exists. `resolve_outdated` closes those threads, using GitHub's own `isOutdated` flag — no LLM is involved in the decision, and the action only ever touches threads GitHub has already marked outdated. + +| Value | Behavior | +|-------|----------| +| `'false'` (default) | Does nothing. No GraphQL calls are made at all. | +| `'report'` | Lists what would be resolved, and why each other thread was skipped. Changes nothing. | +| `'true'` | Resolves them. | + +Start with `'report'` for a few PRs and read the `[resolve-outdated]` log line before switching to `'true'`. + +```yaml +permissions: + contents: write # required by resolve_outdated: 'true' + pull-requests: write + +steps: + - uses: alibaba/open-code-review@main + with: + resolve_outdated: 'report' +``` + +A thread is left alone unless **the action created it**. Ownership is decided by the marker OCR writes into every inline comment it posts, not by the comment's author — under the default `GITHUB_TOKEN` every workflow in your repository posts as `github-actions[bot]`, so a sibling workflow's review threads would otherwise be indistinguishable from OCR's own. Beyond that, a thread is left alone whenever **a human has replied** to it, when it is already resolved, when a finding from the current run still covers its lines, or when it has more comments than one API page returns (so a reply the action cannot see is never resolved over). Resolution also runs only after a run that actually produced findings: a run that failed to parse OCR's output, or that reported nothing, resolves nothing — "the model said nothing this time" is not evidence the old findings are gone. At most 50 threads are resolved per run; the rest carry over to the next one. + +Outputs: `comments_resolved` (threads resolved, `'true'` mode) and `comments_resolved_preview` (threads that would be resolved, `'report'` mode). Both are always present. + +Four things worth knowing before you turn this on: + +- **`'true'` requires `contents: write`.** The default read-only `GITHUB_TOKEN` cannot resolve a thread; the calling workflow must grant `contents: write` in its own `permissions:` block. Without it the action logs a warning naming the missing permission and continues — the review itself still posts. +- **A force-push can mark a live finding's thread outdated.** GitHub sets `isOutdated` when the thread's lines are no longer in the diff, and it stays set even after a force-push that lands identical content, so a rebase can outdate a thread whose finding is still real. The current-run overlap check catches this when the model re-reports the finding; if it doesn't, the thread closes while the problem remains. This is the main reason to run `'report'` first. +- **It only does anything on re-runs.** A thread can only become outdated after a later push, so the feature is exercised only by workflows that review on update (`types: [opened, synchronize, reopened]`, as in the sample workflow above). This repository's own review workflow triggers on `opened` only and never reaches the resolution path — do not read its runs as evidence the feature works for you. +- **Resolving is not deleting.** Resolved threads collapse but stay readable, and anyone can unresolve one. + ### 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 231aad0db..fb7347957 100644 --- a/scripts/github-actions/post-review-comments.js +++ b/scripts/github-actions/post-review-comments.js @@ -84,6 +84,11 @@ async function runPostReviewComments({ // (fail-open for the policy itself, upholding I1). routeSeverityBelow = "", routeCategories = "", + // Outdated-thread resolution (#567). Opt-in, string-valued exactly like + // routeSeverityBelow: 'true' resolves, 'report' only logs what it would + // resolve, anything else (including the default) is a hard no-op that issues + // zero GraphQL calls. + resolveOutdated = "", }) { const log = (msg) => { if (core && typeof core.info === "function") core.info(msg); @@ -112,6 +117,8 @@ async function runPostReviewComments({ skipped: 0, failed: 0, routed: 0, + resolved: 0, + resolvedPreview: 0, summaryUrl: "", }; @@ -137,6 +144,13 @@ async function runPostReviewComments({ stats.total = comments.length; // No comments: post a "looks good" summary. + // + // Outdated-thread resolution deliberately does NOT run here (nor at the + // parse-failure exit above). "The model reported nothing this run" is not + // evidence that previously-reported findings are gone — a truncated review, a + // model hiccup, or a diff the reviewer skipped all land on this exit, and any + // of them would otherwise close every open thread on the PR in one sweep. + // Resolution requires a run that actually produced findings. if (comments.length === 0) { const message = result.message || "No comments generated. Looks good to me."; const body = `${SUMMARY_MARKER}\n✅ **OpenCodeReview**: ${message}`; @@ -348,6 +362,51 @@ async function runPostReviewComments({ }); if (finalized) stats.summaryUrl = finalized.url; + // ---- Resolve our own outdated threads (#567) ---- + // Reachable only from here: both earlier exits (unparseable output, zero + // findings) return before this point on purpose. The gate is therefore + // positional and exact — "this run parsed >= 1 finding AND completed the + // posting loop". Findings that never went out as inline comments (suppressed + // by incremental dedupe, routed to the summary, or failed to post) still + // count towards the gate AND still veto resolution of a thread on their own + // lines, which is why currentSpans is built from the raw parsed findings + // rather than from `toSend`: a finding we chose not to repeat is still a + // finding that is live. + const resolveMode = + resolveOutdated === "true" ? "resolve" : resolveOutdated === "report" ? "report" : "off"; + // Unknown values fall to "off", which is the right default but an invisible + // one: a workflow that says 'True' or 'yes' would look configured and do + // nothing, and "silently does nothing" is the failure mode this feature can + // least afford. Empty/unset is the documented default, so it is not flagged. + if (resolveMode === "off" && resolveOutdated && resolveOutdated !== "false") { + const msg = + `[resolve-outdated] ignoring unrecognized resolve_outdated value ${JSON.stringify(resolveOutdated)}; ` + + `expected 'false', 'report', or 'true'. Resolving nothing.`; + if (core && typeof core.warning === "function") core.warning(msg); + else log(msg); + } + if (resolveMode !== "off") { + const currentSpans = comments.map((c) => ({ + path: c.path, + start_line: c.start_line, + line: c.end_line, + })); + const r = await resolveOutdatedThreads({ + github, + owner, + repo, + prNumber, + core, + log, + botLogin: await getAuthenticatedLogin(github, log), + currentSpans, + overlapThreshold: incrementalOverlapThreshold, + dryRun: resolveMode === "report", + }); + stats.resolved = r.resolved; + stats.resolvedPreview = resolveMode === "report" ? r.attempted : 0; + } + setStatsOutputs(out, stats, batchCounters, batchSize); } @@ -772,6 +831,11 @@ function setStatsOutputs(out, stats, batchCounters, batchSize) { out("comments_skipped", String(stats.skipped)); out("comments_routed", String(stats.routed)); out("comments_failed", String(stats.failed)); + // Emitted on every exit (always "0" when resolve_outdated is off or in + // 'true' mode respectively) so a consumer workflow can read them + // unconditionally instead of testing for their existence. + out("comments_resolved", String(stats.resolved || 0)); + out("comments_resolved_preview", String(stats.resolvedPreview || 0)); out("summary_comment_url", stats.summaryUrl || ""); // Per-batch telemetry (B7). These are additional outputs; the five above are // unchanged so existing consumers of comments_* / summary_comment_url are @@ -1050,6 +1114,284 @@ function num(v) { return Number.isFinite(n) && n >= 1 ? n : null; } +// The one description of OCR's inline-comment marker. Two call sites depend on +// it — the retry idempotency check (getPostedCommentIds) and the resolve +// ownership check (threadIsOurs) — and a drift between them would be silent in +// both directions, so they are built from this single source. Anchored to the +// HTML comment wrapper so user content or a quoted suggestion cannot forge it. +const OCR_COMMENT_ID_SOURCE = String.raw``; + +// ---- Outdated thread resolution (#567) ---- +// +// Deterministic, zero-LLM cleanup of the bot's OWN stale inline threads. The +// "is this finding still relevant?" judgement is never ours: GitHub computes +// `isOutdated` server-side (the thread's original lines no longer exist in the +// current diff) and we only ever act on threads it has already marked that way. +// Everything else in the predicate is a veto — a human reply, an already +// resolved thread, or a finding from THIS run still sitting on the same lines +// all keep the thread open. + +const REVIEW_THREADS_QUERY = ` +query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + id + path + isOutdated + isResolved + originalLine + originalStartLine + comments(first: 100) { totalCount nodes { author { login __typename } } } + # Aliased second selection so only the ROOT comment's body crosses the + # wire: ownership is proved by the marker OCR stamps on the comment it + # created, and replies cannot carry that proof. Fetching every body + # would multiply the payload for a field only nodes[0] is read from. + root: comments(first: 1) { nodes { body } } + } + } + } + } +}`; + +const RESOLVE_THREAD_MUTATION = ` +mutation($threadId: ID!) { + resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } +}`; + +// Hard cap on resolve mutations per run. Resolution is cleanup, not the job: +// a PR that somehow accumulated hundreds of outdated bot threads should trickle +// down over several runs rather than burn a run's whole GraphQL budget (and +// trip secondary rate limits) in one burst. +const MAX_RESOLVE_PER_RUN = 50; + +// List every review thread on the PR, paginated. Read-only and fail-open: +// any GraphQL error degrades to "no threads", which makes the whole feature a +// no-op rather than failing the run over a cleanup step. +async function listBotReviewThreads({ github, owner, repo, prNumber, log, warn }) { + const all = []; + let cursor = null; + const MAX_PAGES = 10; // 1000 threads; far past any realistic PR. + try { + for (let page = 0; page < MAX_PAGES; page++) { + const res = await github.graphql(REVIEW_THREADS_QUERY, { + owner, + repo, + number: prNumber, + cursor, + }); + const threads = + res && res.repository && res.repository.pullRequest + ? res.repository.pullRequest.reviewThreads + : null; + if (!threads) break; + for (const node of threads.nodes || []) { + if (node) all.push(node); + } + if (!threads.pageInfo || !threads.pageInfo.hasNextPage) break; + cursor = threads.pageInfo.endCursor; + } + } catch (e) { + const msg = `[resolve-outdated] listing review threads failed (${e.message}); resolving nothing.`; + if (warn) warn(msg); + else log(msg); + return []; + } + return all; +} + +// Does this thread's ROOT comment carry a marker OCR wrote? Only the root is +// consulted: OCR creates a thread and never replies into one, so a marker +// appearing deeper would mean someone quoted our comment back at us, which is +// evidence of a human conversation rather than of ownership. A thread whose +// root body did not come back (older data, a truncated response) is not +// demonstrably ours and returns false — the fail-closed direction, since the +// cost is a stale thread left open rather than someone else's thread resolved. +function threadIsOurs(thread) { + const root = thread && thread.root && thread.root.nodes && thread.root.nodes[0]; + const body = (root && root.body) || ""; + return new RegExp(OCR_COMMENT_ID_SOURCE).test(body); +} + +// GraphQL's reviewThreads returns a bot's SLUG ("github-actions") where REST — +// and therefore getAuthenticatedLogin() and isBotComment() — uses the suffixed +// form ("github-actions[bot]"). Measured on a live PR: the same comment reads +// `github-actions` with __typename "Bot" through GraphQL and +// `github-actions[bot]` through REST. Comparing the raw GraphQL login against a +// REST-shaped botLogin would make every one of our own threads look like a +// human reply, so the feature would resolve nothing and look merely inert. +// Normalizing here keeps isBotComment's REST contract untouched for incremental +// mode, which is its other caller. +function graphqlAuthorLogin(author) { + if (!author || !author.login) return ""; + const login = String(author.login); + return author.__typename === "Bot" && !/\[bot\]$/i.test(login) ? `${login}[bot]` : login; +} + +// Pure predicate: may this thread be resolved, and if not, why not? +// Returns "resolve" | "not_outdated" | "already_resolved" | "unverified" | +// "not_ours" | "human_reply" | "overlap". +// +// Deliberately does NOT consult `viewerCanResolve`: it reports false for tokens +// that can in fact resolve the thread, so gating on it would silently disable +// the feature. The mutation is attempted and its failure is caught instead. +function shouldResolveThread(thread, { botLogin, currentSpans = [], overlapThreshold } = {}) { + if (!thread || thread.isOutdated !== true) return "not_outdated"; + if (thread.isResolved === true) return "already_resolved"; + // Any participant we cannot positively identify as the bot counts as a human + // (an unknown/ghost author is treated as human on purpose — the failure mode + // of leaving a thread open is strictly cheaper than closing someone's reply). + const nodes = (thread.comments && thread.comments.nodes) || []; + // "No comment is a human's" is only evidence when we can see every comment. + // A thread with nothing visible is not demonstrably ours, and one with more + // comments than the query returned could carry a human reply we never saw — + // both stay open rather than being resolved on a partial view. + const total = thread.comments && thread.comments.totalCount; + if (nodes.length === 0 || (typeof total === "number" && total > nodes.length)) return "unverified"; + // Authorship alone cannot establish that WE created this thread. Every + // workflow in a repo that uses the default GITHUB_TOKEN posts as the same + // identity, so a sibling workflow's outdated review threads are + // indistinguishable from ours by author; and under a GitHub App token the + // `github-actions[bot]` fallback in isBotComment would accept those threads + // outright. The marker OCR stamps into every inline comment it creates + // (newCommentId, via formatComment) is the actual proof of authorship, so it + // is what the destructive path gates on. Checked before the human-reply loop + // because "not ours" is the more fundamental answer: a thread we did not + // create is not ours to classify, let alone resolve. + if (!threadIsOurs(thread)) return "not_ours"; + for (const c of nodes) { + const login = graphqlAuthorLogin(c && c.author); + if (!isBotComment({ user: { login } }, botLogin)) return "human_reply"; + } + // A thread whose ORIGINAL lines are still covered by a finding from this run + // is the rebase case: GitHub calls it outdated because the diff moved, but the + // model just re-reported the same problem. Leave it open. Reuses the exact + // incremental same-comment test (lineSpan + sameCommentSpan + resolveThreshold + // via overlapsHistory) so the two features can never disagree about identity. + const span = { path: thread.path, start_line: thread.originalStartLine, line: thread.originalLine }; + // No usable original line means overlapsHistory can only ever answer "no + // overlap", so the veto below is unreachable for this thread — and that veto + // is the whole mitigation for GitHub reporting a still-live finding's thread + // as outdated after a force-push. Resolving on a check that cannot fail is + // worse than leaving the thread open, so treat it exactly like a partial + // comment view. + if (!lineSpan(span)) return "unverified"; + if (overlapsHistory(span, currentSpans, overlapThreshold)) return "overlap"; + return "resolve"; +} + +// Classify a resolve-mutation failure. Both "forbidden" and "throttled" mean +// "stop trying for this run" — the first because every later mutation will fail +// identically, the second because hammering a throttled endpoint deepens the +// incident. Throttling is checked first: GitHub reports secondary rate limits +// as 403, which would otherwise read as a permission problem. +function classifyResolveError(e) { + if (!e) return "other"; + const types = []; + const msgs = [e.message]; + if (e.type) types.push(String(e.type)); + // Octokit surfaces GraphQL errors either directly on the error (`.errors`) or + // under the parsed response (`.response.errors`); check both shapes. + for (const list of [e.errors, e.response && e.response.errors]) { + if (!Array.isArray(list)) continue; + for (const item of list) { + if (item && item.type) types.push(String(item.type)); + if (item && item.message) msgs.push(String(item.message)); + } + } + const text = msgs.filter(Boolean).join(" "); + if (/rate limit|abuse|secondary/i.test(text) || e.status === 429) return "throttled"; + if ( + types.some((t) => /^(FORBIDDEN|UNAUTHORIZED|INSUFFICIENT_SCOPES)$/i.test(t)) || + /not accessible by integration|resource not accessible|forbidden|permission/i.test(text) || + e.status === 403 + ) { + return "forbidden"; + } + return "other"; +} + +// Resolve this run's stale threads, sequentially and capped. Never throws: +// every failure path degrades to "resolved fewer threads than we could have". +// Returns { candidates, attempted, resolved, failed, reasons }. +async function resolveOutdatedThreads({ + github, + owner, + repo, + prNumber, + core, + log, + botLogin, + currentSpans = [], + overlapThreshold, + dryRun = false, +}) { + const warn = (msg) => { + if (core && typeof core.warning === "function") core.warning(msg); + else log(msg); + }; + const threads = await listBotReviewThreads({ github, owner, repo, prNumber, log, warn }); + + const reasons = {}; + const candidates = []; + for (const thread of threads) { + const reason = shouldResolveThread(thread, { botLogin, currentSpans, overlapThreshold }); + reasons[reason] = (reasons[reason] || 0) + 1; + if (reason === "resolve") candidates.push(thread); + } + const attempted = candidates.slice(0, MAX_RESOLVE_PER_RUN); + + let resolved = 0; + let failed = 0; + if (!dryRun) { + // Sequential on purpose: a burst of parallel mutations is exactly what + // trips GitHub's secondary rate limiter on a write endpoint. + const delay = parseNonNegInt(process.env.OCR_RESOLVE_DELAY, 1000); + for (let i = 0; i < attempted.length; i++) { + try { + await github.graphql(RESOLVE_THREAD_MUTATION, { threadId: attempted[i].id }); + resolved++; + } catch (e) { + const kind = classifyResolveError(e); + if (kind === "forbidden") { + warn( + `[resolve-outdated] cannot resolve review threads with this token (${e.message}). ` + + `resolve_outdated: 'true' needs the calling workflow to grant \`contents: write\` in its permissions block. ` + + `Resolved ${resolved} of ${attempted.length} thread(s); skipping the rest.` + ); + break; + } + if (kind === "throttled") { + warn( + `[resolve-outdated] rate limited while resolving threads (${e.message}); ` + + `resolved ${resolved} of ${attempted.length} thread(s); skipping the rest this run.` + ); + break; + } + failed++; + warn(`[resolve-outdated] failed to resolve thread ${attempted[i].id}: ${e.message}`); + } + if (delay > 0 && i < attempted.length - 1) await sleep(delay); + } + } + + const skipped = + Object.keys(reasons) + .filter((k) => k !== "resolve") + .sort() + .map((k) => `${k}:${reasons[k]}`) + .join(",") || "none"; + log( + `[resolve-outdated] mode=${dryRun ? "report" : "resolve"} threads=${threads.length} ` + + `candidates=${candidates.length} attempted=${attempted.length} resolved=${resolved} ` + + `failed=${failed} skipped=${skipped}` + ); + + return { candidates: candidates.length, attempted: attempted.length, resolved, failed, reasons }; +} + // ---- Rate-limit / retry helpers (ported verbatim) ---- function sleep(ms) { @@ -1286,13 +1628,12 @@ async function getPostedCommentIds({ github, owner, repo, prNumber, log }) { github.rest.pulls.listReviewComments({ owner, repo, pull_number: prNumber, per_page, page }), log ); 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, so we can add it directly - // without stripping comment markers. - const ID_RE = //g; + // The ID format is `ocr--` where RUN_TAG is + // `-` and is a per-comment random hex token. + // Capture group 1 holds the bare ID, so we can add it directly without + // stripping comment markers. Built with /g (own instance — /g carries + // lastIndex, so it must never be shared) to walk every ID in one body. + const ID_RE = new RegExp(OCR_COMMENT_ID_SOURCE, "g"); for (const c of comments) { const body = c.body || ""; let m; @@ -2115,4 +2456,9 @@ module.exports = { isLineResolutionFailure, cooldownAndReconcile, getPrDiffHunks, + listBotReviewThreads, + shouldResolveThread, + resolveOutdatedThreads, + classifyResolveError, + MAX_RESOLVE_PER_RUN, }; diff --git a/scripts/github-actions/post-review-comments.test.js b/scripts/github-actions/post-review-comments.test.js index a08c258eb..3fbd9064c 100644 --- a/scripts/github-actions/post-review-comments.test.js +++ b/scripts/github-actions/post-review-comments.test.js @@ -157,10 +157,46 @@ function makeGithub(opts = {}) { return n; } + // GraphQL surface for outdated-thread resolution (#567). Records every call + // so a test can assert the default-off path issues literally none. Queries + // return opts.threads (single page unless opts.threadPages is given); + // mutations consult opts.resolveErrorSpec(index) for injected failures. + const graphqlCalls = []; + let resolveMutations = 0; + async function graphql(query, vars) { + graphqlCalls.push({ query, vars }); + if (/resolveReviewThread/.test(query)) { + const idx = resolveMutations++; + if (typeof opts.resolveErrorSpec === "function") { + const err = opts.resolveErrorSpec(idx); + if (err) throw err; + } + return { resolveReviewThread: { thread: { id: vars.threadId, isResolved: true } } }; + } + if (opts.threadsThrow) throw new Error(opts.threadsError || "graphql unavailable"); + const pages = opts.threadPages || [opts.threads || []]; + const pageIdx = vars.cursor ? Number(vars.cursor) : 0; + const hasNextPage = pageIdx + 1 < pages.length; + return { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage, endCursor: String(pageIdx + 1) }, + nodes: pages[pageIdx] || [], + }, + }, + }, + }; + } + const resolveMutationCalls = () => graphqlCalls.filter((c) => /resolveReviewThread/.test(c.query)); + return { createReviewCalls, issueComments, updatedComments, + graphqlCalls, + resolveMutationCalls, + graphql, listCommentsCalls, listReviewCommentsCalls, listReviewsCalls, @@ -351,11 +387,14 @@ function makeGithub(opts = {}) { function mockCore() { const outputs = {}; const logs = []; + const warnings = []; return { outputs, logs, + warnings, setOutput(name, value) { outputs[name] = value; }, info(message) { logs.push(message); }, + warning(message) { warnings.push(message); }, }; } @@ -2210,6 +2249,18 @@ async function main() { await testDiffFetchFailureDegradesToPerComment(); await testAllCommentsFilteredOutAccounting(); await testCrossHunkRangeIsFilteredOut(); + // Outdated thread resolution (#567) + testShouldResolveThreadTable(); + await testShouldResolveThreadPartialCommentView(); + await testResolveOutdatedThreadsCapsAndSequences(); + await testResolveOutdatedThreadsErrorHandling(); + await testListBotReviewThreadsPaginatesAndDegrades(); + await testResolveOutdatedThreadsDryRun(); + await testResolveOutdatedOffByDefaultIssuesNoGraphql(); + await testResolveOutdatedNeverRunsOnEmptyRuns(); + await testResolveOutdatedResolvesAfterASuccessfulRun(); + await testResolveOutdatedReportModePreviewsOnly(); + await testResolveOutdatedForbiddenDoesNotFailTheRun(); console.log("All post-review-comments tests passed."); } function testParseDiffHunkRanges() { @@ -3187,6 +3238,471 @@ async function testCrossHunkRangeIsFilteredOut() { assert.strictEqual(summaryText.includes("Lines 2-51 could not be resolved"), true); } +// ---- Outdated thread resolution (#567) ---- + +// Exported separately from the big destructure above so the pre-existing import +// line stays untouched. +const { + listBotReviewThreads, + shouldResolveThread, + resolveOutdatedThreads, + MAX_RESOLVE_PER_RUN, +} = require(path.join(__dirname, "post-review-comments.js")); + +const BOT = "github-actions[bot]"; + +// A root body carrying the marker OCR stamps into every inline comment it +// creates (`ocr---`, per newCommentId). This is +// what proves a thread is ours, so the resolvable fixture must carry it. +const OURS = "\nfindings go here"; + +// An outdated, unresolved, bot-only thread on src/a.js:10 that OCR created — +// the one shape that is resolvable. Every case below is this minus exactly one +// condition. +function botThread(over = {}) { + return Object.assign( + { + id: "T1", + path: "src/a.js", + isOutdated: true, + isResolved: false, + originalLine: 10, + originalStartLine: null, + comments: { nodes: [{ author: { login: BOT } }] }, + root: { nodes: [{ body: OURS }] }, + }, + over + ); +} + +function resolveArgs(gh, core, over = {}) { + return Object.assign( + { + github: gh, + owner: "owner", + repo: "repo", + prNumber: 123, + core, + log: (m) => core.info(m), + botLogin: BOT, + currentSpans: [], + }, + over + ); +} + +// Count of the single per-invocation summary line. Everything else the feature +// emits goes to core.warning, so this stays exactly 1 whatever happens. +function resolveLogLines(core) { + return core.logs.filter((l) => l.includes("[resolve-outdated]")); +} + +function testShouldResolveThreadTable() { + const sameLine = [{ path: "src/a.js", start_line: 10, line: 10 }]; + const cases = [ + { name: "outdated bot thread, nothing current on those lines", thread: botThread(), spans: [], want: "resolve" }, + // viewerCanResolve reports false for tokens that CAN resolve, so the + // predicate must ignore it entirely (no "cannot_resolve" outcome exists). + { name: "viewerCanResolve:false is ignored", thread: botThread({ viewerCanResolve: false }), spans: sameLine.slice(0, 0), want: "resolve" }, + // With no usable original line the overlap veto below cannot fire, so the + // thread would be resolved on a check that is structurally unable to fail. + // That veto is the mitigation for a force-push marking a still-live + // finding's thread outdated, so this must stay open, not resolve. + { name: "no original line information", thread: botThread({ originalLine: null, originalStartLine: null }), spans: sameLine, want: "unverified" }, + { name: "no original line information, nothing current either", thread: botThread({ originalLine: null, originalStartLine: null }), spans: [], want: "unverified" }, + // GraphQL returns the bot SLUG; getAuthenticatedLogin/isBotComment use the + // REST "[bot]"-suffixed form. Measured on a live PR: the same comment is + // `github-actions` (__typename Bot) via GraphQL and `github-actions[bot]` + // via REST. Without normalizing, every one of our own threads reads as a + // human reply and the feature silently resolves nothing. + { + name: "GraphQL bot slug without [bot] still counts as ours", + thread: botThread({ comments: { totalCount: 1, nodes: [{ author: { login: "github-actions", __typename: "Bot" } }] } }), + spans: [], + want: "resolve", + }, + { + name: "a User whose login merely looks bot-ish is still a human", + thread: botThread({ comments: { totalCount: 1, nodes: [{ author: { login: "github-actions", __typename: "User" } }] } }), + spans: [], + want: "human_reply", + }, + { name: "same line on a DIFFERENT path never vetoes", thread: botThread({ path: "src/b.js" }), spans: sameLine, want: "resolve" }, + // Ownership is the marker, not the author. Every workflow in a repo using + // the default GITHUB_TOKEN posts as this same identity, so these threads + // are bot-authored AND indistinguishable from ours by author alone — yet + // they are someone else's conversation and must never be resolved. + { + name: "a sibling workflow's thread under the same bot identity is not ours", + thread: botThread({ root: { nodes: [{ body: "Dependency update available." }] } }), + spans: [], + want: "not_ours", + }, + { name: "root body absent", thread: botThread({ root: undefined }), spans: [], want: "not_ours" }, + { name: "root present but empty", thread: botThread({ root: { nodes: [] } }), spans: [], want: "not_ours" }, + // The marker is anchored to its HTML comment wrapper, so a body that merely + // mentions the ID (a quote, a code block, a suggestion echoing our comment) + // cannot pass ownership off to another author's thread. + { + name: "an unwrapped mention of an OCR id does not prove ownership", + thread: botThread({ root: { nodes: [{ body: "re: ocr-42-1-deadbeefcafe1234 — see above" }] } }), + spans: [], + want: "not_ours", + }, + // "Not ours" is answered before the author loop: a thread we did not create + // is not ours to classify, so a human reply on it is not our business. + { + name: "not ours outranks a human reply", + thread: botThread({ + root: { nodes: [{ body: "some other bot" }] }, + comments: { nodes: [{ author: { login: BOT } }, { author: { login: "octocat" } }] }, + }), + spans: [], + want: "not_ours", + }, + { name: "not outdated", thread: botThread({ isOutdated: false }), spans: [], want: "not_outdated" }, + { name: "isOutdated absent", thread: botThread({ isOutdated: undefined }), spans: [], want: "not_outdated" }, + { name: "already resolved", thread: botThread({ isResolved: true }), spans: [], want: "already_resolved" }, + { + name: "a human replied", + thread: botThread({ comments: { nodes: [{ author: { login: BOT } }, { author: { login: "octocat" } }] } }), + spans: [], + want: "human_reply", + }, + { + name: "unidentifiable author counts as human", + thread: botThread({ comments: { nodes: [{ author: null }] } }), + spans: [], + want: "human_reply", + }, + { name: "current single-line finding on the same line (rebase)", thread: botThread(), spans: sameLine, want: "overlap" }, + { + name: "current multi-line finding above the IoU threshold", + thread: botThread({ originalStartLine: 10, originalLine: 20 }), + spans: [{ path: "src/a.js", start_line: 11, line: 20 }], + want: "overlap", + }, + { + name: "current multi-line finding below the IoU threshold", + thread: botThread({ originalStartLine: 10, originalLine: 20 }), + spans: [{ path: "src/a.js", start_line: 19, line: 40 }], + want: "resolve", + }, + ]; + for (const c of cases) { + const got = shouldResolveThread(c.thread, { botLogin: BOT, currentSpans: c.spans }); + assert.strictEqual(got, c.want, c.name); + assert.notStrictEqual(got, "cannot_resolve", "permission is never a predicate outcome"); + } +} + +// The author check can only clear a thread when the query returned all of its +// comments. A truncated or empty comment list must veto, or a human reply past +// the page boundary would be invisible and the thread resolved on top of it. +async function testShouldResolveThreadPartialCommentView() { + const botComments = (n) => Array.from({ length: n }, () => ({ author: { login: BOT } })); + const cases = [ + { name: "no visible comments", comments: { totalCount: 0, nodes: [] }, want: "unverified" }, + { name: "comments object missing entirely", comments: undefined, want: "unverified" }, + { name: "more comments than the page returned", comments: { totalCount: 101, nodes: botComments(100) }, want: "unverified" }, + { name: "full page, nothing truncated", comments: { totalCount: 100, nodes: botComments(100) }, want: "resolve" }, + { name: "totalCount absent, comments visible", comments: { nodes: botComments(2) }, want: "resolve" }, + ]; + for (const c of cases) { + const thread = botThread({ comments: c.comments }); + assert.strictEqual(shouldResolveThread(thread, { botLogin: BOT, currentSpans: [] }), c.want, c.name); + } + + // End to end: a truncated thread is counted and reported, never mutated. + const gh = makeGithub({ + threads: [botThread({ id: "TRUNC", comments: { totalCount: 101, nodes: botComments(100) } })], + }); + const core = mockCore(); + const out = await withEnv({ OCR_RESOLVE_DELAY: "0" }, () => + resolveOutdatedThreads(resolveArgs(gh, core)) + ); + assert.strictEqual(gh.resolveMutationCalls().length, 0, "a partially-visible thread is never resolved"); + assert.strictEqual(out.resolved, 0); + assert.deepStrictEqual(out.reasons, { unverified: 1 }); + assert.match(resolveLogLines(core)[0], /skipped=unverified:1/); +} + +async function testResolveOutdatedThreadsCapsAndSequences() { + const threads = []; + for (let i = 0; i < 60; i++) threads.push(botThread({ id: `T${i}`, originalLine: 100 + i })); + const gh = makeGithub({ threads }); + const core = mockCore(); + + // Wrap the mock to observe concurrency: a Promise.all implementation would + // show 50 in flight at once, a sequential loop never more than 1. + const inner = gh.graphql; + let inFlight = 0; + let maxInFlight = 0; + gh.graphql = async (q, v) => { + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + await new Promise((r) => setImmediate(r)); + const res = await inner(q, v); + inFlight--; + return res; + }; + + const started = Date.now(); + const out = await withEnv({ OCR_RESOLVE_DELAY: "0" }, () => + resolveOutdatedThreads(resolveArgs(gh, core)) + ); + + assert.strictEqual(MAX_RESOLVE_PER_RUN, 50); + assert.strictEqual(gh.resolveMutationCalls().length, 50, "capped at MAX_RESOLVE_PER_RUN"); + assert.strictEqual(maxInFlight, 1, "mutations must be strictly sequential"); + assert.deepStrictEqual( + { candidates: out.candidates, attempted: out.attempted, resolved: out.resolved, failed: out.failed }, + { candidates: 60, attempted: 50, resolved: 50, failed: 0 } + ); + // OCR_RESOLVE_DELAY is honored: at the 1000ms default this loop would need + // ~49s, so finishing in under 2s proves the env knob is the pacing source. + assert.strictEqual(Date.now() - started < 2000, true, "OCR_RESOLVE_DELAY must control pacing"); + assert.strictEqual(resolveLogLines(core).length, 1, "exactly one summary line per invocation"); + assert.match(resolveLogLines(core)[0], /mode=resolve threads=60 candidates=60 attempted=50 resolved=50/); +} + +async function testResolveOutdatedThreadsErrorHandling() { + const gqlErr = (message, extra) => Object.assign(new Error(message), extra || {}); + const threads = [botThread({ id: "A" }), botThread({ id: "B" }), botThread({ id: "C" })]; + const cases = [ + { + name: "FORBIDDEN on the first mutation stops the run", + spec: () => gqlErr("Resource not accessible by integration", { errors: [{ type: "FORBIDDEN" }] }), + mutations: 1, + want: { resolved: 0, failed: 0 }, + warnings: 1, + warnMatch: /contents: write/, + }, + { + name: "FORBIDDEN nested under response.errors is recognized too", + spec: () => gqlErr("GraphQL request failed", { response: { errors: [{ type: "FORBIDDEN" }] } }), + mutations: 1, + want: { resolved: 0, failed: 0 }, + warnings: 1, + warnMatch: /contents: write/, + }, + { + name: "secondary rate limit stops the run without blaming permissions", + spec: () => gqlErr("You have exceeded a secondary rate limit", { status: 403 }), + mutations: 1, + want: { resolved: 0, failed: 0 }, + warnings: 1, + warnMatch: /rate limited/, + }, + { + name: "a generic failure tallies and keeps going", + spec: (i) => (i === 0 ? gqlErr("Something odd happened") : null), + mutations: 3, + want: { resolved: 2, failed: 1 }, + warnings: 1, + warnMatch: /failed to resolve thread A/, + }, + ]; + + for (const c of cases) { + const gh = makeGithub({ threads, resolveErrorSpec: c.spec }); + const core = mockCore(); + const out = await withEnv({ OCR_RESOLVE_DELAY: "0" }, () => + resolveOutdatedThreads(resolveArgs(gh, core)) + ); + assert.strictEqual(gh.resolveMutationCalls().length, c.mutations, c.name); + assert.strictEqual(out.resolved, c.want.resolved, `${c.name}: resolved`); + assert.strictEqual(out.failed, c.want.failed, `${c.name}: failed`); + assert.strictEqual(core.warnings.length, c.warnings, `${c.name}: warning count`); + assert.match(core.warnings[0], c.warnMatch, `${c.name}: warning text`); + assert.strictEqual(resolveLogLines(core).length, 1, `${c.name}: one summary line`); + } +} + +async function testListBotReviewThreadsPaginatesAndDegrades() { + // Multi-page walk: the cursor from pageInfo drives the next request. + const paged = makeGithub({ threadPages: [[botThread({ id: "P1" })], [botThread({ id: "P2" })]] }); + const core = mockCore(); + const all = await listBotReviewThreads({ + github: paged, + owner: "owner", + repo: "repo", + prNumber: 123, + log: (m) => core.info(m), + warn: (m) => core.warning(m), + }); + assert.deepStrictEqual(all.map((t) => t.id), ["P1", "P2"]); + + // A throwing query degrades to no threads instead of rejecting, and the + // orchestrator on top of it resolves nothing. + const broken = makeGithub({ threadsThrow: true }); + const core2 = mockCore(); + assert.deepStrictEqual( + await listBotReviewThreads({ + github: broken, + owner: "owner", + repo: "repo", + prNumber: 123, + log: (m) => core2.info(m), + warn: (m) => core2.warning(m), + }), + [] + ); + const core3 = mockCore(); + const broken2 = makeGithub({ threadsThrow: true }); + const out = await resolveOutdatedThreads(resolveArgs(broken2, core3)); + assert.strictEqual(out.resolved, 0); + assert.strictEqual(broken2.resolveMutationCalls().length, 0); + assert.strictEqual(resolveLogLines(core3).length, 1, "listing failure is a warning, not a second summary"); +} + +async function testResolveOutdatedThreadsDryRun() { + const gh = makeGithub({ + threads: [botThread({ id: "A" }), botThread({ id: "B", isResolved: true }), botThread({ id: "C", isOutdated: false })], + }); + const core = mockCore(); + const out = await resolveOutdatedThreads(resolveArgs(gh, core, { dryRun: true })); + assert.strictEqual(gh.resolveMutationCalls().length, 0, "report mode never mutates"); + assert.strictEqual(out.candidates, 1); + assert.strictEqual(out.attempted, 1); + assert.strictEqual(out.resolved, 0); + assert.deepStrictEqual(out.reasons, { resolve: 1, already_resolved: 1, not_outdated: 1 }); + assert.match(resolveLogLines(core)[0], /mode=report .*skipped=already_resolved:1,not_outdated:1/); +} + +// ---- Outdated thread resolution: wiring into a full run (#567) ---- + +const TWO_FINDINGS = { + comments: [ + { path: "src/a.js", content: "still broken", start_line: 5, end_line: 5 }, + { path: "src/a.js", content: "new problem", start_line: 42, end_line: 42 }, + ], +}; + +async function testResolveOutdatedOffByDefaultIssuesNoGraphql() { + const threads = [botThread({ id: "STALE", originalLine: 99 })]; + const base = await run({ result: TWO_FINDINGS, githubOpts: { threads } }); + assert.strictEqual(base.github.graphqlCalls.length, 0, "default off must not touch GraphQL at all"); + assert.strictEqual(base.outputs.comments_resolved, "0"); + assert.strictEqual(base.outputs.comments_resolved_preview, "0"); + + // Unrecognized values are off, not a crash and not an implicit 'true'. A + // non-empty one is also WARNED about: falling back to off is correct but + // invisible, and a workflow that says 'TRUE' would otherwise look configured + // while doing nothing. Empty/unset/'false' are the documented ways to be off, + // so they must stay silent. + for (const { value, warns } of [ + { value: "yes", warns: true }, + { value: "1", warns: true }, + { value: "TRUE", warns: true }, + { value: "false", warns: false }, + { value: "", warns: false }, + { value: undefined, warns: false }, + ]) { + const r = await run({ result: TWO_FINDINGS, opts: { resolveOutdated: value }, githubOpts: { threads } }); + assert.strictEqual(r.github.graphqlCalls.length, 0, `resolve_outdated=${value} must be off`); + assert.strictEqual(r.outputs.comments_resolved, "0", `resolve_outdated=${value}`); + const warned = r.core.warnings.filter((w) => w.includes("unrecognized resolve_outdated")); + assert.strictEqual(warned.length, warns ? 1 : 0, `resolve_outdated=${JSON.stringify(value)} warning`); + } + + // Turning the feature on changes nothing about how findings are published. + const on = await run({ result: TWO_FINDINGS, opts: { resolveOutdated: "true" }, githubOpts: { threads } }); + for (const key of ["comments_total", "comments_inline", "comments_skipped", "comments_routed", "comments_failed"]) { + assert.strictEqual(on.outputs[key], base.outputs[key], `${key} must be unaffected by resolve_outdated`); + } +} + +async function testResolveOutdatedNeverRunsOnEmptyRuns() { + const threads = [botThread({ id: "STALE", originalLine: 99 })]; + + // Unparseable OCR output: the run exits before it knows anything about the + // findings, so it must not close a single thread. + const parseFail = await run({ + result: "{ not json", + stderr: "boom", + opts: { resolveOutdated: "true" }, + githubOpts: { threads }, + }); + assert.strictEqual(parseFail.github.graphqlCalls.length, 0, "parse-failure exit must never resolve"); + assert.strictEqual(parseFail.outputs.comments_resolved, "0"); + assert.strictEqual(parseFail.outputs.comments_resolved_preview, "0"); + + // A run that reported nothing is not evidence that old findings are gone: + // no mutation AND no thread listing. + const noFindings = await run({ + result: { comments: [] }, + opts: { resolveOutdated: "true" }, + githubOpts: { threads }, + }); + assert.strictEqual(noFindings.github.graphqlCalls.length, 0, "zero-findings exit must never resolve"); + assert.strictEqual(noFindings.outputs.comments_resolved, "0"); +} + +async function testResolveOutdatedResolvesAfterASuccessfulRun() { + // Two outdated bot threads. One sits on line 5 — where this run produced a + // finding that incremental dedupe then suppressed. A suppressed finding is + // still a live finding, so that thread must survive; only line 99 is stale. + const threads = [ + botThread({ id: "COVERED", originalLine: 5 }), + botThread({ id: "STALE", originalLine: 99 }), + ]; + const { github, outputs, core } = await run({ + result: TWO_FINDINGS, + opts: { resolveOutdated: "true", incremental: true }, + githubOpts: { + threads, + history: [{ path: "src/a.js", line: 5, side: "RIGHT", user: { login: BOT } }], + }, + }); + + assert.strictEqual(outputs.comments_skipped, "1", "the line-5 finding was deduped away"); + const mutations = github.resolveMutationCalls(); + assert.strictEqual(mutations.length, 1, "only the uncovered thread is resolved"); + assert.strictEqual(mutations[0].vars.threadId, "STALE"); + assert.strictEqual(outputs.comments_resolved, "1"); + assert.strictEqual(outputs.comments_resolved_preview, "0", "preview stays 0 outside report mode"); + assert.match(resolveLogLines(core)[0], /mode=resolve .*resolved=1 failed=0 skipped=overlap:1/); +} + +async function testResolveOutdatedReportModePreviewsOnly() { + const threads = [ + botThread({ id: "STALE", originalLine: 99 }), + botThread({ id: "REPLIED", originalLine: 100, comments: { nodes: [{ author: { login: "octocat" } }] } }), + ]; + const { github, outputs, core } = await run({ + result: TWO_FINDINGS, + opts: { resolveOutdated: "report" }, + githubOpts: { threads }, + }); + assert.strictEqual(github.resolveMutationCalls().length, 0, "report mode never mutates"); + assert.strictEqual(outputs.comments_resolved, "0"); + assert.strictEqual(outputs.comments_resolved_preview, "1"); + assert.match(resolveLogLines(core)[0], /mode=report .*candidates=1 attempted=1 resolved=0 .*skipped=human_reply:1/); +} + +async function testResolveOutdatedForbiddenDoesNotFailTheRun() { + const { github, outputs, core } = await run({ + result: TWO_FINDINGS, + opts: { resolveOutdated: "true" }, + githubOpts: { + threads: [botThread({ id: "STALE", originalLine: 99 })], + resolveErrorSpec: () => + Object.assign(new Error("Resource not accessible by integration"), { + errors: [{ type: "FORBIDDEN" }], + }), + }, + }); + // The run completes normally: findings are published, the summary is + // finalized, and the permission problem is a warning, not a thrown error. + assert.strictEqual(github.resolveMutationCalls().length, 1); + assert.strictEqual(outputs.comments_resolved, "0"); + assert.strictEqual(outputs.comments_inline, "2"); + assert.strictEqual(github.updatedComments.length, 1, "summary still finalized"); + assert.strictEqual(core.warnings.length, 1); + assert.match(core.warnings[0], /contents: write/); +} + main().catch((err) => { console.error(err); process.exit(1);