Skip to content

Commit 46aad5b

Browse files
committed
docs: correct the duplicate-w:id guidance and sync docs/api.md
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.
1 parent 4f8ec96 commit 46aad5b

3 files changed

Lines changed: 23 additions & 6 deletions

File tree

‎ROADMAP.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ Only `w:instrText` is known to the library today. A `replace()` targeting text i
5959

6060
### 75. Author-filtered bulk resolution must not touch other authors' revisions [data loss; after #73 step 9]
6161

62-
`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `<w:ins w:id="7">` before A's `<w:del w:id="7">` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documents this and `tests/test_track_changes.py::…:1143` pins it as expected — it is not acceptable for a data-loss path. Fix: resolve the exact elements selected by the listing (element identity), not their ids; flip the pinning test. Lives in the resolution cluster, so it lands after #73 moves it (found by CodeRabbit on PR #75; verified 2026-08-30).
62+
`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `<w:ins w:id="7">` before A's `<w:del w:id="7">` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documented this and a test pinned it as expected — not acceptable for a data-loss path (found by CodeRabbit on PR #75; verified 2026-08-30). **Fixed in PR #96**, by scoping the w:id index to the author being resolved rather than the filed suggestion of resolving element identities: `Revision` carries no element handle, so identity would have meant a wider signature change for the same outcome — a same-id element of the *same* author still resolves in one pass either way. The pinning test is flipped. The per-id verbs keep the defect; that is #83.
6363

6464
### 79. Inserted text loses formatting after an empty split segment [after #73 step 7]
6565

@@ -75,7 +75,7 @@ The #91 diagnostics caught it on their first CI failure (run 33331432527, load a
7575

7676
### 83. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss]
7777

78-
The per-id half of #75, which fixed only `accept_all`/`reject_all`. With B's `<w:ins w:id="7">` before A's `<w:del w:id="7">`, `list_revisions(author="A")` returns one row with id 7, and `reject_revision(7)` deletes B's inserted text and leaves A's deletion pending; `accept_revision(7)` makes B's insertion permanent. The caller cannot disambiguate — `Revision` carries no element handle — and `SKILL.md` documents this exact per-id pattern. Fix: either give the per-id verbs an author/identity selector, or refuse an ambiguous id with a teaching error; the manager already knows which ids are ambiguous, since `_reconstruct_groups` is the sole writer of `self._revision_groups[rev_id] = None` (`registry.py`) and writes it only for duplicated ids. SKILL.md carries an interim caveat. Found by the multi-review of PR #96.
78+
The per-id half of #75, which fixed only `accept_all`/`reject_all`. With B's `<w:ins w:id="7">` before A's `<w:del w:id="7">`, `list_revisions(author="A")` returns one row with id 7, and `reject_revision(7)` deletes B's inserted text and leaves A's deletion pending; `accept_revision(7)` makes B's insertion permanent. The caller cannot disambiguate — `Revision` carries no element handle — and `SKILL.md` documents this exact per-id pattern. Fix: either give the per-id verbs an author/identity selector, or refuse an ambiguous id with a teaching error. The duplicate set is already computed — `duplicate_ids` in `_reconstruct_groups` (`registry.py`) — and then discarded; retain or recompute it rather than testing `_revision_groups[id] is None`, which `_adopt_split_tail` also writes for an unambiguous split tail whose origin was ungroupable. SKILL.md carries an interim caveat. Found by the multi-review of PR #96.
7979

8080
### 84. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss]
8181

‎docs/api.md‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -867,6 +867,14 @@ accepting the `move_to` and rejecting the `move_from` duplicates the text, the
867867
inverse loses it. A lone half in a damaged file behaves as what it structurally
868868
is: a `move_from` alone is a deletion, a `move_to` alone an insertion.
869869

870+
**Duplicate ids are ambiguous here.** A producer may reuse one `w:id` across
871+
authors, and this call takes only the id: handed a duplicated one it resolves
872+
whichever element comes first in the document, which may be another author's.
873+
Groups are no way around it — a duplicated id is barred from grouping, so both
874+
rows carry `group_id=None`. Two rows sharing an id in `list_revisions()` is the
875+
signal; resolve them with `accept_all(author=…)`/`reject_all(author=…)`, which
876+
is author-exact.
877+
870878
**Parameters:**
871879

872880
- `revision_id` (int): ID of the revision to accept
@@ -892,7 +900,8 @@ Reject a revision by ID.
892900
- For `property_change`: restores the paragraph's recorded previous properties. A record with none (LibreOffice writes a self-closing `w:pPrChange` for "previously no properties") clears them; the recorded style id is restored verbatim even when the document defines no such style (Word falls back to Normal for it)
893901
- Nested revisions: rejecting an insertion removes everything inside it — deletions another author nested inside it disappear with it
894902

895-
Resolve both halves of a move together — see `accept_revision()`.
903+
Resolve both halves of a move together — see `accept_revision()`, whose
904+
duplicate-`w:id` caveat applies here too.
896905

897906
**Parameters:**
898907

@@ -1080,6 +1089,11 @@ marker is dropped without merging or splitting paragraphs.
10801089
Only `word/document.xml` is inspected; headers, footers and footnotes are the
10811090
container-parts epic (ROADMAP.md #30).
10821091

1092+
An `author=` call resolves that author's own elements and no one else's, even
1093+
where a producer repeats one `w:id` across authors (LibreOffice does): the id
1094+
lookup is scoped to the author being resolved, so another author's same-id
1095+
revision is never a candidate.
1096+
10831097
**Parameters:**
10841098

10851099
- `author` (str, optional): If provided, only accept revisions by this author
@@ -1109,7 +1123,8 @@ Reject every listed revision.
11091123
Resolves insertions, deletions, content moves (both halves as a unit — the
11101124
text is back at its source exactly once) and paragraph-property changes (the
11111125
recorded previous properties are restored); every other revision type is left
1112-
pending and reported exactly as in `accept_all()`.
1126+
pending and reported exactly as in `accept_all()`. `author=` is author-exact in
1127+
the same way, duplicate `w:id`s across authors included.
11131128

11141129
**Parameters:**
11151130

‎skills/docx/SKILL.md‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -682,8 +682,10 @@ nested deletion's `nested_under` points back at its host.
682682
reuse one `w:id` across authors, and `accept_revision(id)`/
683683
`reject_revision(id)` take only the id: handed a duplicated one they resolve
684684
whichever element comes first in the document, which may be the other
685-
author's. When a listed revision shares its id with a second row, prefer
686-
`accept_all(author=…)` / the row's `group_id`.
685+
author's. Groups are no way around it — a duplicated id is barred from
686+
grouping, so both rows carry `group_id=None`. Two rows sharing an id in
687+
`list_revisions()` is the signal; resolve them with
688+
`accept_all(author=…)`/`reject_all(author=…)`, which is author-exact.
687689

688690
**Moves are two rows.** A drag-and-drop move lists as a `move_from` (the
689691
source, text excluded from the visible view) and a `move_to` (the

0 commit comments

Comments
 (0)