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
2 changes: 1 addition & 1 deletion .claude/agent-memory/product-architect/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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('<amount> (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('<amount> (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
Expand Down
39 changes: 39 additions & 0 deletions .claude/agent-memory/product-architect/recurring-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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 #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
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Loading
Loading