chore(ci): fail CI when unapproved non-English text appears in source files - #876
Conversation
…ption tools.json advertised the example output as "File:path/to/example.go" with a fullwidth colon (U+FF1A), while file_read.go actually emits "File: %s". The description is sent to the model on every review, so the example did not match the output it was describing. Also switches action.yml's OCR_LANGUAGE example from 中文 to Chinese, for the same reason as #861: the value is fed to the LLM and Chinese is what the rest of the project uses.
Comments, identifiers and strings in this repository are meant to be English, but nothing enforced it — #861 had to clean up leftovers by hand, and the same drift keeps arriving through generated code and contributions written internally. scripts/verify-cjk.go walks the index plus untracked files and reports Han ideographs, kana, CJK punctuation and fullwidth forms. Written in Go rather than shell so it does not depend on the container's grep having PCRE, and so `unicode.Is` decides what counts as CJK instead of a byte range that would flag the em dashes used throughout the comments. `//go:build ignore` keeps it out of ./..., so it does not affect go vet, go build or the coverage threshold. Untracked files are included (--others --exclude-standard) so a new file is checked before it lands: while writing this, the script's own comment used Chinese punctuation as an example and went unreported until it was staged. Two escape hatches, preferring the narrow one: an `allow-cjk: <reason>` marker comment on a single line, or a prefix in allowedPrefixes for a whole tree. 23 existing lines get markers (UTF-8 encoding fixtures, multibyte truncation fixtures, language-switcher labels, the fullwidth bar used as a terminal cursor). pages/src/i18n/ is allowlisted as translated UI copy; extensions/vscode/ is allowlisted TEMPORARILY — its comments, test names and zh-cn NLS bundle are still Chinese and need a follow-up pass. Wired into CI next to the license and action-pin checks, plus `make cjk-check` and `make check` for local runs.
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
[maintainability · low] 📄
|
| out, err := exec.Command("git", "ls-files", "--cached", "--others", "--exclude-standard").Output() | ||
| if err != nil { | ||
| return fmt.Errorf("git ls-files: %w", err) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
When git ls-files fails (e.g., not a git repository, git not installed, corrupt index), Output() discards stderr. The resulting *exec.ExitError formats only as exit status N, losing the actual diagnostic message from git. This will make CI failures harder to debug.
Consider capturing stderr separately (or using CombinedOutput() if stdout pollution is acceptable here) and including it in the error message. For example:
cmd := exec.Command("git", "ls-files", "--cached", "--others", "--exclude-standard")
out, err := cmd.Output()
if err != nil {
if exitErr, ok := err.(*exec.ExitError); ok {
return fmt.Errorf("git ls-files: %w: %s", err, exitErr.Stderr)
}
return fmt.Errorf("git ls-files: %w", err)
}Note: the codebase pattern in cmd/opencodereview/git.go uses CombinedOutput() for the same reason.
| out, err := exec.Command("git", "ls-files", "--cached", "--others", "--exclude-standard").Output() | ||
| if err != nil { | ||
| return fmt.Errorf("git ls-files: %w", err) | ||
| } | ||
|
|
||
| var findings []finding | ||
| var scanned int | ||
| for _, file := range strings.Split(strings.TrimSpace(string(out)), "\n") { |
There was a problem hiding this comment.
| out, err := exec.Command("git", "ls-files", "--cached", "--others", "--exclude-standard").Output() | |
| if err != nil { | |
| return fmt.Errorf("git ls-files: %w", err) | |
| } | |
| var findings []finding | |
| var scanned int | |
| for _, file := range strings.Split(strings.TrimSpace(string(out)), "\n") { | |
| out, err := exec.Command( | |
| "git", "ls-files", "-z", "--cached", "--others", "--exclude-standard", | |
| ).CombinedOutput() | |
| if err != nil { | |
| return fmt.Errorf("git ls-files: %w: %s", err, strings.TrimSpace(string(out))) | |
| } | |
| var findings []finding | |
| var scanned int | |
| for _, file := range strings.Split(string(out), "\x00") { |
| switch { | ||
| case unicode.Is(unicode.Han, r), | ||
| unicode.Is(unicode.Hiragana, r), | ||
| unicode.Is(unicode.Katakana, r): |
There was a problem hiding this comment.
| unicode.Is(unicode.Katakana, r): | |
| unicode.Is(unicode.Katakana, r): | |
| unicode.Is(unicode.Hangul, r): |
There was a problem hiding this comment.
Currently omitting Korean Hangul.
| - name: Verify action pins | ||
| run: bash scripts/verify-action-pins.sh | ||
|
|
||
| - name: Verify sources are English-only |
There was a problem hiding this comment.
| - name: Verify sources are English-only | |
| - name: Verify sources contain no unapproved CJK text |
There was a problem hiding this comment.
Not quite accurate here, but fine.
| - After writing code, run `make check` to format and check the code. | ||
| - `make check` runs: license check, `go mod tidy`, `gofmt -s -w .`, and `go vet`. | ||
| - `make check` runs: license check, CJK check, `go mod tidy`, `gofmt -s -w .`, and `go vet`. | ||
| - Source files are English-only — comments, identifiers and strings alike. Translated prose belongs in `README.<locale>.md`, `pages/src/content/docs/<locale>/` or an i18n table. `make cjk-check` enforces this in CI; it also rejects fullwidth punctuation (`:`, `(`), which is easy to leave behind in an otherwise English sentence. |
There was a problem hiding this comment.
| - Source files are English-only — comments, identifiers and strings alike. Translated prose belongs in `README.<locale>.md`, `pages/src/content/docs/<locale>/` or an i18n table. `make cjk-check` enforces this in CI; it also rejects fullwidth punctuation (`:`, `(`), which is easy to leave behind in an otherwise English sentence. | |
| - Source files must not contain unapproved CJK text in comments, identifiers or strings. Translated prose belongs in `README.<locale>.md`, `pages/src/content/docs/<locale>/` or an i18n table. `make cjk-check` enforces this in CI; it also rejects fullwidth punctuation (`:`, `(`), which is easy to leave behind in an otherwise English sentence. |
There was a problem hiding this comment.
This is as same as ci.yml:L35
| } | ||
|
|
||
| // exemptMarker on a line suppresses the report for that line. | ||
| const exemptMarker = "allow-cjk" |
There was a problem hiding this comment.
| const exemptMarker = "allow-cjk" | |
| const exemptMarker = "allow-cjk:" |
Maybe why: requiring the colon prevents incomplete allow-cjk substrings from accidentally exempting a line.
Addresses the review feedback, and widens the rule that the feedback exposed. Review feedback: - exemptMarker requires its colon, so a bare "allow-cjk" can no longer exempt a line without giving a reason. - The script is named for CJK but missed Hangul. - git ls-files gains -z, so paths that are not plain ASCII arrive unquoted, and its stderr is reported rather than a bare exit status. - main discarded run()'s error entirely and only called os.Exit(1), which is what made the lost stderr invisible in the first place. - The CI step and AGENTS.md say "unapproved", since escape hatches exist. The check was skewed by writing system rather than by language. In one array the 'zh' and 'ja' labels each needed a marker while the adjacent 'ru' label passed untouched, and nine lines of Russian sat in the tree unflagged: two language-switcher labels and the heading-ID fixtures. Contributors writing Chinese had to justify every line; contributors writing Russian had nothing to justify. The rule is now "a letter outside ASCII", since written English needs no letter beyond the ASCII 26 -- Cyrillic and Han as much as the diacritics that spell German or Vietnamese. Scripts are not enumerated, so one nobody has contributed in yet is covered when it arrives. Common and Inherited pass, so letterlike symbols (U+2139, U+2113) are not mistaken for prose, and combining accents are caught, so the decomposed spelling of an accented letter cannot slip through. Symbols and emoji stay out of scope by construction: they are not letters. Renamed to scripts/verify-english-only.go and make english-check, and the marker to allow-non-english:. Text spelled entirely in ASCII still takes a dictionary to identify and stays a matter for review.
The rule was one dense bullet that led with the detection mechanism and mentioned the exemptions only in passing, which is the wrong order for the reader: an agent needs to know where a translation may go before it needs to know which Unicode scripts are flagged. Split into three. The homes are now spelled out from what the tree actually holds, rather than left as "<locale> docs or an i18n table": README and CONTRIBUTING in zh-CN, ja-JP, ko-KR and ru-RU; the doc pages under pages/src/content/docs/ in en, zh, ja and ru; the UI copy tables in pages/src/i18n/. Also why the two are exempt for different reasons -- Markdown by extension, the i18n tables by prefix because they are .ts -- since that decides where a new translation can safely go. Drops the enumerated list of what "make check" runs. It duplicated the Makefile, went stale the moment a check was added (this PR had to edit it), and told an agent nothing it would not read in the output anyway. What is worth saying is that the target writes to the tree.
| func isNonEnglish(r rune) bool { | ||
| switch { | ||
| case r < 0x80: // ASCII, the overwhelming majority of every scanned line | ||
| return false | ||
| case unicode.IsLetter(r) && | ||
| !unicode.Is(unicode.Common, r) && | ||
| !unicode.Is(unicode.Inherited, r): | ||
| return true | ||
| case r >= 0x0300 && r <= 0x036F: | ||
| // Combining diacritical marks, so that the decomposed spelling of an | ||
| // accented letter is caught too: NFD writes e-acute as "e" plus U+0301, | ||
| // where the letter itself is plain ASCII and the accent carries the | ||
| // language. Variation selectors (U+FE0F, which follows an emoji) are | ||
| // combining marks as well, but sit outside this block and pass. | ||
| return true | ||
| case r >= 0x3000 && r <= 0x303F: // CJK Symbols and Punctuation | ||
| return true | ||
| case r >= 0xFF00 && r <= 0xFFEF: // Halfwidth and Fullwidth Forms | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
A few classes of non-English text slip past the check
I put each of these on its own line in an untracked .ts file and ran make english-check — none of them was reported:
| Sample | Code points | Why it passes |
|---|---|---|
⼀⼥⽥ |
U+2E80–U+2FD5, CJK and Kangxi radicals | script=Han, but category So, so not a letter |
﹐﹖︑ |
U+FE10–U+FE6F, small form and vertical form CJK punctuation | only U+3000–U+303F and U+FF00–U+FFEF are range-checked |
٢٠٢٦ |
U+0660–U+0669, Arabic-Indic digits | script=Arabic, category Nd |
ー |
U+30FC, Japanese long vowel mark | category Lm, but script=Common, so exclusion 1 clears it |
𝛼𝛽 |
U+1D6A8+, maths-styled Greek | script=Common |
Not sure whether the script should defend against these. The check is per-line, so real prose still gets caught by its other letters; it only matters for a line built entirely from the characters above. The U+FE10–U+FE6F row is the one I'd lean towards adding, since its U+FF00 counterparts are already caught and ﹖ reads as correct English punctuation in review.
The vertical forms (U+FE10–FE19), CJK compatibility forms (U+FE30–FE4F) and small form variants (U+FE50–FE6F) were not caught, even though their fullwidth counterparts (U+FF00–FFEF) already were. A small question mark (U+FE56 ﹖) or vertical comma (U+FE10 ︐) left in source reads as correct English punctuation and is invisible in review — the same class of typo the fullwidth range already defends against. Skip U+FE20–FE2F (Combining Half Marks) which are used in Latin text.
Why
Comments, identifiers and strings in this repository are meant to be English, but nothing enforced it. #861 had to clean up leftovers by hand, and the same drift keeps arriving through generated code and contributions written internally — reviewers cannot reliably catch it by eye.
What
scripts/verify-english-only.gowalks the index plus untracked files and reports non-English text across 14 source extensions (.go .ts .tsx .js .cjs .mjs .py .sh .ps1 .css .html .yml .yaml .json) plusMakefile. Markdown is not scanned — the translated READMEs, CONTRIBUTING files and doc pages are legitimately non-English.Wired into CI next to the existing license and action-pin checks, plus
make english-checkandmake checkfor local runs.AGENTS.mdstates the rule in three parts: the rule itself, where translated prose may go (spelled out from what the tree actually holds, since Markdown is exempt by extension while the.tsi18n tables are exempt by prefix), and the two escape hatches.What counts as non-English
A letter outside ASCII, whichever the writing system. Written English needs no letter beyond the ASCII 26, so anything past that belongs to another language: Cyrillic and Han as much as the diacritics that spell German, French or Vietnamese. Plus CJK and fullwidth punctuation, and combining accents.
Scripts are deliberately not enumerated. A language nobody has contributed in yet is covered on the day it arrives, with no edit to the script.
Three exclusions, each load-bearing:
CommonandInheritedpass. Those two scripts hold the characters belonging to no writing system in particular, and the letterlike symbols among them are letters only by Unicode category — the information source (U+2139, categoryLl) that renders as an info icon, the script small l (U+2113), the capitals of the maths alphabets. An earlier revision without this exclusion flagged threeℹ️lines.─alone) are not letters, and neither are the em dashes used throughout the comments.eplus U+0301, where the letter is plain ASCII and only the accent carries the language. Variation selectors are combining marks too, but sit outside that block and pass, so⚠️is fine.Letterlike forms that Unicode assigns to a real script stay in scope, so the ohm sign (U+2126 — script Greek, since it is equivalent to U+03A9) is reported like any other Greek letter. A comment spelling sigma or omega as a glyph needs a marker. That is deliberate: exempting Greek to allow maths notation would exempt Greek prose with it.
Not detected: another language spelled entirely in ASCII — a romanised transcription, or German with its umlauts written out (
Loeschen der Datei). Telling that from English takes a dictionary rather than a character test, so it stays a matter for review.Design notes
grephaving PCRE, andunicodedecides what counts.//go:build ignorekeeps the script out of./..., so it does not affectgo vet,go buildor the 90% coverage threshold.--others --exclude-standard, ignored paths still excluded) so a new file is checked before it lands rather than the run after.git ls-files -z. Without it git quotes and escapes any path that is not plain ASCII — exactly the kind of pathinternal/diff/git_test.gohas fixtures for.Escape hatches, narrow one preferred
allow-non-english: <reason>marker comment on a single line — the rest of the file stays protected. 28 existing lines get one: UTF-8 encoding fixtures, multibyte truncation fixtures, language-switcher labels, heading-ID fixtures, and the fullwidth bar used as a terminal cursor. The colon is part of the marker, so a bareallow-non-englishcannot exempt a line without giving a reason.allowedPrefixesfor a whole tree. Only two entries:pages/src/i18n/(translated UI copy) andextensions/vscode/, marked TEMPORARY.Issues the check found
The rule was skewed by writing system rather than by language. In
pages/src/components/Footer.tsxthezhandjalabels each needed a marker while the adjacent{ value: 'ru', label: 'Русский' }passed untouched — a CJK-shaped rule cannot see Cyrillic. Nine lines of Russian sat in the tree unflagged: two language-switcher labels and the heading-ID fixtures inMarkdownRenderer.test.tsxandheadingId.test.ts. Contributors writing Chinese had to justify every line; contributors writing Russian had nothing to justify. Widening the rule to "a letter outside ASCII" removes the skew and costs nothing: the whole tree contains four non-ASCII Latin letters, two of which already carry a marker.internal/config/toolsconfig/tools.jsonadvertised thefile_readexample output asFile:path/to/example.gowith a fullwidth colon (U+FF1A), whileinternal/tool/file_read.go:68emitsFile: %s. That description is sent to the model on every review, so the example did not match the output it described. A Han-only check would have missed this.The script's own doc comment used Chinese punctuation as an example and stayed green — because it was still untracked and
git ls-filesonly lists tracked files. That is what motivated including untracked files;scripts/verify-license.shhas the same limitation today. It has since caught its own comments three more times during this PR (a文件.tspath, a sigma/omega glyph, an umlaut), each time correctly.maindiscarded the error.run()returned wrapped errors that nothing ever printed —maincalledos.Exit(1)and dropped them, so agit ls-filesfailure surfaced as a bare exit status with no diagnostic at all. Fixed alongside capturing git's stderr, which would otherwise have been invisible for the same reason.Also switches
action.yml'sOCR_LANGUAGEexample from中文toChinese, for the same reason as #861: the value is fed to the LLM, andChineseis what the rest of the project uses.Verification
make english-check— clean across 384 scanned filesℹ️,ℓand box drawing all pass; a bareallow-non-englishwithout a reason does not exempt; an untracked file with non-English text is reported and exits 1make license-check,gofmt -l,go vet ./...,go test ./cmd/... ./internal/...— all passnpx tsc --noEmit,npx eslint,npx vitest runinpages/— no errorspython3 -m unittestinexamples/gerrit_ci/(61 tests) andnode --test scripts/github-actions/*.test.js— passFollow-up
extensions/vscode/still has roughly 450 non-English lines (comments, test names,package.nls.zh-cn.json). Once those are translated, the allowlist entry should be deleted — worth tracking as its own issue so the temporary entry does not become permanent.