Skip to content

Add explicit review and comment export profiles - #508

Closed
JSv4 wants to merge 20 commits into
agent/issue-443-pdf-fidelity-cleanfrom
agent/issue-444-review-comment-profiles
Closed

Add explicit review and comment export profiles#508
JSv4 wants to merge 20 commits into
agent/issue-443-pdf-fidelity-cleanfrom
agent/issue-444-review-comment-profiles

Conversation

@JSv4

@JSv4 JSv4 commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Closes #444

Summary

  • Add explicit final, original, and markup review profiles plus hidden, inline, endnotes, and margin comment profiles across the browser API, Node API, CLI, worker, and WASM bridge.
  • Preserve revision authorship/dates and comment bodies, ranges, replies, authors, dates, and resolution status while keeping source package bytes immutable.
  • Inventory auxiliary revision stories for export diagnostics, including comments, glossary, and styles, without broadening the historical mutation API.
  • Fail closed or warn deterministically for unsupported revision families, malformed/ambiguous comment topology, missing fragment targets, and clipped margin threads.
  • De-duplicate comment definitions and targets cloned through repeated headers/footers.
  • Align the projection path with the corrected Add deterministic OOXML package manifests #493 behavior already present in this stack.

Verification

  • Full .NET suite: 3,864 passed, 3 skipped
  • Focused review/projection/HTML suite: 81 passed
  • Chromium review/pagination/print/export suite: 27 passed
  • Node/CLI/Chromium/PDF exporter suite: 22 passed
  • Generated-PDF fidelity ratchet: 10/10 cases passed; semantics and geometry passed
  • Production WASM build: 3.38 MB Brotli / 4 MB budget
  • Package boundary: 175 runtime/license files

Viewable evidence

CI uploads playwright-report even if a later test fails. It now contains both npm/test-artifacts/ and npm-export/test-artifacts/.

Local viewers from the final run:

  • npm/test-artifacts/review-comment-profiles/index.html — all 12 browser profile combinations
  • npm-export/test-artifacts/view-artifacts.html — Node/PDF matrix plus warn/strict/fail-closed evidence
  • /tmp/docxodus-444-release-artifacts-2/index.html — complete generated-PDF ratchet evidence

JSv4 commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

Follow-up f3fda3d closes the #465 integration edge discovered after #493 landed:

  • adds reviewProfileAlreadyApplied for exact final/original packages across browser, worker, Node, framed host, CLI, schema, and docs;
  • inventories the supplied package and fails closed if any tracked revision remains;
  • preserves the caller's exact bytes/digest and omits derivedProfileSource;
  • rejects the declaration for markup and leaves default profile behavior unchanged.

Verification:

  • browser review/comment suite: 4/4 (including the 12-profile matrix)
  • Node/CLI/framed-host suite: 23/23
  • browser and Node typechecks: pass
  • browser package boundary: 175 files; Node package boundary: 51 files

New viewable evidence is generated at npm/test-artifacts/profile-resolved-sources/index.html and npm-export/test-artifacts/success/profile-resolved-sources/index.html; both live under the paths already uploaded by CI.

@JSv4
JSv4 force-pushed the agent/issue-444-review-comment-profiles branch from f3fda3d to 2672eee Compare August 17, 2026 04:34
@JSv4
JSv4 force-pushed the agent/issue-443-pdf-fidelity-clean branch from cf5d7aa to 13ae323 Compare August 17, 2026 04:34
@JSv4
JSv4 force-pushed the agent/issue-444-review-comment-profiles branch from 2672eee to 130b2b0 Compare August 17, 2026 04:58
@JSv4
JSv4 force-pushed the agent/issue-443-pdf-fidelity-clean branch from cc46333 to 61b62d5 Compare August 18, 2026 04:55

JSv4 commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

Closing as part of retiring the #505#506#507#508 stack — see the rationale on #505. The stack forked before #493/#500/#501 landed and re-adds merged work; this tip is also conflicted against its base.

#444 is being redone against main as the last of four serial PRs. It is the smallest of the four in real terms: --review-profile / --comments are already wired end-to-end on mainhandleProjectReviewProfile routes to AcceptRevisions/RejectRevisions, and commentProfile maps to CommentRenderMode.{Disabled,Inline,EndnoteStyle,Margin} in npm/src/export-browser.ts. What remains is hardening, not plumbing: unsupported-family warnings, comment-topology validation, dedup of definitions cloned through repeated headers/footers, and margin-thread clipping. The branch is kept as the archive of record.

Per #437's design doc, #443 only becomes the release gate once this has landed, so a reviewed ratchet refresh for the comment-bearing fixtures is budgeted into it.


Generated by Claude Code

@JSv4 JSv4 closed this Aug 21, 2026
JSv4 pushed a commit that referenced this pull request Aug 21, 2026
The browser-page benchmark has rastered through Poppler for some time; the PDFs
`@docxodus/export` actually produces were never measured. This routes them
through the same raster contract over a ten-document pinned corpus with recorded
provenance.

The gates are layered deliberately. Conversion, page count, physical geometry,
semantic content and the chart-vector contract are unconditional — no raster
severity or attribution disposition can waive them, because a document that
converted to the wrong number of pages is broken regardless of how similar those
pages look. SSIM and ink metrics ratchet separately against a numbers-only
record, and text and link extraction are gated independently so a text
regression cannot hide behind an acceptable raster score.

