Repository navigation
feat: hash-anchored paragraph references, batch editing, and required paragraph scoping - #10
Merged
Merged
Conversation
Add content-derived hashes to paragraph references so edits can be scoped to a specific paragraph and stale edits are rejected before corrupting the document. New APIs: - Document.list_paragraphs() returns hash-tagged previews (P1#a7b2| ...) - Optional `paragraph` param on replace/delete/insert_after/insert_before - HashMismatchError raised when paragraph content changed since last read - ParagraphRef dataclass and compute_paragraph_hash utility All existing APIs remain backward-compatible (paragraph param is optional).
Speed: ~0.1ms overhead per operation (negligible). Accuracy: plain occurrence-based edits silently corrupt 9/10 paragraphs after an index-shifting edit; hash-anchored gets 10/10.
…cation New Document.batch_edit() accepts a list of EditOperation objects, validates all paragraph hashes upfront (rejects entire batch if any stale), then applies edits in reverse paragraph order so a single list_paragraphs() snapshot suffices for the whole batch. Includes EditOperation dataclass, 8 tests, and OpenSpec proposal.
Batch mode is 1.8x faster than individual calls for 10 edits (21.6ms vs 39.7ms) by avoiding redundant list_paragraphs() calls.
BREAKING: Document.replace(), delete(), insert_after(), insert_before() now require the `paragraph` keyword argument (hash-anchored ref from list_paragraphs()). Plain occurrence-based editing is removed. This eliminates silent corruption when edits shift paragraph indices. The benchmark showed plain mode silently edited the wrong paragraph in 9/10 cases after an index-shifting edit. Updated all tests (120 call sites), README, and SKILL.md.
|
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:
📝 WalkthroughWalkthroughAdded paragraph hash-anchored references and listing, made edit APIs paragraph-scoped, introduced atomic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant RevisionManager
participant XMLModel
User->>Document: list_paragraphs()
Document->>RevisionManager: build_text_map per w:p / compute_paragraph_hash
RevisionManager->>XMLModel: read paragraph nodes
XMLModel-->>RevisionManager: paragraph nodes + visible text
RevisionManager-->>Document: ["P{n}#{hash}| preview"...]
User->>Document: batch_edit([EditOperation...])
Document->>RevisionManager: batch_edit(operations)
RevisionManager->>RevisionManager: parse ParagraphRef for each op
loop validate all refs
RevisionManager->>XMLModel: resolve by index -> read paragraph
RevisionManager->>RevisionManager: compute_paragraph_hash and compare
end
alt any mismatch
RevisionManager-->>Document: raise HashMismatchError (abort)
Document-->>User: propagate HashMismatchError
else all valid
RevisionManager->>RevisionManager: sort ops by paragraph index desc
loop apply ops
RevisionManager->>XMLModel: apply replace/insert/delete within resolved paragraph
XMLModel-->>RevisionManager: change id
end
RevisionManager-->>Document: return [change_id...]
Document-->>User: return change ids
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/hash_anchored_vs_plain.py`:
- Around line 83-96: In the benchmark_speed function remove the unused local
variable assignment to paragraphs (the doc.list_paragraphs() call) since the
document is immediately closed and the variable is never used; update the
function by deleting the assignment line or replacing it with a throwaway
underscore (e.g., _ = doc.list_paragraphs()) so there are no unused-variable
warnings for benchmark_speed, build_multi_paragraph_doc, or
doc.list_paragraphs().
- Around line 244-270: The f-string used in the call doc1.replace("committee",
f"EDITED", occurrence=old_occ) has no placeholders—remove the unnecessary f
prefix so the literal is "EDITED"; also change the exception handler from except
Exception as e: to except Exception: (or otherwise use/log e) to avoid an unused
variable, leaving the existing plain_error increment and comment behavior
unchanged.
- Around line 412-414: The second print in benchmarks/hash_anchored_vs_plain.py
is using an f-string with no placeholders (print(f" Batch: 1 x
list_paragraphs() + 1 x batch_edit()")); remove the leading f so it becomes a
normal string literal. Keep the first print (print(f" Individual: {n_edits} x
list_paragraphs() + {n_edits} x replace()")) as an f-string since it uses the
n_edits placeholder.
In `@openspec/changes/add-batch-edit/proposal.md`:
- Around line 18-19: Update the proposal's impact statement to accurately
reflect that the change is breaking: replace "Non-breaking" with "Breaking" (or
explicitly state that edit methods now require the paragraph parameter) and call
out the dependency on add-paragraph-hash-anchors which introduces ParagraphRef,
HashMismatchError, and the _resolve_paragraph behavior; ensure the Depends on
section and any consumer guidance mention that existing edit methods must
include the paragraph argument to avoid downstream integration issues.
In `@openspec/changes/add-paragraph-hash-anchors/design.md`:
- Around line 19-20: Update the design doc text that currently says "Keep the
API simple — one new optional parameter on existing methods" (and the related
lines 53-54) to reflect the implemented behavior: state that paragraph
parameters are not optional but enforced and that batch editing support has
already been implemented in this branch; remove or rephrase any forward-looking
language about "future" batch editing and optional params so the doc matches
current implementation and API surface.
In `@openspec/changes/add-paragraph-hash-anchors/proposal.md`:
- Line 25: Update the Impact section to reflect that this is a breaking change:
replace the current line stating "- **Non-breaking**: All new parameters are
optional. Existing API is fully preserved." with text that clearly marks the
proposal as "- **Breaking**" and explains that the new required paragraph API
(the required "paragraph" parameter) changes callers' contracts; also add a
short migration note describing that callers must supply the paragraph parameter
(or how to compute it) and the expected failure modes for missing/invalid
paragraph values so integrators know how to update their code.
In `@README.md`:
- Around line 129-132: The README snippet calls Document.open("contract.docx",
author=author) but references an undefined variable author; fix by either adding
an import and definition (e.g., import os and set author = os.getenv("USER") or
a literal string) before calling Document.open, or remove the named author
argument entirely so Document.open("contract.docx") is used; update the example
around Document.open and the top of the snippet to ensure it runs standalone.
In `@skills/docx/SKILL.md`:
- Around line 417-427: Add a blank line before and after each fenced code block
inside the numbered list (the triple-backtick blocks shown after steps "2." and
"3.") so the markdown has an empty line separating list items from the
```python``` blocks and after the closing ```; this will eliminate MD031
warnings and preserve formatting for the code examples in SKILL.md.
- Around line 400-403: The example uses Document.open("file.docx",
author=author) but author is undefined; either define a local variable (e.g.,
author = "Your Name") before calling Document.open or remove the author argument
so it reads Document.open("file.docx") to make the SKILL.md snippet runnable;
update the snippet around Document.open and ensure the variable is declared in
the same code block so list_paragraphs() can be executed without NameError.
In `@tests/conftest.py`:
- Around line 11-16: The helper function find_ref currently returns the first
paragraph when the search text isn't found, which can hide test problems; update
find_ref to raise an explicit exception (e.g., ValueError or AssertionError)
when text is not found instead of silently returning
doc.list_paragraphs()[0].split("|")[0], or if a fallback is intentional, make
the behavior explicit by adding a clear docstring note and/or a named parameter
like allow_fallback=False that controls returning the first paragraph; reference
the find_ref function and doc.list_paragraphs() when making the change.
In `@tests/test_batch_edit.py`:
- Line 224: The assertion `assert "item 5" not in vis or "[P05]" in vis` is
ambiguous and can pass even if the deletion failed; update the test in
tests/test_batch_edit.py to explicitly assert the marker remains and the deleted
text is gone by replacing that single ambiguous line with two clear checks:
ensure `"[P05]"` is in the `vis` string (marker preserved) and ensure `"item 5"`
is not present when excluding the marker (e.g., `assert "item 5" not in
vis.replace("[P05]", "")`) so the deletion is unambiguous.
- Around line 153-160: The loop that builds ops can be simplified into a list
comprehension: replace the for-loop that appends EditOperation instances to ops
with a single assignment using a list comprehension that iterates over range(10)
and constructs EditOperation(action="replace", find=f"item {i + 1}",
replace_with=f"BATCH_{i + 1}", paragraph=refs[i].split("|")[0]) for each i; this
keeps the same semantics and uses the existing EditOperation and refs symbols.
In `@tests/test_coverage_gaps.py`:
- Around line 421-423: The test anchors the replacement to an ambiguous
paragraph by calling find_ref(doc, "test") and then using doc.replace(...,
paragraph=ref, occurrence=1) which makes occurrence paragraph-local; instead,
locate the second global occurrence and replace it—either call find_ref(doc,
"test", occurrence=2) and pass that ref to doc.replace, or call
doc.replace("test", "REPLACED", occurrence=2) without providing paragraph—update
the code around find_ref, doc.replace, and occurrence to use one of these
approaches so the test targets the second global occurrence reliably.
In `@tests/test_document.py`:
- Around line 182-191: The tests call doc.replace, doc.delete, doc.insert_after,
and doc.insert_before with paragraph="dummy", which may be rejected for format
reasons before hitting the closed-state check; update those calls to use a
syntactically valid placeholder paragraph reference (e.g., a valid paragraph id
string your code accepts) so the ValueError("closed") path is exercised reliably
and the tests fail only for the closed-state condition.
In `@tests/test_paragraph_hash.py`:
- Around line 191-196: The helper function _get_paragraph_ref is missing a
return type annotation; update its signature to include a string return type
(e.g., def _get_paragraph_ref(self, doc, index) -> str:) so static analysis
(ANN202) is satisfied and the function clearly documents that it returns the
paragraph reference string extracted from entry.split("|")[0].
- Around line 45-73: Update the failing tests that call ParagraphRef.parse to
assert the specific error text by adding a match parameter to
pytest.raises(ValueError) in test_reject_missing_hash,
test_reject_uppercase_hash, test_reject_short_hash, test_reject_long_hash,
test_reject_empty_string, and test_reject_no_p_prefix: keep
test_reject_zero_index as-is, and for each failing case supply a regex that
matches the message raised by ParagraphRef.parse (e.g., match=r"missing hash",
match=r"hash must be lowercase hex", match=r"hash too short", match=r"hash too
long", match=r"empty reference", match=r"missing 'P' prefix" respectively) so
the tests verify the specific validation error rather than any ValueError.
🪄 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: 68742261-1dfa-4bec-84c0-dac10904fd25
📒 Files selected for processing (24)
README.mdbenchmarks/hash_anchored_vs_plain.pydocx_editor/__init__.pydocx_editor/document.pydocx_editor/exceptions.pydocx_editor/track_changes.pydocx_editor/xml_editor.pyopenspec/changes/add-batch-edit/proposal.mdopenspec/changes/add-batch-edit/specs/text-operations/spec.mdopenspec/changes/add-batch-edit/tasks.mdopenspec/changes/add-paragraph-hash-anchors/design.mdopenspec/changes/add-paragraph-hash-anchors/proposal.mdopenspec/changes/add-paragraph-hash-anchors/specs/text-operations/spec.mdopenspec/changes/add-paragraph-hash-anchors/tasks.mdskills/docx/SKILL.mdtests/conftest.pytests/test_batch_edit.pytests/test_coverage_gaps.pytests/test_cross_boundary_replace.pytests/test_document.pytests/test_mixed_state.pytests/test_multi_wt.pytests/test_paragraph_hash.pytests/test_track_changes.py
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
tests/test_batch_edit.py (2)
166-175: 🧹 Nitpick | 🔵 TrivialConsider using a list comprehension.
The loop can be simplified to a list comprehension for clarity and minor performance improvement.
♻️ Suggested refactor
- ops = [] - for i in range(10): - ops.append( - EditOperation( - action="replace", - find=f"item {i + 1}", - replace_with=f"BATCH_{i + 1}", - paragraph=refs[i].split("|")[0], - ) - ) + ops = [ + EditOperation( + action="replace", + find=f"item {i + 1}", + replace_with=f"BATCH_{i + 1}", + paragraph=refs[i].split("|")[0], + ) + for i in range(10) + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_batch_edit.py` around lines 166 - 175, Replace the explicit loop that builds ops with a list comprehension to simplify and clarify creation of EditOperation objects: construct ops = [EditOperation(action="replace", find=f"item {i+1}", replace_with=f"BATCH_{i+1}", paragraph=refs[i].split("|")[0]) for i in range(10)] so you still use EditOperation, refs, and the same string formatting while removing the manual append loop.
247-250:⚠️ Potential issue | 🟡 MinorAssertion logic is ambiguous and may not verify deletion.
The condition
"item 5" not in vis or "[P05]" in visalways passes if"[P05]"exists (which it should, since the marker isn't deleted). This doesn't actually verify that"item 5"was removed.🔧 Suggested fix
vis = doc.get_visible_text() assert "REPLACED" in vis - assert "item 5" not in vis or "[P05]" in vis # "item 5" deleted + # Verify "item 5" was deleted but marker [P05] remains + assert "[P05]" in vis # Marker preserved + # Check that "item 5" text (not the marker) is gone + lines_with_p05 = [line for line in vis.split("\n") if "[P05]" in line] + assert lines_with_p05 # P05 paragraph exists + assert all("item 5" not in line for line in lines_with_p05) # "item 5" deleted from P05 assert "[APPENDED]" in vis🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_batch_edit.py` around lines 247 - 250, The current assertion uses "or" which can hide a failure to remove "item 5" if "[P05]" is present; change the assertion to explicitly verify deletion and marker presence by asserting that "item 5" is not in vis AND "[P05]" is in vis (e.g., replace `assert "item 5" not in vis or "[P05]" in vis` with two checks or a single combined check ensuring both conditions) so get_visible_text() truly shows "item 5" was removed while the "[P05]" marker remains.tests/test_paragraph_hash.py (2)
45-73: 🧹 Nitpick | 🔵 TrivialAdd
matchparameter topytest.raises(ValueError)for specificity.Multiple tests use
pytest.raises(ValueError)without amatchparameter, which could mask unexpected errors.♻️ Suggested improvements
def test_reject_missing_hash(self): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid paragraph reference"): ParagraphRef.parse("P3") def test_reject_uppercase_hash(self): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid paragraph reference"): ParagraphRef.parse("P3#A7B2") def test_reject_short_hash(self): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid paragraph reference"): ParagraphRef.parse("P3#a7b") def test_reject_long_hash(self): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid paragraph reference"): ParagraphRef.parse("P3#a7b2c") def test_reject_empty_string(self): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid paragraph reference"): ParagraphRef.parse("") def test_reject_no_p_prefix(self): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid paragraph reference"): ParagraphRef.parse("3#a7b2")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_paragraph_hash.py` around lines 45 - 73, Update the failing tests that call ParagraphRef.parse to assert the specific error text by adding a match parameter to pytest.raises(ValueError) for each test (test_reject_missing_hash, test_reject_uppercase_hash, test_reject_short_hash, test_reject_long_hash, test_reject_empty_string, test_reject_no_p_prefix); keep test_reject_zero_index as-is (it does not raise). Use the exact error message produced by ParagraphRef.parse (or a stable substring/regex) in each match so the tests fail only for the intended validation error (e.g., match patterns for missing hash, invalid-case hash, hash length too short/long, empty input, and missing 'P' prefix).
187-192: 🧹 Nitpick | 🔵 TrivialAdd return type annotation to helper function.
♻️ Suggested fix
- def _get_paragraph_ref(self, doc, index): + def _get_paragraph_ref(self, doc, index) -> str: """Helper to get a fresh paragraph reference for a given 1-based index."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_paragraph_hash.py` around lines 187 - 192, The helper function _get_paragraph_ref is missing a return type annotation; update its signature to declare the return type (str) so it reads like def _get_paragraph_ref(self, doc, index) -> str:, leaving the existing implementation that calls doc.list_paragraphs() and returns entry.split("|")[0] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/hash_anchored_vs_plain.py`:
- Around line 300-302: The catch-all "except Exception as e" that increments
hash_error and prints the error (using variables hash_error and old_ref) should
be narrowed: determine the specific exceptions you expect in this block (e.g.,
ValueError, KeyError, IOError, etc.) and replace the generic except with an
except tuple of those specific exception types to handle anticipated failures;
keep the existing increment and print for those expected exceptions, and for
truly unexpected errors either log them and re-raise (raise) or add a broad
except that re-raises after logging so unexpected issues are not silently
swallowed. Ensure the updated handler still references hash_error and old_ref as
before.
- Around line 267-270: The except block currently catches a bare "except
Exception" (around the code incrementing plain_error), which can hide unexpected
issues; update the handler for the specific expected failure (e.g., catch
IndexError/ValueError or whatever the benchmark's failure raises) or at minimum
change it to "except Exception as e" and log the exception type/message (use the
existing logger or print) before incrementing plain_error so unexpected errors
are visible; look for the try/except surrounding plain_error and replace the
bare except Exception with a specific exception class or add logging of "e" to
reveal the error details.
- Around line 87-90: Remove the dead call to list_paragraphs() on the doc
object: the call doc.list_paragraphs() returns a value that's discarded and
provides no side effects, so delete that line (after
build_multi_paragraph_doc(30) where doc is assigned) and keep the remaining
save_path = doc.save(), doc.close() flow; confirm no other code relies on
list_paragraphs() being invoked for side effects.
In `@tests/conftest.py`:
- Around line 11-16: The fallback in find_ref uses doc.list_paragraphs()[0]
which will raise IndexError for empty documents; update find_ref to first store
paragraphs = doc.list_paragraphs(), check if paragraphs is empty and either
raise a clear exception (e.g., ValueError("no paragraphs in document")) or
return a sentinel (e.g., None), otherwise proceed with the current search logic
(loop over paragraphs and return paragraph.split("|")[0] on match, or
paragraphs[0].split("|")[0] as the safe fallback).
---
Duplicate comments:
In `@tests/test_batch_edit.py`:
- Around line 166-175: Replace the explicit loop that builds ops with a list
comprehension to simplify and clarify creation of EditOperation objects:
construct ops = [EditOperation(action="replace", find=f"item {i+1}",
replace_with=f"BATCH_{i+1}", paragraph=refs[i].split("|")[0]) for i in
range(10)] so you still use EditOperation, refs, and the same string formatting
while removing the manual append loop.
- Around line 247-250: The current assertion uses "or" which can hide a failure
to remove "item 5" if "[P05]" is present; change the assertion to explicitly
verify deletion and marker presence by asserting that "item 5" is not in vis AND
"[P05]" is in vis (e.g., replace `assert "item 5" not in vis or "[P05]" in vis`
with two checks or a single combined check ensuring both conditions) so
get_visible_text() truly shows "item 5" was removed while the "[P05]" marker
remains.
In `@tests/test_paragraph_hash.py`:
- Around line 45-73: Update the failing tests that call ParagraphRef.parse to
assert the specific error text by adding a match parameter to
pytest.raises(ValueError) for each test (test_reject_missing_hash,
test_reject_uppercase_hash, test_reject_short_hash, test_reject_long_hash,
test_reject_empty_string, test_reject_no_p_prefix); keep test_reject_zero_index
as-is (it does not raise). Use the exact error message produced by
ParagraphRef.parse (or a stable substring/regex) in each match so the tests fail
only for the intended validation error (e.g., match patterns for missing hash,
invalid-case hash, hash length too short/long, empty input, and missing 'P'
prefix).
- Around line 187-192: The helper function _get_paragraph_ref is missing a
return type annotation; update its signature to declare the return type (str) so
it reads like def _get_paragraph_ref(self, doc, index) -> str:, leaving the
existing implementation that calls doc.list_paragraphs() and returns
entry.split("|")[0] unchanged.
🪄 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: 46eeb863-f6f6-4e10-866f-98a8789d0145
📒 Files selected for processing (6)
benchmarks/hash_anchored_vs_plain.pydocx_editor/track_changes.pydocx_editor/xml_editor.pytests/conftest.pytests/test_batch_edit.pytests/test_paragraph_hash.py
…tring - Benchmark called Document.replace() without required paragraph= arg (would crash at runtime). Plain path now uses _revision_manager directly. - Updated __init__.py module docstring to show hash-anchored API. - Fixed _insert_near_match type: str → Literal["before", "after"]. Skipped: add_comment lacks paragraph= (out of scope, separate PR). Skipped: RevisionManager paragraph= still optional (intentional layering).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
=======================================
+ Coverage 93.5% 93.7% +0.2%
=======================================
Files 10 10
Lines 1867 1993 +126
Branches 433 458 +25
=======================================
+ Hits 1746 1869 +123
+ Misses 33 31 -2
- Partials 88 93 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix ambiguous test assertion in test_batch_edit (item 5 deletion) - find_ref() now raises ValueError instead of silently falling back - Use valid paragraph ref format P1#0000 in closed-state tests - Fix undefined author variable in README batch-edit example - Update openspec proposals: non-breaking → breaking
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 `@benchmarks/hash_anchored_vs_plain.py`:
- Around line 245-266: The verifier currently scans the whole visible text after
calling doc1._revision_manager.replace_text("committee", "EDITED",
occurrence=old_occ) and picks the first "EDITED" it finds, which may be from
earlier edits; instead capture the state before the edit (vis_before =
doc1.get_visible_text()) or use the change ID returned by replace_text, then
after the edit get vis_after = doc1.get_visible_text() and locate only the new
insertion by diffing vis_before→vis_after or by filtering occurrences of
"EDITED" whose surrounding marker (e.g. "[P...]" substring) is present only in
vis_after and not in vis_before; then increment plain_correct/plain_wrong based
on that attributed landed_para (old_occ and marker remain the anchors).
In `@docx_editor/__init__.py`:
- Around line 11-14: The example currently uses hard-coded paragraph hashes for
tracked edits; instead use the refs variable returned by doc.list_paragraphs()
to look up paragraph identifiers dynamically and pass those into doc.replace,
doc.insert_after, and doc.delete. Update the snippet so refs =
doc.list_paragraphs() is shown being indexed (e.g., selecting refs by index or
key) and those values are used for the paragraph= parameter in the calls to
doc.replace, doc.insert_after, and doc.delete to demonstrate the real workflow.
In `@docx_editor/track_changes.py`:
- Around line 120-137: The batch apply currently only parses paragraph refs
(ParagraphRef.parse) and calls self._resolve_paragraph but leaves
locate/find/anchor resolution to _apply_single_edit, allowing earlier edits to
mutate the DOM and break later ones; fix by pre-resolving and binding each
operation to a concrete in-document target (e.g., resolved run/anchor index or
explicit occurrence position) before the apply loop: for each op in operations
call the same locate logic used by _apply_single_edit (find/anchor matching,
occurrence selection, and any TextNotFound checks) and store the resolved target
alongside original_idx in parsed, and either reject batches that contain
multiple ops for the same paragraph (or for the same resolved occurrence) with a
clear ValueError, or ensure bound targets are immutable so _apply_single_edit
uses the bound target instead of re-resolving; update _apply_single_edit to
accept a pre-resolved target if present.
🪄 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: a1a3bdd8-e74a-4b9d-8168-37b45b496a2d
📒 Files selected for processing (3)
benchmarks/hash_anchored_vs_plain.pydocx_editor/__init__.pydocx_editor/track_changes.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openspec/changes/add-batch-edit/proposal.md (1)
18-19:⚠️ Potential issue | 🟠 MajorImpact classification is still contradictory (
Non-breakingvs requiredparagraph).Line 18 should be marked as breaking for existing edit-method callers;
batch_edit()itself is additive.📝 Proposed fix
-- **Non-breaking**: New `batch_edit()` method; requires `paragraph` on each operation (consistent with breaking change in `add-paragraph-hash-anchors`) +- **Breaking**: Existing edit APIs require `paragraph` (`replace()`, `delete()`, `insert_after()`, `insert_before()`); callers must pass hash-anchored refs. +- **Non-breaking addition**: New `batch_edit()` method for atomic multi-edit application.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/add-batch-edit/proposal.md` around lines 18 - 19, The impact note is contradictory: mark the change as breaking because batch_edit() requires a `paragraph` field on each operation which will break existing edit-method callers; update the proposal text replacing "Non-breaking" with "Breaking" (while preserving that the API addition itself is additive) and keep the dependency list referencing `add-paragraph-hash-anchors` and symbols `ParagraphRef`, `HashMismatchError`, and `_resolve_paragraph` to show the linkage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 74-76: The example defines a variable named author but then
hardcodes "Editor" when opening the document; update the snippet so the
Document.open call uses the defined author variable (author=author) or remove
the unused author variable entirely for consistency; locate the example around
the Document.open("contract.docx", author="Editor") usage and update it (also
apply the same change to the other occurrence referenced at lines 105-107) so
the author value is not contradictory or unused.
In `@tests/conftest.py`:
- Around line 13-15: In find_ref(), the current loop checks "text in entry"
which can match the hash-ref prefix; instead split each entry once on the first
"|" to separate the prefix and preview (use entry.split("|", 1)), search only
the preview portion for text, and when matched return the prefix part (the
portion before the "|"); update the loop that iterates doc.list_paragraphs() and
refer to the variables entry and the find_ref() function when making this
change.
---
Duplicate comments:
In `@openspec/changes/add-batch-edit/proposal.md`:
- Around line 18-19: The impact note is contradictory: mark the change as
breaking because batch_edit() requires a `paragraph` field on each operation
which will break existing edit-method callers; update the proposal text
replacing "Non-breaking" with "Breaking" (while preserving that the API addition
itself is additive) and keep the dependency list referencing
`add-paragraph-hash-anchors` and symbols `ParagraphRef`, `HashMismatchError`,
and `_resolve_paragraph` to show the linkage.
🪄 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: c13435ae-607c-4734-9aef-297a754b84d8
📒 Files selected for processing (6)
README.mdopenspec/changes/add-batch-edit/proposal.mdopenspec/changes/add-paragraph-hash-anchors/proposal.mdtests/conftest.pytests/test_batch_edit.pytests/test_document.py
Tests for missing fields, text-not-found, and unknown action in batch edit operations. Raises patch coverage from 85.7% to ~97%.
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 `@tests/test_batch_edit.py`:
- Around line 251-252: Replace the list-comprehension-with-[0] pattern used to
extract p05_line with an idiomatic next(...) over a generator expression: use
next(line for line in vis.split("\n") if "[P05]" in line) to obtain the single
matching line (this keeps the same failure behavior if no match) and then assert
"item 5" not in p05_line; update the expression that assigns p05_line
(referencing the variables p05_line and vis) accordingly.
🪄 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: 3fdf5d0b-8ed8-4595-b360-a3e8d9b3f526
📒 Files selected for processing (1)
tests/test_batch_edit.py
pablospe
changed the title
pablospe
changed the title
4 tasks done
This was referenced Jul 17, 2026
This was referenced Jul 24, 2026
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
Adds hash-anchored paragraph references for safe LLM editing of Word documents. Paragraphs are identified by
P{index}#{hash}(e.g.,P3#a7b2) using CRC32 content hashes, preventing silent corruption when document content changes between operations.Changes
list_paragraphs(): Returns stable paragraph references with content hashes and text previewsreplace,delete,insert_after,insert_before) now requireparagraph=keyword argument for scoped, safe operationsHashMismatchError: Raised when paragraph content changed since last snapshot, includes current hash for retrybatch_edit(): Atomic all-or-nothing validation with reverse-order application — onelist_paragraphs()snapshot suffices for an entire batchparagraphparameter is now required on all Document edit methodsHow it works
Benchmark results (30-paragraph document)
Test plan
ruff check,ruff format,ty check,pre-commitall greenbenchmarks/