Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test PR #1

Closed
wants to merge 87 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
87 commits
Select commit Hold shift + click to select a range
5e39d6a
chore(application): fixed
DarrellRichards May 31, 2024
ea756f4
chore(action): updated node v
DarrellRichards May 31, 2024
d1b19df
chore(action): node latest
DarrellRichards May 31, 2024
359de21
chore(node): updated node
DarrellRichards May 31, 2024
38d26f0
chore(updated): moved libs to services
DarrellRichards May 31, 2024
1fcadcd
chore(action): fixed issue with var
DarrellRichards May 31, 2024
44903e2
chore(action): fixed issue with var
DarrellRichards May 31, 2024
d040757
chore(action): fixed issue with var
DarrellRichards May 31, 2024
19ece9f
chore(action): fixed issue with var
DarrellRichards May 31, 2024
00fbc88
chore(action): fixed issue with var
DarrellRichards May 31, 2024
cd1f545
chore(action): fixed issue with var
DarrellRichards May 31, 2024
cd7612e
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
2eea851
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
d1548d2
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
c255374
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
2670f16
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
3ba739b
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
9e7b01e
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
ad28ce7
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
90edab3
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
9779458
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
3392d42
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
c44418b
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
f9d9fb8
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
9532026
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
13af433
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
5a1f0b1
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
9235dd5
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
84608b5
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
926f206
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
08084a8
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
a17cbdf
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
1d3de99
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
f31493d
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
27ffb14
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
328e9f7
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
9c15f54
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
c892320
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
6867bab
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
288b79a
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
3d3f2da
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
443ee90
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
f699e0f
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
ecfa3df
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
eda91ff
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
93b24a1
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
716ada6
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
7e0ebe9
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
c244bfc
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
e91ae52
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
6691a7c
chore(action): fixed issue with var
DarrellRichards Jun 1, 2024
72eb518
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
3f4b20e
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
2f08e55
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
434df9b
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
76d58df
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
8bf7caf
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
df108ec
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
42b7769
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
78060e8
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
e1f0363
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
7fe820f
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
6660840
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
312f03f
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
61746e2
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
a52477e
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
764c820
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
6e79ab1
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
f54bd21
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
b73aea8
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
4795359
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
89d1351
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
1c77949
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
1eb203c
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
a4d7d5b
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
d051108
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
34a9de8
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
56a1938
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
042c527
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
dd04a67
chore(action): fixed issue with var
DarrellRichards Jun 2, 2024
99a9db5
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
0e6e6b7
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
4c62433
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
f1b4568
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
f1cb5f0
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
982a3b6
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
b25ee58
chore(action): fixed issue with var
DarrellRichards Jun 3, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions .github/workflows/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,19 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v2
- name: Set up Node
uses: actions/setup-node@v2
- uses: actions/setup-node@v4
with:
node-version: 18
- name: Install dependencies
run: npm install
- name: Build Code
run: npm run build
- name: Run Custom Action
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good to see the action updated to newer versions; however, there is a minor typo in the property name expluded_files. It should be excluded_files.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you've updated the actions/setup-node action to version 4, but the previous line referring to it has been left commented out. It's good practice to remove commented-out code to keep the file clean.

