Skip to content

fix(docx-core): clean only vacated selected paragraph-mark properties - #993

Merged
stevenobiajulu merged 4 commits into
mainfrom
982-clean-terminal-mark-properties-20260917
Sep 17, 2026
Merged

stevenobiajulu merged 4 commits into
mainfrom
982-clean-terminal-mark-properties-20260917

Conversation

@stevenobiajulu

Copy link
Copy Markdown
Member

Summary

Remove genuinely vacated paragraph-mark property containers after selected revision resolution, including terminal and table-cell cases. Preserve glyph formatting, foreign histories, attributes, namespace declarations, XML comments/PIs/CDATA and unrelated untracked empty containers. AST vacancy checks use the same conservative boundary without deleting arbitrary empty paragraphs.

Why / scope

Native resolution left empty pPr/rPr husks that comparison projection removed. The first cleanup candidate treated comments/PIs as vacancy; independent opposite-model red/green controls identified and repaired that loss. Capture selected mark/property-history paragraphs before revision removal, clean after resolution, and require attribute-free blank-text-only children.

One concern, no public API, renderer runtime dependency, protection mutation or hosted API work. Cleanup policy is explicitly implementation normalization, not a fabricated normative requirement. Reuse shared DOCX builders. The inherited pPrChange-only empty pPr case remains outside this consumed-mark repair; no global normalization promise.

Verification at 0f4a844

Reviewed7d0a3373 normally merged with mainca618780/PR988 before final gates; no force push or reviewed-history rewrite.

  • All six full root pre-submit gates exit0 (build, workspace lint, full tests, spec coverage, conformance citations, conformance doc).
  • Required single-worker real corpus21/21,23-row manifest unchanged (352.50s).
  • Thirty-three conservative cleanup controls: all four ins/del projection directions, body/cell/glyph/foreign histories, comments/PIs, selected-only property history and no-match byte identity.
  • Fresh built public NVCA terminal-mark edits: four projection directions restore the exact original main-story XML, preserving actual glyph properties. Eight complete source/resolved main-document schema controls validate. This real fixture exercises preservation, while the synthetic controls exercise true vacancy.
  • Fresh dynamic Claude Opus5 APPROVE both repair and exact main integration;222focused controls plus309property-history sweep controls pass. Fourteen well-formed combined schema projections validate; two deliberately malformed input ordering controls fail before and after and are not counted as valid.

Fresh full build removes the stale-dist risk identified during review; built public smoke runs against final dist, not only Vitest aliases.

Word Accept/Reject UNVERIFIED. Cleanup does not fix the separate terminal move reader limitation#973, the inherited section/page-selection difference, or selective paragraph-mark rPrChange foreign-marker loss now tracked in#991. XML metadata directly under a replaced pPr snapshot remains a separate inherited limitation, not a guarantee of this vacancy predicate.

Base freshness is checked before arming automerge. Exact merged-commit clean-build/full-suite/public DOCX/schema smoke and read-only production-docs merge-SHA verification remain required after landing. Primary checkout/evidence preserved; no cleanup/close-out while other follow-ups remain.

Fixes: #982
Ref: #941, #985, #984, #986, #991

Native terminal mark resolution left vacated paragraph-mark property containers while the AST projection removed them. Clean only selected resolved terminal marks, preserve attributes, remaining glyph formatting, foreign history, content-bearing terminal paragraphs, and the required final table-cell paragraph. Twelve independent body/cell and selective-author controls establish the intended bounded cleanup; no content-only paragraph deletion rule is introduced.

Fixes: #982
The initial terminal cleanup treated XML comments and processing instructions as empty and missed mirror revision directions. Capture selected paragraph-mark histories before resolution, then remove only attribute-free containers containing no nodes beyond blank text. Apply the same conservative vacancy rule to AST projection rather than using agreement with its inherited metadata loss as proof. Independent controls now cover all four ins/del directions, comments, processing instructions, and history-only selection while keeping foreign metadata untouched.

Ref: #982
@vercel

vercel Bot commented Sep 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
site Ready Ready Preview Sep 17, 2026 9:54pm UTC

Request Review

@usejunior-llm-gate

usejunior-llm-gate Bot commented Sep 17, 2026

