feat(docx-compare): fail closed when a comparison input already contains tracked changes

[stevenobiajulu]

Fails closed when a comparison input already contains tracked changes, instead of silently emitting a Word-unreadable file.

Refs #742 (deliberately not "Fixes": the issue also sketches an --allow-tracked-input accept-on-ingest follow-up, which is out of scope here — see the OpenSpec proposal's Out-of-scope section. Maintainer may prefer to close it with this PR; wording left conservative on purpose.)

DRAFT / do not merge: PR #841 rewrites packages/docx-compare/src/index.ts and cli/compare-two.ts, the surfaces this guard touches. This PR stays draft until #841 lands, then rebases. Auto-merge is intentionally NOT armed.

Premise gate (reproduced at HEAD before any edit)

Clean A; B = A edited with the edit kept as tracked changes (w:ins/w:del, author "Original Author"); then compareDocuments(A, B):

• Returned normally, exit 0, normal stats: {"insertions":2,"deletions":2,"modifications":2,...} — nothing indicates the input was unsuitable.
• Output word/document.xml revision authors: ["Comparison","Original Author"] — the issue's smoking gun that pre-existing revisions were passed through rather than accepted before diffing.
• Directly nested w:ins-inside-w:ins present.
• The transitional-schema gate (scripts/check_emitted_document_schema.mjs) passes the corrupt file — the corruption is behavioral, not schema-visible, which is why no existing gate caught it.
• Independent Codex premise check (read-only sandbox): CONFIRMED — traced the call path (no refusal anywhere) and reproduced the two-author, nested-ins output independently.
• Independent corpus confirmation (520-document SHA-pinned differential, run separately): in rebuild mode the comparison unwraps pre-existing tracked changes into bare w:delText outside any w:del wrapper — exactly the Word-unreadable shape compare_documents silently produces a Word-unreadable file when an input already contains tracked changes #742 reports — while inplace passes the two-author markup through. The guard therefore refuses in both modes.
• Live Word-for-Mac probe of the reproduction output: INDETERMINATE both attempts — the probe correctly refuses to act while the user's own Word session holds a modal dialog, and interfering was out of bounds. The issue itself documents the repair prompt for this exact class, and the two checked-in files it names (atomizer_redline.docx, typescript_redline.docx) are already memorialized as raising "Word found unreadable content".

Design: one guard, one boundary

Every public path funnels through compareDocumentsAtomizer — compareDocuments, both CLIs (which take compare as an injectable dependency), the MCP tool, and the benchmark runner — and compareDocumentsAtomizer is itself publicly exported (a genuine bypass if guarded higher). So the scan runs once, there:

• New module packages/docx-compare/src/baselines/atomizer/trackedInputRevisionSafety.ts: assertComparisonInputsUntracked + typed recoverable TrackedInputRevisionError (operand, partPath, markers), following the UnsupportedTextBoxRevisionError precedent; exported from the package root.
• Scan scope: word/document.xml + enumerateRevisionStoryPartPaths (footnotes, endnotes, comments, glossary, every numbered header/footer). Detects w:ins/w:del/w:moveFrom/w:moveTo, the six *PrChange records, the cell-topology records w:cellIns/w:cellDel/w:cellMerge, and w:numberingChange — the last four added after Codex's review probe proved they passed the original ten-name scan and survived comparison with their prior author. Row-level w:trPr > w:ins|w:del markers trip it via the same local names. Range-boundary markers (w:*RangeStart/End, w:customXml*Range*) are documented non-triggers (the probe showed an isolated pair is dropped, not passed through).
• Surfaces map, they don't re-scan: MCP compare_documents returns distinct INPUT_HAS_TRACKED_CHANGES (never the catch-all COMPARE_ERROR) with an accept_changes recovery hint. Both CLI entry handlers already print error.message and exit 1, and the message names the offending operand — zero CLI code changes, which also keeps the diff rebase-friendly vs feat(docx-comparison): default to tagged-tree comparison #841.
• Malformed parts defer: parts the scan cannot parse are skipped by this guard so the ancillary safety boundary keeps its precise diagnostics (AncillaryStorySafetyError / NOTE_PART_XML_INVALID) — the same rule textBoxRevisionSafety documents for its own preparatory scan. Missing parts are skipped.

The one judgment call reviewers should look at

