Skip to content

fix: replace raw echo with output helpers in check #706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
github-actions wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: replace raw echo with output helpers in check #706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c107189
fix: replace raw echo with output helpers in check
dnyw4l3n13 Jun 24, 2026
111228a
fix: call success() after all checks pass in check script
dnyw4l3n13 Jun 24, 2026
4673a79
fix: prefix sh-portability advisory with Hint: to distinguish from pr…
dnyw4l3n13 Jun 24, 2026
cd3db9a
fix: use mapfile array to avoid word-splitting on filenames with spaces
dnyw4l3n13 Jun 24, 2026
1b089c1
fix: guard against empty FILES array before loop and success message
dnyw4l3n13 Jun 24, 2026
29e568b
fix: include filename in all die messages inside the per-file loop
dnyw4l3n13 Jun 24, 2026
2b9127a
docs: add CHANGELOG entries for correctness fixes in check
dnyw4l3n13 Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,16 @@ Please ADD ALL Changes to the UNRELEASED SECTION and not a specific release
- Replace raw echo with output helpers in db/createmssqldb
- Replace raw echo with standard output helpers (die/info/success) in db/create-deploy-script
- git/fetch: Unset core.hookspath for each repo during fetch so that globally-configured hook paths do not persist on individual repos
- check: use mapfile array for file collection to handle filenames containing spaces correctly
- check: guard against empty file list before checking — prevents false-positive success when no scripts are found
### Changed
- Replace raw echo with standard output helpers (die/info/success) in github/cancel-workflows
- Replace raw echo with standard output helpers (die/info/success) in git/update-repos-personal
- Replace raw echo with output helpers (die, info, success) in git/missing-release-branches
- git/make-preview: replaced raw echo with die/info/success output helpers
- Replaced raw echo with output helpers (die/info/success) in git/clean-all
- GEOIP - Updated GEOIP DB from MaxMind (2026-06-24)
- check: replaced raw echo-based output with standard die, info, and success helpers
### Deprecated
### Removed
- db/sqlcompare script deleted — no longer in use
Expand Down
66 changes: 38 additions & 28 deletions check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,28 @@
#! /bin/bash

die() {
echo "$@"
if [ -t 2 ]; then
printf '\n\033[31m✗\033[0m %s\n' "$*" >&2
else
printf '\n✗ %s\n' "$*" >&2
fi
exit 1
}

testing() {
echo " + $*"
}

