Repository navigation
feat: ergonomics grab-bag — SearchResult edit targets, ParagraphInfo structure, workspace reset (ISSUES.md #52 + #60) - #73
Merged
Conversation
…structure, workspace reset (ISSUES.md #52 + #60) Six code items plus two documentation items from the round-3/4 dogfood papercut list. - SearchResult stands in for (text, paragraph, occurrence) on replace, delete, insert_after, insert_before, add_comment and the four EditOperation constructors. Passing paragraph=/occurrence= alongside one raises rather than picking a winner. - ParagraphInfo gains in_table / style / outline_level, so a table of contents needs one list_paragraphs_structured() call instead of a second list_paragraph_locations() pass. One factory now builds the record for all three producers, and its single text map serves both hash and text. - EditValidationResult.current_ref gives dry-run stale-hash rows the ref for the paragraph's current content, so recovery no longer parses the error prose. - Document.discard_workspace() replaces the deep Workspace import (or a hand-deleted cache directory) for resetting after a crashed script. - ExecResult/EvalResult gain started, distinguishing a timeout of your own running code from one that never left the kernel queue; the CLI prints different advice for each. Exit codes unchanged. - replace() trims whitespace-only spans character-wise, so a spacing fix is a pure insertion or deletion instead of an invisible del+ins pair. Word spans are never char-trimmed. - Docs: pagination overshoot is documented as a valid empty answer (no behaviour change), and get_paragraph()/context() are marked O(document). Also fixes a fidelity bug this exposed: w:delText never carried xml:space="preserve", so a conforming consumer trimmed the edges and rejecting a whitespace deletion restored the wrong spacing. The editor's existing xml:space injection now covers w:delText as well as w:t. word/styles.xml is memoized per Document and parsed without the line-tracking parser, keeping get_paragraph() at its previous per-call cost now that it needs the style maps; the location APIs get the same win. 1406 passed, 2 skipped.
|
Warning Review limit reached
Next review available in: 51 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 Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds direct ChangesDocument editing and batch recovery
Paragraph and workspace behavior
Session execution reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Document
participant SearchResult
participant EditOperation
participant TrackChanges
Caller->>Document: find_text or find_all
Document-->>Caller: SearchResult
Caller->>Document: edit using SearchResult
Document->>TrackChanges: resolve and validate target
TrackChanges-->>Document: edit result
Document-->>Caller: completed operation
sequenceDiagram
participant CLI
participant Session
participant Kernel
CLI->>Session: submit exec request
Session->>Kernel: execute code
Kernel-->>Session: execute_input or timeout
Session-->>CLI: started flag and status
CLI-->>CLI: emit matching timeout guidance
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api.md`:
- Around line 1476-1480: Update the Document.discard_workspace reference in the
surrounding documentation to use the heading’s actual anchor fragment,
document-discard_workspace-path-workspace_dirnone, so the internal link resolves
correctly.
In `@docx_editor/session.py`:
- Line 562: Add direct assertions for EvalResult.started in the evaluation tests
covering successful execution, execution errors, and queued timeouts. Verify it
is true when evaluation starts running and false when a timeout occurs before
leaving the queue, alongside the existing ExecResult.started checks.
In `@docx_editor/track_changes.py`:
- Around line 431-478: Replace the bare tuple result of _resolve_search_target
with a named internal result object, following the existing _ValidationOutcome
pattern, while preserving the same text, paragraph, and occurrence values and
validation behavior. Update all eight callers in Document and EditOperation to
use the named fields instead of positional unpacking.
In `@tests/test_session.py`:
- Around line 135-136: In the hog execution setup around hog.execute, replace
the fixed one-second time.sleep with a bounded wait for the hog client to
receive the matching execute_input message. Synchronize on that kernel message
before sending the test request, while preserving the existing timeout behavior
and ensuring the request is confirmed started before asserting started=False.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1e16061-0ee1-4a28-bb9a-4701f9f35699
📒 Files selected for processing (16)
README.mddocs/api.mddocs/quickstart.mddocx_editor/document.pydocx_editor/session.pydocx_editor/track_changes.pydocx_editor/xml_editor.pyskills/docx/SKILL.mdtests/conftest.pytests/test_batch_edit.pytests/test_comments.pytests/test_error_quality.pytests/test_paragraph_location.pytests/test_session.pytests/test_track_changes.pytests/test_workspace.py
Comment on lines
+1476
to
+1480
| `Document.open(path, force_recreate=True)` and | ||
| [`Document.discard_workspace(path)`](#documentdiscard_workspacepath-workspace_dirnone) | ||
| both recover but discard the workspace's unsaved edits. To rescue them first, | ||
| save the orphaned workspace to a new file (`Workspace` is not exported at the | ||
| package root — use the deep import): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Extract the actual heading text and compare against the anchor used.
rg -n '#### `Document.discard_workspace' docs/api.md
rg -n 'documentdiscard_workspacepath' docs/api.mdRepository: pablospe/docx-editor
Length of output: 316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant surrounding lines:"
sed -n '33,43p' docs/api.md
sed -n '1474,1481p' docs/api.md
echo
echo "Markdown link anchor(s):"
python3 - <<'PY'
import re
text = open('docs/api.md', encoding='utf-8').read()
for m in re.finditer(r'(?P<prefix></[^>]+>")(?P<link>\[(?P<display>.*?)\]\((?P<fragment>#[^)\s]+)\))', text):
if 'discard_workspace' in m.group('link') or 'Document.discard' in m.group('display'):
print(f"line={text[:m.start()].count(chr(10))+1} display={m.group('display')!r} fragment={m.group('fragment')!r}")
PY
echo
echo "Headings containing discard_workspace:"
python3 - <<'PY'
import re
text = open('docs/api.md', encoding='utf-8').read()
for i, line in enumerate(text.splitlines(), 1):
if 'discard_workspace' in line:
print(f"{i}: {line}")
PYRepository: pablospe/docx-editor
Length of output: 1560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def anchor_from_heading(heading):
# Mirrors GitHub Markdown heading slugifier behavior for visible trailing
# punctuation/removal is not required by this concern; only normalize the
# provided heading characters to the claimed fragment form.
text = heading
# Common slug behavior: lowercased, whitespace/commas → hyphen, optional trailing
# punctuation stripped, collapsing multiple hyphens, no parentheses by default.
slug = ""
prev_hyphen = False
for ch in text[:100].lower():
if ch in " \t," or ch in ".,!?;:'\"()[]{}|\\*&^%$#~":
if not prev_hyphen:
slug += "-"
prev_hyphen = True
elif ch.isalnum() or ch == "_":
slug += ch
prev_hyphen = False
else:
if slug and not prev_hyphen:
prev_hyphen = True
return slug.strip("-")
heading = "`Document.discard_workspace(path, *, workspace_dir=None)`"
print("heading:", heading)
print("slug:", anchor_from_heading(heading))
print("link fragment:", "`#documentdiscard_workspacepath-workspace_dirnone`")
PY
echo
echo "Check whether Markdown/HTML processors resolve this heading and anchor, if available:"
python3 - <<'PY'
import subprocess, sys
programs = ["python3 -m markdown --extension mdx_tricky_sane_lists", "python3 -c 'import markdown; print(markdown.markdown(\"#### \`Document.discard_workspace(path, *, workspace_dir=None)\`\"))'"]
text = "#### `Document.discard_workspace(path, *, workspace_dir=None)`\n\n[link](`#documentdiscard_workspacepath-workspace_dirnone`)"
for p in programs:
print("command:", p)
try:
out = subprocess.run(p.split(), input=text.encode(), stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=20)
print("stdout:", out.stdout.decode(errors="replace").strip())
print("stderr:", out.stderr.decode(errors="replace").strip())
except Exception as e:
print("Error:", repr(e))
PYRepository: pablospe/docx-editor
Length of output: 800
Fix the discard_workspace anchor link.
The heading anchors as document-discard_workspace-path-workspace_dirnone, but the link uses #documentdiscard_workspacepath-workspace_dirnone, so this internal reference is broken. Update the fragment or add an explicit id on the heading.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 1477-1477: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/api.md` around lines 1476 - 1480, Update the Document.discard_workspace
reference in the surrounding documentation to use the heading’s actual anchor
fragment, document-discard_workspace-path-workspace_dirnone, so the internal
link resolves correctly.
Source: Linters/SAST tools
Comment on lines
+431
to
+478
| def _resolve_search_target( | ||
| target: "str | SearchResult", | ||
| paragraph: str | None, | ||
| occurrence: int | None, | ||
| *, | ||
| ctx: str, | ||
| arg: str, | ||
| ) -> tuple[str, str | None, int | None]: | ||
| """Normalize a text-or-SearchResult edit target into ``(text, paragraph, occurrence)``. | ||
|
|
||
| A plain string passes through untouched, so every existing call site keeps | ||
| its exact semantics. A :class:`SearchResult` supplies all three values — | ||
| its ``text``, its ``paragraph_ref`` and its ``paragraph_occurrence`` — | ||
| which is what makes ``doc.replace(match, "60 days")`` mean exactly what | ||
| spelling those three fields out by hand means. | ||
|
|
||
| Passing a SearchResult *and* ``paragraph=``/``occurrence=`` is a | ||
| contradiction rather than a merge (which of the two paragraphs wins?), so | ||
| it raises instead of silently preferring one. | ||
|
|
||
| Args: | ||
| target: Text to locate, or a SearchResult that already located it. | ||
| paragraph: The call's ``paragraph=`` argument (None when omitted). | ||
| occurrence: The call's ``occurrence=`` argument (None when omitted). | ||
| ctx: Message prefix naming the caller, e.g. ``"replace(): "``. | ||
| arg: Display name of the target parameter, e.g. ``"'find'"``. | ||
|
|
||
| Returns: | ||
| ``(text, paragraph, occurrence)``, for the caller to validate exactly | ||
| as it validates hand-written arguments. | ||
|
|
||
| Raises: | ||
| ValueError: If ``target`` is a SearchResult and ``paragraph`` or | ||
| ``occurrence`` was given too. | ||
| """ | ||
| if not isinstance(target, SearchResult): | ||
| return target, paragraph, occurrence | ||
| redundant = [ | ||
| name | ||
| for name, given in (("paragraph=", paragraph is not None), ("occurrence=", occurrence is not None)) | ||
| if given | ||
| ] | ||
| if redundant: | ||
| raise ValueError( | ||
| f"{ctx}{arg} is a SearchResult, which already pins the paragraph ({target.paragraph_ref!r}) " | ||
| f"and the occurrence ({target.paragraph_occurrence}) — drop {' and '.join(redundant)}" | ||
| ) | ||
| return target.text, target.paragraph_ref, target.paragraph_occurrence |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Return a named result instead of a bare 3-tuple.
_resolve_search_target returns tuple[str, str | None, int | None], unpacked identically at 8 call sites (Document.replace/delete/insert_after/insert_before/add_comment and EditOperation.replace/delete/_insert). As per coding guidelines, "Use named objects (dataclasses) instead of tuples for functions returning 3 or more values." _ValidationOutcome (added in this same PR, just below) already follows this convention for a similar internal result — worth applying the same pattern here so the meaning of each position isn't left to memorizing call-site order.
♻️ Proposed refactor
+@dataclass(frozen=True)
+class _ResolvedTarget:
+ text: str
+ paragraph: str | None
+ occurrence: int | None
+
+
def _resolve_search_target(
target: "str | SearchResult",
paragraph: str | None,
occurrence: int | None,
*,
ctx: str,
arg: str,
-) -> tuple[str, str | None, int | None]:
+) -> _ResolvedTarget:
...
if not isinstance(target, SearchResult):
- return target, paragraph, occurrence
+ return _ResolvedTarget(target, paragraph, occurrence)
redundant = [...]
if redundant:
raise ValueError(...)
- return target.text, target.paragraph_ref, target.paragraph_occurrence
+ return _ResolvedTarget(target.text, target.paragraph_ref, target.paragraph_occurrence)Call sites become e.g.:
- find, paragraph, occurrence = _resolve_search_target(
+ resolved = _resolve_search_target(
find, paragraph, occurrence, ctx="replace(): ", arg="'find'"
)
+ find, paragraph, occurrence = resolved.text, resolved.paragraph, resolved.occurrence🧰 Tools
🪛 Ruff (0.16.0)
[warning] 474-477: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docx_editor/track_changes.py` around lines 431 - 478, Replace the bare tuple
result of _resolve_search_target with a named internal result object, following
the existing _ValidationOutcome pattern, while preserving the same text,
paragraph, and occurrence values and validation behavior. Update all eight
callers in Document and EditOperation to use the named fields instead of
positional unpacking.
Source: Coding guidelines
Comment on lines
+135
to
+136
| hog.execute("import time; time.sleep(30)") # never waited on | ||
| time.sleep(1.0) # let the kernel pick it up and go busy |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching test_session.py:"
fd -a 'test_session\.py$' . || true
file="$(fd 'test_session\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "File: $file"
wc -l "$file"
echo
echo "Relevant lines:"
sed -n '80,180p' "$file" | nl -ba -v80
echo
echo "Search for hog/started/execute_input patterns:"
rg -n "hog|started|execute_input|get\(|poll|timeout" "$file" || true
fiRepository: pablospe/docx-editor
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also inspect nearby tests and imports to understand the execution model.
file="$(fd 'test_session\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "Top 80 lines:"
sed -n '1,90p' "$file" | nl -ba -v1
fiRepository: pablospe/docx-editor
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'test_session\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "File: $file"
wc -l "$file"
echo
echo "Relevant lines 80-180:"
sed -n '80,180p' "$file" | awk '{printf "%6d\t%s\n", NR+79, $0}'
echo
echo "Search for hog/started/execute_input patterns:"
rg -n "hog|started|execute_input|get\(|poll|timeout" "$file" || true
echo
echo "Top 80 lines:"
sed -n '1,80p' "$file" | awk '{printf "%6d\t%s\n", NR, $0}'
fiRepository: pablospe/docx-editor
Length of output: 9865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "session.py file candidate:"
fd -a 'session\.py$' . || true
file="$(fd 'session\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "File: $file"
wc -l "$file"
echo
echo "Outline:"
ast-grep outline "$file" --view expanded || true
echo
echo "Relevant symbols:"
rg -n "class Client|_client|def execute|def stop_channels|def get|poll|execute_input|execute_reply|ExecResult|started|timeout" "$file" || true
echo
echo "Read session.py lines 1-260:"
sed -n '1,260p' "$file" | awk '{printf "%6d\t%s\n", NR, $0}'
fiRepository: pablospe/docx-editor
Length of output: 19875
Synchronize on the hog request instead of sleeping.
The one-second delay does not prove that the hog execute request has been started by the kernel. Replace it with a bounded wait for the hog client’s matching execute_input message before sending the test request, so started=False cannot fail nondeterministically under CI load.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_session.py` around lines 135 - 136, In the hog execution setup
around hog.execute, replace the fixed one-second time.sleep with a bounded wait
for the hog client to receive the matching execute_input message. Synchronize on
that kernel message before sending the test request, while preserving the
existing timeout behavior and ensuring the request is confirmed started before
asserting started=False.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
=======================================
+ Coverage 96.1% 96.3% +0.1%
=======================================
Files 11 11
Lines 4355 4418 +63
Branches 859 869 +10
=======================================
+ Hits 4187 4256 +69
+ Misses 78 73 -5
+ Partials 90 89 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…s boundary, doc corrections
Round of independent review (3 reviewers + codex, red-teamed): 11 findings
confirmed, 3 killed as false positives, 6 downgraded.
Correctness:
- session.py: ExecResult's docstring claimed "ok/error cannot happen without
executing". False — ipykernel aborts everything still queued behind a command
that raises, so a request sent while an earlier one was failing returns
status="ok", started=False, empty stdout, having run nothing. Reproduced end
to end (exit code 0, and the variable it set was never bound). The docstring
now describes it, the CLI prints a loud warning instead of a silent success,
and two tests pin both. Exit code stays EXIT_OK — changing it is a contract
change, so it is left for a separate decision.
- tests/test_batch_edit.py: the two new classes were inserted inside
TestBatchEditDryRun's body, so six pre-existing dry-run tests had silently
become methods of TestEditOperationSearchResultTarget. They still ran, but
`-k TestBatchEditDryRun` collected 5 instead of 11. Classes moved below them.
Documentation (all verified by execution):
- api.md's "Equivalent, spelled out" SearchResult example raised
HashMismatchError as written — the second call reused the match the first one
made stale. Commented out with the reason, matching SKILL.md's treatment.
- SKILL.md's "the paragraph argument is required for all edit methods"
contradicted the SearchResult form this PR adds (and was already false for
document-wide add_comment).
- api.md said a None target raises ValueError for the whole method family;
add_comment raises CommentError, which is not a ValueError.
- api.md over-promised current_ref: it clears the hash check only, so a row can
carry one and still have a target that moved. Says so now, including how a
stale retry fails (loudly and atomically).
- quickstart.md piped an unchecked find_text() straight into replace(), the
exact pattern api.md tells readers to avoid.
- quickstart.md and api.md's save section still named force_recreate=True as
the only workspace recovery; both now mention discard_workspace().
- SKILL.md documents that exit code 0 does not always mean the code ran.
Consistency and dead code:
- Renamed the new helpers' `arg` parameter to `field`, matching
_reject_control_chars(value, *, field, ctx).
- _ValidationOutcome is now constructed with keywords — its own docstring cites
transposition as the reason it exists.
- _build_paragraph_info's `style_outlines=None` default was unreachable (both
call sites pass _style_maps()[0], which is already {} when styles.xml is
absent); made required.
- _stale_row returned a tuple whose first element no caller read.
- _style_maps' docstring notes the cached maps are handed out live, so callers
must treat them as read-only.
- _resolve_search_target's docstring explains why it returns a plain tuple.
- Dropped the package's only RST literal-block marker.
Skipped, with reasons: setting `started` on any parent-matched traffic (both
proposed variants destroy the discard signal above — the abort publishes
status busy/idle with no execute_input); moving the defusedxml call out of
document.py (ooxml/unpack.py and ooxml/pack.py already parse directly); making
workspace_dir positional (keyword-only matches the newer APIs).
1408 passed, 2 skipped.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api.md (1)
1173-1176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the original operation when repairing stale refs.
These examples rebuild every stale row as
replace("old", "new", ...); a stale delete or insert would become a different edit. They also apply immediately even thoughcurrent_refonly refreshes the hash. Preserve the original operation fields (for example, withdataclasses.replace(..., paragraph=row.current_ref)), then dry-run the repaired batch before applying it.
docs/api.md#L1173-L1176: update the recovery snippet to retain the existing operation and revalidate beforebatch_edit.docx_editor/document.py#L1366-L1371: make the API docstring example use the same action-preserving, revalidated flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api.md` around lines 1173 - 1176, Update the recovery examples in docs/api.md lines 1173-1176 and docx_editor/document.py lines 1366-1371 to preserve each original operation’s action and fields while replacing only its paragraph reference, then run the repaired operations through a second dry-run before applying batch_edit. Use dataclasses.replace or the established equivalent rather than rebuilding every stale row as replace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/quickstart.md`:
- Around line 85-87: Update the quickstart guidance around find_text() and
add_comment() to correct the exception claim: state that the four edit methods
raise ValueError when given a None target, while add_comment() raises
CommentError for a None anchor.
---
Outside diff comments:
In `@docs/api.md`:
- Around line 1173-1176: Update the recovery examples in docs/api.md lines
1173-1176 and docx_editor/document.py lines 1366-1371 to preserve each original
operation’s action and fields while replacing only its paragraph reference, then
run the repaired operations through a second dry-run before applying batch_edit.
Use dataclasses.replace or the established equivalent rather than rebuilding
every stale row as replace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1abf16b5-6884-4997-90c5-1a4bd3a88eda
📒 Files selected for processing (9)
docs/api.mddocs/quickstart.mddocx_editor/document.pydocx_editor/session.pydocx_editor/track_changes.pydocx_editor/xml_editor.pyskills/docx/SKILL.mdtests/test_batch_edit.pytests/test_session.py
Comment on lines
+85
to
+87
| Check the result first: `find_text()` returns `None` when there is no match, and | ||
| passing that `None` as a target raises `ValueError` rather than quietly doing | ||
| nothing. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the add_comment(None, ...) exception claim.
add_comment() is included among the five methods above, but it raises CommentError, not ValueError, for a None anchor. Qualify this as applying to the four edit methods or document the exception split.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/quickstart.md` around lines 85 - 87, Update the quickstart guidance
around find_text() and add_comment() to correct the exception claim: state that
the four edit methods raise ValueError when given a None target, while
add_comment() raises CommentError for a None anchor.
Second review pass caught the previous commit overstating its own fix: the SKILL.md bullet claimed a discarded request shows up as "started": false in eval's JSON envelope. It does not. eval_code sees result=None and raised "eval transport failed: kernel returned no result for the eval wrapper" — exit 1, no envelope, and a message that sends the caller debugging the transport instead of re-sending the command. - eval_code now distinguishes the two: with started=False it says the kernel discarded the request without running it; the transport message is kept for the case it was written for (the wrapper ran and still returned nothing). - SKILL.md describes both CLI paths as they actually behave — exec exits 0 with a stderr warning, eval exits 1 with the discard message and no envelope. - quickstart.md mirrors api.md's caveat that add_comment raises CommentError, not ValueError, for a None target. - test_eval_transport_no_result_raises now builds its ExecResult with started=True, which is what makes that fixture a transport bug rather than a discard; it caught this branch immediately, so its intent is worth pinning. Exit codes and the envelope contract are unchanged.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
The backlog lived in an untracked ISSUES.md that existed only on one machine. It is now ROADMAP.md: current status, the design commitments every future item inherits, an ordered Next list (#73 decomposition of track_changes.py first), demand-gated Later items, the deliberate refusals, and a per-release Shipped list generated from the old Done entries. Item numbers are unchanged; PR titles and docs now cite (ROADMAP.md #N).
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…ixin (ROADMAP.md #73) (#87) * refactor: move the batch and rewrite methods into a RevisionManager mixin (ROADMAP.md #73) Cut batch_edit, _resolve_action_target, _apply_single_edit, validate_batch, _validate_single, batch_rewrite, rewrite_paragraph, _rewrite_paragraph_inner, _find_match_at_position and _rewrite_insert_at out of manager.py into a new _BatchMixin in batch.py, byte for byte. base.py gains the twenty stubs the cluster calls across mixin boundaries (four registry, two locate, fourteen still on RevisionManager) so ty resolves them without an override. * test: cover the batch edge cases moved in ROADMAP.md #73 step 4 batch_rewrite must surface the original apply error when the rollback re-parse itself fails, and _find_match_at_position must reject a hit past the expected offset and tolerate an empty search.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…(ROADMAP.md #73) (#89) * refactor: move the replace-site methods into a RevisionManager mixin (ROADMAP.md #73) Cut the replace cluster out of manager.py into replace.py as _ReplaceMixin: replace_text, _get_run_info, _get_node_text, _set_node_text, _build_cross_boundary_parts, _majority_rPr, _classify_segments, _replace_across_nodes, _replace_same_context, _replace_within_own_ins, _replace_mixed_state, _find_ancestor, _owns_ins and _ins_identity_attrs. The bodies are byte-identical; manager.py only loses the two imports that became unused (_trim_replace_affixes, _node_depth) and gains the mixin base. base.py grows six stubs for the cross-cluster callees the moved code reaches through self: _locate_document_wide, _group_positions_by_ins, _delete_from_ins_positions, _split_ins_after_child, _delete_regular_segment and _split_replace. scripts/check_pure_move.py --base main: 3/3 ok. * test: cover the replace edge cases moved in ROADMAP.md #73 step 5 Eight tests for the defensive branches of _ReplaceMixin that nothing exercised before: _majority_rPr on empty parts, _set_node_text keeping a non-text child, a run-less <w:t> in _build_cross_boundary_parts and _replace_mixed_state, a detached run in _replace_same_context, an own-insertion-only match in _replace_mixed_state, and the two editor stubs that hand back no <w:ins>. replace.py: 100% lines and branches under the replace-related test set.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…ROADMAP.md #73) (#90) * refactor: move the delete-site methods into a RevisionManager mixin (ROADMAP.md #73) Cut the delete cluster out of manager.py into delete.py as _DeleteMixin: suggest_deletion, _group_positions_by_ins, _delete_from_ins_positions, _split_ins_after_child, _split_foreign_ins_at, _insert_own_ins_within_foreign_ins, _remove_from_insertion, _remove_wt_and_maybe_run, _content_nodes_in_ancestor, _delete_regular_segment, _delete_across_nodes, _delete_same_context and _delete_mixed_state. The bodies are byte-identical; manager.py only loses the four imports that became unused (OrderedDict, TextPosition, is_tab_node, _validate_edit_target) and gains the mixin base. base.py grows four stubs for the cross-cluster callees the moved code reaches through self: _build_cross_boundary_parts, _classify_segments, _ins_identity_attrs and _adopt_split_tail. scripts/check_pure_move.py --base main: 3/3 ok. * test: cover the delete edge cases moved in ROADMAP.md #73 step 6 Eight tests for the defensive branches of _DeleteMixin that nothing exercised before: _remove_from_insertion on a detached <w:ins> (whole insertion and whole-node matches) and on a run-less <w:t> middle split, _remove_wt_and_maybe_run on an unattached <w:t>, _delete_regular_segment skipping a run-less <w:t> and tolerating a run the editor already detached, and _delete_same_context with a detached matched run and with an editor that hands back no <w:del>. delete.py: 100% lines and branches under the delete-related test set.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…ompleting the package split (ROADMAP.md #73) (#94) * refactor: move the resolution methods into a RevisionManager mixin (ROADMAP.md #73) Completes the track_changes package split: manager.py keeps only the RevisionManager class header and __init__, and the 21 accept/reject methods move verbatim into _ResolutionMixin. base.py gains six stubs for the cross-cluster callees the cluster reaches (_find_revision_element, list_revisions, _unhandled_elements, group_revisions, changeset_groups, _paragraph_mark_rPr). scripts/check_pure_move.py --base main: 3/3 ok. * test: cover rejecting a w:pPrChange placed outside w:pPr (ROADMAP.md #73) The schema-invalid-placement guard in _restore_paragraph_properties had no test: a record whose parent is not w:pPr is dropped rather than restored into whatever parent it has. Takes resolution.py to 100% statement and branch coverage.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six code items plus two documentation items from the round-3/4 dogfood papercut
list. Tracked as ISSUES.md #52 and #60 (repo-local item numbers, not GitHub
issues — deliberately not auto-closing anything here).
What changed
1. A
SearchResultcan stand in for its own three fields.replace,delete,insert_after,insert_before,add_commentand the fourEditOperationconstructors accept a match where they expect target text, andtake
paragraph/occurrencefrom it:Passing
paragraph=/occurrence=as well raisesValueErrorrather thanpicking a winner, and a stale match raises
HashMismatchErrorlike any otherstale ref. The dataclass fields stay plain strings, so an op built from a match
is indistinguishable from a hand-written one.
2.
ParagraphInfogainsin_table/style/outline_level. A table ofcontents is now one
list_paragraphs_structured()call instead of a secondlist_paragraph_locations()pass. The semantics areParagraphLocation's,reusing its extractors — no new extraction logic — and one factory builds the
record for all three producers so they cannot drift.
3.
EditValidationResult.current_ref— dry-run rows that failed on a stalehash carry the ref for the paragraph's current content, so recovery stops
regexing the hash out of the error prose.
Noneon every other outcome, whichis also how you tell a stale hash from a target that no longer exists.
4.
Document.discard_workspace(path)— the crashed-script reset, replacingthe deep
Workspaceimport (or a hand-deleted cache directory) the docs used toprescribe. Idempotent, clears the lock sidecar, honours
workspace_dir.5.
ExecResult.started/EvalResult.started— tells a timeout of your ownrunning code apart from one that never left the kernel queue, taken from the
kernel's
execute_inputbroadcast for our ownmsg_id. The CLI printsdifferent advice for each; status enum and exit codes are unchanged.
6. Whitespace-only replaces are minimal revisions —
"clause 2"→"clause 2"is now a pure deletion instead of an invisible del+ins pair.Character-level trimming is gated on both trimmed sides being whitespace, so
"30 days"→"60 days"stays one whole-word replacement.7–8. Docs only. Pagination overshoot is documented as a valid request with
an empty answer — no behaviour change, deliberately, since raising would break
the loop-until-empty idiom — and
get_paragraph()/context()are markedO(document) with the "list once, index the result" alternative.
Behaviour change beyond #52/#60 (please look at this one)
Review of item 6 turned up a fidelity bug it exposed:
w:delTextnever carriedxml:space="preserve". A conforming consumer trims the edges of an unpreservedw:delText, and_restore_deletioncopies that element's attributes onto thew:tit rebuilds — so rejecting a whitespace deletion restored the wrongspacing after a Word round-trip. Reproduced before the fix:
get_visible_text()reads the DOM directly, which is why the tests passed overit. Item 6 makes whitespace-only
delTextthe normal output shape, so the fixships here: the editor's existing
xml:spaceinjection now coversw:delTextas well as
w:t. This also repairs the latent pre-existing case on ordinarydeletions (
delete("brown "),replace("quick brown", "quick")), so everytracked deletion whose text has a whitespace edge now carries the attribute —
visible in output XML.
Performance
get_paragraph()needs the style maps now, which naively meant aword/styles.xmlparse per call — a 31x regression on a 3300-paragraphdocument.
_style_maps()is therefore memoized perDocument(nothing in thislibrary ever writes
styles.xml) and parsed withoutXMLEditor's line-trackingparser, whose
parse_positionmetadata the map builders never read.list_paragraphs_structured(limit=None)get_paragraph(1500)get_paragraph_location(...)The structured listing is faster than before even on its first call (one text
map now serves both hash and text); the one honest regression is the first
get_paragraph()on a document with a large styles part, which pays a parse anylocation call would have paid anyway.
list_paragraphs()— the string API, theknown #56 bottleneck — is deliberately untouched.
Testing
1406 passed, 2 skipped(baseline 1345 → +61: 59 for the feature work, 2 forthe
xml:spaceregression).ruff check/ruff format --checkclean,ty checkexit 0 with only the 6 pre-existingpossibly-missing-attributewarnings in files this branch does not touch,
mkdocs build -sclean.Docs snippets were verified by execution against purpose-built documents, and
the trim invariants were brute-forced over 225 find/replace pairs (affixes never
overlap, are always genuinely shared, and reconstruct both strings).
Notes
Document.discard_workspace()will delete the workspace of aDocumentstillopen in this process. Documented;
open(force_recreate=True)has always hadthe identical hazard.
current_ref(a ready-to-use ref) ratherthan the
stale_hashthe issue text suggested, because the value consumerscurrently regex out of the prose is the current hash and a ref removes the
string surgery entirely.
HashMismatchError.actual_hashstill exposes the rawhash on the apply path.
time out behind it) and each get their own kernel — the likeliest flake source
here if CI is loaded.
Summary by CodeRabbit
New Features
SearchResultobjects directly (eliminating extra paragraph/occurrence wiring).discard_workspace.current_refto recover from stale edits.started).Bug Fixes
Documentation