verify_layout's orphan rule assumes the stock template's bbox geometry (and can go silent on a real orphan)

[yang2632]

Filing this as an Idea rather than a PR, because CONTRIBUTING is clear that a fix has to reproduce on the real path and the trigger is a template registered via /add-template, which this repo does not ship. A reproduction and a candidate patch exist; I would rather hear whether the direction is wanted than send a PR you would have to decline.

The observation

find_orphans() in tools/verify_layout.py uses left indent as its proxy for "this line is a list item":

def indented(line: Line) -> bool:
    return line.left > doc_left + INDENT_PT

That proxy is a property of the stock moderncv bbox output, not of CVs in general. The docstring already warns that a template registered via /add-template "may need them retuned", and gives the phantom-hole/footer example. The orphan rule is a different kind of case: it is not a threshold that can be retuned.

When a template emits its bullets as a separate list below the entry header rather than inside the entry, Poppler derives each word's yMin/yMax from font ascent/descent rather than glyph ink, so the bullet marker and the item's first line land in the same rounded bucket and merge into one bbox line — whose left edge is the marker's, not the text's. Continuation lines start at the item indent. Measured on a compiled PDF:

doc_left = 58.87,  INDENT_PT = 8.0
p1 last line   left = 58.87  = doc_left + 0.00   '• Laid a new cobbled path through the kitchen garden ...'   <- marker+text merged
p2 first line  left = 72.07  = doc_left + 13.21  'that rain ran off into the borders ...'              <- continuation

So indented(last) is False and indented(first) is True, and the wrapped bullet is reported as an orphaned \cventry. No value of INDENT_PT separates the two: the marker line sits below the header indent and the continuation above it, so any threshold either misses real orphans or invents them.

It cuts both ways

The false positive is the annoying half. The quiet half is worse — the rule also goes silent on a genuine orphan, which is the defect the tool exists to catch:

$ python3 tools/verify_layout.py cv_orphan_false_negative.pdf     # master
LAYOUT PROBLEMS:
  - p2 is the last page and 64% empty ...
EXIT=1        # says nothing about the orphan

Page 1 there ends on a two-line entry header with zero bullets under it. master is silent because the first bullet's bbox line is at doc_left itself, under INDENT_PT, so indented(first) is False.

Neither shape is a knife edge. Sweeping the profile-statement length over 40 content lengths on the same document, the false positive appeared at 23 of them, and the false negative at 36 consecutive lengths.

What I tried, and the control that matters

Adding a glyph test alongside the indent test — a line whose text opens with a list marker is a list item whatever its indent — clears both shapes, and is inert on the shipped templates:

$ diff <(master/verify_layout.py cv/main_example.pdf) <(patched/verify_layout.py cv/main_example.pdf)
# identical
$ diff <(master/verify_layout.py cover_letters/cover_example.pdf) <(patched/verify_layout.py cover_letters/cover_example.pdf)
# identical

