Skip to content

Commit cc10ed8

Browse files
committed
docs: scope the author-exact claim to targeting, and renumber the filed 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.
1 parent b0600d7 commit cc10ed8

3 files changed

Lines changed: 21 additions & 10 deletions

File tree

‎ROADMAP.md‎

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

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

39-
`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.
39+
`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 #84.
4040

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

@@ -54,18 +54,22 @@ Every path that decides which `rPr` a newly inserted run copies picks its bounda
5454

5555
The #91 diagnostics caught it on their first CI failure (run 33331432527, load average 1.14 — an idle runner): the kernel wrote `kernel.json` in 0.5 s, then our `kernel_info` probe on the control channel raised inside ipykernel's ZMQStream callback — `KeyError: '_control_lock'` (traitlets) — because the control message arrived before the kernel finished initializing; the callback's exception left the control stream broken and the kernel never replied within the remaining 59.5 s. So the residual "flake" is a race our own probe triggers, not load. Fix directions, in preference order: probe on the shell channel or wait for the iopub `status: starting` message before touching control; retry with a *fresh* client (the broken stream may be permanent for that connection); check whether current ipykernel has fixed `_control_lock` initialization order and pin/upgrade. Regression test: a kernel whose control channel receives `kernel_info` immediately after `kernel.json` appears must still become ready.
5656

57-
### 83. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss]
57+
### 84. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss]
5858

5959
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.
6060

61-
### 84. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss]
61+
### 85. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss]
6262

6363
Rejecting a paragraph-mark insertion migrates only the mark `w:ins` off the tail paragraph and then drops that paragraph's whole `w:pPr`, so another author's paragraph-mark `w:del`/`w:moveFrom`/`w:moveTo` and their `w:pPrChange` are destroyed — uncounted, unwarned, and with the merged paragraph's break left permanently untracked. Reproduces with no author filter, and is order-dependent within a single author: the same document returns 1 or 2 depending only on whether the mark-ins id sorts above the mark-del id. The `accept_all` docstring names only the `w:pPrChange` case. Fix: migrate the whole downstream mark set onto the surviving paragraph's mark `rPr`, since they all describe the break it now owns. Found by the multi-review of PR #96.
6464

65-
### 85. Historical marks inside change records list as live revisions
65+
### 86. Historical marks inside change records list as live revisions
6666

6767
`list_revisions` and `_revision_element_index` walk with `skip_change_records=False` while `_unhandled_elements` uses `True`, so a revision mark recorded *inside* a change record is offered as adjudicable. `w:pPr/w:rPr/w:rPrChange/w:rPr/w:ins` is conforming (CT_ParaRPrOriginal admits EG_ParaRPrTrackChanges) and lists as a live insertion, and resolving it mutates the other author's change record; the nonconforming `w:pPrChange`-holding-`w:rPr` shape is worse — it merges two live paragraphs. Fix: have both walks pass `skip_change_records=True` and leave recorded marks to the honesty floor. Needs its own regression test. Found by the multi-review of PR #96.
6868

69+
### 87. An unhandled mark's `w:id` can resolve a handled revision [data loss]
70+
71+
`list_unhandled_revisions()` reports a numeric `w:id` when the mark has one, and ids are unique only per producer: with `<w:rPrChange w:id="7" w:author="B">` alongside `<w:ins w:id="7" w:author="A">`, `reject_revision(row.id)` returns True, deletes A's inserted text and leaves the `w:rPrChange` standing — the caller adjudicated a revision it never asked about while the one it named survives. `UnhandledRevision`'s docstring claimed this could only fail; it now says what actually happens. Not reproduced on the corpus (0 handled/unhandled id collisions across 77 files), so it is a latent shape, not an observed one. Fix: have the per-id verbs verify the located element is the one the id was read from, or refuse when a handled and an unhandled element share the id. Related to #84, which is the same hazard between two *handled* rows. Found by the multi-review of PR #96.
72+
6973
### 69. Return-leg reconciliation: which changesets survived? [design]
7074

7175
The product loop is agent proposes → human adjudicates in Word → document comes back; accepted revisions become plain text, rejected ones vanish, and there is no way to ask "which of my changesets survived?". A sent-vs-returned compare keyed on changeset `w:date` + content. Design first; large.

‎docs/api.md‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1089,10 +1089,12 @@ marker is dropped without merging or splitting paragraphs.
10891089
Only `word/document.xml` is inspected; headers, footers and footnotes are the
10901090
container-parts epic (ROADMAP.md #30).
10911091

1092-
An `author=` call resolves that author's own elements and no one else's, even
1092+
An `author=` call targets that author's own elements and no one else's, even
10931093
where a producer repeats one `w:id` across authors (LibreOffice does): the id
10941094
lookup is scoped to the author being resolved, so another author's same-id
1095-
revision is never a candidate.
1095+
revision is never a candidate. That is about which elements are *targeted*, not
1096+
about what survives — rejecting an insertion still removes whatever another
1097+
author nested inside it, `reject_all(author=…)` included.
10961098

10971099
**Parameters:**
10981100

@@ -1123,8 +1125,9 @@ Reject every listed revision.
11231125
Resolves insertions, deletions, content moves (both halves as a unit — the
11241126
text is back at its source exactly once) and paragraph-property changes (the
11251127
recorded previous properties are restored); every other revision type is left
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.
1128+
pending and reported exactly as in `accept_all()`. `author=` targets one
1129+
author's elements in the same way, duplicate `w:id`s across authors included —
1130+
and carries away nested revisions in the same way too.
11281131

11291132
**Parameters:**
11301133

‎docx_editor/track_changes/models.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -925,8 +925,12 @@ class UnhandledRevision:
925925
"""One revision element this library does not accept or reject.
926926
927927
Returned by ``Document.list_unhandled_revisions()``. Deliberately not a
928-
:class:`Revision`: these carry no adjudicable id, so handing them to
929-
``accept_revision()`` could only fail — see ``list_unhandled_revisions``.
928+
:class:`Revision`: nothing here is adjudicable, so there is no id to hand
929+
``accept_revision()`` — see ``list_unhandled_revisions``. Where ``id`` is
930+
not None it is reported for identification only: an unhandled mark's
931+
``w:id`` may coincide with a *handled* revision's, and passing it to
932+
``accept_revision``/``reject_revision`` then resolves that other element
933+
instead (ROADMAP.md #87).
930934
931935
Attributes:
932936
tag: the element's tag — one of ``UNHANDLED_REVISION_TAGS``, or a

0 commit comments

Comments
 (0)