Skip to content

fix(equations): read every line of a multi-line display block - #30

Open
xsvm wants to merge 2 commits into
kklimuk:mainfrom
xsvm:fix/multiline-omathpara
Open

xsvm wants to merge 2 commits into
kklimuk:mainfrom
xsvm:fix/multiline-omathpara

Conversation

@xsvm

@xsvm xsvm commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Summary

A display equation block that holds several lines, Word's <m:oMathPara> with more than one <m:oMath>, read as its first line only:

<w:p><m:oMathPara><m:oMath>x=1</m:oMath><m:oMath>y=2</m:oMath></m:oMathPara></w:p>
  • read showed $$x=1$$; the EquationRun's latex was x=1 while its text was x=1y=2.
  • equations edit --at eq0 --equation "x=3" --track replaced the whole block, so the y=2 line was tracked as deleted although the agent never saw it.

The fix

  • ommlToLatex reads such a block as \begin{gathered}x=1\\y=2\end{gathered}, so read, --ast and Markdown show every line.
  • latexToOmml turns that form back into one <m:oMath> per line, so read → edit → read keeps Word's line structure. Only a top-level \\ splits; a \\ inside braces or an inner environment (a matrix row) does not.
  • Still one eqN per block, so locators don't shift.
  • Short note in src/core/equation/CLAUDE.md.

Doesn't touch ast/read.ts; #27 changes other lines of equation/read.ts, so the two should merge cleanly.

Test plan

  • bunx biome check ., bunx knip, bunx tsc --noEmit
  • 4 new tests in tests/cli/equations.test.ts: read shows every line, edit round-trip, tracked edit (old block in --baseline, new lines in --accepted), a matrix inside one line stays one line
  • tests/cli/equations.test.ts passes (51 tests)
  • Full suite: CI on this PR

A Word display block holding several <m:oMath> lines inside one
<m:oMathPara> read as its first line only: read showed `$x=1$` while the
block also held `y=2`, and a tracked `equations edit` then deleted the
line the agent never saw.

- ommlToLatex reads such a block as \begin{gathered}x=1\y=2\end{gathered}.
- latexToOmml splits that form (top-level \ only, not a matrix row) back
  into one <m:oMath> per line, so read -> edit -> read keeps the lines.
@kklimuk

kklimuk commented Oct 29, 2026

Copy link
Copy Markdown
Owner

🤖 docx-cli triage

Thanks @xsvm — this is a real bug and the fix is sound. Review of 7fcf42d below. My follow-up commit couldn't be pushed, because the session's git proxy refused to send credentials for xsvm/docx-cli. Details are at the end.

What the PR claims

If an <m:oMathPara> holds several <m:oMath> lines, read shows only the first line, and a later equations edit deletes lines the agent never saw. The PR reads such a block as \begin{gathered}…\\…\end{gathered}, and latexToOmml splits that form back into one <m:oMath> per line.

Before / after

The test doc is create followed by raw replace --at p0 with <m:oMathPara><m:oMath>x=1</m:oMath><m:oMath>y=2</m:oMath></m:oMathPara>.

origin/main PR
read $$x=1$$ (the y=2 line is hidden) $$\begin{gathered}x=1\\y=2\end{gathered}$$
equations add --display '\begin{gathered}a=1\\b=2\end{gathered}' 1 <m:oMath> (aligned eqArr) 2 <m:oMath> lines

bun run check ✅ and bun run test:unit ✅ pass at the PR head (1913 tests). The diff touches only src/, tests/ and the equation CLAUDE.md note, and the description matches it. The CLAUDE.md paragraph is accurate against the code.

