Skip to content

Use bash shebang in generated scripts - #44836

Merged
allenhouchins merged 1 commit into
mainfrom
allenhouchins-bash-fmas
May 6, 2026
Merged

Use bash shebang in generated scripts#44836
allenhouchins merged 1 commit into
mainfrom
allenhouchins-bash-fmas

Conversation

@allenhouchins

@allenhouchins allenhouchins commented May 6, 2026

Copy link
Copy Markdown
Member

Summary
Switches the maintained-apps homebrew script generator from #!/bin/sh to #!/bin/bash so the declared interpreter matches what the scripts actually use.

Why
The generated install/uninstall scripts use bash-only constructs throughout — [[ ... ]], (( ... )), local, eval, indexed/associative arrays, ${var:1} substring expansion, $EUID, SECONDS — but declared #!/bin/sh. They've worked in production because macOS's /bin/sh is bash in POSIX mode, but the shebang was technically inaccurate.

These functions are direct ports of Homebrew's own implementation (referenced in scripts.go), and Homebrew itself uses bash. Declaring #!/bin/bash makes the generated scripts match the source they were ported from and removes a portability lie.

This change is safe because:

  • Fleet's shebang validator already accepts /bin/bash (server/fleet/scripts.go:488)
  • Orbit direct-executes scripts with recognized shebangs (orbit/pkg/scripts/exec_nonwindows.go:31), so the kernel honors #!/bin/bash
  • These scripts only run on macOS, which always ships /bin/bash

Summary by CodeRabbit

  • Bug Fixes
    • Updated installer script shell configuration to improve compatibility and reliability across different system environments.

Change the shebang written by scriptBuilder.String() from "#!/bin/sh" to "#!/bin/bash" so generated scripts explicitly use bash instead of sh. This ensures the scripts run under bash semantics.
@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.69%. Comparing base (3aac728) to head (f43bf41).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #44836   +/-   ##
=======================================
  Coverage   66.68%   66.69%           
=======================================
  Files        2664     2664           
  Lines      214781   214855   +74     
  Branches     9882     9882           
=======================================
+ Hits       143224   143287   +63     
- Misses      58521    58526    +5     
- Partials    13036    13042    +6     
Flag Coverage Δ
backend 68.56% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@allenhouchins
allenhouchins marked this pull request as ready for review May 6, 2026 15:20
@allenhouchins
allenhouchins requested a review from a team as a code owner May 6, 2026 15:20
Copilot AI review requested due to automatic review settings May 6, 2026 15:20

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The shebang line in the String() method of the script builder was updated from #!/bin/sh to #!/bin/bash in the Homebrew ingester scripts module. This changes the interpreter used when generating installer scripts from the POSIX shell to Bash. The modification affects only the script header declaration with no changes to the script content or logic itself.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description provides a clear summary, well-articulated rationale, and addresses safety considerations, but it does not follow the repository's PR description template structure or include required checklist items. Update the description to follow the required template format, including the related issue field and applicable checklist items (e.g., changes file, testing, or GitOps sections as relevant).
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Use bash shebang in generated scripts' clearly and concisely summarizes the main change in the pull request, which is updating the shebang from #!/bin/sh to #!/bin/bash.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch allenhouchins-bash-fmas

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 and usage tips.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
ee/maintained-apps/ingesters/homebrew/scripts.go (1)

575-619: ⚡ Quick win

Optional: replace eval with printf -v for safer variable indirection.

var_name is constructed via tr '.-' '__', which only replaces . and -. If a bundle ID ever contains characters like $ or backticks, the eval "export $var_name=..." / eval "was_running=\$$var_name" calls could have unintended effects. Since the script now explicitly targets Bash, printf -v or declare are idiomatic, eval-free alternatives:

♻️ Safer variable indirection using printf -v
-  eval "export $var_name=0"
+  printf -v "$var_name" '%s' '0'; export "$var_name"
-  eval "export $var_name=1"
+  printf -v "$var_name" '%s' '1'; export "$var_name"
-  eval "was_running=\$$var_name"
+  was_running="${!var_name}"

The ${!var_name} indirect expansion is a Bash built-in and avoids eval entirely.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ee/maintained-apps/ingesters/homebrew/scripts.go` around lines 575 - 619, The
quit_and_track_application() function uses eval "export $var_name=..." and eval
"was_running=\$$var_name" which is unsafe; replace these eval uses with
Bash-safe indirection: when setting the flag, use declare -x "$var_name"=0 or
declare -x "$var_name"=1 (or printf -v to assign then export) instead of eval,
and when reading the flag use the indirect expansion ${!var_name} (e.g.,
was_running="${!var_name}") rather than eval. Update all occurrences in
quit_and_track_application() that set or read var_name accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Around line 575-619: The quit_and_track_application() function uses eval
"export $var_name=..." and eval "was_running=\$$var_name" which is unsafe;
replace these eval uses with Bash-safe indirection: when setting the flag, use
declare -x "$var_name"=0 or declare -x "$var_name"=1 (or printf -v to assign
then export) instead of eval, and when reading the flag use the indirect
expansion ${!var_name} (e.g., was_running="${!var_name}") rather than eval.
Update all occurrences in quit_and_track_application() that set or read var_name
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9eff748d-73db-43f4-a9a1-a62013f2d702

📥 Commits

Reviewing files that changed from the base of the PR and between 55d63e9 and f43bf41.

📒 Files selected for processing (1)
  • ee/maintained-apps/ingesters/homebrew/scripts.go

@allenhouchins
allenhouchins merged commit a5baf36 into main May 6, 2026
57 checks passed
@allenhouchins
allenhouchins deleted the allenhouchins-bash-fmas branch May 6, 2026 15:53
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.

3 participants