feat: add MHTML support - #149
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
af6e811 to
049b63a
Compare
|
@cubic-dev-ai review this pull request |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/formats/mhtml.rs">
<violation number="1" location="src/formats/mhtml.rs:152">
P3: The new `html_resource_base` re-parses the entire root HTML with `Html::parse_document`, and `collect_stylesheets_in_order` parses the same string again immediately after (plus a third parse inside `parse_text_with_context`). Parsing the document twice for resource-base extraction adds a redundant full-DOM-build of a potentially large HTML root. Parse the document once in `parse()` and pass the parsed root (or the extracted base) into both helpers to avoid the duplicate parse.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai Please re-review the two still-open older threads on this PR against the current head "f23fdfdddf6ec2de475c4727bb2455ec8b11a956":
The current head has passed the complete AnyDoc validation gate, and the real 505,883-byte Chrome/Blink MHTML fixture was also converted directly and validated successfully. Please verify whether these two findings still apply to the current head and resolve/update the old threads accordingly if they are now addressed. |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/formats/mhtml.rs">
<violation number="1" location="src/formats/mhtml.rs:158">
P2: When an MHTML document contains `<base href="">`, relative embedded resources stop resolving against the root `Content-Location`. Treat an empty resolved base as absent so the root location remains the fallback.</violation>
<violation number="2" location="src/formats/mhtml.rs:418">
P2: When an embedded image or stylesheet URL includes a fragment, the lookup retains the fragment even though MIME parts are keyed by the underlying resource URL. Strip fragments for embedded-resource matching while preserving them for external image URLs.</violation>
<violation number="3" location="src/formats/mhtml.rs:424">
P2: When an MHTML root has no usable URL base, protocol-relative images are dropped because `is_absolute_uri` rejects `//host/path`. Preserve network-path references as external images, matching the standalone HTML frontend.</violation>
</file>
<file name="src/formats/html.rs">
<violation number="1" location="src/formats/html.rs:246">
P2: Alternating 257 heading tags triggers `max_xml_depth` before HTML5 parsing can repair them, because `close_implied_before_start` does not close prior `h1`–`h6` elements. Track heading tags as mutually implied-closing elements so malformed HTML is not rejected solely by the preflight stack approximation.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
17fe665 to
32c6474
Compare
|
@cubic-dev-ai Please start a fresh AI code review of the current rewritten head |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/formats/html.rs">
<violation number="1" location="src/formats/html.rs:64">
P2: Valid frameset HTML has no `<body>`, so this standalone parser returns `Malformed` for a supported HTML5 document. Handle a frameset/no-body tree as an empty convertible document or convert its frame sources instead of unconditionally erroring.</violation>
<violation number="2" location="src/formats/html.rs:493">
P2: When standalone HTML uses a relative `<img src>` without alt text, `StandaloneCtx::image_source` drops the image instead of preserving its relative URL. Return a URL-bearing image source for nonempty relative references so Markdown retains the image.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| } | ||
| } | ||
|
|
||
| let body = root |
There was a problem hiding this comment.
P2: Valid frameset HTML has no <body>, so this standalone parser returns Malformed for a supported HTML5 document. Handle a frameset/no-body tree as an empty convertible document or convert its frame sources instead of unconditionally erroring.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/formats/html.rs, line 64:
<comment>Valid frameset HTML has no `<body>`, so this standalone parser returns `Malformed` for a supported HTML5 document. Handle a frameset/no-body tree as an empty convertible document or convert its frame sources instead of unconditionally erroring.</comment>
<file context>
@@ -0,0 +1,530 @@
+ }
+ }
+
+ let body = root
+ .descendent_elements()
+ .find(|e| e.value().name() == "body")
</file context>
| if src.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| Ok((is_absolute_uri(src) || src.starts_with("//")) |
There was a problem hiding this comment.
P2: When standalone HTML uses a relative <img src> without alt text, StandaloneCtx::image_source drops the image instead of preserving its relative URL. Return a URL-bearing image source for nonempty relative references so Markdown retains the image.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/formats/html.rs, line 493:
<comment>When standalone HTML uses a relative `<img src>` without alt text, `StandaloneCtx::image_source` drops the image instead of preserving its relative URL. Return a URL-bearing image source for nonempty relative references so Markdown retains the image.</comment>
<file context>
@@ -0,0 +1,530 @@
+ if src.is_empty() {
+ return Ok(None);
+ }
+ Ok((is_absolute_uri(src) || src.starts_with("//"))
+ .then(|| ImageSource::External(src.to_owned())))
+ }
</file context>
1 participant
Scope
Adds MHTML/MHT parsing and conversion, completing the MHTML portion of #52.
This head is explicitly reconciled with standalone HTML PR #147 head
c6b7bb18608d4bde426a71d9a5300070d8f653fc.The reconciled functional commit
32c647468193fbff6e4d21b07ab8395295e79e75is a direct child of that #147 head. Its tree is48566f9d8eee0e9836c7b48a05ea05c827a1a1ec, exactly the same tree as the previously validated/reviewed #149 head17fe665d8f921bb1a881bb33a5ec4b87ceb43dd5. The reconciliation therefore changes ancestry only; it does not change the MHTML/HTML code content.Summary
Format::Mhtmlwith.mhtml/.mhtsupportmultipart/relatedarchives without classifying ordinary multipart email as MHTMLstartContent-ID,Content-Location, relative URLs,<base href>, and case-insensitivecid:references#147 reconciliation and realistic corpus
The branches were reconciled semantically rather than by blindly taking either side's HTML implementation. The current #149 commit is now directly based on the validated #147 head, while retaining the exact content tree of the previous #149 head. This proves the latest #147 anchor-recovery change was already present semantically in #149 and no MHTML-specific behavior was lost during the ancestry rewrite.
The #149 tree includes the shared HTML list fixes from #147, its controlled/LibreOffice HTML corpus, HTML list/corpus regressions and snapshots, while preserving the MHTML-specific MIME, charset, resource-resolution, CSS, and asset behavior.
tests/mhtml_corpus.rsreuses the #147 fixtures. It validates byte-for-byte standalone HTML vs MHTML invariance where MIME should not change semantics, base64 UTF-8 roots, realistic linked CSS and embedded images, deliberate relative-image resolution through MHTMLContent-Location, and conservative handling of ordinary MIME messages.Review follow-up fixes
The current head includes the earlier review fixes plus the latest Cubic follow-up:
Content-IDhas the same bare text; Content-ID resources requirecid:semanticsContent-Locationare removed from the resource-index key, matching fragment-stripped lookup behavior<a>start tags without accumulating false nesting depthtests/html.rs, MHTML resource-resolution cases intests/mhtml.rs;tests/review_followup.rswas removedThe earlier resource-safety fix remains: decoded MHTML HTML roots run
preflight_html_complexitybefore the MHTML DOM is materialized.The previous Cubic review of the identical content tree completed with 0 new issues in the latest delta. Because the #147 reconciliation rewrote branch ancestry, Cubic intentionally skipped its automatic review of the rewritten head. A manual re-review was triggered for exact head
32c647468193fbff6e4d21b07ab8395295e79e75(check99186037778); it has reviewed all 4 files and is currently completing its final review processing.A separate P3 performance/maintainability thread about multiple HTML passes remains intentionally unchanged after audit: the bounded charset sniff, streaming pre-DOM complexity preflight, and actual HTML5 tree construction serve distinct purposes. Removing the full-document preflight would weaken pre-DOM resource enforcement; combining it with HTML5 tree construction would require a substantially larger parser/tree-sink refactor.
Fresh GitHub Actions validation
Validated functional SHA:
32c647468193fbff6e4d21b07ab8395295e79e75.Parent HTML SHA:
c6b7bb18608d4bde426a71d9a5300070d8f653fc(current validated head of #147).Temporary CI-only branch:
verify/mhtml-reconciled-c6b7bb1-full.Temporary CI-only commit used to launch the authoritative gate:
156d6d858e1d5349e8716e278b329830187a071f. This commit is not part of the PR code; its sole parent is the validated functional SHA above, and the workflow explicitly checks out that SHA detached before testing.Authoritative verification: run
33284379175, job99184901026— success.html,html_corpus,html_list,mhtml,mhtml_charset,mhtml_corpus)cargo fmt --all -- --check: passed-D warnings: passedcargo test --locked: passednpm ci, build, tests, and generated binding determinism: passedsite-packagesmodule, full unittest suite: passedThe current PR head is exactly the validated functional SHA above.
Real MHTML validation
The authoritative run reconstructed and SHA-256-verified the original 505,883-byte SEI/TRE-PB Chrome/Blink MHTML fixture and converted it directly.
387c6f2d7223da224a8f55962b97eac947734c97fc84888e1e5619e93745837cmhtml�,Ã, orÂmojibake markersf6f75cec7bc06afe038bd89cd7967e5af358ec3562553f5bfc5c47c0299eb80c