Skip to content

fix(variables): keep dynamic well-known vars live across declare -a / array assign#1228

Open
lu-zero wants to merge 4 commits into
reubeno:mainfrom
lu-zero:pipestatus-dynamic-freeze
Open

fix(variables): keep dynamic well-known vars live across declare -a / array assign#1228
lu-zero wants to merge 4 commits into
reubeno:mainfrom
lu-zero:pipestatus-dynamic-freeze

Conversation

@lu-zero

@lu-zero lu-zero commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Problem

Once a script declare -as or plain-array-assigns a dynamic well-known variable such as PIPESTATUS, brush never updates it again on later pipelines in that shell — it stays frozen at whatever was last assigned. Real bash keeps the variable live and refreshes it on every pipeline.

Minimal repro:

declare -a PIPESTATUS=([0]="1")
true | true | true
echo "${PIPESTATUS[@]}"
  • bash: 0 0 0
  • brush (current origin/main): 1 (frozen)

Same freeze happens with a plain array assignment (no declare):

PIPESTATUS=([0]="9")
true | false | true
echo "${PIPESTATUS[@]}"
  • bash: 0 1 0
  • brush: 9

Root cause

PIPESTATUS (and other dynamic well-known variables: RANDOM, SECONDS, FUNCNAME, BASH_LINENO, BASH_SOURCE, BASH_ALIASES, …) are backed by a ShellValue::Dynamic { getter, setter } whose getter is consulted on every read and whose setter is a no-op (|_| ()) for all of them.

Two write paths in brush-core/src/variables.rs unconditionally overwrote self.value with a static array, discarding the Dynamic binding and permanently freezing the variable:

  1. convert_to_indexed_array — the _ arm hit for Dynamic values and replaced self.value. Triggered by declare -a VAR=(...) via brush-builtins/src/declare.rs.
  2. assign (non-append Array-literal arm) — explicitly matched ShellValue::Dynamic { .. } in the overwrite set. Triggered by a plain VAR=([0]="...") assignment.

From that point on the getter closure was gone, so the variable stopped refreshing for the rest of the shell's life.

Fix

  • convert_to_indexed_array: add an explicit ShellValue::Dynamic { .. } => Ok(()) arm so the binding survives.
  • assign: drop ShellValue::Dynamic { .. } from the overwrite set; the existing (ShellValue::Dynamic { .. }, _) => Ok(()) arm below now handles it correctly.

convert_to_associative_array intentionally unchanged

