From e79497b3cffab57f2509d6b10f2683a16f047943 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Thu, 13 Aug 2026 22:23:45 +0000 Subject: [PATCH] fix(tests): guard remaining bash 3.2 hazards from PR #95 review Review findings #5 and #7 on PR #95 flagged two bash-3.2-incompatible patterns despite the surrounding scripts claiming 3.2 safety: - tests/run-bats.sh used `mapfile` (bash 4.0+), which fails immediately under macOS's stock bash 3.2 before any batching logic runs. Replaced with the `while read` loop already established in tests/run-tests.sh, and guarded the two downstream `${TEST_FILES[@]}` expansions with `${arr[@]+"${arr[@]}"}` to match that file's convention. - `trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT` was unguarded in tests/test-sync-marketplace-mirror.sh and tests/test-sync-plugin-content.sh: under `set -u`, if `mktemp -d` fails before the array is populated, the trap itself throws an unbound-variable error that masks the real test failure. A repo-wide grep for the same pattern turned up a third, unreviewed instance in tests/test-check-release-needed.sh. Fixed all three with the guarded idiom already used elsewhere in the repo. Extended the existing bash-3.2-hazard static check (test 16 in tests/test-vale-wrap.sh) to scan all four fixed files going forward, so a regression of either pattern fails the suite instead of only surfacing on a real bash 3.2 host. Refs: PR #95 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT --- tests/run-bats.sh | 14 +++++++++++--- tests/test-check-release-needed.sh | 2 +- tests/test-sync-marketplace-mirror.sh | 2 +- tests/test-sync-plugin-content.sh | 2 +- tests/test-vale-wrap.sh | 13 +++++++++++-- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/tests/run-bats.sh b/tests/run-bats.sh index d82c2d2..80daa06 100755 --- a/tests/run-bats.sh +++ b/tests/run-bats.sh @@ -16,7 +16,15 @@ if [[ ! -x "$BATS" ]]; then exit 1 fi -mapfile -t TEST_FILES < <( +# Collected with a `while read` loop rather than `mapfile` — macOS ships +# /bin/bash 3.2, which has no `mapfile`. Process substitution (not a pipe) +# keeps the loop in this shell so the appends survive. `sort` is still fed +# newline-delimited output, exactly as before. Same convention as +# tests/run-tests.sh. +TEST_FILES=() +while IFS= read -r f; do + TEST_FILES+=("$f") +done < <( find "$REPO_ROOT" -name "*.bats" \ -not -path "*/tests/bats/*" \ -not -path "*/test_helper/*" \ @@ -48,7 +56,7 @@ source "$REPO_ROOT/scripts/lib/batch-run.sh" declare -a batch_args=() i=0 -for f in "${TEST_FILES[@]}"; do +for f in ${TEST_FILES[@]+"${TEST_FILES[@]}"}; do i=$((i + 1)) cmd="$(printf '%q %q; echo $? >%q' "$BATS" "$f" "$SCRATCH_ROOT/$i.status")" batch_args+=("$i" "$cmd") @@ -59,7 +67,7 @@ FAIL=0 TOTAL_OK=0 TOTAL_NOT_OK=0 i=0 -for f in "${TEST_FILES[@]}"; do +for f in ${TEST_FILES[@]+"${TEST_FILES[@]}"}; do i=$((i + 1)) rel="${f#"$REPO_ROOT"/}" echo "=== $rel ===" diff --git a/tests/test-check-release-needed.sh b/tests/test-check-release-needed.sh index 410281b..6e1afb2 100755 --- a/tests/test-check-release-needed.sh +++ b/tests/test-check-release-needed.sh @@ -95,7 +95,7 @@ run_check() { } CLEANUP_DIRS=() -trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT +trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT track() { CLEANUP_DIRS+=("$1"); } # --- 1. Not targeting main: silent no-op regardless of state --- diff --git a/tests/test-sync-marketplace-mirror.sh b/tests/test-sync-marketplace-mirror.sh index 7fd6de5..79a4973 100755 --- a/tests/test-sync-marketplace-mirror.sh +++ b/tests/test-sync-marketplace-mirror.sh @@ -44,7 +44,7 @@ run_script() { } CLEANUP_DIRS=() -trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT +trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT track() { CLEANUP_DIRS+=("$1"); } # --- 1. No .claude-plugin/marketplace.json at all: real-sync mode is a no-op, exit 0 --- diff --git a/tests/test-sync-plugin-content.sh b/tests/test-sync-plugin-content.sh index 44080e0..024bd78 100755 --- a/tests/test-sync-plugin-content.sh +++ b/tests/test-sync-plugin-content.sh @@ -104,7 +104,7 @@ EOF } CLEANUP_DIRS=() -trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT +trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT track() { CLEANUP_DIRS+=("$1"); } # --- 1. --check reports drift before any sync has run --- diff --git a/tests/test-vale-wrap.sh b/tests/test-vale-wrap.sh index 03e681e..0c7e1eb 100755 --- a/tests/test-vale-wrap.sh +++ b/tests/test-vale-wrap.sh @@ -444,7 +444,12 @@ fi # site, so the construct is not a hazard there and demanding the guarded form # would be a wrong test. The file list covers every script this repo ships or # runs that a macOS user reaches: the wrapper itself, the two pre-commit hook -# scripts, and the test runner AGENTS.md tells contributors to run by hand. +# scripts, the test runner AGENTS.md tells contributors to run by hand, its +# bats-dispatch companion, and the three test-*.sh scripts whose +# `trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT` cleanup traps were unguarded (PR +# #95 review finding #7 named two of them; a repo-wide grep for the same +# pattern turned up test-check-release-needed.sh as a third) until they were +# switched to the guarded form. # `mapfile` is checked alongside, because it is bash 4.0+ and the expansion scan # cannot see it — run-tests.sh carried one until it was replaced with a # `while read` loop, and nothing would have caught its return. `declare -A` @@ -478,7 +483,11 @@ for BASH32_SCRIPT in \ "$REPO_ROOT/scripts/skill-size-check.sh" \ "$REPO_ROOT/scripts/check-release-needed.sh" \ "$REPO_ROOT/scripts/check-vale-style-sync.sh" \ - "$REPO_ROOT/tests/run-tests.sh"; do + "$REPO_ROOT/tests/run-tests.sh" \ + "$REPO_ROOT/tests/run-bats.sh" \ + "$REPO_ROOT/tests/test-sync-marketplace-mirror.sh" \ + "$REPO_ROOT/tests/test-sync-plugin-content.sh" \ + "$REPO_ROOT/tests/test-check-release-needed.sh"; do FOUND16="$(unguarded_expansions "$BASH32_SCRIPT")" if [[ -n "$FOUND16" ]]; then HAZARDS16+="$FOUND16 "