fix: handle empty arrays with set -u (nounset) enabled#97
Open
rdasilveiracabral wants to merge 1 commit into
Open
fix: handle empty arrays with set -u (nounset) enabled#97rdasilveiracabral wants to merge 1 commit into
rdasilveiracabral wants to merge 1 commit into
Conversation
Reviewer's GuideThis PR hardens the shell-based CLI and Docker tooling against Sequence diagram for CLI parsing and dispatch with safe empty arrayssequenceDiagram
actor User
participant main_sh as main_sh
participant cli_sh as cli_sh_parse_cli_args
participant docker_sh as docker_sh_run_claudebox_container
User->>main_sh: invoke claudebox [args]
activate main_sh
main_sh->>cli_sh: parse_cli_args original_args saved_flags
activate cli_sh
cli_sh-->>main_sh: CLI_HOST_FLAGS CLI_CONTROL_FLAGS CLI_SCRIPT_COMMAND CLI_PASS_THROUGH
deactivate cli_sh
main_sh->>main_sh: process_host_flags
note over main_sh: process_host_flags returns immediately if CLI_HOST_FLAGS length is 0
alt script command requires no docker
main_sh->>main_sh: dispatch_command CLI_SCRIPT_COMMAND CLI_PASS_THROUGH CLI_CONTROL_FLAGS
note over main_sh: Arrays expanded with ${arr[@]+"${arr[@]}"}
else script command requires docker/image
main_sh->>main_sh: preflight_check CLI_SCRIPT_COMMAND CLI_PASS_THROUGH
note over main_sh: CLI_PASS_THROUGH expanded safely even if empty
main_sh->>docker_sh: run_claudebox_container container_name interactive CLI_CONTROL_FLAGS CLI_PASS_THROUGH
activate docker_sh
docker_sh->>docker_sh: cleanup_mcp_files
note over docker_sh: Loop over mcp_temp_files only if length > 0
docker_sh-->>main_sh: container exit code
deactivate docker_sh
end
main_sh-->>User: exit status (no nounset errors on empty arrays)
deactivate main_sh
File-Level Changes
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 - I've found 4 issues, and left some high level feedback:
- Several of the new
${arr[@]+"${arr[@]}"}expansions are passed unquoted (e.g. inmain()calls toparse_cli_args,dispatch_command,run_claudebox_container), which changes the original quoting/word-splitting semantics and may break args containing spaces or glob characters; consider keeping these expansions inside double quotes and/or guarding calls withif [[ ${#arr[@]} -gt 0 ]]instead. - The empty-array handling patterns (
if [[ ${#arr[@]} -gt 0 ]]plus${arr[@]+"${arr[@]}"}) are now duplicated in multiple places acrossmain.sh,lib/cli.sh,lib/docker.sh; it may be worth introducing small helper functions or a consistent macro-style pattern to reduce repetition and the risk of subtle inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several of the new `${arr[@]+"${arr[@]}"}` expansions are passed unquoted (e.g. in `main()` calls to `parse_cli_args`, `dispatch_command`, `run_claudebox_container`), which changes the original quoting/word-splitting semantics and may break args containing spaces or glob characters; consider keeping these expansions inside double quotes and/or guarding calls with `if [[ ${#arr[@]} -gt 0 ]]` instead.
- The empty-array handling patterns (`if [[ ${#arr[@]} -gt 0 ]]` plus `${arr[@]+"${arr[@]}"}`) are now duplicated in multiple places across `main.sh`, `lib/cli.sh`, `lib/docker.sh`; it may be worth introducing small helper functions or a consistent macro-style pattern to reduce repetition and the risk of subtle inconsistencies.
## Individual Comments
### Comment 1
<location> `main.sh:112` </location>
<code_context>
# Re-parse WITH saved flags, but the command structure is preserved
# because the command was already identified from original args
- parse_cli_args "${original_args[@]}" "${saved_flags[@]}"
+ parse_cli_args ${original_args[@]+"${original_args[@]}"} "${saved_flags[@]}"
process_host_flags
</code_context>
<issue_to_address>
**issue (bug_risk):** New ${arr[@]+"${arr[@]}"} usage is fragile and may not behave as intended for arguments with spaces or set -u
Using `${original_args[@]+"${original_args[@]}"}` unquoted in a command position changes how word splitting and globbing apply after `${param+word}` expansion, which can mis-handle args with spaces, wildcards, or leading dashes and is difficult to reason about.
Given callers should ensure `original_args` is defined (or initialized to an empty array), it’s safer to keep:
```bash
parse_cli_args "${original_args[@]}" "${saved_flags[@]}"
```
If `set -u` safety is needed, prefer initializing `original_args=()` where it’s declared instead of using this pattern at call sites.
</issue_to_address>
### Comment 2
<location> `main.sh:140` </location>
<code_context>
if [[ "$cmd_requirements" == "none" ]]; then
# Dispatch the command directly and exit
- dispatch_command "${CLI_SCRIPT_COMMAND}" "${CLI_PASS_THROUGH[@]}" "${CLI_CONTROL_FLAGS[@]}"
+ dispatch_command "${CLI_SCRIPT_COMMAND}" ${CLI_PASS_THROUGH[@]+"${CLI_PASS_THROUGH[@]}"} ${CLI_CONTROL_FLAGS[@]+"${CLI_CONTROL_FLAGS[@]}"}
exit $?
fi
</code_context>
<issue_to_address>
**issue (bug_risk):** Unquoted array expansions when invoking dispatch_command can corrupt arguments containing spaces or glob characters
The prior quoted form (`"${CLI_PASS_THROUGH[@]}" "${CLI_CONTROL_FLAGS[@]}"`) correctly preserved argument boundaries. The new pattern with unquoted expansions (`${CLI_PASS_THROUGH[@]+"${CLI_PASS_THROUGH[@]}"}` and the similar control flags) reintroduces word splitting and glob expansion:
- Flags/args with spaces are split into multiple arguments.
- `*`, `?`, `[]` may be treated as globs.
Given users can pass arbitrary prompts/flags, this is risky. Please keep these arrays quoted and handle `set -u` by ensuring arrays are always defined or wrapping calls, e.g.:
```bash
dispatch_command "${CLI_SCRIPT_COMMAND}" \
"${CLI_PASS_THROUGH[@]}" \
"${CLI_CONTROL_FLAGS[@]}"
```
Same applies to the `run_claudebox_container` call sites using this pattern.
</issue_to_address>
### Comment 3
<location> `main.sh:461` </location>
<code_context>
# Combine original args with saved flags
- local all_args=("${original_args[@]}" "${saved_flags[@]}")
-
+ local all_args=(${original_args[@]+"${original_args[@]}"} "${saved_flags[@]}")
+
# Re-parse to properly sort flags
</code_context>
<issue_to_address>
**issue (bug_risk):** Unquoted array assignment can mangle elements with spaces or wildcards
`local all_args=(...)` without quoting will perform word splitting and glob expansion on the elements, so any argument containing spaces or metacharacters can be altered. The previous version preserved arguments by keeping expansions quoted.
To keep argument integrity while retaining `set -u` safety, you can append arrays instead of using an unquoted literal:
```bash
local all_args=()
if [[ ${#original_args[@]} -gt 0 ]]; then
all_args+=("${original_args[@]}")
fi
if [[ ${#saved_flags[@]} -gt 0 ]]; then
all_args+=("${saved_flags[@]}")
fi
```
</issue_to_address>
### Comment 4
<location> `lib/cli.sh:62-65` </location>
<code_context>
- export CLI_SCRIPT_COMMAND="$script_command"
- export CLI_PASS_THROUGH=("${pass_through[@]}")
+ # Use ${arr[@]+"${arr[@]}"} pattern to handle empty arrays with set -u
+ CLI_HOST_FLAGS=("${host_flags[@]+"${host_flags[@]}"}")
+ CLI_CONTROL_FLAGS=("${control_flags[@]+"${control_flags[@]}"}")
+ CLI_SCRIPT_COMMAND="$script_command"
+ CLI_PASS_THROUGH=("${pass_through[@]+"${pass_through[@]}"}")
}
</code_context>
<issue_to_address>
**suggestion:** The ${arr[@]+"${arr[@]}"} pattern inside array literals is unusual and hard to reason about
These assignments:
```bash
CLI_HOST_FLAGS=(${host_flags[@]+"${host_flags[@]}"})
CLI_CONTROL_FLAGS=(${control_flags[@]+"${control_flags[@]}"})
CLI_PASS_THROUGH=(${pass_through[@]+"${pass_through[@]}"})
```
are a non-obvious workaround for `set -u`. In this function the local arrays are always initialized (just possibly empty), so you can safely use the simpler form:
```bash
CLI_HOST_FLAGS=("${host_flags[@]}")
CLI_CONTROL_FLAGS=("${control_flags[@]}")
CLI_PASS_THROUGH=("${pass_through[@]}")
```
If you’re worried about them ever being unset, it’s clearer to ensure they’re initialized at declaration (e.g. `local host_flags=()`), rather than relying on the `${arr[@]+…}` pattern in assignments.
Suggested implementation:
```
# Ensure arrays are initialized (even if empty) so ${arr[@]} is safe with set -u
```
```
# Ensure arrays are initialized (even if empty) so ${arr[@]} is safe with set -u
```
```
# Ensure arrays are initialized (even if empty) so ${arr[@]} is safe with set -u
```
```
+ # Export results for use by main script; arrays are always initialized, so ${arr[@]} is safe
+ CLI_HOST_FLAGS=("${host_flags[@]}")
+ CLI_CONTROL_FLAGS=("${control_flags[@]}")
+ CLI_SCRIPT_COMMAND="$script_command"
+ CLI_PASS_THROUGH=("${pass_through[@]}")
}
```
If `host_flags`, `control_flags`, and `pass_through` are not already guaranteed to be initialized as arrays in all code paths, add explicit initialization near their declaration, e.g.:
```bash
local host_flags=()
local control_flags=()
local pass_through=()
```
This keeps the behavior safe under `set -u` while allowing the simpler `"${arr[@]}"` usage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Multiple functions failed with "unbound variable" errors when arrays
were empty and set -u was enabled. This affected:
- MCP temp files cleanup trap in lib/docker.sh
- CLI argument parsing in lib/cli.sh
- Profile listing in lib/commands.profile.sh
- Profile config reading in lib/config.sh
- Various array iterations in main.sh
Fixes applied:
- Wrap array iterations in length checks: if [[ ${#arr[@]} -gt 0 ]]
- Use ${arr[@]+"${arr[@]}"} pattern for safe array expansion
- Initialize global CLI arrays even when no args provided
- Convert echo to printf for portability
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
rdasilveiracabral
force-pushed
the
fix/mcp-cleanup-unbound-variable
branch
from
300906a to
9f4b6de
Compare
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.
Summary
set -uenabled triggers errorslib/docker.sh,lib/cli.sh,lib/commands.profile.sh,lib/config.sh,main.shChanges
if [[ ${#arr[@]} -gt 0 ]]${arr[@]+"${arr[@]}"}pattern for safe array expansionechotoprintffor portabilityTest plan
claudeboxwith no arguments and exit - should not show "unbound variable" errorclaudebox --verboseto verify CLI parsing works with flagsclaudebox profilesto verify profile listing works🤖 Generated with Claude Code
Summary by Sourcery
Handle empty arrays safely across CLI, Docker, and profile handling logic so claudebox works correctly with nounset enabled.
Bug Fixes:
Enhancements: