ci: avoid storage-check failures for newly added contracts/libraries#1008
Conversation
📝 WalkthroughWalkthroughThis change introduces a new Bash script and test suite to manage Solidity contract storage check matrices, replacing inline matrix generation logic in two GitHub Actions workflows. The new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/scripts/test-build-storage-check-matrix.sh (1)
43-60: Add a regression for invalidBASE_SHAbehavior.Please add a scenario that passes a non-resolvable SHA and asserts the intended behavior (fail fast vs explicit fallback). This prevents silent behavior drift in CI.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/scripts/build-storage-check-matrix.sh.github/scripts/test-build-storage-check-matrix.sh.github/workflows/core-contracts-storage-check.yml.github/workflows/diamond-storage-check.yml
| MATRIX_ENTRIES="" | ||
|
|
||
| while IFS= read -r FILE; do | ||
| [[ -z "$FILE" ]] && continue | ||
|
|
||
| # Only process Solidity files inside packages/contracts | ||
| [[ "$FILE" != packages/contracts/*.sol ]] && continue | ||
|
|
||
| CONTRACT_NAME="$(basename "$FILE" .sol)" | ||
| CONTRACT_PATH="${FILE#packages/contracts/}" | ||
| ENTRY="${CONTRACT_PATH}:${CONTRACT_NAME}" | ||
|
|
||
| # Newly-added contracts/libraries should be skipped from storage layout diff checks, | ||
| # because no baseline artifact exists for them yet. | ||
| if [[ -n "$BASE_SHA" ]] && git cat-file -e "${BASE_SHA}:${FILE}" 2>/dev/null; then | ||
| MATRIX_ENTRIES+="${ENTRY}"$'\n' | ||
| elif [[ -z "$BASE_SHA" ]]; then | ||
| # Fallback for unusual contexts with no baseline sha. | ||
| MATRIX_ENTRIES+="${ENTRY}"$'\n' | ||
| else | ||
| echo "Skipping new file without baseline on base sha: ${FILE}" >&2 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the script exists and read the relevant lines
cat -n .github/scripts/build-storage-check-matrix.sh | head -50Repository: ubiquity/ubiquity-dollar
Length of output: 1714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
repo_root="$(pwd)"
cd "$tmp"
git init -q
git config user.name "ci"
git config user.email "ci@example.com"
mkdir -p packages/contracts/src/dollar/core
cat > packages/contracts/src/dollar/core/ExistingCore.sol <<'EOF'
contract ExistingCore { uint256 public a; }
EOF
git add .
git commit -qm "base"
bad_sha="0000000000000000000000000000000000000000"
echo "Testing with bad_sha: $bad_sha"
out="$(bash "$repo_root/.github/scripts/build-storage-check-matrix.sh" "packages/contracts/src/dollar/core/ExistingCore.sol" "$bad_sha" 2>&1 || true)"
echo "Output: $out"Repository: ubiquity/ubiquity-dollar
Length of output: 330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
repo_root="$(pwd)"
cd "$tmp"
git init -q
git config user.name "ci"
git config user.email "ci@example.com"
mkdir -p packages/contracts/src/dollar/core
cat > packages/contracts/src/dollar/core/ExistingCore.sol <<'EOF'
contract ExistingCore { uint256 public a; }
EOF
git add .
git commit -qm "base"
# Create a temporary script wrapper to test
cat > test_script.sh <<'TESTEOF'
#!/usr/bin/env bash
set -euo pipefail
CHANGED_FILES="${1:-}"
BASE_SHA="${2:-}"
if [[ -z "${CHANGED_FILES//[[:space:]]/}" ]]; then
echo "[]"
exit 0
fi
MATRIX_ENTRIES=""
while IFS= read -r FILE; do
[[ -z "$FILE" ]] && continue
[[ "$FILE" != packages/contracts/*.sol ]] && continue
CONTRACT_NAME="$(basename "$FILE" .sol)"
CONTRACT_PATH="${FILE#packages/contracts/}"
ENTRY="${CONTRACT_PATH}:${CONTRACT_NAME}"
if [[ -n "$BASE_SHA" ]] && git cat-file -e "${BASE_SHA}:${FILE}" 2>/dev/null; then
MATRIX_ENTRIES+="${ENTRY}"$'\n'
elif [[ -z "$BASE_SHA" ]]; then
MATRIX_ENTRIES+="${ENTRY}"$'\n'
else
echo "Skipping new file without baseline on base sha: ${FILE}" >&2
fi
done <<< "$(printf '%s\n' "$CHANGED_FILES" | tr ' ' '\n')"
printf '%s' "$MATRIX_ENTRIES" | jq -R -s -c 'split("\n")[:-1]'
TESTEOF
bad_sha="0000000000000000000000000000000000000000"
echo "=== Test 1: Invalid BASE_SHA ==="
bash test_script.sh "packages/contracts/src/dollar/core/ExistingCore.sol" "$bad_sha" 2>&1 || true
echo ""
echo "=== Test 2: Empty BASE_SHA (no baseline) ==="
bash test_script.sh "packages/contracts/src/dollar/core/ExistingCore.sol" "" 2>&1 || true
echo ""
echo "=== Test 3: Valid BASE_SHA (current commit) ==="
valid_sha=$(git rev-parse HEAD)
bash test_script.sh "packages/contracts/src/dollar/core/ExistingCore.sol" "$valid_sha" 2>&1 || trueRepository: ubiquity/ubiquity-dollar
Length of output: 477
Unvalidated BASE_SHA silently skips real files from storage checks.
If BASE_SHA is set to an invalid/unavailable commit hash, git cat-file fails for every file. The script then treats all files (existing or new) as "new" and skips them, resulting in an empty matrix. The error message "Skipping new file" is misleading when the root cause is an invalid BASE_SHA.
This creates a silent bypass risk: storage layout diffs won't be checked when they should be.
The proposed fix validates BASE_SHA upfront before the loop and exits early if it's unavailable, making the failure explicit rather than silent.
| MATRIX=$(bash .github/scripts/build-storage-check-matrix.sh "$CHANGED_CONTRACTS" "$BASE_SHA") | ||
| echo "matrix=$MATRIX" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant lines
cat -n .github/workflows/core-contracts-storage-check.yml | sed -n '35,40p'Repository: ubiquity/ubiquity-dollar
Length of output: 377
🏁 Script executed:
# Also run the verification script provided
rg -n '>> \$GITHUB_OUTPUT' .github/workflows/core-contracts-storage-check.ymlRepository: ubiquity/ubiquity-dollar
Length of output: 120
Quote $GITHUB_OUTPUT in redirection.
Line 38 uses an unquoted variable in the redirection target. Quote it to follow shell best practices and avoid potential word-splitting issues:
- echo "matrix=$MATRIX" >> $GITHUB_OUTPUT
+ echo "matrix=$MATRIX" >> "$GITHUB_OUTPUT"📝 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.
| MATRIX=$(bash .github/scripts/build-storage-check-matrix.sh "$CHANGED_CONTRACTS" "$BASE_SHA") | |
| echo "matrix=$MATRIX" >> $GITHUB_OUTPUT | |
| MATRIX=$(bash .github/scripts/build-storage-check-matrix.sh "$CHANGED_CONTRACTS" "$BASE_SHA") | |
| echo "matrix=$MATRIX" >> "$GITHUB_OUTPUT" |
| MATRIX=$(bash .github/scripts/build-storage-check-matrix.sh "$CHANGED_LIBS" "$BASE_SHA") | ||
| echo "matrix=$MATRIX" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and verify lines 41-42
head -50 .github/workflows/diamond-storage-check.yml | tail -15Repository: ubiquity/ubiquity-dollar
Length of output: 754
🏁 Script executed:
# Also search for the specific pattern to confirm the exact line
rg -n 'GITHUB_OUTPUT' .github/workflows/diamond-storage-check.ymlRepository: ubiquity/ubiquity-dollar
Length of output: 120
Quote $GITHUB_OUTPUT in the redirection.
Line 42: $GITHUB_OUTPUT should be quoted to prevent issues if the variable contains whitespace or special characters. Use >> "$GITHUB_OUTPUT" instead.
1 participant
Summary
Fixes #972 by preventing storage-layout checks from running against newly added contracts/libraries that have no baseline artifact yet.
What changed
.github/scripts/build-storage-check-matrix.sh:github.event.pull_request.base.shaorgithub.event.before)..github/workflows/core-contracts-storage-check.yml.github/workflows/diamond-storage-check.yml.github/scripts/test-build-storage-check-matrix.shWhy this fixes the issue
For newly added contracts/libraries, there is no prior run artifact on the base side. Attempting to compare them causes:
No workflow run found with an artifact named ...By skipping only entries absent from the base commit, we avoid false failures while preserving collision detection for real storage changes on existing files.
QA
Executed locally:
Result:
[]matrix)[]), so check does not fail on missing baseline artifactConsole output: