Repository navigation
fix(html2md): preserve mathematical and inline formatting semantics - #1335
lanyue-llk wants to merge 14 commits into
Conversation
Spider Bench A/B✅ All benchmark browser service runs completed; results are informational. Public HEAD: 182 / 240 rows; 37 / 48 sites produced rows; 11 unexpected empty sites. ✅ Deterministic fixture contract is clean. Common ancestor Public 48-site run · informational
Public-site content, timing, and memory are noisy. This single A/B run reports evidence only; site outcome counts explain missing rows without treating them as a deterministic regression. CPU and memory timelinesThese are bounded 40-point views of the same complete process-tree samples used by the HTML report. CPU uses 100% per occupied logical core; memory lines are RSS first and PSS second when complete PSS samples are available. Basexychart-beta
title "Base CPU"
x-axis "Elapsed seconds" [1.536, 2.538, 3.038, 4.039, 4.54, 5.041, 5.541, 7.042, 8.543, 10.043, 11.545, 13.045, 14.045, 15.546, 17.046, 18.548, 20.05, 21.552, 22.554, 24.056, 25.558, 27.06, 28.561, 30.064, 31.065, 32.565, 34.066, 35.567, 37.068, 38.572, 39.572, 41.072, 42.572, 44.075, 45.577, 47.078, 48.08, 49.582]
y-axis "CPU percent" 0 --> 200
line [111.76, 107.88, 113.91, 111.89, 157.71, 121.77, 130.03, 83.88, 105.99, 31.94, 144.01, 98, 66.01, 39.95, 0, 3.99, 0, 3.99, 9.98, 29.94, 103.96, 97.83, 101.9, 85.78, 121.95, 129.94, 101.98, 67.99, 59.88, 113.69, 11.99, 0, 127.87, 99.9, 99.92, 121.9, 127.76, 89.95]
xychart-beta
title "Base memory: RSS then PSS"
x-axis "Elapsed seconds" [0.035, 1.536, 2.538, 3.038, 4.039, 4.54, 5.041, 5.541, 7.042, 8.543, 10.043, 11.545, 13.045, 14.045, 15.546, 17.046, 18.548, 20.05, 21.552, 22.554, 24.056, 25.558, 27.06, 28.561, 30.064, 31.065, 32.565, 34.066, 35.567, 37.068, 38.572, 39.572, 41.072, 42.572, 44.075, 45.577, 47.078, 48.08, 49.582, 51.022]
y-axis "Memory MiB" 0 --> 500
line [13.02, 97.04, 100.89, 119.38, 135.04, 135.1, 152.2, 152.7, 143.29, 131.24, 154.71, 147.26, 154.47, 145.46, 120.58, 119.63, 119.52, 119.14, 118.17, 119.6, 153.36, 138.87, 254.4, 186.93, 129.67, 143.75, 145.19, 136.22, 136.29, 156.16, 149.96, 127.01, 126.05, 162.52, 174.13, 174.14, 167.24, 143.35, 143.96, 0]
line [9.67, 93.87, 96.37, 115.96, 131.52, 131.64, 148.68, 150.47, 139.63, 129.82, 151.16, 143.38, 150.91, 141.8, 117.01, 116.06, 115.95, 115.61, 114.63, 116.06, 149.81, 135.66, 250.91, 183.4, 126.12, 140.23, 141.65, 132.75, 132.76, 152.64, 146.42, 123.47, 122.51, 159.03, 170.58, 170.59, 163.75, 140.46, 140.94, 0]
HEADxychart-beta
title "HEAD CPU"
x-axis "Elapsed seconds" [1.037, 2.039, 2.54, 3.541, 4.542, 5.544, 6.044, 7.046, 8.548, 9.549, 10.55, 12.05, 13.051, 14.053, 15.553, 16.554, 17.556, 19.057, 20.058, 21.561, 22.562, 23.563, 25.065, 26.066, 27.068, 28.57, 29.571, 30.573, 32.075, 33.076, 34.078, 35.579, 36.58, 38.083, 39.085, 40.086, 41.588, 42.088]
y-axis "CPU percent" 0 --> 200
line [25.94, 87.75, 127.85, 101.97, 125.91, 127.67, 113.96, 141.91, 71.82, 85.98, 133.96, 94.03, 57.97, 17.97, 24.01, 123.84, 111.71, 135.94, 102, 105.78, 132, 89.8, 109.86, 105.92, 97.92, 33.98, 35.92, 115.73, 3.99, 101.87, 131.86, 100.06, 99.89, 99.71, 99.89, 41.91, 119.88, 163.88]
xychart-beta
title "HEAD memory: RSS then PSS"
x-axis "Elapsed seconds" [0.036, 1.037, 2.039, 2.54, 3.541, 4.542, 5.544, 6.044, 7.046, 8.548, 9.549, 10.55, 12.05, 13.051, 14.053, 15.553, 16.554, 17.556, 19.057, 20.058, 21.561, 22.562, 23.563, 25.065, 26.066, 27.068, 28.57, 29.571, 30.573, 32.075, 33.076, 34.078, 35.579, 36.58, 38.083, 39.085, 40.086, 41.588, 42.088, 42.356]
y-axis "Memory MiB" 0 --> 500
line [13.08, 81.7, 102.66, 124.13, 112.48, 129.16, 153.09, 149.76, 141.36, 135.29, 135.11, 142.17, 143.82, 143.27, 117.46, 126.63, 175.66, 146.5, 142.72, 274.43, 207.58, 162.11, 144.96, 137.32, 132.37, 139.41, 139, 141.02, 150.47, 123.92, 158.2, 173.23, 173.39, 173.39, 173.41, 163.02, 138.59, 151.56, 160.57, 0]
line [9.75, 78.32, 99.21, 120.65, 109.32, 125.68, 150, 146.29, 137.88, 131.79, 131.57, 138.63, 140.36, 139.78, 113.95, 123.11, 167.97, 143.24, 138.39, 270.94, 204.12, 158.62, 141.44, 133.99, 128.86, 135.88, 135.43, 137.48, 146.93, 120.38, 154.66, 169.68, 169.85, 169.85, 169.86, 161, 135.03, 148.49, 155.67, 0]
Deterministic fixture · required contractThe fixture exercises 15 routes: 8 are expected to emit rows and 7 intentionally exercise empty, loading, or timeout behavior.
Full HTML, JSON, CSV, logs, and page snapshots: workflow run and full The public-web A/B is informational. The exact HEAD fixture contract runs as a separate required CI check; timing and public-site content deltas remain non-blocking. |
CI Regression ReportSource CI run · source state at render:
Release regression — ✅ HEAD/base failures 0/0; raw binary +0.017394%Package and image size
Startup latency and PSS
Concurrency matrix
Frontend differential — ✅ 1,020/1,020 cases matched; 0 issues
Agent episodes · Moli vs Chromium — ✅ 8/8 Moli episodes passed; 0 failures
Runtime and CDP session contracts — ✅ 26 contract cases; 0 failures
CDP smoke — ⚪ artifact unavailable or invalidArtifact unavailable or invalid. See the source CI run for infrastructure details. WebMainBench · 545 pages — ✅ 545/545 completed; 0 unexpected failures
Frozen HTML · Linux · Diagnostics: All artifact fields are parsed by the trusted default-branch renderer; missing or invalid inputs remain visible as unavailable. |
c57d52b to
f3cceb4
Compare
Sequential Navigation Soak A/B✅ HEAD completed the 200-navigation resilience and memory observation. One browser process, one target, and one CDP session navigate CSDN → SegmentFault → Huaban → example.com repeatedly. This run issued 200 Common ancestor Session resilience
A public-page failure is reported separately from an unrecoverable session. The soak requires all navigation attempts, zero failed recovery, zero lifecycle/network ordering violations, and complete resource evidence. Process-tree memory
HEAD memory by 50-navigation quarter
Raw reports and per-navigation boundary samples: workflow run and full Public-site timing and memory are observational. Use the A/B deltas and trend shape as evidence, not as a deterministic performance threshold. |
8928f56 to
0f62c65
Compare
0f62c65 to
d5b0e56
Compare
d5b0e56 to
cd0193d
Compare
|
Closing after the consolidation audit. This belongs to an earlier experimental Markdown stack and was not part of the measured fixed build. Current failure evidence does not justify carrying its broad formatting expansion into #1501, so it is intentionally not merged. |
5fac85d to
9ab07c1
Compare
b810898 to
014debd
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Re-review of the updated version (014debd29)
This PR keeps author TeX across transparent inline wrappers, passes filtered MathML through as inline HTML, emits <sup>/<sub>, drops interactive controls from code and keeps multiline alt/title attributes block-safe. The new revision fixes all three majors from last round, and each fix has a test rendered through pulldown with ENABLE_MATH that would fail on the old code. The CDP hunk is gone and the tracking cases have moved.
Verdict: approve once the minor items below are addressed (COMMENT). Nothing blocking is left. The remaining problems are edge cases in the new TeX normalization, $ handling outside accepted spans and in MathML, and commit hygiene.
| Previous finding | Status | Evidence |
|---|---|---|
[major] A rejected $ closer was never retried as an opener (Pay $5 for $x_i$) |
FIXED | math.rs:31-39: accept_candidate now returns a bool, and a rejected single-dollar closer followed by a non-space character becomes the next opener. review_semantics.rs tests Pay $5 for $x_i$. with ENABLE_MATH. |
[major] < followed by a letter made the whole span fall back to plain text |
FIXED | The rejection is removed. normalized_span (math.rs:161-167) emits < before [A-Za-z/!?]. Tested with $0<x<1$ and \(n<k\), and the inert-HTML test still passes. Nit inline: the inserted space is visible inside \text{}. |
| [major] Raw newlines inside math broke block structure and table rows | FIXED | normalized_span collapses whitespace runs to one space after stripping unescaped % comments. Tests cover - , # , > , a blank line, a table cell, \% and a comment. The % parity logic is right for \%, \\% and \\\%. Two new edge cases are inline (comment next to a single $, and \verb). |
| [minor] MathML text was not protected from Markdown parsing | FIXED (one character still missing) | mathml.rs:204 encodes \ * _ ~ [ ]in text nodes only. The test checks</mo>and that noappears.$` is still unencoded (inline). |
| [minor] CDP smoke hunk duplicated origin/main and conflicted | FIXED | moli-cdp-smoke no longer appears in the PR diff. git merge-tree against new-1334 is clean. |
[minor] Fixed tracking cases still lived in tracking.rs and the README |
FIXED, with stale comments | The cases moved to inline_boundaries.rs with exact rendered-HTML assertions. The README table rows are removed. The comment "Turndown emits the same Markdown. Matching it does not preserve the image." moved along with the test and is now false (inline). |
[minor] The price test never parsed with ENABLE_MATH; no < or multiline coverage |
FIXED | mixed_currency_inequalities_and_multiline_math_keep_math_payloads |
[nit] The PR description did not say that <sup>/<sub> output changes downstream |
NOT ADDRESSED | The description says only "sup/sub 保留 HTML 语义". Footnote refs now come out as <sup>[1](#n)</sup> on every page. |
New findings
- [minor] An unescaped
%comment that reaches a single-$delimiter leaves a space beside it, so$a%b$and$5%$become$a $and$5 $, which math readers no longer parse (math.rs:170). - [minor]
$stays raw in MathML text and outside accepted spans, so math-aware readers can pair it across MathML tags or links (mathml.rs:204,writer.rs:114). - [nit] Comment stripping and whitespace collapse change
\verb|...|content, and the inserted<space shows up inside\text{}(math.rs:142). - [nit] The moved test comments are stale (
inline_boundaries.rs:260). - [nit] The raw-string
\+newline informulas_keep_their_order_across_output_boundariesputs a literal backslash into the HTML fixture (review_semantics.rs:104). - [nit]
escapecallsformat!once per encoded character (mathml.rs:205).
Not anchored to a diff line
- Commit hygiene [minor]:
a246449cb ci: refresh checks after main gamepad test repairand77af093d0 ci: validate after credential fixture fix reached mainare still empty commits.f13d43cb9(and the commits under it) movestruct Countedinsidenested_accessible_labels_do_not_rescan_the_whole_treeeven thoughaccessible_label_queries_remain_linear_with_large_unreadable_branches(line 138 at that commit) also uses it. Socargo testdoesn't compile at those intermediate commits, and37ce36a70only moves it back. That commit's title, "fix(ci): validate standalone formatting layer and transport retries", no longer describes it: after the CDP hunk was dropped it touches onlyreview_semantics.rs. Squash the stack into the feature commit(s) and drop the empty ones so every commit builds. - Stale PR description [minor]: the "CI 修复验证" section still describes the Chromium transport-retry smoke change, which is no longer in this PR, and reports CI and test counts (339) for the old head
b8108989. Update it for014debd2. On the new head, Nextest is still in progress and the release builds are queued, so CI isn't green yet. Please also note the earlier close/reopen in the description, so reviewers know the stack is still live.
Automated review by Claude Code.
014debd to
b94d257
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Round-3 re-review of the updated version (0783f7d93)
This PR keeps author TeX across transparent inline wrappers, passes filtered MathML through as inline HTML, emits <sup>/<sub>, drops interactive controls from code and keeps multiline alt/title attributes block-safe. The new head 0783f7d93 is the round-2 code rebased onto v3-1334, plus b94d257c5 (fixes for the round-2 findings) and 0783f7d93 (rustfmt).
Verdict: request changes. All seven round-2 inline findings are fixed, and each new test would fail on the old code. One of the fixes brings back something the PR tests against: when the < spacing is skipped inside \text{}/\verb, HTML in a math span is emitted live again (inline at math.rs:229). I suggested "skip or document" for that nit last round without pointing out the trade-off. That's on me. This is the last round from me: everything I found is in this review.
| Previous finding (round 2 unless noted) | Status | Evidence |
|---|---|---|
[minor] A % comment next to a single $ left a space ($a%b$ -> $a $) |
FIXED | math.rs:214-219 drops a pending space at the opener, and the trailing push is gone. $a%b$, $5%$ and $%c\nx$ are asserted as InlineMath. New side effect: a trailing control space \ is now dropped (inline at math.rs:210). |
[nit] \verb / \text{} normalization |
FIXED, with a regression | Verbatim runs are copied as-is and \text{/\mbox{ depth is tracked (math.rs:143-199). Tested. The exemption re-opens raw HTML (inline at math.rs:229). |
[minor] $ unencoded in MathML text |
FIXED | mathml.rs:205 encodes $, and the <mi>$</mi>...<mi>$</mi> case asserts that no math event appears. |
[minor] plain_text never escaped $ |
FIXED | writer.rs:157 escapes $. $[a](b)$ and $x<a>l</a>y$ produce no math and keep the link. The price tests are updated. Undocumented output change: see below. |
| [nit] Stale moved test comments | FIXED | inline_boundaries.rs:260,283 now describe the protected behavior. |
[nit] Raw-string \+newline fixture |
FIXED | review_semantics.rs:104 now has a real newline. |
[nit] format! per encoded char |
FIXED | write! into the output (mathml.rs:206). |
[nit, rounds 1-2] Description doesn't call out the <sup>/<sub> output change |
NOT FIXED | The description still says only "sup/sub 保留 HTML 语义". |
[minor, round 2 summary + stack #11] Empty ci: commits, a commit that doesn't compile, a mis-titled commit |
NOT FIXED | 9bafbdf34 and 49890f527 are still empty. 5e47ad5b2 still moves struct Counted out of scope of its second user (fixed only in 381ae9c9c, which is still titled "...transport retries"). |
| [minor, round 2 summary] Stale PR description | NOT FIXED | The "Chromium 连接重置 smoke" paragraph still describes a change this PR no longer contains. |
New findings
- [major]
$\text{<img src=x onerror=alert(1)>}$and$\verb|<script>|$reach the output as live raw HTML for non-math CommonMark readers. This breaks the guarantee indollar_wrapped_literal_html_remains_inert_without_a_math_extension(math.rs:229). - [minor, missed in rounds 1-2]
\(...\)and\[...\]spans are emitted raw. CommonMark treats\(,\),\[and\]as backslash escapes, so every reader renders(x_i + \alpha)and drops the delimiters. The base output (\\() kept them visible to MathJax/KaTeX auto-render (writer.rs:119). - [minor, introduced by this round's edge-space fix] A trailing TeX control space is dropped, and its backslash then escapes the closing delimiter:
\(x\ \)->\(x\\),$$a\ $$->$$a\$$(math.rs:210). - [nit] A payload that is all comment normalizes to nothing:
$%x$->$$, which can open display math with a later$$(math.rs:106). - [nit] The
\text{/\mbox{list is ad hoc.\textrm{},\textbf{},\texttt{},\hbox{}and\text {still get the visible<. The pending-space flush is copied three times, and the doc comment doesn't describe the exemptions (math.rs:179). - [nit] An unterminated
\verbcopies the rest of the span verbatim, raw newlines included, which brings back the block-break bug from round 1 ($$\verb|a\n- b$$) (math.rs:169).
Not anchored to a diff line
- Commit hygiene [minor] (missed in round 2): the round-2 head failed
turndown::upstream_036.ec8031df2makes image alt/title newlines unconditional (), butexpectations.jsonkeptuntilb94d257c5, a commit about dollar boundaries. Soec8031df2..29e1c56d8fail the suite, andb94d257c5failscargo fmt --checkuntil0783f7d93. Together with the items in the table: squash into the feature commit(s), drop the empty commits and check that every commit builds and passes tests. - PR description [minor]: remove the Chromium smoke paragraph. Document the two downstream output changes: footnote refs and ordinals now come out as
<sup>...</sup>, and every literal dollar is now\$in the raw Markdown (\$5,\$unfinished), which matters for consumers that read the Markdown unrendered (search, LLM input). The README (moli-html2md/README.md:135-137) mentions neither. - CI: on
0783f7d93, Clippy is running and Nextest and both release builds are queued, so the "350 tests pass" claim isn't confirmed by CI yet. Format, CDP smoke, Python/Node harness and Windows storage pass.git merge-treewith origin/main and withv3-1334is clean. - Stack review: #11 (hygiene) is the only stack item owned by this PR, and it's not fixed (see above). The
role="button"whole-value check inmachine.rsbelongs under stack #10's singlerole_tokens()helper whenever that lands. I'm not raising it again separately.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
0783f7d to
f3c46b4
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Round-4 re-review of the updated version (f3c46b402)
This PR keeps author TeX across transparent inline wrappers, passes filtered MathML through as inline HTML, emits <sup>/<sub>, drops interactive controls from code and keeps multiline alt/title attributes block-safe. Round 4 head f3c46b402 is the round-3 code rebased unchanged onto v4-1334, plus one new commit, f3c46b402 ("validate normalized math before emitting delimiters"). The rest of the range-diff is a blank line that 266c101fe adds and f3c46b402 removes.
Verdict: request changes. The new commit closes almost every raw-HTML path and the \(/\[ delimiter loss. One path is still open: the \verb delimiter character is never checked, so $\verb<img src=x onerror=alert(1)><$ still produces a live <img> for non-math CommonMark readers (inline at math.rs:165). It's a one-line fix. Everything else in this review is minor or a nit. This is my final pass, and it lists everything I found.
| Previous finding (round 3 unless noted) | Status | Evidence at f3c46b402 |
|---|---|---|
[major] Live raw HTML inside \text{}/\mbox{}/\verb |
PARTIAL | Closed: \text{<img…>}, < right after {, nested braces (\text{a{<b>}}: the depth goes to 2 and math.rs:235 rejects), \textbf/\textit/\texttt/\hbox{…}, \text { (math.rs:185-212), \verb|<b>| and \verb*|<b>| (:171), an unterminated \verb (:180) and an unclosed \text{ (:251). Entity-decoded < (<, <) is decoded before normalized_span and so takes the same path. A double-encoded &lt; stays inert text. Outside text mode, every <+[A-Za-z/!?] becomes < (:238). Still open: the \verb delimiter itself (:165-168), see the inline comment. |
[minor] \(…\) and \[…\] delimiters lost to CommonMark escapes |
FIXED | Spans are rewritten to $…$ or $$…$$ (math.rs:254-260). The continue bypass is gone, and \(x_i\)2 and \[y_1=2\] are asserted through ENABLE_MATH. Side effects are in nits 3 and 4 below. |
[minor] Trailing control space dropped, \(x\ \) -> \(x\\) |
FIXED | math.rs:225-226 pushes the space right away. $$a\ $$ and \(x\ \) are asserted, and both fail on v3. |
[nit] Comment-only payload normalizes to $$ |
FIXED | math.rs:251 rejects an empty output, and acceptance now runs normalization (:117-123). $%a$ b $%c$ is asserted to produce no math event. |
[nit] Ad-hoc \text{/\mbox{ list, copied space flush, stale doc comment |
FIXED | There is now one TEXT_COMMANDS table with optional whitespace before {, a single flush_space, and an updated doc comment. Test gap: see nit 2. |
[nit] Unterminated \verb copies raw newlines |
FIXED | math.rs:171-182 rejects on \n/\r or a missing closer. $$\verb|a\n- b$$ is asserted to produce no <li> and no math. |
[nit, rounds 1-3] Description doesn't mention the <sup>/<sub> output change |
NOT FIXED | The description still says only "sup/sub 保留 HTML 语义". |
| [minor, rounds 2-3 and stack #11] Commit hygiene | NOT FIXED, and worse | The empty commits aa5fafed8 and 6ba81c991 are still there. 102bca6ae still doesn't compile alone. 957d1be5b is still titled "fix(ci) … transport retries" but only touches review_semantics.rs. cda46464f..957d1be5b still fail turndown::upstream_036. New: 266c101fe adds a blank line before } (mathml_operators_survive…), and f3c46b402 removes it, so 266c101fe..654ea498e now fail cargo fmt --check. |
| [minor, rounds 2-3] Stale description and undocumented output changes | NOT FIXED | The "Chromium 连接重置 smoke" paragraph is still in the description. Three output changes are documented in neither the description nor README.md:135-137: footnote refs and ordinals now come out as <sup>, every literal $ is now \$, and \(…\)/\[…\] are now rewritten to $/$$. The description mentions the third one only in passing. |
New findings
- [major] The
\verbdelimiter is never checked byhtml_start. A<delimiter followed by a letter emits live HTML:<p>$\verb<img src=x onerror=alert(1)><$</p>produces$\verb<img src=x onerror=alert(1)><$(math.rs:165). - [nit] The
\textrm{a<2}case passes on v3 as well, because<2never triggershtml_start. Changinga<btoa<2left no test that exercises text-mode rejection beyond\text{. Nested braces,\textbf/\hbox,\verb*and the\verb<delimiter are untested (review_semantics.rs:461). - [nit] Inline
\(…\)becomes display math in pulldown and GitHub when it ends in a control space (\(x\ \)->$$x\ $$). The digit-follow fallback I suggested last round (\(x_i\)2->$$x_i$$2) does the same. The new test acceptsInlineMath | DisplayMath, so it hides the mode change (review_semantics.rs:452). - [nit] Adjacent converted spans produce runs of dollars:
\(a\)\(b\)->$a$$b$, and\[a\]\[b\]->$$a$$$$b$$. Pulldown splits these correctly, but run-length readers (remark-math / micromark-extension-math) read$a$$b$as one formula,a$$b(writer.rs:119). - [nit] Every accepted span is normalized twice, once in
accept_candidateand once in the writer. The writer also recomputesfollowed_by_digitand unwraps with.expect, so the two call sites must stay in sync forever (writer.rs:119-125).
Not anchored
- CI on
f3c46b402: Format, Clippy, CDP smoke, the Python/Node harnesses and Windows storage pass. Nextest and both release builds are still queued, so the "352 项通过" claim isn't confirmed by CI yet.git merge-treeagainstv4-1334and againstorigin/mainis clean. - Stack: #11 (hygiene) is the only stack item owned by this PR, and it's in the table above. I'm not raising anything else from the stack review here.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
f3c46b4 to
396960e
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Round-5 re-review of the updated version (396960e42)
This PR keeps author TeX across transparent inline wrappers, passes filtered MathML through as inline HTML, emits <sup>/<sub>, drops interactive controls from code and keeps multiline alt/title attributes block-safe. Round 5 head 396960e42 is the round-4 code plus one new commit, 396960e42 ("keep math delimiters safe and independent"). The 404a9f6a1 amend only changes diff context in review_semantics.rs. The blank line it adds is unchanged.
Verdict: approve. The \verb< bypass is closed. I traced every path that can push a < in normalized_span and found no remaining way for raw HTML to reach a non-math reader inside an accepted span. What's left is nits, plus the long-standing hygiene and description items, which a squash merge and a description edit fix. Approval assumes Nextest goes green: it is still queued on 396960e42.
| Previous finding | Status | Evidence at 396960e42 |
|---|---|---|
[major, r4] \verb delimiter bypasses html_start |
FIXED | math.rs:165 rejects a < delimiter, and * is consumed before that, so \verb*< is rejected too. $\verb<b>x<$ and $\verb*<img src=x>a<$ assert no math event and no <b>/<img in non-math HTML. Both fail on v4. |
[major, r3] Raw HTML inside text commands/\verb |
FIXED | Its last open path was the \verb< above. See the adversarial sweep below. |
| [nit, r4] Text-mode test gap | FIXED | \textbf, \hbox {, nested braces, unclosed \text{, \verb*|…| and the < delimiter are all in the rejection list (review_semantics.rs:501-507). Each one checks math events and rendered HTML. |
[nit, r4] InlineMath | DisplayMath hides mode change |
FIXED | The event kind is now asserted per case. \(x\ \) stays inline as $x\ {}$ (math.rs:253-257). The digit fallback is asserted as display and documented. README wording nit below. |
| [nit, r4] Adjacent spans form dollar runs | PARTIAL | Fixed within one pending text run (writer.rs:119). Still open across a flush boundary: <p>\(a\)<b></b>\(b\)</p> -> $a$$b$. See the inline comment. |
[nit, r4] Double normalization / .expect |
FIXED | spans() now returns the normalized payload, and the second pass and .expect are gone. Leftover: the digit probe is still computed twice in accept_candidate (inline nit). I accept declining the command-name allocation fix. |
[nit, r1-4] Description omits <sup>/<sub> output change |
NOT FIXED | The description still says only "sup/sub 保留 HTML 语义". |
| [minor, r2-4] Stale description / undocumented output changes | PARTIAL | The \(…\)/\[…\] -> $/$$ rewrite is now in the description and README.md:140-143. The "Chromium 连接重置 smoke" paragraph is still there. Footnote refs/ordinals as <sup> and every literal $ as \$ are still undocumented. |
| [minor, r2-4, stack #11] Commit hygiene | NOT FIXED | The empty commits 2f69186f5 and 0abaab870 are still there. cfa2e9917 is still titled fix(ci) … transport retries but only touches tests. 404a9f6a1 still adds the blank line that 5eb7ce139 removes, so 404a9f6a1..125acbbef fail cargo fmt --check. 014bf0852 doesn't compile alone, and 3749dbf4a..cfa2e9917 fail turndown::upstream_036. A squash merge fixes all of this. |
Adversarial sweep (no finding). normalized_span pushes < in only three places: the main loop (:232-236), \verb content (:169-172) and the \verb delimiter (:165, now filtered). In the main loop, html_start looks at the next raw character. I checked every case where the next output character could differ from it. Whitespace and % comments become a flush_space space, or nothing at the end of the payload, so the output is < x, <$ or <$$. A following \verb/\text{ emits its \ first. The appended {} only follows whitespace. Inside \verb, the peek is the real next output character, including the closer (\verb/a</ is rejected). Escapes (\<b -> \< b, \\verb|<b>| -> \\verb|< b>|) and nested $…$ inside \text{} (still text depth > 0) are covered. So are $, $$, \(, \[ and overlapping candidates, because each retained candidate was normalized on its own. Entity-decoded < takes the same path, and raw < in a payload only decodes to text. Across span edges: plain text always escapes < and $, the <!-- --> separator is a closed comment that never sits at line start (so it can't open an HTML block), and MathML escapes text and attribute values and emits only allow-listed tag names (mathml.rs:91,106-118,202-217).
New findings
- [nit] Cross-flush adjacency and digit lookahead (
writer.rs:119). - [nit] README wording on the digit fallback, and the "remain separate" overclaim (
README.md:141). - [nit] The digit probe is still duplicated in
accept_candidate(math.rs:116).
Not anchored
- CI on
396960e42: Format, Clippy, CDP smoke, Python/Node harnesses and Windows storage pass. Nextest and both release builds are queued, so the "354 项通过" claim isn't confirmed by CI yet.git merge-treeagainstv5-1334andorigin/mainis clean. - Stack: #11 (hygiene, in the table) is the only stack item owned by this PR.
Verdict: approve (posted as a comment; self-approval is not allowed).
Automated review by Claude Code.
396960e to
ceecd7f
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Final re-review (P0/P1 only) (ceecd7f6d)
Verdict: approve. No P0/P1 issues found. Head ceecd7f6d is the round-5 code plus two commits: 9121dd968 ("preserve math boundaries across empty wrappers") and ceecd7f6d (README). The range-diff shows commits 1-11 unchanged.
| Previous P0/P1 finding | Status | Evidence at v6 |
|---|---|---|
[major, r4] \verb delimiter bypasses html_start |
FIXED (unchanged) | math.rs:163-165 still rejects a < delimiter, after consuming *. The rejection tests in review_semantics.rs are still there. |
[major, r3] Raw HTML inside text commands/\verb |
FIXED (unchanged) | 9121dd968 touches normalized_span in one place only: the followed_by_digit parameter is gone and the marker is now $ or $$ by mode alone. None of the < push sites (main loop, verb body, verb delimiter) changed. |
New commit check (no finding).
- No raw-HTML path reopened. The only new output is the existing
<!-- -->separator, now also placed before a digit. It is emitted only whenmath_end == output.len(). Every normalized span ends in$, andoutputonly grows between resets (block()resetsmath_end;finish()consumes the writer). So the separator always follows a$mid-line. It can't sit at line start and open an HTML block. - Stale offsets are harmless. The emphasis-to-HTML
replace_rangecan leavemath_endstale. It always grows the string and is followed by a closer, so a stale value is belowlenfrom then on. It can't drop a needed separator, and at worst it adds an invisible one. - Pairing stays correct. Every other path that can follow math changes
lenbefore the check:prepare_inlinespaces, openers and closers, links,<sup>, inline HTML, images and code. Examples:<b>\(x\)</b>2-><strong>$x$</strong>2,\(x\)<b>2</b>->$x$**2**. Child writers start withmath_end = None. - Digit handling.
\(x_i\)2now stays inline as$x_i$<!-- -->2, and the test was updated todisplay: false. Same-run$x$2is still rejected as currency-like text. - Unchanged behaviour.
prepare_inline('$')gives the same edge decisions as the old raw\/$, since both are punctuation. - Tests. The new test parses all six cases with
ENABLE_MATHand asserts theInlineMathevents.
The round-5 nits (cross-flush adjacency, README wording, duplicate digit probe) are also resolved. Squash-merge hygiene items from earlier rounds are unchanged and below P1.
CI on ceecd7f6d: Format, Clippy, CDP smoke, Python/Node harnesses, Windows storage and Resolve revisions pass. Nextest and both release builds are still pending, so approval assumes they go green. Mergeable; git merge-tree against v6-1334 is clean.
Verdict: approve (posted as a comment; self-approval is not allowed).
Automated review by Claude Code.
ceecd7f to
9fc264a
Compare
9fc264a to
6ddd9ba
Compare
6ddd9ba to
6166aea
Compare
问题
普通 Markdown 转义会破坏 TeX 命令和下标；透明内联包装可能割裂同一公式。MathML、上下标和嵌套格式也需要保留原有语义。代码块中的交互按钮则可能混入源代码。
一图看懂
修改
导出图片 alt 和链接/图片 title 时，将 HTML 空白折叠为普通空格，避免模板换行变成
；保留 Unicode 空格和字面实体。在实际格式边界处理连续文字，保留完整 TeX。输出采用支持数学扩展的 Markdown；是否有数学框架 class 不影响作者公式的保留。
MathML 保留经过筛选的数学结构，并排除重复视觉副本；sup/sub 保留 HTML 语义。
代码内按 button、表单控件和 role=button 排除交互内容，保留普通 class 包装中的真实代码。移除 copy/btn 名称猜测。
增加公式在图片、代码、链接和块边界两侧的顺序验收。
在接受公式前检查规范化结果：注释移除后为空、未闭合 verb 或文本命令中含 HTML 标签的片段，转为安全的普通文字。
保留 TeX 控制空格；括号与方括号分隔符转换为 Markdown 数学扩展能识别的美元分隔符，测试直接检查解析后的公式载荷。
公式分隔以实际输出位置为准：空包装与合并样式不会粘连相邻公式，紧接数字的括号公式也保持行内。不可见注释把闭合美元符号与下一段分开。
verb的<分隔符及文本命令里的标签会安全降级为普通文本；规范化结果随已接受区间传递，Writer 不再重复解析。验证
cargo fmt --all、cargo clippy --workspace --all-targets --all-features -- -D warnings、cargo nextest run --no-fail-fast均通过；全工作区 20,812 项通过、19 项跳过。cargo test -p moli-html2md --locked通过。数学扩展解析器验证 TeX 注释边界、verb 原文、text/mbox 文本、字面美元符号和 MathML，确保保留的公式可解析、普通文本不会意外成为公式。#1335 的转换库测试共 372 项通过。
Chromium 连接重置 smoke 按一次逻辑请求验证完整 CDP 事件、请求头和错误；底层传输允许 Chromium 实际支持的一至两次尝试，Moli 仍限定一次。
复现
本 PR 的转换库测试可独立运行：
cargo test -p moli-html2md --locked。浏览器接入在 #1337，以下 CLI 与冻结页复现使用完整 stack。最小用例
两个公式不能粘成一个，公式后的数字仍在公式外，Copy 按钮不属于代码。
实际输出：
真实 WebMainBench 页面
bfaf0844…Jacobian 文章：保留\frac与矩阵公式的 TeX 载荷。七个相关页面通过 browser-eval 的现有 runner 共用一次定向运行，7/7 正常导出，并核对上述内容；不是 545 页质量重测。查看冻结输入、实际输出与核对结果。
19bfc785；二进制 SHA-256：1c17bba8cb6b0a532144697411d13ec6a3872d1abad96d5872da0ded93d7b8b4。345ea093。0efaa4b49a45e320a27fe6e5a0b6aad5b57259fc3321ac3448519cacc74c537e，runner 会校验。保留原网址，代理返回冻结 HTML，外部资源禁止访问，无点击或 DOM 改写。环境准备与可复制命令
results.jsonl记录成功状态、错误和输入/输出哈希，execution.json记录二进制身份，artifacts/moli/保存实际 Markdown。定向运行只用于定位和回归，不进入正式榜单；报告中的历史成绩仍绑定原测量版本。依赖
直接基于 #1334，本 PR 的 diff 只含公式与格式增量。#1334 → #1335 → #1336 → #1337 → #1380