Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .claude/agent-memory/product-architect/MEMORY.md

Large diffs are not rendered by default.

56 changes: 56 additions & 0 deletions .claude/agent-memory/product-architect/client-pdf-pipeline.md
Original file line number Diff line number Diff line change
Expand Up @@ -584,3 +584,59 @@ while `realRender.test.ts` covers only Usage — Vendor at 45pt is the binding c
lists over the same union, and the tests that "count" them pin the literals 7/6, so a new key is
silently absent everywhere. Recommended fix shape: derive the canonical order from an exhaustive
`Record<ReportColumnKey, number>` and define the base sets as filters over it.

## #1940 / PR #2032 — runt-merge + continuation marker (reviewed 2026-08-06, APPROVED)

`packUsageCellRowsWithMinimum(segments, maxChars, minTrailingChars)` wraps `packUsageCellRows`
(both `packUsageCellRows` and `splitIntoPageSafeChunks` byte-identical to `beta`, hash-verified).
Production floor: `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS /* 20 */, usageSafeTokenChars)`.
Render-time marker: a bare `{ text: '… ' }` run prepended by `buildUsageCell(segments, true)` for
`packedCellRows` index >= 1 — never in `UsageCellSegment.text`, so I1 stays trivially true.

**The AC1-vs-AC2 resolution worth reusing.** Three candidate designs; only one works:
merge-without-repack is *unsound* (receiver can already be at `maxChars`); always-repack-at-reduced
budget is sound but regresses the zero-degradation range; **gate the reduced-budget repack behind an
actual runt check** is the only one that pays neither. A "lookahead inside the packer" is not a
single-pass alternative — you cannot know a remainder exists without packing to the end, so the
lookahead *is* the first pass, and folding it in would cost the primitive its clean
"every row <= the budget I was given" contract, which is what the safety proof rests on.

**The backward-merge induction, stated so it can be defended.** (1) primitive guarantees
`rowCharCount <= B`; (2) **receiver virginity is structural**: at counter `i` the only index written
is `i-1`, the counter strictly decreases, so index `k` is written iff the counter equals `k+1` —
exactly once ever; `splice(i,1)` shifts only indices `> i`; (3) the donor is `< min` *at the moment
of donation* because the guard re-reads the possibly-already-grown row. Hence
`(<= M-m) + (< m) < M` at any cascade depth. Verified by hand **and** by a 400k-case fuzz
(verbatim ports, `maxChars` 2..61, `min` 1..maxChars+4 incl. the degenerate band, meta segments,
leading empty segments): 0 AC1 / 0 AC2 / 0 I1 violations. **Fuzzing verbatim function ports in a
throwaway `.mjs` is the cheapest independent read of an induction argument — do this again whenever
a doc comment carries a proof.**

**Wrapping row-level output was the right layer, for a stronger reason than the PR gave.** The PR's
reason (a runt arises from two paths, one of which the chunker never sees) is true — the packer's
own `used > 0 && rest.length <= maxChars` flush creates runts with `splitIntoPageSafeChunks` never
invoked. But the load-bearing reason is a **unit mismatch**: the AC's unit is the rendered *row*;
the chunker's unit is a chunk within one *segment*. A row can hold a prose chunk AND the grey meta
segment, so a chunk-level floor bounds the wrong quantity.

**Findings left open (all non-blocking, for the ADR-034 pass):**
- ADR-034 **line 152**'s call-site quote is stale a **third** time (`MAX_SAFE_USAGE_CHUNK_CHARS` ->
`usageChunkChars` -> now `packUsageCellRowsWithMinimum(..., minTrailingUsageChars)`). Three
staleness events on one quoted signature: name the *contract*, drop the literal call.
- ADR-034 **line 148**'s rule ("bound what a cell *renders*") is now literally under-satisfied — the
marker is rendered and unbounded. Safe *by size only*: 2 chars, worst case **+1 line** (when the
next token is <= 16 chars so no `break-all`, but too long for the 14 slots left beside `'… '`);
0 lines in the break-all case. 41 -> 42 vs the 44-line `№` budget. Nobody wrote that down, and the
ux spec already budgets 14 chars for a *textual* marker variant — the obvious next request.
- `packUsageCellRows` now has **exactly one production caller** (the wrapper). Add a "production
callers go through the wrapper" line to its doc comment.
- ux-designer's "threshold-to-ceiling **ratio** stays roughly constant across subsets" is **false**
(the one-sided clamp pins `usageChunkChars` at 650 while `usageSafeTokenCharsForWidth` scales with
width: ratio runs ~3% -> ~9.5%). Harmless — the algebraic bound is subset-independent — but do not
let the ratio framing get copied into the ADR as the reason.

