d-flex flex-column width-full">

test(docx-core): count-guard duplicate w16cid:commentId rows in merge…

4e64c51

Select commit

Loading

Failed to load commit list.

Uh oh!

There was an error while loading. Please reload this page.

Vercel

Vercel Preview Comments

PR Title Lint on: pull_request_target

Validate conventional title

Apply type label from PR title

Lean Build on: pull_request

lean-build

LLM-Based Quality Gate on: pull_request

Parse checklist

Check 01 — **`read_file` response metadata parity**: If this PR touches `packages/docx-mcp/src/tools/read_file.ts`, budgeted pagination returns, or additive response metadata like `warnings` / `comment_load_error`, do every successful return path (default budgeted early return, non-budget fallthrough, explicit `limit`/`node_ids`) preserve the same additive diagnostic fields? `read_file` has multiple success exits; diagnostics have already disappeared on one path before. Reference: #180 surfaced `comment_load_error`, #186 added an early budget return + warnings, #191 fixed the missing `comment_load_error` on the default budgeted path.

Check 02 — **Live DOM namespace-safe OOXML writes**: If this PR touches `packages/docx-core/src/primitives/comments.ts` or writes prefixed OOXML attributes/elements (`w14:*`, `w15:*`, `xmlns:*`, `comments.xml`, `commentsExtended.xml`, `people.xml`), are prefixed OOXML names written with namespace-aware APIs — root aliases bound with `setAttributeNS(XMLNS_NS, ...)`, prefixed attributes with `setAttributeNS(W14_NS/W15_NS, ...)`, and is there a test that proves the live DOM works before serialization/reparse? String-prefixed attributes can serialize plausibly while the live DOM still throws namespace errors. Reference: #180 (xmlns:w14/w15 declared on comments root before writing prefixed attrs).

Check 03 — **Deleted field markup keeps `w:fldChar` outside `w:del`**: If this PR touches field atomization, `validateFieldStructure`, `hasFldCharInsideDel`, `w:fldChar`, `w:instrText`, `w:delInstrText`, or collapsed field comparison logic, does deleted field output stay ECMA-376-conformant — `w:fldChar` sibling-level (never inside `w:del`), deleted instructions use `w:delInstrText` only inside valid delete wrappers, accept/reject safety checks still reject malformed combined output? Word treats deleted field-state markup in the wrong container as document-corrupting. References: #211, #225, #228.

Check 04 — **Field validation per story, not global**: If this PR touches `packages/docx-core/src/baselines/atomizer/pipeline.ts`, `splitStories`, `validateFieldStructure`, side-part merge logic, or footnote/endnote field handling, is field validation run independently per ECMA story (`document.xml`, each footnote, each endnote), with sidecars from both original and revised archives considered, and global counter balance not treated as sufficient? A document can be globally balanced but have an invalid field sequence inside one story. References: #225, #228, #218.

Check 05 — **Revision IDs seeded from all revision-bearing side parts**: If this PR touches `packages/docx-mcp/src/session/manager.ts` (especially `getRevisionContextForSession` or `FIXED_REVISION_ID_SEED_PARTS`), `createRevisionContext`, revision-ID allocation, or MCP tools that create tracked changes/comments/footnotes, does revision-ID allocation scan all relevant package parts before issuing new IDs — comments, footnotes, endnotes, glossary, headers, footers — ignore non-revision `w:id` values (comment IDs, bookmarks), and handle malformed optional parts gracefully? Revision IDs are package-wide; document-only seeding collides with existing side-part revisions. Reference: #216 (seed revision ids from side parts).

Check 06 — **Accept/reject sweep side parts and caches**: If this PR touches `DocxDocument.acceptChanges`, `DocxDocument.rejectChanges`, `REVISION_STORY_PART_PATHS`, `accept_changes`, `reject_changes`, or side-part revision markup, does accept/reject process every revision-bearing story — updating `document.xml` + `footnotes.xml` + `endnotes.xml` + `comments.xml`, writing back only changed side parts while refreshing cached XML, and pruning orphan footnotes without deleting reserved separator entries? Accepting only in the main document leaves stale revisions and dangling references in the package. References: #218, #216, #225.

Check 07 — **`DocumentViewNode.heading` stays canonical**: If this PR touches `packages/docx-core/src/primitives/document_view.ts`, `HeadingValue`, heading heuristics, `ListMetadata.header_style`, or Google Docs document-view heading normalization, does `node.heading` remain a structural heading signal — exact Word styles `Heading1`–`Heading6` win, heuristic sources suppressed inside table cells while real Word heading styles still pass, ordinary body paragraphs omit the `heading` key? Consumers use `node.heading != null` as a structural test; heuristic false positives break downstream navigation. References: #178, #188, #190.

