diff --git a/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md b/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md index ffc5485fc..fff5d9e88 100644 --- a/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md +++ b/.claude/agent-memory/e2e-test-engineer/issue-1959-inline-meta-and-labels.md @@ -21,9 +21,21 @@ class*="inlineNote">` in the **Allocated Amount cell**: `(partial)` / `(less dep `depositReducedInvoiceIds.size > 0` → one `'depositReduced'` footnote. `footnotesBlock` / `footnoteItems` are NO LONGER negative-only guards. Scenarios with split or deposit-reduced rows must assert a **positive** count; constituted-deposit-only rows (Scenario 17) still assert - `toHaveCount(0)` because neither set is non-empty for them. Scenario 18 (two split invoices) - asserts `footnotesBlock` count=1, `footnoteItems` count=1, and the legend sentence IS present in - `main`'s text content. + `toHaveCount(0)` because neither set is non-empty for them. + + **AC5 Issue #1980 update (fix/2003-1980-realrender-overflow-legend-assertions, commit 4ffc1425):** + Scenario 18 extended with a third invoice (`${testPrefix}-SPLITDR-003`) that is BOTH split AND + deposit-reduced (split via `seedSplitInvoice` across the same two sources + untagged deposit via + `createDepositViaApi` with `budgetSourceId: null`). Assertions updated: + - `footnoteItems` count: **2** (was 1) + - `footnoteItems.nth(0)` contains the split sentence + - `footnoteItems.nth(1)` contains "This position reflects deposits claimed separately." + - `invoice3`'s row asserts `inlineNote` count=2 (both `(partial)` and `(less deposit)`) + - `toContainText('(less deposit)')` on the page-wide text confirms both labels appear + + **Deposit-reduced trigger**: `budgetSourceId: null` on a deposit = untagged = never matches + any source ID → `isDepositReduced: true` from `buildReportContent.ts` when the deposit is on an + invoice that also has budget-line allocation to the reported source (`isSplit: true` too). 2. `.usageAreaText` sub-line + the separate editable `Attachments Note` column → ONE read-only `.usageMetaText` line inside the Usage cell: `[areaText, attachmentsNote].join(' · ')` diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index ef16e91fa..1cbc88a80 100644 --- a/.claude/agent-memory/product-architect/MEMORY.md +++ b/.claude/agent-memory/product-architect/MEMORY.md @@ -2,11 +2,11 @@ ## 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) +- [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) - [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 †/‡ classification is a proxy; proposed `splitKind`; pdfmake `'2*'` width trap; wiki + shared type JSDoc both fixed (API-Contract #1914, sourceReport.ts #1917/PR #1994) - [Story reviews](story-reviews.md) — per-story and per-PR review log -- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) — ADR-034 B4 rule + legend addendum landed in PR #1979; per-locale header character budget + "no interface `t` in header/footer" (#1937/#1938, PR #1982); pdfmake `Content` is unspreadable (TS2698) but `Object.assign` needs no cast, and per-item `wordBreak`/newline-only-run facts (#1968, PR #2002). **ADR-034 debt fully PAID 2026-08-04 (#1914)**: width rule #1 (`max(horizontalRatio) <= 1`, not `_minWidth`), module table, override keys, dontBreakRows/height-bound section, injection-only locale contract. `merge.ts` footer/header interface-`t` defect **CLOSED** (footer PR #2000/#1993, header #1938); `TFunction` removed from all of `reportPdf/*` in #2001/PR #2007, so the locale contract is now compiler-enforced there and `buildReportContent.ts` is the single legitimate `TFunction` entry point — ADR-034 lines 82/178/186-188/200/219 all state the old weaker contract and needed a wiki pass (flagged in the PR #2007 review). **ADR-034 rule #1 is documented but unenforced (`horizontalRatio`: 0 hits in `client/`) → issue #2003, mine** +- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule, document-level deduplicated legend (#1965) — ADR-034 B4 rule + legend addendum landed in PR #1979; per-locale header character budget + "no interface `t` in header/footer" (#1937/#1938, PR #1982); pdfmake `Content` is unspreadable (TS2698) but `Object.assign` needs no cast, and per-item `wordBreak`/newline-only-run facts (#1968, PR #2002). **ADR-034 debt fully PAID 2026-08-04 (#1914)**: width rule #1 (`max(horizontalRatio) <= 1`, not `_minWidth`), module table, override keys, dontBreakRows/height-bound section, injection-only locale contract. `merge.ts` footer/header interface-`t` defect **CLOSED** (footer PR #2000/#1993, header #1938); `TFunction` removed from all of `reportPdf/*` in #2001/PR #2007, so the locale contract is now compiler-enforced there and `buildReportContent.ts` is the single legitimate `TFunction` entry point — ADR-034 lines 82/178/186-188/200/219 all state the old weaker contract and needed a wiki pass (flagged in the PR #2007 review). **ADR-034 rule #1 is WRONG A SECOND TIME (proven in the PR #2008 review, 2026-08-05): `horizontalRatio` is a cell-origin bound, not a content-extent check — it is unconditionally `<= 1` on the all-fixed-width production table and cannot detect token overflow. The check that works is per-cell `_minWidth <= _calcWidth` (the ADR's `wordBreak` false-positive rationale for banning `_minWidth` is empirically false). **Third correction PAID 2026-08-05** (wiki `da1324b`): rule #1 leads with per-cell `_minWidth`, `maxHorizontalRatio` re-scoped to table-box positioning with a vacuity warning, `_minWidth` ban split table-level vs per-cell, 3rd Deviation Log row added. Semantics verified by mutation test in the PR #2008 round-2 review; **two follow-ups still open on the page — every quoted figure (69.28/33.54/266.16pt) is stale, and the rule says "every cell" while the test covers only Usage.** - [Diary drafts pattern](diary-drafts-pattern.md) — ADR-022 draft lifecycle via status column on parent table - [EPIC-03 refinement](epic03-refinement.md) — 40 consolidated refinement items - [EPIC-04 household items](epic04-household-items.md) · [EPIC-05 budget](epic05-budget.md) · [EPIC-17 i18n](epic17-i18n.md) · [EPIC-18 areas & trades](epic18-areas-trades.md) @@ -39,6 +39,11 @@ `git -C wiki log` (shows unpushed commits as HEAD) or a refspec-less `fetch` (leaves origin/master stale) - Pages: Architecture, Schema, API-Contract, Home, ADR-Index, ADR-NNN-*, Style-Guide (ux-designer), Security-Audit (security-engineer) - **Always push wiki before creating the PR** — the submodule ref must be committed on the feature branch. If you push wiki content outside the branch, flag that the PR's ref needs bumping. +- **No workflow in `.github/workflows/` checks out submodules** — an unpushed wiki commit recorded as the + parent's submodule ref keeps CI fully green and merges silently, then breaks `git submodule update` on + `beta`. Recurred on PR #2008 (recorded `da1324b`, remote at `b12ebb1`; I pushed it during re-review). + **Make the ls-remote-vs-ls-tree check a standing step of every PR review that touches `wiki`**, not just + of your own wiki pushes — a later commit in the same branch can advance local HEAD past what was pushed. ### Wiki Update Discipline (CRITICAL) diff --git a/.claude/agent-memory/product-architect/client-pdf-pipeline.md b/.claude/agent-memory/product-architect/client-pdf-pipeline.md index 96c14f788..c04c0e669 100644 --- a/.claude/agent-memory/product-architect/client-pdf-pipeline.md +++ b/.claude/agent-memory/product-architect/client-pdf-pipeline.md @@ -430,3 +430,96 @@ The grey `areaText`/`attachmentsNote` suffix inherits `tableCell`'s `TABLE_BODY_ deposit/split labels in the AllocatedAmount cell do (`overviewPdf.ts:720-740`). That is why `usageSafeTokenChars` (from `BODY_WORST_CASE_CHAR_WIDTH_PT`) is the correct break threshold for it. Adding a `fontSize` to those runs later would decouple threshold from rendered size. + +## ADR-034 rule #1 is WRONG A SECOND TIME: `horizontalRatio` is a cell-origin bound, not an overflow check + +Measured against `pdfmake@0.3.11` during the PR #2008 review (#2003). `ElementWriter.addLine` +(`src/ElementWriter.js:32`) captures `position = this.getCurrentPositionOnPage()` **before** placing the +line, so `positions[].horizontalRatio` records the **left origin of each text line**, never its right +extent. Consequences: + +| fixture (A4, 40pt margins, five 80pt fixed cols) | `max(horizontalRatio)` | +| ------------------------------------------------ | ---------------------- | +| 400-char unbreakable `W` token in an 80pt column | 0.7006 | +| **4000**-char unbreakable `W` token, same column | 0.7006 (identical) | +| 4000-char token in one 500pt column | 0.0097 | +| `widths: [600, 50]` | 1.1916 | +| widths summing 900pt | 1.2091 | + +The overview table is all-fixed-width summing to exactly `printableWidth()` (asserted at +`realRender.test.ts:1751`/`:1890`), so **no cell origin is ever past the right margin** ⇒ +`max(horizontalRatio) <= 1` is _unconditionally true_ on every production fixture. It cannot detect token +overflow within a column — the failure mode `safeTokenChars`/`wordBreak: 'break-all'` exist to prevent. +It only catches table-box overflow, which the `tableOffsetsTotal + sum(_calcWidth) === printableWidth()` +assertion already forbids more strictly. Note even B2 as literally described (last column starts at +~511pt of 515.28) would have scored < 1. + +**The assertion that DOES work: per-cell `cell._minWidth <= table.widths[c]._calcWidth`.** +`_minWidth` is computed _after_ `TextBreaker` applies `wordBreak`, so `break-all` collapses it to one +glyph. Measured on the real inline-run cell shape, Usage column `_calcWidth` = 69.28pt: + +| cell | `_minWidth` | +| ------------------------------------------------- | ----------------- | +| prose + grey 30-`W` areaName **with** `break-all` | 33.54 → PASS | +| same **without** `break-all` (the regression) | 266.16 → **FAIL** | +| all plain prose | 33.54 → PASS | + +Same cell with `'W'.repeat(400)`: 3548.83 without `break-all`, **8.87** with. So ADR-034's +"Do not assert `_minWidth`" paragraph is right about the **table-level** form but its `wordBreak` +false-positive rationale is **empirically false**, and it wrongly generalises to the **cell-level** +form, which is the discriminating revert-provable check. Needs no new renders — existing renders already +mutate both fields. + +**PAID 2026-08-05** (wiki `b12ebb1`, parent ref `2a1862c7`, on the PR #2008 branch). Rule #1 now leads with +the per-cell `_minWidth <= widths[i]._calcWidth` content-extent check; `maxHorizontalRatio` was re-scoped +(not deleted) to "no element is positioned past the right margin" with an explicit vacuity warning against +asserting it on production content; the `_minWidth` ban was split into table-level (still banned, the +`starMaxMin` reason) vs per-cell (the correct check); finding B2's cross-reference now says its quoted +`_minWidth` figures are table-level sums. Third Deviation Log row added, carrying the process lesson: +**a revert test proves the helper can fire on _some_ input, not that it can fire on the input the rule is +about — the negative control must mutate the production code path the rule governs.** Note the ADR row I +wrote on 2026-08-04 is the one that introduced the wrong guidance, and #2003 implemented it faithfully: +when I replace a rule, I owe the replacement its own falsifiability check, not just a source-line citation. + +### Probing pdfmake from bare Node (no jsdom) + +Memory previously said `getBuffer()` callbacks don't fire in bare Node CJS. Workaround that does work +for layout-geometry probes (`createPdf().getBlob()` not needed): + +```js +const PdfPrinter = require('/node_modules/pdfmake/js/printer.js').default; +const printer = new PdfPrinter( + fonts, + { existsSync: () => false }, + { resolve() {}, resolved: async () => {} }, + () => true, +); +const doc = await printer.createPdfKitDocument(def); // async in 0.3.x; mutates def.content in place +``` + +Fonts: real TTF paths under `node_modules/pdfmake/fonts/Roboto/`. The `virtualfs`/`urlResolver`/ +`localAccessPolicy` ctor args are required (positional) or `resolveUrls` throws on `undefined.resolve`. +Line numbers differ between `src/` (528) and the `js/` build (490) — cite the file, not just the number. + +## Measured overview-table width/`_minWidth` figures (PR #2008 re-review, 2026-08-05) + +Measured directly by instrumenting `realRender.test.ts`'s `#2003` block. **Use these, not the numbers +in ADR-034's rule #1 prose — those are stale (see M1 in the PR #2008 round-2 review).** + +| Quantity | Value | Source | +| ------------------------------------------------ | -------- | --------------------------------------- | +| Usage `_calcWidth`, 6-col (`claim`) | 186.78pt | `USAGE_WIDTH_6COL`, `overviewPdf.ts:58` | +| Usage `_calcWidth`, 7-col (`budget-overview`) | 138.28pt | `USAGE_WIDTH_7COL`, `overviewPdf.ts:57` | +| Cell `_minWidth`, 30x'W' **with** `break-all` | 7.098pt | single 'W' glyph @ 8pt `tableCell` | +| Cell `_minWidth`, 30x'W' **without** `break-all` | 212.93pt | 30 x 7.098pt | + +Which use case maps to which shape: `claim` → 6 columns, `budget-overview` → 7 columns +(`reportContent.isOverview` selects the width, `overviewPdf.ts:536`). + +**69.28pt is not the Usage column width and has not been for some time** — it is a pre-rebalance +figure that still appears at ADR-034 lines 105, 120, 129, 138 and in the Deviation Log. Treat any +`69.28` in this pipeline's docs as suspect. + +Narrowest text column is **Vendor at 45pt** (`VENDOR_WIDTH`, `overviewPdf.ts:26`, +`VENDOR_SAFE_TOKEN_CHARS` = 5) — it is the binding constraint for the `wordBreak` rule, not Usage, +and as of PR #2008 it has no `_minWidth` coverage. diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index a64a7ec58..abe21f03f 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -1213,3 +1213,86 @@ existed in `overviewPdf.test.ts`; all six contribute zero coverage for the injec Only fixtures with a **unique sentinel** (`FETCH-SENTINEL`) discriminate. When a PR adds an AC-driven sentinel block alongside pre-existing key-echo assertions, say in the review that the sentinel block is the load-bearing one and must not later be "harmonised" into the surrounding style. + +## A revert test can prove a _different_ proposition than the one it licenses (#2003, PR #2008) + +PR #2008 added a genuine revert test (synthetic `widths: [600, 50]` table → `maxHorizontalRatio > 1`) +and then used it to justify three `<= 1` assertions on production fixtures that target a **different +failure mode** (over-wide token inside a fixed-width column) which the helper is provably blind to. The +revert test passes, the assertions pass, and nothing is enforced. Seven extra real pdfmake renders for +zero signal. + +**The check:** a revert test only licenses assertions whose failure mode it actually reproduces. Ask +"is the thing I broke in the revert fixture the _same mechanism_ as the thing the production assertions +guard?" — not merely "does the helper return a different number for _some_ input?" A synthetic fixture +shaped unlike production content is the tell. + +Corollary: when a helper's own doc comment documents a limitation ("overflow is only detectable +when..."), treat that paragraph as a **finding about the assertions**, not as a caveat to be noted and +moved past. #2008's comment stated the limitation accurately and the tests below it ignored it. + +## Implementing a documented rule for the first time is when you learn the rule is wrong + +ADR-034 rule #1 has now been wrong twice (`_minWidth` table-form → corrected 2026-08-04 → +`horizontalRatio`, falsified 2026-08-05 during PR #2008). Both times the wrongness surfaced only when +someone tried to _enforce_ it. An unenforced documented bar is not merely unenforced — it is +**unvalidated**, and citing it as authority ("verified against `DocumentContext.js:490`") verifies the +field exists, not that the assertion means what the prose claims. + +**How to apply:** when a PR lands the first implementation of a documented rule, review the _rule_ as +well as the code, and never let the PR add an "Implementing test:" pointer without checking that the +test enforces the property the prose states. A pointer linking a bar to a non-enforcing test is worse +than no pointer — it retires the debt on paper. + +## Forked tree-walk helpers: three copies of `collectAllStrings` + +`realRender.test.ts` had two describe-scoped copies (`:837`, `:1094`) before PR #2008 added a third, +each with a comment justifying the fork ("so this block is self-contained"). Self-containment is not a +reason to fork a pure recursive tree walk — hoist to module scope. Watch for this whenever a new +top-level describe block is appended to a long test file. + +## Mutation-test the fix, don't read it (PR #2008 round 2, 2026-08-05) + +Round 1's finding was "this assertion cannot fail". The round-2 fix _looked_ right on inspection. The +only thing that settled it was reverting the production line the rule governs +(`wordBreak: 'break-all'`, `overviewPdf.ts:385`) and confirming 8-of-9 tests flip red while the +revert-test stays green. Cost: two jest runs. **Whenever a round-1 finding was "vacuous assertion", +round 2's verification is a mutation, not a re-read** — you already know reading cannot distinguish the +two states. Restore with a `cp` backup and confirm `git status --porcelain -- ` is empty after. + +Corollary worth the extra run: also capture the _pass-side_ numbers (add a temporary `console.log`, +then `git checkout` the test file). That is what exposed M1 below — the assertion was correct but every +figure documenting it was wrong, which reading alone would never have surfaced. + +## Corrected prose re-imports the very figures the correction reclassified + +ADR-034's 3rd rule-#1 correction (PR #2008) explicitly reclassified `_minWidth` 33.54pt/266.16pt as +**table-level sums** and diagnostics (ADR line 162) — and then quoted those same two numbers, plus a +stale 69.28pt Usage width, as the _measured evidence_ for the new **per-cell** check. Real values: +`_calcWidth` 186.78/138.28pt (= exported `USAGE_WIDTH_6COL`/`_7COL`, `overviewPdf.ts:57-58`), +`_minWidth` 7.098pt with `break-all`, 212.93pt without. + +**Why it happens:** a correction rewrites the _claim_ by editing around the existing sentence, and the +numbers ride along because they were never the thing under dispute. The rule's semantics get fixed +three times while its arithmetic is never re-measured. + +**How to apply:** when reviewing a corrected documented bound, verify the _numbers_ separately from the +_semantics_ — a stale figure elsewhere on the page (69.28pt also survives at ADR lines 105 and 138) is +the tell that it predates a geometry change. Prefer citing exported constants by name over literals so +the doc cannot drift from the code. + +Same class, conceptual variant: the ADR's no-false-positive argument read "plain prose yields 33.54pt". +Prose `_minWidth` is its widest **word**, which can be large — that is the entire reason over-long +tokens need flagging. A no-false-positive claim must be stated as a _condition_ ("prose whose words all +fit under `safeTokenChars`"), never as a constant. + +## "For every cell" in the rule, one column in the test + +ADR-034 rule #1 states the bar as "for every cell of the overview table"; #2003's implementing test +reads only `body[i][usageColIndex]`. The **uncovered** cells are the tighter ones: Vendor body at 45pt +/ `VENDOR_SAFE_TOKEN_CHARS` = 5 (`overviewPdf.ts:620`) and every header cell via `buildHeaderCell`, +where DE is the documented binding locale (#1937). Usage at 138-187pt is the _widest_ text column. + +**How to apply:** when a test lands against a universally-quantified documented rule, check the +quantifier. Picking the column the issue happened to mention is not the same as picking the binding +one — and when the render already happened, iterating all cells is nearly free. diff --git a/.claude/agent-memory/product-architect/story-reviews.md b/.claude/agent-memory/product-architect/story-reviews.md index 5bed19434..5603e71f6 100644 --- a/.claude/agent-memory/product-architect/story-reviews.md +++ b/.claude/agent-memory/product-architect/story-reviews.md @@ -954,3 +954,31 @@ Left three non-blocking follow-ups: stale signature in `overviewPdf.test.ts:10`' (cross-reference rot, one layer down from the ADR lines this PR fixed), two read-side `as Map` casts in `merge.test.ts:316,330`, and the `attachmentType` dynamic-key echo in `buildReportContent.ts:82` (same key-echo class, outside scope). + +## PR #2008 — ADR-034 horizontal-overflow + legend coverage (#2003, #1980) — CHANGES REQUESTED (2026-08-05) + +Test-only PR. **Posted as `gh pr comment`, not `--request-changes`** (GitHub refuses review verdicts on +your own PR — recurring constraint). + +Blocking finding: the three production `maxHorizontalRatio(...) <= 1` assertions **cannot fail**, so +#2003's explicit "must demonstrably fail" AC is unmet. Full mechanism, measurements and the working +replacement (per-cell `_minWidth <= _calcWidth`) are in [client-pdf-pipeline.md](client-pdf-pipeline.md); +the two generalisable review lessons are in [recurring-patterns.md](recurring-patterns.md). ADR-034 +rule #1 needs its **third** correction — mine, not the author's fault; the implementation followed my +prose faithfully. + +Recommended split: land the #1980 half (sound), rework the #2003 half separately. + +Verified sound and worth not re-deriving: + +- AC4's `budgetLines: []` + untagged deposit **is** the right flag isolation (`buildReportContent.ts:157` + gates `splitInvoiceIds` on `isSplit && budgetLines.length > 0`). +- The E2E deposit-reduced path is genuine end-to-end: `sourceReportService.ts:389` filters deposits with + `budgetSourceId === null || === sourceId`, so an untagged deposit **survives** into the response; + combined with `seedSplitInvoice`'s `isSplit` that yields `isDepositReduced === true`. No fixture/production + divergence — this was the specific gap I was asked to chase. +- `footnoteItems.nth(0)`/`nth(1)` correctly pins `buildReportContent`'s deterministic push order. + +Non-blocking: en-only worst-case token fixtures (#2003 asks for both locales); third forked +`collectAllStrings`; `src/DocumentContext.js:528` vs ADR's `:490` citation drift (both correct — src vs +js build — but name the file). diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index 5248e3fa1..4cf80f37b 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -40,6 +40,7 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) - Photo: #1723 lightbox picker UX (2026-06-16) - **DataTable: #1955** two-column toggle race silently hides 2nd column, all 6 DataTable pages (Should Have, S, Backlog, 2026-08-02). See [datatable-column-preference-race.md](datatable-column-preference-race.md) — records that **fast clicking is the SAFE case** (I judged this backwards; debounce `clearTimeout` coalesces rapid input, the >500ms reading-pace gap is the reachable one) and that #1920's E2E-only fix (`InvoicesPage.enableColumn()` awaits the PATCH) makes CI green **without** fixing production — don't close #1955 on a green shard. - **#1957** latent cross-file E2E test-isolation hazard (shared-admin `user_preferences` writes under `fullyParallel`, `LocaleContext.syncWithServer` actively flips a victim test's locale) — Should Have, bug, Backlog, 2026-08-02/03, filed from `/fix-e2e` work on PR #1956. Scoped as an audit + per-spec sweep, not a single-file fix — found a second live instance (`diary-uat-fixes.spec.ts` vs `dashboard.spec.ts`, key `dashboard.hiddenCards`) while researching it. Distinct from #1955 (production race) and #1920 (E2E workaround for #1955). Detail in [e2e-shared-admin-preference-hazard.md](e2e-shared-admin-preference-hazard.md). +- **PR #2008 (#2003 ADR-034 overflow enforcement + #1980 legend assertions)** — **REJECTED round 1** (`664bf048`, 2026-08-05), verdict in a comment (human-authored PR). Three criticals: **C1** the new `max(horizontalRatio) <= 1` assertions are the constant `0` — pdfmake records the write cursor's _left_ edge (`DocumentContext.js:515-529`), so a left-margin-anchored full-width table can never exceed 0; proven by 3 mutations (`WORST_CASE_CHAR_ADVANCE_EM 1.04→0.1` and `VENDOR_WIDTH 45→600` both stayed green; inverting the bound to `-1` printed `Received: 0` at all 8 sites in all states). **C2** the ADR pointer claims the assertion is "non-vacuous", refuted by the PR's own helper comment. **C3** the new E2E test is red twice (NBSP: `locator.textContent()` doesn't normalize, `toContainText()` does). New reusable patterns: **invert an inequality's bound to read the measured value**, **a synthetic revert-test proves the helper not the assertion**, **a comment conceding non-coverage IS the finding**, **a documented ADR bar can be unmeasurable — escalate as a rule correction**. → **APPROVED round 2** (`4f4b93e3`): metric replaced by per-cell `_minWidth <= widths[i]._calcWidth` (8 assertions, claim/budget-overview × en/de), ADR-034 rule #1 corrected (3rd correction + Deviation Log), **#2003's body amended with a supersession block** so the dead `<= 1` bullet isn't read as live spec. Round-2 patterns: **verify a vacuity fix by re-running the exact mutations that stayed green last round** (both went green→all-8-fail); **"moves with the input" is the positive signal** (`212.93`/`7.10`/~`33`; legend `_minWidth` 36.80 en vs 54.86 de); **when a fix removes a vacuous assertion, check it was REPLACED not just deleted** (#1980 AC1 went vacuous→**absent**, carried as Medium — re-grading my own round-1 Medium upward would move goalposts); **probe that a measurement exists before demanding it**; **prove an E2E failure is a flake via a byte-identical-subtree green run** (`git diff -- e2e/` empty + all-16-green 16 min earlier + shard 3/16 green last round rules out a shard-boundary shift). #2003 Done on merge; **#1980 Done only if M1 lands**. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"PR #2008". - **PR #2007 (#2001 remove `TFunction` from `reportPdf/*`)** — **APPROVED round 1** (`df7d2c73`, 2026-08-05), all 9 ACs met, **Done on merge** (every AC machine-checkable, no UAT). One Medium non-blocking: the fix re-opened the same hazard class in a new form (`labels.skipReasonLabels[reason as 'a'|'b'] ?? reason` — unchecked cast + silent fallback echoes a raw identifier if a third `SkippedDocument['reason']` is added). **Pattern: when a story's purpose is "close the class", check whether the fix introduces a fresh instance of it.** Verification: grep ACs get run directory-wide _and_ one step past the AC text (also checked `i18n`/`useTranslation`/`toLocale`/`Intl.`); AC7 discrimination proven by 3 mutations in a throwaway `/tmp` worktree (symlink root `node_modules`) — **mutate the i18n _key_, not just the value**; call-time→build-time refactors need a `useMemo` dep-array staleness check. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#2001 reviewed". - **#2005** now **CLOSED** — all 16 E2E shards green on `df7d2c73`; the promotion blocker below is cleared. E2E shard 8/16 red across four beta PRs — dashboard "New Invoice" (Scenario 13, #1735) opens no modal in either Paperless branch, fails on retry too (bug, Must Have, Todo, 2026-08-05, filed from PR #2004 round-5 sign-off). **Blocks the next `beta`→`main` promotion** (`E2E Gates` is main-only). Proved PR-independent by checking the head commits of #1999/#2000/#2002 — not `beta`, which never runs full E2E. AC1 is a _classification_ AC so the dev-team-lead keeps the Test Failure Debugging Protocol call. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"round 5". - **PR #2004 (#1888 + #1910)** — **APPROVED, M1 closed round 5** (`6a3eb7ec`, 2026-08-05): `lang={lang}` on the `