diff --git a/README.md b/README.md index 4c4aaa21..0aeae18f 100644 --- a/README.md +++ b/README.md @@ -5,13 +5,47 @@ ## 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. + **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 + 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: @@ -58,6 +92,7 @@ jobs: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} with: debug: false + review_simple_changes: false review_comment_lgtm: false ``` @@ -101,7 +136,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 +254,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..d4cbf259 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) { @@ -7240,11 +7247,14 @@ ${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.sha}\` 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* ')} @@ -7256,7 +7266,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/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..ed5c0b81 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}`) @@ -653,14 +656,19 @@ ${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.sha + }\` 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* ')} @@ -676,7 +684,7 @@ ${ reviewsSkipped.length }) -### Skipped review +### Skipped review in the recent run * ${reviewsSkipped.join('\n* ')}