uses: ./
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
github_token: ${{ secrets.GITHUB_TOKEN }}
pr-number: ${{ github.event.number }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The github-token field has been modified to github_token. Make sure this is consistent with the naming conventions used in other workflows or scripts to avoid potential issues.

openai_api_key: ${{ secrets.OPENAI_API_KEY }}
openai_api_key: ${{ secrets.OPEN_AI_KEY }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secret key has been changed from OPENAI_API_KEY to OPEN_AI_KEY. Ensure that the correct key is updated in the repository secrets.

openai_model: 'gpt-4'
review_code: true
expluded_files: 'node_modules, package.json, package-lock.json'
17 changes: 13 additions & 4 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ name: 'Github PR Magic'
description: 'Automatically reviewing and approving PRs'
author: 'Darrell Richards'
inputs:
github-token:
github_token:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input parameter change from github-token to github_token seems to be inconsistent with common GitHub Actions naming conventions which typically use dashes (-) instead of underscores (_). Please ensure this aligns with the expected naming conventions for the inputs of GitHub Actions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the input parameter from github-token to github_token is a breaking change for existing workflows. Ensure to communicate this change and update documentation accordingly. Consider keeping github-token as an alias for backward compatibility.

description: 'Github token'
required: true
pr-number:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to the previous comment, changing pr-number to pr_number may not align with the usual GitHub Actions input naming conventions. It's commonly recommended to use dashes to separate words in input names.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the change on line 5, altering pr-number to pr_number can break existing workflows. It's important to maintain consistency and backward compatibility. Adding an alias for pr-number or providing migration steps for users can help prevent disruptions.

description: 'PR number'
required: true
excluded_files:
description: 'Files to exclude from review'
required: false
default: 'node_modules, package-lock.json, yarn.lock'
openai_api_key:
description: 'OpenAI API key'
required: true
Expand All @@ -19,6 +20,14 @@ inputs:
description: 'Review code'
required: false
default: true
generate_summary:
description: 'Generates Pull Request summary based on git diff and code changes'
required: false
default: false
overall_code_review:
description: 'Overall code review as a comment'
required: false
default: false
runs:
using: 'node20'
main: 'lib/index.js'
9 changes: 8 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
"dependencies": {
"@actions/core": "^1.10.1",
"@octokit/rest": "^20.1.1",
"@octokit/types": "^13.5.0",
"@tandil/diffparse": "^0.2.0",
"minimatch": "^9.0.4",
"openai": "^4.47.3"
"openai": "^4.47.3",
"parse-diff": "^0.11.1"
}
}
206 changes: 190 additions & 16 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,40 +1,214 @@
const parser = require('@tandil/diffparse');
import { readFileSync } from "fs"
import OpenAI from "openai"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readFileSync method is used without specifying the character encoding. It's recommended to specify the encoding ('utf-8') to ensure correct behavior.

import core from "@actions/core";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented out code if it's not intended to be used. It can cause confusion and make the codebase harder to maintain.

import { PRDetails } from "./lib/github";
import { minimatch } from "minimatch";
import parseDiff, { File } from "parse-diff"
import * as core from "@actions/core"

import { commentOnPullRequest, compareCommits, createReviewComment, gitDiff, PRDetails, updateBody } from "./services/github";
import { filter, minimatch } from "minimatch";
import { obtainFeedback, prSummaryCreation, summaryAllMessages, summaryOfAllFeedback, validateCodeViaAI } from "./services/ai";


const excludedFiles = core.getInput("excluded_files").split(",").map((s: string) => s.trim());
const createPullRequestSummary = core.getInput("generate_summary");
const reviewCode = core.getInput("review_code")
const overallReview = core.getInput("overall_code_review");


export interface Details {
title: string;
description: string;
}

async function validatePullRequest(diff: File[], details: Details) {
const foundSummary = [];
for (const file of diff) {
for (const chunk of file.chunks) {
const message = await prSummaryCreation(file, chunk, details);
if (message) {
const mappedResults = message.flatMap((result: any) => {
if (!result.changes) {
return [];
}

if (!result.typeChanges) {
return [];
}

if (!result.checklist) {
return [];
}


return {
changes: result.changes,
typeChanges: result.typeChanges,
checklist: result.checklist,
};
});

foundSummary.push(...mappedResults);
}
}
}


if (foundSummary && foundSummary.length > 0) {
const bodyIdea = await summaryAllMessages(foundSummary);
return bodyIdea;
}

return '';
}

async function validateCode(diff: File[], details: Details) {
const neededComments = [];
for (const file of diff) {
for (const chunk of file.chunks) {
const results = await validateCodeViaAI(file, chunk, details);

if (results) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition !file.to seems unnecessary because file.to is already checked in the minimatch pattern below, which filters out null values. Consider removing this redundant check.

const mappedResults = results.flatMap((result: any) => {
if (!file.to) {
return [];
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like result.lineNumber is expected to be a number. Should the condition check for a falsy value (such as 0) that might be a valid line number? Consider explicitly checking for undefined or null.


if (!result.lineNumber) {
return [];
}

if (!result.review) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing result.required_changed affects the check for whether a change is required or not. If no checks are implemented elsewhere, this could lead to the problem that code reviews are not properly enforced. Ensure that this behavior is captured in a different part of the workflow or consider adding the required change logic back.

return [];
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body, path, and position fields in the returned object should be double-checked to ensure they match the expected structure of a GitHub review comment.

return {
body: result.review,
path: file.to,
position: Number(result.lineNumber)
};
});

if (mappedResults) {
neededComments.push(...mappedResults);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the function validateOverallCodeReview has been introduced, but its body is currently incomplete. The obtainFeedback function is called, however, the results are not being used except for being logged to the console. Make sure to implement the functionality that processes these results, including mapping them to detailedFeedback and handling them accordingly.

}
}

return neededComments;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable dif is initialized but never used in the main function. It would be cleaner to remove this unused variable.

async function validateOverallCodeReview(diff: File[], details: Details) {
const detailedFeedback = [];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results are checked for existence but not for their content structure before mapping. Ensure that the results follow the expected structure for changesOverview, feedback, improvements, and conclusion before mapping. This will prevent runtime errors in case the structure is not what the code expects.

for (const file of diff) {
for (const chunk of file.chunks) {
const results = await obtainFeedback(file, chunk, details);
if (results) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the log for generating a summary of new PRs has been removed. This information could be useful for debugging. Consider adding it back or replacing it with a more informative message, especially if other logging mechanisms haven't been provided elsewhere.

const mappedResults = results.flatMap((result: any) => {
if (!file.to) {
return [];
}

return {
changesOverview: result.changesOverview,
feedback: result.feedback,
improvements: result.improvements,
conclusion: result.conclusion,
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable mappedResults is assumed not to be empty before pushing its contents into detailedFeedback. Consider validating it contains items before this operation. Empty or incorrect data could potentially cause issues downstream.

});

if (mappedResults) {
detailedFeedback.push(...mappedResults);
}
}
}
}

return detailedFeedback;
}

const openai = new OpenAI()
const excludedFiles = core.getInput("exclude").split(",").map((s: string) => s.trim());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that process.env.GITHUB_EVENT_PATH may not always be set, which could lead to a runtime error when calling JSON.parse(readFileSync()). Handling this potential absence with a clear error message would be beneficial.


async function main() {
let dif: string | null = null;
const { action, repository, number } = JSON.parse(readFileSync(process.env.GITHUB_EVENT_PATH || "", "utf-8"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comments and commented out code should be removed before merging to keep the codebase clean.

const { title, description } = await PRDetails(repository, number);
const { action, repository, number, before, after } = JSON.parse(readFileSync(process.env.GITHUB_EVENT_PATH || "", "utf-8"))
const { title, description, patch_url, diff_url } = await PRDetails(repository, number);

if (action === "opened") {
// Generate a summary of the PR since it's a new PR

const data = await gitDiff(repository.owner.login, repository.name, number);
dif = data as unknown as string;
} else if (action === "synchronize") {
const newBaseSha = before;
const newHeadSha = after;

const data = await compareCommits({
owner: repository.owner.login,
repo: repository.name,
before: newBaseSha,
after: newHeadSha,
number
})

dif = String(data);
} else {
console.log('Unknown action', process.env.GITHUB_EVENT_NAME);
return;
}

if (!dif) {
// Well shit.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nullish coalescing operator (??) is used here. Ensure that the intention is to only bypass file.to if it is null or undefined, and not other falsy values like empty strings.

return;
}

const diff = parser.parseDiffString(dif);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This @ToDo comment should be tracked as an issue in the project's issue tracker instead of being left in the code.

const filteredDiff = diff.filter((file: { to: any; }) => {
const diff = parseDiff(dif);
const filteredDiff = diff.filter((file) => {
return !excludedFiles.some((pattern) =>
minimatch(file.to ?? "", pattern)
);
});

console.log(filteredDiff);
if (action === "opened") {
if (createPullRequestSummary) {
console.log('Generating summary for new PR');
const summary = await validatePullRequest(diff, {
title,
description
});

await updateBody(repository.owner.login, repository.name, number, summary)
}

// Validate Some Code Yo!
if (overallReview) {
const detailedFeedback = await validateOverallCodeReview(filteredDiff, {
title,
description
});

if (detailedFeedback && detailedFeedback.length > 0) {
const resultsFullFeedback = await summaryOfAllFeedback(detailedFeedback);

await commentOnPullRequest({
owner: repository.owner.login,
repo: repository.name,
number
}, resultsFullFeedback);
}
}
}

// Post some comments
if (reviewCode) {
// @TODO Improve the support for comments,
// IE: Remove outdated comments when code is changed, revalidated if the Pull Request is ready to be approved.
const neededComments = await validateCode(filteredDiff, {
title,
description
});
if (neededComments && neededComments.length > 0) {
await createReviewComment(repository.owner.login, repository.name, number, neededComments);
} else {
// @TODO We need to veirfy if any other comments was created by the AI Bot, if so see if they was updated.
// @TODO If they have been fixed or no reviews are required than we can approve the Pull Request
await createReviewComment(repository.owner.login, repository.name, number, neededComments)
}
}
}

main();
main();
Loading
Loading