Repository navigation
feat: changeset tier — accept/reject_changeset over whole calls (ISSUES.md #54) - #68
Merged
Merged
Conversation
…ES.md #54) Third revision-tracking tier, one level above groups: one call (a single edit, or a whole batch_edit/batch_rewrite) = one changeset ⊇ ≥1 group ⊇ revisions A changeset is the (author, date) equivalence class over groups — a global class, not a contiguous run, so a batch_edit spanning several paragraphs is one changeset over non-contiguous groups. Recorded live via a reentrant _changeset() boundary (nested rewrite_paragraph inside batch_rewrite merges into one changeset); reconstructed as inferred changesets at parse time. Registry snapshot/restore extended so a failed batch never leaves a ghost changeset. Adds Document.accept_changeset/reject_changeset, changeset_id on EditResult, changeset_id/changeset_source on Revision, and changeset_id on RevisionError. 14 new tests + updated foreign-revision fixtures; docs updated.
|
Warning Review limit reached
Next review available in: 33 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 (2)
📝 WalkthroughWalkthroughAdds a changeset tier above revision groups, tracking revisions created by whole edit calls. It introduces changeset metadata, accept/reject APIs, inferred reconstruction, rollback-safe registries, updated error contracts, documentation, and comprehensive tests. ChangesChangeset tier
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Document
participant RevisionManager
participant TrackedDocument
Document->>RevisionManager: perform batch_edit inside _changeset()
RevisionManager->>TrackedDocument: create revisions across groups
RevisionManager-->>Document: return EditResult with changeset_id
Document->>RevisionManager: reject_changeset(changeset_id)
RevisionManager->>TrackedDocument: resolve revisions in all member groups
TrackedDocument-->>Document: return rejected revision count
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 `@docs/api.md`:
- Around line 899-900: Update the changeset documentation links at
docs/api.md:899-900 and docs/api.md:985-985 to point directly to the
accept_changeset changeset documentation section, replacing fragile combined
accept_changeset/reject_changeset fragments while preserving the existing link
text and surrounding descriptions.
🪄 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: ef38a382-f724-4931-9004-4ebe9d210150
📒 Files selected for processing (6)
docs/api.mddocx_editor/document.pydocx_editor/exceptions.pydocx_editor/track_changes.pytests/test_foreign_revisions.pytests/test_revision_groups.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
=====================================
Coverage 95.8% 95.9%
=====================================
Files 11 11
Lines 3984 4057 +73
Branches 798 803 +5
=====================================
+ Hits 3818 3891 +73
Misses 77 77
Partials 89 89 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…type nits Multi-reviewer pass (3 Claude subagents + codex + red-team meta-review): correctness CLEAN, zero blockers. Applied the confirmed LOW nits: - Remove write-only _ChangesetCapture; _changeset() now yields None. The bundled id is always read via changeset_id_of() (through _edit_result / list_revisions), never off the context manager, so the capture was dead surface (YAGNI). - Tighten _group_changesets value type to dict[int, int] (None is never stored, unlike the sibling _revision_groups). - changeset_groups() docstring now documents member ordering, mirroring group_revisions(). - docs/api.md: name changeset_id in the per-method EditResult 'Returns' blurbs; split accept_changeset/reject_changeset into two headers matching every other accept/reject pair; fix the EditResult/Revision table anchor links. Skipped as false positives / no value: a cs '(inferred)' repr marker (a group and its changeset always share provenance, so it would only duplicate the group marker), a ChangesetSource alias (identical to GroupSource — DRY), an EditResult.__new__ param reorder (str subclass, not a dataclass — cosmetic), and the always-true _group_changesets guard (cheap belt-and-braces).
This was referenced Jul 23, 2026
pablospe
added a commit
that referenced
this pull request
Aug 28, 2026
The rationale for CHANGE_RECORD_TAGS claimed only w:tcPrChange's recorded subtree can hold revision marks. Two more can: a paragraph-mark w:rPrChange records CT_ParaRPrOriginal, which opens with EG_ParaRPrTrackChanges, and w:pPrChange records CT_PPrBase, whose w:numPr admits w:ins/w:numberingChange. Only the pending-revision path skips those subtrees; list_revisions and accept_all still walk every w:ins/w:del, so a historical w:del recorded in a change record is listed and resolved like a live one. That is pre-existing, and is now pinned by an xfail(strict=True) test so ISSUES.md #68 inherits it rather than rediscovering it. Also: guard CHANGE_RECORD_TAGS against drifting out of the property-change family; group w:trPrChange with the property changes in the warning docstring; stop labelling every w:rPr context as a paragraph mark, since a change record's recorded rPr shares the tag; report files whose census never ran instead of counting them as revision-free; build the location context only when list_unhandled_revisions has rows; name the author scope in the warning.
pablospe
added a commit
that referenced
this pull request
Aug 28, 2026
…#76) * feat: report revision types accept_all cannot resolve (ISSUES.md #64) accept_all/reject_all walk w:ins/w:del only, so a redline whose revisions are format changes or moves returned a bare 0 and read as "there was nothing to accept". They now return a ResolveResult (an int subclass carrying .unhandled/.unhandled_types), warn with UnhandledRevisionWarning when adjudication is incomplete, and list_unhandled_revisions() names the marks that are still pending. Adds count_revision_elements()/RevisionCensus and a corpus-harness --census mode recording which revision types real producers emit. * fix: don't count marks recorded inside a change record as pending A w:tcPrChange records the cell's previous w:tcPr, which is CT_TcPrInner and so may legally hold a w:cellIns/w:cellDel/w:cellMerge. Those describe state that is already gone, but the unhandled scan descended into the record and reported them as extra pending revisions. iter_revision_elements now takes skip_change_records, which _unhandled_elements uses; count_revision_elements stays a raw inventory and documents the difference. Also: extract _parse_w_date instead of a third copy of the w:date parse; correct the "excluded from every filtered call" author rule, which the author="Unknown" test contradicted; move the SKILL.md honesty check next to the call it inspects, since it read a result four calls stale; drop the incorrect ECMA sub-numbers; label the census error count by part rather than file and stop truncating it silently. * fix: correct the change-record schema note and pin the handled-path gap The rationale for CHANGE_RECORD_TAGS claimed only w:tcPrChange's recorded subtree can hold revision marks. Two more can: a paragraph-mark w:rPrChange records CT_ParaRPrOriginal, which opens with EG_ParaRPrTrackChanges, and w:pPrChange records CT_PPrBase, whose w:numPr admits w:ins/w:numberingChange. Only the pending-revision path skips those subtrees; list_revisions and accept_all still walk every w:ins/w:del, so a historical w:del recorded in a change record is listed and resolved like a live one. That is pre-existing, and is now pinned by an xfail(strict=True) test so ISSUES.md #68 inherits it rather than rediscovering it. Also: guard CHANGE_RECORD_TAGS against drifting out of the property-change family; group w:trPrChange with the property changes in the warning docstring; stop labelling every w:rPr context as a paragraph mark, since a change record's recorded rPr shares the tag; report files whose census never ran instead of counting them as revision-free; build the location context only when list_unhandled_revisions has rows; name the author scope in the warning.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…ES.md #68) w:moveFrom, w:moveTo and w:pPrChange become first-class, id-addressable revisions: list_revisions() lists them as move_from / move_to / property_change rows, accept_revision / reject_revision resolve them, and accept_all / reject_all, the author filter and the inferred changeset cover them through the existing _resolve_all loop. Per-element semantics mirror Word's revision pane. A move's four range marks are scaffolding: never listed, never counted as unhandled, swept by _sweep_move_range_marks once no pending move content lies between them. Rejecting a w:pPrChange restores the recorded properties (_restore_paragraph_properties), including a self-closing record and an undefined style id. The accepted text-map view now excludes w:t inside w:moveFrom, which Word writes for moved-away text. Listing and the id index use one iter_revision_elements walk over HANDLED_REVISION_TAGS instead of a getElementsByTagName per tag.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…SSUES.md #68) After accept_all the saved file may hold only UNHANDLED_REVISION_TAGS; any ins/del/move/pPrChange element left behind fails save2 with AssertResolvedTypesRemain. The reopen record carries the unhandled census, the census marker keys on UNHANDLED_REVISION_TAGS, and the README census is re-run with 0 unresolved elements.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…#68) Revision.type's five values, per-type accept/reject semantics, the two-rows-per-move rule and lone-half behaviour, and ResolveResult / UnhandledRevision examples switched to types that are still unhandled.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
….md #68) The high-water mark folded only w:ins/w:del ids, while moves and paragraph-property changes are now addressed by w:id in the same space. On a document holding only a move, a new edit was allocated id 0 — the foreign w:moveFrom marker's id — so both reported one group and reject_group on our own edit removed the foreign move content instead. DocxXMLEditor now folds every ALL_REVISION_TAGS id in one walk, at open and on fragment parse. Also strips move paragraph-mark markers from a split paragraph's copied w:pPr/w:rPr (a copied marker duplicated its id) and hoists comment markers above w:moveFrom/w:moveTo wrappers as it does above w:ins/w:del, via one _RUN_TRACK_CHANGE_TAGS constant.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
… call (ISSUES.md #68) A w:moveFrom/w:moveTo/w:pPrChange with a missing or non-numeric w:id is unreachable by id, so list_revisions() omits it — and since its tag left UNHANDLED_REVISION_TAGS it vanished from list_unhandled_revisions() too, letting accept_all() report a clean document with the mark (and a moveFrom's hidden text) still in it. _unhandled_elements now also yields handled-type marks with no adjudicable id, via one shared _adjudicable_id. _resolve_all/_resolve_ids defer the move range-mark sweep to a single walk at the end of the call instead of one per move half (quadratic in the number of moves). _restore_deletion leaves a w:del nested inside the host it restores untouched (its delText and rsidDel stay), for both w:del and w:moveFrom hosts. The sweep treats id-less range marks and a Start whose id a later Start reuses as unpaired. group_spans skips every paragraph-mark marker, not only w:ins. Docstrings for UnhandledRevisionWarning, get_visible_text, get_original_text, list_unhandled_revisions and _insertion_text_nodes describe the new contract; the harness save2 post-condition gets a unit test.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…nested ids (ISSUES.md #68) Move content can leave the document inside another resolved element — a w:del wrapping a w:moveFrom, a w:ins wrapping a w:moveTo, a rejoined paragraph whose mark carried a move marker — and the range marks around it were then orphaned in neither listing. accept_revision/reject_revision now sweep after every resolution (one deferred walk per bulk call). _nearest_revision_ancestor_id and _descendant_revision_ids go through _adjudicable_id, so a non-numeric w:id on a host or nested handled mark no longer raises from list_revisions(); the mark is reported as unhandled. The ISSUES.md #56/#62 walk-count pins gained count_revision_walks, a counter for full-document iter_revision_elements traversals, so they pin a real constant again now that the resolution path no longer uses getElementsByTagName. An own edit made inside a foreign w:moveTo is carried away when that move is rejected — documented as a known gap and pinned with a strict xfail; docs no longer say list_unhandled_revisions() lists exactly UNHANDLED_REVISION_TAGS.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
… (ISSUES.md #68) DocxXMLEditor.holds_move_range_marks is set by the id-seeding walk when it sees a MOVE_RANGE_TAGS element and cleared by the sweep once none remain, so the sweep — a full-document walk — costs nothing on the move-free document every accept_revision/accept_group used to pay it on. Also: _restore_deletion keeps w:rsidDel on runs inside a nested w:moveFrom; the corpus save2 post-condition is measured against accept_all's own unhandled report rather than a static tag set, so a handled-type mark the library reported as unreachable does not fail the file while an unreported leftover (range marks included) still does; the README no longer claims that accepting the drag-and-drop table move is exact (the source table survives as empty cells); quickstart, SKILL.md and the accept_all docstring describe the own-edit-inside-a-foreign-move gap, the moved-table residue and a pPrChange carried away by a rejected split; _MARKUP_KIND_BY_TAG follows its siblings' naming.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…S.md #68) A rolled-back batch replaces the DOM wholesale; holds_move_range_marks now follows it (one walk on the rollback path only). Pins the corpus save2 rule that range marks may stay while their move is reported as unhandled.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
* feat: resolve moves and paragraph-property changes as revisions (ISSUES.md #68) w:moveFrom, w:moveTo and w:pPrChange become first-class, id-addressable revisions: list_revisions() lists them as move_from / move_to / property_change rows, accept_revision / reject_revision resolve them, and accept_all / reject_all, the author filter and the inferred changeset cover them through the existing _resolve_all loop. Per-element semantics mirror Word's revision pane. A move's four range marks are scaffolding: never listed, never counted as unhandled, swept by _sweep_move_range_marks once no pending move content lies between them. Rejecting a w:pPrChange restores the recorded properties (_restore_paragraph_properties), including a self-closing record and an undefined style id. The accepted text-map view now excludes w:t inside w:moveFrom, which Word writes for moved-away text. Listing and the id index use one iter_revision_elements walk over HANDLED_REVISION_TAGS instead of a getElementsByTagName per tag. * test: fail the corpus save2 stage when resolvable revisions remain (ISSUES.md #68) After accept_all the saved file may hold only UNHANDLED_REVISION_TAGS; any ins/del/move/pPrChange element left behind fails save2 with AssertResolvedTypesRemain. The reopen record carries the unhandled census, the census marker keys on UNHANDLED_REVISION_TAGS, and the README census is re-run with 0 unresolved elements. * docs: document moves and pPrChange as listed revision types (ISSUES.md #68) Revision.type's five values, per-type accept/reject semantics, the two-rows-per-move rule and lone-half behaviour, and ResolveResult / UnhandledRevision examples switched to types that are still unhandled. * fix: seed the change-id counter from every revision mark's id (ISSUES.md #68) The high-water mark folded only w:ins/w:del ids, while moves and paragraph-property changes are now addressed by w:id in the same space. On a document holding only a move, a new edit was allocated id 0 — the foreign w:moveFrom marker's id — so both reported one group and reject_group on our own edit removed the foreign move content instead. DocxXMLEditor now folds every ALL_REVISION_TAGS id in one walk, at open and on fragment parse. Also strips move paragraph-mark markers from a split paragraph's copied w:pPr/w:rPr (a copied marker duplicated its id) and hoists comment markers above w:moveFrom/w:moveTo wrappers as it does above w:ins/w:del, via one _RUN_TRACK_CHANGE_TAGS constant. * fix: report id-less handled marks and sweep range marks once per bulk call (ISSUES.md #68) A w:moveFrom/w:moveTo/w:pPrChange with a missing or non-numeric w:id is unreachable by id, so list_revisions() omits it — and since its tag left UNHANDLED_REVISION_TAGS it vanished from list_unhandled_revisions() too, letting accept_all() report a clean document with the mark (and a moveFrom's hidden text) still in it. _unhandled_elements now also yields handled-type marks with no adjudicable id, via one shared _adjudicable_id. _resolve_all/_resolve_ids defer the move range-mark sweep to a single walk at the end of the call instead of one per move half (quadratic in the number of moves). _restore_deletion leaves a w:del nested inside the host it restores untouched (its delText and rsidDel stay), for both w:del and w:moveFrom hosts. The sweep treats id-less range marks and a Start whose id a later Start reuses as unpaired. group_spans skips every paragraph-mark marker, not only w:ins. Docstrings for UnhandledRevisionWarning, get_visible_text, get_original_text, list_unhandled_revisions and _insertion_text_nodes describe the new contract; the harness save2 post-condition gets a unit test. * fix: sweep range marks after any resolution and tolerate non-numeric nested ids (ISSUES.md #68) Move content can leave the document inside another resolved element — a w:del wrapping a w:moveFrom, a w:ins wrapping a w:moveTo, a rejoined paragraph whose mark carried a move marker — and the range marks around it were then orphaned in neither listing. accept_revision/reject_revision now sweep after every resolution (one deferred walk per bulk call). _nearest_revision_ancestor_id and _descendant_revision_ids go through _adjudicable_id, so a non-numeric w:id on a host or nested handled mark no longer raises from list_revisions(); the mark is reported as unhandled. The ISSUES.md #56/#62 walk-count pins gained count_revision_walks, a counter for full-document iter_revision_elements traversals, so they pin a real constant again now that the resolution path no longer uses getElementsByTagName. An own edit made inside a foreign w:moveTo is carried away when that move is rejected — documented as a known gap and pinned with a strict xfail; docs no longer say list_unhandled_revisions() lists exactly UNHANDLED_REVISION_TAGS. * perf: skip the range-mark sweep on documents without move range marks (ISSUES.md #68) DocxXMLEditor.holds_move_range_marks is set by the id-seeding walk when it sees a MOVE_RANGE_TAGS element and cleared by the sweep once none remain, so the sweep — a full-document walk — costs nothing on the move-free document every accept_revision/accept_group used to pay it on. Also: _restore_deletion keeps w:rsidDel on runs inside a nested w:moveFrom; the corpus save2 post-condition is measured against accept_all's own unhandled report rather than a static tag set, so a handled-type mark the library reported as unreachable does not fail the file while an unreported leftover (range marks included) still does; the README no longer claims that accepting the drag-and-drop table move is exact (the source table survives as empty cells); quickstart, SKILL.md and the accept_all docstring describe the own-edit-inside-a-foreign-move gap, the moved-table residue and a pPrChange carried away by a rejected split; _MARKUP_KIND_BY_TAG follows its siblings' naming. * fix: recompute the range-mark flag when a snapshot is restored (ISSUES.md #68) A rolled-back batch replaces the DOM wholesale; holds_move_range_marks now follows it (one walk on the rollback path only). Pins the corpus save2 rule that range marks may stay while their move is reported as unhandled.
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 the changeset tier (ISSUES.md #54, v0.7.0) — the third
revision-tracking tier, one level above revision groups:
A changeset is the
(author, date)equivalence class over groups — aglobal class, not a contiguous run — so a
batch_editwhose ops land indifferent paragraphs is one changeset even though its groups are non-contiguous.
What's new
Document.accept_changeset(id)/reject_changeset(id)— resolve everygroup a whole call created, as one unit. Rump-tolerant.
EditResult.changeset_id— carried by every result a call returns(shared across a batch);
Noneiffgroup_idisNone.Revision.changeset_id/changeset_source— stamped bylist_revisions();"recorded"for this session's calls,"inferred"forrevisions reconstructed at parse time.
RevisionError.changeset_id— structured field for unknown-changeset errors.Mechanics
_changeset()boundary that mirrorsfrozen_timestamp: only the outermost call bundles, so a nestedrewrite_paragraphinsidebatch_rewritemerges into one changeset while astandalone call gets its own.
identical
w:author+w:date.batch_edit/batch_rewritenever leaves a ghost changeset.EditResultconstruction routes through the single_edit_resultfactory; the
_resolve_groupinternals were extracted into a shared_resolve_idsreused by group and changeset resolution.Tests & docs
TestChangesetTiertests (single edit, non-contiguous batch,batch_rewrite reentrancy, standalone separation, unknown-id error, rollback
registry restore, save/reopen round-trip, same-paragraph over-merge contract,
rump changesets, partial resolution, author/date partitioning, ungrouped,
foreign) + updated
test_foreign_revisionsfixtures.docs/api.mdupdated for the new methods and fields.Verification
ruff check/ruff format --check: clean ·ty check: exit 0Summary by CodeRabbit
New Features
Documentation