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

[DO NOT MERGE] Add coderabbit.ai github actions workflow #442

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 30 additions & 0 deletions .github/workflows/ai-pr-reviewer.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
name: Code Review

permissions:
contents: read
pull-requests: write

on:
pull_request:
pull_request_review_comment:
types: [created]

concurrency:
group:
${{ github.repository }}-${{ github.event.number || github.head_ref ||
github.sha }}-${{ github.workflow }}-${{ github.event_name ==
'pull_request_review_comment' && 'pr_comment' || 'pr' }}
cancel-in-progress: ${{ github.event_name != 'pull_request_review_comment' }}

jobs:
review:
runs-on: ubuntu-latest
steps:
- uses: coderabbitai/ai-pr-reviewer@latest
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
with:
debug: false
review_simple_changes: true
review_comment_lgtm: true
Comment on lines +1 to +30
Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

The workflow file looks good overall. However, there's a potential security concern with the use of secrets in the environment variables. It's generally not recommended to expose sensitive information like GITHUB_TOKEN and OPENAI_API_KEY directly in the workflow file. Instead, you should use GitHub's built-in secret management feature to securely handle these values.

      - uses: coderabbitai/ai-pr-reviewer@latest
        env:
-25:           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-26:           OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
+25:           GITHUB_TOKEN: ${{ secrets['GITHUB_TOKEN'] }}
+26:           OPENAI_API_KEY: ${{ secrets['OPENAI_API_KEY'] }}
        with:
          debug: false
          review_simple_changes: true
          review_comment_lgtm: true

This change ensures that even if an attacker gains access to your repository, they won't be able to read the contents of these secrets directly from the workflow file.

5 changes: 5 additions & 0 deletions protocol/x/clob/keeper/orders.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ func (k Keeper) PlaceStatefulOrder(
return err
}

/*
Nonsense changes to see if AI code reviewer will catch it.
*/
fmt.Println("This should not be merged to main")

Comment on lines +209 to +213
Copy link

@github-actions github-actions bot Oct 3, 2023

Choose a reason for hiding this comment

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

Image description CodeRabbit

The added code block seems to be a debug statement which should not be included in the production code. It's important to remove such statements before merging to maintain the cleanliness and efficiency of the codebase.

- 	/*
- 		Nonsense changes to see if AI code reviewer will catch it.
- 	*/
- 	fmt.Println("This should not be merged to main")

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree with mr rabbit, this code is nonsense

Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

@jiajames, I'm glad you agree. Debugging or test statements like these can clutter the codebase and potentially cause confusion or unexpected behavior in production. It's always a good practice to clean up such statements before merging changes. Let's ensure we maintain the quality and clarity of our code.

// 4. Perform a collateralization check for the full size of the order to mitigate spam.
// TODO(CLOB-725): Consider using a pessimistic collateralization check.
_, successPerSubaccountUpdate := k.AddOrderToOrderbookCollatCheck(
Expand Down
Loading