Repository navigation
feat: add rewrite_paragraph and return paragraph refs from all edit methods - #11
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Document.rewrite_paragraph() and Document.batch_rewrite() plus RevisionManager implementations that compute word-level diffs (tokenize + SequenceMatcher), map diffs to XML character positions via TextMap, and emit fine-grained Word tracked-change edits (<w:del>/<w:ins>) applied in reverse order; includes docs and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Doc as Document
participant RM as RevisionManager
participant TM as TextMap
participant Tok as Tokenizer
participant SM as SequenceMatcher
participant XML as XMLEditor
Client->>Doc: rewrite_paragraph(ref, new_text)
Doc->>RM: rewrite_paragraph(ref, new_text)
RM->>RM: _resolve_paragraph(ref) / validate hash
RM->>TM: build_text_map(paragraph)
TM-->>RM: text_map
RM->>Tok: _tokenize_words(old_text)
RM->>Tok: _tokenize_words(new_text)
Tok-->>RM: old_tokens / new_tokens
RM->>SM: compute opcodes(old_tokens, new_tokens)
SM-->>RM: opcodes
loop apply hunks (in reverse)
alt delete / replace
RM->>RM: _find_match_at_position(text_map, search, pos)
RM->>XML: apply <w:del> / replace across runs
end
alt insert / replace
RM->>RM: _rewrite_insert_at(paragraph, text_map, char_pos, text)
RM->>XML: create/splice <w:ins>, split runs, inherit w:rPr
end
end
RM-->>Doc: mutated document (tracked changes)
Doc-->>Client: return
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
=======================================
- Coverage 93.7% 93.6% -0.2%
=======================================
Files 10 10
Lines 1993 2133 +140
Branches 458 485 +27
=======================================
+ Hits 1869 1998 +129
- Misses 31 35 +4
- Partials 93 100 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add rewrite_paragraph(ref, new_text) to RevisionManager and Document that automatically diffs old vs new text at word level and generates fine-grained tracked changes (insertions, deletions, replacements).
Accepts a list of (ref, new_text) pairs and applies them atomically: - Validates all paragraph hashes upfront (rejects entire batch on mismatch) - Rejects duplicate paragraph references - Applies rewrites in reverse paragraph order (highest index first) - Delegates each individual rewrite to rewrite_paragraph()
Add TestRewriteWithTrackedChanges class covering: - Rewrite after prior surgical edit (existing tracked changes) - Save and reopen round-trip preserving revisions - Individual rewrite after batch rewrite
Add paragraph rewrite section to SKILL.md with decision table, concrete examples showing when rewrite is better than surgical edits, and batch_rewrite usage. Update README.md features and quick start.
Proposal for rewrite_paragraph() and batch_rewrite() with word-level diffing, design decisions, and spec deltas for text-operations.
pablospe
force-pushed
the
user/pablo/rewrites
branch
from
April 2, 2026 12:17
6966190 to
2a6303a
Compare
- Use _find_match_at_position() to match text at expected character positions instead of always finding first occurrence. Fixes incorrect edits when the same text appears multiple times in a paragraph. - Raise RevisionError when a hunk's text cannot be found after DOM mutation, instead of silently skipping. - Add tests for duplicate text scenarios.
Replaces vague "3+ changes" heuristic with a mechanical decision rule: "Can each change be expressed as an independent find→replace?" If yes, use surgical methods. If no (restructuring, reordering, intertwined changes), use rewrite_paragraph. Tested with 10 LLM scenarios (5 clear-cut + 5 ambiguous edge cases), all correctly classified.
7 new tests covering real-world rewrite_paragraph use cases: - passive→active voice conversion - list item reordering - full sentence rephrasing - prose tightening (multiple deletions) - duplicate phrases with different replacements per occurrence - clause insertion combined with value change - duplicate word with different substitutions
- Remove redundant _find_ancestor call in _rewrite_insert_at (always None after early return on the truthy branch) - Make test_formatting_preserved assert unconditionally that rPr exists on w:ins elements, instead of silently skipping when absent Skipped: naming inconsistency (ref vs ref_str) — consistent within new methods; multi-w:t sibling handling — pre-existing pattern limitation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docx_editor/track_changes.py`:
- Around line 377-391: The current split logic replaces the entire <w:r> and
loses sibling <w:t> elements; update the block that builds new_xml to iterate
run.getElementsByTagName("w:t") and for each wt: if wt is pos.node, split its
text into before_text/after_text and append preserved <w:t> for before_text,
then the <w:ins><w:r>...<w:t>text</w:t></w:r></w:ins>, then preserved <w:t> for
after_text (using rPr_xml and _escape_xml), else append the sibling wt unchanged
(serialized) so all non-target <w:t> siblings are preserved before calling
self.editor.replace_node(run, new_xml); use the same variables (run, pos.node,
rPr_xml, text, _escape_xml, self.editor.replace_node) to locate and implement
the change.
- Around line 356-359: The duplicated ancestor lookup ins_ancestor_ref =
self._find_ancestor(run, "w:ins") is redundant because an earlier ins_ancestor =
self._find_ancestor(run, "w:ins") already returns early if truthy; remove the
second call and the ins_ancestor_ref variable and always use run as the insert
reference when calling self.editor.insert_after(run, ins_xml). Update the
surrounding code in the same block that builds ins_xml so it no longer declares
or uses ins_ancestor_ref and simply calls self.editor.insert_after(run,
ins_xml).
- Around line 344-346: The early silent return after calling
self._get_run_info(last_pos.node) should be replaced with raising RevisionError
to avoid masking DOM corruption or unexpected document structure; update the
method containing this snippet so that if _get_run_info(...) returns a falsy
run, it raises RevisionError with a clear message (include context like
last_pos.node or the failing node) instead of returning, keeping behavior
consistent with other failure modes in this class.
In `@openspec/changes/add-rewrite-paragraph/design.md`:
- Line 1: Add a top-level H1 heading at the very start of the design document
and leave the existing sections (like the "## Context" heading) unchanged
beneath it; specifically, insert a single-line H1 (e.g., "# Design: Add Rewrite
Paragraph" or another descriptive title) as the first line of
openspec/changes/add-rewrite-paragraph/design.md so the file begins with H1 and
the current "## Context" remains the first subsection.
In `@openspec/changes/add-rewrite-paragraph/tasks.md`:
- Line 1: The file's first line uses a level-2 heading ("## 1. Core Diffing
Infrastructure") which violates markdownlint MD041; add a top-level H1 title
line above that section (e.g., "# ...") so the document begins with an H1 before
"## 1. Core Diffing Infrastructure" to satisfy the rule and keep existing
headings intact.
In `@skills/docx/SKILL.md`:
- Around line 433-437: The example calling batch_edit() uses plain dicts instead
of EditOperation objects; update the example to construct EditOperation
instances for each edit (use the EditOperation type expected by batch_edit), set
action="replace", move "replace"->"replace_with", keep "find" and "paragraph"
fields, and pass the list of EditOperation objects to batch_edit() so the
function receives the proper shape (refer to batch_edit and EditOperation, and
the required fields action, find, replace_with, paragraph).
In `@tests/test_rewrite_paragraph.py`:
- Line 615: The test assigns an unused second value from the fixture return
(tmp) when calling rewrite_doc; remove the unused binding by only capturing the
value you need (doc) or using the conventional underscore placeholder for the
second value so Ruff RUF059 is no longer triggered; update the call site where
rewrite_doc is unpacked (the assignment to doc, tmp) accordingly.
- Around line 265-270: The current test only asserts that if an insertion
element ("w:ins") has a "w:rPr" child then that "w:rPr" contains a "w:b", which
can pass even if no inserted run actually has bold; change the check to require
that at least one inserted run under editor.dom.getElementsByTagName("w:ins")
contains a "w:rPr" element with a "w:b" child. Concretely, iterate inserted runs
inside each "w:ins" (e.g., find "w:r" or direct children of the "w:ins"),
collect whether any run's "w:rPr" contains "w:b", and assert that this
collection has at least one true entry so the test fails when no inserted run
inherits bold formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0bc8cf2-898e-4793-a802-5c3862f9b94d
📒 Files selected for processing (9)
README.mddocx_editor/document.pydocx_editor/track_changes.pyopenspec/changes/add-rewrite-paragraph/design.mdopenspec/changes/add-rewrite-paragraph/proposal.mdopenspec/changes/add-rewrite-paragraph/specs/text-operations/spec.mdopenspec/changes/add-rewrite-paragraph/tasks.mdskills/docx/SKILL.mdtests/test_rewrite_paragraph.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docx_editor/track_changes.py`:
- Around line 362-364: The code currently returns silently when
_get_run_info(pos.node) yields run is None (seen where run, rPr_xml =
self._get_run_info(pos.node); if not run: return), which can hide DOM problems;
instead replace the silent return with a clear failure path: either log an error
including pos.node and rPr_xml and raise a descriptive exception (e.g.,
ValueError or custom DOMParseError) or raise immediately with a message
referencing _get_run_info and pos.node so callers cannot continue on invalid
state; ensure the change is applied in the mid-text insertion method that
contains this snippet so callers get explicit failure instead of silent no-op.
In `@tests/test_rewrite_paragraph.py`:
- Line 354: Replace the Yoda condition in the assertion so the variable
expression comes first: change the equality check in the assert that iterates
over ins_texts (currently using "a" == t.strip()) to use t.strip() == "a"
instead; update the assertion that uses any("a" == t.strip() for t in ins_texts)
to any(t.strip() == "a" for t in ins_texts) to improve readability.
- Line 357: The helper function _make_doc_with_paragraphs is missing an explicit
return type; update its signature to include a concrete return annotation (for
example the actual Document type used in the codebase or typing.Any if unsure)
to improve IDE support and docs, and add any necessary import (e.g., from typing
import Any or the Document type) so the annotated return type resolves
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5630576-7420-4796-9c6a-3860813068fc
📒 Files selected for processing (2)
docx_editor/track_changes.pytests/test_rewrite_paragraph.py
- SKILL.md: batch_edit example used plain dicts instead of EditOperation objects — would cause runtime errors - Fix Yoda condition in test assertion Skipped from review: silent returns (pre-existing pattern), sibling w:t loss (pre-existing limitation), markdown lint (OpenSpec convention).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_rewrite_paragraph.py (2)
357-357: 🧹 Nitpick | 🔵 TrivialAdd a return type annotation to
_make_doc_with_paragraphs.Line 357 is missing an explicit return annotation, which keeps ANN202 noisy.
🧹 Suggested fix
-def _make_doc_with_paragraphs(paragraphs: list[str]): +def _make_doc_with_paragraphs(paragraphs: list[str]) -> tuple[Document, Path]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rewrite_paragraph.py` at line 357, The helper function _make_doc_with_paragraphs lacks an explicit return type annotation; update its signature to include a concrete return type (e.g., -> Document or the actual type the function constructs) so ANN202 is satisfied. Locate the _make_doc_with_paragraphs definition and add the correct return type annotation matching the object it returns (or use typing.Any if uncertain), and run tests/typechecks to confirm the annotation is correct.
615-615:⚠️ Potential issue | 🟡 MinorUse
_for the unused fixture value.
tmpis unpacked but unused on Line 615.🧹 Suggested fix
- doc, tmp = rewrite_doc + doc, _ = rewrite_doc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rewrite_paragraph.py` at line 615, The tuple unpacking assigns an unused fixture value to tmp in the test (doc, tmp = rewrite_doc); change tmp to the conventional unused variable name _ by updating the unpacking to (doc, _) wherever rewrite_doc is unpacked in tests (e.g., in tests/test_rewrite_paragraph.py) so the intent is clear and linters won’t flag the unused variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/docx/SKILL.md`:
- Around line 459-460: The batch_rewrite example uses the variable author when
calling Document.open but never defines it; add a preceding definition of author
(e.g., assign a sample string or pull from function args/config) before the
Document.open(...) call so the example is runnable. Update the example around
Document.open("contract.docx", author=author) and refs = doc.list_paragraphs()
to include a clear author assignment (or note it comes from the surrounding
scope) so readers can copy-paste and run the snippet.
---
Duplicate comments:
In `@tests/test_rewrite_paragraph.py`:
- Line 357: The helper function _make_doc_with_paragraphs lacks an explicit
return type annotation; update its signature to include a concrete return type
(e.g., -> Document or the actual type the function constructs) so ANN202 is
satisfied. Locate the _make_doc_with_paragraphs definition and add the correct
return type annotation matching the object it returns (or use typing.Any if
uncertain), and run tests/typechecks to confirm the annotation is correct.
- Line 615: The tuple unpacking assigns an unused fixture value to tmp in the
test (doc, tmp = rewrite_doc); change tmp to the conventional unused variable
name _ by updating the unpacking to (doc, _) wherever rewrite_doc is unpacked in
tests (e.g., in tests/test_rewrite_paragraph.py) so the intent is clear and
linters won’t flag the unused variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bae3a8f-2717-486e-ab4e-2c39b51fd50a
📒 Files selected for processing (2)
skills/docx/SKILL.mdtests/test_rewrite_paragraph.py
After batch edits, LLMs only need fresh hashes — not full previews. list_paragraphs(max_chars=20) reduces output tokens when the LLM already knows which paragraphs it edited.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
skills/docx/SKILL.md (1)
466-473:⚠️ Potential issue | 🟡 MinorDefine
authorvariable before using it in the example.The code block uses
author=authoron Line 466 without defining theauthorvariable, which will cause aNameErrorif users copy-paste this example.This issue was previously flagged but remains unfixed.
🛠️ Suggested fix
+import os +author = os.environ.get("USER") or os.environ.get("USERNAME") or "Reviewer" + with Document.open("contract.docx", author=author) as doc: refs = doc.list_paragraphs() doc.batch_rewrite([ (refs[1].split("|")[0], "Rephrased paragraph 2 text here."), (refs[4].split("|")[0], "Restructured paragraph 5 text here."), ]) doc.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/docx/SKILL.md` around lines 466 - 473, The example uses Document.open(..., author=author) without defining author, causing a NameError; add a prior definition for the author variable (e.g., a string username or Author-type expected by the SDK) before calling Document.open in the example so refs = doc.list_paragraphs(), doc.batch_rewrite(...) and doc.save() run correctly; update the sample to declare author (referencing the example's use of Document.open, list_paragraphs, batch_rewrite, save) and ensure its value matches the API's expected type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/docx/SKILL.md`:
- Around line 414-420: The wording "faster hash refresh" is misleading because
list_paragraphs(max_chars=...) only truncates returned preview text and does not
change how hashes are computed; update the SKILL.md text to state that max_chars
reduces the size of returned preview output (and thus output token usage) but
does not affect hash computation, and reference the actual
functions/implementation: list_paragraphs(max_chars=...) controls preview length
while compute_paragraph_hash(p) (used by the paragraph-hashing code path in
document.py) always computes the hash from the paragraph DOM regardless of
max_chars.
---
Duplicate comments:
In `@skills/docx/SKILL.md`:
- Around line 466-473: The example uses Document.open(..., author=author)
without defining author, causing a NameError; add a prior definition for the
author variable (e.g., a string username or Author-type expected by the SDK)
before calling Document.open in the example so refs = doc.list_paragraphs(),
doc.batch_rewrite(...) and doc.save() run correctly; update the sample to
declare author (referencing the example's use of Document.open, list_paragraphs,
batch_rewrite, save) and ensure its value matches the API's expected type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 726e0137-7583-4f27-9dcc-a4ee504848fd
📒 Files selected for processing (1)
skills/docx/SKILL.md
Prevents LLMs from passing absurdly low values like max_chars=1 which would produce useless single-character previews.
Reverts minimum enforcement. max_chars=0 gives a clean "just hashes" mode for refreshing refs after edits without wasting tokens on previews.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docx_editor/document.py`:
- Around line 312-329: The docstring for Document.batch_rewrite claims atomic
behavior but RevisionManager.batch_rewrite applies edits one-by-one and lacks
rollback; either implement true transaction semantics in
RevisionManager.batch_rewrite or remove the atomic claim—preferably implement
rollback by first validating all upfront paragraph hashes (use the same
validation logic currently used per-edit), then apply edits to a copy or staged
buffer and only commit the changes to the live document if all edits succeed (or
on failure discard the copy/staged changes and leave the document unchanged);
update or keep references to Document.batch_rewrite and
RevisionManager.batch_rewrite when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8df95df7-7eb0-435e-8273-972dec113701
📒 Files selected for processing (1)
docx_editor/document.py
All edit methods (replace, delete, insert_after, insert_before, rewrite_paragraph, batch_edit, batch_rewrite) now return the new paragraph reference after mutation. This eliminates the need to call list_paragraphs() between sequential edits on the same paragraph. EditResult subclasses int for backwards compatibility — existing code using the return value as a change ID for accept/reject still works. New code uses .ref for the fresh paragraph hash. Adds 9 tests covering ref chaining, backwards compat, and batch refs. Updates SKILL.md with ref chaining examples.
All edit methods (replace, delete, insert_after, insert_before, batch_edit) now return the new paragraph reference as a plain string, matching rewrite_paragraph and batch_rewrite. The EditResult class is removed entirely — change IDs are available via list_revisions().
Show chaining pattern throughout, document list_revisions() for getting change IDs, remove stale "returns int change ID" text.
- Remove misleading "atomically" from batch_rewrite docstrings — upfront hash validation prevents stale edits, but there's no rollback on failure - Add author definition to batch_rewrite SKILL.md example for self-contained usage
- Fix ruff import sorting and line length violations - Fix unused variable r3 in test - Show ref chaining in README Quick Start - Fix author inconsistency in Quick Start example
pablospe
changed the title
Cover empty paragraph, insert-at-end, insert inside w:ins, sectPr, and formatted empty paragraph paths. Raises patch coverage from 76% to ~85%.
4 tasks done
8 tasks done
This was referenced Jul 14, 2026
Merged
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
1 participant
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
rewrite_paragraph(ref, new_text)— specify desired paragraph text, auto-generates fine-grained tracked changes via word-level diffingbatch_rewrite(rewrites)— multi-paragraph rewrite with upfront hash validationstr— enables chaining edits without callinglist_paragraphs()between themReturn value change
All edit methods (
replace,delete,insert_after,insert_before,batch_edit) now return the new paragraph reference (str) instead of a change ID (int). This enables:Change IDs for accept/reject are available via
doc.list_revisions().When to use rewrite vs surgical methods
Default: always use surgical methods (replace, delete, insert_after, insert_before, batch_edit).
Use rewrite_paragraph() only when the edit cannot be decomposed into independent find-replace pairs:
Test plan