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
Merged
Show file tree
Hide file tree
Changes from 2 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
170 changes: 161 additions & 9 deletions .github/workflows/solidity-foundry.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ env:
# * use the top-level matrix to decide, which checks should run for each product.
# * when enabling code coverage, remember to adjust the minimum code coverage as it's set to 98.5% by default.

# This pipeline will run product tests only if product-specific contracts were modified or if broad-impact changes were made (e.g. changes to this pipeline, Foundry configuration, etc.)
# For modified contracts we use a LLM to extract new issues introduced by the changes. For new contracts full report is delivered.
# Slither has a default configuration, but also supports per-product configuration. If a product-specific configuration is not found, the default one is used.
# Changes to test files do not trigger static analysis or formatting checks.

jobs:
define-matrix:
name: Define test matrix
Expand Down Expand Up @@ -53,8 +58,10 @@ jobs:
runs-on: ubuntu-latest
outputs:
non_src_changes: ${{ steps.changes.outputs.non_src }}
sol_modified: ${{ steps.changes.outputs.sol }}
sol_modified_files: ${{ steps.changes.outputs.sol_files }}
sol_modified_added: ${{ steps.changes.outputs.sol }}
sol_modified_added_files: ${{ steps.changes.outputs.sol_files }}
sol_mod_only: ${{ steps.changes.outputs.sol_mod_only }}
sol_mod_only_files: ${{ steps.changes.outputs.sol_mod_only_files }}
not_test_sol_modified: ${{ steps.changes.outputs.not_test_sol }}
not_test_sol_modified_files: ${{ steps.changes.outputs.not_test_sol_files }}
all_changes: ${{ steps.changes.outputs.changes }}
Expand All @@ -73,6 +80,8 @@ jobs:
- 'contracts/package.json'
sol:
- modified|added: 'contracts/src/v0.8/**/*.sol'
sol_mod_only:
- modified: 'contracts/src/v0.8/**/!(*.t).sol'
not_test_sol:
- modified|added: 'contracts/src/v0.8/**/!(*.t).sol'
automation:
Expand Down Expand Up @@ -199,7 +208,6 @@ jobs:
|| needs.changes.outputs.non_src_changes == 'true')
&& matrix.product.setup.run-coverage }}
run: |
sudo apt-get install lcov
./contracts/scripts/lcov_prune ${{ matrix.product.name }} ./contracts/lcov.info ./contracts/lcov.info.pruned

- name: Report code coverage for ${{ matrix.product.name }}
Expand Down Expand Up @@ -229,6 +237,7 @@ jobs:
this-job-name: Foundry Tests ${{ matrix.product.name }}
continue-on-error: true

# runs only if non-test contracts were modified; scoped only to modified or added contracts
analyze:
needs: [ changes, define-matrix ]
name: Run static analysis
Expand Down Expand Up @@ -268,8 +277,98 @@ jobs:
# modify remappings so that solc can find dependencies
./contracts/scripts/ci/modify_remappings.sh contracts contracts/remappings.txt
mv remappings_modified.txt remappings.txt

# without it Slither sometimes fails to use remappings correctly
cp contracts/foundry.toml foundry.toml

FILES="${{ needs.changes.outputs.not_test_sol_modified_files }}"

for FILE in $FILES; do
PRODUCT=$(echo "$FILE" | awk -F'src/[^/]*/' '{print $2}' | cut -d'/' -f1)
echo "::debug::Running Slither for $FILE in $PRODUCT"
SLITHER_CONFIG="contracts/configs/slither/.slither.config-$PRODUCT-pr.json"
if [ ! -f $SLITHER_CONFIG ]; then
Tofel marked this conversation as resolved.
Show resolved Hide resolved
echo "::debug::No Slither config found for $PRODUCT, using default"
SLITHER_CONFIG="contracts/configs/slither/.slither.config-default-pr.json"
fi
./contracts/scripts/ci/generate_slither_report.sh "${{ github.server_url }}/${{ github.repository }}/blob/${{ github.sha }}/" "$SLITHER_CONFIG" "." "$FILE" "contracts/slither-reports-current" "--solc-remaps @=contracts/node_modules/@"
done

