fix: macOS compatibility and shell robustness fixes - #105
Open
fericsepi wants to merge 1 commit into
Open
Conversation
- replace awk template substitution with pure bash loop in Dockerfile generation to avoid BSD awk issues with multi-line -v values on macOS - guard unbound variables (user_mcp_file, project_mcp_file) with to prevent errors under set -u - rename --verbose to --debug for claudebox debug output so --verbose can pass through to claude CLI
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces awk-based Dockerfile templating with a pure Bash loop for better macOS compatibility, renames the claudebox debug flag from --verbose to --debug so --verbose can pass through to the Claude CLI, and guards optional MCP temp files against unset-variable errors under Sequence diagram for CLI debug flag handling and Claude CLI passthroughsequenceDiagram
actor User
participant ClaudeboxCLI as claudebox_shell
participant CliLib as cli_sh
participant DockerLib as docker_sh
participant ClaudeCLI as claude_cli
User->>ClaudeboxCLI: Run claudebox with flags
ClaudeboxCLI->>CliLib: parse_cli_args argv
CliLib->>CliLib: Identify HOST_ONLY_FLAGS
CliLib->>CliLib: process_host_flags
CliLib-->>ClaudeboxCLI: VERBOSE exported if --debug present
alt debug_enabled
ClaudeboxCLI->>DockerLib: run_claudebox_container with VERBOSE
else no_debug
ClaudeboxCLI->>DockerLib: run_claudebox_container
end
Note over ClaudeboxCLI,ClaudeCLI: --verbose is no longer consumed by claudebox
ClaudeboxCLI->>ClaudeCLI: Invoke claude with passthrough flags including --verbose
ClaudeCLI-->>ClaudeboxCLI: Claude output
ClaudeboxCLI-->>User: Display result
Class diagram for updated shell modules and flags/cleanup behaviorclassDiagram
class main_sh {
+string base_dockerfile
+string final_dockerfile
+string profile_installations
+string labels
+generate_project_dockerfile(base_dockerfile, profile_installations, labels) string
}
class cli_sh {
+string[] HOST_ONLY_FLAGS
+string[] CONTROL_FLAGS
+string[] SCRIPT_COMMANDS
+string[] CLI_HOST_FLAGS
+parse_cli_args(argc, argv)
+process_host_flags()
}
class docker_sh {
+string~user_mcp_file~
+string~project_mcp_file~
+run_claudebox_container()
+cleanup_temp_files()
}
main_sh --> docker_sh : uses
main_sh --> cli_sh : uses
%% Flag behavior details
class HostOnlyFlags {
+--debug
+rebuild
}
cli_sh --> HostOnlyFlags : defines
class DebugFlagHandling {
+bool VERBOSE
+handle_debug_flag(flag)
}
cli_sh --> DebugFlagHandling : exports VERBOSE when flag is --debug
class OptionalMcpFiles {
+string user_mcp_file_optional
+string project_mcp_file_optional
+guard_unset_variables()
}
docker_sh --> OptionalMcpFiles : guards with parameter expansion
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 1 issue, and left some high level feedback:
- In the Dockerfile templating guard,
if grep -q '{{PROFILE_INSTALLATIONS}}' <<<"$final_dockerfile" grep -q '{{LABELS}}' <<<"$final_dockerfile"; thenis missing a logical operator (||/&&) between the twogrepcalls, which will cause a syntax error; add the appropriate connector. - In the new bash-based template substitution, the regex on the right-hand side of
[[ "$line" =~ ... ]]is quoted, which makes Bash treat it as a literal string rather than a regex; remove the surrounding quotes to ensure the character classes and anchors work as intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Dockerfile templating guard, `if grep -q '{{PROFILE_INSTALLATIONS}}' <<<"$final_dockerfile" grep -q '{{LABELS}}' <<<"$final_dockerfile"; then` is missing a logical operator (`||`/`&&`) between the two `grep` calls, which will cause a syntax error; add the appropriate connector.
- In the new bash-based template substitution, the regex on the right-hand side of `[[ "$line" =~ ... ]]` is quoted, which makes Bash treat it as a literal string rather than a regex; remove the surrounding quotes to ensure the character classes and anchors work as intended.
## Individual Comments
### Comment 1
<location path="main.sh" line_range="589-599" />
<code_context>
+ # Uses pure bash to avoid BSD awk issues with multi-line -v values on macOS
+ local final_dockerfile=""
+ while IFS= read -r line; do
+ if [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}[[:space:]]*$ ]]; then
+ final_dockerfile+="$profile_installations"$'\n'
+ elif [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*LABELS[[:space:]]*\}\}[[:space:]]*$ ]]; then
+ final_dockerfile+="$labels"$'\n'
</code_context>
<issue_to_address>
**suggestion:** Double-check newline behavior when injecting multi-line `profile_installations`/`labels` blocks
Because `final_dockerfile+="$profile_installations"$'\n'` always appends a newline, any multi-line value that already ends with `\n` will now get an extra blank line. Previously, `awk` emitted `pi` unchanged, so this is a behavior change. If that extra blank line can affect Dockerfile parsing (e.g., around `RUN` or label blocks), consider trimming a trailing newline before appending (e.g. `final_dockerfile+="${profile_installations%$'\n'}"$'\n'`) or constructing the output via `printf` to match the old semantics.
```suggestion
# Uses pure bash to avoid BSD awk issues with multi-line -v values on macOS
local final_dockerfile=""
while IFS= read -r line; do
if [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}[[:space:]]*$ ]]; then
# Trim a single trailing newline from profile_installations to avoid adding an extra blank line
final_dockerfile+="${profile_installations%$'\n'}"$'\n'
elif [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*LABELS[[:space:]]*\}\}[[:space:]]*$ ]]; then
# Trim a single trailing newline from labels to avoid adding an extra blank line
final_dockerfile+="${labels%$'\n'}"$'\n'
else
final_dockerfile+="$line"$'\n'
fi
done <<<"$base_dockerfile"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+589
to
+599
| # Uses pure bash to avoid BSD awk issues with multi-line -v values on macOS | ||
| local final_dockerfile="" | ||
| while IFS= read -r line; do | ||
| if [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}[[:space:]]*$ ]]; then | ||
| final_dockerfile+="$profile_installations"$'\n' | ||
| elif [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*LABELS[[:space:]]*\}\}[[:space:]]*$ ]]; then | ||
| final_dockerfile+="$labels"$'\n' | ||
| else | ||
| final_dockerfile+="$line"$'\n' | ||
| fi | ||
| done <<<"$base_dockerfile" |
There was a problem hiding this comment.
suggestion: Double-check newline behavior when injecting multi-line profile_installations/labels blocks
Because final_dockerfile+="$profile_installations"$'\n' always appends a newline, any multi-line value that already ends with \n will now get an extra blank line. Previously, awk emitted pi unchanged, so this is a behavior change. If that extra blank line can affect Dockerfile parsing (e.g., around RUN or label blocks), consider trimming a trailing newline before appending (e.g. final_dockerfile+="${profile_installations%$'\n'}"$'\n') or constructing the output via printf to match the old semantics.
Suggested change
| # Uses pure bash to avoid BSD awk issues with multi-line -v values on macOS | |
| local final_dockerfile="" | |
| while IFS= read -r line; do | |
| if [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}[[:space:]]*$ ]]; then | |
| final_dockerfile+="$profile_installations"$'\n' | |
| elif [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*LABELS[[:space:]]*\}\}[[:space:]]*$ ]]; then | |
| final_dockerfile+="$labels"$'\n' | |
| else | |
| final_dockerfile+="$line"$'\n' | |
| fi | |
| done <<<"$base_dockerfile" | |
| # Uses pure bash to avoid BSD awk issues with multi-line -v values on macOS | |
| local final_dockerfile="" | |
| while IFS= read -r line; do | |
| if [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}[[:space:]]*$ ]]; then | |
| # Trim a single trailing newline from profile_installations to avoid adding an extra blank line | |
| final_dockerfile+="${profile_installations%$'\n'}"$'\n' | |
| elif [[ "$line" =~ ^[[:space:]]*\{\{[[:space:]]*LABELS[[:space:]]*\}\}[[:space:]]*$ ]]; then | |
| # Trim a single trailing newline from labels to avoid adding an extra blank line | |
| final_dockerfile+="${labels%$'\n'}"$'\n' | |
| else | |
| final_dockerfile+="$line"$'\n' | |
| fi | |
| done <<<"$base_dockerfile" |
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.
featuring:
--verboseto--debugfor claudebox debug output so--verbosecan pass through to claude CLISummary by Sourcery
Improve macOS compatibility and shell robustness in Dockerfile generation, CLI flags, and container cleanup.
Bug Fixes:
set -uduring container cleanup.Enhancements:
--verboseto--debugso--verbosecan pass through to the underlying Claude CLI.