You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Bash local is dynamically scoped, not lexically scoped. A local declared inside a callee is visible to (and shadows) any caller variable of the same name during the call. This bit us in #662 / PR #672:
functionbashunit::runner::format_subshell_output() {
local _out=$1local subshell_output=$2# LOCAL shadows caller's same-named var
...
eval"$_out=\$line"# assigns to LOCAL, not caller
}
# Caller — passes its own var name as the outvarlocal subshell_output="raw"
bashunit::runner::format_subshell_output subshell_output "$subshell_output"echo"$subshell_output"# still "raw" — silent data loss
That bug surfaced as 12 parallel test failures and was only fixed by renaming all internal locals with a __bu_ prefix. Worth a sweep to confirm no similar landmines exist elsewhere and to codify the rule so future helpers don't regress.
Audit findings
I scanned src/*.sh for the three relevant patterns:
1. Eval-assignment outvar (write back via parameter name)
Pattern: eval "$out_var_name=..." where out_var_name is $1.
No other eval-assignment outvar helpers exist today. Good baseline.
2. Indirect-read by parameter-constructed name (${!var})
Pattern: function takes a value, constructs a variable name from it, reads via ${!constructed_name}. Vulnerable when a caller's local happens to match the constructed name.
These resolve names like mycmd_times_file (derived from sanitized command name). A test that locally declares a variable with that exact derived name and then calls the spy/assert helper would silently read the local instead of the exported global. Real-world likelihood is low but the failure mode is invisible (no error, just wrong assertion).
3. Intentional dynamic-scope mutation
Location
Notes
src/coverage.sh:818-829 — _branch_push_if, _branch_close_if_arm, etc.
Documented in the comment block at lines 818-821 as intentional (helpers mutate caller's if_decision_line, if_arms, if_depth, if_arm_start arrays). Fine as-is; the rationale is recorded.
src/coverage.sh:784 — _BASHUNIT_BRANCH_ARMS_OUT global slot
Single-slot global output; safe (no name collision possible, only one output channel).
What to do
Must
Codify the convention in .claude/rules/bash-style.md under a new section "Outvar helpers (dynamic-scope safety)". Required wording:
"Helpers that take a caller-named variable as an outvar (first arg) MUST prefix all internal locals with __bu_ to avoid shadowing the caller's variable through dynamic scoping."
Include the minimal failing example (the subshell_output case above).
Mention that printf -v is not Bash 3.0-safe and declare -n requires 4.3+, hence the eval "$_out=\$__bu_val" + prefix pattern is the portable contract.
Should
Defensive audit of src/test_doubles.sh: rename internal locals that participate in constructed names so a colliding caller local cannot be confused with the exported global. Concretely, the variable, file_var, times_file_var, params_file_var locals on the indirect-read paths. No bug filed today; this is hardening.
Add a regression test that exercises the shadow scenario for at least one outvar helper from perf(runner): outvar pattern in hot-path result helpers #672: caller declares a local with the same name as one of the helper's internal locals, then calls the helper; assert the helper still produces the right value. Cheap insurance against someone dropping the prefix later.
Nice to have
shellcheck-style lint guard: a small CI script that greps src/*.sh for eval "\$[a-zA-Z_]*= and asserts every match lives in a function whose locals are __bu_-prefixed. Catches the regression mechanically.
Acceptance
Rule documented in .claude/rules/bash-style.md
test_doubles.sh indirect-read sites audited and either renamed or explicitly noted as safe
At least one regression test for the shadow scenario in tests/unit/runner_test.sh
Bash 3.0+ compat preserved (no namrefs, no declare -n)
Context
Bash
localis dynamically scoped, not lexically scoped. Alocaldeclared inside a callee is visible to (and shadows) any caller variable of the same name during the call. This bit us in #662 / PR #672:That bug surfaced as 12 parallel test failures and was only fixed by renaming all internal locals with a
__bu_prefix. Worth a sweep to confirm no similar landmines exist elsewhere and to codify the rule so future helpers don't regress.Audit findings
I scanned
src/*.shfor the three relevant patterns:1. Eval-assignment outvar (write back via parameter name)
Pattern:
eval "$out_var_name=..."whereout_var_nameis$1.src/runner.sh—extract_encoded_field,extract_subshell_type,format_subshell_output,compute_total_assertions__bu_prefixNo other eval-assignment outvar helpers exist today. Good baseline.
2. Indirect-read by parameter-constructed name (
${!var})Pattern: function takes a value, constructs a variable name from it, reads via
${!constructed_name}. Vulnerable when a caller's local happens to match the constructed name.src/test_doubles.sh:21,22—bashunit::unmock${variable}_times_file,${variable}_params_filesrc/test_doubles.sh:94,95—assert_have_been_called${variable}_times_filesrc/test_doubles.sh:124,126,128—assert_have_been_called_with${variable}_params_filesrc/test_doubles.sh:152,153—assert_have_been_called_times${variable}_times_filesrc/test_doubles.sh:181,182,193,194—assert_have_been_called_nth_with${variable}_times_file,${variable}_params_fileThese resolve names like
mycmd_times_file(derived from sanitized command name). A test that locally declares a variable with that exact derived name and then calls the spy/assert helper would silently read the local instead of the exported global. Real-world likelihood is low but the failure mode is invisible (no error, just wrong assertion).3. Intentional dynamic-scope mutation
src/coverage.sh:818-829—_branch_push_if,_branch_close_if_arm, etc.if_decision_line,if_arms,if_depth,if_arm_startarrays). Fine as-is; the rationale is recorded.src/coverage.sh:784—_BASHUNIT_BRANCH_ARMS_OUTglobal slotWhat to do
Must
.claude/rules/bash-style.mdunder a new section "Outvar helpers (dynamic-scope safety)". Required wording:__bu_to avoid shadowing the caller's variable through dynamic scoping."subshell_outputcase above).printf -vis not Bash 3.0-safe anddeclare -nrequires 4.3+, hence theeval "$_out=\$__bu_val"+ prefix pattern is the portable contract.Should
Defensive audit of
src/test_doubles.sh: rename internal locals that participate in constructed names so a colliding caller local cannot be confused with the exported global. Concretely, thevariable,file_var,times_file_var,params_file_varlocals on the indirect-read paths. No bug filed today; this is hardening.Add a regression test that exercises the shadow scenario for at least one outvar helper from perf(runner): outvar pattern in hot-path result helpers #672: caller declares a local with the same name as one of the helper's internal locals, then calls the helper; assert the helper still produces the right value. Cheap insurance against someone dropping the prefix later.
Nice to have
src/*.shforeval "\$[a-zA-Z_]*=and asserts every match lives in a function whose locals are__bu_-prefixed. Catches the regression mechanically.Acceptance
.claude/rules/bash-style.mdtest_doubles.shindirect-read sites audited and either renamed or explicitly noted as safetests/unit/runner_test.shdeclare -n)