Repository navigation
feat: add locale and ICU correctness core - #3
Conversation
Canonicalize locale identity across configuration, state, translation memory, and policy hashes. Parse and structurally validate ICU messages before provider or translation-memory output is persisted. Signed-off-by: Tom Main <tom@execufunction.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78cc535d7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !p.consume('{') { | ||
| return nil, p.errorf("expected message branch for selector %q", selector) | ||
| } | ||
| branch, err := p.parseBranch(argumentType == ArgumentPlural || argumentType == ArgumentSelectOrdinal) |
There was a problem hiding this comment.
When a select is nested inside a plural or selectordinal branch, # within the select's branches still refers to the enclosing plural. This call resets pluralContext to false for every select, so those placeholders are parsed as ordinary text and excluded from poundCount; for example, a provider can remove # from {n, plural, other {{g, select, male {# men} other {# people}}}} and the output will pass validation and be written. Pass the enclosing plural context through select branches while allowing a nested plural to establish its own context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3819472. The parser and deterministic printer now carry the enclosing plural context through nested select branches, and TestComparePreservesPluralPoundThroughNestedSelect covers the reported removal case.
| func isIdentifierByte(value byte) bool { | ||
| return value == '_' || value == '-' || value == '.' || value >= '0' && value <= '9' || value >= 'A' && value <= 'Z' || value >= 'a' && value <= 'z' |
There was a problem hiding this comment.
When a valid ICU message uses a non-ASCII argument name such as {имя}, this ASCII-only predicate causes LooksLike to return false, so a source consisting of that argument is skipped entirely by ICU validation and the provider may remove it without triggering either ICU or legacy interpolation checks. If the message also contains a detectable ASCII argument, parsing instead rejects the valid source. Tokenize ICU identifiers as Unicode code points according to ICU identifier rules rather than individual ASCII bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3819472. ICU identifiers now decode Unicode code points and apply Unicode Pattern_Syntax/Pattern_White_Space boundaries while preserving the existing ASCII compatibility set. TestCompareSupportsUnicodeArgumentNames covers detection, comparison, and parse/print stability.
Carry enclosing plural context through nested select branches and accept Unicode ICU argument identifiers. Add regressions for both write-boundary validation gaps. Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3819472791
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _, sourceHasSelector := sourceOptions[selector]; !sourceHasSelector && !isExactSelector(selector) { | ||
| issues = append(issues, compareMessages(sourceOther, targetBranch, targetLocale, name)...) |
There was a problem hiding this comment.
When a provider adds an exact branch that was not present in the source, this condition skips the branch completely. For example, {n, plural, other {# items}} can become {n, plural, =0 {{price, number}} other {# articles}} without an ICU finding; the legacy interpolation check also misses typed arguments, so the translation can introduce a runtime argument that callers never supply. Exact branches may legitimately omit source placeholders, but they still need validation that they do not introduce unsupported arguments or incompatible nested structures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f81a969. Target-only exact branches now allow omission of source fallback placeholders while requiring every target argument occurrence to have a structurally compatible source-fallback counterpart. Added a regression for the reported typed price argument.
| func isExactSelector(selector string) bool { | ||
| if !strings.HasPrefix(selector, "=") || len(selector) == 1 { | ||
| return false | ||
| } | ||
| _, err := strconv.ParseFloat(selector[1:], 64) | ||
| return err == nil |
There was a problem hiding this comment.
strconv.ParseFloat accepts values such as NaN, Inf, and Infinity, so messages containing selectors like =NaN are treated as syntactically valid exact-number selectors and can pass source/provider validation even though they are not valid ICU numeric selectors. Validate the selector against the ICU number-literal grammar and reject non-finite values before accepting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f81a969. Exact selectors now use a finite decimal grammar instead of ParseFloat, rejecting NaN, infinities, hexadecimal floats, and incomplete decimals while retaining integer, negative, and decimal selectors. Added positive and negative parser tests.
| func hasCompactExponentMany(tag language.Tag) bool { | ||
| base, _ := tag.Base() | ||
| switch base.String() { | ||
| case "ca", "es", "fr", "gl", "it", "pt", "scn", "vec": | ||
| return true |
There was a problem hiding this comment.
For the valid lld locale, modern CLDR defines one, many, and other cardinal categories and many/other ordinal categories, but Ladin is absent from both this compact-exponent overlay and needsCurrentOrdinalMany. Because the underlying evaluator is documented here as using older CLDR data, CardinalCategories("lld") and OrdinalCategories("lld") omit many; i18next-v4 consequently fails to generate a required _many key, while ICU validation rejects a legitimate many branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f81a969. Ladin now receives one/many/other cardinal and many/other ordinal categories, with the supported-locale CLDR matrix extended to cover both.
Reject unsupported arguments in target-only exact branches, enforce finite ICU exact-selector syntax, and add modern Ladin plural categories with regressions. Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f81a9699b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| p.position++ | ||
| return quoted.String(), nil | ||
| } | ||
| return "", p.errorf("unterminated apostrophe quote") |
There was a problem hiding this comment.
When a valid ICU pattern ends while apostrophe-quoted syntax is still open, ICU MessageFormat implicitly closes the quoted span at the end of the pattern, but this path reports a syntax error. For example, Hello {name}: '{ is valid and renders the final brace literally, yet ICUSourceFindings rejects it and blocks every translation job containing that source value. Return the accumulated literal when input ends rather than treating this case as unterminated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2631b48. Apostrophe-quoted syntax now implicitly closes at end-of-pattern, and TestParseImplicitlyClosesApostropheQuoteAtEnd verifies parse plus deterministic reparse.
| for selector := range sourceSelectors { | ||
| _, targetHasSelector := targetSelectors[selector] | ||
| if targetHasSelector { | ||
| continue | ||
| } | ||
| _, applicableCategory := allowed[selector] | ||
| if isExactSelector(selector) || applicableCategory { |
There was a problem hiding this comment.
When a provider preserves an exact plural value but changes its decimal spelling, selector identity is compared as raw text rather than numeric value. Thus a source =1 branch translated as =1.0 (or =01) is reported missing even though ICU selects both branches for the same number, causing otherwise valid provider output and explicit adoption to be rejected. Canonicalize finite exact-number selectors before building and comparing the selector sets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2631b48. Exact numeric selectors are canonicalized during parsing, including leading/trailing zeros and negative zero; equivalent selectors compare identically and numeric duplicates are rejected. Regression coverage includes =1, =1.0, =01, and =01.000.
Treat end-of-pattern as an implicit apostrophe quote close and canonicalize exact numeric selectors before structural comparison. Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2631b48e28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| p.position += len("offset:") | ||
| start := p.position | ||
| for p.position < len(p.input) && p.input[p.position] >= '0' && p.input[p.position] <= '9' { |
There was a problem hiding this comment.
For valid ICU patterns that separate an offset from its value, such as {n, plural, offset: 1 one {One} other {# items}}, parsing starts the digit scan immediately after : and therefore reports that the offset is missing. This causes source validation to abort the translation job, or rejects otherwise valid provider output; skip ICU whitespace before scanning the offset value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d660b71. The parser now skips ICU whitespace between offset: and the numeric value; TestParseAllowsWhitespaceAfterPluralOffsetColon covers the reported form.
| if argumentType != ArgumentSelect && !isPluralSelector(selector) { | ||
| return nil, p.errorf("invalid plural selector %q", selector) |
There was a problem hiding this comment.
For select arguments, this condition bypasses all selector validation even though readSelector accepts punctuation up to whitespace or a brace. Consequently an invalid ICU pattern such as {g, select, male,female {x} other {y}} passes source self-validation and can be written unchanged, but ICU runtimes reject the comma-containing selector; apply the ICU identifier boundaries to select keywords as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d660b71. Select keywords must now consume a complete ICU-compatible identifier, rejecting comma punctuation while retaining Unicode keyword support. Added positive and negative parser regressions.
Allow ICU whitespace after plural offset colons and validate select keywords against identifier boundaries. Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d660b71d9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if findings := validation.ICUFindings(key, source, target, targetLocale); len(findings) > 0 { | ||
| return fmt.Errorf("%s for %q", findings[0].Message, key) |
There was a problem hiding this comment.
When a target locale adds a valid plural category containing the same nested argument as the source other branch, ProtectedFindings counts occurrences globally and rejects the result before this branch-aware ICU validator runs. For example, an English plural with {name} in one and other has two flat occurrences, while a valid Russian target adding few and many has four; provider output, TM reuse, and adoption are therefore blocked even though ICUFindings explicitly permits and validates these target-only categories. Successfully parsed ICU messages need branch-aware interpolation validation instead of the legacy flat multiset check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fede190. Valid ICU source/target pairs now bypass the legacy flat interpolation count and use branch-aware ICU comparison, while HTML, code, and link protections still run. Added Russian target-only few/many branch regressions at validation and provider boundaries.
| for p.position < len(p.input) && p.input[p.position] >= '0' && p.input[p.position] <= '9' { | ||
| p.position++ | ||
| } | ||
| if start == p.position { | ||
| return nil, p.errorf("plural offset must be a non-negative integer") |
There was a problem hiding this comment.
ICU plural offsets are non-negative numeric values rather than integers only, so a valid pattern such as {n, plural, offset:0.5 other {# items}} is rejected here: the scan stops after 0, and .5 is subsequently treated as an invalid selector. Because source validation now runs before every translation job, any existing message using a fractional offset prevents that bundle/locale job from running; offsets should be parsed and compared as finite ICU numbers rather than stored as int.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fede190. Plural offsets now accept, canonicalize, print, and compare non-negative decimal numbers (for example 00.500 becomes 0.5), matching the ICU pluralStyle grammar: https://unicode-org.github.io/icu-docs/apidoc/dev/icu4j/com/ibm/icu/text/PluralFormat.html
| func spaceWidth(input string, position int) int { | ||
| character, width := utf8.DecodeRuneInString(input[position:]) | ||
| if character == utf8.RuneError && width == 1 || !unicode.IsSpace(character) { | ||
| return 0 | ||
| } | ||
| return width |
There was a problem hiding this comment.
The follow-up identifier fix checks unicode.Pattern_White_Space, but spaceWidth still uses the different unicode.IsSpace property. Consequently a valid separator such as U+200E LEFT-TO-RIGHT MARK in {имя,select, other {x}} terminates the identifier but is not consumed, causing the parser to reject the message; conversely characters such as NBSP are accepted as syntax whitespace even though ICU does not classify them as Pattern_White_Space. Use the ICU property consistently for both identifier boundaries and whitespace consumption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fede190. Syntax whitespace now uses Unicode Pattern_White_Space consistently with ICU identifiers. Added coverage for U+200E as whitespace and NBSP remaining part of an identifier.
| for key, sourceValue := range validationKeys { | ||
| findings := ICUSourceFindings(key, sourceValue, sourceLocale) | ||
| sourceICUValid[key] = len(findings) == 0 | ||
| report.Findings = append(report.Findings, findings...) |
There was a problem hiding this comment.
This loop appends findings while ranging over a map, but when the target file is missing or unreadable the function returns before reaching sortFindings. With multiple malformed ICU source entries, human and JSON validation output therefore changes order nondeterministically between runs, unlike the previously deterministic missing-key output. Iterate over sorted keys or sort findings on each early-return path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fede190. Source ICU validation iterates sorted keys and both early-return paths sort findings before returning. Added a missing-target regression that asserts deterministic finding order.
Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review Addressed the four latest review findings in fede190. Full race suite, vet, npm wrapper/version/pack checks, go mod tidy -diff, 72.1% coverage, and all three 10-second fuzz targets pass. The formats fuzzer also surfaced a root-string JSON round-trip failure; the same commit fixes it and adds a permanent seed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fede1909ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for p.position < len(p.input) && p.input[p.position] != '}' { | ||
| if p.input[p.position] == '{' { | ||
| return nil, p.errorf("unexpected opening brace in %s style", argumentType) |
There was a problem hiding this comment.
When a supported date or time argument uses a custom formatter pattern containing an apostrophe-quoted brace, such as {d,date,'{'yyyy'}'}, ICU accepts the pattern and treats the brace as a literal, but this byte scanner reaches the quoted { and reports unexpected opening brace. Source validation therefore aborts translation and reports valid ICU messages as malformed; scan formatter styles with ICU apostrophe quoting rather than rejecting every opening brace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 709ba1f. Formatter-style scanning now tracks ICU apostrophe-quoted spans and doubled apostrophes, so quoted braces are preserved while unquoted opening braces remain rejected. Added a parse/print regression for {d, date, '{'yyyy'}'}.
| // These languages postdate x/text's embedded plural tables. | ||
| switch base.String() { | ||
| case "lld", "scn", "vec": | ||
| return true |
There was a problem hiding this comment.
For the valid bal locale, current CLDR uses one and other for both cardinal and ordinal plurals, but Balochi is absent here and from needsCurrentOrdinalOne. Because the underlying evaluator is explicitly based on CLDR 32, both category functions fall back to other only; i18next-v4 then omits a required _one key and ICU validation rejects a legitimate one branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 709ba1f. Added bal to the modern cardinal-one and ordinal-one overlays, with CLDR 49 category-set coverage for one/other in both modes: https://unicode.org/cldr/charts/49/supplemental/language_plural_rules.html
Signed-off-by: Tom Main <tom@execufunction.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 709ba1fb62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the other interpolation syntaxes Internationalizer already supports. | ||
| func LooksLike(input string) bool { | ||
| for index := 0; index < len(input); index++ { | ||
| if input[index] != '{' || (index > 0 && (input[index-1] == '{' || input[index-1] == '%')) || (index+1 < len(input) && input[index+1] == '{') { |
There was a problem hiding this comment.
When a valid ICU argument is immediately preceded by a literal percent sign, such as %{amount, number}, this condition skips its opening brace. If it is the message's only argument, both LooksLike checks return false and the legacy interpolation regex also ignores it because of the comma, so a provider can remove or alter amount without any finding. Only exclude the legacy %{name} form rather than every ICU-shaped argument following %.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 569ca2a. LooksLike now excludes only the legacy percent simple-placeholder form and still recognizes typed ICU arguments after a literal percent. Added a regression proving removal of %{amount, number} reports an argument mismatch.
| } | ||
| p.position++ | ||
| } | ||
| argument.Style = strings.TrimSpace(p.input[styleStart:p.position]) |
There was a problem hiding this comment.
strings.TrimSpace removes characters such as NBSP even though ICU does not classify them as Pattern_White_Space. In a custom date/time style like {d,date, yyyy}, that NBSP is literal formatter content, but parsing collapses the style to yyyy; consequently a target that removes the NBSP compares equal and can be written with changed output. Trim only ICU Pattern_White_Space around the style.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 569ca2a. Formatter styles now trim only Unicode Pattern_White_Space, preserving NBSP as literal style content. Added parse/print and structural-comparison coverage.
| } | ||
| if character < utf8.RuneSelf { | ||
| value := byte(character) | ||
| if value == '_' || value == '-' || value == '.' || value >= '0' && value <= '9' || value >= 'A' && value <= 'Z' || value >= 'a' && value <= 'z' { |
There was a problem hiding this comment.
Fresh evidence after the comma-selector fix is that this ASCII fast path still treats - and . as identifier characters, although both are Unicode Pattern_Syntax. Because isIdentifier reuses this predicate, malformed source patterns such as {g, select, male-female {x} other {y}} pass self-validation and can be written unchanged even though ICU runtimes reject that selector keyword. Keep the legacy compatibility characters only for argument names, not select keywords.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 569ca2a. Select keywords now use a dedicated Pattern_Syntax/Pattern_White_Space validator, while the permissive legacy argument-name parser remains unchanged. Added rejection coverage for hyphen and dot plus the existing Unicode keyword case.
Signed-off-by: Tom Main <tom@execufunction.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 569ca2ac55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var findings []Finding | ||
| if mismatch := InterpolationMismatch(key, source, target); mismatch != nil { | ||
| findings = append(findings, protectedFinding(key, "interpolation variables", mismatch.SourceVars, mismatch.TargetVars)) | ||
| if !usesICUValidation(source, target) { |
There was a problem hiding this comment.
When a target locale adds valid plural categories whose branches preserve source markup, only interpolation is exempted from flat comparison; the subsequent HTML, fenced-code, inline-code, and link extractors still compare global occurrence counts. For example, an English one/other plural containing <b> tags translated to Russian with one/few/many/other duplicates those tags in the required new branches, so validateTranslationValue rejects provider output and adoption even though ICUFindings considers the branches valid. These protected structures need the same branch-aware comparison used for ICU arguments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 171a584. Protected HTML, fenced code, inline code, and link destinations are now tokenized and compared within corresponding ICU branches, including target-only locale categories; exact-selector branches retain the existing argument-subset semantics. Added table coverage for all four structure types plus provider-boundary markup coverage. Fresh format/vet/test/race/lint gates, npm checks, 72.6% coverage, and parser fuzzing pass.
Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review Addressed the branch-aware protected-content P1 in 171a584. Regression coverage spans HTML, fenced code, inline code, links, target-only Russian plural branches, and the provider boundary. Fresh offline Go format/vet/test/race/lint gates, npm checks, go mod tidy -diff, 72.6% coverage, and a 10-second ICU parser fuzz run all pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 171a58401b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| direct.WriteByte('{') | ||
| direct.WriteString(element.argument.Name) | ||
| direct.WriteByte('}') |
There was a problem hiding this comment.
When protected syntax encloses a nested ICU argument, replacing that argument with only {name} discards the protected context before recursively tokenizing its branches. For example, in a fenced block such as ` ```sh\n{os, select, win {dir} other {ls}}\n``` `, the outer token is identical for any branch text and the recursive calls see dir/ls without the fence, so a provider can translate or corrupt the commands without any protected-content finding. The same gap affects inline code, link destinations, and HTML tags containing nested select/plural arguments; carry the enclosing tokenizer context into option branches rather than tokenizing each branch in isolation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 168f8a4. Protected-token matching now places stable markers at nested ICU arguments, detects which protected token encloses each marker, and propagates that protection context through descendant option branches at any depth. Added regressions for fenced commands, inline code, link destinations, and HTML attributes; each corrupted nested branch failed before the fix and is now rejected. Full offline gates, npm checks, 72.5% coverage, and parser fuzzing pass.
Signed-off-by: Tom Main <tom@execufunction.com>
|
@codex review Addressed nested protected-context propagation in 168f8a4. Regression coverage spans fenced commands, inline code, links, and HTML attributes containing nested ICU selects. Fresh offline Go format/vet/test/race/lint gates, npm checks, go mod tidy -diff, 72.5% coverage, and a 10-second ICU parser fuzz run all pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 168f8a4c2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if pluralType == Cardinal && hasCurrentCardinalTwo(tag) { | ||
| seen[plural.Two] = struct{}{} | ||
| } | ||
| if pluralType == Cardinal && needsCurrentCardinalOne(tag) { |
There was a problem hiding this comment.
For the valid blo locale, current CLDR defines zero, one, and other cardinal categories and zero, one, few, and other ordinal categories. The underlying evaluator uses CLDR 32, where these rules are absent, and none of these overlays handles blo, so both category functions return only other; i18next-v4 consequently omits required keys and ICU validation rejects valid Anii branches. Add Anii-specific cardinal and ordinal overlays.
Useful? React with 👍 / 👎.
| case elementText: | ||
| direct.WriteString(element.text) | ||
| case elementArgument: | ||
| marker := fmt.Sprintf("\x00icu:%08d:%s\x00", index, element.argument.Name) |
There was a problem hiding this comment.
When a valid translation reorders ICU arguments around a protected token, embedding the element index in this marker changes the synthetic token name even if the protected structure is identical. For example, moving {first} from before to after <a href="{url}">x</a> changes the marker for url from index 2 to index 1, so ProtectedFindings reports an HTML mismatch and rejects provider output or adoption despite both tags and ICU arguments being preserved. Use a stable occurrence identity that is unaffected by unrelated argument or text reordering.
Useful? React with 👍 / 👎.
1 participant
Summary
#placeholdersThis is the first phase of the localization-platform plan. Framework extraction/setup adapters, branch-aware review/CI, and optional runtime delivery remain separate follow-up work.
Test Coverage
npm pack --dry-run: passPre-Landing Review
Three focused review rounds covered maintainability, security/performance, and tests. Findings around canonical locale lookup, legacy state/TM handling, repeated ICU arguments, target-only plural branches,
#preservation, and newer CLDR category deltas were fixed. Final independent rechecks found no remaining blockers.GitHub automated review rounds found further ICU grammar and structure cases around nested plural context, Unicode identifiers, exact-selector safety and normalization, apostrophe semantics, offset spacing, select keywords, and newer Ladin categories. All were fixed through
d660b71with regression tests.Scope
Clean for phase 1: locale identity, ICU structure, CLDR category correctness, write-boundary enforcement, regression tests, and documentation. No runtime SDK/CDN or framework extraction work is included.
Test plan