convert_to_associative_array has the same overwrite pattern, but real bash is asymmetric here: declare -A PIPESTATUS=([x]="9") does permanently freeze it in bash itself (PIPESTATUS becomes a plain associative array that pipelines don't refresh). Pre-fix brush already matched bash's -A behavior, so changing it would diverge from bash compat.

Verification

All four behavioral cases now match bash:

op bash brush (after)
declare -a then pipe 0 0 0 0 0 0
declare -A then pipe 9 9
plain =([0]="9") then pipe 0 1 0 0 1 0
plain pipe 0 0 0 0 0 0

Adds three compat test cases under brush-shell/tests/cases/compat/well_known_vars.yaml (declare -a, plain array assign, declare -A). cargo fmt --check clean.

This surfaced in the wild: a worker-env dump/restore mechanism restored a declare -a PIPESTATUS=([0]="1") line and a downstream pipestatus || die check fired incorrectly, misreporting a successful pipeline as failed.

@lu-zero

lu-zero commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

The Source code checks (1.88.0, ubuntu) failure is cargo deny --all-features check all flagging two newly-published advisories (RUSTSEC-2026-0194, RUSTSEC-2026-0195) in the advisory DB. bans, licenses, and sources all pass. This is unrelated to this change (a 2-line variables.rs edit + tests) and would fail on origin/main too — it is an upstream advisory-DB/infra issue affecting all open PRs.

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 13.52 μs 13.97 μs 0.45 μs 🟠 +3.34%
eval_arithmetic 0.12 μs 0.13 μs 0.01 μs 🟠 +11.97%
expand_one_string 1.27 μs 1.33 μs 0.07 μs ⚪ Unchanged
for_loop 25.68 μs 25.67 μs -0.01 μs ⚪ Unchanged
full_peg_complex 42.92 μs 42.57 μs -0.35 μs ⚪ Unchanged
full_peg_for_loop 4.71 μs 4.66 μs -0.05 μs 🟢 -1.15%
full_peg_nested_expansions 12.37 μs 12.23 μs -0.14 μs 🟢 -1.11%
full_peg_pipeline 3.19 μs 3.13 μs -0.05 μs 🟢 -1.60%
full_peg_simple 1.35 μs 1.33 μs -0.02 μs 🟢 -1.19%
function_call 2.85 μs 2.96 μs 0.11 μs ⚪ Unchanged
instantiate_shell 44.70 μs 44.53 μs -0.17 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 21811.38 μs 22912.07 μs 1100.69 μs 🟠 +5.05%
parse_peg_bash_completion 1615.34 μs 1612.83 μs -2.51 μs ⚪ Unchanged
parse_peg_complex 14.58 μs 14.60 μs 0.03 μs ⚪ Unchanged
parse_peg_for_loop 1.42 μs 1.46 μs 0.03 μs 🟠 +2.32%
parse_peg_pipeline 1.47 μs 1.57 μs 0.09 μs 🟠 +6.45%
parse_peg_simple 0.79 μs 0.78 μs -0.01 μs 🟢 -1.26%
run_echo_builtin_command 12.59 μs 13.20 μs 0.61 μs 🟠 +4.87%
tokenize_sample_script 2.73 μs 2.73 μs -0.01 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-builtins/src/declare.rs 🟢 87.4% 🟢 87.6% 🟢 0.2%
brush-core/src/env.rs 🟢 87.16% 🟢 88.38% 🟢 1.22%
brush-core/src/variables.rs 🟢 91.38% 🟢 91.44% 🟢 0.06%
brush-core/src/wellknownvars.rs 🟢 84.2% 🟢 84.91% 🟢 0.71%
brush-test-harness/src/reporting.rs 🔴 46.83% 🔴 46.48% 🔴 -0.35%
Overall Coverage 🟢 75.41% 🟢 75.48% 🟢 0.07%

Minimum allowed coverage is 70%, this run produced 75.48%
Maximum allowed coverage difference is -5%, this run produced 0.07%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1581 74.96
❗️ Error 17 0.81
❌ Fail 157 7.44
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

@lu-zero
lu-zero force-pushed the pipestatus-dynamic-freeze branch 2 times, most recently from 2af5bea to 5add53b Compare July 5, 2026 16:38
@reubeno
reubeno requested a review from Copilot July 11, 2026 19:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes PIPESTATUS becoming static after indexed-array declarations or assignments.

Changes:

  • Preserves dynamic values during indexed-array conversion and assignment.
  • Adds compatibility tests for indexed, associative, and plain assignments.
  • Review found the conversion is incorrectly applied to all dynamic variable types.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
brush-core/src/variables.rs Adjusts dynamic variable conversion and assignment.
brush-shell/tests/cases/compat/well_known_vars.yaml Adds PIPESTATUS regression tests.

Comment thread brush-core/src/variables.rs Outdated
// freeze the variable at whatever snapshot we materialized, breaking it
// forever. Real bash keeps these variables live across `declare -a`, so
// accept the declaration syntactically but leave the value untouched.
ShellValue::Dynamic { .. } => Ok(()),
lu-zero added 4 commits July 16, 2026 06:42
… array assign

`PIPESTATUS` (and other dynamic well-known variables: RANDOM, SECONDS,
FUNCNAME, BASH_LINENO, BASH_SOURCE, BASH_ALIASES, ...) are backed by a
`ShellValue::Dynamic { getter, setter }` whose `getter` is consulted on
every read and whose `setter` is a no-op (`|_| ()`) for all of them.

Two write paths were unconditionally overwriting `self.value` with a
static array, discarding the Dynamic binding and permanently freezing
the variable at whatever was last assigned:

1. `convert_to_indexed_array` -- the `_` arm hit for Dynamic values.
   Triggered by `declare -a VAR=(...)`.
2. `assign` (non-append Array-literal arm) -- explicitly matched
   `ShellValue::Dynamic { .. }` in the overwrite set. Triggered by a
   plain `VAR=([0]="...")` assignment.

From that point on the getter closure was gone, so the variable stopped
refreshing for the rest of the shell's life. Minimal repro:

    declare -a PIPESTATUS=([0]="1")
    true | true | true
    echo "${PIPESTATUS[@]}"          # bash: 0 0 0 ; brush: 1 (frozen)

Fix:
- `convert_to_indexed_array`: add an explicit `ShellValue::Dynamic { .. }`
  => Ok(()) arm so the binding survives.
- `assign`: drop `ShellValue::Dynamic { .. }` from the overwrite set; the
  existing `(ShellValue::Dynamic { .. }, _) => Ok(())` arm below now
  handles it correctly.

`convert_to_associative_array` is intentionally left unchanged: real
bash itself permanently freezes PIPESTATUS into a plain associative
array after `declare -A`, and pre-fix brush already matched that
behavior. Changing it would diverge from bash compat.

Adds three compat test cases mirroring the repro (`declare -a`, plain
array assign, and `declare -A`) under well_known_vars.yaml.
…version

Copilot correctly flagged that the earlier blanket
`ShellValue::Dynamic { .. } => Ok(())` in convert_to_indexed_array applied to
every dynamic variable, not just indexed-array-shaped ones like PIPESTATUS.
Verified against real bash: declare -a/-A on a dynamic variable should behave
according to its native shape, not uniformly:

- indexed-shaped (PIPESTATUS): declare -a keeps it live (already correct);
  declare -A errors, matching a real indexed array.
- associative-shaped (BASH_ALIASES, BASH_CMDS): declare -A keeps it live;
  declare -a errors, matching a real associative array.
- scalar-shaped (RANDOM, SECONDS, ...): both declare -a and declare -A
  materialize (and permanently freeze) a real array, matching bash's observed
  behavior for these.

Added a `DynamicValueKind` tag to `ShellValue::Dynamic` so the conversion
logic can make this decision without needing a shell reference to invoke the
getter. Also discovered (by testing against real bash) that an accompanying
value bypasses the shape check for dynamic variables specifically
(`declare -A PIPESTATUS=(...)` succeeds and freezes, while a bare
`declare -A PIPESTATUS` errors) — added `_for_reassignment` variants used
only when declare supplies an explicit value, to preserve this without
regressing the existing PIPESTATUS compat tests.
${BASH_ALIASES[x]} was silently returning empty instead of the alias value.
The check that decides whether an array-index expansion should treat its
index as a literal key (associative) vs. an arithmetic expression (indexed)
only recognized real ShellValue::AssociativeArray, not a Dynamic variable
that resolves to one. So "x" was evaluated arithmetically (as 0, via the
unrelated variable x) instead of used as the alias name, and the lookup
missed.

Now that Dynamic carries a DynamicValueKind, extend the check to also treat
DynamicValueKind::AssociativeArray as an associative array for this purpose.
Verified against real bash that ${BASH_ALIASES[x]} now resolves correctly.
…s it

When declare -a/-A materializes a scalar-shaped dynamic variable (e.g.
RANDOM, SECONDS) into a real array, it previously always froze it to an
empty string, because the conversion path had no way to invoke the dynamic
getter (which needs a shell reference).

Thread a resolved snapshot through: at the one caller that has shell access
(declare.rs), resolve the variable's current dynamic value via
resolve_value() before taking the mutable borrow needed to perform the
conversion, then pass it in as an optional pre-resolved value. Internal
callers within variables.rs (String-only promotions) pass None, since they
never operate on Dynamic values.

Verified: `declare -a RANDOM; declare -p RANDOM` now freezes at RANDOM's
actual current reading (e.g. `declare -ai RANDOM=([0]="1364")`) instead of
an empty array, matching real bash.
@lu-zero
lu-zero force-pushed the pipestatus-dynamic-freeze branch from 5add53b to ac587fc Compare July 16, 2026 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants