From d4e73a5cae83ee7ab02a1ca6cf4c21bde15088e5 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Sun, 23 Apr 2023 09:56:25 -0700 Subject: [PATCH 1/5] provide option to review simple changes --- README.md | 6 +++++- action.yml | 4 ++++ dist/index.js | 53 ++++++++++++++++++++++++++++---------------------- src/main.ts | 1 + src/options.ts | 4 ++++ src/prompts.ts | 15 ++++++++++---- src/review.ts | 39 ++++++++++++++++++++----------------- 7 files changed, 76 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 4c4aaa21..751c15d9 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,7 @@ jobs: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} with: debug: false + review_simple_changes: false review_comment_lgtm: false ``` @@ -101,7 +102,9 @@ To ignore a PR, add the following keyword in the PR description: - `OPENAI_API_KEY`: use this to authenticate with OpenAI API. You can get one [here](https://platform.openai.com/account/api-keys). Please add this key to your GitHub Action secrets. -- `OPENAI_API_ORG`: (optional) use this to use the specified organisation with OpenAI API if you have multiple. Please add this key to your GitHub Action secrets. +- `OPENAI_API_ORG`: (optional) use this to use the specified organisation with + OpenAI API if you have multiple. Please add this key to your GitHub Action + secrets. ### Models: `gpt-4` and `gpt-3.5-turbo` @@ -217,6 +220,7 @@ jobs: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} with: debug: false + review_simple_changes: false review_comment_lgtm: false ``` diff --git a/action.yml b/action.yml index 6e19c23d..aa1cfca7 100644 --- a/action.yml +++ b/action.yml @@ -15,6 +15,10 @@ inputs: 'Max files to summarize and review. Less than or equal to 0 means no limit.' default: '150' + review_simple_changes: + required: false + description: 'Review even when the changes are simple' + default: 'false' review_comment_lgtm: required: false description: 'Leave comments even if the patch is LGTM' diff --git a/dist/index.js b/dist/index.js index 84dfb323..5f6b853a 100644 --- a/dist/index.js +++ b/dist/index.js @@ -4123,7 +4123,7 @@ __nccwpck_require__.r(__webpack_exports__); async function run() { - const options = new _options__WEBPACK_IMPORTED_MODULE_2__/* .Options */ .Ei((0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('debug'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('summary_only'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('max_files'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('review_comment_lgtm'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getMultilineInput)('path_filters'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('system_message'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_light_model'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_heavy_model'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_model_temperature'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_retries'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_timeout_ms'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_concurrency_limit')); + const options = new _options__WEBPACK_IMPORTED_MODULE_2__/* .Options */ .Ei((0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('debug'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('summary_only'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('max_files'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('review_simple_changes'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getBooleanInput)('review_comment_lgtm'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getMultilineInput)('path_filters'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('system_message'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_light_model'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_heavy_model'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_model_temperature'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_retries'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_timeout_ms'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('openai_concurrency_limit')); // print options options.print(); const prompts = new _prompts__WEBPACK_IMPORTED_MODULE_5__/* .Prompts */ .j((0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('summarize'), (0,_actions_core__WEBPACK_IMPORTED_MODULE_0__.getInput)('summarize_release_notes')); @@ -6035,6 +6035,7 @@ class Options { debug; summaryOnly; maxFiles; + reviewSimpleChanges; reviewCommentLGTM; pathFilters; systemMessage; @@ -6046,10 +6047,11 @@ class Options { openaiConcurrencyLimit; lightTokenLimits; heavyTokenLimits; - constructor(debug, summaryOnly, maxFiles = '0', reviewCommentLGTM = false, pathFilters = null, systemMessage = '', openaiLightModel = 'gpt-3.5-turbo', openaiHeavyModel = 'gpt-3.5-turbo', openaiModelTemperature = '0.0', openaiRetries = '3', openaiTimeoutMS = '120000', openaiConcurrencyLimit = '4') { + constructor(debug, summaryOnly, maxFiles = '0', reviewSimpleChanges = false, reviewCommentLGTM = false, pathFilters = null, systemMessage = '', openaiLightModel = 'gpt-3.5-turbo', openaiHeavyModel = 'gpt-3.5-turbo', openaiModelTemperature = '0.0', openaiRetries = '3', openaiTimeoutMS = '120000', openaiConcurrencyLimit = '4') { this.debug = debug; this.summaryOnly = summaryOnly; this.maxFiles = parseInt(maxFiles); + this.reviewSimpleChanges = reviewSimpleChanges; this.reviewCommentLGTM = reviewCommentLGTM; this.pathFilters = new PathFilter(pathFilters); this.systemMessage = systemMessage; @@ -6067,6 +6069,7 @@ class Options { (0,core.info)(`debug: ${this.debug}`); (0,core.info)(`summary_only: ${this.summaryOnly}`); (0,core.info)(`max_files: ${this.maxFiles}`); + (0,core.info)(`review_simple_changes: ${this.reviewSimpleChanges}`); (0,core.info)(`review_comment_lgtm: ${this.reviewCommentLGTM}`); (0,core.info)(`path_filters: ${this.pathFilters}`); (0,core.info)(`system_message: ${this.systemMessage}`); @@ -6172,8 +6175,8 @@ $file_diff \`\`\` I would like you to summarize the diff within 50 words. - -Below the summary, I would also like you to triage the diff as \`NEEDS_REVIEW\` or +`; + triageFileDiff = `Below the summary, I would also like you to triage the diff as \`NEEDS_REVIEW\` or \`APPROVED\` based on the following criteria: - If the diff involves any modifications to the logic or functionality, even if they @@ -6394,8 +6397,12 @@ $patches this.summarize = summarize; this.summarizeReleaseNotes = summarizeReleaseNotes; } - renderSummarizeFileDiff(inputs) { - return inputs.render(this.summarizeFileDiff); + renderSummarizeFileDiff(inputs, reviewSimpleChanges) { + let prompt = this.summarizeFileDiff; + if (reviewSimpleChanges === false) { + prompt += this.triageFileDiff; + } + return inputs.render(prompt); } renderSummarizeChangesets(inputs) { return inputs.render(this.summarizeChangesets); @@ -6939,7 +6946,7 @@ ${hunks.oldHunk} } ins.filename = filename; // render prompt based on inputs so far - let tokens = (0,tokenizer/* getTokenCount */.V)(prompts.renderSummarizeFileDiff(ins)); + let tokens = (0,tokenizer/* getTokenCount */.V)(prompts.renderSummarizeFileDiff(ins, options.reviewSimpleChanges)); const diffTokens = (0,tokenizer/* getTokenCount */.V)(fileDiff); if (tokens + diffTokens > options.lightTokenLimits.requestTokens) { (0,core.info)(`summarize: diff tokens exceeds limit, skip ${filename}`); @@ -6962,29 +6969,29 @@ ${hunks.oldHunk} } // summarize content try { - const [summarizeResp] = await lightBot.chat(prompts.renderSummarizeFileDiff(ins), {}); + const [summarizeResp] = await lightBot.chat(prompts.renderSummarizeFileDiff(ins, options.reviewSimpleChanges), {}); if (summarizeResp === '') { (0,core.info)('summarize: nothing obtained from openai'); summariesFailed.push(`${filename} (nothing obtained from openai)`); return null; } else { - // parse the comment to look for triage classification - // Format is : [TRIAGE]: - // if the change needs review return true, else false - const triageRegex = /\[TRIAGE\]:\s*(NEEDS_REVIEW|APPROVED)/; - const triageMatch = summarizeResp.match(triageRegex); - if (triageMatch != null) { - const triage = triageMatch[1]; - const needsReview = triage === 'NEEDS_REVIEW'; - // remove this line from the comment - const summary = summarizeResp.replace(triageRegex, '').trim(); - (0,core.info)(`filename: ${filename}, triage: ${triage}`); - return [filename, summary, needsReview]; - } - else { - return [filename, summarizeResp, true]; + if (options.reviewSimpleChanges === false) { + // parse the comment to look for triage classification + // Format is : [TRIAGE]: + // if the change needs review return true, else false + const triageRegex = /\[TRIAGE\]:\s*(NEEDS_REVIEW|APPROVED)/; + const triageMatch = summarizeResp.match(triageRegex); + if (triageMatch != null) { + const triage = triageMatch[1]; + const needsReview = triage === 'NEEDS_REVIEW'; + // remove this line from the comment + const summary = summarizeResp.replace(triageRegex, '').trim(); + (0,core.info)(`filename: ${filename}, triage: ${triage}`); + return [filename, summary, needsReview]; + } } + return [filename, summarizeResp, true]; } } catch (e) { diff --git a/src/main.ts b/src/main.ts index 39ed8b5f..21572553 100644 --- a/src/main.ts +++ b/src/main.ts @@ -16,6 +16,7 @@ async function run(): Promise { getBooleanInput('debug'), getBooleanInput('summary_only'), getInput('max_files'), + getBooleanInput('review_simple_changes'), getBooleanInput('review_comment_lgtm'), getMultilineInput('path_filters'), getInput('system_message'), diff --git a/src/options.ts b/src/options.ts index 080773da..b5f3f6ff 100644 --- a/src/options.ts +++ b/src/options.ts @@ -6,6 +6,7 @@ export class Options { debug: boolean summaryOnly: boolean maxFiles: number + reviewSimpleChanges: boolean reviewCommentLGTM: boolean pathFilters: PathFilter systemMessage: string @@ -22,6 +23,7 @@ export class Options { debug: boolean, summaryOnly: boolean, maxFiles = '0', + reviewSimpleChanges = false, reviewCommentLGTM = false, pathFilters: string[] | null = null, systemMessage = '', @@ -35,6 +37,7 @@ export class Options { this.debug = debug this.summaryOnly = summaryOnly this.maxFiles = parseInt(maxFiles) + this.reviewSimpleChanges = reviewSimpleChanges this.reviewCommentLGTM = reviewCommentLGTM this.pathFilters = new PathFilter(pathFilters) this.systemMessage = systemMessage @@ -53,6 +56,7 @@ export class Options { info(`debug: ${this.debug}`) info(`summary_only: ${this.summaryOnly}`) info(`max_files: ${this.maxFiles}`) + info(`review_simple_changes: ${this.reviewSimpleChanges}`) info(`review_comment_lgtm: ${this.reviewCommentLGTM}`) info(`path_filters: ${this.pathFilters}`) info(`system_message: ${this.systemMessage}`) diff --git a/src/prompts.ts b/src/prompts.ts index 3e93eb30..26d2c912 100644 --- a/src/prompts.ts +++ b/src/prompts.ts @@ -23,8 +23,8 @@ $file_diff \`\`\` I would like you to summarize the diff within 50 words. - -Below the summary, I would also like you to triage the diff as \`NEEDS_REVIEW\` or +` + triageFileDiff = `Below the summary, I would also like you to triage the diff as \`NEEDS_REVIEW\` or \`APPROVED\` based on the following criteria: - If the diff involves any modifications to the logic or functionality, even if they @@ -248,8 +248,15 @@ $patches this.summarizeReleaseNotes = summarizeReleaseNotes } - renderSummarizeFileDiff(inputs: Inputs): string { - return inputs.render(this.summarizeFileDiff) + renderSummarizeFileDiff( + inputs: Inputs, + reviewSimpleChanges: boolean + ): string { + let prompt = this.summarizeFileDiff + if (reviewSimpleChanges === false) { + prompt += this.triageFileDiff + } + return inputs.render(prompt) } renderSummarizeChangesets(inputs: Inputs): string { diff --git a/src/review.ts b/src/review.ts index 13aa7947..1cd3f0f4 100644 --- a/src/review.ts +++ b/src/review.ts @@ -261,7 +261,9 @@ ${hunks.oldHunk} ins.filename = filename // render prompt based on inputs so far - let tokens = getTokenCount(prompts.renderSummarizeFileDiff(ins)) + let tokens = getTokenCount( + prompts.renderSummarizeFileDiff(ins, options.reviewSimpleChanges) + ) const diffTokens = getTokenCount(fileDiff) if (tokens + diffTokens > options.lightTokenLimits.requestTokens) { @@ -291,7 +293,7 @@ ${hunks.oldHunk} // summarize content try { const [summarizeResp] = await lightBot.chat( - prompts.renderSummarizeFileDiff(ins), + prompts.renderSummarizeFileDiff(ins, options.reviewSimpleChanges), {} ) @@ -300,23 +302,24 @@ ${hunks.oldHunk} summariesFailed.push(`${filename} (nothing obtained from openai)`) return null } else { - // parse the comment to look for triage classification - // Format is : [TRIAGE]: - // if the change needs review return true, else false - const triageRegex = /\[TRIAGE\]:\s*(NEEDS_REVIEW|APPROVED)/ - const triageMatch = summarizeResp.match(triageRegex) - - if (triageMatch != null) { - const triage = triageMatch[1] - const needsReview = triage === 'NEEDS_REVIEW' - - // remove this line from the comment - const summary = summarizeResp.replace(triageRegex, '').trim() - info(`filename: ${filename}, triage: ${triage}`) - return [filename, summary, needsReview] - } else { - return [filename, summarizeResp, true] + if (options.reviewSimpleChanges === false) { + // parse the comment to look for triage classification + // Format is : [TRIAGE]: + // if the change needs review return true, else false + const triageRegex = /\[TRIAGE\]:\s*(NEEDS_REVIEW|APPROVED)/ + const triageMatch = summarizeResp.match(triageRegex) + + if (triageMatch != null) { + const triage = triageMatch[1] + const needsReview = triage === 'NEEDS_REVIEW' + + // remove this line from the comment + const summary = summarizeResp.replace(triageRegex, '').trim() + info(`filename: ${filename}, triage: ${triage}`) + return [filename, summary, needsReview] + } } + return [filename, summarizeResp, true] } } catch (e: any) { warning(`summarize: error from openai: ${e as string}`) From 588c1d0ebab9b279710a41ce3132e052c51b6dd8 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Sun, 23 Apr 2023 10:32:02 -0700 Subject: [PATCH 2/5] readme --- README.md | 46 +++++++++++++++++++++++++++++++++++++++------- dist/index.js | 8 +++++--- src/review.ts | 10 +++++++--- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 751c15d9..efefaa79 100644 --- a/README.md +++ b/README.md @@ -5,13 +5,45 @@ ## Overview This [OpenAI ChatGPT-based](https://platform.openai.com/docs/guides/chat) GitHub -Action provides a summary, release notes and review of pull requests. The -prompts have been tuned for a concise response. To prevent excessive -notifications, this action can be configured to skip adding review comments when -the changes look good for the most part. - -In addition, this action can also reply to the user comments made on the review -by this action. +Action provides a summary, release notes and review of pull requests. The unique +features of this action are: + +- Unlike other approaches that provide a simple summary and/or conversation, + this action reviews the changes line by line and provides code change + suggestions that can be directly committed from the GitHub UI. The prompts + have been tuned carefully to comment on exact lines within changed hunks of + code. +- Continuous, yet incremental, reviews on each commit with a pull request. This + is unlike other approaches that provide a one-time review on the entire pull + request when requested by the user. +- Incremental reviews save on OpenAI costs while also reducing noise. Changed + files are tracked between commits and the base of the pull request +- The action is designed to be used with a "light" summarization model (e.g. + `gpt-3.5-turbo`) and a "heavy" review model (e.g. `gpt-4`). This allows for a + cheaper and faster summarization process and a more accurate review process. +- This action supports a conversation with the bot in the context of lines of + code or entire files. Useful for providing further context to the bot for its + next review, to generate test cases, to reduce complexity of the code and so + on. +- By default, the action is configured to skip more in-depth review when the + changes are simple (e.g. typo fixes). This is based on the triage done during + the summarization stage. This feature can be disabled by setting + `review_simple_changes` to `true`. +- By default, the action is configured to skip adding review comments when the + changes look good for the most part. This feature can be disabled by setting + `review_comment_lgtm` to `true`. +- You can tailor the following prompts: + - `system_message`: Defines the objective and the personality of the bot. You + can change this prompt to focus on or ignore certain aspects of the review + process, e.g. documentation, code quality, etc. Furthermore, you can even + change the bot to do marketing material review instead of code review. + - `summarize`: Summarizes the pull request into a table of changes etc. + - `summarize_release_notes`: Summarize the changes in the pull request for + release notes purposes. +- You can altogether skip the reviews, setting the `summary_only` to `true`. But + that defeats the main purpose of this action. Other tools such as GitHub's + [Copliot for Pull Requests](https://githubnext.com/projects/copilot-for-pull-requests) + may be a cheaper and good enough alternative in that case. NOTES: diff --git a/dist/index.js b/dist/index.js index 5f6b853a..0b92229f 100644 --- a/dist/index.js +++ b/dist/index.js @@ -7247,11 +7247,13 @@ ${commentChain} } await Promise.all(reviewPromises); summarizeComment += ` +In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${context.payload.pull_request.head.ref}\` commits were reviewed. + ${reviewsFailed.length > 0 ? `
-Files not reviewed due to errors in this run (${reviewsFailed.length}) +Files not reviewed due to errors in the recent run (${reviewsFailed.length}) -### Failed to review +### Failed to review in the last run * ${reviewsFailed.join('\n* ')} @@ -7263,7 +7265,7 @@ ${reviewsSkipped.length > 0 ? `
Files not reviewed due to simple changes (${reviewsSkipped.length}) -### Skipped review +### Skipped review in the recent run * ${reviewsSkipped.join('\n* ')} diff --git a/src/review.ts b/src/review.ts index 1cd3f0f4..b1c09c50 100644 --- a/src/review.ts +++ b/src/review.ts @@ -656,14 +656,18 @@ ${commentChain} await Promise.all(reviewPromises) summarizeComment += ` +In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${ + context.payload.pull_request.head.ref + }\` commits were reviewed. + ${ reviewsFailed.length > 0 ? `
-Files not reviewed due to errors in this run (${ +Files not reviewed due to errors in the recent run (${ reviewsFailed.length }) -### Failed to review +### Failed to review in the last run * ${reviewsFailed.join('\n* ')} @@ -679,7 +683,7 @@ ${ reviewsSkipped.length }) -### Skipped review +### Skipped review in the recent run * ${reviewsSkipped.join('\n* ')} From 8e1419b320aac33efe8964f7a5fa1196044c1344 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Sun, 23 Apr 2023 10:35:46 -0700 Subject: [PATCH 3/5] readme --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index efefaa79..0aeae18f 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,8 @@ features of this action are: - The action is designed to be used with a "light" summarization model (e.g. `gpt-3.5-turbo`) and a "heavy" review model (e.g. `gpt-4`). This allows for a cheaper and faster summarization process and a more accurate review process. + **For best results, setting `gpt-4` as the "heavy" model instead of the + `gpt-3.5-turbo` (default) is highly recommended.** - This action supports a conversation with the bot in the context of lines of code or entire files. Useful for providing further context to the bot for its next review, to generate test cases, to reduce complexity of the code and so From 82258c1e473a27af54ab1d4d23c87ca5644bbd86 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Sun, 23 Apr 2023 10:37:40 -0700 Subject: [PATCH 4/5] nit --- dist/index.js | 1 + src/review.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/dist/index.js b/dist/index.js index 0b92229f..bdf354a5 100644 --- a/dist/index.js +++ b/dist/index.js @@ -7247,6 +7247,7 @@ ${commentChain} } await Promise.all(reviewPromises); summarizeComment += ` +--- In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${context.payload.pull_request.head.ref}\` commits were reviewed. ${reviewsFailed.length > 0 diff --git a/src/review.ts b/src/review.ts index b1c09c50..78839b10 100644 --- a/src/review.ts +++ b/src/review.ts @@ -656,6 +656,7 @@ ${commentChain} await Promise.all(reviewPromises) summarizeComment += ` +--- In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${ context.payload.pull_request.head.ref }\` commits were reviewed. From 0658f71b33e60d242546cde47902b6e48d085238 Mon Sep 17 00:00:00 2001 From: Harjot Gill Date: Sun, 23 Apr 2023 10:38:41 -0700 Subject: [PATCH 5/5] nit --- 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 bdf354a5..d4cbf259 100644 --- a/dist/index.js +++ b/dist/index.js @@ -7248,7 +7248,7 @@ ${commentChain} await Promise.all(reviewPromises); summarizeComment += ` --- -In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${context.payload.pull_request.head.ref}\` commits were reviewed. +In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${context.payload.pull_request.head.sha}\` commits were reviewed. ${reviewsFailed.length > 0 ? `
diff --git a/src/review.ts b/src/review.ts index 78839b10..ed5c0b81 100644 --- a/src/review.ts +++ b/src/review.ts @@ -658,7 +658,7 @@ ${commentChain} summarizeComment += ` --- In the recent run, only the files that changed from the \`base\` of the PR and between \`${highestReviewedCommitId}\` and \`${ - context.payload.pull_request.head.ref + context.payload.pull_request.head.sha }\` commits were reviewed. ${