# all the actions below, up to printing results, run only if any existing contracts were modified
# in that case we extract new issues introduced by the changes by using an LLM model
- name: Upload Slither results for current branch
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
timeout-minutes: 2
continue-on-error: true
with:
name: slither-reports-current-${{ github.sha }}
path: contracts/slither-reports-current
retention-days: 7

# we need to upload scripts and configuration in case base_ref doesn't have the scripts, or they are in different version
- name: Upload Slither scripts
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
timeout-minutes: 2
continue-on-error: true
with:
name: tmp-slither-scripts-${{ github.sha }}
path: contracts/scripts/ci
retention-days: 7

- name: Upload configs
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
timeout-minutes: 2
continue-on-error: true
with:
name: tmp-configs-${{ github.sha }}
path: contracts/configs
retention-days: 7

- name: Checkout the repo
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/checkout@9bb56186c3b09b4f86b1c65136769dd318469633 # v4.1.2
with:
ref: ${{ github.base_ref }}

- name: Download Slither scripts
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7
with:
name: tmp-slither-scripts-${{ github.sha }}
path: contracts/scripts/ci

- name: Download configs
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7
with:
name: tmp-configs-${{ github.sha }}
path: contracts/configs

FILES="${{ needs.changes.outputs.not_test_sol_modified_files }}"
# since we have just checked out the repository again, we lose NPM dependencies installs previously, we need to install them again to compile contracts
- name: Setup NodeJS
if: needs.changes.outputs.sol_mod_only == 'true'
uses: ./.github/actions/setup-nodejs

- name: Run Slither for base reference
if: needs.changes.outputs.sol_mod_only == 'true'
shell: bash
run: |
# we need to set file permission again since they are lost during download
for file in contracts/scripts/ci/*.sh; do
chmod +x "$file"
done

# modify remappings so that solc can find dependencies
./contracts/scripts/ci/modify_remappings.sh contracts contracts/remappings.txt
mv remappings_modified.txt remappings.txt

# without it Slither sometimes fails to use remappings correctly
cp contracts/foundry.toml foundry.toml

FILES="${{ needs.changes.outputs.sol_mod_only_files }}"

for FILE in $FILES; do
PRODUCT=$(echo "$FILE" | awk -F'src/[^/]*/' '{print $2}' | cut -d'/' -f1)
Expand All @@ -279,14 +378,62 @@ jobs:
echo "::debug::No Slither config found for $PRODUCT, using default"
SLITHER_CONFIG="contracts/configs/slither/.slither.config-default-pr.json"
fi
./contracts/scripts/ci/generate_slither_report.sh "${{ github.server_url }}/${{ github.repository }}/blob/${{ github.sha }}/" "$SLITHER_CONFIG" "." "$FILE" "contracts/slither-reports" "--solc-remaps @=contracts/node_modules/@"
./contracts/scripts/ci/generate_slither_report.sh "${{ github.server_url }}/${{ github.repository }}/blob/${{ github.sha }}/" "$SLITHER_CONFIG" "." "$FILE" "contracts/slither-reports-base-ref" "--solc-remaps @=contracts/node_modules/@"
done

- name: Upload Slither report
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
timeout-minutes: 10
continue-on-error: true
with:
name: slither-reports-base-${{ github.sha }}
path: |
contracts/slither-reports-base-ref
retention-days: 7

- name: Download Slither results for current branch
if: needs.changes.outputs.sol_mod_only == 'true'
uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7
with:
name: slither-reports-current-${{ github.sha }}
path: contracts/slither-reports-current