An environment fingerprint covering LibreOffice, Chromium, Poppler and the font
contract is what keeps the ratchet honest: a changed environment reports
`environment-changed` rather than being misattributed to the renderer.

The committed record's numbers were recorded on the retired #505-#508 stack,
whose barrier and font runtime differ from the ones that landed, so they are
carried over as a starting point and annotated as such. The first complete run
on this lineage will report `environment-changed` and needs a re-record. The
unconditional gates do not depend on those numbers and are live now.

The benchmark stays `workflow_dispatch`-only. The salvaged version ran on every
pull request, but per the design doc this becomes the release gate only once
#444 has landed its pixel-moving changes and the comment-bearing fixtures have
had their reviewed refresh; until then a 90-minute LibreOffice and Poppler
traversal per pull request buys signal the ratchet cannot act on.

Building the corpus surfaced a real converter defect. A visually empty paragraph
was synthesized with a normal space, which is eligible for the converter's own
leading/trailing-whitespace suppression and could reach the browser as an empty
span. That removed the paragraph-mark line box and left a canonical paragraph
anchor with zero geometry, so the strict PageMap rejected otherwise-valid
documents — signature-table spacer rows most visibly. The placeholder now uses a
non-breaking space, confined to that synthesized run.
JSv4 pushed a commit that referenced this pull request Aug 21, 2026
The browser-page benchmark has rastered through Poppler for some time; the PDFs
`@docxodus/export` actually produces were never measured. This routes them
through the same raster contract over a ten-document pinned corpus with recorded
provenance.

The gates are layered deliberately. Conversion, page count, physical geometry,
semantic content and the chart-vector contract are unconditional — no raster
severity or attribution disposition can waive them, because a document that
converted to the wrong number of pages is broken regardless of how similar those
pages look. SSIM and ink metrics ratchet separately against a numbers-only
record, and text and link extraction are gated independently so a text
regression cannot hide behind an acceptable raster score.

An environment fingerprint covering LibreOffice, Chromium, Poppler and the font
contract is what keeps the ratchet honest: a changed environment reports
`environment-changed` rather than being misattributed to the renderer.

The committed record's numbers were recorded on the retired #505-#508 stack,
whose barrier and font runtime differ from the ones that landed, so they are
carried over as a starting point and annotated as such. The first complete run
on this lineage will report `environment-changed` and needs a re-record. The
unconditional gates do not depend on those numbers and are live now.

The benchmark stays `workflow_dispatch`-only. The salvaged version ran on every
pull request, but per the design doc this becomes the release gate only once
#444 has landed its pixel-moving changes and the comment-bearing fixtures have
had their reviewed refresh; until then a 90-minute LibreOffice and Poppler
traversal per pull request buys signal the ratchet cannot act on.

Building the corpus surfaced a real converter defect. A visually empty paragraph
was synthesized with a normal space, which is eligible for the converter's own
leading/trailing-whitespace suppression and could reach the browser as an empty
span. That removed the paragraph-mark line box and left a canonical paragraph
anchor with zero geometry, so the strict PageMap rejected otherwise-valid
documents — signature-table spacer rows most visibly. The placeholder now uses a
non-breaking space, confined to that synthesized run.
JSv4 added a commit that referenced this pull request Aug 23, 2026
The browser-page benchmark has rastered through Poppler for some time; the PDFs
`@docxodus/export` actually produces were never measured. This routes them
through the same raster contract over a ten-document pinned corpus with recorded
provenance.

The gates are layered deliberately. Conversion, page count, physical geometry,
semantic content and the chart-vector contract are unconditional — no raster
severity or attribution disposition can waive them, because a document that
converted to the wrong number of pages is broken regardless of how similar those
pages look. SSIM and ink metrics ratchet separately against a numbers-only
record, and text and link extraction are gated independently so a text
regression cannot hide behind an acceptable raster score.

An environment fingerprint covering LibreOffice, Chromium, Poppler and the font
contract is what keeps the ratchet honest: a changed environment reports
`environment-changed` rather than being misattributed to the renderer.

The committed record's numbers were recorded on the retired #505-#508 stack,
whose barrier and font runtime differ from the ones that landed, so they are
carried over as a starting point and annotated as such. The first complete run
on this lineage will report `environment-changed` and needs a re-record. The
unconditional gates do not depend on those numbers and are live now.

The benchmark stays `workflow_dispatch`-only. The salvaged version ran on every
pull request, but per the design doc this becomes the release gate only once
#444 has landed its pixel-moving changes and the comment-bearing fixtures have
had their reviewed refresh; until then a 90-minute LibreOffice and Poppler
traversal per pull request buys signal the ratchet cannot act on.

Building the corpus surfaced a real converter defect. A visually empty paragraph
was synthesized with a normal space, which is eligible for the converter's own
leading/trailing-whitespace suppression and could reach the browser as an empty
span. That removed the paragraph-mark line box and left a canonical paragraph
anchor with zero geometry, so the strict PageMap rejected otherwise-valid
documents — signature-table spacer rows most visibly. The placeholder now uses a
non-breaking space, confined to that synthesized run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant