fix(shell): declare the shell — shebang where executed, directive where sourced - #335
Conversation
…re sourced
shellcheck SC2148 ('target shell is unknown') on 13 files here.
Deliberately NOT a blanket shebang. The files split by how they are USED:
* 0 executed (have +x, never source'd) -> added '#!/usr/bin/env bash'
* 13 sourced (referenced by source/.) -> added '# shellcheck shell=bash'
A source'd file is never run as its own process, so a shebang there is
misleading — it claims an execution model the file does not have. The
shellcheck directive states the dialect without making that false claim.
Found by an estate-wide sweep of 5,111 tracked scripts across 375 repos: 85
files lack a shell declaration. 40 of those were left alone deliberately —
they are vendored duplicates (7 identical copies of one recoverer.sh) or a
nested kith/ tree, and 37 of the 40 are never invoked by name anywhere, so
editing them would be churn in vendored code.
Remaining SC2148 in this repo after the change: 0
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🔇 Additional comments (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds ChangesBash ShellCheck declarations
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change only adds explicit Bash declarations to sourced shell files, with no actionable merge-blocking risk remaining after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 13 files. ✨ Finishing Touches📝 Generate docstrings
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 |
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR resolves ShellCheck SC2148 ('target shell is unknown') across 13 shell configuration files by implementing the # shellcheck shell=bash directive for sourced modules. Codacy analysis indicates the changes are up to standards with zero new quality issues.
While the implementation correctly distinguishes between executed and sourced scripts, there is a minor documentation conflict in baseline.sh regarding POSIX compatibility. Furthermore, the PR does not include CI configuration changes to verify that these new directives are being enforced or that the reported issues are fully resolved. You should also ensure that the 40 files 'left alone' do not impact local development workflows.
About this PR
- The PR lacks automated tests or CI configuration changes to verify that ShellCheck is running and passing with these changes. Without this, it is difficult to confirm the resolution of SC2148 across the environment.
Test suggestions
- Verify that all 13 modified files pass ShellCheck without SC2148 errors.
- Ensure that the added directive correctly identifies bash-specific syntax (e.g., shopt, BASH_SOURCE) during linting.
- Verify that no execution permissions (+x) are present or added to files using the shellcheck directive instead of a shebang.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that all 13 modified files pass ShellCheck without SC2148 errors.
2. Ensure that the added directive correctly identifies bash-specific syntax (e.g., shopt, BASH_SOURCE) during linting.
3. Verify that no execution permissions (+x) are present or added to files using the shellcheck directive instead of a shebang.
Low confidence findings
- The description mentions 40 files were 'left alone' (vendored or inactive), but these are not identified. This makes it difficult to audit if any of those files require shell declarations for local development tools.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| @@ -1,3 +1,4 @@ | |||
| # shellcheck shell=bash | |||
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The documentation on line 7 describes the script as POSIX-compatible, but the addition of the bash directive and the use of ${BASH_SOURCE[0]} on line 19 make it Bash-specific. Consider updating the description to clarify that this is a Bash configuration to avoid confusion for users of shells like dash or ash.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
shellcheck
SC2148(target shell is unknown) on 13 files here.Deliberately not a blanket shebang. The files split by how they are used:
+x, neversourced)#!/usr/bin/env bashsource/.)# shellcheck shell=bashA
sourced file is never run as its own process, so a shebang there is misleading — it claims an execution model the file does not have. The shellcheck directive states the dialect without making that false claim.Found by an estate-wide sweep of 5,111 tracked scripts across 375 repos: 85 files lack a shell declaration. 40 were left alone deliberately — vendored duplicates (7 identical copies of one
recoverer.sh) or a nestedkith/tree, and 37 of the 40 are never invoked by name anywhere, so editing them would be churn in vendored code.Remaining
SC2148in this repo after the change: 0