Copy link
Copy Markdown

LLM gate (advisory)

All evaluated rules passed - 8 pass, 0 warn, 0 error, 8 skipped, 16 total

Findings

None.

All 16 rules (8 evaluated, 8 skipped)
Rule Verdict Detail
read_file response metadata parity SKIPPED paths not touched by this PR
Live DOM namespace-safe OOXML writes SKIPPED paths not touched by this PR
Complex-field revisions preserve complete accept/reject state machines PASS The PR does not touch field atomization, validateFieldStructure, w:fldChar, w:instrText, w:delInstrText, or collapsed-field comparison logic, and instead focuses on terminal paragraph-mark property cleanup.
Field validation per story, not global SKIPPED paths not touched by this PR
Revision IDs seeded from all revision-bearing side parts SKIPPED paths not touched by this PR
Accept/reject sweep side parts and caches PASS The PR correctly implements terminal mark property cleanup in packages/docx-core/src/primitives/accept_changes.ts:366-370,498 and packages/docx-core/src/primitives/reject_changes.ts:464-466,693 without affecting side-story sweeps, XML caching, or separator-preserving footnote pruning as defined in packages/docx-core/src/primitives/document.ts:430-520.
DocumentViewNode.heading stays canonical SKIPPED paths not touched by this PR
AI-author parity across entry points SKIPPED paths not touched by this PR
Property-change wrapper discipline SKIPPED paths not touched by this PR
SUPPORT.md Table A drift vs. implementation PASS The PR only modifies revision resolution and formatting cleanup in internal consumer files (accept_changes.ts, reject_changes.ts, and paragraph_merge_formatting.ts) which are classified as non-contract utilities in SUPPORT.md, and does not alter the revision-emitting surface in Table A.
Table A / Table B boundary on side-part revisions SKIPPED paths not touched by this PR
Canonical-emission surface completeness PASS The PR only modifies track-changes acceptance and rejection resolution logic in packages/docx-core/src/primitives/accept_changes.ts:289 and reject_changes.ts:290 to perform empty property cleanup, and does not add or change any write-time tracked-edit emission surface or MCP tools from Table A.
Unit-test quality (avoid tautological / change-detector tests) PASS The added test file packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts contains high-quality, independent assertions constructed from first principles (such as paragraph counts, text content, and comment preservation on lines 27-31, 38-40, 48-51, 62-63, 72-75) that make concrete semantic claims without mocking the SUT or relying on tautological snapshots.
Re-derived facts vs canonical sources PASS The PR implements parallel element-emptying logic with inline vacated helper functions in packages/docx-compare/src/tagged/trackChangesAcceptorAst.ts:100 and packages/docx-core/src/primitives/paragraph_merge_formatting.ts:18. This duplication is explicitly tested and pinned in exact parity by the newly added packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts test suite.
.openspec tag ↔ test-assertion drift PASS The PR only adds a new test file using 'test.conformance' for ECMA-376 and does not add, move, or change any '.openspec' tags.
Library stays general (no downstream-domain leakage) PASS The PR introduces removeEmptyParagraphMarkProperties in packages/docx-core/src/primitives/paragraph_merge_formatting.ts:13, which uses strictly generic OOXML vocabulary and avoids any downstream-domain concepts.

@stevenobiajulu

Copy link
Copy Markdown
Member Author

Advisory adjudication for head 0f4a8446c77e666ec2e3c6ee85677eba3bbc94a8:

The duplicated vacancy predicate warning is a real, low-severity maintenance tradeoff, not a missed canonical package/source walk. Both implementations deliberately use the same conservative rule: no attributes, and only whitespace text children. This PR does not introduce another source of revision IDs, package parts or conformance facts.

The native traversal is selected-revision-scoped; the comparison traversal operates on its own projected tree. Keeping the small predicate local avoids expanding this focused consumer cleanup into a shared projection-policy refactor. The 33 independent controls include attributes, namespaces, comments, processing instructions, CDATA, foreign/unselected history and real glyph properties; native/AST parity is asserted against concrete expected outcomes. Fresh Opus review and all six local gates plus the required 21-test corpus pass on this integrated head.

