From ff3c5742a2f26abef7b8cbdee5a23c5b26d1abd5 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Thu, 16 Jul 2026 23:10:57 +0200 Subject: [PATCH 1/6] =?UTF-8?q?fix:=20input=20validation=20=E2=80=94=20emp?= =?UTF-8?q?ty=20search=20text,=20batch=20element=20types,=20non-string=20r?= =?UTF-8?q?efs=20(ISSUES.md=20#40)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three input-validation gaps fixed, all raising field-specific ValueErrors up front instead of failing silently or leaking raw internal errors: - Empty/None search text: _locate_in_paragraph and _locate_document_wide now reject it before any change is made. Previously an empty needle with an explicit occurrence created zero revisions (the edit silently vanished); without one it raised a meaningless AmbiguousTextError. - Non-EditOperation batch elements: batch_edit raises BatchOperationError with the offending index (original=None, like other batch-level rules); validate_batch keeps its never-raises contract and returns an invalid row. Both previously raised raw AttributeError. - Non-string paragraph refs: ParagraphRef.parse rejects them before the regex (was raw TypeError), covering all EditOperation constructors; the four Document edit methods reject non-string paragraph args, which previously fell through to the document-wide search branch silently. Docstrings and SKILL.md error-contract table updated; 13 new tests (TDD-first), including silent-no-op regression repros and batch atomicity checks. --- docx_editor/document.py | 25 +++++- docx_editor/track_changes.py | 49 +++++++++-- docx_editor/xml_editor.py | 7 +- skills/docx/SKILL.md | 4 +- tests/test_batch_edit.py | 13 +++ tests/test_error_quality.py | 153 +++++++++++++++++++++++++++++++++++ 6 files changed, 238 insertions(+), 13 deletions(-) diff --git a/docx_editor/document.py b/docx_editor/document.py index e2ddeb0..88af3c8 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -40,6 +40,14 @@ ) +def _require_ref_string(paragraph: str) -> None: + """Reject non-string paragraph refs before they can silently select the + RevisionManager's document-wide search branch (its ``paragraph=None`` + mode is intentional at that layer, not at this one).""" + if not isinstance(paragraph, str): + raise ValueError(f"'paragraph' must be a paragraph ref string like 'P3#a7b2', got {type(paragraph).__name__}") + + class Document: """Word document with track changes and comment support. @@ -725,6 +733,8 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i for accept_group()/reject_group(). Raises: + ValueError: If ``find`` is empty or None, ``paragraph`` is not a + ref string, or ``occurrence`` is negative. TextNotFoundError: If ``find`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``find`` @@ -736,6 +746,7 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i doc.replace("other text", "new text", paragraph=new_ref) """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.replace_text(find, replace_with, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -755,6 +766,8 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> revisions this edit created. Raises: + ValueError: If ``text`` is empty or None, ``paragraph`` is not a + ref string, or ``occurrence`` is negative. TextNotFoundError: If ``text`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``text`` @@ -765,6 +778,7 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> new_ref = doc.delete("obsolete clause", paragraph="P2#f3c1") """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.suggest_deletion(text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -785,6 +799,8 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in revisions this edit created. Raises: + ValueError: If ``anchor`` is empty or None, ``paragraph`` is not + a ref string, or ``occurrence`` is negative. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -795,6 +811,7 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in new_ref = doc.insert_after("Section 5", " (as amended)", paragraph="P2#f3c1") """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.insert_text_after(anchor, text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -815,6 +832,8 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i revisions this edit created. Raises: + ValueError: If ``anchor`` is empty or None, ``paragraph`` is not + a ref string, or ``occurrence`` is negative. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -825,6 +844,7 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i new_ref = doc.insert_before("Section 6", "New clause: ", paragraph="P2#f3c1") """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.insert_text_before(anchor, text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -863,8 +883,9 @@ def batch_edit( Raises: BatchOperationError: The only exception a non-dry-run batch raises - for a failing operation — validation (malformed ref, stale - hash, bad index) and apply (missing text, ambiguous target) + for a failing operation — validation (element is not an + EditOperation, malformed ref, stale hash, bad index) and apply + (missing text, ambiguous target) failures alike. ``operation_index`` names the failing op and ``original`` (also ``__cause__``) holds the underlying typed exception (e.g. a HashMismatchError with ``actual_hash``). diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index 76a721a..2287cca 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -172,6 +172,15 @@ def insert_before(cls, anchor: str, text: str, *, paragraph: str, occurrence: in return cls._insert("insert_before", anchor, text, paragraph, occurrence) +def _not_an_edit_operation(op: object) -> str: + """Shared batch_edit/validate_batch message for a non-EditOperation element, + so the raising and never-raises paths cannot drift apart.""" + return ( + f"expected EditOperation, got {type(op).__name__} — build operations with " + "EditOperation.replace()/.delete()/.insert_after()/.insert_before()" + ) + + @dataclass class EditValidationResult: """Outcome of validating one EditOperation in a dry-run batch.""" @@ -608,7 +617,8 @@ def _locate_in_paragraph(self, paragraph, paragraph_ref: str, text: str, occurre be unique within the paragraph. Raises: - ValueError: If ``occurrence`` is negative. + ValueError: If ``occurrence`` is negative, or ``text`` is empty + or None. TextNotFoundError: If the text is absent, or ``occurrence`` is out of range (then with ``occurrence``/``total_occurrences`` set). AmbiguousTextError: If ``occurrence`` is None and the text matches @@ -616,6 +626,8 @@ def _locate_in_paragraph(self, paragraph, paragraph_ref: str, text: str, occurre """ if occurrence is not None and occurrence < 0: raise ValueError(f"occurrence must be >= 0, got {occurrence}") + if not text: + raise ValueError(f"search text must be a non-empty string, got {text!r}") text_map = build_text_map(paragraph) total = count_in_text_map(text_map, text) @@ -658,11 +670,12 @@ def batch_edit(self, operations: list[EditOperation]) -> list[int]: List of change IDs, one per operation (in original input order) Raises: - BatchOperationError: If any operation fails — validation (malformed - ref, stale hash, bad index) or apply (missing text, ambiguous - target). Carries ``operation_index`` so the caller knows which - op failed, and ``original`` (also ``__cause__``) with the - underlying typed exception. No edits are applied on failure. + BatchOperationError: If any operation fails — validation (element + is not an EditOperation, malformed ref, stale hash, bad index) + or apply (missing text, ambiguous target). Carries + ``operation_index`` so the caller knows which op failed, and + ``original`` (also ``__cause__``) with the underlying typed + exception. No edits are applied on failure. """ if not operations: return [] @@ -670,6 +683,8 @@ def batch_edit(self, operations: list[EditOperation]) -> list[int]: # Parse and validate all refs upfront parsed: list[tuple[int, ParagraphRef, EditOperation]] = [] for i, op in enumerate(operations): + if not isinstance(op, EditOperation): + raise BatchOperationError(i, _not_an_edit_operation(op)) if not op.paragraph: raise BatchOperationError(i, "paragraph reference is required for batch mode") try: @@ -780,10 +795,17 @@ def validate_batch(self, operations: list[EditOperation]) -> list[EditValidation operations: List of EditOperation objects (each should have paragraph set) Returns: - One EditValidationResult per operation, in input order. + One EditValidationResult per operation, in input order. An element + that is not an EditOperation at all comes back as an invalid + result (``paragraph=None``), never as an exception. """ results = [] for i, op in enumerate(operations): + if not isinstance(op, EditOperation): + results.append( + EditValidationResult(index=i, paragraph=None, valid=False, error=_not_an_edit_operation(op)) + ) + continue error = self._validate_single(op) results.append( EditValidationResult( @@ -1122,7 +1144,8 @@ def _locate_document_wide(self, text: str, occurrence: int | None = None) -> Tex the exact total is part of the error contract. Raises: - ValueError: If ``occurrence`` is negative. + ValueError: If ``occurrence`` is negative, or ``text`` is empty + or None. TextNotFoundError: If the text is not found or occurrence doesn't exist; ``total_occurrences`` matches :meth:`count_matches`. AmbiguousTextError: If ``occurrence`` is None and the text matches @@ -1130,6 +1153,8 @@ def _locate_document_wide(self, text: str, occurrence: int | None = None) -> Tex """ if occurrence is not None and occurrence < 0: raise ValueError(f"occurrence must be >= 0, got {occurrence}") + if not text: + raise ValueError(f"search text must be a non-empty string, got {text!r}") occ = occurrence if occurrence is not None else 0 match = self._find_across_boundaries(text, occ) if match is None: @@ -1269,6 +1294,8 @@ def replace_text( The change ID of the insertion Raises: + ValueError: If ``find`` is empty or None, or ``occurrence`` is + negative TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``find`` matches more than once in the search scope @@ -1298,6 +1325,8 @@ def suggest_deletion(self, text: str, occurrence: int | None = None, paragraph: The change ID of the deletion Raises: + ValueError: If ``text`` is empty or None, or ``occurrence`` is + negative TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``text`` matches more than once in the search scope @@ -2133,6 +2162,8 @@ def insert_text_after( The change ID of the insertion Raises: + ValueError: If ``anchor`` is empty or None, or ``occurrence`` is + negative TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope @@ -2157,6 +2188,8 @@ def insert_text_before( The change ID of the insertion Raises: + ValueError: If ``anchor`` is empty or None, or ``occurrence`` is + negative TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index 0e58908..7369e0d 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -122,8 +122,13 @@ def parse(cls, ref: str) -> "ParagraphRef": """Parse a paragraph reference string like 'P3#a7b2'. Raises: - ValueError: If the reference format is invalid + ValueError: If the reference is not a string or its format is + invalid """ + if not isinstance(ref, str): + raise ValueError( + f"Invalid paragraph reference {ref!r}: expected a string like 'P3#a7b2', got {type(ref).__name__}" + ) m = _PARAGRAPH_REF_RE.match(ref) if not m: raise ValueError( diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index 48bd424..a855d1d 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -303,7 +303,7 @@ doc.close() **Multi-author documents:** Editing inside *another* author's pending insertion preserves their proposal, matching Word: deletions nest a `` under your authorship inside their ``, and replacements/insertions put your text in your own sibling `` (splitting theirs when needed) instead of silently rewriting it. `accept_all(author=...)` / `reject_all(author=...)` then resolve each author's changes independently. Only your own pending insertions are edited in place. -**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. +**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. `ValueError` for empty or missing search/anchor text, a non-string `paragraph` ref, or a negative `occurrence` — all rejected up front, before any change is made. ### Comments API @@ -658,7 +658,7 @@ All LLM-facing errors inherit from `DocxEditError` and carry structured fields s | `TextNotFoundError` | `search_text`, `paragraph_ref`, `paragraph_preview`, `occurrence`, `total_occurrences` | Use `paragraph_preview` to pick a substring that actually appears; if `total_occurrences` is set, retry with an `occurrence` < `total_occurrences`. | | `AmbiguousTextError` | `search_text`, `paragraph_ref`, `paragraph_preview`, `total_occurrences` | The target matched more than once with no `occurrence` given. Enumerate with `find_all()` and pick, or pass an explicit `occurrence` (0-based). | | `ParagraphIndexError` | `index`, `total_paragraphs` | Clamp to `1..total_paragraphs` or call `list_paragraphs()` to pick a valid ref. | -| `BatchOperationError` | `operation_index`, `reason`, `original` | Fix the op at `operations[operation_index]` (or drop it) and retry the batch; `original` is the underlying typed error (e.g. use its `actual_hash` to re-target a stale ref), or None for batch-level rules with no underlying exception (missing paragraph ref, duplicate paragraph). Batch methods never raise the inner types directly. | +| `BatchOperationError` | `operation_index`, `reason`, `original` | Fix the op at `operations[operation_index]` (or drop it) and retry the batch; `original` is the underlying typed error (e.g. use its `actual_hash` to re-target a stale ref), or None for batch-level rules with no underlying exception (missing paragraph ref, duplicate paragraph, element is not an EditOperation). Batch methods never raise the inner types directly. | | `DocumentOpenError` | `path`, `owner_file` | **Do not retry blindly.** The destination is open in Word. Stop and tell the user to close it. Only pass `force=True` if the user confirms the `~$` lock is stale (crashed session). | ```python diff --git a/tests/test_batch_edit.py b/tests/test_batch_edit.py index 8ab01ba..001aa46 100644 --- a/tests/test_batch_edit.py +++ b/tests/test_batch_edit.py @@ -1049,6 +1049,19 @@ def test_empty_paragraph_ref_rejected(self): with pytest.raises(ValueError, match="Invalid paragraph reference"): EditOperation.delete("a", paragraph="") + def test_none_paragraph_ref_rejected(self): + """paragraph=None gets the field-specific ValueError, not a raw regex TypeError.""" + with pytest.raises(ValueError, match="Invalid paragraph reference None"): + EditOperation.replace("a", "b", paragraph=None) # type: ignore[arg-type] + + def test_delete_none_paragraph_ref_rejected(self): + with pytest.raises(ValueError, match="Invalid paragraph reference None"): + EditOperation.delete("x", paragraph=None) # type: ignore[arg-type] + + def test_non_string_paragraph_ref_names_the_type(self): + with pytest.raises(ValueError, match="expected a string like 'P3#a7b2', got int"): + EditOperation.insert_after("a", "x", paragraph=3) # type: ignore[arg-type] + def test_negative_occurrence_rejected(self): with pytest.raises(ValueError, match=r"EditOperation\.replace\(\): occurrence must be >= 0, got -1"): EditOperation.replace("a", "b", paragraph="P2#f3c1", occurrence=-1) diff --git a/tests/test_error_quality.py b/tests/test_error_quality.py index 4786a1c..ea53bf2 100644 --- a/tests/test_error_quality.py +++ b/tests/test_error_quality.py @@ -235,6 +235,59 @@ def test_inner_missing_field_failure_carries_operation_index(self, doc_path): finally: doc.close() + def test_non_editoperation_element_rejected_with_index(self, doc_path): + """A dict element is a batch-level rule violation, not a raw AttributeError.""" + doc = self._build_batch_doc(doc_path) + try: + before = doc.get_visible_text() + ops = [{"action": "replace", "find": "a", "replace_with": "b"}] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit(ops) # type: ignore[arg-type] + + err = exc.value + assert err.operation_index == 0 + assert err.original is None + assert "expected EditOperation, got dict" in err.reason + assert "EditOperation.replace()" in err.reason # names the recovery path + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_non_editoperation_mixed_batch_names_offending_index(self, doc_path): + """Atomicity: the valid op at index 0 is not applied when index 1 is rejected.""" + doc = self._build_batch_doc(doc_path) + try: + before = doc.get_visible_text() + refs = doc.list_paragraphs() + valid = EditOperation.replace("Paragraph 1", "x", paragraph=refs[0].split("|")[0]) + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([valid, "not an op"]) # type: ignore[list-item] + + assert exc.value.operation_index == 1 + assert "expected EditOperation, got str" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_dry_run_reports_non_editoperation_without_raising(self, doc_path): + """validate_batch never raises — a bad element type comes back as an invalid row.""" + doc = self._build_batch_doc(doc_path) + try: + refs = doc.list_paragraphs() + valid = EditOperation.replace("Paragraph 1", "x", paragraph=refs[0].split("|")[0]) + results = doc.batch_edit([valid, {"action": "delete"}], dry_run=True) # type: ignore[list-item] + + assert len(results) == 2 + assert results[0].valid + bad = results[1] + assert bad.index == 1 + assert not bad.valid + assert bad.paragraph is None + assert bad.error is not None + assert "expected EditOperation, got dict" in bad.error + finally: + doc.close() + class TestAmbiguousTextErrorQuality: """ @@ -409,6 +462,106 @@ def test_all_paths_raise_value_error(self, doc_path): doc.close() +class TestEmptySearchTextRejected: + """ + Before: an empty search string slipped into ``find_in_text_map``, where + ``str.find("", start)`` matches at every position. With an explicit + ``occurrence`` the zero-width match carried no DOM positions, so the + apply helpers created zero revisions and the edit silently vanished; + without one, the caller got a meaningless AmbiguousTextError + ("'' matches N times"). + + After: both shared locate paths reject empty (and None) search text up + front with the same ValueError, before any change is made. + (``add_comment`` never had this bug — it pre-rejects empty anchors with + its own CommentError; pinned below so the asymmetry stays deliberate.) + """ + + def test_empty_find_with_occurrence_no_longer_a_silent_no_op(self, doc_path): + """The original regression: explicit occurrence=0 made the edit vanish.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="search text must be a non-empty string"): + doc.replace("", "TEXT", paragraph=ref, occurrence=0) + assert doc.list_revisions() == [] # nothing applied, nothing vanished + finally: + doc.close() + + def test_all_paths_raise_value_error(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + rm = doc._revision_manager + calls = [ + # scoped, no occurrence (was: meaningless AmbiguousTextError) + lambda: doc.replace("", "x", paragraph=ref), + lambda: doc.delete("", paragraph=ref), + lambda: doc.insert_after("", "x", paragraph=ref), + lambda: doc.insert_before("", "x", paragraph=ref), + # scoped, explicit occurrence (was: silent no-op) + lambda: doc.delete("", paragraph=ref, occurrence=0), + # document-wide RM paths, both failure modes + lambda: rm.replace_text("", "x"), + lambda: rm.replace_text("", "x", occurrence=0), + ] + for call in calls: + with pytest.raises(ValueError, match="search text must be a non-empty string"): + call() + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_none_search_text_names_the_value(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="search text must be a non-empty string, got None"): + doc.replace(None, "x", paragraph=ref) # type: ignore[arg-type] + finally: + doc.close() + + def test_add_comment_still_rejects_empty_anchor_its_own_way(self, doc_path): + """add_comment pre-rejects empty anchors with CommentError, not ValueError.""" + from docx_editor import CommentError + + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(CommentError, match="anchor_text must not be empty"): + doc.add_comment("", "note", paragraph=ref) + finally: + doc.close() + + +class TestNonStringParagraphRefRejected: + """ + Before: ``Document.replace(..., paragraph=None)`` didn't error at all — + None slipped through the keyword-only ``paragraph: str`` signature into + the RevisionManager, silently selecting its document-wide search branch. + + After: the four Document edit methods reject non-string paragraph refs + up front. (RevisionManager keeps ``paragraph=None`` as its intended + document-wide mode.) + """ + + def test_all_edit_methods_reject_none(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + calls = [ + lambda: doc.replace("needle", "x", paragraph=None), # type: ignore[arg-type] + lambda: doc.delete("needle", paragraph=None), # type: ignore[arg-type] + lambda: doc.insert_after("needle", "x", paragraph=None), # type: ignore[arg-type] + lambda: doc.insert_before("needle", "x", paragraph=None), # type: ignore[arg-type] + ] + for call in calls: + with pytest.raises(ValueError, match="'paragraph' must be a paragraph ref string"): + call() + assert doc.list_revisions() == [] # the doc-wide fallback never fired + finally: + doc.close() + + class TestSharedBaseClass: """All structured errors inherit from `DocxEditError`. From 1706d61e7fef67d5914a2c79265433bbe89707e2 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Thu, 16 Jul 2026 23:47:50 +0200 Subject: [PATCH 2/6] fix: reject truthy non-string search text and payloads (review findings) Review found that the falsiness checks let truthy non-strings through: find=123 raised a raw TypeError from str.find that escaped both documented batch contracts (batch_edit raises only BatchOperationError; validate_batch never raises), and a non-string payload (replace_with=123) passed dry-run as valid then crashed apply with a raw AttributeError. - Locate helpers: guard is now `not isinstance(text, str) or not text` - _validate_single: catch (ValueError, DocxEditError) around the locate call, mirroring batch_edit's apply loop, so dry-run never raises - _resolve_action_target: payload checks upgraded to isinstance so both batch paths reject non-string replace_with / insert text - replace_text/_insert_text: direct-API payload guards - EditOperation constructors: falsiness/None checks upgraded to isinstance, so bad ops fail at construction 8 new tests (TDD-first) covering constructors, direct API, batch apply (wrapped with op index, document unchanged), and dry-run invalid rows. Docstrings and SKILL.md error contract updated to match. Skipped as non-blocking per red-team triage: _require_ref_string naming (module-level pure guard, distinct from _ensure_* instance methods) and the third "non-empty" message phrasing (cosmetic). --- docx_editor/document.py | 19 ++++--- docx_editor/track_changes.py | 66 ++++++++++++---------- skills/docx/SKILL.md | 2 +- tests/test_batch_edit.py | 15 +++++ tests/test_error_quality.py | 104 +++++++++++++++++++++++++++++++++++ 5 files changed, 168 insertions(+), 38 deletions(-) diff --git a/docx_editor/document.py b/docx_editor/document.py index 88af3c8..18705cb 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -733,8 +733,9 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i for accept_group()/reject_group(). Raises: - ValueError: If ``find`` is empty or None, ``paragraph`` is not a - ref string, or ``occurrence`` is negative. + ValueError: If ``find`` is not a non-empty string, + ``replace_with`` is not a string, ``paragraph`` is not a ref + string, or ``occurrence`` is negative. TextNotFoundError: If ``find`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``find`` @@ -766,8 +767,8 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> revisions this edit created. Raises: - ValueError: If ``text`` is empty or None, ``paragraph`` is not a - ref string, or ``occurrence`` is negative. + ValueError: If ``text`` is not a non-empty string, ``paragraph`` + is not a ref string, or ``occurrence`` is negative. TextNotFoundError: If ``text`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``text`` @@ -799,8 +800,9 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in revisions this edit created. Raises: - ValueError: If ``anchor`` is empty or None, ``paragraph`` is not - a ref string, or ``occurrence`` is negative. + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, ``paragraph`` is not a ref string, or + ``occurrence`` is negative. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -832,8 +834,9 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i revisions this edit created. Raises: - ValueError: If ``anchor`` is empty or None, ``paragraph`` is not - a ref string, or ``occurrence`` is negative. + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, ``paragraph`` is not a ref string, or + ``occurrence`` is negative. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index 2287cca..263ad07 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -82,12 +82,13 @@ def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: in Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``find`` is empty, or ``replace_with`` is None. + negative, ``find`` is not a non-empty string, or + ``replace_with`` is not a string. """ cls._validate_common("replace", paragraph, occurrence) - if not find: + if not isinstance(find, str) or not find: raise ValueError("EditOperation.replace(): 'find' must be a non-empty string — the text to search for") - if replace_with is None: + if not isinstance(replace_with, str): raise ValueError("EditOperation.replace(): 'replace_with' must be a string (empty string is allowed)") return cls( action="replace", @@ -110,10 +111,10 @@ def delete(cls, text: str, *, paragraph: str, occurrence: int | None = None) -> Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, or ``text`` is empty. + negative, or ``text`` is not a non-empty string. """ cls._validate_common("delete", paragraph, occurrence) - if not text: + if not isinstance(text, str) or not text: raise ValueError("EditOperation.delete(): 'text' must be a non-empty string — the text to mark as deleted") return cls(action="delete", paragraph=paragraph, text=text, occurrence=occurrence) @@ -127,9 +128,9 @@ def _insert( occurrence: int | None, ) -> "EditOperation": cls._validate_common(action, paragraph, occurrence) - if not anchor: + if not isinstance(anchor, str) or not anchor: raise ValueError(f"EditOperation.{action}(): 'anchor' must be a non-empty string — the text to insert near") - if text is None: + if not isinstance(text, str): raise ValueError(f"EditOperation.{action}(): 'text' must be a string (empty string is allowed)") return cls(action=action, paragraph=paragraph, anchor=anchor, text=text, occurrence=occurrence) @@ -148,7 +149,8 @@ def insert_after(cls, anchor: str, text: str, *, paragraph: str, occurrence: int Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``anchor`` is empty, or ``text`` is None. + negative, ``anchor`` is not a non-empty string, or ``text`` + is not a string. """ return cls._insert("insert_after", anchor, text, paragraph, occurrence) @@ -167,7 +169,8 @@ def insert_before(cls, anchor: str, text: str, *, paragraph: str, occurrence: in Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``anchor`` is empty, or ``text`` is None. + negative, ``anchor`` is not a non-empty string, or ``text`` + is not a string. """ return cls._insert("insert_before", anchor, text, paragraph, occurrence) @@ -617,8 +620,8 @@ def _locate_in_paragraph(self, paragraph, paragraph_ref: str, text: str, occurre be unique within the paragraph. Raises: - ValueError: If ``occurrence`` is negative, or ``text`` is empty - or None. + ValueError: If ``occurrence`` is negative, or ``text`` is not a + non-empty string. TextNotFoundError: If the text is absent, or ``occurrence`` is out of range (then with ``occurrence``/``total_occurrences`` set). AmbiguousTextError: If ``occurrence`` is None and the text matches @@ -626,7 +629,7 @@ def _locate_in_paragraph(self, paragraph, paragraph_ref: str, text: str, occurre """ if occurrence is not None and occurrence < 0: raise ValueError(f"occurrence must be >= 0, got {occurrence}") - if not text: + if not isinstance(text, str) or not text: raise ValueError(f"search text must be a non-empty string, got {text!r}") text_map = build_text_map(paragraph) total = count_in_text_map(text_map, text) @@ -737,22 +740,23 @@ def _resolve_action_target(self, op: EditOperation) -> str: Raises: ValueError: If ``occurrence`` is negative, required arguments for - op.action are missing, or the action is unrecognized. + op.action are missing or not strings, or the action is + unrecognized. """ if op.occurrence is not None and op.occurrence < 0: raise ValueError(f"occurrence must be >= 0, got {op.occurrence}") if op.action == "replace": - if not op.find or op.replace_with is None: - raise ValueError("replace requires 'find' and 'replace_with'") + if not op.find or not isinstance(op.replace_with, str): + raise ValueError("replace requires 'find' and a string 'replace_with'") return op.find elif op.action == "delete": if not op.text: raise ValueError("delete requires 'text'") return op.text elif op.action in ("insert_after", "insert_before"): - if not op.anchor or op.text is None: - raise ValueError(f"{op.action} requires 'anchor' and 'text'") + if not op.anchor or not isinstance(op.text, str): + raise ValueError(f"{op.action} requires 'anchor' and a string 'text'") return op.anchor else: raise ValueError(f"Unknown action: {op.action}") @@ -850,7 +854,7 @@ def _validate_single(self, op: EditOperation) -> str | None: try: self._locate_in_paragraph(p, op.paragraph, target, op.occurrence) - except DocxEditError as e: + except (ValueError, DocxEditError) as e: return str(e) return None @@ -1144,8 +1148,8 @@ def _locate_document_wide(self, text: str, occurrence: int | None = None) -> Tex the exact total is part of the error contract. Raises: - ValueError: If ``occurrence`` is negative, or ``text`` is empty - or None. + ValueError: If ``occurrence`` is negative, or ``text`` is not a + non-empty string. TextNotFoundError: If the text is not found or occurrence doesn't exist; ``total_occurrences`` matches :meth:`count_matches`. AmbiguousTextError: If ``occurrence`` is None and the text matches @@ -1153,7 +1157,7 @@ def _locate_document_wide(self, text: str, occurrence: int | None = None) -> Tex """ if occurrence is not None and occurrence < 0: raise ValueError(f"occurrence must be >= 0, got {occurrence}") - if not text: + if not isinstance(text, str) or not text: raise ValueError(f"search text must be a non-empty string, got {text!r}") occ = occurrence if occurrence is not None else 0 match = self._find_across_boundaries(text, occ) @@ -1294,13 +1298,15 @@ def replace_text( The change ID of the insertion Raises: - ValueError: If ``find`` is empty or None, or ``occurrence`` is - negative + ValueError: If ``find`` is not a non-empty string, ``replace_with`` + is not a string, or ``occurrence`` is negative TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``find`` matches more than once in the search scope HashMismatchError: If the paragraph hash doesn't match """ + if not isinstance(replace_with, str): + raise ValueError(f"'replace_with' must be a string (empty string is allowed), got {replace_with!r}") with self._grouped(): if paragraph is not None: ref = ParagraphRef.parse(paragraph) @@ -1325,8 +1331,8 @@ def suggest_deletion(self, text: str, occurrence: int | None = None, paragraph: The change ID of the deletion Raises: - ValueError: If ``text`` is empty or None, or ``occurrence`` is - negative + ValueError: If ``text`` is not a non-empty string, or + ``occurrence`` is negative TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``text`` matches more than once in the search scope @@ -2162,8 +2168,8 @@ def insert_text_after( The change ID of the insertion Raises: - ValueError: If ``anchor`` is empty or None, or ``occurrence`` is - negative + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, or ``occurrence`` is negative TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope @@ -2188,8 +2194,8 @@ def insert_text_before( The change ID of the insertion Raises: - ValueError: If ``anchor`` is empty or None, or ``occurrence`` is - negative + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, or ``occurrence`` is negative TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope @@ -2206,6 +2212,8 @@ def _insert_text( paragraph: str | None = None, ) -> int: """Insert text before or after anchor with tracked changes.""" + if not isinstance(text, str): + raise ValueError(f"'text' must be a string (empty string is allowed), got {text!r}") with self._grouped(): if paragraph is not None: ref = ParagraphRef.parse(paragraph) diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index a855d1d..063c2f1 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -303,7 +303,7 @@ doc.close() **Multi-author documents:** Editing inside *another* author's pending insertion preserves their proposal, matching Word: deletions nest a `` under your authorship inside their ``, and replacements/insertions put your text in your own sibling `` (splitting theirs when needed) instead of silently rewriting it. `accept_all(author=...)` / `reject_all(author=...)` then resolve each author's changes independently. Only your own pending insertions are edited in place. -**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. `ValueError` for empty or missing search/anchor text, a non-string `paragraph` ref, or a negative `occurrence` — all rejected up front, before any change is made. +**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. `ValueError` for search/anchor text that is not a non-empty string, replacement/insertion text that is not a string, a non-string `paragraph` ref, or a negative `occurrence` — all rejected up front, before any change is made. ### Comments API diff --git a/tests/test_batch_edit.py b/tests/test_batch_edit.py index 001aa46..a54f166 100644 --- a/tests/test_batch_edit.py +++ b/tests/test_batch_edit.py @@ -1062,6 +1062,21 @@ def test_non_string_paragraph_ref_names_the_type(self): with pytest.raises(ValueError, match="expected a string like 'P3#a7b2', got int"): EditOperation.insert_after("a", "x", paragraph=3) # type: ignore[arg-type] + def test_non_string_search_text_rejected(self): + """A truthy non-string target fails at construction, not deep in the search.""" + with pytest.raises(ValueError, match=r"'find' must be a non-empty string"): + EditOperation.replace(123, "x", paragraph="P2#f3c1") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"'text' must be a non-empty string"): + EditOperation.delete(123, paragraph="P2#f3c1") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"'anchor' must be a non-empty string"): + EditOperation.insert_after(123, "x", paragraph="P2#f3c1") # type: ignore[arg-type] + + def test_non_string_payload_rejected(self): + with pytest.raises(ValueError, match=r"'replace_with' must be a string"): + EditOperation.replace("a", 123, paragraph="P2#f3c1") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"'text' must be a string"): + EditOperation.insert_before("a", 123, paragraph="P2#f3c1") # type: ignore[arg-type] + def test_negative_occurrence_rejected(self): with pytest.raises(ValueError, match=r"EditOperation\.replace\(\): occurrence must be >= 0, got -1"): EditOperation.replace("a", "b", paragraph="P2#f3c1", occurrence=-1) diff --git a/tests/test_error_quality.py b/tests/test_error_quality.py index ea53bf2..3a48042 100644 --- a/tests/test_error_quality.py +++ b/tests/test_error_quality.py @@ -562,6 +562,110 @@ def test_all_edit_methods_reject_none(self, doc_path): doc.close() +class TestNonStringSearchAndPayloadRejected: + """ + Before: a truthy non-string search target (``find=123``) slipped past + the falsiness checks into the text-map search and surfaced as a raw + ``TypeError`` from ``str.find`` — escaping both documented batch + contracts (``batch_edit`` raises only BatchOperationError; + ``validate_batch`` never raises). A non-string payload + (``replace_with=123``) was worse: dry-run reported the op as *valid*, + then apply crashed with a raw ``AttributeError``. + + After: search/anchor text must be a non-empty ``str`` and payloads must + be ``str`` at every boundary — direct API, typed constructors, batch + apply (wrapped with the op index), and dry-run (invalid row, no raise). + """ + + def test_direct_api_rejects_non_string_search_text(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="search text must be a non-empty string, got 123"): + doc.replace(123, "x", paragraph=ref) # type: ignore[arg-type] + with pytest.raises(ValueError, match="search text must be a non-empty string, got b'needle'"): + doc.delete(b"needle", paragraph=ref) # type: ignore[arg-type] + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_direct_api_rejects_non_string_payload(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="'replace_with' must be a string"): + doc.replace("needle", 123, paragraph=ref) # type: ignore[arg-type] + with pytest.raises(ValueError, match="'text' must be a string"): + doc.insert_after("needle", 123, paragraph=ref) # type: ignore[arg-type] + with pytest.raises(ValueError, match="'text' must be a string"): + doc.insert_before("needle", 123, paragraph=ref) # type: ignore[arg-type] + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_batch_wraps_non_string_target_not_raw_typeerror(self, doc_path): + """Hand-built op (bypassing the typed constructors) with an int find.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + before = doc.get_visible_text() + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find=123, replace_with="x") # type: ignore[arg-type] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([op]) + + assert exc.value.operation_index == 0 + assert isinstance(exc.value.original, ValueError) + assert "search text must be a non-empty string" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_batch_wraps_non_string_payload_not_raw_attributeerror(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + before = doc.get_visible_text() + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with=123) # type: ignore[arg-type] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([op]) + + assert exc.value.operation_index == 0 + assert "a string 'replace_with'" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_dry_run_reports_non_string_target_as_invalid(self, doc_path): + """validate_batch's never-raises contract holds for an int find.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find=123, replace_with="x") # type: ignore[arg-type] + results = doc.batch_edit([op], dry_run=True) + + assert len(results) == 1 + assert not results[0].valid + assert results[0].error is not None + assert "search text must be a non-empty string" in results[0].error + finally: + doc.close() + + def test_dry_run_reports_non_string_payload_as_invalid(self, doc_path): + """Before the fix this came back valid=True, then crashed at apply.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with=123) # type: ignore[arg-type] + results = doc.batch_edit([op], dry_run=True) + + assert len(results) == 1 + assert not results[0].valid + assert results[0].error is not None + assert "a string 'replace_with'" in results[0].error + finally: + doc.close() + + class TestSharedBaseClass: """All structured errors inherit from `DocxEditError`. From 8992d0adbbc4261a66ce452de825908826f21092 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 17 Jul 2026 00:27:46 +0200 Subject: [PATCH 3/6] fix: harden occurrence type validation; polish constructor messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Iteration-2 review found occurrence was the one input the type-hardening pass skipped: occurrence="0" hit `"0" < 0` and raised a raw TypeError on every path (constructors, direct API, batch apply, dry-run, add_comment); a float passed the < 0 check and TypeErrored deeper in the search; a bool was silently misread as occurrence 0/1. New shared guard _require_valid_occurrence in xml_editor.py (home of the text-map search both callers already import from) rejects non-int and negative occurrences with ValueError, called from _validate_common, _locate_in_paragraph, _resolve_action_target, _locate_document_wide, and comments._locate_anchor. The negative-int message keeps its exact pinned wording; the type check fires first. Also from review triage (LOW polish): - EditOperation constructor messages now include the offending value (got {value!r}), matching the convention everywhere else in the diff - _not_an_edit_operation renamed to _not_an_edit_operation_message — it builds a message string, not a predicate 5 new tests (TDD-first) covering constructor/direct/batch/dry-run/ add_comment paths; docstrings and SKILL.md updated. --- docx_editor/comments.py | 8 ++-- docx_editor/document.py | 8 ++-- docx_editor/track_changes.py | 70 ++++++++++++++++++++--------------- docx_editor/xml_editor.py | 12 ++++++ skills/docx/SKILL.md | 2 +- tests/test_error_quality.py | 71 ++++++++++++++++++++++++++++++++++++ 6 files changed, 132 insertions(+), 39 deletions(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 8080b3d..7208ecd 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -24,6 +24,7 @@ TextMapMatch, _escape_xml, _generate_hex_id, + _require_valid_occurrence, build_text_map, compute_paragraph_hash, count_in_text_map, @@ -190,11 +191,10 @@ def _locate_anchor( ``_locate_document_wide`` so comment anchors and edit anchors find the same text and fail the same way: ``occurrence=None`` requires a unique anchor (else AmbiguousTextError), an explicit out-of-range occurrence - reports the actual total, and a negative occurrence is rejected with - ValueError. + reports the actual total, and a negative or non-int occurrence is + rejected with ValueError. """ - if occurrence is not None and occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence) occ = occurrence if occurrence is not None else 0 if paragraph is not None: diff --git a/docx_editor/document.py b/docx_editor/document.py index 18705cb..d1efd56 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -735,7 +735,7 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i Raises: ValueError: If ``find`` is not a non-empty string, ``replace_with`` is not a string, ``paragraph`` is not a ref - string, or ``occurrence`` is negative. + string, or ``occurrence`` is negative or not an integer. TextNotFoundError: If ``find`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``find`` @@ -768,7 +768,7 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> Raises: ValueError: If ``text`` is not a non-empty string, ``paragraph`` - is not a ref string, or ``occurrence`` is negative. + is not a ref string, or ``occurrence`` is negative or not an integer. TextNotFoundError: If ``text`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``text`` @@ -802,7 +802,7 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in Raises: ValueError: If ``anchor`` is not a non-empty string, ``text`` is not a string, ``paragraph`` is not a ref string, or - ``occurrence`` is negative. + ``occurrence`` is negative or not an integer. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -836,7 +836,7 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i Raises: ValueError: If ``anchor`` is not a non-empty string, ``text`` is not a string, ``paragraph`` is not a ref string, or - ``occurrence`` is negative. + ``occurrence`` is negative or not an integer. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index 263ad07..ba59284 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -29,6 +29,7 @@ TextMapMatch, TextPosition, _escape_xml, + _require_valid_occurrence, build_text_map, compute_paragraph_hash, compute_text_hash, @@ -64,8 +65,7 @@ class EditOperation: def _validate_common(constructor: str, paragraph: str, occurrence: int | None) -> None: """Construction-time checks shared by all typed constructors.""" ParagraphRef.parse(paragraph) - if occurrence is not None and occurrence < 0: - raise ValueError(f"EditOperation.{constructor}(): occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence, f"EditOperation.{constructor}(): ") @classmethod def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: int | None = None) -> "EditOperation": @@ -82,14 +82,19 @@ def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: in Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``find`` is not a non-empty string, or - ``replace_with`` is not a string. + not a non-negative integer, ``find`` is not a non-empty + string, or ``replace_with`` is not a string. """ cls._validate_common("replace", paragraph, occurrence) if not isinstance(find, str) or not find: - raise ValueError("EditOperation.replace(): 'find' must be a non-empty string — the text to search for") + raise ValueError( + f"EditOperation.replace(): 'find' must be a non-empty string — the text to search for, got {find!r}" + ) if not isinstance(replace_with, str): - raise ValueError("EditOperation.replace(): 'replace_with' must be a string (empty string is allowed)") + raise ValueError( + f"EditOperation.replace(): 'replace_with' must be a string (empty string is allowed), " + f"got {replace_with!r}" + ) return cls( action="replace", paragraph=paragraph, @@ -111,11 +116,14 @@ def delete(cls, text: str, *, paragraph: str, occurrence: int | None = None) -> Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, or ``text`` is not a non-empty string. + not a non-negative integer, or ``text`` is not a non-empty + string. """ cls._validate_common("delete", paragraph, occurrence) if not isinstance(text, str) or not text: - raise ValueError("EditOperation.delete(): 'text' must be a non-empty string — the text to mark as deleted") + raise ValueError( + f"EditOperation.delete(): 'text' must be a non-empty string — the text to mark as deleted, got {text!r}" + ) return cls(action="delete", paragraph=paragraph, text=text, occurrence=occurrence) @classmethod @@ -129,9 +137,14 @@ def _insert( ) -> "EditOperation": cls._validate_common(action, paragraph, occurrence) if not isinstance(anchor, str) or not anchor: - raise ValueError(f"EditOperation.{action}(): 'anchor' must be a non-empty string — the text to insert near") + raise ValueError( + f"EditOperation.{action}(): 'anchor' must be a non-empty string — the text to insert near, " + f"got {anchor!r}" + ) if not isinstance(text, str): - raise ValueError(f"EditOperation.{action}(): 'text' must be a string (empty string is allowed)") + raise ValueError( + f"EditOperation.{action}(): 'text' must be a string (empty string is allowed), got {text!r}" + ) return cls(action=action, paragraph=paragraph, anchor=anchor, text=text, occurrence=occurrence) @classmethod @@ -149,8 +162,8 @@ def insert_after(cls, anchor: str, text: str, *, paragraph: str, occurrence: int Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``anchor`` is not a non-empty string, or ``text`` - is not a string. + not a non-negative integer, ``anchor`` is not a non-empty + string, or ``text`` is not a string. """ return cls._insert("insert_after", anchor, text, paragraph, occurrence) @@ -169,13 +182,13 @@ def insert_before(cls, anchor: str, text: str, *, paragraph: str, occurrence: in Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``anchor`` is not a non-empty string, or ``text`` - is not a string. + not a non-negative integer, ``anchor`` is not a non-empty + string, or ``text`` is not a string. """ return cls._insert("insert_before", anchor, text, paragraph, occurrence) -def _not_an_edit_operation(op: object) -> str: +def _not_an_edit_operation_message(op: object) -> str: """Shared batch_edit/validate_batch message for a non-EditOperation element, so the raising and never-raises paths cannot drift apart.""" return ( @@ -620,15 +633,14 @@ def _locate_in_paragraph(self, paragraph, paragraph_ref: str, text: str, occurre be unique within the paragraph. Raises: - ValueError: If ``occurrence`` is negative, or ``text`` is not a + ValueError: If ``occurrence`` is negative or not an integer, or ``text`` is not a non-empty string. TextNotFoundError: If the text is absent, or ``occurrence`` is out of range (then with ``occurrence``/``total_occurrences`` set). AmbiguousTextError: If ``occurrence`` is None and the text matches more than once in the paragraph. """ - if occurrence is not None and occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence) if not isinstance(text, str) or not text: raise ValueError(f"search text must be a non-empty string, got {text!r}") text_map = build_text_map(paragraph) @@ -687,7 +699,7 @@ def batch_edit(self, operations: list[EditOperation]) -> list[int]: parsed: list[tuple[int, ParagraphRef, EditOperation]] = [] for i, op in enumerate(operations): if not isinstance(op, EditOperation): - raise BatchOperationError(i, _not_an_edit_operation(op)) + raise BatchOperationError(i, _not_an_edit_operation_message(op)) if not op.paragraph: raise BatchOperationError(i, "paragraph reference is required for batch mode") try: @@ -739,12 +751,11 @@ def _resolve_action_target(self, op: EditOperation) -> str: so both paths fail cleanly before the search. Raises: - ValueError: If ``occurrence`` is negative, required arguments for + ValueError: If ``occurrence`` is negative or not an integer, required arguments for op.action are missing or not strings, or the action is unrecognized. """ - if op.occurrence is not None and op.occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {op.occurrence}") + _require_valid_occurrence(op.occurrence) if op.action == "replace": if not op.find or not isinstance(op.replace_with, str): @@ -807,7 +818,7 @@ def validate_batch(self, operations: list[EditOperation]) -> list[EditValidation for i, op in enumerate(operations): if not isinstance(op, EditOperation): results.append( - EditValidationResult(index=i, paragraph=None, valid=False, error=_not_an_edit_operation(op)) + EditValidationResult(index=i, paragraph=None, valid=False, error=_not_an_edit_operation_message(op)) ) continue error = self._validate_single(op) @@ -1148,15 +1159,14 @@ def _locate_document_wide(self, text: str, occurrence: int | None = None) -> Tex the exact total is part of the error contract. Raises: - ValueError: If ``occurrence`` is negative, or ``text`` is not a + ValueError: If ``occurrence`` is negative or not an integer, or ``text`` is not a non-empty string. TextNotFoundError: If the text is not found or occurrence doesn't exist; ``total_occurrences`` matches :meth:`count_matches`. AmbiguousTextError: If ``occurrence`` is None and the text matches more than once in the document. """ - if occurrence is not None and occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence) if not isinstance(text, str) or not text: raise ValueError(f"search text must be a non-empty string, got {text!r}") occ = occurrence if occurrence is not None else 0 @@ -1299,7 +1309,7 @@ def replace_text( Raises: ValueError: If ``find`` is not a non-empty string, ``replace_with`` - is not a string, or ``occurrence`` is negative + is not a string, or ``occurrence`` is negative or not an integer TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``find`` matches more than once in the search scope @@ -1332,7 +1342,7 @@ def suggest_deletion(self, text: str, occurrence: int | None = None, paragraph: Raises: ValueError: If ``text`` is not a non-empty string, or - ``occurrence`` is negative + ``occurrence`` is negative or not an integer TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``text`` matches more than once in the search scope @@ -2169,7 +2179,7 @@ def insert_text_after( Raises: ValueError: If ``anchor`` is not a non-empty string, ``text`` is - not a string, or ``occurrence`` is negative + not a string, or ``occurrence`` is negative or not an integer TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope @@ -2195,7 +2205,7 @@ def insert_text_before( Raises: ValueError: If ``anchor`` is not a non-empty string, ``text`` is - not a string, or ``occurrence`` is negative + not a string, or ``occurrence`` is negative or not an integer TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index 7369e0d..5ceadcc 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -60,6 +60,18 @@ class TextMapMatch: spans_boundary: bool # True if match spans different revision contexts +def _require_valid_occurrence(occurrence: int | None, label: str = "") -> None: + """Reject a non-int or negative ``occurrence`` before it can hit an int + comparison (raw TypeError from ``"0" < 0``), TypeError deeper in the + search (float), or be silently misread as 0/1 (bool).""" + if occurrence is None: + return + if isinstance(occurrence, bool) or not isinstance(occurrence, int): + raise ValueError(f"{label}occurrence must be a non-negative integer, got {occurrence!r}") + if occurrence < 0: + raise ValueError(f"{label}occurrence must be >= 0, got {occurrence}") + + def find_in_text_map(text_map: TextMap, search: str, occurrence: int = 0) -> TextMapMatch | None: """Find the nth occurrence of text in a text map. diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index 063c2f1..d8424b4 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -303,7 +303,7 @@ doc.close() **Multi-author documents:** Editing inside *another* author's pending insertion preserves their proposal, matching Word: deletions nest a `` under your authorship inside their ``, and replacements/insertions put your text in your own sibling `` (splitting theirs when needed) instead of silently rewriting it. `accept_all(author=...)` / `reject_all(author=...)` then resolve each author's changes independently. Only your own pending insertions are edited in place. -**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. `ValueError` for search/anchor text that is not a non-empty string, replacement/insertion text that is not a string, a non-string `paragraph` ref, or a negative `occurrence` — all rejected up front, before any change is made. +**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. `ValueError` for search/anchor text that is not a non-empty string, replacement/insertion text that is not a string, a non-string `paragraph` ref, or an `occurrence` that is not a non-negative integer — all rejected up front, before any change is made. ### Comments API diff --git a/tests/test_error_quality.py b/tests/test_error_quality.py index 3a48042..3ad71be 100644 --- a/tests/test_error_quality.py +++ b/tests/test_error_quality.py @@ -562,6 +562,77 @@ def test_all_edit_methods_reject_none(self, doc_path): doc.close() +class TestNonIntOccurrenceRejected: + """ + Before: ``occurrence`` was the one input the type-hardening pass skipped. + ``occurrence="0"`` hit ``"0" < 0`` → raw TypeError on every path + (constructors, direct API, batch apply, dry-run, add_comment); + ``occurrence=1.5`` passed the ``< 0`` check and TypeErrored deeper in + the search; ``occurrence=True`` was silently misread as occurrence 1. + + After: a shared guard rejects non-int (including bool) occurrences with + the same ValueError at every boundary, before any comparison or search. + """ + + def test_direct_api_rejects_str_occurrence(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="occurrence must be a non-negative integer, got '0'"): + doc.replace("needle", "x", paragraph=ref, occurrence="0") # type: ignore[arg-type] + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_constructor_rejects_float_and_bool(self): + with pytest.raises( + ValueError, match=r"EditOperation\.replace\(\): occurrence must be a non-negative integer, got 1\.5" + ): + EditOperation.replace("a", "b", paragraph="P2#f3c1", occurrence=1.5) # type: ignore[arg-type] + with pytest.raises( + ValueError, match=r"EditOperation\.delete\(\): occurrence must be a non-negative integer, got True" + ): + EditOperation.delete("a", paragraph="P2#f3c1", occurrence=True) # type: ignore[arg-type] + + def test_batch_wraps_str_occurrence_not_raw_typeerror(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + before = doc.get_visible_text() + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with="x", occurrence="0") # type: ignore[arg-type] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([op]) + + assert exc.value.operation_index == 0 + assert "occurrence must be a non-negative integer" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_dry_run_reports_str_occurrence_as_invalid(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with="x", occurrence="0") # type: ignore[arg-type] + results = doc.batch_edit([op], dry_run=True) + + assert len(results) == 1 + assert not results[0].valid + assert results[0].error is not None + assert "occurrence must be a non-negative integer" in results[0].error + finally: + doc.close() + + def test_add_comment_rejects_str_occurrence(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="occurrence must be a non-negative integer, got '0'"): + doc.add_comment("needle", "note", paragraph=ref, occurrence="0") # type: ignore[arg-type] + finally: + doc.close() + + class TestNonStringSearchAndPayloadRejected: """ Before: a truthy non-string search target (``find=123``) slipped past From 8e4c3a3db9570407ece7f0a62ec62129703faaf9 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 17 Jul 2026 00:49:13 +0200 Subject: [PATCH 4/6] fix: add_comment rejects non-string anchors with CommentError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Iteration-3 review found the one sibling anchor-search path the earlier type-hardening missed: add_comment's guard was falsiness-only, so a truthy non-string anchor (123, b"needle") passed it and raised a raw TypeError from str.find inside the text-map search — violating the documented contract (CommentError for bad anchors). The guard now requires a non-empty str and names the offending value, keeping add_comment's established CommentError type. Test extended to pin int and bytes anchors in both scoped and document-wide modes. --- docx_editor/comments.py | 6 +++--- tests/test_error_quality.py | 11 ++++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 7208ecd..738be97 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -156,10 +156,10 @@ def add_comment( ``anchor_text`` matches more than once in the search scope. HashMismatchError: If a paragraph reference's hash is stale. ParagraphIndexError: If a paragraph reference's index is out of range. - CommentError: If ``anchor_text`` is empty. + CommentError: If ``anchor_text`` is not a non-empty string. """ - if not anchor_text: - raise CommentError("anchor_text must not be empty") + if not isinstance(anchor_text, str) or not anchor_text: + raise CommentError(f"anchor_text must be a non-empty string, got {anchor_text!r}") _, match = self._locate_anchor(anchor_text, paragraph, occurrence) comment_id = self.next_comment_id diff --git a/tests/test_error_quality.py b/tests/test_error_quality.py index 3ad71be..cf20aec 100644 --- a/tests/test_error_quality.py +++ b/tests/test_error_quality.py @@ -521,15 +521,20 @@ def test_none_search_text_names_the_value(self, doc_path): finally: doc.close() - def test_add_comment_still_rejects_empty_anchor_its_own_way(self, doc_path): - """add_comment pre-rejects empty anchors with CommentError, not ValueError.""" + def test_add_comment_still_rejects_bad_anchors_its_own_way(self, doc_path): + """add_comment pre-rejects empty AND non-string anchors with CommentError, + not ValueError (its documented type) — and never a raw TypeError.""" from docx_editor import CommentError doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) try: ref = doc.list_paragraphs()[0].split("|")[0] - with pytest.raises(CommentError, match="anchor_text must not be empty"): + with pytest.raises(CommentError, match="anchor_text must be a non-empty string, got ''"): doc.add_comment("", "note", paragraph=ref) + with pytest.raises(CommentError, match="anchor_text must be a non-empty string, got 123"): + doc.add_comment(123, "note", paragraph=ref) # type: ignore[arg-type] + with pytest.raises(CommentError, match="anchor_text must be a non-empty string, got b'needle'"): + doc.add_comment(b"needle", "note") # type: ignore[arg-type] finally: doc.close() From 3814b1297f7378845fec2dfc5635896f9932fa70 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 17 Jul 2026 01:11:02 +0200 Subject: [PATCH 5/6] fix: add_comment rejects non-string comment_text before mutating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final review pass found the sibling of the anchor guard: a non-string comment_text passed anchor validation and _locate_anchor, then crashed in html.escape with a raw AttributeError — after _place_markers had already mutated document.xml, leaving orphaned commentRangeStart/End markers (partial corruption, not a clean no-op). The guard now fires before any mutation, keeping add_comment's documented CommentError type. Test asserts both the error and that no range markers were placed. Known follow-up (untouched by this PR): reply_to_comment has the same non-string payload gap via the shared _add_to_comments_xml path. --- docx_editor/comments.py | 7 ++++++- tests/test_error_quality.py | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 738be97..929c123 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -156,10 +156,15 @@ def add_comment( ``anchor_text`` matches more than once in the search scope. HashMismatchError: If a paragraph reference's hash is stale. ParagraphIndexError: If a paragraph reference's index is out of range. - CommentError: If ``anchor_text`` is not a non-empty string. + CommentError: If ``anchor_text`` is not a non-empty string, or + ``comment_text`` is not a string. """ if not isinstance(anchor_text, str) or not anchor_text: raise CommentError(f"anchor_text must be a non-empty string, got {anchor_text!r}") + if not isinstance(comment_text, str): + # Must fail here, before _place_markers mutates document.xml — + # a crash later (html.escape) would leave orphaned range markers. + raise CommentError(f"comment_text must be a string, got {comment_text!r}") _, match = self._locate_anchor(anchor_text, paragraph, occurrence) comment_id = self.next_comment_id diff --git a/tests/test_error_quality.py b/tests/test_error_quality.py index cf20aec..3f644db 100644 --- a/tests/test_error_quality.py +++ b/tests/test_error_quality.py @@ -538,6 +538,22 @@ def test_add_comment_still_rejects_bad_anchors_its_own_way(self, doc_path): finally: doc.close() + def test_add_comment_rejects_non_string_comment_before_mutating(self, doc_path): + """Before: comment_text=123 crashed in html.escape AFTER the range + markers were placed — orphaned commentRangeStart/End corruption. + After: rejected up front with CommentError, document untouched.""" + from docx_editor import CommentError + + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(CommentError, match="comment_text must be a string, got 123"): + doc.add_comment("needle", 123, paragraph=ref) # type: ignore[arg-type] + assert doc.list_comments() == [] + assert "commentRangeStart" not in doc._document_editor.dom.toxml() + finally: + doc.close() + class TestNonStringParagraphRefRejected: """ From 5624e61019e6bece5a565bcb5b7b8619bc868b96 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 17 Jul 2026 01:20:47 +0200 Subject: [PATCH 6/6] docs: complete add_comment Raises contract (CommentError, ValueError) --- docx_editor/comments.py | 1 + docx_editor/document.py | 3 +++ 2 files changed, 4 insertions(+) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 929c123..e889155 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -158,6 +158,7 @@ def add_comment( ParagraphIndexError: If a paragraph reference's index is out of range. CommentError: If ``anchor_text`` is not a non-empty string, or ``comment_text`` is not a string. + ValueError: If ``occurrence`` is negative or not an integer. """ if not isinstance(anchor_text, str) or not anchor_text: raise CommentError(f"anchor_text must be a non-empty string, got {anchor_text!r}") diff --git a/docx_editor/document.py b/docx_editor/document.py index d1efd56..5738d87 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -1013,6 +1013,9 @@ def add_comment( AmbiguousTextError: If ``occurrence`` is omitted and ``anchor_text`` matches more than once in the search scope. HashMismatchError: If ``paragraph``'s hash is stale. + CommentError: If ``anchor_text`` is not a non-empty string, or + ``comment`` is not a string. + ValueError: If ``occurrence`` is negative or not an integer. Example: doc.add_comment("Section 5", "Please review this section")