fix(scripts): make bash branch-name sanitizing match the Python and PowerShell twins - #4286
Conversation
There was a problem hiding this comment.
Pull request overview
Aligns Bash branch-name sanitization with Python and PowerShell across locales and platforms.
Changes:
- Uses locale-independent sanitization, portable
sed, and safeprintf. - Adds parity coverage for locales, repeated separators, and option-like names.
- Introduces locale probing for tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/bash/create-new-feature.sh |
Fixes core Bash sanitization. |
extensions/git/scripts/bash/create-new-feature-branch.sh |
Applies equivalent Git extension changes. |
tests/test_create_new_feature_python_parity.py |
Adds core parity tests. |
tests/extensions/git/test_git_extension_python_parity.py |
Expands extension parity tests. |
tests/parity_helpers.py |
Adds locale detection helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # LC_ALL=C for the same collation reason documented on clean_branch_name, | ||
| # and so the `grep -qw` acronym probe below uses ASCII word boundaries like | ||
| # the Python twin's (?<![0-9A-Za-z_]) lookarounds. | ||
| local -x LC_ALL=C |
There was a problem hiding this comment.
Confirmed and fixed. "éDBé cache" gave db-cache from bash and cache from the extension's Python twin — LC_ALL=C makes the accent a word boundary for grep -qw,
while Unicode \b treats éDBé as one word.
Aligned to explicit ASCII lookarounds, matching what the core Python twin (scripts/python/create_new_feature.py:185) already did. Also fixed
scripts/powershell/create-new-feature.ps1, which is outside your comment but had the same \b against a core Python twin that already used ASCII boundaries, so core had the
divergence too.
Added test_acronym_adjacent_to_non_ascii_matches_python to the extension parity suite and an acronym_next_to_non_ascii case to test_python_branch_name_generation_matches_bash.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/extensions/git/test_git_extension_python_parity.py:179
- This extension parity table covers the separator-collapse fix but not the
echo-option fix made in the duplicated extension Bash script. Only the core script gets-n/-e/-Ecoverage, so the extension copy could regress independently while CI remains green. Add those values here as well.
("User_Auth!", "001-user-auth"),
("User__Auth!!", "001-user-auth"),
("auth -- v2", "001-auth-v2"),
],
ids=["single_separators", "repeated_separators", "separator_run"],
| if ($word.Length -ge 3) { | ||
| $meaningfulWords += $word | ||
| } elseif ($Description -cmatch "\b$($word.ToUpper())\b") { | ||
| } elseif ($Description -cmatch "(?<![0-9A-Za-z_])$($word.ToUpper())(?![0-9A-Za-z_])") { |
| if ($word.Length -ge 3) { | ||
| $meaningfulWords += $word | ||
| } elseif ($Description -cmatch "\b$($word.ToUpper())\b") { | ||
| } elseif ($Description -cmatch "(?<![0-9A-Za-z_])$($word.ToUpper())(?![0-9A-Za-z_])") { |
Description
clean_branch_name/generate_branch_namein the bash twins produce branch andspecs/directory names that differ from the Python and PowerShell twins. Threeindependent defects, all in the same sanitizing pipeline, all in two copies of it
(
scripts/bash/create-new-feature.shandextensions/git/scripts/bash/create-new-feature-branch.sh).1.
[^a-z0-9]is locale-dependent (the user-visible one)glibc resolves a bracket-expression range through the locale's collation
table, so under
en_US.UTF-8thea-zinsed 's/[^a-z0-9]/-/g'coversaccented lowercase letters. Under
C.UTF-8/ POSIX it does not.Same repo, same command, same description — only
LANGdiffers:en_US.UTF-8is the default on macOS Terminal and most Linux desktops; CIrunners generally run in the POSIX locale, which is why this never showed up in
the suite. Two teammates running
/speckit.specifyon the same description gettwo different
specs/directories, and the second one's/speckit.plancannotfind the first one's feature. The git extension checks out a branch with those
bytes in it.
Same divergence for Spanish (
añadirvsadir), German (prüfungvsfung),Portuguese, Nordic — anything in Latin-1 Supplement / Latin Extended-A.
2.
sed 's/-\+/-/g'does not collapse anything on macOS\+is a GNU extension, not POSIX BRE. POSIX/BSDsedreads it as a literal+, so the collapse step is a no-op there:--short-name 'My Fancy!! Name'therefore yields001-my-fancy---nameon macOSand
001-my-fancy-nameon Linux and from the Python/PowerShell twins. The repoalready knew:
tests/extensions/git/test_git_extension_python_parity.pycarriedand deliberately tested only inputs that could not hit it. That comment and that
restriction are removed here.
3.
echo "$name"eats-n/-e/-EThe raw value went through
echo, so those three short names were consumed asoptions and produced an empty suffix (
001-) where Python produces001-n.Fix
LC_ALL=Cfor the sanitizing pipeline (byte-exact classes, and ASCII wordboundaries for the
grep -qwacronym probe, matching the Python twin's(?<![0-9A-Za-z_])lookarounds), the portable--*in place of\+, andprintf '%s\n'in place ofecho. The same edit in both copies of thefunction. Behaviour on ASCII input in the POSIX locale — which is what CI has
been exercising — is unchanged.
local -xscopes and exports the override to the pipeline's children only; itis restored on return, so nothing else in the script or the caller's environment
sees it.
Follow-up from review: ASCII acronym boundaries
Copilot spotted that scoping
LC_ALL=Cover the whole ofgenerate_branch_namealso changes the
grep -qwacronym probe, and that the extension's Python twinstill used a Unicode
\bthere. That was correct:Under
LC_ALL=Can accented letter is a non-word byte, soDBhas boundariesand survives; Python's
\band .NET's\bare Unicode-aware, treatéDBéasone word, and drop it. Only words shorter than three characters reach this
probe, so the window is narrow, but the twins genuinely disagreed.
The core Python twin had already settled this the ASCII way
(
scripts/python/create_new_feature.py:185, with a comment saying it mirrorsbash's
grep -qw). Three files were still using\band are now aligned to thesame explicit lookarounds:
extensions/git/scripts/python/create_new_feature_branch.pyextensions/git/scripts/powershell/create-new-feature-branch.ps1scripts/powershell/create-new-feature.ps1The last one is beyond the reported comment: it had the identical
\bagainst acore Python twin that already used ASCII lookarounds, so the same divergence
existed in core. Fixing only the extension would have left the two halves
inconsistent with each other.
Covered by
test_acronym_adjacent_to_non_ascii_matches_pythonin the extensionparity suite and an
acronym_next_to_non_asciicase intest_python_branch_name_generation_matches_bash.Testing
uv run specify --helpuv sync && uv run pytestFull suite on this branch:
7151 passed, 181 skipped, 1 failedon Linux /Python 3.14. On
mainthe same run is7136 passed, 181 skipped, 1 failed;this branch adds 15 test cases (7136 + 15 = 7151).
The one failure,
tests/contract/test_bundle_cli.py::test_build_escapes_markup_in_output_path,is unrelated and pre-existing on
main: it asserts across a Rich wrap point, soit passes or fails depending on how long the runner's temp path is. Fixed
separately in #4280.
Fail-before / pass-after, with the sources reverted and the new tests kept:
test_bash_branch_name_ignores_locale_collation(3 params)test_python_dash_prefixed_short_name_matches_bash(3 params)TestCreateFeatureBranchParity::test_branch_name_ignores_locale_collation(2 params)test_bash_collapses_repeated_separators(2 params)test_short_name_cleaning[repeated_separators, separator_run]The two separator tests are the macOS guard — on a GNU-sed runner they pass
either way, which is exactly why the gap survived. They will exercise the real
thing on the
macos-latestleg of the matrix.The locale tests probe
sedunderen_US.UTF-8and skip when the environmentcannot reproduce collation-ordered ranges (locale not installed, non-glibc libc,
Git-for-Windows), rather than asserting against a locale name that may not exist.
Manual test results
Agent: Claude Code | OS/Shell: Ubuntu 24.04.4, bash 5.2.21,
LANG=en_INScaffolded from this branch with
specify init <dir> --integration claude --extension git, so the project undertest carries the patched scripts.
/speckit-specify Ajouter la réservation hôtelière(accented)specs/001-hotel-booking/, branch001-hotel-booking; spec dir and branch agree/speckit-git-feature(same description)create-new-feature.sh --json --dry-run --short-name "réservation hôtelière"002-r-servation-h-teli-reextensions/git/.../create-new-feature-branch.sh --json --short-name "réservation hôtelière"002-r-servation-h-teli-rechecked outWorth noting for reviewers: in the two slash-command runs the agent summarised
the French description into an ASCII short name of its own ("hotel-booking"),
so those runs confirm the commands still work but do not reach the accented
path. The last two rows drive the scripts with a non-ASCII
--short-namedirectly, which is what the skills do internally, and that is where the
divergence used to appear.
en_INis a stock desktop locale and reproduces itexactly like
en_US.UTF-8.AI Disclosure
Code and tests generated with Claude Code. I reviewed the diff, reproduced the
locale divergence, ran the suite, and understand what the change does.