fix: Bash 3.2 compatibility with set -u flag#89
Open
TonyHernandezAtMS wants to merge 3 commits into
Open
Conversation
Reviewer's GuideThis PR retrofits the codebase for Bash 3.2 compatibility under Sequence diagram for safe array expansion in CLI argument parsingsequenceDiagram
participant User
participant "main.sh"
participant "lib/cli.sh"
User->>"main.sh": Run ClaudeBox command
"main.sh"->>"lib/cli.sh": parse_cli_args "$@"
"lib/cli.sh"->>"lib/cli.sh": Iterate over "$@" directly (Bash 3.2 safe)
"lib/cli.sh"->>"lib/cli.sh": Guard array exports with "${array[@]+...}"
"lib/cli.sh"->>"main.sh": Export CLI_HOST_FLAGS, CLI_CONTROL_FLAGS, CLI_PASS_THROUGH
"main.sh"->>"main.sh": Use guarded array expansions for dispatch_command
"main.sh"->>User: Command executes without unbound variable errors
Class diagram for updated CLI argument buckets and array handlingclassDiagram
class CLIParser {
+parse_cli_args(args)
+process_host_flags()
+debug_parsed_args()
-host_flags[]
-control_flags[]
-script_command
-pass_through[]
}
CLIParser : host_flags[] guarded with ${host_flags[@]+...}
CLIParser : control_flags[] guarded with ${control_flags[@]+...}
CLIParser : pass_through[] guarded with ${pass_through[@]+...}
CLIParser : parse_cli_args(args) iterates over "$@" directly
CLIParser : process_host_flags() checks array length before loop
Class diagram for updated profile management functions in config.shclassDiagram
class Config {
+read_profile_section(profile_file, section)
+update_profile_section(profile_file, section, new_items[])
+get_current_profiles(profiles_file)
-existing_items[]
-all_items[]
-current_profiles[]
}
Config : read_profile_section() prints guarded array
Config : update_profile_section() replaces readarray with while read loop
Config : update_profile_section() guards all_items[] and new_items[]
Config : get_current_profiles() prints guarded array
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/commands.profile.sh:35` </location>
<code_context>
- fi
- done
+ # Check if profile is currently enabled (guard for empty array)
+ if [ ${#current_profiles[@]} -gt 0 ]; then
+ for enabled in "${current_profiles[@]}"; do
+ if [[ "$enabled" == "$profile" ]]; then
+ is_enabled=true
</code_context>
<issue_to_address>
**suggestion:** Guarding array iteration with length check improves robustness.
Please apply this array length guard consistently in all relevant parts of the codebase for Bash compatibility and to avoid errors with set -u.
Suggested implementation:
```
if [ ${#current_profiles[@]} -gt 0 ]; then
for enabled in "${current_profiles[@]}"; do
if [[ "$enabled" == "$profile" ]]; then
is_enabled=true
break
fi
done
fi
```
```
if [ ${#all_profiles[@]} -gt 0 ]; then
for p in "${all_profiles[@]}"; do
# do something
done
fi
```
```
if [ ${#some_array[@]} -gt 0 ]; then
for item in "${some_array[@]}"; do
# process item
done
fi
```
- You should apply this pattern to every array iteration in the file, including any loops over arrays such as `for x in "${array[@]}"`.
- If you have other arrays (e.g., `disabled_profiles`, `profile_list`, etc.), ensure each loop is guarded with a length check.
- If you use associative arrays, adapt the guard accordingly (e.g., `${#assoc_array[@]}`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
Thanks @sourcery-ai! I've verified that all array iterations in |
|
No operation ID found for this PR |
- Replace readarray with while read loops (Bash 3.2 doesn't have readarray)
- Add safe array expansion patterns: "${array[@]+"${array[@]}"} "
- Add array length checks before iteration to prevent unbound variable errors
- Remove problematic 'local' declarations in subshell contexts
- Use temp files for awk multiline string handling instead of string substitution
- Fix cleanup_mcp_files trap to safely check array existence
- All changes ensure compatibility with both macOS (Bash 3.2) and Linux (Bash 4+)
TonyHernandezAtMS
force-pushed
the
main
branch
from
32c5e21 to
702118a
Compare
Author
|
@sourcery-ai review |
|
No operation ID found for this PR |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There’s a lot of repeated
if [ ${#array[@]} -gt 0 ]guards—consider initializing your arrays up front (e.g.declare -a host_flags pass_through) or wrapping that pattern in a small helper to reduce boilerplate. - In the Dockerfile templating block, you delete temp files only on success or awk failure—consider adding a trap to clean up those mktemp files on any exit or signal to avoid leftovers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of repeated `if [ ${#array[@]} -gt 0 ]` guards—consider initializing your arrays up front (e.g. `declare -a host_flags pass_through`) or wrapping that pattern in a small helper to reduce boilerplate.
- In the Dockerfile templating block, you delete temp files only on success or awk failure—consider adding a trap to clean up those mktemp files on any exit or signal to avoid leftovers.
## Individual Comments
### Comment 1
<location> `main.sh:603-607` </location>
<code_context>
+ rm -f "$temp_pi" "$temp_lbs"
# Guard: ensure no unreplaced placeholders remain
- if grep -q '{{PROFILE_INSTALLATIONS}}' <<<"$final_dockerfile" grep -q '{{LABELS}}' <<<"$final_dockerfile"; then
- error "Unreplaced placeholders remain in generated Dockerfile"
+ if grep -q '{{PROFILE_INSTALLATIONS}}' <<<"$final_dockerfile" || grep -q '{{LABELS}}' <<<"$final_dockerfile"; then
+ error "Unreplaced placeholders remain in generated Dockerfile"
fi
</code_context>
<issue_to_address>
**issue (bug_risk):** Corrected logical operator in placeholder check.
The previous implementation lacked a logical operator, which could have caused the check to fail silently. This fix ensures both placeholders are properly validated.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Addresses sourcery-ai feedback to ensure temp files are always cleaned up, even on errors or signals during Dockerfile template substitution.
Author
|
Thanks @sourcery-ai for the excellent feedback! I've addressed your suggestions: ✅ Added trap for temp file cleanup (commit 2cf9c3f)
Regarding the repeated array guards:
The
This PR prioritizes explicitness and safety over conciseness, which aligns with ClaudeBox's philosophy of "fail fast, fail loud" and maintainable code for a project with 1000+ users. Thanks again for the thorough review! 🙏 |
|
No operation ID found for this PR |
Found and fixed three instances where current_profiles array was iterated
without guards, which would cause 'unbound variable' errors with set -u
when profiles.ini exists but contains no profiles.
Fixes:
- Line 356-369: Profile rebuild check loop
- Line 545-556: Profile installations generation loop
- Line 564-576: Docker-affecting profiles hash calculation loop
All loops now guarded with: if [ ${#current_profiles[@]} -gt 0 ]
Author
|
Follow-up: Found and fixed 3 additional instances (commit d0eb56c) During final review, discovered three more places in
All now properly guarded with This finding validates our engineering approach:
The PR now has comprehensive Bash 3.2 + |
|
No operation ID found for this PR |
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.
Description
Fixes "unbound variable" errors on macOS when running ClaudeBox with default Bash 3.2.
Fixes #71
Problem
ClaudeBox fails on macOS with errors like:
This occurs because:
set -uflag (treats unset variables as errors)Current workaround is
brew install bash, but this shouldn't be required.Root Cause
When
set -uis enabled and arrays are empty, Bash 3.2 treats${array[@]}as an unbound variable error. This affects:lib/cli.sh)When This Broke
This issue was introduced in commit 1e621cc (July 12, 2025) during the "CLI architecture overhaul" which:
set -euo pipefailflags to the main scriptset -uIssue #71 was reported 2 months later (September 9, 2025) when macOS users started encountering the errors.
Solution
Applied Bash 3.2 +
set -ucompatible patterns throughout the codebase:Safe Array Expansion
Safe Array Iteration
Safe Parameter Expansion in Traps
Files Changed
lib/cli.sh- Fixed argument parsing arrayslib/commands.core.sh- Safe array expansionslib/commands.profile.sh- Guards for empty profile arrayslib/config.sh- Replacedreadarray(Bash 4+) withwhile readloopslib/docker.sh- Safe cleanup trap array handlingmain.sh- Temp files for awk instead of complex string substitutionsbuild/docker-entrypoint- Removed problematiclocalin subshellsTesting
Tested on:
Verified fixes:
./claudebox.runinstalls without "unbound variable" errorsclaudebox add pythonworksclaudebox shellworksImpact
Comparison with PR #77
This PR takes a similar approach to #77 but focuses on:
readarray)Related Issues
Summary by Sourcery
Enable Bash 3.2 compatibility with set -u by guarding empty array expansions, replacing Bash-4+ features, and refactoring array and Dockerfile handling across all scripts
Bug Fixes:
Enhancements: