Skip to content

fix(interpreter): preserve associative array compound values - #445

Open
trieloff wants to merge 1 commit into
vercel-labs:mainfrom
trieloff:bb/investigate-brew-install-issue-thr_x98dqz7qf6
Open

trieloff wants to merge 1 commit into
vercel-labs:mainfrom
trieloff:bb/investigate-brew-install-issue-thr_x98dqz7qf6

Conversation

@trieloff

@trieloff trieloff commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Context

Fixes the upstream shell defect reported in ai-ecoverse/slicc#3329.

Problem

An associative-array compound assignment made through declare -A silently truncated every value at its first space:

declare -A A=([k]="a b c" [j]="x y")
printf 'k=[%s] j=[%s]\n' "${A[k]}" "${A[j]}"
# just-bash: k=[a] j=[x]
# bash 5.x:  k=[a b c] j=[x y]

Keys and element counts stayed intact, and element-by-element assignment plus bare-array compound assignment were already correct, so the corruption was easy to miss until the value reached a later API call, filename, or commit message.

Cause

Literal assignment builtins (declare, local, typeset, export, and readonly) use a declaration-specific expansion helper so that assignment values do not undergo ordinary word splitting. That helper correctly expanded [k]="a b c" to one element, but reconstructed it as:

[k]=a b c

The declare builtin's array parser then treated the first space as the end of the value and retained only a.

Fix

Keep the keyed [key]= prefix visible to the declaration parser, while serializing the already-expanded value as a quoted string. Backslashes and double quotes are escaped during that internal handoff, so whitespace, empty strings, quotes, and backslashes survive the second parse without being expanded again.

The declaration helper now recognizes both replacement (NAME=(...)) and append (NAME+=(...)) operators and preserves the operator when rebuilding the argument. This also fixes the review-discovered declare A+=([new]="two words") case without clearing existing entries.

A patch changeset is included.

Tests

New focused regression coverage pins:

  • the exact multi-element report, including cardinality;
  • single- and double-quoted values;
  • empty values, embedded quotes, and backslashes;
  • whitespace introduced by parameter expansion.
  • associative declaration appends, including preservation of existing entries and the same special-character cases.

All expected bytes were checked against GNU bash 5.3.15. No Homebrew install was needed because /opt/homebrew/bin/bash was already GNU bash 5.3.15.

Validation:

  • focused associative-array, local-builtin, and expansion-limit suites: 50 passed, 1 skipped;
  • pnpm lint:fix: clean;
  • pnpm typecheck: clean;
  • pnpm knip: clean apart from the repository's two existing configuration hints;
  • pnpm build: clean (the existing CJS import.meta warnings remain);
  • broad suite excluding spec tests: 590 files / 11,774 tests passed, 97 skipped, 11 failed. Ten of the eleven failures reproduce on a detached clean origin/main worktree (four macOS special-mode assertions and six CPython-WASM worker/bundle cases). The remaining defense-in-depth lifecycle failure is order-dependent in the broad run and passes standalone on both this branch and origin/main; none touches assignment expansion.

Co-authored with Codex.

@vercel

vercel Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

@trieloff is attempting to deploy a commit to the Vercel Labs Team on Vercel.

A member of the Team first needs to authorize it.

@trieloff
trieloff force-pushed the bb/investigate-brew-install-issue-thr_x98dqz7qf6 branch from 3050c93 to f6822b1 Compare September 21, 2026 07:37
@auto-maintain

auto-maintain Bot commented Sep 21, 2026

Copy link
Copy Markdown

🤖 auto-maintain review

Automated, advisory triage for @trieloff's PR. Facts below are read from the GitHub API.

Check Result
Author's merged PRs (this repo) 18
Account established ✅ (age 6493d · 135 followers · 204 public repos)
Commits signed/verified ⚠️ 0/1
Changeset included ✅ (.changeset/assoc-array-compound-values.md)

Review panel: 🟡 medium highest severity

just-bash maintainer code review: 🟢 low

