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

[TT-1435] use chatgpt to find new issues in modified Solidity files #14104

Merged
merged 9 commits into from
Aug 20, 2024

Conversation

Tofel
Copy link
Contributor

@Tofel Tofel commented Aug 13, 2024

Following changes have been implemented:

  • find existing contracts, which were modified; if there are none proceed as before
  • run Slither for current branch HEAD
  • checkout base reference (HEAD of target branch)
  • run Slither for modified contracts
  • ask ChatGPT to compare both reports and find new issues
  • ask ChatGPT to evaluate whether the result of that comparison shows all new issues
  • if there are no differences, do not display any Slither findings
  • if interaction with ChatGPT failed, display full report
  • if interaction with ChatGPT was successful display only new issues together with confidence rating according to ChatGPT

It's not bullet-proof, but I'd still give it a go and see if for now it's good enough.
Run, when no changes were found: https://github.com/smartcontractkit/chainlink/actions/runs/10385260800?pr=14111
Run, when changes were correctly found: https://github.com/smartcontractkit/chainlink/actions/runs/10385422699

@Tofel Tofel changed the base branch from develop to tt_1435_update_Sol_Foundry_with_diff August 13, 2024 09:24
@Tofel Tofel changed the base branch from tt_1435_update_Sol_Foundry_with_diff to develop August 13, 2024 09:25
# throw error openai_result when is not 200
if [ "$openai_result" != '200' ]; then
echo "::error::OpenAI API call failed with status $openai_result: $(cat prompt_response.json)"
return 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this just be exit 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't that cause the pipeline to fail? If so, that's not what I want, I want to display a warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this would return 1 here no from this?

if ./contracts/scripts/ci/find_slither_report_diff.sh "$base_report" "$current_report" "$new_issues_report" "contracts/scripts/ci/prompt-difference.md" "contracts/scripts/ci/prompt-validation.md"; then

then should it be if [[ $x == 1 ]]

maybe the above should be assigned to a var and then use that in the if to make it clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, changed return to exit. but I'd keep as-is, as from what I found it's the recommended way of testing command exit code, not to mention the fact that in strict mode capturing that output in a variable and then doing if [[ $? -eq 0 ]]; wouldn't work as expected :/ non-zero exit code would terminate the execution.

momentmaker
momentmaker previously approved these changes Aug 14, 2024
@Tofel Tofel marked this pull request as ready for review August 14, 2024 13:28
@Tofel Tofel requested review from a team and RensR as code owners August 14, 2024 13:28
@kalanyuz kalanyuz self-requested a review August 19, 2024 16:00
first_report_content=$(cat "$first_report_path" | sed 's/"//g' | sed -E 's/\\+$//g' | sed -E 's/\\+ //g')
second_report_content=$(cat "$second_report_path" | sed 's/"//g' | sed -E 's/\\+$//g' | sed -E 's/\\+ //g')
openai_prompt=$(cat "$report_prompt_path" | sed 's/"/\\"/g' | sed -E 's/\\+$//g' | sed -E 's/\\+ //g')
openai_model="gpt-4o"

Choose a reason for hiding this comment

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

I'd suggest that we pin the mode version to the current one that you are testing with (or make it a variable.)
Otherwise you are at risk of behavioral changes as newer versions are deployed to the 4o endpoints, we have seen breaking changes in format / responses before.

exit 1
fi

if [[ -z "${OPEN_API_KEY+x}" ]]; then

Choose a reason for hiding this comment

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

If preferred, you can safely remove this as $openai_result below will fail with ::error:: log telling you that you haven't supplied the key and we won't be charged for the call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to have it, because it's running in strict mode and will error if env var is not set, when read. I prefer to return a more explicit error than what bash would.


if [[ -n "$validation_prompt_path" ]]; then
echo "::debug::Validating the diff report using the validation prompt"
openai_model="gpt-4-turbo"

Choose a reason for hiding this comment

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

Same comment for version pinning. To check the latest model version number, see: https://platform.openai.com/docs/models

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea!

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@Tofel Tofel added this pull request to the merge queue Aug 20, 2024
Merged via the queue into develop with commit 6d072dd Aug 20, 2024
147 of 148 checks passed
@Tofel Tofel deleted the tt_1435_slither_diff_chatgpt branch August 20, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants