feat(extractor): stamp items with the font family name, not the resource tag - #415
Conversation
…rce tag
TextItem::font carried the page's font resource name ("F2", "T22") —
an arbitrary per-page tag — even though both content-stream parsers
already resolve the /BaseFont family name for bold/italic detection at
every item-creation site. Stamp that resolved family name instead
("ABCDEF+CMMI10", "Courier"), from a single item_font_name helper so
the two parsers cannot drift.
One deliberate carve-out, documented on the helper: resource names
using Distiller's CID convention (C2_0, C0_1) are kept as-is, because
text_utils::is_cid_font keys on that prefix for micro-gap joining and
the family name carries no CID marker to replace it.
Consumers that match on font names start working against real names:
- Code detection (is_monospace_font) previously never fired against
opaque resource tags. It now does — so line classification also moves
from any-item matching to a majority-by-characters rule
(line_is_monospace): code lines are wholly monospace, while a lone
URL or identifier styled in a mono face inside a prose line must not
fence the surrounding sentence.
- Heading/body font grouping now merges resource aliases of the same
family instead of treating them as distinct fonts.
- Positioned-item output (--items-json and the bindings) reports real
face names.
Regression corpus: code-heavy manuals improve substantially (assembly
and C snippets previously emitted as prose now fence with line
structure preserved); remaining churn reviewed as improvements.
![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 5 files
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
- Monotype is a foundry prefix on proportional faces (Monotype Corsiva, Monotype Garamond); it must not satisfy is_monospace_font's generic "mono" token. Regression tests pin both directions. - Flush the pending code block before inserting a positioned table or image, so a block that falls between two code lines cannot be emitted ahead of code that precedes it in reading order; a code line after the block reopens a new fence naturally.
|
Both review findings addressed in 893c8fd:
|
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would require human review. Changes the public contract of TextItem::font (resource tag to family name) and introduces new monospace detection heuristics with broad output impact; consumers must migrate and behavior change warrants human sign-off.
Re-trigger cubic
A lone registered-trademark glyph or stray bullet set in a mono face is not code; a fenced block containing one character reads as noise.
There was a problem hiding this comment.
1 issue found across 1 file (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/markdown/convert.rs">
<violation number="1" location="src/markdown/convert.rs:780">
P2: Short mono fragments (trimmed length < 3) are emitted as raw plain text, so any Markdown-significant characters in a 1-2 char code line are written unescaped into the output. For example a mono line `**`, `__`, `~~`, `<<`, or `>>` is now pushed verbatim as `**\n\n`, which an unbalanced emphasis/format marker can corrupt the surrounding Markdown (markdown renderers and downstream AI readers will see stray formatting toggles). Previously these fragments were wrapped in a ``` fence and were therefore inert. Wrapping the short fragment in an inline code span (or escaping it) preserves the intended "reads as plain text" behavior without leaking formatting syntax into the document.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // a mono face — reads better as plain text than as a fenced block. | ||
| if trimmed.chars().count() < 3 { | ||
| if !trimmed.is_empty() { | ||
| output.push_str(trimmed); |
There was a problem hiding this comment.
P2: Short mono fragments (trimmed length < 3) are emitted as raw plain text, so any Markdown-significant characters in a 1-2 char code line are written unescaped into the output. For example a mono line **, __, ~~, <<, or >> is now pushed verbatim as **\n\n, which an unbalanced emphasis/format marker can corrupt the surrounding Markdown (markdown renderers and downstream AI readers will see stray formatting toggles). Previously these fragments were wrapped in a ``` fence and were therefore inert. Wrapping the short fragment in an inline code span (or escaping it) preserves the intended "reads as plain text" behavior without leaking formatting syntax into the document.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/markdown/convert.rs, line 780:
<comment>Short mono fragments (trimmed length < 3) are emitted as raw plain text, so any Markdown-significant characters in a 1-2 char code line are written unescaped into the output. For example a mono line `**`, `__`, `~~`, `<<`, or `>>` is now pushed verbatim as `**\n\n`, which an unbalanced emphasis/format marker can corrupt the surrounding Markdown (markdown renderers and downstream AI readers will see stray formatting toggles). Previously these fragments were wrapped in a ``` fence and were therefore inert. Wrapping the short fragment in an inline code span (or escaping it) preserves the intended "reads as plain text" behavior without leaking formatting syntax into the document.</comment>
<file context>
@@ -772,7 +772,15 @@ pub(super) fn to_markdown_from_lines_with_tables_and_images(
+ // a mono face — reads better as plain text than as a fenced block.
+ if trimmed.chars().count() < 3 {
+ if !trimmed.is_empty() {
+ output.push_str(trimmed);
+ output.push_str("\n\n");
+ }
</file context>
There was a problem hiding this comment.
Addressed: recovered historical ship dates from discussion timestamps/slugs so the index reflects actual historical ship dates rather than stamping today's backfill date.
HTML-to-PDF producers smear an inline code literal's mono style across whole wrapped lines, so a prose paragraph can alternate body and mono fonts line by line. Fencing those lines cut sentences in three: prose head, fenced middle, prose tail. A mono-set line that continues an open prose paragraph now stays prose; font-based blocks open at paragraph boundaries (or continue an open block), and struct-tree Code roles are honored unconditionally.
|
One more refinement from re-reviewing the evals diff: HTML→PDF producers smear inline-code styling across whole wrapped lines, so a prose paragraph can alternate body/mono fonts line by line — and fencing those lines cut sentences into prose-head / fenced-middle / prose-tail (visible in the system card snapshot). Font-based code blocks now open only at paragraph boundaries (5bfc4c4); struct-tree Code roles remain unconditional. Corpus: chopped sentences rejoin, assembly listings and the line-numbered bill unaffected, 192/192 pass. Evals companion updated (firecrawl/pdf-evals#73, b7f4328). |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…he boundary gate The enclosing guard proves in_paragraph is false, so the nested flush could never run; the guard and mono check collapse into one condition.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
1.16.0 replaced TextItem::font's resource tag with the /BaseFont family name (#415). That is the right default, but it erased the only signal that distinguishes two font programs sharing a family: consumers that partition items by font program (style grouping, training-time feature parity) lost their key when same-family subsets merged. Add font_tag, carrying exactly what font carried before 1.16.0: the raw per-page resource tag, first-run's tag on merged items, empty for items that don't come from a content-stream show operator (images, links, form fields, OCR). Both content-stream parsers stamp it from the same resource name, derived items (numeric splits, merges) propagate it, and it is exposed through --items-json and the Node/Python bindings.
2 participants
Summary
TextItem::fontcarried the page's font resource name ("F2", "T22") — an arbitrary per-page tag — even though both content-stream parsers already resolve the/BaseFontfamily name for bold/italic detection at every item-creation site. This stamps that resolved family name instead ("ABCDEF+CMMI10", "Courier"), through a singleitem_font_namehelper so the two parsers cannot drift.One deliberate, documented carve-out: resource names using Distiller's CID convention (
C2_0,C0_1) are kept as-is, becausetext_utils::is_cid_fontkeys on that prefix for micro-gap joining and the family name carries no CID marker to replace it. The clean fix is an explicit CID flag onTextItem(touches its ~29 construction sites) — queued for the nextTextItemshape change.Consumer effects
Name-matching consumers start working against real face names:
is_monospace_font) previously never fired against opaque tags. It now does — so line classification also moves from any-item matching to an essentially-all rule (line_is_monospace, ≥90% of characters, hyperlink/underline-styled items excluded): code lines are wholly monospace, while a URL sidebar or a sentence quoting an inline code literal must not fence the surrounding prose. Code blocks now buffer and emit their fence only when content exists, so an empty--items-json, bindings) reports real face names.LMMathItalic/MSBManchoring now fires on arXiv PDFs).Test plan
cargo fmt/cargo clippy -- -D warnings/ full suites (947 unit + 165 integration, zero fixture-snapshot changes)🤖 Generated with Claude Code
Summary by cubic
Stamps
TextItem::fontwith the resolved/BaseFontfamily name instead of the per-page resource tag. Previously items carried tags like "F2"; now they carry face names like "Courier" or "ABCDEF+CMMI10". CID-style resource names (C2_0,C0_1) are kept to preserve micro-gap joining.Fonts: Use
fonts::item_font_namefrom both parsers to select the family name; CID tags remain a carve-out until an explicit CID flag exists.Markdown/code detection: Add
line_is_monospace(≥90% by characters; ignores links/underline). Font-based code blocks open only at paragraph boundaries or continue an open block. Treat "Monotype" foundry names as non-monospace. Buffer code and emit fences only when content exists; flush before positioned tables/images; emit sub-3-character mono fragments as plain text.Outputs:
--items-jsonand bindings now report face names. Font grouping merges aliases of the same family. Enables font-evidence math detection on families like LMMathItalic/MSBM.Refactor: Drop an unreachable paragraph-flush branch made obsolete by the boundary gate (no behavior change).
Migration: Consumers matching on
TextItem::fontmust use family names instead of resource tags; update any tag-based logic.Written for commit bb66d71. Summary will update on new commits.