diff --git a/.claude/agent-memory/docs-writer/MEMORY.md b/.claude/agent-memory/docs-writer/MEMORY.md index 016524018..ecb06b9f7 100644 --- a/.claude/agent-memory/docs-writer/MEMORY.md +++ b/.claude/agent-memory/docs-writer/MEMORY.md @@ -30,7 +30,7 @@ - `getting-started/` -- index, docker-setup, first-login, configuration - `guides/work-items/` -- index, creating-work-items, tags, notes-and-subtasks, dependencies, keyboard-shortcuts - `guides/users/` -- index, oidc-setup, admin-panel -- `guides/budget/` -- index, categories, financing-sources, work-item-budgets, vendors-and-invoices, subsidies, budget-overview +- `guides/budget/` -- index, categories, financing-sources, work-item-budgets, vendors-and-invoices, subsidies, budget-overview, bank-reports (added PR #2041, see [Bank Report Wizard docs](bank-report-wizard-docs.md)) - `guides/timeline/` -- index, gantt-chart, milestones, calendar-view - `guides/documents/` -- index, setup, browsing-documents, linking-documents - `guides/household-items/` -- index, creating-editing-items, budget-and-invoices, work-item-linking, delivery-and-dependencies @@ -103,6 +103,20 @@ Three user-facing changes documented (no new pages, no sidebar changes): Pre-existing lint baseline in worktree: ~8 eslint ERRORS in production/test .ts across client/server/e2e (photoService OrientationSummary unused, usePaperless import() type, etc.) -- NOT introduced by docs changes (docs edits are markdown-only; eslint doesn't lint .md). Likely a local `npm install --ignore-scripts` artifact since beta CI requires lint green. Do NOT touch those files as docs-writer. +## Release: PR #2041 (Bank Report Wizard docs gap, subsidies drift fix, .env.example drift fix) + +Full env-var scan (`grep` for `getValue('...')` in `server/src/`) confirmed 3 vars missing/wrong in `.env.example`: `AUTH_RATE_LIMIT_MAX` and `AUTH_RATE_LIMIT_WINDOW` (new, added commented-out under Server section near `TRUST_PROXY`/`EXTERNAL_URL`) and `VAT_RATE` (pre-existing gap, added commented-out under Localization near `CURRENCY`). `CLAUDE.md`'s env-var table was already current for all three (added in an earlier commit within the same 40-commit range) -- always diff `.env.example` against `server/src/plugins/config.ts` directly rather than assuming the two docs surfaces drift together. + +New page `guides/budget/bank-reports.md` closes a total docs gap for the Bank Report Wizard feature -- see [Bank Report Wizard docs](bank-report-wizard-docs.md) for what it covers and why the gap existed. Same pass also rewrote stale portions of `subsidies.md` (multi-category + "No Category" + real 5-status enum) -- same memory file has the detail. + ## Build Note (still true) `npm run docs:build` fails in worktrees with webpack `ProgressPlugin` ValidationError (node_modules corruption, NOT content). Build reaches the webpack bundling stage, so MDX/content/link loading succeeded. Validate internal links/anchors statically with grep instead; CI does the real build. + +Update: in the `batch-develop-1973` worktree (2026-08), `npm run docs:build` completed successfully end-to-end (only the expected pre-existing screenshot-image warnings) -- the corruption is worktree-instance-specific, not universal. Still try the build first; fall back to static grep validation only if it actually fails. + +**CI is green-by-vacuity on docs-only PRs.** `Detect Changes` routes a docs-only diff so every real CI job (including the `onBrokenAnchors: 'throw'` docs build) skips, and the required wrapper gate still reports success. `Quality Gates` passing on a docs-only PR is NOT evidence the anchors/links resolve -- the real `docusaurus build` with strict broken-link/anchor checking only runs at release time. Always run `npm run docs:build` locally yourself after any edit touching links or headings; don't rely on CI for this on docs-only PRs. + +## configuration.md sections + +`docs/src/getting-started/configuration.md` has an `## Authentication Rate Limiting` section (added for #1990) documenting `AUTH_RATE_LIMIT_MAX`/`AUTH_RATE_LIMIT_WINDOW` (defaults 20 / `15 minutes`, `ms`-format window, startup-failure-on-invalid-value caveat, and household-NAT-vs-internet-exposed tuning guidance), cross-linked with the `## Reverse Proxy` section's `TRUST_PROXY` explanation (bucket-by-proxy-IP vs bucket-by-client-IP). The setup-endpoint's fixed 5/15min limit is mentioned as non-configurable, per issue Notes. This is the only general env-var reference table on the docs site -- `guides/users/oidc-setup.md` has a small OIDC-scoped var table but it's not a second general reference. diff --git a/.claude/agent-memory/docs-writer/bank-report-wizard-docs.md b/.claude/agent-memory/docs-writer/bank-report-wizard-docs.md new file mode 100644 index 000000000..dbc4ff05b --- /dev/null +++ b/.claude/agent-memory/docs-writer/bank-report-wizard-docs.md @@ -0,0 +1,40 @@ +--- +name: bank-report-wizard-docs +description: Bank Report Wizard had zero docs-site coverage until PR #2041 (2026-08-06); new guide at guides/budget/bank-reports.md, plus the subsidies.md drift that was fixed in the same pass +metadata: + type: project +--- + +# Bank Report Wizard docs gap (closed 2026-08-06, PR #2041) + +**Why this mattered:** EPIC-07 (Reporting and Export) had been checked off in the roadmap for a long +time, and the feature (`client/src/pages/ReportWizardPage`, route `/budget/reports`) had grown into a +large, actively-developed area (see `product-owner`'s `bank-report-wizard.md` memory for the full +mini-epic history), but `docs/src/` and `docs/sidebars.js` had **no page for it at all** -- not even a +stub. A release task that assumed "extend the existing reports docs" surfaced the gap. + +**How to apply:** The gap is now closed -- `docs/src/guides/budget/bank-reports.md` (sidebar position 9, +registered in `docs/sidebars.js` under the Budget category, cross-linked from `guides/budget/index.md`). +It documents the wizard as a single comprehensive page (index-style, no sub-pages, matching the +`guides/backup/index.md` pattern) covering: the 3 report types (Budget Overview / Claim / Proof of +Funds) and their invoice-status eligibility, the 5 wizard steps (Report Type, Budget Source, Select +Invoices, Settings, Preview & Export), column visibility toggles, AI-assisted generation +("Enhance with AI", gated on `llmEnabled`), marking invoices claimed, and long-content/multi-page PDF +handling. **Before extending this page**, re-derive current UI/copy from +`client/src/i18n/en/budget.json` (`sourceReports` key) and `ReportWizardPage.tsx` rather than trusting +this page alone to stay current -- the feature has a long history of fast iteration (see +`[[release-notes-drift]]` if that file exists, or the product-owner memory directly). + +## Related fix in the same pass: subsidies.md was stale, not just missing "No Category" + +`docs/src/guides/budget/subsidies.md` described a **single** "Budget Category" field and a **4-status** +lifecycle (Pending/Approved/Rejected/Disbursed) that no longer matched the shipped `SubsidyProgram` +type (`shared/src/types/subsidyProgram.ts`): categories are actually **multi-select** +(`applicableCategories: BudgetCategory[]`, empty = universal) with an independent `includesNoCategoryItems` +("No Category") checkbox, and the real status enum is +`eligible | applied | approved | received | rejected` (only `approved`/`received` count toward budget +math). This predated the "No Category" release task -- rewrote the whole Creating/Statuses/How-it-affects +sections rather than just appending the new checkbox, since the old text would have stayed actively +wrong. **Lesson: when a task says "add feature X to this doc", verify the doc's existing claims against +the current type/schema before touching it — drift compounds silently on release cycles that only ever +append.** diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index c2037cf37..d8696067a 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -31,6 +31,8 @@ - [issue-1959-inline-meta-and-labels.md](issue-1959-inline-meta-and-labels.md) — #1959 reversed #1923's †/‡ footnotes → inline `(partial)`/`(less deposit)` and merged area+attachments into one `.usageMetaText` line; POM renames, rewritten scenarios 2/17/18/20 + AI 8, attachment-tier facts, untested column toggles. - [claim-deposit-scope-1922.md](claim-deposit-scope-1922.md) — PR #1922 invoice/deposit claim-scope split: `handleMarkClaimed`'s two-array submit, server-truth success-banner counts, the three "deposit surfaces the invoice" shapes, `claimNothingClaimable` guard. +- [issue-1973-column-visibility-pdf.md](issue-1973-column-visibility-pdf.md) — column-visibility toggles wired to the PDF (supersedes #1966): reducer reset facts (use-case change resets `hiddenColumns`, `SET_ATTACH_DOCUMENTS` doesn't), Scenarios 28-33, size-diff PDF-consequence proof pattern, `goBack()` viewport-independent multi-step walk, new `columnToggleGroup`/`usageHiddenAttachmentsWarning` POM locators. +- [issue-1911-splitkind-e2e.md](issue-1911-splitkind-e2e.md) — #1911 `splitKind` field: incoming spec said "no E2E changes needed", was wrong — fixed Scenario 18's mis-seeded fixture AND found Scenario 17 independently affected (zero-contribution-line case) by cross-referencing unit-test ACs; new AC-3.2 regression-guard sibling test; POM docblock corrections. ## Open follow-ups to flag to orchestrator diff --git a/.claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md b/.claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md new file mode 100644 index 000000000..68375972f --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md @@ -0,0 +1,126 @@ +--- +name: issue-1911-splitkind-e2e +description: Issue #1911 (SourceReportInvoice.splitKind) E2E fallout in reportWizardEditableContent.spec.ts — dev-team-lead's "no E2E changes needed" spec conclusion was wrong; two scenarios (17 AND 18) were affected, found by cross-referencing server/client unit-test ACs, not by running the browser suite. +metadata: + type: project +--- + +## What #1911 actually changed (semantics, not just a field addition) + +`server/src/services/sourceReportService.ts` step f: `isSplit`/`isDepositReduced` used to be +derived client-side from `invoice.isSplit(raw) && budgetLines.length>0` / +`invoice.isSplit(raw) && deposits.length>0 && !ownTagged`. Both gates were unsound (claim reports +drop zero-contribution budget lines; a foreign-tagged deposit never appears in `deposits[]` at +all). #1911 replaced them with a purely server-derived `splitKind: 'lines'|'deposits'|'both'|null` +(SQL computes `has_foreign_line_source`/`has_foreign_deposit_source` — "this arm contains a source +≠ the reported one", untagged deposits excluded from the deposit arm entirely since the query +filters `budget_source_id IS NOT NULL`). `buildReportContent.ts`: `row.isSplit ⟺ splitKind ∈ +{'lines','both'}`, `row.isDepositReduced ⟺ splitKind ∈ {'deposits','both'}`. `row.isDeposit` +(constituted-deposit badge) trigger is UNCHANGED and now independent of the other two — all three +can co-occur on one row (old code's implicit either/or is gone). + +## Two E2E scenarios were affected, not one — the incoming spec only caught the second + +`e2e/tests/budget/reportWizardEditableContent.spec.ts`: + +- **Scenario 18** ("split + deposit-reduced labels"): `invoice3`'s deposit was seeded + `budgetSourceId: null` (untagged) with a comment claiming that produces `isDepositReduced: true` + — true under the OLD buggy code, false under the fix (this was the literal bug #1911 exists to + fix). Retagged to `otherSourceId` (a source ≠ the reported one) to get the genuine + `splitKind: 'both'` shape. +- **Scenario 17** ("constituted-deposit row … carries NO marker/label"): NOT flagged by the + incoming spec at all, found by grepping every `createDepositViaApi(...budgetSourceId...)` call + site per the task's own hint ("tagged to the reported source" is one of the two shapes to + check). Its invoice has budget lines ENTIRELY on a different source (A) and a deposit tagged to + the reported source (B) itself. Under the OLD code: `isSplit(row)` was gated by + `budgetLines.length>0` for B, which is 0 → false → no `(partial)`. Under NEW code: + `has_foreign_line_source` is true (A's line is foreign to B) regardless of whether B itself has + ANY line contribution → `splitKind: 'lines'` → `isSplit(row)` **true**. This is the "AC 3.1 + zero-contribution-line regression case" explicitly called out in + `client/src/lib/reportContent/buildReportContent.test.ts` (search that phrase) — a unit test + already asserted this new behavior; the E2E suite just hadn't been told. Net: the row now shows + BOTH the "Deposit" badge AND `(partial)`, plus one footnote entry (previously zero). + +**How I found it without a live browser**: cross-referenced `server/src/services/ +sourceReportService.test.ts` (search `Story #1891 regression: invoice with lines only for source B ++ a deposit tagged to source A → isSplit true in both A and B reports`, line ~346) against the +E2E fixture shape — that unit test's `resultB.invoices[0].isSplit` assertion is `true`, which is +the DB-level raw `isSplit` (unchanged by #1911) that Scenario 17's OLD stale comment claimed was +`false`. Then confirmed via `buildReportContent.test.ts`'s "AC 3.1 (regression, #1898/claim +zero-contribution-line drop)" test that the row-level flag inherits this. Static/unit-test +cross-referencing caught a bug the incoming E2E spec missed — worth doing whenever a spec claims +"no E2E changes needed" for a semantic (not just additive) server change. + +## Regression-guard test added + +A new sibling `test()` inside Scenario 18's `describe` block (not folded into the existing test, +so `footnoteItems` can assert count 1 cleanly — the existing test's count is 2 for unrelated +reasons): a split invoice (lines on two sources) + an UNTAGGED deposit must show `(partial)` but +never `(less deposit)`, with exactly one footnote entry. This is literally the fixture shape +Scenario 18's `invoice3` used to have (before being retagged) — without a standalone guard, the +AC 3.2 over-inclusive bug could regress silently since every other test in the file now uses +either no deposit, an own-tagged deposit, or a genuinely-foreign-tagged one. + +## POM docblock corrections + +`e2e/pages/ReportWizardPage.ts` had TWO now-false invariants baked into JSDoc comments (found by +reading the file, not just the test): `depositBadge()`'s doc claimed a constituted-deposit row +"carries no inline note of its own" and `inlineNote()`'s doc claimed such a row "gets NEITHER — +it gets the inline depositBadge instead". Both corrected with an "Issue #1911" note; also added a +dedicated "Issue #1911" paragraph to the class docstring (same location/style as the existing +"Issue #1965" paragraph) so a future reader hits the corrected model before writing a new fixture +against stale assumptions. + +## Reusable lesson + +When a dev-team-lead spec says a server change is "purely additive, no E2E changes needed" for a +field that DRIVES existing conditional rendering, don't take it on faith — grep every fixture that +feeds the changed derivation (here: every `createDepositViaApi` call with `budgetSourceId` null or +equal to the reported/requested source) and check its assertions against the NEW derivation logic, +not just the ONE scenario the spec happened to mention. The unit test suite (already written and +green) is a fast, authoritative way to derive "what SHOULD this fixture shape now assert" without +needing a live browser. + +## PR #2015 review round: two red shards, both self-inflicted, neither a production defect + +`product-architect` and `product-owner` independently traced both E2E failures to stale +assertions I wrote, not to product behavior: + +- **Badge-vs-note DOM order isn't a fact worth asserting.** Scenario 17's rewrite added + `toContainText('€150.00 (partial)')`, but the `depositBadge` renders BETWEEN the amount and the + note (`ReportContentEditor.tsx`: value → badge → split note → deposit-reduced note, in that + literal JSX order), so the DOM text is `€150.00Deposit (partial)` and the substring can never + match. Fix was to **delete the assertion**, not rewrite it to encode the ordering — two sibling + assertions already pin the same fact against the `inlineNote` locator directly (count 1, text + `(partial)`), and hardcoding badge-before-note relative order is exactly the kind of brittleness + this area (already reshuffled twice: #1959, #1911) keeps punishing. +- **Get the money math from the actual formula, not intuition.** Scenario 18's invoice3 retag (see + above) changed the deposit from untagged to tagged-to-`otherSourceId`, but I left the OLD + expected amount (`€75.00`) on the row assertion below it. The correct value is **€56.25** — + `depositAggregateUtils.ts`'s `splitByDepositsExcludingTagged`: `residualFraction` ALWAYS + subtracts every deposit (tagged or not) from the invoice total in the denominator + ((200−50)/200=0.75), but `depositFractions` (which gets ADDED back per line) only includes + UNTAGGED deposits — a tagged one is filtered out entirely (it's handled by Rail B, on a + different source's row). So `75 × 0.75 = 56.25`, full stop, no returned fraction. Contrast the + sibling negative-control test's untagged deposit (60/90 split, 25 untagged deposit, invoice + 150): residual `(150−25)/150=0.8333` PLUS the returned `depositFraction` `25/150=0.1667` sum to + exactly 1.0 (true whenever there's exactly one deposit, tagged-or-not doesn't matter to the + sum-to-1 property when it's the ONLY deposit and it's untagged) → `60 × 1.0 = 60`, i.e. the + original `€60.00` assertion was already correct and needed no change, only a comment. +- **The arithmetic proves the fix in both directions** — worth stating explicitly in test comments + next to both numbers, not just implied: foreign-tagged deposit → allocation genuinely drops + (75→56.25), so "claimed separately" is true; untagged deposit → residual + returned fraction net + to the FULL original amount (no drop at all), so the pre-#1911 "claimed separately" label on + that shape was literally false to a bank recipient. This is why the AC 3.2 negative-control test + exists, and it's a stronger justification than "the bug fired on the wrong condition" — worth + reaching for in future PR descriptions/comments on this area, not just re-deriving silently. +- **Lesson**: when a fixture retag changes the underlying formula's inputs, don't assume "keep + every other assertion the same, just add the new one" — re-derive EVERY downstream numeric + assertion from the actual utility function (not from a coordinator's or reviewer's restated + number without checking it against the source), and put the derivation in a comment so a future + reader (or reviewer) can tell "the fixture changed and the arithmetic followed" apart from + "the assertion was made convenient." I re-verified the €56.25 figure independently against + `depositAggregateUtils.ts` rather than taking two reviewers' restated arithmetic on faith — it + checked out, but the habit is the point: derive, don't just relay. + +See [[story-1879-report-wizard]], [[issue-1959-inline-meta-and-labels]]. 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 5375285b2..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 @@ -1,6 +1,6 @@ --- name: issue-1959-inline-meta-and-labels -description: PR #1959 reversed two earlier report-table designs (†/‡ shared footnotes from #1923, distinct area sub-line) into inline labels + one combined meta line; which E2E locators/scenarios had to be rewritten and how each new assertion was made non-vacuous. +description: PR #1959 reversed two earlier report-table designs (†/‡ shared footnotes from #1923, distinct area sub-line) into inline labels + one combined meta line; which E2E locators/scenarios had to be rewritten and how each new assertion was made non-vacuous. Issue #1965 then reinstated legend footnotes for split/depositReduced rows. metadata: type: project --- @@ -9,11 +9,34 @@ PR #1959 ("improve report PDF UX") deliberately **superseded** two designs earli asked for, in `ReportContentEditor.tsx` / `buildReportContent.ts`: 1. `†`/`‡` markers + the shared footnote list (Story #1923 AC1) → grey inline `` in the **Allocated Amount cell**: `(partial)` / `(less deposit)` +class*="inlineNote">` in the **Allocated Amount cell**: `(partial)` / `(less deposit)` (de `(Teilbetrag)` / `(abzgl. Abschlag)`). `ReportContentRow.allocatedMarkers` → `isSplit` / - `isDepositReduced` booleans. `buildReportContent` now pushes **zero** footnotes, so - `.footnotes` has no producer at all — `footnotesBlock`/`footnoteItems` survive in the POM as + `isDepositReduced` booleans. `buildReportContent` pushed **zero** footnotes after #1959, so + `.footnotes` had no producer — `footnotesBlock`/`footnoteItems` survived in the POM as **negative-only** guards. + + **Issue #1965 update (fix/report-pdf-ux-improvements branch):** `buildReportContent.ts` now + pushes ONE deduplicated legend entry per active flag: `splitInvoiceIds.size > 0` → one `'split'` + footnote ("Amount shown reflects only the portion allocated to this source."), + `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. + + **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(' · ')` (U+00B7 middle dot, spaces on both sides). The `attachmentsNote` `EditableField` is gone @@ -27,9 +50,9 @@ the PR body the spec, so the tests were rewritten, not the code. `mobileUsageAreaText`→`mobileUsageMetaText`, plus new `inlineNote()`/`mobileInlineNote()`; `attachmentsNoteField()` deleted. Rewritten scenarios: editableContent 2, 17, 18, 20 and aiGeneration 8. Every "old design is gone" negative is paired with a positive so it cannot pass -against a mis-seeded page (e.g. Scenario 18 asserts `(partial)` present *and* `†`/`‡` absent -*and* the long-form footnote sentence absent from `main`; Scenario 20 asserts the attachments -note text IS rendered *and* the row has one textbox). +against a mis-seeded page (e.g. Scenario 18 asserts `(partial)` present _and_ `†`/`‡` absent +_and_ the long-form footnote sentence absent from `main`; Scenario 20 asserts the attachments +note text IS rendered _and_ the row has one textbox). Facts worth reusing: diff --git a/.claude/agent-memory/e2e-test-engineer/issue-1973-column-visibility-pdf.md b/.claude/agent-memory/e2e-test-engineer/issue-1973-column-visibility-pdf.md new file mode 100644 index 000000000..0235d92f3 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/issue-1973-column-visibility-pdf.md @@ -0,0 +1,121 @@ +--- +name: issue-1973-column-visibility-pdf +description: Issue #1973 E2E coverage — column-visibility toggles wired through to the generated PDF, superseding #1966's DOM-only coverage. New POM locators, 6 rewritten/added scenarios, viewport-scope reasoning. +metadata: + type: project +--- + +## What changed in production (frontend-developer, this issue) + +`hiddenColumns` moved from `ReportContentEditor` local `useState` into `ReportWizardPage`'s +wizard-reducer `ContentTier` (`wizardReducer.ts`), threaded through to +`generatePdfFromContent`/`overviewPdf.ts`. Single source of truth for the base column set per use +case and the locked column lives in `client/src/lib/reportContent/columns.ts` +(`isColumnLocked`/`visibleReportColumns`/`REQUIRED_REPORT_COLUMN = 'allocatedAmount'`), consumed +by both the editor UI and the PDF geometry engine (AC 2.1). + +Key reducer facts (`client/src/pages/ReportWizardPage/wizardReducer.ts`): +- `SELECT_USE_CASE` spreads `freshContentTier()` → `hiddenColumns` resets to `new Set()` on every + use-case change (AC 5.1). This is the SAME mechanism that already clears `overrides`/`aiContent`. +- `DISCARD_EDITS` explicitly PRESERVES `hiddenColumns` (`hiddenColumns: state.hiddenColumns` + overridden back in after the `freshContentTier()` spread) — column visibility is a presentation + choice, not a "content edit" that the discard-confirm modal guards. +- `hiddenColumns` is never persisted (no preference endpoint involved at all) — AC 5.2/5.3 fall + out for free: a full page reload wipes the in-memory reducer state entirely, same as every other + wizard-run-scoped field. +- `SET_ATTACH_DOCUMENTS` only touches `SettingsTier` — going back to Settings and toggling + attachDocuments does NOT reset `hiddenColumns`, so a single seeded fixture can walk through all + 4 combinations of the AC 6.2 warning-banner matrix without re-navigating from scratch. + +Both the desktop `` and the mobile `.mobileCardList` gate on the exact same `show(col)` +derivation in `ReportContentEditor.tsx` — confirmed by grep, no `@media` rule anywhere touches +`.columnToggles`/`.columnToggleGroup`. The ux-designer's finding that the toggle group itself has +**no responsive hiding** is correct and verified independently here. + +## E2E work done + +Rewrote the pre-existing "Scenario 24, #1966" describe block in +`e2e/tests/budget/reportWizardEditableContent.spec.ts` (renamed **Scenario 28** — the file already +had an unrelated, unrenumbered "Scenario 24" collision for the signature-reset test at the OLD +line ~2236; both used the literal string "Scenario 24" and I did not touch the signature one) and +added 5 new scenarios (29-33), picking fresh numbers past the file's existing max (27, the +lang-attribute scenarios) rather than reusing/renumbering anything else in the file. + +- **Scenario 28** (desktop only, by documented exclusion): DOM-level baseline carried forward + verbatim per the spec's explicit instruction — checkbox presence/count, `
`+`` removal + via `getByRole('columnheader'/'cell')` (requires real table semantics, absent from the mobile + card list), no-PATCH assertion (AC 7.1, AC 5.2). +- **Scenario 29** (desktop only — download mechanism isn't viewport-dependent): THE scenario that + closes the #1966 gap (AC 1.2/7.2). Size-diff, not byte-parsing: seed Usage with ~40 sentences of + real text, download baseline (all columns), hide Usage, download again, assert + `hiddenSize < baselineSize`. Deliberately NOT a bare `>1000 bytes` check (that's Scenario 8's + weaker shape, which would pass identically whether or not the toggle reached generation) — a + code comment at the assertion says so explicitly per the spec's instruction, to survive a future + "simplify this" pass. +- **Scenario 30** (`@responsive`, all 3 viewports): AC 2.2 locked checkbox — `toBeDisabled()`, + non-empty resolvable `aria-describedby` target, and `uncheck({force: true})` (bypasses + Playwright's actionability check, which would otherwise refuse to interact with a disabled + element outright) still leaves it checked afterward — a genuine behavioral proof, not a + restatement of `toBeDisabled()`. +- **Scenario 31** (`@responsive`, all 3 viewports): AC 6.2 warning banner, all 4 combinations of + (Usage hidden/visible) × (attachDocuments on/off) in ONE test/ONE fixture, using the + `SET_ATTACH_DOCUMENTS`-doesn't-reset-`hiddenColumns` fact above to avoid re-seeding. +- **Scenario 32** (`@responsive`, all 3 viewports): AC 5.1 use-case reset — hide a column on + `claim` (6 checkboxes), walk back to step 1 via 4× `goBack()` (viewport-independent — see + below), switch to `budget-overview`, walk forward, assert 7 checkboxes ALL checked (not just + "no longer hidden" — proves the new use case's own base-set SIZE, not just a stale 6). +- **Scenario 33** (`@responsive`, all 3 viewports): AC 5.3 reload reset — reach step 5 via the + `?sourceId=` deep-link pattern (`wizard.goto(sourceId)`, mirroring `reportWizard.spec.ts` + Scenario 7), hide a column, `page.reload()` (URL still carries the query param), re-walk the + deep-link flow, assert the full 6-checkbox base set restored. + +## Viewport-scope decision (AC 7.3) + +Scenario 28 stays desktop-only WITH a documented reason (ARIA table-role dependency) — this is the +one exclusion AC 7.3 explicitly allows ("a code comment states which are excluded and why"). +Scenarios 30-33 (checkbox-state assertions that don't depend on table semantics at all — the +toggle group renders identically at every viewport) run at all 3 configured viewports via +`{ tag: '@responsive' }`, with NO per-viewport branching needed in the assertion bodies (verified +via `--list`: 53 tests total in the file across [desktop]/[tablet]/[mobile], vs. 34 before this +issue). Scenario 29 (PDF download) stays desktop-only — the download mechanism itself isn't +viewport-dependent, only the DOM-interaction scenarios are; this matches the E2E spec's explicit +carve-out. + +## POM additions (`e2e/pages/ReportWizardPage.ts`) + +- `columnToggleGroup` = `page.getByRole('group', { name: 'Show/hide columns' })` — was previously + inlined at every call site in the old #1966 test; factored out since it's now reused across 6 + scenarios. +- `usageHiddenAttachmentsWarning` = `page.locator('[class*="bannerWarning"]')` — scoped by + CSS-module class (verified via grep: `bannerWarning` is used by exactly ONE component in the + whole client tree, `ReportContentEditor.tsx`), not by text, so it's stable against copy edits + and unambiguous against the page's other `role="status"` regions (`Toast`, several + `srOnly`/loading indicators — confirmed via grep there are ~15 other `role="status"` elements + across the app, several of which could plausibly be present on this same page). + +## Reusable navigation fact confirmed this session + +`goBack()` (`page.locator('[class*="buttonRow"] [class*="btnSecondary"]').first()`, re-queried +lazily at each `.click()`) is safe to call repeatedly to walk backward through MULTIPLE steps +(verified: only one `buttonRow` is ever mounted at a time across all 5 wizard steps) — 4 calls +walk step 5 → step 1. This is viewport-independent, unlike `goToStep()` which clicks the +desktop-only stepper widget (CSS-hidden below 768px) and would fail at the `mobile` project. +Prefer `goBack()` over `goToStep()` for any `@responsive`-tagged test that needs to navigate +backward. + +## Prior-CI triage performed for this session (see also `known-flakes-and-regressions.md`) + +Checked recent beta-merged PRs' full E2E results (`gh pr view --json statusCheckRollup`, +since `gh run list --branch beta --workflow "Quality Gates"` only surfaces promotion-PR runs, not +individual story/bugfix PRs — those run as `pull_request` checks on the PR's own head branch, not +a push to `beta`). Found ONE red shard in the last ~15 merges: PR #2007 ("refactor(reports): +remove TFunction from reportPdf/*", merged 2026-08-05), shard 10/16, both attempt+retry failed on +`invoices/invoice-vendor-change.spec.ts:129` [tablet] "Changing the vendor and saving updates the +detail page and vendor list" with `TimeoutError: locator.waitFor: Timeout 10000ms exceeded`. +Confirmed via `gh pr view 2007 --json files` that PR #2007's diff touches ONLY +`reportContent`/`reportPdf`/`ReportWizardPage.tsx` files — nothing under `invoices/` or +`vendors/` — so this is unrelated to that PR's own change and NOT caused by #1973's work either +(different domain entirely). Not yet triaged to root cause (single occurrence so far, not +established as a recurring flake) — flagging here for whoever next touches +`invoice-vendor-change.spec.ts` or investigates a shard-10 tablet failure. Did not attempt a fix +(out of scope for #1973, and a single occurrence isn't enough evidence to diagnose confidently). diff --git a/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md b/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md index 902bb9655..4864167a8 100644 --- a/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md +++ b/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md @@ -7,6 +7,7 @@ metadata: ## Currently open / unresolved +- **PR #2007 shard 10/16 (2026-08-05, merged to beta despite this)**: `invoices/invoice-vendor-change.spec.ts:129` [tablet] "Changing the vendor and saving updates the detail page and vendor list" — `TimeoutError: locator.waitFor: Timeout 10000ms exceeded` on both attempt and retry. Confirmed via `gh pr view 2007 --json files` that #2007's entire diff is `reportContent`/`reportPdf`/`ReportWizardPage.tsx` — nothing under `invoices/`/`vendors/` — so this is unrelated to that PR and not a regression it introduced. Single occurrence so far (not yet confirmed as a recurring flake vs. one-off CI resource contention) — no root-cause investigation done yet. Flag for whoever next touches this spec file or hits a shard-10/tablet failure. - `i18n-categories.spec.ts` "German locale: Manage trades tab shows 'Sanitär'..." — intermittent, locale doesn't initialize before English page renders. Pre-existing (seen before PR #1186 too). - **Latent cross-file hazard (unfixed, needs owner):** `e2e/tests/navigation/dashboard.spec.ts`'s file-level `beforeEach` PATCHes `locale='en'` on the **shared admin** user for all ~30 of its tests (added defensively so an i18n test's leftover `de` wouldn't break German card headings). `i18n.spec.ts`'s file-scoped serial mode does NOT protect against this — it only serializes within its own file. They happen to land in different shards today (verified: shard 4 contains `i18n*.spec.ts` but not `dashboard.spec.ts`), so it is currently latent. If E2E shard redistribution ever co-locates them, every i18n German assertion becomes racy again and the real fix is dedicated users in `i18n.spec.ts` (`i18n-categories.spec.ts` pattern). - `budget-overview-print.spec.ts` "Dark mode: print resets CSS variables" — HARD FAIL, production bug #1451 (`:global(@media print)` dropped by bundler). @@ -17,6 +18,11 @@ metadata: ## Resolved / fixed +- **Issue #2030 (2026-08-06)** `diary-automatic-events.spec.ts:100` "Clicking `work_item_status` type chip sends correct type parameter" — `expect(lastRequest).toBeDefined()` could fail because the test reset a shared `requests: URL[]` array (populated by a `page.route` handler at *request*-interception time) and then awaited a generic `page.waitForResponse((resp) => resp.url().includes('/api/diary-entries') && resp.status() === 200)` — a predicate matching ANY diary-entries 200, not specifically the chip-click's own request. A trailing in-flight response from the immediately-preceding "All" mode-chip click could resolve that promise before the chip's own request was ever pushed to the array, leaving it empty. The in-code comment claiming `waitForLoaded()` (races timeline/empty-state/error-banner visibility) "guarantees the API response was received and captured" was false — it only proves the UI settled, not that no request is still in flight; this is the same false-guarantee shape as the `dashboard.spec.ts`/`invoices.spec.ts` races above, just with `waitForResponse` racing a request-time array push instead of two tests racing a shared preference row. + **Fix (test-only, 3 files)**: replaced the generic predicate + shared-array-read pattern with a predicate scoped to the specific query param the test cares about (`new URL(resp.url()).searchParams.get('type')?.includes('work_item_status')`), and read the type param straight off the resolved `Response.url()` instead of an array — eliminates the request/response race by construction rather than narrowing its window. Confirmed by code-reasoning (live browser unavailable in sandbox, see [sandbox-live-verification.md](sandbox-live-verification.md)) that this still fails correctly if production stops sending the param: `URLSearchParams.get()` returns `null` for an absent param, the predicate never matches any response, `waitForResponse` times out, test goes red — not a silently-passing weak assertion. + **Sibling sweep (AC4)**: grepped `e2e/tests/` for the `: URL[] = []` capture-array idiom (found only in the 3 diary spec files) and for `.length = 0` resets (diary files + one unrelated `prefPatches` array in `reportWizardEditableContent.spec.ts` that isn't gated by `waitForResponse` at all, confirmed not the same shape). Found and fixed TWO more instances of the exact same bug shape: `diary-list.spec.ts` Scenario 9 ("daily_log" chip) and `diary-r2-uat.spec.ts` Scenario 9 ("Automatic mode" chip). Also hardened `diary-r2-uat.spec.ts` Scenario 10 ("Manual mode"), which had ALREADY been fixed at some earlier point by reading straight off the resolved `Response` (comment on that fix explicitly references the array-race) — its predicate was still generic, so tightened it too (marker: "all" mode sends no `type` param at all, so `!!typeParam` alone distinguishes a stale All-response) even though its pre-existing fix meant a residual race there would have surfaced as a loud assertion failure, not a silent pass. + **General lesson**: `waitForResponse` predicates that only check `url().includes(...) && status() === 200` are unsafe synchronization points whenever more than one request to that same endpoint can be in flight across an action sequence (mode switch → type filter, tab switch → chip, etc.) — predicate on the specific query param/method/body the test is about, and read assertions off the resolved `Response` object directly rather than a separately-populated capture array. The array-capture idiom itself (`push` at request time, read at response time) is the actual defect mechanism, independent of how tight the `waitForResponse` predicate is — removing the array's role in the assertion is the more robust fix, not just narrowing the predicate. + - **`i18n/i18n.spec.ts` shard-4 failures — TWO distinct mechanisms, both root-caused from traces 2026-08-03 (`fix/e2e-flakes-promotion`, CI run 30790367863 shard 4, both blocking `E2E Gates` on promotion).** Prior memory entries had guessed "intermittent locale-init race" for both; only one of the two was interference. **`:305` "DELETE preference resets to system locale" (failed attempt AND retry) — self-inflicted, NOT cross-test interference.** Proof it needs no second test: the retry ran with zero concurrent i18n tests (previous test ended 06:35:46.466, retry began 06:35:56.679) and failed identically. Mechanism, read straight off `0-trace.network` joined against `0-trace.trace` call IDs: `setLanguage(page,'de')` PATCHes the server row AND writes `localStorage.locale='de'`, then leaves the page open. The test's `DELETE /api/users/me/preferences/locale` (204) lands while that page's `LocaleContext.syncWithServer` has a `GET /preferences` in flight; the GET is served **after** the delete commits, returns no `locale` row, so syncWithServer takes its else-branch — `readStoredPreference()` still reads `'de'` from localStorage → `upsertPreference('locale','de')` **re-creates the row the test just deleted**. Observed as an app-issued `PATCH /api/users/me/preferences` 13 ms after the DELETE on attempt 1 (`40.449Z` → `40.462Z`) and 18 ms after on the retry (`57.138Z` → `57.156Z`), with no corresponding test call anywhere in the trace's call list. The subsequent reload then reads `locale=de` and the page stays German — `error-context.md` shows `heading "Profil"`, `navigation "Hauptnavigation"`, `"Zeitplan"`. This is intended production behaviour (the localStorage→server migration path for pre-server-preference clients), so the app was not touched. The narrow ms-wide window is why it failed only sometimes (it passed on #1926). **Fix**: set German **server-side only** (inline PATCH, deliberately not `setLanguage()`), then `goto` + await the German `Profil` heading. That heading is the synchronisation point that proves syncWithServer took its _other_ branch ("server has a preference"), which applies the locale **and calls `localStorage.removeItem('locale')`** — so localStorage is provably empty before the DELETE, making the migration branch a guarded no-op (`if (localLocale !== 'system')` is false). No wait/timeout was added; the old `waitForLoadState('networkidle')` + `{timeout: 15000}` were **removed** (that timeout equalled the 15 s test timeout, so the assertion could only ever surface as an uninterpretable test timeout, never as an assertion failure). diff --git a/.claude/agent-memory/e2e-test-engineer/sandbox-live-verification.md b/.claude/agent-memory/e2e-test-engineer/sandbox-live-verification.md index 9e5391947..5ccae3262 100644 --- a/.claude/agent-memory/e2e-test-engineer/sandbox-live-verification.md +++ b/.claude/agent-memory/e2e-test-engineer/sandbox-live-verification.md @@ -75,6 +75,22 @@ CSP header itself, via a raw `curl`/`page.request` HTTP check that doesn't need plus code-reasoning for the browser-rendering part, and document the CI expectation explicitly — this is what Story #1891's CSP-hardening verification did. +## Re-confirmed 2026-08-06 (Issue #2030 session, no container build attempted) + +`npx playwright install chromium` still 403s identically on all three mirrors +(`cdn.playwright.dev`, its `dbazure` path, `playwright.download.prss.microsoft.com`) — +`no matching allow rule — blocked by default deny policy`. `chromium-browser --version` still +fails with the same `requires the chromium snap to be installed` message, and there is still no +`snapd` socket/service (`systemctl status snapd` → "System has not been booted with systemd as +init system"). This means a mutation-test AC ("change production code, confirm the E2E test goes +red, revert, confirm byte-identical") cannot be executed live in this sandbox class — do the +edit/observe-would-fail-by-code-reading/revert cycle instead, verify the revert is byte-identical +via `git diff`/`git status` (that part *is* mechanically provable), and state the live-red +confirmation as deferred to CI in the report. Did not attempt the `dhi.io` container build this +session (task didn't need it — pure E2E test-file edits, no app change), so that half of the +2026-07-30 finding is unverified this round; re-attempt it fresh next time a task needs it rather +than assuming either outcome. + ## Practical technique: overriding the browser executable without touching committed config If a real Chromium binary ever IS available (e.g. `sbx policy allow` was granted, or a cached diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index 373f93876..c704f9737 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, cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), AC reversal by a polish issue (#1959) +- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\`` doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python `len()` not `awk length()` (em-dashes are 3 bytes) + two open findings: the `BACKUP_DIR` default/gate is wrong on Architecture.md and API-Contract.md's `splitKind` table is a latent `format:check` failure (#1992, wiki `e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms` while enforcing a regex subset, a caution box falsified by `parseInt` leniency, and `trustProxy: 1` being a hop count not "trust all proxies" (#1990, PR #2027)**, **fuzz the verbatim ports when a doc comment carries an induction proof (a hand-trace only re-reads the author's argument) + a safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940, PR #2032)**, **flex `gap` + child `margin` are additive not collapsing (bit twice in one PR — code AND the spec reviewing it) + a cohesive prop group modelled as N independent optionals + "leaves N chars for X" comments invite a guard test that pins a fiction: check whether X is bounded at all and whether the consumer clips or paginates (#1941, PR #2033)**, **a mutation count is not evidence of independent coverage — diff a new test's assertion body against its neighbours before trusting its title, esp. a negative-dependency title whose body never names the dependency + when a structural guard would have to re-encode the coupling under removal, a reason-carrying comment IS the right tool (#1953, PR #2035)** - [Dual-rail aggregation](dual-rail-aggregation.md) — Rail A/B tagged-deposit invariants (#1891/PR #1894), residual-denominator rule, isSplit UNION -- [Source-report split inference](source-report-split-inference.md) — budgetLines[]/deposits[] are this-source-scoped, so †/‡ classification is a proxy; proposed `splitKind`; pdfmake `'2*'` width trap +- [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 -- [Client PDF pipeline](client-pdf-pipeline.md) — ADR-034 report PDF generation, reportContent content/layout split (#1900), `dontBreakRows` silent-drop rule + **owed ADR-034 corrections** (#1959) +- [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.** **#1973/PR #2010 (2026-08-05, CHANGES REQUESTED): the 2-hardcoded-shapes era is over — 96 legal column subsets via a single-absorber `computeColumnWidths`, `reportContent/columns.ts` as the AC 2.1 single derivation; `USAGE_WIDTH_7COL` 138.28pt is the NARROWEST Usage width so the 650 chunk budget needed no re-measurement (one-sided clamp). ADR-034's "Geometry constraint that blocks a feature" (line 153) was a false prohibition, plus line 144's constant reference and line 155's "hangs" claim (it throws). **All three PAID by me 2026-08-05, wiki `eb24774`**: section replaced with "Column geometry is a computed engine, not two pinned shapes", `columns.ts` added to the module structure, Deviation Log row. Same commit documented the wizard **tier-factory convention** in Architecture.md (never documented anywhere before, born #1947 after #1943/#1946) incl. the "second `DISCARD_EDITS` opt-out must trigger a tier split" limit.** **#1912/PR #2028 (2026-08-06, APPROVED): required `reportFormatters` is ADR-034 line 230's own "remove the channel" principle, and line 248 already documented the 6-arg signature — no Deviation Log row, but invariant 1 (line 206) now under-claims and wants one sentence on the next ADR-034 pass, alongside a one-liner naming `Formatters` (PDF injection contract) vs `AppFormatters extends Formatters` (app-wide superset). Open medium: `buildReportContent.ts` still has 4 unguarded union-into-`reportT()` keys (lines 143/146/204/274) rendering into the bank PDF — see recurring-patterns "1 of N sites".** **#1940/PR #2032 (2026-08-06, APPROVED): runt-merge gate + `'… '` continuation marker; the backward-merge induction (receiver virginity is structural) verified by hand AND a 400k-case verbatim-port fuzz; ADR-034 line 152's call-site quote stale a THIRD time and line 148's "bound what a cell renders" rule now under-satisfied by the 2-char unbounded marker (safe by size only: worst case +1 line, 41->42 vs the 44-line budget). #1950 needs no reorder, but its guard pins a rendered quantity that the marker moves to 36 chars / 4 lines / 44.8pt on continuation rows.** **#1941/PR #2033 (2026-08-06, APPROVED, comment not `--approve` — own PR): editor-side input caps land as module constants in `ReportContentEditor.tsx`, `USAGE_TEXT_MAX_LENGTH = 500` against the computed 650. **ADR-034 pass gains a FIFTH item**: line 148's "bound what a cell renders" now needs an input-side counterpart sentence — "input caps sit below the render budget so a typed value never needs a continuation row on its own; the render budget remains the only correctness bound." Also routed to #1950: extend its guard to `USAGE_TEXT_MAX_LENGTH < usageChunkCharsForWidth(USAGE_WIDTH_7COL)` (needs the constant exported; keep it in the editor — an input constraint in the renderer inverts the dependency). NOT a guard on `500 + 150 <= 650`: that arithmetic is a fiction, see recurring-patterns.** - [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) @@ -33,10 +33,17 @@ ## GitHub Wiki -- Git submodule at `wiki/`. Sync: `git submodule update --init wiki && git -C wiki pull origin master` +- Git submodule at `wiki/`. The orchestrator syncs it once per skill run (CLAUDE.md > Agent Context Discipline) — read the checked-out files directly, don't re-sync - Submodule is normally in **detached HEAD** at origin/master — push with `git push origin HEAD:master` +- **Verify published-ness with `git -C wiki ls-remote origin master` vs `git ls-tree HEAD wiki`**, never with + `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) @@ -74,4 +81,16 @@ is still undocumented in Schema.md. - esbuild SIGILL on emulated aarch64; Docker build fails behind the TLS firewall - 4GB RAM: Jest OOM mitigated with `--maxWorkers=2 --max-old-space-size=2048` - Stale worktrees under `.claude/worktrees/` cause jest-haste-map duplicate-package failures. - Work around with `npx jest --modulePathIgnorePatterns='/.claude/worktrees/'` + Work around with `npx jest --modulePathIgnorePatterns='/.claude/worktrees/'` — **but only from + the base checkout.** Inside a worktree that pattern matches the cwd itself, so jest reports + `0 files checked across 3 projects` / `Pattern: - 0 matches` and exits 1. That looks like a + missing/misnamed test file, not a config problem, and can be misread as "the tests don't exist". + When running from a worktree, drop the flag entirely. +- Confirm a run actually executed something: `Tests: N passed` — a `--maxWorkers=1 -t ` run that + matched nothing still exits 0 in some invocations, so a silent pass is not evidence. +- **`.claude/agent-memory/` exists in BOTH the base checkout and every worktree, at diverging lengths.** + An "absolute" path that omits the `.claude/worktrees//` segment silently reads/edits the _base_ + copy — no error, just stale content and an edit that never reaches the PR. Hit this on 2026-08-04 + (base 263 lines vs worktree 416). Build memory paths off the cwd shown in the env block, and if a + `Read` offset unexpectedly reports "file is shorter than offset", suspect the wrong copy before + assuming the memory is wrong. `wc -l` both paths to confirm. diff --git a/.claude/agent-memory/product-architect/client-pdf-pipeline.md b/.claude/agent-memory/product-architect/client-pdf-pipeline.md index 746bccc37..be3b7b88d 100644 --- a/.claude/agent-memory/product-architect/client-pdf-pipeline.md +++ b/.claude/agent-memory/product-architect/client-pdf-pipeline.md @@ -20,19 +20,18 @@ ADR-034 in future reviews instead of re-deriving. **ADR-035 is the next free num ## Module seams (good decomposition, keep it) -| Module | Role | -| ------------------- | --------------------------------------------------------------- | -| `loader.ts` | Lazy `import()` of both packages, promise-cached | -| `shared.ts` | Page header/footer builders, table layout, PDF formatters | -| `coverLetterPdf.ts` | Pure fn: report -> cover-letter `Content[]` | -| `overviewPdf.ts` | Pure fn: report -> overview-table `Content[]` | -| `merge.ts` | Orchestration: fetch docs, build, pdfmake render, pdf-lib merge | -| `sinks.ts` | Output: download / preview blob URL / upload to Paperless | -| `types.ts` | `ReportPdfOptions`, `GeneratedReport`, `SkippedDocument` | - -Builders are pure and take `t: TFunction` -- they cannot use hooks, which is exactly why locale -and currency get dropped (see [[story-reviews]] B3). Anything locale-dependent must be threaded -in as a parameter. +**The authoritative module table is ADR-034's** (corrected 2026-08-04, issue #1914) — read it there +rather than duplicating it here, since this copy drifted twice. Current shape: `loader.ts`, +`pageGeometry.ts` (sole owner of the pt coordinate system, extracted #1939), `shared.ts` (header/footer +builders + `TABLE_LAYOUT` callbacks, **no formatters, no geometry constants**), `coverLetterPdf.ts`, +`overviewPdf.ts`, `merge.ts`, `sinks.ts`, `types.ts`, `index.ts`. + +Builders are pure -- they cannot use hooks, which is exactly why locale and currency got dropped +(see [[story-reviews]] B3). Anything locale-dependent must be threaded in as a parameter. Since #1900 +the builders take `(reportContent: ReportContent, …, t: TFunction)` and **no formatters at all**: +`buildReportContent` is the single place `reportFormatters` is applied. The residual `t` covers only +generation-time strings (`*N` skip-note reasons, cover-letter `Reference:`/`Subject:` prefixes) and is +always `reportT`. ## Lazy-loading contract (fragile -- verify on every change) @@ -81,14 +80,14 @@ change to `overviewPdf.ts` widths, `TABLE_LAYOUT`, or `pageMargins` must be vali `/CreationDate` + `/ID`); a unit test asserting `TABLE_LAYOUT.dontBreakRows === true` passes and proves nothing. 2. **Declared widths are CONTENT widths.** pdfmake subtracts `_offsets.total` from the available - width *before* distributing them. `offsetsTotal = cols * (paddingLeft + paddingRight + - vLineWidth) + vLineWidth`. With `TABLE_LAYOUT`'s `8/8/0.5` that is **116.0pt for 7 columns, + width _before_ distributing them. `offsetsTotal = cols * (paddingLeft + paddingRight + +vLineWidth) + vLineWidth`. With `TABLE_LAYOUT`'s `8/8/0.5` that is **116.0pt for 7 columns, 99.5pt for 6** out of the 515.28pt A4 printable width. Budget columns against `515.28 - offsetsTotal(cols)`, not 515.28. Getting this wrong made a comment claim Usage got 185.28pt when it actually got **69.28pt**. 3. **A `'*'` column never shrinks below its longest unbreakable word.** `columnCalculator.js:66-75` — when `minW >= availableWidth` the star is set to `starMaxMin` and - *the table overflows the page*. So no static assertion on the `widths` array can prove "no + _the table overflows the page_. So no static assertion on the `widths` array can prove "no horizontal overflow": German compounds (`Wärmedämmverbundsystem` ~128pt @10pt Roboto) push a 69.28pt star to 128pt and the table to 574pt on a 515.28pt page. 4. **`dontBreakRows` + a row taller than the printable height = silent data loss.** pdfmake does @@ -107,11 +106,12 @@ Node API cannot expose computed widths is **wrong** — it can, it is just a pri the version. Same trick measures text: `pdfkit` + the Roboto TTF out of `vfs_fonts` gives `doc.widthOfString(s)` for exact fit checks (avg lowercase prose char ~4.68pt @10pt Roboto). -**Page geometry is scattered across three files** (`PAGE_TOP_MARGIN` in `shared.ts`, L/R/B inline +**Page geometry WAS scattered across four sites** (`PAGE_TOP_MARGIN` in `shared.ts`, L/R/B inline in `merge.ts`, printable-width prose comment in `overviewPdf.ts`, paddings in `TABLE_LAYOUT`). -Recommended in the PR #1935 review: one `pageGeometry` module exporting `PAGE_WIDTH/HEIGHT`, -`PAGE_MARGIN_*`, `CELL_PADDING_X`, `V_LINE_WIDTH`, `printableWidth()`, `printableHeight()`, -`tableOffsetsTotal(cols)`, `usableColumnWidth(cols)`. `tokens.css` is explicitly NOT the answer — +Recommended in the PR #1935 review and **since implemented as `pageGeometry.ts` (#1939)**, exporting +`PAGE_WIDTH/HEIGHT`, `PAGE_MARGIN_*`, `CELL_PADDING_X`, `V_LINE_WIDTH`, font sizes, `PDF_STYLES`, +`printableWidth()`, `printableHeight()`, `tableOffsetsTotal(cols)`, `usableColumnWidth(cols)`, +`headerFootprint()`, and the derived `PAGE_TOP_MARGIN` (now 93, not 75). `tokens.css` is explicitly NOT the answer — the pdfmake layer is its own pt coordinate system outside the design system. **`PAGE_TOP_MARGIN` (75) is derived from a single-line-header assumption.** `buildPageHeader` @@ -234,13 +234,13 @@ document): page count must be **monotonic** in content size, and **channel-indep costs the same paper whichever field carries it. **The generic-assertion / hand-enumerated-input asymmetry** (the reason it recurred, now the top open item): -the per-row budget assertion counts characters generically over all runs, so a new channel *would* be +the per-row budget assertion counts characters generically over all runs, so a new channel _would_ be counted — but the test inputs are hand-listed (`usageText`/`areaText`/`attachmentsNote`), so a new `ReportContentRow` string field defaults empty and every assertion passes **vacuously**. Fix is key-driven saturation (`Object.entries(row)` -> saturate every string field), not another hand-written case. Verified empirically during review: losslessness + per-row budget hold over 3k fuzzed inputs (0 failures); a hypothetical **second** grey/meta segment trips the existing `splitUsageCell` "at most one grey run per row" -throw in ~49% of inputs, so *that* channel class is already guarded. +throw in ~49% of inputs, so _that_ channel class is already guarded. **Geometry constraint (blocks a feature):** `USAGE_WIDTH_7COL/_6COL` derive from `usableColumnWidth(n)`, and `MAX_SAFE_USAGE_CHUNK_CHARS = 650` was **measured against the 7-column shape**. So making column visibility @@ -251,13 +251,392 @@ through" is a re-measurement story, not a UI change. #1959's toggles are preview New to the packer — `splitIntoPageSafeChunks` fails loudly instead (`RangeError`). Unreachable while the budget is a constant; matters if it ever becomes computed. -### ADR-034 debt (owed, NOT yet written — carry this forward) +### Legend is document-level, not per-row (#1965) + +DONE 2026-08-03: ADR-034 records this (wiki master `03ed804`, addendum "the legend is document-level and +deduplicated" + a Deviation Log row + a reworded B4 rule). The old B4 wording ("every footnote is referenced +from the row that owns it") described the pre-#1965 inline-symbol design (`†`/`‡`) and was **wrong** for the +current model. + +Two structurally different note kinds now share the legend block but **not** a numbering scheme: + +- **`*N` skipped-document notes** — numbered, row-owned, one per skipped document, built in `overviewPdf.ts` + at generation time (never in `ReportContent`). B4's "referenced from the owning row" rule applies here only. +- **`content.footnotes[]` legend entries** — **at most one per flag type for the whole document** + (currently 2: `split`, `depositReduced`). `buildReportContent.ts` accumulates `splitInvoiceIds` / + `depositReducedInvoiceIds` as `Set` and pushes gated on `set.size > 0`, so cardinality is + independent of how many rows carry the flag. `marker` is the repeated human-readable inline label + (`partial` / `less deposit`, report-language) that the Allocated Amount cell prints, **not** an identifier + — `id` is the machine key. Row↔legend link is **by repetition of the label**, not by stored reference; + rows carry only `isSplit`/`isDepositReduced` booleans. Storing a footnote index on a row would recreate + B4's second numbering namespace. + +Regression to guard when adding a flag type: emitting one entry per flagged row. Assert +`footnotes.length === N` (never `>= 1`) on a fixture where several rows share a flag. + +### ADR-034 debt — PAID 2026-08-04 (issue #1914) + +All five items below are now in the wiki. Do not re-file them. + +1. `dontBreakRows` lesson + "bound the rendered cell, not a source field" + both detection recipes + (monotonic page count, channel-independence) → new section "Unbreakable rows are silently dropped", + plus minimum-bar rule #2 pointing at it. +2. **Minimum-bar rule #1 corrected.** The old `table._minWidth <= 515.28` was a mis-transcription of B2's + _diagnostic_ into a _correctness check_. Replaced by two render-derived assertions: + `max(node.positions[].horizontalRatio) <= 1` and + `tableOffsetsTotal(cols) + sum(widths[i]._calcWidth) <= printableWidth()`. **Verified in this pass:** + `horizontalRatio = (x - pageMargins.left) / innerWidth`, set at `pdfmake/js/DocumentContext.js:490`. + `horizontalRatio` appears **nowhere in the Cornerstone codebase** — the assertion is documented but not + yet implemented; `realRender.test.ts` uses the `_calcWidth` sum form only. + **Re-verified 2026-08-04** (PR #2002 round-2 review): still zero hits for `horizontalRatio` across + `client/`. Consequence worth naming — every horizontal-overflow fix in this pipeline is verified by + **mechanism** (`wordBreak: 'break-all'` is present on the expected run) rather than **outcome** (nothing + advanced past the printable edge). A mechanism assertion cannot catch "the mechanism fired and it still + overflowed" (wrong `safeTokenChars` threshold, or a column-width edit that silently invalidates one). + Now tracked as **issue #2003** (tech-debt, should-have, on the board backlog), which I own. Its AC + requires a revert test on the new assertion — a documented-bar-turned-test that cannot fail would just + move the debt. Pair candidate: rule #2's `_calcWidth` table-box form is also documented-and-unimplemented. +3. Module table rewritten: added `pageGeometry.ts` (sole owner of the pt coordinate system) and `index.ts`; + `shared.ts` no longer claims formatters or geometry constants. Recorded _why_ the formatters were + deleted (a PDF-local formatter is a second formatter bound to a different locale = B3). +4. Override keys corrected against `reportContent/overrideKeys.ts`: `coverLetter.{sender,recipient, +reference,subject,body,signature}` + `row..usageText`. `attachmentsNote` is gone; `signature` was + added with a **three-way precedence** the old flat sentence misdescribed — explicit override wins, + else re-derive from sender **only if `senderChanged`**, else baseline. +5. Fixed 6-or-7 column-count constraint recorded (blocks plumbing the column toggles into the PDF). + +Also folded in during the same pass: the "injection is the only channel" contract (4 numbered invariants + +grep guard, story #1899 / architect L5), the strengthened labels rule, and the `_minWidth` +diagnostic-vs-check pointer on B2's narrative. Six Deviation Log rows added. + +### `merge.ts` footer locale leak — STILL OPEN (code defect, not wiki debt) + +`merge.ts:134` is still `buildPageFooter(t('sourceReports.table.pageLabel'))` — the **interface** `t`. With +interface DE + report EN, page 2+ is footed `Seite 2 / 5` under an English report. Verified still present +2026-08-04. The header was fixed in #1938; the footer was not. Fix shape: add `pageLabel` to +`ReportContentLabels` (which is `reportT`-derived). Now recorded in ADR-034 as a known open violation — +**needs a GitHub issue**, it has only ever been a PR-review follow-up note. + +## ADR-034 legend model, corrected in PR #1979 (wiki `03ed804`) + +The ADR-034 debt owed since #1959 is now paid. Two structurally different note kinds share the block below +the overview table and must never share a numbering scheme: + +| Kind | Marker | Cardinality | Built by | +| ------------------------------------ | ------------------------------------------------------ | ------------------------------------------ | ------------------------------------------------------------ | +| Skipped-document note | `*N`, numbered, referenced by the owning row | one per skipped document | `overviewPdf.ts` at generation time (not in `ReportContent`) | +| Legend entry (`content.footnotes[]`) | repeated inline word label — `partial`, `less deposit` | **at most one per flag type per document** | `buildReportContent.ts` | + +B4's old generalized rule ("every footnote is referenced from the row that owns it") applied only to the +numbered kind and was reworded. Invariants now recorded in the ADR's legend addendum: + +- `footnotes[].marker` is `sourceReports.table.{split,depositReduced}InlineLabel` — the _same_ keys as + `labels.{splitNote,depositReducedNote}` and as the inline label the row cell prints. Row↔legend joins by + **repetition of that literal**, not by id/index/number. NBSP in `less deposit` / `abzgl. Abschlag` is + load-bearing; `expect(footnotes[0].marker).toBe(content.labels.splitNote)` is the assertion that pins it. +- Gated on `splitInvoiceIds.size > 0` / `depositReducedInvoiceIds.size > 0` (`Set` accumulated in the + `includedInvoiceIds`-filtered row loop), so `footnotes.length` is bounded by flag count (2), never row count. +- Adding a flag type = new `Set` + `size > 0` push in `buildReportContent.ts`, new boolean on + `ReportContentRow`, new inline label in `overviewPdf.ts`. Assert exact `footnotes.length` (not `>= 1`) on a + fixture where several rows share a flag. +- Preview/export parity trap: once markers became _words_, `ReportContentEditor`'s + `{marker}:{text}` ran them together while the PDF used `${marker}: ${text}`. Fixed in #1979 — + any change to either surface must keep the separator identical. + +## Fixed-width column headers impose a per-locale character budget (#1937/#1938, PR #1982) + +The overview table's columns are fixed-width (`VENDOR_WIDTH = 45`, `INVOICE_AMOUNT_WIDTH = 48`, …) and +pdfmake's `elasticWidth` never grows a fixed column to fit its own header. So **every DE translation of a +`sourceReports.table.*` header key is width-constrained**, and DE is always the binding locale. + +- `buildHeaderCell` applies `buildUsageTextRuns` (per-token `wordBreak: 'break-all'`) to every header cell. + That is a _last-resort_ fallback (pdfmake 0.3.x has no hyphenation), not the fix: a mid-word break with + no hyphen on a bank-facing document is a defect in its own right. The fix is a shorter localized label. +- #1937 shortened `vendor` `Auftragnehmer` → `Firma` and `invoiceAmount` `Rechnungsbetrag` → `Betrag`. + The break-all mechanism **must stay** — vendor _data_ (server cap 200 chars, German compounds) still + needs it, and #1937 explicitly accepted broken vendor names as unfixable without a layout change. +- Correct guard: a real-render assertion that the header cell resolves to `positions.length === 1` in the + `de` locale. Character-count arithmetic is a weaker proxy (see recurring-patterns.md). +- `overviewPdf.test.ts:833-861` and `VENDOR_HEADER_WORST_CASE_LINES` use hardcoded `'Auftragnehmer'` + fixtures/literals, _not_ the live bundle — so they survive translation changes, but their comments and + test titles rot into claiming to describe the live DE labels. +- Consumers of `labels.*`: `overviewPdf.ts` (PDF) and `ReportContentEditor.tsx` (`` preview, mobile + card captions, column-toggle text). `ReportContentLabels` is `reportT`-derived and **not user-editable**, + so a shortened label is safe — and must be identical in both surfaces by design. +- Glossary tension: `glossary.json` maps `Vendor` → `Auftragnehmer`. PDF column-header short forms diverge + from glossary terms under a measured constraint; that exception needs recording _in glossary.json_, not + just in translator memory, or an audit reverts it. + +### Running header/footer must source strings from the report content model + +`merge.ts`'s `header:` callback took the interface `t` for the generated-at label and never passed the +value (#1938) — a bare label on pages 2+ of every multi-page report. Fixed in PR #1982 to +`` `${reportContent.labels.generatedAt}: ${reportContent.sourceInfo.generatedAtText}` ``, byte-identical to +the page-1 block in `overviewPdf.ts:531`. Rule (from #1909): **artifact content resolves through +`reportT`/`reportFormatters`; only edit affordances use the interface `t`.** + +- **Still violating it: `merge.ts:134`** — `buildPageFooter(t('sourceReports.table.pageLabel'))`. With + interface DE / report EN the footer reads `Seite 2 / 5` under an English report. Needs a new + `pageLabel` on `ReportContentLabels`; flagged as a follow-up in the PR #1982 review. +- Header height budget: `headerFootprint()` (`pageGeometry.ts`) models only the LEFT stack (title + + two-line subheader = 57.2pt) + 20pt block margin → `PAGE_TOP_MARGIN = 93`. The generated-at line is the + right child of a two-column node at implicit `'*'` (~257pt on A4) in `small` style, so appending the + value cannot threaten the margin — even a two-line wrap (~18pt) stays far under the left stack. + +## pdfmake typing: `Content` cannot be spread, but `Object.assign` needs no cast + +Verified with `tsc --strict` in-repo (PR #2002 review): + +- `{ ...r, color }` where `r: Content` → **TS2698 "Spread types may only be created from object types"** + (the union includes `string`). Real error, not a lint rule — `tseslint.configs.recommended` is not + type-checked, so `no-misused-spread` is not even enabled here. +- `Object.assign({}, r, { color })` **is** already assignable to `Content` — the `as Content` assertion + seen in `overviewPdf.ts:702` is redundant. **Confirmed against the real project 2026-08-04** (not just a + scratch file): deleting ` as Content` and running `npx tsc --noEmit -p client/tsconfig.json` yields no + error at that line. PR #2002 shipped the redundant cast anyway (accepted as non-blocking M1). + Method note worth reusing: the client project has ~63 **pre-existing** errors, so "tsc is clean" is not + an available signal — ask "any error in MY file?" and prove tsc really checked it with a **positive + control** (drop a `const __probe: number = "not-a-number"` into the same function and confirm tsc reports + it). Without that control, a clean grep for your filename is indistinguishable from tsc never reaching it. + Those 63 errors are stale-`shared`-type noise (`llmEnabled`, `claimable`, `areaId`) that survives + `npm run build --workspace @cornerstone/shared`, so don't chase them. +- Cleanest fix: narrow the helper's return type. `type TextRun = { text: string; wordBreak?: 'break-all' }`, + `buildUsageTextRuns(...): TextRun[]` — then `{ ...r, color }` compiles, `TextRun[]` stays assignable to + `Content[]` (so `buildHeaderCell` and the vendor/usage cells keep working), and the signature stops + over-promising that the helper might return a column/table/stack node. +- `Content` resolves via `@types/pdfmake/build/pdfmake.d.ts` → `export * from "../index"` → `interfaces.d.ts`, + so `pdfmake/interfaces.js` and `pdfmake/build/pdfmake` give the identical type. Scratch typecheck files + must live under `client/src/` to resolve it; `/tmp` gets TS2307. + +## pdfmake run-splitting facts (read from source, PR #2002) + +`node_modules/pdfmake/src/TextBreaker.js`: + +- `wordBreak` is resolved **per item** (`getBreaks`, :124-127) via `StyleContextStack.getStyleProperty`, and + `copyStyle(item)` copies `color` onto every produced word. So `wordBreak: 'break-all'` and `color` + coexist on one run correctly, and `Object.assign` preserves the flag (own enumerable prop). +- A run whose whole text is `'\n'` still forces a line break: `splitWords('\n')` → `{ text: '', lineEnd: true }` + (:43-46). Crucially `getLastWord` returns `null` when the last word has `lineEnd` (:88-90), so the + cross-run merge that sets `noNewLine` (:135-141) is **skipped** after a newline-only run. +- Why that matters: had `noNewLine` landed on the `'\n'` word, `LayoutBuilder.js:1388`'s + `hasEnoughSpaceForInline(...) || isForceContinue` would have pulled the next token onto the same line + despite `Line.newLineForced`, silently swallowing the separator. Splitting a `'\n'`-prefixed run into + per-token runs is safe, but check `getLastWord`/`noNewLine` before assuming so for any new split site. + +## Usage-cell meta suffix renders at body size, not note size + +The grey `areaText`/`attachmentsNote` suffix inherits `tableCell`'s `TABLE_BODY_FONT_SIZE` (8pt, +`pageGeometry.ts:104-106`) and deliberately does **not** set `DEPOSIT_NOTE_FONT_SIZE` the way the +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 +``` -1. Add the `dontBreakRows` lesson + the "bound the rendered cell, not a field" rule + both detection recipes. -2. **Minimum-bar rule #1 is wrong**: `table._minWidth <= 515.28` fails on correct code (`_minWidth` is the - widest unbreakable *word*, not the laid-out width). Correct check: `max(horizontalRatio) <= 1`. - B2's narrative is fine; the generalized rule was mis-transcribed. -3. Module table drifted twice: add `pageGeometry.ts` (#1939) and `index.ts`; drop "PDF-local formatters" from - `shared.ts` (deleted in review round 2) and move "table layout constants" to `pageGeometry.ts`. -4. Override-key list (line 148): drop `attachmentsNote` — unreachable since #1959. -5. Record the fixed 6-or-7 column-count constraint above. +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. + +## #1973 / PR #2010 — column-visibility geometry engine (reviewed 2026-08-05, CHANGES REQUESTED) + +The two hardcoded table shapes are gone. `overviewPdf.ts` now renders any of **96 legal column +subsets** (64 budget-overview + 32 claim; `allocatedAmount` locked, R1), driven by +`client/src/lib/reportContent/columns.ts` — the AC 2.1 single derivation consumed by *both* +`overviewPdf.ts` and `ReportContentEditor.tsx`. + +**Width mechanism (endorsed): single absorber.** `computeColumnWidths(visible)` picks +`usage` → else `vendor` → else `null`; the absorber takes `usableColumnWidth(n) − fixedSum`, every +other column keeps its pinned constant. This discharges R7/AC 3.1–3.5 *algebraically* +(`total = printableWidth()` exactly with an absorber, strictly less without) instead of by 96 +assertions. `'*'` was correctly rejected (#1929 `columnCalculator.js` case-1); proportional +slack-sharing would violate R7's "don't stretch a numeric table across 515pt". + +**Key derived fact: `USAGE_WIDTH_7COL` (138.28pt) is the NARROWEST Usage width across all 96 +subsets.** Removing any column both shrinks `fixedSum` (≥ 40pt) and grows `usableColumnWidth` +(+8.5pt), so Usage is strictly monotone-decreasing in column count. That is why +`MAX_SAFE_USAGE_CHUNK_CHARS` (650, measured at 138.28pt) needed **no re-measurement** — hiding +columns only makes it more conservative. `usageChunkCharsForWidth` is a deliberate one-sided clamp +(`min(650, floor(650 · w/138.28))`): scales down for a future *added* column, never up. + +**Standing review lesson — a wiki page can state a PROHIBITION that a later PR deletes.** +ADR-034's "Geometry constraint that blocks a feature" (line 153) said the PDF column count is +fixed at 6 or 7 and that wiring the toggles through "is a re-measurement story, not a UI change." +PR #2010 falsified every clause and touched no wiki file. **When reviewing a PR that removes a +constraint, grep the wiki for the constraint's own statement, not just for the API/schema surface +the PR touches** — a stale prohibition actively steers the next agent away from work that already +shipped. Also stale on that page and folded into the same ask: line 144 quotes +`packUsageCellRows(segments, MAX_SAFE_USAGE_CHUNK_CHARS)` (now per-subset `usageChunkChars`), and +line 155's "Related sharp edge" says `packUsageCellRows` **hangs** on `maxChars <= 0` — it throws +(`overviewPdf.ts:338`), and its own prediction ("it matters the moment it becomes computed") went +live with this PR. + +**Summary-label three-tier fallback (R2)** — tier 1 last visible leading column, tier 2 +`invoiceAmount`, tier 3 a stack block below the table. Real subset counts are **88 / 4 / 4**; the +issue body and the test title both say "92" for tier 1 (they merged tiers 1+2). Tier 3's block is +**not width-constrained** — two implicit `'*'` columns across 515.28pt while the +`{allocatedAmount}`-alone table is only 84pt, so the total floats 431pt from its column (2 of 96 +subsets; flagged M3). + +**ADR-034 debt for #1973 is PAID (wiki `eb24774`, 2026-08-05, by me during PR #2010 review.)** The old +"Geometry constraint that blocks a feature" section is replaced by **"Column geometry is a computed +engine, not two pinned shapes"**, which now carries the narrowest-Usage-width proof, the absorber +rationale (and why `'*'` / proportional slack-sharing were rejected), the one-sided clamp, the +residual "adding a column IS still a re-measurement story" constraint, and the forcing-function +audit. `reportContent/columns.ts` is in the module structure. Same commit added Architecture.md's +**"Multi-step wizard state: tier factories"** subsection — the `freshXTier()`-spread convention had +never been documented anywhere despite being the fix for #1943/#1946/#1947. + +**Still open on ADR-034 after this pass** (all pre-existing, none introduced by #1973): every quoted +`69.28`/`33.54`/`266.16pt` figure is stale (lines ~105/120/129/138), and rule #1 says "every cell" +while `realRender.test.ts` covers only Usage — Vendor at 45pt is the binding column. + +**Exhaustiveness audit pattern for a keyed geometry engine.** Compile-error-enforced here: +`PINNED_WIDTHS: Record`, `HEADER_LABEL: Record`, +`buildBodyCell`'s `default`-less switch. **Not** enforced: `OVERVIEW_COLUMNS`/`CLAIM_COLUMNS` in +`columns.ts`, `LEADING_COLUMNS`/`RIGHT_ALIGNED_COLUMNS`, and the absorber ternary — all hand-typed +lists over the same union, and the tests that "count" them pin the literals 7/6, so a new key is +silently absent everywhere. Recommended fix shape: derive the canonical order from an exhaustive +`Record` and define the base sets as filters over it. + +## #1940 / PR #2032 — runt-merge + continuation marker (reviewed 2026-08-06, APPROVED) + +`packUsageCellRowsWithMinimum(segments, maxChars, minTrailingChars)` wraps `packUsageCellRows` +(both `packUsageCellRows` and `splitIntoPageSafeChunks` byte-identical to `beta`, hash-verified). +Production floor: `Math.max(MIN_CONTINUATION_ROW_FLOOR_CHARS /* 20 */, usageSafeTokenChars)`. +Render-time marker: a bare `{ text: '… ' }` run prepended by `buildUsageCell(segments, true)` for +`packedCellRows` index >= 1 — never in `UsageCellSegment.text`, so I1 stays trivially true. + +**The AC1-vs-AC2 resolution worth reusing.** Three candidate designs; only one works: +merge-without-repack is *unsound* (receiver can already be at `maxChars`); always-repack-at-reduced +budget is sound but regresses the zero-degradation range; **gate the reduced-budget repack behind an +actual runt check** is the only one that pays neither. A "lookahead inside the packer" is not a +single-pass alternative — you cannot know a remainder exists without packing to the end, so the +lookahead *is* the first pass, and folding it in would cost the primitive its clean +"every row <= the budget I was given" contract, which is what the safety proof rests on. + +**The backward-merge induction, stated so it can be defended.** (1) primitive guarantees +`rowCharCount <= B`; (2) **receiver virginity is structural**: at counter `i` the only index written +is `i-1`, the counter strictly decreases, so index `k` is written iff the counter equals `k+1` — +exactly once ever; `splice(i,1)` shifts only indices `> i`; (3) the donor is `< min` *at the moment +of donation* because the guard re-reads the possibly-already-grown row. Hence +`(<= M-m) + (< m) < M` at any cascade depth. Verified by hand **and** by a 400k-case fuzz +(verbatim ports, `maxChars` 2..61, `min` 1..maxChars+4 incl. the degenerate band, meta segments, +leading empty segments): 0 AC1 / 0 AC2 / 0 I1 violations. **Fuzzing verbatim function ports in a +throwaway `.mjs` is the cheapest independent read of an induction argument — do this again whenever +a doc comment carries a proof.** + +**Wrapping row-level output was the right layer, for a stronger reason than the PR gave.** The PR's +reason (a runt arises from two paths, one of which the chunker never sees) is true — the packer's +own `used > 0 && rest.length <= maxChars` flush creates runts with `splitIntoPageSafeChunks` never +invoked. But the load-bearing reason is a **unit mismatch**: the AC's unit is the rendered *row*; +the chunker's unit is a chunk within one *segment*. A row can hold a prose chunk AND the grey meta +segment, so a chunk-level floor bounds the wrong quantity. + +**Findings left open (all non-blocking, for the ADR-034 pass):** +- ADR-034 **line 152**'s call-site quote is stale a **third** time (`MAX_SAFE_USAGE_CHUNK_CHARS` -> + `usageChunkChars` -> now `packUsageCellRowsWithMinimum(..., minTrailingUsageChars)`). Three + staleness events on one quoted signature: name the *contract*, drop the literal call. +- ADR-034 **line 148**'s rule ("bound what a cell *renders*") is now literally under-satisfied — the + marker is rendered and unbounded. Safe *by size only*: 2 chars, worst case **+1 line** (when the + next token is <= 16 chars so no `break-all`, but too long for the 14 slots left beside `'… '`); + 0 lines in the break-all case. 41 -> 42 vs the 44-line `№` budget. Nobody wrote that down, and the + ux spec already budgets 14 chars for a *textual* marker variant — the obvious next request. +- `packUsageCellRows` now has **exactly one production caller** (the wrapper). Add a "production + callers go through the wrapper" line to its doc comment. +- ux-designer's "threshold-to-ceiling **ratio** stays roughly constant across subsets" is **false** + (the one-sided clamp pins `usageChunkChars` at 650 while `usageSafeTokenCharsForWidth` scales with + width: ratio runs ~3% -> ~9.5%). Harmless — the algebraic bound is subset-independent — but do not + let the ratio framing get copied into the ADR as the reason. + +**#1950 sequencing: confirmed no reorder needed.** No geometry constant moves, and the repack budget +is strictly *below* the ceiling, so the new consumer is more conservative. But #1950's guard pins a +**rendered** quantity, and the marker adds 2 uncounted rendered characters: on a continuation row the +real overage against the derived `Ѹ` 616 ceiling is 36 chars / **4 lines / 44.8pt**, not 34 / 3 / +33.6. State which quantity the guard pins when #1950 lands. diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index 65a674ab1..2560ea5e8 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -32,6 +32,25 @@ core formula against the original line by line — that divergence is where the `splitByDepositsExcludingTagged` (PR #1894), where the residual expression was the sole difference and the sole defect. Prefer an options flag over a fork; when a fork ships anyway, file the collapse follow-up. +### Forked _test harness_ — `realRender.test.ts` re-implements merge.ts's docDefinition + +`renderOverviewPdfContent` (`client/src/lib/reportPdf/realRender.test.ts` ~L136-159) hand-copies +production's pdfmake `header:`/`footer:` callbacks while its own docstring claims parity with merge.ts +("never hand-copied — #1929 AC11"). It imports `pageMargins`/`styles` but forks the callbacks. PR #1982 +changed `merge.ts`'s header string and left the harness on the old expression, so every multi-page +real-render test (incl. the 3-page long-`sourceName` clipping test) measures a string production no +longer emits. **Whenever `merge.ts`'s docDefinition changes, grep this helper.** Fix direction: pass +`content` and build the same string, rather than re-deriving it. + +### Proxy bound looser than the production threshold it guards + +PR #1982's AC7 tests bound DE header labels at `floor(width / 5.19pt)` (an _average_ glyph advance) — +8/9 chars — while production's own break trigger is `safeTokenChars(width, HEADER_WORST_CASE_CHAR_WIDTH_PT += 10.4pt)` = 4 chars. An 8-char wide-glyph label passes the test and still breaks in the PDF. When a test +re-derives a width/size bound instead of importing the production constant, check which direction the +error runs: a bound _looser_ than production's greenlights the regression it exists to catch. The real +guard there is the renderer-level `positions.length === 1` assertion. + ## Test smells worth escalating in review - A combined-path test that places the two interacting entities on **different** parents proves nothing @@ -404,7 +423,77 @@ constraint is enforced by a comment instead of by the type**: Rule: **delete rather than comment.** A write path with no reader and no producer is how the #1929 round-3/round-4 confusion started — the code said one thing and the comment said another. When triaging a -"keep it as a capability?" question, check for a *producer* first: no producer => dead, remove it. +"keep it as a capability?" question, check for a _producer_ first: no producer => dead, remove it. + +## Reinstating a removed producer breaks the negative guards left behind (#1965 / PR #1979) + +The exact inverse of the pattern above, and it bites one story later. #1959 removed the `content.footnotes` +producer and left **negative-only guards plus a prose directive**: `ReportWizardPage.ts` said "NOTHING +populates `content.footnotes` … Never assert a positive count on these", and Scenario 18 asserted +`footnotesBlock/footnoteItems` count 0 + the sentence absent from `main`. #1965 restored the producer with a +3-line push in `buildReportContent.ts` — and silently turned a green 3-viewport E2E scenario red. + +**Review rule for any "reinstate / re-enable X" PR:** `grep` the whole repo (especially `e2e/pages/*` and +spec-file header docstrings) for assertions and _directives_ that pin X's absence. A PR that adds a producer +without inverting those is incomplete, and because `E2E Gates` is `main`-only it merges green into `beta` and +surfaces only at promotion (see [[merge-gate-vs-done-gate]] / the beta-merges-past-red-E2E trap). + +Corollary: a stale POM docstring is worse than a stale code comment — it is an instruction later agents obey. +Inverting the E2E assertion usually also discharges the story's "count occurrences in the rendered DOM" AC, so +it is the same edit, not extra work. + +## Staleness tokens: the `finally` block is the hole (#1946 / PR #1977) + +A monotonic-token guard (`if (ref.current !== token) return;` in `.then`/`.catch`) does **not** protect a +`finally` block — `return` inside `try` still runs `finally`. So `finally { setIsLoading(false) }` lets an +_abandoned_ request clear a flag that a _newer_ request now owns. + +PR #1977 shipped exactly this in `runAiGeneration`: discard-confirm bumps the token and clears +`isGeneratingAi`; the user starts a second generation; the first one resolves ~seconds later, bails at the +token check, and its `finally` stops the second one's spinner, re-enables the trigger button, **and drops +`isGeneratingAi` out of `guardedUpdate`'s dirty predicate — reintroducing the very bug the PR fixed**, one +discard later. Note the `finally` was safe _before_ the token existed (the button gated concurrency), so this +is a defect introduced by widening the lifecycle without widening the flag's ownership check. + +**Review rules for any staleness-token PR:** + +1. Read the whole `try/catch/finally`, not just the two guard lines. Every side effect in `finally` needs the + same `ref.current === token` condition. +2. Ask "can a _second_ request now overlap the first?" A token bump usually re-enables a trigger that + concurrency was previously gated on. Demand a test that starts request B while A is still pending and + asserts A's arrival changes **nothing** — the new-describe blocks in these PRs test only A-alone. +3. Anything derived from the flag (elapsed-seconds timers, disabled states, and especially **dirty + predicates**) inherits the bug. Enumerate the flag's readers: `grep -n ` and check each. +4. `reportRequestRef` in the same file is the clean template precisely because it has no `finally`. + +Related: the conditional `if (isGeneratingAi)` inside a `pendingChangeRef` closure reads the value captured +at guard time, not at confirm time. Idempotent invalidation (always bump, always clear) removes the +stale-closure reasoning for free — prefer it. + +**Resolved in round 2** (`83afc72f`): `finally { if (aiGenerationTokenRef.current === token) setIsGeneratingAi(false) }`, +plus a two-controlled-promise test (start A, discard, start B, resolve A → assert still-disabled, resolve B → +assert content). Rule 2's "demand a test that starts B while A is pending" is what produced that test — keep asking. + +## Discard/confirm dialogs: conditionalize the title, not just the body + +Same PR: the body got an accurate in-flight variant, the title stayed `"Discard your edits?"` in the case +where no edits exist — and the AC5 test asserted that title, pinning the inaccuracy. When an AC says "copy +must not claim edits exist that do not", the title is copy and it is the most prominent line. Check every +string in the modal, and check the test isn't locking in the wrong one. + +Round-2 fix pattern worth reusing: select the title with the **byte-identical predicate expression** already +used for the body, not a re-derived equivalent — a copied-and-tweaked predicate is exactly how the two drift +apart again. + +### A conditional modal title breaks E2E page-object dialog locators + +Playwright POMs address dialogs by accessible name (`page.getByRole('dialog', { name: 'Discard your edits?' })`, +`e2e/pages/ReportWizardPage.ts`), so making a title conditional silently narrows that locator to one branch. +In PR #1977 nothing broke — the only E2E usage opens the modal _after_ generation resolved, so the old title +still renders — but the POM docstring now documents the title as unconditional, and the next test that opens +the modal mid-generation will fail to find the dialog. **Whenever a PR conditionalizes any modal/heading string, +grep `e2e/pages/` for the literal** and flag the locator + docstring as an e2e-test-engineer follow-up. Same +family as the cross-reference-rot entry: a POM docstring is a contract surface, not a comment. ## Verify the AC record when a PR reverses a recently-shipped story (PR #1959) @@ -416,5 +505,1080 @@ Tell: the reversing issue had **zero comments**, no `**[product-owner]**` header a PR-style summary), and no ux-designer visual spec — whereas the story it reversed had all four. **A requirements reversal authored as a polish issue is the signature.** Cheap fix: PO supersession comment on the old issue + PO ratification on the new one. Check this whenever a PR deletes user-visible report/document -content — and check whether the replacement text preserves *meaning* (`(abzgl. Abschlag)` lost the footnote's +content — and check whether the replacement text preserves _meaning_ (`(abzgl. Abschlag)` lost the footnote's "claimed separately", which is compliance-relevant in a bank-facing document). + +## Enumerated multi-site doc fixes come back half-done (PR #1979 r2) + +When a review finding names N sites for the same stale claim, expect the fix commit to update the _nearest_ +ones and miss the rest. #1979's HIGH 2 named four sites for "nothing populates `content.footnotes`"; the fix +updated the field-declaration comment and the spec header (both adjacent to the changed assertions) and left +the two class-docstring paragraphs — which contained the strongest form ("they can never be populated by the +current code path, and any test asserting a footnote `
  • ` is asserting a superseded design"). + +Two habits that follow: + +- **Re-grep the literal on re-review**, never trust the fix commit's diff to cover the enumeration. One + `grep -n -i footnote e2e/pages/ReportWizardPage.ts` found both misses instantly. +- **Check the test _name_, not just the body.** #1979 inverted Scenario 18's assertions to `toHaveCount(1)` + but left the Playwright title reading "and no footnote list anywhere on the page". A title that states the + inverse of its body is worse than a stale comment: it renders that way in every CI report and is the first + artifact a future reader uses to conclude the _body_ drifted. Same for the `// Scenario NN:` block header. + +Why this is worth blocking on (I did, r2): the POM class docstring is the contract the spec header points at +("See `ReportWizardPage.ts`'s class docstring for the full locator reference"), so a directive there plus a +lying test title is a complete instruction set for deleting the coverage the PR exists to add — and with +`E2E Gates` main-only, that deletion lands on `beta` silently. It is the same mechanism that produced #1965: +#1959 removed a producer and left comments asserting the removal was permanent. + +## Amount-threshold booleans silently narrow status-existence booleans (PR #1984, #1897) + +When a deposit-blind SQL helper is collapsed into the shared deposit-aware path, the _money_ fields +(`actualCost`, `actualCostPaid`) port cleanly but any **boolean** flag does not. #1984 re-derived +`hasClaimedInvoice` from `actualCostClaimed > 0` where the old SQL used +`COUNT(CASE WHEN i.status = 'claimed' ...) > 0`. Those are different predicates: + +- **Gains** the intended case (pending invoice + claimed deposit → `true`). +- **Loses** a claimed invoice whose deposits fully cover it with non-claimed status: `residualFraction` + is 0 in `splitByDeposits`, so the claimed residual contributes nothing and the flag flips to `false`. +- **Loses** a refund-neutralised claim (refunds carry a negative fraction, netting the bucket to 0). + +Fix shape: derive booleans from statuses on the raw rows, never from a post-split amount — +`rows.some((r) => r.invoice_status === 'claimed' || r.deposit_status === 'claimed')`. Strict superset of +the old predicate, threshold-free, so rounding and refunds cannot flip it. + +**Why this is worth blocking on**: the flag's only consumer was `MassMoveModal`'s `claimedCount`, which +gates the "I understand" confirmation before mass-moving bank-claimed lines. A display-parity bug fix +quietly disabled a safety confirmation. Generalise: before accepting a boolean's re-derivation, find its +consumers — if any is a guard rail rather than a label, demand predicate equivalence, not "the tests pass". + +**Companion test smell**: every pre-existing claimed-invoice test used an invoice with **no deposits**, so +`residualFraction === 1` and the two predicates coincide. A whole suite can agree with a wrong predicate +because no case exercises the branch where they differ. Ask "which fixture makes the old and new +definitions disagree?" and require exactly that fixture. + +## Prettier is not CI-gated — the local gate is the only gate + +`static-analysis` in `.github/workflows/ci.yml` runs `npm audit signatures`, `npm run typecheck`, and +Stylelint. No `format:check`, no ESLint. So `npx prettier --check ` on review is worth the +ten seconds: #1984 shipped two violations (a 101-char inline return type, and a rider edit left +artificially wrapped after the expression shortened) that nothing downstream would have caught. + +## Positive membership on a _shared fixture_ row is not a filtering assertion (#1971, PR #1985) + +The stock "fix" for `expect(rows.length).toBeGreaterThan(0)` is to add +`expect(await getUserRow(TEST_ADMIN.email)).not.toBeNull()`. That closes nothing: the shared admin/fixture +row is present in the **unfiltered** list too, so the assertion passes verbatim when the filter is a no-op. +A filtering assertion needs one of: + +- a **universal negative** — loop every rendered row and assert it contains the query, or +- a **seeded non-matching row** asserted absent (the shape the `filters by email` rewrite in #1985 got right). + +Watch for the comment that ships alongside it claiming the new positive check "makes the `> 0` guard +meaningful" — a documented-but-false guarantee is worse than the bare `> 0`, because the next maintainer +stops looking. Block on the comment/code conflict even if the assertion itself is a mild improvement. + +Detail that bites when writing the universal-negative loop: `UserManagementPage.tsx` filters on +`displayName || email`, so assert on `` `${cells[0]} ${cells[1]}` `` — a name-cell-only check produces false +failures for rows that matched by email. Join the cells with a **space**: a query with no space in it cannot +then be matched by bridging two adjacent cells, so no false positives. + +Ranking the two remedies (settled on PR #1985 round 2, APPROVED): the universal-negative loop is only +discriminating when the table happens to contain a non-matching row. `e2e/playwright.config.ts` sets +`fullyParallel: true` across 16 shards and `e2e/fixtures/seed.ts` seeds only the setup admin, so a test can +land in a shard whose user table is nearly empty and a broken filter still passes vacuously. Treat the loop +as sufficient-to-approve (it can no longer pass while wrong rows render) but the **seeded non-matching row** +as the airtight form; ask for it as a follow-up, not a block. + +Positional cell indices (`cells[0]`/`cells[1]`) are coupled to `useColumnPreferences(pageKey, columns)`, +which persists both visibility **and** order. No E2E test toggles columns on `/settings/users` today and the +POM's `getUserRow` already assumes `td` nth(1) === email, so it is currently consistent — but a future +column toggle silently repoints those loops at role/date text. Prefer POM accessors resolved from header +text when this comes up again. + +E2E-only PRs: `Detect Changes` skips Static Analysis, unit shards, and Trailer Check, and `Quality Gates` +runs smoke only — so the changed spec's real result lives in the 16 `E2E Tests (Shard n/16)` runs, which are +non-gating on beta. Always tell the orchestrator to confirm the relevant shard is green on **that PR** before +merging (see MEMORY.md's "Beta merges past red E2E"). + +## E2E user "cleanup" never frees the email — DELETE /api/users is a soft delete + +`server/src/routes/users.ts` DELETE sets `deactivatedAt`; `userService.listUsers` returns deactivated rows +and the user-management page applies no default status filter. So `deleteUserViaApi` leaves the row visible +for the rest of the run, and `POST /api/users` still 409s on that email (`findByEmail` does not exclude +deactivated users). Consequences for review: + +- A `finally`-block "delete" comment claiming the DB is left clean is wrong — say _deactivated_. +- Deterministic seed emails (`${testPrefix}@…`) are one-shot per DB. Currently masked because Playwright + gives a retried test a fresh `workerIndex` (so `testPrefix` differs), but `--repeat-each` or a switch to + `parallelIndex` would make `createLocalUserViaApi`'s `expect(response.ok())` fail and mask the real + failure. Require `${testPrefix}-${Date.now()}@e2e-test.local` (precedent: `i18n-categories.spec.ts`). +- `deleteUserViaApi` ignores the response status, so cleanup failures in this family are always silent. + +## `page.route()` matcher traps in e2e specs (PR #1986) + +Two independent ways a route-interception assertion becomes vacuous, both invisible to CI: + +1. **`API` is an object map**, not a string (`e2e/fixtures/testData.ts:39`). ``page.route(`${API}/users/me/preferences`)`` interpolates to the glob `[object Object]/users/me/preferences`, which matches nothing — so `expect(captured).toHaveLength(0)` passes forever. The repo convention is `` `**${API.}` `` (property access + `**` prefix); `reportWizardEditableContent.spec.ts:969,998` do it right. ESLint's `restrict-template-expressions` would flag it but **CI runs no ESLint** (`static-analysis` = `npm audit signatures` + `typecheck` + `Stylelint` only). +2. Any **negative** route assertion (`toHaveLength(0)`, `not.toHaveBeenCalled`) is indistinguishable from a broken matcher. Always require the author to prove the matcher fires once (assert `1` against a deliberate request, then invert) before accepting the guard. + +## "Runs at all three viewports" is false unless the test is `@responsive`-tagged + +`e2e/playwright.config.ts`: `tablet` (iPad gen 7, 810px, webkit) and `mobile` (iPhone 13, 390px, webkit) +projects both set `grep: /@responsive/`. An untagged test runs **desktop only** — reject any AC/docstring +claiming multi-viewport coverage without `{ tag: '@responsive' }`. + +Adding the tag is not a free fix when the component has a **dual layout in the DOM**: `ReportContentEditor` +renders both a `` and a `.mobileCardList`, CSS-gated at `@media (max-width: 767px)`. `display: none` +drops the table from the a11y tree, so `getByRole('columnheader')` is 0 at mobile _regardless of state_ — +`toHaveCount(0)` passes vacuously and `toHaveCount(1)` fails. Layout-dependent assertions must branch on +viewport (assert `.mobileCardRow` captions at mobile). Scenario 1b in that spec is the precedent guard. + +Related smell from the same PR: a test **title** naming behavior the body never asserts ("reset on remount", +"`
    ` cells" when only `` is checked) — a coverage illusion; trim the title or add the assertions. + +### Accessible-name locators: what they are and are not immune to (#1966 round 3) + +`getByRole(..., { name })` computes the name from **DOM text**, so it is immune to CSS `text-transform` — +the opposite of `innerText`/`toHaveText` assertions, which fail on transformed labels. Prefer the role+name +form when a component may style its casing. + +Two follow-on facts worth reusing: + +- An embedded control inside a name-from-content traversal contributes its **value**, not its `aria-label`. + So an `EditableField` whose `ariaLabel` interpolates a neighbouring column's text cannot inflate the + containing cell's accessible name (and `exact: true` guards even if it could). +- `role=cell` / `role=columnheader` exposure depends on the table keeping table semantics — a `display: block` + or `display: flex` on the `` strips them in Chromium and silently zeroes such locators. Before + trusting a new `cell` assertion, confirm a sibling `columnheader` assertion already passes in CI; both rest + on the same exposure. + +Absence assertions need a **positive baseline in the same test** (`toHaveCount(1)` before, `toHaveCount(0)` +after). Without it, a typo'd or mis-scoped locator makes the absence check pass on nothing. With it, every +mis-scoping fails loudly instead — that property is the review bar, not the assertion count. + +### Single-occurrence guard tests prove nothing about delimiter pairing (#1952, PR #1987) + +A "false-positive guard" test that feeds the sanitizer **one** unpaired delimiter cannot detect that the +regex pairs up **two** unpaired ones. `stripMarkup`'s guards were `'value_field without close'` (one `_`) +and `'Price: 5 EUR* (VAT incl.)'` (one `*`) — both green while `budget_line_id` -> `budgetlineid`, +`RE_2024_117` -> `RE2024117`, and `'5 EUR* … 10%* …'` -> both stripped. When reviewing any +strip/sanitize/unescape regex, the question is not "is there a guard test?" but **"is there a guard test with +two or more of the delimiter on one line?"** + +The fix is CommonMark's flanking rules, and they are the right reference for any markdown-ish stripper: +opening delimiter must be followed by non-space, closing preceded by non-space, and `_` must additionally +not be intraword (CommonMark disables intraword `_` emphasis precisely because of snake_case and e-mails): +`/(?=2 consecutive numbered lines** +(a lone marker is an ordinal, not a list). Bullet markers (`- `/`* `/`+ `) stay unconditional — a leading +`- ` is not idiomatic prose. Document the asymmetry in the JSDoc so a later reader does not "harmonize" it. + +### Two ACs in direct tension, silently resolved (#1952 AC 1.2 vs AC 2.5) + +AC 1.2 mandated stripping `1. `/`1) `; AC 2.5 mandated compliant plain prose pass through **byte-identical**. +These cannot both hold for German prose. The implementation picked 1.2 without recording the trade-off. +When an issue states a preference ordering in prose ("conservative stripping matters more than exhaustive +stripping… when in doubt, leave the text alone"), that prose **is** the tie-breaker — read the issue's +narrative sections, not just the checkbox list, before accepting an AC-satisfying implementation. + +### Validate a proposed regex fix before writing it into the review + +For any non-trivial regex fix spec, transcribe the current + proposed implementation into a throwaway +`/tmp/*.mjs`, and assert the proposed version against (a) every existing test case, (b) the new false +positives, and (c) the issue's Verification scenarios. PR #1987's spec was validated 45/45 this way, which +turns "here is a suggestion" into "here is a drop-in that breaks no existing test" — the difference between +one fix round and three. + +### Strip-order tests need a `toBe`, not just a `toHaveLength` + +To pin "strip runs before truncate", the boundary fixture `'**' + 'X'.repeat(limit) + '**'` yields a +`limit`-length string under **both** orderings (`'X'.repeat(limit)` vs `'**XXX…'`). Only the `toBe` assertion +discriminates. Good pattern to reuse; also a reminder that a length assertion alone is often vacuous. + +### A bumped submodule ref is not a pushed wiki commit (PR #1987) + +PR #1987 had the parent ref bumped to a wiki commit that was **never pushed** — the wiki remote was two +commits behind. `git -C wiki log --oneline` shows the commit as HEAD, so the wiki _looks_ published, and +`git ls-tree HEAD wiki` matches it, so the ref _looks_ correct. Anyone cloning the branch and running +`git submodule update` would fail on an unresolvable ref. + +Verify with `git -C wiki ls-remote origin master` compared against `git ls-tree HEAD wiki` — those are the +only two facts that matter. **Do not** trust `git -C wiki fetch origin master` here: without an explicit +refspec it only writes `FETCH_HEAD` and leaves `refs/remotes/origin/master` stale, which initially made the +remote look already-current. Use `git -C wiki fetch origin master:refs/remotes/origin/master`, or `ls-remote`. + +Add this to every PR review touching `wiki/`. "The ref is bumped on the branch" is a weaker claim than it +sounds — I asserted AC 4.1 satisfied on that basis before catching it. + +### Shared worktrees: never `git add -A` (PR #1987) + +`fix-1913-1952-server-tests` had ~48 dirty files from concurrent agents plus the known repo-wide prettier +union-type drift (`shared/src/types/{dependency,diary,document,subsidyProgram}.ts` — the same four every +time). Stage explicit paths only: for a wiki/ADR change that is `git add wiki .claude/agent-memory/` +and nothing else. A scoped ref-bump commit does not disturb an implementer mid-edit in the same tree. + +### Shell heredocs: a bare `cat >> file` with no redirect hangs the tool + +`cat >> a.md 2>/dev/null || true` followed by a second `cat >> b.md <<'EOF'` — the heredoc binds to the +_second_ cat, so the first reads stdin and blocks until the 120s timeout. Prefer the Edit/Write tools for +appending to memory files; if you must use bash, one heredoc per command and never a redirect-less `cat`. + +### `Pick` is not a forcing function (#1947 action-set review) + +A reducer "tier factory" typed `function freshTier(): Pick` claims to make +"what does this transition clear" a compile-time decision. It does not: adding a field to `State` +produces **no error** — the key union just doesn't mention it, the spread leaves it untouched, and it +silently defaults to _kept_. The key union is a second hand-maintained list, i.e. the very thing being +replaced. The working version is a **named tier type** (`interface ReportTier {...}`) whose factory has an +**explicit return-type annotation** and returns a total object literal — missing property = compile error. +The annotation is load-bearing: an inferred return type re-derives the shape from the literal and the +error vanishes. Partition state as a flat intersection of tiers, not nested objects (nesting churns every +read site). Generalises to any "exhaustive mapping" claim made with `Pick`/`Omit`/`Record`. + +### Caller-supplied monotonic seq in an action payload reintroduces the ref it replaces (#1947) + +`dispatch({type:'SELECT_SOURCE', payload:{ newReportSeq }})` asks the caller to produce a value that must +stay **in sync with reducer-owned state** — only achievable with an out-of-reducer counter ref, so the +"staleness is enforced in the reducer" claim is false. Fix: **opaque nullable token** (`requestId: string | +null`) used as identity, never ordering — caller generates via a module-level `nextRequestId()`, echoes it +back in the completion action, reducer no-ops on mismatch. `null` then means "nothing in flight, discard +every outstanding response", so a reset invalidates in-flight work with no bump arithmetic. Monotonicity is +never needed when nothing compares generations for order. Corollary: an in-flight **boolean flag** +(`isGeneratingAi`) alongside such a token must be **derived** (`token !== null`), never stored — the two +disagreeing is exactly the bug class the token exists to kill. + +### A refactor's cascade table smuggles behaviour changes (#1947) + +Diff every row of a proposed reset/cascade table against the actual handler line-by-line. Two of three rows +in #1947's table cleared `aiError` where the code does not: one handler never clears it, and the other +clears it only inside `if (isGeneratingAi)`. Both were reachable, user-visible, and would have landed inside +a PR whose stated AC was "no user-visible change". Also watch for **generic setter actions** +(`SET_MAX_STEP`) — a setter wearing an action's clothes preserves the ad-hoc call it was meant to replace +and names nothing about what it invalidates. And check whether the _unfixed_ instances of the same race +exist elsewhere in the file (#1947 had a third, unguarded, in the Step-2 fan-out fetch). + +### The neutralised trigger left in the code (#1947 `deepLinkAppliedRef`) + +When a defect's trigger condition is _neutralised by a new guard_ rather than removed, the guarantee lives +in a comment. `if (… && !report && !appliedRef.current)` — `!report` was the AC8 trigger, kept alive behind +a ref and a nine-line comment. Removing the redundant condition also removes `report` from the effect's +dep array, making "clearing report cannot re-fire this" structural. Look for this shape in any fix that +_added_ a guard without deleting what it guards against. + +### A total-object tier factory only forces a decision in the cases that spread it (PR #1988 review) + +Follow-up to the `Pick<>` entry above: getting the factory right is necessary but not sufficient. A named +tier type + annotated total-literal factory produces the compile error, but **any reducer case that +hand-lists that tier's fields instead of spreading the factory keeps the hole** — the new field silently +defaults to _kept_ there. PR #1988 had `freshContentTier()` correct and then bypassed it in `SELECT_SOURCE` +and `DISCARD_EDITS`, the two cases that clear content state, because each needed one field _preserved_ +(`aiError`). Reviewing a tier-factory design: grep every case for the tier's field names appearing as +literal keys; each hit is an unenforced case. The fix is always the same shape — spread the factory, then +name the exception on the next line (`...freshContentTier(), aiError: state.aiError`), which is +behaviour-identical and makes the KEEP the thing that is written down rather than the CLEAR. + +## Hand-rolled regex mirroring a third-party grammar accepts values the library rejects (PR #1989, #1970) + +Validating an env var with a regex that _approximates_ a library's parser produces configs that pass +startup validation and then blow up at request time. Concrete case: `AUTH_RATE_LIMIT_WINDOW` validated +by a bespoke duration regex, while `@fastify/rate-limit` v11 parses `timeWindow` strings with +**`@lukeed/ms`** (not the classic `ms` package). + +Two divergences found, both reaching the same failure: + +- **Zero magnitudes.** `@lukeed/ms` guards with `if (arr != null && (num = parseFloat(arr[1])))`, so + `'0s'`, `'0 minutes'`, `'0ms'`, `'0.0h'` all `parse()` to `undefined`. +- **Whitespace class.** `@lukeed/ms` uses ` *` between number and unit; `\s*` additionally accepts + `'15\tminutes'` / `'15\nminutes'`, which `parse()` rejects. + +Why it's fatal, not a fallback: `mergeParams()` in `@fastify/rate-limit/index.js:163-169` is an +`if / else if` chain — a _string_ takes branch 2, gets `undefined`, and never reaches the +`defaultTimeWindow` branch. At request time `await params.timeWindow(req, key)` throws, so **every** +request to the route returns `500 {"message":"params.timeWindow is not a function"}`. Verified: +`timeWindow: '0s'` on a route → 500. A zero window is the obvious way an operator tries to disable a +rate limit, so this defeats a "no value may disable the control" acceptance criterion. + +**Rule:** when a config value is handed verbatim to a third-party parser, validate it _with that +parser_ (declare the dep) rather than re-deriving its grammar. If a regex is unavoidable, also assert +the parsed result is defined and `> 0`, and check the library's actual source for which package it +uses — `@fastify/*` deps are not always the popular one. + +**Resolved in `47ee190` (APPROVED)** with regex + guard rather than delegating to the parser, and that +was accepted. Two transferable lessons: + +- **Direction of divergence is what matters, not divergence itself.** A hand-rolled regex that is + strictly _narrower_ than the library is fail-closed and fine: config rejects `1y`, `1wk`, `100msec`, + `.5s`, `-5m` at startup with an actionable message. Only the _wider_ direction (config accepts what + the parser chokes on) is a blocker. Don't demand exact grammar parity in review — demand that the + accept-set be a subset, then sanity-check that the excluded values are ones nobody wants. +- **Verify a grammar claim by brute force, not by reading.** Cross-checking "does anything pass my gate + that the library can't parse?" over all units × magnitudes × separator widths took one throwaway + script and turned an inspection argument into `config-accepts-but-lukeed-fails: NONE`. Import the + library's built file by relative path (`./node_modules//dist/index.mjs`) from a script placed in + the repo root — a script in `/tmp` cannot resolve the bare package name. +- Ordering detail worth preserving: the positive-magnitude guard must be an `else if` _after_ the + pattern test, so `parseFloat` only sees strings already known to start with `\d+`. Reversing them + reintroduces a `NaN` path. + +## `parseInt` config validation accepts trailing garbage repo-wide + +`loadConfig()` uses `parseInt(str, 10)` + `isNaN` for every numeric env var (`PORT`, +`SESSION_DURATION`, `PHOTO_MAX_FILE_SIZE_MB`, `LLM_MAX_TOKENS`, `BACKUP_RETENTION`, +`AUTH_RATE_LIMIT_MAX`). So `20abc` → `20`, `20.9` → `20`, `1e3` → `1`, despite error messages that say +"must be a positive integer". Any AC demanding "non-numeric value fails startup" is only partly met. +Don't request a one-variable fix in review — it creates local inconsistency; either accept the pattern +or propose a uniform `/^\d+$/` guard across `loadConfig()` as its own item. + +## Env vars are documented in four places, not one + +Adding an env var means: `CLAUDE.md` table, `wiki/Architecture.md` (topic-grouped tables — e.g. +"Authentication & Sessions" ~L393), `wiki/API-Contract.md` ("Environment Variables (Auth)" ~L107), and +`docs/src/getting-started/configuration.md` (**docs-writer-owned** — file a request, don't edit). +The first three belong in the implementing PR with the submodule ref bumped on the branch. PR #1989 +updated only `CLAUDE.md` at first review; `47ee190` added both wiki pages, leaving the docs-writer one +as a release-staging follow-up — that is the correct end state, so treat "3 of 4 + a flagged follow-up" +as the passing bar, not 4 of 4. + +Cheap way to find every location when adding a var: grep an _existing_ comparable var repo-wide +(`grep -rln SESSION_DURATION --include='*.md' --include='*.yml' .`) instead of guessing which files +need touching. It also surfaces the ADR pages that pin a default. + +## A guard deleted because it looked like the bug (#1303 -> #1995, PR #1998) + +CVE-2026-15144 was caused by a custom `keyGenerator` in `rateLimitPlugin.ts` bypassing +`@fastify/rate-limit` 11.2.0's IPv6 /64 normalization. The fix deleted the option — correct +direction, but the deleted block had been added deliberately by 69d90882 (#1303) as a nullish-IP +guard, with **zero test coverage**, which is exactly why deleting it looked free. Before approving +any deletion framed as "this override was a mistake", run `git log -S` on the removed lines and read +the commit that introduced them. `git show --stat` touching no test file is the tell that the +behaviour is unguarded and its removal will pass CI. + +Two library facts worth keeping: + +- **`@fastify/rate-limit` gates normalization on an _identity_ check**, `index.js:249`: + `params.keyGenerator === defaultKeyGenerator ? defaultKeyGenerator(req, subnet) : keyGenerator(req)`. + So even a custom generator written as literally `(req) => req.ip` bypasses /64 normalization. A + custom generator is only safe if it calls the library's exported `normalizeIP` itself + (typed export, `types/index.d.ts:164`; works as an ESM named import). +- **`normalizeIP(undefined)` throws** (`ip.toLowerCase()` on line 1), and Fastify's `request.ip` is + typed `string` but nullable at runtime in **both** getters (`fastify/lib/request.js:231` + non-trustProxy — `undefined` socket or destroyed-socket `remoteAddress`; `:110` trustProxy — + empty `proxyAddr.all()`). 11.1.0's default returned `undefined` (shared bucket, mild); 11.2.0 + turned the same input into a throw -> 500 on every rate-limited route. Another types-lie: + `tsc` cannot see it, so a nullability regression is invisible in the diff. + +Generalisation: when a minor version replaces a "return the raw value" default with a "transform the +value" default, any guard written against the old milder failure mode may now be load-bearing against +a throw. Check the version-bump PR's semantics, not just the option's name. + +## A CVE fix's shared-bucket test needs a negative control + +"These two IPv6 addresses share a bucket" passes identically if the key collapsed to a constant for +_all_ clients — which is a self-inflicted DoS (one attacker locks out every user), i.e. the opposite +defect. Upstream's own `test/ip-normalization.test.js` asserts the third case: a _different_ /64 gets +a fresh bucket. Same family as "assertions that pass on nothing", but subtler — the test does catch +the regression it was written for, and only fails to catch the over-correction. + +## Prettier config resolution is path-based — "was it clean before?" checks must run inside the repo + +Copying a file to `/tmp` and running `npx prettier --check` on it silently uses prettier **defaults** +(printWidth 80), not the repo's `.prettierrc` (100). During the #1998 wiki pass this made a dirty file +look clean and a clean file look dirty — the opposite of the truth, in both directions at once. +Verify baselines with `git show HEAD: > /_chk_` **inside the working tree**, then delete. + +Related: `wiki/` is **not** in `.prettierignore` and `npm run format` globs `**/*.md`, so a repo-wide +format touches wiki pages — including `Security-Audit.md` (security-engineer-owned) and ADR pages. +Before committing a wiki edit, run `git -C wiki status --short` and revert any page you did not +intend to touch; scope your own formatting to the files you edited. + +## Documented "absence of code" is falsified by the next commit — document the invariant instead + +PR #1998 fixed CVE-2026-15144 by **deleting** a `keyGenerator` override, so the natural wiki sentence +is "`rateLimitPlugin` deliberately sets no `keyGenerator`". That documents an absence: it goes stale the +moment anyone adds a _correct_ override, and it gives a reviewer no rule to check the new code against. +Found live during this pass — an uncommitted working-tree change already reintroduced +`keyGenerator: (request) => normalizeIP(request.ip ?? 'unknown')`, which is safe (normalizeIP defaults +`ipv6Subnet = 64`) yet contradicted the sentence. + +Rule: state the **invariant** ("the key must always be `normalizeIP`-normalized, forwarding the +configured `ipv6Subnet`"), then note the current mechanism as the preferred way of satisfying it, then +enumerate the specific forbidden shapes. Applies to any fix whose diff is a deletion. + +**Confirmed within the same PR (#1999).** The absence-sentence was falsified before the PR even merged, +including the "Cornerstone does not override `ipv6Subnet`" clause and both Deviation Log rows that cited +"the deliberate absence of a `keyGenerator`" as rationale — a self-contradicting PR caught only at +review. A Deviation Log row is not append-only history while its PR is still open: amend the Resolution +cell (framed as "the first pass said X; PR #N invalidated that, because …") rather than stacking a +second row about an unmerged one. Also: an override can be _mandatory_ rather than stylistic — here two +library facts force it, so "prefer the library default" was wrong advice, not merely stale. + +## Verify library internals against the pinned tarball, not `node_modules` + +The base checkout's `node_modules/@fastify/rate-limit` was **11.1.0** while the lockfile and +`server/package.json` pin **11.2.0** — and 11.2.0 is the version that introduced `normalizeIP` and the +generator-identity gate. Grepping the installed copy showed _no_ `normalizeIP` at all, which reads as +"the claim in the code comment is false" instead of "the install is stale". Worktrees have no +`node_modules` of their own, so this is the default situation, not an edge case. + +Rule: before documenting or refuting a claim about a dependency's internals, check the installed version +against the lockfile pin. If they differ, `cd /tmp && npm pack @ && tar xzf …` and read that +source. Cheap, exact, and it produced the file/line citations (`index.js:14` `defaultIPv6Subnet = 64`, +`:249-251` identity gate, `:33-34` `ip.toLowerCase()` null deref) that the wiki text now rests on. + +## A config option consumed only inside a bypassed branch is inert, not redundant-but-harmless + +`rateLimitPlugin` passes `ipv6Subnet: IPV6_SUBNET` to `@fastify/rate-limit`, but 11.2.0 reads +`params.ipv6Subnet` **only** inside the identity gate (`index.js:250`, the `keyGenerator === +defaultKeyGenerator` arm). With a custom generator set, that option can never influence a key. Keeping +it is still correct — it becomes load-bearing the moment the override is deleted — but it is a _latch +for a future state_, not the mechanism doing the work today. + +Rule: when reviewing "the constant is shared so the two cannot drift" rationales, check whether the +second consumer is actually reachable. If it isn't, say so in the code comment ("intentionally redundant +while the override exists"), otherwise the next reader sees the same value configured twice, believes the +option delivers the behaviour, and deletes the explicit call as duplicated config — reintroducing the +very bug. Related: a single constant referenced twice cannot "drift" at all, so that phrasing in wiki +prose overstates the guarantee it buys. + +## The revert test: a fix that relaxes an invariant and adds no assertion is unobserved + +PR #2002 (#1968) routed the report-PDF usage-cell grey meta suffix through `buildUsageTextRuns` so +per-token `wordBreak: 'break-all'` applies. To let the resulting multiple grey runs through, it relaxed +both `splitUsageCell` test helpers from "exactly one grey run" to "grey runs contiguous at the tail" — +and in doing so made the helpers **synthesize** the meta run (`{ text: , color: GREY }`), +discarding the per-run `wordBreak` flag. Net effect: reverting the production hunk left all 95 + 73 +tests green. The PR body's "all existing tests pass unchanged" was true and was the problem. + +Shape to watch for: **a fix whose enabling step is a loosened assertion.** The loosened assertion is +by construction the one that used to observe the structure being changed; if nothing new observes the +new structure, coverage went _down_ while the diff looked like it went up. + +Rule: for any bug fix, ask "if I reverted just the production hunk, which test goes red?" If the answer +is none, the fix has no regression guard regardless of suite size. Two specific tells: (a) the diff +touches only test _helpers_, never test _cases_; (b) the helper reconstructs a synthetic object from the +real one, silently dropping exactly the property the fix adds. + +Corollary: relaxing an invariant is fine and often correct (contiguous-at-tail still catches interleaved +or mis-coloured runs — it is weaker only in the dimension the fix deliberately changed). What is not fine +is relaxing it _and_ leaving the new dimension unassertable. + +**Round-2 outcome (2026-08-04) — run the revert test yourself; don't grade the description of it.** #2002 +came back claiming H1 fixed. It genuinely was, but I only know that because I re-ran the revert: reverting +the hunk to `runs.push({ text, color: DEPOSIT_NOTE_TEXT_COLOR })` failed all three new tests (`greyRuns.length` +1 not >1; `wordBreak` assertions false), and restoring passed 171/171. The fix was to stop synthesizing — +the helper now returns the **raw** run objects (`greyIndexes.map((i) => runs[i]!)`) so no pdfmake property is +dropped, plus a second test that bypasses the helper entirely and reads the run array off the rendered doc. +Generalizable: when the round-1 finding was "the test cannot observe the fix", the round-2 evidence is a +**demonstrated red**, and that is cheap to produce (one edit, one `-t` jest run) — a prose summary of which +assertions were added is not a substitute, because the whole failure mode was an assertion that looked right. +Two good repair shapes to accept: return the raw object instead of a reconstruction, and add one test that +skips the helper layer under suspicion. + +**Also check the fix's _other_ axis.** `break-all` converts horizontal overflow into extra wrapped lines, +which in this table meets the `dontBreakRows` silent-drop hazard — so an overflow fix can create a height +bug. Here it cannot: `packUsageCellRows` budgets by _character_ count derived from a per-line char count, so +the pre-fix unbroken token used _fewer_ lines than already budgeted and the fix only moves actual behaviour +toward the budget's assumption. Worth asking every time a wrap/break flag is introduced. + +## Broad-scope attribute + partial counter-tagging (PR #2004, #1910) + +An inherited HTML attribute (`lang`, `dir`, `aria-hidden`, `role`) applied to a **container** is a claim about +every descendant. The tempting shape is "tag the container, then counter-tag the exceptions" — and it +ships correct only if the counter-tag list is exhaustive. #1910's AC named three exception classes +("editable-field labels, buttons, headings"); the implementation put `lang={reportLanguage}` on +`ReportContentEditor`'s root and counter-tagged `

    ` + one hint `

    `, leaving six `EditableField` +`

    ` only — so the desktop `` (statusText, splitNote, depositReducedNote, refundNote, +deposit badge) and the _entire_ mobile card tree lost coverage. Net worse than round 2: it traded a +minor over-tag (English chrome read with German rules) for a larger under-tag (German data read with +English rules), and below the 767px breakpoint `.table { display: none }` means zero coverage. +**Why:** an "over-tagging" finding asks you to _relocate or except_ the tag, never to drop it. The +HTML idiom for a nested language exception is **counter-tagging** the inner chrome (`lang={uiLang}` +on the reset button + sr-only hint), not removing the outer boundary. +**How to apply:** when a review round removes an attribute/wrapper, diff the set of leaf nodes that +_were_ covered against those that _are_ covered and demand the delta be re-covered. Two specific +traps here: (a) responsive CSS-only duplicate trees — a fix applied to the desktop table silently +leaves the mobile card list uncovered, and mobile/tablet Playwright projects only run +`@responsive`-tagged tests so E2E won't catch it; (b) a blanket rule like "EditableField labels are +UI chrome" holds only where labels come from `t()` — the mobile usage field's label is +`content.labels.usage`, i.e. report-language, so the rule inverts inside the table region. +Also: the code fix for a round-N finding landing **without an assertion** (the `.readOnlyValue` +`lang` spans) means it can be reverted with every suite green — always ask "what test would fail?" +for each item the author claims to have addressed. + +**Round-3 addendum (#1910, PR #2004) — "the prop landed" is not "the prop is wired".** The fix for a +review finding can introduce a _new_ optional prop, unit-test the prop on the leaf component, thread +it through N call sites, and still have zero coverage of the threading: the leaf tests pass the prop +in themselves. Revert test applied at the call-site level (not the component level) is the only thing +that catches it — delete the `foo={foo}` lines, not the `foo` implementation, and see what goes red. +Optional props make this silent because removing them from JSX is type-legal. + +Companion trap: **a redundant tag that a test asserts.** After restoring an ancestor tag, the +descendant tag it duplicates becomes redundant, and if a test asserts _both_ the redundancy is +locked in. The danger is not the duplication, it is that a later cleanup reads the pair as an error +and removes the ancestor — reintroducing the original finding. Ask for a comment naming the +duplication as deliberate. + +Third: **an `aria-label` cannot be language-tagged.** When an element's accessible name comes from +`aria-label` but its content is in another language, no `lang` placement fixes both — the name is +computed on the element that carries the `lang`. The only exact fix is a visually-hidden span with +its own `lang` plus `aria-labelledby`. Worth naming as a known limit rather than looping on it. + +**Round-4 addendum (#1910, PR #2004) — a positive anchor only pins the call sites the fixture +actually renders.** The round-3 fix for "all 8 `uiLang={uiLang}` props could be deleted with every +suite green" was a test asserting `button[lang="en"]` count `>= 1` plus an all-must-match loop. It +does close the _stated_ gap (deleting all 8 fails), and the handoff claimed "removing **any** prop +fails" — but per-site mutation testing showed **1 of 8** pinned. The fixture put exactly one field +(`coverLetter.sender`) into edited state, so exactly one reset button ever rendered, so the anchor +could only ever cover that one site; the other 7 still delete silently. + +Generalises well beyond `lang`: **`count >= 1` + "all matches satisfy P" is a per-instance assertion +masquerading as a coverage assertion.** It pins the instances the fixture happens to produce, and the +count floor hides how few that is. When N call sites thread a prop, the discriminating shape is +`expect(matches.length).toBe(N)` with a fixture that forces all N to render — an exact count is the +only version that fails when a site disappears. Two review habits that follow: + +- Never accept "removing any X fails" on the strength of an all-at-once revert. Revert each site + **individually** — the all-at-once test passing tells you nothing about per-site coverage. +- When a fix is partial, say which fraction is pinned. "M1 resolved" and "1 of 8 sites pinned" get + recorded very differently, and the second is what stops the gap being re-found in three months. + +Related smell confirmed the same round: a **near-vacuous negative guard** (`button[lang="de"] === 0` +when no button can ever receive `lang={lang}`) is still worth keeping if it pins a _contract_ on +another component ("chrome is always `uiLang`") rather than restating the positive assertion. + +## Untyped E2E route fixtures drift silently from shared contracts (#2005, PR #2006) + +E2E shard 8/16 went red because `mockInvoicesFullSummary()` in `e2e/tests/navigation/dashboard.spec.ts` +returned a `summary` missing two **required** members of `InvoiceStatusBreakdown` (`claimable`, +`quotationCoveredByDeposits`) and used `pagination.total` where `PaginationMeta` says `totalItems`. + +The failure mode is worth remembering because it is maximally indirect. `InvoicesPage` initialises +`summary` to a _complete_ default, so first paint is clean; `setSummary(response.summary)` then swaps +in the incomplete mock and the next render throws on `summary.claimable.count`. React unmounts the +tree, the `loadIntegrationStatus` cleanup sets `cancelled = true`, `integrationStatus.paperless` stays +`null`, and the `?create=1` effect's readiness gate never opens. **Reported symptom: "the New Invoice +shortcut opens no modal."** Nothing in that symptom points at a fixture field name. When an E2E +failure looks like a missing feature or a race, check the mock against the shared response type before +theorising about timing. + +Three durable rules: + +- **Annotate route fixtures with the shared response type** (`function mockX(): InvoiceListPaginatedResponse`). + `e2e/` already imports from `@cornerstone/shared`, so both defects here were compile-time detectable. + An untyped literal handed straight to `JSON.stringify` has _zero_ coupling to the contract it mimics — + adding a required field to a shared type will never break it, which is exactly backwards. +- **A consumer's early return can mask an incomplete fixture indefinitely.** `mockInvoices()` in the + same file also omits `quotation`/`overdue`/`claimable`, and `InvoicePipelineCard` dereferences + `summary.quotation.totalAmount` — it survives only because the card early-returns its empty state + when `invoices: []`. The first person to add one invoice to that fixture reproduces the same + TypeError. "It's been green for months" is not evidence a fixture is complete. +- **Fix the fixture, not the dereference.** The tempting patch is optional chaining in the page. But + `claimable` is _required_ by the contract, so the page is entitled to dereference it unconditionally; + adding `?.` relaxes a correct invariant to accommodate a wrong test. Source-of-truth ordering holds. + +Also load-bearing and undocumented in that file: scenarios 13c–13f register **two** `**/api/invoices*` +handlers (`interceptDashboardApis` then `interceptInvoicesPageApis`), and correctness depends on +Playwright's reverse-registration precedence letting the later full-summary handler win. Swapping the +two `intercept*` call lines silently reintroduces the identical failure. When reviewing Playwright +specs, treat duplicate route globs as an ordering dependency that must be commented. + +Review-craft note from the same PR: verifying "would this test still fail if the feature were removed?" +was cheap here because all three scenarios guard with **positive** `toBeVisible()` assertions. The one +negative (`not.toContain('create=1')`) is only non-vacuous because the preceding positive assertion +can't pass unless `create=1` was present — negatives anchored to a positive in the same test are fine; +negatives standing alone are the ones to challenge. + +## Widen-then-`as`-narrow round trip defeats union exhaustiveness (#2001, PR #2007) + +A field typed as a proper string-literal union gets widened to `string` by an _intermediate_ container +(`SkippedDocument.reason: 'a'|'b'` → `new Map()` in `merge.ts`), then re-narrowed at +the consumer with `reason as 'a'|'b'` plus a `?? reason` fallback. Every symptom of type safety, none of +the enforcement: + +- Adding a third union member produces **zero** compile errors — not at the `as`, not at the hand-written + label literal in `buildReportContent.ts`, not at the interface declaration. +- The `?? reason` fallback is dead code today (indexed access into a fully-keyed object type is + non-optional `string`), so it reads as a guard while guarding nothing — and it is exactly what would + _mask_ the future regression instead of surfacing it. Here it would print a raw i18n identifier into a + bank-facing PDF. + +Fix shape: name the union (`export type ReportSkipReason = …`), type the intermediate container with it, +and declare the lookup table as `Record`. Then adding a member is a compile error at +the single population site. Type the container, don't assert at the consumer. + +**Review heuristic:** any `as '' | ''` in new code is a claim that some _upstream_ +type was needlessly widened. Trace where the widening happened — that's the real fix site. Doubly so +when the PR's stated purpose is "compiler-enforced, not convention-enforced": a cast is convention +re-entering through the back door. + +## A refactor that changes an invariant falsifies the ADR that states it (#2001, PR #2007) + +Removing a hazard _upgrades_ the enforcement level, which means the ADR passages describing the old +weaker level are now actively harmful — they instruct the next contributor to reintroduce what was +removed. ADR-034 had four: a signature quote naming the removed `t` param, a "nothing in the type system +distinguishes X from Y, so the only defences are contract/review/tests" sentence (false once the channel +is gone), a grep guard that cannot see the new violation shape (a _parameter type_ — `useTranslation| +/i18n/` greps miss it), and a list of "sites still doing it independently" that is now empty. + +**Checklist when reviewing any hardening/refactor PR:** grep the governing ADR for (a) quoted +signatures, (b) "the only defence is…" / "nothing enforces…" sentences, (c) enumerated +remaining-violation lists, (d) grep guards — all four go stale from a fix, and none is caught by CI. +Also: the strongest guard is usually the story's own AC grep — promote it into the ADR as a numbered +invariant rather than leaving it in the closed issue. + +Corollary on routing: when the wiki delta is ADR _prose_, request that the orchestrator route it back to +`product-architect`, not to the implementing dev agent. + +## Key-echo test fixtures make same-layer assertions non-discriminating (#2001, PR #2007) + +`reportPdf` test fixtures follow a convention where label values _equal the i18n key strings_ +(`makeLabels()` → `pageLabel: 'sourceReports.table.pageLabel'`), matching the mock `t`'s echo behaviour. +Consequence: an assertion expecting `'… — sourceReports.table.footnoteFetchFailed'` passes identically +whether the production code reads `labels.x` or calls `t('sourceReports.table.x')`. Six such assertions +existed in `overviewPdf.test.ts`; all six contribute zero coverage for the injection contract. + +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. + +## A wiki page can state a PROHIBITION that the PR under review deletes + +**Why:** #1973/PR #2010 generalised `overviewPdf.ts` to 96 column subsets. ADR-034's "Geometry +constraint that blocks a feature" said the PDF column count is fixed at 6 or 7 and that wiring the +wizard's toggles through "is a re-measurement story, not a UI change." The PR touched no wiki file, +so the merged state would have documented the shipped feature as impossible — actively steering the +next agent away from it. Same page, same round: a quoted constant reference (`MAX_SAFE_USAGE_CHUNK_CHARS` +→ per-subset `usageChunkChars`) and a "this function **hangs** on `maxChars <= 0`" claim that a prior +fix had already turned into a throw. + +**How to apply:** on any PR that *removes* a limitation, grep the wiki for the limitation's own +statement — not just for the API/schema surface the diff touches. Constraint prose lives in ADR +Consequences and "sharp edge" sections that no schema/contract diff would ever point you at. Bonus +tell: if the issue body cites a wiki constraint as its motivation, that exact paragraph is the one +the PR must rewrite. + +## Exhaustiveness audit for a keyed engine: list which links are compile-enforced and which are not + +**Why:** #1973's geometry engine keys everything off a `ReportColumnKey` union. Three links force a +new key at compile time (`Record`, `Record`, a +`default`-less switch with a declared return type). Four do not — the base-set arrays in `columns.ts`, +`LEADING_COLUMNS`, `RIGHT_ALIGNED_COLUMNS`, and the absorber-priority ternary — and the tests that +look like they'd catch it pin literal counts (`toHaveLength(7)`) derived from the test's own hand-typed +array, so they can't. + +**How to apply:** when reviewing a union-keyed engine, enumerate every site that consumes the union and +classify each as forcing or non-forcing; report the non-forcing ones even when they are correct today. +The cheap fix is almost always to derive the hand-typed list from an exhaustive `Record` and +filter, which converts a silent omission into a compile error. + +--- + +## A documented env var the code never reads, and how to sweep for more (#1992, wiki `e14bcbe`) + +Both auth env-var tables documented `OIDC_REDIRECT_URI`; `server/src/plugins/config.ts` never reads it. +The gate is three vars (`config.ts:142`), and the redirect URI is built per request at +`server/src/routes/oidc.ts:45` as `externalUrl || \`${request.protocol}://${request.host}\`` + +`/api/auth/oidc/callback`. Fixed wiki-only. + +**The cheap sweep** (run it whenever you touch an env-var table, it is two commands): +`grep -oE "getValue\('[A-Z0-9_]+'\)" server/src/plugins/config.ts` gives the authoritative read-set; +`grep -oE '^\| \`[A-Z][A-Z0-9_]+\`' wiki/.md` gives the documented set; `comm -23` the sorted +pair. Then `grep -n "enabled when\|If unset\|If either is missing"` — **any sentence that states a +variable count or an enablement gate is a second, independent drift surface** that the name-level diff +cannot see. That is how the "all four OIDC variables" sentence survived. + +**Findings from that sweep — both FILED, do not re-file:** + +- **#2023** — `Architecture.md` "Backup & Restore": `BACKUP_DIR` is documented as default `(none)` with + "Backup functionality is enabled when `BACKUP_DIR` is set. If unset, all `/api/backups/*` endpoints + return 503." Both halves are wrong — `config.ts:259` is `getValue('BACKUP_DIR') ?? '/backups'`, so + `backupEnabled = !!backupDir` (line 288) is **unconditionally true** and the 503 path is dead. + CLAUDE.md already documents the `/backups` default, so the wiki is the outlier. +- **#2024** — `npm run format` reformats `wiki/*.md`: `.prettierignore` excludes `docs/` but **not** + `wiki/`, while `format`/`format:check` glob `**/*.{...,md}`. Surfaced via `API-Contract.md` + lines ~3681-3720 (the `invoices[].splitKind` table from #1911/PR #2015), whose type cell overflows + the table's padded width — the only prettier-dirty region of that file, and a latent format-check + failure sitting on `beta`. +- **Ruled a CODE defect and handed to the coordinator to file** (issue number unknown at write time — + search issues for `oidc.ts:106` / `redirect_uri` before filing anything) — the two OIDC legs derive the + callback URL differently: `oidc.ts:45` uses `externalUrl || request-origin`, `oidc.ts:106` uses the + request origin unconditionally. openid-client sends the **token-request** `redirect_uri` derived + from the URL you hand `authorizationCodeGrant` (`index.js:974`, `redirectUri = stripParams(currentUrl)`), + so with `EXTERNAL_URL` set and `TRUST_PROXY` unset the two legs send *different* `redirect_uri` + values and the provider rejects the exchange with `invalid_grant` (RFC 6749 §4.1.3 requires them to + be identical). **Backend fix, not a wiki fix** — the wiki paragraph deliberately documents only the + login leg, because documenting leg 2's derivation as intended behaviour would enshrine the bug. + +**Wiki table mechanics (bit me, cost two rounds):** these tables are prettier-padded so every row is +the *same character width* (auth tables 131; API-Contract Deviation Log 2153 = cells 10/74/780/1276). +Measure with python `len()`, **never `awk length()`** — awk counts bytes here, and the em-dashes that +are everywhere in this wiki make a correctly-padded row read 2 bytes long per dash, which looks like a +padding bug and isn't. Editing a Deviation Log cell means re-padding the cell to its exact column +width, not just swapping the sentence. + +## Operator-facing docs make behavioural claims a validator must actually back (#1990, PR #2027) + +Reviewed a docs-site-only PR for technical accuracy (no architecture surface). Everything structural +checked out — defaults, the startup-failure chain, the setup route's hardcoded 5/15min. The defects +were all in the gap between *what the prose promises* and *what the code validates*: + +- **Hyperlinking a third-party library while enforcing a strict subset of it.** Copy said + "`AUTH_RATE_LIMIT_WINDOW` in [`ms`](vercel/ms) duration format". `config.ts:364` is a hand-rolled + regex that rejects `1y`, bare `900000`, `.5h`, `1 msec` — all valid `ms` input shown in that README. + Paired with a caution box promising a hard startup failure, the link *invites* the crash it warns + about (and the library is named `ms`, so "ms format" reads as "milliseconds" to many). This is the + reader-facing twin of the #1970 "regex mirroring a third-party grammar" trap: **when docs link the + upstream spec, the regex's subset becomes a documentation bug, not just a code smell.** Same loose + phrasing exists in CLAUDE.md + both wiki pages — the hyperlink is what made it actionable. +- **A caution box can be falsified by `parseInt` leniency.** "A typo is caught immediately rather than + producing an unexpectedly loose or strict limit" is false while `parseInt('2e3',10) === 2` and + `parseInt('20abc',10) === 20`. Open issue #1991 would make the copy true — **don't let prose depend + on an unlanded fix**; soften now or sequence the docs behind the fix. +- **`trustProxy: 1` is a hop count, not "trust all proxies".** `app.ts:75` + (`TRUST_PROXY === 'true' ? 1 : false`); `proxy-addr` compiles a number `n` to `(addr,i) => i < n`. + Correct for one reverse-proxy hop, but with CDN → nginx → app the list is + `[nginx(socket), cfEdge, client]`, index 1 is untrusted, and `request.ip` = the CDN edge — so + `TRUST_PROXY=true` does **not** always deliver "each client's real IP", and buckets still collapse. +- **The rate-limit key is per-/64 for IPv6**, not per-address (`rateLimitPlugin.ts` `IPV6_SUBNET = 64`), + so "keys on the client's IP" is imprecise and shared-bucket guidance isn't NAT-only. + +**Method that worked:** execute the validator's regex against every example the prose gives *and* +against examples the linked upstream spec gives — the second set is where the mismatch lives. Verify a +"fails at startup" claim by walking to the entrypoint (`server.ts` had no try/catch around +`buildApp()`; the only `try` wrapped `app.listen`) and confirming intermediate `catch` blocks *push +onto* the error array rather than swallow. + +## A guard applied to 1 of N sites of the same hazard — check the finding's own file first (#1912, PR #2028, 2026-08-06) + +My M3 finding said `getAttachmentNote` interpolates `attachmentType` into a template-literal i18n +key, so a 4th `AttachmentType` member prints a raw key onto a bank-facing PDF. Fix landed as +`ATTACHMENT_TYPE_KEYS: Record` in `buildReportContent.ts`. Real guard (missing +property on the object literal; indexed access is total). **But `buildReportContent.ts` has five +union-into-`reportT()`-key sites and the fix covers one** — lines 143 `table.title.${useCase}`, +146 `sourceType.${source.sourceType}` (5-member `BudgetSourceType`), 204 `invoiceStatus.${status}`, +274 `coverLetter.subject.${useCase}` all still interpolate, all render into the same PDF. Line 204 +even launders through `invoice.status as InvoiceStatus` — the exact widening-plus-cast shape the fix +removed two functions above. Same `AttachmentType` union is also interpolated at +`LinkedDocumentsSection.tsx:276`, plus ~a dozen status-union sites app-wide. + +- **When a finding names one call site, grep the finding's own file before reviewing the fix.** + The finding text is a sample, not a boundary — it names whatever the review chain happened to read. +- **Detection:** `grep -rEn 't\(`[^`]*\$\{' client/src` misses `reportT(` / `fixedT(` aliases. Grep the + actual alias too. `_${count === 1 ? 'one' : 'other'}` plural suffixes are a closed boolean — not + part of this class, filter them out. +- **Internal asymmetry is the tell:** the same PR argued (correctly, for the `toBcp47Locale` item) + that "a fix covering two of six would not achieve the finding's stated purpose", then shipped 1 of 2 + for the key-map item. When one item in a batch widens scope on that reasoning, apply the reasoning + to the *other* items before approving. +- **Don't ask for 15 hand-written `Record` maps.** The generalisation is one small generic + (`unionKeyMap(prefix, Record)`); leave the shape to the follow-up. +- **Residual gap the `Record` does NOT close:** union↔map parity is enforced, map↔locale-JSON parity + is not. A 4th member with a map entry and no `budget.json` key still prints a raw key. The map was + module-private so no test could iterate it — ask for the export. +- Verify "live bug or latent drift" before setting severity: I loaded both `budget.json` files with + python and diffed key sets against each union. All complete → medium, not high, approve. + +**Required-parameter hardening is ADR-034's own principle, not merely "better than a runtime throw".** +Making `reportFormatters` required on `buildReportContent` (deleting six dead silent fallbacks) is +line 230's "when a hazard is enforced only by convention, remove the channel, not the individual +call", applied to the formatters channel exactly as #2001 applied it to `TFunction`. A runtime throw +would be worse than *both* alternatives — it converts a silently-degraded bank PDF into an +export-time crash with no compile-time signal either way. **ADR-034 line 248 already documented the +6-arg signature with no optionality marker, so the change moved code toward the ADR — no Deviation +Log row.** The ADR now *under-claims* (invariant 1 at line 206 describes injection as convention where +it is now compiler-enforced at the `buildReportContent` boundary). Under-claiming is the benign +direction: note it for the next ADR-034 pass, don't request changes. + +**`toBcp47Locale` placement (the "is `formatters.ts` a grab-bag?" question).** Kept it there: all six +consumers feed the tag straight into `Intl`-backed calls (`getMonthName`/`getDayName`/ +`formatDateForAria` in `calendarUtils`, `formatWeekdayMonthDay`, `createFormatters`), so it *is* the +boundary `formatters.ts` owns. `GanttHeader`/`CalendarView` already imported it; only `MonthGrid`/ +`WeekGrid` are new importers. The file is mildly grab-baggy already (`computeActualDuration`/ +`computeWorkDuration` are arithmetic, not formatting) and the long-term shape is +`client/src/lib/locale.ts` owning `ResolvedLocale` + `toBcp47Locale`, with both `LocaleContext` and +`formatters` importing it — that also inverts the odd current direction of a pure module importing a +React context file for a type. **Trigger for doing it: a third locale-derivation helper.** ADR-034 +invariant 4 (locale chosen once at the page boundary) is unaffected — `ReportWizardPage` is still the +only mapper. + +**Checks worth repeating on refactor-only PRs:** `ReturnType` grep before approving a named +return interface (proves nothing depended on the structural-only relation); `composes:` must be the +first declaration in the rule and the composed class must not share properties with the composer +(source order decides, both being single-class selectors); grep the *old* CSS-module class name across +`e2e/` — a POM `[class*="step4Body"]` locator survives a rename as a zero-match locator with Jest green. + +## Fuzz the verbatim ports when a doc comment carries a proof (#1940, PR #2032) + +When an AC's whole correctness rests on an induction argument written in a doc comment, a hand-trace +(mine, plus the dev-team-lead's) is two reads of the same reasoning, not two independent checks. +Copy the functions verbatim into a throwaway `.mjs` and fuzz the *stated postconditions* across a +parameter space that includes the degenerate guards — 400k cases took under a minute and covered +the cascade, the mid-list runt, the hard-split path, and the meta-segment boundary at once. + +**Why:** a hand-trace confirms the argument the author wrote; it does not search for the case the +author did not consider. Only randomized inputs do that. +**How to apply:** any PR whose doc comment says "and this bound holds at any depth / for all N". +Write the ports, assert the postconditions, `rm` the file before committing. Note that the harness +must be created with `Write` (the Bash tool refuses heredoc redirects inside a worktree session). + +## A safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940) + +The #1940 ux spec argued the merge stays safe across all 96 subsets because "the threshold-to-ceiling +ratio stays roughly constant." False: `usageChunkCharsForWidth`'s **one-sided clamp** pins the +numerator's ceiling at 650 for every subset while `usageSafeTokenCharsForWidth` scales linearly with +width — the ratio runs ~3% to ~9.5%. The implementation was safe anyway (its bound is algebraic and +subset-independent), so this never became a defect — but the ratio sentence was on its way into a +code comment and the ADR. + +**Why:** proportional-scaling arguments are the first thing a clamp, a floor, or a `Math.max` breaks, +and they are exactly the arguments that read as obviously true. +**How to apply:** whenever a spec or comment justifies safety by "both sides scale off the same +basis", grep the chain for `Math.min`/`Math.max`/`Math.floor` before letting the sentence land. + +## Flex `gap` and a child's `margin` are ADDITIVE, not collapsing (#1941, PR #2033) + +`EditableField.module.css`'s `.container` is `display: flex; flex-direction: column; gap: var(--spacing-2)`. +A new `.metaRow` child was given `margin-top: var(--spacing-1)`; the two **stack**, so the reset button's +spacing went 8px -> 12px on ordinary edits — a 50% regression to chrome nobody meant to touch (#1932's +already-approved sizing). Caught in review, fixed by dropping `margin-top` entirely. The ux-designer then +found the **same bug in their own spec** for the counter-showing case. + +**Why:** people carry the block-layout intuition that adjacent margins collapse. Flex/grid `gap` does not +participate in margin collapsing at all — the child margin is added on top of the gap. It bit twice in one +PR, in the code and in the spec reviewing the code, which is the signature of a wrong mental model rather +than a slip. +**How to apply:** whenever a container owns its spacing via `gap`, no child may set `margin` in the gap +axis. Check this on any PR that introduces a new wrapper element inside an existing flex/grid container — +the symptom is only "looks slightly off", so no test catches it unless a DOM-shape assertion exists. +Recommended to @ux-designer for `Style-Guide.md` as a spacing-model rule (I don't own that page). + +## A cohesive prop group modelled as N independent optionals (#1941, PR #2033) + +`EditableField` gained four optionals: `maxLength` + three pre-translated hint strings. `hasMaxLength` +alone gates both `srOnly` spans and puts both ids in `aria-describedby`, so `maxLength={200}` with no +hints compiles clean, passes every test, and yields an `aria-describedby` pointing at two empty elements. +Invisible today (one consumer, all 8 call sites pass all four); bites at consumer #2. The fix is one +optional object prop (`lengthLimit?: { max, hint, overHint?, reachedAnnouncement }`) — absent/present +becomes a single discriminant and the compiler enforces the group. + +**Why:** same family as "the-prop-landed-is-not-the-prop-is-wired" (#1910/PR #2004), but arriving through +the *type system* instead of a call site. Optional props that are only correct together are a latent +contract, not a flexible API. +**How to apply:** when a shared component gains >1 optional prop for ONE feature, ask whether any subset is +legal. If not, make it one object. Note this is NOT an argument for the component calling +`useTranslation()` — the injection-only locale convention (same as `reportPdf/*`, ADR-034) is correct and +should be kept; only the grouping is wrong. + +## An input cap coupled to a render budget: name the packer, not the arithmetic (#1941, PR #2033) + +`USAGE_TEXT_MAX_LENGTH = 500` was documented as "leaving 150 chars for the derived suffix" under +`MAX_SAFE_USAGE_CHUNK_CHARS` (650). Three things wrong with that framing: (1) 650 is **not a cliff** — +`packUsageCellRows` splits the whole cell stream losslessly, so exceeding it costs a continuation row, not +content; (2) the 150 is **unenforceable** — `areaText` is aggregate-unbounded and `attachmentsNote` has no +`maxLength` at all, which is *why* the bound was moved to the whole cell; (3) since #1973 the budget is +**computed** (`usageChunkCharsForWidth`), pinned at 650 only by a one-sided clamp against the narrowest +subset. #1940's `'… '` marker is orthogonal: it's applied post-packing to rows `i >= 1` only, so a +single-row cell never gets one and it cannot consume headroom. + +**Why:** an arithmetic budget-split comment invites a guard test that pins a fiction. The true invariant is +`USAGE_TEXT_MAX_LENGTH < usageChunkCharsForWidth(USAGE_WIDTH_7COL)` — a typed value at the cap must fit one +row on its own — and it is the one that fails loudly if the Usage column ever narrows. +**How to apply:** before writing a guard for a "leaves N for X" comment, check whether X is bounded at all +and whether the consumer clips or paginates. Routed to #1950 (chunk-ceiling drift guard) rather than a +bespoke test; needs `USAGE_TEXT_MAX_LENGTH` exported. Keep the constant in the editor — an input +constraint living in the renderer inverts the dependency. + +## A duplicate test with a stronger title, and when a comment beats machinery (#1953, PR #2035) + +Two findings from reviewing the split of `LETTER_SUBJECT_FONT_SIZE` out of `SUBHEADER_FONT_SIZE` in +`client/src/lib/reportPdf/pageGeometry.ts`. The split itself was clean (own literal `12`, not an alias; +`headerFootprint()`/`PAGE_TOP_MARGIN` byte-identical; reason-carrying comment; inverted comment removed). + +**(a) A new test whose assertions duplicate an existing test, under a title that claims more.** +`pageGeometry.test.ts:98-101` was assertion-for-assertion identical to the pre-existing test at lines +160-167 (`toBe(93)` + `toBe(Math.ceil(headerFootprint() + 15))`, order swapped) but titled *"PAGE_TOP_MARGIN +does not depend on letterSubject.fontSize"* — a proposition its body never references. Zero added +discrimination; it catches exactly the older test's mutation set. + +**Why:** QA's mutation evidence *corroborated* rather than exposed it — "SUBHEADER 12→11 fails 4 tests" +reads as strong coverage, but two of the four are the duplicated pair. **A mutation count is not evidence +of independent coverage; it counts assertions, not propositions.** Compare each new test's failing-mutation +set against the existing suite's, not against zero. Same family as the PR #2008 "revert test proves a +different proposition than the one it licenses" and the PR #2004 r4 `count >= 1` finding. +**How to apply:** when a new test lands next to an existing one in the same file, diff the assertion bodies +before reading the titles. A title asserting a *negative dependency* ("X does not depend on Y") whose body +never mentions Y is the tell. + +**(b) When a comment is genuinely the right guard — the argument, not the shrug.** +Two adjacent `expect(...).toBe(12)` assertions protected only by a "do NOT deduplicate these" comment is +the right shape here. Not because no machinery exists, but because: (1) what is guarded is a *test's own +discrimination* — collapsing it loses coverage, it does not regress production, since the production split +and its comment stand regardless; and (2) **any structural guard would have to encode the coupling you just +removed** — "these two `number`s must be permitted to differ" is not expressible in TS, and its closest +approximation is exactly what already exists: two identifiers, two literals. The production split *is* the +structural guard. + +Rejected strengthenings, both costing more than the comment: asserting the constant through its role in +`headerFootprint()` restates production's formula in the test (the very anti-pattern `pageGeometry.ts`'s +own header comment warns against, from #1929); mirroring the `TABLE_SMALL_FONT_SIZE` constant-to-style tie +needs a module-private constant exported purely to be read by a test. +**How to apply:** before proposing machinery for a test-integrity concern, ask what the failure mode +actually costs (coverage loss vs regression) and whether the enforcement would re-express the coupling +under removal. If both answers are "yes", a comment naming the *reason* is the correct tool — and say so +affirmatively rather than as an absence of alternatives. diff --git a/.claude/agent-memory/product-architect/source-report-split-inference.md b/.claude/agent-memory/product-architect/source-report-split-inference.md index d5249cc6f..d829bfbc1 100644 --- a/.claude/agent-memory/product-architect/source-report-split-inference.md +++ b/.claude/agent-memory/product-architect/source-report-split-inference.md @@ -31,23 +31,45 @@ invoice-level residual rule means a B-tagged deposit does reduce A's Rail A shar The mirror view is correct: from B's report the same invoice has `budgetLines: []` + the tagged deposit → `‡ constituted`, no `†`. -## Proposed fix (not yet filed as of 2026-07-31) - -Add to `SourceReportInvoice`, derived in the existing `isSplit` UNION query whose two arms already -separate line-sourced from deposit-sourced `source_id`s — no extra query, no other-source data leak: - -```ts -/** How the cross-source split arises. Only meaningful when isSplit. */ -splitKind: 'lines' | 'deposits' | 'both'; -``` - -Client becomes `† iff splitKind !== 'deposits'`, `‡ iff splitKind !== 'lines'` — no `.length` proxies. - -## Wiki deviation to fix (open) - -`wiki/API-Contract.md` L~3610 and `shared/src/types/sourceReport.ts` both describe `budgetLines[]` -as _"all ibl lines per invoice (even portion 0)"_. Wrong — it is all of **this source's** ibl lines. -Pre-existing since #1878/#1891. Needs the correction + a Deviation Log row on API-Contract.md. +## SHIPPED as `splitKind` — #1911 / PR #2015 (reviewed 2026-08-05) + +`splitKind: 'lines' | 'deposits' | 'both' | null` (**required**, `null` when unsplit), derived in the +existing step-f UNION at zero extra query cost. Predicate per arm is **"a source ≠ S exists in this +arm"**, NOT "this arm has ≥2 distinct sources" — the headline case has exactly one source per arm. +`row.isSplit ⟺ 'lines'|'both'`; `row.isDepositReduced ⟺ 'deposits'|'both'`; `row.isDeposit` trigger +UNCHANGED (`invoice.isSplit && hasOwnTaggedDeposit`). Wiki API-Contract documents all of it (`1f3eb7c`). + +**Arithmetic proof AC 3.2 is right in both directions** (`depositAggregateUtils.ts:545-562`, invoice 200, +line 75, deposit 50): foreign-tagged deposit → residual 0.75, `depositFractions` **empty** (tagged rows +filtered) → 56.25, allocation genuinely drops, so "claimed **separately**" is TRUE. Untagged deposit → +residual 0.75 **plus** returned fraction 0.25 → 75.00, net zero, nothing claimed elsewhere, so the old +`(less deposit)` was literally false to a bank. Use this table whenever the trigger is re-litigated. + +**UNION dedup:** adding the `origin` literal defeats *cross-arm* row dedup. Safe only because +`COUNT(DISTINCT source_id)` and `MAX(CASE …)` are multiplicity-insensitive. Pre-#1911, `COUNT(*)` and +`COUNT(DISTINCT source_id)` were equivalent; **they are not anymore** — a future "simplification" to +`COUNT(*) > 1` silently flips isSplit for every invoice with a line and a tagged deposit in the same +source. Guarded by the AC 1.9 fixture; I asked for a comment at the query itself. `UNION ALL` would be +semantically identical, cheaper, and more honest about multiplicity. + +**Keep `isSplit`, and for a better reason than back-compat:** it and `splitKind` come from two +*independent* expressions over the same rows, which is what makes `expect(splitKind !== null).toBe(isSplit)` +a real cross-check instead of a tautology. Deriving one from the other destroys the only test that would +catch the `COUNT(*)` regression from the other side. + +## `budgetLines[]` scope deviation — wiki FIXED, shared type FIXED + +Both `wiki/API-Contract.md` and `shared/src/types/sourceReport.ts` described `budgetLines[]` as +_"all ibl lines per invoice (even portion 0)"_. Wrong twice over: it is all of **this source's** ibl +lines, and `claim` reports additionally **skip** zero-contribution lines (`sourceReportService.ts` +step h: `if (type === 'claim' && portion === 0) continue`). Pre-existing since #1878/#1891. + +- **API-Contract.md: FIXED 2026-08-04** (issue #1914). Field description rewritten + a "Budget Line + Scope" note added (subtraction basis not inventory; `isSplit` is the _only_ answer to multi-source + funding; the `claim`-only zero filter) + a Deviation Log row. +- **`shared/src/types/sourceReport.ts:60`: FIXED 2026-08-04** (issue #1917, PR #1994). JSDoc corrected + to "Budget lines allocated to this invoice for the requested source only. Other sources' lines are + absent (not present with zero portion). Used as a subtraction basis for line-exclusion math." ## pdfmake width gotcha (confirmed by QA on #1898) diff --git a/.claude/agent-memory/product-architect/story-reviews.md b/.claude/agent-memory/product-architect/story-reviews.md index a9c5d70dd..5603e71f6 100644 --- a/.claude/agent-memory/product-architect/story-reviews.md +++ b/.claude/agent-memory/product-architect/story-reviews.md @@ -518,3 +518,467 @@ Open follow-ups I own or should file: - Pre-hydration toggle window (F4): editing before the mount fetch resolves discards stored prefs for the session. Practically unreachable; `usePreferences.isLoading` is available if it ever matters. - `isLoaded` is dead API surface — returned by the hook, not destructured by `DataTable.tsx:171-172`. + +## PR #1982 — #1937 (DE header word-break) + #1938 (running-header timestamp) — APPROVED + +Two-line production diff (`merge.ts` header string, two DE strings) plus test updates. Verified locally: +`npx jest realRender -t '#1937'` (5 passed, incl. the two `positions.length === 1` real-render assertions) +and `npx jest reportPdf/merge.test -t 'pdfmake header callback'`. Note the jest invocation trap here: +`--modulePathIgnorePatterns='/.claude/worktrees/'` matches the worktree's own rootDir and silently yields +"0 files checked across 3 projects" — drop it when running inside a worktree. + +AC6 of #1938 (header still fits `PAGE_TOP_MARGIN`) discharged by analysis, not a new test — see +client-pdf-pipeline.md for the footprint reasoning. AC4/AC5 are pinned discriminatingly because the mocked +interface `t` returns the bare key, so a regression to `t()` fails rather than passing. + +Findings, all non-blocking: M1 forked harness header callback; M2 average-vs-worst-case bound in the new +AC7 tests; M3 four stale `Auftragnehmer`/`Rechnungsbetrag` cross-references (the `buildHeaderCell` +docstring one matters — it could lead someone to delete break-all protection vendor _data_ still needs); +M4 undocumented glossary divergence (`Vendor` → `Auftragnehmer` vs `Firma`); L6 follow-up: `merge.ts:134` +footer page label still uses the interface `t`. + +**Mine to do:** ADR-034 B-rule addendum — fixed-width columns impose a per-locale header character budget +(break-all is the fallback, a shorter label is the fix, real-render single-line assertion is the guard), +plus the companion rule that running headers/footers never use the interface `t`. Deliberately not made a +condition of this PR to avoid a wiki submodule bump on a two-string fix. + +## PR #1984 — deposit-aware budget-source drill-down (#1897) — CHANGES_REQUIRED (2026-08-04) + +The structural fix is right: two forked deposit-blind SQL helpers (`getWorkItemLineInvoiceData`, +`getHouseholdItemLineInvoiceData`) deleted in favour of `getInvoiceAggregates(db, line.id, +'work_item_budget_id' | 'household_item_budget_id')`. FK columns correct, no circular import +(`budgetServiceFactory` does not import `budgetSourceService`), additive for `ResolvedBudgetRelations` +(it destructures only three fields and the `undefined`-column fallback literal keeps those three, so the +union resolves). `invoiceCount`'s row-count → distinct-invoice change is a no-op because +`invoice_budget_lines` has _partial unique indexes_ on `work_item_budget_id` / `household_item_budget_id` +(`schema.ts:457-462`) — at most one ibl row per budget line. Worth remembering: that constraint makes the +"one line, many invoices" mental model wrong, and makes `wiki/API-Contract.md`'s `"invoiceCount": 2` +example impossible. + +Two blocking findings: + +- **HIGH-1** `hasClaimedInvoice: actualCostClaimed > 0` — see recurring-patterns.md + ("Amount-threshold booleans silently narrow status-existence booleans"). +- **HIGH-2** the change broadens a field documented at `wiki/API-Contract.md:4694` ("whether any linked + invoice has status `'claimed'`") without a wiki update; `actualCostPaid`'s field note on the same + endpoint is also stale (still describes whole-invoice-by-status, not the proportional split). + +Plus MEDIUM prettier violations and a MEDIUM test gap (AC8: claimed invoice fully covered by `paid` +deposits). Non-blocking: `hasClaimedInvoice` is now a misnomer — flagged as a polish follow-up, not a +rename in this PR. + +The rider (`new Set([status])`) is genuinely untestable: `computeDiscretionaryInvoiceAmount` is +module-private with two call sites passing only `'claimed'`/`'paid'`, so no test can distinguish old from +new. AC7 is honestly labelled a regression guard — accepted as-is rather than demanding a contrived test. + +**Process note**: the PR's GitHub author is `steilerDev` (the orchestrator's token), so +`gh pr review --request-changes` is rejected as a self-review. Used `gh pr comment` and stated the verdict +in the body — same workaround already noted in MEMORY.md for `--approve`. + +### Round 2 (`1f9de9b8`) — APPROVED + +Both HIGHs fixed as specified. `hasClaimedInvoice` is now +`rows.some((r) => r.invoice_status === 'claimed' || r.deposit_status === 'claimed')` — derived from the raw +join tuples _before_ `splitByDeposits`, so residual/refund arithmetic cannot reach it, and empty `rows` +still yields `false` (matches the old `COUNT(...) > 0`). Wiki `API-Contract.md:4694,4696` updated (wiki +commit `e744969`, submodule ref bumped **on the branch** — the ordering rule held). AC8 verified to be a +real mutation-killer, not a restatement: invoice 1000 `claimed` + deposit 1000 `paid` gives +`residualFraction = 0` and no claimed deposit, so the old predicate returned `false`. + +Three follow-ups left open, all informational: the `hasClaimedInvoice` rename (a claimed **refund** flips +it too — same issue), and `wiki/API-Contract.md`'s unreachable `"invoiceCount": 2` example. Also noted for +the record: `actualCostPaid`'s "Quotations are always excluded" wiki note is now only approximate — a +`quotation` invoice with a `paid` deposit contributes that portion under the proportional split. That is a +property of `computeDepositAwareAggregates`, shared by **every** consumer of the deposit-aware path, so it +is a repo-wide question for `depositAggregateUtils.ts`, never a per-endpoint patch. + +## #1971 / PR #1985 — email-search test self-containment (E2E-only) — CHANGES_REQUIRED + +Verdict posted as a `gh pr comment` (author was the authenticated user, so `gh pr review` self-review is +refused). The `Search filters by email` rewrite itself was correct and needed no changes — worker-scoped +`testPrefix` search term, seeded match + non-match, `finally` cleanup, and the negative assertion has real +teeth (`DataTable` does not slice `items`, so a no-op search renders every user and the absence check fires). + +Blocked on AC4: three of four audited `rows.length > 0` sites kept only a positive membership check against +the **shared admin row**, which passes on a no-op filter — and the committed comment claimed the opposite. +See [[recurring-patterns]] for the generalised pattern plus the soft-delete/seed-email findings. + +### Round 2 (`c23169f1`) — APPROVED + +Universal-negative loops added to `Search is case-insensitive` and both steps of `Search updates results +dynamically`, plus `fullRows.length <= partialRows.length`. Checks that made the monotonicity assertion +safe to accept: filtering is a client-side `useMemo` over a `users` array fetched once on mount, so both +reads come from one snapshot and `'admin'` narrowing `'ad'` under `includes()` cannot flake. Verified +`createLocalUser` stores the email verbatim (no lowercasing), so the POM's exact-equality `getUserRow` +still matches the uppercase `E2E-` prefix in `${testPrefix}-${Date.now()}@…`. Also confirmed `DataTable` +keeps both `tbody tr` rows and the mobile card list in the DOM, so the loops behave the same on all three +viewports. Three non-blocking follow-ups (loop-vs-seeded-row discriminating power, non-worker-scoped +`no-match-` email, positional cell indices vs column preferences) — all recorded in +[[recurring-patterns]]. + +## #1966 + #1969 / PR #1986 — column-toggle E2E coverage + testPrefix decoupling + +Round 1 CHANGES_REQUIRED (`${API}` object-interpolation making AC3 vacuous; untagged test claiming +three-viewport coverage; `no-empty-pattern` lint error; over-claiming test title). Round 2 (`9e4b0e57`) +still CHANGES_REQUIRED — but on a gap **my own round-1 review created**, see below. + +### I told them to trim a title when the AC required the assertion (my error) + +Round 1 I wrote "neither `` (ReportContentEditor.tsx:251) ← `vendor: invoice.vendorName` +(buildReportContent.ts:200) ← `vendorName: vendors.name` join (invoiceService.ts:272). + +Remaining non-blocking: unformatted new line (Prettier, invisible to CI on e2e-only PRs), stale AC4 docstring +paragraph, hardcoded preferences glob ×3, #1969 AC2 premise error (product-owner). + +## PR #1987 (#1913 + #1952) — CHANGES_REQUIRED (round 1, `23c35371`) + +`fix(server): calendar-drift test fixtures + LLM plain-prose enforcement`. Review posted via +`gh pr comment` (self-authored PR blocks `--request-changes`). + +**#1913 clean.** `futureDateStr(500)` uses real `new Date()` — no fake timers, per #1913's explicit ban +(they poison `schedulingEngine.ts`'s module-level `lastRescheduleDate` gate). Both checklist sites hit; +`insertWorkItem` defaults to `not_started` so both are genuinely CPM-today-floor-sensitive. The surviving +`'2027-06-15'` at `householdItemDepService.test.ts:186/208` is correctly left alone — `in_progress` **and** +a `listDeps` read-back with no scheduler in the path, so it cannot expire. + +**#1952 — 2 HIGH false positives** in `stripMarkup`, both violating AC 2.5's byte-identical guarantee: +intraword `_` mangling reference numbers/e-mails, and line-start `\d+[.)] ` eating German ordinals and +dates. Plus MEDIUM: two unpaired `*` on one line pairing up; AC 3.2 (`'- Pos. 3 - Dachstuhl'`) untested. +See [[recurring-patterns]] for the generalized rules — single-occurrence guard tests, German ordinals, +AC-tension, and pre-validating regex fix specs. + +Structure/integration were all correct and worth noting as the good half: strip-before-truncate at all +three call sites, `LlmInvalidResponseError` paths untouched (strip runs after the type/non-empty guards and +the empty-fallback makes it incapable of emptying a valid field), prompt rule 4 preserved (AC 3.3), wiki +amended not deleted with the submodule ref bumped on-branch (AC 4.1). 193/193 + 187/187 green locally. + +Non-blocking: `futureDateStr` now triplicated (`timeline.test.ts:151` + 2 copies) while +`server/src/test-helpers/` exists — and the two new copies dropped the JSDoc that carries the _reason_ +(CPM today-floor on `not_started`), i.e. exactly the knowledge #1913 was filed to preserve. + +### Round 2 (`857fcedd`) — APPROVED + +All six findings fixed; shipped regexes **byte-identical** to the 45/45-validated spec (diffed, not eyeballed). +207/207 pass. Test file `--numstat` = `80 0`, so no existing assertion was weakened to fit the new behaviour. + +The technique worth reusing: **prove non-vacuity by mutation, not inspection.** I replayed all 15 new +scenarios against the round-1 implementation — 13/15 fail against it. The 2 that pass under both are the +deliberate regression guards (AC 3.2 bullet+hyphen, genuine numbered run), so passing either way is their +intended property. That split is the evidence an approval should rest on; "N tests added" is not. + +Accepted residuals, recorded so they are not rediscovered as bugs: `` pseudo-tags are still +stripped (INFO-1, AC 2.4's `Beträge < 500 EUR` safe via the space-after-`<` guard); a genuine German date +list of ≥2 lines (`15. Mai: …\n16. Mai: …`) still loses its numbers, which is arguably correct (INFO-2). +`futureDateStr` extraction to `server/src/test-helpers/dates.ts` deferred as a follow-up. + +## PR #1988 (#1967 dead `attachmentsNote` override + #1947 `ReportWizardPage` → `useReducer`) + +### Round 1 (`b503e496`) — CHANGES_REQUIRED, one HIGH + +The tier design I specified in the #1947 action-set review landed correctly (named tier interfaces, +annotated total-literal factories, opaque nullable request tokens, `isGeneratingAi` derived not stored, +the Step-2 fan-out race tokenized, `deepLinkAppliedRef` holding the applied id with `report` out of the +dep array). H1 was that `freshContentTier()` was **bypassed** in `SELECT_SOURCE` and `DISCARD_EDITS` — +the two cascades — because each needed `aiError` preserved, so a future 5th `ContentTier` field would +silently default to _kept_ in exactly the handler that produced #1943 and M2. See the recurring-patterns +entry; the fix shape is spread-the-factory-then-name-the-exception. + +Method note: for a behaviour-preserving refactor, **green CI is necessary but not the review**. What +settled AC3 here was walking each silently-changed semantic and proving it unreachable — `GO_TO_STEP` +now bumping `maxReachedStep` at every call site (no-op: step-1 Next only renders once `useCase` is set, +and `WizardStepper` gates clickability on `maxReachedStep`), the new `Math.min` step clamps (use case is +only selectable at step 1, source at step 2), `freshContentTier()` in `SELECT_USE_CASE` (no-op because +`isDirty` covers all three content fields, and the dirty path dispatches `DISCARD_EDITS` first). + +### Round 2 (`01d8ff12`) — APPROVED + +Both cascades now spread the factory and override `aiError` back; the M-I test passes unmodified, which is +the pin that matters. 57 tests, 100% on all four metrics. Also confirmed the one-file Prettier fix did not +drag repo-wide drift with it. Carried forward non-blocking: whole-`wizardState` in a `useCallback` dep +array, `REPORT_REFRESHED` as the last untokenized async write, `reportStatus` duplicating the page-local +`PageStatus` union, and a comma-operator exhaustiveness guard that invites deletion. + +## PR #1998 — CVE-2026-15144 IPv6 /64 rate-limit key (#1995) + +### Round 1 — CHANGES_REQUIRED (posted via `gh pr comment`; `--request-changes` refused: own PR) + +Deleting the custom `keyGenerator` is the only correct shape (identity-check gate, see +[[recurring-patterns]]), and coverage is complete — no other `keyGenerator` exists repo-wide, and the +four route-level `config.rateLimit` blocks (`auth.ts:81`/`:145`, `users.ts:123`, `davTokens.ts:157`) +only override `max`/`timeWindow`, so one deletion fixes login, setup, password-change, and DAV tokens +together. 11.2.0 is correctly pinned in `server/package.json:20` + lockfile, no bump needed. + +MUST FIX: the deletion drops #1303's nullish-IP guard, and 11.2.0's default generator _throws_ on +`request.ip === undefined` -> 500 on the whole auth path. Asked for +`keyGenerator: (request) => normalizeIP(request.ip ?? 'unknown')` plus a comment, explicitly **not** +restoring the `x-forwarded-for`/`x-real-ip` fallbacks (they read headers regardless of `TRUST_PROXY` +— a spoofable key source; dropping them is a genuine win in this PR). Regression test can't go through +`app.inject` (light-my-request defaults `remoteAddress` to `127.0.0.1`) — suggested exporting the +generator as a named function and unit-testing `{ ip: undefined }` directly. + +Also MUST FIX: confirm `e2e/tests/proxy/proxy-setup.spec.ts:186` was green on _this PR's_ run — it is +the test #1303 cited, and `E2E Gates` is main-only, so this merges green to beta then blocks promotion. + +SHOULD FIX: negative control missing from the new test; `wiki/API-Contract.md:121` + +`wiki/Architecture.md:406` both still say "keyed on the client IP" (now the /64 for IPv6 — a whole +household's IPv6 prefix shares one 20-per-15-min login budget, operationally meaningful for +self-hosters), needs a Deviation Log row on each. MINOR: `wiki/Security-Audit.md:90`'s remediation +snippet `keyGenerator: (req) => req.ip` is now precisely the vulnerable pattern — flagged to +security-engineer rather than edited (their page). No ADR needed. + +## PR #2002 — #1968 usage-cell grey meta suffix per-token runs (2026-08-04) — CHANGES_REQUIRED + +Production change correct; blocked on coverage. Routed the grey meta suffix through `buildUsageTextRuns` +and coloured each resulting run. Verified against pdfmake source that per-run `wordBreak` + `color` +coexist and the `'\n'`-only run still forces its line break (details in `client-pdf-pipeline.md`). + +BLOCKING (H1): reverting the production hunk left all 95 + 73 tests green — the PR only relaxed the two +`splitUsageCell` helpers (which now synthesize the meta run and drop `wordBreak`) and added no test case. +#1968 AC1 (measured, real embedded font) and AC2 (short-suffix baseline unchanged) both unmet. Asked for a +`realRender.test.ts` case using the existing `WORST_CASE_TOKENS` + `renderCellScopeRow` fixtures, plus an +`overviewPdf.test.ts` unit case asserting both `color` and `wordBreak` on the raw runs. Generalised as the +"revert test" pattern in `recurring-patterns.md`. + +Non-blocking: redundant `as Content` at `overviewPdf.ts:702`; narrow-return-type alternative (M2); two +stale helper doc comments still saying "one grey run" (`overviewPdf.test.ts:162-169`, +`realRender.test.ts:289-294`); font-size coupling note. + +Notable: the comment update **repaired** a pre-existing inconsistency — `overviewPdf.ts:451` already said +"the two exceptions" on beta while the tail list enumerated three. Worth checking header-vs-list agreement +whenever that channel enumeration is edited. No schema/API/ADR surface touched, no wiki update needed. + +Mechanics: PR authored by `steilerDev` (the authenticated account), so `gh pr review --request-changes` +fails with "Can not request changes on your own pull request" — posted via `gh pr comment` instead. + +### Round 2 (2026-08-04) — APPROVED + +H1 properly fixed, verified by re-running the revert myself rather than reading the summary: reverted hunk +→ all three new tests red (`greyRuns.length` 1 not >1, both `wordBreak` assertions false); restored → +171/171. Repair shape was right — `greyRuns` returns the **raw** run objects instead of a reconstruction, +plus a `realRender` case that bypasses the helper and reads the rendered doc's run array. The relaxed +invariant keeps the two load-bearing properties (contiguous, tail-anchored) with distinct error messages. + +M1 (`as Content`) resolved as _unnecessary_, not merely deferrable: removing it type-checks clean, proven +with a tsc positive control (the client project carries ~63 pre-existing stale-`shared` errors, so +"tsc is clean" was not available as a signal). Left in place as non-blocking. + +M2 (new, non-blocking, pre-existing): ADR-034 rule #1 `max(horizontalRatio) <= 1` has **zero hits** in +`client/`, so this pipeline verifies overflow fixes by mechanism (`wordBreak` present) not outcome. Filed +**issue #2003** (tech-debt / should-have / backlog) and took ownership, since it's my ADR text setting the bar. + +Also checked and cleared: the fix's _vertical_ axis (break-all adds wrapped lines → `dontBreakRows` +silent-drop hazard) — `packUsageCellRows`' character budget already assumes worst-case per-line counts, so +the bound is not weakened. And confirmed no consumer of the old single-grey-run invariant exists in +production, `e2e/`, or any wiki page → no wiki update owed. prettier + eslint clean on all three files. + +Method note: this worktree's HEAD already contained the PR head with byte-identical `client/src/lib`, so the +revert test ran in place with no extra worktree or `npm install`. Check `git merge-base HEAD ` +plus a scoped `git diff --stat` before paying for isolation. + +## PR #2004 — #1910 preview `lang` attribute + #1888 attachments note (2026-08-05, CHANGES_REQUIRED) + +- **H1 blocking**: container-level `lang` + partial counter-tagging left UI-locale labels/buttons + mis-tagged → see recurring-patterns "Broad-scope attribute + partial counter-tagging". +- M1 coupled `lang`/`uiLang` prop pair; M2 vacuous negative test via earlier `EmptyState` early return; + L: en/de terminal-punctuation mismatch in a new key pair, `sourceReports.attachmentsNote` collides by + concept with `editable.attachmentsNoteLabel` + `table.attachmentsNote_one/_other`, + `[class*="container"]` POM selector, `styles.step4Body` is step **5**'s wrapper (pre-existing misnomer). +- Verified fine: `SourceReportType` union exactly matches the three `sourceReports.useCase.*` keys in both + locales (dynamic `t()` key is exhaustive); no client-side document filtering added (#1930 AC7 intact). +- Could not `gh pr review` (own PR) → posted via `gh pr comment`. +- E2E: new Scenarios 25/26/27 pass; Shard 8's 3 failures are pre-existing + `navigation/dashboard.spec.ts` Scenario 13 (#1735) — unrelated. + +### PR #2004 round 2 — Option A H1 fix (2026-08-05, CHANGES_REQUIRED again) + +- H1 correctly fixed via Option A (surgical positive tagging, `uiLang` deleted). Design is clean: + every `lang`-bearing element's own text is `content.*` (report language); all `t()` chrome is outside. +- **New blocking H1-r2**: E2E Scenario 25's assertion still expects container `lang="de"` (comment was + updated, assertion was not) → Shard 2/16 red, confirmed via shard-diff vs `2744d75b`. + See recurring-patterns "Comment refreshed, assertion left behind". +- **H2-r2**: Scenario 27 became unconditional (see "Inverting a contract can make an existing negative + test unconditional"). +- M: `.readOnlyValue` spans (`dateLine`, `closing`) missed by the tagging; no test proves + `ReportContentEditor` passes `lang` to its `EditableField`s (delete all 8 props → still green); + 4 of 5 tagged sections unasserted. +- L: POM `reportContentContainer()` docstring now describes the removed behaviour; `aria-label` in UI + locale inside a `lang`-tagged `` is an inherent, accepted residual — leave a code comment so + nobody "fixes" it by deleting the attribute; `expect(tableWrapper).toBeVisible()` would fail if + Scenario 26 were ever tagged `@responsive` (≤767px hides `.table`, not `.tableWrapper`). +- Playwright projects: `tablet` and `mobile` both `grep: /@responsive/` — untagged scenarios are + **desktop-only**. Useful when judging whether a viewport-sensitive assertion is actually at risk. + +### PR #2004 round 3 — thead retarget (2026-08-05, CHANGES_REQUIRED, 3rd round) + +- **Both round-2 blockers CLEARED**: E2E 25/26/27 now target `` with real assertions + (`'de'` / `null`); `` is unique in the component (`.summaryTable` has no `thead`), so + `.first()` is unambiguous. `toBeVisible()` on `` is only safe because 25-27 are untagged → + desktop-only. M2-r2 (integration test on the usage input, scoped via `getDesktopTable` + + `getByDisplayValue`), M3-r2 (double guard), L1-r2 (POM docstring, verified line-by-line) all fixed. + `uiLang` is now 0 hits repo-wide. +- **New blocking H1-r3**: removing `lang` from `.tableWrapper`/`.mobileCardList` and re-adding it to + `` only dropped coverage for the desktop `` and the whole mobile card tree — + AC1 of #1910 explicitly enumerates "table captions … status text". Recommended fix: restore the + wrapper tags and counter-tag `EditableField`'s sr-only hint + reset button via a new `uiLang` prop + (do NOT counter-tag `` is now redundant with the wrapper tag and a test asserts both — a future + cleanup could delete the _wrapper_ instead and reintroduce H5. +- E2E scenarios 25-27 are untagged, therefore desktop-only (`e2e/playwright.config.ts` gates + tablet/mobile on `grep: /@responsive/`), so their `expect(thead).toBeVisible()` is safe despite + `.table { display: none }` at <=767px. Consequence: the mobile fix has unit coverage only. +- CI: `Quality Gates` green. `E2E Tests (Shard 8/16)` fails on `navigation/dashboard.spec.ts` + 1130/1164/1192 (#1735 Add dropdown) on **all four** head commits including the first -> pre-existing, + main-only, needs its own issue before the next promotion. + +## PR #2006 — #2005 E2E shard 8/16 red, dashboard "New Invoice" opens no modal — APPROVED + +Test-fixture-only diff (`e2e/tests/navigation/dashboard.spec.ts`), no production/schema/API surface. +`mockInvoicesFullSummary()` gained `claimable` + `quotationCoveredByDeposits` and `pagination.total` +became `totalItems`. Verified against `shared/src/types/invoice.ts:159-169` and +`shared/src/types/pagination.ts:4-9`, every consumer site in `InvoicesPage.tsx` / +`InvoicePipelineCard.tsx`, the `?create=1` gate at `InvoicesPage.tsx:277-293`, the navigation source at +`DashboardPage.tsx:552`, trailers, and CI (shard 8/16 + Quality Gates green on 7b7a1ea). + +Five non-blocking follow-ups filed in the review comment, none yet ticketed: +type the fixtures against `InvoiceListPaginatedResponse` (would have caught both defects at typecheck — +highest leverage); backfill `mockInvoices()` (latently broken, see recurring-patterns); refresh the +stale JSDoc at ~L976 that enumerates an outdated field list; comment the duplicate-route-glob ordering +dependency; re-add `@smoke` now that #1735 is in beta. **If these are still unticketed, file them.** + +Process note: memory updates for this review were left **uncommitted** rather than pushed onto the +author's branch — appending a commit to an approved PR with green CI would retrigger E2E and invalidate +the review. As reviewer, hand memory edits back to the orchestrator to ride along on a later PR. + +## PR #2007 / issue #2001 — remove `TFunction` from `reportPdf/*` (2026-08-05) — CHANGES_REQUIRED (r1) + +Pure refactor closing the ADR-034 locale-leak class. AC5 (no `TFunction` in `reportPdf/*` production +files) verified holding on head; AC1/3/4/6/8 verified; all CI gates green including full E2E. + +Two blocking findings: + +1. `overviewPdf.ts:835-838` — `skipReasonLabels[reason as 'footnoteFetchFailed'|'footnoteInvalidPdf'] ?? reason`. + The union is already exact on `SkippedDocument.reason`; `merge.ts:101`'s `Map` + widens it. Fix: name the union in `reportContent/types.ts`, `Record`, + `Map` in merge + the `buildOverviewContent` signature, drop cast and `??`. + Verified no test passes an out-of-union reason, so the change is annotation-only. +2. ADR-034 stale at lines 82 / 178 / 186-188 (grep guard) / 200 / 219 — all describe the pre-#2001 + weaker contract. Requested the wiki delta be routed back to product-architect on the same branch, + plus a Deviation Log row. + +Informational: `buildReportContent.ts:82` `getAttachmentNote(invoice, t)` is the last one-character +`t`/`reportT` confusion (private, single caller, correct); its dynamic +`sourceReports.table.attachmentType.${type}` key can echo a raw identifier into the PDF if the server +sends an untranslated `attachmentType` — same key-echo class, needs its own issue if the set is open. +Also pre-existing: `merge.ts` pushes appendix-load failures to `skippedDocuments` _after_ +`buildOverviewContent` already consumed the map, so those get no `*N` footnote. + +Process note: could not `gh pr review --request-changes` (own-PR restriction) — posted via `gh pr comment`. + +### Round 2 — APPROVED (2026-08-05) + +Both blocking findings closed on head `7a40fa3`. H1 fixed exactly as specced _and_ the widened +`Map` was replaced too — that second half is the load-bearing one, since keeping the +widened transport type forces the assertion straight back. `SkippedDocument.reason` now references +`ReportSkipReason` rather than re-declaring the literals (the one gap that survived the first fix +attempt; caught by re-reading rather than trusting the "resolved" report). `Static Analysis` green is +itself the proof no call site smuggles an out-of-union reason. + +H2: wiki commit `cfbb0bd` published and `git ls-tree wiki` on the branch resolves to it, so the +PR carries the ref — worth checking explicitly, because a wiki push outside the branch leaves the ref +un-bumped and the PR technically still fails the finding. + +The unrelated repo-wide prettier union-collapse drift (5 files) was kept out of the commit after I +flagged it; verified with `git diff --stat origin/beta... -- ` being empty. + +Left three non-blocking follow-ups: stale signature in `overviewPdf.test.ts:10`'s header comment +(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 7ebea615a..03764c121 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -21,9 +21,9 @@ ## Active / Planned Epics -- EPIC-04 (#4) Household Items [Must] — IN PROGRESS, 11 stories (#387-#394, #413, #415, #467). Full detail + per-story review notes in [epic-04-planning.md](epic-04-planning.md) +- EPIC-04 (#4) Household Items [Must] — IN PROGRESS, 11 stories (#387-#394, #413, #415, #467). See [epic-04-planning.md](epic-04-planning.md) - EPIC-16 (#752) Floor Plans & Utility Tracking (2.5D) [Should, Future] — 22 stories (#753-#775), all Backlog, 6 phases. See [epic-16-planning.md](epic-16-planning.md) -- Backlog-only (not started): EPIC-07 Reporting [#7], EPIC-09 Dashboard [#9], EPIC-10 UX Polish [#10]. Deps: 07←05,08; 09←02,03,05; 10←all. +- Backlog-only (not started): EPIC-07 Reporting [#7], EPIC-09 Dashboard [#9], EPIC-10 UX Polish [#10]. Deps: 07←05,08; 09←02,03,05; 10←all. See [epic-09-planning.md](epic-09-planning.md), [epic-17-planning.md](epic-17-planning.md), [epic-18-planning.md](epic-18-planning.md) ## Completed Sprint Story References @@ -31,17 +31,27 @@ ## Standalone bugs & stories (no active parent epic) -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. +Detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) (budget/invoice), [standalone-diary-bugs.md](standalone-diary-bugs.md), [standalone-photo-stories.md](standalone-photo-stories.md). Growing budget/invoice cluster (20+) → propose a "Budget/Invoice UX Polish" epic next planning cycle. -- **#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". **PR #1951 APPROVED round 1** (all 40 ACs, 3 MUST FIX); architect review filed 2 follow-ups 2026-08-02: **#1952** (tech-debt, Should Have, Todo — plain-prose guarantee is prompt-level only; ruled *strip, not reject* in the validator, §2 weighted on false-positive guards) and **#1953** (tech-debt, Could Have, Backlog, blocked-by #1932 — `letterSubject` false-shares `SUBHEADER_FONT_SIZE` with `headerFootprint()`; ruled the equality **coincidental**, independent literal not an alias, + records the `PDF_STYLES` split trigger in the file header). Architect's own `react/no-danger` follow-up **verified not filed** — prompted on the PR, not duplicated. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"#1951 architecture-review follow-ups". -- 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) -- **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). -- **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". -- **PR #1959 (report PDF UX, user's own PR, held promotion #1958) — 3 rulings 2026-08-03.** Footnotes `†`/`‡` + their sentences replaced by inline labels `(partial)`/`(less deposit)`, reversing **#1923 AC1.1/1.2/2.3/2.4** (which had already superseded #1898 §4). **Ruled: labels kept, sentences return as a non-editable report-level legend → #1965 (Must Have)** — `(partial)` is near-self-evident beside the two amount columns, but `(less deposit)` drops "claimed **separately**", a materially different audit claim. Cheap because the footnote channel is orphaned-not-removed (producer-only fix). **Closed/released ACs get a dated supersession comment, never a rewrite** (posted on #1898 + #1923). #1923 AC5.3 substance survives — `areaText` still a separate field, so judged against its stated rationale, not its prescribed sub-line rendering. Glossary: **`Abschlag` APPROVED** as a measured-space short form of `Abschlagszahlung` (option (c) is arithmetically impossible — the full term eats 72.85 of 75pt, so no qualifier fits; collapsing into the constituted-deposit label would lose more); **`split`'s three German forms get NO entry** (glossary prevents semantic divergence, is not a surface-form registry). Both on **#1917**. Issues filed: **#1965** legend, **#1966** column-toggle E2E, **#1967** `attachmentsNote` override unreachable, **#1968** meta-suffix single run, **#1969** `testPrefix`/`authenticatedPage`, **#1970** configurable auth rate limits, **#1971** `search-users.spec.ts` leftovers, **#1972** silent failed column-pref saves + dead `isLoaded`. Full rulings in [bank-report-wizard.md](bank-report-wizard.md) §"PR #1959 rulings". +- Budget/invoice batches: #1369-#1373, #1389-#1390, #1401, #1421-#1425, #1439-#1441, #1553 +- Auto-itemize (standalone, no parent epic): #1545-#1547 mini-epic, #1600, #1833 duplicate budget lines on commit retry +- Diary: #1426 critical photo data loss | Photo: #1723 lightbox picker UX +- **#1970 auth rate limits** — PR #1989 approved R3, all 7 ACs met, Done on merge; follow-ups #1990/#1991/#1992 open. See [auth-rate-limits-1970.md](auth-rate-limits-1970.md) +- **#1955 DataTable two-column toggle race** (Should Have, Backlog) — fast clicking is the SAFE case; #1920's E2E-only fix makes CI green without fixing production. See [datatable-column-preference-race.md](datatable-column-preference-race.md) +- **#1957 E2E test-isolation hazard** (shared-admin `user_preferences` writes under `fullyParallel`) — Should Have, Backlog; an audit + per-spec sweep, not a single-file fix. See [e2e-shared-admin-preference-hazard.md](e2e-shared-admin-preference-hazard.md) +- **#2005 CLOSED** — dashboard "New Invoice" shard 8/16 failure; all 16 shards green on `df7d2c73`, promotion blocker cleared. +- **#1991 numeric env-var parsing** — AC1 amended to `/^-?\d+$/` 2026-08-06 (rev 2, numbering unchanged); implementer's choice upheld. Docs half → **#2036** (blocked-by #1991, needs Backlog). +- **#1992 wiki-only OIDC env-var fix** — PR #2022 APPROVED, 7/7 ACs, **close on merge + board Done, no UAT** (all claims machine-checkable against `config.ts`/`oidc.ts`). Deviation-Log ruling in [pr-review-patterns.md](pr-review-patterns.md). Sweep spin-offs: **#2023** (`BACKUP_DIR` default+dead 503 gate; its AC2 hides a product decision — making backups opt-in is a behaviour change for existing deployments and comes to me), **#2024** (`wiki/` missing from `.prettierignore`; recommend splitting its AC3/AC4 out so the 2-line safety fix ships first). + +## Bank Report Wizard mini-epic (no parent epic) + +All rulings, contract facts, per-PR review outcomes and filed follow-ups live in [bank-report-wizard.md](bank-report-wizard.md) — consult it before any work on this cluster. + +- Shipped: #1876-#1879, #1898-#1901, #1929-#1933, #1958, #1959, #1965, #2001, #2003, #1973 +- Open: #1888, #1891, #1895-#1897, #1910, #1912, #1917, #1938, #1940, #1941, #1946 (Must Have), #1947, #1950, #1952, #1953, #1966-#1972, #2011-#2014, #2016-#2021 (#1937/#1939 CLOSED — no open home for allocated-column geometry) +- Merged but **not Done** (awaiting live-LLM UAT): #1931 (ACs 3.2/3.3) +- **#1973** column visibility → PDF: PR #2010 approved R1, 28/33 ACs, → **UAT, stays In Progress**. Follow-ups #2011-#2014 all Backlog + blocked-by #1973. +- **#1911** `splitKind`: ACs refined 2026-08-05 onto an orchestrator-filed body → **UAT, not Done-on-merge**. Privacy: no objection. Legend: no new sentence. Addendum ruling (pre-merge): zero-line-contribution row now shows `(Deposit)`+`(partial)` — **intended, do not narrow**, AC 3.6/3.7/4.7 added. **PR #2015 CHANGES REQUESTED round 1** — logic correct, evidence layer not: 2 red E2E scenarios (stale assertions, not defects) + AC 4.5's render measurement unfalsifiable by construction. ## Requirements Coverage @@ -50,15 +60,15 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) - Household Items are explicitly NOT work items (Section 5, Key Decisions). - Budget sub-domains: categories, vendors, creditors, subsidies — all in EPIC-05. - EPIC-11 covers cross-cutting non-functional: testing, Docker, security. +- Glossary/terminology decisions: [glossary-decisions.md](glossary-decisions.md) ## GitHub Projects Board — operational -- Project ID: `PVT_kwHOAGtLQM4BOlve` | Status Field ID: `PVTSSF_lAHOAGtLQM4BOlvezg9P0yo` -- Status Option IDs: Backlog=`7404f88c`, Todo=`dc74a3b0`, In Progress=`296eeabe`, Done=`c558f50d`, Wont-Do=`90c1bc33` -- Native `gh project` commands (not raw GraphQL) for board mgmt: `item-edit --id --project-id --field-id --single-select-option-id `; `item-add 4 --owner steilerDev --url ` -- **`gh project item-list` has NO `--query` flag** in the installed gh (only `--format`/`--jq`/`--limit`/`--owner`/`--template`) — the `--query "is:issue #"` form documented in the agent definition fails with `unknown flag`. Resolve an item node ID via `gh api graphql '{ repository(owner:"steilerDev",name:"cornerstone"){ issue(number:N){ projectItems(first:5){ nodes{ id project{number} } } } } }'` and verify status by node ID with `node(id:"PVTI_…"){ ... on ProjectV2Item { fieldValueByName(name:"Status"){ ... on ProjectV2ItemFieldSingleSelectValue { name } } } }`. Confirmed 2026-08-01 (#1917). -- GraphQL still needed for `addSubIssue` and `addBlockedBy` (`addBlockedBy` uses `blockingIssueId`, NOT `blockedByIssueId`) -- If `item-list` is empty right after `item-add` (indexing lag), resolve item node ID via issue `projectItems` GraphQL and set status by that ID. See [board-operations.md](board-operations.md) +- Board status changes go through `bash scripts/board.sh ` — it owns the project/field/option IDs. +- GraphQL still needed for `addSubIssue` and `addBlockedBy` (`addBlockedBy` uses `blockingIssueId`, NOT `blockedByIssueId`). +- **`gh project item-list` has NO `--query` flag** in the installed gh — the form in the agent definition fails. Resolve item node IDs via the issue's `projectItems` GraphQL field instead. +- **`gh issue view ` fails** with a Projects-classic deprecation error — use `gh api repos/steilerDev/cornerstone/issues/ --jq '.body'` instead (confirmed 2026-08-05). +- If `item-list` is empty right after `item-add` (indexing lag), resolve the item node ID via the issue's `projectItems` GraphQL field and set status by that ID. ## Patterns and Conventions @@ -66,6 +76,48 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) - Epic body has task list of story issues; story body references `**Parent Epic**: #NN`. - IMPORTANT: GitHub issue numbers ≠ epic numbers (EPIC-11 is issue #12). See [epic-patterns.md](epic-patterns.md). +## Standing rulings (reusable across clusters) + +Full derivations and the incidents behind each are in [pr-review-patterns.md](pr-review-patterns.md) and [bank-report-wizard.md](bank-report-wizard.md). + +- **Merge is a code gate; Done is an acceptance gate.** An unverifiable AC *with* a substitute assertion = documented deviation; *without* one → UAT, reopen on failure. +- **A finding that defeats the PR's own AC belongs in that PR, not a follow-up.** Conversely: **a green PR is not reopened to absorb non-blocking findings — file, don't expand.** +- **Closed/released ACs get a dated supersession comment, never a rewrite.** When a body *is* rewritten, always report "body rewritten, numbering reassigned" — omitting that let two agents spec from a stale revision. +- **Wiki Deviation Log: the Observation column of a dated entry is immutable; corrections go forward in that entry's Resolution.** Ruled 2026-08-06 on PR #2022 (#1992). Same shape as the AC supersession rule. Rationale + boundaries (spurious entries are withdrawn not deleted; lead a correction with "Correction to the observation above:", never a trailing parenthetical) in [pr-review-patterns.md](pr-review-patterns.md). +- **Operator-facing docs never carry a "known issue" pointer to an open bug** — docs ship with releases, the tracker doesn't, and nothing forces the line's removal when the fix lands. Put the *workaround* in as a plain requirement instead: prescriptive copy ages into harmlessness, diagnostic copy ages into lies. Ruled 2026-08-06 on PR #2027 (#1990) re #2026. +- **Docs-only PRs make `Quality Gates`/`E2E Gates` green by vacuity** — `Detect Changes` skips every real job, and the `onBrokenAnchors: 'throw'` docs build runs only on release. Run `npm run docs:build` yourself when reviewing a docs PR that adds anchors. +- **An item whose every claim is machine-checkable against source has no UAT surface** — the PO review *is* the acceptance gate; close on merge (#1992). UAT is for rendered artefacts and operator-observable behaviour. +- **ACs that misdescribe reality fail correct implementations at UAT** (seen 4×). Amend the text; don't design around it. **Shipped error copy that misdescribes an operator's own input is a worse instance than an AC that misdescribes code** — the AC is read once by someone who can check the source; the message is read at 2am with nothing else (#1991). +- **When an AC names a mechanism (a regex, an algorithm) as if it were the contract, the contract is the *outcome* AC.** The mechanism gives way. Check first whether the strict form is even *satisfiable* alongside the guard AC — #1991's `/^\d+$/` required modifying tests that AC6 said must pass unmodified, so it wasn't a tension to balance, it was unsatisfiable. +- **"The downstream check will catch it" is true seven times out of eight — enumerate all eight.** Verifying every one of #1991's call sites had a lower bound turned a plausible argument into a safety proof, and revealed the disputed behaviour was observable at only 3 of 8 sites (the other 5 collapse both branches into one message). +- **An input cap on a field whose baseline is *derived* must clear what the system itself legally generates**, or the AC's exception state becomes the routine state and a near-limit affordance turns into permanent furniture. Find the floor before picking the number (#1941: `usageText` 500, not the suggested 150). +- **Before routing a "rendering capacity" question to an architect, check whether the renderer has a container at all.** Flowing content with no table/`dontBreakRows`/fixed height has no capacity ceiling — an over-long value just makes more pages, so the question was a *product* one all along (#1941 `coverLetter.body`). Saves a routing round. +- **Before calling a missing mechanism an accessibility gap, check which mechanism already carries the fact.** A two-mode component can legitimately use the accessible *name* in one mode and the *description* in the other; "fixing" the omission then double-announces (#1941 `EditableField` dense vs labelled mode). Separate the enabling refactor (in scope) from the behaviour change (rejected). +- **Chasing an AC's vacuity often exposes a defect in its neighbour** — #1941's AC5 (no server round-trip) proved AC4's "existing *saved* value" fixture unconstructible. Also: a vacuous AC needs an explicit *prohibition*, not just a note, when the literal reading invites the inverse mistake. +- **An issue filed by another agent is a snapshot of that round's codebase** — re-verify the *mechanism*, not just the defect, and correct the mechanism while keeping the story. +- **Premises go stale at *pickup*, not at filing — check the dates before blaming filing diligence.** #1941 and #1950 were both correct when written and invalidated 1–2 days later by *other queue items reworking the same files*; no care at filing could have prevented it. When a queue holds several items touching one area, re-verify every premise against `HEAD` when the item is picked up. Both were caught by implementers who flagged instead of fabricating — the control working, not failing. +- **An AC satisfiable only by undoing a shipped decision is a bug in the acceptance record, not an AC** (#1950's AC 1.5 required reinstating the constant its own guard exists to keep from returning). Strike it; don't let an implementer comply. +- **Before adding a replacement assertion for a struck AC, check whether the invariant is already covered.** Split the question: *drift* is a test's job, *deliberate change* is a doc comment's job. If both are covered, record the invariant as prose and add nothing (#1950). +- **When an AC's correct predicate is one plausible misreading away from a no-op, write the misreading into the AC** and demand a test pinning that shape. +- **Check the other direction of any reported boolean defect** — the mirror case is often live too. +- **`Refs #N`, not `Fixes #N`, for any issue in a parent-less cluster carrying a UAT disposition** — `/epic-close` (the only skill with a UAT step) never runs without an epic, so standalone `/release` would auto-close it unvalidated. **Before trusting "the lifecycle protects this", check the item actually enters that lifecycle.** The acceptance gate is the board status, which I set — not open/closed, which GitHub sets. +- **When a review finding invokes a principle, check whether the principle actually condemns the code.** A principle stated only in its prohibiting direction generates false positives; write the permission into the ADR alongside it. +- **When a change looks like new behaviour, check whether the underlying field already carried the right value and only a downstream gate was lying.** If so it is a fix, and the burden flips to whoever wants the old output. If the only available narrowing is the removed gate under a new name, there is no narrowing — say so plainly. +- **A corrected assumption appearing independently in code + docs + tests is one spec gap, not three bugs** — and is the argument for writing the AC down even when the code is already right. +- **"Dominated by an existing measurement, do not re-measure" is as valuable as demanding the measurement.** Rank flagged risks against each other instead of treating every new co-occurrence as equally alarming. +- **When a fix removes a mutual exclusion, ask what combination just became reachable** that never rendered before. +- **A documented measurement (glossary space budget, column width) can close a wording debate before it starts.** +- **"Real render" ≠ "measured."** An unmocked render whose assertions read the *input* content tree, or read a quantity fixed by construction (e.g. a table width that is `printableWidth()` for any input), is still a vacuous assertion. Ask what *varies* when the guarded content changes — not whether a render happened. +- **Check that the issue a routing rule points at is still open before restating the rule.** +- **Answer boundary/privacy questions about the artifact that leaves the system, not only about the API.** +- **A finding's severity is capped by my own enumeration failure** — if I missed it in earlier rounds, it can't be blocking now. End mirror-image review cycles by **stating the enumeration as exhaustive**. +- **Comment keeps the rationale, issue owns the guard.** Bounded-and-quantified earns a tracked owner; unbounded-and-estimated gets documentation only. +- **Price intrinsic tensions differently from oversights** — offer a documented deviation for genuine conflicts. + ## PR Review -Detailed checklist, recurring violations, and per-PR findings in [pr-review-patterns.md](pr-review-patterns.md). Top recurring items to check FIRST: **CI shard status (`Quality Gates` green ≠ E2E green)**, **test doubles hiding defects (wrong-sign fixtures / key-echoing `t` mocks / mocked libs)**, **every `t()` path actually resolves in `en/.json`**, dependency pinning (exact versions), keyboard :focus indicators (WCAG AA), test authorship (QA not devs), E2E gate for "Automated (E2E)" scenarios, raw-value display bugs (formatDate/percent/"—" placeholder). Verdict matrix: `--request-changes` for functional AC gaps; `--comment` "MUST FIX" for display/formatting; `--approve` when all met. +Detailed checklist, verdict matrix, recurring violations, and per-PR findings in [pr-review-patterns.md](pr-review-patterns.md). Check FIRST: **CI shard status** (`Quality Gates` green ≠ E2E green), **test doubles hiding defects**, **vacuous assertions** (invert an inequality's bound to read the measured value; re-run the exact mutations that stayed green last round), **every `t()` path resolves in `en/.json`**, dependency pinning, keyboard focus indicators, test authorship (QA not devs), raw-value display bugs. + +- Verdict matrix: `--request-changes` for functional AC gaps; `--approve` with "MUST FIX" notes for display/formatting; never `--comment` as a verdict. +- `gh pr review` **cannot** request changes on a human-authored PR — the verdict goes in a comment. +- `npm run lint` has **no Prettier** and CI has no `format:check`, so formatting drift merges silently. diff --git a/.claude/agent-memory/product-owner/auth-rate-limits-1970.md b/.claude/agent-memory/product-owner/auth-rate-limits-1970.md new file mode 100644 index 000000000..6161eca83 --- /dev/null +++ b/.claude/agent-memory/product-owner/auth-rate-limits-1970.md @@ -0,0 +1,102 @@ +--- +name: auth-rate-limits-1970 +description: Story #1970 (configurable auth rate limits) and the two-round PR #1989 review — rulings, filed follow-ups, and the AC-assertion gap that keeps it out of Done +metadata: + type: project +--- + +# #1970 — configurable login rate limits (`AUTH_RATE_LIMIT_MAX` / `_WINDOW`) + +Standalone story, no parent epic. Should Have. Filed 2026-08-03 out of the PR #1959 ruling +sweep. Implemented in PR #1989 (`feat/1970-auth-rate-limits-configurable`). + +**Why it exists:** self-hosted households behind one NAT share a rate-limit bucket, so +legitimate family retries can lock out login; internet-exposed instances want the opposite. +20/15min was hardcoded at `auth.ts:139`. The old security-hygiene home (#315) is CLOSED. + +## Review round 1 (2026-08-03) — CHANGES_REQUIRED + +6 of 7 ACs met. Blocking: `AUTH_RATE_LIMIT_WINDOW=0s` passed validation — `max` rejected +`<= 0`, the window was pattern-matched with no bound on the resulting duration. Defeated AC2 +("does not silently disable the limit") and AC7 ("no value that removes the limit entirely"). +Medium (M1): nothing proved the *window* reached the route. + +Rulings made in round 1: + +- **AC5** ("documented in CLAUDE.md **and** on the docs site — file a request if needed") is + satisfiable by filing the request → **#1990** (docs-writer, Todo, blocked-by #1970). Must + cross-reference `TRUST_PROXY`: it decides whether the bucket keys on the real client IP or + the proxy's, and the NAT operator needs both settings together. +- **AC6** (setup route stays hardcoded) ACCEPTED. The store is in-memory, so a restart clears + the bootstrap limiter, and `/setup` 403s unconditionally once setup is complete — tuning it + has no operational value. Rationale comment at `auth.ts:76-78`. +- **`parseInt` leniency** ruled house convention, out of scope for this story. + +## Review round 2 (`47ee190`, 2026-08-04) — APPROVED with one MUST FIX + +**B1 resolved.** `config.ts:371-375` adds `else if (parseFloat(str) <= 0)`, rejecting `0s`, +`0 minutes`, `0.0h`, `00 minutes` with a message naming the variable (3 tests in +`config.test.ts:1093-1109`). The pattern also changed `\s*` → ` *` (`config.ts:365`), which +closes the second instance of the same drift class: `15\tminutes` no longer validates. + +**My round-1 mechanism was wrong** — see [pr-review-patterns.md](pr-review-patterns.md) +§"Configurable security controls". I claimed silent disable via `LocalStore.incr`; the verified +behaviour (product-architect, against `node_modules`) is `parse('0s') → undefined` → +`mergeParams()`'s `if/else if` never reaches `defaultTimeWindow` → **every login 500s** on +`params.timeWindow is not a function`. Do not cite my round-1 comment as the mechanism. + +**M1 (was MUST FIX).** The round-2 assertions were on `x-ratelimit-limit` (`'3'`, `'20'`), fed by +`max` only. Deleting `timeWindow` from `auth.ts:147` inherits the global `'1 minute'` and leaves +both green — exactly the drift AC1 and AC4 were written to catch. Fix specified as one line on +the default-config test: `expect(response.headers['x-ratelimit-reset']).toBe('900')` +(`Math.ceil(ttl/1000)`, `index.js:265`; emitted on non-exceeded responses too). + +**Gate ruling: merge is a code gate, Done is an acceptance gate.** The PR may merge on B1; +#1970 does **not** go to Done until the window assertion lands. If it merges without, +**reopen #1970** rather than filing a follow-up (same precedent as #1931). + +## Review round 3 (`5446b29a`, 2026-08-04) — APPROVED, M1 RESOLVED + +Fix applied verbatim at `rateLimitPlugin.test.ts:157`. All 7 ACs met, **no unverifiable AC +remains** → the Done gate above is satisfied on both sides: #1970 goes to **Done on merge**, no +reopen, no substitute follow-up. Follow-ups #1990/#1991/#1992 stay independent and don't gate. + +Two things worth reusing: + +- **Verify a "proves X reached the route" assertion by mutation, not by reading it.** I deleted + `timeWindow` from `auth.ts:147` locally, ran the one test file, got + `Expected: "900" / Received: "60"`, then `git checkout -- server/src/routes/auth.ts`. That is + the only evidence that distinguishes a load-bearing assertion from one that merely *looks* + specific. Reverting immediately keeps it inside PO boundaries — it is verification, not + authoring. Generalized in [pr-review-patterns.md](pr-review-patterns.md). +- **Check the header is deterministic before accepting it as an AC probe.** `LocalStore.incr` + sets `ttl: timeWindow` exactly on the *first* request of a fresh window + (`store/LocalStore.js:17`), and each test builds its own app → fresh in-memory store. So `900` + is exact, not timing-sensitive. Had the assertion been on a *later* request, ttl would be + `timeWindow - elapsed` (`LocalStore.js:38`) and the same assertion would have been a flake. + When approving a numeric-header probe, ask which request in the window it observes. + +## Follow-ups filed from this review + +| Issue | Kind | Status | Substance | +| --- | --- | --- | --- | +| **#1990** | user-story, Should Have | Todo | Docs-site rate-limit copy (docs-writer), must cross-reference `TRUST_PROXY` | +| **#1991** | tech-debt, Could Have | Backlog | Uniform integer parsing across the 8 `parseInt` sites in `loadConfig()` — the tracked home for the leniency ruling | +| **#1992** | documentation, Should Have | Todo | Wiki documents a nonexistent `OIDC_REDIRECT_URI` and a four-variable OIDC gate; `config.ts:142` gates on three | + +**#1991 rationale:** the ruling stays "out of scope for #1970", but `product-architect` +(Medium) and `security-engineer` (Low) both raised it independently, so a review comment alone +guarantees a fourth reviewer raises it again. A local `/^\d+$/` guard on one variable would +leave seven inconsistent siblings. Same shape as the #1950 ruling. + +**#1992 rationale:** the architect's wiki commit `5c1c7e71` flagged the deviation in the +API-Contract Deviation Log as an explicit unresolved follow-up but nothing tracked it. Verified +real. `CLAUDE.md` and the docs site are already correct → wiki-only fix (2 pages), owned by +product-architect. + +Wiki MEDIUM from round 1 resolved: submodule bumped to `5c1c7e71`, verified on `origin/master`; +both auth tables carry the new rows, `TRUST_PROXY` backfilled, plus a "Rate Limiting (Auth)" +subsection. + +`gh pr review --approve` refused again ("Can not approve your own pull request") — verdict +posted via `gh pr comment` with an explicit `## Verdict:` line. diff --git a/.claude/agent-memory/product-owner/bank-report-wizard.md b/.claude/agent-memory/product-owner/bank-report-wizard.md index a0596a427..ccb627ff2 100644 --- a/.claude/agent-memory/product-owner/bank-report-wizard.md +++ b/.claude/agent-memory/product-owner/bank-report-wizard.md @@ -157,76 +157,93 @@ Related: [[pr-review-patterns]]. Source: user inspection of downloaded report PDFs + a wizard walkthrough after #1901 merged. No parent epic; all board **Todo**, destined for `/batch-develop` (one issue = one branch/PR, no dependency chain declared — they touch disjoint files, but #1929 and #1932 both touch `reportPdf/`, so whichever lands second must re-verify the other's ACs). - **#1929 — PDF layout robustness** (`bug`, **Must Have**). Three verified defects in the pdfmake pipeline: (a) `overviewPdf.ts` widths `['*','auto','auto','auto','auto','auto','*']` — five `auto` columns eat the printable width before the two `*` columns get anything, so the Usage column collapses and the table overflows the right page edge; the `allocatedAmount` cell is the worst offender because it carries an inline deposit badge + footnote markers. (b) `TABLE_LAYOUT` in `shared.ts` never sets `dontBreakRows`, so multi-line rows orphan across page breaks. (c) `buildPageHeader` renders ~60pt of content (14pt bold + 12pt subheader at `lineHeight: 1.4`, plus a 20pt bottom margin) into a **40pt** `pageMargins` top band → clipped and overlapping on pages 2+. ACs are outcome-focused; fixes are not prescribed. -- **#1930 — Attachment tier rules per report type** (`user-story`, Should Have). Replaces the per-invoice stage matching in `sourceReportService.ts` step h (~L286–339). **Tier order quotation(1) → deposit(2) → invoice(3); floors: budget-overview=1, claim=2, proof-of-funds=3; embed at-or-above the floor.** Depends only on report type + document type — no longer on invoice status, deposit split, or `targetStatuses`. **PR #1942 APPROVED round 1 (2026-08-02)**, all 11 AC met, 80/80 green. **But see #1943** — the architect found a frontend route that reaches AC2's forbidden outcome without violating AC2: `handleUseCaseChange` (`ReportWizardPage.tsx` L198–224) never clears `report`/`reportStatus`/`sourceId`, and step 2's Next is gated on `disabled={!sourceId}` (L686), which survives. Switching **budget-overview → claim** and clicking through carries a report filtered at the *budget-overview* tier floor into a claim export → **quotations embedded in a claim PDF handed to a bank**. Pre-existing (staled invoice slice + totals all along), but #1930 raised the consequence from a reconciliation error to an evidentiary one. Filed `bug` / **Must Have** / Todo, 2026-08-02; cross-referenced on #1930 (`issuecomment-5158312814`). **My ruling, recorded so it isn't re-litigated: clear `sourceId` too**, not just `report` — clearing `report` alone leaves the `!sourceId` gate satisfied, trading a stale-data bug for an empty-state bug. Clearing `sourceId` restores "step 3 is reachable only after an explicit source selection under the current use case", the same invariant step 1→2 already enforces; the extra click lands on a source list whose amounts were just re-fetched for the new use case. Watch the `?sourceId=` deep-link effect (L255–260, keyed on `!report`) — clearing `report` re-arms it, so #1943 AC8 requires that interaction be reasoned about explicitly. **Generalisable lesson: an AC that constrains a server-side derivation is not satisfied until the client is proven to re-derive it whenever its inputs change — check the state-reset paths, not just the computation.** +- **#1930 — Attachment tier rules per report type** (`user-story`, Should Have). Replaces the per-invoice stage matching in `sourceReportService.ts` step h (~L286–339). **Tier order quotation(1) → deposit(2) → invoice(3); floors: budget-overview=1, claim=2, proof-of-funds=3; embed at-or-above the floor.** Depends only on report type + document type — no longer on invoice status, deposit split, or `targetStatuses`. **PR #1942 APPROVED round 1 (2026-08-02)**, all 11 AC met, 80/80 green. **But see #1943** — the architect found a frontend route that reaches AC2's forbidden outcome without violating AC2: `handleUseCaseChange` (`ReportWizardPage.tsx` L198–224) never clears `report`/`reportStatus`/`sourceId`, and step 2's Next is gated on `disabled={!sourceId}` (L686), which survives. Switching **budget-overview → claim** and clicking through carries a report filtered at the _budget-overview_ tier floor into a claim export → **quotations embedded in a claim PDF handed to a bank**. Pre-existing (staled invoice slice + totals all along), but #1930 raised the consequence from a reconciliation error to an evidentiary one. Filed `bug` / **Must Have** / Todo, 2026-08-02; cross-referenced on #1930 (`issuecomment-5158312814`). **My ruling, recorded so it isn't re-litigated: clear `sourceId` too**, not just `report` — clearing `report` alone leaves the `!sourceId` gate satisfied, trading a stale-data bug for an empty-state bug. Clearing `sourceId` restores "step 3 is reachable only after an explicit source selection under the current use case", the same invariant step 1→2 already enforces; the extra click lands on a source list whose amounts were just re-fetched for the new use case. Watch the `?sourceId=` deep-link effect (L255–260, keyed on `!report`) — clearing `report` re-arms it, so #1943 AC8 requires that interaction be reasoned about explicitly. **Generalisable lesson: an AC that constrains a server-side derivation is not satisfied until the client is proven to re-derive it whenever its inputs change — check the state-reset paths, not just the computation.** - **#1931 — Single "Enhance with AI" action + purpose-focused prompt** (`user-story`, Should Have). Drops the step-4 "Enable AI assistance" toggle entirely (it gated nothing but a button), renders one button when `llmEnabled`, relabels "Generate with AI" → "Enhance with AI", and rewrites the prompt to explain **why** each cost was incurred rather than restating the table columns. - **#1932 — Cover letter overhaul** (`user-story`, Should Have). Formatted body (no markdown lib in `client/package.json` today — deliberately left as an architect/UX decision), explicit editable signature field + signature block, sender = user `displayName` + household address, professional letter layout, and the oversized reset-`X` fix. - **#1933 — Select Invoices step UI fixes** (`bug`, Should Have). Wrong glyph, no open-invoice affordance, misaligned select-all, misaligned deposit dates cell. ### Rulings made while writing these — do not re-litigate -- **`attachmentType: null` = tier `invoice`** (#1930). Rationale: nulls are legacy/ambiguous, not known-weak evidence — the invoice-creation Paperless picker hard-sets `'invoice'`, so nulls come from pre-#1877 links and from users skipping the type choice. Treating null as the *lowest* tier would silently drop evidence from claim/proof-of-funds reports for existing data, which is worse than being over-inclusive (the user can deselect). Treating it as tier 3 is exactly no-regression while still stopping typed quotations from reaching claim reports. **This supersedes #1888's deferred design question** — #1888 stays open but is re-scoped to indicator *presentation* only. +- **`attachmentType: null` = tier `invoice`** (#1930). Rationale: nulls are legacy/ambiguous, not known-weak evidence — the invoice-creation Paperless picker hard-sets `'invoice'`, so nulls come from pre-#1877 links and from users skipping the type choice. Treating null as the _lowest_ tier would silently drop evidence from claim/proof-of-funds reports for existing data, which is worse than being over-inclusive (the user can deselect). Treating it as tier 3 is exactly no-regression while still stopping typed quotations from reaching claim reports. **This supersedes #1888's deferred design question** — #1888 stays open but is re-scoped to indicator _presentation_ only. - **Server-side single filter** (#1930 AC7). `merge.ts` embeds whatever `invoice.documents` holds and `ReportInvoiceList` lights on `documents.length > 0`, so filtering once server-side makes step 3 and the PDF agree for free. Never add a second client-side document filter. ### #1930 shipped — PR #1942 APPROVED (2026-08-02, round 1) All 11 ACs met on head `4dfce4b8`; 80/80 tests green. Implementation is `server/src/services/shared/attachmentTierUtils.ts` (`ATTACHMENT_TIER`, `REPORT_TYPE_TIER_FLOOR`, `isDocumentIncludedForReportType`) — the single site for both the ordering and the floors. `splitByDepositsExcludingTagged` is gone from `sourceReportService`'s document path (still used by `budgetSourceService` for amounts — the #1930 Notes' "do not delete it" meant the util, not the local variable). Wiki `API-Contract.md` @ `a9b6e9e`. -- **QA deviation accepted**: AC1's table-driven scenario uses a *fresh invoice per report-type block* rather than one shared invoice queried three times. Correct call — no single invoice status sits in all three target slices (proof-of-funds needs `claimed`, which the claim slice excludes), so a shared fixture would have varied invoice-selection, the wrong variable. Status-invariance is proven separately by the `AC5` test. **General rule: when a table-driven test can't hold every variable constant, isolate the variable under test per block and prove the invariance claim in its own named test.** +- **QA deviation accepted**: AC1's table-driven scenario uses a _fresh invoice per report-type block_ rather than one shared invoice queried three times. Correct call — no single invoice status sits in all three target slices (proof-of-funds needs `claimed`, which the claim slice excludes), so a shared fixture would have varied invoice-selection, the wrong variable. Status-invariance is proven separately by the `AC5` test. **General rule: when a table-driven test can't hold every variable constant, isolate the variable under test per block and prove the invariance claim in its own named test.** - **Non-change-detecting tests are acceptable when the contract is asserted correctly** (informational finding I1). The proof-of-funds blocks of `scenario 16` and the `AC3` test would also have passed on `beta` (old stage derivation for a `claimed` no-deposit invoice also produced `stages={invoice}`). Flagged, not blocked — the ACs describe outcomes, and change-detection lives in the unit test plus AC1's budget-overview/claim blocks. - **#1888 re-scope APPLIED** (issue body rewritten 2026-08-02, was still stale at review time). Null-handling AC replaced by a pointer to the tier ruling; the "attached but not stage-matched" third state struck (non-qualifying docs never reach the client now); a "no client-side filtering" AC added to protect #1930 AC7; coordination note with #1933 (same glyph) added. **Lesson: a supersedes-ruling written into issue A does not update issue B — apply the re-scope to B's body at the same time, or it will be found stale at review.** - **#1909's "signature derived from sender" acceptance is REVERSED** (#1932). It was accepted at review time on the reasoning that `sender.split('\n')[0]` (the household name) was an adequate signatory; the user saw the output and rejected it. Record reversals like this rather than re-arguing them. - **#1925 closes as a duplicate of #1932** when #1932 lands; its ACs are carried forward verbatim as #1932 section 6. #1925's own Notes already anticipated this. - **The `Konstruktionsprojekt` prompt nit moves from #1917 to #1931.** `buildReportContentUserPrompt` L153 inverts the language ternary and is wrong in both branches; #1931 rewrites that prompt wholesale. #1917 keeps everything else, incl. the M2 `computeIncludedTotal` extraction and the `KI` glossary entry. -- **Prompt/validator cap divergence** (#1931). Prompt states 150/2000/200 (subject/body/description); `openAICompatibleProvider.ts` truncates at 200/3000/300. Resolution: one shared definition, effective values **150/2000/200** — the tighter set, partly because long descriptions aggravate #1929's Usage-column overflow. But #1929's ACs must hold at *any* length, since the step-5 editor is unbounded; neither issue may lean on the other. **Reaffirmed unchanged 2026-08-02** when ruling on #1929's AC conflict — a UI `maxLength` was explicitly rejected as the safety mechanism (see next entry). +- **Prompt/validator cap divergence** (#1931). Prompt states 150/2000/200 (subject/body/description); `openAICompatibleProvider.ts` truncates at 200/3000/300. Resolution: one shared definition, effective values **150/2000/200** — the tighter set, partly because long descriptions aggravate #1929's Usage-column overflow. But #1929's ACs must hold at _any_ length, since the step-5 editor is unbounded; neither issue may lean on the other. **Reaffirmed unchanged 2026-08-02** when ruling on #1929's AC conflict — a UI `maxLength` was explicitly rejected as the safety mechanism (see next entry). ### #1929 AC2-vs-AC4 conflict — ruling of 2026-08-02 (issue comment `5156932089`) PR #1935 got CHANGES_REQUIRED from both `product-architect` and `ux-designer`. Architect measured real pdfmake 0.3.11 renders: `dontBreakRows` on an unbreakable row **taller than one page** makes pdfmake **silently drop the row's content** (cliff ≈ 475 chars in the 7-col shape, flat at 14 text-show ops from 500 chars to 3000). So AC2 ("no characters dropped") and AC4 ("no row split across pages") were mutually exclusive at unbounded length. Ruling, now in the issue body: -- **Precedence ladder replaces flat peer ACs.** I1 no character lost > I2 nothing outside the printable area > I3 row stays on one page > I4 no word broken. Lower number wins on conflict. **I3 yields to I1** (option (a)): a row that *can* fit one page is never split; a row that genuinely cannot may span pages, but content is never silently dropped. Rationale: a split row is visible and recoverable (repeating header + reconcilable totals); a dropped row is undetectable in a document handed to a bank. Integrity > presentation. -- **AC2's "no word cut mid-word" was over-broad and got rewritten.** It described the *clipping* defect, not typographic line-breaking. As written it created a second latent contradiction: a pdfmake `'*'` column never renders below its widest word's width (`columnCalculator.js:66-75`), and `Wärmedämmverbundsystem` = 128pt at 10pt Roboto, so AC1 was unsatisfiable for unbounded German compounds. **Now: a word may be broken across lines iff it is wider than its column alone, losing no character.** This is what made the contract satisfiable — the general lesson is to check whether an AC forbids a legitimate mechanism while trying to forbid a defect. +- **Precedence ladder replaces flat peer ACs.** I1 no character lost > I2 nothing outside the printable area > I3 row stays on one page > I4 no word broken. Lower number wins on conflict. **I3 yields to I1** (option (a)): a row that _can_ fit one page is never split; a row that genuinely cannot may span pages, but content is never silently dropped. Rationale: a split row is visible and recoverable (repeating header + reconcilable totals); a dropped row is undetectable in a document handed to a bank. Integrity > presentation. +- **AC2's "no word cut mid-word" was over-broad and got rewritten.** It described the _clipping_ defect, not typographic line-breaking. As written it created a second latent contradiction: a pdfmake `'*'` column never renders below its widest word's width (`columnCalculator.js:66-75`), and `Wärmedämmverbundsystem` = 128pt at 10pt Roboto, so AC1 was unsatisfiable for unbounded German compounds. **Now: a word may be broken across lines iff it is wider than its column alone, losing no character.** This is what made the contract satisfiable — the general lesson is to check whether an AC forbids a legitimate mechanism while trying to forbid a defect. - **Rejected (b) truncation** (data loss on a bank document, politely announced) and **(c) a step-5 `maxLength`** — a UI cap is a UX affordance, not a correctness guarantee: it doesn't close the hole (vendor name + area line + attachments can still overflow a row), it isn't the only ingress (#1901 AI generation), and it puts a renderer invariant two layers away. A soft counter/hint is fine as separate future work — deliberately **not** filed. - **Targets set** (7-col shape, worst-case other columns, measured not estimated): **600 chars** of German prose with zero degradation (3× #1931's 200 target, 2× the 300 validator cap); Usage column fits **~30 chars of German prose per line** (69.28pt / ~14 chars is a collapsed column in product terms); table body font floor **8pt**. -- **Permitted levers widened, scope unchanged** (still presentation-layer only): padding, border widths, table body font down to 8pt, column widths, fixed-vs-star, page margins, and the row's internal layout — including taking the usage stack out of the 7-column grid into a full-width sub-row. The column grid is *not* fixed by the issue. 7 real columns + a prose column on A4 portrait is genuinely tight; say so explicitly or the implementer assumes the grid is a constraint. +- **Permitted levers widened, scope unchanged** (still presentation-layer only): padding, border widths, table body font down to 8pt, column widths, fixed-vs-star, page margins, and the row's internal layout — including taking the usage stack out of the 7-column grid into a full-width sub-row. The column grid is _not_ fixed by the issue. 7 real columns + a prose column on A4 portrait is genuinely tight; say so explicitly or the implementer assumes the grid is a constraint. - **No continuation marker** on split rows — needs page-aware rendering, too much risk on a blocking Must Have, case is rare once columns are right. -- **New ACs**: AC12 (measure the ceiling from real renders, record it in the issue *and* a code comment, pin with boundary tests both sides), AC13 (running header survives an unbounded `sourceName` — architect's MEDIUM 5), AC14 (falsy-`statusText` malformed-row crash at `overviewPdf.ts` L~160, verified: 6 cells pushed against a 7-entry `widths`). AC11 strengthened: config-only assertions don't satisfy it; AC1–AC4 each need a real-render assertion. **Fix order is part of the contract**: geometry first, *then* the unbreakable-rows flag, then the residual over-tall row — reversing it converts a visible defect into silent data loss. +- **New ACs**: AC12 (measure the ceiling from real renders, record it in the issue _and_ a code comment, pin with boundary tests both sides), AC13 (running header survives an unbounded `sourceName` — architect's MEDIUM 5), AC14 (falsy-`statusText` malformed-row crash at `overviewPdf.ts` L~160, verified: 6 cells pushed against a 7-entry `widths`). AC11 strengthened: config-only assertions don't satisfy it; AC1–AC4 each need a real-render assertion. **Fix order is part of the contract**: geometry first, _then_ the unbreakable-rows flag, then the residual over-tall row — reversing it converts a visible defect into silent data loss. - **Process lesson**: both this round's CRITICAL findings and my own AC conflict came from configuration asserted in a comment rather than measured against a real render (`dontBreakRows` on `layout` where pdfmake never reads it; a Usage width documented as 185.28pt that renders at 69.28pt because pdfmake subtracts ~116pt of cell offsets first). For any PDF/layout AC, require the assertion to be made against the rendered result. ## #1929 closed — PR #1935 merged 2026-08-02 (squash `1c5aa62c`), 4 rounds, 5 follow-ups filed Merged after four implementation rounds. Both `product-architect` and `ux-designer` reviewed by **rendering and rasterizing real PDFs** (throwaway Jest test → `/tmp` blob → `pdftoppm -r 150/300` → inspect PNGs), not by reading config — that technique is what caught every round's defect and is now the standard for any PDF-layout review here. -**The four-round arc, as a generalisable lesson** (architect, round 3): *"every cell that can hold unbounded text needs the cap, not just the first one that was noticed. Round 1 capped nothing, round 2 capped the wrong quantity (average glyph + perfect packing), round 3 capped the right quantity in the wrong scope (one field of a multi-field cell)."* Round 4 finally capped the right quantity at cell scope. When an AC is about a bound, ask **what quantity, at what scope** before accepting the fix. +**The four-round arc, as a generalisable lesson** (architect, round 3): _"every cell that can hold unbounded text needs the cap, not just the first one that was noticed. Round 1 capped nothing, round 2 capped the wrong quantity (average glyph + perfect packing), round 3 capped the right quantity in the wrong scope (one field of a multi-field cell)."_ Round 4 finally capped the right quantity at cell scope. When an AC is about a bound, ask **what quantity, at what scope** before accepting the fix. Final state worth knowing: table width is now **exactly 515.28pt, unfalsifiable by input** (no `'*'` column left; 22 pathological cases all identical to the hundredth). `MAX_SAFE_USAGE_CHUNK_CHARS = 650`, `MAX_SAFE_SMALL_CHUNK_CHARS = 450`, `PAGE_TOP_MARGIN = 75`, table body font 8pt, `VENDOR_WIDTH = 45pt`. AC12's 600-char zero-degradation guarantee holds. ### Follow-ups filed 2026-08-02 (all parentless, Bank Report Wizard cluster) -- **#1937 — German header labels break mid-word** (`bug`, Should Have, **Todo**). `Auftragnehmer` 67.50pt in a 45pt column, `Rechnungsbetrag` 78.66pt in 48pt. pdfmake 0.3.11 has no hyphenation mode; widening was **measured and rejected** (drops Usage to ~79pt, fails AC3's ~30-chars-per-line floor). Fix is at the **i18n layer** — 2 finite translator-owned strings in 1 locale, not an engineering fix. Ranked near-term because it shows on *every page of every German report*, unconditionally. +- **#1937 — German header labels break mid-word** (`bug`, Should Have, **Todo**). `Auftragnehmer` 67.50pt in a 45pt column, `Rechnungsbetrag` 78.66pt in 48pt. pdfmake 0.3.11 has no hyphenation mode; widening was **measured and rejected** (drops Usage to ~79pt, fails AC3's ~30-chars-per-line floor). Fix is at the **i18n layer** — 2 finite translator-owned strings in 1 locale, not an engineering fix. Ranked near-term because it shows on _every page of every German report_, unconditionally. - **#1938 — running header `generated at` label with no timestamp on pages 2+** (`bug`, Should Have, **Todo**). `merge.ts` L163–167 passes only `t('sourceReports.table.generatedAt')`; page 1 does it right at `overviewPdf.ts` L333 (`${label}: ${generatedAtText}`). **Pre-existing**, verified against `origin/beta` — not a #1929 regression. -- **#1939 — reportPdf geometry hygiene** (`tech-debt`, Should Have, **Todo**, **blocks #1932**). `HEADER_ROW_HEIGHT` → `HEADER_ROW_HEIGHT_MAX` (exports 68pt vs measured 45.81pt — correct *bound*, wrong *estimate*, and #1932 could under-fill whole pages reading it as typical); scope the `WORST_CASE_CHAR_ADVANCE_EM` comment (overclaimed at 0.89 and again at 1.04 — a 3,919-codepoint sweep found Cyrillic `Ѹ` U+0478 at 1.1611em; **value stays 1.04**, raising it drops the 7-col threshold 19→16 chars and breaks more German compounds); enumerate cell-content channels; relocate `PDF_STYLES` **down** into the geometry layer. -- **#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. +- **#1939 — reportPdf geometry hygiene** (`tech-debt`, Should Have, **Todo**, **blocks #1932**). `HEADER_ROW_HEIGHT` → `HEADER_ROW_HEIGHT_MAX` (exports 68pt vs measured 45.81pt — correct _bound_, wrong _estimate_, and #1932 could under-fill whole pages reading it as typical); scope the `WORST_CASE_CHAR_ADVANCE_EM` comment (overclaimed at 0.89 and again at 1.04 — a 3,919-codepoint sweep found Cyrillic `Ѹ` U+0478 at 1.1611em; **value stays 1.04**, raising it drops the 7-col threshold 19→16 chars and breaks more German compounds); enumerate cell-content channels; relocate `PDF_STYLES` **down** into the geometry layer. +- **#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). **Amended to rev 2 on 2026-08-06 — see §"#1941 rev 2" below.** Input-side gap only; round 4 + #1940 removed the correctness risk. + +### #1941 rev 2 — the length limits, set 2026-08-06 (before implementation) + +Title changed (`attachmentsNote et al.` dropped); body rebuilt; **AC numbering unchanged, AC1–AC9 still mean the same things**. `attachmentsNote` is dead as an override key (removed in `217cb408`, static since #1959) — it and `areaText` are now both derived read-only text bounded only by chunking. `EditableField` has **exactly one consumer** (`ReportContentEditor.tsx`) and seven editable values; `recipient` and `reference` are **conditionally rendered**. + +Limits, all anchored on a measured constant or an existing server cap: `usageText` **500**, `reference` **100**, `subject` **200**, `signature` **200**, `sender` **300**, `recipient` **300**, `body` **4000**. + +- **`usageText` 500 overrides the ux-designer's suggested 120–150.** Its baseline is _derived_, not typed — `getUsageText()` joins `workItems.title`/`householdItems.name` (≤500) or `invoiceBudgetLines.description` (≤500) across N budget lines, so **a single line already admits 500 legal chars**. A cap of 150 would flag ordinary invoices over-limit on load, making the exception state routine and turning the near-limit counter into permanent orange furniture — defeating the designer's own 90%-gating while nominally satisfying it. Bracketed below by 500 and above by `MAX_SAFE_USAGE_CHUNK_CHARS` (650, the whole Usage cell's shared budget), leaving 150 for the derived suffix. +- **`body` 4000 needed no architect round.** `buildCoverLetterContent()` emits plain flowing paragraphs — no table, no `dontBreakRows`, no fixed-height container — so pdfmake paginates natively and an over-long body just makes _more pages_. **No capacity ceiling exists to ask about**; the question was purely "how long should a bank cover letter be", i.e. mine. The realistic runaway is the AI path (`LLM_MAX_TOKENS` 16384 ≈ 60k chars), not a verbose human. + +**AC5 is vacuous and now says so.** Overrides live only in `wizardReducer` (no localStorage/sessionStorage/API); no report-content override concept exists in `server/src` or `shared/src`. Retained with a reactivation condition (if wizard state ever gains persistence) **plus an explicit prohibition** — the live risk is the inverse of the original one: an implementer could build server validation for fields that never arrive. + +**Chasing AC5 exposed a defect in AC4**: "existing **saved** value" is unconstructible, since overrides never persist. The two real over-limit-on-load sources are **baseline** values — AI-generated cover letter, and derived `usageText`. AC4 now names them. + +**Accessibility ruling — there is no pre-existing gap.** The `ariaDescribedBy` ternary→filtered-array refactor is **in scope and mandatory** (AC8 can't compose two ids otherwise; widens the Could Have by zero). Extending `editedHintId` into dense mode is **rejected as a regression**: dense mode already conveys the edited state via the accessible _name_ (`EditableField.tsx:51-55` appends `editedSuffix` to `aria-label`) while labelled mode uses the _description_ — two correct mechanisms for two contexts, so adding the hint would announce "edited" twice. No follow-up issue filed, and none should be. ### #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. +`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. +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. + +**Rev 2 (2026-08-06, on PR #2038): AC 1.5 STRUCK, nothing replaces it.** `MAX_SAFE_SMALL_CHUNK_CHARS` was deleted by #1959 (`3cc89676`, 2026-08-03) — the day *after* #1950 was filed, so the AC was correct when written. The Usage cell now has **one** budget at `TABLE_BODY_FONT_SIZE`; there is no second ceiling to hold a margin against. Derived-507 computation retained as a `TABLE_SMALL_FONT_SIZE` **staleness pin only** (AC 1.2/1.7), asserting nothing about safety. No replacement assertion: drift is covered by that computation, deliberate 9pt re-adoption by the two doc comments (AC 2.1 protects them). **AC 1.4a/b/c added** for #1940's continuation rows — overage there is **36 / 4 / 44.8**, not 34 / 3 / 33.6, because the `'… '` marker is applied post-packing in `buildUsageCell` (`overviewPdf.ts:894-907`) and no budget counts it; 1.4b pins the marker-shorter-than-a-line precondition, 1.4c **prohibits** "fixing" it by counting the marker (would change I1 reconstruction). Existing AC numbers unchanged. Stale #1941 cross-ref in Notes withdrawn. 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. +- **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. -- **Vendor *data* breaking mid-word was recorded as an accepted limitation in #1937's Notes, not filed.** `ux-designer` round 4: at 45pt/8pt any 14+ char word breaks, and German trade names compound freely (`Rückerstattung` → `Rück`/`erstattung`) — a non-trivial minority of realistic names. Not filed because it is unbounded user data, AC2 permits it, nothing is lost, and the only lever (widening Vendor) costs Usage width and breaks AC3. Revisiting it needs a layout change, not a width tweak. -- **`PDF_STYLES` relocation had been deferred *to* #1932 in the round-3 review but never entered #1932's ACs** — it now lives in #1939 §4 so it isn't lost. Watch for this pattern: "we'll handle it in issue X" is only real if it lands in X's acceptance criteria. +- **Vendor _data_ breaking mid-word was recorded as an accepted limitation in #1937's Notes, not filed.** `ux-designer` round 4: at 45pt/8pt any 14+ char word breaks, and German trade names compound freely (`Rückerstattung` → `Rück`/`erstattung`) — a non-trivial minority of realistic names. Not filed because it is unbounded user data, AC2 permits it, nothing is lost, and the only lever (widening Vendor) costs Usage width and breaks AC3. Revisiting it needs a layout change, not a width tweak. +- **`PDF_STYLES` relocation had been deferred _to_ #1932 in the round-3 review but never entered #1932's ACs** — it now lives in #1939 §4 so it isn't lost. Watch for this pattern: "we'll handle it in issue X" is only real if it lands in X's acceptance criteria. - **Not filed:** the page-1 `PAGE_TOP_MARGIN = 93pt` blank gap above the cover-letter sender block — already inside #1932 AC 4.1; flagged on #1932 rather than duplicated. - `addBlockedBy(#1932 ← #1939)` set, plus a prominent sequencing comment on #1932 (`issuecomment-5158212341`) covering the block, the `PDF_STYLES` direction constraint (`pageGeometry.ts` must **never** import `merge.ts` — that edge already runs the other way), and the #1941/#1938 shared-ground warnings. @@ -244,9 +261,9 @@ AC 3.2/3.3 assert **live model output quality** ("reads as a purpose statement", ### Other rulings -- **"Mit KI verbessern" accepted for AC 2.3.** My AC deliberately did not prescribe the string ("an equivalent in German that uses 'KI', consistent with existing `de` copy") — wording is `ux-designer`/`translator` territory. *verbessern* (improve existing) over *überarbeiten* (rework) is right and matches the English: the whole point of renaming Generate→Enhance was that the action improves content that already exists; *überarbeiten* would reintroduce in German the overstatement removed in English. +- **"Mit KI verbessern" accepted for AC 2.3.** My AC deliberately did not prescribe the string ("an equivalent in German that uses 'KI', consistent with existing `de` copy") — wording is `ux-designer`/`translator` territory. _verbessern_ (improve existing) over _überarbeiten_ (rework) is right and matches the English: the whole point of renaming Generate→Enhance was that the action improves content that already exists; _überarbeiten_ would reintroduce in German the overstatement removed in English. - **Unconditional `aria-describedby` description accepted as in-scope** though not literally in an AC: deleting the checkbox deleted its helper text, which was the only place overwrite behaviour was explained. Dirty-gating it would hide the warning from the user who most needs it. -- **Good AC-writing pattern to repeat**: AC 4.1 asked for "exactly one definition that both sides derive from". `contentLimits.test.ts` satisfied it by building its expected substrings *by interpolating the constant*, never typing the literal — so a hardcoded number reappearing in `prompts.ts` fails the assertion instead of silently passing. Ask for derivation, not equality. +- **Good AC-writing pattern to repeat**: AC 4.1 asked for "exactly one definition that both sides derive from". `contentLimits.test.ts` satisfied it by building its expected substrings _by interpolating the constant_, never typing the literal — so a hardcoded number reappearing in `prompts.ts` fails the assertion instead of silently passing. Ask for derivation, not equality. - Non-blocking follow-ups left on the PR (not filed): user-prompt tail still says `letterBody` "summarizing the report" (old framing, weaker instruction sitting closer to the output — first suspect if UAT 3.4 fails); stale E2E locator name `generateWithAiButton` vs the "Enhance with AI" accessible name. ### #1917 bookkeeping done @@ -259,29 +276,29 @@ AC 3.2/3.3 assert **live model output quality** ("reads as a purpose statement", ### The M1 precedent: a finding that defeats the PR's own AC is not a follow-up -M1 was an untokenized `getSourceReport` race — an in-flight fetch from the *previous* use case could win an out-of-order resolution and re-populate `report` while step 3 was reachable, reaching **exactly the #1943 end state** (claim export embedding quotation-tier docs). Filing it would have marked #1943's AC1 met while a live route to the headline defect remained open. Fixed in-PR with a monotonic `reportRequestRef` token (`ReportWizardPage.tsx` L146/229/269/274/281). **Rule: a review finding that defeats an AC of the story under review belongs in that story's PR, regardless of the reviewer's medium/low severity label** — reviewer severity answers "does this block merge", not "is the AC actually met". +M1 was an untokenized `getSourceReport` race — an in-flight fetch from the _previous_ use case could win an out-of-order resolution and re-populate `report` while step 3 was reachable, reaching **exactly the #1943 end state** (claim export embedding quotation-tier docs). Filing it would have marked #1943's AC1 met while a live route to the headline defect remained open. Fixed in-PR with a monotonic `reportRequestRef` token (`ReportWizardPage.tsx` L146/229/269/274/281). **Rule: a review finding that defeats an AC of the story under review belongs in that story's PR, regardless of the reviewer's medium/low severity label** — reviewer severity answers "does this block merge", not "is the AC actually met". ### #1946 (bug, **Must Have**, Todo) — in-flight AI generation survives a use-case change -`runAiGeneration` resolves into `setAiContent(result)`; while generating, `aiContent` is `null`, so `guardedUpdate`'s dirty predicate is false and a use-case change applies **with no confirmation**. Post-#1931 the prompt is purpose-focused and the request carries `type: useCase`, so the landed result is narrative written for the **wrong report purpose** — the architect's point that the *mechanism* is symmetric with a source change but the *consequence* is not. +`runAiGeneration` resolves into `setAiContent(result)`; while generating, `aiContent` is `null`, so `guardedUpdate`'s dirty predicate is false and a use-case change applies **with no confirmation**. Post-#1931 the prompt is purpose-focused and the request carries `type: useCase`, so the landed result is narrative written for the **wrong report purpose** — the architect's point that the _mechanism_ is symmetric with a source change but the _consequence_ is not. - **Must Have despite the architect's "Medium"** — recorded on the issue to stop re-litigation. Medium-for-PR and MoSCoW are different scales; the architect explicitly asked for it before the cluster's `beta`→`main` promotion. Same blast radius as #1943/#1929: credibility of a bank-facing artifact. -- **Product ruling on the discard question (option b of three)**: widen `guardedUpdate`'s dirty predicate to include `isGeneratingAi` → the confirmation runs; confirm invalidates the in-flight request via a token, cancel lets it finish. Rejected (a) silent invalidation — generation is slow (visible elapsed timer) and metered, the guard exists to protect content that isn't cheap to recreate, and an in-flight generation *is* that, just not arrived. Rejected (c) block-until-settled — freezes the wizard for up to `LLM_REQUEST_TIMEOUT_MS` and invents a trapped-behind-a-hung-request failure mode. **Never trap a user behind a network call they can't cancel.** +- **Product ruling on the discard question (option b of three)**: widen `guardedUpdate`'s dirty predicate to include `isGeneratingAi` → the confirmation runs; confirm invalidates the in-flight request via a token, cancel lets it finish. Rejected (a) silent invalidation — generation is slow (visible elapsed timer) and metered, the guard exists to protect content that isn't cheap to recreate, and an in-flight generation _is_ that, just not arrived. Rejected (c) block-until-settled — freezes the wizard for up to `LLM_REQUEST_TIMEOUT_MS` and invents a trapped-behind-a-hung-request failure mode. **Never trap a user behind a network call they can't cancel.** - **Widen the predicate inside `guardedUpdate` itself, not per-handler** — every caller mutates an AI-request input or the baseline; one place makes the invariant structural, which is the whole lesson of #1943. Closes the symmetric source-change variant for free. - AC12 explicitly **forbids new E2E**: a late-resolving stale response is invisible to the assertions (architect's own analysis of scenarios 13/14). Deterministic unit test with controlled promise resolution, not a timing spec. ### #1947 (tech-debt, **Should Have**, Backlog, blocked-by #1946) — `useReducer` refactor -1,156 lines / 38 hooks; the "what a transition invalidates" invariant is hand-maintained across two handlers. **Filed as its own issue, NOT folded into #1912** — #1912 is a Could Have grab-bag of cosmetic nits; folding a state-machine refactor in would bury it behind a Could Have label, make #1912 un-sizable, and lose the evidence trail, which *is* the justification. +1,156 lines / 38 hooks; the "what a transition invalidates" invariant is hand-maintained across two handlers. **Filed as its own issue, NOT folded into #1912** — #1912 is a Could Have grab-bag of cosmetic nits; folding a state-machine refactor in would bury it behind a Could Have label, make #1912 un-sizable, and lose the evidence trail, which _is_ the justification. -- **The evidence table is the argument**: one handler produced four defects of one shape in one batch — #1943 (transition didn't invalidate state it owned), its AC8 deep-link second-order effect (the fix created the next bug), M1 (pending write re-populated cleared state), M2/#1946 (same, plus the guard couldn't see what it guards). Two of the four were *caused by the patch before them*. Put that table in any future "should we refactor" argument. -- **Should Have, with a checkable trigger instead of a vague "soon"**: *the next change that adds transition-owned state to this component should be preceded by this refactor.* Raise at refinement if a report-wizard state story lands while it's open. +- **The evidence table is the argument**: one handler produced four defects of one shape in one batch — #1943 (transition didn't invalidate state it owned), its AC8 deep-link second-order effect (the fix created the next bug), M1 (pending write re-populated cleared state), M2/#1946 (same, plus the guard couldn't see what it guards). Two of the four were _caused by the patch before them_. Put that table in any future "should we refactor" argument. +- **Should Have, with a checkable trigger instead of a vague "soon"**: _the next change that adds transition-owned state to this component should be preceded by this refactor._ Raise at refinement if a report-wizard state story lands while it's open. - Architect's **L3** (`deepLinkAppliedRef` boolean → `useRef`) folded in as a nice-to-have per coordinator — the applied id is immutable for the component's lifetime (sole `?sourceId=` producer is a cross-route `navigate()` from `BudgetSourcesPage.tsx:1318`), so a boolean is sufficient today. ### #1943 body amended + audit comment (`issuecomment-5159716825`) -- **AC4 reworded — original was unsatisfiable by design.** "always identical to a clean start" is violated by `attachDocuments` and `reportLanguageOverride`, which are *correctly* sticky. **A UAT tester reading it literally would have failed the story for working as designed.** Now scoped to the `getSourceReport` payload, the exclusion sets, and the tier floor, with the two preferences named as out-of-scope. **Pattern to watch: an equivalence AC must name what is excluded, or sticky user preferences will read as failures.** -- **AC5 enumeration completed — `skippedDocuments` and `aiError` were omitted, both ruled CLEAR** (not KEEP, against the architect's "probably fine"). `skippedDocuments` is only overwritten on a *successful* generation, so a later failure re-displays the previous report's warnings against a new report — in a bank-facing flow. `aiError` can hold `EMPTY_SELECTION`, raised from exclusion sets this very reset clears, so it's guaranteed inapplicable. Both one-line, zero reachability risk. +- **AC4 reworded — original was unsatisfiable by design.** "always identical to a clean start" is violated by `attachDocuments` and `reportLanguageOverride`, which are _correctly_ sticky. **A UAT tester reading it literally would have failed the story for working as designed.** Now scoped to the `getSourceReport` payload, the exclusion sets, and the tier floor, with the two preferences named as out-of-scope. **Pattern to watch: an equivalence AC must name what is excluded, or sticky user preferences will read as failures.** +- **AC5 enumeration completed — `skippedDocuments` and `aiError` were omitted, both ruled CLEAR** (not KEEP, against the architect's "probably fine"). `skippedDocuments` is only overwritten on a _successful_ generation, so a later failure re-displays the previous report's warnings against a new report — in a bank-facing flow. `aiError` can hold `EMPTY_SELECTION`, raised from exclusion sets this very reset clears, so it's guaranteed inapplicable. Both one-line, zero reachability risk. - **Carried as #1946 AC9/AC10, not by reopening #1943** — the PR is approved and neither is a defect in what it shipped; the gap was in the enumeration, which now lives on the issue. Said so explicitly on the comment so the addendum doesn't read as goalposts moving after approval. ## #1933 ACs 2.1/2.7 corrected 2026-08-02 — AC described a layout that doesn't exist @@ -296,7 +313,7 @@ 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. +- 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 @@ -306,13 +323,13 @@ Comments: `issuecomment-5160251632` (decision), `issuecomment-5160258752` (AC ch ### 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**. +**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 +### 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.** @@ -329,7 +346,7 @@ Deleting a vacuous AC reads as an oversight and invites re-litigation; striking 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. +- **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 @@ -350,15 +367,15 @@ All 40 ACs met on `c17d9d44` + the locally-committed E2E follow-up `d60a98b3`. P ### 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. +- **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. +**#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) @@ -381,10 +398,10 @@ Architect's PR #1951 review ([comment 5160566459](https://github.com/steilerDev/ ### Rulings worth reusing -- **Coerce-vs-reject at an LLM response boundary: match the policy the field already has.** Ruled #1952 as *strip*, not *reject*. Decisive argument was **blast radius per call**: one generation produces subject + body + every per-invoice description, so rejecting over two asterisks discards unrelated correct output, costs a second paid round-trip, and may fail identically on retry with the same model. Reinforcing: the validator already *truncates* over-length values on these very fields, so adding a harsher policy for a *milder* violation is incoherent; and the fields are human-editable in the preview, which makes repair-and-continue the right default with the human as backstop. Generalizable: **at an LLM boundary, prefer the repair that preserves the expensive parts of the response, and never introduce a stricter failure mode for a cosmetic defect than the one already accepted for a structural one.** +- **Coerce-vs-reject at an LLM response boundary: match the policy the field already has.** Ruled #1952 as _strip_, not _reject_. Decisive argument was **blast radius per call**: one generation produces subject + body + every per-invoice description, so rejecting over two asterisks discards unrelated correct output, costs a second paid round-trip, and may fail identically on retry with the same model. Reinforcing: the validator already _truncates_ over-length values on these very fields, so adding a harsher policy for a _milder_ violation is incoherent; and the fields are human-editable in the preview, which makes repair-and-continue the right default with the human as backstop. Generalizable: **at an LLM boundary, prefer the repair that preserves the expensive parts of the response, and never introduce a stricter failure mode for a cosmetic defect than the one already accepted for a structural one.** - **When a hardening AC's real risk is false positives, weight the ACs there.** #1952's §2 (six byte-identical-passthrough guards: `Pos. 3 - Dachstuhl`, `Rechnung #2024-117`, `Beträge < 500 EUR`, lone `*`, umlauts/`ß`/`€`, and the compliant-body case) is as long as §1 (the stripping itself). Reason recorded in-issue: a strip that mangles a reference number is worse than the markup, because the reader cannot tell a character went missing. Also AC 1.8 — if stripping empties a non-empty field, keep the original. -- **"Don't repeat the literal" is not "these are the same value."** #1953's whole ruling. The UX spec directed reusing `SUBHEADER_FONT_SIZE` with the rationale *"don't hand-write `fontSize: 12` as a second copy of that constant"* — a magic-literal argument. Its design reasoning for the subject line ("bold + bumped size makes it read as a subject") is standalone and never references the running header. So the equality is **coincidental** and the fix is an **independent literal**; the architect's suggested `const LETTER_SUBJECT_FONT_SIZE = SUBHEADER_FONT_SIZE;` is explicitly ruled out because it fixes the name while preserving the coupling. **When a shared constant is challenged, read the sharing rationale for whether it argues DRY or argues semantic identity — only the latter justifies keeping the share.** -- **#1939's drift class has an inverse, and it needs filing too.** #1939 removed *two drifting copies of one value*; #1953 splits *one shared name over two values that happen to be equal*. Same symptom (an edit with a consequence the author never looked at), opposite cause. #1937 and #1938 are both open against that same running header, which is what makes the split worth doing *before* they land. +- **"Don't repeat the literal" is not "these are the same value."** #1953's whole ruling. The UX spec directed reusing `SUBHEADER_FONT_SIZE` with the rationale _"don't hand-write `fontSize: 12` as a second copy of that constant"_ — a magic-literal argument. Its design reasoning for the subject line ("bold + bumped size makes it read as a subject") is standalone and never references the running header. So the equality is **coincidental** and the fix is an **independent literal**; the architect's suggested `const LETTER_SUBJECT_FONT_SIZE = SUBHEADER_FONT_SIZE;` is explicitly ruled out because it fixes the name while preserving the coupling. **When a shared constant is challenged, read the sharing rationale for whether it argues DRY or argues semantic identity — only the latter justifies keeping the share.** +- **#1939's drift class has an inverse, and it needs filing too.** #1939 removed _two drifting copies of one value_; #1953 splits _one shared name over two values that happen to be equal_. Same symptom (an edit with a consequence the author never looked at), opposite cause. #1937 and #1938 are both open against that same running header, which is what makes the split worth doing _before_ they land. - **Record an architect's deferral trigger in the file, not only in the issue.** `letterSubject` is the first `PDF_STYLES` entry with no geometry consumer; architect set the split trigger at the **second** one, target shape `pageGeometry ← pdfStyles ← merge`. #1953 AC 3.1 puts that in the `pageGeometry.ts` module header. Same principle as #1950's "comment owns the rationale, issue owns the guard" — a trigger recorded only in a closed issue is lost. - **Amend an honest interim wiki statement, don't delete it.** #1932's PR adds "instructed but not enforced" to `API-Contract.md`; #1952 AC 4.1 says amend that bullet. Prevents the next author reading a stale "not enforced" line after enforcement lands. @@ -405,28 +422,28 @@ The "No new E2E coverage — deferred" line was already corrected (now lists Sce #1959 removed the `†`/`‡` markers **and their explanatory sentences**, replacing them with inline grey labels on the allocated amount: `(partial)`/`(Teilbetrag)` and `(less deposit)`/`(abzgl. Abschlag)`. This reverses **#1923 AC1.1, AC1.2, AC2.3, AC2.4** — which #1898 §4 had itself already been superseded by. `allocatedMarkers` gone from `types.ts`, replaced by `isSplit`/`isDepositReduced`. -**Ruling: labels accepted, sentences must come back as a report-level legend (#1965, Must Have).** The differentiator is *which* sentence is load-bearing: +**Ruling: labels accepted, sentences must come back as a report-level legend (#1965, Must Have).** The differentiator is _which_ sentence is load-bearing: - `(partial)` is nearly self-evident — the table prints **Invoice Amount** and **Allocated Amount** side by side, so the label only has to name the reason for a difference the reader can already see. - `(less deposit)` loses a **materially different claim**. The footnote said the deposit was claimed **separately** — accounted for in another submission. "less deposit" is equally consistent with the deposit never being claimed, and leaves a double-claim question open when it resurfaces. On a Verwendungsnachweis/Mittelabruf that has audit consequences. -**Generalizable: for outbound financial copy, ask what a label lets the reader *conclude*, not whether it is accurate. `(less deposit)` is true and still misleading by omission.** +**Generalizable: for outbound financial copy, ask what a label lets the reader _conclude_, not whether it is accurate. `(less deposit)` is true and still misleading by omission.** -**Why the fix is cheap (checked, load-bearing for the ruling):** the footnote *rendering channel is fully intact* and merely unused. `buildReportContent.ts` declares `footnotes: ReportContentFootnote[] = []` (L232) and returns it (L289) but never pushes; `ReportContentEditor.tsx` L445-450 renders it; `overviewPdf.ts` appends `reportContent.footnotes` **verbatim** (pinned by the `overviewPdf.test.ts` case "appends reportContent.footnotes verbatim…", which is also the only surviving reader of the two i18n keys — as fixture text). So the legend is a **producer-only change** and preview/PDF parity comes free. **Pattern: before ruling a restoration too expensive, check whether the mechanism was removed or only orphaned.** +**Why the fix is cheap (checked, load-bearing for the ruling):** the footnote _rendering channel is fully intact_ and merely unused. `buildReportContent.ts` declares `footnotes: ReportContentFootnote[] = []` (L232) and returns it (L289) but never pushes; `ReportContentEditor.tsx` L445-450 renders it; `overviewPdf.ts` appends `reportContent.footnotes` **verbatim** (pinned by the `overviewPdf.test.ts` case "appends reportContent.footnotes verbatim…", which is also the only surviving reader of the two i18n keys — as fixture text). So the legend is a **producer-only change** and preview/PDF parity comes free. **Pattern: before ruling a restoration too expensive, check whether the mechanism was removed or only orphaned.** **Legend must NOT go in the cover letter** — it is user-editable (#1932), so an editable qualification can be deleted, silently removing a material statement from a financial document. Report-level and non-editable, like the existing footnote block. -**The orphan-key cleanup was deliberately NOT filed as a deletion issue.** `splitFootnote`/`depositReducedFootnote` are *consumed* by #1965, so filing "delete these orphans" would race the legend. #1965 AC 3.2 pins retention; its Notes record deletion as the Wont-Do alternative. **Pattern: when a cleanup follow-up and a restoration follow-up target the same artifact, one issue must own both or the cleanup wins by arriving first.** +**The orphan-key cleanup was deliberately NOT filed as a deletion issue.** `splitFootnote`/`depositReducedFootnote` are _consumed_ by #1965, so filing "delete these orphans" would race the legend. #1965 AC 3.2 pins retention; its Notes record deletion as the Wont-Do alternative. **Pattern: when a cleanup follow-up and a restoration follow-up target the same artifact, one issue must own both or the cleanup wins by arriving first.** ### What was NOT an AC reversal (checked, initially framed as one) -**#1923 AC5.3 substance survives.** The area name moved from a sub-line to an inline grey suffix, but `areaText` is **still a separate row field** (`buildReportContent.ts` L196/L214) and `applyAiContent.ts` still only assigns `row.usageText` (L50) — so AI-generated usage text cannot drop the area, which was AC5.3's *stated rationale*. #1923's own Notes delegated the area sub-line's **visual treatment** to `ux-designer`. So E2E Scenario 20's rewrite is a presentation change within delegated authority. **Pattern: an AC that states its own rationale should be judged against the rationale, not the prescribed rendering — that is what the rationale is there for.** +**#1923 AC5.3 substance survives.** The area name moved from a sub-line to an inline grey suffix, but `areaText` is **still a separate row field** (`buildReportContent.ts` L196/L214) and `applyAiContent.ts` still only assigns `row.usageText` (L50) — so AI-generated usage text cannot drop the area, which was AC5.3's _stated rationale_. #1923's own Notes delegated the area sub-line's **visual treatment** to `ux-designer`. So E2E Scenario 20's rewrite is a presentation change within delegated authority. **Pattern: an AC that states its own rationale should be judged against the rationale, not the prescribed rendering — that is what the rationale is there for.** ### Stale ACs on closed/released issues — comment, don't rewrite -Both #1898 and #1923 are CLOSED + `released on @beta`. **Ruled: do not rewrite their ACs; post a dated supersession comment** naming which ACs died, by which PR, what still holds, and where the new source of truth is. Rewriting a released story's ACs falsifies the record of what was accepted and loses the design reasoning. The standing "stale AC is a real defect" rule targets ACs that a *future* implementer or UAT run will read as live spec — a supersession comment discharges that risk without destroying history. Posted on both (#1923 comment also lists the non-superseded ACs explicitly, since AC2.1/2.2/2.5/3/4 all still hold). +Both #1898 and #1923 are CLOSED + `released on @beta`. **Ruled: do not rewrite their ACs; post a dated supersession comment** naming which ACs died, by which PR, what still holds, and where the new source of truth is. Rewriting a released story's ACs falsifies the record of what was accepted and loses the design reasoning. The standing "stale AC is a real defect" rule targets ACs that a _future_ implementer or UAT run will read as live spec — a supersession comment discharges that risk without destroying history. Posted on both (#1923 comment also lists the non-superseded ACs explicitly, since AC2.1/2.2/2.5/3/4 all still hold). -Also noted on #1898: its §4 has now been superseded **twice** (#1898 → #1923 → #1959). That churn is itself the argument that the footnote *presentation* was never settled, and it supports abandoning the glyphs — which is why #1965 keeps the sentences but not the markers. +Also noted on #1898: its §4 has now been superseded **twice** (#1898 → #1923 → #1959). That churn is itself the argument that the footnote _presentation_ was never settled, and it supports abandoning the glyphs — which is why #1965 keeps the sentences but not the markers. ### Glossary: `Abschlag` APPROVED as a space-constrained short form @@ -434,12 +451,707 @@ Full ruling on **#1917** (the pending glossary-refinement pass, same place the ` `depositReducedInlineLabel` = `abzgl. Abschlag` ships as written, against the glossary-approved `Deposit → Abschlagszahlung`. Translator measured with fontkit against the real embedded `Roboto-Regular.ttf` at 8pt in the fixed 75pt `ALLOCATED_AMOUNT_WIDTH`: `" (Abschlagszahlung)"` = 72.85pt (the existing sibling badge, so it **sets the real ceiling**), `" (abzgl. Abschlag)"` = 63.95pt, `" (abzgl. Abschlagszahlung)"` = **96.07pt, overflows by ~21pt**. -**The decisive arithmetic the options list missed: option (c), "a shorter compliant string", is unavailable.** `Abschlagszahlung` alone eats 72.85 of 75pt, so **no qualifier of any length fits** — not `abzgl.`, not `ohne`, not the accounting `./.`. Keeping the full term therefore forces dropping the qualifier, collapsing this label into the *constituted*-deposit label (`(Abschlagszahlung)`, #1923 AC2.1). Those are different facts — "this row **is** a deposit" vs "this amount is **reduced because** deposits were claimed separately". **Collapsing them is a worse information loss than the abbreviation.** +**The decisive arithmetic the options list missed: option (c), "a shorter compliant string", is unavailable.** `Abschlagszahlung` alone eats 72.85 of 75pt, so **no qualifier of any length fits** — not `abzgl.`, not `ohne`, not the accounting `./.`. Keeping the full term therefore forces dropping the qualifier, collapsing this label into the _constituted_-deposit label (`(Abschlagszahlung)`, #1923 AC2.1). Those are different facts — "this row **is** a deposit" vs "this amount is **reduced because** deposits were claimed separately". **Collapsing them is a worse information loss than the abbreviation.** Option (b), widening the column, rejected on **risk not cost**: `ALLOCATED_AMOUNT_WIDTH` is #1929 geometry that took **four rounds** and real render-and-rasterize measurement, precisely because Usage was collapsing and the table overflowed the page edge. Reopening it for zero reader benefit is a bad trade. And `Abschlag`/`Abschlagszahlung` are the same concept in German construction practice (cf. `Abschlagsrechnung`); `abzgl. Abschlag` is idiomatic invoice German. No reader is misled — the bar set by the Verwendungsnachweis/Einreichung precedent. -**Why it earns a glossary entry rather than a silent exception: without one a future compliance sweep "fixes" it in good faith and silently breaks a bank-facing PDF, invisibly to the unit suite.** The entry must record the 75pt column and the measurement as the *reason*, not just list the variant. **Generalizable: a deliberate deviation from an approved term needs a recorded reason at the glossary, or the next sweep reverts it.** +**Why it earns a glossary entry rather than a silent exception: without one a future compliance sweep "fixes" it in good faith and silently breaks a bank-facing PDF, invisibly to the unit suite.** The entry must record the 75pt column and the measurement as the _reason_, not just list the variant. **Generalizable: a deliberate deviation from an approved term needs a recorded reason at the glossary, or the next sweep reverts it.** ### Glossary: `split`'s three German forms — NO entry, deliberately -`anteilig` (adj.) / `Anteil` (noun) / `Teilbetrag` (noun), each role-correct, mirroring English's own `partial`/`split`/`portion`. **Ruled: not drift, no entry.** The glossary prevents *semantic* divergence — one concept becoming two concepts. It is **not a single-surface-form registry**, and pinning one form here would force ungrammatical copy across an adjective and two nouns. Recorded on #1917 as "reject if proposed later" so a future translator does not re-escalate. Revisit only if the *English* is unified — a copy story, not a glossary one. +`anteilig` (adj.) / `Anteil` (noun) / `Teilbetrag` (noun), each role-correct, mirroring English's own `partial`/`split`/`portion`. **Ruled: not drift, no entry.** The glossary prevents _semantic_ divergence — one concept becoming two concepts. It is **not a single-surface-form registry**, and pinning one form here would force ungrammatical copy across an adjective and two nouns. Recorded on #1917 as "reject if proposed later" so a future translator does not re-escalate. Revisit only if the _English_ is unified — a copy story, not a glossary one. + +### Issues filed from the PR #1959 sweep (2026-08-03) + +All parentless, Bank Report Wizard cluster. #1959 was the user's own PR and held promotion #1958. + +| Issue | Substance | +| --------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **#1965** | Report-level, non-editable legend restoring the `(partial)` / `(less deposit)` explanatory sentences (Must Have; producer-only fix — the footnote channel is orphaned, not removed) | +| **#1966** | E2E coverage for the column toggles | +| **#1967** | `attachmentsNote` override is unreachable | +| **#1968** | Meta-suffix emitted as a single run | +| **#1969** | `testPrefix` / `authenticatedPage` fixture cleanup | +| **#1970** | Configurable auth rate limits — see [auth-rate-limits-1970.md](auth-rate-limits-1970.md) | +| **#1971** | `search-users.spec.ts` leftovers | +| **#1972** | Column-preference saves fail silently + dead `isLoaded` | + +## #1973 column visibility (2026-08-03) — user overruled my invented floor + +**Requirement**: _"If i de-select a column in the preview it shouldn't render in the pdf"_, then, when asked which columns are mandatory: _"generalize the use case i want to be able to specify an arbitrary amount of columns - only the allocated amount is a mandatory column"_. + +**My floor was rejected.** I proposed "at least one of Vendor / Invoice # must remain, on row-auditability grounds." The user rejected it as an **invented compliance rule**. Allocated Amount alone is a legal document. **Lesson, generalizable: when I flag "I don't want to invent a compliance rule" and then invent one anyway on plausible-sounding domain reasoning, that is still inventing one.** Allocated Amount's own mandatory status survived only because it rests on two _structural_ facts in this codebase (summary-row amounts and the #1959 inline labels both live in that cell), not on a domain claim. **Structural justification survives user scrutiny; purposive domain reasoning does not.** + +**The floor was accidentally protecting something real** — worth remembering as a pattern. `buildSummaryRow` puts the label in the last _leading_ cell (before the amount columns). With no leading columns visible the label had nowhere to go and totals would print as bare numbers. Removing the floor promoted that from "impossible by construction" to "must be ruled on": R2 = no total ever prints unlabelled; where no leading cell exists the label goes **in the same cell as the amount**. **When a constraint is removed, re-derive what it was silently guaranteeing — the constraint's _reason_ may have been wrong while its _effect_ was load-bearing.** + +**Corrected the coordinator's reading of "arbitrary"** (Q5). It read as "the report type's base set is not a ceiling — a claim could re-add Status." Wrong: `buildReportContent.ts:203` sets `status: isOverview ? status : null`, so there is **no status value** for claim/proof-of-funds. Re-adding it renders a column of blanks in a bank document. Ruled: **the base set IS the ceiling, for a data reason not a policy one.** "Arbitrary" frees the subset among columns the report _has_; it does not conjure unproduced data. Named the alternative explicitly (make `buildReportContent` produce status for claims) so the user can request it rather than having me decide. **Generalizable: a user's scope-widening ruling does not implicitly authorize inventing data.** + +**Q3 — legend conditional or unconditional?** Ruled **unconditional**, #1965 **blocks** #1973 (`addBlockedBy` set). Hiding Invoice Amount destroys the adjacency that was my _entire_ stated reason for accepting bare `(partial)` on PR #1959. Decisive argument against conditionality: **`(less deposit)` was already insufficient regardless of adjacency** (missing word = _separately_), so the legend block must print unconditionally anyway — conditioning the other sentence saves nothing and adds a branch whose output a reader cannot predict. AC 6.1 explicitly forbids implementing it as `if invoiceAmount hidden then legend`. Cross-link comment posted on #1965 so the raised necessity isn't lost. + +**Q4 persistence** — per-session in `ReportWizardPage` state, **not** `useColumnPreferences`. Strengthened by the ruling: 96 legal subsets with no floor means the right set varies per recipient, so a sticky per-user value is wrong more often than right _and_ wrong invisibly. Resets on use-case change (third instance of the #1943/#1946 hazard — handled up front, not filed later). + +**Q7 (new, from the ruling)** — Usage is now hideable and it is the **only elastic column** (`USAGE_WIDTH_*COL` = leftover). Ruled the _observable outcome_, left the mechanism to the architect: width never exceeds `printableWidth()` (unconditional); surplus goes to a free-form text column (Usage, else Vendor); when neither is visible **the table renders narrower than the page, left-aligned** — a 2-column numeric table stretched across 515pt looks broken in a bank document. Degenerate case = one 75pt column. + +**Geometry facts pinned** (`overviewPdf.ts` / `pageGeometry.ts`): fixed widths Vendor 45, Invoice # 63, Date 46, Status 40, InvoiceAmount 48, Allocated 75; Usage = `usableColumnWidth(n) - fixedSum`. Subsets: overview 2^6=**64**, claim 2^5=**32**, **96 total**, counts 1–7. **Hiding a column can only make Usage _wider_** → per-line char counts rise, row heights fall, so every measurement-pinned bound moves in the _safe_ direction (exception: hiding Usage itself). Noted in AC 3.6 as a mitigating fact for the implementer. + +**#1966 closed as superseded, not amended** (board Wont-Do, supersession comment with an AC→AC carry-forward table). Its Notes asserted "nothing about them should reach the generated PDF" — the deleted premise — and its AC1 asserted DOM removal only, which **would pass while the PDF still contained every column**. Amending would have left a tech-debt/test-only issue carrying a functional change and erased the record of the reversal. **Rule: when a user reverses a design decision, close the issue built on the old premise and carry its still-valid ACs forward with attribution — don't rewrite it.** + +**Sequencing: after the #1958 promotion.** Stated as my own opinion, not deferred. #1958 is green/CLEAN at 54 commits with #1959 in it; this blocks on #1965 anyway; and it generalizes the exact module that produced two real defects that day. The ruling made the surface _larger_ (no floor → degenerate single-column geometry + summary-label relocation). The shipped hint is **honest** — a stale string is cheap to reverse, a malformed bank document is not. + +### #1973 rev 3 — spec reconciliation, and a stale-body process failure + +**Process failure worth avoiding: I rewrote the #1973 body (rev 1 → rev 2) but only reported the _rulings_ in my handback, not "the body has been rewritten and the numbering changed."** The coordinator and `dev-team-lead` both then worked from a cached rev 1, and the dev-team-lead filed the 1-column floor as a _contradiction to be fixed_ when it had already been fixed. Substance never diverged; only R/AC numbers did (rev 2 reassigned R1–R8 and the AC numbers wholesale). **Rule: when amending an already-reported issue body, say "body rewritten, numbering reassigned" explicitly in the handback, and put an amendment log in the issue's Notes.** #1973 now carries one. + +**Adopted the spec's answer over my own on the summary label.** I ruled "label in the same cell as the amount"; the spec's **three-tier fallback** is better and is now R2 + AC 4.6: last visible leading column (**92**/96 subsets) → Invoice Amount if visible → **separate two-column block beneath the table** (**4** subsets: `{allocatedAmount}` and `{allocatedAmount, usage}` × 2 use cases). Verified the parity argument on disk: `ReportContentEditor.tsx:442-445` renders `content.summaryRows` as its own block independent of column visibility, so the PDF _matches_ the HTML preview exactly where in-table placement is impossible. **Tier 3 increases preview parity rather than costing it — a fallback that converges on the existing preview is strictly better than one that invents a new form.** + +**AC 3.7, the one-sided chunk-budget clamp — a real correction to my AC.** My 3.6 said "recompute `MAX_SAFE_USAGE_CHUNK_CHARS` from the subset's actual Usage width", which implies upward scaling is legitimate. It is not. Hiding columns only _widens_ Usage (chars/line 16 → up to 50), so the 650 budget gets more conservative and this change cannot breach it. **The hazard is the opposite and arrives later: a future _added_ column narrows Usage, drops the true ceiling below 650, and silently reinstates the #1929 content-loss defect.** 650 rests on a **single real-render measurement at one width**; extrapolating upward is what caused the round-3 defect. So the clamp **scales down, never up**, AC 3.7 requires a test for _both_ directions, and a comment must record the asymmetry as deliberate so it isn't "optimised" away as dead code. **Generalizable: a bound pinned by one measurement may be scaled toward safety but never away from it — and one-sided clamps need a recorded reason or they read as bugs.** + +**Verified geometry figures** (taken as computed from the spec, not re-derived): **72** of 96 subsets equal `printableWidth()` (48 overview + 24 claim); **24** render narrower (16 + 8), totals **84.00pt** (`tableOffsetsTotal(1)` 9.00 + 75) to **315.00pt** (`tableOffsetsTotal(5)` 43.00 + 272). I independently spot-checked both endpoints and the 72/24 split against the constants and they hold. + +**Q5/R6 confirmed by the coordinator** ("your Q5 correction was right and I was wrong") — the type's base set is the ceiling for the `status: isOverview ? status : null` **data** reason. Note it is **R6** in rev 2+, not R7 as in rev 1. + +### PR #2004 review — #1888 accepted, #1910 rejected on AC3 (2026-08-05) + +**#1888 shipped clean** as a step-3 helper line (AC4's second branch), not per-row accessible naming: `sourceReports.attachmentsNote` interpolating `t('sourceReports.useCase.${report.type}')`. Two checks that made it an accept rather than a "probably fine": all three `SourceReportType` values have `useCase.*` keys in **both** locales (no raw-key fallback for any report type), and the render gate `allocatedInvoices.length > 0` coincides **exactly** with the rows that render an indicator — unallocated rows render none — so there is no state with an unexplained paperclip and none with an explanation and nothing to explain. **When an AC is satisfied by a page-level helper line rather than per-item labelling, the finding to hunt is the gate: does the line appear in every state where the thing it explains is visible, and only those?** + +**#1910 AC3 failed — the "blanket-tag + partial counter-tag" antipattern.** The PR put `lang=""` on `ReportContentEditor`'s `.container` and counter-tagged only the `

    `s and the column-toggle hint `

    `. AC3 enumerates "editable-field labels, **buttons**, headings"; `EditableField`'s visible `

    ` cells nor remount reset is required by AC1-AC4, so the cheap fix is to trim +the title" — without re-reading AC1, which bolds "the corresponding `` **and every matching `**`… +asserts **both** return". They trimmed, as instructed, and the required assertion stayed missing. +**Rule: when a test title over-claims, re-read the AC before recommending the trim.** An over-claiming title +has two fixes and they are not interchangeable — trimming is only correct once you have confirmed no AC +demands the named behavior. Getting this backwards converts a MEDIUM cosmetic finding into a silently +dropped requirement, and costs an extra review round on top. + +### `page.route` does not intercept `page.request.*` + +`page.route` only sees requests from the **browser context**. `page.request.patch()` / any +`APIRequestContext` call bypasses it. So the positive control for a route guard must be +`page.evaluate(() => fetch(...))`, not `page.request.*` — my round-1 fix spec suggested +`page.request.patch()` for exactly this purpose, which would have failed and looked like a broken matcher. +The author correctly used `page.evaluate`. Ordering is deterministic without any wait: the Node-side handler +pushes before `route.continue()`, so the in-page `await fetch` cannot resolve until the capture has happened. + +### Other verified facts from this review + +- `--report-unused-disable-directives` is the cheap way to prove an `eslint-disable` is live rather than + cargo-culted — run it whenever a PR adds a suppression. +- `Detect Changes` **skips `Static Analysis` entirely** on `e2e/`+`.claude/`-only PRs, so on those PRs the + local lint policy is the only lint gate that exists at all (weaker even than the usual "CI runs no ESLint"). +- AC premise error in #1969 AC2: asks that `testPrefix` "values differ" between two tests in one file, but + the value is `E2E-` — identical within a worker despite `{ scope: 'test' }`. + Flagged to product-owner for amendment rather than designed around (cf. the AC-premise-error rule). +- AC4's own suggested rationale ("the mobile card list exposes no column toggles") is factually wrong for + `ReportContentEditor` — the card layout gates every row on the same `show()` predicate. The desktop-only + exclusion is a limitation of the `columnheader` locator under `display: none`, not an absence of toggles. + +### Round 3 (`4cf5a735`) — APPROVED + +The `` gap was fixed the right way: `getByRole('cell', { name: , exact: true })` with a +**baseline `toHaveCount(1)` before the toggle** and `toHaveCount(1)` again after re-checking. That baseline is +what makes the `toHaveCount(0)` non-vacuous — insist on it every time a test asserts an element's absence. +Verified chain: `{row.vendor}
    `s, column-toggle label text, mobile-card captions, source-info block, deposit/split notes) — server-generated report content. `ReportPdfPreview.tsx` untouched and renders an `