-
Notifications
You must be signed in to change notification settings - Fork 295
fix(terminal): inherit the host UTF-8 locale instead of forcing en_US.UTF-8 #1713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import { getUtf8LocaleEnv } from "../localeEnv" | ||
|
|
||
| describe("getUtf8LocaleEnv", () => { | ||
| it("falls back to en_US.UTF-8 when the host provides no locale at all", () => { | ||
| expect(getUtf8LocaleEnv({})).toEqual({ LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8" }) | ||
| }) | ||
|
|
||
| it("falls back when the host only provides empty locale variables", () => { | ||
| expect(getUtf8LocaleEnv({ LANG: "", LC_ALL: "", LC_CTYPE: "" })).toEqual({ | ||
| LANG: "en_US.UTF-8", | ||
| LC_ALL: "en_US.UTF-8", | ||
| }) | ||
| }) | ||
|
|
||
| it("falls back for ASCII locales such as C and POSIX", () => { | ||
| expect(getUtf8LocaleEnv({ LANG: "C" })).toEqual({ LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8" }) | ||
| expect(getUtf8LocaleEnv({ LC_ALL: "POSIX" })).toEqual({ LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8" }) | ||
| }) | ||
|
|
||
| it("leaves an inherited UTF-8 LANG untouched (#1084)", () => { | ||
| expect(getUtf8LocaleEnv({ LANG: "en_AU.UTF-8" })).toEqual({}) | ||
| expect(getUtf8LocaleEnv({ LANG: "en_GB.UTF-8", PATH: "/usr/bin" })).toEqual({}) | ||
| }) | ||
|
|
||
| it("leaves an inherited UTF-8 LC_ALL untouched", () => { | ||
| expect(getUtf8LocaleEnv({ LC_ALL: "de_DE.UTF-8" })).toEqual({}) | ||
| }) | ||
|
|
||
| it("accepts the utf8 spelling and LC_CTYPE as a UTF-8 signal", () => { | ||
| expect(getUtf8LocaleEnv({ LANG: "en_AU.utf8" })).toEqual({}) | ||
| expect(getUtf8LocaleEnv({ LC_CTYPE: "zh_CN.UTF-8" })).toEqual({}) | ||
| }) | ||
|
|
||
| it("honors the POSIX precedence order, where LC_ALL wins over LANG", () => { | ||
| expect(getUtf8LocaleEnv({ LANG: "en_AU.UTF-8", LC_ALL: "POSIX" })).toEqual({ | ||
| LANG: "en_US.UTF-8", | ||
| LC_ALL: "en_US.UTF-8", | ||
| }) | ||
| }) | ||
|
Comment on lines
+34
to
+39
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Add an The tests verify 🤖 Prompt for AI AgentsSource: Path instructions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 4221563. Valid again, same class as the previous one - one precedence level up. The lookup it("honors the POSIX precedence order, where LC_ALL wins over LC_CTYPE", () => {
// LC_ALL overrides the category variables, so a UTF-8 LC_CTYPE must not
// rescue an ASCII LC_ALL.
expect(getUtf8LocaleEnv({ LC_ALL: "POSIX", LC_CTYPE: "de_DE.UTF-8" })).toEqual({
LANG: "en_US.UTF-8",
LC_ALL: "en_US.UTF-8",
})
})Mutation control (hoisting Reverted afterwards; worktree clean. With this case plus the Implementation untouched, spec
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
||
|
|
||
| it("honors the POSIX precedence order, where LC_CTYPE wins over LANG", () => { | ||
| // Guards the resolution order: reading LANG before LC_CTYPE would wrongly | ||
| // treat the ASCII LC_CTYPE as an inherited UTF-8 locale. | ||
| expect(getUtf8LocaleEnv({ LANG: "en_AU.UTF-8", LC_CTYPE: "POSIX" })).toEqual({ | ||
| LANG: "en_US.UTF-8", | ||
| LC_ALL: "en_US.UTF-8", | ||
| }) | ||
| }) | ||
|
|
||
| it("honors the POSIX precedence order, where LC_ALL wins over LC_CTYPE", () => { | ||
| // LC_ALL overrides the category variables, so a UTF-8 LC_CTYPE must not | ||
| // rescue an ASCII LC_ALL. | ||
| expect(getUtf8LocaleEnv({ LC_ALL: "POSIX", LC_CTYPE: "de_DE.UTF-8" })).toEqual({ | ||
| LANG: "en_US.UTF-8", | ||
| LC_ALL: "en_US.UTF-8", | ||
| }) | ||
| }) | ||
|
|
||
| it("returns a fresh object so callers cannot share and mutate the override", () => { | ||
| const first = getUtf8LocaleEnv({}) | ||
| first.LANG = "mutated" | ||
|
|
||
| expect(getUtf8LocaleEnv({})).toEqual({ LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8" }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * Locale environment handling for commands spawned by Zoo Code. | ||
| * | ||
| * Commands used to be spawned with a hardcoded `LANG`/`LC_ALL` of `en_US.UTF-8` so that | ||
| * tools such as Ruby and CocoaPods always emit UTF-8. Overriding the locale | ||
| * unconditionally is harmful on hosts where `en_US.UTF-8` is not generated: every | ||
| * command then prints `setlocale: LC_ALL: cannot change locale (en_US.UTF-8)`, and | ||
| * locale-sensitive tools behave as if the machine were US English. | ||
| */ | ||
|
|
||
| /** UTF-8 locale used when (and only when) the host does not provide one of its own. */ | ||
| const FALLBACK_UTF8_LOCALE = "en_US.UTF-8" | ||
|
Check warning on line 12 in src/integrations/terminal/localeEnv.ts
|
||
|
|
||
| /** | ||
| * Resolves the effective locale of a process environment. | ||
| * | ||
| * POSIX resolves the locale from the first non-empty value of `LC_ALL`, `LC_CTYPE` and | ||
| * `LANG`, so the effective locale is what matters here, not the individual variables. | ||
| */ | ||
| function getEffectiveLocale(env: NodeJS.ProcessEnv): string { | ||
| return env.LC_ALL || env.LC_CTYPE || env.LANG || "" | ||
|
Check warning on line 21 in src/integrations/terminal/localeEnv.ts
|
||
| } | ||
|
|
||
| /** Whether a locale string selects a UTF-8 codeset (accepts the `UTF-8` and `utf8` spellings). */ | ||
| function isUtf8Locale(locale: string): boolean { | ||
| return /utf-?8/i.test(locale) | ||
| } | ||
|
|
||
| /** | ||
| * Returns the locale overrides to merge into the environment of a spawned command. | ||
| * | ||
| * A host that already resolves to a UTF-8 locale is left untouched so that commands | ||
| * inherit the user's locale; only a host without any UTF-8 locale receives the UTF-8 | ||
| * fallback that the historical hardcoded override was meant to provide. | ||
| */ | ||
| export function getUtf8LocaleEnv(env: NodeJS.ProcessEnv = process.env): { LANG?: string; LC_ALL?: string } { | ||
| if (isUtf8Locale(getEffectiveLocale(env))) { | ||
| return {} | ||
| } | ||
|
|
||
| return { LANG: FALLBACK_UTF8_LOCALE, LC_ALL: FALLBACK_UTF8_LOCALE } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 59534c7 (rebased onto
main@08d05eb).Valid catch. The resolution order in
getEffectiveLocale()was alreadyLC_ALL || LC_CTYPE || LANG, so the behaviour was right, but nothing pinnedLC_CTYPEagainstLANG— the existing precedence case only coveredLC_ALLvsLANG, so reordering the two would indeed have slipped through. Added the requested case:Mutation control, to show the new case guards the order rather than merely restating the outcome: with the lookup swapped to
LC_ALL || LANG || LC_CTYPE, exactly this one test fails (Tests 1 failed | 8 passed (9),expected {} to deeply equal { LANG: 'en_US.UTF-8', … }) while theLC_ALLcase still passes — i.e. precisely the gap you described. With the correct order the spec isTest Files 1 passed | Tests 9 passed (9).The branch is still a single commit and the surrounding suites are unchanged (targeted specs
2 passed / 28 passed,eslint/prettier/tscclean).