Both still report exactly what they report today (the stock CV's "p2 is the last page and 77% empty", which is the by-design case named in the docstring). It is inert because the stock template wraps each \cventry in an outer itemize, so entry headers already sit at doc_left + 11.43, past INDENT_PT, and the stock bullets live inside the entry's own box, so a page break cannot separate a stock marker from its text. Across the stock CV, the stock cover letter and the two reproduction documents, only the reproduction documents' output changes.

All CI gates pass on the patched tree (470 tests, check_framework_version, security_guards).

Two things I would want reviewed in any version of this:

1. Whether the lone-marker branch should be widened (dropping not indented(last)), which would be more than a reorder. Unreachable on the stock template, but it would be a behaviour change, not a refactor.
2. Whether ASCII - and * should count as markers. A word-bearing un-indented line that merely starts with a hyphen would then suppress a real orphan report. A Unicode-only set (–—•‣▪●·) would be safer and still cover the case.

The actual question

I am not asking you to take my patch as it stands. The question is narrower:

Should verify_layout's orphan rule know what template is active? ACTIVE-TEMPLATE already declares a compile command and a page limit. Three directions I can see:

• Declare it. A template registered via /add-template states its list geometry, and find_orphans uses that instead of guessing from indent.
• Skip it. When a non-stock template is active, find_orphans reports "orphan check skipped: non-stock template" rather than guessing — the same fail-loud-not-wrong posture the -bbox degradation already takes with skipped: exit 2.
• Document it. Extend the existing docstring caveat to name the orphan rule specifically, so the next person to hit this knows it is a known limit rather than a bug — and closes this.

Happy with "document it" as the answer. The reason I am raising it at all is that the current failure is silent in the direction that matters: the false negative looks exactly like a clean document.

Reproduction files are linked in the comment below.

[MadsLorentzen]

This is the right way to raise it, thank you. A registered template is on the real path, since /apply Step 5b runs verify_layout on whatever template is active, and a silent false negative is the worse half, as you say.

Of your three directions: "declare it" is more machinery than the evidence supports yet, and "skip it" throws away a check that works for many templates. I would rather fix the proxy. Indent is a stand-in for "this line is a list item", and a line that opens with a list marker is direct evidence of the same thing. So I would take a PR with the glyph test alongside the indent test, with two limits you already named: the Unicode-only marker set (no ASCII - or *), and the lone-marker branch left as it is unless a reproduction shows it needs widening. Plus the docstring caveat naming the orphan rule. Please attach the two .tex files and PDFs here first so I can reproduce both shapes before the PR, and include the geometry as synthetic fixtures the way the existing tests do.

[yang2632]

Agreed on all four limits: Unicode-only marker set, lone-marker branch untouched, glyph test beside the indent test, and a docstring caveat naming the orphan rule. Here are the two shapes to reproduce before the PR. Both are an invented gardener's CV ("Toby Mercer"; every name, place, employer and date is made up), built on the stock moderncv class in banking style, and compile with lualatex to 2 pages. Each entry's bullets are an itemize after the \cventry, relabelled with \textbullet and given a narrow indent: a template of the kind /add-template registers, which /apply Step 5b then checks with verify_layout.

• cv_orphan_false_positive.tex/.pdf: page 1 ends on a bullet whose text wraps onto page 2. On master, python tools/verify_layout.py reports p1 ends on the un-indented line '• Laid a new cobbled path through the kitchen garden, beddin' while p2 opens with the indented line 'that rain ran off into the borders...': an entry header is orphaned from its bullets (exit 1). Expected: clean.
• cv_orphan_false_negative.tex/.pdf: page 1 ends on the entry header Kitchen Gardener 1975-1980, and page 2 opens with its first bullet. On master, the only report is p2 is the last page and 64% empty. Expected: the orphan as well.

The geometry behind both, from pdftotext -bbox: entry headers sit at x59.5, a bullet's first line (marker and text merged into one bbox line) at x58.9, level with the headers, and wrapped continuations at x72.1. With moderncv's default itemize inside \cventry, the marker is its own bbox line and the text is indented +11pt, which is why the indent rule works there. The patch leaves that output unchanged.

The four files:

• https://raw.githubusercontent.com/yang2632/ai-job-search-upstream/repro/481-orphan-shapes/docs/repro-481/cv_orphan_false_positive.tex
• https://raw.githubusercontent.com/yang2632/ai-job-search-upstream/repro/481-orphan-shapes/docs/repro-481/cv_orphan_false_positive.pdf
• https://raw.githubusercontent.com/yang2632/ai-job-search-upstream/repro/481-orphan-shapes/docs/repro-481/cv_orphan_false_negative.tex
• https://raw.githubusercontent.com/yang2632/ai-job-search-upstream/repro/481-orphan-shapes/docs/repro-481/cv_orphan_false_negative.pdf

The PR will follow once you have reproduced it. It keeps upstream's branch structure: opens_with_marker means a Unicode marker glyph with text after it, list_line means indented or a list item, and the orphan rule's two indented() calls become list_line(). Five synthetic fixtures pin both limits (the ASCII fixture fails if - joins the marker set). The docstring caveat is included.

[MadsLorentzen]

Reproduced both shapes exactly, on your PDFs and on my own lualatex builds of the two .tex files (byte-identical tool output): the false positive fires the "entry header orphaned" branch on • Laid a new cobbled path... (58.9) against that rain ran off... (72.1), and the false negative is silent because • Worked in the kitchen garden... sits at 58.9, level with the header. The bbox geometry is as you measured: headers 59.5, merged marker+text bullet 58.9, continuation 72.1, and Poppler's own -bbox-layout groups the relabelled \textbullet into one line too. Thank you for fixtures that hold up.

The fix shape we agreed on, though, does not survive the stock template, and that is on me for agreeing to it before measuring. The stock main_example.tex wraps each \cventry in an outer \item, and moderncv's \large\rmfamily\textbullet for that outer marker comes out of Poppler as ○ only 0.4 pt above the title, so on rounding luck it merges into the title line: four of the five stock entries extract as '○ [Job Title] [YYYY-YYYY]' at x 59.0. A "starts with a marker glyph and has text after it" test therefore classifies a stock entry header as a list line. I prototyped your shape and swept a \vspace before the "Previous Role" entry through 75 page-break positions: wherever page 1 ends on that merged header with its bullets on page 2, master reports the orphan and the prototype says clean. That is the true-orphan case the rule exists for, and the merged form is the common one on stock. Dropping ○ from the glyph set makes the sweep match master again, but that only works because of what one font setup's ToUnicode happens to emit, so I would not take it.

What does discriminate across all three geometries (yours, stock, and plain moderncv defaults) is the x of the text, not of the line: strip a leading lone marker word, then compare the remaining text's x on page 1's last line with page 2's first non-marker line. Equal x (72.1 = 72.1) is a wrapped bullet, clean; more indented (59.5 -> 72.1 on yours, 70.4 -> 81.3 on stock) is an orphaned header. That keeps the Unicode-only marker set and leaves the lone-marker branch alone, as before. One case it cannot separate without font information is a stock [Degree] line from its [Brief description] (both 70.4); master flags that today, so please keep it flagged, and say in the docstring that the rule reads text x and why. Same fixtures as synthetic tests, plus the two stock shapes above (merged ○ header at the break, and the outer marker on its own line, which master currently misreports as a split bullet). PR welcome on that shape.

[MadsLorentzen]

Landed as #508 (e355eaf): the orphan rule now reads where the text starts, after any leading Unicode list marker, and compares that across the break. Both fixtures from this thread are in the tests as synthetic geometry, plus the two stock shapes. Thank you for fixtures that held up and a PR that measured itself.