Check 08 — **AI-author parity across entry points**: If this PR touches `packages/docx-mcp/src/server.ts`, `packages/docx-mcp/src/cli/tool_runner.ts`, `packages/docx-mcp/src/cli/commands/**`, or adds any new `new SessionManager(...)` call site in `docx-mcp`, does every entry point that constructs a `SessionManager` resolve `SAFE_DOCX_AI_AUTHOR` with the same three-way semantics (set → use it; empty string → opt out to untracked; unset → `defaultAiAuthor`), or has a new entry path silently bypassed tracked emission? Each entry path looks locally correct while diverging from another; tracked emission has gone dark in one path before anyone noticed. References: #172 (production MCP wiring would have kept tracked emission dark), #182 (CLI runners constructing bare `SessionManager()` silently produced untracked edits).

Check 09 — **Property-change wrapper discipline**: If this PR touches `packages/docx-core/src/primitives/layout.ts`, `packages/docx-core/src/primitives/text.ts`, `packages/docx-mcp/src/tools/clear_formatting.ts`, or `packages/docx-core/src/primitives/track-changes-emitter.ts`, do tracked formatting/property edits emit exactly one correct `*PrChange` wrapper (`pPrChange` / `rPrChange` / `trPrChange` / `tcPrChange`) carrying a snapshot of the prior live properties — not stacking stale wrappers, not stripping valid historical children (`cellIns`/`cellDel`/`cellMerge`), and not omitting the snapshot when the operation is formatting-aware? Emitted OOXML is visually plausible but subtle snapshot mistakes only surface during later accept/reject or in Word's tracked-changes UI. References: #167 (duplicate `pPrChange`/`trPrChange`/`tcPrChange` stacking + over-broad `tcPr` exclusion), #170 (`clear_formatting` failing to strip stale `rPrChange`), #215 (`rPrChange` for formatted paragraph replacements + filtering nes

Check 10 — **`SUPPORT.md` Table A drift vs. implementation**: If this PR modifies OOXML revision emission behavior (`w:ins`, `w:del`, `w:rPrChange`, etc.) in `packages/docx-core/src/primitives/**`, or touches `packages/docx-core/SUPPORT.md`, does the PR symmetrically update Table A in `SUPPORT.md` when the supported revision-emission surface in primitives changed — added, removed, or weakened — or is the documented contract now lying about what's supported? Reviewers focus on TS AST correctness and golden tests; Markdown contract tables get treated as an afterthought, so the documented surface drifts from the actual surface. Reference: #143 review caught #173 (formatting mismatch in Table A) and #174 (comment body revision omission forcing a Table A softening) late in peer review.

Check 11 — **Table A / Table B boundary on side-part revisions**: If this PR touches `packages/docx-core/src/primitives/comments.ts`, `packages/docx-core/src/primitives/footnotes.ts`, or other side-part primitives, and adds/changes revision markup (`w:ins`, `w:del`), does tracked-change revision logic stay scoped to Table A (document-body content inside the side part) without leaking revision markup into Table B (the side-part package bootstrap — `comments.xml`/`footnotes.xml` element registration itself)? Body runs and side-part package elements share nearly identical XML namespace schemas; revisions emitted in the wrong table corrupt the package contract while looking plausible. References: #138 (comment-body straddle constraints), #139 (footnote-reference straddle constraints).

Check 12 — **Canonical-emission surface completeness**: If this PR adds or changes a tracked-edit surface in `packages/docx-core/src/primitives/**` or `packages/docx-mcp/src/tools/**`, are the paired artifacts updated together — `packages/docx-core/src/integration/canonical-emission-regression.test.ts`, `packages/docx-mcp/src/integration/canonical-emission-mcp.test.ts`, and the documented emitter surface (Table A) — or is the rollout only partially wired? The primitive change looks done before the MCP path, regression matrix, and documented contract are wired through; partial rollouts ship undocumented surface that drifts. References: #172 (`RevisionContext` threaded through every Table A MCP tool), #175 (24-test regression suite + verified write-time emitter rows), #215 (re-enabled `rPrChange` regression + updated support surface for `replaceParagraphTextRange`).