Problems found and fixed locally (commit 6c36401, not pushed)

  • A \\[4pt] / \\* option turned into literal text. For example, \begin{gathered}a=1\\[4pt]b=2\end{gathered} wrote [4pt]b=2 into the document. gatheredLines now skips the break's own * and […] option, reusing findMatchingBracket from latex.ts.
  • A line that starts with [ or * must survive read → edit. The reader now adds a space before such a line (x=1\\ [A,B]=0). Without the space, the fix above would treat it as a spacing option.
  • The tracked-edit test passed without testing anything. On main, read --accepted and read --baseline don't filter tracked equations: both views show the old and new equation ($$x=1$$$$x=3$$), even for a plain single-line equation. That is a separate, pre-existing bug. So the toContain checks passed in either view. The test now checks that the <w:del> holds both old lines and the <w:ins> holds both new ones. It was also renamed, because the whole block is replaced, not just the changed lines.
  • Fixture coverage. CLAUDE.md "Testing" asks for fixture coverage, so tests/fixtures/setup/equations.ts gains a "Multi-line block (display)" entry and equations.docx was rebuilt. The multi-<m:oMath> shape now round-trips through LibreOffice. read output is unchanged apart from the new entry, and docx validate passes.
  • 2 new tests: break options, and a line that starts with a bracket. check ✅ and test:unit ✅ after the changes (1915 tests).

Open questions for the maintainer

  • Is the change wanted? (No issue asked for it, but it fixes silent data loss on Word multi-line equations, so I'd say yes.)
  • Behavior change for existing users: any agent-authored display \begin{gathered}…\\…\end{gathered} now becomes separate <m:oMath> lines instead of one eqArr. This is closer to Word, but different from before.
  • read prints the block on one line as $$…$$, which remark-math parses as inline math on read → create. This also happens on main for every display equation, so it is a separate issue.
  • The --accepted/--baseline views don't filter tracked equations (pre-existing, see above). Worth its own issue.

Action needed: routine blocked — the git proxy wouldn't push to xsvm/docx-cli. Either add that repo to the routine's session sources and re-run, or apply commit 6c36401 (the diff summarized above) yourself. Pushing it from a maintainer account counts as the last push, so your own approval won't satisfy the repo's last-push rule: merge with admin bypass or get a second approver.


Generated by Claude Code

… real tracked test

- gatheredLines skips a break's own `*` and `[4pt]` option (reusing
  findMatchingBracket) instead of writing it into the next line as text.
- The reader prefixes `{}` to a line that opens with `[` or `*`, so
  read -> edit doesn't take it for the break's option.
- The tracked-edit test checks the markers: <w:del> holds both old lines,
  <w:ins> both new ones (read --accepted/--baseline don't filter tracked
  equations yet, so the old toContain checks passed either way).
- equations.docx gains a "Multi-line block (display)" entry so the
  multi-<m:oMath> shape round-trips through LibreOffice.

Follows the triage review on kklimuk#30.
@xsvm

xsvm commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I reproduced every item on 7fcf42d before changing anything. \\[4pt] wrote [4pt] into the line, \\* wrote ∗, a [A,B]=0 line read back as \\[A,B], and read --accepted/--baseline both showed old and new. The fixes are in 73d3f08, which I pushed myself so the last-push rule doesn't get in the way:

  • Break options: gatheredLines now skips the break's own * and […] (reusing findMatchingBracket), so they don't get written in as text.
  • A line that starts with [ or *: the reader prefixes it with {} (x=1\\{}[A,B]=0), not a space. In real LaTeX \\ also skips spaces before [, so \\ [A,B] would still be read as a spacing option. {} is unambiguous for temml, for our splitter and for LaTeX. read → edit → read is now a fixed point (tested).
  • Tracked-edit test: it now checks the markers themselves (<w:del> holds x=1,y=2; <w:ins> holds x=3,y=2) and is renamed, since the whole block is replaced.
  • Fixture: equations.docx gains a "Multi-line block (display)" entry. Apart from that entry and the section's applies-to range, read output is unchanged, and docx validate passes.

I checked that both new tests fail with the src change reverted. biome, knip, tsc and test:unit pass locally. The only failure is the Windows-only compiled-binary upgrade test, which also fails on main.

The two pre-existing issues you raised ($$…$$ on one line parsing as inline math on read → create, and --accepted/--baseline not filtering tracked equations) are outside this PR. I can open issues for them if you'd like.

@kklimuk

kklimuk commented Oct 29, 2026

Copy link
Copy Markdown
Owner

🤖 docx-cli triage

Thanks @xsvm. I re-reviewed 73d3f08 and everything from my last review is fixed. I didn't push anything, so the last push is still yours.

Prior review items: all addressed

  • Break options: \\[4pt], \\* and \\*[2ex] are dropped, not written in as text (tested). An unclosed \\[4pt b stays as text, which is fine.
  • A line that starts with [ or *: it reads as x=1\\{}[A,B]=0, and read → edit → read gives the same LaTeX back with 2 <m:oMath> (tested). {} is a better choice than my space.
  • Tracked-edit test: it now checks that the <w:del> holds the lines x=1,y=2 and the <w:ins> holds x=3,y=2.
  • Fixture: I re-ran bun tests/fixtures/setup/equations.ts. The rebuilt equations.docx has the same parts as the committed one, byte for byte, and gives the same read output. Compared with main, the only change is the new "Multi-line block (display)" entry ($$\begin{gathered}a+b=c\\x^2=4\end{gathered}$$) and the applies-to range moving from p219 to p222. docx validate passes. I didn't run LibreOffice.
  • The CLAUDE.md note (reviewed as text) matches the code.

Before / after (on 73d3f08)

The test doc is a <m:oMathPara> holding two <m:oMath> lines, x=1 and y=2.

origin/main PR
read $$x=1$$ (y=2 is hidden) $$\begin{gathered}x=1\\y=2\end{gathered}$$
equations add --display '\begin{gathered}a\\b\end{gathered}' 1 <m:oMath>, reads back as aligned 2 <m:oMath>, reads back as gathered
inline equations add with the same LaTeX 1 <m:oMath> (aligned) unchanged

Other edge cases that work: a \\ inside \text{…} or \left\{…\right., a matrix row inside a line, leading or trailing whitespace, and 3 or more lines. A trailing \\ with nothing after it stays one line. Two gathered blocks side by side aren't split.

bun run check ✅. bun run test:unit ✅ (1915 pass). The first run had one flaky failure that I couldn't reproduce. It passed on the rerun, and no PR test was involved.

Optional nit (not blocking)

\begin{gathered}a\\ [4pt]b\end{gathered} (with a space before [) keeps [4pt]b as line content, and it reads back as a\\{}[4pt]b. You pointed out that LaTeX's \\ skips spaces before [, so gatheredLines could skip whitespace before checking for */[. This is safe because the reader already writes {} before a line that starts with [. Agents rarely write this, and the text stays visible on read, so it's fine as a follow-up.

Open questions for the maintainer

  • Is the change wanted? No issue asked for it, but it fixes silent data loss on Word multi-line equations. I'd say yes.
  • Behavior change: an agent-authored display \begin{gathered}…\\…\end{gathered} now becomes separate <m:oMath> lines instead of one eqArr. This is closer to Word, and it now round-trips as gathered instead of aligned. Inline equations are unchanged.
  • Pre-existing, outside this PR: read prints a display equation as $$…$$ on one line, so read → create parses it as inline math (and gathered becomes aligned). Also, read --accepted/--baseline don't filter tracked equations. @xsvm offered to open issues for both.

Action needed: approve/merge — PR #30 is ready. I didn't push, so the contributor made the last push and your own approval satisfies the last-push rule. Also answer the questions above, and whether you want issues for the two pre-existing bugs.


Generated by Claude Code

@kklimuk

kklimuk commented Oct 30, 2026

Copy link
Copy Markdown
Owner

@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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants