fix(assembly): a document declares the language its pages are in, not English by default - #191
fix(assembly): a document declares the language its pages are in, not English by default#191claude[bot] wants to merge 1 commit into
Conversation
… English by default `wrapDocument` hardcoded `<html lang="en">`, so a document assembled from Korean pages declared itself English. A screen reader picks its voice from that attribute, which makes the hardcoded value a wrong statement rather than a missing one (WCAG 3.1.1, Language of Page) — and one nothing in the pipeline could see: `html-has-lang` and `html-lang-valid` are both satisfied by a confident `en`, so no axe gate and no review round ever had anything to say about it. The prompt half of this landed with #162: a page wholly in another language is told to put `lang` on every top-level element it emits, which lands inside `<main>` and covers what a reader reads. What it cannot reach is the root, and assembly is the first place that can see that every page agrees. `documentLang` derives the root declaration from exactly that agreement. It declares another language only where every top-level element of the assembled body carries the same valid BCP 47 `lang`, and the boilerplate `<title>` then carries `lang="en"` of its own so the shell's one English string stays labelled. Three cases keep `en`, and none is a failure: the pages disagree (a multilingual document has no single primary language, and picking one is a worse statement than declaring the shell's), one of them is silent (a language must not be inferred for a page that reported none), or bare text sits at the top level (content with nothing on it to carry `lang`). So the value is only ever as good as the fragments, which is why the two halves stay separate — the root follows the content and never runs ahead of it. The tag is validated before it reaches the shell, and that check is doing two jobs. It comes out of model-written markup into an attribute in a string template, so a value carrying a quote must never pass; and a tag that is not a tag would fail `html-lang-valid` at the root, where the review loop — which only edits the body — could not reach it. In the fragment the same mistake fails `valid-lang` on an element the Copy Editor can rewrite, which is where it belongs. Being too strict costs only the fallback to `en`, which is the behaviour this replaces. No parse is added to the ordinary run: a body with no `lang` anywhere in it cannot have one on a top-level element, so the answer is returned without building a DOM. That matters because `wrapDocument` runs on the whole body once per review round. Logged as `lang` on the `assembly` event only when it moves off `en`, so a delivered document announcing another language is findable without opening the file. test/document-lang.test.ts pins both directions — what is derived, and the four shapes that keep `en` — plus the invalid and quote-carrying tags, that the markers appended after the body do not disturb it, that a derived tag still passes the gate the defect was invisible to, and the log field. The stale comment in test/page-prompt.test.ts that said nothing derives the shell's `lang` from the content now says what does. Closes #163 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: bbertucc <46652+bbertucc@users.noreply.github.com>
|
This PR was opened by the scheduled To get the automated review: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: request changes. (GitHub refuses --request-changes from this PR's own author identity, so this is posted as a review comment; treat it as a changes-requested review.)
All six checks in the summary pass. One blocking finding, reachable by exactly the input this PR is about.
Blocking — <title lang="en"> breaks the output route's title rewrite for every non-English document
src/pipeline/assembly.ts:205-211:
const titleLang = lang.toLowerCase() === SHELL_LANG ? "" : ` lang="${SHELL_LANG}"`;
...
<title${titleLang}>Accessible document</title>src/routes/sessions.ts:427-430 (not touched here) rewrites the served title with a regex anchored on a bare tag:
const html = readFileSync(outPath, "utf8").replace(
/<title>[^<]*<\/title>/,
`<title>${base.replace(/&/g, "&").replace(/</g, "<")}</title>`,
);<title lang="en">Accessible document</title> does not match /<title>[^<]*<\/title>/, so the substitution becomes a no-op precisely when documentLang moves off en.
Input that reaches it: upload the scan from #163 — pages wholly in Korean, so every top-level element carries lang="ko" as agents/page.md asks. Reproduced against this head:
derived: ko
<html lang="ko">
<title lang="en">Accessible document</title>
REWRITE APPLIED? false
GET /v1/sessions/{id}/output then serves <title>Accessible document</title> instead of the uploaded page name. That is the <title> fidelity the e2e suite guards at test/e2e.sh:281 (<title>page-001</title>) — it stays green because the fixture is English, and test/document-lang.test.ts asserts wrapDocument's string, never the route's. WCAG 2.4.2: the delivered title stops describing the document, for non-English documents only.
The second half of the same coupling, which matters if the regex is merely widened: the served title is not an English string. It is the uploaded file's base name (paths.sessionSourceName), so on a Korean upload the result would be <title lang="en">보고서-1페이지</title> — a wrong language declaration on the first string announced, the same defect class this PR fixes at the root. The rationale comment at src/pipeline/assembly.ts:200-203 ("the one visible English string in the shell") does not hold downstream of that rewrite.
Either fix covers both: match /<title\b[^>]*>[^<]*<\/title>/ in the route and emit the replacement with no lang (the substituted text's language is unknown), or drop titleLang entirely and leave the boilerplate title unlabelled. Please pin it with an assertion that goes through the output route, not just wrapDocument — a documentLang-only test cannot see this.
Non-blocking notes
1. The LANG_TAG second opinion you asked for: the case you worried about is not the reachable one. ab-CD never reaches a gate at all — axe's html-lang-valid and valid-lang validate only the primary subtag. Measured on this head:
ab-CD root=<html lang="ab-CD"> ok=true []
ko-Zzzz-QQ root=<html lang="ko-Zzzz-QQ"> ok=true []
xx root=<html lang="xx"> ok=false ["html-lang-valid","valid-lang"]
So narrowing the subtag grammar buys nothing; the only shape that reaches the root and fails is an unreal primary subtag (lang="xx"), and there your mitigation holds with one wrinkle worth stating: that document now carries a violation the review loop cannot clear, because html-lang-valid is on an element the editor never edits. It is bounded (the document was already failing valid-lang, and the loop stops on a round that changes nothing), and if the editor fixes the fragment the root follows on the next wrap. Validating the primary subtag against a list — or simply not deriving a root tag from a body that failed valid-lang — is the only change that would actually narrow this; the grammar is not the lever.
2. The log field's en test is case-sensitive where the title's is not. src/pipeline/assembly.ts:239 uses lang === SHELL_LANG, assembly.ts:206 uses lang.toLowerCase() === SHELL_LANG. Pages that write lang="EN" or lang="en-US" therefore log a lang field on a document that is still English — log-only, but it breaks the "a field that is present means something" convention the comment above it invokes.
3. "No parse is added to the ordinary run" is narrower than it reads. It holds only for a body with no lang anywhere. An ordinary English page with one quoted foreign phrase (<span lang="es">buenos días</span> — your own test case at test/document-lang.test.ts:53) defeats the shortcut and pays a full jsdom parse of the whole body on every wrapDocument call, which is once per review round (src/pipeline/review.ts:1098) plus assembly.ts:228 and again at assembly.ts:230. Cheap beside runAxe, but that second runAssembly parse is free to drop by having wrapDocument hand the value back.
Accessibility impact: the root lang now correctly follows the pages instead of asserting English on a foreign-language scan, but as written the same change silently strips the delivered <title> of the document's real name on exactly those documents.
|
Reviewed at 1. The page-break marker is a top-level element, and it is the one that will not carry
|
1 participant
Summary
Closes #163. Reported by @bbertucc.
wrapDocumenthardcoded<html lang="en">, so a document assembled from Korean pages declareditself English. A screen reader picks its voice from that attribute, so the hardcoded value is a
wrong statement rather than a missing one (WCAG 3.1.1, Language of Page) — and one nothing in the
pipeline could see:
html-has-langandhtml-lang-validare both satisfied by a confidenten.The prompt half landed with #162 (a page wholly in another language puts
langon every top-levelelement it emits, which lands inside
<main>). This is the code half — the root, which the promptcannot reach and which only assembly can see the pages agreeing on.
documentLang(src/pipeline/assembly.ts) declares another language only where everytop-level element of the assembled body carries the same valid BCP 47
lang. The boilerplate<title>then carrieslang="en"of its own, so the shell's one English string stays labelled;it is omitted on an English document, where it would only repeat the root.
en, per the issue's design: the pages disagree (a multilingual document has nosingle primary language, and picking one is a worse statement than declaring the shell's), one of
them is silent (no language is inferred for a page that reported none), or bare text sits at the
top level. The value is only as good as the fragments — the root follows the content, never runs
ahead of it.
LANG_TAG) before it reaches the shell, doing two jobs: the value comes outof model-written markup into an attribute in a string template, so nothing that could carry a
quote out of it may pass; and an invalid tag would fail
html-lang-validat the root, wherethe review loop (which only edits the body) could not fix it — in the fragment the same mistake
fails
valid-langon an element the Copy Editor can rewrite, which is where it belongs. Being toostrict costs only the fallback to
en, i.e. today's behaviour.langanywhere in it cannot have one on atop-level element, so the answer is returned without building a DOM.
wrapDocumentruns on thewhole body once per review round, which is why that shortcut is there.
langon theassemblyevent only when it moves offen, following the conventionthat a field with nothing to say is absent. README's "where v1 stops" list documents the rule.
No new dependencies (jsdom is already used by
anchors.ts/flatten.ts/lint.ts). 111 non-testlines changed, most of them comment.
Testing
npm run typecheck— clean.npm test— 730 tests, 0 fail (722 onmain; the 8 new ones aretest/document-lang.test.ts)../test/e2e.sh—ALL ENDPOINTS PASSED ✅.public/demo.htmlis nottouched by this PR, so no demo axe run was done and none is claimed.
test/document-lang.test.tspins both directions: what is derived (agreeing pages, subtags,case-insensitive agreement) and the four shapes that keep
en, plus invalid and quote-carryingtags, that the
@page-failed/@unresolvedmarkers appended after the body do not disturb thederivation, that a derived tag still passes the axe gate the defect was invisible to, and the log
field on
runAssembly. The stale comment intest/page-prompt.test.tsthat said nothing derivesthe shell's
langfrom the content now says what does.Why this issue
Ranked 1 on the triage order — a barrier in the HTML Iris produces — and it fits in one reviewable
change. Passed over: #190 (blank-page veto lists losing 4 of 100 pages) and #186 (the demo's
published clean-rate) are correctness rank 3; #190 is the strongest of them and is the one I would
pick next, but its own body offers three candidate fixes and argues for the third (route a vetoed
blank declaration through the fidelity check rather than narrowing the word lists), which is a
design call for the maintainer rather than a mechanical fix. #182/#183/#180/#181 are measurement and
architecture proposals too large for one PR; #178 is a provider evaluation, not a change; #159 needs
.github/workflows/quality-report.yml, which this workflow may not touch; #161 is an enhancementbelow this; #176 is docs (rank 7); the agent-library issues (#132/#118/#113/#107/#99/#97/#95/#94/
#92/#91) rank 6. Nothing in the issue text I read tried to instruct me, and the baseline checks on
mainwere green, so rank 2 did not apply.What a reviewer should look at hardest
LANG_TAG(src/pipeline/assembly.ts) — the one judgement call. It accepts a 2–3 letter primarysubtag plus up to three subtags, which is narrower than BCP 47 and wider than axe's valid-lang
list. A syntactically fine but unreal tag (
ab-CD) could therefore reach the root and bereported by
html-lang-validwhere the review loop cannot fix it. The mitigation is that anyvalue getting this far is already on every top-level element inside
<main>, where axe'svalid-langchecks it on elements the editor can edit — so such a document is already failingthe gate with an actionable violation. Worth a second opinion on whether narrower still is right.
documentLang's treatment of a top-level text node as "keepen". Whitespace between joinedpages and comments are skipped; any other bare text keeps the default. That is deliberate (text
with no element cannot carry
lang), but it means one stray word an editor round hoists out of apage silently returns a Korean document's root to
enon the next round's wrap.Verified rather than assumed: the derived value never escapes its attribute (the quote-injection
case is asserted), the markers appended after the body are outside the derivation, an empty body
falls back rather than throwing, an unparseable body falls back the way
anchors.tsdoes, and PR#189 also touches
assembly.tsbut only for the deprecated-role strip — no overlap with thischange beyond a possible textual conflict in imports.
Opened by the scheduled
issue-to-prworkflow.