Skip to content

fix(docx-compare): remove accepted ghost section breaks - #981

Open
stevenobiajulu wants to merge 1 commit into
mainfrom
918-nvca-formatting-fidelity-20260914
Open

stevenobiajulu wants to merge 1 commit into
mainfrom
918-nvca-formatting-fidelity-20260914

Conversation

@stevenobiajulu

Copy link
Copy Markdown
Member

Summary

  • removes a ghost paragraph-level section break left when accept-all discards a w:sectPrChange snapshot whose container has no live section properties
  • preserves body-level final section properties and paragraph sections with live formatting
  • leaves the exact formatting-fidelity gate and thresholds unchanged
  • adds a strictly validated OpenSpec delta plus independently invented XML and built-DOCX regressions

Dependency of UseJunior/legal-context#918. This PR does not close that issue.

Exact revision

  • base (main): 31b1d8fd2e9f0f285cc6167906ef6e2c9f220699
  • head: 3b9f80095c753bcb4d99771b5adabf906fd0eebb

Evidence

  • invented pre-fix-equivalent invariant: formatting score 0.6666666666666666, with two divergent section units caused by the empty paragraph-owned section node
  • invented fixed regression: accept 1.0, reject 1.0
  • private verified real comparison: succeeded through the unchanged publication gate; accept 1.0, reject 1.0; zero run, paragraph, table, or section divergences and zero unaligned paragraphs
  • private source text, filenames, paths, and artifacts are not included here or in the commit/package

Verification

  • focused comparison tests: 69 passed
  • npm run build
  • npm run lint:workspaces (six pre-existing unused-disable warnings, zero errors)
  • full workspace tests; sandbox-only IPC/LibreOffice invocation failures were rerun under normal managed approval and passed (docx-core: 1,314 passed/1 skipped; render verifier: 56 passed)
  • npm run check:spec-coverage
  • npm run check:conformance-citations
  • npm run check:conformance-doc
  • openspec validate remove-accepted-ghost-section-break --strict
  • package audit: 12 tarballs / 1,207 entries, zero private sentinel hits

Review status

Supplemental same-model self-review found and fixed a predicate hardening issue by requiring the change node itself to be the sole direct element child. No independent-family review was available under the owner constraint; this self-review does not satisfy that independent gate.

READY for review. No merge, automerge, deployment, or publication is requested or authorized.

Accepting a tracked removal can leave an empty paragraph-owned sectPr after its revision snapshot is discarded. That node remains a semantic section break and causes the exact source-formatting publication gate to reject otherwise faithful comparisons. Remove only that narrowly identified container while preserving live and final section properties, and cover the behavior with invented XML and built-DOCX regressions.

Ref: UseJunior/legal-context#918
@vercel

vercel Bot commented Sep 14, 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 14, 2026 6:32pm UTC

Request Review

@github-actions github-actions Bot added the fix label Sep 14, 2026
@stevenobiajulu

Copy link
Copy Markdown
Member Author

Supplemental implementation self-review adjudication (same model; not an independent-family review):

  • Finding — predicate precision: child-count alone was not explicit enough about which child authorizes removal. Adjudicated/fixed: the implementation now requires the w:sectPrChange node itself to be the sole direct element child of a paragraph-owned w:sectPr.
  • Finding — preservation controls: an over-broad empty-container cleanup could remove final or live section properties. Adjudicated/fixed: dedicated regression coverage proves live paragraph section formatting and the body final section container survive acceptance.
  • Finding — synthetic evidence level: raw XML alone would not exercise package construction and the publication gate. Adjudicated/fixed: an independently invented built-DOCX comparison now runs end to end. Its pre-fix-equivalent invariant scores 0.6666666666666666; the fixed accept and reject projections both score 1.0.
  • Private verification: the verified real pair succeeds through the unchanged exact publication gate at accept 1.0 / reject 1.0, with zero formatting divergences and zero unaligned paragraphs. No source text, filenames, paths, or artifacts are disclosed.
  • Leakage: package audit covered 12 tarballs and 1,207 entries with zero private sentinel hits.

Local checks are complete. CI is currently running. Independent-family review is unavailable under the owner constraint; this same-model review is supplemental and does not satisfy that gate. No merge/automerge/deploy/publication is authorized.

@usejunior-llm-gate

Copy link
Copy Markdown

LLM gate (advisory)

1 finding to review (advisory, does not block) - 4 pass, 1 warn, 0 error, 11 skipped, 16 total

Findings (1)

Rule Verdict Detail
Re-derived facts vs canonical sources WARN The PR implements ghost section break cleanup in trackChangesAcceptorAst.ts:856-871 (acceptAllChanges) but does not apply this logic to the core acceptChanges primitive in accept_changes.ts:451-454, causing these sibling implementations to drift apart despite the requirement that they agree on all cases as noted in trackChangesAcceptorAst.test.ts:948-953.
All 16 rules (5 evaluated, 11 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 only modifies paragraph-level section break removal logic and does not touch field atomization, validateFieldStructure, w:fldChar, w:instrText, w:delInstrText, or collapsed-field comparison logic.
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 does not meet the precondition as it only touches AST candidate projection helpers in packages/docx-compare/src/tagged/trackChangesAcceptorAst.ts and does not modify DocxDocument, REVISION_STORY_PART_PATHS, accept_changes, reject_changes, or any side-part revision markup.
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 SKIPPED paths not touched by this PR
Table A / Table B boundary on side-part revisions SKIPPED paths not touched by this PR
Canonical-emission surface completeness SKIPPED paths not touched by this PR
Unit-test quality (avoid tautological / change-detector tests) PASS The newly added tests in packages/docx-compare/src/tagged/formattingFidelity.test.ts:267 and packages/docx-compare/src/tagged/trackChangesAcceptorAst.test.ts:229 are independent of the system under test, asserting concrete semantic XML structures and exact fidelity scores from first principles without relying on SUT mocks or behavioral snapshots.
Re-derived facts vs canonical sources WARN The PR implements ghost section break cleanup in trackChangesAcceptorAst.ts:856-871 (acceptAllChanges) but does not apply this logic to the core acceptChanges primitive in accept_changes.ts:451-454, causing these sibling implementations to drift apart despite the requirement that they agree on all cases as noted in trackChangesAcceptorAst.test.ts:948-953.
.openspec tag ↔ test-assertion drift PASS The added .openspec tag in review/packages/docx-compare/src/tagged/formattingFidelity.test.ts:269 matches a test body that fully exercises and asserts the scenario's GIVEN, WHEN, and THEN criteria without any tag-stuffing.
Library stays general (no downstream-domain leakage) SKIPPED paths not touched by this PR

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...docx-compare/src/tagged/trackChangesAcceptorAst.ts 85.71% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@stevenobiajulu

Copy link
Copy Markdown
Member Author

Final CI update: all required checks are complete and successful at head 3b9f80095c753bcb4d99771b5adabf906fd0eebb; the PR is READY and GitHub reports a clean merge state. Neutral/skipped advisory or deployment-only jobs are non-failing. No merge, automerge, deployment, or publication was performed.

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.

1 participant