No actionable correctness issues found in the complete diff.

General code review: 🟢 low

No actionable defects found in the complete diff.

Adversarial security: 🟢 low

No actionable security issues found in the complete diff.

Adversarial security (second opinion): 🟢 low

Narrow, well-scoped fix: keyed associative-array values are now quoted and escaped before the declare parser re-reads them. The downstream consumers are pure string scanners with no re-expansion, the escaping round-trips exactly, and the change additionally removes a pre-existing key-forgery vector from expansion-supplied subscripts. No backdoors, dependency/CI changes, or sandbox-relevant behavior; no actionable findings.

Standard Bash and host portability: 🟡 medium

Associative compound assignment is fixed only for replacement, leaving Bash-compatible declaration append assignments corrupted.

  • packages/just-bash/src/interpreter/assignment-expansion.ts:42 — The fix excludes `NAME+=(...)`: the helper only recognizes `NAME=(...)`, so `declare A+=([new]="two words")` still loses quoting and stores `two` instead of Bash's `two words`. Accept `+=(` and preserve the operator during serialization.

Posted by auto-maintain. This automated code review is advisory; a human maintainer makes the call.

@trieloff
trieloff force-pushed the bb/investigate-brew-install-issue-thr_x98dqz7qf6 branch from f6822b1 to 0b70c7a Compare September 21, 2026 07:57
@trieloff

trieloff commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the append-assignment finding in 32e355d. The declaration expansion helper now accepts both NAME=(...) and NAME+=(...), preserves the operator during reconstruction, and routes appended keyed values through the same quote/escape handoff. Added a GNU Bash 5.3.15-matched regression covering declare A+=([new]="two words"), preservation of the existing entry, empty values, quotes, and backslashes. Focused suites pass: 50 passed, 1 skipped. The amended commit is GPG-signed with key 8A9F77FB33C0F3E7 and GitHub reports the signature as Verified.

The declaration expansion path rebuilt keyed elements after expansion but emitted their values without quotes. A value like [key]="two words" therefore reached declare as [key]=two words, and its parser silently retained only the first token.

Keep the keyed prefix parseable while serializing the already-expanded value as a quoted, escaped string. Preserve both replacement and append operators so declare A+=(...) follows the same safe path without clearing existing entries.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Lars Trieloff <lars@trieloff.net>
@trieloff

Copy link
Copy Markdown
Contributor Author

Verified this fix against the defect I reported in ai-ecoverse/slicc#3329, and checked the one thing in the diff that looked risky -- it is not:

Keyed values are rebuilt inside double quotes with only \ and " escaped, so I wanted to know whether an already-expanded value containing a literal $ or backtick could be expanded a second time. It cannot: the consumer, declare-array-parsing.ts, is a pure character tokenizer (tracks single/double quote state and \ escapes, accumulates characters, splits on unquoted whitespace) with no expansion anywhere, and it un-escapes exactly the two characters this fix escapes. So declare -A A=( [k]='cost $HOME and date' ) is safe under the new quoting.

Two neighbouring defects measured on 3.4.2 while testing, both outside this PR's scope:

  • local -A NAME drops the -A attribute (declare -- NAME), so a later NAME[key]=value creates an indexed array and every string key collapses to index 0 -- a map silently keeps only its last value while lookups still return it. Filed as local -A drops the associative attribute: string keys then collapse to index 0, so a map silently keeps only its last value #446. Worth noting because element-by-element assignment is the natural workaround for this PR's bug, and it is also wrong inside a function.
  • readonly -A X=( [k]="a b c" ) produces an indexed array of three elements ([k]=a, b, c) rather than an associative array. typeset -A truncates exactly like declare -A, so it is already covered here; export -A is rejected, matching bash.

If it is useful, the test matrix in assoc-array-compound-assignment.test.ts could pin the scope by asserting typeset -A and readonly -A alongside declare -A, plus a single-quoted value carrying a literal $ and a backtick.

This branch has not been deployed

No deployments
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.

1 participant