Fix lint workflow and issues that were re-introduced #1254
Conversation
…on warnings The lint workflow had two critical issues: 1. **Did not fail on warnings**: The workflow used continue-on-error with exit code capture via $?, but $? was always 0 because continue-on-error prevents failures. Fixed by checking step outcomes in a final verification step. 2. **Different results than local linting**: CI ran linters from repo root while local 'pnpm lint' runs per-package scripts from package directories. This caused Solhint's import-path-check rule to produce false warnings in CI. Fixed by running Solhint from each package directory using 'pnpm -r exec'. Additional improvements: - Use git ls-files instead of globs to only lint tracked files (much faster) - Exclude Ignition/build artifacts from JSON linting (939 → 102 files, 89% reduction) - Add --no-warn-ignored to ESLint to suppress ignore pattern warnings - Add --no-error-on-unmatched-pattern to Prettier to handle symlinks gracefully - Simplify workflow structure with fewer, clearer steps Also fixed YAML syntax warning in build-test.yml (removed redundant branches filter).
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical issues in the lint workflow that prevented it from properly failing on errors and caused inconsistent results between CI and local linting. The main improvements are:
- Fixed the continue-on-error mechanism by checking step outcomes instead of exit codes
- Aligned CI linting with local behavior by running Solhint from package directories using
pnpm -r exec - Improved performance by using
git ls-filesinstead of globs and excluding build artifacts from JSON linting (89% reduction in files checked)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
.github/workflows/lint.yml |
Refactored workflow to consolidate linting steps, fix failure detection via step outcomes, run Solhint from package directories to match local behavior, and optimize file discovery with git ls-files |
.github/workflows/build-test.yml |
Removed redundant branches: '*' filter to fix YAML syntax warning |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… scripts - Add proper interfaces for artifact structure in decode-creation-args.ts - Replace any[] with typed tuple for event args in read-immutables-from-event.ts - Fixes all pnpm lint:ts warnings
- Add -r flag to all xargs commands to handle empty input gracefully - Add || true to grep commands to prevent exit code 1 when no matches - Add error handling when no package.json found for Solidity files - Ensures workflow doesn't fail on edge cases with empty file lists - Fails with error if package.json not found (unexpected condition)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Run solhint in each workspace package that has contracts (check mode) | ||
| pnpm -r exec bash -c 'if [ -d "contracts" ]; then npx solhint --max-warnings=0 --noPrompt --noPoster "contracts/**/*.sol" 2>/dev/null || exit 1; fi' | ||
| # Check formatting with prettier | ||
| git ls-files '*.sol' | xargs -r npx prettier --check --cache --log-level warn |
There was a problem hiding this comment.
The -r flag for xargs is a GNU extension and may not be available on all systems (macOS uses BSD xargs). Consider using xargs with a conditional check or find -print0 | xargs -0 pattern for better portability. This same issue affects lines 144, 145, 148, 149, 158, 159, 162, 163, 173, 176, 185, 186, 189, and 190.
| echo "Linting changed TypeScript/JavaScript files..." | ||
| cat changed_ts_js.txt | xargs -r npx eslint --max-warnings=0 --no-warn-ignored | ||
| cat changed_ts_js.txt | xargs -r npx prettier --check --cache --log-level warn --no-error-on-unmatched-pattern |
There was a problem hiding this comment.
[nitpick] Missing else clause to skip linting when no files have changed. Without this, if ts_js_count is 0, the step will succeed but won't echo any message. While not critical, this is inconsistent with the 'all' mode branch which explicitly handles all cases. For consistency and clarity, add an else clause with echo 'No TypeScript/JavaScript files to lint.' This same pattern affects the Markdown (lines 160-164), JSON (lines 174-177), and YAML (lines 187-191) linting steps.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1254 +/- ##
=======================================
Coverage 84.05% 84.06%
=======================================
Files 42 42
Lines 2070 2071 +1
Branches 615 615
=======================================
+ Hits 1740 1741 +1
Misses 330 330
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Now that lint issues were cleaned update, it has became apparent the lint workflow had two issues:
Did not fail on warnings: The workflow used continue-on-error with exit code capture via$?, but $ ? was always 0 because continue-on-error prevents failures. Fixed by checking step outcomes in a final verification step.
Different results than local linting: CI ran linters from repo root while local 'pnpm lint' runs per-package scripts from package directories. This caused Solhint's import-path-check rule to produce false warnings in CI. Fixed by running Solhint from each package directory using 'pnpm -r exec'.
Additional improvements:
Also fixed YAML syntax warning in build-test.yml (removed redundant branches filter).
Adding fixes for linting issues introduced on main in interim period prior to globalling fixing lint issues.