diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index 7091a2734..ff9acee2d 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -305,3 +305,38 @@ labelled U+046C when it is U+04AC. **My own figures are not exempt.** Both HIGH findings in PR #1948 traced to numbers I supplied in the #1929 round-4 review and the PO transcribed faithfully into ACs. When a later PR exists only to make my recommendations true, re-derive them from scratch rather than checking transcription fidelity. + +## E2E: the report wizard's cover letter auto-enables only for sources with contact/reference (#1932 / PR #1951) + +`ReportWizardPage.tsx` sets `setIncludeCoverLetter(Boolean(r.source.contactAddress || r.source.reference))` +on report fetch, and `reachStep5()` in `e2e/tests/budget/reportWizardEditableContent.spec.ts` never clicks +`includeCoverLetterCheckbox`. `createBudgetSourceViaApi` defaults **only** `sourceType`/`status`. + +So a scenario seeding `{name, totalAmount}` alone gets `content.coverLetter === null`, no +`[class*="coverLetterCard"]`, and every `wizard.letterField(...)` locator resolves to **zero elements**. +The failure is asymmetric and easy to miss in review: + +- `await expect(resetButtonFor(field)).not.toBeVisible()` **passes vacuously** on 0 elements; +- `editField(field, v)` (→ `field.fill()`) then times out. + +**Reviewer move:** for any E2E scenario touching `letterField`/`coverLetterCard`, grep its +`createBudgetSourceViaApi` seed for `contactAddress`/`reference` before anything else. All pre-existing +letter scenarios seed both. Same class as the "vacuously-passing negative assertion" smell already listed +above — a `.not.toBeVisible()` that can pass because the element never existed proves nothing. + +## pdfmake: `.positions` is the post-render line count; `._inlines` drains to `[]` (#1932) + +For "did this text node really render N visual lines" assertions in `realRender.test.ts`: + +- `LayoutBuilder.js` L1183 `node.positions.push(this.writer.addLine(line))` runs **once per rendered line** + inside `processLine`'s loop, with `node` bound to the text node; `decorateNode()` L1279 inits + `node.positions = []`. So `positions.length` === visual line count, and `positions[i].top` gives spacing. +- `._inlines` looks like the natural source (DocMeasure sets it) but LayoutBuilder consumes it as a queue + (see the `unshift` on the reflow path, L1174) — it is `[]` by the time `getBlob()` resolves. +- An **empty** text node reserves a full line height (~18pt at 11pt/1.4) — it is not collapsed. This is why + the cover letter's signature block can be emitted unconditionally with no NBSP workaround, and also why + the `{ text: '', pageBreak: 'after' }` sentinel costs a real line. + +Caveat when reviewing such a proof: line-count + uniform-gap catches collapse/doubling/per-token reflow, +but **not** content rewrites that preserve line count. Insist on `expect(node['text']).toBe(input)` +alongside the count. diff --git a/.claude/agent-memory/product-architect/story-reviews.md b/.claude/agent-memory/product-architect/story-reviews.md index bef601e2d..7840a5819 100644 --- a/.claude/agent-memory/product-architect/story-reviews.md +++ b/.claude/agent-memory/product-architect/story-reviews.md @@ -464,3 +464,32 @@ Residual, non-blocking: `PDF_DEFAULT_STYLE` stayed in `merge.ts` with hardcoded `lineHeight: 1.4`, while `pageGeometry.ts:55` has `DEFAULT_LINE_HEIGHT = 1.4 // matches merge.ts's defaultStyle.lineHeight` — the load-bearing half of the duplicate (feeds `headerFootprint()` -> `PAGE_TOP_MARGIN` and `HEADER_ROW_HEIGHT_MAX`). Moving it down closes AC8's intent fully. + +## PR #1951 — #1932 cover letter overhaul (CHANGES_REQUIRED, 2026-08-02) + +Blocking: E2E Scenario 24 seeds a source with no `contactAddress`/`reference`, so the cover letter never +enables and every `letterField` locator is empty — see the cover-letter-auto-enable section in +[recurring-patterns.md](recurring-patterns.md). + +Verified clean and worth not re-litigating: `pageGeometry.ts` still has no `merge.ts` import; +`HEADER_ROW_HEIGHT_MAX` is untouched by `coverLetterPdf.ts` (the #1932 drift signal did not fire); +`pageBreak: 'after'` genuinely guarantees the table starts on a fresh page (`merge.ts` L114-119 pushes +letter then overview into one flat array). `ReportContentCoverLetter` is **client-only** — `shared/` and +`server/` never mention `coverLetter`, so a client-only typecheck really was sufficient for the new +required `closing` field. + +Non-blocking, recorded as the intended end state: +- `letterSubject` reuses `SUBHEADER_FONT_SIZE`, which `headerFootprint()` consumes (`pageGeometry.ts:144`) + — false sharing of two semantically unrelated 12pt values. Alias it if it recurs. +- `pageGeometry.ts` now holds its first `PDF_STYLES` entry with no geometry consumer. Trigger for splitting + out `pdfStyles.ts` (direction `pageGeometry <- pdfStyles <- merge`): the second such entry. +- `buildReportContent`'s `options.user` is `user?:` while its sibling `household` is required-nullable — + no design reason; end state is `user: Pick | null`. Deferred: ~47 + option-passing call sites. +- AC 1.5 ("no `dangerouslySetInnerHTML`, no markup parsing") is review-enforced only. `react/no-danger` + is a zero-diff enable — `dangerouslySetInnerHTML` appears nowhere in `client/src`. **Mine to do.** +- AC 1.6's plain-prose guarantee is prompt-level only; the response validator truncates length but does + not strip markup, so a drifting model still puts `**` in a bank PDF. +- `wiki/API-Contract.md` `POST /api/source-reports/generate-content` needs a `letterBody` plain-prose + bullet + a note that `closing`/`dateLine`/`signature` are client-derived and deliberately not in the + response shape. diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index d5577bd61..43de713bc 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -33,11 +33,12 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) (budget/invoice) and [standalone-diary-bugs.md](standalone-diary-bugs.md) / [standalone-photo-stories.md](standalone-photo-stories.md). Growing budget/invoice cluster (20+) → propose new "Budget/Invoice UX Polish" epic next planning cycle. Auto-itemize is standalone (no parent epic): stories #1545-#1547 + bug fixes. +- **#1932 scope ruled by user 2026-08-02**: cover-letter body = **plain text with line breaks**, no markdown/WYSIWYG/new dep; bold+lists out of scope. Premise correction: **pdfmake already honours `\n`** (`TextBreaker.js` L30-34/53-58) so §1 became regression guards + the AI-prompt guard (AC 1.6). ACs 1.1-1.6 amended, 1.4 struck, 1.5 kept as negative constraint, **AC 2.6 added** (`applyOverrides` signature-from-sender recompute), paragraph spacing moved §1→§4.1. Blocked-by = only #1939 (PR #1948). Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1932 user scope ruling". - Budget/invoice batches: #1369-#1373 (2026-04-28), #1389-#1390 (2026-04-29), #1401 (2026-05-10), #1421-#1425 (2026-05-15), #1439-#1441 (2026-05-17), #1553 (2026-05-22) - Auto-itemize: #1545/#1546/#1547 mini-epic (2026-05-21), #1600 (2026-05-26), **#1833 duplicate budget lines on commit retry (2026-07-07)** - Diary: #1426 critical photo data loss (2026-05-15) - Photo: #1723 lightbox picker UX (2026-06-16) -- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question) — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **but #1943** (bug, **Must Have**, Todo, 2026-08-02) — `handleUseCaseChange` never clears `report`/`sourceId`, so budget-overview→claim carries a stale report and can embed **quotation-tier docs in a claim PDF**, reaching #1930 AC2's forbidden outcome by a route AC2 doesn't cover; ruled: clear `sourceId` too — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **#1888 body re-scoped to indicator presentation only at review time** (it was still stale), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **PR #1945 (#1943) review follow-ups filed 2026-08-02**: **#1946** in-flight AI generation survives a use-case change (bug, **Must Have**, Todo — product ruling: widen `guardedUpdate`'s dirty predicate to include `isGeneratingAi`, confirm invalidates via token, cancel lets it finish) and **#1947** `ReportWizardPage` `useReducer` refactor (tech-debt, Should Have, Backlog, blocked-by #1946, filed separately from #1912 on purpose). **M1 was fixed inside PR #1945, not filed** — precedent: a finding that defeats an AC of the story under review belongs in that story's PR regardless of reviewer severity. #1943 AC4 reworded + AC5 enumeration completed (`skippedDocuments`/`aiError` ruled CLEAR, carried as #1946 AC9/AC10); #1933 AC2.1/2.7 corrected (invoice row has no mobile card). See [bank-report-wizard.md](bank-report-wizard.md) §"#1945 review follow-ups" and §"#1933 ACs 2.1/2.7" — the latter records a **recurring AC-writing failure mode** (ACs that misdescribe reality fail correct implementations at UAT). **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". **#1931 PR #1944 APPROVED round 1, 2026-08-02** — all ACs met **except 3.2/3.3, deliberately NOT claimed**: they assert live-model output quality, which a mocked LLM cannot verify. Ruling: **merge is a code gate, Done is an acceptance gate** — PR merges, story stays out of Done until a human reads real EN+DE output with `LLM_*` set; UAT scenarios posted on #1931; failure → reopen #1931, don't file a follow-up. Contrast #1909 AC 4.6: an unverifiable AC **with** a substitute assertion may be waived as a documented deviation; **without** one it goes to UAT. "Mit KI verbessern" accepted for AC 2.3. **#1917 L3 struck** (verified fixed); rest of #1917 open, **`KI` glossary entry still #1917's** — `glossary.json` untouched by #1944. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1931 reviewed". +- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question) — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **but #1943** (bug, **Must Have**, Todo, 2026-08-02) — `handleUseCaseChange` never clears `report`/`sourceId`, so budget-overview→claim carries a stale report and can embed **quotation-tier docs in a claim PDF**, reaching #1930 AC2's forbidden outcome by a route AC2 doesn't cover; ruled: clear `sourceId` too — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **#1888 body re-scoped to indicator presentation only at review time** (it was still stale), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance) — **PR #1951 APPROVED round 1, 2026-08-02**, all 40 ACs met incl. both struck-as-vacuous negative constraints; 3 MUST FIX (German `Mit freundlichen Grüßen` comma, unpinned Closing row, #1925 AC5 editor pin). **#1925 CLOSED as duplicate** (board Wont-Do) — my §6 carried 4 of its 6 ACs, a second instance of the AC-transcription failure mode, **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **PR #1945 (#1943) review follow-ups filed 2026-08-02**: **#1946** in-flight AI generation survives a use-case change (bug, **Must Have**, Todo — product ruling: widen `guardedUpdate`'s dirty predicate to include `isGeneratingAi`, confirm invalidates via token, cancel lets it finish) and **#1947** `ReportWizardPage` `useReducer` refactor (tech-debt, Should Have, Backlog, blocked-by #1946, filed separately from #1912 on purpose). **M1 was fixed inside PR #1945, not filed** — precedent: a finding that defeats an AC of the story under review belongs in that story's PR regardless of reviewer severity. #1943 AC4 reworded + AC5 enumeration completed (`skippedDocuments`/`aiError` ruled CLEAR, carried as #1946 AC9/AC10); #1933 AC2.1/2.7 corrected (invoice row has no mobile card). See [bank-report-wizard.md](bank-report-wizard.md) §"#1945 review follow-ups" and §"#1933 ACs 2.1/2.7" — the latter records a **recurring AC-writing failure mode** (ACs that misdescribe reality fail correct implementations at UAT). **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog), **#1950** guard test recomputing the derived `Ѹ` ceiling (tech-debt, could have, Backlog, blocked-by #1939 — filed 2026-08-02 from PR #1948 §2; architect reframed its own ask from "re-run the 3,919-codepoint sweep" to "a test that recomputes 616 from `USAGE_WIDTH_7COL`/font sizes/`DEFAULT_LINE_HEIGHT`", sweep now an explicit non-goal. Rulings: **comment keeps the rationale, issue owns the guard, neither replaces the other**; bounded-and-quantified earns a tracked owner where unbounded-and-estimated gets documentation only; a derived bound with no test is a comment waiting to go stale — `overviewPdf.test.ts` pins `704`/`546` as literals tied to no geometry constant). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". **#1931 PR #1944 APPROVED round 1, 2026-08-02** — all ACs met **except 3.2/3.3, deliberately NOT claimed**: they assert live-model output quality, which a mocked LLM cannot verify. Ruling: **merge is a code gate, Done is an acceptance gate** — PR merges, story stays out of Done until a human reads real EN+DE output with `LLM_*` set; UAT scenarios posted on #1931; failure → reopen #1931, don't file a follow-up. Contrast #1909 AC 4.6: an unverifiable AC **with** a substitute assertion may be waived as a documented deviation; **without** one it goes to UAT. "Mit KI verbessern" accepted for AC 2.3. **#1917 L3 struck** (verified fixed); rest of #1917 open, **`KI` glossary entry still #1917's** — `glossary.json` untouched by #1944. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1931 reviewed". ## Requirements Coverage diff --git a/.claude/agent-memory/product-owner/bank-report-wizard.md b/.claude/agent-memory/product-owner/bank-report-wizard.md index 2f4fb3d3d..ac9d6d22d 100644 --- a/.claude/agent-memory/product-owner/bank-report-wizard.md +++ b/.claude/agent-memory/product-owner/bank-report-wizard.md @@ -208,6 +208,20 @@ Final state worth knowing: table width is now **exactly 515.28pt, unfalsifiable - **#1940 — continuation rows read as broken** (`enhancement`, Could Have, Backlog). The deferred "Could Have" from the #1929 ruling, now *observed*: `splitIntoPageSafeChunks` has no minimum trailing-chunk floor, so a row can carry a **single stray character** with all other columns blank. Only above the chunk ceilings, i.e. beyond AC12's guaranteed 600-char range; no data loss (I1 holds). - **#1941 — editable override fields have no length limit** (`enhancement`, Could Have, Backlog). Zero `maxLength` in `client/src/components/reports/` or `EditableField/`; `attachmentsNote` is a client-side override that never round-trips, `areaText` is aggregate-unbounded (N × 200). **No longer a correctness risk** — round 4 bounded the renderer at cell scope. Input-side gap only. +### #1950 — guard test for the derived `Ѹ` ceiling (filed 2026-08-02 from PR #1948 round-3 review) + +`tech-debt`, **Could Have**, Backlog, **blocked-by #1939**. Filed off the architect's PR #1948 approval comment ([5160266124](https://github.com/steilerDev/cornerstone/pull/1948#issuecomment-5160266124) §2), which **reframed its own earlier ask**: the deliverable is *not* re-running the 3,919-codepoint sweep, it's a **guard test that recomputes** the derived ceiling from `USAGE_WIDTH_7COL` / `TABLE_BODY_FONT_SIZE` / `TABLE_SMALL_FONT_SIZE` / `DEFAULT_LINE_HEIGHT`. Sweep left out as an explicit **non-goal**, not an optional AC — an "optional" AC isn't binary and makes the issue unfalsifiable. + +The risk being guarded: `MAX_SAFE_USAGE_CHUNK_CHARS` (650) is **34 chars / 3 lines / 33.6pt over** its *derived* `Ѹ` ceiling of 616 (`44 lines × 14 chars`). Accepted on **input reachability** (needs 650 unbroken chars of archaic Church Slavonic Uk in one Usage cell), and because a `Ѹ`-safe value must sit in `[600, 616]`, collapsing AC12's margin over its 600-char floor from 8.3% to ~2.7%. `MAX_SAFE_SMALL_CHUNK_CHARS` (450) is genuinely safe (11.2% under 507). **Not a request to change 650** — the architect is comfortable with the risk. + +Three durable rulings, all written into the issue rather than left implicit: + +- **Comment and issue both, never one instead of the other.** The rationale stays in the code comment (AC 2.1 forbids moving/shortening/replacing it; AC 2.3 pins 650/450/1.04 and every width byte-identical) because *"anyone changing 650 or a column width reads that comment, not an issue tracker. Moving it out recreates the provenance loss that produced #1939."* The issue owns the **guard**; the comment owns the **rationale**. +- **Bounded-quantified vs unbounded-estimated is the line for "does this deserve a tracked owner."** `markerText` is unbounded with an estimated break-even → documentation only (folded into #1939). This is a bounded constant *provably* 34 chars past a derived ceiling → *"a quantified exceedance is a standing accepted risk with a number on it."* I would have collapsed these two; don't. +- **A derived bound with no test is a comment waiting to go stale.** Verified live: `overviewPdf.test.ts` pins `MEASURED_TRUE_CEILING` as re-typed `704`/`546` literals referencing **no geometry constant**, so widening the Usage column leaves them green while the real ceiling moves. Generalise: when a review accepts a *derived* number, ask what fails if its inputs change. + +AC 1.3 fails in **both** directions (growth widens a reviewed risk; shrinkage makes the comment's figure wrong). AC 1.6 keeps the measured 44/39-line budgets as the sole pinned literals, labelled as real-render measurements. The architect's two "informational, do not re-round" cosmetics (`~2.6%`→`~2.7%`, the self-asserted-infallibility sentence) were **already fixed at head `a6871975`** — checked before deciding, nothing folded in. + ### Merge/scope decisions in this triage - **`markerText` (unbounded, ~250-skipped-doc break-even) and `invoiceNumber` (unbroken, capped at 100) were folded into #1939 as a documentation-only AC**, not filed separately. Their value is entirely "the next person reading this file knows the enumeration"— the same category as the comment-scoping work, and a standalone Could Have would never be picked up. AC7 + a scope guard forbid actually implementing a bound for them. @@ -283,3 +297,77 @@ Reworded to "every viewport" with the 44×44px target **unconditional** (strictl - Before writing a viewport- or layout-conditional AC, **check the CSS for an actual breakpoint** — don't infer a responsive variant from the presence of `mobileCard` classes elsewhere in the same file. - For equivalence/"identical to" ACs, **name what is excluded** or sticky user preferences will read as failures. - When an AC is corrected on an open issue, annotate the AC inline with a date + pointer to the correction comment, and say in the comment *why*, so the original premise isn't reintroduced from memory of the old text. + +## #1932 user scope ruling 2026-08-02 — plain text with line breaks, not markdown + +Comments: `issuecomment-5160251632` (decision), `issuecomment-5160258752` (AC change log). + +**User, verbatim: "no full wysiwyg necessary - just a simple text body with line breaks".** So: plain text, no markdown, no rich-text editor, **no new client dependency**. Bold phrases and bulleted lists are **out of scope** even though #1932's Problem section cites them as motivation. The issue's "Needs a design decision before implementation" note is struck — this is no longer an architect/UX call. `ux-designer` still owns letter layout (AC 4.4). + +### Premise correction — the stated defect was largely false + +**pdfmake already honours `\n`.** `node_modules/pdfmake/js/TextBreaker.js` L30–34 and L53–58 treat `\n`/`\r\n` as a *required* line end, so a single text node renders embedded newlines as line breaks. The sender block has depended on this all along (`senderLines.join('\n')` in one node, pinned in `coverLetterPdf.test.ts` L80–90). The body's line-break round trip **already works and is merely unpinned**. + +Consequence: #1932 section 1 collapsed from a feature build to **regression guards + one new requirement**. Worth keeping the guard anyway — the per-token inline-run technique used elsewhere in `reportPdf/` for pdfmake's all-or-nothing `wordBreak` would silently destroy `\n` handling if ever applied to the body. That is a working-but-unpinned behaviour with a plausible silent breaker, which is exactly what a test is for. + +**Lesson: verify a "does not survive rendering" claim against the renderer's source before writing ACs around fixing it.** Cheap (one grep in `node_modules`), and it flipped this section's size. + +### The plain-text ruling *created* one requirement rather than removing it + +**AC 1.6 is now load-bearing.** `server/src/services/budgetExtraction/prompts.ts` L~142 ("Letter body") says nothing about output format. An LLM asked for a business letter readily emits `**emphasis**` and `- bullets`; under plain-text rendering those print as literal asterisks in the PDF a bank reads. Same defect class as the #1916 prompt-input findings — **when a formatting model is simplified, re-check what the LLM prompt assumes about it.** + +### Vacuous ACs: strike with a stated replacement, never delete + +- **1.4** (formatting discoverability) → STRUCK, replaced inline by a negative (must not advertise formatting support) + a concrete ask (`rows={6}` textarea must show multi-paragraph structure without scrolling). +- **1.5** (XSS-safe formatted rendering) → VACUOUS BY CONSTRUCTION, retained as a negative constraint: no `dangerouslySetInnerHTML`, no HTML parsing, no markup interpretation on the body path. Flagged as blocking if violated — it is precisely the AC a future "let's just add marked" PR breaks. +- **1.3** inverted: markup-looking characters must render **literally**; no markdown/rich-text dep added to `client/package.json`. + +Deleting a vacuous AC reads as an oversight and invites re-litigation; striking it with the replacement stated inline does not. + +### Non-formatting defects are genuinely independent — with one intra-issue coupling I had to add + +Sections 2 (signature), 3 (sender), 4 (layout), 5 (reset-X CSS) do **not** depend on the formatting model. Two caveats found on disk: + +- **§2 ↔ §3 are coupled to each other**, in code today: `applyOverrides.ts` L66–68 recomputes `signature` from an overridden sender (`sender.split('\n')[0]`), `types.ts` L44 documents `signature` as `DERIVED`, and `realRender.test.ts` L997 pins the recompute. Making signature first-class means a sender edit must stop overwriting an explicit signature, and that test must be **updated, not deleted**. Filed as new **AC 2.6** — would otherwise have been a review-time surprise. +- **Paragraph *spacing* moved §1 → §4.** With no markup carrying paragraph semantics, whether a blank line stays a full empty line or becomes typographic spacing is a layout call. AC 4.1 amended to own it. +- §5 (reset-X) is pure shared-component CSS and is fully severable — could be split out if #1932 ever needs shrinking. + +### #1925 fold-in unaffected + +All four carried ACs stand verbatim — #1925 is the date **caption** in the letter head plus caption chrome styling, neither of which touches the body model. Added a note above section 6 saying so, so the ruling isn't read as having disturbed the fold-in. + +### #1939 → #1932 handoff (blocked-by is a single edge) + +`#1932 blockedBy` = **only #1939** (verified via GraphQL `blockedBy(first:10)`). PR **#1948** open, no review posted yet. Clear the edge as soon as #1948 merges; nothing else gates #1932. Post-#1939 facts recorded in #1932's Notes for the implementer: + +- **`PDF_STYLES` is defined in `pageGeometry.ts`** and re-exported from `merge.ts` (existing `from './merge.js'` imports keep working) — new letter styles go in `pageGeometry.ts`, which must **never** import from `merge.ts`. +- **`HEADER_ROW_HEIGHT_MAX`** is a ceiling (68pt vs 45.81pt measured), not an estimate. The cover letter ends with a hard `pageBreak: 'after'` so it never shares a page with the table — if #1932's implementation reaches for the constant at all, the layout approach has drifted. +- #1932 should not touch `overviewPdf.ts`; #1937 owns that file's German-header work. + +## #1932 reviewed — PR #1951 APPROVED round 1 (2026-08-02) + +All 40 ACs met on `c17d9d44` + the locally-committed E2E follow-up `d60a98b3`. Provisional local runs: 149/149 (applyOverrides + coverLetterPdf + prompts), 63/63 (realRender), stylelint exit 0. + +### Rulings worth reusing + +- **AC 1.2 — a line-count-plus-spacing proof satisfies a "real render" AC.** `.positions.length` read off a node after `getBlob()`, plus uniform non-zero inter-line gaps, is *sufficient* proof that typed line/blank-line structure survived — no per-line text reconstruction needed — **when the body is a single text node whose `.text` is separately asserted byte-identical**. It genuinely discriminates: a collapsed blank line gives 3 not 4, and a per-token inline-run reflow (the #1929 `wordBreak` technique) destroys `\n` and fails. `._inlines` is the wrong signal — LayoutBuilder drains it to `[]` via `.shift()`; `.positions` is what survives with the right cardinality. +- **"Updated, not deleted" is satisfied by "kept intact and still correct."** `realRender.test.ts:1057` (sender-override recomputes signature) was left untouched and still passes — it now describes the *fallback* branch. My AC's real concern was deletion of the pin. Downgraded the un-reworded title to informational; the adjacent AC 2.6 test is the actual guard against restoring the unconditional recompute. +- **Chrome-vs-content adjacency in different languages is correct, not broken.** `closingLabel` ("Grußformel", interface `t()`) sitting directly above `closing` ("Sincerely,", `reportT`) is #1909's rule applied consistently — same relationship "Betreff" already has with English subject text. Stacking them **vertically** is what makes it read as caption-and-artifact rather than one broken sentence. This is also the whole basis of Option B below. +- **#1925 Option B (restyle caption as chrome) beat Option A (`reportT` the caption)** because Option A would have fixed AC 6.1 by breaking AC 6.2 — every sibling caption in that panel is interface-language, so translating only one makes it the single inconsistent caption. +- **Duplicate closure transfers ownership; it does not require every AC independently green.** Closed #1925 (board Wont-Do) with one AC only partially met, moving that residual to a MUST FIX on the PR where it would actually be acted on. Keeping it open would track the same work twice. + +### My own AC-transcription error — second instance of this failure mode + +**#1925 has SIX ACs; my #1932 §6 carried four** and I wrote "all four carried ACs stand verbatim." Dropped its AC3 (PDF date stays bare/label-free) and AC5 (unit pins both sides). Both had to be checked at review time, and AC5 turned out **partial** — the PDF side is pinned by exact equality, the editor side pins only pre-existing behaviour, not the colon-free caption that *is* the fix. **Rule: when folding issue B into issue A, count B's ACs and map every one explicitly — a dropped AC surfaces as an unverified claim at close time.** Companion to the #1933 AC 2.1/2.7 entry (ACs that misdescribe reality); this is ACs that silently go missing. + +### Findings filed as MUST FIX on #1951 (non-blocking) + +1. **German `closing` carries a comma** — `"Mit freundlichen Grüßen,"`. DIN 5008: **no comma** after the Grußformel; English `"Sincerely,"` correctly takes one. One-char `de`-only fix. Recurring class: translator mirroring English punctuation into a locale with a different convention. +2. **New Closing read-only row has zero unit coverage** — deleting the JSX leaves every test green, so AC 4.2's preview-mirroring is unpinned. +3. **#1925 AC5 editor-side pin missing** — nothing asserts the date caption renders colon-free. + +Also flagged: stray untracked `client/src/lib/reportPdf/__scratch_ux1951.test.ts` (ux-designer's scratch render test, claimed deleted, wasn't; not gitignored → `git add -A` would sweep it in), and re-run `ci-wait.sh` after pushing `d60a98b3` since the green on `c17d9d44` predates the E2E suite change. + +### AC 7.3 ordering ruled correct + +E2E belongs in the PR that closes the story, not a follow-up issue — deferring lets the story merge with a documented AC unmet. Scenarios 21–24 pair viewport with theme (desktop+light, mobile+dark) rather than a full 2×2; accepted as the existing convention in that spec file. **#1932 stays out of Done until UAT** — the user rejected the prior output by looking at a generated PDF, so acceptance is a human reading a real exported cover letter in EN and DE (same merge-gate-vs-Done-gate split as #1931). diff --git a/.claude/agent-memory/translator/MEMORY.md b/.claude/agent-memory/translator/MEMORY.md index 2f4623b28..5c74d3d50 100644 --- a/.claude/agent-memory/translator/MEMORY.md +++ b/.claude/agent-memory/translator/MEMORY.md @@ -100,3 +100,14 @@ New `sourceReports.expand.*` (chevron-expand sub-tables for budget lines + depos ## Audit Protocol History - [Audit pitfalls](audit-pitfalls.md) — incident history behind the mandatory 4-step full-coverage audit protocol: a parity-only audit missed 13 code-referenced keys (Area UI raw-key bug); loose substring greps flagged 52 false positives + +## Cover Letter Signature Block Keys (Issue #1932, 2026-08-02) + +`sourceReports.editable.signatureLabel` → "Unterschrift"; `sourceReports.coverLetter.closing` → "Mit freundlichen Grüßen,"; `sourceReports.editable.closingLabel` → "Grußformel". Confirmed: neither "signature" nor "closing salutation" belongs in the glossary (grep across `glossary.json` for signature/closing/Gruß terms found nothing, and these are generic letter-writing vocabulary, not Cornerstone domain terms) — did not add. + +**Chrome-vs-content split, worked example**: `coverLetter.closing` and `editable.closingLabel` sit right next to each other in the UI but are translated by two different rules, and getting this backwards is the easy mistake: +- `coverLetter.closing` = the letter's actual salutation text, rendered via `reportT()` into the PDF in the *report's* language → full letter-register translation ("Mit freundlichen Grüßen,"). +- `editable.closingLabel` = the read-only caption *above* that value in the step-5 editor, rendered via `t()` in the *interface's* language → UI chrome, translated like its sibling captions (`senderLabel`→"Absender", `subjectLabel`→"Betreff"), using the precise German business-letter term for "closing salutation" ("Grußformel", paired with "Anrede" for the opening salutation) rather than a literal "Schließen"/"Closing"-style calque. +- Rule of thumb: check the **call site**, not the JSON namespace path — both keys live under `sourceReports.*` but `t()` vs `reportT()` at the usage site is what actually decides chrome vs. content, per the established artifact-content-vs-edit-affordance convention from #1909/#1924. + +**Concurrent-worktree gotcha**: at spec time `closingLabel` didn't exist in `en/budget.json` yet (frontend-developer added it mid-session, confirmed via `git diff client/src/i18n/en/budget.json` showing the line appear between my two check-ins). When a spec says "check whether the en key already landed," re-check immediately before finalizing rather than trusting an earlier read — in a shared worktree the answer can flip within the same task. diff --git a/.claude/agent-memory/ux-designer/MEMORY.md b/.claude/agent-memory/ux-designer/MEMORY.md index 2ed7884fa..b7a3a97b1 100644 --- a/.claude/agent-memory/ux-designer/MEMORY.md +++ b/.claude/agent-memory/ux-designer/MEMORY.md @@ -29,4 +29,7 @@ - `formatDateForAria`-style aria-labels that interpolate localized weekday/month into a hardcoded English sentence order (`"{weekday}, {month} {day}, {year}"`) render grammatically odd in German (`"Dienstag, Februar 24, 2026"` vs correct `"Dienstag, 24. Februar 2026"`) — this is an accepted, deferred, non-blocking gap across the app, not a new bug to flag each time. - When a PR claims a Badge/shared-component consolidation is "zero visual delta," build a before/after table (padding/font-size/weight) per migrated component — the shared base class is usually extracted from only a subset of sources, so other migrated components can silently inherit a different size than they had. See PR #1848 in [pr-review-findings.md](pr-review-findings.md). - For `client/src/lib/reportPdf/` PRs (pdfmake document generation, no CSS/components/dark-mode in scope): don't judge column widths/wrapping/page-break behavior from font-metric arithmetic — render the real pipeline to a throwaway `/tmp/*.pdf` via a scratch Jest test and rasterize with `pdftoppm` for visual inspection, then delete the scratch file. See [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md) for the exact technique and a confirmed `dontBreakRows` row-split bug found this way. +- pdfmake: an empty-string text node (`{ text: '' }`) reserves the SAME line-height as a non-empty node — never worry it'll collapse to zero, no nbsp workaround needed for an "always reserve this line's space" AC. Also: at 11pt/1.4 line-height, a real rendered line is ~18pt, not the naive `fontSize*lineHeight=15.4pt` — render-and-measure, don't hand-compute. See [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md), Issue #1932. +- When an AC complains a UI panel's caption/label "mixes languages" with its value, check whether the real bug is inline concatenation (`"Label: value"` on one line) before translating the caption into the value's language — if sibling captions in the same panel are all interface-language chrome, translating just one caption breaks panel consistency instead of fixing it. Fix by visually separating caption from value (label-above-value, matching the panel's existing label recipe) instead. See Issue #1932 in [feature-spec-history.md](feature-spec-history.md). +- **Playwright browser download is blocked in this sandbox** (`cdn.playwright.dev`/`playwright.download.prss.microsoft.com` both 403 by default-deny network policy), and there is no installable system Chromium (`chromium-browser` here is a snap-transitional stub, `apt-get install chromium` no-ops). For CSS-only component review (no pdfmake involved) when no pixel render is available: build a static HTML harness using the *actual* `.module.css` file contents verbatim (literal, unhashed class names — safe because you write matching literal-class markup yourself, sidestepping the webpack-hashing problem) plus real `tokens.css`, with hand-written markup mirroring the exact JSX tree, then verify by direct CSS-rule/box-model comparison (e.g. "is this property byte-for-byte identical to that sibling's rule", "does `align-self` override the flex container's default stretch") rather than a screenshot. This is real, defensible verification for deterministic box-model questions (button stretch/sizing) but not a substitute for an actual render on subjective spacing/contrast calls — say so explicitly in the review rather than presenting it as equivalent. See PR #1951 in [pr-review-findings.md](pr-review-findings.md). - **`:global(.foo)` in a `.module.css` file only matches a DOM element whose class is the literal unhashed string `"foo"`.** If a component applies another module's class via plain `className={otherModuleStyles.foo}` (not `composes:`), that class resolves to a hashed string (`foo_a1b2c` per `webpack.config.cjs`'s `localIdentName`) in real builds — `:global(.foo)` selectors written to "layer styles on top of" that class never match, silently killing the entire rule block. This is invisible in Jest: `jest.config.ts` maps `.module.css` → `identity-obj-proxy`, which resolves classes to their literal key name, so `:global(.foo)` _does_ match in tests. The correct cross-module extension technique is `composes: foo from '../other/Other.module.css';` inside a locally-scoped class, then apply _that_ local class — this actually merges the real hashed class list onto the element. Check for this specific pattern (`:global(...)` selectors targeting another module's class name, applied via plain className not `composes`) whenever a new component's CSS "doesn't seem to apply" or a spec's described states (hover/focus/at-rest tint/indicator dots) go missing — see PR #1909 in [pr-review-findings.md](pr-review-findings.md). diff --git a/.claude/agent-memory/ux-designer/feature-spec-history.md b/.claude/agent-memory/ux-designer/feature-spec-history.md index 7fbaec1a2..0c15c1f80 100644 --- a/.claude/agent-memory/ux-designer/feature-spec-history.md +++ b/.claude/agent-memory/ux-designer/feature-spec-history.md @@ -157,3 +157,12 @@ Part of the Bank Report Wizard Refinement Round 2 mini-epic (follows #1898). Spe - **Focus ring decision**: deliberately did NOT add a custom `box-shadow: var(--shadow-focus)` ring to the new radio group — kept the native browser `:focus-visible` outline to stay consistent with the sibling `.optionCheckbox` controls in the very same step, which also rely on the native outline (no custom ring exists there today). Don't treat "every new control must get a custom focus ring" as an absolute rule — matching the immediate visual siblings can be the more consistent choice. - **Found a real responsive risk while auditing `WizardStepper` for a 5th step**: the desktop `