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.
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:
|
Address PR feedback: - Replace xargs -r (GNU-specific) with tr/xargs -0 for cross-platform compatibility - Add empty-file handling to prevent stdin processing - Add consistent messaging for all file-type scenarios The xargs -r flag is a GNU extension not available on BSD/macOS. Using null-delimited input (tr '\n' '\0' | xargs -0) is POSIX-compliant and works across all platforms.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
With all lint issues were addressed it became apparent the lint GH 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).
Added fixes for linting issues introduced on main in interim period prior to globalling fixing lint issues.
Added ignore of .claude/ in .gitignore.