From 6f091cdaf1df6cf63adcdb6ac9ff0b7dec0614af Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 07:28:29 +0200 Subject: [PATCH 1/3] refactor(reports): split LETTER_SUBJECT_FONT_SIZE from SUBHEADER_FONT_SIZE The cover letter's subject line reused SUBHEADER_FONT_SIZE, but that constant is load-bearing arithmetic for the running header: headerFootprint() consumes it and the result drives PAGE_TOP_MARGIN, which #1929 spent four review rounds getting right because getting it wrong clips the header off every page. Two unrelated 12pt values shared one name. Shrinking SUBHEADER_FONT_SIZE to reclaim header space -- a plausible fix for #1937 or #1938, both open against that same header -- would have silently shrunk the subject line on an already-approved document; bumping the subject line for legibility would have quietly reflowed every page of every report. The ux-designer's spec did direct the reuse, but its rationale argues against a magic literal rather than claiming the two sizes are the same thing, and its design reasoning for the subject line never mentions the running header. The equality is coincidental, so the new constant carries its own literal rather than aliasing -- an alias would satisfy the naming complaint while preserving the exact coupling that is the problem. Also removes the now-inverted comment directing the reuse, and records the PDF_STYLES split trigger in the module header: letterSubject is the first style entry with no geometry consumer, a split is not warranted until the second, and the target direction is pageGeometry <- pdfStyles <- merge. No behaviour change: headerFootprint() and PAGE_TOP_MARGIN are byte-identical and the subject line still renders 12pt bold. The two font sizes are now pinned independently in tests, verified by mutation in both directions. Fixes #1953 Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester --- client/src/lib/reportPdf/pageGeometry.test.ts | 29 +++++++++++++++++++ client/src/lib/reportPdf/pageGeometry.ts | 22 +++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/client/src/lib/reportPdf/pageGeometry.test.ts b/client/src/lib/reportPdf/pageGeometry.test.ts index 09459aecb..21974cf19 100644 --- a/client/src/lib/reportPdf/pageGeometry.test.ts +++ b/client/src/lib/reportPdf/pageGeometry.test.ts @@ -72,6 +72,35 @@ describe('pageGeometry — page constants', () => { }); }); +describe('pageGeometry — PDF_STYLES.letterSubject / PDF_STYLES.subheader font sizes are independently pinned (#1953)', () => { + // #1953: LETTER_SUBJECT_FONT_SIZE and SUBHEADER_FONT_SIZE are two module-private constants that + // currently both equal 12, but production deliberately split them into two separate literals + // (see pageGeometry.ts's LETTER_SUBJECT_FONT_SIZE comment) because they mean different things: + // SUBHEADER_FONT_SIZE is load-bearing footprint arithmetic feeding PAGE_TOP_MARGIN (via + // headerFootprint()); LETTER_SUBJECT_FONT_SIZE is plain cover-letter typography with no geometry + // consumer at all. The two assertions below MUST stay separate, each pinned to its own literal — + // do NOT "deduplicate" them into `expect(PDF_STYLES.letterSubject.fontSize).toBe(PDF_STYLES.subheader.fontSize)` + // (or into `SUBHEADER_FONT_SIZE`) just because the numbers currently match. Doing so would + // silently re-couple two values production deliberately decoupled, and this test would stop + // catching the exact regression #1953 exists to prevent: an edit to one font size that + // unintentionally changes the other (or, for SUBHEADER_FONT_SIZE, reflows every page's top + // margin — see PAGE_TOP_MARGIN below). + it('PDF_STYLES.letterSubject.fontSize is 12pt, pinned to its own literal', () => { + expect(PDF_STYLES['letterSubject']).toBeDefined(); + expect(PDF_STYLES['letterSubject']!.fontSize).toBe(12); + }); + + it('PDF_STYLES.subheader.fontSize is 12pt, pinned to its own literal (independently of letterSubject above)', () => { + expect(PDF_STYLES['subheader']).toBeDefined(); + expect(PDF_STYLES['subheader']!.fontSize).toBe(12); + }); + + it('PAGE_TOP_MARGIN does not depend on letterSubject.fontSize: headerFootprint() sums only HEADER_FONT_SIZE, SUBHEADER_FONT_SIZE, SUBHEADER_MARGIN_TOP, and HEADER_BLOCK_BOTTOM_MARGIN — letterSubject is not one of its inputs, so a future change to the cover-letter subject size cannot reflow any page of the report (#1953 Verification)', () => { + expect(PAGE_TOP_MARGIN).toBe(93); + expect(PAGE_TOP_MARGIN).toBe(Math.ceil(headerFootprint() + 15)); + }); +}); + describe('pageGeometry — printableWidth (scenario 1)', () => { it('printableWidth() === 515.28 (595.28 page width minus 40pt left/right margins)', () => { expect(printableWidth()).toBe(515.28); diff --git a/client/src/lib/reportPdf/pageGeometry.ts b/client/src/lib/reportPdf/pageGeometry.ts index 5ef40606e..70fc0b999 100644 --- a/client/src/lib/reportPdf/pageGeometry.ts +++ b/client/src/lib/reportPdf/pageGeometry.ts @@ -12,6 +12,14 @@ * merge.ts imports `PDF_STYLES` from here and re-exports it for its own consumers. This module * must never import from merge.ts — merge.ts already imports geometry from this file, and * reversing that edge would create a circular import. + * + * PDF_STYLES SPLIT TRIGGER (#1953): `letterSubject` (below) is the first `PDF_STYLES` entry with + * NO geometry consumer at all — nothing in this module's math reads its font size, unlike every + * other style here. That's not a problem by itself and does NOT warrant splitting `PDF_STYLES` + * out of this module yet. The trigger for when it does: the SECOND style entry with no geometry + * consumer. When that happens, move `PDF_STYLES` into its own `pdfStyles.ts` that imports geometry + * constants from this module and is re-exported by merge.ts — i.e. `pageGeometry <- pdfStyles <- + * merge`, preserving the edge direction fixed above. Until then, leave it here. */ import type { Style } from 'pdfmake/build/pdfmake'; @@ -65,6 +73,18 @@ const SUBHEADER_MARGIN_TOP = 4; // PDF_STYLES.subheader margin, below const HEADER_BLOCK_BOTTOM_MARGIN = 20; // buildPageHeader's own margin: [0,0,0,20] const HEADER_TOP_GAP = 15; // visible separation kept above the computed footprint +/** + * Cover letter subject-line font size, pt — PDF_STYLES.letterSubject below, only. This equals + * SUBHEADER_FONT_SIZE (12) above by COINCIDENCE, not by design: it is deliberately its own + * literal, not derived from or aliased to SUBHEADER_FONT_SIZE, and changing one must NOT change + * the other (#1953). SUBHEADER_FONT_SIZE is load-bearing footprint arithmetic that feeds + * headerFootprint() and, through it, PAGE_TOP_MARGIN below; LETTER_SUBJECT_FONT_SIZE is plain + * letter typography with no geometry consumer. Reusing the header's constant here would silently + * couple a future header-spacing fix (or subject-line legibility tweak) to the other's page + * layout — see #1953 for the incident this constant exists to prevent. + */ +const LETTER_SUBJECT_FONT_SIZE = 12; // PDF_STYLES.letterSubject, below + /** * Shared pdfmake document-definition style dictionary. Relocated here from merge.ts (#1939, AC8/ * AC9) so its font-size literals (`tableHeader`, `tableCell`, `small`, `header`, `subheader`) are @@ -109,7 +129,7 @@ export const PDF_STYLES: Record = { color: '#6b7280', }, letterSubject: { - fontSize: SUBHEADER_FONT_SIZE, // 12pt — reuse the existing constant, don't add a new literal + fontSize: LETTER_SUBJECT_FONT_SIZE, bold: true, color: '#111827', // matches PDF_STYLES.header's color — same "this is important" dark tone }, From 937ec34e025a142d45f6380547145dbcf8a74967 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 07:34:07 +0200 Subject: [PATCH 2/3] refactor(reports): file the constant under its own heading, drop a duplicated test - LETTER_SUBJECT_FONT_SIZE sat as the last member of the header-footprint section, so the constant whose whole reason for existing is that it is NOT header-footprint arithmetic was filed under the header-footprint banner. It now has its own marker naming it as letter typography with no geometry consumer. - Remove a third test that was assertion-for-assertion identical to the existing PAGE_TOP_MARGIN formula test, order swapped. It added no discrimination -- the SUBHEADER_FONT_SIZE mutation failed both copies for one reason, not two -- and its title claimed PAGE_TOP_MARGIN does not depend on letterSubject.fontSize while neither assertion referenced letterSubject. A name licensing a stronger claim than its body establishes is worse than no test, because a reader consults the name to decide whether a guarantee is covered and stops looking. The removal leaves a note recording that the guarantee is not expressible as a standing assertion here, since neither constant is exported, and what would make it expressible. Refs #1953 Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude product-architect --- .../agent-memory/product-architect/MEMORY.md | 2 +- .../product-architect/recurring-patterns.md | 39 +++++++++++++++++++ client/src/lib/reportPdf/pageGeometry.test.ts | 18 +++++++-- client/src/lib/reportPdf/pageGeometry.ts | 1 + 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index e869f20c7..237f8c020 100644 --- a/.claude/agent-memory/product-architect/MEMORY.md +++ b/.claude/agent-memory/product-architect/MEMORY.md @@ -2,7 +2,7 @@ ## Topic Files -- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\`` doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python `len()` not `awk length()` (em-dashes are 3 bytes) + two open findings: the `BACKUP_DIR` default/gate is wrong on Architecture.md and API-Contract.md's `splitKind` table is a latent `format:check` failure (#1992, wiki `e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms` while enforcing a regex subset, a caution box falsified by `parseInt` leniency, and `trustProxy: 1` being a hop count not "trust all proxies" (#1990, PR #2027)**, **fuzz the verbatim ports when a doc comment carries an induction proof (a hand-trace only re-reads the author's argument) + a safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940, PR #2032)**, **flex `gap` + child `margin` are additive not collapsing (bit twice in one PR — code AND the spec reviewing it) + a cohesive prop group modelled as N independent optionals + "leaves N chars for X" comments invite a guard test that pins a fiction: check whether X is bounded at all and whether the consumer clips or paginates (#1941, PR #2033)** +- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\`` doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python `len()` not `awk length()` (em-dashes are 3 bytes) + two open findings: the `BACKUP_DIR` default/gate is wrong on Architecture.md and API-Contract.md's `splitKind` table is a latent `format:check` failure (#1992, wiki `e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms` while enforcing a regex subset, a caution box falsified by `parseInt` leniency, and `trustProxy: 1` being a hop count not "trust all proxies" (#1990, PR #2027)**, **fuzz the verbatim ports when a doc comment carries an induction proof (a hand-trace only re-reads the author's argument) + a safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940, PR #2032)**, **flex `gap` + child `margin` are additive not collapsing (bit twice in one PR — code AND the spec reviewing it) + a cohesive prop group modelled as N independent optionals + "leaves N chars for X" comments invite a guard test that pins a fiction: check whether X is bounded at all and whether the consumer clips or paginates (#1941, PR #2033)**, **a mutation count is not evidence of independent coverage — diff a new test's assertion body against its neighbours before trusting its title, esp. a negative-dependency title whose body never names the dependency + when a structural guard would have to re-encode the coupling under removal, a reason-carrying comment IS the right tool (#1953, PR #2035)** - [Dual-rail aggregation](dual-rail-aggregation.md) — Rail A/B tagged-deposit invariants (#1891/PR #1894), residual-denominator rule, isSplit UNION - [Source-report split inference](source-report-split-inference.md) — budgetLines[]/deposits[] are this-source-scoped so array-shape gates are proxies; **`splitKind` SHIPPED #1911/PR #2015** incl. the ≠S-per-arm predicate, the residual arithmetic proving `(less deposit)` in both directions, the UNION-dedup/`COUNT(*)` trap, and why `isSplit` must be retained as an independent cross-check; pdfmake `'2*'` width trap; wiki + shared type JSDoc fixed (#1914, #1917/PR #1994) - [Story reviews](story-reviews.md) — per-story and per-PR review log diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index cc3ec62ca..2560ea5e8 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -1543,3 +1543,42 @@ row on its own — and it is the one that fails loudly if the Usage column ever and whether the consumer clips or paginates. Routed to #1950 (chunk-ceiling drift guard) rather than a bespoke test; needs `USAGE_TEXT_MAX_LENGTH` exported. Keep the constant in the editor — an input constraint living in the renderer inverts the dependency. + +## A duplicate test with a stronger title, and when a comment beats machinery (#1953, PR #2035) + +Two findings from reviewing the split of `LETTER_SUBJECT_FONT_SIZE` out of `SUBHEADER_FONT_SIZE` in +`client/src/lib/reportPdf/pageGeometry.ts`. The split itself was clean (own literal `12`, not an alias; +`headerFootprint()`/`PAGE_TOP_MARGIN` byte-identical; reason-carrying comment; inverted comment removed). + +**(a) A new test whose assertions duplicate an existing test, under a title that claims more.** +`pageGeometry.test.ts:98-101` was assertion-for-assertion identical to the pre-existing test at lines +160-167 (`toBe(93)` + `toBe(Math.ceil(headerFootprint() + 15))`, order swapped) but titled *"PAGE_TOP_MARGIN +does not depend on letterSubject.fontSize"* — a proposition its body never references. Zero added +discrimination; it catches exactly the older test's mutation set. + +**Why:** QA's mutation evidence *corroborated* rather than exposed it — "SUBHEADER 12→11 fails 4 tests" +reads as strong coverage, but two of the four are the duplicated pair. **A mutation count is not evidence +of independent coverage; it counts assertions, not propositions.** Compare each new test's failing-mutation +set against the existing suite's, not against zero. Same family as the PR #2008 "revert test proves a +different proposition than the one it licenses" and the PR #2004 r4 `count >= 1` finding. +**How to apply:** when a new test lands next to an existing one in the same file, diff the assertion bodies +before reading the titles. A title asserting a *negative dependency* ("X does not depend on Y") whose body +never mentions Y is the tell. + +**(b) When a comment is genuinely the right guard — the argument, not the shrug.** +Two adjacent `expect(...).toBe(12)` assertions protected only by a "do NOT deduplicate these" comment is +the right shape here. Not because no machinery exists, but because: (1) what is guarded is a *test's own +discrimination* — collapsing it loses coverage, it does not regress production, since the production split +and its comment stand regardless; and (2) **any structural guard would have to encode the coupling you just +removed** — "these two `number`s must be permitted to differ" is not expressible in TS, and its closest +approximation is exactly what already exists: two identifiers, two literals. The production split *is* the +structural guard. + +Rejected strengthenings, both costing more than the comment: asserting the constant through its role in +`headerFootprint()` restates production's formula in the test (the very anti-pattern `pageGeometry.ts`'s +own header comment warns against, from #1929); mirroring the `TABLE_SMALL_FONT_SIZE` constant-to-style tie +needs a module-private constant exported purely to be read by a test. +**How to apply:** before proposing machinery for a test-integrity concern, ask what the failure mode +actually costs (coverage loss vs regression) and whether the enforcement would re-express the coupling +under removal. If both answers are "yes", a comment naming the *reason* is the correct tool — and say so +affirmatively rather than as an absence of alternatives. diff --git a/client/src/lib/reportPdf/pageGeometry.test.ts b/client/src/lib/reportPdf/pageGeometry.test.ts index 21974cf19..63538984a 100644 --- a/client/src/lib/reportPdf/pageGeometry.test.ts +++ b/client/src/lib/reportPdf/pageGeometry.test.ts @@ -95,10 +95,20 @@ describe('pageGeometry — PDF_STYLES.letterSubject / PDF_STYLES.subheader font expect(PDF_STYLES['subheader']!.fontSize).toBe(12); }); - it('PAGE_TOP_MARGIN does not depend on letterSubject.fontSize: headerFootprint() sums only HEADER_FONT_SIZE, SUBHEADER_FONT_SIZE, SUBHEADER_MARGIN_TOP, and HEADER_BLOCK_BOTTOM_MARGIN — letterSubject is not one of its inputs, so a future change to the cover-letter subject size cannot reflow any page of the report (#1953 Verification)', () => { - expect(PAGE_TOP_MARGIN).toBe(93); - expect(PAGE_TOP_MARGIN).toBe(Math.ceil(headerFootprint() + 15)); - }); + // A third test asserting PAGE_TOP_MARGIN/headerFootprint() was deliberately removed here: it was + // assertion-for-assertion identical to the pre-existing "PAGE_TOP_MARGIN is a computed expression" + // test below (same two `toBe` checks, order swapped), so it added zero discrimination — the + // #1953 SUBHEADER_FONT_SIZE 12->11 mutation failed both copies for the same reason, not two + // independent reasons. Neither assertion actually references `letterSubject`, so a title claiming + // "PAGE_TOP_MARGIN does not depend on letterSubject.fontSize" overclaimed what the body proved. + // That guarantee (changing LETTER_SUBJECT_FONT_SIZE cannot move PAGE_TOP_MARGIN) is not + // expressible as a standing assertion here: neither constant is exported, so the only proof + // available is the manual mutation test performed for #1953 (see PR #2035 / the qa-integration- + // tester's #1953 report) — LETTER_SUBJECT_FONT_SIZE 12->13 left PAGE_TOP_MARGIN and + // headerFootprint() unchanged, confirmed by re-running this file with the mutation applied and + // reverted. If a future change makes that guarantee expressible in code (e.g. an exported + // computation helper), assert it directly rather than restating the existing PAGE_TOP_MARGIN + // formula test under a claim it doesn't back. }); describe('pageGeometry — printableWidth (scenario 1)', () => { diff --git a/client/src/lib/reportPdf/pageGeometry.ts b/client/src/lib/reportPdf/pageGeometry.ts index 70fc0b999..344dbd921 100644 --- a/client/src/lib/reportPdf/pageGeometry.ts +++ b/client/src/lib/reportPdf/pageGeometry.ts @@ -73,6 +73,7 @@ const SUBHEADER_MARGIN_TOP = 4; // PDF_STYLES.subheader margin, below const HEADER_BLOCK_BOTTOM_MARGIN = 20; // buildPageHeader's own margin: [0,0,0,20] const HEADER_TOP_GAP = 15; // visible separation kept above the computed footprint +// --- Cover letter typography (#1953) — NOT page-geometry arithmetic; no geometry consumer --- /** * Cover letter subject-line font size, pt — PDF_STYLES.letterSubject below, only. This equals * SUBHEADER_FONT_SIZE (12) above by COINCIDENCE, not by design: it is deliberately its own From a0470ab60218b19c321060e6708267585e638b5d Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 07:44:26 +0200 Subject: [PATCH 3/3] docs(memory): record that a mutation-failure count can hide a duplicate A mutation count alone does not prove each failing assertion pulls independent weight -- the #1953 SUBHEADER_FONT_SIZE mutation failed four tests, but two of them were the same assertion written twice. And a test title must not claim more than its body proves, since a reader consults the name to decide whether a guarantee is already covered. Refs #1953 Co-Authored-By: Claude qa-integration-tester --- .../qa-integration-tester/MEMORY.md | 1 + .../story-1953-independent-pinning.md | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 .claude/agent-memory/qa-integration-tester/story-1953-independent-pinning.md diff --git a/.claude/agent-memory/qa-integration-tester/MEMORY.md b/.claude/agent-memory/qa-integration-tester/MEMORY.md index 41dd46d63..101380647 100644 --- a/.claude/agent-memory/qa-integration-tester/MEMORY.md +++ b/.claude/agent-memory/qa-integration-tester/MEMORY.md @@ -15,6 +15,7 @@ ## Recent bug/story notes (2026-08) +- [Issue #1953 — independent-pinning test hygiene](story-1953-independent-pinning.md) (2026-08-06, PR #2035) — a duplicate assertion inflated a mutation-failure count without adding discrimination; a test title claimed an independence guarantee ("X does not depend on Y") that no assertion in its body actually tested — dedupe against the *pre-existing* suite, not just new tests, and re-check names against bodies only. - [Issue #1941 — EditableField maxLength](story-1941-editable-field-maxlength.md) (2026-08-06) — jsdom does NOT clamp controlled-input value/fireEvent.change against `maxlength` attr (verified empirically); a broken #1932 DOM-shape test was RIGHT — it caught a real additive flex-gap+margin spacing regression, don't reflexively "fix" a test a new wrapper breaks; ariaDescribedBy has exactly 4 reachable id-count combos (0/1/2/3, fixed order). - [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 diff --git a/.claude/agent-memory/qa-integration-tester/story-1953-independent-pinning.md b/.claude/agent-memory/qa-integration-tester/story-1953-independent-pinning.md new file mode 100644 index 000000000..ec6031aa9 --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/story-1953-independent-pinning.md @@ -0,0 +1,47 @@ +--- +name: story-1953-independent-pinning +description: Issue #1953 (LETTER_SUBJECT_FONT_SIZE/SUBHEADER_FONT_SIZE split) — mutation-count pitfall and test-title overclaim caught in PR #2035 review +metadata: + type: feedback +--- + +Writing regression coverage for `client/src/lib/reportPdf/pageGeometry.test.ts` (two module-private +constants, `LETTER_SUBJECT_FONT_SIZE`/`SUBHEADER_FONT_SIZE`, deliberately split apart with equal +literals). Wrote 3 new tests: pin `PDF_STYLES.letterSubject.fontSize`, pin +`PDF_STYLES.subheader.fontSize`, and a third asserting `PAGE_TOP_MARGIN`/`headerFootprint()`. Ran the +standard backup/restore mutation technique ([[story-1929-round2-real-render-technique]]) both +directions and reported "confirmed discriminating" because the SUBHEADER_FONT_SIZE 12->11 mutation +failed 4 tests, not 1. + +`product-architect` caught what the mutation count masked, in PR #2035 review: the third test was +**assertion-for-assertion identical** to a pre-existing test 60 lines below (same two `toBe` checks, +order swapped) — zero added discrimination. The 4-failure count was itself the tell in hindsight: it +included *both* copies of the duplicated pair failing for the identical reason, not four independent +reasons. A mutation count only proves "this mutation moves some needle" — it does not prove each +individual failing assertion is pulling separate weight. Check for duplicate assertions against the +*existing* suite before citing a multi-test-failure count as evidence of thoroughness. + +Second, sharper problem: the test's title claimed *"PAGE_TOP_MARGIN does not depend on +letterSubject.fontSize"* while **neither assertion in the body referenced `letterSubject`** — the +title asserted a causal-independence guarantee (issue's own Verification section: "change the +subject size and no page reflows") that the body never tested, because neither constant is exported +so a live-mutation-based standing assertion isn't expressible in the permanent suite. **A test name +that licenses a stronger claim than its body establishes is worse than no test** — a future reader +trusts the name, concludes the guarantee is covered, and stops looking. + +Resolution: deleted the duplicate test outright (architect's stated preference over retitling, since +the pre-existing test already covers the formula and duplication is its own maintenance cost) and +left an explanatory comment at the deletion site documenting: why it was removed, that the "no page +reflows" guarantee was only proven by the one-off manual mutation test (not a standing assertion), +and what would make it expressible in code if that changes later (an exported computation helper). + +**Why**: mirrors [[bug-1955-echo-race-harness]]'s mutation-probe discipline but adds a check that +discipline didn't include — dedupe candidate assertions against the pre-existing suite, not just +against the new tests in the same PR, before trusting a failure count. + +**How to apply**: whenever citing "N assertions failed under this mutation" as proof of +discrimination, first check none of those N are a near-identical restatement of a pre-existing test +elsewhere in the file (same expected values, same formula, reordered). And before naming a test, +re-read the name against only the assertion body — if the name asserts an independence/causality +claim ("X does not depend on Y") but no assertion in the body actually varies or references Y, either +write an assertion that does or narrow the name to what's actually checked.