Repository navigation
ci: add a shellcheck job for shell scripts - #442
Merged
Merged
Conversation
Scoped to files changed in each push/PR, not the whole tree. A full scan finds 816 pre-existing issues across test/*.sh, mostly info/style level, which would block every PR on unrelated legacy content. Follows the same diff-scoping approach as proot-rs's own shellcheck job, extended to also cover .bash and .bats. proot-rs's own \.sh$ filter misses both, and would have caught neither of the two real bugs (SC2145, SC2128) found in this session's own test/helper.bash. shellcheck lints whole files, not just changed lines, so touching any legacy .sh file surfaces that file's full existing backlog. Checked the files most likely to get touched soon: test-chroot01.sh has 2 pre-existing issues, test-tempdire.sh has 12, the three test-socket*.sh have none. Not blocking today, but worth knowing before someone picks up one of the known-flaky tests next.
|
oxr463
added a commit
that referenced
this pull request
Oct 2, 2026
…rename Renaming test/test-c6b77b77.sh (formerly test-cb1143ab.sh before this branch's sweep, naming aside) and its siblings made git diff treat their new paths as changed content, so shellcheck ran on files it had never checked before and surfaced issues the diff-scoped job was built to avoid (#442). -M lets git diff tell a byte-identical rename (R100) apart from one that actually changed, so only genuinely new or modified content gets linted. Also fixes test-make-under-proot.sh's own pre-existing shellcheck issues (missing shebang, unquoted expansions), since it got a real one-line edit as part of the rename and is no longer a pure R100. Drops .gitremotes, a local scratch file that ended up staged by an earlier `git add -A`. Claude-Session: https://claude.ai/code/session_016jPQ9wt6qox2XEbE71wwCW
oxr463
added a commit
that referenced
this pull request
Oct 2, 2026
…#451) * refactor(test): give the hash-named black-box tests descriptive names 60% of test/ used opaque hash names like test-5bed7141.c, making it impossible to tell what a test covers without opening it. Renamed the 122 affected files (test/GNUmakefile's wildcard-driven pattern rules pick up the new names automatically; only the hardcoded special-case rules needed updating), verified the full suite still produces the same pass/fail/skip results. Closes #164. Claude-Session: https://claude.ai/code/session_016jPQ9wt6qox2XEbE71wwCW * ci(shellcheck): don't re-lint a file's pre-existing issues on a pure rename Renaming test/test-c6b77b77.sh (formerly test-cb1143ab.sh before this branch's sweep, naming aside) and its siblings made git diff treat their new paths as changed content, so shellcheck ran on files it had never checked before and surfaced issues the diff-scoped job was built to avoid (#442). -M lets git diff tell a byte-identical rename (R100) apart from one that actually changed, so only genuinely new or modified content gets linted. Also fixes test-make-under-proot.sh's own pre-existing shellcheck issues (missing shebang, unquoted expansions), since it got a real one-line edit as part of the rename and is no longer a pure R100. Drops .gitremotes, a local scratch file that ended up staged by an earlier `git add -A`. Claude-Session: https://claude.ai/code/session_016jPQ9wt6qox2XEbE71wwCW * test: revert cosmetic string-literal edits in three renamed tests These three files got an unnecessary touch-up during the rename sweep, updating an embedded /tmp path template or socket name to match the new filename. mktemp(3)'s XXXXXX suffix already guarantees uniqueness, so the prefix text was cosmetic, not functional - and touching that one line was enough to make SonarCloud treat each file as new code and flag the same /tmp usage pattern this suite has used safely for years. Reverting restores all 122 renamed files to genuine pure renames. Claude-Session: https://claude.ai/code/session_016jPQ9wt6qox2XEbE71wwCW
oxr463
added a commit
that referenced
this pull request
Oct 2, 2026
…#451) * refactor(test): give the hash-named black-box tests descriptive names 60% of test/ used opaque hash names like test-5bed7141.c, making it impossible to tell what a test covers without opening it. Renamed the 122 affected files (test/GNUmakefile's wildcard-driven pattern rules pick up the new names automatically; only the hardcoded special-case rules needed updating), verified the full suite still produces the same pass/fail/skip results. Closes #164. * ci(shellcheck): don't re-lint a file's pre-existing issues on a pure rename Renaming test/test-c6b77b77.sh (formerly test-cb1143ab.sh before this branch's sweep, naming aside) and its siblings made git diff treat their new paths as changed content, so shellcheck ran on files it had never checked before and surfaced issues the diff-scoped job was built to avoid (#442). -M lets git diff tell a byte-identical rename (R100) apart from one that actually changed, so only genuinely new or modified content gets linted. Also fixes test-make-under-proot.sh's own pre-existing shellcheck issues (missing shebang, unquoted expansions), since it got a real one-line edit as part of the rename and is no longer a pure R100. Drops .gitremotes, a local scratch file that ended up staged by an earlier `git add -A`. * test: revert cosmetic string-literal edits in three renamed tests These three files got an unnecessary touch-up during the rename sweep, updating an embedded /tmp path template or socket name to match the new filename. mktemp(3)'s XXXXXX suffix already guarantees uniqueness, so the prefix text was cosmetic, not functional - and touching that one line was enough to make SonarCloud treat each file as new code and flag the same /tmp usage pattern this suite has used safely for years. Reverting restores all 122 renamed files to genuine pure renames.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a `shellcheck` job to `.github/workflows/pull-request.yml`, scoped to files changed in each push/PR rather than the whole tree.
A full scan finds 816 pre-existing issues across `test/*.sh`, mostly info/style level, which would block every future PR on unrelated legacy content if the job scanned everything. Follows the same diff-scoping approach proot-rs already uses for its own shellcheck job, extended to also cover `.bash` and `.bats`: proot-rs's own `.sh$` filter misses both, and would have caught neither of the two real bugs (SC2145, SC2128) found in this session's own `test/helper.bash` (from #441, fixed there directly).
One tradeoff worth flagging: shellcheck lints whole files, not just changed lines, so touching any legacy `.sh` file surfaces that file's full existing backlog, not just what changed. Checked the files most likely to get touched soon, since we've been tracking known-flaky tests this session: `test-chroot01.sh` has 2 pre-existing issues, `test-tempdire.sh` has 12, the three `test-socket*.sh` have none. Not blocking anything today, but whoever picks up `test-tempdire.sh` next should expect to see those surface.
Verified locally: the diff-detection logic correctly picks up exactly the files changed in a real branch (tested against #441's diff), correctly no-ops when nothing shell-related changed, and correctly fails on an injected real issue.