Fix audit verifier action test #947
Conversation
|
Warning Rate limit exceeded@0xDEnYO has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces a consistent approach to managing workflow control across multiple GitHub Actions workflows by adding an environment variable Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
.github/workflows_deactivated/ensureDeployProcessIntegrity.yml (1)
Line range hint
92-93: Security Issue: Incorrect group name assignment.The commented line shows the correct group name 'information-security-manager', but the actual assignment uses 'smart-contract-core'. This could lead to incorrect permission checks.
Apply this fix:
- # GROUP_NAME='information-security-manager' - GROUP_NAME='smart-contract-core' + GROUP_NAME='information-security-manager'.github/workflows/protectSecurityRelevantCode.yml (1)
Line range hint
84-85: Security Enhancement: Improve GitHub CLI authentication error handling.The current implementation could expose the token in error messages. Consider using a more secure approach.
Apply this improvement:
- gh auth login --with-token < <(echo "$GH_PAT") || { echo "Failed to login with GitHub CLI"; exit 1; } + if ! gh auth login --with-token < <(echo "$GH_PAT" 2>/dev/null); then + echo "Failed to authenticate with GitHub CLI" + exit 1 + fi
🧹 Nitpick comments (2)
.github/workflows/versionCheck.yml (1)
Line range hint
49-50: Consider usingexit 0for expected failures.When no files are found, this is an expected failure case. Consider using
exit 0instead ofexit 1to indicate that the workflow completed successfully but found no work to do.- echo "CONTINUE=false" >> "$GITHUB_ENV" # explicit assignment, even though already initialized with false - exit 1 + echo "CONTINUE=false" >> "$GITHUB_ENV" # explicit assignment, even though already initialized with false + exit 0.github/workflows/verifyAudit.yml (1)
173-173: Consider consolidating CONTINUE variable assignments.The step sets
CONTINUE=falsetwice: once implicitly through the condition and once explicitly in the script. Consider removing the explicit assignment since the condition already handles this case.- # set CONTINUE to false to ensure that action fails (correctly) if an error occurs unexpectedly - echo "CONTINUE=false" >> "$GITHUB_ENV"Also applies to: 179-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ensureSCCoreDevApproval.yml(1 hunks).github/workflows/protectAuditLabels.yml(1 hunks).github/workflows/protectSecurityRelevantCode.yml(1 hunks).github/workflows/verifyAudit.yml(7 hunks).github/workflows/versionCheck.yml(1 hunks).github/workflows_deactivated/enforceTestCoverage.yml(0 hunks).github/workflows_deactivated/ensureDeployProcessIntegrity.yml(1 hunks).github/workflows_deactivated/ensureSCCoreDevApproval.yml(0 hunks).github/workflows_deactivated/protectAuditCompletedLabel.yml(0 hunks).github/workflows_deactivated/protectAuditFolder.yml(1 hunks)audit/auditLog.json(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows_deactivated/enforceTestCoverage.yml
- .github/workflows_deactivated/protectAuditCompletedLabel.yml
- .github/workflows_deactivated/ensureSCCoreDevApproval.yml
✅ Files skipped from review due to trivial changes (1)
- audit/auditLog.json
🧰 Additional context used
📓 Learnings (4)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
.github/workflows/versionCheck.yml (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
.github/workflows/ensureSCCoreDevApproval.yml (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
.github/workflows_deactivated/ensureDeployProcessIntegrity.yml (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
🪛 actionlint (1.7.4)
.github/workflows/verifyAudit.yml
174-174: shellcheck reported issue in this script: SC2034:warning:12:1: COMMIT_HASHES_FILE appears unused. Verify use (or export if used externally)
(shellcheck)
174-174: shellcheck reported issue in this script: SC2034:warning:13:1: AUDITOR_GIT_HANDLES_FILE appears unused. Verify use (or export if used externally)
(shellcheck)
174-174: shellcheck reported issue in this script: SC2027:warning:125:94: The surrounding quotes actually unquote this. Remove or escape them
(shellcheck)
174-174: shellcheck reported issue in this script: SC2086:info:125:94: Double quote to prevent globbing and word splitting
(shellcheck)
174-174: shellcheck reported issue in this script: SC2086:info:162:19: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: analyze
- GitHub Check: generate-tag
🔇 Additional comments (8)
.github/workflows_deactivated/protectAuditFolder.yml (1)
18-19: LGTM! Environment variable initialization is consistent with team practices.The initialization of the
CONTINUEvariable follows the established pattern and properly controls the workflow execution..github/workflows/protectAuditLabels.yml (1)
18-19: LGTM! Environment variable initialization is consistent.The initialization of the
CONTINUEvariable aligns with the team's practices and effectively controls the workflow execution..github/workflows/ensureSCCoreDevApproval.yml (1)
15-16: LGTM! Implementation aligns with established team practices.Based on the retrieved learning from PR #921, the initialization of the
CONTINUEvariable at the workflow level is the accepted practice in this repository..github/workflows_deactivated/ensureDeployProcessIntegrity.yml (1)
14-15: LGTM! Environment variable initialization is consistent.The initialization of the
CONTINUEvariable follows the established pattern..github/workflows/protectSecurityRelevantCode.yml (1)
16-17: LGTM! Environment variable initialization is consistent.The initialization of the
CONTINUEvariable follows the established pattern..github/workflows/versionCheck.yml (1)
21-22: LGTM! Good practice initializing the CONTINUE variable at job level.The initialization of
CONTINUEtofalseat the job level ensures consistent variable state across all steps..github/workflows/verifyAudit.yml (2)
28-28: LGTM! Good practice initializing the CONTINUE variable at job level.The initialization of
CONTINUEtofalseat the job level ensures consistent variable state across all steps.
398-398: LGTM! Good use of always() with CONTINUE variable.The combination of
always()with theCONTINUEvariable ensures robust execution flow, particularly in error scenarios.Also applies to: 409-409, 416-416
|
|
||
| - name: Check Audit Log | ||
| continue-on-error: true | ||
| id: check-audit-log | ||
| if: env.AUDIT_REQUIRED == 'true' | ||
| if: ${{ always() && env.AUDIT_REQUIRED == 'true' && env.CONTINUE == 'true' }} # always() ensures that validation is always executed, even if env variable is not set | ||
| run: | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix unused variables and quoting issues.
Static analysis identified several issues:
- Unused variables:
COMMIT_HASHES_FILEandAUDITOR_GIT_HANDLES_FILE - Quoting issues that could lead to word splitting
Apply these fixes:
- COMMIT_HASHES_FILE="commit_hashes.txt"
- AUDITOR_GIT_HANDLES_FILE="auditor_handles.txt"And fix the quoting issues:
- echo -e "\033[31mCould not find an audit report in path $AUDIT_REPORT_PATH for contract "$FILENAME".\033[0m"
+ echo -e "\033[31mCould not find an audit report in path $AUDIT_REPORT_PATH for contract \"$FILENAME\".\033[0m"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.4)
174-174: shellcheck reported issue in this script: SC2034:warning:12:1: COMMIT_HASHES_FILE appears unused. Verify use (or export if used externally)
(shellcheck)
174-174: shellcheck reported issue in this script: SC2034:warning:13:1: AUDITOR_GIT_HANDLES_FILE appears unused. Verify use (or export if used externally)
(shellcheck)
174-174: shellcheck reported issue in this script: SC2027:warning:125:94: The surrounding quotes actually unquote this. Remove or escape them
(shellcheck)
174-174: shellcheck reported issue in this script: SC2086:info:125:94: Double quote to prevent globbing and word splitting
(shellcheck)
174-174: shellcheck reported issue in this script: SC2086:info:162:19: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/Facets/DiamondCutFacet.sol (1)
Line range hint
10-10: Update contract version.The contract was modified but the version tag was not updated. Please update the
@custom:versiontag to reflect the changes.-/// @custom:version 1.0.0 +/// @custom:version 1.0.1🧰 Tools
🪛 GitHub Actions: Version Control Checker
[error] Contract was modified but version tag was not updated. Please update the contract's version to reflect the changes.
src/Facets/AcrossFacetV3.sol (1)
Line range hint
15-15: Update contract version.The contract was modified but the version tag was not updated. Please update the
@custom:versiontag to reflect the changes.-/// @custom:version 1.1.0 +/// @custom:version 1.1.1🧰 Tools
🪛 GitHub Actions: Version Control Checker
[error] Contract was modified but version tag was not updated. Please update the contract's version to reflect the changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Facets/AcrossFacetV3.sol(1 hunks)src/Facets/DiamondCutFacet.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
🪛 GitHub Actions: Version Control Checker
src/Facets/DiamondCutFacet.sol
[error] Contract was modified but version tag was not updated. Please update the contract's version to reflect the changes.
src/Facets/AcrossFacetV3.sol
[error] Contract was modified but version tag was not updated. Please update the contract's version to reflect the changes.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: protect_audit_labels
- GitHub Check: check-version
- GitHub Check: run-unit-tests
- GitHub Check: verify-audit
- GitHub Check: analyze
- GitHub Check: generate-tag
lifi-action-bot
changed the title
lifi-action-bot
changed the title
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/verifyAudit.yml (1)
306-309:⚠️ Potential issueFix incorrect variable reference in commit verification.
The code uses
TARGET_COMMITbut should useAUDIT_COMMIT_HASHas noted in past review comments.Apply this fix:
- if echo "$COMMIT_LIST" | grep -q "$TARGET_COMMIT"; then + if echo "$COMMIT_LIST" | grep -q "$AUDIT_COMMIT_HASH"; then
🧹 Nitpick comments (2)
.github/workflows/versionControlAndAuditCheck.yml (1)
540-581: Clean up commented code.There's a large block of commented code related to PR review verification. Either implement this functionality or remove the commented code to maintain cleanliness.
Would you like me to help implement the PR review verification logic or should we remove this commented section?
.github/workflows/verifyAudit.yml (1)
31-31: Consider using GITHUB_TOKEN for improved security.While using a classic PAT token works, consider using the built-in
GITHUB_TOKENwith fine-grained permissions for improved security. This would require:
- Defining necessary permissions in the workflow
- Using
secrets.GITHUB_TOKENinstead ofsecrets.GIT_ACTIONS_BOT_PAT_CLASSIC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/verifyAudit.yml(7 hunks).github/workflows/versionCheck.yml(1 hunks).github/workflows/versionControlAndAuditCheck.yml(1 hunks)src/Facets/AcrossFacetV3.sol(1 hunks)src/Facets/AllBridgeFacet.sol(1 hunks)src/Facets/AmarokFacet.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Facets/AmarokFacet.sol
- src/Facets/AllBridgeFacet.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Facets/AcrossFacetV3.sol
- .github/workflows/versionCheck.yml
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
🪛 actionlint (1.7.4)
.github/workflows/versionControlAndAuditCheck.yml
45-45: shellcheck reported issue in this script: SC2086:info:31:28: Double quote to prevent globbing and word splitting
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:35:27: Double quote to prevent globbing and word splitting
(shellcheck)
89-89: shellcheck reported issue in this script: SC2086:info:84:28: Double quote to prevent globbing and word splitting
(shellcheck)
185-185: shellcheck reported issue in this script: SC2001:style:11:14: See if you can use ${variable//search/replace} instead
(shellcheck)
185-185: shellcheck reported issue in this script: SC2086:info:35:48: Double quote to prevent globbing and word splitting
(shellcheck)
185-185: shellcheck reported issue in this script: SC2086:info:36:27: Double quote to prevent globbing and word splitting
(shellcheck)
185-185: shellcheck reported issue in this script: SC2086:info:39:28: Double quote to prevent globbing and word splitting
(shellcheck)
386-386: shellcheck reported issue in this script: SC2027:warning:111:94: The surrounding quotes actually unquote this. Remove or escape them
(shellcheck)
386-386: shellcheck reported issue in this script: SC2086:info:111:94: Double quote to prevent globbing and word splitting
(shellcheck)
386-386: shellcheck reported issue in this script: SC2086:info:145:19: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: analyze
🔇 Additional comments (8)
.github/workflows/versionControlAndAuditCheck.yml (2)
1-30: LGTM! The workflow configuration is well-structured.The workflow name is correctly spelled, and the trigger events are appropriate for the intended functionality.
257-596: LGTM! The audit verification logic is well-implemented.The audit verification job:
- Properly manages protected labels
- Verifies audit log entries
- Validates audit report paths
- Checks auditor GitHub handles
- Verifies commit hashes
This implementation addresses the previous concerns about missing audit verification logic.
🧰 Tools
🪛 actionlint (1.7.4)
386-386: shellcheck reported issue in this script: SC2027:warning:111:94: The surrounding quotes actually unquote this. Remove or escape them
(shellcheck)
386-386: shellcheck reported issue in this script: SC2086:info:111:94: Double quote to prevent globbing and word splitting
(shellcheck)
386-386: shellcheck reported issue in this script: SC2086:info:145:19: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/verifyAudit.yml (6)
28-28: LGTM! Environment variable declaration follows repository conventions.The
AUDIT_REQUIREDenvironment variable is correctly declared at the job level, which aligns with the repository's conventions as noted in the retrieved learnings.
38-47: LGTM! Proactive label cleanup ensures accuracy.The new step to remove the "AuditCompleted" label at the start of the workflow is a good practice. It prevents false positives by ensuring the label is only present when all current checks pass.
59-59: Early exit improves workflow efficiency.Good practice to exit early when no files are found, preventing unnecessary processing.
166-166: LGTM! Robust condition handling.Using
always()with the environment variable check ensures consistent validation execution, preventing potential skip scenarios.
374-374: LGTM! Conditional label assignment.The condition
if: ${{ env.AUDIT_REQUIRED == 'true' }}correctly ensures the "AuditCompleted" label is only assigned when an audit is required and all checks pass.
17-17: Consider expanding PR trigger events.The workflow currently only triggers on PR 'opened' event. This might miss subsequent changes that could require re-verification of the audit status.
Consider adding more event types like 'synchronize' (triggered when new commits are pushed) and 'reopened' to ensure continuous audit verification:
- types: [opened] + types: [opened, synchronize, reopened]
| echo "" > contracts_for_audit.txt | ||
|
|
||
| ##### get all files modified by this PR | ||
| FILES=$(git diff --name-only "origin/${BASE_REF}" HEAD) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix shell script quoting issues.
Several shell script commands need proper quoting to prevent globbing and word splitting issues.
Apply these fixes:
- Quote the redirection in the while loop
- Quote variables in echo commands
- Quote the file path in cat command
- echo -e "$CONTRACTS" > modified_contracts.txt
+ echo -e "${CONTRACTS}" > modified_contracts.txt
- CONTRACTS=$(cat modified_contracts.txt)
+ CONTRACTS=$(cat "modified_contracts.txt")
- while IFS= read -r FILE_PATH; do
+ while IFS= read -r FILE_PATH; do
echo "Now checking contract: ${FILE_PATH}"
VERSION_TAG=$(grep -E '^/// @custom:version' "${FILE_PATH}" || true)
VERSION=$(echo "${VERSION_TAG}" | sed -E 's/^\/\/\/ @custom:version ([0-9]+\.[0-9]+\.[0-9]+).*$/\1/' || true)Also applies to: 65-69, 84-84, 92-92, 103-138
| COMMIT_LIST=$(gh pr view "$PR_NUMBER" --json commits --jq '.commits[].oid') | ||
|
|
||
| ##### Check if the target commit is in the list | ||
| if echo "$COMMIT_LIST" | grep -q "$TARGET_COMMIT"; then |
There was a problem hiding this comment.
Fix undefined variable reference.
The variable TARGET_COMMIT is used but not defined. This will cause the script to fail.
Apply this diff to fix the issue:
- if echo "$COMMIT_LIST" | grep -q "$TARGET_COMMIT"; then
+ if echo "$COMMIT_LIST" | grep -q "$AUDIT_COMMIT_HASH"; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if echo "$COMMIT_LIST" | grep -q "$TARGET_COMMIT"; then | |
| if echo "$COMMIT_LIST" | grep -q "$AUDIT_COMMIT_HASH"; then |
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)