Repository navigation
fix: don't resolve another author's same-id revision (ROADMAP.md #75) - #96
Merged
Merged
Conversation
… elements (ROADMAP.md #75) accept_all(author=A) and reject_all(author=A) selected revisions by author but resolved them by w:id against an index holding every author's elements. Where a producer repeats a w:id across authors (LibreOffice does, and the corpus has such files), the id lookup returned the other author's element: reject_all(author="A") deleted B's inserted text and accept_all(author="A") made it permanent, both reporting 2. _revision_element_index gains an optional author parameter and _resolve_all passes it, so a filtered call has no other author's element to land on. The filter uses list_revisions' own author expression -- a missing w:author reads as "Unknown" -- so the index and the listing cannot disagree about who owns an element, which is what would otherwise turn the fix into a silent no-op. The group/changeset path (_resolve_ids) needs no author scoping: a member id is never a duplicated one, since _reconstruct_groups bars every occurrence of a repeated w:id from every group and _seed_max_change_id keeps a freshly allocated id above every id already in the file.
…uthors (ROADMAP.md #75) Flips the pin that documented the defect (accept_all's filtered pass no longer resolves another author's same-id revision as collateral) and adds the exact ROADMAP shape -- B's w:ins w:id="7" before A's w:del w:id="7" -- with the data-loss case, the accept counterpart, both unfiltered paths (so the scoping cannot regress into over-filtering), and a count-equals-listed-rows check. test_unattributed_revision_resolves_under_the_unknown_author covers the other half of the index's author expression: dropping the "Unknown" fallback leaves accept_all(author="Unknown") returning 0 on an unattributed revision, with the revision still pending and no warning raised, and passed every other test. TestGroupResolutionWithDuplicateIds pins why the group/changeset path needs no author scoping: both duplicate rows report group_id and changeset_id None, the neighbouring real group resolves exactly its own member, and the duplicated id is not a group or changeset id either.
…DMAP.md #75) The "author= filters process each author's changes independently" bullet now says the guarantee survives a producer repeating a w:id across authors.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe revision index now supports author filtering and normalized numeric IDs. Filtered resolution only processes the selected author’s elements when IDs repeat across authors. Grouping, listing, lookup, documentation, and regression tests cover the updated behavior. ChangesAuthor-scoped revision resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Author-filtered bulk revision operations now avoid changing another author’s same-ID revision, while normalized IDs keep listing and resolution consistent. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Caller
participant ResolutionManager
participant RevisionIndex
participant Document
Caller->>ResolutionManager: accept_all(author) or reject_all(author)
ResolutionManager->>RevisionIndex: build author-scoped index
RevisionIndex->>Document: return matching revision elements
ResolutionManager->>Document: resolve selected elements
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 9 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
=====================================
Coverage 97.9% 97.9%
=====================================
Files 23 23
Lines 5138 5141 +3
Branches 992 996 +4
=====================================
+ Hits 5035 5038 +3
Misses 49 49
Partials 54 54 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The index keyed on the raw w:id attribute while every lookup asks for
str(int(w:id)) and the listing reports int(w:id), so a nonconforming
w:id="007" listed as id 7 and was reachable by nothing: accept_all() returned
0, left it pending, and reported a clean document. Nothing warned, because a
mark with a numeric id is not part of the unhandled honesty floor -- and
groups_are_dead() read the live group as dead, which reaps its note comment.
Both sides now normalize on _adjudicable_id: the index keys on str of it and
drops elements that have none (nothing id-keyed can reach those, and
_unhandled_elements already reports them), and the fresh scan in
_find_revision_element compares the parsed id instead of the raw string.
Every consumer already looked ids up in the normalized form.
_revision_author joins _adjudicable_id in dom.py as the one definition of the
other half of a revision's identity, replacing five copies of
`getAttribute("w:author") or "Unknown"` in listing.py. That makes the
index-and-listing agreement this fix rests on structural rather than a
docstring claim backed by one regression test.
…MAP.md #75) test_unknown_id_still_raises passed for the wrong reason: it asked accept_group/accept_changeset for id 7 -- a w:id -- and group and changeset ids come from their own counters, so it raised only because this fixture's counters never reach 7, and would have started testing the opposite thing had they. It now asks for an id past the one actually allocated. DUPLICATE_ID_BODY builds its runs with the file's own _ins_xml/_del_xml helpers, which emit byte-identical XML, instead of hand-writing them.
…ects The duplicate-w:id guarantee belongs to accept_all/reject_all alone: accept_revision(id)/reject_revision(id) take only the id, so a duplicated one resolves whichever element comes first in the document. SKILL.md recommended the per-id pattern with no caveat; it now points at the bulk call or the row's group_id when an id is shared. Files three defects the PR #96 multi-review turned up, all pre-existing and each needing its own change and regression test: - #83 the per-id half of #75, with a zero-cost ambiguity test the manager already computes - #84 _rejoin_paragraph drops the tail paragraph's w:pPr, destroying another author's paragraph-mark revisions uncounted - #85 marks recorded inside a change record list as live revisions
Keying the fresh scan on _adjudicable_id made None match: the parse returns
None for exactly the marks no id-keyed call can reach, so an unguarded
comparison handed back the first of them. list_unhandled_revisions() reports
id=None for such a row, so this was reachable from the library's own output --
reject_revision(None) returned True and destroyed the mark's text, where
before it returned False. The index path never matched (no "None" key), so the
two lookup paths disagreed as well.
get_markup_text renders the adjudicable id too, so the one view that names
revision ids in text names the id accept_revision actually takes ("?" when
there is none) rather than the raw attribute.
Tests: id None resolves nothing through either verb and leaves the mark and
its text in place; and author scoping still holds for ids that collide only
once parsed (B's w:id="007" against A's w:id="7"), the duplicate-id class that
normalizing the key widened.
The per-id caveat advised falling back to the row's group_id, which is exactly what a duplicated id does not have: _reconstruct_groups bars every occurrence from grouping, as this PR's own TestGroupResolutionWithDuplicateIds asserts. Two rows sharing an id in list_revisions() is the signal instead, and the author-filtered bulk calls are the way to resolve them. docs/api.md carried neither the author-scoping guarantee nor the per-id caveat; its accept_all/reject_all/accept_revision/reject_revision sections now match SKILL.md, as every prior change to this prose has kept them. ROADMAP #75 no longer describes the fixed behaviour as current, and records why the w:id index was scoped by author instead of the filed element-identity suggestion. #83's fix direction named _reconstruct_groups as the sole writer of an explicit None group; _adopt_split_tail also writes one for an unambiguous split tail, so it points at the duplicate_ids set instead. has_own_revisions says why it keeps the raw w:author rather than _revision_author: an editor named "Unknown" must not claim unattributed marks.
…filtered-bulk-resolutio
…75) Keying the index on the parsed id left the two paths comparing different things: the fresh scan compared _adjudicable_id's int, the index looked up str(id). Anything int-equal but not an int then resolved through one and not the other -- accept_revision(True) destroyed revision 1 on the scan path (bool subclasses int) while .get("True") found nothing, and None, which list_unhandled_revisions() reports for a mark with no numeric id, matched every unreachable mark through == while missing the index entirely. Both sides now compare str of the adjudicable id. That restores what every id type did before this PR -- including "7", which both paths accepted and which narrowing to int would have broken for a caller round-tripping ids through JSON -- while keeping w:id="007" reachable. The None special case goes away with it, being just one value that now misses on both paths. get_markup_text's id is pinned: w:id="007" renders [ins#7:A] and a mark with no numeric id renders #?, the rows list_unhandled_revisions() reports.
…istry
The registry hand-rolled int(elem.getAttribute("w:id")) with a ValueError
guard in four places. Identical in behaviour -- int("") raises ValueError too
-- but it was the last place that answered "how is a revision id read?" on its
own, after the listing, the index and the honesty floor were folded onto one
definition.
…ed items docs/api.md promised an author= call "resolves that author's own elements and no one else's", which overstates a data-loss guarantee: the scoping decides which elements are targeted, not what survives, and rejecting an insertion still carries away revisions another author nested inside it. api.md's own rationale-notes section already said so. UnhandledRevision's docstring claimed its rows "carry no adjudicable id, so handing them to accept_revision() could only fail". An unhandled mark often does carry a numeric w:id, and where it collides with a handled revision's the call resolves that other element: verified, reject_revision(row.id) on a w:rPrChange id=7 deleted a w:ins id=7's text and left the rPrChange standing. Filed as #87. main added its own #83 (PR #95) after this branch forked, so the items filed here move to #84/#85/#86 and #75's cross-reference follows them.
The accept_all/reject_all docstrings and SKILL.md promised a filtered call "resolves only that author's own elements". Scoping decides which elements are targeted, not what survives: rejecting an insertion still carries away revisions another author nested inside it, reject_all(author=...) included. docs/api.md was corrected already; these are the sources it mirrors. _resolve_all's index bullet claimed the call "resolves exactly the rows it listed". It can resolve fewer -- a listed row nested inside a rejected insertion goes with its host -- so the honest claim is the negative one: it cannot resolve a row it did not list. The test named for the false generalization is renamed and says why equality holds in its own fixture. _revision_element_index's "a duplicated id resolves the same element whichever path finds it" predates the author parameter and is now false by design: the scan hands back the first element carrying the id, a scoped index the first one that author wrote. It is scoped to the unscoped case. accept_revision/reject_revision gain the duplicate-w:id caveat that SKILL.md and docs/api.md already carry.
groups_are_dead answers through the same w:id index resolution uses, so a group whose member carries w:id="007" was reported dead while it was live -- and a dead group has nothing left to explain, which is what deletes its note= comment. Nothing covered it; reverting the index key to the raw attribute now fails this test. _adopt_split_tail reads its origin id through _adjudicable_id like the rest of the registry. ROADMAP #75 keeps the one-line record of the fix and leaves the design rationale to the PR.
pablospe
changed the title
…P.md #75) "never someone else's" still read as a survival guarantee where its siblings had already been qualified: another author's revision nested inside a rejected insertion does go with it, uncounted. test_accept_all_author_filter_resolves_every_row_it_listed credited itself with covering the author expression, which its fixture cannot -- both rows carry an explicit w:author, so the "Unknown" fallback is pinned by test_unattributed_revision_resolves_under_the_unknown_author instead. It now says which half it covers and points at the other.
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.
What
accept_all(author=A)/reject_all(author=A)selected revisions by author butresolved them by
w:idagainst an index holding every author's elements.Where a producer repeats a
w:idacross authors, the id lookup returned theother author's element. A filtered call now targets only that author's own
elements.
Why
Data loss. With B's
<w:ins w:id="7">before A's<w:del w:id="7">— a shapeLibreOffice emits, present in the corpus —
reject_all(author="A")deleted B'sinserted text outright and
accept_all(author="A")made B's pending insertionpermanent, both reporting 2 and leaving nothing pending. B's revision was
silently adjudicated by a call that named A.
How
The fix rests on one property: the index must select exactly the elements the
listing selected. Both halves of a revision's identity now have a single
definition in
track_changes/dom.py, used by the listing, the index, thehonesty floor and the group registry alike:
_revision_author(a missingw:authorreads as"Unknown")._revision_element_index(author)filters on it and_resolve_allpasses it;that one argument is the behaviour change.
_adjudicable_id. The index keyed on the raw attribute while thelisting reported
int(w:id)and every lookup asked forstr(int), so anonconforming
w:id="007"was listed as 7 and reachable by nothing:accept_all()returned 0 and reported a clean document, andgroups_are_dead()read a live group as dead, which reaps itsnote=comment. Both lookup paths now compare
strof the adjudicable id.Scoping decides which elements are targeted, not what survives — rejecting an
insertion still carries away revisions nested inside it, and the docstrings,
docs/api.mdandSKILL.mdnow say so._resolve_ids(groups/changesets)needs no scoping:
_reconstruct_groupsbars every occurrence of a repeated idfrom every group and
_seed_max_change_idkeeps allocated ids above every id inthe file, so a member id is never duplicated.
TestGroupResolutionWithDuplicateIdspins that.
Files four adjacent defects this turned up, all pre-existing: #84 (the per-id
verbs still resolve a duplicated id by position), #85 (
_rejoin_paragraphdrops the tail paragraph's
w:pPr), #86 (marks recorded inside a change recordlist as live), #87 (an unhandled mark's
w:idcan resolve a handled revision).Testing
Full suite 1900 passed, 2 skipped, 2 xfailed.
ruff format/ruff checkclean,ty checkexit 0. Corpus harness unchanged from baseline (76/77; the onerejection is invalid input).
Each half of the invariant is independently pinned — reverting the author
scoping fails four tests, the id normalization five, the
"Unknown"fallbackone (both its cases).
test_author_filter_holds_when_ids_collide_only_after_normalizingfails under either of the first two by design: it needs both halves. The
scoping tests assert on surviving text, not just counts. Also pinned:
accept_revision(None)andaccept_revision(True)resolve nothing on both lookup paths, group liveness for a
w:id="007"member,and
get_markup_textrendering the idaccept_revisionaccepts.Repro on the ROADMAP shape, old index vs new:
Summary by CodeRabbit