From 4eca2c1ac61d4b63928d2a8c34d0b9273edb5666 Mon Sep 17 00:00:00 2001 From: Peter Dave Hello Date: Sat, 25 Jul 2026 01:17:45 +0800 Subject: [PATCH] Keep reviews aligned with current pull request diffs Use commit comparisons only to select files changed since the last review, then use their current pull request patches. Fall back to the full pull request diff when incremental comparison is unsafe. Validate review identity and hunk locations before creating comments. Propagate handler failures to GitHub Actions. --- action/index.cjs | 248 +++++++++---- action/src/bot.d.ts | 32 +- jest.config.js => jest.config.cjs | 3 + src/bot.ts | 331 ++++++++++++----- test/index.test.ts | 593 +++++++++++++++++++++++++++--- 5 files changed, 983 insertions(+), 224 deletions(-) rename jest.config.js => jest.config.cjs (81%) diff --git a/action/index.cjs b/action/index.cjs index 0779c91..f199b2b 100644 --- a/action/index.cjs +++ b/action/index.cjs @@ -147027,15 +147027,176 @@ var __importDefault = (this && this.__importDefault) || function (mod) { return (mod && mod.__esModule) ? mod : { "default": mod }; }; Object.defineProperty(exports, "__esModule", ({ value: true })); -exports.robot = void 0; +exports.robot = exports.getChangedFiles = exports.createReviewBody = exports.createInlineReviewComment = exports.isHunkHeaderInPatch = exports.getReviewCommentLocation = void 0; const minimatch_1 = __nccwpck_require__(4501); const chat_js_1 = __nccwpck_require__(85365); const loglevel_1 = __importDefault(__nccwpck_require__(78063)); const OPENAI_API_KEY = 'OPENAI_API_KEY'; +const COMPARE_FILES_LIMIT = 300; +const REVIEW_MARKER = ''; const MAX_PATCH_COUNT = process.env.MAX_PATCH_LENGTH ? +process.env.MAX_PATCH_LENGTH : Infinity; +const parseHunkHeader = (hunkHeader) => { + if (!hunkHeader) { + return null; + } + const match = hunkHeader.trim().match(/^@@\s+-\s*(\d+)(?:,(\d+))?\s+\+\s*(\d+)(?:,(\d+))?\s+@@/); + if (!match) { + return null; + } + const [, oldStartValue, oldCountValue, newStartValue, newCountValue] = match; + const oldStart = Number(oldStartValue); + const oldCount = oldCountValue === undefined ? 1 : Number(oldCountValue); + const newStart = Number(newStartValue); + const newCount = newCountValue === undefined ? 1 : Number(newCountValue); + return { oldStart, oldCount, newStart, newCount }; +}; +const getReviewCommentLocation = (hunkHeader) => { + const range = parseHunkHeader(hunkHeader); + if (!range) { + return null; + } + const { oldStart, oldCount, newStart, newCount } = range; + if (newCount > 0) { + return { + line: newStart + newCount - 1, + side: 'RIGHT', + }; + } + if (oldCount > 0) { + return { + line: oldStart + oldCount - 1, + side: 'LEFT', + }; + } + return null; +}; +exports.getReviewCommentLocation = getReviewCommentLocation; +const isHunkHeaderInPatch = (hunkHeader, patch) => { + const expectedRange = parseHunkHeader(hunkHeader); + if (!expectedRange) { + return false; + } + return patch.split('\n').some((line) => { + if (!line.startsWith('@@')) { + return false; + } + const actualRange = parseHunkHeader(line); + return (actualRange?.oldStart === expectedRange.oldStart && + actualRange.oldCount === expectedRange.oldCount && + actualRange.newStart === expectedRange.newStart && + actualRange.newCount === expectedRange.newCount); + }); +}; +exports.isHunkHeaderInPatch = isHunkHeaderInPatch; +const createInlineReviewComment = (path, body, hunkHeader, patch) => { + const location = (0, exports.getReviewCommentLocation)(hunkHeader); + if (!location || !(0, exports.isHunkHeaderInPatch)(hunkHeader, patch)) { + return null; + } + return { + path, + body, + line: location.line, + side: location.side, + }; +}; +exports.createInlineReviewComment = createInlineReviewComment; +const createReviewBody = (hasInlineComments, bodyComments) => { + const heading = hasInlineComments || bodyComments.length + ? 'Code review by ChatGPT' + : 'LGTM 👍'; + return [heading, ...bodyComments, REVIEW_MARKER].join('\n\n'); +}; +exports.createReviewBody = createReviewBody; +const getChangedFiles = async (context) => { + const repo = context.repo(); + const pullRequestFiles = await context.octokit.paginate(context.octokit.pulls.listFiles, { + owner: repo.owner, + repo: repo.repo, + pull_number: context.pullRequest().pull_number, + per_page: 100, + }); + if (context.payload.action !== 'synchronize') { + return pullRequestFiles; + } + let comparisonBase; + try { + const reviews = await context.octokit.paginate(context.octokit.pulls.listReviews, { + owner: repo.owner, + repo: repo.repo, + pull_number: context.pullRequest().pull_number, + per_page: 100, + }); + const botReview = reviews + .slice() + .reverse() + .find((r) => r.user?.type === 'Bot' && + r.body?.includes(REVIEW_MARKER)); + if (botReview) { + if (!botReview.commit_id) { + return pullRequestFiles; + } + comparisonBase = botReview.commit_id; + } + else { + const hasLegacyReview = reviews.some((r) => r.user?.type === 'Bot' && + r.body && + (r.body.startsWith('Code review by ChatGPT') || + r.body.startsWith('LGTM'))); + if (hasLegacyReview) { + return pullRequestFiles; + } + } + } + catch (err) { + loglevel_1.default.debug('failed to detect previous bot review', err); + return pullRequestFiles; + } + if (!comparisonBase) { + comparisonBase = context.payload.before; + } + if (!comparisonBase) { + return pullRequestFiles; + } + try { + const { data } = await context.octokit.repos.compareCommits({ + owner: repo.owner, + repo: repo.repo, + base: comparisonBase, + head: context.payload.pull_request.head.sha, + }); + if (data.status === 'identical') { + return []; + } + if (data.status === 'ahead') { + if (!data.files) { + loglevel_1.default.debug('commit comparison omitted files; using the full pull request diff'); + return pullRequestFiles; + } + const comparisonFiles = data.files; + if (comparisonFiles.length >= COMPARE_FILES_LIMIT) { + loglevel_1.default.debug('commit comparison reached the GitHub file limit; using the full pull request diff'); + return pullRequestFiles; + } + const changedFilenames = new Set(comparisonFiles.map((file) => file.filename)); + return pullRequestFiles.filter((file) => changedFilenames.has(file.filename)); + } + loglevel_1.default.debug(`commit comparison from ${comparisonBase} is ${data.status}; using the full pull request diff`); + } + catch (err) { + loglevel_1.default.debug(`failed to compare commits from ${comparisonBase}`, err); + } + return pullRequestFiles; +}; +exports.getChangedFiles = getChangedFiles; const robot = (app) => { + if (process.env.GITHUB_ACTIONS === 'true') { + app.onError(() => { + process.exitCode = 1; + }); + } const loadChat = async (context) => { if (process.env.USE_GITHUB_MODELS === 'true' && process.env.GITHUB_TOKEN) { return new chat_js_1.Chat(process.env.GITHUB_TOKEN); @@ -147086,61 +147247,7 @@ const robot = (app) => { loglevel_1.default.info('no target label attached'); return 'no target label attached'; } - const data = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: context.payload.pull_request.base.sha, - head: context.payload.pull_request.head.sha, - }); - let { files: changedFiles, commits } = data.data; - if (context.payload.action === 'synchronize') { - // Try to detect the last commit we reviewed (by looking for our previous review) - try { - const reviewsResp = await context.octokit.pulls.listReviews({ - owner: repo.owner, - repo: repo.repo, - pull_number: context.pullRequest().pull_number, - }); - const reviews = reviewsResp.data || []; - // Find the most recent review created by this bot (we mark our reviews with a body) - const botReview = reviews - .slice() - .reverse() - .find((r) => r.body && (r.body.startsWith('Code review by ChatGPT') || r.body.startsWith('LGTM'))); - if (botReview?.commit_id) { - const { data: { files, commits: newCommits }, } = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: botReview.commit_id, - head: context.payload.pull_request.head.sha, - }); - changedFiles = files; - commits = newCommits; - } - else if (commits.length >= 2) { - // fallback: compare last two commits in the PR - const { data: { files }, } = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: commits[commits.length - 2].sha, - head: commits[commits.length - 1].sha, - }); - changedFiles = files; - } - } - catch (err) { - loglevel_1.default.debug('failed to detect previous bot review, falling back', err); - if (commits.length >= 2) { - const { data: { files }, } = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: commits[commits.length - 2].sha, - head: commits[commits.length - 1].sha, - }); - changedFiles = files; - } - } - } + let changedFiles = await (0, exports.getChangedFiles)(context); loglevel_1.default.debug('changedFiles:', changedFiles); loglevel_1.default.debug; const ignoreList = (process.env.IGNORE || process.env.ignore || '') @@ -147173,6 +147280,7 @@ const robot = (app) => { } console.time('gpt cost'); const ress = []; + const bodyComments = []; for (let i = 0; i < changedFiles.length; i++) { const file = changedFiles[i]; const patch = file.patch || ''; @@ -147189,26 +147297,14 @@ const robot = (app) => { const reviews = Array.isArray(res) ? res : [res]; for (const review of reviews) { if (!review.lgtm && !!review.review_comment) { - let line; - let side = 'RIGHT'; - // Extract line number from hunk header if available - if (review.hunk_header) { - const c = review.hunk_header.match(/\+\s*(\d+),(\d+)/); - if (c) { - const [, start, count] = c; - line = Number(start) + Number(count) - 1; - } - else { - loglevel_1.default.error(`Failed to parse hunk header: ${review.hunk_header}`); - continue; - } + const inlineComment = (0, exports.createInlineReviewComment)(file.filename, review.review_comment, review.hunk_header, patch); + if (!inlineComment) { + const filename = file.filename.replace(/`/g, '\\`'); + bodyComments.push(`**File:** \`${filename}\`\n\n${review.review_comment}`); + loglevel_1.default.error(`Failed to locate inline review comment: ${review.hunk_header || 'missing hunk header'}`); + continue; } - ress.push({ - path: file.filename, - body: review.review_comment, - line: line, - side: side, - }); + ress.push(inlineComment); } } } @@ -147222,7 +147318,7 @@ const robot = (app) => { repo: repo.repo, owner: repo.owner, pull_number: context.pullRequest().pull_number, - body: ress.length ? "Code review by ChatGPT" : "LGTM 👍", + body: (0, exports.createReviewBody)(ress.length > 0, bodyComments), event: 'COMMENT', commit_id: context.payload.pull_request.head.sha, comments: ress, diff --git a/action/src/bot.d.ts b/action/src/bot.d.ts index a458589..8fe8f84 100644 --- a/action/src/bot.d.ts +++ b/action/src/bot.d.ts @@ -1,2 +1,32 @@ -import { Probot } from 'probot'; +import { Context, Probot } from 'probot'; +type PullRequestContext = Context<'pull_request.opened' | 'pull_request.synchronize'>; +export declare const getReviewCommentLocation: (hunkHeader?: string) => { + line: number; + side: "RIGHT"; +} | { + line: number; + side: "LEFT"; +} | null; +export declare const isHunkHeaderInPatch: (hunkHeader: string | undefined, patch: string) => boolean; +export declare const createInlineReviewComment: (path: string, body: string, hunkHeader: string | undefined, patch: string) => { + path: string; + body: string; + line: number; + side: "RIGHT" | "LEFT"; +} | null; +export declare const createReviewBody: (hasInlineComments: boolean, bodyComments: string[]) => string; +export declare const getChangedFiles: (context: PullRequestContext) => Promise<{ + sha: string; + filename: string; + status: "added" | "removed" | "modified" | "renamed" | "copied" | "changed" | "unchanged"; + additions: number; + deletions: number; + changes: number; + blob_url: string; + raw_url: string; + contents_url: string; + patch?: string | undefined; + previous_filename?: string | undefined; +}[]>; export declare const robot: (app: Probot) => void; +export {}; diff --git a/jest.config.js b/jest.config.cjs similarity index 81% rename from jest.config.js rename to jest.config.cjs index 9ee81b0..0ab9764 100644 --- a/jest.config.js +++ b/jest.config.cjs @@ -5,5 +5,8 @@ module.exports = { }, testRegex: "(/__tests__/.*|\\.(test|spec))\\.[tj]sx?$", moduleFileExtensions: ["ts", "tsx", "js", "jsx", "json", "node"], + moduleNameMapper: { + "^(\\.{1,2}/.*)\\.js$": "$1", + }, testEnvironment: "node", }; diff --git a/src/bot.ts b/src/bot.ts index 71eb783..489e85e 100644 --- a/src/bot.ts +++ b/src/bot.ts @@ -5,11 +5,234 @@ import { Chat } from './chat.js'; import log from 'loglevel'; const OPENAI_API_KEY = 'OPENAI_API_KEY'; +const COMPARE_FILES_LIMIT = 300; +const REVIEW_MARKER = ''; const MAX_PATCH_COUNT = process.env.MAX_PATCH_LENGTH ? +process.env.MAX_PATCH_LENGTH : Infinity; +type PullRequestContext = Context< + 'pull_request.opened' | 'pull_request.synchronize' +>; + +const parseHunkHeader = (hunkHeader?: string) => { + if (!hunkHeader) { + return null; + } + + const match = hunkHeader.trim().match( + /^@@\s+-\s*(\d+)(?:,(\d+))?\s+\+\s*(\d+)(?:,(\d+))?\s+@@/ + ); + if (!match) { + return null; + } + + const [, oldStartValue, oldCountValue, newStartValue, newCountValue] = match; + const oldStart = Number(oldStartValue); + const oldCount = oldCountValue === undefined ? 1 : Number(oldCountValue); + const newStart = Number(newStartValue); + const newCount = newCountValue === undefined ? 1 : Number(newCountValue); + + return { oldStart, oldCount, newStart, newCount }; +}; + +export const getReviewCommentLocation = (hunkHeader?: string) => { + const range = parseHunkHeader(hunkHeader); + if (!range) { + return null; + } + + const { oldStart, oldCount, newStart, newCount } = range; + + if (newCount > 0) { + return { + line: newStart + newCount - 1, + side: 'RIGHT' as const, + }; + } + + if (oldCount > 0) { + return { + line: oldStart + oldCount - 1, + side: 'LEFT' as const, + }; + } + + return null; +}; + +export const isHunkHeaderInPatch = ( + hunkHeader: string | undefined, + patch: string +) => { + const expectedRange = parseHunkHeader(hunkHeader); + if (!expectedRange) { + return false; + } + + return patch.split('\n').some((line) => { + if (!line.startsWith('@@')) { + return false; + } + const actualRange = parseHunkHeader(line); + return ( + actualRange?.oldStart === expectedRange.oldStart && + actualRange.oldCount === expectedRange.oldCount && + actualRange.newStart === expectedRange.newStart && + actualRange.newCount === expectedRange.newCount + ); + }); +}; + +export const createInlineReviewComment = ( + path: string, + body: string, + hunkHeader: string | undefined, + patch: string +) => { + const location = getReviewCommentLocation(hunkHeader); + if (!location || !isHunkHeaderInPatch(hunkHeader, patch)) { + return null; + } + + return { + path, + body, + line: location.line, + side: location.side, + }; +}; + +export const createReviewBody = ( + hasInlineComments: boolean, + bodyComments: string[] +) => { + const heading = + hasInlineComments || bodyComments.length + ? 'Code review by ChatGPT' + : 'LGTM 👍'; + return [heading, ...bodyComments, REVIEW_MARKER].join('\n\n'); +}; + +export const getChangedFiles = async (context: PullRequestContext) => { + const repo = context.repo(); + const pullRequestFiles = await context.octokit.paginate( + context.octokit.pulls.listFiles, + { + owner: repo.owner, + repo: repo.repo, + pull_number: context.pullRequest().pull_number, + per_page: 100, + } + ); + + if (context.payload.action !== 'synchronize') { + return pullRequestFiles; + } + + let comparisonBase: string | undefined; + + try { + const reviews = await context.octokit.paginate( + context.octokit.pulls.listReviews, + { + owner: repo.owner, + repo: repo.repo, + pull_number: context.pullRequest().pull_number, + per_page: 100, + } + ); + const botReview = reviews + .slice() + .reverse() + .find((r) => + r.user?.type === 'Bot' && + r.body?.includes(REVIEW_MARKER) + ); + + if (botReview) { + if (!botReview.commit_id) { + return pullRequestFiles; + } + comparisonBase = botReview.commit_id; + } else { + const hasLegacyReview = reviews.some( + (r) => + r.user?.type === 'Bot' && + r.body && + (r.body.startsWith('Code review by ChatGPT') || + r.body.startsWith('LGTM')) + ); + if (hasLegacyReview) { + return pullRequestFiles; + } + } + } catch (err) { + log.debug('failed to detect previous bot review', err); + return pullRequestFiles; + } + + if (!comparisonBase) { + comparisonBase = context.payload.before; + } + + if (!comparisonBase) { + return pullRequestFiles; + } + + try { + const { data } = await context.octokit.repos.compareCommits({ + owner: repo.owner, + repo: repo.repo, + base: comparisonBase, + head: context.payload.pull_request.head.sha, + }); + + if (data.status === 'identical') { + return []; + } + + if (data.status === 'ahead') { + if (!data.files) { + log.debug( + 'commit comparison omitted files; using the full pull request diff' + ); + return pullRequestFiles; + } + + const comparisonFiles = data.files; + if (comparisonFiles.length >= COMPARE_FILES_LIMIT) { + log.debug( + 'commit comparison reached the GitHub file limit; using the full pull request diff' + ); + return pullRequestFiles; + } + + const changedFilenames = new Set( + comparisonFiles.map((file) => file.filename) + ); + return pullRequestFiles.filter((file) => + changedFilenames.has(file.filename) + ); + } + + log.debug( + `commit comparison from ${comparisonBase} is ${data.status}; using the full pull request diff` + ); + } catch (err) { + log.debug(`failed to compare commits from ${comparisonBase}`, err); + } + + return pullRequestFiles; +}; + export const robot = (app: Probot) => { + if (process.env.GITHUB_ACTIONS === 'true') { + app.onError(() => { + process.exitCode = 1; + }); + } + const loadChat = async (context: Context) => { if (process.env.USE_GITHUB_MODELS === 'true' && process.env.GITHUB_TOKEN) { return new Chat(process.env.GITHUB_TOKEN); @@ -80,72 +303,7 @@ export const robot = (app: Probot) => { return 'no target label attached'; } - const data = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: context.payload.pull_request.base.sha, - head: context.payload.pull_request.head.sha, - }); - - let { files: changedFiles } = data.data; - - if (context.payload.action === 'synchronize') { - // Try to detect the last commit we reviewed (by looking for our previous review) - try { - const reviewsResp = await context.octokit.pulls.listReviews({ - owner: repo.owner, - repo: repo.repo, - pull_number: context.pullRequest().pull_number, - }); - - const reviews = reviewsResp.data || []; - // Find the most recent review created by this bot (we mark our reviews with a body) - const botReview = reviews - .slice() - .reverse() - .find((r) => r.body && (r.body.startsWith('Code review by ChatGPT') || r.body.startsWith('LGTM'))); - - if (botReview?.commit_id) { - const { - data: { files }, - } = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: botReview.commit_id, - head: context.payload.pull_request.head.sha, - }); - - changedFiles = files; - } else if (context.payload.before) { - // Use the 'before' SHA from the synchronize event to capture - // all commits in the push, not just the last one. - const { - data: { files }, - } = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: context.payload.before, - head: context.payload.pull_request.head.sha, - }); - - changedFiles = files; - } - } catch (err) { - log.debug('failed to detect previous bot review, falling back', err); - if (context.payload.before) { - const { - data: { files }, - } = await context.octokit.repos.compareCommits({ - owner: repo.owner, - repo: repo.repo, - base: context.payload.before, - head: context.payload.pull_request.head.sha, - }); - - changedFiles = files; - } - } - } + let changedFiles = await getChangedFiles(context); log.debug('changedFiles:', changedFiles); log.debug @@ -189,6 +347,7 @@ export const robot = (app: Probot) => { console.time('gpt cost'); const ress = []; + const bodyComments = []; for (let i = 0; i < changedFiles.length; i++) { const file = changedFiles[i]; @@ -211,28 +370,24 @@ export const robot = (app: Probot) => { for (const review of reviews) { if (!review.lgtm && !!review.review_comment) { - let line: number | undefined; - let side: 'LEFT' | 'RIGHT' = 'RIGHT'; - - // Extract line number from hunk header if available - if (review.hunk_header) { - const c = review.hunk_header.match(/\+\s*(\d+),(\d+)/); - if (c) { - const [, start, count] = c; - line = Number(start) + Number(count) - 1; - } - else { - log.error(`Failed to parse hunk header: ${review.hunk_header}`); - continue; - } + const inlineComment = createInlineReviewComment( + file.filename, + review.review_comment, + review.hunk_header, + patch + ); + if (!inlineComment) { + const filename = file.filename.replace(/`/g, '\\`'); + bodyComments.push( + `**File:** \`${filename}\`\n\n${review.review_comment}` + ); + log.error( + `Failed to locate inline review comment: ${review.hunk_header || 'missing hunk header'}` + ); + continue; } - ress.push({ - path: file.filename, - body: review.review_comment, - line: line, - side: side, - }) + ress.push(inlineComment) } } } catch (e) { @@ -245,7 +400,7 @@ export const robot = (app: Probot) => { repo: repo.repo, owner: repo.owner, pull_number: context.pullRequest().pull_number, - body: ress.length ? "Code review by ChatGPT" : "LGTM 👍", + body: createReviewBody(ress.length > 0, bodyComments), event: 'COMMENT', commit_id: context.payload.pull_request.head.sha, comments: ress, @@ -279,4 +434,4 @@ const matchPatterns = (patterns: string[], path: string) => { } } }) -} \ No newline at end of file +} diff --git a/test/index.test.ts b/test/index.test.ts index d3a6e55..5bee4ce 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1,74 +1,549 @@ -// You can import your modules -// import index from '../src/index' - -import nock from "nock"; -// Requiring our app implementation -import myProbotApp from "../src"; -import { Probot, ProbotOctokit } from "probot"; -// Requiring our fixtures -import payload from "./fixtures/issues.opened.json"; -const issueCreatedBody = { body: "Thanks for opening this issue!" }; -const fs = require("fs"); -const path = require("path"); - -const privateKey = fs.readFileSync( - path.join(__dirname, "fixtures/mock-cert.pem"), - "utf-8" -); - -describe("My Probot app", () => { - let probot: any; - - beforeEach(() => { - nock.disableNetConnect(); - probot = new Probot({ - appId: 123, - privateKey, - // disable request throttling and retries for testing - Octokit: ProbotOctokit.defaults({ - retry: { enabled: false }, - throttle: { enabled: false }, +import { + createInlineReviewComment, + createReviewBody, + getChangedFiles, + getReviewCommentLocation, + isHunkHeaderInPatch, + robot, +} from '../src/bot'; + +const pullRequestFiles = [ + { + filename: 'src/current.ts', + status: 'modified', + contents_url: 'https://api.github.com/repos/owner/repo/contents/src/current.ts', + patch: 'current pull request patch', + }, + { + filename: 'src/incremental.ts', + status: 'modified', + contents_url: 'https://api.github.com/repos/owner/repo/contents/src/incremental.ts', + patch: 'incremental pull request patch', + }, +]; + +const incrementalFiles = [ + { + filename: 'src/incremental.ts', + status: 'modified', + contents_url: 'https://api.github.com/repos/owner/repo/contents/src/incremental.ts', + patch: 'commit comparison patch', + }, +]; + +const unrelatedFiles = [ + { + filename: 'README.md', + status: 'modified', + contents_url: 'https://api.github.com/repos/owner/repo/contents/README.md', + patch: 'unrelated comparison patch', + }, +]; + +const patch = [ + '@@ -10,2 +10,3 @@ function example() {', + ' unchanged', + '-old', + '+new', + '+added', + '@@ -20,3 +21,0 @@ function removed() {', + '-one', + '-two', + '-three', +].join('\n'); + +const createBotReview = ( + body = 'Code review by ChatGPT', + commitId: string | null = 'reviewed-head' +) => ({ + body: `${body}\n\n`, + commit_id: commitId, + user: { type: 'Bot' }, +}); + +const createContext = ({ + action = 'synchronize', + before = 'previous-head', + reviews = [], + comparisons = [], + reviewsError, +}: { + action?: 'opened' | 'synchronize'; + before?: string; + reviews?: Array<{ + body: string; + commit_id?: string | null; + user?: { type: string }; + }>; + comparisons?: Array< + | { + status: 'ahead' | 'behind' | 'diverged' | 'identical'; + files?: typeof pullRequestFiles; + } + | Error + >; + reviewsError?: Error; +} = {}) => { + const compareCommits = jest.fn(); + const listFiles = jest.fn(); + const listReviews = jest.fn(); + const paginate = jest.fn((method) => { + if (method === listFiles) { + return Promise.resolve(pullRequestFiles); + } + if (method === listReviews) { + if (reviewsError) { + return Promise.reject(reviewsError); + } + return Promise.resolve(reviews); + } + return Promise.reject(new Error('unexpected pagination method')); + }); + + for (const comparison of comparisons) { + if (comparison instanceof Error) { + compareCommits.mockRejectedValueOnce(comparison); + } else { + compareCommits.mockResolvedValueOnce({ data: comparison }); + } + } + + return { + context: { + repo: () => ({ owner: 'owner', repo: 'repo' }), + pullRequest: () => ({ + owner: 'owner', + repo: 'repo', + pull_number: 123, }), + payload: { + action, + before, + pull_request: { + head: { sha: 'current-head' }, + }, + }, + octokit: { + paginate, + pulls: { + listFiles, + listReviews, + }, + repos: { + compareCommits, + }, + }, + } as any, + compareCommits, + listFiles, + listReviews, + paginate, + }; +}; + +describe('getChangedFiles', () => { + test('uses the current pull request files when the pull request is opened', async () => { + const { context, compareCommits } = createContext({ action: 'opened' }); + + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + expect(context.octokit.pulls.listReviews).not.toHaveBeenCalled(); + expect(compareCommits).not.toHaveBeenCalled(); + }); + + test('uses current pull request patches for files in an incremental diff', async () => { + const { context, compareCommits } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'ahead', + files: incrementalFiles, + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual([ + pullRequestFiles[1], + ]); + expect(compareCommits).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + base: 'reviewed-head', + head: 'current-head', + }); + }); + + test('ignores matching review text from a human reviewer', async () => { + const { context, compareCommits } = createContext({ + reviews: [ + { + body: 'LGTM 👍', + commit_id: 'human-reviewed-head', + user: { type: 'User' }, + }, + ], + comparisons: [ + { + status: 'ahead', + files: incrementalFiles, + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual([ + pullRequestFiles[1], + ]); + expect(compareCommits).toHaveBeenCalledTimes(1); + expect(compareCommits).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + base: 'previous-head', + head: 'current-head', }); - // Load our app into probot - probot.load(myProbotApp); }); - test("creates a comment when an issue is opened", async () => { - const mock = nock("https://api.github.com") - // Test that we correctly return a test token - .post("/app/installations/2/access_tokens") - .reply(200, { - token: "test", - permissions: { - issues: "write", + test('uses the full pull request for a legacy bot review without the marker', async () => { + const { context, compareCommits } = createContext({ + reviews: [ + { + body: 'Code review by ChatGPT', + commit_id: 'legacy-reviewed-head', + user: { type: 'Bot' }, }, - }) + ], + }); - // Test that a comment is posted - .post("/repos/hiimbex/testing-things/issues/1/comments", (body: any) => { - expect(body).toMatchObject(issueCreatedBody); - return true; - }) - .reply(200); + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + expect(compareCommits).not.toHaveBeenCalled(); + }); - // Receive a webhook event - await probot.receive({ name: "issues", payload }); + test('uses the full pull request when a trusted review has no commit', async () => { + const { context, compareCommits } = createContext({ + reviews: [createBotReview('Code review by ChatGPT', null)], + }); - expect(mock.pendingMocks()).toStrictEqual([]); + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + expect(compareCommits).not.toHaveBeenCalled(); }); - afterEach(() => { - nock.cleanAll(); - nock.enableNetConnect(); + test('uses the full pull request when review history cannot be listed', async () => { + const { context, compareCommits } = createContext({ + reviewsError: new Error('reviews unavailable'), + }); + + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + expect(compareCommits).not.toHaveBeenCalled(); + }); + + test('excludes files that are only present in an ahead comparison', async () => { + const { context } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'ahead', + files: unrelatedFiles, + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual([]); + }); + + test('falls back when a comparison reaches the GitHub file limit', async () => { + const comparisonFiles = Array.from({ length: 300 }, (_, index) => ({ + filename: `src/file-${index}.ts`, + status: 'modified', + contents_url: `https://api.github.com/repos/owner/repo/contents/src/file-${index}.ts`, + patch: 'commit comparison patch', + })); + const { context } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'ahead', + files: comparisonFiles, + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + }); + + test('falls back when an ahead comparison omits files', async () => { + const { context } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'ahead', + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + }); + + test('returns no files for an identical comparison', async () => { + const { context } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'identical', + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual([]); + }); + + test('falls back to the current pull request files after diverged comparisons', async () => { + const { context, compareCommits } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'diverged', + files: unrelatedFiles, + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + expect(compareCommits).toHaveBeenCalledTimes(1); + expect(compareCommits).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + base: 'reviewed-head', + head: 'current-head', + }); + }); + + test('uses the synchronize before SHA when no prior bot review exists', async () => { + const { context, compareCommits } = createContext({ + comparisons: [ + { + status: 'ahead', + files: incrementalFiles, + }, + ], + }); + + await expect(getChangedFiles(context)).resolves.toEqual([ + pullRequestFiles[1], + ]); + expect(compareCommits).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + base: 'previous-head', + head: 'current-head', + }); + }); + + test('uses the full pull request when the trusted review comparison fails', async () => { + const { context, compareCommits } = createContext({ + reviews: [createBotReview('LGTM 👍')], + comparisons: [new Error('reviewed commit is unavailable')], + }); + + await expect(getChangedFiles(context)).resolves.toEqual(pullRequestFiles); + expect(compareCommits).toHaveBeenCalledTimes(1); + expect(compareCommits).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + base: 'reviewed-head', + head: 'current-head', + }); + }); + + test('paginates pull request reviews before selecting the latest one', async () => { + const { context, listReviews, paginate } = createContext({ + reviews: [createBotReview()], + comparisons: [ + { + status: 'ahead', + files: incrementalFiles, + }, + ], + }); + + await getChangedFiles(context); + + expect(paginate).toHaveBeenCalledWith(listReviews, { + owner: 'owner', + repo: 'repo', + pull_number: 123, + per_page: 100, + }); }); }); -// For more information about testing with Jest see: -// https://facebook.github.io/jest/ +describe('getReviewCommentLocation', () => { + test('uses the last right-side line for a multi-line hunk', () => { + expect(getReviewCommentLocation('@@ -10,5 +10,7 @@')).toEqual({ + line: 16, + side: 'RIGHT', + }); + }); -// For more information about using TypeScript in your tests, Jest recommends: -// https://github.com/kulshekhar/ts-jest + test('defaults omitted hunk counts to one line', () => { + expect(getReviewCommentLocation('@@ -10 +12 @@ function name')).toEqual({ + line: 12, + side: 'RIGHT', + }); + }); -// For more information about testing with Nock see: -// https://github.com/nock/nock + test('uses the left side for a deletion-only hunk', () => { + expect(getReviewCommentLocation('@@ -10,3 +10,0 @@')).toEqual({ + line: 12, + side: 'LEFT', + }); + }); + + test('rejects malformed hunk headers', () => { + expect(getReviewCommentLocation('not a hunk header')).toBeNull(); + }); + + test('rejects a missing hunk header', () => { + expect(getReviewCommentLocation()).toBeNull(); + }); +}); + +describe('isHunkHeaderInPatch', () => { + test('accepts a hunk range present in the current patch', () => { + expect( + isHunkHeaderInPatch('@@ -10,2 +10,3 @@ function example() {', patch) + ).toBe(true); + }); + + test('normalizes omitted single-line counts', () => { + expect( + isHunkHeaderInPatch('@@ -30 +30 @@', '@@ -30,1 +30,1 @@') + ).toBe(true); + }); + + test('rejects a valid hunk range absent from the current patch', () => { + expect(isHunkHeaderInPatch('@@ -100,2 +100,3 @@', patch)).toBe(false); + }); + + test('does not treat a context line as a hunk header', () => { + expect( + isHunkHeaderInPatch( + '@@ -100,2 +100,3 @@', + ' @@ -100,2 +100,3 @@ example text' + ) + ).toBe(false); + }); + + test('rejects a malformed hunk header', () => { + expect(isHunkHeaderInPatch('not a hunk header', patch)).toBe(false); + }); +}); + +describe('createInlineReviewComment', () => { + test('creates a right-side comment for a hunk in the current patch', () => { + expect( + createInlineReviewComment( + 'src/file.ts', + 'Finding', + '@@ -10,2 +10,3 @@ function example() {', + patch + ) + ).toEqual({ + path: 'src/file.ts', + body: 'Finding', + line: 12, + side: 'RIGHT', + }); + }); + + test('creates a left-side comment for a deletion hunk in the patch', () => { + expect( + createInlineReviewComment( + 'src/file.ts', + 'Finding', + '@@ -20,3 +21,0 @@ function removed() {', + patch + ) + ).toEqual({ + path: 'src/file.ts', + body: 'Finding', + line: 22, + side: 'LEFT', + }); + }); + + test('rejects a hallucinated hunk range', () => { + expect( + createInlineReviewComment( + 'src/file.ts', + 'Finding', + '@@ -100,2 +100,3 @@', + patch + ) + ).toBeNull(); + }); + + test('rejects a missing hunk header', () => { + expect( + createInlineReviewComment('src/file.ts', 'Finding', undefined, patch) + ).toBeNull(); + }); +}); + +describe('createReviewBody', () => { + test('marks a review with inline comments as a ChatGPT review', () => { + expect(createReviewBody(true, [])).toBe( + 'Code review by ChatGPT\n\n' + ); + }); + + test('keeps unpositioned findings in the review body', () => { + expect(createReviewBody(false, ['**File:** `src/file.ts`\n\nFinding'])).toBe( + 'Code review by ChatGPT\n\n**File:** `src/file.ts`\n\nFinding\n\n' + ); + }); + + test('marks a review with no findings as LGTM', () => { + expect(createReviewBody(false, [])).toBe( + 'LGTM 👍\n\n' + ); + }); +}); + +describe('robot', () => { + const githubActions = process.env.GITHUB_ACTIONS; + + afterEach(() => { + if (githubActions === undefined) { + delete process.env.GITHUB_ACTIONS; + } else { + process.env.GITHUB_ACTIONS = githubActions; + } + process.exitCode = undefined; + }); + + test('marks the GitHub Action as failed when a webhook handler fails', () => { + process.env.GITHUB_ACTIONS = 'true'; + const app = { + on: jest.fn(), + onError: jest.fn(), + }; + + robot(app as any); + + expect(app.onError).toHaveBeenCalledTimes(1); + const errorHandler = app.onError.mock.calls[0][0]; + errorHandler(); + expect(process.exitCode).toBe(1); + }); + + test('does not change process error handling outside GitHub Actions', () => { + delete process.env.GITHUB_ACTIONS; + const app = { + on: jest.fn(), + onError: jest.fn(), + }; + + robot(app as any); + + expect(app.onError).not.toHaveBeenCalled(); + expect(process.exitCode).toBeUndefined(); + }); +});