fix: codespell typo fixes, replace unsafe array assignments, quote variable expansions to prevent globbing
#980
Conversation
Improve shell script robustness by consistently quoting variable expansions and using array-safe patterns. Replace unquoted variables with quoted ones, use mapfile for array assignments from command output, and update command invocations to prevent word splitting and globbing issues. These changes enhance script reliability and maintainability.
📝 WalkthroughWalkthroughThis PR corrects multiple typos in documentation and shell script comments, improves shell script robustness through consistent variable quoting and array handling using mapfile, and fixes a control-flow bug where a return statement was misspelled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
rootfs/usr/local/bin/codefresh-pipeline (1)
93-99: Return-path bug fixed; consider quotingpipelineindirnamecallThe change from
reutrn 5toreturn 5correctly restores the intended non‑zero exit when project/pipeline cannot be parsed. The new behavior looks good and matches the comment.While you’re here, the
dirnamecall would be a bit safer with quoting in case pipeline names ever contain special characters:- local project=$(dirname $pipeline) + local project + project=$(dirname "$pipeline")rootfs/etc/init.d/atlantis.sh (2)
4-8: Install check is clear; consider suppressingwhichstderr
if ! which atlantis >/dev/null; thenis functionally correct and simpler than many alternatives. If you want to avoid an extrawhich: no atlantis in ...message when Atlantis is missing, you could redirect stderr as well or prefercommand -v:if ! command -v atlantis >/dev/null 2>&1; then ... fi
44-45: Dynamicsource <(... )is appropriate; optionally silence SC1090The
source <(chamber exec ...)andsource <(gosu "${ATLANTIS_USER}" ssh-agent -s)patterns are valid bash usage for importing dynamic environments and are consistent with the script’s shebang.If you’re aiming for clean ShellCheck runs, you can locally disable SC1090 above these lines:
# shellcheck disable=SC1090 source <(chamber exec "${ATLANTIS_CHAMBER_SERVICE}" -- sh -c "export -p") # shellcheck disable=SC1090 source <(gosu "${ATLANTIS_USER}" ssh-agent -s)Also applies to: 71-72
rootfs/templates/wrapper-body.sh (2)
335-356: Mapfile-based filtering ofshell_pidsis an improvement; consider also mapping the initial assignmentSwitching to:
mapfile -t shell_pids < <(printf "%s\n" "${shell_pids[@]}" | grep -v "^$our_shell_pid\$")avoids word splitting when filtering out our own PID and keeps the subsequent logic intact.
For full consistency and to eliminate the remaining command‑substitution array assignment, you could also populate
shell_pidsviamapfile:- local shell_pids=($(_running_shell_pids)) + local shell_pids + mapfile -t shell_pids < <(_running_shell_pids || true)
656-673: Improved stop logic and quoting; refineRUNNING_NAMESemptiness checkThe quoting and
docker pschecks in_polite_stopandstopimprove robustness (no more ambiguoustestarguments when output is empty, and safer handling ofname), andmapfile -t RUNNING_NAMEScorrectly captures container names line‑by‑line.To align with array semantics and address ShellCheck SC2128, you can tighten the “no containers found” check to look at the array length instead of the first element:
- if [ -z "$RUNNING_NAMES" ]; then + if [ "${#RUNNING_NAMES[@]}" -eq 0 ]; then echo "# No running containers found for ${DOCKER_NAME}" return fiAlso applies to: 683-695
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
README.md(1 hunks)README.yaml(1 hunks)os/debian/Dockerfile.debian(1 hunks)rootfs/etc/codefresh/require_vars(2 hunks)rootfs/etc/init.d/atlantis.sh(5 hunks)rootfs/etc/profile.d/_40-preferences.sh(1 hunks)rootfs/etc/profile.d/aws.sh(5 hunks)rootfs/etc/profile.d/fzf.sh(1 hunks)rootfs/etc/profile.d/prompt.sh(1 hunks)rootfs/etc/profile.d/Ω_overrides.sh(1 hunks)rootfs/templates/wrapper-body.sh(5 hunks)rootfs/usr/local/bin/codefresh-pipeline(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 961
File: README.yaml:201-211
Timestamp: 2025-01-20T04:30:32.198Z
Learning: Makefile customizations for Geodesic should follow security best practices including lowercase image names with hyphens, pinned versions, and proper access controls for Docker registries.
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 961
File: rootfs/templates/wrapper-body.sh:544-570
Timestamp: 2025-01-20T08:30:16.434Z
Learning: Shell scripts in the Geodesic project must be POSIX compliant and avoid Bash-specific features like `readarray`.
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 970
File: rootfs/templates/wrapper-body.sh:595-595
Timestamp: 2025-02-20T06:20:41.753Z
Learning: In the Geodesic wrapper script's ONE_SHELL branch, word splitting with `$*` in the Docker run command is intentional and should not be changed to `"$@"`.
📚 Learning: 2025-01-01T00:32:22.159Z
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 961
File: rootfs/etc/profile.d/_01-launch-warning.sh:1-8
Timestamp: 2025-01-01T00:32:22.159Z
Learning: All the files in the `profile.d` directory ending in `.sh` are meant to be sourced, not executed, so we do not need to add a shebang directive.
Applied to files:
rootfs/etc/profile.d/prompt.shrootfs/etc/profile.d/Ω_overrides.sh
📚 Learning: 2025-01-20T04:30:32.198Z
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 961
File: README.yaml:201-211
Timestamp: 2025-01-20T04:30:32.198Z
Learning: Makefile customizations for Geodesic should follow security best practices including lowercase image names with hyphens, pinned versions, and proper access controls for Docker registries.
Applied to files:
os/debian/Dockerfile.debian
📚 Learning: 2025-02-20T06:20:41.753Z
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 970
File: rootfs/templates/wrapper-body.sh:595-595
Timestamp: 2025-02-20T06:20:41.753Z
Learning: In the Geodesic wrapper script's ONE_SHELL branch, word splitting with `$*` in the Docker run command is intentional and should not be changed to `"$@"`.
Applied to files:
rootfs/etc/profile.d/aws.shrootfs/templates/wrapper-body.sh
📚 Learning: 2024-12-31T04:41:45.788Z
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 961
File: rootfs/templates/wrapper-body.sh:1-1
Timestamp: 2024-12-31T04:41:45.788Z
Learning: The file `rootfs/templates/wrapper-body.sh` is a partial template fragment that merges with a shebang from an earlier template, so no separate shebang is required here. Also, `homedir_default_mounts` must remain a string in order to be properly exported to a subshell.
Applied to files:
rootfs/templates/wrapper-body.sh
📚 Learning: 2025-02-20T06:20:15.610Z
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 970
File: rootfs/templates/wrapper-body.sh:603-607
Timestamp: 2025-02-20T06:20:15.610Z
Learning: In the Geodesic wrapper script's Docker exec command, using unquoted `$*` for argument expansion is intentional to allow word splitting, rather than using `"$@"` which would preserve argument boundaries.
Applied to files:
rootfs/templates/wrapper-body.sh
📚 Learning: 2025-01-28T06:17:36.167Z
Learnt from: Nuru
Repo: cloudposse/geodesic PR: 961
File: rootfs/templates/wrapper-body.sh:304-304
Timestamp: 2025-01-28T06:17:36.167Z
Learning: In the Geodesic codebase, the use of `$*` instead of `"$@"` in Docker commands is intentional, particularly in wrapper scripts that pass arguments to `docker exec` and `docker run` commands.
Applied to files:
rootfs/templates/wrapper-body.sh
🪛 Shellcheck (0.11.0)
rootfs/etc/init.d/atlantis.sh
[warning] 44-44: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[warning] 71-71: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
rootfs/templates/wrapper-body.sh
[warning] 688-688: Expanding an array without an index only gives the first element.
(SC2128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (16)
rootfs/etc/profile.d/_40-preferences.sh (1)
20-20: Typo fix approved.Minor documentation improvement; no functional changes.
README.md (1)
235-235: Typo fix approved.Documentation improvement; no functional impact.
rootfs/etc/profile.d/prompt.sh (1)
122-122: Typo fix approved.Comment spelling correction; no behavioral impact.
rootfs/etc/profile.d/fzf.sh (1)
37-37: Typo fix approved.Comment spelling correction; no functional impact.
README.yaml (1)
233-233: Typo fix approved.Documentation improvement; mirrors correction in README.md.
rootfs/etc/profile.d/Ω_overrides.sh (1)
6-6: Typo fix approved.Comment spelling correction; no functional impact.
os/debian/Dockerfile.debian (1)
286-286: Typo fix approved.Comment spelling correction; no functional impact.
rootfs/etc/codefresh/require_vars (3)
29-29: Typo fix approved.Comment spelling correction; no functional impact.
53-53: Typo fix approved.Comment spelling correction; no functional impact.
1-1: Verify comprehensiveness of shell script robustness improvements.The PR objectives and AI summary mention significant changes—array handling with
mapfile, consistent variable quoting, and a control-flow bug fix (misspelled return statement)—but the provided code shows only typo corrections. These more substantial improvements are not visible in the files reviewed.Additionally, per retrieved learnings, the Geodesic project requires POSIX compliance and should avoid Bash-specific features. The use of
mapfile(mentioned in the summary) is Bash-specific, which may conflict with POSIX requirements noted in PR 961.Please confirm:
- Are array handling improvements with
mapfilepresent in unprovided files?- Are variable quoting improvements present throughout the changed files?
- Where is the control-flow bug fix (misspelled return)?
- Have POSIX compliance implications of
mapfilebeen reviewed?rootfs/etc/profile.d/aws.sh (4)
6-8: Saferaws-regioninvocation with quoted argumentsQuoting
AWS_REGION_ABBREVIATION_TYPEandAWS_DEFAULT_REGIONin theaws-regioncall makes the export ofAWS_DEFAULT_SHORT_REGIONmore robust against odd values while preserving existing behavior.
82-108: Using"$@"inaws_sdk_assume_rolepreserves argument boundariesThe switch from
$*to"$@"when invoking the role‑scoped command:AWS_PROFILE="$role" "$@"correctly preserves arguments containing spaces and avoids accidental word splitting, with no downside for simple commands.
140-178:mapfileusage avoids word-splitting issues for profile/role namesThe new
mapfile -tpopulation ofprofile_namesandrole_namesfromcrudinioutput is a solid improvement over array assignment from command substitution, avoiding word splitting and globbing while keeping the loop logic unchanged.
252-265: QuotingGEODESIC_AWS_ROLE_CACHEin fingerprint comparisonUpdating the comparison to:
if [[ $role_fingerprint != "$GEODESIC_AWS_ROLE_CACHE" ]]; thenremoves any chance of unintended glob expansion or word splitting in the cache value, while preserving the fingerprint semantics.
rootfs/templates/wrapper-body.sh (2)
271-275:_running_shell_countmapfile change is correct and more robustUsing:
local count mapfile -t count < <(_running_shell_pids || true) echo "${#count[@]}"eliminates word-splitting/globbing issues from command substitution while preserving the original “number of pids” semantics.
300-305: Quotedbasename "$0"in guidance messageQuoting
basename "$0"and${DOCKER_NAME}in the user-facing guidance line makes the message safe for script/container names with spaces or special characters, with no behavior change otherwise.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @RoseSecurity. * #980 (comment) The following files were modified: * `rootfs/etc/profile.d/aws.sh` * `rootfs/etc/profile.d/fzf.sh` * `rootfs/etc/profile.d/prompt.sh` * `rootfs/templates/wrapper-body.sh`
|
@osterman Is there anyway you would be able to merge this as I'm not authorized to push to this branch? |
|
@osterman Does the Cloud Posse release bot work for this repository or do manual releases need to be cut? |
what
Note
All modifications are non-functional improvements focused on reliability and security
array=($(command))with the more robustmapfile -t array < <(command)syntaxwhy
references