**#1950 sequencing: confirmed no reorder needed.** No geometry constant moves, and the repack budget
is strictly *below* the ceiling, so the new consumer is more conservative. But #1950's guard pins a
**rendered** quantity, and the marker adds 2 uncounted rendered characters: on a continuation row the
real overage against the derived `Ѹ` 616 ceiling is 36 chars / **4 lines / 44.8pt**, not 34 / 3 /
33.6. State which quantity the guard pins when #1950 lands.
28 changes: 28 additions & 0 deletions .claude/agent-memory/product-architect/recurring-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -1462,3 +1462,31 @@ return interface (proves nothing depended on the structural-only relation); `com
first declaration in the rule and the composed class must not share properties with the composer
(source order decides, both being single-class selectors); grep the *old* CSS-module class name across
`e2e/` — a POM `[class*="step4Body"]` locator survives a rename as a zero-match locator with Jest green.

## Fuzz the verbatim ports when a doc comment carries a proof (#1940, PR #2032)

When an AC's whole correctness rests on an induction argument written in a doc comment, a hand-trace
(mine, plus the dev-team-lead's) is two reads of the same reasoning, not two independent checks.
Copy the functions verbatim into a throwaway `.mjs` and fuzz the *stated postconditions* across a
parameter space that includes the degenerate guards — 400k cases took under a minute and covered
the cascade, the mid-list runt, the hard-split path, and the meta-segment boundary at once.

**Why:** a hand-trace confirms the argument the author wrote; it does not search for the case the
author did not consider. Only randomized inputs do that.
**How to apply:** any PR whose doc comment says "and this bound holds at any depth / for all N".
Write the ports, assert the postconditions, `rm` the file before committing. Note that the harness
must be created with `Write` (the Bash tool refuses heredoc redirects inside a worktree session).

## A safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940)

The #1940 ux spec argued the merge stays safe across all 96 subsets because "the threshold-to-ceiling
ratio stays roughly constant." False: `usageChunkCharsForWidth`'s **one-sided clamp** pins the
numerator's ceiling at 650 for every subset while `usageSafeTokenCharsForWidth` scales linearly with
width — the ratio runs ~3% to ~9.5%. The implementation was safe anyway (its bound is algebraic and
subset-independent), so this never became a defect — but the ratio sentence was on its way into a
code comment and the ADR.

**Why:** proportional-scaling arguments are the first thing a clamp, a floor, or a `Math.max` breaks,
and they are exactly the arguments that read as obviously true.
**How to apply:** whenever a spec or comment justifies safety by "both sides scale off the same
basis", grep the chain for `Math.min`/`Math.max`/`Math.floor` before letting the sentence land.
1 change: 1 addition & 0 deletions .claude/agent-memory/qa-integration-tester/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

## Recent bug/story notes (2026-08)

- [Issue #1940 — continuation marker + runt-merge](story-1940-continuation-marker-runt-merge.md) (2026-08-06) — `packUsageCellRowsWithMinimum`/`… ` marker; centralized `stripContinuationMarker` pattern (both test files); ripple far wider than the 2 named tests (whole "cell-scope invariant" block's shared helper in realRender.test.ts); fontkit glyph-id-0 technique (added as pinned devDependency); blank pdfmake cells DO get `.positions`; mid-list-runt fixture recipe; git-revert genuine-regression proof (clean import-error + 2 real assertion failures).
- [Issue #1912 — ESM mock blast radius](issue-1912-esm-mock-blast-radius.md) (2026-08-06) — new export on a shared module breaks a *different* file's partial `jest.unstable_mockModule` factory, not the one I checked; mechanical sweep + batch-jest-run pattern
- [Issue #1911 — splitKind field](story-1911-splitkind.md) (2026-08-05) — server UNION `origin`-column trap (AC1.5: "≠S per arm" not "≥2 distinct per arm"); UNION-dedup regression guard (AC1.9); `jest.spyOn(db,'all')` pass-through + drizzle `queryChunks` introspection for round-trip proof; client-side ripple broke 9 pre-existing tests in 2 files beyond the "add splitKind default" checklist item (found by just running the file); AC1.2 git-stash anti-vacuity proof (9/70 genuinely fail on reverted code).
- [Issue #2001 — remove TFunction from reportPdf](story-2001-remove-tfunc-reportpdf.md) (2026-08-05, 2 rounds) — 7 test files total; realRender.test.ts has renderOverviewPdfContent (KEEP its t) vs buildOverviewContent/generateReportPdf (REMOVE); perl regex targets `{ attachDocuments: }` context to avoid removing renderOverviewPdfContent args; comment on same line blocks regex → explicit Edit; 218 tests pass round 2.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
---
name: story-1940-continuation-marker-runt-merge
description: Issue #1940 (continuation-row "… " marker + runt-remainder merge in overviewPdf.ts) — centralized marker-stripping pattern, fontkit glyph-coverage technique, ripple scope
metadata:
type: project
---

Issue #1940: `packUsageCellRowsWithMinimum` (AC1/AC2 runt-remainder merge) and `buildUsageCell`'s
`isContinuation` marker (AC5, a literal `{ text: '… ' }` run prepended to every Usage continuation
row, no color override). Tests: `client/src/lib/reportPdf/overviewPdf.test.ts` (+~350 lines: new
`packUsageCellRowsWithMinimum` describe block, `AC5`/`AC8`/`AC2` describe blocks) and
`client/src/lib/reportPdf/realRender.test.ts` (+~190 lines: new `#1940` describe block).