Check 13 — **Lean predicate drift against engine semantics (asymmetric)**: If this PR changes field-wrapper semantics, the proof boundary, or atomizer behavior — `packages/docx-core/src/baselines/atomizer/**`, `verification/lean/LeanSpike/Spec.lean`, `verification/lean/Tier2/**`, or `packages/docx-core/src/integration/lean-spec-bridge.test.ts` — and if the TS engine semantics shifted, did the PR also update the Lean residual predicate and bridge tests, or is the proof now pinned to a stale stronger/weaker assumption? Asymmetric: a TS change without a corresponding Lean update is WARN; a Lean-only change without a TS update should not fire. The Lean side can still compile while the abstraction boundary is subtly wrong for the next engine refactor. References: #208 (closed `inv_field_001` using stronger `recursivelyWellformed`), #220 (weakened the axiom to document-level `preservationFriendly` to avoid breakage when field fragmentation lands).

Check 14 — **Unit-test quality (avoid tautological / change-detector tests)**: If this PR adds or modifies any `**/*.test.ts` (or other test files), are the test assertions independent of the system under test — expected values constructed from first principles rather than re-derived from the function under test, mocks limited to external boundaries (filesystem, network, clocks) rather than mocking the SUT itself, assertions making concrete semantic claims rather than just snapshotting current behavior or asserting non-null, and any test added alongside a bug fix actually exercising the bug? Tests that re-implement the production code as the "expected" value, or mock out the system under test, pass green while providing no regression protection.

Check 15 — **Re-derived facts vs canonical sources**: If this PR adds logic that re-computes a fact the codebase already derives elsewhere — another fldChar begin/separate/end visible-content walk (canonical: getParagraphRuns in packages/docx-core/src/primitives/text.ts), a second footnote display-number map (buildFootnoteDisplayMap vs getFootnotes().displayNumber), or any sibling re-implementation of an existing helper instead of importing or exporting it — does the PR consume the canonical source, or explicitly justify the new derivation and add a test pinning the copies in agreement? Independent derivations of the same fact drift apart; #382's doubled footnote markers were two passes disagreeing about one fact, and the fix initially added a fifth copy of the field-state walk. References: #382, #386, #393.

Check 16 — **`.openspec` tag ↔ test-assertion drift**: If this PR adds, moves, or changes any `.openspec('[ID] …')` tag on a `test(...)`/`it(...)`, does the tagged test body actually exercise that scenario's GIVEN/WHEN/THEN — establishing the scenario's precondition and asserting its THEN observable — or is the tag attached without a covering assertion (tag-stuffing)? Judge each added/changed `[ID]` independently against the body of the test it sits on. Pay special attention to (a) tags moved onto generic clean/round-trip/property tests that never set up the scenario's specific precondition, (b) a single test absorbing several distinct scenario IDs it does not separately assert, and (c) "empirical proxy" / repo-convention justifications standing in for a real covering assertion. Deterministic coverage checks confirm the tag exists and (with the THEN-keyword guard) that a scenario token is mentioned; they cannot judge whether the assertion actually covers the scenario — this asymmetric, oversells-c

Check 17 — **Library stays general (no downstream-domain leakage)**: If this PR adds or renames public API, types, recipes, functions, or identifiers in `packages/*/src/**`, does it avoid baking in names or concepts specific to a single downstream consumer's domain — agreements especially (`signature`, `signatory`, `party`, `coverTerms`, print-name/title/date signing semantics, or other agreement field names)? safe-docx is a general OOXML library: its surface should be Word/OOXML vocabulary (table, row, cell, border, run) plus a small allowlist of genuinely LLM-driven affordances that are not in Word but exist because LLMs are the primary consumers (e.g. an outline/TOC view sized for a context window). Domain-shaped composition (a "signature block" is just a `w:tbl` built from `TableSpec`/`BorderSpec`/`RunProps`) belongs in the consumer, not here. WARN if a new symbol is named for, or shaped around, one product rather than the general OOXML/LLM surface. See CONTRIBUTING "Library scope & domain boundaries"

Aggregate and post review

CI on: pull_request

dependency-review

actionlint

gemini-manifest

readme-sync

workspace-lint

spec-coverage

site-check

tool-docs-freshness

mcpb-manifest-contract

allure-labels

allure-quality

codeql

semgrep / semgrep/scan

workspace-test (20)

workspace-test (22)

mcpb-bundle

deploy-allure

Code scanning results

Semgrep OSS

CodeQL

test(docx-core): count-guard duplicate w16cid:commentId rows in merge…

Uh oh!

Select a check to view from the sidebar