fix(merge): visibleText skips vanish runs at object interiors (#648)

[thewrz]

Why

src/merge/extract.ts's visibleText re-derives paragraph text from OOXML and had no w:vanish handling. The object-tier AST walk skips hidden runs (#641, ADR-092), so the two disagreed: an SDT-anchored cell paragraph mixing a hidden and a visible run produced "HIDDEN SECRETvisible text" from visibleText but "visible text" from the AST's objectText. A DOCX that round-trips untouched could therefore report as modified.

What

• Threaded a new objectInterior: boolean through walkBlocks → visitParagraph → visibleText, set true on descent into any OBJECT_BLOCK_TAGS (w:tbl, w:drawing, w:pict) — the same pattern already used for inTable.
• Inside an object interior, visibleText now skips any w:r for which hasRunVanish (imported from ../parser/index.js) returns true, matching objectText's existing behavior.
• The ordinary paragraph tier is untouched: a mixed hidden/visible paragraph outside an object interior still reads as fully visible (the pinned KNOWN AMBIGUITY at src/parser/docx/document.test.ts:259).
• hasRunVanish remains the single ST_OnOff-aware vanish predicate; no second copy was added under src/merge/.

Design decisions

Synthesized from both candidates plus direct reads of extract.ts, body-objects.ts, spec-diff.ts, diff.integration.test.ts, and module-boundaries.md (neither candidate had verified these against source). Key calls:

1. objectInterior rides on ParaContext rather than a new positional parameter on visibleText — visibleText's only two params are nodes/ctx, and ctx already transparently threads through every recursive visitRunNode-to-visibleText call (w:ins, w:hyperlink, w:r fallthrough) with zero extra call-site edits, which a raw third boolean parameter would require touching every one of. This departs slightly from a literal reading of "threaded through walkBlocks, then visitParagraph, then visibleText, in exactly that way" but honors its intent since the value does flow through all three in order.
2. The vanish-skip check lives inside visibleText's loop (not visitRunNode's tag dispatch), since visibleText already owns the "does this child contribute text" decision (that's exactly what its PROPERTY_TAGS skip already does), keeping visitRunNode's contract as pure compute, never skip-with-no-output.
3. hasRunVanish is reached via two new barrel re-export lines (parser/docx/index.ts, parser/index.ts) rather than a deep import — satisfies the enforceable "import only through a sibling's index.ts barrel" rule. This does cross module-boundaries.md's prose diagram line that merge knows nothing about parsing, but that line has no ESLint enforcement (confirmed by reading eslint.config.js — no import-boundary rule exists) and the issue's SETTLED section explicitly mandates this exact reuse, so it is treated as an approved narrow exception recorded in code comments and this PR body, not an ADR (banned this sprint) and not an escalation (the issue itself already made the call).
4. OrderedNode becomes a type alias of ObjectBlobNode instead of casting at the hasRunVanish call site, removing a duplicate node-shape definition rather than adding a forbidden cross-boundary assertion. This was flagged as the top spike risk; strict-tsconfig fallout across the four existing accessor functions was confirmed clean (lint + tsc --noEmit pass with zero changes needed to tagOf/childrenOf/attrStr/elementText).
5. The "prove it at that level" acceptance bullet is satisfied by extending the existing api/diff.integration.test.ts body-level object round-trip coverage — computeSpecDiff's theirs comes from extractContentControls(docxBuffer) while base/ours come from AST-derived DB paragraph snapshots, exactly where the two paths' disagreement would surface as a false "modified" diff entry today — in addition to a unit-level pin in extract.test.ts.
6. Rejected a permanent grep-based "single vanish predicate" guard as a repo-owned test file that scans source content as its oracle — brittle anti-pattern per prior-sprint feedback (a unit test must never read VCS/source state as its oracle). The grep audit instead ran once as manual evidence below, matching the acceptance criterion's literal "assert" wording as a one-time audit.

Manual audit evidence (acceptance bullet: "hasRunVanish remains the single definition")

$ grep -rn 'vanish' src/merge/
src/merge/extract.ts:81:/** A run with w:vanish explicitly val="0" (ST_OnOff OFF — visible, #648). */
src/merge/extract.ts:198-202: (doc comment referencing hasRunVanish)
src/merge/extract.ts:208:    if (ctx.objectInterior && hasRunVanish(node)) continue;
src/merge/extract.ts:426-458: (structural guard test asserting no local predicate + the hasRunVanish import)

No second vanish-detection predicate exists under src/merge/ — all references are either doc comments or calls into the single imported hasRunVanish.

---

Additional scope folded in: #652 — textBox/pict fingerprint asymmetry

The #648 work surfaced a second, unrelated correctness bug on the same file and originally reverted its own coverage for it into issue #652. Per this sprint's zero-new-issues policy the fix belongs here, so #652 is now closed by this PR.

The bug: fingerprintBlob was applied asymmetrically for textBox/pict-kind body objects, so every unmodified round-trip of a captured text box falsely reported an objectConflict. diff.ts's detectObjectConflicts fingerprints the DB-stored ObjectMeta.blob directly; per body-objects.ts's own capture convention a textBox/pict blob root is the host body w:p carrying the drawing run. But extract.ts's walkObjectBlocks fingerprinted the matched OBJECT_BLOCK_TAGS node itself — the bare w:drawing/w:pict. The sides hashed w:p(w:r(w:drawing(...))) against w:drawing(...), so fingerprintsDiverge was unconditionally true. A table's blob root is the w:tbl, which is also what walkObjectBlocks matches, so the table tier was already symmetric — exactly why every existing table test passed and this went unseen.

Design decisions (#652)

7. Option 1 over option 2 — walkObjectBlocks now tracks the nearest enclosing w:p and fingerprints it for w:drawing/w:pict matches, mirroring capture. Option 2 (changing capture to store the bare node) was rejected on two independent grounds. First, correctness: the generator emits blob[0] as a block-level body child (generator/index.ts → buildObjectBlocks), and a bare w:drawing is not a valid block-level child — it must sit inside a run inside a paragraph, so option 2 would produce invalid OOXML on re-emit. Second, ownership: it would require editing src/parser/docx/body-objects.ts, which sibling branch fix/issue-650 owns this sprint.
8. interiorUuids stay scoped to the matched drawing's own subtree, not the host paragraph's. findMatchingBlock matches on any interiorUuid overlap, so per-drawing granularity is preserved when one host paragraph carries two text boxes; widening it to the host w:p would emit duplicate-uuid blocks.
9. A drawing with no enclosing w:p degrades to the bare node rather than throwing — extract.ts never rejects a document it can still partially read.

Option 1 verified against the two paths #652 flagged as unverified

• findInteriorUuids (interior-uuid capture): holds. The fix does not touch it — interiorUuids still come from childrenOf(node, tag) on the matched drawing. The mutation run below proves the path is live: with the fix reverted the failure reports both a base and a theirs fingerprint, which detectObjectConflicts only produces for a block findMatchingBlock actually matched by interior uuid.
• Generator re-emit wiring: holds. buildObjectBlocks re-emits blob[0] (the host w:p) unchanged, and the new test round-trips through the real generateDocx endpoint. Full suite green: 3635 unit + 1813 integration, 0 failures, including every #520/#517 object and generator test.

Mutation verification (#652 regression test)

The new test is a real DB → generateDocx → extractContentControls → computeDiff integration test for a textBox-kind object, mirroring the table-kind wiring test. With the fix reverted, only its assertion fails and every table-kind test stays green:

$ npx vitest run --project integration src/api/diff.integration.test.ts   # fix REVERTED
 × an unmodified generated DOCX round-trips with ZERO objectConflicts 21ms

 FAIL  src/api/diff.integration.test.ts > body-level object round-trip —
       textBox fingerprint symmetry (#652) > an unmodified generated DOCX
       round-trips with ZERO objectConflicts
 AssertionError: expected [ { ...(4) } ] to deeply equal []
       + "hash": "da8378f8231962f38adce2d3991016429a52bf630132e76f68fba5e3ba6cb2cd",
       +   "kind": "textBox",          <- base: hashed the host w:p
       + "hash": "e5745b969217bbbfa17715655d7690b60963e1c350e9f728a1eae69217620b95",
       +   "kind": "textBox",          <- theirs: hashed the bare w:drawing

 Test Files  1 failed (1)
      Tests  1 failed | 10 passed (11)

$ npx vitest run --project integration src/api/diff.integration.test.ts   # fix RESTORED
 Test Files  1 passed (1)
      Tests  11 passed (11)

The gate is non-vacuous by construction: it asserts the whole diff is empty, not objectConflicts alone, so a future regression that stopped findInteriorUuids from seeing the interior anchor would surface as a deleted/modified entry rather than silently passing. Teardown is id-scoped to the single spec row the block created.

Testing

• Unit tests pass (pnpm test — 255 files, 3635 tests, all green)
• Integration tests pass (pnpm test:integration — 158 files, 1813 tests, 0 failures)
• Manual verification: object-interior mixed hidden/visible paragraph extracts only visible text; ordinary paragraph KNOWN AMBIGUITY unchanged; w:vanish w:val="0" run inside an object interior survives; single-predicate audit above
• CI green

🤖 Co-authored by Claude Sonnet 5 (#648) and Claude Opus 5 (#652 + review). Closes #648. Closes #652.

Summary by CodeRabbit

• Bug Fixes

  • Fixed document merging for hidden and visible text runs inside tables and text boxes.
  • Prevented false object conflicts when text boxes are round-tripped.
  • Correctly preserves explicitly visible text and standard paragraph behavior.

• Tests

  • Added regression and integration coverage for hidden runs, text boxes, object round-tripping, and visibility handling.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c5f78af-3f61-49ee-a9cb-9711cfeb4382

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Changes

Object merge extraction

Layer / File(s)	Summary
Object-interior visibility traversal src/parser/docx/index.ts, src/parser/index.ts, src/merge/extract.ts, src/merge/extract.test.ts	Exports the shared hasRunVanish predicate and skips enabled vanished runs only inside tables, DrawingML text boxes, and VML text boxes. Ordinary paragraph behavior and w:vanish w:val="0" visibility remain covered.
Host paragraph fingerprinting src/merge/extract.ts, src/api/diff.integration.test.ts	Fingerprints drawing and pict objects from their host paragraphs and adds table and textBox round-trip diff coverage.

Possibly related issues

• wrzonance/SpecR#650 — Extends the same shared hasRunVanish behavior for style-resolved vanish state.
• wrzonance/SpecR#300 — Covers merge round-trip behavior for tables and text boxes.

Possibly related PRs

• wrzonance/SpecR#647: Introduced shared hidden-run handling that this change applies to merge object extraction.
• wrzonance/SpecR#523: Added related object structural-conflict extraction.
• wrzonance/SpecR#636: Modified related DOCX text-box visibility and object extraction behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the object-interior vanish handling fix, which is a primary change in the pull request.
Linked Issues check	✅ Passed	The changes satisfy #648 and #652, including object-only vanish handling, shared predicate reuse, host-paragraph fingerprinting, regression coverage, and reported validation.
Out of Scope Changes check	✅ Passed	The implementation, exports, tests, and structural checks directly support the linked issue objectives without unrelated code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

_{Comment @coderabbitai help to get the list of available commands.}

[thewrz]

This was written agentically; verify its assertions and edit accordingly:

Adversarial review — Codex gpt-5.6-sol (xhigh)

Ran once as the final draft-phase gate, against origin/main. 1 finding (0 P1, 1 P2), 1 handled.

[P2] "Keep the parser barrel out of merge extraction" — src/merge/extract.ts:15 — DECLINED (documented)

When a merge-only worker or test imports extractContentControls, this import evaluates all of parser/index.ts, which statically loads the heavyweight PDF/OCR stack (pdfjs-dist, unpdf, tesseract.js) even though extraction only needs one OOXML predicate.

The transitive load is real — I reproduced it. parser/pdf/index.ts does statically import ./extract.js (pdfjs-dist, unpdf) and ./ocr.js (tesseract.js). Measured A/B on a cold module graph:

module	cold import	RSS
merge/extract.ts @ origin/main	262 ms	43.5 MB
merge/extract.ts @ this PR	632 ms	332.4 MB

But the stated harm does not exist in this codebase. There is no merge-only worker, script, or CLI — grep over scripts/ and tools/ finds nothing importing merge/. And every production consumer already loads the parser barrel in the same file:

• src/api/diff.ts:4 imports assertDocxSafe from ../parser/index.js, one line above its ../merge/index.js import on line 5.
• src/mcp/handlers.ts:21 does the same, two lines above computeSpecDiff on line 23.

The remaining three consumers (api/merge.ts, mcp/merge-handlers.ts, mcp/resources.ts) are mounted in that same process. Net production cold-start and memory delta: zero. The only module graph that actually grows is merge/extract.test.ts — 31 tests, 1.35 s.

Both proposed alternatives are strictly worse:

1. Relocate hasRunVanish to a lib/AST leaf (Codex's suggestion) requires editing src/parser/docx/body-objects.ts. A concurrent branch (fix/issue-650) is extending that exact function with an optional character-style-vanish parameter; moving it now would collide with that work and risks splitting the single predicate — which is precisely the capture/rewrite drift ADR-092 closed. hasRunVanish must remain the one ST_OnOff-aware vanish predicate.
2. Deep-import ../parser/docx/body-objects.js would violate the same module-boundaries.md sibling-barrel-only rule more deeply than the barrel import Codex objects to, trading a measured-zero cost for a worse architectural violation.

The import therefore stands. The finding, the numbers, and this reasoning are now recorded at the import site in fbb3ea3 so the tradeoff is not re-litigated from scratch.

Also verified (not flagged, checked independently)

• Vanish-skip parity with the object tier. visibleText re-enters itself for w:r/w:hyperlink/w:smartTag/w:ins via visitRunNode's fallthrough, so hasRunVanish is applied at every nesting depth — matching body-objects.ts's collectText. A hidden run wrapped in a w:hyperlink is skipped by both.
• The attrStr null-check removal is sound. ObjectBlobNode's ':@' is declared readonly ':@'?: Readonly<Record<string, string | number>> — optional, never nullable — and isObjectBlobNode explicitly rejects null. typeof attrs !== 'object' alone is a complete guard.
• The ordinary-paragraph KNOWN AMBIGUITY is untouched. The vanish skip is gated on ctx.objectInterior; a mixed hidden/visible paragraph outside an object interior still reads as visible, as pinned in document.test.ts.

CI green on fbb3ea3: Build, Lint, Verify harness, Test (unit + integration), LOC delta.

🤖 Co-authored by Claude Opus 5 (1M context).

[thewrz]

This was written agentically; verify its assertions and edit accordingly:

Adversarial review pass 2 — Codex gpt-5.6-sol (xhigh) — the #652 fix

Disclosure: the draft phase normally spends exactly one adversarial pass, and it had already run (see the comment above) when the #652 scope was folded into this branch. Rather than ship a new correctness fix with no adversarial coverage, I ran a second pass scoped strictly to commit 298e338 — the walkObjectBlocks host-paragraph change and its regression test — not a re-review of the #648 work.

Verdict: no [P1] or [P2] findings. Confirmed independently:

• hostParagraph is lexically scoped per recursive call — it cannot leak across siblings or escape a subtree.
• Multiple drawings in one host paragraph keep per-drawing interiorUuids while sharing the host fingerprint; findMatchingBlock still matches correctly.
• Drawings inside tables and nested text boxes stay folded into the outer block via inBlock — they cannot select an interior paragraph as a new top-level host.
• Table tier byte-identical: fingerprintRoot returns the original w:tbl, so the same node and uuid subtree are fingerprinted as before.
• isWholeObjectDeletion unaffected.
• The integration test is non-vacuous for this regression.

One caveat accepted and fixed — d605ce6

Codex flagged (as a non-finding) that my test comment overstated its own guarantee: it claimed asserting the whole diff protects against a future findInteriorUuids regression. It does not — theirsControlled is built by walkBlocks independently, so interior text would still round-trip cleanly if interior-uuid collection regressed. Correct as a matter of fact, and a misleading comment is exactly the kind of debt that outlives the PR, so I rewrote it in d605ce6 to state what actually establishes non-vacuity: the mutation run fails carrying both a base and a theirs fingerprint, a pairing detectObjectConflicts only emits for a block findMatchingBlock matched by interior uuid. extract.test.ts is named as the real pin on that path.

Codex could not run the integration suite (no Postgres on its default port). I ran it here: 158 files / 1813 tests / 0 failures, plus 255 files / 3635 unit tests.

🤖 Co-authored by Claude Opus 5 (1M context).

[thewrz]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

🧹 Nitpick comments (1)

src/merge/extract.test.ts (1)

8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import SpecTree through the AST barrel.

Use ../ast/index.js after confirming that it re-exports SpecTree. This test currently depends on an AST internal module.

As per coding guidelines, tests must use module API boundaries. Based on learnings, public AST exports outside src/ast must use src/ast/index.ts.

Proposed change

-import type { SpecTree } from '../ast/types.js';
+import type { SpecTree } from '../ast/index.js';

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/merge/extract.test.ts` at line 8, Update the SpecTree import in the
extract test to use the public AST barrel at ../ast/index.js, confirming that
this barrel re-exports SpecTree; do not import the AST internal types module
directly.

Sources: Coding guidelines, Learnings

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/merge/extract.test.ts`:
- Line 8: Update the SpecTree import in the extract test to use the public AST
barrel at ../ast/index.js, confirming that this barrel re-exports SpecTree; do
not import the AST internal types module directly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d56552d-6207-4a09-914d-39aae622fdae

📥 Commits

Reviewing files that changed from the base of the PR and between 14f06e3 and d605ce6.

📒 Files selected for processing (5)

• src/api/diff.integration.test.ts
• src/merge/extract.test.ts
• src/merge/extract.ts
• src/parser/docx/index.ts
• src/parser/index.ts

[thewrz]

Nitpick handled in 96ae5846 (body-level nitpick, no thread to resolve).

src/merge/extract.test.ts:8 — valid, fixed. docs/architecture/module-boundaries.md line 7 states it without qualification: "Modules import only from a sibling's index.ts barrel, never from its internal files." I checked specifically for a type-only carve-out and there is none — lib/ is the sole documented exception, and ast/ is not lib/. ast/index.ts already re-exports SpecTree from its export type { ... } block, so this was a drop-in.

Scope, stated explicitly rather than left silent. The cited import is pre-existing on main — this branch never touched that line. Sweeping the repo for the same class found 25 files outside src/ast reaching ../ast/types.js directly, including src/parser/index.ts (a barrel itself) and src/merge/types.ts. There is no no-restricted-imports rule in eslint.config.js, which is why the convention drifted this far without anything going red.

I fixed only the file this PR already modifies. The remaining 24 are deliberately untouched: a repo-wide import rewrite would collide with six concurrent branches and has nothing to do with #648's deliverable. That call — and whether the rule should be made executable as a lint rule instead of prose — is the owner's, and has been surfaced to them directly rather than filed as an issue.

Verified: prettier, eslint, tsc --noEmit, and 31/31 extract.test.ts all clean.