## The marker-stripping ripple was much larger than the two "extend this block" instructions implied

The spec named 2 existing tests to fix by hand (scenario 10/11 in overviewPdf.test.ts). Actually
affected once the marker landed: **every existing test that reconstructs a Usage cell's text on a
row known to be a continuation row**, in BOTH files. Found by grepping every `splitUsageCell(`/
`rowTexts(`/`usageCellText(` call site and checking whether the row it read was ever index >= 1 of
a packed group:
- `overviewPdf.test.ts`: scenario 10, scenario 11, and the "[#1959 fix round] suffix gets a row of
its own" test (3 existing tests).
- `realRender.test.ts`: scenario 19c's reconstruction (`usageCellText` used directly, not
`splitUsageCell`), AND — the big one — the entire "cell-scope invariant" describe block's shared
`renderCellScopeRow` helper, which ~8 downstream tests depend on (700+400, 700+20-leaf-area,
2000-alone, the three page-count-saturation guards, two #1968 regressions). That helper also
derived its expected row COUNT from bare `packUsageCellRows`, which could disagree with
production's `packUsageCellRowsWithMinimum` count whenever a runt merge fires on one of those
large fixtures — fixed by switching the helper to call `packUsageCellRowsWithMinimum` with the
same `Math.max(20, USAGE_SAFE_TOKEN_CHARS_6COL)` floor production uses (that block's fixture is
always the 6-col/claim shape).

**Lesson**: when a spec says "extend the existing X and Y tests," always grep every call site of
the shared reconstruction helper across BOTH files before assuming the ripple is scoped to the two
named tests — a shared test helper used by N tests is one bug away from breaking all N silently.

## Centralization pattern (applied identically in both files)

Added `stripContinuationMarker<T extends {text,color}>(runs): T[]` once per file — strips exactly
one leading run whose `.text === '… '` AND `.color === undefined`. Gated behind an explicit
`opts.isContinuation` boolean on `rowTexts`/`splitUsageCell`, never applied unconditionally by
substring match (that would risk masking a genuine defect that happened to produce a leading '… '
in real content on row 0, which never gets the marker). Every call site on a continuation row
either passes `{ isContinuation: true }` directly, or (when mapping over a whole row group)
`{ isContinuation: i > 0 }` since row 0 of a packed group is never a continuation row.

## Deriving expected row counts: use `packUsageCellRowsWithMinimum`, not `packUsageCellRows`/`splitIntoPageSafeChunks`

Any test that derives "how many rows should this produce" from the bare packer instead of the
wrapper is a latent bug the moment a runt merge fires on that fixture — happened to not matter for
the specific pre-existing fixtures in this PR (verified by tracing: none of them contained an
actual runt), but the cell-scope block's fixtures easily could have. Always derive from the SAME
formula `buildOverviewContent` calls: `packUsageCellRowsWithMinimum(segments, usageChunkChars,
Math.max(20, usageSafeTokenCharsForWidth(colWidths.usage)))`. For the two reference shapes,
`usageChunkChars` is always `MAX_SAFE_USAGE_CHUNK_CHARS` (650) — `usageChunkCharsForWidth`'s
one-sided clamp means the 6-col shape's wider Usage column still clamps down to 650, never up.

## Pathological-runt fixture that reliably produces a MID-list runt (not just trailing)

`'A'.repeat(BUDGET) + ' hi ' + 'B'.repeat(18)` at a small BUDGET — the leftover `' hi '` (4 chars)
doesn't fit after `'A'*BUDGET` fills row 0, but DOES fit its own row, so plain `packUsageCellRows`
strands it alone as row 1, sandwiched before `'B'*18`'s own row 2. Useful whenever a spec explicitly
wants "not merely a trailing runt" coverage — most naive fixtures (a single oversized token) only
ever produce a TRAILING runt.

## fontkit glyph-coverage technique (AC9's "verify the glyph exists, don't assume from precedent")

`fontkit` is a transitive dependency (via `pdfmake` -> `pdfkit`) already resolvable at
`node_modules/fontkit` even before being declared — added it + `@types/fontkit` as EXACT-pinned
`client/package.json` devDependencies (both `2.0.4`, matching the already-hoisted version) per the
Dependency Policy, then `npm install` (root) to regenerate the lockfile — `npm ls fontkit`/`npm ls
undici` afterward confirmed no tree damage; audit findings unchanged (all pre-existing, unrelated to
fontkit). Technique, verified working under this repo's ts-jest ESM jsdom config with zero jest
config changes:

```ts
const fontkitModule = await import('fontkit');
const fontkit = (fontkitModule as { default?: typeof fontkitModule }).default ?? fontkitModule;
const vfsModule = await import('pdfmake/build/vfs_fonts'); // same module loader.ts uses
const vfs = (vfsModule as { default?: Record<string, string> }).default ?? vfsModule;
const font = fontkit.create(Buffer.from(vfs['Roboto-Regular.ttf'], 'base64'));
const glyph = font.glyphForCodePoint(0x2026); // U+2026 HORIZONTAL ELLIPSIS
expect(glyph.id).not.toBe(0); // glyph id 0 == .notdef by spec — the real "tofu box" signal
```

Confirmed empirically (Roboto-Regular): U+2026 -> glyph id 400 (real). A genuinely-unmapped astral
codepoint (`0x10ffff`) -> glyph id 0 — a positive control proving glyph id 0 is reachable/meaningful
in this exact font, not a fontkit quirk that always returns non-zero. **Glyph id, not a raw
advance-width measurement, is the dispositive check** — `.notdef` commonly has a real non-zero
advance in real fonts, so width alone can't rule out a tofu box.

## Blank pdfmake cells DO get `.positions` populated (verified empirically, not assumed)

Before writing an AC7 "every cell in the row shares one pageNumber" assertion across a continuation
row (which has blank leading/amount cells from `buildEmptyBodyCell`), I worried `cellPageNumber`
(which throws on an empty `.positions` array) would fail on those blank cells. Verified via a
disposable probe test: an empty-string `{text:'', style:'tableCell'}` cell gets exactly ONE
`.positions` entry after a real render, same shape as a non-empty cell's first entry. So
`row.map(cellPageNumber)` is safe and meaningful across a full row including blanks — no need to
filter to non-empty cells or weaken the assertion.

## Genuine-regression proof

Backup/restore technique (per [story-1929-round2-real-render-technique.md](story-1929-round2-real-render-technique.md)):
reverted `overviewPdf.ts` to `git show HEAD:...` (pre-#1940), ran the new test surface:
- `overviewPdf.test.ts`: whole suite fails to even LOAD (`SyntaxError: ... does not provide an
export named 'packUsageCellRowsWithMinimum'`) — a clean, unambiguous module-resolution proof for
the entire new Part A/B surface at once.
- `realRender.test.ts`'s new `#1940` describe block: 2 of 4 tests fail with REAL assertion
failures (not import errors, since the module still loads fine using only pre-existing exports)
— the AC9 runt-avoidance test fails because the last row's marker-stripped usage text is too
short, and the AC5 marker test fails because the first run is real prose (`'zzz...'`), not
`{text:'… '}`. The other 2 (glyph coverage, `_minWidth`/page-number) correctly still pass
pre-fix, since they test generic layout properties unrelated to this fix — expected, not a gap.

Restored via `cp` from a `/tmp` backup; confirmed `git diff --stat` on `overviewPdf.ts` matched the
pre-revert diff exactly (99 insertions / 5 deletions) before finishing.

## Review round 1 fixes (product-architect + security-engineer)

- **`fontkit` devDependency confirmed harmless**: security-engineer verified it was already a
production transitive dependency via `pdfmake` -> `pdfkit`, so declaring it in
`client/package.json` installs nothing new — the Docker runtime image is byte-identical.
- **`TS2352` — Jest passing does NOT mean `tsc` is clean.** `(fontkitModule as { default?:
typeof fontkitModule }).default ?? fontkitModule` compiled fine under `ts-jest` (which runs no
type diagnostics by default) but failed `npx tsc --noEmit -p client/tsconfig.json` — "neither
type sufficiently overlaps with the other." Fix: route through `unknown` first, exactly like the
`vfsModule` cast two lines below it already did: `(fontkitModule as unknown as { default?:
typeof fontkitModule }).default ?? fontkitModule`. **Lesson reinforced a third time in this
batch** (after #1911's missing factory field, #1912's ESM mock): run the scoped `npx tsc --noEmit
-p client/tsconfig.json` (or the server equivalent) before handback whenever a test file adds a
nontrivial type assertion — Jest green is not proof the build is green. Make this reflexive, not
something that has to be asked for.
- **Bare literal `20` retyped at 6 call sites — exactly the anti-pattern issue #1950 exists to
prevent.** Once `frontend-developer` extracted `MIN_CONTINUATION_ROW_FLOOR_CHARS` (exported from
`overviewPdf.ts`, value `20`), all 6 sites (4 in `overviewPdf.test.ts`, 2 in `realRender.test.ts`)
were switched from `Math.max(20, ...)` to `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS, ...)`.
**The two operands are a deliberate non-redundant pair, not simplifiable to either alone**:
`usageSafeTokenCharsForWidth(usageWidth)` does the main per-subset-width work; the named constant
is the absolute fallback floor for subsets where that per-line figure is itself small. Added a
comment at the point of use so a future reader doesn't "simplify" one operand away.
- **General lesson**: whenever a spec/AC references a bare numeric literal that also appears as a
production magic number, check whether a name for it exists (or is about to land) before writing
the test — retyping it in N places is itself the bug class this batch's #1950 targets.
Loading