The warning is acknowledged, not silently counted as PASS. No blocker was found by independent review. Inherited selective property-history loss is tracked in #991, and Word projections remain UNVERIFIED.

@stevenobiajulu

Copy link
Copy Markdown
Member Author

Shipping held: required workspace-test (22) failed and workspace-test (20) was cancelled. Failure logs are retained locally at /private/tmp/993-node22-ci-failure-20260917.log. Investigating the exact failure before retrying or changing code; automerge is not armed. No failing gate is being bypassed.

@stevenobiajulu

Copy link
Copy Markdown
Member Author

The unchanged-head retry reproduced the same existing real-ILPA annotation test timeout (30 seconds); no assertion mismatch was reported. Further retries are stopped. Preparing a separate focused test-only blocker repair that registers the same two-source/two-destination combinations as four individually bounded cases, preserving every assertion and the 30-second per-case timeout. All21 annotation tests pass locally. This cleanup PR stays unarmed; it will integrate the blocker through normal main history and repeat exact-head review/local gates before shipping. No test skip, timeout increase, runtime change or protection bypass.

stevenobiajulu added a commit that referenced this pull request Sep 17, 2026
Two consecutive Node22 CI runs time out while one test aggregates two real ILPA sources and two destination projections under a single thirty-second limit. Register the same four source/destination combinations as individual cases so a slow runner can complete and report each one. Keep every hyperlink, named-style, size and destination-relationship assertion and the thirty-second per-case bound; do not skip inputs, raise timeouts, weaken assertions or change compiler/runtime behavior. The full annotation file now reports twenty-one passing tests instead of eighteen, with each public projection independently accountable. Keep this shipping-blocker repair separate from the focused native cleanup PR.

Ref: #982, #993
Preserve reviewed history while incorporating main's independently bounded public ILPA annotation cases. Repeat final integration gates and review before pushing this focused runtime repair; do not rewrite or bypass protected history.

Ref: #982
@stevenobiajulu

Copy link
Copy Markdown
Member Author

Evidence-label correction to the earlier advisory adjudication: the 33 checked-in controls cover comments, processing instructions, glyph color, foreign-author history and last-cell paragraphs. Attribute-bearing containers, foreign namespaces and CDATA were exercised separately by dynamic Opus reviewer probes; they are not part of the 33-control count. The acknowledged duplicated-predicate maintenance tradeoff remains unchanged.

The latest integrated head is still held for a fresh clean release-gate run. One prior full run timed out in an unchanged comparison-emission test while verification sessions overlapped; that red log is retained and is not passing evidence. Both supervisor and Opus isolated runs passed the affected file. Word projections remain UNVERIFIED.

@stevenobiajulu

Copy link
Copy Markdown
Member Author

Final local release evidence for head 5b880dc409af8743d2dc798eca6cdbc1e59de119, normally integrated with main 24b749524c62f8bf1c9852736482801a129c4b5c / shipped CI blocker #995:

  • All six required local gates passed; 3,462 tests passed and 47 platform/opt-in skips. The clean full core suite passed 1,328 tests with one skip, replacing—not relabeling—the retained earlier timeout failure. No timeout, assertion, exclusion or CI configuration was weakened.
  • Required public corpus: all 21 tests passed, including all 23 reviewed manifest rows; no absent-corpus skip. Complete controlled one-worker log: /private/tmp/982-integration-995-controlled-presubmit-20260917.log.
  • Independent dynamic Claude Opus 5: APPROVE, exact final head verified; actual 33-control execution, 57 additional adversarial probes, main-vs-head/property-snapshot interaction controls and 10 fresh input/output schema controls. The required clean full-suite evidence is now supplied. Complete local report: .peer-review/982-integration-995-opus5-20260917.review.txt.
  • Fresh built-code public SHA-pinned NVCA management-rights letter: all four terminal mark directions restore exact original main-story XML while preserving genuine glyph properties; all eight tracked/resolved documents pass the repository MCE-aware schema gate. True empty-container vacancy is separately exercised by synthetic regression controls; the real fixture is not falsely described as a vacancy case.
  • The low-severity duplicate-predicate maintenance tradeoff remains acknowledged. The earlier evidence wording was corrected to distinguish the 33 checked-in controls from additional reviewer probes. Inherited property-history limitations remain tracked in fix(docx-core): preserve foreign paragraph-mark revisions during property rejection #991; Word projections remain UNVERIFIED. Public/synthetic inputs only; no private documents or hosted API work.

