Skip to content

fix(shell): declare the shell — shebang where executed, directive where sourced - #193

Closed
hyperpolymath wants to merge 1 commit into
mainfrom
fix/shellcheck-shebang-and-shell-directive
Closed

fix(shell): declare the shell — shebang where executed, directive where sourced#193
hyperpolymath wants to merge 1 commit into
mainfrom
fix/shellcheck-shebang-and-shell-directive

Conversation

@hyperpolymath

Copy link
Copy Markdown
Owner

shellcheck SC2148 (target shell is unknown) on 12 files here.

Deliberately not a blanket shebang. The files split by how they are used:

class count fix
executed (has +x, never sourced) 12 #!/usr/bin/env bash
sourced (referenced by source/.) 0 # shellcheck shell=bash

A 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 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: 22

…re sourced

shellcheck SC2148 ('target shell is unknown') on 12 files here.

Deliberately NOT a blanket shebang. The files split by how they are USED:

  * 12 executed (have +x, never source'd)  -> added '#!/usr/bin/env bash'
  * 0 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: 22
@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 51 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6b293dbf-6e4b-4005-89e9-b90443add2dc

📥 Commits

Reviewing files that changed from the base of the PR and between 1642050 and 32e2b8f.

📒 Files selected for processing (12)
  • rescript-ecosystem/packages/core/compiler-source/tests/analysis_tests/tests-generic-jsx-transform/test.sh
  • rescript-ecosystem/packages/core/compiler-source/tests/analysis_tests/tests-incremental-typechecking/test.sh
  • rescript-ecosystem/packages/core/compiler-source/tests/analysis_tests/tests-reanalyze/deadcode/test.sh
  • rescript-ecosystem/packages/core/compiler-source/tests/analysis_tests/tests-reanalyze/termination/test.sh
  • rescript-ecosystem/packages/core/compiler-source/tests/analysis_tests/tests/test.sh
  • rescript-ecosystem/packages/core/compiler-source/tests/tools_tests/test.sh
  • rescript-ecosystem/rescript/tests/analysis_tests/tests-generic-jsx-transform/test.sh
  • rescript-ecosystem/rescript/tests/analysis_tests/tests-incremental-typechecking/test.sh
  • rescript-ecosystem/rescript/tests/analysis_tests/tests-reanalyze/deadcode/test.sh
  • rescript-ecosystem/rescript/tests/analysis_tests/tests-reanalyze/termination/test.sh
  • rescript-ecosystem/rescript/tests/analysis_tests/tests/test.sh
  • rescript-ecosystem/rescript/tests/tools_tests/test.sh

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@codacy-production

Copy link
Copy Markdown
Contributor

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request successfully addresses ShellCheck SC2148 by adding #!/usr/bin/env bash shebangs to 12 test scripts across the rescript-ecosystem.

Codacy analysis indicates that the changes are up to standards with no new quality issues or complexity regressions introduced. There are no critical security flaws or logic bugs identified that would prevent merging.

Test suggestions

  • Verify that all 12 modified scripts pass ShellCheck SC2148 validation
  • Ensure scripts remain functional when invoked directly as bash scripts
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that all 12 modified scripts pass ShellCheck SC2148 validation
2. Ensure scripts remain functional when invoked directly as bash scripts

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

@hyperpolymath

Copy link
Copy Markdown
Owner Author

Closing: every one of the 12 files in this PR is vendored third-party code, not ours to patch.

They are the ReScript compiler's own test fixtures, carried in two separate vendored copies:

  • rescript-ecosystem/packages/core/compiler-source/tests/…
  • rescript-ecosystem/rescript/tests/…

Adding a shebang to upstream test fixtures creates permanent divergence from ReScript upstream and will conflict on every future vendor sync, for a lint warning that does not affect us.

The estate-wide sweep that produced this classified them as EXECUTED because they carry +x — which is true, but the relevant question is whose code is it, not is it executable. My mistake in the filter; the vendored-code exclusion I applied to 40 other files should have caught these too.

The genuine first-party shebang fixes from the same sweep stand: ambientops#335, boj-server#319, coord-tui#49.

@hyperpolymath
hyperpolymath deleted the fix/shellcheck-shebang-and-shell-directive branch August 26, 2026 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant