feat(reports): merge runt continuation chunks and mark continuation rows (#1940)

[steilerDev]

Summary

A continuation row blanks every leading column, so sitting under a freshly repeated header it is visually indistinguishable from the orphaned-cell bug #1929 fixed. The degenerate form is worse: with no minimum trailing-chunk size, text whose tail lands just past a chunk boundary produced a row blank except for a single stray character, which reads as document corruption on a document whose entire purpose is to look credible. Both forms were reproduced by rasterized render during the #1935 review rounds, not hypothesised.

• Runt merge — new packUsageCellRowsWithMinimum merges a would-be runt into the row before it. It wraps packUsageCellRows's row-level output rather than modifying the chunker, because a runt arises from two paths — the hard-split remainder and the "doesn't fit this row's leftover but fits a fresh row" branch — and a fix inside splitIntoPageSafeChunks would have missed the second. Both of those functions are byte-identical to beta, verified independently by the reviewer against the beta blob.
• Continuation marker — a literal … run prepended to continuation rows.
• Per-subset floor — max(MIN_CONTINUATION_ROW_FLOOR_CHARS, usageSafeTokenChars), so a merged runt fills at least one real line at whatever width the active subset gives Usage. The two operands are a deliberate non-redundant pair: the per-line figure does the main work, the constant is the absolute fallback for subsets where that figure is itself small.

How AC1's tension with AC2 was resolved

AC1 wants a runt merged backwards; AC2 forbids that merge pushing the preceding row past the one-page budget the ceiling exists to guarantee. The naive fix — reduce every row's budget by the minimum — is provably safe but regresses AC8: it splits content that fits in one row today whenever that content falls in the (maxChars - min, maxChars] band, even though no runt was involved.

The implementation instead gates the reduced-budget repack behind an actual runt check. Pack once at full budget; if no row after the first is short, return it untouched. Only when a genuine runt exists does it repack at the reduced budget and merge backwards. So the zero-degradation range is byte-for-byte unchanged, and no ceiling is re-derived — usageChunkCharsForWidth's deliberate one-sided clamp is untouched.

The AC2 safety argument is carried in the doc comment rather than just its conclusion: every row packUsageCellRows returns is <= the budget it was called with, and backward iteration means a merge receiver has never yet been grown by the pass, so every merge yields (<= maxChars - min) + (< min) < maxChars at any cascade depth. The reviewer traced that induction by hand against the code and confirmed the comment matches the implementation.

Two corrections to the issue text

• AC2 references MAX_SAFE_SMALL_CHUNK_CHARS (450). That constant no longer exists — it was Report PDF layout breaks: usage column overflows the page, rows split across page breaks, running header clipped #1929 round-4's 9pt suffix ceiling, removed when fix(reports): improve report PDF UX — paragraph breaks, inline meta, inline notes, column toggles #1959 moved the suffix inline. There is one ceiling today.
• AC1's flat "~20 characters" became per-subset on the ux-designer's recommendation, since Usage width now varies across Report column visibility: wire the preview toggles through to the generated PDF #1973's 96 subsets rather than two fixed shapes — a flat number looks runty in wide subsets and redundant in narrow ones.

Why the marker is a glyph and not text or colour

The ux-designer verified structurally that continuation rows are only reachable when Usage is visible, and that every other cell is unconditionally blanked — so Usage is the only cell that can carry a signal, and a repeated-identifier approach fails outright in subsets like {allocatedAmount, usage} where no leading column exists. Colour and fill were rejected on two grounds: making TABLE_LAYOUT row-aware is disproportionate here, and colour is precisely what degrades under the greyscale printing and photocopying a bank document undergoes. Ink shape survives that; a fill does not.

No i18n key is introduced, so AC6 is satisfied vacuously.

Test evidence

• The marker is render-time only and never enters UsageCellSegment.text, so AC3's exact-reconstruction invariant (Report PDF layout breaks: usage column overflows the page, rows split across page breaks, running header clipped #1929 I1, highest precedence) is preserved.
• A property-style sweep across total lengths from maxChars - min to maxChars * 4 asserts jointly that no row after the first is under the floor and no row exceeds the ceiling — the test most likely to catch a flaw in the induction that a hand-picked fixture would miss.
• Genuine-regression verified by reverting the production file: the unit suite fails to load on the missing export, and 2 of 4 new real-render tests fail on real assertions (runt length, marker presence). The other two are legitimately independent of the fix — reported as such rather than overclaimed.
• Glyph coverage measured, not assumed — the ux-designer asked for this explicitly. fontkit.glyphForCodePoint(0x2026).id !== 0 (glyph id 0 is .notdef by spec), with a positive control proving the check can fail in this exact font. A raw width measurement would not have been meaningful, since fonts routinely give .notdef a real advance.
• 241/241 passing; 100% statements/functions/lines on overviewPdf.ts.

On the new devDependency

fontkit and @types/fontkit, exact-pinned. The security-engineer confirmed it was already a production transitive dependency via pdfmake → pdfkit, so declaring it installs nothing new, adds no transitive packages, changes no integrity hash, and leaves the Docker runtime image byte-identical. @types/fontkit verified genuine DefinitelyTyped (publisher content hash present), not a typosquat. npm audit unchanged.

Fixes #1940

🤖 Generated with Claude Code

[steilerDev]

[security-engineer] Dependency-change review — verdict: clean, approve.

Scope was the client/package.json / package-lock.json change only; the rest of this PR is PDF layout logic with no security surface.

Supply-chain posture. fontkit@2.0.4 was already resolved in the root lockfile before this change, as a production dependency of pdfkit, which pdfmake pulls in. Declaring it directly installs nothing new — same integrity hash, no lockfile entry change for node_modules/fontkit itself. 2.0.4 is also the current latest on the registry, so no patch is being skipped. No install/postinstall scripts (only fontkit's own prepublish/build, which never run on consumer install), no native binaries, no binding.gyp — pure JS. Every transitive dependency (@swc/helpers, brotli, clone, dfa, fast-deep-equal, restructure, tiny-inflate, unicode-properties, unicode-trie) was already inherited from the pre-existing pdfkit → fontkit edge.

npm audit — verified independently. Nine vulnerable packages in the tree, none matching fontkit or @types/fontkit. --omit=dev shows one pre-existing finding (react-router, GHSA-qwww-vcr4-c8h2) that this diff does not touch or move.

Lockfile correctness. Both packages hoisted to root node_modules/ rather than nesting under client/node_modules/, confirming a full npm install rather than --package-lock-only — that nesting failure mode has previously broken TypeScript type resolution for other workspace consumers here. The full lockfile diff is exactly three changes: the two additions plus one cosmetic undici re-hoist (same version, dev-only, still present under testcontainers). No production dependency moved, lockfileVersion unchanged, root package.json untouched.

@types/fontkit@2.0.4 authenticity. Genuine DefinitelyTyped: registry metadata shows the DefinitelyTyped/DefinitelyTyped repository at types/fontkit, the types bot maintainer, and a typesPublisherContentHash — the publish-pipeline marker a squatted package cannot fake. Type packages are a known typosquatting vector and this one was added by an agent rather than chosen by a human, so it was worth checking rather than assuming.

Classification. Correct as a devDependency. The only reference anywhere in client/src is the dynamic await import('fontkit') inside the test file, so it cannot reach the webpack production bundle, and @types/fontkit is excluded by the Dockerfile's npm ci --omit=dev --ignore-scripts. Net effect on the runtime image: zero.

On proportionality, which the orchestrator asked me to judge rather than assume: no concern. Since the package was already a transitive production dependency with zero new install footprint, this is not "pulling in a package for one test" — it is pinning one that was already there. The weaker fallback (measuring the marker's rendered width) is not needed on security grounds.

[steilerDev]

[ux-designer]

Verdict: APPROVED — implements the spec I posted on #1940 faithfully, and I re-verified the outcome it exists for by rendering.

Point-by-point against the spec

1. Marker on every continuation row, never row 0. buildUsageCell(segments, isContinuation = false) only prepends the marker when isContinuation is true; the call site passes true for packedCellRows[1..] and leaves the default false for packedCellRows[0]. Confirmed both by the real-render test (row0Cell.text[0].text asserted !== '… ', every continuation row's first run asserted .toEqual({ text: '… ' })) and by my own render (below).

2. No colour/bold/font-size override. The run pushed is the literal object { text: '… ' } — no color, bold, or fontSize key. It's the first element of the runs array, and the whole cell returns { text: runs, style: 'tableCell' }, so it inherits tableCell (8pt, non-bold, default ink) exactly like the rest of the row — same font as the body content, verified against merge.ts's tableCell style and loader.ts's Roboto-Regular.ttf mapping. I read the actual run object in the diff, not just a "marker is present" assertion — this is the detail a well-meaning "let's grey it like the deposit note" edit would have quietly broken, and it wasn't.

3. Floor expression. minTrailingUsageChars = usageVisible ? Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, usageSafeTokenChars) : 0, with MIN_CONTINUATION_ROW_FLOOR_CHARS = 20 exported and documented as the absolute fallback, usageSafeTokenChars (per-subset per-line budget) as the operand doing the main work. The doc comment on the constant states this relationship explicitly and matches what I asked for — the two operands stay non-redundant (one is subset-derived and usually dominates; the constant only binds when a subset's line budget is itself small).

4. Does a bare leading ellipsis actually read as "this text carries on"? Re-examined this on purpose rather than rubber-stamping my own prior call. I rendered the real pipeline at both the narrowest subset (allocatedAmount + usage only — the extreme case flagged as worth checking) and the widest (7-column) subset, forcing a runt-triggering continuation group in each, and rasterized both. In both, the continuation row presents as: every leading cell blank (the existing "this repeats the row above" convention already used for Report PDF layout breaks: usage column overflows the page, rows split across page breaks, running header clipped #1929) plus … opening the Usage cell. The combination — blank identity columns and a leading ellipsis on the one cell that isn't blank — reads unambiguously as continuation, not corruption, in isolation. I don't think a text label ("cont'd") would add anything here and would cost real width in the narrow subsets; holding the line on "no i18n string" was correct. I'm not walking back the round-3 call.

5. The degenerate near-empty-row case. Traced the merge loop (packUsageCellRowsWithMinimum's backward for scan) by hand against the exact repro I filed the issue from ('z'.repeat(2*maxChars+1) → [max, max, 1] under plain packing) — the loop provably closes it, not just narrows it: every row index ≥ 1 after the pass is bound below by minTrailingChars (proven by the doc comment's induction, which I re-derived independently and it holds), and the "any cascade depth" case (a runt merging into a row that itself becomes a runt) is handled since the scan is strictly backward and re-checks the receiver's own threshold before moving on. Confirmed empirically too: my rendered fixture at both column subsets shows no near-empty trailing row — the last row in each case is full, substantial content, not a stray character. The mid-list-runt test the PR added (not something I explicitly asked for, but the right generalization of the same bug) is a good catch.

The two things flagged for extra scrutiny

• Glyph coverage measurement. fontkit.glyphForCodePoint(0x2026).id !== 0 against Roboto-Regular.ttf pulled from the real vfs_fonts blob, with a positive control (an astral codepoint that does resolve to .notdef) proving the check isn't vacuous. This is exactly the right verification and targets the actual font tableCell/non-continuation-row content renders with (confirmed via loader.ts's normal: 'Roboto-Regular.ttf' mapping) — not a stand-in font.
• Narrowest/widest subset legibility. Checked by real render (see above): the marker is legible and unambiguous at both extremes, including the 2-column shape where Usage is the only cell with any content at all on a continuation row.

Not in scope, not reviewed

Chunking algorithm correctness beyond the runt-merge property tests, the fontkit devDependency itself, CI status — per the brief, these are covered elsewhere.

No blocking findings. This closes the loop from #1929 round 4 cleanly.

[steilerDev]

[product-architect]

VERDICT: APPROVE — no blocking findings. The AC1/AC2 resolution is the right one, and the safety argument holds under an independent read. Four non-blocking follow-ups below, one of which folds into the ADR-034 pass already owed from #2007/#2028.

---

1. The AC1/AC2 resolution — right design, and the induction is sound

Is this a two-pass design where a single-pass would do? No. I worked the alternatives and each one is strictly worse:

• Merge without repacking (single pass over the full-budget output) is unsound: the receiver rows[i-1] can already be at exactly maxChars, so any merge breaches AC2. A repack is not optional.
• Always pack at maxChars - minTrailingChars (single pass, no gate) is sound but regresses AC8 exactly as the PR body says — content in the (630, 650] band splits for no reason.
• Reserve the floor inside the packer (a lookahead in packUsageCellRows) is the same work: you cannot know whether a remainder exists without packing to the end, so the "lookahead" is a first pass. Folding it in would only hide the second pass, and would cost packUsageCellRows its clean primitive contract ("every row ≤ the budget I was given"), which is precisely what the wrapper's safety proof rests on.

So the gate isn't a hedge — it is the only structure that satisfies AC1 and AC2 without paying AC8. Cost is one extra packUsageCellRows call per invoice row, only on the runt path, on inputs already above 650 characters. Correct trade.

AC2 induction — independently verified, twice. I traced it by hand and then falsification-tested it. The hand proof, stated in the terms I'd want it defended in:

1. packUsageCellRows(segments, B) returns rows with rowCharCount ≤ B — used only ever advances by rest.length ≤ remaining or by a splitIntoPageSafeChunks head bounded by remaining. So every raw row from the repack is ≤ maxChars - minTrailingChars.
2. Receiver virginity is structural, not incidental. At loop counter i, the only index written is i - 1. The counter is strictly decreasing, so index k is written iff the counter equals k + 1 — exactly once, ever. splice(i, 1) shifts only indices > i, never the receiver. So a receiver's size at the moment it receives is always its raw packed size, ≤ maxChars - minTrailingChars.
3. The donor is always < minTrailingChars at the moment of donation — that is the loop's own guard, re-evaluated on the current (possibly already-grown) content. This is the part that makes the cascade work: a row that absorbed a runt and is still short donates its whole combined self, and the guard measures that combined self.

(≤ M - m) + (< m) < M therefore holds at any cascade depth, and the doc comment states it correctly. Symmetrically, AC1's postcondition holds because every index ≥ 1 is examined exactly once, after all merges into it have completed and before any further growth is possible.

Falsification run (throwaway harness, verbatim ports of all three functions, discarded after): 400,000 randomized cases — maxChars ∈ [2, 61], minTrailingChars ∈ [1, maxChars + 4] (deliberately including the degenerate band), lengths 0 … 5 × maxChars, prose with random whitespace runs and over-budget unbroken tokens, half with a trailing meta segment, 10% with a leading empty segment.

{"checked":400000,"violAC1":0,"violAC2":0,"violI1":0,"worstOver":0}

Zero AC1 violations, zero rows over maxChars (max overage observed: 0), zero I1 violations. That covers the cascade, the mid-list runt, the hard-split path, and the meta-segment boundary. I'm satisfied AC2 is safe.

2. Wrapping packUsageCellRows's output is the right layer — for a stronger reason than the PR gives

The PR argues the chunker would miss one of two runt paths. True, and I confirmed it: the second path (used > 0 && rest.length <= maxChars → flush to a fresh row, overviewPdf.ts L371-376) creates a runt entirely inside the packer, with splitIntoPageSafeChunks never invoked. My fuzz found these routinely.

But the load-bearing reason is a unit mismatch: AC1's unit is the rendered row; the chunker's unit is a chunk within one segment. Those are not the same object — a row can hold a prose chunk and the grey meta segment. A floor expressed in chunks would be a floor on the wrong quantity, and would go silently wrong the next time a segment type is added to the stream. Same reasoning as ADR-034's "bound the cell's content stream, not a source field." The row level is the only level where the AC is even statable.

On "two overlapping packers": packUsageCellRows now has exactly one production caller — the wrapper (verified: every other occurrence in client/src outside tests is a doc-comment mention). It stays exported for the tests, which is right, but nothing marks it as off-limits to buildOverviewContent. [low] Add one line to its doc comment — "Production callers must go through packUsageCellRowsWithMinimum; this primitive is the unfloored building block it composes." That is what stops the next reader treating them as interchangeable. Not blocking.

3. usageChunkCharsForWidth's one-sided clamp — untouched, and not defeated

Confirmed. usageChunkChars is still the maxChars handed to the wrapper, and the repack path uses maxChars - minTrailingChars, which is strictly below the clamped ceiling. The clamp's asymmetric guarantee ("may scale down, must never scale up") is preserved a fortiori — this change can only ever approach the ceiling less aggressively than beta does. Nothing is dead: the wrapper reads maxChars on every path, including the fast path and both degenerate guards.

Also confirmed byte-identical to beta by hash, independently of the diff:

splitIntoPageSafeChunks  3272ac71954be335dd257ff8f761d4a5  (both)
packUsageCellRows        7f48c4c71ce74c07920678788cd5bca3  (both)

[informational] One framing correction worth catching before it propagates. The ux-designer's spec argues the merge stays safe across subsets because "the threshold-to-ceiling ratio stays roughly constant." That is not true, and the clamp is why: usageChunkChars is pinned at 650 for all 96 of today's subsets while usageSafeTokenCharsForWidth scales linearly with width, so the ratio runs from ~3% at the 7-column shape to ~9.5% at the widest. It doesn't matter — the algebraic bound in §1 is subset-independent and makes the ratio argument unnecessary — but the ratio framing should not be copied into a code comment or the ADR as if it were the reason.

4. #1950 sequencing — the spec's ruling is correct, with one note to carry forward

Confirmed: #1940 does not need to sequence after #1950, and #1950 needs no re-derivation because of #1940. MAX_SAFE_USAGE_CHUNK_CHARS (650), USAGE_WIDTH_7COL, both font sizes and the line height are all byte-identical, so #1950 AC 2.3 is untouched and its constant-vs-constant assertions (34 chars / 3 lines / 33.6pt) still pass and are still correct. The repack budget is lower than the ceiling, so the new consumer is strictly more conservative. No reordering needed.

But carry this into #1950 when you get to it (see also finding 6): the derived-Ѹ overage the guard pins is a rendered quantity, and #1940 introduces two rendered characters the budget does not count. Worst case the marker costs one extra line on a continuation row — when the following token is short enough to skip wordBreak: 'break-all' (≤ USAGE_SAFE_TOKEN_CHARS_7COL = 16) but too long for the 14 slots left beside '… ', so line 1 carries only the marker. In the pathological break-all case it costs zero (break-all packs from slot 2: ceil(652/16) = ceil(650/16) = 41).

Against the measured № budget that is fine — 41 → 42 lines against 44, still inside. Against the derived Ѹ ceiling it moves the accepted overage on continuation rows from 3 lines / 33.6pt to 4 lines / 44.8pt. Still the same non-credible-input class you already accepted, so no action here — but #1950's guard should say which quantity it is pinning, or the next person to read it will believe the rendered overage is 34 characters when on a continuation row it is 36.

5. MIN_CONTINUATION_ROW_FLOOR_CHARS — extraction complete, doc comment accurate

Complete. All 6 previously-retyped sites now import the constant (4 in overviewPdf.test.ts, 2 in realRender.test.ts). The four bare 20s that remain in overviewPdf.test.ts (L460, L471, L522, L703) are local chunk budgets (maxChars / BUDGET) for the packer describe blocks — a different quantity that happens to share a value. Correctly left alone; replacing them would have been the more damaging error, coupling a test's legibility budget to a production floor.

The two-job split with usageSafeTokenChars is described accurately and matches the code: usageSafeTokenCharsForWidth does the per-subset work, the constant is the absolute fallback. The comment also names the failure mode (a future reader "simplifying" one operand away), which is the part that makes it survive. Good.

6. ADR-034 — folds into the open pass, no new ADR

No new ADR needed; nothing here changes a decision. But this PR does stale two things on ADR-034, and both belong in the pass already owed from the #2007 and #2028 reviews:

[medium] Line 152's call-site quote is stale for the third time. It reads:

The bound belongs where the cell's content stream is assembled (packUsageCellRows(segments, usageChunkChars) in overviewPdf.ts)

Production now calls packUsageCellRowsWithMinimum(segments, usageChunkChars, minTrailingUsageChars). That same sentence was already corrected once by the #1973 pass (it had quoted MAX_SAFE_USAGE_CHUNK_CHARS as the argument). Three staleness events on one quoted call site is a signal about the quote, not about the PRs — when the pass happens, consider naming the contract ("the bound is applied where the cell's segment stream is packed into rows") and dropping the literal signature, which is what keeps going out of date.

[medium] The section's own rule is now literally under-satisfied. Line 148 states it as an imperative:

Bound the height of what a cell renders, never the length of a source field.

The '… ' marker is rendered and is not under the bound. At 2 characters and a worst case of one extra line it is comfortably safe today (quantified in §4), and keeping it out of the packed stream is correct — it is what makes I1 trivially true, and the code comment says so. But "safe because it is two characters" is a fact nobody has written down, and this file's culture is that unrecorded margins get consumed. The exposure is a future contributor widening the marker (a reportT('…continued') string is the obvious next request — the ux spec even budgets 14 characters for it in §4 of the visual spec) and reasoning that the marker is decoration, therefore free. It is not free; it is currently free by size.

[low] Suggested fix, one line each, at the next pass: add the worked line-count figure to buildUsageCell's isContinuation doc comment ("2 characters, ≤ 1 extra line at 16 chars/line, inside the 44-line measured budget — a longer marker must be re-derived against MAX_SAFE_USAGE_CHUNK_CHARS"), and one sentence in ADR-034's section recording that the marker is a deliberate, quantified exception to the rendered-content bound.

7. Smaller notes (all informational, none actionable in this PR)

• rowCharCount counts the meta segment's leading \n, which buildUsageCell strips when the meta segment starts a cell (L917). The floor check therefore over-counts by 1 on a meta-only continuation row — safe direction for AC2, and it means such a row can render one character below the nominal floor. Below noise against a 20-character floor.
• The runt path can cost one extra row. In my fuzz, ~9.6% of cases returned more rows than plain packing — the price of the reduced repack budget, and structurally impossible inside AC8's range (single-row content has no rows.slice(1), so hasRunt is false and the raw result is returned by identity). Worth knowing, not worth changing.
• Test quality is high. The property sweep asserts the floor and the ceiling jointly, which is the pairing that catches an induction flaw — an implementation that satisfied AC1 by ignoring AC2 passes neither. The mid-list-runt fixture covers the packer-created runt path that a trailing-only fixture would miss. expect(cell.text[0]).toEqual({ text: '… ' }) is a shape assertion, so an accidental color fails it — the right strictness for a run whose whole point is that it carries no colour. Gating stripContinuationMarker on an explicit isContinuation flag rather than a prefix match is the correct call: a prefix match would silently absorb a genuine defect that produced a leading '… ' in real content.
• stripContinuationMarker is now forked across two test files. Acceptable — it matches this file pair's existing local-helper convention, and both copies are 4 lines. Flagging only so it is a known duplication rather than a discovered one.

---

Verified: architecture compliance (no contract, schema, or API surface touched); AC1/AC2/AC3/AC4/AC7/AC8 by proof and by 400k-case falsification; AC5/AC6/AC9 against the ux-designer's visual spec, which the implementation follows exactly including the no-colour instruction; packUsageCellRows/splitIntoPageSafeChunks byte-identity against beta by hash; the usageChunkCharsForWidth clamp intact; the MIN_CONTINUATION_ROW_FLOOR_CHARS extraction complete; the wiki submodule ref matching origin/master (69cb2e6, and this PR touches no wiki file). Static Analysis, Trailer Check, Docker, and E2E Smoke green at d3811f2; unit shards still running at time of review — merge on the gate, not on this comment.

[github-actions]

🎉 This PR is included in version 2.14.0-beta.20 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