Repository navigation
fix(schema-gate): check well-formedness as emitted and reject repeated mc:Ignorable tokens - #1501
Conversation
…d mc:Ignorable tokens The emitted-document gate rejected a root that repeats xmlns:w14 or xmlns:mc, because xmldom throws on a repeated attribute. It did not reject what such a fixture leaves in a comparison's output: docx-core's parser drops the repeated declaration (#1032) but keeps mc:Ignorable="w14 w14 ...", and the gate passed that. It also validated only the Markup Compatibility projection, so two prefixes bound to one URI that gave an element the same expanded attribute name (Namespaces in XML 1.0 section 6.3) passed whenever mc:Ignorable dropped that namespace. The gate now runs libxml2 over every instance as emitted, before preprocessing, and fails on any error line (libxml2 reports namespace errors with exit 0). A pre-pass checks mc:Ignorable, mc:MustUnderstand and mc:ProcessContent against ECMA-376 Part 3 sections 7.2 to 7.4 (bound prefix, not the MC namespace, ProcessContent token shape and ignorable scope) and rejects a repeated token. The self-test now proves the well-formedness pass rejects a namespace error. buildDocxFromBodyXml (both copies) merges caller namespaces into its built-in w/w14/mc bindings: restating a binding is a no-op, a different URI for a built-in prefix throws, Ignorable prefixes are listed once and must be bound and not the MC namespace. Fixes: #1484
Every comparison run writes compared.docx, so a smoke over several runs passes inputs that share a basename. The gate extracted each package to one scratch path per basename, so only the last package was validated, once per input. Each package and each projection now gets its own scratch directory, and failures name the source package. Ref: #1484
The #1032 tests built their malformed input by passing w in namespaces to buildDocxFromBodyXml, which now declares each prefix once and rejects a conflicting URI. They now build the clean package and write the repeated declaration into word/document.xml themselves. The fixture comment also stops naming the spec without a registry-backed citation. Ref: #1484
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review Summary: ✅ CompletedThis 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. |
LLM gate (advisory)All evaluated rules passed - 8 pass, 0 warn, 0 error, 8 skipped, 16 total FindingsNone. All 16 rules (8 evaluated, 8 skipped)
|
…ocessContent parts and stage instances under controlled names Review of #1501: mc:MustUnderstand="xml" was rejected as unbound because xmldom's lookup does not report the predefined xml binding (Namespaces in XML 1.0 section 3). ProcessContent local parts such as ext:1bad or ext:** passed; ECMA-376 Part 3 section 7.3 requires a local name (an NCName) or exactly *. xmllint diagnostics were attributed by matching the input path, which a path containing a newline, or one path prefixing another, defeats; every instance is now staged as instance-<n>.xml in the scratch directory and reported under its source name. Ref: #1484
Codex peer review, round 1 (gpt-6.1-sol, medium) at 470acce: CHANGES_REQUIREDCodex ran the gate tests, the self-test and both fixture suites, and probed the gate further. I accepted all three findings and fixed them in 6f4b3c9.
Re-verified after the fixes:
A confirmation round follows. |
Codex peer review, rounds 2 and 3
After both rounds, the review left nothing behind: the working tree is clean, HEAD is unchanged and the branch is unchanged. |
|
Pre-submit rerun at bd41331 passes:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Post-merge smoke passedSquash commit Gate over emitted corpora
The one failure, #1484 reproduction
Real documents. NVCA Indemnification Agreement and NVCA Voting Agreement, from the real comparison corpus, each compared through
Follow-up (not filed). The Cleanup: remote branch |
Problem
Issue #1484 says the emitted-document schema gate (
scripts/check_emitted_document_schema.mjs) accepts aw:documentroot that declaresxmlns:w14andxmlns:mctwice. Reproducing at base 1b1149e showed a narrower gap:Attribute xmlns:w14 redefined. The issue's literal shape fails on main (exit 1).parseXmldrops an exact repeat of a namespace declaration on input (docx-core: a part that declares xmlns:w twice on one element throws a raw ParseError instead of loading or failing closed #1032) but keeps the attribute value, so the redline's root carriesmc:Ignorable="w14 w14 wps". The gate accepted that (exit 0). Reproduced with the pre-d98d03db fixture from fix(docx-compare): track header/footer table rows beside a text box and never wrap a cell in w:ins #1479: the original fails the gate, the compared output passes.mc:Ignorabledrops a namespace, the gate re-serializes the DOM and gives xmllint the projection, not the bytes as emitted. Two prefixes bound to one URI can give one element the same expanded attribute name twice, which is not namespace-well-formed (Namespaces in XML 1.0 §6.3). That passed whenever preprocessing removed the namespace. Without preprocessing, xmllint caught it.mc:Ignorablerules were only partly checked. An unbound prefix failed. A prefix bound to the MC namespace, a repeated token, andmc:MustUnderstand/mc:ProcessContenttokens were not checked.LibreOffice 26.2.5.2 (headless
--convert-to odt, through the heavy-run gate) on the three shapes:document.xmlwithxmlns:w14/xmlns:mcdeclared twicemc:Ignorable="w14 w14 wps"mc:Ignorable="w14 wps"So the LibreOffice refusal in #1479 came from the input fixture, not the redline. The repeated declarations are a well-formedness error that any XML parser rejects. The repeated Ignorable token is not a LibreOffice problem. It is what such a fixture leaves behind in compared output.
Approach
Gate (
scripts/check_emitted_document_schema.mjs)checkWellFormedBatch). This uses libxml2 (xmllint --noout, no schema), the parser LibreOffice's XML fast parser is built on. libxml2 reports namespace errors with exit status 0, so the gate fails on anyerror :line naming the file, not on the exit code. Instances that fail this check skip schema validation, so one problem is not reported twice.checkMarkupCompatibilityAttributes, checks the attributes against ECMA-376 Part 3, 5th edition (2015). I checked these clauses against the vendoredspec-compliance/ecma-376/source-artifacts/ECMA-376-3_5th_edition_december_2015.zip:mc:Ignorableand §7.4mc:MustUnderstand: each prefix is bound to an in-scope namespace that is not the MC namespace.mc:ProcessContent: each token isprefix:localNameorprefix:*, its prefix is bound to a namespace other than MC, and that namespace is declared ignorable at the same element or an ancestor.The pre-pass visits content that preprocessing later removes (ignorable elements,
mc:AlternateContent).--self-testnow also proves that the well-formedness pass rejects a namespace error hidden bymc:Ignorable.compared.docxvalidated only the last one. This was a latent bug, and it is fixed: each package and each projection gets its own scratch directory, and failures name the source package.Fixture helper
buildDocxFromBodyXml(packages/docx-core/src/testing/ooxml-fixtures.tsand its copy inpackages/docx-compare/src/testing/ooxml-fixtures.ts)namespacesinto the built-inw/w14/mcbindings. Restating a binding with the same URI is a no-op. A different URI for a built-in prefix throws.ignorablePrefixesare listed once. Each one must be bound on the root and must not be the MC namespace. Every existing caller (git grep -n "ignorablePrefixes\|namespaces:" packages/*/src) passes bound, non-MC prefixes and keeps working. The fix(docx-compare): track header/footer table rows beside a text box and never wrap a cell in w:ins #1479 workaround that stripsw14/mcis now unnecessary but harmless, so I left it alone.Callers that built malformed input on purpose. The #1032 tests (
packages/docx-core/src/primitives/xml_repeated_namespace_declaration.test.tsandpackages/docx-compare/src/tagged/xmlPartRepeatedNamespaceDeclaration.test.ts) passednamespaces: { w: … }to get a repeatedxmlns:w. They now build the clean package and write the repeat intoword/document.xmlthemselves.There is no
@conformancetag. Part 3 has no entry inspec-compliance/registry/, and the citation lint scans only.ts. The Part 3 clauses are cited in prose.Evidence
check_emitted_document_schema.test.mjsagainst the base gate (git show 1b1149e1:scripts/check_emitted_document_schema.mjs): 4 of 11 failed. The failures were the doubled-w14compared root, the §6.3 duplicate hidden by preprocessing, an Ignorable prefix bound to MC, and the MustUnderstand/ProcessContent checks. The test for the literal repeated-declaration root passes on base and stays as a guard. After the fix, 11 of 11 pass.ParseError: Attribute xmlns:w14 redefinedand "promise resolved instead of rejecting". After the fix, 5 of 5 pass in both packages.test:runran withSDX_SCHEMA_CORPUS_DIRset, and the gate (--self-test --known-failures coverage/emitted-schema-known-failures.json) then ran over that corpus: 2339 of 2339 instances validate. Nothing new failed.lint:workspaces,check:spec-coverage,check:conformance-citations,check:conformance-doc,check:emitted-schemaandcheck:real-corpus(4 files, 37 tests) all pass.test:runran while the host's load average was over 100. Every failure was a 5 s timeout except the two docx-core: a part that declares xmlns:w twice on one element throws a raw ParseError instead of loading or failing closed #1032 tests above. I fixed those tests, then reran each failing file once the load had dropped (docx-compare 23 files, docx-markdoc 10, docx-core 4): all pass.~/.cache/safe-docx-compare-fanout/word-staged/**/*.docx): 141 of 142 pass. The one failure,1471/word-staged/sdt-del-word-children.docx, is a schema error (customXmlDelRangeStartas a directw:sdtchild) that the base gate reports the same way. It predates this change and is not a well-formedness or Markup Compatibility finding.Left out
mc:ProcessContentasks to be processed. Only the attribute's syntax and scope are checked now. No emitted corpus uses ProcessContent.node --test scripts/check_emitted_document_schema.test.mjs. It runs insidenpm run check:emitted-schema, which is not a CI step. CI does run the gate's--self-test, which now covers the well-formedness pass. I did not edit the workflows.Fixes #1484