Skip to content

ref(bash): codify dynamic-scope safety for helpers that take variable names #674

@Chemaclass

Description

@Chemaclass

Context

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:

function bashunit::runner::format_subshell_output() {
  local _out=$1
  local 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 outvar
local 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.

Location Status
src/runner.shextract_encoded_field, extract_subshell_type, format_subshell_output, compute_total_assertions Fixed in #672 with __bu_ prefix

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.

Location Construct Risk
src/test_doubles.sh:21,22bashunit::unmock ${variable}_times_file, ${variable}_params_file Theoretical
src/test_doubles.sh:94,95assert_have_been_called ${variable}_times_file Theoretical
src/test_doubles.sh:124,126,128assert_have_been_called_with ${variable}_params_file Theoretical
src/test_doubles.sh:152,153assert_have_been_called_times ${variable}_times_file Theoretical
src/test_doubles.sh:181,182,193,194assert_have_been_called_nth_with ${variable}_times_file, ${variable}_params_file Theoretical

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

  1. 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

  1. 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.

  2. 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

  1. 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)

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentationrefactoringRefactoring or cleaning related

Type

Projects

Status

Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions