fix(ci): fix solidity analysis workflow issues#7808
Conversation
|
|
Claude finished @yorhodes's task in 41s —— View job Security Review SummaryOverviewThis PR fixes two CI-related issues in the Solidity analysis workflows. Since the changes are limited to CI infrastructure (shell script and Foundry configuration), they don't involve smart contract code modifications. File 1:
|
|
Warning Rate limit exceeded
⌛ 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. 📥 CommitsReviewing files that changed from the base of the PR and between e1c34f178aaab3de00a5cbc30185c215ddcbe47d and 94588d7. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughReplaces JSON-based changeset-status lookup with direct parsing of Changes
Sequence Diagram(s)(Skipped — change is small and sequence visualization not required.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7808 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
🚀 New features to boost your workflow:
|
larryob
left a comment
larryob
left a comment
There was a problem hiding this comment.
LGTM. I'm not sure that this fixes the error seen in jobs like the following since the PR has a changeset: https://github.com/hyperlane-xyz/hyperlane-monorepo/actions/runs/21082393759/job/60638835659?pr=7668.
|
Claude finished @yorhodes's task in 1m 16s —— View job Trail of Bits Security Analysis
Security FindingsSeverity: None (No Security Issues) This PR modifies CI infrastructure files only - no smart contract code changes. 1. Shell Script Analysis (
|
| Check | Status |
|---|---|
Command injection via $REQUIRED_LEVEL |
✅ Safe - only used in case statement |
Command injection via $PACKAGE |
✅ Safe - hardcoded value |
Command injection via $STATUS_FILE |
✅ Safe - uses $$ (PID) for uniqueness |
| jq argument injection | ✅ Safe - --arg pkg properly escapes values |
| Temp file cleanup | ✅ Safe - trap ensures cleanup on exit |
set -euo pipefail |
✅ Present - proper error handling |
Note: The $$ PID-based filename is predictable but acceptable for this use case since the file is created and consumed locally within the same script execution.
2. Foundry Configuration Analysis (solidity/foundry.toml)
Change: Added bytecode_hash = "none" to [profile.ci] section.
Security Review:
- ✅ This setting only affects the CI profile, not production builds
- ✅ Required for Foundry v1.5.0+ compatibility when
cbor_metadata = false - ✅ No security impact - this controls metadata embedding in compiled bytecode
3. Response to Larry's Review Comment
Larry noted uncertainty about whether this fixes the error in PR #7668.
Analysis: Looking at the original script on main, the issue was:
pnpm changeset status --output "$STATUS_FILE" 2>/dev/null || trueThis would suppress errors AND ignore the exit code, then proceed to run jq on a file that might not exist. The new code:
- Removes
2>/dev/nullto show error output - Captures the exit code explicitly
- Critically: Checks if the file exists before proceeding
The fix addresses the root cause: if pnpm changeset status fails to create the output file for any reason (not just missing changesets), the script now fails with a clear error message instead of a cryptic jq error.
However, this PR adds debug output which is appropriate for investigation but may be verbose for production. Consider removing the DEBUG: lines once the issue is resolved.
Hyperlane-Specific Concerns
| Concern | Applicable |
|---|---|
| Reentrancy in message handling | ❌ No contract changes |
| Access control on privileged functions | ❌ No contract changes |
| Merkle proof verification correctness | ❌ No contract changes |
| Domain ID validation | ❌ No contract changes |
| ISM threshold and validator set security | ❌ No contract changes |
| Warp route collateral accounting | ❌ No contract changes |
Verdict
Approved from a security perspective. This is a straightforward CI fix with proper defensive programming. The changes improve error handling and debugging without introducing vulnerabilities.
|
Claude finished @yorhodes's task in 52s —— View job Trail of Bits Security Analysis
ScopeThis PR modifies CI infrastructure only - no smart contract code changes. Therefore, the Trail of Bits smart contract vulnerability analysis (building-secure-contracts, differential-review for Solidity, etc.) is not applicable here. Security FindingsSeverity: None (No Security Issues) 1. Shell Script Analysis (
|
| Security Check | Status | Details |
|---|---|---|
Command injection via $REQUIRED_LEVEL |
✅ Safe | Only used in case statement matching (line 58-60) |
Command injection via $PACKAGE |
✅ Safe | Hardcoded value @hyperlane-xyz/core (line 10) |
Command injection via $STATUS_FILE |
✅ Safe | Uses $$ (PID) for uniqueness, consumed locally |
| jq argument injection | ✅ Safe | --arg pkg "$PACKAGE" properly escapes values (line 54) |
| Temp file cleanup | ✅ Safe | trap "rm -f $STATUS_FILE" EXIT ensures cleanup (line 20) |
| Error handling | ✅ Present | set -euo pipefail at script start (line 7) |
Changes reviewed:
- Added debug output for environment visibility
- Added main branch fetch for shallow clones in CI
- Changed from
2>/dev/null || trueto explicit exit code capture (set +e/set -e) - Added file existence check before
jqparsing
2. Foundry Configuration Analysis (solidity/foundry.toml)
Change: Added bytecode_hash = "none" to [profile.ci] section (line 22).
| Check | Status |
|---|---|
| Scope limited to CI profile | ✅ Yes - only affects CI builds, not production |
| Required for Foundry v1.5.0+ | ✅ Correct - cbor_metadata = false requires this |
| Security impact | ✅ None - controls metadata embedding only |
3. Response to Larry's Review Comment
Larry noted uncertainty about whether this fixes the error in PR #7668 job.
Analysis: The original script had:
pnpm changeset status --output "$STATUS_FILE" 2>/dev/null || trueThis would:
- Suppress stderr (
2>/dev/null) - Ignore exit codes (
|| true) - Proceed to run
jqon a potentially non-existent file
The fix addresses this by:
- Showing stderr for debugging
- Capturing the exit code explicitly
- Critically: Checking if the file exists before proceeding (lines 45-51)
If pnpm changeset status fails to create the output file for any reason (shallow clone issues, missing main branch, no changesets, etc.), the script now fails with a clear error message instead of a cryptic jq error like null: argument 1 is null.
The additional main branch fetch (lines 30-33) specifically addresses the shallow clone issue that was likely causing the PR #7668 failure - changeset status needs the base branch to compare against.
Hyperlane-Specific Security Concerns
| Concern | Applicable |
|---|---|
| Reentrancy in message handling | ❌ No contract changes |
| Access control on privileged functions | ❌ No contract changes |
| Merkle proof verification correctness | ❌ No contract changes |
| Domain ID validation | ❌ No contract changes |
| ISM threshold and validator set security | ❌ No contract changes |
| Warp route collateral accounting | ❌ No contract changes |
Verdict
Approved from a security perspective. This is a straightforward CI fix with proper defensive programming:
- Shell script follows security best practices
- No command injection vulnerabilities
- Proper error handling and cleanup
- Foundry configuration change is correct for v1.5.0+ compatibility
The debug output is appropriate for diagnosing CI issues and should help identify root causes of future failures.
|
Claude finished @yorhodes's task in 1m 18s —— View job Trail of Bits Security Analysis
ScopeThis PR modifies CI infrastructure only - no smart contract code changes. The Trail of Bits smart contract vulnerability analysis (building-secure-contracts, differential-review, property-based-testing, variant-analysis) is not applicable to this PR. Security FindingsSeverity: None (No Security Issues) 1. Shell Script Analysis (
|
| Security Check | Status | Details |
|---|---|---|
Command injection via $REQUIRED_LEVEL |
✅ Safe | Only used in case statement matching (line 30) |
Command injection via $PACKAGE |
✅ Safe | Hardcoded value @hyperlane-xyz/core (line 10) |
Command injection via $CHANGESET_FILES |
✅ Safe | Used in for loop, files validated with [ -f "$file" ] |
awk script injection via $PACKAGE |
✅ Safe | Passed via -v pkg="$PACKAGE" which escapes properly |
| Path traversal | ✅ Safe | find is scoped to .changeset with -maxdepth 1 |
| Error handling | ✅ Present | set -euo pipefail at script start (line 7) |
Key Change: The new approach parses changeset markdown files directly using find + awk instead of relying on pnpm changeset status --output. This is more robust as it:
- Doesn't depend on changeset CLI behavior
- Doesn't require git history/comparison against main branch
- Works reliably in shallow clones
2. Foundry Configuration Analysis (solidity/foundry.toml)
Change: Added bytecode_hash = "none" to [profile.ci] section (line 22).
| Check | Status |
|---|---|
| Scope limited to CI profile | ✅ Yes - only affects CI builds, not production |
| Required for Foundry v1.5.0+ | ✅ Correct - cbor_metadata = false requires this |
| Security impact | ✅ None - controls metadata embedding only |
3. Response to Larry's Review Comment
Larry noted uncertainty about whether this fixes the error in PR #7668 job.
Analysis of the #7668 Failure:
From the job log (line 594):
jq: error: Could not open file .changeset-status-6150.json: No such file or directory
The OLD script used:
pnpm changeset status --output "$STATUS_FILE" 2>/dev/null || true
FOUND_LEVEL=$(jq -r ... "$STATUS_FILE")This failed because:
pnpm changeset status --outputrequires git history to compare against the base branch- In CI's shallow clone environment, this comparison often fails silently
- The
|| truesuppressed the error but didn't create the file jqthen failed trying to read a non-existent file
Does This PR Fix It?
Yes, definitively. The script has been completely rewritten to:
- Use
find .changeset -name "*.md"to locate changeset files directly - Parse YAML frontmatter using
awkinstead of relying on CLI output - Remove all dependency on
pnpm changeset statusand its git comparison logic
This approach is fundamentally more robust because it reads changeset files directly from disk rather than depending on git history operations.
Hyperlane-Specific Security Concerns
| Concern | Applicable |
|---|---|
| Reentrancy in message handling | ❌ No contract changes |
| Access control on privileged functions | ❌ No contract changes |
| Merkle proof verification correctness | ❌ No contract changes |
| Domain ID validation | ❌ No contract changes |
| ISM threshold and validator set security | ❌ No contract changes |
| Warp route collateral accounting | ❌ No contract changes |
Verdict
Approved from a security perspective. This PR:
- Fixes the root cause of the jq error by removing dependency on
pnpm changeset status - Uses secure shell scripting practices
- Correctly configures Foundry v1.5.0+ for CI bytecode comparison
The rewritten script is more robust and should resolve the intermittent CI failures.
Rationale for Direct File ParsingThis PR parses Related Issue: changesets/changesets#700 The CLI uses This issue has been open since December 2021 with no resolution. Multiple users have reported the same problem across different CI platforms (GitHub Actions, Azure DevOps, Bitbucket). One commenter in the issue asked the same question we had:
Given this is a known limitation of the changesets CLI, parsing the files directly is a reasonable workaround that works reliably in all CI environments. |
yorhodes
force-pushed
the
fix-ci-analysis-checks
branch
from
9ed73c0 to
e1c34f1
Compare
|
Claude finished @yorhodes's task in 58s —— View job Trail of Bits Security Analysis
ScopeThis PR modifies CI infrastructure only - no smart contract code changes. The Trail of Bits security skills for Solidity analysis ( Security FindingsSeverity: None (No Security Issues) 1. Shell Script Analysis (
|
| Security Check | Status | Details |
|---|---|---|
Command injection via $REQUIRED_LEVEL |
✅ Safe | Only used in case statement matching (line 24) |
Command injection via $PACKAGE |
✅ Safe | Hardcoded value @hyperlane-xyz/core (line 14) |
Glob injection via .changeset/*.md |
✅ Safe | Glob is static, not user-controlled |
| Error handling | ✅ Present | set -euo pipefail at script start (line 11) |
| Grep regex injection | ✅ Safe | $PACKAGE is hardcoded, not user input |
Key Improvements:
- Removed dependency on
pnpm changeset statuswhich fails in shallow clones - Removed temporary file creation/cleanup (no longer needed)
- Uses simple
grepto search changeset files directly - Checks bump levels in priority order (major > minor > patch)
Minor Note: The 2>/dev/null on grep commands is acceptable here as it suppresses errors when no .changeset/*.md files exist, which is a valid state (no changesets).
2. Foundry Configuration Analysis (solidity/foundry.toml)
Change: Added bytecode_hash = "none" to [profile.ci] section (line 22).
| Check | Status |
|---|---|
| Scope limited to CI profile | ✅ Yes - only affects CI builds, not production |
| Required for Foundry v1.5.0+ | ✅ Correct - cbor_metadata = false requires this |
| Security impact | ✅ None - controls metadata embedding only |
This is a configuration fix to make bytecode comparison work with newer Foundry versions. The setting only affects the CI profile and has no security implications.
3. Addressing Larry's Review Comment
Larry expressed uncertainty about whether this fixes the error in PR #7668 job.
Analysis: The #7668 job failed with:
jq: error: Could not open file .changeset-status-6150.json: No such file or directory
Root Cause: The OLD script used:
pnpm changeset status --output "$STATUS_FILE" 2>/dev/null || true
FOUND_LEVEL=$(jq -r ... "$STATUS_FILE")This failed because:
pnpm changeset statususesgit merge-baseinternally- In CI shallow clones, this fails to find where HEAD diverged from main
- The
|| truesuppressed the error but didn't create the output file jqthen failed trying to read a non-existent file
Does This PR Fix It?
Yes, definitively. The new approach:
- Uses
grepto search.changeset/*.mdfiles directly - Parses the YAML frontmatter for package bump levels
- Has zero dependency on git history or
pnpm changeset status - Works reliably in all CI environments (shallow clones, full clones, etc.)
Hyperlane-Specific Security Concerns
| Concern | Applicable |
|---|---|
| Reentrancy in message handling | ❌ No contract changes |
| Access control on privileged functions | ❌ No contract changes |
| Merkle proof verification correctness | ❌ No contract changes |
| Domain ID validation | ❌ No contract changes |
| ISM threshold and validator set security | ❌ No contract changes |
| Warp route collateral accounting | ❌ No contract changes |
Verdict
Approved from a security perspective. This PR:
- Fixes the root cause of CI failures by removing dependency on
pnpm changeset status - Uses secure shell scripting practices with proper error handling
- Correctly configures Foundry v1.5.0+ for CI bytecode comparison
- The rewritten script is simpler and more robust
1. Parse changeset files directly instead of using CLI The changeset CLI uses git merge-base which fails in CI shallow clones. Simple grep-based parsing works in any environment. See: changesets/changesets#700 2. Add bytecode_hash=none for CI profile Foundry v1.5.0+ requires this when cbor_metadata=false.
yorhodes
force-pushed
the
fix-ci-analysis-checks
branch
from
e1c34f1 to
94588d7
Compare
|
Claude finished @yorhodes's task in 1m 5s —— View job Trail of Bits Security Analysis
ScopeThis PR modifies CI infrastructure only - no smart contract code changes. The Trail of Bits smart contract security skills ( Security FindingsSeverity: None (No Security Issues) 1. Shell Script Analysis
|
| Security Check | Status | Details |
|---|---|---|
Command injection via $REQUIRED_LEVEL |
✅ Safe | Only used in case statement (lines 27-29) |
Command injection via $PACKAGE |
✅ Safe | Hardcoded value @hyperlane-xyz/core (line 18) |
Command injection via $BASE_REF |
✅ Safe | Used with git diff --name-only which properly handles refs |
| Glob handling | ✅ Safe | `2>/dev/null |
xargs cat usage |
✅ Safe | File list comes from git, not user input |
Grep pattern with $PACKAGE |
✅ Safe | Hardcoded value, not user-controlled |
| Error handling | ✅ Present | set -euo pipefail at script start (line 14) |
Key Improvements:
- Removed dependency on
pnpm changeset statuswhich fails in shallow clones - Removed temporary file creation/cleanup (no longer needed)
- Uses git directly to find new changeset files relative to base ref
.github/scripts/check-diff-changeset.sh
| Security Check | Status | Details |
|---|---|---|
| Directory validation | ✅ Present | Checks [ -d "$BASE_DIR" ] and [ -d "$HEAD_DIR" ] (lines 23-30) |
| Command injection via dirs | ✅ Safe | Used with diff --unified --recursive which handles paths properly |
$SCRIPT_DIR resolution |
✅ Safe | Uses ${BASH_SOURCE[0]} for reliable path resolution (line 32) |
| Error handling | ✅ Present | set -euo pipefail at script start (line 10) |
2. Foundry Configuration Analysis (solidity/foundry.toml)
Change: Added bytecode_hash = "none" to [profile.ci] section (line 22).
| Check | Status |
|---|---|
| Scope limited to CI profile | ✅ Yes - only affects CI builds, not production |
| Required for Foundry v1.5.0+ | ✅ Correct - cbor_metadata = false requires this |
| Security impact | ✅ None - controls metadata embedding only |
3. CI Status
| Run | Status |
|---|---|
| codespell | ✅ Passed |
| Lint PR | ✅ Passed |
| Other checks | ⏳ In progress |
4. Response to Larry's Review Comment
Larry expressed uncertainty about whether this fixes the error in PR #7668 job.
Analysis: The #7668 job failed with:
jq: error: Could not open file .changeset-status-6150.json: No such file or directory
Root Cause: The OLD script used:
pnpm changeset status --output "$STATUS_FILE" 2>/dev/null || true
FOUND_LEVEL=$(jq -r ... "$STATUS_FILE")This failed because:
pnpm changeset statususesgit merge-baseinternally- In CI shallow clones, this fails to find where HEAD diverged from main
- The
|| truesuppressed the error but didn't create the output file jqthen failed trying to read a non-existent file
Does This PR Fix It?
Yes, definitively. The new approach:
- Uses
git diff --name-only --diff-filter=A "$BASE_REF"to find new changeset files - Parses the changeset content directly with
grep - Has zero dependency on
pnpm changeset statusorjq - Works reliably in all CI environments (shallow clones, full clones, etc.)
Hyperlane-Specific Security Concerns
| Concern | Applicable |
|---|---|
| Reentrancy in message handling | ❌ No contract changes |
| Access control on privileged functions | ❌ No contract changes |
| Merkle proof verification correctness | ❌ No contract changes |
| Domain ID validation | ❌ No contract changes |
| ISM threshold and validator set security | ❌ No contract changes |
| Warp route collateral accounting | ❌ No contract changes |
Verdict
Approved from a security perspective. This PR:
- Fixes the root cause of CI failures by removing dependency on
pnpm changeset status - Uses secure shell scripting practices with proper error handling and input validation
- Correctly configures Foundry v1.5.0+ for CI bytecode comparison
- The rewritten script is simpler, more robust, and eliminates the shallow clone issue
2 participants
Summary
Fixes issues with the solidity analysis CI workflows:
Changeset CLI fails in shallow clones - The
pnpm changeset statuscommand usesgit merge-baseinternally, which fails in CI shallow clones (fetch-depth: 1) with:Solution: Parse
.changeset/*.mdfiles directly instead of using the CLI. This extracts the package bump levels from YAML frontmatter without requiring git history.Bytecode check fails with Foundry v1.5.0+ - The CI profile in
foundry.tomlsetscbor_metadata = falsebut was missingbytecode_hash = "none". Newer Foundry versions require this combination.Root Cause
This is a known limitation of the changesets CLI documented in changesets/changesets#700 (open since December 2021). Multiple users have reported the same issue across GitHub Actions, Azure DevOps, and Bitbucket.
As one commenter noted:
Since there's no upstream fix, parsing the changeset files directly is a reliable workaround that works in all CI environments.
References
changeset statussucceeds locally, fails in CI with a changeset file verifiably present changesets/changesets#700Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.