This is pre-merge evidence, not a claim that fresh CI or post-merge smoke has passed. Automerge remains unarmed pending fresh CI/advisory/legacy deployment checks. Exact-merge clean-build real-document smoke and production SHA verification still required after merge.

@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ackages/docx-core/src/primitives/accept_changes.ts 75.00% 1 Missing ⚠️
...-core/src/primitives/paragraph_merge_formatting.ts 85.71% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@stevenobiajulu
stevenobiajulu merged commit 9d82bab into main Sep 17, 2026
27 checks passed
@stevenobiajulu
stevenobiajulu deleted the 982-clean-terminal-mark-properties-20260917 branch September 17, 2026 22:25
@stevenobiajulu

Copy link
Copy Markdown
Member Author

Post-merge smoke passed

Merged and freshly built in an isolated detached worktree: 9d82bab96ec3bd23c772a1bc2792cfbabc146652 (PR #993 / #982). Primary checkout untouched; merge tree verified identical to reviewed head 5b880dc4.

Own locked dependencies and all six required gates passed: build, workspace lint, full suite 3,462 passed / 47 platform or opt-in skips, spec coverage, conformance citations and conformance document. All process exits collected as 0.

Real-world fixture: public NVCA Management Rights Letter, SHA-verified against this exact merge's manifest. Fresh built public native API Accept/Reject in all four terminal ins/del directions restores the exact main-story XML and preserves original paragraph-mark glyph properties. All 8/8 tracked/resolved main-story schema controls validate. The real fixture has genuine glyph properties; synthetic regressions separately exercise genuinely vacant containers, comments, PIs, foreign history and required cell paragraphs.

Actual LibreOffice Accept/Reject dispatches used the unresolved tracked packages, compared with independently imported original identity: all four outputs match all 33 paragraph/heading texts. Native pre-projected files were not substituted as reader controls. Word projection remains UNVERIFIED; reader text agreement is not a full formatting or Word oracle.

Visual evidence: skipped — conservative removal of vacated property containers is structurally invisible; original glyph properties are preserved exactly.

Production docs/redirect deployment: READY, actual dpl_3HeTcs2hkw2xVpYUesfpzYSS6Y2Z, metadata githubCommitSha=9d82bab96ec3bd23c772a1bc2792cfbabc146652, githubCommitRef=main, verified read-only. No hosted API build/deployment or branch-protection change.

Local log: /private/tmp/automerge-smoke-993-20260917.log; local public-only artifacts/production receipt: /private/tmp/automerge-smoke-993-20260917/.peer-review/. No documents or renders uploaded. Worktrees retained because other follow-ups remain open; no cleanup/close-out.

First 50 log lines
9d82bab96ec3bd23c772a1bc2792cfbabc146652
npm warn deprecated node-domexception@1.0.0: Use your platform's native DOMException instead

added 617 packages, and audited 630 packages in 2s

155 packages are looking for funding
  run `npm fund` for details

7 vulnerabilities (6 moderate, 1 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
npm warn install-scripts 2 packages have install scripts not yet covered by allowScripts:
npm warn install-scripts   esbuild@0.28.1 (postinstall: node install.js)
npm warn install-scripts   fsevents@2.3.3 (install: (install scripts present))
npm warn install-scripts
npm warn install-scripts Run `npm install-scripts ls` to review, or `npm install-scripts approve <pkg>` to allow.

> safe-docx-suite@0.20.1 build
> npm run build --workspaces --if-present


> @usejunior/docx-compare@0.20.1 build
> npm run build -w @usejunior/docx-core && npm run clean:dist && tsc -p tsconfig.build.json


> @usejunior/docx-core@0.20.1 build
> npm run clean:dist && tsc -p tsconfig.build.json


> @usejunior/docx-core@0.20.1 clean:dist
> node -e "const { rmSync } = require('node:fs'); rmSync('dist', { recursive: true, force: true });"


> @usejunior/docx-compare@0.20.1 clean:dist
> node -e "const { rmSync } = require('node:fs'); rmSync('dist', { recursive: true, force: true });"


> @usejunior/docx-core@0.20.1 build
> npm run clean:dist && tsc -p tsconfig.build.json


> @usejunior/docx-core@0.20.1 clean:dist
> node -e "const { rmSync } = require('node:fs'); rmSync('dist', { recursive: true, force: true });"

Last 50 log lines
PASS add-safe-docx-batch-apply-and-strict-anchor-resolution: 6 scenarios covered by 6 story mappings
PASS add-safe-docx-docx-helper-tools: 11 scenarios covered by 11 story mappings
PASS add-safe-docx-layout-format-controls: 7 scenarios covered by 7 story mappings
PASS add-safe-docx-ts-formatting-parity: 11 scenarios covered by 11 story mappings
PASS add-section-break-insertion: 3 scenarios covered by 3 story mappings
PASS add-section-page-numbering-formatting: 8 scenarios covered by 17 story mappings (+9 bonus tests beyond spec)
PASS add-section-page-setup-formatting: 8 scenarios covered by 8 story mappings
PASS add-selective-ai-accept-reject: 6 scenarios covered by 6 story mappings
PASS add-table-context-to-document-view: 4 scenarios covered by 4 story mappings
PASS add-text-export: 5 scenarios covered by 5 story mappings
PASS add-typescript-mcp-server: 21 scenarios covered by 21 story mappings
PASS document-paragraph-id-stability-and-fingerprint: 9 scenarios covered by 9 story mappings
PASS expose-comment-range-metadata: 4 scenarios covered by 4 story mappings
PASS forbid-untracked-ai-mutations: 8 scenarios covered by 8 story mappings
PASS guard-row-level-revision-resolution: 5 scenarios covered by 5 story mappings
PASS remove-comparison-from-default-save: 3 scenarios covered by 3 story mappings
PASS replace-plan-tools-with-batch-edit: 12 scenarios covered by 12 story mappings
PASS update-safe-docx-save-defaults-and-stable-node-ids: 12 scenarios covered by 12 story mappings
PASS update-safe-docx-sessionless-entry-and-session-controls: 11 scenarios covered by 12 story mappings (+1 bonus tests beyond spec)

> @usejunior/docx-core@0.20.1 check:spec-coverage-generation
> node scripts/validate_generation_openspec_coverage.mjs --strict

PASS docx-generation (canonical): 34 scenarios covered (+32 bonus tests beyond spec)
PASS add-custom-theme: 1 scenarios covered
PASS add-docx-generation: 34 scenarios covered
PASS add-generation-ancillary-parts: 1 scenarios covered (+2 bonus tests beyond spec)
PASS add-generation-baseline-settings: 1 scenarios covered
PASS add-generation-compare-roundtrip: 5 scenarios covered
PASS add-numbering-level-justification: 1 scenarios covered
PASS add-paragraph-borders: 2 scenarios covered
PASS add-run-highlight: 1 scenarios covered
PASS add-signature-and-keeplines: 1 scenarios covered
PASS remove-agreement-domain-recipes: 0 scenarios covered

> safe-docx-suite@0.20.1 check:conformance-citations
> node scripts/check_conformance_citations.mjs

check_conformance_citations: OK (121 registry entries, 51 XSDs)

> safe-docx-suite@0.20.1 check:conformance-doc
> node scripts/check_conformance_doc.mjs

generate_conformance_doc: wrote spec-compliance/CONFORMANCE.md
check_conformance_doc: OK
{"builtRoot":"/private/tmp/automerge-smoke-993-20260917","smoke":"982","publicFixtureOnly":true}
Public source SHA pin verified against exact-merge manifest.
PASS fresh built public API: four terminal mark directions restore the exact NVCA main story, preserving all original glyph properties
8 of 8 document.xml instances validate against the Transitional WML schema
{"fixture":"SHA-pinned public NVCA Management Rights Letter","reader":"actual unresolved tracked inputs with independent identity control","projections":["accept-del","accept-ins","reject-del","reject-ins"],"paragraphs":[33,33,33,33,33],"Word":"UNVERIFIED"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(docx-core): characterize terminal paragraph-mark cleanup parity

1 participant