Repository navigation
proposal: cross-boundary text operations - #2
Merged
Merged
Conversation
Add OpenSpec proposal addressing issue #1 - the library cannot find or edit text that spans tracked change boundaries. Proposes a three-phase solution: 1. Virtual Text Map (read-only flattened text view) 2. Cross-Boundary Search 3. Boundary-Aware Replace with explicit failure on edge cases
Initialize OpenSpec and create change proposal addressing issue #1: the library cannot find or edit text that spans tracked change boundaries. Proposal includes: - Virtual Text Map for flattened text view - Cross-boundary search capability - Boundary-aware text replacement - Explicit RevisionBoundaryError for edge cases Replaces the informal proposal with proper OpenSpec structure.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
Ok, I went trough your suggestions and ran into AI, see below: This proposal is a massive leap forward for the library. It successfully moves the mental model from "editing XML nodes" to "editing a document," which is how users actually think. However, it stops short of being a fully-featured editor for complex documents, opting for safety over power. What Was Accomplished Safety First: The smartest thing this proposal does is introduce the RevisionBoundaryError. Previously, the library might have corrupted a file by blindly editing across boundaries. Now, it explicitly stops you. It turns a "silent data corruption bug" into a "known limitation," which is excellent engineering. Visibility: The get_visible_text() API finally gives users a way to see what the document actually looks like to a human reader, which is essential for analysis workflows. What Was Not Accomplished (The Limitations) No "Merge" Logic: Because it blocks mixed-state editing, the library cannot be used to "clean up" documents. You can't programmatically accept a sentence that is half-original/half-inserted and rewrite it. How to Fix "Mixed State" Editing To fix this, you need to stop thinking of "Replace" as one action and start thinking of it as Atomic Decomposition. You must break the user's request into smaller, valid XML operations. The Scenario: User wants to delete: "Aim: To" The Algorithm: Segment 1: Pure text ("Aim: ") Execute Action 2 (The Hard Part): For Segment 2 ("To"), you cannot wrap it in <w:del> because it is already inside <w:ins>. You effectively need to "undo" this specific part of the insertion. Step A: Split the Insertion. The library must slice the <w:ins> element into two parts: the part we want to remove ("To") and the part we want to keep (" examine"). Why this works: You never created invalid XML (like <w:del><w:ins>...</w:ins></w:del>). You respected the rules of each specific node type by breaking the big problem into small, solvable atomic `problems.``` |
Incorporate feedback from @gabrielpacosta: instead of raising RevisionBoundaryError when edits span tracked changes, decompose the operation into per-segment atomic actions that respect each node's revision context. Key changes: - Replace Decision 2 (reject) with atomic decomposition algorithm - Add Phase 5 tasks for mixed-state editing implementation - Add spec scenarios for insertion splitting and cross-boundary replace - Remove RevisionBoundaryError (no longer needed)
Each phase now starts by writing failing tests before implementation, following TDD methodology.
Owner
Author
|
@gabrielpacosta thanks! I gave another pass to the spec/plan. I believe it covers your suggestions now, and it is ready for implementation. Let me know otherwise. |
- Document run property (w:rPr) preservation requirement when splitting runs across boundaries - Require round-trip validation in tests to catch malformed XML - Add corresponding test tasks (5.5, 5.6)
Add support for finding and replacing text that spans multiple XML elements and revision boundaries (<w:ins>/<w:del>) in OOXML documents. Core changes: - TextPosition, TextMap, TextMapMatch dataclasses for virtual text mapping - build_text_map() flattens paragraph XML into searchable text - get_visible_text() and find_text() public APIs on Document - Cross-boundary replace/delete via _replace_across_nodes/_delete_across_nodes - Atomic decomposition for mixed-state editing (text spanning revision boundaries): regular text wrapped in <w:del>, insertion text split/removed - replace_text() and suggest_deletion() fall back to cross-boundary search when single-element search fails 46 new tests (294 total), all passing. Closes #1
Previously, insert_after/before placed the <w:ins> after/before the entire <w:r> run containing the anchor text. When the anchor was part of a longer text run, the insertion appeared at the wrong position (e.g., end of sentence instead of after the matched word). Now _insert_text splits the run into [before][anchor][ins][after], placing the insertion inline at the correct position.
_insert_text now falls back to _find_across_boundaries when the anchor text spans element boundaries, matching the pattern used by replace_text and suggest_deletion. Also adds 32 tests covering gaps: _classify_segments, mixed-state deletion, _remove_from_insertion all 4 branches, XML special characters, occurrence parameter, and cross-boundary anchor insertion.
pablospe
force-pushed
the
feedback-issue-1
branch
from
January 29, 2026 13:30
6a9ab6e to
c88507b
Compare
- Group positions by w:t node instead of run in _build_cross_boundary_parts - Handle multi-node segments in _remove_from_insertion and _delete_regular_segment - Deduplicate _delete_mixed_state by delegating to _delete_regular_segment - Deduplicate _find_across_boundaries by delegating to find_in_text_map - Resolve open questions in design.md - Add tests for multi-w:t scenarios (verified failing on old code)
Owner
Author
|
@gabrielpacosta can you give a try to these changes? Hopefully they solve your problems. |
📝 WalkthroughWalkthroughAdd per-paragraph visible-text mapping and cross-boundary editing: new TextMap/TextPosition/TextMapMatch types and builders, Document.get_visible_text/find_text APIs, extensive cross-boundary replace/delete/insert logic in RevisionManager, package-level exports, many tests, and OpenSpec documentation and governance. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Doc as Document
participant RM as RevisionManager
participant TM as TextMap
participant XML as XML/DOM
Client->>Doc: find_text(search, occurrence)
Doc->>RM: request_search(search, occurrence)
RM->>TM: build_text_map(paragraph)
TM-->>RM: TextMapMatch(start,end,is_inside_ins,spans_boundary)
alt spans_boundary
RM->>RM: classify_segments & plan atomic ops
RM->>XML: split/run-wrap/delete/insert across w:t, w:ins, w:del (preserve rPr)
else within single context
RM->>XML: split/wrap/replace nodes
end
XML-->>RM: updated DOM
RM-->>Doc: match/result or edit outcome
Doc-->>Client: return match/result or commit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 329-334: The current branching in _replace_across_nodes (and the
analogous delete path using _delete_same_context) only checks
match.spans_boundary and will treat matches fully inside an insertion as
same-context, which can produce nested <w:ins>/<w:del> and invalid XML; update
the condition so that any match where match.is_inside_ins is True is routed to
_replace_mixed_state (and likewise route matches with is_inside_ins to the
mixed-state delete handler instead of _delete_same_context), i.e., change the
branch from solely checking spans_boundary to check (spans_boundary or
is_inside_ins) and call _replace_mixed_state/_delete_mixed_state accordingly.
- Around line 335-380: The loop in _replace_same_context removes a run's parent
for each part, which can attempt to remove the same parent multiple times when a
run contains multiple <w:t> nodes; change the removal logic to deduplicate
parents before removing: collect parent nodes from parts (e.g., iterate parts
and add run.parentNode to a set/list using object identity), then iterate that
unique collection once and call parent.removeChild(run) (or remove the
appropriate child run node) for each unique parent; keep the existing
insert_before/new_xml logic (editor.insert_before and nodes handling) unchanged
and only replace the per-part removal loop with a deduplicated removal pass.
- Around line 732-756: In _insert_near_match, inserting using last_run (and
similarly first_run) can place a new <w:ins> inside an existing <w:ins>; walk up
the ancestor chain from last_run (and first_run) to find the nearest ancestor
element with tagName "w:ins" and use that ancestor as the insertion reference
(i.e., pass the ancestor node to editor.insert_after/insert_before) so the new
insertion lands outside existing <w:ins>; ensure you still handle the case where
no ancestor <w:ins> exists by falling back to the original run node and preserve
the existing return behavior around node.getAttribute("w:id").
- Around line 465-483: When removing a matched single-node insertion, avoid
deleting the entire <w:ins> if it contains other <w:t> nodes: instead of
unconditionally calling ins_elem.parentNode.removeChild(ins_elem) when the
matched node becomes empty, first inspect the number of wt nodes inside that
ins_elem (use self._get_wt_nodes_in_ancestor(ins_elem) or a similar helper); if
that count == 1 then remove the ins_elem, otherwise remove only the matched
run/node (e.g. remove first_node via its parentNode) while preserving the rest
of the <w:ins>; keep the existing parentNode existence checks when removing.
- Around line 524-577: The current _delete_regular_segment groups positions by
node id which breaks when multiple w:t nodes share the same run; instead group
the incoming positions by their run (use the run returned by
self._get_run_info(node) or run identity) so you perform a single replacement
per run: aggregate first/last offsets across all positions belonging to the same
run, build a combined before/matched/after sequence spanning the run's text
(respecting intermediate full-node matches), call self.editor.insert_before once
for that run, and only then remove the original run; keep existing logic to
extract rPr_xml, compute _escape_xml on fragments, and set first_del_id from the
first inserted w:del node.
In
`@openspec/changes/archive/2026-01-29-add-cross-boundary-text-operations/design.md`:
- Around line 17-28: Replace the bold labels currently used as headings in the
"Goals / Non-Goals" section with proper Markdown heading syntax to satisfy
markdownlint MD036; for example, change the bolded "**Goals:**" and
"**Non-Goals:**" markers to heading lines like "## Goals" and "## Non-Goals" (or
an appropriate level consistent with the rest of the document) so the section
headings are semantic and lint-clean.
In `@openspec/specs/text-operations/spec.md`:
- Around line 3-5: Replace the placeholder under the "## Purpose" heading
(currently "TBD - created by archiving change
add-cross-boundary-text-operations. Update Purpose after archive.") with a
concise final Purpose statement that summarizes this spec’s intent, scope, and
intended audience; update the text to a single clear paragraph (1-2 sentences)
that references the cross-boundary text operations feature and its goal (e.g.,
why it exists and what it enables) so the spec no longer contains placeholders.
- Around line 97-102: Add a trailing newline at the end of the file so the file
ends with a single newline character; locate the paragraph containing the
snippet "<w:ins>To examine whether</w:ins>" in spec.md and ensure the file
terminates with a newline character (EOF newline) to satisfy the
end-of-file-fixer.
In `@tests/test_coverage_gaps.py`:
- Around line 382-391: The test test_find_text_second_occurrence is flaky
because comparing match0.start or match0.text can be equal for occurrences in
different paragraphs; update the assertion to verify the underlying
container/node differs instead: after obtaining match0 and match1 from
doc.find_text (and keeping the existing skip logic), assert that the resolved
node/element reference (e.g., match0.node, match0.paragraph, or
match0.container) is not equal to match1.node (or the appropriate node property
provided by the Match object) so the test reliably detects distinct occurrences
across paragraphs; keep the Document.open/doc.close usage and only change the
final assertion to compare node identity rather than start/text.
In `@tests/test_multi_wt.py`:
- Around line 18-25: Change single-quoted string literals to double quotes to
satisfy Ruff Q000: replace NS =
'xmlns:w="http://schemas.openxmlformats.org/wordprocessingml/2006/main"' with NS
= "xmlns:w=\"http://schemas.openxmlformats.org/wordprocessingml/2006/main\"" and
change any other single-quoted string usages in this file (e.g., full =
f'<w:body {NS}>{xml}</w:body>' -> use double quotes for that literal) including
the occurrences around lines 87-105; you can also run `ruff --fix` / `ruff
format` to apply the formatting consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 393-409: The insertion currently always uses ref_node (computed
from first_run or w:ins) and calls editor.insert_before, which places the
replacement before any preserved prefix when a match begins mid-run; modify the
logic in the block around _get_run_info, _find_ancestor and editor.insert_before
so you detect a mid-run start (use match.positions[0] and first_run and its
offset), split the first run/node at the match start (or split out the preserved
prefix and wrap it in the existing deletion/ins wrapper), and then insert the
ins_xml after the preserved prefix (or after the deletion wrapper) instead of
before ref_node; ensure you update references used by insert_before/insert_after
so the visible text order remains correct.
- Around line 543-570: The loop that replaces matched text currently removes the
entire run node (run.parentNode.removeChild(run)) after inserting new_xml which
will delete unrelated <w:t> text and can cause double-removal when runs contain
multiple <w:t> elements; update the logic in the groups-processing block (around
self._get_run_info, self.editor.insert_before and run.parentNode.removeChild) to
first normalize or split runs so each run contains a single <w:t> (or
alternatively reconstruct a run preserving any non-matched <w:t> children), then
only remove or replace the specific <w:t> elements you modified and guard the
removal with a DOM-existence check to avoid removing the same run twice. Ensure
insert_before still targets the correct node and that any preserved <w:t>
children are reattached to maintain unrelated text.
🧹 Nitpick comments (7)
CLAUDE.md (1)
43-454: Fix markdownlint issues (headings, code fence languages, table spacing).Static analysis flags MD036/MD040/MD060 in this file. Suggest converting emphasized lines to headings, adding languages to fenced code blocks, and adjusting table pipes.
🛠️ Suggested edits
-**Workflow** +#### Workflow @@ -```bash +```bash @@ -``` +``` @@ -```markdown +```markdown @@ -``` +``` @@ -```bash +```bash @@ -``` +``` @@ -| Task | Tool | Why | -|------|------|-----| -| Find files by pattern | Glob | Fast pattern matching | -| Search code content | Grep | Optimized regex search | -| Read specific files | Read | Direct file access | -| Explore unknown scope | Task | Multi-step investigation | +| Task | Tool | Why | +| --- | --- | --- | +| Find files by pattern | Glob | Fast pattern matching | +| Search code content | Grep | Optimized regex search | +| Read specific files | Read | Direct file access | +| Explore unknown scope | Task | Multi-step investigation |openspec/project.md (1)
29-38: Add language identifier to fenced code block.The code block showing the directory structure should specify a language (e.g.,
textorplaintext) for consistency and to satisfy markdown linting.📝 Suggested fix
-``` +```text docx_editor/ ├── document.py # Public API facade (Document class)docx_editor/document.py (1)
120-126: Add return type annotation.The
find_textmethod is missing a return type annotation. Based on the delegation to_find_across_boundaries, it should returnTextMapMatch | None.📝 Suggested fix
- def find_text(self, text: str, occurrence: int = 0): + def find_text(self, text: str, occurrence: int = 0) -> "TextMapMatch | None": """Find text in the document, including across element boundaries. Returns match info or None if not found. """ self._ensure_open() return self._revision_manager._find_across_boundaries(text, occurrence)Note: You may need to add
TextMapMatchto the imports or use a forward reference string annotation.openspec/specs/text-operations/spec.md (1)
3-5: Update the Purpose section.The Purpose section still contains placeholder text from the archive process. Consider updating it with a clear description of what the text-operations feature provides, such as: "Enable cross-boundary text search and editing operations in Word documents with tracked changes."
tests/test_coverage_gaps.py (1)
95-183: Consider moving repeated imports to the top of the file.The
MagicMockandRevisionManagerimports are repeated in each test method withinTestClassifySegments. Consider moving these to the top of the file with the other imports for cleaner code.♻️ Suggested refactor
import defusedxml.minidom import pytest +from unittest.mock import MagicMock from docx_editor import Document from docx_editor.xml_editor import ( TextMapMatch, TextPosition, build_text_map, ) +from docx_editor.track_changes import RevisionManagerThen remove the local imports from each test method.
openspec/changes/archive/2026-01-29-add-cross-boundary-text-operations/design.md (1)
57-58: Minor: Add blank line before table and normalize table spacing.Static analysis flagged missing blank line before the table (MD058) and inconsistent table column spacing (MD060). This is a minor formatting issue that doesn't affect readability.
📝 Suggested fix
**Alternatives considered:** + | Option | Pros | Cons | -|--------|------|------| -| A. Reject with error | Explicit, safe | Blocks valid use cases | -| B. Implicit accept | "Just works" | Destroys revision history | -| C. Atomic decomposition (chosen) | Handles mixed state, no invalid XML | More complex implementation | +| ------ | ---- | ---- | +| A. Reject with error | Explicit, safe | Blocks valid use cases | +| B. Implicit accept | "Just works" | Destroys revision history | +| C. Atomic decomposition (chosen) | Handles mixed state, no invalid XML | More complex implementation |tests/test_mixed_state.py (1)
67-70: Avoid skips that can mask regressions.
Line 67–70 (and similar skips below) turn missing boundary creation into a silent pass. Consider making the setup deterministic (e.g., crafted XML) and assert the boundary so regressions fail loudly.
When operating on text already inside a pending insertion (<w:ins>), the 7 affected call sites now edit in-place or insert bare runs instead of wrapping in new <w:ins>/<w:del> elements, which would produce invalid OOXML.
Set mock_run.parentNode = None in all 14 mocked edge case tests to prevent infinite loop in _find_ancestor. Also fix _insert_near_match to only skip w:ins wrapper when all positions are inside an insertion, and relax change_id assertion for in-place edits.
- Use proper markdown headings for Goals/Non-Goals in archived design doc - Replace TBD Purpose placeholder in text-operations spec - Fix flaky test assertion to compare node identity instead of start/text
- De-duplicate run removal in _replace_same_context and _delete_same_context to prevent double-remove DOM errors with multi-w:t runs - Preserve unmatched w:t siblings when replacing/deleting runs that contain multiple w:t nodes (both _delete_same_context and _delete_regular_segment) - Guard _remove_from_insertion single-node branch against dropping sibling w:t nodes: remove just the w:t element instead of the entire w:ins - Redirect single-element replace_text/suggest_deletion to cross-boundary path when run has multiple w:t children - Fix mid-node insertion ordering in _replace_mixed_state using marker-based approach: process deletions first, then insert after last w:del
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Line 667: Replace the generator passed to set() with a set comprehension for
clarity: change the creation of matched_node_ids that currently uses
set(id(ng["node"]) for ng in run_info["nodes"].values()) to use a set
comprehension over run_info["nodes"].values(); update the assignment to
matched_node_ids accordingly (refer to the matched_node_ids variable and
run_info["nodes"].values() in track_changes.py).
In
`@openspec/changes/archive/2026-01-29-add-cross-boundary-text-operations/design.md`:
- Around line 3-6: Add a blank line before and after the "## Context" heading
(and any other headings in this document) so the markdown conforms to
markdownlint rule MD022; locate the "## Context" heading in design.md and insert
an empty line above it and another empty line after the heading line (and
similarly for other headings) to ensure proper spacing.
In `@openspec/specs/text-operations/spec.md`:
- Around line 3-6: Add blank lines above and below the markdown headings to
satisfy markdownlint MD022: insert an empty line before "## Purpose" and ensure
there is a blank line between "## Purpose" and the following content, add blank
lines around "## Requirements" and also around "### Requirement: Virtual Text
Map" so each heading is separated by a blank line above and below; update the
three headings ("## Purpose", "## Requirements", "### Requirement: Virtual Text
Map") accordingly.
In `@tests/test_mixed_state.py`:
- Around line 12-15: The helper function _parse_paragraph is duplicated across
tests; extract it and the NS constant into a shared test fixture (e.g., add NS
and parse_paragraph to tests/conftest.py) and replace uses of _parse_paragraph
in tests/test_mixed_state.py and tests/test_text_map.py with imports from
conftest (use a stable name like parse_paragraph and NS). Update the test files
to import parse_paragraph and NS instead of defining _parse_paragraph locally,
and ensure the function signature and behavior match the original
_parse_paragraph.
- Line 49: The assertion comprehension shadows the outer variable p (the
paragraph from earlier), so change the generator variable in the assertion that
references match.positions to a distinct name (e.g., pos or position) to avoid
shadowing; update the expression in tests/test_mixed_state.py where assert
all(p.is_inside_ins for p in match.positions) is written to use the new variable
name (e.g., assert all(pos.is_inside_ins for pos in match.positions)).
- Around line 169-171: The condition uses a short variable name `p` that shadows
another `p` elsewhere; rename the generator variable to a non-conflicting name
(e.g., `pos` or `position`) in the check `all(p.is_inside_ins for p in
match.positions)` so it becomes `all(pos.is_inside_ins for pos in
match.positions)`, and apply the same renaming pattern to any other occurrences
in this file where the loop/generator variable `p` shadows an outer `p` to keep
naming consistent.
- Around line 98-100: The comprehension using `p` shadows the outer paragraph
variable; change the generator in the if condition to use a different name
(e.g., `pos`) so it reads `if not all(pos.is_inside_ins for pos in
match.positions):`, keep the calls to `doc.close()` and `pytest.skip(...)`
unchanged, and ensure the new `pos` identifier is used consistently in that
expression to avoid shadowing the outer `p`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_track_changes_coverage.py`:
- Around line 119-123: The test suite contains empty placeholder test classes
(e.g., TestReplaceSameContextReturnMinusOne) that add no assertions and skew
coverage; remove these empty classes or convert them into explicit xfail tests
by adding a pytest.xfail call in their setup or replacing the class with a test
function that calls pytest.xfail with a short reason; apply the same change to
the other placeholders referenced (around the blocks for lines noted in the
review) so symbols like TestReplaceSameContextReturnMinusOne are either deleted
or replaced with an explicit xfail test that names the intended edge case.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/test_track_changes_coverage.py`:
- Around line 89-100: The test class name and docstring are misleading: rename
the class TestBuildCrossBoundaryPartsSkipsOrphan to something reflecting what it
actually tests (e.g., TestReplaceSameContextEmptyParts) and update the class
docstring to describe that _replace_same_context returns -1 for an empty
TextMapMatch; keep the test method test_empty_parts_returns_minus_one and ensure
the docstring references _replace_same_context and the empty parts scenario so
the intent matches the code.
- Around line 254-264: The test
TestDeleteRegularSegmentIntermediateNode::test_three_wt_nodes_spanning_deletion
currently checks that surrounding text "xx" and "yy" remain but doesn't assert
the deleted content is gone; add an assertion after text =
_get_text_content(mgr) to verify the suggested deletion via
mgr.suggest_deletion("ABCDEF") was applied by asserting the deleted string is
absent (e.g., assert "ABCDEF" not in text) to match the pattern used in
test_multi_node_removal_with_before_and_after.
- Around line 328-340: Rename the leftover suffixed variables in the test method
test_replace_all_inside_ins_removes_then_appends of class
TestReplaceSameContextInsAppendNoFirstChild: change xml_path2 to xml_path and
mgr2 to mgr so they match the naming convention used by other tests; update all
references within the method (including the call _make_manager and subsequent
uses) to the new names.
…d marker scan - Rewrite count_matches to use text maps for accurate cross-boundary counting - Add xml:space="preserve" after in-place .data edits (Sites A, B, C) - Bound marker sibling scan to stop at first non-w:del after finding one - Move OrderedDict import to top-level (remove 4 inline imports)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 396-424: The code currently calls
self._remove_from_insertion(match.positions) which can delete the original ins
element and then inserts a bare <w:r>, losing tracked-change metadata; fix by
preserving or recreating the wrapper: before calling _remove_from_insertion,
clone the found ins element (ins_elem_clone = ins_elem.cloneNode(False)) so you
can reinsert a <w:ins> wrapper if ins_elem is removed, then after parsing
new_run_xml use ins_elem if still present, otherwise create a new_ins =
ins_elem_clone (or cloneNode(False) from saved clone), append/insert the parsed
nodes into new_ins, and insert new_ins into ins_parent before ins_next so the
replacement stays inside a <w:ins> and keeps tracking (refer to match.positions,
ins_elem, ins_parent, ins_next, _remove_from_insertion, new_run_xml, and
editor._parse_fragment).
- Around line 897-935: The _insert_text logic reconstructs an entire <w:r> from
the matched text node (elem) which drops any sibling <w:t> nodes; instead, only
split and replace the specific text node (elem) while preserving other children
of the run. Locate _insert_text and where it gets elem/run and uses
elem.firstChild.data and self.editor.replace_node; build new DOM fragments that
replace the single text node with three text nodes (before, anchor, after) and
an inserted <w:ins><w:r>... node at the correct position, or clone the original
run and splice in the new nodes between existing children, then call
self.editor.replace_node(run, new_xml) with the run’s full child serialization
so sibling <w:t> nodes remain intact; ensure the branch that handles existing
<w:ins> (self._find_ancestor(run, "w:ins")) also updates only
elem.firstChild.data and not drop siblings.
- Around line 594-632: When truncating or replacing text in runs (in code around
first_node/last_node, groups and the single-node split branch), avoid removing
the entire <w:r> unconditionally: locate the specific <w:t> child nodes (use
_find_ancestor to get the run, then run.getElementsByTagName("w:t") or the exact
matched node) and remove only those matched <w:t> nodes; only call
run.parentNode.removeChild(run) if the run has no remaining element children
afterward. Also, whenever you write to node.firstChild.data (the assignments to
before_text/after_text, before, after and the single-node branch where
first_node.firstChild.data and after_xml are created), call
_set_xml_space_preserve on the text node or ensure the element has
xml:space="preserve" so leading/trailing spaces are preserved. Reference
symbols: first_node, last_node, groups, run, _find_ancestor,
_set_xml_space_preserve, self.editor.insert_after, and the single-node split
block that constructs after_xml.
- Around line 426-455: _in _replace_same_context, we are removing entire run
nodes which drops unrelated <w:t> siblings; change the logic to preserve
unmatched siblings per run by: for each entry in parts use the existing run node
(variable run) and only remove or replace the specific matched child text nodes
instead of removing the whole run (do not call parent.removeChild(run)); build
and insert replacement/deletion XML adjacent to the matched child nodes
(insert_before the first matched child or after the last matched child overall)
and if you must create new run nodes use first_rPr for the inserted <w:ins>, but
when cleaning up only remove the specific matched <w:t> or <w:delText> nodes (or
replace their text) mimicking _delete_regular_segment behavior; keep use of
first_run and xml_parts for insertion but adjust cleanup to target child nodes
rather than run nodes so unmatched <w:t> siblings remain.
- Wrap replacement in <w:ins> when ins_elem is fully removed (Site D) - Preserve sibling w:t nodes in _replace_same_context non-ins path - Remove only w:t (not entire run) in _remove_from_insertion multi-node, add xml:space preserve on truncated nodes - Delegate to cross-boundary path in _insert_text for multi-w:t runs
Insert relative to the w:ins ancestor (not the run) when the reference run is inside an existing w:ins, preventing invalid nested markup. Also fix variable naming in test_track_changes_coverage.py.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 660-664: The condition inside the loop over groups[1:-1] is
tautological because node_text is assigned from self._get_node_text(node) just
above; replace the current check with the intended behavior: either (A) only
remove empty intermediate nodes — change the condition to "if not node_text"
(keep self._get_node_text and _remove_wt_and_maybe_run as-is), or (B) remove all
intermediate nodes for cross-boundary matches — drop the if entirely and call
self._remove_wt_and_maybe_run(node) unconditionally; update the block around
node_text, self._get_node_text(node), and the call to _remove_wt_and_maybe_run
accordingly.
- Around line 301-375: Add explicit return type annotations to the private
helpers to improve readability and static checks: annotate _get_run_info as
returning Tuple[Optional[xml.dom.minidom.Node], str] (or Tuple[Optional[Node],
str]), ensure _get_node_text has -> str (if missing), annotate
_build_cross_boundary_parts as returning List[Tuple[xml.dom.minidom.Node, str,
str, str, str]] (i.e., list of (run, rPr_xml, before, matched, after) tuples),
annotate _classify_segments as returning List[Tuple[bool,
List[TextMapPosition]]] (or List[Tuple[bool, List[PositionType]]] depending on
your position class), and annotate _find_ancestor to return
Optional[xml.dom.minidom.Node]; import typing primitives (Optional, Tuple, List)
and the specific DOM/position types used in the module.
In `@tests/test_review_fixes.py`:
- Around line 24-26: Add an explicit return type annotation to the helper
function _make_manager so its signature declares it returns RevisionManager
(e.g., def _make_manager(xml_path) -> RevisionManager:), and ensure the
RevisionManager type is imported or available in the test module namespace so
the annotation is valid; keep the function body unchanged (DocxXMLEditor
creation and return RevisionManager(editor)).
Add explicit return types to private helpers (_get_run_info, _build_cross_boundary_parts, _classify_segments, _find_ancestor). Fix tautological condition in intermediate node removal loop. Add return type to test helper _make_manager.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 526-560: The temporary element created as w:_marker via marker =
self.editor.dom.createElement("w:_marker") is not valid OOXML and could leave
invalid XML if an exception occurs before marker.parentNode.removeChild(marker);
replace the ad-hoc element with a comment node (e.g., use
self.editor.dom.createComment(...) or equivalent DOM comment creation) and
update the insertion/search logic to use that comment as the anchor (adjust uses
of marker.nextSibling, marker.parentNode.insertBefore(marker, ref_node) and
marker.parentNode.removeChild(marker) accordingly) so the temporary anchor is
always a valid node even if an error occurs.
- Around line 819-825: The current matching loop (referencing
matched_parts_by_node, rparts, all_wt and matched_nodes) assigns parts to <w:t>
nodes by picking the first unmatched node, which is fragile; instead use the
node-to-part identity information produced by _build_cross_boundary_parts to map
parts deterministically to their originating w:t nodes. Change the logic so that
for each part in rparts you look up the exact node id (or node object) from the
data structure returned by _build_cross_boundary_parts (or propagate node
identity alongside parts in that builder), and populate
matched_parts_by_node[id(wt)] from that mapping rather than scanning all_wt for
the first unmatched id; ensure matched_nodes is used only as a membership set,
not an ordering mechanism.
Replace ad-hoc w:_marker element with a comment node to avoid invalid OOXML if an exception leaves the marker in the DOM. Propagate node identity from _build_cross_boundary_parts and use it to build deterministic node-to-part mappings in _replace_same_context and _delete_same_context, replacing the fragile scan-based matching.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 151-156: The occurrence index drifts because code paths switch
from _get_nth_match (which counts per-node hits) to _find_across_boundaries
(which uses a global text-map count); instead of reusing the global occurrence
index when you detect multiple w:t children, build the paragraph's text_map,
compute the node+offset range for the already-found elem/occurrence and select
the TextMapMatch whose start..end covers that node/offset, then pass that
specific TextMapMatch into _replace_across_nodes (or into
_find_across_boundaries wrapper) — update the logic in the branches that
currently call _find_across_boundaries (referencing _get_nth_match,
_find_across_boundaries, _replace_across_nodes, TextMapMatch, text_map,
elem/occurrence) so you locate the matching text-map entry for the found element
instead of reusing the global occurrence index.
- Around line 410-425: The current code uses self.editor._parse_fragment plus
DOM insertBefore/appendChild (around symbols ins_elem, ins_parent, ins_next and
new_run_xml), which bypasses DocxXMLEditor's automatic RSID/author/date/w:id and
xml:space injection; change the insertion to use the editor's helper methods
(e.g., insert_before and append_to) instead of raw insertBefore/appendChild so
the editor can inject tracked-change attributes — for the ins_elem branch, parse
the fragment but call self.editor.append_to or self.editor.insert_before on the
run nodes into ins_elem (using ins_elem.firstChild as anchor when needed), and
for the ins_parent branch wrap new_run_xml in <w:ins> then insert using
self.editor.insert_before into ins_parent with ins_next as the anchor.
Add _find_match_containing_node to locate the TextMapMatch for a specific w:t node, avoiding index drift when falling back from _get_nth_match to cross-boundary search in multi-w:t runs. Replace raw _parse_fragment + DOM ops in Site D with editor.insert_before and editor.append_to so RSID/author/date attributes are injected. Rewrite _insert_text multi-w:t handling to split inline rather than delegating to cross-boundary, preserving sibling w:t nodes and correct anchor positioning.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docx_editor/track_changes.py (1)
145-176:⚠️ Potential issue | 🟠 MajorOccurrence selection can still drift when a single w:t contains multiple matches.
count_matches()now counts occurrences via text maps, butreplace_text(and similarlysuggest_deletion/_insert_text) still selects by node using_get_nth_matchand then uses the first in‑node match. If a node contains multiple hits and later nodes also match, occurrence>0 can target the wrong occurrence.Consider aligning occurrence resolution with the text‑map for occurrence>0 (or counting occurrences within each node before choosing). Example approach (apply similarly to delete/insert):
⚙️ Example alignment for occurrence>0
- try: - elem = self._get_nth_match(find, occurrence) - except TextNotFoundError: - # Fall back to cross-boundary search - match = self._find_across_boundaries(find, occurrence) - if match is None: - raise - return self._replace_across_nodes(match, replace_with) + if occurrence > 0: + match = self._find_across_boundaries(find, occurrence) + if match is None: + raise TextNotFoundError(f"Text not found: '{find}'") + return self._replace_across_nodes(match, replace_with) + try: + elem = self._get_nth_match(find, occurrence) + except TextNotFoundError: + match = self._find_across_boundaries(find, occurrence) + if match is None: + raise + return self._replace_across_nodes(match, replace_with)
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 683-689: The current _remove_wt_and_maybe_run removes the ancestor
w:r whenever there are no w:t children, but that can delete other non-text
children (tabs, breaks, drawings). Change the removal condition in
_remove_wt_and_maybe_run so it only removes the run if it contains no element
children other than w:t: after removing the w:t node, inspect run.childNodes and
skip removal if any child is an ELEMENT_NODE whose tagName is not "w:t" (i.e.,
preserve w:tab, w:br, drawing elements); otherwise proceed to remove the run as
before.
Guard _remove_wt_and_maybe_run to keep the parent w:r if it still contains content children like w:tab, w:br, or w:drawing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 448-509: The current rebuild in methods using
run_order/run_map/parts/node_to_part/all_wt/xml_parts replaces whole <w:r>
elements and thus drops non-text children like <w:tab>, <w:br>, <w:drawing>;
change the approach to preserve non-text children by cloning the original run
node and only splicing/replacing its <w:t> children (or mutate matched <w:t>
nodes in place) instead of reconstructing a new <w:r> from rPr_xml alone.
Specifically, for each info["run"] (from run_map) use run.cloneNode(False) or
similar to get the run shell (attributes and non-<w:t> children), iterate its
child nodes and for each <w:t> decide to emit preserved text, wrap matched text
in <w:del>/<w:delText> and insert <w:ins> at the correct spot, appending
non-<w:t> children unchanged; then call self.editor.insert_before(new_xml) and
remove originals as before (first_run, parts, parent.removeChild) so non-text
children are retained.
- Around line 634-651: When updating or removing a single w:t node in this
block, ensure leading/trailing spaces are preserved by calling
_set_xml_space_preserve on the text node before mutating
first_node.firstChild.data in the branches that assign after_text or
before_text; and instead of directly removing the run (the code that checks run
and run.getElementsByTagName("w:t")), delegate to the existing helper that knows
how to handle non-text children—reuse _remove_wt_and_maybe_run (or copy its
non-text-child guard) when removing the w:t and potentially its w:r ancestor so
tabs/drawings aren’t lost; keep references to ins_elem, first_node,
_get_wt_nodes_in_ancestor and _find_ancestor to locate the correct nodes.
…rker placement Verify that xml:space="preserve" is set on generated w:t with leading/trailing spaces, that cross-boundary occurrence fallback targets the correct match, and that mixed-state marker scan places insertions after deletions (not before prefix text).
- Rebuilding runs from only <w:t> drops non-text run children. - Preserve whitespace and non-text children in single-node removal.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docx_editor/track_changes.py`:
- Around line 948-952: The long inline exception message in the fallback path
should be moved into the exception class: define or update the TextNotFoundError
class to accept the anchor (and optionally occurrence) and construct the
formatted message internally, then replace raise TextNotFoundError(f"Anchor text
not found: '{anchor}'") from None with raise TextNotFoundError(anchor) (or raise
TextNotFoundError(anchor, occurrence)), leaving the surrounding logic in
_find_across_boundaries and _insert_near_match unchanged so the error
construction follows TRY003 style.
4 tasks done
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Context
Addresses feedback from @gabrielpacosta in anthropics/claude-code#9631: the library cannot find or edit text that spans tracked change boundaries (
<w:ins>,<w:del>).Proposal Summary
The proposal adds a Virtual Text Map — a flattened text view that maps character positions back to source XML nodes. This enables:
<w:t>elementsget_visible_text()API — read-only flattened text for analysisKey design decision: atomic decomposition over rejection. When a replace spans
<w:ins>/<w:del>boundaries, the operation is decomposed into per-segment actions (wrap regular text in<w:del>, split and remove inserted text, insert new text) — avoiding invalid XML nesting.Design Constraints
w:rPrpreservation — split run fragments keep their source formatting.docxFiles
openspec/project.md— project contextopenspec/changes/add-cross-boundary-text-operations/proposal.md— why and whatopenspec/changes/add-cross-boundary-text-operations/design.md— technical decisionsopenspec/changes/add-cross-boundary-text-operations/tasks.md— implementation planopenspec/changes/add-cross-boundary-text-operations/specs/text-operations/spec.md— requirements with scenariosCloses #1
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.