ci: add translation-sync guardrails for READMEs and docs#455
Conversation
Add a check-translation-sync script + tests wired into CI: a blocking check that all five README.<locale>.md files share an identical level-2 heading structure (compared by structure not translated text, and code-fence-aware), and a non-blocking warning when a docs/en page changes without its zh/ja counterparts. Addresses alibaba#419.
|
|
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| runs-on: self-hosted | ||
| timeout-minutes: 10 | ||
| container: | ||
| image: node:24 |
There was a problem hiding this comment.
The container image node:24 is a mutable major-version tag that can change unexpectedly (e.g., when Node 24.x releases a new minor/patch). Other jobs in this workflow pin to exact versions (e.g., golang:1.26.5). Consider pinning to a specific version like node:24.3.0 (or whatever the current latest 24.x is) for reproducible builds.
There was a problem hiding this comment.
Pinned to node:24.18.0 in deaffd6, matching the golang:1.26.5 pin used by the other jobs.
| let fenceChar = null; // '`' or '~' while inside a fence, else null | ||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]; | ||
| const fence = /^\s{0,3}(`{3,}|~{3,})/.exec(line); | ||
| if (fence) { | ||
| const ch = fence[1][0]; | ||
| if (fenceChar === null) fenceChar = ch; // opening fence | ||
| else if (fenceChar === ch) fenceChar = null; // matching closing fence | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Bug: The fence parser only tracks the fence character (\`` or ~) but not the opening fence *length*. Per CommonMark spec, a closing fence must use the same character AND be at least as long as the opening fence. For example, if a README contains a code block opened with ````` (4 backticks) that has ``` (3 backticks) inside it, this parser would prematurely close the fence at the inner ```, causing subsequent # comment` lines in the code block to be misidentified as headings. This could produce false-positive structure divergence errors, blocking valid PRs.
Suggestion: Track the opening fence length and only close when the matching character sequence is >= the opening length.
Suggestion:
| let fenceChar = null; // '`' or '~' while inside a fence, else null | |
| for (let i = 0; i < lines.length; i++) { | |
| const line = lines[i]; | |
| const fence = /^\s{0,3}(`{3,}|~{3,})/.exec(line); | |
| if (fence) { | |
| const ch = fence[1][0]; | |
| if (fenceChar === null) fenceChar = ch; // opening fence | |
| else if (fenceChar === ch) fenceChar = null; // matching closing fence | |
| continue; | |
| } | |
| let fenceChar = null; // '`' or '~' while inside a fence, else null | |
| let fenceLen = 0; // length of the opening fence sequence | |
| for (let i = 0; i < lines.length; i++) { | |
| const line = lines[i]; | |
| const fence = /^\s{0,3}(`{3,}|~{3,})/.exec(line); | |
| if (fence) { | |
| const ch = fence[1][0]; | |
| const len = fence[1].length; | |
| if (fenceChar === null) { | |
| fenceChar = ch; | |
| fenceLen = len; // record opening fence length | |
| } else if (fenceChar === ch && len >= fenceLen) { | |
| fenceChar = null; // only close if >= opening length | |
| fenceLen = 0; | |
| } | |
| continue; | |
| } |
There was a problem hiding this comment.
Good catch - fixed in deaffd6 by tracking the opening fence length and only closing on a same-char run that is at least as long (per CommonMark). Added a regression test (testExtractHeadingsInnerShorterFenceDoesNotClose) that reproduces the premature-close case.
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Review feedback
Consider a separate workflow file
The translation-sync job checks both README structure and docs translation sync — it's a content validation concern, not a code quality one. I'd suggest moving it to its own workflow file (e.g. .github/workflows/translation-sync.yml) instead of appending to ci.yml:
- Separation of concerns:
ci.ymlfocuses on build/test/lint; translation sync is content validation. - Independent triggers: you can scope it with
paths:filters (only run whenREADME*.mdorpages/src/content/docs/**change), avoiding unnecessary runs. - No blocking the main pipeline: if the container pull is slow or the check has issues, it won't delay core CI feedback.
Minor suggestions
-
Remove dead variables in test —
testReorderedHeadingFailsdeclaresenandzhthen immediately discards them withvoid. Just remove the unused declarations and keep onlyen2/zh2. -
Code fence closing is length-unaware — The fence parser matches by character type (
`vs~) but doesn't track the opening fence length. Per CommonMark, a closing fence must be at least as long as the opening one. Practically this won't bite you in real READMEs, but worth a TODO comment for correctness. -
missingfiles still allow structure comparison to proceed — InrunReadmeCheck, if a README file is missing, the remaining files are still compared. This could mask divergence (e.g. if the reference file is missing, the comparison runs against the second file as reference). Consider short-circuiting with exit code 1 if any expected file is absent.
Overall the code is clean, well-tested, and follows existing project conventions. Nice work.
- Move the translation-sync job out of ci.yml into its own .github/workflows/translation-sync.yml, scoped with paths: filters (README*.md, pages/src/content/docs/**, the checker scripts) so it only runs on translation changes and never blocks the core pipeline. - runReadmeCheck: short-circuit with exit 1 when any expected README*.md is missing, before the structure comparison, so a missing file can no longer mask a real divergence. - extractHeadings: add a TODO(commonmark) note that closing fences are matched by fence char only, not by length. - test: drop the dead en/zh fixtures and the void statements in testReorderedHeadingFails; keep the en2/zh2 pair that actually drives the assertion.
Address the automated review comments beyond the maintainer's minor note: - extractHeadings: track the opening fence length and only close on a same-char run that is at least as long (per CommonMark). Previously a block opened with ```` and containing an inner ``` closed early, so lines inside the block could be misread as headings (or real headings dropped), producing false-positive structure-divergence errors. Adds a regression test covering the inner-shorter-fence case. - translation-sync.yml: pin node:24.18.0 instead of the mutable node:24 major tag, matching the golang:1.26.5 pin used by the other jobs.
|
Thanks for the thorough review, @lizhengfeng101 - all points addressed. Separate workflow file (5718464): moved the job to Minor suggestions:
Also pinned the container to All unit tests pass locally ( |
|
Suggestion: consider removing the Since our main branch only accepts changes via PRs, the |
3 participants
Adds the translation-sync guardrails from #419.
check-translation-sync.js readmesverifies all fiveREADME.<locale>.mdfiles share an identical level-2 heading structure. It compares structure, not text (the headings are translated -README.ko-KR.mdeven keeps English for some - so a text compare would wrongly fail), and is code-fence-aware so bash# commentlines are not misread as headings.check-translation-sync.js docswarns (never fails) when apages/src/content/docs/en/**page changes without itszh/jacounterparts..github/workflows/ci.yml(newtranslation-syncjob) following the existingpost-review-comments.jspattern;package.jsontest:github-actionsruns both suites.Verified:
node check-translation-sync.test.js(17 tests) pass; the blocking check passes on the current 5 READMEs and correctly exits 1 on an injected divergence; the docs warn path emits annotations without failing.AI disclosure: drafted with Claude Code (AI-assisted); verified with the unit tests and against the real repo READMEs.