Repository navigation
Conversation
An equation takes no characters in find/replace offsets, so a match could run across one unseen. Replacing it cut the words around the equation and left the equation stranded in the new text, exit 0, no hint; a whole- paragraph `edit --text` moved it to the front of the paragraph. - find tags each match with the equations strictly inside it (`equationsWithin`, on the same runViewText offsets as trackedChangesOverlapping), so `find --json` shows them. - replace (single, --batch, cross-paragraph) and `edit --at pN:S-E` refuse such a match with UNSUPPORTED, naming the eqN, before anything is written; `--dry-run` refuses the same. A match touching only an equation's edge still replaces. - `edit --text` on a paragraph holding an equation is refused; the hint points to a markdown rewrite with $…$.
|
🤖 docx-cli triage Thanks @xsvm, this is a real bug and the fix is well-shaped. Here's what I checked and what I'd still change. What I checked (head
|
| Command | main |
this PR |
|---|---|---|
replace "Energy.*physics" "Gone" --regex |
Gone$E=mc^2$., exit 0 |
UNSUPPORTED naming eq0, exit 1, file untouched |
edit --at p0 --text "New words." |
$E=mc^2$New words., exit 0 |
UNSUPPORTED, exit 1 |
edit --at p0:7-13 --text XX |
Energy XX$E=mc^2$ physics., exit 0 |
UNSUPPORTED, exit 1 |
Also verified on the PR head:
- Edge-touching matches still replace.
- Table-cell paragraphs are covered.
edit --batchtext and span entries refuse.--text "",delete,--boldand--styleon an equation paragraph still work.bun run checkandbun run test:unitpass.
Gaps the review found, and my fixes
I wrote these as commit 3276118 on top of yours, but I couldn't push it to your branch: the session's git proxy refused the push to xsvm/docx-cli (403, repo not in the session's authorized set). The commit is ready locally. Here's what it does:
- Cross-paragraph replace still moved an equation sitting exactly at the match start.
replace " in physics.\nMid" " X"gaveEnergy is X$E=mc^2$ …with exit 0.equationsWithincorrectly treats that equation as an edge, butsplitChildrenAtsent zero-width children at the start offset into the cut. The fix keeps edge children outside the cut on both sides. The result is nowEnergy is $E=mc^2$ X …. - Whole-paragraph
edit --runsdropped the equation silently. It is now refused like--text. edit --dry-runreported success for edits the real call refuses, both single and--batch. It now refuses the same way.edit --batch --dry-runnow applies the entries in memory and skips only the save, the wayreplace --batchalready does.- An equation already inside a pending
<w:del>blocked every later--textrewrite. The whole-paragraph check now ignores deleted equations. - The refusal hint suggested pasting the canned sentence
"Energy is $E=mc^2$ here."with the user's real locator. A weak agent could overwrite the real paragraph with that sentence. The hint now says to copy the line fromdocx read. - Refusals now name the full span (
p0:7-13, orp0:14-p1:8across paragraphs), not just the first block. - A shared
describeEquationshelper replaces the duplicated formatting, and thefind --jsontest moved tofind.test.ts.
With those fixes: bun run check ✅ and bun run test:unit ✅ (1950 pass). /simplify extracted the duplicated formatting into describeEquations. The /code-review xhigh findings are the items above.
Left as is:
equationsWithinstill counts tracked-deleted equations. Fixing that needs fix(track-changes): show tracked equations, images and line breaks #27'strackedChangeonEquationRun, as you noted.- A range
edit --at pN-pM --textstill drops equations. The range replaces whole paragraphs by design.
Decisions for you
- Is the change wanted at all? No issue asked for it. I think it is: it closes silent data loss with an exit 0.
- Should whole-paragraph
edit --textand--runsrefuse when the paragraph has an equation? This changes behavior for existing users. The alternative is to drop the equation, which is what a range--textdoes. - Should a range
edit --at pN-pM --textthat covers equations refuse too, to match the new REFERENCE line "Text edits never strand an equation"? - Should a refused equation entry in
replace --batchfail the whole batch with nothing saved? That's what an invalid pattern does. A missed entry, by contrast, still saves the entries that matched.
Action needed: routine blocked — the push of fix commit 3276118 to xsvm/docx-cli:fix/replace-span-equation was refused by the session git proxy. Add the fork to the session's sources and re-run, or ask @xsvm to apply the changes listed above. Once anything is pushed as kklimuk, your approval alone won't satisfy the last-push rule: you'll need a second approver or an admin-bypass merge.
Generated by Claude Code
- A cross-paragraph replace keeps an equation sitting exactly at the match start outside the cut (splitChildrenAt sends edge children outward on both sides), so it no longer moves behind the replacement. - Whole-paragraph edit --runs refuses like --text instead of dropping the equation silently. - edit --dry-run (single, span and --batch) applies in memory and skips only the save, so the preview refuses exactly what the real call would. - An equation already inside a pending <w:del>/<w:moveFrom> no longer blocks a later --text rewrite. - The refusal hint points at the paragraph's own read line instead of a sample sentence an agent could paste over the real paragraph. - Refusals name the full span (p0:7-13, p0:14-p1:8); a shared describeEquations formats the equations. - The find --json test moves to find.test.ts. Follows the triage review on kklimuk#29.
|
Thanks for the review. I reproduced each gap on
As you noted, |
|
🤖 docx-cli triage Thanks @xsvm. I re-reviewed the new head What I checkedRepro doc:
Also verified:
I made no changes this round. One small fix I'd still like (non-blocking, your call)The new REFERENCE.md example and both refusal hints put markdown with Decisions for you (carried over)
Action needed: approve/merge — PR #29 is ready once you've answered the questions above (optionally after @xsvm switches the examples to single quotes). The contributor made the last push, so your own approval satisfies the last-push rule. Generated by Claude Code |
|
@xsvm hold off from making further changes till i've had a bit of time to read through and do my own reviews, it's gonna take me a few days. |
Summary
An inline equation takes no characters in find/replace offsets, so a match could run across one without the matched text showing it. Repro: a paragraph
Energy is $E=mc^2$ in physics.replace "Energy.*physics" "Gone" --regexGone$E=mc^2$., exit 0, no hint: the words around the equation are cut and the equation is stranded in the new text--trackedit --at p0:7-13 --text XXedit --at p0 --text "New text"find "is in"A footnote reference inside the span is deleted with it (it lives inside a
<w:r>); a direct<m:oMath>child of the paragraph fell through the span walkers' keep-unknown-siblings branch.The fix is a data check on what
findalready returnsfindtags each match with the equations strictly inside it:equationsWithin(find/index.ts) sits next totrackedChangesOverlappingand walks the same AST runs on the samerunViewTextwidths, so it can't disagree with the offsetsfindreports.TextMatch.equations/ParagraphSpanMatch.equationscarry theEquationRuns, sofind --jsonnow shows them.replace,replace --batch, cross-paragraph replace andedit --at pN:S-Erefuse such a match withUNSUPPORTEDbefore anything is applied, through one sharedspanOverEquationsError. The message names the locator, e.g.p0: the text span covers eq0 ($E=mc^2$), and the hint points toequations edit --at eqNor a markdown rewrite.--dry-runrefuses the same way; in a batch nothing is saved, like an invalid pattern.applyAcrossReplacereports the refused match ascrossingand applies nothing.replace "Energy is " "Mass ") still works and keeps the equation.edit --texton a paragraph holding an equation is refused:--textis the literal channel and has no way to say where the equation goes. A markdown rewrite with$…$keeps working. TheEditlens gets the AST paragraph through a smallBody.findBlockByNode, the reverse ofresolveBlock.No new XML offset walker, no new error class.
Note on #27: on
mainanEquationRundoesn't record the<w:del>around it, so an equation already tracked as deleted still counts as inside a span. Once #27 addstrackedChangetoEquationRun,equationsWithinshould skip runsisRevisionVisiblehides; I can add that line to whichever PR merges second.Test plan
bunx biome check .,bunx knip,bunx tsc --noEmittests/cli/replace.test.ts(single/--track/--dry-runrefusal, edge match,find --json, cross-paragraph, batch) and 3 intests/cli/edit.test.ts(span,--text, markdown rewrite)