- name: Generate diff of Slither reports for modified files
if: needs.changes.outputs.sol_mod_only == 'true'
env:
OPEN_API_KEY: ${{ secrets.OPEN_AI_SLITHER_API_KEY }}
run: |
for base_report in contracts/slither-reports-base-ref/*.md; do
filename=$(basename "$base_report")
current_report="contracts/slither-reports-current/$filename"
new_issues_report="contracts/slither-reports-current/${filename%.md}_new_issues.md"
if [ -f "$current_report" ]; then
Tofel marked this conversation as resolved.
Show resolved Hide resolved
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
if [ -s $diff_report ]; then
Tofel marked this conversation as resolved.
Show resolved Hide resolved
awk 'NR==2{print "*This new issues report has been automatically generated by LLM model using two Slither reports. One based on `${{ github.base_ref}}` and another on `${{ github.sha }}` commits.*"}1' $new_issues_report > tmp.md && mv tmp.md $new_issues_report
echo "Replacing full Slither report with diff for $current_report"
rm $current_report && mv $new_issues_report $current_report
else
echo "No difference detected between $base_report and $current_report reports. Won't include any of them."
rm $current_report
fi
else
echo "::warning::Failed to generate a diff report with new issues for $base_report using an LLM model, will use full report."
fi

else
echo "::error::Failed to find current commit's equivalent of $base_report (file $current_file doesn't exist, but should have been generated). Please check Slither logs."
exit 1
fi
done

# actions that execute only if any existing contracts were modified end here
- name: Print Slither summary
shell: bash
run: |
echo "# Static analysis results " >> $GITHUB_STEP_SUMMARY
for file in "contracts/slither-reports"/*.md; do
for file in "contracts/slither-reports-current"/*.md; do
if [ -e "$file" ]; then
cat "$file" >> $GITHUB_STEP_SUMMARY
fi
Expand All @@ -296,17 +443,17 @@ jobs:
uses: ./.github/actions/validate-solidity-artifacts
with:
validate_slither_reports: 'true'
slither_reports_path: 'contracts/slither-reports'
slither_reports_path: 'contracts/slither-reports-current'
sol_files: ${{ needs.changes.outputs.not_test_sol_modified_files }}

- name: Upload Slither report
- name: Upload Slither reports
uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
timeout-minutes: 10
continue-on-error: true
with:
name: slither-reports-${{ github.sha }}
path: |
contracts/slither-reports
contracts/slither-reports-current
retention-days: 7

- name: Collect Metrics
Expand All @@ -320,6 +467,11 @@ jobs:
this-job-name: Run static analysis
continue-on-error: true

- name: Remove temp artifacts
uses: geekyeggo/delete-artifact@24928e75e6e6590170563b8ddae9fac674508aa1 # v5.0
with:
name: tmp-*

solidity-forge-fmt:
name: Forge fmt ${{ matrix.product.name }}
if: ${{ needs.changes.outputs.non_src_changes == 'true' || needs.changes.outputs.not_test_sol_modified == 'true' }}
Expand Down
94 changes: 94 additions & 0 deletions contracts/scripts/ci/find_slither_report_diff.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
#!/usr/bin/env bash

set -euo pipefail

if [[ "$#" -lt 4 ]]; then
>&2 echo "Generates a markdown file with diff in new issues detected by ChatGPT between two Slither reports."
>&2 echo "Usage: $0 <path-to-first-report> <path-to-second-report> <path-to-diff-report-output> <path-to-prompt> [path-to-validation-prompt]"
exit 1
fi

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

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.

>&2 echo "OPEN_API_KEY is not set."
exit 1
fi

first_report_path=$1
second_report_path=$2
new_issues_report_path=$3
report_prompt_path=$4
if [[ "$#" -eq 5 ]]; then
validation_prompt_path=$5
else
validation_prompt_path=""
fi

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"
Copy link
Contributor

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.

openai_result=$(echo '{
"model": "'$openai_model'",
"temperature": 0.01,
"messages": [
{
"role": "system",
"content": "'$openai_prompt' \nreport1:\n```'$first_report_content'```\nreport2:\n```'$second_report_content'```"
}
]
}' | envsubst | curl https://api.openai.com/v1/chat/completions \
-w "%{http_code}" \
-o prompt_response.json \
-H "Content-Type: application/json" \
-H "Authorization: Bearer $OPEN_API_KEY" \
-d @-
)

# 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.

fi

# replace lines starting with ' -' (1space) with ' -' (2spaces)
response_content=$(cat prompt_response.json | jq -r '.choices[0].message.content')
new_issues_report_content=$(echo "$response_content" | sed -e 's/^ -/ -/g')
echo "$new_issues_report_content" > "$new_issues_report_path"

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

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!

report_input=$(echo "$new_issues_report_content" | sed 's/"//g' | sed -E 's/\\+$//g' | sed -E 's/\\+ //g')
validation_prompt_content=$(cat "$validation_prompt_path" | sed 's/"/\\"/g' | sed -E 's/\\+$//g' | sed -E 's/\\+ //g')
validation_result=$(echo '{
"model": "'$openai_model'",
"temperature": 0.01,
"messages": [
{
"role": "system",
"content": "'$validation_prompt_content' \nreport1:\n```'$first_report_content'```\nreport2:\n```'$second_report_content'```\nnew_issues:\n```'$report_input'```"
}
]
}' | envsubst | curl https://api.openai.com/v1/chat/completions \
-w "%{http_code}" \
-o prompt_validation_response.json \
-H "Content-Type: application/json" \
-H "Authorization: Bearer $OPEN_API_KEY" \
-d @-
)

# throw error openai_result when is not 200
if [ "$validation_result" != '200' ]; then
echo "::error::OpenAI API call failed with status $validation_result: $(cat prompt_validation_response.json)"
return 1
fi

# replace lines starting with ' -' (1space) with ' -' (2spaces)
response_content=$(cat prompt_validation_response.json | jq -r '.choices[0].message.content')

echo "$response_content" | sed -e 's/^ -/ -/g' >> "$new_issues_report_path"
echo "" >> "$new_issues_report_path"
echo "*Confidence rating presented above is an automatic validation (self-check) of the differences between two reports generated by ChatGPT ${openai_model} model. It has a scale of 1 to 5, where 1 means that all new issues are missing and 5 that all new issues are present*." >> "$new_issues_report_path"
echo "" >> "$new_issues_report_path"
echo "*If confidence rating is low it's advised to look for differences manually by downloading Slither reports for base reference and current commit from job's artifacts*." >> "$new_issues_report_path"
fi
21 changes: 21 additions & 0 deletions contracts/scripts/ci/prompt-difference.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
You are a helpful expert data engineer with expertise in Blockchain and Decentralized Oracle Networks.

Given two reports generated by Slither - a Solidity static analysis tool - provided at the bottom of the reply, your task is to help create a report for your peers with new issues introduced in the second report in order to decrease noise resulting from irrelevant changes to the report, by focusing on a single topic: **New Issues**.

First report is provided under Heading 2 (##) called `report1` and is surrounded by triple backticks (```) to indicate the beginning and end of the report.
Second report is provided under Heading 2 (##) called `report2` and is surrounded by triple backticks (```) to indicate the beginning and end of the report.

First report is report generated by Slither using default branch of the code repository. Second report is report generated by Slither using a feature branch of the code repository. You want to help your peers understand the impact of changes they introduced in the pull request on the codebase and whether they introduced any new issues.

**New Issues**

Provide a bullet point summary of new issues that were introduced in the second report. If a given issue is not present in first report, but is present in the second one, it is considered a new issue. If the count for given issue type is higher in the second report than in the first one, it is considered a new issue.
For each issue include original description text from the report together with severity level, issue ID, line number and a link to problematic line in the code.
Group the issues by their type, which is defined as Heading 2 (##).

Output your response starting from**New Issues** in escaped, markdown text that can be sent as http body to API. Do not wrap output in code blocks.
Extract the name of the file from the first line of the report and title the new report with it in a following way: "# Slither's new issues in: <file_name>"

Remember that it might be possible that second report does not introduce any new issues. In such case, provide an empty report.

Format **New Issues** as Heading 2 using double sharp characters (##). Otherwise, do not include any another preamble and postamble to your answer.
Loading
Loading