fix(gbrain-lib): pin LC_ALL=C in varname validator (macOS locale guard)#1606
Open
andrey-esipov wants to merge 1 commit into
Open
fix(gbrain-lib): pin LC_ALL=C in varname validator (macOS locale guard)#1606andrey-esipov wants to merge 1 commit into
andrey-esipov wants to merge 1 commit into
Conversation
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).
7ce67e6 to
b56666f
Compare
Author
|
Self-review caught a side-effect bug in v1 and force-pushed 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 behaviorAfter the fix ( LC_ALL after: en_US.UTF-8 # ← caller preservedThe case statement still gets ASCII-only bracket semantics inside the function, so Commit message and code comment updated to call out both the locale pin and the |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_gstack_gbrain_validate_varnameinbin/gstack-gbrain-lib.shusescase "$name" in [A-Z_][A-Z0-9_]*)to validate that callers pass an ASCII-uppercase identifieren_US.UTF-8) makes bashcaseglob brackets locale-collation-sensitive, so[A-Z]matches lowercase letters too —lower-casepasses the checkprintf -v "$varname"andexport "$varname"withnot a valid identifiererrors that surface as confusing mid-prompt failures (instead of the cleaninvalid var name '…' (must match [A-Z_][A-Z0-9_]*)message the validator was supposed to emit)LC_ALL=Cfor 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_]*contractThe 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 typicallyC/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)
Before the fix:
After the fix the validator rejects on the first line with the documented error message before either
printf -vorexportruns.Test plan
bun test test/gbrain-lib-verify.test.tspasses (22/22) on macOS Sonoma + bash 3.2 (system) and bash 5.x (homebrew). Linux CI was already green._gstack_gbrain_validate_varname lower-case; echo $?returns2;_gstack_gbrain_validate_varname FOO_BAR; echo $?returns0.caseglob inbin/is similarly locale-sensitive (I grep'd; the rest of the script uses literal strings or=~regex, which also honorsLC_COLLATEin bash 4+ but isn't used for[A-Z]-style ranges in this file).🤖 Generated with Claude Code