Skip to content

fix(gbrain-lib): pin LC_ALL=C in varname validator (macOS locale guard)#1606

Open
andrey-esipov wants to merge 1 commit into
garrytan:mainfrom
andrey-esipov:fix-gbrain-lib-locale
Open

fix(gbrain-lib): pin LC_ALL=C in varname validator (macOS locale guard)#1606
andrey-esipov wants to merge 1 commit into
garrytan:mainfrom
andrey-esipov:fix-gbrain-lib-locale

Conversation

@andrey-esipov
Copy link
Copy Markdown

Summary

  • _gstack_gbrain_validate_varname in bin/gstack-gbrain-lib.sh uses case "$name" in [A-Z_][A-Z0-9_]*) to validate that callers pass an ASCII-uppercase identifier
  • On macOS, the default user locale (en_US.UTF-8) makes bash case glob brackets locale-collation-sensitive, so [A-Z] matches lowercase letters too — lower-case passes the check
  • The function then trips printf -v "$varname" and export "$varname" with not a valid identifier errors that surface as confusing mid-prompt failures (instead of the clean invalid var name '…' (must match [A-Z_][A-Z0-9_]*) message the validator was supposed to emit)
  • Fix: pin LC_ALL=C for the duration of the function so the bracket expression has ASCII-only semantics on both macOS and Linux, matching the documented [A-Z_][A-Z0-9_]* contract

The existing test test/gbrain-lib-verify.test.ts'rejects invalid var names (must match [A-Z_][A-Z0-9_]*)' already covers the macOS repro shape. It passes silently on Linux CI (whose default is typically C / POSIX) and fails on macOS dev boxes — so this PR is the source-side fix that makes that test pass on every platform without changing the test.

Repro (macOS, default user locale)

$ bash -c 'name="lower-case"; case "$name" in [A-Z_][A-Z0-9_]*) echo "MATCHED";; *) echo "NO MATCH";; esac'
MATCHED                                # ← bug

$ LC_ALL=C bash -c 'name="lower-case"; case "$name" in [A-Z_][A-Z0-9_]*) echo "MATCHED";; *) echo "NO MATCH";; esac'
NO MATCH                               # ← desired

Before the fix:

$ . bin/gstack-gbrain-lib.sh && read_secret_to_env "lower-case" "Prompt: " <<< "anything"
Prompt: bin/gstack-gbrain-lib.sh: line 89: printf: `lower-case': not a valid identifier
bin/gstack-gbrain-lib.sh: line 91: export: `lower-case': not a valid identifier

After the fix the validator rejects on the first line with the documented error message before either printf -v or export runs.

Test plan

  • bun test test/gbrain-lib-verify.test.ts passes (22/22) on macOS Sonoma + bash 3.2 (system) and bash 5.x (homebrew). Linux CI was already green.
  • Manual: _gstack_gbrain_validate_varname lower-case; echo $? returns 2; _gstack_gbrain_validate_varname FOO_BAR; echo $? returns 0.
  • No other call sites of the validator changed.
  • Reviewers may want to confirm no other case glob in bin/ is similarly locale-sensitive (I grep'd; the rest of the script uses literal strings or =~ regex, which also honors LC_COLLATE in bash 4+ but isn't used for [A-Z]-style ranges in this file).

🤖 Generated with Claude Code

In many macOS shells the default locale (e.g. en_US.UTF-8) makes bash
glob brackets like `[A-Z]` match lowercase letters too, so the existing
`case "$name" in [A-Z_][A-Z0-9_]*)` branch lets names like `lower-case`
through validation. The function then trips `printf -v "$varname"` and
`export "$varname"` with `not a valid identifier` errors that surface
mid-prompt, which is exactly what the validator was supposed to prevent.

Pinning `LC_ALL=C` inside the function gives ASCII-only bracket semantics
on both macOS and Linux, matching the documented `[A-Z_][A-Z0-9_]*`
contract. Declared `local` so it doesn't leak to the calling shell —
`gstack-gbrain-lib.sh` is documented as a sourced helper, so a bare
assignment would mutate the caller's locale for the rest of the process
(silently affecting downstream `sort`, `tr`, locale-aware globs in the
same shell, etc.).

The existing regression test
`test/gbrain-lib-verify.test.ts:'rejects invalid var names'`
already covers the macOS repro shape (passes `lower-case` and expects
the validator to reject + emit `invalid var name`). On Linux CI the
test silently passed because `LC_ALL=C` is the typical default; on
macOS dev boxes it fails.

Verified:
- `bun test test/gbrain-lib-verify.test.ts`: 22 pass, 0 fail (on macOS).
- `_gstack_gbrain_validate_varname lower-case; echo $?` → 2.
- `_gstack_gbrain_validate_varname FOO_BAR; echo $?` → 0.
- Caller's LC_ALL preserved across calls (confirmed via sourced bash).
@andrey-esipov andrey-esipov force-pushed the fix-gbrain-lib-locale branch from 7ce67e6 to b56666f Compare May 19, 2026 16:01
@andrey-esipov
Copy link
Copy Markdown
Author

Self-review caught a side-effect bug in v1 and force-pushed b56666f. The original commit had a bare LC_ALL=C inside the validator — without local, it leaks into the calling shell because gstack-gbrain-lib.sh is documented as a sourced helper.

Repro for the leak (before the fix):

$ bash -c '. bin/gstack-gbrain-lib.sh; LC_ALL=en_US.UTF-8; _gstack_gbrain_validate_varname FOO; echo "LC_ALL after: $LC_ALL"'
LC_ALL after: C    # ← downstream sort/tr/grep -P now silently change behavior

After the fix (local LC_ALL=C):

LC_ALL after: en_US.UTF-8    # ← caller preserved

The case statement still gets ASCII-only bracket semantics inside the function, so lower-case is still correctly rejected. 22/22 tests in gbrain-lib-verify.test.ts still pass.

Commit message and code comment updated to call out both the locale pin and the local requirement so future contributors don't drop the local keyword by accident.

andrey-esipov added a commit to andrey-esipov/astack that referenced this pull request May 19, 2026
Self-review of the upstream PR (garrytan#1606) caught that the
bare `LC_ALL=C` leaks into the calling shell, since this file is sourced
not executed. Downstream `sort`, `tr`, and locale-aware globs in the
same process would silently change behavior after a single call to
`read_secret_to_env`.

`local LC_ALL=C` scopes the locale change to the function — bash
restores the previous value on return.
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