Twenty-one existing tests across eleven files (six in docx-compare, five in docx-core's integration suite) deliberately drive pre-tracked fixtures through the public entry to pin engine internals (original-insertion provenance restoration, insertion-collision promotion, preserved-move identity seeding, pre-existing-wrapper bookmark splitting, canonical-emission round-trips, the ADV mode-preservation characterization) — the "preserve pre-existing tracked changes" behaviors. Deleting that coverage would also trip the coverage ratchet, and the code remains live for the future accept-on-ingest path. Instead, the orchestration below the guard is exposed as compareDocumentsAtomizerUnguarded and those tests use it with a per-site comment citing #742; assertions unchanged.

Containment (revised after Codex review): the seam is NOT exported from the package root — Codex demonstrated a root export was a live public bypass, so it was removed and [SDX-TRKIN-06] now pins its absence. docx-compare tests import it from the pipeline module; docx-core integration tests import it via the package's dist subpath, which docx-core's vitest config aliases back to the same source module graph its root alias uses (TypeScript resolves the subpath against the built .d.ts, exactly like the root import already does; a relative source import violates docx-core's tsc rootDir).

OpenSpec

add-tracked-input-comparison-guard — validated --strict. ADDED requirements on docx-comparison (8 scenarios, SDX-TRKIN-01..08) and mcp-server (3 scenarios, SDX-TRKIN-MCP-01..03). ADDED rather than MODIFIED: no deployed requirement in either capability promises anything about comparison-input validation, so there was nothing to modify without inventing prior text for the archiver to overwrite.

Tests

• trackedInputRevisionSafety.test.ts (docx-compare): all ten revision kinds + row-level marker, both operands, both reconstruction modes, all six story flavors, missing parts skipped, malformed part defers to AncillaryStorySafetyError, clean-pair control, both public entry points, and the real runCompareCli (no injected fake — an injected fake would bypass the guard and prove nothing) refusing with no output written.
• add_tracked_input_comparison_guard.test.ts (docx-mcp): INPUT_HAS_TRACKED_CHANGES mapping + no file written, real runCompareCommand rejection naming the operand, clean control.
• Red→green: with the guard call and MCP mapping neutralized, 6/8 and 2/3 of the new tests fail respectively (the passing ones are the deliberate clean/malformed controls); restored, everything is green.

Codex peer review (dynamic, execution-backed)

Verdict on the initial commit: REQUEST-CHANGES, three findings, all confirmed by live probes and all addressed in the follow-up commit (adjudication detailed in a PR comment):

1. Detector gaps — w:cellIns/w:cellDel/w:cellMerge/w:numberingChange execution-proven to pass through with their prior author → added as triggers, with per-kind public-boundary fixtures.
2. Package-root bypass — the root export of the unguarded seam was demonstrated as a working bypass → removed; [SDX-TRKIN-06] pins its absence; docx-core tests reach the seam via a vitest-aliased dist subpath instead.
3. Hint accuracy — accept_changes cannot clean headers/footers, so the recovery hint is now part-aware ([SDX-TRKIN-MCP-04]); the recommended session-mode refusal test was added as [SDX-TRKIN-MCP-05].

Gates

Full pre-submit sequence run on exit codes (build, lint:workspaces, test:run, check:spec-coverage, check:conformance-citations, check:conformance-doc), plus openspec validate --strict, check:tool-docs after regenerating the tool reference, and git diff --check. Rebased onto origin/main at 26c6640 (#869).

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
site	Ready	Preview	Aug 16, 2026 4:55am

[codecov]

Codecov Report

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

Files with missing lines	Patch %	Lines
packages/docx-mcp/src/tools/compare_documents.ts	75.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[stevenobiajulu]

Codex peer review — verdict and adjudication

A dynamic Codex review (workspace-write sandbox, full execution trace) was run on the initial commit 8b1bfb8. Prompt and raw output are preserved under the repo-local .peer-review/ (gitignored). Codex independently re-ran both new suites, all eleven re-pointed engine suites, the builds, openspec validate --strict, spec coverage, conformance citations, tool-docs drift, and wrote its own live bypass/marker probes.

Verdict: REQUEST-CHANGES — three findings. Adjudication:

1. Detector gaps — ACCEPTED (blocking, execution-proven)

Codex probed the public compareDocuments with fixtures for revision records outside the ten-name list and showed w:cellIns, w:cellDel, w:cellMerge, and w:numberingChange pass the guard and survive in the output with their prior author intact — a direct violation of the advertised fail-closed contract, independent of whether each record alone makes Word refuse the file.

Fix (second commit): all four are now detection triggers, each with a public-boundary fixture in [SDX-TRKIN-04]. The range-boundary family (w:moveFromRangeStart/End, w:moveToRangeStart/End, w:customXml*Range*) is explicitly classified as non-triggers in the detector JSDoc and the spec delta: Codex's own probe showed an isolated range pair is dropped by the comparison rather than passed through, they carry no author-bearing content, and content-bearing moves are caught via their w:moveFrom/w:moveTo wrappers.

2. Package-root bypass — ACCEPTED (blocking, execution-proven)

Codex imported compareDocumentsAtomizerUnguarded from the package root and produced a two-author output, disproving the "loudly-named export is enough" containment (and catching a stale JSDoc that still claimed the seam was not root-exported).

Fix: the root export is removed and [SDX-TRKIN-06] now asserts the package root does not expose it. Of Codex's three suggested remedies, the "genuinely test-only entrypoint" was implemented in the least invasive form available: docx-core integration tests import the pipeline module through the package's dist subpath (@usejunior/docx-compare/dist/baselines/atomizer/pipeline.js), which docx-core's vitest config aliases back to the same source module graph its root alias already uses. TypeScript resolves the subpath against the built .d.ts exactly as the root import already does, so no new build-order constraint is introduced, and the runtime alias prevents a second dist module instance. Relocating the five cross-package characterization suites into docx-compare (Codex's first preference) was rejected for this PR as a large, rebase-hostile move — they lean on docx-core-internal builders — and is a reasonable follow-up refactor.

3. Recovery hint wrong for header/footer detections — ACCEPTED

Codex checked the deployed mcp-server spec: accept_changes covers the body and revisionable side stories (footnotes, endnotes, comments, glossary) but not headers or footers, so the hint would have looped for exactly the ancillary cases this guard newly covers.

Fix: the hint is part-aware — header/footer detections state that accept_changes does not cover them and direct the caller to produce a fully accepted/rejected copy. Pinned by [SDX-TRKIN-MCP-04]. Codex's coverage recommendation (session-mode refusal) was also adopted as [SDX-TRKIN-MCP-05]: a session opened on a document with pre-existing tracked changes is refused in session-mode compare.

Points reviewed and retained as-is

• Comments-part scanning stays broad (Codex: "consistent with the design brief and single clean-input contract; I would retain it").
• OpenSpec ADDED (not MODIFIED) usage — Codex verified no deployed requirement defines comparison-input validation.

After the fixes: full pre-submit gate re-run green on exit codes (build, lint:workspaces, test:run, check:spec-coverage, check:conformance-citations, check:conformance-doc), plus openspec validate add-tracked-input-comparison-guard --strict and git diff --check.