[SPEC] i18n / zh-CN locale support — product and technical spec#10990
[SPEC] i18n / zh-CN locale support — product and technical spec#10990ZacharyZcR wants to merge 12 commits into
Conversation
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Reviewed the product and technical specs for adding a custom i18n framework with zh-CN as the first non-English locale. The specs cover the main architecture, fallback model, locale file format, and broad rollout/testing intent.
Concerns
- Locale resolution is internally inconsistent and over-broad: the product and tech specs disagree on whether non-Chinese
WARP_LANGvalues override system locale, and allzh*locales are mapped to Simplified Chinese. - The technical macro example is not type-correct against the declared
Cow<'static, str>API, which would lead to a non-compiling implementation if followed literally. - The validation plan does not yet gate the core locale-file and callsite invariants needed for a 2,700-key migration.
Security
$WARP_LOCALES_DIRintroduces an external locale-file override without specifying its trust boundary, production availability, or integrity/validation requirements.
Verdict
Found: 0 critical, 5 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 3. If no locale can be determined, the application defaults to English (`en`). | ||
|
|
||
| 1.2. The resolved locale is matched as follows: | ||
| - Any locale starting with `"zh"` (e.g., `zh-CN`, `zh-TW`, `zh-HK`, `zh`) resolves to `zh-CN`. |
There was a problem hiding this comment.
zh* locale, including zh-TW and zh-HK, to Simplified Chinese makes Traditional Chinese users automatically see the wrong script. Limit auto-selection to zh, zh-CN, and zh-Hans*, or fall other Chinese variants back to English until their locale files exist.
|
|
||
| ### 2. Translation rendering | ||
|
|
||
| 2.1. Every user-facing text string in the Warp UI is wrapped in a translation call (`t!()` macro in Rust source). English strings serve as both the translation key and the fallback value — the lookup key is a dot-separated path into a YAML locale file. |
There was a problem hiding this comment.
💡 [SUGGESTION] This says English strings are lookup keys, but the examples use stable dot-separated keys and English strings as values. Rewrite this to distinguish keys from fallback display values so implementers do not key lookups by user-visible English copy.
|
|
||
| 5.2. To change locale, the user sets `WARP_LANG=zh-CN` (or `WARP_LANG=zh`) before launching Warp, and the change takes effect on next launch. | ||
|
|
||
| 5.3. Setting `WARP_LANG` to a non-Chinese value (or unsetting it) restores the system-locale-based default (English for non-Chinese systems). |
There was a problem hiding this comment.
WARP_LANG is the highest-priority override, WARP_LANG=en or another non-Chinese value should force English even on a Chinese system; the tech spec currently falls through to system locale instead. Specify whether non-Chinese overrides are honored or ignored.
| `load_dir` reads all `.yml`/`.yaml` files in a directory, parses them with `serde_yaml`, identifies the locale from the top-level key, and recursively flattens nested YAML into dot-separated keys via `flatten_value`. | ||
|
|
||
| **File discovery order** (non-WASM): | ||
| 1. `$WARP_LOCALES_DIR` env var (if set) |
There was a problem hiding this comment.
$WARP_LOCALES_DIR the first production load path lets any launcher-controlled environment replace strings in security-sensitive UI such as auth, billing, and permission dialogs. Restrict this override to dev/test builds or define trust, validation, and integrity requirements for external locale files.
| // Arm 2: Explicit interpolation | ||
| t!("terminal.hand_off", environment = name) | ||
| // Expands to: i18n::t("terminal.hand_off") | ||
| // .map(|v| i18n::interpolate(&v, &[("environment", format!("{}", name))])) |
There was a problem hiding this comment.
i18n::t() returns Cow<'static, str>, which has no .map() method, so the proposed macro shape would not compile. Specify a type-correct expansion for interpolation and fallback detection.
|
|
||
| ### Unit tests | ||
|
|
||
| - `warp_i18n` should have tests for: |
There was a problem hiding this comment.
PRODUCT.md: identical key sets across en.yml/zh-CN.yml, matching placeholder names, and every callsite key existing in en.yml. Add CI checks for these, otherwise missing or broken translations will silently ship behind the English fallback.
…oundary, locale integrity checks
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the product and technical specs for the i18n / zh-CN locale support proposal. The specs cover the intended architecture and user behavior, but several contradictions and validation gaps need to be resolved before implementation.
Concerns
- The keying model is internally inconsistent: the technical spec says English string values are canonical key identifiers while the API examples use dot-separated YAML paths.
- Locale resolution has conflicting behavior for non-Chinese
WARP_LANGvalues, and the currentzh*prefix match would map Traditional Chinese locales to Simplified Chinese. - Automated locale validation does not cover interpolation placeholder parity, which can silently break dynamic UI strings.
Security
- The locale file discovery trust boundary needs an explicit production enforcement mechanism so dev-only filesystem paths cannot be used by shipped builds to load untrusted YAML into startup parsing and UI rendering.
Verdict
Found: 1 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 3. If no locale can be determined, the application defaults to English (`en`). | ||
|
|
||
| 1.2. The resolved locale is matched as follows: | ||
| - Any locale starting with `"zh"` (e.g., `zh-CN`, `zh-TW`, `zh-HK`, `zh`) resolves to `zh-CN`. |
There was a problem hiding this comment.
zh-TW and zh-HK to Simplified Chinese by prefix will automatically show the wrong script for Traditional Chinese users. Narrow this to zh-CN/zh-Hans or explicitly justify that product behavior.
|
|
||
| 5.2. To change locale, the user sets `WARP_LANG=zh-CN` (or `WARP_LANG=zh`) before launching Warp, and the change takes effect on next launch. | ||
|
|
||
| 5.3. Setting `WARP_LANG` to a non-Chinese value (or unsetting it) restores the system-locale-based default (English for non-Chinese systems). |
There was a problem hiding this comment.
WARP_LANG is specified to force en, while this says it restores the system-locale default. Clarify whether only unsetting returns to system locale, or whether non-Chinese overrides are ignored.
| `load_dir` reads all `.yml`/`.yaml` files in a directory, parses them with `serde_yaml`, identifies the locale from the top-level key, and recursively flattens nested YAML into dot-separated keys via `flatten_value`. | ||
|
|
||
| **File discovery order** (non-WASM): | ||
| 1. `$WARP_LOCALES_DIR` env var (if set). **Trust boundary:** this path is intended for development and testing only. It is not used in production builds — the bundled resources path (priority 2) is the sole source of locale files in shipped binaries. No integrity validation is performed on locale files loaded from this path. If a file is malformed (e.g., invalid YAML, wrong locale key), it is silently skipped and the next discovery path is tried. |
There was a problem hiding this comment.
$WARP_LOCALES_DIR, $CARGO_MANIFEST_DIR, and $PWD discovery are compiled out or gated from shipped builds; otherwise untrusted locale YAML can be injected into startup parsing and UI text rendering.
|
|
||
| Both files share identical structure — 94 top-level YAML sections corresponding to UI areas: `menu`, `tab`, `workspace`, `auth`, `billing`, `settings`, `ai_settings_page`, `terminal`, `shared_session`, `common`, etc. | ||
|
|
||
| Each string value in `en.yml` serves as the canonical key identifier. The `zh-CN.yml` file has the same keys with Chinese translations. |
There was a problem hiding this comment.
🚨 [CRITICAL] This conflicts with the lookup model above and the callsite examples: string values cannot be canonical key identifiers if code calls t!("common.save"). Define dot-separated YAML paths as keys and en.yml values as fallback text, or update all examples to the intended keying scheme.
| ### Locale file integrity (automated) | ||
| - Every key present in `en.yml` must have a corresponding key in `zh-CN.yml`. A script or build-time check verifies this invariant — missing keys in `zh-CN.yml` should cause a CI failure. | ||
| - Both YAML files must parse successfully as valid YAML and produce the expected top-level locale key (`en:` / `zh-CN:`). | ||
| - No orphaned keys: every key referenced by a `t!()` call in the codebase must exist in `en.yml`. A static analysis script (e.g., `rg 't!\("([^"]+)"' --only-matching | sort -u` diffed against keys extracted from `en.yml`) should be runnable locally and in CI to catch callsite-locale drift. |
There was a problem hiding this comment.
t!() call arguments, so translations cannot silently drop dynamic values or render raw placeholders.
…ompile-time security gate
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the product and technical specs for adding a custom Warp i18n framework with zh-CN as the first non-English locale. The specs cover the main behavior, architecture, locale file format, and validation strategy, but several ambiguities would lead to inconsistent implementation if left unresolved.
Concerns
- The translation key model is internally inconsistent: the product spec says English strings serve as keys while the technical spec uses dot-path YAML keys.
WARP_LANGprecedence and fallback semantics conflict between sections, especially for non-Chinese overrides on Chinese-system machines.- The macro-sharing plan duplicates
t!()across crates without defining how the copies stay aligned.
Security
- The release trust boundary for locale files is not consistently enforced: dev and current-working-directory discovery paths remain listed after the spec says shipped builds should only load bundled resources.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ### 2. Translation rendering | ||
|
|
||
| 2.1. Every user-facing text string in the Warp UI is wrapped in a translation call (`t!()` macro in Rust source). English strings serve as both the translation key and the fallback value — the lookup key is a dot-separated path into a YAML locale file. |
There was a problem hiding this comment.
|
|
||
| 5.2. To change locale, the user sets `WARP_LANG=zh-CN` (or `WARP_LANG=zh`) before launching Warp, and the change takes effect on next launch. | ||
|
|
||
| 5.3. Setting `WARP_LANG` to a non-Chinese value (or unsetting it) restores the system-locale-based default (English for non-Chinese systems). |
There was a problem hiding this comment.
WARP_LANG is described both as forcing en and as restoring system-locale detection; specify whether WARP_LANG=en/fr overrides to English or is ignored.
| 3. `$CARGO_MANIFEST_DIR/../resources/bundled/locales` (and up to 4 ancestor levels) | ||
| 4. `$PWD/resources/bundled/locales` |
There was a problem hiding this comment.
CARGO_MANIFEST_DIR/PWD fallbacks behind debug_assertions too or a shipped app can load locale YAML from repo/CWD-controlled paths.
|
|
||
| The match guard `value if value == key` detects when `t()` returned the key itself (translation missing). In that case, the key string is returned as `Cow::Owned` — interpolation is skipped because there is no template to interpolate into. | ||
|
|
||
| A duplicate `t!()` macro exists in `crates/onboarding/src/lib.rs` so the onboarding binary can use translations without depending on the full `app` crate. |
There was a problem hiding this comment.
💡 [SUGGESTION] Duplicating the macro will let app and onboarding drift; define a shared exported macro or require a test that asserts both expansions stay identical.
…o sync, dev-path compile gate
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines a custom i18n framework plus zh-CN as the first non-English locale. The structure covers product behavior, implementation architecture, fallback behavior, and validation, but several requirements need tightening before implementation.
Concerns
WARP_LANGbehavior is internally inconsistent for non-Chinese overrides versus system-locale fallback.- The technical spec does not define the runtime/build behavior when no locale bundle can be loaded.
- Validation focuses on key and placeholder parity, but does not require review of security-sensitive translated UI copy.
Security
- Security-sensitive surfaces such as auth, billing, sharing, and agent UI need translation validation requirements so localized text cannot weaken warnings, consent, or permission semantics.
- Debug-only arbitrary locale loading is release-gated, but the spec should still bound file size/shape to avoid poisoned local environments causing excessive startup parsing.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| 5.2. To change locale, the user sets `WARP_LANG=zh-CN` (or `WARP_LANG=zh`) before launching Warp, and the change takes effect on next launch. | ||
|
|
||
| 5.3. Setting `WARP_LANG` to a non-Chinese value (or unsetting it) restores the system-locale-based default (English for non-Chinese systems). |
There was a problem hiding this comment.
WARP_LANG is specified to force en, while this says it restores the system-locale default. State separately that non-Chinese WARP_LANG forces English, and only unsetting WARP_LANG restores system-locale detection.
| return t; | ||
| } | ||
| } | ||
| // On WASM, use include_str!() instead of filesystem access |
There was a problem hiding this comment.
| ## Testing and validation | ||
|
|
||
| ### Locale file integrity (automated) | ||
| - Every key present in `en.yml` must have a corresponding key in `zh-CN.yml`. A script or build-time check verifies this invariant — missing keys in `zh-CN.yml` should cause a CI failure. |
There was a problem hiding this comment.
| | 3 | `$CARGO_MANIFEST_DIR/../resources/bundled/locales` (and up to 4 ancestor levels) | `#[cfg(debug_assertions)]` | | ||
| | 4 | `$PWD/resources/bundled/locales` | `#[cfg(debug_assertions)]` | | ||
|
|
||
| **Security:** Paths 1, 3, and 4 are compiled out of release binaries. This prevents shipped builds from loading arbitrary YAML from environment variables or the current working directory into the startup parsing and UI rendering pipeline. If a file loaded from a dev-only path is malformed (e.g., invalid YAML, wrong locale key), it is silently skipped and the next discovery path is tried — the application does not crash. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Dev builds still load arbitrary locale YAML from env/PWD-controlled paths. Add size, file-count, and nesting limits so a poisoned local environment cannot cause unbounded startup parsing or memory use.
…ity copy review, debug file size cap
|
/oz-review |
|
This is your last |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines the initial i18n framework and zh-CN rollout with product behavior, implementation architecture, locale-file layout, and validation strategy. The scope is appropriate for a spec-only PR, but several requirements need tightening before implementation.
Concerns
- Locale resolution is internally inconsistent: the product examples make non-Chinese
WARP_LANGan explicit English override, while the technical pipeline would fall through to the system locale. - The degraded no-locale mode would render raw keys even for auth, billing, sharing, and agent-consent UI, which can leave security-sensitive prompts unreadable.
- Interpolation explicitly has no escaping or trust-boundary rule, which is unsafe if interpolated values ever enter formatted/link-capable UI.
Security
- Require meaningful English fallback text, embedded fallback data, or fail-closed behavior for security-sensitive prompts when locale files cannot load.
- Define how interpolated dynamic values are escaped or constrained before rendering in any rich/formatted UI contexts.
- Ensure the manual security-sensitive translation checklist includes the exact agent-mode consent and data-handling key prefixes.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ``` | ||
|
|
||
| **Locale resolution pipeline** (`init_locale`): | ||
| 1. Check `WARP_LANG` env var → if set and starts with `"zh"`, use `zh-CN`. |
There was a problem hiding this comment.
WARP_LANG=fr should classify to en without checking the system locale, but this pipeline would fall through and select zh-CN on a Chinese OS. Specify whether non-zh WARP_LANG is an explicit English override or is ignored.
|
|
||
| **Interpolation** (`interpolate`): | ||
| - Simple `str::replace` of `{name}` placeholders with provided values. | ||
| - Not a template engine — no escaping, no positional arguments, no format specifiers. |
There was a problem hiding this comment.
|
|
||
| **Security:** Paths 1, 3, and 4 are compiled out of release binaries. This prevents shipped builds from loading arbitrary YAML from environment variables or the current working directory into the startup parsing and UI rendering pipeline. In debug builds, locale files loaded from dev-only paths are subject to a size cap (8 MB per file) to prevent intentionally large or malformed YAML from causing excessive startup parsing in poisoned local environments. If a file loaded from a dev-only path exceeds the cap or is malformed, it is silently skipped and the next discovery path is tried — the application does not crash. | ||
|
|
||
| **No-locale fallback:** If no locale file can be loaded from any discovery path (e.g., corrupt installation, missing resource directory), `init_locale()` still completes successfully. The translation map remains empty, and every `t!()` call returns its raw key string as the rendered text. The application starts and functions normally — menu items, buttons, and labels display their dot-path keys (e.g., `"menu.file"`, `"common.save"`) instead of English text. This degraded mode ensures the application never fails to launch due to missing locale data. |
There was a problem hiding this comment.
| ### Security-sensitive translation review (manual) | ||
| - Strings in security-relevant UI surfaces must be manually reviewed by a maintainer before merge. These surfaces include: auth dialogs (sign-in, sign-up, permission prompts), billing pages (pricing, plan descriptions, credit consumption warnings), sharing/permission controls (access grant text, visibility labels), and agent-mode consent prompts (data-handling disclosures, handoff confirmations). | ||
| - The reviewer verifies that Chinesetranslations preserve the legal and behavioral intent of the original English text — warnings are not weakened, consent semantics are not altered, and permission descriptions remain accurate. | ||
| - A checklist of security-sensitive keys (identified by YAML section prefix: `auth.*`, `billing.*`, `teams.*`, `shared_session.*`, `hoa_onboarding.*`) is maintained alongside the locale files for targeted review. |
There was a problem hiding this comment.
…ove fallthrough ambiguity
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the product and technical specs for adding Warp i18n support with zh-CN as the first non-English locale. The specs define the desired user behavior and a plausible implementation direction, but several details need to be resolved before implementation to avoid incorrect locale detection, incomplete UI coverage, and unverifiable release criteria.
Concerns
- Locale classification does not cover common real-world locale strings such as encoded Linux locale values and
LANGUAGEpreference lists. - The migration scope is narrower in the technical plan than the product requirement for the entire UI surface.
- CI/build validation and cross-platform manual verification are not concrete enough for a platform-sensitive feature that depends on bundled resources and system locale APIs.
Security
- Security-sensitive fallback behavior is stated as a policy, but the spec does not require an enforceable API, lint, or CI check to keep auth, billing, permission, and agent-consent surfaces from accidentally using the raw-key fallback.
Verdict
Found: 0 critical, 5 important, 2 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 4. Default: `"en"` | ||
|
|
||
| The candidate is then classified: | ||
| - `zh`, `zh-CN`, `zh_CN`, `zh-Hans*`, or `zh_Hans*` → `"zh-CN"` |
There was a problem hiding this comment.
zh_CN.UTF-8, zh_CN.utf8, case variants, modifiers, and LANGUAGE=zh_CN:en_US; otherwise zero-config Linux detection can incorrectly fall back to English.
|
|
||
| **Security:** Paths 1, 3, and 4 are compiled out of release binaries. This prevents shipped builds from loading arbitrary YAML from environment variables or the current working directory into the startup parsing and UI rendering pipeline. In debug builds, locale files loaded from dev-only paths are subject to a size cap (8 MB per file) to prevent intentionally large or malformed YAML from causing excessive startup parsing in poisoned local environments. If a file loaded from a dev-only path exceeds the cap or is malformed, it is silently skipped and the next discovery path is tried — the application does not crash. | ||
|
|
||
| **No-locale fallback:** If no locale file can be loaded from any discovery path (e.g., corrupt installation, missing resource directory), `init_locale()` still completes successfully. For ordinary non-sensitive UI, the translation map remains empty and `t!()` returns the raw key string as the rendered text. Security-sensitive UI is different: auth, billing, sharing/permission, and agent-consent surfaces must not render raw dot-path keys. Those call sites must either use embedded English fallback strings that do not depend on locale files, or fail closed by disabling the affected action and showing a readable English error. |
There was a problem hiding this comment.
t_secure!(key, fallback) API, compile-time lint, or CI check to ensure auth/billing/permission/consent call sites use embedded English fallbacks or fail closed.
|
|
||
| ### 4. Usage patterns in the codebase | ||
|
|
||
| All user-facing string literals are replaced with `t!()` calls. ~4,700+ callsites across `app/src/`. Key patterns: |
There was a problem hiding this comment.
app/src/, but PRODUCT.md requires the entire UI and this tech spec also mentions onboarding. Specify every crate, binary, resource, and exception in scope so visible English is not left outside app/src/.
| - A checklist of security-sensitive keys is maintained alongside the locale files for targeted review. It must include exact YAML sections and prefixes for auth, billing, sharing, agent consent, and data-handling surfaces: `auth.*`, `billing.*`, `billing_ext.*`, `teams.*`, `privacy.*`, `shared_session.*`, `agent_management.*`, `hoa_onboarding.*`, `ai_ext.grant_access_files`, `ai_ext.grant_access_repository`, `ai_ext.missing_github_auth`, `ai_ext.authenticate_github`, `ai_ext.hand_off_to_cloud`, `ai_ext.handoff_to_cloud`, `ai_ext.hand_off_to_environment`, `ai_output.manage_ai_autonomy_permissions`, `ai_output.read_mcp_resource_permission`, `ai_output.upload_artifact_permission`, `ai_output.manage_agent_permissions`, and `ai_output.use_computer_permission`. When new permission, handoff, data-retention, or account/billing warning keys are added, the checklist must be updated in the same PR. | ||
|
|
||
| ### Locale file integrity (automated) | ||
| - Every key present in `en.yml` must have a corresponding key in `zh-CN.yml`. A script or build-time check verifies this invariant — missing keys in `zh-CN.yml` should cause a CI failure. |
There was a problem hiding this comment.
|
|
||
| ### Integration / manual verification | ||
|
|
||
| - Launch Warp with `WARP_LANG=zh-CN` on macOS and verify: |
There was a problem hiding this comment.
WARP_LANG, system-locale detection, and resource loading.
| } | ||
| ``` | ||
|
|
||
| `load_dir` reads all `.yml`/`.yaml` files in a directory, parses them with `serde_yaml`, identifies the locale from the top-level key, and recursively flattens nested YAML into dot-separated keys via `flatten_value`. |
There was a problem hiding this comment.
💡 [SUGGESTION] Define how flatten_value handles non-string leaves, duplicate YAML keys, and flattening collisions like menu.file both as a scalar and nested path; otherwise malformed locale files can produce ambiguous runtime translations.
|
|
||
| - **Language picker in Settings:** Should a future iteration add a UI language selector in the Appearance or General settings? This would require runtime locale switching without restart. | ||
| - **Contributor workflow:** How should translators contribute locale files for additional languages? A CONTRIBUTING guide for new locale YAML files may be needed. | ||
| - **String extraction:** Should locale keys be auto-generated from source code, or manually authored in YAML? The current approach is manual YAML authorship. |
There was a problem hiding this comment.
💡 [SUGGESTION] Resolve the string extraction workflow before implementation; with ~4,700 callsites and ~2,700 keys, leaving auto-generation vs manual authoring open makes key naming, review ownership, and drift prevention ambiguous.
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines product behavior and a technical plan for adding Warp i18n with zh-CN as the first non-English locale. The overall scope is well bounded, but several requirements need to be tightened before implementation because they affect locale resolution behavior, API feasibility, release packaging, and security validation.
Concerns
- Locale fallback semantics are ambiguous for unsupported candidates outside
WARP_LANG, which can produce inconsistent behavior betweenLANGUAGE, other environment variables, and system locale detection. - The proposed
interpolatereturn type does not match the lifetimes implied by the macro examples. - Release locale discovery does not name the actual per-platform resource paths the implementation and bundle checks must share.
Security
- Parsed/rich text interpolation is called out as requiring escaping, but the validation plan does not require an audit or lint for those call sites, leaving agent/server-controlled labels and paths vulnerable to markup/link injection if implemented as written.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| - Strip encoding and modifier suffixes after `.` or `@` (for example, `.UTF-8`, `.utf8`, `@pinyin`). | ||
| - Treat `_` and `-` as equivalent separators, and compare language/script/region subtags case-insensitively. | ||
|
|
||
| The first supported normalized candidate is then classified: |
There was a problem hiding this comment.
WARP_LANG candidates: this says classify the first supported candidate, the next bullet maps everything else to en, and the LANGUAGE=fr:zh_CN:en example skips fr. Specify whether unsupported entries in LANGUAGE and lower-priority env vars are skipped or force English before implementation.
| pub fn set_locale(locale: &str) // Force a specific locale | ||
| pub fn t(key: &'static str) -> Cow<'static, str> // Primary translation lookup | ||
| pub fn t_required(key: &'static str, fallback: &'static str) -> Cow<'static, str> | ||
| pub fn interpolate(template: &str, args: &[(&str, String)]) -> Cow<'static, str> |
There was a problem hiding this comment.
'static templates. Make interpolate lifetime-parametric (Cow<'a, str>) or require it to always return an owned String/Cow::Owned, then update the macro examples accordingly.
| - Simple `str::replace` of `{name}` placeholders with provided values. | ||
| - Not a template engine — no escaping, no positional arguments, no format specifiers. | ||
| - Values are pre-formatted to `String` by the `t!()` macro before reaching `interpolate`. | ||
| - Interpolated values are treated as plain text. Call sites that render into rich, Markdown, link-capable, or otherwise parsed UI must pass interpolated values through the appropriate escaping layer or build the rich UI from structured fragments rather than by concatenating formatted strings. File paths, branch names, agent-provided labels, and server-provided metadata must never be allowed to inject markup, links, or formatting through translation interpolation. |
There was a problem hiding this comment.
| | Priority | Path | Gate | | ||
| |----------|------|------| | ||
| | 1 | `$WARP_LOCALES_DIR` env var | `#[cfg(debug_assertions)]` | | ||
| | 2 | Platform bundled resources: `<exe_dir>/resources/bundled/locales` | Always enabled | |
There was a problem hiding this comment.
<exe_dir>/resources/bundled/locales is too vague for macOS app bundles and packaged Linux/Windows layouts. Specify the exact release resource paths per platform that locale_dirs() will search so the bundle check validates the same paths the binary uses.
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines a custom i18n framework, zh-CN as the first translated locale, locale resolution, fallback behavior, migration scope, and validation expectations. The broad direction is clear, but several requirements are not specific enough to safely guide implementation across the full UI surface.
Concerns
- The rich/parsed-text interpolation mitigation names a risk but does not define the concrete escaping or structured-rendering APIs needed to make the security requirement testable.
- The migration and extraction scope is narrower than the product promise of the full Warp UI surface.
- The duplicated macro approach depends on manual synchronization for behavior that includes security-sensitive fallback semantics.
- Locale parity checks do not prove zh-CN coverage because empty or copied-English translations could pass.
Security
- Parsed/rich text interpolation needs an explicit approved escaping or structured-fragment strategy per renderer before implementation, otherwise attacker-controlled labels, paths, repository names, or server metadata can be interpolated into Markdown/link-capable UI inconsistently.
Verdict
Found: 0 critical, 4 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| - Not a template engine — no escaping, no positional arguments, no format specifiers. | ||
| - Values are pre-formatted to `String` by the `t!()` macro before reaching `interpolate`. | ||
| - `interpolate()` returns an owned `String`; the macros wrap it in `Cow::Owned(...)`. This avoids returning a borrowed value tied to a temporary `Cow` from `t()` or `t_required()`. | ||
| - Interpolated values are treated as plain text. Call sites that render into rich, Markdown, link-capable, or otherwise parsed UI must pass interpolated values through the appropriate escaping layer or build the rich UI from structured fragments rather than by concatenating formatted strings. File paths, branch names, agent-provided labels, and server-provided metadata must never be allowed to inject markup, links, or formatting through translation interpolation. |
There was a problem hiding this comment.
appropriate escaping layer is not implementable or testable without naming the approved escaping or structured-fragment API for each parsed renderer; define those APIs and require tests/lints so attacker-controlled paths, branch names, agent labels, and server metadata cannot inject Markdown, links, or formatting.
|
|
||
| The required macro expands through `i18n::t_required(key, fallback)`, then applies interpolation to the translated value or embedded fallback and wraps interpolated output with `Cow::Owned(...)`. It must never branch back to the raw key. The fallback argument must be a string literal so static analysis can verify that every security-sensitive call site has readable English text even when locale files are missing. | ||
|
|
||
| Duplicate `t!()` and `t_required!()` macros exist in `crates/onboarding/src/lib.rs` so the onboarding binary can use translations without depending on the full `app` crate. The macro copies are structurally identical — same call shapes, same fallback logic, same interpolation semantics. The only difference is the function reference: `$crate::i18n::*` in the app crate vs `warp_i18n::*` in onboarding. When changes to either macro are needed, both copies must be updated in the same commit. A code comment at each macro location references the other copy's file path to prevent drift. |
There was a problem hiding this comment.
t!() and t_required!() with only a comment is not an enforceable drift control; either move the macros into a shared crate/export or add CI/compile-time parity checks so onboarding cannot diverge from app fallback and interpolation semantics.
|
|
||
| ### 4. Usage patterns in the codebase | ||
|
|
||
| All user-facing string literals are replaced with `t!()` or `t_required!()` calls. ~4,700+ callsites across `app/src/`. Key patterns: |
There was a problem hiding this comment.
app/src/, but the product spec requires the whole Warp UI surface; define the complete extractor roots/crate list, including non-app/src UI crates and resource-generated strings, so implementation cannot leave user-visible strings untranslated outside this path.
| - Every key in that checklist must be reached through `t_required!()` or an explicit fail-closed path. A CI lint rejects security-sensitive keys rendered through plain `t!()` and rejects `t_required!()` calls whose fallback is not a string literal. | ||
|
|
||
| ### Locale file integrity (automated) | ||
| - Every key present in `en.yml` must have a corresponding key in `zh-CN.yml`. A script or build-time check verifies this invariant — missing keys in `zh-CN.yml` cause a CI failure. |
There was a problem hiding this comment.
|
|
||
| - **Language picker in Settings:** Should a future iteration add a UI language selector in the Appearance or General settings? This would require runtime locale switching without restart. | ||
| - **Contributor workflow:** How should translators contribute locale files for additional languages? A CONTRIBUTING guide for new locale YAML files may be needed. | ||
| - **String extraction:** Should locale keys be auto-generated from source code, or manually authored in YAML? The current approach is manual YAML authorship. |
There was a problem hiding this comment.
💡 [SUGGESTION] This is already decided by TECH.md as manual YAML authorship plus static extraction; move it to follow-up work or update the technical spec if key generation is still unresolved.
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec PR defines a custom i18n framework, zh-CN locale behavior, fallback semantics, validation checks, and platform verification for adding Simplified Chinese support. The product requirements are generally concrete, and the technical spec covers migration scope, security-sensitive copy handling, and release packaging.
Concerns
- The locale loading algorithm does not define completeness criteria before a directory is accepted, so a partial dev or bundled directory could silently shadow complete fallback resources.
- The key-extraction validation example is not robust enough for the multiline macro forms the spec itself allows, which can leave CI blind to missing locale keys in security-sensitive call sites.
Security
- Runtime YAML parsing is bounded only for dev-only paths; the spec should also bound or explicitly trust release-bundled locale resources to avoid startup DoS from corrupted packages or installations.
Verdict
Found: 0 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ```rust | ||
| fn load_translations() -> Translations { | ||
| for dir in locale_dirs() { // Try multiple filesystem paths | ||
| if let Some(t) = load_dir(dir) { // First successful directory wins |
There was a problem hiding this comment.
en.yml or a malformed zh-CN.yml can be treated as the first successful directory and silently disable zh-CN instead of falling through to bundled resources.
| | Linux app packages/AppImage | `/opt/warpdotdev/<package>/resources/bundled/locales` inside the package/AppDir | | ||
| | Linux CLI artifact | `<bundle-output>/resources/bundled/locales` adjacent to the CLI binary bundle | | ||
|
|
||
| **Security:** Paths 1, 3, and 4 are compiled out of release binaries. This prevents shipped builds from loading arbitrary YAML from environment variables or the current working directory into the startup parsing and UI rendering pipeline. In debug builds, locale files loaded from dev-only paths are subject to a size cap (8 MB per file) to prevent intentionally large or malformed YAML from causing excessive startup parsing in poisoned local environments. If a file loaded from a dev-only path exceeds the cap or is malformed, it is silently skipped and the next discovery path is tried — the application does not crash. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Apply a bounded read/parse limit to release-bundled locale files too, or explicitly document why those resources are trusted. Runtime serde_yaml parsing of unbounded files at startup leaves corrupted packages or installations with a cheap DoS path.
| - Every `zh-CN.yml` value must be non-empty after trimming whitespace, unless the key is listed in a checked-in empty-value allowlist with a reason. | ||
| - **Interpolation placeholder parity:** for every key whose English value contains `{name}` placeholders, the zh-CN value must contain the exact same set of placeholder names (same count, same names). Mismatched placeholder names (e.g., en has `{count}` but zh-CN has `{number}`) produce runtime rendering bugs and must be rejected at CI time. | ||
| - Both YAML files must parse successfully as valid YAML and produce the expected top-level locale key (`en:` / `zh-CN:`). | ||
| - No orphaned keys: every key referenced by a `t!()` or `t_required!()` call in the codebase must exist in `en.yml`. A static analysis script (e.g., `rg 't(_required)?!\("([^"]+)"' --only-matching | sort -u` diffed against keys extracted from `en.yml`) must run locally and in CI to catch callsite-locale drift. |
There was a problem hiding this comment.
t_required! form shown above, so CI can fail to detect missing en.yml keys for exactly the security-sensitive calls this spec requires.
|
/oz-review |
|
This is your last |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines a comprehensive i18n framework for Warp with zh-CN as the first non-English locale, including product behavior, runtime lookup semantics, locale file integrity checks, packaging validation, and security-sensitive fallback handling.
Concerns
- The locale detection order can make ordinary POSIX locale environment variables override the native OS language, which conflicts with the zero-configuration detection goal on desktop launches.
- The proposed macro expansion uses returned string equality with the key as the missing-translation sentinel, which makes lookup state ambiguous and can break valid translations.
- Security-sensitive embedded fallbacks are duplicated outside
en.ymlwithout a required drift check.
Security
t_required!()fallback literals must be validated against canonical English locale entries or intentionally approved exceptions so auth, billing, sharing, and agent-consent copy cannot diverge silently.- The spec introduces startup YAML parsing via
serde_yaml; it should state the dependency maintenance/pinning posture or selected parser constraints as part of the supply-chain review.
Verdict
Found: 0 critical, 3 important, 2 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| 1.1. The active locale is always exactly `"zh-CN"` or `"en"`. These are the only two supported locales. The determination is: | ||
|
|
||
| Candidate locale strings are selected from the first available source: |
There was a problem hiding this comment.
LANGUAGE/LC_*/LANG force English before the native OS locale is consulted, so a zh-CN desktop user with inherited LANG=en_US.UTF-8 or C will not get zero-config localization. Specify platform-specific precedence or make only WARP_LANG authoritative over the native locale.
|
|
||
| ### 1. New crate: `warp_i18n` | ||
|
|
||
| A new workspace crate at `crates/i18n/` with dependencies `serde_yaml` (for parsing YAML) and `sys-locale` (for OS-level locale detection). |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] This adds a runtime YAML parser dependency but does not state the dependency maintenance, pinning, or parser choice criteria. Document why the selected parser is acceptable for startup resource parsing and how dependency updates/vulnerability response will be handled.
| |-------------------|-------------------| | ||
| | Plain UI text, labels, buttons, menus, tooltips, toasts without links | Use `t!()` / `t_required!()` directly; interpolation values are plain text. | | ||
| | Formatted text fragments | Build structured fragments and pass dynamic values only through plain-text fragment constructors such as `FormattedTextFragment::plain_text(...)`; do not concatenate dynamic values into markup-bearing fragments. | | ||
| | Markdown body text | Escape dynamic values with `warp_i18n::escape_markdown_text(...)` before interpolation, or avoid interpolation and insert the dynamic value as a plain-text fragment after Markdown parsing. | |
There was a problem hiding this comment.
💡 [SUGGESTION] The rich-rendering contract references escape_markdown_text and escape_markdown_link_text, but the public API and tests do not include those helpers. Add them to the API/testing sections so implementation and lint recognition are unambiguous.
| } | ||
| ``` | ||
|
|
||
| The match guard `value if value == key` detects when `t()` returned the key itself (translation missing). In that case, the key string is returned as `Cow::Owned` — interpolation is skipped because there is no template to interpolate into. |
There was a problem hiding this comment.
value == key is ambiguous: a legitimate English or zh-CN value can equal the key, and interpolated strings whose resolved template equals the key will skip interpolation. Define a lookup result that carries found/missing status instead of using string equality as the sentinel.
| ) | ||
| ``` | ||
|
|
||
| The required macro expands through `$crate::t_required(key, fallback)`, then applies interpolation to the translated value or embedded fallback and wraps interpolated output with `Cow::Owned(...)`. It must never branch back to the raw key. The fallback argument must be a string literal so static analysis can verify that every security-sensitive call site has readable English text even when locale files are missing. |
There was a problem hiding this comment.
en.yml text for auth, billing, sharing, and agent-consent keys. Require CI to compare each t_required!() fallback against the matching en.yml value, with explicit owner-approved exceptions, so no-locale fallback preserves the reviewed security/legal wording.
|
/oz-review |
|
You've used all 5 |
1 participant
Description
Product and technical specifications for adding i18n support to Warp with Simplified Chinese (zh-CN) as the first non-English locale. This PR contains only the spec documents (no code).
Closes #1194
Documents
specs/gh-1194/PRODUCT.md— User-facing behavior spec: locale detection, translation rendering, interpolation, fallback strategy, locale file format, and platform consistency requirements. Written as numbered, testable invariants.specs/gh-1194/TECH.md— Implementation plan: customwarp_i18ncrate architecture,t!()macro design, YAML loading pipeline, locale resolution logic, usage patterns in the codebase (~4,700 callsites, 2,732 keys), and testing strategy.What this covers
The i18n framework is a custom, lightweight system:
resources/bundled/locales/OnceLockWARP_LANGenv var → system locale → English fallback{key}interpolationCHANGELOG-NEW-FEATURE: Add i18n framework with Simplified Chinese (zh-CN) locale support (spec)