fix: input validation — empty search text, batch element types, non-string refs (ISSUES.md #40)#57
Conversation
…tring refs (ISSUES.md #40) 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.
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
=====================================
Coverage 95.3% 95.3%
=====================================
Files 11 11
Lines 3576 3596 +20
Branches 719 725 +6
=====================================
+ Hits 3410 3430 +20
Misses 79 79
Partials 87 87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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).
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.
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.
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.
…alidation-bugs--empty-n # Conflicts: # skills/docx/SKILL.md
1 participant
Fixes the three input-validation bugs tracked as ISSUES.md #40 (repo-local issue file, not a GitHub issue number), targeting 0.6.1 — plus the wider type-hardening the multi-reviewer loop surfaced on top of them.
Bugs fixed (original scope)
Empty search text silently vanished edits. An empty needle with an explicit
occurrenceproduced a zero-width match with no DOM positions, so the apply helpers created zero revisions and the edit was silently dropped; withoutoccurrenceit raised a meaninglessAmbiguousTextError. Both shared locate helpers now reject it withValueErrorbefore any change is made.Non-EditOperation batch elements crashed with raw
AttributeError.batch_editnow raisesBatchOperationErrorcarrying the offendingoperation_index;validate_batchkeeps its never-raises contract and reports the element as an invalid row.Non-string paragraph refs leaked raw
TypeError.ParagraphRef.parserejects non-strings before the regex; the fourDocumentedit methods reject non-stringparagraphup front (paragraph=Nonepreviously fell through to the document-wide search branch silently).Review-loop hardening (found by the multi-reviewer loop, same bug class)
Truthy non-string search text and payloads (
find=123,replace_with=123,b"needle") escaped the falsiness checks and surfaced as rawTypeError/AttributeError, breaking the documented batch contracts; dry-run even reported such ops as valid. Now rejected withValueErrorat every boundary (constructors, direct API, batch apply wrapped with op index, dry-run invalid rows), and_validate_singlecatchesValueErrorlike the apply loop does.occurrencetype-leak.occurrence="0"hit"0" < 0→ rawTypeErroron every path incl.add_comment; a float TypeErrored deeper in the search; a bool was silently misread as 0/1. New shared_require_valid_occurrenceguard (xml_editor.py) used at all six boundaries; the negative-int message keeps its exact pinned wording.add_commentanchor and payload. A non-string anchor raised rawTypeError; worse, a non-stringcommentcrashed inhtml.escapeafter the range markers were placed, leaving orphanedcommentRangeStart/Endmarkers in document.xml. Both now rejected up front withCommentError, before any mutation.Notes
add_commentnever had bug 1 — it pre-rejects empty anchors with its ownCommentError; a test pins that asymmetry as deliberate.Raises:sections and the SKILL.md error-contract table updated throughout.reply_to_commenthas the same non-string payload gap via the shared_add_to_comments_xmlpath.Testing
27 new tests, all written TDD-first and observed failing on the buggy behaviors before each fix. Every fix verified end-to-end with live repro scripts (constructors, direct API, batch apply, dry-run, add_comment), including batch atomicity and the no-orphaned-markers invariant.
uv run pytest— 1084 passed, 2 skipped (pre-existing), re-run after every commituv run ruff check ./ruff format --check .— cleanuv run ty check— exit 0 (only the 6 pre-existing warnings in untouched code)Review
5-iteration multi-reviewer loop (3 Claude subagents + codex per round, opus red-team verification of every finding). Iteration 5 verdict: SHIP. One codex HIGH was refuted with a live repro (claimed a dry-run
ValueErrorescape via a nonexistent function; the actual code path returns invalid rows).