Repository navigation
feat: find_all enumeration, occurrence ambiguity guard, batch operation_index (ISSUES.md #34) - #53
Merged
Conversation
…on_index (ISSUES.md #34) - Document.find_all(text, paragraph=None) enumerates every match as SearchResults whose paragraph_ref/paragraph_occurrence plug directly into follow-up edits. - Omitted occurrence now means the target must be unique in the search scope; ambiguous targets raise AmbiguousTextError (new exception with search_text, paragraph_ref, paragraph_preview, total_occurrences) instead of silently editing the first match. Explicit occurrence=0 keeps the old behavior per call. - Out-of-range occurrences in scoped edits report occurrence and total_occurrences instead of a self-contradicting 'not found'. - batch_edit/batch_rewrite raise only BatchOperationError for per-op failures (stale hash included), with operation_index and the typed underlying exception on original/__cause__. Batches stay atomic. - Occurrence handling consolidated into _locate_in_paragraph, replacing four duplicated find-or-raise paths; dry-run validation shares it. - Docs (SKILL.md, api.md) and benchmarks updated; 14 new find_all tests plus ambiguity, out-of-range, and single-exception-contract suites.
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds ChangesOccurrence and error contracts
Search and edit resolution
Atomic batch failures
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Document
participant RevisionManager
Caller->>Document: find_all(text, paragraph)
Document->>RevisionManager: enumerate matches
RevisionManager-->>Document: SearchResult list
Document-->>Caller: ordered matches
Caller->>Document: replace using paragraph_ref and occurrence
Document->>RevisionManager: apply targeted edit
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docx_editor/comments.py (1)
183-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated occurrence-resolution logic between
CommentManagerandRevisionManager.The docstring itself admits this "Mirrors
RevisionManager._locate_in_paragraph/_find_document_wide" instead of sharing code with it, and Ruff flags the resulting complexity (PLR0912: Too many branches (16 > 12)). The ambiguity/out-of-range decision logic (count total → locate match → raiseAmbiguousTextError/TextNotFoundError/return) is inlined twice here (paragraph-scoped and document-wide) and a third time inRevisionManager. Any future tweak to this contract (e.g. the message wording, or how totals are computed) has to be kept in sync manually across two files.Consider extracting a shared helper (e.g.
resolve_occurrence(text_map_or_paragraphs, anchor_text, occurrence)returning the match + total, or raising the appropriate error) that bothCommentManager._find_anchorandRevisionManagercall, reducing branch count and duplication risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docx_editor/comments.py` around lines 183 - 268, The occurrence resolution in CommentManager._find_anchor duplicates RevisionManager logic and exceeds the branch limit. Extract the shared counting, matching, ambiguity, out-of-range, and not-found behavior into a reusable helper, then update both _find_anchor and the corresponding RevisionManager methods to call it while preserving paragraph/document-wide context and existing error details.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docx_editor/comments.py`:
- Line 194: Validate that occurrence is non-negative before the anchor lookup
through find_in_text_map, raising ValueError for negative values. Apply the same
guard used by the edit operation, before assigning the default occurrence or
starting the search.
In `@docx_editor/track_changes.py`:
- Around line 912-932: Update _find_document_wide so occurrence=None checks only
whether a second match exists using a bounded lookup rather than calling
count_matches(text) unconditionally. Preserve the unique-match fast path, and
call count_matches only when the second occurrence is found to provide
total_occurrences for AmbiguousTextError; keep the existing not-found behavior
unchanged.
In `@tests/test_error_quality.py`:
- Around line 256-264: Add a return type annotation to the `_edit_calls` helper,
matching the annotation style used by `_build_doc_with_paragraphs`,
`_build_batch_doc`, and `_build_doc`; preserve its existing list of edit-call
lambdas.
---
Outside diff comments:
In `@docx_editor/comments.py`:
- Around line 183-268: The occurrence resolution in CommentManager._find_anchor
duplicates RevisionManager logic and exceeds the branch limit. Extract the
shared counting, matching, ambiguity, out-of-range, and not-found behavior into
a reusable helper, then update both _find_anchor and the corresponding
RevisionManager methods to call it while preserving paragraph/document-wide
context and existing error details.
🪄 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: 079573be-57e6-49ca-b70a-7fee90982032
📒 Files selected for processing (13)
benchmarks/hash_anchored_vs_plain.pydocs/api.mddocx_editor/__init__.pydocx_editor/comments.pydocx_editor/document.pydocx_editor/exceptions.pydocx_editor/track_changes.pyskills/docx/SKILL.mdtests/test_batch_edit.pytests/test_error_quality.pytests/test_find_all.pytests/test_rewrite_paragraph.pytests/test_run_child_order.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
=====================================
Coverage 95.3% 95.3%
=====================================
Files 11 11
Lines 3356 3445 +89
Branches 689 698 +9
=====================================
+ Hits 3200 3285 +85
- Misses 73 76 +3
- Partials 83 84 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Multi-reviewer findings on PR #53: - The find_all docs recommended batch_edit for several matches in one paragraph, but ops in find_all (ascending) order silently mis-target or fail: each edit shifts the occurrence numbering of the matches after it in the accepted text view. Corrected all three doc spots (Document.find_all docstring, docs/api.md, SKILL.md) to require descending occurrence order (an edit never shifts the matches before it) with the self-overlapping-string caveat, and added TestSameParagraphBatch locking in both the working descending pattern and the atomic failure of the ascending one. - Consolidated the two match-counting idioms into one count_in_text_map() helper in xml_editor.py, now used by _locate_in_paragraph, count_matches, and CommentManager._find_anchor (also drops count_matches' quadratic inner loop). - Dropped the one-off 'Note:' docstring heading in Document.find_all. Skipped (reviewed, deliberate): the extra count_matches pass on the document-wide occurrence=None success path — short-circuiting at the second match would break the exact total_occurrences the AmbiguousTextError contract promises.
Round-2 multi-reviewer findings on PR #53: - batch_rewrite now snapshots the DOM and rolls back on mid-apply failure, matching batch_edit — the documented 'nothing is applied on failure' contract previously rested entirely on upfront validation precluding every apply-phase error. Added a rollback test that simulates a mid-loop failure (none is reachable today). - Renamed _find_document_wide to _locate_document_wide: the _find_* helpers return None on miss, while this one raises like _locate_in_paragraph — the name now signals the find-or-raise contract. - TextNotFoundError docstring now covers the out-of-range occurrence case that docs/api.md already documented. - Factored the 'Current content' message suffix into _append_preview so TextNotFoundError and AmbiguousTextError build it identically instead of one being period-safe by coincidence. - Documented why _locate_document_wide counts via count_matches rather than count_in_text_map (text maps are per-paragraph; exact total is part of the error contract). Rejected finding: a claim that _apply_single_edit re-validates hashes mid-batch, breaking same-paragraph descending-order batches — refuted by the code (apply resolves by index only), the passing TestSameParagraphBatch, and an end-to-end repro.
Round-3 multi-reviewer findings on PR #53 (correctness reviewer CLEAN): - Renamed CommentManager._find_anchor to _locate_anchor, completing the find-or-raise naming convention round 2 introduced (_find_* helpers return None on miss; this one always raises). - The find_all examples in the docstring, api.md, and SKILL.md now show the pattern that is safe for any match distribution — one batch_edit over reversed(find_all(...)), which yields the required descending occurrence order for same-paragraph ops — instead of the sequential loop that only works with one match per paragraph. - api.md and SKILL.md now note BatchOperationError.original is None for batch-level rule violations (missing paragraph ref, duplicate paragraph) that have no underlying exception.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docx_editor/track_changes.py`:
- Around line 455-456: Reject negative occurrence values before anchor lookup by
adding the same ValueError guard used in _resolve_action_target to
_locate_in_paragraph in docx_editor/track_changes.py at lines 455-456,
_locate_document_wide in docx_editor/track_changes.py at lines 933-934, and
_locate_anchor in docx_editor/comments.py at lines 195-196; perform the check
before resolving the fallback occurrence value.
🪄 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: 4bf49b29-f3d5-4821-ba86-3b6ce7b067b3
📒 Files selected for processing (9)
docs/api.mddocx_editor/comments.pydocx_editor/document.pydocx_editor/exceptions.pydocx_editor/track_changes.pydocx_editor/xml_editor.pyskills/docx/SKILL.mdtests/test_batch_edit.pytests/test_rewrite_paragraph.py
Comment on lines
+455
to
+456
| occ = occurrence if occurrence is not None else 0 | ||
| match = find_in_text_map(text_map, text, occ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject negative occurrence before anchor lookup.
All three shared locator methods allow a negative occurrence to proceed to the search phase. For paragraph-scoped searches, occurrence < 0 skips the search loop in find_in_text_map and crashes with an UnboundLocalError. For document-wide searches, it scans the entire document and misreports a TextNotFoundError.
Add a fail-fast guard at the start of these methods to mirror the validation already present in _resolve_action_target.
docx_editor/track_changes.py#L455-L456: guard againstoccurrence < 0(raisingValueError) before resolvingoccin_locate_in_paragraph.docx_editor/track_changes.py#L933-L934: guard againstoccurrence < 0before resolvingoccin_locate_document_wide.docx_editor/comments.py#L195-L196: guard againstoccurrence < 0before resolvingoccin_locate_anchor.
📍 Affects 2 files
docx_editor/track_changes.py#L455-L456(this comment)docx_editor/track_changes.py#L933-L934docx_editor/comments.py#L195-L196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docx_editor/track_changes.py` around lines 455 - 456, Reject negative
occurrence values before anchor lookup by adding the same ValueError guard used
in _resolve_action_target to _locate_in_paragraph in
docx_editor/track_changes.py at lines 455-456, _locate_document_wide in
docx_editor/track_changes.py at lines 933-934, and _locate_anchor in
docx_editor/comments.py at lines 195-196; perform the check before resolving the
fallback occurrence value.
CodeRabbit findings on PR #53: - A negative occurrence passed to the direct (non-batch) edit methods reached find_in_text_map, where range(occurrence + 1) never runs and the scoped paths crashed with UnboundLocalError (document-wide paths produced a nonsensical 'occurrence=-1 requested' message). All three locate paths (_locate_in_paragraph, _locate_document_wide, CommentManager._locate_anchor) now reject negatives up front with the same ValueError the EditOperation constructors raise; regression test covers all four public paths. - Added the missing return annotation on the _edit_calls test helper. Skipped (deliberate, settled in earlier rounds): the second count_matches scan on the document-wide occurrence=None path — the exact total is part of the AmbiguousTextError contract and the trade-off is documented in _locate_document_wide's docstring.
pablospe
added a commit
that referenced
this pull request
Jul 22, 2026
…ES.md #53) One changeset (a single edit call, or one whole batch_edit/batch_rewrite call) stamps one whole-second w:date, bumped past the previous stamp when the same author's changesets collide in one real second. Parse-time group reconstruction (#46) therefore never merges two of our own changesets; same-paragraph ops of one batch call share their date and merge by design. - xml_editor: _next_changeset_timestamp() allocates collision-bumped stamps (monotonic per author, also across clock regressions); frozen_timestamp() becomes reentrant-by-reuse so nested per-op freezes join the enclosing batch changeset. - track_changes: batch_edit/batch_rewrite wrap the whole call in one frozen_timestamp() scope — one date per batch call, one group per op. - tests: TestChangesetDateStamping pins the contract (shared batch date under a ticking clock, no bump when seconds differ, bounded drift, distinct dates for two batches in one second, nested freeze reuse, w16du:dateUtc parity); the former same-second over-merge relaxation flips into test_same_paragraph_same_second_two_groups.
pablospe
added a commit
that referenced
this pull request
Jul 22, 2026
…ES.md #53) (#65) * feat: collision-bumped w:date stamping at changeset granularity (ISSUES.md #53) One changeset (a single edit call, or one whole batch_edit/batch_rewrite call) stamps one whole-second w:date, bumped past the previous stamp when the same author's changesets collide in one real second. Parse-time group reconstruction (#46) therefore never merges two of our own changesets; same-paragraph ops of one batch call share their date and merge by design. - xml_editor: _next_changeset_timestamp() allocates collision-bumped stamps (monotonic per author, also across clock regressions); frozen_timestamp() becomes reentrant-by-reuse so nested per-op freezes join the enclosing batch changeset. - track_changes: batch_edit/batch_rewrite wrap the whole call in one frozen_timestamp() scope — one date per batch call, one group per op. - tests: TestChangesetDateStamping pins the contract (shared batch date under a ticking clock, no bump when seconds differ, bounded drift, distinct dates for two batches in one second, nested freeze reuse, w16du:dateUtc parity); the former same-second over-merge relaxation flips into test_same_paragraph_same_second_two_groups. * docs+style: scope the never-share-a-second guarantee to one open session Multi-review round 1: the collision counter is per-editor-instance and not seeded from persisted dates, so the docs (api.md, SKILL.md, Revision.group_source and RevisionManager docstrings) now state the guarantee as per-session — own writes from a previous session merge like foreign revisions on identical author + date. Also rename _next_changeset_timestamp to _get_next_changeset_timestamp to match the sibling allocators (_get_next_change_id, _get_next_comment_id) and rewrap the api.md paragraph left with an orphan line. Skipped per red-team triage: stamp-slot consumption by empty/failed changesets (accepted bounded drift, in plan), per-author stamp dict (author is immutable per editor — false positive), test docstring issue-reference form (subjective). * docs+test: round-2 review polish — session qualifier, helper reuse, regression test - frozen_timestamp docstring and TestChangesetDateStamping class docstring now carry the same per-session qualifier as every other statement of the never-share-a-second guarantee (round 1 missed these two spots). - test_nested_frozen_timestamp_reuses_outer_stamp builds its editor via the file's _make_manager helper instead of a raw DocxXMLEditor construction. - New test_clock_regression_keeps_dates_monotonic drives the settable clock strictly backwards, pinning the documented NTP-step-back behavior (dates never regress; previous+1 until the clock catches up) — previously only the equality branch was exercised. * fix: dedupe SKILL.md lines duplicated in rebase conflict resolution
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.
Implements ISSUES.md item #34 (repo-local tracker, not a GitHub issue number).
What
Document.find_all(text, paragraph=None)— enumerate every match in document order asSearchResults. Each result'sparagraph_ref/paragraph_occurrenceplug directly into a follow-up edit'sparagraph=/occurrence=, replacing N+1find_textprobes. No-match returns[](enumeration, not lookup). Also available onRevisionManager.occurrenceis nowint | None = Noneacrossreplace,delete,insert_after,insert_before,add_comment, and theEditOperationconstructors. An omitted occurrence means the target must be unique in the search scope; ambiguous targets raise the newAmbiguousTextError(search_text,paragraph_ref,paragraph_preview,total_occurrences) naming both recovery paths. Deliberately not aTextNotFoundErrorsubclass — the text was found.occurrence/total_occurrences("Only 2 occurrence(s)… occurrence=9 requested") instead of a self-contradicting "not found" whose preview visibly contains the text. Dry-run (batch_edit(dry_run=True)) reports the same messages.batch_edit/batch_rewriteraise onlyBatchOperationErrorfor per-op failures in both phases (validation and apply, stale hashes included), carryingoperation_indexand the typed underlying exception onoriginal/__cause__(e.g.e.original.actual_hashre-targets a stale ref withoutlist_paragraphs()). Batches remain atomic._locate_in_paragraphhelper (replaces four duplicated find-or-raise paths); dry-run validation shares it, so messages cannot drift.occurrencenow raiseAmbiguousTextErrorinstead of silently editing the first match. Per-call opt-out: passoccurrence=0explicitly.BatchOperationError— code catchingHashMismatchError/TextNotFoundErroraroundbatch_edit/batch_rewritemust switch (the typed exception is onoriginal/__cause__).except DocxEditErrorcallers are unaffected.Testing
uv run pytest: 1018 passed, 2 skipped (14 new find_all tests; new ambiguity, out-of-range, and single-exception-contract suites; rollback atomicity re-verified under the new contract)uv run ruff check ./uv run ruff format --check .: cleanuv run ty check: 6 diagnostics, identical to the clean-tree baseline (pre-existing, unrelated test files)benchmarks/hash_anchored_vs_plain.pyruns end-to-end with expected resultse.original.actual_hashSummary by CodeRabbit
New Features
find_all()to enumerate all text matches with paragraph references and per-match occurrence details.occurrence=Noneto require unique targets;AmbiguousTextErroris publicly exposed.Bug Fixes
BatchOperationError, exposing the failing operation index and the original underlying error.Documentation
find_all(), newoccurrencerules, and batch error/rollback behavior.