From a3a5ec296db19e9e572c0b1e877793bdda1a93ee Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:24:23 -0700 Subject: [PATCH 01/14] incremental reviews --- src/review.ts | 125 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 117 insertions(+), 8 deletions(-) diff --git a/src/review.ts b/src/review.ts index 5d9f3d56..83aaa973 100644 --- a/src/review.ts +++ b/src/review.ts @@ -52,12 +52,21 @@ export const codeReview = async ( ) } + // get SUMMARIZE_TAG message + const existing_summarize_comment = await commenter.find_comment_with_tag( + SUMMARIZE_TAG, + context.payload.pull_request.number + ) + const existing_comment_ids_block = getReviewedCommitIdsBlock( + existing_summarize_comment + ) + // if the description contains ignore_keyword, skip if (inputs.description.includes(ignore_keyword)) { core.info(`Skipped: description contains ignore_keyword`) // post a comment to notify the user await commenter.comment( - `Skipped: ignored by the user`, + `Skipped: description contains ignore_keyword\n${existing_comment_ids_block}`, SUMMARIZE_TAG, 'replace' ) @@ -67,18 +76,37 @@ export const codeReview = async ( // as gpt-3.5-turbo isn't paying attention to system message, add to inputs for now inputs.system_message = options.system_message + const allCommitIds = await getAllCommitIds() + // find highest reviewed commit id + let highest_reviewed_commit_id = '' + if (existing_comment_ids_block) { + highest_reviewed_commit_id = getHighestReviewedCommitId( + allCommitIds, + getReviewedCommitIds(existing_comment_ids_block) + ) + } + + if (highest_reviewed_commit_id === '') { + core.info( + `Will review from the base commit: ${context.payload.pull_request.base.sha}` + ) + context.payload.pull_request.base.sha + } else { + core.info(`Will review from commit: ${highest_reviewed_commit_id}`) + } + // collect diff chunks const diff = await octokit.repos.compareCommits({ owner: repo.owner, repo: repo.repo, - base: context.payload.pull_request.base.sha, + base: highest_reviewed_commit_id, head: context.payload.pull_request.head.sha }) const {files, commits} = diff.data if (!files) { core.warning(`Skipped: diff.data.files is null`) await commenter.comment( - `Skipped: no files to review`, + `Skipped: no files to review\n${existing_comment_ids_block}`, SUMMARIZE_TAG, 'replace' ) @@ -349,7 +377,6 @@ ${ : '' } ` - await commenter.comment(`${summarize_comment}`, SUMMARIZE_TAG, 'replace') // final release notes @@ -658,7 +685,7 @@ ${comment_chain} // comment about skipped files for review and summarize if (skipped_files_to_review.length > 0) { // make bullet points for skipped files - const comment = ` + let comment = ` ${ skipped_files_to_review.length > 0 ? `
@@ -689,9 +716,13 @@ ${comment_chain} : '' } ` - if (comment.length > 0) { - await commenter.comment(comment, SUMMARIZE_TAG, 'append') - } + // add existing_comment_ids_block with latest head sha + comment += `\n${addReviewedCommitId( + existing_comment_ids_block, + context.payload.pull_request.head.sha + )}` + + await commenter.comment(comment, SUMMARIZE_TAG, 'append') } } @@ -883,3 +914,81 @@ function parseReview(response: string, debug = false): Review[] { return reviews } + +const commit_ids_marker_start = '' +const commit_ids_marker_end = '' + +// function that takes a comment body and returns the list of commit ids that have been reviewed +// commit ids are comments between the commit_ids_reviewed_start and commit_ids_reviewed_end markers +// +function getReviewedCommitIds(commentBody: string): string[] { + const start = commentBody.indexOf(commit_ids_marker_start) + const end = commentBody.indexOf(commit_ids_marker_end) + if (start === -1 || end === -1) { + return [] + } + const ids = commentBody.substring(start + commit_ids_marker_start.length, end) + // remove the markers from each id and extract the id + return ids.split('', '').trim()) +} + +// get review commit ids comment block from the body as a string +// including markers +function getReviewedCommitIdsBlock(commentBody: string): string { + const start = commentBody.indexOf(commit_ids_marker_start) + const end = commentBody.indexOf(commit_ids_marker_end) + if (start === -1 || end === -1) { + return '' + } + return commentBody.substring(start, end + commit_ids_marker_end.length) +} + +// add a commit id to the list of reviewed commit ids +// if the marker doesn't exist, add it +function addReviewedCommitId(commentBody: string, commitId: string): string { + const start = commentBody.indexOf(commit_ids_marker_start) + const end = commentBody.indexOf(commit_ids_marker_end) + if (start === -1 || end === -1) { + return `${commentBody}\n${commit_ids_marker_start}\n\n${commit_ids_marker_end}` + } + const ids = commentBody.substring(start + commit_ids_marker_start.length, end) + return `${commentBody.substring( + 0, + start + commit_ids_marker_start.length + )}\n${ids}\n\n${commentBody.substring(end)}` +} + +// given a list of commit ids provide the highest commit id that has been reviewed +function getHighestReviewedCommitId( + commitIds: string[], + reviewedCommitIds: string[] +): string { + for (const commitId of commitIds) { + if (reviewedCommitIds.includes(commitId)) { + return commitId + } + } + return '' +} + +async function getAllCommitIds(): Promise { + const allCommits = [] + let page = 1 + let commits + if (context && context.payload && context.payload.pull_request) { + do { + commits = await octokit.pulls.listCommits({ + owner: repo.owner, + repo: repo.repo, + pull_number: context.payload.pull_request.number, + per_page: 100, + page + }) + + allCommits.push(...commits.data.map(commit => commit.sha)) + page++ + } while (commits.data.length > 0) + } + + return allCommits +} From b11054cbbd44b50a509c0ff34cce990cf37e2168 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:24:43 -0700 Subject: [PATCH 02/14] incremental reviews --- dist/index.js | 120 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 101 insertions(+), 19 deletions(-) diff --git a/dist/index.js b/dist/index.js index fb66b08a..7890e15b 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6423,26 +6423,42 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { if (context.payload.pull_request.body) { inputs.description = commenter.get_description(context.payload.pull_request.body); } + // get SUMMARIZE_TAG message + const existing_summarize_comment = await commenter.find_comment_with_tag(lib_commenter/* SUMMARIZE_TAG */.Rp, context.payload.pull_request.number); + const existing_comment_ids_block = getReviewedCommitIdsBlock(existing_summarize_comment); // if the description contains ignore_keyword, skip if (inputs.description.includes(ignore_keyword)) { core.info(`Skipped: description contains ignore_keyword`); // post a comment to notify the user - await commenter.comment(`Skipped: ignored by the user`, lib_commenter/* SUMMARIZE_TAG */.Rp, 'replace'); + await commenter.comment(`Skipped: description contains ignore_keyword\n${existing_comment_ids_block}`, lib_commenter/* SUMMARIZE_TAG */.Rp, 'replace'); return; } // as gpt-3.5-turbo isn't paying attention to system message, add to inputs for now inputs.system_message = options.system_message; + const allCommitIds = await getAllCommitIds(); + // find highest reviewed commit id + let highest_reviewed_commit_id = ''; + if (existing_comment_ids_block) { + highest_reviewed_commit_id = getHighestReviewedCommitId(allCommitIds, getReviewedCommitIds(existing_comment_ids_block)); + } + if (highest_reviewed_commit_id === '') { + core.info(`Will review from the base commit: ${context.payload.pull_request.base.sha}`); + context.payload.pull_request.base.sha; + } + else { + core.info(`Will review from commit: ${highest_reviewed_commit_id}`); + } // collect diff chunks const diff = await octokit.repos.compareCommits({ owner: repo.owner, repo: repo.repo, - base: context.payload.pull_request.base.sha, + base: highest_reviewed_commit_id, head: context.payload.pull_request.head.sha }); const { files, commits } = diff.data; if (!files) { core.warning(`Skipped: diff.data.files is null`); - await commenter.comment(`Skipped: no files to review`, lib_commenter/* SUMMARIZE_TAG */.Rp, 'replace'); + await commenter.comment(`Skipped: no files to review\n${existing_comment_ids_block}`, lib_commenter/* SUMMARIZE_TAG */.Rp, 'replace'); return; } // skip files if they are filtered out @@ -6498,12 +6514,12 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { continue; } const hunks_str = ` ----new_hunk_for_review--- +---new_hunk--- \`\`\` ${hunks.new_hunk} \`\`\` ----old_hunk_for_context--- +---old_hunk--- \`\`\` ${hunks.old_hunk} \`\`\` @@ -6685,17 +6701,17 @@ ${summaries_failed.length > 0 // Pack instructions ins.patches += ` Format for changes: - ---new_hunk_for_review--- + ---new_hunk--- \`\`\` \`\`\` - ---old_hunk_for_context--- + ---old_hunk--- \`\`\` \`\`\` - ---comment_chains_for_context--- + ---comment_chains--- \`\`\` \`\`\` @@ -6704,9 +6720,11 @@ Format for changes: ... The above format for changes consists of multiple change sections. -Each change section consists of a new hunk (annotated with line numbers), -an old hunk (that was replaced with new hunk) and optionally, comment -chains for context. +Each change section consists of a new hunk (annotated with line numbers) +and an old hunk. Note that the code in old_hunk does not exist anymore +as it was replaced by the new hunk. The old_hunk is only included for +context. The new hunk is the code that you should review. Optionally, +existing review comment chains are included for additional context. Important instructions: - Your task is to do a line by line review of new hunks and point out @@ -6732,7 +6750,7 @@ Important instructions: - If needed, provide a replacement suggestion using fenced code blocks with the \`suggestion\` as the language identifier. The line number range in the review section must map exactly to the line number range (inclusive) - that need to be replaced within a new_hunk_for_review. + that need to be replaced within a new_hunk. For instance, if 2 lines of code in a hunk need to be replaced with 15 lines of code, the line number range must be those exact 2 lines. If an entire hunk need to be replaced with new code, then the line number range must be the @@ -6772,7 +6790,7 @@ Response format expected: ... Example changes: - ---new_hunk_for_review--- + ---new_hunk--- 1: def add(x, y): 2: z = x+y 3: retrn z @@ -6780,7 +6798,7 @@ Example changes: 5: def multiply(x, y): 6: return x * y - ---old_hunk_for_context--- + ---old_hunk--- def add(x, y): return x + y @@ -6857,7 +6875,7 @@ ${patch} `; if (comment_chain !== '') { ins.patches += ` ----comment_chains_for_review--- +---comment_chains--- \`\`\` ${comment_chain} \`\`\` @@ -6916,7 +6934,7 @@ ${comment_chain} // comment about skipped files for review and summarize if (skipped_files_to_review.length > 0) { // make bullet points for skipped files - const comment = ` + let comment = ` ${skipped_files_to_review.length > 0 ? `
Files not reviewed due to max files limit (${skipped_files_to_review.length}) @@ -6941,9 +6959,9 @@ ${comment_chain} ` : ''} `; - if (comment.length > 0) { - await commenter.comment(comment, lib_commenter/* SUMMARIZE_TAG */.Rp, 'append'); - } + // add existing_comment_ids_block with latest head sha + comment += `\n${addReviewedCommitId(existing_comment_ids_block, context.payload.pull_request.head.sha)}`; + await commenter.comment(comment, lib_commenter/* SUMMARIZE_TAG */.Rp, 'append'); } }; const split_patch = (patch) => { @@ -7095,6 +7113,70 @@ function parseReview(response, debug = false) { } return reviews; } +const commit_ids_marker_start = ''; +const commit_ids_marker_end = ''; +// function that takes a comment body and returns the list of commit ids that have been reviewed +// commit ids are comments between the commit_ids_reviewed_start and commit_ids_reviewed_end markers +// +function getReviewedCommitIds(commentBody) { + const start = commentBody.indexOf(commit_ids_marker_start); + const end = commentBody.indexOf(commit_ids_marker_end); + if (start === -1 || end === -1) { + return []; + } + const ids = commentBody.substring(start + commit_ids_marker_start.length, end); + // remove the markers from each id and extract the id + return ids.split('', '').trim()); +} +// get review commit ids comment block from the body as a string +// including markers +function getReviewedCommitIdsBlock(commentBody) { + const start = commentBody.indexOf(commit_ids_marker_start); + const end = commentBody.indexOf(commit_ids_marker_end); + if (start === -1 || end === -1) { + return ''; + } + return commentBody.substring(start, end + commit_ids_marker_end.length); +} +// add a commit id to the list of reviewed commit ids +// if the marker doesn't exist, add it +function addReviewedCommitId(commentBody, commitId) { + const start = commentBody.indexOf(commit_ids_marker_start); + const end = commentBody.indexOf(commit_ids_marker_end); + if (start === -1 || end === -1) { + return `${commentBody}\n${commit_ids_marker_start}\n\n${commit_ids_marker_end}`; + } + const ids = commentBody.substring(start + commit_ids_marker_start.length, end); + return `${commentBody.substring(0, start + commit_ids_marker_start.length)}\n${ids}\n\n${commentBody.substring(end)}`; +} +// given a list of commit ids provide the highest commit id that has been reviewed +function getHighestReviewedCommitId(commitIds, reviewedCommitIds) { + for (const commitId of commitIds) { + if (reviewedCommitIds.includes(commitId)) { + return commitId; + } + } + return ''; +} +async function getAllCommitIds() { + const allCommits = []; + let page = 1; + let commits; + if (context && context.payload && context.payload.pull_request) { + do { + commits = await octokit.pulls.listCommits({ + owner: repo.owner, + repo: repo.repo, + pull_number: context.payload.pull_request.number, + per_page: 100, + page + }); + allCommits.push(...commits.data.map(commit => commit.sha)); + page++; + } while (commits.data.length > 0); + } + return allCommits; +} /***/ }), From e15b134de0ffe95e1195442a8dde3480c2d9e1f7 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:28:22 -0700 Subject: [PATCH 03/14] incremental reviews --- dist/index.js | 5 ++++- src/review.ts | 9 ++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/dist/index.js b/dist/index.js index 7890e15b..11f8cfa3 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6425,7 +6425,10 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { } // get SUMMARIZE_TAG message const existing_summarize_comment = await commenter.find_comment_with_tag(lib_commenter/* SUMMARIZE_TAG */.Rp, context.payload.pull_request.number); - const existing_comment_ids_block = getReviewedCommitIdsBlock(existing_summarize_comment); + let existing_comment_ids_block = ''; + if (existing_summarize_comment) { + existing_comment_ids_block = getReviewedCommitIdsBlock(existing_summarize_comment); + } // if the description contains ignore_keyword, skip if (inputs.description.includes(ignore_keyword)) { core.info(`Skipped: description contains ignore_keyword`); diff --git a/src/review.ts b/src/review.ts index 83aaa973..67b26ac8 100644 --- a/src/review.ts +++ b/src/review.ts @@ -57,9 +57,12 @@ export const codeReview = async ( SUMMARIZE_TAG, context.payload.pull_request.number ) - const existing_comment_ids_block = getReviewedCommitIdsBlock( - existing_summarize_comment - ) + let existing_comment_ids_block = '' + if (existing_summarize_comment) { + existing_comment_ids_block = getReviewedCommitIdsBlock( + existing_summarize_comment + ) + } // if the description contains ignore_keyword, skip if (inputs.description.includes(ignore_keyword)) { From b2456e503df9ac5169681273e3231deb34a861d5 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:30:36 -0700 Subject: [PATCH 04/14] incremental reviews --- dist/index.js | 2 +- src/review.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index 11f8cfa3..890f0adf 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6446,7 +6446,7 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { } if (highest_reviewed_commit_id === '') { core.info(`Will review from the base commit: ${context.payload.pull_request.base.sha}`); - context.payload.pull_request.base.sha; + highest_reviewed_commit_id = context.payload.pull_request.base.sha; } else { core.info(`Will review from commit: ${highest_reviewed_commit_id}`); diff --git a/src/review.ts b/src/review.ts index 67b26ac8..a7d2851b 100644 --- a/src/review.ts +++ b/src/review.ts @@ -93,7 +93,7 @@ export const codeReview = async ( core.info( `Will review from the base commit: ${context.payload.pull_request.base.sha}` ) - context.payload.pull_request.base.sha + highest_reviewed_commit_id = context.payload.pull_request.base.sha } else { core.info(`Will review from commit: ${highest_reviewed_commit_id}`) } From 7296713713ee8d37fe9088ae5e499d80407439aa Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:36:36 -0700 Subject: [PATCH 05/14] incremental reviews --- dist/index.js | 5 ++++- src/commenter.ts | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index 890f0adf..15bf430f 100644 --- a/dist/index.js +++ b/dist/index.js @@ -3550,11 +3550,14 @@ class Commenter { if (!tag) { tag = COMMENT_TAG; } - const body = `${COMMENT_GREETING} + let body = `${message}`; + if (mode === 'create' || mode === 'replace') { + body = `${COMMENT_GREETING} ${message} ${tag}`; + } if (mode === 'create') { await this.create(body, target); } diff --git a/src/commenter.ts b/src/commenter.ts index b905a25d..0d2a5b56 100644 --- a/src/commenter.ts +++ b/src/commenter.ts @@ -49,11 +49,15 @@ export class Commenter { tag = COMMENT_TAG } - const body = `${COMMENT_GREETING} + let body = `${message}` + + if (mode === 'create' || mode === 'replace') { + body = `${COMMENT_GREETING} ${message} ${tag}` + } if (mode === 'create') { await this.create(body, target) From 7aee0b00467723e0f4a6769a189eb4bd614ba717 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:38:16 -0700 Subject: [PATCH 06/14] incremental reviews --- dist/index.js | 9 +++++---- src/review.ts | 17 +++++++++-------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/dist/index.js b/dist/index.js index 15bf430f..53bec7db 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6937,10 +6937,11 @@ ${comment_chain} } } await Promise.all(reviewPromises); + let summary_append = ''; // comment about skipped files for review and summarize if (skipped_files_to_review.length > 0) { // make bullet points for skipped files - let comment = ` + summary_append += ` ${skipped_files_to_review.length > 0 ? `
Files not reviewed due to max files limit (${skipped_files_to_review.length}) @@ -6965,10 +6966,10 @@ ${comment_chain} ` : ''} `; - // add existing_comment_ids_block with latest head sha - comment += `\n${addReviewedCommitId(existing_comment_ids_block, context.payload.pull_request.head.sha)}`; - await commenter.comment(comment, lib_commenter/* SUMMARIZE_TAG */.Rp, 'append'); } + // add existing_comment_ids_block with latest head sha + summary_append += `\n${addReviewedCommitId(existing_comment_ids_block, context.payload.pull_request.head.sha)}`; + await commenter.comment(summary_append, lib_commenter/* SUMMARIZE_TAG */.Rp, 'append'); }; const split_patch = (patch) => { if (!patch) { diff --git a/src/review.ts b/src/review.ts index a7d2851b..52abcbc8 100644 --- a/src/review.ts +++ b/src/review.ts @@ -685,10 +685,11 @@ ${comment_chain} await Promise.all(reviewPromises) + let summary_append = '' // comment about skipped files for review and summarize if (skipped_files_to_review.length > 0) { // make bullet points for skipped files - let comment = ` + summary_append += ` ${ skipped_files_to_review.length > 0 ? `
@@ -719,14 +720,14 @@ ${comment_chain} : '' } ` - // add existing_comment_ids_block with latest head sha - comment += `\n${addReviewedCommitId( - existing_comment_ids_block, - context.payload.pull_request.head.sha - )}` - - await commenter.comment(comment, SUMMARIZE_TAG, 'append') } + // add existing_comment_ids_block with latest head sha + summary_append += `\n${addReviewedCommitId( + existing_comment_ids_block, + context.payload.pull_request.head.sha + )}` + + await commenter.comment(summary_append, SUMMARIZE_TAG, 'append') } const split_patch = (patch: string | null | undefined): string[] => { From b80425e47058b3190f0ef2e59295f28cf55c9267 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:43:56 -0700 Subject: [PATCH 07/14] incremental reviews --- dist/index.js | 13 +++++++------ src/review.ts | 20 ++++++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/dist/index.js b/dist/index.js index 53bec7db..9c776e62 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6427,11 +6427,12 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { inputs.description = commenter.get_description(context.payload.pull_request.body); } // get SUMMARIZE_TAG message - const existing_summarize_comment = await commenter.find_comment_with_tag(lib_commenter/* SUMMARIZE_TAG */.Rp, context.payload.pull_request.number); - let existing_comment_ids_block = ''; - if (existing_summarize_comment) { - existing_comment_ids_block = getReviewedCommitIdsBlock(existing_summarize_comment); + const existing_summarize_cmt = await commenter.find_comment_with_tag(lib_commenter/* SUMMARIZE_TAG */.Rp, context.payload.pull_request.number); + let existing_summarize_comment = ''; + if (existing_summarize_cmt) { + existing_summarize_comment = existing_summarize_cmt.body; } + const existing_comment_ids_block = getReviewedCommitIdsBlock(existing_summarize_comment); // if the description contains ignore_keyword, skip if (inputs.description.includes(ignore_keyword)) { core.info(`Skipped: description contains ignore_keyword`); @@ -6944,7 +6945,7 @@ ${comment_chain} summary_append += ` ${skipped_files_to_review.length > 0 ? `
-Files not reviewed due to max files limit (${skipped_files_to_review.length}) +Files not reviewed due to max files limit in this run (${skipped_files_to_review.length}) ### Not reviewed @@ -6956,7 +6957,7 @@ ${comment_chain} ${reviews_failed.length > 0 ? `
-Files not reviewed due to errors (${reviews_failed.length}) +Files not reviewed due to errors in this run (${reviews_failed.length}) ### Not reviewed diff --git a/src/review.ts b/src/review.ts index 52abcbc8..777434c7 100644 --- a/src/review.ts +++ b/src/review.ts @@ -53,17 +53,19 @@ export const codeReview = async ( } // get SUMMARIZE_TAG message - const existing_summarize_comment = await commenter.find_comment_with_tag( + const existing_summarize_cmt = await commenter.find_comment_with_tag( SUMMARIZE_TAG, context.payload.pull_request.number ) - let existing_comment_ids_block = '' - if (existing_summarize_comment) { - existing_comment_ids_block = getReviewedCommitIdsBlock( - existing_summarize_comment - ) + let existing_summarize_comment = '' + if (existing_summarize_cmt) { + existing_summarize_comment = existing_summarize_cmt.body } + const existing_comment_ids_block = getReviewedCommitIdsBlock( + existing_summarize_comment + ) + // if the description contains ignore_keyword, skip if (inputs.description.includes(ignore_keyword)) { core.info(`Skipped: description contains ignore_keyword`) @@ -693,7 +695,7 @@ ${comment_chain} ${ skipped_files_to_review.length > 0 ? `
-Files not reviewed due to max files limit (${ +Files not reviewed due to max files limit in this run (${ skipped_files_to_review.length }) @@ -709,7 +711,9 @@ ${comment_chain} ${ reviews_failed.length > 0 ? `
-Files not reviewed due to errors (${reviews_failed.length}) +Files not reviewed due to errors in this run (${ + reviews_failed.length + }) ### Not reviewed From 17242abf0355fcc36610996e3e4b7e0dd30ebd5e Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 11:55:58 -0700 Subject: [PATCH 08/14] incremental reviews --- dist/index.js | 6 +++--- src/review.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dist/index.js b/dist/index.js index 9c776e62..c0d9d965 100644 --- a/dist/index.js +++ b/dist/index.js @@ -7159,9 +7159,9 @@ function addReviewedCommitId(commentBody, commitId) { } // given a list of commit ids provide the highest commit id that has been reviewed function getHighestReviewedCommitId(commitIds, reviewedCommitIds) { - for (const commitId of commitIds) { - if (reviewedCommitIds.includes(commitId)) { - return commitId; + for (let i = commitIds.length - 1; i >= 0; i--) { + if (reviewedCommitIds.includes(commitIds[i])) { + return commitIds[i]; } } return ''; diff --git a/src/review.ts b/src/review.ts index 777434c7..ecc7302d 100644 --- a/src/review.ts +++ b/src/review.ts @@ -971,9 +971,9 @@ function getHighestReviewedCommitId( commitIds: string[], reviewedCommitIds: string[] ): string { - for (const commitId of commitIds) { - if (reviewedCommitIds.includes(commitId)) { - return commitId + for (let i = commitIds.length - 1; i >= 0; i--) { + if (reviewedCommitIds.includes(commitIds[i])) { + return commitIds[i] } } return '' From b100acab71cded02595ee29887fd92b6a9c0d484 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 12:39:23 -0700 Subject: [PATCH 09/14] incremental reviews --- dist/index.js | 215 ++++++++++++++++++----------------------------- src/commenter.ts | 49 +---------- src/review.ts | 193 +++++++++++++++++++++--------------------- 3 files changed, 181 insertions(+), 276 deletions(-) diff --git a/dist/index.js b/dist/index.js index c0d9d965..887133fc 100644 --- a/dist/index.js +++ b/dist/index.js @@ -3533,7 +3533,7 @@ const DESCRIPTION_TAG = ''; class Commenter { /** - * @param mode Can be "create", "replace", "append" and "prepend". Default is "replace". + * @param mode Can be "create", "replace". Default is "replace". */ async comment(message, tag, mode) { let target; @@ -3550,26 +3550,17 @@ class Commenter { if (!tag) { tag = COMMENT_TAG; } - let body = `${message}`; - if (mode === 'create' || mode === 'replace') { - body = `${COMMENT_GREETING} + const body = `${COMMENT_GREETING} ${message} ${tag}`; - } if (mode === 'create') { await this.create(body, target); } else if (mode === 'replace') { await this.replace(body, tag, target); } - else if (mode === 'append') { - await this.append(body, tag, target); - } - else if (mode === 'prepend') { - await this.prepend(body, tag, target); - } else { _actions_core__WEBPACK_IMPORTED_MODULE_0__.warning(`Unknown mode: ${mode}, use "replace" instead`); await this.replace(body, tag, target); @@ -3846,6 +3837,7 @@ ${chain} } async create(body, target) { try { + // get commend ID from the response await octokit.issues.createComment({ owner: repo.owner, repo: repo.repo, @@ -3876,44 +3868,6 @@ ${chain} _actions_core__WEBPACK_IMPORTED_MODULE_0__.warning(`Failed to replace comment: ${e}`); } } - async append(body, tag, target) { - try { - const cmt = await this.find_comment_with_tag(tag, target); - if (cmt) { - await octokit.issues.updateComment({ - owner: repo.owner, - repo: repo.repo, - comment_id: cmt.id, - body: `${cmt.body} ${body}` - }); - } - else { - await this.create(body, target); - } - } - catch (e) { - _actions_core__WEBPACK_IMPORTED_MODULE_0__.warning(`Failed to append comment: ${e}`); - } - } - async prepend(body, tag, target) { - try { - const cmt = await this.find_comment_with_tag(tag, target); - if (cmt) { - await octokit.issues.updateComment({ - owner: repo.owner, - repo: repo.repo, - comment_id: cmt.id, - body: `${body} ${cmt.body}` - }); - } - else { - await this.create(body, target); - } - } - catch (e) { - _actions_core__WEBPACK_IMPORTED_MODULE_0__.warning(`Failed to prepend comment: ${e}`); - } - } async find_comment_with_tag(tag, target) { try { const comments = await this.list_comments(target); @@ -6628,60 +6582,6 @@ ${filename}: ${summary} } else { inputs.summary = summarize_final_response; - const summarize_comment = `${summarize_final_response} - ---- - -### Chat with 🤖 OpenAI Bot (\`@openai\`) -- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file. -- Invite the bot into a review comment chain by tagging \`@openai\` in a reply. - -### Code suggestions -- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned. -- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off. - ---- - -${filter_ignored_files.length > 0 - ? ` -
-Files ignored due to filter (${filter_ignored_files.length}) - -### Ignored files - -* ${filter_ignored_files.map(file => file.filename).join('\n* ')} - -
-` - : ''} - -${skipped_files_to_summarize.length > 0 - ? ` -
-Files not summarized due to max files limit (${skipped_files_to_summarize.length}) - -### Not summarized - -* ${skipped_files_to_summarize.join('\n* ')} - -
-` - : ''} - -${summaries_failed.length > 0 - ? ` -
-Files not summarized due to errors (${summaries_failed.length}) - -### Failed to summarize - -* ${summaries_failed.join('\n* ')} - -
-` - : ''} -`; - await commenter.comment(`${summarize_comment}`, lib_commenter/* SUMMARIZE_TAG */.Rp, 'replace'); // final release notes next_summarize_ids = summarize_final_response_ids; const [release_notes_response, release_notes_ids] = await heavyBot.chat(prompts.render_summarize_release_notes(inputs), next_summarize_ids); @@ -6695,10 +6595,6 @@ ${summaries_failed.length > 0 commenter.update_description(context.payload.pull_request.number, message); } } - if (options.summary_only === true) { - core.info('summary_only is true, exiting'); - return; - } // failed reviews array const reviews_failed = []; const do_review = async (filename, file_content, patches) => { @@ -6928,23 +6824,73 @@ ${comment_chain} }; const reviewPromises = []; const skipped_files_to_review = []; - for (const [filename, file_content, , patches] of files_to_review) { - if (options.max_files_to_review <= 0 || - reviewPromises.length < options.max_files_to_review) { - reviewPromises.push(openai_concurrency_limit(async () => do_review(filename, file_content, patches))); - } - else { - skipped_files_to_review.push(filename); + if (options.summary_only !== true) { + for (const [filename, file_content, , patches] of files_to_review) { + if (options.max_files_to_review <= 0 || + reviewPromises.length < options.max_files_to_review) { + reviewPromises.push(openai_concurrency_limit(async () => do_review(filename, file_content, patches))); + } + else { + skipped_files_to_review.push(filename); + } } + await Promise.all(reviewPromises); } - await Promise.all(reviewPromises); - let summary_append = ''; - // comment about skipped files for review and summarize - if (skipped_files_to_review.length > 0) { - // make bullet points for skipped files - summary_append += ` + let summarize_comment = `${summarize_final_response} + +--- + +### Chat with 🤖 OpenAI Bot (\`@openai\`) +- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file. +- Invite the bot into a review comment chain by tagging \`@openai\` in a reply. + +### Code suggestions +- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned. +- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off. + +--- + +${filter_ignored_files.length > 0 + ? ` +
+Files ignored due to filter (${filter_ignored_files.length}) + +### Ignored files + +* ${filter_ignored_files.map(file => file.filename).join('\n* ')} + +
+` + : ''} + +${skipped_files_to_summarize.length > 0 + ? ` +
+Files not summarized due to max files limit (${skipped_files_to_summarize.length}) + +### Not summarized + +* ${skipped_files_to_summarize.join('\n* ')} + +
+` + : ''} + +${summaries_failed.length > 0 + ? ` +
+Files not summarized due to errors (${summaries_failed.length}) + +### Failed to summarize + +* ${summaries_failed.join('\n* ')} + +
+` + : ''} + ${skipped_files_to_review.length > 0 - ? `
+ ? `
Files not reviewed due to max files limit in this run (${skipped_files_to_review.length}) ### Not reviewed @@ -6953,24 +6899,26 @@ ${comment_chain}
` - : ''} + : ''} ${reviews_failed.length > 0 - ? `
+ ? `
Files not reviewed due to errors in this run (${reviews_failed.length}) -### Not reviewed +### Failed to review * ${reviews_failed.join('\n* ')}
` - : ''} - `; + : ''} + +`; + if (options.summary_only !== true) { + // add existing_comment_ids_block with latest head sha + summarize_comment += `\n${addReviewedCommitId(existing_comment_ids_block, context.payload.pull_request.head.sha)}`; } - // add existing_comment_ids_block with latest head sha - summary_append += `\n${addReviewedCommitId(existing_comment_ids_block, context.payload.pull_request.head.sha)}`; - await commenter.comment(summary_append, lib_commenter/* SUMMARIZE_TAG */.Rp, 'append'); + await commenter.comment(`${summarize_comment}`, lib_commenter/* SUMMARIZE_TAG */.Rp, 'replace'); }; const split_patch = (patch) => { if (!patch) { @@ -7133,8 +7081,11 @@ function getReviewedCommitIds(commentBody) { return []; } const ids = commentBody.substring(start + commit_ids_marker_start.length, end); - // remove the markers from each id and extract the id - return ids.split('', '').trim()); + // remove the markers from each id and extract the id and remove empty strings + return ids + .split('', '').trim()) + .filter(id => id !== ''); } // get review commit ids comment block from the body as a string // including markers @@ -7155,7 +7106,7 @@ function addReviewedCommitId(commentBody, commitId) { return `${commentBody}\n${commit_ids_marker_start}\n\n${commit_ids_marker_end}`; } const ids = commentBody.substring(start + commit_ids_marker_start.length, end); - return `${commentBody.substring(0, start + commit_ids_marker_start.length)}\n${ids}\n\n${commentBody.substring(end)}`; + return `${commentBody.substring(0, start + commit_ids_marker_start.length)}${ids}\n\n${commentBody.substring(end)}`; } // given a list of commit ids provide the highest commit id that has been reviewed function getHighestReviewedCommitId(commitIds, reviewedCommitIds) { diff --git a/src/commenter.ts b/src/commenter.ts index 0d2a5b56..87199570 100644 --- a/src/commenter.ts +++ b/src/commenter.ts @@ -30,7 +30,7 @@ export const DESCRIPTION_TAG_END = export class Commenter { /** - * @param mode Can be "create", "replace", "append" and "prepend". Default is "replace". + * @param mode Can be "create", "replace". Default is "replace". */ async comment(message: string, tag: string, mode: string) { let target: number @@ -49,24 +49,16 @@ export class Commenter { tag = COMMENT_TAG } - let body = `${message}` - - if (mode === 'create' || mode === 'replace') { - body = `${COMMENT_GREETING} + const body = `${COMMENT_GREETING} ${message} ${tag}` - } if (mode === 'create') { await this.create(body, target) } else if (mode === 'replace') { await this.replace(body, tag, target) - } else if (mode === 'append') { - await this.append(body, tag, target) - } else if (mode === 'prepend') { - await this.prepend(body, tag, target) } else { core.warning(`Unknown mode: ${mode}, use "replace" instead`) await this.replace(body, tag, target) @@ -428,6 +420,7 @@ ${chain} async create(body: string, target: number) { try { + // get commend ID from the response await octokit.issues.createComment({ owner: repo.owner, repo: repo.repo, @@ -457,42 +450,6 @@ ${chain} } } - async append(body: string, tag: string, target: number) { - try { - const cmt = await this.find_comment_with_tag(tag, target) - if (cmt) { - await octokit.issues.updateComment({ - owner: repo.owner, - repo: repo.repo, - comment_id: cmt.id, - body: `${cmt.body} ${body}` - }) - } else { - await this.create(body, target) - } - } catch (e) { - core.warning(`Failed to append comment: ${e}`) - } - } - - async prepend(body: string, tag: string, target: number) { - try { - const cmt = await this.find_comment_with_tag(tag, target) - if (cmt) { - await octokit.issues.updateComment({ - owner: repo.owner, - repo: repo.repo, - comment_id: cmt.id, - body: `${body} ${cmt.body}` - }) - } else { - await this.create(body, target) - } - } catch (e) { - core.warning(`Failed to prepend comment: ${e}`) - } - } - async find_comment_with_tag(tag: string, target: number) { try { const comments = await this.list_comments(target) diff --git a/src/review.ts b/src/review.ts index ecc7302d..fda6ab6d 100644 --- a/src/review.ts +++ b/src/review.ts @@ -319,71 +319,6 @@ ${filename}: ${summary} } else { inputs.summary = summarize_final_response - const summarize_comment = `${summarize_final_response} - ---- - -### Chat with 🤖 OpenAI Bot (\`@openai\`) -- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file. -- Invite the bot into a review comment chain by tagging \`@openai\` in a reply. - -### Code suggestions -- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned. -- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off. - ---- - -${ - filter_ignored_files.length > 0 - ? ` -
-Files ignored due to filter (${filter_ignored_files.length}) - -### Ignored files - -* ${filter_ignored_files.map(file => file.filename).join('\n* ')} - -
-` - : '' -} - -${ - skipped_files_to_summarize.length > 0 - ? ` -
-Files not summarized due to max files limit (${ - skipped_files_to_summarize.length - }) - -### Not summarized - -* ${skipped_files_to_summarize.join('\n* ')} - -
-` - : '' -} - -${ - summaries_failed.length > 0 - ? ` -
-Files not summarized due to errors (${ - summaries_failed.length - }) - -### Failed to summarize - -* ${summaries_failed.join('\n* ')} - -
-` - : '' -} -` - await commenter.comment(`${summarize_comment}`, SUMMARIZE_TAG, 'replace') - // final release notes next_summarize_ids = summarize_final_response_ids const [release_notes_response, release_notes_ids] = await heavyBot.chat( @@ -400,11 +335,6 @@ ${ } } - if (options.summary_only === true) { - core.info('summary_only is true, exiting') - return - } - // failed reviews array const reviews_failed: string[] = [] const do_review = async ( @@ -670,28 +600,89 @@ ${comment_chain} const reviewPromises = [] const skipped_files_to_review = [] - for (const [filename, file_content, , patches] of files_to_review) { - if ( - options.max_files_to_review <= 0 || - reviewPromises.length < options.max_files_to_review - ) { - reviewPromises.push( - openai_concurrency_limit(async () => - do_review(filename, file_content, patches) + + if (options.summary_only !== true) { + for (const [filename, file_content, , patches] of files_to_review) { + if ( + options.max_files_to_review <= 0 || + reviewPromises.length < options.max_files_to_review + ) { + reviewPromises.push( + openai_concurrency_limit(async () => + do_review(filename, file_content, patches) + ) ) - ) - } else { - skipped_files_to_review.push(filename) + } else { + skipped_files_to_review.push(filename) + } } + + await Promise.all(reviewPromises) } - await Promise.all(reviewPromises) + let summarize_comment = `${summarize_final_response} + +--- + +### Chat with 🤖 OpenAI Bot (\`@openai\`) +- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file. +- Invite the bot into a review comment chain by tagging \`@openai\` in a reply. + +### Code suggestions +- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned. +- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off. + +--- + +${ + filter_ignored_files.length > 0 + ? ` +
+Files ignored due to filter (${filter_ignored_files.length}) + +### Ignored files + +* ${filter_ignored_files.map(file => file.filename).join('\n* ')} + +
+` + : '' +} + +${ + skipped_files_to_summarize.length > 0 + ? ` +
+Files not summarized due to max files limit (${ + skipped_files_to_summarize.length + }) + +### Not summarized + +* ${skipped_files_to_summarize.join('\n* ')} + +
+` + : '' +} + +${ + summaries_failed.length > 0 + ? ` +
+Files not summarized due to errors (${ + summaries_failed.length + }) + +### Failed to summarize + +* ${summaries_failed.join('\n* ')} + +
+` + : '' +} - let summary_append = '' - // comment about skipped files for review and summarize - if (skipped_files_to_review.length > 0) { - // make bullet points for skipped files - summary_append += ` ${ skipped_files_to_review.length > 0 ? `
@@ -715,7 +706,7 @@ ${comment_chain} reviews_failed.length })
 -### Not reviewed +### Failed to review * ${reviews_failed.join('\n* ')} @@ -723,15 +714,18 @@ ${comment_chain} ` : '' } - ` + +` + + if (options.summary_only !== true) { + // add existing_comment_ids_block with latest head sha + summarize_comment += `\n${addReviewedCommitId( + existing_comment_ids_block, + context.payload.pull_request.head.sha + )}` } - // add existing_comment_ids_block with latest head sha - summary_append += `\n${addReviewedCommitId( - existing_comment_ids_block, - context.payload.pull_request.head.sha - )}` - await commenter.comment(summary_append, SUMMARIZE_TAG, 'append') + await commenter.comment(`${summarize_comment}`, SUMMARIZE_TAG, 'replace') } const split_patch = (patch: string | null | undefined): string[] => { @@ -936,8 +930,11 @@ function getReviewedCommitIds(commentBody: string): string[] { return [] } const ids = commentBody.substring(start + commit_ids_marker_start.length, end) - // remove the markers from each id and extract the id - return ids.split('', '').trim()) + // remove the markers from each id and extract the id and remove empty strings + return ids + .split('', '').trim()) + .filter(id => id !== '') } // get review commit ids comment block from the body as a string @@ -963,7 +960,7 @@ function addReviewedCommitId(commentBody: string, commitId: string): string { return `${commentBody.substring( 0, start + commit_ids_marker_start.length - )}\n${ids}\n\n${commentBody.substring(end)}` + )}${ids}\n\n${commentBody.substring(end)}` } // given a list of commit ids provide the highest commit id that has been reviewed From 6088c67761fdf874b67f4aa80b13f1d1ff3371b6 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 12:45:48 -0700 Subject: [PATCH 10/14] incremental reviews --- dist/index.js | 8 ++++---- src/review.ts | 30 +++++++++++++++--------------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/dist/index.js b/dist/index.js index 887133fc..dc0f63cf 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6889,8 +6889,9 @@ ${summaries_failed.length > 0 ` : ''} - ${skipped_files_to_review.length > 0 - ? `
+${skipped_files_to_review.length > 0 + ? ` +
Files not reviewed due to max files limit in this run (${skipped_files_to_review.length}) ### Not reviewed @@ -6901,7 +6902,7 @@ ${summaries_failed.length > 0 ` : ''} - ${reviews_failed.length > 0 +${reviews_failed.length > 0 ? `
Files not reviewed due to errors in this run (${reviews_failed.length}) @@ -6912,7 +6913,6 @@ ${summaries_failed.length > 0
` : ''} - `; if (options.summary_only !== true) { // add existing_comment_ids_block with latest head sha diff --git a/src/review.ts b/src/review.ts index fda6ab6d..3969e6c4 100644 --- a/src/review.ts +++ b/src/review.ts @@ -683,12 +683,13 @@ ${ : '' } - ${ - skipped_files_to_review.length > 0 - ? `
+${ + skipped_files_to_review.length > 0 + ? ` +
Files not reviewed due to max files limit in this run (${ - skipped_files_to_review.length - }) + skipped_files_to_review.length + }) ### Not reviewed @@ -696,15 +697,15 @@ ${
` - : '' - } + : '' +} - ${ - reviews_failed.length > 0 - ? `
+${ + reviews_failed.length > 0 + ? `
Files not reviewed due to errors in this run (${ - reviews_failed.length - }) + reviews_failed.length + }) ### Failed to review @@ -712,9 +713,8 @@ ${
` - : '' - } - + : '' +} ` if (options.summary_only !== true) { From 3f3a00cbd10b909ee631398aa873a1ea1be19423 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 12:52:12 -0700 Subject: [PATCH 11/14] incremental reviews --- dist/index.js | 2 +- src/review.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index dc0f63cf..002c0510 100644 --- a/dist/index.js +++ b/dist/index.js @@ -7106,7 +7106,7 @@ function addReviewedCommitId(commentBody, commitId) { return `${commentBody}\n${commit_ids_marker_start}\n\n${commit_ids_marker_end}`; } const ids = commentBody.substring(start + commit_ids_marker_start.length, end); - return `${commentBody.substring(0, start + commit_ids_marker_start.length)}${ids}\n\n${commentBody.substring(end)}`; + return `${commentBody.substring(0, start + commit_ids_marker_start.length)}${ids}\n${commentBody.substring(end)}`; } // given a list of commit ids provide the highest commit id that has been reviewed function getHighestReviewedCommitId(commitIds, reviewedCommitIds) { diff --git a/src/review.ts b/src/review.ts index 3969e6c4..22d13dd6 100644 --- a/src/review.ts +++ b/src/review.ts @@ -960,7 +960,7 @@ function addReviewedCommitId(commentBody: string, commitId: string): string { return `${commentBody.substring( 0, start + commit_ids_marker_start.length - )}${ids}\n\n${commentBody.substring(end)}` + )}${ids}\n${commentBody.substring(end)}` } // given a list of commit ids provide the highest commit id that has been reviewed From 5e4fdcd9c36d279213453bb864e012cab817532e Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 13:12:59 -0700 Subject: [PATCH 12/14] incremental reviews --- dist/index.js | 1 + src/review.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/dist/index.js b/dist/index.js index 002c0510..a5aae0d2 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6755,6 +6755,7 @@ Changes for review are below: try { const all_chains = await commenter.get_comment_chains_within_range(context.payload.pull_request.number, filename, start_line, end_line, lib_commenter/* COMMENT_REPLY_TAG */.aD); if (all_chains.length > 0) { + core.info(`Found comment chains: ${all_chains} for ${filename}`); comment_chain = all_chains; } else { diff --git a/src/review.ts b/src/review.ts index 22d13dd6..094cdc65 100644 --- a/src/review.ts +++ b/src/review.ts @@ -516,6 +516,7 @@ Changes for review are below: ) if (all_chains.length > 0) { + core.info(`Found comment chains: ${all_chains} for ${filename}`) comment_chain = all_chains } else { comment_chain = '' From 6704a1774860e5dc59b14defbdcf2e6689e82b1e Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 13:35:44 -0700 Subject: [PATCH 13/14] incremental reviews --- dist/index.js | 59 ++++++++++++++++++++------------- src/review.ts | 90 ++++++++++++++++++++++++++++++++++----------------- 2 files changed, 97 insertions(+), 52 deletions(-) diff --git a/dist/index.js b/dist/index.js index a5aae0d2..7f9635bc 100644 --- a/dist/index.js +++ b/dist/index.js @@ -6396,24 +6396,11 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { } // as gpt-3.5-turbo isn't paying attention to system message, add to inputs for now inputs.system_message = options.system_message; - const allCommitIds = await getAllCommitIds(); - // find highest reviewed commit id - let highest_reviewed_commit_id = ''; - if (existing_comment_ids_block) { - highest_reviewed_commit_id = getHighestReviewedCommitId(allCommitIds, getReviewedCommitIds(existing_comment_ids_block)); - } - if (highest_reviewed_commit_id === '') { - core.info(`Will review from the base commit: ${context.payload.pull_request.base.sha}`); - highest_reviewed_commit_id = context.payload.pull_request.base.sha; - } - else { - core.info(`Will review from commit: ${highest_reviewed_commit_id}`); - } // collect diff chunks const diff = await octokit.repos.compareCommits({ owner: repo.owner, repo: repo.repo, - base: highest_reviewed_commit_id, + base: context.payload.pull_request.base.sha, head: context.payload.pull_request.head.sha }); const { files, commits } = diff.data; @@ -6435,7 +6422,7 @@ const codeReview = async (lightBot, heavyBot, options, prompts) => { } } // find hunks to review - const filtered_files_to_review = await Promise.all(filter_selected_files.map(async (file) => { + const filtered_files = await Promise.all(filter_selected_files.map(async (file) => { // retrieve file contents let file_content = ''; if (!context.payload.pull_request) { @@ -6499,8 +6486,8 @@ ${hunks.old_hunk} } })); // Filter out any null results - const files_to_review = filtered_files_to_review.filter(file => file !== null); - if (files_to_review.length === 0) { + const files_and_changes = filtered_files.filter(file => file !== null); + if (files_and_changes.length === 0) { core.error(`Skipped: no files to review`); return; } @@ -6555,7 +6542,7 @@ ${hunks.old_hunk} }; const summaryPromises = []; const skipped_files_to_summarize = []; - for (const [filename, file_content, file_diff] of files_to_review) { + for (const [filename, file_content, file_diff] of files_and_changes) { if (options.max_files_to_summarize <= 0 || summaryPromises.length < options.max_files_to_summarize) { summaryPromises.push(openai_concurrency_limit(async () => do_summary(filename, file_content, file_diff))); @@ -6758,9 +6745,6 @@ Changes for review are below: core.info(`Found comment chains: ${all_chains} for ${filename}`); comment_chain = all_chains; } - else { - comment_chain = ''; - } } catch (e) { core.warning(`Failed to get comments: ${e}, skipping. backtrace: ${e.stack}`); @@ -6826,7 +6810,38 @@ ${comment_chain} const reviewPromises = []; const skipped_files_to_review = []; if (options.summary_only !== true) { - for (const [filename, file_content, , patches] of files_to_review) { + const allCommitIds = await getAllCommitIds(); + // find highest reviewed commit id + let highest_reviewed_commit_id = ''; + if (existing_comment_ids_block) { + highest_reviewed_commit_id = getHighestReviewedCommitId(allCommitIds, getReviewedCommitIds(existing_comment_ids_block)); + } + let files_and_changes_for_review = files_and_changes; + if (highest_reviewed_commit_id === '') { + core.info(`Will review from the base commit: ${context.payload.pull_request.base.sha}`); + highest_reviewed_commit_id = context.payload.pull_request.base.sha; + } + else { + core.info(`Will review from commit: ${highest_reviewed_commit_id}`); + // get the list of files changed between the highest reviewed commit + // and the latest (head) commit + // use octokit.pulls.compareCommits to get the list of files changed + // between the highest reviewed commit and the latest (head) commit + const diff_since_last_review = await octokit.repos.compareCommits({ + owner: repo.owner, + repo: repo.repo, + base: highest_reviewed_commit_id, + head: context.payload.pull_request.head.sha + }); + if (diff_since_last_review && + diff_since_last_review.data && + diff_since_last_review.data.files) { + const files_changed = diff_since_last_review.data.files.map((file) => file.filename); + core.info(`Files changed since last review: ${files_changed}`); + files_and_changes_for_review = files_and_changes.filter(([filename]) => files_changed.includes(filename)); + } + } + for (const [filename, file_content, , patches] of files_and_changes_for_review) { if (options.max_files_to_review <= 0 || reviewPromises.length < options.max_files_to_review) { reviewPromises.push(openai_concurrency_limit(async () => do_review(filename, file_content, patches))); diff --git a/src/review.ts b/src/review.ts index 094cdc65..4ff5e90b 100644 --- a/src/review.ts +++ b/src/review.ts @@ -81,30 +81,11 @@ export const codeReview = async ( // as gpt-3.5-turbo isn't paying attention to system message, add to inputs for now inputs.system_message = options.system_message - const allCommitIds = await getAllCommitIds() - // find highest reviewed commit id - let highest_reviewed_commit_id = '' - if (existing_comment_ids_block) { - highest_reviewed_commit_id = getHighestReviewedCommitId( - allCommitIds, - getReviewedCommitIds(existing_comment_ids_block) - ) - } - - if (highest_reviewed_commit_id === '') { - core.info( - `Will review from the base commit: ${context.payload.pull_request.base.sha}` - ) - highest_reviewed_commit_id = context.payload.pull_request.base.sha - } else { - core.info(`Will review from commit: ${highest_reviewed_commit_id}`) - } - // collect diff chunks const diff = await octokit.repos.compareCommits({ owner: repo.owner, repo: repo.repo, - base: highest_reviewed_commit_id, + base: context.payload.pull_request.base.sha, head: context.payload.pull_request.head.sha }) const {files, commits} = diff.data @@ -131,7 +112,7 @@ export const codeReview = async ( } // find hunks to review - const filtered_files_to_review: ( + const filtered_files: ( | [string, string, string, [number, number, string][]] | null )[] = await Promise.all( @@ -204,11 +185,14 @@ ${hunks.old_hunk} ) // Filter out any null results - const files_to_review = filtered_files_to_review.filter( - file => file !== null - ) as [string, string, string, [number, number, string][]][] - - if (files_to_review.length === 0) { + const files_and_changes = filtered_files.filter(file => file !== null) as [ + string, + string, + string, + [number, number, string][] + ][] + + if (files_and_changes.length === 0) { core.error(`Skipped: no files to review`) return } @@ -280,7 +264,7 @@ ${hunks.old_hunk} const summaryPromises = [] const skipped_files_to_summarize = [] - for (const [filename, file_content, file_diff] of files_to_review) { + for (const [filename, file_content, file_diff] of files_and_changes) { if ( options.max_files_to_summarize <= 0 || summaryPromises.length < options.max_files_to_summarize @@ -518,8 +502,6 @@ Changes for review are below: if (all_chains.length > 0) { core.info(`Found comment chains: ${all_chains} for ${filename}`) comment_chain = all_chains - } else { - comment_chain = '' } } catch (e: any) { core.warning( @@ -603,7 +585,55 @@ ${comment_chain} const skipped_files_to_review = [] if (options.summary_only !== true) { - for (const [filename, file_content, , patches] of files_to_review) { + const allCommitIds = await getAllCommitIds() + // find highest reviewed commit id + let highest_reviewed_commit_id = '' + if (existing_comment_ids_block) { + highest_reviewed_commit_id = getHighestReviewedCommitId( + allCommitIds, + getReviewedCommitIds(existing_comment_ids_block) + ) + } + + let files_and_changes_for_review = files_and_changes + if (highest_reviewed_commit_id === '') { + core.info( + `Will review from the base commit: ${context.payload.pull_request.base.sha}` + ) + highest_reviewed_commit_id = context.payload.pull_request.base.sha + } else { + core.info(`Will review from commit: ${highest_reviewed_commit_id}`) + // get the list of files changed between the highest reviewed commit + // and the latest (head) commit + // use octokit.pulls.compareCommits to get the list of files changed + // between the highest reviewed commit and the latest (head) commit + const diff_since_last_review = await octokit.repos.compareCommits({ + owner: repo.owner, + repo: repo.repo, + base: highest_reviewed_commit_id, + head: context.payload.pull_request.head.sha + }) + if ( + diff_since_last_review && + diff_since_last_review.data && + diff_since_last_review.data.files + ) { + const files_changed = diff_since_last_review.data.files.map( + (file: any) => file.filename + ) + core.info(`Files changed since last review: ${files_changed}`) + files_and_changes_for_review = files_and_changes.filter(([filename]) => + files_changed.includes(filename) + ) + } + } + + for (const [ + filename, + file_content, + , + patches + ] of files_and_changes_for_review) { if ( options.max_files_to_review <= 0 || reviewPromises.length < options.max_files_to_review From ad5140154ae79864d47902d2fd7b6f24362826c1 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Mon, 17 Apr 2023 13:44:44 -0700 Subject: [PATCH 14/14] incremental reviews --- dist/index.js | 6 +++--- src/options.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dist/index.js b/dist/index.js index 7f9635bc..d33e0067 100644 --- a/dist/index.js +++ b/dist/index.js @@ -5881,15 +5881,15 @@ class TokenLimits { response_tokens; constructor(model = 'gpt-3.5-turbo') { if (model === 'gpt-4-32k') { - this.max_tokens = 32700; + this.max_tokens = 32600; this.response_tokens = 4000; } else if (model === 'gpt-4') { - this.max_tokens = 8100; + this.max_tokens = 8000; this.response_tokens = 2000; } else { - this.max_tokens = 4000; + this.max_tokens = 3900; this.response_tokens = 1000; } this.request_tokens = this.max_tokens - this.response_tokens; diff --git a/src/options.ts b/src/options.ts index 2be6e7b6..d46d16b9 100644 --- a/src/options.ts +++ b/src/options.ts @@ -146,13 +146,13 @@ export class TokenLimits { constructor(model = 'gpt-3.5-turbo') { if (model === 'gpt-4-32k') { - this.max_tokens = 32700 + this.max_tokens = 32600 this.response_tokens = 4000 } else if (model === 'gpt-4') { - this.max_tokens = 8100 + this.max_tokens = 8000 this.response_tokens = 2000 } else { - this.max_tokens = 4000 + this.max_tokens = 3900 this.response_tokens = 1000 } this.request_tokens = this.max_tokens - this.response_tokens