hint() {
echo " ? CHECK: $*"
success() {

@dnyw4l3n13 dnyw4l3n13 Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

success() is defined here but is never called anywhere in the script.

Every file that passes all checks exits the loop silently — the user sees a stream of → progress lines and then nothing, making it impossible to distinguish "completed successfully" from "script terminated mid-run". The convention in shell-scripts.instructions.md states success is for completion (green ✓). At minimum it should be called once after the loop to confirm all checks passed:

Suggested change
success() {
success() {
if [ -t 1 ]; then
printf '\n\033[32m✓\033[0m %s\n' "$*"
else
printf '\n✓ %s\n' "$*"
fi
}

And at the end of the script (after done), add:

success "All checks passed"

@dnyw4l3n13 dnyw4l3n13 Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fixed in 111228a — added success "All checks passed" after the loop.

if [ -t 1 ]; then
printf '\n\033[32m✓\033[0m %s\n' "$*"
else
printf '\n✓ %s\n' "$*"
fi
}

fail() {
echo " - ERROR: $*"
exit 1

info() {
if [ -t 1 ]; then
printf '\n\033[32m→\033[0m %s\n' "$*"
else
printf '\n→ %s\n' "$*"
fi
}


Expand All @@ -39,7 +45,7 @@ excluded() {
}

# Find all the script files both executable and ones with shebang
FILES=$({ \
mapfile -t FILES < <({ \
find "$BASEDIR" -not -path '*/.*' -type f -executable -print || die "Search: find"
while IFS= read -r SHEBANG_FILE; do
IFS= read -r FIRST_LINE < "$SHEBANG_FILE" || continue
Expand All @@ -52,47 +58,51 @@ FILES=$({ \
done < <(find "$BASEDIR" -not -path '*/.*' -type f -print) || die "Search: Shebang"
} | sort | uniq)

echo "Testing:"
for FILE in $FILES; do
echo "* $FILE"
testing "Executable"
[ "${#FILES[@]}" -gt 0 ] || die "No script files found in ${BASEDIR}"

info "Testing:"
for FILE in "${FILES[@]}"; do
info "* $FILE"

@dnyw4l3n13 dnyw4l3n13 Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Pre-existing: for FILE in $FILES iterates over an unquoted variable, which word-splits on $IFS (space, tab, newline). If any file path under $BASEDIR contains a space, the path is split into fragments and each fragment is processed as a separate (nonexistent) filename, causing spurious die "Not Executable" or die "No Shebang" failures.

The fix is to read into an array and iterate with "${FILES[@]}", or collect via mapfile:

mapfile -t FILES < <({ \
      find "$BASEDIR" -not -path '*/.*' -type f -executable -print || die "Search: find"
      ...
      } | sort -u)

for FILE in "${FILES[@]}"; do

This is adjacent to the changes in this PR — worth fixing here while the loop is being touched.

@dnyw4l3n13 dnyw4l3n13 Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fixed in cd3db9a — replaced FILES=$(...) + unquoted $FILES with mapfile -t FILES < <(...) and "${FILES[@]}".

info "Executable"
if [ ! -x "$FILE" ]; then
fail "Not Executable"
die "Not Executable: ${FILE}"
fi

testing "Shebang"
info "Shebang"
FIRST_LINE=$(head -1 "$FILE")
if [[ $FIRST_LINE =~ $REGEX_SHEBANG ]]; then
SHEBANG_EXEC="${BASH_REMATCH[1]}"
SHELL=
if [ "$SHEBANG_EXEC" = "/bin/bash" ]; then
SHELL="bash"
bash -n -u "$FILE" || fail "$SHELL: Invalid Script"
sh -n "$FILE" > /dev/null 2>&1 && hint "Could be run in bash?"
bash -n -u "$FILE" || die "$SHELL: Invalid Script: ${FILE}"
sh -n "$FILE" > /dev/null 2>&1 && info "Hint: Could be run in sh?"
elif [ "$SHEBANG_EXEC" = "/usr/bin/env bash" ]; then
SHELL="bash"
bash -n -u "$FILE" || fail "$SHELL: Invalid Script"
sh -n "$FILE" > /dev/null 2>&1 && hint "Could be run in bash?"
bash -n -u "$FILE" || die "$SHELL: Invalid Script: ${FILE}"
sh -n "$FILE" > /dev/null 2>&1 && info "Hint: Could be run in sh?"
elif [ "$SHEBANG_EXEC" = "/bin/sh" ]; then
SHELL="sh"
sh -n "$FILE" || fail "$SHELL: Invalid Script"
sh -n "$FILE" || die "$SHELL: Invalid Script: ${FILE}"
elif [ "$SHEBANG_EXEC" = "/usr/bin/env sh" ]; then
SHELL="sh"
sh -n "$FILE" || fail "$SHELL: Invalid Script"
sh -n "$FILE" || die "$SHELL: Invalid Script: ${FILE}"
else
fail "Unknown Shell: $SHEBANG_EXEC"
die "Unknown Shell: $SHEBANG_EXEC in ${FILE}"
fi


else
fail "No Shebang: $FIRST_LINE"
die "No Shebang: $FIRST_LINE in ${FILE}"
fi

## Run Shellcheck on the file
if [ "$(excluded "$FILE")" = "1" ]; then
hint "Excluded from shellcheck"
info "Excluded from shellcheck"
else
shellcheck "$FILE" || fail "Shellcheck"
shellcheck "$FILE" || die "Shellcheck: ${FILE}"
fi

done

success "All checks passed"

@dnyw4l3n13 dnyw4l3n13 Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

success "All checks passed" fires even when FILES is empty (no scripts found).

If BASEDIR is wrong, a mount fails, or all files are excluded by the -not -path '*/.*' filter, mapfile populates an empty array. The for FILE in "${FILES[@]}" loop body is skipped silently. success "All checks passed" then prints unconditionally — a false pass in CI with nothing actually checked.

Add a guard before the loop:

Suggested change
success "All checks passed"
[ "${#FILES[@]}" -gt 0 ] || die "No script files found in ${BASEDIR}"
info "Testing:"
for FILE in "${FILES[@]}"; do

@dnyw4l3n13 dnyw4l3n13 Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fixed in 1b089c1 — added [ "${#FILES[@]}" -gt 0 ] || die "No script files found" guard before the loop.

Loading